[libvirt] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device

Thomas Huth posted 1 patch 5 years, 4 months ago
Failed in applying to current master (apply log)
docs/specs/ivshmem-spec.txt |   8 +-
hw/i386/pc_piix.c           |   4 -
hw/misc/ivshmem.c           | 206 +-------------------------------------------
qemu-deprecated.texi        |   5 --
scripts/device-crash-test   |   1 -
tests/ivshmem-test.c        |  65 +++++---------
6 files changed, 29 insertions(+), 260 deletions(-)
[libvirt] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device
Posted by Thomas Huth 5 years, 4 months ago
It's been marked as deprecated in QEMU v2.6.0 already, so really nobody
should use the legacy "ivshmem" device anymore (but use ivshmem-plain or
ivshmem-doorbell instead). Time to remove the deprecated device now.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/specs/ivshmem-spec.txt |   8 +-
 hw/i386/pc_piix.c           |   4 -
 hw/misc/ivshmem.c           | 206 +-------------------------------------------
 qemu-deprecated.texi        |   5 --
 scripts/device-crash-test   |   1 -
 tests/ivshmem-test.c        |  65 +++++---------
 6 files changed, 29 insertions(+), 260 deletions(-)

diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
index a1f5499..042f7ea 100644
--- a/docs/specs/ivshmem-spec.txt
+++ b/docs/specs/ivshmem-spec.txt
@@ -17,12 +17,16 @@ get interrupted by its peers.
 
 There are two basic configurations:
 
-- Just shared memory: -device ivshmem-plain,memdev=HMB,...
+- Just shared memory:
+
+      -device ivshmem-plain,memdev=HMB,...
 
   This uses host memory backend HMB.  It should have option "share"
   set.
 
-- Shared memory plus interrupts: -device ivshmem,chardev=CHR,vectors=N,...
+- Shared memory plus interrupts:
+
+      -device ivshmem-doorbell,chardev=CHR,vectors=N,...
 
   An ivshmem server must already be running on the host.  The device
   connects to the server's UNIX domain socket via character device
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 7653fbb..5cbe976 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -725,10 +725,6 @@ DEFINE_I440FX_MACHINE(v1_3, "pc-1.3", pc_compat_1_3,
             .property = "msix",\
             .value    = "off",\
         },{\
-            .driver   = "ivshmem",\
-            .property = "use64",\
-            .value    = "0",\
-        },{\
             .driver   = "qxl",\
             .property = "revision",\
             .value    = stringify(3),\
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 8213659..2ab741f 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -112,13 +112,6 @@ typedef struct IVShmemState {
     /* migration stuff */
     OnOffAuto master;
     Error *migration_blocker;
-
-    /* legacy cruft */
-    char *role;
-    char *shmobj;
-    char *sizearg;
-    size_t legacy_size;
-    uint32_t not_legacy_32bit;
 } IVShmemState;
 
 /* registers for the Inter-VM shared memory device */
@@ -529,17 +522,6 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
 
     size = buf.st_size;
 
-    /* Legacy cruft */
-    if (s->legacy_size != SIZE_MAX) {
-        if (size < s->legacy_size) {
-            error_setg(errp, "server sent only %zd bytes of shared memory",
-                       (size_t)buf.st_size);
-            close(fd);
-            return;
-        }
-        size = s->legacy_size;
-    }
-
     /* mmap the region and map into the BAR2 */
     memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s),
                                    "ivshmem.bar2", size, true, fd, &local_err);
@@ -882,8 +864,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
     IVShmemState *s = IVSHMEM_COMMON(dev);
     Error *err = NULL;
     uint8_t *pci_conf;
-    uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
-        PCI_BASE_ADDRESS_MEM_PREFETCH;
+    const uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
+        PCI_BASE_ADDRESS_MEM_PREFETCH | PCI_BASE_ADDRESS_MEM_TYPE_64;
     Error *local_err = NULL;
 
     /* IRQFD requires MSI */
