To enable detection of GICv3 Interrupt Translation Service capability
in the ACPI MADT,a new pcd setting has been created in edk2.This pcd
setting would be referenced by edk2-platform code to advertise the ITS
physical base address within GIC ITS structure of MADT.

Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org>
---
ArmPkg/ArmPkg.dec | 1 +
1 file changed, 1 insertion(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a8a22c649f..c22b7d0c42 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -266,6 +266,7 @@
# Base address for the GIC Redistributor region that contains the boot CPU
gArmTokenSpaceGuid.PcdGicRedistributorsBase|0|UINT64|0x0000000E
gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0|UINT64|0x0000000D
+ gArmTokenSpaceGuid.PcdGicItsBase|0|UINT64|0x0000000F
gArmTokenSpaceGuid.PcdGicSgiIntId|0|UINT32|0x00000025

#
--
2.27.0

(+cc Achin)

On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via groups.io <ilias.apalodimas=
linaro.org@groups.io> wrote:

Hi Pierre,

On Wed, Mar 10, 2021 at 09:58:19AM +0000, Pierre wrote:
> Hi Ilias,
> Just minor coding style nit picking:
>
> * Drivers/OpTee/OpteeRpmbPkg/FixupPcd.c:FixPcdMemory() I think the
> error codes are missing in the function header

Ah you mean the return values of locate protocol?

> * Thanks for adding the IN/OUT function parameter descriptors. Is it
> possible to also add them in the function headers [1] ?

Sure, I'll send a v7 anyways since I managed to mess up the maintainers
patch
somehow...

I hope I haven't missed any of your other requests.

>
> About the FFA/SVC call:
>
> >> If this is an FFA call, is it possible to:
> >> - put a reference in the header to the spec (it should be similar to
the
> >> one at
> >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/
ArmMmuStandaloneMmLib.c)
> >> - check the return status of the SVC call against the ones available
at
> >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> >> - if possible, remove the dependency to <IndustryStandard/ArmMmSvc.h>
> >
> > The call is technically an FFA one but at the moment OP-TEE returns the
> StMM
> > return code which is defined in the last header you mention.
> > The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function
> > tee2stmm_ret_val().
> > So unless we redefine that in OP-TEE or (better imho), wait for a full
FFA
> > mechanism to be in place, I'd prefer leaving it as is.
> > Keep in mind that adding the full FFA will also get rid of the
hardcoded
> IDs
> > on the beginning of the file.
>
> The description of a FFA_MSG_SEND_DIRECT_REQ (s10.2 of [2]) doesn't seem
to
> return the same error codes as the ones optee returns (in
> optee_os/core/arch/arm/kernel/stmm_sp.c:tee2stmm_ret_val()). I am not
sure a
> new FFA specification will change these error codes.
> Another thing is that I think the mMemMgrId variable described in
> edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c is currently
> defined as edk2/ArmPkg/Include/IndustryStandard/
ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
> (the name seems to be misleading).
> I think it would be better to:
>
> * align the optee error codes with what is in the FFA spec
> * handle these error codes in edk2 with what is in
> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h and remove the
> dependendy to edk2/ArmPkg/Include/IndustryStandard/ArmMmSvc.h if
> possible
> * rename
> edk2/ArmPkg/Include/IndustryStandard/
ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID
> define to a proper name, according to what is in
> optee_os/core/arch/arm/kernel/stmm_sp.c, and add one define for
> 'mem_mgr_id'
> * remove the mMemMgrId and mStorageId variables from
> edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c and use the
> newly created defines from
> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
>
> This would allow to be aligned with the current FFA spec and when a new
one
> comes, these endpoints IDs (mMemMgrId/mStorageId) can just be removed
from
> one location (as you said).
> In the end it seems you and Sami will maintain this, so I will let you
> decide what is best.

I get the whole point, and for the record you are technically right.

But there's something (once again) that's 'weird' here.
This StandAloneMM that's compiling over here is only used by OP-TEE.
In order to use that you need to call an SMC into OP-TEE (not FFA) from
the non-secure world to initiate it.
There's an OP-TEE PTA (pseudo-TA), that then converts the message to MM and
sends it over to StandAloneMM. There are no FFA manifests yet, that's why
the
get/set memory attributes code is still running, to set up page
permissions
as well.

The FFA mechanism you are seeing right now, is just the internal contract
between OP-TEE and this driver. We did some of the calls depend on FFA
since
this was a good way to start introducing FFA code into EDK2 (which will
eventually be needed), without being too intrusive.

In the long run OP-TEE will be fully converted into FFA the changes you are
talking about make sense. In fact there's a ./core/arch/arm/kernel/
secure_partition.c
in OP-TEE doing exactly that but it's not yet complete.
I tried to describe the entire situation here [1].

If anyone feels really strong about this, we can go and change it. The
changes
aren't too big to begin with. That being said I'd prefer keeping it as is,
since this will naturally evolve to a complete FFA spec, but the mechanisms
are still missing from OP-TEE. Last but not least when OP-TEE gets that's
FFA
support you won't bundle StandAloneMM with the driver right? You'd have 2
discrete Secure partitions, one dealing with variables and one dealing with
storage.

[1] https://apalos.github.io/
Protected%20UEFI%20variables%20with%20U-Boot.html#
Protected%20UEFI%20variables%20with%20U-Boot


Thanks
/Ilias
>
> [1] https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/
6_documenting_software/610_special_commands#
6-10-4-param-in-out-parameter-description
> [2] https://developer.arm.com/documentation/den0077/a
>
> Regards,
> Pierre

+=0D
+ PinFunction (Exclusive, PullDown, BCM_ALT3, "\\_SB.GDV0.GPI0", 0, Reso=
urceConsumer, , ) { 32, 33 }=0D
+=0D
+ // fake the CTS signal as we don't support HW flow control yet=0D
+ // BCM_ALT2 is set as output (low) by default=0D
+ PinFunction (Exclusive, PullNone, BCM_ALT2, "\\_SB.GDV0.GPI0", 0, Reso=
urceConsumer, , ) { 31 }=0D
})=0D

This change is to fix the compiler error on GCC49 release build.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Signed-off-by: Liming Gao <gaoliming@byosoft.com.cn>
---
MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c | 1 +
.../Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c | 1 +
2 files changed, 2 insertions(+)

diff --git a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
index e99a812a44..0779f94f9e 100644
--- a/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
+++ b/MdeModulePkg/Bus/Ata/AtaAtapiPassThru/AhciMode.c
@@ -1122,6 +1122,7 @@ AhciDmaTransfer (

Map = NULL;
PciIo = Instance->PciIo;
+ Status = EFI_SUCCESS;

if (PciIo == NULL) {
return EFI_INVALID_PARAMETER;
diff --git a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
index 0c9299c8b0..7822cbf4bb 100644
--- a/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
+++ b/MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.c
@@ -117,6 +117,7 @@ CreateBasicVariablePolicy (

// Now we've gotta determine the total size of the buffer required for
// the VariablePolicy structure.
+ NameSize = 0;
TotalSize = sizeof( VARIABLE_POLICY_ENTRY );
if (Name != NULL) {
NameSize = StrnSizeS( Name, MAX_UINT16 );

On 03/10/21 19:56, Laszlo Ersek wrote:

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249

Generalize the current OVMF SEV subsystem entry, so that we can use
it for Intel TDX in the future, ensuring proper patch circulation
for reviews.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Thanks
Laszlo

Laszlo Ersek (2):
Maintainers: refresh the OVMF SEV subsystem after TianoCore #2198
and #3077
Maintainers: rename the OVMF SEV subsystem to "Confidential
Computing"

Maintainers.txt | 28 +++++++++++++-------
1 file changed, 18 insertions(+), 10 deletions(-)


base-commit: edd46cd407ea4a0adaa8d6ca86f550c2a4d5c507

For merging this series, I still need ACKs from James and Min Xu,
please.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249

Generalize the current OVMF SEV subsystem entry, so that we can use it
for Intel TDX in the future, ensuring proper patch circulation for
reviews.

Cc: Andrew Fish <afish@apple.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Thanks
Laszlo

Laszlo Ersek (2):
Maintainers: refresh the OVMF SEV subsystem after TianoCore #2198 and
#3077
Maintainers: rename the OVMF SEV subsystem to "Confidential Computing"

Maintainers.txt | 28 +++++++++++++-------
1 file changed, 18 insertions(+), 10 deletions(-)


base-commit: edd46cd407ea4a0adaa8d6ca86f550c2a4d5c507

On 03/11/21 10:48, Jon Nettleton wrote:

On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io
<jon=solid-run.com@groups.io> wrote:

On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote:

On 03/10/21 09:04, Jon Nettleton wrote:

I am debugging a failure that I am seeing while using the HoneyComb's
spi-nor flash for runtime variable storage. I am hoping someone on
the list can give me some insight as what may be the problem.

The problem showed up when we switched the MMIO region for the fspi
flash device to be marked as non executable. reading variables is
fine, however writes began throwing an error.

[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

I have patched the kernel and removed the X86 requirement and enabled
the sysfs runtime mappings kernel config so I can get an easy view of
the mappings the kernel carries for runtime services. I then track
that virtual address to the MMIO region of nor flash, which makes
sense that region is marked as non executable. The question is why is
code being executed from this address range

attribute
::::::::::::::
0x8000000000000001
::::::::::::::
num_pages
::::::::::::::
0x40
::::::::::::::
phys_addr
::::::::::::::
0x20500000
::::::::::::::
type
::::::::::::::
0xb
::::::::::::::
virt_addr
::::::::::::::
0x20680000

So then I patched the PL011 serial driver to be able to log to the
console in runtime and I track down the access to Status =
Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore().
What I don't understand is why EfiConvertPointer is mapping that
pointer into the Virtual address space occupied by the runtime mmio of
the flash. The pointer is being properly remapped. Here are the
pointer addresses in EFI and Kernel Runtime

EFI:
UpdateVariableStore:156 ECE33968
FvbGetPhysicalAddress(BaseAddress=0x20000000)

KERNEL:
UpdateVariableStore:156 206A3968
[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

Any insight that anyone could provide would be much appreciated.

Your platform appears misconfigured -- the flash MMIO range appears to
overlap runtime services code even before SetVirtualAddressMap. The
virtual address conflict is likely the result of the original physical
address conflict.

There is no physical address conflict. Running in physical mode the pointer
for GetPhysicalAddress is at 0xECE33968 and the MMIO physical
region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above.
Obviously I don't use that full MMIO region because our flash is not
256MB.

After the virtual address updates, the EfiMemoryMappedIO (0xb) type
range is mapped at virtual address range [0x20680000, 0x206C0000). The
GetPhysicalAddress function seems to be located at offset 0x23968 in
that range (with 0x1C698 bytes to go in the range). That's inexplicable.

Yes, exactly why I am reaching out.

What is the physical base address of the flash? What are the PCDs used
in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"?

We don't use the ArmPlatformPkg

The code is available here.
https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe

Thanks

Found the root of my problem and fixed it. As used from other devices
they are relocating the individual pointer for each function in FvbProtocol
of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c,
as well as lots of vendor drivers. I was able to dump the addresses being used
in RuntimeDriverConvertPointer and could locate the pointer to
GetPhysicalAddress.

Convert 0xEC8C1648:0xEC8B0000:0x203D0000
New virtual address is then 203E1648 which is fine.

However then later down in the remapping I found
Convert 0x203E1648:0x20000000:0x20800000

And this is where the pointer gets remapped again and into the
MMIO space of the nor flash. If I remove the calls to
ConvertPointer for the FvbProtocol I am still seeing those addresses
getting remapped but only once and runtime works as expected.

I am seeing that in MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
&mVariableModuleGlobal->FvbInstance->* are all being converted. It is
possible this is a long standing bug and it just so happens that our
configuration
has caused a conflict and exposed it.

Yes, this is curious, I noticed it too yesterday, trying to see where
the FVB protocol member function pointers were converted. I found that
OVMF's flash driver (OvmfPkg/QemuFlashFvbServicesRuntimeDxe) didn't do
it, but MdeModulePkg/Universal/Variable/RuntimeDxe did. That was
certainly strange, as the variable driver is a consumer of the protocol
(not the producer thereof), so I'd say it has no business poking new
values into the protocol interface structure.

So the issue (IIUC) on your platform is that the function pointers are
doubly-converted, once by your flash driver (which is arguably the right
thing), and another time by the variable driver.

I recommend filing a bug about this, at
<https://bugzilla.tianocore.org/>, against "VariableDxe.c".

I suggest CC'ing the "UEFI Variable modules" reviewers on the BZ as
well, from "Maintainers.txt" -- FWIW, I'm CC'ing Hao and Liming on this
email, now.

... It feels as if the Platform Init specification should speak to the
FVB[2] protocol's runtime aspects... But (as of v1.7), I don't see
anything runtime-related in that section. In fact I've checked the whole
PI v1.7 spec for "runtime", and the closest matches are under
EFI_VARIABLE_WRITE_ARCH_PROTOCOL. FVB2 is not spelled out as a
(potential) runtime dependency of EFI_VARIABLE_WRITE_ARCH_PROTOCOL. The
only explicitly runtime protocol (unrelated to runtime services) is
"Status Code Runtime Protocol".

So what we're seeing here is an edk2-ism, I believe.

I tried seeing if one of the "tour beyond BIOS" edk2 whitepapers said
anything on FVB -- but the only reference I found (in "Implementing
UEFI Authenticated Variables in SMM with EDKII", version 2) mentions
only *SMM* FVB.

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-white-papers

I think we should have a TianoCore BZ about this, even if we don' change
the code as a result -- which driver is responsible for updating the
runtime FVB protocol member functions should be spelled out somewhere
"searchable".

... Strangely, the other flash (FVB) driver in edk2,
ArmPlatformPkg/Drivers/NorFlashDxe, *does* perform the conversion
itself! See NorFlashVirtualNotifyEvent().

I don't understand that. Is it possible that, with
"ArmPlatformPkg/Drivers/NorFlashDxe" too, the conversion happens
*twice*, but (at least) one of those mappings is "identity"?

Thanks
Laszlo

On Thu, Mar 11, 2021 at 7:54 AM Jon Nettleton via groups.io
<jon=solid-run.com@groups.io> wrote:

On Wed, Mar 10, 2021 at 3:52 PM Laszlo Ersek <lersek@redhat.com> wrote:

On 03/10/21 09:04, Jon Nettleton wrote:

I am debugging a failure that I am seeing while using the HoneyComb's
spi-nor flash for runtime variable storage. I am hoping someone on
the list can give me some insight as what may be the problem.

The problem showed up when we switched the MMIO region for the fspi
flash device to be marked as non executable. reading variables is
fine, however writes began throwing an error.

[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

I have patched the kernel and removed the X86 requirement and enabled
the sysfs runtime mappings kernel config so I can get an easy view of
the mappings the kernel carries for runtime services. I then track
that virtual address to the MMIO region of nor flash, which makes
sense that region is marked as non executable. The question is why is
code being executed from this address range

attribute
::::::::::::::
0x8000000000000001
::::::::::::::
num_pages
::::::::::::::
0x40
::::::::::::::
phys_addr
::::::::::::::
0x20500000
::::::::::::::
type
::::::::::::::
0xb
::::::::::::::
virt_addr
::::::::::::::
0x20680000

So then I patched the PL011 serial driver to be able to log to the
console in runtime and I track down the access to Status =
Fvb->GetPhysicalAddress(Fvb, &FvVolHdr); in UpdateVariableStore().
What I don't understand is why EfiConvertPointer is mapping that
pointer into the Virtual address space occupied by the runtime mmio of
the flash. The pointer is being properly remapped. Here are the
pointer addresses in EFI and Kernel Runtime

EFI:
UpdateVariableStore:156 ECE33968
FvbGetPhysicalAddress(BaseAddress=0x20000000)

KERNEL:
UpdateVariableStore:156 206A3968
[ 556.709828] Unable to handle kernel execute from non-executable
memory at virtual address 00000000206a3968

Any insight that anyone could provide would be much appreciated.

Your platform appears misconfigured -- the flash MMIO range appears to
overlap runtime services code even before SetVirtualAddressMap. The
virtual address conflict is likely the result of the original physical
address conflict.

There is no physical address conflict. Running in physical mode the pointer
for GetPhysicalAddress is at 0xECE33968 and the MMIO physical
region is 0x20000000 - 0x2FFFFFFF. Shown as the BaseAddress above.
Obviously I don't use that full MMIO region because our flash is not
256MB.

After the virtual address updates, the EfiMemoryMappedIO (0xb) type
range is mapped at virtual address range [0x20680000, 0x206C0000). The
GetPhysicalAddress function seems to be located at offset 0x23968 in
that range (with 0x1C698 bytes to go in the range). That's inexplicable.

Yes, exactly why I am reaching out.

What is the physical base address of the flash? What are the PCDs used
in "ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c"?

We don't use the ArmPlatformPkg

The code is available here.
https://github.com/SolidRun/edk2-platforms/tree/LX2160_UEFI_ACPI_EAR3-lx2160cex7/Silicon/NXP/Drivers/SpiNorFlashDxe

Thanks

Found the root of my problem and fixed it. As used from other devices
they are relocating the individual pointer for each function in FvbProtocol
of the DXE. This is done in ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c,
as well as lots of vendor drivers. I was able to dump the addresses being used
in RuntimeDriverConvertPointer and could locate the pointer to
GetPhysicalAddress.

Convert 0xEC8C1648:0xEC8B0000:0x203D0000
New virtual address is then 203E1648 which is fine.

However then later down in the remapping I found
Convert 0x203E1648:0x20000000:0x20800000

And this is where the pointer gets remapped again and into the
MMIO space of the nor flash. If I remove the calls to
ConvertPointer for the FvbProtocol I am still seeing those addresses
getting remapped but only once and runtime works as expected.

I am seeing that in MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
&mVariableModuleGlobal->FvbInstance->* are all being converted. It is
possible this is a long standing bug and it just so happens that our
configuration
has caused a conflict and exposed it.