Re: [RFC][v2][PATCH 04/14] tools/libxl: detect and avoid conflicts with RDM

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

On 2015/6/7 19:20, Wei Liu wrote:
> On Wed, Jun 03, 2015 at 10:25:47AM +0800, Chen, Tiejun wrote:
> [...]
>>>> +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 = NULL;
>>>> +    int rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>>>> +                                           xrdm, nr_entries);
>>>> +
>>>
>>> Please separate declaration and function call. Also change xrdm to NULL
>>
>> Are you saying this?
>>
>>      struct xen_reserved_device_memory *xrdm = NULL;
>>      int rc;
>>
>>      rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>>                                         xrdm, nr_entries);
>
> Yes, splitting "rc = " to a separate line. The other thing is:
>
>       rc = xc_reserved_device_memory_map(CTX->xch, flag, seg, bus, devfn,
>                                          NULL, nr_entries);
> 					^ here
>
> It's mostly cosmetic. IMHO it is clearer than passing xrdm which is
> always NULL in that function call.

Right.

>
>>
>>> in that function call.
>>
>> Sorry, what do you mean by this point? Or you let me to change xrdm to NULL
>> inside xc_reserved_device_memory_map()?
>>
>>>
>>>> +    assert( rc <= 0 );
>>>> +    /* "0" means we have no any rdm entry. */
>>>> +    if ( !rc )
>>>> +        goto out;
>>>
>>> Also set *nr_entries = 0; otherwise you can't distinguish error vs 0
>>> entries.
>>
>> *nr_entries is always updated by xc_reserved_device_memory_map() above.
>>
>
> Actually no. If xc_hypercall_bounce_pre fails in the function,
> nr_entries is untouched.

Sure.

>
>>>
>>>> +
>>>> +    if ( errno == ENOBUFS )
>>>> +    {
>>>> +        if ( (xrdm = malloc(*nr_entries *
>>>> +                            sizeof(xen_reserved_device_memory_t))) == NULL )
> [...]
>>>> +    return -1;
>>>
>>> Please return libxl error code.
>>
>> ERROR_FAIL?
>>
>
> Yes, that's fine.

Thanks
Tiejun

>
> [...]
>>>> +        args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH;
>>>> +    mmio_start = (1ull << 32) - args.mmio_size;
>>>> +
>>>> +    if (args.lowmem_size > mmio_start)
>>>> +        args.lowmem_size = mmio_start;
>>>> +
>>>> +    /*
>>>> +     * We'd like to set a memory boundary to determine if we need to check
>>>> +     * any overlap with reserved device memory.
>>>> +     */
>>>> +    rdm_mem_boundary = 0x80000000;
>>>> +    if (info->rdm_mem_boundary_memkb)
>>>
>>
>> I'm going to update this chunk of codes as follows:
>>
>> #1. @@ -858,6 +858,12 @@ const char *libxl_defbool_to_string(libxl_defbool
>> b);
>>   #define LIBXL_TIMER_MODE_DEFAULT -1
>>   #define LIBXL_MEMKB_DEFAULT ~0ULL
>>
>> +/*
>> + * We'd like to set a memory boundary to determine if we need to check
>> + * any overlap with reserved device memory.
>> + */
>> +#define LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT (2048 * 1024)
>> +
>>   #define LIBXL_MS_VM_GENID_LEN 16
>>   typedef struct {
>>       uint8_t bytes[LIBXL_MS_VM_GENID_LEN];
>>
>>> I think you mean info->rdm_mem_boundary_memkb != LIBXL_MEMKB_DEFAULT?
>>>
>>
>> #2.
>>
>> @@ -109,6 +109,10 @@ void libxl__rdm_setdefault(libxl__gc *gc,
>> libxl_domain_build_info *b_info)
>>   {
>>       if (b_info->rdm.reserve == LIBXL_RDM_RESERVE_FLAG_INVALID)
>>           b_info->rdm.reserve = LIBXL_RDM_RESERVE_FLAG_RELAXED;
>> +
>> +    if (b_info->rdm_mem_boundary_memkb == LIBXL_MEMKB_DEFAULT)
>> +        b_info->rdm_mem_boundary_memkb =
>> +                            LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
>
> This looks plausible.
>
> Wei.
>