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

[David Gibson]

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 <address@hidden>
[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));
> >          }
> >      }
> >  
> > 
> 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature