Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
> The current code used by Intel VTd will only map RMRR regions for the
> hardware domain, but will fail to map RMRR regions for unprivileged domains
> unless the page tables are shared between EPT and IOMMU.

Okay, if that's the case it surely should get fixed.

> Fix this and
> simplify the code, removing the {set/clear}_identity_p2m_entry helpers and
> just using the normal MMIO mapping functions.

This simplification, however, goes too far. Namely ...

> -int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> -                           p2m_access_t p2ma, unsigned int flag)
> -{
> -    p2m_type_t p2mt;
> -    p2m_access_t a;
> -    mfn_t mfn;
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -    int ret;
> -
> -    if ( !paging_mode_translate(p2m->domain) )
> -    {
> -        if ( !need_iommu(d) )
> -            return 0;
> -        return iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
> -    }
> -
> -    gfn_lock(p2m, gfn, 0);
> -
> -    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
> -
> -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> -        ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
> -                            p2m_mmio_direct, p2ma);
> -    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
> -    {
> -        ret = 0;
> -        /*
> -         * PVH fixme: during Dom0 PVH construction, p2m entries are being set
> -         * but iomem regions are not mapped with IOMMU. This makes sure that
> -         * RMRRs are correctly mapped with IOMMU.
> -         */
> -        if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
> -            ret = iommu_map_page(d, gfn, gfn, 
> IOMMUF_readable|IOMMUF_writable);
> -    }
> -    else
> -    {
> -        if ( flag & XEN_DOMCTL_DEV_RDM_RELAXED )
> -            ret = 0;
> -        else
> -            ret = -EBUSY;
> -        printk(XENLOG_G_WARNING
> -               "Cannot setup identity map d%d:%lx,"
> -               " gfn already mapped to %lx.\n",
> -               d->domain_id, gfn, mfn_x(mfn));

... this logic (and its clear side counterpart) should not be removed
without replacement. Note in this context how you render "flag" an
unused parameter of rmrr_identity_mapping().

> --- a/xen/include/xen/p2m-common.h
> +++ b/xen/include/xen/p2m-common.h
> @@ -2,6 +2,7 @@
>  #define _XEN_P2M_COMMON_H
>  
>  #include <public/vm_event.h>
> +#include <xen/softirq.h>
>  
>  /*
>   * Additional access types, which are used to further restrict
> @@ -46,6 +47,35 @@ int unmap_mmio_regions(struct domain *d,
>                         mfn_t mfn);
>  
>  /*
> + * Preemptive Helper for mapping/unmapping MMIO regions.
> + */

Single line comment.

> +static inline int modify_mmio_11(struct domain *d, unsigned long pfn,
> +                                 unsigned long nr_pages, bool map)

Why do you make this an inline function? And I have to admit that I
dislike this strange use of number 11 - what's wrong with continuing
to use the term "direct map" in one way or another?

> +{
> +    int rc;
> +
> +    while ( nr_pages > 0 )
> +    {
> +        rc = (map ? map_mmio_regions : unmap_mmio_regions)
> +             (d, _gfn(pfn), nr_pages, _mfn(pfn));
> +        if ( rc == 0 )
> +            break;
> +        if ( rc < 0 )
> +        {
> +            printk(XENLOG_ERR
> +                "Failed to %smap %#lx - %#lx into domain %d memory map: 
> %d\n",

"Failed to identity %smap [%#lx,%#lx) for d%d: %d\n"

And I think XENLOG_WARNING would do - whether this actually is
a problem depends on further factors.

> +                   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, rc);
> +            return rc;
> +        }
> +        nr_pages -= rc;
> +        pfn += rc;
> +        process_pending_softirqs();

Is this what you call "preemptive"? 

> +    }
> +
> +    return rc;

The way this is coded it appears to possibly return non-zero even in
success case. I think this would therefore better be a for ( ; ; ) loop.

Jan

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