On Wed, Sep 26, 2012 at 10:15:39AM +0200, Markus Armbruster wrote: > Jason Baron <jba...@redhat.com> writes: > > > On Mon, Sep 24, 2012 at 06:52:29PM +0200, Markus Armbruster wrote: > >> Jason Baron <jba...@redhat.com> writes: > >> > >> > 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. > >> > >> What exactly is broken? > >> > > > > I think that -drive if=ide should result in a disk attached piix3-ide. > > Not in an ide disk attached to the ahci controller (which is current q35 > > bahavior, and is 'broken' b/c we don't want that to change after q35 is > > introdued). The reason being is that I think there should be an easy way > > to create an ide drive on piix3-ide, and an ide drive on the ahci > > controller. But it sounds like you don't agree with this point. > > Two issues with that: > > 1. Why should q35 have a piix3-ide? The ICH9 southbridge provides only > SATA, so the board needs additional circuitry to provide PATA. As far > as I can tell, intel's Q35 doesn't. ICH9-based boards that do certainly > won't use a piix3-ide, because that's a *function* of the PIIX > southbridge device. It doesn't exist separately. > > 2. Why should we connect -drive if=ide to a slow PATA controller instead > of a perfectly servicable SATA controller? >
The only way I can install windows xp, and various bsd flavors at the momemnt is by using the PATA controller. > >> > 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. > >> > >> Achieved by setting if_max_devs[IF_AHCI] to one. bus becomes an alias > >> for index, and unit must be zero. > >> > >> Alternatively, keep if_max_devs[IF_AHCI] zero. Swaps role of bus and > >> unit. > >> > >> Alex had if_max_devs[IF_AHCI] = 6. > >> > >> > 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. > >> > >> We discussed if=ahci at length before, without reaching consensus. I'd > >> rather not rehash the old arguments again. Instead, let's examine how > >> the command line should behave, and only then figure out how to get > >> that. > >> > >> 1. Drives created with -hd[a-d], -cdrom, or the non-option image > >> argument should connect to well-known connectors of the board's > >> preferred controller. > >> > >> For current pc, the preferred controller is piix3-ide. -hda connects > >> to its primary bus as master, -hdb as slave. -hdc connects to its > >> secondary bus as master, -hdd as slave. > >> > >> For pseries, the preferred controller is spapr-vscsi. -hda connects > >> as SCSI ID 0, -hdb as 1, and so forth. > >> > >> For s390-virtio, the preferred controller is virtio-blk-s390. -hda > >> and -hdb connect to their own virtio-blk-s390 controller, -hdc and > >> -hdd get silently ignored. Yes, that's wacky. Your current q35 > >> patch is similarly wacky, as far as I can tell: -hdb and -hdd get > >> silently ignored. > >> > >> For q35, the preferred controller is ich9-ahci. I'd expect -hda to > >> connect to port 0, -hdb to port 1, and so forth. > >> > >> Below the hood, -hda is currently like -drive index=0,media=disk, > >> -hd[b-d] same with index=1..3, and -cdrom is like -drive > >> index=2,media=cdrom, independent of the board. > >> > >> It follows that -cdrom connects to the same connector as -hdc for all > >> boards. Fine for pc, but may not be as fine for some other boards. > >> You can't use both -hdc and -cdrom at the same time. > >> > >> The non-option image argument is equivalent to -hda. You can't use > >> both at the same time. > >> > >> 2. Drives created with -drive without if, index, bus, and unit connect > >> to the next unused connector of the board's preferred controller. > >> > >> If all connectors are in use, behavior currently depends on the > >> board, I think. > >> > >> 3. -drive parameters (if, index) provide more control over the connector > >> to use. > >> > >> Which controller you get for which if depends on the board. So does > >> the meaning of index. The details can get messy. > >> > >> For instance, drives with (if, index) the board doesn't support > >> sometimes get ignored silently, and sometimes get flagged as error. > >> > >> Currently, -drive without parameter if is equivalent to either if=ide > >> or if=scsi. Could be changed for new machine types. > >> > >> For q35, -drive index=0..5 should connect to ports 0..5 of the > >> board's ich9-ahci. > >> > >> 4. -drive parameters (bus, unit) are an alternate way to specify > >> parameter index. The mapping between index and (bus, unit) depends > >> on the board. > >> > >> Actually, it depends only on parameter if right now. For if=ide, > >> index = bus * 2 + unit, unit<2. For if=scsi, index = bus * 7 + unit, > >> unit < 7. For everything else, index = unit, bus = 0. We might want > >> to make it depend on the board, see commit 27d6bf40. > >> > >> For q35, we want index = bus * 6 + unit, unit<5. > >> > >> An easy way to get that is new if=ahci. Backfires when an AHCI > >> controller with a different number of ports shows up. > >> > > > > I agree with points 1-4. > > > >> We really should make the mapping between index and (bus, unit) > >> depend on the board. And then we can just as well use if=ide to > >> refer to q35's one and only IDE-like controller ich9-ahci. > > > > I agree mapping should depend on the board. > > We agree on the important things then. > > > But I'm not sure about using if=ide to use ich9-ahci. I'm suggesting > > that if=ide should continue to refer to piix3-ide. > > Perhaps it should mean "the board's preferred ATA controller", perhaps > it should mean "the board's preferred PATA controller". Certainly > debatable. > > If the latter, not necessarily piix3-ide. > Sure. My main point was that 'if=ide' in the q35 context doesn't specify a SATA controller. I think it would be nice to have a short hand way of specify both PATA and SATA on q35. Thanks, -Jason