From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [PATCH 1/1] xen:vtd: missing RMRR mapping while
 share EPT
Date: Thu, 24 Jul 2014 09:23:29 +0800
Message-ID: <53D06011.5080106@intel.com>
References: <1406108103-11981-1-git-send-email-tiejun.chen@intel.com>
	<53CFF40A020000780002539F@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <53CFF40A020000780002539F@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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