On Fri, Sep 21, 2012 at 04:05:14PM +0200, Markus Armbruster wrote: > Jason Baron <jba...@redhat.com> writes: > > > From: Isaku Yamahata <yamah...@valinux.co.jp> > > > > Introduce a helper function which initializes the ahci port with ide > > devices. > > It will be used by q35 support. > > > > Cc: Alexander Graf <ag...@suse.de> > > Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp> > > Signed-off-by: Jason Baron <jba...@redhat.com> > > --- > > hw/ide.h | 3 +++ > > hw/ide/ahci.c | 16 ++++++++++++++++ > > 2 files changed, 19 insertions(+), 0 deletions(-) > > > > diff --git a/hw/ide.h b/hw/ide.h > > index 2db4079..8df872e 100644 > > --- a/hw/ide.h > > +++ b/hw/ide.h > > @@ -36,4 +36,7 @@ int ide_get_bios_chs_trans(BusState *bus, int unit); > > /* ide/core.c */ > > void ide_drive_get(DriveInfo **hd, int max_bus); > > > > +/* ide/ahci.c */ > > +void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table); > > + > > #endif /* HW_IDE_H */ > > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c > > index 5ea3cad..9561210 100644 > > --- a/hw/ide/ahci.c > > +++ b/hw/ide/ahci.c > > @@ -1260,3 +1260,19 @@ static void sysbus_ahci_register_types(void) > > } > > > > type_init(sysbus_ahci_register_types) > > + > > +void pci_ahci_ide_create_devs(PCIDevice *pci_dev, DriveInfo **hd_table) > > +{ > > + struct AHCIPCIState *dev = DO_UPCAST(struct AHCIPCIState, card, > > pci_dev); > > + int i; > > + > > + for (i = 0; i < dev->ahci.ports; i++) { > > + /* master device only, ignore slaves */ > > + if (hd_table[i * MAX_IDE_DEVS] == NULL) { > > + continue; > > + } > > + ide_create_drive(&dev->ahci.dev[i].port, 0, > > + hd_table[i * MAX_IDE_DEVS]); > > + } > > +} > > + > > Ignores odd entries in hd_table[] (MAX_IDE_DEVS is 2). Here's my > attempt at explaining why. > > -drive has parameters bus, unit, and index. index and (bus, unit) are > related in a well-known way that depends on parameter if. For if=ide, > index = bus * 2 + unit. This relationship is ABI, i.e. it cannot be > changed. > > "bus * 2 + unit" makes sense for IDE, because each IDE bus can connect > two IDE devices, "master" and "slave". > > Boards implementing IDE reject drives with (bus, unit) that make no > sense for the board's IDE controller(s). A typical board has a single > controller with two buses, which means bus > 1 get rejected. > > q35 implements AHCI instead of IDE. It connects if=ide drives to AHCI, > because that's felt to be convenient. > > An AHCI port can connect a single AHCI device, unlike an IDE bus. This > patch identifies maps -drive's bus to AHCI port number. > > PATCH 11/25 sets up argument hd_table[] as follows: > > ide_drive_get(hd, MAX_SATA_PORTS); > > This rejects bus > MAX_SATA_PORTS. It doesn't reject unit == 1. I > believe these get silently ignored. Bug or feature? > > Should we reject unit == 1 instead? > > Should we map -drive's index to AHCI port number instead?
Right, so now that we have ide disks that can be attached to either the legacy ide controller or to ahci, I think we need to differentiate which controller we mean. That is, as proposed q35 is treating -drive if=ide as an ide attached to the ahci controller. I think that is broken behavior b/c we need a way to differentiate between the controllers. As Alexander Graf has proposed before, I think we need a -drive if=ahci introduced. In that case, I think we reject unit > 0, as you've suggested. In terms of the current q35 patch series, I think the first step would be to introduce the ahci interface type, and have hda-hdd be added with the default type for q35 of ahci. Then, we can simply fetch ahci drives of index 0-3, and populate the controller, without any of this skipping odd numbers stuff. The next step would then to add if=ahci interface to -drive. Thanks, -Jason