> -----Original Message-----
> From: Roger Pau Monne
> Sent: 11 June 2018 11:26
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Kevin Tian <kevin.t...@intel.com>;
> Stefano Stabellini <sstabell...@kernel.org>; Wei Liu <wei.l...@citrix.com>;
> George Dunlap <george.dun...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Tim
> (Xen.org) <t...@xen.org>; Julien Grall <julien.gr...@arm.com>; Jan Beulich
> <jbeul...@suse.com>
> 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.durr...@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.coop...@citrix.com>
> > Cc: George Dunlap <george.dun...@eu.citrix.com>
> > Cc: Ian Jackson <ian.jack...@eu.citrix.com>
> > Cc: Jan Beulich <jbeul...@suse.com>
> > Cc: Julien Grall <julien.gr...@arm.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
> > Cc: Stefano Stabellini <sstabell...@kernel.org>
> > Cc: Tim Deegan <t...@xen.org>
> > Cc: Wei Liu <wei.l...@citrix.com>
> > Cc: Kevin Tian <kevin.t...@intel.com>
> > ---
> > 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
