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

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

On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
> >>> 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().

OK, so I'm just going to fix {set/clear}_identity_p2m_entry, because leaving 
the current logic while using something like modify_mmio_11 (or whatever we 
end up calling it) it's too complex IMHO.

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

I've renamed it to modify_mmio_direct and moved it to common/memory.c, since 
none of the files in passthrough/ seemed like a good place to put it.

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

Done.

> > +                   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"? 

Right, I've removed preemptive from the comment since it makes no sense.

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

I don't think this is possible, {un}map_mmio_regions will return < 0 on 
error, > 0 if there are pending pages to map, and 0 when all the requested 
pages have been mapped successfully.

Thanks, Roger. 

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