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

[Oleksandr]

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@xxxxxxxx>
> ---
> 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