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.
> > 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.
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.
Thanks,
-Jason
