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@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.
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
