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

[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@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.

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature