[patch 0/2] Fix rawhide FV booting on VMX

[patch 0/2] Fix rawhide FV booting on VMX

[Stephen C. Tweedie]

Hi all,

The following two patches --- one to the HV, one to the tools --- allow
current rawhide/FC7 iso images to boot under Xen full virtualisation on
VMX.

The bug is due to Xen's insanely convoluted way of dealing with 16-bit
code on Intel VMX.  The vmxassist layer needs to be able to respond to
CPU traps properly.  To be able to distinguish between traps being sent
to the vmxassist, and interrupts being delivered to the 16-bit code
being sub-emulated, it remaps the 16-bit guest's IRQs to generate
different interrupt numbers.  This is really really ugly --- whenever
the 16-bit guest performs IO to the (virtual) PIC, vmxassist watches to
see if it's a request to reprogram the PIC interrupt vector; and if so,
rewrites the outb so that the PIC vector is 32 (master PIC) or 40 (slave
PIC). vmxassist then installs 32-bit trap handlers at those offsets in
the IDT, and those handlers bounce the interrupt back into 16-bit mode
at the appropriate vector for the guest (which is *FORCED* to be 8
(master) or 0x70 (slave), the int handlers which the hvmloader rombios
sets up.)

All of this works (barely) while we're in 16-bit mode.  But when the
bootloader transitions into 32-bit mode, vmxassist gracefully steps out
of the way, but the guest's PIC vectors still point to the vmxassist
interrupts (32/40), not the rombios ones (8/0x70).  So if an interrupt
is delivered in this state, it triggers the wrong trap in the guest.

Now, the FC7 iso boot loader has a 32-bit graphical menu module loaded
by syslinux.  That 32-bit code installs interrupt traps to bounce all
received interrupts back down to 16-bit mode.  So it relies heavily on
the PIC vectors set up by 16-bit mode still being valid while in 32-bit
mode.

vmxassist can't really fix this on its own.  The only way to reprogram
the guest's virtual PIC is to reinitialise the vPIC, but doing so throws
away all sorts of important vPIC state (some of it recoverable, like the
initialisation modes; some of it not recoverable, such as edge-trigger
latches.)

So the fix here is to first of all extend the virtual PIC provided by
the hypervisor, supporting a new 2-byte control sequence which lets the
guests change the interrupt vectors _without_ fully reinitialising the
vPIC; and secondly to have vmxassist use that sequence to reset the vPIC
vectors appropriately whenever we transition between 16 and 32-bit mode.

This has been verified to allow rawhide installs to proceed under Xen
FV.  

Points for discussion:

* Do we need/want vmxassist to fail gracefully if the enhanced vPIC
sequence is not available?
* Is the magic sequence used here (0xff written to ICW1) genuinely
impossible for other guests to use? (see patch 1/2 to follow.)

--Stephen

[patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Stephen C. Tweedie]

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

Hi,

> So the fix here is to first of all extend the virtual PIC provided by
> the hypervisor, supporting a new 2-byte control sequence which lets the
> guests change the interrupt vectors _without_ fully reinitialising the
> PIC

And this is the patch for that.  The new control sequence is:

Write 0xff to ICW1: this is an otherwise unused value, since it has bit
4 set (ie. it's an initialisation register write), and yet bits 5--7 are
also set (only valid for MCS-80 mode, which the Xen vPIC doesn't emulate
at all.)  This sets the PIC to expect ICW2 to follow, just as ICW1
usually does, except that it does not reset the PIC internal state.

Write the new vector to ICW2: completes the initialisation sequence.
This the same as a normal ICW2 write, except that it does not expect to
be followed by the usual ICW3/ICW4 writes.

Note that the flag used to indicate this "special" ICW1 write is shared
between the two vPICs (master and slave), so it is not legal for the
guest to write 0xff to both master/slave ICW1 before following up with
the ICW2 writes.  But since the only software that will use this is the
modified vmxassist, that's OK.

--Stephen



[-- Attachment #2: Type: text/x-patch, Size: 1308 bytes --]

--- xen-3.1.0-rc7-7041b52471c3/xen/arch/x86/hvm/vpic.c.~1~	2007-05-03 17:49:31.000000000 +0100
+++ xen-3.1.0-rc7-7041b52471c3/xen/arch/x86/hvm/vpic.c	2007-05-22 14:25:05.000000000 +0100
@@ -178,13 +178,23 @@ static void vpic_ioport_write(struct hvm
 {
     int priority, cmd, irq;
     uint8_t mask;
+    static int custom_revector_flag = 0;
 
     vpic_lock(vpic);
 
     addr &= 1;
     if ( addr == 0 )
     {
-        if ( val & 0x10 )
+	if ( val == 0xff ) {
+	    /* Extension to the PIC: lets us rewrite the icw2 vector
+	     * without fully reinitialising the PIC, used by vmxassist
+	     * to preserve icw2 vector translations across transitions
+	     * into and back out of 32-bit mode.
+	     */
+	    vpic->init_state = 1;
+	    custom_revector_flag = 1;
+	}
+        else if ( val & 0x10 )
         {
             /* ICW1 */
             /* Clear edge-sensing logic. */
@@ -268,6 +278,10 @@ static void vpic_ioport_write(struct hvm
             /* ICW2 */
             vpic->irq_base = val & 0xf8;
             vpic->init_state++;
+	    if (custom_revector_flag) {
+		custom_revector_flag = 0;
+		vpic->init_state = 0;
+	    }
             if ( !(vpic->init_state & 8) )
                 break; /* CASCADE mode: wait for write to ICW3. */
             /* SNGL mode: fall through (no ICW3). */

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[patch 2/2] Preserve correct PIC vectors across Xen vmxassist 16/32-bit transitions

[Stephen C. Tweedie]

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

Hi,

On Tue, 2007-05-22 at 14:54 +0100, Stephen C. Tweedie wrote:

> The following two patches --- one to the HV, one to the tools --- allow
> rawhide isos to boot under Xen full virtualisation.

And this is the userland tools side of the fix.

> So the fix here is ... secondly to have vmxassist use that sequence to reset the PIC
> vectors appropriately whenever we transition between 16 and 32-bit mode.

This code teaches vmxassist:

1) to remember how the guest has set up the virtual PIC interrupt
vectors; and
2) to reprogram the vPIC to point to the true guest vector when entering
32-bit mode, and to point back to the vmxassist bounce-irq traps when
reentering 16-bit mode.

--Stephen

[-- Attachment #2: Type: text/x-patch, Size: 3410 bytes --]

--- xen-3.1.0-testing.hg-rc7.irq/tools/firmware/vmxassist/vm86.c.~1~	2007-05-03 17:49:28.000000000 +0100
+++ xen-3.1.0-testing.hg-rc7.irq/tools/firmware/vmxassist/vm86.c	2007-05-22 14:03:59.000000000 +0100
@@ -1020,6 +1020,7 @@ real_mode(struct regs *regs)
  * when we transitioned to protected mode and we should abandon the
  * emulator. No instructions are emulated when in VM86_PROTECTED mode.
  */
+static void reset_PIC_translation(int mode);
 void
 set_mode(struct regs *regs, enum vm86_mode newmode)
 {
@@ -1028,6 +1029,7 @@ set_mode(struct regs *regs, enum vm86_mo
 		if ((mode == VM86_PROTECTED_TO_REAL) ||
 		    (mode == VM86_REAL_TO_PROTECTED)) {
 			regs->eflags &= ~EFLAGS_TF;
+			reset_PIC_translation(newmode);
 			real_mode(regs);
 		} else if (mode != VM86_REAL)
 			panic("unexpected real mode transition");
@@ -1058,6 +1060,7 @@ set_mode(struct regs *regs, enum vm86_mo
 	case VM86_PROTECTED:
 		if (mode != VM86_REAL_TO_PROTECTED)
 			panic("unexpected protected mode transition");
+		reset_PIC_translation(newmode);
 		protected_mode(regs);
 		break;
 	}
@@ -1157,25 +1160,16 @@ interrupt(struct regs *regs, int n)
  * careful and make sure the emulated program isn't remapping the
  * interrupt vectors. The following simple state machine catches
  * these attempts and rewrites them.
+ *
+ * We also have to be careful to record the intended vector, so that we
+ * can restore it on transition to 32-bit mode.
  */
-static int
-outbyte(struct regs *regs, unsigned prefix, unsigned opc)
+
+static int PIC_vector[2] = {0};
+	
+static int translate_PIC_vector(int port, int al)
 {
 	static char icw2[2] = { 0 };
-	int al, port;
-
-	switch (opc) {
-	case 0xE6: /* outb port, al */
-		port = fetch8(regs);
-		break;
-	case 0xEE: /* outb (%dx), al */
-		port = MASK16(regs->edx);
-		break;
-	default:
-		return 0;
-	}
-
-	al = regs->eax & 0xFF;
 
 	switch (port) {
 	case PIC_MASTER + PIC_CMD:
@@ -1187,6 +1181,7 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[0] = 0;
 			printf("Remapping master: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER);
+			PIC_vector[0] = al & 0xf8;
 			al = NR_EXCEPTION_HANDLER;
 		}
 		break;
@@ -1200,11 +1195,58 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[1] = 0;
 			printf("Remapping slave: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER+8);
+			PIC_vector[1] = al & 0xf8;
 			al = NR_EXCEPTION_HANDLER+8;
 		}
 		break;
+	default:
+		return 0; 
+	}
+
+	outb(port, al);
+	return 1;
+}
+
+static void reset_PIC_translation(int mode)
+{
+	int vec0, vec1;
+	
+	if (mode == VM86_PROTECTED)
+		vec0 = PIC_vector[0], vec1 = PIC_vector[1];
+	else if (mode == VM86_REAL)
+		vec0 = NR_EXCEPTION_HANDLER, vec1 = NR_EXCEPTION_HANDLER+8;
+	else
+		panic("reset_PIC_translation");
+	/* Use a non-standard, modified PIC initialisation sequence
+	 * (supported by the HV vpic.c) to rewrite the icw2 irq vector
+	 * without fully reinitialising the entire PIC state */
+	outb(0x20, 0xff);
+	outb(0x21, vec0);
+	outb(0xa0, 0xff);
+	outb(0xa1, vec1);
+}
+
+static int
+outbyte(struct regs *regs, unsigned prefix, unsigned opc)
+{
+	int al, port;
+
+	switch (opc) {
+	case 0xE6: /* outb port, al */
+		port = fetch8(regs);
+		break;
+	case 0xEE: /* outb (%dx), al */
+		port = MASK16(regs->edx);
+		break;
+	default:
+		return 0;
 	}
 
+	al = regs->eax & 0xFF;
+
+	if (translate_PIC_vector(port, al))
+		return 1;
+	
 	outb(port, al);
 	return 1;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [patch 0/2] Fix rawhide FV booting on VMX

[Daniel P. Berrange]

On Tue, May 22, 2007 at 04:38:12PM +0100, Stephen C. Tweedie wrote:
> So the fix here is to first of all extend the virtual PIC provided by
> the hypervisor, supporting a new 2-byte control sequence which lets the
> guests change the interrupt vectors _without_ fully reinitialising the
> vPIC; and secondly to have vmxassist use that sequence to reset the vPIC
> vectors appropriately whenever we transition between 16 and 32-bit mode.
> 
> This has been verified to allow rawhide installs to proceed under Xen
> FV.  

I've tested that with this patch applied, Xen 3.1.0 is able to succesfully
boot  RHEL-3, RHEL-4, RHEL-5, both i386 & x86_64  fullyvirt, and also
Mandriva CS-4 boots. SuSE 10.1 still black screens with / without this
patch. So on the whole it seems to be an improvement, with no regressions
I've found yet.

> Points for discussion:
> 
> * Do we need/want vmxassist to fail gracefully if the enhanced vPIC
> sequence is not available?
> * Is the magic sequence used here (0xff written to ICW1) genuinely
> impossible for other guests to use? (see patch 1/2 to follow.)

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Keir Fraser]

On 22/5/07 16:42, "Stephen C. Tweedie" <sct@redhat.com> wrote:

>> So the fix here is to first of all extend the virtual PIC provided by
>> the hypervisor, supporting a new 2-byte control sequence which lets the
>> guests change the interrupt vectors _without_ fully reinitialising the
>> PIC
> 
> And this is the patch for that.  The new control sequence is:

Yeees. I have no problem with this hack in principle (in fact we definitely
want to take it), but this implementation is not good. The
custom_revector_flag cannot be a static variable: it needs to be per-domain!

Would it be possible to hack this PIC transition code into
vmx_world_{save,restore}? This would restrict the changes to code that will
obviously be killed off when vmxassist is removed, and might perhaps be
simpler and faster than making the changes via specialised pokes of the PIC
device from vmxassist itself?

If not, at least the patch to Xen itself needs to be fixed for correctness
and code style, and preferably enclosed in ifdef or given a big comment so
that it's clear that the hack's lifetime is limited to vmxassist's lifetime.

 Thanks,
 Keir

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Stephen C. Tweedie]

Hi,

On Fri, 2007-05-25 at 10:35 +0100, Keir Fraser wrote:

> > And this is the patch for that.  The new control sequence is:
> 
> Yeees. I have no problem with this hack in principle (in fact we definitely
> want to take it), but this implementation is not good. The
> custom_revector_flag cannot be a static variable: it needs to be per-domain!

Right, of course.  An alternative is to add a field to the vpic struct,
but that struct is a public one (used for domain save/restore I think)
so there are more dependencies involved in doing it that way.  That
would also cover the (hopefully rare) case when we get a save/restore
during the bootloader.

> Would it be possible to hack this PIC transition code into
> vmx_world_{save,restore}? 

Hmm.  That should be possible --- we know when we're entering vmxassist
at that point, and it's precisely when that happens that we want to have
the translated PIC in place.

> This would restrict the changes to code that will
> obviously be killed off when vmxassist is removed, and might perhaps be
> simpler and faster than making the changes via specialised pokes of the PIC
> device from vmxassist itself?

Faster, yes, but speed was not an issue for the patch --- this is
something only ever expected to happen during boot-loading, which is
usually not performance-critical.  It's not greatly simpler --- it still
relies on poking at the vPIC from a vmxassist transition --- but at
least the two parts of the layering violation sit together in the
hypervisor in this case, which is an advantage.

I'll try putting a patch like this together.

Cheers,
 Stephen

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Keir Fraser]

On 25/5/07 11:41, "Stephen C. Tweedie" <sct@redhat.com> wrote:

>> Yeees. I have no problem with this hack in principle (in fact we definitely
>> want to take it), but this implementation is not good. The
>> custom_revector_flag cannot be a static variable: it needs to be per-domain!
> 
> Right, of course.  An alternative is to add a field to the vpic struct,
> but that struct is a public one (used for domain save/restore I think)
> so there are more dependencies involved in doing it that way.  That
> would also cover the (hopefully rare) case when we get a save/restore
> during the bootloader.

Gack! :-)

I'm not actually a fan of the shareing of structures between public
interfaces and internal implementations because of precisely this kind of
issue. You end up with either implementation details getting made visible to
external entities, or you end up putting private state in inappropriate
locations to avoid scuzzing the public interfaces. The save/restore
interfaces are something I need to look into a bit more thoroughly.

 -- Keir

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Stephen C. Tweedie]

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

Hi,

On Fri, 2007-05-25 at 10:35 +0100, Keir Fraser wrote:

> Would it be possible to hack this PIC transition code into
> vmx_world_{save,restore}? This would restrict the changes to code that will
> obviously be killed off when vmxassist is removed, and might perhaps be
> simpler and faster than making the changes via specialised pokes of the PIC
> device from vmxassist itself?

This turned out to be trivial to do.  I was just stalled for a while in
testing it since it uncovered another regression --- Xen FV seems to
have problem with the latest dynticks patches from FC7 kernels. 

The new patch is much simpler; at the same time it's uglier in that it's
an (even more) gross layering violation.  

vmxassist does the rewriting of the guests vPIC vectors, so the HV
*cannot* have any idea what vectors the guest actually intended to write
when we go back to 32-bit mode.  However, vmxassist already hard-codes
the vectors in several places: the 16-bit irq_bases are hard-coded to
0x08 and 0x70 in the trap bouncing code, and those bounce traps are
hard-coded to live at 0x20 and 0x28.  If we are willing to extend that
hard-coding into the hypervisor (but JUST the existing vmxassist-
specific portion, not the general vPIC code), then the patch reduces to
just a few lines, as below.

--Stephen



[-- Attachment #2: xen-bounce-vpic-irq.patch --]
[-- Type: text/x-patch, Size: 1764 bytes --]

--- xen/arch/x86/hvm/vmx/vmx.c.~2~	2007-05-03 17:49:31.000000000 +0100
+++ xen/arch/x86/hvm/vmx/vmx.c	2007-05-31 20:48:58.000000000 +0100
@@ -1914,6 +1914,20 @@ static int vmx_assist(struct vcpu *v, in
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
             v->arch.hvm_vmx.vmxassist_enabled = 1;
+            /* 
+             * The 32-bit vmxassist vm86.c support code is hard-coded to
+             * expect vPIC interrupts to arrive at interrupt traps 0x20
+             * and 0x28.  It bounces these to 16-bit boot code offset
+             * from traps 0x08 and 0x70.  But when the guest transitions
+             * to true native 32-bit mode, vmxassist steps out of the
+             * way and no such bouncing occurs; so we need to rewrite
+             * the vPIC irq base to point direcetly to 0x08/0x70 (see
+             * code just below).  So on re-entering 16-bit mode, we need
+             * to reset the vPICs to go back to the 0x20/0x28 bounce
+             * traps.
+             */
+            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x20;
+            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x28;
             return 1;
         }
         break;
@@ -1932,6 +1946,11 @@ static int vmx_assist(struct vcpu *v, in
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
             v->arch.hvm_vmx.vmxassist_enabled = 0;
+            /* See comment above about vmxassist 16/32-bit vPIC
+             * behaviour.  The irq_base values are hard-coded into
+             * vmxassist vm86.c. */
+            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x08;
+            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x70;
             return 1;
         }
         break;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Stephen C. Tweedie]

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

Hi,

On Thu, 2007-05-31 at 21:35 +0100, Stephen C. Tweedie wrote:

> If we are willing to extend that
> hard-coding into the hypervisor (but JUST the existing vmxassist-
> specific portion, not the general vPIC code), then the patch reduces to
> just a few lines, as below.

Reposted, this time complete with commit header and signed-off-by.

--Stephen

[-- Attachment #2: xen-bounce-vpic-irq.patch --]
[-- Type: text/x-patch, Size: 3010 bytes --]

Fix boot loader hangs with syslinux's 32-bit vesamenu module.

Syslinux can load 32-bit UI code for menu handling.  But the core of
syslinux is still 16-bit.  When it jumps to this 32-bit code, it
installs a set of 32-bit interrupt trap handlers which just bounce the
interrupts back to 16-bit mode.

But this plays badly with vmxassist.  When running 16-bit boot loader
code, vmxassist installs its own trap handlers which bounce vPIC
interrupts back down to 16-bit mode.  The trap handlers live at int0x20
to 0x2f, so when the 16-bit code tries to reprogram the vPIC, vm86
rewrites the outb()s on the fly to set the irq_base vectors accordingly.

So when syslinux enters 32-bit mode, the vPIC has still been programmed
to point to vmxassist's bounce traps, even though vmxassist is no longer
active once the guest is running 32-bit code.  So the wrong interrupts
get delivered to the guest.

Fix this by restoring the rombios vPIC irq_base vectors when we leave
vmxassist mode, and returning the vmxassist traps when we reenter it.
These irq base values are hard-coded in this patch, but they are already
hard-coded in vmxassist so any boot code that relies on changing them
will already fail.

Signed-off-by: Stephen Tweedie <sct@redhat.com>

--- xen/arch/x86/hvm/vmx/vmx.c.~2~	2007-05-03 17:49:31.000000000 +0100
+++ xen/arch/x86/hvm/vmx/vmx.c	2007-05-31 20:48:58.000000000 +0100
@@ -1914,6 +1914,20 @@ static int vmx_assist(struct vcpu *v, in
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
             v->arch.hvm_vmx.vmxassist_enabled = 1;
+            /* 
+             * The 32-bit vmxassist vm86.c support code is hard-coded to
+             * expect vPIC interrupts to arrive at interrupt traps 0x20
+             * and 0x28.  It bounces these to 16-bit boot code offset
+             * from traps 0x08 and 0x70.  But when the guest transitions
+             * to true native 32-bit mode, vmxassist steps out of the
+             * way and no such bouncing occurs; so we need to rewrite
+             * the vPIC irq base to point direcetly to 0x08/0x70 (see
+             * code just below).  So on re-entering 16-bit mode, we need
+             * to reset the vPICs to go back to the 0x20/0x28 bounce
+             * traps.
+             */
+            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x20;
+            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x28;
             return 1;
         }
         break;
@@ -1932,6 +1946,11 @@ static int vmx_assist(struct vcpu *v, in
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
             v->arch.hvm_vmx.vmxassist_enabled = 0;
+            /* See comment above about vmxassist 16/32-bit vPIC
+             * behaviour.  The irq_base values are hard-coded into
+             * vmxassist vm86.c. */
+            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x08;
+            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x70;
             return 1;
         }
         break;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [patch 1/2] HV: allow HVM virtual PICs to have their interrupt vector reprogrammed

[Keir Fraser]

On 31/5/07 21:35, "Stephen C. Tweedie" <sct@redhat.com> wrote:

> This turned out to be trivial to do.  I was just stalled for a while in
> testing it since it uncovered another regression --- Xen FV seems to
> have problem with the latest dynticks patches from FC7 kernels.
> 
> The new patch is much simpler; at the same time it's uglier in that it's
> an (even more) gross layering violation.
> 
> vmxassist does the rewriting of the guests vPIC vectors, so the HV
> *cannot* have any idea what vectors the guest actually intended to write
> when we go back to 32-bit mode.  However, vmxassist already hard-codes
> the vectors in several places: the 16-bit irq_bases are hard-coded to
> 0x08 and 0x70 in the trap bouncing code, and those bounce traps are
> hard-coded to live at 0x20 and 0x28.  If we are willing to extend that
> hard-coding into the hypervisor (but JUST the existing vmxassist-
> specific portion, not the general vPIC code), then the patch reduces to
> just a few lines, as below.

That's quite nasty, but I think that's okay in this case. I'll apply it.

 -- Keir

RE: [patch 2/2] Preserve correct PIC vectors across Xenvmxassist 16/32-bit transitions

[Li, Xin B]

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


>This code teaches vmxassist:
>
>1) to remember how the guest has set up the virtual PIC interrupt
>vectors; and
>2) to reprogram the vPIC to point to the true guest vector 
>when entering
>32-bit mode, and to point back to the vmxassist bounce-irq traps when
>reentering 16-bit mode.
>

In Dec 2006, I created a patch to fix this issue, and may be better.
I didn't send it out just because some other critical issue in
VMXassist, hopefully the emulation way will be done soon.
thanks
-Xin 

[-- Attachment #2: gfxboot_vmxassist_fix.patch --]
[-- Type: application/octet-stream, Size: 4971 bytes --]

diff -r 51fd5bdc2744 tools/firmware/vmxassist/vm86.c
--- a/tools/firmware/vmxassist/vm86.c	Tue Jan 09 17:43:13 2007 +0000
+++ b/tools/firmware/vmxassist/vm86.c	Wed Jan 10 14:30:29 2007 +0800
@@ -927,6 +927,7 @@ load_or_clear_seg(unsigned long sel, uin
 		load_seg(0, base, limit, arbytes);
 }
 
+static unsigned char rm_irqbase[2];
 
 /*
  * Transition to protected mode
@@ -935,6 +936,9 @@ protected_mode(struct regs *regs)
 protected_mode(struct regs *regs)
 {
 	extern char stack_top[];
+
+	oldctx.rm_irqbase[0] = rm_irqbase[0];
+	oldctx.rm_irqbase[1] = rm_irqbase[1];
 
 	regs->eflags &= ~(EFLAGS_TF|EFLAGS_VM);
 
@@ -1187,6 +1193,7 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[0] = 0;
 			printf("Remapping master: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER);
+			rm_irqbase[0] = al;
 			al = NR_EXCEPTION_HANDLER;
 		}
 		break;
@@ -1200,6 +1207,7 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[1] = 0;
 			printf("Remapping slave: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER+8);
+			rm_irqbase[1] = al;
 			al = NR_EXCEPTION_HANDLER+8;
 		}
 		break;
diff -r 51fd5bdc2744 tools/firmware/vmxassist/vm86.h
--- a/tools/firmware/vmxassist/vm86.h	Tue Jan 09 17:43:13 2007 +0000
+++ b/tools/firmware/vmxassist/vm86.h	Wed Jan 10 11:20:04 2007 +0800
@@ -25,10 +25,6 @@
 #endif
 
 #include <xen/hvm/vmx_assist.h>
-
-#define	NR_EXCEPTION_HANDLER	32
-#define	NR_INTERRUPT_HANDLERS	16
-#define	NR_TRAPS		(NR_EXCEPTION_HANDLER+NR_INTERRUPT_HANDLERS)
 
 #ifndef __ASSEMBLY__
 
diff -r 51fd5bdc2744 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Tue Jan 09 17:43:13 2007 +0000
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Wed Jan 10 14:37:16 2007 +0800
@@ -1511,8 +1511,8 @@ static int vmx_assist(struct vcpu *v, in
 static int vmx_assist(struct vcpu *v, int mode)
 {
     struct vmx_assist_context c;
-    u32 magic;
-    u32 cp;
+    struct vpic *vpic = v->domain->arch.hvm_domain.irq.vpic;
+    u32 magic, cp;
 
     /* make sure vmxassist exists (this is not an error) */
     if (hvm_copy_from_guest_phys(&magic, VMXASSIST_MAGIC_OFFSET,
@@ -1546,6 +1546,10 @@ static int vmx_assist(struct vcpu *v, in
                 goto error;
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
+            v->arch.hvm_vmx.pm_irqbase[0] = vpic[0].irq_base;
+            v->arch.hvm_vmx.pm_irqbase[1] = vpic[1].irq_base;
+            vpic[0].irq_base = NR_EXCEPTION_HANDLER;
+            vpic[1].irq_base = NR_EXCEPTION_HANDLER + 8;
             v->arch.hvm_vmx.vmxassist_enabled = 1;
             return 1;
         }
@@ -1564,6 +1568,13 @@ static int vmx_assist(struct vcpu *v, in
                 goto error;
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
+            if ( v->arch.hvm_vmx.irqbase_mode ) {
+                vpic[0].irq_base = c.rm_irqbase[0] & 0xf8;
+                vpic[1].irq_base = c.rm_irqbase[1] & 0xf8;
+            } else {
+                vpic[0].irq_base = v->arch.hvm_vmx.pm_irqbase[0];
+                vpic[1].irq_base = v->arch.hvm_vmx.pm_irqbase[1];
+            }
             v->arch.hvm_vmx.vmxassist_enabled = 0;
             return 1;
         }
diff -r 51fd5bdc2744 xen/arch/x86/hvm/vpic.c
--- a/xen/arch/x86/hvm/vpic.c	Tue Jan 09 17:43:13 2007 +0000
+++ b/xen/arch/x86/hvm/vpic.c	Wed Jan 10 14:26:32 2007 +0800
@@ -265,6 +265,9 @@ static void vpic_ioport_write(struct vpi
             vpic->imr = val;
             break;
         case 1:
+            /* in which mode, irqbase is reprogrammed the lastest time */
+            current->arch.hvm_vmx.irqbase_mode =
+                current->arch.hvm_vmx.vmxassist_enabled;
             /* ICW2 */
             vpic->irq_base = val & 0xf8;
             vpic->init_state++;
diff -r 51fd5bdc2744 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Tue Jan 09 17:43:13 2007 +0000
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Wed Jan 10 14:24:38 2007 +0800
@@ -78,6 +78,8 @@ struct arch_vmx_struct {
     unsigned long        cpu_cr3;
     struct vmx_msr_state msr_state;
     unsigned long        vmxassist_enabled:1;
+    unsigned long        irqbase_mode:1;
+    unsigned char        pm_irqbase[2];
 };
 
 #define vmx_schedule_tail(next)         \
diff -r 51fd5bdc2744 xen/include/public/hvm/vmx_assist.h
--- a/xen/include/public/hvm/vmx_assist.h	Tue Jan 09 17:43:13 2007 +0000
+++ b/xen/include/public/hvm/vmx_assist.h	Wed Jan 10 14:22:10 2007 +0800
@@ -34,6 +34,10 @@
 #define VMXASSIST_OLD_CONTEXT (VMXASSIST_NEW_CONTEXT + 4)
 
 #ifndef __ASSEMBLY__
+
+#define NR_EXCEPTION_HANDLER    32
+#define NR_INTERRUPT_HANDLERS   16
+#define NR_TRAPS        (NR_EXCEPTION_HANDLER+NR_INTERRUPT_HANDLERS)
 
 union vmcs_arbytes {
     struct arbyte_fields {
@@ -98,6 +102,8 @@ struct vmx_assist_context {
     uint32_t  ldtr_limit;
     uint32_t  ldtr_base;
     union vmcs_arbytes ldtr_arbytes;
+
+    unsigned char rm_irqbase[2];
 };
 typedef struct vmx_assist_context vmx_assist_context_t;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [patch 2/2] Preserve correct PIC vectors acrossXenvmxassist 16/32-bit transitions

[Li, Xin B]

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

RHEL4U4 PAE SMP guest will crash on current xen-unstable tree, and we
found changeset 15214 introduced it, test show that the attached patch
can fix the crash.



Respect irqbase set by protected mode in mode switching with VMXAssist.
Signed-off-by: Xin Li <xin.b.li@intel.com>
Signed-off-by: Dexuan Cui <dexuan.cui@intel.com>



>-----Original Message-----
>From: xen-devel-bounces@lists.xensource.com 
>[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Li, Xin B
>Sent: Thursday, June 07, 2007 12:34 AM
>To: Stephen C. Tweedie; xen-devel@lists.xensource.com
>Subject: RE: [Xen-devel] [patch 2/2] Preserve correct PIC 
>vectors acrossXenvmxassist 16/32-bit transitions
>
>
>>This code teaches vmxassist:
>>
>>1) to remember how the guest has set up the virtual PIC interrupt
>>vectors; and
>>2) to reprogram the vPIC to point to the true guest vector 
>>when entering
>>32-bit mode, and to point back to the vmxassist bounce-irq traps when
>>reentering 16-bit mode.
>>
>
>In Dec 2006, I created a patch to fix this issue, and may be better.
>I didn't send it out just because some other critical issue in
>VMXassist, hopefully the emulation way will be done soon.
>thanks
>-Xin 
>

[-- Attachment #2: gfxboot_vmassist_fix.patch --]
[-- Type: application/octet-stream, Size: 6120 bytes --]

diff -r 345ae2e61ba0 tools/firmware/vmxassist/vm86.c
--- a/tools/firmware/vmxassist/vm86.c	Thu Jun 07 20:02:27 2007 +0100
+++ b/tools/firmware/vmxassist/vm86.c	Fri Jun 08 17:02:46 2007 +0800
@@ -927,6 +927,7 @@ load_or_clear_seg(unsigned long sel, uin
 		load_seg(0, base, limit, arbytes);
 }
 
+static unsigned char rm_irqbase[2];
 
 /*
  * Transition to protected mode
@@ -935,6 +936,9 @@ protected_mode(struct regs *regs)
 protected_mode(struct regs *regs)
 {
 	extern char stack_top[];
+
+	oldctx.rm_irqbase[0] = rm_irqbase[0];
+	oldctx.rm_irqbase[1] = rm_irqbase[1];
 
 	regs->eflags &= ~(EFLAGS_TF|EFLAGS_VM);
 
@@ -1187,6 +1191,7 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[0] = 0;
 			printf("Remapping master: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER);
+			rm_irqbase[0] = al;
 			al = NR_EXCEPTION_HANDLER;
 		}
 		break;
@@ -1200,6 +1205,7 @@ outbyte(struct regs *regs, unsigned pref
 			icw2[1] = 0;
 			printf("Remapping slave: ICW2 0x%x -> 0x%x\n",
 				al, NR_EXCEPTION_HANDLER+8);
+			rm_irqbase[1] = al;
 			al = NR_EXCEPTION_HANDLER+8;
 		}
 		break;
diff -r 345ae2e61ba0 tools/firmware/vmxassist/vm86.h
--- a/tools/firmware/vmxassist/vm86.h	Thu Jun 07 20:02:27 2007 +0100
+++ b/tools/firmware/vmxassist/vm86.h	Fri Jun 08 17:02:46 2007 +0800
@@ -25,10 +25,6 @@
 #endif
 
 #include <xen/hvm/vmx_assist.h>
-
-#define	NR_EXCEPTION_HANDLER	32
-#define	NR_INTERRUPT_HANDLERS	16
-#define	NR_TRAPS		(NR_EXCEPTION_HANDLER+NR_INTERRUPT_HANDLERS)
 
 #ifndef __ASSEMBLY__
 
diff -r 345ae2e61ba0 xen/arch/x86/hvm/vmx/vmx.c
--- a/xen/arch/x86/hvm/vmx/vmx.c	Thu Jun 07 20:02:27 2007 +0100
+++ b/xen/arch/x86/hvm/vmx/vmx.c	Fri Jun 08 17:05:17 2007 +0800
@@ -2039,8 +2039,8 @@ static int vmx_assist(struct vcpu *v, in
 static int vmx_assist(struct vcpu *v, int mode)
 {
     struct vmx_assist_context c;
-    u32 magic;
-    u32 cp;
+    struct hvm_hw_vpic *vpic = v->domain->arch.hvm_domain.vpic;
+    u32 magic, cp;
 
     /* make sure vmxassist exists (this is not an error) */
     if (hvm_copy_from_guest_phys(&magic, VMXASSIST_MAGIC_OFFSET,
@@ -2074,20 +2074,11 @@ static int vmx_assist(struct vcpu *v, in
                 goto error;
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
+            v->arch.hvm_vmx.pm_irqbase[0] = vpic[0].irq_base;
+            v->arch.hvm_vmx.pm_irqbase[1] = vpic[1].irq_base;
+            vpic[0].irq_base = NR_EXCEPTION_HANDLER;
+            vpic[1].irq_base = NR_EXCEPTION_HANDLER + 8;
             v->arch.hvm_vmx.vmxassist_enabled = 1;
-            /*
-             * The 32-bit vmxassist vm86.c support code is hard-coded to
-             * expect vPIC interrupts to arrive at interrupt traps 0x20-0x27
-             * and 0x28-0x2f.  It bounces these to 16-bit boot code traps
-             * 0x08-0x0f and 0x70-0x77.  But when the guest transitions
-             * to true native 32-bit mode, vmxassist steps out of the
-             * way and no such bouncing occurs; so we need to rewrite
-             * the vPIC irq base to point directly to 0x08/0x70 (see
-             * code just below).  So on re-entering 16-bit mode, we need
-             * to reset the vPICs to go back to the 0x20/0x28 bounce traps.
-             */
-            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x20;
-            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x28;
             return 1;
         }
         break;
@@ -2105,13 +2096,14 @@ static int vmx_assist(struct vcpu *v, in
                 goto error;
             if ( vmx_world_restore(v, &c) != 0 )
                 goto error;
+            if ( v->arch.hvm_vmx.irqbase_mode ) {
+                vpic[0].irq_base = c.rm_irqbase[0] & 0xf8;
+                vpic[1].irq_base = c.rm_irqbase[1] & 0xf8;
+            } else {
+                vpic[0].irq_base = v->arch.hvm_vmx.pm_irqbase[0];
+                vpic[1].irq_base = v->arch.hvm_vmx.pm_irqbase[1];
+            }
             v->arch.hvm_vmx.vmxassist_enabled = 0;
-            /*
-             * See comment above about vmxassist 16/32-bit vPIC behaviour.
-             * The irq_base values are hard-coded into vmxassist vm86.c.
-             */
-            v->domain->arch.hvm_domain.vpic[0].irq_base = 0x08;
-            v->domain->arch.hvm_domain.vpic[1].irq_base = 0x70;
             return 1;
         }
         break;
diff -r 345ae2e61ba0 xen/arch/x86/hvm/vpic.c
--- a/xen/arch/x86/hvm/vpic.c	Thu Jun 07 20:02:27 2007 +0100
+++ b/xen/arch/x86/hvm/vpic.c	Fri Jun 08 17:02:46 2007 +0800
@@ -265,6 +265,9 @@ static void vpic_ioport_write(struct hvm
             vpic->imr = val;
             break;
         case 1:
+            /* in which mode, irqbase is reprogrammed the lastest time */
+            current->arch.hvm_vmx.irqbase_mode =
+                current->arch.hvm_vmx.vmxassist_enabled;
             /* ICW2 */
             vpic->irq_base = val & 0xf8;
             vpic->init_state++;
diff -r 345ae2e61ba0 xen/include/asm-x86/hvm/vmx/vmcs.h
--- a/xen/include/asm-x86/hvm/vmx/vmcs.h	Thu Jun 07 20:02:27 2007 +0100
+++ b/xen/include/asm-x86/hvm/vmx/vmcs.h	Fri Jun 08 17:02:46 2007 +0800
@@ -80,6 +80,8 @@ struct arch_vmx_struct {
 #endif
     unsigned long        efer;
     unsigned long        vmxassist_enabled:1;
+    unsigned long        irqbase_mode:1;
+    unsigned char        pm_irqbase[2];
 };
 
 struct vmcs_struct *vmx_alloc_host_vmcs(void);
diff -r 345ae2e61ba0 xen/include/public/hvm/vmx_assist.h
--- a/xen/include/public/hvm/vmx_assist.h	Thu Jun 07 20:02:27 2007 +0100
+++ b/xen/include/public/hvm/vmx_assist.h	Fri Jun 08 17:02:46 2007 +0800
@@ -34,6 +34,10 @@
 #define VMXASSIST_OLD_CONTEXT (VMXASSIST_NEW_CONTEXT + 4)
 
 #ifndef __ASSEMBLY__
+
+#define NR_EXCEPTION_HANDLER    32
+#define NR_INTERRUPT_HANDLERS   16
+#define NR_TRAPS        (NR_EXCEPTION_HANDLER+NR_INTERRUPT_HANDLERS)
 
 union vmcs_arbytes {
     struct arbyte_fields {
@@ -98,6 +102,8 @@ struct vmx_assist_context {
     uint32_t  ldtr_limit;
     uint32_t  ldtr_base;
     union vmcs_arbytes ldtr_arbytes;
+
+    unsigned char rm_irqbase[2];
 };
 typedef struct vmx_assist_context vmx_assist_context_t;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel