| Message ID | 20200212134254.11073-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ram blocks with resizable anonymous allocations under POSIX | expand |
On Wed, Feb 12, 2020 at 02:42:39PM +0100, David Hildenbrand wrote: > Factor it out into common code when a new notifier is registered, just > as done with the memory region notifier. This allows us to have the > logic about how to process existing ram blocks at a central place (which > will be extended soon). > > Just like when adding a new ram block, we have to register the max_length > for now. We don't have a way to get notified about resizes yet, and some > memory would not be mapped when growing the ram block. > > Note: Currently, ram blocks are only "fake resized". All memory > (max_length) is accessible. > > We can get rid of a bunch of functions in stubs/ram-block.c . Print the > warning from inside qemu_vfio_ram_block_added(). > > Cc: Richard Henderson <rth@twiddle.net> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > exec.c | 5 +++++ > hw/core/numa.c | 14 ++++++++++++++ > include/exec/cpu-common.h | 1 + > stubs/ram-block.c | 20 -------------------- > util/vfio-helpers.c | 28 +++++++--------------------- > 5 files changed, 27 insertions(+), 41 deletions(-) > > diff --git a/exec.c b/exec.c > index 67e520d18e..05cfe868ab 100644 > --- a/exec.c > +++ b/exec.c > @@ -2017,6 +2017,11 @@ ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) > return rb->used_length; > } > > +ram_addr_t qemu_ram_get_max_length(RAMBlock *rb) > +{ > + return rb->max_length; > +} > + > bool qemu_ram_is_shared(RAMBlock *rb) > { > return rb->flags & RAM_SHARED; > diff --git a/hw/core/numa.c b/hw/core/numa.c > index 0d1b4be76a..6599c69e05 100644 > --- a/hw/core/numa.c > +++ b/hw/core/numa.c > @@ -899,9 +899,23 @@ void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms) > } > } > > +static int ram_block_notify_add_single(RAMBlock *rb, void *opaque) > +{ > + const ram_addr_t max_size = qemu_ram_get_max_length(rb); > + void *host = qemu_ram_get_host_addr(rb); > + RAMBlockNotifier *notifier = opaque; > + > + if (host) { > + notifier->ram_block_added(notifier, host, max_size); > + } > + return 0; > +} > + > void ram_block_notifier_add(RAMBlockNotifier *n) > { > QLIST_INSERT_HEAD(&ram_list.ramblock_notifiers, n, next); > + /* Notify about all existing ram blocks. */ > + qemu_ram_foreach_block(ram_block_notify_add_single, n); > } > > void ram_block_notifier_remove(RAMBlockNotifier *n) > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > index 81753bbb34..9760ac9068 100644 > --- a/include/exec/cpu-common.h > +++ b/include/exec/cpu-common.h > @@ -59,6 +59,7 @@ const char *qemu_ram_get_idstr(RAMBlock *rb); > void *qemu_ram_get_host_addr(RAMBlock *rb); > ram_addr_t qemu_ram_get_offset(RAMBlock *rb); > ram_addr_t qemu_ram_get_used_length(RAMBlock *rb); > +ram_addr_t qemu_ram_get_max_length(RAMBlock *rb); > bool qemu_ram_is_shared(RAMBlock *rb); > bool qemu_ram_is_uf_zeroable(RAMBlock *rb); > void qemu_ram_set_uf_zeroable(RAMBlock *rb); > diff --git a/stubs/ram-block.c b/stubs/ram-block.c > index 73c0a3ee08..10855b52dd 100644 > --- a/stubs/ram-block.c > +++ b/stubs/ram-block.c > @@ -2,21 +2,6 @@ > #include "exec/ramlist.h" > #include "exec/cpu-common.h" > > -void *qemu_ram_get_host_addr(RAMBlock *rb) > -{ > - return 0; > -} > - > -ram_addr_t qemu_ram_get_offset(RAMBlock *rb) > -{ > - return 0; > -} > - > -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) > -{ > - return 0; > -} Maybe put into another patch? Actually I'm thinking whether it would worth to do... They're still declared in include/exec/cpu-common.h, so logically who includes the header but linked against stubs can still call this function. So keeping them there still make sense to me. > - > void ram_block_notifier_add(RAMBlockNotifier *n) > { > } > @@ -24,8 +9,3 @@ void ram_block_notifier_add(RAMBlockNotifier *n) > void ram_block_notifier_remove(RAMBlockNotifier *n) > { > } > - > -int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque) > -{ > - return 0; > -} > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c > index 813f7ec564..71e02e7f35 100644 > --- a/util/vfio-helpers.c > +++ b/util/vfio-helpers.c > @@ -376,8 +376,13 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, > void *host, size_t size) > { > QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier); > + int ret; > + > trace_qemu_vfio_ram_block_added(s, host, size); > - qemu_vfio_dma_map(s, host, size, false, NULL); > + ret = qemu_vfio_dma_map(s, host, size, false, NULL); > + if (ret) { > + error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret); > + } Irrelevant change (another patch)? > } > > static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n, > @@ -390,33 +395,14 @@ static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n, > } > } > > -static int qemu_vfio_init_ramblock(RAMBlock *rb, void *opaque) > -{ > - void *host_addr = qemu_ram_get_host_addr(rb); > - ram_addr_t length = qemu_ram_get_used_length(rb); > - int ret; > - QEMUVFIOState *s = opaque; > - > - if (!host_addr) { > - return 0; > - } > - ret = qemu_vfio_dma_map(s, host_addr, length, false, NULL); > - if (ret) { > - fprintf(stderr, "qemu_vfio_init_ramblock: failed %p %" PRId64 "\n", > - host_addr, (uint64_t)length); > - } > - return 0; > -} > - > static void qemu_vfio_open_common(QEMUVFIOState *s) > { > qemu_mutex_init(&s->lock); > s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added; > s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed; > - ram_block_notifier_add(&s->ram_notifier); > s->low_water_mark = QEMU_VFIO_IOVA_MIN; > s->high_water_mark = QEMU_VFIO_IOVA_MAX; > - qemu_ram_foreach_block(qemu_vfio_init_ramblock, s); > + ram_block_notifier_add(&s->ram_notifier); Pure question: this looks like a good improvement, however do you know why HAX and SEV do not need to init ramblock? Thanks, > } > > /** > -- > 2.24.1 > >
On 18.02.20 23:00, Peter Xu wrote: > On Wed, Feb 12, 2020 at 02:42:39PM +0100, David Hildenbrand wrote: >> Factor it out into common code when a new notifier is registered, just >> as done with the memory region notifier. This allows us to have the >> logic about how to process existing ram blocks at a central place (which >> will be extended soon). >> >> Just like when adding a new ram block, we have to register the max_length >> for now. We don't have a way to get notified about resizes yet, and some >> memory would not be mapped when growing the ram block. >> >> Note: Currently, ram blocks are only "fake resized". All memory >> (max_length) is accessible. >> >> We can get rid of a bunch of functions in stubs/ram-block.c . Print the >> warning from inside qemu_vfio_ram_block_added(). [...] >> #include "exec/ramlist.h" >> #include "exec/cpu-common.h" >> >> -void *qemu_ram_get_host_addr(RAMBlock *rb) >> -{ >> - return 0; >> -} >> - >> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb) >> -{ >> - return 0; >> -} >> - >> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) >> -{ >> - return 0; >> -} > > Maybe put into another patch? > > Actually I'm thinking whether it would worth to do... They're still > declared in include/exec/cpu-common.h, so logically who includes the > header but linked against stubs can still call this function. So > keeping them there still make sense to me. Why keep dead code around? If you look closely, the stubs really only contain what's strictly necessary to make current code compile, not any available ramblock related function. I don't see a good reason for a separate patch either (after all, we're removing the last users in this patch), but if more people agree, I can move it to a separate patch. [...] >> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c >> index 813f7ec564..71e02e7f35 100644 >> --- a/util/vfio-helpers.c >> +++ b/util/vfio-helpers.c >> @@ -376,8 +376,13 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, >> void *host, size_t size) >> { >> QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier); >> + int ret; >> + >> trace_qemu_vfio_ram_block_added(s, host, size); >> - qemu_vfio_dma_map(s, host, size, false, NULL); >> + ret = qemu_vfio_dma_map(s, host, size, false, NULL); >> + if (ret) { >> + error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret); >> + } > > Irrelevant change (another patch)? This is the error that was printed in qemu_vfio_init_ramblock(). Not moving it in this patch would mean we would stop printing the error. [...] >> - >> static void qemu_vfio_open_common(QEMUVFIOState *s) >> { >> qemu_mutex_init(&s->lock); >> s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added; >> s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed; >> - ram_block_notifier_add(&s->ram_notifier); >> s->low_water_mark = QEMU_VFIO_IOVA_MIN; >> s->high_water_mark = QEMU_VFIO_IOVA_MAX; >> - qemu_ram_foreach_block(qemu_vfio_init_ramblock, s); >> + ram_block_notifier_add(&s->ram_notifier); > > Pure question: this looks like a good improvement, however do you know > why HAX and SEV do not need to init ramblock? They register very early (e.g., at accel init time), before any ram blocks are added. Thanks for your review!
On 19.02.20 09:43, David Hildenbrand wrote: > On 18.02.20 23:00, Peter Xu wrote: >> On Wed, Feb 12, 2020 at 02:42:39PM +0100, David Hildenbrand wrote: >>> Factor it out into common code when a new notifier is registered, just >>> as done with the memory region notifier. This allows us to have the >>> logic about how to process existing ram blocks at a central place (which >>> will be extended soon). >>> >>> Just like when adding a new ram block, we have to register the max_length >>> for now. We don't have a way to get notified about resizes yet, and some >>> memory would not be mapped when growing the ram block. >>> >>> Note: Currently, ram blocks are only "fake resized". All memory >>> (max_length) is accessible. >>> >>> We can get rid of a bunch of functions in stubs/ram-block.c . Print the >>> warning from inside qemu_vfio_ram_block_added(). > > [...] > >>> #include "exec/ramlist.h" >>> #include "exec/cpu-common.h" >>> >>> -void *qemu_ram_get_host_addr(RAMBlock *rb) >>> -{ >>> - return 0; >>> -} >>> - >>> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb) >>> -{ >>> - return 0; >>> -} >>> - >>> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) >>> -{ >>> - return 0; >>> -} >> >> Maybe put into another patch? >> >> Actually I'm thinking whether it would worth to do... They're still >> declared in include/exec/cpu-common.h, so logically who includes the >> header but linked against stubs can still call this function. So >> keeping them there still make sense to me. > > Why keep dead code around? If you look closely, the stubs really only > contain what's strictly necessary to make current code compile, not any > available ramblock related function. > > I don't see a good reason for a separate patch either (after all, we're > removing the last users in this patch), but if more people agree, I can > move it to a separate patch. FWIW, moved it to a separate patch :)
On Wed, Feb 19, 2020 at 09:43:02AM +0100, David Hildenbrand wrote: > On 18.02.20 23:00, Peter Xu wrote: > > On Wed, Feb 12, 2020 at 02:42:39PM +0100, David Hildenbrand wrote: > >> Factor it out into common code when a new notifier is registered, just > >> as done with the memory region notifier. This allows us to have the > >> logic about how to process existing ram blocks at a central place (which > >> will be extended soon). > >> > >> Just like when adding a new ram block, we have to register the max_length > >> for now. We don't have a way to get notified about resizes yet, and some > >> memory would not be mapped when growing the ram block. > >> > >> Note: Currently, ram blocks are only "fake resized". All memory > >> (max_length) is accessible. > >> > >> We can get rid of a bunch of functions in stubs/ram-block.c . Print the > >> warning from inside qemu_vfio_ram_block_added(). > > [...] > > >> #include "exec/ramlist.h" > >> #include "exec/cpu-common.h" > >> > >> -void *qemu_ram_get_host_addr(RAMBlock *rb) > >> -{ > >> - return 0; > >> -} > >> - > >> -ram_addr_t qemu_ram_get_offset(RAMBlock *rb) > >> -{ > >> - return 0; > >> -} > >> - > >> -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) > >> -{ > >> - return 0; > >> -} > > > > Maybe put into another patch? > > > > Actually I'm thinking whether it would worth to do... They're still > > declared in include/exec/cpu-common.h, so logically who includes the > > header but linked against stubs can still call this function. So > > keeping them there still make sense to me. > > Why keep dead code around? If you look closely, the stubs really only > contain what's strictly necessary to make current code compile, not any > available ramblock related function. Seems correct. Then it's fine. > > I don't see a good reason for a separate patch either (after all, we're > removing the last users in this patch), but if more people agree, I can > move it to a separate patch. > [...] > > >> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c > >> index 813f7ec564..71e02e7f35 100644 > >> --- a/util/vfio-helpers.c > >> +++ b/util/vfio-helpers.c > >> @@ -376,8 +376,13 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, > >> void *host, size_t size) > >> { > >> QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier); > >> + int ret; > >> + > >> trace_qemu_vfio_ram_block_added(s, host, size); > >> - qemu_vfio_dma_map(s, host, size, false, NULL); > >> + ret = qemu_vfio_dma_map(s, host, size, false, NULL); > >> + if (ret) { > >> + error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret); > >> + } > > > > Irrelevant change (another patch)? > > This is the error that was printed in qemu_vfio_init_ramblock(). Not > moving it in this patch would mean we would stop printing the error. > [...] > > >> - > >> static void qemu_vfio_open_common(QEMUVFIOState *s) > >> { > >> qemu_mutex_init(&s->lock); > >> s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added; > >> s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed; > >> - ram_block_notifier_add(&s->ram_notifier); > >> s->low_water_mark = QEMU_VFIO_IOVA_MIN; > >> s->high_water_mark = QEMU_VFIO_IOVA_MAX; > >> - qemu_ram_foreach_block(qemu_vfio_init_ramblock, s); > >> + ram_block_notifier_add(&s->ram_notifier); > > > > Pure question: this looks like a good improvement, however do you know > > why HAX and SEV do not need to init ramblock? > > They register very early (e.g., at accel init time), before any ram > blocks are added. That's what I thought but I did feel like it's tricky (not anything about this patch, though). Say, I don't see how that's guaranteed that accel init will always happen before creating any ramblocks. Anyway, your patch looks good from that point of view. :) Thanks,
diff --git a/exec.c b/exec.c index 67e520d18e..05cfe868ab 100644 --- a/exec.c +++ b/exec.c @@ -2017,6 +2017,11 @@ ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) return rb->used_length; } +ram_addr_t qemu_ram_get_max_length(RAMBlock *rb) +{ + return rb->max_length; +} + bool qemu_ram_is_shared(RAMBlock *rb) { return rb->flags & RAM_SHARED; diff --git a/hw/core/numa.c b/hw/core/numa.c index 0d1b4be76a..6599c69e05 100644 --- a/hw/core/numa.c +++ b/hw/core/numa.c @@ -899,9 +899,23 @@ void query_numa_node_mem(NumaNodeMem node_mem[], MachineState *ms) } } +static int ram_block_notify_add_single(RAMBlock *rb, void *opaque) +{ + const ram_addr_t max_size = qemu_ram_get_max_length(rb); + void *host = qemu_ram_get_host_addr(rb); + RAMBlockNotifier *notifier = opaque; + + if (host) { + notifier->ram_block_added(notifier, host, max_size); + } + return 0; +} + void ram_block_notifier_add(RAMBlockNotifier *n) { QLIST_INSERT_HEAD(&ram_list.ramblock_notifiers, n, next); + /* Notify about all existing ram blocks. */ + qemu_ram_foreach_block(ram_block_notify_add_single, n); } void ram_block_notifier_remove(RAMBlockNotifier *n) diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index 81753bbb34..9760ac9068 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -59,6 +59,7 @@ const char *qemu_ram_get_idstr(RAMBlock *rb); void *qemu_ram_get_host_addr(RAMBlock *rb); ram_addr_t qemu_ram_get_offset(RAMBlock *rb); ram_addr_t qemu_ram_get_used_length(RAMBlock *rb); +ram_addr_t qemu_ram_get_max_length(RAMBlock *rb); bool qemu_ram_is_shared(RAMBlock *rb); bool qemu_ram_is_uf_zeroable(RAMBlock *rb); void qemu_ram_set_uf_zeroable(RAMBlock *rb); diff --git a/stubs/ram-block.c b/stubs/ram-block.c index 73c0a3ee08..10855b52dd 100644 --- a/stubs/ram-block.c +++ b/stubs/ram-block.c @@ -2,21 +2,6 @@ #include "exec/ramlist.h" #include "exec/cpu-common.h" -void *qemu_ram_get_host_addr(RAMBlock *rb) -{ - return 0; -} - -ram_addr_t qemu_ram_get_offset(RAMBlock *rb) -{ - return 0; -} - -ram_addr_t qemu_ram_get_used_length(RAMBlock *rb) -{ - return 0; -} - void ram_block_notifier_add(RAMBlockNotifier *n) { } @@ -24,8 +9,3 @@ void ram_block_notifier_add(RAMBlockNotifier *n) void ram_block_notifier_remove(RAMBlockNotifier *n) { } - -int qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque) -{ - return 0; -} diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c index 813f7ec564..71e02e7f35 100644 --- a/util/vfio-helpers.c +++ b/util/vfio-helpers.c @@ -376,8 +376,13 @@ static void qemu_vfio_ram_block_added(RAMBlockNotifier *n, void *host, size_t size) { QEMUVFIOState *s = container_of(n, QEMUVFIOState, ram_notifier); + int ret; + trace_qemu_vfio_ram_block_added(s, host, size); - qemu_vfio_dma_map(s, host, size, false, NULL); + ret = qemu_vfio_dma_map(s, host, size, false, NULL); + if (ret) { + error_report("qemu_vfio_dma_map(%p, %zu) failed: %d", host, size, ret); + } } static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n, @@ -390,33 +395,14 @@ static void qemu_vfio_ram_block_removed(RAMBlockNotifier *n, } } -static int qemu_vfio_init_ramblock(RAMBlock *rb, void *opaque) -{ - void *host_addr = qemu_ram_get_host_addr(rb); - ram_addr_t length = qemu_ram_get_used_length(rb); - int ret; - QEMUVFIOState *s = opaque; - - if (!host_addr) { - return 0; - } - ret = qemu_vfio_dma_map(s, host_addr, length, false, NULL); - if (ret) { - fprintf(stderr, "qemu_vfio_init_ramblock: failed %p %" PRId64 "\n", - host_addr, (uint64_t)length); - } - return 0; -} - static void qemu_vfio_open_common(QEMUVFIOState *s) { qemu_mutex_init(&s->lock); s->ram_notifier.ram_block_added = qemu_vfio_ram_block_added; s->ram_notifier.ram_block_removed = qemu_vfio_ram_block_removed; - ram_block_notifier_add(&s->ram_notifier); s->low_water_mark = QEMU_VFIO_IOVA_MIN; s->high_water_mark = QEMU_VFIO_IOVA_MAX; - qemu_ram_foreach_block(qemu_vfio_init_ramblock, s); + ram_block_notifier_add(&s->ram_notifier); } /**
Factor it out into common code when a new notifier is registered, just as done with the memory region notifier. This allows us to have the logic about how to process existing ram blocks at a central place (which will be extended soon). Just like when adding a new ram block, we have to register the max_length for now. We don't have a way to get notified about resizes yet, and some memory would not be mapped when growing the ram block. Note: Currently, ram blocks are only "fake resized". All memory (max_length) is accessible. We can get rid of a bunch of functions in stubs/ram-block.c . Print the warning from inside qemu_vfio_ram_block_added(). Cc: Richard Henderson <rth@twiddle.net> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- exec.c | 5 +++++ hw/core/numa.c | 14 ++++++++++++++ include/exec/cpu-common.h | 1 + stubs/ram-block.c | 20 -------------------- util/vfio-helpers.c | 28 +++++++--------------------- 5 files changed, 27 insertions(+), 41 deletions(-)