Re: [RFC 0 PATCH 3/3] PVH dom0: construct_dom0 changes

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 26 Sep 2013 09:02:41 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.09.13 at 23:03, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > @@ -307,6 +308,136 @@ static void __init
> > process_dom0_ioports_disable(void) }
> >  }
> >  
> > +/*
> > + * Set the 1:1 map for all non-RAM regions for dom 0. Thus, dom0
> > will have
> > + * the entire io region mapped in the EPT/NPT.
> > + *
> > + * PVH FIXME: The following doesn't map MMIO ranges when they sit
> > above the
> > + *            highest E820 covered address.
> 
> This absolutely needs fixing before this can go in.

Any suggestions on how to fix it? Mapping all the way to end could
result in a huge hap table. 


> > +    const struct e820entry *entry;
> > +    unsigned int i, nump;
> > +    int rc;
> > +
> > +    for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ )
> > +    {
> > +        end = entry->addr + entry->size;
> > +
> > +        if ( entry->type == E820_RAM || entry->type ==
> > E820_UNUSABLE ||
> 
> The inclusion of E820_UNUSABLE here clearly needs an explanation
> (ideally in the shape of a code comment).
> 
> > +             i == e820.nr_map - 1 )
> > +        {
> > +            start_pfn = PFN_DOWN(start);
> > +            end_pfn = PFN_UP(end);
> > +
> > +            if ( entry->type == E820_RAM || entry->type ==
> > E820_UNUSABLE )
> > +                end_pfn = PFN_UP(entry->addr);
> 
> Better coded as if/else construct (making more obvious what the
> intention here is - it took me quite some time to see that what
> you're doing here is correct, because the general case really is
> what sits in the if() body, and the exception is what gets done
> before the if(), which is counter intuitive).

Based on xen_set_identity_and_release linux function. Took me a while
to figure that one too. So, I tried simplifying it, but in the end
came back to this. Not sure where I would the else.

thanks
Mukesh