<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 14, 2013 at 1:21 AM, Paul Menzel <span dir="ltr"><<a href="mailto:paulepanter@users.sourceforge.net" target="_blank">paulepanter@users.sourceforge.net</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Dear Evgeny,<br>
<br>
<br>
thank you for the patch iterations!<br>
<br>
A minor nitpick, I prefer having a verb in the commit summary.<br>
<br>
    Add pvscsi boot support<br>
<br>
Also you could add your reply (the things you tested on) to the commit<br>
message.<br>
<br></blockquote><div><br></div><div>done</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Am Sonntag, den <a href="tel:13.10.2013" value="+13102013">13.10.2013</a>, 20:07 +0300 schrieb Evgeny Budilovsky:<br>
<div><div class="h5">> Signed-off-by: Evgeny Budilovsky <<a href="mailto:evgeny.budilovsky@ravellosystems.com">evgeny.budilovsky@ravellosystems.com</a>><br>
> ---<br>
>  Makefile          |    2 +-<br>
>  src/Kconfig       |   12 ++<br>
>  src/block.c       |    1 +<br>
>  src/block.h       |    1 +<br>
>  src/hw/blockcmd.c |    3 +<br>
>  src/hw/pci_ids.h  |    3 +<br>
>  src/hw/pvscsi.c   |  365 +++++++++++++++++++++++++++++++++++++++++++++++++++++<br>
>  src/hw/pvscsi.h   |    8 ++<br>
>  src/post.c        |    2 +<br>
>  9 files changed, 396 insertions(+), 1 deletion(-)<br>
>  create mode 100644 src/hw/pvscsi.c<br>
>  create mode 100644 src/hw/pvscsi.h<br>
><br>
> diff --git a/Makefile b/Makefile<br>
> index 3218970..6c6302e 100644<br>
> --- a/Makefile<br>
> +++ b/Makefile<br>
> @@ -34,7 +34,7 @@ SRCBOTH=misc.c stacks.c output.c string.c x86.c block.c cdrom.c mouse.c kbd.c \<br>
>      hw/usb-hid.c hw/usb-msc.c hw/usb-uas.c \<br>
>      hw/blockcmd.c hw/floppy.c hw/ata.c hw/ahci.c hw/ramdisk.c \<br>
>      hw/virtio-ring.c hw/virtio-pci.c hw/virtio-blk.c hw/virtio-scsi.c \<br>
> -    hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c<br>
> +    hw/lsi-scsi.c hw/esp-scsi.c hw/megasas.c hw/pvscsi.c<br>
>  SRC16=$(SRCBOTH) system.c disk.c font.c<br>
>  SRC32FLAT=$(SRCBOTH) post.c memmap.c malloc.c pmm.c romfile.c optionroms.c \<br>
>      boot.c bootsplash.c jpeg.c bmp.c \<br>
> diff --git a/src/Kconfig b/src/Kconfig<br>
> index c40cc61..6e6814e 100644<br>
> --- a/src/Kconfig<br>
> +++ b/src/Kconfig<br>
> @@ -161,6 +161,18 @@ menu "Hardware support"<br>
>          default y<br>
>          help<br>
>              Support boot from virtio-scsi storage.<br>
> +    config PVSCSI<br>
> +        depends on DRIVES && QEMU_HARDWARE<br>
> +        bool "pvscsi controllers"<br>
<br>
</div></div>Should it be capitalizied?<br>
<div class="im"><br></div></blockquote><div>done  </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +        default y<br>
> +        help<br>
> +            Support boot from Paravirtualized SCSI storage. This kind of storage<br>
> +            is mainly supported by VMware ESX hypervisor. It is commonly used<br>
> +            to allow fast storage access by communicating directly with the<br>
> +            underlying hypervisor. Enabling this type of boot will allow<br>
> +            booting directly from images that were imported from ESX platform,<br>
<br>
</div>»ESX platform*s*« or »from an ESX platform«? (I am no native speaker<br>
though.)<br>
<br>
To make it shorter remove the »that were«: … imported from images<br>
imported from … (as above).<br>
<div class="im"><br></div></blockquote><div>done</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +            without the need to use slower emulation of storage controllers<br>
> +            such as IDE.<br>
<br>
<br>
</div>[…]<br>
<div><div class="h5"><br>
<br>
> diff --git a/src/hw/pvscsi.c b/src/hw/pvscsi.c<br>
> new file mode 100644<br>
> index 0000000..df5faeb<br>
> --- /dev/null<br>
> +++ b/src/hw/pvscsi.c<br>
> @@ -0,0 +1,365 @@<br>
> +// QEMU VMWARE Paravirtualized SCSI boot support.<br>
> +//<br>
> +// Copyright (c) 2013 Ravello Systems LTD (<a href="http://ravellosystems.com" target="_blank">http://ravellosystems.com</a>)<br>
> +//<br>
> +// Authors:<br>
> +//  Evgeny Budilovsky <<a href="mailto:evgeny.budilovsky@ravellosystems.com">evgeny.budilovsky@ravellosystems.com</a>><br>
> +//<br>
> +// This file may be distributed under the terms of the GNU LGPLv3 license.<br>
> +<br>
> +#include "biosvar.h" // GET_GLOBAL<br>
> +#include "block.h" // struct drive_s<br>
> +#include "blockcmd.h" // scsi_drive_setup<br>
> +#include "config.h" // CONFIG_*<br>
> +#include "malloc.h" // free<br>
> +#include "output.h" // dprintf<br>
> +#include "pci.h" // foreachpci<br>
> +#include "pci_ids.h" // PCI_DEVICE_ID_VMWARE_PVSCSI<br>
> +#include "pci_regs.h" // PCI_VENDOR_ID<br>
> +#include "std/disk.h" // DISK_RET_SUCCESS<br>
> +#include "string.h" // memset<br>
> +#include "util.h" // usleep<br>
> +#include "pvscsi.h"<br>
> +#include "virtio-ring.h" // PAGE_SHIFT, virt_to_phys<br>
> +<br>
> +#define MASK(n) ((1 << (n)) - 1)<br>
> +<br>
> +#define SIMPLE_QUEUE_TAG 0x20<br>
> +<br>
> +#define PVSCSI_INTR_CMPL_0                 (1 << 0)<br>
> +#define PVSCSI_INTR_CMPL_1                 (1 << 1)<br>
> +#define PVSCSI_INTR_CMPL_MASK              MASK(2)<br>
> +<br>
> +#define PVSCSI_INTR_MSG_0                  (1 << 2)<br>
> +#define PVSCSI_INTR_MSG_1                  (1 << 3)<br>
> +#define PVSCSI_INTR_MSG_MASK               (MASK(2) << 2)<br>
> +#define PVSCSI_INTR_ALL_SUPPORTED          MASK(4)<br>
> +<br>
> +#define PVSCSI_FLAG_CMD_WITH_SG_LIST       (1 << 0)<br>
> +#define PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB    (1 << 1)<br>
> +#define PVSCSI_FLAG_CMD_DIR_NONE           (1 << 2)<br>
> +#define PVSCSI_FLAG_CMD_DIR_TOHOST         (1 << 3)<br>
> +#define PVSCSI_FLAG_CMD_DIR_TODEVICE       (1 << 4)<br>
> +<br>
> +enum PVSCSIRegOffset {<br>
> +    PVSCSI_REG_OFFSET_COMMAND        =    0x0,<br>
> +    PVSCSI_REG_OFFSET_COMMAND_DATA   =    0x4,<br>
> +    PVSCSI_REG_OFFSET_COMMAND_STATUS =    0x8,<br>
> +    PVSCSI_REG_OFFSET_LAST_STS_0     =  0x100,<br>
> +    PVSCSI_REG_OFFSET_LAST_STS_1     =  0x104,<br>
> +    PVSCSI_REG_OFFSET_LAST_STS_2     =  0x108,<br>
> +    PVSCSI_REG_OFFSET_LAST_STS_3     =  0x10c,<br>
> +    PVSCSI_REG_OFFSET_INTR_STATUS    = 0x100c,<br>
> +    PVSCSI_REG_OFFSET_INTR_MASK      = 0x2010,<br>
> +    PVSCSI_REG_OFFSET_KICK_NON_RW_IO = 0x3014,<br>
> +    PVSCSI_REG_OFFSET_DEBUG          = 0x3018,<br>
> +    PVSCSI_REG_OFFSET_KICK_RW_IO     = 0x4018,<br>
> +};<br>
> +<br>
> +enum PVSCSICommands {<br>
> +    PVSCSI_CMD_FIRST             = 0,<br>
> +    PVSCSI_CMD_ADAPTER_RESET     = 1,<br>
> +    PVSCSI_CMD_ISSUE_SCSI        = 2,<br>
> +    PVSCSI_CMD_SETUP_RINGS       = 3,<br>
> +    PVSCSI_CMD_RESET_BUS         = 4,<br>
> +    PVSCSI_CMD_RESET_DEVICE      = 5,<br>
> +    PVSCSI_CMD_ABORT_CMD         = 6,<br>
> +    PVSCSI_CMD_CONFIG            = 7,<br>
> +    PVSCSI_CMD_SETUP_MSG_RING    = 8,<br>
> +    PVSCSI_CMD_DEVICE_UNPLUG     = 9,<br>
> +    PVSCSI_CMD_LAST              = 10<br>
> +};<br>
> +<br>
> +#define PVSCSI_SETUP_RINGS_MAX_NUM_PAGES        32<br>
> +struct PVSCSICmdDescSetupRings {<br>
> +    u32    reqRingNumPages;<br>
> +    u32    cmpRingNumPages;<br>
> +    u64    ringsStatePPN;<br>
> +    u64    reqRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];<br>
> +    u64    cmpRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];<br>
> +} PACKED;<br>
> +<br>
> +struct PVSCSIRingCmpDesc {<br>
> +    u64    context;<br>
> +    u64    dataLen;<br>
> +    u32    senseLen;<br>
> +    u16    hostStatus;<br>
> +    u16    scsiStatus;<br>
> +    u32    pad[2];<br>
> +} PACKED;<br>
> +<br>
> +struct PVSCSIRingsState {<br>
> +    u32    reqProdIdx;<br>
> +    u32    reqConsIdx;<br>
> +    u32    reqNumEntriesLog2;<br>
> +<br>
> +    u32    cmpProdIdx;<br>
> +    u32    cmpConsIdx;<br>
> +    u32    cmpNumEntriesLog2;<br>
> +<br>
> +    u8     pad[104];<br>
> +<br>
> +    u32    msgProdIdx;<br>
> +    u32    msgConsIdx;<br>
> +    u32    msgNumEntriesLog2;<br>
> +} PACKED;<br>
> +<br>
> +struct PVSCSIRingReqDesc {<br>
> +    u64    context;<br>
> +    u64    dataAddr;<br>
> +    u64    dataLen;<br>
> +    u64    senseAddr;<br>
> +    u32    senseLen;<br>
> +    u32    flags;<br>
> +    u8     cdb[16];<br>
> +    u8     cdbLen;<br>
> +    u8     lun[8];<br>
> +    u8     tag;<br>
> +    u8     bus;<br>
> +    u8     target;<br>
> +    u8     vcpuHint;<br>
> +    u8     unused[59];<br>
> +} PACKED;<br>
> +<br>
> +struct pvscsi_ring_dsc_s {<br>
> +    struct PVSCSIRingsState *ring_state;<br>
> +    struct PVSCSIRingReqDesc *ring_reqs;<br>
> +    struct PVSCSIRingCmpDesc *ring_cmps;<br>
> +};<br>
> +<br>
> +struct pvscsi_lun_s {<br>
> +    struct drive_s drive;<br>
> +    struct pci_device *pci;<br>
> +    u32 iobase;<br>
> +    u8 target;<br>
> +    u8 lun;<br>
> +    struct pvscsi_ring_dsc_s *ring_dsc;<br>
> +};<br>
<br>
</div></div>Should these go into the header file?<br>
<div><div class="h5"><br></div></div></blockquote><div>I saw that other storage controllers add constants and structs to the *.c file (lsi-scsi.c, pvscsi.c).</div><div>So I assumed this was a convention in seabios code base.</div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +<br>
> +static void<br>
> +pvscsi_write_cmd_desc(u32 iobase, u32 cmd, const void *desc, size_t len)<br>
> +{<br>
> +    const u32 *ptr = desc;<br>
> +    size_t i;<br>
> +<br>
> +    len /= sizeof(*ptr);<br>
> +    pci_writel(iobase + PVSCSI_REG_OFFSET_COMMAND, cmd);<br>
> +    for (i = 0; i < len; i++)<br>
> +        pci_writel(iobase + PVSCSI_REG_OFFSET_COMMAND_DATA, ptr[i]);<br>
> +}<br>
> +<br>
> +static void<br>
> +pvscsi_kick_rw_io(u32 iobase)<br>
> +{<br>
> +    pci_writel(iobase + PVSCSI_REG_OFFSET_KICK_RW_IO, 0);<br>
> +}<br>
> +<br>
> +static void<br>
> +pvscsi_wait_intr_cmpl(u32 iobase)<br>
> +{<br>
> +    while (!(pci_readl(iobase + PVSCSI_REG_OFFSET_INTR_STATUS) & PVSCSI_INTR_CMPL_MASK))<br>
> +        usleep(5);<br>
> +    pci_writel(iobase + PVSCSI_REG_OFFSET_INTR_STATUS, PVSCSI_INTR_CMPL_MASK);<br>
> +<br>
> +}<br>
> +<br>
> +static void<br>
> +pvscsi_init_rings(u32 iobase, struct pvscsi_ring_dsc_s **ring_dsc)<br>
> +{<br>
> +    struct PVSCSICmdDescSetupRings cmd = {0,};<br>
> +<br>
> +    struct pvscsi_ring_dsc_s *dsc = memalign_low(sizeof(*dsc), PAGE_SIZE);<br>
> +    if (!dsc) {<br>
> +        warn_noalloc();<br>
> +        return;<br>
> +    }<br>
> +<br>
> +    memset(dsc, 0, sizeof(*dsc));<br>
<br>
</div></div>That memset is not needed, right?<br>
<div class="im"><br></div></blockquote><div>right. I've removed it </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +<br>
> +    dsc->ring_state =<br>
> +        (struct PVSCSIRingsState *)memalign_low(PAGE_SIZE, PAGE_SIZE);<br>
> +    dsc->ring_reqs =<br>
> +        (struct PVSCSIRingReqDesc *)memalign_low(PAGE_SIZE, PAGE_SIZE);<br>
> +    dsc->ring_cmps =<br>
> +        (struct PVSCSIRingCmpDesc *)memalign_low(PAGE_SIZE, PAGE_SIZE);<br>
> +    if (!dsc->ring_state || !dsc->ring_reqs || !dsc->ring_cmps) {<br>
> +        warn_noalloc();<br>
> +        return;<br>
> +    }<br>
> +    memset(dsc->ring_state, 0, PAGE_SIZE);<br>
> +    memset(dsc->ring_reqs, 0, PAGE_SIZE);<br>
> +    memset(dsc->ring_cmps, 0, PAGE_SIZE);<br>
> +<br>
> +    cmd.reqRingNumPages = 1;<br>
> +    cmd.cmpRingNumPages = 1;<br>
> +    cmd.ringsStatePPN = virt_to_phys(dsc->ring_state) >> PAGE_SHIFT;<br>
> +    cmd.reqRingPPNs[0] = virt_to_phys(dsc->ring_reqs) >> PAGE_SHIFT;;<br>
> +    cmd.cmpRingPPNs[0] = virt_to_phys(dsc->ring_cmps) >> PAGE_SHIFT;;<br>
<br>
</div>1. Just one semicolon is needed at the end.<br>
<br></blockquote><div>done</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2. I am no C professional, but why are the `memset()` calls above needed<br>
when the content does not seems to be used, when the pointer is casted<br>
to `unsigned int`?<br></blockquote><div>The pointers I memset, point to physical pages of memory. I need to zero them so that the pvscsi controller on qemu won't see some garbage values such as invalid requests count.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +<br>
> +    pvscsi_write_cmd_desc(iobase, PVSCSI_CMD_SETUP_RINGS,<br>
> +                          &cmd, sizeof(cmd));<br>
> +    *ring_dsc = dsc;<br>
> +}<br>
<br>
</div>[…]<br>
<br>
<br>
Thanks,<br>
<br>
Paul<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Best Regards,<div>Evgeny</div></div>
</div></div>