Patchwork ivshmem: fix guest unable to start with ioeventfd

login
register
mail settings
Submitter zanghongyong@huawei.com
Date Nov. 24, 2011, 10:05 a.m.
Message ID <1322129109-18140-1-git-send-email-zanghongyong@huawei.com>
Download mbox | patch
Permalink /patch/127476/
State New
Headers show

Comments

zanghongyong@huawei.com - Nov. 24, 2011, 10:05 a.m.
From: Hongyong Zang <zanghongyong@huawei.com>

When a guest boots with ioeventfd, an error (by gdb) occurs:
  Program received signal SIGSEGV, Segmentation fault.
  0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
      at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
  363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
The bug is due to accessing s->peers which is NULL.

This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
eventfd information from ivshmem_server.

Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
---
 hw/ivshmem.c |   41 ++++++++++++++---------------------------
 1 files changed, 14 insertions(+), 27 deletions(-)
Cam Macdonell - Nov. 24, 2011, 8:29 p.m.
On Thu, Nov 24, 2011 at 3:05 AM,  <zanghongyong@huawei.com> wrote:
> From: Hongyong Zang <zanghongyong@huawei.com>
>
> When a guest boots with ioeventfd, an error (by gdb) occurs:
>  Program received signal SIGSEGV, Segmentation fault.
>  0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>      at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>  363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> The bug is due to accessing s->peers which is NULL.

Can you share the command-line that caused the fault?

>
> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
> eventfd information from ivshmem_server.

Should this patch be split into two patches, to separate the bug fix
from the other changes related to the Memory API?  Unless I
misunderstand how the two are necessarily related.

Cam

>
> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
> ---
>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>  1 files changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..be26f03 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>     CharDriverState *server_chr;
>     MemoryRegion ivshmem_mmio;
>
> -    pcibus_t mmio_addr;
>     /* We might need to register the BAR before we actually have the memory.
>      * So prepare a container MemoryRegion for the BAR immediately and
>      * add a subregion when we have the memory.
> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>     guest_curr_max = s->peers[posn].nb_eventfds;
>
>     for (i = 0; i < guest_curr_max; i++) {
> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> +            memory_region_del_eventfd(&s->ivshmem_mmio,
> +                                     DOORBELL,
> +                                     4,
> +                                     true,
> +                                     (posn << 16) | i,
> +                                     s->peers[posn].eventfds[i]);
> +        }
>         close(s->peers[posn].eventfds[i]);
>     }
>
> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>     s->peers[posn].nb_eventfds = 0;
>  }
>
> -static void setup_ioeventfds(IVShmemState *s) {
> -
> -    int i, j;
> -
> -    for (i = 0; i <= s->max_peer; i++) {
> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> -            memory_region_add_eventfd(&s->ivshmem_mmio,
> -                                      DOORBELL,
> -                                      4,
> -                                      true,
> -                                      (i << 16) | j,
> -                                      s->peers[i].eventfds[j]);
> -        }
> -    }
> -}
> -
>  /* this function increase the dynamic storage need to store data about other
>  * guests */
>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>     }
>
>     if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
> -        }
> +        memory_region_add_eventfd(&s->ivshmem_mmio,
> +                                  DOORBELL,
> +                                  4,
> +                                  true,
> +                                  (incoming_posn << 16) | guest_max_eventfd,
> +                                  incoming_fd);
>     }
>
>     return;
> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>     memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>                           "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>
> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        setup_ioeventfds(s);
> -    }
> -
>     /* region for registers*/
>     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                      &s->ivshmem_mmio);
> --
> 1.7.1
>
>
zanghongyong@huawei.com - Nov. 25, 2011, 9:12 a.m.
于 2011/11/25,星期五 4:29, Cam Macdonell 写道:
> On Thu, Nov 24, 2011 at 3:05 AM,<zanghongyong@huawei.com>  wrote:
>> From: Hongyong Zang<zanghongyong@huawei.com>
>>
>> When a guest boots with ioeventfd, an error (by gdb) occurs:
>>   Program received signal SIGSEGV, Segmentation fault.
>>   0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>>       at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>>   363             for (j = 0; j<  s->peers[i].nb_eventfds; j++) {
>> The bug is due to accessing s->peers which is NULL.
> Can you share the command-line that caused the fault?
The command-lines:
1) ivshmem_server -m 4 -p /tmp/nahanni
2) qemu-system-x86_64 -smp 1 -m 1024 -boot c -drive 
file=./rhel6_ivsh1.img,if=virtio
-chardev socket,path=/tmp/nahanni,id=lzw -device 
ivshmem,chardev=lzw,size=4m,ioeventfd=on,vectors=8 -vnc :41

>> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
>> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
>> eventfd information from ivshmem_server.
> Should this patch be split into two patches, to separate the bug fix
> from the other changes related to the Memory API?  Unless I
> misunderstand how the two are necessarily related.

This bug locates in setup_ioeventfds(). The setup_ioeventfds() function 
wants to call memory_region_add_eventfd()
to configure eventfd info for every s->peer, but s->peers are 
uninitialized at this moment.

When qemu receives eventfd info from "ivshmem_server", ivshmem_read() is 
called to set eventfd info.
Function ivshmem_read() calls kvm_set_ioeventfd_mmio_long() which can be 
encapsulated in the new Memory API.

So this patch uses memory_region_add_eventfd() to replace 
kvm_set_ioeventfd_mmio_long(). In this way, each qemu
has already configured the proper eventfd info, and there's no need to 
use the function setup_ioeventfds().
Furthermore, nobody uses IVShmemState's member "pcibus_t mmio_addr". So 
we can remove the member as well.

Regards,
Hongyong Zang

>
> Cam
>
>> Signed-off-by: Hongyong Zang<zanghongyong@huawei.com>
>> ---
>>   hw/ivshmem.c |   41 ++++++++++++++---------------------------
>>   1 files changed, 14 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>> index 242fbea..be26f03 100644
>> --- a/hw/ivshmem.c
>> +++ b/hw/ivshmem.c
>> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>>      CharDriverState *server_chr;
>>      MemoryRegion ivshmem_mmio;
>>
>> -    pcibus_t mmio_addr;
>>      /* We might need to register the BAR before we actually have the memory.
>>       * So prepare a container MemoryRegion for the BAR immediately and
>>       * add a subregion when we have the memory.
>> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>      guest_curr_max = s->peers[posn].nb_eventfds;
>>
>>      for (i = 0; i<  guest_curr_max; i++) {
>> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
>> -                    s->mmio_addr + DOORBELL, (posn<<  16) | i, 0);
>> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> +            memory_region_del_eventfd(&s->ivshmem_mmio,
>> +                                     DOORBELL,
>> +                                     4,
>> +                                     true,
>> +                                     (posn<<  16) | i,
>> +                                     s->peers[posn].eventfds[i]);
>> +        }
>>          close(s->peers[posn].eventfds[i]);
>>      }
>>
>> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>      s->peers[posn].nb_eventfds = 0;
>>   }
>>
>> -static void setup_ioeventfds(IVShmemState *s) {
>> -
>> -    int i, j;
>> -
>> -    for (i = 0; i<= s->max_peer; i++) {
>> -        for (j = 0; j<  s->peers[i].nb_eventfds; j++) {
>> -            memory_region_add_eventfd(&s->ivshmem_mmio,
>> -                                      DOORBELL,
>> -                                      4,
>> -                                      true,
>> -                                      (i<<  16) | j,
>> -                                      s->peers[i].eventfds[j]);
>> -        }
>> -    }
>> -}
>> -
>>   /* this function increase the dynamic storage need to store data about other
>>   * guests */
>>   static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>>      }
>>
>>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
>> -                        (incoming_posn<<  16) | guest_max_eventfd, 1)<  0) {
>> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
>> -        }
>> +        memory_region_add_eventfd(&s->ivshmem_mmio,
>> +                                  DOORBELL,
>> +                                  4,
>> +                                  true,
>> +                                  (incoming_posn<<  16) | guest_max_eventfd,
>> +                                  incoming_fd);
>>      }
>>
>>      return;
>> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>      memory_region_init_io(&s->ivshmem_mmio,&ivshmem_mmio_ops, s,
>>                            "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>>
>> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> -        setup_ioeventfds(s);
>> -    }
>> -
>>      /* region for registers*/
>>      pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>                       &s->ivshmem_mmio);
>> --
>> 1.7.1
>>
>>
> .
>
zanghongyong@huawei.com - Nov. 30, 2011, 8:29 a.m.
Can this bug fix patch be applied yet?
With this bug, guest os cannot successfully boot with ioeventfd.
Thus the new PIO DoorBell patch cannot be posted.

Thanks,
Hongyong

于 2011/11/24,星期四 18:05, zanghongyong@huawei.com 写道:
> From: Hongyong Zang <zanghongyong@huawei.com>
>
> When a guest boots with ioeventfd, an error (by gdb) occurs:
>   Program received signal SIGSEGV, Segmentation fault.
>   0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>       at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>   363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> The bug is due to accessing s->peers which is NULL.
>
> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
> eventfd information from ivshmem_server.
>
> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
> ---
>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>  1 files changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..be26f03 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>      CharDriverState *server_chr;
>      MemoryRegion ivshmem_mmio;
>  
> -    pcibus_t mmio_addr;
>      /* We might need to register the BAR before we actually have the memory.
>       * So prepare a container MemoryRegion for the BAR immediately and
>       * add a subregion when we have the memory.
> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>      guest_curr_max = s->peers[posn].nb_eventfds;
>  
>      for (i = 0; i < guest_curr_max; i++) {
> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> +            memory_region_del_eventfd(&s->ivshmem_mmio,
> +                                     DOORBELL,
> +                                     4,
> +                                     true,
> +                                     (posn << 16) | i,
> +                                     s->peers[posn].eventfds[i]);
> +        }
>          close(s->peers[posn].eventfds[i]);
>      }
>  
> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>      s->peers[posn].nb_eventfds = 0;
>  }
>  
> -static void setup_ioeventfds(IVShmemState *s) {
> -
> -    int i, j;
> -
> -    for (i = 0; i <= s->max_peer; i++) {
> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> -            memory_region_add_eventfd(&s->ivshmem_mmio,
> -                                      DOORBELL,
> -                                      4,
> -                                      true,
> -                                      (i << 16) | j,
> -                                      s->peers[i].eventfds[j]);
> -        }
> -    }
> -}
> -
>  /* this function increase the dynamic storage need to store data about other
>   * guests */
>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>      }
>  
>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
> -        }
> +        memory_region_add_eventfd(&s->ivshmem_mmio,
> +                                  DOORBELL,
> +                                  4,
> +                                  true,
> +                                  (incoming_posn << 16) | guest_max_eventfd,
> +                                  incoming_fd);
>      }
>  
>      return;
> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>      memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>                            "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>  
> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        setup_ioeventfds(s);
> -    }
> -
>      /* region for registers*/
>      pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                       &s->ivshmem_mmio);
Cam Macdonell - Nov. 30, 2011, 10:26 p.m.
2011/11/30 Zang Hongyong <zanghongyong@huawei.com>:
> Can this bug fix patch be applied yet?

Sorry, for not replying yet.  I'll test your patch within the next day.

> With this bug, guest os cannot successfully boot with ioeventfd.
> Thus the new PIO DoorBell patch cannot be posted.

Well, you can certainly post the new patch, just clarify that it's
dependent on this patch.

Sincerely,
Cam

