Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/7/23 23:42, Jan Beulich wrote:
>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>> intel_iommu_map_page() does nothing if VT-d shares EPT page table.
>> So rmrr_identity_mapping() never create RMRR mapping but in some
>> cases like some GFX drivers it still need to access RMRR.
>>
>> Here we will create those RMRR mappings even in shared EPT case.
>
> Technically this is fine, but there are mechanical issues:
>
>> @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
>>
>>       while ( base_pfn < end_pfn )
>>       {
>> -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>> +        if ( iommu_use_hap_pt(d) ) {
>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>> +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
>> +                    base_pfn, mfn_x(_mfn(base_pfn)));
>
> Do we really need this message, even more so not at guest level?

Its useful to debug as I think, but if you insist on this point, I'm 
fine to remove this as well.

> And if you're convinced we need it, please use %#lx instead of
> 0x%lx.

Okay.

>
>> +            p2m_lock(p2m);
>> +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
>> +                    p2m_mmio_direct, p2m_access_rw) ) {
>> +                p2m_unlock(p2m);
>> +                return -1;
>> +            }
>> +            p2m_unlock(p2m);
>> +        }
>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>                                     IOMMUF_readable|IOMMUF_writable) )
>>               return -1;
>>           base_pfn++;
>
> Apart from the above there are several coding style issues here.

Are you saying this thing?

if ()
{
}

So what about this?


@@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,

      while ( base_pfn < end_pfn )
      {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
+        if ( iommu_use_hap_pt(d) )
+        {
+            dprintk(XENLOG_DEBUG VTDPREFIX,
+                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
+                    base_pfn, mfn_x(_mfn(base_pfn)));
+            p2m_lock(p2m);
+            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), 
PAGE_ORDER_4K,
+                    p2m_mmio_direct, p2m_access_rw) )
+            {
+                p2m_unlock(p2m);
+                return -1;
+            }
+            p2m_unlock(p2m);
+        }
+        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
                                    IOMMUF_readable|IOMMUF_writable) )
              return -1;
          base_pfn++;

Thanks
Tiejun