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

[Oleksandr <olekstysh@gmail.com>]

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