| Message ID | 20231016124559.8220-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.18] xen/arm: Check return code from recursive calls to scan_pfdt_node() | expand |
> On 16 Oct 2023, at 13:45, Michal Orzel <michal.orzel@amd.com> wrote: > > At the moment, we do not check a return code from scan_pfdt_node() > called recursively. This means that any issue that may occur while > parsing and copying the passthrough nodes is hidden and Xen continues > to boot a domain despite errors. This may lead to incorrect device tree > generation and various guest issues (e.g. trap on attempt to access MMIO > not mapped in P2M). Fix it. > > Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Hi Michal, Yes makes sense! Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> > --- > @Henry: > This is a bug fix, so I think we should have it in 4.18 given the possible > consequences I described in the commit msg. I don't see any risks as this change > only checks the return code for an error. > --- > xen/arch/arm/domain_build.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 24c9019cc43c..49792dd590ee 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, > node_next = fdt_first_subnode(pfdt, nodeoff); > while ( node_next > 0 ) > { > - scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, > - scan_passthrough_prop); > + rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, > + scan_passthrough_prop); > + if ( rc ) > + return rc; > + > node_next = fdt_next_subnode(pfdt, node_next); > } > > -- > 2.25.1 > >
Hi Michal, > On 16 Oct 2023, at 14:45, Michal Orzel <michal.orzel@amd.com> wrote: > > At the moment, we do not check a return code from scan_pfdt_node() > called recursively. This means that any issue that may occur while > parsing and copying the passthrough nodes is hidden and Xen continues > to boot a domain despite errors. This may lead to incorrect device tree > generation and various guest issues (e.g. trap on attempt to access MMIO > not mapped in P2M). Fix it. > > Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Good finding :-) Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > @Henry: > This is a bug fix, so I think we should have it in 4.18 given the possible > consequences I described in the commit msg. I don't see any risks as this change > only checks the return code for an error. > --- > xen/arch/arm/domain_build.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 24c9019cc43c..49792dd590ee 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, > node_next = fdt_first_subnode(pfdt, nodeoff); > while ( node_next > 0 ) > { > - scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, > - scan_passthrough_prop); > + rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, > + scan_passthrough_prop); > + if ( rc ) > + return rc; > + > node_next = fdt_next_subnode(pfdt, node_next); > } > > -- > 2.25.1 >
Hi Michal, > On Oct 16, 2023, at 21:28, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote: > > Hi Michal, > >> On 16 Oct 2023, at 14:45, Michal Orzel <michal.orzel@amd.com> wrote: >> >> At the moment, we do not check a return code from scan_pfdt_node() >> called recursively. This means that any issue that may occur while >> parsing and copying the passthrough nodes is hidden and Xen continues >> to boot a domain despite errors. This may lead to incorrect device tree >> generation and various guest issues (e.g. trap on attempt to access MMIO >> not mapped in P2M). Fix it. >> >> Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb") >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> > > Good finding :-) > > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry > > Cheers > Bertrand > >> --- >> @Henry: >> This is a bug fix, so I think we should have it in 4.18 given the possible >> consequences I described in the commit msg. I don't see any risks as this change >> only checks the return code for an error. >> --- >> xen/arch/arm/domain_build.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 24c9019cc43c..49792dd590ee 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, >> node_next = fdt_first_subnode(pfdt, nodeoff); >> while ( node_next > 0 ) >> { >> - scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, >> - scan_passthrough_prop); >> + rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, >> + scan_passthrough_prop); >> + if ( rc ) >> + return rc; >> + >> node_next = fdt_next_subnode(pfdt, node_next); >> } >> >> -- >> 2.25.1 >> >
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 24c9019cc43c..49792dd590ee 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2872,8 +2872,11 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, const void *pfdt, node_next = fdt_first_subnode(pfdt, nodeoff); while ( node_next > 0 ) { - scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, - scan_passthrough_prop); + rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells, + scan_passthrough_prop); + if ( rc ) + return rc; + node_next = fdt_next_subnode(pfdt, node_next); }
At the moment, we do not check a return code from scan_pfdt_node() called recursively. This means that any issue that may occur while parsing and copying the passthrough nodes is hidden and Xen continues to boot a domain despite errors. This may lead to incorrect device tree generation and various guest issues (e.g. trap on attempt to access MMIO not mapped in P2M). Fix it. Fixes: 669ecdf8d6cd ("xen/arm: copy dtb fragment to guest dtb") Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- @Henry: This is a bug fix, so I think we should have it in 4.18 given the possible consequences I described in the commit msg. I don't see any risks as this change only checks the return code for an error. --- xen/arch/arm/domain_build.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)