[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3.1 08/15] x86/vtd: fix mapping of RMRR regions
On Fri, Nov 04, 2016 at 03:16:47AM -0600, Jan Beulich wrote: > >>> On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |