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

[Oleksandr]

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

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



> Juergen

--
Regards,

Oleksandr Tyshchenko