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

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

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?

Or just one is already fine?

>
>> @@ -370,6 +375,9 @@ static int setup_guest(xc_interface *xch,
>>       rc = check_rmrr_overlap(xch, mmio_start, mmio_start);
>
> I skipped several tools side patches, but here I see that somewhere
> you still left the term "RMRR" in the code, when you were asked
> before to use more abstract naming (and this of course not only
> extended to the public interface).

I will replace those info with 'device reserved memory maps' and s/RMRR/DRM.

>
>>       if ( rc < 0 )
>>           goto error_out;
>> +    /* Always return entries number. */
>> +    else
>
> The "else" here is bogus considering the "goto" above.

Remove this 'else'.

>
>> --- 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.


Thanks
Tiejun