Xen devel - Mar 2006 - [PATCH] This patch fixes several issues related to vmxassist

This patch fixes several issues related to vmxassist:
1) AP bring up;
2) RHEL4 IA32e installation;
3) SLES10 IA32e installation;

Signed-off-by: Xin Li <xin.b.li@intel.com>


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

On 30 Mar 2006, at 15:33, Li, Xin B wrote:
> This patch fixes several issues related to vmxassist:
> 1) AP bring up;
> 2) RHEL4 IA32e installation;
> 3) SLES10 IA32e installation;
Can you explain what was wrong with the function this patch changes, 
and how the patch fixes that problem? The patch looks kludgy to me (as 
far as I understand) -- apparently trying to differentiate between 
real-mode and protected-mode segments? Why is that needed? Shouldn''t 
you be looking at the execution mode of the caller (e.g., EFLAGS.VM)?

  -- Keir


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

>Can you explain what was wrong with the function this patch changes, 
>and how the patch fixes that problem? The patch looks kludgy to me (as 
>far as I understand) -- apparently trying to differentiate between 
>real-mode and protected-mode segments? Why is that needed?
Shouldn''t
>you be looking at the execution mode of the caller (e.g., EFLAGS.VM)?
>
>  -- Keir
Changeset 9366 from Leendert is to support big real mode, in turn to
support the SYSLINUX/ISOLINUX.
so the address calculation is kludgy, the point here is to identify if a
cpu is in big real mode, if so, we should use protected mode addressing
even in real mode.
-Xin

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

On 30 Mar 2006, at 17:09, Li, Xin B wrote:
> Changeset 9366 from Leendert is to support big real mode, in turn to
> support the SYSLINUX/ISOLINUX.
> so the address calculation is kludgy, the point here is to identify if 
> a
> cpu is in big real mode, if so, we should use protected mode addressing
> even in real mode.
Shouldn''t we get the Xen portion of vmxassist help us with that, for 
example by making the hidden descriptor info (base, limit, etc) 
available to us? There''s already a method for loading that stuff out of
Xen, right?

Looks to me as though the kludge won''t work if you unluckily load a 
real-mode segment value that happens to also reference a ''big
segment''
in the currently registered GDT.

  -- Keir


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

>-----Original Message-----
>From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
>Sent: 2006年3月31日 0:38
>To: Li, Xin B
>Cc: Xen Devel
>Subject: Re: [Xen-devel] [PATCH] This patch fixes several 
>issues related to vmxassist
>
>
>On 30 Mar 2006, at 17:09, Li, Xin B wrote:
>
>> Changeset 9366 from Leendert is to support big real mode, in turn to
>> support the SYSLINUX/ISOLINUX.
>> so the address calculation is kludgy, the point here is to 
>identify if 
>> a
>> cpu is in big real mode, if so, we should use protected mode 
>addressing
>> even in real mode.
>
>Shouldn''t we get the Xen portion of vmxassist help us with that,
for
>example by making the hidden descriptor info (base, limit, etc) 
>available to us? There''s already a method for loading that 
>stuff out of 
>Xen, right?
>
>Looks to me as though the kludge won''t work if you unluckily load a
>real-mode segment value that happens to also reference a ''big
segment''
>in the currently registered GDT.
Yes, we may have potential bug here, maybe we should hold this patch and try to
find a cleaner way.
-Xin

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

On 30 Mar 2006, at 18:02, Li, Xin B wrote:
>> Shouldn''t we get the Xen portion of vmxassist help us with
that, for
>> example by making the hidden descriptor info (base, limit, etc)
>> available to us? There''s already a method for loading that
>> stuff out of
>> Xen, right?
>>
>> Looks to me as though the kludge won''t work if you unluckily
load a
>> real-mode segment value that happens to also reference a ''big
segment''
>> in the currently registered GDT.
>
> Yes, we may have potential bug here, maybe we should hold this patch 
> and try to find a cleaner way.
> -Xin
In fact, the existing implementation of address() is kludgy. It already 
does tests on the selector value to decide whether it is likely to 
refer to a protected-mode or real-mode segment. Unfortunately the test 
may sometimes yield false positives (selectors that look like they 
could be a valid protected-mode value, but actually it''s some arbitrary
real-mode selector).

