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(+)

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)) )
-- 
2.34.1

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

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.

> 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