Re: [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 31 July 2018 10:42
> To: Roger Pau Monne <roger.pau@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Paul
> Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Kevin
> Tian <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>;
> xen-devel <xen-devel@lists.xenproject.org>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> iommu_inclusive_mapping
> 
> >>> On 31.07.18 at 11:34, <roger.pau@citrix.com> wrote:
> > On Tue, Jul 31, 2018 at 03:14:47AM -0600, Jan Beulich wrote:
> >> >>> On 31.07.18 at 11:05, <roger.pau@citrix.com> wrote:
> >> > On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
> >> >> >>> On 31.07.18 at 10:37, <Paul.Durrant@citrix.com> wrote:
> >> >> >>  -----Original Message-----
> >> >> >> From: Roger Pau Monne
> >> >> >> Sent: 31 July 2018 09:34
> >> >> >> To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> >> Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> >> >> >> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> <wei.liu2@citrix.com>;
> >> >> >> George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Tim
> >> >> >> (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>; Jan
> Beulich
> >> >> >> <jbeulich@suse.com>
> >> >> >> Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> iommu_inclusive_mapping
> >> >> >>
> >> >> >> On Tue, Jul 31, 2018 at 09:27:03AM +0100, Paul Durrant wrote:
> >> >> >> > > -----Original Message-----
> >> >> >> > > From: Roger Pau Monne
> >> >> >> > > Sent: 31 July 2018 09:16
> >> >> >> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> >> >> > > Cc: xen-devel@lists.xenproject.org; Kevin Tian
> <kevin.tian@intel.com>;
> >> >> >> > > Stefano Stabellini <sstabellini@kernel.org>; Wei Liu
> >> >> >> <wei.liu2@citrix.com>;
> >> >> >> > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>;
> >> >> >> Tim
> >> >> >> > > (Xen.org) <tim@xen.org>; Julien Grall <julien.grall@arm.com>;
> Jan
> >> >> >> Beulich
> >> >> >> > > <jbeulich@suse.com>
> >> >> >> > > Subject: Re: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> > > iommu_inclusive_mapping
> >> >> >> > >
> >> >> >> > > On Tue, Jul 31, 2018 at 08:18:36AM +0100, Paul Durrant wrote:
> >> >> >> > > > > -----Original Message-----
> >> >> >> > > > > From: Xen-devel [mailto:xen-devel-
> bounces@lists.xenproject.org]
> >> >> >> On
> >> >> >> > > Behalf
> >> >> >> > > > > Of Roger Pau Monne
> >> >> >> > > > > Sent: 27 July 2018 16:32
> >> >> >> > > > > To: xen-devel@lists.xenproject.org
> >> >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>; Stefano Stabellini
> >> >> >> > > > > <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>;
> George
> >> >> >> Dunlap
> >> >> >> > > > > <George.Dunlap@citrix.com>; Andrew Cooper
> >> >> >> > > > > <Andrew.Cooper3@citrix.com>; Ian Jackson
> >> >> >> <Ian.Jackson@citrix.com>;
> >> >> >> > > Tim
> >> >> >> > > > > (Xen.org) <tim@xen.org>; Julien Grall
> <julien.grall@arm.com>; Jan
> >> >> >> > > Beulich
> >> >> >> > > > > <jbeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> >> >> >> > > > > Subject: [Xen-devel] [PATCH 2/4] iommu: generalize
> >> >> >> > > > > iommu_inclusive_mapping
> >> >> >> > > > >
> >> >> >> > > > > Introduce a new iommu=inclusive generic option that
> supersedes
> >> >> >> > > > > iommu_inclusive_mapping. This should be a non-functional
> change
> >> >> >> on
> >> >> >> > > > > Intel hardware, while AMD hardware will gain the same
> functionality
> >> >> >> of
> >> >> >> > > > > mapping almost everything below the 4GB boundary.
> >> >> >> > > > >
> >> >> >> > > > > Note that is a noop for ARM hardware.
> >> >> >> > > > >
> >> >> >> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> >> > > > > ---
> >> >> >> > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >> >> >> > > > > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> >> >> >> > > > > Cc: Ian Jackson <ian.jackson@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: Wei Liu <wei.liu2@citrix.com>
> >> >> >> > > > > Cc: Kevin Tian <kevin.tian@intel.com>
> >> >> >> > > > > ---
> >> >> >> > > > >  docs/misc/xen-command-line.markdown   | 14 ++++++
> >> >> >> > > > >  xen/drivers/passthrough/arm/iommu.c   |  4 ++
> >> >> >> > > > >  xen/drivers/passthrough/iommu.c       |  6 +++
> >> >> >> > > > >  xen/drivers/passthrough/vtd/extern.h  |  2 -
> >> >> >> > > > >  xen/drivers/passthrough/vtd/iommu.c   |  6 ---
> >> >> >> > > > >  xen/drivers/passthrough/vtd/x86/vtd.c | 66 +-----------------
> -------
> >> >> >> > > > >  xen/drivers/passthrough/x86/iommu.c   | 70
> >> >> >> > > > > +++++++++++++++++++++++++++
> >> >> >> > > > >  xen/include/xen/iommu.h               |  2 +
> >> >> >> > > > >  8 files changed, 97 insertions(+), 73 deletions(-)
> >> >> >> > > > >
> >> >> >> > > > > diff --git a/docs/misc/xen-command-line.markdown
> >> >> >> b/docs/misc/xen-
> >> >> >> > > > > command-line.markdown
> >> >> >> > > > > index 65b4754418..91a8bfc9a6 100644
> >> >> >> > > > > --- a/docs/misc/xen-command-line.markdown
> >> >> >> > > > > +++ b/docs/misc/xen-command-line.markdown
> >> >> >> > > > > @@ -1198,6 +1198,17 @@ detection of systems known to
> misbehave
> >> >> >> > > upon
> >> >> >> > > > > accesses to that port.
> >> >> >> > > > >
> >> >> >> > > > >  >> Enable IOMMU debugging code (implies `verbose`).
> >> >> >> > > > >
> >> >> >> > > > > +> `inclusive`
> >> >> >> > > >
> >> >> >> > > > This is a dom0 (or hwdom) specific setting so perhaps dom0-
> inclusive?
> >> >> >> > > >
> >> >> >> > > > Actually the dom0 iommu options are starting to get unwieldy
> as they
> >> >> >> are
> >> >> >> > > conflated with the general host iommu options so I think it may
> be
> >> >> >> > > worthwhile splitting things out into a separate 'dom0-iommu='
> top level
> >> >> >> > > parameter at this stage. (My reasons are slightly selfish as I
> intend to
> >
> >> >> > add
> >> >> >> > > another dom0 iommu option to give it just reserved regions, to
> avoid
> >> >> >> > > unnecessary set-up if we know it will be using PV-IOMMU).
> >> >> >> > >
> >> >> >> > > Mapping just the reserved regions is what I actually do for PVH
> with
> >> >> >> > > iommu=inclusive (patch 4/4), so maybe it would make sense to
> speak
> >> >> >> about
> >> >> >> > > the
> >> >> >> > > naming here in order to use the same naming for PV and PVH.
> >> >> >> > >
> >> >> >> > > TBH I don't really like the dom0- prefix, the command line
> iommu
> >> >> >> > > options either apply to all domains or Dom0 only, having
> >> >> >> > > domu-inclusive for example makes no sense IMO.
> >> >> >> >
> >> >> >> > No, I think there are some options that you may want to apply to
> dom0
> >> >> >> only, but these are more like the dom0_mem or dom0_max_vpus
> options.
> >> >> >> Particularly, the inclusive option is probably something that is only
> >> > desirable
> >> >> >> for dom0. Clearly dom0-passthrough and dom0-strict are already
> defined to
> >> >> >> relate to dom0 only, and options such as 'reserved' should only be
> > specific
> >> > on
> >> >> >> the command line in relation to dom0 IMO. For other domains such
> an option
> >> >> >> should be specified via xl.cfg.
> >> >> >>
> >> >> >> Yes, we already have a bunch of those, so then I think dom0-
> inclusive
> >> >> >> and dom0-reserved would be appropriate?
> >> >> >>
> >> >> >> dom0-inclusive-mapping or dom0-reserved-mapping seems too
> long.
> >> >> >
> >> >> > Yes, those names are ok, but I still think it better in the long run if we
> >
> >> >> > have something like:
> >> >> >
> >> >> > dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >> >> >
> >> >> > where relaxed is the default and 'none' (I think) is equivalent to the
> >> >> > current iommu=dom0-passthrough.
> >> >>
> >> >> Or, along the lines of the other reply just sent, e.g.
> >> >>
> >> >> dom0=pvh,iommu:inclusive;reserved,shadow
> >> >>
> >> >> But perhaps the difference between , and ; gets too confusing then.
> >> >
> >> > So I think we have the following options:
> >> >
> >> > 1. dom0_iommu=[inclusive,][reserved,][strict,][none,][relaxed]
> >>
> >> Nit: dom0-iommu= (no underscores in new options)
> >>
> >> > 2.
> dom0=[pvh,][shadow,][iommu=[inclusive;][reserved;][strict;][none;][relaxe
> d]]
> >> > 3. dom0=[pvh,][shadow,][iommu-inclusive,][iommu-reserved,][iommu-
> strict,][iommu-none,][iommu-relaxed]
> >> >
> >> > I don't have a strong preference between 1 and 3, but I would prefer
> >> > to avoid 2 because I think suboptions inside of options it's too
> >> > complex IMO.
> >>
> >> While generally I prefer to limit the number of top level options, in
> >> this case I think I'd prefer 1 after all. Or wait - does any pair of the
> >> (sub)options actually make sense to be specified?
> >
> > Yes, for example you can use strict and inclusive at the same time, I
> > think it's something like:
> >
> >
> dom0=[pvh,][shadow,][iommu=[inclusive|reserved;][strict|none|relaxed]]
> >
> >> Isn't it rather a
> >> choice of five than an enumeration of up to 5? In which case I'd
> >> still prefer 2 (as then there's no need for a second separator
> >> beside comma), the more that we have at least one example with
> >> such sub-options (cpufreq).
> >
> > OK, I can do the nested iommu option inside of dom0 if that's the
> > preference.
> 
> Well, no, then let's go with 1 (with the dash) I would say.
> 

+1

  Paul

> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel