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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> 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.

Jan