[Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[John Snow]

Currently, the drive definitions created by drive_new() when using
the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
picked up by the Q35 initialization routine.

To fix this, we have to add hooks to search for these drives using
something like pc_piix's ide_drive_get and then add them using
something like pci_ide_create_devs.

Where it gets slightly wonky is the fact that if=ide is specified
to use two devices per bus, whereas AHCI does not utilize that
same master/slave mechanic. Therefore, many places inside of
blockdev.c where we add and define new drives use incorrect math
for AHCI devices and try to place them on impossible buses.
Notably -hdb and -hdd would become inaccessible.

To remedy this, I added a new interface type, IF_AHCI. Corresponding
to this change, I modified the default machine properties for Q35
to use this interface as a default.

The changes appear to work well, but where I'd like some feedback
is what should happen if people do something like:

qemu -M q35 -drive if=ide,file=fedora.qcow2

The code as presented here is not going to look for or attempt to
connect IDE devices, because it is now looking for /AHCI/ devices.

At worst, this may break a few existing scripts, but I actually think
that since the if=ide,file=... shorthand never worked to begin with,
the impact might actually be minimal.

But since the legacy IDE interface of the ICH9 is as of yet unemulated,
the if=ide drives don't have a reasonable place to go yet. I am also
not sure what a reasonable way to handle people specifying BOTH
if=ide and if=ahci drives would be.

John Snow (4):
  blockdev: add if_get_max_devs
  blockdev: add IF_AHCI to support -cdrom and -hd[a-d]
  ide: update ide_drive_get to work with both PCI-IDE and AHCI
    interfaces
  ahci: implement -cdrom and -hd[a-d]

 blockdev.c                | 18 +++++++++++++++---
 hw/i386/pc_piix.c         |  2 +-
 hw/i386/pc_q35.c          |  4 ++++
 hw/ide/ahci.c             | 17 +++++++++++++++++
 hw/ide/ahci.h             |  2 ++
 hw/ide/core.c             | 11 +++++++----
 include/hw/ide.h          |  3 ++-
 include/sysemu/blockdev.h |  3 ++-
 8 files changed, 50 insertions(+), 10 deletions(-)

-- 
1.9.3

[Qemu-devel] [RFC 1/4] blockdev: add if_get_max_devs

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 9 +++++++++
 include/sysemu/blockdev.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 48bd9a3..58da77f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -110,6 +110,15 @@ void blockdev_auto_del(BlockDriverState *bs)
     }
 }
 
+int if_get_max_devs(BlockInterfaceType type)
+{
+    if (type >= IF_IDE && type < IF_COUNT) {
+        return if_max_devs[type];
+    }
+
+    return 0;
+}
+
 static int drive_index_to_bus_id(BlockInterfaceType type, int index)
 {
     int max_devs = if_max_devs[type];
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 23a5d10..2420abd 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -48,6 +48,7 @@ struct DriveInfo {
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
+int if_get_max_devs(BlockInterfaceType type);
 DriveInfo *drive_get_next(BlockInterfaceType type);
 DriveInfo *drive_get_by_blockdev(BlockDriverState *bs);
 
-- 
1.9.3

[Qemu-devel] [RFC 2/4] blockdev: add IF_AHCI to support -cdrom and -hd[a-d]

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c                | 9 ++++++---
 include/sysemu/blockdev.h | 2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 58da77f..a9efe1f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -57,6 +57,7 @@ static const char *const if_name[IF_COUNT] = {
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
     [IF_XEN] = "xen",
+    [IF_AHCI] = "ahci",
 };
 
 static const int if_max_devs[IF_COUNT] = {
@@ -76,6 +77,7 @@ static const int if_max_devs[IF_COUNT] = {
      */
     [IF_IDE] = 2,
     [IF_SCSI] = 7,
+    [IF_AHCI] = 1,
 };
 
 /*
@@ -876,7 +878,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     if (qemu_opts_id(all_opts) == NULL) {
         char *new_id;
         const char *mediastr = "";
-        if (type == IF_IDE || type == IF_SCSI) {
+        if (type == IF_IDE || type == IF_SCSI || type == IF_AHCI) {
             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
         }
         if (max_devs) {
@@ -918,7 +920,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     werror = qemu_opt_get(legacy_opts, "werror");
     if (werror != NULL) {
         if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO &&
-            type != IF_NONE) {
+            type != IF_NONE && type != IF_AHCI) {
             error_report("werror is not supported by this bus type");
             goto fail;
         }
@@ -928,7 +930,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
     rerror = qemu_opt_get(legacy_opts, "rerror");
     if (rerror != NULL) {
         if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI &&
-            type != IF_NONE) {
+            type != IF_NONE && type != IF_AHCI) {
             error_report("rerror is not supported by this bus type");
             goto fail;
         }
@@ -966,6 +968,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     switch(type) {
     case IF_IDE:
+    case IF_AHCI:
     case IF_SCSI:
     case IF_XEN:
     case IF_NONE:
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 2420abd..6b02cf4 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -25,7 +25,7 @@ typedef enum {
      */
     IF_IDE = 0,
     IF_NONE,
-    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN, IF_AHCI,
     IF_COUNT
 } BlockInterfaceType;
 
-- 
1.9.3

[Qemu-devel] [RFC 3/4] ide: update ide_drive_get to work with both PCI-IDE and AHCI interfaces

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/i386/pc_piix.c |  2 +-
 hw/ide/core.c     | 11 +++++++----
 include/hw/ide.h  |  3 ++-
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 47ac1b5..9da6f0e 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -239,7 +239,7 @@ static void pc_init1(MachineState *machine,
 
     pc_nic_init(isa_bus, pci_bus);
 
-    ide_drive_get(hd, MAX_IDE_BUS);
+    ide_drive_get(hd, IF_IDE, MAX_IDE_BUS);
     if (pci_enabled) {
         PCIDevice *dev;
         if (xen_enabled()) {
diff --git a/hw/ide/core.c b/hw/ide/core.c
index b48127f..408d7c1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2481,16 +2481,19 @@ const VMStateDescription vmstate_ide_bus = {
     }
 };
 
-void ide_drive_get(DriveInfo **hd, int max_bus)
+void ide_drive_get(DriveInfo **hd, BlockInterfaceType type, int max_bus)
 {
     int i;
 
-    if (drive_get_max_bus(IF_IDE) >= max_bus) {
+    if (drive_get_max_bus(type) >= max_bus) {
         fprintf(stderr, "qemu: too many IDE bus: %d\n", max_bus);
         exit(1);
     }
 
-    for(i = 0; i < max_bus * MAX_IDE_DEVS; i++) {
-        hd[i] = drive_get(IF_IDE, i / MAX_IDE_DEVS, i % MAX_IDE_DEVS);
+    for (i = 0; i < max_bus * if_get_max_devs(type); i++) {
+        fprintf(stderr, "hd[%u] := drive_get_by_index(%d, %u);\n",
+                i, type, i);
+        hd[i] = drive_get_by_index(type, i);
+        fprintf(stderr, "hd[%u] := %p\n", i, (void *)hd[i]);
     }
 }
diff --git a/include/hw/ide.h b/include/hw/ide.h
index bc8bd32..c4400ab 100644
--- a/include/hw/ide.h
+++ b/include/hw/ide.h
@@ -4,6 +4,7 @@
 #include "hw/isa/isa.h"
 #include "hw/pci/pci.h"
 #include "exec/memory.h"
+#include "sysemu/blockdev.h"
 
 #define MAX_IDE_DEVS	2
 
@@ -28,6 +29,6 @@ int ide_get_geometry(BusState *bus, int unit,
 int ide_get_bios_chs_trans(BusState *bus, int unit);
 
 /* ide/core.c */
-void ide_drive_get(DriveInfo **hd, int max_bus);
+void ide_drive_get(DriveInfo **hd, BlockInterfaceType type, int max_bus);
 
 #endif /* HW_IDE_H */
-- 
1.9.3

[Qemu-devel] [RFC 4/4] ahci: implement -cdrom and -hd[a-d]

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/i386/pc_q35.c |  4 ++++
 hw/ide/ahci.c    | 17 +++++++++++++++++
 hw/ide/ahci.h    |  2 ++
 3 files changed, 23 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 4b5a274..4613565 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -86,6 +86,7 @@ static void pc_q35_init(MachineState *machine)
     DeviceState *icc_bridge;
     PcGuestInfo *guest_info;
     ram_addr_t lowmem;
+    DriveInfo *hd[MAX_SATA_PORTS];
 
     /* Check whether RAM fits below 4G (leaving 1/2 GByte for IO memory
      * and 256 Mbytes for PCI Express Enhanced Configuration Access Mapping
@@ -253,6 +254,8 @@ static void pc_q35_init(MachineState *machine)
                                            true, "ich9-ahci");
     idebus[0] = qdev_get_child_bus(&ahci->qdev, "ide.0");
     idebus[1] = qdev_get_child_bus(&ahci->qdev, "ide.1");
+    ide_drive_get(hd, IF_AHCI, MAX_SATA_PORTS);
+    ahci_ide_create_devs(ahci, hd);
 
     if (usb_enabled(false)) {
         /* Should we create 6 UHCI according to ich9 spec? */
@@ -354,6 +357,7 @@ static QEMUMachine pc_q35_machine_v2_2 = {
     .name = "pc-q35-2.2",
     .alias = "q35",
     .init = pc_q35_init,
+    .block_default_type = IF_AHCI,
 };
 
 #define PC_Q35_2_1_MACHINE_OPTIONS PC_Q35_2_2_MACHINE_OPTIONS
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 4cda0d0..bc8b8e9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1403,3 +1403,20 @@ static void sysbus_ahci_register_types(void)
 }
 
 type_init(sysbus_ahci_register_types)
+
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
+{
+    AHCIPCIState *d = ICH_AHCI(dev);
+    AHCIState *ahci = &d->ahci;
+    unsigned i;
+
+    for (i = 0; i < ahci->ports; i++) {
+        if (hd_table[i] == NULL) {
+            continue;
+        }
+        fprintf(stderr, "ahci_ide_create_devs: ide_create_drive(bus:%p, 0, drive[index:%u])\n",
+                (void *)&ahci->dev[i].port, i);
+        ide_create_drive(&ahci->dev[i].port, 0, hd_table[i]);
+    }
+
+}
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1543df7..41daedb 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -332,4 +332,6 @@ void ahci_uninit(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
+void ahci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
+
 #endif /* HW_IDE_AHCI_H */
-- 
1.9.3

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Currently, the drive definitions created by drive_new() when using
> the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
> picked up by the Q35 initialization routine.
>
> To fix this, we have to add hooks to search for these drives using
> something like pc_piix's ide_drive_get and then add them using
> something like pci_ide_create_devs.

ide_drive_get() isn't pc_piix's, it's a helper function in the IDE core
which boards (not just pc_piix) use to find the if=ide drives.  It fills
in an array of DriveInfo.

pci_ide_create_devs() is a helper function in the IDE PCI code which PCI
IDE controllers (not just piix3-ide) use to create IDE devices for an
array of DriveInfo.

> Where it gets slightly wonky is the fact that if=ide is specified
> to use two devices per bus, whereas AHCI does not utilize that
> same master/slave mechanic. Therefore, many places inside of
> blockdev.c where we add and define new drives use incorrect math
> for AHCI devices and try to place them on impossible buses.
> Notably -hdb and -hdd would become inaccessible.

Yes.

> To remedy this, I added a new interface type, IF_AHCI. Corresponding
> to this change, I modified the default machine properties for Q35
> to use this interface as a default.
>
> The changes appear to work well, but where I'd like some feedback
> is what should happen if people do something like:
>
> qemu -M q35 -drive if=ide,file=fedora.qcow2
>
> The code as presented here is not going to look for or attempt to
> connect IDE devices, because it is now looking for /AHCI/ devices.
>
> At worst, this may break a few existing scripts, but I actually think
> that since the if=ide,file=... shorthand never worked to begin with,
> the impact might actually be minimal.
>
> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
> the if=ide drives don't have a reasonable place to go yet. I am also
> not sure what a reasonable way to handle people specifying BOTH
> if=ide and if=ahci drives would be.

We've been through IF_AHCI before, more than once, but that was before
you got involved :)

The problem at hand is that "-drive if=ide" and its sugared forms -hda,
-hdb, -cdrom, ... don't work with Q35.

You provide a solution for the sugared forms, you leave the problem
unsolved for the unsugared form, and you add a new problem: -drive
if=ahci doesn't work with i440FX.  Progress, sort of.

Let's take a step back, and recap previous discussion.  There are two
defensible point of views, in my opinion.

One is that IDE and AHCI should be separate interface types, just like
IDE and SCSI are.

Attempts to define an if=X drive with a board that doesn't provide a
controller for X should fail[*].  Only onboard controllers matter,
add-ons plugged in with -device don't.  An i440FX board provides only
IDE.  A Q35 board provides only AHCI, not IDE.  If we implement an
ich9-ahci legacy mode, and switch it on, then it provides only IDE, not
AHCI.  Or maybe both, depending on how we do it.

The other point of view is that IDE and AHCI are no more different than
the different kinds of SCSI HBAs.  This is certainly true from a qdev
point of view: just like SCSI devices can connect to any SCSI qbus,
regardless of the HBA providing it, so can IDE devices connect to any
IDE qbus, regardless of the controller providing it.

There's a wrinkle: the mapping between index to (bus, unit).  This
mapping is ABI.  The current mapping makes sense for the first
generation of controllers: PATA (two devices per bus, thus
if_max_devs[IF_IDE] = 2), and 8-bit SCSI (seven per bus, thus
if_max_devs[IF_SCSI = 7).

The mapping is silly for newer SCSI HBAs.  Commit 622b520f tried to make
it less silly, but had to be reverted in 27d6bf4 because the silliness
was ABI.

The mapping is also silly for ich9-ahci.  You side-step that silliness
only, by adding a new interface type for it.  But shouldn't we add a
number of SCSI interface types then, too?  Where does that end?

Can we do better?  I think we can, by making this part of the ABI
board-specific.  The general form of the mapping remains

    (bus, unit) = (index / N, index % N)

but N now depends on board and interface type, not just the latter.

If the board connects if=scsi to an lsi53c895a, then N = 7.

If the board connects if=ide to an piix3-ide, then N = 2.

If the board connects if=ide to an ich9-ahci, then N = 1.

I trust you get the idea :)


[*] Currently, they're silently ignored with most boards for most X, but
I regard that as implementation defect.

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[John Snow]

On 08/19/2014 04:05 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Currently, the drive definitions created by drive_new() when using
>> the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
>> picked up by the Q35 initialization routine.
>>
>> To fix this, we have to add hooks to search for these drives using
>> something like pc_piix's ide_drive_get and then add them using
>> something like pci_ide_create_devs.
>
> ide_drive_get() isn't pc_piix's, it's a helper function in the IDE core
> which boards (not just pc_piix) use to find the if=ide drives.  It fills
> in an array of DriveInfo.
>
> pci_ide_create_devs() is a helper function in the IDE PCI code which PCI
> IDE controllers (not just piix3-ide) use to create IDE devices for an
> array of DriveInfo.

Yes. I meant to say pc_piix's /call to/ ide_drive_get. I would have to 
patch up the other boards if I changed this function! Only an RFC before 
I got too far down this path :]

>> Where it gets slightly wonky is the fact that if=ide is specified
>> to use two devices per bus, whereas AHCI does not utilize that
>> same master/slave mechanic. Therefore, many places inside of
>> blockdev.c where we add and define new drives use incorrect math
>> for AHCI devices and try to place them on impossible buses.
>> Notably -hdb and -hdd would become inaccessible.
>
> Yes.
>
>> To remedy this, I added a new interface type, IF_AHCI. Corresponding
>> to this change, I modified the default machine properties for Q35
>> to use this interface as a default.
>>
>> The changes appear to work well, but where I'd like some feedback
>> is what should happen if people do something like:
>>
>> qemu -M q35 -drive if=ide,file=fedora.qcow2
>>
>> The code as presented here is not going to look for or attempt to
>> connect IDE devices, because it is now looking for /AHCI/ devices.
>>
>> At worst, this may break a few existing scripts, but I actually think
>> that since the if=ide,file=... shorthand never worked to begin with,
>> the impact might actually be minimal.
>>
>> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
>> the if=ide drives don't have a reasonable place to go yet. I am also
>> not sure what a reasonable way to handle people specifying BOTH
>> if=ide and if=ahci drives would be.
>
> We've been through IF_AHCI before, more than once, but that was before
> you got involved :)
>
> The problem at hand is that "-drive if=ide" and its sugared forms -hda,
> -hdb, -cdrom, ... don't work with Q35.
>
> You provide a solution for the sugared forms, you leave the problem
> unsolved for the unsugared form, and you add a new problem: -drive
> if=ahci doesn't work with i440FX.  Progress, sort of.

Adding a call to boards that support the AHCI device during their 
initialization should be easy enough, if we decide that "ide means 
ISA/PCI, ahci means the AHCI HBA." I could probably even write one 
generic routine between i440fx and q35 to do both IDE/AHCI.

If we decide that IF_IDE and IF_AHCI mean different things, the problem 
of the unsugared form being unsolved depends on me (well, or someone) 
implementing the legacy IDE interface for Q35.

> Let's take a step back, and recap previous discussion.  There are two
> defensible point of views, in my opinion.
>
> One is that IDE and AHCI should be separate interface types, just like
> IDE and SCSI are.
>
> Attempts to define an if=X drive with a board that doesn't provide a
> controller for X should fail[*].  Only onboard controllers matter,
> add-ons plugged in with -device don't.  An i440FX board provides only
> IDE.  A Q35 board provides only AHCI, not IDE.  If we implement an
> ich9-ahci legacy mode, and switch it on, then it provides only IDE, not
> AHCI.  Or maybe both, depending on how we do it.

I think I am leaning towards this viewpoint, but it depends on what 
"interface" means in QEMU. Currently, the number of units per bus is 
tied to the "interface" and clearly the AHCI SATA interface only 
supports one per bus, so semantically this makes sense.

I think the real ICH9 AHCI device supports only fully AHCI or fully 
legacy, but the AHCI spec itself appears to allow you to run a 
mixed-mode device.

I am not sure we have a usage case for mixed-mode, so enforcing 
either/or for the AHCI device makes sense for now, I think.

> The other point of view is that IDE and AHCI are no more different than
> the different kinds of SCSI HBAs.  This is certainly true from a qdev
> point of view: just like SCSI devices can connect to any SCSI qbus,
> regardless of the HBA providing it, so can IDE devices connect to any
> IDE qbus, regardless of the controller providing it.

Yes... Really the only difference are some mapping semantics. I don't 
think there's any /other/ reason I needed IF_AHCI, of course, I wasn't 
around for the previous discussions, so maybe there are other reasons I 
am not aware of.

> There's a wrinkle: the mapping between index to (bus, unit).  This
> mapping is ABI.  The current mapping makes sense for the first
> generation of controllers: PATA (two devices per bus, thus
> if_max_devs[IF_IDE] = 2), and 8-bit SCSI (seven per bus, thus
> if_max_devs[IF_SCSI = 7).
>
> The mapping is silly for newer SCSI HBAs.  Commit 622b520f tried to make
> it less silly, but had to be reverted in 27d6bf4 because the silliness
> was ABI.
>
> The mapping is also silly for ich9-ahci.  You side-step that silliness
> only, by adding a new interface type for it.  But shouldn't we add a
> number of SCSI interface types then, too?  Where does that end?
>
> Can we do better?  I think we can, by making this part of the ABI
> board-specific.  The general form of the mapping remains
>
>      (bus, unit) = (index / N, index % N)
>
> but N now depends on board and interface type, not just the latter.
>
> If the board connects if=scsi to an lsi53c895a, then N = 7.
>
> If the board connects if=ide to an piix3-ide, then N = 2.
>
> If the board connects if=ide to an ich9-ahci, then N = 1.
>
> I trust you get the idea :)

I suppose we could make it something like:
if (HBA.max_units > 0) {
   N := min(HBA.max_units, IF.max_units);
} else {
   N := IF.max_units;
}
(bus, unit) := (index / N, index % N);

Which sets a default property for the interface but allows the device 
(not the board) to override. Does that make more sense? If we allow 
people to wire up an AHCI device to piix, we'll run back into the same 
problems of the bus/unit mappings unless we make this a device property.

I do feel like I'd rather just make it an interface property and have 
people specify which type of bus they want to wire it up to, but that 
does create a lot of disparity against the SCSI devices.

> [*] Currently, they're silently ignored with most boards for most X, but
> I regard that as implementation defect.

Yes. Is there a bool in the drive info array that we can set to say 
"this drive has been added as a device" and check for any that went 
unset? I can add one and a routine to check for it, which may help flush 
out more of the weird legacy sugar option bugs.
(Sugar attracts bugs. heh-heh-heh...)

-- 
—js

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[Dr. David Alan Gilbert]

* John Snow (jsnow@redhat.com) wrote:

<snip>

> The changes appear to work well, but where I'd like some feedback
> is what should happen if people do something like:
> 
> qemu -M q35 -drive if=ide,file=fedora.qcow2
> 
> The code as presented here is not going to look for or attempt to
> connect IDE devices, because it is now looking for /AHCI/ devices.

What happens if you try it - does it just silently ignore that -drive?
The ideal would be for something to moan about unused drives so people
realise why it's broken; but I seem to remember form a previous discussion
it's hard to do on all platforms.

Dave

> 
> At worst, this may break a few existing scripts, but I actually think
> that since the if=ide,file=... shorthand never worked to begin with,
> the impact might actually be minimal.
> 
> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
> the if=ide drives don't have a reasonable place to go yet. I am also
> not sure what a reasonable way to handle people specifying BOTH
> if=ide and if=ahci drives would be.
> 
> John Snow (4):
>   blockdev: add if_get_max_devs
>   blockdev: add IF_AHCI to support -cdrom and -hd[a-d]
>   ide: update ide_drive_get to work with both PCI-IDE and AHCI
>     interfaces
>   ahci: implement -cdrom and -hd[a-d]
> 
>  blockdev.c                | 18 +++++++++++++++---
>  hw/i386/pc_piix.c         |  2 +-
>  hw/i386/pc_q35.c          |  4 ++++
>  hw/ide/ahci.c             | 17 +++++++++++++++++
>  hw/ide/ahci.h             |  2 ++
>  hw/ide/core.c             | 11 +++++++----
>  include/hw/ide.h          |  3 ++-
>  include/sysemu/blockdev.h |  3 ++-
>  8 files changed, 50 insertions(+), 10 deletions(-)
> 
> -- 
> 1.9.3
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[John Snow]

On 08/19/2014 12:12 PM, Dr. David Alan Gilbert wrote:
> * John Snow (jsnow@redhat.com) wrote:
>
> <snip>
>
>> The changes appear to work well, but where I'd like some feedback
>> is what should happen if people do something like:
>>
>> qemu -M q35 -drive if=ide,file=fedora.qcow2
>>
>> The code as presented here is not going to look for or attempt to
>> connect IDE devices, because it is now looking for /AHCI/ devices.
>
> What happens if you try it - does it just silently ignore that -drive?
> The ideal would be for something to moan about unused drives so people
> realise why it's broken; but I seem to remember form a previous discussion
> it's hard to do on all platforms.

With this patch, piix would continue to ignore AHCI and Q35 would ignore 
IDE. piix and Q35 both can be corrected to look for either/or IDE/AHCI 
and whine if you mix them. Q35 and piix both could be made to throw an 
error if you specify drives that the board doesn't pick up (AHCI in the 
case of piix if you have not specified that device, legacy IDE in the 
case of Q35 until we add legacy mode support and the user specifies that 
mode.)

I suggested to Markus in a different reply that I could add a little 
flag to the drive list to help identify if any drives don't get picked 
up at board spin-up time... But it sounds like it might not be quite so 
simple.

I will wait to add error handling modes until there is perhaps some 
consensus that the basic approach here is desirable.

-- 
—js

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 08/19/2014 04:05 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> Currently, the drive definitions created by drive_new() when using
>>> the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
>>> picked up by the Q35 initialization routine.
>>>
>>> To fix this, we have to add hooks to search for these drives using
>>> something like pc_piix's ide_drive_get and then add them using
>>> something like pci_ide_create_devs.
>>
>> ide_drive_get() isn't pc_piix's, it's a helper function in the IDE core
>> which boards (not just pc_piix) use to find the if=ide drives.  It fills
>> in an array of DriveInfo.
>>
>> pci_ide_create_devs() is a helper function in the IDE PCI code which PCI
>> IDE controllers (not just piix3-ide) use to create IDE devices for an
>> array of DriveInfo.
>
> Yes. I meant to say pc_piix's /call to/ ide_drive_get. I would have to
> patch up the other boards if I changed this function! Only an RFC
> before I got too far down this path :]
>
>>> Where it gets slightly wonky is the fact that if=ide is specified
>>> to use two devices per bus, whereas AHCI does not utilize that
>>> same master/slave mechanic. Therefore, many places inside of
>>> blockdev.c where we add and define new drives use incorrect math
>>> for AHCI devices and try to place them on impossible buses.
>>> Notably -hdb and -hdd would become inaccessible.
>>
>> Yes.
>>
>>> To remedy this, I added a new interface type, IF_AHCI. Corresponding
>>> to this change, I modified the default machine properties for Q35
>>> to use this interface as a default.
>>>
>>> The changes appear to work well, but where I'd like some feedback
>>> is what should happen if people do something like:
>>>
>>> qemu -M q35 -drive if=ide,file=fedora.qcow2
>>>
>>> The code as presented here is not going to look for or attempt to
>>> connect IDE devices, because it is now looking for /AHCI/ devices.
>>>
>>> At worst, this may break a few existing scripts, but I actually think
>>> that since the if=ide,file=... shorthand never worked to begin with,
>>> the impact might actually be minimal.
>>>
>>> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
>>> the if=ide drives don't have a reasonable place to go yet. I am also
>>> not sure what a reasonable way to handle people specifying BOTH
>>> if=ide and if=ahci drives would be.
>>
>> We've been through IF_AHCI before, more than once, but that was before
>> you got involved :)
>>
>> The problem at hand is that "-drive if=ide" and its sugared forms -hda,
>> -hdb, -cdrom, ... don't work with Q35.
>>
>> You provide a solution for the sugared forms, you leave the problem
>> unsolved for the unsugared form, and you add a new problem: -drive
>> if=ahci doesn't work with i440FX.  Progress, sort of.
>
> Adding a call to boards that support the AHCI device during their
> initialization should be easy enough, if we decide that "ide means
> ISA/PCI, ahci means the AHCI HBA." I could probably even write one
> generic routine between i440fx and q35 to do both IDE/AHCI.
>
> If we decide that IF_IDE and IF_AHCI mean different things, the
> problem of the unsugared form being unsolved depends on me (well, or
> someone) implementing the legacy IDE interface for Q35.

Let me come back to this further down.

>> Let's take a step back, and recap previous discussion.  There are two
>> defensible point of views, in my opinion.
>>
>> One is that IDE and AHCI should be separate interface types, just like
>> IDE and SCSI are.
>>
>> Attempts to define an if=X drive with a board that doesn't provide a
>> controller for X should fail[*].  Only onboard controllers matter,
>> add-ons plugged in with -device don't.  An i440FX board provides only
>> IDE.  A Q35 board provides only AHCI, not IDE.  If we implement an
>> ich9-ahci legacy mode, and switch it on, then it provides only IDE, not
>> AHCI.  Or maybe both, depending on how we do it.
>
> I think I am leaning towards this viewpoint, but it depends on what
> "interface" means in QEMU. Currently, the number of units per bus is
> tied to the "interface" and clearly the AHCI SATA interface only
> supports one per bus, so semantically this makes sense.

An index <-> (bus, unit) mapping doesn't make an interface!  Yes, it's
tightly coupled to the interface, but that became wrong way back when we
went beyond 8-bit SCSI HBAs, long before we added up AHCI HBAs.

> I think the real ICH9 AHCI device supports only fully AHCI or fully
> legacy, but the AHCI spec itself appears to allow you to run a
> mixed-mode device.
>
> I am not sure we have a usage case for mixed-mode, so enforcing
> either/or for the AHCI device makes sense for now, I think.

I can't see a use for mixed mode, either.

>> The other point of view is that IDE and AHCI are no more different than
>> the different kinds of SCSI HBAs.  This is certainly true from a qdev
>> point of view: just like SCSI devices can connect to any SCSI qbus,
>> regardless of the HBA providing it, so can IDE devices connect to any
>> IDE qbus, regardless of the controller providing it.
>
> Yes... Really the only difference are some mapping semantics. I don't
> think there's any /other/ reason I needed IF_AHCI, of course, I wasn't
> around for the previous discussions, so maybe there are other reasons
> I am not aware of.

I've always been in the "we don't need or want if=ahci" camp :)

The one argument for if=ahci I found convincing was a desire for Q35
with its ICH9 in legacy mode.  And that's just as easily done with a
machine option.  Personally, I find that more natural.

>> There's a wrinkle: the mapping between index to (bus, unit).  This
>> mapping is ABI.  The current mapping makes sense for the first
>> generation of controllers: PATA (two devices per bus, thus
>> if_max_devs[IF_IDE] = 2), and 8-bit SCSI (seven per bus, thus
>> if_max_devs[IF_SCSI = 7).
>>
>> The mapping is silly for newer SCSI HBAs.  Commit 622b520f tried to make
>> it less silly, but had to be reverted in 27d6bf4 because the silliness
>> was ABI.
>>
>> The mapping is also silly for ich9-ahci.  You side-step that silliness
>> only, by adding a new interface type for it.  But shouldn't we add a
>> number of SCSI interface types then, too?  Where does that end?
>>
>> Can we do better?  I think we can, by making this part of the ABI
>> board-specific.  The general form of the mapping remains
>>
>>      (bus, unit) = (index / N, index % N)
>>
>> but N now depends on board and interface type, not just the latter.
>>
>> If the board connects if=scsi to an lsi53c895a, then N = 7.
>>
>> If the board connects if=ide to an piix3-ide, then N = 2.
>>
>> If the board connects if=ide to an ich9-ahci, then N = 1.
>>
>> I trust you get the idea :)
>
> I suppose we could make it something like:
> if (HBA.max_units > 0) {
>   N := min(HBA.max_units, IF.max_units);
> } else {
>   N := IF.max_units;
> }
> (bus, unit) := (index / N, index % N);
>
> Which sets a default property for the interface but allows the device
> (not the board) to override. Does that make more sense? If we allow
> people to wire up an AHCI device to piix, we'll run back into the same
> problems of the bus/unit mappings unless we make this a device
> property.

Yes, it is a property of the device (property not in the qdev sense).

Why would HBA.max_units ever be greater than IF.max_units?

If the answer is "only if somebody screwed up the HBA device model",
then the above can be simplified to just N = HBA.max_units.

> I do feel like I'd rather just make it an interface property and have
> people specify which type of bus they want to wire it up to, but that
> does create a lot of disparity against the SCSI devices.

What do you mean by "interface property"?

>> [*] Currently, they're silently ignored with most boards for most X, but
>> I regard that as implementation defect.
>
> Yes. Is there a bool in the drive info array that we can set to say
> "this drive has been added as a device" and check for any that went
> unset?

Not yet :)

>        I can add one and a routine to check for it, which may help
> flush out more of the weird legacy sugar option bugs.

We do something like that for -netdev:

    $ qemu -nodefaults -display none -netdev user,id=foo
    Warning: netdev foo has no peer

and -net:

    $ qemu -nodefaults -display none -net user,id=foo
    Warning: vlan 0 with no nics

I think as long as we leave picking up configuration to boards, having
the boards mark the pieces they pick up is the best we can do.

An alternative to leaving it to boards is making the boads define
callbacks that get fed configuration.  But that's more surgery.

There's more than just -netdev, -net and -drive, though.  Many command
line options to configure devices also merely create a piece of
configuration for boards to pick up:

* Character devices (-serial, -parallel) end up in serial_hds[],
  parallel_hds[].

* Graphics devices (-vga) end up in vga_interface_type.

These all need the same "did the board pick it up?" check as -drive.

Some old options have been converted to work independent of boards:

* Virtio consoles (-virtioconsole) in commit 98b1925.

* Sound devices (-soundhw) in commit b3e6d59.

Newer convenience options should always worked this way.  -watchdog
does.

I may have missed options.

> (Sugar attracts bugs. heh-heh-heh...)

Indeed!

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[John Snow]

On 08/19/2014 02:08 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 08/19/2014 04:05 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Currently, the drive definitions created by drive_new() when using
>>>> the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
>>>> picked up by the Q35 initialization routine.
>>>>
>>>> To fix this, we have to add hooks to search for these drives using
>>>> something like pc_piix's ide_drive_get and then add them using
>>>> something like pci_ide_create_devs.
>>>
>>> ide_drive_get() isn't pc_piix's, it's a helper function in the IDE core
>>> which boards (not just pc_piix) use to find the if=ide drives.  It fills
>>> in an array of DriveInfo.
>>>
>>> pci_ide_create_devs() is a helper function in the IDE PCI code which PCI
>>> IDE controllers (not just piix3-ide) use to create IDE devices for an
>>> array of DriveInfo.
>>
>> Yes. I meant to say pc_piix's /call to/ ide_drive_get. I would have to
>> patch up the other boards if I changed this function! Only an RFC
>> before I got too far down this path :]
>>
>>>> Where it gets slightly wonky is the fact that if=ide is specified
>>>> to use two devices per bus, whereas AHCI does not utilize that
>>>> same master/slave mechanic. Therefore, many places inside of
>>>> blockdev.c where we add and define new drives use incorrect math
>>>> for AHCI devices and try to place them on impossible buses.
>>>> Notably -hdb and -hdd would become inaccessible.
>>>
>>> Yes.
>>>
>>>> To remedy this, I added a new interface type, IF_AHCI. Corresponding
>>>> to this change, I modified the default machine properties for Q35
>>>> to use this interface as a default.
>>>>
>>>> The changes appear to work well, but where I'd like some feedback
>>>> is what should happen if people do something like:
>>>>
>>>> qemu -M q35 -drive if=ide,file=fedora.qcow2
>>>>
>>>> The code as presented here is not going to look for or attempt to
>>>> connect IDE devices, because it is now looking for /AHCI/ devices.
>>>>
>>>> At worst, this may break a few existing scripts, but I actually think
>>>> that since the if=ide,file=... shorthand never worked to begin with,
>>>> the impact might actually be minimal.
>>>>
>>>> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
>>>> the if=ide drives don't have a reasonable place to go yet. I am also
>>>> not sure what a reasonable way to handle people specifying BOTH
>>>> if=ide and if=ahci drives would be.
>>>
>>> We've been through IF_AHCI before, more than once, but that was before
>>> you got involved :)
>>>
>>> The problem at hand is that "-drive if=ide" and its sugared forms -hda,
>>> -hdb, -cdrom, ... don't work with Q35.
>>>
>>> You provide a solution for the sugared forms, you leave the problem
>>> unsolved for the unsugared form, and you add a new problem: -drive
>>> if=ahci doesn't work with i440FX.  Progress, sort of.
>>
>> Adding a call to boards that support the AHCI device during their
>> initialization should be easy enough, if we decide that "ide means
>> ISA/PCI, ahci means the AHCI HBA." I could probably even write one
>> generic routine between i440fx and q35 to do both IDE/AHCI.
>>
>> If we decide that IF_IDE and IF_AHCI mean different things, the
>> problem of the unsugared form being unsolved depends on me (well, or
>> someone) implementing the legacy IDE interface for Q35.
>
> Let me come back to this further down.
>
>>> Let's take a step back, and recap previous discussion.  There are two
>>> defensible point of views, in my opinion.
>>>
>>> One is that IDE and AHCI should be separate interface types, just like
>>> IDE and SCSI are.
>>>
>>> Attempts to define an if=X drive with a board that doesn't provide a
>>> controller for X should fail[*].  Only onboard controllers matter,
>>> add-ons plugged in with -device don't.  An i440FX board provides only
>>> IDE.  A Q35 board provides only AHCI, not IDE.  If we implement an
>>> ich9-ahci legacy mode, and switch it on, then it provides only IDE, not
>>> AHCI.  Or maybe both, depending on how we do it.
>>
>> I think I am leaning towards this viewpoint, but it depends on what
>> "interface" means in QEMU. Currently, the number of units per bus is
>> tied to the "interface" and clearly the AHCI SATA interface only
>> supports one per bus, so semantically this makes sense.
>
> An index <-> (bus, unit) mapping doesn't make an interface!  Yes, it's
> tightly coupled to the interface, but that became wrong way back when we
> went beyond 8-bit SCSI HBAs, long before we added up AHCI HBAs.

