SeaBIOS July 2017

seabios@seabios.org

19 participants
46 discussions

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

26 Jul '17

On 25/07/2017 23:25, Phil Dennis-Jordan wrote: > Thanks for this, Paolo. Very interesting idea. > > I couldn't get things working initially, but with a few fixups on the > SeaBIOS side I can boot both legacy and modern OSes. See comments > inline below for details on changes required. > > Successfully booted (only a brief test): > - Windows 2000 > - Windows XP (32 bit) > - Windows 7 (32 bit) > - Windows 10 (64 bit, SeaBIOS) > - Windows 10 (64 bit, OVMF) > - macOS 10.12 (patched OVMF) Thanks Phil! You unwittingly tested the compatibility path on all these OSes, since my QEMU patch forgot to setup rsdp->length, rsdp->revision and the extended checksum. However, I've now tested Windows XP, Linux w/SeaBIOS, Linux w/patched SeaBIOS and Linux w/OVMF. I've now found out that edk2 contains similar logic. It uses a PCD (a compile-time flag essentially) to choose between ACPI >= 2.0 tables or ACPI 1.0-compatible tables. In the latter case, edk2 takes care of producing a v1 FADT if needed (similar to this patch) and linking the RSDT to it; otherwise it keeps whatever FADT was provided by platform code and produces an XSDT. So I'm promoting the SeaBIOS code from "hack" to "right thing to do". Though then I cannot brag about Kevin's nice words, too bad. :) Paolo > On Tue, Jul 25, 2017 at 7:10 PM, Paolo Bonzini <pbonzini(a)redhat.com> wrote: >> 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; > > Modifying the RSDP like this invalidates both its checksums, so they > need to be adjusted. Something like this: > > RsdpAddr->checksum -= checksum(RsdpAddr, offsetof(struct > rsdp_descriptor, length)); > RsdpAddr->extended_checksum -= checksum(RsdpAddr, sizeof(struct > rsdp_descriptor)); > >> + >> + 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) >> + */ > > Comment should be "ACPI 2.0 Extended System Description Table (XSDT)" > >> +#define XSDT_SIGNATURE 0x54445358 // RSDT > > The signature is "XSDT", that works out to 0x58445358 > >> +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; > > We should mask out the flags bits that are reserved in ACPI 1.0 as > they refer to fields not present in this revision's FADT. e.g. > > // upper 23 bits reserved in rev1 FADT > fadt_v1->flags &= 0x1ff; > > >> + 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 >> >> > > With these changes: > > Reviewed-by: Phil Dennis-Jordan <phil(a)philjordan.eu> >

2 2

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

26 Jul '17

On 25/07/2017 20:11, Alexander Bezzubikov wrote: > > вт, 25 июля 2017 г. в 19:10, Marcel Apfelbaum <marcel(a)redhat.com > <mailto:marcel@redhat.com>>: > > On 25/07/2017 17:09, Alexander Bezzubikov wrote: > > 2017-07-25 16:53 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com > <mailto:mst@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 > <mailto:mst@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. > > > Gerd's suggestion was to set min(cap_value, children_found). From such > point of view 0 can be a valid value. > I am lost now :) How can we use the capability to reserve "more" buses since children-found will be always the smaller value? I think you should use max(cap_value, children_found) to ensure you always reserve enough buses for existing children. In this case 0 is actually an invalid value since children_found > 0 for a bridge. Thanks, Marcel > > > 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 > <mailto:zuban32s@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 > > > > > > > > -- > Alexander Bezzubikov

2 1

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

25 Jul '17

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

2 1

Re: [SeaBIOS] [RFC PATCH v2 6/6] hw/pci: add hint capabilty for additional bus reservation to pcie-root-port
by Marcel Apfelbaum 25 Jul '17

25 Jul '17

On 25/07/2017 0:43, Alexander Bezzubikov wrote: > 2017-07-24 23:43 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: >> On Sun, Jul 23, 2017 at 01:15:43AM +0300, Aleksandr Bezzubikov wrote: >>> Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >>> --- >>> hw/pci-bridge/pcie_root_port.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c >>> index b0e49e1..ca92d85 100644 >>> --- a/hw/pci-bridge/pcie_root_port.c >>> +++ b/hw/pci-bridge/pcie_root_port.c >>> @@ -106,6 +106,11 @@ static void rp_realize(PCIDevice *d, Error **errp) >>> pcie_aer_root_init(d); >>> rp_aer_vector_update(d); >>> >>> + rc = pci_bridge_help_cap_init(d, 0, p->bus_reserve, 0, 0, 0, errp); >>> + if (rc < 0) { >>> + goto err; >>> + } >>> + >>> return; >>> >>> err: >> >> It looks like this will add the capability unconditionally to all >> pcie root ports. Two issues with it: >> 1. you can't add vendor properties to devices where vendor is >> not qemu as they might have their own concept of what it does. >> 2. this will break compatibility with old machine types, >> need to disable for these >> > > Actually the original idea was to add it for pcie-root-port excusively > (for now at least), looks like I've confused a little with files naming. Right, for the Generic PCIe Root Port and not for all the root ports. In the future we may want to add it to the PCI-brigde so we can have nested bridges, but we are not there yet. > Will add it for v3. > Thanks, Marcel >>> -- >>> 2.7.4 > > >

1 0

Re: [SeaBIOS] [RFC PATCH v2 4/4] pci: enable RedHat PCI bridges to reserve additional buses on PCI init
by Marcel Apfelbaum 24 Jul '17

24 Jul '17

On 24/07/2017 17:39, Alexander Bezzubikov wrote: > 2017-07-24 12:42 GMT+03:00 Gerd Hoffmann <kraxel(a)redhat.com > <mailto:kraxel@redhat.com>>: > > On Sun, 2017-07-23 at 22:44 +0300, Alexander Bezzubikov wrote: > > By the way, any ideas on how to avoid 'bus overstealing' would > > be greatly appreciated. > > Static BIOS variable isn't applicable since its value isn't saved > > across reboots. > > I think the reservation hints should be a absolute number, not a > increment. i.e. if qemu suggests to reserve three extra bus numbers > seabios should reserve three, no matter whenever there are zero, one, > two or three child busses present. And I guess seabios should > interpret that as minimum, so in case it finds five child busses it > will allocate five bus numbers of course ... > > > Personally I have nothing against it. Marcel, Michael, what do you think? Sounds good to me. > > > Same with the other limit hints. If the hint says to allocate 16M, and > existing device bars sum up to 4M, allocate 16M (and therefore leave > 12M address space for hotplug). If the device bars sum up to 32M, > allocate that. > > While being at it: I have my doubts the capability struct layout > (which mimics register layout) buys us that much, seabios wouldn't > blindly copy over the values anyway. Having regular u32 fields looks > more useful to me. > > > Again, if nobody has any objections, I can change it in v3. > I also had my reservations about prev layout, let's see if it will look cleaner. Thanks, Marcel > cheers, > Gerd > > > > > -- > Alexander Bezzubikov

1 0

Re: [SeaBIOS] [RFC PATCH v2 4/4] pci: enable RedHat PCI bridges to reserve additional buses on PCI init
by Gerd Hoffmann 24 Jul '17

24 Jul '17

On Sun, 2017-07-23 at 22:44 +0300, Alexander Bezzubikov wrote: > By the way, any ideas on how to avoid 'bus overstealing' would > be greatly appreciated. > Static BIOS variable isn't applicable since its value isn't saved > across reboots. I think the reservation hints should be a absolute number, not a increment. i.e. if qemu suggests to reserve three extra bus numbers seabios should reserve three, no matter whenever there are zero, one, two or three child busses present. And I guess seabios should interpret that as minimum, so in case it finds five child busses it will allocate five bus numbers of course ... Same with the other limit hints. If the hint says to allocate 16M, and existing device bars sum up to 4M, allocate 16M (and therefore leave 12M address space for hotplug). If the device bars sum up to 32M, allocate that. While being at it: I have my doubts the capability struct layout (which mimics register layout) buys us that much, seabios wouldn't blindly copy over the values anyway. Having regular u32 fields looks more useful to me. cheers, Gerd

2 1

[RFC PATCH 0/2] Allow RedHat PCI bridges reserve more buses than necessary during init
by Aleksandr Bezzubikov 24 Jul '17

24 Jul '17

Now PCI bridges (and PCIE root port too) get a bus range number in 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. The suggested workaround is to have vendor-specific capability in RedHat generic pcie-root-port that contains number of additional bus to reserve on BIOS PCI init. Aleksandr Bezzubikov (2): pci: add support for direct usage of bdf for capability lookup pci: enable RedHat pci bridges to reserve more buses src/fw/pciinit.c | 12 ++++++++++-- src/hw/pcidevice.c | 24 ++++++++++++++++++++++++ src/hw/pcidevice.h | 1 + 3 files changed, 35 insertions(+), 2 deletions(-) -- 2.7.4

