| Message ID | 20170911172022.4738-11-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Preliminary libqtest cleanups | expand |
On 11.09.2017 19:19, Eric Blake wrote: > Commit 2f8b2767 originally added qpci_plug_device_test() and > qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. > Later, commit cf716b31 moved one half of the pair to pci.c > when adding PPC64 support. Keep the implementations of the > two functions together, and shorten the name to > qpci_unplug_device_test(), since all callers use the two > functions in tandem. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/libqos/pci.h | 2 +- > tests/e1000e-test.c | 2 +- > tests/ivshmem-test.c | 2 +- > tests/libqos/pci-pc.c | 23 ----------------------- > tests/libqos/pci.c | 23 +++++++++++++++++++++++ > tests/virtio-blk-test.c | 2 +- > tests/virtio-net-test.c | 2 +- > tests/virtio-rng-test.c | 2 +- > 8 files changed, 29 insertions(+), 29 deletions(-) > > diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h > index 429c382282..fdda7eca6e 100644 > --- a/tests/libqos/pci.h > +++ b/tests/libqos/pci.h > @@ -111,5 +111,5 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr); > > void qpci_plug_device_test(const char *driver, const char *id, > uint8_t slot, const char *opts); > -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot); > +void qpci_unplug_device_test(const char *id, uint8_t slot); > #endif > diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c > index d8085d944e..4c663a3019 100644 > --- a/tests/e1000e-test.c > +++ b/tests/e1000e-test.c > @@ -461,7 +461,7 @@ static void test_e1000e_hotplug(gconstpointer data) > qtest_start("-device e1000e"); > > qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL); > - qpci_unplug_acpi_device_test("e1000e_net", slot); > + qpci_unplug_device_test("e1000e_net", slot); > > qtest_end(); > } > diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c > index 37763425ee..8c9ed6a568 100644 > --- a/tests/ivshmem-test.c > +++ b/tests/ivshmem-test.c > @@ -427,7 +427,7 @@ static void test_ivshmem_hotplug(void) > > qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts); > if (strcmp(arch, "ppc64") != 0) { > - qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP); > + qpci_unplug_device_test("iv1", PCI_SLOT_HP); > } > > qtest_end(); > diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c > index e267fd1a44..6305d142a5 100644 > --- a/tests/libqos/pci-pc.c > +++ b/tests/libqos/pci-pc.c > @@ -19,9 +19,6 @@ > #include "qemu-common.h" > > > -#define ACPI_PCIHP_ADDR 0xae00 > -#define PCI_EJ_BASE 0x0008 > - > typedef struct QPCIBusPC > { > QPCIBus bus; > @@ -156,23 +153,3 @@ void qpci_free_pc(QPCIBus *bus) > > g_free(s); > } > - > -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot) > -{ > - QDict *response; > - char *cmd; > - > - cmd = g_strdup_printf("{'execute': 'device_del'," > - " 'arguments': {" > - " 'id': '%s'" > - "}}", id); > - response = qmp(cmd); > - g_free(cmd); > - g_assert(response); > - g_assert(!qdict_haskey(response, "error")); > - QDECREF(response); > - > - outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot); > - > - qmp_eventwait("DEVICE_DELETED"); > -} > diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c > index 2dcdeade2a..9f36ec73ef 100644 > --- a/tests/libqos/pci.c > +++ b/tests/libqos/pci.c > @@ -16,6 +16,9 @@ > #include "hw/pci/pci_regs.h" > #include "qemu/host-utils.h" > > +#define ACPI_PCIHP_ADDR 0xae00 > +#define PCI_EJ_BASE 0x0008 > + > void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id, > void (*func)(QPCIDevice *dev, int devfn, void *data), > void *data) > @@ -412,3 +415,23 @@ void qpci_plug_device_test(const char *driver, const char *id, > g_assert(!qdict_haskey(response, "error")); > QDECREF(response); > } > + > +void qpci_unplug_device_test(const char *id, uint8_t slot) > +{ > + QDict *response; > + char *cmd; > + > + cmd = g_strdup_printf("{'execute': 'device_del'," > + " 'arguments': {" > + " 'id': '%s'" > + "}}", id); > + response = qmp(cmd); > + g_free(cmd); > + g_assert(response); > + g_assert(!qdict_haskey(response, "error")); > + QDECREF(response); > + > + outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot); > + > + qmp_eventwait("DEVICE_DELETED"); > +} No, that's a bad idea. ACPI and that outb() is clearly something specific to x86, so this should not reside in pci.c but in pci-pc.c We might be able to unify this - I've had a similar patch here: https://patchwork.kernel.org/patch/9905031/ ... but I think this needs some more careful thinking and discussion, so I'd suggest that you remove this from your already huge patch series for now and we fix it later instead. Thomas
On 09/12/2017 02:29 AM, Thomas Huth wrote: > On 11.09.2017 19:19, Eric Blake wrote: >> Commit 2f8b2767 originally added qpci_plug_device_test() and >> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. >> Later, commit cf716b31 moved one half of the pair to pci.c >> when adding PPC64 support. Keep the implementations of the >> two functions together, and shorten the name to >> qpci_unplug_device_test(), since all callers use the two >> functions in tandem. >> > > No, that's a bad idea. ACPI and that outb() is clearly something > specific to x86, so this should not reside in pci.c but in pci-pc.c > > We might be able to unify this - I've had a similar patch here: > > https://patchwork.kernel.org/patch/9905031/ > > ... but I think this needs some more careful thinking and discussion, so > I'd suggest that you remove this from your already huge patch series for > now and we fix it later instead. Okay, I'm fine dropping this patch, and can base my respin on top of your cleanup instead.
On 12.09.2017 15:28, Eric Blake wrote: > On 09/12/2017 02:29 AM, Thomas Huth wrote: >> On 11.09.2017 19:19, Eric Blake wrote: >>> Commit 2f8b2767 originally added qpci_plug_device_test() and >>> qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. >>> Later, commit cf716b31 moved one half of the pair to pci.c >>> when adding PPC64 support. Keep the implementations of the >>> two functions together, and shorten the name to >>> qpci_unplug_device_test(), since all callers use the two >>> functions in tandem. >>> > >> >> No, that's a bad idea. ACPI and that outb() is clearly something >> specific to x86, so this should not reside in pci.c but in pci-pc.c >> >> We might be able to unify this - I've had a similar patch here: >> >> https://patchwork.kernel.org/patch/9905031/ >> >> ... but I think this needs some more careful thinking and discussion, so >> I'd suggest that you remove this from your already huge patch series for >> now and we fix it later instead. > > Okay, I'm fine dropping this patch, and can base my respin on top of > your cleanup instead. Note that my patches are currently on halt since I'm waiting for your qemu_startf() reworks to get included first :-) ... so feel free to pick up ideas or patches from my series into your series if that helps. Thomas
diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h index 429c382282..fdda7eca6e 100644 --- a/tests/libqos/pci.h +++ b/tests/libqos/pci.h @@ -111,5 +111,5 @@ QPCIBar qpci_legacy_iomap(QPCIDevice *dev, uint16_t addr); void qpci_plug_device_test(const char *driver, const char *id, uint8_t slot, const char *opts); -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot); +void qpci_unplug_device_test(const char *id, uint8_t slot); #endif diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c index d8085d944e..4c663a3019 100644 --- a/tests/e1000e-test.c +++ b/tests/e1000e-test.c @@ -461,7 +461,7 @@ static void test_e1000e_hotplug(gconstpointer data) qtest_start("-device e1000e"); qpci_plug_device_test("e1000e", "e1000e_net", slot, NULL); - qpci_unplug_acpi_device_test("e1000e_net", slot); + qpci_unplug_device_test("e1000e_net", slot); qtest_end(); } diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c index 37763425ee..8c9ed6a568 100644 --- a/tests/ivshmem-test.c +++ b/tests/ivshmem-test.c @@ -427,7 +427,7 @@ static void test_ivshmem_hotplug(void) qpci_plug_device_test("ivshmem", "iv1", PCI_SLOT_HP, opts); if (strcmp(arch, "ppc64") != 0) { - qpci_unplug_acpi_device_test("iv1", PCI_SLOT_HP); + qpci_unplug_device_test("iv1", PCI_SLOT_HP); } qtest_end(); diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c index e267fd1a44..6305d142a5 100644 --- a/tests/libqos/pci-pc.c +++ b/tests/libqos/pci-pc.c @@ -19,9 +19,6 @@ #include "qemu-common.h" -#define ACPI_PCIHP_ADDR 0xae00 -#define PCI_EJ_BASE 0x0008 - typedef struct QPCIBusPC { QPCIBus bus; @@ -156,23 +153,3 @@ void qpci_free_pc(QPCIBus *bus) g_free(s); } - -void qpci_unplug_acpi_device_test(const char *id, uint8_t slot) -{ - QDict *response; - char *cmd; - - cmd = g_strdup_printf("{'execute': 'device_del'," - " 'arguments': {" - " 'id': '%s'" - "}}", id); - response = qmp(cmd); - g_free(cmd); - g_assert(response); - g_assert(!qdict_haskey(response, "error")); - QDECREF(response); - - outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot); - - qmp_eventwait("DEVICE_DELETED"); -} diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c index 2dcdeade2a..9f36ec73ef 100644 --- a/tests/libqos/pci.c +++ b/tests/libqos/pci.c @@ -16,6 +16,9 @@ #include "hw/pci/pci_regs.h" #include "qemu/host-utils.h" +#define ACPI_PCIHP_ADDR 0xae00 +#define PCI_EJ_BASE 0x0008 + void qpci_device_foreach(QPCIBus *bus, int vendor_id, int device_id, void (*func)(QPCIDevice *dev, int devfn, void *data), void *data) @@ -412,3 +415,23 @@ void qpci_plug_device_test(const char *driver, const char *id, g_assert(!qdict_haskey(response, "error")); QDECREF(response); } + +void qpci_unplug_device_test(const char *id, uint8_t slot) +{ + QDict *response; + char *cmd; + + cmd = g_strdup_printf("{'execute': 'device_del'," + " 'arguments': {" + " 'id': '%s'" + "}}", id); + response = qmp(cmd); + g_free(cmd); + g_assert(response); + g_assert(!qdict_haskey(response, "error")); + QDECREF(response); + + outb(ACPI_PCIHP_ADDR + PCI_EJ_BASE, 1 << slot); + + qmp_eventwait("DEVICE_DELETED"); +} diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c index ca4ee645b7..8441ab91ed 100644 --- a/tests/virtio-blk-test.c +++ b/tests/virtio-blk-test.c @@ -672,7 +672,7 @@ static void pci_hotplug(void) /* unplug secondary disk */ if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { - qpci_unplug_acpi_device_test("drv1", PCI_SLOT_HP); + qpci_unplug_device_test("drv1", PCI_SLOT_HP); } qtest_shutdown(qs); } diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c index 635b942c36..24e0f774f0 100644 --- a/tests/virtio-net-test.c +++ b/tests/virtio-net-test.c @@ -248,7 +248,7 @@ static void hotplug(void) qpci_plug_device_test("virtio-net-pci", "net1", PCI_SLOT_HP, NULL); if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { - qpci_unplug_acpi_device_test("net1", PCI_SLOT_HP); + qpci_unplug_device_test("net1", PCI_SLOT_HP); } test_end(); diff --git a/tests/virtio-rng-test.c b/tests/virtio-rng-test.c index dcecf77463..fc4a36474b 100644 --- a/tests/virtio-rng-test.c +++ b/tests/virtio-rng-test.c @@ -25,7 +25,7 @@ static void hotplug(void) qpci_plug_device_test("virtio-rng-pci", "rng1", PCI_SLOT_HP, NULL); if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { - qpci_unplug_acpi_device_test("rng1", PCI_SLOT_HP); + qpci_unplug_device_test("rng1", PCI_SLOT_HP); } }
Commit 2f8b2767 originally added qpci_plug_device_test() and qpci_unplug_acpi_device_test() as a pair, both in pci-pc.c. Later, commit cf716b31 moved one half of the pair to pci.c when adding PPC64 support. Keep the implementations of the two functions together, and shorten the name to qpci_unplug_device_test(), since all callers use the two functions in tandem. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/libqos/pci.h | 2 +- tests/e1000e-test.c | 2 +- tests/ivshmem-test.c | 2 +- tests/libqos/pci-pc.c | 23 ----------------------- tests/libqos/pci.c | 23 +++++++++++++++++++++++ tests/virtio-blk-test.c | 2 +- tests/virtio-net-test.c | 2 +- tests/virtio-rng-test.c | 2 +- 8 files changed, 29 insertions(+), 29 deletions(-)