Re: [PATCH v3.1 08/15] x86/vtd: fix mapping of RMRR regions

[Roger Pau Monne <roger.pau@citrix.com>]

On Fri, Nov 04, 2016 at 03:16:47AM -0600, Jan Beulich wrote:
> >>> On 29.10.16 at 10:59, <roger.pau@citrix.com> wrote:
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1049,22 +1049,29 @@ int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
> >  
> >      mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
> >  
> > -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
> > +    switch ( p2mt )
> > +    {
> > +    case p2m_invalid:
> > +    case 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) )
> > +        if ( ret )
> > +            break;
> > +        /* fallthrough */
> > +    case p2m_mmio_direct:
> > +        if ( p2mt == p2m_mmio_direct && a != p2ma )
> 
> I don't understand the removal of the MFN == GFN check, and it
> also isn't being explained in the commit message.

Maybe I'm not understanding the logic of this function correctly, but it 
seems extremely bogus, and behaves quite differently depending on whether 
gfn == mfn and whether the domain is the hardware domain.

If gfn == mfn (so the page is already mapped in the p2m) and the domain is 
the hardware domain, an IOMMU mapping would be established. If gfn is not 
set, we will just set the p2m entry, but the IOMMU is not going to be 
properly configured, unless it shares the pt with p2m.

This patch fixes the behavior of the function so it's consistent, and we 
can guarantee that after calling it a proper mapping in the p2m and the IOMMU 
will exist, and that it's going to be gfn == mfn, or else an error will be 
returned.

I agree with you that the mfn == gfn check should be kept, so the condition 
above should be:

	if ( p2mt == p2m_mmio_direct && (a != p2ma || gfn != mfn) )

But please see below.

> And then following a case label with a comparison of the respective
> switch expression against the very value from the case label is
> certainly odd. I'm pretty sure a better structure of the code could be
> found.

Hm, the comparison is there because of the fallthrough in the above case. I 
could remove it by also setting the IOMMU entry in the above case, if that's 
better, so it would look like:

case p2m_invalid:
case p2m_mmio_dm:
    ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
                        p2m_mmio_direct, p2ma);
    if ( ret )
        break;
    if ( !iommu_use_hap_pt(d) )
        ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
    break;
case p2m_mmio_direct:
    if ( a != p2ma || gfn != mfn )
    {
        printk(XENLOG_G_WARNING
               "Cannot setup identity map d%d:%lx, already mapped with "
               "different access type or mfn\n", d->domain_id, gfn);
        ret = (flag & XEN_DOMCTL_DEV_RDM_RELAXED) ? 0 : -EBUSY;
        break;
    }
    if ( !iommu_use_hap_pt(d) )
        ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
    break;

Or I could add an if before entering the switch case that checks if type is
p2m_mmio_direct and if the access type and mfn matches what we expect, but I 
think that's not going to make the code easier.

> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "Cannot setup identity map d%d:%lx, already mapped with "
> > +                   "different access type (current: %d, requested: %d).\n",
> 
> Please avoid full stops at the end of log messages.

Done.

Thanks, Roger.

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