6 17

Re: [SeaBIOS] >256 Virtio-net-pci hotplug Devices
by Marcel Apfelbaum 24 Jul '17

24 Jul '17

On 22/07/2017 2:57, Kinsella, Ray wrote: > > Hi Marcel > Hi Ray, > On 21/07/2017 01:33, Marcel Apfelbaum wrote: >> On 20/07/2017 3:44, Kinsella, Ray wrote: >> That's strange. Please ensure the virtio devices are working in >> virtio 1.0 mode (disable-modern=0,disable-legacy=1). >> Let us know any problems you see. >> >>> Not sure what yet, I will try scaling it with hotplugging tomorrow. >>> >> >> Updates? > > I have managed to scale it to 128 devices. > The kernel does complain about IO address space exhaustion. > > [ 83.697956] pci 0000:80:00.0: BAR 13: no space for [io size 0x1000] > [ 83.700958] pci 0000:80:00.0: BAR 13: failed to assign [io size 0x1000] > [ 83.701689] pci 0000:80:00.1: BAR 13: no space for [io size 0x1000] > [ 83.702378] pci 0000:80:00.1: BAR 13: failed to assign [io size 0x1000] > [ 83.703093] pci 0000:80:00.2: BAR 13: no space for [io size 0x1000] > > I was surprised that I am running out of IO address space, as I am > disabling legacy virtio. I assumed that this would remove the need for > SeaBIOS to allocate the PCI Express Root Port IO address space. Indeed, SeeBIOS does not reserve IO ports in this case, but Linux kernel still decides ""it knows better" and tries to allocate IO resources anyway. It does not affect the "modern" virtio-net devices because they don't need IO ports anyway. One way to work around the error message is to have the PCIe Root Port have the corresponding IO headers read-only since IO support is optional. I tried this some time ago, I'll get back to it. > In any > case - it doesn't stop the virtio-net device coming up and working as > expected. > Right. > [ 668.692081] virtio_net virtio103 enp141s0f4: renamed from eth101 > [ 668.707114] virtio_net virtio130 enp144s0f7: renamed from eth128 > [ 668.719795] virtio_net virtio129 enp144s0f6: renamed from eth127 > > I encountered some issues in vhost, due to open file exhaustion but > resolved these with 'ulimit' in the usual way - burned alot of time on > that today. > > When scaling up to 512 Virtio-net devices SeaBIOS appears to really slow > down when configuring PCI Config space - haven't manage to get this to > work yet. > Adding SeaBIOS mailing list and maintainers, maybe there is a known issue about 500+ PCI devices configuration. >> Not really. All you have to do is to add a property to the pxb-pci/pxb >> devices: pci_domain=x; then update the ACPI table to include the pxb >> domain. You also have to tweak a little the pxb-pcie/pxb devices >> to not share the bus numbers if pci_domain > 0. > > Thanks for information, will add to the list. > Is also on my todo list :) Thanks, Marcel > Ray K > \

3 2

Re: [SeaBIOS] [RFC PATCH v2 4/4] pci: enable RedHat PCI bridges to reserve additional buses on PCI init
by Alexander Bezzubikov 23 Jul '17

23 Jul '17

By the way, any ideas on how to avoid 'bus overstealing' would be greatly appreciated. Static BIOS variable isn't applicable since its value isn't saved across reboots. 2017-07-23 19:43 GMT+03:00 Alexander Bezzubikov <zuban32s(a)gmail.com>: > 2017-07-23 5:49 GMT+03:00 Michael S. Tsirkin <mst(a)redhat.com>: > >> On Sun, Jul 23, 2017 at 01:11:50AM +0300, Aleksandr Bezzubikov wrote: >> > In case of Red Hat PCI bridges reserve additional buses, which number >> is provided >> > in a vendor-specific capability. >> > >> > Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com> >> > --- >> > src/fw/pciinit.c | 14 ++++++++++++-- >> > 1 file changed, 12 insertions(+), 2 deletions(-) >> > >> > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c >> > index 864954f..f05a8b9 100644 >> > --- a/src/fw/pciinit.c >> > +++ b/src/fw/pciinit.c >> > @@ -15,6 +15,7 @@ >> > #include "hw/pcidevice.h" // pci_probe_devices >> > #include "hw/pci_ids.h" // PCI_VENDOR_ID_INTEL >> > #include "hw/pci_regs.h" // PCI_COMMAND >> > +#include "hw/pci_cap.h" // qemu_pci_cap >> > #include "list.h" // struct hlist_node >> > #include "malloc.h" // free >> > #include "output.h" // dprintf >> > @@ -578,9 +579,18 @@ pci_bios_init_bus_rec(int bus, u8 *pci_bus) >> > pci_bios_init_bus_rec(secbus, pci_bus); >> > >> > if (subbus != *pci_bus) { >> > + u8 res_bus = 0; >> > + if (pci_config_readw(bdf, PCI_VENDOR_ID) == >> PCI_VENDOR_ID_REDHAT) { >> >> Check device ID as well. > > Not sure what ID/IDs we want to see here exactly. Now it can be > only pcie-root-port (0xC then), but > > >> >> > + u8 cap = pci_find_capability(bdf, PCI_CAP_ID_VNDR, 0); >> >> There could be multiple vendor capabilities. You want to scan them all. >> >> >> > + if (cap) { >> > + res_bus = pci_config_readb(bdf, >> > + cap + offsetof(struct >> redhat_pci_bridge_cap, >> > + bus_res)); >> >> >> You might want to add sanity checks e.g. overflow, and capability >> length. >> >> Also, if all you use is offsetof, don't bother with a struct, just >> add some defines. >> > OK, will move this to defines. > > >> >> > + } >> > + } >> > dprintf(1, "PCI: subordinate bus = 0x%x -> 0x%x\n", >> > - subbus, *pci_bus); >> > - subbus = *pci_bus; >> > + subbus, *pci_bus + res_bus); >> > + subbus = *pci_bus + res_bus; >> >> So you take all present devices and add reserved ones - is that it? >> If so it looks like this will steal extra buses each time you >> add a child bus and reboot. >> > > You're right, such problem persists. Will think on what can be done here. > > >> >> >> > } else { >> > dprintf(1, "PCI: subordinate bus = 0x%x\n", subbus); >> > } >> > -- >> > 2.7.4 >> > > > > -- > Alexander Bezzubikov > -- Alexander Bezzubikov

1 0

Re: [SeaBIOS] [RFC PATCH v2 4/6] hw/pci: introduce bridge-only vendor-specific capability to provide some hints to firmware
by Marcel Apfelbaum 23 Jul '17

23 Jul '17

On 23/07/2017 19:19, Alexander Bezzubikov wrote: > 2017-07-23 18:57 GMT+03:00 Marcel Apfelbaum <marcel(a)redhat.com > <mailto:marcel@redhat.com>>: > > On 23/07/2017 1:15, 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 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. > > > Hi Alexandr, > > Signed-off-by: Aleksandr Bezzubikov <zuban32s(a)gmail.com > <mailto:zuban32s@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, > > > Can you please rename to something like > 'pci_bridge_qemu_cap_init' to be more specific? > > + uint8_t bus_reserve, uint32_t > io_limit, > + uint16_t mem_limit, uint64_t > pref_limit, > > > > I am not sure regarding "limit" suffix, this > is a reservation, not a limitation. > > > I'd like this fields names to match actual registers which > are going to get the values. > For this case I think is better to have io_res..., to describe the parameter rather than match the registers. > > > + Error **errp) > +{ > + size_t cap_len = sizeof(PCIBridgeQemuCap); > + PCIBridgeQemuCap cap; > + > + 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; > + > + 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); > + 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; > + uint16_t pref_lim; > + uint16_t mem_lim; > > > This 32bit IOMEM, right? > > + uint16_t io_lim_upper; > + uint8_t io_lim; > > > Why do we need io_lim and io_lim_upper? > > > The idea was to be able to directly move the capability fields values > to the registers when actually using it (in firmware) code. > Secondly, it saves a little space by avoding usage 32-bit types > when 24-bit ones are desired. And the same thing with prefetchable (48->64). > But if it's more convenient no to split this value, I can do that. With a clear explanation (Mimic of the <PCI config space field>) I personally don't mind keeping it like that. Thanks, Marcel > > > Thanks, > Marcel > > > + uint8_t padding; > +} PCIBridgeQemuCap; > + > +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 */ > > > > > > -- > Alexander Bezzubikov

1 0

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

SeaBIOS July 2017