Re: [Xen-devel] [PATCH] libxl: do not rely on guest to respond when forcing pci device removal

[Gianni Tedesco <gianni.tedesco@xxxxxxxxxx>]

On Tue, 2011-03-08 at 11:49 +0000, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@xxxxxxxxxx>
> # Date 1299584929 0
> # Node ID 5084214b8983045d789a86c01e7a0fede46b5e58
> # Parent  0e3211b5c4da98d170ed665c221bcb00e771fc56
> libxl: do not rely on guest to respond when forcing pci device removal
> 
> This is consistent with the expected semantics of a forced device
> removal and also avoids a delay when destroying an HVM domain which
> either does not support hot unplug (does not respond to SCI) or has
> crashed.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> 
> diff -r 0e3211b5c4da -r 5084214b8983 tools/libxl/libxl_pci.c
> --- a/tools/libxl/libxl_pci.c Tue Mar 08 11:13:12 2011 +0000
> +++ b/tools/libxl/libxl_pci.c Tue Mar 08 11:48:49 2011 +0000
> @@ -866,7 +866,7 @@ static int do_pci_remove(libxl__gc *gc, 
>  
>          /* Remove all functions at once atomically by only signalling
>           * device-model for function 0 */
> -        if ( (pcidev->vdevfn & 0x7) == 0 ) {
> +        if ( !force && (pcidev->vdevfn & 0x7) == 0 ) {
>              xs_write(ctx->xsh, XBT_NULL, path, "pci-rem", strlen("pci-rem"));

Shouldn't we maybe send the pci-rem when force removing to give the
guest a chance to do something if it can.

>              if (libxl__wait_for_device_model(ctx, domid, "pci-removed", 
> NULL, NULL) < 0) {
                        && !force ) {

perhaps?

>                  LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Device Model didn't 
> respond in time");
> @@ -874,8 +874,7 @@ static int do_pci_remove(libxl__gc *gc, 
>                   * SCI, if it doesn't respond in time then we may wish to 
>                   * force the removal.
>                   */
> -                if ( !force )
> -                    return ERROR_FAIL;
> +                return ERROR_FAIL;
>              }
>          }
>          path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", 
> domid);

The more I think about any of this code the worse the idea of the
"xenstore + wait a bit" API seems, frankly, erroneous.

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel