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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 12/3/20 3:17 PM, Paul Durrant wrote:
>> -----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...

My bad, please ignore

Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>

Thank you,

Oleksandr

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