[Qemu-devel] [PATCH 0/8] Some more AHCI work

[Qemu-devel] [PATCH 0/8] Some more AHCI work

[Alexander Graf]

Clearly, AHCI as is is not perfect yet (intentionally, release early,
release often, remember?). This patch set makes it work with SeaBIOS
so booting Windows 7 works flawlessly for me. it also adds some speedups
and fixes a level based interrupts, rendering ahci useful on PPC targets.

In preparation of potential non-ich9 implementations, this set also
splits ahci code from concrete ich9 specific code. That way we can
later on create other AHCI adapters while reusing a lot of code.

Git tree (including BIOS patch to enable booting from AHCI):

 git://repo.or.cz/qemu/ahci.git ahci

Alexander Graf (7):
  ahci: split ICH and AHCI even more
  ahci: send init d2h fis on fis enable
  ahci: use qiov instead of dma helpers
  ahci: Implement HBA reset
  ahci: make number of ports runtime determined
  ahci: free dynamically allocated iovs
  ahci: fix !msi interrupts

Sebastian Herbszt (1):
  ahci: split ICH9 from core

 Makefile.objs |    1 +
 hw/ide/ahci.c |  594 +++++++++++++++------------------------------------------
 hw/ide/ahci.h |  313 ++++++++++++++++++++++++++++++
 hw/ide/ich.c  |  148 ++++++++++++++
 4 files changed, 618 insertions(+), 438 deletions(-)
 create mode 100644 hw/ide/ahci.h
 create mode 100644 hw/ide/ich.c

[Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core

[Alexander Graf]

From: Sebastian Herbszt <herbszt@gmx.de>

There are multiple ahci devices out there. The currently implemented ich-9
is only one of the many. So let's split that one out into a separate file
to stress the difference.

Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 Makefile.objs |    1 +
 hw/ide/ahci.c |  305 +-------------------------------------------------------
 hw/ide/ahci.h |  309 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ide/ich.c  |   61 +++++++++++
 4 files changed, 375 insertions(+), 301 deletions(-)
 create mode 100644 hw/ide/ahci.h
 create mode 100644 hw/ide/ich.c

diff --git a/Makefile.objs b/Makefile.objs
index d6b3d60..d6eb8c8 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -246,6 +246,7 @@ hw-obj-$(CONFIG_IDE_CMD646) += ide/cmd646.o
 hw-obj-$(CONFIG_IDE_MACIO) += ide/macio.o
 hw-obj-$(CONFIG_IDE_VIA) += ide/via.o
 hw-obj-$(CONFIG_AHCI) += ide/ahci.o
+hw-obj-$(CONFIG_AHCI) += ide/ich.o
 
 # SCSI layer
 hw-obj-$(CONFIG_LSI_SCSI_PCI) += lsi53c895a.o
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 968fdce..8b6947a 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -73,6 +73,7 @@
 #include "blockdev.h"
 #include "internal.h"
 #include <hw/ide/pci.h>
+#include <hw/ide/ahci.h>
 
 /* #define DEBUG_AHCI */
 
@@ -84,304 +85,6 @@ do { fprintf(stderr, "ahci: %s: [%d] ", __FUNCTION__, port); \
 #define DPRINTF(port, fmt, ...) do {} while(0)
 #endif
 
-#define AHCI_PCI_BAR              5
-#define AHCI_MAX_PORTS            32
-#define AHCI_MAX_SG               168 /* hardware max is 64K */
-#define AHCI_DMA_BOUNDARY         0xffffffff
-#define AHCI_USE_CLUSTERING       0
-#define AHCI_MAX_CMDS             32
-#define AHCI_CMD_SZ               32
-#define AHCI_CMD_SLOT_SZ          (AHCI_MAX_CMDS * AHCI_CMD_SZ)
-#define AHCI_RX_FIS_SZ            256
-#define AHCI_CMD_TBL_CDB          0x40
-#define AHCI_CMD_TBL_HDR_SZ       0x80
-#define AHCI_CMD_TBL_SZ           (AHCI_CMD_TBL_HDR_SZ + (AHCI_MAX_SG * 16))
-#define AHCI_CMD_TBL_AR_SZ        (AHCI_CMD_TBL_SZ * AHCI_MAX_CMDS)
-#define AHCI_PORT_PRIV_DMA_SZ     (AHCI_CMD_SLOT_SZ + AHCI_CMD_TBL_AR_SZ + \
-                                   AHCI_RX_FIS_SZ)
-
-#define AHCI_IRQ_ON_SG            (1 << 31)
-#define AHCI_CMD_ATAPI            (1 << 5)
-#define AHCI_CMD_WRITE            (1 << 6)
-#define AHCI_CMD_PREFETCH         (1 << 7)
-#define AHCI_CMD_RESET            (1 << 8)
-#define AHCI_CMD_CLR_BUSY         (1 << 10)
-
-#define RX_FIS_D2H_REG            0x40 /* offset of D2H Register FIS data */
-#define RX_FIS_SDB                0x58 /* offset of SDB FIS data */
-#define RX_FIS_UNK                0x60 /* offset of Unknown FIS data */
-
-/* global controller registers */
-#define HOST_CAP                  0x00 /* host capabilities */
-#define HOST_CTL                  0x04 /* global host control */
-#define HOST_IRQ_STAT             0x08 /* interrupt status */
-#define HOST_PORTS_IMPL           0x0c /* bitmap of implemented ports */
-#define HOST_VERSION              0x10 /* AHCI spec. version compliancy */
-
-/* HOST_CTL bits */
-#define HOST_CTL_RESET            (1 << 0)  /* reset controller; self-clear */
-#define HOST_CTL_IRQ_EN           (1 << 1)  /* global IRQ enable */
-#define HOST_CTL_AHCI_EN          (1 << 31) /* AHCI enabled */
-
-/* HOST_CAP bits */
-#define HOST_CAP_SSC              (1 << 14) /* Slumber capable */
-#define HOST_CAP_AHCI             (1 << 18) /* AHCI only */
-#define HOST_CAP_CLO              (1 << 24) /* Command List Override support */
-#define HOST_CAP_SSS              (1 << 27) /* Staggered Spin-up */
-#define HOST_CAP_NCQ              (1 << 30) /* Native Command Queueing */
-#define HOST_CAP_64               (1 << 31) /* PCI DAC (64-bit DMA) support */
-
-/* registers for each SATA port */
-#define PORT_LST_ADDR             0x00 /* command list DMA addr */
-#define PORT_LST_ADDR_HI          0x04 /* command list DMA addr hi */
-#define PORT_FIS_ADDR             0x08 /* FIS rx buf addr */
-#define PORT_FIS_ADDR_HI          0x0c /* FIS rx buf addr hi */
-#define PORT_IRQ_STAT             0x10 /* interrupt status */
-#define PORT_IRQ_MASK             0x14 /* interrupt enable/disable mask */
-#define PORT_CMD                  0x18 /* port command */
-#define PORT_TFDATA               0x20 /* taskfile data */
-#define PORT_SIG                  0x24 /* device TF signature */
-#define PORT_SCR_STAT             0x28 /* SATA phy register: SStatus */
-#define PORT_SCR_CTL              0x2c /* SATA phy register: SControl */
-#define PORT_SCR_ERR              0x30 /* SATA phy register: SError */
-#define PORT_SCR_ACT              0x34 /* SATA phy register: SActive */
-#define PORT_CMD_ISSUE            0x38 /* command issue */
-#define PORT_RESERVED             0x3c /* reserved */
-
-/* PORT_IRQ_{STAT,MASK} bits */
-#define PORT_IRQ_COLD_PRES        (1 << 31) /* cold presence detect */
-#define PORT_IRQ_TF_ERR           (1 << 30) /* task file error */
-#define PORT_IRQ_HBUS_ERR         (1 << 29) /* host bus fatal error */
-#define PORT_IRQ_HBUS_DATA_ERR    (1 << 28) /* host bus data error */
-#define PORT_IRQ_IF_ERR           (1 << 27) /* interface fatal error */
-#define PORT_IRQ_IF_NONFATAL      (1 << 26) /* interface non-fatal error */
-#define PORT_IRQ_OVERFLOW         (1 << 24) /* xfer exhausted available S/G */
-#define PORT_IRQ_BAD_PMP          (1 << 23) /* incorrect port multiplier */
-
-#define PORT_IRQ_PHYRDY           (1 << 22) /* PhyRdy changed */
-#define PORT_IRQ_DEV_ILCK         (1 << 7) /* device interlock */
-#define PORT_IRQ_CONNECT          (1 << 6) /* port connect change status */
-#define PORT_IRQ_SG_DONE          (1 << 5) /* descriptor processed */
-#define PORT_IRQ_UNK_FIS          (1 << 4) /* unknown FIS rx'd */
-#define PORT_IRQ_SDB_FIS          (1 << 3) /* Set Device Bits FIS rx'd */
-#define PORT_IRQ_DMAS_FIS         (1 << 2) /* DMA Setup FIS rx'd */
-#define PORT_IRQ_PIOS_FIS         (1 << 1) /* PIO Setup FIS rx'd */
-#define PORT_IRQ_D2H_REG_FIS      (1 << 0) /* D2H Register FIS rx'd */
-
-#define PORT_IRQ_FREEZE           (PORT_IRQ_HBUS_ERR | PORT_IRQ_IF_ERR |   \
-                                   PORT_IRQ_CONNECT | PORT_IRQ_PHYRDY |    \
-                                   PORT_IRQ_UNK_FIS)
-#define PORT_IRQ_ERROR            (PORT_IRQ_FREEZE | PORT_IRQ_TF_ERR |     \
-                                   PORT_IRQ_HBUS_DATA_ERR)
-#define DEF_PORT_IRQ              (PORT_IRQ_ERROR | PORT_IRQ_SG_DONE |     \
-                                   PORT_IRQ_SDB_FIS | PORT_IRQ_DMAS_FIS |  \
-                                   PORT_IRQ_PIOS_FIS | PORT_IRQ_D2H_REG_FIS)
-
-/* PORT_CMD bits */
-#define PORT_CMD_ATAPI            (1 << 24) /* Device is ATAPI */
-#define PORT_CMD_LIST_ON          (1 << 15) /* cmd list DMA engine running */
-#define PORT_CMD_FIS_ON           (1 << 14) /* FIS DMA engine running */
-#define PORT_CMD_FIS_RX           (1 << 4) /* Enable FIS receive DMA engine */
-#define PORT_CMD_CLO              (1 << 3) /* Command list override */
-#define PORT_CMD_POWER_ON         (1 << 2) /* Power up device */
-#define PORT_CMD_SPIN_UP          (1 << 1) /* Spin up device */
-#define PORT_CMD_START            (1 << 0) /* Enable port DMA engine */
-
-#define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */
-#define PORT_CMD_ICC_ACTIVE       (0x1 << 28) /* Put i/f in active state */
-#define PORT_CMD_ICC_PARTIAL      (0x2 << 28) /* Put i/f in partial state */
-#define PORT_CMD_ICC_SLUMBER      (0x6 << 28) /* Put i/f in slumber state */
-
-#define PORT_IRQ_STAT_DHRS        (1 << 0) /* Device to Host Register FIS */
-#define PORT_IRQ_STAT_PSS         (1 << 1) /* PIO Setup FIS */
-#define PORT_IRQ_STAT_DSS         (1 << 2) /* DMA Setup FIS */
-#define PORT_IRQ_STAT_SDBS        (1 << 3) /* Set Device Bits */
-#define PORT_IRQ_STAT_UFS         (1 << 4) /* Unknown FIS */
-#define PORT_IRQ_STAT_DPS         (1 << 5) /* Descriptor Processed */
-#define PORT_IRQ_STAT_PCS         (1 << 6) /* Port Connect Change Status */
-#define PORT_IRQ_STAT_DMPS        (1 << 7) /* Device Mechanical Presence
-                                              Status */
-#define PORT_IRQ_STAT_PRCS        (1 << 22) /* File Ready Status */
-#define PORT_IRQ_STAT_IPMS        (1 << 23) /* Incorrect Port Multiplier
-                                               Status */
-#define PORT_IRQ_STAT_OFS         (1 << 24) /* Overflow Status */
-#define PORT_IRQ_STAT_INFS        (1 << 26) /* Interface Non-Fatal Error
-                                               Status */
-#define PORT_IRQ_STAT_IFS         (1 << 27) /* Interface Fatal Error */
-#define PORT_IRQ_STAT_HBDS        (1 << 28) /* Host Bus Data Error Status */
-#define PORT_IRQ_STAT_HBFS        (1 << 29) /* Host Bus Fatal Error Status */
-#define PORT_IRQ_STAT_TFES        (1 << 30) /* Task File Error Status */
-#define PORT_IRQ_STAT_CPDS        (1 << 31) /* Code Port Detect Status */
-
-/* ap->flags bits */
-#define AHCI_FLAG_NO_NCQ                  (1 << 24)
-#define AHCI_FLAG_IGN_IRQ_IF_ERR          (1 << 25) /* ignore IRQ_IF_ERR */
-#define AHCI_FLAG_HONOR_PI                (1 << 26) /* honor PORTS_IMPL */
-#define AHCI_FLAG_IGN_SERR_INTERNAL       (1 << 27) /* ignore SERR_INTERNAL */
-#define AHCI_FLAG_32BIT_ONLY              (1 << 28) /* force 32bit */
-
-#define ATA_SRST                          (1 << 2)  /* software reset */
-
-#define STATE_RUN                         0
-#define STATE_RESET                       1
-
-#define SATA_SCR_SSTATUS_DET_NODEV        0x0
-#define SATA_SCR_SSTATUS_DET_DEV_PRESENT_PHY_UP 0x3
-
-#define SATA_SCR_SSTATUS_SPD_NODEV        0x00
-#define SATA_SCR_SSTATUS_SPD_GEN1         0x10
-
-#define SATA_SCR_SSTATUS_IPM_NODEV        0x000
-#define SATA_SCR_SSTATUS_IPM_ACTIVE       0X100
-
-#define AHCI_SCR_SCTL_DET                 0xf
-
-#define SATA_FIS_TYPE_REGISTER_H2D        0x27
-#define SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER 0x80
-
-#define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
-#define AHCI_CMD_HDR_PRDT_LEN              16
-
-#define SATA_SIGNATURE_CDROM               0xeb140000
-#define SATA_SIGNATURE_DISK                0x00000101
-
-#define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
-                                            /* Shouldn't this be 0x2c? */
-
-#define SATA_PORTS                         4
-
-#define AHCI_PORT_REGS_START_ADDR          0x100
-#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
-#define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
-
-#define AHCI_NUM_COMMAND_SLOTS             31
-#define AHCI_SUPPORTED_SPEED               20
-#define AHCI_SUPPORTED_SPEED_GEN1          1
-#define AHCI_VERSION_1_0                   0x10000
-
-#define AHCI_PROGMODE_MAJOR_REV_1          1
-
-#define AHCI_COMMAND_TABLE_ACMD            0x40
-
-#define IDE_FEATURE_DMA                    1
-
-#define READ_FPDMA_QUEUED                  0x60
-#define WRITE_FPDMA_QUEUED                 0x61
-
-#define RES_FIS_DSFIS                      0x00
-#define RES_FIS_PSFIS                      0x20
-#define RES_FIS_RFIS                       0x40
-#define RES_FIS_SDBFIS                     0x58
-#define RES_FIS_UFIS                       0x60
-
-typedef struct AHCIControlRegs {
-    uint32_t    cap;
-    uint32_t    ghc;
-    uint32_t    irqstatus;
-    uint32_t    impl;
-    uint32_t    version;
-} AHCIControlRegs;
-
-typedef struct AHCIPortRegs {
-    uint32_t    lst_addr;
-    uint32_t    lst_addr_hi;
-    uint32_t    fis_addr;
-    uint32_t    fis_addr_hi;
-    uint32_t    irq_stat;
-    uint32_t    irq_mask;
-    uint32_t    cmd;
-    uint32_t    unused0;
-    uint32_t    tfdata;
-    uint32_t    sig;
-    uint32_t    scr_stat;
-    uint32_t    scr_ctl;
-    uint32_t    scr_err;
-    uint32_t    scr_act;
-    uint32_t    cmd_issue;
-    uint32_t    reserved;
-} AHCIPortRegs;
-
-typedef struct AHCICmdHdr {
-    uint32_t    opts;
-    uint32_t    status;
-    uint64_t    tbl_addr;
-    uint32_t    reserved[4];
-} __attribute__ ((packed)) AHCICmdHdr;
-
-typedef struct AHCI_SG {
-    uint64_t    addr;
-    uint32_t    reserved;
-    uint32_t    flags_size;
-} __attribute__ ((packed)) AHCI_SG;
-
-typedef struct AHCIDevice AHCIDevice;
-
-typedef struct NCQTransferState {
-    AHCIDevice *drive;
-    BlockDriverAIOCB *aiocb;
-    QEMUSGList sglist;
-    int is_read;
-    uint16_t sector_count;
-    uint64_t lba;
-    uint8_t tag;
-    int slot;
-    int used;
-} NCQTransferState;
-
-struct AHCIDevice {
-    IDEDMA dma;
-    IDEBus port;
-    int port_no;
-    uint32_t port_state;
-    uint32_t finished;
-    AHCIPortRegs port_regs;
-    struct AHCIState *hba;
-    QEMUBH *check_bh;
-    uint8_t *lst;
-    uint8_t *res_fis;
-    int dma_status;
-    int done_atapi_packet;
-    int busy_slot;
-    BlockDriverCompletionFunc *dma_cb;
-    AHCICmdHdr *cur_cmd;
-    NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
-};
-
-typedef struct AHCIState {
-    AHCIDevice dev[SATA_PORTS];
-    AHCIControlRegs control_regs;
-    int mem;
-    qemu_irq irq;
-} AHCIState;
-
-typedef struct AHCIPCIState {
-    PCIDevice card;
-    AHCIState ahci;
-} AHCIPCIState;
-
-typedef struct NCQFrame {
-    uint8_t fis_type;
-    uint8_t c;
-    uint8_t command;
-    uint8_t sector_count_low;
-    uint8_t lba0;
-    uint8_t lba1;
-    uint8_t lba2;
-    uint8_t fua;
-    uint8_t lba3;
-    uint8_t lba4;
-    uint8_t lba5;
-    uint8_t sector_count_high;
-    uint8_t tag;
-    uint8_t reserved5;
-    uint8_t reserved6;
-    uint8_t control;
-    uint8_t reserved7;
-    uint8_t reserved8;
-    uint8_t reserved9;
-    uint8_t reserved10;
-} __attribute__ ((packed)) NCQFrame;
-
 static void check_cmd(AHCIState *s, int port);
 static int handle_cmd(AHCIState *s,int port,int slot);
 static void ahci_reset_port(AHCIState *s, int port);