I don''t know the heritage of that code. I expect someone decided it was
good enough to be getting on with but maybe now it is time to revisit 
and see if we can implement a watertight version which correctly uses 
hidden segment descriptor state which is readily available when running 
on VMX.

It might be worth pinging Leendert about this and see what he thinks.

  -- Keir


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

>In fact, the existing implementation of address() is kludgy. 
>It already 
>does tests on the selector value to decide whether it is likely to 
>refer to a protected-mode or real-mode segment. Unfortunately the test 
>may sometimes yield false positives (selectors that look like they 
>could be a valid protected-mode value, but actually it''s some 
>arbitrary 
>real-mode selector).
>
>I don''t know the heritage of that code. I expect someone 
>decided it was 
>good enough to be getting on with but maybe now it is time to revisit 
>and see if we can implement a watertight version which correctly uses 
>hidden segment descriptor state which is readily available 
>when running 
>on VMX.
My patch just enhanced the current implementation, and actually it
breaks windows, but I have a updated version in hand, and tests show
that all the combinations is OK till now.

In my mind, the correct way is to identify whether a cpu is in big real
mode, but seems this is a little bit hard to do.
>
>It might be worth pinging Leendert about this and see what he thinks.
Yes, we have contact with him :-)

-Xin

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

On 31 Mar 2006, at 12:06, Li, Xin B wrote:
>> I don''t know the heritage of that code. I expect someone
>> decided it was
>> good enough to be getting on with but maybe now it is time to revisit
>> and see if we can implement a watertight version which correctly uses
>> hidden segment descriptor state which is readily available
>> when running
>> on VMX.
>
> My patch just enhanced the current implementation, and actually it
> breaks windows, but I have a updated version in hand, and tests show
> that all the combinations is OK till now.
>
> In my mind, the correct way is to identify whether a cpu is in big real
> mode, but seems this is a little bit hard to do.
Well, it''s impossible to know for certain without looking at hidden 
descriptor state isn''t it. Big real mode is only possible because the 
hidden state can be out of sync with the current execution mode.

If that state can be made available to the address() function then it''s
implementation, and confidence in its correctness, becomes trivial. Do 
we have that state available directly in the guest at that point? Or 
would we need to grab the state from Xen?

  -- Keir


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

>>> I don''t know the heritage of that code. I expect someone
>>> decided it was
>>> good enough to be getting on with but maybe now it is time 
>to revisit
>>> and see if we can implement a watertight version which 
>correctly uses
>>> hidden segment descriptor state which is readily available
>>> when running
>>> on VMX.
>>
>> My patch just enhanced the current implementation, and actually it
>> breaks windows, but I have a updated version in hand, and tests show
>> that all the combinations is OK till now.
>>
>> In my mind, the correct way is to identify whether a cpu is 
>in big real
>> mode, but seems this is a little bit hard to do.
>
>Well, it''s impossible to know for certain without looking at hidden
>descriptor state isn''t it. Big real mode is only possible because
the
>hidden state can be out of sync with the current execution mode.
>
>If that state can be made available to the address() function 
>then it''s 
>implementation, and confidence in its correctness, becomes trivial. Do 
>we have that state available directly in the guest at that point? Or 
>would we need to grab the state from Xen?
>
Keir, do you think this one seems better?
When in real mode, we should always calculate address as ((seg & 0xFFFF)
<< 4) + off, big real mode obey the same rule.
When in real mode to protected mode transition, only the IP address
calculation should use real mode addressing.
-Xin

@@ -50,23 +50,42 @@ static char *rnames[] = { "ax", "cx",
"d
 static char *rnames[] = { "ax", "cx", "dx",
"bx", "sp", "bp", "si",
"di" };
 #endif /* DEBUG */
 
-unsigned
+static unsigned
 address(struct regs *regs, unsigned seg, unsigned off)
 {
 	unsigned long long entry;
-	unsigned addr;
+	unsigned seg_base, seg_limit;
+	unsigned entry_low, entry_high;
 
 	if (seg == 0)
-		return off;
-
-	if (seg > oldctx.gdtr_limit)
+	{
+		if (mode == VM86_REAL || mode == VM86_REAL_TO_PROTECTED)
+			return off;
+		else
+			panic("segment is zero, but not in real
mode!\n");
+	}
+
+	if (mode == VM86_REAL || seg > oldctx.gdtr_limit ||
+	    (mode == VM86_REAL_TO_PROTECTED && regs->cs == seg))
 		return ((seg & 0xFFFF) << 4) + off;
 
 	entry = ((unsigned long long *) oldctx.gdtr_base)[seg >> 3];
-	addr = (((entry >> (56-24)) & 0xFF000000) |
-		((entry >> (32-16)) & 0x00FF0000) |
-		((entry >> (   16)) & 0x0000FFFF)) + off;
-	return addr;
+	entry_high = entry >> 32;
+	entry_low = entry & 0xFFFFFFFF;
+
+	seg_base  = (entry_high & 0xFF000000) | ((entry >> 16) &
0xFFFFFF);
+	seg_limit = (entry_high & 0xF0000) | (entry_low & 0xFFFF);
+
+	if (entry_high & 0x8000 &&
+	    ((entry_high & 0x800000 && off >> 12 <= seg_limit) ||
+	    (!(entry_high & 0x800000) && off <= seg_limit)))
+		return seg_base + off;
+
+	panic("should never reach here in function address():\n\t"
+	      "entry=0x%08x%08x, mode=%d, seg=0x%08x, offset=0x%08x\n",
+	      entry_high, entry_low, mode, seg, off);
+
+	return 0;
 }
 
 #ifdef DEBUG


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

On 1 Apr 2006, at 11:35, Li, Xin B wrote:
> Keir, do you think this one seems better?
> When in real mode, we should always calculate address as ((seg & 
> 0xFFFF)
> << 4) + off, big real mode obey the same rule.
> When in real mode to protected mode transition, only the IP address
> calculation should use real mode addressing.
Yes, I''ll push this one: I like the extra sanity checking. Why are the 
checks for zero and greater-than-gdt-limit required (why is it 
insufficient to merely check the current mode)?

  -- Keir


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

>
>Yes, I''ll push this one: I like the extra sanity checking. Why are
the
>checks for zero and greater-than-gdt-limit required (why is it 
>insufficient to merely check the current mode)?
You got it :-), and I also suspect they are not required.
I just keep it there and seems it''s not harmful, when we are sure they
are not needed, we can remove it.
-Xin

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

On 1 Apr 2006, at 16:51, Li, Xin B wrote:
>> Yes, I''ll push this one: I like the extra sanity checking. Why
are the
>> checks for zero and greater-than-gdt-limit required (why is it
>> insufficient to merely check the current mode)?
>
> You got it :-), and I also suspect they are not required.
> I just keep it there and seems it''s not harmful, when we are sure
they
> are not needed, we can remove it.
I still think using and maintaining shadow segment state would 
ultimately be the correct solution. But it sounds like I was mistaken 
about how big real mode works -- I didn''t realise writing a segment 
register in real mode doesn''t update the hidden segment limit register 
and that''s the trick that creates big real mode. That given, the 
current approach isn''t so bad (there''s already an acceptable
kludge in
there to deal with ''stale'' hidden base addresses when
switching
protected-to-real mode) and perhaps we''ll get away with leaving the 
code alone now. It''s obviously a pain to revalidate it on a wide range 
of systems if we start making bigger and more fundamental changes to 
it. Also, I trust that Leendert knows rather more about the transitory 
real/protected modes than I do. :-)

  -- Keir


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