Re: [Xen-devel] [PATCH 4/6] libxl: Add wrappers for new commands and add AER error handler

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Jun 27, 2017 at 12:14:56PM -0500, Venu Busireddy wrote:
> libxl: Add wrappers for new commands and add AER error handler

Extraneous line.

> 
> Add wrappers for the newly introduced commands "pci-assignable-hide",
> "pci-assignable-unhide", and "pci-assignable-list-hidden".
> 
> Implement the callback function to handle unrecoverable AER errors.
> 
> Signed-off-by: Venu Busireddy <venu.busireddy@xxxxxxxxxx>
> ---
>  tools/libxl/libxl.h       |   3 +
>  tools/libxl/libxl_event.h |   2 +
>  tools/libxl/libxl_pci.c   | 150 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 155 insertions(+)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index cf8687a..5a5bd14 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1944,6 +1944,9 @@ int libxl_device_events_handler(libxl_ctx *ctx,
>  int libxl_device_pci_assignable_add(libxl_ctx *ctx, libxl_device_pci 
> *pcidev, int rebind);
>  int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci 
> *pcidev, int rebind);
>  libxl_device_pci *libxl_device_pci_assignable_list(libxl_ctx *ctx, int *num);
> +int libxl_device_pci_assignable_hide(libxl_ctx *ctx, libxl_device_pci 
> *pcidev);
> +int libxl_device_pci_assignable_unhide(libxl_ctx *ctx, libxl_device_pci 
> *pcidev);
> +int libxl_device_pci_assignable_is_hidden(libxl_ctx *ctx, libxl_device_pci 
> *pcidev);
>  
>  /* CPUID handling */
>  int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* 
> str);
> diff --git a/tools/libxl/libxl_event.h b/tools/libxl/libxl_event.h
> index 1ea789e..4c78798 100644
> --- a/tools/libxl/libxl_event.h
> +++ b/tools/libxl/libxl_event.h
> @@ -178,6 +178,8 @@ void libxl_event_register_callbacks(libxl_ctx *ctx,
>  typedef struct libxl__evgen_domain_death libxl_evgen_domain_death;
>  int libxl_evenable_domain_death(libxl_ctx *ctx, uint32_t domid,
>                           libxl_ev_user, libxl_evgen_domain_death 
> **evgen_out);
> +int libxl_reg_aer_events_handler(libxl_ctx *, uint32_t) 
> LIBXL_EXTERNAL_CALLERS_ONLY;
> +void libxl_unreg_aer_events_handler(libxl_ctx *, uint32_t);
>  void libxl_evdisable_domain_death(libxl_ctx *ctx, libxl_evgen_domain_death*);
>    /* Arranges for the generation of DOMAIN_SHUTDOWN and DOMAIN_DEATH
>     * events.  A domain which is destroyed before it shuts down
> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> index b14df16..e6996e5 100644
> --- a/tools/libxl/libxl_pci.c
> +++ b/tools/libxl/libxl_pci.c
> @@ -874,6 +874,42 @@ int libxl_device_pci_assignable_add(libxl_ctx *ctx, 
> libxl_device_pci *pcidev,
>      return rc;
>  }
>  
> +int libxl_device_pci_assignable_hide(libxl_ctx *ctx, libxl_device_pci 
> *pcidev)
> +{
> +    GC_INIT(ctx);
> +    int rc;

int r;

> +
> +    rc = xc_hide_device(ctx->xch, pcidev_encode_bdf(pcidev));

r = xc_...

> +    if (rc < 0)

if (r < 0) { LOG(...); rc = ERROR_???; }

> +        LOGD(ERROR, 0, "xc_hide_device failed");
> +
> +    GC_FREE;
> +    return rc;
> +}
> +
[...]
>  
>  int libxl_device_pci_assignable_remove(libxl_ctx *ctx, libxl_device_pci 
> *pcidev,
>                                         int rebind)
> @@ -1292,6 +1328,120 @@ out:
>      return rc;
>  }
>  
> +static void domain_destroy_callback(libxl__egc *egc,
> +                                    libxl__domain_destroy_state *dds,
> +                                    int rc)
> +{
> +    STATE_AO_GC(dds->ao);
> +
> +    if (rc)
> +        LOGD(ERROR, dds->domid, "Destruction of domain failed, rc = %d", rc);
> +
> +    libxl__nested_ao_free(ao);
> +}
> +
> +
> +typedef struct {
> +    uint32_t domid;
> +    libxl__ao *ao;
> +    libxl__ev_xswatch watch;
> +} libxl_aer_watch;
> +static libxl_aer_watch aer_watch;
> +
> +static void aer_backend_watch_callback(libxl__egc *egc,
> +                                       libxl__ev_xswatch *watch,
> +                                       const char *watch_path,
> +                                       const char *event_path)
> +{
> +    libxl_aer_watch *l_aer_watch = CONTAINER_OF(watch, *l_aer_watch, watch);
> +    libxl__ao *nested_ao = libxl__nested_ao_create(l_aer_watch->ao);
> +    STATE_AO_GC(nested_ao);
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    uint32_t domid = l_aer_watch->domid;
> +    uint32_t seg, bus, dev, fn;
> +    int rc;
> +    char *p, *path, *dst_path;
> +    const char *aerFailedSBDF;
> +    struct xs_permissions rwperm[1];
> +    libxl__domain_destroy_state *dds;
> +    GCNEW(dds);
> +
> +    /* Extract the backend directory. */
> +    path = libxl__strdup(gc, event_path);
> +    p = strrchr(path, '/');
> +    if (p == NULL)
> +        goto skip;
> +    if (strcmp(p, "/aerFailedSBDF") != 0)
> +        goto skip;
> +    /* Truncate the string so it points to the backend directory. */
> +    *p = '\0';
> +
> +    /* Fetch the value of the failed PCI device. */
> +    rc = libxl__xs_read_checked(gc, XBT_NULL,
> +            GCSPRINTF("%s/aerFailedSBDF", path), &aerFailedSBDF);
> +    if (rc || !aerFailedSBDF)
> +        goto skip;
> +    sscanf(aerFailedSBDF, "%x:%x:%x.%x", &seg, &bus, &dev, &fn);
> +
> +    libxl_unreg_aer_events_handler(ctx, domid);

You need to be careful about calling a public API. In this case I think
either calling libxl__ev_xswatch_deregister directly or having something
like libxl__unregister_aer_events_handler(libxl__gc *gc, ...).

> +
> +    dds->ao = nested_ao;
> +    dds->domid = domid;
> +    dds->callback = domain_destroy_callback;
> +    libxl__domain_destroy(egc, dds);
> +
> +    rc = xc_hide_device(ctx->xch, seg << 16 | bus << 8 | dev << 3 | fn);

Please provide a helper function or macro to return sbdf form instead of
open coding.

> +    if (rc)
> +        LOGD(ERROR, domid, " xc_hide_device() failed, rc = %d", rc);
> +
> +    rwperm[0].id = 0;
> +    rwperm[0].perms = XS_PERM_READ | XS_PERM_WRITE;
> +    dst_path = GCSPRINTF("/local/domain/0/backend/pci/0/0/%s", 
> "aerFailedPCIs");
> +    rc = libxl__xs_mknod(gc, XBT_NULL, dst_path, rwperm, 1);
> +    if (rc) {
> +        LOGD(ERROR, domid, " libxl__xs_mknod() failed, rc = %d", rc);
> +        goto skip;
> +    }
> +
> +    rc = libxl__xs_write_checked(gc, XBT_NULL, dst_path, aerFailedSBDF);
> +    if (rc)
> +        LOGD(ERROR, domid, " libxl__xs_write_checked() failed, rc = %d", rc);
> +
> +skip:
> +    return;
> +}
> +
> +/* Handler of events for device driver domains */
> +int libxl_reg_aer_events_handler(libxl_ctx *ctx, uint32_t domid)
> +{
> +    AO_CREATE(ctx, 0, 0);
> +    int rc;
> +    char *be_path;
> +
> +    /*
> +     * We use absolute paths because we want xswatch to also return
> +     * absolute paths that can be parsed by libxl__parse_backend_path.
> +     */
> +    aer_watch.ao = ao;
> +    aer_watch.domid = domid;
> +    be_path = GCSPRINTF("/local/domain/0/backend/pci/%u/0/aerFailedSBDF", 
> domid);

Again, what about driver domain?

> +    rc = libxl__ev_xswatch_register(gc, &aer_watch.watch,
> +                                    aer_backend_watch_callback, be_path);
> +    if (rc)
> +        return AO_CREATE_FAIL(rc);
> +
> +    return AO_INPROGRESS;
> +}
> +
> +/* Handler of events for device driver domains */
> +void libxl_unreg_aer_events_handler(libxl_ctx *ctx, uint32_t domid)

unregister

> +{
> +    GC_INIT(ctx);
> +
> +    libxl__ev_xswatch_deregister(gc, &aer_watch.watch);
>

GC_FREE

> +    return;
> +}
> +
>  static void libxl__add_pcidevs(libxl__egc *egc, libxl__ao *ao, uint32_t 
> domid,
>                                 libxl_domain_config *d_config,
>                                 libxl__multidev *multidev)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> https://lists.xen.org/xen-devel

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