| Message ID | 1349280245-16341-4-git-send-email-avi@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 3, 2012 at 4:03 PM, Avi Kivity <avi@redhat.com> wrote: > The construct > > if (address_space == get_system_memory()) { > // memory thing > } else { > // io thing > } > > fails if we have more than two address spaces. Use a separate listener > for memory and I/O, and utilize MemoryListener's address space filtering to > fix this. > > Signed-off-by: Avi Kivity <avi@redhat.com> > --- > kvm-all.c | 83 +++++++++++++++++++++++++++++++++------------------------------ > 1 file changed, 44 insertions(+), 39 deletions(-) > > diff --git a/kvm-all.c b/kvm-all.c > index 92a7137..c69e012 100644 > --- a/kvm-all.c > +++ b/kvm-all.c > @@ -755,9 +755,16 @@ static void kvm_log_global_stop(struct MemoryListener *listener) > assert(r >= 0); > } > > -static void kvm_mem_ioeventfd_add(MemoryRegionSection *section, > - bool match_data, uint64_t data, int fd) > +static void kvm_log_nop(struct MemoryListener *listener) > { > +} > + > +static void kvm_mem_ioeventfd_add(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, > + EventNotifier *e) > +{ > + int fd = event_notifier_get_fd(e); > int r; > > assert(match_data && section->size <= 8); > @@ -769,9 +776,12 @@ static void kvm_mem_ioeventfd_add(MemoryRegionSection *section, > } > } > > -static void kvm_mem_ioeventfd_del(MemoryRegionSection *section, > - bool match_data, uint64_t data, int fd) > +static void kvm_mem_ioeventfd_del(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, > + EventNotifier *e) > { > + int fd = event_notifier_get_fd(e); > int r; > > r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space, > @@ -781,9 +791,12 @@ static void kvm_mem_ioeventfd_del(MemoryRegionSection *section, > } > } > > -static void kvm_io_ioeventfd_add(MemoryRegionSection *section, > - bool match_data, uint64_t data, int fd) > +static void kvm_io_ioeventfd_add(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, > + EventNotifier *e) > { > + int fd = event_notifier_get_fd(e); > int r; > > assert(match_data && section->size == 2); > @@ -795,10 +808,13 @@ static void kvm_io_ioeventfd_add(MemoryRegionSection *section, > } > } > > -static void kvm_io_ioeventfd_del(MemoryRegionSection *section, > - bool match_data, uint64_t data, int fd) > +static void kvm_io_ioeventfd_del(MemoryListener *listener, > + MemoryRegionSection *section, > + bool match_data, uint64_t data, > + EventNotifier *e) > > { > + int fd = event_notifier_get_fd(e); > int r; > > r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space, > @@ -808,34 +824,6 @@ static void kvm_io_ioeventfd_del(MemoryRegionSection *section, > } > } > > -static void kvm_eventfd_add(MemoryListener *listener, > - MemoryRegionSection *section, > - bool match_data, uint64_t data, > - EventNotifier *e) > -{ > - if (section->address_space == get_system_memory()) { > - kvm_mem_ioeventfd_add(section, match_data, data, > - event_notifier_get_fd(e)); > - } else { > - kvm_io_ioeventfd_add(section, match_data, data, > - event_notifier_get_fd(e)); > - } > -} > - > -static void kvm_eventfd_del(MemoryListener *listener, > - MemoryRegionSection *section, > - bool match_data, uint64_t data, > - EventNotifier *e) > -{ > - if (section->address_space == get_system_memory()) { > - kvm_mem_ioeventfd_del(section, match_data, data, > - event_notifier_get_fd(e)); > - } else { > - kvm_io_ioeventfd_del(section, match_data, data, > - event_notifier_get_fd(e)); > - } > -} > - > static MemoryListener kvm_memory_listener = { > .begin = kvm_begin, > .commit = kvm_commit, > @@ -847,8 +835,24 @@ static void kvm_eventfd_del(MemoryListener *listener, > .log_sync = kvm_log_sync, > .log_global_start = kvm_log_global_start, > .log_global_stop = kvm_log_global_stop, > - .eventfd_add = kvm_eventfd_add, > - .eventfd_del = kvm_eventfd_del, > + .eventfd_add = kvm_mem_ioeventfd_add, > + .eventfd_del = kvm_mem_ioeventfd_del, > + .priority = 10, > +}; > + > +static MemoryListener kvm_io_listener = { const > + .begin = kvm_begin, > + .commit = kvm_commit, > + .region_add = kvm_region_nop, > + .region_del = kvm_region_nop, > + .region_nop = kvm_region_nop, > + .log_start = kvm_region_nop, > + .log_stop = kvm_region_nop, > + .log_sync = kvm_region_nop, > + .log_global_start = kvm_log_nop, > + .log_global_stop = kvm_log_nop, > + .eventfd_add = kvm_io_ioeventfd_add, > + .eventfd_del = kvm_io_ioeventfd_del, > .priority = 10, > }; > > @@ -1401,7 +1405,8 @@ int kvm_init(void) > } > > kvm_state = s; > - memory_listener_register(&kvm_memory_listener, NULL); > + memory_listener_register(&kvm_memory_listener, get_system_memory()); > + memory_listener_register(&kvm_io_listener, get_system_io()); > > s->many_ioeventfds = kvm_check_many_ioeventfds(); > > -- > 1.7.12 >
On 10/03/2012 10:16 PM, Blue Swirl wrote: > > +static MemoryListener kvm_io_listener = { > > const > There is a list link field in there. It's a mixed data/ops structure (perhaps we should make it a traditional ->ops-> pointer).
On Thu, Oct 4, 2012 at 6:33 AM, Avi Kivity <avi@redhat.com> wrote: > On 10/03/2012 10:16 PM, Blue Swirl wrote: >> > +static MemoryListener kvm_io_listener = { >> >> const >> > > There is a list link field in there. It's a mixed data/ops structure > (perhaps we should make it a traditional ->ops-> pointer). I grepped for MemoryListener and in hw/xen_pt.c 'const' already exists, so it looked OK but the code actually copies the structure to writable memory. > > -- > I have a truly marvellous patch that fixes the bug which this > signature is too narrow to contain. >
On 10/04/2012 06:44 PM, Blue Swirl wrote: > On Thu, Oct 4, 2012 at 6:33 AM, Avi Kivity <avi@redhat.com> wrote: >> On 10/03/2012 10:16 PM, Blue Swirl wrote: >>> > +static MemoryListener kvm_io_listener = { >>> >>> const >>> >> >> There is a list link field in there. It's a mixed data/ops structure >> (perhaps we should make it a traditional ->ops-> pointer). > > I grepped for MemoryListener and in hw/xen_pt.c 'const' already > exists, so it looked OK but the code actually copies the structure to > writable memory. Yes. We should probably switch to the standard object->ops->callback pattern. I'll write a patch after this series is merged.
diff --git a/kvm-all.c b/kvm-all.c index 92a7137..c69e012 100644 --- a/kvm-all.c +++ b/kvm-all.c @@ -755,9 +755,16 @@ static void kvm_log_global_stop(struct MemoryListener *listener) assert(r >= 0); } -static void kvm_mem_ioeventfd_add(MemoryRegionSection *section, - bool match_data, uint64_t data, int fd) +static void kvm_log_nop(struct MemoryListener *listener) { +} + +static void kvm_mem_ioeventfd_add(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, + EventNotifier *e) +{ + int fd = event_notifier_get_fd(e); int r; assert(match_data && section->size <= 8); @@ -769,9 +776,12 @@ static void kvm_mem_ioeventfd_add(MemoryRegionSection *section, } } -static void kvm_mem_ioeventfd_del(MemoryRegionSection *section, - bool match_data, uint64_t data, int fd) +static void kvm_mem_ioeventfd_del(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, + EventNotifier *e) { + int fd = event_notifier_get_fd(e); int r; r = kvm_set_ioeventfd_mmio(fd, section->offset_within_address_space, @@ -781,9 +791,12 @@ static void kvm_mem_ioeventfd_del(MemoryRegionSection *section, } } -static void kvm_io_ioeventfd_add(MemoryRegionSection *section, - bool match_data, uint64_t data, int fd) +static void kvm_io_ioeventfd_add(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, + EventNotifier *e) { + int fd = event_notifier_get_fd(e); int r; assert(match_data && section->size == 2); @@ -795,10 +808,13 @@ static void kvm_io_ioeventfd_add(MemoryRegionSection *section, } } -static void kvm_io_ioeventfd_del(MemoryRegionSection *section, - bool match_data, uint64_t data, int fd) +static void kvm_io_ioeventfd_del(MemoryListener *listener, + MemoryRegionSection *section, + bool match_data, uint64_t data, + EventNotifier *e) { + int fd = event_notifier_get_fd(e); int r; r = kvm_set_ioeventfd_pio_word(fd, section->offset_within_address_space, @@ -808,34 +824,6 @@ static void kvm_io_ioeventfd_del(MemoryRegionSection *section, } } -static void kvm_eventfd_add(MemoryListener *listener, - MemoryRegionSection *section, - bool match_data, uint64_t data, - EventNotifier *e) -{ - if (section->address_space == get_system_memory()) { - kvm_mem_ioeventfd_add(section, match_data, data, - event_notifier_get_fd(e)); - } else { - kvm_io_ioeventfd_add(section, match_data, data, - event_notifier_get_fd(e)); - } -} - -static void kvm_eventfd_del(MemoryListener *listener, - MemoryRegionSection *section, - bool match_data, uint64_t data, - EventNotifier *e) -{ - if (section->address_space == get_system_memory()) { - kvm_mem_ioeventfd_del(section, match_data, data, - event_notifier_get_fd(e)); - } else { - kvm_io_ioeventfd_del(section, match_data, data, - event_notifier_get_fd(e)); - } -} - static MemoryListener kvm_memory_listener = { .begin = kvm_begin, .commit = kvm_commit, @@ -847,8 +835,24 @@ static void kvm_eventfd_del(MemoryListener *listener, .log_sync = kvm_log_sync, .log_global_start = kvm_log_global_start, .log_global_stop = kvm_log_global_stop, - .eventfd_add = kvm_eventfd_add, - .eventfd_del = kvm_eventfd_del, + .eventfd_add = kvm_mem_ioeventfd_add, + .eventfd_del = kvm_mem_ioeventfd_del, + .priority = 10, +}; + +static MemoryListener kvm_io_listener = { + .begin = kvm_begin, + .commit = kvm_commit, + .region_add = kvm_region_nop, + .region_del = kvm_region_nop, + .region_nop = kvm_region_nop, + .log_start = kvm_region_nop, + .log_stop = kvm_region_nop, + .log_sync = kvm_region_nop, + .log_global_start = kvm_log_nop, + .log_global_stop = kvm_log_nop, + .eventfd_add = kvm_io_ioeventfd_add, + .eventfd_del = kvm_io_ioeventfd_del, .priority = 10, }; @@ -1401,7 +1405,8 @@ int kvm_init(void) } kvm_state = s; - memory_listener_register(&kvm_memory_listener, NULL); + memory_listener_register(&kvm_memory_listener, get_system_memory()); + memory_listener_register(&kvm_io_listener, get_system_io()); s->many_ioeventfds = kvm_check_many_ioeventfds();
The construct if (address_space == get_system_memory()) { // memory thing } else { // io thing } fails if we have more than two address spaces. Use a separate listener for memory and I/O, and utilize MemoryListener's address space filtering to fix this. Signed-off-by: Avi Kivity <avi@redhat.com> --- kvm-all.c | 83 +++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 44 insertions(+), 39 deletions(-)