diff mbox

[PATCHv5,08/12] tests: Clean up IO handling in ide-test

Message ID 1477285201-10244-9-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Oct. 24, 2016, 4:59 a.m. UTC
ide-test uses many explicit inb() / outb() operations for its IO, which
means it's not portable to non-x86 platforms.  This cleans it up to use
the libqos PCI accessors instead.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tests/ide-test.c | 179 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 118 insertions(+), 61 deletions(-)

Comments

Alexey Kardashevskiy Oct. 25, 2016, 7:01 a.m. UTC | #1
On 24/10/16 15:59, David Gibson wrote:
> ide-test uses many explicit inb() / outb() operations for its IO, which
> means it's not portable to non-x86 platforms.  This cleans it up to use
> the libqos PCI accessors instead.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tests/ide-test.c | 179 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 118 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index a8a4081..86c4373 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(void **bmdma_base, void **ide_base)
>  {
>      QPCIDevice *dev;
>      uint16_t vendor_id, device_id;
> @@ -156,7 +156,9 @@ 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_base = qpci_iomap(dev, 4, NULL);
> +
> +    *ide_base = qpci_legacy_iomap(dev, IDE_BASE);
>  
>      qpci_device_enable(dev);
>  
> @@ -179,17 +181,19 @@ typedef struct PrdtEntry {
>  
>  static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>                              PrdtEntry *prdt, int prdt_entries,
> -                            void(*post_exec)(uint64_t sector, int nb_sectors))
> +                            void(*post_exec)(QPCIDevice *dev, void *ide_base,
> +                                             uint64_t sector, int nb_sectors))
>  {
>      QPCIDevice *dev;
> -    uint16_t bmdma_base;
> +    void *bmdma_base;
> +    void *ide_base;
>      uintptr_t guest_prdt;
>      size_t len;
>      bool from_dev;
>      uint8_t status;
>      int flags;
>  
> -    dev = get_pci_device(&bmdma_base);
> +    dev = get_pci_device(&bmdma_base, &ide_base);
>  
>      flags = cmd & ~0xff;
>      cmd &= 0xff;
> @@ -214,59 +218,60 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>      }
>  
>      /* Select device 0 */
> -    outb(IDE_BASE + reg_device, 0 | LBA);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0 | LBA);
>  
>      /* Stop any running transfer, clear any pending interrupt */
> -    outb(bmdma_base + bmreg_cmd, 0);
> -    outb(bmdma_base + bmreg_status, BM_STS_INTR);
> +    qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
> +    qpci_io_writeb(dev, bmdma_base + bmreg_status, BM_STS_INTR);
>  
>      /* Setup PRDT */
>      len = sizeof(*prdt) * prdt_entries;
>      guest_prdt = guest_alloc(guest_malloc, len);
>      memwrite(guest_prdt, prdt, len);
> -    outl(bmdma_base + bmreg_prdt, guest_prdt);
> +    qpci_io_writel(dev, bmdma_base + bmreg_prdt, guest_prdt);
>  
>      /* ATA DMA command */
>      if (cmd == CMD_PACKET) {
>          /* Enables ATAPI DMA; otherwise PIO is attempted */
> -        outb(IDE_BASE + reg_feature, 0x01);
> +        qpci_io_writeb(dev, ide_base + reg_feature, 0x01);
>      } else {
> -        outb(IDE_BASE + reg_nsectors, nb_sectors);
> -        outb(IDE_BASE + reg_lba_low,    sector & 0xff);
> -        outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
> -        outb(IDE_BASE + reg_lba_high,   (sector >> 16) & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_nsectors, nb_sectors);
> +        qpci_io_writeb(dev, ide_base + reg_lba_low,    sector & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_lba_middle, (sector >> 8) & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_lba_high,   (sector >> 16) & 0xff);
>      }
>  
> -    outb(IDE_BASE + reg_command, cmd);
> +    qpci_io_writeb(dev, ide_base + reg_command, cmd);
>  
>      if (post_exec) {
> -        post_exec(sector, nb_sectors);
> +        post_exec(dev, ide_base, sector, nb_sectors);
>      }
>  
>      /* Start DMA transfer */
> -    outb(bmdma_base + bmreg_cmd, BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
> +    qpci_io_writeb(dev, bmdma_base + bmreg_cmd,
> +                   BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
>  
>      if (flags & CMDF_ABORT) {
> -        outb(bmdma_base + bmreg_cmd, 0);
> +        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
>      }
>  
>      /* Wait for the DMA transfer to complete */
>      do {
> -        status = inb(bmdma_base + bmreg_status);
> +        status = qpci_io_readb(dev, bmdma_base + bmreg_status);
>      } while ((status & (BM_STS_ACTIVE | BM_STS_INTR)) == BM_STS_ACTIVE);
>  
>      g_assert_cmpint(get_irq(IDE_PRIMARY_IRQ), ==, !!(status & BM_STS_INTR));
>  
>      /* Check IDE status code */
> -    assert_bit_set(inb(IDE_BASE + reg_status), DRDY);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), BSY | DRQ);
> +    assert_bit_set(qpci_io_readb(dev, ide_base + reg_status), DRDY);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), BSY | DRQ);
>  
>      /* Reading the status register clears the IRQ */
>      g_assert(!get_irq(IDE_PRIMARY_IRQ));
>  
>      /* Stop DMA transfer if still active */
>      if (status & BM_STS_ACTIVE) {
> -        outb(bmdma_base + bmreg_cmd, 0);
> +        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
>      }
>  
>      free_pci_device(dev);
> @@ -276,6 +281,8 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>  
>  static void test_bmdma_simple_rw(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>      uint8_t *buf;
>      uint8_t *cmpbuf;
> @@ -289,6 +296,8 @@ static void test_bmdma_simple_rw(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      buf = g_malloc(len);
>      cmpbuf = g_malloc(len);
>  
> @@ -299,7 +308,7 @@ static void test_bmdma_simple_rw(void)
>      status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Write 0xaa pattern to sector 1 */
>      memset(buf, 0xaa, len);
> @@ -308,14 +317,14 @@ static void test_bmdma_simple_rw(void)
>      status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Read and verify 0x55 pattern in sector 0 */
>      memset(cmpbuf, 0x55, len);
>  
>      status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      memread(guest_buf, buf, len);
>      g_assert(memcmp(buf, cmpbuf, len) == 0);
> @@ -325,7 +334,7 @@ static void test_bmdma_simple_rw(void)
>  
>      status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      memread(guest_buf, buf, len);
>      g_assert(memcmp(buf, cmpbuf, len) == 0);
> @@ -337,6 +346,8 @@ static void test_bmdma_simple_rw(void)
>  
>  static void test_bmdma_short_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      PrdtEntry prdt[] = {
> @@ -346,21 +357,25 @@ static void test_bmdma_short_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_one_sector_short_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      /* Read 2 sectors but only give 1 sector in PRDT */
> @@ -371,21 +386,25 @@ static void test_bmdma_one_sector_short_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 2,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_long_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      PrdtEntry prdt[] = {
> @@ -395,23 +414,29 @@ static void test_bmdma_long_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_no_busmaster(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't be
>       * able to access it anyway because the Bus Master bit in the PCI command
>       * register isn't set. This is complete nonsense, but it used to be pretty
> @@ -424,7 +449,7 @@ static void test_bmdma_no_busmaster(void)
>      /* Not entirely clear what the expected result is, but this is what we get
>       * in practice. At least we want to be aware of any changes. */
>      g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_setup(void)
> @@ -454,6 +479,8 @@ static void string_cpu_to_be16(uint16_t *s, size_t bytes)
>  
>  static void test_identify(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      uint16_t buf[256];
>      int i;
> @@ -464,23 +491,25 @@ static void test_identify(void)
>          "-global ide-hd.ver=%s",
>          tmp_path, "testdisk", "version");
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* IDENTIFY command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_IDENTIFY);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_IDENTIFY);
>  
>      /* Read in the IDENTIFY buffer and check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      for (i = 0; i < 256; i++) {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>          assert_bit_set(data, DRDY | DRQ);
>          assert_bit_clear(data, BSY | DF | ERR);
>  
> -        ((uint16_t*) buf)[i] = inw(IDE_BASE + reg_data);
> +        buf[i] = qpci_io_readw(dev, ide_base + reg_data);
>      }
>  
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, DRDY);
>      assert_bit_clear(data, BSY | DF | ERR | DRQ);
>  
> @@ -505,11 +534,15 @@ static void test_identify(void)
>   */
>  static void make_dirty(uint8_t device)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>      size_t len = 512;
>      uintptr_t guest_buf;
>      void* buf;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      guest_buf = guest_alloc(guest_malloc, len);
>      buf = g_malloc(len);
>      g_assert(guest_buf);
> @@ -527,19 +560,23 @@ static void make_dirty(uint8_t device)
>      status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      g_free(buf);
>  }
>  
>  static void test_flush(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>  
>      ide_test_start(
>          "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
>          tmp_path);
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
> @@ -549,11 +586,11 @@ static void test_flush(void)
>      g_free(hmp("qemu-io ide0-hd0 \"break flush_to_os A\""));
>  
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Check status while request is in flight*/
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, BSY | DRDY);
>      assert_bit_clear(data, DF | ERR | DRQ);
>  
> @@ -561,11 +598,11 @@ static void test_flush(void)
>      g_free(hmp("qemu-io ide0-hd0 \"resume A\""));
>  
>      /* Check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      do {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>      } while (data & BSY);
>  
>      assert_bit_set(data, DRDY);
> @@ -576,6 +613,8 @@ static void test_flush(void)
>  
>  static void test_retry_flush(const char *machine)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      const char *s;
>  
> @@ -587,17 +626,19 @@ static void test_retry_flush(const char *machine)
>          "rerror=stop,werror=stop",
>          debug_path, tmp_path);
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
>      make_dirty(0);
>  
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Check status while request is in flight*/
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, BSY | DRDY);
>      assert_bit_clear(data, DF | ERR | DRQ);
>  
> @@ -608,11 +649,11 @@ static void test_retry_flush(const char *machine)
>      qmp_discard_response(s);
>  
>      /* Check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      do {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>      } while (data & BSY);
>  
>      assert_bit_set(data, DRDY);
> @@ -623,11 +664,16 @@ static void test_retry_flush(const char *machine)
>  
>  static void test_flush_nodev(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
> +
>      ide_test_start("");
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Just testing that qemu doesn't crash... */
>  
> @@ -654,7 +700,8 @@ typedef struct Read10CDB {
>      uint16_t padding;
>  } __attribute__((__packed__)) Read10CDB;
>  
> -static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
> +                                 uint64_t lba, int nblocks)
>  {
>      Read10CDB pkt = { .padding = 0 };
>      int i;
> @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
>  
>      /* Send Packet */
>      for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> -        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
> +        qpci_io_writew(dev, ide_base + reg_data,
> +                       le16_to_cpu(((uint16_t *)&pkt)[i]));


cpu_to_le16 -> le16_to_cpu conversion here and below (at the very end) is
not obvious. Right above this chunk the @pkt fields are initialized as BE:

 /* Construct SCSI CDB packet */
 pkt.opcode = 0x28;
 pkt.lba = cpu_to_be32(lba);
 pkt.nblocks = cpu_to_be16(nblocks);

outw() seems to be CPU-endian, and qpci_io_writew() as well, or not?



>      }
>  }
>  
> @@ -683,13 +731,17 @@ static void nsleep(int64_t nsecs)
>  
>  static uint8_t ide_wait_clear(uint8_t flag)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      time_t st;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Wait with a 5 second timeout */
>      time(&st);
>      while (true) {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>          if (!(data & flag)) {
>              return data;
>          }
> @@ -723,6 +775,8 @@ static void ide_wait_intr(int irq)
>  
>  static void cdrom_pio_impl(int nblocks)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      FILE *fh;
>      int patt_blocks = MAX(16, nblocks);
>      size_t patt_len = ATAPI_BLOCK_SIZE * patt_blocks;
> @@ -741,13 +795,15 @@ static void cdrom_pio_impl(int nblocks)
>  
>      ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
>                     "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
> +    dev = get_pci_device(&bmdma_base, &ide_base);
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* PACKET command on device 0 */
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> -    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> -    outb(IDE_BASE + reg_command, CMD_PACKET);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> +    qpci_io_writeb(dev, ide_base + reg_lba_high,
> +                   (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_PACKET);
>      /* HP0: Check_Status_A State */
>      nsleep(400);
>      data = ide_wait_clear(BSY);
> @@ -756,7 +812,7 @@ static void cdrom_pio_impl(int nblocks)
>      assert_bit_clear(data, ERR | DF | BSY);
>  
>      /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
> -    send_scsi_cdb_read10(0, nblocks);
> +    send_scsi_cdb_read10(dev, ide_base, 0, nblocks);
>  
>      /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
>       * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
> @@ -780,7 +836,8 @@ static void cdrom_pio_impl(int nblocks)
>  
>          /* HP4: Transfer_Data */
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
> -            rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> +            rx[offset + j] = cpu_to_le16(qpci_io_readw(dev,
> +                                                       ide_base + reg_data));
>          }
>      }
>  
>
David Gibson Oct. 25, 2016, 12:25 p.m. UTC | #2
On Tue, Oct 25, 2016 at 06:01:41PM +1100, Alexey Kardashevskiy wrote:
> On 24/10/16 15:59, David Gibson wrote:
> > ide-test uses many explicit inb() / outb() operations for its IO, which
> > means it's not portable to non-x86 platforms.  This cleans it up to use
> > the libqos PCI accessors instead.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[snip]

> > -static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> > +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
> > +                                 uint64_t lba, int nblocks)
> >  {
> >      Read10CDB pkt = { .padding = 0 };
> >      int i;
> > @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> >  
> >      /* Send Packet */
> >      for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> > -        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
> > +        qpci_io_writew(dev, ide_base + reg_data,
> > +                       le16_to_cpu(((uint16_t *)&pkt)[i]));
> 
> 
> cpu_to_le16 -> le16_to_cpu conversion here and below (at the very end) is
> not obvious. Right above this chunk the @pkt fields are initialized as BE:
> 
>  /* Construct SCSI CDB packet */
>  pkt.opcode = 0x28;
>  pkt.lba = cpu_to_be32(lba);
>  pkt.nblocks = cpu_to_be16(nblocks);
> 
> outw() seems to be CPU-endian, and qpci_io_writew() as well, or not?

outw() is guest CPU endian (which is stupid, but that's another
matter).  qpci_io_writew() is different - it is always LE, because PCI
devices are always LE (well, ok, nearly always).

So, yes, this is a bit confusing.  Here's what's going on:
  * the SCSI standard uses BE, so that's what we put into the
    packet structure
  * We need to transfer the packet to the device as a bytestream - so
    no endianness conversions
  * But.. we do so in 16-bit chunks
  * .. and qpci_io_writew() is designed to take CPU values and write
    them out as LE - ie, it contains an implicit cpu_to_le16()
  * So, we use le16_to_cpu() to cancel out that swap, so that we write
    out the bytes in the correct order

If this were a Linux driver here, then we'd use a writew_rep() type
primitive, instead of writew().  The rep / streaming variants don't
byteswap exactly because they're designed for streaming out bytestream
data, not word values.

Really what we want here is a memwrite() type operation, but that
doesn't quite work because this is PIO, not MMIO, so on x86 it turns
into CPU ins and outs which don't have "streaming" variants.


> >      }
> >  }
> >  
> > @@ -683,13 +731,17 @@ static void nsleep(int64_t nsecs)
> >  
> >  static uint8_t ide_wait_clear(uint8_t flag)
> >  {
> > +    QPCIDevice *dev;
> > +    void *bmdma_base, *ide_base;
> >      uint8_t data;
> >      time_t st;
> >  
> > +    dev = get_pci_device(&bmdma_base, &ide_base);
> > +
> >      /* Wait with a 5 second timeout */
> >      time(&st);
> >      while (true) {
> > -        data = inb(IDE_BASE + reg_status);
> > +        data = qpci_io_readb(dev, ide_base + reg_status);
> >          if (!(data & flag)) {
> >              return data;
> >          }
> > @@ -723,6 +775,8 @@ static void ide_wait_intr(int irq)
> >  
> >  static void cdrom_pio_impl(int nblocks)
> >  {
> > +    QPCIDevice *dev;
> > +    void *bmdma_base, *ide_base;
> >      FILE *fh;
> >      int patt_blocks = MAX(16, nblocks);
> >      size_t patt_len = ATAPI_BLOCK_SIZE * patt_blocks;
> > @@ -741,13 +795,15 @@ static void cdrom_pio_impl(int nblocks)
> >  
> >      ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
> >                     "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
> > +    dev = get_pci_device(&bmdma_base, &ide_base);
> >      qtest_irq_intercept_in(global_qtest, "ioapic");
> >  
> >      /* PACKET command on device 0 */
> > -    outb(IDE_BASE + reg_device, 0);
> > -    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> > -    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> > -    outb(IDE_BASE + reg_command, CMD_PACKET);
> > +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> > +    qpci_io_writeb(dev, ide_base + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> > +    qpci_io_writeb(dev, ide_base + reg_lba_high,
> > +                   (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> > +    qpci_io_writeb(dev, ide_base + reg_command, CMD_PACKET);
> >      /* HP0: Check_Status_A State */
> >      nsleep(400);
> >      data = ide_wait_clear(BSY);
> > @@ -756,7 +812,7 @@ static void cdrom_pio_impl(int nblocks)
> >      assert_bit_clear(data, ERR | DF | BSY);
> >  
> >      /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
> > -    send_scsi_cdb_read10(0, nblocks);
> > +    send_scsi_cdb_read10(dev, ide_base, 0, nblocks);
> >  
> >      /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
> >       * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
> > @@ -780,7 +836,8 @@ static void cdrom_pio_impl(int nblocks)
> >  
> >          /* HP4: Transfer_Data */
> >          for (j = 0; j < MIN((limit / 2), rem); j++) {
> > -            rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> > +            rx[offset + j] = cpu_to_le16(qpci_io_readw(dev,
> > +                                                       ide_base + reg_data));
> >          }
> >      }
> >  
> > 
> 
>
Greg Kurz Oct. 25, 2016, 12:36 p.m. UTC | #3
On Mon, 24 Oct 2016 15:59:57 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> ide-test uses many explicit inb() / outb() operations for its IO, which
> means it's not portable to non-x86 platforms.  This cleans it up to use
> the libqos PCI accessors instead.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/ide-test.c | 179 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 118 insertions(+), 61 deletions(-)
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index a8a4081..86c4373 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(void **bmdma_base, void **ide_base)
>  {
>      QPCIDevice *dev;
>      uint16_t vendor_id, device_id;
> @@ -156,7 +156,9 @@ 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_base = qpci_iomap(dev, 4, NULL);
> +
> +    *ide_base = qpci_legacy_iomap(dev, IDE_BASE);
>  
>      qpci_device_enable(dev);
>  
> @@ -179,17 +181,19 @@ typedef struct PrdtEntry {
>  
>  static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>                              PrdtEntry *prdt, int prdt_entries,
> -                            void(*post_exec)(uint64_t sector, int nb_sectors))
> +                            void(*post_exec)(QPCIDevice *dev, void *ide_base,
> +                                             uint64_t sector, int nb_sectors))
>  {
>      QPCIDevice *dev;
> -    uint16_t bmdma_base;
> +    void *bmdma_base;
> +    void *ide_base;
>      uintptr_t guest_prdt;
>      size_t len;
>      bool from_dev;
>      uint8_t status;
>      int flags;
>  
> -    dev = get_pci_device(&bmdma_base);
> +    dev = get_pci_device(&bmdma_base, &ide_base);
>  
>      flags = cmd & ~0xff;
>      cmd &= 0xff;
> @@ -214,59 +218,60 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>      }
>  
>      /* Select device 0 */
> -    outb(IDE_BASE + reg_device, 0 | LBA);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0 | LBA);
>  
>      /* Stop any running transfer, clear any pending interrupt */
> -    outb(bmdma_base + bmreg_cmd, 0);
> -    outb(bmdma_base + bmreg_status, BM_STS_INTR);
> +    qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
> +    qpci_io_writeb(dev, bmdma_base + bmreg_status, BM_STS_INTR);
>  
>      /* Setup PRDT */
>      len = sizeof(*prdt) * prdt_entries;
>      guest_prdt = guest_alloc(guest_malloc, len);
>      memwrite(guest_prdt, prdt, len);
> -    outl(bmdma_base + bmreg_prdt, guest_prdt);
> +    qpci_io_writel(dev, bmdma_base + bmreg_prdt, guest_prdt);
>  
>      /* ATA DMA command */
>      if (cmd == CMD_PACKET) {
>          /* Enables ATAPI DMA; otherwise PIO is attempted */
> -        outb(IDE_BASE + reg_feature, 0x01);
> +        qpci_io_writeb(dev, ide_base + reg_feature, 0x01);
>      } else {
> -        outb(IDE_BASE + reg_nsectors, nb_sectors);
> -        outb(IDE_BASE + reg_lba_low,    sector & 0xff);
> -        outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
> -        outb(IDE_BASE + reg_lba_high,   (sector >> 16) & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_nsectors, nb_sectors);
> +        qpci_io_writeb(dev, ide_base + reg_lba_low,    sector & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_lba_middle, (sector >> 8) & 0xff);
> +        qpci_io_writeb(dev, ide_base + reg_lba_high,   (sector >> 16) & 0xff);
>      }
>  
> -    outb(IDE_BASE + reg_command, cmd);
> +    qpci_io_writeb(dev, ide_base + reg_command, cmd);
>  
>      if (post_exec) {
> -        post_exec(sector, nb_sectors);
> +        post_exec(dev, ide_base, sector, nb_sectors);
>      }
>  
>      /* Start DMA transfer */
> -    outb(bmdma_base + bmreg_cmd, BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
> +    qpci_io_writeb(dev, bmdma_base + bmreg_cmd,
> +                   BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
>  
>      if (flags & CMDF_ABORT) {
> -        outb(bmdma_base + bmreg_cmd, 0);
> +        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
>      }
>  
>      /* Wait for the DMA transfer to complete */
>      do {
> -        status = inb(bmdma_base + bmreg_status);
> +        status = qpci_io_readb(dev, bmdma_base + bmreg_status);
>      } while ((status & (BM_STS_ACTIVE | BM_STS_INTR)) == BM_STS_ACTIVE);
>  
>      g_assert_cmpint(get_irq(IDE_PRIMARY_IRQ), ==, !!(status & BM_STS_INTR));
>  
>      /* Check IDE status code */
> -    assert_bit_set(inb(IDE_BASE + reg_status), DRDY);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), BSY | DRQ);
> +    assert_bit_set(qpci_io_readb(dev, ide_base + reg_status), DRDY);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), BSY | DRQ);
>  
>      /* Reading the status register clears the IRQ */
>      g_assert(!get_irq(IDE_PRIMARY_IRQ));
>  
>      /* Stop DMA transfer if still active */
>      if (status & BM_STS_ACTIVE) {
> -        outb(bmdma_base + bmreg_cmd, 0);
> +        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
>      }
>  
>      free_pci_device(dev);
> @@ -276,6 +281,8 @@ static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
>  
>  static void test_bmdma_simple_rw(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>      uint8_t *buf;
>      uint8_t *cmpbuf;
> @@ -289,6 +296,8 @@ static void test_bmdma_simple_rw(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      buf = g_malloc(len);
>      cmpbuf = g_malloc(len);
>  
> @@ -299,7 +308,7 @@ static void test_bmdma_simple_rw(void)
>      status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Write 0xaa pattern to sector 1 */
>      memset(buf, 0xaa, len);
> @@ -308,14 +317,14 @@ static void test_bmdma_simple_rw(void)
>      status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Read and verify 0x55 pattern in sector 0 */
>      memset(cmpbuf, 0x55, len);
>  
>      status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      memread(guest_buf, buf, len);
>      g_assert(memcmp(buf, cmpbuf, len) == 0);
> @@ -325,7 +334,7 @@ static void test_bmdma_simple_rw(void)
>  
>      status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      memread(guest_buf, buf, len);
>      g_assert(memcmp(buf, cmpbuf, len) == 0);
> @@ -337,6 +346,8 @@ static void test_bmdma_simple_rw(void)
>  
>  static void test_bmdma_short_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      PrdtEntry prdt[] = {
> @@ -346,21 +357,25 @@ static void test_bmdma_short_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_one_sector_short_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      /* Read 2 sectors but only give 1 sector in PRDT */
> @@ -371,21 +386,25 @@ static void test_bmdma_one_sector_short_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 2,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, 0);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_long_prdt(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
>      PrdtEntry prdt[] = {
> @@ -395,23 +414,29 @@ static void test_bmdma_long_prdt(void)
>          },
>      };
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Normal request */
>      status = send_dma_request(CMD_READ_DMA, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      /* Abort the request before it completes */
>      status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
>                                prdt, ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_no_busmaster(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't be
>       * able to access it anyway because the Bus Master bit in the PCI command
>       * register isn't set. This is complete nonsense, but it used to be pretty
> @@ -424,7 +449,7 @@ static void test_bmdma_no_busmaster(void)
>      /* Not entirely clear what the expected result is, but this is what we get
>       * in practice. At least we want to be aware of any changes. */
>      g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  }
>  
>  static void test_bmdma_setup(void)
> @@ -454,6 +479,8 @@ static void string_cpu_to_be16(uint16_t *s, size_t bytes)
>  
>  static void test_identify(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      uint16_t buf[256];
>      int i;
> @@ -464,23 +491,25 @@ static void test_identify(void)
>          "-global ide-hd.ver=%s",
>          tmp_path, "testdisk", "version");
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* IDENTIFY command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_IDENTIFY);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_IDENTIFY);
>  
>      /* Read in the IDENTIFY buffer and check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      for (i = 0; i < 256; i++) {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>          assert_bit_set(data, DRDY | DRQ);
>          assert_bit_clear(data, BSY | DF | ERR);
>  
> -        ((uint16_t*) buf)[i] = inw(IDE_BASE + reg_data);
> +        buf[i] = qpci_io_readw(dev, ide_base + reg_data);
>      }
>  
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, DRDY);
>      assert_bit_clear(data, BSY | DF | ERR | DRQ);
>  
> @@ -505,11 +534,15 @@ static void test_identify(void)
>   */
>  static void make_dirty(uint8_t device)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t status;
>      size_t len = 512;
>      uintptr_t guest_buf;
>      void* buf;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      guest_buf = guest_alloc(guest_malloc, len);
>      buf = g_malloc(len);
>      g_assert(guest_buf);
> @@ -527,19 +560,23 @@ static void make_dirty(uint8_t device)
>      status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
>                                ARRAY_SIZE(prdt), NULL);
>      g_assert_cmphex(status, ==, BM_STS_INTR);
> -    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
> +    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
>  
>      g_free(buf);
>  }
>  
>  static void test_flush(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>  
>      ide_test_start(
>          "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
>          tmp_path);
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
> @@ -549,11 +586,11 @@ static void test_flush(void)
>      g_free(hmp("qemu-io ide0-hd0 \"break flush_to_os A\""));
>  
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Check status while request is in flight*/
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, BSY | DRDY);
>      assert_bit_clear(data, DF | ERR | DRQ);
>  
> @@ -561,11 +598,11 @@ static void test_flush(void)
>      g_free(hmp("qemu-io ide0-hd0 \"resume A\""));
>  
>      /* Check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      do {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>      } while (data & BSY);
>  
>      assert_bit_set(data, DRDY);
> @@ -576,6 +613,8 @@ static void test_flush(void)
>  
>  static void test_retry_flush(const char *machine)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      const char *s;
>  
> @@ -587,17 +626,19 @@ static void test_retry_flush(const char *machine)
>          "rerror=stop,werror=stop",
>          debug_path, tmp_path);
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
>      make_dirty(0);
>  
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Check status while request is in flight*/
> -    data = inb(IDE_BASE + reg_status);
> +    data = qpci_io_readb(dev, ide_base + reg_status);
>      assert_bit_set(data, BSY | DRDY);
>      assert_bit_clear(data, DF | ERR | DRQ);
>  
> @@ -608,11 +649,11 @@ static void test_retry_flush(const char *machine)
>      qmp_discard_response(s);
>  
>      /* Check registers */
> -    data = inb(IDE_BASE + reg_device);
> +    data = qpci_io_readb(dev, ide_base + reg_device);
>      g_assert_cmpint(data & DEV, ==, 0);
>  
>      do {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>      } while (data & BSY);
>  
>      assert_bit_set(data, DRDY);
> @@ -623,11 +664,16 @@ static void test_retry_flush(const char *machine)
>  
>  static void test_flush_nodev(void)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
> +
>      ide_test_start("");
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* FLUSH CACHE command on device 0*/
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
>  
>      /* Just testing that qemu doesn't crash... */
>  
> @@ -654,7 +700,8 @@ typedef struct Read10CDB {
>      uint16_t padding;
>  } __attribute__((__packed__)) Read10CDB;
>  
> -static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
> +                                 uint64_t lba, int nblocks)
>  {
>      Read10CDB pkt = { .padding = 0 };
>      int i;
> @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
>  
>      /* Send Packet */
>      for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> -        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
> +        qpci_io_writew(dev, ide_base + reg_data,
> +                       le16_to_cpu(((uint16_t *)&pkt)[i]));
>      }
>  }
>  
> @@ -683,13 +731,17 @@ static void nsleep(int64_t nsecs)
>  
>  static uint8_t ide_wait_clear(uint8_t flag)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      uint8_t data;
>      time_t st;
>  
> +    dev = get_pci_device(&bmdma_base, &ide_base);
> +
>      /* Wait with a 5 second timeout */
>      time(&st);
>      while (true) {
> -        data = inb(IDE_BASE + reg_status);
> +        data = qpci_io_readb(dev, ide_base + reg_status);
>          if (!(data & flag)) {
>              return data;
>          }
> @@ -723,6 +775,8 @@ static void ide_wait_intr(int irq)
>  
>  static void cdrom_pio_impl(int nblocks)
>  {
> +    QPCIDevice *dev;
> +    void *bmdma_base, *ide_base;
>      FILE *fh;
>      int patt_blocks = MAX(16, nblocks);
>      size_t patt_len = ATAPI_BLOCK_SIZE * patt_blocks;
> @@ -741,13 +795,15 @@ static void cdrom_pio_impl(int nblocks)
>  
>      ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
>                     "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
> +    dev = get_pci_device(&bmdma_base, &ide_base);
>      qtest_irq_intercept_in(global_qtest, "ioapic");
>  
>      /* PACKET command on device 0 */
> -    outb(IDE_BASE + reg_device, 0);
> -    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> -    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> -    outb(IDE_BASE + reg_command, CMD_PACKET);
> +    qpci_io_writeb(dev, ide_base + reg_device, 0);
> +    qpci_io_writeb(dev, ide_base + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
> +    qpci_io_writeb(dev, ide_base + reg_lba_high,
> +                   (BYTE_COUNT_LIMIT >> 8 & 0xFF));
> +    qpci_io_writeb(dev, ide_base + reg_command, CMD_PACKET);
>      /* HP0: Check_Status_A State */
>      nsleep(400);
>      data = ide_wait_clear(BSY);
> @@ -756,7 +812,7 @@ static void cdrom_pio_impl(int nblocks)
>      assert_bit_clear(data, ERR | DF | BSY);
>  
>      /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
> -    send_scsi_cdb_read10(0, nblocks);
> +    send_scsi_cdb_read10(dev, ide_base, 0, nblocks);
>  
>      /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
>       * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
> @@ -780,7 +836,8 @@ static void cdrom_pio_impl(int nblocks)
>  
>          /* HP4: Transfer_Data */
>          for (j = 0; j < MIN((limit / 2), rem); j++) {
> -            rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
> +            rx[offset + j] = cpu_to_le16(qpci_io_readw(dev,
> +                                                       ide_base + reg_data));
>          }
>      }
>
Alexey Kardashevskiy Oct. 26, 2016, 1:57 a.m. UTC | #4
On 25/10/16 23:25, David Gibson wrote:
> On Tue, Oct 25, 2016 at 06:01:41PM +1100, Alexey Kardashevskiy wrote:
>> On 24/10/16 15:59, David Gibson wrote:
>>> ide-test uses many explicit inb() / outb() operations for its IO, which
>>> means it's not portable to non-x86 platforms.  This cleans it up to use
>>> the libqos PCI accessors instead.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [snip]
> 
>>> -static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
>>> +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
>>> +                                 uint64_t lba, int nblocks)
>>>  {
>>>      Read10CDB pkt = { .padding = 0 };
>>>      int i;
>>> @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
>>>  
>>>      /* Send Packet */
>>>      for (i = 0; i < sizeof(Read10CDB)/2; i++) {
>>> -        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
>>> +        qpci_io_writew(dev, ide_base + reg_data,
>>> +                       le16_to_cpu(((uint16_t *)&pkt)[i]));
>>
>>
>> cpu_to_le16 -> le16_to_cpu conversion here and below (at the very end) is
>> not obvious. Right above this chunk the @pkt fields are initialized as BE:
>>
>>  /* Construct SCSI CDB packet */
>>  pkt.opcode = 0x28;
>>  pkt.lba = cpu_to_be32(lba);
>>  pkt.nblocks = cpu_to_be16(nblocks);
>>
>> outw() seems to be CPU-endian, and qpci_io_writew() as well, or not?
> 
> outw() is guest CPU endian (which is stupid, but that's another
> matter).  qpci_io_writew() is different - it is always LE, because PCI
> devices are always LE (well, ok, nearly always).
> 
> So, yes, this is a bit confusing.  Here's what's going on:
>   * the SCSI standard uses BE, so that's what we put into the
>     packet structure
>   * We need to transfer the packet to the device as a bytestream - so
>     no endianness conversions
>   * But.. we do so in 16-bit chunks
>   * .. and qpci_io_writew() is designed to take CPU values and write
>     them out as LE - ie, it contains an implicit cpu_to_le16()

dev->bus->pio_writew() calls outw() which calls qtest_outw() and
qtest_sendf() where @value is a text - where does this implicit
cpu_to_le16() happen? Or I am reading the code wrong?

The other branch (for MMIO) in qpci_io_writew() calls cpu_to_le16() explicitly.

I'd expect a function with a generic name as qpci_io_writew() to always
take data in the some known and always the same endianness (LE in this case
as it is PCI).

In the chunk above we convert host-CPU-endian @lba to BE then treat it as
LE when converting to CPU-endian and then expect qpci_io_writew() to do
swapping again (or not - depends on BAR type - IO vs. MMIO - or conversion
always happens?), this confuses me a lot. However, everybody else is happy
so am I :)
David Gibson Oct. 26, 2016, 4:11 a.m. UTC | #5
On Wed, Oct 26, 2016 at 12:57:26PM +1100, Alexey Kardashevskiy wrote:
> On 25/10/16 23:25, David Gibson wrote:
> > On Tue, Oct 25, 2016 at 06:01:41PM +1100, Alexey Kardashevskiy wrote:
> >> On 24/10/16 15:59, David Gibson wrote:
> >>> ide-test uses many explicit inb() / outb() operations for its IO, which
> >>> means it's not portable to non-x86 platforms.  This cleans it up to use
> >>> the libqos PCI accessors instead.
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > [snip]
> > 
> >>> -static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> >>> +static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
> >>> +                                 uint64_t lba, int nblocks)
> >>>  {
> >>>      Read10CDB pkt = { .padding = 0 };
> >>>      int i;
> >>> @@ -670,7 +717,8 @@ static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
> >>>  
> >>>      /* Send Packet */
> >>>      for (i = 0; i < sizeof(Read10CDB)/2; i++) {
> >>> -        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
> >>> +        qpci_io_writew(dev, ide_base + reg_data,
> >>> +                       le16_to_cpu(((uint16_t *)&pkt)[i]));
> >>
> >>
> >> cpu_to_le16 -> le16_to_cpu conversion here and below (at the very end) is
> >> not obvious. Right above this chunk the @pkt fields are initialized as BE:
> >>
> >>  /* Construct SCSI CDB packet */
> >>  pkt.opcode = 0x28;
> >>  pkt.lba = cpu_to_be32(lba);
> >>  pkt.nblocks = cpu_to_be16(nblocks);
> >>
> >> outw() seems to be CPU-endian, and qpci_io_writew() as well, or not?
> > 
> > outw() is guest CPU endian (which is stupid, but that's another
> > matter).  qpci_io_writew() is different - it is always LE, because PCI
> > devices are always LE (well, ok, nearly always).
> > 
> > So, yes, this is a bit confusing.  Here's what's going on:
> >   * the SCSI standard uses BE, so that's what we put into the
> >     packet structure
> >   * We need to transfer the packet to the device as a bytestream - so
> >     no endianness conversions
> >   * But.. we do so in 16-bit chunks
> >   * .. and qpci_io_writew() is designed to take CPU values and write
> >     them out as LE - ie, it contains an implicit cpu_to_le16()
> 
> dev->bus->pio_writew() calls outw() which calls qtest_outw() and
> qtest_sendf() where @value is a text - where does this implicit
> cpu_to_le16() happen? Or I am reading the code wrong?

You're looking at the PC specific backend, which knows that the target
endianness is LE, and so target_to_le16() is a NOP.  The translation
from hsot to guest endianness happens down inside the outw logic.
qtest.c calls outw, which calls stw_p, which is defined to do the swap
for the target endianness in include/exec/cpu-all.h

If you look at the spapr backend, you'll see that the PIO callbacks
have an unconditional byteswap in them.  The spapr backend is ppc
specific which is notionally BE, so it always needs a swap in order to
get LE writes.

> The other branch (for MMIO) in qpci_io_writew() calls cpu_to_le16() explicitly.
> 
> I'd expect a function with a generic name as qpci_io_writew() to always
> take data in the some known and always the same endianness (LE in this case
> as it is PCI).

It does.  It's just the means to accomplishing that is a bit
convoluted for the PIO case.  That's exactly why I think the base
in/out operations should also be fixed endianness, rather than guest
endian, but that's an argument I'm having elsewhere.

> In the chunk above we convert host-CPU-endian @lba to BE then treat it as
> LE when converting to CPU-endian and then expect qpci_io_writew() to do
> swapping again (or not - depends on BAR type - IO vs. MMIO - or conversion
> always happens?), this confuses me a lot. However, everybody else is happy
> so am I :)

You need to think of this in two different parts.  Building the buffer
as a bytestream, which includes BE components.  Then sending the
buffer to the hardware as a bytestream.  This has balanced le<->cpu
conversions in order to preserve bytestream order.

Remember than endian is a property of a value - something that has a
specific length and location, not of a bytestream or bus of itself.
The fields in the request are BE, hence the BE conversions.  The data
*register* which we write stuff out to is treated as LE, hence the LE
conversions.
diff mbox

Patch

diff --git a/tests/ide-test.c b/tests/ide-test.c
index a8a4081..86c4373 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(void **bmdma_base, void **ide_base)
 {
     QPCIDevice *dev;
     uint16_t vendor_id, device_id;
@@ -156,7 +156,9 @@  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_base = qpci_iomap(dev, 4, NULL);
+
+    *ide_base = qpci_legacy_iomap(dev, IDE_BASE);
 
     qpci_device_enable(dev);
 
@@ -179,17 +181,19 @@  typedef struct PrdtEntry {
 
 static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
                             PrdtEntry *prdt, int prdt_entries,
-                            void(*post_exec)(uint64_t sector, int nb_sectors))
+                            void(*post_exec)(QPCIDevice *dev, void *ide_base,
+                                             uint64_t sector, int nb_sectors))
 {
     QPCIDevice *dev;
-    uint16_t bmdma_base;
+    void *bmdma_base;
+    void *ide_base;
     uintptr_t guest_prdt;
     size_t len;
     bool from_dev;
     uint8_t status;
     int flags;
 
-    dev = get_pci_device(&bmdma_base);
+    dev = get_pci_device(&bmdma_base, &ide_base);
 
     flags = cmd & ~0xff;
     cmd &= 0xff;
@@ -214,59 +218,60 @@  static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
     }
 
     /* Select device 0 */
-    outb(IDE_BASE + reg_device, 0 | LBA);
+    qpci_io_writeb(dev, ide_base + reg_device, 0 | LBA);
 
     /* Stop any running transfer, clear any pending interrupt */
-    outb(bmdma_base + bmreg_cmd, 0);
-    outb(bmdma_base + bmreg_status, BM_STS_INTR);
+    qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
+    qpci_io_writeb(dev, bmdma_base + bmreg_status, BM_STS_INTR);
 
     /* Setup PRDT */
     len = sizeof(*prdt) * prdt_entries;
     guest_prdt = guest_alloc(guest_malloc, len);
     memwrite(guest_prdt, prdt, len);
-    outl(bmdma_base + bmreg_prdt, guest_prdt);
+    qpci_io_writel(dev, bmdma_base + bmreg_prdt, guest_prdt);
 
     /* ATA DMA command */
     if (cmd == CMD_PACKET) {
         /* Enables ATAPI DMA; otherwise PIO is attempted */
-        outb(IDE_BASE + reg_feature, 0x01);
+        qpci_io_writeb(dev, ide_base + reg_feature, 0x01);
     } else {
-        outb(IDE_BASE + reg_nsectors, nb_sectors);
-        outb(IDE_BASE + reg_lba_low,    sector & 0xff);
-        outb(IDE_BASE + reg_lba_middle, (sector >> 8) & 0xff);
-        outb(IDE_BASE + reg_lba_high,   (sector >> 16) & 0xff);
+        qpci_io_writeb(dev, ide_base + reg_nsectors, nb_sectors);
+        qpci_io_writeb(dev, ide_base + reg_lba_low,    sector & 0xff);
+        qpci_io_writeb(dev, ide_base + reg_lba_middle, (sector >> 8) & 0xff);
+        qpci_io_writeb(dev, ide_base + reg_lba_high,   (sector >> 16) & 0xff);
     }
 
-    outb(IDE_BASE + reg_command, cmd);
+    qpci_io_writeb(dev, ide_base + reg_command, cmd);
 
     if (post_exec) {
-        post_exec(sector, nb_sectors);
+        post_exec(dev, ide_base, sector, nb_sectors);
     }
 
     /* Start DMA transfer */
-    outb(bmdma_base + bmreg_cmd, BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
+    qpci_io_writeb(dev, bmdma_base + bmreg_cmd,
+                   BM_CMD_START | (from_dev ? BM_CMD_WRITE : 0));
 
     if (flags & CMDF_ABORT) {
-        outb(bmdma_base + bmreg_cmd, 0);
+        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
     }
 
     /* Wait for the DMA transfer to complete */
     do {
-        status = inb(bmdma_base + bmreg_status);
+        status = qpci_io_readb(dev, bmdma_base + bmreg_status);
     } while ((status & (BM_STS_ACTIVE | BM_STS_INTR)) == BM_STS_ACTIVE);
 
     g_assert_cmpint(get_irq(IDE_PRIMARY_IRQ), ==, !!(status & BM_STS_INTR));
 
     /* Check IDE status code */
-    assert_bit_set(inb(IDE_BASE + reg_status), DRDY);
-    assert_bit_clear(inb(IDE_BASE + reg_status), BSY | DRQ);
+    assert_bit_set(qpci_io_readb(dev, ide_base + reg_status), DRDY);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), BSY | DRQ);
 
     /* Reading the status register clears the IRQ */
     g_assert(!get_irq(IDE_PRIMARY_IRQ));
 
     /* Stop DMA transfer if still active */
     if (status & BM_STS_ACTIVE) {
-        outb(bmdma_base + bmreg_cmd, 0);
+        qpci_io_writeb(dev, bmdma_base + bmreg_cmd, 0);
     }
 
     free_pci_device(dev);
@@ -276,6 +281,8 @@  static int send_dma_request(int cmd, uint64_t sector, int nb_sectors,
 
 static void test_bmdma_simple_rw(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
     uint8_t *buf;
     uint8_t *cmpbuf;
@@ -289,6 +296,8 @@  static void test_bmdma_simple_rw(void)
         },
     };
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     buf = g_malloc(len);
     cmpbuf = g_malloc(len);
 
@@ -299,7 +308,7 @@  static void test_bmdma_simple_rw(void)
     status = send_dma_request(CMD_WRITE_DMA, 0, 1, prdt,
                               ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     /* Write 0xaa pattern to sector 1 */
     memset(buf, 0xaa, len);
@@ -308,14 +317,14 @@  static void test_bmdma_simple_rw(void)
     status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
                               ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     /* Read and verify 0x55 pattern in sector 0 */
     memset(cmpbuf, 0x55, len);
 
     status = send_dma_request(CMD_READ_DMA, 0, 1, prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     memread(guest_buf, buf, len);
     g_assert(memcmp(buf, cmpbuf, len) == 0);
@@ -325,7 +334,7 @@  static void test_bmdma_simple_rw(void)
 
     status = send_dma_request(CMD_READ_DMA, 1, 1, prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     memread(guest_buf, buf, len);
     g_assert(memcmp(buf, cmpbuf, len) == 0);
@@ -337,6 +346,8 @@  static void test_bmdma_simple_rw(void)
 
 static void test_bmdma_short_prdt(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
 
     PrdtEntry prdt[] = {
@@ -346,21 +357,25 @@  static void test_bmdma_short_prdt(void)
         },
     };
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 1,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 }
 
 static void test_bmdma_one_sector_short_prdt(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
 
     /* Read 2 sectors but only give 1 sector in PRDT */
@@ -371,21 +386,25 @@  static void test_bmdma_one_sector_short_prdt(void)
         },
     };
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 2,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, 0);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 }
 
 static void test_bmdma_long_prdt(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
 
     PrdtEntry prdt[] = {
@@ -395,23 +414,29 @@  static void test_bmdma_long_prdt(void)
         },
     };
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* Normal request */
     status = send_dma_request(CMD_READ_DMA, 0, 1,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     /* Abort the request before it completes */
     status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 1,
                               prdt, ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 }
 
 static void test_bmdma_no_busmaster(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* No PRDT_EOT, each entry addr 0/size 64k, and in theory qemu shouldn't be
      * able to access it anyway because the Bus Master bit in the PCI command
      * register isn't set. This is complete nonsense, but it used to be pretty
@@ -424,7 +449,7 @@  static void test_bmdma_no_busmaster(void)
     /* Not entirely clear what the expected result is, but this is what we get
      * in practice. At least we want to be aware of any changes. */
     g_assert_cmphex(status, ==, BM_STS_ACTIVE | BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 }
 
 static void test_bmdma_setup(void)
@@ -454,6 +479,8 @@  static void string_cpu_to_be16(uint16_t *s, size_t bytes)
 
 static void test_identify(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t data;
     uint16_t buf[256];
     int i;
@@ -464,23 +491,25 @@  static void test_identify(void)
         "-global ide-hd.ver=%s",
         tmp_path, "testdisk", "version");
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* IDENTIFY command on device 0*/
-    outb(IDE_BASE + reg_device, 0);
-    outb(IDE_BASE + reg_command, CMD_IDENTIFY);
+    qpci_io_writeb(dev, ide_base + reg_device, 0);
+    qpci_io_writeb(dev, ide_base + reg_command, CMD_IDENTIFY);
 
     /* Read in the IDENTIFY buffer and check registers */
-    data = inb(IDE_BASE + reg_device);
+    data = qpci_io_readb(dev, ide_base + reg_device);
     g_assert_cmpint(data & DEV, ==, 0);
 
     for (i = 0; i < 256; i++) {
-        data = inb(IDE_BASE + reg_status);
+        data = qpci_io_readb(dev, ide_base + reg_status);
         assert_bit_set(data, DRDY | DRQ);
         assert_bit_clear(data, BSY | DF | ERR);
 
-        ((uint16_t*) buf)[i] = inw(IDE_BASE + reg_data);
+        buf[i] = qpci_io_readw(dev, ide_base + reg_data);
     }
 
-    data = inb(IDE_BASE + reg_status);
+    data = qpci_io_readb(dev, ide_base + reg_status);
     assert_bit_set(data, DRDY);
     assert_bit_clear(data, BSY | DF | ERR | DRQ);
 
@@ -505,11 +534,15 @@  static void test_identify(void)
  */
 static void make_dirty(uint8_t device)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t status;
     size_t len = 512;
     uintptr_t guest_buf;
     void* buf;
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     guest_buf = guest_alloc(guest_malloc, len);
     buf = g_malloc(len);
     g_assert(guest_buf);
@@ -527,19 +560,23 @@  static void make_dirty(uint8_t device)
     status = send_dma_request(CMD_WRITE_DMA, 1, 1, prdt,
                               ARRAY_SIZE(prdt), NULL);
     g_assert_cmphex(status, ==, BM_STS_INTR);
-    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+    assert_bit_clear(qpci_io_readb(dev, ide_base + reg_status), DF | ERR);
 
     g_free(buf);
 }
 
 static void test_flush(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t data;
 
     ide_test_start(
         "-drive file=blkdebug::%s,if=ide,cache=writeback,format=raw",
         tmp_path);
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     qtest_irq_intercept_in(global_qtest, "ioapic");
 
     /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
@@ -549,11 +586,11 @@  static void test_flush(void)
     g_free(hmp("qemu-io ide0-hd0 \"break flush_to_os A\""));
 
     /* FLUSH CACHE command on device 0*/
-    outb(IDE_BASE + reg_device, 0);
-    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+    qpci_io_writeb(dev, ide_base + reg_device, 0);
+    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
 
     /* Check status while request is in flight*/
-    data = inb(IDE_BASE + reg_status);
+    data = qpci_io_readb(dev, ide_base + reg_status);
     assert_bit_set(data, BSY | DRDY);
     assert_bit_clear(data, DF | ERR | DRQ);
 
@@ -561,11 +598,11 @@  static void test_flush(void)
     g_free(hmp("qemu-io ide0-hd0 \"resume A\""));
 
     /* Check registers */
-    data = inb(IDE_BASE + reg_device);
+    data = qpci_io_readb(dev, ide_base + reg_device);
     g_assert_cmpint(data & DEV, ==, 0);
 
     do {
-        data = inb(IDE_BASE + reg_status);
+        data = qpci_io_readb(dev, ide_base + reg_status);
     } while (data & BSY);
 
     assert_bit_set(data, DRDY);
@@ -576,6 +613,8 @@  static void test_flush(void)
 
 static void test_retry_flush(const char *machine)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t data;
     const char *s;
 
@@ -587,17 +626,19 @@  static void test_retry_flush(const char *machine)
         "rerror=stop,werror=stop",
         debug_path, tmp_path);
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     qtest_irq_intercept_in(global_qtest, "ioapic");
 
     /* Dirty media so that CMD_FLUSH_CACHE will actually go to disk */
     make_dirty(0);
 
     /* FLUSH CACHE command on device 0*/
-    outb(IDE_BASE + reg_device, 0);
-    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+    qpci_io_writeb(dev, ide_base + reg_device, 0);
+    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
 
     /* Check status while request is in flight*/
-    data = inb(IDE_BASE + reg_status);
+    data = qpci_io_readb(dev, ide_base + reg_status);
     assert_bit_set(data, BSY | DRDY);
     assert_bit_clear(data, DF | ERR | DRQ);
 
@@ -608,11 +649,11 @@  static void test_retry_flush(const char *machine)
     qmp_discard_response(s);
 
     /* Check registers */
-    data = inb(IDE_BASE + reg_device);
+    data = qpci_io_readb(dev, ide_base + reg_device);
     g_assert_cmpint(data & DEV, ==, 0);
 
     do {
-        data = inb(IDE_BASE + reg_status);
+        data = qpci_io_readb(dev, ide_base + reg_status);
     } while (data & BSY);
 
     assert_bit_set(data, DRDY);
@@ -623,11 +664,16 @@  static void test_retry_flush(const char *machine)
 
 static void test_flush_nodev(void)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
+
     ide_test_start("");
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* FLUSH CACHE command on device 0*/
-    outb(IDE_BASE + reg_device, 0);
-    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+    qpci_io_writeb(dev, ide_base + reg_device, 0);
+    qpci_io_writeb(dev, ide_base + reg_command, CMD_FLUSH_CACHE);
 
     /* Just testing that qemu doesn't crash... */
 
