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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2741 bytes --]

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.

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

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]