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

[Oleksandr <olekstysh@gmail.com>]

On 12.10.21 12:24, Jan Beulich wrote:

Hi Jan

Thank you for thorough review.

> On 11.10.2021 19:48, Oleksandr Tyshchenko wrote:
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -38,7 +38,7 @@
>>   #include "hvm/save.h"
>>   #include "memory.h"
>>   
>> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014
>> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015
> So this bump would not have been needed if the rule of making padding
> fields explicit in the public interface had been followed by earlier
> changes, as you could have fit the 8-bit field in the 16-bit gap
> after domid.
>
> Furthermore this bump is only going to be necessary if your patch
> doesn't make 4.16. 4.15 uses 0x13 here, i.e. a bump has already
> occurred in this release cycle.

I got it, will remove the bumping.


>
> Otoh, because of the re-use of the struct in a sysctl, you do need
> to bump XEN_SYSCTL_INTERFACE_VERSION here (unless you fit the new
> field in the existing padding slot, which for the sysctl has been
> guaranteed to be zero; see also below).

Oops, indeed, will bump.


>
>> @@ -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?


diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 46a0c8a..9bca133 100644
--- 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;

      /*
       * - domain is marked as blocked only if all its vcpus are blocked
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 3558641..a7ab95d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -76,7 +76,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
u_sysctl)
      case XEN_SYSCTL_getdomaininfolist:
      {
          struct domain *d;
-        struct xen_domctl_getdomaininfo info = { 0 };
+        struct xen_domctl_getdomaininfo info;
          u32 num_domains = 0;

          rcu_read_lock(&domlist_read_lock);


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


>
> Jan
>
-- 
Regards,

Oleksandr Tyshchenko