Re: [PATCH v3 1/4] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>>> 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