[Qemu-devel] [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Qemu-devel] [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[QiaoChong]

When ahci init ,driver will send ATA_SRST command,ahci device report device type through port's sig register.
Ahci disk lookup change from IF_SD to IF_SCSI now,because IF_SD does not support cdrom media.
I just copy ide_atapi_cmd from hw/ide/core.c into hw/ahci.c,change a little,then the cdrom can be identified,and read by os.
If qemu can change dma_buf_prepare,dma_buf_rw,dma_buf_commit to a function pointer in BMDMAState,then I can rewrite three functions to support ahci's prtd,because it is different from ide's.

test a sata disk like this:
./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
test a sata cd like this:
./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso



QiaoChong (1):
  add cdrom support for ahci.

 hw/ahci.c |  425 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 422 insertions(+), 3 deletions(-)

[Qemu-devel] [PATCH 1/1] add cdrom support for ahci.

[QiaoChong]

ahci disk look up from IF_SCSI now.
test a sata disk:
./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
test a sata cd:
./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso

Signed-off-by: QiaoChong <qiaochong@loongson.cn>
---
 hw/ahci.c |  425 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 422 insertions(+), 3 deletions(-)

diff --git a/hw/ahci.c b/hw/ahci.c
index e1aed4a..fa32c68 100644
--- a/hw/ahci.c
+++ b/hw/ahci.c
@@ -16,7 +16,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  *
  * TODO:
- *  o ahci cd support
+ *  o ahci cd support should use ide,but now ide 's bmdma prdt is different from ahci's prdt.
  */
 #include "hw.h"
 #include "qemu-timer.h"
@@ -25,13 +25,15 @@
 #include "pci.h"
 #include "dma.h"
 #include "cpu-common.h"
+#include "scsi-defs.h"
+#include "scsi.h"
 #include <hw/ide/internal.h>
 
 #define DEBUG_AHCI
 
 #ifdef DEBUG_AHCI
 #define DPRINTF(fmt, ...) \
-do { printf("ahci: " fmt , ## __VA_ARGS__); } while (0)
+do { fprintf(stderr,"ahci: " fmt , ## __VA_ARGS__); } while (0)
 #else
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
@@ -160,6 +162,15 @@ enum {
 	AHCI_FLAG_32BIT_ONLY		= (1 << 28), /* force 32bit */
 };
 
+enum {
+	ATA_SRST		= (1 << 2),	/* software reset */
+};
+
+enum {
+STATE_RUN=0,
+STATE_RESET
+};
+
 /*
  * ATA Commands (only mandatory commands listed here)
  */
@@ -250,6 +261,7 @@ typedef struct ahci_sg {
 typedef struct AHCIState{
 	ahci_control_regs control_regs;
 	ahci_port_regs port_regs[SATA_PORTS];
+	uint32_t port_state[SATA_PORTS];
 	int mem;
 	QEMUTimer *timer;
 	IDEBus *ide;
@@ -472,10 +484,13 @@ static CPUWriteMemoryFunc *ahci_writefn[3]={
 
 static void ahci_reg_init(AHCIState *s)
 {
+	int i;
 	s->control_regs.cap = 3 | (0x1f << 8) | (1 << 20) ; /* 4 ports, 32 command slots, 1.5 Gb/s */
 	s->control_regs.ghc = 1 << 31; /* AHCI Enable */
 	s->control_regs.impl = 1; /* Port 0 implemented */
 	s->control_regs.version = 0x10000;
+	for(i=0;i<SATA_PORTS;i++)
+    s->port_state[i]=STATE_RUN;
 }
 
 static void padstr(char *str, const char *src, int len)
@@ -490,12 +505,91 @@ static void padstr(char *str, const char *src, int len)
 	}
 }
 
+static void padstr8(uint8_t *buf, int buf_size, const char *src)
+{
+    int i;
+    for(i = 0; i < buf_size; i++) {
+        if (*src)
+            buf[i] = *src++;
+        else
+            buf[i] = ' ';
+    }
+}
 
 static void put_le16(uint16_t *p, unsigned int v)
 {
 	*p = cpu_to_le16(v);
 }
 
+static inline void cpu_to_ube16(uint8_t *buf, int val)
+{
+    buf[0] = val >> 8;
+    buf[1] = val & 0xff;
+}
+
+static inline int ube16_to_cpu(const uint8_t *buf)
+{
+    return (buf[0] << 8) | buf[1];
+}
+
+static inline int ube32_to_cpu(const uint8_t *buf)
+{
+    return (buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3];
+}
+static inline void cpu_to_ube32(uint8_t *buf, unsigned int val)
+{
+    buf[0] = val >> 24;
+    buf[1] = val >> 16;
+    buf[2] = val >> 8;
+    buf[3] = val & 0xff;
+}
+static void ide_atapi_identify(IDEState *s)
+{
+    uint16_t *p;
+
+    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;
+    /* Removable CDROM, 50us response, 12 byte packets */
+    put_le16(p + 0, (2 << 14) | (5 << 8) | (1 << 7) | (2 << 5) | (0 << 0));
+    padstr((char *)(p + 10), s->drive_serial_str, 20); /* serial number */
+    put_le16(p + 20, 3); /* buffer type */
+    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 DVD-ROM", 40); /* model */
+    put_le16(p + 48, 1); /* dword I/O (XXX: should not be set on CDROM) */
+#ifdef USE_DMA_CDROM
+    put_le16(p + 49, 1 << 9 | 1 << 8); /* DMA and LBA supported */
+    put_le16(p + 53, 7); /* words 64-70, 54-58, 88 valid */
+    put_le16(p + 62, 7);  /* single word dma0-2 supported */
+    put_le16(p + 63, 7);  /* mdma0-2 supported */
+    put_le16(p + 64, 0x3f); /* PIO modes supported */
+#else
+    put_le16(p + 49, 1 << 9); /* LBA supported, no DMA */
+    put_le16(p + 53, 3); /* words 64-70, 54-58 valid */
+    put_le16(p + 63, 0x103); /* DMA modes XXX: may be incorrect */
+    put_le16(p + 64, 1); /* PIO modes */
+#endif
+    put_le16(p + 65, 0xb4); /* minimum DMA multiword tx cycle time */
+    put_le16(p + 66, 0xb4); /* recommended DMA multiword tx cycle time */
+    put_le16(p + 67, 0x12c); /* minimum PIO cycle time without flow control */
+    put_le16(p + 68, 0xb4); /* minimum PIO cycle time with IORDY flow control */
+
+    put_le16(p + 71, 30); /* in ns */
+    put_le16(p + 72, 30); /* in ns */
+
+    put_le16(p + 80, 0x1e); /* support up to ATA/ATAPI-4 */
+#ifdef USE_DMA_CDROM
+    put_le16(p + 88, 0x3f | (1 << 13)); /* udma5 set and supported */
+#endif
+    memcpy(s->identify_data, p, sizeof(s->identify_data));
+    s->identify_set = 1;
+}
 
 static void ide_identify(IDEState *s)
 {
@@ -611,6 +705,289 @@ static uint32_t read_from_sglist(uint8_t *buffer,uint32_t len,ahci_sg *sglist,ui
 	return total;
 }
 
+static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index,
+                                            uint16_t profile)
+{
+    uint8_t *buf_profile = buf + 12; /* start of profiles */
+
+    buf_profile += ((*index) * 4); /* start of indexed profile */
+    cpu_to_ube16 (buf_profile, profile);
+    buf_profile[2] = ((buf_profile[0] == buf[6]) && (buf_profile[1] == buf[7]));
+
+    /* each profile adds 4 bytes to the response */
+    (*index)++;
+    buf[11] += 4; /* Additional Length */
+
+    return 4;
+}
+
+static inline int media_present(IDEState *s)
+{
+    return (s->nb_sectors > 0);
+}
+static inline int media_is_dvd(IDEState *s)
+{
+    return (media_present(s) && s->nb_sectors > CD_MAX_SECTORS);
+}
+
+static inline int media_is_cd(IDEState *s)
+{
+    return (media_present(s) && s->nb_sectors <= CD_MAX_SECTORS);
+}
+static void ide_atapi_cmd(AHCIState *s,int prdt_num)
+{
+    const uint8_t *packet;
+    uint8_t *buf;
+    int max_len;
+	IDEState *ide_state;
+	ide_state=&s->ide->ifs[0];
+
+    packet = ide_state->io_buffer;
+    buf = ide_state->io_buffer;
+    switch(ide_state->io_buffer[0]) {
+    case GPCMD_TEST_UNIT_READY:
+	break;
+    case GPCMD_INQUIRY:
+        max_len = packet[4];
+        buf[0] = 0x05; /* CD-ROM */
+        buf[1] = 0x80; /* removable */
+        buf[2] = 0x00; /* ISO */
+        buf[3] = 0x21; /* ATAPI-2 (XXX: put ATAPI-4 ?) */
+        buf[4] = 31; /* additional length */
+        buf[5] = 0; /* reserved */
+        buf[6] = 0; /* reserved */
+        buf[7] = 0; /* reserved */
+        padstr8(buf + 8, 8, "QEMU");
+        padstr8(buf + 16, 16, "QEMU DVD-ROM");
+        padstr8(buf + 32, 4, ide_state->version);
+		write_to_sglist(buf,36,s->prdt_buf,prdt_num);
+        break;
+    case GPCMD_MODE_SENSE_6:
+    case GPCMD_MODE_SENSE_10:
+        {
+            int action, code;
+            if (packet[0] == GPCMD_MODE_SENSE_10)
+                max_len = ube16_to_cpu(packet + 7);
+            else
+                max_len = packet[4];
+            action = packet[2] >> 6;
+            code = packet[2] & 0x3f;
+            switch(action) {
+            case 0: /* current values */
+                switch(code) {
+                case GPMODE_R_W_ERROR_PAGE: /* error recovery */
+                    cpu_to_ube16(&buf[0], 16 + 6);
+                    buf[2] = 0x70;
+                    buf[3] = 0;
+                    buf[4] = 0;
+                    buf[5] = 0;
+                    buf[6] = 0;
+                    buf[7] = 0;
+
+                    buf[8] = 0x01;
+                    buf[9] = 0x06;
+                    buf[10] = 0x00;
+                    buf[11] = 0x05;
+                    buf[12] = 0x00;
+                    buf[13] = 0x00;
+                    buf[14] = 0x00;
+                    buf[15] = 0x00;
+					write_to_sglist(buf,16,s->prdt_buf,prdt_num);
+                    break;
+                case GPMODE_AUDIO_CTL_PAGE:
+                    cpu_to_ube16(&buf[0], 24 + 6);
+                    buf[2] = 0x70;
+                    buf[3] = 0;
+                    buf[4] = 0;
+                    buf[5] = 0;
+                    buf[6] = 0;
+                    buf[7] = 0;
+
+                    /* Fill with CDROM audio volume */
+                    buf[17] = 0;
+                    buf[19] = 0;
+                    buf[21] = 0;
+                    buf[23] = 0;
+
+					write_to_sglist(buf,24,s->prdt_buf,prdt_num);
+                    break;
+                case GPMODE_CAPABILITIES_PAGE:
+                    cpu_to_ube16(&buf[0], 28 + 6);
+                    buf[2] = 0x70;
+                    buf[3] = 0;
+                    buf[4] = 0;
+                    buf[5] = 0;
+                    buf[6] = 0;
+                    buf[7] = 0;
+
+                    buf[8] = 0x2a;
+                    buf[9] = 0x12;
+                    buf[10] = 0x00;
+                    buf[11] = 0x00;
+
+                    /* Claim PLAY_AUDIO capability (0x01) since some Linux
+                       code checks for this to automount media. */
+                    buf[12] = 0x71;
+                    buf[13] = 3 << 5;
+                    buf[14] = (1 << 0) | (1 << 3) | (1 << 5);
+                    if (bdrv_is_locked(ide_state->bs))
+                        buf[6] |= 1 << 1;
+                    buf[15] = 0x00;
+                    cpu_to_ube16(&buf[16], 706);
+                    buf[18] = 0;
+                    buf[19] = 2;
+                    cpu_to_ube16(&buf[20], 512);
+                    cpu_to_ube16(&buf[22], 706);
+                    buf[24] = 0;
+                    buf[25] = 0;
+                    buf[26] = 0;
+                    buf[27] = 0;
+					write_to_sglist(buf,28,s->prdt_buf,prdt_num);
+                    break;
+                default:
+                    goto error_cmd;
+                }
+                break;
+            case 1: /* changeable values */
+                goto error_cmd;
+            case 2: /* default values */
+                goto error_cmd;
+            default:
+            case 3: /* saved values */
+                goto error_cmd;
+                break;
+            }
+        }
+        break;
+    case GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL:
+        break;
+    case GPCMD_READ_TOC_PMA_ATIP:
+        {
+            int format, msf, start_track, len;
+            uint64_t total_sectors;
+
+            bdrv_get_geometry(ide_state->bs, &total_sectors);
+            total_sectors >>= 2;
+            if (total_sectors == 0) {
+                ide_atapi_cmd_error(ide_state, SENSE_NOT_READY,
+                                    ASC_MEDIUM_NOT_PRESENT);
+                break;
+            }
+            max_len = ube16_to_cpu(packet + 7);
+            format = packet[9] >> 6;
+            msf = (packet[1] >> 1) & 1;
+            start_track = packet[6];
+            switch(format) {
+            case 0:
+                len = cdrom_read_toc(total_sectors, buf, msf, start_track);
+                if (len < 0)
+                    goto error_cmd;
+				write_to_sglist(buf,len,s->prdt_buf,prdt_num);
+                break;
+            case 1:
+                /* multi session : only a single session defined */
+                memset(buf, 0, 12);
+                buf[1] = 0x0a;
+                buf[2] = 0x01;
+                buf[3] = 0x01;
+				write_to_sglist(buf,12,s->prdt_buf,prdt_num);
+                break;
+            case 2:
+                len = cdrom_read_toc_raw(total_sectors, buf, msf, start_track);
+                if (len < 0)
+                    goto error_cmd;
+				write_to_sglist(buf,len,s->prdt_buf,prdt_num);
+                break;
+            default:
+				goto error_cmd;
+                break;
+            }
+        }
+        break;
+    case GPCMD_READ_10:
+    case GPCMD_READ_12:
+        {
+            int nb_sectors, lba;
+			int ret;
+
+            if (packet[0] == GPCMD_READ_10)
+                nb_sectors = ube16_to_cpu(packet + 7);
+            else
+                nb_sectors = ube32_to_cpu(packet + 6);
+            lba = ube32_to_cpu(packet + 2);
+            if (nb_sectors == 0) {
+                //ide_atapi_cmd_ok(s);
+                break;
+            }
+			ret = bdrv_read(ide_state->bs, lba<<2, ide_state->io_buffer, nb_sectors);
+			if(ret==0)
+			{
+				write_to_sglist(ide_state->io_buffer,nb_sectors*512,s->prdt_buf,prdt_num);
+			}
+		break;
+        }
+    case GPCMD_READ_CDVD_CAPACITY:
+        {
+            uint64_t total_sectors;
+
+            bdrv_get_geometry(ide_state->bs, &total_sectors);
+            total_sectors >>= 2;
+            if (total_sectors == 0) {
+                ide_atapi_cmd_error(ide_state, SENSE_NOT_READY,
+                                    ASC_MEDIUM_NOT_PRESENT);
+                break;
+            }
+            /* NOTE: it is really the number of sectors minus 1 */
+            cpu_to_ube32(buf, total_sectors - 1);
+            cpu_to_ube32(buf + 4, 2048);
+			write_to_sglist(buf,8,s->prdt_buf,prdt_num);
+        }
+        break;
+    case GPCMD_GET_CONFIGURATION:
+        {
+            uint32_t len;
+            uint8_t index = 0;
+
+            /* only feature 0 is supported */
+
+            /* XXX: could result in alignment problems in some architectures */
+            max_len = ube16_to_cpu(packet + 7);
+
+            /*
+             * XXX: avoid overflow for io_buffer if max_len is bigger than
+             *      the size of that buffer (dimensioned to max number of
+             *      sectors to transfer at once)
+             *
+             *      Only a problem if the feature/profiles grow.
+             */
+            if (max_len > 512) /* XXX: assume 1 sector */
+                max_len = 512;
+
+            memset(buf, 0, max_len);
+            /* 
+             * the number of sectors from the media tells us which profile
+             * to use as current.  0 means there is no media
+             */
+            if (media_is_dvd(ide_state))
+                cpu_to_ube16(buf + 6, MMC_PROFILE_DVD_ROM);
+            else if (media_is_cd(ide_state))
+                cpu_to_ube16(buf + 6, MMC_PROFILE_CD_ROM);
+
+            buf[10] = 0x02 | 0x01; /* persistent and current */
+            len = 12; /* headers: 8 + 4 */
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_DVD_ROM);
+            len += ide_atapi_set_profile(buf, &index, MMC_PROFILE_CD_ROM);
+            cpu_to_ube32(buf, len - 4); /* data length */
+
+			write_to_sglist(buf,len,s->prdt_buf,prdt_num);
+            break;
+        }
+    default:
+	error_cmd:
+	 	hw_error("unsupport cmd 0x%08x\n",ide_state->io_buffer[0]);
+        break;
+    }
+}
 static void handle_cmd(AHCIState *s,int port,int slot)
 {
 	int64_t sector_num;
@@ -619,6 +996,7 @@ static void handle_cmd(AHCIState *s,int port,int slot)
 	int ret;
 	int cmdaddr;
 	uint8_t fis[0x80];
+	uint8_t acmd[0x20];
 	int cmd_len;
 	int prdt_num;
 	int i;
@@ -667,17 +1045,56 @@ static void handle_cmd(AHCIState *s,int port,int slot)
 #endif
 	}
 
+	switch(s->port_state[port])
+	{
+		case STATE_RUN:
+			if(fis[15]&ATA_SRST) s->port_state[port]=STATE_RESET;
+			break;
+		case STATE_RESET:
+			if(!(fis[15]&ATA_SRST))
+			{ 
+				if(!s->ide)hw_error("no ahci sata disk now\n");
+				ide_state=&s->ide->ifs[0];
+				s->port_state[port]=STATE_RUN;
+				if(bdrv_get_type_hint(ide_state->bs)==BDRV_TYPE_CDROM)
+					s->port_regs[port].sig=0xeb14<<16;
+				else s->port_regs[port].sig=0;
+			}
+			break;
+	}
+
 	if(fis[1]==(1<<7))
 	{
 		if(!s->ide)hw_error("no ahci sata disk now\n");
 		ide_state=&s->ide->ifs[0];
 		switch(fis[2])
 		{
+			case WIN_PIDENTIFY:
+				ide_atapi_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 ATA_CMD_IDENT:
 				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_PACKETCMD:
+				cpu_physical_memory_read(cmd_hdr.tbl_addr+0x40,acmd,0x20);
+#ifdef DEBUG_AHCI
+				for(i=0;i<32;i++)
+				{
+					if((i&0xf)==0)DPRINTF("\n%02x:",i);
+					DPRINTF("%02x ",acmd[i]);
+				}
+#endif
+				ide_state->atapi_dma = 1;
+				ide_state->nsector = 1;
+				memcpy(ide_state->io_buffer,acmd,32);
+				ide_atapi_cmd(s,prdt_num);
+				pr->irq_stat |= (1<<2);
+				break;
 			case WIN_STANDBYNOW1:
 			case WIN_SETFEATURES:
 				pr->irq_stat |= (1<<2);
@@ -734,16 +1151,18 @@ static AHCIState *ahci_new(void)
 {
 	DriveInfo *dinfo;
 	IDEBus  *bus = qemu_mallocz(sizeof(IDEBus));
+	BMDMAState *bmdma=qemu_mallocz(sizeof(BMDMAState));
 	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);
 
-	if ((dinfo = drive_get(IF_SD, 0, 0)) != NULL)
+	if ((dinfo = drive_get(IF_SCSI, 0, 0)) != NULL)
 	{
 		ide_init2(bus, dinfo, NULL,0);
 		s->ide=bus;
+		s->ide->bmdma=bmdma;
 	}
 	return s;
 }
-- 
1.7.0.3.254.g4503b.dirty

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Alexander Graf]

Hi Chong,

On 10.05.2010, at 13:55, QiaoChong wrote:

> When ahci init ,driver will send ATA_SRST command,ahci device report device type through port's sig register.
> Ahci disk lookup change from IF_SD to IF_SCSI now,because IF_SD does not support cdrom media.
> I just copy ide_atapi_cmd from hw/ide/core.c into hw/ahci.c,change a little,then the cdrom can be identified,and read by os.
> If qemu can change dma_buf_prepare,dma_buf_rw,dma_buf_commit to a function pointer in BMDMAState,then I can rewrite three functions to support ahci's prtd,because it is different from ide's.
> 
> test a sata disk like this:
> ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
> test a sata cd like this:
> ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso

Thanks for improving the patch, but I have some nitpicks considering on how to process here.

For starters, this patch is incremental to the previous one. Since the previous patch did not get applied to qemu, it doesn't make sense to send an incremental patch. Please send the full patchset but bump up the version in that case. You will find many examples for that on the mailing list. In most cases it also makes sense to rethink the splitting between patches.

I also gave you several comments and suggestions on the previous patch which I didn't see addressed. I think it's wrong to deal with the ATA commands in ahci specific code. The only viable solution in making this code usable for upstream is to reuse the IDE command processor. If there's anything missing in the abstaction layers, just change the abstraction in an early patch in your patchset.

Alex

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Sebastian Herbszt]

Alexander Graf wrote:
> Hi Chong,
>
> On 10.05.2010, at 13:55, QiaoChong wrote:
>
> > When ahci init ,driver will send ATA_SRST command,ahci device report device type through port's sig register.
> > Ahci disk lookup change from IF_SD to IF_SCSI now,because IF_SD does not support cdrom media.
> > I just copy ide_atapi_cmd from hw/ide/core.c into hw/ahci.c,change a little,then the cdrom can be identified,and 
> > read by os.
> > If qemu can change dma_buf_prepare,dma_buf_rw,dma_buf_commit to a function pointer in BMDMAState,then I can rewrite 
> > three functions to support ahci's prtd,because it is different from ide's.
> >
> > test a sata disk like this:
> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
> > test a sata cd like this:
> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive 
> > if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso
>
> Thanks for improving the patch, but I have some nitpicks considering on how to process here.
>
> For starters, this patch is incremental to the previous one. Since the previous patch did not get applied to qemu, it 
> doesn't make sense to send an incremental patch. Please send the full patchset but bump up > the version in that case. 
> You will find many examples for that on the mailing list. In most cases it also makes sense to rethink the splitting 
> between patches.

The problem of incremental patches will be a non issue as soon as the git tree is available.

Sebastian

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Alexander Graf]

On 11.05.2010, at 00:13, Sebastian Herbszt wrote:

> Alexander Graf wrote:
>> Hi Chong,
>> 
>> On 10.05.2010, at 13:55, QiaoChong wrote:
>> 
>> > When ahci init ,driver will send ATA_SRST command,ahci device report device type through port's sig register.
>> > Ahci disk lookup change from IF_SD to IF_SCSI now,because IF_SD does not support cdrom media.
>> > I just copy ide_atapi_cmd from hw/ide/core.c into hw/ahci.c,change a little,then the cdrom can be identified,and > read by os.
>> > If qemu can change dma_buf_prepare,dma_buf_rw,dma_buf_commit to a function pointer in BMDMAState,then I can rewrite > three functions to support ahci's prtd,because it is different from ide's.
>> >
>> > test a sata disk like this:
>> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
>> > test a sata cd like this:
>> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive > if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso
>> 
>> Thanks for improving the patch, but I have some nitpicks considering on how to process here.
>> 
>> For starters, this patch is incremental to the previous one. Since the previous patch did not get applied to qemu, it doesn't make sense to send an incremental patch. Please send the full patchset but bump up > the version in that case. You will find many examples for that on the mailing list. In most cases it also makes sense to rethink the splitting between patches.
> 
> The problem of incremental patches will be a non issue as soon as the git tree is available.

Yes, I'll take care of doing that. I hope I'll get to it tomorrow. Until then all the small incremental patches are completely useless to me and impossible to review. They don't make sense without the broader scope.

Alex

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Alexander Graf]

On 11.05.2010, at 00:13, Sebastian Herbszt wrote:

> Alexander Graf wrote:
>> Hi Chong,
>> 
>> On 10.05.2010, at 13:55, QiaoChong wrote:
>> 
>> > When ahci init ,driver will send ATA_SRST command,ahci device report device type through port's sig register.
>> > Ahci disk lookup change from IF_SD to IF_SCSI now,because IF_SD does not support cdrom media.
>> > I just copy ide_atapi_cmd from hw/ide/core.c into hw/ahci.c,change a little,then the cdrom can be identified,and > read by os.
>> > If qemu can change dma_buf_prepare,dma_buf_rw,dma_buf_commit to a function pointer in BMDMAState,then I can rewrite > three functions to support ahci's prtd,because it is different from ide's.
>> >
>> > test a sata disk like this:
>> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive if=scsi,file=/tmp/disk
>> > test a sata cd like this:
>> > ./i386-softmmu/qemu -cdrom KNOPPIX_V6.0.1CD-2009-02-08-EN.iso -drive > if=scsi,media=cdrom,file=KNOPPIX_V6.0.1CD-2009-02-08-EN.iso
>> 
>> Thanks for improving the patch, but I have some nitpicks considering on how to process here.
>> 
>> For starters, this patch is incremental to the previous one. Since the previous patch did not get applied to qemu, it doesn't make sense to send an incremental patch. Please send the full patchset but bump up > the version in that case. You will find many examples for that on the mailing list. In most cases it also makes sense to rethink the splitting between patches.
> 
> The problem of incremental patches will be a non issue as soon as the git tree is available.

I set up a mailing list and a git tree for development. The mailing list is here:

https://lists.sourceforge.net/lists/listinfo/qemu-ahci-devel

The git tree is here (pure mirror of qemu at this moment):

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

Could you guys please send me all the patches you have so far so I can put them into an ahci branch? Please also register with repo.or.cz so I can give you commit rights. If things become too messy, I'll try to put me or Roland as gatekeeper for patches into the repo.


Alex

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Joerg Roedel]

On Tue, May 11, 2010 at 01:19:27AM +0200, Alexander Graf wrote:
> 
> On 11.05.2010, at 00:13, Sebastian Herbszt wrote:
> > 
> > The problem of incremental patches will be a non issue as soon as the git tree is available.
> 
> I set up a mailing list and a git tree for development. The mailing list is here:
> 
> https://lists.sourceforge.net/lists/listinfo/qemu-ahci-devel

Do we need a seperate mailing list for that? The traffic is low enough
to keep it on qemu-devel imho.

> The git tree is here (pure mirror of qemu at this moment):
> 
> git://repo.or.cz/qemu/ahci.git

Great.

	Joerg

[Qemu-devel] Re: [PATCH 0/1] [RFC][AHCI] add cdrom support for ahci.

[Alexander Graf]

Am 11.05.2010 um 11:25 schrieb Joerg Roedel <joro@8bytes.org>:

> On Tue, May 11, 2010 at 01:19:27AM +0200, Alexander Graf wrote:
>>
>> On 11.05.2010, at 00:13, Sebastian Herbszt wrote:
>>>
>>> The problem of incremental patches will be a non issue as soon as  
>>> the git tree is available.
>>
>> I set up a mailing list and a git tree for development. The mailing  
>> list is here:
>>
>> https://lists.sourceforge.net/lists/listinfo/qemu-ahci-devel
>
> Do we need a seperate mailing list for that? The traffic is low enough
> to keep it on qemu-devel imho.

I would like to keep the discussions and commit mails out of qemu- 
devel, yes. There will be more traffic and qemu-devel is almost  
unreadable already.

Alex