system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)
When a new listener for an address space is registered, the hypervisor must be
informed of all existing eventfds for that address space by calling
eventfd_add() for that listener. Similarly, when a listener is de-registered
from an address space, the hypervisor must be informed of all existing eventfds
for that address space with a call to eventfd_del().
Same is also true for coalesced io. Send coalesced io add/del listener
notifications if any flatrage for the address space registered with the
listener intersects with any coalesced io range.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
changelog:
v2: tags added, indentation fixed, commit log fixed, code cleanup.
diff --git a/system/memory.c b/system/memory.c
index 5e6eb459d5..8379e086fb 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
}
}
+static void
+flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr,
+ MemoryRegionSection *mrs,
+ MemoryListener *listener,
+ AddressSpace *as, bool add)
+{
+ CoalescedMemoryRange *cmr;
+ MemoryRegion *mr = fr->mr;
+ AddrRange tmp;
+
+ QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
+ tmp = addrrange_shift(cmr->addr,
+ int128_sub(fr->addr.start,
+ int128_make64(fr->offset_in_region)));
+
+ if (!addrrange_intersects(tmp, fr->addr)) {
+ return;
+ }
+ tmp = addrrange_intersection(tmp, fr->addr);
+
+ if (add) {
+ listener->coalesced_io_add(listener, mrs,
+ int128_get64(tmp.start),
+ int128_get64(tmp.size));
+ } else {
+ listener->coalesced_io_del(listener, mrs,
+ int128_get64(tmp.start),
+ int128_get64(tmp.size));
+ }
+ }
+}
+
static void address_space_update_topology_pass(AddressSpace *as,
const FlatView *old_view,
const FlatView *new_view,
@@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags)
static void listener_add_address_space(MemoryListener *listener,
AddressSpace *as)
{
+ unsigned i;
FlatView *view;
FlatRange *fr;
+ MemoryRegionIoeventfd *fd;
if (listener->begin) {
listener->begin(listener);
@@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener,
if (listener->region_add) {
listener->region_add(listener, §ion);
}
+
+ /* send coalesced io add notifications */
+ flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
+ listener, as, true);
+
if (fr->dirty_log_mask && listener->log_start) {
listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
}
}
+
+ /*
+ * register all eventfds for this address space for the newly registered
+ * listener.
+ */
+ for (i = 0; i < as->ioeventfd_nb; i++) {
+ fd = &as->ioeventfds[i];
+ MemoryRegionSection section = (MemoryRegionSection) {
+ .fv = address_space_to_flatview(as),
+ .offset_within_address_space = int128_get64(fd->addr.start),
+ .size = fd->addr.size,
+ };
+ listener->eventfd_add(listener, §ion,
+ fd->match_data, fd->data, fd->e);
+ }
+
if (listener->commit) {
listener->commit(listener);
}
@@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener,
static void listener_del_address_space(MemoryListener *listener,
AddressSpace *as)
{
+ unsigned i;
FlatView *view;
FlatRange *fr;
+ MemoryRegionIoeventfd *fd;
if (listener->begin) {
listener->begin(listener);
@@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener,
if (fr->dirty_log_mask && listener->log_stop) {
listener->log_stop(listener, §ion, fr->dirty_log_mask, 0);
}
+
+ /* send coalesced io del notifications */
+ flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
+ listener, as, false);
if (listener->region_del) {
listener->region_del(listener, §ion);
}
}
+
+ /*
+ * de-register all eventfds for this address space for the current
+ * listener.
+ */
+ for (i = 0; i < as->ioeventfd_nb; i++) {
+ fd = &as->ioeventfds[i];
+ MemoryRegionSection section = (MemoryRegionSection) {
+ .fv = address_space_to_flatview(as),
+ .offset_within_address_space = int128_get64(fd->addr.start),
+ .size = fd->addr.size,
+ };
+ listener->eventfd_del(listener, §ion,
+ fd->match_data, fd->data, fd->e);
+ }
+
if (listener->commit) {
listener->commit(listener);
}
--
2.42.0Hi, Ani,
On Tue, Sep 10, 2024 at 05:31:00PM +0530, Ani Sinha wrote:
> When a new listener for an address space is registered, the hypervisor must be
> informed of all existing eventfds for that address space by calling
> eventfd_add() for that listener. Similarly, when a listener is de-registered
> from an address space, the hypervisor must be informed of all existing eventfds
> for that address space with a call to eventfd_del().
>
> Same is also true for coalesced io. Send coalesced io add/del listener
> notifications if any flatrage for the address space registered with the
> listener intersects with any coalesced io range.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
This is only to make the API clean, as I see that only kvm uses either of
the features (while kvm never registers during VM running). Am I right?
I'd like to double check whether we need to copy stable, or whether there's
a real bug.
> ---
> system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
>
> changelog:
> v2: tags added, indentation fixed, commit log fixed, code cleanup.
>
> diff --git a/system/memory.c b/system/memory.c
> index 5e6eb459d5..8379e086fb 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
> }
> }
>
> +static void
> +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr,
> + MemoryRegionSection *mrs,
> + MemoryListener *listener,
> + AddressSpace *as, bool add)
> +{
> + CoalescedMemoryRange *cmr;
> + MemoryRegion *mr = fr->mr;
> + AddrRange tmp;
> +
> + QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
> + tmp = addrrange_shift(cmr->addr,
> + int128_sub(fr->addr.start,
> + int128_make64(fr->offset_in_region)));
> +
> + if (!addrrange_intersects(tmp, fr->addr)) {
> + return;
> + }
> + tmp = addrrange_intersection(tmp, fr->addr);
> +
> + if (add) {
Do we need to check ->coalesced_io_add != NULL here? I wonder whether this
could crash if some dynamic listener got inserted (with coalesced list
non-empty).
> + listener->coalesced_io_add(listener, mrs,
> + int128_get64(tmp.start),
> + int128_get64(tmp.size));
> + } else {
> + listener->coalesced_io_del(listener, mrs,
Same.
> + int128_get64(tmp.start),
> + int128_get64(tmp.size));
> + }
> + }
> +}
> +
> static void address_space_update_topology_pass(AddressSpace *as,
> const FlatView *old_view,
> const FlatView *new_view,
> @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags)
> static void listener_add_address_space(MemoryListener *listener,
> AddressSpace *as)
> {
> + unsigned i;
> FlatView *view;
> FlatRange *fr;
> + MemoryRegionIoeventfd *fd;
>
> if (listener->begin) {
> listener->begin(listener);
> @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener,
> if (listener->region_add) {
> listener->region_add(listener, §ion);
> }
> +
> + /* send coalesced io add notifications */
> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
> + listener, as, true);
> +
> if (fr->dirty_log_mask && listener->log_start) {
> listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
> }
> }
> +
> + /*
> + * register all eventfds for this address space for the newly registered
> + * listener.
> + */
> + for (i = 0; i < as->ioeventfd_nb; i++) {
> + fd = &as->ioeventfds[i];
> + MemoryRegionSection section = (MemoryRegionSection) {
> + .fv = address_space_to_flatview(as),
Nitpick: I don't think it'll be anything different if we hold BQL anyway,
but.. might be good to use "view" directly here, as we're holding a
refcount there via address_space_get_flatview().
While it's not crystal clear otherwise (when without knowing BQL held) that
address_space_to_flatview() will return the same flatview.
> + .offset_within_address_space = int128_get64(fd->addr.start),
> + .size = fd->addr.size,
> + };
> + listener->eventfd_add(listener, §ion,
> + fd->match_data, fd->data, fd->e);
Check ->eventfd_add != NULL?
> + }
> +
> if (listener->commit) {
> listener->commit(listener);
> }
> @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener,
> static void listener_del_address_space(MemoryListener *listener,
> AddressSpace *as)
> {
> + unsigned i;
> FlatView *view;
> FlatRange *fr;
> + MemoryRegionIoeventfd *fd;
>
> if (listener->begin) {
> listener->begin(listener);
> @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener,
> if (fr->dirty_log_mask && listener->log_stop) {
> listener->log_stop(listener, §ion, fr->dirty_log_mask, 0);
> }
> +
> + /* send coalesced io del notifications */
> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
> + listener, as, false);
> if (listener->region_del) {
> listener->region_del(listener, §ion);
> }
> }
> +
> + /*
> + * de-register all eventfds for this address space for the current
> + * listener.
> + */
> + for (i = 0; i < as->ioeventfd_nb; i++) {
> + fd = &as->ioeventfds[i];
> + MemoryRegionSection section = (MemoryRegionSection) {
> + .fv = address_space_to_flatview(as),
Same.
> + .offset_within_address_space = int128_get64(fd->addr.start),
> + .size = fd->addr.size,
> + };
> + listener->eventfd_del(listener, §ion,
> + fd->match_data, fd->data, fd->e);
Check eventfd_del != NULL?
Thanks,
> + }
> +
> if (listener->commit) {
> listener->commit(listener);
> }
> --
> 2.42.0
>
--
Peter Xu
> On 18 Sep 2024, at 12:28 AM, Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Ani,
>
> On Tue, Sep 10, 2024 at 05:31:00PM +0530, Ani Sinha wrote:
>> When a new listener for an address space is registered, the hypervisor must be
>> informed of all existing eventfds for that address space by calling
>> eventfd_add() for that listener. Similarly, when a listener is de-registered
>> from an address space, the hypervisor must be informed of all existing eventfds
>> for that address space with a call to eventfd_del().
>>
>> Same is also true for coalesced io. Send coalesced io add/del listener
>> notifications if any flatrage for the address space registered with the
>> listener intersects with any coalesced io range.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>
> This is only to make the API clean, as I see that only kvm uses either of
> the features (while kvm never registers during VM running). Am I right?
Yes currently there is no use case for deregistering a listener and then re-registering. Hence there seems to be a gap in the implementation. I am working on something that would require de-register and re-register so we will need to fix this.
>
> I'd like to double check whether we need to copy stable, or whether there's
> a real bug.
I think this is not a stable material as up until now, people will not hit this gap in the implementation.
>
>> ---
>> system/memory.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 77 insertions(+)
>>
>> changelog:
>> v2: tags added, indentation fixed, commit log fixed, code cleanup.
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 5e6eb459d5..8379e086fb 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -941,6 +941,38 @@ static void flat_range_coalesced_io_add(FlatRange *fr, AddressSpace *as)
>> }
>> }
>>
>> +static void
>> +flat_range_coalesced_io_notify_listener_add_del(FlatRange *fr,
>> + MemoryRegionSection *mrs,
>> + MemoryListener *listener,
>> + AddressSpace *as, bool add)
>> +{
>> + CoalescedMemoryRange *cmr;
>> + MemoryRegion *mr = fr->mr;
>> + AddrRange tmp;
>> +
>> + QTAILQ_FOREACH(cmr, &mr->coalesced, link) {
>> + tmp = addrrange_shift(cmr->addr,
>> + int128_sub(fr->addr.start,
>> + int128_make64(fr->offset_in_region)));
>> +
>> + if (!addrrange_intersects(tmp, fr->addr)) {
>> + return;
>> + }
>> + tmp = addrrange_intersection(tmp, fr->addr);
>> +
>> + if (add) {
>
> Do we need to check ->coalesced_io_add != NULL here? I wonder whether this
> could crash if some dynamic listener got inserted (with coalesced list
> non-empty).
>
>> + listener->coalesced_io_add(listener, mrs,
>> + int128_get64(tmp.start),
>> + int128_get64(tmp.size));
>> + } else {
>> + listener->coalesced_io_del(listener, mrs,
>
> Same.
>
>> + int128_get64(tmp.start),
>> + int128_get64(tmp.size));
>> + }
>> + }
>> +}
>> +
>> static void address_space_update_topology_pass(AddressSpace *as,
>> const FlatView *old_view,
>> const FlatView *new_view,
>> @@ -3015,8 +3047,10 @@ void memory_global_dirty_log_stop(unsigned int flags)
>> static void listener_add_address_space(MemoryListener *listener,
>> AddressSpace *as)
>> {
>> + unsigned i;
>> FlatView *view;
>> FlatRange *fr;
>> + MemoryRegionIoeventfd *fd;
>>
>> if (listener->begin) {
>> listener->begin(listener);
>> @@ -3041,10 +3075,31 @@ static void listener_add_address_space(MemoryListener *listener,
>> if (listener->region_add) {
>> listener->region_add(listener, §ion);
>> }
>> +
>> + /* send coalesced io add notifications */
>> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
>> + listener, as, true);
>> +
>> if (fr->dirty_log_mask && listener->log_start) {
>> listener->log_start(listener, §ion, 0, fr->dirty_log_mask);
>> }
>> }
>> +
>> + /*
>> + * register all eventfds for this address space for the newly registered
>> + * listener.
>> + */
>> + for (i = 0; i < as->ioeventfd_nb; i++) {
>> + fd = &as->ioeventfds[i];
>> + MemoryRegionSection section = (MemoryRegionSection) {
>> + .fv = address_space_to_flatview(as),
>
> Nitpick: I don't think it'll be anything different if we hold BQL anyway,
> but.. might be good to use "view" directly here, as we're holding a
> refcount there via address_space_get_flatview().
>
> While it's not crystal clear otherwise (when without knowing BQL held) that
> address_space_to_flatview() will return the same flatview.
>
>> + .offset_within_address_space = int128_get64(fd->addr.start),
>> + .size = fd->addr.size,
>> + };
>> + listener->eventfd_add(listener, §ion,
>> + fd->match_data, fd->data, fd->e);
>
> Check ->eventfd_add != NULL?
>
>> + }
>> +
>> if (listener->commit) {
>> listener->commit(listener);
>> }
>> @@ -3054,8 +3109,10 @@ static void listener_add_address_space(MemoryListener *listener,
>> static void listener_del_address_space(MemoryListener *listener,
>> AddressSpace *as)
>> {
>> + unsigned i;
>> FlatView *view;
>> FlatRange *fr;
>> + MemoryRegionIoeventfd *fd;
>>
>> if (listener->begin) {
>> listener->begin(listener);
>> @@ -3067,10 +3124,30 @@ static void listener_del_address_space(MemoryListener *listener,
>> if (fr->dirty_log_mask && listener->log_stop) {
>> listener->log_stop(listener, §ion, fr->dirty_log_mask, 0);
>> }
>> +
>> + /* send coalesced io del notifications */
>> + flat_range_coalesced_io_notify_listener_add_del(fr, §ion,
>> + listener, as, false);
>> if (listener->region_del) {
>> listener->region_del(listener, §ion);
>> }
>> }
>> +
>> + /*
>> + * de-register all eventfds for this address space for the current
>> + * listener.
>> + */
>> + for (i = 0; i < as->ioeventfd_nb; i++) {
>> + fd = &as->ioeventfds[i];
>> + MemoryRegionSection section = (MemoryRegionSection) {
>> + .fv = address_space_to_flatview(as),
>
> Same.
>
>> + .offset_within_address_space = int128_get64(fd->addr.start),
>> + .size = fd->addr.size,
>> + };
>> + listener->eventfd_del(listener, §ion,
>> + fd->match_data, fd->data, fd->e);
>
> Check eventfd_del != NULL?
>
> Thanks,
All of the above suggestions are great. I will fix them in a re-spin. Thanks Peter.
>
>> + }
>> +
>> if (listener->commit) {
>> listener->commit(listener);
>> }
>> --
>> 2.42.0
>>
>
> --
> Peter Xu
© 2016 - 2024 Red Hat, Inc.