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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4426 bytes --]

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.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]