Re: [V1 PATCH 06/11] PVH dom0: construct_dom0 changes

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

On Tue, 12 Nov 2013 11:13:31 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Fri, Nov 08, 2013 at 05:23:31PM -0800, Mukesh Rathor wrote:
> > This patch changes construct_dom0 to boot in PVH mode. Changes
> > need to support it are also included here.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/domain_build.c |  224
> > +++++++++++++++++++++++++++++++++++++++----
> > xen/arch/x86/domctl.c       |    2 +- xen/arch/x86/mm/hap/hap.c
> > |   15 +++ xen/include/asm-x86/hap.h   |    1 +
> >  xen/include/xen/domain.h    |    3 +
> >  5 files changed, 227 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/arch/x86/domain_build.c
> > b/xen/arch/x86/domain_build.c index c9ff680..d7c62d1 100644
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -35,6 +35,7 @@
> >  #include <asm/setup.h>
> >  #include <asm/bzimage.h> /* for bzimage_parse */
> >  #include <asm/io_apic.h>
> > +#include <asm/hap.h>
> >  
> >  #include <public/version.h>
> >  
> > @@ -307,6 +308,140 @@ 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.
> > + */
> > +static __init void pvh_map_all_iomem(struct domain *d)
> > +{
> > +    unsigned long start_pfn, end_pfn, end = 0, start = 0;
> > +    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 ||
> > +             i == e820.nr_map - 1 )
> > +        {
> > +            start_pfn = PFN_DOWN(start);
> > +
> > +            /* Unused RAM areas are marked UNUSABLE, so skip it
> > too */
> > +            if ( entry->type != E820_RAM && entry->type !=
> > E820_UNUSABLE )
> 
> The conditional above is of: if ( .. == E820_RAM || .. E820_UNUSABLE )
> 
> why not replicate it here as well? This way it follows the same logic:
> 
> if ( entry->type == E820_RAM || entry->type == E820_UNUSABLE)
> 	end_pfn = PFN_UP(entry->addr);
> else
> 	end_pfn = PFN_UP(end);

Yeah, OK.

> > +                end_pfn = PFN_UP(end);
> > +            else
> > +                end_pfn = PFN_UP(entry->addr);
> > +
> > +            if ( start_pfn < end_pfn )
> > +            {
> > +                nump = end_pfn - start_pfn;
> > +                /* Add pages to the mapping */
> > +                rc = update_memory_mapping(d, start_pfn,
> > start_pfn, nump, 1);
> > +                BUG_ON(rc);
> > +            }
> > +            start = end;
> > +        }
> > +    }
> > +
> > +    /* If the e820 ended under 4GB, we must map the remaining
> > space upto 4GB */
> 
> Could you explain a bit more of 'why'? What if the machine only has
> 3GB and we want to boot dom0 with 512MB.

That's fine. We are mapping the non-ram region, beyond last e820 entry.

Jan, you had pointed this out in earlier review. With the other patch
addressing mmio space above last e820 mapping, this will not be needed
anymore right? can I just remove it from the next patch version then?

thanks
Mukesh