RE: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach are reflected in the config

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
> Sent: 01 December 2020 13:12
> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Ian Jackson <iwj@xxxxxxxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Subject: Re: [PATCH v4 03/23] libxl: Make sure devices added by pci-attach 
> are reflected in the config
> 
> Hi, Paul!
> 
> On 11/24/20 10:01 AM, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@xxxxxxxxxx>
> >
> > Currently libxl__device_pci_add_xenstore() is broken in that does not
> > update the domain's configuration for the first device added (which causes
> > creation of the overall backend area in xenstore). This can be easily 
> > observed
> > by running 'xl list -l' after adding a single device: the device will be
> > missing.
> >
> > This patch fixes the problem and adds a DEBUG log line to allow easy
> > verification that the domain configuration is being modified. Also, the use
> > of libxl__device_generic_add() is dropped as it leads to a confusing 
> > situation
> > where only partial backend information is written under the xenstore
> > '/libxl' path. For LIBXL__DEVICE_KIND_PCI devices the only definitive
> > information in xenstore is under '/local/domain/0/backend' (the '0' being
> > hard-coded).
> >
> > NOTE: This patch includes a whitespace in add_pcis_done().
> >
> > Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>
> > ---
> > Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
> > Cc: Wei Liu <wl@xxxxxxx>
> > Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >
> > v2:
> >   - Avoid having two completely different ways of adding devices into 
> > xenstore
> >
> > v3:
> >   - Revert some changes form v2 as there is confusion over use of the libxl
> >     and backend xenstore paths which needs to be fixed
> > ---
> >   tools/libs/light/libxl_pci.c | 87 
> > +++++++++++++++++++++++---------------------
> >   1 file changed, 45 insertions(+), 42 deletions(-)
> >
> > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> > index 9d44b28f0a..da01c77ba2 100644
> > --- a/tools/libs/light/libxl_pci.c
> > +++ b/tools/libs/light/libxl_pci.c
> > @@ -79,39 +79,55 @@ static void libxl__device_from_pci(libxl__gc *gc, 
> > uint32_t domid,
> >       device->kind = LIBXL__DEVICE_KIND_PCI;
> >   }
> >
> > -static int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
> > -                                     const libxl_device_pci *pci,
> > -                                     int num)
> > +static void libxl__create_pci_backend(libxl__gc *gc, xs_transaction_t t,
> > +                                      uint32_t domid, const 
> > libxl_device_pci *pci)
> >   {
> > -    flexarray_t *front = NULL;
> > -    flexarray_t *back = NULL;
> > -    libxl__device device;
> > -    int i;
> > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > +    flexarray_t *front, *back;
> > +    char *fe_path, *be_path;
> > +    struct xs_permissions fe_perms[2], be_perms[2];
> > +
> > +    LOGD(DEBUG, domid, "Creating pci backend");
> >
> >       front = flexarray_make(gc, 16, 1);
> >       back = flexarray_make(gc, 16, 1);
> >
> > -    LOGD(DEBUG, domid, "Creating pci backend");
> > -
> > -    /* add pci device */
> > -    libxl__device_from_pci(gc, domid, pci, &device);
> > +    fe_path = libxl__domain_device_frontend_path(gc, domid, 0,
> > +                                                 LIBXL__DEVICE_KIND_PCI);
> > +    be_path = libxl__domain_device_backend_path(gc, 0, domid, 0,
> > +                                                LIBXL__DEVICE_KIND_PCI);
> >
> > +    flexarray_append_pair(back, "frontend", fe_path);
> >       flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> > -    flexarray_append_pair(back, "online", "1");
> > +    flexarray_append_pair(back, "online", GCSPRINTF("%d", 1));
> >       flexarray_append_pair(back, "state", GCSPRINTF("%d", 
> > XenbusStateInitialising));
> >       flexarray_append_pair(back, "domain", libxl__domid_to_name(gc, 
> > domid));
> >
> > -    for (i = 0; i < num; i++, pci++)
> > -        libxl_create_pci_backend_device(gc, back, i, pci);
> > +    be_perms[0].id = 0;
> 
> There was a discussion [1] on PCI on ARM and one of the question was that it 
> is possible
> 
> that we have the pci backend running in a late hardware domain/driver domain, 
> which may
> 
> not be Domain-0. Do you think we can avoid using 0 here and get some clue of 
> the domain
> 
> from "*backend=domain-id"? If not set it will return Domain-0's ID and won't 
> break anything*

Not sure what you're asking for since...

> 
> *Thank you,*
> 
> *Oleksandr
> *
> 
> > +    be_perms[0].perms = XS_PERM_NONE;
> > +    be_perms[1].id = domid;
> > +    be_perms[1].perms = XS_PERM_READ;
> > +
> > +    xs_rm(ctx->xsh, t, be_path);
> > +    xs_mkdir(ctx->xsh, t, be_path);
> > +    xs_set_permissions(ctx->xsh, t, be_path, be_perms,
> > +                       ARRAY_SIZE(be_perms));
> > +    libxl__xs_writev(gc, t, be_path, libxl__xs_kvs_of_flexarray(gc, back));
> >
> > -    flexarray_append_pair(back, "num_devs", GCSPRINTF("%d", num));
> > +    flexarray_append_pair(front, "backend", be_path);
> >       flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 0));

... backend-id is written here.

  Paul