[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Question about xen disk unplug support for ahci missed
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu |
Date: |
Tue, 20 Oct 2015 15:52:43 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Tue, 20 Oct 2015, Laszlo Ersek wrote:
> On 10/20/15 13:59, Stefano Stabellini wrote:
> > On Mon, 19 Oct 2015, Laszlo Ersek wrote:
> >> 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.
Good idea
> (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.
The patch works, but you are right, the analysis of the problem was
wrong: it gets stuck in XenPvBlkWaitForBackendState, rather than
returning an error.
> 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".
I'll rework the patch and send it to the list.
Thanks,
Stefano
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, (continued)
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Anthony PERARD, 2015/10/15
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Fabio Fantoni, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Anthony PERARD, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Fabio Fantoni, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Stefano Stabellini, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Fabio Fantoni, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Laszlo Ersek, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Laszlo Ersek, 2015/10/19
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Stefano Stabellini, 2015/10/20
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Laszlo Ersek, 2015/10/20
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu,
Stefano Stabellini <=
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Fabio Fantoni, 2015/10/15
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Paul Durrant, 2015/10/14
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Laszlo Ersek, 2015/10/15
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Kevin Wolf, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Paul Durrant, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Kevin Wolf, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Paul Durrant, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Kevin Wolf, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Paul Durrant, 2015/10/16
- Re: [Qemu-devel] Question about xen disk unplug support for ahci missed in qemu, Kevin Wolf, 2015/10/16