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

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

On 2014/7/24 15:00, Chen, Tiejun wrote:
> On 2014/7/24 14:14, Jan Beulich wrote:
>>>>> On 24.07.14 at 03:23, <tiejun.chen@intel.com> wrote:
>>> On 2014/7/23 23:42, Jan Beulich wrote:
>>>>>>> On 23.07.14 at 11:35, <tiejun.chen@intel.com> wrote:
>>>>> @@ -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.
>>
>> The main question is how frequently this may get printed vs how
>> useful the message is.
>>
>>>> Apart from the above there are several Indentation issues here.
>>>
>>> Are you saying this thing?
>>>
>>> if ()
>>> {
>>> }
>>
>> Yes, among other things.
>>
>>> So what about this?
>>
>> Almost:
>>
>>> @@ -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) )
>>
>> Don't you, btw, need to extend this condition by
>> && (!iommu_passthrough || !is_hardware_domain(d))?
>
> Why do we need these checks here?
>
> Current problem I met is issued when do GFX passthrough for Windows Guest.
>
>>
>>> +        {
>>> +            dprintk(XENLOG_DEBUG VTDPREFIX,
>>
>> This still (if you absolutely want to retain the message) needs
>
> I will remove this simply since this is not a big deal :)
>
>> changing to XENLOG_G_DEBUG, and you want to include the domain
>> ID in what gets printed for the message to be of any practical use.
>>
>>> +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
>>
>> Additionally please omit the stop at the end. Also, with VTDPREFIX
>> not ending with a space, you want the message to be starting
>> with one.
>>
>>> +                    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) )
>>
>> Indentation.
>>
>>> +            {
>>> +                p2m_unlock(p2m);
>>> +                return -1;
>>> +            }
>>> +            p2m_unlock(p2m);
>>> +        }
>>> +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
>>>                                      IOMMUF_readable|IOMMUF_writable) )
>>
>> Again (here you need you also adjust the second line for indentation
>> to match up again).
>
> I'm not familiar with our xen coding style so I'm wondering if we have
> such a similar .pl like checkpatch.pl.
>

Anyway, what about this?

@@ -1867,8 +1869,20 @@ static int rmrr_identity_mapping(struct domain *d,

      while ( base_pfn < end_pfn )
      {
-        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
-                                  IOMMUF_readable|IOMMUF_writable) )
+        if ( iommu_use_hap_pt(d) && (!iommu_passthrough ||
+                                     !is_hardware_domain(d)) )
+        {
+            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