Re: [v8][PATCH 06/17] tools/libxc: check if modules space is overlapping with reserved device memory

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

On 2014/12/3 3:55, Konrad Rzeszutek Wilk wrote:
> On Mon, Dec 01, 2014 at 05:24:24PM +0800, Tiejun Chen wrote:
>> In case of reserved device memory overlapping with ram, it also probably
>
> s/also//

Fixed.

>> overlap with modules space so we need to check these reserved device
> s/overlap/overlaps/

Fixed.

>
> What is 'modules space'?

Please see modules_init(), and looks it includs acpi and smbios currently.

>
>> memory as well.
>
> s/reserved device memory/E820_RSV/ ?

I don't find we have such a definition.

>
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxc/xc_hvm_build_x86.c | 94 +++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 79 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
>> index c81a25b..ddcf06d 100644
>> --- a/tools/libxc/xc_hvm_build_x86.c
>> +++ b/tools/libxc/xc_hvm_build_x86.c
>> @@ -54,9 +54,82 @@
>>
>>   #define VGA_HOLE_SIZE (0x20)
>>
>> +/*
>> + * Check whether there exists mmio hole in the specified memory range.
>> + * Returns 1 if exists, else returns 0.
>> + */
>> +static int check_mmio_hole(uint64_t start, uint64_t memsize,
>> +                           uint64_t mmio_start, uint64_t mmio_size)
>> +{
>> +    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
>> +        return 0;
>> +    else
>> +        return 1;
>> +}
>> +
>> +/* Getting all reserved device memory map info. */
>> +static struct xen_reserved_device_memory
>> +*xc_get_reserved_device_memory_map(xc_interface *xch, unsigned int nr_entries,
>> +                                   uint32_t dom)
>> +{
>> +    struct xen_reserved_device_memory *xrdm = NULL;
>> +    int rc = xc_reserved_device_memory_map(xch, dom, xrdm, &nr_entries);
>> +
>> +    if ( rc < 0 )
>> +    {
>> +        if ( errno == ENOBUFS )
>> +        {
>> +            if ( (xrdm = malloc(nr_entries *
>> +                                sizeof(xen_reserved_device_memory_t))) == NULL )
>> +            {
>> +                PERROR("Could not allocate memory.");
>> +                return 0;
>> +            }
>> +            rc = xc_reserved_device_memory_map(xch, dom, xrdm, &nr_entries);
>> +            if ( rc )
>> +            {
>> +                PERROR("Could not get reserved device memory maps.");
>> +                free(xrdm);
>> +                return 0;
>
> Uhhh, is that the right error to return?
>
> Don't you mean ERR_PTR logic? Or 'return NULL' ?

OOPS, return NULL.

>
>
>> +            }
>> +        }
>> +        else
>> +            PERROR("Could not get reserved device memory maps.");
>> +    }
>> +
>> +    return xrdm;
>> +}
>> +
>> +static int xc_check_modules_space(xc_interface *xch, uint64_t *mstart_out,
>> +                                  uint64_t *mend_out, uint32_t dom)
>> +{
>> +    unsigned int i = 0, nr_entries = 0;
>> +    uint64_t rdm_start = 0, rdm_end = 0;
>> +    struct xen_reserved_device_memory *rdm_map =
>> +                        xc_get_reserved_device_memory_map(xch, nr_entries, dom);
>> +
>
> You need to check whether 'rdm_map' is NULL.

You're right.

Actually nr_entries is always 0 if rdm_map is NULL with my original 
design. But now this should be checked as you mentioned, so

+    if ( !rdm_map )
+        return 0;
+

>
>> +    for ( i = 0; i < nr_entries; i++ )
>> +    {
>> +        rdm_start = (uint64_t)rdm_map[i].start_pfn << XC_PAGE_SHIFT;
>> +        rdm_end = rdm_start + ((uint64_t)rdm_map[i].nr_pages << XC_PAGE_SHIFT);
>> +
>> +        /* Just use check_mmio_hole() to check modules ranges. */
>> +        if ( check_mmio_hole(rdm_start,
>> +                             rdm_end - rdm_start,
>> +                             *mstart_out, *mend_out) )
>> +            return -1;
>> +    }
>> +
>> +    free(rdm_map);
>> +
>> +    return 0;
>> +}
>> +
>>   static int modules_init(struct xc_hvm_build_args *args,
>>                           uint64_t vend, struct elf_binary *elf,
>> -                        uint64_t *mstart_out, uint64_t *mend_out)
>> +                        uint64_t *mstart_out, uint64_t *mend_out,
>> +                        xc_interface *xch,
>> +                        uint32_t dom)
>>   {
>>   #define MODULE_ALIGN 1UL << 7
>>   #define MB_ALIGN     1UL << 20
>> @@ -80,6 +153,10 @@ static int modules_init(struct xc_hvm_build_args *args,
>>       if ( *mend_out > vend )
>>           return -1;
>>
>> +    /* Is it overlapping with reserved device memory? */
>> +    if ( xc_check_modules_space(xch, mstart_out, mend_out, dom) )
>> +        return -1;
>> +
>>       if ( args->acpi_module.length != 0 )
>>           args->acpi_module.guest_addr_out = *mstart_out;
>>       if ( args->smbios_module.length != 0 )
>> @@ -226,19 +303,6 @@ static int loadmodules(xc_interface *xch,
>>       return rc;
>>   }
>>
>> -/*
>> - * Check whether there exists mmio hole in the specified memory range.
>> - * Returns 1 if exists, else returns 0.
>> - */
>> -static int check_mmio_hole(uint64_t start, uint64_t memsize,
>> -                           uint64_t mmio_start, uint64_t mmio_size)
>> -{
>> -    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
>> -        return 0;
>> -    else
>> -        return 1;
>> -}
>> -
>
> This movement of 'check_mmio_hole' needs to be a seperate patch.

Okay.

Thanks
Tiejun

>
>>   static int setup_guest(xc_interface *xch,
>>                          uint32_t dom, struct xc_hvm_build_args *args,
>>                          char *image, unsigned long image_size)
>> @@ -282,7 +346,7 @@ static int setup_guest(xc_interface *xch,
>>           goto error_out;
>>       }
>>
>> -    if ( modules_init(args, v_end, &elf, &m_start, &m_end) != 0 )
>> +    if ( modules_init(args, v_end, &elf, &m_start, &m_end, xch, dom) != 0 )
>>       {
>>           ERROR("Insufficient space to load modules.");
>>           goto error_out;
>> --
>> 1.9.1
>>
>