Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo

[Oleksandr <olekstysh@xxxxxxxxx>]

On 12.10.21 14:49, Jan Beulich wrote:

Hi Jan

> On 12.10.2021 13:28, Oleksandr wrote:
> > On 12.10.21 12:24, Jan Beulich wrote:
> > > On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
> > > > @@ -150,6 +150,7 @@ struct xen_domctl_getdomaininfo {
> > > >        uint32_t ssidref;
> > > >        xen_domain_handle_t handle;
> > > >        uint32_t cpupool;
> > > > +    uint8_t gpaddr_bits; /* Guest physical address space size. */
> > > >        struct xen_arch_domainconfig arch_config;
> > > On the basis of the above, please add uint8_t pad[3] (or perhaps
> > > better pad[7] to be independent of the present
> > > alignof(struct xen_arch_domainconfig) == 4) and make sure the
> > > array will always be zero-filled, such that the padding space can
> > > subsequently be assigned a purpose without needing to bump the
> > > interface version(s) again.
> > ok, will do.
> >
> >
> > > Right now the sysctl caller of getdomaininfo() clears the full
> > > structure (albeit only once, so stale / inapplicable data might get
> > > reported for higher numbered domains if any field was filled only
> > > in certain cases), while the domctl one doesn't. Maybe it would be
> > > best looking forward if the domctl path also cleared the full buffer
> > > up front, or if the clearing was moved into the function. (In the
> > > latter case some writes of zeros into the struct could be eliminated
> > > in return.)
> > Well, I would be OK either way, with a little preference for the latter.
> >
> > Is it close to what you meant?
> Yes, just that ...
>
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -69,10 +69,10 @@ void getdomaininfo(struct domain *d, struct
> > xen_domctl_getdomaininfo *info)
> >        int flags = XEN_DOMINF_blocked;
> >        struct vcpu_runstate_info runstate;
> >
> > +    memset(info, 0, sizeof(*info));
> > +
> >        info->domain = d->domain_id;
> >        info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID;
> > -    info->nr_online_vcpus = 0;
> > -    info->ssidref = 0;
> ... there are a few more "... = 0" further down iirc.
I didn't spot anything except "info->flags = ..." which probably can now 
be converted into "info->flags |= ..."

> > > Perhaps, once properly first zero-filling the struct in all cases,
> > > the padding field near the start should also be made explicit,
> > > clarifying visually that it is an option to use the space there for
> > > something that makes sense to live early in the struct (i.e. I
> > > wouldn't necessarily recommend putting there the new field you add,
> > > albeit - as mentioned near the top - that's certainly an option).
> > I read this as a request to also add uint16_t pad after "domid_t domain"
> > at least. Correct?
> I guess I should really leave this up to you - that's largely a cosmetic
> thing after all once clearing the whole struct up front.
ok, thank you for the clarification.


> Jan
--
Regards,

Oleksandr Tyshchenko