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 > >>> Reviewed-by: Laurent Vivier > >>> Reviewed-by: Greg Kurz > >>> --- > >>> 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. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson