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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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
> <edk2-devel@lists.01.org>. (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