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_grant_foreign_access_ref(grefs[i], dev->otherend_id,
+						gfn, 0);
+	}
 
 	return 0;
 
diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
index b533b4adc835..eaa932b99d8a 100644
--- a/include/xen/xenbus.h
+++ b/include/xen/xenbus.h
@@ -224,8 +224,6 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
 			 const char *pathfmt, ...);
 
 int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
-int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
-		      unsigned int nr_pages, grant_ref_t *grefs);
 int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
 		      unsigned int nr_pages, grant_ref_t *grefs);
 void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,
-- 
2.34.1

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.

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?).



> +		gnttab_grant_foreign_access_ref(grefs[i], dev->otherend_id,
> +						gfn, 0);
> +	}
>   
>   	return 0;
>   
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index b533b4adc835..eaa932b99d8a 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -224,8 +224,6 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
>   			 const char *pathfmt, ...);
>   
>   int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> -int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> -		      unsigned int nr_pages, grant_ref_t *grefs);
>   int xenbus_setup_ring(struct xenbus_device *dev, gfp_t gfp, void **vaddr,
>   		      unsigned int nr_pages, grant_ref_t *grefs);
>   void xenbus_teardown_ring(void **vaddr, unsigned int nr_pages,

-- 
Regards,

Oleksandr Tyshchenko

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

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