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

[Sebastian Herbszt]

Ryan Harper wrote:

> 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?

I would suggest 0x0F (present, enabled and "more").

It would be nice to runtime detect the presence of the hpet and return the
proper value, e.g. 0x0 if not present and skip the HPET ACPI table creation.
The Xen DSDT does it with the help of a bios info table which gets created at
runtime. It detects the hpet by reading the vendor id from HPET_BASE.
Something like this might also be possible inside the DSDT (OperationRegion,
Field and LEqual).

> > >+            }
> > >+            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.

True.

> > 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
> > >
> > >
> > >