@@ -903,10 +885,6 @@ 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) {
-        attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
-    }
-
     if (s->hostmem != NULL) {
         IVSHMEM_DPRINTF("using hostmem\n");
 
@@ -1084,13 +1062,6 @@ static Property ivshmem_plain_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ivshmem_plain_init(Object *obj)
-{
-    IVShmemState *s = IVSHMEM_PLAIN(obj);
-
-    s->not_legacy_32bit = 1;
-}
-
 static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
 {
     IVShmemState *s = IVSHMEM_COMMON(dev);
@@ -1122,7 +1093,6 @@ static const TypeInfo ivshmem_plain_info = {
     .name          = TYPE_IVSHMEM_PLAIN,
     .parent        = TYPE_IVSHMEM_COMMON,
     .instance_size = sizeof(IVShmemState),
-    .instance_init = ivshmem_plain_init,
     .class_init    = ivshmem_plain_class_init,
 };
 
@@ -1155,8 +1125,6 @@ static void ivshmem_doorbell_init(Object *obj)
     IVShmemState *s = IVSHMEM_DOORBELL(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)
@@ -1189,181 +1157,11 @@ static const TypeInfo ivshmem_doorbell_info = {
     .class_init    = ivshmem_doorbell_class_init,
 };
 
-static int ivshmem_load_old(QEMUFile *f, void *opaque, int version_id)
-{
-    IVShmemState *s = opaque;
-    PCIDevice *pdev = PCI_DEVICE(s);
-    int ret;
-
-    IVSHMEM_DPRINTF("ivshmem_load_old\n");
-
-    if (version_id != 0) {
-        return -EINVAL;
-    }
-
-    ret = ivshmem_pre_load(s);
-    if (ret) {
-        return ret;
-    }
-
-    ret = pci_device_load(pdev, f);
-    if (ret) {
-        return ret;
-    }
-
-    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
-        msix_load(pdev, f);
-        ivshmem_msix_vector_use(s);
-    } else {
-        s->intrstatus = qemu_get_be32(f);
-        s->intrmask = qemu_get_be32(f);
-    }
-
-    return 0;
-}
-
-static bool test_msix(void *opaque, int version_id)
-{
-    IVShmemState *s = opaque;
-
-    return ivshmem_has_feature(s, IVSHMEM_MSI);
-}
-
-static bool test_no_msix(void *opaque, int version_id)
-{
-    return !test_msix(opaque, version_id);
-}
-
-static const VMStateDescription ivshmem_vmsd = {
-    .name = "ivshmem",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .pre_load = ivshmem_pre_load,
-    .post_load = ivshmem_post_load,
-    .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(parent_obj, IVShmemState),
-
-        VMSTATE_MSIX_TEST(parent_obj, IVShmemState, test_msix),
-        VMSTATE_UINT32_TEST(intrstatus, IVShmemState, test_no_msix),
-        VMSTATE_UINT32_TEST(intrmask, IVShmemState, test_no_msix),
-
-        VMSTATE_END_OF_LIST()
-    },
-    .load_state_old = ivshmem_load_old,
-    .minimum_version_id_old = 0
-};
-
-static Property ivshmem_properties[] = {
-    DEFINE_PROP_CHR("chardev", IVShmemState, server_chr),
-    DEFINE_PROP_STRING("size", IVShmemState, sizearg),
-    DEFINE_PROP_UINT32("vectors", IVShmemState, vectors, 1),
-    DEFINE_PROP_BIT("ioeventfd", IVShmemState, features, IVSHMEM_IOEVENTFD,
-                    false),
-    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, not_legacy_32bit, 1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void desugar_shm(IVShmemState *s)
-{
-    Object *obj;
-    char *path;
-
-    obj = object_new("memory-backend-file");
-    path = g_strdup_printf("/dev/shm/%s", s->shmobj);
-    object_property_set_str(obj, path, "mem-path", &error_abort);
-    g_free(path);
-    object_property_set_int(obj, s->legacy_size, "size", &error_abort);
-    object_property_set_bool(obj, true, "share", &error_abort);
-    object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
-                              &error_abort);
-    object_unref(obj);
-    user_creatable_complete(USER_CREATABLE(obj), &error_abort);
-    s->hostmem = MEMORY_BACKEND(obj);
-}
-
-static void ivshmem_realize(PCIDevice *dev, Error **errp)
-{
-    IVShmemState *s = IVSHMEM_COMMON(dev);
-
-    if (!qtest_enabled()) {
-        warn_report("ivshmem is deprecated, please use ivshmem-plain"
-                    " or ivshmem-doorbell instead");
-    }
-
-    if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) {
-        error_setg(errp, "You must specify either 'shm' or 'chardev'");
-        return;
-    }
-
-    if (s->sizearg == NULL) {
-        s->legacy_size = 4 * MiB; /* 4 MB default */
-    } else {
-        int ret;
-        uint64_t size;
-
-        ret = qemu_strtosz_MiB(s->sizearg, NULL, &size);
-        if (ret < 0 || (size_t)size != size || !is_power_of_2(size)) {
-            error_setg(errp, "Invalid size %s", s->sizearg);
-            return;
-        }
-        s->legacy_size = size;
-    }
-
-    /* check that role is reasonable */
-    if (s->role) {
-        if (strncmp(s->role, "peer", 5) == 0) {
-            s->master = ON_OFF_AUTO_OFF;
-        } else if (strncmp(s->role, "master", 7) == 0) {
-            s->master = ON_OFF_AUTO_ON;
-        } else {
-            error_setg(errp, "'role' must be 'peer' or 'master'");
-            return;
-        }
-    } else {
-        s->master = ON_OFF_AUTO_AUTO;
-    }
-
-    if (s->shmobj) {
-        desugar_shm(s);
-    }
-
-    /*
-     * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a
-     * bald-faced lie then.  But it's a backwards compatible lie.
-     */
-    pci_config_set_interrupt_pin(dev->config, 1);
-
-    ivshmem_common_realize(dev, errp);
-}
-
-static void ivshmem_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = ivshmem_realize;
-    k->revision = 0;
-    dc->desc = "Inter-VM shared memory (legacy)";
-    dc->props = ivshmem_properties;
-    dc->vmsd = &ivshmem_vmsd;
-}
-
-static const TypeInfo ivshmem_info = {
-    .name          = TYPE_IVSHMEM,
-    .parent        = TYPE_IVSHMEM_COMMON,
-    .instance_size = sizeof(IVShmemState),
-    .class_init    = ivshmem_class_init,
-};
-
 static void ivshmem_register_types(void)
 {
     type_register_static(&ivshmem_common_info);
     type_register_static(&ivshmem_plain_info);
     type_register_static(&ivshmem_doorbell_info);
-    type_register_static(&ivshmem_info);
 }
 
 type_init(ivshmem_register_types)
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 190250f..038df3d 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -110,11 +110,6 @@ documentation of ``query-hotpluggable-cpus'' for additional details.
 
 @section System emulator devices
 
-@subsection ivshmem (since 2.6.0)
-
-The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
-or ``ivshmem-doorbell`` device types.
-
 @subsection bluetooth (since 3.1)
 
 The bluetooth subsystem is unmaintained since many years and likely bitrotten
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index e93a7c0..a835772 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -83,7 +83,6 @@ ERROR_WHITELIST = [
     {'device':'isa-ipmi-bt', 'expected':True},             # IPMI device requires a bmc attribute to be set
     {'device':'isa-ipmi-kcs', 'expected':True},            # IPMI device requires a bmc attribute to be set
     {'device':'isa-parallel', 'expected':True},            # Can't create serial device, empty char device
-    {'device':'ivshmem', 'expected':True},                 # You must specify either 'shm' or 'chardev'
     {'device':'ivshmem-doorbell', 'expected':True},        # You must specify a 'chardev'
     {'device':'ivshmem-plain', 'expected':True},           # You must specify a 'memdev'
     {'device':'loader', 'expected':True},                  # please include valid arguments
diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
index c37b196..9811d66 100644
--- a/tests/ivshmem-test.c
+++ b/tests/ivshmem-test.c
@@ -305,20 +305,18 @@ static void *server_thread(void *data)
     return NULL;
 }
 
-static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
+static void setup_vm_with_server(IVState *s, int nvectors)
 {
-    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
-                                "-device ivshmem%s,chardev=chr0,vectors=%d",
-                                tmpserver,
-                                msi ? "-doorbell" : ",size=1M,msi=off",
-                                nvectors);
+    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait -device"
+                                " ivshmem-doorbell,chardev=chr0,vectors=%d",
+                                tmpserver, nvectors);
 
-    setup_vm_cmd(s, cmd, msi);
+    setup_vm_cmd(s, cmd, true);
 
     g_free(cmd);
 }
 
-static void test_ivshmem_server(bool msi)
+static void test_ivshmem_server(void)
 {
     IVState state1, state2, *s1, *s2;
     ServerThread thread;
@@ -341,9 +339,9 @@ static void test_ivshmem_server(bool msi)
     thread.thread = g_thread_new("ivshmem-server", server_thread, &thread);
     g_assert(thread.thread != NULL);
 
-    setup_vm_with_server(&state1, nvectors, msi);
+    setup_vm_with_server(&state1, nvectors);
     s1 = &state1;
-    setup_vm_with_server(&state2, nvectors, msi);
+    setup_vm_with_server(&state2, nvectors);
     s2 = &state2;
 
     /* check got different VM ids */
@@ -355,39 +353,29 @@ static void test_ivshmem_server(bool msi)
 
     /* check number of MSI-X vectors */
     global_qtest = s1->qs->qts;
-    if (msi) {
-        ret = qpci_msix_table_size(s1->dev);
-        g_assert_cmpuint(ret, ==, nvectors);
-    }
+    ret = qpci_msix_table_size(s1->dev);
+    g_assert_cmpuint(ret, ==, nvectors);
 
     /* TODO test behavior before MSI-X is enabled */
 
     /* ping vm2 -> vm1 on vector 0 */
-    if (msi) {
-        ret = qpci_msix_pending(s1->dev, 0);
-        g_assert_cmpuint(ret, ==, 0);
-    } else {
-        g_assert_cmpuint(in_reg(s1, INTRSTATUS), ==, 0);
-    }
+    ret = qpci_msix_pending(s1->dev, 0);
+    g_assert_cmpuint(ret, ==, 0);
     out_reg(s2, DOORBELL, vm1 << 16);
     do {
         g_usleep(10000);
-        ret = msi ? qpci_msix_pending(s1->dev, 0) : in_reg(s1, INTRSTATUS);
+        ret = qpci_msix_pending(s1->dev, 0);
     } while (ret == 0 && g_get_monotonic_time() < end_time);
     g_assert_cmpuint(ret, !=, 0);
 
     /* ping vm1 -> vm2 on vector 1 */
     global_qtest = s2->qs->qts;
-    if (msi) {
-        ret = qpci_msix_pending(s2->dev, 1);
-        g_assert_cmpuint(ret, ==, 0);
-    } else {
-        g_assert_cmpuint(in_reg(s2, INTRSTATUS), ==, 0);
-    }
+    ret = qpci_msix_pending(s2->dev, 1);
+    g_assert_cmpuint(ret, ==, 0);
     out_reg(s1, DOORBELL, vm2 << 16 | 1);
     do {
         g_usleep(10000);
-        ret = msi ? qpci_msix_pending(s2->dev, 1) : in_reg(s2, INTRSTATUS);
+        ret = qpci_msix_pending(s2->dev, 1);
     } while (ret == 0 && g_get_monotonic_time() < end_time);
     g_assert_cmpuint(ret, !=, 0);
 
@@ -405,27 +393,17 @@ static void test_ivshmem_server(bool msi)
     close(thread.pipe[0]);
 }
 
-static void test_ivshmem_server_msi(void)
-{
-    test_ivshmem_server(true);
-}
-
-static void test_ivshmem_server_irq(void)
-{
-    test_ivshmem_server(false);
-}
-
 #define PCI_SLOT_HP             0x06
 
 static void test_ivshmem_hotplug(void)
 {
     const char *arch = qtest_get_arch();
 
-    qtest_start("");
+    qtest_start("-object memory-backend-ram,size=1M,id=mb1");
 
-    qtest_qmp_device_add("ivshmem",
-                         "iv1", "{'addr': %s, 'shm': %s, 'size': '1M'}",
-                         stringify(PCI_SLOT_HP), tmpshm);
+    qtest_qmp_device_add("ivshmem-plain", "iv1",
+                         "{'addr': %s, 'memdev': 'mb1'}",
+                         stringify(PCI_SLOT_HP));
     if (strcmp(arch, "ppc64") != 0) {
         qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
     }
@@ -525,8 +503,7 @@ int main(int argc, char **argv)
     if (g_test_slow()) {
         qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
         if (strcmp(arch, "ppc64") != 0) {
-            qtest_add_func("/ivshmem/server-msi", test_ivshmem_server_msi);
-            qtest_add_func("/ivshmem/server-irq", test_ivshmem_server_irq);
+            qtest_add_func("/ivshmem/server", test_ivshmem_server);
         }
     }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device
Posted by Markus Armbruster 5 years, 4 months ago
Thomas Huth <thuth@redhat.com> writes:

> It's been marked as deprecated in QEMU v2.6.0 already, so really nobody
> should use the legacy "ivshmem" device anymore (but use ivshmem-plain or
> ivshmem-doorbell instead). Time to remove the deprecated device now.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  docs/specs/ivshmem-spec.txt |   8 +-
>  hw/i386/pc_piix.c           |   4 -
>  hw/misc/ivshmem.c           | 206 +-------------------------------------------
>  qemu-deprecated.texi        |   5 --
>  scripts/device-crash-test   |   1 -
>  tests/ivshmem-test.c        |  65 +++++---------
>  6 files changed, 29 insertions(+), 260 deletions(-)
>
> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
> index a1f5499..042f7ea 100644
> --- a/docs/specs/ivshmem-spec.txt
> +++ b/docs/specs/ivshmem-spec.txt
> @@ -17,12 +17,16 @@ get interrupted by its peers.
>  
>  There are two basic configurations:
>  
> -- Just shared memory: -device ivshmem-plain,memdev=HMB,...
> +- Just shared memory:
> +
> +      -device ivshmem-plain,memdev=HMB,...
>  
>    This uses host memory backend HMB.  It should have option "share"
>    set.
>  
> -- Shared memory plus interrupts: -device ivshmem,chardev=CHR,vectors=N,...
> +- Shared memory plus interrupts:
> +
> +      -device ivshmem-doorbell,chardev=CHR,vectors=N,...
>  
>    An ivshmem server must already be running on the host.  The device
>    connects to the server's UNIX domain socket via character device

Just whitespace.  Intentional?

> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 7653fbb..5cbe976 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -725,10 +725,6 @@ DEFINE_I440FX_MACHINE(v1_3, "pc-1.3", pc_compat_1_3,
>              .property = "msix",\
>              .value    = "off",\
>          },{\
> -            .driver   = "ivshmem",\
> -            .property = "use64",\
> -            .value    = "0",\
> -        },{\
>              .driver   = "qxl",\
>              .property = "revision",\
>              .value    = stringify(3),\
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 8213659..2ab741f 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -112,13 +112,6 @@ typedef struct IVShmemState {
>      /* migration stuff */
>      OnOffAuto master;
>      Error *migration_blocker;
> -
> -    /* legacy cruft */
> -    char *role;
> -    char *shmobj;
> -    char *sizearg;
> -    size_t legacy_size;
> -    uint32_t not_legacy_32bit;
>  } IVShmemState;
>  
>  /* registers for the Inter-VM shared memory device */
> @@ -529,17 +522,6 @@ static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
>  
>      size = buf.st_size;
>  
> -    /* Legacy cruft */
> -    if (s->legacy_size != SIZE_MAX) {
> -        if (size < s->legacy_size) {
> -            error_setg(errp, "server sent only %zd bytes of shared memory",
> -                       (size_t)buf.st_size);
> -            close(fd);
> -            return;
> -        }
> -        size = s->legacy_size;
> -    }
> -
>      /* mmap the region and map into the BAR2 */
>      memory_region_init_ram_from_fd(&s->server_bar2, OBJECT(s),
>                                     "ivshmem.bar2", size, true, fd, &local_err);
> @@ -882,8 +864,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>      IVShmemState *s = IVSHMEM_COMMON(dev);
>      Error *err = NULL;
>      uint8_t *pci_conf;
> -    uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
> -        PCI_BASE_ADDRESS_MEM_PREFETCH;
> +    const uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
> +        PCI_BASE_ADDRESS_MEM_PREFETCH | PCI_BASE_ADDRESS_MEM_TYPE_64;
>      Error *local_err = NULL;
>  
>      /* IRQFD requires MSI */

Sure this belongs to this patch?

> @@ -903,10 +885,6 @@ 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) {
> -        attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> -    }
> -
>      if (s->hostmem != NULL) {
>          IVSHMEM_DPRINTF("using hostmem\n");
>  
> @@ -1084,13 +1062,6 @@ static Property ivshmem_plain_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void ivshmem_plain_init(Object *obj)
> -{
> -    IVShmemState *s = IVSHMEM_PLAIN(obj);
> -
> -    s->not_legacy_32bit = 1;
> -}
> -
>  static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
>  {
>      IVShmemState *s = IVSHMEM_COMMON(dev);
> @@ -1122,7 +1093,6 @@ static const TypeInfo ivshmem_plain_info = {
>      .name          = TYPE_IVSHMEM_PLAIN,
>      .parent        = TYPE_IVSHMEM_COMMON,
>      .instance_size = sizeof(IVShmemState),
> -    .instance_init = ivshmem_plain_init,
>      .class_init    = ivshmem_plain_class_init,
>  };
>  
> @@ -1155,8 +1125,6 @@ static void ivshmem_doorbell_init(Object *obj)
>      IVShmemState *s = IVSHMEM_DOORBELL(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)
> @@ -1189,181 +1157,11 @@ static const TypeInfo ivshmem_doorbell_info = {
>      .class_init    = ivshmem_doorbell_class_init,
>  };
>  
> -static int ivshmem_load_old(QEMUFile *f, void *opaque, int version_id)
> -{
> -    IVShmemState *s = opaque;
> -    PCIDevice *pdev = PCI_DEVICE(s);
> -    int ret;
> -
> -    IVSHMEM_DPRINTF("ivshmem_load_old\n");
> -
> -    if (version_id != 0) {
> -        return -EINVAL;
> -    }
> -
> -    ret = ivshmem_pre_load(s);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    ret = pci_device_load(pdev, f);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    if (ivshmem_has_feature(s, IVSHMEM_MSI)) {
> -        msix_load(pdev, f);
> -        ivshmem_msix_vector_use(s);
> -    } else {
> -        s->intrstatus = qemu_get_be32(f);
> -        s->intrmask = qemu_get_be32(f);
> -    }
> -
> -    return 0;
> -}
> -
> -static bool test_msix(void *opaque, int version_id)
> -{
> -    IVShmemState *s = opaque;
> -
> -    return ivshmem_has_feature(s, IVSHMEM_MSI);
> -}
> -
> -static bool test_no_msix(void *opaque, int version_id)
> -{
> -    return !test_msix(opaque, version_id);
> -}
> -
> -static const VMStateDescription ivshmem_vmsd = {
> -    .name = "ivshmem",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .pre_load = ivshmem_pre_load,
> -    .post_load = ivshmem_post_load,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(parent_obj, IVShmemState),
> -
> -        VMSTATE_MSIX_TEST(parent_obj, IVShmemState, test_msix),
> -        VMSTATE_UINT32_TEST(intrstatus, IVShmemState, test_no_msix),
> -        VMSTATE_UINT32_TEST(intrmask, IVShmemState, test_no_msix),
> -
> -        VMSTATE_END_OF_LIST()
> -    },
> -    .load_state_old = ivshmem_load_old,
> -    .minimum_version_id_old = 0
> -};
> -
> -static Property ivshmem_properties[] = {
> -    DEFINE_PROP_CHR("chardev", IVShmemState, server_chr),
> -    DEFINE_PROP_STRING("size", IVShmemState, sizearg),
> -    DEFINE_PROP_UINT32("vectors", IVShmemState, vectors, 1),
> -    DEFINE_PROP_BIT("ioeventfd", IVShmemState, features, IVSHMEM_IOEVENTFD,
> -                    false),
> -    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, not_legacy_32bit, 1),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
> -static void desugar_shm(IVShmemState *s)
> -{
> -    Object *obj;
> -    char *path;
> -
> -    obj = object_new("memory-backend-file");
> -    path = g_strdup_printf("/dev/shm/%s", s->shmobj);
> -    object_property_set_str(obj, path, "mem-path", &error_abort);
> -    g_free(path);
> -    object_property_set_int(obj, s->legacy_size, "size", &error_abort);
> -    object_property_set_bool(obj, true, "share", &error_abort);
> -    object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
> -                              &error_abort);
> -    object_unref(obj);
> -    user_creatable_complete(USER_CREATABLE(obj), &error_abort);
> -    s->hostmem = MEMORY_BACKEND(obj);
> -}
> -
> -static void ivshmem_realize(PCIDevice *dev, Error **errp)
> -{
> -    IVShmemState *s = IVSHMEM_COMMON(dev);
> -
> -    if (!qtest_enabled()) {
> -        warn_report("ivshmem is deprecated, please use ivshmem-plain"
> -                    " or ivshmem-doorbell instead");
> -    }
> -
> -    if (qemu_chr_fe_backend_connected(&s->server_chr) + !!s->shmobj != 1) {
> -        error_setg(errp, "You must specify either 'shm' or 'chardev'");
> -        return;
> -    }
> -
> -    if (s->sizearg == NULL) {
> -        s->legacy_size = 4 * MiB; /* 4 MB default */
> -    } else {
> -        int ret;
> -        uint64_t size;
> -
> -        ret = qemu_strtosz_MiB(s->sizearg, NULL, &size);
> -        if (ret < 0 || (size_t)size != size || !is_power_of_2(size)) {
> -            error_setg(errp, "Invalid size %s", s->sizearg);
> -            return;
> -        }
> -        s->legacy_size = size;
> -    }
> -
> -    /* check that role is reasonable */
> -    if (s->role) {
> -        if (strncmp(s->role, "peer", 5) == 0) {
> -            s->master = ON_OFF_AUTO_OFF;
> -        } else if (strncmp(s->role, "master", 7) == 0) {
> -            s->master = ON_OFF_AUTO_ON;
> -        } else {
> -            error_setg(errp, "'role' must be 'peer' or 'master'");
> -            return;
> -        }
> -    } else {
> -        s->master = ON_OFF_AUTO_AUTO;
> -    }
> -
> -    if (s->shmobj) {
> -        desugar_shm(s);
> -    }
> -
> -    /*
> -     * Note: we don't use INTx with IVSHMEM_MSI at all, so this is a
> -     * bald-faced lie then.  But it's a backwards compatible lie.
> -     */
> -    pci_config_set_interrupt_pin(dev->config, 1);
> -
> -    ivshmem_common_realize(dev, errp);
> -}
> -
> -static void ivshmem_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->realize = ivshmem_realize;
> -    k->revision = 0;
> -    dc->desc = "Inter-VM shared memory (legacy)";
> -    dc->props = ivshmem_properties;
> -    dc->vmsd = &ivshmem_vmsd;
> -}
> -
> -static const TypeInfo ivshmem_info = {
> -    .name          = TYPE_IVSHMEM,
> -    .parent        = TYPE_IVSHMEM_COMMON,
> -    .instance_size = sizeof(IVShmemState),
> -    .class_init    = ivshmem_class_init,
> -};
> -
>  static void ivshmem_register_types(void)
>  {
>      type_register_static(&ivshmem_common_info);
>      type_register_static(&ivshmem_plain_info);
>      type_register_static(&ivshmem_doorbell_info);
> -    type_register_static(&ivshmem_info);
>  }
>  
>  type_init(ivshmem_register_types)
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 190250f..038df3d 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -110,11 +110,6 @@ documentation of ``query-hotpluggable-cpus'' for additional details.
>  
>  @section System emulator devices
>  
> -@subsection ivshmem (since 2.6.0)
> -
> -The ``ivshmem'' device type is replaced by either the ``ivshmem-plain''
> -or ``ivshmem-doorbell`` device types.
> -
>  @subsection bluetooth (since 3.1)
>  
>  The bluetooth subsystem is unmaintained since many years and likely bitrotten
> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> index e93a7c0..a835772 100755
> --- a/scripts/device-crash-test
> +++ b/scripts/device-crash-test
> @@ -83,7 +83,6 @@ ERROR_WHITELIST = [
>      {'device':'isa-ipmi-bt', 'expected':True},             # IPMI device requires a bmc attribute to be set
>      {'device':'isa-ipmi-kcs', 'expected':True},            # IPMI device requires a bmc attribute to be set
>      {'device':'isa-parallel', 'expected':True},            # Can't create serial device, empty char device
> -    {'device':'ivshmem', 'expected':True},                 # You must specify either 'shm' or 'chardev'
>      {'device':'ivshmem-doorbell', 'expected':True},        # You must specify a 'chardev'
>      {'device':'ivshmem-plain', 'expected':True},           # You must specify a 'memdev'
>      {'device':'loader', 'expected':True},                  # please include valid arguments
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index c37b196..9811d66 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -305,20 +305,18 @@ static void *server_thread(void *data)
>      return NULL;
>  }
>  
> -static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
> +static void setup_vm_with_server(IVState *s, int nvectors)
>  {
> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
> -                                "-device ivshmem%s,chardev=chr0,vectors=%d",
> -                                tmpserver,
> -                                msi ? "-doorbell" : ",size=1M,msi=off",
> -                                nvectors);
> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait -device"

Awkward line break.

> +                                " ivshmem-doorbell,chardev=chr0,vectors=%d",
> +                                tmpserver, nvectors);

Suggest

       char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
                       "-device ivshmem-doorbell,chardev=chr0,vectors=%d",
                       tmpserver, nvectors);

>  
> -    setup_vm_cmd(s, cmd, msi);
> +    setup_vm_cmd(s, cmd, true);
>  
>      g_free(cmd);
>  }
>  
> -static void test_ivshmem_server(bool msi)
> +static void test_ivshmem_server(void)
>  {
>      IVState state1, state2, *s1, *s2;
>      ServerThread thread;
> @@ -341,9 +339,9 @@ static void test_ivshmem_server(bool msi)
>      thread.thread = g_thread_new("ivshmem-server", server_thread, &thread);
>      g_assert(thread.thread != NULL);
>  
> -    setup_vm_with_server(&state1, nvectors, msi);
> +    setup_vm_with_server(&state1, nvectors);
>      s1 = &state1;
> -    setup_vm_with_server(&state2, nvectors, msi);
> +    setup_vm_with_server(&state2, nvectors);
>      s2 = &state2;
>  
>      /* check got different VM ids */
> @@ -355,39 +353,29 @@ static void test_ivshmem_server(bool msi)
>  
>      /* check number of MSI-X vectors */
>      global_qtest = s1->qs->qts;
> -    if (msi) {
> -        ret = qpci_msix_table_size(s1->dev);
> -        g_assert_cmpuint(ret, ==, nvectors);
> -    }
> +    ret = qpci_msix_table_size(s1->dev);
> +    g_assert_cmpuint(ret, ==, nvectors);
>  
>      /* TODO test behavior before MSI-X is enabled */
>  
>      /* ping vm2 -> vm1 on vector 0 */
> -    if (msi) {
> -        ret = qpci_msix_pending(s1->dev, 0);
> -        g_assert_cmpuint(ret, ==, 0);
> -    } else {
> -        g_assert_cmpuint(in_reg(s1, INTRSTATUS), ==, 0);
> -    }
> +    ret = qpci_msix_pending(s1->dev, 0);
> +    g_assert_cmpuint(ret, ==, 0);
>      out_reg(s2, DOORBELL, vm1 << 16);
>      do {
>          g_usleep(10000);
> -        ret = msi ? qpci_msix_pending(s1->dev, 0) : in_reg(s1, INTRSTATUS);
> +        ret = qpci_msix_pending(s1->dev, 0);
>      } while (ret == 0 && g_get_monotonic_time() < end_time);
>      g_assert_cmpuint(ret, !=, 0);
>  
>      /* ping vm1 -> vm2 on vector 1 */
>      global_qtest = s2->qs->qts;
> -    if (msi) {
> -        ret = qpci_msix_pending(s2->dev, 1);
> -        g_assert_cmpuint(ret, ==, 0);
> -    } else {
> -        g_assert_cmpuint(in_reg(s2, INTRSTATUS), ==, 0);
> -    }
> +    ret = qpci_msix_pending(s2->dev, 1);
> +    g_assert_cmpuint(ret, ==, 0);
>      out_reg(s1, DOORBELL, vm2 << 16 | 1);
>      do {
>          g_usleep(10000);
> -        ret = msi ? qpci_msix_pending(s2->dev, 1) : in_reg(s2, INTRSTATUS);
> +        ret = qpci_msix_pending(s2->dev, 1);
>      } while (ret == 0 && g_get_monotonic_time() < end_time);
>      g_assert_cmpuint(ret, !=, 0);
>  
> @@ -405,27 +393,17 @@ static void test_ivshmem_server(bool msi)
>      close(thread.pipe[0]);
>  }
>  
> -static void test_ivshmem_server_msi(void)
> -{
> -    test_ivshmem_server(true);
> -}
> -
> -static void test_ivshmem_server_irq(void)
> -{
> -    test_ivshmem_server(false);
> -}
> -
>  #define PCI_SLOT_HP             0x06
>  
>  static void test_ivshmem_hotplug(void)
>  {
>      const char *arch = qtest_get_arch();
>  
> -    qtest_start("");
> +    qtest_start("-object memory-backend-ram,size=1M,id=mb1");
>  
> -    qtest_qmp_device_add("ivshmem",
> -                         "iv1", "{'addr': %s, 'shm': %s, 'size': '1M'}",
> -                         stringify(PCI_SLOT_HP), tmpshm);
> +    qtest_qmp_device_add("ivshmem-plain", "iv1",
> +                         "{'addr': %s, 'memdev': 'mb1'}",
> +                         stringify(PCI_SLOT_HP));
>      if (strcmp(arch, "ppc64") != 0) {
>          qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP);
>      }
> @@ -525,8 +503,7 @@ int main(int argc, char **argv)
>      if (g_test_slow()) {
>          qtest_add_func("/ivshmem/pair", test_ivshmem_pair);
>          if (strcmp(arch, "ppc64") != 0) {
> -            qtest_add_func("/ivshmem/server-msi", test_ivshmem_server_msi);
> -            qtest_add_func("/ivshmem/server-irq", test_ivshmem_server_irq);
> +            qtest_add_func("/ivshmem/server", test_ivshmem_server);
>          }
>      }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device
Posted by Thomas Huth 5 years, 4 months ago
On 2018-12-18 18:50, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> It's been marked as deprecated in QEMU v2.6.0 already, so really nobody
>> should use the legacy "ivshmem" device anymore (but use ivshmem-plain or
>> ivshmem-doorbell instead). Time to remove the deprecated device now.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  docs/specs/ivshmem-spec.txt |   8 +-
>>  hw/i386/pc_piix.c           |   4 -
>>  hw/misc/ivshmem.c           | 206 +-------------------------------------------
>>  qemu-deprecated.texi        |   5 --
>>  scripts/device-crash-test   |   1 -
>>  tests/ivshmem-test.c        |  65 +++++---------
>>  6 files changed, 29 insertions(+), 260 deletions(-)
>>
>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>> index a1f5499..042f7ea 100644
>> --- a/docs/specs/ivshmem-spec.txt
>> +++ b/docs/specs/ivshmem-spec.txt
>> @@ -17,12 +17,16 @@ get interrupted by its peers.
>>  
>>  There are two basic configurations:
>>  
>> -- Just shared memory: -device ivshmem-plain,memdev=HMB,...
>> +- Just shared memory:
>> +
>> +      -device ivshmem-plain,memdev=HMB,...
>>  
>>    This uses host memory backend HMB.  It should have option "share"
>>    set.
>>  
>> -- Shared memory plus interrupts: -device ivshmem,chardev=CHR,vectors=N,...
>> +- Shared memory plus interrupts:
>> +
>> +      -device ivshmem-doorbell,chardev=CHR,vectors=N,...
>>  
>>    An ivshmem server must already be running on the host.  The device
>>    connects to the server's UNIX domain socket via character device
> 
> Just whitespace.  Intentional?

