Re: [Qemu-ppc] [PATCHv5 07/12] libqos: Implement mmio accessors in terms of mem{read, write}

[David Gibson]

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 <address@hidden>
> >>> Reviewed-by: Laurent Vivier <address@hidden>
> >>> Reviewed-by: Greg Kurz <address@hidden>
> >>> ---
> >>>  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

signature.asc
Description: PGP signature