| Message ID | 1634073720-27901-1-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V6,1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo | expand |
Hi Jan. May I please ask, are you OK with the proposed changes? On 13.10.21 00:22, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We need to pass info about maximum supported guest physical > address space size to the toolstack on Arm in order to properly > calculate the base and size of the extended region (safe range) > for the guest. The extended region is unused address space which > could be safely used by domain for foreign/grant mappings on Arm. > The extended region itself will be handled by the subsequent > patch. > > Currently the same guest physical address space size is used > for all guests (p2m_ipa_bits variable on Arm, the x86 equivalent > is hap_paddr_bits). > > Add an explicit padding after "gpaddr_bits" field and also > (while at it) after "domain" field. > > Also make sure that full structure is cleared in all cases by > moving the clearing into getdomaininfo(). Currently it is only > cleared by the sysctl caller (and only once). > > Please note, we do not need to bump XEN_DOMCTL_INTERFACE_VERSION > as a bump has already occurred in this release cycle. But we do > need to bump XEN_SYSCTL_INTERFACE_VERSION as the structure is > re-used in a sysctl. > > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > Reviewed-by: Ian Jackson <iwj@xenproject.org> > --- > Changes RFC -> V2: > - update patch subject/description > - replace arch-specific sub-struct with common gpaddr_bits > field and update code to reflect that > > Changes V2 -> V3: > - make the field uint8_t and add uint8_t pad[7] after > - remove leading blanks in libxl.h > > Changes V3 -> V4: > - also print gpaddr_bits from output_physinfo() > - add Michal's R-b > > Changes V4 -> V5: > - update patch subject and description > - drop Michal's R-b > - pass gpaddr_bits via createdomain domctl > (struct xen_arch_domainconfig) > > Changes V5 -> V6: > - update patch subject and description > - pass gpaddr_bits via getdomaininfo domctl > (struct xen_domctl_getdomaininfo) > > Changes V6 -> V7: > - update patch description > - do not bump XEN_DOMCTL_INTERFACE_VERSION > - bump XEN_SYSCTL_INTERFACE_VERSION > - add explicit paddings > - clear the full structure in getdomaininfo() > - add Ian's R-b > --- > tools/include/libxl.h | 8 ++++++++ > tools/include/xenctrl.h | 1 + > tools/libs/ctrl/xc_domain.c | 1 + > tools/libs/light/libxl_domain.c | 1 + > tools/libs/light/libxl_types.idl | 1 + > xen/arch/arm/domctl.c | 2 ++ > xen/arch/x86/domctl.c | 1 + > xen/common/domctl.c | 6 +++--- > xen/common/sysctl.c | 2 +- > xen/include/public/domctl.h | 3 +++ > xen/include/public/sysctl.h | 2 +- > 11 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > index b9ba16d..deb5022 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx; > #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1 > > /* > + * LIBXL_HAVE_DOMINFO_GPADDR_BITS > + * > + * If this is defined, libxl_dominfo will contain an uint8 field called > + * gpaddr_bits, containing the guest physical address space size. > + */ > +#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1 > + > +/* > * LIBXL_HAVE_QXL > * > * If defined, then the libxl_vga_interface_type will contain another value: > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index a306399..07b96e6 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -462,6 +462,7 @@ typedef struct xc_dominfo { > unsigned int max_vcpu_id; > xen_domain_handle_t handle; > unsigned int cpupool; > + uint8_t gpaddr_bits; > struct xen_arch_domainconfig arch_config; > } xc_dominfo_t; > > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c > index 23322b7..b155d6a 100644 > --- a/tools/libs/ctrl/xc_domain.c > +++ b/tools/libs/ctrl/xc_domain.c > @@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch, > info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus; > info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id; > info->cpupool = domctl.u.getdomaininfo.cpupool; > + info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits; > info->arch_config = domctl.u.getdomaininfo.arch_config; > > memcpy(info->handle, domctl.u.getdomaininfo.handle, > diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c > index 51a6127..544a9bf 100644 > --- a/tools/libs/light/libxl_domain.c > +++ b/tools/libs/light/libxl_domain.c > @@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx, > xlinfo->vcpu_max_id = xcinfo->max_vcpu_id; > xlinfo->vcpu_online = xcinfo->nr_online_vcpus; > xlinfo->cpupool = xcinfo->cpupool; > + xlinfo->gpaddr_bits = xcinfo->gpaddr_bits; > xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ? > LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV; > } > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index 3f9fff6..2df7258 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[ > ("vcpu_max_id", uint32), > ("vcpu_online", uint32), > ("cpupool", uint32), > + ("gpaddr_bits", uint8), > ("domain_type", libxl_domain_type), > ], dir=DIR_OUT) > > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c > index b7d27f3..6245af6 100644 > --- a/xen/arch/arm/domctl.c > +++ b/xen/arch/arm/domctl.c > @@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d, > { > /* All ARM domains use hardware assisted paging. */ > info->flags |= XEN_DOMINF_hap; > + > + info->gpaddr_bits = p2m_ipa_bits; > } > > static int handle_vuart_init(struct domain *d, > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 26a76d2..7d102e0 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d, > info->flags |= XEN_DOMINF_hap; > > info->arch_config.emulation_flags = d->arch.emulation_flags; > + info->gpaddr_bits = hap_paddr_bits; > } > > static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op, > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 12d6144..2d07a12 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 > @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) > > info->cpu_time = cpu_time; > > - info->flags = (info->nr_online_vcpus ? flags : 0) | > + info->flags |= (info->nr_online_vcpus ? flags : 0) | > ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) | > (d->is_shut_down ? XEN_DOMINF_shutdown : 0) | > (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) | > 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); > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 4cb3f66..46acc8f 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -106,6 +106,7 @@ struct xen_domctl_createdomain { > struct xen_domctl_getdomaininfo { > /* OUT variables. */ > domid_t domain; /* Also echoed in domctl.domain */ > + uint16_t pad1; > /* Domain is scheduled to die. */ > #define _XEN_DOMINF_dying 0 > #define XEN_DOMINF_dying (1U<<_XEN_DOMINF_dying) > @@ -150,6 +151,8 @@ struct xen_domctl_getdomaininfo { > uint32_t ssidref; > xen_domain_handle_t handle; > uint32_t cpupool; > + uint8_t gpaddr_bits; /* Guest physical address space size. */ > + uint8_t pad2[7]; > struct xen_arch_domainconfig arch_config; > }; > typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t; > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h > index 039ccf8..41ef7a2 100644 > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -35,7 +35,7 @@ > #include "domctl.h" > #include "physdev.h" > > -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013 > +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014 > > /* > * Read console content from Xen buffer ring.
On 12.10.2021 23:22, Oleksandr Tyshchenko wrote: > @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) > > info->cpu_time = cpu_time; > > - info->flags = (info->nr_online_vcpus ? flags : 0) | > + info->flags |= (info->nr_online_vcpus ? flags : 0) | > ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) | > (d->is_shut_down ? XEN_DOMINF_shutdown : 0) | > (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) | I don't think this is a useful change - you move from a simple write to a read-modify-write operation. With this dropped again, hypervisor parts: Reviewed-by: Jan Beulich <jbeulich@suse.com> While this has meanwhile moved quite far from the original proposal, I still wonder in how far Andrew may have remaining concerns. Did you check with him, perhaps on irc? But of course catching his attention may be difficult, so no (further) feedback by him should probably not keep this from getting committed (if no other open issues remain). Jan
On 13.10.21 16:56, Jan Beulich wrote: Hi Jan > On 12.10.2021 23:22, Oleksandr Tyshchenko wrote: >> @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) >> >> info->cpu_time = cpu_time; >> >> - info->flags = (info->nr_online_vcpus ? flags : 0) | >> + info->flags |= (info->nr_online_vcpus ? flags : 0) | >> ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) | >> (d->is_shut_down ? XEN_DOMINF_shutdown : 0) | >> (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) | > I don't think this is a useful change - you move from a simple write > to a read-modify-write operation. With this dropped again, hypervisor > parts: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks! Will drop. > > While this has meanwhile moved quite far from the original proposal, > I still wonder in how far Andrew may have remaining concerns. Did > you check with him, perhaps on irc? > But of course catching his > attention may be difficult, so no (further) feedback by him should > probably not keep this from getting committed (if no other open > issues remain). If I remember correctly, I made several attempts to clarify in the initial thread. But, I have to admit, not via IRC (I am expecting some troubles with my IRC client last time and I haven't figured out yet why). Of course, it would be correct to get a feedback to make sure that there would be no remaining concerns (as the main concern to use domctl was addressed) or otherwise. > > Jan >
Hi Oleksandr, On 13/10/2021 16:05, Oleksandr wrote: > On 13.10.21 16:56, Jan Beulich wrote: >> While this has meanwhile moved quite far from the original proposal, >> I still wonder in how far Andrew may have remaining concerns. Did >> you check with him, perhaps on irc? >> But of course catching his >> attention may be difficult, so no (further) feedback by him should >> probably not keep this from getting committed (if no other open >> issues remain). > > If I remember correctly, I made several attempts to clarify in the > initial thread. But, I have to admit, not via IRC (I am expecting some > troubles with my IRC client last time and I haven't figured out yet > why). Of course, it would be correct to get a feedback to make sure that > there would be no remaining concerns (as the main concern to use domctl > was addressed) or otherwise. I have pinged Andrew on IRC for you. Cheers,
On 13.10.21 18:17, Julien Grall wrote: > Hi Oleksandr, Hi Julien > > On 13/10/2021 16:05, Oleksandr wrote: >> On 13.10.21 16:56, Jan Beulich wrote: >>> While this has meanwhile moved quite far from the original proposal, >>> I still wonder in how far Andrew may have remaining concerns. Did >>> you check with him, perhaps on irc? >>> But of course catching his >>> attention may be difficult, so no (further) feedback by him should >>> probably not keep this from getting committed (if no other open >>> issues remain). >> >> If I remember correctly, I made several attempts to clarify in the >> initial thread. But, I have to admit, not via IRC (I am expecting >> some troubles with my IRC client last time and I haven't figured out >> yet why). Of course, it would be correct to get a feedback to make >> sure that there would be no remaining concerns (as the main concern >> to use domctl was addressed) or otherwise. > > I have pinged Andrew on IRC for you. Thank you for the help. > > Cheers, >
diff --git a/tools/include/libxl.h b/tools/include/libxl.h index b9ba16d..deb5022 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -874,6 +874,14 @@ typedef struct libxl__ctx libxl_ctx; #define LIBXL_HAVE_DOMINFO_NEVER_STOP 1 /* + * LIBXL_HAVE_DOMINFO_GPADDR_BITS + * + * If this is defined, libxl_dominfo will contain an uint8 field called + * gpaddr_bits, containing the guest physical address space size. + */ +#define LIBXL_HAVE_DOMINFO_GPADDR_BITS 1 + +/* * LIBXL_HAVE_QXL * * If defined, then the libxl_vga_interface_type will contain another value: diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index a306399..07b96e6 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -462,6 +462,7 @@ typedef struct xc_dominfo { unsigned int max_vcpu_id; xen_domain_handle_t handle; unsigned int cpupool; + uint8_t gpaddr_bits; struct xen_arch_domainconfig arch_config; } xc_dominfo_t; diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c index 23322b7..b155d6a 100644 --- a/tools/libs/ctrl/xc_domain.c +++ b/tools/libs/ctrl/xc_domain.c @@ -396,6 +396,7 @@ int xc_domain_getinfo(xc_interface *xch, info->nr_online_vcpus = domctl.u.getdomaininfo.nr_online_vcpus; info->max_vcpu_id = domctl.u.getdomaininfo.max_vcpu_id; info->cpupool = domctl.u.getdomaininfo.cpupool; + info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits; info->arch_config = domctl.u.getdomaininfo.arch_config; memcpy(info->handle, domctl.u.getdomaininfo.handle, diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c index 51a6127..544a9bf 100644 --- a/tools/libs/light/libxl_domain.c +++ b/tools/libs/light/libxl_domain.c @@ -306,6 +306,7 @@ void libxl__xcinfo2xlinfo(libxl_ctx *ctx, xlinfo->vcpu_max_id = xcinfo->max_vcpu_id; xlinfo->vcpu_online = xcinfo->nr_online_vcpus; xlinfo->cpupool = xcinfo->cpupool; + xlinfo->gpaddr_bits = xcinfo->gpaddr_bits; xlinfo->domain_type = (xcinfo->flags & XEN_DOMINF_hvm_guest) ? LIBXL_DOMAIN_TYPE_HVM : LIBXL_DOMAIN_TYPE_PV; } diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index 3f9fff6..2df7258 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -357,6 +357,7 @@ libxl_dominfo = Struct("dominfo",[ ("vcpu_max_id", uint32), ("vcpu_online", uint32), ("cpupool", uint32), + ("gpaddr_bits", uint8), ("domain_type", libxl_domain_type), ], dir=DIR_OUT) diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c index b7d27f3..6245af6 100644 --- a/xen/arch/arm/domctl.c +++ b/xen/arch/arm/domctl.c @@ -20,6 +20,8 @@ void arch_get_domain_info(const struct domain *d, { /* All ARM domains use hardware assisted paging. */ info->flags |= XEN_DOMINF_hap; + + info->gpaddr_bits = p2m_ipa_bits; } static int handle_vuart_init(struct domain *d, diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 26a76d2..7d102e0 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -151,6 +151,7 @@ void arch_get_domain_info(const struct domain *d, info->flags |= XEN_DOMINF_hap; info->arch_config.emulation_flags = d->arch.emulation_flags; + info->gpaddr_bits = hap_paddr_bits; } static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op, diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 12d6144..2d07a12 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 @@ -95,7 +95,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) info->cpu_time = cpu_time; - info->flags = (info->nr_online_vcpus ? flags : 0) | + info->flags |= (info->nr_online_vcpus ? flags : 0) | ((d->is_dying == DOMDYING_dead) ? XEN_DOMINF_dying : 0) | (d->is_shut_down ? XEN_DOMINF_shutdown : 0) | (d->controller_pause_count > 0 ? XEN_DOMINF_paused : 0) | 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); diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 4cb3f66..46acc8f 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -106,6 +106,7 @@ struct xen_domctl_createdomain { struct xen_domctl_getdomaininfo { /* OUT variables. */ domid_t domain; /* Also echoed in domctl.domain */ + uint16_t pad1; /* Domain is scheduled to die. */ #define _XEN_DOMINF_dying 0 #define XEN_DOMINF_dying (1U<<_XEN_DOMINF_dying) @@ -150,6 +151,8 @@ struct xen_domctl_getdomaininfo { uint32_t ssidref; xen_domain_handle_t handle; uint32_t cpupool; + uint8_t gpaddr_bits; /* Guest physical address space size. */ + uint8_t pad2[7]; struct xen_arch_domainconfig arch_config; }; typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t; diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 039ccf8..41ef7a2 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -35,7 +35,7 @@ #include "domctl.h" #include "physdev.h" -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000013 +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000014 /* * Read console content from Xen buffer ring.