Re: [Qemu-devel] [PATCH 8/8] libqos: Change PCI accessors to take opaque BAR handle

[David Gibson]

On Tue, Oct 18, 2016 at 06:48:16PM +0200, Laurent Vivier wrote:
> 
> 
> On 18/10/2016 12:52, David Gibson wrote:
> > The usual use model for the libqos PCI functions is to map a specific PCI
> > BAR using qpci_iomap() then pass the returned token into IO accessor
> > functions.  This, and the fact that iomap() returns a (void *) which
> > actually contains a PCI space address, kind of suggests that the return
> > value from iomap is supposed to be an opaque token.
> > 
> > ..except that the callers expect to be able to add offsets to it.  Which
> > also assumes the compiler will support pointer arithmetic on a (void *),
> > and treat it as working with byte offsets.
> > 
> > To clarify this situation change iomap() and the IO accessors to take
> > a definitely opaque BAR handle (enforced with a wrapper struct) along with
> > an offset within the BAR.  This changes both the functions and all the
> > callers.
> > 
> > A few notes:
> > * Asserts that iomap() returns non-NULL are removed in some places;
> > iomap() already asserts if it can't map the BAR
> > * In ide-test.c we change explicit outb() etc. calls to matching
> > qpci_io_writeb() calls.  That makes the test more portable, and removes
> > assumptions that the test case shouldn't be making about how iomap()'s
> > return value is formatted internally.
> > 
> > Signed-off-by: David Gibson 
> > 
> > # Conflicts:
> > #   tests/libqos/virtio-pci.c
> 
> A sequel of a cherry-pick?
> 
> > ---
> >  tests/ahci-test.c |   4 +-
> >  tests/e1000e-test.c   |   7 ++-
> >  tests/ide-test.c  |  23 +
> >  tests/ivshmem-test.c  |  28 +--
> >  tests/libqos/ahci.c   |   3 +-
> >  tests/libqos/ahci.h   |   6 +--
> >  tests/libqos/pci.c| 125 
> > +-
> >  tests/libqos/pci.h|  39 +--
> >  tests/libqos/usb.c|   6 +--
> >  tests/libqos/usb.h|   2 +-
> >  tests/libqos/virtio-pci.c | 113 +
> >  tests/libqos/virtio-pci.h |   2 +-
> >  tests/rtl8139-test.c  |  10 ++--
> >  tests/usb-hcd-ehci-test.c |   5 +-
> 
> Perhaps you can move tco-test update in this series?
> 
> >  14 files changed, 186 insertions(+), 187 deletions(-)
> > 
> > diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> > index 9c0adce..4358631 100644
> > --- a/tests/ahci-test.c
> > +++ b/tests/ahci-test.c
> > @@ -90,12 +90,12 @@ static void verify_state(AHCIQState *ahci)
> >  g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
> >  
> >  /* If we haven't initialized, this is as much as can be validated. */
> > -if (!ahci->hba_base) {
> > +if (!ahci->hba_bar.addr) {
> >  return;
> >  }
> >  
> >  hba_base = (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
> > -hba_stored = (uint64_t)(uintptr_t)ahci->hba_base;
> > +hba_stored = ahci->hba_bar.addr;
> >  g_assert_cmphex(hba_base, ==, hba_stored);
> >  
> >  g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
> > diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> > index 3979b20..8c42ca9 100644
> > --- a/tests/e1000e-test.c
> > +++ b/tests/e1000e-test.c
> > @@ -87,7 +87,7 @@
> >  
> >  typedef struct e1000e_device {
> >  QPCIDevice *pci_dev;
> > -void *mac_regs;
> > +QPCIBar mac_regs;
> >  
> >  uint64_t tx_ring;
> >  uint64_t rx_ring;
> > @@ -119,12 +119,12 @@ static QPCIDevice *e1000e_device_find(QPCIBus *bus)
> >  
> >  static void e1000e_macreg_write(e1000e_device *d, uint32_t reg, uint32_t 
> > val)
> >  {
> > -qpci_io_writel(d->pci_dev, d->mac_regs + reg, val);
> > +qpci_io_writel(d->pci_dev, d->mac_regs, reg, val);
> >  }
> >  
> >  static uint32_t e1000e_macreg_read(e1000e_device *d, uint32_t reg)
> >  {
> > -return qpci_io_readl(d->pci_dev, d->mac_regs + reg);
> > +return qpci_io_readl(d->pci_dev, d->mac_regs, reg);
> >  }
> >  
> >  static void e1000e_device_init(QPCIBus *bus, e1000e_device *d)
> > @@ -138,7 +138,6 @@ static void e1000e_device_init(QPCIBus *bus, 
> > e1000e_device *d)
> >  
> >  /* Map BAR0 (mac registers) */
> >  d->mac_regs = qpci_iomap(d->pci_dev, 0, NULL);
> > -g_assert_nonnull(d->mac_regs);
> >  
> >  /* Reset the device */
> >  val = e1000e_macreg_read(d, E1000E_CTRL);
> > diff --git a/tests/ide-test.c b/tests/ide-test.c
> > index a8a4081..dc08536 100644
> > --- a/tests/ide-test.c
> > +++ b/tests/ide-test.c
> > @@ -137,7 +137,7 @@ static void ide_test_quit(void)
> >  qtest_end();
> >  }
> >  
> > -static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
> > +static QPCIDevice *get_pci_device(QPCIBar *bmdma_bar)
> >  {
> >  QPCIDevice *dev;
> >  uint16_t vendor_id, device_id;
> > @@ -156,7 +156,7 @@ static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
> >  g_assert(device_id == PCI_DEVICE_ID_INTEL_82371SB_1);
> >  
> >  /* Map bmdma BAR */
> > -*bmdma_base = 

Re: [Qemu-devel] [PATCH 8/8] libqos: Change PCI accessors to take opaque BAR handle

[Laurent Vivier]

On 18/10/2016 12:52, David Gibson wrote:
> The usual use model for the libqos PCI functions is to map a specific PCI
> BAR using qpci_iomap() then pass the returned token into IO accessor
> functions.  This, and the fact that iomap() returns a (void *) which
> actually contains a PCI space address, kind of suggests that the return
> value from iomap is supposed to be an opaque token.
> 
> ..except that the callers expect to be able to add offsets to it.  Which
> also assumes the compiler will support pointer arithmetic on a (void *),
> and treat it as working with byte offsets.
> 
> To clarify this situation change iomap() and the IO accessors to take
> a definitely opaque BAR handle (enforced with a wrapper struct) along with
> an offset within the BAR.  This changes both the functions and all the
> callers.
> 
> A few notes:
> * Asserts that iomap() returns non-NULL are removed in some places;
> iomap() already asserts if it can't map the BAR
> * In ide-test.c we change explicit outb() etc. calls to matching
> qpci_io_writeb() calls.  That makes the test more portable, and removes
> assumptions that the test case shouldn't be making about how iomap()'s
> return value is formatted internally.
> 
> Signed-off-by: David Gibson 
> 
> # Conflicts:
> # tests/libqos/virtio-pci.c

A sequel of a cherry-pick?

> ---
>  tests/ahci-test.c |   4 +-
>  tests/e1000e-test.c   |   7 ++-
>  tests/ide-test.c  |  23 +
>  tests/ivshmem-test.c  |  28 +--
>  tests/libqos/ahci.c   |   3 +-
>  tests/libqos/ahci.h   |   6 +--
>  tests/libqos/pci.c| 125 
> +-
>  tests/libqos/pci.h|  39 +--
>  tests/libqos/usb.c|   6 +--
>  tests/libqos/usb.h|   2 +-
>  tests/libqos/virtio-pci.c | 113 +
>  tests/libqos/virtio-pci.h |   2 +-
>  tests/rtl8139-test.c  |  10 ++--
>  tests/usb-hcd-ehci-test.c |   5 +-

Perhaps you can move tco-test update in this series?

>  14 files changed, 186 insertions(+), 187 deletions(-)
> 
> diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> index 9c0adce..4358631 100644
> --- a/tests/ahci-test.c
> +++ b/tests/ahci-test.c
> @@ -90,12 +90,12 @@ static void verify_state(AHCIQState *ahci)
>  g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
>  
>  /* If we haven't initialized, this is as much as can be validated. */
> -if (!ahci->hba_base) {
> +if (!ahci->hba_bar.addr) {
>  return;
>  }
>  
>  hba_base = (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
> -hba_stored = (uint64_t)(uintptr_t)ahci->hba_base;
> +hba_stored = ahci->hba_bar.addr;
>  g_assert_cmphex(hba_base, ==, hba_stored);
>  
>  g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index 3979b20..8c42ca9 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -87,7 +87,7 @@
>  
>  typedef struct e1000e_device {
>  QPCIDevice *pci_dev;
> -void *mac_regs;
> +QPCIBar mac_regs;
>  
>  uint64_t tx_ring;
>  uint64_t rx_ring;
> @@ -119,12 +119,12 @@ static QPCIDevice *e1000e_device_find(QPCIBus *bus)
>  
>  static void e1000e_macreg_write(e1000e_device *d, uint32_t reg, uint32_t val)
>  {
> -qpci_io_writel(d->pci_dev, d->mac_regs + reg, val);
> +qpci_io_writel(d->pci_dev, d->mac_regs, reg, val);
>  }
>  
>  static uint32_t e1000e_macreg_read(e1000e_device *d, uint32_t reg)
>  {
> -return qpci_io_readl(d->pci_dev, d->mac_regs + reg);
> +return qpci_io_readl(d->pci_dev, d->mac_regs, reg);
>  }
>  
>  static void e1000e_device_init(QPCIBus *bus, e1000e_device *d)
> @@ -138,7 +138,6 @@ static void e1000e_device_init(QPCIBus *bus, 
> e1000e_device *d)
>  
>  /* Map BAR0 (mac registers) */
>  d->mac_regs = qpci_iomap(d->pci_dev, 0, NULL);
> -g_assert_nonnull(d->mac_regs);
>  
>  /* Reset the device */
>  val = e1000e_macreg_read(d, E1000E_CTRL);
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index a8a4081..dc08536 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -137,7 +137,7 @@ static void ide_test_quit(void)
>  qtest_end();
>  }
>  
> -static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
> +static QPCIDevice *get_pci_device(QPCIBar *bmdma_bar)
>  {
>  QPCIDevice *dev;
>  uint16_t vendor_id, device_id;
> @@ -156,7 +156,7 @@ static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
>  g_assert(device_id == PCI_DEVICE_ID_INTEL_82371SB_1);
>  
>  /* Map bmdma BAR */
> -*bmdma_base = (uint16_t)(uintptr_t) qpci_iomap(dev, 4, NULL);
> +*bmdma_bar = qpci_iomap(dev, 4, NULL);
>  
>  qpci_device_enable(dev);
>  
> @@ -182,14 +182,14 @@ static int send_dma_request(int cmd, uint64_t sector, 
> int nb_sectors,
>  void(*post_exec)(uint64_t sector, int 
> nb_sectors))
>  {
>  

[Qemu-devel] [PATCH 8/8] libqos: Change PCI accessors to take opaque BAR handle

[David Gibson]

The usual use model for the libqos PCI functions is to map a specific PCI
BAR using qpci_iomap() then pass the returned token into IO accessor
functions.  This, and the fact that iomap() returns a (void *) which
actually contains a PCI space address, kind of suggests that the return
value from iomap is supposed to be an opaque token.

..except that the callers expect to be able to add offsets to it.  Which
also assumes the compiler will support pointer arithmetic on a (void *),
and treat it as working with byte offsets.

To clarify this situation change iomap() and the IO accessors to take
a definitely opaque BAR handle (enforced with a wrapper struct) along with
an offset within the BAR.  This changes both the functions and all the
callers.

A few notes:
* Asserts that iomap() returns non-NULL are removed in some places;
iomap() already asserts if it can't map the BAR
* In ide-test.c we change explicit outb() etc. calls to matching
qpci_io_writeb() calls.  That makes the test more portable, and removes
assumptions that the test case shouldn't be making about how iomap()'s
return value is formatted internally.

Signed-off-by: David Gibson 

# Conflicts:
#   tests/libqos/virtio-pci.c
---
 tests/ahci-test.c |   4 +-
 tests/e1000e-test.c   |   7 ++-
 tests/ide-test.c  |  23 +
 tests/ivshmem-test.c  |  28 +--
 tests/libqos/ahci.c   |   3 +-
 tests/libqos/ahci.h   |   6 +--
 tests/libqos/pci.c| 125 +-
 tests/libqos/pci.h|  39 +--
 tests/libqos/usb.c|   6 +--
 tests/libqos/usb.h|   2 +-
 tests/libqos/virtio-pci.c | 113 +
 tests/libqos/virtio-pci.h |   2 +-
 tests/rtl8139-test.c  |  10 ++--
 tests/usb-hcd-ehci-test.c |   5 +-
 14 files changed, 186 insertions(+), 187 deletions(-)

diff --git a/tests/ahci-test.c b/tests/ahci-test.c
index 9c0adce..4358631 100644
--- a/tests/ahci-test.c
+++ b/tests/ahci-test.c
@@ -90,12 +90,12 @@ static void verify_state(AHCIQState *ahci)
 g_assert_cmphex(ahci_fingerprint, ==, ahci->fingerprint);
 
 /* If we haven't initialized, this is as much as can be validated. */
-if (!ahci->hba_base) {
+if (!ahci->hba_bar.addr) {
 return;
 }
 
 hba_base = (uint64_t)qpci_config_readl(ahci->dev, PCI_BASE_ADDRESS_5);
-hba_stored = (uint64_t)(uintptr_t)ahci->hba_base;
+hba_stored = ahci->hba_bar.addr;
 g_assert_cmphex(hba_base, ==, hba_stored);
 
 g_assert_cmphex(ahci_rreg(ahci, AHCI_CAP), ==, ahci->cap);
diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
index 3979b20..8c42ca9 100644
--- a/tests/e1000e-test.c
+++ b/tests/e1000e-test.c
@@ -87,7 +87,7 @@
 
 typedef struct e1000e_device {
 QPCIDevice *pci_dev;
-void *mac_regs;
+QPCIBar mac_regs;
 
 uint64_t tx_ring;
 uint64_t rx_ring;
@@ -119,12 +119,12 @@ static QPCIDevice *e1000e_device_find(QPCIBus *bus)
 
 static void e1000e_macreg_write(e1000e_device *d, uint32_t reg, uint32_t val)
 {
-qpci_io_writel(d->pci_dev, d->mac_regs + reg, val);
+qpci_io_writel(d->pci_dev, d->mac_regs, reg, val);
 }
 
 static uint32_t e1000e_macreg_read(e1000e_device *d, uint32_t reg)
 {
-return qpci_io_readl(d->pci_dev, d->mac_regs + reg);
+return qpci_io_readl(d->pci_dev, d->mac_regs, reg);
 }
 
 static void e1000e_device_init(QPCIBus *bus, e1000e_device *d)
@@ -138,7 +138,6 @@ static void e1000e_device_init(QPCIBus *bus, e1000e_device 
*d)
 
 /* Map BAR0 (mac registers) */
 d->mac_regs = qpci_iomap(d->pci_dev, 0, NULL);
-g_assert_nonnull(d->mac_regs);
 
 /* Reset the device */
 val = e1000e_macreg_read(d, E1000E_CTRL);
diff --git a/tests/ide-test.c b/tests/ide-test.c
index a8a4081..dc08536 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -137,7 +137,7 @@ static void ide_test_quit(void)
 qtest_end();
 }
 
-static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
+static QPCIDevice *get_pci_device(QPCIBar *bmdma_bar)
 {
 QPCIDevice *dev;
 uint16_t vendor_id, device_id;
@@ -156,7 +156,7 @@ static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
 g_assert(device_id == PCI_DEVICE_ID_INTEL_82371SB_1);
 
 /* Map bmdma BAR */
-*bmdma_base = (uint16_t)(uintptr_t) qpci_iomap(dev, 4, NULL);
+*bmdma_bar = qpci_iomap(dev, 4, NULL);
 
 qpci_device_enable(dev);
 
@@ -182,14 +182,14 @@ static int send_dma_request(int cmd, uint64_t sector, int 
nb_sectors,
 void(*post_exec)(uint64_t sector, int nb_sectors))
 {
 QPCIDevice *dev;
-uint16_t bmdma_base;
+QPCIBar bmdma_bar;
 uintptr_t guest_prdt;
 size_t len;
 bool from_dev;
 uint8_t status;
 int flags;
 
-dev = get_pci_device(_base);
+dev = get_pci_device(_bar);
 
 flags = cmd & ~0xff;
 cmd &= 0xff;
@@ -217,14 +217,14 @@ static int send_dma_request(int cmd, uint64_t sector, int 
nb_sectors,