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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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);

?

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

> +    if ( end < GB(4) )
> +    {
> +        start_pfn = PFN_UP(end);
> +        end_pfn = (GB(4)) >> PAGE_SHIFT;
> +        nump = end_pfn - start_pfn;
> +        rc = update_memory_mapping(d, start_pfn, start_pfn, nump, 1);
> +        BUG_ON(rc);
> +    }
> +}

Thank you!