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

[Markus Armbruster <armbru@redhat.com>]

Jason Baron <jbaron@redhat.com> writes:

> On Mon, Sep 24, 2012 at 06:52:29PM +0200, Markus Armbruster wrote:
>> 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?
>> 
>
> 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?

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