| Message ID | 1633974539-7380-2-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space") | expand |
On Mon, 11 Oct 2021, 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). > > As we add new field to the structure bump the interface version. > > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> I couldn't spot any errors in this patch > --- > 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) > --- > 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/include/public/domctl.h | 3 ++- > 8 files changed, 17 insertions(+), 1 deletion(-) > > 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/include/public/domctl.h b/xen/include/public/domctl.h > index 4cb3f66..b93f776 100644 > --- 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 > > /* > * NB. xen_domctl.domain is an IN/OUT parameter for this operation. > @@ -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; > }; > typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t; > -- > 2.7.4 >
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. 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). > @@ -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. 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.) 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). Jan
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 >
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. >> 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. Jan
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. > > Jan >
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
Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> 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.
You say "to the toolstack", but you are exposing this information up
to callers of libxl. Do you mean some higher-layer toolstack that
uses libxl ? What does it use this information for ?
FTAOD I am not opposed to exposing this in this way; indeed it seems
likely to be useful. I just want to fully understand before I give
this my tools ack.
> + info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits;
I'm pleased to find that this is not arch-specific.
Thanks,
Ian.
On 12.10.21 18:15, Ian Jackson wrote: Hi Ian > Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"): >> 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. > You say "to the toolstack", but you are exposing this information up > to callers of libxl. Do you mean some higher-layer toolstack that > uses libxl ? What does it use this information for ? > > FTAOD I am not opposed to exposing this in this way; indeed it seems > likely to be useful. I just want to fully understand before I give > this my tools ack. I didn't mean any higher-layer toolstack, sorry if I was unclear. In the first place this information is needed by the entity which generates the device-tree for the guest on Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended regions to be inserted into the hypervisor node. > >> + info->gpaddr_bits = domctl.u.getdomaininfo.gpaddr_bits; > I'm pleased to find that this is not arch-specific. > > Thanks, > Ian.
Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> > Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> > You say "to the toolstack", but you are exposing this information up
> > to callers of libxl. Do you mean some higher-layer toolstack that
> > uses libxl ? What does it use this information for ?
> >
> > FTAOD I am not opposed to exposing this in this way; indeed it seems
> > likely to be useful. I just want to fully understand before I give
> > this my tools ack.
>
> I didn't mean any higher-layer toolstack, sorry if I was unclear. In the
> first place this information is
> needed by the entity which generates the device-tree for the guest on
> Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended
> regions to be inserted into the hypervisor node.
Right, OK. So I think this is being exposed at the libxl
gratuitously, because someone might want it in the future.
I approve :-).
Reviewed-by: Ian Jackson <iwj@xenproject.org>
Thanks,
Ian.
On 12.10.21 19:18, Ian Jackson wrote: Hi Ian > Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"): >>> Oleksandr Tyshchenko writes ("[PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"): >>> You say "to the toolstack", but you are exposing this information up >>> to callers of libxl. Do you mean some higher-layer toolstack that >>> uses libxl ? What does it use this information for ? >>> >>> FTAOD I am not opposed to exposing this in this way; indeed it seems >>> likely to be useful. I just want to fully understand before I give >>> this my tools ack. >> I didn't mean any higher-layer toolstack, sorry if I was unclear. In the >> first place this information is >> needed by the entity which generates the device-tree for the guest on >> Arm (tools/libs/light/libxl_arm.c) to properly calculate the extended >> regions to be inserted into the hypervisor node. > Right, OK. So I think this is being exposed at the libxl > gratuitously, because someone might want it in the future. > I approve :-). > > Reviewed-by: Ian Jackson <iwj@xenproject.org> Thanks! Please note, it is going to be a new version of this patch based on today's discussion with Jan. > > Thanks, > Ian.
Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"):
> Please note, it is going to be a new version of this patch based on
> today's discussion with Jan.
OK. Please retain my R-b if you don't change any of the tools parts.
Ian.
On 12.10.21 21:07, Ian Jackson wrote: Hi Ian > Oleksandr writes ("Re: [PATCH V6 1/2] xen/arm: Introduce gpaddr_bits field to struct xen_domctl_getdomaininfo"): >> Please note, it is going to be a new version of this patch based on >> today's discussion with Jan. > OK. Please retain my R-b if you don't change any of the tools parts. Sure, thanks for confirming. There won't be no new changes for the tools. > > Ian.
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/include/public/domctl.h b/xen/include/public/domctl.h index 4cb3f66..b93f776 100644 --- 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 /* * NB. xen_domctl.domain is an IN/OUT parameter for this operation. @@ -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; }; typedef struct xen_domctl_getdomaininfo xen_domctl_getdomaininfo_t;