Re: [RFC PATCH] xen: Introduce arch specific field to XEN_SYSCTL_physinfo

[Oleksandr <olekstysh@gmail.com>]

On 16.08.21 10:33, Jan Beulich wrote:

Hi Jan

Sorry for the late response.

> On 14.08.2021 01:28, Oleksandr Tyshchenko wrote:
>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>
>> We need to pass info about maximum supported address space size
>> to the toolstack on Arm in order to properly calculate the base
>> and size of the safe range for the guest. Use p2m_ipa_bits variable
>> which purpose is to hold the bit size of IPAs in P2M tables.
> What is "the safe range"?

It is unallocated (unused) address space which could be safely used by 
domain for foreign/grant mappings on Arm, I will update description.


>
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -332,6 +332,11 @@ struct xen_arch_domainconfig {
>>        */
>>       uint32_t clock_frequency;
>>   };
>> +
>> +struct arch_physinfo {
>> +    /* Holds the bit size of IPAs in p2m tables. */
>> +    uint32_t p2m_ipa_bits;
>> +};
>>   #endif /* __XEN__ || __XEN_TOOLS__ */
>>   
>>   struct arch_vcpu_info {
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -346,6 +346,8 @@ typedef struct xen_msr_entry {
>>   } xen_msr_entry_t;
>>   DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
>>   
>> +struct arch_physinfo {
>> +};
>>   #endif /* !__ASSEMBLY__ */
> While the term "p2m_ipa_bits" surely isn't arch-agnostic, I wonder
> whether the expressed information is (the x86 equivalent being
> hap_paddr_bits, at a guess), and hence whether this really ought
> to live in an arch-specific sub-struct.

Well, on Arm we calculate the number of the IPA bits based on the number 
of PA bits when setting Stage 2 address translation.
I might mistake, but what we currently have on Arm is "ipa_bits == 
pa_bits". So, this means that information we need at the toolstack side 
isn't really arch-specific and
we could probably avoid introducing arch-specific sub-struct by suitable 
renaming the field (pa_bits, paddr_bits, whatever).

We could even name the field as hap_paddr_bits. Although, I don't know 
whether the hap_* is purely x86-ism, but I see there are several 
mentions in the common code (hap_pt_share, use_hap_pt, etc). Any 
suggestions?


> If indeed so, please name
> the struct in a name space clean way, i.e. add xen_ as prefix.

ok


>
> Also please retain a blank line before the #endif. I wonder whether
> on Arm you wouldn't want to add one at this occasion.

ok


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko