Messages in this thread | | | Subject | Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring() | From | Oleksandr <> | Date | Mon, 2 May 2022 17:12:19 +0300 |
| |
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
| |