[Qemu-devel] [PATCH 0/2] ivshmem: clean up building and arguments

[Qemu-devel] [PATCH 0/2] ivshmem: clean up building and arguments

[Levente Kurusa]

Two simple patches.

The first one fixes a build error when IVSHMEM_DEBUG is enabled,
and the second one converts an argument from integer to a bitfield,
and makes all (1) callsites use ivshmem_has_feature.

Levente Kurusa (2):
  ivshmem: fix building when debug mode is enabled
  ivshmem: convert option 'use64' to a bit instead of integer

 hw/misc/ivshmem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] ivshmem: fix building when debug mode is enabled

[Levente Kurusa]

ivsmem_offset was removed, however this debug statement was not updated.
Modify the statement to fit the new mechanic.

Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
---
 hw/misc/ivshmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index bb1f67c..79603c0 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -479,8 +479,8 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
                                    "ivshmem.bar2", s->ivshmem_size, map_ptr);
         vmstate_register_ram(&s->ivshmem, DEVICE(s));
 
-        IVSHMEM_DPRINTF("guest h/w addr = %" PRIu64 ", size = %" PRIu64 "\n",
-                         s->ivshmem_offset, s->ivshmem_size);
+        IVSHMEM_DPRINTF("guest h/w addr = %p, size = %" PRIu64 "\n",
+                         map_ptr, s->ivshmem_size);
 
         memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
 
-- 
1.9.3

[Qemu-devel] [PATCH 2/2] ivshmem: convert option 'use64' to a bit instead of integer

[Levente Kurusa]

It was expecting an integer, however the sole usecase of it just
checked whether it was assigned to or not. Hence, remove integerness,
and add a new bitfield instead.

Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
---
 hw/misc/ivshmem.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 79603c0..eac85ad 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -34,6 +34,7 @@
 
 #define IVSHMEM_IOEVENTFD   0
 #define IVSHMEM_MSI     1
+#define IVSHMEM_64BIT   2
 
 #define IVSHMEM_PEER    0
 #define IVSHMEM_MASTER  1
@@ -83,7 +84,6 @@ typedef struct IVShmemState {
     MemoryRegion ivshmem;
     uint64_t ivshmem_size; /* size of shared memory region */
     uint32_t ivshmem_attr;
-    uint32_t ivshmem_64bit;
     int shm_fd; /* shared memory file descriptor */
 
     Peer *peers;
@@ -706,7 +706,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
     memory_region_init(&s->bar, OBJECT(s), "ivshmem-bar2-container", s->ivshmem_size);
     s->ivshmem_attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
         PCI_BASE_ADDRESS_MEM_PREFETCH;
-    if (s->ivshmem_64bit) {
+    if (ivshmem_has_feature(s, IVSHMEM_64BIT)) {
         s->ivshmem_attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
     }
 
@@ -804,7 +804,7 @@ static Property ivshmem_properties[] = {
     DEFINE_PROP_BIT("msi", IVShmemState, features, IVSHMEM_MSI, true),
     DEFINE_PROP_STRING("shm", IVShmemState, shmobj),
     DEFINE_PROP_STRING("role", IVShmemState, role),
-    DEFINE_PROP_UINT32("use64", IVShmemState, ivshmem_64bit, 1),
+    DEFINE_PROP_BIT("use64", IVShmemState, features, IVSHMEM_64BIT, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2] ivshmem: convert option 'use64' to a bit instead of integer

[Michael S. Tsirkin]

On Mon, Aug 04, 2014 at 05:06:21PM +0200, Levente Kurusa wrote:
> It was expecting an integer, however the sole usecase of it just
> checked whether it was assigned to or not. Hence, remove integerness,
> and add a new bitfield instead.
> 
> Signed-off-by: Levente Kurusa <lkurusa@redhat.com>

This might break some existing scripts.
If you want to address this, add a new type that
can accept both integer and bool.

> ---
>  hw/misc/ivshmem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 79603c0..eac85ad 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -34,6 +34,7 @@
>  
>  #define IVSHMEM_IOEVENTFD   0
>  #define IVSHMEM_MSI     1
> +#define IVSHMEM_64BIT   2
>  
>  #define IVSHMEM_PEER    0
>  #define IVSHMEM_MASTER  1
> @@ -83,7 +84,6 @@ typedef struct IVShmemState {
>      MemoryRegion ivshmem;
>      uint64_t ivshmem_size; /* size of shared memory region */
>      uint32_t ivshmem_attr;
> -    uint32_t ivshmem_64bit;
>      int shm_fd; /* shared memory file descriptor */
>  
>      Peer *peers;
> @@ -706,7 +706,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
>      memory_region_init(&s->bar, OBJECT(s), "ivshmem-bar2-container", s->ivshmem_size);
>      s->ivshmem_attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>          PCI_BASE_ADDRESS_MEM_PREFETCH;
> -    if (s->ivshmem_64bit) {
> +    if (ivshmem_has_feature(s, IVSHMEM_64BIT)) {
>          s->ivshmem_attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>      }
>  
> @@ -804,7 +804,7 @@ static Property ivshmem_properties[] = {
>      DEFINE_PROP_BIT("msi", IVShmemState, features, IVSHMEM_MSI, true),
>      DEFINE_PROP_STRING("shm", IVShmemState, shmobj),
>      DEFINE_PROP_STRING("role", IVShmemState, role),
> -    DEFINE_PROP_UINT32("use64", IVShmemState, ivshmem_64bit, 1),
> +    DEFINE_PROP_BIT("use64", IVShmemState, features, IVSHMEM_64BIT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH 2/2] ivshmem: convert option 'use64' to a bit instead of integer

[Levente Kurusa]

> On Mon, Aug 04, 2014 at 05:06:21PM +0200, Levente Kurusa wrote:
> > It was expecting an integer, however the sole usecase of it just
> > checked whether it was assigned to or not. Hence, remove integerness,
> > and add a new bitfield instead.
> > 
> > Signed-off-by: Levente Kurusa <lkurusa@redhat.com>
> 
> This might break some existing scripts.
> If you want to address this, add a new type that
> can accept both integer and bool.

Yea, makes sense.

I don't really know whether adding a new type is worth the effort;
let's just drop this patch and keep the interface as it is, since it
makes no harm.

Should I resend the first patch as a separate patch or can it be
applied as it is?

Oh, and are we taking the ivshmem command line as set in stone?
I'm asking because I don't really see the point in that, given that
ivshmem is still experimental and broken in many cases.

Thanks,
Levente Kurusa