Re: [PATCH] Support for QEMU's CVS

Avi Kivity
2006-12-21T08:49:43Z

Anthony Liguori wrote:
> Howdy,
>
> The attached patch forward ports the KVM patch to QEMU's CVS. The
> only significant change needed was hacking the Bochs BIOS to
> dynamically disable SMM support if KVM is enabled. This was done by
> using one of the Bochs DEBUG ports. Probably not the best long term
> solution but it works.
>
> The patch is minimal (no VMDK changes, no migration, etc.). This is
> to ease upstream integration.
>
> I am able to boot both a Windows and FC5 guest (under SVM).
>
> You'll need the BIOS (binary file can't be included in diff) from:
>
> http://www.cs.utexas.edu/~aliguori/bios.bin
>
> I reckon some changes still need to be made for CVS inclusion but I
> think it's at least appropriate to send to qemu-devel and begin the
> review process.

There are four issues I can see:

- if there are multiple sources of kvm userspace, there are bound to be
problems with incompatible kernel API and userspace, especially as I
have plans for extensive changes to the API. I'll go and implement a
version check now, so that doesn't become a barrier to anything. A
submission to qemu-devel should include this check.

- the API changes are bound to cause more churn than necessary. I guess
we can live with that though.

- the patch contains some qemu changes that date from the days we did
V2E, and are therefore unnecessary. I'll point them out below.

- where does libkvm live? an external package?

> diff -r 2eac80033ea0 hw/usb-hid.c
> --- a/hw/usb-hid.c Tue Dec 19 09:31:34 2006 +0000
> +++ b/hw/usb-hid.c Wed Dec 20 23:01:25 2006 -0600
> @@ -169,7 +169,9 @@ static const uint8_t qemu_tablet_config_
> 0x81, /* u8 ep_bEndpointAddress; IN Endpoint 1 */
> 0x03, /* u8 ep_bmAttributes; Interrupt */
> 0x08, 0x00, /* u16 ep_wMaxPacketSize; */
> - 0x03, /* u8 ep_bInterval; (255ms -- usb 2.0 spec) */
> + /* Temporarily increase usb polling interval to prevent cpu
> + * saturation (3 ms is way too often for kvm) */
> + 0x33, /* u8 ep_bInterval; (255ms -- usb 2.0 spec) */
>

This might be a good change for qemu, but on the other hand this is a
kvm deficiency that is forced on non-kvm users.

> diff -r 2eac80033ea0 target-i386/helper.c
> --- a/target-i386/helper.c Tue Dec 19 09:31:34 2006 +0000
> +++ b/target-i386/helper.c Wed Dec 20 23:01:25 2006 -0600
> @@ -184,7 +186,15 @@ static inline void get_ss_esp_from_tss(u
> if (!(env->tr.flags & DESC_P_MASK))
> cpu_abort(env, "invalid tss");
> type = (env->tr.flags >> DESC_TYPE_SHIFT) & 0xf;
> +#ifdef USE_KVM
> + /*
> + * Bit 1 is the Busy bit. We believe it is legal to interrupt into a busy
> + * segment
> + */
> + if ((kvm_allowed && (type & 5) != 1) || (type & 7) != 1)
> +#else
> if ((type & 7) != 1)
> +#endif
>

This is unnecessary now. However I also think it is correct (e.g. the
#else part can be eliminated)

> +#ifdef USE_KVM
> + /* Probable qemu bug: 11 is a valid segment type */
> + ((env->tr.flags >> DESC_TYPE_SHIFT) & 0xd) != 9 ||
> +#else
> ((env->tr.flags >> DESC_TYPE_SHIFT) & 0xf) != 9 ||
> +#endif
>

Ditto.

> env->tr.limit < 103)
> goto fail;
> io_offset = lduw_kernel(env->tr.base + 0x66);
> @@ -839,6 +854,13 @@ static void do_interrupt64(int intno, in
> uint32_t e1, e2, e3, ss;
> target_ulong old_eip, esp, offset;
>
> +#ifdef USE_KVM
> + if (kvm_allowed) {
> + printf("%s: unexpect\n", __FUNCTION__);
> + exit(-1);
> + }
> +#endif
> +
>

Unnecessary, I think.

> has_error_code = 0;
> if (!is_int && !is_hw) {
> switch(intno) {
> @@ -1122,6 +1144,12 @@ void do_interrupt_user(int intno, int is
> int dpl, cpl;
> uint32_t e2;
>
> +#ifdef USE_KVM
> + if (kvm_allowed) {
> + printf("%s: unexpect\n", __FUNCTION__);
> + exit(-1);
> + }
> +#endif
>

Ditto.

> dt = &env->idt;
> ptr = dt->base + (intno * 8);
> e2 = ldl_kernel(ptr + 4);
> @@ -1147,6 +1175,12 @@ void do_interrupt(int intno, int is_int,
> void do_interrupt(int intno, int is_int, int error_code,
> target_ulong next_eip, int is_hw)
> {
> +#ifdef USE_KVM
> + if (kvm_allowed) {
> + printf("%s: unexpect\n", __FUNCTION__);
> + exit(-1);
> + }
> +#endif
>

Ditto.

> if (loglevel & CPU_LOG_INT) {
> if ((env->cr[0] & CR0_PE_MASK)) {
> static int count;
> @@ -1958,6 +1992,12 @@ void helper_ljmp_protected_T0_T1(int nex
> cpu_x86_load_seg_cache(env, R_CS, (new_cs & 0xfffc) | cpl,
> get_seg_base(e1, e2), limit, e2);
> EIP = new_eip;
> +#ifdef USE_KVM
> + if (kvm_allowed && (e2 & DESC_L_MASK)) {
> + env->exception_index = -1;
> + cpu_loop_exit();
> + }
> +#endif
>

Ditto.

--
error compiling committee.c: too many arguments to function