From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33959) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1yQy-0004SO-Q2 for qemu-devel@nongnu.org; Mon, 08 Jun 2015 10:53:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1yQt-0002wS-Uo for qemu-devel@nongnu.org; Mon, 08 Jun 2015 10:53:52 -0400 Received: from mail-ie0-f178.google.com ([209.85.223.178]:34594) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1yQt-0002wE-Ov for qemu-devel@nongnu.org; Mon, 08 Jun 2015 10:53:47 -0400 Received: by iebmu5 with SMTP id mu5so64463849ieb.1 for ; Mon, 08 Jun 2015 07:53:47 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87a8wafflw.fsf@blackfin.pond.sub.org> References: <1433434853-19229-1-git-send-email-peter.maydell@linaro.org> <87a8wafflw.fsf@blackfin.pond.sub.org> From: Peter Maydell Date: Mon, 8 Jun 2015 15:53:26 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 0/4] blockdev: Defer creation of implicit PCI devices for IF_VIRTIO drives List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , qemu-block@nongnu.org, Patch Tracking , Alexander Graf , QEMU Developers , Christian Borntraeger , Cornelia Huck On 8 June 2015 at 15:18, Markus Armbruster wrote: > Peter Maydell writes: > >> blockdev.c will create implicit virtio-blk-* devices for IF_VIRTIO >> drives. I want to turn this on for the ARM virt board (now it has PCI), >> so that users can use shorter and more comprehensible command lines. > > I had to read further until understood your goal: you want to make > -drive default to if=virtio for this machine type. It currently > defaults to if=ide, but the board doesn't pick those up. > >> Unfortunately, the code as it stands will always create an implicit >> device, which means that setting the virt block_default_type to IF_VIRTIO >> would break existing command lines like: >> >> -drive file=arm-wheezy.qcow2,id=foo >> -device virtio-blk-pci,drive=foo > > This works basically by accident. The documented way to do it is -drive > if=none. Omitting if= like you do defaults to the board's > block_default_type, in this case if=ide. Yes, but since we didn't reject these command lines, they worked anyway. (We should ideally have rejected them as "hey you asked for an IDE drive but it wasn't connected to anything".) So we should avoid breaking them now. > Since the drive remains unconnected, -device using it succeeds, even > though if=ide. -device should really require if=none. But since it has > never bothered to, this misuse may have hardened into de facto ABI. > > Same for any block_default_type other than IF_NONE. Indeed. >> This patchset fixes this problem by deferring the creation of the >> implicit devices to drive_check_orphaned(), which means we can do >> it only for the drives which haven't been explicitly connected to >> a device by the user. > > This changes QEMU from rejecting a certain pattern of incorrect usage to > guessing what the user meant. Example: > > $ qemu-system-x86_64 -nodefaults -display none -drive if=virtio,file=tmp.qcow2,id=foo -device ide-hd,drive=foo > > The user asks for the drive to be both a virtio and an IDE device, which > is of course nonsense. > > Before your patch, we reject the nonsense: > > qemu-system-x86_64: -drive if=virtio,file=tmp.qcow2,id=foo: Property 'virtio-blk-device.drive' can't take value 'foo', it's in use > > Afterwards, we accept it, guessing the user really meant -device ide=hd, > and not if=virtio. > > But if you try the same with if=scsi, we still reject it. > > Is this really a good idea? We're only rejecting it by accident, I think, because the PC machine happens to support SCSI. If you remove the pc_pci_device_init() code in pc.c which creates SCSI adaptors, then we will happily accept the command line and plug the if=scsi drive into the IDE adaptor. Similarly, the pc machine will currently accept the combination -drive if=sd,...,id=foo -device ide=hd,drive=foo If we care about catching mismatched drive-types and devices, we should actually check that, rather than rejecting them if the machine supports devices for that drive type and ignoring the mismatch if it does not. >> The slight downside of deferral is that it is effectively rearranging >> the order in which the devices are created, which means they will >> end up in different PCI slots, etc. We can get away with this for PCI, >> because at the moment the only machines which set their block_default_type >> to IF_VIRTIO are the S390X ones. I have special-cased S390X to retain >> the old behaviour, which is a bit ugly but functional. If S390X doesn't >> care about cross-version migration we can probably drop that and >> just have everything defer. S390X people? >> >> The code in master didn't seem to take much account of the possibility >> of hotplug -- if the user created a drive via the monitor > > Full monitor command, please? I haven't tested creating anything via the monitor. I merely noted that the other code path calling drive_new() is via hmp_drive_add(). (I notice now that that will actually fail on anything other than an IF_NONE drive, so it's just as well we weren't creating devices for IF_VIRTIO, or we'd probably have leaked them.) >> we would >> apparently try to create the implicit drive, but in fact not do so >> because vl.c had already done device creation long before. I've included >> a patch that makes it more explicit that hotplug does not get you the >> magic implicit devices. > > PATCH 2/4. Haven't thought through its implications. It's not supposed to have implications, it's supposed to be a "no behaviour change" patch :-) Without the s390 special casing patches 2 and 3 collapse down into a single patch, incidentally. -- PMM