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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> 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? Do you want to restrict such workaround only for old
boxes?

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?

> 
>      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...

> +
> +        ASSERT(!xen_in_range(pfn));
> +
> +        a = rangeset_contains_range(mmio_ro_ranges, pfn, pfn) ?
> p2m_access_r
> +                                                              : 
> p2m_access_rw;
> +        rc = set_identity_p2m_entry(d, pfn, a, 0);
> +        if ( rc )
> +           printk(XENLOG_WARNING VTDPREFIX
> +                  " d%d: IOMMU mapping failed pfn %#lx: %d\n",
> +                  d->domain_id, pfn, rc);
> +    }
> +}
> +
> --
> 2.11.0 (Apple Git-81)

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