Re: [Xen-devel] [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.07.18 at 11:05, <roger.pau@xxxxxxxxxx> wrote:
> On Tue, Jul 31, 2018 at 02:49:19AM -0600, Jan Beulich wrote:
>> >>> On 31.07.18 at 10:37, <Paul.Durrant@xxxxxxxxxx> wrote:
>> >>  -----Original Message-----
>> >> From: Roger Pau Monne
>> >> Sent: 31 July 2018 09:34
>> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
>> >> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Kevin Tian <kevin.tian@xxxxxxxxx>;
>> >> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
>> >> <wei.liu2@xxxxxxxxxx>;
>> >> George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
>> >> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
>> >> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan Beulich
>> >> <jbeulich@xxxxxxxx>
>> >> 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@xxxxxxxxxx>
>> >> > > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Kevin Tian <kevin.tian@xxxxxxxxx>;
>> >> > > Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
>> >> <wei.liu2@xxxxxxxxxx>;
>> >> > > George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
>> >> > > <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
>> >> Tim
>> >> > > (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan
>> >> Beulich
>> >> > > <jbeulich@xxxxxxxx>
>> >> > > 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@xxxxxxxxxxxxxxxxxxxx]
>> >> On
>> >> > > Behalf
>> >> > > > > Of Roger Pau Monne
>> >> > > > > Sent: 27 July 2018 16:32
>> >> > > > > To: xen-devel@xxxxxxxxxxxxxxxxxxxx 
>> >> > > > > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
>> >> > > > > <sstabellini@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
>> >> Dunlap
>> >> > > > > <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
>> >> > > > > <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson
>> >> <Ian.Jackson@xxxxxxxxxx>;
>> >> > > Tim
>> >> > > > > (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Jan
>> >> > > Beulich
>> >> > > > > <jbeulich@xxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
>> >> > > > > 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@xxxxxxxxxx>
>> >> > > > > ---
>> >> > > > > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> >> > > > > Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> >> > > > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>> >> > > > > Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> >> > > > > Cc: Julien Grall <julien.grall@xxxxxxx>
>> >> > > > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> >> > > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> >> > > > > Cc: Tim Deegan <tim@xxxxxxx>
>> >> > > > > Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>> >> > > > > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
>> >> > > > > ---
>> >> > > > >  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;][relaxed]]
> 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? 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).

Jan


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