@@ -1411,7 +1114,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .reset = ahci_dma_reset,
 };
 
-static void ahci_init(AHCIState *s, DeviceState *qdev)
+void ahci_init(AHCIState *s, DeviceState *qdev)
 {
     qemu_irq *irqs;
     int i;
@@ -1435,7 +1138,7 @@ static void ahci_init(AHCIState *s, DeviceState *qdev)
     }
 }
 
-static void ahci_pci_map(PCIDevice *pci_dev, int region_num,
+void ahci_pci_map(PCIDevice *pci_dev, int region_num,
         pcibus_t addr, pcibus_t size, int type)
 {
     struct AHCIPCIState *d = (struct AHCIPCIState *)pci_dev;
@@ -1444,7 +1147,7 @@ static void ahci_pci_map(PCIDevice *pci_dev, int region_num,
     cpu_register_physical_memory(addr, size, s->mem);
 }
 
-static void ahci_reset(void *opaque)
+void ahci_reset(void *opaque)
 {
     struct AHCIPCIState *d = opaque;
     int i;
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
new file mode 100644
index 0000000..63ef785
--- /dev/null
+++ b/hw/ide/ahci.h
@@ -0,0 +1,309 @@
+#ifndef HW_IDE_AHCI_H
+#define HW_IDE_AHCI_H
+
+#define AHCI_PCI_BAR              5
+#define AHCI_MAX_PORTS            32
+#define AHCI_MAX_SG               168 /* hardware max is 64K */
+#define AHCI_DMA_BOUNDARY         0xffffffff
+#define AHCI_USE_CLUSTERING       0
+#define AHCI_MAX_CMDS             32
+#define AHCI_CMD_SZ               32
+#define AHCI_CMD_SLOT_SZ          (AHCI_MAX_CMDS * AHCI_CMD_SZ)
+#define AHCI_RX_FIS_SZ            256
+#define AHCI_CMD_TBL_CDB          0x40
+#define AHCI_CMD_TBL_HDR_SZ       0x80
+#define AHCI_CMD_TBL_SZ           (AHCI_CMD_TBL_HDR_SZ + (AHCI_MAX_SG * 16))
+#define AHCI_CMD_TBL_AR_SZ        (AHCI_CMD_TBL_SZ * AHCI_MAX_CMDS)
+#define AHCI_PORT_PRIV_DMA_SZ     (AHCI_CMD_SLOT_SZ + AHCI_CMD_TBL_AR_SZ + \
+                                   AHCI_RX_FIS_SZ)
+
+#define AHCI_IRQ_ON_SG            (1 << 31)
+#define AHCI_CMD_ATAPI            (1 << 5)
+#define AHCI_CMD_WRITE            (1 << 6)
+#define AHCI_CMD_PREFETCH         (1 << 7)
+#define AHCI_CMD_RESET            (1 << 8)
+#define AHCI_CMD_CLR_BUSY         (1 << 10)
+
+#define RX_FIS_D2H_REG            0x40 /* offset of D2H Register FIS data */
+#define RX_FIS_SDB                0x58 /* offset of SDB FIS data */
+#define RX_FIS_UNK                0x60 /* offset of Unknown FIS data */
+
+/* global controller registers */
+#define HOST_CAP                  0x00 /* host capabilities */
+#define HOST_CTL                  0x04 /* global host control */
+#define HOST_IRQ_STAT             0x08 /* interrupt status */
+#define HOST_PORTS_IMPL           0x0c /* bitmap of implemented ports */
+#define HOST_VERSION              0x10 /* AHCI spec. version compliancy */
+
+/* HOST_CTL bits */
+#define HOST_CTL_RESET            (1 << 0)  /* reset controller; self-clear */
+#define HOST_CTL_IRQ_EN           (1 << 1)  /* global IRQ enable */
+#define HOST_CTL_AHCI_EN          (1 << 31) /* AHCI enabled */
+
+/* HOST_CAP bits */
+#define HOST_CAP_SSC              (1 << 14) /* Slumber capable */
+#define HOST_CAP_AHCI             (1 << 18) /* AHCI only */
+#define HOST_CAP_CLO              (1 << 24) /* Command List Override support */
+#define HOST_CAP_SSS              (1 << 27) /* Staggered Spin-up */
+#define HOST_CAP_NCQ              (1 << 30) /* Native Command Queueing */
+#define HOST_CAP_64               (1 << 31) /* PCI DAC (64-bit DMA) support */
+
+/* registers for each SATA port */
+#define PORT_LST_ADDR             0x00 /* command list DMA addr */
+#define PORT_LST_ADDR_HI          0x04 /* command list DMA addr hi */
+#define PORT_FIS_ADDR             0x08 /* FIS rx buf addr */
+#define PORT_FIS_ADDR_HI          0x0c /* FIS rx buf addr hi */
+#define PORT_IRQ_STAT             0x10 /* interrupt status */
+#define PORT_IRQ_MASK             0x14 /* interrupt enable/disable mask */
+#define PORT_CMD                  0x18 /* port command */
+#define PORT_TFDATA               0x20 /* taskfile data */
+#define PORT_SIG                  0x24 /* device TF signature */
+#define PORT_SCR_STAT             0x28 /* SATA phy register: SStatus */
+#define PORT_SCR_CTL              0x2c /* SATA phy register: SControl */
+#define PORT_SCR_ERR              0x30 /* SATA phy register: SError */
+#define PORT_SCR_ACT              0x34 /* SATA phy register: SActive */
+#define PORT_CMD_ISSUE            0x38 /* command issue */
+#define PORT_RESERVED             0x3c /* reserved */
+
+/* PORT_IRQ_{STAT,MASK} bits */
+#define PORT_IRQ_COLD_PRES        (1 << 31) /* cold presence detect */
+#define PORT_IRQ_TF_ERR           (1 << 30) /* task file error */
+#define PORT_IRQ_HBUS_ERR         (1 << 29) /* host bus fatal error */
+#define PORT_IRQ_HBUS_DATA_ERR    (1 << 28) /* host bus data error */
+#define PORT_IRQ_IF_ERR           (1 << 27) /* interface fatal error */
+#define PORT_IRQ_IF_NONFATAL      (1 << 26) /* interface non-fatal error */
+#define PORT_IRQ_OVERFLOW         (1 << 24) /* xfer exhausted available S/G */
+#define PORT_IRQ_BAD_PMP          (1 << 23) /* incorrect port multiplier */
+
+#define PORT_IRQ_PHYRDY           (1 << 22) /* PhyRdy changed */
+#define PORT_IRQ_DEV_ILCK         (1 << 7) /* device interlock */
+#define PORT_IRQ_CONNECT          (1 << 6) /* port connect change status */
+#define PORT_IRQ_SG_DONE          (1 << 5) /* descriptor processed */
+#define PORT_IRQ_UNK_FIS          (1 << 4) /* unknown FIS rx'd */
+#define PORT_IRQ_SDB_FIS          (1 << 3) /* Set Device Bits FIS rx'd */
+#define PORT_IRQ_DMAS_FIS         (1 << 2) /* DMA Setup FIS rx'd */
+#define PORT_IRQ_PIOS_FIS         (1 << 1) /* PIO Setup FIS rx'd */
+#define PORT_IRQ_D2H_REG_FIS      (1 << 0) /* D2H Register FIS rx'd */
+
+#define PORT_IRQ_FREEZE           (PORT_IRQ_HBUS_ERR | PORT_IRQ_IF_ERR |   \
+                                   PORT_IRQ_CONNECT | PORT_IRQ_PHYRDY |    \
+                                   PORT_IRQ_UNK_FIS)
+#define PORT_IRQ_ERROR            (PORT_IRQ_FREEZE | PORT_IRQ_TF_ERR |     \
+                                   PORT_IRQ_HBUS_DATA_ERR)
+#define DEF_PORT_IRQ              (PORT_IRQ_ERROR | PORT_IRQ_SG_DONE |     \
+                                   PORT_IRQ_SDB_FIS | PORT_IRQ_DMAS_FIS |  \
+                                   PORT_IRQ_PIOS_FIS | PORT_IRQ_D2H_REG_FIS)
+
+/* PORT_CMD bits */
+#define PORT_CMD_ATAPI            (1 << 24) /* Device is ATAPI */
+#define PORT_CMD_LIST_ON          (1 << 15) /* cmd list DMA engine running */
+#define PORT_CMD_FIS_ON           (1 << 14) /* FIS DMA engine running */
+#define PORT_CMD_FIS_RX           (1 << 4) /* Enable FIS receive DMA engine */
+#define PORT_CMD_CLO              (1 << 3) /* Command list override */
+#define PORT_CMD_POWER_ON         (1 << 2) /* Power up device */
+#define PORT_CMD_SPIN_UP          (1 << 1) /* Spin up device */
+#define PORT_CMD_START            (1 << 0) /* Enable port DMA engine */
+
+#define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */
+#define PORT_CMD_ICC_ACTIVE       (0x1 << 28) /* Put i/f in active state */
+#define PORT_CMD_ICC_PARTIAL      (0x2 << 28) /* Put i/f in partial state */
+#define PORT_CMD_ICC_SLUMBER      (0x6 << 28) /* Put i/f in slumber state */
+
+#define PORT_IRQ_STAT_DHRS        (1 << 0) /* Device to Host Register FIS */
+#define PORT_IRQ_STAT_PSS         (1 << 1) /* PIO Setup FIS */
+#define PORT_IRQ_STAT_DSS         (1 << 2) /* DMA Setup FIS */
+#define PORT_IRQ_STAT_SDBS        (1 << 3) /* Set Device Bits */
+#define PORT_IRQ_STAT_UFS         (1 << 4) /* Unknown FIS */
+#define PORT_IRQ_STAT_DPS         (1 << 5) /* Descriptor Processed */
+#define PORT_IRQ_STAT_PCS         (1 << 6) /* Port Connect Change Status */
+#define PORT_IRQ_STAT_DMPS        (1 << 7) /* Device Mechanical Presence
+                                              Status */
+#define PORT_IRQ_STAT_PRCS        (1 << 22) /* File Ready Status */
+#define PORT_IRQ_STAT_IPMS        (1 << 23) /* Incorrect Port Multiplier
+                                               Status */
+#define PORT_IRQ_STAT_OFS         (1 << 24) /* Overflow Status */
+#define PORT_IRQ_STAT_INFS        (1 << 26) /* Interface Non-Fatal Error
+                                               Status */
+#define PORT_IRQ_STAT_IFS         (1 << 27) /* Interface Fatal Error */
+#define PORT_IRQ_STAT_HBDS        (1 << 28) /* Host Bus Data Error Status */
+#define PORT_IRQ_STAT_HBFS        (1 << 29) /* Host Bus Fatal Error Status */
+#define PORT_IRQ_STAT_TFES        (1 << 30) /* Task File Error Status */
+#define PORT_IRQ_STAT_CPDS        (1 << 31) /* Code Port Detect Status */
+
+/* ap->flags bits */
+#define AHCI_FLAG_NO_NCQ                  (1 << 24)
+#define AHCI_FLAG_IGN_IRQ_IF_ERR          (1 << 25) /* ignore IRQ_IF_ERR */
+#define AHCI_FLAG_HONOR_PI                (1 << 26) /* honor PORTS_IMPL */
+#define AHCI_FLAG_IGN_SERR_INTERNAL       (1 << 27) /* ignore SERR_INTERNAL */
+#define AHCI_FLAG_32BIT_ONLY              (1 << 28) /* force 32bit */
+
+#define ATA_SRST                          (1 << 2)  /* software reset */
+
+#define STATE_RUN                         0
+#define STATE_RESET                       1
+
+#define SATA_SCR_SSTATUS_DET_NODEV        0x0
+#define SATA_SCR_SSTATUS_DET_DEV_PRESENT_PHY_UP 0x3
+
+#define SATA_SCR_SSTATUS_SPD_NODEV        0x00
+#define SATA_SCR_SSTATUS_SPD_GEN1         0x10
+
+#define SATA_SCR_SSTATUS_IPM_NODEV        0x000
+#define SATA_SCR_SSTATUS_IPM_ACTIVE       0X100
+
+#define AHCI_SCR_SCTL_DET                 0xf
+
+#define SATA_FIS_TYPE_REGISTER_H2D        0x27
+#define SATA_FIS_REG_H2D_UPDATE_COMMAND_REGISTER 0x80
+
+#define AHCI_CMD_HDR_CMD_FIS_LEN           0x1f
+#define AHCI_CMD_HDR_PRDT_LEN              16
+
+#define SATA_SIGNATURE_CDROM               0xeb140000
+#define SATA_SIGNATURE_DISK                0x00000101
+
+#define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
+                                            /* Shouldn't this be 0x2c? */
+
+#define SATA_PORTS                         4
+
+#define AHCI_PORT_REGS_START_ADDR          0x100
+#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
+#define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
+
+#define AHCI_NUM_COMMAND_SLOTS             31
+#define AHCI_SUPPORTED_SPEED               20
+#define AHCI_SUPPORTED_SPEED_GEN1          1
+#define AHCI_VERSION_1_0                   0x10000
+
+#define AHCI_PROGMODE_MAJOR_REV_1          1
+
+#define AHCI_COMMAND_TABLE_ACMD            0x40
+
+#define IDE_FEATURE_DMA                    1
+
+#define READ_FPDMA_QUEUED                  0x60
+#define WRITE_FPDMA_QUEUED                 0x61
+
+#define RES_FIS_DSFIS                      0x00
+#define RES_FIS_PSFIS                      0x20
+#define RES_FIS_RFIS                       0x40
+#define RES_FIS_SDBFIS                     0x58
+#define RES_FIS_UFIS                       0x60
+
+typedef struct AHCIControlRegs {
+    uint32_t    cap;
+    uint32_t    ghc;
+    uint32_t    irqstatus;
+    uint32_t    impl;
+    uint32_t    version;
+} AHCIControlRegs;
+
+typedef struct AHCIPortRegs {
+    uint32_t    lst_addr;
+    uint32_t    lst_addr_hi;
+    uint32_t    fis_addr;
+    uint32_t    fis_addr_hi;
+    uint32_t    irq_stat;
+    uint32_t    irq_mask;
+    uint32_t    cmd;
+    uint32_t    unused0;
+    uint32_t    tfdata;
+    uint32_t    sig;
+    uint32_t    scr_stat;
+    uint32_t    scr_ctl;
+    uint32_t    scr_err;
+    uint32_t    scr_act;
+    uint32_t    cmd_issue;
+    uint32_t    reserved;
+} AHCIPortRegs;
+
+typedef struct AHCICmdHdr {
+    uint32_t    opts;
+    uint32_t    status;
+    uint64_t    tbl_addr;
+    uint32_t    reserved[4];
+} __attribute__ ((packed)) AHCICmdHdr;
+
+typedef struct AHCI_SG {
+    uint64_t    addr;
+    uint32_t    reserved;
+    uint32_t    flags_size;
+} __attribute__ ((packed)) AHCI_SG;
+
+typedef struct AHCIDevice AHCIDevice;
+
+typedef struct NCQTransferState {
+    AHCIDevice *drive;
+    BlockDriverAIOCB *aiocb;
+    QEMUSGList sglist;
+    int is_read;
+    uint16_t sector_count;
+    uint64_t lba;
+    uint8_t tag;
+    int slot;
+    int used;
+} NCQTransferState;
+
+struct AHCIDevice {
+    IDEDMA dma;
+    IDEBus port;
+    int port_no;
+    uint32_t port_state;
+    uint32_t finished;
+    AHCIPortRegs port_regs;
+    struct AHCIState *hba;
+    QEMUBH *check_bh;
+    uint8_t *lst;
+    uint8_t *res_fis;
+    int dma_status;
+    int done_atapi_packet;
+    int busy_slot;
+    BlockDriverCompletionFunc *dma_cb;
+    AHCICmdHdr *cur_cmd;
+    NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
+};
+
+typedef struct AHCIState {
+    AHCIDevice dev[SATA_PORTS];
+    AHCIControlRegs control_regs;
+    int mem;
+    qemu_irq irq;
+} AHCIState;
+
+typedef struct AHCIPCIState {
+    PCIDevice card;
+    AHCIState ahci;
+} AHCIPCIState;
+
+typedef struct NCQFrame {
+    uint8_t fis_type;
+    uint8_t c;
+    uint8_t command;
+    uint8_t sector_count_low;
+    uint8_t lba0;
+    uint8_t lba1;
+    uint8_t lba2;
+    uint8_t fua;
+    uint8_t lba3;
+    uint8_t lba4;
+    uint8_t lba5;
+    uint8_t sector_count_high;
+    uint8_t tag;
+    uint8_t reserved5;
+    uint8_t reserved6;
+    uint8_t control;
+    uint8_t reserved7;
+    uint8_t reserved8;
+    uint8_t reserved9;
+    uint8_t reserved10;
+} __attribute__ ((packed)) NCQFrame;
+
+void ahci_init(AHCIState *s, DeviceState *qdev);
+
+void ahci_pci_map(PCIDevice *pci_dev, int region_num,
+        pcibus_t addr, pcibus_t size, int type);
+
+void ahci_reset(void *opaque);
+
+#endif /* HW_IDE_AHCI_H */
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
new file mode 100644
index 0000000..9868b73
--- /dev/null
+++ b/hw/ide/ich.c
@@ -0,0 +1,61 @@
+#include <hw/hw.h>
+#include <hw/msi.h>
+#include <hw/pc.h>
+#include <hw/pci.h>
+#include <hw/isa.h>
+#include "block.h"
+#include "block_int.h"
+#include "sysemu.h"
+#include "dma.h"
+
+#include <hw/ide/pci.h>
+#include <hw/ide/ahci.h>
+
+static int pci_ich9_ahci_initfn(PCIDevice *dev)
+{
+    struct AHCIPCIState *d;
+    d = DO_UPCAST(struct AHCIPCIState, card, dev);
+
+    pci_config_set_vendor_id(d->card.config, PCI_VENDOR_ID_INTEL);
+    pci_config_set_device_id(d->card.config, PCI_DEVICE_ID_INTEL_82801IR);
+
+    pci_config_set_class(d->card.config, PCI_CLASS_STORAGE_SATA);
+    pci_config_set_revision(d->card.config, 0x02);
+    pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
+
+    d->card.config[PCI_CACHE_LINE_SIZE] = 0x08;  /* Cache line size */
+    d->card.config[PCI_LATENCY_TIMER]   = 0x00;  /* Latency timer */
+    pci_config_set_interrupt_pin(d->card.config, 1);
+
+    /* XXX Software should program this register */
+    d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
+
+    qemu_register_reset(ahci_reset, d);
+
+    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
+    pci_register_bar(&d->card, 5, 0x1000, PCI_BASE_ADDRESS_SPACE_MEMORY,
+                     ahci_pci_map);
+
+    msi_init(dev, 0x50, 1, true, false);
+
+    ahci_init(&d->ahci, &dev->qdev);
+    d->ahci.irq = d->card.irq[0];
+
+    return 0;
+}
+
+static PCIDeviceInfo ich_ahci_info[] = {
+    {
+        .qdev.name    = "ich9-ahci",
+        .qdev.size    = sizeof(AHCIPCIState),
+        .init         = pci_ich9_ahci_initfn,
+    },{
+        /* end of list */
+    }
+};
+
+static void ich_ahci_register(void)
+{
+    pci_qdev_register_many(ich_ahci_info);
+}
+device_init(ich_ahci_register);
-- 
1.6.0.2

[Qemu-devel] [PATCH 2/8] ahci: split ICH and AHCI even more

[Alexander Graf]

Sebastian's patch already did a pretty good job at splitting up ICH-9
AHCI code and the AHCI core. We need some more though. Copyright was missing,
the lspci dump belongs to ICH-9, we don't need the AHCI core to have its
own qdev device duplicate.

So let's split them a bit more in this patch, making things easier to
read an understand.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - reflect ich name in init functions
---
 hw/ide/ahci.c |  110 ---------------------------------------------------------
 hw/ide/ich.c  |   90 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 112 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8b6947a..18187c8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -19,47 +19,6 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  *
- *
- * lspci dump of a ICH-9 real device in IDE mode (hopefully close enough):
- *
- * 00:1f.2 SATA controller [0106]: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller [8086:2922] (rev 02) (prog-if 01 [AHCI 1.0])
- *         Subsystem: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller [8086:2922]
- *         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
- *         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
- *         Latency: 0
- *         Interrupt: pin B routed to IRQ 222
- *         Region 0: I/O ports at d000 [size=8]
- *         Region 1: I/O ports at cc00 [size=4]
- *         Region 2: I/O ports at c880 [size=8]
- *         Region 3: I/O ports at c800 [size=4]
- *         Region 4: I/O ports at c480 [size=32]
- *         Region 5: Memory at febf9000 (32-bit, non-prefetchable) [size=2K]
- *         Capabilities: [80] Message Signalled Interrupts: Mask- 64bit- Count=1/16 Enable+
- *                 Address: fee0f00c  Data: 41d9
- *         Capabilities: [70] Power Management version 3
- *                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold-)
- *                 Status: D0 PME-Enable- DSel=0 DScale=0 PME-
- *         Capabilities: [a8] SATA HBA <?>
- *         Capabilities: [b0] Vendor Specific Information <?>
- *         Kernel driver in use: ahci
- *         Kernel modules: ahci
- * 00: 86 80 22 29 07 04 b0 02 02 01 06 01 00 00 00 00
- * 10: 01 d0 00 00 01 cc 00 00 81 c8 00 00 01 c8 00 00
- * 20: 81 c4 00 00 00 90 bf fe 00 00 00 00 86 80 22 29
- * 30: 00 00 00 00 80 00 00 00 00 00 00 00 0f 02 00 00
- * 40: 00 80 00 80 00 00 00 00 00 00 00 00 00 00 00 00
- * 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
- * 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
- * 70: 01 a8 03 40 08 00 00 00 00 00 00 00 00 00 00 00
- * 80: 05 70 09 00 0c f0 e0 fe d9 41 00 00 00 00 00 00
- * 90: 40 00 0f 82 93 01 00 00 00 00 00 00 00 00 00 00
- * a0: ac 00 00 00 0a 00 12 00 12 b0 10 00 48 00 00 00
- * b0: 09 00 06 20 00 00 00 00 00 00 00 00 00 00 00 00
- * c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
- * d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
- * e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
- * f0: 00 00 00 00 00 00 00 00 86 0f 02 00 00 00 00 00
- *
  */
 
 #include <hw/hw.h>
@@ -1156,72 +1115,3 @@ void ahci_reset(void *opaque)
         ahci_reset_port(&d->ahci, i);
     }
 }
-
-static int pci_ahci_init(PCIDevice *dev)
-{
-    struct AHCIPCIState *d;
-    d = DO_UPCAST(struct AHCIPCIState, card, dev);
-
-    pci_config_set_vendor_id(d->card.config, PCI_VENDOR_ID_INTEL);
-    pci_config_set_device_id(d->card.config, PCI_DEVICE_ID_INTEL_82801IR);
-
-    pci_config_set_class(d->card.config, PCI_CLASS_STORAGE_SATA);
-    pci_config_set_revision(d->card.config, 0x02);
-    pci_config_set_prog_interface(d->card.config, AHCI_PROGMODE_MAJOR_REV_1);
-
-    d->card.config[PCI_CACHE_LINE_SIZE] = 0x08;  /* Cache line size */
-    d->card.config[PCI_LATENCY_TIMER]   = 0x00;  /* Latency timer */
-    pci_config_set_interrupt_pin(d->card.config, 1);
-
-    /* XXX Software should program this register */
-    d->card.config[0x90]   = 1 << 6; /* Address Map Register - AHCI mode */
-
-    qemu_register_reset(ahci_reset, d);
-
-    /* XXX BAR size should be 1k, but that breaks, so bump it to 4k for now */
-    pci_register_bar(&d->card, 5, 0x1000, PCI_BASE_ADDRESS_SPACE_MEMORY,
-                     ahci_pci_map);
-
-    msi_init(dev, 0x50, 1, true, false);
-
-    ahci_init(&d->ahci, &dev->qdev);
-    d->ahci.irq = d->card.irq[0];
-
-    return 0;
-}
-
-static int pci_ahci_uninit(PCIDevice *dev)
-{
-    struct AHCIPCIState *d;
-    d = DO_UPCAST(struct AHCIPCIState, card, dev);
-
-    if (msi_enabled(dev)) {
-        msi_uninit(dev);
-    }
-
-    qemu_unregister_reset(ahci_reset, d);
-
-    return 0;
-}
-
-static void pci_ahci_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);
-}
-
-static PCIDeviceInfo ahci_info = {
-    .qdev.name  = "ahci",
-    .qdev.size  = sizeof(AHCIPCIState),
-    .init       = pci_ahci_init,
-    .exit       = pci_ahci_uninit,
-    .config_write = pci_ahci_write_config,
-};
-
-static void ahci_pci_register_devices(void)
-{
-    pci_qdev_register(&ahci_info);
-}
-
-device_init(ahci_pci_register_devices)
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 9868b73..70cb766 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -1,3 +1,65 @@
+/*
+ * QEMU ICH Emulation
+ *
+ * Copyright (c) 2010 Sebastian Herbszt <herbszt@gmx.de>
+ * Copyright (c) 2010 Alexander Graf <agraf@suse.de>
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library 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.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ *
+ *
+ * lspci dump of a ICH-9 real device
+ *
+ * 00:1f.2 SATA controller [0106]: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller [8086:2922] (rev 02) (prog-if 01 [AHCI 1.0])
+ *         Subsystem: Intel Corporation 82801IR/IO/IH (ICH9R/DO/DH) 6 port SATA AHCI Controller [8086:2922]
+ *         Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
+ *         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
+ *         Latency: 0
+ *         Interrupt: pin B routed to IRQ 222
+ *         Region 0: I/O ports at d000 [size=8]
+ *         Region 1: I/O ports at cc00 [size=4]
+ *         Region 2: I/O ports at c880 [size=8]
+ *         Region 3: I/O ports at c800 [size=4]
+ *         Region 4: I/O ports at c480 [size=32]
+ *         Region 5: Memory at febf9000 (32-bit, non-prefetchable) [size=2K]
+ *         Capabilities: [80] Message Signalled Interrupts: Mask- 64bit- Count=1/16 Enable+
+ *                 Address: fee0f00c  Data: 41d9
+ *         Capabilities: [70] Power Management version 3
+ *                 Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot+,D3cold-)
+ *                 Status: D0 PME-Enable- DSel=0 DScale=0 PME-
+ *         Capabilities: [a8] SATA HBA <?>
+ *         Capabilities: [b0] Vendor Specific Information <?>
+ *         Kernel driver in use: ahci
+ *         Kernel modules: ahci
+ * 00: 86 80 22 29 07 04 b0 02 02 01 06 01 00 00 00 00
+ * 10: 01 d0 00 00 01 cc 00 00 81 c8 00 00 01 c8 00 00
+ * 20: 81 c4 00 00 00 90 bf fe 00 00 00 00 86 80 22 29
+ * 30: 00 00 00 00 80 00 00 00 00 00 00 00 0f 02 00 00
+ * 40: 00 80 00 80 00 00 00 00 00 00 00 00 00 00 00 00
+ * 50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * 70: 01 a8 03 40 08 00 00 00 00 00 00 00 00 00 00 00
+ * 80: 05 70 09 00 0c f0 e0 fe d9 41 00 00 00 00 00 00
+ * 90: 40 00 0f 82 93 01 00 00 00 00 00 00 00 00 00 00
+ * a0: ac 00 00 00 0a 00 12 00 12 b0 10 00 48 00 00 00
+ * b0: 09 00 06 20 00 00 00 00 00 00 00 00 00 00 00 00
+ * c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+ * f0: 00 00 00 00 00 00 00 00 86 0f 02 00 00 00 00 00
+ *
+ */
+
 #include <hw/hw.h>
 #include <hw/msi.h>
 #include <hw/pc.h>
@@ -11,7 +73,7 @@
 #include <hw/ide/pci.h>
 #include <hw/ide/ahci.h>
 
-static int pci_ich9_ahci_initfn(PCIDevice *dev)
+static int pci_ich9_ahci_init(PCIDevice *dev)
 {
     struct AHCIPCIState *d;
     d = DO_UPCAST(struct AHCIPCIState, card, dev);
@@ -44,11 +106,35 @@ static int pci_ich9_ahci_initfn(PCIDevice *dev)
     return 0;
 }
 
+static int pci_ich9_uninit(PCIDevice *dev)
+{
+    struct AHCIPCIState *d;
+    d = DO_UPCAST(struct AHCIPCIState, card, dev);
+
+    if (msi_enabled(dev)) {
+        msi_uninit(dev);
+    }
+
+    qemu_unregister_reset(ahci_reset, d);
+
+    return 0;
+}
+
+static void pci_ich9_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);
+}
+
 static PCIDeviceInfo ich_ahci_info[] = {
     {
         .qdev.name    = "ich9-ahci",
+        .qdev.alias   = "ahci",
         .qdev.size    = sizeof(AHCIPCIState),
-        .init         = pci_ich9_ahci_initfn,
+        .init         = pci_ich9_ahci_init,
+        .exit         = pci_ich9_uninit,
+        .config_write = pci_ich9_write_config,
     },{
         /* end of list */
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH 3/8] ahci: send init d2h fis on fis enable

[Alexander Graf]

The drive sends a d2h init fis on initialization. Usually, the guest doesn't
receive fises yet at that point though, so the delivery is deferred.

Let's reflect that by sending the init fis on fis receive enablement.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |   34 +++++++++++++++++++++++++++-------
 hw/ide/ahci.h |    1 +
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 18187c8..fa97f9b 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -48,6 +48,7 @@ static void check_cmd(AHCIState *s, int port);
 static int handle_cmd(AHCIState *s,int port,int slot);
 static void ahci_reset_port(AHCIState *s, int port);
 static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
+static void ahci_init_d2h(AHCIDevice *ad);
 
 static uint32_t  ahci_port_read(AHCIState *s, int port, int offset)
 {
@@ -231,6 +232,12 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
                 pr->cmd |= PORT_CMD_FIS_ON;
             }
 
+            if ((pr->cmd & PORT_CMD_FIS_ON) &&
+                !s->dev[port].init_d2h_sent) {
+                ahci_init_d2h(&s->dev[port]);
+                s->dev[port].init_d2h_sent = 1;
+            }
+
             check_cmd(s, port);
             break;
         case PORT_TFDATA:
@@ -463,12 +470,29 @@ static void ahci_check_cmd_bh(void *opaque)
     check_cmd(ad->hba, ad->port_no);
 }
 
+static void ahci_init_d2h(AHCIDevice *ad)
+{
+    uint8_t init_fis[0x20];
+    IDEState *ide_state = &ad->port.ifs[0];
+
+    memset(init_fis, 0, sizeof(init_fis));
+
+    init_fis[4] = 1;
+    init_fis[12] = 1;
+
+    if (ide_state->drive_kind == IDE_CD) {
+        init_fis[5] = ide_state->lcyl;
+        init_fis[6] = ide_state->hcyl;
+    }
+
+    ahci_write_fis_d2h(ad, init_fis);
+}
+
 static void ahci_reset_port(AHCIState *s, int port)
 {
     AHCIDevice *d = &s->dev[port];
     AHCIPortRegs *pr = &d->port_regs;
     IDEState *ide_state = &d->port.ifs[0];
-    uint8_t init_fis[0x20];
     int i;
 
     DPRINTF(port, "reset port\n");
@@ -483,6 +507,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     pr->scr_err = 0;
     pr->scr_act = 0;
     d->busy_slot = -1;
+    d->init_d2h_sent = 0;
 
     ide_state = &s->dev[port].port.ifs[0];
     if (!ide_state->bs) {
@@ -505,7 +530,6 @@ static void ahci_reset_port(AHCIState *s, int port)
         ncq_tfs->used = 0;
     }
 
-    memset(init_fis, 0, sizeof(init_fis));
     s->dev[port].port_state = STATE_RUN;
     if (!ide_state->bs) {
         s->dev[port].port_regs.sig = 0;
@@ -515,8 +539,6 @@ static void ahci_reset_port(AHCIState *s, int port)
         ide_state->lcyl = 0x14;
         ide_state->hcyl = 0xeb;
         DPRINTF(port, "set lcyl = %d\n", ide_state->lcyl);
-        init_fis[5] = ide_state->lcyl;
-        init_fis[6] = ide_state->hcyl;
         ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
     } else {
         s->dev[port].port_regs.sig = SATA_SIGNATURE_DISK;
@@ -524,9 +546,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     }
 
     ide_state->error = 1;
-    init_fis[4] = 1;
-    init_fis[12] = 1;
-    ahci_write_fis_d2h(d, init_fis);
+    ahci_init_d2h(d);
 }
 
 static void debug_print_fis(uint8_t *fis, int cmd_len)
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 63ef785..5f8126a 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -259,6 +259,7 @@ struct AHCIDevice {
     int dma_status;
     int done_atapi_packet;
     int busy_slot;
+    int init_d2h_sent;
     BlockDriverCompletionFunc *dma_cb;
     AHCICmdHdr *cur_cmd;
     NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
-- 
1.6.0.2

[Qemu-devel] [PATCH 4/8] ahci: use qiov instead of dma helpers

[Alexander Graf]

The DMA helpers incur additional overhead on data transfers. I'm not
sure we need the additional complexity provided by them. So let's just
use qiovs directly when running in the fast path (ncq).

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 hw/ide/ahci.h |    3 ++
 2 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index fa97f9b..7a29925 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -642,6 +642,87 @@ static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
     }
 }
 
+static int ahci_iov_destroy(AHCIDevice *ad, QEMUIOVector *qiov, int is_write)
+{
+    int i;
+    struct iovec *iov = qiov->iov;
+
+    for (i = 0; i < qiov->niov; i++) {
+        /* flags_size is zero-based */
+        cpu_physical_memory_unmap(iov[i].iov_base, !is_write,
+                                  iov[i].iov_len, iov[i].iov_len);
+    }
+
+    return 0;
+}
+
+static int ahci_populate_iov(AHCIDevice *ad, NCQTransferState *ncq_tfs,
+                             int is_write)
+{
+    QEMUIOVector *qiov = &ncq_tfs->qiov;
+    struct iovec *iov;
+    AHCICmdHdr *cmd = ad->cur_cmd;
+    uint32_t opts = le32_to_cpu(cmd->opts);
+    uint64_t prdt_addr = le64_to_cpu(cmd->tbl_addr) + 0x80;
+    int sglist_alloc_hint = opts >> AHCI_CMD_HDR_PRDT_LEN;
+    target_phys_addr_t prdt_len = (sglist_alloc_hint * sizeof(AHCI_SG));
+    target_phys_addr_t real_prdt_len = prdt_len;
+    uint8_t *prdt;
+    int i;
+    int r = 0;
+
+    if (!sglist_alloc_hint) {
+        DPRINTF(ad->port_no, "no sg list given by guest: 0x%08x\n", opts);
+        return -1;
+    }
+
+    /* map PRDT */
+    if (!(prdt = cpu_physical_memory_map(prdt_addr, &prdt_len, 0))){
+        DPRINTF(ad->port_no, "map failed\n");
+        return -1;
+    }
+
+    if (prdt_len < real_prdt_len) {
+        DPRINTF(ad->port_no, "mapped less than expected\n");
+        r = -1;
+        goto out;
+    }
+
+    /* Ensure we have enough space to store the iovecs */
+    if (ncq_tfs->iov_max < sglist_alloc_hint) {
+        ncq_tfs->iov = qemu_realloc(ncq_tfs->iov,
+                                    sglist_alloc_hint * sizeof(struct iovec));
+        ncq_tfs->iov_max = sglist_alloc_hint;
+    }
+    iov = ncq_tfs->iov;
+
+    if (!iov) {
+        DPRINTF(ad->port_no, "out of memory\n");
+        ncq_tfs->iov_max = 0;
+        goto out;
+    }
+
+    /* Get entries in the PRDT, init a qemu sglist accordingly */
+    if (sglist_alloc_hint > 0) {
+        AHCI_SG *tbl = (AHCI_SG *)prdt;
+        void *p;
+
+        for (i = 0; i < sglist_alloc_hint; i++) {
+            target_phys_addr_t len;
+            /* flags_size is zero-based */
+            len = iov[i].iov_len = le32_to_cpu(tbl[i].flags_size) + 1;
+            p = cpu_physical_memory_map(le64_to_cpu(tbl[i].addr),
+                                        &len, !is_write);
+            iov[i].iov_base = p;
+        }
+        qemu_iovec_init_external(qiov, iov, sglist_alloc_hint);
+    }
+
+out:
+    cpu_physical_memory_unmap(prdt, 0, prdt_len, prdt_len);
+    return r;
+}
+
 static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist)
 {
     AHCICmdHdr *cmd = ad->cur_cmd;
@@ -711,7 +792,7 @@ static void ncq_cb(void *opaque, int ret)
     DPRINTF(ncq_tfs->drive->port_no, "NCQ transfer tag %d finished\n",
             ncq_tfs->tag);
 
-    qemu_sglist_destroy(&ncq_tfs->sglist);
+    ahci_iov_destroy(ncq_tfs->drive, &ncq_tfs->qiov, !ncq_tfs->is_read);
     ncq_tfs->used = 0;
 }
 
@@ -747,7 +828,6 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             ncq_tfs->lba, ncq_tfs->lba + ncq_tfs->sector_count - 2,
             s->dev[port].port.ifs[0].nb_sectors - 1);
 
-    ahci_populate_sglist(&s->dev[port], &ncq_tfs->sglist);
     ncq_tfs->tag = tag;
 
     switch(ncq_fis->command) {
@@ -757,9 +837,11 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             ncq_tfs->is_read = 1;
 
             DPRINTF(port, "tag %d aio read %ld\n", ncq_tfs->tag, ncq_tfs->lba);
-            ncq_tfs->aiocb = dma_bdrv_read(ncq_tfs->drive->port.ifs[0].bs,
-                                           &ncq_tfs->sglist, ncq_tfs->lba,
-                                           ncq_cb, ncq_tfs);
+            ahci_populate_iov(&s->dev[port], ncq_tfs, 0);
+            ncq_tfs->aiocb = bdrv_aio_readv(ncq_tfs->drive->port.ifs[0].bs,
+                                            ncq_tfs->lba, &ncq_tfs->qiov,
+                                            ncq_tfs->sector_count, ncq_cb,
+                                            ncq_tfs);
             break;
         case WRITE_FPDMA_QUEUED:
             DPRINTF(port, "NCQ writing %d sectors to LBA %ld, tag %d\n",
@@ -767,9 +849,11 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis,
             ncq_tfs->is_read = 0;
 
             DPRINTF(port, "tag %d aio write %ld\n", ncq_tfs->tag, ncq_tfs->lba);
-            ncq_tfs->aiocb = dma_bdrv_write(ncq_tfs->drive->port.ifs[0].bs,
-                                            &ncq_tfs->sglist, ncq_tfs->lba,
-                                            ncq_cb, ncq_tfs);
+            ahci_populate_iov(&s->dev[port], ncq_tfs, 1);
+            ncq_tfs->aiocb = bdrv_aio_writev(ncq_tfs->drive->port.ifs[0].bs,
+                                             ncq_tfs->lba, &ncq_tfs->qiov,
+                                             ncq_tfs->sector_count, ncq_cb,
+                                             ncq_tfs);
             break;
         default:
             DPRINTF(port, "error: tried to process non-NCQ command as NCQ\n");
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 5f8126a..0824064 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -243,6 +243,9 @@ typedef struct NCQTransferState {
     uint8_t tag;
     int slot;
     int used;
+    QEMUIOVector qiov;
+    struct iovec *iov;
+    int iov_max;
 } NCQTransferState;
 
 struct AHCIDevice {
-- 
1.6.0.2

[Qemu-devel] [PATCH 5/8] ahci: Implement HBA reset

[Alexander Graf]

The ahci code was missing its soft reset functionality. This wasn't really an
issue for Linux guests, but Windows gets confused when the controller doesn't
reset when it tells it so.

Using this patch I can now successfully boot Windows 7 from AHCI using AHCI
enabled SeaBIOS.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7a29925..bd4f8a4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -332,7 +332,7 @@ static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
             case HOST_CTL: /* R/W */
                 if (val & HOST_CTL_RESET) {
                     DPRINTF(-1, "HBA Reset\n");
-                    /* FIXME reset? */
+                    ahci_reset(container_of(s, AHCIPCIState, ahci));
                 } else {
                     s->control_regs.ghc = (val & 0x3) | HOST_CTL_AHCI_EN;
                     ahci_check_irq(s);
@@ -1215,6 +1215,9 @@ void ahci_reset(void *opaque)
     struct AHCIPCIState *d = opaque;
     int i;
 
+    d->ahci.control_regs.irqstatus = 0;
+    d->ahci.control_regs.ghc = 0;
+
     for (i = 0; i < SATA_PORTS; i++) {
         ahci_reset_port(&d->ahci, i);
     }
-- 
1.6.0.2

[Qemu-devel] [PATCH 6/8] ahci: make number of ports runtime determined

[Alexander Graf]

Different AHCI controllers have a different number of ports, so the core
shouldn't care about the amount of ports available.

This patch makes the number of ports available to the AHCI core runtime
configurable, allowing us to have multiple different AHCI implementations
with different amounts of ports.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |   29 +++++++++++++++++++----------
 hw/ide/ahci.h |   10 +++++-----
 hw/ide/ich.c  |    3 ++-
 3 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bd4f8a4..c0bc5ff 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -146,7 +146,7 @@ static void ahci_check_irq(AHCIState *s)
 
     DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
 
-    for (i = 0; i < SATA_PORTS; i++) {
+    for (i = 0; i < s->ports; i++) {
         AHCIPortRegs *pr = &s->dev[i].port_regs;
         if (pr->irq_stat & pr->irq_mask) {
             s->control_regs.irqstatus |= (1 << i);
@@ -300,7 +300,8 @@ static uint32_t ahci_mem_readl(void *ptr, target_phys_addr_t addr)
 
         DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
-               (addr < AHCI_PORT_REGS_END_ADDR)) {
+               (addr < (AHCI_PORT_REGS_START_ADDR +
+                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
         val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
                              addr & AHCI_PORT_ADDR_OFFSET_MASK);
     }
@@ -352,7 +353,8 @@ static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
                 DPRINTF(-1, "write to unknown register 0x%x\n", (unsigned)addr);
         }
     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
-               (addr < AHCI_PORT_REGS_END_ADDR)) {
+               (addr < (AHCI_PORT_REGS_START_ADDR +
+                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
         ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
                         addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
     }
@@ -375,16 +377,16 @@ static void ahci_reg_init(AHCIState *s)
 {
     int i;
 
-    s->control_regs.cap = (SATA_PORTS - 1) |
+    s->control_regs.cap = (s->ports - 1) |
                           (AHCI_NUM_COMMAND_SLOTS << 8) |
                           (AHCI_SUPPORTED_SPEED_GEN1 << AHCI_SUPPORTED_SPEED) |
                           HOST_CAP_NCQ | HOST_CAP_AHCI;
 
-    s->control_regs.impl = (1 << SATA_PORTS) - 1;
+    s->control_regs.impl = (1 << s->ports) - 1;
 
     s->control_regs.version = AHCI_VERSION_1_0;
 
-    for (i = 0; i < SATA_PORTS; i++) {
+    for (i = 0; i < s->ports; i++) {
         s->dev[i].port_state = STATE_RUN;
     }
 }
@@ -1177,17 +1179,19 @@ static const IDEDMAOps ahci_dma_ops = {
     .reset = ahci_dma_reset,
 };
 
-void ahci_init(AHCIState *s, DeviceState *qdev)
+void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
 {
     qemu_irq *irqs;
     int i;
 
+    s->ports = ports;
+    s->dev = qemu_mallocz(sizeof(AHCIDevice) * ports);
     ahci_reg_init(s);
     s->mem = cpu_register_io_memory(ahci_readfn, ahci_writefn, s,
                                     DEVICE_LITTLE_ENDIAN);
-    irqs = qemu_allocate_irqs(ahci_irq_set, s, SATA_PORTS);
+    irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
 
-    for (i = 0; i < SATA_PORTS; i++) {
+    for (i = 0; i < s->ports; i++) {
         AHCIDevice *ad = &s->dev[i];
 
         ide_bus_new(&ad->port, qdev, i);
@@ -1201,6 +1205,11 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
     }
 }
 
+void ahci_uninit(AHCIState *s)
+{
+    qemu_free(s->dev);
+}
+
 void ahci_pci_map(PCIDevice *pci_dev, int region_num,
         pcibus_t addr, pcibus_t size, int type)
 {
@@ -1218,7 +1227,7 @@ void ahci_reset(void *opaque)
     d->ahci.control_regs.irqstatus = 0;
     d->ahci.control_regs.ghc = 0;
 
-    for (i = 0; i < SATA_PORTS; i++) {
+    for (i = 0; i < d->ahci.ports; i++) {
         ahci_reset_port(&d->ahci, i);
     }
 }
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 0824064..eb87dcd 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -165,11 +165,9 @@
 #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
                                             /* Shouldn't this be 0x2c? */
 
-#define SATA_PORTS                         4
-
 #define AHCI_PORT_REGS_START_ADDR          0x100
-#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
 #define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
+#define AHCI_PORT_ADDR_OFFSET_LEN          0x80
 
 #define AHCI_NUM_COMMAND_SLOTS             31
 #define AHCI_SUPPORTED_SPEED               20
@@ -269,9 +267,10 @@ struct AHCIDevice {
 };
 
 typedef struct AHCIState {
-    AHCIDevice dev[SATA_PORTS];
+    AHCIDevice *dev;
     AHCIControlRegs control_regs;
     int mem;
+    int ports;
     qemu_irq irq;
 } AHCIState;
 
@@ -303,7 +302,8 @@ typedef struct NCQFrame {
     uint8_t reserved10;
 } __attribute__ ((packed)) NCQFrame;
 
-void ahci_init(AHCIState *s, DeviceState *qdev);
+void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
+void ahci_uninit(AHCIState *s);
 
 void ahci_pci_map(PCIDevice *pci_dev, int region_num,
         pcibus_t addr, pcibus_t size, int type);
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 70cb766..f242d7a 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -100,7 +100,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
 
     msi_init(dev, 0x50, 1, true, false);
 
-    ahci_init(&d->ahci, &dev->qdev);
+    ahci_init(&d->ahci, &dev->qdev, 6);
     d->ahci.irq = d->card.irq[0];
 
     return 0;
@@ -116,6 +116,7 @@ static int pci_ich9_uninit(PCIDevice *dev)
     }
 
     qemu_unregister_reset(ahci_reset, d);
+    ahci_uninit(&d->ahci);
 
     return 0;
 }
-- 
1.6.0.2

[Qemu-devel] [PATCH 7/8] ahci: free dynamically allocated iovs

[Alexander Graf]

We allocate iovs on the fly now, but also need to free them on uninit.
This patch does that.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c0bc5ff..97aef68 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1207,6 +1207,16 @@ void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
 
 void ahci_uninit(AHCIState *s)
 {
+    int i, j;
+
+    for (i = 0; i < s->ports; i++) {
+        AHCIDevice *ad = &s->dev[i];
+        for (j = 0; j < AHCI_MAX_CMDS; j++) {
+            if (ad->ncq_tfs[j].iov_max) {
+                qemu_free(ad->ncq_tfs[j].iov);
+            }
+        }
+    }
     qemu_free(s->dev);
 }
 
-- 
1.6.0.2

[Qemu-devel] [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

When not using MSI, receiving an interrupt while the interrupt line is active
pulses the interrupt line. Without this, guests don't realize that a new
interrupt occured.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/ide/ahci.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 97aef68..4c920da 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -153,11 +153,10 @@ static void ahci_check_irq(AHCIState *s)
         }
     }
 
+    ahci_irq_lower(s, NULL);
     if (s->control_regs.irqstatus &&
         (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
             ahci_irq_raise(s, NULL);
-    } else {
-        ahci_irq_lower(s, NULL);
     }
 }
 
-- 
1.6.0.2

Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core

[Stefan Hajnoczi]

On Mon, Dec 20, 2010 at 9:13 PM, Alexander Graf <agraf@suse.de> wrote:
> From: Sebastian Herbszt <herbszt@gmx.de>
>
> There are multiple ahci devices out there. The currently implemented ich-9
> is only one of the many. So let's split that one out into a separate file
> to stress the difference.
>
> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  Makefile.objs |    1 +
>  hw/ide/ahci.c |  305 +-------------------------------------------------------
>  hw/ide/ahci.h |  309 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ide/ich.c  |   61 +++++++++++
>  4 files changed, 375 insertions(+), 301 deletions(-)
>  create mode 100644 hw/ide/ahci.h
>  create mode 100644 hw/ide/ich.c

Do you want to add copyright headers on the new files?

Right now it seems there's not much ICH-9 specific stuff but the
motivation behind this patch is good.

Stefan

Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core

[Alexander Graf]

Am 23.12.2010 um 09:23 schrieb Stefan Hajnoczi <stefanha@gmail.com>:

> On Mon, Dec 20, 2010 at 9:13 PM, Alexander Graf <agraf@suse.de> wrote:
>> From: Sebastian Herbszt <herbszt@gmx.de>
>> 
>> There are multiple ahci devices out there. The currently implemented ich-9
>> is only one of the many. So let's split that one out into a separate file
>> to stress the difference.
>> 
>> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>  Makefile.objs |    1 +
>>  hw/ide/ahci.c |  305 +-------------------------------------------------------
>>  hw/ide/ahci.h |  309 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/ide/ich.c  |   61 +++++++++++
>>  4 files changed, 375 insertions(+), 301 deletions(-)
>>  create mode 100644 hw/ide/ahci.h
>>  create mode 100644 hw/ide/ich.c
> 
> Do you want to add copyright headers on the new files?

That's what patch 2 does :)


Alex

> 

Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core

[Sebastian Herbszt]

Alexander Graf wrote:
> Am 23.12.2010 um 09:23 schrieb Stefan Hajnoczi <stefanha@gmail.com>:
> 
> > On Mon, Dec 20, 2010 at 9:13 PM, Alexander Graf <agraf@suse.de> wrote:
> >> From: Sebastian Herbszt <herbszt@gmx.de>
> >> 
> >> There are multiple ahci devices out there. The currently implemented ich-9
> >> is only one of the many. So let's split that one out into a separate file
> >> to stress the difference.
> >> 
> >> Signed-off-by: Sebastian Herbszt <herbszt@gmx.de>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >>  Makefile.objs |    1 +
> >>  hw/ide/ahci.c |  305 +-------------------------------------------------------
> >>  hw/ide/ahci.h |  309 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  hw/ide/ich.c  |   61 +++++++++++
> >>  4 files changed, 375 insertions(+), 301 deletions(-)
> >>  create mode 100644 hw/ide/ahci.h
> >>  create mode 100644 hw/ide/ich.c
> > 
> > Do you want to add copyright headers on the new files?
> 
> That's what patch 2 does :)

Alex, feel free to merge patch 1 and 2 if it's more reasonable.

Sebastian

Re: [Qemu-devel] [PATCH 0/8] Some more AHCI work

[Gerd Hoffmann]

On 12/20/10 22:13, Alexander Graf wrote:
> Clearly, AHCI as is is not perfect yet (intentionally, release early,
> release often, remember?). This patch set makes it work with SeaBIOS
> so booting Windows 7 works flawlessly for me.

Confirmed.  Had to reinstall my win7 test vm today, tried with this 
patch series applied (still applies fine to master btw) and using AHCI 
for both cdrom and hard disk -- worked just fine.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Jan Kiszka]

On 2010-12-20 22:13, Alexander Graf wrote:
> When not using MSI, receiving an interrupt while the interrupt line is active
> pulses the interrupt line. Without this, guests don't realize that a new
> interrupt occured.

This doesn't look OK. The device model should look at the currently used
mode and switch between edge and level triggering accordingly. As it
appears like this is what it already does, this change may just paper
over the real issue.

Jan

> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ide/ahci.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 97aef68..4c920da 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -153,11 +153,10 @@ static void ahci_check_irq(AHCIState *s)
>          }
>      }
>  
> +    ahci_irq_lower(s, NULL);
>      if (s->control_regs.irqstatus &&
>          (s->control_regs.ghc & HOST_CTL_IRQ_EN)) {
>              ahci_irq_raise(s, NULL);
> -    } else {
> -        ahci_irq_lower(s, NULL);
>      }
>  }
>  

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

Jan Kiszka wrote:
> On 2010-12-20 22:13, Alexander Graf wrote:
>   
>> When not using MSI, receiving an interrupt while the interrupt line is active
>> pulses the interrupt line. Without this, guests don't realize that a new
>> interrupt occured.
>>     
>
> This doesn't look OK. The device model should look at the currently used
> mode and switch between edge and level triggering accordingly. As it
> appears like this is what it already does, this change may just paper
> over the real issue.
>   

Well, I have this internal abstraction to make edge and level triggered
interrupt triggering easier. irq_lower is a simple nop for the edge case.


Alex

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Jan Kiszka]

On 2011-01-17 17:00, Alexander Graf wrote:
> Jan Kiszka wrote:
>> On 2010-12-20 22:13, Alexander Graf wrote:
>>   
>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>> pulses the interrupt line. Without this, guests don't realize that a new
>>> interrupt occured.
>>>     
>>
>> This doesn't look OK. The device model should look at the currently used
>> mode and switch between edge and level triggering accordingly. As it
>> appears like this is what it already does, this change may just paper
>> over the real issue.
>>   
> 
> Well, I have this internal abstraction to make edge and level triggered
> interrupt triggering easier. irq_lower is a simple nop for the edge case.
> 

I'm concerned about the artificial edge you generate for the level
triggered case. That's not like real hw behaves. If you need it,
something else might still be broken.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

Jan Kiszka wrote:
> On 2011-01-17 17:00, Alexander Graf wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> On 2010-12-20 22:13, Alexander Graf wrote:
>>>   
>>>       
>>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>>> pulses the interrupt line. Without this, guests don't realize that a new
>>>> interrupt occured.
>>>>     
>>>>         
>>> This doesn't look OK. The device model should look at the currently used
>>> mode and switch between edge and level triggering accordingly. As it
>>> appears like this is what it already does, this change may just paper
>>> over the real issue.
>>>   
>>>       
>> Well, I have this internal abstraction to make edge and level triggered
>> interrupt triggering easier. irq_lower is a simple nop for the edge case.
>>
>>     
>
> I'm concerned about the artificial edge you generate for the level
> triggered case. That's not like real hw behaves. If you need it,
> something else might still be broken.
>   

Hrm. So worst case we generate a spurious interrupt?


Alex

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Jan Kiszka]

On 2011-01-17 17:04, Alexander Graf wrote:
> Jan Kiszka wrote:
>> On 2011-01-17 17:00, Alexander Graf wrote:
>>   
>>> Jan Kiszka wrote:
>>>     
>>>> On 2010-12-20 22:13, Alexander Graf wrote:
>>>>   
>>>>       
>>>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>>>> pulses the interrupt line. Without this, guests don't realize that a new
>>>>> interrupt occured.
>>>>>     
>>>>>         
>>>> This doesn't look OK. The device model should look at the currently used
>>>> mode and switch between edge and level triggering accordingly. As it
>>>> appears like this is what it already does, this change may just paper
>>>> over the real issue.
>>>>   
>>>>       
>>> Well, I have this internal abstraction to make edge and level triggered
>>> interrupt triggering easier. irq_lower is a simple nop for the edge case.
>>>
>>>     
>>
>> I'm concerned about the artificial edge you generate for the level
>> triggered case. That's not like real hw behaves. If you need it,
>> something else might still be broken.
>>   
> 
> Hrm. So worst case we generate a spurious interrupt?
> 

Worse might also be that unknown issue that force you to inject an IRQ
here. We don't know. That's probably worst.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

Jan Kiszka wrote:
> On 2011-01-17 17:04, Alexander Graf wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> On 2011-01-17 17:00, Alexander Graf wrote:
>>>   
>>>       
>>>> Jan Kiszka wrote:
>>>>     
>>>>         
>>>>> On 2010-12-20 22:13, Alexander Graf wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>>>>> pulses the interrupt line. Without this, guests don't realize that a new
>>>>>> interrupt occured.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> This doesn't look OK. The device model should look at the currently used
>>>>> mode and switch between edge and level triggering accordingly. As it
>>>>> appears like this is what it already does, this change may just paper
>>>>> over the real issue.
>>>>>   
>>>>>       
>>>>>           
>>>> Well, I have this internal abstraction to make edge and level triggered
>>>> interrupt triggering easier. irq_lower is a simple nop for the edge case.
>>>>
>>>>     
>>>>         
>>> I'm concerned about the artificial edge you generate for the level
>>> triggered case. That's not like real hw behaves. If you need it,
>>> something else might still be broken.
>>>   
>>>       
>> Hrm. So worst case we generate a spurious interrupt?
>>
>>     
>
> Worse might also be that unknown issue that force you to inject an IRQ
> here. We don't know. That's probably worst.
>   

Well, IIRC the issue was that usually a level high interrupt line would
simply retrigger an interrupt after enabling the interrupt line in the
APIC again. Unless my memory completely fails on me, that didn't happen,
so I added the manual retrigger on a partial command ACK in ahci code.


Alex

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Jan Kiszka]

On 2011-01-17 17:33, Alexander Graf wrote:
> Jan Kiszka wrote:
>> On 2011-01-17 17:04, Alexander Graf wrote:
>>   
>>> Jan Kiszka wrote:
>>>     
>>>> On 2011-01-17 17:00, Alexander Graf wrote:
>>>>   
>>>>       
>>>>> Jan Kiszka wrote:
>>>>>     
>>>>>         
>>>>>> On 2010-12-20 22:13, Alexander Graf wrote:
>>>>>>   
>>>>>>       
>>>>>>           
>>>>>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>>>>>> pulses the interrupt line. Without this, guests don't realize that a new
>>>>>>> interrupt occured.
>>>>>>>     
>>>>>>>         
>>>>>>>             
>>>>>> This doesn't look OK. The device model should look at the currently used
>>>>>> mode and switch between edge and level triggering accordingly. As it
>>>>>> appears like this is what it already does, this change may just paper
>>>>>> over the real issue.
>>>>>>   
>>>>>>       
>>>>>>           
>>>>> Well, I have this internal abstraction to make edge and level triggered
>>>>> interrupt triggering easier. irq_lower is a simple nop for the edge case.
>>>>>
>>>>>     
>>>>>         
>>>> I'm concerned about the artificial edge you generate for the level
>>>> triggered case. That's not like real hw behaves. If you need it,
>>>> something else might still be broken.
>>>>   
>>>>       
>>> Hrm. So worst case we generate a spurious interrupt?
>>>
>>>     
>>
>> Worse might also be that unknown issue that force you to inject an IRQ
>> here. We don't know. That's probably worst.
>>   
> 
> Well, IIRC the issue was that usually a level high interrupt line would
> simply retrigger an interrupt after enabling the interrupt line in the
> APIC again. Unless my memory completely fails on me, that didn't happen,
> so I added the manual retrigger on a partial command ACK in ahci code.
> 

How many other device models require this workaround? And is this a
limitation of a specific irqchip model or of the irq layer (I can't
believe it's the latter though)? All fairly suspicious...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

Jan Kiszka wrote:
> On 2011-01-17 17:33, Alexander Graf wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> On 2011-01-17 17:04, Alexander Graf wrote:
>>>   
>>>       
>>>> Jan Kiszka wrote:
>>>>     
>>>>         
>>>>> On 2011-01-17 17:00, Alexander Graf wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> Jan Kiszka wrote:
>>>>>>     
>>>>>>         
>>>>>>             
>>>>>>> On 2010-12-20 22:13, Alexander Graf wrote:
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>>>> When not using MSI, receiving an interrupt while the interrupt line is active
>>>>>>>> pulses the interrupt line. Without this, guests don't realize that a new
>>>>>>>> interrupt occured.
>>>>>>>>     
>>>>>>>>         
>>>>>>>>             
>>>>>>>>                 
>>>>>>> This doesn't look OK. The device model should look at the currently used
>>>>>>> mode and switch between edge and level triggering accordingly. As it
>>>>>>> appears like this is what it already does, this change may just paper
>>>>>>> over the real issue.
>>>>>>>   
>>>>>>>       
>>>>>>>           
>>>>>>>               
>>>>>> Well, I have this internal abstraction to make edge and level triggered
>>>>>> interrupt triggering easier. irq_lower is a simple nop for the edge case.
>>>>>>
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> I'm concerned about the artificial edge you generate for the level
>>>>> triggered case. That's not like real hw behaves. If you need it,
>>>>> something else might still be broken.
>>>>>   
>>>>>       
>>>>>           
>>>> Hrm. So worst case we generate a spurious interrupt?
>>>>
>>>>     
>>>>         
>>> Worse might also be that unknown issue that force you to inject an IRQ
>>> here. We don't know. That's probably worst.
>>>   
>>>       
>> Well, IIRC the issue was that usually a level high interrupt line would
>> simply retrigger an interrupt after enabling the interrupt line in the
>> APIC again. Unless my memory completely fails on me, that didn't happen,
>> so I added the manual retrigger on a partial command ACK in ahci code.
>>
>>     
>
> How many other device models require this workaround? And is this a
> limitation of a specific irqchip model or of the irq layer (I can't
> believe it's the latter though)? All fairly suspicious...
>   

I don't know :).


Alex

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Gerd Hoffmann]

   Hi,

>> Worse might also be that unknown issue that force you to inject an IRQ
>> here. We don't know. That's probably worst.
>
> Well, IIRC the issue was that usually a level high interrupt line would
> simply retrigger an interrupt after enabling the interrupt line in the
> APIC again.

edge triggered interrupts wouldn't though.

> Unless my memory completely fails on me, that didn't happen,
> so I added the manual retrigger on a partial command ACK in ahci code.

That re-trigger smells like you are not clearing the interrupt line 
where you should.  For starters try calling ahci_check_irq() after guest 
writes to PORT_IRQ_STAT.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

On 18.01.2011, at 10:08, Gerd Hoffmann wrote:

>  Hi,
> 
>>> Worse might also be that unknown issue that force you to inject an IRQ
>>> here. We don't know. That's probably worst.
>> 
>> Well, IIRC the issue was that usually a level high interrupt line would
>> simply retrigger an interrupt after enabling the interrupt line in the
>> APIC again.
> 
> edge triggered interrupts wouldn't though.

The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.

> 
>> Unless my memory completely fails on me, that didn't happen,
>> so I added the manual retrigger on a partial command ACK in ahci code.
> 
> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.

The problem happened when I had the following:

ahci irq bits = 0
<events happen>
ahci irq bits = 1 | 2
irq line trigger
guest clears 1
ahci bits = 2
<no irq line trigger, since we're still irq high>

On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?


Alex

[Qemu-devel] Re: [PATCH 2/8] ahci: split ICH and AHCI even more

[Kevin Wolf]

Am 20.12.2010 22:13, schrieb Alexander Graf:
> Sebastian's patch already did a pretty good job at splitting up ICH-9
> AHCI code and the AHCI core. We need some more though. Copyright was missing,
> the lspci dump belongs to ICH-9, we don't need the AHCI core to have its
> own qdev device duplicate.
> 
> So let's split them a bit more in this patch, making things easier to
> read an understand.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

The license header is still missing in ahci.h.

Kevin

[Qemu-devel] Re: [PATCH 3/8] ahci: send init d2h fis on fis enable

[Kevin Wolf]

Am 20.12.2010 22:13, schrieb Alexander Graf:
> The drive sends a d2h init fis on initialization. Usually, the guest doesn't
> receive fises yet at that point though, so the delivery is deferred.
> 
> Let's reflect that by sending the init fis on fis receive enablement.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Hm... If I read the spec right, the real solution wouldn't be an
init_d2h_sent flag, but implementing a queue for FISes that are received
when the guest hasn't set FRE yet.

I'm not against taking a hack like this, but maybe leave a comment
somewhere at least.

Kevin

[Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers

[Kevin Wolf]

Am 20.12.2010 22:13, schrieb Alexander Graf:
> The DMA helpers incur additional overhead on data transfers. I'm not
> sure we need the additional complexity provided by them. So let's just
> use qiovs directly when running in the fast path (ncq).
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ide/ahci.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/ide/ahci.h |    3 ++
>  2 files changed, 95 insertions(+), 8 deletions(-)

I don't feel comfortable with this one, and I think a while ago we
discussed on IRC why the DMA helpers even exist. If AHCI doesn't need
them, probably nobody needed them.

However, I'm inclined to think that AHCI actually _does_ need them in
corner cases, even though it might not break in all the common cases
that you have tested. Can you explain why only AHCI doesn't need it or
is it just "didn't break for me in practice"?

Where does the overhead in the DMA helpers come from? Can we optimize
this code instead of making the device emulation less correct?

Kevin

[Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined

[Kevin Wolf]

Am 20.12.2010 22:13, schrieb Alexander Graf:
> Different AHCI controllers have a different number of ports, so the core
> shouldn't care about the amount of ports available.
> 
> This patch makes the number of ports available to the AHCI core runtime
> configurable, allowing us to have multiple different AHCI implementations
> with different amounts of ports.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  hw/ide/ahci.c |   29 +++++++++++++++++++----------
>  hw/ide/ahci.h |   10 +++++-----
>  hw/ide/ich.c  |    3 ++-
>  3 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index bd4f8a4..c0bc5ff 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -146,7 +146,7 @@ static void ahci_check_irq(AHCIState *s)
>  
>      DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
>  
> -    for (i = 0; i < SATA_PORTS; i++) {
> +    for (i = 0; i < s->ports; i++) {
>          AHCIPortRegs *pr = &s->dev[i].port_regs;
>          if (pr->irq_stat & pr->irq_mask) {
>              s->control_regs.irqstatus |= (1 << i);
> @@ -300,7 +300,8 @@ static uint32_t ahci_mem_readl(void *ptr, target_phys_addr_t addr)
>  
>          DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
>      } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
> +               (addr < (AHCI_PORT_REGS_START_ADDR +
> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>          val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>                               addr & AHCI_PORT_ADDR_OFFSET_MASK);
>      }
> @@ -352,7 +353,8 @@ static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
>                  DPRINTF(-1, "write to unknown register 0x%x\n", (unsigned)addr);
>          }
>      } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
> +               (addr < (AHCI_PORT_REGS_START_ADDR +
> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>          ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>                          addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
>      }
> @@ -375,16 +377,16 @@ static void ahci_reg_init(AHCIState *s)
>  {
>      int i;
>  
> -    s->control_regs.cap = (SATA_PORTS - 1) |
> +    s->control_regs.cap = (s->ports - 1) |
>                            (AHCI_NUM_COMMAND_SLOTS << 8) |
>                            (AHCI_SUPPORTED_SPEED_GEN1 << AHCI_SUPPORTED_SPEED) |
>                            HOST_CAP_NCQ | HOST_CAP_AHCI;
>  
> -    s->control_regs.impl = (1 << SATA_PORTS) - 1;
> +    s->control_regs.impl = (1 << s->ports) - 1;
>  
>      s->control_regs.version = AHCI_VERSION_1_0;
>  
> -    for (i = 0; i < SATA_PORTS; i++) {
> +    for (i = 0; i < s->ports; i++) {
>          s->dev[i].port_state = STATE_RUN;
>      }
>  }
> @@ -1177,17 +1179,19 @@ static const IDEDMAOps ahci_dma_ops = {
>      .reset = ahci_dma_reset,
>  };
>  
> -void ahci_init(AHCIState *s, DeviceState *qdev)
> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
>  {
>      qemu_irq *irqs;
>      int i;
>  
> +    s->ports = ports;
> +    s->dev = qemu_mallocz(sizeof(AHCIDevice) * ports);
>      ahci_reg_init(s);
>      s->mem = cpu_register_io_memory(ahci_readfn, ahci_writefn, s,
>                                      DEVICE_LITTLE_ENDIAN);
> -    irqs = qemu_allocate_irqs(ahci_irq_set, s, SATA_PORTS);
> +    irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
>  
> -    for (i = 0; i < SATA_PORTS; i++) {
> +    for (i = 0; i < s->ports; i++) {
>          AHCIDevice *ad = &s->dev[i];
>  
>          ide_bus_new(&ad->port, qdev, i);
> @@ -1201,6 +1205,11 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
>      }
>  }
>  
> +void ahci_uninit(AHCIState *s)
> +{
> +    qemu_free(s->dev);
> +}
> +
>  void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>          pcibus_t addr, pcibus_t size, int type)
>  {
> @@ -1218,7 +1227,7 @@ void ahci_reset(void *opaque)
>      d->ahci.control_regs.irqstatus = 0;
>      d->ahci.control_regs.ghc = 0;
>  
> -    for (i = 0; i < SATA_PORTS; i++) {
> +    for (i = 0; i < d->ahci.ports; i++) {
>          ahci_reset_port(&d->ahci, i);
>      }
>  }
> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
> index 0824064..eb87dcd 100644
> --- a/hw/ide/ahci.h
> +++ b/hw/ide/ahci.h
> @@ -165,11 +165,9 @@
>  #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
>                                              /* Shouldn't this be 0x2c? */
>  
> -#define SATA_PORTS                         4
> -
>  #define AHCI_PORT_REGS_START_ADDR          0x100
> -#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
>  #define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
> +#define AHCI_PORT_ADDR_OFFSET_LEN          0x80
>  
>  #define AHCI_NUM_COMMAND_SLOTS             31
>  #define AHCI_SUPPORTED_SPEED               20
> @@ -269,9 +267,10 @@ struct AHCIDevice {
>  };
>  
>  typedef struct AHCIState {
> -    AHCIDevice dev[SATA_PORTS];
> +    AHCIDevice *dev;
>      AHCIControlRegs control_regs;
>      int mem;
> +    int ports;
>      qemu_irq irq;
>  } AHCIState;
>  
> @@ -303,7 +302,8 @@ typedef struct NCQFrame {
>      uint8_t reserved10;
>  } __attribute__ ((packed)) NCQFrame;
>  
> -void ahci_init(AHCIState *s, DeviceState *qdev);
> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
> +void ahci_uninit(AHCIState *s);
>  
>  void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>          pcibus_t addr, pcibus_t size, int type);
> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
> index 70cb766..f242d7a 100644
> --- a/hw/ide/ich.c
> +++ b/hw/ide/ich.c
> @@ -100,7 +100,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>  
>      msi_init(dev, 0x50, 1, true, false);
>  
> -    ahci_init(&d->ahci, &dev->qdev);
> +    ahci_init(&d->ahci, &dev->qdev, 6);
>      d->ahci.irq = d->card.irq[0];

What about a qdev property instead of just hardcoding the value
somewhere else?

Kevin

[Qemu-devel] Re: [PATCH 7/8] ahci: free dynamically allocated iovs

[Kevin Wolf]

Am 20.12.2010 22:13, schrieb Alexander Graf:
> We allocate iovs on the fly now, but also need to free them on uninit.
> This patch does that.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Isn't this a fix for patch 4? If so, please merge it there.

Kevin

[Qemu-devel] Re: [PATCH 3/8] ahci: send init d2h fis on fis enable

[Alexander Graf]

On 18.01.2011, at 13:25, Kevin Wolf wrote:

> Am 20.12.2010 22:13, schrieb Alexander Graf:
>> The drive sends a d2h init fis on initialization. Usually, the guest doesn't
>> receive fises yet at that point though, so the delivery is deferred.
>> 
>> Let's reflect that by sending the init fis on fis receive enablement.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> Hm... If I read the spec right, the real solution wouldn't be an
> init_d2h_sent flag, but implementing a queue for FISes that are received
> when the guest hasn't set FRE yet.
> 
> I'm not against taking a hack like this, but maybe leave a comment
> somewhere at least.

Yes, they'd get queued. In practice it doesn't really matter that much, which is why the hack works :). But you're right - a comment would be nice.


Alex

[Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers

[Alexander Graf]

On 18.01.2011, at 13:35, Kevin Wolf wrote:

> Am 20.12.2010 22:13, schrieb Alexander Graf:
>> The DMA helpers incur additional overhead on data transfers. I'm not
>> sure we need the additional complexity provided by them. So let's just
>> use qiovs directly when running in the fast path (ncq).
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ide/ahci.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> hw/ide/ahci.h |    3 ++
>> 2 files changed, 95 insertions(+), 8 deletions(-)
> 
> I don't feel comfortable with this one, and I think a while ago we
> discussed on IRC why the DMA helpers even exist. If AHCI doesn't need
> them, probably nobody needed them.
> 
> However, I'm inclined to think that AHCI actually _does_ need them in
> corner cases, even though it might not break in all the common cases
> that you have tested. Can you explain why only AHCI doesn't need it or
> is it just "didn't break for me in practice"?
> 

It's the latter.

> Where does the overhead in the DMA helpers come from? Can we optimize
> this code instead of making the device emulation less correct?

Well, dma helpers involve another malloc which is probably the biggest hog. I frankly don't see the point in making it correct for the fast path though. I'd rather like to have a fast block emulation that works with all OSs than an accurate one that emulates something nobody cares about.

Virtio for example doesn't use dma helpers either - they just claim it's not defined in the spec. So if virtio-blk gets away with it, it means that all OSs should never make use of the additional complexity.


Alex

[Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined

[Alexander Graf]

On 18.01.2011, at 13:40, Kevin Wolf wrote:

> Am 20.12.2010 22:13, schrieb Alexander Graf:
>> Different AHCI controllers have a different number of ports, so the core
>> shouldn't care about the amount of ports available.
>> 
>> This patch makes the number of ports available to the AHCI core runtime
>> configurable, allowing us to have multiple different AHCI implementations
>> with different amounts of ports.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> hw/ide/ahci.c |   29 +++++++++++++++++++----------
>> hw/ide/ahci.h |   10 +++++-----
>> hw/ide/ich.c  |    3 ++-
>> 3 files changed, 26 insertions(+), 16 deletions(-)
>> 
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index bd4f8a4..c0bc5ff 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -146,7 +146,7 @@ static void ahci_check_irq(AHCIState *s)
>> 
>>     DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
>> 
>> -    for (i = 0; i < SATA_PORTS; i++) {
>> +    for (i = 0; i < s->ports; i++) {
>>         AHCIPortRegs *pr = &s->dev[i].port_regs;
>>         if (pr->irq_stat & pr->irq_mask) {
>>             s->control_regs.irqstatus |= (1 << i);
>> @@ -300,7 +300,8 @@ static uint32_t ahci_mem_readl(void *ptr, target_phys_addr_t addr)
>> 
>>         DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
>>     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
>> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
>> +               (addr < (AHCI_PORT_REGS_START_ADDR +
>> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>>         val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>>                              addr & AHCI_PORT_ADDR_OFFSET_MASK);
>>     }
>> @@ -352,7 +353,8 @@ static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
>>                 DPRINTF(-1, "write to unknown register 0x%x\n", (unsigned)addr);
>>         }
>>     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
>> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
>> +               (addr < (AHCI_PORT_REGS_START_ADDR +
>> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>>         ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>>                         addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
>>     }
>> @@ -375,16 +377,16 @@ static void ahci_reg_init(AHCIState *s)
>> {
>>     int i;
>> 
>> -    s->control_regs.cap = (SATA_PORTS - 1) |
>> +    s->control_regs.cap = (s->ports - 1) |
>>                           (AHCI_NUM_COMMAND_SLOTS << 8) |
>>                           (AHCI_SUPPORTED_SPEED_GEN1 << AHCI_SUPPORTED_SPEED) |
>>                           HOST_CAP_NCQ | HOST_CAP_AHCI;
>> 
>> -    s->control_regs.impl = (1 << SATA_PORTS) - 1;
>> +    s->control_regs.impl = (1 << s->ports) - 1;
>> 
>>     s->control_regs.version = AHCI_VERSION_1_0;
>> 
>> -    for (i = 0; i < SATA_PORTS; i++) {
>> +    for (i = 0; i < s->ports; i++) {
>>         s->dev[i].port_state = STATE_RUN;
>>     }
>> }
>> @@ -1177,17 +1179,19 @@ static const IDEDMAOps ahci_dma_ops = {
>>     .reset = ahci_dma_reset,
>> };
>> 
>> -void ahci_init(AHCIState *s, DeviceState *qdev)
>> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
>> {
>>     qemu_irq *irqs;
>>     int i;
>> 
>> +    s->ports = ports;
>> +    s->dev = qemu_mallocz(sizeof(AHCIDevice) * ports);
>>     ahci_reg_init(s);
>>     s->mem = cpu_register_io_memory(ahci_readfn, ahci_writefn, s,
>>                                     DEVICE_LITTLE_ENDIAN);
>> -    irqs = qemu_allocate_irqs(ahci_irq_set, s, SATA_PORTS);
>> +    irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
>> 
>> -    for (i = 0; i < SATA_PORTS; i++) {
>> +    for (i = 0; i < s->ports; i++) {
>>         AHCIDevice *ad = &s->dev[i];
>> 
>>         ide_bus_new(&ad->port, qdev, i);
>> @@ -1201,6 +1205,11 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
>>     }
>> }
>> 
>> +void ahci_uninit(AHCIState *s)
>> +{
>> +    qemu_free(s->dev);
>> +}
>> +
>> void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>>         pcibus_t addr, pcibus_t size, int type)
>> {
>> @@ -1218,7 +1227,7 @@ void ahci_reset(void *opaque)
>>     d->ahci.control_regs.irqstatus = 0;
>>     d->ahci.control_regs.ghc = 0;
>> 
>> -    for (i = 0; i < SATA_PORTS; i++) {
>> +    for (i = 0; i < d->ahci.ports; i++) {
>>         ahci_reset_port(&d->ahci, i);
>>     }
>> }
>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>> index 0824064..eb87dcd 100644
>> --- a/hw/ide/ahci.h
>> +++ b/hw/ide/ahci.h
>> @@ -165,11 +165,9 @@
>> #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
>>                                             /* Shouldn't this be 0x2c? */
>> 
>> -#define SATA_PORTS                         4
>> -
>> #define AHCI_PORT_REGS_START_ADDR          0x100
>> -#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
>> #define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
>> +#define AHCI_PORT_ADDR_OFFSET_LEN          0x80
>> 
>> #define AHCI_NUM_COMMAND_SLOTS             31
>> #define AHCI_SUPPORTED_SPEED               20
>> @@ -269,9 +267,10 @@ struct AHCIDevice {
>> };
>> 
>> typedef struct AHCIState {
>> -    AHCIDevice dev[SATA_PORTS];
>> +    AHCIDevice *dev;
>>     AHCIControlRegs control_regs;
>>     int mem;
>> +    int ports;
>>     qemu_irq irq;
>> } AHCIState;
>> 
>> @@ -303,7 +302,8 @@ typedef struct NCQFrame {
>>     uint8_t reserved10;
>> } __attribute__ ((packed)) NCQFrame;
>> 
>> -void ahci_init(AHCIState *s, DeviceState *qdev);
>> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
>> +void ahci_uninit(AHCIState *s);
>> 
>> void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>>         pcibus_t addr, pcibus_t size, int type);
>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>> index 70cb766..f242d7a 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -100,7 +100,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>> 
>>     msi_init(dev, 0x50, 1, true, false);
>> 
>> -    ahci_init(&d->ahci, &dev->qdev);
>> +    ahci_init(&d->ahci, &dev->qdev, 6);
>>     d->ahci.irq = d->card.irq[0];
> 
> What about a qdev property instead of just hardcoding the value
> somewhere else?

I'm not sure. That specific PCI ID simply has 6 ports. So it somehow belongs there :).


Alex

[Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Jan Kiszka]

On 2011-01-18 13:05, Alexander Graf wrote:
> 
> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> 
>>  Hi,
>>
>>>> Worse might also be that unknown issue that force you to inject an IRQ
>>>> here. We don't know. That's probably worst.
>>>
>>> Well, IIRC the issue was that usually a level high interrupt line would
>>> simply retrigger an interrupt after enabling the interrupt line in the
>>> APIC again.
>>
>> edge triggered interrupts wouldn't though.
> 
> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> 
>>
>>> Unless my memory completely fails on me, that didn't happen,
>>> so I added the manual retrigger on a partial command ACK in ahci code.
>>
>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> 
> The problem happened when I had the following:
> 
> ahci irq bits = 0
> <events happen>
> ahci irq bits = 1 | 2
> irq line trigger
> guest clears 1
> ahci bits = 2
> <no irq line trigger, since we're still irq high>
> 
> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> 

No, real devices would simply leave the line asserted as long as there
is a reason to.

So again my question: With which irqchips do you see this effect, kvm's
in-kernel model and/or qemu's user space model? If there is an issue
with retriggering a CPU interrupt while the source is still asserted,
that probably needs to be fixed.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Gerd Hoffmann]

   Hi,

>> edge triggered interrupts wouldn't though.
>
> The code change doesn't change anything for edge triggered
> interrupts
- they work fine. Only !msi (== level) ones are broken.

apic irqs can be both edge and level triggered too ...

>> That re-trigger smells like you are not clearing the interrupt line
>> where you should.  For starters try calling ahci_check_irq() after
>> guest writes to PORT_IRQ_STAT.

>
> The problem happened when I had the following:
>
> ahci irq bits = 0
> <events happen>
> ahci irq bits = 1 | 2
> irq line trigger
> guest clears 1
> ahci bits = 2
> <no irq line trigger, since we're still irq high>



> On normal hardware, the high state of the irq line would simply
trigger another interrupt in the guest when it gets ACKed on the LAPIC.
Somehow it doesn't get triggered here. I don't see why clearing the
interrupt line would help? It's not what real hardware would do, no?

I think the guest and the ahci emulation simply disagree on whenever a 
irq is pending or not.  Guest thinks it has cleared the IRQ, but the 
code path it has taken didn't trigger ahci_irq_lower().

It is probably related to how the per-port and global irq status play 
together.  It isn't covered very well in the specs :-(

If an interrupt condition happens a bit in pr->irq_stat will be set. 
When the contition is enabled the port bit in irqstatus will be set, 
which in turn will trigger an irq.  That is the easy part.

Now the guest checks irqstatus to find the port, then checks 
pr->irq_stat to find the condition, acks it by clearing the bits it has 
seen in pr->irq_stat.  What does happen with the port bit in irqstatus 
now?  Will it be cleared automatically?  Must the guest clear it 
explicitly?  What happens in case another irq condition happened which 
the guest didn't ack (yet), will the guest be able to clear the bit?

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined

[Gerd Hoffmann]

   Hi,

>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>> index 70cb766..f242d7a 100644
>> --- a/hw/ide/ich.c
>> +++ b/hw/ide/ich.c
>> @@ -100,7 +100,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>>
>>       msi_init(dev, 0x50, 1, true, false);
>>
>> -    ahci_init(&d->ahci,&dev->qdev);
>> +    ahci_init(&d->ahci,&dev->qdev, 6);
>>       d->ahci.irq = d->card.irq[0];
>
> What about a qdev property instead of just hardcoding the value
> somewhere else?

That particular piece of emulated hardware (ich9-ahci) actually has 6 
ports.  ich7 has 4 ports.  A thin hardware-specific wrapper which 
hardcodes this stuff (and pci ids and maybe some other minor 
differences) looks fine to me.  Maybe alex should add a ich7 variant 
just to prove the point ;)

cheers,
   Gerd

[Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers

[Stefan Hajnoczi]

On Tue, Jan 18, 2011 at 01:45:40PM +0100, Alexander Graf wrote:
> 
> On 18.01.2011, at 13:35, Kevin Wolf wrote:
> 
> > Am 20.12.2010 22:13, schrieb Alexander Graf:
> >> The DMA helpers incur additional overhead on data transfers. I'm not
> >> sure we need the additional complexity provided by them. So let's just
> >> use qiovs directly when running in the fast path (ncq).
> >> 
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> hw/ide/ahci.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >> hw/ide/ahci.h |    3 ++
> >> 2 files changed, 95 insertions(+), 8 deletions(-)
> > 
> > I don't feel comfortable with this one, and I think a while ago we
> > discussed on IRC why the DMA helpers even exist. If AHCI doesn't need
> > them, probably nobody needed them.
> > 
> > However, I'm inclined to think that AHCI actually _does_ need them in
> > corner cases, even though it might not break in all the common cases
> > that you have tested. Can you explain why only AHCI doesn't need it or
> > is it just "didn't break for me in practice"?
> > 
> 
> It's the latter.
> 
> > Where does the overhead in the DMA helpers come from? Can we optimize
> > this code instead of making the device emulation less correct?
> 
> Well, dma helpers involve another malloc which is probably the biggest hog. I frankly don't see the point in making it correct for the fast path though. I'd rather like to have a fast block emulation that works with all OSs than an accurate one that emulates something nobody cares about.
> 
> Virtio for example doesn't use dma helpers either - they just claim it's not defined in the spec. So if virtio-blk gets away with it, it means that all OSs should never make use of the additional complexity.

>From what I can tell DMA helpers is common AIO request code plus:

1. It handles map failure using cpu_register_map_client().
2. It handles short maps that are unable to map a full sglist element.

These two requirements are due to QEMU's guest memory mapping API.  IIUC
the limitations on that API are due to a limited amount of bounce buffer
space being used for some targets allowing mapping of non-RAM memory.

Perhaps this means that AHCI does not work on those targets if you
decide to send non-RAM pages to disk?

I'd be interested in understanding how this all works and how the QEMU
RAM API that Anthony and Alex Williamson have been playing with comes
into play.

Stefan

[Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers

[Kevin Wolf]

Am 18.01.2011 14:14, schrieb Stefan Hajnoczi:
> On Tue, Jan 18, 2011 at 01:45:40PM +0100, Alexander Graf wrote:
>>
>> On 18.01.2011, at 13:35, Kevin Wolf wrote:
>>
>>> Am 20.12.2010 22:13, schrieb Alexander Graf:
>>>> The DMA helpers incur additional overhead on data transfers. I'm not
>>>> sure we need the additional complexity provided by them. So let's just
>>>> use qiovs directly when running in the fast path (ncq).
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> hw/ide/ahci.c |  100 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>> hw/ide/ahci.h |    3 ++
>>>> 2 files changed, 95 insertions(+), 8 deletions(-)
>>>
>>> I don't feel comfortable with this one, and I think a while ago we
>>> discussed on IRC why the DMA helpers even exist. If AHCI doesn't need
>>> them, probably nobody needed them.
>>>
>>> However, I'm inclined to think that AHCI actually _does_ need them in
>>> corner cases, even though it might not break in all the common cases
>>> that you have tested. Can you explain why only AHCI doesn't need it or
>>> is it just "didn't break for me in practice"?
>>>
>>
>> It's the latter.
>>
>>> Where does the overhead in the DMA helpers come from? Can we optimize
>>> this code instead of making the device emulation less correct?
>>
>> Well, dma helpers involve another malloc which is probably the biggest hog.

If you can get around this malloc in AHCI (IIUC, you do it by reusing
the old sglist buffer?), we can probably do something similar in the
generic DMA helper code and have IDE etc. benefit from it as well.

> I frankly don't see the point in making it correct for the fast path though. I'd rather like to have a fast block emulation that works with all OSs than an accurate one that emulates something nobody cares about.

Sorry, but "Windows and Linux" != "all OSes".

I'm sure there is a way that gives us correctness _and_ performance.

>> Virtio for example doesn't use dma helpers either - they just claim it's not defined in the spec. So if virtio-blk gets away with it, it means that all OSs should never make use of the additional complexity.
> 
> From what I can tell DMA helpers is common AIO request code plus:
> 
> 1. It handles map failure using cpu_register_map_client().
> 2. It handles short maps that are unable to map a full sglist element.
> 
> These two requirements are due to QEMU's guest memory mapping API.  IIUC
> the limitations on that API are due to a limited amount of bounce buffer
> space being used for some targets allowing mapping of non-RAM memory.
> 
> Perhaps this means that AHCI does not work on those targets if you
> decide to send non-RAM pages to disk?
> 
> I'd be interested in understanding how this all works and how the QEMU
> RAM API that Anthony and Alex Williamson have been playing with comes
> into play.

Yeah, I don't know the details either. I hope that Anthony will comment
on it.

Kevin

[Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined

[Kevin Wolf]

Am 18.01.2011 13:46, schrieb Alexander Graf:
> 
> On 18.01.2011, at 13:40, Kevin Wolf wrote:
> 
>> Am 20.12.2010 22:13, schrieb Alexander Graf:
>>> Different AHCI controllers have a different number of ports, so the core
>>> shouldn't care about the amount of ports available.
>>>
>>> This patch makes the number of ports available to the AHCI core runtime
>>> configurable, allowing us to have multiple different AHCI implementations
>>> with different amounts of ports.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>> hw/ide/ahci.c |   29 +++++++++++++++++++----------
>>> hw/ide/ahci.h |   10 +++++-----
>>> hw/ide/ich.c  |    3 ++-
>>> 3 files changed, 26 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>>> index bd4f8a4..c0bc5ff 100644
>>> --- a/hw/ide/ahci.c
>>> +++ b/hw/ide/ahci.c
>>> @@ -146,7 +146,7 @@ static void ahci_check_irq(AHCIState *s)
>>>
>>>     DPRINTF(-1, "check irq %#x\n", s->control_regs.irqstatus);
>>>
>>> -    for (i = 0; i < SATA_PORTS; i++) {
>>> +    for (i = 0; i < s->ports; i++) {
>>>         AHCIPortRegs *pr = &s->dev[i].port_regs;
>>>         if (pr->irq_stat & pr->irq_mask) {
>>>             s->control_regs.irqstatus |= (1 << i);
>>> @@ -300,7 +300,8 @@ static uint32_t ahci_mem_readl(void *ptr, target_phys_addr_t addr)
>>>
>>>         DPRINTF(-1, "(addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
>>>     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
>>> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
>>> +               (addr < (AHCI_PORT_REGS_START_ADDR +
>>> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>>>         val = ahci_port_read(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>>>                              addr & AHCI_PORT_ADDR_OFFSET_MASK);
>>>     }
>>> @@ -352,7 +353,8 @@ static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
>>>                 DPRINTF(-1, "write to unknown register 0x%x\n", (unsigned)addr);
>>>         }
>>>     } else if ((addr >= AHCI_PORT_REGS_START_ADDR) &&
>>> -               (addr < AHCI_PORT_REGS_END_ADDR)) {
>>> +               (addr < (AHCI_PORT_REGS_START_ADDR +
>>> +                (s->ports * AHCI_PORT_ADDR_OFFSET_LEN)))) {
>>>         ahci_port_write(s, (addr - AHCI_PORT_REGS_START_ADDR) >> 7,
>>>                         addr & AHCI_PORT_ADDR_OFFSET_MASK, val);
>>>     }
>>> @@ -375,16 +377,16 @@ static void ahci_reg_init(AHCIState *s)
>>> {
>>>     int i;
>>>
>>> -    s->control_regs.cap = (SATA_PORTS - 1) |
>>> +    s->control_regs.cap = (s->ports - 1) |
>>>                           (AHCI_NUM_COMMAND_SLOTS << 8) |
>>>                           (AHCI_SUPPORTED_SPEED_GEN1 << AHCI_SUPPORTED_SPEED) |
>>>                           HOST_CAP_NCQ | HOST_CAP_AHCI;
>>>
>>> -    s->control_regs.impl = (1 << SATA_PORTS) - 1;
>>> +    s->control_regs.impl = (1 << s->ports) - 1;
>>>
>>>     s->control_regs.version = AHCI_VERSION_1_0;
>>>
>>> -    for (i = 0; i < SATA_PORTS; i++) {
>>> +    for (i = 0; i < s->ports; i++) {
>>>         s->dev[i].port_state = STATE_RUN;
>>>     }
>>> }
>>> @@ -1177,17 +1179,19 @@ static const IDEDMAOps ahci_dma_ops = {
>>>     .reset = ahci_dma_reset,
>>> };
>>>
>>> -void ahci_init(AHCIState *s, DeviceState *qdev)
>>> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports)
>>> {
>>>     qemu_irq *irqs;
>>>     int i;
>>>
>>> +    s->ports = ports;
>>> +    s->dev = qemu_mallocz(sizeof(AHCIDevice) * ports);
>>>     ahci_reg_init(s);
>>>     s->mem = cpu_register_io_memory(ahci_readfn, ahci_writefn, s,
>>>                                     DEVICE_LITTLE_ENDIAN);
>>> -    irqs = qemu_allocate_irqs(ahci_irq_set, s, SATA_PORTS);
>>> +    irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
>>>
>>> -    for (i = 0; i < SATA_PORTS; i++) {
>>> +    for (i = 0; i < s->ports; i++) {
>>>         AHCIDevice *ad = &s->dev[i];
>>>
>>>         ide_bus_new(&ad->port, qdev, i);
>>> @@ -1201,6 +1205,11 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
>>>     }
>>> }
>>>
>>> +void ahci_uninit(AHCIState *s)
>>> +{
>>> +    qemu_free(s->dev);
>>> +}
>>> +
>>> void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>>>         pcibus_t addr, pcibus_t size, int type)
>>> {
>>> @@ -1218,7 +1227,7 @@ void ahci_reset(void *opaque)
>>>     d->ahci.control_regs.irqstatus = 0;
>>>     d->ahci.control_regs.ghc = 0;
>>>
>>> -    for (i = 0; i < SATA_PORTS; i++) {
>>> +    for (i = 0; i < d->ahci.ports; i++) {
>>>         ahci_reset_port(&d->ahci, i);
>>>     }
>>> }
>>> diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
>>> index 0824064..eb87dcd 100644
>>> --- a/hw/ide/ahci.h
>>> +++ b/hw/ide/ahci.h
>>> @@ -165,11 +165,9 @@
>>> #define AHCI_GENERIC_HOST_CONTROL_REGS_MAX_ADDR 0x20
>>>                                             /* Shouldn't this be 0x2c? */
>>>
>>> -#define SATA_PORTS                         4
>>> -
>>> #define AHCI_PORT_REGS_START_ADDR          0x100
>>> -#define AHCI_PORT_REGS_END_ADDR (AHCI_PORT_REGS_START_ADDR + SATA_PORTS * 0x80)
>>> #define AHCI_PORT_ADDR_OFFSET_MASK         0x7f
>>> +#define AHCI_PORT_ADDR_OFFSET_LEN          0x80
>>>
>>> #define AHCI_NUM_COMMAND_SLOTS             31
>>> #define AHCI_SUPPORTED_SPEED               20
>>> @@ -269,9 +267,10 @@ struct AHCIDevice {
>>> };
>>>
>>> typedef struct AHCIState {
>>> -    AHCIDevice dev[SATA_PORTS];
>>> +    AHCIDevice *dev;
>>>     AHCIControlRegs control_regs;
>>>     int mem;
>>> +    int ports;
>>>     qemu_irq irq;
>>> } AHCIState;
>>>
>>> @@ -303,7 +302,8 @@ typedef struct NCQFrame {
>>>     uint8_t reserved10;
>>> } __attribute__ ((packed)) NCQFrame;
>>>
>>> -void ahci_init(AHCIState *s, DeviceState *qdev);
>>> +void ahci_init(AHCIState *s, DeviceState *qdev, int ports);
>>> +void ahci_uninit(AHCIState *s);
>>>
>>> void ahci_pci_map(PCIDevice *pci_dev, int region_num,
>>>         pcibus_t addr, pcibus_t size, int type);
>>> diff --git a/hw/ide/ich.c b/hw/ide/ich.c
>>> index 70cb766..f242d7a 100644
>>> --- a/hw/ide/ich.c
>>> +++ b/hw/ide/ich.c
>>> @@ -100,7 +100,7 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
>>>
>>>     msi_init(dev, 0x50, 1, true, false);
>>>
>>> -    ahci_init(&d->ahci, &dev->qdev);
>>> +    ahci_init(&d->ahci, &dev->qdev, 6);
>>>     d->ahci.irq = d->card.irq[0];
>>
>> What about a qdev property instead of just hardcoding the value
>> somewhere else?
> 
> I'm not sure. That specific PCI ID simply has 6 ports. So it somehow belongs there :).

Okay, makes sense.

Kevin

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Aurelien Jarno]

On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
> On 2011-01-18 13:05, Alexander Graf wrote:
> > 
> > On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> > 
> >>  Hi,
> >>
> >>>> Worse might also be that unknown issue that force you to inject an IRQ
> >>>> here. We don't know. That's probably worst.
> >>>
> >>> Well, IIRC the issue was that usually a level high interrupt line would
> >>> simply retrigger an interrupt after enabling the interrupt line in the
> >>> APIC again.
> >>
> >> edge triggered interrupts wouldn't though.
> > 
> > The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> > 
> >>
> >>> Unless my memory completely fails on me, that didn't happen,
> >>> so I added the manual retrigger on a partial command ACK in ahci code.
> >>
> >> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> > 
> > The problem happened when I had the following:
> > 
> > ahci irq bits = 0
> > <events happen>
> > ahci irq bits = 1 | 2
> > irq line trigger
> > guest clears 1
> > ahci bits = 2
> > <no irq line trigger, since we're still irq high>
> > 
> > On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> > 
> 
> No, real devices would simply leave the line asserted as long as there
> is a reason to.
> 
> So again my question: With which irqchips do you see this effect, kvm's
> in-kernel model and/or qemu's user space model? If there is an issue
> with retriggering a CPU interrupt while the source is still asserted,
> that probably needs to be fixed.
> 

I guess it might be related to a problem identified long time ago on
some targets, and that leads to the following #ifdef in i8259.c:

| /* all targets should do this rather than acking the IRQ in the cpu */
| #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)

For your information it has been fixed on MIPS in commit 
4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.

Basically when an interrupt line is high, the interrupt is getting
delivered to the CPU. This part works correctly on x86. The CPU will
take a corresponding action, basically either disabling the interrupt
at the CPU or controller level or doing something on the device so that
it lower its IRQ line. This new IRQ line level should then propagate
through the various controllers, which should also lower their IRQ line
if no other interrupt line are active. This ACK process should then
continue up to the CPU.

For x86 the interrupt state is cleared as soon as the interrupt is
signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
is still pending, it won't be seen by the CPU. It's probably what you
observed with AHCI. 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Edgar E. Iglesias]

On Sat, Jan 22, 2011 at 02:13:03PM +0100, Aurelien Jarno wrote:
> On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
> > On 2011-01-18 13:05, Alexander Graf wrote:
> > > 
> > > On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> > > 
> > >>  Hi,
> > >>
> > >>>> Worse might also be that unknown issue that force you to inject an IRQ
> > >>>> here. We don't know. That's probably worst.
> > >>>
> > >>> Well, IIRC the issue was that usually a level high interrupt line would
> > >>> simply retrigger an interrupt after enabling the interrupt line in the
> > >>> APIC again.
> > >>
> > >> edge triggered interrupts wouldn't though.
> > > 
> > > The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> > > 
> > >>
> > >>> Unless my memory completely fails on me, that didn't happen,
> > >>> so I added the manual retrigger on a partial command ACK in ahci code.
> > >>
> > >> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> > > 
> > > The problem happened when I had the following:
> > > 
> > > ahci irq bits = 0
> > > <events happen>
> > > ahci irq bits = 1 | 2
> > > irq line trigger
> > > guest clears 1
> > > ahci bits = 2
> > > <no irq line trigger, since we're still irq high>
> > > 
> > > On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> > > 
> > 
> > No, real devices would simply leave the line asserted as long as there
> > is a reason to.
> > 
> > So again my question: With which irqchips do you see this effect, kvm's
> > in-kernel model and/or qemu's user space model? If there is an issue
> > with retriggering a CPU interrupt while the source is still asserted,
> > that probably needs to be fixed.
> > 
> 
> I guess it might be related to a problem identified long time ago on
> some targets, and that leads to the following #ifdef in i8259.c:
> 
> | /* all targets should do this rather than acking the IRQ in the cpu */
> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
> 
> For your information it has been fixed on MIPS in commit 
> 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
> 
> Basically when an interrupt line is high, the interrupt is getting
> delivered to the CPU. This part works correctly on x86. The CPU will
> take a corresponding action, basically either disabling the interrupt
> at the CPU or controller level or doing something on the device so that
> it lower its IRQ line. This new IRQ line level should then propagate
> through the various controllers, which should also lower their IRQ line
> if no other interrupt line are active. This ACK process should then
> continue up to the CPU.

I totally agree.

 
> For x86 the interrupt state is cleared as soon as the interrupt is
> signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
> is still pending, it won't be seen by the CPU. It's probably what you
> observed with AHCI. 

Yes, essentially, the CPU_INTERRUPT_HARD signal is an input to the CPU.
The CPU cannot drive it directly. To lower it, it must take some kind
of indirect action (IO or whatever) to clear the condition that is
forcing it high. Any assignments to clear or set the CPU_INTERRUPT_HARD
signal from within the CPU core are likely wrong..

FWIW, PPC code in cpu-exec.c:443 looks suspicious aswell...

Cheers

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

On 22.01.2011, at 14:13, Aurelien Jarno wrote:

> On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
>> On 2011-01-18 13:05, Alexander Graf wrote:
>>> 
>>> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
>>> 
>>>> Hi,
>>>> 
>>>>>> Worse might also be that unknown issue that force you to inject an IRQ
>>>>>> here. We don't know. That's probably worst.
>>>>> 
>>>>> Well, IIRC the issue was that usually a level high interrupt line would
>>>>> simply retrigger an interrupt after enabling the interrupt line in the
>>>>> APIC again.
>>>> 
>>>> edge triggered interrupts wouldn't though.
>>> 
>>> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
>>> 
>>>> 
>>>>> Unless my memory completely fails on me, that didn't happen,
>>>>> so I added the manual retrigger on a partial command ACK in ahci code.
>>>> 
>>>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
>>> 
>>> The problem happened when I had the following:
>>> 
>>> ahci irq bits = 0
>>> <events happen>
>>> ahci irq bits = 1 | 2
>>> irq line trigger
>>> guest clears 1
>>> ahci bits = 2
>>> <no irq line trigger, since we're still irq high>
>>> 
>>> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
>>> 
>> 
>> No, real devices would simply leave the line asserted as long as there
>> is a reason to.
>> 
>> So again my question: With which irqchips do you see this effect, kvm's
>> in-kernel model and/or qemu's user space model? If there is an issue
>> with retriggering a CPU interrupt while the source is still asserted,
>> that probably needs to be fixed.
>> 
> 
> I guess it might be related to a problem identified long time ago on
> some targets, and that leads to the following #ifdef in i8259.c:
> 
> | /* all targets should do this rather than acking the IRQ in the cpu */
> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
> 
> For your information it has been fixed on MIPS in commit 
> 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
> 
> Basically when an interrupt line is high, the interrupt is getting
> delivered to the CPU. This part works correctly on x86. The CPU will
> take a corresponding action, basically either disabling the interrupt
> at the CPU or controller level or doing something on the device so that
> it lower its IRQ line. This new IRQ line level should then propagate
> through the various controllers, which should also lower their IRQ line
> if no other interrupt line are active. This ACK process should then
> continue up to the CPU.
> 
> For x86 the interrupt state is cleared as soon as the interrupt is
> signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
> is still pending, it won't be seen by the CPU. It's probably what you
> observed with AHCI. 

I'm not sure what the best way to solve it would be, but maybe a callback on iret would work? Then the interrupt can be checked on again.

Alternatively, env->interrupt_request really should get cleared by the LAPIC when the interrupt line is pulled down there.


Alex

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Alexander Graf]

On 22.01.2011, at 15:14, Edgar E. Iglesias wrote:

> On Sat, Jan 22, 2011 at 02:13:03PM +0100, Aurelien Jarno wrote:
>> On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
>>> On 2011-01-18 13:05, Alexander Graf wrote:
>>>> 
>>>> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
>>>> 
>>>>> Hi,
>>>>> 
>>>>>>> Worse might also be that unknown issue that force you to inject an IRQ
>>>>>>> here. We don't know. That's probably worst.
>>>>>> 
>>>>>> Well, IIRC the issue was that usually a level high interrupt line would
>>>>>> simply retrigger an interrupt after enabling the interrupt line in the
>>>>>> APIC again.
>>>>> 
>>>>> edge triggered interrupts wouldn't though.
>>>> 
>>>> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
>>>> 
>>>>> 
>>>>>> Unless my memory completely fails on me, that didn't happen,
>>>>>> so I added the manual retrigger on a partial command ACK in ahci code.
>>>>> 
>>>>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
>>>> 
>>>> The problem happened when I had the following:
>>>> 
>>>> ahci irq bits = 0
>>>> <events happen>
>>>> ahci irq bits = 1 | 2
>>>> irq line trigger
>>>> guest clears 1
>>>> ahci bits = 2
>>>> <no irq line trigger, since we're still irq high>
>>>> 
>>>> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
>>>> 
>>> 
>>> No, real devices would simply leave the line asserted as long as there
>>> is a reason to.
>>> 
>>> So again my question: With which irqchips do you see this effect, kvm's
>>> in-kernel model and/or qemu's user space model? If there is an issue
>>> with retriggering a CPU interrupt while the source is still asserted,
>>> that probably needs to be fixed.
>>> 
>> 
>> I guess it might be related to a problem identified long time ago on
>> some targets, and that leads to the following #ifdef in i8259.c:
>> 
>> | /* all targets should do this rather than acking the IRQ in the cpu */
>> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
>> 
>> For your information it has been fixed on MIPS in commit 
>> 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
>> 
>> Basically when an interrupt line is high, the interrupt is getting
>> delivered to the CPU. This part works correctly on x86. The CPU will
>> take a corresponding action, basically either disabling the interrupt
>> at the CPU or controller level or doing something on the device so that
>> it lower its IRQ line. This new IRQ line level should then propagate
>> through the various controllers, which should also lower their IRQ line
>> if no other interrupt line are active. This ACK process should then
>> continue up to the CPU.
> 
> I totally agree.
> 
> 
>> For x86 the interrupt state is cleared as soon as the interrupt is
>> signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
>> is still pending, it won't be seen by the CPU. It's probably what you
>> observed with AHCI. 
> 
> Yes, essentially, the CPU_INTERRUPT_HARD signal is an input to the CPU.
> The CPU cannot drive it directly. To lower it, it must take some kind
> of indirect action (IO or whatever) to clear the condition that is
> forcing it high. Any assignments to clear or set the CPU_INTERRUPT_HARD
> signal from within the CPU core are likely wrong..
> 
> FWIW, PPC code in cpu-exec.c:443 looks suspicious aswell...

How's that suspicious? As long as pending_interrupts is only reset on actual interrupt delivery, everything's fine. Interrupts like the decrementor are not level based on some PPCs, so the actual semantics are implementation dependent.

Either way, I agree that getting interrupts right is very hard :).


Alex

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Aurelien Jarno]

On Sat, Jan 22, 2011 at 03:28:06PM +0100, Alexander Graf wrote:
> 
> On 22.01.2011, at 14:13, Aurelien Jarno wrote:
> 
> > On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
> >> On 2011-01-18 13:05, Alexander Graf wrote:
> >>> 
> >>> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> >>> 
> >>>> Hi,
> >>>> 
> >>>>>> Worse might also be that unknown issue that force you to inject an IRQ
> >>>>>> here. We don't know. That's probably worst.
> >>>>> 
> >>>>> Well, IIRC the issue was that usually a level high interrupt line would
> >>>>> simply retrigger an interrupt after enabling the interrupt line in the
> >>>>> APIC again.
> >>>> 
> >>>> edge triggered interrupts wouldn't though.
> >>> 
> >>> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> >>> 
> >>>> 
> >>>>> Unless my memory completely fails on me, that didn't happen,
> >>>>> so I added the manual retrigger on a partial command ACK in ahci code.
> >>>> 
> >>>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> >>> 
> >>> The problem happened when I had the following:
> >>> 
> >>> ahci irq bits = 0
> >>> <events happen>
> >>> ahci irq bits = 1 | 2
> >>> irq line trigger
> >>> guest clears 1
> >>> ahci bits = 2
> >>> <no irq line trigger, since we're still irq high>
> >>> 
> >>> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> >>> 
> >> 
> >> No, real devices would simply leave the line asserted as long as there
> >> is a reason to.
> >> 
> >> So again my question: With which irqchips do you see this effect, kvm's
> >> in-kernel model and/or qemu's user space model? If there is an issue
> >> with retriggering a CPU interrupt while the source is still asserted,
> >> that probably needs to be fixed.
> >> 
> > 
> > I guess it might be related to a problem identified long time ago on
> > some targets, and that leads to the following #ifdef in i8259.c:
> > 
> > | /* all targets should do this rather than acking the IRQ in the cpu */
> > | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
> > 
> > For your information it has been fixed on MIPS in commit 
> > 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
> > 
> > Basically when an interrupt line is high, the interrupt is getting
> > delivered to the CPU. This part works correctly on x86. The CPU will
> > take a corresponding action, basically either disabling the interrupt
> > at the CPU or controller level or doing something on the device so that
> > it lower its IRQ line. This new IRQ line level should then propagate
> > through the various controllers, which should also lower their IRQ line
> > if no other interrupt line are active. This ACK process should then
> > continue up to the CPU.
> > 
> > For x86 the interrupt state is cleared as soon as the interrupt is
> > signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
> > is still pending, it won't be seen by the CPU. It's probably what you
> > observed with AHCI. 
> 
> I'm not sure what the best way to solve it would be, but maybe a callback on iret would work? Then the interrupt can be checked on again.

This looks really like a hack, and the one you put in AHCI is probably
better than this one

> Alternatively, env->interrupt_request really should get cleared by the LAPIC when the interrupt line is pulled down there.

I think this is the way to go, as it matches what happens on real
hardware. The changes might be a bit more invasive though.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Aurelien Jarno]

On Sat, Jan 22, 2011 at 03:32:36PM +0100, Alexander Graf wrote:
> 
> On 22.01.2011, at 15:14, Edgar E. Iglesias wrote:
> 
> > On Sat, Jan 22, 2011 at 02:13:03PM +0100, Aurelien Jarno wrote:
> >> On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
> >>> On 2011-01-18 13:05, Alexander Graf wrote:
> >>>> 
> >>>> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> >>>> 
> >>>>> Hi,
> >>>>> 
> >>>>>>> Worse might also be that unknown issue that force you to inject an IRQ
> >>>>>>> here. We don't know. That's probably worst.
> >>>>>> 
> >>>>>> Well, IIRC the issue was that usually a level high interrupt line would
> >>>>>> simply retrigger an interrupt after enabling the interrupt line in the
> >>>>>> APIC again.
> >>>>> 
> >>>>> edge triggered interrupts wouldn't though.
> >>>> 
> >>>> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> >>>> 
> >>>>> 
> >>>>>> Unless my memory completely fails on me, that didn't happen,
> >>>>>> so I added the manual retrigger on a partial command ACK in ahci code.
> >>>>> 
> >>>>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> >>>> 
> >>>> The problem happened when I had the following:
> >>>> 
> >>>> ahci irq bits = 0
> >>>> <events happen>
> >>>> ahci irq bits = 1 | 2
> >>>> irq line trigger
> >>>> guest clears 1
> >>>> ahci bits = 2
> >>>> <no irq line trigger, since we're still irq high>
> >>>> 
> >>>> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> >>>> 
> >>> 
> >>> No, real devices would simply leave the line asserted as long as there
> >>> is a reason to.
> >>> 
> >>> So again my question: With which irqchips do you see this effect, kvm's
> >>> in-kernel model and/or qemu's user space model? If there is an issue
> >>> with retriggering a CPU interrupt while the source is still asserted,
> >>> that probably needs to be fixed.
> >>> 
> >> 
> >> I guess it might be related to a problem identified long time ago on
> >> some targets, and that leads to the following #ifdef in i8259.c:
> >> 
> >> | /* all targets should do this rather than acking the IRQ in the cpu */
> >> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
> >> 
> >> For your information it has been fixed on MIPS in commit 
> >> 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
> >> 
> >> Basically when an interrupt line is high, the interrupt is getting
> >> delivered to the CPU. This part works correctly on x86. The CPU will
> >> take a corresponding action, basically either disabling the interrupt
> >> at the CPU or controller level or doing something on the device so that
> >> it lower its IRQ line. This new IRQ line level should then propagate
> >> through the various controllers, which should also lower their IRQ line
> >> if no other interrupt line are active. This ACK process should then
> >> continue up to the CPU.
> > 
> > I totally agree.
> > 
> > 
> >> For x86 the interrupt state is cleared as soon as the interrupt is
> >> signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
> >> is still pending, it won't be seen by the CPU. It's probably what you
> >> observed with AHCI. 
> > 
> > Yes, essentially, the CPU_INTERRUPT_HARD signal is an input to the CPU.
> > The CPU cannot drive it directly. To lower it, it must take some kind
> > of indirect action (IO or whatever) to clear the condition that is
> > forcing it high. Any assignments to clear or set the CPU_INTERRUPT_HARD
> > signal from within the CPU core are likely wrong..
> > 
> > FWIW, PPC code in cpu-exec.c:443 looks suspicious aswell...
> 
> How's that suspicious? As long as pending_interrupts is only reset on actual interrupt delivery, everything's fine. Interrupts like the decrementor are not level based on some PPCs, so the actual semantics are implementation dependent.
> 

It's suspicious because it's not the right place to do that. It should
be done in the interrupt controller. This seems to be already done in
hw/ppc.c when updating pending_interrupts, so this code seems to be
useless.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts

[Edgar E. Iglesias]

On Sat, Jan 22, 2011 at 03:40:34PM +0100, Aurelien Jarno wrote:
> On Sat, Jan 22, 2011 at 03:32:36PM +0100, Alexander Graf wrote:
> > 
> > On 22.01.2011, at 15:14, Edgar E. Iglesias wrote:
> > 
> > > On Sat, Jan 22, 2011 at 02:13:03PM +0100, Aurelien Jarno wrote:
> > >> On Tue, Jan 18, 2011 at 01:58:25PM +0100, Jan Kiszka wrote:
> > >>> On 2011-01-18 13:05, Alexander Graf wrote:
> > >>>> 
> > >>>> On 18.01.2011, at 10:08, Gerd Hoffmann wrote:
> > >>>> 
> > >>>>> Hi,
> > >>>>> 
> > >>>>>>> Worse might also be that unknown issue that force you to inject an IRQ
> > >>>>>>> here. We don't know. That's probably worst.
> > >>>>>> 
> > >>>>>> Well, IIRC the issue was that usually a level high interrupt line would
> > >>>>>> simply retrigger an interrupt after enabling the interrupt line in the
> > >>>>>> APIC again.
> > >>>>> 
> > >>>>> edge triggered interrupts wouldn't though.
> > >>>> 
> > >>>> The code change doesn't change anything for edge triggered interrupts - they work fine. Only !msi (== level) ones are broken.
> > >>>> 
> > >>>>> 
> > >>>>>> Unless my memory completely fails on me, that didn't happen,
> > >>>>>> so I added the manual retrigger on a partial command ACK in ahci code.
> > >>>>> 
> > >>>>> That re-trigger smells like you are not clearing the interrupt line where you should.  For starters try calling ahci_check_irq() after guest writes to PORT_IRQ_STAT.
> > >>>> 
> > >>>> The problem happened when I had the following:
> > >>>> 
> > >>>> ahci irq bits = 0
> > >>>> <events happen>
> > >>>> ahci irq bits = 1 | 2
> > >>>> irq line trigger
> > >>>> guest clears 1
> > >>>> ahci bits = 2
> > >>>> <no irq line trigger, since we're still irq high>
> > >>>> 
> > >>>> On normal hardware, the high state of the irq line would simply trigger another interrupt in the guest when it gets ACKed on the LAPIC. Somehow it doesn't get triggered here. I don't see why clearing the interrupt line would help? It's not what real hardware would do, no?
> > >>>> 
> > >>> 
> > >>> No, real devices would simply leave the line asserted as long as there
> > >>> is a reason to.
> > >>> 
> > >>> So again my question: With which irqchips do you see this effect, kvm's
> > >>> in-kernel model and/or qemu's user space model? If there is an issue
> > >>> with retriggering a CPU interrupt while the source is still asserted,
> > >>> that probably needs to be fixed.
> > >>> 
> > >> 
> > >> I guess it might be related to a problem identified long time ago on
> > >> some targets, and that leads to the following #ifdef in i8259.c:
> > >> 
> > >> | /* all targets should do this rather than acking the IRQ in the cpu */
> > >> | #if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA)
> > >> 
> > >> For your information it has been fixed on MIPS in commit 
> > >> 4de9b249d37c1b382cc3e5a21fad1b4a11cec2fa.
> > >> 
> > >> Basically when an interrupt line is high, the interrupt is getting
> > >> delivered to the CPU. This part works correctly on x86. The CPU will
> > >> take a corresponding action, basically either disabling the interrupt
> > >> at the CPU or controller level or doing something on the device so that
> > >> it lower its IRQ line. This new IRQ line level should then propagate
> > >> through the various controllers, which should also lower their IRQ line
> > >> if no other interrupt line are active. This ACK process should then
> > >> continue up to the CPU.
> > > 
> > > I totally agree.
> > > 
> > > 
> > >> For x86 the interrupt state is cleared as soon as the interrupt is
> > >> signaled to the CPU (see cpu-exec.c line 407), therefore if an interrupt
> > >> is still pending, it won't be seen by the CPU. It's probably what you
> > >> observed with AHCI. 
> > > 
> > > Yes, essentially, the CPU_INTERRUPT_HARD signal is an input to the CPU.
> > > The CPU cannot drive it directly. To lower it, it must take some kind
> > > of indirect action (IO or whatever) to clear the condition that is
> > > forcing it high. Any assignments to clear or set the CPU_INTERRUPT_HARD
> > > signal from within the CPU core are likely wrong..
> > > 
> > > FWIW, PPC code in cpu-exec.c:443 looks suspicious aswell...
> > 
> > How's that suspicious? As long as pending_interrupts is only reset on actual interrupt delivery, everything's fine. Interrupts like the decrementor are not level based on some PPCs, so the actual semantics are implementation dependent.
> > 
> 
> It's suspicious because it's not the right place to do that. It should
> be done in the interrupt controller. This seems to be already done in
> hw/ppc.c when updating pending_interrupts, so this code seems to be
> useless.

Exactly.

The CPU model is trying to drive an input signal. We should find a way to
somehow forbid that in C.

Cheers

[Qemu-devel] Re: [PATCH 2/8] ahci: split ICH and AHCI even more

[Alexander Graf]

On 18.01.2011, at 13:19, Kevin Wolf wrote:

> Am 20.12.2010 22:13, schrieb Alexander Graf:
>> Sebastian's patch already did a pretty good job at splitting up ICH-9
>> AHCI code and the AHCI core. We need some more though. Copyright was missing,
>> the lspci dump belongs to ICH-9, we don't need the AHCI core to have its
>> own qdev device duplicate.
>> 
>> So let's split them a bit more in this patch, making things easier to
>> read an understand.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> The license header is still missing in ahci.h.

Yes, because header files don't get license headers. Check most other headers in qemu :). I can add one if it makes you happy, but numbers are generally not licensable.


Alex