[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