Re: [v8][PATCH 09/17] hvmloader/ram: check if guest memory is out of reserved device memory maps

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

On Mon, Dec 01, 2014 at 05:24:27PM +0800, Tiejun Chen wrote:
> We need to check to reserve all reserved device memory maps in e820
> to avoid any potential guest memory conflict.
> 
> Currently, if we can't insert RDM entries directly, we may need to handle
> several ranges as follows:

s/several/two/

s/follows/follow/

> a. Fixed Ranges --> BUG()
>  lowmem_reserved_base-0xA0000: reserved by BIOS implementation,
>  BIOS region,
>  RESERVED_MEMBASE ~ 0x100000000,

I am not sure what you are trying to say here. Could you explain it 
a bit more please?

> b. RAM or RAM:Hole -> Try to reserve

Reading the beginning of the 'Currently'.. this says:

we may need to handle RAM or RAM:Hole -> Try to reserve.

I don't know what 'RAM:Hole' means. And instead of using '->' you can
say: we will try to reserve.

But what are we reserving ? Are we reserving it as an E820_RSV or just
as an hole? What about the RAM behind it? Are we gulping up the RAM regions
(as in losing them) or are we moving the RAM regions (GPFNs) to somewhere else?

> 
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> ---
>  tools/firmware/hvmloader/e820.c | 168 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 168 insertions(+)
> 
> diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
> index 2e05e93..ef87e41 100644
> --- a/tools/firmware/hvmloader/e820.c
> +++ b/tools/firmware/hvmloader/e820.c
> @@ -22,6 +22,7 @@
>  
>  #include "config.h"
>  #include "util.h"
> +#include <xen/memory.h>
>  
>  void dump_e820_table(struct e820entry *e820, unsigned int nr)
>  {
> @@ -68,12 +69,173 @@ void dump_e820_table(struct e820entry *e820, unsigned int nr)
>      }
>  }
>  
> +extern struct xen_reserved_device_memory *rdm_map;
> +static unsigned int construct_rdm_e820_maps(unsigned int next_e820_entry_index,

s/next_e820_entry_index/next_entry/ ?

> +                                            uint32_t nr_map,
> +                                            struct xen_reserved_device_memory *map,
> +                                            struct e820entry *e820,
> +                                            unsigned int lowmem_reserved_base,
> +                                            unsigned int bios_image_base)
> +{
> +    unsigned int i, j, sum_nr;
> +    uint64_t start, end, next_start, rdm_start, rdm_end;
> +    uint32_t type;
> +    int err = 0;
> +
> +    for ( i = 0; i < nr_map; i++ )
> +    {
> +        rdm_start = (uint64_t)map[i].start_pfn << PAGE_SHIFT;
> +        rdm_end = rdm_start + ((uint64_t)map[i].nr_pages << PAGE_SHIFT);
> +
> +        for ( j = 0; j < next_e820_entry_index - 1; j++ )
> +        {
> +            sum_nr = next_e820_entry_index + nr_map;
> +            start = e820[j].addr;
> +            end = e820[j].addr + e820[j].size;
> +            type = e820[j].type;
> +            next_start = e820[j+1].addr;
> +
> +            if ( rdm_start >= start && rdm_start <= end )
> +            {
> +                /*
> +                 * lowmem_reserved_base-0xA0000: reserved by BIOS
> +                 * implementation.
> +                 * Or BIOS region.
> +                 */
> +                if ( (lowmem_reserved_base < 0xA0000 &&
> +                        start == lowmem_reserved_base) ||
> +                     start == bios_image_base )

something is off with your spaces.
> +                {
> +                    err = -1;
> +                    break;

Keep in mind we will just break out of this loop. Do you want to add
at the end of this loop:


if (err)
	break;

> +                }
> +            }
> +
> +            /* Just amid those remaining e820 entries. */
> +            if ( (rdm_start > end) && (rdm_end < next_start) )
> +            {
> +                memmove(&e820[j+2], &e820[j+1],
> +                        (sum_nr - j - 1) * sizeof(struct e820entry));

What if there is something at j+2? Should we have an
j-2 < E820_MAX check somewhere?

This whole 'memmove' logic is making me a bit worried.

Would it be easier to have this logic inside build_e820_table so
that it could construct the e820 with this information right away?

Or if that was deemed incorrect could you explain that in the
commit description?

> +
> +                /* Then fill RMRR into that entry. */
> +                e820[j+1].addr = rdm_start;
> +                e820[j+1].size = rdm_end - rdm_start;
> +                e820[j+1].type = E820_RESERVED;
> +                next_e820_entry_index++;
> +                continue;
> +            }
> +
> +            /* Already at the end. */
> +            if ( (rdm_start > end) && !next_start )
> +            {
> +                e820[next_e820_entry_index].addr = rdm_start;
> +                e820[next_e820_entry_index].size = rdm_end - rdm_start;
> +                e820[next_e820_entry_index].type = E820_RESERVED;
> +                next_e820_entry_index++;
> +                continue;
> +            }
> +
> +            if ( type == E820_RAM )
> +            {
> +                /* If coincide with one RAM range. */
> +                if ( rdm_start == start && rdm_end == end)
> +                {
> +                    e820[j].type = E820_RESERVED;
> +                    continue;
> +                }
> +
> +                /* If we're just aligned with start of one RAM range. */
> +                if ( rdm_start == start && rdm_end < end )
> +                {
> +                    memmove(&e820[j+1], &e820[j],
> +                            (sum_nr - j) * sizeof(struct e820entry));
> +
> +                    e820[j+1].addr = rdm_end;
> +                    e820[j+1].size = e820[j].addr + e820[j].size - rdm_end;
> +                    e820[j+1].type = E820_RAM;
> +                    next_e820_entry_index++;
> +
> +                    e820[j].addr = rdm_start;
> +                    e820[j].size = rdm_end - rdm_start;
> +                    e820[j].type = E820_RESERVED;
> +                    continue;
> +                }
> +
> +                /* If we're just aligned with end of one RAM range. */
> +                if ( rdm_start > start && rdm_end == end )
> +                {
> +                    memmove(&e820[j+1], &e820[j],
> +                            (sum_nr - j) * sizeof(struct e820entry));
> +
> +                    e820[j].size = rdm_start - e820[j].addr;
> +                    e820[j].type = E820_RAM;
> +
> +                    e820[j+1].addr = rdm_start;
> +                    e820[j+1].size = rdm_end - rdm_start;
> +                    e820[j+1].type = E820_RESERVED;
> +                    next_e820_entry_index++;
> +                    continue;
> +                }
> +
> +                /* If we're just in of one RAM range */
> +                if ( rdm_start > start && rdm_end < end )
> +                {
> +                    memmove(&e820[j+2], &e820[j],
> +                            (sum_nr - j) * sizeof(struct e820entry));
> +
> +                    e820[j+2].addr = rdm_end;
> +                    e820[j+2].size = e820[j].addr + e820[j].size - rdm_end;
> +                    e820[j+2].type = E820_RAM;
> +                    next_e820_entry_index++;
> +
> +                    e820[j+1].addr = rdm_start;
> +                    e820[j+1].size = rdm_end - rdm_start;
> +                    e820[j+1].type = E820_RESERVED;
> +                    next_e820_entry_index++;
> +
> +                    e820[j].size = rdm_start - e820[j].addr;
> +                    e820[j].type = E820_RAM;
> +                    continue;
> +                }
> +
> +                /* If we're going last RAM:Hole range */
> +                if ( end < next_start && rdm_start > start &&
> +                     rdm_end < next_start )
> +                {
> +                    memmove(&e820[j+1], &e820[j],
> +                            (sum_nr - j) * sizeof(struct e820entry));
> +
> +                    e820[j].size = rdm_start - e820[j].addr;
> +                    e820[j].type = E820_RAM;
> +
> +                    e820[j+1].addr = rdm_start;
> +                    e820[j+1].size = rdm_end - rdm_start;
> +                    e820[j+1].type = E820_RESERVED;
> +                    next_e820_entry_index++;
> +                    continue;
> +                }
> +            }
> +        }
> +    }
> +
> +    /* These overlap may issue guest can't work well. */
> +    if ( err )
> +    {
> +        printf("Guest can't work with some reserved device memory overlap!\n");
> +        BUG();
> +    }
> +
> +    /* Fine to construct RDM mappings into e820. */
> +    return next_e820_entry_index;
> +}
> +
>  /* Create an E820 table based on memory parameters provided in hvm_info. */
>  int build_e820_table(struct e820entry *e820,
>                       unsigned int lowmem_reserved_base,
>                       unsigned int bios_image_base)
>  {
>      unsigned int nr = 0;
> +    unsigned int nr_entries = 0;
>  
>      if ( !lowmem_reserved_base )
>              lowmem_reserved_base = 0xA0000;
> @@ -169,6 +331,12 @@ int build_e820_table(struct e820entry *e820,
>          nr++;
>      }
>  
> +    nr_entries = hvm_get_reserved_device_memory_map();
> +    if ( nr_entries )
> +        nr = construct_rdm_e820_maps(nr, nr_entries, rdm_map, e820,
> +                                     lowmem_reserved_base,
> +                                     bios_image_base);
> +
>      return nr;
>  }
>  
> -- 
> 1.9.1
>