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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 14 June 2018 16:59
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org)
> <tim@xxxxxxx>
> Subject: Re: [PATCH v2 2/2] VT-d: reconcile iommu_inclusive_mapping and
> iommu=dom0-strict
> 
> >>> On 12.06.18 at 15:47, <paul.durrant@xxxxxxxxxx> 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 4GB, not marked as unusable in the E820 table, will
> >  get a mapping established."
> >
> > 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.
> 
> Hmm, so far I was under the impression that the two options aren't meant
> to work together anyway.
> 
> > --- 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 4GB, not marked as unusable in the E820 table, will
> > -get a mapping established.
> > +get a mapping established. Note that this option is only applicable to a
> > +PV dom0 and if `dom0-strict` mode is enabled then conventional RAM
> pages
> > +not assigned to dom0 will not be mapped.
> 
> I think for readability you want to either split the sentence into two
> (dropping
> the "and") or at least insert a comma ahead of the "and".

Ok.

> 
> > --- 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 && is_pv_domain(d) )
> 
> I don't understand this change: Where is the !need_iommu() condition
> getting enforced now? Oh, wait ...
> 
> > --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> > @@ -143,6 +143,14 @@ void __hwdom_init
> vtd_set_hwdom_mapping(struct domain *d)
> >          if ( xen_in_range(pfn) )
> >              continue;
> >
> > +        /*
> > +         * If dom0-strict mode is enabled then exclude conventional RAM
> > +         * and let the common code map dom0's pages.
> > +         */
> > +        if ( iommu_dom0_strict &&
> 
> This is equivalent to need_iommu(d), and if you wrote it that way the
> patch would be easier to review and ...
> 
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -29,7 +29,7 @@
> >  #include <asm/iommu.h>
> >
> >  extern bool_t iommu_enable, iommu_enabled;
> > -extern bool_t force_iommu, iommu_verbose;
> > +extern bool_t force_iommu, iommu_dom0_strict, iommu_verbose;
> 
> ... this wouldn't be needed.
> 

I could do that, but I think that checking the actual option rather than a 
side-effect of the option is preferable so I'd rather leave it this way. I'll 
add some text to the commit comment.

  Paul

> Jan
> 


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