@@ -654,7 +700,8 @@  typedef struct Read10CDB {
     uint16_t padding;
 } __attribute__((__packed__)) Read10CDB;
 
-static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
+static void send_scsi_cdb_read10(QPCIDevice *dev, void *ide_base,
+                                 uint64_t lba, int nblocks)
 {
     Read10CDB pkt = { .padding = 0 };
     int i;
@@ -670,7 +717,8 @@  static void send_scsi_cdb_read10(uint64_t lba, int nblocks)
 
     /* Send Packet */
     for (i = 0; i < sizeof(Read10CDB)/2; i++) {
-        outw(IDE_BASE + reg_data, cpu_to_le16(((uint16_t *)&pkt)[i]));
+        qpci_io_writew(dev, ide_base + reg_data,
+                       le16_to_cpu(((uint16_t *)&pkt)[i]));
     }
 }
 
@@ -683,13 +731,17 @@  static void nsleep(int64_t nsecs)
 
 static uint8_t ide_wait_clear(uint8_t flag)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     uint8_t data;
     time_t st;
 
+    dev = get_pci_device(&bmdma_base, &ide_base);
+
     /* Wait with a 5 second timeout */
     time(&st);
     while (true) {
-        data = inb(IDE_BASE + reg_status);
+        data = qpci_io_readb(dev, ide_base + reg_status);
         if (!(data & flag)) {
             return data;
         }
@@ -723,6 +775,8 @@  static void ide_wait_intr(int irq)
 
 static void cdrom_pio_impl(int nblocks)
 {
+    QPCIDevice *dev;
+    void *bmdma_base, *ide_base;
     FILE *fh;
     int patt_blocks = MAX(16, nblocks);
     size_t patt_len = ATAPI_BLOCK_SIZE * patt_blocks;
@@ -741,13 +795,15 @@  static void cdrom_pio_impl(int nblocks)
 
     ide_test_start("-drive if=none,file=%s,media=cdrom,format=raw,id=sr0,index=0 "
                    "-device ide-cd,drive=sr0,bus=ide.0", tmp_path);
+    dev = get_pci_device(&bmdma_base, &ide_base);
     qtest_irq_intercept_in(global_qtest, "ioapic");
 
     /* PACKET command on device 0 */
-    outb(IDE_BASE + reg_device, 0);
-    outb(IDE_BASE + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
-    outb(IDE_BASE + reg_lba_high, (BYTE_COUNT_LIMIT >> 8 & 0xFF));
-    outb(IDE_BASE + reg_command, CMD_PACKET);
+    qpci_io_writeb(dev, ide_base + reg_device, 0);
+    qpci_io_writeb(dev, ide_base + reg_lba_middle, BYTE_COUNT_LIMIT & 0xFF);
+    qpci_io_writeb(dev, ide_base + reg_lba_high,
+                   (BYTE_COUNT_LIMIT >> 8 & 0xFF));
+    qpci_io_writeb(dev, ide_base + reg_command, CMD_PACKET);
     /* HP0: Check_Status_A State */
     nsleep(400);
     data = ide_wait_clear(BSY);
@@ -756,7 +812,7 @@  static void cdrom_pio_impl(int nblocks)
     assert_bit_clear(data, ERR | DF | BSY);
 
     /* SCSI CDB (READ10) -- read n*2048 bytes from block 0 */
-    send_scsi_cdb_read10(0, nblocks);
+    send_scsi_cdb_read10(dev, ide_base, 0, nblocks);
 
     /* Read data back: occurs in bursts of 'BYTE_COUNT_LIMIT' bytes.
      * If BYTE_COUNT_LIMIT is odd, we transfer BYTE_COUNT_LIMIT - 1 bytes.
@@ -780,7 +836,8 @@  static void cdrom_pio_impl(int nblocks)
 
         /* HP4: Transfer_Data */
         for (j = 0; j < MIN((limit / 2), rem); j++) {
-            rx[offset + j] = le16_to_cpu(inw(IDE_BASE + reg_data));
+            rx[offset + j] = cpu_to_le16(qpci_io_readw(dev,
+                                                       ide_base + reg_data));
         }
     }