Re: [Xen-devel] [PATCH 4/4] x86/iommu: add PVH support to the inclusive options

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jul 31, 2018 at 08:36:02AM +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: 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 4/4] x86/iommu: add PVH support to the
> > inclusive options
> > 
> > Several people have reported hardware issues (malfunctioning USB
> > controllers) due to iommu page faults. Those faults are caused by
> > missing RMRR (VTd) or IRVS (AMD-Vi) entries in the ACPI tables. Those
> > can be worked around on VTd hardware by manually adding RMRR entries
> > on the command line, this is however limited to Intel hardware and
> > quite cumbersome to do.
> > 
> > In order to solve those issues add PVH support to the inclusive option
> > that identity maps all regions marked as reserved in the memory map.
> > Note that regions used by devices emulated by Xen (LAPIC, IO-APIC or
> > PCIe MCFG regions) are specifically avoided. Note that this option
> > currently relies on no MSIX MMIO areas residing in a reserved region,
> > or else Xen won't be able to trap those accesses.
> > 
> > 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>
> > ---
> >  docs/misc/xen-command-line.markdown | 16 ++++--
> >  xen/drivers/passthrough/x86/iommu.c | 82 +++++++++++++++++++++++--
> > ----
> >  2 files changed, 77 insertions(+), 21 deletions(-)
> > 
> > diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-
> > command-line.markdown
> > index 91a8bfc9a6..c7c9a38c19 100644
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1203,11 +1203,17 @@ detection of systems known to misbehave upon
> > accesses to that port.
> >  > Default: `true`
> > 
> >  >> Use this to work around firmware issues providing incorrect RMRR or
> > IVMD
> > ->> 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. Note that this option is only
> > ->> applicable to a PV dom0. Also note that if `dom0-strict` mode is enabled
> > ->> then conventional RAM pages not assigned to dom0 will not be mapped.
> > +>> entries. The behaviour of this option is slightly different between a PV
> > and
> > +>> a PVH Dom0:
> > +>>
> > +>> * For a PV Dom0 all pages up to 4GB not marked as unusable in the
> > memory
> > +>>   map 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.
> > +>>
> > +>> * For a PVH Dom0 all memory regions marked as reserved in the
> > memory map
> > +>>   that don't overlap with any MMIO region from emulated devices will be
> > +>>   identity mapped.
> > 
> >  ### iommu\_dev\_iotlb\_timeout
> >  > `= <integer>`
> > diff --git a/xen/drivers/passthrough/x86/iommu.c
> > b/xen/drivers/passthrough/x86/iommu.c
> > index 24cc591aa5..cfafe1b572 100644
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -20,6 +20,8 @@
> >  #include <xen/softirq.h>
> >  #include <xsm/xsm.h>
> > 
> > +#include <asm/apicdef.h>
> > +#include <asm/io_apic.h>
> >  #include <asm/setup.h>
> > 
> >  void iommu_update_ire_from_apic(
> > @@ -134,11 +136,62 @@ void arch_iommu_domain_destroy(struct domain
> > *d)
> >  {
> >  }
> > 
> > +static bool __hwdom_init pv_inclusive_map(unsigned long pfn,
> > +                                          unsigned long max_pfn)
> 
> Perhaps pv_hwdom_inclusive_map() (and similarly pvh_hwdom_inclusive_map()) to 
> make it obvious that they are intended only for the hardware domain. (I know 
> the annotation makes this reasonably obvious but other hwdom-specific 
> functions seem to carry this in their name)

Given the naming conversation in patch 2 I think this should be named
hwdom_inclusive_map and the new function should be hwdom_reserved_map.
I think the pv/pvh prefix can be avoided if you think the reserved
mapping is helpful for classic PV also.

Thanks, Roger.

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