From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52237) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf611-0001oT-Ev for qemu-devel@nongnu.org; Thu, 14 Jun 2012 05:07:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sf60u-0008Co-Ip for qemu-devel@nongnu.org; Thu, 14 Jun 2012 05:06:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63965) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf60u-0008Cj-AW for qemu-devel@nongnu.org; Thu, 14 Jun 2012 05:06:48 -0400 From: Markus Armbruster References: <1339495183-29577-1-git-send-email-agraf@suse.de> <6E4CA486-F0DC-402C-A59E-743FD647CE6D@suse.de> Date: Thu, 14 Jun 2012 11:06:44 +0200 In-Reply-To: <6E4CA486-F0DC-402C-A59E-743FD647CE6D@suse.de> (Alexander Graf's message of "Thu, 14 Jun 2012 09:34:33 +0200") Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH] ahci: add -drive support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Kevin Wolf , "qemu-devel@nongnu.org Developers" Alexander Graf writes: > On 14.06.2012, at 09:29, Markus Armbruster wrote: > >> Alexander Graf 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, bumping it en par with virtio. >> >> If I understand your patch correctly, this makes if=ahci work like >> if=virtio and unlike if=ide: >> >> * if=virtio: configure a new PCI device. >> >> * if=ide: instruct the board to add an IDE device to its IDE controller. >> >> For -M pc, the board's IDE controller happens to be piix3-ide. >> >> For -M q35, I'd expect the board's IDE controller to be an ich9-ahci. >> >> Once we switch to q35, if=ahci will become a redundant wart: to add >> drives to the existing AHCI controller, you'll have to use if=ide. >> if=ahci will create a new controller, which is generally not what you >> want. Ugh. > > Yeah, I couldn't come up with anything else that's not completely ugly like the IF_SCSI implementation. Here's a non-ugly solution: finish the q35 job, and if=ide just works :) Except for old VMs that still have piix3-ide. And for those VMs an easy way to add AHCI controllers and drives isn't exactly a priority. >> A few questions inline. >> >>> Signed-off-by: Alexander Graf >>> --- >>> blockdev.c | 25 +++++++++++++++++++++++-- >>> blockdev.h | 1 + >>> qemu-options.hx | 7 ++++++- >>> 3 files changed, 30 insertions(+), 3 deletions(-) >>> >>> diff --git a/blockdev.c b/blockdev.c >>> index 622ecba..5405f6c 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] = { >>> @@ -519,7 +520,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 (type == IF_IDE || type == IF_SCSI || type == IF_AHCI) >>> mediastr = (media == MEDIA_CDROM) ? "-cd" : "-hd"; >>> if (max_devs) >>> snprintf(dinfo->id, 32, "%s%i%s%i", >>> @@ -549,6 +550,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: >>> @@ -582,6 +584,25 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) >>> default: >>> abort(); >>> } >>> + >>> + if (type == IF_AHCI) { >>> + static int ahci_bus = 0; >>> + char devname[] = "ahciXXX"; >>> + char busname[] = "ahciXXX.0"; >>> + snprintf(devname, sizeof(devname), "ahci%d", ahci_bus); >>> + snprintf(busname, sizeof(busname), "ahci%d.0", ahci_bus++); >>> + >>> + /* add ahci host controller */ >>> + opts = qemu_opts_create(qemu_find_opts("device"), devname, 0, NULL); >>> + qemu_opt_set(opts, "driver", "ich9-ahci"); >>> + >>> + /* and attach a single 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); >>> + } >>> + >> >> Doesn't this create a new ich9-ahci controller per -drive? >> >> If yes, it's problematic in practice, as you'll run out of PCI slots >> pretty darn fast. That problem made us replace virtio-blk by >> virtio-scsi. Let's not re-create it. > > Hrm. If you have a great idea on how to implement it, I'm all open for it. Talking about it from a high level perspective I had the same feelings at first. Looking at how to implement index= and bus= for real, I quickly withdrew myself from the approach. I'm afraid the sane way to do this is going to be complicated, just like if=scsi. I understand your reluctance to do that; I feel the same. That's why I'm suggesting to make the problem go away via -M q35. > The good news is that the limitation here is only a -drive if=ahci limitation. It does not apply to -device. There you can still plug up to 6 ahci devices onto a single HBA. A good convenience option covers a useful subset of common cases neatly. Your -drive if=ahci covers one common case: add the first AHCI drive (automatically adding the controller for it as well). It doesn't cover the common case of adding a second AHCI drive (to the same controller, of course). If I have to resort to -device for something as simple as that, then I wonder why it's worth having -drive if=ahci at all. >> IF_VIRTIO device option creation is done in the preceeding switch. You >> don't do that for IF_AHCI because you already do something else there: >> handling the media option. I think one switch for media and a separate >> one for device options would be clearer. > > We can do that, yeah :). > >> >>> if (!file || !*file) { >>> return dinfo; >>> } >>> @@ -604,7 +625,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) >>> ro = 1; >>> } else if (ro == 1) { >>> if (type != IF_SCSI && type != IF_VIRTIO && type != IF_FLOPPY && >>> - type != IF_NONE && type != IF_PFLASH) { >>> + type != IF_NONE && type != IF_PFLASH && type != IF_AHCI) { >>> error_report("readonly not supported by this bus type"); >>> goto err; >>> } >> >> Are you sure AHCI can handle read-only? > > Isn't read-only handled in generic ATA code? I think it is. But treating IF_AHCI and IF_IDE differently here is confusing, isn't it? >> [...] >>> diff --git a/blockdev.h b/blockdev.h >>> index 260e16b..e14c1d5 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; >>> >>> 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 >>> + >> >> If I'm reading drive_init() correctly, if=ahci doesn't attach a disk, it >> creates a controller with a disk. So this is somewhat misleading. > > It creates an HBA with a disk, yes. Which for almost everyone is the same thing as attaching a disk. People who want to create more sophisticated setups are still free to use the complex -device based syntax :). I think we'd owe users a fair warning that if=ahci does something rather unexpected, namely adding a controller.