Re: [Xen-devel] [PATCH v2 3/4] x86/vtd: introduce a PVH implementation of iommu_inclusive_mapping

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Thu, Aug 17, 2017 at 03:28:29AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > Sent: Saturday, August 12, 2017 12:43 AM
> > 
> > On certain Intel systems, as far as I can tell almost all pre-Haswell ones,
> > trying to boot a PVH Dom0 will freeze the box completely, up to the point
> > that
> > not even the watchdog works. The freeze happens exactly when enabling
> > the DMA
> > remapping in the IOMMU, the last line seen is:
> > 
> > (XEN) [VT-D]iommu_enable_translation: iommu->reg = ffff82c00021b000
> > 
> > In order to workaround this (which seems to be a lack of proper RMRR
> > entries,
> 
> since you position this patch as 'workaround', what is the side-effect with
> such workaround?

Side effect is that Xen basically maps everything below 4GB to the PVH
Dom0 p2m. It's farily similar to what's already done for PV Dom0.

> Do you want to restrict such workaround only for old
> boxes?

Hm, I don't think so, the more that I don't think it's feasible to
identify the broken boxes from Xen's PoV.

> better you can also put some comment in the code so others can understand
> why pvh requires its own way when reading the code.
> 
> > plus the IOMMU being unable to generate faults and freezing the entire
> > system)
> > add a PVH specific implementation of iommu_inclusive_mapping, that
> > maps
> > non-RAM, non-unusable regions into Dom0 p2m. Note that care is taken to
> > not map
> > device MMIO regions that Xen is emulating, like the local APIC or the IO
> > APIC.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> > ---
> >  xen/drivers/passthrough/vtd/extern.h  |  1 +
> >  xen/drivers/passthrough/vtd/iommu.c   |  2 ++
> >  xen/drivers/passthrough/vtd/x86/vtd.c | 39
> > +++++++++++++++++++++++++++++++++++
> >  3 files changed, 42 insertions(+)
> > 
> > diff --git a/xen/drivers/passthrough/vtd/extern.h
> > b/xen/drivers/passthrough/vtd/extern.h
> > index fb7edfaef9..0eaf8956ff 100644
> > --- a/xen/drivers/passthrough/vtd/extern.h
> > +++ b/xen/drivers/passthrough/vtd/extern.h
> > @@ -100,5 +100,6 @@ bool_t platform_supports_intremap(void);
> >  bool_t platform_supports_x2apic(void);
> > 
> >  void vtd_set_hwdom_mapping(struct domain *d);
> > +void vtd_set_pvh_hwdom_mapping(struct domain *d);
> > 
> >  #endif // _VTD_EXTERN_H_
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index daaed0abbd..8ed28defe2 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1303,6 +1303,8 @@ static void __hwdom_init
> > intel_iommu_hwdom_init(struct domain *d)
> >          /* Set up 1:1 page table for hardware domain. */
> >          vtd_set_hwdom_mapping(d);
> >      }
> > +    else if ( is_hvm_domain(d) )
> > +        vtd_set_pvh_hwdom_mapping(d);
> 
> Can you elaborate a bit here? Current condition is:
> 
>     if ( !iommu_passthrough && !need_iommu(d) )
>     {
>         /* Set up 1:1 page table for hardware domain. */
>         vtd_set_hwdom_mapping(d);
>     }
> 
> So you assume for PVH above condition will never be true?

No, PVH Dom0 always requires an iommu, so the above condition will
never be true for a PVH Dom0.

> > 
> >      setup_hwdom_pci_devices(d, setup_hwdom_device);
> >      setup_hwdom_rmrr(d);
> > diff --git a/xen/drivers/passthrough/vtd/x86/vtd.c
> > b/xen/drivers/passthrough/vtd/x86/vtd.c
> > index 88a60b3307..79c9b0526f 100644
> > --- a/xen/drivers/passthrough/vtd/x86/vtd.c
> > +++ b/xen/drivers/passthrough/vtd/x86/vtd.c
> > @@ -21,10 +21,12 @@
> >  #include <xen/softirq.h>
> >  #include <xen/domain_page.h>
> >  #include <asm/paging.h>
> > +#include <xen/iocap.h>
> >  #include <xen/iommu.h>
> >  #include <xen/irq.h>
> >  #include <xen/numa.h>
> >  #include <asm/fixmap.h>
> > +#include <asm/p2m.h>
> >  #include <asm/setup.h>
> >  #include "../iommu.h"
> >  #include "../dmar.h"
> > @@ -159,3 +161,40 @@ void __hwdom_init
> > vtd_set_hwdom_mapping(struct domain *d)
> >      }
> >  }
> > 
> > +void __hwdom_init vtd_set_pvh_hwdom_mapping(struct domain *d)
> > +{
> > +    unsigned long pfn;
> > +
> > +    BUG_ON(!is_hardware_domain(d));
> > +
> > +    if ( !iommu_inclusive_mapping )
> > +        return;
> > +
> > +    /* NB: the low 1MB is already mapped in pvh_setup_p2m. */
> > +    for ( pfn = PFN_DOWN(MB(1)); pfn < PFN_DOWN(GB(4)); pfn++ )
> > +    {
> > +        p2m_access_t a;
> > +        int rc;
> > +
> > +        if ( !(pfn & 0xfff) )
> > +            process_pending_softirqs();
> > +
> > +        /* Skip RAM, ACPI and unusable regions. */
> > +        if ( page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) ||
> > +             page_is_ram_type(pfn, RAM_TYPE_UNUSABLE) ||
> > +             page_is_ram_type(pfn, RAM_TYPE_ACPI) ||
> > +             !iomem_access_permitted(d, pfn, pfn) )
> > +            continue;
> 
> I'm a bit confused here. So you only handle RESERVED memory
> type here, which doesn't match the definition of inclusive mapping.
> 
> /*
>  * iommu_inclusive_mapping: when set, all memory below 4GB is included in dom0
>  * 1:1 iommu mappings except xen and unusable regions.
>  */
> 
> there must be some background which I missed...

Right, RAM and ACPI regions are already mapped by the Dom0 builder, so
the only thing left are reserved regions not being used by Xen.

I can expand the comment above to say:

/*
 * Skip RAM, ACPI and unusable regions because they have been already
 * mapped by the PVH Dom0 builder.
 */

Does that seem better?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel