[Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Hosts threads which handle vring should have high MemoryListener priority
than kvm. For currently code, take the following scenario:
  kvm_region_add() run earlier before vhost_region_add(), then in guest,
vring's desc[i] can refer to addressX in the new region known by guest.
But vhost does not know this new region yet, and the vring handler will
fail.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/virtio/dataplane/hostmem.c |    2 +-
 hw/virtio/vhost.c             |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
index 37292ff..67cbce1 100644
--- a/hw/virtio/dataplane/hostmem.c
+++ b/hw/virtio/dataplane/hostmem.c
@@ -158,7 +158,7 @@ void hostmem_init(HostMem *hostmem)
         .eventfd_del = hostmem_listener_eventfd_dummy,
         .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy,
         .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy,
-        .priority = 10,
+        .priority = 9,
     };
 
     memory_listener_register(&hostmem->listener, &address_space_memory);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index fbabf99..91c313b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -856,7 +856,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
         .log_global_stop = vhost_log_global_stop,
         .eventfd_add = vhost_eventfd_add,
         .eventfd_del = vhost_eventfd_del,
-        .priority = 10
+        .priority = 9
     };
     hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
     hdev->n_mem_sections = 0;
-- 
1.7.4.4

[Qemu-devel] [PATCH 2/2] mem: highlight the listener's priority as enum

[Liu Ping Fan]

From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

It will make the priority prominent, when new listener added.
All the priority's value are kept unchanged, except for vhost
and hostmem.(Changes introduced by prev patch)

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 exec.c                        |    4 ++--
 hw/virtio/dataplane/hostmem.c |    2 +-
 hw/virtio/vhost.c             |    2 +-
 include/exec/memory.h         |   12 +++++++++++-
 kvm-all.c                     |    4 ++--
 xen-all.c                     |    2 +-
 6 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index 19725db..aef0349 100644
--- a/exec.c
+++ b/exec.c
@@ -1766,13 +1766,13 @@ static MemoryListener core_memory_listener = {
     .begin = core_begin,
     .log_global_start = core_log_global_start,
     .log_global_stop = core_log_global_stop,
-    .priority = 1,
+    .priority = PRI_CORE,
 };
 
 static MemoryListener io_memory_listener = {
     .region_add = io_region_add,
     .region_del = io_region_del,
-    .priority = 0,
+    .priority = PRI_DEFAULT,
 };
 
 static MemoryListener tcg_memory_listener = {
diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
index 67cbce1..6be182c 100644
--- a/hw/virtio/dataplane/hostmem.c
+++ b/hw/virtio/dataplane/hostmem.c
@@ -158,7 +158,7 @@ void hostmem_init(HostMem *hostmem)
         .eventfd_del = hostmem_listener_eventfd_dummy,
         .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy,
         .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy,
-        .priority = 9,
+        .priority = PRI_VRING,
     };
 
     memory_listener_register(&hostmem->listener, &address_space_memory);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 91c313b..df6d8c5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -856,7 +856,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
         .log_global_stop = vhost_log_global_stop,
         .eventfd_add = vhost_eventfd_add,
         .eventfd_del = vhost_eventfd_del,
-        .priority = 9
+        .priority = PRI_VRING
     };
     hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
     hdev->n_mem_sections = 0;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9e88320..77e0d40 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -192,6 +192,16 @@ struct MemoryRegionSection {
 
 typedef struct MemoryListener MemoryListener;
 
+/* The list of priority, ex, vhost should have higher priority (less num) than
+ * kvm, ie PRI_VRING < PRI_HYPV
+ */
+typedef enum {
+    PRI_DEFAULT = 0,
+    PRI_CORE = 1,
+    PRI_VRING = 9,
+    PRI_HYPERV = 10,
+} MemListenerPriority;
+
 /**
  * MemoryListener: callbacks structure for updates to the physical memory map
  *
@@ -218,7 +228,7 @@ struct MemoryListener {
     void (*coalesced_mmio_del)(MemoryListener *listener, MemoryRegionSection *section,
                                hwaddr addr, hwaddr len);
     /* Lower = earlier (during add), later (during del) */
-    unsigned priority;
+    MemListenerPriority priority;
     AddressSpace *address_space_filter;
     QTAILQ_ENTRY(MemoryListener) link;
 };
diff --git a/kvm-all.c b/kvm-all.c
index 3a31602..2794dee 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -875,13 +875,13 @@ static MemoryListener kvm_memory_listener = {
     .eventfd_del = kvm_mem_ioeventfd_del,
     .coalesced_mmio_add = kvm_coalesce_mmio_region,
     .coalesced_mmio_del = kvm_uncoalesce_mmio_region,
-    .priority = 10,
+    .priority = PRI_HYPERV,
 };
 
 static MemoryListener kvm_io_listener = {
     .eventfd_add = kvm_io_ioeventfd_add,
     .eventfd_del = kvm_io_ioeventfd_del,
-    .priority = 10,
+    .priority = PRI_HYPERV,
 };
 
 static void kvm_handle_interrupt(CPUState *cpu, int mask)
diff --git a/xen-all.c b/xen-all.c
index 539a154..7062420 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -562,7 +562,7 @@ static MemoryListener xen_memory_listener = {
     .log_sync = xen_log_sync,
     .log_global_start = xen_log_global_start,
     .log_global_stop = xen_log_global_stop,
-    .priority = 10,
+    .priority = PRI_HYPERV,
 };
 
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Stefan Hajnoczi]

On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Hosts threads which handle vring should have high MemoryListener priority
> than kvm. For currently code, take the following scenario:
>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
> vring's desc[i] can refer to addressX in the new region known by guest.
> But vhost does not know this new region yet, and the vring handler will
> fail.

Is there a concrete scenario where this happens?

I can think of situations like the ioeventfd being readable before
vhost/hostmem is populated.  But I don't see how that's related to the
priority of kvm_region_add().

Stefan

Re: [Qemu-devel] [PATCH 2/2] mem: highlight the listener's priority as enum

[Stefan Hajnoczi]

On Thu, May 09, 2013 at 08:40:22AM +0800, Liu Ping Fan wrote:
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 9e88320..77e0d40 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -192,6 +192,16 @@ struct MemoryRegionSection {
>  
>  typedef struct MemoryListener MemoryListener;
>  
> +/* The list of priority, ex, vhost should have higher priority (less num) than
> + * kvm, ie PRI_VRING < PRI_HYPV

s/PRI_HYPV/PRI_HYPERV/

> + */
> +typedef enum {
> +    PRI_DEFAULT = 0,
> +    PRI_CORE = 1,
> +    PRI_VRING = 9,
> +    PRI_HYPERV = 10,

HYPERV == hypervisor?  Easy to confuse with Microsoft Hyper-V.  Please
use PRI_ACCEL or PRI_HYPERVISOR.

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Michael S. Tsirkin]

On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Hosts threads which handle vring should have high MemoryListener priority
> than kvm. For currently code, take the following scenario:
>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
> vring's desc[i] can refer to addressX in the new region known by guest.
> But vhost does not know this new region yet, and the vring handler will
> fail.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Is this seen in testing, or are you describing a theorecitical
scenario? Please make this clear in the commit log.

> ---
>  hw/virtio/dataplane/hostmem.c |    2 +-
>  hw/virtio/vhost.c             |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
> index 37292ff..67cbce1 100644
> --- a/hw/virtio/dataplane/hostmem.c
> +++ b/hw/virtio/dataplane/hostmem.c
> @@ -158,7 +158,7 @@ void hostmem_init(HostMem *hostmem)
>          .eventfd_del = hostmem_listener_eventfd_dummy,
>          .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy,
>          .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy,
> -        .priority = 10,
> +        .priority = 9,
>      };
>  
>      memory_listener_register(&hostmem->listener, &address_space_memory);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index fbabf99..91c313b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -856,7 +856,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
>          .log_global_stop = vhost_log_global_stop,
>          .eventfd_add = vhost_eventfd_add,
>          .eventfd_del = vhost_eventfd_del,
> -        .priority = 10
> +        .priority = 9
>      };
>      hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
>      hdev->n_mem_sections = 0;
> -- 
> 1.7.4.4

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[liu ping fan]

On Thu, May 9, 2013 at 4:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Hosts threads which handle vring should have high MemoryListener priority
>> than kvm. For currently code, take the following scenario:
>>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
>> vring's desc[i] can refer to addressX in the new region known by guest.
>> But vhost does not know this new region yet, and the vring handler will
>> fail.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Is this seen in testing, or are you describing a theorecitical
> scenario? Please make this clear in the commit log.
>
A  theorecitical scenario.  I think vcpu thread and vhost are async,
so we need this method to sync.
>> ---
>>  hw/virtio/dataplane/hostmem.c |    2 +-
>>  hw/virtio/vhost.c             |    2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/dataplane/hostmem.c b/hw/virtio/dataplane/hostmem.c
>> index 37292ff..67cbce1 100644
>> --- a/hw/virtio/dataplane/hostmem.c
>> +++ b/hw/virtio/dataplane/hostmem.c
>> @@ -158,7 +158,7 @@ void hostmem_init(HostMem *hostmem)
>>          .eventfd_del = hostmem_listener_eventfd_dummy,
>>          .coalesced_mmio_add = hostmem_listener_coalesced_mmio_dummy,
>>          .coalesced_mmio_del = hostmem_listener_coalesced_mmio_dummy,
>> -        .priority = 10,
>> +        .priority = 9,
>>      };
>>
>>      memory_listener_register(&hostmem->listener, &address_space_memory);
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index fbabf99..91c313b 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -856,7 +856,7 @@ int vhost_dev_init(struct vhost_dev *hdev, int devfd, const char *devpath,
>>          .log_global_stop = vhost_log_global_stop,
>>          .eventfd_add = vhost_eventfd_add,
>>          .eventfd_del = vhost_eventfd_del,
>> -        .priority = 10
>> +        .priority = 9
>>      };
>>      hdev->mem = g_malloc0(offsetof(struct vhost_memory, regions));
>>      hdev->n_mem_sections = 0;
>> --
>> 1.7.4.4

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[liu ping fan]

On Thu, May 9, 2013 at 4:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Hosts threads which handle vring should have high MemoryListener priority
>> than kvm. For currently code, take the following scenario:
>>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
>> vring's desc[i] can refer to addressX in the new region known by guest.
>> But vhost does not know this new region yet, and the vring handler will
>> fail.
>
> Is there a concrete scenario where this happens?
>
> I can think of situations like the ioeventfd being readable before
> vhost/hostmem is populated.  But I don't see how that's related to the
> priority of kvm_region_add().
>
For kvm, ie, In guest, vring_desc.addr can point to a chunk of data in
the new added memory, and kick vhost. The vhost has not added this new
region, so its local lookup table can not translate this new address,
and vring handler will fail.  If vhost priority is higher than kvm,
then, it will know this new address earlier than kvm.

> Stefan

Re: [Qemu-devel] [PATCH 2/2] mem: highlight the listener's priority as enum

[liu ping fan]

On Thu, May 9, 2013 at 4:31 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, May 09, 2013 at 08:40:22AM +0800, Liu Ping Fan wrote:
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 9e88320..77e0d40 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -192,6 +192,16 @@ struct MemoryRegionSection {
>>
>>  typedef struct MemoryListener MemoryListener;
>>
>> +/* The list of priority, ex, vhost should have higher priority (less num) than
>> + * kvm, ie PRI_VRING < PRI_HYPV
>
> s/PRI_HYPV/PRI_HYPERV/
>
Will fix.
>> + */
>> +typedef enum {
>> +    PRI_DEFAULT = 0,
>> +    PRI_CORE = 1,
>> +    PRI_VRING = 9,
>> +    PRI_HYPERV = 10,
>
> HYPERV == hypervisor?  Easy to confuse with Microsoft Hyper-V.  Please
> use PRI_ACCEL or PRI_HYPERVISOR.

Ok.

Regards,
Pingfan

Re: [Qemu-devel] [PATCH 2/2] mem: highlight the listener's priority as enum

[Peter Maydell]

On 9 May 2013 01:40, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> It will make the priority prominent, when new listener added.
> All the priority's value are kept unchanged, except for vhost
> and hostmem.(Changes introduced by prev patch)

Any chance of something somewhere which explains why
these various things care about the order and thus
need these particular priorities? (ie what are the
dependencies between the listeners?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] mem: highlight the listener's priority as enum

[liu ping fan]

On Thu, May 9, 2013 at 5:21 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 May 2013 01:40, Liu Ping Fan <qemulist@gmail.com> wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> It will make the priority prominent, when new listener added.
>> All the priority's value are kept unchanged, except for vhost
>> and hostmem.(Changes introduced by prev patch)
>
> Any chance of something somewhere which explains why
> these various things care about the order and thus
> need these particular priorities? (ie what are the
> dependencies between the listeners?)
>
Sorry, I do not go so deeply, so leave the original value as they are.
 As far as I know, the most prominent one is the kvm and the core. If
a region removed from core earlier than kvm, then
cpu_physical_memory_rw() can use an address valid in kvm, but invalid
for radix-tree, and cause error.

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Paolo Bonzini]

Il 09/05/2013 10:54, liu ping fan ha scritto:
> On Thu, May 9, 2013 at 4:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Hosts threads which handle vring should have high MemoryListener priority
>>> than kvm. For currently code, take the following scenario:
>>>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
>>> vring's desc[i] can refer to addressX in the new region known by guest.
>>> But vhost does not know this new region yet, and the vring handler will
>>> fail.
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Is this seen in testing, or are you describing a theorecitical
>> scenario? Please make this clear in the commit log.
>>
> A  theorecitical scenario.  I think vcpu thread and vhost are async,
> so we need this method to sync.

But why should this matter for hostmem?  It doesn't communicate in any
way with the hypervisor.

Paolo

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Stefan Hajnoczi]

