Re: [Xen-devel] [PATCH v3 12/13] x86/altp2m: Add altp2mhvm HVM domain parameter.

[Ed White <edmund.h.white@xxxxxxxxx>]

On 07/06/2015 11:18 AM, Wei Liu wrote:
> On Mon, Jul 06, 2015 at 11:01:27AM -0700, Ed White wrote:
>> On 07/06/2015 10:49 AM, Wei Liu wrote:
>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>> index 23f27d4..66a89cf 100644
>>>> --- a/tools/libxl/libxl_types.idl
>>>> +++ b/tools/libxl/libxl_types.idl
>>>> @@ -437,6 +437,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>>                                         ("mmio_hole_memkb",  MemKB),
>>>>                                         ("timer_mode",       
>>>> libxl_timer_mode),
>>>>                                         ("nested_hvm",       
>>>> libxl_defbool),
>>>> +                                       ("altp2mhvm",        
>>>> libxl_defbool),
>>>
>>> It's redundant to have "hvm" in the name of this field. Calling it
>>> "altp2m" would be fine IMHO.
>>>
>>
>> When I originally started writing this code, I modelled the naming
>> and some of the structure on nestedhvm, which is why so many things
>> had hvm in the name. I've now removed a lot of those, but in this
>> instance I wonder if doing so would cause confusion, since we have
>> a Xen command-line parameter called altp2m.
>>
> 
> I don't think it will cause confusion. This is part of guest
> configuration which has nothing to do with Xen and in theory another
> name space.
> 
> I asked you to remove hvm because it's redundant -- that field is a
> sub-field of u.hvm already.
> 

My mistake, I hadn't appreciated that you were only suggesting
renaming the field, not renaming the string in the .cfg.

I've addressed your other feedback for the next version of the
series. I added LIBXL_HAVE_ALTP2M, even though libxl only
supports interpreting the domain parameter in our patches.
Tamas has more libxl support in his patches, which have not
yet been submitted.

Ed

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