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 <[email protected]>
---
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 <[email protected]>
> ---
> 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 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
>
>
> Juergen
--
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 <[email protected]>
>> ---
>> 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