[PATCH 0/3] Run pvscsi driver entirely in 32bit mode.

[Kevin O'Connor]

As with the recent change to AHCI, if the pvscsi driver needs to jump
into 32bit mode to access a register, then it's better to run the
pvscsi driver entirely in 32bit mode.

Evgeny, can you take a look at this and give it a test?  Also, what
command line do you use for testing?

I tried testing locally by adding:

-device pvscsi,id=pvscsi0 -device scsi-disk,bus=pvscsi0.0,drive=drive0 -drive
id=drive0,if=none,file=dos-drivec-new

to my qemu command line (both qemu v1.6 and qemu v1.7), but it doesn't
work for me even before my changes.  It hangs in
pvscsi_wait_intr_cmpl().

-Kevin


Kevin O'Connor (3):
  pvscsi: Don't store reference to struct pci_device.
  pvscsi: Always run entirely in 32bit mode.
  pvscsi: Remove use of LOWFLAT and GLOBALFLAT macros.

 Makefile          |   4 +--
 src/block.c       |   8 +++--
 src/hw/blockcmd.c |   3 +-
 src/hw/pvscsi.c   | 106 +++++++++++++++++++++++++-----------------------------
 4 files changed, 58 insertions(+), 63 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] pvscsi: Don't store reference to struct pci_device.

[Kevin O'Connor]

The pci_device reference isn't used by pvscsi, and it's confusing to
keep a long held reference to a short lived object.

Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net&gt;
---
 src/hw/pvscsi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c
index 6911230..00ce377 100644
--- a/src/hw/pvscsi.c
+++ b/src/hw/pvscsi.c
@@ -129,7 +129,6 @@ struct pvscsi_ring_dsc_s {
 
 struct pvscsi_lun_s {
     struct drive_s drive;
-    struct pci_device *pci;
     u32 iobase;
     u8 target;
     u8 lun;
@@ -291,7 +290,6 @@ pvscsi_add_lun(struct pci_device *pci, u32 iobase,
     memset(plun, 0, sizeof(*plun));
     plun->drive.type = DTYPE_PVSCSI;
     plun->drive.cntl_id = pci->bdf;
-    plun->pci = pci;
     plun->target = target;
     plun->lun = lun;
     plun->iobase = iobase;
-- 
1.8.3.1

[PATCH 2/3] pvscsi: Always run entirely in 32bit mode.

[Kevin O'Connor]

Instead of jumping into 32bit mode to access the PCI config space, run
the entire driver in 32bit mode.

Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net&gt;
---
 Makefile          |  4 ++--
 src/block.c       |  8 ++++++--
 src/hw/blockcmd.c |  3 ++-
 src/hw/pvscsi.c   | 30 +++++++++++++++---------------
 4 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/Makefile b/Makefile
index aba8a62..414c90a 100644
--- a/Makefile
+++ b/Makefile
@@ -34,11 +34,11 @@ SRCBOTH=misc.c stacks.c output.c string.c x86.c block.c cdrom.c
mouse.c kbd.c \
     hw/usb-hid.c hw/usb-msc.c hw/usb-uas.c \
     hw/blockcmd.c hw/floppy.c hw/ata.c hw/ramdisk.c \
     hw/virtio-ring.c hw/virtio-pci.c hw/virtio-blk.c hw/virtio-scsi.c \
-    hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c hw/pvscsi.c
+    hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c
 SRC16=$(SRCBOTH) system.c disk.c font.c
 SRC32FLAT=$(SRCBOTH) post.c memmap.c malloc.c pmm.c romfile.c optionroms.c \
     boot.c bootsplash.c jpeg.c bmp.c \
-    hw/ahci.c hw/usb-hub.c \
+    hw/ahci.c hw/pvscsi.c hw/usb-hub.c \
     fw/coreboot.c fw/lzmadecode.c fw/csm.c fw/biostables.c \
     fw/paravirt.c fw/shadow.c fw/pciinit.c fw/smm.c fw/mtrr.c fw/xen.c \
     fw/acpi.c fw/mptable.c fw/pirtable.c fw/smbios.c fw/romfile_loader.c
diff --git a/src/block.c b/src/block.c
index ac2a830..898c279 100644
--- a/src/block.c
+++ b/src/block.c
@@ -313,7 +313,7 @@ __disk_ret_unimplemented(struct bregs *regs, u32 linecode, const char
*fname)
  * 16bit calling interface
  ****************************************************************/
 
-static int
+int VISIBLE32FLAT
 process_scsi_op(struct disk_op_s *op)
 {
     switch (op->command) {
@@ -386,9 +386,13 @@ process_op(struct disk_op_s *op)
     case DTYPE_LSI_SCSI:
     case DTYPE_ESP_SCSI:
     case DTYPE_MEGASAS:
-    case DTYPE_PVSCSI:
         ret = process_scsi_op(op);
         break;
+    case DTYPE_PVSCSI: ;
+        extern void _cfunc32flat_process_scsi_op(void);
+        ret = call32(_cfunc32flat_process_scsi_op
+                     , (u32)MAKE_FLATPTR(GET_SEG(SS), op), DISK_RET_EPARAM);
+        break;
     default:
         ret = DISK_RET_EPARAM;
         break;
diff --git a/src/hw/blockcmd.c b/src/hw/blockcmd.c
index 97c6675..db61cbd 100644
--- a/src/hw/blockcmd.c
+++ b/src/hw/blockcmd.c
@@ -44,7 +44,8 @@ cdb_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
     case DTYPE_MEGASAS:
         return megasas_cmd_data(op, cdbcmd, blocksize);
     case DTYPE_PVSCSI:
-        return pvscsi_cmd_data(op, cdbcmd, blocksize);
+        if (!MODESEGMENT)
+            return pvscsi_cmd_data(op, cdbcmd, blocksize);
     case DTYPE_AHCI_ATAPI:
         if (!MODESEGMENT)
             return ahci_cmd_data(op, cdbcmd, blocksize);
diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c
index 00ce377..8b4b10f 100644
--- a/src/hw/pvscsi.c
+++ b/src/hw/pvscsi.c
@@ -129,41 +129,41 @@ struct pvscsi_ring_dsc_s {
 
 struct pvscsi_lun_s {
     struct drive_s drive;
-    u32 iobase;
+    void *iobase;
     u8 target;
     u8 lun;
     struct pvscsi_ring_dsc_s *ring_dsc;
 };
 
 static void
-pvscsi_write_cmd_desc(u32 iobase, u32 cmd, const void *desc, size_t len)
+pvscsi_write_cmd_desc(void *iobase, u32 cmd, const void *desc, size_t len)
 {
     const u32 *ptr = desc;
     size_t i;
 
     len /= sizeof(*ptr);
-    pci_writel(iobase + PVSCSI_REG_OFFSET_COMMAND, cmd);
+    writel(iobase + PVSCSI_REG_OFFSET_COMMAND, cmd);
     for (i = 0; i < len; i++)
-        pci_writel(iobase + PVSCSI_REG_OFFSET_COMMAND_DATA, ptr[i]);
+        writel(iobase + PVSCSI_REG_OFFSET_COMMAND_DATA, ptr[i]);
 }
 
 static void
-pvscsi_kick_rw_io(u32 iobase)
+pvscsi_kick_rw_io(void *iobase)
 {
-    pci_writel(iobase + PVSCSI_REG_OFFSET_KICK_RW_IO, 0);
+    writel(iobase + PVSCSI_REG_OFFSET_KICK_RW_IO, 0);
 }
 
 static void
-pvscsi_wait_intr_cmpl(u32 iobase)
+pvscsi_wait_intr_cmpl(void *iobase)
 {
-    while (!(pci_readl(iobase + PVSCSI_REG_OFFSET_INTR_STATUS) &
PVSCSI_INTR_CMPL_MASK))
+    while (!(readl(iobase + PVSCSI_REG_OFFSET_INTR_STATUS) & PVSCSI_INTR_CMPL_MASK))
         usleep(5);
-    pci_writel(iobase + PVSCSI_REG_OFFSET_INTR_STATUS, PVSCSI_INTR_CMPL_MASK);
+    writel(iobase + PVSCSI_REG_OFFSET_INTR_STATUS, PVSCSI_INTR_CMPL_MASK);
 
 }
 
 static void
-pvscsi_init_rings(u32 iobase, struct pvscsi_ring_dsc_s **ring_dsc)
+pvscsi_init_rings(void *iobase, struct pvscsi_ring_dsc_s **ring_dsc)
 {
     struct PVSCSICmdDescSetupRings cmd = {0,};
 
@@ -279,7 +279,7 @@ pvscsi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
 }
 
 static int
-pvscsi_add_lun(struct pci_device *pci, u32 iobase,
+pvscsi_add_lun(struct pci_device *pci, void *iobase,
                struct pvscsi_ring_dsc_s *ring_dsc, u8 target, u8 lun)
 {
     struct pvscsi_lun_s *plun = malloc_fseg(sizeof(*plun));
@@ -311,7 +311,7 @@ fail:
 }
 
 static void
-pvscsi_scan_target(struct pci_device *pci, u32 iobase,
+pvscsi_scan_target(struct pci_device *pci, void *iobase,
                    struct pvscsi_ring_dsc_s *ring_dsc, u8 target)
 {
     /* TODO: send REPORT LUNS.  For now, only LUN 0 is recognized.  */
@@ -324,12 +324,12 @@ init_pvscsi(struct pci_device *pci)
     struct pvscsi_ring_dsc_s *ring_dsc = NULL;
     int i;
     u16 bdf = pci->bdf;
-    u32 iobase = pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0)
-        & PCI_BASE_ADDRESS_MEM_MASK;
+    void *iobase = (void*)(pci_config_readl(pci->bdf, PCI_BASE_ADDRESS_0)
+                           & PCI_BASE_ADDRESS_MEM_MASK);
 
     pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_MASTER);
 
-    dprintf(1, "found pvscsi at %02x:%02x.%x, io @ %x\n",
+    dprintf(1, "found pvscsi at %02x:%02x.%x, io @ %p\n",
             pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf),
             pci_bdf_to_fn(bdf), iobase);
 
-- 
1.8.3.1

[PATCH 3/3] pvscsi: Remove use of LOWFLAT and GLOBALFLAT macros.

[Kevin O'Connor]

Now that pvscsi runs entirely in 32bit mode, there is no need to use
the memory segment access macros.

This also fixes up an incorrect memcpy and memset call.

Signed-off-by: Kevin O'Connor <kevin(a)koconnor.net&gt;
---
 src/hw/pvscsi.c | 74 +++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 41 deletions(-)

diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c
index 8b4b10f..601a551 100644
--- a/src/hw/pvscsi.c
+++ b/src/hw/pvscsi.c
@@ -7,7 +7,6 @@
 //
 // This file may be distributed under the terms of the GNU LGPLv3 license.
 
-#include "biosvar.h" // GET_GLOBALFLAT
 #include "block.h" // struct drive_s
 #include "blockcmd.h" // scsi_drive_setup
 #include "config.h" // CONFIG_*
@@ -16,11 +15,12 @@
 #include "pci.h" // foreachpci
 #include "pci_ids.h" // PCI_DEVICE_ID_VMWARE_PVSCSI
 #include "pci_regs.h" // PCI_VENDOR_ID
+#include "pvscsi.h" // pvscsi_setup
 #include "std/disk.h" // DISK_RET_SUCCESS
 #include "string.h" // memset
 #include "util.h" // usleep
-#include "pvscsi.h"
 #include "virtio-ring.h" // PAGE_SHIFT, virt_to_phys
+#include "x86.h" // writel
 
 #define MASK(n) ((1 << (n)) - 1)
 
@@ -159,7 +159,6 @@ pvscsi_wait_intr_cmpl(void *iobase)
     while (!(readl(iobase + PVSCSI_REG_OFFSET_INTR_STATUS) & PVSCSI_INTR_CMPL_MASK))
         usleep(5);
     writel(iobase + PVSCSI_REG_OFFSET_INTR_STATUS, PVSCSI_INTR_CMPL_MASK);
-
 }
 
 static void
@@ -203,60 +202,58 @@ static void pvscsi_fill_req(struct PVSCSIRingsState *s,
                             u16 target, u16 lun, void *cdbcmd, u16 blocksize,
                             struct disk_op_s *op)
 {
-    SET_LOWFLAT(req->bus, 0);
-    SET_LOWFLAT(req->target, target);
-    memset(LOWFLAT2LOW(&req->lun[0]), 0, sizeof(req->lun));
-    SET_LOWFLAT(req->lun[1], lun);
-    SET_LOWFLAT(req->senseLen, 0);
-    SET_LOWFLAT(req->senseAddr, 0);
-    SET_LOWFLAT(req->cdbLen, 16);
-    SET_LOWFLAT(req->vcpuHint, 0);
-    memcpy(LOWFLAT2LOW(&req->cdb[0]), cdbcmd, 16);
-    SET_LOWFLAT(req->tag, SIMPLE_QUEUE_TAG);
-    SET_LOWFLAT(req->flags,
-                cdb_is_read(cdbcmd, blocksize) ?
-                PVSCSI_FLAG_CMD_DIR_TOHOST : PVSCSI_FLAG_CMD_DIR_TODEVICE);
-
-    SET_LOWFLAT(req->dataLen, op->count * blocksize);
-    SET_LOWFLAT(req->dataAddr, (u32)op->buf_fl);
-    SET_LOWFLAT(s->reqProdIdx, GET_LOWFLAT(s->reqProdIdx) + 1);
-
+    req->bus = 0;
+    req->target = target;
+    memset(req->lun, 0, sizeof(req->lun));
+    req->lun[1] = lun;
+    req->senseLen = 0;
+    req->senseAddr = 0;
+    req->cdbLen = 16;
+    req->vcpuHint = 0;
+    memcpy(req->cdb, cdbcmd, 16);
+    req->tag = SIMPLE_QUEUE_TAG;
+    req->flags = cdb_is_read(cdbcmd, blocksize) ?
+        PVSCSI_FLAG_CMD_DIR_TOHOST : PVSCSI_FLAG_CMD_DIR_TODEVICE;
+
+    req->dataLen = op->count * blocksize;
+    req->dataAddr = (u32)op->buf_fl;
+    s->reqProdIdx = s->reqProdIdx + 1;
 }
 
 static u32
 pvscsi_get_rsp(struct PVSCSIRingsState *s,
                struct PVSCSIRingCmpDesc *rsp)
 {
-    u32 status = GET_LOWFLAT(rsp->hostStatus);
-    SET_LOWFLAT(s->cmpConsIdx, GET_LOWFLAT(s->cmpConsIdx)+1);
+    u32 status = rsp->hostStatus;
+    s->cmpConsIdx = s->cmpConsIdx + 1;
     return status;
 }
 
 static int
-pvscsi_cmd(struct pvscsi_lun_s *plun_gf, struct disk_op_s *op,
+pvscsi_cmd(struct pvscsi_lun_s *plun, struct disk_op_s *op,
            void *cdbcmd, u16 target, u16 lun, u16 blocksize)
 {
-    struct pvscsi_ring_dsc_s *ring_dsc = GET_GLOBALFLAT(plun_gf->ring_dsc);
-    struct PVSCSIRingsState *s = GET_LOWFLAT(ring_dsc->ring_state);
-    u32 req_entries = GET_LOWFLAT(s->reqNumEntriesLog2);
-    u32 cmp_entries = GET_LOWFLAT(s->cmpNumEntriesLog2);
+    struct pvscsi_ring_dsc_s *ring_dsc = plun->ring_dsc;
+    struct PVSCSIRingsState *s = ring_dsc->ring_state;
+    u32 req_entries = s->reqNumEntriesLog2;
+    u32 cmp_entries = s->cmpNumEntriesLog2;
     struct PVSCSIRingReqDesc *req;
     struct PVSCSIRingCmpDesc *rsp;
     u32 status;
 
-    if (GET_LOWFLAT(s->reqProdIdx) - GET_LOWFLAT(s->cmpConsIdx) >= 1 <<
req_entries) {
+    if (s->reqProdIdx - s->cmpConsIdx >= 1 << req_entries) {
         dprintf(1, "pvscsi: ring full: reqProdIdx=%d cmpConsIdx=%d\n",
-                GET_LOWFLAT(s->reqProdIdx), GET_LOWFLAT(s->cmpConsIdx));
+                s->reqProdIdx, s->cmpConsIdx);
         return DISK_RET_EBADTRACK;
     }
 
-    req = GET_LOWFLAT(ring_dsc->ring_reqs) + (GET_LOWFLAT(s->reqProdIdx) &
MASK(req_entries));
+    req = ring_dsc->ring_reqs + (s->reqProdIdx & MASK(req_entries));
     pvscsi_fill_req(s, req, target, lun, cdbcmd, blocksize, op);
 
-    pvscsi_kick_rw_io(GET_GLOBALFLAT(plun_gf->iobase));
-    pvscsi_wait_intr_cmpl(GET_GLOBALFLAT(plun_gf->iobase));
+    pvscsi_kick_rw_io(plun->iobase);
+    pvscsi_wait_intr_cmpl(plun->iobase);
 
-    rsp = GET_LOWFLAT(ring_dsc->ring_cmps) + (GET_LOWFLAT(s->cmpConsIdx) &
MASK(cmp_entries));
+    rsp = ring_dsc->ring_cmps + (s->cmpConsIdx & MASK(cmp_entries));
     status = pvscsi_get_rsp(s, rsp);
 
     return status == 0 ? DISK_RET_SUCCESS : DISK_RET_EBADTRACK;
@@ -268,14 +265,10 @@ pvscsi_cmd_data(struct disk_op_s *op, void *cdbcmd, u16 blocksize)
     if (!CONFIG_PVSCSI)
         return DISK_RET_EBADTRACK;
 
-    struct pvscsi_lun_s *plun_gf =
+    struct pvscsi_lun_s *plun =
         container_of(op->drive_gf, struct pvscsi_lun_s, drive);
 
-    return pvscsi_cmd(plun_gf, op, cdbcmd,
-                      GET_GLOBALFLAT(plun_gf->target),
-                      GET_GLOBALFLAT(plun_gf->lun),
-                      blocksize);
-
+    return pvscsi_cmd(plun, op, cdbcmd, plun->target, plun->lun, blocksize);
 }
 
 static int
@@ -340,7 +333,6 @@ init_pvscsi(struct pci_device *pci)
         pvscsi_scan_target(pci, iobase, ring_dsc, i);
 
     return;
-
 }
 
 void
-- 
1.8.3.1

[Don Koch]

On Fri, 27 Dec 2013 12:17:42 -0500
Kevin O'Connor <kevin(a)koconnor.net&gt; wrote:

 
 
 As with the recent change to AHCI, if the pvscsi driver needs to jump
 into 32bit mode to access a register, then it's better to run the
 pvscsi driver entirely in 32bit mode.
 
 Evgeny, can you take a look at this and give it a test?  Also, what
 command line do you use for testing?
 
 I tried testing locally by adding:
 
 -device pvscsi,id=pvscsi0 -device scsi-disk,bus=pvscsi0.0,drive=drive0 -drive
id=drive0,if=none,file=dos-drivec-new
 
 to my qemu command line (both qemu v1.6 and qemu v1.7), but it doesn't
 work for me even before my changes.  It hangs in
 pvscsi_wait_intr_cmpl().
Try the following patch to qemu. I was getting hangs in this routine due to
language definition/compiler/bogus input issues. The old one returned an
off-by-one value.

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index e35bff7..cfd7247 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -119,11 +119,8 @@ typedef struct PVSCSIRequest {
 static int
 pvscsi_log2(uint32_t input)
 {
-    int log = 0;
     assert(input > 0);
-    while (input >> ++log) {
-    }
-    return log;
+    return 31 - clz32(input);
 }
 
 static void

If this works, I'll pass it on to the qemu list.

-d

 -Kevin
 
 
 Kevin O'Connor (3):
   pvscsi: Don't store reference to struct pci_device.
   pvscsi: Always run entirely in 32bit mode.
   pvscsi: Remove use of LOWFLAT and GLOBALFLAT macros.
 
  Makefile          |   4 +--
  src/block.c       |   8 +++--
  src/hw/blockcmd.c |   3 +-
  src/hw/pvscsi.c   | 106 +++++++++++++++++++++++++-----------------------------
  4 files changed, 58 insertions(+), 63 deletions(-)
 
 -- 
 1.8.3.1
 
 
 _______________________________________________
 SeaBIOS mailing list
 SeaBIOS(a)seabios.org
 http://www.seabios.org/mailman/listinfo/seabios

[Kevin O'Connor]

On Wed, May 14, 2014 at 01:00:39PM -0400, Don Koch wrote:
 On Fri, 27 Dec 2013 12:17:42 -0500
 Kevin O'Connor <kevin(a)koconnor.net&gt; wrote:
 > As with the recent change to AHCI, if the pvscsi driver needs to jump
 > into 32bit mode to access a register, then it's better to run the
 > pvscsi driver entirely in 32bit mode.
 > 
 > Evgeny, can you take a look at this and give it a test?  Also, what
 > command line do you use for testing?
 > 
 > I tried testing locally by adding:
 > 
 > -device pvscsi,id=pvscsi0 -device scsi-disk,bus=pvscsi0.0,drive=drive0 -drive
id=drive0,if=none,file=dos-drivec-new
 > 
 > to my qemu command line (both qemu v1.6 and qemu v1.7), but it doesn't
 > work for me even before my changes.  It hangs in
 > pvscsi_wait_intr_cmpl().
 
 Try the following patch to qemu. I was getting hangs in this routine due to
 language definition/compiler/bogus input issues. The old one returned an
 off-by-one value.
It still fails for me.  I patched qemu and ran it with the
command-line above and it still causes a hang in SeaBIOS.

-Kevin

[Don Koch]

On Wed, 14 May 2014 19:40:23 -0400
Kevin O'Connor <kevin(a)koconnor.net&gt; wrote:

 On Wed, May 14, 2014 at 01:00:39PM -0400, Don Koch wrote:
 > On Fri, 27 Dec 2013 12:17:42 -0500
 > Kevin O'Connor <kevin(a)koconnor.net&gt; wrote:
 > > As with the recent change to AHCI, if the pvscsi driver needs to jump
 > > into 32bit mode to access a register, then it's better to run the
 > > pvscsi driver entirely in 32bit mode.
 > > 
 > > Evgeny, can you take a look at this and give it a test?  Also, what
 > > command line do you use for testing?
 > > 
 > > I tried testing locally by adding:
 > > 
 > > -device pvscsi,id=pvscsi0 -device scsi-disk,bus=pvscsi0.0,drive=drive0 -drive
id=drive0,if=none,file=dos-drivec-new
 > > 
 > > to my qemu command line (both qemu v1.6 and qemu v1.7), but it doesn't
 > > work for me even before my changes.  It hangs in
 > > pvscsi_wait_intr_cmpl().
 > 
 > Try the following patch to qemu. I was getting hangs in this routine due to
 > language definition/compiler/bogus input issues. The old one returned an
 > off-by-one value.
 
 It still fails for me.  I patched qemu and ran it with the
 command-line above and it still causes a hang in SeaBIOS.
Not sure if this is your problem, but we noted an issue with the generated code
for seabios which includes the build machine name; depending on the length of
the machine name, it changed the value in an uninitialized variable that caused
hanging (or not). Shorter names seem to be less problematic.

 -Kevin
Still researching.

-d