|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC][PATCH 04/13] tools/libxl: detect and avoid conflicts with RDM
>>> On 06.05.15 at 17:00, <tiejun.chen@xxxxxxxxx> wrote:
> On 2015/4/20 19:13, Jan Beulich wrote:
>>>>> On 10.04.15 at 11:21, <tiejun.chen@xxxxxxxxx> 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
maintainers may be of different opinion.
>>> + 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).
>>> @@ -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.
>> 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".
>>> +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
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.
>>> + * '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
issue fixed).
>>> + *
>>> + * 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
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.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |