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

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

Campbell, Jackson, Wei and Stefano,

Any consideration?

I can follow up Jan's idea but I need you guys make sure I'm going to do 
this properly.

Thanks
Tiejun

On 2015/5/7 10:22, Chen, Tiejun wrote:
> 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
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>