Re: [PATCH 2/2][RFC] Kernel changes for HPET legacy mode (v7)

[Beth Kon <eak@us.ibm.com>]

Jan Kiszka wrote:
> Beth Kon wrote:
>   
>> When kvm is in hpet_legacy_mode, the hpet is providing the 
>> timer interrupt and the pit should not be. So in legacy mode, the pit timer is
>> destroyed, but the *state* of the pit is maintained. So if kvm or the guest
>> tries to modify the state of the pit, this modification is accepted, *except* 
>> that the timer isn't actually started. When we exit hpet_legacy_mode, 
>> the current state of the pit (which is up to date since we've been 
>> accepting modifications) is used to restart the pit timer.
>>
>> The saved_mode code in kvm_pit_load_count temporarily changes mode to 
>> 0xff in order to destroy the timer, but then restores the actual value,
>> again maintaining "current" state of the pit for possible later reenablement.
>>
>> changes from v6:
>>
>> - Added ioctl interface for legacy mode in order not to break the abi.
>>
>>
>> Signed-off-by: Beth Kon <eak@us.ibm.com>
>>     
>
> ...
>
>   
>> @@ -1986,7 +1987,24 @@ static int kvm_vm_ioctl_set_pit(struct kvm *kvm, struct kvm_pit_state *ps)
>>  	int r = 0;
>>  
>>  	memcpy(&kvm->arch.vpit->pit_state, ps, sizeof(struct kvm_pit_state));
>> -	kvm_pit_load_count(kvm, 0, ps->channels[0].count);
>> +	kvm_pit_load_count(kvm, 0, ps->channels[0].count, 0);
>> +	return r;
>> +}
>> +
>> +static int kvm_vm_ioctl_get_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
>> +{
>> +	int r = 0;
>> +	*mode = kvm->arch.vpit->pit_state.hpet_legacy_mode;
>> +	return r;
>> +}
>>     
>
> This only applies if we go for a separate mode IOCTL:
> The legacy mode is not directly modifiable by the guest. Is it planned
> to add in-kernel hpet support? Otherwise get_hpet_legacy_mode looks a
> bit like overkill given that user space could easily track the state.
>   
Assuming I will at least generalize the ioctl, I'll leave this question 
for the time being.
>   
>> +
>> +static int kvm_vm_ioctl_set_hpet_legacy_mode(struct kvm *kvm, u8 *mode)
>> +{
>> +	int r = 0, start = 0;
>> +	if (kvm->arch.vpit->pit_state.hpet_legacy_mode == 0 && *mode == 1)
>>     
>
> Here you check more mode == 1, but legacy_mode is only checked for != 0.
> I would make this consistent.
>
>   
ok
>> +		start = 1;
>> +	kvm->arch.vpit->pit_state.hpet_legacy_mode = *mode;
>> +	kvm_pit_load_count(kvm, 0, kvm->arch.vpit->pit_state.channels[0].count, start);
>>  	return r;
>>  }
>>  
>> @@ -2047,6 +2065,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>  		struct kvm_pit_state ps;
>>  		struct kvm_memory_alias alias;
>>  		struct kvm_pit_config pit_config;
>> +		u8 hpet_legacy_mode;
>>     
>
> Hmm, stead of introducing a new pair of singe-purpose IOCTLs, why not
> add KVM_GET/SET_PIT2 which exchanges an extended kvm_pit_state2. And
> that struct should also include some flags field and enough padding to
> be potentially extended yet again in the future. In that case I see no
> problem having also a mode read-back interface.
>
>   
I thought about that, but it seemed to add unnecessary complexity, since 
this legacy control is really outside of normal PIT operation, which is 
embodied by KVM_GET/SET_PIT. It might be worth making this ioctl more 
general. Rather than SET_HPET_LEGACY, have SET_PIT_CONTROLS and pass a 
bit field, one of which is HPET_LEGACY. But, if general consensus is 
that it would be better to create a kvm_pit_state2, and get/set that, I 
can do that.
> Jan
>
>