On 12.07.2012, at 13:09, Markus Armbruster wrote: > Alexander Graf <ag...@suse.de> writes: > >> On 12.07.2012, at 10:17, Markus Armbruster wrote: >> >>> [Alex's illegibly long lines wrapped] >>> >>> Alexander Graf <ag...@suse.de> writes: >>> >>>> On 09.07.2012, at 10:50, Markus Armbruster wrote: >>>> >>>>> Alexander Graf <ag...@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 <ag...@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