* [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination @ 2016-09-14 7:55 Herongguang (Stephen) 2016-09-14 9:05 ` Paolo Bonzini ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-14 7:55 UTC (permalink / raw) To: qemu-devel, pbonzini, quintela, amit.shah; +Cc: arei.gonglei, Huangweidong (C) Hi, We found a problem that when a redhat 6 VM reboots (in grub countdown UI), migrating this VM will result in VM’s memory difference between source and destination side. The difference always resides in GPA 0xA0000~0xC0000, i.e. SMRAM area. Occasionally this result in VM instruction emulation error in destination side. After some digging, I think this is because in migration code, in migration_bitmap_sync(), only memory slots in address space address_space_memory’s dirty bitmap fetched from kvm-kmod, while SMRAM memory slot, in address space smram_address_space’s dirty bitmap not fetched from kvm-kmod, thus modifications in SMRAM in source side are not sent to destination side. I tried following patch, and this phenomenon does not happen anymore. Do you think this patch is OK or do you have better idea? Thanks. diff --git a/migration/ram.c b/migration/ram.c index a3d70c4..1cc4360 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -607,6 +607,8 @@ static void migration_bitmap_sync_init(void) iterations_prev = 0; } +extern AddressSpace smram_address_space; + static void migration_bitmap_sync(void) { RAMBlock *block; @@ -627,6 +629,7 @@ static void migration_bitmap_sync(void) trace_migration_bitmap_sync_start(); address_space_sync_dirty_bitmap(&address_space_memory); + address_space_sync_dirty_bitmap(&smram_address_space); qemu_mutex_lock(&migration_bitmap_mutex); rcu_read_lock(); diff --git a/target-i386/kvm.c b/target-i386/kvm.c index d1a25c5..b98fe22 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -1111,7 +1111,7 @@ static int kvm_get_supported_msrs(KVMState *s) static Notifier smram_machine_done; static KVMMemoryListener smram_listener; -static AddressSpace smram_address_space; +AddressSpace smram_address_space; static MemoryRegion smram_as_root; static MemoryRegion smram_as_mem; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-14 7:55 [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination Herongguang (Stephen) @ 2016-09-14 9:05 ` Paolo Bonzini 2016-09-22 13:16 ` Herongguang (Stephen) 2016-09-22 7:51 ` [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly Herongguang (Stephen) 2016-09-22 7:56 ` [Qemu-devel] [RFC/PATCH 3] kvm: fix events.flags (KVM_VCPUEVENT_VALID_SMM) overwritten by 0 Herongguang (Stephen) 2 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2016-09-14 9:05 UTC (permalink / raw) To: Herongguang (Stephen), qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 14/09/2016 09:55, Herongguang (Stephen) wrote: > Hi, > We found a problem that when a redhat 6 VM reboots (in grub countdown > UI), migrating this VM will result in VM’s memory difference between > source and destination side. The difference always resides in GPA > 0xA0000~0xC0000, i.e. SMRAM area. > > Occasionally this result in VM instruction emulation error in > destination side. > > After some digging, I think this is because in migration code, in > migration_bitmap_sync(), only memory slots in address space > address_space_memory’s dirty bitmap fetched from kvm-kmod, while SMRAM > memory slot, in address space smram_address_space’s dirty bitmap not > fetched from kvm-kmod, thus modifications in SMRAM in source side are > not sent to destination side. > > I tried following patch, and this phenomenon does not happen anymore. Do > you think this patch is OK or do you have better idea? Thanks. Nice caatch! I think the right solution here is to sync all RAM memory regions instead of the address spaces. You can do that by putting a notifier in MemoryRegion; register the notifier in all the RAM creation functions (basically after every mr->ram=true or mr->rom_device=true), and unregister it in memory_region_destructor_ram. Thanks, Paolo > diff --git a/migration/ram.c b/migration/ram.c > index a3d70c4..1cc4360 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -607,6 +607,8 @@ static void migration_bitmap_sync_init(void) > iterations_prev = 0; > } > > +extern AddressSpace smram_address_space; > + > static void migration_bitmap_sync(void) > { > RAMBlock *block; > @@ -627,6 +629,7 @@ static void migration_bitmap_sync(void) > > trace_migration_bitmap_sync_start(); > address_space_sync_dirty_bitmap(&address_space_memory); > + address_space_sync_dirty_bitmap(&smram_address_space); > > qemu_mutex_lock(&migration_bitmap_mutex); > rcu_read_lock(); > diff --git a/target-i386/kvm.c b/target-i386/kvm.c > index d1a25c5..b98fe22 100644 > --- a/target-i386/kvm.c > +++ b/target-i386/kvm.c > @@ -1111,7 +1111,7 @@ static int kvm_get_supported_msrs(KVMState *s) > > static Notifier smram_machine_done; > static KVMMemoryListener smram_listener; > -static AddressSpace smram_address_space; > +AddressSpace smram_address_space; > static MemoryRegion smram_as_root; > static MemoryRegion smram_as_mem; > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-14 9:05 ` Paolo Bonzini @ 2016-09-22 13:16 ` Herongguang (Stephen) 2016-09-23 1:11 ` Herongguang (Stephen) 2016-09-23 7:17 ` Paolo Bonzini 0 siblings, 2 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-22 13:16 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 2016/9/14 17:05, Paolo Bonzini wrote: > > > On 14/09/2016 09:55, Herongguang (Stephen) wrote: >> Hi, >> We found a problem that when a redhat 6 VM reboots (in grub countdown >> UI), migrating this VM will result in VM’s memory difference between >> source and destination side. The difference always resides in GPA >> 0xA0000~0xC0000, i.e. SMRAM area. >> >> Occasionally this result in VM instruction emulation error in >> destination side. >> >> After some digging, I think this is because in migration code, in >> migration_bitmap_sync(), only memory slots in address space >> address_space_memory’s dirty bitmap fetched from kvm-kmod, while SMRAM >> memory slot, in address space smram_address_space’s dirty bitmap not >> fetched from kvm-kmod, thus modifications in SMRAM in source side are >> not sent to destination side. >> >> I tried following patch, and this phenomenon does not happen anymore. Do >> you think this patch is OK or do you have better idea? Thanks. > > Nice caatch! > > I think the right solution here is to sync all RAM memory regions > instead of the address spaces. You can do that by putting a notifier in > MemoryRegion; register the notifier in all the RAM creation functions > (basically after every mr->ram=true or mr->rom_device=true), and > unregister it in memory_region_destructor_ram. > > Thanks, > > Paolo > I have some concern: 1. For example, vhost does not know about as_id, I wonder if guests in SMM can operate disk or ether card, as in that case vhost would not logging dirty pages correctly, without knowing as_id. 2. If a memory region is disabled/enabled/disabled frequently, since disabled memory regions would be removed from memory slots in kvm-kmod, dirty pages would be discarded in kvm-kmod and qemu when disabled, thus missing. Is my assumption correct? 3. I agree your opinion that the right solution is to get dirty-page information for all memory region from kvm-kmod. But I found it’s somewhat hard to implement since kvm_log_sync() expects a MemoryRegionSection* parameter. Do you have good idea? As to all the ram memory regions, I think they are all in the ram_list.blocks, so there is no need to create a notifier, is this correct? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-22 13:16 ` Herongguang (Stephen) @ 2016-09-23 1:11 ` Herongguang (Stephen) 2016-09-23 7:17 ` Paolo Bonzini 1 sibling, 0 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-23 1:11 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 2016/9/22 21:16, Herongguang (Stephen) wrote: > > > On 2016/9/14 17:05, Paolo Bonzini wrote: >> >> >> On 14/09/2016 09:55, Herongguang (Stephen) wrote: >>> Hi, >>> We found a problem that when a redhat 6 VM reboots (in grub countdown >>> UI), migrating this VM will result in VM’s memory difference between >>> source and destination side. The difference always resides in GPA >>> 0xA0000~0xC0000, i.e. SMRAM area. >>> >>> Occasionally this result in VM instruction emulation error in >>> destination side. >>> >>> After some digging, I think this is because in migration code, in >>> migration_bitmap_sync(), only memory slots in address space >>> address_space_memory’s dirty bitmap fetched from kvm-kmod, while SMRAM >>> memory slot, in address space smram_address_space’s dirty bitmap not >>> fetched from kvm-kmod, thus modifications in SMRAM in source side are >>> not sent to destination side. >>> >>> I tried following patch, and this phenomenon does not happen anymore. Do >>> you think this patch is OK or do you have better idea? Thanks. >> >> Nice caatch! >> >> I think the right solution here is to sync all RAM memory regions >> instead of the address spaces. You can do that by putting a notifier in >> MemoryRegion; register the notifier in all the RAM creation functions >> (basically after every mr->ram=true or mr->rom_device=true), and >> unregister it in memory_region_destructor_ram. >> >> Thanks, >> >> Paolo >> > > I have some concern: > 1. For example, vhost does not know about as_id, I wonder if guests in SMM can operate disk or ether card, as in > that case vhost would not logging dirty pages correctly, without knowing as_id. > > 2. If a memory region is disabled/enabled/disabled frequently, since disabled memory regions would be removed > from memory slots in kvm-kmod, dirty pages would be discarded in kvm-kmod and qemu when disabled, thus missing. > Is my assumption correct? After reviewing code, I think question 2 does not exist as qemu will sync dirty page before removing memory slots in kvm_set_phys_mem. > > 3. I agree your opinion that the right solution is to get dirty-page information for all memory region from > kvm-kmod. But I found it’s somewhat hard to implement since kvm_log_sync() expects a MemoryRegionSection* > parameter. Do you have good idea? > > As to all the ram memory regions, I think they are all in the ram_list.blocks, so there is no need to create > a notifier, is this correct? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-22 13:16 ` Herongguang (Stephen) 2016-09-23 1:11 ` Herongguang (Stephen) @ 2016-09-23 7:17 ` Paolo Bonzini 2016-09-23 8:51 ` Herongguang (Stephen) 2016-09-25 11:33 ` Herongguang (Stephen) 1 sibling, 2 replies; 14+ messages in thread From: Paolo Bonzini @ 2016-09-23 7:17 UTC (permalink / raw) To: Herongguang (Stephen), qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 22/09/2016 15:16, Herongguang (Stephen) wrote: > I have some concern: > 1. For example, vhost does not know about as_id, I wonder if guests in > SMM can operate disk or ether card, as in > that case vhost would not logging dirty pages correctly, without knowing > as_id. In the end memory is logged by ram_addr_t, not by address space. So if vhost_sync_dirty_bitmap is called on the right region everything works. Guests in SMM can operate on storage devices, but storage devices cannot write to 0xA0000-0xBFFFF so that's safe. > 2. If a memory region is disabled/enabled/disabled frequently, since > disabled memory regions would be removed > from memory slots in kvm-kmod, dirty pages would be discarded in > kvm-kmod and qemu when disabled, thus missing. > Is my assumption correct? As you found, this is handled by kvm_set_phys_mem: if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { kvm_physical_sync_dirty_bitmap(kml, section); } > 3. I agree your opinion that the right solution is to get dirty-page > information for all memory region from > kvm-kmod. But I found it’s somewhat hard to implement since > kvm_log_sync() expects a MemoryRegionSection* > parameter. Do you have good idea? You're right, memory_region_sync_dirty_bitmap handles this but it's inefficient. So your patch is correct, but it breaks for !x86 as you probably know. We need to look at the address spaces which have a listener that implements log_sync. > As to all the ram memory regions, I think they are all in the > ram_list.blocks, so there is no need to create > a notifier, is this correct? Yes, makes sense. The patch here is a bit of a hack but is efficient. It looks at each address space just once and calls log_sync just once per FlatRange. Paolo diff --git a/include/exec/memory.h b/include/exec/memory.h index 3e4d416..23b086d 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -1154,12 +1154,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, hwaddr addr, uint64_t size); /** - * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory + * memory_global_dirty_log_sync: synchronize the dirty log for all memory * - * Synchronizes the dirty page log for an entire address space. - * @as: the address space that contains the memory being synchronized + * Synchronizes the dirty page log for all address spaces. */ -void address_space_sync_dirty_bitmap(AddressSpace *as); +void memory_global_dirty_log_sync(void); /** * memory_region_transaction_begin: Start a transaction. diff --git a/memory.c b/memory.c index 1a1baf5..6cac674 100644 --- a/memory.c +++ b/memory.c @@ -158,15 +158,11 @@ static bool memory_listener_match(MemoryListener *listener, /* No need to ref/unref .mr, the FlatRange keeps it alive. */ #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...) \ - MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) { \ - .mr = (fr)->mr, \ - .address_space = (as), \ - .offset_within_region = (fr)->offset_in_region, \ - .size = (fr)->addr.size, \ - .offset_within_address_space = int128_get64((fr)->addr.start), \ - .readonly = (fr)->readonly, \ - }), ##_args) - + do { \ + MemoryRegionSection mrs = section_from_flat_range(fr, as); \ + MEMORY_LISTENER_CALL(callback, dir, &mrs, ##_args); \ + } while(0) + struct CoalescedMemoryRange { AddrRange addr; QTAILQ_ENTRY(CoalescedMemoryRange) link; @@ -245,6 +241,19 @@ typedef struct AddressSpaceOps AddressSpaceOps; #define FOR_EACH_FLAT_RANGE(var, view) \ for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var) +static inline MemoryRegionSection +section_from_flat_range(FlatRange *fr, AddressSpace *as) +{ + return (MemoryRegionSection) { + .mr = fr->mr, + .address_space = as, + .offset_within_region = fr->offset_in_region, + .size = fr->addr.size, + .offset_within_address_space = int128_get64(fr->addr.start), + .readonly = fr->readonly, + }; +} + static bool flatrange_equal(FlatRange *a, FlatRange *b) { return a->mr == b->mr @@ -2124,16 +2133,25 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr) return mr && mr != container; } -void address_space_sync_dirty_bitmap(AddressSpace *as) +void memory_global_dirty_log_sync(void) { + MemoryListener *listener; + AddressSpace *as; FlatView *view; FlatRange *fr; - view = address_space_get_flatview(as); - FOR_EACH_FLAT_RANGE(fr, view) { - MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); + QTAILQ_FOREACH(listener, &memory_listeners, link) { + if (!listener->log_sync || !listener->address_space_filter) { + continue; + } + as = listener->address_space_filter; + view = address_space_get_flatview(as); + FOR_EACH_FLAT_RANGE(fr, view) { + MemoryRegionSection mrs = section_from_flat_range(fr, as); + listener->log_sync(listener, &mrs); + } + flatview_unref(view); } - flatview_unref(view); } void memory_global_dirty_log_start(void) diff --git a/migration/ram.c b/migration/ram.c index a6e1c63..e33ff5e 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -626,7 +626,7 @@ static void migration_bitmap_sync(void) } trace_migration_bitmap_sync_start(); - address_space_sync_dirty_bitmap(&address_space_memory); + memory_global_dirty_log_sync(); qemu_mutex_lock(&migration_bitmap_mutex); rcu_read_lock(); ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-23 7:17 ` Paolo Bonzini @ 2016-09-23 8:51 ` Herongguang (Stephen) 2016-09-23 8:59 ` Paolo Bonzini 2016-09-25 11:33 ` Herongguang (Stephen) 1 sibling, 1 reply; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-23 8:51 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 2016/9/23 15:17, Paolo Bonzini wrote: > > > On 22/09/2016 15:16, Herongguang (Stephen) wrote: >> I have some concern: >> 1. For example, vhost does not know about as_id, I wonder if guests in >> SMM can operate disk or ether card, as in >> that case vhost would not logging dirty pages correctly, without knowing >> as_id. > > In the end memory is logged by ram_addr_t, not by address space. So if > vhost_sync_dirty_bitmap is called on the right region everything works. > > Guests in SMM can operate on storage devices, but storage devices cannot > write to 0xA0000-0xBFFFF so that's safe. You are right, as vhost does not have a smram address space listener. So is this a qemu's flaw that virtual devices can not write to SMRAM? > >> 2. If a memory region is disabled/enabled/disabled frequently, since >> disabled memory regions would be removed >> from memory slots in kvm-kmod, dirty pages would be discarded in >> kvm-kmod and qemu when disabled, thus missing. >> Is my assumption correct? > > As you found, this is handled by kvm_set_phys_mem: > > if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { > kvm_physical_sync_dirty_bitmap(kml, section); > } > >> 3. I agree your opinion that the right solution is to get dirty-page >> information for all memory region from >> kvm-kmod. But I found it’s somewhat hard to implement since >> kvm_log_sync() expects a MemoryRegionSection* >> parameter. Do you have good idea? > > You're right, memory_region_sync_dirty_bitmap handles this but it's > inefficient. > > So your patch is correct, but it breaks for !x86 as you probably know. > We need to look at the address spaces which have a listener that > implements log_sync. > >> As to all the ram memory regions, I think they are all in the >> ram_list.blocks, so there is no need to create >> a notifier, is this correct? > > Yes, makes sense. > > The patch here is a bit of a hack but is efficient. It looks at > each address space just once and calls log_sync just once per > FlatRange. > > Paolo > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 3e4d416..23b086d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1154,12 +1154,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, > hwaddr addr, uint64_t size); > > /** > - * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory > + * memory_global_dirty_log_sync: synchronize the dirty log for all memory > * > - * Synchronizes the dirty page log for an entire address space. > - * @as: the address space that contains the memory being synchronized > + * Synchronizes the dirty page log for all address spaces. > */ > -void address_space_sync_dirty_bitmap(AddressSpace *as); > +void memory_global_dirty_log_sync(void); > > /** > * memory_region_transaction_begin: Start a transaction. > diff --git a/memory.c b/memory.c > index 1a1baf5..6cac674 100644 > --- a/memory.c > +++ b/memory.c > @@ -158,15 +158,11 @@ static bool memory_listener_match(MemoryListener *listener, > > /* No need to ref/unref .mr, the FlatRange keeps it alive. */ > #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...) \ > - MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) { \ > - .mr = (fr)->mr, \ > - .address_space = (as), \ > - .offset_within_region = (fr)->offset_in_region, \ > - .size = (fr)->addr.size, \ > - .offset_within_address_space = int128_get64((fr)->addr.start), \ > - .readonly = (fr)->readonly, \ > - }), ##_args) > - > + do { \ > + MemoryRegionSection mrs = section_from_flat_range(fr, as); \ > + MEMORY_LISTENER_CALL(callback, dir, &mrs, ##_args); \ > + } while(0) > + > struct CoalescedMemoryRange { > AddrRange addr; > QTAILQ_ENTRY(CoalescedMemoryRange) link; > @@ -245,6 +241,19 @@ typedef struct AddressSpaceOps AddressSpaceOps; > #define FOR_EACH_FLAT_RANGE(var, view) \ > for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var) > > +static inline MemoryRegionSection > +section_from_flat_range(FlatRange *fr, AddressSpace *as) > +{ > + return (MemoryRegionSection) { > + .mr = fr->mr, > + .address_space = as, > + .offset_within_region = fr->offset_in_region, > + .size = fr->addr.size, > + .offset_within_address_space = int128_get64(fr->addr.start), > + .readonly = fr->readonly, > + }; > +} > + > static bool flatrange_equal(FlatRange *a, FlatRange *b) > { > return a->mr == b->mr > @@ -2124,16 +2133,25 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr) > return mr && mr != container; > } > > -void address_space_sync_dirty_bitmap(AddressSpace *as) > +void memory_global_dirty_log_sync(void) > { > + MemoryListener *listener; > + AddressSpace *as; > FlatView *view; > FlatRange *fr; > > - view = address_space_get_flatview(as); > - FOR_EACH_FLAT_RANGE(fr, view) { > - MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); > + QTAILQ_FOREACH(listener, &memory_listeners, link) { > + if (!listener->log_sync || !listener->address_space_filter) { > + continue; > + } > + as = listener->address_space_filter; > + view = address_space_get_flatview(as); > + FOR_EACH_FLAT_RANGE(fr, view) { > + MemoryRegionSection mrs = section_from_flat_range(fr, as); > + listener->log_sync(listener, &mrs); > + } > + flatview_unref(view); > } > - flatview_unref(view); > } > > void memory_global_dirty_log_start(void) > diff --git a/migration/ram.c b/migration/ram.c > index a6e1c63..e33ff5e 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -626,7 +626,7 @@ static void migration_bitmap_sync(void) > } > > trace_migration_bitmap_sync_start(); > - address_space_sync_dirty_bitmap(&address_space_memory); > + memory_global_dirty_log_sync(); > > qemu_mutex_lock(&migration_bitmap_mutex); > rcu_read_lock(); > Fine, your patch is simpler than I thought and functions right. Reviewed-by: He Rongguang <herongguang.he@huawei.com> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-23 8:51 ` Herongguang (Stephen) @ 2016-09-23 8:59 ` Paolo Bonzini 2016-09-23 9:14 ` Herongguang (Stephen) 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2016-09-23 8:59 UTC (permalink / raw) To: Herongguang (Stephen), qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 23/09/2016 10:51, Herongguang (Stephen) wrote: > > > On 2016/9/23 15:17, Paolo Bonzini wrote: >> >> >> On 22/09/2016 15:16, Herongguang (Stephen) wrote: >>> I have some concern: >>> 1. For example, vhost does not know about as_id, I wonder if guests in >>> SMM can operate disk or ether card, as in >>> that case vhost would not logging dirty pages correctly, without knowing >>> as_id. >> >> In the end memory is logged by ram_addr_t, not by address space. So if >> vhost_sync_dirty_bitmap is called on the right region everything works. >> >> Guests in SMM can operate on storage devices, but storage devices cannot >> write to 0xA0000-0xBFFFF so that's safe. > You are right, as vhost does not have a smram address space listener. So > is this a qemu's flaw that virtual devices can not write to SMRAM? No, it's how it works in real hardware. However, it is a (minor) bug that vhost doesn't have the equivalent of kvm_set_phys_mem's call to kvm_physical_sync_dirty_bitmap. > Fine, your patch is simpler than I thought and functions right. Great! > Reviewed-by: He Rongguang <herongguang.he@huawei.com> Also Tested-by? Thanks, Paolo > >> . >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-23 8:59 ` Paolo Bonzini @ 2016-09-23 9:14 ` Herongguang (Stephen) 0 siblings, 0 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-23 9:14 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 2016/9/23 16:59, Paolo Bonzini wrote: > > > On 23/09/2016 10:51, Herongguang (Stephen) wrote: >> >> >> On 2016/9/23 15:17, Paolo Bonzini wrote: >>> >>> >>> On 22/09/2016 15:16, Herongguang (Stephen) wrote: >>>> I have some concern: >>>> 1. For example, vhost does not know about as_id, I wonder if guests in >>>> SMM can operate disk or ether card, as in >>>> that case vhost would not logging dirty pages correctly, without knowing >>>> as_id. >>> >>> In the end memory is logged by ram_addr_t, not by address space. So if >>> vhost_sync_dirty_bitmap is called on the right region everything works. >>> >>> Guests in SMM can operate on storage devices, but storage devices cannot >>> write to 0xA0000-0xBFFFF so that's safe. >> You are right, as vhost does not have a smram address space listener. So >> is this a qemu's flaw that virtual devices can not write to SMRAM? > > No, it's how it works in real hardware. However, it is a (minor) bug > that vhost doesn't have the equivalent of kvm_set_phys_mem's call to > kvm_physical_sync_dirty_bitmap. > >> Fine, your patch is simpler than I thought and functions right. > > Great! > >> Reviewed-by: He Rongguang <herongguang.he@huawei.com> > > Also Tested-by? Yes, I will test this patch in these days. > > Thanks, > > Paolo >> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-23 7:17 ` Paolo Bonzini 2016-09-23 8:51 ` Herongguang (Stephen) @ 2016-09-25 11:33 ` Herongguang (Stephen) 2016-09-26 7:15 ` Paolo Bonzini 1 sibling, 1 reply; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-25 11:33 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 2016/9/23 15:17, Paolo Bonzini wrote: > > > On 22/09/2016 15:16, Herongguang (Stephen) wrote: >> I have some concern: >> 1. For example, vhost does not know about as_id, I wonder if guests in >> SMM can operate disk or ether card, as in >> that case vhost would not logging dirty pages correctly, without knowing >> as_id. > > In the end memory is logged by ram_addr_t, not by address space. So if > vhost_sync_dirty_bitmap is called on the right region everything works. > > Guests in SMM can operate on storage devices, but storage devices cannot > write to 0xA0000-0xBFFFF so that's safe. > >> 2. If a memory region is disabled/enabled/disabled frequently, since >> disabled memory regions would be removed >> from memory slots in kvm-kmod, dirty pages would be discarded in >> kvm-kmod and qemu when disabled, thus missing. >> Is my assumption correct? > > As you found, this is handled by kvm_set_phys_mem: > > if (mem->flags & KVM_MEM_LOG_DIRTY_PAGES) { > kvm_physical_sync_dirty_bitmap(kml, section); > } > >> 3. I agree your opinion that the right solution is to get dirty-page >> information for all memory region from >> kvm-kmod. But I found it’s somewhat hard to implement since >> kvm_log_sync() expects a MemoryRegionSection* >> parameter. Do you have good idea? > > You're right, memory_region_sync_dirty_bitmap handles this but it's > inefficient. > > So your patch is correct, but it breaks for !x86 as you probably know. > We need to look at the address spaces which have a listener that > implements log_sync. > >> As to all the ram memory regions, I think they are all in the >> ram_list.blocks, so there is no need to create >> a notifier, is this correct? > > Yes, makes sense. > > The patch here is a bit of a hack but is efficient. It looks at > each address space just once and calls log_sync just once per > FlatRange. > > Paolo > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 3e4d416..23b086d 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1154,12 +1154,11 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr, > hwaddr addr, uint64_t size); > > /** > - * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory > + * memory_global_dirty_log_sync: synchronize the dirty log for all memory > * > - * Synchronizes the dirty page log for an entire address space. > - * @as: the address space that contains the memory being synchronized > + * Synchronizes the dirty page log for all address spaces. > */ > -void address_space_sync_dirty_bitmap(AddressSpace *as); > +void memory_global_dirty_log_sync(void); > > /** > * memory_region_transaction_begin: Start a transaction. > diff --git a/memory.c b/memory.c > index 1a1baf5..6cac674 100644 > --- a/memory.c > +++ b/memory.c > @@ -158,15 +158,11 @@ static bool memory_listener_match(MemoryListener *listener, > > /* No need to ref/unref .mr, the FlatRange keeps it alive. */ > #define MEMORY_LISTENER_UPDATE_REGION(fr, as, dir, callback, _args...) \ > - MEMORY_LISTENER_CALL(callback, dir, (&(MemoryRegionSection) { \ > - .mr = (fr)->mr, \ > - .address_space = (as), \ > - .offset_within_region = (fr)->offset_in_region, \ > - .size = (fr)->addr.size, \ > - .offset_within_address_space = int128_get64((fr)->addr.start), \ > - .readonly = (fr)->readonly, \ > - }), ##_args) > - > + do { \ > + MemoryRegionSection mrs = section_from_flat_range(fr, as); \ > + MEMORY_LISTENER_CALL(callback, dir, &mrs, ##_args); \ > + } while(0) > + > struct CoalescedMemoryRange { > AddrRange addr; > QTAILQ_ENTRY(CoalescedMemoryRange) link; > @@ -245,6 +241,19 @@ typedef struct AddressSpaceOps AddressSpaceOps; > #define FOR_EACH_FLAT_RANGE(var, view) \ > for (var = (view)->ranges; var < (view)->ranges + (view)->nr; ++var) > > +static inline MemoryRegionSection > +section_from_flat_range(FlatRange *fr, AddressSpace *as) > +{ > + return (MemoryRegionSection) { > + .mr = fr->mr, > + .address_space = as, > + .offset_within_region = fr->offset_in_region, > + .size = fr->addr.size, > + .offset_within_address_space = int128_get64(fr->addr.start), > + .readonly = fr->readonly, > + }; > +} > + > static bool flatrange_equal(FlatRange *a, FlatRange *b) > { > return a->mr == b->mr > @@ -2124,16 +2133,25 @@ bool memory_region_present(MemoryRegion *container, hwaddr addr) > return mr && mr != container; > } > > -void address_space_sync_dirty_bitmap(AddressSpace *as) > +void memory_global_dirty_log_sync(void) > { > + MemoryListener *listener; > + AddressSpace *as; > FlatView *view; > FlatRange *fr; > > - view = address_space_get_flatview(as); > - FOR_EACH_FLAT_RANGE(fr, view) { > - MEMORY_LISTENER_UPDATE_REGION(fr, as, Forward, log_sync); > + QTAILQ_FOREACH(listener, &memory_listeners, link) { > + if (!listener->log_sync || !listener->address_space_filter) { > + continue; > + } > + as = listener->address_space_filter; > + view = address_space_get_flatview(as); > + FOR_EACH_FLAT_RANGE(fr, view) { > + MemoryRegionSection mrs = section_from_flat_range(fr, as); > + listener->log_sync(listener, &mrs); > + } > + flatview_unref(view); > } > - flatview_unref(view); > } > > void memory_global_dirty_log_start(void) > diff --git a/migration/ram.c b/migration/ram.c > index a6e1c63..e33ff5e 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -626,7 +626,7 @@ static void migration_bitmap_sync(void) > } > > trace_migration_bitmap_sync_start(); > - address_space_sync_dirty_bitmap(&address_space_memory); > + memory_global_dirty_log_sync(); > > qemu_mutex_lock(&migration_bitmap_mutex); > rcu_read_lock(); > Tested-by: He Rongguang <herongguang.he@huawei.com> > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination 2016-09-25 11:33 ` Herongguang (Stephen) @ 2016-09-26 7:15 ` Paolo Bonzini 0 siblings, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2016-09-26 7:15 UTC (permalink / raw) To: Herongguang (Stephen), qemu-devel, quintela, amit.shah Cc: arei.gonglei, Huangweidong (C) On 25/09/2016 13:33, Herongguang (Stephen) wrote: > Tested-by: He Rongguang <herongguang.he@huawei.com> Thanks! Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly 2016-09-14 7:55 [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination Herongguang (Stephen) 2016-09-14 9:05 ` Paolo Bonzini @ 2016-09-22 7:51 ` Herongguang (Stephen) 2016-09-22 9:29 ` Paolo Bonzini 2016-09-22 7:56 ` [Qemu-devel] [RFC/PATCH 3] kvm: fix events.flags (KVM_VCPUEVENT_VALID_SMM) overwritten by 0 Herongguang (Stephen) 2 siblings, 1 reply; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-22 7:51 UTC (permalink / raw) To: pbonzini, quintela, amit.shah, rkrcmar, kvm, herongguang.he Cc: qemu-devel, arei.gonglei, Huangweidong (C) After making memory consistent between source and destination (https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03069.html), there can still reproduce instruction emulation failure in destination side if migration when VM’s in grub stage: [2016-09-15 06:29:24] monitor_qapi_event_emit:478 {"timestamp": {"seconds": 1473892164, "microseconds": 99652}, "event": "RESUME"} KVM internal error. Suberror: 1 emulation failure EAX=000000b5 EBX=00008fc6 ECX=00005678 EDX=00000000 ESI=000f254b EDI=00000000 EBP=0000f958 ESP=000ee958 EIP=00008000 EFL=00010002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0 ES =0000 00000000 ffffffff 00809300 CS =a000 000a0000 ffffffff 00809300 SS =0000 00000000 ffffffff 00809300 DS =0000 00000000 ffffffff 00809300 FS =0000 00000000 ffffffff 00809300 GS =0000 00000000 ffffffff 00809300 LDT=0000 00000000 0000ffff 00008200 TR =0000 00000000 0000ffff 00008b00 GDT= 000f71a0 00000037 IDT= 00000000 00000000 CR0=00000010 CR2=00000000 CR3=00000000 CR4=00000000 DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 DR3=0000000000000000 DR6=00000000ffff0ff0 DR7=0000000000000400 EFER=0000000000000000 Code=ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff <ff> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [2016-09-15 06:29:24] virtio_set_status:712 virtio-blk device status is 7 that means DRIVER OK [2016-09-15 06:29:24] monitor_qapi_event_emit:478 {"timestamp": {"seconds": 1473892164, "microseconds": 106738}, "event": "STOP"} Note CS+EIP=0xA8000, which is within SMRAM, however SMM=0 is confusing! So I found that in kvm_put_vcpu_events, events.flags(KVM_VCPUEVENT_VALID_SMM) is overwritten by setting events.flags to 0, which is obvious mistaken. So it comes to patch 3. After patch 3, however it results kvm-kmod crash: [69328.761479] kvm [51353]: vcpu7, guest rIP: 0xfd31c unhandled wrmsr: 0xd1 data 0 [69337.406083] BUG: unable to handle kernel NULL pointer dereference at (null) [69337.414193] IP: [<ffffffffa056a59d>] gfn_to_rmap+0xcd/0xe0 [kvm] [69337.420357] PGD 0 [69337.422514] Oops: 0000 [#1] SMP [69337.425783] Modules linked in: rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache nfsd auth_rpcgss nfs_acl lockd grace sunrpc xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat xt_conntrack ipt_REJECT nf_reject_ipv4 bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter openvswitch nf_conntrack_ipv6 nf_nat_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_defrag_ipv6 nf_nat nf_conntrack libcrc32c ipmi_devintf ipmi_si ipmi_msghandler intel_rapl sb_edac edac_core x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel lrw kvm_intel gf128mul iTCO_wdt kvm glue_helper iTCO_vendor_support ablk_helper cryptd pcspkr sg mei_me mei ioatdma shpchp lpc_ich wmi mfd_core i2c_i801 vhost_net tun vhost macvtap macvlan ip_tables ext4 jbd2 mbcache sr_mod sd_mod cdrom isci libsas igb ahci libahci scsi_transport_sas crc32c_intel libata ptp pps_core serio_raw i2c_algo_bit megaraid_sas i2c_co! re dca d m_mod vfio_pci irqbypass vfio_virqfd vfio_iommu_type1 vfio [69337.524020] CPU: 3 PID: 51375 Comm: CPU 0/KVM Not tainted 4.7.0 #1 [69337.530315] Hardware name: Huawei Technologies Co., Ltd. Tecal RH2288 V2-8S/BC11SRSB1, BIOS RMIBV365 09/06/2013 [69337.540644] task: ffff88104bcc8000 ti: ffff881055650000 task.ti: ffff881055650000 [69337.548382] RIP: 0010:[<ffffffffa056a59d>] [<ffffffffa056a59d>] gfn_to_rmap+0xcd/0xe0 [kvm] [69337.557105] RSP: 0018:ffff881055653a88 EFLAGS: 00010202 [69337.562541] RAX: ffffc9000b3cf428 RBX: ffff88085624a538 RCX: 000000000000000c [69337.569788] RDX: ffff88083a6e2bd0 RSI: 00000000000000a7 RDI: 00000000000000a0 [69337.577036] RBP: ffff881055653a88 R08: 000000000000000c R09: ffffc9000b3cf008 [69337.584283] R10: ffffc9000b3cf000 R11: 0000000000000037 R12: ffff8810569d0000 [69337.591530] R13: 0000000000000000 R14: 00000000000000a7 R15: ffff8808d624a538 [69337.598777] FS: 00007faeb900d700(0000) GS:ffff88085e0c0000(0000) knlGS:0000000000000000 [69337.607116] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [69337.612978] CR2: 0000000000000000 CR3: 000000104c203000 CR4: 00000000000426e0 [69337.620225] Stack: [69337.622369] ffff881055653af0 ffffffffa056b28d ffffffff00000000 0000000000000001 [69337.630059] 0000000000000001 00007fadb50a7000 0100000000000000 ffff88083a6e2bd0 [69337.637751] ffff8810569d0000 0000000000000001 000ffffffffff000 ffff880000000000 [69337.645442] Call Trace: [69337.648029] [<ffffffffa056b28d>] mmu_set_spte+0x19d/0x280 [kvm] [69337.654166] [<ffffffffa056f56b>] __direct_map.part.122+0x19b/0x210 [kvm] [69337.661078] [<ffffffffa056fa29>] tdp_page_fault+0x239/0x260 [kvm] [69337.667385] [<ffffffffa0568bf0>] kvm_mmu_page_fault+0x60/0xf0 [kvm] [69337.673856] [<ffffffffa03f8656>] handle_ept_violation+0x96/0x190 [kvm_intel] [69337.681106] [<ffffffffa03ff740>] vmx_handle_exit+0x1f0/0xc30 [kvm_intel] [69337.688019] [<ffffffffa055d9ce>] vcpu_enter_guest+0x90e/0x1190 [kvm] [69337.694585] [<ffffffffa056444d>] kvm_arch_vcpu_ioctl_run+0xcd/0x3f0 [kvm] [69337.701581] [<ffffffffa0549fb5>] kvm_vcpu_ioctl+0x295/0x610 [kvm] [69337.707880] [<ffffffff8110b6cc>] ? do_futex+0x11c/0x530 [69337.713314] [<ffffffff8122e436>] do_vfs_ioctl+0xa6/0x5c0 [69337.718833] [<ffffffff81003276>] ? do_audit_syscall_entry+0x66/0x70 [69337.725303] [<ffffffff810037cf>] ? syscall_trace_enter_phase1+0x11f/0x140 [69337.732291] [<ffffffff8122e9c9>] SyS_ioctl+0x79/0x90 [69337.737470] [<ffffffff81003b12>] do_syscall_64+0x62/0x110 [69337.743085] [<ffffffff816c88a1>] entry_SYSCALL64_slow_path+0x25/0x25 [69337.749641] Code: 8b 38 0f b6 52 28 5d 83 e2 0f 83 ea 01 8d 0c d2 48 63 d2 48 8b 44 d0 18 48 d3 ee 48 d3 ef 48 29 fe 48 8d 04 f0 c3 8d 48 01 eb 80 <48> 8b 3c 25 00 00 00 00 31 c0 eb cb 0f 1f 80 00 00 00 00 66 66 [69337.769707] RIP [<ffffffffa056a59d>] gfn_to_rmap+0xcd/0xe0 [kvm] [69337.775938] RSP <ffff881055653a88> [69337.779552] CR2: 0000000000000000 [69337.783467] ---[ end trace 5350f10b8de91e83 ]--- [69337.843957] Kernel panic - not syncing: Fatal exception [69337.849430] Kernel Offset: disabled I found that when kernel crashes, in tdp_page_fault ->__direct_map ->mmu_set_spte ->rmap_add->gfn_to_rmap-> __gfn_to_memslot &__gfn_to_rmap, in __gfn_to_memslot returned NULL, and __gfn_to_rmap uses this NULL pointer (slot) without checking, which is bad. After some investigation, I found when kernel crashes, gfn is 0xA7, in SMRAM region again. And at this time, kvm_memslots_for_spte_role is false, however actually (vcpu->arch.hflags & HF_SMM_MASK) is true! So I think there is some lacking in kvm-kmod’s kvm_vcpu_ioctl_x86_set_vcpu_events that handles KVM_VCPUEVENT_VALID_SMM. I tried following patch, it seems works fine. Do you think this patch is appropriate or not enough? Thanks. diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 19f9f9e..f39e839 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3013,8 +3013,10 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, vcpu->arch.apic->sipi_vector = events->sipi_vector; if (events->flags & KVM_VCPUEVENT_VALID_SMM) { - if (events->smi.smm) + if (events->smi.smm) { vcpu->arch.hflags |= HF_SMM_MASK; + kvm_mmu_reset_context(vcpu); + } else vcpu->arch.hflags &= ~HF_SMM_MASK; vcpu->arch.smi_pending = events->smi.pending; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly 2016-09-22 7:51 ` [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly Herongguang (Stephen) @ 2016-09-22 9:29 ` Paolo Bonzini 2016-09-22 13:19 ` Herongguang (Stephen) 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2016-09-22 9:29 UTC (permalink / raw) To: Herongguang (Stephen), quintela, amit.shah, rkrcmar, kvm Cc: qemu-devel, arei.gonglei, Huangweidong (C) On 22/09/2016 09:51, Herongguang (Stephen) wrote: > After making memory consistent between source and destination > (https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03069.html), > there can > still reproduce instruction emulation failure in destination side if > migration when VM’s in grub stage: Hi! Did you follow up on that patch, by the way? > So I think there is some lacking in kvm-kmod’s > kvm_vcpu_ioctl_x86_set_vcpu_events that handles KVM_VCPUEVENT_VALID_SMM. > I tried following patch, > it seems works fine. > > Do you think this patch is appropriate or not enough? Thanks. Yes. I would just call kvm_mmu_reset_context unconditionally at the end of kvm_vcpu_iocyl_x86_set_x86_vcpu_events. Please send this patch as non-RFC. Patch 3 is also okay, please send it separately. Thanks, Paolo > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 19f9f9e..f39e839 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3013,8 +3013,10 @@ static int > kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, > vcpu->arch.apic->sipi_vector = events->sipi_vector; > > if (events->flags & KVM_VCPUEVENT_VALID_SMM) { > - if (events->smi.smm) > + if (events->smi.smm) { > vcpu->arch.hflags |= HF_SMM_MASK; > + kvm_mmu_reset_context(vcpu); > + } > else > vcpu->arch.hflags &= ~HF_SMM_MASK; > vcpu->arch.smi_pending = events->smi.pending; > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly 2016-09-22 9:29 ` Paolo Bonzini @ 2016-09-22 13:19 ` Herongguang (Stephen) 0 siblings, 0 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-22 13:19 UTC (permalink / raw) To: Paolo Bonzini, quintela, amit.shah, rkrcmar, kvm Cc: qemu-devel, arei.gonglei, Huangweidong (C) On 2016/9/22 17:29, Paolo Bonzini wrote: > > > On 22/09/2016 09:51, Herongguang (Stephen) wrote: >> After making memory consistent between source and destination >> (https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg03069.html), >> there can >> still reproduce instruction emulation failure in destination side if >> migration when VM’s in grub stage: > > Hi! Did you follow up on that patch, by the way? Yes, I have some concern, see that post. > >> So I think there is some lacking in kvm-kmod’s >> kvm_vcpu_ioctl_x86_set_vcpu_events that handles KVM_VCPUEVENT_VALID_SMM. >> I tried following patch, >> it seems works fine. >> >> Do you think this patch is appropriate or not enough? Thanks. > > Yes. I would just call kvm_mmu_reset_context unconditionally at the end > of kvm_vcpu_iocyl_x86_set_x86_vcpu_events. Please send this patch as > non-RFC. > > Patch 3 is also okay, please send it separately. Ok, I will test and post it tomorrow, thanks! > > Thanks, > > Paolo > >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 19f9f9e..f39e839 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -3013,8 +3013,10 @@ static int >> kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu, >> vcpu->arch.apic->sipi_vector = events->sipi_vector; >> >> if (events->flags & KVM_VCPUEVENT_VALID_SMM) { >> - if (events->smi.smm) >> + if (events->smi.smm) { >> vcpu->arch.hflags |= HF_SMM_MASK; >> + kvm_mmu_reset_context(vcpu); >> + } >> else >> vcpu->arch.hflags &= ~HF_SMM_MASK; >> vcpu->arch.smi_pending = events->smi.pending; >> >> > > . > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [RFC/PATCH 3] kvm: fix events.flags (KVM_VCPUEVENT_VALID_SMM) overwritten by 0 2016-09-14 7:55 [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination Herongguang (Stephen) 2016-09-14 9:05 ` Paolo Bonzini 2016-09-22 7:51 ` [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly Herongguang (Stephen) @ 2016-09-22 7:56 ` Herongguang (Stephen) 2 siblings, 0 replies; 14+ messages in thread From: Herongguang (Stephen) @ 2016-09-22 7:56 UTC (permalink / raw) To: qemu-devel, pbonzini, quintela, amit.shah, rth, ehabkost, mtosatti Cc: arei.gonglei, Huangweidong (C) Fix events.flags (KVM_VCPUEVENT_VALID_SMM) overwritten by 0. Signed-off-by: He Rongguang <herongguang.he@huawei.com> --- Note without patch 2, this would result in kvm-kmod crash, as described in patch 2 --- target-i386/kvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target-i386/kvm.c b/target-i386/kvm.c index d1a25c5..7db33d2 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -2456,6 +2456,7 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level) events.sipi_vector = env->sipi_vector; + events.flags = 0; if (has_msr_smbase) { events.smi.smm = !!(env->hflags & HF_SMM_MASK); events.smi.smm_inside_nmi = !!(env->hflags2 & HF2_SMM_INSIDE_NMI_MASK); @@ -2474,7 +2475,6 @@ static int kvm_put_vcpu_events(X86CPU *cpu, int level) events.flags |= KVM_VCPUEVENT_VALID_SMM; } - events.flags = 0; if (level >= KVM_PUT_RESET_STATE) { events.flags |= KVM_VCPUEVENT_VALID_NMI_PENDING | KVM_VCPUEVENT_VALID_SIPI_VECTOR; -- 1.8.3.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-09-26 7:16 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-14 7:55 [Qemu-devel] [RFC/PATCH] migration: SMRAM dirty bitmap not fetched from kvm-kmod and not send to destination Herongguang (Stephen) 2016-09-14 9:05 ` Paolo Bonzini 2016-09-22 13:16 ` Herongguang (Stephen) 2016-09-23 1:11 ` Herongguang (Stephen) 2016-09-23 7:17 ` Paolo Bonzini 2016-09-23 8:51 ` Herongguang (Stephen) 2016-09-23 8:59 ` Paolo Bonzini 2016-09-23 9:14 ` Herongguang (Stephen) 2016-09-25 11:33 ` Herongguang (Stephen) 2016-09-26 7:15 ` Paolo Bonzini 2016-09-22 7:51 ` [Qemu-devel] [RFC/PATCH 2] kvm: x86: handle KVM_SET_VCPU_EVENTS/KVM_VCPUEVENT_VALID_SMM properly Herongguang (Stephen) 2016-09-22 9:29 ` Paolo Bonzini 2016-09-22 13:19 ` Herongguang (Stephen) 2016-09-22 7:56 ` [Qemu-devel] [RFC/PATCH 3] kvm: fix events.flags (KVM_VCPUEVENT_VALID_SMM) overwritten by 0 Herongguang (Stephen)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).