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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.14 at 04:07, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/9/2 16:34, Jan Beulich wrote:
>>>>> On 26.08.14 at 13:02, <tiejun.chen@xxxxxxxxx> 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.

> 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,
but I'd also prefer to have input from the tools maintainers here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel