[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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"
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;


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?



Oleksandr Tyshchenko



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.