It's not just whitespace, I had to change "-device ivshmem" into
"-device ivshmem-doorbell" here, and that did not fit into one line anymore.

>> @@ -882,8 +864,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>>      IVShmemState *s = IVSHMEM_COMMON(dev);
>>      Error *err = NULL;
>>      uint8_t *pci_conf;
>> -    uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>> -        PCI_BASE_ADDRESS_MEM_PREFETCH;
>> +    const uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>> +        PCI_BASE_ADDRESS_MEM_PREFETCH | PCI_BASE_ADDRESS_MEM_TYPE_64;
>>      Error *local_err = NULL;
>>  
>>      /* IRQFD requires MSI */
> 
> Sure this belongs to this patch?

Yes. I had to remove the "if (s->not_legacy_32bit)" below, so I moved
the PCI_BASE_ADDRESS_MEM_TYPE_64 to the other bits above. I could also
leave it below, but I think that would look a little bit lonely there
without the if-statement.

>> @@ -903,10 +885,6 @@ 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) {
>> -        attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>> -    }
>> -
>>      if (s->hostmem != NULL) {
>>          IVSHMEM_DPRINTF("using hostmem\n");
>>  
[...]
>> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
>> index c37b196..9811d66 100644
>> --- a/tests/ivshmem-test.c
>> +++ b/tests/ivshmem-test.c
>> @@ -305,20 +305,18 @@ static void *server_thread(void *data)
>>      return NULL;
>>  }
>>  
>> -static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>> +static void setup_vm_with_server(IVState *s, int nvectors)
>>  {
>> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>> -                                "-device ivshmem%s,chardev=chr0,vectors=%d",
>> -                                tmpserver,
>> -                                msi ? "-doorbell" : ",size=1M,msi=off",
>> -                                nvectors);
>> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait -device"
> 
> Awkward line break.
> 
>> +                                " ivshmem-doorbell,chardev=chr0,vectors=%d",
>> +                                tmpserver, nvectors);
> 
> Suggest
> 
>        char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>                        "-device ivshmem-doorbell,chardev=chr0,vectors=%d",
>                        tmpserver, nvectors);

Ok, I can do that in v2.

 Thomas

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [Qemu-devel] [QEMU PATCH] hw/misc/ivshmem: Remove deprecated "ivshmem" legacy device
Posted by Markus Armbruster 5 years, 4 months ago
Thomas Huth <thuth@redhat.com> writes:

> On 2018-12-18 18:50, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> It's been marked as deprecated in QEMU v2.6.0 already, so really nobody
>>> should use the legacy "ivshmem" device anymore (but use ivshmem-plain or
>>> ivshmem-doorbell instead). Time to remove the deprecated device now.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  docs/specs/ivshmem-spec.txt |   8 +-
>>>  hw/i386/pc_piix.c           |   4 -
>>>  hw/misc/ivshmem.c           | 206 +-------------------------------------------
>>>  qemu-deprecated.texi        |   5 --
>>>  scripts/device-crash-test   |   1 -
>>>  tests/ivshmem-test.c        |  65 +++++---------
>>>  6 files changed, 29 insertions(+), 260 deletions(-)
>>>
>>> diff --git a/docs/specs/ivshmem-spec.txt b/docs/specs/ivshmem-spec.txt
>>> index a1f5499..042f7ea 100644
>>> --- a/docs/specs/ivshmem-spec.txt
>>> +++ b/docs/specs/ivshmem-spec.txt
>>> @@ -17,12 +17,16 @@ get interrupted by its peers.
>>>  
>>>  There are two basic configurations:
>>>  
>>> -- Just shared memory: -device ivshmem-plain,memdev=HMB,...
>>> +- Just shared memory:
>>> +
>>> +      -device ivshmem-plain,memdev=HMB,...
>>>  
>>>    This uses host memory backend HMB.  It should have option "share"
>>>    set.
>>>  
>>> -- Shared memory plus interrupts: -device ivshmem,chardev=CHR,vectors=N,...
>>> +- Shared memory plus interrupts:
>>> +
>>> +      -device ivshmem-doorbell,chardev=CHR,vectors=N,...
>>>  
>>>    An ivshmem server must already be running on the host.  The device
>>>    connects to the server's UNIX domain socket via character device
>> 
>> Just whitespace.  Intentional?
>
> It's not just whitespace, I had to change "-device ivshmem" into
> "-device ivshmem-doorbell" here, and that did not fit into one line anymore.

Oww, that should've been done in commit 5400c02b90b.  Turns out I'm not
only blind to mistakes in my own texts, I'm also blind to the fixes.
Suggest to add to the commit message:

    Belatedly update a mention of the deprecated "ivshmem" in
    docs/specs/ivshmem-spec.txt to "ivshmem-doorbell".  Missed in commit
    5400c02b90b.

>>> @@ -882,8 +864,8 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>>>      IVShmemState *s = IVSHMEM_COMMON(dev);
>>>      Error *err = NULL;
>>>      uint8_t *pci_conf;
>>> -    uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>>> -        PCI_BASE_ADDRESS_MEM_PREFETCH;
>>> +    const uint8_t attr = PCI_BASE_ADDRESS_SPACE_MEMORY |
>>> +        PCI_BASE_ADDRESS_MEM_PREFETCH | PCI_BASE_ADDRESS_MEM_TYPE_64;
>>>      Error *local_err = NULL;
>>>  
>>>      /* IRQFD requires MSI */
>> 
>> Sure this belongs to this patch?
>
> Yes. I had to remove the "if (s->not_legacy_32bit)" below, so I moved
> the PCI_BASE_ADDRESS_MEM_TYPE_64 to the other bits above. I could also
> leave it below, but I think that would look a little bit lonely there
> without the if-statement.

I missed that part over the addition of const; I should've read more
closely.

I don't care for const here, but I don't really mind it either.  But
let's eliminate the variable instead.  It's used just once, and there's
considerable distance.

>>> @@ -903,10 +885,6 @@ 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) {
>>> -        attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>>> -    }
>>> -
>>>      if (s->hostmem != NULL) {
>>>          IVSHMEM_DPRINTF("using hostmem\n");
>>>  
> [...]
>>> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
>>> index c37b196..9811d66 100644
>>> --- a/tests/ivshmem-test.c
>>> +++ b/tests/ivshmem-test.c
>>> @@ -305,20 +305,18 @@ static void *server_thread(void *data)
>>>      return NULL;
>>>  }
>>>  
>>> -static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>>> +static void setup_vm_with_server(IVState *s, int nvectors)
>>>  {
>>> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>>> -                                "-device ivshmem%s,chardev=chr0,vectors=%d",
>>> -                                tmpserver,
>>> -                                msi ? "-doorbell" : ",size=1M,msi=off",
>>> -                                nvectors);
>>> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait -device"
>> 
>> Awkward line break.
>> 
>>> +                                " ivshmem-doorbell,chardev=chr0,vectors=%d",
>>> +                                tmpserver, nvectors);
>> 
>> Suggest
>> 
>>        char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
>>                        "-device ivshmem-doorbell,chardev=chr0,vectors=%d",
>>                        tmpserver, nvectors);
>
> Ok, I can do that in v2.
>
>  Thomas

Thanks!

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list