Re: [PATCH 13/13] tools/libxl: Add 'vtsc_khz' option to set guest TSC rate

[Julien Grall <julien.grall@citrix.com>]

On 29/09/15 10:50, Haozhong Zhang wrote:
> On Tue, Sep 29, 2015 at 10:20:21AM +0100, Wei Liu wrote:
>> On Tue, Sep 29, 2015 at 08:40:23AM +0800, Haozhong Zhang wrote:
>>> On Mon, Sep 28, 2015 at 03:19:25PM +0100, Wei Liu wrote:
>>>> On Mon, Sep 28, 2015 at 03:13:58PM +0800, Haozhong Zhang wrote:
>>>>> This patch adds an option 'vtsc_khz' to allow users to set vcpu's TSC
>>>>> rate in KHz. In the case that tsc_mode = 'default', the default value of
>>>>> 'vtsc_khz' option is the host TSC rate which is used when 'vtsc_khz'
>>>>> option is set to 0 or does not appear in the configuration. In all other
>>>>> cases of tsc_mode, 'vtsc_khz' option is just ignored.
>>>>>
>>>>> Another purpose of adding this option is to keep vcpu's TSC rate across
>>>>> guest reboot. In existing code, a new domain is created from the
>>>>> configuration of the previous domain which was just rebooted. vcpu's TSC
>>>>> rate is not stored in the configuration and the host TSC rate is the
>>>>> used as vcpu's TSC rate. This works fine unless the previous domain was
>>>>> migrated from another host machine with a different host TSC rate than
>>>>> the current one.
>>>>>
>>>>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>>>>> ---
>>>>>  tools/libxl/libxl_types.idl |  1 +
>>>>>  tools/libxl/libxl_x86.c     |  4 +++-
>>>>>  tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++++
>>>>>  3 files changed, 26 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
>>>>> index 9f6ec00..91cb0be 100644
>>>>> --- a/tools/libxl/libxl_types.idl
>>>>> +++ b/tools/libxl/libxl_types.idl
>>>>> @@ -413,6 +413,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>>>      ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
>>>>>      ("numa_placement",  libxl_defbool),
>>>>>      ("tsc_mode",        libxl_tsc_mode),
>>>>> +    ("vtsc_khz",        uint32),
>>>>
>>>> This field should be in arch-specific substructure, i.e. "hvm".
>>>>
>>>
>>> Julien also pointed out this and suggested to moving to an
>>> arch-specific substructure. I'm going to add a new substructure
>>> "arch_x86" and move "vtsc_khz" there. Is this good for you?
>>>
>>
>> My initial thought was that this was a feature of HVM. I don't recollect
>> why I got that idea. I could be wrong.  Does it work with (or intent to
>> work with) PV too?  If yes, adding it to arch_x86 would be appropriate.
>> If not, hvm specific is good enough.  Please correct my
>> misunderstanding, I'm definitely no expert on x86.  
>>
> 
> No, it only works with HVM. So it should be in 'hvm'? Julien, what is
> your opinion?

I'm fine with having vtsc_khw directly in hvm.

Regards,

-- 
Julien Grall