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
