Re: [Xen-devel] [PATCH 2/2] VT-d: reconcile iommu_inclusive_mapping and iommu=dom0-strict

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 11 June 2018 11:26
> 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/2] VT-d: reconcile
> iommu_inclusive_mapping and iommu=dom0-strict
> 
> On Fri, Jun 08, 2018 at 04:30:30PM +0100, Paul Durrant wrote:
> > The documentation for the iommu_inclusive_mapping Xen command line
> option
> > states:
> >
> > "Use this to work around firmware issues providing incorrect RMRR
> entries"
> >
> > Unfortunately this workaround does not function correctly if the dom0-
> strict
> > iommu option is also specified.
> >
> > The documentation goes on to say:
> >
> > "Rather than only mapping RAM pages for IOMMU accesses for Dom0, with
> this
> >  option all pages up to and including 4GB, not marked as unusable in the
> >  E820 table, will get a  mapping established."
> 
> My version of xen-command-line.markdown says:
> 
> "Use this to work around firmware issues providing incorrect RMRR entries.
> Rather than only mapping RAM pages for IOMMU accesses for Dom0, with
> this
> option all pages not marked as unusable in the E820 table will get a mapping
> established."

It will, without patch #1 applied.

> 
> I think the documentation or the code needs fixing, because the
> current code does indeed only map up to 4GB.
> 

That's what patch #1 does :-)

> >
> > This patch modifies the VT-d hardware domain initialization code such that
> > the workaround will continue to function in dom0-strict mode, by mapping
> > all pages not marked as unusable *unless* they are RAM pages not
> assigned
> > to dom0.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@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   | 4 +++-
> >  xen/drivers/passthrough/iommu.c       | 2 +-
> >  xen/drivers/passthrough/vtd/iommu.c   | 2 +-
> >  xen/drivers/passthrough/vtd/x86/vtd.c | 8 ++++++++
> >  xen/include/xen/iommu.h               | 2 +-
> >  5 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> command-line.markdown
> > index 6beb28dada..97768f1633 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1213,7 +1213,9 @@ wait descriptor timed out', try increasing this
> value.
> >  Use this to work around firmware issues providing incorrect RMRR entries.
> >  Rather than only mapping RAM pages for IOMMU accesses for Dom0, with
> this
> >  option all pages up to and including 4GB, not marked as unusable in the
> > -E820 table, will get a mapping established.
> > +E820 table, will get a mapping established. Note that if `dom0-strict`
> > +mode is enabled then conventional RAM pages not assigned to dom0 will
> not
> > +be mapped.
> >
> >  ### irq\_ratelimit (x86)
> >  > `= <integer>`
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index 2c44fabf99..a483212356 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -52,7 +52,7 @@ custom_param("iommu", parse_iommu_param);
> >  bool_t __initdata iommu_enable = 1;
> >  bool_t __read_mostly iommu_enabled;
> >  bool_t __read_mostly force_iommu;
> > -bool_t __hwdom_initdata iommu_dom0_strict;
> > +bool_t __read_mostly iommu_dom0_strict;
> 
> I'm not sure why you need to change the attributes of
> iommu_dom0_strict, AFAICT it's still only used in hwdom_init (or init)
> functions?
> 

That's true at this point. I plan to change it as part of PV-IOMMU. TBH though 
I prefer the consistency but I can drop the change if there are any major 
objections.

> >  bool_t __read_mostly iommu_verbose;
> >  bool_t __read_mostly iommu_workaround_bios_bug;
> >  bool_t __read_mostly iommu_igfx = 1;
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> > index 08bce92d40..e3f043288b 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1304,7 +1304,7 @@ static void __hwdom_init
> intel_iommu_hwdom_init(struct domain *d)
> >  {
> >      struct acpi_drhd_unit *drhd;
> >
> > -    if ( !iommu_passthrough && !need_iommu(d) )
> > +    if ( !iommu_passthrough )
> 
> I think you will have to add an is_pv_domain(d) check here, or else PVH
> Dom0 will also get those mappings, which is wrong because in the PVH
> case we don't want to identity map PFNs into guest p2m.
> 
> PVH Dom0 wasn't calling vtd_set_hwdom_mapping because of the
> !need_iommu check.

Ah, ok. I'll add that.

Cheers,

  Paul

> 
> Thanks, Roger.

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