Re: [PATCH RFC 1/2] gnttab: add IOMMU entries for grant mappings on auto-translated guests

["Roger Pau Monné" <roger.pau@citrix.com>]

On 04/04/14 17:35, Jan Beulich wrote:
>>>> On 04.04.14 at 16:37, <roger.pau@citrix.com> wrote:
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -749,6 +749,22 @@ __gnttab_map_grant_ref(
>>              goto undo_out;
>>          }
>>      }
>> +    else if ( need_iommu(ld) )
>> +    {
>> +        int err;
>> +
>> +        BUG_ON(!paging_mode_translate(ld));
>> +        err = iommu_map_page(ld, op->host_addr >> PAGE_SHIFT, frame,
>> +                             op->flags & GNTMAP_readonly ?
>> +                                IOMMUF_readable :
>> +                                IOMMUF_readable|IOMMUF_writable);
>> +        if ( err )
>> +        {
>> +            double_gt_unlock(lgt, rgt);
>> +            rc = GNTST_general_error;
>> +            goto undo_out;
>> +        }
>> +    }
> 
> As much of this as possible should be folded with the if() branch.
> And looking at the PV code, I think it makes no sense for the
> conditions whether to map the page r/o or r/w to be different
> between PV and non-PV.
> 
> Plus - wouldn't it be better to have this taken care of via
> create_grant_host_mapping(), by not blindly calling
> iommu_unmap_page() on anything other than p2m_ram_rw in
> ept_set_entry() and p2m_set_entry(), the more that this should
> already be taken care of in the iommu_hap_pt_share case.

Thanks for the comment, it indeed makes much more sense to fix this in 
ept_set_entry/p2m_set_entry, the following patch adds IOMMU support for 
all page types except p2m_mmio_direct (when the hap memory map is not 
shared with IOMMU):

---
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index beb7288..611c8e2 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -475,18 +475,22 @@ out:
             iommu_pte_flush(d, gfn, (u64*)ept_entry, order, vtd_pte_present);
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
             {
+                unsigned int flags;
+
+                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ?
+                        IOMMUF_readable : IOMMUF_readable | IOMMUF_writable;
                 if ( order > 0 )
                 {
                     for ( i = 0; i < (1 << order); i++ )
                         iommu_map_page(
                             p2m->domain, gfn - offset + i, mfn_x(mfn) - offset + i,
-                            IOMMUF_readable | IOMMUF_writable);
+                            flags);
                 }
                 else if ( !order )
                     iommu_map_page(
-                        p2m->domain, gfn, mfn_x(mfn), IOMMUF_readable | IOMMUF_writable);
+                        p2m->domain, gfn, mfn_x(mfn), flags);
             }
             else
             {
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c
index 766fd67..c564cb6 100644
--- a/xen/arch/x86/mm/p2m-pt.c
+++ b/xen/arch/x86/mm/p2m-pt.c
@@ -450,11 +450,15 @@ p2m_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
         }
         else
         {
-            if ( p2mt == p2m_ram_rw )
+            if ( (p2mt != p2m_invalid) && (p2mt != p2m_mmio_direct) )
+            {
+                unsigned int flags;
+
+                flags = ( (p2mt == p2m_grant_map_ro) || (p2mt == p2m_ram_ro) ) ?
+                        IOMMUF_readable : IOMMUF_readable | IOMMUF_writable;
                 for ( i = 0; i < (1UL << page_order); i++ )
-                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i,
-                                   IOMMUF_readable|IOMMUF_writable);
-            else
+                    iommu_map_page(p2m->domain, gfn+i, mfn_x(mfn)+i, flags);
+            } else
                 for ( int i = 0; i < (1UL << page_order); i++ )
                     iommu_unmap_page(p2m->domain, gfn+i);
         }