From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web12.7021.1624539056814645385 for ; Thu, 24 Jun 2021 05:50:57 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dOLHgQqC; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624539056; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=v9N+GnKsWP2gJFAmEJt3rLej33zjkqyVCToUGfi28Uo=; b=dOLHgQqCfVnbT+rnpfeDHO9yjR3ulnXBqBAB5Uz7MLb7qw7Yd/eWvCT+RXFRrQc176mdVA Ev2a41Cz0gIv4yfUN4tDQgYTx2Ihw5dkKQyDCBmDTtW8R0FEqe7B2ONswpKPfUwSs0IrmW 2UMftzu3Wt01g+2x/kUGU6nX6O06lck= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-464-ygoHfZkFMuy0d-Ceu5GJGg-1; Thu, 24 Jun 2021 08:50:52 -0400 X-MC-Unique: ygoHfZkFMuy0d-Ceu5GJGg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 20EC11922969; Thu, 24 Jun 2021 12:50:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-106.ams2.redhat.com [10.36.114.106]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4589160C05; Thu, 24 Jun 2021 12:50:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 4/5] ArmVirtPkg: Enable ACPI support for Kvmtool To: devel@edk2.groups.io, pierre.gondois@arm.com, Sami Mujawar Cc: Ard Biesheuvel , Leif Lindholm , Akanksha Jain , Alexandru Elisei References: <20210623140640.16754-1-Pierre.Gondois@arm.com> <20210623140640.16754-5-Pierre.Gondois@arm.com> From: "Laszlo Ersek" Message-ID: <9ce28b8a-0a59-4915-79f7-54fcdbf1f430@redhat.com> Date: Thu, 24 Jun 2021 14:50:47 +0200 MIME-Version: 1.0 In-Reply-To: <20210623140640.16754-5-Pierre.Gondois@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/23/21 16:06, PierreGondois wrote: > From: Sami Mujawar > > 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 > Signed-off-by: Pierre Gondois > --- > 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 { 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 Thanks Laszlo