* [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2010-12-23 8:23 ` Stefan Hajnoczi
2010-12-20 21:13 ` [Qemu-devel] [PATCH 2/8] ahci: split ICH and AHCI even more Alexander Graf
` (7 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core
2010-12-20 21:13 ` [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core Alexander Graf
@ 2010-12-23 8:23 ` Stefan Hajnoczi
2010-12-23 10:32 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Stefan Hajnoczi @ 2010-12-23 8:23 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core
2010-12-23 8:23 ` Stefan Hajnoczi
@ 2010-12-23 10:32 ` Alexander Graf
2010-12-27 11:51 ` Sebastian Herbszt
0 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-23 10:32 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core
2010-12-23 10:32 ` Alexander Graf
@ 2010-12-27 11:51 ` Sebastian Herbszt
0 siblings, 0 replies; 45+ messages in thread
From: Sebastian Herbszt @ 2010-12-27 11:51 UTC (permalink / raw)
To: Alexander Graf, Stefan Hajnoczi
Cc: Kevin Wolf, Joerg Roedel, qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 2/8] ahci: split ICH and AHCI even more
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-18 12:19 ` [Qemu-devel] " Kevin Wolf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 3/8] ahci: send init d2h fis on fis enable Alexander Graf
` (6 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 2/8] ahci: split ICH and AHCI even more
2010-12-20 21:13 ` [Qemu-devel] [PATCH 2/8] ahci: split ICH and AHCI even more Alexander Graf
@ 2011-01-18 12:19 ` Kevin Wolf
2011-02-01 14:12 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 12:19 UTC (permalink / raw)
To: Alexander Graf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 2/8] ahci: split ICH and AHCI even more
2011-01-18 12:19 ` [Qemu-devel] " Kevin Wolf
@ 2011-02-01 14:12 ` Alexander Graf
0 siblings, 0 replies; 45+ messages in thread
From: Alexander Graf @ 2011-02-01 14:12 UTC (permalink / raw)
To: Kevin Wolf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 3/8] ahci: send init d2h fis on fis enable
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 1/8] ahci: split ICH9 from core Alexander Graf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 2/8] ahci: split ICH and AHCI even more Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-18 12:25 ` [Qemu-devel] " Kevin Wolf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 4/8] ahci: use qiov instead of dma helpers Alexander Graf
` (5 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 3/8] ahci: send init d2h fis on fis enable
2010-12-20 21:13 ` [Qemu-devel] [PATCH 3/8] ahci: send init d2h fis on fis enable Alexander Graf
@ 2011-01-18 12:25 ` Kevin Wolf
2011-01-18 12:42 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 12:25 UTC (permalink / raw)
To: Alexander Graf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 3/8] ahci: send init d2h fis on fis enable
2011-01-18 12:25 ` [Qemu-devel] " Kevin Wolf
@ 2011-01-18 12:42 ` Alexander Graf
0 siblings, 0 replies; 45+ messages in thread
From: Alexander Graf @ 2011-01-18 12:42 UTC (permalink / raw)
To: Kevin Wolf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 4/8] ahci: use qiov instead of dma helpers
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (2 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 3/8] ahci: send init d2h fis on fis enable Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-18 12:35 ` [Qemu-devel] " Kevin Wolf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 5/8] ahci: Implement HBA reset Alexander Graf
` (4 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers
2010-12-20 21:13 ` [Qemu-devel] [PATCH 4/8] ahci: use qiov instead of dma helpers Alexander Graf
@ 2011-01-18 12:35 ` Kevin Wolf
2011-01-18 12:45 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 12:35 UTC (permalink / raw)
To: Alexander Graf
Cc: Stefan Hajnoczi, Joerg Roedel, qemu-devel Developers,
Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers
2011-01-18 12:35 ` [Qemu-devel] " Kevin Wolf
@ 2011-01-18 12:45 ` Alexander Graf
2011-01-18 13:14 ` Stefan Hajnoczi
0 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-18 12:45 UTC (permalink / raw)
To: Kevin Wolf
Cc: Stefan Hajnoczi, Joerg Roedel, qemu-devel Developers,
Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers
2011-01-18 12:45 ` Alexander Graf
@ 2011-01-18 13:14 ` Stefan Hajnoczi
2011-01-18 13:30 ` Kevin Wolf
0 siblings, 1 reply; 45+ messages in thread
From: Stefan Hajnoczi @ 2011-01-18 13:14 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, qemu-devel Developers, Gerd Hoffmann,
Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 4/8] ahci: use qiov instead of dma helpers
2011-01-18 13:14 ` Stefan Hajnoczi
@ 2011-01-18 13:30 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 13:30 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Joerg Roedel, Alexander Graf, qemu-devel Developers,
Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 5/8] ahci: Implement HBA reset
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (3 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 4/8] ahci: use qiov instead of dma helpers Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 6/8] ahci: make number of ports runtime determined Alexander Graf
` (3 subsequent siblings)
8 siblings, 0 replies; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 6/8] ahci: make number of ports runtime determined
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (4 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 5/8] ahci: Implement HBA reset Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-18 12:40 ` [Qemu-devel] " Kevin Wolf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 7/8] ahci: free dynamically allocated iovs Alexander Graf
` (2 subsequent siblings)
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined
2010-12-20 21:13 ` [Qemu-devel] [PATCH 6/8] ahci: make number of ports runtime determined Alexander Graf
@ 2011-01-18 12:40 ` Kevin Wolf
2011-01-18 12:46 ` Alexander Graf
2011-01-18 13:09 ` Gerd Hoffmann
0 siblings, 2 replies; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 12:40 UTC (permalink / raw)
To: Alexander Graf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined
2011-01-18 12:40 ` [Qemu-devel] " Kevin Wolf
@ 2011-01-18 12:46 ` Alexander Graf
2011-01-18 13:33 ` Kevin Wolf
2011-01-18 13:09 ` Gerd Hoffmann
1 sibling, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-18 12:46 UTC (permalink / raw)
To: Kevin Wolf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined
2011-01-18 12:46 ` Alexander Graf
@ 2011-01-18 13:33 ` Kevin Wolf
0 siblings, 0 replies; 45+ messages in thread
From: Kevin Wolf @ 2011-01-18 13:33 UTC (permalink / raw)
To: Alexander Graf
Cc: Joerg Roedel, Gerd Hoffmann, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 6/8] ahci: make number of ports runtime determined
2011-01-18 12:40 ` [Qemu-devel] " Kevin Wolf
2011-01-18 12:46 ` Alexander Graf
@ 2011-01-18 13:09 ` Gerd Hoffmann
1 sibling, 0 replies; 45+ messages in thread
From: Gerd Hoffmann @ 2011-01-18 13:09 UTC (permalink / raw)
To: Kevin Wolf
Cc: Joerg Roedel, Sebastian Herbszt, Alexander Graf, qemu-devel Developers
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] [PATCH 7/8] ahci: free dynamically allocated iovs
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (5 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 6/8] ahci: make number of ports runtime determined Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-18 12:41 ` [Qemu-devel] " Kevin Wolf
2010-12-20 21:13 ` [Qemu-devel] [PATCH 8/8] ahci: fix !msi interrupts Alexander Graf
2011-01-17 13:25 ` [Qemu-devel] [PATCH 0/8] Some more AHCI work Gerd Hoffmann
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] [PATCH 8/8] ahci: fix !msi interrupts
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (6 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 7/8] ahci: free dynamically allocated iovs Alexander Graf
@ 2010-12-20 21:13 ` Alexander Graf
2011-01-17 14:37 ` [Qemu-devel] " Jan Kiszka
2011-01-17 13:25 ` [Qemu-devel] [PATCH 0/8] Some more AHCI work Gerd Hoffmann
8 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2010-12-20 21:13 UTC (permalink / raw)
To: qemu-devel Developers
Cc: Kevin Wolf, Joerg Roedel, Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply related [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2010-12-20 21:13 ` [Qemu-devel] [PATCH 8/8] ahci: fix !msi interrupts Alexander Graf
@ 2011-01-17 14:37 ` Jan Kiszka
2011-01-17 16:00 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Jan Kiszka @ 2011-01-17 14:37 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 14:37 ` [Qemu-devel] " Jan Kiszka
@ 2011-01-17 16:00 ` Alexander Graf
2011-01-17 16:03 ` Jan Kiszka
0 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-17 16:00 UTC (permalink / raw)
To: Jan Kiszka
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:00 ` Alexander Graf
@ 2011-01-17 16:03 ` Jan Kiszka
2011-01-17 16:04 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Jan Kiszka @ 2011-01-17 16:03 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:03 ` Jan Kiszka
@ 2011-01-17 16:04 ` Alexander Graf
2011-01-17 16:20 ` Jan Kiszka
0 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-17 16:04 UTC (permalink / raw)
To: Jan Kiszka
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:04 ` Alexander Graf
@ 2011-01-17 16:20 ` Jan Kiszka
2011-01-17 16:33 ` Alexander Graf
0 siblings, 1 reply; 45+ messages in thread
From: Jan Kiszka @ 2011-01-17 16:20 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:20 ` Jan Kiszka
@ 2011-01-17 16:33 ` Alexander Graf
2011-01-17 16:48 ` Jan Kiszka
2011-01-18 9:08 ` Gerd Hoffmann
0 siblings, 2 replies; 45+ messages in thread
From: Alexander Graf @ 2011-01-17 16:33 UTC (permalink / raw)
To: Jan Kiszka
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:33 ` Alexander Graf
@ 2011-01-17 16:48 ` Jan Kiszka
2011-01-17 16:50 ` Alexander Graf
2011-01-18 9:08 ` Gerd Hoffmann
1 sibling, 1 reply; 45+ messages in thread
From: Jan Kiszka @ 2011-01-17 16:48 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:48 ` Jan Kiszka
@ 2011-01-17 16:50 ` Alexander Graf
0 siblings, 0 replies; 45+ messages in thread
From: Alexander Graf @ 2011-01-17 16:50 UTC (permalink / raw)
To: Jan Kiszka
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt,
qemu-devel Developers, Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-17 16:33 ` Alexander Graf
2011-01-17 16:48 ` Jan Kiszka
@ 2011-01-18 9:08 ` Gerd Hoffmann
2011-01-18 12:05 ` Alexander Graf
1 sibling, 1 reply; 45+ messages in thread
From: Gerd Hoffmann @ 2011-01-18 9:08 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Jan Kiszka, Sebastian Herbszt, qemu-devel Developers,
Joerg Roedel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-18 9:08 ` Gerd Hoffmann
@ 2011-01-18 12:05 ` Alexander Graf
2011-01-18 12:58 ` Jan Kiszka
2011-01-18 13:02 ` Gerd Hoffmann
0 siblings, 2 replies; 45+ messages in thread
From: Alexander Graf @ 2011-01-18 12:05 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Kevin Wolf, Jan Kiszka, Sebastian Herbszt, qemu-devel Developers,
Joerg Roedel
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-18 12:05 ` Alexander Graf
@ 2011-01-18 12:58 ` Jan Kiszka
2011-01-22 13:13 ` Aurelien Jarno
2011-01-18 13:02 ` Gerd Hoffmann
1 sibling, 1 reply; 45+ messages in thread
From: Jan Kiszka @ 2011-01-18 12:58 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, Sebastian Herbszt, Gerd Hoffmann,
qemu-devel Developers
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-18 12:58 ` Jan Kiszka
@ 2011-01-22 13:13 ` Aurelien Jarno
2011-01-22 14:14 ` Edgar E. Iglesias
2011-01-22 14:28 ` Alexander Graf
0 siblings, 2 replies; 45+ messages in thread
From: Aurelien Jarno @ 2011-01-22 13:13 UTC (permalink / raw)
To: Jan Kiszka
Cc: Kevin Wolf, Joerg Roedel, qemu-devel Developers, Alexander Graf,
Gerd Hoffmann, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 13:13 ` Aurelien Jarno
@ 2011-01-22 14:14 ` Edgar E. Iglesias
2011-01-22 14:32 ` Alexander Graf
2011-01-22 14:28 ` Alexander Graf
1 sibling, 1 reply; 45+ messages in thread
From: Edgar E. Iglesias @ 2011-01-22 14:14 UTC (permalink / raw)
To: Aurelien Jarno
Cc: Kevin Wolf, Jan Kiszka, Alexander Graf, qemu-devel Developers,
Gerd Hoffmann, Joerg Roedel, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 14:14 ` Edgar E. Iglesias
@ 2011-01-22 14:32 ` Alexander Graf
2011-01-22 14:40 ` Aurelien Jarno
0 siblings, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-22 14:32 UTC (permalink / raw)
To: Edgar E.Iglesias
Cc: Kevin Wolf, Jan Kiszka, qemu-devel Developers, Gerd Hoffmann,
Joerg Roedel, Aurelien Jarno, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 14:32 ` Alexander Graf
@ 2011-01-22 14:40 ` Aurelien Jarno
2011-01-23 3:34 ` Edgar E. Iglesias
0 siblings, 1 reply; 45+ messages in thread
From: Aurelien Jarno @ 2011-01-22 14:40 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, qemu-devel Developers, Gerd Hoffmann,
Jan Kiszka, Edgar E.Iglesias, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 14:40 ` Aurelien Jarno
@ 2011-01-23 3:34 ` Edgar E. Iglesias
0 siblings, 0 replies; 45+ messages in thread
From: Edgar E. Iglesias @ 2011-01-23 3:34 UTC (permalink / raw)
To: Aurelien Jarno
Cc: Kevin Wolf, Jan Kiszka, qemu-devel Developers, Alexander Graf,
Gerd Hoffmann, Joerg Roedel, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 13:13 ` Aurelien Jarno
2011-01-22 14:14 ` Edgar E. Iglesias
@ 2011-01-22 14:28 ` Alexander Graf
2011-01-22 14:34 ` Aurelien Jarno
1 sibling, 1 reply; 45+ messages in thread
From: Alexander Graf @ 2011-01-22 14:28 UTC (permalink / raw)
To: Aurelien Jarno
Cc: Kevin Wolf, Jan Kiszka, qemu-devel Developers, Gerd Hoffmann,
Joerg Roedel, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-22 14:28 ` Alexander Graf
@ 2011-01-22 14:34 ` Aurelien Jarno
0 siblings, 0 replies; 45+ messages in thread
From: Aurelien Jarno @ 2011-01-22 14:34 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Jan Kiszka, qemu-devel Developers, Gerd Hoffmann,
Joerg Roedel, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] Re: [PATCH 8/8] ahci: fix !msi interrupts
2011-01-18 12:05 ` Alexander Graf
2011-01-18 12:58 ` Jan Kiszka
@ 2011-01-18 13:02 ` Gerd Hoffmann
1 sibling, 0 replies; 45+ messages in thread
From: Gerd Hoffmann @ 2011-01-18 13:02 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Jan Kiszka, Joerg Roedel, qemu-devel Developers,
Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [Qemu-devel] [PATCH 0/8] Some more AHCI work
2010-12-20 21:13 [Qemu-devel] [PATCH 0/8] Some more AHCI work Alexander Graf
` (7 preceding siblings ...)
2010-12-20 21:13 ` [Qemu-devel] [PATCH 8/8] ahci: fix !msi interrupts Alexander Graf
@ 2011-01-17 13:25 ` Gerd Hoffmann
8 siblings, 0 replies; 45+ messages in thread
From: Gerd Hoffmann @ 2011-01-17 13:25 UTC (permalink / raw)
To: Alexander Graf
Cc: Kevin Wolf, Joerg Roedel, qemu-devel Developers, Sebastian Herbszt
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
^ permalink raw reply [flat|nested] 45+ messages in thread