Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring()

[Oleksandr <olekstysh@gmail.com>]

On 02.05.22 16:30, Juergen Gross wrote:

Hello Juergen


> On 29.04.22 17:10, Oleksandr wrote:
>>
>> On 28.04.22 11:27, Juergen Gross wrote:
>>
>>
>> Hello Juergen
>>
>>
>>> There is no external user of xenbus_grant_ring() left, so merge it into
>>> the only caller xenbus_setup_ring().
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V2:
>>> - make error message more precise (Andrew Cooper)
>>> ---
>>>   drivers/xen/xenbus/xenbus_client.c | 65 
>>> +++++++++---------------------
>>>   include/xen/xenbus.h               |  2 -
>>>   2 files changed, 19 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/xen/xenbus/xenbus_client.c 
>>> b/drivers/xen/xenbus/xenbus_client.c
>>> index 1a2e0d94ccd1..d6fdd2d209d3 100644
>>> --- a/drivers/xen/xenbus/xenbus_client.c
>>> +++ b/drivers/xen/xenbus/xenbus_client.c
>>> @@ -363,50 +363,6 @@ static void xenbus_switch_fatal(struct 
>>> xenbus_device *dev, int depth, int err,
>>>           __xenbus_switch_state(dev, XenbusStateClosing, 1);
>>>   }
>>> -/**
>>> - * xenbus_grant_ring
>>> - * @dev: xenbus device
>>> - * @vaddr: starting virtual address of the ring
>>> - * @nr_pages: number of pages to be granted
>>> - * @grefs: grant reference array to be filled in
>>> - *
>>> - * Grant access to the given @vaddr to the peer of the given device.
>>> - * Then fill in @grefs with grant references.  Return 0 on success, or
>>> - * -errno on error.  On error, the device will switch to
>>> - * XenbusStateClosing, and the error will be saved in the store.
>>> - */
>>> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>>> -              unsigned int nr_pages, grant_ref_t *grefs)
>>> -{
>>> -    int err;
>>> -    unsigned int i;
>>> -    grant_ref_t gref_head;
>>> -
>>> -    err = gnttab_alloc_grant_references(nr_pages, &gref_head);
>>> -    if (err) {
>>> -        xenbus_dev_fatal(dev, err, "granting access to ring page");
>>> -        return err;
>>> -    }
>>> -
>>> -    for (i = 0; i < nr_pages; i++) {
>>> -        unsigned long gfn;
>>> -
>>> -        if (is_vmalloc_addr(vaddr))
>>> -            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
>>> -        else
>>> -            gfn = virt_to_gfn(vaddr);
>>> -
>>> -        grefs[i] = gnttab_claim_grant_reference(&gref_head);
>>> -        gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
>>> -                        gfn, 0);
>>> -
>>> -        vaddr = vaddr + XEN_PAGE_SIZE;
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -EXPORT_SYMBOL_GPL(xenbus_grant_ring);
>>> -
>>>   /*
>>>    * xenbus_setup_ring
>>>    * @dev: xenbus device
>>> @@ -424,6 +380,7 @@ int xenbus_setup_ring(struct xenbus_device *dev, 
>>> gfp_t gfp, void **vaddr,
>>>                 unsigned int nr_pages, grant_ref_t *grefs)
>>>   {
>>>       unsigned long ring_size = nr_pages * XEN_PAGE_SIZE;
>>> +    grant_ref_t gref_head;
>>>       unsigned int i;
>>>       int ret;
>>> @@ -433,9 +390,25 @@ int xenbus_setup_ring(struct xenbus_device 
>>> *dev, gfp_t gfp, void **vaddr,
>>>           goto err;
>>>       }
>>> -    ret = xenbus_grant_ring(dev, *vaddr, nr_pages, grefs);
>>> -    if (ret)
>>> +    ret = gnttab_alloc_grant_references(nr_pages, &gref_head);
>>> +    if (ret) {
>>> +        xenbus_dev_fatal(dev, ret, "granting access to %u ring pages",
>>> +                 nr_pages);
>>>           goto err;
>>> +    }
>>> +
>>> +    for (i = 0; i < nr_pages; i++) {
>>> +        unsigned long gfn;
>>> +
>>> +        if (is_vmalloc_addr(*vaddr))
>>> +            gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr[i]));
>>> +        else
>>> +            gfn = virt_to_gfn(vaddr[i]);
>>> +
>>> +        grefs[i] = gnttab_claim_grant_reference(&gref_head);
>>
>> gnttab_claim_grant_reference() can return error if no free grant 
>> reference remains.
>
> This can happen only in case gnttab_alloc_grant_references() didn't
> allocate enough grants but told us it succeeded doing so.
>
>> I understand this patch only moves the code, but probably it would be 
>> better to add a missing check here (and likely rollback already 
>> processed grants if any?).
>
> I don't think this is needed, as this would be a clear bug in the code.

I would put WARN_ON_ONCE if ref is an error value then like xen-netfront 
does. Either way, with or without it you can add my:

Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>



>
>
>
> Juergen

-- 
Regards,

Oleksandr Tyshchenko