Re: [PATCH v4 0/7] Static shared memory followup v2 - pt2
Hi, On 11/06/2024 14:35, Luca Fancellu wrote: > > > + Oleksii > >> On 24 May 2024, at 13:40, Luca Fancellu wrote: >> >> This serie is a partial rework of this other serie: >> https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ >> >> The original serie is addressing an issue of the static shared memory feature >> that impacts the memory footprint of other component when the feature is >> enabled, another issue impacts the device tree generation for the guests when >> the feature is enabled and used and the last one is a missing feature that is >> the option to have a static shared memory region that is not from the host >> address space. >> >> This serie is handling some comment on the original serie and it is splitting >> the rework in two part, this first part is addressing the memory footprint >> issue >> and the device tree generation and currently is fully merged >> (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), >> this serie is addressing the static shared memory allocation from the Xen >> heap. >> >> Luca Fancellu (5): >> xen/arm: Lookup bootinfo shm bank during the mapping >> xen/arm: Wrap shared memory mapping code in one function >> xen/arm: Parse xen,shared-mem when host phys address is not provided >> xen/arm: Rework heap page allocation outside allocate_bank_memory >> xen/arm: Implement the logic for static shared memory from Xen heap >> >> Penny Zheng (2): >> xen/p2m: put reference for level 2 superpage >> xen/docs: Describe static shared memory when host address is not >>provided >> >> docs/misc/arm/device-tree/booting.txt | 52 ++- >> xen/arch/arm/arm32/mmu/mm.c | 11 +- >> xen/arch/arm/dom0less-build.c | 4 +- >> xen/arch/arm/domain_build.c | 84 +++-- >> xen/arch/arm/include/asm/domain_build.h | 9 +- >> xen/arch/arm/mmu/p2m.c | 82 +++-- >> xen/arch/arm/setup.c| 14 +- >> xen/arch/arm/static-shmem.c | 432 +--- >> 8 files changed, 502 insertions(+), 186 deletions(-) >> >> -- >> 2.34.1 >> >> > > Hi, > > We would like this serie to be in Xen 4.19, there was a misunderstanding on > our side because we thought > that since the serie was sent before the last posting date, it could have > been a candidate for merging in the > new release, instead after speaking with Julien and Oleksii we are now aware > that we need to provide a > justification for its presence. > > Pros to this serie is that we are closing the circle for static shared > memory, allowing it to use memory from > the host or from Xen, it is also a feature that is not enabled by default, so > it should not cause too much > disruption in case of any bugs that escaped the review, however we’ve tested > many configurations for that > with/without enabling the feature if that can be an additional value. > > Cons: we are touching some common code related to p2m, but also there the > impact should be minimal because > the new code is subject to l2 foreign mapping (to be confirmed maybe from a > p2m expert like Julien). > > The comments on patch 3 of this serie are addressed by this patch: > https://patchwork.kernel.org/project/xen-devel/patch/20240528125603.2467640-1-luca.fance...@arm.com/ > And the serie is fully reviewed. > > So our request is to allow this serie in 4.19, Oleksii, ARM maintainers, do > you agree on that? As a main reviewer of this series I'm ok to have this series in. It is nicely encapsulated and the feature itself is still in unsupported state. I don't foresee any issues with it. ~Michal
Re: [PATCH 2/3] CI: Use a debug build of Xen for the Xilinx HW tests
Hi Andrew,
On 29/05/2024 16:19, Andrew Cooper wrote:
>
>
> ... like the other hardware tests. This gets more value out of the testing.
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Roger Pau Monné
> CC: Stefano Stabellini
> CC: Michal Orzel
> CC: Marek Marczykowski-Górecki
> CC: Oleksii Kurochko
> ---
> automation/gitlab-ci/test.yaml | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
> index efd3ad46f08e..e96ccdfad54c 100644
> --- a/automation/gitlab-ci/test.yaml
> +++ b/automation/gitlab-ci/test.yaml
> @@ -149,7 +149,7 @@ xilinx-smoke-dom0less-arm64-gcc:
> - ./automation/scripts/xilinx-smoke-dom0less-arm64.sh 2>&1 | tee
> ${LOGFILE}
>needs:
> - *arm64-test-needs
> -- alpine-3.18-gcc-arm64
> +- alpine-3.18-gcc-debug-arm64
This change should be reflected in the name of the test changed (here and
below), so that it contains -debug suffix just like every other debug job.
With that done:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v4 4/4] docs/features/dom0less: Update the late XenStore init protocol
On 25/05/2024 00:55, Stefano Stabellini wrote: > From: Henry Wang > > With the new allocation strategy of Dom0less DomUs XenStore page, > update the doc of the late XenStore init protocol accordingly. > > Signed-off-by: Henry Wang Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v4 2/4] xen/arm: Alloc XenStore page for Dom0less DomUs from hypervisor
Hi Stefano,
Wasn't this patch supposed to be folded with patch no.3 to avoid breaking the
bisection?
On 25/05/2024 00:55, Stefano Stabellini wrote:
> From: Henry Wang
>
> There are use cases (for example using the PV driver) in Dom0less
> setup that require Dom0less DomUs start immediately with Dom0, but
> initialize XenStore later after Dom0's successful boot and call to
> the init-dom0less application.
>
> An error message can seen from the init-dom0less application on
> 1:1 direct-mapped domains:
> ```
> Allocating magic pages
> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> Error on alloc magic pages
> ```
>
> The "magic page" is a terminology used in the toolstack as reserved
> pages for the VM to have access to virtual platform capabilities.
> Currently the magic pages for Dom0less DomUs are populated by the
> init-dom0less app through populate_physmap(), and populate_physmap()
> automatically assumes gfn == mfn for 1:1 direct mapped domains. This
> cannot be true for the magic pages that are allocated later from the
> init-dom0less application executed in Dom0. For domain using statically
> allocated memory but not 1:1 direct-mapped, similar error "failed to
> retrieve a reserved page" can be seen as the reserved memory list is
> empty at that time.
>
> Since for init-dom0less, the magic page region is only for XenStore.
> To solve above issue, this commit allocates the XenStore page for
> Dom0less DomUs at the domain construction time. The PFN will be
> noted and communicated to the init-dom0less application executed
> from Dom0. To keep the XenStore late init protocol, set the connection
> status to XENSTORE_RECONNECT.
>
> Reported-by: Alec Kwapis
> Suggested-by: Daniel P. Smith
> Signed-off-by: Henry Wang
> Signed-off-by: Stefano Stabellini
> ---
> xen/arch/arm/dom0less-build.c | 55 ++-
> 1 file changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242..2963ecc0b3 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
> #include
> +#include
> #include
> #include
> #include
> @@ -10,6 +11,8 @@
> #include
> #include
>
> +#include
> +
> #include
> #include
> #include
> @@ -739,6 +742,53 @@ static int __init alloc_xenstore_evtchn(struct domain *d)
> return 0;
> }
>
> +#define XENSTORE_PFN_OFFSET 1
> +static int __init alloc_xenstore_page(struct domain *d)
> +{
> +struct page_info *xenstore_pg;
> +struct xenstore_domain_interface *interface;
> +mfn_t mfn;
> +gfn_t gfn;
> +int rc;
> +
> +if ( (UINT_MAX - d->max_pages) < 1 )
> +{
> +printk(XENLOG_ERR "%pd: Over-allocation for d->max_pages by 1
> page.\n",
> + d);
> +return -EINVAL;
> +}
> +d->max_pages += 1;
NIT: empty line here for readability
> +xenstore_pg = alloc_domheap_page(d, MEMF_bits(32));
> +if ( xenstore_pg == NULL && is_64bit_domain(d) )
> +xenstore_pg = alloc_domheap_page(d, 0);
> +if ( xenstore_pg == NULL )
> +return -ENOMEM;
> +
> +mfn = page_to_mfn(xenstore_pg);
> +if ( !mfn_x(mfn) )
> +return -ENOMEM;
I think you should free the allocated page here
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v3 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
Hi Luca,
On 22/05/2024 09:51, Luca Fancellu wrote:
>
>
> Handle the parsing of the 'xen,shared-mem' property when the host physical
> address is not provided, this commit is introducing the logic to parse it,
> but the functionality is still not implemented and will be part of future
> commits.
>
> Rework the logic inside process_shm_node to check the shm_id before doing
> the other checks, because it ease the logic itself, add more comment on
> the logic.
> Now when the host physical address is not provided, the value
> INVALID_PADDR is chosen to signal this condition and it is stored as
> start of the bank, due to that change also early_print_info_shmem and
> init_sharedmem_pages are changed, to not handle banks with start equal
> to INVALID_PADDR.
>
> Another change is done inside meminfo_overlap_check, to skip banks that
> are starting with the start address INVALID_PADDR, that function is used
> to check banks from reserved memory, shared memory and ACPI and since
> the comment above the function states that wrapping around is not handled,
> it's unlikely for these bank to have the start address as INVALID_PADDR.
> Same change is done inside consider_modules, find_unallocated_memory and
> dt_unreserved_regions functions, in order to skip banks that starts with
> INVALID_PADDR from any computation.
> The changes above holds because of this consideration.
>
> Signed-off-by: Luca Fancellu
> Reviewed-by: Michal Orzel
> ---
> v3 changes:
> - fix typo in commit msg, add R-by Michal
> v2 changes:
> - fix comments, add parenthesis to some conditions, remove unneeded
>variables, remove else branch, increment counter in the for loop,
>skip INVALID_PADDR start banks from also consider_modules,
>find_unallocated_memory and dt_unreserved_regions. (Michal)
> ---
> xen/arch/arm/arm32/mmu/mm.c | 11 +++-
> xen/arch/arm/domain_build.c | 5 ++
> xen/arch/arm/setup.c| 14 +++-
> xen/arch/arm/static-shmem.c | 125 +---
> 4 files changed, 111 insertions(+), 44 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
> index be480c31ea05..30a7aa1e8e51 100644
> --- a/xen/arch/arm/arm32/mmu/mm.c
> +++ b/xen/arch/arm/arm32/mmu/mm.c
> @@ -101,8 +101,15 @@ static paddr_t __init consider_modules(paddr_t s,
> paddr_t e,
> nr += reserved_mem->nr_banks;
> for ( ; i - nr < shmem->nr_banks; i++ )
> {
> -paddr_t r_s = shmem->bank[i - nr].start;
> -paddr_t r_e = r_s + shmem->bank[i - nr].size;
> +paddr_t r_s, r_e;
> +
> +r_s = shmem->bank[i - nr].start;
> +
> +/* Shared memory banks can contain INVALID_PADDR as start */
> +if ( INVALID_PADDR == r_s )
> +continue;
> +
> +r_e = r_s + shmem->bank[i - nr].size;
>
> if ( s < r_e && r_s < e )
> {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 968c497efc78..02e741685102 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -927,6 +927,11 @@ static int __init find_unallocated_memory(const struct
> kernel_info *kinfo,
> for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
> {
> start = mem_banks[i]->bank[j].start;
> +
> +/* Shared memory banks can contain INVALID_PADDR as start */
> +if ( INVALID_PADDR == start )
> +continue;
> +
> end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
> res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
> PFN_DOWN(end - 1));
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index c4e5c19b11d6..0c2fdaceaf21 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -240,8 +240,15 @@ static void __init dt_unreserved_regions(paddr_t s,
> paddr_t e,
> offset = reserved_mem->nr_banks;
> for ( ; i - offset < shmem->nr_banks; i++ )
> {
> -paddr_t r_s = shmem->bank[i - offset].start;
> -paddr_t r_e = r_s + shmem->bank[i - offset].size;
> +paddr_t r_s, r_e;
> +
> +r_s = shmem->bank[i - offset].start;
> +
> +/* Shared memory banks can contain INVALID_PADDR as start */
> +if ( INVALID_PADDR == r_s )
> +continue;
> +
> +r_e = r_s + shmem->bank[i - offset].size;
>
> if ( s < r_e && r_s < e )
> {
> @@ -272,7 +279,8 @@ static bool __init meminfo_overlap_check(const struct
> membanks *mem,
> bank_start = mem->bank[i].start;
Re: [PATCH v3 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Luca, On 22/05/2024 09:51, Luca Fancellu wrote: > > > This commit implements the logic to have the static shared memory banks > from the Xen heap instead of having the host physical address passed from > the user. > > When the host physical address is not supplied, the physical memory is > taken from the Xen heap using allocate_domheap_memory, the allocation > needs to occur at the first handled DT node and the allocated banks > need to be saved somewhere. > > Introduce the 'shm_heap_banks' for that reason, a struct that will hold > the banks allocated from the heap, its field bank[].shmem_extra will be > used to point to the bootinfo shared memory banks .shmem_extra space, so > that there is not further allocation of memory and every bank in > shm_heap_banks can be safely identified by the shm_id to reconstruct its > traceability and if it was allocated or not. > > A search into 'shm_heap_banks' will reveal if the banks were allocated > or not, in case the host address is not passed, and the callback given > to allocate_domheap_memory will store the banks in the structure and > map them to the current domain, to do that, some changes to > acquire_shared_memory_bank are made to let it differentiate if the bank > is from the heap and if it is, then assign_pages is called for every > bank. > > When the bank is already allocated, for every bank allocated with the > corresponding shm_id, handle_shared_mem_bank is called and the mapping > are done. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v3 3/7] xen/p2m: put reference for level 2 superpage
Hi Luca, On 22/05/2024 09:51, Luca Fancellu wrote: > > > From: Penny Zheng > > We are doing foreign memory mapping for static shared memory, and > there is a great possibility that it could be super mapped. > But today, p2m_put_l3_page could not handle superpages. > > This commits implements a new function p2m_put_l2_superpage to handle > 2MB superpages, specifically for helping put extra references for > foreign superpages. > > Modify relinquish_p2m_mapping as well to take into account preemption > when type is foreign memory and order is above 9 (2MB). > > Currently 1GB superpages are not handled because Xen is not preemptible > and therefore some work is needed to handle such superpages, for which > at some point Xen might end up freeing memory and therefore for such a > big mapping it could end up in a very long operation. > > Signed-off-by: Penny Zheng > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
On 21/05/2024 09:51, Henry Wang wrote: > Hi Michal, > > On 5/21/2024 3:47 PM, Michal Orzel wrote: >> Hi Henry. >> >> On 3/21/2024 11:57 AM, Henry Wang wrote: >>>> In the common sysctl command XEN_SYSCTL_physinfo, the value of >>>> cores_per_socket is calculated based on the cpu_core_mask of CPU0. >>>> Currently on Arm this is a fixed value 1 (can be checked via xl info), >>>> which is not correct. This is because during the Arm CPU online >>>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu >>>> cpu_core_mask for itself. >>>> >>>> cores_per_socket refers to the number of cores that belong to the same >>>> socket (NUMA node). Currently Xen on Arm does not support physical >>>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore >>>> cores_per_socket means all possible CPUs detected from the device >>>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map() >>>> accordingly. Modify the in-code comment which seems to be outdated. Add >>>> a warning to users if Xen is running on processors with multithread >>>> support. >>>> >>>> Signed-off-by: Henry Wang >>>> Signed-off-by: Henry Wang >> Reviewed-by: Michal Orzel > > Thanks. > >>>>/* ID of the PCPU we're running on */ >>>>DEFINE_PER_CPU(unsigned int, cpu_id); >>>> -/* XXX these seem awfully x86ish... */ >>>> +/* >>>> + * Although multithread is part of the Arm spec, there are not many >>>> + * processors support multithread and current Xen on Arm assumes there >> NIT: s/support/supporting > > Sorry, it should have been spotted locally before sending. Anyway, I > will correct this in v4 with your Reviewed-by tag taken. Thanks for > pointing this out. I don't think there is a need to resend a patch just for fixing this typo. It can be done on commit. ~Michal
Re: [PATCH v3] xen/arm: Set correct per-cpu cpu_core_mask
Hi Henry.
On 20/05/2024 04:57, Henry Wang wrote:
> Hi All,
>
> Gentle ping since it has been a couple of months, any comments on this
> updated patch? Thanks!
Sorry for the late reply.
>
> Kind regards,
> Henry
>
> On 3/21/2024 11:57 AM, Henry Wang wrote:
>> In the common sysctl command XEN_SYSCTL_physinfo, the value of
>> cores_per_socket is calculated based on the cpu_core_mask of CPU0.
>> Currently on Arm this is a fixed value 1 (can be checked via xl info),
>> which is not correct. This is because during the Arm CPU online
>> process at boot time, setup_cpu_sibling_map() only sets the per-cpu
>> cpu_core_mask for itself.
>>
>> cores_per_socket refers to the number of cores that belong to the same
>> socket (NUMA node). Currently Xen on Arm does not support physical
>> CPU hotplug and NUMA, also we assume there is no multithread. Therefore
>> cores_per_socket means all possible CPUs detected from the device
>> tree. Setting the per-cpu cpu_core_mask in setup_cpu_sibling_map()
>> accordingly. Modify the in-code comment which seems to be outdated. Add
>> a warning to users if Xen is running on processors with multithread
>> support.
>>
>> Signed-off-by: Henry Wang
>> Signed-off-by: Henry Wang
Reviewed-by: Michal Orzel
>> ---
>> v3:
>> - Use cpumask_copy() to set cpu_core_mask and drop the unnecessary
>>cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu)).
>> - In-code comment adjustments.
>> - Add a warning for multithread.
>> v2:
>> - Do not do the multithread check.
>> ---
>> xen/arch/arm/smpboot.c | 18 +++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index a84e706d77..b6268be27a 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -66,7 +66,11 @@ static bool cpu_is_dead;
>>
>> /* ID of the PCPU we're running on */
>> DEFINE_PER_CPU(unsigned int, cpu_id);
>> -/* XXX these seem awfully x86ish... */
>> +/*
>> + * Although multithread is part of the Arm spec, there are not many
>> + * processors support multithread and current Xen on Arm assumes there
NIT: s/support/supporting
>> + * is no multithread.
>> + */
>> /* representing HT siblings of each logical CPU */
>> DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
>> /* representing HT and core siblings of each logical CPU */
>> @@ -85,9 +89,13 @@ static int setup_cpu_sibling_map(int cpu)
>>!zalloc_cpumask_var(_cpu(cpu_core_mask, cpu)) )
>> return -ENOMEM;
>>
>> -/* A CPU is a sibling with itself and is always on its own core. */
>> +/*
>> + * Currently we assume there is no multithread and NUMA, so
>> + * a CPU is a sibling with itself, and the all possible CPUs
>> + * are supposed to belong to the same socket (NUMA node).
>> + */
>> cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
>> -cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
>> +cpumask_copy(per_cpu(cpu_core_mask, cpu), _possible_map);
>>
>> return 0;
>> }
>> @@ -277,6 +285,10 @@ void __init smp_init_cpus(void)
>> warning_add("WARNING: HMP COMPUTING HAS BEEN ENABLED.\n"
>> "It has implications on the security and stability of
>> the system,\n"
>> "unless the cpu affinity of all domains is
>> specified.\n");
>> +
>> +if ( system_cpuinfo.mpidr.mt == 1 )
>> +warning_add("WARNING: MULTITHREADING HAS BEEN DETECTED ON THE
>> PROCESSOR.\n"
>> +"It might impact the security of the system.\n");
>> }
>>
>> unsigned int __init smp_get_max_cpus(void)
>
~Michal
Re: [PATCH v3 1/4] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains
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
>>> 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
Re: [PATCH v3 1/4] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains
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
>
> 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
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
On 20/05/2024 15:11, Luca Fancellu wrote:
>
>
>>>
> +struct shmem_membank_extra *bank_extra_info;
> +} alloc_heap_pages_cb_extra;
> +
> +static struct meminfo __initdata shm_heap_banks = {
> +.common.max_banks = NR_MEM_BANKS
Do we expect that many banks?
>>>
>>> Not really, but I was trying to don’t introduce another type, do you think
>>> it’s better instead to
>>> introduce a new type only here, with a lower amount of banks?
>> I'd be ok with meminfo provided you add a reasoning behind this being
>> meminfo and not shared_meminfo.
>>
>>>
>>> Because if we take struct shared_meminfo, we would waste mem for its
>>> ‘extra’ member.
>> Would it result in a smaller footprint overall?
>
> Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total,
> even if we use 32 of them and
> 32 are wasted.
>
> Otherwise, as I said, I could do something like this in this module:
>
> static struct shared_heap_meminfo {
> struct membanks_hdr common;
> struct membank bank[NR_SHMEM_BANKS];
> } __initdata shm_heap_banks = {
> .common.max_banks = NR_SHMEM_BANKS
> };
If that's all it takes to avoid defining unnecessarily big variable, then I'd
be ok with it.
~Michal
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
On 20/05/2024 14:44, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>> On 20 May 2024, at 12:16, Michal Orzel wrote:
>>
>> Hi Luca,
>>
>> On 15/05/2024 16:26, Luca Fancellu wrote:
>>>
>>>
>>> This commit implements the logic to have the static shared memory banks
>>> from the Xen heap instead of having the host physical address passed from
>>> the user.
>>>
>>> When the host physical address is not supplied, the physical memory is
>>> taken from the Xen heap using allocate_domheap_memory, the allocation
>>> needs to occur at the first handled DT node and the allocated banks
>>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>>> global variable of type 'struct meminfo' that will hold the banks
>>> allocated from the heap, its field .shmem_extra will be used to point
>>> to the bootinfo shared memory banks .shmem_extra space, so that there
>>> is not further allocation of memory and every bank in shm_heap_banks
>>> can be safely identified by the shm_id to reconstruct its traceability
>>> and if it was allocated or not.
>> NIT for the future: it's better to split 10 lines long sentence into
>> multiple ones.
>> Otherwise it reads difficult.
>
> I’ll do,
>
>>>
>>> xen/arch/arm/static-shmem.c | 186 ++--
>>> 1 file changed, 155 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>>> index ddaacbc77740..9c3a83042d8b 100644
>>> --- a/xen/arch/arm/static-shmem.c
>>> +++ b/xen/arch/arm/static-shmem.c
>>> @@ -9,6 +9,22 @@
>>> #include
>>> #include
>>>
>>> +typedef struct {
>>> +struct domain *d;
>>> +paddr_t gbase;
>>> +const char *role_str;
>> You could swap role_str and gbase to avoid a 4B hole on arm32
>
> Sure I will,
>
>>
>>> +struct shmem_membank_extra *bank_extra_info;
>>> +} alloc_heap_pages_cb_extra;
>>> +
>>> +static struct meminfo __initdata shm_heap_banks = {
>>> +.common.max_banks = NR_MEM_BANKS
>> Do we expect that many banks?
>
> Not really, but I was trying to don’t introduce another type, do you think
> it’s better instead to
> introduce a new type only here, with a lower amount of banks?
I'd be ok with meminfo provided you add a reasoning behind this being meminfo
and not shared_meminfo.
>
> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’
> member.
Would it result in a smaller footprint overall?
>
>>>
>>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>>> + bool bank_from_heap,
>>>const struct membank *shm_bank)
>>> {
>>> mfn_t smfn;
>>> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain
>>> *d, paddr_t gbase,
>>> psize = shm_bank->size;
>>> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>>>
>>> -printk("%pd: allocate static shared memory BANK
>>> %#"PRIpaddr"-%#"PRIpaddr".\n",
>>> - d, pbase, pbase + psize);
>>> -
>>> -smfn = acquire_shared_memory_bank(d, pbase, psize);
>>> +smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
>>> if ( mfn_eq(smfn, INVALID_MFN) )
>>> return -EINVAL;
>>>
>>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo,
>>> paddr_t start,
>>>
>>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> const char *role_str,
>>> + bool bank_from_heap,
>>> const struct membank *shm_bank)
>>> {
>>> bool owner_dom_io = true;
>>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain
>>> *d, paddr_t gbase,
>>> pbase = shm_bank->start;
>>> psize = shm_bank->size;
>>>
>>> +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);
>> This looks more like a debug print since I don't expect user to want to s
Re: [PATCH v3 1/4] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains
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
>
> Signed-off-by: Henry Wang
> ---
> 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
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> This commit implements the logic to have the static shared memory banks
> from the Xen heap instead of having the host physical address passed from
> the user.
>
> When the host physical address is not supplied, the physical memory is
> taken from the Xen heap using allocate_domheap_memory, the allocation
> needs to occur at the first handled DT node and the allocated banks
> need to be saved somewhere, so introduce the 'shm_heap_banks' static
> global variable of type 'struct meminfo' that will hold the banks
> allocated from the heap, its field .shmem_extra will be used to point
> to the bootinfo shared memory banks .shmem_extra space, so that there
> is not further allocation of memory and every bank in shm_heap_banks
> can be safely identified by the shm_id to reconstruct its traceability
> and if it was allocated or not.
NIT for the future: it's better to split 10 lines long sentence into multiple
ones.
Otherwise it reads difficult.
>
> A search into 'shm_heap_banks' will reveal if the banks were allocated
> or not, in case the host address is not passed, and the callback given
> to allocate_domheap_memory will store the banks in the structure and
> map them to the current domain, to do that, some changes to
> acquire_shared_memory_bank are made to let it differentiate if the bank
> is from the heap and if it is, then assign_pages is called for every
> bank.
>
> When the bank is already allocated, for every bank allocated with the
> corresponding shm_id, handle_shared_mem_bank is called and the mapping
> are done.
>
> Signed-off-by: Luca Fancellu
> ---
> v2 changes:
> - add static inline get_shmem_heap_banks(), given the changes to the
>struct membanks interface. Rebase changes due to removal of
>owner_dom_io arg from handle_shared_mem_bank.
>Change save_map_heap_pages return type given the changes to the
>allocate_domheap_memory callback type.
> ---
> xen/arch/arm/static-shmem.c | 186 ++--
> 1 file changed, 155 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index ddaacbc77740..9c3a83042d8b 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -9,6 +9,22 @@
> #include
> #include
>
> +typedef struct {
> +struct domain *d;
> +paddr_t gbase;
> +const char *role_str;
You could swap role_str and gbase to avoid a 4B hole on arm32
> +struct shmem_membank_extra *bank_extra_info;
> +} alloc_heap_pages_cb_extra;
> +
> +static struct meminfo __initdata shm_heap_banks = {
> +.common.max_banks = NR_MEM_BANKS
Do we expect that many banks?
> +};
> +
> +static inline struct membanks *get_shmem_heap_banks(void)
> +{
> +return container_of(_heap_banks.common, struct membanks, common);
> +}
> +
> static void __init __maybe_unused build_assertions(void)
> {
> /*
> @@ -64,7 +80,8 @@ static bool __init is_shm_allocated_to_domio(paddr_t pbase)
> }
>
> static mfn_t __init acquire_shared_memory_bank(struct domain *d,
> - paddr_t pbase, paddr_t psize)
> + paddr_t pbase, paddr_t psize,
> + bool bank_from_heap)
> {
> mfn_t smfn;
> unsigned long nr_pfns;
> @@ -84,19 +101,31 @@ static mfn_t __init acquire_shared_memory_bank(struct
> domain *d,
> d->max_pages += nr_pfns;
>
> smfn = maddr_to_mfn(pbase);
> -res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> +if ( bank_from_heap )
> +/*
> + * When host address is not provided, static shared memory is
> + * allocated from heap and shall be assigned to owner domain.
> + */
> +res = assign_pages(maddr_to_page(pbase), nr_pfns, d, 0);
> +else
> +res = acquire_domstatic_pages(d, smfn, nr_pfns, 0);
> +
> if ( res )
> {
> -printk(XENLOG_ERR
> - "%pd: failed to acquire static memory: %d.\n", d, res);
> -d->max_pages -= nr_pfns;
> -return INVALID_MFN;
> +printk(XENLOG_ERR "%pd: failed to %s static memory: %d.\n", d,
> + bank_from_heap ? "assign" : "acquire", res);
> +goto fail;
> }
>
> return smfn;
> +
> + fail:
> +d->max_pages -= nr_pfns;
> +return INVALID_MFN;
> }
>
> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
> + bool bank_from_heap,
> const struct membank *shm_bank)
> {
> mfn_t smfn;
> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d,
> paddr_t gbase,
> psize = shm_bank->size;
> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>
> -printk("%pd: allocate static shared memory BANK
> %#"PRIpaddr"-%#"PRIpaddr".\n",
> - d, pbase, pbase + psize);
> -
> -
Re: [PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
Hi Luca, On 15/05/2024 16:26, Luca Fancellu wrote: > > > The function allocate_bank_memory allocates pages from the heap and > maps them to the guest using guest_physmap_add_page. > > As a preparation work to support static shared memory bank when the > host physical address is not provided, Xen needs to allocate memory > from the heap, so rework allocate_bank_memory moving out the page > allocation in a new function called allocate_domheap_memory. > > The function allocate_domheap_memory takes a callback function and > a pointer to some extra information passed to the callback and this > function will be called for every region, until a defined size is > reached. > > In order to keep allocate_bank_memory functionality, the callback > passed to allocate_domheap_memory is a wrapper for > guest_physmap_add_page. > > Let allocate_domheap_memory be externally visible, in order to use > it in the future from the static shared memory module. > > Take the opportunity to change the signature of allocate_bank_memory > and remove the 'struct domain' parameter, which can be retrieved from > 'struct kernel_info'. > > No functional changes is intended. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
Hi Luca, On 15/05/2024 16:26, Luca Fancellu wrote: > > > Handle the parsing of the 'xen,shared-mem' property when the host physical > address is not provided, this commit is introducing the logic to parse it, > but the functionality is still not implemented and will be part of future > commits. > > Rework the logic inside process_shm_node to check the shm_id before doing > the other checks, because it ease the logic itself, add more comment on > the logic. > Now when the host physical address is not provided, the value > INVALID_PADDR is chosen to signal this condition and it is stored as > start of the bank, due to that change also early_print_info_shmem and > init_sharedmem_pages are changed, to don't handle banks with start equal s/don't/not/ > to INVALID_PADDR. > > Another change is done inside meminfo_overlap_check, to skip banks that > are starting with the start address INVALID_PADDR, that function is used > to check banks from reserved memory, shared memory and ACPI and since > the comment above the function states that wrapping around is not handled, > it's unlikely for these bank to have the start address as INVALID_PADDR. > Same change is done inside consider_modules, find_unallocated_memory and > dt_unreserved_regions functions, in order to skip banks that starts with > INVALID_PADDR from any computation. > The changes above holds because of this consideration. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel Note: This patch will need rebasing since it does not apply cleanly on staging. ~Michal
Re: [PATCH v2 3/7] xen/p2m: put reference for level 2 superpage
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> From: Penny Zheng
>
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.
> But today, p2m_put_l3_page could not handle superpages.
s/could/can
>
> This commits implements a new function p2m_put_l2_superpage to handle
> 2MB superpages, specifically for helping put extra references for
> foreign superpages.
>
> Modify relinquish_p2m_mapping as well to take into account preemption
> when type is foreign memory and order is above 9 (2MB).
I don't see the reasoning why we don't handle 1GB super pages. It would be
beneficial
to include such in commit msg as well as in the code.
>
> Signed-off-by: Penny Zheng
> Signed-off-by: Luca Fancellu
> ---
> v2:
> - Do not handle 1GB super page as there might be some issue where
>a lot of calls to put_page(...) might be issued which could lead
>to free memory that is a long operation.
> v1:
> - patch from
> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/
> ---
> xen/arch/arm/mmu/p2m.c | 57 +-
> 1 file changed, 40 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 41fcca011cf4..29fdb3b87dd0 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain
> *p2m, gfn_t gfn,
> return rc;
> }
>
> -/*
> - * Put any references on the single 4K page referenced by pte.
> - * TODO: Handle superpages, for now we only take special references for leaf
> - * pages (specifically foreign ones, which can't be super mapped today).
> - */
> -static void p2m_put_l3_page(const lpae_t pte)
> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type)
> {
> -mfn_t mfn = lpae_get_mfn(pte);
> -
> -ASSERT(p2m_is_valid(pte));
> -
> /*
> * TODO: Handle other p2m types
> *
> @@ -771,16 +763,44 @@ static void p2m_put_l3_page(const lpae_t pte)
> * flush the TLBs if the page is reallocated before the end of
> * this loop.
> */
> -if ( p2m_is_foreign(pte.p2m.type) )
> +if ( p2m_is_foreign(type) )
> {
> ASSERT(mfn_valid(mfn));
> put_page(mfn_to_page(mfn));
> }
> /* Detect the xenheap page and mark the stored GFN as invalid. */
> -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
> page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
> }
>
> +/* Put any references on the superpage referenced by mfn. */
> +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type)
> +{
> +unsigned int i;
> +unsigned int level_order = XEN_PT_LEVEL_ORDER(3);
If we know the third level order is always 0 no matter the page shift, does it
make sense to have this variable?
> +
> +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> +{
> +p2m_put_l3_page(mfn, type);
> +
> +mfn = mfn_add(mfn, 1 << level_order);
> +}
> +}
> +
> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(const lpae_t pte, unsigned int level)
> +{
> +mfn_t mfn = lpae_get_mfn(pte);
> +
> +ASSERT(p2m_is_valid(pte));
> +
> +/* We have a second level 2M superpage */
> +if ( p2m_is_superpage(pte, level) && (level == 2) )
> +return p2m_put_l2_superpage(mfn, pte.p2m.type);
> +else if ( level == 3 )
> +return p2m_put_l3_page(mfn, pte.p2m.type);
> +}
> +
> /* Free lpae sub-tree behind an entry */
> static void p2m_free_entry(struct p2m_domain *p2m,
> lpae_t entry, unsigned int level)
> @@ -809,9 +829,10 @@ static void p2m_free_entry(struct p2m_domain *p2m,
> #endif
>
> p2m->stats.mappings[level]--;
> -/* Nothing to do if the entry is a super-page. */
> -if ( level == 3 )
> -p2m_put_l3_page(entry);
> +/* TODO: Currently we don't handle 1GB super-page. */
As a future reader of the code it would be beneficial to say why.
> +if ( level >= 2 )
> +p2m_put_page(entry, level);
> +
> return;
> }
>
> @@ -1558,9 +1579,11 @@ int relinquish_p2m_mapping(struct domain *d)
>
> count++;
> /*
> - * Arbitrarily preempt every 512 iterations.
> + * Arbitrarily preempt every 512 iterations or when type is foreign
> + * mapping and the order is above 9 (2MB).
> */
> -if ( !(count % 512) && hypercall_preempt_check() )
> +if ( (!(count % 512) || (p2m_is_foreign(t) && (order > 9))) &&
Instead of (order > 9), use XEN_PT_LEVEL_ORDER(2)
> + hypercall_preempt_check() )
> {
> rc = -ERESTART;
> break;
> --
> 2.34.1
>
~Michal
Re: [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> Wrap the code and logic that is calling assign_shared_memory
> and map_regions_p2mt into a new function 'handle_shared_mem_bank',
> it will become useful later when the code will allow the user to
> don't pass the host physical address.
>
> Signed-off-by: Luca Fancellu
> ---
> v2 changes:
> - add blank line, move owner_dom_io computation inside
>handle_shared_mem_bank in order to reduce args count, remove
>not needed BUGON(). (Michal)
> ---
> xen/arch/arm/static-shmem.c | 87 ++---
> 1 file changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 0afc86c43f85..8a14d120690c 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo,
> paddr_t start,
> return 0;
> }
>
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> + const char *role_str,
> + const struct membank *shm_bank)
> +{
> +bool owner_dom_io = true;
> +paddr_t pbase, psize;
> +int ret;
> +
> +pbase = shm_bank->start;
> +psize = shm_bank->size;
> +
> +/*
> + * "role" property is optional and if it is defined explicitly,
> + * then the owner domain is not the default "dom_io" domain.
> + */
> +if ( role_str != NULL )
> +owner_dom_io = false;
> +
> +/*
> + * DOMID_IO is a fake domain and is not described in the Device-Tree.
> + * Therefore when the owner of the shared region is DOMID_IO, we will
> + * only find the borrowers.
> + */
> +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> + (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> +{
> +/*
> + * We found the first borrower of the region, the owner was not
> + * specified, so they should be assigned to dom_io.
> + */
> +ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
> shm_bank);
> +if ( ret )
> +return ret;
> +}
> +
> +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
> +{
> +/* Set up P2M foreign mapping for borrower domain. */
> +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
> + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
> +if ( ret )
> +return ret;
> +}
> +
> +return 0;
> +}
> +
> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> paddr_t gbase, pbase, psize;
> int ret = 0;
> unsigned int i;
> -const char *role_str;
> +const char *role_str = NULL;
> const char *shm_id;
> -bool owner_dom_io = true;
>
> if ( !dt_device_is_compatible(shm_node,
> "xen,domain-shared-memory-v1") )
> continue;
> @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> return -EINVAL;
> }
>
> -/*
> - * "role" property is optional and if it is defined explicitly,
> - * then the owner domain is not the default "dom_io" domain.
> - */
> -if ( dt_property_read_string(shm_node, "role", _str) == 0 )
> - owner_dom_io = false;
> +/* "role" property is optional */
> +dt_property_read_string(shm_node, "role", _str);
This now violates a MISRA rule saying that if a function returns a value, this
value needs to be checked.
I think you should check if return value is non zero and if such, assign
role_str NULL (thus removing such
assignment from a definition).
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
Hi Luca,
On 15/05/2024 16:26, Luca Fancellu wrote:
>
>
> The current static shared memory code is using bootinfo banks when it
> needs to find the number of borrowers, so every time assign_shared_memory
> is called, the bank is searched in the bootinfo.shmem structure.
>
> There is nothing wrong with it, however the bank can be used also to
> retrieve the start address and size and also to pass less argument to
s/argument/arguments
> assign_shared_memory. When retrieving the information from the bootinfo
> bank, it's also possible to move the checks on alignment to
> process_shm_node in the early stages.
>
> So create a new function find_shm_bank_by_id() which takes a
> 'struct shared_meminfo' structure and the shared memory ID, to look for a
> bank with a matching ID, take the physical host address and size from the
> bank, pass the bank to assign_shared_memory() removing the now unnecessary
> arguments and finally remove the acquire_nr_borrower_domain() function
> since now the information can be extracted from the passed bank.
> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
> case of errors (unlikely), as said above, move the checks on alignment
> to process_shm_node.
>
> Drawback of this change is that now the bootinfo are used also when the
> bank doesn't need to be allocated, however it will be convinient later
s/convinient/convenient
> to use it as an argument for assign_shared_memory when dealing with
> the use case where the Host physical address is not supplied by the user.
>
> Signed-off-by: Luca Fancellu
> ---
> v2 changes:
> - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(),
>swap region size check different from zero and size alignment, remove
>not necessary BUGON(). (Michal)
> ---
> xen/arch/arm/static-shmem.c | 101 +++-
> 1 file changed, 54 insertions(+), 47 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 78881dd1d3f7..0afc86c43f85 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -19,29 +19,22 @@ static void __init __maybe_unused build_assertions(void)
> offsetof(struct shared_meminfo, bank)));
> }
>
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - unsigned long *nr_borrowers)
> +static const struct membank __init *
> +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id)
> {
> -const struct membanks *shmem = bootinfo_get_shmem();
> unsigned int bank;
>
> -/* Iterate reserved memory to find requested shm bank. */
> for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
> {
> -paddr_t bank_start = shmem->bank[bank].start;
> -paddr_t bank_size = shmem->bank[bank].size;
> -
> -if ( (pbase == bank_start) && (psize == bank_size) )
> +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
Does it really need to be strncmp? You validated it a few times already.
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v9 04/15] xen/bitops: put __ffs() into linux compatible header
On 06/05/2024 12:15, Oleksii Kurochko wrote: > > > The mentioned macros exist only because of Linux compatible purpose. > > The patch defines __ffs() in terms of Xen bitops and it is safe > to define in this way ( as __ffs() - 1 ) as considering that __ffs() > was defined as __builtin_ctzl(x), which has undefined behavior when x=0, > so it is assumed that such cases are not encountered in the current code. > > To not include to Xen library files __ffs() and __ffz() > were defined locally in find-next-bit.c. > > Except __ffs() usage in find-next-bit.c only one usage of __ffs() leave > in smmu-v3.c. It seems that it __ffs can be changed to ffsl(x)-1 in > this file, but to keep smmu-v3.c looks close to linux it was deciced just > to define __ffs() in xen/linux-compat.h and include it in smmu-v3.c > > Signed-off-by: Oleksii Kurochko > Acked-by: Shawn Anastasio > Reviewed-by: Jan Beulich Acked-by: Michal Orzel ~Michal
Re: [PATCH 7/7] xen/docs: Describe static shared memory when host address is not provided
Hi Luca, On 23/04/2024 10:25, Luca Fancellu wrote: > > > From: Penny Zheng > > This commit describe the new scenario where host address is not provided > in "xen,shared-mem" property and a new example is added to the page to > explain in details. > > Take the occasion to fix some typos in the page. > > Signed-off-by: Penny Zheng > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Luca, On 10/05/2024 11:25, Luca Fancellu wrote: > > >> On 10 May 2024, at 10:17, Michal Orzel wrote: >> >> Hi Luca, >> >> On 23/04/2024 10:25, Luca Fancellu wrote: >>> >>> >>> This commit implements the logic to have the static shared memory banks >>> from the Xen heap instead of having the host physical address passed from >>> the user. >>> >>> When the host physical address is not supplied, the physical memory is >>> taken from the Xen heap using allocate_domheap_memory, the allocation >>> needs to occur at the first handled DT node and the allocated banks >>> need to be saved somewhere, so introduce the 'shm_heap_banks' static >>> global variable of type 'struct meminfo' that will hold the banks >>> allocated from the heap, its field .shmem_extra will be used to point >>> to the bootinfo shared memory banks .shmem_extra space, so that there >>> is not further allocation of memory and every bank in shm_heap_banks >>> can be safely identified by the shm_id to reconstruct its traceability >>> and if it was allocated or not. >>> >>> A search into 'shm_heap_banks' will reveal if the banks were allocated >>> or not, in case the host address is not passed, and the callback given >>> to allocate_domheap_memory will store the banks in the structure and >>> map them to the current domain, to do that, some changes to >>> acquire_shared_memory_bank are made to let it differentiate if the bank >>> is from the heap and if it is, then assign_pages is called for every >>> bank. >>> >>> When the bank is already allocated, for every bank allocated with the >>> corresponding shm_id, handle_shared_mem_bank is called and the mapping >>> are done. >>> >>> Signed-off-by: Luca Fancellu >> >> I tested this patch and it resulted in assertion: >> Assertion 's <= e' failed at common/rangeset.c:189 >> >> I checked and in find_unallocated_memory(), given that start is ~0UL (host >> address not provided), >> start + size would overflow. Did you test this patch? > > Hi Michal, > > Mmm I’m testing with a dom0less setup, without dom0, I think I missed that > part because my guests doesn’t have > the hypervisor node (enhanced), sorry about that, I’ll test using your setup, > can you confirm what are you using? I have these Qemu tests (with and without SMMU): - shmem between domU1 and domU2 with/without host address provided (owner domIO or domU1) - shmem between dom0 and domU1 with/without host address provided (owner domIO or dom0) BTW. What was the conclusion about the issue if shmem region clashes with RAM allocated for 1:1 domU e.g. dom0. Accidentally, I end up with a configuration where Xen allocated for dom0 a region of RAM clashing with my shmem region. ~Michal
Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Luca, On 23/04/2024 10:25, Luca Fancellu wrote: > > > This commit implements the logic to have the static shared memory banks > from the Xen heap instead of having the host physical address passed from > the user. > > When the host physical address is not supplied, the physical memory is > taken from the Xen heap using allocate_domheap_memory, the allocation > needs to occur at the first handled DT node and the allocated banks > need to be saved somewhere, so introduce the 'shm_heap_banks' static > global variable of type 'struct meminfo' that will hold the banks > allocated from the heap, its field .shmem_extra will be used to point > to the bootinfo shared memory banks .shmem_extra space, so that there > is not further allocation of memory and every bank in shm_heap_banks > can be safely identified by the shm_id to reconstruct its traceability > and if it was allocated or not. > > A search into 'shm_heap_banks' will reveal if the banks were allocated > or not, in case the host address is not passed, and the callback given > to allocate_domheap_memory will store the banks in the structure and > map them to the current domain, to do that, some changes to > acquire_shared_memory_bank are made to let it differentiate if the bank > is from the heap and if it is, then assign_pages is called for every > bank. > > When the bank is already allocated, for every bank allocated with the > corresponding shm_id, handle_shared_mem_bank is called and the mapping > are done. > > Signed-off-by: Luca Fancellu I tested this patch and it resulted in assertion: Assertion 's <= e' failed at common/rangeset.c:189 I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided), start + size would overflow. Did you test this patch? ~Michal
Re: [PATCH 1/3] xen/arm/dom0less-build: Alloc magic pages for Dom0less DomUs from hypervisor
Hi Henry,
On 26/04/2024 05:14, Henry Wang wrote:
> There are use cases (for example using the PV driver) in Dom0less
> setup that require Dom0less DomUs start immediately with Dom0, but
> initialize XenStore later after Dom0's successful boot and call to
> the init-dom0less application.
>
> An error message can seen from the init-dom0less application on
> 1:1 direct-mapped domains:
> ```
> Allocating magic pages
> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1
> Error on alloc magic pages
> ```
> This is because currently the magic pages for Dom0less DomUs are
> populated by the init-dom0less app through populate_physmap(), and
> populate_physmap() automatically assumes gfn == mfn for 1:1 direct
> mapped domains. This cannot be true for the magic pages that are
> allocated later from the init-dom0less application executed in Dom0.
> For domain using statically allocated memory but not 1:1 direct-mapped,
> similar error "failed to retrieve a reserved page" can be seen as the
> reserved memory list is empty at that time.
>
> To solve above issue, this commit allocates the magic pages for
> Dom0less DomUs at the domain construction time. The base address/PFN
> of the magic page region will be noted and communicated to the
> init-dom0less application in Dom0.
>
> Reported-by: Alec Kwapis
> Suggested-by: Daniel P. Smith
> Signed-off-by: Henry Wang
> ---
> tools/libs/guest/xg_dom_arm.c | 1 -
> xen/arch/arm/dom0less-build.c | 22 ++
> xen/include/public/arch-arm.h | 1 +
> 3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> index 2fd8ee7ad4..8cc7f27dbb 100644
> --- a/tools/libs/guest/xg_dom_arm.c
> +++ b/tools/libs/guest/xg_dom_arm.c
> @@ -25,7 +25,6 @@
>
> #include "xg_private.h"
>
> -#define NR_MAGIC_PAGES 4
Moving only this macro to arch-arm.h while leaving the offsets does not make
much sense to me.
I think they all should be moved. This would also allow init-dom0less.h not to
re-define XENSTORE_PFN_OFFSET.
> #define CONSOLE_PFN_OFFSET 0
> #define XENSTORE_PFN_OFFSET 1
> #define MEMACCESS_PFN_OFFSET 2
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index fb63ec6fd1..40dc85c759 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -834,11 +834,33 @@ static int __init construct_domU(struct domain *d,
>
> if ( kinfo.dom0less_feature & DOM0LESS_XENSTORE )
> {
> +struct page_info *magic_pg;
> +mfn_t mfn;
> +gfn_t gfn;
> +
> ASSERT(hardware_domain);
> rc = alloc_xenstore_evtchn(d);
> if ( rc < 0 )
> return rc;
> d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
> +
> +d->max_pages += NR_MAGIC_PAGES;
> +magic_pg = alloc_domheap_pages(d,
> get_order_from_pages(NR_MAGIC_PAGES), 0);
80 char exceeded
> +if ( magic_pg == NULL )
> +return -ENOMEM;
> +
> +mfn = page_to_mfn(magic_pg);
> +if ( !is_domain_direct_mapped(d) )
> +gfn = gaddr_to_gfn(GUEST_MAGIC_BASE);
> +else
> +gfn = gaddr_to_gfn(mfn_to_maddr(mfn));
> +
> +rc = guest_physmap_add_pages(d, gfn, mfn, NR_MAGIC_PAGES);
> +if ( rc )
> +{
> +free_domheap_pages(magic_pg,
> get_order_from_pages(NR_MAGIC_PAGES));
> +return rc;
> +}
Please create a function alloc_magic_pages to encapsulate the above block.
> }
>
> return rc;
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index e167e14f8d..f24e7bbe37 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -475,6 +475,7 @@ typedef uint64_t xen_callback_t;
>
> #define GUEST_MAGIC_BASE xen_mk_ullong(0x3900)
> #define GUEST_MAGIC_SIZE xen_mk_ullong(0x0100)
> +#define NR_MAGIC_PAGES 4
>
> #define GUEST_RAM_BANKS 2
>
~Michal
Re: [PATCH 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
Hi Luca,
On 23/04/2024 10:25, Luca Fancellu wrote:
>
>
> The function allocate_bank_memory allocates pages from the heap and
> map them to the guest using guest_physmap_add_page.
s/map/maps
>
> As a preparation work to support static shared memory bank when the
> host physical address is not provided, Xen needs to allocate memory
> from the heap, so rework allocate_bank_memory moving out the page
> allocation in a new function called allocate_domheap_memory.
>
> The function allocate_domheap_memory takes a callback function and
> a pointer to some extra information passed to the callback and this
> function will be called for every page allocated, until a defined
for every region given that you pass sgfn and order
> size is reached.
>
> In order to keep allocate_bank_memory functionality, the callback
> passed to allocate_domheap_memory is a wrapper for
> guest_physmap_add_page.
>
> Let allocate_domheap_memory be externally visible, in order to use
> it in the future from the static shared memory module.
>
> Take the opportunity to change the signature of allocate_bank_memory
> and remove the 'struct domain' parameter, which can be retrieved from
> 'struct kernel_info'.
>
> No functional changes is intended.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/dom0less-build.c | 4 +-
> xen/arch/arm/domain_build.c | 77 +
> xen/arch/arm/include/asm/domain_build.h | 9 ++-
> 3 files changed, 62 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 74f053c242f4..20ddf6f8f250 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d,
> struct kernel_info *kinfo)
>
> mem->nr_banks = 0;
> bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> bank_size) )
> goto fail;
>
> bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> bank_size) )
> goto fail;
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 0784e4c5e315..148db06b8ca3 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -417,26 +417,13 @@ static void __init allocate_memory_11(struct domain *d,
> }
>
> #ifdef CONFIG_DOM0LESS_BOOT
> -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
> - gfn_t sgfn, paddr_t tot_size)
> +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> +alloc_domheap_mem_cb cb, void *extra)
> {
> -struct membanks *mem = kernel_info_get_mem(kinfo);
> -int res;
> +unsigned int max_order = UINT_MAX;
> struct page_info *pg;
You can limit the visibility of these variables by moving them into loop
> -struct membank *bank;
> -unsigned int max_order = ~0;
>
> -/*
> - * allocate_bank_memory can be called with a tot_size of zero for
> - * the second memory bank. It is not an error and we can safely
> - * avoid creating a zero-size memory bank.
> - */
> -if ( tot_size == 0 )
> -return true;
> -
> -bank = >bank[mem->nr_banks];
> -bank->start = gfn_to_gaddr(sgfn);
> -bank->size = tot_size;
> +BUG_ON(!cb);
No need for that
>
> while ( tot_size > 0 )
> {
> @@ -463,17 +450,61 @@ bool __init allocate_bank_memory(struct domain *d,
> struct kernel_info *kinfo,
> continue;
> }
>
> -res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order);
> -if ( res )
> -{
> -dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +if ( cb(d, pg, order, extra) )
> return false;
> -}
>
> -sgfn = gfn_add(sgfn, 1UL << order);
> tot_size -= (1ULL << (PAGE_SHIFT + order));
> }
>
> +return true;
> +}
> +
> +static int __init guest_map_pages(struct domain *d, struct page_info *pg,
Does it make sense to return int if it is not taken into account by the user?
> + unsigned int order, void *extra)
> +{
> +gfn_t *sgfn = (gfn_t *)extra;
> +int res;
> +
> +BUG_ON(!sgfn);
> +res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order);
> +if ( res )
> +{
> +dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> +return res;
> +}
> +
> +*sgfn = gfn_add(*sgfn, 1UL << order);
> +
> +return 0;
> +}
> +
> +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> +
Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
Hi Luca,
On 23/04/2024 10:25, Luca Fancellu wrote:
>
>
> Handle the parsing of the 'xen,shared-mem' property when the host physical
> address is not provided, this commit is introducing the logic to parse it,
> but the functionality is still not implemented and will be part of future
> commits.
>
> Rework the logic inside process_shm_node to check the shm_id before doing
> the other checks, because it ease the logic itself, add more comment on
> the logic.
> Now when the host physical address is not provided, the value
> INVALID_PADDR is chosen to signal this condition and it is stored as
> start of the bank, due to that change also early_print_info_shmem and
> init_sharedmem_pages are changed, to don't handle banks with start equal
> to INVALID_PADDR.
>
> Another change is done inside meminfo_overlap_check, to skip banks that
> are starting with the start address INVALID_PADDR, that function is used
> to check banks from reserved memory and ACPI and it's unlikely for these
also from shmem
> bank to have the start address as INVALID_PADDR. The change holds
> because of this consideration.
On arm64 and LPAE arm32 we don't have this problem. In theory we could have a
bank
starting at INVALID_PADDR if PA range was 32bit but as the comment above the
function states,
wrapping around is not handled. You might want to use it as a justification to
be clear.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/setup.c| 3 +-
> xen/arch/arm/static-shmem.c | 129 +---
> 2 files changed, 93 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index d242674381d4..f15d40a85a5f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -297,7 +297,8 @@ static bool __init meminfo_overlap_check(const struct
> membanks *mem,
> bank_start = mem->bank[i].start;
> bank_end = bank_start + mem->bank[i].size;
>
> -if ( region_end <= bank_start || region_start >= bank_end )
> +if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
> + region_start >= bank_end )
> continue;
> else
> {
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 24e40495a481..1c03bb7f1882 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> pbase = boot_shm_bank->start;
> psize = boot_shm_bank->size;
>
> +if ( INVALID_PADDR == pbase )
> +{
> +printk("%pd: host physical address must be chosen by users at
> the moment.", d);
The dot at the end is not needed.
> +return -EINVAL;
> +}
> +
> /*
> * xen,shared-mem = ;
> * TODO: pbase is optional.
> @@ -382,7 +388,8 @@ int __init process_shm_node(const void *fdt, int node,
> uint32_t address_cells,
> {
> const struct fdt_property *prop, *prop_id, *prop_role;
> const __be32 *cell;
> -paddr_t paddr, gaddr, size, end;
> +paddr_t paddr = INVALID_PADDR;
> +paddr_t gaddr, size, end;
> struct membanks *mem = bootinfo_get_shmem();
> struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra();
> unsigned int i;
> @@ -437,24 +444,37 @@ int __init process_shm_node(const void *fdt, int node,
> uint32_t address_cells,
> if ( !prop )
> return -ENOENT;
>
> +cell = (const __be32 *)prop->data;
> if ( len != dt_cells_to_size(address_cells + size_cells + address_cells)
> )
> {
> -if ( len == dt_cells_to_size(size_cells + address_cells) )
> -printk("fdt: host physical address must be chosen by users at
> the moment.\n");
> -
> -printk("fdt: invalid `xen,shared-mem` property.\n");
> -return -EINVAL;
> +if ( len == dt_cells_to_size(address_cells + size_cells) )
> +device_tree_get_reg(, address_cells, size_cells, ,
> +);
> +else
> +{
> +printk("fdt: invalid `xen,shared-mem` property.\n");
> +return -EINVAL;
> +}
> }
> +else
> +{
> +device_tree_get_reg(, address_cells, address_cells, ,
> +);
> +size = dt_next_cell(size_cells, );
>
> -cell = (const __be32 *)prop->data;
> -device_tree_get_reg(, address_cells, address_cells, , );
> -size = dt_next_cell(size_cells, );
> +if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
> +{
> +printk("fdt: physical address 0x%"PRIpaddr" is not suitably
> aligned.\n",
> +paddr);
> +return -EINVAL;
> +}
>
> -if ( !IS_ALIGNED(paddr, PAGE_SIZE) )
> -{
> -printk("fdt: physical address 0x%"PRIpaddr" is not suitably
> aligned.\n",
> - paddr);
> -return -EINVAL;
> +end = paddr + size;
> +if (
Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
On 07/05/2024 16:15, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>
>
> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
>if ( dt_property_read_string(shm_node, "role", _str) == 0 )
>owner_dom_io = false;
Looking at owner_dom_io, why don't you move parsing role and setting
owner_dom_io accordingly to handle_shared_mem_bank()?
>>>
>>> I think I wanted to keep all dt_* functions on the same level inside
>>> process_shm, otherwise yes, I could
>>> pass down shm_node and do the reading of role_str in
>>> handle_shared_mem_bank, or I could derive
>>> owner_dom_io from role_str being passed or not, something like:
>>>
>>> role_str = NULL;
>>> dt_property_read_string(shm_node, "role", _str)
>>>
>>> [inside handle_shared_mem_bank]:
>>> If ( role_str )
>>>owner_dom_io = false;
>>>
>>> And pass only role_str to handle_shared_mem_bank.
>>>
>>> Is this comment to reduce the number of parameters passed? I guess it’s not
>>> for where we call
>> In this series as well as the previous one you limit the number of arguments
>> passed to quite a few functions.
>> So naturally I would expect it to be done here as well. owner_dom_io is used
>> only by handle_shared_mem_bank, so it makes more sense to move parsing to
>> this
>> function so that it is self-contained.
>
> Ok I will, just to be on the same page here, you mean having
> dt_property_read_string inside handle_shared_mem_bank?
> Or the above example would work for you as well? That one would have role_str
> passed instead of shm_node.
I'm ok with the solution above to pass role_str.
~Michal
Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
On 07/05/2024 15:57, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>>>
>>> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>>> + bool owner_dom_io,
>>> + const char *role_str,
>>> + const struct membank *shm_bank)
>>> +{
>>> +paddr_t pbase, psize;
>>> +int ret;
>>> +
>>> +BUG_ON(!shm_bank);
>> not needed
>>
>>> +
>>> +pbase = shm_bank->start;
>>> +psize = shm_bank->size;
>> please add empty line here
>
> Will do
>>>
>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>>>const struct dt_device_node *node)
>>> {
>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct
>>> kernel_info *kinfo,
>>> if ( dt_property_read_string(shm_node, "role", _str) == 0 )
>>> owner_dom_io = false;
>> Looking at owner_dom_io, why don't you move parsing role and setting
>> owner_dom_io accordingly to handle_shared_mem_bank()?
>
> I think I wanted to keep all dt_* functions on the same level inside
> process_shm, otherwise yes, I could
> pass down shm_node and do the reading of role_str in handle_shared_mem_bank,
> or I could derive
> owner_dom_io from role_str being passed or not, something like:
>
> role_str = NULL;
> dt_property_read_string(shm_node, "role", _str)
>
> [inside handle_shared_mem_bank]:
> If ( role_str )
> owner_dom_io = false;
>
> And pass only role_str to handle_shared_mem_bank.
>
> Is this comment to reduce the number of parameters passed? I guess it’s not
> for where we call
In this series as well as the previous one you limit the number of arguments
passed to quite a few functions.
So naturally I would expect it to be done here as well. owner_dom_io is used
only by handle_shared_mem_bank, so it makes more sense to move parsing to this
function so that it is self-contained.
~Michal
Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
On 07/05/2024 15:44, Luca Fancellu wrote:
>
>
> Hi Michal,
>
> Thanks for your review.
>
>> On 6 May 2024, at 14:24, Michal Orzel wrote:
>>
>> Hi Luca,
>>
>> On 23/04/2024 10:25, Luca Fancellu wrote:
>>>
>>>
>>> The current static shared memory code is using bootinfo banks when it
>>> needs to find the number of borrower, so every time assign_shared_memory
>> s/borrower/borrowers
>
> Will fix
>
>>
>>> is called, the bank is searched in the bootinfo.shmem structure.
>>>
>>> There is nothing wrong with it, however the bank can be used also to
>>> retrieve the start address and size and also to pass less argument to
>>> assign_shared_memory. When retrieving the information from the bootinfo
>>> bank, it's also possible to move the checks on alignment to
>>> process_shm_node in the early stages.
>> Is this change really required for what you want to achieve? At the moment
>> the alignment checks
>> are done before first use, which requires these values to be aligned. FDT
>> processing part does not need it.
>
> That’s true, but it would separate better the parsing part, in the end what
> is the point of failing later if, for example,
> some value are passed but not aligned?
>
>>
>>>
>>> So create a new function find_shm() which takes a 'struct shared_meminfo'
>> Can we name it find_shm_bank() or find_shm_bank_by_id()?
>> I agree that it's better to use a unique ID rather than matching by
>> address/size
>
> Yes either names are good for me, I would use find_shm_bank_by_id
>
>>
>>> structure and the shared memory ID, to look for a bank with a matching ID,
>>> take the physical host address and size from the bank, pass the bank to
>>> assign_shared_memory() removing the now unnecessary arguments and finally
>>> remove the acquire_nr_borrower_domain() function since now the information
>>> can be extracted from the passed bank.
>>> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
>>> case of errors (unlikely), as said above, move the checks on alignment
>>> to process_shm_node.
>>>
>>> Drawback of this change is that now the bootinfo are used also when the
>>> bank doesn't need to be allocated, however it will be convinient later
>>> to use it as an argument for assign_shared_memory when dealing with
>>> the use case where the Host physical address is not supplied by the user.
>>>
>>> Signed-off-by: Luca Fancellu
>>> ---
>>> xen/arch/arm/static-shmem.c | 105
>>> 1 file changed, 58 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>>> index 09f474ec6050..f6cf74e58a83 100644
>>> --- a/xen/arch/arm/static-shmem.c
>>> +++ b/xen/arch/arm/static-shmem.c
>>> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
>>> offsetof(struct shared_meminfo, bank)));
>>> }
>>>
>>> -static int __init acquire_nr_borrower_domain(struct domain *d,
>>> - paddr_t pbase, paddr_t psize,
>>> - unsigned long *nr_borrowers)
>>> +static const struct membank __init *find_shm(const struct membanks *shmem,
>>> + const char *shm_id)
>>> {
>>> -const struct membanks *shmem = bootinfo_get_shmem();
>>> unsigned int bank;
>>>
>>> -/* Iterate reserved memory to find requested shm bank. */
>>> +BUG_ON(!shmem || !shm_id);
>> Is it really necessary? For example, before calling find_shm(), strlen is
>> used on shm_id
>
> So, I guess I did that to have more robust code, in case someone changes the
> code in the
> future and perhaps removes something we rely on. If you object to them I will
> remove though,
> here and the other related points below.
>
>>
>>> +
>>> for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
>>> {
>>> -paddr_t bank_start = shmem->bank[bank].start;
>>> -paddr_t bank_size = shmem->bank[bank].size;
>>> -
>>> -if ( (pbase == bank_start) && (psize == bank_size) )
>>> +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
>>> + MAX_SHM_ID_LENGTH) == 0 )
>> Why not strcmp? AFAICS it's been valida
Re: [PATCH 3/7] xen/p2m: put reference for superpage
Hi Luca,
On 23/04/2024 10:25, Luca Fancellu wrote:
>
>
> From: Penny Zheng
>
> We are doing foreign memory mapping for static shared memory, and
> there is a great possibility that it could be super mapped.
> But today, p2m_put_l3_page could not handle superpages.
>
> This commits implements a new function p2m_put_superpage to handle superpages,
> specifically for helping put extra references for foreign superpages.
>
> Signed-off-by: Penny Zheng
> Signed-off-by: Luca Fancellu
> ---
> v1:
> - patch from
> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/
> ---
> xen/arch/arm/mmu/p2m.c | 58 +++---
> 1 file changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
> index 41fcca011cf4..479a80fbd4cf 100644
> --- a/xen/arch/arm/mmu/p2m.c
> +++ b/xen/arch/arm/mmu/p2m.c
> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain
> *p2m, gfn_t gfn,
> return rc;
> }
>
> -/*
> - * Put any references on the single 4K page referenced by pte.
> - * TODO: Handle superpages, for now we only take special references for leaf
> - * pages (specifically foreign ones, which can't be super mapped today).
> - */
> -static void p2m_put_l3_page(const lpae_t pte)
> +/* Put any references on the single 4K page referenced by mfn. */
> +static void p2m_put_l3_page(mfn_t mfn, unsigned type)
Shouldn't type be of p2m_type_t?
> {
> -mfn_t mfn = lpae_get_mfn(pte);
> -
> -ASSERT(p2m_is_valid(pte));
> -
> /*
> * TODO: Handle other p2m types
> *
> @@ -771,16 +763,53 @@ static void p2m_put_l3_page(const lpae_t pte)
> * flush the TLBs if the page is reallocated before the end of
> * this loop.
> */
> -if ( p2m_is_foreign(pte.p2m.type) )
> +if ( p2m_is_foreign(type) )
> {
> ASSERT(mfn_valid(mfn));
> put_page(mfn_to_page(mfn));
> }
> /* Detect the xenheap page and mark the stored GFN as invalid. */
> -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
> +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
> page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
> }
>
> +/* Put any references on the superpage referenced by mfn. */
> +static void p2m_put_superpage(mfn_t mfn, unsigned int next_level, unsigned
> type)
Shouldn't type be of p2m_type_t?
> +{
> +unsigned int i;
> +unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level);
> +
> +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
> +{
> +if ( next_level == 3 )
> +p2m_put_l3_page(mfn, type);
> +else
> +p2m_put_superpage(mfn, next_level + 1, type);
> +
> +mfn = mfn_add(mfn, 1 << level_order);
> +}
> +}
> +
> +/* Put any references on the page referenced by pte. */
> +static void p2m_put_page(const lpae_t pte, unsigned int level)
> +{
> +mfn_t mfn = lpae_get_mfn(pte);
> +
> +ASSERT(p2m_is_valid(pte));
> +
> +/*
> + * We are either having a first level 1G superpage or a
> + * second level 2M superpage.
> + */
> +if ( p2m_is_superpage(pte, level) )
> +return p2m_put_superpage(mfn, level + 1, pte.p2m.type);
> +else
No need for this else
> +{
> +ASSERT(level == 3);
> +return p2m_put_l3_page(mfn, pte.p2m.type);
> +}
> +}
> +
> /* Free lpae sub-tree behind an entry */
> static void p2m_free_entry(struct p2m_domain *p2m,
> lpae_t entry, unsigned int level)
> @@ -809,9 +838,8 @@ static void p2m_free_entry(struct p2m_domain *p2m,
> #endif
>
> p2m->stats.mappings[level]--;
> -/* Nothing to do if the entry is a super-page. */
> -if ( level == 3 )
> -p2m_put_l3_page(entry);
> +p2m_put_page(entry, level);
> +
> return;
> }
>
> --
> 2.34.1
>
~Michal
Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
Hi Luca,
On 23/04/2024 10:25, Luca Fancellu wrote:
>
>
> Wrap the code and logic that is calling assign_shared_memory
> and map_regions_p2mt into a new function 'handle_shared_mem_bank',
> it will become useful later when the code will allow the user to
> don't pass the host physical address.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/static-shmem.c | 71 +++--
> 1 file changed, 45 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index f6cf74e58a83..24e40495a481 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo
> *kinfo_shm_mem, paddr_t start,
> return 0;
> }
>
> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
> + bool owner_dom_io,
> + const char *role_str,
> + const struct membank *shm_bank)
> +{
> +paddr_t pbase, psize;
> +int ret;
> +
> +BUG_ON(!shm_bank);
not needed
> +
> +pbase = shm_bank->start;
> +psize = shm_bank->size;
please add empty line here
> +/*
> + * DOMID_IO is a fake domain and is not described in the Device-Tree.
> + * Therefore when the owner of the shared region is DOMID_IO, we will
> + * only find the borrowers.
> + */
> +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
> + (!owner_dom_io && strcmp(role_str, "owner") == 0) )
> +{
> +/*
> + * We found the first borrower of the region, the owner was not
> + * specified, so they should be assigned to dom_io.
> + */
> +ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase,
> shm_bank);
> +if ( ret )
> +return ret;
> +}
> +
> +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) )
> +{
> +/* Set up P2M foreign mapping for borrower domain. */
> +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize),
> + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw);
> +if ( ret )
> +return ret;
> +}
> +
> +return 0;
> +}
> +
> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> if ( dt_property_read_string(shm_node, "role", _str) == 0 )
> owner_dom_io = false;
Looking at owner_dom_io, why don't you move parsing role and setting
owner_dom_io accordingly to handle_shared_mem_bank()?
~Michal
Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
Hi Luca,
On 23/04/2024 10:25, Luca Fancellu wrote:
>
>
> The current static shared memory code is using bootinfo banks when it
> needs to find the number of borrower, so every time assign_shared_memory
s/borrower/borrowers
> is called, the bank is searched in the bootinfo.shmem structure.
>
> There is nothing wrong with it, however the bank can be used also to
> retrieve the start address and size and also to pass less argument to
> assign_shared_memory. When retrieving the information from the bootinfo
> bank, it's also possible to move the checks on alignment to
> process_shm_node in the early stages.
Is this change really required for what you want to achieve? At the moment the
alignment checks
are done before first use, which requires these values to be aligned. FDT
processing part does not need it.
>
> So create a new function find_shm() which takes a 'struct shared_meminfo'
Can we name it find_shm_bank() or find_shm_bank_by_id()?
I agree that it's better to use a unique ID rather than matching by address/size
> structure and the shared memory ID, to look for a bank with a matching ID,
> take the physical host address and size from the bank, pass the bank to
> assign_shared_memory() removing the now unnecessary arguments and finally
> remove the acquire_nr_borrower_domain() function since now the information
> can be extracted from the passed bank.
> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in
> case of errors (unlikely), as said above, move the checks on alignment
> to process_shm_node.
>
> Drawback of this change is that now the bootinfo are used also when the
> bank doesn't need to be allocated, however it will be convinient later
> to use it as an argument for assign_shared_memory when dealing with
> the use case where the Host physical address is not supplied by the user.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/static-shmem.c | 105
> 1 file changed, 58 insertions(+), 47 deletions(-)
>
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 09f474ec6050..f6cf74e58a83 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void)
> offsetof(struct shared_meminfo, bank)));
> }
>
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - unsigned long *nr_borrowers)
> +static const struct membank __init *find_shm(const struct membanks *shmem,
> + const char *shm_id)
> {
> -const struct membanks *shmem = bootinfo_get_shmem();
> unsigned int bank;
>
> -/* Iterate reserved memory to find requested shm bank. */
> +BUG_ON(!shmem || !shm_id);
Is it really necessary? For example, before calling find_shm(), strlen is used
on shm_id
> +
> for ( bank = 0 ; bank < shmem->nr_banks; bank++ )
> {
> -paddr_t bank_start = shmem->bank[bank].start;
> -paddr_t bank_size = shmem->bank[bank].size;
> -
> -if ( (pbase == bank_start) && (psize == bank_size) )
> +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id,
> + MAX_SHM_ID_LENGTH) == 0 )
Why not strcmp? AFAICS it's been validated many times already
> break;
> }
>
> if ( bank == shmem->nr_banks )
> -return -ENOENT;
> -
> -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers;
> +return NULL;
>
> -return 0;
> +return >bank[bank];
> }
>
> /*
> @@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct
> domain *d,
> return smfn;
> }
>
> -static int __init assign_shared_memory(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> - paddr_t gbase)
> +static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
> + const struct membank *shm_bank)
> {
> mfn_t smfn;
> int ret = 0;
> unsigned long nr_pages, nr_borrowers, i;
> struct page_info *page;
> +paddr_t pbase, psize;
> +
> +BUG_ON(!shm_bank || !shm_bank->shmem_extra);
Is it really necessary? Isn't shm_bank already validated? It's not very common
to have NULL checks in internal functions.
> +
> +pbase = shm_bank->start;
> +psize = shm_bank->size;
> +nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>
> printk("%pd: allocate static shared memory BANK
> %#"PRIpaddr"-%#"PRIpaddr".\n",
> d, pbase, pbase + psize);
> @@ -135,14 +136,6 @@ static int __init assign_shared_memory(struct domain *d,
> }
> }
>
> -/*
> - * Get the right amount of references per page, which is the number of
> - * borrower domains.
> - */
> -ret
Re: [PATCH] arm/vpci: make prefetchable mem 64 bit
Hi Stewart, On 24/04/2024 18:27, Stewart Hildebrand wrote: > The vPCI prefetchable memory range is >= 4GB, so the memory space flags > should be set to 64-bit. See IEEE Std 1275-1994 [1] for a definition of NIT: would be beneficial to provide chapter (in this case 2.2.1.1) > the field. > > [1] https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf > > Signed-off-by: Stewart Hildebrand Acked-by: Michal Orzel ~Michal > --- > xen/include/public/arch-arm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index e167e14f8df9..289af81bd69d 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -487,7 +487,7 @@ typedef uint64_t xen_callback_t; > #define GUEST_RAM0_SIZE xen_mk_ullong(0xc000) > > /* 4GB @ 4GB Prefetch Memory for VPCI */ > -#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x4200) > +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x4300) > #define GUEST_VPCI_PREFETCH_MEM_ADDRxen_mk_ullong(0x1) > #define GUEST_VPCI_PREFETCH_MEM_SIZExen_mk_ullong(0x1) > > > base-commit: 410ef3343924b5a3928bbe8e392491992b322cf0
[PATCH 3/3] automation: Add arm64 test for running Xen with GICv3
At the moment, all the Arm64 Qemu tests use GICv2 which is the default
GIC version used by Qemu. Improve the coverage by adding a new test in
which Qemu will be configured to have GICv3.
Rename host device tree name to "virt.dtb" to be GIC version agnostic.
Use "gic-version" Qemu option to select the version to use. Unless the
test variant is set to "gicv3", version 2 will be used.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/test.yaml| 8
.../scripts/qemu-smoke-dom0less-arm64.sh | 19 ++-
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 1e5d86763f6c..ad249fa0a5d9 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -255,6 +255,14 @@ qemu-smoke-dom0less-arm64-gcc-debug:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
+qemu-smoke-dom0less-arm64-gcc-debug-gicv3:
+ extends: .qemu-arm64
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm64.sh gicv3 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm64-test-needs
+- alpine-3.18-gcc-debug-arm64
+
qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
extends: .qemu-arm64
script:
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh
b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index fc943a1a622c..292c38a56147 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -4,6 +4,9 @@ set -ex
test_variant=$1
+# Default GIC version
+gic_version="2"
+
if [ -z "${test_variant}" ]; then
passed="ping test passed"
domU_check="
@@ -66,16 +69,22 @@ if [[ "${test_variant}" == "earlyprintk" ]]; then
passed="\- Ready \-"
fi
+if [[ "${test_variant}" == "gicv3" ]]; then
+gic_version=3
+passed="${test_variant} test passed"
+domU_check="echo \"${passed}\""
+fi
+
# XXX QEMU looks for "efi-virtio.rom" even if it is unneeded
curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
./binaries/qemu-system-aarch64 \
-machine virtualization=true \
- -cpu cortex-a57 -machine type=virt \
+ -cpu cortex-a57 -machine type=virt,gic-version=$gic_version \
-m 2048 -smp 2 -display none \
- -machine dumpdtb=binaries/virt-gicv2.dtb
+ -machine dumpdtb=binaries/virt.dtb
# XXX disable pl061 to avoid Linux crash
-fdtput binaries/virt-gicv2.dtb -p -t s /pl061@903 status disabled
+fdtput binaries/virt.dtb -p -t s /pl061@903 status disabled
# Busybox
mkdir -p initrd
@@ -138,7 +147,7 @@ cd ..
echo 'MEMORY_START="0x4000"
MEMORY_END="0x5000"
-DEVICE_TREE="virt-gicv2.dtb"
+DEVICE_TREE="virt.dtb"
XEN="xen"
DOM0_KERNEL="Image"
DOM0_RAMDISK="dom0-rootfs.cpio.gz"
@@ -200,7 +209,7 @@ echo " virtio scan; dhcp; tftpb 0x4000 boot.scr;
source 0x4000"| \
timeout -k 1 240 \
./binaries/qemu-system-aarch64 \
-machine virtualization=true \
--cpu cortex-a57 -machine type=virt \
+-cpu cortex-a57 -machine type=virt,gic-version=$gic_version \
-m 2048 -monitor none -serial stdio \
-smp 2 \
-no-reboot \
--
2.25.1
[PATCH 2/3] automation: Add arm{64,32} earlyprintk jobs
Introduce qemu based Arm earlyprintk test and build jobs to cover this
feature in debug variant. The tests simply check for the presence of the
last message printed by the bootstrap code before entering the C world.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/build.yaml | 17 +
automation/gitlab-ci/test.yaml | 16
automation/scripts/qemu-smoke-dom0less-arm32.sh | 7 +++
automation/scripts/qemu-smoke-dom0less-arm64.sh | 5 +
4 files changed, 45 insertions(+)
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index f3c934471f32..49d6265ad5b4 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -402,6 +402,15 @@ debian-bookworm-gcc-arm32-debug-staticmem:
CONFIG_UNSUPPORTED=y
CONFIG_STATIC_MEMORY=y
+debian-bookworm-gcc-arm32-debug-earlyprintk:
+ extends: .gcc-arm32-cross-build-debug
+ variables:
+CONTAINER: debian:bookworm-arm64v8-arm32-gcc
+HYPERVISOR_ONLY: y
+EXTRA_XEN_CONFIG: |
+ CONFIG_EARLY_UART_CHOICE_PL011=y
+ CONFIG_EARLY_UART_BASE_ADDRESS=0x900
+
# Arm builds
debian-bookworm-gcc-arm64:
@@ -473,6 +482,14 @@ alpine-3.18-gcc-debug-arm64-boot-cpupools:
EXTRA_XEN_CONFIG: |
CONFIG_BOOT_TIME_CPUPOOLS=y
+alpine-3.18-gcc-debug-arm64-earlyprintk:
+ extends: .gcc-arm64-build-debug
+ variables:
+CONTAINER: alpine:3.18-arm64v8
+EXTRA_XEN_CONFIG: |
+ CONFIG_EARLY_UART_CHOICE_PL011=y
+ CONFIG_EARLY_UART_BASE_ADDRESS=0x900
+
# RISC-V 64 cross-build
.riscv-fixed-randconfig:
variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 55a7831ad292..1e5d86763f6c 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -287,6 +287,14 @@ qemu-smoke-dom0less-arm64-gcc-debug-boot-cpupools:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-boot-cpupools
+qemu-smoke-dom0less-arm64-gcc-debug-earlyprintk:
+ extends: .qemu-arm64
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm64.sh earlyprintk 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm64-test-needs
+- alpine-3.18-gcc-debug-arm64-earlyprintk
+
qemu-xtf-dom0less-arm64-gcc-hyp-xen-version:
extends: .qemu-arm64
script:
@@ -359,6 +367,14 @@ qemu-smoke-dom0less-arm32-gcc-debug-without-dom0:
- *arm32-test-needs
- debian-bookworm-gcc-arm32-debug
+qemu-smoke-dom0less-arm32-gcc-debug-earlyprintk:
+ extends: .qemu-arm32
+ script:
+- ./automation/scripts/qemu-smoke-dom0less-arm32.sh earlyprintk 2>&1 | tee
${LOGFILE}
+ needs:
+- *arm32-test-needs
+- debian-bookworm-gcc-arm32-debug-earlyprintk
+
qemu-alpine-x86_64-gcc:
extends: .qemu-x86-64
script:
diff --git a/automation/scripts/qemu-smoke-dom0less-arm32.sh
b/automation/scripts/qemu-smoke-dom0less-arm32.sh
index e31b6b9014e1..1e2b939aadf7 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm32.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm32.sh
@@ -53,6 +53,13 @@ echo \"${passed}\"
"
fi
+if [[ "${test_variant}" == "earlyprintk" ]]; then
+# Clear dom0 prompt
+dom0_prompt=""
+# Last early printk message before entering C world
+passed="\- Ready \-"
+fi
+
# dom0/domU rootfs
# We are using the same rootfs for dom0 and domU. The only difference is
# that for the former, we set explictly rdinit to /bin/sh, whereas for the
diff --git a/automation/scripts/qemu-smoke-dom0less-arm64.sh
b/automation/scripts/qemu-smoke-dom0less-arm64.sh
index e748b8ef1699..fc943a1a622c 100755
--- a/automation/scripts/qemu-smoke-dom0less-arm64.sh
+++ b/automation/scripts/qemu-smoke-dom0less-arm64.sh
@@ -61,6 +61,11 @@ fi
"
fi
+if [[ "${test_variant}" == "earlyprintk" ]]; then
+# Last early printk message before entering C world
+passed="\- Ready \-"
+fi
+
# XXX QEMU looks for "efi-virtio.rom" even if it is unneeded
curl -fsSLO https://github.com/qemu/qemu/raw/v5.2.0/pc-bios/efi-virtio.rom
./binaries/qemu-system-aarch64 \
--
2.25.1
[PATCH 0/3] automation: improve Arm coverage
This came up as part of the following discussion:
https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2402291108290.853156@ubuntu-linux-20-04-desktop/
Michal Orzel (3):
automation: Drop some of the non-debug variants of the same Arm jobs
automation: Add arm{64,32} earlyprintk jobs
automation: Add arm64 test for running Xen with GICv3
automation/gitlab-ci/build.yaml | 41 --
automation/gitlab-ci/test.yaml| 56 ++-
.../scripts/qemu-smoke-dom0less-arm32.sh | 7 +++
.../scripts/qemu-smoke-dom0less-arm64.sh | 24 ++--
4 files changed, 52 insertions(+), 76 deletions(-)
--
2.25.1
[PATCH 1/3] automation: Drop some of the non-debug variants of the same Arm jobs
To save some bandwith that can be later on used to increase the test
coverage by adding new tests, drop the following non-debug test/build
jobs existing in both debug and non-debug variants:
- static memory (arm64, arm32)
- static shared memory (arm64)
- static heap (arm64)
- boot cpupools (arm64)
- gzip (arm32)
More generic tests existing in both variants were left unmodified.
Signed-off-by: Michal Orzel
---
automation/gitlab-ci/build.yaml | 38 --
automation/gitlab-ci/test.yaml | 48 -
2 files changed, 86 deletions(-)
diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index aac29ee13ab6..f3c934471f32 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -392,16 +392,6 @@ debian-bookworm-gcc-arm32-debug-randconfig:
HYPERVISOR_ONLY: y
RANDCONFIG: y
-debian-bookworm-gcc-arm32-staticmem:
- extends: .gcc-arm32-cross-build
- variables:
-CONTAINER: debian:bookworm-arm64v8-arm32-gcc
-HYPERVISOR_ONLY: y
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
-
debian-bookworm-gcc-arm32-debug-staticmem:
extends: .gcc-arm32-cross-build-debug
variables:
@@ -458,15 +448,6 @@ alpine-3.18-gcc-debug-arm64-randconfig:
CONTAINER: alpine:3.18-arm64v8
RANDCONFIG: y
-alpine-3.18-gcc-arm64-staticmem:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
-
alpine-3.18-gcc-debug-arm64-staticmem:
extends: .gcc-arm64-build-debug
variables:
@@ -476,15 +457,6 @@ alpine-3.18-gcc-debug-arm64-staticmem:
CONFIG_UNSUPPORTED=y
CONFIG_STATIC_MEMORY=y
-alpine-3.18-gcc-arm64-static-shared-mem:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_UNSUPPORTED=y
- CONFIG_STATIC_MEMORY=y
- CONFIG_STATIC_SHM=y
-
alpine-3.18-gcc-debug-arm64-static-shared-mem:
extends: .gcc-arm64-build-debug
variables:
@@ -494,16 +466,6 @@ alpine-3.18-gcc-debug-arm64-static-shared-mem:
CONFIG_STATIC_MEMORY=y
CONFIG_STATIC_SHM=y
-alpine-3.18-gcc-arm64-boot-cpupools:
- extends: .gcc-arm64-build
- variables:
-CONTAINER: alpine:3.18-arm64v8
-EXTRA_XEN_CONFIG: |
- CONFIG_EXPERT=y
- CONFIG_UNSUPPORTED=y
- CONFIG_SCHED_NULL=y
- CONFIG_BOOT_TIME_CPUPOOLS=y
-
alpine-3.18-gcc-debug-arm64-boot-cpupools:
extends: .gcc-arm64-build-debug
variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 8b7b2e4da92d..55a7831ad292 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -255,14 +255,6 @@ qemu-smoke-dom0less-arm64-gcc-debug:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
-qemu-smoke-dom0less-arm64-gcc-staticmem:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-mem 2>&1 | tee
${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-staticmem
-
qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
extends: .qemu-arm64
script:
@@ -271,14 +263,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-staticmem:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-staticmem
-qemu-smoke-dom0less-arm64-gcc-staticheap:
- extends: .qemu-arm64
- script:
- - ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-heap 2>&1 | tee
${LOGFILE}
- needs:
- - *arm64-test-needs
- - alpine-3.18-gcc-arm64
-
qemu-smoke-dom0less-arm64-gcc-debug-staticheap:
extends: .qemu-arm64
script:
@@ -287,14 +271,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-staticheap:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64
-qemu-smoke-dom0less-arm64-gcc-static-shared-mem:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh static-shared-mem 2>&1
| tee ${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-static-shared-mem
-
qemu-smoke-dom0less-arm64-gcc-debug-static-shared-mem:
extends: .qemu-arm64
script:
@@ -303,14 +279,6 @@ qemu-smoke-dom0less-arm64-gcc-debug-static-shared-mem:
- *arm64-test-needs
- alpine-3.18-gcc-debug-arm64-static-shared-mem
-qemu-smoke-dom0less-arm64-gcc-boot-cpupools:
- extends: .qemu-arm64
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm64.sh boot-cpupools 2>&1 |
tee ${LOGFILE}
- needs:
-- *arm64-test-needs
-- alpine-3.18-gcc-arm64-boot-cpupools
-
qemu-smoke-dom0less-arm64-gcc-debug-boot-cpupools:
extends: .qemu-arm64
script:
@@ -359,14 +327,6 @@ qemu-smoke-dom0less-arm32-gcc-debug:
- *arm32-test-needs
- debian-bookworm-gcc-arm32-debug
-qemu-smoke-dom0less-arm32-gcc-staticmem:
- extends: .qemu-arm32
- script:
-- ./automation/scripts/qemu-smoke-dom0less-arm32.sh static-mem 2&g
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
On 22/04/2024 10:07, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>>> +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells +=
>>> reg_size )
>>> +{
>>> +u64 start = dt_read_number(cells, addrcells);
>> We should no longer use Linux derived types like u64. Use uint64_t.
>>
>>> +u64 size = dt_read_number(cells + addrcells, sizecells);
>>> +
>>> +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n",
>>> + i, start, start + size);
>> i is unsigned so the correct format specifier should be %u
>
> Right, should have been more careful when copying the code from above
>
>>>
>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
>>> +__be32 *reg, int *nr_cells,
>>> +int addrcells, int sizecells)
>>> +{
>>> +const struct membanks *mem = >shm_mem.common;
>>> +unsigned int i;
>>> +__be32 *cells;
>>> +
>>> +BUG_ON(!nr_cells || !reg);
>>> +
>>> +cells = [*nr_cells];
>>> +for ( i = 0; i < mem->nr_banks; i++ )
>>> +{
>>> +u64 start = mem->bank[i].start;
>> ditto
>
> Will fix, here paddr_t should be ok isn’t it?
yes
>
>>
>> Rest LGTM:
>> Reviewed-by: Michal Orzel
>
> Thanks, I will send the next one shortly.
I don't think there is a need to respin the whole series just for these fixes.
You should wait for the committers opinion.
~Michal
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
> if ( reserved_mem->nr_banks > 0 )
> {
> -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
> - reserved_mem);
> +res = make_memory_node(kinfo, addrcells, sizecells,
> reserved_mem);
> if ( res )
> return res;
> }
> diff --git a/xen/arch/arm/include/asm/domain_build.h
> b/xen/arch/arm/include/asm/domain_build.h
> index 026d975da28e..45936212ca21 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -14,7 +14,7 @@ int make_chosen_node(const struct kernel_info *kinfo);
> int make_cpus_node(const struct domain *d, void *fdt);
> int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
> int addrcells, int sizecells);
> -int make_memory_node(const struct domain *d, void *fdt, int addrcells,
> +int make_memory_node(const struct kernel_info *kinfo, int addrcells,
> int sizecells, const struct membanks *mem);
> int make_psci_node(void *fdt);
> int make_timer_node(const struct kernel_info *kinfo);
> diff --git a/xen/arch/arm/include/asm/static-shmem.h
> b/xen/arch/arm/include/asm/static-shmem.h
> index 7495a91e7a31..3b6569e5703f 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -3,10 +3,15 @@
> #ifndef __ASM_STATIC_SHMEM_H_
> #define __ASM_STATIC_SHMEM_H_
>
> +#include
> #include
> +#include
>
> #ifdef CONFIG_STATIC_SHM
>
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4)
> +
> int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>int sizecells);
>
> @@ -37,8 +42,14 @@ int remove_shm_holes_for_domU(const struct kernel_info
> *kinfo,
> int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
>int sizecells);
>
> +void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32
> *reg,
> + int *nr_cells, int addrcells, int
> sizecells);
> +
> #else /* !CONFIG_STATIC_SHM */
>
> +/* Worst case /memory node reg element: (addrcells + sizecells) */
> +#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4)
> +
> static inline int make_resv_memory_node(const struct kernel_info *kinfo,
> int addrcells, int sizecells)
> {
> @@ -86,6 +97,10 @@ static inline int make_shm_resv_memory_node(const struct
> kernel_info *kinfo,
> return 0;
> }
>
> +static inline void shm_mem_node_fill_reg_range(const struct kernel_info
> *kinfo,
> + __be32 *reg, int *nr_cells,
> + int addrcells, int sizecells)
> {};
> +
> #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 c85f60dd1bf7..07c93a820508 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -1,5 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0-only */
>
> +#include
> #include
> #include
> #include
> @@ -668,6 +669,28 @@ int __init remove_shm_holes_for_domU(const struct
> kernel_info *kinfo,
> return res;
> }
>
> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
> +__be32 *reg, int *nr_cells,
> +int addrcells, int sizecells)
> +{
> +const struct membanks *mem = >shm_mem.common;
> +unsigned int i;
> +__be32 *cells;
> +
> +BUG_ON(!nr_cells || !reg);
> +
> +cells = [*nr_cells];
> +for ( i = 0; i < mem->nr_banks; i++ )
> +{
> +u64 start = mem->bank[i].start;
ditto
Rest LGTM:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v3 10/12] xen/arm: remove shm holes from extended regions
On 18/04/2024 09:36, Luca Fancellu wrote: > > > From: Penny Zheng > > 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 find_unallocated_memory, we do nothing. > > Signed-off-by: Penny Zheng > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v3 06/12] xen/arm: Avoid code duplication in find_unallocated_memory
Hi Luca, On 18/04/2024 09:36, Luca Fancellu wrote: > > > The function find_unallocated_memory is using the same code to > loop through 2 structure of the same type, in order to avoid > code duplication, rework the code to have only one loop that > goes through all the structures, this will be used to avoid > duplication when the static shared memory banks will be introduced > as a separate structure from reserved memory. > > Take the occasion to add the error code to the error message in > case 'rangeset_remove_range' fails. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH] public: xen: Define missing guest handle for int32_t
Hi Jan,
On 18/04/2024 09:26, Jan Beulich wrote:
>
>
> On 17.04.2024 14:14, Michal Orzel wrote:
>> Commit afab29d0882f ("public: s/int/int32_t") replaced int with int32_t
>> in XEN_GUEST_HANDLE() in memory.h but there is no guest handle defined
>> for it. This results in a build failure. Example on Arm:
>>
>> ./include/public/arch-arm.h:205:41: error: unknown type name
>> ‘__guest_handle_64_int32_t’
>> 205 | #define __XEN_GUEST_HANDLE(name)__guest_handle_64_ ## name
>> | ^~
>> ./include/public/arch-arm.h:206:41: note: in expansion of macro
>> ‘__XEN_GUEST_HANDLE’
>> 206 | #define XEN_GUEST_HANDLE(name) __XEN_GUEST_HANDLE(name)
>> | ^~
>> ./include/public/memory.h:277:5: note: in expansion of macro
>> ‘XEN_GUEST_HANDLE’
>> 277 | XEN_GUEST_HANDLE(int32_t) errs;
>>
>> Fix it. Also, drop guest handle definition for int given no further use.
>
> Such cannot be done like this. Consumers of the header may have grown their
> own uses. The declaration needs to remain active for any consumers
> supplying __XEN_INTERFACE_VERSION__ < 0x00041900. Which means you need to
> bump __XEN_LATEST_INTERFACE_VERSION__ and wrap the handle type declaration
> in #ifdef. Provided the removal of that handle type for newer interface
> versions is really warranting all this effort.
I think we could just leave this guest handle definition as is then.
~Michal
Re: [ImageBuilder 5/5] uboot-script-gen: Add ability to specify "nr_spis"
On 17/04/2024 14:07, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko > > This is needed to have a possibility of assigning a specified number > of shared peripheral interrupts (SPIs) to domain. > > Signed-off-by: Oleksandr Tyshchenko > Signed-off-by: Stefano Stabellini Reviewed-by: Michal Orzel ~Michal
Re: [ImageBuilder 4/5] uboot-script-gen: Add ability to unselect "vpl011"
On 17/04/2024 14:07, Oleksandr Tyshchenko wrote:
>
>
> From: Oleksandr Tyshchenko
>
> Introduce new option DOMU_VPL011[nr] that can be set to 0
> or 1 (default).
>
> Also align "console=ttyAMA0" Linux cmd arg setting with "vpl011" presense.
>
> Suggested-by: Michal Orzel
> Signed-off-by: Oleksandr Tyshchenko
Reviewed-by: Michal Orzel
That said, I noticed ...
> ---
> README.md| 7 ++-
> scripts/uboot-script-gen | 7 +--
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/README.md b/README.md
> index b2459fd..63c4708 100644
> --- a/README.md
> +++ b/README.md
> @@ -151,7 +151,8 @@ Where:
> - DOMU_KERNEL[number] specifies the DomU kernel to use.
>
> - DOMU_CMD[number] specifies the command line arguments for DomU's Linux
> - kernel. If not set, then "console=ttyAMA0" is used.
> + kernel. If not set and DOMU_VPL011[number] is not set to 0, then
> + "console=ttyAMA0" is used.
>
> - DOMU_RAMDISK[number] specifies the DomU ramdisk to use.
>
> @@ -232,6 +233,10 @@ Where:
> - DOMU_MAPTRACK_FRAMES[number] is optional but specifies the maximum number
>of grant maptrack frames (the default value used by Xen on Arm64 is 1024)
>
> +- DOMU_VPL011[number] is optional but used to enable (1)/disable (0) a
> virtual
> + PL011 UART for domain. The default is 1. If explicitly set to 0, then
> + "console=ttyAMA0" is not used as a default DOMU_CMD[number].
> +
> - DOMU_CPUPOOL[number] specifies the id of the cpupool (created using
>CPUPOOL[number] option, where number == id) that will be assigned to domU.
>
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index adec6f9..fd37e18 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -344,7 +344,10 @@ function xen_device_tree_editing()
> add_device_tree_static_mem "/chosen/domU$i"
> "${DOMU_STATIC_MEM[$i]}"
> dt_set "/chosen/domU$i" "direct-map" "bool"
> "${DOMU_DIRECT_MAP[$i]}"
> fi
> -dt_set "/chosen/domU$i" "vpl011" "hex" "0x1"
> +if test -z "${DOMU_VPL011[$i]}" || test "${DOMU_VPL011[$i]}" -eq "1"
> +then
> +dt_set "/chosen/domU$i" "vpl011" "hex" "0x1"
... that the property type is incorrect. According to Xen docs, this should be
a bool property.
@Stefano, can you fix it on commit?
~Michal
Re: [ImageBuilder 3/5] uboot-script-gen: Add ability to specify grant table params
On 17/04/2024 14:07, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko > > Use DOMU_GRANT_VER to set "max_grant_version" dt property. > Use DOMU_GRANT_FRAMES to set "max_grant_frames" dt property. > Use DOMU_MAPTRACK_FRAMES to set "max_maptrack_frames" dt property. > > Signed-off-by: Oleksandr Tyshchenko Reviewed-by: Michal Orzel ~Michal
Re: [ImageBuilder 2/5] uboot-script-gen: Extend DOMU_ENHANCED to specify "no-xenstore"
On 17/04/2024 14:07, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko > > We need some Xen services to be available within single dom0less DomU. > Just using "enabled" will lead to Xen panic because of no Dom0. > > (XEN) > (XEN) Panic on CPU 0: > (XEN) At the moment, Xenstore support requires dom0 to be present > (XEN) > > Signed-off-by: Oleksandr Tyshchenko Reviewed-by: Michal Orzel ~Michal
Re: [ImageBuilder 1/5] uboot-script-gen: Update to deal with uImage which is not executable
On 17/04/2024 14:07, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko > > uImage is the Image that has a U-Boot wrapper, it doesn't contain > "executable" string which subsequent "file" command is looking for > when inspecting it. > > Below the proof: > > otyshchenko@EPUAKYIW03DD:~/work/xen_tests/input$ file -L binaries/uImage.gz > binaries/uImage.gz: u-boot legacy uImage, Linux Kernel Image, Linux/ARM > 64-bit, > OS Kernel Image (gzip), 9822180 bytes, Fri Sep 29 15:39:42 2023, Load > Address: 0X4000, > Entry Point: 0X4000, Header CRC: 0XE1EF21BF, Data CRC: 0XC418025 > > otyshchenko@EPUAKYIW03DD:~/work/xen_tests/input$ file -L binaries/uImage > binaries/uImage: u-boot legacy uImage, Linux Kernel Image, Linux/ARM 64-bit, > OS Kernel Image (Not compressed), 23269888 bytes, Fri Sep 29 15:40:19 2023, > Load Address: 0X4000, Entry Point: 0X4000, Header CRC: 0XA0B7D051, > Data CRC: 0X42083F51 > > Suggested-by: Stefano Stabellini > Signed-off-by: Oleksandr Tyshchenko Reviewed-by: Michal Orzel ~Michal
[PATCH] public: xen: Define missing guest handle for int32_t
Commit afab29d0882f ("public: s/int/int32_t") replaced int with int32_t
in XEN_GUEST_HANDLE() in memory.h but there is no guest handle defined
for it. This results in a build failure. Example on Arm:
./include/public/arch-arm.h:205:41: error: unknown type name
‘__guest_handle_64_int32_t’
205 | #define __XEN_GUEST_HANDLE(name)__guest_handle_64_ ## name
| ^~
./include/public/arch-arm.h:206:41: note: in expansion of macro
‘__XEN_GUEST_HANDLE’
206 | #define XEN_GUEST_HANDLE(name) __XEN_GUEST_HANDLE(name)
| ^~
./include/public/memory.h:277:5: note: in expansion of macro ‘XEN_GUEST_HANDLE’
277 | XEN_GUEST_HANDLE(int32_t) errs;
Fix it. Also, drop guest handle definition for int given no further use.
Fixes: afab29d0882f ("public: s/int/int32_t")
Signed-off-by: Michal Orzel
---
xen/include/public/xen.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b47d48d0e2d6..8fd0cec880d5 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -28,7 +28,6 @@
/* Guest handles for primitive C types. */
DEFINE_XEN_GUEST_HANDLE(char);
__DEFINE_XEN_GUEST_HANDLE(uchar, unsigned char);
-DEFINE_XEN_GUEST_HANDLE(int);
__DEFINE_XEN_GUEST_HANDLE(uint, unsigned int);
#if __XEN_INTERFACE_VERSION__ < 0x00040300
DEFINE_XEN_GUEST_HANDLE(long);
@@ -36,6 +35,7 @@ __DEFINE_XEN_GUEST_HANDLE(ulong, unsigned long);
#endif
DEFINE_XEN_GUEST_HANDLE(void);
+DEFINE_XEN_GUEST_HANDLE(int32_t);
DEFINE_XEN_GUEST_HANDLE(uint64_t);
DEFINE_XEN_GUEST_HANDLE(xen_pfn_t);
DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
--
2.25.1
Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
Hi Julien, On 16/04/2024 10:50, Julien Grall wrote: > > > On 16/04/2024 07:27, Luca Fancellu wrote: >> Hi Julien, > > Hi Luca, > >> >>> On 15 Apr 2024, at 19:41, Julien Grall wrote: >>> >>> Hi Luca, >>> >>> On 09/04/2024 12:45, Luca Fancellu wrote: Currently Xen is not exporting the static shared memory regions to the device tree as /memory node, this commit is fixing this issue. The static shared memory banks can be part of the memory range available for the domain, so if they are overlapping with the normal memory banks, they need to be merged together in order to produce a /memory node with non overlapping ranges in 'reg'. >>> >>> Before reviewing the code in more details, I would like to understand a bit >>> more the use case and whether it should be valid. >>> >>> From my understanding, the case you are trying to prevent is the following >>> setup: >>> 1. The Guest Physical region 0x to 0x8000 is used for RAM >>> 2. The Guest Physical region 0x to 0x4000 is used for static memory >> >> So far, it was possible to map guest physical regions inside the memory >> range given to the guest, >> so the above configuration was allowed and the underlying host physical >> regions were of course >> different and enforced with checks. So I’m not trying to prevent this >> behaviour, however ... >> >>> >>> The underlying Host Physical regions may be different. Xen doesn't >>> guarantee in which order the regions will be mapped, So whether the >>> overlapped region will point to the memory or the shared region is unknown >>> (we don't guarantee the order of the mapping). So nothing good will happen >>> to the guest. >> >> ... now here I don’t understand if this was wrong from the beginning or not, >> shall we enforce also that >> guest physical regions for static shared memory are outside the memory given >> to the guest? > > Nothing good will happen if you are trying to overwrite mappings. So I > think this should be enforced. However, this is a more general problem. > At the moment, this is pretty much as mess because you can overwrite any > mapping (e.g. map MMIO on top of the RAM). > > I think the easiest way to enforce is to do it in the P2M code like x86 > does for certain mappings. > > Anyway, I don't think the problem should be solved here or by you (this > is likely going to be a can of worms). For now, I would consider to > simply drop the patches that are trying to do the merge. > > Any thoughts? I agree with your analysis. For now, let's drop this patch. @Luca: This means I reviewed your series completely and you can send a respin. ~Michal
[PATCH] docs: arm: Update where Xen should be loaded in memory
Since commit 6cd046c501bc ("xen/arm: Enlarge identity map space to 10TB")
Xen can be loaded below 10 TiB. Update docs accordingly.
Take the opportunity to update stale links to Linux docs.
Signed-off-by: Michal Orzel
---
docs/misc/arm/booting.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/docs/misc/arm/booting.txt b/docs/misc/arm/booting.txt
index 547f58a7d981..21ae74837dcc 100644
--- a/docs/misc/arm/booting.txt
+++ b/docs/misc/arm/booting.txt
@@ -23,7 +23,7 @@ The exceptions to this on 32-bit ARM are as follows:
The exceptions to this on 64-bit ARM are as follows:
- Xen binary should be loaded in memory below 2 TiB.
+ Xen binary should be loaded in memory below 10 TiB.
Booting Guests
--
@@ -64,10 +64,10 @@ Xen relies on some settings the firmware has to configure
in EL3 before starting
[1] linux/Documentation/arm/booting.rst
-Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/booting.rst
+Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm/booting.rst
[2] linux/Documentation/arm64/booting.rst
-Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.rst
+Latest version:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arch/arm64/booting.rst
[3] legacy format header
Latest version:
https://source.denx.de/u-boot/u-boot/-/blob/master/include/image.h#L315
--
2.25.1
Re: [PATCH] xen/include: move definition of ASM_INT() to xen/linkage.h
On 03/04/2024 14:03, Juergen Gross wrote: > > > ASM_INT() is defined in arch/[arm|x86]/include/asm/asm_defns.h in > exactly the same way. Instead of replicating this definition for riscv > and ppc, move it to include/xen/linkage.h, where other arch agnostic > definitions for assembler code are living already. > > Adapt the generation of assembler sources via tools/binfile to include > the new home of ASM_INT(). > > Signed-off-by: Juergen Gross Acked-by: Michal Orzel ~Michal
Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi Jens, On 11/04/2024 08:12, Jens Wiklander wrote: > > > Hi Michal, > > On Wed, Apr 10, 2024 at 3:24 PM Michal Orzel wrote: >> >> Hi Jens, >> >> On 09/04/2024 17:36, Jens Wiklander wrote: >>> >>> >>> Updates so request_irq() can be used with a dynamically assigned SGI irq >>> as input. >> At this point it would be handy to mention the use case for which you need >> to add this support > > OK, I'll add something like: > This prepares for a later patch where an FF-A schedule receiver > interrupt handler is installed for an SGI generated by the secure > world. ok > >> >>> >>> gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have >>> their type assigned earlier during boot >> Could you elaborate more on that? Do you mean that SGIs are always edge >> triggered and there's no need >> for setting the type? > > Yes, see https://developer.arm.com/documentation/ihi0069/h > 4.4 Software Generated Interrupts > SGIs are typically used for inter-processor communication, and are > generated by a write to an SGI register in the > GIC. SGIs can be either Group 0 or Group 1 interrupts, and they can > support only edge-triggered behavior. Exactly. But you wrote "have their type assigned earlier during boot" which is not true. There is no write to ICFGR0 in Xen codebase. They are implicitly edge triggered. So I would write: "... for SGIs since they are always edge triggered" ~Michal
Re: [PATCH v2 3/4] xen/virtual-region: Link the list build time
Hi Andrew,
On 10/04/2024 20:42, Andrew Cooper wrote:
>
>
> Given 3 statically initialised objects, its easy to link the list at build
> time. There's no need to do it during runtime at boot (and with IRQs-off,
> even).
>
> As a consequence, register_virtual_region() can now move inside ifdef
> CONFIG_LIVEPATCH like unregister_virtual_region().
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Michal Orzel
Maybe with ...
> ---
> CC: Jan Beulich
> CC: Roger Pau Monné
> CC: Wei Liu
> CC: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Michal Orzel
> CC: Oleksii Kurochko
> CC: Shawn Anastasio
> CC: Konrad Rzeszutek Wilk
> CC: Ross Lagerwall
>
> v2:
> * Collect the initialisers togoether too.
> ---
> xen/common/virtual_region.c | 37 ++---
> 1 file changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/xen/common/virtual_region.c b/xen/common/virtual_region.c
> index 7d8bdeb61282..db3e0dc9fe74 100644
> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -15,8 +15,18 @@ extern const struct bug_frame
> __start_bug_frames_2[], __stop_bug_frames_2[],
> __start_bug_frames_3[], __stop_bug_frames_3[];
>
> +/*
> + * For the built-in regions, the double linked list can be constructed at
> + * build time. Forward-declare the elements and their initialisers.
> + */
> +static struct list_head virtual_region_list;
> +static struct virtual_region core, core_init;
... empty line here for better readability
> +#define LIST_ENTRY_HEAD() { .next = , .prev =
> _init.list }
> +#define LIST_ENTRY_CORE() { .next = _init.list, .prev =
> _region_list }
> +#define LIST_ENTRY_INIT() { .next = _region_list, .prev =
> }
~Michal
Re: [PATCH v2 2/4] xen/virtual-region: Rework how bugframe linkage works
Hi Andrew, On 10/04/2024 20:42, Andrew Cooper wrote: > > > The start/stop1/etc linkage scheme predates struct virtual_region, and as > setup_virtual_regions() shows, it's awkward to express in the new scheme. > > Change the linker to provide explicit start/stop symbols for each bugframe > type, and change virtual_region to have a stop pointer rather than a count. > > This marginly simplifies both do_bug_frame()s and prepare_payload(), but it NIT: s/marginly/marginally > massively simplifies setup_virtual_regions() by allowing the compiler to > initialise the .frame[] array at build time. > > virtual_region.c is the only user of the linker symbols, and this is unlikely > to change given the purpose of struct virtual_region, so move their externs > out of bug.h > > No functional change. > > Signed-off-by: Andrew Cooper > Reviewed-by: Ross Lagerwall > Reviewed-by: Jan Beulich For Arm: Acked-by: Michal Orzel ~Michal
Re: [PATCH v2 1/4] xen/link: Introduce a common BUGFRAMES definition
Hi Andrew, On 10/04/2024 20:42, Andrew Cooper wrote: > > > Bugframe linkage is identical in all architectures. This is not surprising > given that it is (now) only consumed by common/virtual_region.c > > Introduce a common BUGFRAMES define in xen.lds.h ahead of rearranging their > structure. > > No functional change. > > Signed-off-by: Andrew Cooper > Reviewed-by: Jan Beulich For Arm: Acked-by: Michal Orzel ~Michal
Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
Hi Luca,
On 10/04/2024 16:08, Luca Fancellu wrote:
>
>
> Hi Michal,
>
>>>
>>> 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.
>> Stale info given that shmem is no longer part of reserved memory banks. It's
>> been taken care
>> of by removing shmem regions in find_unallocated_memory()
>
> Sure, will amend for this:
>
>>>
>>> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>>> +struct rangeset *rangeset)
>>> +{
>>> +const struct membanks *shm_mem = >shm_mem.common;
>>> +unsigned int i;
>>> +
>>> +/* Remove static shared memory regions */
>>> +for ( i = 0; i < shm_mem->nr_banks; i++ )
>>> +{
>>> +paddr_t start, end;
>>> +int res;
>>> +
>>> +start = shm_mem->bank[i].start;
>>> +end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
>> If you look at other rangeset_remove_range use cases and error messages, 1
>> is subtracted
>> in PFN_DOWN() so that the error message contains end unchanged. Please
>> adhere to that so that
>> printed messages are consistent.
>
> Yes I will change it to have -1 inside PFN_DOWN(), here and in the other
> occurrences
>>>
>>> +/* Remove static shared memory regions */
>>> +res = remove_shm_from_rangeset(kinfo, guest_holes);
>>> +if ( res )
>>> +goto out;
>>> +
>> Could you please add a comment explaining here what's done below?
>
> Is it ok something like this:
I'm ok with your proposal in the other e-mail.
>
> /*
> * Take the interval of memory starting from the first extended region bank
> * start address and ending to the end of the last extended region bank.
I would stop here. The rest reads quite difficult.
> * The interval will be passed to rangeset_report_ranges to allow it to
> * create, by the add_ext_regions callback, a set of extended memory region
> * banks from the guest_holes rangeset, which contains the original extended
> * memory region ranges where the static shared memory ranges are carved
> * out.
> */
>
>>> +i = ext_regions->nr_banks - 1;
>>> +start = ext_regions->bank[0].start;
>>> +end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
>>> +
>>> +/* Reset original extended regions to hold new value */
>>> +ext_regions->nr_banks = 0;
>>> +res = rangeset_report_ranges(guest_holes, PFN_DOWN(start),
>>> PFN_DOWN(end),
>>> + add_ext_regions, ext_regions);
>>> +if ( res )
>>> +ext_regions->nr_banks = 0;
>>> +else if ( !ext_regions->nr_banks )
>>> +res = -ENOENT;
>>> +
>>> + out:
>>> +rangeset_destroy(guest_holes);
>>> +
>>> +return res;
>>> +}
>>> +
>>> /*
>>> * Local variables:
>>> * mode: C
>>> --
>>> 2.34.1
>>>
>>
>> ~Michal
>>
>
> Cheers,
> Luca
~Michal
Re: [XEN PATCH v1 4/5] xen/arm: allow dynamically assigned SGI handlers
Hi Jens, On 09/04/2024 17:36, Jens Wiklander wrote: > > > Updates so request_irq() can be used with a dynamically assigned SGI irq > as input. At this point it would be handy to mention the use case for which you need to add this support > > gic_route_irq_to_xen() don't gic_set_irq_type() for SGIs since they have > their type assigned earlier during boot Could you elaborate more on that? Do you mean that SGIs are always edge triggered and there's no need for setting the type? > > gic_interrupt() is updated to route the dynamically assigned SGIs to > do_IRQ() instead of do_sgi(). The latter still handles the statically > assigned SGI handlers like for instance GIC_SGI_CALL_FUNCTION. > > Signed-off-by: Jens Wiklander Other than that, it LGTM: Acked-by: Michal Orzel but I would like other maintainers (especially Julien) to cross check it. ~Michal
Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
On 10/04/2024 13:19, Luca Fancellu wrote: > > > > Afterwards, create a new structure 'struct shared_meminfo' which > has the same interface of 'struct meminfo', but requires less I would expect some justification for selecting 32 as the max number of shmem banks >>> >>> So I have to say I picked up a value I thought was ok for the amount of >>> shared memory >>> Banks, do you think it is too low? The real intention here was to decouple >>> the number >>> of shared memory banks from the number of generic memory banks, and I felt >>> 32 was enough, >>> but if you think it might be an issue I could bump it, or we could have a >>> Kconfig... >> No need for Kconfig. 32 is enough for now but I expect a paragraph in commit >> msg that you select >> 32 which should be enough for current use cases and can be bumped in the >> future in case there is a need. > > What do you think of this proposal: > > [...] > hence the 'struct membank' won't grow in size. > > Afterwards, create a new structure 'struct shared_meminfo' which > has the same interface of 'struct meminfo', but requires less > banks, defined by the number in NR_SHMEM_BANKS, which is 32 at the > moment and should be enough for the current use cases, the value > might be increased in te future if needed. > Finally, this structure hosts also the extra information for the > static shared memory banks. > The fields 'bank' and 'extra' of this structure are meant to be > [...] reads ok ~Michal
Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> From: Penny Zheng
>
> 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.
Stale info given that shmem is no longer part of reserved memory banks. It's
been taken care
of by removing shmem regions in find_unallocated_memory()
>
> Signed-off-by: Penny Zheng
> Signed-off-by: Luca Fancellu
> ---
> v2:
> - Fixed commit title, fixed comment, moved call of remove_shm_from_rangeset
>after populating rangeset in find_memory_holes, print error code when
>possible
> - used PFN_DOWN where needed
> 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 | 88 +
> 4 files changed, 121 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 01d66fbde92b..9b36d6bb70c9 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;
> @@ -978,6 +978,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,shared-mem"
> */
> static int __init find_memory_holes(const struct kernel_info *kinfo,
> struct membanks *ext_regions)
> @@ -1005,6 +1007,11 @@ static int __init find_memory_holes(const struct
> kernel_info *kinfo,
> goto out;
> }
>
> +/* Remove static shared memory regions */
> +res = remove_shm_from_rangeset(kinfo, mem_holes);
> +if ( res )
> +goto out;
> +
> /*
> * Remove regions described by "reg" and "ranges" properties where
> * the memory is addressable (MMIO, RAM, PCI BAR, etc).
> @@ -1097,7 +1104,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 90aafc81e740..2e8b138eb989 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -28,6 +28,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 kernel_info *kinfo,
> @@ -59,6 +65,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;
> +}
> +
>
Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
On 10/04/2024 12:56, Luca Fancellu wrote: > > > Hi Michal, > >> On 10 Apr 2024, at 11:01, Michal Orzel wrote: >> >> Hi Luca, >> >> On 09/04/2024 13:45, Luca Fancellu wrote: >>> >>> >>> Currently the memory footprint of the static shared memory feature >>> is impacting all the struct meminfo instances with memory space >>> that is not going to be used. >>> >>> To solve this issue, rework the static shared memory extra >>> information linked to the memory bank to another structure, >>> struct shmem_membank_extra, and exploit the struct membank >>> padding to host a pointer to that structure in a union with the >>> enum membank_type, with this trick the 'struct membank' has the >>> same size with or without the static shared memory, given that >>> the 'type' and 'shmem_extra' are never used at the same time, >>> hence the 'struct membank' won't grow in size. >>> >>> Afterwards, create a new structure 'struct shared_meminfo' which >>> has the same interface of 'struct meminfo', but requires less >> I would expect some justification for selecting 32 as the max number of >> shmem banks > > So I have to say I picked up a value I thought was ok for the amount of > shared memory > Banks, do you think it is too low? The real intention here was to decouple > the number > of shared memory banks from the number of generic memory banks, and I felt 32 > was enough, > but if you think it might be an issue I could bump it, or we could have a > Kconfig... No need for Kconfig. 32 is enough for now but I expect a paragraph in commit msg that you select 32 which should be enough for current use cases and can be bumped in the future in case there is a need. > >>> >>> >>> Signed-off-by: Luca Fancellu >> With the find_unallocated_memory() issue fixed: >> Reviewed-by: Michal Orzel > > Thanks, I took the opportunity to improve the comment in that function in > this way, > adding “ (when the feature is enabled)": > > * 3) Remove static shared memory (when the feature is enabled) > > Please tell me if that works for you so I will keep your R-by You can retain Rb. ~Michal
Re: [PATCH v2 09/13] xen/arm: Reduce struct membank size on static shared memory
Hi Luca, On 09/04/2024 13:45, Luca Fancellu wrote: > > > Currently the memory footprint of the static shared memory feature > is impacting all the struct meminfo instances with memory space > that is not going to be used. > > To solve this issue, rework the static shared memory extra > information linked to the memory bank to another structure, > struct shmem_membank_extra, and exploit the struct membank > padding to host a pointer to that structure in a union with the > enum membank_type, with this trick the 'struct membank' has the > same size with or without the static shared memory, given that > the 'type' and 'shmem_extra' are never used at the same time, > hence the 'struct membank' won't grow in size. > > Afterwards, create a new structure 'struct shared_meminfo' which > has the same interface of 'struct meminfo', but requires less I would expect some justification for selecting 32 as the max number of shmem banks > banks and hosts the extra information for the static shared memory. > The fields 'bank' and 'extra' of this structure are meant to be > linked by the index (e.g. extra[idx] will have the information for > the bank[idx], for i=0..NR_SHMEM_BANKS), the convinient pointer > 'shmem_extra' of 'struct membank' is then linked to the related > 'extra' bank to ease the fruition when a function has access only > to the 'struct membanks common' of 'struct shared_meminfo'. > > The last part of this work is to move the allocation of the > static shared memory banks from the 'reserved_mem' to a new > 'shmem' member of the 'struct bootinfo'. > Change also the 'shm_mem' member type to be 'struct shared_meminfo' > in order to match the above changes and allow a memory space > reduction also in 'struct kernel_info'. > > Signed-off-by: Luca Fancellu With the find_unallocated_memory() issue fixed: Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v2 07/13] xen/arm: Avoid code duplication in check_reserved_regions_overlap
Hi Luca, On 09/04/2024 13:45, Luca Fancellu wrote: > > > The function check_reserved_regions_overlap is calling > 'meminfo_overlap_check' on the same type of structure, this code > can be written in a way to avoid code duplication, so rework the > function to do that. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v4 1/3] xen/arm: Add imx8q{m,x} platform glue
Hi John,
On 10/04/2024 08:53, John Ernberg wrote:
>
>
> On 4/9/24 8:47 AM, Michal Orzel wrote:
>> Hi John,
>>
>> On 08/04/2024 18:11, John Ernberg wrote:
>>>
>>>
>>> When using Linux for dom0 there are a bunch of drivers that need to do SMC
>>> SIP calls into the firmware to enable certain hardware bits like the
>>> watchdog.
>>>
>>> Provide a basic platform glue that implements the needed SMC forwarding.
>>>
>>> The format of these calls are as follows:
>>> - reg 0: function ID
>>> - reg 1: subfunction ID (when there's a subfunction)
>>> remaining regs: args
>>>
>>> For now we only allow Dom0 to make these calls as they are all managing
>>> hardware. There is no specification for these SIP calls, the IDs and names
>>> have been extracted from the upstream linux kernel and the vendor kernel.
>>>
>>> Most of the SIP calls are only available for the iMX8M series of SoCs, so
>>> they are easy to reject and they need to be revisited when iMX8M series
>>> support is added.
>> Stale paragraph. Should be removed given that the driver targets only
>> Q{M,XP}.
>>
>>>
>>> From the other calls we can reject CPUFREQ because Dom0 cannot make an
>>> informed decision regarding CPU frequency scaling, WAKEUP_SRC is to wake
>>> up from suspend, which Xen doesn't support at this time.
>>>
>>> This leaves the TIME SIP, OTP SIPs, BUILDINFO SIP and TEMP ALARM SIP, which
>>> for now are allowed to Dom0.
>> BUILDINFO, TEMP ALARM are leftovers from previous revision.
>>
>>>
>>> NOTE: This code is based on code found in NXP Xen tree located here:
>>> https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c
>>>
>>> Signed-off-by: Peng Fan
>>> [jernberg: Add SIP call filtering]
>>> Signed-off-by: John Ernberg
>> Reviewed-by: Michal Orzel
>>
>> The commit msg can be fixed on commit.
>>
>> ~Michal
>
>
> Apologies for forgetting to adjust that. Let me know if it's easier for
> you if I do a v5 with the fixed commit message.
>
> Thanks! // John Ernberg
The series is already committed and Stefano fixed my remarks on commit.
~Michal
Re: [PATCH v2 06/13] xen/arm: Avoid code duplication in find_unallocated_memory
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The function find_unallocated_memory is using the same code to
> loop through 3 structure of the same type, in order to avoid
> code duplication, rework the code to have only one loop that
> goes through all the structures.
>
> Signed-off-by: Luca Fancellu
> ---
> v2:
> - Add comment in the loop inside find_unallocated_memory to
>improve readability
> v1:
> - new patch
> ---
> ---
> xen/arch/arm/domain_build.c | 70 +
> 1 file changed, 25 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 57cf92668ae6..269aaff4d067 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn,
> unsigned long e_gfn,
> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>struct membanks *ext_regions)
> {
> -const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
> -const struct membanks *mem = bootinfo_get_mem();
> -const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> +const struct membanks *mem_banks[] = {
> +bootinfo_get_mem(),
> +kernel_info_get_mem_const(kinfo),
> +bootinfo_get_reserved_mem(),
> +};
> struct rangeset *unalloc_mem;
> paddr_t start, end;
> -unsigned int i;
> +unsigned int i, j;
> int res;
>
> dt_dprintk("Find unallocated memory for extended regions\n");
> @@ -883,50 +885,28 @@ static int __init find_unallocated_memory(const struct
> kernel_info *kinfo,
> if ( !unalloc_mem )
> return -ENOMEM;
>
> -/* Start with all available RAM */
> -for ( i = 0; i < mem->nr_banks; i++ )
> -{
> -start = mem->bank[i].start;
> -end = mem->bank[i].start + mem->bank[i].size;
> -res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> - PFN_DOWN(end - 1));
> -if ( res )
> +/*
> + * Exclude the following regions, in order:
> + * 1) Start with all available RAM
> + * 2) Remove RAM assigned to Dom0
> + * 3) Remove reserved memory
Given this commit and the previous code, I expect one call to
rangeset_add_range() and
3 calls to rangeset_remove_range(). However ...
> + * The order comes from the initialization of the variable "mem_banks"
> + * above
> + */
> +for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> +for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
> {
> -printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
> - start, end);
> -goto out;
> -}
> -}
> -
> -/* Remove RAM assigned to Dom0 */
> -for ( i = 0; i < kinfo_mem->nr_banks; i++ )
> -{
> -start = kinfo_mem->bank[i].start;
> -end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
> -res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
> +start = mem_banks[i]->bank[j].start;
> +end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size;
> +res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
... here you always call rangeset_add_range() which is wrong. For direct mapped
domain
you would e.g. register its RAM region as extended region.
~Michal
Re: [PATCH v2 04/13] xen/arm: Introduce a generic way to access memory bank structures
Hi Luca,
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> Currently the 'stuct meminfo' is defining a static defined array of
s/stuct/struct
> 'struct membank' of NR_MEM_BANKS elements, some feature like
s/feature/features
> shared memory don't require such amount of memory allocation but
> might want to reuse existing code to manipulate this kind of
> structure that is just as 'struct meminfo' but less bulky.
>
> For this reason introduce a generic way to access this kind of
> structure using a new structure 'struct membanks', which implements
> all the fields needed by a structure related to memory banks
> without the need to specify at build time the size of the
> 'struct membank' array, using a flexible array member.
>
> Modify 'struct meminfo' to implement the field related to the new
> introduced structure, given the change all usage of this structure
> are updated in this way:
> - code accessing bootinfo.{mem,reserved_mem,acpi} field now uses
>3 new introduced static inline helpers to access the new field
>of 'struct meminfo' named 'common'.
> - code accessing 'struct kernel_info *' member 'mem' now use the
>new introduced macro 'kernel_info_get_mem(...)' to access the
>new field of 'struct meminfo' named 'common'.
I would also mention introduction of KERNEL_INFO_INIT, BOOTINFO_INIT that
should be used
from now onwards to initialize corresponding structs (.max_banks member).
>
> Constify pointers where needed.
>
> Suggested-by: Julien Grall
> Signed-off-by: Luca Fancellu
> ---
> v2:
> - Fixed typos in commit message and mention flexible array member
> - Add static assert for struct membanks
> - use static inline for the kernel_info structure instead of macro
> - use xzalloc_flex_struct inside make_hypervisor_node instead of
>xzalloc
> - Fix trailing backslash style
[...]
>
> +static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo)
> +{
> +return >mem.common;
> +}
> +
> +static inline const struct membanks *
> +kernel_info_get_mem_const(const struct kernel_info *kinfo)
I'm not a fan of having 2 helpers named this way. Maybe macro (as in v1) would
be better here.
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH v2 03/13] xen/arm: Pass struct kernel_info parameter to make_resv_memory_node
Hi Luca,
The title should be "xen/arm: Pass struct kernel_info parameter to
make_{resv,shm}_memory_node
given that you're modifying both functions.
On 09/04/2024 13:45, Luca Fancellu wrote:
>
>
> The struct domain parameter is not used in make_resv_memory_node
> and in its called function make_shm_memory_node, so drop it from
> both function, also, take the occasion to pass directly
s/function/functions
> struct kernel_info, from which we can infer other parameter
s/parameter/parameters
> passed to the functions and drop them as well.
>
> Signed-off-by: Luca Fancellu
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH] xen/acpi: Allow xen/acpi.h to be included on non-ACPI archs
Hi Shawn, On 05/04/2024 20:20, Shawn Anastasio wrote: > > > Conditionalize xen/acpi.h's inclusion of acpi/acpi.h and asm/acpi.h on > CONFIG_ACPI and import ARM's !CONFIG_ACPI stub for acpi_disabled() so > that the header can be included on architectures without ACPI support, > like ppc. > > This change revealed some missing #includes across the ARM tree, so fix > those as well. > > Suggested-by: Jan Beulich > Signed-off-by: Shawn Anastasio Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v4 1/3] xen/arm: Add imx8q{m,x} platform glue
Hi John,
On 08/04/2024 18:11, John Ernberg wrote:
>
>
> When using Linux for dom0 there are a bunch of drivers that need to do SMC
> SIP calls into the firmware to enable certain hardware bits like the
> watchdog.
>
> Provide a basic platform glue that implements the needed SMC forwarding.
>
> The format of these calls are as follows:
> - reg 0: function ID
> - reg 1: subfunction ID (when there's a subfunction)
> remaining regs: args
>
> For now we only allow Dom0 to make these calls as they are all managing
> hardware. There is no specification for these SIP calls, the IDs and names
> have been extracted from the upstream linux kernel and the vendor kernel.
>
> Most of the SIP calls are only available for the iMX8M series of SoCs, so
> they are easy to reject and they need to be revisited when iMX8M series
> support is added.
Stale paragraph. Should be removed given that the driver targets only Q{M,XP}.
>
> From the other calls we can reject CPUFREQ because Dom0 cannot make an
> informed decision regarding CPU frequency scaling, WAKEUP_SRC is to wake
> up from suspend, which Xen doesn't support at this time.
>
> This leaves the TIME SIP, OTP SIPs, BUILDINFO SIP and TEMP ALARM SIP, which
> for now are allowed to Dom0.
BUILDINFO, TEMP ALARM are leftovers from previous revision.
>
> NOTE: This code is based on code found in NXP Xen tree located here:
> https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c
>
> Signed-off-by: Peng Fan
> [jernberg: Add SIP call filtering]
> Signed-off-by: John Ernberg
Reviewed-by: Michal Orzel
The commit msg can be fixed on commit.
~Michal
Re: [PATCH V4] Add requirements for Device Passthrough
Hi Oleksandr, On 08/04/2024 15:19, Oleksandr Tyshchenko wrote: > > > From: Oleksandr Tyshchenko > > Please refer to chapter "Device Passthrough": > https://groups.io/g/amd-xen-safety/message/1300 > > Create corresponding directory and README file. > > Signed-off-by: Oleksandr Tyshchenko Now, it LGTM. I will wait for Stewart to check PCI requirements Reviewed-by: Michal Orzel ~Michal
Re: [PATCH] drivers: char: Enable OMAP UART driver for TI K3 devices
Hi, On 08/04/2024 14:11, Vaishnav Achath wrote: > > > TI K3 devices (J721E, J721S2, AM62X .etc) has the same variant s/has/have > of UART as OMAP4. Add the compatible used in Linux device tree, > "ti,am654-uart" to the omap-uart dt_match so that the driver can > be used with these devices. Also enable the driver for ARM64 > platforms. > > Signed-off-by: Vaishnav Achath > --- > > Xen logs from J721E EVM: > https://gist.github.com/vaishnavachath/8185e2378981705e1deb121f109f46b5 > > xen/drivers/char/Kconfig | 2 +- > xen/drivers/char/omap-uart.c | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig > index e18ec3788c..dcb9e85853 100644 > --- a/xen/drivers/char/Kconfig > +++ b/xen/drivers/char/Kconfig > @@ -55,7 +55,7 @@ config HAS_EXYNOS4210 > config HAS_OMAP > bool "Texas Instruments OMAP UART driver" > default y > - depends on ARM_32 > + depends on ARM_32 || ARM_64 depends on ARM please With that: Reviewed-by: Michal Orzel ~Michal
Re: [PATCH v6 1/8] xen/spinlock: add explicit non-recursive locking functions
On 04/04/2024 13:22, Juergen Gross wrote: > On 27.03.24 16:22, Juergen Gross wrote: >> In order to prepare a type-safe recursive spinlock structure, add >> explicitly non-recursive locking functions to be used for non-recursive >> locking of spinlocks, which are used recursively, too. >> >> Signed-off-by: Juergen Gross >> Acked-by: Jan Beulich > > Could any of the Arm maintainers please have a look at this patch? I can see that Julien took a look at this patch and he was ok, so: Acked-by: Michal Orzel ~Michal
Re: [PATCH 08/11] xen/arm: Reduce struct membank size on static shared memory
Hi Luca,
On 04/04/2024 11:33, Luca Fancellu wrote:
>
>
>> On 22 Mar 2024, at 10:30, Michal Orzel wrote:
>>
>> Hi Luca,
>>
>> On 12/03/2024 14:03, Luca Fancellu wrote:
>>>
>>>
>>> Currently the memory footprint of the static shared memory feature
>>> is impacting all the struct meminfo instances with memory space
>>> that is not going to be used.
>>>
>>> To solve this issue, rework the static shared memory extra
>>> information linked to the memory bank to another structure,
>>> struct shmem_membank_extra, and exploit the struct membank
>>> padding to host a pointer to that structure in a union with the
>> NIT: AFAICT the padding will be reused on Arm64 but on Arm32 there will
>> still be 4B padding.
>
> Yes, my purpose was to make clear that no additional space was needed
> for that pointer, should I rephrase it or it’s ok?
Since you are modifying this patch anyway, you can take the opportunity to
rephrase it.
>
>
>>>
>>>
>>> +struct shared_meminfo {
>>> +struct membanks common;
>>> +struct membank bank[NR_SHMEM_BANKS];
>>> +struct shmem_membank_extra extra[NR_SHMEM_BANKS];
>>> +};
>> Same as with meminfo, please add a BUILD_BUG_ON for padding between common
>> and bank.
>
> Sure
>
>>>
>>>
>>> -static int __init append_shm_bank_to_domain(struct membanks *shm_mem,
>>> -paddr_t start, paddr_t size,
>>> -const char *shm_id)
>>> +static int __init
>>> +append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t
>>> start,
>> Is there any particular reason to prepend the shm_mem name with kinfo?
>
> I think because usually kinfo is used to point to ’struct kernel_info’,
> instead here we point to
> 'struct shared_meminfo'
>
>>
>> ~Michal
>
~Michal
Re: [PATCH 04/11] xen/arm: Conditional compilation of kernel_info.shm_mem member
Hi Luca, On 04/04/2024 11:27, Luca Fancellu wrote: > > >> On 19 Mar 2024, at 14:58, Michal Orzel wrote: >> >> Hi Luca, >> >> On 12/03/2024 14:03, Luca Fancellu wrote: >>> >>> >>> The user of shm_mem member of the 'struct kernel_info' is only >>> the code managing the static shared memory feature, which can be >>> compiled out using CONFIG_STATIC_SHM, so in case the feature is >>> not requested, that member won't be used and will waste memory >>> space. >>> >>> To address this issue, protect the member with the Kconfig parameter >>> and modify the signature of the only function using it to remove >>> any reference to the member from outside the static-shmem module. >>> >>> Signed-off-by: Luca Fancellu >> Reviewed-by: Michal Orzel >> >> NIT: I always wonder why we have hundreds of functions taking both struct >> domain and >> struct kernel_info as arguments if the latter has the former as its member. >> As you are >> revisiting the function and modifying parameter list, you could take the >> opportunity >> to change it. But you don't have to. > > You are right, can I do this modification as part of patch 3 and this one? > Also, can I keep your R-by > here when doing this change? You can do this as part of patch 3 (afaict there will be no need to modify the argument list in patch 4) and you can keep my Rb. ~Michal
Re: [PATCH 1/2] drivers: char: Drop useless suspend/resume stubs in Arm drivers
Hi Jan,
On 04/04/2024 10:06, Jan Beulich wrote:
>
>
> On 04.04.2024 09:51, Michal Orzel wrote:
>> On Arm we don't use console_{suspend,resume} and the corresponding
>> stubs in serial drivers are being redundantly copied whenever a new
>> driver is added. Drop them as well as useless .endboot = NULL assignment.
>>
>> Signed-off-by: Michal Orzel
>
> Since hook invocations are suitably guarded:
> Reviewed-by: Jan Beulich
thanks
>
> However, from a Misra perspective more wants doing here if suspend/resume
> indeed isn't wanted / needed on Arm: console_{suspend,resume}() are
> unreachable there, and hence want hiding behind some (presumably)
> CONFIG_HAS_*. In turn the two hooks then would also want making conditional
> upon that option actually being selected by an architecture.
Yes, this will be handled in the future as part of the bigger activity to
compile out unused code.
~Michal
[PATCH 2/2] char: lpuart: Drop useless variables from UART structure
These variables are useless. They are being assigned a value which is
never used since UART is expected to be pre-configured.
No functional change.
Signed-off-by: Michal Orzel
---
xen/drivers/char/imx-lpuart.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
index faf4693b66e1..7770d158bf59 100644
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -30,7 +30,6 @@
#define imx_lpuart_write(uart, off, val) writel((val), (uart)->regs + (off))
static struct imx_lpuart {
-uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
uint32_t irq;
char __iomem *regs;
struct irqaction irqaction;
@@ -197,11 +196,6 @@ static int __init imx_lpuart_init(struct dt_device_node
*dev,
uart = _com;
-uart->baud = 115200;
-uart->data_bits = 8;
-uart->parity = 0;
-uart->stop_bits = 1;
-
res = dt_device_get_paddr(dev, 0, , );
if ( res )
{
--
2.25.1
[PATCH 0/2] drivers: char: simple cleanup
Simple serial drivers cleanup. No functional change intended. Michal Orzel (2): drivers: char: Drop useless suspend/resume stubs in Arm drivers char: lpuart: Drop useless variables from UART structure xen/drivers/char/cadence-uart.c| 13 - xen/drivers/char/exynos4210-uart.c | 13 - xen/drivers/char/imx-lpuart.c | 19 --- xen/drivers/char/meson-uart.c | 13 - xen/drivers/char/mvebu-uart.c | 13 - xen/drivers/char/omap-uart.c | 13 - xen/drivers/char/pl011.c | 13 - xen/drivers/char/scif-uart.c | 13 - 8 files changed, 110 deletions(-) -- 2.25.1
[PATCH 1/2] drivers: char: Drop useless suspend/resume stubs in Arm drivers
On Arm we don't use console_{suspend,resume} and the corresponding
stubs in serial drivers are being redundantly copied whenever a new
driver is added. Drop them as well as useless .endboot = NULL assignment.
Signed-off-by: Michal Orzel
---
xen/drivers/char/cadence-uart.c| 13 -
xen/drivers/char/exynos4210-uart.c | 13 -
xen/drivers/char/imx-lpuart.c | 13 -
xen/drivers/char/meson-uart.c | 13 -
xen/drivers/char/mvebu-uart.c | 13 -
xen/drivers/char/omap-uart.c | 13 -
xen/drivers/char/pl011.c | 13 -
xen/drivers/char/scif-uart.c | 13 -
8 files changed, 104 deletions(-)
diff --git a/xen/drivers/char/cadence-uart.c b/xen/drivers/char/cadence-uart.c
index 3618fa88d868..b2f379833f02 100644
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -90,16 +90,6 @@ static void __init cuart_init_postirq(struct serial_port
*port)
cuart_write(uart, R_UART_IER, UART_SR_INTR_RTRIG);
}
-static void cuart_suspend(struct serial_port *port)
-{
-BUG();
-}
-
-static void cuart_resume(struct serial_port *port)
-{
-BUG();
-}
-
static int cuart_tx_ready(struct serial_port *port)
{
struct cuart *uart = port->uart;
@@ -143,9 +133,6 @@ static const struct vuart_info *cuart_vuart(struct
serial_port *port)
static struct uart_driver __read_mostly cuart_driver = {
.init_preirq = cuart_init_preirq,
.init_postirq = cuart_init_postirq,
-.endboot = NULL,
-.suspend = cuart_suspend,
-.resume = cuart_resume,
.tx_ready = cuart_tx_ready,
.putc = cuart_putc,
.getc = cuart_getc,
diff --git a/xen/drivers/char/exynos4210-uart.c
b/xen/drivers/char/exynos4210-uart.c
index b29fd75c5a1c..58901df554cb 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -212,16 +212,6 @@ static void __init exynos4210_uart_init_postirq(struct
serial_port *port)
exynos4210_write(uart, UMCON, exynos4210_read(uart, UMCON) | UMCON_INT_EN);
}
-static void exynos4210_uart_suspend(struct serial_port *port)
-{
-BUG(); // XXX
-}
-
-static void exynos4210_uart_resume(struct serial_port *port)
-{
-BUG(); // XXX
-}
-
static int exynos4210_uart_tx_ready(struct serial_port *port)
{
struct exynos4210_uart *uart = port->uart;
@@ -286,9 +276,6 @@ static const struct vuart_info
*exynos4210_vuart_info(struct serial_port *port)
static struct uart_driver __read_mostly exynos4210_uart_driver = {
.init_preirq = exynos4210_uart_init_preirq,
.init_postirq = exynos4210_uart_init_postirq,
-.endboot = NULL,
-.suspend = exynos4210_uart_suspend,
-.resume = exynos4210_uart_resume,
.tx_ready = exynos4210_uart_tx_ready,
.putc = exynos4210_uart_putc,
.getc = exynos4210_uart_getc,
diff --git a/xen/drivers/char/imx-lpuart.c b/xen/drivers/char/imx-lpuart.c
index 522680a25cec..faf4693b66e1 100644
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -101,16 +101,6 @@ static void __init imx_lpuart_init_postirq(struct
serial_port *port)
imx_lpuart_write(uart, UARTCTRL, temp);
}
-static void imx_lpuart_suspend(struct serial_port *port)
-{
-BUG();
-}
-
-static void imx_lpuart_resume(struct serial_port *port)
-{
-BUG();
-}
-
static int imx_lpuart_tx_ready(struct serial_port *port)
{
struct imx_lpuart *uart = port->uart;
@@ -185,9 +175,6 @@ static void imx_lpuart_stop_tx(struct serial_port *port)
static struct uart_driver __read_mostly imx_lpuart_driver = {
.init_preirq = imx_lpuart_init_preirq,
.init_postirq = imx_lpuart_init_postirq,
-.endboot = NULL,
-.suspend = imx_lpuart_suspend,
-.resume = imx_lpuart_resume,
.tx_ready = imx_lpuart_tx_ready,
.putc = imx_lpuart_putc,
.getc = imx_lpuart_getc,
diff --git a/xen/drivers/char/meson-uart.c b/xen/drivers/char/meson-uart.c
index 694381986d07..407a276085b7 100644
--- a/xen/drivers/char/meson-uart.c
+++ b/xen/drivers/char/meson-uart.c
@@ -116,16 +116,6 @@ static void __init meson_uart_init_postirq(struct
serial_port *port)
(AML_UART_RX_INT_EN | AML_UART_TX_INT_EN));
}
-static void meson_uart_suspend(struct serial_port *port)
-{
-BUG();
-}
-
-static void meson_uart_resume(struct serial_port *port)
-{
-BUG();
-}
-
static void meson_uart_putc(struct serial_port *port, char c)
{
struct meson_uart *uart = port->uart;
@@ -191,9 +181,6 @@ static int meson_uart_tx_ready(struct serial_port *port)
static struct uart_driver __read_mostly meson_uart_driver = {
.init_preirq = meson_uart_init_preirq,
.init_postirq = meson_uart_init_postirq,
-.endboot = NULL,
-.suspend = meson_uart_suspend,
-.resume = meson_uart_resume,
.putc = meson_uart_putc,
.getc = meson_uart_getc,
Re: [PATCH 2/2] xen/arm: Add i.MX UART driver
Hi Oleksandr,
On 02/04/2024 14:05, Oleksandr Tyshchenko wrote:
>
>
> From: Oleksandr Tyshchenko
>
> The i.MX UART Documentation:
> https://www.nxp.com/webapp/Download?colCode=IMX8MMRM
> Chapter 16.2 Universal Asynchronous Receiver/Transmitter (UART)
>
> Tested on i.MX 8M Mini only, but I guess, it should be
imperative mood
> suitable for other i.MX8M* SoCs (those UART device tree nodes
> contain "fsl,imx6q-uart" compatible string).
>
> Signed-off-by: Oleksandr Tyshchenko
> ---
> I used the "earlycon=ec_imx6q,0x3089" cmd arg and
> selected CONFIG_SERIAL_IMX_EARLYCON in Linux for enabling vUART.
> ---
> ---
> MAINTAINERS | 1 +
> xen/drivers/char/Kconfig| 7 +
> xen/drivers/char/Makefile | 1 +
> xen/drivers/char/imx-uart.c | 299
> 4 files changed, 308 insertions(+)
> create mode 100644 xen/drivers/char/imx-uart.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1bd22fd75f..bd4084fd20 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -249,6 +249,7 @@ F: xen/drivers/char/arm-uart.c
> F: xen/drivers/char/cadence-uart.c
> F: xen/drivers/char/exynos4210-uart.c
> F: xen/drivers/char/imx-lpuart.c
> +F: xen/drivers/char/imx-uart.c
> F: xen/drivers/char/meson-uart.c
> F: xen/drivers/char/mvebu-uart.c
> F: xen/drivers/char/omap-uart.c
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index e18ec3788c..f51a1f596a 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,13 @@ config HAS_IMX_LPUART
> help
> This selects the i.MX LPUART. If you have i.MX8QM based board, say
> Y.
>
> +config HAS_IMX_UART
> + bool "i.MX UART driver"
> + default y
> + depends on ARM_64
> + help
> + This selects the i.MX UART. If you have i.MX8M* based board, say Y.
> +
> config HAS_MVEBU
> bool "Marvell MVEBU UART driver"
> default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index e7e374775d..147530a1ed 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> obj-$(CONFIG_XHCI) += xhci-dbc.o
> obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
> +obj-$(CONFIG_HAS_IMX_UART) += imx-uart.o
> obj-$(CONFIG_ARM) += arm-uart.o
> obj-y += serial.o
> obj-$(CONFIG_XEN_GUEST) += xen_pv_console.o
> diff --git a/xen/drivers/char/imx-uart.c b/xen/drivers/char/imx-uart.c
> new file mode 100644
> index 00..13bb189063
> --- /dev/null
> +++ b/xen/drivers/char/imx-uart.c
> @@ -0,0 +1,299 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
Can it be GPL-2.0-only? Some companies are worried about v3.
> +/*
> + * xen/drivers/char/imx-uart.c
> + *
> + * Driver for i.MX UART.
> + *
> + * Based on Linux's drivers/tty/serial/imx.c
> + *
> + * Copyright (C) 2024 EPAM Systems Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define imx_uart_read(uart, off) readl((uart)->regs + (off))
> +#define imx_uart_write(uart, off, val)writel((val), (uart)->regs + (off))
> +
> +static struct imx_uart {
> +uint32_t baud, clock_hz, data_bits, parity, stop_bits, fifo_size;
What's the use of these variables? AFAICT they are set but never used.
> +uint32_t irq;
unsigned int
> +char __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} imx_com;
> +
> +static void imx_uart_interrupt(int irq, void *data)
> +{
> +struct serial_port *port = data;
> +struct imx_uart *uart = port->uart;
> +uint32_t usr1, usr2;
> +
> +usr1 = imx_uart_read(uart, USR1);
> +usr2 = imx_uart_read(uart, USR2);
> +
> +if ( usr1 & (USR1_RRDY | USR1_AGTIM) )
> +{
> +imx_uart_write(uart, USR1, USR1_AGTIM);
> +serial_rx_interrupt(port);
> +}
> +
> +if ( (usr1 & USR1_TRDY) || (usr2 & USR2_TXDC) )
> +serial_tx_interrupt(port);
> +}
> +
> +static void imx_uart_clear_rx_errors(struct serial_port *port)
> +{
> +struct imx_uart *uart = port->uart;
> +uint32_t usr1, usr2;
> +
> +usr1 = imx_uart_read(uart, USR1);
> +usr2 = imx_uart_read(uart, USR2);
> +
> +if ( usr2 & USR2_BRCD )
> +imx_uart_write(uart, USR2, USR2_BRCD);
> +else if ( usr1 & USR1_FRAMERR )
> +imx_uart_write(uart, USR1, USR1_FRAMERR);
> +else if ( usr1 & USR1_PARITYERR )
> +imx_uart_write(uart, USR1, USR1_PARITYERR);
> +
> +if ( usr2 & USR2_ORE )
> +imx_uart_write(uart, USR2, USR2_ORE);
> +}
> +
> +static void __init imx_uart_init_preirq(struct serial_port *port)
> +{
> +struct imx_uart *uart = port->uart;
> +uint32_t reg;
> +
> +/*
> + * Wait for the transmission to complete. This is needed for a smooth
> + * transition when we come from early printk.
> + */
> +while (
Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver
Hi, On 02/04/2024 12:25, Michal Orzel wrote: > > > Hello, > > On 29/03/2024 01:08, Volodymyr Babchuk wrote: >> >> >> Generic Interface (GENI) is a newer interface for low speed interfaces >> like UART, I2C or SPI. This patch adds the simple driver for the UART >> instance of GENI. Code is based on similar drivers in U-Boot and Linux >> kernel. > Do you have a link to a manual? > >> >> This driver implements only simple synchronous mode, because although >> GENI supports FIFO mode, it needs to know number of >> characters **before** starting TX transaction. This is a stark >> contrast when compared to other UART peripherals, which allow adding >> characters to a FIFO while TX operation is running. >> >> The patch adds both normal UART driver and earlyprintk version. >> >> Signed-off-by: Volodymyr Babchuk >> --- >> xen/arch/arm/Kconfig.debug | 19 +- >> xen/arch/arm/arm64/debug-qcom.inc| 76 +++ > Shouldn't all the files (+ other places) have geni in their names? Could qcom > refer to other type of serial device? > > >> xen/arch/arm/include/asm/qcom-uart.h | 48 + >> xen/drivers/char/Kconfig | 8 + >> xen/drivers/char/Makefile| 1 + >> xen/drivers/char/qcom-uart.c | 288 +++ >> 6 files changed, 439 insertions(+), 1 deletion(-) >> create mode 100644 xen/arch/arm/arm64/debug-qcom.inc >> create mode 100644 xen/arch/arm/include/asm/qcom-uart.h >> create mode 100644 xen/drivers/char/qcom-uart.c >> >> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug >> index eec860e88e..f6ab0bb30e 100644 >> --- a/xen/arch/arm/Kconfig.debug >> +++ b/xen/arch/arm/Kconfig.debug >> @@ -119,6 +119,19 @@ choice >> selecting one of the platform specific options below >> if >> you know the parameters for the port. >> >> + This option is preferred over the platform specific >> + options; the platform specific options are deprecated >> + and will soon be removed. >> + config EARLY_UART_CHOICE_QCOM > The list is sorted alphabetically. Please adhere to that. > >> + select EARLY_UART_QCOM >> + bool "Early printk via Qualcomm debug UART" > Shouldn't you add depends on ARM_64? Isn't this device Arm64 specific like in > Linux? > >> + help >> + Say Y here if you wish the early printk to direct >> their > help text here should be indented by 2 tabs and 2 spaces (do not take example > from surrounding code) > >> + output to a Qualcomm debug UART. You can use this >> option to >> + provide the parameters for the debug UART rather than >> + selecting one of the platform specific options below >> if >> + you know the parameters for the port. >> + >> This option is preferred over the platform specific >> options; the platform specific options are deprecated >> and will soon be removed. >> @@ -211,6 +224,9 @@ config EARLY_UART_PL011 >> config EARLY_UART_SCIF >> select EARLY_PRINTK >> bool >> +config EARLY_UART_QCOM >> + select EARLY_PRINTK >> + bool > The list is sorted alphabetically. Please adhere to that. > >> >> config EARLY_PRINTK >> bool >> @@ -261,7 +277,7 @@ config EARLY_UART_PL011_MMIO32 >> will be done using 32-bit only accessors. >> >> config EARLY_UART_INIT >> - depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0 >> + depends on (EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0) || >> EARLY_UART_QCOM >> def_bool y >> >> config EARLY_UART_8250_REG_SHIFT >> @@ -308,3 +324,4 @@ config EARLY_PRINTK_INC >> default "debug-mvebu.inc" if EARLY_UART_MVEBU >> default "debug-pl011.inc" if EARLY_UART_PL011 >> default "debug-scif.inc" if EARLY_UART_SCIF >> + default "debug-qcom.inc" if EARLY_UART_QCOM >> diff --git a/xen/arch/arm/arm64/debug-qcom.inc >> b/xen/arch/arm/arm64/debug-qcom.inc >> new file mode 100644 >> index 00..130d08d6e9 >> --- /dev/null >> +++ b/xen/arch/arm/arm64/debug-qcom.inc >> @@ -0,0 +1,76 @@ >&g
Re: [PATCH 1/2] xen/arm: Add i.MX UART early printk support
GPL-2.0-or-later */ > +/* > + * xen/arch/arm/include/asm/imx-uart.h > + * > + * Common constant definition between early printk and the UART driver > + * > + * Copyright (C) 2024 EPAM Systems Inc. > + */ > + > +#ifndef __ASM_ARM_IMX_UART_H__ > +#define __ASM_ARM_IMX_UART_H__ > + > +/* 32-bit register definition */ > +#define URXD0(0x00) /* Receiver Register */ There is no need to surround these values > +#define URTX0(0x40) /* Transmitter Register */ > +#define UCR1 (0x80) /* Control Register 1 */ > +#define UCR2 (0x84) /* Control Register 2 */ > +#define UCR3 (0x88) /* Control Register 3 */ > +#define UCR4 (0x8c) /* Control Register 4 */ > +#define UFCR (0x90) /* FIFO Control Register */ > +#define USR1 (0x94) /* Status Register 1 */ > +#define USR2 (0x98) /* Status Register 2 */ > +#define IMX21_UTS(0xb4) /* Test Register */ > + > +#define URXD_ERRBIT(14, UL) /* Error detect */ > +#define URXD_RX_DATAGENMASK(7, 0) /* Received data mask */ > + > +#define UCR1_TRDYEN BIT(13, UL) /* Transmitter ready interrupt enable */ > +#define UCR1_RRDYEN BIT(9, UL) /* Receiver ready interrupt enable */ NIT: please align comments within a block Other than that: Reviewed-by: Michal Orzel ~Michal
Re: [PATCH 3/3] arm: platform: qcom: add basic support SA8155P SoC
Hello,
On 29/03/2024 01:08, Volodymyr Babchuk wrote:
>
>
> Qualcomm SA8155P is the automotive variant of SM8150 aka Snapdragon
> 855.
>
> This patch adds very basic support for the platform. We need to handle
> Qualcomm-specific SMC to workaround quirk in the QCOM SCM driver in
> the Linux kernel. Basically the driver tries multiple different SMCs
> to determine which calling convention is supported by a SoC. If all
> calls fail it decides that the SoC uses "legacy SMC" and tries to
> communicate with SCM by issuing SMC with funcid = 1. Problem is that
> Xen has own understanding on how such SMC should be handled. It
> interprets this SMC as legacy PSCI_cpu_off and happily turns of Linux
> boot CPU.
>
> To workaround this, we pretend that we support
> QCOM_SCM_INFO_IS_CALL_AVAIL, this will make the driver use the latest
> calling convention. All subsequent calls will fail anyways and the
> driver will terminate self gracefully. This is not a big deal, because
> right now (with Linux 6.8) even on baremetal setup the driver fails
> anyways, because it does not know how to work with this SoC.
Therefore I would consider adding a Kconfig option and placing it under
experimental
>
> Signed-off-by: Volodymyr Babchuk
> ---
> xen/arch/arm/platforms/Makefile | 1 +
> xen/arch/arm/platforms/qcom.c | 77 +
> 2 files changed, 78 insertions(+)
> create mode 100644 xen/arch/arm/platforms/qcom.c
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..6873735ef0 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT) += sunxi.o
> obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> +obj-$(CONFIG_ALL64_PLAT) += qcom.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/qcom.c b/xen/arch/arm/platforms/qcom.c
> new file mode 100644
> index 00..77e9c58649
> --- /dev/null
> +++ b/xen/arch/arm/platforms/qcom.c
> @@ -0,0 +1,77 @@
> +/*
> + * xen/arch/arm/platforms/qcom.c
> + *
> + * Qualcomm SoCs specific code
> + *
> + * Volodymyr Babchuk
> + *
> + * Copyright (c) 2024 EPAM Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Please use SPDX identifier instead of license text
> + */
> +
> +#include
> +#include
no need for this public header
> +#include
> +
> +#define SCM_SMC_FNID(s, c) s) & 0xFF) << 8) | ((c) & 0xFF))
spaces instead of tabs
> +#define QCOM_SCM_SVC_INFO 0x06
spaces instead of tabs
> +#define QCOM_SCM_INFO_IS_CALL_AVAIL0x01
> +
> +#define ARM_SMCCC_SIP_QCOM_SCM_IS_CALL_AVAIL\
> +ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_CONV_64, \
> + ARM_SMCCC_OWNER_SIP, \
> + SCM_SMC_FNID(QCOM_SCM_SVC_INFO, \
> +QCOM_SCM_INFO_IS_CALL_AVAIL))
> +
> +static const char * const sa8155p_dt_compat[] __initconst =
> +{
> +"qcom,sa8155p",
> +NULL
> +};
> +
> +static bool sa8155p_smc(struct cpu_user_regs *regs)
> +{
> +uint32_t funcid = get_user_reg(regs, 0);
> +
> +switch ( funcid ) {
brace should be placed on a new line
~Michal
Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver
On 02/04/2024 23:19, Volodymyr Babchuk wrote: > > > Hi Michal, > > Michal Orzel writes: > >> Hello, >> >> On 29/03/2024 01:08, Volodymyr Babchuk wrote: >>> >>> >>> Generic Interface (GENI) is a newer interface for low speed interfaces >>> like UART, I2C or SPI. This patch adds the simple driver for the UART >>> instance of GENI. Code is based on similar drivers in U-Boot and Linux >>> kernel. >> Do you have a link to a manual? > > I wish I had. Qualcomm provides HW manuals only under very strict > NDAs. At the time of writing I don't have access to the manual at > all. Those patches are based solely on similar drivers in U-Boot and > Linux kernel. > >>> >>> This driver implements only simple synchronous mode, because although >>> GENI supports FIFO mode, it needs to know number of >>> characters **before** starting TX transaction. This is a stark >>> contrast when compared to other UART peripherals, which allow adding >>> characters to a FIFO while TX operation is running. >>> >>> The patch adds both normal UART driver and earlyprintk version. >>> >>> Signed-off-by: Volodymyr Babchuk >>> --- >>> xen/arch/arm/Kconfig.debug | 19 +- >>> xen/arch/arm/arm64/debug-qcom.inc| 76 +++ >> Shouldn't all the files (+ other places) have geni in their names? Could >> qcom refer to other type of serial device? > > AFAIK, there is an older type of serial device. Both Linux and U-Boot > use "msm" instead of "qcom" in drivers names for those devices. > > But I can add "geni" to the names, no big deal. > >> >>> xen/arch/arm/include/asm/qcom-uart.h | 48 + >>> xen/drivers/char/Kconfig | 8 + >>> xen/drivers/char/Makefile| 1 + >>> xen/drivers/char/qcom-uart.c | 288 +++ >>> 6 files changed, 439 insertions(+), 1 deletion(-) >>> create mode 100644 xen/arch/arm/arm64/debug-qcom.inc >>> create mode 100644 xen/arch/arm/include/asm/qcom-uart.h >>> create mode 100644 xen/drivers/char/qcom-uart.c >>> >>> diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug >>> index eec860e88e..f6ab0bb30e 100644 >>> --- a/xen/arch/arm/Kconfig.debug >>> +++ b/xen/arch/arm/Kconfig.debug >>> @@ -119,6 +119,19 @@ choice >>> selecting one of the platform specific options >>> below if >>> you know the parameters for the port. >>> >>> + This option is preferred over the platform specific >>> + options; the platform specific options are >>> deprecated >>> + and will soon be removed. >>> + config EARLY_UART_CHOICE_QCOM >> The list is sorted alphabetically. Please adhere to that. >> > > Sure > >>> + select EARLY_UART_QCOM >>> + bool "Early printk via Qualcomm debug UART" >> Shouldn't you add depends on ARM_64? Isn't this device Arm64 specific like >> in Linux? >> > > In linux it depends on ARCH_QCOM which can be enabled both for arm and > arm64. But for Xen case... I believe it is better to make ARM_64 > dependency. > >>> + help >>> + Say Y here if you wish the early printk to direct >>> their >> help text here should be indented by 2 tabs and 2 spaces (do not take >> example from surrounding code) >> > > Would anyone mind if I'll send patch that aligns surrounding code as well? > >>> + output to a Qualcomm debug UART. You can use this >>> option to >>> + provide the parameters for the debug UART rather >>> than >>> + selecting one of the platform specific options >>> below if >>> + you know the parameters for the port. >>> + >>> This option is preferred over the platform specific >>> options; the platform specific options are >>> deprecated >>> and will soon be removed. >>> @@ -211,6 +224,9 @@ config EARLY_UART_PL011 >>> config EARLY_UART_SCIF >>> select EARLY_PRINTK >>> bool >>> +config EARLY_UART_QCOM >>> + select EARLY_PRINTK >>> + bool >> The list is
Re: [PATCH 2/3] drivers: serial: add Qualcomm GENI-based serial driver
Hello, On 29/03/2024 01:08, Volodymyr Babchuk wrote: > > > Generic Interface (GENI) is a newer interface for low speed interfaces > like UART, I2C or SPI. This patch adds the simple driver for the UART > instance of GENI. Code is based on similar drivers in U-Boot and Linux > kernel. Do you have a link to a manual? > > This driver implements only simple synchronous mode, because although > GENI supports FIFO mode, it needs to know number of > characters **before** starting TX transaction. This is a stark > contrast when compared to other UART peripherals, which allow adding > characters to a FIFO while TX operation is running. > > The patch adds both normal UART driver and earlyprintk version. > > Signed-off-by: Volodymyr Babchuk > --- > xen/arch/arm/Kconfig.debug | 19 +- > xen/arch/arm/arm64/debug-qcom.inc| 76 +++ Shouldn't all the files (+ other places) have geni in their names? Could qcom refer to other type of serial device? > xen/arch/arm/include/asm/qcom-uart.h | 48 + > xen/drivers/char/Kconfig | 8 + > xen/drivers/char/Makefile| 1 + > xen/drivers/char/qcom-uart.c | 288 +++ > 6 files changed, 439 insertions(+), 1 deletion(-) > create mode 100644 xen/arch/arm/arm64/debug-qcom.inc > create mode 100644 xen/arch/arm/include/asm/qcom-uart.h > create mode 100644 xen/drivers/char/qcom-uart.c > > diff --git a/xen/arch/arm/Kconfig.debug b/xen/arch/arm/Kconfig.debug > index eec860e88e..f6ab0bb30e 100644 > --- a/xen/arch/arm/Kconfig.debug > +++ b/xen/arch/arm/Kconfig.debug > @@ -119,6 +119,19 @@ choice > selecting one of the platform specific options below > if > you know the parameters for the port. > > + This option is preferred over the platform specific > + options; the platform specific options are deprecated > + and will soon be removed. > + config EARLY_UART_CHOICE_QCOM The list is sorted alphabetically. Please adhere to that. > + select EARLY_UART_QCOM > + bool "Early printk via Qualcomm debug UART" Shouldn't you add depends on ARM_64? Isn't this device Arm64 specific like in Linux? > + help > + Say Y here if you wish the early printk to direct > their help text here should be indented by 2 tabs and 2 spaces (do not take example from surrounding code) > + output to a Qualcomm debug UART. You can use this > option to > + provide the parameters for the debug UART rather than > + selecting one of the platform specific options below > if > + you know the parameters for the port. > + > This option is preferred over the platform specific > options; the platform specific options are deprecated > and will soon be removed. > @@ -211,6 +224,9 @@ config EARLY_UART_PL011 > config EARLY_UART_SCIF > select EARLY_PRINTK > bool > +config EARLY_UART_QCOM > + select EARLY_PRINTK > + bool The list is sorted alphabetically. Please adhere to that. > > config EARLY_PRINTK > bool > @@ -261,7 +277,7 @@ config EARLY_UART_PL011_MMIO32 > will be done using 32-bit only accessors. > > config EARLY_UART_INIT > - depends on EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0 > + depends on (EARLY_UART_PL011 && EARLY_UART_PL011_BAUD_RATE != 0) || > EARLY_UART_QCOM > def_bool y > > config EARLY_UART_8250_REG_SHIFT > @@ -308,3 +324,4 @@ config EARLY_PRINTK_INC > default "debug-mvebu.inc" if EARLY_UART_MVEBU > default "debug-pl011.inc" if EARLY_UART_PL011 > default "debug-scif.inc" if EARLY_UART_SCIF > + default "debug-qcom.inc" if EARLY_UART_QCOM > diff --git a/xen/arch/arm/arm64/debug-qcom.inc > b/xen/arch/arm/arm64/debug-qcom.inc > new file mode 100644 > index 00..130d08d6e9 > --- /dev/null > +++ b/xen/arch/arm/arm64/debug-qcom.inc > @@ -0,0 +1,76 @@ > +/* > + * xen/arch/arm/arm64/debug-qcom.inc > + * > + * Qualcomm debug UART specific debug code > + * > + * Volodymyr Babchuk > + * Copyright (C) 2024, EPAM Systems. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. No need for the license text. You should use SPDX identifier instead at the top of the file: /* SPDX-License-Identifier:
Re: [PATCH 1/3] arm: smmu: allow SMMU to have more IRQs than context banks
Hello,
On 29/03/2024 01:08, Volodymyr Babchuk wrote:
>
>
> I encountered platform, namely Qualcomm SA8155P where SMMU-compatible
NIT: a commit msg should be written in imperative mood
> IO-MMU advertises more context IQRs than there are context banks. This
> should not be an issue, we need to relax the check in the SMMU driver
> to allow such configuration.
>
> Signed-off-by: Volodymyr Babchuk
> ---
> xen/drivers/passthrough/arm/smmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c
> b/xen/drivers/passthrough/arm/smmu.c
> index 32e2ff279b..2dd3688f3b 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -2550,7 +2550,7 @@ static int arm_smmu_device_dt_probe(struct
> platform_device *pdev)
> parse_driver_options(smmu);
>
> if (smmu->version > ARM_SMMU_V1 &&
> - smmu->num_context_banks != smmu->num_context_irqs) {
> + smmu->num_context_banks > smmu->num_context_irqs) {
This was done in Linux by commit:
d1e20222d537 ("iommu/arm-smmu: Error out only if not enough context interrupts")
However, they also ignore superfluous interrupts. Shouldn't we do the same?
~Michal
Re: [PATCH v3 1/3] xen/arm: Add imx8q{m,x} platform glue
Hi John,
On 28/03/2024 17:34, John Ernberg wrote:
>
>
> When using Linux for dom0 there are a bunch of drivers that need to do SMC
> SIP calls into the firmware to enable certain hardware bits like the
> watchdog.
>
> Provide a basic platform glue that implements the needed SMC forwarding.
>
> The format of these calls are as follows:
> - reg 0: function ID
> - reg 1: subfunction ID (when there's a subfunction)
> remaining regs: args
>
> For now we only allow Dom0 to make these calls as they are all managing
> hardware. There is no specification for these SIP calls, the IDs and names
> have been extracted from the upstream linux kernel and the vendor kernel.
>
> Most of the SIP calls are only available for the iMX8M series of SoCs, so
> they are easy to reject and they need to be revisited when iMX8M series
> support is added.
I just realized that you pinged me in v2 for clarification, sorry I missed that.
I still believe we shouldn't list FIDs that are meant for IMX8M, given that
the driver is written for IMX8QM/QXP.
>
> From the other calls we can reject CPUFREQ because Dom0 cannot make an
> informed decision regarding CPU frequency scaling, WAKEUP_SRC is to wake
> up from suspend, which Xen doesn't support at this time.
>
> This leaves the TIME SIP, OTP SIPs, BUILDINFO SIP and TEMP ALARM SIP, which
> for now are allowed to Dom0.
>
> NOTE: This code is based on code found in NXP Xen tree located here:
> https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c
>
> Signed-off-by: Peng Fan
> [jernberg: Add SIP call filtering]
> Signed-off-by: John Ernberg
>
> ---
>
> v3:
> - Adhere to style guidelines for line length and label indentation (Michal
> Orzel)
> - Use smccc macros to build the SIP function identifier (Michal Orzel)
> - Adjust platform name to be specific to QM and QXP variants (Michal Orzel)
> - Pick up additional SIPs which may be used by other Linux versions - for
> review purposes
> - Changes to the commit message due to above
>
> v2:
> - Reword the commit message to be a bit clearer
> - Include the link previously added as a context note to the commit message
> (Julien Grall)
> - Add Pengs signed off (Julien Grall, Peng Fan)
> - Add basic SIP call filter (Julien Grall)
> - Expand the commit message a whole bunch because of the changes to the code
> ---
> xen/arch/arm/platforms/Makefile | 1 +
> xen/arch/arm/platforms/imx8qm.c | 168
> 2 files changed, 169 insertions(+)
> create mode 100644 xen/arch/arm/platforms/imx8qm.c
>
> diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile
> index 8632f4115f..bec6e55d1f 100644
> --- a/xen/arch/arm/platforms/Makefile
> +++ b/xen/arch/arm/platforms/Makefile
> @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT) += sunxi.o
> obj-$(CONFIG_ALL64_PLAT) += thunderx.o
> obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o
> obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o
> +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp.o
> obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp-eemi.o
> diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c
> new file mode 100644
> index 00..0992475474
> --- /dev/null
> +++ b/xen/arch/arm/platforms/imx8qm.c
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * xen/arch/arm/platforms/imx8qm.c
> + *
> + * i.MX 8QM setup
> + *
> + * Copyright (c) 2016 Freescale Inc.
> + * Copyright 2018-2019 NXP
> + *
> + *
> + * Peng Fan
> + */
> +
> +#include
> +#include
> +#include
> +
> +static const char * const imx8qm_dt_compat[] __initconst =
> +{
> +"fsl,imx8qm",
> +"fsl,imx8qxp",
> +NULL
> +};
> +
> +#define IMX_SIP_FID(fid) \
> +ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_CONV_64, \
> + ARM_SMCCC_OWNER_SIP, \
> + fid)
macro parameter should be enclosed within parenthesis
> +
> +#define IMX_SIP_F_GPC0x
> +#define IMX_SIP_F_CPUFREQ0x0001
> +#define IMX_SIP_F_TIME 0x0002
> +#define IMX_SIP_F_BUILDINFO 0x0003
> +#define IMX_SIP_F_DDR_DVFS 0x0004
> +#define IMX_SIP_F_SRC0x0005
> +#define IMX_SIP_F_GET_SOC_INFO 0x0006
> +#define IMX_SIP_F_NOC0x0008
> +#define IMX_SIP_F_WAKEUP_SRC 0x0009
> +#define IMX_SIP_F_OTP_READ 0x000A
> +#define IMX_SIP_F_OTP_WRITE 0x000B
> +#define IMX_SIP_F_SET_TEMP_ALARM 0x000C
Is there specific reason for leading zeros?
> +
> +#define IMX_SIP_TIME_SF_RTC
Re: [PATCH v7 04/14] xen/arm: add Dom0 cache coloring support
On 27/03/2024 12:39, Carlo Nonato wrote: > > > Hi guys, > > On Fri, Mar 22, 2024 at 8:26 AM Jan Beulich wrote: >> >> On 21.03.2024 18:31, Carlo Nonato wrote: >>> On Thu, Mar 21, 2024 at 4:57 PM Jan Beulich wrote: On 21.03.2024 16:04, Carlo Nonato wrote: > On Tue, Mar 19, 2024 at 4:30 PM Jan Beulich wrote: >> On 15.03.2024 11:58, Carlo Nonato wrote: >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -963,6 +963,15 @@ Controls for the dom0 IOMMU setup. >>> >>> Specify a list of IO ports to be excluded from dom0 access. >>> >>> +### dom0-llc-colors >>> +> `= List of [ | - ]` >>> + >>> +> Default: `All available LLC colors` >>> + >>> +Specify dom0 LLC color configuration. This option is available only >>> when >>> +`CONFIG_LLC_COLORING` is enabled. If the parameter is not set, all >>> available >>> +colors are used. >> >> My reservation towards this being a top-level option remains. > > How can I turn this into a lower-level option? Moving it into "dom0=" > doesn't > seem possible to me. How can I express a list (llc-colors) inside another > list > (dom0)? dom0=llc-colors=0-3,12-15,other-param=... How can I stop parsing > before reaching other-param? For example by using a different separator: dom0=llc-colors=0-3+12-15,other-param=... >>> >>> Ok, but that would mean to change the implementation of the parsing function >>> and to adopt this syntax also in other places, something that I would've >>> preferred to avoid. Anyway I'll follow your suggestion. >> >> Well, this is all used by Arm only for now. You will want to make sure Arm >> folks are actually okay with this alternative approach. >> >> Jan > > Are you Arm maintainers ok with this? I'm not a fan of this syntax and I find it more difficult to parse compared to the usual case, where every option is clearly separated. That said, I won't oppose to it. ~Michal
Re: [PATCH 11/11] xen/arm: List static shared memory regions as /memory nodes
Hi Luca, On 26/03/2024 15:19, Luca Fancellu wrote: > > >> On 25 Mar 2024, at 08:58, Michal Orzel wrote: >> >> Hi Luca, >> > > Hi Michal, > >> On 12/03/2024 14:03, Luca Fancellu wrote: >>> >>> >>> Currently Xen is not exporting the static shared memory regions >>> to the device tree as /memory node, this commit is fixing this >>> issue. >> Looking at the implementation, you will always call make_shm_memory_node() >> twice. For the first >> time, to create /memory node and for the second time to create entry under >> /reserved-memory. Also, >> you will create a separate /memory node for every single shmem region >> instead of combining them >> in a single /memory region like make_memory_node() would do. Can't we reuse >> this function for simplicity? > > You mean using make_memory_node() to populate /reserved-memory and /memory? I > feel they are very different > In terms of properties to be created, so I don’t think we should create a > make_memory_node() that does both. > > Otherwise if you were suggesting to modify make_memory_node() only for what > concerns the /memory nodes, yes, this is what I meant > it might be feasible, however there are some parts that need to be skipped > with some flags (all the code accessing .type > member), if I understand correctly you like this function because it doesn’t > create one node for every bank, but it creates > reg addresses instead, in that case I could modify the current > make_shm_memory_node() to do the same. My concern is that we will have 2 functions to create memory nodes instead of one that can be reused. I know the issue is with .type member. If skipping results in a worse code, then I'm ok with make_shm_memory_node() used for 2 purposes (would it be possible to create /memory and entry under /reserved in the same call to a function?). > >> >> Also, afaict it is not forbidden to specify shmem range (correct me if I'm >> wrong), where guest address will be >> within with RAM allocated by Xen (e.g. GPA RAM range 0x4000 - 0x6000 >> and shmem is at 0x5000). In this case, >> you would create yet another /memory node that would result in overlap (i.e. >> more than one /memory node specifying the same range). > > This is a good point I didn’t think about, yes currently the code is creating > overlapping nodes in that case, wow so it means I > need to compute the non overlapping regions and emit a /memory node then! :) > ouch > > Please let me know if I understood correctly your comments. > > Cheers, > Luca > >> >> ~Michal > ~Michal
Re: [PATCH 11/11] xen/arm: List static shared memory regions as /memory nodes
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > Currently Xen is not exporting the static shared memory regions > to the device tree as /memory node, this commit is fixing this > issue. Looking at the implementation, you will always call make_shm_memory_node() twice. For the first time, to create /memory node and for the second time to create entry under /reserved-memory. Also, you will create a separate /memory node for every single shmem region instead of combining them in a single /memory region like make_memory_node() would do. Can't we reuse this function for simplicity? Also, afaict it is not forbidden to specify shmem range (correct me if I'm wrong), where guest address will be within with RAM allocated by Xen (e.g. GPA RAM range 0x4000 - 0x6000 and shmem is at 0x5000). In this case, you would create yet another /memory node that would result in overlap (i.e. more than one /memory node specifying the same range). ~Michal
Re: [PATCH 10/11] xen/arm: fix duplicate /reserved-memory node in Dom0
ls, int sizecells, > - const struct membanks *mem) > +int __init make_shm_memory_node(const struct domain *d, > +const struct kernel_info *kinfo, int > addrcells, > +int sizecells) Strictly speaking, changing the function signature is not mandatory for this change, so I would at least mention it in the commit msg. Other than that: Reviewed-by: Michal Orzel ~Michal
Re: [PATCH 09/11] xen/arm: remove shm holes for extended regions
Hi Luca,
NIT: title s/for/from?
On 12/03/2024 14:03, Luca Fancellu wrote:
>
>
> From: Penny Zheng
>
> 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
> Signed-off-by: Luca Fancellu
> ---
> 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().
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
Re: [PATCH 08/11] xen/arm: Reduce struct membank size on static shared memory
Hi Luca,
On 12/03/2024 14:03, Luca Fancellu wrote:
>
>
> Currently the memory footprint of the static shared memory feature
> is impacting all the struct meminfo instances with memory space
> that is not going to be used.
>
> To solve this issue, rework the static shared memory extra
> information linked to the memory bank to another structure,
> struct shmem_membank_extra, and exploit the struct membank
> padding to host a pointer to that structure in a union with the
NIT: AFAICT the padding will be reused on Arm64 but on Arm32 there will still
be 4B padding.
> enum membank_type, with this trick the 'struct membank' has the
> same size with or without the static shared memory, given that
> the 'type' and 'shmem_extra' are never used at the same time.
>
> Afterwards, create a new structure 'struct shared_meminfo' which
> has the same interface of 'struct meminfo', but requires less
> banks and hosts the extra information for the static shared memory.
> The fields 'bank' and 'extra' of this structure are meant to be
> linked by the index (e.g. extra[idx] will have the information for
> the bank[idx], for i=0..NR_SHMEM_BANKS), the convinient pointer
> 'shmem_extra' of 'struct membank' is then linked to the related
> 'extra' bank to ease the fruition when a function has access only
> to the 'struct membanks common' of 'struct shared_meminfo'.
>
> The last part of this work is to move the allocation of the
> static shared memory banks from the 'reserved_mem' to a new
> 'shmem' member of the 'struct bootinfo'.
> Change also the 'shm_mem' member type to be 'struct shared_meminfo'
> in order to match the above changes and allow a memory space
> reduction also in 'struct kernel_info'.
Ok, so from this point onwards, when dealing with "reserved" memory, one needs
to account for shmem as well, as it will no longer be part of
bootinfo.reserved_mem
(unlike static-mem or static-heap). This is really the only disadvantage. The
changes
LGTM, just a few comments below.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/arm32/mmu/mm.c | 24 +
> xen/arch/arm/arm64/mmu/mm.c | 2 +
> xen/arch/arm/bootfdt.c | 1 +
> xen/arch/arm/domain_build.c | 4 ++
> xen/arch/arm/include/asm/kernel.h | 4 +-
> xen/arch/arm/include/asm/setup.h| 41 +--
> xen/arch/arm/include/asm/static-shmem.h | 8 +++
> xen/arch/arm/setup.c| 25 -
> xen/arch/arm/static-shmem.c | 69 ++---
> 9 files changed, 154 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c
> index e6bb5d934c16..45e42b307e20 100644
> --- a/xen/arch/arm/arm32/mmu/mm.c
> +++ b/xen/arch/arm/arm32/mmu/mm.c
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
>
> static unsigned long opt_xenheap_megabytes __initdata;
> integer_param("xenheap_megabytes", opt_xenheap_megabytes);
> @@ -42,6 +43,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
> int first_mod)
> {
> const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> +#ifdef CONFIG_STATIC_SHM
> +const struct membanks *shmem = bootinfo_get_shmem();
> +#endif
> const struct bootmodules *mi =
> int i;
> int nr;
> @@ -118,6 +122,25 @@ static paddr_t __init consider_modules(paddr_t s,
> paddr_t e,
> return consider_modules(s, r_s, size, align, i + 1);
> }
> }
> +
> +#ifdef CONFIG_STATIC_SHM
> +nr += reserved_mem->nr_banks;
> +for ( ; i - nr < shmem->nr_banks; i++ )
> +{
> +paddr_t r_s = shmem->bank[i - nr].start;
> +paddr_t r_e = r_s + shmem->bank[i - nr].size;
> +
> +if ( s < r_e && r_s < e )
> +{
> +r_e = consider_modules(r_e, e, size, align, i + 1);
> +if ( r_e )
> +return r_e;
> +
> +return consider_modules(s, r_s, size, align, i + 1);
> +}
> +}
> +#endif
> +
> return e;
> }
>
> @@ -294,6 +317,7 @@ void __init setup_mm(void)
> mfn_to_maddr(directmap_mfn_end));
>
> init_staticmem_pages();
> +init_sharedmem_pages();
> }
>
> /*
> diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c
> index f8aaf4ac18be..293acb67e09c 100644
> --- a/xen/arch/arm/arm64/mmu/mm.c
> +++ b/xen/arch/arm/arm64/mmu/mm.c
> @@ -6,6 +6,7 @@
>
> #include
> #include
> +#include
>
> /* Override macros from asm/page.h to make them work with mfn_t */
> #undef virt_to_mfn
> @@ -236,6 +237,7 @@ void __init setup_mm(void)
> max_page = PFN_DOWN(ram_end);
>
> init_staticmem_pages();
> +init_sharedmem_pages();
> }
>
> /*
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index ecbc80d6a112..f2344863062e 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -499,6 +499,7 @@ static void __init
Re: [PATCH 06/11] xen/arm: Avoid code duplication in find_unallocated_memory
Hi Luca,
On 20/03/2024 15:53, Luca Fancellu wrote:
>
>
>> On 20 Mar 2024, at 10:57, Michal Orzel wrote:
>>
>> Hi Luca,
>>
>> On 12/03/2024 14:03, Luca Fancellu wrote:
>>>
>>>
>>> The function find_unallocated_memory is using the same code to
>>> loop through 3 structure of the same type, in order to avoid
>>> code duplication, rework the code to have only one loop that
>>> goes through all the structures.
>>>
>>> Signed-off-by: Luca Fancellu
>>> ---
>>> xen/arch/arm/domain_build.c | 62 ++---
>>> 1 file changed, 17 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index b254f252e7cb..d0f2ac6060eb 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long
>>> s_gfn, unsigned long e_gfn,
>>> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>>> struct membanks *ext_regions)
>>> {
>>> -const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo);
>>> -const struct membanks *mem = bootinfo_get_mem();
>>> -const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
>>> +const struct membanks *mem_banks[] = {
>>> +bootinfo_get_mem(),
>>> +kernel_info_get_mem(kinfo),
>>> +bootinfo_get_reserved_mem(),
>>> +};
>>> struct rangeset *unalloc_mem;
>>> paddr_t start, end;
>>> -unsigned int i;
>>> +unsigned int i, j;
>>> int res;
>>>
>>> dt_dprintk("Find unallocated memory for extended regions\n");
>>> @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const
>>> struct kernel_info *kinfo,
>>> if ( !unalloc_mem )
>>> return -ENOMEM;
>>>
>>> -/* Start with all available RAM */
>>> -for ( i = 0; i < mem->nr_banks; i++ )
>>> -{
>>> -start = mem->bank[i].start;
>>> -end = mem->bank[i].start + mem->bank[i].size;
>>> -res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
>>> - PFN_DOWN(end - 1));
>>> -if ( res )
>>> -{
>>> -printk(XENLOG_ERR "Failed to add:
>>> %#"PRIpaddr"->%#"PRIpaddr"\n",
>>> - start, end);
>>> -goto out;
>>> -}
>>> -}
>>> -
>>> -/* Remove RAM assigned to Dom0 */
>>> -for ( i = 0; i < kinfo_mem->nr_banks; i++ )
>>> -{
>>> -start = kinfo_mem->bank[i].start;
>>> -end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
>>> -res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
>>> -PFN_DOWN(end - 1));
>>> -if ( res )
>>> +for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
>>> +for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
>> It might be a matter of personal opinion, but I would actually prefer the
>> current code
>> that looks simpler/neater (the steps are clear) to me. I'd like to know
>> other maintainers opinion.
>
> Ok, I’ll wait the other maintainers then, I’m going to use this construct in
> other part
> of the code to simplify and remove duplication so it would be important to
> know if
> It’s desirable or not.
>
> Maybe your opinion could change with a proper comment on top of the structure
> and the loop,
> listing the operation performed in order?
>
> 1) Start with all available RAM
> 2) Remove RAM assigned to Dom0
> 3) Remove reserved mem
>
> 4) Remove static shared memory regions
Yes, that would help with the overall readability.
~Michal
Re: [PATCH 06/11] xen/arm: Avoid code duplication in find_unallocated_memory
Hi Luca,
On 12/03/2024 14:03, Luca Fancellu wrote:
>
>
> The function find_unallocated_memory is using the same code to
> loop through 3 structure of the same type, in order to avoid
> code duplication, rework the code to have only one loop that
> goes through all the structures.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/domain_build.c | 62 ++---
> 1 file changed, 17 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b254f252e7cb..d0f2ac6060eb 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -869,12 +869,14 @@ static int __init add_ext_regions(unsigned long s_gfn,
> unsigned long e_gfn,
> static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>struct membanks *ext_regions)
> {
> -const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo);
> -const struct membanks *mem = bootinfo_get_mem();
> -const struct membanks *reserved_mem = bootinfo_get_reserved_mem();
> +const struct membanks *mem_banks[] = {
> +bootinfo_get_mem(),
> +kernel_info_get_mem(kinfo),
> +bootinfo_get_reserved_mem(),
> +};
> struct rangeset *unalloc_mem;
> paddr_t start, end;
> -unsigned int i;
> +unsigned int i, j;
> int res;
>
> dt_dprintk("Find unallocated memory for extended regions\n");
> @@ -883,50 +885,20 @@ static int __init find_unallocated_memory(const struct
> kernel_info *kinfo,
> if ( !unalloc_mem )
> return -ENOMEM;
>
> -/* Start with all available RAM */
> -for ( i = 0; i < mem->nr_banks; i++ )
> -{
> -start = mem->bank[i].start;
> -end = mem->bank[i].start + mem->bank[i].size;
> -res = rangeset_add_range(unalloc_mem, PFN_DOWN(start),
> - PFN_DOWN(end - 1));
> -if ( res )
> -{
> -printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n",
> - start, end);
> -goto out;
> -}
> -}
> -
> -/* Remove RAM assigned to Dom0 */
> -for ( i = 0; i < kinfo_mem->nr_banks; i++ )
> -{
> -start = kinfo_mem->bank[i].start;
> -end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size;
> -res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start),
> -PFN_DOWN(end - 1));
> -if ( res )
> +for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
> +for ( j = 0; j < mem_banks[i]->nr_banks; j++ )
It might be a matter of personal opinion, but I would actually prefer the
current code
that looks simpler/neater (the steps are clear) to me. I'd like to know other
maintainers opinion.
~Michal
Re: [PATCH 05/11] xen/arm: Introduce helper for static memory pages
Hi Luca,
On 12/03/2024 14:03, Luca Fancellu wrote:
>
>
> Introduce a new helper function in the static-memory module
> that can be called to manage static memory banks, this is
> done to reuse the code when other modules would like to
> manage static memory banks that are not part of the
> reserved_mem structure.
The placement of this patch in a series is a bit strange given no use of
init_staticmem_bank()
in the subsequent patch. I would move it right before patch #8.
Also, you could mention that this is because the information about shared
memory regions
will no longer be stored in reserved_mem.
>
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/include/asm/static-memory.h | 12
> xen/arch/arm/static-memory.c | 12 +---
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/static-memory.h
> b/xen/arch/arm/include/asm/static-memory.h
> index 3e3efd70c38d..665d4df50eda 100644
> --- a/xen/arch/arm/include/asm/static-memory.h
> +++ b/xen/arch/arm/include/asm/static-memory.h
> @@ -7,6 +7,18 @@
>
> #ifdef CONFIG_STATIC_MEMORY
>
> +static inline void init_staticmem_bank(const struct membank *bank)
> +{
> +mfn_t bank_start = _mfn(PFN_UP(bank->start));
The header should be self-contained so you should add that is not
included in asm/kernel.h path.
Other than that:
Reviewed-by: Michal Orzel
~Michal
Re: [PATCH 04/11] xen/arm: Conditional compilation of kernel_info.shm_mem member
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > The user of shm_mem member of the 'struct kernel_info' is only > the code managing the static shared memory feature, which can be > compiled out using CONFIG_STATIC_SHM, so in case the feature is > not requested, that member won't be used and will waste memory > space. > > To address this issue, protect the member with the Kconfig parameter > and modify the signature of the only function using it to remove > any reference to the member from outside the static-shmem module. > > Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel NIT: I always wonder why we have hundreds of functions taking both struct domain and struct kernel_info as arguments if the latter has the former as its member. As you are revisiting the function and modifying parameter list, you could take the opportunity to change it. But you don't have to. ~Michal
Re: [PATCH 03/11] xen/arm: Introduce a generic way to access memory bank structures
Hi Luca,
On 12/03/2024 14:03, Luca Fancellu wrote:
>
>
> Currently the 'stuct meminfo' is defining a static defined array of
> 'struct membank' of NR_MEM_BANKS elements, some feature like
> shared memory don't require such amount of memory allocation but
> might want to reuse existing code to manipulate this kind of
> structure that is just as 'struct meminfo' but less bulky.
>
> For this reason introduce a generic way to access this kind of
> structure using a new stucture 'struct membanks', which implements
s/stucture/structure
> all the fields needed by a structure related to memory banks
> without the need to specify at build time the size of the
> 'struct membank' array.
Might be beneficial here to mention the use of FAM.
>
> Modify 'struct meminfo' to implement the field related to the new
> introduced structure, given the change all usage of this structure
> are updated in this way:
> - code accessing bootinfo.{mem,reserved_mem,acpi} field now uses
>3 new introduced static inline helpers to access the new field
>of 'struct meminfo' named 'common'.
> - code accessing 'struct kernel_info *' member 'mem' now use the
>new introduced macro 'kernel_info_get_mem(...)' to access the
>new field of 'struct meminfo' named 'common'.
>
> Constify pointers where needed.
>
> Suggested-by: Julien Grall
> Signed-off-by: Luca Fancellu
> ---
> xen/arch/arm/acpi/domain_build.c| 6 +-
> xen/arch/arm/arm32/mmu/mm.c | 44 +-
> xen/arch/arm/arm64/mmu/mm.c | 2 +-
> xen/arch/arm/bootfdt.c | 27 +++---
> xen/arch/arm/dom0less-build.c | 18 ++--
> xen/arch/arm/domain_build.c | 106 +---
> xen/arch/arm/efi/efi-boot.h | 8 +-
> xen/arch/arm/efi/efi-dom0.c | 13 +--
> xen/arch/arm/include/asm/domain_build.h | 2 +-
> xen/arch/arm/include/asm/kernel.h | 9 ++
> xen/arch/arm/include/asm/setup.h| 40 -
> xen/arch/arm/include/asm/static-shmem.h | 4 +-
> xen/arch/arm/kernel.c | 12 +--
> xen/arch/arm/setup.c| 58 +++--
> xen/arch/arm/static-memory.c| 27 +++---
> xen/arch/arm/static-shmem.c | 34
> 16 files changed, 243 insertions(+), 167 deletions(-)
Lots of mechanical changes but in general I like this approach.
I'd like other maintainers to share their opinion.
[...]
> @@ -1157,10 +1163,12 @@ int __init make_hypervisor_node(struct domain *d,
> }
> else
> {
> -ext_regions = xzalloc(struct meminfo);
> +ext_regions = (struct membanks *)xzalloc(struct meminfo);
You're making assumption that struct membanks is the first member of meminfo
but there's no sanity check.
Why not xzalloc_flex_struct()?
> if ( !ext_regions )
> return -ENOMEM;
[...]
> diff --git a/xen/arch/arm/include/asm/kernel.h
> b/xen/arch/arm/include/asm/kernel.h
> index 0a23e86c2d37..d28b843c01a9 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -78,6 +78,15 @@ struct kernel_info {
> };
> };
>
> +#define kernel_info_get_mem(kinfo) \
> +(&(kinfo)->mem.common)
Why this is a macro but for bootinfo you use static inline helpers?
> +
> +#define KERNEL_INFO_INIT \
NIT: in most places we prefer \ to be aligned (the same apply to other places)
> +{ \
> +.mem.common.max_banks = NR_MEM_BANKS, \
> +.shm_mem.common.max_banks = NR_MEM_BANKS, \
> +}
> +
> /*
> * Probe the kernel to detemine its type and select a loader.
> *
> diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> index d15a88d2e0d1..a3e1dc8fdb6c 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -56,8 +56,14 @@ struct membank {
> #endif
> };
>
> -struct meminfo {
> +struct membanks {
> unsigned int nr_banks;
> +unsigned int max_banks;
> +struct membank bank[];
> +};
> +
> +struct meminfo {
> +struct membanks common;
You were supposed to make sure there is no extra padding here. I don't see any
check in this patch.
I'd at least do sth like:
BUILD_BUG_ON((offsetof(struct membanks, bank)) != (offsetof(struct meminfo,
bank)));
~Michal
Re: [PATCH 02/11] xen/arm: avoid repetitive checking in process_shm_node
Hi Luca, On 12/03/2024 14:03, Luca Fancellu wrote: > > > From: Penny Zheng > > Putting overlap and overflow checking in the loop is causing repetitive > operation, so this commit extracts both checking outside the loop. > > Signed-off-by: Penny Zheng > Signed-off-by: Luca Fancellu > --- > v1: > - Rework of > https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-3-penny.zh...@arm.com/ > - use strncmp to match the branch above I don't understand why we need strncmp if we already validated that shm_id is NULL terminated within MAX_SHM_ID_LENGTH at the beginning of the function with strnlen. Other than that: Reviewed-by: Michal Orzel ~Michal
[PATCH] xen/nospec: Include
After introduction of lock_evaluate_nospec() using bool type, building
Xen on Arm with UBSAN enabled fails:
In file included from ./include/xen/spinlock.h:4,
from common/ubsan/ubsan.c:13:
./include/xen/nospec.h:79:22: error: unknown type name ‘bool’
79 | static always_inline bool lock_evaluate_nospec(bool condition)
There is no issue on x86, as xen/stdbool.h is included somewhere along
the asm/nospec.h path, which is not the case for other architectures.
Fixes: 7ef0084418e1 ("x86/spinlock: introduce support for blocking speculation
into critical regions")
Signed-off-by: Michal Orzel
---
xen/include/xen/nospec.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index e8d73f9538e5..9fb15aa26aa9 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -7,6 +7,8 @@
#ifndef XEN_NOSPEC_H
#define XEN_NOSPEC_H
+#include
+
#include
/**
--
2.25.1
Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU
On 11/03/2024 14:46, Michal Orzel wrote:
>
>
> Hi Henry,
>
> On 08/03/2024 02:54, Henry Wang wrote:
>> For 1:1 direct-mapped dom0less DomUs, the magic pages should not clash
>> with any RAM region. To find a proper region for guest magic pages,
>> we can reuse the logic of finding domain extended regions.
>>
>> Extract the logic of finding domain extended regions to a helper
>> function named find_unused_memory() and use it to find unallocated
>> spaces for magic pages before make_hypervisor_node(). The result magic
>> page region is added to the reserved memory section of the bootinfo so
>> that it is carved out from the extended regions.
>>
>> Reported-by: Alec Kwapis
>> Signed-off-by: Henry Wang
>> ---
>> v2:
>> - New patch
>> ---
>> xen/arch/arm/dom0less-build.c | 43 +
>> xen/arch/arm/domain_build.c | 30 ++---
>> xen/arch/arm/include/asm/domain_build.h | 2 ++
>> 3 files changed, 64 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 1e1c8d83ae..99447bfb0c 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d,
>> struct kernel_info *kinfo)
>>
>> if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>> {
>> +if ( is_domain_direct_mapped(d) )
>> +{
> This whole block is dependent on static memory feature that is compiled out
> by default.
> Shouldn't you move it to static-memory.c ?
>
>> +struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
> I can't see corresponding xfree(avail_magic_regions). It's not going to be
> used after unused memory
> regions are retrieved.
>
>> +struct meminfo *rsrv_mem = _mem;
>> +struct mem_map_domain *mem_map = >arch.mem_map;
>> +uint64_t magic_region_start = INVALID_PADDR;
> What's the purpose of this initialization? magic_region_start is going to be
> re-assigned before making use of this value.
>
>> +uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> Why not paddr_t?
>
>> +unsigned int i;
>> +
>> +if ( !avail_magic_regions )
>> +return -ENOMEM;
> What about memory allocated for kinfo->fdt? You should goto err;
>
>> +
>> +ret = find_unused_memory(d, kinfo, avail_magic_regions);
>> +if ( ret )
>> +{
>> +printk(XENLOG_WARNING
>> + "%pd: failed to find a region for domain magic
>> pages\n",
>> + d);
>> +goto err;
> What about memory allocated for avail_magic_regions? You should free it.
>
>> +}
>> +
>> +magic_region_start = avail_magic_regions->bank[0].start;
>> +
>> +/*
>> + * Register the magic region as reserved mem to make sure this
>> + * region will not be counted when allocating extended regions.
> Well, this is only true in case find_unallocated_memory() is used to retrieve
> free regions.
> What if our direct mapped domU used partial dtb and IOMMU is in use? In this
> case,
> find_memory_holes() will be used and the behavior will be different.
>
> Also, I'm not sure if it is a good idea to call find_unused_memory twice
> (with lots of steps inside)
> just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions)
> region for magic pages.
> I'll let other maintainers share their opinion.
>
> Also, CCing Carlo since he was in a need of retrieving free memory regions as
> well for cache coloring with dom0.
In the end, I forgot to CC Carlo. Adding him now.
~Michal
