[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 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.



Oleksandr Tyshchenko



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