"Interface" seems nebulous in QEMU. In the physical world, there 
definitely is a difference between IDE/EIDE/PATA and SATA/AHCI. 
Different physical and link layers -- the verbs stayed the same. What I 
am getting at is that I am not sure what an "interface" is /supposed/ to 
encompass here in QEMU. Underlying device type? If that's the case, then 
IDE/AHCI are definitely identical.

>> I think the real ICH9 AHCI device supports only fully AHCI or fully
>> legacy, but the AHCI spec itself appears to allow you to run a
>> mixed-mode device.
>>
>> I am not sure we have a usage case for mixed-mode, so enforcing
>> either/or for the AHCI device makes sense for now, I think.
>
> I can't see a use for mixed mode, either.
>
>>> The other point of view is that IDE and AHCI are no more different than
>>> the different kinds of SCSI HBAs.  This is certainly true from a qdev
>>> point of view: just like SCSI devices can connect to any SCSI qbus,
>>> regardless of the HBA providing it, so can IDE devices connect to any
>>> IDE qbus, regardless of the controller providing it.
>>
>> Yes... Really the only difference are some mapping semantics. I don't
>> think there's any /other/ reason I needed IF_AHCI, of course, I wasn't
>> around for the previous discussions, so maybe there are other reasons
>> I am not aware of.
>
> I've always been in the "we don't need or want if=ahci" camp :)
>
> The one argument for if=ahci I found convincing was a desire for Q35
> with its ICH9 in legacy mode.  And that's just as easily done with a
> machine option.  Personally, I find that more natural.

So piix would always try to add to the PCI IDE bus, and Q35 would always 
try to add to the AHCI bus.

With a machine option for Q35, we could tell it to use the AHCI device 
in legacy mode and add to that device accordingly.

Do we care about the case for adding an AHCI device to piix? Do we 
change the behavior of what bus we use by a machine option (like a 
default_hba machine property and hba_type drive properties?), or by the 
presence of an AHCI device if someone adds one?

As it stands now, using if=ahci or if=ide is a pretty strong hint for 
what bus to look for and add to; though it is inflexible between command 
invocations that use different HBAs.

>>> There's a wrinkle: the mapping between index to (bus, unit).  This
>>> mapping is ABI.  The current mapping makes sense for the first
>>> generation of controllers: PATA (two devices per bus, thus
>>> if_max_devs[IF_IDE] = 2), and 8-bit SCSI (seven per bus, thus
>>> if_max_devs[IF_SCSI = 7).
>>>
>>> The mapping is silly for newer SCSI HBAs.  Commit 622b520f tried to make
>>> it less silly, but had to be reverted in 27d6bf4 because the silliness
>>> was ABI.
>>>
>>> The mapping is also silly for ich9-ahci.  You side-step that silliness
>>> only, by adding a new interface type for it.  But shouldn't we add a
>>> number of SCSI interface types then, too?  Where does that end?
>>>
>>> Can we do better?  I think we can, by making this part of the ABI
>>> board-specific.  The general form of the mapping remains
>>>
>>>       (bus, unit) = (index / N, index % N)
>>>
>>> but N now depends on board and interface type, not just the latter.
>>>
>>> If the board connects if=scsi to an lsi53c895a, then N = 7.
>>>
>>> If the board connects if=ide to an piix3-ide, then N = 2.
>>>
>>> If the board connects if=ide to an ich9-ahci, then N = 1.
>>>
>>> I trust you get the idea :)
>>
>> I suppose we could make it something like:
>> if (HBA.max_units > 0) {
>>    N := min(HBA.max_units, IF.max_units);
>> } else {
>>    N := IF.max_units;
>> }
>> (bus, unit) := (index / N, index % N);
>>
>> Which sets a default property for the interface but allows the device
>> (not the board) to override. Does that make more sense? If we allow
>> people to wire up an AHCI device to piix, we'll run back into the same
>> problems of the bus/unit mappings unless we make this a device
>> property.
>
> Yes, it is a property of the device (property not in the qdev sense).
>
> Why would HBA.max_units ever be greater than IF.max_units?
>
> If the answer is "only if somebody screwed up the HBA device model",
> then the above can be simplified to just N = HBA.max_units.

Oh, yeah. I did Web UI programming for a while during college. Not 
trusting plugin values is a side effect! I might pepper in an assertion 
to make it clear that you can't bump the number of units /up/, though. 
That's a smarter thing to do.

>> I do feel like I'd rather just make it an interface property and have
>> people specify which type of bus they want to wire it up to, but that
>> does create a lot of disparity against the SCSI devices.
>
> What do you mean by "interface property"?

A logical property of the interface specification; what QEMU and this 
patch does now.

>>> [*] Currently, they're silently ignored with most boards for most X, but
>>> I regard that as implementation defect.
>>
>> Yes. Is there a bool in the drive info array that we can set to say
>> "this drive has been added as a device" and check for any that went
>> unset?
>
> Not yet :)
>
>>         I can add one and a routine to check for it, which may help
>> flush out more of the weird legacy sugar option bugs.
>
> We do something like that for -netdev:
>
>      $ qemu -nodefaults -display none -netdev user,id=foo
>      Warning: netdev foo has no peer
>
> and -net:
>
>      $ qemu -nodefaults -display none -net user,id=foo
>      Warning: vlan 0 with no nics
>
> I think as long as we leave picking up configuration to boards, having
> the boards mark the pieces they pick up is the best we can do.
>
> An alternative to leaving it to boards is making the boads define
> callbacks that get fed configuration.  But that's more surgery.
>
> There's more than just -netdev, -net and -drive, though.  Many command
> line options to configure devices also merely create a piece of
> configuration for boards to pick up:
>
> * Character devices (-serial, -parallel) end up in serial_hds[],
>    parallel_hds[].
>
> * Graphics devices (-vga) end up in vga_interface_type.
>
> These all need the same "did the board pick it up?" check as -drive.
>
> Some old options have been converted to work independent of boards:
>
> * Virtio consoles (-virtioconsole) in commit 98b1925.
>
> * Sound devices (-soundhw) in commit b3e6d59.
>
> Newer convenience options should always worked this way.  -watchdog
> does.
>
> I may have missed options.

That is indeed more than a few. I probably need a little bit more 
exposure to different boards and how configuration works before I attack 
the problem as a whole. I may just fix -drive for now ...

>
>> (Sugar attracts bugs. heh-heh-heh...)
>
> Indeed!
>

Your position is pretty clear. I will give other people the time to 
chime in before I do too much more work on this, though I will probably 
go ahead and add the unused drive check now, since we'll want that no 
matter which path we take.

-- 
—js

Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 08/19/2014 02:08 PM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 08/19/2014 04:05 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Currently, the drive definitions created by drive_new() when using
>>>>> the -drive file=...[,if=ide] or -cdrom or -hd[abcd] options are not
>>>>> picked up by the Q35 initialization routine.
>>>>>
>>>>> To fix this, we have to add hooks to search for these drives using
>>>>> something like pc_piix's ide_drive_get and then add them using
>>>>> something like pci_ide_create_devs.
>>>>
>>>> ide_drive_get() isn't pc_piix's, it's a helper function in the IDE core
>>>> which boards (not just pc_piix) use to find the if=ide drives.  It fills
>>>> in an array of DriveInfo.
>>>>
>>>> pci_ide_create_devs() is a helper function in the IDE PCI code which PCI
>>>> IDE controllers (not just piix3-ide) use to create IDE devices for an
>>>> array of DriveInfo.
>>>
>>> Yes. I meant to say pc_piix's /call to/ ide_drive_get. I would have to
>>> patch up the other boards if I changed this function! Only an RFC
>>> before I got too far down this path :]
>>>
>>>>> Where it gets slightly wonky is the fact that if=ide is specified
>>>>> to use two devices per bus, whereas AHCI does not utilize that
>>>>> same master/slave mechanic. Therefore, many places inside of
>>>>> blockdev.c where we add and define new drives use incorrect math
>>>>> for AHCI devices and try to place them on impossible buses.
>>>>> Notably -hdb and -hdd would become inaccessible.
>>>>
>>>> Yes.
>>>>
>>>>> To remedy this, I added a new interface type, IF_AHCI. Corresponding
>>>>> to this change, I modified the default machine properties for Q35
>>>>> to use this interface as a default.
>>>>>
>>>>> The changes appear to work well, but where I'd like some feedback
>>>>> is what should happen if people do something like:
>>>>>
>>>>> qemu -M q35 -drive if=ide,file=fedora.qcow2
>>>>>
>>>>> The code as presented here is not going to look for or attempt to
>>>>> connect IDE devices, because it is now looking for /AHCI/ devices.
>>>>>
>>>>> At worst, this may break a few existing scripts, but I actually think
>>>>> that since the if=ide,file=... shorthand never worked to begin with,
>>>>> the impact might actually be minimal.
>>>>>
>>>>> But since the legacy IDE interface of the ICH9 is as of yet unemulated,
>>>>> the if=ide drives don't have a reasonable place to go yet. I am also
>>>>> not sure what a reasonable way to handle people specifying BOTH
>>>>> if=ide and if=ahci drives would be.
>>>>
>>>> We've been through IF_AHCI before, more than once, but that was before
>>>> you got involved :)
>>>>
>>>> The problem at hand is that "-drive if=ide" and its sugared forms -hda,
>>>> -hdb, -cdrom, ... don't work with Q35.
>>>>
>>>> You provide a solution for the sugared forms, you leave the problem
>>>> unsolved for the unsugared form, and you add a new problem: -drive
>>>> if=ahci doesn't work with i440FX.  Progress, sort of.
>>>
>>> Adding a call to boards that support the AHCI device during their
>>> initialization should be easy enough, if we decide that "ide means
>>> ISA/PCI, ahci means the AHCI HBA." I could probably even write one
>>> generic routine between i440fx and q35 to do both IDE/AHCI.
>>>
>>> If we decide that IF_IDE and IF_AHCI mean different things, the
>>> problem of the unsugared form being unsolved depends on me (well, or
>>> someone) implementing the legacy IDE interface for Q35.
>>
>> Let me come back to this further down.

I didn't.  This time I will.

>>>> Let's take a step back, and recap previous discussion.  There are two
>>>> defensible point of views, in my opinion.
>>>>
>>>> One is that IDE and AHCI should be separate interface types, just like
>>>> IDE and SCSI are.
>>>>
>>>> Attempts to define an if=X drive with a board that doesn't provide a
>>>> controller for X should fail[*].  Only onboard controllers matter,
>>>> add-ons plugged in with -device don't.  An i440FX board provides only
>>>> IDE.  A Q35 board provides only AHCI, not IDE.  If we implement an
>>>> ich9-ahci legacy mode, and switch it on, then it provides only IDE, not
>>>> AHCI.  Or maybe both, depending on how we do it.
>>>
>>> I think I am leaning towards this viewpoint, but it depends on what
>>> "interface" means in QEMU. Currently, the number of units per bus is
>>> tied to the "interface" and clearly the AHCI SATA interface only
>>> supports one per bus, so semantically this makes sense.
>>
>> An index <-> (bus, unit) mapping doesn't make an interface!  Yes, it's
>> tightly coupled to the interface, but that became wrong way back when we
>> went beyond 8-bit SCSI HBAs, long before we added up AHCI HBAs.
>
> "Interface" seems nebulous in QEMU. In the physical world, there
> definitely is a difference between IDE/EIDE/PATA and
> SATA/AHCI. Different physical and link layers -- the verbs stayed the
> same. What I am getting at is that I am not sure what an "interface"
> is /supposed/ to encompass here in QEMU. Underlying device type? If
> that's the case, then IDE/AHCI are definitely identical.

"Interface" in the sense of BlockInterfaceType isn't exactly a product
of careful design, weighing considerations from the physical world
against practical needs of the virtual world.  It's more like "for every
board, list ways to plug in block devices, then look at these lists out
of the corner of your eye to come up with an enumeration type that'll do
for all boards."

Its not a big user interface deal anyway.  It's just for -drive, and
-drive is getting less and less prominent.  If you want sugar, there's
-hda & friends.  If you want power, there's -device with -drive if=none
(soon to be superseded by -blockdev).

>From a purely pragmatic point of view, I can accept a new
BlockInterfaceType member when a board has a way to plug in block
devices we want to offer with -drive that doesn't fit existing members.
I don't think that's the case for AHCI.

>From a post hoc design point of view, the SCSI precedence of using the
same IF_SCSI for all the different SCSI HBAs makes me want to use the
same IF_IDE for all the different ATA HBAs.

>>> I think the real ICH9 AHCI device supports only fully AHCI or fully
>>> legacy, but the AHCI spec itself appears to allow you to run a
>>> mixed-mode device.
>>>
>>> I am not sure we have a usage case for mixed-mode, so enforcing
>>> either/or for the AHCI device makes sense for now, I think.
>>
>> I can't see a use for mixed mode, either.
>>
>>>> The other point of view is that IDE and AHCI are no more different than
>>>> the different kinds of SCSI HBAs.  This is certainly true from a qdev
>>>> point of view: just like SCSI devices can connect to any SCSI qbus,
>>>> regardless of the HBA providing it, so can IDE devices connect to any
>>>> IDE qbus, regardless of the controller providing it.
>>>
>>> Yes... Really the only difference are some mapping semantics. I don't
>>> think there's any /other/ reason I needed IF_AHCI, of course, I wasn't
>>> around for the previous discussions, so maybe there are other reasons
>>> I am not aware of.
>>
>> I've always been in the "we don't need or want if=ahci" camp :)
>>
>> The one argument for if=ahci I found convincing was a desire for Q35
>> with its ICH9 in legacy mode.  And that's just as easily done with a
>> machine option.  Personally, I find that more natural.
>
> So piix would always try to add to the PCI IDE bus, and Q35 would
> always try to add to the AHCI bus.

Yes.

> With a machine option for Q35, we could tell it to use the AHCI device
> in legacy mode and add to that device accordingly.

Yes.

> Do we care about the case for adding an AHCI device to piix? Do we
> change the behavior of what bus we use by a machine option (like a
> default_hba machine property and hba_type drive properties?), or by
> the presence of an AHCI device if someone adds one?

No.

-drive plugs into onboard devices.  If you want additional devices, use
-device.

There's one exception: pc machines automatically add N+1 lsi53c895a
HBAs, where N is the largest bus number used with -drive if=scsi,
explicitly or implicitly.  This HBA has become a pretty bad choice, and
it's getting worse.  I'd like us to kill the "create SCSI HBAs" feature
for new machine types.

> As it stands now, using if=ahci or if=ide is a pretty strong hint for
> what bus to look for and add to; though it is inflexible between
> command invocations that use different HBAs.

Yes, but

1. there is no board sporting both IDE and AHCI HBA at the same time
(like what we called "mixed mode" above), and

2. if there was, we still could route -drive to HBAs according to bus,
exactly like we do now with multiple homogeneous HBAs, albeit with a
more complex index <-> (bus, unit) mapping.

Besides, let me reiterate: why only IDE vs. AHCI?  Why not lsi53c895a
vs. virtio-scsi vs. megasas?  Where would that end?

>>>> There's a wrinkle: the mapping between index to (bus, unit).  This
>>>> mapping is ABI.  The current mapping makes sense for the first
>>>> generation of controllers: PATA (two devices per bus, thus
>>>> if_max_devs[IF_IDE] = 2), and 8-bit SCSI (seven per bus, thus
>>>> if_max_devs[IF_SCSI = 7).
>>>>
>>>> The mapping is silly for newer SCSI HBAs.  Commit 622b520f tried to make
>>>> it less silly, but had to be reverted in 27d6bf4 because the silliness
>>>> was ABI.
>>>>
>>>> The mapping is also silly for ich9-ahci.  You side-step that silliness
>>>> only, by adding a new interface type for it.  But shouldn't we add a
>>>> number of SCSI interface types then, too?  Where does that end?
>>>>
>>>> Can we do better?  I think we can, by making this part of the ABI
>>>> board-specific.  The general form of the mapping remains
>>>>
>>>>       (bus, unit) = (index / N, index % N)
>>>>
>>>> but N now depends on board and interface type, not just the latter.
>>>>
>>>> If the board connects if=scsi to an lsi53c895a, then N = 7.
>>>>
>>>> If the board connects if=ide to an piix3-ide, then N = 2.
>>>>
>>>> If the board connects if=ide to an ich9-ahci, then N = 1.
>>>>
>>>> I trust you get the idea :)
>>>
>>> I suppose we could make it something like:
>>> if (HBA.max_units > 0) {
>>>    N := min(HBA.max_units, IF.max_units);
>>> } else {
>>>    N := IF.max_units;
>>> }
>>> (bus, unit) := (index / N, index % N);
>>>
>>> Which sets a default property for the interface but allows the device
>>> (not the board) to override. Does that make more sense? If we allow
>>> people to wire up an AHCI device to piix, we'll run back into the same
>>> problems of the bus/unit mappings unless we make this a device
>>> property.
>>
>> Yes, it is a property of the device (property not in the qdev sense).
>>
>> Why would HBA.max_units ever be greater than IF.max_units?
>>
>> If the answer is "only if somebody screwed up the HBA device model",
>> then the above can be simplified to just N = HBA.max_units.
>
> Oh, yeah. I did Web UI programming for a while during college. Not
> trusting plugin values is a side effect! I might pepper in an
> assertion to make it clear that you can't bump the number of units
> /up/, though. That's a smarter thing to do.

If we have a bus-specific limit, then asserting the device's limit is in
range makes lots of sense :)

>>> I do feel like I'd rather just make it an interface property and have
>>> people specify which type of bus they want to wire it up to, but that
>>> does create a lot of disparity against the SCSI devices.
>>
>> What do you mean by "interface property"?
>
> A logical property of the interface specification; what QEMU and this
> patch does now.

I guess we can discuss that over patches.

I promised to talk about desugaring.  I feel right after discussing
index <-> (bus, unit) mapping is a good place.

There are three desugaring steps: 1. -hda, ... -> -drive, 2. -drive ->
DriveInfo + BlockDriverState, 3. DriveInfo -> device model.

1. -hda, ... -> -drive

   Straightforward macro expansion, done by main() with help from
   drive_add().

   Example: -hda FNAME -> -drive index=0,file=FNAME,media=disk
   Note: no if=..., this means "use board default".

   Example: -sd FNAME -> -drive if=sd,file=FNAME
   Note: no index=..., this means "use next available".

2. -drive -> DriveInfo + BlockDriverState

   Done by drive_new().  Takes care of supplying defaults, such as
   board's default interface type, mapping index to (bus, unit), picking
   next available bus unit, ...

3. DriveInfo -> device model

   Done by board code, separately for each interface type the board
   recognizes.  Picks a device model, where to plug and how to configure
   it, all according to DriveInfo,

Making the index <-> (bus, unit) mapping depend on the board hopefully
affects just step 2: have boards export suitable parameters for the
mapping, just like they export their default interface type.

Obvious idea: move if_name[] into struct QEMUMachine.  You may want to
use a pointer instead of an array there, so you can avoid duplication.
Maybe have NULL mean the traditional value of if_name[]; then only new
boards have to bother setting up the pointer.

>>>> [*] Currently, they're silently ignored with most boards for most X, but
>>>> I regard that as implementation defect.
>>>
>>> Yes. Is there a bool in the drive info array that we can set to say
>>> "this drive has been added as a device" and check for any that went
>>> unset?
>>
>> Not yet :)
>>
>>>         I can add one and a routine to check for it, which may help
>>> flush out more of the weird legacy sugar option bugs.
>>
>> We do something like that for -netdev:
>>
>>      $ qemu -nodefaults -display none -netdev user,id=foo
>>      Warning: netdev foo has no peer
>>
>> and -net:
>>
>>      $ qemu -nodefaults -display none -net user,id=foo
>>      Warning: vlan 0 with no nics
>>
>> I think as long as we leave picking up configuration to boards, having
>> the boards mark the pieces they pick up is the best we can do.
>>
>> An alternative to leaving it to boards is making the boads define
>> callbacks that get fed configuration.  But that's more surgery.
>>
>> There's more than just -netdev, -net and -drive, though.  Many command
>> line options to configure devices also merely create a piece of
>> configuration for boards to pick up:
>>
>> * Character devices (-serial, -parallel) end up in serial_hds[],
>>    parallel_hds[].
>>
>> * Graphics devices (-vga) end up in vga_interface_type.
>>
>> These all need the same "did the board pick it up?" check as -drive.
>>
>> Some old options have been converted to work independent of boards:
>>
>> * Virtio consoles (-virtioconsole) in commit 98b1925.
>>
>> * Sound devices (-soundhw) in commit b3e6d59.
>>
>> Newer convenience options should always worked this way.  -watchdog
>> does.
>>
>> I may have missed options.
>
> That is indeed more than a few. I probably need a little bit more
> exposure to different boards and how configuration works before I
> attack the problem as a whole. I may just fix -drive for now ...

I'm not in the habit of rejecting incremental progress we can have now
in favor of a perfect solution we can't have now :)

>>> (Sugar attracts bugs. heh-heh-heh...)
>>
>> Indeed!
>>
>
> Your position is pretty clear. I will give other people the time to
> chime in before I do too much more work on this, though I will
> probably go ahead and add the unused drive check now, since we'll want
> that no matter which path we take.

Makes sense.  Have fun!