Alexander Graf <ag...@suse.de> writes: > 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.
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. [...]