Jiang, Yunhong
2010-Jul-14 07:39 UTC
[Xen-devel] [PATCH] Add a new p2m type for broken memory
Add a new p2m type for broken memory. Currently, this is used only for EPT guest. When memory assigned to guest is poisoned, we will mark it as broken in p2m table, the corresponding EPT entry is set as not present. Later, if guest try to access the affected memory, EPT violation will happen and Xen hypervisor can trap this access. Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> diff -r 29f0479830cd xen/arch/x86/hvm/hvm.c --- a/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:12:31 2010 +0800 +++ b/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:58:55 2010 +0800 @@ -971,6 +971,12 @@ bool_t hvm_hap_nested_page_fault(unsigne mfn = gfn_to_mfn_type_current(gfn, &p2mt, p2m_guest); + if ( unlikely(p2mt == p2m_ram_broken) ) + { + domain_crash(current->domain); + return 1; + } + /* * If this GFN is emulated MMIO or marked as read-only, pass the fault * to the mmio handler. diff -r 29f0479830cd xen/include/asm-x86/p2m.h --- a/xen/include/asm-x86/p2m.h Mon Jul 12 13:12:31 2010 +0800 +++ b/xen/include/asm-x86/p2m.h Mon Jul 12 13:56:02 2010 +0800 @@ -85,6 +85,7 @@ typedef enum { p2m_ram_paging_in = 11, /* Memory that is being paged in */ p2m_ram_paging_in_start = 12, /* Memory that is being paged in */ p2m_ram_shared = 13, /* Shared or sharable memory */ + p2m_ram_broken = 14, /* broken page, access cause domain crash*/ } p2m_type_t; typedef enum { @@ -132,6 +133,8 @@ typedef enum { | p2m_to_mask(p2m_ram_paging_in)) #define P2M_PAGED_TYPES (p2m_to_mask(p2m_ram_paged)) + +#define P2M_BROKEN_TYPES (p2m_to_mask(p2m_ram_broken)) /* Shared types */ /* XXX: Sharable types could include p2m_ram_ro too, but we would need to @@ -155,6 +158,7 @@ typedef enum { #define p2m_is_paged(_t) (p2m_to_mask(_t) & P2M_PAGED_TYPES) #define p2m_is_sharable(_t) (p2m_to_mask(_t) & P2M_SHARABLE_TYPES) #define p2m_is_shared(_t) (p2m_to_mask(_t) & P2M_SHARED_TYPES) +#define p2m_is_broken(_t) (p2m_to_mask(_t) & P2M_BROKEN_TYPES) /* Populate-on-demand */ diff -r 29f0479830cd xen/include/asm-x86/page.h --- a/xen/include/asm-x86/page.h Mon Jul 12 13:12:31 2010 +0800 +++ b/xen/include/asm-x86/page.h Mon Jul 12 13:56:21 2010 +0800 @@ -323,6 +323,7 @@ void setup_idle_pagetable(void); #define _PAGE_PSE_PAT 0x1000U #define _PAGE_PAGED 0x2000U #define _PAGE_SHARED 0x4000U +#define _PAGE_BROKEN 0x8000U /* * Debug option: Ensure that granted mappings are not implicitly unmapped. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jul-14 07:54 UTC
[Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
On 14/07/2010 08:39, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:> diff -r 29f0479830cd xen/include/asm-x86/page.h > --- a/xen/include/asm-x86/page.h Mon Jul 12 13:12:31 2010 +0800 > +++ b/xen/include/asm-x86/page.h Mon Jul 12 13:56:21 2010 +0800 > @@ -323,6 +323,7 @@ void setup_idle_pagetable(void); > #define _PAGE_PSE_PAT 0x1000U > #define _PAGE_PAGED 0x2000U > #define _PAGE_SHARED 0x4000U > +#define _PAGE_BROKEN 0x8000UI don''t see this used anywhere. Also, not that you started it, but it doesn''t seem nice to add extra _PAGE_* definitions in page.h for flags which never (afaict) actually appear in any pte. At least the extra return-code-only flags should be separated off and commented. Having the macros not start with _PAGE_ might be a good idea too. Of course all this is ultimately to be acked/nacked by Tim. -- Keir> /* > * Debug option: Ensure that granted mappings are not implicitly unmapped. > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Jul-14 08:10 UTC
[Xen-devel] RE: [PATCH] Add a new p2m type for broken memory
>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Wednesday, July 14, 2010 3:54 PM >To: Jiang, Yunhong; Tim Deegan >Cc: xen-devel >Subject: Re: [PATCH] Add a new p2m type for broken memory > >On 14/07/2010 08:39, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > > >> diff -r 29f0479830cd xen/include/asm-x86/page.h >> --- a/xen/include/asm-x86/page.h Mon Jul 12 13:12:31 2010 +0800 >> +++ b/xen/include/asm-x86/page.h Mon Jul 12 13:56:21 2010 +0800 >> @@ -323,6 +323,7 @@ void setup_idle_pagetable(void); >> #define _PAGE_PSE_PAT 0x1000U >> #define _PAGE_PAGED 0x2000U >> #define _PAGE_SHARED 0x4000U >> +#define _PAGE_BROKEN 0x8000U > >I don''t see this used anywhere. Also, not that you started it, but itYes, this is not used now, although it will be used in future when we add the PV support. I will remove it and resend the patch after get Tim''s feedback for the patch.>doesn''t seem nice to add extra _PAGE_* definitions in page.h for flags which >never (afaict) actually appear in any pte. At least the extraYes, this confused me a bit at first glance.>return-code-only flags should be separated off and commented. Having the >macros not start with _PAGE_ might be a good idea too. Of course all this is >ultimately to be acked/nacked by Tim. > > -- Keir > >> /* >> * Debug option: Ensure that granted mappings are not implicitly unmapped. >> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Jul-14 09:18 UTC
[Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
Hi, At 08:39 +0100 on 14 Jul (1279096791), Jiang, Yunhong wrote:> Add a new p2m type for broken memory. > > Currently, this is used only for EPT guest. When memory assigned to guest is poisoned, we will mark it as broken in p2m table, the corresponding EPT entry is set as not present. Later, if guest try to access the affected memory, EPT violation will happen and Xen hypervisor can trap this access. > > Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> > > diff -r 29f0479830cd xen/arch/x86/hvm/hvm.c > --- a/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:12:31 2010 +0800 > +++ b/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:58:55 2010 +0800 > @@ -971,6 +971,12 @@ bool_t hvm_hap_nested_page_fault(unsigne > > mfn = gfn_to_mfn_type_current(gfn, &p2mt, p2m_guest); > > + if ( unlikely(p2mt == p2m_ram_broken) ) > + { > + domain_crash(current->domain); > + return 1; > + } > +You should probably do this in more places, even if you don''t care about shadow pagetables -- MMIO emulation should behave the same as normal accesses. What behaviour would you like when qemu tries to DMA to a broken page? Or when a backend driver grant-copies to it? Is there a case for just having the P2M lookups (at least the _query() kind) call domain_crash when they hit a poisoned page? Cheers, Tim.> /* > * If this GFN is emulated MMIO or marked as read-only, pass the fault > * to the mmio handler. > diff -r 29f0479830cd xen/include/asm-x86/p2m.h > --- a/xen/include/asm-x86/p2m.h Mon Jul 12 13:12:31 2010 +0800 > +++ b/xen/include/asm-x86/p2m.h Mon Jul 12 13:56:02 2010 +0800 > @@ -85,6 +85,7 @@ typedef enum { > p2m_ram_paging_in = 11, /* Memory that is being paged in */ > p2m_ram_paging_in_start = 12, /* Memory that is being paged in */ > p2m_ram_shared = 13, /* Shared or sharable memory */ > + p2m_ram_broken = 14, /* broken page, access cause domain crash*/ > } p2m_type_t; > > typedef enum { > @@ -132,6 +133,8 @@ typedef enum { > | p2m_to_mask(p2m_ram_paging_in)) > > #define P2M_PAGED_TYPES (p2m_to_mask(p2m_ram_paged)) > + > +#define P2M_BROKEN_TYPES (p2m_to_mask(p2m_ram_broken)) > > /* Shared types */ > /* XXX: Sharable types could include p2m_ram_ro too, but we would need to > @@ -155,6 +158,7 @@ typedef enum { > #define p2m_is_paged(_t) (p2m_to_mask(_t) & P2M_PAGED_TYPES) > #define p2m_is_sharable(_t) (p2m_to_mask(_t) & P2M_SHARABLE_TYPES) > #define p2m_is_shared(_t) (p2m_to_mask(_t) & P2M_SHARED_TYPES) > +#define p2m_is_broken(_t) (p2m_to_mask(_t) & P2M_BROKEN_TYPES) > > > /* Populate-on-demand */ > diff -r 29f0479830cd xen/include/asm-x86/page.h > --- a/xen/include/asm-x86/page.h Mon Jul 12 13:12:31 2010 +0800 > +++ b/xen/include/asm-x86/page.h Mon Jul 12 13:56:21 2010 +0800 > @@ -323,6 +323,7 @@ void setup_idle_pagetable(void); > #define _PAGE_PSE_PAT 0x1000U > #define _PAGE_PAGED 0x2000U > #define _PAGE_SHARED 0x4000U > +#define _PAGE_BROKEN 0x8000U > > /* > * Debug option: Ensure that granted mappings are not implicitly unmapped. > >-- 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
Jiang, Yunhong
2010-Jul-14 13:41 UTC
RE: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
>-----Original Message----- >From: xen-devel-bounces@lists.xensource.com >[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Tim Deegan >Sent: Wednesday, July 14, 2010 5:19 PM >To: Jiang, Yunhong >Cc: xen-devel; Keir Fraser >Subject: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory > >Hi, > >At 08:39 +0100 on 14 Jul (1279096791), Jiang, Yunhong wrote: >> Add a new p2m type for broken memory. >> >> Currently, this is used only for EPT guest. When memory assigned to guest is >poisoned, we will mark it as broken in p2m table, the corresponding EPT entry is set >as not present. Later, if guest try to access the affected memory, EPT violation will >happen and Xen hypervisor can trap this access. >> >> Signed-off-by: Jiang, Yunhong <yunhong.jiang@intel.com> >> >> diff -r 29f0479830cd xen/arch/x86/hvm/hvm.c >> --- a/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:12:31 2010 +0800 >> +++ b/xen/arch/x86/hvm/hvm.c Mon Jul 12 13:58:55 2010 +0800 >> @@ -971,6 +971,12 @@ bool_t hvm_hap_nested_page_fault(unsigne >> >> mfn = gfn_to_mfn_type_current(gfn, &p2mt, p2m_guest); >> >> + if ( unlikely(p2mt == p2m_ram_broken) ) >> + { >> + domain_crash(current->domain); >> + return 1; >> + } >> + > >You should probably do this in more places, even if you don''t care >about shadow pagetables -- MMIO emulation should behave the same as >normal accesses.What do you mean of " the same as normal access"? MMIO will not be poisoned and will not be marked as p2m_ram_broken. We only need track guest''s access to poison RAM. There are some case need considered, like hypervisor emulate instruction for guest. For example, considering "movs (*rsi), (*rdi)", where rdi points to MMIO or APIC, while rsi points to poison memory. However, In such situation, it will trigger EPT fault firstly and cause the guest be crashed (I tested movs from poison memory to apic range). As there is no prefetch in EPT situation if I understand correctly, I assume it should be ok at least for EPT guest.> >What behaviour would you like when qemu tries to DMA to a broken page? >Or when a backend driver grant-copies to it?Yes, we need consider this also, but that is more like unmap for PV guest. I do have internal patch to handle more situation, like following. But I didn''t find method to test it for EPT guest, I delay it to future work. @@ -94,6 +94,12 @@ static inline void *map_domain_gfn(struc { /* Translate the gfn, unsharing if shared */ *mfn = gfn_to_mfn_unshare(d, gfn_x(gfn), p2mt, 0); + + if ( p2m_is_broken(*p2mt) ) + { + *rc = _PAGE_BROKEN; + return NULL; + } if ( p2m_is_paging(*p2mt) ) { p2m_mem_paging_populate(d, gfn_x(gfn)); BTW, I think we need try our best to unmap, but maybe we can''t cover every scenerio. For example, this patch will cover the access from EPT guest. But maybe we will not unmap for grant map, if the implementation is too intrusive, or maintainer will not accept it if too complex :-).> >Is there a case for just having the P2M lookups (at least the _query() >kind) call domain_crash when they hit a poisoned page?I''m not sure that will be over-killed. Will caller of p2m lookup can tolerate failure? Thanks --jyh> >Cheers, > >Tim. > >> /* >> * If this GFN is emulated MMIO or marked as read-only, pass the fault >> * to the mmio handler. >> diff -r 29f0479830cd xen/include/asm-x86/p2m.h >> --- a/xen/include/asm-x86/p2m.h Mon Jul 12 13:12:31 2010 +0800 >> +++ b/xen/include/asm-x86/p2m.h Mon Jul 12 13:56:02 2010 +0800 >> @@ -85,6 +85,7 @@ typedef enum { >> p2m_ram_paging_in = 11, /* Memory that is being paged in */ >> p2m_ram_paging_in_start = 12, /* Memory that is being paged in */ >> p2m_ram_shared = 13, /* Shared or sharable memory */ >> + p2m_ram_broken = 14, /* broken page, access cause domain >crash*/ >> } p2m_type_t; >> >> typedef enum { >> @@ -132,6 +133,8 @@ typedef enum { >> | p2m_to_mask(p2m_ram_paging_in)) >> >> #define P2M_PAGED_TYPES (p2m_to_mask(p2m_ram_paged)) >> + >> +#define P2M_BROKEN_TYPES (p2m_to_mask(p2m_ram_broken)) >> >> /* Shared types */ >> /* XXX: Sharable types could include p2m_ram_ro too, but we would need to >> @@ -155,6 +158,7 @@ typedef enum { >> #define p2m_is_paged(_t) (p2m_to_mask(_t) & P2M_PAGED_TYPES) >> #define p2m_is_sharable(_t) (p2m_to_mask(_t) & P2M_SHARABLE_TYPES) >> #define p2m_is_shared(_t) (p2m_to_mask(_t) & P2M_SHARED_TYPES) >> +#define p2m_is_broken(_t) (p2m_to_mask(_t) & P2M_BROKEN_TYPES) >> >> >> /* Populate-on-demand */ >> diff -r 29f0479830cd xen/include/asm-x86/page.h >> --- a/xen/include/asm-x86/page.h Mon Jul 12 13:12:31 2010 +0800 >> +++ b/xen/include/asm-x86/page.h Mon Jul 12 13:56:21 2010 +0800 >> @@ -323,6 +323,7 @@ void setup_idle_pagetable(void); >> #define _PAGE_PSE_PAT 0x1000U >> #define _PAGE_PAGED 0x2000U >> #define _PAGE_SHARED 0x4000U >> +#define _PAGE_BROKEN 0x8000U >> >> /* >> * Debug option: Ensure that granted mappings are not implicitly unmapped. >> >> > > > >-- >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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Jul-14 13:50 UTC
Re: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
On 14/07/2010 14:41, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:>> You should probably do this in more places, even if you don''t care >> about shadow pagetables -- MMIO emulation should behave the same as >> normal accesses. > > What do you mean of " the same as normal access"? > MMIO will not be poisoned and will not be marked as p2m_ram_broken. We only > need track guest''s access to poison RAM. > > There are some case need considered, like hypervisor emulate instruction for > guest. For example, considering "movs (*rsi), (*rdi)", where rdi points to > MMIO or APIC, while rsi points to poison memory. However, In such situation, > it will trigger EPT fault firstly and cause the guest be crashed (I tested > movs from poison memory to apic range). As there is no prefetch in EPT > situation if I understand correctly, I assume it should be ok at least for EPT > guest.I doubt it''s architecturally guaranteed. What about edge cases like ''PUSH mem'' from an MMIO location, destination is stack, pointer to which has just crossed a page boundary to a broken page? I don''t think it''s hard to fill in the blanks for emulation of guest instructions so you might as well do so. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Jul-14 14:07 UTC
RE: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Wednesday, July 14, 2010 9:50 PM >To: Jiang, Yunhong; Tim Deegan >Cc: xen-devel >Subject: Re: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory > >On 14/07/2010 14:41, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >>> You should probably do this in more places, even if you don''t care >>> about shadow pagetables -- MMIO emulation should behave the same as >>> normal accesses. >> >> What do you mean of " the same as normal access"? >> MMIO will not be poisoned and will not be marked as p2m_ram_broken. We only >> need track guest''s access to poison RAM. >> >> There are some case need considered, like hypervisor emulate instruction for >> guest. For example, considering "movs (*rsi), (*rdi)", where rdi points to >> MMIO or APIC, while rsi points to poison memory. However, In such situation, >> it will trigger EPT fault firstly and cause the guest be crashed (I tested >> movs from poison memory to apic range). As there is no prefetch in EPT >> situation if I understand correctly, I assume it should be ok at least for EPT >> guest. > >I doubt it''s architecturally guaranteed. What about edge cases like ''PUSH >mem'' from an MMIO location, destination is stack, pointer to which has just >crossed a page boundary to a broken page? I don''t think it''s hard to fill in >the blanks for emulation of guest instructions so you might as well do so.I do have the code in hand for guest instruction emulation (not fully reviewed, but should be ok), but as I can''t any test case to test it at all and I thought it will not happen for EPT guest, so I hold it in hand and didn''t send out. Your case is a excellent one that this is needed for EPT guest, I will crate a test case for it and try tomorrow. Thanks --jyh> > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jiang, Yunhong
2010-Jul-14 15:18 UTC
RE: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory
>-----Original Message----- >From: xen-devel-bounces@lists.xensource.com >[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Jiang, Yunhong >Sent: Wednesday, July 14, 2010 10:08 PM >To: Keir Fraser; Tim Deegan >Cc: xen-devel >Subject: RE: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory > > > >>-----Original Message----- >>From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >>Sent: Wednesday, July 14, 2010 9:50 PM >>To: Jiang, Yunhong; Tim Deegan >>Cc: xen-devel >>Subject: Re: [Xen-devel] Re: [PATCH] Add a new p2m type for broken memory >> >>On 14/07/2010 14:41, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: >> >>>> You should probably do this in more places, even if you don''t care >>>> about shadow pagetables -- MMIO emulation should behave the same as >>>> normal accesses. >>> >>> What do you mean of " the same as normal access"? >>> MMIO will not be poisoned and will not be marked as p2m_ram_broken. We only >>> need track guest''s access to poison RAM. >>> >>> There are some case need considered, like hypervisor emulate instruction for >>> guest. For example, considering "movs (*rsi), (*rdi)", where rdi points to >>> MMIO or APIC, while rsi points to poison memory. However, In such situation, >>> it will trigger EPT fault firstly and cause the guest be crashed (I tested >>> movs from poison memory to apic range). As there is no prefetch in EPT >>> situation if I understand correctly, I assume it should be ok at least for EPT >>> guest. >> >>I doubt it''s architecturally guaranteed. What about edge cases like ''PUSH >>mem'' from an MMIO location, destination is stack, pointer to which has just >>crossed a page boundary to a broken page? I don''t think it''s hard to fill in >>the blanks for emulation of guest instructions so you might as well do so. > >I do have the code in hand for guest instruction emulation (not fully reviewed, but >should be ok), but as I can''t any test case to test it at all and I thought it will not >happen for EPT guest, so I hold it in hand and didn''t send out. Your case is a excellent >one that this is needed for EPT guest, I will crate a test case for it and try tomorrow.Just realized your case will not trigger error either. The push cmd will write the broken memory, instead of read. Write a broken memory will not consume the data, instead, it will overwrite it and will not cause MCE. In fact, this is similar to my experiment movs (*rsi), (*rdi), where the src is the local apic id and the target is broken memory, which will cause APIC access VMExit. But xen''s emulation will not cause MCE. But I do agree with you that it is not architecturely guranteed, so I will send out the emulation patch seperately for review. I suspect it will be tested only when we do unmap for PV guest. --jyh> >Thanks >--jyh > >> >> -- Keir >> > > >_______________________________________________ >Xen-devel mailing list >Xen-devel@lists.xensource.com >http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel