Adding Ard and Leif, comments below:

On 03/11/21 15:50, Laszlo Ersek wrote:
> On 03/11/21 10:48, Jon Nettleton wrote:

[...]

>> 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.

[...]

> ... 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"?

Confirmed.

I had to write some elaborate debug patches for determining this,
because in ArmVirtQemu, I cannot produce DEBUG output from the
SetVirtualAddressMap() notification functions. So here's the approach I
took:

(1) Introduce a new GUID-ed HOB structure in MdeModulePkg. The structure
itself lives in reserved memory, but its address is exposed in a GUID-ed
HOB. The structure is named FVB_ADDRESS_LIST, and it has the following
fields:

  - signature ("FVBADRLS" -- FVB Address List)
  - 16 entries of:
    - owner signature [what driver set this entry]
    - address
  - number of entries used (aka next entry to fill)

(2) In PlatformPei, allocate and initialize this structure (in reserved
memory), and expose its address via the GUID-ed HOB. Furthermore,
produce a log message with the allocation address.

(3) In NorFlashDxe, look up the structure via the GUID-ed HOB, in the
entry point function; remember the address in a global variable. In the
SetVirtualAddressMap() handler function, treat the conversion of the
"GetPhysicalAddress" FVB member function specially: via the global
variable pointer to FVB_ADDRESS_LIST in reserved memory, save both the
physical (original) and the virtual (converted) address of the
"GetPhysicalAddress" FVB member function, in new entries. As owner
signature in both entries, use "NORFLASH".

(4) In the runtime DXE variable driver, do the exact same thing, just
use a different "owner signature" -- "VARIABLE".

(5) Once the guest is up and running, run "efibootmgr --delete-timeout"
at a root prompt in the guest, deleting the existent "Timeout" UEFI
non-volatile variable, for verifying that the runtime variable (write)
service is functional.

(6) Using the log message from point (2):

> PlatformPeim: FvbAddressList @ 13FEC9000

hexdump the guest memory containing the FVB_ADDRESS_LIST, as follows:

> $ virsh qemu-monitor-command aavmf.rhel7.registered --hmp xp  /268cb 0x13FEC9000

Ccomments to the right of the hexdump:

> 000000013fec9000: 'F' 'V' 'B' 'A' 'D' 'R' 'L' 'S'                         <- structure signature: FVBADRLS
> 000000013fec9008: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[0], signature: NORFLASH
> 000000013fec9010: 'T' ' ' '\xc6' ';' '\x01' '\x00' '\x00' '\x00'          <- entry[0], GetPhysicalAddress *physical*: 0x000000013bc62054
> 000000013fec9018: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[1], signature: NORFLASH
> 000000013fec9020: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[1], GetPhysicalAddress *virtual*: 0x00000000244e2054
> 000000013fec9028: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[2], signature: VARIABLE
> 000000013fec9030: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[2], GetPhysicalAddress *physical*: 0x00000000244e2054
> 000000013fec9038: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[3], signature: VARIABLE
> 000000013fec9040: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[3], GetPhysicalAddress *virtual*: 0x00000000244e2054
> 000000013fec9048: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9050: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9058: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9060: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9068: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9070: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9078: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9080: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9088: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9090: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9098: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90a0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90a8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90b0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90b8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90c0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90c8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90d0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90d8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90e0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90e8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90f0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec90f8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9100: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> 000000013fec9108: '\x04' '\x00' '\x00' '\x00'                             <- number of entries used: 4

This shows the following:

- both NorFlashDxe and the runtime DXE variable driver converted the
  FVB.GetPhysicalAddress member function,

- the NorFlashDxe driver acted first, the runtime DXE variable driver
  acted second,

- when the runtime DXE variable driver "converted" the "physical"
  address to virtual address, there was no change (and no crash!),
  because the virtual address map passed in by the Linux kernel
  apparently identity maps this area -- just as I guessed.

So we definitely have a bug (only Linux's page tables save us from the
crash); now the question is:

Which driver is wrong to even attempt the conversion of the FVB member
functions?

The answer must be documented somewhere highly visible.

Debug patches attached, for the record (based on commit edd46cd407ea).

Thanks
Laszlo