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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.13 at 02:17, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 26 Sep 2013 09:02:41 +0100 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 25.09.13 at 23:03, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
>> > +/*
>> > + * 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. 

You'll probably need a call down from Dom0 telling you where it
finds/puts MMIO resources. Or perhaps that could be mapped
in on demand from the EPT fault handler (since these regions
shouldn't be subject to DMA, and hence IOMMU faults shouldn't
occur - perhaps that's even a reason to not share page tables
at least in dom0-strict mode)?

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

            if ( entry->type != E820_RAM && entry->type != E820_UNUSABLE )
                end_pfn = PFN_UP(end);
            else
                end_pfn = PFN_UP(entry->addr);

Jan


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