| Message ID | 1477285201-10244-8-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 24/10/16 15:59, David Gibson wrote: > In the libqos PCI code we now have accessors both for registers (byte > significance preserving) and for streaming data (byte address order > preserving). These exist in both the interface for qtest drivers and in > the machine specific backends. > > However, the register-style accessors aren't actually necessary in the > backend. They can be implemented in terms of the byte address order > preserving accessors by the libqos wrappers. This works because PCI is > always little endian. > > This does assume that the back end byte address order preserving accessors > will perform the equivalent of a single bus transaction for short lengths. > This is the case, and in fact they currently end up using the same > cpu_physical_memory_rw() implementation within the qtest accelerator. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > Reviewed-by: Laurent Vivier <lvivier@redhat.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > --- > tests/libqos/pci-pc.c | 38 -------------------------------------- > tests/libqos/pci-spapr.c | 44 -------------------------------------------- > tests/libqos/pci.c | 20 ++++++++++++++------ > tests/libqos/pci.h | 8 -------- > 4 files changed, 14 insertions(+), 96 deletions(-) > [...] > diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h > index 2b08362..ce6ed08 100644 > --- a/tests/libqos/pci.h > +++ b/tests/libqos/pci.h > @@ -27,18 +27,10 @@ struct QPCIBus { > uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); > uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); > > - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); > - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); > - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); > - > void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > > - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > - > void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len); > void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t len); > > You added them in "libqos: Handle PCI IO de-multiplexing in common code" (few patched before) and removing them now - if you moved this patch earlier, it would reduce the series, or what do I miss?
On Tue, Oct 25, 2016 at 05:47:43PM +1100, Alexey Kardashevskiy wrote: > On 24/10/16 15:59, David Gibson wrote: > > In the libqos PCI code we now have accessors both for registers (byte > > significance preserving) and for streaming data (byte address order > > preserving). These exist in both the interface for qtest drivers and in > > the machine specific backends. > > > > However, the register-style accessors aren't actually necessary in the > > backend. They can be implemented in terms of the byte address order > > preserving accessors by the libqos wrappers. This works because PCI is > > always little endian. > > > > This does assume that the back end byte address order preserving accessors > > will perform the equivalent of a single bus transaction for short lengths. > > This is the case, and in fact they currently end up using the same > > cpu_physical_memory_rw() implementation within the qtest accelerator. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com> > > Reviewed-by: Greg Kurz <groug@kaod.org> > > --- > > tests/libqos/pci-pc.c | 38 -------------------------------------- > > tests/libqos/pci-spapr.c | 44 -------------------------------------------- > > tests/libqos/pci.c | 20 ++++++++++++++------ > > tests/libqos/pci.h | 8 -------- > > 4 files changed, 14 insertions(+), 96 deletions(-) > > > > [...] > > > diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h > > index 2b08362..ce6ed08 100644 > > --- a/tests/libqos/pci.h > > +++ b/tests/libqos/pci.h > > @@ -27,18 +27,10 @@ struct QPCIBus { > > uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); > > uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); > > > > - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); > > - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); > > - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); > > - > > void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > > void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > > void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > > > > - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > > - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > > - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > > - > > void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len); > > void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t len); > > > > > > You added them in "libqos: Handle PCI IO de-multiplexing in common code" > (few patched before) and removing them now - if you moved this patch > earlier, it would reduce the series, or what do I miss? Well, it can't go before the PIO / MMIO split, because on x86 the PIO part is implemented with inw/outw instead of readw/writew, and those don't have a memread/memwrite equivalent. The change could go at the same time, but my feeling was that logical separation of the steps was worth a bit of temporary extra code.
On 25/10/16 23:16, David Gibson wrote: > On Tue, Oct 25, 2016 at 05:47:43PM +1100, Alexey Kardashevskiy wrote: >> On 24/10/16 15:59, David Gibson wrote: >>> In the libqos PCI code we now have accessors both for registers (byte >>> significance preserving) and for streaming data (byte address order >>> preserving). These exist in both the interface for qtest drivers and in >>> the machine specific backends. >>> >>> However, the register-style accessors aren't actually necessary in the >>> backend. They can be implemented in terms of the byte address order >>> preserving accessors by the libqos wrappers. This works because PCI is >>> always little endian. >>> >>> This does assume that the back end byte address order preserving accessors >>> will perform the equivalent of a single bus transaction for short lengths. >>> This is the case, and in fact they currently end up using the same >>> cpu_physical_memory_rw() implementation within the qtest accelerator. >>> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>> Reviewed-by: Laurent Vivier <lvivier@redhat.com> >>> Reviewed-by: Greg Kurz <groug@kaod.org> >>> --- >>> tests/libqos/pci-pc.c | 38 -------------------------------------- >>> tests/libqos/pci-spapr.c | 44 -------------------------------------------- >>> tests/libqos/pci.c | 20 ++++++++++++++------ >>> tests/libqos/pci.h | 8 -------- >>> 4 files changed, 14 insertions(+), 96 deletions(-) >>> >> >> [...] >> >>> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h >>> index 2b08362..ce6ed08 100644 >>> --- a/tests/libqos/pci.h >>> +++ b/tests/libqos/pci.h >>> @@ -27,18 +27,10 @@ struct QPCIBus { >>> uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); >>> uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); >>> >>> - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); >>> - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); >>> - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); >>> - >>> void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); >>> void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); >>> void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); >>> >>> - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); >>> - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); >>> - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); >>> - >>> void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len); >>> void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t len); >>> >>> >> >> You added them in "libqos: Handle PCI IO de-multiplexing in common code" >> (few patched before) and removing them now - if you moved this patch >> earlier, it would reduce the series, or what do I miss? > > Well, it can't go before the PIO / MMIO split, because on x86 the PIO > part is implemented with inw/outw instead of readw/writew, and those > don't have a memread/memwrite equivalent. > > The change could go at the same time, but my feeling was that logical > separation of the steps was worth a bit of temporary extra code. It is a bit hard to follow the logic of the patchset when you do not know if the new code is going to stay or not - I automatically assumed it is staying and when I saw it is being removed - I wondered if you are removing what you just added, and this - in my opinion - kills the idea of making smaller patches to make review easier, better just squash them all... But since Greg is happy and things seems not working worse (make check fails on my setup but whatever), you can ignore me :)
On Wed, Oct 26, 2016 at 12:18:03PM +1100, Alexey Kardashevskiy wrote: > On 25/10/16 23:16, David Gibson wrote: > > On Tue, Oct 25, 2016 at 05:47:43PM +1100, Alexey Kardashevskiy wrote: > >> On 24/10/16 15:59, David Gibson wrote: > >>> In the libqos PCI code we now have accessors both for registers (byte > >>> significance preserving) and for streaming data (byte address order > >>> preserving). These exist in both the interface for qtest drivers and in > >>> the machine specific backends. > >>> > >>> However, the register-style accessors aren't actually necessary in the > >>> backend. They can be implemented in terms of the byte address order > >>> preserving accessors by the libqos wrappers. This works because PCI is > >>> always little endian. > >>> > >>> This does assume that the back end byte address order preserving accessors > >>> will perform the equivalent of a single bus transaction for short lengths. > >>> This is the case, and in fact they currently end up using the same > >>> cpu_physical_memory_rw() implementation within the qtest accelerator. > >>> > >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > >>> Reviewed-by: Laurent Vivier <lvivier@redhat.com> > >>> Reviewed-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> tests/libqos/pci-pc.c | 38 -------------------------------------- > >>> tests/libqos/pci-spapr.c | 44 -------------------------------------------- > >>> tests/libqos/pci.c | 20 ++++++++++++++------ > >>> tests/libqos/pci.h | 8 -------- > >>> 4 files changed, 14 insertions(+), 96 deletions(-) > >>> > >> > >> [...] > >> > >>> diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h > >>> index 2b08362..ce6ed08 100644 > >>> --- a/tests/libqos/pci.h > >>> +++ b/tests/libqos/pci.h > >>> @@ -27,18 +27,10 @@ struct QPCIBus { > >>> uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); > >>> uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); > >>> > >>> - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); > >>> - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); > >>> - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); > >>> - > >>> void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > >>> void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > >>> void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > >>> > >>> - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); > >>> - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); > >>> - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); > >>> - > >>> void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len); > >>> void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t len); > >>> > >>> > >> > >> You added them in "libqos: Handle PCI IO de-multiplexing in common code" > >> (few patched before) and removing them now - if you moved this patch > >> earlier, it would reduce the series, or what do I miss? > > > > Well, it can't go before the PIO / MMIO split, because on x86 the PIO > > part is implemented with inw/outw instead of readw/writew, and those > > don't have a memread/memwrite equivalent. > > > > The change could go at the same time, but my feeling was that logical > > separation of the steps was worth a bit of temporary extra code. > > It is a bit hard to follow the logic of the patchset when you do not know > if the new code is going to stay or not - I automatically assumed it is > staying and when I saw it is being removed - I wondered if you are removing > what you just added, and this - in my opinion - kills the idea of making > smaller patches to make review easier, better just squash them all... But > since Greg is happy and things seems not working worse (make check fails on > my setup but whatever), you can ignore me :) Well, I guess it's a trade-off between conceptual simplicity and minimal code changes. Putting those callbacks in temporarily means more code change, but I think it's worth it to make each patch conceptually simpler.
diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c index 84aee25..849ea56 100644 --- a/tests/libqos/pci-pc.c +++ b/tests/libqos/pci-pc.c @@ -32,61 +32,31 @@ static uint8_t qpci_pc_pio_readb(QPCIBus *bus, uint32_t addr) return inb(addr); } -static uint8_t qpci_pc_mmio_readb(QPCIBus *bus, uint32_t addr) -{ - return readb(addr); -} - static void qpci_pc_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val) { outb(addr, val); } -static void qpci_pc_mmio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val) -{ - writeb(addr, val); -} - static uint16_t qpci_pc_pio_readw(QPCIBus *bus, uint32_t addr) { return inw(addr); } -static uint16_t qpci_pc_mmio_readw(QPCIBus *bus, uint32_t addr) -{ - return readw(addr); -} - static void qpci_pc_pio_writew(QPCIBus *bus, uint32_t addr, uint16_t val) { outw(addr, val); } -static void qpci_pc_mmio_writew(QPCIBus *bus, uint32_t addr, uint16_t val) -{ - writew(addr, val); -} - static uint32_t qpci_pc_pio_readl(QPCIBus *bus, uint32_t addr) { return inl(addr); } -static uint32_t qpci_pc_mmio_readl(QPCIBus *bus, uint32_t addr) -{ - return readl(addr); -} - static void qpci_pc_pio_writel(QPCIBus *bus, uint32_t addr, uint32_t val) { outl(addr, val); } -static void qpci_pc_mmio_writel(QPCIBus *bus, uint32_t addr, uint32_t val) -{ - writel(addr, val); -} - static void qpci_pc_memread(QPCIBus *bus, uint32_t addr, void *buf, size_t len) { memread(addr, buf, len); @@ -148,14 +118,6 @@ QPCIBus *qpci_init_pc(QGuestAllocator *alloc) ret->bus.pio_writew = qpci_pc_pio_writew; ret->bus.pio_writel = qpci_pc_pio_writel; - ret->bus.mmio_readb = qpci_pc_mmio_readb; - ret->bus.mmio_readw = qpci_pc_mmio_readw; - ret->bus.mmio_readl = qpci_pc_mmio_readl; - - ret->bus.mmio_writeb = qpci_pc_mmio_writeb; - ret->bus.mmio_writew = qpci_pc_mmio_writew; - ret->bus.mmio_writel = qpci_pc_mmio_writel; - ret->bus.memread = qpci_pc_memread; ret->bus.memwrite = qpci_pc_memwrite; diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c index ad12c2e..f26488a 100644 --- a/tests/libqos/pci-spapr.c +++ b/tests/libqos/pci-spapr.c @@ -48,72 +48,36 @@ static uint8_t qpci_spapr_pio_readb(QPCIBus *bus, uint32_t addr) return readb(s->pio_cpu_base + addr); } -static uint8_t qpci_spapr_mmio32_readb(QPCIBus *bus, uint32_t addr) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - return readb(s->mmio32_cpu_base + addr); -} - static void qpci_spapr_pio_writeb(QPCIBus *bus, uint32_t addr, uint8_t val) { QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); writeb(s->pio_cpu_base + addr, val); } -static void qpci_spapr_mmio32_writeb(QPCIBus *bus, uint32_t addr, uint8_t val) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - writeb(s->mmio32_cpu_base + addr, val); -} - static uint16_t qpci_spapr_pio_readw(QPCIBus *bus, uint32_t addr) { QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); return bswap16(readw(s->pio_cpu_base + addr)); } -static uint16_t qpci_spapr_mmio32_readw(QPCIBus *bus, uint32_t addr) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - return bswap16(readw(s->mmio32_cpu_base + addr)); -} - static void qpci_spapr_pio_writew(QPCIBus *bus, uint32_t addr, uint16_t val) { QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); writew(s->pio_cpu_base + addr, bswap16(val)); } -static void qpci_spapr_mmio32_writew(QPCIBus *bus, uint32_t addr, uint16_t val) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - writew(s->mmio32_cpu_base + addr, bswap16(val)); -} - static uint32_t qpci_spapr_pio_readl(QPCIBus *bus, uint32_t addr) { QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); return bswap32(readl(s->pio_cpu_base + addr)); } -static uint32_t qpci_spapr_mmio32_readl(QPCIBus *bus, uint32_t addr) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - return bswap32(readl(s->mmio32_cpu_base + addr)); -} - static void qpci_spapr_pio_writel(QPCIBus *bus, uint32_t addr, uint32_t val) { QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); writel(s->pio_cpu_base + addr, bswap32(val)); } -static void qpci_spapr_mmio32_writel(QPCIBus *bus, uint32_t addr, uint32_t val) -{ - QPCIBusSPAPR *s = container_of(bus, QPCIBusSPAPR, bus); - writel(s->mmio32_cpu_base + addr, bswap32(val)); -} - static void qpci_spapr_memread(QPCIBus *bus, uint32_t addr, void *buf, size_t len) { @@ -194,14 +158,6 @@ QPCIBus *qpci_init_spapr(QGuestAllocator *alloc) ret->bus.pio_writew = qpci_spapr_pio_writew; ret->bus.pio_writel = qpci_spapr_pio_writel; - ret->bus.mmio_readb = qpci_spapr_mmio32_readb; - ret->bus.mmio_readw = qpci_spapr_mmio32_readw; - ret->bus.mmio_readl = qpci_spapr_mmio32_readl; - - ret->bus.mmio_writeb = qpci_spapr_mmio32_writeb; - ret->bus.mmio_writew = qpci_spapr_mmio32_writew; - ret->bus.mmio_writel = qpci_spapr_mmio32_writel; - ret->bus.memread = qpci_spapr_memread; ret->bus.memwrite = qpci_spapr_memwrite; diff --git a/tests/libqos/pci.c b/tests/libqos/pci.c index 146b063..bdffeb3 100644 --- a/tests/libqos/pci.c +++ b/tests/libqos/pci.c @@ -230,7 +230,9 @@ uint8_t qpci_io_readb(QPCIDevice *dev, void *data) if (addr < QPCI_PIO_LIMIT) { return dev->bus->pio_readb(dev->bus, addr); } else { - return dev->bus->mmio_readb(dev->bus, addr); + uint8_t val; + dev->bus->memread(dev->bus, addr, &val, sizeof(val)); + return val; } } @@ -241,7 +243,9 @@ uint16_t qpci_io_readw(QPCIDevice *dev, void *data) if (addr < QPCI_PIO_LIMIT) { return dev->bus->pio_readw(dev->bus, addr); } else { - return dev->bus->mmio_readw(dev->bus, addr); + uint16_t val; + dev->bus->memread(dev->bus, addr, &val, sizeof(val)); + return le16_to_cpu(val); } } @@ -252,7 +256,9 @@ uint32_t qpci_io_readl(QPCIDevice *dev, void *data) if (addr < QPCI_PIO_LIMIT) { return dev->bus->pio_readl(dev->bus, addr); } else { - return dev->bus->mmio_readl(dev->bus, addr); + uint32_t val; + dev->bus->memread(dev->bus, addr, &val, sizeof(val)); + return le32_to_cpu(val); } } @@ -263,7 +269,7 @@ void qpci_io_writeb(QPCIDevice *dev, void *data, uint8_t value) if (addr < QPCI_PIO_LIMIT) { dev->bus->pio_writeb(dev->bus, addr, value); } else { - dev->bus->mmio_writeb(dev->bus, addr, value); + dev->bus->memwrite(dev->bus, addr, &value, sizeof(value)); } } @@ -274,7 +280,8 @@ void qpci_io_writew(QPCIDevice *dev, void *data, uint16_t value) if (addr < QPCI_PIO_LIMIT) { dev->bus->pio_writew(dev->bus, addr, value); } else { - dev->bus->mmio_writew(dev->bus, addr, value); + value = cpu_to_le16(value); + dev->bus->memwrite(dev->bus, addr, &value, sizeof(value)); } } @@ -285,7 +292,8 @@ void qpci_io_writel(QPCIDevice *dev, void *data, uint32_t value) if (addr < QPCI_PIO_LIMIT) { dev->bus->pio_writel(dev->bus, addr, value); } else { - dev->bus->mmio_writel(dev->bus, addr, value); + value = cpu_to_le32(value); + dev->bus->memwrite(dev->bus, addr, &value, sizeof(value)); } } diff --git a/tests/libqos/pci.h b/tests/libqos/pci.h index 2b08362..ce6ed08 100644 --- a/tests/libqos/pci.h +++ b/tests/libqos/pci.h @@ -27,18 +27,10 @@ struct QPCIBus { uint16_t (*pio_readw)(QPCIBus *bus, uint32_t addr); uint32_t (*pio_readl)(QPCIBus *bus, uint32_t addr); - uint8_t (*mmio_readb)(QPCIBus *bus, uint32_t addr); - uint16_t (*mmio_readw)(QPCIBus *bus, uint32_t addr); - uint32_t (*mmio_readl)(QPCIBus *bus, uint32_t addr); - void (*pio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); void (*pio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); void (*pio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); - void (*mmio_writeb)(QPCIBus *bus, uint32_t addr, uint8_t value); - void (*mmio_writew)(QPCIBus *bus, uint32_t addr, uint16_t value); - void (*mmio_writel)(QPCIBus *bus, uint32_t addr, uint32_t value); - void (*memread)(QPCIBus *bus, uint32_t addr, void *buf, size_t len); void (*memwrite)(QPCIBus *bus, uint32_t addr, const void *buf, size_t len);