[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMC
From: |
Isaku Yamahata |
Subject: |
Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region |
Date: |
Fri, 12 Feb 2021 12:51:57 -0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Fri, Feb 12, 2021 at 04:40:38PM +0100,
Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 10 Feb 2021 22:46:43 -0800
> Isaku Yamahata <isaku.yamahata@intel.com> wrote:
>
> > Declare PNP0C01 device to reserve MMCONFIG region to conform to the
> > spec better and play nice with guest BIOSes/OSes.
> >
> > According to PCI Firmware Specification[0], MMCONFIG region must be
> > reserved by declaring a motherboard resource. It's optional to reserve
> > the region in memory map by Int 15 E820h or EFIGetMemoryMap.
> > Guest Linux checks if the MMCFG region is reserved by bios memory map
> > or ACPI resource. If it's not reserved, Linux falls back to legacy PCI
> > configuration access.
> >
> > TDVF [1] [2] doesn't reserve MMCONFIG the region in memory map.
> > On the other hand OVMF reserves it in memory map without declaring a
> > motherboard resource. With memory map reservation, linux guest uses
> > MMCONFIG region. However it doesn't comply to PCI Firmware
> > specification.
> >
> > [0] PCI Firmware specification Revision 3.2
> > 4.1.2 MCFG Table Description table 4-2 NOTE 2
> > If the operating system does not natively comprehend reserving the
> > MMCFG region, The MMCFG region must e reserved by firmware. ...
> > For most systems, the mortheroard resource would appear at the root
> > of the ACPI namespace (under \_SB)...
> > The resource can optionally be returned in Int15 E820h or
> > EFIGetMemoryMap as reserved memory but must always be reported
> > through ACPI as a motherboard resource
> >
> > [1] TDX: Intel Trust Domain Extension
> >
> > https://software.intel.com/content/www/us/en/develop/articles/intel-trust-domain-extensions.html
> > [2] TDX Virtual Firmware
> > https://github.com/tianocore/edk2-staging/tree/TDVF
> >
> > The change to DSDT is as follows.
> > @@ -68,32 +68,51 @@
> >
> > If ((CDW3 != Local0))
> > {
> > CDW1 |= 0x10
> > }
> >
> > CDW3 = Local0
> > }
> > Else
> > {
> > CDW1 |= 0x04
> > }
> >
> > Return (Arg3)
> > }
> > }
> > +
> > + Device (DRAC)
> > + {
> > + Name (_HID, "PNP0C01" /* System Board */) // _HID: Hardware ID
> > + Name (RBUF, ResourceTemplate ()
> > + {
> > + QWordMemory (ResourceProducer, PosDecode, MinFixed,
> > MaxFixed, NonCacheable, ReadWrite,
> > + 0x0000000000000000, // Granularity
> > + 0x00000000B0000000, // Range Minimum
> > + 0x00000000BFFFFFFF, // Range Maximum
> > + 0x0000000000000000, // Translation Offset
> > + 0x0000000010000000, // Length
> > + ,, , AddressRangeMemory, TypeStatic)
> > + })
> > + Method (_CRS, 0, Serialized) // _CRS: Current Resource
> > Settings
> > + {
> > + Return (RBUF) /* \_SB_.DRAC.RBUF */
> > + }
> > + }
> > }
> >
> > Scope (_SB)
> > {
> > Device (HPET)
> > {
> > Name (_HID, EisaId ("PNP0103") /* HPET System Timer */) //
> > _HID: Hardware ID
> > Name (_UID, Zero) // _UID: Unique ID
> > OperationRegion (HPTM, SystemMemory, 0xFED00000, 0x0400)
> > Field (HPTM, DWordAcc, Lock, Preserve)
> > {
> > VEND, 32,
> > PRD, 32
> > }
> >
> > Method (_STA, 0, NotSerialized) // _STA: Status
> >
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> > hw/i386/acpi-build.c | 46 +++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 00cc119362..e369908b1a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1072,6 +1072,46 @@ static void build_q35_pci0_int(Aml *table)
> > aml_append(table, sb_scope);
> > }
> >
> > +static Aml *build_q35_dram_controller(void)
> > +{
> > + AcpiMcfgInfo mcfg;
> build_dsdt() already calls acpi_get_mcfg(),
> I suggest to cache it there and pass to build_q35_dram_controller
> as an argument.
Sure.
> > + Aml *dev;
> > + Aml *rbuf;
> > + Aml *resource_template;
> > + Aml *rbuf_name;
> > + Aml *crs;
> > +
> > + if (!acpi_get_mcfg(&mcfg)) {
> > + return NULL;
> > + }
> > +
> > + /* DRAM controller */
> > + dev = aml_device("DRAC");
> > + aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C01")));
> > +
> > + resource_template = aml_resource_template();
> > + aml_append(resource_template,
> > + aml_qword_memory(AML_POS_DECODE,
> > + AML_MIN_FIXED,
> > + AML_MAX_FIXED,
> > + AML_NON_CACHEABLE,
> > + AML_READ_WRITE,
> > + 0x0000000000000000,
> > + mcfg.base,
> > + mcfg.base + mcfg.size - 1,
> s/mcfg.base + mcfg.size - 1/mcfg.base/
AddressMaximum needs to be the highest address of the region.
Not base address. By disassemble/assembl it, iasl complains as follows.
Also there are similar examples in acpi-build.c.
iasl drm-1.dsl
Intel ACPI Component Architecture
ASL+ Optimizing Compiler/Disassembler version 20190509
Copyright (c) 2000 - 2019 Intel Corporation
drm-1.dsl 23: 0x10000000, // Length = 256M
Error 6049 - ^ Length is larger than Min/Max
window
ASL Input: drm-1.dsl - 1000 bytes 6 keywords 37 source lines
Compilation failed. 1 Errors, 0 Warnings, 0 Remarks
No AML files were generated due to compiler error(s)
> > + 0x0000000000000000,
> > + mcfg.size));
> > + rbuf = aml_name_decl("RBUF", resource_template);
> > + aml_append(dev, rbuf);
> > +
> > + crs = aml_method("_CRS", 0, AML_SERIALIZED);
> > + rbuf_name = aml_name("RBUF");
> > + aml_append(crs, aml_return(rbuf_name));
> > + aml_append(dev, crs);
> > +
> > + return dev;
> > +}
> > +
> > static void build_q35_isa_bridge(Aml *table)
> > {
> > Aml *dev;
> > @@ -1212,7 +1252,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > Range *pci_hole, Range *pci_hole64, MachineState *machine)
> > {
> > CrsRangeEntry *entry;
> > - Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
> > + Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs, *drac;
> maybe limit drac to
> if (misc->is_piix4) {
> ... } else {
> Aml *drac
> scope
ok.
> > CrsRangeSet crs_range_set;
> > PCMachineState *pcms = PC_MACHINE(machine);
> > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > @@ -1256,6 +1296,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > aml_append(dev, build_q35_osc_method());
> > aml_append(sb_scope, dev);
> > + drac = build_q35_dram_controller();
> > + if (drac) {
> > + aml_append(sb_scope, drac);
> > + }
> mmcfg isn't optional for q35, is it?
> I'd simplify to:
> aml_append(sb_scope, build_q35_dram_controller(mmcfg_info));
> or put all of it under condition if it's optional
> if(foo) aml_append(sb_scope, build_q35_dram_controller(mmcfg_info))
It's optional. If MCFG isn't setup after reset, PCIE_BASE_ADDR_UNMAPPED
is returned.
--
Isaku Yamahata <isaku.yamahata@gmail.com>
- Re: [PATCH v3 03/10] i386: add properoty, x-smm-compat-5, to keep compatibility of SMM, (continued)
- [PATCH v3 04/10] acpi/core: always set SCI_EN when SMM isn't supported, Isaku Yamahata, 2021/02/11
- [PATCH v3 05/10] acpi: set fadt.smi_cmd to zero when SMM is not supported, Isaku Yamahata, 2021/02/11
- [PATCH v3 06/10] acpi: add test case for smm unsupported -machine smm=off, Isaku Yamahata, 2021/02/11
- [PATCH v3 09/10] acpi: add test case for -no-hpet, Isaku Yamahata, 2021/02/11
- [PATCH v3 08/10] i386: acpi: Don't build HPET ACPI entry if HPET is disabled, Isaku Yamahata, 2021/02/11
- [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Isaku Yamahata, 2021/02/11
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Igor Mammedov, 2021/02/12
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region,
Isaku Yamahata <=
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Igor Mammedov, 2021/02/15
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Isaku Yamahata, 2021/02/16
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Michael S. Tsirkin, 2021/02/16
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Isaku Yamahata, 2021/02/16
- Re: [PATCH v3 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region, Igor Mammedov, 2021/02/16
[PATCH v3 10/10] qtest/acpi/bios-tables-test: update acpi tables, Isaku Yamahata, 2021/02/11