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

[Oleksandr <olekstysh@xxxxxxxxx>]

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