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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2021 15:18, Oleksandr wrote:
> On 12.10.21 14:49, Jan Beulich wrote:
>> 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 |= ..."

Oh, I guess you're right: I've been looking at my own tree, with
"paged_pages field is MEM_PAGING-only" and "shr_pages field is
MEM_SHARING-only" already applied. These sadly are still waiting
to go in, as they depend on earlier patches in their series.

Jan