On Thu, May 09, 2013 at 05:00:20PM +0800, liu ping fan wrote:
> On Thu, May 9, 2013 at 4:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >>
> >> Hosts threads which handle vring should have high MemoryListener priority
> >> than kvm. For currently code, take the following scenario:
> >>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
> >> vring's desc[i] can refer to addressX in the new region known by guest.
> >> But vhost does not know this new region yet, and the vring handler will
> >> fail.
> >
> > Is there a concrete scenario where this happens?
> >
> > I can think of situations like the ioeventfd being readable before
> > vhost/hostmem is populated.  But I don't see how that's related to the
> > priority of kvm_region_add().
> >
> For kvm, ie, In guest, vring_desc.addr can point to a chunk of data in
> the new added memory, and kick vhost. The vhost has not added this new
> region, so its local lookup table can not translate this new address,
> and vring handler will fail.  If vhost priority is higher than kvm,
> then, it will know this new address earlier than kvm.

Isn't the real solution to ensure that the memory API is up-to-date
before we notify the guest of memory hotplug?

I still don't see a kvm vs vhost race.  I see a guest vs vhost race
which priority doesn't fix.

Stefan

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[liu ping fan]

On Thu, May 9, 2013 at 11:26 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, May 09, 2013 at 05:00:20PM +0800, liu ping fan wrote:
>> On Thu, May 9, 2013 at 4:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
>> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >>
>> >> Hosts threads which handle vring should have high MemoryListener priority
>> >> than kvm. For currently code, take the following scenario:
>> >>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
>> >> vring's desc[i] can refer to addressX in the new region known by guest.
>> >> But vhost does not know this new region yet, and the vring handler will
>> >> fail.
>> >
>> > Is there a concrete scenario where this happens?
>> >
>> > I can think of situations like the ioeventfd being readable before
>> > vhost/hostmem is populated.  But I don't see how that's related to the
>> > priority of kvm_region_add().
>> >
>> For kvm, ie, In guest, vring_desc.addr can point to a chunk of data in
>> the new added memory, and kick vhost. The vhost has not added this new
>> region, so its local lookup table can not translate this new address,
>> and vring handler will fail.  If vhost priority is higher than kvm,
>> then, it will know this new address earlier than kvm.
>
> Isn't the real solution to ensure that the memory API is up-to-date
> before we notify the guest of memory hotplug?
>
No, it is not.

> I still don't see a kvm vs vhost race.  I see a guest vs vhost race
> which priority doesn't fix.
>
Yes, you are right.
The priority should be vhost > guest, and kvm > guest. So vhost == kvm
is OK. But can it be higher or why chosen as 10 not zero?

If the dependency only lies between MemoryListeners and guest, not
between listeners, then is the priority meanless?  I think we should
make sure about this, because if converting core listener to rcu
style, we will definitely break the sequence of region_add/del, ie
both add&del comes after kvm.

> Stefan

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[Stefan Hajnoczi]

On Fri, May 10, 2013 at 02:03:34PM +0800, liu ping fan wrote:
> On Thu, May 9, 2013 at 11:26 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Thu, May 09, 2013 at 05:00:20PM +0800, liu ping fan wrote:
> >> On Thu, May 9, 2013 at 4:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> > On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
> >> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> >>
> >> >> Hosts threads which handle vring should have high MemoryListener priority
> >> >> than kvm. For currently code, take the following scenario:
> >> >>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
> >> >> vring's desc[i] can refer to addressX in the new region known by guest.
> >> >> But vhost does not know this new region yet, and the vring handler will
> >> >> fail.
> >> >
> >> > Is there a concrete scenario where this happens?
> >> >
> >> > I can think of situations like the ioeventfd being readable before
> >> > vhost/hostmem is populated.  But I don't see how that's related to the
> >> > priority of kvm_region_add().
> >> >
> >> For kvm, ie, In guest, vring_desc.addr can point to a chunk of data in
> >> the new added memory, and kick vhost. The vhost has not added this new
> >> region, so its local lookup table can not translate this new address,
> >> and vring handler will fail.  If vhost priority is higher than kvm,
> >> then, it will know this new address earlier than kvm.
> >
> > Isn't the real solution to ensure that the memory API is up-to-date
> > before we notify the guest of memory hotplug?
> >
> No, it is not.
> 
> > I still don't see a kvm vs vhost race.  I see a guest vs vhost race
> > which priority doesn't fix.
> >
> Yes, you are right.
> The priority should be vhost > guest, and kvm > guest. So vhost == kvm
> is OK. But can it be higher or why chosen as 10 not zero?
> 
> If the dependency only lies between MemoryListeners and guest, not
> between listeners, then is the priority meanless?  I think we should
> make sure about this, because if converting core listener to rcu
> style, we will definitely break the sequence of region_add/del, ie
> both add&del comes after kvm.

Okay, so now we're left with the question "what are the ordering
dependencies between memory listeners?".

I poked around with git-blame(1) but didn't find an explanation.  The
best I can come up with is that the core listeners in exec.c update
QEMU's guest RAM and I/O port mappings, kvm/vhost/xen should be able to
query them.  Therefore exec.c listeners have priority 0 or 1.

BTW the commit that introduced priorities is:

  commit 72e22d2fe17b85e56b4f0c437c61c6e2de97b308
  Author: Avi Kivity <avi@redhat.com>
  Date:   Wed Feb 8 15:05:50 2012 +0200

      memory: switch memory listeners to a QTAILQ

Stefan

Re: [Qemu-devel] [PATCH 1/2] Vring: vring's listener's priority should higher than kvm

[liu ping fan]

On Fri, May 10, 2013 at 3:12 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Fri, May 10, 2013 at 02:03:34PM +0800, liu ping fan wrote:
>> On Thu, May 9, 2013 at 11:26 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Thu, May 09, 2013 at 05:00:20PM +0800, liu ping fan wrote:
>> >> On Thu, May 9, 2013 at 4:30 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> >> > On Thu, May 09, 2013 at 08:40:21AM +0800, Liu Ping Fan wrote:
>> >> >> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> >> >>
>> >> >> Hosts threads which handle vring should have high MemoryListener priority
>> >> >> than kvm. For currently code, take the following scenario:
>> >> >>   kvm_region_add() run earlier before vhost_region_add(), then in guest,
>> >> >> vring's desc[i] can refer to addressX in the new region known by guest.
>> >> >> But vhost does not know this new region yet, and the vring handler will
>> >> >> fail.
>> >> >
>> >> > Is there a concrete scenario where this happens?
>> >> >
>> >> > I can think of situations like the ioeventfd being readable before
>> >> > vhost/hostmem is populated.  But I don't see how that's related to the
>> >> > priority of kvm_region_add().
>> >> >
>> >> For kvm, ie, In guest, vring_desc.addr can point to a chunk of data in
>> >> the new added memory, and kick vhost. The vhost has not added this new
>> >> region, so its local lookup table can not translate this new address,
>> >> and vring handler will fail.  If vhost priority is higher than kvm,
>> >> then, it will know this new address earlier than kvm.
>> >
>> > Isn't the real solution to ensure that the memory API is up-to-date
>> > before we notify the guest of memory hotplug?
>> >
>> No, it is not.
>>
>> > I still don't see a kvm vs vhost race.  I see a guest vs vhost race
>> > which priority doesn't fix.
>> >
>> Yes, you are right.
>> The priority should be vhost > guest, and kvm > guest. So vhost == kvm
>> is OK. But can it be higher or why chosen as 10 not zero?
>>
>> If the dependency only lies between MemoryListeners and guest, not
>> between listeners, then is the priority meanless?  I think we should
>> make sure about this, because if converting core listener to rcu
>> style, we will definitely break the sequence of region_add/del, ie
>> both add&del comes after kvm.
>
> Okay, so now we're left with the question "what are the ordering
> dependencies between memory listeners?".
>
> I poked around with git-blame(1) but didn't find an explanation.  The
> best I can come up with is that the core listeners in exec.c update
> QEMU's guest RAM and I/O port mappings, kvm/vhost/xen should be able to
> query them.  Therefore exec.c listeners have priority 0 or 1.
>
I think
  1. vhost has not relation with exec.c listeners, right?
  2. kvm has relation with exec.c listeners, but as discussed, the
real dependency is between guest and exec.c listeners.
      So kvm just need to get ready before guest, and this is not
guarded by "priority"
  3. xen ? I do not know, but I guess it is like kvm.

So can we ignore the priority? It seems redundant since we rely on
other mechenism.
> BTW the commit that introduced priorities is:
>
>   commit 72e22d2fe17b85e56b4f0c437c61c6e2de97b308
>   Author: Avi Kivity <avi@redhat.com>
>   Date:   Wed Feb 8 15:05:50 2012 +0200
>
>       memory: switch memory listeners to a QTAILQ
>
> Stefan