Re: [v3][PATCH 13/16] tools/libxl: detect and avoid conflicts with RDM

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/6/13 0:39, Wei Liu wrote:
> On Thu, Jun 11, 2015 at 09:15:22AM +0800, Tiejun Chen wrote:
> [...]
>> +++ b/tools/libxc/xc_hvm_build_x86.c
>> @@ -21,6 +21,7 @@
>>   #include <stdlib.h>
>>   #include <unistd.h>
>>   #include <zlib.h>
>> +#include <assert.h>
>>
>>   #include "xg_private.h"
>>   #include "xc_private.h"
>> @@ -270,7 +271,7 @@ static int setup_guest(xc_interface *xch,
>>
>>       elf_parse_binary(&elf);
>>       v_start = 0;
>> -    v_end = args->mem_size;
>> +    v_end = args->lowmem_end;
>
> Why is this needed?

This was left to handle something inside modules_init(). But I think 
this change can be removed now.

>>
>>       if ( nr_pages > target_pages )
>>           memflags |= XENMEMF_populate_on_demand;
>> @@ -754,6 +755,8 @@ int xc_hvm_build_target_mem(xc_interface *xch,
>>       args.mem_size = (uint64_t)memsize << 20;
>>       args.mem_target = (uint64_t)target << 20;
>>       args.image_file_name = image_name;
> [...]
>>
>> +static struct xen_reserved_device_memory
>> +*xc_device_get_rdm(libxl__gc *gc,
>> +                   uint32_t flag,
>> +                   uint16_t seg,
>> +                   uint8_t bus,
>> +                   uint8_t devfn,
>> +                   unsigned int *nr_entries)
>> +{
>> +    struct xen_reserved_device_memory *xrdm;
>> +    int rc;
>> +
>> +    rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>> +                                       NULL, nr_entries);
>
> xc_reserved_device_memory_map dereferences nr_entries. You need to make
> sure there is no garbage value in nr_entries. I.e. you need to
> initialise nr_entries to 0 before passing it in.

Sure, so what about this?

/*
  * We really can't presume how many entries we can get in advance.
  */
if (*nr_entries)
     *nr_entries = 0;

>
>> +    assert(rc <= 0);
>> +    /* "0" means we have no any rdm entry. */
>> +    if (!rc)
>> +        goto out;
>> +
>> +    if (errno == ENOBUFS) {
>> +        xrdm = malloc(*nr_entries * sizeof(xen_reserved_device_memory_t));
>> +        if (!xrdm) {
>> +            LOG(ERROR, "Could not allocate RDM buffer!\n");
>> +            goto out;
>> +        }
>> +        rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>> +                                           xrdm, nr_entries);
>> +        if (rc) {
>> +            LOG(ERROR, "Could not get reserved device memory maps.\n");
>> +            *nr_entries = 0;
>> +            free(xrdm);
>> +            xrdm = NULL;
>> +        }
>> +    } else
>> +        LOG(ERROR, "Could not get reserved device memory maps.\n");
>> +
>> + out:
>> +    return xrdm;
>> +}
>> +
>> +/*
>> + * Check whether there exists rdm hole in the specified memory range.
>> + * Returns true if exists, else returns false.
>> + */
>> +static bool overlaps_rdm(uint64_t start, uint64_t memsize,
>> +                         uint64_t rdm_start, uint64_t rdm_size)
>> +{
>> +    return (start + memsize > rdm_start) && (start < rdm_start + rdm_size);
>> +}
>> +
>> +/*
>> + * Check reported RDM regions and handle potential gfn conflicts according
>> + * to user preferred policy.
>> + *
>> + * RMRR can reside in address space beyond 4G theoretically, but we never
>> + * see this in real world. So in order to avoid breaking highmem layout
>> + * we don't solve highmem conflict. Note this means highmem rmrr could still
>> + * be supported if no conflict.
>> + *
>> + * But in the case of lowmem, RMRR probably scatter the whole RAM space.
>> + * Especially multiple RMRR entries would worsen this to lead a complicated
>> + * memory layout. And then its hard to extend hvm_info_table{} to work
>> + * hvmloader out. So here we're trying to figure out a simple solution to
>> + * avoid breaking existing layout. So when a conflict occurs,
>> + *
>> + * #1. Above a predefined boundary (default 2G)
>> + * - Move lowmem_end below reserved region to solve conflict;
>> + *
>> + * #2. Below a predefined boundary (default 2G)
>> + * - Check strict/relaxed policy.
>> + * "strict" policy leads to fail libxl. Note when both policies
>> + * are specified on a given region, 'strict' is always preferred.
>> + * "relaxed" policy issue a warning message and also mask this entry
>> + * INVALID to indicate we shouldn't expose this entry to hvmloader.
>> + */
>
> This looks sensible. Thanks.
>
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 4364ba4..85d74fd 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -1374,6 +1374,9 @@ static void parse_config_data(const char *config_source,
>>       if (!xlu_cfg_get_long (config, "videoram", &l, 0))
>>           b_info->video_memkb = l * 1024;
>>
>> +    if (!xlu_cfg_get_long (config, "rdm_mem_boundary", &l, 0))
>> +        b_info->rdm_mem_boundary_memkb = l * 1024;
>> +
>
> Maybe you need to rearrange this patch series a bit more. The toolstack
> side patches have mixed libxc, libxl and xl changes which is a bit hard
> for me to tell what is needed by what. We can discuss this if you have
> questions.

Okay, just let me try firstly.

Thanks
Tiejun

>
> Wei.
>
>>       if (!xlu_cfg_get_long(config, "max_event_channels", &l, 0))
>>           b_info->event_channels = l;
>>
>> --
>> 1.9.1
>