[PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2600 bytes --]

Tearing down a 1:1 mapping that was never established isn't really nice
(and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
wrapper macro to a proper function which then can take care of the
situation.

Also take the opportunity to remove the 'page_order' parameter of
clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1019,6 +1019,38 @@ int clear_mmio_p2m_entry(struct domain *
     return rc;
 }
 
+int clear_identity_p2m_entry(struct domain *d, unsigned long gfn)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret;
+
+    if ( !paging_mode_translate(d) )
+        return 0;
+
+    gfn_lock(p2m, gfn, 0);
+
+    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
+    if ( p2mt == p2m_mmio_direct && mfn_x(mfn) == gfn )
+    {
+        ret = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K,
+                            p2m_invalid, p2m->default_access);
+        gfn_unlock(p2m, gfn, 0);
+    }
+    else
+    {
+        gfn_unlock(p2m, gfn, 0);
+        printk(XENLOG_G_WARNING
+               "non-identity map d%d:%lx not cleared (mapped to %lx)\n",
+               d->domain_id, gfn, mfn_x(mfn));
+        ret = 0;
+    }
+
+    return ret;
+}
+
 /* Returns: 0 for success, -errno for failure */
 int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
 {
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct 
 
             while ( base_pfn < end_pfn )
             {
-                if ( clear_identity_p2m_entry(d, base_pfn, 0) )
+                if ( clear_identity_p2m_entry(d, base_pfn) )
                     ret = -ENXIO;
                 base_pfn++;
             }
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -563,9 +563,7 @@ int clear_mmio_p2m_entry(struct domain *
 /* Set identity addresses in the p2m table (for pass-through) */
 int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
                            p2m_access_t p2ma, unsigned int flag);
-
-#define clear_identity_p2m_entry(d, gfn, page_order) \
-                        guest_physmap_remove_page(d, gfn, gfn, page_order)
+int clear_identity_p2m_entry(struct domain *d, unsigned long gfn);
 
 /* Add foreign mapping to the guest's p2m table. */
 int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,



[-- Attachment #2: x86-p2m-ident-unmap-conditional.patch --]
[-- Type: text/plain, Size: 2669 bytes --]

x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

Tearing down a 1:1 mapping that was never established isn't really nice
(and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
wrapper macro to a proper function which then can take care of the
situation.

Also take the opportunity to remove the 'page_order' parameter of
clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().

--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1019,6 +1019,38 @@ int clear_mmio_p2m_entry(struct domain *
     return rc;
 }
 
+int clear_identity_p2m_entry(struct domain *d, unsigned long gfn)
+{
+    p2m_type_t p2mt;
+    p2m_access_t a;
+    mfn_t mfn;
+    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    int ret;
+
+    if ( !paging_mode_translate(d) )
+        return 0;
+
+    gfn_lock(p2m, gfn, 0);
+
+    mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
+    if ( p2mt == p2m_mmio_direct && mfn_x(mfn) == gfn )
+    {
+        ret = p2m_set_entry(p2m, gfn, _mfn(INVALID_MFN), PAGE_ORDER_4K,
+                            p2m_invalid, p2m->default_access);
+        gfn_unlock(p2m, gfn, 0);
+    }
+    else
+    {
+        gfn_unlock(p2m, gfn, 0);
+        printk(XENLOG_G_WARNING
+               "non-identity map d%d:%lx not cleared (mapped to %lx)\n",
+               d->domain_id, gfn, mfn_x(mfn));
+        ret = 0;
+    }
+
+    return ret;
+}
+
 /* Returns: 0 for success, -errno for failure */
 int set_shared_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
 {
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1839,7 +1839,7 @@ static int rmrr_identity_mapping(struct 
 
             while ( base_pfn < end_pfn )
             {
-                if ( clear_identity_p2m_entry(d, base_pfn, 0) )
+                if ( clear_identity_p2m_entry(d, base_pfn) )
                     ret = -ENXIO;
                 base_pfn++;
             }
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -563,9 +563,7 @@ int clear_mmio_p2m_entry(struct domain *
 /* Set identity addresses in the p2m table (for pass-through) */
 int set_identity_p2m_entry(struct domain *d, unsigned long gfn,
                            p2m_access_t p2ma, unsigned int flag);
-
-#define clear_identity_p2m_entry(d, gfn, page_order) \
-                        guest_physmap_remove_page(d, gfn, gfn, page_order)
+int clear_identity_p2m_entry(struct domain *d, unsigned long gfn);
 
 /* Add foreign mapping to the guest's p2m table. */
 int p2m_add_foreign(struct domain *tdom, unsigned long fgfn,

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[Andrew Cooper]

On 12/08/15 16:05, Jan Beulich wrote:
> Tearing down a 1:1 mapping that was never established isn't really nice
> (and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
> wrapper macro to a proper function which then can take care of the
> situation.
>
> Also take the opportunity to remove the 'page_order' parameter of
> clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().

What about identity mapping with 2MB frames?  IMO, the RMRR code is
incorrect in assuming that only 4K frames will be wanted, but that is a
separate issue.

As this at least matches the set_identity side of things, and improves
the situation, so

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

(I presume you intended to include a SoB tag?)

~Andrew

Re: [PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[Jan Beulich]

>>> On 12.08.15 at 18:06, <andrew.cooper3@citrix.com> wrote:
> On 12/08/15 16:05, Jan Beulich wrote:
>> Tearing down a 1:1 mapping that was never established isn't really nice
>> (and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
>> wrapper macro to a proper function which then can take care of the
>> situation.
>>
>> Also take the opportunity to remove the 'page_order' parameter of
>> clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().
> 
> What about identity mapping with 2MB frames?  IMO, the RMRR code is
> incorrect in assuming that only 4K frames will be wanted, but that is a
> separate issue.

Why would it be incorrect? It would be an optimization to use larger
pages for larger regions, but I don't see it being wrong.

> As this at least matches the set_identity side of things, and improves
> the situation, so
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> (I presume you intended to include a SoB tag?)

Oops, yes, of course.

Jan

Re: [PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[Wei Liu]

On Wed, Aug 12, 2015 at 09:05:43AM -0600, Jan Beulich wrote:
> Tearing down a 1:1 mapping that was never established isn't really nice
> (and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
> wrapper macro to a proper function which then can take care of the
> situation.
> 
> Also take the opportunity to remove the 'page_order' parameter of
> clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().
> 

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH] x86/p2m: clear_identity_p2m_entry() must cope with 'relaxed' RDM mode

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, August 12, 2015 11:06 PM
> 
> Tearing down a 1:1 mapping that was never established isn't really nice
> (and in fact hits an ASSERT() in p2m_remove_page()). Convert from a
> wrapper macro to a proper function which then can take care of the
> situation.
> 
> Also take the opportunity to remove the 'page_order' parameter of
> clear_identity_p2m_entry(), to make it match set_identity_p2m_entry().

Acked-by: Kevin Tian <kevin.tian@intel.com>