On 12/3/20 3:17 PM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>> Sent: 01 December 2020 13:12
>> To: Paul Durrant <p...@xen.org>; xen-devel@lists.xenproject.org
>> Cc: Paul Durrant <pdurr...@amazon.com>; Ian Jackson <i...@xenproject.org>;
>> Wei Liu <w...@xen.org>;
>> Anthony PERARD <anthony.per...@citrix.com>
>> 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 <pdurr...@amazon.com>
>>>
>>> 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 <pdurr...@amazon.com>
>>> ---
>>> Cc: Ian Jackson <i...@xenproject.org>
>>> Cc: Wei Liu <w...@xen.org>
>>> Cc: Anthony PERARD <anthony.per...@citrix.com>
>>>
>>> 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_andrushche...@epam.com>
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
>
>
