Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> We've had support for creating AHCI devices using -device for a while now,
> but it's cumbersome to users. We really should provide an easier way for
> them to leverage the power of AHCI!
>
> So let's introduce a new if= option to -drive, giving users the same
> command line experience as for scsi or ide.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> v1 -> v2:
>
>   - support more than a single drive per adapter
>   - support index= option
>   - treat IF_AHCI the same as IF_IDE

Inhowfar?  Not obvious to me from the patch, or the diff patch v1.

>   - add is_ata() helper to match AHCI || IDE

Not addressed:

Once we switch to q35, if=ahci will become a redundant wart: to add
drives to the board's AHCI controller, you'll have to use if=ide.
if=ahci will create new controllers, which is generally not what you
want.  Ugh.


> ---
>  blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  blockdev.h      |    7 +++++++
>  qemu-options.hx |    7 ++++++-
>  vl.c            |    2 ++
>  4 files changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9e0a72a..744a886 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -32,6 +32,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] = {
> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>       */
>      [IF_IDE] = 2,
>      [IF_SCSI] = 7,
> +    [IF_AHCI] = 6,
>  };
>  
>  /*
> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
       if ((buf = qemu_opt_get(opts, "if")) != NULL) {
           for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
               ;
           if (type == IF_COUNT) {
               error_report("unsupported bus type '%s'", buf);
               return NULL;
           }
       } else {
           type = default_to_scsi ? IF_SCSI : IF_IDE;
       }

A board can't default to IF_AHCI.  I guess what such a board would do is
treat IF_IDE and IF_AHCI just the same.

Leads me this question: what do "if=ide", "if=ahci" and "no if given"
mean?  Let me try:

* "if=ide" means "if the board provides an IDE controller, create an IDE
  device attached to it.  What kind of IDE controller the board provides
  doesn't matter.  In particular, an AHCI controller is fine.

* "no if given" means "create a block device of the board's preferred
  kind" in theory, and "default to either if=ide or if=scsi" in current
  practice.

* "if=ahci" means "create an IDE device and attach it to a completely
  seperate set of ich9-ahci controllers specifically created for the
  "-drive if=ahci".  If the board provides an AHCI controller, it's not
  used for if=ahci.  It may still be used for if=ide (depends on board).

  Isn't this an embarrassment?

>      max_devs = if_max_devs[type];
>  
>      if (cyls || heads || secs) {
> -        if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
> +        if (cyls < 1 || (is_ata(type) && cyls > 16383)) {
>              error_report("invalid physical cyls number");
>  	    return NULL;
>  	}
> -        if (heads < 1 || (type == IF_IDE && heads > 16)) {
> +        if (heads < 1 || (is_ata(type) && heads > 16)) {
>              error_report("invalid physical heads number");
>  	    return NULL;
>  	}
> -        if (secs < 1 || (type == IF_IDE && secs > 63)) {
> +        if (secs < 1 || (is_ata(type) && secs > 63)) {
>              error_report("invalid physical secs number");
>  	    return NULL;
>  	}

Trivial conflict with my "blockdev: Drop redundant CHS validation for
if=ide".  Don't worry about it.

A few more instances of IF_IDE:

       on_write_error = BLOCK_ERR_STOP_ENOSPC;
       if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
           if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
               error_report("werror is not supported by this bus type");
               return NULL;
           }

           on_write_error = parse_block_error_action(buf, 0);
           if (on_write_error < 0) {
               return NULL;
           }
       }

       on_read_error = BLOCK_ERR_REPORT;
       if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
           if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
               error_report("rerror is not supported by this bus type");
               return NULL;
           }

           on_read_error = parse_block_error_action(buf, 1);
           if (on_read_error < 0) {
               return NULL;
           }
       }

Are you sure you don't want to check for IF_AHCI?

> @@ -516,7 +518,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      } else {
>          /* no id supplied -> create one */
>          dinfo->id = g_malloc0(32);
> -        if (type == IF_IDE || type == IF_SCSI)
> +        if (is_ata(type) || type == IF_SCSI)
>              mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>          if (max_devs)
>              snprintf(dinfo->id, 32, "%s%i%s%i",
> @@ -546,6 +548,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>      case IF_IDE:
>      case IF_SCSI:
>      case IF_XEN:
> +    case IF_AHCI:
>      case IF_NONE:
>          switch(media) {
>  	case MEDIA_DISK:
> @@ -628,6 +631,49 @@ err:
>      return NULL;
>  }
>  
> +static void drive_populate_ahci(void)
> +{
> +    int bus;
> +    QemuOpts *opts;
> +
> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
> +        char devname[] = "ahciXXX";
> +        int dev;
> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
> +
> +        /* add ahci host controller */
> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
> +        qemu_opt_set(opts, "driver", "ich9-ahci");

Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
ahci_init() correctly, it provides six.

Creates it even if the bus is unused.

While better than v1, which created one per -drive, it still wastes
precious PCI slots, doesn't it?

Hardcodes the AHCI controller device to "ich9-ahci".  Just we do for
IF_SCSI, only in generic code instead of board-specific code.  No
problem, we can always add another IF_ ;)  Just kidding; we'd add a way
for the board to define the preferred controller device.  SCSI could use
that, too.

> +        for (dev = 0; dev < if_max_devs[IF_AHCI]; dev++) {
> +            DriveInfo *dinfo = drive_get(IF_AHCI, bus, dev);
> +            char busname[] = "ahciXXX.XXX";
> +
> +            if (!dinfo) {
> +                continue;
> +            }
> +            snprintf(busname, sizeof(busname), "ahci%d.%d", bus, dev);
> +
> +            /* attach this ata disk to its bus */
> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
> +            qemu_opt_set(opts, "driver", "ide-drive");
> +            qemu_opt_set(opts, "bus", busname);
> +            qemu_opt_set(opts, "drive", dinfo->id);
> +        }
> +    }
> +}
> +
> +/*
> + * This function creates -device options out of IF_xxx elements,
> + * so that we don't have to mess with user friendly syntax parsing
> + * in device emulation code.
> + *
> + * For now, only AHCI is implemented here.
> + */
> +void drive_populate(void)
> +{
> +    drive_populate_ahci();
> +}
> +
>  void do_commit(Monitor *mon, const QDict *qdict)
>  {
>      const char *device = qdict_get_str(qdict, "device");
> diff --git a/blockdev.h b/blockdev.h
> index 260e16b..9d79558 100644
> --- a/blockdev.h
> +++ b/blockdev.h
> @@ -23,6 +23,7 @@ typedef enum {
>      IF_DEFAULT = -1,            /* for use with drive_add() only */
>      IF_NONE,
>      IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_AHCI,
>      IF_COUNT
>  } BlockInterfaceType;
>  
> @@ -53,6 +54,12 @@ QemuOpts *drive_def(const char *optstr);
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
>  DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
> +void drive_populate(void);
> +
> +static inline bool is_ata(int type)
> +{
> +    return (type == IF_IDE) || (type == IF_AHCI);
> +}
>  
>  /* device-hotplug */
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8b66264..9527c51 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -160,7 +160,7 @@ Special files such as iSCSI devices can be specified using protocol
>  specific URLs. See the section for "Device URL Syntax" for more information.
>  @item if=@var{interface}
>  This option defines on which type on interface the drive is connected.
> -Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
> +Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio, ahci.
>  @item bus=@var{bus},unit=@var{unit}
>  These options define where is connected the drive by defining the bus number and
>  the unit id.
> @@ -260,6 +260,11 @@ You can connect a SCSI disk with unit ID 6 on the bus #0:
>  qemu-system-i386 -drive file=file,if=scsi,bus=0,unit=6
>  @end example
>  
> +You can attach a SATA disk using AHCI:
> +@example
> +qemu-system-i386 -drive file=file,if=ahci
> +@end example
> +

I still think the automatic controller creation should be documented.
*Especially* since it does that even when the board provides a perfectly
usable AHCI controller already.

>  Instead of @option{-fda}, @option{-fdb}, you can use:
>  @example
>  qemu-system-i386 -drive file=file,index=0,if=floppy
> diff --git a/vl.c b/vl.c
> index 1329c30..65410a0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3438,6 +3438,8 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_sdcard, snapshot, machine->use_scsi,
>                    IF_SD, 0, SD_OPTS);
>  
> +    drive_populate();
> +
>      register_savevm_live(NULL, "ram", 0, 4, NULL, ram_save_live, NULL,
>                           ram_load, NULL);

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Kevin Wolf]

Am 09.07.2012 10:50, schrieb Markus Armbruster:
> Alexander Graf <agraf@suse.de> writes:
> 
>> We've had support for creating AHCI devices using -device for a while now,
>> but it's cumbersome to users. We really should provide an easier way for
>> them to leverage the power of AHCI!
>>
>> So let's introduce a new if= option to -drive, giving users the same
>> command line experience as for scsi or ide.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v1 -> v2:
>>
>>   - support more than a single drive per adapter
>>   - support index= option
>>   - treat IF_AHCI the same as IF_IDE
> 
> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
> 
>>   - add is_ata() helper to match AHCI || IDE
> 
> Not addressed:
> 
> Once we switch to q35, if=ahci will become a redundant wart: to add
> drives to the board's AHCI controller, you'll have to use if=ide.
> if=ahci will create new controllers, which is generally not what you
> want.  Ugh.

Can we even make it the default with q35 as long as our AHCI controller
doesn't also expose a legacy interface for compatibility?

Kevin

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 11:11, Kevin Wolf wrote:

> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> We've had support for creating AHCI devices using -device for a while now,
>>> but it's cumbersome to users. We really should provide an easier way for
>>> them to leverage the power of AHCI!
>>> 
>>> So let's introduce a new if= option to -drive, giving users the same
>>> command line experience as for scsi or ide.
>>> 
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> 
>>> ---
>>> 
>>> v1 -> v2:
>>> 
>>>  - support more than a single drive per adapter
>>>  - support index= option
>>>  - treat IF_AHCI the same as IF_IDE
>> 
>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>> 
>>>  - add is_ata() helper to match AHCI || IDE
>> 
>> Not addressed:
>> 
>> Once we switch to q35, if=ahci will become a redundant wart: to add
>> drives to the board's AHCI controller, you'll have to use if=ide.
>> if=ahci will create new controllers, which is generally not what you
>> want.  Ugh.
> 
> Can we even make it the default with q35 as long as our AHCI controller
> doesn't also expose a legacy interface for compatibility?

What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Kevin Wolf]

Am 09.07.2012 11:13, schrieb Alexander Graf:
> 
> On 09.07.2012, at 11:11, Kevin Wolf wrote:
> 
>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>> Alexander Graf <agraf@suse.de> writes:
>>>
>>>> We've had support for creating AHCI devices using -device for a while now,
>>>> but it's cumbersome to users. We really should provide an easier way for
>>>> them to leverage the power of AHCI!
>>>>
>>>> So let's introduce a new if= option to -drive, giving users the same
>>>> command line experience as for scsi or ide.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>
>>>>  - support more than a single drive per adapter
>>>>  - support index= option
>>>>  - treat IF_AHCI the same as IF_IDE
>>>
>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>
>>>>  - add is_ata() helper to match AHCI || IDE
>>>
>>> Not addressed:
>>>
>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>> if=ahci will create new controllers, which is generally not what you
>>> want.  Ugh.
>>
>> Can we even make it the default with q35 as long as our AHCI controller
>> doesn't also expose a legacy interface for compatibility?
> 
> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?

I didn't actually look into this much. I just supposed that the
existence of an AHCI Enable bit in the GHC register implies that some
compatibility mechanism can be implemented that is transparent to older
OSes.

If it can't, then I guess we can't change the meaning of -hda as long as
we claim that the command line is a stable API (or at least not if q35
is meant to become the default machine type at some point)

Kevin

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 11:27, Kevin Wolf wrote:

> Am 09.07.2012 11:13, schrieb Alexander Graf:
>> 
>> On 09.07.2012, at 11:11, Kevin Wolf wrote:
>> 
>>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>> them to leverage the power of AHCI!
>>>>> 
>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>> command line experience as for scsi or ide.
>>>>> 
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> 
>>>>> ---
>>>>> 
>>>>> v1 -> v2:
>>>>> 
>>>>> - support more than a single drive per adapter
>>>>> - support index= option
>>>>> - treat IF_AHCI the same as IF_IDE
>>>> 
>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>> 
>>>>> - add is_ata() helper to match AHCI || IDE
>>>> 
>>>> Not addressed:
>>>> 
>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>> if=ahci will create new controllers, which is generally not what you
>>>> want.  Ugh.
>>> 
>>> Can we even make it the default with q35 as long as our AHCI controller
>>> doesn't also expose a legacy interface for compatibility?
>> 
>> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?
> 
> I didn't actually look into this much. I just supposed that the
> existence of an AHCI Enable bit in the GHC register implies that some
> compatibility mechanism can be implemented that is transparent to older
> OSes.

Yeah, I was hoping the same too when I read the spec back then. Unfortunately, it's really a firmware configuration bit which doesn't help us at all.

I think there is a hack in some driver somewhere that on bootup checks if the AHCI bit is disabled and then forcefully enables it, so that the device during OS boot magically changes its ID and semantics. But I don't think we really want to rely on that. IIRC it never went upstream and I doubt Windows does it.

> If it can't, then I guess we can't change the meaning of -hda as long as
> we claim that the command line is a stable API (or at least not if q35
> is meant to become the default machine type at some point)

If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gleb Natapov]

On Mon, Jul 09, 2012 at 11:36:47AM +0200, Alexander Graf wrote:
> 
> On 09.07.2012, at 11:27, Kevin Wolf wrote:
> 
> > Am 09.07.2012 11:13, schrieb Alexander Graf:
> >> 
> >> On 09.07.2012, at 11:11, Kevin Wolf wrote:
> >> 
> >>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
> >>>> Alexander Graf <agraf@suse.de> writes:
> >>>> 
> >>>>> We've had support for creating AHCI devices using -device for a while now,
> >>>>> but it's cumbersome to users. We really should provide an easier way for
> >>>>> them to leverage the power of AHCI!
> >>>>> 
> >>>>> So let's introduce a new if= option to -drive, giving users the same
> >>>>> command line experience as for scsi or ide.
> >>>>> 
> >>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>> 
> >>>>> ---
> >>>>> 
> >>>>> v1 -> v2:
> >>>>> 
> >>>>> - support more than a single drive per adapter
> >>>>> - support index= option
> >>>>> - treat IF_AHCI the same as IF_IDE
> >>>> 
> >>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
> >>>> 
> >>>>> - add is_ata() helper to match AHCI || IDE
> >>>> 
> >>>> Not addressed:
> >>>> 
> >>>> Once we switch to q35, if=ahci will become a redundant wart: to add
> >>>> drives to the board's AHCI controller, you'll have to use if=ide.
> >>>> if=ahci will create new controllers, which is generally not what you
> >>>> want.  Ugh.
> >>> 
> >>> Can we even make it the default with q35 as long as our AHCI controller
> >>> doesn't also expose a legacy interface for compatibility?
> >> 
> >> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?
> > 
> > I didn't actually look into this much. I just supposed that the
> > existence of an AHCI Enable bit in the GHC register implies that some
> > compatibility mechanism can be implemented that is transparent to older
> > OSes.
> 
> Yeah, I was hoping the same too when I read the spec back then. Unfortunately, it's really a firmware configuration bit which doesn't help us at all.
> 
> I think there is a hack in some driver somewhere that on bootup checks if the AHCI bit is disabled and then forcefully enables it, so that the device during OS boot magically changes its ID and semantics. But I don't think we really want to rely on that. IIRC it never went upstream and I doubt Windows does it.
> 
Doesn't enabling the bit change PCI bars size/number?

> > If it can't, then I guess we can't change the meaning of -hda as long as
> > we claim that the command line is a stable API (or at least not if q35
> > is meant to become the default machine type at some point)
> 
> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
> 
> 
> Alex
> 

--
			Gleb.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 11:41, Gleb Natapov wrote:

> On Mon, Jul 09, 2012 at 11:36:47AM +0200, Alexander Graf wrote:
>> 
>> On 09.07.2012, at 11:27, Kevin Wolf wrote:
>> 
>>> Am 09.07.2012 11:13, schrieb Alexander Graf:
>>>> 
>>>> On 09.07.2012, at 11:11, Kevin Wolf wrote:
>>>> 
>>>>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>>> 
>>>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>>>> them to leverage the power of AHCI!
>>>>>>> 
>>>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>>>> command line experience as for scsi or ide.
>>>>>>> 
>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>> 
>>>>>>> ---
>>>>>>> 
>>>>>>> v1 -> v2:
>>>>>>> 
>>>>>>> - support more than a single drive per adapter
>>>>>>> - support index= option
>>>>>>> - treat IF_AHCI the same as IF_IDE
>>>>>> 
>>>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>>>> 
>>>>>>> - add is_ata() helper to match AHCI || IDE
>>>>>> 
>>>>>> Not addressed:
>>>>>> 
>>>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>>>> if=ahci will create new controllers, which is generally not what you
>>>>>> want.  Ugh.
>>>>> 
>>>>> Can we even make it the default with q35 as long as our AHCI controller
>>>>> doesn't also expose a legacy interface for compatibility?
>>>> 
>>>> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?
>>> 
>>> I didn't actually look into this much. I just supposed that the
>>> existence of an AHCI Enable bit in the GHC register implies that some
>>> compatibility mechanism can be implemented that is transparent to older
>>> OSes.
>> 
>> Yeah, I was hoping the same too when I read the spec back then. Unfortunately, it's really a firmware configuration bit which doesn't help us at all.
>> 
>> I think there is a hack in some driver somewhere that on bootup checks if the AHCI bit is disabled and then forcefully enables it, so that the device during OS boot magically changes its ID and semantics. But I don't think we really want to rely on that. IIRC it never went upstream and I doubt Windows does it.
>> 
> Doesn't enabling the bit change PCI bars size/number?

Don't remember, but I think so. It makes it a completely different device.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gleb Natapov]

On Mon, Jul 09, 2012 at 11:44:11AM +0200, Alexander Graf wrote:
> 
> On 09.07.2012, at 11:41, Gleb Natapov wrote:
> 
> > On Mon, Jul 09, 2012 at 11:36:47AM +0200, Alexander Graf wrote:
> >> 
> >> On 09.07.2012, at 11:27, Kevin Wolf wrote:
> >> 
> >>> Am 09.07.2012 11:13, schrieb Alexander Graf:
> >>>> 
> >>>> On 09.07.2012, at 11:11, Kevin Wolf wrote:
> >>>> 
> >>>>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
> >>>>>> Alexander Graf <agraf@suse.de> writes:
> >>>>>> 
> >>>>>>> We've had support for creating AHCI devices using -device for a while now,
> >>>>>>> but it's cumbersome to users. We really should provide an easier way for
> >>>>>>> them to leverage the power of AHCI!
> >>>>>>> 
> >>>>>>> So let's introduce a new if= option to -drive, giving users the same
> >>>>>>> command line experience as for scsi or ide.
> >>>>>>> 
> >>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>>>>>> 
> >>>>>>> ---
> >>>>>>> 
> >>>>>>> v1 -> v2:
> >>>>>>> 
> >>>>>>> - support more than a single drive per adapter
> >>>>>>> - support index= option
> >>>>>>> - treat IF_AHCI the same as IF_IDE
> >>>>>> 
> >>>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
> >>>>>> 
> >>>>>>> - add is_ata() helper to match AHCI || IDE
> >>>>>> 
> >>>>>> Not addressed:
> >>>>>> 
> >>>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
> >>>>>> drives to the board's AHCI controller, you'll have to use if=ide.
> >>>>>> if=ahci will create new controllers, which is generally not what you
> >>>>>> want.  Ugh.
> >>>>> 
> >>>>> Can we even make it the default with q35 as long as our AHCI controller
> >>>>> doesn't also expose a legacy interface for compatibility?
> >>>> 
> >>>> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?
> >>> 
> >>> I didn't actually look into this much. I just supposed that the
> >>> existence of an AHCI Enable bit in the GHC register implies that some
> >>> compatibility mechanism can be implemented that is transparent to older
> >>> OSes.
> >> 
> >> Yeah, I was hoping the same too when I read the spec back then. Unfortunately, it's really a firmware configuration bit which doesn't help us at all.
> >> 
> >> I think there is a hack in some driver somewhere that on bootup checks if the AHCI bit is disabled and then forcefully enables it, so that the device during OS boot magically changes its ID and semantics. But I don't think we really want to rely on that. IIRC it never went upstream and I doubt Windows does it.
> >> 
> > Doesn't enabling the bit change PCI bars size/number?
> 
> Don't remember, but I think so. It makes it a completely different device.
> 
Yeah, so pci addr space should be reinitialized to. Definitely hacky.

--
			Gleb.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Kevin Wolf]

Am 09.07.2012 11:36, schrieb Alexander Graf:
> 
> On 09.07.2012, at 11:27, Kevin Wolf wrote:
> 
>> Am 09.07.2012 11:13, schrieb Alexander Graf:
>>>
>>> On 09.07.2012, at 11:11, Kevin Wolf wrote:
>>>> Can we even make it the default with q35 as long as our AHCI controller
>>>> doesn't also expose a legacy interface for compatibility?
>>>
>>> What legacy interface? The ICH-9 can be controlled by the _BIOS_ to switch between 2 PCI IDs. One for IDE mode, one for AHCI mode. I don't think that would help us here, would it?
>>
>> I didn't actually look into this much. I just supposed that the
>> existence of an AHCI Enable bit in the GHC register implies that some
>> compatibility mechanism can be implemented that is transparent to older
>> OSes.
> 
> Yeah, I was hoping the same too when I read the spec back then. Unfortunately, it's really a firmware configuration bit which doesn't help us at all.
> 
> I think there is a hack in some driver somewhere that on bootup checks if the AHCI bit is disabled and then forcefully enables it, so that the device during OS boot magically changes its ID and semantics. But I don't think we really want to rely on that. IIRC it never went upstream and I doubt Windows does it.

