SeaBIOS

seabios@seabios.org

4 participants
3801 discussions

Re: [SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
by Marcel Apfelbaum 01 Aug '17

01 Aug '17

On 31/07/2017 22:01, Alexander Bezzubikov wrote: > 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: >>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: >>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote: >>>>> >>>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: >>>>>> >>>>>> On PCI init PCI bridge devices may need some >>>>>> extra info about bus number to reserve, IO, memory and >>>>>> prefetchable memory limits. QEMU can provide this >>>>>> with special vendor-specific PCI capability. >>>>>> >>>>>> This capability is intended to be used only >>>>>> for Red Hat PCI bridges, i.e. QEMU cooperation. >>>>>> >>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>>>>> --- >>>>>> src/fw/dev-pci.h | 62 >>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 62 insertions(+) >>>>>> create mode 100644 src/fw/dev-pci.h >>>>>> >>>>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h >>>>>> new file mode 100644 >>>>>> index 0000000..fbd49ed >>>>>> --- /dev/null >>>>>> +++ b/src/fw/dev-pci.h >>>>>> @@ -0,0 +1,62 @@ >>>>>> +#ifndef _PCI_CAP_H >>>>>> +#define _PCI_CAP_H >>>>>> + >>>>>> +#include "types.h" >>>>>> + >>>>>> +/* >>>>>> + >>>>>> +QEMU-specific vendor(Red Hat)-specific capability. >>>>>> +It's intended to provide some hints for firmware to init PCI devices. >>>>>> + >>>>>> +Its is shown below: >>>>>> + >>>>>> +Header: >>>>>> + >>>>>> +u8 id; Standard PCI Capability Header field >>>>>> +u8 next; Standard PCI Capability Header field >>>>>> +u8 len; Standard PCI Capability Header field >>>>>> +u8 type; Red Hat vendor-specific capability type: >>>>>> + now only REDHAT_QEMU_CAP 1 exists >>>>>> +Data: >>>>>> + >>>>>> +u16 non_prefetchable_16; non-prefetchable memory limit >>>>>> + >>>>>> +u8 bus_res; minimum bus number to reserve; >>>>>> + this is necessary for PCI Express Root Ports >>>>>> + to support PCIE-to-PCI bridge hotplug >>>>>> + >>>>>> +u8 io_8; IO limit in case of 8-bit limit value >>>>>> +u32 io_32; IO limit in case of 16-bit limit value >>>>>> + io_8 and io_16 are mutually exclusive, in other words, >>>>>> + they can't be non-zero simultaneously >>>>>> + >>>>>> +u32 prefetchable_32; non-prefetchable memory limit >>>>>> + in case of 32-bit limit value >>>>>> +u64 prefetchable_64; non-prefetchable memory limit >>>>>> + in case of 64-bit limit value >>>>>> + prefetachable_32 and prefetchable_64 are >>>>>> + mutually exclusive, in other words, >>>>>> + they can't be non-zero simultaneously >>>>>> +If any field in Data section is 0, >>>>>> +it means that such kind of reservation >>>>>> +is not needed. >>> >>> I really don't like this 'mutually exclusive' fields approach because >>> IMHO it increases confusion level when undertanding this capability structure. >>> But - if we came to consensus on that, then IO fields should be used >>> in the same way, >>> because as I understand, this 'mutual exclusivity' serves to distinguish cases >>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT >>> registers. >>> And this is how both IO and PREFETCHABLE works, isn't it? >> >> I would just use simeple 64 bit registers. PCI spec has an ugly format >> with fields spread all over the place but that is because of >> compatibility concerns. It makes not sense to spend cycles just >> to be similarly messy. > > Then I suggest to use exactly one field of a maximum possible size > for each reserving object, and get rid of mutually exclusive fields. > Then it can be something like that (order and names can be changed): > u8 bus; > u16 non_pref; > u32 io; > u64 pref; > I think Michael suggested: u64 bus_res; u64 mem_res; u64 io_res; u64 mem_pref_res; OR: u32 bus_res; u32 mem_res; u32 io_res; u64 mem_pref_res; We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's requests. Thanks, Marcel > >> >> >>>>> >>>>> >>>> >>>> Hi Michael, >>>> >>>>> We also want a way to say "no hint for this type". >>>>> >>>>> One way to achive this would be to have instead multiple >>>>> vendor specific capabilities, one for each of >>>>> bus#/io/mem/prefetch. 0 would mean do not reserve anything, >>>>> absence of capability would mean "no info, up to firmware". >>>>> >>>> >>>> First version of the series was implemented exactly like you propose, >>>> however Gerd preferred only one capability with multiple fields. >>>> >>>> I personally like the simplicity of vendor cap per io/mem/bus, >>>> even if it is on the expense of the limited PCI Config space. >>>> >>> >>> Personally I agree with Marcel since all this fields express >>> reservations of some objects. >>> >>>> We need a consensus here :) >>>> >>> >>> Absolutely :) >>> >>>> Thanks, >>>> Marcel >>>> >>>> >>>>> >>>>>> + >>>>>> +*/ >>>>>> + >>>>>> +/* Offset of vendor-specific capability type field */ >>>>>> +#define PCI_CAP_VNDR_SPEC_TYPE 3 >>>>> >>>>> >>>>> This is a QEMU specific thing. Please name it as such. >>>>> >>>>>> + >>>>>> +/* List of valid Red Hat vendor-specific capability types */ >>>>>> +#define REDHAT_CAP_TYPE_QEMU 1 >>>>>> + >>>>>> + >>>>>> +/* Offsets of QEMU capability fields */ >>>>>> +#define QEMU_PCI_CAP_NON_PREF 4 >>>>>> +#define QEMU_PCI_CAP_BUS_RES 6 >>>>>> +#define QEMU_PCI_CAP_IO_8 7 >>>>>> +#define QEMU_PCI_CAP_IO_32 8 >>>>>> +#define QEMU_PCI_CAP_PREF_32 12 >>>>>> +#define QEMU_PCI_CAP_PREF_64 16 >>>>>> +#define QEMU_PCI_CAP_SIZE 24 >>>>>> + >>>>>> +#endif /* _PCI_CAP_H */ >>>>>> -- >>>>>> 2.7.4 >>>> >>>> >>> >>> >>> >>> -- >>> Alexander Bezzubikov > > >

2 1

Re: [SeaBIOS] [PATCH v3 2/5] hw/pci: introduce pcie-pci-bridge device
by Michael S. Tsirkin 01 Aug '17

01 Aug '17

On Mon, Jul 31, 2017 at 09:40:41PM +0300, Alexander Bezzubikov wrote: > >> +typedef struct PCIEPCIBridge { > >> + /*< private >*/ > >> + PCIBridge parent_obj; > >> + > >> + bool msi_enable; > > > > > > Please rename the msi_enable property to "msi" in order > > to be aligned with the existent PCIBridgeDev and > > consider making it OnOffAuto for the same reason. > > (I am not sure about the last part though, we have > > no meaning for "auto" here) > > > > Agreed about "msi", but OnOffAuto looks weird to me > as we always want MSI to be enabled. Why even have a property then? Can't you enable it unconditionally? -- MST

2 3

Re: [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware
by Michael S. Tsirkin 31 Jul '17

31 Jul '17

On Thu, Jul 27, 2017 at 12:54:07AM +0300, Alexander Bezzubikov wrote: > 2017-07-26 22:43 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: > > On Sun, Jul 23, 2017 at 01:15:41AM +0300, Aleksandr Bezzubikov wrote: > >> On PCI init PCI bridges may need some > >> extra info about bus number to reserve, IO, memory and > >> prefetchable memory limits. QEMU can provide this > >> with special > > > > with a special > > > >> vendor-specific PCI capability. > >> > >> Sizes of limits match ones from > >> PCI Type 1 Configuration Space Header, > >> number of buses to reserve occupies only 1 byte > >> since it is the size of Subordinate Bus Number register. > >> > >> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> > >> --- > >> hw/pci/pci_bridge.c | 27 +++++++++++++++++++++++++++ > >> include/hw/pci/pci_bridge.h | 18 ++++++++++++++++++ > >> 2 files changed, 45 insertions(+) > >> > >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c > >> index 720119b..8ec6c2c 100644 > >> --- a/hw/pci/pci_bridge.c > >> +++ b/hw/pci/pci_bridge.c > >> @@ -408,6 +408,33 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, > >> br->bus_name = bus_name; > >> } > >> > >> + > >> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset, > > > > help? should be qemu_cap_init? > > > >> + uint8_t bus_reserve, uint32_t io_limit, > >> + uint16_t mem_limit, uint64_t pref_limit, > >> + Error **errp) > >> +{ > >> + size_t cap_len = sizeof(PCIBridgeQemuCap); > >> + PCIBridgeQemuCap cap; > > > > This leaks info to guest. You want to init all fields here: > > > > cap = { > > .len = .... > > }; > > I surely can do this for len field, but as Laszlo proposed > we can use mutually exclusive fields, > e.g. pref_32 and pref_64, the only way I have left > is to use ternary operator (if we surely need this > big initializer). Keeping some if's would look better, > I think. > > > > >> + > >> + cap.len = cap_len; > >> + cap.bus_res = bus_reserve; > >> + cap.io_lim = io_limit & 0xFF; > >> + cap.io_lim_upper = io_limit >> 8 & 0xFFFF; > >> + cap.mem_lim = mem_limit; > >> + cap.pref_lim = pref_limit & 0xFFFF; > >> + cap.pref_lim_upper = pref_limit >> 16 & 0xFFFFFFFF; > > > > Please use pci_set_word etc or cpu_to_leXX. > > > > Since now we've decided to avoid fields separation into <field> + <field_upper>, > this bitmask along with pci_set_word are no longer needed. > > > I think it's easiest to replace struct with a set of macros then > > pci_set_word does the work for you. > > > > I don't really want to use macros here because structure > show us the whole capability layout and this can > decrease documenting efforts. More than that, > memcpy usage is very convenient here, and I wouldn't like > to lose it. > > > > >> + > >> + int offset = pci_add_capability(dev, PCI_CAP_ID_VNDR, > >> + cap_offset, cap_len, errp); > >> + if (offset < 0) { > >> + return offset; > >> + } > >> + > >> + memcpy(dev->config + offset + 2, (char *)&cap + 2, cap_len - 2); > > > > +2 is yacky. See how virtio does it: > > > > memcpy(dev->config + offset + PCI_CAP_FLAGS, &cap->cap_len, > > cap->cap_len - PCI_CAP_FLAGS); > > > > > > OK. > > >> + return 0; > >> +} > >> + > >> static const TypeInfo pci_bridge_type_info = { > >> .name = TYPE_PCI_BRIDGE, > >> .parent = TYPE_PCI_DEVICE, > >> diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h > >> index ff7cbaa..c9f642c 100644 > >> --- a/include/hw/pci/pci_bridge.h > >> +++ b/include/hw/pci/pci_bridge.h > >> @@ -67,4 +67,22 @@ void pci_bridge_map_irq(PCIBridge *br, const char* bus_name, > >> #define PCI_BRIDGE_CTL_DISCARD_STATUS 0x400 /* Discard timer status */ > >> #define PCI_BRIDGE_CTL_DISCARD_SERR 0x800 /* Discard timer SERR# enable */ > >> > >> +typedef struct PCIBridgeQemuCap { > >> + uint8_t id; /* Standard PCI capability header field */ > >> + uint8_t next; /* Standard PCI capability header field */ > >> + uint8_t len; /* Standard PCI vendor-specific capability header field */ > >> + uint8_t bus_res; > >> + uint32_t pref_lim_upper; > > > > Big endian? Ugh. > > > > Agreed, and this's gonna to disappear with > the new layout. > > >> + uint16_t pref_lim; > >> + uint16_t mem_lim; > > > > I'd say we need 64 bit for memory. > > > > Why? Non-prefetchable MEMORY_LIMIT register is 16 bits long. Hmm ok, but e.g. for io there are bridges that have extra registers to specify non-standard non-aligned registers. > >> + uint16_t io_lim_upper; > >> + uint8_t io_lim; > >> + uint8_t padding; > > > > IMHO each type should have a special "don't care" flag > > that would mean "I don't know". > > > > > > Don't know what? Now 0 is an indicator to do nothing with this field. In that case how do you say "don't allocate any memory"? > >> +} PCIBridgeQemuCap; > > > > You don't really need this struct in the header. And pls document all fields. > > > >> + > >> +int pci_bridge_help_cap_init(PCIDevice *dev, int cap_offset, > >> + uint8_t bus_reserve, uint32_t io_limit, > >> + uint16_t mem_limit, uint64_t pref_limit, > >> + Error **errp); > >> + > >> #endif /* QEMU_PCI_BRIDGE_H */ > >> -- > >> 2.7.4 > > > > -- > Alexander Bezzubikov

3 9

Re: [SeaBIOS] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure
by Michael S. Tsirkin 31 Jul '17

31 Jul '17

On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote: > 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com>: > > On 31/07/2017 17:00, Michael S. Tsirkin wrote: > >> > >> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote: > >>> > >>> On PCI init PCI bridge devices may need some > >>> extra info about bus number to reserve, IO, memory and > >>> prefetchable memory limits. QEMU can provide this > >>> with special vendor-specific PCI capability. > >>> > >>> This capability is intended to be used only > >>> for Red Hat PCI bridges, i.e. QEMU cooperation. > >>> > >>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> > >>> --- > >>> src/fw/dev-pci.h | 62 > >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>> 1 file changed, 62 insertions(+) > >>> create mode 100644 src/fw/dev-pci.h > >>> > >>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h > >>> new file mode 100644 > >>> index 0000000..fbd49ed > >>> --- /dev/null > >>> +++ b/src/fw/dev-pci.h > >>> @@ -0,0 +1,62 @@ > >>> +#ifndef _PCI_CAP_H > >>> +#define _PCI_CAP_H > >>> + > >>> +#include "types.h" > >>> + > >>> +/* > >>> + > >>> +QEMU-specific vendor(Red Hat)-specific capability. > >>> +It's intended to provide some hints for firmware to init PCI devices. > >>> + > >>> +Its is shown below: > >>> + > >>> +Header: > >>> + > >>> +u8 id; Standard PCI Capability Header field > >>> +u8 next; Standard PCI Capability Header field > >>> +u8 len; Standard PCI Capability Header field > >>> +u8 type; Red Hat vendor-specific capability type: > >>> + now only REDHAT_QEMU_CAP 1 exists > >>> +Data: > >>> + > >>> +u16 non_prefetchable_16; non-prefetchable memory limit > >>> + > >>> +u8 bus_res; minimum bus number to reserve; > >>> + this is necessary for PCI Express Root Ports > >>> + to support PCIE-to-PCI bridge hotplug > >>> + > >>> +u8 io_8; IO limit in case of 8-bit limit value > >>> +u32 io_32; IO limit in case of 16-bit limit value > >>> + io_8 and io_16 are mutually exclusive, in other words, > >>> + they can't be non-zero simultaneously > >>> + > >>> +u32 prefetchable_32; non-prefetchable memory limit > >>> + in case of 32-bit limit value > >>> +u64 prefetchable_64; non-prefetchable memory limit > >>> + in case of 64-bit limit value > >>> + prefetachable_32 and prefetchable_64 are > >>> + mutually exclusive, in other words, > >>> + they can't be non-zero simultaneously > >>> +If any field in Data section is 0, > >>> +it means that such kind of reservation > >>> +is not needed. > > I really don't like this 'mutually exclusive' fields approach because > IMHO it increases confusion level when undertanding this capability structure. > But - if we came to consensus on that, then IO fields should be used > in the same way, > because as I understand, this 'mutual exclusivity' serves to distinguish cases > when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT > registers. > And this is how both IO and PREFETCHABLE works, isn't it? I would just use simeple 64 bit registers. PCI spec has an ugly format with fields spread all over the place but that is because of compatibility concerns. It makes not sense to spend cycles just to be similarly messy. > >> > >> > > > > Hi Michael, > > > >> We also want a way to say "no hint for this type". > >> > >> One way to achive this would be to have instead multiple > >> vendor specific capabilities, one for each of > >> bus#/io/mem/prefetch. 0 would mean do not reserve anything, > >> absence of capability would mean "no info, up to firmware". > >> > > > > First version of the series was implemented exactly like you propose, > > however Gerd preferred only one capability with multiple fields. > > > > I personally like the simplicity of vendor cap per io/mem/bus, > > even if it is on the expense of the limited PCI Config space. > > > > Personally I agree with Marcel since all this fields express > reservations of some objects. > > > We need a consensus here :) > > > > Absolutely :) > > > Thanks, > > Marcel > > > > > >> > >>> + > >>> +*/ > >>> + > >>> +/* Offset of vendor-specific capability type field */ > >>> +#define PCI_CAP_VNDR_SPEC_TYPE 3 > >> > >> > >> This is a QEMU specific thing. Please name it as such. > >> > >>> + > >>> +/* List of valid Red Hat vendor-specific capability types */ > >>> +#define REDHAT_CAP_TYPE_QEMU 1 > >>> + > >>> + > >>> +/* Offsets of QEMU capability fields */ > >>> +#define QEMU_PCI_CAP_NON_PREF 4 > >>> +#define QEMU_PCI_CAP_BUS_RES 6 > >>> +#define QEMU_PCI_CAP_IO_8 7 > >>> +#define QEMU_PCI_CAP_IO_32 8 > >>> +#define QEMU_PCI_CAP_PREF_32 12 > >>> +#define QEMU_PCI_CAP_PREF_64 16 > >>> +#define QEMU_PCI_CAP_SIZE 24 > >>> + > >>> +#endif /* _PCI_CAP_H */ > >>> -- > >>> 2.7.4 > > > > > > > > -- > Alexander Bezzubikov

2 1

[PATCH v3 0/3] Allow RedHat PCI bridges reserve more buses than necessary during init
by Aleksandr Bezzubikov 31 Jul '17

31 Jul '17

Now PCI bridges get a bus range number on a system init, basing on currently plugged devices. That's why when one wants to hotplug another bridge, it needs his child bus, which the parent is unable to provide (speaking about virtual device). The suggested workaround is to have vendor-specific capability in Red Hat PCI bridges that contains number of additional bus to reserve (as well as various space limit hints, unused for now) on BIOS PCI init. So this capability is intented only for pure QEMU->SeaBIOS usage. Considering all aforesaid, this series is directly connected with QEMU series (v3) "Generic PCIE-PCI Bridge". Although the new PCI capability is supposed to contain various limits along with bus number to reserve, now only its full layout is proposed. And only bus_reserve field is used in QEMU and BIOS. Limits usage is still a subject for implementation as now the main goal of this series to provide necessary support from the firmware side to PCIE-PCI bridge hotplug. Changes v2->v3: 1. Merge commit 2 (Red Hat vendor ID) into commit 4 - addresses Marcel's comment, and add Generic PCIE Root Port device ID - addresses Michael's comment. 2. Changes of the capability layout (QEMU side has the same changes): - add 'type' field to distinguish multiple RedHat-specific capabilities - addresses Michael's comment - do not mimiс PCI Config space register layout, but use mutually exclusive differently sized fields for IO and prefetchable memory limits - addresses Laszlo's comment - use defines instead of structure and offsetof - addresses Michael's comment 3. Interpret 'bus_reserve' field as a minimum necessary range to reserve - addresses Gerd's comment 4. pci_find_capability moved to pci.c - addresses Kevin's comment 5. Move capability layout header to src/fw/dev-pci.h - addresses Kevin's comment 6. Add the capability documentation - addresses Michael's comment 7. Add capability length and bus_reserve field sanity checks - addresses Michael's comment Changes v1->v2: 1. New #define for Red Hat vendor added (addresses Konrad's comment). 2. Refactored pci_find_capability function (addresses Marcel's comment). 3. Capability reworked: - data type added; - reserve space in a structure for IO, memory and prefetchable memory limits. Aleksandr Bezzubikov (3): pci: refactor pci_find_capapibilty to get bdf as the first argument instead of the whole pci_device pci: add QEMU-specific PCI capability structure pci: enable RedHat PCI bridges to reserve additional buses on PCI init src/fw/dev-pci.h | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/fw/pciinit.c | 41 +++++++++++++++++++++++++++++++---- src/hw/pci.c | 25 +++++++++++++++++++++ src/hw/pci.h | 1 + src/hw/pci_ids.h | 3 +++ src/hw/pcidevice.c | 24 --------------------- src/hw/pcidevice.h | 1 - src/hw/virtio-pci.c | 6 +++--- src/types.h | 2 ++ 9 files changed, 133 insertions(+), 32 deletions(-) create mode 100644 src/fw/dev-pci.h -- 2.7.4

5 10

Re: [SeaBIOS] [FWD: Request for Chromebook Authentication Module Development]
by victor 31 Jul '17

31 Jul '17

> Hi > > > We are looking for developers to develop an authentication module in the > standard Chromebook OS/firmware. > > > 1) The purpose of this authentication module is to ‘lock' the device > at factory during manufacturing. > A lock PIN consist of 10-12 numeric is written to the device memory. > > > 2) During first boot up, the device will prompt for a login pin. > > > 3) If there is a matched between the login pin and the lock pin stored > in the device memory, the Chromebook will be ‘unlocked’ and normal > operation can proceed. If the PIN numbers are not matched, the > Chromebook will remain lock. (see attached flow-chart) > > > 4) The development needs not be confined to only firmware. We are open > to all options (example thru apps) so long the authentication process > can be achieved. > > > We are writing to request whether if any Chromebook expert/specialist > can provide Non-recurring engineering (NRE) services for the > development. Please do contact the undersign should you require more > info. > > > > > > Hope to hear from you > > > > -- > Best Regards > Victor Chng > > Send from my iphone > > > > > -- Best Regards Victor Chng Send from my iphone

1 0

[qemu PATCH for 2.10] i386: acpi: provide an XSDT instead of an RSDT
by Paolo Bonzini 29 Jul '17

29 Jul '17

The tables that QEMU provides are not ACPI 1.0 compatible since commit 77af8a2b95 ("hw/i386: Use Rev3 FADT (ACPI 2.0) instead of Rev1 to improve guest OS support.", 2017-05-03). This is visible with Windows 2000, which refuses to parse the rev3 FADT and fails to boot. The recommended solution in this case is to build two FADTs, v1 being pointed to by the RSDT and v3 by the XSDT. However, we leave this task to the firmware. This patch simply switches the RSDT to the XSDT, which is valid for all ACPI 2.0-friendly operating systems and also leaves SeaBIOS the freedom to build an RSDT that points to the compatibility FADT. When running Windows 2000 with an old BIOS, Windows would simply fall back to a non-ACPI HAL; however, the plan should be to include a BIOS with the new feature in 2.10. Reported-by: Programmingkid <programmingkidx(a)gmail.com> Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- hw/acpi/aml-build.c | 27 --------------------------- hw/i386/acpi-build.c | 30 ++++++++++++++++++------------ include/hw/acpi/aml-build.h | 3 --- 3 files changed, 18 insertions(+), 42 deletions(-) diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 36a6cc450e..ea750d54d9 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -1573,33 +1573,6 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre) g_array_free(tables->vmgenid, mfre); } -/* Build rsdt table */ -void -build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, - const char *oem_id, const char *oem_table_id) -{ - int i; - unsigned rsdt_entries_offset; - AcpiRsdtDescriptorRev1 *rsdt; - const unsigned table_data_len = (sizeof(uint32_t) * table_offsets->len); - const unsigned rsdt_entry_size = sizeof(rsdt->table_offset_entry[0]); - const size_t rsdt_len = sizeof(*rsdt) + table_data_len; - - rsdt = acpi_data_push(table_data, rsdt_len); - rsdt_entries_offset = (char *)rsdt->table_offset_entry - table_data->data; - for (i = 0; i < table_offsets->len; ++i) { - uint32_t ref_tbl_offset = g_array_index(table_offsets, uint32_t, i); - uint32_t rsdt_entry_offset = rsdt_entries_offset + rsdt_entry_size * i; - - /* rsdt->table_offset_entry to be filled by Guest linker */ - bios_linker_loader_add_pointer(linker, - ACPI_BUILD_TABLE_FILE, rsdt_entry_offset, rsdt_entry_size, - ACPI_BUILD_TABLE_FILE, ref_tbl_offset); - } - build_header(linker, table_data, - (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id); -} - /* Build xsdt table */ void build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 6b7bade183..e76cad2ad7 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2557,27 +2557,33 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) } static GArray * -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset) +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) { AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp); - unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address); - unsigned rsdt_pa_offset = - (char *)&rsdp->rsdt_physical_address - rsdp_table->data; + unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address); + unsigned xsdt_pa_offset = + (char *)&rsdp->xsdt_physical_address - rsdp_table->data; bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16, true /* fseg memory */); memcpy(&rsdp->signature, "RSD PTR ", 8); memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6); + rsdp->length = cpu_to_le32(sizeof(*rsdp)); + rsdp->revision = 0x02; + /* Address to be filled by Guest linker */ bios_linker_loader_add_pointer(linker, - ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size, - ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset); + ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size, + ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset); /* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, - (char *)rsdp - rsdp_table->data, sizeof *rsdp, + (char *)rsdp - rsdp_table->data, offsetof(AcpiRsdpDescriptor, length), (char *)&rsdp->checksum - rsdp_table->data); + bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, + (char *)rsdp - rsdp_table->data, sizeof *rsdp, + (char *)&rsdp->extended_checksum - rsdp_table->data); return rsdp_table; } @@ -2621,7 +2627,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) PCMachineState *pcms = PC_MACHINE(machine); PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); GArray *table_offsets; - unsigned facs, dsdt, rsdt, fadt; + unsigned facs, dsdt, xsdt, fadt; AcpiPmInfo pm; AcpiMiscInfo misc; AcpiMcfgInfo mcfg; @@ -2729,13 +2735,13 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) g_array_append_vals(tables_blob, u, len); } - /* RSDT is pointed to by RSDP */ - rsdt = tables_blob->len; - build_rsdt(tables_blob, tables->linker, table_offsets, + /* XSDT is pointed to by RSDP */ + xsdt = tables_blob->len; + build_xsdt(tables_blob, tables->linker, table_offsets, slic_oem.id, slic_oem.table_id); /* RSDP is in FSEG memory, so allocate it separately */ - build_rsdp(tables->rsdp, tables->linker, rsdt); + build_rsdp(tables->rsdp, tables->linker, xsdt); /* We'll expose it all to Guest so we want to reduce * chance of size changes. diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index 88d0738d76..4bdd88ee23 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -379,9 +379,6 @@ void acpi_add_table(GArray *table_offsets, GArray *table_data); void acpi_build_tables_init(AcpiBuildTables *tables); void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre); void -build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, - const char *oem_id, const char *oem_table_id); -void build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, const char *oem_id, const char *oem_table_id); -- 2.13.3

5 10

Re: [SeaBIOS] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port
by Marcel Apfelbaum 28 Jul '17

28 Jul '17

On 25/07/2017 17:09, Alexander Bezzubikov wrote: > 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >> On Tue, Jul 25, 2017 at 04:50:49PM +0300, Alexander Bezzubikov wrote: >>> 2017-07-25 16:43 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >>>> On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote: >>>>> On 23/07/2017 15:22, Michael S. Tsirkin wrote: >>>>>> On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote: >>>>>>> To enable hotplugging of a newly created pcie-pci-bridge, >>>>>>> we need to tell firmware (SeaBIOS in this case) >>>>>> >>>>> >>>>> Hi Michael, >>>>> >>>>>> Presumably, EFI would need to support this too? >>>>>> >>>>> >>>>> Sure, Eduardo added to CC, but he is in PTO now. >>>>> >>>>>>> to reserve >>>>>>> additional buses for pcie-root-port, that allows us to >>>>>>> hotplug pcie-pci-bridge into this root port. >>>>>>> The number of buses to reserve is provided to the device via a corresponding >>>>>>> property, and to the firmware via new PCI capability (next patch). >>>>>>> The property's default value is 1 as we want to hotplug at least 1 bridge. >>>>>> >>>>>> If so you should just teach firmware to allocate one bus # >>>>>> unconditionally. >>>>>> >>>>> >>>>> That would be a problem for the PCIe machines, since each PCIe >>>>> devices is plugged in a different bus and we are already >>>>> limited to 256 PCIe devices. Allocating an extra-bus always >>>>> would really limit the PCIe devices we can use. >>>> >>>> One of the declared advantages of PCIe is easy support for multiple roots. >>>> We really should look at that IMHO so we do not need to pile up hacks. >>>> >>>>>> But why would that be so? What's wrong with a device >>>>>> directly in the root port? >>>>>> >>>> >>>> To clarify, my point is we might be wasting bus numbers by reservation >>>> since someone might just want to put pcie devices there. >>> >>> I think, changing default value to 0 can help us avoid this, >>> as no bus reservation by default. If one's surely wants >>> to hotplug pcie-pci-bridge into this root port in future, >>> the property gives him such an opportunity. >>> So, sure need pcie-pci-bridge hotplug -> creating a root port with >>> bus_reserve > 0. Otherwise (and default) - just as now, no changes >>> in bus topology. >> >> I guess 0 should mean "do not reserve any buses". So I think we also >> need a flag to just avoid the capability altogether. Maybe -1? *That* >> should be the default. > > -1 might be useful if any limit value 0 is legal, but is it? > If not, we can set every field to 0 and > this is a sign of avoiding capability since none legal > values are provided. > As Gerd suggested, this value is not a "delta" but the number of buses to be reserved behind the bridge. If I got it right, 0 is not a valid value, since the bridge by definition has a list one bus behind. Michael, would you be OK with that? Thanks, Marcel >> >>>> >>>>> First, plugging a legacy PCI device into a PCIe Root Port >>>>> looks strange at least, and it can;t be done on real HW anyway. >>>>> (incompatible slots) >>>>> >>>>> Second (and more important), if we want 2 or more PCI >>>>> devices we would loose both IO ports space and bus numbers. >>>>> >>>>>> >>>>>>> >>>>>>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>>>>>> --- >>>>>>> hw/pci-bridge/pcie_root_port.c | 1 + >>>>>>> include/hw/pci/pcie_port.h | 3 +++ >>>>>>> 2 files changed, 4 insertions(+) >>>>>>> >>>>>>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c >>>>>>> index 4d588cb..b0e49e1 100644 >>>>>>> --- a/hw/pci-bridge/pcie_root_port.c >>>>>>> +++ b/hw/pci-bridge/pcie_root_port.c >>>>>>> @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d) >>>>>>> static Property rp_props[] = { >>>>>>> DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present, >>>>>>> QEMU_PCIE_SLTCAP_PCP_BITNR, true), >>>>>>> + DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1), >>>>>>> DEFINE_PROP_END_OF_LIST() >>>>>>> }; >>>>>>> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h >>>>>>> index 1333266..1b2dd1f 100644 >>>>>>> --- a/include/hw/pci/pcie_port.h >>>>>>> +++ b/include/hw/pci/pcie_port.h >>>>>>> @@ -34,6 +34,9 @@ struct PCIEPort { >>>>>>> /* pci express switch port */ >>>>>>> uint8_t port; >>>>>>> + >>>>>>> + /* additional buses to reserve on firmware init */ >>>>>>> + uint8_t bus_reserve; >>>>>>> }; >>>>>>> void pcie_port_init_reg(PCIDevice *d); >>>>>> >>>>>> So here is a property and it does not do anything. >>>>>> It makes it easier to work on series maybe, but review >>>>>> is harder since we do not see what it does at all. >>>>>> Please do not split up patches like this - you can maintain >>>>>> it split up in your branch if you like and merge before sending. >>>>>> >>>>> >>>>> Agreed, Alexandr please merge patches 4-5-6 for your next submission. >>>>> >>>>> Thanks, >>>>> Marcel >>>>> >>>>> >>>>>>> -- >>>>>>> 2.7.4 >>> >>> >>> >>> -- >>> Alexander Bezzubikov > > >

3 2

Re: [SeaBIOS] [RFC PATCH v2 5/6] hw/pci: add bus_reserve property to pcie-root-port
by Michael S. Tsirkin 28 Jul '17

28 Jul '17

On Tue, Jul 25, 2017 at 12:41:12AM +0300, Alexander Bezzubikov wrote: > 2017-07-24 23:46 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: > > On Sun, Jul 23, 2017 at 05:13:11PM +0300, Marcel Apfelbaum wrote: > >> On 23/07/2017 15:22, Michael S. Tsirkin wrote: > >> > On Sun, Jul 23, 2017 at 01:15:42AM +0300, Aleksandr Bezzubikov wrote: > >> > > To enable hotplugging of a newly created pcie-pci-bridge, > >> > > we need to tell firmware (SeaBIOS in this case) > >> > > >> > >> Hi Michael, > >> > >> > Presumably, EFI would need to support this too? > >> > > >> > >> Sure, Eduardo added to CC, but he is in PTO now. > >> > >> > > to reserve > >> > > additional buses for pcie-root-port, that allows us to > >> > > hotplug pcie-pci-bridge into this root port. > >> > > The number of buses to reserve is provided to the device via a corresponding > >> > > property, and to the firmware via new PCI capability (next patch). > >> > > The property's default value is 1 as we want to hotplug at least 1 bridge. > >> > > >> > If so you should just teach firmware to allocate one bus # > >> > unconditionally. > >> > > >> > >> That would be a problem for the PCIe machines, since each PCIe > >> devices is plugged in a different bus and we are already > >> limited to 256 PCIe devices. Allocating an extra-bus always > >> would really limit the PCIe devices we can use. > > > > But this is exactly what this patch does as the property is added to all > > buses and default to 1 (1 extra bus). > > > >> > But why would that be so? What's wrong with a device > >> > directly in the root port? > >> > > >> > >> First, plugging a legacy PCI device into a PCIe Root Port > >> looks strange at least, and it can;t be done on real HW anyway. > >> (incompatible slots) > > > > You can still plug in PCIe devices there. > > > >> Second (and more important), if we want 2 or more PCI > >> devices we would loose both IO ports space and bus numbers. > > > > What I am saying is maybe default should not be 1. > > The only sensible variant left is 0. > But as we want pcie-pci-bridge to be used for every legacy PCI device > on q35 machine, every time one hotplugs the bridge into the root port, > he must be sure rp's prop value >0 (for Linux). I'm not sure > that it is a very convenient way to utilize the bridge - always remember > to set property. That's what I'm saying then - if in your opinion default is >0 anyway, tweak firmware to do it by default. > Another way - we can set this to 0 by default, and to 1 for pcie-root-port, > and recommend to use it for hotplugging of the pcie-pci-bridge itself. I wonder about something: imagine hotplugging a hierarchy of bridges below a root port. It seems that nothing prevents guest from finding a free range of buses to cover this hierarchy and setting that as secondary/subordinate bus for this bridge. This does need support on QEMU side to hotplug a hierarchy at once, and might need some fixes in Linux, on the plus side you can defer management decision on how many are needed until you are actually adding something, and you don't need vendor specific patches. > > > >> > > >> > > > >> > > Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> > >> > > --- > >> > > hw/pci-bridge/pcie_root_port.c | 1 + > >> > > include/hw/pci/pcie_port.h | 3 +++ > >> > > 2 files changed, 4 insertions(+) > >> > > > >> > > diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c > >> > > index 4d588cb..b0e49e1 100644 > >> > > --- a/hw/pci-bridge/pcie_root_port.c > >> > > +++ b/hw/pci-bridge/pcie_root_port.c > >> > > @@ -137,6 +137,7 @@ static void rp_exit(PCIDevice *d) > >> > > static Property rp_props[] = { > >> > > DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present, > >> > > QEMU_PCIE_SLTCAP_PCP_BITNR, true), > >> > > + DEFINE_PROP_UINT8("bus_reserve", PCIEPort, bus_reserve, 1), > >> > > DEFINE_PROP_END_OF_LIST() > >> > > }; > >> > > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h > >> > > index 1333266..1b2dd1f 100644 > >> > > --- a/include/hw/pci/pcie_port.h > >> > > +++ b/include/hw/pci/pcie_port.h > >> > > @@ -34,6 +34,9 @@ struct PCIEPort { > >> > > /* pci express switch port */ > >> > > uint8_t port; > >> > > + > >> > > + /* additional buses to reserve on firmware init */ > >> > > + uint8_t bus_reserve; > >> > > }; > >> > > void pcie_port_init_reg(PCIDevice *d); > >> > > >> > So here is a property and it does not do anything. > >> > It makes it easier to work on series maybe, but review > >> > is harder since we do not see what it does at all. > >> > Please do not split up patches like this - you can maintain > >> > it split up in your branch if you like and merge before sending. > >> > > >> > >> Agreed, Alexandr please merge patches 4-5-6 for your next submission. > >> > >> Thanks, > >> Marcel > >> > >> > >> > > -- > >> > > 2.7.4 > > > > -- > Alexander Bezzubikov

2 2

Re: [SeaBIOS] [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support
by Paolo Bonzini 28 Jul '17

28 Jul '17

On 25/07/2017 18:23, Paolo Bonzini wrote: > On 25/07/2017 18:14, Laszlo Ersek wrote: >> "No regressions became apparent in tests with a range of Windows >> (XP-10)" >> >> In theory, w2k falls within that range. > > Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :( > > One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT > and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself, > patching the RSDT to point to it. > > It's a hack, but it's the only place I see to make it "just work". And > it could be extended nicely in the future. > > All QEMU would have to do is to provide an XSDT _instead_ of an RSDT. Completely untested code follows. Machine type compatibility code should not be necessary because new QEMU always starts with a new BIOS. Not sure I'll have much time for this in the next few days, so help is appreciated. Paolo QEMU: diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 36a6cc450e..ea750d54d9 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -1573,33 +1573,6 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre) g_array_free(tables->vmgenid, mfre); } -/* Build rsdt table */ -void -build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, - const char *oem_id, const char *oem_table_id) -{ - int i; - unsigned rsdt_entries_offset; - AcpiRsdtDescriptorRev1 *rsdt; - const unsigned table_data_len = (sizeof(uint32_t) * table_offsets->len); - const unsigned rsdt_entry_size = sizeof(rsdt->table_offset_entry[0]); - const size_t rsdt_len = sizeof(*rsdt) + table_data_len; - - rsdt = acpi_data_push(table_data, rsdt_len); - rsdt_entries_offset = (char *)rsdt->table_offset_entry - table_data->data; - for (i = 0; i < table_offsets->len; ++i) { - uint32_t ref_tbl_offset = g_array_index(table_offsets, uint32_t, i); - uint32_t rsdt_entry_offset = rsdt_entries_offset + rsdt_entry_size * i; - - /* rsdt->table_offset_entry to be filled by Guest linker */ - bios_linker_loader_add_pointer(linker, - ACPI_BUILD_TABLE_FILE, rsdt_entry_offset, rsdt_entry_size, - ACPI_BUILD_TABLE_FILE, ref_tbl_offset); - } - build_header(linker, table_data, - (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id); -} - /* Build xsdt table */ void build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets, diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 6b7bade183..ad00f6affd 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2557,12 +2557,12 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker) } static GArray * -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset) +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset) { AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp); - unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address); - unsigned rsdt_pa_offset = - (char *)&rsdp->rsdt_physical_address - rsdp_table->data; + unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address); + unsigned xsdt_pa_offset = + (char *)&rsdp->xsdt_physical_address - rsdp_table->data; bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16, true /* fseg memory */); @@ -2571,8 +2571,8 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset) memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6); /* Address to be filled by Guest linker */ bios_linker_loader_add_pointer(linker, - ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size, - ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset); + ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size, + ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset); /* Checksum to be filled by Guest linker */ bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE, @@ -2621,7 +2621,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) PCMachineState *pcms = PC_MACHINE(machine); PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); GArray *table_offsets; - unsigned facs, dsdt, rsdt, fadt; + unsigned facs, dsdt, xsdt, fadt; AcpiPmInfo pm; AcpiMiscInfo misc; AcpiMcfgInfo mcfg; @@ -2730,12 +2730,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine) } /* RSDT is pointed to by RSDP */ - rsdt = tables_blob->len; - build_rsdt(tables_blob, tables->linker, table_offsets, + xsdt = tables_blob->len; + build_xsdt(tables_blob, tables->linker, table_offsets, slic_oem.id, slic_oem.table_id); /* RSDP is in FSEG memory, so allocate it separately */ - build_rsdp(tables->rsdp, tables->linker, rsdt); + build_rsdp(tables->rsdp, tables->linker, xsdt); /* We'll expose it all to Guest so we want to reduce * chance of size changes. SeaBIOS: >From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini(a)redhat.com> Date: Tue, 25 Jul 2017 18:50:19 +0200 Subject: [PATCH 1/2] seabios: build RSDT from XSDT Old operating systems would like to have a v1 FADT, but new operating systems would like to have v3. Since old operating systems do not know about XSDTs, the solution is to point the RSDT to a v1 FADT and the XSDT to a v3 FADT. But, OVMF is not able to do that and barfs when it sees the second FADT---plus really it's only BIOS operating systems such as win2k that complain. So instead: 1) make QEMU provide an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS. This patch makes SeaBIOS build an RSDT out of an existing XSDT. Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- src/fw/paravirt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- src/std/acpi.h | 11 +++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c index 5b23d78..19a4abe 100644 --- a/src/fw/paravirt.c +++ b/src/fw/paravirt.c @@ -25,6 +25,7 @@ #include "x86.h" // cpuid #include "xen.h" // xen_biostable_setup #include "stacks.h" // yield +#include "std/acpi.h" // Amount of continuous ram under 4Gig u32 RamSize; @@ -147,6 +148,46 @@ static void msr_feature_control_setup(void) wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits); } +static void +build_compatibility_rsdt(void) +{ + if (RsdpAddr->rsdt_physical_address) + return; + + u64 xsdt_addr = RsdpAddr->xsdt_physical_address; + if (xsdt_addr & ~0xffffffffULL) + return; + + struct xsdt_descriptor_rev1 *xsdt = (void*)(u32)xsdt_addr; + void *end = (void*)xsdt + xsdt->length; + struct rsdt_descriptor_rev1 *rsdt; + int rsdt_size = offsetof(struct rsdt_descriptor_rev1, table_offset_entry[0]); + int i; + for (i=0; (void*)&xsdt->table_offset_entry[i] < end; i++) { + u64 tbl_addr = xsdt->table_offset_entry[i]; + if (!tbl_addr || (tbl_addr & ~0xffffffffULL)) + continue; + rsdt_size += 4; + } + + rsdt = malloc_high(rsdt_size); + RsdpAddr->rsdt_physical_address = (u32)rsdt; + + memcpy(rsdt, xsdt, sizeof(struct acpi_table_header)); + rsdt->signature = RSDT_SIGNATURE; + rsdt->length = rsdt_size; + rsdt->revision = 1; + int j; + for (i=j=0; (void*)&xsdt->table_offset_entry[i] < end; i++) { + u64 tbl_addr = xsdt->table_offset_entry[i]; + if (!tbl_addr || (tbl_addr & ~0xffffffffULL)) + continue; + rsdt->table_offset_entry[j++] = (u32)tbl_addr; + } + + rsdt->checksum -= checksum(rsdt, rsdt_size); +} + void qemu_platform_setup(void) { @@ -186,8 +227,10 @@ qemu_platform_setup(void) RsdpAddr = find_acpi_rsdp(); - if (RsdpAddr) + if (RsdpAddr) { + build_compatibility_rsdt(); return; + } /* If present, loader should have installed an RSDP. * Not installed? We might still be able to continue diff --git a/src/std/acpi.h b/src/std/acpi.h index c2ea707..caf5e7e 100644 --- a/src/std/acpi.h +++ b/src/std/acpi.h @@ -133,6 +133,17 @@ struct rsdt_descriptor_rev1 } PACKED; /* + * ACPI 2.0 Root System Description Table (RSDT) + */ +#define XSDT_SIGNATURE 0x54445358 // RSDT +struct xsdt_descriptor_rev1 +{ + ACPI_TABLE_HEADER_DEF /* ACPI common table header */ + u64 table_offset_entry[0]; /* Array of pointers to other */ + /* ACPI tables */ +} PACKED; + +/* * ACPI 1.0 Firmware ACPI Control Structure (FACS) */ #define FACS_SIGNATURE 0x53434146 // FACS -- 2.13.3 >From 46c7a296d27bd487cfd89a40973b1e61426dfbd0 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini(a)redhat.com> Date: Tue, 25 Jul 2017 18:59:20 +0200 Subject: [PATCH 2/2] seabios: create v1 FADT in compatibility RSDT This patch presents a v1 FADT inside the compatibility RSDT, so that old operating systems are not broken. Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com> --- src/fw/paravirt.c | 18 +++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c index 19a4abe..a9b9e80 100644 --- a/src/fw/paravirt.c +++ b/src/fw/paravirt.c @@ -148,6 +148,18 @@ static void msr_feature_control_setup(void) wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits); } +static void* +build_rev1_fadt(struct fadt_descriptor_rev1 *fadt_v3) +{ + struct fadt_descriptor_rev1 *fadt_v1 = malloc_high(sizeof *fadt_v1); + + memcpy(fadt_v1, fadt_v3, sizeof *fadt_v1); + fadt_v1->length = sizeof *fadt_v1; + fadt_v1->revision = 1; + fadt_v1->checksum -= checksum(fadt_v1, fadt_v1->length); + return fadt_v1; +} + static void build_compatibility_rsdt(void) { @@ -182,6 +194,12 @@ build_compatibility_rsdt(void) u64 tbl_addr = xsdt->table_offset_entry[i]; if (!tbl_addr || (tbl_addr & ~0xffffffffULL)) continue; + struct acpi_table_header *tbl = (void*)(u32)tbl_addr; + /* The RSDT should only have an ACPI 1.0 FADT according to + * the spec. + */ + if (tbl->signature == FACP_SIGNATURE && tbl->revision > 1) + tbl_addr = (u32)build_rev1_fadt((void *)tbl); rsdt->table_offset_entry[j++] = (u32)tbl_addr; } -- 2.13.3

5 10

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS