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

[Venu Busireddy <venu.busireddy@oracle.com>]

On 2017-06-30 11:18:15 +0100, Wei Liu wrote:
> 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.

My mistake. Will remove it.

> > 
> > 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@oracle.com>
> > ---
> >  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;
> > +}
> > +
> [...]

Will make these changes, if deemed necessary, depending on what we agree
on the final solution. This code may go away if our approach changes.
Please see [1] and [2] below.

> >  
> >  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, ...).

Will switch to using libxl__ev_xswatch_deregister().

> > +
> > +    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.

Will do.

> > +    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?

Please see [1] and [2] below:

> 
> > +    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

Will get rid of this wrapper.

> > +{
> > +    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)
> > 


[1] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00552.html
[2] https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00555.html


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