[RFC] add ahci support into qemu

None

Hi,

乔崇  wrote:
> Hi,Alexander Graf.
>
>  I am very glad you noticed my patch about ahci.I love qemu just like I love linux.I wish I could do much more work on 
> qemu development.
>
> I had cloned qemu from master branch,add my patch into it,and tested ahci on i386 softmmu.
>
> If anyone is interested on ahci,you can test my patch like this:
>
> git-clone -ls git://git.savannah.nongnu.org/qemu
> patch -p1 -i 0001-add-ahci-support-into-qemu-only-support-sata-disk.patch
> patch -p1 -i 0002-add-ahci-device-into-i386-pc-just-for-test.patch

The last submission was missing the qemu "glue", but this one looks complete. Nonetheless the old one was working fine 
enough.

> ./configure --target-list=i386-softmmu
> make
>
> dd if=/dev/zero of=/tmp/disk bs=1M count=100
> ./i386-softmmu/qemu  -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -boot d -drive if=sd,file=/tmp/disk
>
> After linux boot,you will find a ahci device named sda.
>
> Now this patch only support sata disk.
> Most ahci registers and operations which are not necessary on linux are ignored.
> Now this patch support disk identify,dma read,dma write,ignore other opertions.

Do you intent to work on this and complet the support?
There are also some minor changes needed like moving PCI_VENDOR_MYDEVICE
and PCI_PRODUCT_MYDEVICE to pci.h and others.

Did you use the Intel document #301473 for reference?

> By the way how you send patch to mail list?

Try "git send-email".


Regards,
Sebastian

On 05/02/2010 06:36 PM, Sebastian Herbszt wrote:
>
>> ./configure --target-list=i386-softmmu
>> make
>>
>> dd if=/dev/zero of=/tmp/disk bs=1M count=100
>> ./i386-softmmu/qemu  -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -boot 
>> d -drive if=sd,file=/tmp/disk
>>
>> After linux boot,you will find a ahci device named sda.
>>
>> Now this patch only support sata disk.
>> Most ahci registers and operations which are not necessary on linux 
>> are ignored.
>> Now this patch support disk identify,dma read,dma write,ignore other 
>> opertions.
>
> Do you intent to work on this and complet the support?

At the very least, unsupported registers and features should print out a 
warning instead of silently failing.

Hi all,

I am the GSoC student working on the project. I haven't tried the patch
yet, but I've read the code. It could use general cleanup and a lot of
development indeed, but otherwise it looks like a good base to build
complete support on so far. I'm going to try the patch in practice as
soon as I finish my university project. (It's due today.)

Chong, I'd be happy to stay in touch with you. I'm going to work on this
in the summer full time.

Regards,
Roland

On v, 2010-05-02 at 17:36 +0200, Sebastian Herbszt wrote:
> Hi,
> 
> 乔崇  wrote:
> > Hi,Alexander Graf.
> >
> >  I am very glad you noticed my patch about ahci.I love qemu just like I love linux.I wish I could do much more work on 
> > qemu development.
> >
> > I had cloned qemu from master branch,add my patch into it,and tested ahci on i386 softmmu.
> >
> > If anyone is interested on ahci,you can test my patch like this:
> >
> > git-clone -ls git://git.savannah.nongnu.org/qemu
> > patch -p1 -i 0001-add-ahci-support-into-qemu-only-support-sata-disk.patch
> > patch -p1 -i 0002-add-ahci-device-into-i386-pc-just-for-test.patch
> 
> The last submission was missing the qemu "glue", but this one looks complete. Nonetheless the old one was working fine 
> enough.
> 
> > ./configure --target-list=i386-softmmu
> > make
> >
> > dd if=/dev/zero of=/tmp/disk bs=1M count=100
> > ./i386-softmmu/qemu  -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -boot d -drive if=sd,file=/tmp/disk
> >
> > After linux boot,you will find a ahci device named sda.
> >
> > Now this patch only support sata disk.
> > Most ahci registers and operations which are not necessary on linux are ignored.
> > Now this patch support disk identify,dma read,dma write,ignore other opertions.
> 
> Do you intent to work on this and complet the support?
> There are also some minor changes needed like moving PCI_VENDOR_MYDEVICE
> and PCI_PRODUCT_MYDEVICE to pci.h and others.
> 
> Did you use the Intel document #301473 for reference?
> 
> > By the way how you send patch to mail list?
> 
> Try "git send-email".
> 
> 
> Regards,
> Sebastian
>

On v, 2010-05-02 at 17:56 +0200, Elek Roland wrote:
> I'm going to try the patch in practice as
> soon as I finish my university project. (It's due today.)

I've tried the patch, and I can confirm that it does provide an AHCI
device and a SATA disk. It shows up in qemu as an Intel Corporation
82801FR/FRW (ICH6R/ICH6RW) SATA Controller, which is consistent with the
device and vendor IDs in the code. I could successfully create an ext2
filesystem on the virtual disk, mount it, write a text file on it and
read it back.
I've noticed the following errors:
 - The source code claims to implement 2 ports, Linux reports 3.
 - When I halt the guest OS, the AHCI device throws a hardware error
about an unimplemented command.

I think we could clean up the code and use it as a basis for full
support.

Regards,
Roland

Hi Chong (or Qiao? Which one is your first name?),

Upfront - please don't top post. Top posting is when you write your reply at the top of the mail. This is considered bad style on most open source mailing lists. Instead, people here usually send "inline" comments. So they put their reply below the text they're referring to. That makes it easier to jump into a discussion for others.

On 02.05.2010, at 14:52, 乔崇 wrote:

> Hi,Alexander Graf.
>   
>   I am very glad you noticed my patch about ahci.I love qemu just like I love linux.I wish I could do much more work on qemu development.

Yes, please! This looks like an amazing start.

In case any of my comments sound harsh or unfriendly, please don't interpret them that way. I very much appreciate you working on this and I hope you, Roland, Joerg and me can bring this code base to a ready to use state for everything within the next few months.

> I had cloned qemu from master branch,add my patch into it,and tested ahci on i386 softmmu.
> 
> If anyone is interested on ahci,you can test my patch like this:
> 
> git-clone -ls git://git.savannah.nongnu.org/qemu

I would do a "git checkout -b ahci" here to switch to a new branch called "ahci". That way you can work in the branch and compare things to the master branch later on using "git diff master" or even "git checkout master; git pull; git checkout ahci; git rebase master" to get your patches on top of the current master tree.

> patch -p1 -i 0001-add-ahci-support-into-qemu-only-support-sata-disk.patch
> patch -p1 -i 0002-add-ahci-device-into-i386-pc-just-for-test.patch

As a hint: "git am" is the counterpart to "git format-patch" :)

I'd also suggest we set up a git tree somewhere so we can work on this together with everyone pushing to it until we deem it ready.

> ./configure --target-list=i386-softmmu
> make
> 
> dd if=/dev/zero of=/tmp/disk bs=1M count=100
> ./i386-softmmu/qemu  -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -boot d -drive if=sd,file=/tmp/disk

if=sd? I thought that was used for sd card emulation on embedded boards.

> 
> After linux boot,you will find a ahci device named sda.
> 
> Now this patch only support sata disk.
> Most ahci registers and operations which are not necessary on linux are ignored.
> Now this patch support disk identify,dma read,dma write,ignore other opertions.
> 
> By the way how you send patch to mail list?

I usually use "git format-patch --cover-letter -n -o <patch directory> master" while in my working branch. Then I go to the patch directory and use "git send-email" to send off the generated files.

> 2009年 11月 16日 星期一 10:31:04 CST
> From 2844d2ea0ead3bcf5382e578a2d658902b0314c8 Mon Sep 17 00:00:00 2001
> From: QiaoChong <qiaochong@loongson.cn>
> Date: Sun, 2 May 2010 20:07:32 +0800
> Subject: [PATCH] add ahci support into qemu,only support sata disk.
> 
> use -drive if=sd,file=diskname to add a ahci disk into qemu.
> 
> Signed-off-by: QiaoChong <qiaochong@loongson.cn>
> ---
> Makefile.target |    4 +
> hw/ahci.c       |  805 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 809 insertions(+), 0 deletions(-)
> create mode 100644 hw/ahci.c
> 
> diff --git a/Makefile.target b/Makefile.target
> index 65beed5..189a776 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -186,6 +186,10 @@ obj-$(CONFIG_USB_OHCI) += usb-ohci.o
> obj-y += rtl8139.o
> obj-y += e1000.o
> 
> +# ahci
> +#
> +obj-$(CONFIG_AHCI) += ahci.o
> +
> # Hardware support
> obj-i386-y = pckbd.o dma.o
> obj-i386-y += vga.o
> diff --git a/hw/ahci.c b/hw/ahci.c
> new file mode 100644
> index 0000000..a332a45
> --- /dev/null
> +++ b/hw/ahci.c
> @@ -0,0 +1,805 @@
> +/*
> + * QEMU AHCI Emulation
> + * Copyright (c) 2010 qiaochong@loongson.cn
> + *
> + * 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/>.
> + *
> + * TODO:
> + *  o ahci cd support
> + */
> +#include "hw.h"
> +#include "qemu-timer.h"
> +#include "monitor.h"
> +#include "sysbus.h"
> +#include "pci.h"
> +#include "dma.h"
> +#include "cpu-common.h"
> +#include <hw/ide/internal.h>
> +#define DPRINTF(...)
> +
> +
> +enum {


This looks like it's not really an enum. You munge together quite some definitions that don't belong in the same namespace, as is indicated by the different mid-names (CMD, IRQ, PCI, ...). Please just use #define statements here.

Also keep in mind that in qemu code, there are no tabs. Use spaces for everything.

> +	AHCI_PCI_BAR		= 5,
> +	AHCI_MAX_PORTS		= 32,
> +	AHCI_MAX_SG		= 168, /* hardware max is 64K */
> +	AHCI_DMA_BOUNDARY	= 0xffffffff,
> +	AHCI_USE_CLUSTERING	= 0,
> +	AHCI_MAX_CMDS		= 32,
> +	AHCI_CMD_SZ		= 32,
> +	AHCI_CMD_SLOT_SZ	= AHCI_MAX_CMDS * AHCI_CMD_SZ,
> +	AHCI_RX_FIS_SZ		= 256,
> +	AHCI_CMD_TBL_CDB	= 0x40,
> +	AHCI_CMD_TBL_HDR_SZ	= 0x80,
> +	AHCI_CMD_TBL_SZ		= AHCI_CMD_TBL_HDR_SZ + (AHCI_MAX_SG * 16),

Why 16?

> +	AHCI_CMD_TBL_AR_SZ	= AHCI_CMD_TBL_SZ * AHCI_MAX_CMDS,
> +	AHCI_PORT_PRIV_DMA_SZ	= AHCI_CMD_SLOT_SZ + AHCI_CMD_TBL_AR_SZ +
> +		AHCI_RX_FIS_SZ,
> +	AHCI_IRQ_ON_SG		= (1 << 31),
> +	AHCI_CMD_ATAPI		= (1 << 5),
> +	AHCI_CMD_WRITE		= (1 << 6),
> +	AHCI_CMD_PREFETCH	= (1 << 7),
> +	AHCI_CMD_RESET		= (1 << 8),
> +	AHCI_CMD_CLR_BUSY	= (1 << 10),
> +
> +	RX_FIS_D2H_REG		= 0x40,	/* offset of D2H Register FIS data */
> +	RX_FIS_SDB		= 0x58, /* offset of SDB FIS data */
> +	RX_FIS_UNK		= 0x60, /* offset of Unknown FIS data */
> +
> +	board_ahci		= 0,

lower case? Also what do the different boards mean?

> +	board_ahci_pi		= 1,
> +	board_ahci_vt8251	= 2,
> +	board_ahci_ign_iferr	= 3,
> +	board_ahci_sb600	= 4,
> +
> +	/* global controller registers */
> +	HOST_CAP		= 0x00, /* host capabilities */
> +	HOST_CTL		= 0x04, /* global host control */
> +	HOST_IRQ_STAT		= 0x08, /* interrupt status */
> +	HOST_PORTS_IMPL		= 0x0c, /* bitmap of implemented ports */
> +	HOST_VERSION		= 0x10, /* AHCI spec. version compliancy */

These are register numbers, right? Please mark them as such in the define. I imagine REG_HOST_CAP, REG_HOST_CTL, ... would be a good fit?

> +
> +	/* HOST_CTL bits */
> +	HOST_RESET		= (1 << 0),  /* reset controller; self-clear */
> +	HOST_IRQ_EN		= (1 << 1),  /* global IRQ enable */
> +	HOST_AHCI_EN		= (1 << 31), /* AHCI enabled */

HOST_CTL_RESET, HOST_CTL_IRQ_EN, ...

> +
> +	/* HOST_CAP bits */
> +	HOST_CAP_SSC		= (1 << 14), /* Slumber capable */
> +	HOST_CAP_CLO		= (1 << 24), /* Command List Override support */
> +	HOST_CAP_SSS		= (1 << 27), /* Staggered Spin-up */
> +	HOST_CAP_NCQ		= (1 << 30), /* Native Command Queueing */
> +	HOST_CAP_64		= (1 << 31), /* PCI DAC (64-bit DMA) support */
> +
> +	/* registers for each SATA port */
> +	PORT_LST_ADDR		= 0x00, /* command list DMA addr */
> +	PORT_LST_ADDR_HI	= 0x04, /* command list DMA addr hi */
> +	PORT_FIS_ADDR		= 0x08, /* FIS rx buf addr */
> +	PORT_FIS_ADDR_HI	= 0x0c, /* FIS rx buf addr hi */
> +	PORT_IRQ_STAT		= 0x10, /* interrupt status */
> +	PORT_IRQ_MASK		= 0x14, /* interrupt enable/disable mask */
> +	PORT_CMD		= 0x18, /* port command */
> +	PORT_TFDATA		= 0x20,	/* taskfile data */
> +	PORT_SIG		= 0x24,	/* device TF signature */
> +	PORT_CMD_ISSUE		= 0x38, /* command issue */
> +	PORT_SCR		= 0x28, /* SATA phy register block */
> +	PORT_SCR_STAT		= 0x28, /* SATA phy register: SStatus */
> +	PORT_SCR_CTL		= 0x2c, /* SATA phy register: SControl */
> +	PORT_SCR_ERR		= 0x30, /* SATA phy register: SError */
> +	PORT_SCR_ACT		= 0x34, /* SATA phy register: SActive */

REG_PORT_

> +
> +	/* PORT_IRQ_{STAT,MASK} bits */
> +	PORT_IRQ_COLD_PRES	= (1 << 31), /* cold presence detect */
> +	PORT_IRQ_TF_ERR		= (1 << 30), /* task file error */
> +	PORT_IRQ_HBUS_ERR	= (1 << 29), /* host bus fatal error */
> +	PORT_IRQ_HBUS_DATA_ERR	= (1 << 28), /* host bus data error */
> +	PORT_IRQ_IF_ERR		= (1 << 27), /* interface fatal error */
> +	PORT_IRQ_IF_NONFATAL	= (1 << 26), /* interface non-fatal error */
> +	PORT_IRQ_OVERFLOW	= (1 << 24), /* xfer exhausted available S/G */
> +	PORT_IRQ_BAD_PMP	= (1 << 23), /* incorrect port multiplier */
> +
> +	PORT_IRQ_PHYRDY		= (1 << 22), /* PhyRdy changed */
> +	PORT_IRQ_DEV_ILCK	= (1 << 7), /* device interlock */
> +	PORT_IRQ_CONNECT	= (1 << 6), /* port connect change status */
> +	PORT_IRQ_SG_DONE	= (1 << 5), /* descriptor processed */
> +	PORT_IRQ_UNK_FIS	= (1 << 4), /* unknown FIS rx'd */
> +	PORT_IRQ_SDB_FIS	= (1 << 3), /* Set Device Bits FIS rx'd */
> +	PORT_IRQ_DMAS_FIS	= (1 << 2), /* DMA Setup FIS rx'd */
> +	PORT_IRQ_PIOS_FIS	= (1 << 1), /* PIO Setup FIS rx'd */
> +	PORT_IRQ_D2H_REG_FIS	= (1 << 0), /* D2H Register FIS rx'd */

Please try to sort them descending.

> +
> +	PORT_IRQ_FREEZE		= PORT_IRQ_HBUS_ERR |
> +		PORT_IRQ_IF_ERR |
> +		PORT_IRQ_CONNECT |
> +		PORT_IRQ_PHYRDY |
> +		PORT_IRQ_UNK_FIS,
> +	PORT_IRQ_ERROR		= PORT_IRQ_FREEZE |
> +		PORT_IRQ_TF_ERR |
> +		PORT_IRQ_HBUS_DATA_ERR,
> +	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 */
> +	PORT_CMD_ATAPI		= (1 << 24), /* Device is ATAPI */
> +	PORT_CMD_LIST_ON	= (1 << 15), /* cmd list DMA engine running */
> +	PORT_CMD_FIS_ON		= (1 << 14), /* FIS DMA engine running */
> +	PORT_CMD_FIS_RX		= (1 << 4), /* Enable FIS receive DMA engine */
> +	PORT_CMD_CLO		= (1 << 3), /* Command list override */
> +	PORT_CMD_POWER_ON	= (1 << 2), /* Power up device */
> +	PORT_CMD_SPIN_UP	= (1 << 1), /* Spin up device */
> +	PORT_CMD_START		= (1 << 0), /* Enable port DMA engine */

descending

> +
> +	PORT_CMD_ICC_MASK	= (0xf << 28), /* i/f ICC state mask */
> +	PORT_CMD_ICC_ACTIVE	= (0x1 << 28), /* Put i/f in active state */
> +	PORT_CMD_ICC_PARTIAL	= (0x2 << 28), /* Put i/f in partial state */
> +	PORT_CMD_ICC_SLUMBER	= (0x6 << 28), /* Put i/f in slumber state */
> +
> +	/* ap->flags bits */
> +	AHCI_FLAG_NO_NCQ		= (1 << 24),
> +	AHCI_FLAG_IGN_IRQ_IF_ERR	= (1 << 25), /* ignore IRQ_IF_ERR */
> +	AHCI_FLAG_HONOR_PI		= (1 << 26), /* honor PORTS_IMPL */
> +	AHCI_FLAG_IGN_SERR_INTERNAL	= (1 << 27), /* ignore SERR_INTERNAL */
> +	AHCI_FLAG_32BIT_ONLY		= (1 << 28), /* force 32bit */
> +};
> +
> +/*
> + * ATA Commands (only mandatory commands listed here)
> + */
> +#define ATA_CMD_READ	0x20	/* Read Sectors (with retries)	*/
> +#define ATA_CMD_READN	0x21	/* Read Sectors ( no  retries)	*/
> +#define ATA_CMD_WRITE	0x30	/* Write Sectores (with retries)*/
> +#define ATA_CMD_WRITEN	0x31	/* Write Sectors  ( no  retries)*/
> +#define ATA_CMD_VRFY	0x40	/* Read Verify  (with retries)	*/
> +#define ATA_CMD_VRFYN	0x41	/* Read verify  ( no  retries)	*/
> +#define ATA_CMD_SEEK	0x70	/* Seek				*/
> +#define ATA_CMD_DIAG	0x90	/* Execute Device Diagnostic	*/
> +#define ATA_CMD_INIT	0x91	/* Initialize Device Parameters	*/
> +#define ATA_CMD_RD_MULT	0xC4	/* Read Multiple		*/
> +#define ATA_CMD_WR_MULT	0xC5	/* Write Multiple		*/
> +#define ATA_CMD_SETMULT	0xC6	/* Set Multiple Mode		*/
> +#define ATA_CMD_RD_DMA	0xC8	/* Read DMA (with retries)	*/
> +#define ATA_CMD_RD_DMAN	0xC9	/* Read DMS ( no  retries)	*/
> +#define ATA_CMD_WR_DMA	0xCA	/* Write DMA (with retries)	*/
> +#define ATA_CMD_WR_DMAN	0xCB	/* Write DMA ( no  retires)	*/
> +#define ATA_CMD_IDENT	0xEC	/* Identify Device		*/
> +#define ATA_CMD_SETF	0xEF	/* Set Features			*/
> +#define ATA_CMD_CHK_PWR	0xE5	/* Check Power Mode		*/
> +
> +#define ATA_CMD_READ_EXT 0x24	/* Read Sectors (with retries)	with 48bit addressing */
> +#define ATA_CMD_WRITE_EXT	0x34	/* Write Sectores (with retries) with 48bit addressing */
> +#define ATA_CMD_VRFY_EXT	0x42	/* Read Verify	(with retries)	with 48bit addressing */

This is the same as in ide, right? We should think about how to reuse that code.

> +
> +/*
> + * ATAPI Commands
> + */
> +#define ATAPI_CMD_IDENT 0xA1 /* Identify AT Atachment Packed Interface Device */
> +#define ATAPI_CMD_PACKET 0xA0 /* Packed Command */
> +
> +
> +#define ATAPI_CMD_INQUIRY 0x12
> +#define ATAPI_CMD_REQ_SENSE 0x03
> +#define ATAPI_CMD_READ_CAP 0x25
> +#define ATAPI_CMD_START_STOP 0x1B
> +#define ATAPI_CMD_READ_12 0xA8

These are all in ide already too.

> +
> +
> +
> +typedef struct ahci_control_regs {
> +	uint32_t  cap;
> +	uint32_t  ghc;
> +	uint32_t  irqstatus;
> +	uint32_t  impl;
> +	uint32_t  version;
> +} ahci_control_regs;
> +
> +typedef struct ahci_port_regs {
> +	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;
> +} ahci_port_regs; 
> +
> +
> +typedef struct ahci_cmd_hdr {
> +	uint32_t			opts;
> +	uint32_t			status;
> +	uint32_t			tbl_addr;
> +	uint32_t			tbl_addr_hi;
> +	uint32_t			reserved[4];
> +} ahci_cmd_hdr;
> +
> +typedef struct ahci_sg {
> +	uint32_t			addr;
> +	uint32_t			addr_hi;
> +	uint32_t			reserved;
> +	uint32_t			flags_size;
> +} ahci_sg;

Structs can be difficult if you want to map guest data on them, as you might have to do byte swapping for endianness conversion. IMHO the easiest way to get the best of both worlds (endianness conversion and structs) is to use for example something like "addr = ldw_phys(offsetof(ahci_sg, addr))"

> +
> +
> +typedef struct AHCIState{
> +	ahci_control_regs control_regs;
> +	ahci_port_regs port_regs[2];
> +	int mem;
> +	QEMUTimer *timer;
> +	IDEBus *ide;
> +	ahci_sg *prdt_buf;
> +	qemu_irq irq;
> +} AHCIState;
> +
> +typedef struct ahci_pci_state {
> +	PCIDevice card;
> +	AHCIState *ahci;
> +} ahci_pci_state;
> +
> +typedef struct ahci_sysbus_state {
> +	SysBusDevice busdev;
> +	AHCIState *ahci;
> +} ahci_sysbus_state;
> +
> +static uint32_t  ahci_port_read(AHCIState *s,int port,int offset)
> +{
> +	uint32_t val;
> +	uint32_t *p;
> +	ahci_port_regs *pr;
> +	pr=&s->port_regs[port];
> +
> +	switch(offset)
> +	{
> +		case PORT_SCR:
> +			if(s->ide && port==0) val=3;
> +			else val=0;
> +			break;
> +		case PORT_IRQ_STAT:
> +			val=pr->irq_stat;
> +			break;
> +		case PORT_TFDATA:
> +
> +		case PORT_SIG:
> +
> +		case PORT_CMD_ISSUE:
> +
> +
> +		case PORT_SCR_CTL:
> +
> +		case PORT_SCR_ERR:
> +
> +		case PORT_SCR_ACT:
> +		default:
> +			p=(uint32_t *)&s->port_regs[port];
> +			val= p[offset>>2];

val = p[offset / sizeof(uint32_t)];

> +			break;
> +	}
> +	return val;
> +
> +}
> +
> +static void ahci_check_irq(AHCIState *s)
> +{
> +	ahci_port_regs *pr;
> +	int i;
> +	for(i=0;i<2;i++)

ARRAY_SIZE(s->port_regs)

> +	{
> +		pr=&s->port_regs[i];
> +
> +		if(pr->irq_stat&pr->irq_mask){
> +			s->control_regs.irqstatus |= (1<<i);
> +		}
> +	}
> +
> +	if(s->control_regs.irqstatus)
> +		qemu_irq_raise(s->irq);
> +	else qemu_irq_lower(s->irq);
> +}
> +
> +
> +static void  ahci_port_write(AHCIState *s,int port,int offset,uint32_t val)
> +{
> +	ahci_port_regs *pr=&s->port_regs[port];
> +	uint32_t *p;
> +
> +	switch(offset)
> +	{
> +		case PORT_LST_ADDR:
> +			pr->lst_addr=val; break;
> +
> +		case PORT_LST_ADDR_HI:
> +			pr->lst_addr_hi=val; break;
> +
> +		case PORT_FIS_ADDR:
> +			pr->fis_addr = val; break;
> +
> +		case PORT_FIS_ADDR_HI:
> +			pr->fis_addr_hi = val; break;

Any reason why you don't do the default trick on these?
I also prefer having the "break" on a new line.

> +
> +		case PORT_IRQ_STAT:
> +			pr->irq_stat &= ~val; 
> +			ahci_check_irq(s);
> +			break;
> +
> +		case PORT_IRQ_MASK:
> +			pr->irq_mask = val; 
> +			ahci_check_irq(s);
> +			break;
> +
> +		case PORT_CMD:
> +			pr->cmd=val&(PORT_CMD_ATAPI|PORT_CMD_LIST_ON|PORT_CMD_FIS_ON|PORT_CMD_FIS_RX|PORT_CMD_CLO|PORT_CMD_POWER_ON|PORT_CMD_SPIN_UP|PORT_CMD_START);
> +			if(pr->cmd&PORT_CMD_START)
> +				qemu_mod_timer(s->timer,qemu_get_clock(vm_clock)+muldiv64(1, get_ticks_per_sec(), 1000));

Enlighten me - what exactly do we need a timer for here?

> +			break;
> +
> +
> +		case PORT_CMD_ISSUE:
> +			pr->cmd_issue=val;
> +			if(pr->cmd&PORT_CMD_START)
> +				qemu_mod_timer(s->timer,qemu_get_clock(vm_clock)+muldiv64(1, get_ticks_per_sec(), 1000));
> +			break;
> +
> +		case PORT_TFDATA:
> +
> +		case PORT_SIG:
> +
> +
> +		case PORT_SCR:
> +
> +		case PORT_SCR_CTL:
> +
> +		case PORT_SCR_ERR:
> +
> +		case PORT_SCR_ACT:
> +
> +
> +		default:
> +			p=(uint32_t *)pr;
> +			p[offset>>2]=val;

see above

> +			break;
> +	}
> +
> +}
> +
> +static uint32_t ahci_mem_readl(void *ptr, target_phys_addr_t addr)
> +{
> +	AHCIState *s = ptr;
> +	uint32_t val;
> +	uint32_t *p;
> +	addr=addr&0xfff;
> +	if(addr<0x20)

see my comment on writel

> +	{
> +		switch(addr)
> +		{
> +			case HOST_IRQ_STAT:
> +
> +			default:
> +				/* genernal host control */
> +				p=(uint32_t *)&s->control_regs;
> +				val=p[addr>>2];

see above

> +		}
> +	}
> +	else if(addr>=0x100 && addr<0x200)
> +	{

doesn't belong on a new line. Please read the file "CODING_STYLE".

> +		val=ahci_port_read(s,(addr-0x100)>>7,addr&0x7f);
> +	}
> +	else val=0;

coding style

> +
> +
> +	DPRINTF("ahci_mem_readl:  (addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
> +
> +	return val;
> +}
> +
> +
> +
> +static void ahci_mem_writel(void *ptr, target_phys_addr_t addr, uint32_t val)
> +{
> +	AHCIState *s = ptr;
> +	uint32_t *p;
> +	addr=addr&0xfff;
> +
> +	/* Only aligned reads are allowed on OHCI */

How does OHCI come in here? This is also about writes.

> +	if (addr & 3) {
> +		fprintf(stderr, "ahci: Mis-aligned write to addr 0x"
> +				TARGET_FMT_plx "\n", addr);
> +		return;
> +	}
> +
> +	if(addr<0x20)

Please make a define for this. I suppose it's REG_HOST_MAX?

> +	{
> +		switch(addr)
> +		{
> +			case HOST_IRQ_STAT:
> +				s->control_regs.irqstatus &= ~val; 
> +				ahci_check_irq(s);
> +				break;
> +			default:
> +				/* genernal host control */
> +				p=(uint32_t *)&s->control_regs;

huh?

> +		}
> +	}
> +	else if(addr>=0x100 && addr<0x200)
> +	{
> +		ahci_port_write(s,(addr-0x100)>>7,addr&0x7f,val);
> +	}
> +
> +	DPRINTF("ahci_mem_writel:  (addr 0x%08X), val 0x%08X\n", (unsigned) addr, val);
> +
> +}
> +
> +static CPUReadMemoryFunc *ahci_readfn[3]={
> +	ahci_mem_readl,
> +	ahci_mem_readl,
> +	ahci_mem_readl
> +};
> +
> +static CPUWriteMemoryFunc *ahci_writefn[3]={
> +	ahci_mem_writel,
> +	ahci_mem_writel,
> +	ahci_mem_writel
> +};
> +
> +static void ahci_reg_init(AHCIState *s)
> +{
> +	s->control_regs.cap=2|(0x1f<<8); /*2 ports,32 cmd slot*/
> +	s->control_regs.ghc=1<<31;
> +	s->control_regs.impl=1;/*2 ports*/
> +	s->control_regs.version=0x10100;

Too much magic. These should all be constants defined above.

> +}
> +
> +static void padstr(char *str, const char *src, int len)
> +{
> +	int i, v;
> +	for(i = 0; i < len; i++) {
> +		if (*src)
> +			v = *src++;
> +		else
> +			v = ' ';
> +		str[i^1] = v;
> +	}
> +}
> +
> +
> +static void put_le16(uint16_t *p, unsigned int v)
> +{
> +	*p = cpu_to_le16(v);
> +}

stw_p?

> +
> +
> +static void ide_identify(IDEState *s)

This is all in the ide code already. We should reuse it.

> +{
> +	uint16_t *p;
> +	unsigned int oldsize;
> +
> +	if (s->identify_set) {
> +		memcpy(s->io_buffer, s->identify_data, sizeof(s->identify_data));
> +		return;
> +	}
> +
> +	memset(s->io_buffer, 0, 512);
> +	p = (uint16_t *)s->io_buffer;
> +	put_le16(p + 0, 0x0040);
> +	put_le16(p + 1, s->cylinders);
> +	put_le16(p + 3, s->heads);
> +	put_le16(p + 4, 512 * s->sectors); /* XXX: retired, remove ? */
> +	put_le16(p + 5, 512); /* XXX: retired, remove ? */
> +	put_le16(p + 6, s->sectors);
> +	padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
> +	put_le16(p + 20, 3); /* XXX: retired, remove ? */
> +	put_le16(p + 21, 512); /* cache size in sectors */
> +	put_le16(p + 22, 4); /* ecc bytes */
> +	padstr((char *)(p + 23), s->version, 8); /* firmware version */
> +	padstr((char *)(p + 27), "QEMU HARDDISK", 40); /* model */
> +#if MAX_MULT_SECTORS > 1
> +	put_le16(p + 47, 0x8000 | MAX_MULT_SECTORS);
> +#endif
> +	put_le16(p + 48, 1); /* dword I/O */
> +	put_le16(p + 49, (1 << 11) | (1 << 9) | (1 << 8)); /* DMA and LBA supported */
> +	put_le16(p + 51, 0x200); /* PIO transfer cycle */
> +	put_le16(p + 52, 0x200); /* DMA transfer cycle */
> +	put_le16(p + 53, 1 | (1 << 1) | (1 << 2)); /* words 54-58,64-70,88 are valid */
> +	put_le16(p + 54, s->cylinders);
> +	put_le16(p + 55, s->heads);
> +	put_le16(p + 56, s->sectors);
> +	oldsize = s->cylinders * s->heads * s->sectors;
> +	put_le16(p + 57, oldsize);
> +	put_le16(p + 58, oldsize >> 16);
> +	if (s->mult_sectors)
> +		put_le16(p + 59, 0x100 | s->mult_sectors);
> +	put_le16(p + 60, s->nb_sectors);
> +	put_le16(p + 61, s->nb_sectors >> 16);
> +	put_le16(p + 62, 0x07); /* single word dma0-2 supported */
> +	put_le16(p + 63, 0x07); /* mdma0-2 supported */
> +	put_le16(p + 65, 120);
> +	put_le16(p + 66, 120);
> +	put_le16(p + 67, 120);
> +	put_le16(p + 68, 120);
> +	put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */
> +	put_le16(p + 81, 0x16); /* conforms to ata5 */
> +	/* 14=NOP supported, 0=SMART supported */
> +	put_le16(p + 82, (1 << 14) | 1);
> +	/* 13=flush_cache_ext,12=flush_cache,10=lba48 */
> +	put_le16(p + 83, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
> +	/* 14=set to 1, 1=SMART self test, 0=SMART error logging */
> +	put_le16(p + 84, (1 << 14) | 0);
> +	/* 14 = NOP supported, 5=WCACHE enabled, 0=SMART feature set enabled */
> +	if (bdrv_enable_write_cache(s->bs))
> +		put_le16(p + 85, (1 << 14) | (1 << 5) | 1);
> +	else
> +		put_le16(p + 85, (1 << 14) | 1);
> +	/* 13=flush_cache_ext,12=flush_cache,10=lba48 */
> +	put_le16(p + 86, (1 << 14) | (1 << 13) | (1 <<12) | (1 << 10));
> +	/* 14=set to 1, 1=smart self test, 0=smart error logging */
> +	put_le16(p + 87, (1 << 14) | 0);
> +	put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */
> +	put_le16(p + 93, 1 | (1 << 14) | 0x2000);
> +	put_le16(p + 100, s->nb_sectors);
> +	put_le16(p + 101, s->nb_sectors >> 16);
> +	put_le16(p + 102, s->nb_sectors >> 32);
> +	put_le16(p + 103, s->nb_sectors >> 48);
> +
> +	memcpy(s->identify_data, p, sizeof(s->identify_data));
> +	s->identify_set = 1;
> +}
> +
> +#define min(a,b) (((a)<(b))?(a):(b))
> +
> +static uint32_t write_to_sglist(uint8_t *buffer,uint32_t len,ahci_sg *sglist,uint32_t sgcount)
> +{
> +	uint32_t i=0;
> +	uint32_t total=0,once;
> +	for(i=0;len&&sgcount;i++)
> +	{
> +		once=min(sglist->flags_size+1,len);
> +		cpu_physical_memory_write(sglist->addr,buffer,once);
> +		sglist++;
> +		sgcount--;
> +		len -= once;
> +		buffer += once;
> +		total += once;
> +	}
> +
> +	return total;
> +}
> +
> +static uint32_t read_from_sglist(uint8_t *buffer,uint32_t len,ahci_sg *sglist,uint32_t sgcount)
> +{
> +	uint32_t i=0;
> +	uint32_t total=0,once;
> +	for(i=0;len&&sgcount;i++)
> +	{
> +		once=min(sglist->flags_size+1,len);
> +		cpu_physical_memory_read(sglist->addr,buffer,once);
> +		sglist++;
> +		sgcount--;
> +		len -= once;
> +		buffer += once;
> +		total += once;
> +	}
> +
> +	return total;
> +}
> +
> +static void handle_cmd(AHCIState *s,int port,int slot)
> +{
> +	int64_t sector_num;
> +	int nb_sectors;
> +	IDEState *ide_state;
> +	int ret;
> +	int cmdaddr;
> +	uint8_t fis[0x80];
> +	int cmd_len;
> +	int prdt_num;
> +	int i;
> +	ahci_port_regs *pr;
> +	ahci_cmd_hdr cmd_hdr;
> +	pr=&s->port_regs[port];
> +	cmdaddr=pr->lst_addr+slot*32;
> +	cpu_physical_memory_read(cmdaddr,(uint8_t *)&cmd_hdr,16);

Breaks on big endian hosts

> +	cmd_len=(cmd_hdr.opts&0x1f)*4;
> +	cpu_physical_memory_read(cmd_hdr.tbl_addr,fis,cmd_len);

Breaks on big endian hosts.

> +	prdt_num=cmd_hdr.opts>>16;
> +	if(prdt_num) cpu_physical_memory_read(cmd_hdr.tbl_addr+0x80,(uint8_t *)s->prdt_buf,prdt_num*32);

Breaks on big endian hosts.
Also, why keep the prdt_buf in the struct? Can't that stay on stack?

> +
> +
> +	for(i=0;i<cmd_len;i++)
> +	{
> +		if((i&0xf)==0)DPRINTF("\n%02x:",i);
> +		DPRINTF("%02x ",fis[i]);
> +	}
> +
> +	switch(fis[0])

Magic constant?

> +	{
> +		case 0x27:

Magic constant?

> +			break;
> +		default:
> +			hw_error("unkonow command fis[0]=%02x fis[1]=%02x fis[2]=%02x\n",fis[0],fis[1],fis[2]);break;
> +	}
> +
> +	switch(fis[1])

Magic constant?

> +	{
> +		case 1<<7: /* cmd fis */

Magic constant?

> +			break;
> +		case 0:
> +			break;
> +		default:
> +			hw_error("unkonow command fis[0]=%02x fis[1]=%02x fis[2]=%02x\n",fis[0],fis[1],fis[2]);break;
> +	}
> +
> +	if(fis[1]==0)

Magic constant? Why not reuse the switch from above?

> +	{
> +
> +	}
> +
> +	if(fis[1]==(1<<7))

Magic constant?

> +	{
> +		if(!s->ide)hw_error("no ahci sata disk now\n");
> +		ide_state=&s->ide->ifs[0];
> +		switch(fis[2])

Magic constant?

> +		{
> +			case ATA_CMD_IDENT:

These should directly go to the ide command interpreter.

> +				ide_identify(ide_state);
> +				write_to_sglist(ide_state->identify_data, sizeof(ide_state->identify_data),s->prdt_buf,prdt_num);
> +				pr->irq_stat |= (1<<2);
> +				break;
> +			case WIN_SETFEATURES:
> +				pr->irq_stat |= (1<<2);
> +				break;
> +			case ATA_CMD_RD_DMA:
> +				sector_num=(((int64_t)fis[10])<<40)|(((int64_t)fis[9])<<32)|(fis[8]<<24)|(fis[6]<<16)|(fis[5]<<8)|fis[4];
> +				nb_sectors=(fis[13]<<8)|fis[12];
> +				if(!nb_sectors)nb_sectors=256;
> +				printf("nb_sectors=%x,prdt_num=%x\n",nb_sectors,prdt_num);
> +				ret = bdrv_read(ide_state->bs, sector_num, ide_state->io_buffer, nb_sectors);
> +				if(ret==0)
> +				{
> +					write_to_sglist(ide_state->io_buffer,nb_sectors*512,s->prdt_buf,prdt_num);
> +				}
> +				pr->irq_stat |= (1<<2);
> +				break;
> +			case ATA_CMD_WR_DMA:
> +				sector_num=(((int64_t)fis[10])<<40)|(((int64_t)fis[9])<<32)|(fis[8]<<24)|(fis[6]<<16)|(fis[5]<<8)|fis[4];
> +				nb_sectors=(fis[13]<<8)|fis[12];
> +				if(!nb_sectors)nb_sectors=256;
> +				read_from_sglist(ide_state->io_buffer,nb_sectors*512,s->prdt_buf,prdt_num);
> +				ret = bdrv_write(ide_state->bs, sector_num, ide_state->io_buffer, nb_sectors);
> +				pr->irq_stat |= (1<<2);
> +				break;
> +			default:
> +				hw_error("unkonow command fis[0]=%02x fis[1]=%02x fis[2]=%02x\n",fis[0],fis[1],fis[2]);break;
> +		}
> +
> +	}
> +
> +	pr->cmd_issue &=~(1<<slot);
> +	ahci_check_irq(s);
> +}
> +
> +static void ahci_timer_function(void *opaque)
> +{
> +	AHCIState *s = opaque;
> +	ahci_port_regs *pr;
> +	int i,j;
> +	for(i=0;i<2;i++)
> +	{
> +		pr=&s->port_regs[i];
> +		for(j=0;j<32 && pr->cmd_issue;j++)
> +		{
> +			if(pr->cmd_issue&(1<<j))
> +			{
> +				handle_cmd(s,i,j);

So the register trigger above just waits for some milliseconds and then checks if there's a new command? Can't the command checking be done immediately?

> +			}	
> +		}
> +	}
> +}
> +
> +static AHCIState *ahci_new(void)
> +{
> +	DriveInfo *dinfo;
> +	IDEBus  *bus = qemu_mallocz(sizeof(IDEBus));
> +	AHCIState *s = qemu_mallocz(sizeof(AHCIState));
> +	ahci_reg_init(s);
> +	s->mem = cpu_register_io_memory(ahci_readfn, ahci_writefn, s);
> +	s->timer = qemu_new_timer(vm_clock, ahci_timer_function, s);
> +	s->prdt_buf = qemu_malloc(65535*32);

Oh, it can get this big? I guess I'll have to read again what this really is.

> +
> +	if ((dinfo = drive_get(IF_SD, 0, 0)) != NULL)

SATA should be a new if type. Please don't reuse the SD one.

> +	{
> +		ide_init2(bus, dinfo, NULL,0);
> +		s->ide=bus;
> +	}
> +	return s;
> +}
> +
> +static void ahci_pci_map(PCIDevice *pci_dev, int region_num,
> +		pcibus_t addr, pcibus_t size, int type)
> +{
> +	struct ahci_pci_state *d = (struct ahci_pci_state *)pci_dev;
> +	AHCIState *s = d->ahci;
> +
> +	cpu_register_physical_memory(addr, size, s->mem);
> +}
> +
> +#define PCI_VENDOR_MYDEVICE  0x8086
> +#define PCI_PRODUCT_MYDEVICE 0x2652

See hw/pci_ids.h

> +
> +#define PCI_CLASS_HEADERTYPE_00h	0x00

Please have a look at hw/pci_regs.h.

> +
> +static int pci_ahci_init(PCIDevice *dev)
> +{
> +	struct ahci_pci_state *d;
> +	d = DO_UPCAST(struct ahci_pci_state, card, dev);
> +	pci_config_set_vendor_id(d->card.config,PCI_VENDOR_MYDEVICE);
> +	pci_config_set_device_id(d->card.config,PCI_PRODUCT_MYDEVICE);
> +	d->card.config[PCI_COMMAND]		= 0x07;		/* I/O + Memory */

Magic constant?

> +	d->card.config[PCI_CLASS_DEVICE]	= 0;
> +	d->card.config[0x0b]		= 1;//storage

Magic constant?

> +	d->card.config[0x0c]		= 0x08;		/* Cache line size */
> +	d->card.config[0x0d]		= 0x40;		/* Latency timer */
> +	d->card.config[0x0e]		= PCI_CLASS_HEADERTYPE_00h;

d->card.config[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;

> +	d->card.config[0x3d] = 1;    /* interrupt pin 0 */
> +
> +	pci_register_bar(&d->card, 5, 0x200,
> +			PCI_BASE_ADDRESS_SPACE_MEMORY, ahci_pci_map);
> +	d->ahci=ahci_new();
> +	d->ahci->irq = d->card.irq[0];
> +	return 0;
> +}
> +
> +static int pci_ahci_uninit(PCIDevice *dev)
> +{
> +	return 0;
> +}
> +
> +static PCIDeviceInfo ahci_info = {
> +	.qdev.name  = "ahci",
> +	.qdev.size  = sizeof(ahci_pci_state),
> +	.init       = pci_ahci_init,
> +	.exit       = pci_ahci_uninit,
> +	.qdev.props = (Property[]) {
> +		DEFINE_PROP_END_OF_LIST(),
> +	}
> +};

The ahci device is pretty similar to a virtio-blk device from a topology point of view. It is basically a proxy for IDE. So the qdev structure should be pretty similar as well.

> +
> +static void ahci_pci_register_devices(void)
> +{
> +	pci_qdev_register(&ahci_info);
> +}
> +
> +device_init(ahci_pci_register_devices)
> +
> +
> +
> +static int ahci_sysbus_init(SysBusDevice *dev)
> +{
> +    ahci_sysbus_state *d = FROM_SYSBUS(ahci_sysbus_state, dev);
> +	d->ahci=ahci_new();
> +    sysbus_init_mmio(dev, 0x200, d->ahci->mem);
> +    sysbus_init_irq(dev, &d->ahci->irq);
> +    return 0;
> +}
> +
> +static void ahci_sysbus_register_devices(void)
> +{
> +    sysbus_register_dev("ahci", sizeof(ahci_sysbus_state), ahci_sysbus_init);
> +}
> +
> +device_init(ahci_sysbus_register_devices)
> -- 
> 1.5.6.5
> 
> From 446cce5657bf570fc612dea513f5bedcdc3be275 Mon Sep 17 00:00:00 2001
> From: QiaoChong <qiaochong@loongson.cn>
> Date: Sun, 2 May 2010 16:53:43 +0800
> Subject: [PATCH] add ahci device into i386 pc just for test.
> 
> test like this:
> 
> dd if=/dev/zero of=/tmp/disk bs=1M count=100
> ./i386-softmmu/qemu  -cdrom /mnt/hdb1/knoppix-dvd/KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -boot d -drive if=sd,file=/tmp/disk
> 
> Signed-off-by: QiaoChong <qiaochong@loongson.cn>
> ---
> default-configs/i386-softmmu.mak |    2 ++
> hw/pc.c                          |    1 +
> 2 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 4c1495f..bd72f39 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -20,3 +20,5 @@ CONFIG_NE2000_ISA=y
> CONFIG_PIIX_PCI=y
> CONFIG_SOUND=y
> CONFIG_VIRTIO_PCI=y
> +CONFIG_AHCI=y

This should also be enabled for x86_64.

> +
> diff --git a/hw/pc.c b/hw/pc.c
> index b659344..7ea437f 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1005,6 +1005,7 @@ static void pc_init1(ram_addr_t ram_size,
> 
>     if (pci_enabled) {
>         pci_piix3_ide_init(pci_bus, hd, piix3_devfn + 1);
> +		pci_create_simple(pci_bus,-1,"ahci");

Hrm. Isn't there a way to only instantiate it when used? How does if=scsi work? The same logic should apply here.


So far for a quick glimpse through the code. It all looks very good for a first revision :)

Alex

Elek Roland wrote:
> On v, 2010-05-02 at 17:56 +0200, Elek Roland wrote:
>> I'm going to try the patch in practice as
>> soon as I finish my university project. (It's due today.)
> 
> I've tried the patch, and I can confirm that it does provide an AHCI
> device and a SATA disk. It shows up in qemu as an Intel Corporation
> 82801FR/FRW (ICH6R/ICH6RW) SATA Controller, which is consistent with the
> device and vendor IDs in the code. I could successfully create an ext2
> filesystem on the virtual disk, mount it, write a text file on it and
> read it back.
> I've noticed the following errors:
> - The source code claims to implement 2 ports, Linux reports 3.
> - When I halt the guest OS, the AHCI device throws a hardware error
> about an unimplemented command.

Those issues should be fixed now by the following two patches:

0001-fix-port-count-cap-and-version-etc-to-ahci.patch
0002-add-WIN_STANDBYNOW1-process-into-ahci.patch

Sebastian

> I think we could clean up the code and use it as a basis for full
> support.
> 
> Regards,
> Roland
> 
> 
> 
>

Hi Chong,


乔崇 wrote:
> Alexander Graf 写道:
>> Hi Chong (or Qiao? Which one is your first name?),
>>
>>   
> Thanks your advice,My first name is Chong.
>> Upfront - please don't top post. Top posting is when you write your reply at the top of the mail. This is considered bad style on most open source mailing lists. Instead, people here usually send "inline" comments. So they put their reply below the text they're referring to. That makes it easier to jump into a discussion for others.
>>
>> On 02.05.2010, at 14:52, 乔崇 wrote:
>>
>>   
>>> Hi,Alexander Graf.
>>>   
>>>   I am very glad you noticed my patch about ahci.I love qemu just like I love linux.I wish I could do much more work on qemu development.
>>>     
>>
>> Yes, please! This looks like an amazing start.
>>
>> In case any of my comments sound harsh or unfriendly, please don't interpret them that way. I very much appreciate you working on this and I hope you, Roland, Joerg and me can bring this code base to a ready to use state for everything within the next few months.
>>
>>   
> I will work on ahci at weekend.I am a OS software engineer.I wrote
> ahci support just for testing my ahci driver transplanting on pmon and
> linux kernel for loongson cpu. Qemu is import tool,it help me a lot.
> So I am glad to share my work on qemu to others.

That sounds great. Roland will start working on this heavily soon, so
he'll hopefully make a lot of progress on the code. Nevertheless I would
love to keep you around on it as well. This patch was an amazing
kickstart and you seem like a clever guy which is always great to have
for projects like Qemu! :)

I'll try to get a public ahci branch ready this week. A mail announcing
that follows.
Do you think you'll have time to continue contributing on this
significantly within the next months?


Alex