[04/25] ahci: add ide device initialization helper

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(-)

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?

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.

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

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.

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.


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

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.

Am 26.09.2012 10:15, schrieb Markus Armbruster:
> Jason Baron <jbaron@redhat.com> writes:
>> 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?

Because the guest OS doesn't have an AHCI driver.

But I don't think that connecting to a slow PATA controller describes
exactly what we would ideally want to emulate. We'll probably want to
emulate a SATA controller that is in IDE emulation mode (or whatever it
is called). But as long as we don't have it, the PATA controller is
probably the right thing to provide.

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

I'd rephrase as "the board's preferred way to provide an IDE interface",
but yes, I think that's the real question.

If we ever want to make Q35 the default, if=ide must implement this
semantics. If we want to stay with PIIX forever, we have a choice. I'm
leaning towards implementing it anyway and just making if=ahci the
default for Q35 if no if is specified at all.

Kevin

On Wed, Sep 26, 2012 at 10:15:39AM +0200, Markus Armbruster wrote:
> 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?
> 

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