[Qemu-devel] Re: [PATCH] Add HPET support to BIOS

[Ryan Harper]

* Sebastian Herbszt <address@hidden> [2008-07-10 10:46]:

Hey Sebastian, 

Thanks for the review,

> Beth Kon wrote:
> >This patch, written by Ryan Harper, adds HPET support to BIOS.
> >
> >Signed-off-by: Beth Kon <address@hidden>
> >
> >diff --git a/bios/Makefile b/bios/Makefile
> >index 48022ea..3e73fb5 100644
> >--- a/bios/Makefile
> >+++ b/bios/Makefile
> >@@ -40,7 +40,7 @@ LIBS =  -lm
> >RANLIB = ranlib
> >
> >BCC = bcc
> >-GCC = gcc -m32
> >+GCC = gcc -m32 -fno-stack-protector
> >HOST_CC = gcc
> >AS86 = as86
> >
> >diff --git a/bios/acpi-dsdt.dsl b/bios/acpi-dsdt.dsl
> >index d1bfa2c..1548c86 100755
> >--- a/bios/acpi-dsdt.dsl
> >+++ b/bios/acpi-dsdt.dsl
> >@@ -262,6 +262,24 @@ DefinitionBlock (
> >                Return (MEMP)
> >            }
> >        }
> >+        Device(HPET) {
> >+            Name(_HID,  EISAID("PNP0103"))
> >+            Name(_UID, 0)
> 
> _UID is optional if only one timer block is present.

OK
> 
> >+            Method (_STA, 0, NotSerialized) {
> >+                    Return(0x00)
> 
> Not present?

Was playing around with this when trying to get Linux to see the device
in the ACPI tables. AFAICT, Linux doesn't care about this value.  Should
be 1 here then?

> 
> >+            }
> >+            Name(_CRS, ResourceTemplate() {
> >+                DWordMemory(
> >+                    ResourceConsumer, PosDecode, MinFixed, MaxFixed,
> >+                    NonCacheable, ReadWrite,
> >+                    0x00000000,
> >+                    0xFED00000,
> >+                    0xFED003FF,
> >+                    0x00000000,
> >+                    0x00000400 /* 1K memory: FED00000 - FED003FF */
> >+                )
> >+            })
> >+        }
> >    }
> >
> >    Scope(\_SB.PCI0) {
> >@@ -628,7 +646,7 @@ DefinitionBlock (
> >                {
> >                    Or (PRQ3, 0x80, PRQ3)
> >                }
> >-                Method (_CRS, 0, NotSerialized)
> >+                Method (_CRS, 1, NotSerialized)
> >                {
> >                    Name (PRR0, ResourceTemplate ()
> >                    {
> 
> Is this change related?

Doubtful, I'll confirm whether or not it is needed.

> 
> >diff --git a/bios/rombios32.c b/bios/rombios32.c
> >index 2dc1d25..c1ec015 100755
> >--- a/bios/rombios32.c
> >+++ b/bios/rombios32.c
> >@@ -1182,7 +1182,7 @@ struct rsdp_descriptor         /* Root System
> >Descriptor Pointer */
> >struct rsdt_descriptor_rev1
> >{
> > ACPI_TABLE_HEADER_DEF                           /* ACPI common table
> >header */
> >- uint32_t                             table_offset_entry [2]; /* Array
> >of pointers to other */
> >+ uint32_t                             table_offset_entry [3]; /* Array
> >of pointers to other */
> > /* ACPI tables */
> >};
> >
> >@@ -1322,6 +1322,30 @@ struct madt_processor_apic
> >#endif
> >};
> >
> >+/*
> >+ * ACPI 2.0 Generic Address Space definition.
> >+ */
> >+struct acpi_20_generic_address {
> >+    uint8_t  address_space_id;
> >+    uint8_t  register_bit_width;
> >+    uint8_t  register_bit_offset;
> >+    uint8_t  reserved;
> >+    uint64_t address;
> >+};
> >+
> >+/*
> >+ * HPET Description Table
> >+ */
> >+struct acpi_20_hpet {
> >+    ACPI_TABLE_HEADER_DEF                           /* ACPI common
> >table header */
> >+    uint32_t           timer_block_id;
> >+    struct acpi_20_generic_address addr;
> >+    uint8_t            hpet_number;
> >+    uint16_t           min_tick;
> >+    uint8_t            page_protect;
> >+};
> >+#define ACPI_HPET_ADDRESS 0xFED00000UL
> >+
> >struct madt_io_apic
> >{
> > APIC_HEADER_DEF
> >@@ -1393,8 +1417,9 @@ void acpi_bios_init(void)
> >    struct fadt_descriptor_rev1 *fadt;
> >    struct facs_descriptor_rev1 *facs;
> >    struct multiple_apic_table *madt;
> >+    struct acpi_20_hpet *hpet;
> >    uint8_t *dsdt;
> >-    uint32_t base_addr, rsdt_addr, fadt_addr, addr, facs_addr,
> >dsdt_addr;
> >+    uint32_t base_addr, rsdt_addr, fadt_addr, addr, facs_addr,
> >dsdt_addr, hpet_addr;
> >    uint32_t acpi_tables_size, madt_addr, madt_size;
> >    int i;
> >
> >@@ -1436,6 +1461,11 @@ void acpi_bios_init(void)
> >    madt = (void *)(addr);
> >    addr += madt_size;
> >
> >+    addr = (addr + 7) & ~7;
> >+    hpet_addr = addr;
> >+    hpet = (void *)(addr);
> >+    addr += sizeof(*hpet);
> >+
> >    acpi_tables_size = addr - base_addr;
> >
> >    BX_INFO("ACPI tables: RSDP addr=0x%08lx ACPI DATA addr=0x%08lx
> >size=0x%x\n",
> >@@ -1457,6 +1487,7 @@ void acpi_bios_init(void)
> >    memset(rsdt, 0, sizeof(*rsdt));
> >    rsdt->table_offset_entry[0] = cpu_to_le32(fadt_addr);
> >    rsdt->table_offset_entry[1] = cpu_to_le32(madt_addr);
> >+    rsdt->table_offset_entry[2] = cpu_to_le32(hpet_addr);
> >    acpi_build_table_header((struct acpi_table_header *)rsdt,
> >                            "RSDT", sizeof(*rsdt), 1);
> >
> >@@ -1540,6 +1571,15 @@ void acpi_bios_init(void)
> >        acpi_build_table_header((struct acpi_table_header *)madt,
> >                                "APIC", madt_size, 1);
> >    }
> >+
> >+    /* HPET */
> >+    memset(hpet, 0, sizeof(*hpet));
> >+    hpet->timer_block_id = cpu_to_le32(0x8086a201);
> >+   // hpet->timer_block_id = cpu_to_le32(0x80862201);
> 
> This "magic value" could need some explanation so people don't have to look 
> it up.
> Something like:
> 8086 = pci vendor id
> a201 = 1010001000000001
> 1                                  LegacyReplacement IRQ Routing Capable
>  0                                reserved
>   1                               COUNT_SIZE_CAP counter size
>    00010                     Number of Comparators
>             00000001      Hardwave revision id

That's a fair point though one needs the spec to make any sort of
intelligent change anyhow.

> 
> Also add a comment that it should be kept in sync with the emulation 
> (hpet.c).

Yeah, this is an important point as they do need to agree.


> 
> - Sebastian
> 
> >+    hpet->addr.address = cpu_to_le32(ACPI_HPET_ADDRESS);
> >+    acpi_build_table_header((struct  acpi_table_header *)hpet,
> >+                             "HPET", sizeof(*hpet), 1);
> >+
> >}
> >
> >/* SMBIOS entry point -- must be written to a 16-bit aligned address
> >
> >
> >

-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253
address@hidden