Re: [Xen-devel] [PATCH 2/4] iommu: generalize iommu_inclusive_mapping

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.07.18 at 17:31, <roger.pau@xxxxxxxxxx> wrote:
> Introduce a new iommu=inclusive generic option that supersedes
> iommu_inclusive_mapping. This should be a non-functional change on
> Intel hardware, while AMD hardware will gain the same functionality of
> mapping almost everything below the 4GB boundary.

So first of all - what's the motivation behind this change? So far we
had no need for hacks line the VT-d side one on AMD. I don't think
this should be widened without there being indication of a problem
with non-niche AMD systems.

> --- 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/setup.h>

Why? And if it's needed here now, can it be dropped from the VT-d
file where you removed the code?

> @@ -132,6 +134,74 @@ void arch_iommu_domain_destroy(struct domain *d)
>  {
>  }
>  
> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> +{
> +    unsigned long i, j, tmp, top, max_pfn;
> +
> +    if ( iommu_passthrough || !is_pv_domain(d) )
> +        return;
> +
> +    BUG_ON(!is_hardware_domain(d));
> +
> +    max_pfn = (GB(4) >> PAGE_SHIFT) - 1;
> +    top = max(max_pdx, pfn_to_pdx(max_pfn) + 1);
> +
> +    for ( i = 0; i < top; i++ )
> +    {
> +        unsigned long pfn = pdx_to_pfn(i);
> +        bool map;
> +        int rc = 0;
> +
> +        /*
> +         * Set up 1:1 mapping for dom0. Default to include only
> +         * conventional RAM areas and let RMRRs include needed reserved
> +         * regions. When set, the inclusive mapping additionally maps in
> +         * every pfn up to 4GB except those that fall in unusable ranges.
> +         */
> +        if ( pfn > max_pfn && !mfn_valid(_mfn(pfn)) )
> +            continue;
> +
> +        if ( iommu_inclusive && pfn <= max_pfn )
> +            map = !page_is_ram_type(pfn, RAM_TYPE_UNUSABLE);
> +        else
> +            map = page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL);
> +
> +        if ( !map )
> +            continue;
> +
> +        /* Exclude Xen bits */
> +        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 &&
> +             page_is_ram_type(pfn, RAM_TYPE_CONVENTIONAL) )
> +            continue;
> +
> +        tmp = 1 << (PAGE_SHIFT - PAGE_SHIFT_4K);
> +        for ( j = 0; j < tmp; j++ )
> +        {
> +            int ret = iommu_map_page(d, pfn * tmp + j, pfn * tmp + j,
> +                                     IOMMUF_readable|IOMMUF_writable);
> +
> +            if ( !rc )
> +               rc = ret;
> +        }

To VT-d specific code was this way to also cope with ia64. I don't
see the need for this to be a loop when the code is now x86-
specific.

> +        if ( rc )
> +            printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n",
> +                   d->domain_id, rc);
> +
> +        if (!(i & (0xfffff >> (PAGE_SHIFT - PAGE_SHIFT_4K))))

Same here for the unnecessary shifting.

> +            process_pending_softirqs();
> +    }
> +
> +
> +}

Double blank line above here when there should be none at all.

Jan



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