>
> Thanks,
> Hongyong
>
> 于 2011/11/24,星期四 18:05, zanghongyong@huawei.com 写道:
>> From: Hongyong Zang <zanghongyong@huawei.com>
>>
>> When a guest boots with ioeventfd, an error (by gdb) occurs:
>>   Program received signal SIGSEGV, Segmentation fault.
>>   0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>>       at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>>   363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>> The bug is due to accessing s->peers which is NULL.
>>
>> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
>> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
>> eventfd information from ivshmem_server.
>>
>> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
>> ---
>>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>>  1 files changed, 14 insertions(+), 27 deletions(-)
>>
>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>> index 242fbea..be26f03 100644
>> --- a/hw/ivshmem.c
>> +++ b/hw/ivshmem.c
>> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>>      CharDriverState *server_chr;
>>      MemoryRegion ivshmem_mmio;
>>
>> -    pcibus_t mmio_addr;
>>      /* We might need to register the BAR before we actually have the memory.
>>       * So prepare a container MemoryRegion for the BAR immediately and
>>       * add a subregion when we have the memory.
>> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>      guest_curr_max = s->peers[posn].nb_eventfds;
>>
>>      for (i = 0; i < guest_curr_max; i++) {
>> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
>> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
>> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> +            memory_region_del_eventfd(&s->ivshmem_mmio,
>> +                                     DOORBELL,
>> +                                     4,
>> +                                     true,
>> +                                     (posn << 16) | i,
>> +                                     s->peers[posn].eventfds[i]);
>> +        }
>>          close(s->peers[posn].eventfds[i]);
>>      }
>>
>> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>      s->peers[posn].nb_eventfds = 0;
>>  }
>>
>> -static void setup_ioeventfds(IVShmemState *s) {
>> -
>> -    int i, j;
>> -
>> -    for (i = 0; i <= s->max_peer; i++) {
>> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>> -            memory_region_add_eventfd(&s->ivshmem_mmio,
>> -                                      DOORBELL,
>> -                                      4,
>> -                                      true,
>> -                                      (i << 16) | j,
>> -                                      s->peers[i].eventfds[j]);
>> -        }
>> -    }
>> -}
>> -
>>  /* this function increase the dynamic storage need to store data about other
>>   * guests */
>>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>>      }
>>
>>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
>> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
>> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
>> -        }
>> +        memory_region_add_eventfd(&s->ivshmem_mmio,
>> +                                  DOORBELL,
>> +                                  4,
>> +                                  true,
>> +                                  (incoming_posn << 16) | guest_max_eventfd,
>> +                                  incoming_fd);
>>      }
>>
>>      return;
>> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>      memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>>                            "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>>
>> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>> -        setup_ioeventfds(s);
>> -    }
>> -
>>      /* region for registers*/
>>      pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>                       &s->ivshmem_mmio);
>
>
Cam Macdonell - Dec. 2, 2011, 8 a.m.
2011/11/30 Cam Macdonell <cam@cs.ualberta.ca>:
> 2011/11/30 Zang Hongyong <zanghongyong@huawei.com>:
>> Can this bug fix patch be applied yet?
>
> Sorry, for not replying yet.  I'll test your patch within the next day.

Have you confirmed the proper receipt of interrupts in the receiving guests?

I can confirm the bug occurs with ioeventfd enabled and that the
patches fixes it, but sometime after 15.1, I no longer see interrupts
(MSI or regular) being delivered in the guest.

I will bisect tomorrow.

Cam

>
>> With this bug, guest os cannot successfully boot with ioeventfd.
>> Thus the new PIO DoorBell patch cannot be posted.
>
> Well, you can certainly post the new patch, just clarify that it's
> dependent on this patch.
>
> Sincerely,
> Cam
>
>>
>> Thanks,
>> Hongyong
>>
>> 于 2011/11/24,星期四 18:05, zanghongyong@huawei.com 写道:
>>> From: Hongyong Zang <zanghongyong@huawei.com>
>>>
>>> When a guest boots with ioeventfd, an error (by gdb) occurs:
>>>   Program received signal SIGSEGV, Segmentation fault.
>>>   0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>>>       at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>>>   363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>>> The bug is due to accessing s->peers which is NULL.
>>>
>>> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
>>> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
>>> eventfd information from ivshmem_server.
>>>
>>> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
>>> ---
>>>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>>>  1 files changed, 14 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>>> index 242fbea..be26f03 100644
>>> --- a/hw/ivshmem.c
>>> +++ b/hw/ivshmem.c
>>> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>>>      CharDriverState *server_chr;
>>>      MemoryRegion ivshmem_mmio;
>>>
>>> -    pcibus_t mmio_addr;
>>>      /* We might need to register the BAR before we actually have the memory.
>>>       * So prepare a container MemoryRegion for the BAR immediately and
>>>       * add a subregion when we have the memory.
>>> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>>      guest_curr_max = s->peers[posn].nb_eventfds;
>>>
>>>      for (i = 0; i < guest_curr_max; i++) {
>>> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
>>> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
>>> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>> +            memory_region_del_eventfd(&s->ivshmem_mmio,
>>> +                                     DOORBELL,
>>> +                                     4,
>>> +                                     true,
>>> +                                     (posn << 16) | i,
>>> +                                     s->peers[posn].eventfds[i]);
>>> +        }
>>>          close(s->peers[posn].eventfds[i]);
>>>      }
>>>
>>> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>>      s->peers[posn].nb_eventfds = 0;
>>>  }
>>>
>>> -static void setup_ioeventfds(IVShmemState *s) {
>>> -
>>> -    int i, j;
>>> -
>>> -    for (i = 0; i <= s->max_peer; i++) {
>>> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>>> -            memory_region_add_eventfd(&s->ivshmem_mmio,
>>> -                                      DOORBELL,
>>> -                                      4,
>>> -                                      true,
>>> -                                      (i << 16) | j,
>>> -                                      s->peers[i].eventfds[j]);
>>> -        }
>>> -    }
>>> -}
>>> -
>>>  /* this function increase the dynamic storage need to store data about other
>>>   * guests */
>>>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>>> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>>>      }
>>>
>>>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
>>> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
>>> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
>>> -        }
>>> +        memory_region_add_eventfd(&s->ivshmem_mmio,
>>> +                                  DOORBELL,
>>> +                                  4,
>>> +                                  true,
>>> +                                  (incoming_posn << 16) | guest_max_eventfd,
>>> +                                  incoming_fd);
>>>      }
>>>
>>>      return;
>>> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>>      memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>>>                            "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>>>
>>> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>> -        setup_ioeventfds(s);
>>> -    }
>>> -
>>>      /* region for registers*/
>>>      pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>                       &s->ivshmem_mmio);
>>
>>
Cam Macdonell - Dec. 5, 2011, 7:27 p.m.
2011/12/2 Cam Macdonell <cam@cs.ualberta.ca>:
> 2011/11/30 Cam Macdonell <cam@cs.ualberta.ca>:
>> 2011/11/30 Zang Hongyong <zanghongyong@huawei.com>:
>>> Can this bug fix patch be applied yet?
>>
>> Sorry, for not replying yet.  I'll test your patch within the next day.
>
> Have you confirmed the proper receipt of interrupts in the receiving guests?
>
> I can confirm the bug occurs with ioeventfd enabled and that the
> patches fixes it, but sometime after 15.1, I no longer see interrupts
> (MSI or regular) being delivered in the guest.
>
> I will bisect tomorrow.

With Michael's help we debugged msi-x interrupt delivery.  With that
fix in place, this patch fixes ioeventfd in ivshmem.

>
> Cam
>
>>
>>> With this bug, guest os cannot successfully boot with ioeventfd.
>>> Thus the new PIO DoorBell patch cannot be posted.
>>
>> Well, you can certainly post the new patch, just clarify that it's
>> dependent on this patch.
>>
>> Sincerely,
>> Cam
>>
>>>
>>> Thanks,
>>> Hongyong
>>>
>>> 于 2011/11/24,星期四 18:05, zanghongyong@huawei.com 写道:
>>>> From: Hongyong Zang <zanghongyong@huawei.com>
>>>>
>>>> When a guest boots with ioeventfd, an error (by gdb) occurs:
>>>>   Program received signal SIGSEGV, Segmentation fault.
>>>>   0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>>>>       at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>>>>   363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>>>> The bug is due to accessing s->peers which is NULL.
>>>>
>>>> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
>>>> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
>>>> eventfd information from ivshmem_server.
>>>>
>>>> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
>>>> ---
>>>>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>>>>  1 files changed, 14 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
>>>> index 242fbea..be26f03 100644
>>>> --- a/hw/ivshmem.c
>>>> +++ b/hw/ivshmem.c
>>>> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>>>>      CharDriverState *server_chr;
>>>>      MemoryRegion ivshmem_mmio;
>>>>
>>>> -    pcibus_t mmio_addr;
>>>>      /* We might need to register the BAR before we actually have the memory.
>>>>       * So prepare a container MemoryRegion for the BAR immediately and
>>>>       * add a subregion when we have the memory.
>>>> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>>>      guest_curr_max = s->peers[posn].nb_eventfds;
>>>>
>>>>      for (i = 0; i < guest_curr_max; i++) {
>>>> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
>>>> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
>>>> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>>> +            memory_region_del_eventfd(&s->ivshmem_mmio,
>>>> +                                     DOORBELL,
>>>> +                                     4,
>>>> +                                     true,
>>>> +                                     (posn << 16) | i,
>>>> +                                     s->peers[posn].eventfds[i]);
>>>> +        }
>>>>          close(s->peers[posn].eventfds[i]);
>>>>      }
>>>>
>>>> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>>>>      s->peers[posn].nb_eventfds = 0;
>>>>  }
>>>>
>>>> -static void setup_ioeventfds(IVShmemState *s) {
>>>> -
>>>> -    int i, j;
>>>> -
>>>> -    for (i = 0; i <= s->max_peer; i++) {
>>>> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
>>>> -            memory_region_add_eventfd(&s->ivshmem_mmio,
>>>> -                                      DOORBELL,
>>>> -                                      4,
>>>> -                                      true,
>>>> -                                      (i << 16) | j,
>>>> -                                      s->peers[i].eventfds[j]);
>>>> -        }
>>>> -    }
>>>> -}
>>>> -
>>>>  /* this function increase the dynamic storage need to store data about other
>>>>   * guests */
>>>>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
>>>> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>>>>      }
>>>>
>>>>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>>> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
>>>> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
>>>> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
>>>> -        }
>>>> +        memory_region_add_eventfd(&s->ivshmem_mmio,
>>>> +                                  DOORBELL,
>>>> +                                  4,
>>>> +                                  true,
>>>> +                                  (incoming_posn << 16) | guest_max_eventfd,
>>>> +                                  incoming_fd);
>>>>      }
>>>>
>>>>      return;
>>>> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>>>>      memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>>>>                            "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>>>>
>>>> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
>>>> -        setup_ioeventfds(s);
>>>> -    }
>>>> -
>>>>      /* region for registers*/
>>>>      pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>>>>                       &s->ivshmem_mmio);
>>>
>>>
Cam Macdonell - Jan. 13, 2012, 10:47 p.m.
Can this patch please be merged?  It fixes an important regression
with ioeventfd.

Thanks,
Cam

On Thu, Nov 24, 2011 at 3:05 AM,  <zanghongyong@huawei.com> wrote:
> From: Hongyong Zang <zanghongyong@huawei.com>
>
> When a guest boots with ioeventfd, an error (by gdb) occurs:
>  Program received signal SIGSEGV, Segmentation fault.
>  0x00000000006009cc in setup_ioeventfds (s=0x171dc40)
>      at /home/louzhengwei/git_source/qemu-kvm/hw/ivshmem.c:363
>  363             for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> The bug is due to accessing s->peers which is NULL.
>
> This patch uses the memory region API to replace the old one kvm_set_ioeventfd_mmio_long().
> And this patch makes memory_region_add_eventfd() called in ivshmem_read() when qemu receives
> eventfd information from ivshmem_server.
>
> Signed-off-by: Hongyong Zang <zanghongyong@huawei.com>
> ---
>  hw/ivshmem.c |   41 ++++++++++++++---------------------------
>  1 files changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/hw/ivshmem.c b/hw/ivshmem.c
> index 242fbea..be26f03 100644
> --- a/hw/ivshmem.c
> +++ b/hw/ivshmem.c
> @@ -58,7 +58,6 @@ typedef struct IVShmemState {
>     CharDriverState *server_chr;
>     MemoryRegion ivshmem_mmio;
>
> -    pcibus_t mmio_addr;
>     /* We might need to register the BAR before we actually have the memory.
>      * So prepare a container MemoryRegion for the BAR immediately and
>      * add a subregion when we have the memory.
> @@ -346,8 +345,14 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>     guest_curr_max = s->peers[posn].nb_eventfds;
>
>     for (i = 0; i < guest_curr_max; i++) {
> -        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
> -                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
> +        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> +            memory_region_del_eventfd(&s->ivshmem_mmio,
> +                                     DOORBELL,
> +                                     4,
> +                                     true,
> +                                     (posn << 16) | i,
> +                                     s->peers[posn].eventfds[i]);
> +        }
>         close(s->peers[posn].eventfds[i]);
>     }
>
> @@ -355,22 +360,6 @@ static void close_guest_eventfds(IVShmemState *s, int posn)
>     s->peers[posn].nb_eventfds = 0;
>  }
>
> -static void setup_ioeventfds(IVShmemState *s) {
> -
> -    int i, j;
> -
> -    for (i = 0; i <= s->max_peer; i++) {
> -        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
> -            memory_region_add_eventfd(&s->ivshmem_mmio,
> -                                      DOORBELL,
> -                                      4,
> -                                      true,
> -                                      (i << 16) | j,
> -                                      s->peers[i].eventfds[j]);
> -        }
> -    }
> -}
> -
>  /* this function increase the dynamic storage need to store data about other
>  * guests */
>  static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
> @@ -491,10 +480,12 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>     }
>
>     if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
> -                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
> -            fprintf(stderr, "ivshmem: ioeventfd not available\n");
> -        }
> +        memory_region_add_eventfd(&s->ivshmem_mmio,
> +                                  DOORBELL,
> +                                  4,
> +                                  true,
> +                                  (incoming_posn << 16) | guest_max_eventfd,
> +                                  incoming_fd);
>     }
>
>     return;
> @@ -659,10 +650,6 @@ static int pci_ivshmem_init(PCIDevice *dev)
>     memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
>                           "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
>
> -    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
> -        setup_ioeventfds(s);
> -    }
> -
>     /* region for registers*/
>     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                      &s->ivshmem_mmio);
> --
> 1.7.1
>

