Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig

[Oleksandr <olekstysh@gmail.com>]

On 07.10.21 03:49, Stefano Stabellini wrote:


Hi Stefano

> On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported guest physical
>> address space size to the toolstack on Arm in order to properly
>> calculate the base and size of the extended region (safe range)
>> for the guest. The extended region is unused address space which
>> could be safely used by domain for foreign/grant mappings on Arm.
>> The extended region itself will be handled by the subsequent
>> patch.
>>
>> Currently the same guest physical address space size is used
>> for all guests.
>>
>> As we add new field to the structure bump the interface version.
>>
>> Suggested-by: Julien Grall <jgrall@amazon.com>
>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>> Changes RFC -> V2:
>>     - update patch subject/description
>>     - replace arch-specific sub-struct with common gpaddr_bits
>>       field and update code to reflect that
>>
>> Changes V2 -> V3:
>>     - make the field uint8_t and add uint8_t pad[7] after
>>     - remove leading blanks in libxl.h
>>
>> Changes V3 -> V4:
>>     - also print gpaddr_bits from output_physinfo()
>>     - add Michal's R-b
>>
>> Changes V4 -> V5:
>>     - update patch subject and description
>>     - drop Michal's R-b
>>     - pass gpaddr_bits via createdomain domctl
>>       (struct xen_arch_domainconfig)
>> ---
>>   tools/include/libxl.h            | 5 +++++
>>   tools/libs/light/libxl_arm.c     | 2 ++
>>   tools/libs/light/libxl_types.idl | 1 +
>>   xen/arch/arm/domain.c            | 6 ++++++
>>   xen/include/public/arch-arm.h    | 5 +++++
>>   xen/include/public/domctl.h      | 2 +-
>>   6 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index b9ba16d..33b4bfb 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -279,6 +279,11 @@
>>   #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1
>>   
>>   /*
>> + * libxl_domain_build_info has the gpaddr_bits field.
>> + */
>> +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1
>> +
>> +/*
>>    * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing
>>    * 'soft reset' for domains and there is 'soft_reset' shutdown reason
>>    * in enum libxl_shutdown_reason.
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6..45e0386 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
>>   
>>       state->clock_frequency = config->arch.clock_frequency;
>>   
>> +    d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits;
>> +
>>       return 0;
>>   }
>>   
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 3f9fff6..39482db 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>   
>>       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>                                  ("vuart", libxl_vuart_type),
>> +                               ("gpaddr_bits", uint8),
>>                                 ])),
>>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                                 ])),
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 19c756a..dfecc45 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d,
>>       if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
>>           goto fail;
>>   
>> +    /*
>> +     * Pass maximum IPA bits to the toolstack, currently the same guest
>> +     * physical address space size is used for all guests.
>> +     */
>> +    config->arch.gpaddr_bits = p2m_ipa_bits;
> This could also be set in arch_sanitise_domain_config together with
> config->arch.gic_version. I prefer if it was done in
> arch_sanitise_domain_config but also here is OK I think.

I don't mind, being honest I had an idea to place this in 
arch_sanitise_domain_config(), but couldn't convince myself.


>
> Given that everything else looks fine:
>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Thanks!

Sadly, according to the recent discussion most likely this version is 
also no-go.


[snip]

-- 
Regards,

Oleksandr Tyshchenko