Re: [PATCH v4 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region

2021-02-16 Thread Igor Mammedov
On Mon, 15 Feb 2021 17:04:12 -0800
isaku.yamah...@gmail.com wrote:

> From: Isaku Yamahata 
> 
> 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 ()
> +{
> +DWordMemory (ResourceProducer, PosDecode, MinFixed, 
> MaxFixed, NonCacheable, ReadWrite,
> +0x, // Granularity
> +0xB000, // Range Minimum
> +0xB000, // Range Maximum
> +0x, // Translation Offset
> +0x1000, // 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, 0xFED0, 0x0400)
>  Field (HPTM, DWordAcc, Lock, Preserve)
>  {
>  VEND,   32,
>  PRD,32
>  }
> 
>  Method (_STA, 0, NotSerialized)  // _STA: Status
> 
> Signed-off-by: Isaku Yamahata 
> ---
>  hw/i386/acpi-build.c | 55 +++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e3386ae674..30326f69b3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1072,6 +1072,55 @@ static void build_q35_pci0_int(Aml *table)
>  aml_append(table, sb_scope);
>  }
>  
> +static Aml *build_q35_dram_controller(AcpiMcfgInfo *mcfg)
> +{
> +Aml *dev;
> +Aml *rbuf;
> +Aml *resource_template;
> +Aml *rbuf_name;
> +Aml *crs;
> +
> +/* DRAM controller */
> +dev = aml_device("DRAC");
> +aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C01")));
> +
> +resource_template = aml_resource_template();
> +if (mcfg->base + mcfg->size - 1 >= (1ULL << 32)) {
> +aml_append(resource_template,
> +   aml_qword_memory(AML_POS_DECODE,
> +AML_MIN_FIXED,
> +AML_MAX_FIXED,
> +AML_NON_CACHEABLE,
> +AML_READ_WRITE,
> +0x,
> +mcfg->base,
> +mcfg->base,
here   ^^^

> +aml_append(resource_template,
> +   aml_dword_memory(AML_POS_DECODE,
> +AML_MIN_FIXED,
> +   

[PATCH v4 07/10] hw/i386: declare ACPI mother board resource for MMCONFIG region

2021-02-15 Thread isaku . yamahata
From: Isaku Yamahata 

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 ()
+{
+DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, 
NonCacheable, ReadWrite,
+0x, // Granularity
+0xB000, // Range Minimum
+0xB000, // Range Maximum
+0x, // Translation Offset
+0x1000, // 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, 0xFED0, 0x0400)
 Field (HPTM, DWordAcc, Lock, Preserve)
 {
 VEND,   32,
 PRD,32
 }

 Method (_STA, 0, NotSerialized)  // _STA: Status

Signed-off-by: Isaku Yamahata 
---
 hw/i386/acpi-build.c | 55 +++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e3386ae674..30326f69b3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1072,6 +1072,55 @@ static void build_q35_pci0_int(Aml *table)
 aml_append(table, sb_scope);
 }
 
+static Aml *build_q35_dram_controller(AcpiMcfgInfo *mcfg)
+{
+Aml *dev;
+Aml *rbuf;
+Aml *resource_template;
+Aml *rbuf_name;
+Aml *crs;
+
+/* DRAM controller */
+dev = aml_device("DRAC");
+aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C01")));
+
+resource_template = aml_resource_template();
+if (mcfg->base + mcfg->size - 1 >= (1ULL << 32)) {
+aml_append(resource_template,
+   aml_qword_memory(AML_POS_DECODE,
+AML_MIN_FIXED,
+AML_MAX_FIXED,
+AML_NON_CACHEABLE,
+AML_READ_WRITE,
+0x,
+mcfg->base,
+mcfg->base,
+0x,
+mcfg->size));
+} else {
+aml_append(resource_template,
+   aml_dword_memory(AML_POS_DECODE,
+AML_MIN_FIXED,
+AML_MAX_FIXED,
+AML_NON_CACHEABLE,
+AML_READ_WRITE,
+0x,
+mcfg->base,
+