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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.07.18 at 17:31, <roger.pau@xxxxxxxxxx> wrote:
> 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.

But that would be a firmware bug anyway: These are sub-ranges
of PCI device BARs, and those must not overlap reserved ranges
in E820.

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

Why? You're looking for the guest view of things, which I don't
think you can derive from definitions in these two headers.

> +static bool __hwdom_init pvh_inclusive_map(const struct domain *d,
> +                                           unsigned long pfn)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * Ignore any address below 1MB, that's already identity mapped by the
> +     * domain builder.
> +     */
> +    if ( pfn < PFN_DOWN(MB(1)) )
> +        return false;
> +
> +    /* Only add reserved regions. */
> +    if ( !page_is_ram_type(pfn, RAM_TYPE_RESERVED) )
> +        return false;
> +
> +    /* Check that it doesn't overlap with the LAPIC */
> +    if ( pfn == PFN_DOWN(APIC_DEFAULT_PHYS_BASE) )

I.e. this should interact with vlapic.c.

> +        return false;
> +    /* ... or the IO-APIC */
> +    for ( i = 0; i < nr_ioapics; i++ )
> +        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> +            return false;

Ah, nr_ioapics legitimately comes from that header. But do you
really need that? Can't you use d->arch.hvm_domain.nr_vioapics?

> +    /* ... or the PCIe MCFG regions. */
> +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> +    {
> +        unsigned long addr = PFN_DOWN(pci_mmcfg_config[i].address);
> +
> +        if ( pfn >= addr + (pci_mmcfg_config[i].start_bus_number << 8) &&
> +             pfn < addr + (pci_mmcfg_config[i].end_bus_number << 8) )
> +            return false;
> +    }

Same here - this would better use domain state.

Also end_bus_number is inclusive iirc, so the above doesn't cover
the entire range as it seems.

Jan



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