Re: [PATCH V2 1/3] xen: Introduce "gpaddr_bits" field to XEN_SYSCTL_physinfo

[Oleksandr <olekstysh@xxxxxxxxx>]

On 16.09.21 17:49, Jan Beulich wrote:

Hi Jan

> On 10.09.2021 20:18, Oleksandr Tyshchenko wrote:
> > --- a/tools/include/libxl.h
> > +++ b/tools/include/libxl.h
> > @@ -855,6 +855,13 @@ typedef struct libxl__ctx libxl_ctx;
> >    */
> >   #define LIBXL_HAVE_PHYSINFO_MAX_POSSIBLE_MFN 1
> >   
> > + /*
> > +  * LIBXL_HAVE_PHYSINFO_GPADDR_BITS
> > +  *
> > +  * If this is defined, libxl_physinfo has a "gpaddr_bits" field.
> > +  */
> > + #define LIBXL_HAVE_PHYSINFO_GPADDR_BITS 1
> Nit: I don't think you mean to have leading blanks here?
Yes, will remove.


> > @@ -120,6 +120,7 @@ struct xen_sysctl_physinfo {
> >       uint64_aligned_t outstanding_pages;
> >       uint64_aligned_t max_mfn; /* Largest possible MFN on this host */
> >       uint32_t hw_cap[8];
> > +    uint32_t gpaddr_bits;
> >   };
> Please make trailing padding explicit. I wonder whether this needs
> to be a 32-bit field: I expect we would need a full new ABI by the
> time we might reach 256 address bits. Otoh e.g. threads_per_core is
> pretty certainly oversized as well ...
I take it, this is a suggestion to make the field uint8_t and add 
uint8_t pad[7] after?

Please note, these are still unaddressed review comments for the last 
version [1], with a suggestion to use domctl (new?).
Also it is not entirely clear to me regarding whether this field will 
even remain gpaddr_bits or became maxphysaddr for example.
[1] 
https://lore.kernel.org/xen-devel/973f5344-aa10-3ad6-ff02-ad5f358ad279@xxxxxxxxxx/

> Jan
--
Regards,

Oleksandr Tyshchenko