[Qemu-devel] Re: [Bochs-developers] [PATCH v2 2/6] Add S3 state to DSDT. Handleresume event in the BIOS.

["Sebastian Herbszt" <herbszt@gmx.de>]

Gleb Natapov wrote:

> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> ---
> 
> bios/acpi-dsdt.dsl |   30 +++++++++++++++++----
> bios/acpi-dsdt.hex |   17 +++++++-----
> bios/rombios.c     |   41 +++++++++++++++++++++++++++++
> bios/rombios32.c   |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> 4 files changed, 146 insertions(+), 16 deletions(-)
> 
> diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
> index 19ac2f9..f201396 100644
> --- a/bios/acpi-dsdt.dsl
> +++ b/bios/acpi-dsdt.dsl
> @@ -531,11 +531,29 @@ DefinitionBlock (
>         }
>     }
> 
> -    /* S5 = power off state */
> -    Name (_S5, Package (4) {
> -        0x00, // PM1a_CNT.SLP_TYP
> -        0x00, // PM2a_CNT.SLP_TYP
> -        0x00, // reserved
> -        0x00, // reserved
> +    /*
> +     * S3 (suspend-to-ram), S4 (suspend-to-disc) and S5 (power-off) type codes:
> +     * must match piix4 emulation.
> +     */

Is it suspend-to-disc or suspend-to-disk?

> +    Name (\_S3, Package (0x04)
> +    {
> +        0x01,  /* PM1a_CNT.SLP_TYP */
> +        0x01,  /* PM1b_CNT.SLP_TYP */
> +        Zero,  /* reserved */
> +        Zero   /* reserved */
> +    })
> +    Name (\_S4, Package (0x04)
> +    {
> +        Zero,  /* PM1a_CNT.SLP_TYP */
> +        Zero,  /* PM1b_CNT.SLP_TYP */
> +        Zero,  /* reserved */
> +        Zero   /* reserved */
> +    })

Do we need \_S4 for S3 support?

> +    Name (\_S5, Package (0x04)
> +    {
> +        Zero,  /* PM1a_CNT.SLP_TYP */
> +        Zero,  /* PM1b_CNT.SLP_TYP */
> +        Zero,  /* reserved */
> +        Zero   /* reserved */
>     })
> }
> diff --git a/bios/acpi-dsdt.hex b/bios/acpi-dsdt.hex
> index 6bc6268..6088b18 100644
> --- a/bios/acpi-dsdt.hex
> +++ b/bios/acpi-dsdt.hex
> @@ -1,22 +1,22 @@
> /*
>  * 
>  * Intel ACPI Component Architecture
> - * ASL Optimizing Compiler version 20060912 [Nov 25 2006]
> + * ASL Optimizing Compiler version 20061109 [May 15 2007]
>  * Copyright (C) 2000 - 2006 Intel Corporation
>  * Supports ACPI Specification Revision 3.0a
>  * 
> - * Compilation of "acpi-dsdt.dsl" - Sun Sep 14 10:27:40 2008
> + * Compilation of "acpi-dsdt.dsl" - Mon Oct 27 10:37:05 2008
>  * 
>  * C source code output
>  *
>  */
> -unsigned char AmlCode[] =
> +const unsigned char AmlCode[] =
> {
> -    0x44,0x53,0x44,0x54,0xC9,0x07,0x00,0x00,  /* 00000000    "DSDT...." */
> -    0x01,0x0E,0x42,0x58,0x50,0x43,0x00,0x00,  /* 00000008    "..BXPC.." */
> +    0x44,0x53,0x44,0x54,0xE1,0x07,0x00,0x00,  /* 00000000    "DSDT...." */
> +    0x01,0x24,0x42,0x58,0x50,0x43,0x00,0x00,  /* 00000008    ".$BXPC.." */
>     0x42,0x58,0x44,0x53,0x44,0x54,0x00,0x00,  /* 00000010    "BXDSDT.." */
>     0x01,0x00,0x00,0x00,0x49,0x4E,0x54,0x4C,  /* 00000018    "....INTL" */
> -    0x12,0x09,0x06,0x20,0x10,0x1C,0x5C,0x00,  /* 00000020    "... ..\." */
> +    0x09,0x11,0x06,0x20,0x10,0x1C,0x5C,0x00,  /* 00000020    "... ..\." */
>     0x5B,0x80,0x44,0x42,0x47,0x5F,0x01,0x0B,  /* 00000028    "[.DBG_.." */
>     0x44,0xB0,0x0A,0x04,0x5B,0x81,0x0B,0x44,  /* 00000030    "D...[..D" */
>     0x42,0x47,0x5F,0x03,0x44,0x42,0x47,0x4C,  /* 00000038    "BG_.DBGL" */
> @@ -260,6 +260,9 @@ unsigned char AmlCode[] =
>     0x8B,0x68,0x01,0x54,0x4D,0x50,0x5F,0x82,  /* 000007A8    ".h.TMP_." */
>     0x54,0x4D,0x50,0x5F,0x60,0x76,0x60,0x70,  /* 000007B0    "TMP_`v`p" */
>     0x60,0x50,0x52,0x51,0x33,0x08,0x5F,0x53,  /* 000007B8    "`PRQ3._S" */
> -    0x35,0x5F,0x12,0x06,0x04,0x00,0x00,0x00,  /* 000007C0    "5_......" */
> +    0x33,0x5F,0x12,0x06,0x04,0x01,0x01,0x00,  /* 000007C0    "3_......" */
> +    0x00,0x08,0x5F,0x53,0x34,0x5F,0x12,0x06,  /* 000007C8    ".._S4_.." */
> +    0x04,0x00,0x00,0x00,0x00,0x08,0x5F,0x53,  /* 000007D0    "......_S" */
> +    0x35,0x5F,0x12,0x06,0x04,0x00,0x00,0x00,  /* 000007D8    "5_......" */
>     0x00,
> };
> diff --git a/bios/rombios.c b/bios/rombios.c
> index 88eac04..03540cb 100644
> --- a/bios/rombios.c
> +++ b/bios/rombios.c
> @@ -2198,6 +2198,31 @@ debugger_off()
>   outb(0xfedc, 0x00);
> }
> 
> +void
> +s3_resume()
> +{
> +    Bit32u s3_wakeup_vector;
> +    Bit8u s3_resume_flag;
> +
> +    s3_resume_flag = read_byte(0x40, 0xb0);
> +    s3_wakeup_vector = read_dword(0x40, 0xb2);
> +
> +    BX_INFO("S3 resume called %x 0x%lx\n", s3_resume_flag, s3_wakeup_vector);
> +    if (s3_resume_flag != 0xFE || !s3_wakeup_vector)
> +     return;
> +
> +    write_byte(0x40, 0xb0, 0);
> +
> +    /* setup wakeup vector */
> +    write_word(0x40, 0xb6, (s3_wakeup_vector & 0xF)); /* IP */
> +    write_word(0x40, 0xb8, (s3_wakeup_vector >> 4)); /* CS */
> +
> +    BX_INFO("S3 resume jump to %x:%x\n", *(Bit16u*)0x04b8, *(Bit16u*)0x04b6);

Is DS always 0x0 here?
Maybe use s3_wakeup_vector & 0xF and s3_wakeup_vector >> 4?

> +ASM_START
> +    jmpf [0x04b6]
> +ASM_END
> +}
> +
> #if BX_USE_ATADRV
> 
> // ---------------------------------------------------------------------------
> @@ -9081,6 +9106,12 @@ retf_post_0x467:
>   mov ss, [0x469]
>   retf
> 
> +s3_post:
> +#if BX_ROMBIOS32
> +  call rombios32_init
> +#endif
> +  call _s3_resume
> +  hlt

Why HLT here? Is IF always 0?
I think we can use BX_PANIC instead:
BX_PANIC("Returned from s3_resume.\n");

> ;--------------------
> eoi_both_pics:
> @@ -10005,6 +10036,10 @@ rombios32_05:
>   ;; init the stack pointer
>   mov esp, #0x00080000
> 
> +  ;; pass pointer to s3_resume_flag and s3_resume_vector to rombios32
> +  push #0x04b0
> +  push #0x04b2
> +
>   ;; call rombios32 code
>   mov eax, #0x00040000
>   call eax
> @@ -10375,6 +10410,12 @@ normal_post:
>   mov  ds, ax
>   mov  ss, ax
> 
> +  ;; Save shutdown status
> +  mov 0x04b0, bl
> +
> +  cmp bl, #0xfe
> +  jz s3_post
> +
>   ;; zero out BIOS data area (40:00..40:ff)
>   mov  es, ax
>   mov  cx, #0x0080 ;; 128 words
> diff --git a/bios/rombios32.c b/bios/rombios32.c
> index f0daf15..b8968e4 100644
> --- a/bios/rombios32.c
> +++ b/bios/rombios32.c
> @@ -180,6 +180,20 @@ void *memmove(void *d1, const void *s1, size_t len)
>     return d1;
> }
> 
> +int memcmp(const void *s1, const void *s2, size_t len)
> +{
> + const int8_t *p1 = s1;
> + const int8_t *p2 = s2;
> +
> + while (len--) {
> + int r = *p1++ - *p2++;
> + if(r)
> + return r;
> + }
> +
> + return 0;
> +}
> +
> size_t strlen(const char *s)
> {
>     const char *s1;
> @@ -644,7 +658,7 @@ static void bios_shadow_init(PCIDevice *d)
> {
>     int v;
> 
> -    if (find_bios_table_area() < 0)
> +    if (bios_table_cur_addr == 0)
>         return;
> 
>     /* remap the BIOS to shadow RAM an keep it read/write while we
> @@ -1461,7 +1475,7 @@ void acpi_bios_init(void)
>     memset(facs, 0, sizeof(*facs));
>     memcpy(facs->signature, "FACS", 4);
>     facs->length = cpu_to_le32(sizeof(*facs));
> -
> +    BX_INFO("Firmware waking vector %p\n", &facs->firmware_waking_vector);

Please keep the empty line before /* DSDT */

>     /* DSDT */
>     memcpy(dsdt, AmlCode, sizeof(AmlCode));
> 
> @@ -2011,9 +2025,48 @@ void smbios_init(void)
>     BX_INFO("SMBIOS table addr=0x%08lx\n", (unsigned long)start);
> }
> 
> -void rombios32_init(void)
> +static uint32_t find_resume_vector(void)
> +{
> +    unsigned long addr, start, end;
> +
> +#ifdef BX_USE_EBDA_TABLES
> +    start = align(ebda_cur_addr, 16);
> +    end = 0xa0000 << 4;

0xA00000? 10MB?

> +#else
> +    if (bios_table_cur_addr == 0)
> +        return 0;
> +    start = align(bios_table_cur_addr, 16);
> +    end = bios_table_end_addr;
> +#endif
> +
> +    for (addr = start; addr < end; addr += 16) {
> +        if (!memcmp((void*)addr, "RSD PTR ", 8)) {
> +            struct rsdp_descriptor *rsdp = (void*)addr;
> +            struct rsdt_descriptor_rev1 *rsdt = (void*)rsdp->rsdt_physical_address;
> +            struct fadt_descriptor_rev1 *fadt = (void*)rsdt->table_offset_entry[0];
> +            struct facs_descriptor_rev1 *facs = (void*)fadt->firmware_ctrl;
> +            return facs->firmware_waking_vector;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void find_440fx(PCIDevice *d)
> +{
> +    uint16_t vendor_id, device_id;
> +
> +    vendor_id = pci_config_readw(d, PCI_VENDOR_ID);
> +    device_id = pci_config_readw(d, PCI_DEVICE_ID);
> +
> +    if (vendor_id == PCI_VENDOR_ID_INTEL && device_id == PCI_DEVICE_ID_INTEL_82441)
> +        i440_pcidev = *d;
> +}
> +
> +void rombios32_init(uint32_t *s3_resume_vector, uint8_t *shutdown_flag)
> {
>     BX_INFO("Starting rombios32\n");
> +    BX_INFO("Shutdown flag %x\n", *shutdown_flag);
> 
> #ifdef BX_QEMU
>     qemu_cfg_port = qemu_cfg_port_probe();
> @@ -2025,6 +2078,21 @@ void rombios32_init(void)
> 
>     smp_probe();
> 
> +    find_bios_table_area();

The return value is no longer used, remove it?

> +
> +    if (*shutdown_flag == 0xfe) {
> +        *s3_resume_vector = find_resume_vector();
> +        if (!*s3_resume_vector) {
> +         BX_INFO("This is S3 resume but wakeup vector is NULL\n");
> +        } else {
> +         BX_INFO("S3 resume vector %p\n", *s3_resume_vector);
> +            /* redirect bios read access to RAM */
> +            pci_for_each_device(find_440fx);
> +            bios_lock_shadow_ram(); /* bios is already copied */
> +            return;
> +        }
> +    }
> +
>     pci_bios_init();
> 
>     if (bios_table_cur_addr != 0) {
> 

- Sebastian