| Message ID | 20190912111744.40410-4-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add per-domain IOMMU control | expand |
> On 12 Sep 2019, at 12:17, Paul Durrant <paul.durrant@citrix.com> wrote: > > tools/libxl/libxl_types.idl | 1 + > tools/ocaml/libs/xc/xenctrl.ml | 1 + > tools/ocaml/libs/xc/xenctrl.mli | 2 +- Acked-by: Christian Lindig <christian.lindig@citrix.com>
On 12.09.2019 13:17, Paul Durrant wrote: > --- a/xen/arch/arm/sysctl.c > +++ b/xen/arch/arm/sysctl.c > @@ -15,6 +15,9 @@ > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > { > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; > + > + if ( iommu_enabled && iommu_hap_pt_share ) > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > } I think this should be done in common code. > --- a/xen/arch/x86/sysctl.c > +++ b/xen/arch/x86/sysctl.c > @@ -164,7 +164,12 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > if ( IS_ENABLED(CONFIG_PV) ) > pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; > if ( hvm_hap_supported() ) > + { > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; > + > + if ( iommu_enabled && iommu_hap_pt_share ) > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > + } > } And if it's important to not have the bit set when !hvm_hap_supported(), then iommu_hap_pt_share should be cleared in __init code in this case. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 12 September 2019 14:04 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Christian Lindig > <christian.lindig@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; DavidScott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wl@xen.org> > Subject: Re: [PATCH v9 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported > > On 12.09.2019 13:17, Paul Durrant wrote: > > --- a/xen/arch/arm/sysctl.c > > +++ b/xen/arch/arm/sysctl.c > > @@ -15,6 +15,9 @@ > > void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > > { > > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; > > + > > + if ( iommu_enabled && iommu_hap_pt_share ) > > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > > } > > I think this should be done in common code. > > > --- a/xen/arch/x86/sysctl.c > > +++ b/xen/arch/x86/sysctl.c > > @@ -164,7 +164,12 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > > if ( IS_ENABLED(CONFIG_PV) ) > > pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; > > if ( hvm_hap_supported() ) > > + { > > pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; > > + > > + if ( iommu_enabled && iommu_hap_pt_share ) > > + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > > + } > > } > > And if it's important to not have the bit set when !hvm_hap_supported(), ...and that's what it’s not in common code... there is no hvm_hap_supported() for Arm > then iommu_hap_pt_share should be cleared in __init code in this case. That would have been an alternative approach if you'd not wanted it #defined in patch #5. (Yes it's a later patch in the series, but this one is later chronologically and I didn't want to invalidate the other patch.) I could perhaps implement hvm_hap_supported() for Arm? Paul > > Jan
On 12.09.2019 15:18, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 12 September 2019 14:04 >> To: Paul Durrant <Paul.Durrant@citrix.com> >> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper >> <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Christian Lindig >> <christian.lindig@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson >> <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk >> <konrad.wilk@oracle.com>; DavidScott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu >> <wl@xen.org> >> Subject: Re: [PATCH v9 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported >> >> On 12.09.2019 13:17, Paul Durrant wrote: >>> --- a/xen/arch/arm/sysctl.c >>> +++ b/xen/arch/arm/sysctl.c >>> @@ -15,6 +15,9 @@ >>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) >>> { >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; >>> + >>> + if ( iommu_enabled && iommu_hap_pt_share ) >>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; >>> } >> >> I think this should be done in common code. >> >>> --- a/xen/arch/x86/sysctl.c >>> +++ b/xen/arch/x86/sysctl.c >>> @@ -164,7 +164,12 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) >>> if ( IS_ENABLED(CONFIG_PV) ) >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; >>> if ( hvm_hap_supported() ) >>> + { >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; >>> + >>> + if ( iommu_enabled && iommu_hap_pt_share ) >>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; >>> + } >>> } >> >> And if it's important to not have the bit set when !hvm_hap_supported(), > > ...and that's what it’s not in common code... there is no hvm_hap_supported() for Arm > >> then iommu_hap_pt_share should be cleared in __init code in this case. > > That would have been an alternative approach if you'd not wanted > it #defined in patch #5. (Yes it's a later patch in the series, > but this one is later chronologically and I didn't want to > invalidate the other patch.) > I could perhaps implement hvm_hap_supported() for Arm? Well, implementing it for Arm is an option (at which point XEN_SYSCTL_PHYSCAP_hap could also move to common code). But personally I'd advise against providing more stubs than necessary on Arm. I may not understand your remark regarding patch 5: There's no problem with iommu_hap_pt_share now being a #define on Arm - whether the system is HVM capable is an x86 specific decision anyway, and hence the clearing of the flag could occur e.g. in hvm_enable() when it bails early. And btw., with !CONFIG_HVM it could be a #define on x86 as well, just to "false" then. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 12 September 2019 14:28 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: JulienGrall <julien.grall@arm.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Anthony Perard > <anthony.perard@citrix.com>; Christian Lindig <christian.lindig@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Stefano Stabellini > <sstabellini@kernel.org>; xen-devel@lists.xenproject.org; Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com>; DavidScott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > <wl@xen.org> > Subject: Re: [PATCH v9 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported > > On 12.09.2019 15:18, Paul Durrant wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: 12 September 2019 14:04 > >> To: Paul Durrant <Paul.Durrant@citrix.com> > >> Cc: xen-devel@lists.xenproject.org; Julien Grall <julien.grall@arm.com>; Andrew Cooper > >> <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Christian Lindig > >> <christian.lindig@citrix.com>; George Dunlap <George.Dunlap@citrix.com>; Ian Jackson > >> <Ian.Jackson@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk > >> <konrad.wilk@oracle.com>; DavidScott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; Wei Liu > >> <wl@xen.org> > >> Subject: Re: [PATCH v9 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is > supported > >> > >> On 12.09.2019 13:17, Paul Durrant wrote: > >>> --- a/xen/arch/arm/sysctl.c > >>> +++ b/xen/arch/arm/sysctl.c > >>> @@ -15,6 +15,9 @@ > >>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > >>> { > >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; > >>> + > >>> + if ( iommu_enabled && iommu_hap_pt_share ) > >>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > >>> } > >> > >> I think this should be done in common code. > >> > >>> --- a/xen/arch/x86/sysctl.c > >>> +++ b/xen/arch/x86/sysctl.c > >>> @@ -164,7 +164,12 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) > >>> if ( IS_ENABLED(CONFIG_PV) ) > >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; > >>> if ( hvm_hap_supported() ) > >>> + { > >>> pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; > >>> + > >>> + if ( iommu_enabled && iommu_hap_pt_share ) > >>> + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; > >>> + } > >>> } > >> > >> And if it's important to not have the bit set when !hvm_hap_supported(), > > > > ...and that's what it’s not in common code... there is no hvm_hap_supported() for Arm > > > >> then iommu_hap_pt_share should be cleared in __init code in this case. > > > > That would have been an alternative approach if you'd not wanted > > it #defined in patch #5. (Yes it's a later patch in the series, > > but this one is later chronologically and I didn't want to > > invalidate the other patch.) > > I could perhaps implement hvm_hap_supported() for Arm? > > Well, implementing it for Arm is an option (at which point > XEN_SYSCTL_PHYSCAP_hap could also move to common code). But > personally I'd advise against providing more stubs than > necessary on Arm. I may not understand your remark regarding > patch 5: There's no problem with iommu_hap_pt_share now being > a #define on Arm - whether the system is HVM capable is an > x86 specific decision anyway, and hence the clearing of the > flag could occur e.g. in hvm_enable() when it bails early. > And btw., with !CONFIG_HVM it could be a #define on x86 as > well, just to "false" then. Ok, I had assumed you meant to clear it common code too. But, yes, clearing it in x86 code would indeed work and I'll see about the !CONFIG_HVM definition. Paul > > Jan
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 5c0fcf320e..dd468eb18e 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -400,6 +400,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) physinfo->cap_hvm_directio = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_directio); physinfo->cap_hap = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_hap); + physinfo->cap_iommu_hap_pt_share = + !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share); GC_FREE; return 0; diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 3ff67792a7..670a282c5a 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -401,6 +401,13 @@ */ #define LIBXL_HAVE_PHYSINFO_CAP_HAP 1 +/* + * LIBXL_HAVE_PHYSINFO_CAP_IOMMU_HAP_PT_SHARE indicates that libxl_physinfo + * has a cap_iommu_hap_pt_share field that indicates whether the hardware + * supports sharing the IOMMU and HAP page tables. + */ +#define LIBXL_HAVE_PHYSINFO_CAP_IOMMU_HAP_PT_SHARE 1 + /* * libxl ABI compatibility * diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index 9e1f8515d3..bd427def1a 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -1026,6 +1026,7 @@ libxl_physinfo = Struct("physinfo", [ ("cap_pv", bool), ("cap_hvm_directio", bool), # No longer HVM specific ("cap_hap", bool), + ("cap_iommu_hap_pt_share", bool), ], dir=DIR_OUT) libxl_connectorinfo = Struct("connectorinfo", [ diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index eaf009f0f9..30da6ba370 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -109,6 +109,7 @@ type physinfo_cap_flag = | CAP_PV | CAP_DirectIO | CAP_HAP + | CAP_IOMMU_HAP_PT_SHARE type physinfo = { diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index e0636de23a..64c35418e8 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -57,7 +57,6 @@ type domain_create_flag = | CDF_OOS_OFF | CDF_XS_DOMAIN | CDF_IOMMU - type domctl_create_config = { ssidref: int32; handle: string; @@ -94,6 +93,7 @@ type physinfo_cap_flag = | CAP_PV | CAP_DirectIO | CAP_HAP + | CAP_IOMMU_HAP_PT_SHARE type physinfo = { threads_per_core : int; cores_per_socket : int; diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c index aa6724bc7f..94da5ec91e 100644 --- a/tools/xl/xl_info.c +++ b/tools/xl/xl_info.c @@ -210,12 +210,13 @@ static void output_physinfo(void) info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7] ); - maybe_printf("virt_caps :%s%s%s%s%s\n", + maybe_printf("virt_caps :%s%s%s%s%s%s\n", info.cap_pv ? " pv" : "", info.cap_hvm ? " hvm" : "", info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "", info.cap_pv && info.cap_hvm_directio ? " pv_directio" : "", - info.cap_hap ? " hap" : "" + info.cap_hap ? " hap" : "", + info.cap_iommu_hap_pt_share ? " iommu_hap_pt_share" : "" ); vinfo = libxl_get_version_info(ctx); diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c index f87944e847..6238c85be0 100644 --- a/xen/arch/arm/sysctl.c +++ b/xen/arch/arm/sysctl.c @@ -15,6 +15,9 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) { pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap; + + if ( iommu_enabled && iommu_hap_pt_share ) + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; } long arch_do_sysctl(struct xen_sysctl *sysctl, diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c index 5777a05ffc..c0adfbadb5 100644 --- a/xen/arch/x86/sysctl.c +++ b/xen/arch/x86/sysctl.c @@ -164,7 +164,12 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi) if ( IS_ENABLED(CONFIG_PV) ) pi->capabilities |= XEN_SYSCTL_PHYSCAP_pv; if ( hvm_hap_supported() ) + { pi->capabilities |= XEN_SYSCTL_PHYSCAP_hap; + + if ( iommu_enabled && iommu_hap_pt_share ) + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; + } } long arch_do_sysctl( diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index d4b455619c..99bac0d33e 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -93,9 +93,13 @@ struct xen_sysctl_tbuf_op { /* The platform supports Hardware Assisted Paging. */ #define _XEN_SYSCTL_PHYSCAP_hap 3 #define XEN_SYSCTL_PHYSCAP_hap (1u<<_XEN_SYSCTL_PHYSCAP_hap) +/* The platform supports sharing of HAP page tables with the IOMMU. */ +#define _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share 4 +#define XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share \ + (1u << _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share) /* Max XEN_SYSCTL_PHYSCAP_* constant. Used for ABI checking. */ -#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_hap +#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share struct xen_sysctl_physinfo { uint32_t threads_per_core;
This patch defines a new bit reported in the hw_cap field of struct xen_sysctl_physinfo to indicate whether the platform supports sharing of HAP page tables (i.e. the P2M) with the IOMMU. This informs the toolstack whether the domain needs extra memory to store discrete IOMMU page tables or not. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Julien Grall <julien.grall@arm.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Christian Lindig <christian.lindig@citrix.com> Cc: David Scott <dave@recoil.org> v9: - New in v9 --- tools/libxl/libxl.c | 2 ++ tools/libxl/libxl.h | 7 +++++++ tools/libxl/libxl_types.idl | 1 + tools/ocaml/libs/xc/xenctrl.ml | 1 + tools/ocaml/libs/xc/xenctrl.mli | 2 +- tools/xl/xl_info.c | 5 +++-- xen/arch/arm/sysctl.c | 3 +++ xen/arch/x86/sysctl.c | 5 +++++ xen/include/public/sysctl.h | 6 +++++- 9 files changed, 28 insertions(+), 4 deletions(-)