Patch

diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 242fbea..be26f03 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -58,7 +58,6 @@  typedef struct IVShmemState {
     CharDriverState *server_chr;
     MemoryRegion ivshmem_mmio;
 
-    pcibus_t mmio_addr;
     /* We might need to register the BAR before we actually have the memory.
      * So prepare a container MemoryRegion for the BAR immediately and
      * add a subregion when we have the memory.
@@ -346,8 +345,14 @@  static void close_guest_eventfds(IVShmemState *s, int posn)
     guest_curr_max = s->peers[posn].nb_eventfds;
 
     for (i = 0; i < guest_curr_max; i++) {
-        kvm_set_ioeventfd_mmio_long(s->peers[posn].eventfds[i],
-                    s->mmio_addr + DOORBELL, (posn << 16) | i, 0);
+        if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
+            memory_region_del_eventfd(&s->ivshmem_mmio,
+                                     DOORBELL,
+                                     4,
+                                     true,
+                                     (posn << 16) | i,
+                                     s->peers[posn].eventfds[i]);
+        }
         close(s->peers[posn].eventfds[i]);
     }
 
@@ -355,22 +360,6 @@  static void close_guest_eventfds(IVShmemState *s, int posn)
     s->peers[posn].nb_eventfds = 0;
 }
 
-static void setup_ioeventfds(IVShmemState *s) {
-
-    int i, j;
-
-    for (i = 0; i <= s->max_peer; i++) {
-        for (j = 0; j < s->peers[i].nb_eventfds; j++) {
-            memory_region_add_eventfd(&s->ivshmem_mmio,
-                                      DOORBELL,
-                                      4,
-                                      true,
-                                      (i << 16) | j,
-                                      s->peers[i].eventfds[j]);
-        }
-    }
-}
-
 /* this function increase the dynamic storage need to store data about other
  * guests */
 static void increase_dynamic_storage(IVShmemState *s, int new_min_size) {
@@ -491,10 +480,12 @@  static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
     }
 
     if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
-        if (kvm_set_ioeventfd_mmio_long(incoming_fd, s->mmio_addr + DOORBELL,
-                        (incoming_posn << 16) | guest_max_eventfd, 1) < 0) {
-            fprintf(stderr, "ivshmem: ioeventfd not available\n");
-        }
+        memory_region_add_eventfd(&s->ivshmem_mmio,
+                                  DOORBELL,
+                                  4,
+                                  true,
+                                  (incoming_posn << 16) | guest_max_eventfd,
+                                  incoming_fd);
     }
 
     return;
@@ -659,10 +650,6 @@  static int pci_ivshmem_init(PCIDevice *dev)
     memory_region_init_io(&s->ivshmem_mmio, &ivshmem_mmio_ops, s,
                           "ivshmem-mmio", IVSHMEM_REG_BAR_SIZE);
 
-    if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
-        setup_ioeventfds(s);
-    }
-
     /* region for registers*/
     pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
                      &s->ivshmem_mmio);