From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48822) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJodL-00050A-MA for qemu-devel@nongnu.org; Tue, 19 Aug 2014 14:59:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJodF-00064c-CK for qemu-devel@nongnu.org; Tue, 19 Aug 2014 14:59:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:5925) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJodF-00064J-5D for qemu-devel@nongnu.org; Tue, 19 Aug 2014 14:59:45 -0400 Message-ID: <53F39E9B.2050200@redhat.com> Date: Tue, 19 Aug 2014 14:59:39 -0400 From: John Snow MIME-Version: 1.0 References: <1408399520-12778-1-git-send-email-jsnow@redhat.com> <87ppfwaoqr.fsf@blackfin.pond.sub.org> <53F373B7.9000706@redhat.com> <87bnrgjqss.fsf@blackfin.pond.sub.org> In-Reply-To: <87bnrgjqss.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 0/4] Adding -cdrom, -hd[abcd] and -drive file=... to Q35 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, agraf@suse.de On 08/19/2014 02:08 PM, Markus Armbruster wrote: > John Snow writes: > >> On 08/19/2014 04:05 AM, Markus Armbruster wrote: >>> John Snow writes: >>> >>>> Currently, the drive definitions created by drive_new() when using >>>> the -drive file=3D...[,if=3Dide] or -cdrom or -hd[abcd] options are = not >>>> picked up by the Q35 initialization routine. >>>> >>>> To fix this, we have to add hooks to search for these drives using >>>> something like pc_piix's ide_drive_get and then add them using >>>> something like pci_ide_create_devs. >>> >>> ide_drive_get() isn't pc_piix's, it's a helper function in the IDE co= re >>> which boards (not just pc_piix) use to find the if=3Dide drives. It = fills >>> in an array of DriveInfo. >>> >>> pci_ide_create_devs() is a helper function in the IDE PCI code which = PCI >>> IDE controllers (not just piix3-ide) use to create IDE devices for an >>> array of DriveInfo. >> >> Yes. I meant to say pc_piix's /call to/ ide_drive_get. I would have to >> patch up the other boards if I changed this function! Only an RFC >> before I got too far down this path :] >> >>>> Where it gets slightly wonky is the fact that if=3Dide is specified >>>> to use two devices per bus, whereas AHCI does not utilize that >>>> same master/slave mechanic. Therefore, many places inside of >>>> blockdev.c where we add and define new drives use incorrect math >>>> for AHCI devices and try to place them on impossible buses. >>>> Notably -hdb and -hdd would become inaccessible. >>> >>> Yes. >>> >>>> To remedy this, I added a new interface type, IF_AHCI. Corresponding >>>> to this change, I modified the default machine properties for Q35 >>>> to use this interface as a default. >>>> >>>> The changes appear to work well, but where I'd like some feedback >>>> is what should happen if people do something like: >>>> >>>> qemu -M q35 -drive if=3Dide,file=3Dfedora.qcow2 >>>> >>>> The code as presented here is not going to look for or attempt to >>>> connect IDE devices, because it is now looking for /AHCI/ devices. >>>> >>>> At worst, this may break a few existing scripts, but I actually thin= k >>>> that since the if=3Dide,file=3D... shorthand never worked to begin w= ith, >>>> the impact might actually be minimal. >>>> >>>> But since the legacy IDE interface of the ICH9 is as of yet unemulat= ed, >>>> the if=3Dide drives don't have a reasonable place to go yet. I am al= so >>>> not sure what a reasonable way to handle people specifying BOTH >>>> if=3Dide and if=3Dahci drives would be. >>> >>> We've been through IF_AHCI before, more than once, but that was befor= e >>> you got involved :) >>> >>> The problem at hand is that "-drive if=3Dide" and its sugared forms -= hda, >>> -hdb, -cdrom, ... don't work with Q35. >>> >>> You provide a solution for the sugared forms, you leave the problem >>> unsolved for the unsugared form, and you add a new problem: -drive >>> if=3Dahci doesn't work with i440FX. Progress, sort of. >> >> Adding a call to boards that support the AHCI device during their >> initialization should be easy enough, if we decide that "ide means >> ISA/PCI, ahci means the AHCI HBA." I could probably even write one >> generic routine between i440fx and q35 to do both IDE/AHCI. >> >> If we decide that IF_IDE and IF_AHCI mean different things, the >> problem of the unsugared form being unsolved depends on me (well, or >> someone) implementing the legacy IDE interface for Q35. > > Let me come back to this further down. > >>> Let's take a step back, and recap previous discussion. There are two >>> defensible point of views, in my opinion. >>> >>> One is that IDE and AHCI should be separate interface types, just lik= e >>> IDE and SCSI are. >>> >>> Attempts to define an if=3DX drive with a board that doesn't provide = a >>> controller for X should fail[*]. Only onboard controllers matter, >>> add-ons plugged in with -device don't. An i440FX board provides only >>> IDE. A Q35 board provides only AHCI, not IDE. If we implement an >>> ich9-ahci legacy mode, and switch it on, then it provides only IDE, n= ot >>> AHCI. Or maybe both, depending on how we do it. >> >> I think I am leaning towards this viewpoint, but it depends on what >> "interface" means in QEMU. Currently, the number of units per bus is >> tied to the "interface" and clearly the AHCI SATA interface only >> supports one per bus, so semantically this makes sense. > > An index <-> (bus, unit) mapping doesn't make an interface! Yes, it's > tightly coupled to the interface, but that became wrong way back when w= e > went beyond 8-bit SCSI HBAs, long before we added up AHCI HBAs. "Interface" seems nebulous in QEMU. In the physical world, there=20 definitely is a difference between IDE/EIDE/PATA and SATA/AHCI.=20 Different physical and link layers -- the verbs stayed the same. What I=20 am getting at is that I am not sure what an "interface" is /supposed/ to=20 encompass here in QEMU. Underlying device type? If that's the case, then=20 IDE/AHCI are definitely identical. >> I think the real ICH9 AHCI device supports only fully AHCI or fully >> legacy, but the AHCI spec itself appears to allow you to run a >> mixed-mode device. >> >> I am not sure we have a usage case for mixed-mode, so enforcing >> either/or for the AHCI device makes sense for now, I think. > > I can't see a use for mixed mode, either. > >>> The other point of view is that IDE and AHCI are no more different th= an >>> the different kinds of SCSI HBAs. This is certainly true from a qdev >>> point of view: just like SCSI devices can connect to any SCSI qbus, >>> regardless of the HBA providing it, so can IDE devices connect to any >>> IDE qbus, regardless of the controller providing it. >> >> Yes... Really the only difference are some mapping semantics. I don't >> think there's any /other/ reason I needed IF_AHCI, of course, I wasn't >> around for the previous discussions, so maybe there are other reasons >> I am not aware of. > > I've always been in the "we don't need or want if=3Dahci" camp :) > > The one argument for if=3Dahci I found convincing was a desire for Q35 > with its ICH9 in legacy mode. And that's just as easily done with a > machine option. Personally, I find that more natural. So piix would always try to add to the PCI IDE bus, and Q35 would always=20 try to add to the AHCI bus. With a machine option for Q35, we could tell it to use the AHCI device=20 in legacy mode and add to that device accordingly. Do we care about the case for adding an AHCI device to piix? Do we=20 change the behavior of what bus we use by a machine option (like a=20 default_hba machine property and hba_type drive properties?), or by the=20 presence of an AHCI device if someone adds one? As it stands now, using if=3Dahci or if=3Dide is a pretty strong hint for= =20 what bus to look for and add to; though it is inflexible between command=20 invocations that use different HBAs. >>> There's a wrinkle: the mapping between index to (bus, unit). This >>> mapping is ABI. The current mapping makes sense for the first >>> generation of controllers: PATA (two devices per bus, thus >>> if_max_devs[IF_IDE] =3D 2), and 8-bit SCSI (seven per bus, thus >>> if_max_devs[IF_SCSI =3D 7). >>> >>> The mapping is silly for newer SCSI HBAs. Commit 622b520f tried to m= ake >>> it less silly, but had to be reverted in 27d6bf4 because the sillines= s >>> was ABI. >>> >>> The mapping is also silly for ich9-ahci. You side-step that sillines= s >>> only, by adding a new interface type for it. But shouldn't we add a >>> number of SCSI interface types then, too? Where does that end? >>> >>> Can we do better? I think we can, by making this part of the ABI >>> board-specific. The general form of the mapping remains >>> >>> (bus, unit) =3D (index / N, index % N) >>> >>> but N now depends on board and interface type, not just the latter. >>> >>> If the board connects if=3Dscsi to an lsi53c895a, then N =3D 7. >>> >>> If the board connects if=3Dide to an piix3-ide, then N =3D 2. >>> >>> If the board connects if=3Dide to an ich9-ahci, then N =3D 1. >>> >>> I trust you get the idea :) >> >> I suppose we could make it something like: >> if (HBA.max_units > 0) { >> N :=3D min(HBA.max_units, IF.max_units); >> } else { >> N :=3D IF.max_units; >> } >> (bus, unit) :=3D (index / N, index % N); >> >> Which sets a default property for the interface but allows the device >> (not the board) to override. Does that make more sense? If we allow >> people to wire up an AHCI device to piix, we'll run back into the same >> problems of the bus/unit mappings unless we make this a device >> property. > > Yes, it is a property of the device (property not in the qdev sense). > > Why would HBA.max_units ever be greater than IF.max_units? > > If the answer is "only if somebody screwed up the HBA device model", > then the above can be simplified to just N =3D HBA.max_units. Oh, yeah. I did Web UI programming for a while during college. Not=20 trusting plugin values is a side effect! I might pepper in an assertion=20 to make it clear that you can't bump the number of units /up/, though.=20 That's a smarter thing to do. >> I do feel like I'd rather just make it an interface property and have >> people specify which type of bus they want to wire it up to, but that >> does create a lot of disparity against the SCSI devices. > > What do you mean by "interface property"? A logical property of the interface specification; what QEMU and this=20 patch does now. >>> [*] Currently, they're silently ignored with most boards for most X, = but >>> I regard that as implementation defect. >> >> Yes. Is there a bool in the drive info array that we can set to say >> "this drive has been added as a device" and check for any that went >> unset? > > Not yet :) > >> I can add one and a routine to check for it, which may help >> flush out more of the weird legacy sugar option bugs. > > We do something like that for -netdev: > > $ qemu -nodefaults -display none -netdev user,id=3Dfoo > Warning: netdev foo has no peer > > and -net: > > $ qemu -nodefaults -display none -net user,id=3Dfoo > Warning: vlan 0 with no nics > > I think as long as we leave picking up configuration to boards, having > the boards mark the pieces they pick up is the best we can do. > > An alternative to leaving it to boards is making the boads define > callbacks that get fed configuration. But that's more surgery. > > There's more than just -netdev, -net and -drive, though. Many command > line options to configure devices also merely create a piece of > configuration for boards to pick up: > > * Character devices (-serial, -parallel) end up in serial_hds[], > parallel_hds[]. > > * Graphics devices (-vga) end up in vga_interface_type. > > These all need the same "did the board pick it up?" check as -drive. > > Some old options have been converted to work independent of boards: > > * Virtio consoles (-virtioconsole) in commit 98b1925. > > * Sound devices (-soundhw) in commit b3e6d59. > > Newer convenience options should always worked this way. -watchdog > does. > > I may have missed options. That is indeed more than a few. I probably need a little bit more=20 exposure to different boards and how configuration works before I attack=20 the problem as a whole. I may just fix -drive for now ... > >> (Sugar attracts bugs. heh-heh-heh...) > > Indeed! > Your position is pretty clear. I will give other people the time to=20 chime in before I do too much more work on this, though I will probably=20 go ahead and add the unused drive check now, since we'll want that no=20 matter which path we take. --=20 =97js