Re: [Qemu-block] [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu

[Laszlo Ersek]

On 10/20/15 13:59, Stefano Stabellini wrote:
> On Mon, 19 Oct 2015, Laszlo Ersek wrote:
>> On 10/16/15 21:09, Laszlo Ersek wrote:
>>> On 10/16/15 13:34, Fabio Fantoni wrote:
>>>> Il 16/10/2015 12:47, Stefano Stabellini ha scritto:
>>>>> On Fri, 16 Oct 2015, Fabio Fantoni wrote:
>>>>>> Il 16/10/2015 12:13, Anthony PERARD ha scritto:
>>>>>>> On Fri, Oct 16, 2015 at 10:32:44AM +0200, Fabio Fantoni wrote:
>>>>>>>> Il 15/10/2015 20:02, Anthony PERARD ha scritto:
>>>>>>>>> On Thu, Oct 15, 2015 at 06:27:17PM +0200, Fabio Fantoni wrote:
>>>>>>>>>> Il 14/10/2015 13:06, Stefano Stabellini ha scritto:
>>>>>>>>>>> I would suggest Fabio to avoid AHCI disks altogether and just use
>>>>>>>>>>> OVMF
>>>>>>>>>>> with PV disks only and Anthony's patch to libxl to avoid creating
>>>>>>>>>>> any
>>>>>>>>>>> IDE disks: http://marc.info/?l=xen-devel&m=144482080812353.
>>>>>>>>>>>
>>>>>>>>>>> Would that work for you?
>>>>>>>>>> Thanks for the advice, I tried it:
>>>>>>>>>> https://github.com/Fantu/Xen/commits/rebase/m2r-testing-4.6
>>>>>>>>>>
>>>>>>>>>> I installed W10 pro 64 bit with ide disk, installed the win pv
>>>>>>>>>> drivers
>>>>>>>>>> and
>>>>>>>>>> after changed to xvdX instead hdX, is the only change needed, right?
>>>>>>>>>> Initial boot is ok (ovmf part about pv disks seems ok) but windows
>>>>>>>>>> boot
>>>>>>>>>> fails with problem with pv drivers.
>>>>>>>>>> In attachment full qemu log with xen_platform trace and domU's xl
>>>>>>>>>> cfg.
>>>>>>>>>>
>>>>>>>>>> Someone have windows domUs with ovmf and pv disks only working?
>>>>>>>>>> If yes
>>>>>>>>>> can
>>>>>>>>>> tell me the difference to understand what can be the problem please?
>>>>>>>>> When I worked on the PV disk implementation in OVMF, I was able to
>>>>>>>>> boot
>>>>>>>>> a Windows 8 with pv disk only.
>>>>>>>>>
>>>>>>>>> I don't have access to the guest configuration I was using, but I
>>>>>>>>> think
>>>>>>>>> one
>>>>>>>>> difference would be the viridian setting, I'm pretty sure I did
>>>>>>>>> not set
>>>>>>>>> it.
>>>>>>>>>
>>>>>>>> I tried with viridian disabled but did the same thing, looking
>>>>>>>> cdrom as
>>>>>>>> latest thing before xenbug trace in qemu log I tried also to remove
>>>>>>>> it but
>>>>>>>> also in this case don't boot correctly, full qemu log in attachment.
>>>>>>>> I don't know if is a ovmf thing to improve (like what seems in
>>>>>>>> Laszlo and
>>>>>>>> Kevin mails) or xen winpv drivers unexpected case, have you tried also
>>>>>>>> with
>>>>>>>> latest winpv builds? (for exclude regression)
>>>>>>> No, I did not tried the latest winpv drivers.
>>>>>>>
>>>>>>> Sorry I can help much more that that. When I install this win8 guest
>>>>>>> tried
>>>>>>> to boot it with pv drivers only, that was more than a year ago. I
>>>>>>> have not
>>>>>>> check if it's still working. (Also I can not try anything more recent,
>>>>>>> right now.)
>>>>>>>
>>>>>> I did many other tests, retrying with ide first boot working but show pv
>>>>>> devices not working, I did another reboot (with ide) and pv devices was
>>>>>> working, after I retried with pv (xvdX) and boot correctly.
>>>>>> After other tests I found that with empty cdrom device (required for xl
>>>>>> cd-insert/cd-eject) boot stop at start (tianocore image), same result
>>>>>> with ide
>>>>>> instead.
>>>>>>  From xl cfg:
>>>>>> disk=['/mnt/vm/disks/W10UEFI.disk1.cow-sn1,qcow2,xvda,rw',',raw,xvdb,ro,cdrom']
>>>>>>
>>>>>> With seabios domU boot also with empty cdrom.
>>>>>> In qemu log I found only these that can be related:
>>>>>>> xen be: qdisk-51728: error: Could not open image: No such file or
>>>>>>> directory
>>>>>>> xen be: qdisk-51728: initialise() failed
>>>>>> And latest xl dmesg line is:
>>>>>>> (d1) Invoking OVMF ...
>>>>>> If you need more informations/test tell me and I'll post them.
>>>>> Are you saying that without any cdrom drives, it works correctly?
>>>> Yes, I did also another test to be sure, starting with ide, installing
>>>> windows, the pv drivers, rebooting 2 times (with one at boot of time
>>>> boot with ide only and without net and disks pv drivers working) and
>>>> after rebooting with pv disks (xvdX) works.
>>>> With cdrom not empty (with iso) works, with empty not, tried with both
>>>> ide (hdX) and pv (xvdX).
>>>> Empty cdrom not working with ovmf I suppose is ovmf bug or inexpected case.
>>>> About major of winpv drivers problem at boot I suppose can be solved
>>>> improving ovmf and winpv driver removing bad hybrid thing actually, but
>>>> I have too low knowledge to be sure.
>>>> About the problem of pv start after install that requiring at least 2
>>>> reboot can be also a windows 10 problem (only a suppose).
>>>>
>>>> About empty cdrom with ovmf can be solved please?
>>>>
>>>
>>> Sorry, I find your problem report impenetrable. :( Please slow down and
>>> try to spend time on punctuation at least.
>>>
>>> For me to make heads or tails of this, I'll need the following:
>>>
>>> - The debug output of an OVMF binary built with the DEBUG_VERBOSE bit
>>> (0x00400000) enabled in PcdDebugPrintErrorLevel, in addition to the
>>> default setting.
>>>
>>> - Preferably, I'll need two logs, one for the "working" case, and
>>> another for the "non-working" case.
>>>
>>> - A description of the virtual hardware (disks etc) that is
>>> understandable to someone who hasn't booted Xen in several years; for
>>> both cases above.
>>>
>>> - Please try to make an exact, itemized list of the steps you perform
>>> before executing the successful vs. failing test.
>>>
>>> - It would also help if you entered the UEFI shell in both the
>>> successful and the failing case, and ran the MAP command. The output can
>>> be snarfed from the virtual serial port too.
>>>
>>> - another command to run from the UEFI shell, in both cases:
>>>
>>>   dh -d -v -p SimpleFileSystem
>>
>> Okay, despite none of the info having surfaced that I asked for, John
>> and I continued discussing this.
> 
> Many thanks for spending time on this
> 
> 
>> >From the vague memories of a RHEL-5 Xen dom0 bug (blkback) that I fixed
>> on Aug 19 *2011*, I now managed to remember the VDISK_CDROM flag, and
>> that we had had many issues with CD-ROMs.
>>
>> So I looked at the current guest kernel driver now,
>> "drivers/block/xen-blkfront.c", to see if there were any CD-ROM related
>> "hacks" in place still. Apparently so; please consider the following
>> "git blame" snippet (excuse me for the long lines), from the
>> blkfront_probe() function:
>>
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1427)  if 
>> (xen_hvm_domain()) {
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1428)          
>> char *type;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1429)          
>> int len;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1430)          
>> /* no unplug has been done: do not hook devices != xen vbds */
>> 51c71a3b (Konrad Rzeszutek Wilk     2013-11-26 15:05:40 -0500 1431)          
>> if (xen_has_pv_and_legacy_disk_devices()) {
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1432)          
>>         int major;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1433) 
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1434)          
>>         if (!VDEV_IS_EXTENDED(vdevice))
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1435)          
>>                 major = BLKIF_MAJOR(vdevice);
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1436)          
>>         else
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1437)          
>>                 major = XENVBD_MAJOR;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1438) 
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1439)          
>>         if (major != XENVBD_MAJOR) {
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1440)          
>>                 printk(KERN_INFO
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1441)          
>>                                 "%s: HVM does not support vbd %d as xen 
>> block device\n",
>> 02f1f217 (Rasmus Villemoes          2015-02-12 15:01:31 -0800 1442)          
>>                                 __func__, vdevice);
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1443)          
>>                 return -ENODEV;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1444)          
>>         }
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1445)          
>> }
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1446)          
>> /* do not create a PV cdrom device if we are an HVM guest */
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1447)          
>> type = xenbus_read(XBT_NIL, dev->nodename, "device-type", &len);
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1448)          
>> if (IS_ERR(type))
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1449)          
>>         return -ENODEV;
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1450)          
>> if (strncmp(type, "cdrom", 5) == 0) {
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1451)          
>>         kfree(type);
>> c1c5413a (Stefano Stabellini        2010-05-14 12:44:30 +0100 1452)          
>>         return -ENODEV;
>> c1c5413a (Stefano Stabellini        2010-05-14 12:44:30 +0100 1453)          
>> }
>> b98a409b (Stefano Stabellini        2010-07-29 14:53:16 +0100 1454)          
>> kfree(type);
>> c1c5413a (Stefano Stabellini        2010-05-14 12:44:30 +0100 1455)  }
> 
> /me hides in a corner
> 
> 
>> Side point: commit 51c71a3b from Konrad
>> <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=51c71a3b>
>> should be instrumental for those who are looking for more background on
>> emulated device / driver unplugging in Xen domUs.
>>
>> But, the really interesting bit here is the commit message of b98a409b
>> <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b98a409b>,
>> from 2010, authored by Stefano:
>>
>>     blkfront: do not create a PV cdrom device if xen_hvm_guest
>>
>>     It is not possible to unplug emulated cdrom devices, and PV cdroms don't
>>     handle media insert, eject and stream, so we are better off disabling PV
>>     cdroms when running as a Xen HVM guest.
> 
> I think it all comes from the way PV drivers for HVM guests were
> originally written: the cdrom device was emulated rather than
> paravirtualized. So the unplug protocol in QEMU was implemented like
> this:
> 
> int pci_piix3_xen_ide_unplug(DeviceState *dev)
> {
>     PCIIDEState *pci_ide;
>     DriveInfo *di;
>     int i;
>     IDEDevice *idedev;
> 
>     pci_ide = PCI_IDE(dev);
> 
>     for (i = 0; i < 4; i++) {
>         di = drive_get_by_index(IF_IDE, i);
>         if (di != NULL && !di->media_cd) {
>             <unplug>
> 
> In particular see the !di->media_cd. As a consequence cdroms are left
> emulated.  Given that they cannot be unplugged and given that the PV
> block protocol is lacking in terms of cdrom-like functionalities, I
> disabled blkfront in Linux for PV interfaces corresponding to cdrom
> devices. But it could work.
> 
> 
>> Could that be related to the issue you are experiencing with OVMF?
>> Because, OVMF implies HVM (or PV-on-HVM), and your report ("empty
>> paravirt CD-ROM" or some such -- sorry, the report remains unclear)
>> appears to match the above message.
>>
>> Given that this is guest code, shouldn't the same logic be mirrored in
>> the OVMF guest driver?
>>
>> /* do not create a PV cdrom device if we are an HVM guest */
>>
>> In other words, given that OVMF implies HVM, shouldn't OVMF too forego
>> driving a paravirt CD-ROM entirely?
> 
> In the case of OVMF I think we can use the PV block interface to access
> the cdrom, the problem is just that it cannot handle empty cdrom drives
> at the moment and XenPvBlockFrontInitialization simply returns an error.

(*)

> A simple patch like this one should prevent OVMF from getting stuck with
> an error when an empty cdrom is found:
> 
> diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c 
> b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> index 256ac55..ae7cab9 100644
> --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> @@ -186,6 +186,10 @@ XenPvBlockFrontInitialization (
>    }
>    FreePool (DeviceType);
>  
> +  Status = XenBusReadUint64 (XenBusIo, "sectors", TRUE, &Value);
> +  if (Dev->MediaInfo.CdRom && Status != XENSTORE_STATUS_SUCCESS)
> +     return EFI_NO_MEDIA;
> +
>    Status = XenBusReadUint64 (XenBusIo, "backend-id", FALSE, &Value);
>    if (Status != XENSTORE_STATUS_SUCCESS || Value > MAX_UINT16) {
>      DEBUG ((EFI_D_ERROR, "XenPvBlk: Failed to get backend-id (%d)\n",

(1) Directly returning at that point will leak "Dev". I think you should
set Status, and then goto Error. XenPvBlockFree() under that label will
free Dev.

(2) I agree that returning an error code here will propagate through
XenPvBlkDxeDriverBindingStart() to the caller, and ultimately prevent
the binding. That's probably the right thing to do.

But, how does it differ from what you wrote above (*):

> [...] the problem is just that it cannot handle empty cdrom drives
> at the moment and XenPvBlockFrontInitialization simply returns an
> error.

If XenPvBlockFrontInitialization() simply returned an error right now,
then that would achieve the exact same thing as your proposal -- the
driver wouldn't bind the device. So either your idea wouldn't make a
difference, or your analysis that XenPvBlockFrontInitialization()
currently fails is incorrect.

I think it's the latter though, and that this patch should be tested.

If it works in your testing, please submit it to
<address@hidden>. (You have to be subscribed to post, sorry
about that.) Please fix the leak (1), and add the following line to the
commit message just before your signoff:

Contributed-under: TianoCore Contribution Agreement 1.0

What it means is explained in "OvmfPkg/Contributions.txt".

Thank you!
Laszlo

> 
> 
>> Now, the Xen guest code in OVMF, from
>> <https://github.com/tianocore/edk2/commit/5cce8524>, function
>> XenPvBlockFrontInitialization(), *does* look for "cdrom" in
>> "device-type":
>>
>>   XenBusIo->XsRead (XenBusIo, XST_NIL, "device-type", (VOID**)&DeviceType);
>>   if (AsciiStrCmp (DeviceType, "cdrom") == 0) {
>>     Dev->MediaInfo.CdRom = TRUE;
>>   } else {
>>     Dev->MediaInfo.CdRom = FALSE;
>>   }
>>
>> but the *only* thing that the CdRom field is used for is to force a 2KB
>> block size in XenPvBlkDxeDriverBindingStart(), for ElTorito
>> compatibility:
>>
>>   if (Dev->MediaInfo.CdRom) {
>>     //
>>     // If it's a cdrom, the blocksize value need to be 2048 for OVMF to
>>     // recognize it as a cdrom:
>>     //    MdeModulePkg/Universal/Disk/PartitionDxe/ElTorito.c
>>     //
>>     Media->BlockSize = 2048;
>>     Media->LastBlock = DivU64x32 (Dev->MediaInfo.Sectors,
>>                                   Media->BlockSize / 
>> Dev->MediaInfo.SectorSize) - 1;
>>   } else {
>>
>> While in the same situation (i.e., in an HVM guest), the guest kernel
>> driver does not report a paravirt CD-ROM at all; instead it forces an
>> emulated (IDE) CD-ROM.
>>
>> ... Sorry if the above turns out fully irrelevant, but I still have no
>> info from you to go on.
> 
> Actually it was helpful, thank you.
>