Re: [Qemu-devel] [PATCH 04/25] ahci: add ide device initialization helper

[Markus Armbruster <armbru@redhat.com>]

Jason Baron <jbaron@redhat.com> writes:

> On Fri, Sep 21, 2012 at 04:05:14PM +0200, Markus Armbruster wrote:
>> Jason Baron <jbaron@redhat.com> writes:
>> 
>> > From: Isaku Yamahata <yamahata@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 <agraf@suse.de>
>> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>> > Signed-off-by: Jason Baron <jbaron@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?

> 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.

   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.