qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/4 V3] PVCSI paravirtualized device implementat


From: Deep Debroy
Subject: Re: [Qemu-devel] [PATCH 4/4 V3] PVCSI paravirtualized device implementation
Date: Mon, 9 Jul 2012 00:31:34 -0700

On Sat, Jul 7, 2012 at 1:38 AM, Blue Swirl <address@hidden> wrote:
> On Sat, Jul 7, 2012 at 5:07 AM, Deep Debroy <address@hidden> wrote:
>> Signed-off-by: Deep Debroy <address@hidden>
>> ---
>>  default-configs/pci.mak    |    1 +
>>  docs/specs/pvscsi-spec.txt |   92 ++++
>>  hw/Makefile.objs           |    1 +
>>  hw/pci.h                   |    1 +
>>  hw/pvscsi.c                | 1260 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  hw/pvscsi.h                |  442 ++++++++++++++++
>>  6 files changed, 1797 insertions(+)
>>  create mode 100644 docs/specs/pvscsi-spec.txt
>>  create mode 100644 hw/pvscsi.c
>>  create mode 100644 hw/pvscsi.h
>>
>> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
>> index 9d3e1db..9fd4896 100644
>> --- a/default-configs/pci.mak
>> +++ b/default-configs/pci.mak
>> @@ -10,6 +10,7 @@ CONFIG_EEPRO100_PCI=y
>>  CONFIG_PCNET_PCI=y
>>  CONFIG_PCNET_COMMON=y
>>  CONFIG_LSI_SCSI_PCI=y
>> +CONFIG_PVSCSI_SCSI_PCI=y
>>  CONFIG_RTL8139_PCI=y
>>  CONFIG_E1000_PCI=y
>>  CONFIG_IDE_CORE=y
>> diff --git a/docs/specs/pvscsi-spec.txt b/docs/specs/pvscsi-spec.txt
>> new file mode 100644
>> index 0000000..27f5529
>> --- /dev/null
>> +++ b/docs/specs/pvscsi-spec.txt
>> @@ -0,0 +1,92 @@
>> +General Description
>> +===================
>> +
>> +This document describes VMWare PVSCSI device interface specification.
>> +Created by Dmitry Fleytman (address@hidden), Daynix Computing LTD.
>> +Based on source code of PVSCSI Linux driver from kernel 3.0.4
>> +
>> +PVSCSI Device Interface Overview
>> +================================
>> +
>> +The interface is based on memory area shared between hypervisor and VM.
>> +Memory area is obtained by driver as device IO memory resource of
>> +PVSCSI_MEM_SPACE_SIZE length.
>> +The shared memory consists of registers area and rings area.
>> +The registers area is used to raise hypervisorinterrupts and issue device
>
> hypervisor interrupts
>
>> +commands. The rings area is used to transfer data descruiptors and SCSI
>
> descriptors
>
>> +commands from VM to hypervisor and to transfer messages produced by
>> +hypervisor to VM. Data itself is transferred via virtual scatter-gather DMA.
>> +
>> +PVSCSI Device Registers
>> +=======================
>> +
>> +Registers area length is 1 page (PVSCSI_MEM_SPACE_COMMAND_NUM_PAGES).
>> +Registers area structure is described by PVSCSIRegOffset enumeration.
>> +There are registers to issue device command (with optional short data),
>> +issue device interrupt, control interrupts masking.
>> +
>> +PVSCSI Device Rings
>> +===================
>> +
>> +There are three rings in shared memory:
>> +
>> +    1. Request ring (struct PVSCSIRingReqDesc *req_ring)
>> +        - ring for OS to device requests
>> +    2. Completion ring (struct PVSCSIRingCmpDesc *cmp_ring)
>> +        - ring for device request completions
>> +    3. Message ring (struct PVSCSIRingMsgDesc *msg_ring)
>> +        - ring for messages from device.
>> +       This ring is optional and may be not configured.
>> +There is a control area (struct PVSCSIRingsState *rings_state) used to 
>> control
>> +rings operation.
>> +
>> +PVSCSI Device to Host Interrupts
>> +================================
>> +There are following interrupt types supported by PVSCSI device:
>> +    1. Completion interrupts (completion ring notifications):
>> +        PVSCSI_INTR_CMPL_0
>> +        PVSCSI_INTR_CMPL_1
>> +    2. Message interrupts (message ring notifications):
>> +        PVSCSI_INTR_MSG_0
>> +        PVSCSI_INTR_MSG_1
>> +
>> +Interrupts are controlled via PVSCSI_REG_OFFSET_INTR_MASK register
>> +Bit set means interrupt enabled, bit cleared - disabled
>> +
>> +Interrupt modes supported are legacy, MSI and MSI-X
>> +In case of legacy interrupts register PVSCSI_REG_OFFSET_INTR_STATUS
>> +used to verify interrupt arrival and to clear interrupt state
>> +Interrupts are cleared by writing processed bits back
>> +to interrupt status register.
>> +
>> +PVSCSI Device Operation Sequences
>> +=================================
>> +
>> +1. Startup sequence:
>> +    a. Issue PVSCSI_CMD_ADAPTER_RESET command;
>> +    aa. Windows driver reads interrupt status register here;
>> +    b. Issue PVSCSI_CMD_SETUP_MSG_RING command with no additional data,
>> +       check status and disable device messages if error returned;
>> +       (Omitted if device messages disabled by driver configuration)
>> +    c. Issue PVSCSI_CMD_SETUP_RINGS command, provide rings configuration
>> +       as struct PVSCSICmdDescSetupRings;
>> +    d. Issue PVSCSI_CMD_SETUP_MSG_RING command again, provide
>> +       rings configuration as struct PVSCSICmdDescSetupMsgRing;
>> +    e. Unmask completion and message (if device messages enabled) 
>> interrupts.
>> +
>> +2. Shutdown sequences
>> +    a. Mask interrupts;
>> +    b. Flush request ring using PVSCSI_REG_OFFSET_KICK_NON_RW_IO;
>> +    c. Issue PVSCSI_CMD_ADAPTER_RESET command.
>> +
>> +3. Send request
>> +    a. Fill next free request ring descriptor;
>> +    b. Issue PVSCSI_REG_OFFSET_KICK_RW_IO for R/W operations;
>> +       or PVSCSI_REG_OFFSET_KICK_NON_RW_IO for other operations.
>> +
>> +4. Abort command
>> +    a. Issue PVSCSI_CMD_ABORT_CMD command;
>> +
>> +5. Request completion processing
>> +    a. Upon completion interrupt arrival process completion
>> +       and message (if enabled) rings.
>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> index 3d77259..6dd1eea 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -86,6 +86,7 @@ hw-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>>
>>  # SCSI layer
>>  hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
>> +hw-obj-$(CONFIG_PVSCSI_SCSI_PCI) += pvscsi.o
>>  hw-obj-$(CONFIG_ESP) += esp.o
>>
>>  hw-obj-y += sysbus.o isa-bus.o
>> diff --git a/hw/pci.h b/hw/pci.h
>> index 79d38fd..3c197d9 100644
>> --- a/hw/pci.h
>> +++ b/hw/pci.h
>> @@ -60,6 +60,7 @@
>>  #define PCI_DEVICE_ID_VMWARE_NET         0x0720
>>  #define PCI_DEVICE_ID_VMWARE_SCSI        0x0730
>>  #define PCI_DEVICE_ID_VMWARE_IDE         0x1729
>> +#define PCI_DEVICE_ID_VMWARE_PVSCSI      0x07C0
>
> Please retain numeric order.

The pvscsi driver in the linux guests use the 0x7C0 ID as part of
MODULE_DEVICE_TABLE. So I will have to keep that intact. The other IDs
(_NET, _SCSI and _IDE) don't appear to be used by anything in qemu.
Should I remove them?
>
>>
>>  /* Intel (0x8086) */
>>  #define PCI_DEVICE_ID_INTEL_82551IT      0x1209
>> diff --git a/hw/pvscsi.c b/hw/pvscsi.c
>> new file mode 100644
>> index 0000000..719ea98
>> --- /dev/null
>> +++ b/hw/pvscsi.c
>> @@ -0,0 +1,1260 @@
>> +/*
>> + * QEMU VMWARE PVSCSI paravirtual SCSI bus
>> + *
>> + * Copyright (c) 2012 Ravello Systems LTD (http://ravellosystems.com)
>> + *
>> + * Developed by Daynix Computing LTD (http://www.daynix.com)
>> + *
>> + * Based on implementation by Paolo Bonzini
>> + * http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00729.html
>> + *
>> + * Authors:
>> + * Paolo Bonzini <address@hidden>
>> + * Dmitry Fleytman <address@hidden>
>> + * Yan Vugenfirer <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#define DEBUG_PVSCSI_WARNINGS
>> +#define DEBUG_PVSCSI_ERRORS
>> +/*
>> +#define DEBUG_PVSCSI_CALLBACKS
>> +#define DEBUG_PVSCSI_RINGS
>> +#define DEBUG_PVSCSI_COMMANDS
>> +#define DEBUG_PVSCSI_INTERRUPTS
>> +#define DEBUG_PVSCSI_SHMEM_ACCESS
>> +#define DEBUG_PVSCSI_SHMEM_ACCESS
>> +#define DEBUG_SCSI_EXCHANGE
>> +*/
>
> Please use //, it's easier to enable one line in the middle. With
> tracepoints there would be no need.

Sounds good. Will modify the code to use tracepoints.
>
>> +
>> +#ifdef DEBUG_PVSCSI_SHMEM_ACCESS
>> +#define DSHPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][SH][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DSHPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_PVSCSI_CALLBACKS
>> +#define DCBPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][CB][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DCBPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_PVSCSI_WARNINGS
>> +#define DWRPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][WR][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DWRPRINTF(fmt, ...) do {} while (0)
>> +#endif
>
> Please check if qemu_log(LOG_UNIMP,...) would make sense for the cases
> where the guest may be issuing commands which QEMU doesn't implement
> yet.
>
>> +
>> +#ifdef DEBUG_PVSCSI_ERRORS
>> +#define DERPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][ER][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DERPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_PVSCSI_COMMANDS
>> +#define DCMPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][CM][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DCMPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_SCSI_EXCHANGE
>> +#define DSXPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][SX][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DSXPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_PVSCSI_RINGS
>> +#define DRIPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][RI][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DRIPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#ifdef DEBUG_PVSCSI_INTERRUPTS
>> +#define DIRPRINTF(fmt, ...)                                                 
>> \
>> +    do {                                                                    
>> \
>> +        printf("[pvscsi][IR][%s]: " fmt "\n", __func__, ## __VA_ARGS__);    
>> \
>> +    } while (0)
>> +#else
>> +#define DIRPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +/*
>> + * MSI-X support is disabled because it leads Windows OS to crash on 
>> startup.
>> + * The crash happens because Windows driver requires MSI-X shared memory
>> + * to be part of the same BAR used for rings state, registers, etc.
>> + * This is not supported by QEMU infrastructure so separate BAR created from
>> + * MSI-X purposes. Windows driver fails to deal with 2 BARs.
>> + * Linux works fine with current MSI-X implementation enabled.
>> + */
>> +#undef PVSCSI_ENABLE_MSIX
>> +
>> +#include "softfloat.h"
>> +#include "compiler.h"
>> +#include "scsi-defs.h"
>> +#include "hw/hw.h"
>> +#include "hw/pci.h"
>> +#include "hw/scsi.h"
>> +#ifdef PVSCSI_ENABLE_MSIX
>
> Please remove.

For now, I think I will remove MSIX support altogether from pvscsi on
the qemu side so that both Linux and Windows guests can work using
regular MSI. If I can figure out a way to get the Windows driver to
work with MSIX, I will introduce that later. Does that sound good?
>
>> +#include "msix.h"
>> +#endif
>> +#include "msi.h"
>> +#include "qemu-queue.h"
>> +#include "pvscsi.h"
>> +#include "vmware_utils.h"
>
> The include lines should be just after the copyright header.
>
>> +
>> +#define PVSCSI_MAX_DEVS                   (64)
>> +#define PVSCSI_MSIX_NUM_VECTORS           (1)
>> +
>> +typedef struct {
>
> typedef struct PVSCSIRingsMgr {
>
>> +    uint64_t            rs_pa;
>> +
>> +    uint32_t            txr_len_mask;
>> +    uint32_t            rxr_len_mask;
>> +    uint64_t            req_ring_pages_pa[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
>> +    uint64_t            cmp_ring_pages_pa[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
>> +    uint64_t            consumed_ptr;
>> +    uint64_t            filled_cmp_ptr;
>> +} PVSCSIRingsMgr;
>> +
>> +#define _RS_GET_FIELD(rs_pa, field) \
>> +    (vmw_shmem_ld32(rs_pa + offsetof(struct PVSCSIRingsState, field)))
>> +#define _RS_SET_FIELD(rs_pa, field, val) \
>> +    (vmw_shmem_st32(rs_pa + offsetof(struct PVSCSIRingsState, field), val))
>
> Don't use identifiers with leading underscores.
>
>> +
>> +/* Integer binary logarithm */
>> +static int
>> +pvscsi_log2(uint32_t input)
>> +{
>> +    int log = 0;
>> +    assert(input > 0);
>> +    while (input >> ++log) {
>> +
>> +        };
>
> Useless semicolon.
>
>> +    return log;
>> +}
>> +
>> +static void
>> +pvscsi_rings_mgr_init(PVSCSIRingsMgr *m, struct PVSCSICmdDescSetupRings *ri)
>> +{
>> +    int i;
>> +    uint32_t txr_len_log2, rxr_len_log2;
>> +    uint32_t req_ring_size, cmp_ring_size;
>> +    m->rs_pa = ri->ringsStatePPN << PAGE_SHIFT;
>> +
>> +    req_ring_size = ri->reqRingNumPages * 
>> PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
>> +    cmp_ring_size = ri->cmpRingNumPages * 
>> PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE;
>> +    txr_len_log2 = pvscsi_log2(req_ring_size - 1);
>> +    rxr_len_log2 = pvscsi_log2(cmp_ring_size - 1);
>> +
>> +    m->txr_len_mask = MASK(txr_len_log2);
>> +    m->rxr_len_mask = MASK(rxr_len_log2);
>> +
>> +    m->consumed_ptr = 0;
>> +    m->filled_cmp_ptr = 0;
>> +
>> +    for (i = 0; i < ri->reqRingNumPages; i++) {
>> +        m->req_ring_pages_pa[i] = ri->reqRingPPNs[i] << PAGE_SHIFT;
>> +    }
>> +
>> +    for (i = 0; i < ri->cmpRingNumPages; i++) {
>> +        m->cmp_ring_pages_pa[i] = ri->cmpRingPPNs[i] << PAGE_SHIFT;
>> +    }
>> +
>> +    _RS_SET_FIELD(m->rs_pa, reqProdIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, reqConsIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, reqNumEntriesLog2, txr_len_log2);
>> +
>> +    _RS_SET_FIELD(m->rs_pa, cmpProdIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, cmpConsIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, cmpNumEntriesLog2, rxr_len_log2);
>> +
>> +    _RS_SET_FIELD(m->rs_pa, msgProdIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, msgConsIdx, 0);
>> +    _RS_SET_FIELD(m->rs_pa, msgNumEntriesLog2, 0);
>> +
>> +    DRIPRINTF("TX/RX rings logarithms set to %d/%d",
>> +              txr_len_log2, rxr_len_log2);
>> +
>> +    /* Flush ring state page changes */
>> +    smp_wmb();
>> +}
>> +
>> +static void
>> +pvscsi_rings_mgr_cleanup(PVSCSIRingsMgr *mgr)
>> +{
>> +    mgr->rs_pa = 0;
>> +    mgr->txr_len_mask = 0;
>> +    mgr->consumed_ptr = 0;
>> +    mgr->filled_cmp_ptr = 0;
>> +    memset(mgr->req_ring_pages_pa, 0, sizeof(mgr->req_ring_pages_pa));
>> +    memset(mgr->cmp_ring_pages_pa, 0, sizeof(mgr->cmp_ring_pages_pa));
>> +}
>> +
>> +static inline target_phys_addr_t
>
> 'inline' may be premature optimization.
>
>> +pvscsi_rings_mgr_pop_req_descr(PVSCSIRingsMgr *mgr)
>> +{
>> +    uint32_t ready_prt = _RS_GET_FIELD(mgr->rs_pa, reqProdIdx);
>> +
>> +    if (ready_prt != mgr->consumed_ptr) {
>> +        uint32_t next_ready_ptr =
>> +            mgr->consumed_ptr++ & mgr->txr_len_mask;
>> +        uint32_t next_ready_page =
>> +            next_ready_ptr / PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
>> +        uint32_t inpage_idx =
>> +            next_ready_ptr % PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE;
>> +
>> +        return mgr->req_ring_pages_pa[next_ready_page] +
>> +               inpage_idx * sizeof(PVSCSIRingReqDesc);
>> +    } else {
>> +        return 0;
>> +    }
>> +}
>> +
>> +static inline void
>> +pvscsi_rings_mgr_flush_req_ring(PVSCSIRingsMgr *mgr)
>> +{
>> +    _RS_SET_FIELD(mgr->rs_pa, reqConsIdx, mgr->consumed_ptr);
>> +}
>> +
>> +static inline target_phys_addr_t
>> +pvscsi_rings_mgr_pop_cmp_descr(PVSCSIRingsMgr *mgr)
>> +{
>> +    /*
>> +     * According to Linux driver code it explicitly verifies that number
>> +     * of requests being processed by device is less then the size of
>> +     * completion queue, so device may omit completion queue overflow
>> +     * conditions check. We assume that this is true for other (Windows)
>> +     * drivers as well.
>> +     */
>> +
>> +    uint32_t free_cmp_ptr =
>> +        mgr->filled_cmp_ptr++ & mgr->rxr_len_mask;
>> +    uint32_t free_cmp_page =
>> +        free_cmp_ptr / PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE;
>> +    uint32_t inpage_idx =
>> +        free_cmp_ptr % PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE;
>> +    return mgr->cmp_ring_pages_pa[free_cmp_page] +
>> +           inpage_idx * sizeof(PVSCSIRingCmpDesc);
>> +}
>> +
>> +static inline void
>> +pvscsi_rings_mgr_flush_cmp_ring(PVSCSIRingsMgr *mgr)
>> +{
>> +    /* Flush descriptor changes */
>> +    smp_wmb();
>> +
>> +    DRIPRINTF("New production counter of completion ring is %" PRIx64,
>> +              mgr->filled_cmp_ptr);
>> +
>> +    _RS_SET_FIELD(mgr->rs_pa, cmpProdIdx, mgr->filled_cmp_ptr);
>> +}
>> +
>> +typedef QTAILQ_HEAD(, PVSCSIRequest) PVSCSIRequestList;
>> +#define PVSCSI_MAX_CMD_DATA_WORDS \
>> +    (sizeof(struct PVSCSICmdDescSetupRings)/sizeof(uint32_t))
>> +
>> +typedef struct {
>> +    PCIDevice dev;
>> +    MemoryRegion io_space;
>> +    SCSIBus bus;
>> +    QEMUBH *completion_worker;
>> +    PVSCSIRequestList pending_queue;
>> +    PVSCSIRequestList completion_queue;
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>
> I think this can be removed.
>
>> +    MemoryRegion msix_space;
>> +    uint8_t msix_used;   /* Whether MSI-X support was installed 
>> successfully */
>> +#endif
>> +    uint8_t msi_used;    /* Whether MSI support was installed successfully  
>>  */
>> +    uint64_t reg_interrupt_status;        /* Interrupt status register 
>> value */
>> +    uint64_t reg_interrupt_enabled;       /* Interrupt mask register value  
>>  */
>> +    uint64_t reg_command_status;          /* Command status register value  
>>  */
>> +
>> +    /* Command data adoption mechanism */
>> +    uint64_t curr_cmd;                   /* Last command arrived            
>>  */
>> +    uint32_t curr_cmd_data_cntr;      /* Amount of data for last command  */
>> +
>> +    /* Collector for current command data */
>> +    uint32_t curr_cmd_data[PVSCSI_MAX_CMD_DATA_WORDS];
>> +
>> +    uint8_t rings_info_valid;            /* Whether rings are initialized   
>>  */
>> +    PVSCSIRingsMgr rings;                /* Data transfer rings manager     
>>  */
>> +} PVSCSI_State;
>
> The order of the fields could be optimized, there are a lot of
> structure holes due to alignment.
>
>> +
>> +static void
>> +pvscsi_reset_state(PVSCSI_State *s)
>> +{
>> +    s->curr_cmd = PVSCSI_CMD_FIRST;
>> +    s->curr_cmd_data_cntr = 0;
>> +    s->reg_command_status = PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +    s->reg_interrupt_status = 0;
>> +    pvscsi_rings_mgr_cleanup(&s->rings);
>> +    s->rings_info_valid = FALSE;
>> +    QTAILQ_INIT(&s->pending_queue);
>> +    QTAILQ_INIT(&s->completion_queue);
>> +}
>> +
>> +typedef struct PVSCSISGState {
>> +    target_phys_addr_t elemAddr;
>> +    target_phys_addr_t dataAddr;
>> +    uint32_t resid;
>> +} PVSCSISGState;
>> +
>> +typedef struct PVSCSIRequest {
>> +    SCSIRequest *sreq;
>> +    uint8_t sense_key;
>> +    uint8_t completed;
>> +    int lun;
>> +    QEMUSGList sgl;
>> +    PVSCSISGState sg;
>> +    struct PVSCSIRingReqDesc req;
>> +    struct PVSCSIRingCmpDesc cmp;
>> +    QTAILQ_ENTRY(PVSCSIRequest) next;
>> +} PVSCSIRequest;
>> +
>> +static void
>> +pvscsi_free_queue(PVSCSIRequestList *req_list)
>> +{
>> +    PVSCSIRequest *pvscsi_req;
>> +
>> +    while (!QTAILQ_EMPTY(req_list)) {
>> +        pvscsi_req = QTAILQ_FIRST(req_list);
>> +        QTAILQ_REMOVE(req_list, pvscsi_req, next);
>> +        g_free(pvscsi_req);
>> +    }
>> +}
>> +
>> +static void
>> +pvscsi_reset_adapter(PVSCSI_State *s)
>> +{
>> +    qbus_reset_all_fn(&s->bus);
>> +    pvscsi_free_queue(&s->completion_queue);
>> +    assert(QTAILQ_EMPTY(&s->pending_queue));
>> +    pvscsi_reset_state(s);
>> +}
>> +
>> +static inline void
>> +pvscsi_update_irq_status(PVSCSI_State *s)
>> +{
>> +    int should_raise = !!(s->reg_interrupt_enabled & 
>> s->reg_interrupt_status);
>
> bool
>
>> +
>> +    DIRPRINTF("Interrupt level set to %d (MASK: 0x%llX, STATUS: 0x%llx)",
>> +              should_raise,
>> +              (long long unsigned int) s->reg_interrupt_enabled,
>> +              (long long unsigned int) s->reg_interrupt_status);
>
> Just use PRIx64 and remove the casts.
>
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +    if (s->msix_used && msix_enabled(&s->dev)) {
>> +        if (should_raise) {
>> +            DIRPRINTF("Sending MSI-X notification");
>> +            msix_notify(&s->dev, PVSCSI_VECTOR_COMPLETION);
>> +        }
>> +        return;
>> +    }
>> +#endif
>> +
>> +    if (s->msi_used && msi_enabled(&s->dev)) {
>> +        if (should_raise) {
>> +            DIRPRINTF("Sending MSI notification");
>> +            msi_notify(&s->dev, PVSCSI_VECTOR_COMPLETION);
>> +        }
>> +        return;
>> +    }
>> +
>> +    qemu_set_irq(s->dev.irq[0], should_raise);
>> +}
>> +
>> +static inline void
>> +pvscsi_raise_completion_interrupt(PVSCSI_State *s)
>> +{
>> +    s->reg_interrupt_status |= PVSCSI_INTR_CMPL_0;
>> +
>> +    /* Memory barrier to flush interrupt status register changes*/
>> +    smp_wmb();
>> +
>> +    pvscsi_update_irq_status(s);
>> +}
>> +
>> +static void
>> +pvscsi_cmp_ring_put(PVSCSI_State *s, struct PVSCSIRingCmpDesc *cmp_desc)
>> +{
>> +    target_phys_addr_t cmp_descr_pa;
>> +
>> +    cmp_descr_pa = pvscsi_rings_mgr_pop_cmp_descr(&s->rings);
>> +    DRIPRINTF("Got completion descriptor %"PRIx64, cmp_descr_pa);
>> +
>> +    vmw_shmem_write(cmp_descr_pa, (void *)cmp_desc, sizeof(*cmp_desc));
>> +}
>> +
>> +static void
>> +pvscsi_process_completion_queue(void *opaque)
>> +{
>> +    PVSCSI_State *s = opaque;
>> +    PVSCSIRequest *pvscsi_req;
>> +    bool has_completed = false;
>> +
>> +    while (!QTAILQ_EMPTY(&s->completion_queue)) {
>> +        pvscsi_req = QTAILQ_FIRST(&s->completion_queue);
>> +        QTAILQ_REMOVE(&s->completion_queue, pvscsi_req, next);
>> +        pvscsi_cmp_ring_put(s, &pvscsi_req->cmp);
>> +        g_free(pvscsi_req);
>> +        has_completed++;
>> +    }
>> +
>> +    if (has_completed) {
>> +        pvscsi_rings_mgr_flush_cmp_ring(&s->rings);
>> +        pvscsi_raise_completion_interrupt(s);
>> +    }
>> +}
>> +
>> +static inline void
>> +pvscsi_schedule_completion_processing(PVSCSI_State *s)
>> +{
>> +    /* Try putting more complete requests on the ring. */
>> +    if (!QTAILQ_EMPTY(&s->completion_queue)) {
>> +        qemu_bh_schedule(s->completion_worker);
>> +    }
>> +}
>> +
>> +static inline void
>> +pvscsi_complete_request(PVSCSI_State *s, PVSCSIRequest *r)
>> +{
>> +    assert(!r->completed);
>> +    DSXPRINTF("Completion: ctx: %"PRIx64", len: %"PRIx64", sense key: %u",
>> +              r->cmp.context, r->cmp.dataLen, r->sense_key);
>> +    if (r->sreq != NULL) {
>> +        scsi_req_unref(r->sreq);
>> +        r->sreq = NULL;
>> +    }
>> +    r->completed = 1;
>> +    QTAILQ_REMOVE(&s->pending_queue, r, next);
>> +    QTAILQ_INSERT_TAIL(&s->completion_queue, r, next);
>> +    pvscsi_schedule_completion_processing(s);
>> +}
>> +
>> +static QEMUSGList *pvscsi_get_sg_list(SCSIRequest *r)
>> +{
>> +    PVSCSIRequest *req = r->hba_private;
>> +
>> +    DSXPRINTF("Get SG list: depth: %u, size: %lu",
>> +              req->sgl.nsg, req->sgl.size);
>> +
>> +    return &req->sgl;
>> +}
>> +
>> +static void
>> +pvscsi_get_next_sg_elem(PVSCSISGState *sg)
>> +{
>> +    struct PVSCSISGElement elem;
>> +
>> +    for (;; sg->elemAddr = elem.addr) {
>> +        vmw_shmem_read(sg->elemAddr, (void *)&elem, sizeof(elem));
>> +        if (0 != (elem.flags & ~PVSCSI_KNOWN_FLAGS)) {
>
> The order in the expression is unnatural, please reverse.
>
>> +            /*
>> +             * There is PVSCSI_SGE_FLAG_CHAIN_ELEMENT flag described in
>> +             * header file but its value is unknown. This flag requires
>> +             * additional processing, so we put warning here to catch it
>> +             * some day and make proper implementation
>> +             */
>> +            DWRPRINTF("Unknown flags in SG element (val: 0x%X)", 
>> elem.flags);
>
> Lowercase hex, please.
>
>> +        }
>> +        break;
>> +    }
>> +
>> +    sg->elemAddr += sizeof(elem);
>> +    sg->dataAddr = elem.addr;
>> +    sg->resid = elem.length;
>> +}
>> +
>> +static inline void
>> +pvscsi_write_sense(PVSCSIRequest *r, uint8_t *sense, int len)
>> +{
>> +    r->cmp.senseLen = MIN(r->req.senseLen, len);
>> +    r->sense_key = sense[2];
>> +    vmw_shmem_write(r->req.senseAddr, sense, r->cmp.senseLen);
>> +}
>> +
>> +static void
>> +pvscsi_command_complete(SCSIRequest *req, uint32_t status, size_t resid)
>> +{
>> +    PVSCSI_State *s = DO_UPCAST(PVSCSI_State, dev.qdev, 
>> req->bus->qbus.parent);
>> +    PVSCSIRequest *pvscsi_req = req->hba_private;
>> +
>> +    if (!pvscsi_req) {
>> +        DERPRINTF("PVSCSI: Can't find request for tag 0x%x", req->tag);
>> +        return;
>> +    }
>> +
>> +    if (resid) {
>> +        /* Short transfer.  */
>> +        DSXPRINTF("Not all data required for command transferred.");
>> +        pvscsi_req->cmp.hostStatus = BTSTAT_DATARUN;
>> +    }
>> +
>> +    pvscsi_req->cmp.scsiStatus = status;
>> +    if (CHECK_CONDITION == pvscsi_req->cmp.scsiStatus) {
>
> Order
>
>> +        uint8_t sense[SCSI_SENSE_BUF_SIZE];
>> +        int sense_len =
>> +            scsi_req_get_sense(pvscsi_req->sreq, sense, sizeof(sense));
>> +
>> +        DSXPRINTF("Sense information length is %d bytes", sense_len);
>> +        pvscsi_write_sense(pvscsi_req, sense, sense_len);
>> +    }
>> +    qemu_sglist_destroy(&pvscsi_req->sgl);
>> +    pvscsi_complete_request(s, pvscsi_req);
>> +}
>> +
>> +static void
>> +pvscsi_request_cancelled(SCSIRequest *req)
>> +{
>> +    PVSCSI_State *s = DO_UPCAST(PVSCSI_State, dev.qdev, 
>> req->bus->qbus.parent);
>> +    PVSCSIRequest *pvscsi_req = req->hba_private;
>> +
>> +    if (BTSTAT_SUCCESS == pvscsi_req->cmp.hostStatus) {
>> +        pvscsi_req->cmp.hostStatus = BTSTAT_ABORTQUEUE;
>> +    }
>> +    pvscsi_complete_request(s, pvscsi_req);
>> +}
>> +
>> +static inline SCSIDevice*
>> +pvscsi_device_find(PVSCSI_State *s, int channel, int target,
>> +                   uint8_t *requested_lun, uint8_t *target_lun)
>> +{
>> +    if (requested_lun[0] || requested_lun[2] || requested_lun[3] ||
>> +        requested_lun[4] || requested_lun[5] || requested_lun[6] ||
>> +        requested_lun[7] || (PVSCSI_MAX_DEVS < target)) {
>> +        return NULL;
>> +    } else {
>> +        *target_lun = requested_lun[1];
>> +        return scsi_device_find(&s->bus, channel, target, *target_lun);
>> +    }
>> +}
>> +
>> +static PVSCSIRequest *
>> +pvscsi_queue_pending_descriptor(PVSCSI_State *s, SCSIDevice **d,
>> +                                struct PVSCSIRingReqDesc *descr)
>> +{
>> +    PVSCSIRequest *pvscsi_req;
>> +    uint8_t lun;
>> +
>> +    pvscsi_req = g_malloc0(sizeof(*pvscsi_req));
>> +    pvscsi_req->req = *descr;
>> +    pvscsi_req->cmp.context = pvscsi_req->req.context;
>> +    QTAILQ_INSERT_TAIL(&s->pending_queue, pvscsi_req, next);
>> +
>> +    *d = pvscsi_device_find(s, descr->bus, descr->target, descr->lun, &lun);
>> +    if (!*d) {
>> +        return pvscsi_req;
>> +    }
>> +
>> +    pvscsi_req->lun = lun;
>> +    return pvscsi_req;
>> +}
>> +
>> +static void
>> +pvscsi_convert_sglist(PVSCSIRequest *r)
>> +{
>> +    int chunk_size;
>> +    uint64_t data_length = r->req.dataLen;
>> +    PVSCSISGState sg = r->sg;
>> +    while (data_length) {
>> +        while (!sg.resid) {
>> +            pvscsi_get_next_sg_elem(&sg);
>> +            DCMPRINTF("Element: ctx: %"PRIx64", addr: %"PRIx64", len: 
>> 0x%ul",
>> +                      r->req.context, (uint64_t) r->sg.dataAddr, 
>> r->sg.resid);
>
> TARGET_FMT_plx? Didn't check the type.
>
>> +        }
>> +        assert(data_length > 0);
>> +        chunk_size = MIN((unsigned) data_length, sg.resid);
>> +        if (chunk_size) {
>> +            qemu_sglist_add(&r->sgl, sg.dataAddr, chunk_size);
>> +        }
>> +
>> +        sg.dataAddr += chunk_size;
>> +        data_length -= chunk_size;
>> +        sg.resid -= chunk_size;
>> +    }
>> +}
>> +
>> +static inline void
>> +pvscsi_build_sglist(PVSCSIRequest *r)
>> +{
>> +    qemu_sglist_init(&r->sgl, 1, NULL);
>> +    if (r->req.flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
>> +        pvscsi_convert_sglist(r);
>> +    } else {
>> +        qemu_sglist_add(&r->sgl, r->req.dataAddr, r->req.dataLen);
>> +    }
>> +}
>> +
>> +static void
>> +pvscsi_process_request_descriptor(PVSCSI_State *s,
>> +                                  struct PVSCSIRingReqDesc *descr)
>> +{
>> +    SCSIDevice *d;
>> +    PVSCSIRequest *r = pvscsi_queue_pending_descriptor(s, &d, descr);
>> +    int64_t n;
>> +
>> +    DSXPRINTF("SCSI cmd 0x%X, ctx: %" PRIx64, descr->cdb[0], 
>> descr->context);
>> +
>> +    if (!d) {
>> +        r->cmp.hostStatus = BTSTAT_SELTIMEO;
>> +        DSXPRINTF("Command directed to unknown device rejected.");
>> +        pvscsi_complete_request(s, r);
>> +        return;
>> +    }
>> +
>> +    if (descr->flags & PVSCSI_FLAG_CMD_WITH_SG_LIST) {
>> +        r->sg.elemAddr = descr->dataAddr;
>> +    }
>> +
>> +    r->sreq = scsi_req_new(d, descr->context, r->lun, descr->cdb, r);
>> +    if (r->sreq->cmd.mode == SCSI_XFER_FROM_DEV &&
>> +        (descr->flags & PVSCSI_FLAG_CMD_DIR_TODEVICE)) {
>> +        r->cmp.hostStatus = BTSTAT_BADMSG;
>> +        DSXPRINTF("Command with invalid transfer direction rejected.");
>> +        scsi_req_cancel(r->sreq);
>> +        return;
>> +    }
>> +    if (r->sreq->cmd.mode == SCSI_XFER_TO_DEV &&
>> +        (descr->flags & PVSCSI_FLAG_CMD_DIR_TOHOST)) {
>> +        r->cmp.hostStatus = BTSTAT_BADMSG;
>> +        DSXPRINTF("Command with invalid transfer direction rejected.");
>> +        scsi_req_cancel(r->sreq);
>> +        return;
>> +    }
>> +
>> +    pvscsi_build_sglist(r);
>> +    n = scsi_req_enqueue(r->sreq);
>> +
>> +    if (n) {
>> +        scsi_req_continue(r->sreq);
>> +    }
>> +}
>> +
>> +static void
>> +pvscsi_process_io(PVSCSI_State *s)
>> +{
>> +    PVSCSIRingReqDesc descr;
>> +    target_phys_addr_t next_descr_pa;
>> +
>> +    assert(s->rings_info_valid);
>> +    while (0 != (next_descr_pa = 
>> pvscsi_rings_mgr_pop_req_descr(&s->rings))) {
>
> Order
>
>> +        DRIPRINTF("Got descriptor %"PRIx64, next_descr_pa);
>> +        vmw_shmem_read(next_descr_pa, &descr, sizeof(descr));
>> +        pvscsi_process_request_descriptor(s, &descr);
>> +    }
>> +
>> +    pvscsi_rings_mgr_flush_req_ring(&s->rings);
>> +}
>> +
>> +static void
>> +pvscsi_dbg_dump_tx_rings_config(struct PVSCSICmdDescSetupRings *rc)
>> +{
>> +#ifdef DEBUG_PVSCSI_RINGS
>> +    int i;
>> +    DRIPRINTF("TX Rings configuration received:");
>
> Since the code is surrounded by #ifdeffery, you can just use fprintf.
> With tracepoints, the #ifdeffery could be removed.
>
>> +    DRIPRINTF("Rings state page: %"PRIx64":", rc->ringsStatePPN);
>> +
>> +    DRIPRINTF("Request ring pages: %u:", rc->reqRingNumPages);
>> +    for (i = 0; i < rc->reqRingNumPages; i++) {
>> +        DRIPRINTF("\t%"PRIx64" ", rc->reqRingPPNs[i]);
>> +    }
>> +
>> +    DRIPRINTF("Confirm ring pages: %u:", rc->cmpRingNumPages);
>> +    for (i = 0; i < rc->cmpRingNumPages; i++) {
>> +        DRIPRINTF("\t%"PRIx64" ", rc->cmpRingPPNs[i]);
>> +    }
>> +#endif
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_config(PVSCSI_State *s)
>> +{
>> +    DWRPRINTF("Unimplemented command PVSCSI_CMD_CONFIG ignored.");
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_unplug(PVSCSI_State *s)
>> +{
>> +    DWRPRINTF("Unimplemented command PVSCSI_CMD_DEVICE_UNPLUG ignored.");
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_issue_scsi(PVSCSI_State *s)
>> +{
>> +    DWRPRINTF("Unimplemented command PVSCSI_CMD_ISSUE_SCSI ignored.");
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_setup_rings(PVSCSI_State *s)
>> +{
>> +    struct PVSCSICmdDescSetupRings *rc =
>> +        (struct PVSCSICmdDescSetupRings *) s->curr_cmd_data;
>> +
>> +    DCMPRINTF("Command PVSCSI_CMD_SETUP_RINGS arrived");
>> +
>> +    pvscsi_dbg_dump_tx_rings_config(rc);
>> +    pvscsi_rings_mgr_init(&s->rings, rc);
>> +    s->rings_info_valid = TRUE;
>> +    return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_abort(PVSCSI_State *s)
>> +{
>> +#ifdef DEBUG_PVSCSI_COMMANDS
>> +    struct PVSCSICmdDescAbortCmd *data =
>> +        (struct PVSCSICmdDescAbortCmd *) s->curr_cmd_data;
>> +
>> +    DCMPRINTF("PVSCSI_CMD_ABORT_CMD for ctx %"PRIx64", target 0x%X",
>> +              data->context, data->target);
>> +#endif
>> +
>> +    return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_unknown(PVSCSI_State *s)
>> +{
>> +    DWRPRINTF("Data for unknown command : 0x%X", s->curr_cmd_data[0]);
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_reset_device(PVSCSI_State *s)
>> +{
>> +    uint8_t target_lun = 0;
>> +    struct PVSCSICmdDescResetDevice *cmd =
>> +        (struct PVSCSICmdDescResetDevice *) s->curr_cmd_data;
>> +    SCSIDevice *sdev;
>> +
>> +    sdev = pvscsi_device_find(s, 0, cmd->target, cmd->lun, &target_lun);
>> +
>> +    DWRPRINTF("PVSCSI_CMD_RESET_DEVICE[target 0x%u lun 0x%d (dev 0x%p)]",
>> +              cmd->target, (int) target_lun, sdev);
>> +
>> +    if (sdev != NULL) {
>> +        device_reset(&sdev->qdev);
>> +        return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +    }
>> +
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_reset_bus(PVSCSI_State *s)
>> +{
>> +    DCMPRINTF("Command PVSCSI_CMD_RESET_BUS arrived");
>> +
>> +    qbus_reset_all_fn(&s->bus);
>> +    return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_setup_msg_ring(PVSCSI_State *s)
>> +{
>> +    /* No message ring needed for HDD devices */
>> +    DCMPRINTF("Command PVSCSI_CMD_SETUP_MSG_RING arrived");
>> +    return PVSCSI_COMMAND_PROCESSING_FAILED;
>> +}
>> +
>> +static uint64_t
>> +pvscsi_on_cmd_adapter_reset(PVSCSI_State *s)
>> +{
>> +    /* No message ring needed for HDD devices */
>> +    DCMPRINTF("Command PVSCSI_CMD_ADAPTER_RESET arrived");
>> +
>> +    pvscsi_reset_adapter(s);
>> +    return PVSCSI_COMMAND_PROCESSING_SUCCEEDED;
>> +}
>> +
>> +struct {
>
> 'static const'
>
>> +    int       data_size;
>> +    uint64_t  (*handler_fn)(PVSCSI_State *s);
>> +} pvscsi_commands[] = {
>> +    [PVSCSI_CMD_FIRST] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_cmd_unknown
>
> Comma at the end of line is more friendly for future patches.
>
>> +    },
>> +
>> +    /* Not implemented, data size defined based on what arrives on windows 
>> */
>> +    [PVSCSI_CMD_CONFIG] = {
>> +        .data_size = 6*sizeof(uint32_t),
>
> Missing spaces around '*'. Please use checkpatch.pl to catch these.
>
>> +        .handler_fn = pvscsi_on_cmd_config
>> +    },
>> +
>> +    /* Command not implemented, data size is unknown */
>> +    [PVSCSI_CMD_ISSUE_SCSI] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_issue_scsi
>> +    },
>> +
>> +    /* Command not implemented, data size is unknown */
>> +    [PVSCSI_CMD_DEVICE_UNPLUG] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_cmd_unplug
>> +    },
>> +
>> +    [PVSCSI_CMD_SETUP_RINGS] = {
>> +        .data_size = sizeof(struct PVSCSICmdDescSetupRings),
>> +        .handler_fn = pvscsi_on_cmd_setup_rings
>> +    },
>> +
>> +    [PVSCSI_CMD_RESET_DEVICE] = {
>> +        .data_size = sizeof(struct PVSCSICmdDescResetDevice),
>> +        .handler_fn = pvscsi_on_cmd_reset_device
>> +    },
>> +
>> +    [PVSCSI_CMD_RESET_BUS] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_cmd_reset_bus
>> +    },
>> +
>> +    [PVSCSI_CMD_SETUP_MSG_RING] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_cmd_setup_msg_ring
>> +    },
>> +
>> +    [PVSCSI_CMD_ADAPTER_RESET] = {
>> +        .data_size = 0,
>> +        .handler_fn = pvscsi_on_cmd_adapter_reset
>> +    },
>> +
>> +    [PVSCSI_CMD_ABORT_CMD] = {
>> +        .data_size = sizeof(struct PVSCSICmdDescAbortCmd),
>> +        .handler_fn = pvscsi_on_cmd_abort
>> +    }
>> +};
>> +
>> +static void
>> +pvscsi_do_command_processing(PVSCSI_State *s)
>> +{
>> +    size_t bytes_arrived = s->curr_cmd_data_cntr*sizeof(uint32_t);
>
> Spacing.
>
>> +
>> +    if (bytes_arrived >= pvscsi_commands[s->curr_cmd].data_size) {
>> +        s->reg_command_status = pvscsi_commands[s->curr_cmd].handler_fn(s);
>> +        s->curr_cmd = PVSCSI_CMD_FIRST;
>> +        s->curr_cmd_data_cntr   = 0;
>> +    }
>> +}
>> +
>> +static void
>> +pvscsi_on_command_data(PVSCSI_State *s, uint32_t value)
>> +{
>> +    size_t bytes_arrived = s->curr_cmd_data_cntr*sizeof(uint32_t);
>> +
>> +    assert(bytes_arrived < sizeof(s->curr_cmd_data));
>> +    s->curr_cmd_data[s->curr_cmd_data_cntr++] = value;
>> +
>> +    pvscsi_do_command_processing(s);
>> +}
>> +
>> +static void
>> +pvscsi_on_command(PVSCSI_State *s, uint64_t cmd_id)
>> +{
>> +    if ((cmd_id > PVSCSI_CMD_FIRST) && (cmd_id < PVSCSI_CMD_LAST)) {
>> +        s->curr_cmd = cmd_id;
>> +    } else {
>> +        s->curr_cmd = PVSCSI_CMD_FIRST;
>> +        DWRPRINTF("Unknown command %"PRIx64, cmd_id);
>> +    }
>> +
>> +    s->curr_cmd_data_cntr = 0;
>> +    s->reg_command_status = PVSCSI_COMMAND_PROCESSING_FAILED;
>> +
>> +    pvscsi_do_command_processing(s);
>> +}
>> +
>> +static void
>> +pvscsi_io_write(void *opaque, target_phys_addr_t addr,
>> +                uint64_t val, unsigned size)
>> +{
>> +    PVSCSI_State *s = opaque;
>> +
>> +    switch (addr) {
>> +    case PVSCSI_REG_OFFSET_COMMAND:
>> +        pvscsi_on_command(s, val);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_COMMAND_DATA:
>> +        pvscsi_on_command_data(s, (uint32_t) val);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_INTR_STATUS:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_INTR_STATUS write: %"PRIx64, val);
>> +        s->reg_interrupt_status &= ~val;
>> +        pvscsi_update_irq_status(s);
>> +        pvscsi_schedule_completion_processing(s);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_INTR_MASK:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_INTR_MASK write: %"PRIx64, val);
>> +        s->reg_interrupt_enabled = val;
>> +        pvscsi_update_irq_status(s);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_KICK_NON_RW_IO:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_KICK_NON_RW_IO write: %"PRIx64, val);
>> +        pvscsi_process_io(s);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_KICK_RW_IO:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_KICK_RW_IO write: %"PRIx64, val);
>> +        pvscsi_process_io(s);
>> +        break;
>> +
>> +    case PVSCSI_REG_OFFSET_DEBUG:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_DEBUG write: %"PRIx64, val);
>> +        break;
>> +
>> +    default:
>> +        DWRPRINTF("Unknown write: %" PRIx64 "%u bytes, val %" PRIx64,
>> +                  (uint64_t) addr, size, val);
>
> TARGET_FMT_plx
>
>> +        break;
>> +    }
>> +
>> +}
>> +
>> +static uint64_t
>> +pvscsi_io_read(void *opaque, target_phys_addr_t addr, unsigned size)
>> +{
>> +    PVSCSI_State *s = opaque;
>> +
>> +    switch (addr) {
>> +    case PVSCSI_REG_OFFSET_INTR_STATUS:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_INTR_STATUS read: %"PRIx64,
>> +                  s->reg_interrupt_status);
>> +        return s->reg_interrupt_status;
>> +
>> +    case PVSCSI_REG_OFFSET_INTR_MASK:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_INTR_MASK read: %"PRIx64,
>> +                  s->reg_interrupt_enabled);
>> +        return s->reg_interrupt_enabled;
>> +
>> +    case PVSCSI_REG_OFFSET_COMMAND_STATUS:
>> +        DCBPRINTF("PVSCSI_REG_OFFSET_COMMAND_STATUS read: %"PRIx64,
>> +                  s->reg_command_status);
>> +        return s->reg_command_status;
>> +
>> +    default:
>> +        DWRPRINTF("Unknown read, address %" PRIx64 ", %d bytes",
>> +                  (uint64_t) addr, size);
>
> Ditto
>
>> +        return 0;
>> +    }
>> +}
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +static bool
>> +pvscsi_init_msix(PVSCSI_State *s)
>> +{
>> +    int res;
>> +    memory_region_init(&s->msix_space, "pvscsi-msix", PAGE_SIZE);
>> +    res = msix_init(&s->dev, PVSCSI_MSIX_NUM_VECTORS, &s->msix_space, 1, 0);
>> +    if (0 > res) {
>
> Unnatural order again, also below.
>
>> +        DWRPRINTF("Failed to initialize MSI-X, error %d", res);
>> +        memory_region_destroy(&s->msix_space);
>> +        s->msix_used = false;
>> +    } else {
>> +        res = msix_vector_use(&s->dev, PVSCSI_VECTOR_COMPLETION);
>> +        if (0 > res) {
>> +            DWRPRINTF("Failed to use MSI-X vector, error %d", res);
>> +            msix_uninit(&s->dev, &s->msix_space);
>> +            memory_region_destroy(&s->msix_space);
>> +            s->msix_used = false;
>> +        } else {
>> +            pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
>> +                             &s->msix_space);
>> +            s->msix_used = true;
>> +        }
>> +    }
>
> How about moving the #ifdef lower and add here
> #else
>   s->msix_used = false;
> #endif
>
>> +    return s->msix_used;
>> +}
>> +
>> +static void
>> +pvscsi_cleanup_msix(PVSCSI_State *s)
>> +{
>
> and here #ifdef to make the function empty on !msix?
>
>> +    if (s->msix_used) {
>> +        msix_vector_unuse(&s->dev, PVSCSI_VECTOR_COMPLETION);
>> +        msix_uninit(&s->dev, &s->msix_space);
>> +        memory_region_destroy(&s->msix_space);
>> +    }
>> +}
>> +#endif
>> +
>> +static bool
>> +pvscsi_init_msi(PVSCSI_State *s)
>> +{
>> +#define PVSCSI_MSI_OFFSET        (0x50)
>> +#define PVSCSI_USE_64BIT         (true)
>> +#define PVSCSI_PER_VECTOR_MASK   (false)
>
> #defines should go to top of the file.
>
>> +
>> +    int res;
>> +    res = msi_init(&s->dev, PVSCSI_MSI_OFFSET, PVSCSI_MSIX_NUM_VECTORS,
>> +                   PVSCSI_USE_64BIT, PVSCSI_PER_VECTOR_MASK);
>> +    if (0 > res) {
>
> Order
>
>> +        DWRPRINTF("Failed to initialize MSI, error %d", res);
>> +        s->msi_used = false;
>> +    } else {
>> +        s->msi_used = true;
>> +    }
>> +
>> +    return s->msi_used;
>> +}
>> +
>> +static void
>> +pvscsi_cleanup_msi(PVSCSI_State *s)
>> +{
>> +    if (s->msi_used) {
>> +        msi_uninit(&s->dev);
>> +    }
>> +}
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +
>> +static void
>> +pvscsi_msix_save(QEMUFile *f, void *opaque)
>> +{
>> +    msix_save(&((PVSCSI_State *)opaque)->dev, f);
>> +}
>> +
>> +static int
>> +pvscsi_msix_load(QEMUFile *f, void *opaque, int version_id)
>> +{
>> +    msix_load(&((PVSCSI_State *)opaque)->dev, f);
>> +    return 0;
>> +}
>> +
>> +#endif
>> +
>> +static int
>> +pvscsi_init(PCIDevice *dev)
>> +{
>> +    static const MemoryRegionOps pv_scsi_ops = {
>> +            .read = pvscsi_io_read,
>> +            .write = pvscsi_io_write,
>> +            .endianness = DEVICE_NATIVE_ENDIAN,
>> +            .impl = {
>> +                    .min_access_size = 4,
>> +                    .max_access_size = 4,
>> +            },
>> +    };
>> +
>> +    static const struct SCSIBusInfo pvscsi_scsi_info = {
>> +            .tcq = true,
>> +            .max_target = PVSCSI_MAX_DEVS,
>> +            .max_channel = 0,
>> +            .max_lun = 0,
>> +
>> +            .get_sg_list = pvscsi_get_sg_list,
>> +            .complete = pvscsi_command_complete,
>> +            .cancel = pvscsi_request_cancelled
>
> comma
>
>> +    };
>> +
>> +    PVSCSI_State *s = DO_UPCAST(PVSCSI_State, dev, dev);
>> +
>> +    DCBPRINTF("Starting init...");
>> +
>> +    /* PCI subsystem ID */
>> +    s->dev.config[PCI_SUBSYSTEM_ID] = 0x00;
>> +    s->dev.config[PCI_SUBSYSTEM_ID + 1] = 0x10;
>> +
>> +    /* PCI latency timer = 255 */
>> +    s->dev.config[PCI_LATENCY_TIMER] = 0xff;
>> +
>> +    /* Interrupt pin A */
>> +    s->dev.config[PCI_INTERRUPT_PIN] = 0x01;
>> +
>> +    memory_region_init_io(&s->io_space, &pv_scsi_ops, s,
>> +                          "pvscsi-io", PVSCSI_MEM_SPACE_SIZE);
>> +    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, 
>> &s->io_space);
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +    register_savevm(&dev->qdev, "pvscsi-msix", -1, 1,
>> +                    pvscsi_msix_save, pvscsi_msix_load, s);
>> +
>> +    if (!pvscsi_init_msix(s)) {
>> +        DWRPRINTF("Failed to initialize MSI-X.");
>> +    }
>> +#endif
>
> With the adjusted #ifdeffery for the functions themselves, this code
> could be enabled always.
>
>> +    if (!pvscsi_init_msi(s)) {
>> +        DWRPRINTF("Failed to initialize MSI.");
>> +    }
>> +
>> +    s->completion_worker = qemu_bh_new(pvscsi_process_completion_queue, s);
>> +    if (!s->completion_worker) {
>> +        pvscsi_cleanup_msi(s);
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +        pvscsi_cleanup_msix(s);
>> +#endif
>> +        memory_region_destroy(&s->io_space);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    scsi_bus_new(&s->bus, &dev->qdev, &pvscsi_scsi_info);
>> +    pvscsi_reset_state(s);
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +pvscsi_uninit(PCIDevice *dev)
>> +{
>> +    PVSCSI_State *s = DO_UPCAST(PVSCSI_State, dev, dev);
>> +
>> +    DCBPRINTF("Starting uninit...");
>> +    qemu_bh_delete(s->completion_worker);
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +    unregister_savevm(&dev->qdev, "pvscsi-msix", s);
>> +    pvscsi_cleanup_msix(s);
>> +#endif
>> +    pvscsi_cleanup_msi(s);
>> +
>> +    memory_region_destroy(&s->io_space);
>> +
>> +    return 0;
>> +}
>> +
>> +static void
>> +pvscsi_reset(DeviceState *dev)
>> +{
>> +    PVSCSI_State *s = DO_UPCAST(PVSCSI_State, dev.qdev, dev);
>> +    DCBPRINTF("Starting reset...");
>> +    pvscsi_reset_adapter(s);
>> +}
>> +
>> +static void
>> +pvscsi_pre_save(void *opaque)
>> +{
>> +    PVSCSI_State *s = (PVSCSI_State *) opaque;
>> +
>> +    DCBPRINTF("Starting presave...");
>> +
>> +    assert(QTAILQ_EMPTY(&s->pending_queue));
>> +    assert(QTAILQ_EMPTY(&s->completion_queue));
>> +}
>> +
>> +static int
>> +pvscsi_post_load(void *opaque, int version_id)
>> +{
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +    PVSCSI_State *s = (PVSCSI_State *) opaque;
>
> Useless cast.
>
>> +#endif
>> +    DCBPRINTF("Starting postload...");
>> +
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +    if (s->msix_used) {
>> +        if (!msix_vector_use(&s->dev, PVSCSI_VECTOR_COMPLETION)) {
>> +            DERPRINTF("Failed to re-use MSI-X vectors");
>> +            return -1;
>> +        }
>> +    }
>> +#endif
>> +    return 0;
>> +}
>> +
>> +static VMStateDescription vmstate_pvscsi = {
>> +        .name = "pvscsi",
>> +        .version_id = 0,
>> +        .minimum_version_id = 0,
>> +        .minimum_version_id_old = 0,
>> +        .pre_save = pvscsi_pre_save,
>> +        .post_load = pvscsi_post_load,
>> +        .fields      = (VMStateField[]) {
>> +                VMSTATE_PCI_DEVICE(dev, PVSCSI_State),
>> +#ifdef PVSCSI_ENABLE_MSIX
>> +                VMSTATE_UINT8(msix_used, PVSCSI_State),
>> +#endif
>> +                VMSTATE_UINT8(msi_used, PVSCSI_State),
>> +                VMSTATE_UINT64(reg_interrupt_status, PVSCSI_State),
>> +                VMSTATE_UINT64(reg_interrupt_enabled, PVSCSI_State),
>> +                VMSTATE_UINT64(reg_command_status, PVSCSI_State),
>> +                VMSTATE_UINT64(curr_cmd, PVSCSI_State),
>> +                VMSTATE_UINT32(curr_cmd_data_cntr, PVSCSI_State),
>> +                VMSTATE_UINT32_ARRAY(curr_cmd_data, PVSCSI_State,
>> +                    ARRAY_SIZE(((PVSCSI_State *)NULL)->curr_cmd_data)),
>> +                VMSTATE_UINT8(rings_info_valid, PVSCSI_State),
>> +
>> +                VMSTATE_UINT64(rings.rs_pa, PVSCSI_State),
>> +                VMSTATE_UINT32(rings.txr_len_mask, PVSCSI_State),
>> +                VMSTATE_UINT32(rings.rxr_len_mask, PVSCSI_State),
>> +                VMSTATE_UINT64_ARRAY(rings.req_ring_pages_pa, PVSCSI_State,
>> +                    PVSCSI_SETUP_RINGS_MAX_NUM_PAGES),
>> +                VMSTATE_UINT64_ARRAY(rings.cmp_ring_pages_pa, PVSCSI_State,
>> +                    PVSCSI_SETUP_RINGS_MAX_NUM_PAGES),
>> +                VMSTATE_UINT64(rings.consumed_ptr, PVSCSI_State),
>> +                VMSTATE_UINT64(rings.filled_cmp_ptr, PVSCSI_State),
>> +
>> +                VMSTATE_END_OF_LIST()
>> +            }
>> +};
>> +
>> +static void
>> +pvscsi_write_config(PCIDevice *pci, uint32_t addr, uint32_t val, int len)
>> +{
>> +    pci_default_write_config(pci, addr, val, len);
>> +    msi_write_config(pci, addr, val, len);
>> +#if defined(PVSCSI_ENABLE_MSIX)
>> +    msix_write_config(pci, addr, val, len);
>> +#endif
>> +}
>> +
>> +static void pvscsi_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> +
>> +    k->init = pvscsi_init;
>> +    k->exit = pvscsi_uninit;
>> +    k->vendor_id = PCI_VENDOR_ID_VMWARE;
>> +    k->device_id = PCI_DEVICE_ID_VMWARE_PVSCSI;
>> +    k->class_id = PCI_CLASS_STORAGE_SCSI;
>> +    k->subsystem_id = 0x1000;
>> +    dc->reset = pvscsi_reset;
>> +    dc->vmsd = &vmstate_pvscsi;
>> +    k->config_write = pvscsi_write_config;
>> +}
>> +
>> +static TypeInfo pvscsi_info = {
>> +    .name          = "pvscsi",
>> +    .parent        = TYPE_PCI_DEVICE,
>> +    .instance_size = sizeof(PVSCSI_State),
>> +    .class_init    = pvscsi_class_init,
>> +};
>> +
>> +static void
>> +pvscsi_register_types(void)
>> +{
>> +    type_register_static(&pvscsi_info);
>> +
>> +    DCBPRINTF("PVSCSI QEMU device emulation registered");
>> +}
>> +
>> +type_init(pvscsi_register_types);
>> diff --git a/hw/pvscsi.h b/hw/pvscsi.h
>> new file mode 100644
>> index 0000000..611c549
>> --- /dev/null
>> +++ b/hw/pvscsi.h
>> @@ -0,0 +1,442 @@
>> +/*
>> + * QEMU VMWARE PVSCSI paravirtual SCSI bus
>> + *
>> + * Copyright (c) 2012 Ravello Systems LTD (http://ravellosystems.com)
>> + *
>> + * Developed by Daynix Computing LTD (http://www.daynix.com)
>> + *
>> + * Based on implementation by Paolo Bonzini
>> + * http://lists.gnu.org/archive/html/qemu-devel/2011-08/msg00729.html
>> + *
>> + * Authors:
>> + * Paolo Bonzini <address@hidden>
>> + * Dmitry Fleytman <address@hidden>
>> + * Yan Vugenfirer <address@hidden>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef _PVSCSI_H_
>> +#define _PVSCSI_H_
>
> Underscores
>
>> +
>> +#define PAGE_SIZE (4096)
>> +#define PAGE_SHIFT (12)
>
> These probably conflict with system defines, please rename. Also,
> define size using shift.
>
>> +
>> +#define PVSCSI_MAX_NUM_SG_ENTRIES_PER_SEGMENT 128
>> +
>> +#define MASK(n)        ((1 << (n)) - 1)        /* make an n-bit mask */
>> +
>> +/*
>> + * Following is an interface definition for
>> + * PVSCSI device as provided by VMWARE
>> + * See original copyright from Linux kernel v3.2.8
>> + * header file drivers/scsi/vmw_pvscsi.h below.
>> + */
>> +
>> + /*
>> +  * VMware PVSCSI header file
>> +  *
>> +  * Copyright (C) 2008-2009, VMware, Inc. All Rights Reserved.
>> +  *
>> +  * This program is free software; you can redistribute it and/or modify it
>> +  * under the terms of the GNU General Public License as published by the
>> +  * Free Software Foundation; version 2 of the License and no later version.
>
> GPLv2only conflicts with GPLv2+.

Indeed. Please note that this blurb was earlier copy-pasted from the
driver's code checked into the linux kernel (and it still seems to say
v2). I will update this with the latest maintainer's name and address.
However, I was wondering if the copyright blurb needs to be
copy-pasted here at all or is a reference to the source's location in
the linux kernel tree good enough?
>
>> +  *
>> +  * This program is distributed in the hope that it will be useful, but
>> +  * WITHOUT ANY WARRANTY; without even the implied warranty of
>> +  * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
>> +  * NON INFRINGEMENT.  See the GNU General Public License for more
>> +  * details.
>> +  *
>> +  * You should have received a copy of the GNU General Public License
>> +  * along with this program; if not, write to the Free Software
>> +  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 
>> USA.
>
> Web version, please.
>
>> +  *
>> +  * Maintained by: Alok N Kataria <address@hidden>
>
> <address@hidden>?
>
>> +  *
>> +  */
>> +
>> +/*
>> + * host adapter status/error codes
>> + */
>> +enum HostBusAdapterStatus {
>> +   BTSTAT_SUCCESS       = 0x00,  /* CCB complete normally with no errors */
>> +   BTSTAT_LINKED_COMMAND_COMPLETED           = 0x0a,
>> +   BTSTAT_LINKED_COMMAND_COMPLETED_WITH_FLAG = 0x0b,
>> +   BTSTAT_DATA_UNDERRUN = 0x0c,
>> +   BTSTAT_SELTIMEO      = 0x11,  /* SCSI selection timeout */
>> +   BTSTAT_DATARUN       = 0x12,  /* data overrun/underrun */
>> +   BTSTAT_BUSFREE       = 0x13,  /* unexpected bus free */
>> +   BTSTAT_INVPHASE      = 0x14,  /* invalid bus phase or sequence */
>> +                                 /* requested by target           */
>> +   BTSTAT_LUNMISMATCH   = 0x17,  /* linked CCB has different LUN  */
>> +                                 /* from first CCB                */
>> +   BTSTAT_SENSFAILED    = 0x1b,  /* auto request sense failed */
>> +   BTSTAT_TAGREJECT     = 0x1c,  /* SCSI II tagged queueing message */
>> +                                 /* rejected by target              */
>> +   BTSTAT_BADMSG        = 0x1d,  /* unsupported message received by */
>> +                                 /* the host adapter                */
>> +   BTSTAT_HAHARDWARE    = 0x20,  /* host adapter hardware failed */
>> +   BTSTAT_NORESPONSE    = 0x21,  /* target did not respond to SCSI ATN, */
>> +                                 /* sent a SCSI RST                     */
>> +   BTSTAT_SENTRST       = 0x22,  /* host adapter asserted a SCSI RST */
>> +   BTSTAT_RECVRST       = 0x23,  /* other SCSI devices asserted a SCSI RST 
>> */
>> +   BTSTAT_DISCONNECT    = 0x24,  /* target device reconnected improperly */
>> +                                 /* (w/o tag)                            */
>> +   BTSTAT_BUSRESET      = 0x25,  /* host adapter issued BUS device reset */
>> +   BTSTAT_ABORTQUEUE    = 0x26,  /* abort queue generated */
>> +   BTSTAT_HASOFTWARE    = 0x27,  /* host adapter software error */
>> +   BTSTAT_HATIMEOUT     = 0x30,  /* host adapter hardware timeout error */
>> +   BTSTAT_SCSIPARITY    = 0x34,  /* SCSI parity error detected */
>> +};
>> +
>> +/*
>> + * Register offsets.
>> + *
>> + * These registers are accessible both via i/o space and mm i/o.
>> + */
>> +
>> +enum PVSCSIRegOffset {
>> +    PVSCSI_REG_OFFSET_COMMAND        =    0x0,
>> +    PVSCSI_REG_OFFSET_COMMAND_DATA   =    0x4,
>> +    PVSCSI_REG_OFFSET_COMMAND_STATUS =    0x8,
>> +    PVSCSI_REG_OFFSET_LAST_STS_0     =  0x100,
>> +    PVSCSI_REG_OFFSET_LAST_STS_1     =  0x104,
>> +    PVSCSI_REG_OFFSET_LAST_STS_2     =  0x108,
>> +    PVSCSI_REG_OFFSET_LAST_STS_3     =  0x10c,
>> +    PVSCSI_REG_OFFSET_INTR_STATUS    = 0x100c,
>> +    PVSCSI_REG_OFFSET_INTR_MASK      = 0x2010,
>> +    PVSCSI_REG_OFFSET_KICK_NON_RW_IO = 0x3014,
>> +    PVSCSI_REG_OFFSET_DEBUG          = 0x3018,
>> +    PVSCSI_REG_OFFSET_KICK_RW_IO     = 0x4018,
>> +};
>> +
>> +/*
>> + * Virtual h/w commands.
>> + */
>> +
>> +enum PVSCSICommands {
>> +    PVSCSI_CMD_FIRST             = 0, /* has to be first */
>> +
>> +    PVSCSI_CMD_ADAPTER_RESET     = 1,
>> +    PVSCSI_CMD_ISSUE_SCSI        = 2,
>> +    PVSCSI_CMD_SETUP_RINGS       = 3,
>> +    PVSCSI_CMD_RESET_BUS         = 4,
>> +    PVSCSI_CMD_RESET_DEVICE      = 5,
>> +    PVSCSI_CMD_ABORT_CMD         = 6,
>> +    PVSCSI_CMD_CONFIG            = 7,
>> +    PVSCSI_CMD_SETUP_MSG_RING    = 8,
>> +    PVSCSI_CMD_DEVICE_UNPLUG     = 9,
>> +
>> +    PVSCSI_CMD_LAST              = 10  /* has to be last */
>> +};
>> +
>> +#define PVSCSI_COMMAND_PROCESSING_SUCCEEDED   (0)
>> +#define PVSCSI_COMMAND_PROCESSING_FAILED     (-1)
>> +
>> +/*
>> + * Command descriptor for PVSCSI_CMD_RESET_DEVICE --
>> + */
>> +
>> +struct PVSCSICmdDescResetDevice {
>> +    uint32_t    target;
>> +    uint8_t     lun[8];
>> +} QEMU_PACKED;
>
> Typedefs missing (also below), please add.
>
>> +
>> +/*
>> + * Command descriptor for PVSCSI_CMD_ABORT_CMD --
>> + *
>> + * - currently does not support specifying the LUN.
>> + * - _pad should be 0.
>> + */
>> +
>> +struct PVSCSICmdDescAbortCmd {
>> +    uint64_t    context;
>> +    uint32_t    target;
>> +    uint32_t    _pad;
>> +} QEMU_PACKED;
>> +
>> +/*
>> + * Command descriptor for PVSCSI_CMD_SETUP_RINGS --
>> + *
>> + * Notes:
>> + * - reqRingNumPages and cmpRingNumPages need to be power of two.
>> + * - reqRingNumPages and cmpRingNumPages need to be different from 0,
>> + * - reqRingNumPages and cmpRingNumPages need to be inferior to
>> + *   PVSCSI_SETUP_RINGS_MAX_NUM_PAGES.
>> + */
>> +
>> +#define PVSCSI_SETUP_RINGS_MAX_NUM_PAGES        32
>> +struct PVSCSICmdDescSetupRings {
>> +    uint32_t    reqRingNumPages;
>> +    uint32_t    cmpRingNumPages;
>> +    uint64_t    ringsStatePPN;
>> +    uint64_t    reqRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
>> +    uint64_t    cmpRingPPNs[PVSCSI_SETUP_RINGS_MAX_NUM_PAGES];
>> +} QEMU_PACKED;
>> +
>> +/*
>> + * Command descriptor for PVSCSI_CMD_SETUP_MSG_RING --
>> + *
>> + * Notes:
>> + * - this command was not supported in the initial revision of the h/w
>> + *   interface. Before using it, you need to check that it is supported by
>> + *   writing PVSCSI_CMD_SETUP_MSG_RING to the 'command' register, then
>> + *   immediately after read the 'command status' register:
>> + *       * a value of -1 means that the cmd is NOT supported,
>> + *       * a value != -1 means that the cmd IS supported.
>> + *   If it's supported the 'command status' register should return:
>> + *      sizeof(PVSCSICmdDescSetupMsgRing) / sizeof(uint32_t).
>> + * - this command should be issued _after_ the usual SETUP_RINGS so that the
>> + *   RingsState page is already setup. If not, the command is a nop.
>> + * - numPages needs to be a power of two,
>> + * - numPages needs to be different from 0,
>> + * - _pad should be zero.
>> + */
>> +
>> +#define PVSCSI_SETUP_MSG_RING_MAX_NUM_PAGES  16
>> +
>> +struct PVSCSICmdDescSetupMsgRing {
>> +    uint32_t    numPages;
>> +    uint32_t    _pad;
>
> Underscore
>
>> +    uint64_t    ringPPNs[PVSCSI_SETUP_MSG_RING_MAX_NUM_PAGES];
>> +} QEMU_PACKED;
>> +
>> +enum PVSCSIMsgType {
>> +    PVSCSI_MSG_DEV_ADDED          = 0,
>> +    PVSCSI_MSG_DEV_REMOVED        = 1,
>> +    PVSCSI_MSG_LAST               = 2,
>> +};
>> +
>> +/*
>> + * Msg descriptor.
>> + *
>> + * sizeof(struct PVSCSIRingMsgDesc) == 128.
>> + *
>> + * - type is of type enum PVSCSIMsgType.
>> + * - the content of args depend on the type of event being delivered.
>> + */
>> +
>> +struct PVSCSIRingMsgDesc {
>> +    uint32_t    type;
>> +    uint32_t    args[31];
>> +} QEMU_PACKED;
>> +
>> +struct PVSCSIMsgDescDevStatusChanged {
>> +    uint32_t    type;  /* PVSCSI_MSG_DEV _ADDED / _REMOVED */
>> +    uint32_t    bus;
>> +    uint32_t    target;
>> +    uint8_t     lun[8];
>> +    uint32_t    pad[27];
>> +} QEMU_PACKED;
>> +
>> +/*
>> + * Rings state.
>> + *
>> + * - the fields:
>> + *    . msgProdIdx,
>> + *    . msgConsIdx,
>> + *    . msgNumEntriesLog2,
>> + *   .. are only used once the SETUP_MSG_RING cmd has been issued.
>> + * - '_pad' helps to ensure that the msg related fields are on their own
>> + *   cache-line.
>> + */
>> +
>> +struct PVSCSIRingsState {
>> +    uint32_t    reqProdIdx;
>> +    uint32_t    reqConsIdx;
>> +    uint32_t    reqNumEntriesLog2;
>> +
>> +    uint32_t    cmpProdIdx;
>> +    uint32_t    cmpConsIdx;
>> +    uint32_t    cmpNumEntriesLog2;
>> +
>> +    uint8_t     _pad[104];
>
> Underscore.
>
>> +
>> +    uint32_t    msgProdIdx;
>> +    uint32_t    msgConsIdx;
>> +    uint32_t    msgNumEntriesLog2;
>> +} QEMU_PACKED;
>> +
>> +/*
>> + * Request descriptor.
>> + *
>> + * sizeof(RingReqDesc) = 128
>> + *
>> + * - context: is a unique identifier of a command. It could normally be any
>> + *   64bit value, however we currently store it in the serialNumber variable
>> + *   of struct SCSI_Command, so we have the following restrictions due to 
>> the
>> + *   way this field is handled in the vmkernel storage stack:
>> + *    * this value can't be 0,
>> + *    * the upper 32bit need to be 0 since serialNumber is as a uint32_t.
>> + *   Currently tracked as PR 292060.
>> + * - dataLen: contains the total number of bytes that need to be 
>> transferred.
>> + * - dataAddr:
>> + *   * if PVSCSI_FLAG_CMD_WITH_SG_LIST is set: dataAddr is the PA of the 
>> first
>> + *     s/g table segment, each s/g segment is entirely contained on a single
>> + *     page of physical memory,
>> + *   * if PVSCSI_FLAG_CMD_WITH_SG_LIST is NOT set, then dataAddr is the PA 
>> of
>> + *     the buffer used for the DMA transfer,
>> + * - flags:
>> + *   * PVSCSI_FLAG_CMD_WITH_SG_LIST: see dataAddr above,
>> + *   * PVSCSI_FLAG_CMD_DIR_NONE: no DMA involved,
>> + *   * PVSCSI_FLAG_CMD_DIR_TOHOST: transfer from device to main memory,
>> + *   * PVSCSI_FLAG_CMD_DIR_TODEVICE: transfer from main memory to device,
>> + *   * PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB: reserved to handle CDBs larger than
>> + *     16bytes. To be specified.
>> + * - vcpuHint: vcpuId of the processor that will be most likely waiting for 
>> the
>> + *   completion of the i/o. For guest OSes that use lowest priority message
>> + *   delivery mode (such as windows), we use this "hint" to deliver the
>> + *   completion action to the proper vcpu. For now, we can use the vcpuId of
>> + *   the processor that initiated the i/o as a likely candidate for the vcpu
>> + *   that will be waiting for the completion..
>> + * - bus should be 0: we currently only support bus 0 for now.
>> + * - unused should be zero'd.
>> + */
>> +
>> +#define PVSCSI_FLAG_CMD_WITH_SG_LIST        (1 << 0)
>> +#define PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB     (1 << 1)
>> +#define PVSCSI_FLAG_CMD_DIR_NONE            (1 << 2)
>> +#define PVSCSI_FLAG_CMD_DIR_TOHOST          (1 << 3)
>> +#define PVSCSI_FLAG_CMD_DIR_TODEVICE        (1 << 4)
>> +
>> +#define PVSCSI_KNOWN_FLAGS \
>> +  (PVSCSI_FLAG_CMD_WITH_SG_LIST     | \
>> +   PVSCSI_FLAG_CMD_OUT_OF_BAND_CDB  | \
>> +   PVSCSI_FLAG_CMD_DIR_NONE         | \
>> +   PVSCSI_FLAG_CMD_DIR_TOHOST       | \
>> +   PVSCSI_FLAG_CMD_DIR_TODEVICE)
>> +
>> +struct PVSCSIRingReqDesc {
>> +    uint64_t    context;
>> +    uint64_t    dataAddr;
>> +    uint64_t    dataLen;
>> +    uint64_t    senseAddr;
>> +    uint32_t    senseLen;
>> +    uint32_t    flags;
>> +    uint8_t     cdb[16];
>> +    uint8_t     cdbLen;
>> +    uint8_t     lun[8];
>> +    uint8_t     tag;
>> +    uint8_t     bus;
>> +    uint8_t     target;
>> +    uint8_t     vcpuHint;
>> +    uint8_t     unused[59];
>> +} QEMU_PACKED;
>> +
>> +typedef struct PVSCSIRingReqDesc PVSCSIRingReqDesc;
>> +
>> +/*
>> + * Scatter-gather list management.
>> + *
>> + * As described above, when PVSCSI_FLAG_CMD_WITH_SG_LIST is set in the
>> + * RingReqDesc.flags, then RingReqDesc.dataAddr is the PA of the first s/g
>> + * table segment.
>> + *
>> + * - each segment of the s/g table contain a succession of struct
>> + *   PVSCSISGElement.
>> + * - each segment is entirely contained on a single physical page of memory.
>> + * - a "chain" s/g element has the flag PVSCSI_SGE_FLAG_CHAIN_ELEMENT set in
>> + *   PVSCSISGElement.flags and in this case:
>> + *     * addr is the PA of the next s/g segment,
>> + *     * length is undefined, assumed to be 0.
>> + */
>> +
>> +struct PVSCSISGElement {
>> +    uint64_t    addr;
>> +    uint32_t    length;
>> +    uint32_t    flags;
>> +} QEMU_PACKED;
>> +
>> +/*
>> + * Completion descriptor.
>> + *
>> + * sizeof(RingCmpDesc) = 32
>> + *
>> + * - context: identifier of the command. The same thing that was specified
>> + *   under "context" as part of struct RingReqDesc at initiation time,
>> + * - dataLen: number of bytes transferred for the actual i/o operation,
>> + * - senseLen: number of bytes written into the sense buffer,
>> + * - hostStatus: adapter status,
>> + * - scsiStatus: device status,
>> + * - _pad should be zero.
>> + */
>> +
>> +struct PVSCSIRingCmpDesc {
>> +    uint64_t    context;
>> +    uint64_t    dataLen;
>> +    uint32_t    senseLen;
>> +    uint16_t    hostStatus;
>> +    uint16_t    scsiStatus;
>> +    uint32_t    _pad[2];
>> +} QEMU_PACKED;
>> +typedef struct PVSCSIRingCmpDesc PVSCSIRingCmpDesc;
>> +
>> +/*
>> + * Interrupt status / IRQ bits.
>> + */
>> +
>> +#define PVSCSI_INTR_CMPL_0                 (1 << 0)
>> +#define PVSCSI_INTR_CMPL_1                 (1 << 1)
>> +#define PVSCSI_INTR_CMPL_MASK              MASK(2)
>> +
>> +#define PVSCSI_INTR_MSG_0                  (1 << 2)
>> +#define PVSCSI_INTR_MSG_1                  (1 << 3)
>> +#define PVSCSI_INTR_MSG_MASK               (MASK(2) << 2)
>> +
>> +#define PVSCSI_INTR_ALL_SUPPORTED          MASK(4)
>> +
>> +/*
>> + * Number of MSI-X vectors supported.
>> + */
>> +#define PVSCSI_MAX_INTRS        24
>> +
>> +/*
>> + * Enumeration of supported MSI-X vectors
>> + */
>> +#define PVSCSI_VECTOR_COMPLETION   0
>> +
>> +/*
>> + * Misc constants for the rings.
>> + */
>> +
>> +#define PVSCSI_MAX_NUM_PAGES_REQ_RING   PVSCSI_SETUP_RINGS_MAX_NUM_PAGES
>> +#define PVSCSI_MAX_NUM_PAGES_CMP_RING   PVSCSI_SETUP_RINGS_MAX_NUM_PAGES
>> +#define PVSCSI_MAX_NUM_PAGES_MSG_RING   PVSCSI_SETUP_MSG_RING_MAX_NUM_PAGES
>> +
>> +#define PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE \
>> +                (PAGE_SIZE / sizeof(struct PVSCSIRingReqDesc))
>> +
>> +#define PVSCSI_MAX_NUM_CMP_ENTRIES_PER_PAGE \
>> +                (PAGE_SIZE / sizeof(PVSCSIRingCmpDesc))
>> +
>> +#define PVSCSI_MAX_REQ_QUEUE_DEPTH \
>> +    (PVSCSI_MAX_NUM_PAGES_REQ_RING * PVSCSI_MAX_NUM_REQ_ENTRIES_PER_PAGE)
>> +
>> +#define PVSCSI_MEM_SPACE_COMMAND_NUM_PAGES     1
>> +#define PVSCSI_MEM_SPACE_INTR_STATUS_NUM_PAGES 1
>> +#define PVSCSI_MEM_SPACE_MISC_NUM_PAGES        2
>> +#define PVSCSI_MEM_SPACE_KICK_IO_NUM_PAGES     2
>> +#define PVSCSI_MEM_SPACE_MSIX_NUM_PAGES        2
>> +
>> +enum PVSCSIMemSpace {
>> +    PVSCSI_MEM_SPACE_COMMAND_PAGE       = 0,
>> +    PVSCSI_MEM_SPACE_INTR_STATUS_PAGE   = 1,
>> +    PVSCSI_MEM_SPACE_MISC_PAGE          = 2,
>> +    PVSCSI_MEM_SPACE_KICK_IO_PAGE       = 4,
>> +    PVSCSI_MEM_SPACE_MSIX_TABLE_PAGE    = 6,
>> +    PVSCSI_MEM_SPACE_MSIX_PBA_PAGE      = 7,
>> +};
>> +
>> +#define PVSCSI_MEM_SPACE_NUM_PAGES \
>> +    (PVSCSI_MEM_SPACE_COMMAND_NUM_PAGES +       \
>> +     PVSCSI_MEM_SPACE_INTR_STATUS_NUM_PAGES +   \
>> +     PVSCSI_MEM_SPACE_MISC_NUM_PAGES +          \
>> +     PVSCSI_MEM_SPACE_KICK_IO_NUM_PAGES +       \
>> +     PVSCSI_MEM_SPACE_MSIX_NUM_PAGES)
>> +
>> +#define PVSCSI_MEM_SPACE_SIZE        (PVSCSI_MEM_SPACE_NUM_PAGES * 
>> PAGE_SIZE)
>> +
>> +#endif /* _VMW_PVSCSI_H_ */
>> --
>> 1.7.9.5
>>

Thanks for the detailed review. I will address all the rest of the
items pointed out in the next version.

Thanks,
Deep



reply via email to

[Prev in Thread] Current Thread [Next in Thread]