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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.09.16 at 17:02, <roger.pau@xxxxxxxxxx> wrote:
> 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).

Well, my suggestion was not to use "a for loop" but specifically
"for ( ; ; )" to clarify the only loop exit condition is the single break
statement. That break statement could of course also become a
"return 0". I'd rather not see the return at the end of the function
become "return 0"; if anything (i.e. if you follow my suggestion) it
could be deleted altogether.

Jan


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