[Qemu-devel] [PATCH v2 0/2] ivshmem: fix misconfig of not_legacy_32bit

[Qemu-devel] [PATCH v2 0/2] ivshmem: fix misconfig of not_legacy_32bit

[Zhuangyanying]

From: ZhuangYanying <ann.zhuangyanying@huawei.com>

Recently, I tested ivshmem, found that use64, that is not_legacy_32bit 
implementation is odd, or even the opposite.
Previous use64 = ivshmem_64bit = 1, then attr |= PCI_BASE_ADDRESS_MEM_TYPE_64, 
ivshmem support 1G and above packaged into bar2, presented to the virtual 
machine.
But after commit 5400c02, PCI_BASE_ADDRESS_MEM_TYPE_64 is configured 
while not_legacy_32bit = 0, that is the legacy model.

ZhuangYanying (2):
  hw/misc/ivshmem: fix misconfig of not_legacy_32bit
  hw/misc/ivshmem: set not_legacy_32bit to 1 for ivshmem_doorbell and
    ivshmem-plain

 hw/misc/ivshmem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] ivshmem: fix misconfig of not_legacy_32bit

[Zhuangyanying]

From: ZhuangYanying <ann.zhuangyanying@huawei.com>

After commit 5400c02, ivshmem_64bit renamed to not_legacy_32bit, 
and changed the implementation of this property.
Then use64 = 1, ~PCI_BASE_ADDRESS_MEM_TYPE_64 (default for ivshmem),
the actual use is the legacy model, 
can not support greater than or equal 1G mapping,
which is the opposite of configuration requirements.

Cc: qemu-stable@nongnu.org
Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/misc/ivshmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 230e51b..b897685 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -858,7 +858,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
                      &s->ivshmem_mmio);
 
-    if (!s->not_legacy_32bit) {
+    if (s->not_legacy_32bit) {
         attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] ivshmem: set not_legacy_32bit to 1 for ivshmem_doorbell and ivshmem-plain

[Zhuangyanying]

From: ZhuangYanying <ann.zhuangyanying@huawei.com>

Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>
---
 hw/misc/ivshmem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index b897685..abeaf3d 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -1045,6 +1045,7 @@ static void ivshmem_plain_init(Object *obj)
                              ivshmem_check_memdev_is_busy,
                              OBJ_PROP_LINK_UNREF_ON_RELEASE,
                              &error_abort);
+    s->not_legacy_32bit = 1;
 }
 
 static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
@@ -1116,6 +1117,7 @@ static void ivshmem_doorbell_init(Object *obj)
 
     s->features |= (1 << IVSHMEM_MSI);
     s->legacy_size = SIZE_MAX;  /* whatever the server sends */
+    s->not_legacy_32bit = 1;
 }
 
 static void ivshmem_doorbell_realize(PCIDevice *dev, Error **errp)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] ivshmem: fix misconfig of not_legacy_32bit

[Marc-André Lureau]

Hi

----- Original Message -----
> From: ZhuangYanying <ann.zhuangyanying@huawei.com>
> 
> After commit 5400c02, ivshmem_64bit renamed to not_legacy_32bit,
> and changed the implementation of this property.
> Then use64 = 1, ~PCI_BASE_ADDRESS_MEM_TYPE_64 (default for ivshmem),
> the actual use is the legacy model,
> can not support greater than or equal 1G mapping,
> which is the opposite of configuration requirements.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>
> Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/misc/ivshmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 230e51b..b897685 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -858,7 +858,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error
> **errp)
>      pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                       &s->ivshmem_mmio);
>  
> -    if (!s->not_legacy_32bit) {
> +    if (s->not_legacy_32bit) {
>          attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>      }
>  
> --
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: set not_legacy_32bit to 1 for ivshmem_doorbell and ivshmem-plain

[Marc-André Lureau]

----- Original Message -----
> From: ZhuangYanying <ann.zhuangyanying@huawei.com>
> 
> Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/misc/ivshmem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index b897685..abeaf3d 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1045,6 +1045,7 @@ static void ivshmem_plain_init(Object *obj)
>                               ivshmem_check_memdev_is_busy,
>                               OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                               &error_abort);
> +    s->not_legacy_32bit = 1;
>  }
>  
>  static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
> @@ -1116,6 +1117,7 @@ static void ivshmem_doorbell_init(Object *obj)
>  
>      s->features |= (1 << IVSHMEM_MSI);
>      s->legacy_size = SIZE_MAX;  /* whatever the server sends */
> +    s->not_legacy_32bit = 1;
>  }
>  
>  static void ivshmem_doorbell_realize(PCIDevice *dev, Error **errp)
> --
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 2/2] ivshmem: set not_legacy_32bit to 1 for ivshmem_doorbell and ivshmem-plain

[Gonglei (Arei)]

>
> Subject: [PATCH v2 2/2] ivshmem: set not_legacy_32bit to 1 for
> ivshmem_doorbell and ivshmem-plain
> 
> From: ZhuangYanying <ann.zhuangyanying@huawei.com>
> 
> Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>
> ---
>  hw/misc/ivshmem.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
Reviewed-by: Gonglei <arei.gonglei@huawei.com>

> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index b897685..abeaf3d 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1045,6 +1045,7 @@ static void ivshmem_plain_init(Object *obj)
>                               ivshmem_check_memdev_is_busy,
>                               OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                               &error_abort);
> +    s->not_legacy_32bit = 1;
>  }
> 
>  static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
> @@ -1116,6 +1117,7 @@ static void ivshmem_doorbell_init(Object *obj)
> 
>      s->features |= (1 << IVSHMEM_MSI);
>      s->legacy_size = SIZE_MAX;  /* whatever the server sends */
> +    s->not_legacy_32bit = 1;
>  }
> 
>  static void ivshmem_doorbell_realize(PCIDevice *dev, Error **errp)
> --
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v2 1/2] ivshmem: fix misconfig of not_legacy_32bit

[Markus Armbruster]

Zhuangyanying <ann.zhuangyanying@huawei.com> writes:

> From: ZhuangYanying <ann.zhuangyanying@huawei.com>
>
> After commit 5400c02, ivshmem_64bit renamed to not_legacy_32bit, 
> and changed the implementation of this property.
> Then use64 = 1, ~PCI_BASE_ADDRESS_MEM_TYPE_64 (default for ivshmem),
> the actual use is the legacy model, 
> can not support greater than or equal 1G mapping,
> which is the opposite of configuration requirements.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Zhuang Yanying <ann.zhuangyanying@huawei.com>
> Reviewed-by: Gonglei <arei.gonglei@huawei.com>

I find the commit message hard to understand.  To improve it, let's
figure out what exactly is broken.

The sense of device ivshmem's property use64 is reverted: use64=0
enables the 64 bit memory bar, use64=1 disables it.  Default is 32 bit
unless pc-1.2 or older.  This regressed in commit 5400c02.  Before,
use64 had the correct sense, and the default was 64 bit unless pc-1.2 or
older.

Note that devices ivshmem-plain and ivshmem-doorbell always have a 64
bit memory bar, as intended.

> ---
>  hw/misc/ivshmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 230e51b..b897685 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -858,7 +858,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>      pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY,
>                       &s->ivshmem_mmio);
>  
> -    if (!s->not_legacy_32bit) {
> +    if (s->not_legacy_32bit) {
>          attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>      }

Thanks a lot for catching my mistake!

However, this breaks ivshmem-plain and ivshmem-doorbell.  You fix that
in the next patch, but that's no good, the two need to be squashed into
a single commit.

Suggested commit message for the squashed commit:

    ivshmem: Fix 64 bit memory bar configuration

    Device ivshmem property use64=0 is designed to make the device
    expose a 32 bit shared memory BAR instead of 64 bit one.  The
    default is a 64 bit BAR, except pc-1.2 and older retain a 32 bit
    BAR.  A 32 bit BAR can support only up to 1 GiB of shared memory.

    This worked as designed until commit 5400c02 accidentally flipped
    its sense: since then, we misinterpret use64=0 as use64=1 and vice
    versa.  Worse, the default got flipped as well.  Devices
    ivshmem-plain and ivshmem-doorbell are not affected.

    Fix by restoring the test of IVShmemState member not_legacy_32bit
    that got messed up in commit 5400c02.  Also update its
    initialization for devices ivhsmem-plain and ivshmem-doorbell.
    Without that, they'd regress to 32 bit BARs.

    Cc: qemu-stable@nongnu.org

Would that work for you?