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(-)
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@koconnor.net --- 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;
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@koconnor.net --- 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);
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@koconnor.net --- 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
On Fri, 27 Dec 2013 12:17:42 -0500 Kevin O'Connor kevin@koconnor.net 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@seabios.org http://www.seabios.org/mailman/listinfo/seabios
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@koconnor.net 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
On Wed, 14 May 2014 19:40:23 -0400 Kevin O'Connor kevin@koconnor.net 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@koconnor.net 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
-
Don Koch -
Kevin O'Connor