Re: [Qemu-devel] [PATCH 4/8] tests: Better handle legacy IO addresses in tco-test

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

On Tue, Oct 18, 2016 at 06:28:26PM +0200, Laurent Vivier wrote:
> 
> 
> On 18/10/2016 17:14, Laurent Vivier wrote:
> > 
> > 
> > On 18/10/2016 12:52, David Gibson wrote:
> >> tco_test uses the libqos PCI code to access the device.  This makes perfect
> >> sense for the PCI config space accesses.  However for IO, rather than the
> >> usual PCI approach of mapping a PCI BAR, then accessing that, it instead
> >> uses the legacy approach of fixed, known addresses in PCI IO space.
> >>
> >> That doesn't work very well with the qpci_io_{read,write} functions because
> >> we never use qpci_iomap() and so have to make assumptions about the
> >> internal encoding of the address tokens iomap() returns.
> >>
> >> This patch avoids that, by directly using the bus's pio_{read,write}
> >> callbacks, which are defined to take addresses within the PCI IO space.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  tests/tco-test.c | 87 ++++++++++++++++++++++++++++----------------------------
> >>  1 file changed, 44 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/tests/tco-test.c b/tests/tco-test.c
> >> index 0d201b1..e668630 100644
> >> --- a/tests/tco-test.c
> >> +++ b/tests/tco-test.c
> >> @@ -40,13 +40,13 @@ enum {
> >>  typedef struct {
> >>      const char *args;
> >>      bool noreboot;
> >> +    QPCIBus *bus;
> >>      QPCIDevice *dev;
> >> -    void *tco_io_base;
> >> +    uint16_t tco_io_base;
> >>  } TestData;
> >>  
> >>  static void test_init(TestData *d)
> >>  {
> >> -    QPCIBus *bus;
> >>      QTestState *qs;
> >>      char *s;
> >>  
> >> @@ -57,8 +57,8 @@ static void test_init(TestData *d)
> >>      qtest_irq_intercept_in(qs, "ioapic");
> >>      g_free(s);
> >>  
> >> -    bus = qpci_init_pc(NULL);
> >> -    d->dev = qpci_device_find(bus, QPCI_DEVFN(0x1f, 0x00));
> >> +    d->bus = qpci_init_pc(NULL);
> > 
> > You can use qtest_pc_boot() now.
> > 
> >> +    d->dev = qpci_device_find(d->bus, QPCI_DEVFN(0x1f, 0x00));
> >>      g_assert(d->dev != NULL);
> >>  
> >>      qpci_device_enable(d->dev);
> >> @@ -70,42 +70,42 @@ static void test_init(TestData *d)
> >>      /* set Root Complex BAR */
> >>      qpci_config_writel(d->dev, ICH9_LPC_RCBA, RCBA_BASE_ADDR | 0x1);
> >>  
> >> -    d->tco_io_base = (void *)((uintptr_t)PM_IO_BASE_ADDR + 0x60);
> >> +    d->tco_io_base = PM_IO_BASE_ADDR + 0x60;
> > 
> > Why don't you use QPCIBar in TestData to store the address?
> > And you can call qpci_io_XXX() with it.
> 
> OK, I was watching the state of qpci_io_XXX() after the series, so you
> can't do that here, but perhaps this patch should be moved to PATCH 8/8?

No, this is quite deliberately before 8/8.  The point of 8/8 is that
QPCIBar is supposed to be opaque to the tests/drivers.  Using the
legacy addresses requires that the test/driver work directly with the
actual IO address.  Mixing the two - having the test case construct
the base pointer makes the conversion to an opaque token more awkward.

However.. another approach just occurred to me.  I could define a
special QPCIBar constant - exported as a global by libqos, or returned
by a function - which references the legacy PIO space instead of a
particular bar.  The qpci_io_*() functions could be used with that.

I'll look into that, and see how it works.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]