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

[Jason Baron]

On Fri, Sep 21, 2012 at 04:05:14PM +0200, Markus Armbruster wrote:
> Jason Baron <address@hidden> writes:
> 
> > From: Isaku Yamahata <address@hidden>
> >
> > Introduce a helper function which initializes the ahci port with ide 
> > devices.
> > It will be used by q35 support.
> >
> > Cc: Alexander Graf <address@hidden>
> > Signed-off-by: Isaku Yamahata <address@hidden>
> > Signed-off-by: Jason Baron <address@hidden>
> > ---
> >  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