[Xen-devel] [PATCH 3/4] rombios interface for HVM S3

[Juergen Keil <jk@xxxxxxxx>]

"Ke, Liping" wrote:

> This patch has no change.
>
> [PATCH 3/4] rombios interface for HVM S3
>  - add S3 package in ACPI DSDT table. Guest OS will get S3 value from
> this package and write the value to PM1A control register to trigger S3
> suspend.
>  - Add S3 resume logic in rombios post code. the CMOS shutdown register
>   is used to indicate if this is a S3 resume.
> - if it is s3 resume, rombios will get wakeup vector from ACPI FACS
>   table and jump to wakeup vector.

Has anyone tested this with an AMD cpu?  I suspect that the code was
tested with Intel CPUs; and reading old mailing list messages, it seems
to  be able to s3resume both linux and windows hvm guests on intel vtd.

With an amd64 x2 cpu (using an opensolaris dom0), the hvm bios seems
to hang in rombios.c when I use "xm trigger domain s3resume", here:

    /* get x_firmware_waking_vector */
    s3_wakeup_vector = *((Bit16u*)(ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+24));


Matching the CS:IP (f000:1692) listed in the xen console's register
dump with the xen.hg/tools/firmware/rombios/rombios.txt file,
we're at this instruction:

06098                                           ! 1816
06099                                           ! 1817
06100                                           ! 1818     s3_wakeup_vector = 
*((Bit16u*)(0xEA000 
+0x10 +24));
06101                                           ! Debug: eq unsigned short = 
[+$EA028] to 
unsigned short s3_wakeup_vector = [S+4-4] (used reg = )
06102 1692   67      A1     000EA028            mov     ax,[$EA028]


Any address >= 0x10000 (e.g. 0x000EA028) hangs in the bios
on my box.  0xfffe works ok  -  that doesn't use the
67 prefix byte and uses a 16 bit address.



>   Per ACPI spec, the wakeup vector
>   jumping must be the forms CS:IP, in which CS=(wakeup vector>>4)
>   IP=(wakeup vector)&0xF, for example, for vector=0x12345,
>   CS:IP=0x1234:0x5


Hmm, the current code in s3_resume() reads a 16-bit value from
the FACS wakeup vector fields (see rombios.c, lines 2317 / 2342).
A vector like 0x12345 wouldn't work at this time, because it
gets truncated to 0x2345!

I'd say we need to load at least a 20-bit value from
ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+12 (and the
x_firmware_waking_vector is supposed to be a 64-bit
physical address, so that proably needs more changes).

  2314  void
  2315  s3_resume()
  2316  {
  2317      Bit16u s3_wakeup_vector;
  ...
  2338      /* get x_firmware_waking_vector */
  2339      s3_wakeup_vector = 
*((Bit16u*)(ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+24));
  2340      if (s3_wakeup_vector == 0){
  2341          /* get firmware_waking_vector */
  2342          s3_wakeup_vector = 
*((Bit16u*)(ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+12));
  2343          if (s3_wakeup_vector == 0){
  2344              goto s3_out;
  2345          }
  2346      }
  2347  
  2348      /* setup wakeup vector */
  2349      s3_wakeup_ip = s3_wakeup_vector & 0xF;
  2350      s3_wakeup_cs = s3_wakeup_vector >> 4;



Another thing that looks strange is that s3_wakeup_ip / s3_wakeup_cs
must be copied from segment #0000 to #f000 (lines 2353-2359).
I'd say the s3_resume code could corrupt some random 4-bytes
in the lower 64 kbyte segment:


(DS = #0x0000 at this point ->)
  
  2348      /* setup wakeup vector */
  2349      s3_wakeup_ip = s3_wakeup_vector & 0xF;
  2350      s3_wakeup_cs = s3_wakeup_vector >> 4;
  2351
  2352  ASM_START
  2353      mov bx, [_s3_wakeup_cs]
  2354      mov dx, [_s3_wakeup_ip]
  2355
  2356      mov ax, #0xF000
  2357      mov ds, ax
  2358      mov [_s3_wakeup_cs], bx
  2359      mov [_s3_wakeup_ip], dx
  2360      jmpf [_s3_wakeup_ip]



For now I'm using the attached patch.  It doesn't
hang any more in the bios on s3resume, and the
standard firmware_waking_vector can point anywere
in memory below 1MB.  And it doesn't write
to 0000:_s3_wakeup_{cs,ip} any more.

For unknown reasons, HVM S3 resume hangs in the bios when trying to load
the x_firmware_waking_vector from absolute address 0xEA01C (it hangs for
access to address 0x10000, but doesn't hang for 0xfffe).

diff --git a/tools/firmware/rombios/rombios.c b/tools/firmware/rombios/rombios.c
--- a/tools/firmware/rombios/rombios.c
+++ b/tools/firmware/rombios/rombios.c
@@ -2311,10 +2311,31 @@
 #define ACPI_FACS_OFFSET 0x10
 /* S3 resume status in CMOS 0Fh shutdown status byte*/
 
+Bit32u facs_get32(offs)
+Bit16u offs;
+{
+ASM_START
+  push bp
+  mov  bp, sp
+
+    push ds
+    mov ax, #(ACPI_FACS_ADDRESS >> 4)
+    mov ds, ax
+
+    mov bx, 4[bp]
+    mov ax, [bx]
+    mov dx, 2[bx]
+    pop ds
+
+  pop  bp
+ASM_END
+}
+
+
 void 
 s3_resume()
 {
-    Bit16u s3_wakeup_vector;
+    Bit32u s3_wakeup_vector;
     extern Bit16u s3_wakeup_ip;
     extern Bit16u s3_wakeup_cs;
     extern Bit8u s3_resume_flag;
@@ -2330,19 +2351,14 @@
     }
     s3_resume_flag = 0;
 
-ASM_START
-    mov ax, #0x0
-    mov ds, ax
-ASM_END
-
     /* get x_firmware_waking_vector */
-    s3_wakeup_vector = *((Bit16u*)(ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+24));
-    if (s3_wakeup_vector == 0){
+    s3_wakeup_vector = facs_get32(ACPI_FACS_OFFSET+24);
+    if (!s3_wakeup_vector) {
         /* get firmware_waking_vector */
-        s3_wakeup_vector = *((Bit16u*)(ACPI_FACS_ADDRESS+ACPI_FACS_OFFSET+12));
-        if (s3_wakeup_vector == 0){
+       s3_wakeup_vector = facs_get32(ACPI_FACS_OFFSET+12);
+       if (!s3_wakeup_vector) {
             goto s3_out;
-        }
+       }
     }
 
     /* setup wakeup vector */
@@ -2350,13 +2366,6 @@
     s3_wakeup_cs = s3_wakeup_vector >> 4;
 
 ASM_START
-    mov bx, [_s3_wakeup_cs]
-    mov dx, [_s3_wakeup_ip]
-
-    mov ax, #0xF000
-    mov ds, ax
-    mov [_s3_wakeup_cs], bx
-    mov [_s3_wakeup_ip], dx
     jmpf [_s3_wakeup_ip]
 
 ; S3 data

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