Re: [Xen-devel] [edk2] [PATCH v2] OvmfPkg: XenPvBlkDxe: handle empty cdrom drives

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 21 Oct 2015, Fabio Fantoni wrote:
> Il 21/10/2015 14:45, Laszlo Ersek ha scritto:
> > On 10/21/15 13:39, Stefano Stabellini wrote:
> > > Empty cdroms are not going to connect, avoid waiting for the backend to
> > > switch to state 4, which is never going to happen, and return
> > > error instead from XenPvBlockFrontInitialization(). Detect an
> > > empty cdrom by looking at the "params" node on xenstore, which is set to
> > > "" or "aio:" for empty drives by libxl.
> > > 
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > 
> > > ---
> > > Changes in v2:
> > > - better commit message
> > > - return EFI_DEVICE_ERROR instead of EFI_NO_MEDIA
> > > - check the return status of XenBusIo->XsBackendRead
> > > ---
> > >   OvmfPkg/XenPvBlkDxe/BlockFront.c |   15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> > > b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> > > index 256ac55..d07e980 100644
> > > --- a/OvmfPkg/XenPvBlkDxe/BlockFront.c
> > > +++ b/OvmfPkg/XenPvBlkDxe/BlockFront.c
> > > @@ -169,6 +169,7 @@ XenPvBlockFrontInitialization (
> > >     XEN_BLOCK_FRONT_DEVICE *Dev;
> > >     XenbusState State;
> > >     UINT64 Value;
> > > +  CHAR8 *Params;
> > >       ASSERT (NodeName != NULL);
> > >   @@ -186,6 +187,20 @@ XenPvBlockFrontInitialization (
> > >     }
> > >     FreePool (DeviceType);
> > >   +  if (Dev->MediaInfo.CdRom) {
> > > +    Status = XenBusIo->XsBackendRead (XenBusIo, XST_NIL, "params",
> > > (VOID**)&Params);
> > > +    if (Status != XENSTORE_STATUS_SUCCESS) {
> > > +      DEBUG ((EFI_D_ERROR, "%a: Failed to read params (%d)\n",
> > > __FUNCTION__, Status));
> > > +      goto Error;
> > > +    }
> > > +    if (AsciiStrLen (Params) == 0 || AsciiStrCmp (Params, "aio:") == 0) {
> > > +      FreePool (Params);
> > > +      DEBUG ((EFI_D_INFO, "%a: Empty cdrom\n", __FUNCTION__));
> > > +      goto Error;
> > > +    }
> > > +    FreePool (Params);
> > > +  }
> > > +
> > >     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",
> > > 
> > Reviewed-by: Laszlo Ersek <lersek@xxxxxxxxxx>
> > 
> > I test-built this for X64 and Ia32, and then committed it to SVN as rev
> > 18651.
> > 
> > Thanks!
> > Laszlo
> 
> Thanks to both.
> I tested it with a git cherry-pick on my tested build of some days ago.
> With ide disks it boots correctly and xl cd-insert and xl cd-eject are
> working.
> With pvdisk don't start, but this time instead freeze on "tianocore logo" but
> freeze with black screen after initial windows boot.
> I retried for take full logs as possibile but it booted correctly and with xl
> cd-insert and xl cd-eject was working.
> I retried other 2 reboot without problem but at the third windows freeze after
> arrived to login screen.
> I suppose that this patch is ok and the problem is another but I not
> understand exactly what.
> 
> In attachment full qemu log and xl dmesg about the latest test where windows
> freezed at login screen, rdp and xl shutdown was not working, its qemu process
> was still active and with near 100% cpu usage.
> If you need more informations/tests tell me and I'll post them.

Yes, that's likely a completely different bug. Does it only happen using
OVMF as guest firmare? I am saying this because it looks like the guest
PV drivers were loaded at the time of the freeze.

In any case it might be good to repost it as a separate bug report to xen-devel.

Thanks again for testing,

Stefano

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel