Re: [edk2-devel] [PATCH v1 4/5] ArmVirtPkg: Enable ACPI support for Kvmtool

["Laszlo Ersek" <lersek@redhat.com>]

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> A Configuration Manager that uses the Dynamic Tables framework
> to generate ACPI tables for Kvmtool Guests has been provided.
> This Configuration Manager uses the FdtHwInfoParser module to
> parse the Kvmtool Device Tree and generate the required
> Configuration Manager objects for generating the ACPI tables.
> 
> Therefore, enable ACPI table generation for Kvmtool.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirtKvmTool.dsc | 15 +++++++++++++--
>  ArmVirtPkg/ArmVirtKvmTool.fdf | 11 +++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index 920880796ac2..b02324312f18 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -28,6 +28,7 @@ [Defines]
>    FLASH_DEFINITION               = ArmVirtPkg/ArmVirtKvmTool.fdf
>  
>  !include ArmVirtPkg/ArmVirt.dsc.inc
> +!include DynamicTablesPkg/DynamicTables.dsc.inc

(1) This doesn't seem right. In fact, ARM (not AARCH64) support claimed in "DynamicTablesPkg/DynamicTablesPkg.dsc" seems bogus in the first place; to my understanding, ACPI is not defined for 32-bit ARM.

More precisely, this !include directive is OK, but "DynamicTablesPkg/DynamicTables.dsc.inc" file should not provide a

  [Components.common]

section, but a

  [Components.AARCH64]

section. Refer to "ArmVirtPkg/ArmVirt.dsc.inc" please:

[Components.AARCH64]
  #
  # ACPI Support
  #
  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
    <LibraryClasses>
      NULL|EmbeddedPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf
  }


>  
>  !include MdePkg/MdeLibs.dsc.inc
>  
> @@ -144,6 +145,11 @@ [PcdsFixedAtBuild.common]
>    #
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
> +  #
> +  # ACPI Table Version
> +  #
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
> +
>  [PcdsPatchableInModule.common]
>    #
>    # This will be overridden in the code

(2) This hunk is superfluous. Please refer to "MdeModulePkg/MdeModulePkg.dec":

[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20|UINT32|0x0001004c

If you simply don't list the PCD in your DSC file, you'll get the default value, and the most restrictive access method declared for the PCD in the DEC file (here: fixed-at-build).


> @@ -198,8 +204,8 @@ [PcdsDynamicDefault.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
>  
> -  ## Force DTB
> -  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|TRUE
> +  ## Set default option to ACPI
> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|FALSE
>  
>    # Setup Flash storage variables
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0

(3) Same story as (2), please refer to "ArmVirtPkg/ArmVirtPkg.dec":

[PcdsDynamic]
  #
  # Whether to force disable ACPI, regardless of the fw_cfg settings
  # exposed by QEMU
  #
  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|0x0|BOOLEAN|0x00000003


> @@ -356,3 +362,8 @@ [Components.common]
>    }
>    OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>    OvmfPkg/Virtio10Dxe/Virtio10.inf
> +  #
> +  # ACPI Support
> +  #
> +  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf

(4) Superfluous by virtue of including "ArmVirtPkg/ArmVirt.dsc.inc" already.


> +  ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

(5) This should be in a [Components.AARCH64] section, not in [Components.common].


> diff --git a/ArmVirtPkg/ArmVirtKvmTool.fdf b/ArmVirtPkg/ArmVirtKvmTool.fdf
> index 076155199905..5ba4c579f050 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.fdf
> +++ b/ArmVirtPkg/ArmVirtKvmTool.fdf
> @@ -204,6 +204,17 @@ [FV.FvMain]
>    INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>    INF OvmfPkg/Virtio10Dxe/Virtio10.inf
>  
> +  #
> +  # ACPI Support
> +  #
> +  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +  #
> +  # Dynamic Table fdf
> +  #
> +  !include DynamicTablesPkg/DynamicTables.fdf.inc
> +
> +  INF ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> +
>    #
>    # TianoCore logo (splash screen)
>    #
> 

(6) Please do what "ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc" does:

!if $(ARCH) == AARCH64
  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
  ...
!endif


Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo