Xen devel - Jul 2010 - [PATCH] Unmmap guest''s EPT mapping for poison memory

Unmmap guest''s EPT mapping for poison memory

Unmmap poisone memory that is assigned to guest, so that guest can''t
access the memory anymore. Currently the unmmap is only done for EPT guest,
through remove the EPT entry. Support for PV guest should be added later. No
unmmap support for shadow guest because system support recoverable MCA should
also support EPT.

If we can''t unmmap, we will destroy the guest.

signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com>

diff -r bf51b671f269 xen/arch/x86/cpu/mcheck/mce.h
--- a/xen/arch/x86/cpu/mcheck/mce.h	Mon Jul 12 13:59:39 2010 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce.h	Mon Jul 12 14:23:05 2010 +0800
@@ -49,6 +49,7 @@ void amd_nonfatal_mcheck_init(struct cpu
 void amd_nonfatal_mcheck_init(struct cpuinfo_x86 *c);
 
 int is_vmce_ready(struct mcinfo_bank *bank, struct domain *d);
+int unmmap_broken_page(struct domain *d, unsigned long mfn, unsigned long gfn);
 
 u64 mce_cap_init(void);
 extern int firstbank;
diff -r bf51b671f269 xen/arch/x86/cpu/mcheck/mce_intel.c
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Mon Jul 12 13:59:39 2010 +0800
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Mon Jul 12 17:25:47 2010 +0800
@@ -654,16 +654,22 @@ static void intel_memerr_dhandler(int bn
             BUG_ON( result->owner == DOMID_COW );
             if ( result->owner != DOMID_XEN ) {
                 d = get_domain_by_id(result->owner);
+                ASSERT(d);
+                gfn = get_gpfn_from_mfn((bank->mc_addr) >>
PAGE_SHIFT);
+
                 if ( !is_vmce_ready(bank, d) )
                 {
-                    /* Should not inject vMCE to guest */
-                    if ( d )
-                        put_domain(d);
-                    return;
+                    printk("DOM%d not ready for vMCE\n",
d->domain_id);
+                    goto vmce_failed;
                 }
 
-                ASSERT(d);
-                gfn = get_gpfn_from_mfn((bank->mc_addr) >>
PAGE_SHIFT);
+                if ( unmmap_broken_page(d, mfn, gfn) )
+                {
+                    printk("Unmap broken memory %lx for DOM%d
failed\n",
+                            mfn, d->domain_id);
+                    goto vmce_failed;
+                }
+
                 bank->mc_addr =  gfn << PAGE_SHIFT |
                   (bank->mc_addr & (PAGE_SIZE -1 ));
                 if ( fill_vmsr_data(bank, d,
@@ -671,18 +677,15 @@ static void intel_memerr_dhandler(int bn
                 {
                     mce_printk(MCE_QUIET, "Fill vMCE# data for DOM%d
"
                       "failed\n", result->owner);
-                    put_domain(d);
-                    domain_crash(d);
-                    return;
+                    goto vmce_failed;
                 }
+
                 /* We will inject vMCE to DOMU*/
                 if ( inject_vmce(d) < 0 )
                 {
                     mce_printk(MCE_QUIET, "inject vMCE to DOM%d"
                       " failed\n", d->domain_id);
-                    put_domain(d);
-                    domain_crash(d);
-                    return;
+                    goto vmce_failed;
                 }
                 /* Impacted domain go on with domain''s recovery job
                  * if the domain has its own MCA handler.
@@ -691,6 +694,11 @@ static void intel_memerr_dhandler(int bn
                  */
                 result->result = MCA_RECOVERED;
                 put_domain(d);
+
+                return;
+vmce_failed:
+                put_domain(d);
+                domain_crash(d);
             }
         }
     }
diff -r bf51b671f269 xen/arch/x86/cpu/mcheck/vmce.c
--- a/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 13:59:39 2010 +0800
+++ b/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 14:30:21 2010 +0800
@@ -558,3 +558,28 @@ int is_vmce_ready(struct mcinfo_bank *ba
 
     return 0;
 }
+
+/* Now we only have support for HAP guest */
+int unmmap_broken_page(struct domain *d, unsigned long mfn, unsigned long gfn)
+{
+    /* Always trust dom0 */
+    if ( d == dom0 )
+        return 0;
+
+    if (is_hvm_domain(d) && (paging_mode_hap(d)) )
+    {
+        p2m_type_t pt;
+
+        gfn_to_mfn_query(d, gfn, &pt);
+        /* What will happen if is paging-in? */
+        if ( pt == p2m_ram_rw )
+        {
+            p2m_change_type(d, gfn, pt, p2m_ram_broken);
+            return 0;
+        }
+
+    }
+
+    return -1;
+}
+




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

Hi, 

At 08:41 +0100 on 14 Jul (1279096872), Jiang, Yunhong
wrote:
> diff -r bf51b671f269 xen/arch/x86/cpu/mcheck/vmce.c
> --- a/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 13:59:39 2010 +0800
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 14:30:21 2010 +0800
> @@ -558,3 +558,28 @@ int is_vmce_ready(struct mcinfo_bank *ba
>  
>      return 0;
>  }
> +
> +/* Now we only have support for HAP guest */
> +int unmmap_broken_page(struct domain *d, unsigned long mfn, unsigned long
gfn)
> +{
> +    /* Always trust dom0 */
> +    if ( d == dom0 )
> +        return 0;
> +
> +    if (is_hvm_domain(d) && (paging_mode_hap(d)) )
> +    {
> +        p2m_type_t pt;
> +
> +        gfn_to_mfn_query(d, gfn, &pt);
> +        /* What will happen if is paging-in? */
> +        if ( pt == p2m_ram_rw )
Or any of the other types?  This should be called for ram_ro, and
ram_logdirty certainly, and probably mmio_direct too.

I''m not sure that it''s safe to nobble other types - e.g.
changing from
grant_map_*, paging_* or ram_shared might break state-machines/refcounts
elsewhere.

Actually wouldn''t it be be better to encode brokenness in the
frametable
instead of the P2M and then forbid new mappings of broken MFNs?  It''s
not really a property of the PFN (wasn''t there a patch series a while
ago that swapped broken MFNs under a VM''s feet?).

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Wednesday, July 14, 2010 5:28 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel
>Subject: Re: [PATCH] Unmmap guest''s EPT mapping for poison memory
>
>Hi,
>
>At 08:41 +0100 on 14 Jul (1279096872), Jiang, Yunhong wrote:
>> diff -r bf51b671f269 xen/arch/x86/cpu/mcheck/vmce.c
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 13:59:39 2010 +0800
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c	Mon Jul 12 14:30:21 2010 +0800
>> @@ -558,3 +558,28 @@ int is_vmce_ready(struct mcinfo_bank *ba
>>
>>      return 0;
>>  }
>> +
>> +/* Now we only have support for HAP guest */
>> +int unmmap_broken_page(struct domain *d, unsigned long mfn, unsigned
long
>gfn)
>> +{
>> +    /* Always trust dom0 */
>> +    if ( d == dom0 )
>> +        return 0;
>> +
>> +    if (is_hvm_domain(d) && (paging_mode_hap(d)) )
>> +    {
>> +        p2m_type_t pt;
>> +
>> +        gfn_to_mfn_query(d, gfn, &pt);
>> +        /* What will happen if is paging-in? */
>> +        if ( pt == p2m_ram_rw )
>
>Or any of the other types?  This should be called for ram_ro, and
>ram_logdirty certainly, and probably mmio_direct too.
Yes, we need consider rw/ro/logdirty. Thanks for remind and will fix it. But why
should we cover mmio_direct? Can you please give some hints?

For ram_shared, it deserve more consideration, seems currently the shared memory
situation is not handled in the whole offline page flow.
>
>I''m not sure that it''s safe to nobble other types - e.g.
changing from
>grant_map_*, paging_* or ram_shared might break state-machines/refcounts
>elsewhere.
I think this code does not change anything for the refcounts, we simply destroy
the guest.
Or you mean race happens when other components is changing the p2m table also? I
assume that should be ok since we only query the type and destroy the guest.
Did I missed anything?
>
>Actually wouldn''t it be be better to encode brokenness in the
frametable
Encode brokeness in frametable is done already. But that is only a mark, and
that page will not be allocated anymore. If the page is being used by guest, we
need unmap for the guest, so that guest can''t access the memory
anymore.

The background here is: In some platform, system can find poison memory through
like memory scrubbing or L3 cache explicit write back (i.e. async memory
checking, not in current context). However, whenenever the poison memory is
accessed, it will cause fatal MCE and system crash. So we need make sure the
guest can''t access the broken memory.
>instead of the P2M and then forbid new mappings of broken MFNs? 
It''s
>not really a property of the PFN (wasn''t there a patch series a
while
>ago that swapped broken MFNs under a VM''s feet?).
The swap broken mfn is in fact when the page is likely to be broken (i.e. the
page can still be accessed). For example, for page with ECC support, when too
many corrected error (i.e. 1 bit error) happens to a page, we assume the page is
fragile, and may have un-correctable error ( two bit error ) in future, and swap
it with a new page wil keep thing continue. However, if the page is broken
already, we can''t access the page anymore (this usually causes MCE), in
such situation, we can''t swap the page, but unmap it.
Hope this make thing clear.

Thanks
--jyh
>
>Cheers,
>
>Tim.
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, XenServer Engineering
>Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi, 

At 14:56 +0100 on 14 Jul (1279119388), Jiang, Yunhong
wrote:
> >Or any of the other types?  This should be called for ram_ro, and
> >ram_logdirty certainly, and probably mmio_direct too.
> 
> Yes, we need consider rw/ro/logdirty. Thanks for remind and will fix
> it. But why should we cover mmio_direct? Can you please give some
> hints?
I''ve seen cases where people use mmio_direct to point at actual memory,
in order to allow uncached mappings.
> For ram_shared, it deserve more consideration, seems currently the
> shared memory situation is not handled in the whole offline page flow.
Or vice versa.  I''m happy to ack an initial patch that deals with the
easy cases, though.
> >I''m not sure that it''s safe to nobble other types -
e.g. changing from
> >grant_map_*, paging_* or ram_shared might break
state-machines/refcounts
> >elsewhere.
> 
> I think this code does not change anything for the refcounts, we simply
destroy the guest.
> Or you mean race happens when other components is changing the p2m table
also? I assume that should be ok since we only query the type and destroy the
guest.
> Did I missed anything?
No, I was just suggesting that if you do handle other p2m types here
it might not be safe to change a page from shared, grant-map &c to
broken because it would cause bugs in the sharing/granting code.
> The background here is: In some platform, system can find poison
> memory through like memory scrubbing or L3 cache explicit write back
> (i.e. async memory checking, not in current context). However,
> whenenever the poison memory is accessed, it will cause fatal MCE and
> system crash. So we need make sure the guest can''t access the
broken
> memory.
OK - you''re protecting against a _host_ crash here?  Now I understand,
thanks.

In that case I definitely suggest that you move the domain_crash()
into the p2m lookup functions - all p2m lookups of a broken page should 
return type=broken, and non-"query" p2m lookups should call
domain_crash() too.   That will catch all the MMIO-emulator and shadow
paths for free.

If you also signal qemu-dm to blow its mapcache that will catch DMA too
(since it won''t be able to re-map the broken page) though it''s
unfortunate to have to rely on good behaviour from qemu-dm for safety. 
Presumably the PV patches will solve that in a better way. 

Cheers,

Tim.

P.S. Another thing I forgot - please wrap the code that sets the type to
     broken in a "#ifdef __x86_64__"; it won''t work on
32-bit.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Thursday, July 15, 2010 11:13 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel
>Subject: Re: [PATCH] Unmmap guest''s EPT mapping for poison memory
>
>Hi,
>
>At 14:56 +0100 on 14 Jul (1279119388), Jiang, Yunhong wrote:
>> >Or any of the other types?  This should be called for ram_ro, and
>> >ram_logdirty certainly, and probably mmio_direct too.
>>
>> Yes, we need consider rw/ro/logdirty. Thanks for remind and will fix
>> it. But why should we cover mmio_direct? Can you please give some
>> hints?
>
>I''ve seen cases where people use mmio_direct to point at actual
memory,
>in order to allow uncached mappings.
Thanks for notify this.
>
>> For ram_shared, it deserve more consideration, seems currently the
>> shared memory situation is not handled in the whole offline page flow.
>
>Or vice versa.  I''m happy to ack an initial patch that deals with
the
>easy cases, though.
>
>> >I''m not sure that it''s safe to nobble other types
- e.g. changing from
>> >grant_map_*, paging_* or ram_shared might break
state-machines/refcounts
>> >elsewhere.
>>
>> I think this code does not change anything for the refcounts, we simply
destroy the
>guest.
>> Or you mean race happens when other components is changing the p2m
table also?
>I assume that should be ok since we only query the type and destroy the
guest.
>> Did I missed anything?
>
>No, I was just suggesting that if you do handle other p2m types here
>it might not be safe to change a page from shared, grant-map &c to
>broken because it would cause bugs in the sharing/granting code.
Yes, agree. Maybe I need have a look on grant-map in future for PV guest.
>
>> The background here is: In some platform, system can find poison
>> memory through like memory scrubbing or L3 cache explicit write back
>> (i.e. async memory checking, not in current context). However,
>> whenenever the poison memory is accessed, it will cause fatal MCE and
>> system crash. So we need make sure the guest can''t access the
broken
>> memory.
>
>OK - you''re protecting against a _host_ crash here?  Now I
understand,
>thanks.
>
>In that case I definitely suggest that you move the domain_crash()
>into the p2m lookup functions - all p2m lookups of a broken page should
>return type=broken, and non-"query" p2m lookups should call
>domain_crash() too.   That will catch all the MMIO-emulator and shadow
>paths for free.
Ok, I will try this way.
>
>If you also signal qemu-dm to blow its mapcache that will catch DMA too
>(since it won''t be able to re-map the broken page) though
it''s
>unfortunate to have to rely on good behaviour from qemu-dm for safety.
>Presumably the PV patches will solve that in a better way.
Yes, I will defer it to PV.
>
>Cheers,
>
>Tim.
>
>P.S. Another thing I forgot - please wrap the code that sets the type to
>     broken in a "#ifdef __x86_64__"; it won''t work on
32-bit.
I''m not sure. At least it should work for EPT on 32-bit, since EPT
table can support >8 p2m type per my understanding.

Thanks
--jyh
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, XenServer Engineering
>Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

At 07:45 +0100 on 16 Jul (1279266321), Jiang, Yunhong
wrote:
> I''m not sure. At least it should work for EPT on 32-bit, since EPT
> table can support >8 p2m type per my understanding.
Yes, that''s OK, at least while this is EPT-only.

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Hi, Tim, sorry for the long delay for the updated patch.  Can you please have a
review for it? I will update according to your feedback, and re-send as a
seperated patch submission.

I''m not sure if the change to the _gfn_to_mfn_type() is acceptable.
Especially, if it is right to change the mfn to be INVALID_MFN. The idea to
change the mfn to be INVALID_MFN is, we don''t need to change every
caller for broken type (I noticed the PoD/sharing etc checked each caller), but
depends on caller will check the mfn to be valid or not.  But that may have some
potential issue: Some caller may assume the mfn should not be INVALID_MFN (since
hypervisor setup the p2m table) and BUG() on INVALID_MFN. I try to check the
code and didn''t find such situation. Do you think if it is reasonable?

I''m now working on support for PoD/Shared memory.

Thanks
--jyh

>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Friday, July 16, 2010 4:52 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel
>Subject: Re: [PATCH] Unmmap guest''s EPT mapping for poison memory
>
>At 07:45 +0100 on 16 Jul (1279266321), Jiang, Yunhong wrote:
>> I''m not sure. At least it should work for EPT on 32-bit, since
EPT
>> table can support >8 p2m type per my understanding.
>
>Yes, that''s OK, at least while this is EPT-only.
>
>Cheers,
>
>Tim.
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, XenServer Engineering
>Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)


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

At 10:19 +0100 on 13 Sep (1284373193), Jiang, Yunhong
wrote:
> I''m not sure if the change to the _gfn_to_mfn_type() is
> acceptable. Especially, if it is right to change the mfn to be
> INVALID_MFN. The idea to change the mfn to be INVALID_MFN is, we
don''t
> need to change every caller for broken type (I noticed the PoD/sharing
> etc checked each caller), but depends on caller will check the mfn to
> be valid or not.
That''s fine.  All callers _should_ check the type but there''s
no harm in
changing the mfn as a double precaution, even if everything else was right.

Acked-by: Tim Deegan <Tim.Deegan@citrix.com>

-- 
Tim Deegan <Tim.Deegan@citrix.com>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

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

Thanks for review. I will re-send it as patch submission.

Thanks
--jyh
>-----Original Message-----
>From: Tim Deegan [mailto:Tim.Deegan@citrix.com]
>Sent: Monday, September 13, 2010 9:38 PM
>To: Jiang, Yunhong
>Cc: Keir Fraser; xen-devel
>Subject: Re: [PATCH] Unmmap guest''s EPT mapping for poison memory
>
>At 10:19 +0100 on 13 Sep (1284373193), Jiang, Yunhong wrote:
>> I''m not sure if the change to the _gfn_to_mfn_type() is
>> acceptable. Especially, if it is right to change the mfn to be
>> INVALID_MFN. The idea to change the mfn to be INVALID_MFN is, we
don''t
>> need to change every caller for broken type (I noticed the PoD/sharing
>> etc checked each caller), but depends on caller will check the mfn to
>> be valid or not.
>
>That''s fine.  All callers _should_ check the type but
there''s no harm in
>changing the mfn as a double precaution, even if everything else was right.
>
>Acked-by: Tim Deegan <Tim.Deegan@citrix.com>
>
>--
>Tim Deegan <Tim.Deegan@citrix.com>
>Principal Software Engineer, XenServer Engineering
>Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel