Re: [Qemu-ppc] [PATCH v3 1/6] libqos: add PPC64 PCI support

[Cédric Le Goater]

On 10/04/2016 02:22 AM, David Gibson wrote:
> On Mon, Oct 03, 2016 at 01:23:27PM +0200, Cédric Le Goater wrote:
>> On 09/29/2016 07:27 AM, David Gibson wrote:
>>> On Wed, Sep 28, 2016 at 08:51:28PM +0200, Laurent Vivier wrote:
>>>> Signed-off-by: Laurent Vivier <address@hidden>
>>>> ---
>>>>  tests/Makefile.include   |   1 +
>>>>  tests/libqos/pci-pc.c    |  22 ----
>>>>  tests/libqos/pci-spapr.c | 280 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  tests/libqos/pci-spapr.h |  17 +++
>>>>  tests/libqos/pci.c       |  22 +++-
>>>>  tests/libqos/rtas.c      |  45 ++++++++
>>>>  tests/libqos/rtas.h      |   4 +
>>>>  7 files changed, 368 insertions(+), 23 deletions(-)
>>>>  create mode 100644 tests/libqos/pci-spapr.c
>>>>  create mode 100644 tests/libqos/pci-spapr.h
>>>>
>>>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>>>> index 8162f6f..92c82d8 100644
>>>> --- a/tests/Makefile.include
>>>> +++ b/tests/Makefile.include
>>>> @@ -590,6 +590,7 @@ libqos-obj-y += tests/libqos/i2c.o 
>>>> tests/libqos/libqos.o
>>>>  libqos-spapr-obj-y = $(libqos-obj-y) tests/libqos/malloc-spapr.o
>>>>  libqos-spapr-obj-y += tests/libqos/libqos-spapr.o
>>>>  libqos-spapr-obj-y += tests/libqos/rtas.o
>>>> +libqos-spapr-obj-y += tests/libqos/pci-spapr.o
>>>>  libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
>>>>  libqos-pc-obj-y += tests/libqos/malloc-pc.o tests/libqos/libqos-pc.o
>>>>  libqos-pc-obj-y += tests/libqos/ahci.o
>>>> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
>>>> index 1ae2d37..82066b8 100644
>>>> --- a/tests/libqos/pci-pc.c
>>>> +++ b/tests/libqos/pci-pc.c
>>>> @@ -255,28 +255,6 @@ void qpci_free_pc(QPCIBus *bus)
>>>>      g_free(s);
>>>>  }
>>>>  
>>>> -void qpci_plug_device_test(const char *driver, const char *id,
>>>> -                           uint8_t slot, const char *opts)
>>>> -{
>>>> -    QDict *response;
>>>> -    char *cmd;
>>>> -
>>>> -    cmd = g_strdup_printf("{'execute': 'device_add',"
>>>> -                          " 'arguments': {"
>>>> -                          "   'driver': '%s',"
>>>> -                          "   'addr': '%d',"
>>>> -                          "   %s%s"
>>>> -                          "   'id': '%s'"
>>>> -                          "}}", driver, slot,
>>>> -                          opts ? opts : "", opts ? "," : "",
>>>> -                          id);
>>>> -    response = qmp(cmd);
>>>> -    g_free(cmd);
>>>> -    g_assert(response);
>>>> -    g_assert(!qdict_haskey(response, "error"));
>>>> -    QDECREF(response);
>>>> -}
>>>> -
>>>>  void qpci_unplug_acpi_device_test(const char *id, uint8_t slot)
>>>>  {
>>>>      QDict *response;
>>>> diff --git a/tests/libqos/pci-spapr.c b/tests/libqos/pci-spapr.c
>>>> new file mode 100644
>>>> index 0000000..78df823
>>>> --- /dev/null
>>>> +++ b/tests/libqos/pci-spapr.c
>>>> @@ -0,0 +1,280 @@
>>>> +/*
>>>> + * libqos PCI bindings for SPAPR
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>>> later.
>>>> + * See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "qemu/osdep.h"
>>>> +#include "libqtest.h"
>>>> +#include "libqos/pci-spapr.h"
>>>> +#include "libqos/rtas.h"
>>>> +
>>>> +#include "hw/pci/pci_regs.h"
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "qemu/host-utils.h"
>>>> +
>>>> +
>>>> +/* From include/hw/pci-host/spapr.h */
>>>> +
>>>> +#define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
>>>> +
>>>> +#define SPAPR_PCI_MEM_WIN_BUS_OFFSET 0x80000000ULL
>>>> +
>>>> +#define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
>>>> +#define SPAPR_PCI_WINDOW_SPACING     0x1000000000ULL
>>>> +#define SPAPR_PCI_MMIO_WIN_OFF       0xA0000000
>>>> +#define SPAPR_PCI_MMIO_WIN_SIZE      (SPAPR_PCI_WINDOW_SPACING - \
>>>> +                                     SPAPR_PCI_MEM_WIN_BUS_OFFSET)
>>>> +#define SPAPR_PCI_IO_WIN_OFF         0x80000000
>>>> +#define SPAPR_PCI_IO_WIN_SIZE        0x10000
>>>> +
>>>> +/* index is the phb index */
>>>> +
>>>> +#define BUIDBASE(index)              (SPAPR_PCI_BASE_BUID + (index))
>>>> +#define PCIBASE(index)               (SPAPR_PCI_WINDOW_BASE + \
>>>> +                                      (index) * SPAPR_PCI_WINDOW_SPACING)
>>>> +#define IOBASE(index)                (PCIBASE(index) + 
>>>> SPAPR_PCI_IO_WIN_OFF)
>>>> +#define MMIOBASE(index)              (PCIBASE(index) + 
>>>> SPAPR_PCI_MMIO_WIN_OFF)
>>>> +
>>>> +typedef struct QPCIBusSPAPR {
>>>> +    QPCIBus bus;
>>>> +    QGuestAllocator *alloc;
>>>> +
>>>> +    uint64_t pci_hole_start;
>>>> +    uint64_t pci_hole_size;
>>>> +    uint64_t pci_hole_alloc;
>>>> +
>>>> +    uint32_t pci_iohole_start;
>>>> +    uint32_t pci_iohole_size;
>>>> +    uint32_t pci_iohole_alloc;
>>>> +} QPCIBusSPAPR;
>>>> +
>>>> +static uint8_t qpci_spapr_io_readb(QPCIBus *bus, void *addr)
>>>> +{
>>>> +    uint64_t port = (uint64_t)addr;
>>>> +    uint8_t v;
>>>> +    if (port < SPAPR_PCI_IO_WIN_SIZE) {
>>>> +        v = readb(IOBASE(0) + port);
>>>> +    } else {
>>>> +        v = readb(MMIOBASE(0) + port);
>>>> +    }
>>>> +    return v;
>>>> +}
>>>> +
>>>> +static uint16_t qpci_spapr_io_readw(QPCIBus *bus, void *addr)
>>>> +{
>>>> +    uint64_t port = (uint64_t)addr;
>>>> +    uint16_t v;
>>>> +    if (port < SPAPR_PCI_IO_WIN_SIZE) {
>>>> +        v = readw(IOBASE(0) + port);
>>>> +    } else {
>>>> +        v = readw(MMIOBASE(0) + port);
>>>> +    }
>>>
>>> Ok, so, I've looked through the qtest stuff in more detail now, and
>>> I've got a better idea of how the endianness works.  Some of my
>>> earlier comments were confused about which pieces were in the test
>>> case side and which were on the qtest accel stub side.
>>>
>>> So, what I think you need is an unconditional byteswap in each of the
>>> spapr pci IO functions.  Why?
>>>
>>>    * The plain readw() etc. functions return values read from memory
>>>      assuming guest endianness.  For guest native values in memory, so
>>>      far so good.
>>>    * Generic users of the pci read/write functions in qtest expect to
>>>      get native values.
>>>    * But PCI devices are always LE, not guest endian
>>>
>>> The guest endianness of spapr (as far as tswap() etc. are concerned)
>>> is BE, even though that's not really true in practice these days, so
>>> to correct for the PCI registers being read as BE when they should be
>>> LE we always need a swap.
>>>
>>> That should remove the need for swaps further up the test stack.
>>>
>>> Of course, "guest endianness" is a poorly defined concept, so longer
>>> term it might be better to completely replace the "readw" etc. qtest
>>> operations with explicit "readw_le" and "readw_be" ops.
>>
>>
>> I have a similar need for a unit test of the AST2400 flash controller. 
>>
>> The flash module is accessible through a memory window and, when in 
>> SPI mode, the commands are sent to the slave by writing at the beginning 
>> of the region. Addresses are necessarily BE to be understood by the SPI
>> flash module. So, sequences like
>>
>>     writeb(AST2400_FLASH_BASE, EN_4BYTE_ADDR);
>>     writeb(AST2400_FLASH_BASE, READ);
>>     writel(AST2400_FLASH_BASE, cpu_to_be32(addr));
>>
>> will just fail under a BE host as an extra swap is done by the qtest
>> layer. I have used memwrite to have endian-agnostic mem accessors. 
> 
> Right.  You could fix this by writing tswap32(cpu_to_be32(addr)), of
> course but it's starting to get ugly.
> 
>> Maybe something like below would be helpful in libqtest.h
> 
> 
> 
>>
>>
>> +/*
>> + * Generic read/write helpers for a BE region
>> + */
>> +static inline void writew_be(uint64_t addr, uint16_t value)
>> +{
>> +    value = cpu_to_be16(value);
>> +    memwrite(addr, &value, sizeof(value));
>> +}
>> +
>> +static inline uint16_t readw_be(uint64_t addr)
>> +{
>> +    uint16_t value;
>> +
>> +    memread(addr, &value, sizeof(value));
>> +    return be16_to_cpu(value);
>> +}
>> +
>> +static inline void writel_be(uint64_t addr, uint32_t value)
>> +{
>> +    value = cpu_to_be32(value);
>> +    memwrite(addr, &value, sizeof(value));
>> +}
>> +
>> +static inline uint32_t readl_be(uint64_t addr)
>> +{
>> +    uint32_t value;
>> +
>> +    memread(addr, &value, sizeof(value));
>> +    return be32_to_cpu(value);
>> +}
>>
>> If this is correct, I can add the LE equivalents and send a patch.
> 
> I like the idea, but we probably need buy in from some more people.

Yes. I should be sending the latest patch as an RFC but I need to test 
the  LE read/write first. 

Thanks,

C.