On Fri, 2010-07-30 at 14:40 +0100, Ian Jackson wrote:
> Gianni Tedesco writes ("[Xen-devel] [PATCH, RFC]: qemu:
> hang-free/error-tolerant PCI hot-plug protocol"):
> > The interface for PCI hotplug is flexible enough to shoot ones-self in
> > the foot. It is possible to try to insert a PCI device in to a slot
> > already occupied by a qemu emulated device (NIC, PIIX, ISA-bridge, etc.)
> > In this case qemu (wisely) refuses to do the hotplug. Since there is no
> > way for a toolstack to query qemu's pci device layout there is no way to
> > check for this ahead of time. In this case the toolstack must wait for
> > device-model state to change to pci-inserted which never happens.
>
> Hrm.
>
> > I propose that when qemu decides not to hot-plug a device that it raise
> > the "pci-inserted" status anyway. The tools must then check the
> > "parameter" key in xenbus for a non-error string. In other words:
>
> Why do this rather than a new status "pci-insert-failed" ? How does
> this affect existing toolstacks ?
As stefano says, the new status would make existing tools keep waiting.
On the other hand I'm not sure I'm as confident as him about backward
compatibility. In this case libxl will sscanf(error_string + 2, "%x",
&pcidev->vdevfn) -- with somewhat undefined results.
> > --- a/hw/piix4acpi.c
> > +++ b/hw/piix4acpi.c
>
> I haven't looked at the code near here. Does qemu log anything ? If
> so then the corresponding toolstack patches should say "consult qemu
> logfile". Otherwise perhaps qemu should.
Yes, the "parameter" key is already used for error reporting as well as
to return the inserted devices virtual devfn. IMO that is a better
approach than having toolstacks grovel around in log files which could
be anywhere.
I think that perhaps "pci-insert-failed" status may be the way to go
here. To hell with the old tool-stacks because at least then nothing
will change for them. If we can agree on this protocol then I'll submit
the following patches in the next go-around:
diff --git a/hw/piix4acpi.c b/hw/piix4acpi.c
index 1efa77d..a74177d 100644
--- a/hw/piix4acpi.c
+++ b/hw/piix4acpi.c
@@ -620,6 +620,7 @@ void acpi_php_add(int devfn)
if ( strlen(ret_str) > 0 )
xenstore_record_dm("parameter", ret_str);
+ xenstore_record_dm_state("pci-insert-failed");
return;
}
diff -r 901603018da8 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Thu Jul 29 19:48:19 2010 +0100
+++ b/tools/libxl/libxl.c Fri Jul 30 17:36:04 2010 +0100
@@ -1419,12 +1419,12 @@ int libxl_detach_device_model(libxl_ctx
int libxl_confirm_device_model_startup(libxl_ctx *ctx,
libxl_device_model_starting *starting)
{
- int problem = libxl_wait_for_device_model(ctx, starting->domid, "running",
- libxl_spawn_check,
- starting->for_spawn);
- int detach = libxl_detach_device_model(ctx, starting);
+ int problem = libxl_wait_for_device_model(ctx, starting->domid, "running",
NULL, NULL);
+ int detach;
+ if ( !problem )
+ problem = libxl_spawn_check(ctx, starting->for_spawn);
+ detach = libxl_detach_device_model(ctx, starting);
return problem ? problem : detach;
- return 0;
}
diff -r 901603018da8 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c Thu Jul 29 19:48:19 2010 +0100
+++ b/tools/libxl/libxl_device.c Fri Jul 30 17:36:04 2010 +0100
@@ -380,6 +380,8 @@ int libxl_device_del(libxl_ctx *ctx, lib
int libxl_wait_for_device_model(libxl_ctx *ctx,
uint32_t domid, char *state,
int (*check_callback)(libxl_ctx *ctx,
+ uint32_t domid,
+ const char *state,
void *userdata),
void *check_callback_userdata)
{
@@ -402,18 +404,24 @@ int libxl_wait_for_device_model(libxl_ct
nfds = xs_fileno(xsh) + 1;
while (rc > 0 || (!rc && tv.tv_sec > 0)) {
p = xs_read(xsh, XBT_NULL, path, &len);
- if (p && (!state || !strcmp(state, p))) {
- free(p);
- xs_unwatch(xsh, path, path);
- xs_daemon_close(xsh);
- if (check_callback) {
- rc = check_callback(ctx, check_callback_userdata);
- if (rc) return rc;
- }
- return 0;
+ if ( NULL == p )
+ goto again;
+
+ if ( NULL != state && strcmp(p, state) )
+ goto again;
+
+ if ( NULL != check_callback ) {
+ rc = (*check_callback)(ctx, domid, p, check_callback_userdata);
+ if ( rc > 0 )
+ goto again;
}
+
free(p);
+ xs_unwatch(xsh, path, path);
+ xs_daemon_close(xsh);
+ return rc;
again:
+ free(p);
FD_ZERO(&rfds);
FD_SET(xs_fileno(xsh), &rfds);
rc = select(nfds, &rfds, NULL, NULL, &tv);
diff -r 901603018da8 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Thu Jul 29 19:48:19 2010 +0100
+++ b/tools/libxl/libxl_internal.h Fri Jul 30 17:36:04 2010 +0100
@@ -162,6 +162,8 @@ int libxl_devices_destroy(libxl_ctx *ctx
int libxl_wait_for_device_model(libxl_ctx *ctx,
uint32_t domid, char *state,
int (*check_callback)(libxl_ctx *ctx,
+ uint32_t domid,
+ const char *state,
void *userdata),
void *check_callback_userdata);
int libxl_wait_for_backend(libxl_ctx *ctx, char *be_path, char *state);
diff -r 901603018da8 tools/libxl/libxl_pci.c
--- a/tools/libxl/libxl_pci.c Thu Jul 29 19:48:19 2010 +0100
+++ b/tools/libxl/libxl_pci.c Fri Jul 30 17:36:04 2010 +0100
@@ -480,6 +480,20 @@ static int is_assigned(libxl_device_pci
return 0;
}
+static int pci_ins_check(libxl_ctx *ctx, uint32_t domid, const char *state,
void *priv)
+{
+ char *orig_state = priv;
+
+ if ( !strcmp(state, "pci-insert-failed") )
+ return -1;
+ if ( !strcmp(state, "pci-inserted") )
+ return 0;
+ if ( !strcmp(state, orig_state) )
+ return 1;
+
+ return -1;
+}
+
static int do_pci_add(libxl_ctx *ctx, uint32_t domid, libxl_device_pci *pcidev)
{
char *path;
@@ -502,13 +516,17 @@ static int do_pci_add(libxl_ctx *ctx, ui
pcidev->bus, pcidev->dev, pcidev->func);
path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/command",
domid);
xs_write(ctx->xsh, XBT_NULL, path, "pci-ins", strlen("pci-ins"));
- if (libxl_wait_for_device_model(ctx, domid, "pci-inserted", NULL,
NULL) < 0)
- XL_LOG(ctx, XL_LOG_ERROR, "Device Model didn't respond in time");
+ rc = libxl_wait_for_device_model(ctx, domid, NULL, pci_ins_check,
state);
path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/parameter",
domid);
vdevfn = libxl_xs_read(ctx, XBT_NULL, path);
- sscanf(vdevfn + 2, "%x", &pcidev->vdevfn);
path = libxl_sprintf(ctx, "/local/domain/0/device-model/%d/state",
domid);
+ if ( rc < 0 )
+ XL_LOG(ctx, XL_LOG_ERROR, "qemu refused to add device: %s",
vdevfn);
+ else if ( sscanf(vdevfn, "0x%x", &pcidev->vdevfn) != 1 )
+ rc = -1;
xs_write(ctx->xsh, XBT_NULL, path, state, strlen(state));
+ if ( rc )
+ return ERROR_FAIL;
} else {
char *sysfs_path = libxl_sprintf(ctx,
SYSFS_PCI_DEV"/"PCI_BDF"/resource", pcidev->domain,
pcidev->bus, pcidev->dev,
pcidev->func);
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home