From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Chen, Tiejun" <tiejun.chen@intel.com>
Subject: Re: [v3][PATCH 03/16] xen/vtd: create RMRR mapping
Date: Fri, 12 Jun 2015 10:43:17 +0800
Message-ID: <557A4745.1030608@intel.com>
References: <1433985325-16676-1-git-send-email-tiejun.chen@intel.com>
	<1433985325-16676-4-git-send-email-tiejun.chen@intel.com>
	<AADFC41AFE54684AB9EE6CBC0274A5D126289FEF@SHSMSX101.ccr.corp.intel.com>
	<5579558C.30709@intel.com>
	<20150611140729.GC1734@deinos.phlegethon.org>
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: <20150611140729.GC1734@deinos.phlegethon.org>
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: Tim Deegan <tim@xen.org>
Cc: "Tian, Kevin" <kevin.tian@intel.com>, "wei.liu2@citrix.com" <wei.liu2@citrix.com>, "ian.campbell@citrix.com" <ian.campbell@citrix.com>, "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>, "Ian.Jackson@eu.citrix.com" <Ian.Jackson@eu.citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, "stefano.stabellini@citrix.com" <stefano.stabellini@citrix.com>, "jbeulich@suse.com" <jbeulich@suse.com>, "Zhang,
	Yang Z" <yang.z.zhang@intel.com>
List-Id: xen-devel@lists.xenproject.org

On 2015/6/11 22:07, Tim Deegan wrote:
> At 17:31 +0800 on 11 Jun (1434043916), Chen, Tiejun wrote:
>>>>        while ( base_pfn < end_pfn )
>>>>        {
>>>> -        int err = intel_iommu_map_page(d, base_pfn, base_pfn,
>>>> -                                       IOMMUF_readable|IOMMUF_writable);
>>>> +        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw);
>>>>
>>>>            if ( err )
>>>>                return err;
>>>
>>> Tim has another comment to replace earlier unmap with
>>
>> Yes, I knew this.
>>
>>> guest_physmap_remove_page() which will call iommu
>>> unmap internally. Please include this change too.
>>>
>>
>> But,
>>
>> guest_physmap_remove_page()
>>       |
>>       + p2m_remove_page()
>> 	|
>> 	+ iommu_unmap_page()
>> 	|
>> 	+ p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), xxx)
>>
>> I think this already remove these pages both on ept/vt-d sides, right?
>
> Yes; this is about this code further up in the same function:
>
>             while ( base_pfn < end_pfn )
>             {
>                 if ( intel_iommu_unmap_page(d, base_pfn) )
>                     ret = -ENXIO;
>                 base_pfn++;
>             }
>
> which ought to be calling guest_physmap_remove_page() or similar, to
> make sure that both iommu and EPT mappings get removed.
>

I still just think current implementation might be fine at this point.

We have two scenarios here, the case of shared ept and the case of 
non-shared ept. But no matter what case we're tracking, shouldn't 
guest_physmap_remove_page() always call p2m->set_entry() to clear *all* 
*valid* mfn which is owned by a given VM? And p2m->set_entry() also 
calls iommu_unmap_page() internally. So nothing special should further 
consider.

If I'm wrong or misunderstanding, please correct me :)

Thanks
Tiejun