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@xxxxxxxxxx>
...
> @@ -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:
signature.asc
Description: OpenPGP digital signature
