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

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

On 2015/5/6 23:34, Jan Beulich wrote:
>>>> On 06.05.15 at 17:00, <tiejun.chen@intel.com> wrote:
>> On 2015/4/20 19:13, Jan Beulich wrote:
>>>>>> On 10.04.15 at 11:21, <tiejun.chen@intel.com> wrote:
>>>> --- a/tools/libxc/xc_domain.c
>>>> +++ b/tools/libxc/xc_domain.c
>>>> @@ -1665,6 +1665,46 @@ int xc_assign_device(
>>>>        return do_domctl(xch, &domctl);
>>>>    }
>>>>
>>>> +struct xen_reserved_device_memory
>>>> +*xc_device_get_rdm(xc_interface *xch,
>>>> +                   uint32_t flag,
>>>> +                   uint16_t seg,
>>>> +                   uint8_t bus,
>>>> +                   uint8_t devfn,
>>>> +                   unsigned int *nr_entries)
>>>> +{
>>>
>>> So what's the point of having both this new function and
>>> xc_reserved_device_memory_map()? Is the latter useful for
>>> anything besides the purpose here?
>>
>> I just hope xc_reserved_device_memory_map() is a standard interface to
>> call that XENMEM_reserved_device_memory_map, but xc_device_get_rdm() can
>> handle some errors in current case.
>>
>> I think you are hinting we just need one, right?
>
> Correct. But remember - I'm not a maintainer of this code, so

But this may be a little complex with one...

> maintainers may be of different opinion.

Anyway, let me ask our tools maintainers.

Campbell, Jackson, Wei and Stefano,

What about your concern to this?

>
>>>> +    struct xen_reserved_device_memory *xrdm = NULL;
>>>> +    int rc = xc_reserved_device_memory_map(xch, flag, seg, bus, devfn, 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.");
>>>
>>> Now that's exactly the kind of error message that makes no sense:
>>> As errno will already cause PERROR() to print something along the
>>> lines of the message you provide here, you're just creating
>>> redundancy. Indicating the purpose of the allocation, otoh, would
>>> add helpful context for the one inspecting the resulting log.
>>
>> What about this?
>>
>> PERROR("Could not allocate memory buffers to store reserved device
>> memory entries.");
>
> You kind of go from one extreme to the other - the message
> doesn't need to be overly long, but it should be distinct from
> all other messages (so that when seen one can identify what
> went wrong).

I originally refer to some existing examples like this,

int
xc_core_arch_memory_map_get(xc_interface *xch, struct 
xc_core_arch_context *unused,
                             xc_dominfo_t *info, shared_info_any_t 
*live_shinfo,
                             xc_core_memory_map_t **mapp,
                             unsigned int *nr_entries)
{
     ...
     map = malloc(sizeof(*map));
     if ( map == NULL )
     {
         PERROR("Could not allocate memory");
         return -1;
     }

Maybe this is wrong to my case. Could I change this?

PERROR("Could not allocate memory for XENMEM_reserved_device_memory_map 
hypercall");

Or just give me your line.

>
>>>> @@ -302,8 +300,11 @@ static int setup_guest(xc_interface *xch,
>>>>
>>>>        for ( i = 0; i < nr_pages; i++ )
>>>>            page_array[i] = i;
>>>> -    for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
>>>> -        page_array[i] += mmio_size >> PAGE_SHIFT;
>>>> +    /*
>>>> +     * This condition 'lowmem_end <= mmio_start' is always true.
>>>> +     */
>>>
>>> For one I think you mean "The", not "This", as there's no such
>>> condition around here. And then - why? DYM "is supposed to
>>> always be true"? In which case you may want to check...
>>
>> I always do this inside libxl__build_hvm() but before setup_guest(),
>>
>> +    if (args.lowmem_size > mmio_start)
>> +        args.lowmem_size = mmio_start;
>>
>> And plus, we also another policy to rdm,
>>
>>       #1. Above a predefined boundary (default 2G)
>>           - move lowmem_end below reserved region to solve conflict;
>>
>> This means there's such a likelihood of args.lowmem_size < mmio_start)
>> as well.
>>
>> So here I'm saying the condition is always true.
>
> Okay, but again - if this is relevant to the following code, an
> assertion or alike may still be warranted.

Yes I should add 'assert()' here.

>
>>> and hence don't have the final say on stylistic issues, I don't see
>>> why the above couldn't be expressed with a single return statement.
>>
>> Are you saying something like this? Note this was showed by yourself
>> long time ago.
>
> I know, and hence I was puzzled to still see you use the more
> convoluted form.
>
>> static bool check_mmio_hole_conflict(uint64_t start, uint64_t memsize,
>>                                         uint64_t mmio_start, uint64_t mmio_size)
>> {
>>        return start + memsize > mmio_start && start < mmio_start + mmio_size;
>> }
>>
>> But I don't think this really can't work out our case.
>
> It's equivalent to the original you had, so I don't see what you
> mean with "this really can't work out our case".
>

Let me make this point clear.

The original implementation,

+static int check_rdm_hole(uint64_t start, uint64_t memsize,
+                          uint64_t rdm_start, uint64_t rdm_size)
+{
+    if (start + memsize <= rdm_start || start >= rdm_start + rdm_size)
+        return 0;
+    else
+        return 1;
+}

means it returns 'false' in two cases:

#1. end = start + memsize; end <= rdm_start;

This region [start, end] is below of rdm entry.

#2. rdm_end = rdm_start + rdm_size; stat >= rdm_end;

This region [start, end] is above of rdm entry.

So others conditions should indicate that rdm entry is overlapping with 
this region. Actually this has three cases:

#1. This region just conflicts with the first half of rdm entry;
#2. This region just conflicts with the second half of rdm entry;
#3. This whole region falls inside of rdm entry;

Then it should return 'true', right?

But with this single line,

return start + memsize > rdm_start && start < rdm_start + rdm_size;

=>

return end > rdm_start && start < rdm_end;

This just guarantee it return 'true' *only* if #3 above occurs.

>>>> +int libxl__domain_device_check_rdm(libxl__gc *gc,
>>>> +                                   libxl_domain_config *d_config,
>>>> +                                   uint64_t rdm_mem_guard,
>>>> +                                   struct xc_hvm_build_args *args)
>>>> +{
>>>> +    int i, j, conflict;
>>>> +    libxl_ctx *ctx = libxl__gc_owner(gc);
>>>> +    struct xen_reserved_device_memory *xrdm = NULL;
>>>> +    unsigned int nr_all_rdms = 0;
>>>> +    uint64_t rdm_start, rdm_size, highmem_end = (1ULL << 32);
>>>> +    uint32_t type = d_config->b_info.rdm.type;
>>>> +    uint16_t seg;
>>>> +    uint8_t bus, devfn;
>>>> +
>>>> +    /* Might not to expose rdm. */
>>>> +    if ((type == LIBXL_RDM_RESERVE_TYPE_NONE) && !d_config->num_pcidevs)
>>>> +        return 0;
>>>> +
>>>> +    /* Collect all rdm info if exist. */
>>>> +    xrdm = xc_device_get_rdm(ctx->xch, LIBXL_RDM_RESERVE_TYPE_HOST,
>>>> +                             0, 0, 0, &nr_all_rdms);
>>>
>>> What meaning has passing a libxl private value to a libxc function?
>>
>> We intend to collect all rdm entries info in advance and then we can
>> construct d_config->rdms based on our policies as follows. Because we
>> need to first allocate d_config->rdms properly to store rdms, but in
>> some cases we don't know how many buffers are enough. For example, we
>> don't have that global flag but with multiple pci devices. And even a
>> shared entry worsen this situation.
>>
>> So here, we set that flag as LIBXL_RDM_RESERVE_TYPE_HOST but without any
>> SBDF to grab all rdms.
>
> I'm afraid you didn't get my point: Values passed to libxc should be

Sorry for this misunderstanding.

> known to libxc. Values privately defined by libxl for its own purposes
> aren't known to libxc, and hence shouldn't be passed to libxc
> functions.

I think we should set this with 'PCI_DEV_RDM_ALL' since,

struct xen_reserved_device_memory_map {
     /* IN */
     /* Currently just one bit to indicate checkng all Reserved Device 
Memory. */
#define PCI_DEV_RDM_ALL   0x1

>
>>>> +     * 'try' policy is specified, and we also mark this as INVALID not to expose
>>>> +     * this entry to hvmloader.
>>>
>>> What is "this" in "... also mark this as ..."? Certainly neither the conflict
>>> nor the warning.
>>
>> Sorry, this is my fault.
>>
>>        * If a conflict is detected on a given RMRR entry, an error will be
>>        * returned if 'strict' policy is specified. Or conflict is treated as a
>>        * warning if 'relaxed' policy is specified, and we also mark this as
>>        * INVALID not to expose this entry to hvmloader.
>
> The same "this" still doesn't have anything reasonable it references. I
> think you mean "the entry" (in which case the subsequent "this entry"
> could become just "it" afaict). But (not being a native speaker) the
> grammar of the second half of the sentence looks odd (and hence
> potentially confusing) to me anyway (i.e. even with the previous

Sure, we need to make this better and clear.

> issue fixed).

      * If a conflict is detected on a given RMRR entry, an error will be
      * returned if 'strict' policy is specified. Instead, if 'relaxed' 
policy
      * specified, this conflict is treated just as a warning, but we 
mark this
      * RMRR entry as INVALID to indicate that this entry shouldn't be 
exposed
      * to hvmloader.

I hope this can help us understand what we do.

>
>>>> +     *
>>>> +     * Firstly we should check the case of rdm < 4G because we may need to
>>>> +     * expand highmem_end.
>>>> +     */
>>>> +    for (i = 0; i < d_config->num_rdms; i++) {
>>>> +        rdm_start = d_config->rdms[i].start;
>>>> +        rdm_size = d_config->rdms[i].size;
>>>> +        conflict = check_rdm_hole(0, args->lowmem_size, rdm_start, rdm_size);
>>>> +
>>>> +        if (!conflict)
>>>> +            continue;
>>>> +
>>>> +        /*
>>>> +         * Just check if RDM > our memory boundary
>>>> +         */
>>>> +        if (d_config->rdms[i].start > rdm_mem_guard) {
>>>> +            /*
>>>> +             * We will move downwards lowmem_end so we have to expand
>>>> +             * highmem_end.
>>>> +             */
>>>> +            highmem_end += (args->lowmem_size - rdm_start);
>>>> +            /* Now move downwards lowmem_end. */
>>>> +            args->lowmem_size = rdm_start;
>>>
>>> Considering that the action here doesn't depend on the specific
>>> ->rdms[] slot being looked at, I don't see why the loop needs to
>>
>> I'm not sure if I understand what you mean.
>>
>> All rdm entries are organized disorderly in d_config->rdms, so we should
>> traverse all entries to make sure args->lowmem_size is below all rdms'
>> start address.
>
> I think I see what confused me: in the if() condition you reference
> d_config->rdms[i].start, yet the body of the if() has no reference
> to d_config->rdms[i] at all. If the if() used rdm_start it would
> become obvious that this is being latched at the beginning of the

Indeed, I really should use rdm_start here.

> body (which is what I overlooked, assuming the variable's value
> to have got set prior to the loop), and hence the body is not loop
> invariant.
>

So just replace d_config->rdms[i].start with rdm_start like this,

         /*
          * Just check if RDM > our memory boundary
          */
         if (rdm_start > rdm_mem_guard) {
             /*
              * We will move downwards lowmem_end so we have to expand
              * highmem_end.
              */
             highmem_end += (args->lowmem_size - rdm_start);
             /* Now move downwards lowmem_end. */
             args->lowmem_size = rdm_start;
         }
     }

Thanks
Tiejun