Meh. So once again hardware doesn't work as you would expect.

>> If it can't, then I guess we can't change the meaning of -hda as long as
>> we claim that the command line is a stable API (or at least not if q35
>> is meant to become the default machine type at some point)
> 
> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.

It doesn't per se, that is as long as you need to explicitly specify -M
q35. But then changing the default machine from the existing pc to q35
would break the command line.

Kevin

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gleb Natapov]

On Mon, Jul 09, 2012 at 11:52:32AM +0200, Kevin Wolf wrote:
> > If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
> 
> It doesn't per se, that is as long as you need to explicitly specify -M
> q35. But then changing the default machine from the existing pc to q35
> would break the command line.
> 
Would break it how?

--
			Gleb.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Kevin Wolf]

Am 09.07.2012 12:02, schrieb Gleb Natapov:
> On Mon, Jul 09, 2012 at 11:52:32AM +0200, Kevin Wolf wrote:
>>> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
>>
>> It doesn't per se, that is as long as you need to explicitly specify -M
>> q35. But then changing the default machine from the existing pc to q35
>> would break the command line.
>>
> Would break it how?

A guest that has an IDE driver, but not an AHCI one can be started with
'qemu -hda foo.img' today. After changing the default to a q35 machine
type which uses AHCI by default, this guest wouldn't boot any more with
the same command line.

Kevin

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gleb Natapov]

On Mon, Jul 09, 2012 at 12:03:49PM +0200, Kevin Wolf wrote:
> Am 09.07.2012 12:02, schrieb Gleb Natapov:
> > On Mon, Jul 09, 2012 at 11:52:32AM +0200, Kevin Wolf wrote:
> >>> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
> >>
> >> It doesn't per se, that is as long as you need to explicitly specify -M
> >> q35. But then changing the default machine from the existing pc to q35
> >> would break the command line.
> >>
> > Would break it how?
> 
> A guest that has an IDE driver, but not an AHCI one can be started with
> 'qemu -hda foo.img' today. After changing the default to a q35 machine
> type which uses AHCI by default, this guest wouldn't boot any more with
> the same command line.
> 
It is not suppose too. q35 is a totally different machine.

--
			Gleb.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gerd Hoffmann]

  Hi,

>> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
> 
> It doesn't per se, that is as long as you need to explicitly specify -M
> q35. But then changing the default machine from the existing pc to q35
> would break the command line.

That will happen _anyway_.  No doubt there will be configurations which
work with -M pc and break with -M q35.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>>> If -hda has the semantics of "create an IDE device", then no, we can't change it. It doesn't however. IIRC on -M pseries -hda creates SCSI devices. On s390 -hda creates virtio devices. So if on -M q35 -hda would create if=ahci, I don't see how that breaks the CLI.
>> 
>> It doesn't per se, that is as long as you need to explicitly specify -M
>> q35. But then changing the default machine from the existing pc to q35
>> would break the command line.
>
> That will happen _anyway_.  No doubt there will be configurations which
> work with -M pc and break with -M q35.

Yup.

Without -M, you get whatever the developer picked for you.

If we took backward compatibility dogmatically serious, the default
would still be pc-0.10.  Fortunately, we don't.  We pick something we
think makes sense for most users, in particular casual users unaware of
-M.  Whether and when that will become some q35 machine will be debated
when q35 works.

If you want the best possible compatibility, use -M FOO, where FOO is
whatever machine type you've been using (machine type, not alias,
i.e. -M pc-1.2, not -M pc).

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> We've had support for creating AHCI devices using -device for a while now,
>>> but it's cumbersome to users. We really should provide an easier way for
>>> them to leverage the power of AHCI!
>>>
>>> So let's introduce a new if= option to -drive, giving users the same
>>> command line experience as for scsi or ide.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>>   - support more than a single drive per adapter
>>>   - support index= option
>>>   - treat IF_AHCI the same as IF_IDE
>> 
>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>> 
>>>   - add is_ata() helper to match AHCI || IDE
>> 
>> Not addressed:
>> 
>> Once we switch to q35, if=ahci will become a redundant wart: to add
>> drives to the board's AHCI controller, you'll have to use if=ide.
>> if=ahci will create new controllers, which is generally not what you
>> want.  Ugh.
>
> Can we even make it the default with q35 as long as our AHCI controller
> doesn't also expose a legacy interface for compatibility?

Err, what else could we do with if=ide with q35?

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 13:06, Markus Armbruster wrote:

> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> We've had support for creating AHCI devices using -device for a while now,
>>>> but it's cumbersome to users. We really should provide an easier way for
>>>> them to leverage the power of AHCI!
>>>> 
>>>> So let's introduce a new if= option to -drive, giving users the same
>>>> command line experience as for scsi or ide.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> 
>>>> ---
>>>> 
>>>> v1 -> v2:
>>>> 
>>>>  - support more than a single drive per adapter
>>>>  - support index= option
>>>>  - treat IF_AHCI the same as IF_IDE
>>> 
>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>> 
>>>>  - add is_ata() helper to match AHCI || IDE
>>> 
>>> Not addressed:
>>> 
>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>> if=ahci will create new controllers, which is generally not what you
>>> want.  Ugh.
>> 
>> Can we even make it the default with q35 as long as our AHCI controller
>> doesn't also expose a legacy interface for compatibility?
> 
> Err, what else could we do with if=ide with q35?

We could create a cmd646 and attach drives to that.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 09.07.2012, at 13:06, Markus Armbruster wrote:
>
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>> them to leverage the power of AHCI!
>>>>> 
>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>> command line experience as for scsi or ide.
>>>>> 
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> 
>>>>> ---
>>>>> 
>>>>> v1 -> v2:
>>>>> 
>>>>>  - support more than a single drive per adapter
>>>>>  - support index= option
>>>>>  - treat IF_AHCI the same as IF_IDE
>>>> 
>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>> 
>>>>>  - add is_ata() helper to match AHCI || IDE
>>>> 
>>>> Not addressed:
>>>> 
>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>> if=ahci will create new controllers, which is generally not what you
>>>> want.  Ugh.
>>> 
>>> Can we even make it the default with q35 as long as our AHCI controller
>>> doesn't also expose a legacy interface for compatibility?
>> 
>> Err, what else could we do with if=ide with q35?
>
> We could create a cmd646 and attach drives to that.

Naive use of the command line then connects your drives to some an old
and slow add-on controller instead of the perfectly servicable and fast
on-board controller.

Let me rephrase my question: s/could/should/

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 13:19, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 09.07.2012, at 13:06, Markus Armbruster wrote:
>> 
>>> Kevin Wolf <kwolf@redhat.com> writes:
>>> 
>>>> Am 09.07.2012 10:50, schrieb Markus Armbruster:
>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>> 
>>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>>> them to leverage the power of AHCI!
>>>>>> 
>>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>>> command line experience as for scsi or ide.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> 
>>>>>> ---
>>>>>> 
>>>>>> v1 -> v2:
>>>>>> 
>>>>>> - support more than a single drive per adapter
>>>>>> - support index= option
>>>>>> - treat IF_AHCI the same as IF_IDE
>>>>> 
>>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>>> 
>>>>>> - add is_ata() helper to match AHCI || IDE
>>>>> 
>>>>> Not addressed:
>>>>> 
>>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>>> if=ahci will create new controllers, which is generally not what you
>>>>> want.  Ugh.
>>>> 
>>>> Can we even make it the default with q35 as long as our AHCI controller
>>>> doesn't also expose a legacy interface for compatibility?
>>> 
>>> Err, what else could we do with if=ide with q35?
>> 
>> We could create a cmd646 and attach drives to that.
> 
> Naive use of the command line then connects your drives to some an old
> and slow add-on controller instead of the perfectly servicable and fast
> on-board controller.
> 
> Let me rephrase my question: s/could/should/

Well, what I'm saying is that we should distinguish between -hda and if=ide. Make -hda attach the disk to the default controller. Make -cdrom attach the cdrom to the default controller. Make if=ide attach the disk to an IDE controller. If there's none in the system, spawn one.

That way the "naive" use case of passing no -hda or -hda -hdb is guaranteed to be good and fast. The case where a user actually asks for an IDE device, he still gets what he asked for.

Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 09.07.2012, at 10:50, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> We've had support for creating AHCI devices using -device for a while now,
>> but it's cumbersome to users. We really should provide an easier way for
>> them to leverage the power of AHCI!
>> 
>> So let's introduce a new if= option to -drive, giving users the same
>> command line experience as for scsi or ide.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> 
>> ---
>> 
>> v1 -> v2:
>> 
>>  - support more than a single drive per adapter
>>  - support index= option
>>  - treat IF_AHCI the same as IF_IDE
> 
> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
> 
>>  - add is_ata() helper to match AHCI || IDE
> 
> Not addressed:
> 
> Once we switch to q35, if=ahci will become a redundant wart: to add
> drives to the board's AHCI controller, you'll have to use if=ide.
> if=ahci will create new controllers, which is generally not what you
> want.  Ugh.
> 
> 
>> ---
>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>> blockdev.h      |    7 +++++++
>> qemu-options.hx |    7 ++++++-
>> vl.c            |    2 ++
>> 4 files changed, 65 insertions(+), 5 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 9e0a72a..744a886 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -32,6 +32,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] = {
>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>      */
>>     [IF_IDE] = 2,
>>     [IF_SCSI] = 7,
>> +    [IF_AHCI] = 6,
>> };
>> 
>> /*
>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>       if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>           for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>               ;
>           if (type == IF_COUNT) {
>               error_report("unsupported bus type '%s'", buf);
>               return NULL;
>           }
>       } else {
>           type = default_to_scsi ? IF_SCSI : IF_IDE;
>       }
> 
> A board can't default to IF_AHCI.  I guess what such a board would do is
> treat IF_IDE and IF_AHCI just the same.
> 
> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
> mean?  Let me try:
> 
> * "if=ide" means "if the board provides an IDE controller, create an IDE
>  device attached to it.  What kind of IDE controller the board provides
>  doesn't matter.  In particular, an AHCI controller is fine.

I don't think this is what we want it to mean. What we want is:

"if=ide" means "if the board provides an IDE controller, create an IDE device attached to it. If it does not provide one, create one".

> * "no if given" means "create a block device of the board's preferred
>  kind" in theory, and "default to either if=ide or if=scsi" in current
>  practice.

Yes. This should be ide for -M pc, scsi for -M pseries and ahci for -M q35.

> * "if=ahci" means "create an IDE device and attach it to a completely
>  seperate set of ich9-ahci controllers specifically created for the
>  "-drive if=ahci".  If the board provides an AHCI controller, it's not
>  used for if=ahci.  It may still be used for if=ide (depends on board).

The board should simply not create one then, no?

> 
>  Isn't this an embarrassment?
> 
>>     max_devs = if_max_devs[type];
>> 
>>     if (cyls || heads || secs) {
>> -        if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
>> +        if (cyls < 1 || (is_ata(type) && cyls > 16383)) {
>>             error_report("invalid physical cyls number");
>> 	    return NULL;
>> 	}
>> -        if (heads < 1 || (type == IF_IDE && heads > 16)) {
>> +        if (heads < 1 || (is_ata(type) && heads > 16)) {
>>             error_report("invalid physical heads number");
>> 	    return NULL;
>> 	}
>> -        if (secs < 1 || (type == IF_IDE && secs > 63)) {
>> +        if (secs < 1 || (is_ata(type) && secs > 63)) {
>>             error_report("invalid physical secs number");
>> 	    return NULL;
>> 	}
> 
> Trivial conflict with my "blockdev: Drop redundant CHS validation for
> if=ide".  Don't worry about it.
> 
> A few more instances of IF_IDE:
> 
>       on_write_error = BLOCK_ERR_STOP_ENOSPC;
>       if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
>           if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
>               error_report("werror is not supported by this bus type");
>               return NULL;
>           }
> 
>           on_write_error = parse_block_error_action(buf, 0);
>           if (on_write_error < 0) {
>               return NULL;
>           }
>       }
> 
>       on_read_error = BLOCK_ERR_REPORT;
>       if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
>           if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>               error_report("rerror is not supported by this bus type");
>               return NULL;
>           }
> 
>           on_read_error = parse_block_error_action(buf, 1);
>           if (on_read_error < 0) {
>               return NULL;
>           }
>       }
> 
> Are you sure you don't want to check for IF_AHCI?

Oh? Must have missed those...

> 
>> @@ -516,7 +518,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>     } else {
>>         /* no id supplied -> create one */
>>         dinfo->id = g_malloc0(32);
>> -        if (type == IF_IDE || type == IF_SCSI)
>> +        if (is_ata(type) || type == IF_SCSI)
>>             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>>         if (max_devs)
>>             snprintf(dinfo->id, 32, "%s%i%s%i",
>> @@ -546,6 +548,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>     case IF_IDE:
>>     case IF_SCSI:
>>     case IF_XEN:
>> +    case IF_AHCI:
>>     case IF_NONE:
>>         switch(media) {
>> 	case MEDIA_DISK:
>> @@ -628,6 +631,49 @@ err:
>>     return NULL;
>> }
>> 
>> +static void drive_populate_ahci(void)
>> +{
>> +    int bus;
>> +    QemuOpts *opts;
>> +
>> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
>> +        char devname[] = "ahciXXX";
>> +        int dev;
>> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
>> +
>> +        /* add ahci host controller */
>> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
>> +        qemu_opt_set(opts, "driver", "ich9-ahci");
> 
> Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
> provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
> ahci_init() correctly, it provides six.

I don't think I understand?

> Creates it even if the bus is unused.
> 
> While better than v1, which created one per -drive, it still wastes
> precious PCI slots, doesn't it?

Why would it? If no -drive if=ahci is give, drive_get_max_bus(IF_AHCI) returns 0, so no device gets created.

> 
> Hardcodes the AHCI controller device to "ich9-ahci".  Just we do for
> IF_SCSI, only in generic code instead of board-specific code.  No
> problem, we can always add another IF_ ;)  Just kidding; we'd add a way
> for the board to define the preferred controller device.  SCSI could use
> that, too.

I agree, we need some hint from the machine to tell us which devices it would prefer for its defaults. We have a hack that does something similar for virtio and s390, so we can say "virtio-blk" and get "virtio-blk-s390" on s390, but "virtio-blk-pci" on pci capable platforms. But this logic really should be machine, not architecture specific.

> 
>> +        for (dev = 0; dev < if_max_devs[IF_AHCI]; dev++) {
>> +            DriveInfo *dinfo = drive_get(IF_AHCI, bus, dev);
>> +            char busname[] = "ahciXXX.XXX";
>> +
>> +            if (!dinfo) {
>> +                continue;
>> +            }
>> +            snprintf(busname, sizeof(busname), "ahci%d.%d", bus, dev);
>> +
>> +            /* attach this ata disk to its bus */
>> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
>> +            qemu_opt_set(opts, "driver", "ide-drive");
>> +            qemu_opt_set(opts, "bus", busname);
>> +            qemu_opt_set(opts, "drive", dinfo->id);
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * This function creates -device options out of IF_xxx elements,
>> + * so that we don't have to mess with user friendly syntax parsing
>> + * in device emulation code.
>> + *
>> + * For now, only AHCI is implemented here.
>> + */
>> +void drive_populate(void)
>> +{
>> +    drive_populate_ahci();
>> +}
>> +
>> void do_commit(Monitor *mon, const QDict *qdict)
>> {
>>     const char *device = qdict_get_str(qdict, "device");
>> diff --git a/blockdev.h b/blockdev.h
>> index 260e16b..9d79558 100644
>> --- a/blockdev.h
>> +++ b/blockdev.h
>> @@ -23,6 +23,7 @@ typedef enum {
>>     IF_DEFAULT = -1,            /* for use with drive_add() only */
>>     IF_NONE,
>>     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
>> +    IF_AHCI,
>>     IF_COUNT
>> } BlockInterfaceType;
>> 
>> @@ -53,6 +54,12 @@ QemuOpts *drive_def(const char *optstr);
>> QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>>                     const char *optstr);
>> DriveInfo *drive_init(QemuOpts *arg, int default_to_scsi);
>> +void drive_populate(void);
>> +
>> +static inline bool is_ata(int type)
>> +{
>> +    return (type == IF_IDE) || (type == IF_AHCI);
>> +}
>> 
>> /* device-hotplug */
>> 
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 8b66264..9527c51 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -160,7 +160,7 @@ Special files such as iSCSI devices can be specified using protocol
>> specific URLs. See the section for "Device URL Syntax" for more information.
>> @item if=@var{interface}
>> This option defines on which type on interface the drive is connected.
>> -Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
>> +Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio, ahci.
>> @item bus=@var{bus},unit=@var{unit}
>> These options define where is connected the drive by defining the bus number and
>> the unit id.
>> @@ -260,6 +260,11 @@ You can connect a SCSI disk with unit ID 6 on the bus #0:
>> qemu-system-i386 -drive file=file,if=scsi,bus=0,unit=6
>> @end example
>> 
>> +You can attach a SATA disk using AHCI:
>> +@example
>> +qemu-system-i386 -drive file=file,if=ahci
>> +@end example
>> +
> 
> I still think the automatic controller creation should be documented.
> *Especially* since it does that even when the board provides a perfectly
> usable AHCI controller already.

We don't have any board today that provides a controller.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

[Alex's illegibly long lines wrapped]

Alexander Graf <agraf@suse.de> writes:

> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> We've had support for creating AHCI devices using -device for a while now,
>>> but it's cumbersome to users. We really should provide an easier way for
>>> them to leverage the power of AHCI!
>>> 
>>> So let's introduce a new if= option to -drive, giving users the same
>>> command line experience as for scsi or ide.
>>> 
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> 
>>> ---
>>> 
>>> v1 -> v2:
>>> 
>>>  - support more than a single drive per adapter
>>>  - support index= option
>>>  - treat IF_AHCI the same as IF_IDE
>> 
>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>> 
>>>  - add is_ata() helper to match AHCI || IDE
>> 
>> Not addressed:
>> 
>> Once we switch to q35, if=ahci will become a redundant wart: to add
>> drives to the board's AHCI controller, you'll have to use if=ide.
>> if=ahci will create new controllers, which is generally not what you
>> want.  Ugh.
>> 
>> 
>>> ---
>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>> blockdev.h      |    7 +++++++
>>> qemu-options.hx |    7 ++++++-
>>> vl.c            |    2 ++
>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/blockdev.c b/blockdev.c
>>> index 9e0a72a..744a886 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -32,6 +32,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] = {
>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>      */
>>>     [IF_IDE] = 2,
>>>     [IF_SCSI] = 7,
>>> +    [IF_AHCI] = 6,
>>> };
>>> 
>>> /*
>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>       if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>           for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>               ;
>>           if (type == IF_COUNT) {
>>               error_report("unsupported bus type '%s'", buf);
>>               return NULL;
>>           }
>>       } else {
>>           type = default_to_scsi ? IF_SCSI : IF_IDE;
>>       }
>> 
>> A board can't default to IF_AHCI.  I guess what such a board would do is
>> treat IF_IDE and IF_AHCI just the same.
>> 
>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>> mean?  Let me try:
>> 
>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>  device attached to it.  What kind of IDE controller the board provides
>>  doesn't matter.  In particular, an AHCI controller is fine.
>
> I don't think this is what we want it to mean. What we want is:
>
> "if=ide" means "if the board provides an IDE controller, create an IDE
> device attached to it. If it does not provide one, create one".

Okay, we got two competing ideas here.

1. -drive doesn't give you any control over the kind of IDE controller.
You can select an IDE bus by number (bus=...), and you get whatever
existing controller provides this bus.  If none exists, a board-specific
one may be created for your convenience.  If you need more control, use
-device to set up controllers the way you want.

2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
you get an existing AHCI controller.  If none exists, a board-specific
one may be created for your convenience.  With if=ide, you get an
existing non-AHCI controller.  If none exists, a board-specific one may
be created for your convenience.  If you need more control, use -device
to set up controllers the way you want.

The question to answer is whether the difference between AHCI and
non-AHCI is so important that we want to provide control in -drive.

What I'd like to avoid is casual users setting up new guests with our
shiny new q35 board getting their IDE drives connected to some slow, old
controller just because they haven't discovered that if=ide doesn't cut
it anymore, and they need to use if=ahci now.

>> * "no if given" means "create a block device of the board's preferred
>>  kind" in theory, and "default to either if=ide or if=scsi" in current
>>  practice.
>
> Yes. This should be ide for -M pc, scsi for -M pseries and ahci for -M
> q35.
>
>> * "if=ahci" means "create an IDE device and attach it to a completely
>>  seperate set of ich9-ahci controllers specifically created for the
>>  "-drive if=ahci".  If the board provides an AHCI controller, it's not
>>  used for if=ahci.  It may still be used for if=ide (depends on board).
>
> The board should simply not create one then, no?

The controller is an integral part of the board.  Chipset even.  It
should always be created, drive or no drives.

Here's a meaning that's consistent with the one you proposed for if=ide:
if the board provides an AHCI controller, create an IDE device attached
to it.  If it does not provide one, create one first.

See, I really don't want yet another if=FOO with its own special
behavior.  I'd be much happier with a set of if=... that behaves pretty
much the same.  Ideally, all of them, but that's a tall order.  However,
let's not make it worse by adding new special behaviors.  I'm trying to
find a way to make if=ide and if=ahci behave pretty much the same.  Do
you agree with this goal in principle?

Let me refine.  If the controller identified by if and bus exists, use
it.  Else, if such a controller can be created, create and use it.
Else, fail.

if=ide is already consistent with this meaning, in a somewhat degenerate
form: "can be created" is always false.  Only the board creates IDE
controllers.  All our boards create zero or two IDE buses.  If it
creates two, attempts to use more fail.  Wart: if it creates none,
attempts to use any are silently ignored.  Wart: if the user adds IDE
controllers with -device, we don't use them.

if=scsi isn't consistent with this meaning.  It could be made
consistent, but I'm not asking you do that work now.

Your if=ahci isn't quite consistent with this meaning, because it never
uses existing controllers.

I'm open to other ways to make if=ide and if=ahci consistent.

>>  Isn't this an embarrassment?
>> 
>>>     max_devs = if_max_devs[type];
>>> 
>>>     if (cyls || heads || secs) {
>>> -        if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
>>> +        if (cyls < 1 || (is_ata(type) && cyls > 16383)) {
>>>             error_report("invalid physical cyls number");
>>> 	    return NULL;
>>> 	}
>>> -        if (heads < 1 || (type == IF_IDE && heads > 16)) {
>>> +        if (heads < 1 || (is_ata(type) && heads > 16)) {
>>>             error_report("invalid physical heads number");
>>> 	    return NULL;
>>> 	}
>>> -        if (secs < 1 || (type == IF_IDE && secs > 63)) {
>>> +        if (secs < 1 || (is_ata(type) && secs > 63)) {
>>>             error_report("invalid physical secs number");
>>> 	    return NULL;
>>> 	}
>> 
>> Trivial conflict with my "blockdev: Drop redundant CHS validation for
>> if=ide".  Don't worry about it.
>> 
>> A few more instances of IF_IDE:
>> 
>>       on_write_error = BLOCK_ERR_STOP_ENOSPC;
>>       if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
>>           if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
>>               error_report("werror is not supported by this bus type");
>>               return NULL;
>>           }
>> 
>>           on_write_error = parse_block_error_action(buf, 0);
>>           if (on_write_error < 0) {
>>               return NULL;
>>           }
>>       }
>> 
>>       on_read_error = BLOCK_ERR_REPORT;
>>       if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
>>           if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>>               error_report("rerror is not supported by this bus type");
>>               return NULL;
>>           }
>> 
>>           on_read_error = parse_block_error_action(buf, 1);
>>           if (on_read_error < 0) {
>>               return NULL;
>>           }
>>       }
>> 
>> Are you sure you don't want to check for IF_AHCI?
>
> Oh? Must have missed those...

You're welcome ;)

>>> @@ -516,7 +518,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>     } else {
>>>         /* no id supplied -> create one */
>>>         dinfo->id = g_malloc0(32);
>>> -        if (type == IF_IDE || type == IF_SCSI)
>>> +        if (is_ata(type) || type == IF_SCSI)
>>>             mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>>>         if (max_devs)
>>>             snprintf(dinfo->id, 32, "%s%i%s%i",
>>> @@ -546,6 +548,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>     case IF_IDE:
>>>     case IF_SCSI:
>>>     case IF_XEN:
>>> +    case IF_AHCI:
>>>     case IF_NONE:
>>>         switch(media) {
>>> 	case MEDIA_DISK:
>>> @@ -628,6 +631,49 @@ err:
>>>     return NULL;
>>> }
>>> 
>>> +static void drive_populate_ahci(void)
>>> +{
>>> +    int bus;
>>> +    QemuOpts *opts;
>>> +
>>> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
>>> +        char devname[] = "ahciXXX";
>>> +        int dev;
>>> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
>>> +
>>> +        /* add ahci host controller */
>>> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
>>> +        qemu_opt_set(opts, "driver", "ich9-ahci");
>> 
>> Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
>> provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
>> ahci_init() correctly, it provides six.
>
> I don't think I understand?

With "-drive if=ahci,bus=5,...", drive_get_max_bus() returns 5, and the
loop executes six times, creating six AHCI controllers, doesn't it?
Each of them provides six buses, for a total of 36.

>> Creates it even if the bus is unused.
>> 
>> While better than v1, which created one per -drive, it still wastes
>> precious PCI slots, doesn't it?
>
> Why would it? If no -drive if=ahci is give, drive_get_max_bus(IF_AHCI)
> returns 0, so no device gets created.
>
>> Hardcodes the AHCI controller device to "ich9-ahci".  Just we do for
>> IF_SCSI, only in generic code instead of board-specific code.  No
>> problem, we can always add another IF_ ;)  Just kidding; we'd add a way
>> for the board to define the preferred controller device.  SCSI could use
>> that, too.
>
> I agree, we need some hint from the machine to tell us which devices
> it would prefer for its defaults. We have a hack that does something
> similar for virtio and s390, so we can say "virtio-blk" and get
> "virtio-blk-s390" on s390, but "virtio-blk-pci" on pci capable
> platforms. But this logic really should be machine, not architecture
> specific.

Agree.

>>> +        for (dev = 0; dev < if_max_devs[IF_AHCI]; dev++) {
>>> +            DriveInfo *dinfo = drive_get(IF_AHCI, bus, dev);
>>> +            char busname[] = "ahciXXX.XXX";
>>> +
>>> +            if (!dinfo) {
>>> +                continue;
>>> +            }
>>> +            snprintf(busname, sizeof(busname), "ahci%d.%d", bus, dev);
>>> +
>>> +            /* attach this ata disk to its bus */
>>> +            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0, NULL);
>>> +            qemu_opt_set(opts, "driver", "ide-drive");
>>> +            qemu_opt_set(opts, "bus", busname);
>>> +            qemu_opt_set(opts, "drive", dinfo->id);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * This function creates -device options out of IF_xxx elements,
>>> + * so that we don't have to mess with user friendly syntax parsing
>>> + * in device emulation code.
>>> + *
>>> + * For now, only AHCI is implemented here.
>>> + */
>>> +void drive_populate(void)
>>> +{
>>> +    drive_populate_ahci();
>>> +}
>>> +
>>> void do_commit(Monitor *mon, const QDict *qdict)
>>> {
>>>     const char *device = qdict_get_str(qdict, "device");
[...]
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 8b66264..9527c51 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -160,7 +160,7 @@ Special files such as iSCSI devices can be specified using protocol
>>> specific URLs. See the section for "Device URL Syntax" for more information.
>>> @item if=@var{interface}
>>> This option defines on which type on interface the drive is connected.
>>> -Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.
>>> +Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio, ahci.
>>> @item bus=@var{bus},unit=@var{unit}
>>> These options define where is connected the drive by defining the bus number and
>>> the unit id.
>>> @@ -260,6 +260,11 @@ You can connect a SCSI disk with unit ID 6 on the bus #0:
>>> qemu-system-i386 -drive file=file,if=scsi,bus=0,unit=6
>>> @end example
>>> 
>>> +You can attach a SATA disk using AHCI:
>>> +@example
>>> +qemu-system-i386 -drive file=file,if=ahci
>>> +@end example
>>> +
>> 
>> I still think the automatic controller creation should be documented.
>> *Especially* since it does that even when the board provides a perfectly
>> usable AHCI controller already.
>
> We don't have any board today that provides a controller.

Let's revisit this when we agree on how if=ahci should work.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 12.07.2012, at 10:17, Markus Armbruster wrote:

> [Alex's illegibly long lines wrapped]
> 
> Alexander Graf <agraf@suse.de> writes:
> 
>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> We've had support for creating AHCI devices using -device for a while now,
>>>> but it's cumbersome to users. We really should provide an easier way for
>>>> them to leverage the power of AHCI!
>>>> 
>>>> So let's introduce a new if= option to -drive, giving users the same
>>>> command line experience as for scsi or ide.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> 
>>>> ---
>>>> 
>>>> v1 -> v2:
>>>> 
>>>> - support more than a single drive per adapter
>>>> - support index= option
>>>> - treat IF_AHCI the same as IF_IDE
>>> 
>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>> 
>>>> - add is_ata() helper to match AHCI || IDE
>>> 
>>> Not addressed:
>>> 
>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>> if=ahci will create new controllers, which is generally not what you
>>> want.  Ugh.
>>> 
>>> 
>>>> ---
>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>> blockdev.h      |    7 +++++++
>>>> qemu-options.hx |    7 ++++++-
>>>> vl.c            |    2 ++
>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 9e0a72a..744a886 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -32,6 +32,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] = {
>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>     */
>>>>    [IF_IDE] = 2,
>>>>    [IF_SCSI] = 7,
>>>> +    [IF_AHCI] = 6,
>>>> };
>>>> 
>>>> /*
>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>      if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>          for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>              ;
>>>          if (type == IF_COUNT) {
>>>              error_report("unsupported bus type '%s'", buf);
>>>              return NULL;
>>>          }
>>>      } else {
>>>          type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>      }
>>> 
>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>> treat IF_IDE and IF_AHCI just the same.
>>> 
>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>> mean?  Let me try:
>>> 
>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>> device attached to it.  What kind of IDE controller the board provides
>>> doesn't matter.  In particular, an AHCI controller is fine.
>> 
>> I don't think this is what we want it to mean. What we want is:
>> 
>> "if=ide" means "if the board provides an IDE controller, create an IDE
>> device attached to it. If it does not provide one, create one".
> 
> Okay, we got two competing ideas here.
> 
> 1. -drive doesn't give you any control over the kind of IDE controller.
> You can select an IDE bus by number (bus=...), and you get whatever
> existing controller provides this bus.  If none exists, a board-specific
> one may be created for your convenience.  If you need more control, use
> -device to set up controllers the way you want.
> 
> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
> you get an existing AHCI controller.  If none exists, a board-specific
> one may be created for your convenience.  With if=ide, you get an
> existing non-AHCI controller.  If none exists, a board-specific one may
> be created for your convenience.  If you need more control, use -device
> to set up controllers the way you want.
> 
> The question to answer is whether the difference between AHCI and
> non-AHCI is so important that we want to provide control in -drive.
> 
> What I'd like to avoid is casual users setting up new guests with our
> shiny new q35 board getting their IDE drives connected to some slow, old
> controller just because they haven't discovered that if=ide doesn't cut
> it anymore, and they need to use if=ahci now.

Ah, I think I understand the main issue now: You're thinking of AHCI as an IDE controller.

It isn't. AHCI is on the same level as IDE. They both speak ATA, but the guest os interface is completely different. You can write a generic IDE driver, but that won't be able to talk to an AHCI controller in AHCI mode. You can write a generic AHCI driver, but that won't be able to talk to an IDE controller.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 12.07.2012, at 10:17, Markus Armbruster wrote:
>
>> [Alex's illegibly long lines wrapped]
>> 
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>>> 
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>> them to leverage the power of AHCI!
>>>>> 
>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>> command line experience as for scsi or ide.
>>>>> 
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> 
>>>>> ---
>>>>> 
>>>>> v1 -> v2:
>>>>> 
>>>>> - support more than a single drive per adapter
>>>>> - support index= option
>>>>> - treat IF_AHCI the same as IF_IDE
>>>> 
>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>> 
>>>>> - add is_ata() helper to match AHCI || IDE
>>>> 
>>>> Not addressed:
>>>> 
>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>> if=ahci will create new controllers, which is generally not what you
>>>> want.  Ugh.
>>>> 
>>>> 
>>>>> ---
>>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>> blockdev.h      |    7 +++++++
>>>>> qemu-options.hx |    7 ++++++-
>>>>> vl.c            |    2 ++
>>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>>> 
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index 9e0a72a..744a886 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -32,6 +32,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] = {
>>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>>     */
>>>>>    [IF_IDE] = 2,
>>>>>    [IF_SCSI] = 7,
>>>>> +    [IF_AHCI] = 6,
>>>>> };
>>>>> 
>>>>> /*
>>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>      if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>>          for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>>              ;
>>>>          if (type == IF_COUNT) {
>>>>              error_report("unsupported bus type '%s'", buf);
>>>>              return NULL;
>>>>          }
>>>>      } else {
>>>>          type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>>      }
>>>> 
>>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>>> treat IF_IDE and IF_AHCI just the same.
>>>> 
>>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>>> mean?  Let me try:
>>>> 
>>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>>> device attached to it.  What kind of IDE controller the board provides
>>>> doesn't matter.  In particular, an AHCI controller is fine.
>>> 
>>> I don't think this is what we want it to mean. What we want is:
>>> 
>>> "if=ide" means "if the board provides an IDE controller, create an IDE
>>> device attached to it. If it does not provide one, create one".
>> 
>> Okay, we got two competing ideas here.
>> 
>> 1. -drive doesn't give you any control over the kind of IDE controller.
>> You can select an IDE bus by number (bus=...), and you get whatever
>> existing controller provides this bus.  If none exists, a board-specific
>> one may be created for your convenience.  If you need more control, use
>> -device to set up controllers the way you want.
>> 
>> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
>> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
>> you get an existing AHCI controller.  If none exists, a board-specific
>> one may be created for your convenience.  With if=ide, you get an
>> existing non-AHCI controller.  If none exists, a board-specific one may
>> be created for your convenience.  If you need more control, use -device
>> to set up controllers the way you want.
>> 
>> The question to answer is whether the difference between AHCI and
>> non-AHCI is so important that we want to provide control in -drive.
>> 
>> What I'd like to avoid is casual users setting up new guests with our
>> shiny new q35 board getting their IDE drives connected to some slow, old
>> controller just because they haven't discovered that if=ide doesn't cut
>> it anymore, and they need to use if=ahci now.
>
> Ah, I think I understand the main issue now: You're thinking of AHCI
> as an IDE controller.
>
> It isn't. AHCI is on the same level as IDE. They both speak ATA, but
> the guest os interface is completely different. You can write a
> generic IDE driver, but that won't be able to talk to an AHCI
> controller in AHCI mode. You can write a generic AHCI driver, but that
> won't be able to talk to an IDE controller.

Yes, but does the naive command line user care?

-serial configures a serial device.  The kind of device depends on the
board: 16550A UARTs with -M pc, Exynos 4210 UARTs with -M nuri, ColdFire
UARTs with -M an5206, ...  You can't write a generic serial device
driver.

Any thoughts on the remainder of my message, behavior of if=ahci in
particular?

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 12.07.2012, at 13:09, Markus Armbruster wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 12.07.2012, at 10:17, Markus Armbruster wrote:
>> 
>>> [Alex's illegibly long lines wrapped]
>>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>>>> 
>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>> 
>>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>>> them to leverage the power of AHCI!
>>>>>> 
>>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>>> command line experience as for scsi or ide.
>>>>>> 
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>> 
>>>>>> ---
>>>>>> 
>>>>>> v1 -> v2:
>>>>>> 
>>>>>> - support more than a single drive per adapter
>>>>>> - support index= option
>>>>>> - treat IF_AHCI the same as IF_IDE
>>>>> 
>>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>>> 
>>>>>> - add is_ata() helper to match AHCI || IDE
>>>>> 
>>>>> Not addressed:
>>>>> 
>>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>>> if=ahci will create new controllers, which is generally not what you
>>>>> want.  Ugh.
>>>>> 
>>>>> 
>>>>>> ---
>>>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>> blockdev.h      |    7 +++++++
>>>>>> qemu-options.hx |    7 ++++++-
>>>>>> vl.c            |    2 ++
>>>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>>>> 
>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>> index 9e0a72a..744a886 100644
>>>>>> --- a/blockdev.c
>>>>>> +++ b/blockdev.c
>>>>>> @@ -32,6 +32,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] = {
>>>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>>>    */
>>>>>>   [IF_IDE] = 2,
>>>>>>   [IF_SCSI] = 7,
>>>>>> +    [IF_AHCI] = 6,
>>>>>> };
>>>>>> 
>>>>>> /*
>>>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>>     if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>>>         for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>>>             ;
>>>>>         if (type == IF_COUNT) {
>>>>>             error_report("unsupported bus type '%s'", buf);
>>>>>             return NULL;
>>>>>         }
>>>>>     } else {
>>>>>         type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>>>     }
>>>>> 
>>>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>>>> treat IF_IDE and IF_AHCI just the same.
>>>>> 
>>>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>>>> mean?  Let me try:
>>>>> 
>>>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>>>> device attached to it.  What kind of IDE controller the board provides
>>>>> doesn't matter.  In particular, an AHCI controller is fine.
>>>> 
>>>> I don't think this is what we want it to mean. What we want is:
>>>> 
>>>> "if=ide" means "if the board provides an IDE controller, create an IDE
>>>> device attached to it. If it does not provide one, create one".
>>> 
>>> Okay, we got two competing ideas here.
>>> 
>>> 1. -drive doesn't give you any control over the kind of IDE controller.
>>> You can select an IDE bus by number (bus=...), and you get whatever
>>> existing controller provides this bus.  If none exists, a board-specific
>>> one may be created for your convenience.  If you need more control, use
>>> -device to set up controllers the way you want.
>>> 
>>> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
>>> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
>>> you get an existing AHCI controller.  If none exists, a board-specific
>>> one may be created for your convenience.  With if=ide, you get an
>>> existing non-AHCI controller.  If none exists, a board-specific one may
>>> be created for your convenience.  If you need more control, use -device
>>> to set up controllers the way you want.
>>> 
>>> The question to answer is whether the difference between AHCI and
>>> non-AHCI is so important that we want to provide control in -drive.
>>> 
>>> What I'd like to avoid is casual users setting up new guests with our
>>> shiny new q35 board getting their IDE drives connected to some slow, old
>>> controller just because they haven't discovered that if=ide doesn't cut
>>> it anymore, and they need to use if=ahci now.
>> 
>> Ah, I think I understand the main issue now: You're thinking of AHCI
>> as an IDE controller.
>> 
>> It isn't. AHCI is on the same level as IDE. They both speak ATA, but
>> the guest os interface is completely different. You can write a
>> generic IDE driver, but that won't be able to talk to an AHCI
>> controller in AHCI mode. You can write a generic AHCI driver, but that
>> won't be able to talk to an IDE controller.
> 
> Yes, but does the naive command line user care?
> 
> -serial configures a serial device.  The kind of device depends on the
> board: 16550A UARTs with -M pc, Exynos 4210 UARTs with -M nuri, ColdFire
> UARTs with -M an5206, ...  You can't write a generic serial device
> driver.
> 
> Any thoughts on the remainder of my message, behavior of if=ahci in
> particular?

I think this is the core of the disagreement we're having. I believe that IDE and AHCI are as much different as IDE and SCSI, while you're trying to see them as a single thing with different implementations.

If it was me, I think I'd go for if=<device name> and create machine specific aliases. That way we could be specific about the device we want to create (LSI vs megasas vs virtio-scsi for example), but at the same time provide "sane" defaults, like "ide" or "scsi". Keep in mind that if=ahci will never work with the full ide syntax as Gerd pointed out. It's fundamentally a different bus logic.

However, I think the best way forward would be to bring this up on the call, so we can reach a conclusion.


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 12.07.2012, at 13:09, Markus Armbruster wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 12.07.2012, at 10:17, Markus Armbruster wrote:
>>> 
>>>> [Alex's illegibly long lines wrapped]
>>>> 
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>>>>> 
>>>>>> Alexander Graf <agraf@suse.de> writes:
>>>>>> 
>>>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>>>> them to leverage the power of AHCI!
>>>>>>> 
>>>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>>>> command line experience as for scsi or ide.
>>>>>>> 
>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>> 
>>>>>>> ---
>>>>>>> 
>>>>>>> v1 -> v2:
>>>>>>> 
>>>>>>> - support more than a single drive per adapter
>>>>>>> - support index= option
>>>>>>> - treat IF_AHCI the same as IF_IDE
>>>>>> 
>>>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>>>> 
>>>>>>> - add is_ata() helper to match AHCI || IDE
>>>>>> 
>>>>>> Not addressed:
>>>>>> 
>>>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>>>> if=ahci will create new controllers, which is generally not what you
>>>>>> want.  Ugh.
>>>>>> 
>>>>>> 
>>>>>>> ---
>>>>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>>>> blockdev.h      |    7 +++++++
>>>>>>> qemu-options.hx |    7 ++++++-
>>>>>>> vl.c            |    2 ++
>>>>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>>> index 9e0a72a..744a886 100644
>>>>>>> --- a/blockdev.c
>>>>>>> +++ b/blockdev.c
>>>>>>> @@ -32,6 +32,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] = {
>>>>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>>>>    */
>>>>>>>   [IF_IDE] = 2,
>>>>>>>   [IF_SCSI] = 7,
>>>>>>> +    [IF_AHCI] = 6,
>>>>>>> };
>>>>>>> 
>>>>>>> /*
>>>>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>>>     if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>>>>         for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>>>>             ;
>>>>>>         if (type == IF_COUNT) {
>>>>>>             error_report("unsupported bus type '%s'", buf);
>>>>>>             return NULL;
>>>>>>         }
>>>>>>     } else {
>>>>>>         type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>>>>     }
>>>>>> 
>>>>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>>>>> treat IF_IDE and IF_AHCI just the same.
>>>>>> 
>>>>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>>>>> mean?  Let me try:
>>>>>> 
>>>>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>>>>> device attached to it.  What kind of IDE controller the board provides
>>>>>> doesn't matter.  In particular, an AHCI controller is fine.
>>>>> 
>>>>> I don't think this is what we want it to mean. What we want is:
>>>>> 
>>>>> "if=ide" means "if the board provides an IDE controller, create an IDE
>>>>> device attached to it. If it does not provide one, create one".
>>>> 
>>>> Okay, we got two competing ideas here.
>>>> 
>>>> 1. -drive doesn't give you any control over the kind of IDE controller.
>>>> You can select an IDE bus by number (bus=...), and you get whatever
>>>> existing controller provides this bus.  If none exists, a board-specific
>>>> one may be created for your convenience.  If you need more control, use
>>>> -device to set up controllers the way you want.
>>>> 
>>>> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
>>>> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
>>>> you get an existing AHCI controller.  If none exists, a board-specific
>>>> one may be created for your convenience.  With if=ide, you get an
>>>> existing non-AHCI controller.  If none exists, a board-specific one may
>>>> be created for your convenience.  If you need more control, use -device
>>>> to set up controllers the way you want.
>>>> 
>>>> The question to answer is whether the difference between AHCI and
>>>> non-AHCI is so important that we want to provide control in -drive.
>>>> 
>>>> What I'd like to avoid is casual users setting up new guests with our
>>>> shiny new q35 board getting their IDE drives connected to some slow, old
>>>> controller just because they haven't discovered that if=ide doesn't cut
>>>> it anymore, and they need to use if=ahci now.
>>> 
>>> Ah, I think I understand the main issue now: You're thinking of AHCI
>>> as an IDE controller.
>>> 
>>> It isn't. AHCI is on the same level as IDE. They both speak ATA, but
>>> the guest os interface is completely different. You can write a
>>> generic IDE driver, but that won't be able to talk to an AHCI
>>> controller in AHCI mode. You can write a generic AHCI driver, but that
>>> won't be able to talk to an IDE controller.
>> 
>> Yes, but does the naive command line user care?
>> 
>> -serial configures a serial device.  The kind of device depends on the
>> board: 16550A UARTs with -M pc, Exynos 4210 UARTs with -M nuri, ColdFire
>> UARTs with -M an5206, ...  You can't write a generic serial device
>> driver.
>> 
>> Any thoughts on the remainder of my message, behavior of if=ahci in
>> particular?
>
> I think this is the core of the disagreement we're having. I believe
> that IDE and AHCI are as much different as IDE and SCSI, while you're
> trying to see them as a single thing with different implementations.

Actually, this is *one* topic.  There's a second one further down, which
you haven't replied to at all.  Namely: assuming we want if=ahci, how
should it behave?

Your patch makes it behave unlike any other if=FOO.  I explained why I
don't like that, and what could be done about it.  Please reply to my
comments.  Thanks.

[...]

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Paolo Bonzini]

Il 12/07/2012 13:09, Markus Armbruster ha scritto:
>> >
>> > It isn't. AHCI is on the same level as IDE. They both speak ATA, but
>> > the guest os interface is completely different. You can write a
>> > generic IDE driver, but that won't be able to talk to an AHCI
>> > controller in AHCI mode. You can write a generic AHCI driver, but that
>> > won't be able to talk to an IDE controller.
> Yes, but does the naive command line user care?
> 
> -serial configures a serial device.  The kind of device depends on the
> board: 16550A UARTs with -M pc, Exynos 4210 UARTs with -M nuri, ColdFire
> UARTs with -M an5206, ...  You can't write a generic serial device
> driver.

The naive command line user uses -cdrom or just "qemu file.img", they do
not use -drive.

The non-naive command line user who has so far been using -drive with
if=ide, can learn to omit it when they move to q35.

Paolo

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Gerd Hoffmann]

  Hi,

> What I'd like to avoid is casual users setting up new guests with our
> shiny new q35 board getting their IDE drives connected to some slow, old
> controller just because they haven't discovered that if=ide doesn't cut
> it anymore, and they need to use if=ahci now.

That will not work.  ahci has no such thing as master/slave.  Each ahci
port (modeled as ide bus in qemu for ata emulation code reuse reasons)
accepts one drive only.

Thus -drive if=ide,bus=x,unit=1,file=... will not work with ahci.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> What I'd like to avoid is casual users setting up new guests with our
>> shiny new q35 board getting their IDE drives connected to some slow, old
>> controller just because they haven't discovered that if=ide doesn't cut
>> it anymore, and they need to use if=ahci now.
>
> That will not work.  ahci has no such thing as master/slave.  Each ahci
> port (modeled as ide bus in qemu for ata emulation code reuse reasons)
> accepts one drive only.
>
> Thus -drive if=ide,bus=x,unit=1,file=... will not work with ahci.

I see. To be precise: the part that doesn't work is unit=N with N>0.

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Alexander Graf]

On 12.07.2012, at 10:17, Markus Armbruster wrote:

> [Alex's illegibly long lines wrapped]
> 
> Alexander Graf <agraf@suse.de> writes:
> 
>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> We've had support for creating AHCI devices using -device for a while now,
>>>> but it's cumbersome to users. We really should provide an easier way for
>>>> them to leverage the power of AHCI!
>>>> 
>>>> So let's introduce a new if= option to -drive, giving users the same
>>>> command line experience as for scsi or ide.
>>>> 
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> 
>>>> ---
>>>> 
>>>> v1 -> v2:
>>>> 
>>>> - support more than a single drive per adapter
>>>> - support index= option
>>>> - treat IF_AHCI the same as IF_IDE
>>> 
>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>> 
>>>> - add is_ata() helper to match AHCI || IDE
>>> 
>>> Not addressed:
>>> 
>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>> if=ahci will create new controllers, which is generally not what you
>>> want.  Ugh.
>>> 
>>> 
>>>> ---
>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>> blockdev.h      |    7 +++++++
>>>> qemu-options.hx |    7 ++++++-
>>>> vl.c            |    2 ++
>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 9e0a72a..744a886 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -32,6 +32,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] = {
>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>     */
>>>>    [IF_IDE] = 2,
>>>>    [IF_SCSI] = 7,
>>>> +    [IF_AHCI] = 6,
>>>> };
>>>> 
>>>> /*
>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>      if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>          for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>              ;
>>>          if (type == IF_COUNT) {
>>>              error_report("unsupported bus type '%s'", buf);
>>>              return NULL;
>>>          }
>>>      } else {
>>>          type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>      }
>>> 
>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>> treat IF_IDE and IF_AHCI just the same.
>>> 
>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>> mean?  Let me try:
>>> 
>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>> device attached to it.  What kind of IDE controller the board provides
>>> doesn't matter.  In particular, an AHCI controller is fine.
>> 
>> I don't think this is what we want it to mean. What we want is:
>> 
>> "if=ide" means "if the board provides an IDE controller, create an IDE
>> device attached to it. If it does not provide one, create one".
> 
> Okay, we got two competing ideas here.
> 
> 1. -drive doesn't give you any control over the kind of IDE controller.
> You can select an IDE bus by number (bus=...), and you get whatever
> existing controller provides this bus.  If none exists, a board-specific
> one may be created for your convenience.  If you need more control, use
> -device to set up controllers the way you want.
> 
> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
> you get an existing AHCI controller.  If none exists, a board-specific
> one may be created for your convenience.  With if=ide, you get an
> existing non-AHCI controller.  If none exists, a board-specific one may
> be created for your convenience.  If you need more control, use -device
> to set up controllers the way you want.
> 
> The question to answer is whether the difference between AHCI and
> non-AHCI is so important that we want to provide control in -drive.
> 
> What I'd like to avoid is casual users setting up new guests with our
> shiny new q35 board getting their IDE drives connected to some slow, old
> controller just because they haven't discovered that if=ide doesn't cut
> it anymore, and they need to use if=ahci now.
> 
>>> * "no if given" means "create a block device of the board's preferred
>>> kind" in theory, and "default to either if=ide or if=scsi" in current
>>> practice.
>> 
>> Yes. This should be ide for -M pc, scsi for -M pseries and ahci for -M
>> q35.
>> 
>>> * "if=ahci" means "create an IDE device and attach it to a completely
>>> seperate set of ich9-ahci controllers specifically created for the
>>> "-drive if=ahci".  If the board provides an AHCI controller, it's not
>>> used for if=ahci.  It may still be used for if=ide (depends on board).
>> 
>> The board should simply not create one then, no?
> 
> The controller is an integral part of the board.  Chipset even.  It
> should always be created, drive or no drives.
> 
> Here's a meaning that's consistent with the one you proposed for if=ide:
> if the board provides an AHCI controller, create an IDE device attached
> to it.  If it does not provide one, create one first.
> 
> See, I really don't want yet another if=FOO with its own special
> behavior.  I'd be much happier with a set of if=... that behaves pretty
> much the same.  Ideally, all of them, but that's a tall order.  However,
> let's not make it worse by adding new special behaviors.  I'm trying to
> find a way to make if=ide and if=ahci behave pretty much the same.  Do
> you agree with this goal in principle?

I agree that they should behave similar within their scope, yes.

> Let me refine.  If the controller identified by if and bus exists, use
> it.  Else, if such a controller can be created, create and use it.
> Else, fail.

At the point in time when -drive is passed in, we don't know what controllers the board might create yet, no?

> if=ide is already consistent with this meaning, in a somewhat degenerate
> form: "can be created" is always false.  Only the board creates IDE
> controllers.  All our boards create zero or two IDE buses.  If it

Sure, which renders if=ide completely useless on machines that don't spawn an IDE controller by default, like -M mpc8544ds.

> creates two, attempts to use more fail.  Wart: if it creates none,
> attempts to use any are silently ignored.  Wart: if the user adds IDE
> controllers with -device, we don't use them.
> 
> if=scsi isn't consistent with this meaning.  It could be made
> consistent, but I'm not asking you do that work now.
> 
> Your if=ahci isn't quite consistent with this meaning, because it never
> uses existing controllers.
> 
> I'm open to other ways to make if=ide and if=ahci consistent.

I think we have 2 options

1) Keep the creation process 2-tiered. Parse the cmdline before machine create. Plug disks on after/during machine create. This can get ugly pretty quick.

2) Make the process be linear. Command line parsing goes first. Then goes drive parsing which creates -device parameters. Then machine creation. During the time we parse the drives we should be able to evaluate machine info, so we can put bits in there like "spawns the first 2 IDE buses itself".

> 
>>> Isn't this an embarrassment?
>>> 
>>>>    max_devs = if_max_devs[type];
>>>> 
>>>>    if (cyls || heads || secs) {
>>>> -        if (cyls < 1 || (type == IF_IDE && cyls > 16383)) {
>>>> +        if (cyls < 1 || (is_ata(type) && cyls > 16383)) {
>>>>            error_report("invalid physical cyls number");
>>>> 	    return NULL;
>>>> 	}
>>>> -        if (heads < 1 || (type == IF_IDE && heads > 16)) {
>>>> +        if (heads < 1 || (is_ata(type) && heads > 16)) {
>>>>            error_report("invalid physical heads number");
>>>> 	    return NULL;
>>>> 	}
>>>> -        if (secs < 1 || (type == IF_IDE && secs > 63)) {
>>>> +        if (secs < 1 || (is_ata(type) && secs > 63)) {
>>>>            error_report("invalid physical secs number");
>>>> 	    return NULL;
>>>> 	}
>>> 
>>> Trivial conflict with my "blockdev: Drop redundant CHS validation for
>>> if=ide".  Don't worry about it.
>>> 
>>> A few more instances of IF_IDE:
>>> 
>>>      on_write_error = BLOCK_ERR_STOP_ENOSPC;
>>>      if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
>>>          if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type != IF_NONE) {
>>>              error_report("werror is not supported by this bus type");
>>>              return NULL;
>>>          }
>>> 
>>>          on_write_error = parse_block_error_action(buf, 0);
>>>          if (on_write_error < 0) {
>>>              return NULL;
>>>          }
>>>      }
>>> 
>>>      on_read_error = BLOCK_ERR_REPORT;
>>>      if ((buf = qemu_opt_get(opts, "rerror")) != NULL) {
>>>          if (type != IF_IDE && type != IF_VIRTIO && type != IF_SCSI && type != IF_NONE) {
>>>              error_report("rerror is not supported by this bus type");
>>>              return NULL;
>>>          }
>>> 
>>>          on_read_error = parse_block_error_action(buf, 1);
>>>          if (on_read_error < 0) {
>>>              return NULL;
>>>          }
>>>      }
>>> 
>>> Are you sure you don't want to check for IF_AHCI?
>> 
>> Oh? Must have missed those...
> 
> You're welcome ;)
> 
>>>> @@ -516,7 +518,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>    } else {
>>>>        /* no id supplied -> create one */
>>>>        dinfo->id = g_malloc0(32);
>>>> -        if (type == IF_IDE || type == IF_SCSI)
>>>> +        if (is_ata(type) || type == IF_SCSI)
>>>>            mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd";
>>>>        if (max_devs)
>>>>            snprintf(dinfo->id, 32, "%s%i%s%i",
>>>> @@ -546,6 +548,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>    case IF_IDE:
>>>>    case IF_SCSI:
>>>>    case IF_XEN:
>>>> +    case IF_AHCI:
>>>>    case IF_NONE:
>>>>        switch(media) {
>>>> 	case MEDIA_DISK:
>>>> @@ -628,6 +631,49 @@ err:
>>>>    return NULL;
>>>> }
>>>> 
>>>> +static void drive_populate_ahci(void)
>>>> +{
>>>> +    int bus;
>>>> +    QemuOpts *opts;
>>>> +
>>>> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
>>>> +        char devname[] = "ahciXXX";
>>>> +        int dev;
>>>> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
>>>> +
>>>> +        /* add ahci host controller */
>>>> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
>>>> +        qemu_opt_set(opts, "driver", "ich9-ahci");
>>> 
>>> Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
>>> provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
>>> ahci_init() correctly, it provides six.
>> 
>> I don't think I understand?
> 
> With "-drive if=ahci,bus=5,...", drive_get_max_bus() returns 5, and the
> loop executes six times, creating six AHCI controllers, doesn't it?
> Each of them provides six buses, for a total of 36.

How is that different from if=scsi?


Alex

Re: [Qemu-devel] [PATCH v2] ahci: add -drive support

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 12.07.2012, at 10:17, Markus Armbruster wrote:
>
>> [Alex's illegibly long lines wrapped]
>> 
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 09.07.2012, at 10:50, Markus Armbruster wrote:
>>> 
>>>> Alexander Graf <agraf@suse.de> writes:
>>>> 
>>>>> We've had support for creating AHCI devices using -device for a while now,
>>>>> but it's cumbersome to users. We really should provide an easier way for
>>>>> them to leverage the power of AHCI!
>>>>> 
>>>>> So let's introduce a new if= option to -drive, giving users the same
>>>>> command line experience as for scsi or ide.
>>>>> 
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>> 
>>>>> ---
>>>>> 
>>>>> v1 -> v2:
>>>>> 
>>>>> - support more than a single drive per adapter
>>>>> - support index= option
>>>>> - treat IF_AHCI the same as IF_IDE
>>>> 
>>>> Inhowfar?  Not obvious to me from the patch, or the diff patch v1.
>>>> 
>>>>> - add is_ata() helper to match AHCI || IDE
>>>> 
>>>> Not addressed:
>>>> 
>>>> Once we switch to q35, if=ahci will become a redundant wart: to add
>>>> drives to the board's AHCI controller, you'll have to use if=ide.
>>>> if=ahci will create new controllers, which is generally not what you
>>>> want.  Ugh.
>>>> 
>>>> 
>>>>> ---
>>>>> blockdev.c      |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>>> blockdev.h      |    7 +++++++
>>>>> qemu-options.hx |    7 ++++++-
>>>>> vl.c            |    2 ++
>>>>> 4 files changed, 65 insertions(+), 5 deletions(-)
>>>>> 
>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>> index 9e0a72a..744a886 100644
>>>>> --- a/blockdev.c
>>>>> +++ b/blockdev.c
>>>>> @@ -32,6 +32,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] = {
>>>>> @@ -51,6 +52,7 @@ static const int if_max_devs[IF_COUNT] = {
>>>>>     */
>>>>>    [IF_IDE] = 2,
>>>>>    [IF_SCSI] = 7,
>>>>> +    [IF_AHCI] = 6,
>>>>> };
>>>>> 
>>>>> /*
>>>>> @@ -330,15 +332,15 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi)
>>>>      if ((buf = qemu_opt_get(opts, "if")) != NULL) {
>>>>          for (type = 0; type < IF_COUNT && strcmp(buf, if_name[type]); type++)
>>>>              ;
>>>>          if (type == IF_COUNT) {
>>>>              error_report("unsupported bus type '%s'", buf);
>>>>              return NULL;
>>>>          }
>>>>      } else {
>>>>          type = default_to_scsi ? IF_SCSI : IF_IDE;
>>>>      }
>>>> 
>>>> A board can't default to IF_AHCI.  I guess what such a board would do is
>>>> treat IF_IDE and IF_AHCI just the same.
>>>> 
>>>> Leads me this question: what do "if=ide", "if=ahci" and "no if given"
>>>> mean?  Let me try:
>>>> 
>>>> * "if=ide" means "if the board provides an IDE controller, create an IDE
>>>> device attached to it.  What kind of IDE controller the board provides
>>>> doesn't matter.  In particular, an AHCI controller is fine.
>>> 
>>> I don't think this is what we want it to mean. What we want is:
>>> 
>>> "if=ide" means "if the board provides an IDE controller, create an IDE
>>> device attached to it. If it does not provide one, create one".
>> 
>> Okay, we got two competing ideas here.
>> 
>> 1. -drive doesn't give you any control over the kind of IDE controller.
>> You can select an IDE bus by number (bus=...), and you get whatever
>> existing controller provides this bus.  If none exists, a board-specific
>> one may be created for your convenience.  If you need more control, use
>> -device to set up controllers the way you want.
>> 
>> 2. -drive gives you control over AHCI (if=ahci) vs. "other" (if=ide).
>> IDE buses are numbered separately for if=ahci and if=ide.  With if=ahci,
>> you get an existing AHCI controller.  If none exists, a board-specific
>> one may be created for your convenience.  With if=ide, you get an
>> existing non-AHCI controller.  If none exists, a board-specific one may
>> be created for your convenience.  If you need more control, use -device
>> to set up controllers the way you want.
>> 
>> The question to answer is whether the difference between AHCI and
>> non-AHCI is so important that we want to provide control in -drive.
>> 
>> What I'd like to avoid is casual users setting up new guests with our
>> shiny new q35 board getting their IDE drives connected to some slow, old
>> controller just because they haven't discovered that if=ide doesn't cut
>> it anymore, and they need to use if=ahci now.
>> 
>>>> * "no if given" means "create a block device of the board's preferred
>>>> kind" in theory, and "default to either if=ide or if=scsi" in current
>>>> practice.
>>> 
>>> Yes. This should be ide for -M pc, scsi for -M pseries and ahci for -M
>>> q35.
>>> 
>>>> * "if=ahci" means "create an IDE device and attach it to a completely
>>>> seperate set of ich9-ahci controllers specifically created for the
>>>> "-drive if=ahci".  If the board provides an AHCI controller, it's not
>>>> used for if=ahci.  It may still be used for if=ide (depends on board).
>>> 
>>> The board should simply not create one then, no?
>> 
>> The controller is an integral part of the board.  Chipset even.  It
>> should always be created, drive or no drives.
>> 
>> Here's a meaning that's consistent with the one you proposed for if=ide:
>> if the board provides an AHCI controller, create an IDE device attached
>> to it.  If it does not provide one, create one first.
>> 
>> See, I really don't want yet another if=FOO with its own special
>> behavior.  I'd be much happier with a set of if=... that behaves pretty
>> much the same.  Ideally, all of them, but that's a tall order.  However,
>> let's not make it worse by adding new special behaviors.  I'm trying to
>> find a way to make if=ide and if=ahci behave pretty much the same.  Do
>> you agree with this goal in principle?
>
> I agree that they should behave similar within their scope, yes.
>
>> Let me refine.  If the controller identified by if and bus exists, use
>> it.  Else, if such a controller can be created, create and use it.
>> Else, fail.
>
> At the point in time when -drive is passed in, we don't know what
> controllers the board might create yet, no?

Correct.

>> if=ide is already consistent with this meaning, in a somewhat degenerate
>> form: "can be created" is always false.  Only the board creates IDE
>> controllers.  All our boards create zero or two IDE buses.  If it
>
> Sure, which renders if=ide completely useless on machines that don't
> spawn an IDE controller by default, like -M mpc8544ds.

Correct.  Having it attempt to create a controller would be nicer, and
consistent with how you want if=ahci behave (I think).

>> creates two, attempts to use more fail.  Wart: if it creates none,
>> attempts to use any are silently ignored.  Wart: if the user adds IDE
>> controllers with -device, we don't use them.

The technical reason for this behavior: -drive if=ide creates the
backends immediately, but leaves creating device models to the board
initialization function.  If the board doesn't know about IDE...

>> if=scsi isn't consistent with this meaning.  It could be made
>> consistent, but I'm not asking you do that work now.
>> 
>> Your if=ahci isn't quite consistent with this meaning, because it never
>> uses existing controllers.
>> 
>> I'm open to other ways to make if=ide and if=ahci consistent.
>
> I think we have 2 options
>
> 1) Keep the creation process 2-tiered. Parse the cmdline before
> machine create. Plug disks on after/during machine create. This can
> get ugly pretty quick.
>
> 2) Make the process be linear. Command line parsing goes first. Then
> goes drive parsing which creates -device parameters. Then machine
> creation. During the time we parse the drives we should be able to
> evaluate machine info, so we can put bits in there like "spawns the
> first 2 IDE buses itself".

I'm afraid this is a bit too terse and abstract for me to follow...

[...]
>>>>> +static void drive_populate_ahci(void)
>>>>> +{
>>>>> +    int bus;
>>>>> +    QemuOpts *opts;
>>>>> +
>>>>> +    for (bus = 0; bus <= drive_get_max_bus(IF_AHCI); bus++) {
>>>>> +        char devname[] = "ahciXXX";
>>>>> +        int dev;
>>>>> +        snprintf(devname, sizeof(devname), "ahci%d", bus);
>>>>> +
>>>>> +        /* add ahci host controller */
>>>>> +        opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL);
>>>>> +        qemu_opt_set(opts, "driver", "ich9-ahci");
>>>> 
>>>> Creates one ich9-ahci device per IDE bus.  Doesn't the ich9-ahci device
>>>> provide multiple IDE buses?  If I read pci_ich9_ahci_init() and
>>>> ahci_init() correctly, it provides six.
>>> 
>>> I don't think I understand?
>> 
>> With "-drive if=ahci,bus=5,...", drive_get_max_bus() returns 5, and the
>> loop executes six times, creating six AHCI controllers, doesn't it?
>> Each of them provides six buses, for a total of 36.
>
> How is that different from if=scsi?

First of all, if=scsi isn't exactly a shining example of how to do
things right.

But one thing it does right is not creating unnecessary controllers
unless ordered.  If you don't specify bus, unit, you get one controller
per seven -drive.  That's how it should be; the LSI controller supports
seven units.

Your patch appears to create one AHCI controller per -drive.  But
ich9-ahci has six ports, doesn't it?  Shouldn't you create one
controller per six -drive?