Re: [Xen-devel] [PATCH v2 1/2] libxl: Implement the handler to handle unrecoverable AER errors.

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Venu Busireddy writes ("[PATCH v2 1/2] libxl: Implement the handler to handle 
unrecoverable AER errors."):
> Implement the callback function to handle unrecoverable AER errors, and
> also the public APIs that can be used to register/unregister the handler.
> When an AER error occurs, the handler will forcibly remove the erring
> PCIe device from the guest.

Why is this only sometimes the right thing to do ?  On what basis
might a user choose ?

If this is always the right thing to do then maybe we need to recast
this as a general "please run monitoring for this domain" call ?

> +int libxl_reg_aer_events_handler(libxl_ctx *, uint32_t) 
> LIBXL_EXTERNAL_CALLERS_ONLY;
> +void libxl_unreg_aer_events_handler(libxl_ctx *, uint32_t) 
> LIBXL_EXTERNAL_CALLERS_ONLY;

I think these names are very unintuitive.  They describe the
implementation, not the effect.

The API seems awkward.  Inside libxl, events are only processed while
the application is inside libxl.  So for these functions to be
effective, the calling application must arrange to be running the
libxl event loop.  This should be documented, at least.

What happens if more than one process calls this at once ?

I looked at the message referred to in the 0/2
  https://lists.xen.org/archives/html/xen-devel/2017-06/msg03274.html
  https://lists.xen.org/archives/html/xen-devel/2017-06/msg03269.html
and they says that the approach taken is to kill the guest.
But the approach in these patches is not to kill the guest.

What am I missing ?

> +typedef struct {
> +    uint32_t domid;
> +    libxl__ev_xswatch watch;
> +} libxl_aer_watch;
> +static libxl_aer_watch aer_watch;

The global variable is completely unacceptable, I'm afraid.

> +static void aer_backend_watch_callback(libxl__egc *egc,
> +                                       libxl__ev_xswatch *watch,
> +                                       const char *watch_path,
> +                                       const char *event_path)
> +{
...

I haven't read this code in detail at this stage.

Thanks,
Ian.

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