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

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

On Fri, Nov 04, 2016 at 07:16:08AM -0600, Jan Beulich wrote:
> >>> On 04.11.16 at 14:03, <roger.pau@xxxxxxxxxx> wrote:
> > On Fri, Nov 04, 2016 at 06:53:09AM -0600, Jan Beulich wrote:
> >> >>> On 04.11.16 at 13:25, <roger.pau@xxxxxxxxxx> wrote:
> >> > On Fri, Nov 04, 2016 at 04:34:58AM -0600, Jan Beulich wrote:
> >> >> >>> On 04.11.16 at 10:45, <roger.pau@xxxxxxxxxx> wrote:
> >> >> > 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);
> >> >> 
> >> >> Well, since according to what I've said above this code should
> >> >> really not be here, I think the code structuring question is moot
> >> >> now. The conditional call to iommu_map_page() really just needs
> >> >> adding alongside the p2m_set_entry() call.
> >> > 
> >> > OK, so if the gfn is already mapped into the p2m we don't care whether 
> >> > it 
> >> > has a valid IOMMU mapping or not?
> >> 
> >> We do care, but it is the responsibility of whoever established the
> >> first mapping to make sure it's present in both P2M and IOMMU.
> >> IOW if the GFN is already mapped, we should be able to imply that
> >> it's mapped in both places.
> > 
> > But how is the first caller that established the mapping supposed to know 
> > if 
> > it needs an IOMMU entry or not? (p2m_mmio_direct types don't get an IOMMU 
> > mapping at all)
> 
> And it's that fact stated in parentheses which I'd like to question.
> I don't see what's wrong with e.g. DMAing right into / out of a
> video frame buffer.

Right, so what about the following patch. It would fix my issues, and also 
remove the PVH hack in set_identity_p2m_entry:

---
x86/iommu: add IOMMU entries for p2m_mmio_direct pages

There's nothing wrong with allowing the domain to perform DMA transfers to 
MMIO areas that it already can access from the CPU, and this allows us to 
remove the hack in set_identity_p2m_entry for PVH Dom0.

Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
---
 xen/arch/x86/mm/p2m.c     |    9 ---------
 xen/include/asm-x86/p2m.h |    1 +
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6a45185..7e33ab6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1053,16 +1053,7 @@ int set_identity_p2m_entry(struct domain *d, unsigned 
long gfn,
         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 )
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 7035860..b562da3 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -834,6 +834,7 @@ static inline unsigned int 
p2m_get_iommu_flags(p2m_type_t p2mt)
     case p2m_grant_map_rw:
     case p2m_ram_logdirty:
     case p2m_map_foreign:
+    case p2m_mmio_direct:
         flags =  IOMMUF_readable | IOMMUF_writable;
         break;
     case p2m_ram_ro:


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