| Message ID | 20240517032156.1490515-2-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Guest XenStore page allocation for 11 Dom0less domUs | expand |
Hi Henry, +CC: Luca On 17/05/2024 05:21, Henry Wang wrote: > > > Currently, users are allowed to map static shared memory in a > non-direct-mapped way for direct-mapped domains. This can lead to > clashing of guest memory spaces. Also, the current extended region > finding logic only removes the host physical addresses of the > static shared memory areas for direct-mapped domains, which may be > inconsistent with the guest memory map if users map the static > shared memory in a non-direct-mapped way. This will lead to incorrect > extended region calculation results. > > To make things easier, add restriction that static shared memory > should also be direct-mapped for direct-mapped domains. Check the > host physical address to be matched with guest physical address when > parsing the device tree. Document this restriction in the doc. I'm ok with this restriction. @Luca, do you have any use case preventing us from making this restriction? This patch clashes with Luca series so depending on which goes first, Acked-by: Michal Orzel <michal.orzel@amd.com> > > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > v3: > - New patch. > --- > docs/misc/arm/device-tree/booting.txt | 3 +++ > xen/arch/arm/static-shmem.c | 6 ++++++ > 2 files changed, 9 insertions(+) > > diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt > index bbd955e9c2..c994e48391 100644 > --- a/docs/misc/arm/device-tree/booting.txt > +++ b/docs/misc/arm/device-tree/booting.txt > @@ -591,6 +591,9 @@ communication. > shared memory region in host physical address space, a size, and a guest > physical address, as the target address of the mapping. > e.g. xen,shared-mem = < [host physical address] [guest address] [size] > > + Note that if a domain is direct-mapped, i.e. the Dom0 and the Dom0less > + DomUs with `direct-map` device tree property, the static shared memory > + should also be direct-mapped (host physical address == guest address). > > It shall also meet the following criteria: > 1) If the SHM ID matches with an existing region, the address range of the > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index 78881dd1d3..b26fb69874 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -235,6 +235,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > d, psize); > return -EINVAL; > } > + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) > + { > + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not 1:1 direct-mapped.\n", NIT: 1:1 and direct-mapped means the same so no need to place them next to each other > + d, pbase, gbase); > + return -EINVAL; > + } > > for ( i = 0; i < PFN_DOWN(psize); i++ ) > if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > -- > 2.34.1 > > ~Michal
> On 20 May 2024, at 12:24, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Henry, > > +CC: Luca > > On 17/05/2024 05:21, Henry Wang wrote: >> >> >> Currently, users are allowed to map static shared memory in a >> non-direct-mapped way for direct-mapped domains. This can lead to >> clashing of guest memory spaces. Also, the current extended region >> finding logic only removes the host physical addresses of the >> static shared memory areas for direct-mapped domains, which may be >> inconsistent with the guest memory map if users map the static >> shared memory in a non-direct-mapped way. This will lead to incorrect >> extended region calculation results. >> >> To make things easier, add restriction that static shared memory >> should also be direct-mapped for direct-mapped domains. Check the >> host physical address to be matched with guest physical address when >> parsing the device tree. Document this restriction in the doc. > I'm ok with this restriction. > > @Luca, do you have any use case preventing us from making this restriction? Hi Michal, Henry, I think it’s sensible, I don’t think we have any use case for direct-mapped domains using non direct mapped static shared memory. Cheers, Luca
Hi Michal, Luca, On 5/20/2024 7:24 PM, Michal Orzel wrote: > Hi Henry, > > +CC: Luca > > On 17/05/2024 05:21, Henry Wang wrote: >> To make things easier, add restriction that static shared memory >> should also be direct-mapped for direct-mapped domains. Check the >> host physical address to be matched with guest physical address when >> parsing the device tree. Document this restriction in the doc. > I'm ok with this restriction. > > @Luca, do you have any use case preventing us from making this restriction? > > This patch clashes with Luca series so depending on which goes first, I agree that there will be some conflicts between the two series. To avoid back and forth, if Luca's series goes in first, would it be ok for you if I place the same check from this patch in handle_shared_mem_bank() like below? diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 9c3a83042d..2d23fa4917 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -219,6 +219,13 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, pbase = shm_bank->start; psize = shm_bank->size; + 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; + } + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n", d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize); > Acked-by: Michal Orzel <michal.orzel@amd.com> Thanks. I will take the tag if you are ok with above diff (for the case if this series goes in later than Luca's). >> } >> + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) >> + { >> + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not 1:1 direct-mapped.\n", > NIT: 1:1 and direct-mapped means the same so no need to place them next to each other Ok. I will drop the "1:1" in the next version. Thanks. Kind regards, Henry > ~Michal
Hi Henry, On 20/05/2024 16:52, Henry Wang wrote: > Hi Michal, Luca, > > On 5/20/2024 7:24 PM, Michal Orzel wrote: >> Hi Henry, >> >> +CC: Luca >> >> On 17/05/2024 05:21, Henry Wang wrote: >>> To make things easier, add restriction that static shared memory >>> should also be direct-mapped for direct-mapped domains. Check the >>> host physical address to be matched with guest physical address when >>> parsing the device tree. Document this restriction in the doc. >> I'm ok with this restriction. >> >> @Luca, do you have any use case preventing us from making this restriction? >> >> This patch clashes with Luca series so depending on which goes first, > > I agree that there will be some conflicts between the two series. To > avoid back and forth, if Luca's series goes in first, would it be ok for > you if I place the same check from this patch in > handle_shared_mem_bank() like below? > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index 9c3a83042d..2d23fa4917 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -219,6 +219,13 @@ static int __init handle_shared_mem_bank(struct > domain *d, paddr_t gbase, > pbase = shm_bank->start; > psize = shm_bank->size; > > + 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; > + } > + > printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys > 0x%"PRIpaddr", size 0x%"PRIpaddr"\n", > d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize); > >> Acked-by: Michal Orzel <michal.orzel@amd.com> > > Thanks. I will take the tag if you are ok with above diff (for the case > if this series goes in later than Luca's). I would move this check to process_shm() right after "gbase = dt_read_paddr" setting. This would be the most natural placement for such a check. ~Michal
Hi Michal, On 5/20/2024 11:46 PM, Michal Orzel wrote: > Hi Henry, > > On 20/05/2024 16:52, Henry Wang wrote: >> Hi Michal, Luca, >> >> On 5/20/2024 7:24 PM, Michal Orzel wrote: >>> Hi Henry, >>> >>> +CC: Luca >>> >>> On 17/05/2024 05:21, Henry Wang wrote: >>>> To make things easier, add restriction that static shared memory >>>> should also be direct-mapped for direct-mapped domains. Check the >>>> host physical address to be matched with guest physical address when >>>> parsing the device tree. Document this restriction in the doc. >>> I'm ok with this restriction. >>> >>> @Luca, do you have any use case preventing us from making this restriction? >>> >>> This patch clashes with Luca series so depending on which goes first, >> I agree that there will be some conflicts between the two series. To >> avoid back and forth, if Luca's series goes in first, would it be ok for >> you if I place the same check from this patch in >> handle_shared_mem_bank() like below? >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index 9c3a83042d..2d23fa4917 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -219,6 +219,13 @@ static int __init handle_shared_mem_bank(struct >> domain *d, paddr_t gbase, >> pbase = shm_bank->start; >> psize = shm_bank->size; >> >> + 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; >> + } >> + >> printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys >> 0x%"PRIpaddr", size 0x%"PRIpaddr"\n", >> d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize); >> >>> Acked-by: Michal Orzel <michal.orzel@amd.com> >> Thanks. I will take the tag if you are ok with above diff (for the case >> if this series goes in later than Luca's). > I would move this check to process_shm() right after "gbase = dt_read_paddr" setting. > This would be the most natural placement for such a check. That sounds good. Thanks! IIUC we only need to add the check for the pbase != INVALID_PADDR case correct? Kind regards, Henry > ~Michal
On 20/05/2024 17:51, Henry Wang wrote: > Hi Michal, > > On 5/20/2024 11:46 PM, Michal Orzel wrote: >> Hi Henry, >> >> On 20/05/2024 16:52, Henry Wang wrote: >>> Hi Michal, Luca, >>> >>> On 5/20/2024 7:24 PM, Michal Orzel wrote: >>>> Hi Henry, >>>> >>>> +CC: Luca >>>> >>>> On 17/05/2024 05:21, Henry Wang wrote: >>>>> To make things easier, add restriction that static shared memory >>>>> should also be direct-mapped for direct-mapped domains. Check the >>>>> host physical address to be matched with guest physical address when >>>>> parsing the device tree. Document this restriction in the doc. >>>> I'm ok with this restriction. >>>> >>>> @Luca, do you have any use case preventing us from making this restriction? >>>> >>>> This patch clashes with Luca series so depending on which goes first, >>> I agree that there will be some conflicts between the two series. To >>> avoid back and forth, if Luca's series goes in first, would it be ok for >>> you if I place the same check from this patch in >>> handle_shared_mem_bank() like below? >>> >>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >>> index 9c3a83042d..2d23fa4917 100644 >>> --- a/xen/arch/arm/static-shmem.c >>> +++ b/xen/arch/arm/static-shmem.c >>> @@ -219,6 +219,13 @@ static int __init handle_shared_mem_bank(struct >>> domain *d, paddr_t gbase, >>> pbase = shm_bank->start; >>> psize = shm_bank->size; >>> >>> + 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; >>> + } >>> + >>> printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys >>> 0x%"PRIpaddr", size 0x%"PRIpaddr"\n", >>> d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize); >>> >>>> Acked-by: Michal Orzel <michal.orzel@amd.com> >>> Thanks. I will take the tag if you are ok with above diff (for the case >>> if this series goes in later than Luca's). >> I would move this check to process_shm() right after "gbase = dt_read_paddr" setting. >> This would be the most natural placement for such a check. > > That sounds good. Thanks! IIUC we only need to add the check for the > pbase != INVALID_PADDR case correct? Yes, but at the same time I wonder whether we should also return error if a user omits pbase for direct mapped domain. ~Michal
Hi Michal, On 5/21/2024 12:09 AM, Michal Orzel wrote: >>>> Thanks. I will take the tag if you are ok with above diff (for the case >>>> if this series goes in later than Luca's). >>> I would move this check to process_shm() right after "gbase = dt_read_paddr" setting. >>> This would be the most natural placement for such a check. >> That sounds good. Thanks! IIUC we only need to add the check for the >> pbase != INVALID_PADDR case correct? > Yes, but at the same time I wonder whether we should also return error if a user omits pbase > for direct mapped domain. I think this makes sense. So I will add also a check for the case if users omit pbase in the device tree for the direct mapped domain. Kind regards, Henry > > ~Michal
On Tue, 21 May 2024, Henry Wang wrote: > Hi Michal, > > On 5/21/2024 12:09 AM, Michal Orzel wrote: > > > > > Thanks. I will take the tag if you are ok with above diff (for the > > > > > case > > > > > if this series goes in later than Luca's). > > > > I would move this check to process_shm() right after "gbase = > > > > dt_read_paddr" setting. > > > > This would be the most natural placement for such a check. > > > That sounds good. Thanks! IIUC we only need to add the check for the > > > pbase != INVALID_PADDR case correct? > > Yes, but at the same time I wonder whether we should also return error if a > > user omits pbase > > for direct mapped domain. > > I think this makes sense. So I will add also a check for the case if users > omit pbase in the device tree for the direct mapped domain. I fixed the NIT and added the ack, but as Luca's series hasn't been committed yet, I have not made this change. I'll leave it to Julien when he commits both series.
diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2..c994e48391 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -591,6 +591,9 @@ communication. shared memory region in host physical address space, a size, and a guest physical address, as the target address of the mapping. e.g. xen,shared-mem = < [host physical address] [guest address] [size] > + Note that if a domain is direct-mapped, i.e. the Dom0 and the Dom0less + DomUs with `direct-map` device tree property, the static shared memory + should also be direct-mapped (host physical address == guest address). It shall also meet the following criteria: 1) If the SHM ID matches with an existing region, the address range of the diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 78881dd1d3..b26fb69874 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -235,6 +235,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, d, psize); return -EINVAL; } + if ( is_domain_direct_mapped(d) && (pbase != gbase) ) + { + printk("%pd: physical address 0x%"PRIpaddr" and guest address 0x%"PRIpaddr" are not 1:1 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)) )
Currently, users are allowed to map static shared memory in a non-direct-mapped way for direct-mapped domains. This can lead to clashing of guest memory spaces. Also, the current extended region finding logic only removes the host physical addresses of the static shared memory areas for direct-mapped domains, which may be inconsistent with the guest memory map if users map the static shared memory in a non-direct-mapped way. This will lead to incorrect extended region calculation results. To make things easier, add restriction that static shared memory should also be direct-mapped for direct-mapped domains. Check the host physical address to be matched with guest physical address when parsing the device tree. Document this restriction in the doc. Signed-off-by: Henry Wang <xin.wang2@amd.com> --- v3: - New patch. --- docs/misc/arm/device-tree/booting.txt | 3 +++ xen/arch/arm/static-shmem.c | 6 ++++++ 2 files changed, 9 insertions(+)