> On 22 Mar 2024, at 15:09, Michal Orzel <michal.or...@amd.com> wrote:
>
> Hi Luca,
>
> NIT: title s/for/from?
>
> On 12/03/2024 14:03, Luca Fancellu wrote:
>>
>>
>> From: Penny Zheng <penny.zh...@arm.com>
>>
>> Static shared memory acts as reserved memory in guest, so it shall be
>> excluded from extended regions.
>>
>> Extended regions are taken care of under three different scenarios:
>> normal DomU, direct-map domain with iommu on, and direct-map domain
>> with iommu off.
>>
>> For normal DomU, we create a new function "remove_shm_holes_for_domU",
>> to firstly transfer original outputs into the format of
>> "struct rangeset", then use "remove_shm_from_rangeset" to remove static
>> shm from them.
>>
>> For direct-map domain with iommu on, after we get guest shm info from
>> "kinfo",
>> we use "remove_shm_from_rangeset" to remove static shm.
>>
>> For direct-map domain with iommu off, as static shm has already been taken
>> care of through reserved memory banks, we do nothing.
>>
>> Signed-off-by: Penny Zheng <penny.zh...@arm.com>
>> Signed-off-by: Luca Fancellu <luca.fance...@arm.com>
>> ---
>> v1:
>> - Rework of
>> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-penny.zh...@arm.com/
>> ---
>> xen/arch/arm/domain_build.c | 16 ++++-
>> xen/arch/arm/include/asm/domain_build.h | 2 +
>> xen/arch/arm/include/asm/static-shmem.h | 18 ++++++
>> xen/arch/arm/static-shmem.c | 86 +++++++++++++++++++++++++
>> 4 files changed, 119 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 9fad9e8b2c40..740c483ea2db 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -817,8 +817,8 @@ int __init make_memory_node(const struct domain *d,
>> return res;
>> }
>>
>> -static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>> - void *data)
>> +int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>> + void *data)
>> {
>> struct membanks *ext_regions = data;
>> paddr_t start, size;
>> @@ -969,6 +969,8 @@ static int __init handle_pci_range(const struct
>> dt_device_node *dev,
>> * - MMIO
>> * - Host RAM
>> * - PCI aperture
>> + * - Static shared memory regions, which are described by special property
>> + * "xen,domain-shared-memory-v1"
> I'm not sure if providing a compatible here makes sense. If at all, I would
> put "xen,shared-mem" which holds the addresses.
>
>> */
>> static int __init find_memory_holes(const struct kernel_info *kinfo,
>> struct membanks *ext_regions)
>> @@ -985,6 +987,11 @@ static int __init find_memory_holes(const struct
>> kernel_info *kinfo,
>> if ( !mem_holes )
>> return -ENOMEM;
>>
>> + /* Remove static shared memory regions */
>> + res = remove_shm_from_rangeset(kinfo, mem_holes);
>> + if ( res )
>> + goto out;
> How can you remove from a rangeset without first adding to it?
> This should be moved after rangeset_add_range().
You are very right
> Also, usually (and this is the case in this function) we pass frames
> to rangeset and not addresses (argument is of type ul). However...
>
>> +
>> /* Start with maximum possible addressable physical memory range */
>> start = 0;
>> end = (1ULL << p2m_ipa_bits) - 1;
>> @@ -1089,7 +1096,10 @@ static int __init find_domU_holes(const struct
>> kernel_info *kinfo,
>> res = 0;
>> }
>>
>> - return res;
>> + if ( res )
>> + return res;
>> +
>> + return remove_shm_holes_for_domU(kinfo, ext_regions);
>> }
>>
>> int __init make_hypervisor_node(struct domain *d,
>> diff --git a/xen/arch/arm/include/asm/domain_build.h
>> b/xen/arch/arm/include/asm/domain_build.h
>> index a6f276cc4263..026d975da28e 100644
>> --- a/xen/arch/arm/include/asm/domain_build.h
>> +++ b/xen/arch/arm/include/asm/domain_build.h
>> @@ -51,6 +51,8 @@ static inline int prepare_acpi(struct domain *d, struct
>> kernel_info *kinfo)
>> int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
>> #endif
>>
>> +int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
>> +
>> #endif
>>
>> /*
>> diff --git a/xen/arch/arm/include/asm/static-shmem.h
>> b/xen/arch/arm/include/asm/static-shmem.h
>> index c6fac9906656..2f70aed53ac7 100644
>> --- a/xen/arch/arm/include/asm/static-shmem.h
>> +++ b/xen/arch/arm/include/asm/static-shmem.h
>> @@ -29,6 +29,12 @@ void early_print_info_shmem(void);
>>
>> void init_sharedmem_pages(void);
>>
>> +int remove_shm_from_rangeset(const struct kernel_info *kinfo,
>> + struct rangeset *rangeset);
>> +
>> +int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>> + struct membanks *ext_regions);
>> +
>> #else /* !CONFIG_STATIC_SHM */
>>
>> static inline int make_resv_memory_node(const struct domain *d,
>> @@ -61,6 +67,18 @@ static inline void early_print_info_shmem(void) {};
>>
>> static inline void init_sharedmem_pages(void) {};
>>
>> +static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
>> + struct rangeset *rangeset)
>> +{
>> + return 0;
>> +}
>> +
>> +static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>> + struct membanks *ext_regions)
>> +{
>> + return 0;
>> +}
>> +
>> #endif /* CONFIG_STATIC_SHM */
>>
>> #endif /* __ASM_STATIC_SHMEM_H_ */
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index 6143f52cb991..b3e2105dd3f2 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>>
>> #include <xen/libfdt/libfdt.h>
>> +#include <xen/rangeset.h>
>> #include <xen/sched.h>
>>
>> #include <asm/domain_build.h>
>> @@ -564,6 +565,91 @@ void __init init_sharedmem_pages(void)
>> init_staticmem_bank(&shmem->bank[bank]);
>> }
>>
>> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>> + struct rangeset *rangeset)
>> +{
>> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
>> + unsigned int i;
>> +
>> + /* Remove static shared memory regions */
>> + for ( i = 0; i < shm_mem->nr_banks; i++ )
>> + {
>> + paddr_t start, end;
> ... here, these hold physical addresses and...
>
>> + int res;
>> +
>> + start = shm_mem->bank[i].start;
>> + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
>> + res = rangeset_remove_range(rangeset, start, end);
> you will end up in a mix which won't work. Switch to PFN_DOWN()
Will fix
>
>> + if ( res )
> What's the point of res variable if it is not printed below?
I guess this part was copied from find_memory_holes which doesn’t print out res,
anyway you are right, we should print it.
>
>> + {
>> + printk(XENLOG_ERR "Failed to remove:
>> %#"PRIpaddr"->%#"PRIpaddr"\n",
>> + start, end);
>> + return -EINVAL;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
>> + struct membanks *ext_regions)
>> +{
>> + const struct membanks *shm_mem = &kinfo->shm_mem.common;
>> + struct rangeset *guest_holes;
>> + unsigned int i;
>> + paddr_t start;
>> + paddr_t end;
>> + int res;
>> +
>> + /* No static shared memory region. */
>> + if ( shm_mem->nr_banks == 0 )
>> + return 0;
>> +
>> + dt_dprintk("Remove static shared memory holes for extended regions of
>> DomU\n");
>> +
>> + guest_holes = rangeset_new(NULL, NULL, 0);
>> + if ( !guest_holes )
>> + return -ENOMEM;
>> +
>> + /* Copy extended regions sets into the rangeset */
>> + for ( i = 0; i < ext_regions->nr_banks; i++ )
>> + {
>> + start = ext_regions->bank[i].start;
>> + end = start + ext_regions->bank[i].size - 1;
>> +
>> + res = rangeset_add_range(guest_holes, start, end);
> Ditto, PFN_DOWN().
Thanks for the review, I will fix the issues in the next push, I’ll just wait
for the serie to be fully reviewed, or if you don’t want to check the remaining
two patches before I fix the previous comments, just let me know.
>
> ~Michal
