Message ID | e79beeb5-7288-c8fd-8823-feaf8d4e7e77@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | None | expand |
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 13 September 2019 12:10 > To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien Grall <julien.grall@arm.com>; Andrew > Cooper <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Christian Lindig > <christian.lindig@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George Dunlap > <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Kevin Tian <kevin.tian@intel.com>; > Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; David > Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; WeiLiu <wl@xen.org> > Subject: [PATCH v11.1 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported > > 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. > > NOTE: This patch makes sure iommu_hap_pt_shared is clear if HAP is not > supported or the IOMMU is disabled, and defines it to false if > !CONFIG_HVM. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Christian Lindig <christian.lindig@citrix.com> Reviewed-by: Paul Durrant <paul.durrant@citrix.com> ...with one observation... [snip] > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -49,7 +49,11 @@ int8_t __hwdom_initdata iommu_hwdom_rese > * default until we find a good solution to resolve it. > */ > bool_t __read_mostly iommu_intpost; > -bool_t __read_mostly iommu_hap_pt_share = 1; > + > +#ifndef iommu_hap_pt_share > +bool __read_mostly iommu_hap_pt_share = true; > +#endif > + > bool_t __read_mostly iommu_debug; > bool_t __read_mostly amd_iommu_perdev_intremap = 1; > > @@ -102,8 +106,10 @@ static int __init parse_iommu_param(cons > iommu_hwdom_passthrough = val; > else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 ) > iommu_hwdom_strict = val; > +#ifndef iommu_hap_pt_share > else if ( (val = parse_boolean("sharept", s, ss)) >= 0 ) > iommu_hap_pt_share = val; > +#endif > else > rc = -EINVAL; > With this change there will be a command line parse error if 'no-sharept' is passed on the command line to a hypervisor built without CONFIG_HVM. I don't know whether you really want that behaviour, which is why my patch did: @@ -103,7 +107,14 @@ static int __init parse_iommu_param(const char *s) else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 ) iommu_hwdom_strict = val; else if ( (val = parse_boolean("sharept", s, ss)) >= 0 ) + { +#ifndef iommu_hap_pt_share iommu_hap_pt_share = val; +#else + if (val != iommu_hap_pt_share) + rc = -EINVAL; +#endif + } else rc = -EINVAL; Paul
On 13.09.2019 13:47, Paul Durrant wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 13 September 2019 12:10 >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org >> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien Grall <julien.grall@arm.com>; Andrew >> Cooper <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>; Christian Lindig >> <christian.lindig@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; George Dunlap >> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Kevin Tian <kevin.tian@intel.com>; >> Stefano Stabellini <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; David >> Scott <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; WeiLiu <wl@xen.org> >> Subject: [PATCH v11.1 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is supported >> >> 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. >> >> NOTE: This patch makes sure iommu_hap_pt_shared is clear if HAP is not >> supported or the IOMMU is disabled, and defines it to false if >> !CONFIG_HVM. >> >> Signed-off-by: Paul Durrant <paul.durrant@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Acked-by: Christian Lindig <christian.lindig@citrix.com> > > Reviewed-by: Paul Durrant <paul.durrant@citrix.com> Thanks. > ...with one observation... > > [snip] >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -49,7 +49,11 @@ int8_t __hwdom_initdata iommu_hwdom_rese >> * default until we find a good solution to resolve it. >> */ >> bool_t __read_mostly iommu_intpost; >> -bool_t __read_mostly iommu_hap_pt_share = 1; >> + >> +#ifndef iommu_hap_pt_share >> +bool __read_mostly iommu_hap_pt_share = true; >> +#endif >> + >> bool_t __read_mostly iommu_debug; >> bool_t __read_mostly amd_iommu_perdev_intremap = 1; >> >> @@ -102,8 +106,10 @@ static int __init parse_iommu_param(cons >> iommu_hwdom_passthrough = val; >> else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 ) >> iommu_hwdom_strict = val; >> +#ifndef iommu_hap_pt_share >> else if ( (val = parse_boolean("sharept", s, ss)) >= 0 ) >> iommu_hap_pt_share = val; >> +#endif >> else >> rc = -EINVAL; >> > > With this change there will be a command line parse error if 'no-sharept' is passed on the command line to a hypervisor built without CONFIG_HVM. I don't know whether you really want that behaviour, which is why my patch did: > > @@ -103,7 +107,14 @@ static int __init parse_iommu_param(const char *s) > else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 ) > iommu_hwdom_strict = val; > else if ( (val = parse_boolean("sharept", s, ss)) >= 0 ) > + { > +#ifndef iommu_hap_pt_share > iommu_hap_pt_share = val; > +#else > + if (val != iommu_hap_pt_share) > + rc = -EINVAL; > +#endif > + } > else > rc = -EINVAL; Yes, I did change this intentionally, as I think the "sharept" option (in its positive or negative form) should not be specified to Xen built with this tied to a fixed value. Jan
On Fri, Sep 13, 2019 at 01:10:18PM +0200, Jan Beulich wrote: > --- 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 > - Stray deletion? > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -96,9 +96,13 @@ struct xen_sysctl_tbuf_op { > /* The platform supports software paging. */ > #define _XEN_SYSCTL_PHYSCAP_shadow 4 > #define XEN_SYSCTL_PHYSCAP_shadow (1u<<_XEN_SYSCTL_PHYSCAP_shadow) > +/* The platform supports sharing of HAP page tables with the IOMMU. */ > +#define _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share 5 > +#define XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share \ > + (1u << _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share) I would drop the _hap part of this, since I don't think it adds much, it's not like the iommu page tables can be shared with anything else? I don't have a strong opinion, and given that the code already makes extensive use of iommu_hap_pt_share I would be fine with that. With the removed newline fixed (if applicable): Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> -----Original Message----- > From: Roger Pau Monne <roger.pau@citrix.com> > Sent: 13 September 2019 14:54 > To: Jan Beulich <jbeulich@suse.com> > Cc: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org; Suravee Suthikulpanit > <suravee.suthikulpanit@amd.com>; 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>; Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini > <sstabellini@kernel.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; David Scott > <dave@recoil.org>; Tim (Xen.org) <tim@xen.org>; WeiLiu <wl@xen.org> > Subject: Re: [PATCH v11.1 3/6] sysctl / libxl: report whether IOMMU/HAP page table sharing is > supported > > On Fri, Sep 13, 2019 at 01:10:18PM +0200, Jan Beulich wrote: > > --- 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 > > - > > Stray deletion? Yes, it is. It gets fixed up by a later patch though. > > > --- a/xen/include/public/sysctl.h > > +++ b/xen/include/public/sysctl.h > > @@ -96,9 +96,13 @@ struct xen_sysctl_tbuf_op { > > /* The platform supports software paging. */ > > #define _XEN_SYSCTL_PHYSCAP_shadow 4 > > #define XEN_SYSCTL_PHYSCAP_shadow (1u<<_XEN_SYSCTL_PHYSCAP_shadow) > > +/* The platform supports sharing of HAP page tables with the IOMMU. */ > > +#define _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share 5 > > +#define XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share \ > > + (1u << _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share) > > I would drop the _hap part of this, since I don't think it adds much, > it's not like the iommu page tables can be shared with anything else? > > I don't have a strong opinion, and given that the code already makes > extensive use of iommu_hap_pt_share I would be fine with that. > > With the removed newline fixed (if applicable): > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Paul
Hi Jan, On 9/13/19 12:10 PM, Jan Beulich wrote: > 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. > > NOTE: This patch makes sure iommu_hap_pt_shared is clear if HAP is not > supported or the IOMMU is disabled, and defines it to false if > !CONFIG_HVM. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Christian Lindig <christian.lindig@citrix.com> For the common code: Acked-by: Julien Grall <julien.grall@arm.com> Cheers,
> From: Jan Beulich [mailto:jbeulich@suse.com] > Sent: Friday, September 13, 2019 7:10 PM > > 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. > > NOTE: This patch makes sure iommu_hap_pt_shared is clear if HAP is not > supported or the IOMMU is disabled, and defines it to false if > !CONFIG_HVM. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Acked-by: Christian Lindig <christian.lindig@citrix.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
--- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -402,6 +402,8 @@ int libxl_get_physinfo(libxl_ctx *ctx, l physinfo->cap_hap = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_hap); physinfo->cap_shadow = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_shadow); + physinfo->cap_iommu_hap_pt_share = + !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share); GC_FREE; return 0; --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -402,6 +402,13 @@ #define LIBXL_HAVE_PHYSINFO_CAP_HAP_SHADOW 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 * * The only guarantee which libxl makes regarding ABI compatibility --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -1027,6 +1027,7 @@ libxl_physinfo = Struct("physinfo", [ ("cap_hvm_directio", bool), # No longer HVM specific ("cap_hap", bool), ("cap_shadow", bool), + ("cap_iommu_hap_pt_share", bool), ], dir=DIR_OUT) libxl_connectorinfo = Struct("connectorinfo", [ --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -110,6 +110,7 @@ type physinfo_cap_flag = | CAP_DirectIO | CAP_HAP | CAP_Shadow + | CAP_IOMMU_HAP_PT_SHARE type physinfo = { --- 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; @@ -95,6 +94,8 @@ type physinfo_cap_flag = | CAP_DirectIO | CAP_HAP | CAP_Shadow + | CAP_IOMMU_HAP_PT_SHARE + type physinfo = { threads_per_core : int; cores_per_socket : int; --- a/tools/xl/xl_info.c +++ b/tools/xl/xl_info.c @@ -210,13 +210,14 @@ 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%s\n", + maybe_printf("virt_caps :%s%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_shadow ? " shadow" : "" + info.cap_shadow ? " shadow" : "", + info.cap_iommu_hap_pt_share ? " iommu_hap_pt_share" : "" ); vinfo = libxl_get_version_info(ctx); --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -142,6 +142,23 @@ static struct notifier_block cpu_nfb = { .notifier_call = cpu_callback }; +static bool __init hap_supported(const struct hvm_function_table *fns) +{ + if ( !fns->hap_supported ) + { + printk("HVM: Hardware Assisted Paging (HAP) not detected\n"); + return false; + } + + if ( !opt_hap_enabled ) + { + printk("HVM: Hardware Assisted Paging (HAP) detected but disabled\n"); + return false; + } + + return true; +} + static int __init hvm_enable(void) { const struct hvm_function_table *fns = NULL; @@ -158,13 +175,8 @@ static int __init hvm_enable(void) hvm_enabled = 1; printk("HVM: %s enabled\n", fns->name); - if ( !fns->hap_supported ) - printk("HVM: Hardware Assisted Paging (HAP) not detected\n"); - else if ( !opt_hap_enabled ) - { - hvm_funcs.hap_supported = 0; - printk("HVM: Hardware Assisted Paging (HAP) detected but disabled\n"); - } + if ( !hap_supported(fns) ) + clear_iommu_hap_pt_share(); else { printk("HVM: Hardware Assisted Paging (HAP) detected\n"); --- a/xen/common/sysctl.c +++ b/xen/common/sysctl.c @@ -269,6 +269,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe arch_do_physinfo(pi); if ( iommu_enabled ) pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio; + if ( iommu_hap_pt_share ) + pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share; if ( copy_to_guest(u_sysctl, op, 1) ) ret = -EFAULT; --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -1406,7 +1406,8 @@ int __init amd_iommu_init(bool xt) * since it only supports p2m_ram_rw, and this would * prevent doing IO to/from mapped grant frames. */ - iommu_hap_pt_share = 0; + clear_iommu_hap_pt_share(); + printk(XENLOG_DEBUG "AMD-Vi: Disabled HAP memory map sharing with IOMMU\n"); /* per iommu initialization */ --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -49,7 +49,11 @@ int8_t __hwdom_initdata iommu_hwdom_rese * default until we find a good solution to resolve it. */ bool_t __read_mostly iommu_intpost; -bool_t __read_mostly iommu_hap_pt_share = 1; + +#ifndef iommu_hap_pt_share +bool __read_mostly iommu_hap_pt_share = true; +#endif + bool_t __read_mostly iommu_debug; bool_t __read_mostly amd_iommu_perdev_intremap = 1; @@ -102,8 +106,10 @@ static int __init parse_iommu_param(cons iommu_hwdom_passthrough = val; else if ( (val = parse_boolean("dom0-strict", s, ss)) >= 0 ) iommu_hwdom_strict = val; +#ifndef iommu_hap_pt_share else if ( (val = parse_boolean("sharept", s, ss)) >= 0 ) iommu_hap_pt_share = val; +#endif else rc = -EINVAL; @@ -511,7 +517,10 @@ int __init iommu_setup(void) iommu_enabled = (rc == 0); } if ( !iommu_enabled ) + { iommu_intremap = 0; + clear_iommu_hap_pt_share(); + } if ( (force_iommu && !iommu_enabled) || (force_intremap && !iommu_intremap) ) --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2310,7 +2310,7 @@ static int __init vtd_setup(void) iommu_intpost = 0; if ( !vtd_ept_page_compatible(iommu) ) - iommu_hap_pt_share = 0; + clear_iommu_hap_pt_share(); ret = iommu_set_interrupt(drhd); if ( ret ) --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -96,9 +96,13 @@ struct xen_sysctl_tbuf_op { /* The platform supports software paging. */ #define _XEN_SYSCTL_PHYSCAP_shadow 4 #define XEN_SYSCTL_PHYSCAP_shadow (1u<<_XEN_SYSCTL_PHYSCAP_shadow) +/* The platform supports sharing of HAP page tables with the IOMMU. */ +#define _XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share 5 +#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_shadow +#define XEN_SYSCTL_PHYSCAP_MAX XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share struct xen_sysctl_physinfo { uint32_t threads_per_core; --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -55,7 +55,22 @@ static inline bool_t dfn_eq(dfn_t x, dfn extern bool_t iommu_enable, iommu_enabled; extern bool_t force_iommu, iommu_verbose, iommu_igfx; extern bool_t iommu_snoop, iommu_qinval, iommu_intremap, iommu_intpost; -extern bool_t iommu_hap_pt_share; + +#ifdef CONFIG_HVM +extern bool iommu_hap_pt_share; +#else +#define iommu_hap_pt_share false +#endif + +static inline void clear_iommu_hap_pt_share(void) +{ +#ifndef iommu_hap_pt_share + iommu_hap_pt_share = false; +#elif iommu_hap_pt_share + ASSERT_UNREACHABLE(); +#endif +} + extern bool_t iommu_debug; extern bool_t amd_iommu_perdev_intremap;