| Message ID | 20240619064652.18266-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-4.19] xen/arm: static-shmem: fix "gbase/pbase used uninitialized" build failure | expand |
Hi Michal, > On 19 Jun 2024, at 08:46, Michal Orzel <michal.orzel@amd.com> wrote: > > Building Xen with CONFIG_STATIC_SHM=y results in a build failure: > > arch/arm/static-shmem.c: In function 'process_shm': > arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] > 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here > 305 | paddr_t gbase, pbase, psize; > > This is because the commit cb1ddafdc573 adds a check referencing > gbase/pbase variables which were not yet assigned a value. Fix it. > > Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> Cheers Bertrand > --- > Rationale for 4.19: this patch fixes a build failure reported by CI: > https://gitlab.com/xen-project/xen/-/jobs/7131807878 > --- > xen/arch/arm/static-shmem.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index c434b96e6204..cd48d2896b7e 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > printk("%pd: static shared memory bank not found: '%s'", d, shm_id); > return -ENOENT; > } > - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > - { > - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > - d, pbase, gbase); > - return -EINVAL; > - } > > pbase = boot_shm_bank->start; > psize = boot_shm_bank->size; > @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > /* guest phys address is after host phys address */ > gbase = dt_read_paddr(cells + addr_cells, addr_cells); > > + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > + { > + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > + d, pbase, gbase); > + return -EINVAL; > + } > + > for ( i = 0; i < PFN_DOWN(psize); i++ ) > if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > { > -- > 2.25.1 >
Hi, Adding Oleksii for Release ack. Cheers Bertrand > On 19 Jun 2024, at 08:46, Michal Orzel <michal.orzel@amd.com> wrote: > > Building Xen with CONFIG_STATIC_SHM=y results in a build failure: > > arch/arm/static-shmem.c: In function 'process_shm': > arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] > 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here > 305 | paddr_t gbase, pbase, psize; > > This is because the commit cb1ddafdc573 adds a check referencing > gbase/pbase variables which were not yet assigned a value. Fix it. > > Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > Rationale for 4.19: this patch fixes a build failure reported by CI: > https://gitlab.com/xen-project/xen/-/jobs/7131807878 > --- > xen/arch/arm/static-shmem.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index c434b96e6204..cd48d2896b7e 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > printk("%pd: static shared memory bank not found: '%s'", d, shm_id); > return -ENOENT; > } > - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > - { > - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > - d, pbase, gbase); > - return -EINVAL; > - } > > pbase = boot_shm_bank->start; > psize = boot_shm_bank->size; > @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > /* guest phys address is after host phys address */ > gbase = dt_read_paddr(cells + addr_cells, addr_cells); > > + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > + { > + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > + d, pbase, gbase); > + return -EINVAL; > + } > + > for ( i = 0; i < PFN_DOWN(psize); i++ ) > if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > { > -- > 2.25.1 >
Hi, On Wed, 2024-06-19 at 09:02 +0000, Bertrand Marquis wrote: > Hi, > > Adding Oleksii for Release ack. > > Cheers > Bertrand > > > On 19 Jun 2024, at 08:46, Michal Orzel <michal.orzel@amd.com> > > wrote: > > > > Building Xen with CONFIG_STATIC_SHM=y results in a build failure: > > > > arch/arm/static-shmem.c: In function 'process_shm': > > arch/arm/static-shmem.c:327:41: error: 'gbase' may be used > > uninitialized [-Werror=maybe-uninitialized] > > 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) > > ) > > arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here > > 305 | paddr_t gbase, pbase, psize; > > > > This is because the commit cb1ddafdc573 adds a check referencing > > gbase/pbase variables which were not yet assigned a value. Fix it. > > > > Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be > > direct-mapped for direct-mapped domains") > > Signed-off-by: Michal Orzel <michal.orzel@amd.com> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > > --- > > Rationale for 4.19: this patch fixes a build failure reported by > > CI: > > https://gitlab.com/xen-project/xen/-/jobs/7131807878 > > --- > > xen/arch/arm/static-shmem.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static- > > shmem.c > > index c434b96e6204..cd48d2896b7e 100644 > > --- a/xen/arch/arm/static-shmem.c > > +++ b/xen/arch/arm/static-shmem.c > > @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, > > struct kernel_info *kinfo, > > printk("%pd: static shared memory bank not found: > > '%s'", d, shm_id); > > return -ENOENT; > > } > > - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > > - { > > - printk("%pd: physical address 0x%"PRIpaddr" and guest > > address 0x%"PRIpaddr" are not direct-mapped.\n", > > - d, pbase, gbase); > > - return -EINVAL; > > - } > > > > pbase = boot_shm_bank->start; > > psize = boot_shm_bank->size; > > @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, > > struct kernel_info *kinfo, > > /* guest phys address is after host phys address */ > > gbase = dt_read_paddr(cells + addr_cells, addr_cells); > > > > + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > > + { > > + printk("%pd: physical address 0x%"PRIpaddr" and > > guest address 0x%"PRIpaddr" are not direct-mapped.\n", > > + d, pbase, gbase); > > + return -EINVAL; > > + } > > + > > for ( i = 0; i < PFN_DOWN(psize); i++ ) > > if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > > { > > -- > > 2.25.1 > > >
Hi, On 19/06/2024 12:19, Oleksii K. wrote: > Hi, > On Wed, 2024-06-19 at 09:02 +0000, Bertrand Marquis wrote: >> Hi, >> >> Adding Oleksii for Release ack. >> >> Cheers >> Bertrand >> >>> On 19 Jun 2024, at 08:46, Michal Orzel <michal.orzel@amd.com> >>> wrote: >>> >>> Building Xen with CONFIG_STATIC_SHM=y results in a build failure: >>> >>> arch/arm/static-shmem.c: In function 'process_shm': >>> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used >>> uninitialized [-Werror=maybe-uninitialized] >>> 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) >>> ) >>> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here >>> 305 | paddr_t gbase, pbase, psize; >>> >>> This is because the commit cb1ddafdc573 adds a check referencing >>> gbase/pbase variables which were not yet assigned a value. Fix it. >>> >>> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be >>> direct-mapped for direct-mapped domains") >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> I have committed to unblock the CI. But I have some questions on the approach. I will ask them separately. Cheers,
Hi Julein, On 19/06/2024 13:37, Julien Grall wrote: > > > Hi, > > On 19/06/2024 12:19, Oleksii K. wrote: >> Hi, >> On Wed, 2024-06-19 at 09:02 +0000, Bertrand Marquis wrote: >>> Hi, >>> >>> Adding Oleksii for Release ack. >>> >>> Cheers >>> Bertrand >>> >>>> On 19 Jun 2024, at 08:46, Michal Orzel <michal.orzel@amd.com> >>>> wrote: >>>> >>>> Building Xen with CONFIG_STATIC_SHM=y results in a build failure: >>>> >>>> arch/arm/static-shmem.c: In function 'process_shm': >>>> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used >>>> uninitialized [-Werror=maybe-uninitialized] >>>> 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) >>>> ) >>>> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here >>>> 305 | paddr_t gbase, pbase, psize; >>>> >>>> This is because the commit cb1ddafdc573 adds a check referencing >>>> gbase/pbase variables which were not yet assigned a value. Fix it. >>>> >>>> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be >>>> direct-mapped for direct-mapped domains") >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > > I have committed to unblock the CI. But I have some questions on the > approach. I will ask them separately. The CI failures as seen in: https://gitlab.com/xen-project/xen/-/pipelines/1338067978 are due to 2 issues. This patch solves the first one. The other is related to Henry's xenstore series that without a corresponding Linux patch, that has been merged into mainline, causes a regression. And thus all the dom0less PV tests fail. We will need to revert the xenstore patches for now. ~Michal
Hi Michal, On 19/06/2024 07:46, Michal Orzel wrote: > Building Xen with CONFIG_STATIC_SHM=y results in a build failure: > > arch/arm/static-shmem.c: In function 'process_shm': > arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] > 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here > 305 | paddr_t gbase, pbase, psize; > > This is because the commit cb1ddafdc573 adds a check referencing > gbase/pbase variables which were not yet assigned a value. Fix it. > > Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > Rationale for 4.19: this patch fixes a build failure reported by CI: > https://gitlab.com/xen-project/xen/-/jobs/7131807878 > --- > xen/arch/arm/static-shmem.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index c434b96e6204..cd48d2896b7e 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > printk("%pd: static shared memory bank not found: '%s'", d, shm_id); > return -ENOENT; > } > - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > - { > - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > - d, pbase, gbase); > - return -EINVAL; > - } > > pbase = boot_shm_bank->start; > psize = boot_shm_bank->size; > @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > /* guest phys address is after host phys address */ > gbase = dt_read_paddr(cells + addr_cells, addr_cells); > > + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > + { > + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", > + d, pbase, gbase); > + return -EINVAL; > + } > + Before this patch, the check was globally. I guess the intention was it covers the two part of the "if". But now, you only have it in when "paddr" is specified in the DT. From a brief look at the code, I can't figure out why we don't need a similar check on the else path. Is this because it is guarantee that will be paddr == gaddr? Cheers,
Hi Julien, On 19/06/2024 13:55, Julien Grall wrote: > > > Hi Michal, > > On 19/06/2024 07:46, Michal Orzel wrote: >> Building Xen with CONFIG_STATIC_SHM=y results in a build failure: >> >> arch/arm/static-shmem.c: In function 'process_shm': >> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] >> 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here >> 305 | paddr_t gbase, pbase, psize; >> >> This is because the commit cb1ddafdc573 adds a check referencing >> gbase/pbase variables which were not yet assigned a value. Fix it. >> >> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> --- >> Rationale for 4.19: this patch fixes a build failure reported by CI: >> https://gitlab.com/xen-project/xen/-/jobs/7131807878 >> --- >> xen/arch/arm/static-shmem.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index c434b96e6204..cd48d2896b7e 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >> printk("%pd: static shared memory bank not found: '%s'", d, shm_id); >> return -ENOENT; >> } >> - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >> - { >> - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >> - d, pbase, gbase); >> - return -EINVAL; >> - } >> >> pbase = boot_shm_bank->start; >> psize = boot_shm_bank->size; >> @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >> /* guest phys address is after host phys address */ >> gbase = dt_read_paddr(cells + addr_cells, addr_cells); >> >> + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >> + { >> + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >> + d, pbase, gbase); >> + return -EINVAL; >> + } >> + > > Before this patch, the check was globally. I guess the intention was it > covers the two part of the "if". But now, you only have it in when > "paddr" is specified in the DT. > > From a brief look at the code, I can't figure out why we don't need a > similar check on the else path. Is this because it is guarantee that > will be paddr == gaddr? The reason why I added this check only in the first case is due to what doc states. It says that if a domain is 1:1, the shmem should be also 1:1 i.e. pbase == gbase. In the else case the pbase is omitted and thus a user cannot know and has no guarantee what will be the backing physical address. Thus, reading this doc makes me feel that for 1:1 guests user needs to specify pbase == gbase. ~Michal
On 19/06/2024 13:06, Michal Orzel wrote: > Hi Julien, > > On 19/06/2024 13:55, Julien Grall wrote: >> >> >> Hi Michal, >> >> On 19/06/2024 07:46, Michal Orzel wrote: >>> Building Xen with CONFIG_STATIC_SHM=y results in a build failure: >>> >>> arch/arm/static-shmem.c: In function 'process_shm': >>> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] >>> 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here >>> 305 | paddr_t gbase, pbase, psize; >>> >>> This is because the commit cb1ddafdc573 adds a check referencing >>> gbase/pbase variables which were not yet assigned a value. Fix it. >>> >>> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") >>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>> --- >>> Rationale for 4.19: this patch fixes a build failure reported by CI: >>> https://gitlab.com/xen-project/xen/-/jobs/7131807878 >>> --- >>> xen/arch/arm/static-shmem.c | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >>> index c434b96e6204..cd48d2896b7e 100644 >>> --- a/xen/arch/arm/static-shmem.c >>> +++ b/xen/arch/arm/static-shmem.c >>> @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>> printk("%pd: static shared memory bank not found: '%s'", d, shm_id); >>> return -ENOENT; >>> } >>> - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>> - { >>> - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >>> - d, pbase, gbase); >>> - return -EINVAL; >>> - } >>> >>> pbase = boot_shm_bank->start; >>> psize = boot_shm_bank->size; >>> @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>> /* guest phys address is after host phys address */ >>> gbase = dt_read_paddr(cells + addr_cells, addr_cells); >>> >>> + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>> + { >>> + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >>> + d, pbase, gbase); >>> + return -EINVAL; >>> + } >>> + >> >> Before this patch, the check was globally. I guess the intention was it >> covers the two part of the "if". But now, you only have it in when >> "paddr" is specified in the DT. >> >> From a brief look at the code, I can't figure out why we don't need a >> similar check on the else path. Is this because it is guarantee that >> will be paddr == gaddr? > The reason why I added this check only in the first case is due to what doc states. > It says that if a domain is 1:1, the shmem should be also 1:1 i.e. pbase == gbase. In the else > case the pbase is omitted and thus a user cannot know and has no guarantee what will be the backing physical address. The property "direct-map" has the following definition: "- direct-map Only available when statically allocated memory is used for the domain. An empty property to request the memory of the domain to be direct-map (guest physical address == physical address). " So I think it would be fair for someone to interpret it as shared memory would also be 1:1 mapped. > Thus, reading this doc makes me feel that for 1:1 guests user needs to specify pbase == gbase. See above, I think this is not 100% clear. I am concerned that someone may try to use the version where only the guest address is specified. It would likely be hard to realize that the extended regions would not work properly. So something needs to be done. I don't have any preference on how to address. It could simply be a check in Xen to request that both "gaddr" and "paddr" are specified for direct mapped domain. Cheers,
On 19/06/2024 14:34, Julien Grall wrote: > > > On 19/06/2024 13:06, Michal Orzel wrote: >> Hi Julien, >> >> On 19/06/2024 13:55, Julien Grall wrote: >>> >>> >>> Hi Michal, >>> >>> On 19/06/2024 07:46, Michal Orzel wrote: >>>> Building Xen with CONFIG_STATIC_SHM=y results in a build failure: >>>> >>>> arch/arm/static-shmem.c: In function 'process_shm': >>>> arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] >>>> 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>>> arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here >>>> 305 | paddr_t gbase, pbase, psize; >>>> >>>> This is because the commit cb1ddafdc573 adds a check referencing >>>> gbase/pbase variables which were not yet assigned a value. Fix it. >>>> >>>> Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") >>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >>>> --- >>>> Rationale for 4.19: this patch fixes a build failure reported by CI: >>>> https://gitlab.com/xen-project/xen/-/jobs/7131807878 >>>> --- >>>> xen/arch/arm/static-shmem.c | 13 +++++++------ >>>> 1 file changed, 7 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >>>> index c434b96e6204..cd48d2896b7e 100644 >>>> --- a/xen/arch/arm/static-shmem.c >>>> +++ b/xen/arch/arm/static-shmem.c >>>> @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>> printk("%pd: static shared memory bank not found: '%s'", d, shm_id); >>>> return -ENOENT; >>>> } >>>> - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>>> - { >>>> - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >>>> - d, pbase, gbase); >>>> - return -EINVAL; >>>> - } >>>> >>>> pbase = boot_shm_bank->start; >>>> psize = boot_shm_bank->size; >>>> @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>> /* guest phys address is after host phys address */ >>>> gbase = dt_read_paddr(cells + addr_cells, addr_cells); >>>> >>>> + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >>>> + { >>>> + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", >>>> + d, pbase, gbase); >>>> + return -EINVAL; >>>> + } >>>> + >>> >>> Before this patch, the check was globally. I guess the intention was it >>> covers the two part of the "if". But now, you only have it in when >>> "paddr" is specified in the DT. >>> >>> From a brief look at the code, I can't figure out why we don't need a >>> similar check on the else path. Is this because it is guarantee that >>> will be paddr == gaddr? >> The reason why I added this check only in the first case is due to what doc states. >> It says that if a domain is 1:1, the shmem should be also 1:1 i.e. pbase == gbase. In the else >> case the pbase is omitted and thus a user cannot know and has no guarantee what will be the backing physical address. > > The property "direct-map" has the following definition: > > "- direct-map > > Only available when statically allocated memory is used for the domain. > An empty property to request the memory of the domain to be > direct-map (guest physical address == physical address). > " > > So I think it would be fair for someone to interpret it as shared memory > would also be 1:1 mapped. > >> Thus, reading this doc makes me feel that for 1:1 guests user needs to specify pbase == gbase. > > See above, I think this is not 100% clear. I am concerned that someone > may try to use the version where only the guest address is specified. > > It would likely be hard to realize that the extended regions would not > work properly. So something needs to be done. > > I don't have any preference on how to address. It could simply be a > check in Xen to request that both "gaddr" and "paddr" are specified for > direct mapped domain. Fair enough. I can add a check for 1:1 in the else case to return error with a message that host and guest physical address must be supplied for direct-mapped domains. Would we consider it for 4.19? In my opinion yes as it would remove the possibility of a feature misuse. ~Michal
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index c434b96e6204..cd48d2896b7e 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -324,12 +324,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, printk("%pd: static shared memory bank not found: '%s'", d, shm_id); return -ENOENT; } - if ( is_domain_direct_mapped(d) && (pbase != gbase) ) - { - printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", - d, pbase, gbase); - return -EINVAL; - } pbase = boot_shm_bank->start; psize = boot_shm_bank->size; @@ -353,6 +347,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, /* guest phys address is after host phys address */ gbase = dt_read_paddr(cells + addr_cells, addr_cells); + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) + { + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not direct-mapped.\n", + d, pbase, gbase); + return -EINVAL; + } + for ( i = 0; i < PFN_DOWN(psize); i++ ) if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) {
Building Xen with CONFIG_STATIC_SHM=y results in a build failure: arch/arm/static-shmem.c: In function 'process_shm': arch/arm/static-shmem.c:327:41: error: 'gbase' may be used uninitialized [-Werror=maybe-uninitialized] 327 | if ( is_domain_direct_mapped(d) && (pbase != gbase) ) arch/arm/static-shmem.c:305:17: note: 'gbase' was declared here 305 | paddr_t gbase, pbase, psize; This is because the commit cb1ddafdc573 adds a check referencing gbase/pbase variables which were not yet assigned a value. Fix it. Fixes: cb1ddafdc573 ("xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains") Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- Rationale for 4.19: this patch fixes a build failure reported by CI: https://gitlab.com/xen-project/xen/-/jobs/7131807878 --- xen/arch/arm/static-shmem.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)