Re: [v5][PATCH 06/10] hvm_info_table: introduce nr_reserved_device_memory_map

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

On 2014/9/4 14:32, Jan Beulich wrote:
>>>> On 04.09.14 at 04:07, <tiejun.chen@intel.com> wrote:
>> On 2014/9/2 16:34, Jan Beulich wrote:
>>>>>> On 26.08.14 at 13:02, <tiejun.chen@intel.com> wrote:
>>>> libxc can expose how many reserved device memory entries
>>>> hvmloader should get. And '0' means that doesn't exist so
>>>> we can skip this check.
>>>
>>> I assume you introduce this without consumer to limit patch size. In
>>> such a case title _and_ description should be more meaningful as
>>> to what this really does and what it's intended use is.
>>
>> This patch involves two files, one is xen file,
>> xen/include/public/hvm/hvm_info_table.h, and a tools file,
>> tools/libxc/xc_hvm_build_x86.c.
>>
>> So I'm considering to split with two small patches like this:
>>
>> #1 Introduce this new field in xen/include/public/hvm/hvm_info_table.h
>>
>> xen/hvm_info_table: introduce a new field nr_device_reserved_memory_map
>>
>> In hvm_info_table this field represents the number of all device memory
>> maps. It will be convenient to expose such a information to VM.
>>
>> #2 Construct this field in tools/libxc/xc_hvm_build_x86.c
>>
>> tools/libxc: construct nr_device_reserved_memory_map
>>
>> While building hvm info, libxc is responsible for constructing
>> this number after check_drm_overlap().
>>
>> Is it reasonable to use different patches covering xen internal and
>> tools, respectively?
>
> I don't see the "xen internal" here. As said this is an interface
> between tool stack and hvmloader, both of which are tools
> components.

Understood.

>
>> Or just one is already fine?
>
> I think so.
>
>>>> --- a/xen/include/public/hvm/hvm_info_table.h
>>>> +++ b/xen/include/public/hvm/hvm_info_table.h
>>>> @@ -67,6 +67,9 @@ struct hvm_info_table {
>>>>
>>>>        /* Bitmap of which CPUs are online at boot time. */
>>>>        uint8_t     vcpu_online[(HVM_MAX_VCPUS + 7)/8];
>>>> +
>>>> +    /* How many reserved device memory does this domain have? */
>>>> +    uint32_t    nr_reserved_device_memory_map;
>>>>    };
>>>
>>> This being defacto a private interface between tools and hvmloader
>>> I wonder if it wouldn't be better to put this before the (in the future)
>>> eventually growing vcpu_online[].
>>>
>>
>> Any latest consideration? Could we push line above 'uint8_t
>> vcpu_online[(HVM_MAX_VCPUS + 7)/8];'?
>>
>> And I just think it may be reasonable and convenient to expose this info
>> in hvm_info_table since this is a fixed value that we should record for
>> all VMs.
>
> I think information easily obtained via hypercall doesn't belong here,

Yes, but just don't feel better to call twice hypercall in hvmloader 
since this means we have to reallocate memory to store all entries.

This is a little bit different what we did in libxc since we can't get 
that number of all entries directly in libxc. But here it may be fine.

> but I'd also prefer to have input from the tools maintainers here.
>

I think I already include all guys by get_maintainer.pc here, but maybe 
I need ping them actively, so you think who can give us a ack or nack 
eventually?

Thanks
Tiejun