Re: [Xen-devel] [PATCH v2 09/30] x86/vtd: fix and simplify mapping RMRR regions

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Fri, Sep 30, 2016 at 07:21:41AM -0600, Jan Beulich wrote:
> >>> On 30.09.16 at 13:27, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, Sep 29, 2016 at 08:18:36AM -0600, Jan Beulich wrote:
> >> >>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
> >> > +{
> >> > +    int rc;
> >> > +
> >> > +    while ( nr_pages > 0 )
> >> > +    {
> >> > +        rc = (map ? map_mmio_regions : unmap_mmio_regions)
> >> > +             (d, _gfn(pfn), nr_pages, _mfn(pfn));
> >> > +        if ( rc == 0 )
> >> > +            break;
> >> > +        if ( rc < 0 )
> >> > +        {
> >> > +            printk(XENLOG_ERR
> >> > +                "Failed to %smap %#lx - %#lx into domain %d memory map: 
> >> > %d\n",
> >> > +                   map ? "" : "un", pfn, pfn + nr_pages, d->domain_id, 
> >> > rc);
> >> > +            return rc;
> >> > +        }
> >> > +        nr_pages -= rc;
> >> > +        pfn += rc;
> >> > +        process_pending_softirqs();
> >> > +    }
> >> > +
> >> > +    return rc;
> >> 
> >> The way this is coded it appears to possibly return non-zero even in
> >> success case. I think this would therefore better be a for ( ; ; ) loop.
> > 
> > I don't think this is possible, {un}map_mmio_regions will return < 0 on 
> > error, > 0 if there are pending pages to map, and 0 when all the requested 
> > pages have been mapped successfully.
> 
> Right - hence the "appears to" in my reply; it took me a while to
> figure it's not actually possible, and hence my desire to make this
> more obvious to the reader.

Ah, OK, I misunderstood you then. What about changing the last return rc
to return 0? This would make it more obvious, because I'm not really sure a 
for loop would change much (IMHO the problem is the return semantics used by 
{un}map_mmio_regions).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel