Messages in this thread | | | Date | Mon, 2 May 2022 15:30:21 +0200 | Subject | Re: [PATCH v2 19/19] xen/xenbus: eliminate xenbus_grant_ring() | From | Juergen Gross <> |
| |
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 [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |