From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46278) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJlmG-0004Fo-C9 for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:56:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJlmA-0000bS-5u for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:56:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:2198) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJlm9-0000bL-Ti for qemu-devel@nongnu.org; Tue, 19 Aug 2014 11:56:46 -0400 Message-ID: <53F373B7.9000706@redhat.com> Date: Tue, 19 Aug 2014 11:56: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> In-Reply-To: <87ppfwaoqr.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 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 no= t >> 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 core > which boards (not just pc_piix) use to find the if=3Dide drives. It fi= lls > in an array of DriveInfo. > > pci_ide_create_devs() is a helper function in the IDE PCI code which PC= I > 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=20 patch up the other boards if I changed this function! Only an RFC before=20 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 think >> that since the if=3Dide,file=3D... shorthand never worked to begin wit= h, >> the impact might actually be minimal. >> >> But since the legacy IDE interface of the ICH9 is as of yet unemulated= , >> the if=3Dide drives don't have a reasonable place to go yet. I am also >> 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 before > you got involved :) > > The problem at hand is that "-drive if=3Dide" and its sugared forms -hd= a, > -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=20 initialization should be easy enough, if we decide that "ide means=20 ISA/PCI, ahci means the AHCI HBA." I could probably even write one=20 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=20 of the unsugared form being unsolved depends on me (well, or someone)=20 implementing the legacy IDE interface for Q35. > 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 like > 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, not > AHCI. Or maybe both, depending on how we do it. I think I am leaning towards this viewpoint, but it depends on what=20 "interface" means in QEMU. Currently, the number of units per bus is=20 tied to the "interface" and clearly the AHCI SATA interface only=20 supports one per bus, so semantically this makes sense. I think the real ICH9 AHCI device supports only fully AHCI or fully=20 legacy, but the AHCI spec itself appears to allow you to run a=20 mixed-mode device. I am not sure we have a usage case for mixed-mode, so enforcing=20 either/or for the AHCI device makes sense for now, I think. > The other point of view is that IDE and AHCI are no more different than > 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=20 think there's any /other/ reason I needed IF_AHCI, of course, I wasn't=20 around for the previous discussions, so maybe there are other reasons I=20 am not aware of. > 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 mak= e > it less silly, but had to be reverted in 27d6bf4 because the silliness > was ABI. > > The mapping is also silly for ich9-ahci. You side-step that silliness > 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=20 (not the board) to override. Does that make more sense? If we allow=20 people to wire up an AHCI device to piix, we'll run back into the same=20 problems of the bus/unit mappings unless we make this a device property. I do feel like I'd rather just make it an interface property and have=20 people specify which type of bus they want to wire it up to, but that=20 does create a lot of disparity against the SCSI devices. > [*] Currently, they're silently ignored with most boards for most X, bu= t > I regard that as implementation defect. Yes. Is there a bool in the drive info array that we can set to say=20 "this drive has been added as a device" and check for any that went=20 unset? I can add one and a routine to check for it, which may help flush=20 out more of the weird legacy sugar option bugs. (Sugar attracts bugs. heh-heh-heh...) --=20 =97js