Re: [PATCH 0/6] OvmfPkg/VirtioGpuDxe: map system memory addresses to device addresses

[Laszlo Ersek <lersek@redhat.com>]

On 08/28/17 15:24, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: virtio_gpu_map
>
> This series brings IOMMU support to VirtioGpuDxe. The interesting part
> is patch#5.
>
> I formatted the patches with function context, for easier review.
>
> I regression-tested this series in various environments; IA32, X64,
> AARCH64. My access to a SEV machine is still in the making, so I
> couldn't test the patches on SEV.
>
> Brijesh, if you could test the series for me on SEV, that would be
> great. I also plan to test it on SEV, once my access is established.

I have great test results on SEV. Everything works as expected:

- normal display behavior (setup browser, shell commands, scrolling),

- connect, disconnect, reconnect in the UEFI shell, issued from the
  serial console,

- mode changes in the UEFI shell (after reconnect too).

I can see the IOMMU / SEV library debug messages too, and I can
associate them with GPU actions.

I also tested ExitBootServices(). In the log, I see the following six
(+1) consecutive lines (broken up here for easier reading):

>  244888 IoMmuUnmap PlainText 0x7D273000 Crypted 0x7D273000 Pages 0x300 Bytes 0x300000
>  244889 IoMmuDxe:InternalMemEncryptSevSetMemoryEncrypted Set C-bit Cr3 0 Base 7D273000 Length 300000 flush 1

>  244890 IoMmuUnmap PlainText 0x7ED81000 Crypted 0x7ED81000 Pages 0x2 Bytes 0x2000
>  244891 IoMmuDxe:InternalMemEncryptSevSetMemoryEncrypted Set C-bit Cr3 0 Base 7ED81000 Length 2000 flush 1

>  244892 IoMmuUnmap PlainText 0x7F0DA000 Crypted 0x7F0DA000 Pages 0x2 Bytes 0x2000
>  244893 IoMmuDxe:InternalMemEncryptSevSetMemoryEncrypted Set C-bit Cr3 0 Base 7F0DA000 Length 2000 flush 1

>  244894 MpInitChangeApLoopCallback() done!

* The first two lines correspond to unmapping (re-encrypting) the
  backing store of the GOP's current video mode (see VirtioGpuExitBoot()
  in patch #5). Searching the log backwards for the same address
  (0x7D273000), I find it associated with the latest MODE change from
  the UEFI shell (when this backing store was allocated and decrypted):

>  188052 IoMmuAllocateBuffer Address 0x7D273000 Pages 0x300
>  188053 IoMmuDxe:InternalMemEncryptSevSetMemoryDecrypted Clear C-bit Cr3 0 Base 7D273000 Length 300000 flush 1
>  188054 IoMmuDxe:SetMemoryEncDec Spliting 2MB page at 7D273000
>  188055 IoMmuMap PlainText 0x7D273000 Crypted 0x7D273000 Pages 0x300 Bytes 0x300000

* The second two lines correspond to unmapping (re-encrypting) the
  virtio ring for the virtio-gpu device (see VirtioGpuExitBoot() in
  patch #1). Searching the log backwards for the same address
  (0x7ED81000), I find it associated with the latest connecting of the
  virtio-gpu device, which is when the virtio-ring is allocated and
  mapped/decrypted (see VirtioRingMap() in patch #1):

>  158667 InstallProtocolInterface: [VirtioDeviceProtocol] 7F104B20
>  158668 IoMmuAllocateBuffer Address 0x7ED81000 Pages 0x2
>  158669 IoMmuDxe:InternalMemEncryptSevSetMemoryDecrypted Clear C-bit Cr3 0 Base 7ED81000 Length 2000 flush 1
>  158670 IoMmuMap PlainText 0x7ED81000 Crypted 0x7ED81000 Pages 0x2 Bytes 0x2000
>  ...
>  158700 InstallProtocolInterface: [EfiGraphicsOutputProtocol] 7F1546C0
>  158701 VirtioGpuDriverBindingStart: produced GOP while binding VirtIo=7F104B20

* The third pair of lines stands for unmapping the virtio ring of the
  virtio-blk device that I have also configured for the guest. Searching
  the log backwards for the same address (0x7F0DA000), we find

>    7347 IoMmuAllocateBuffer Address 0x7F0DA000 Pages 0x2
>    7348 IoMmuDxe:InternalMemEncryptSevSetMemoryDecrypted Clear C-bit Cr3 0 Base 7F0DA000 Length 2000 flush 1
>    7349 IoMmuMap PlainText 0x7F0DA000 Crypted 0x7F0DA000 Pages 0x2 Bytes 0x2000
>    7350 VirtioBlkInit: LbaSize=0x200[B] NumBlocks=0xC800000[Lba]
>    7351 VirtioBlkInit: FirstAligned=0x0[Lba] PhysBlkSize=0x1[Lba]
>    7352 VirtioBlkInit: OptimalTransferLengthGranularity=0x0[Lba]
>    7353 InstallProtocolInterface: [EfiBlockIoProtocol] 7F0DE310
>    7354 InstallProtocolInterface: [EfiDiskIoProtocol] 7F0DE4A0

* The final line that I quoted ("MpInitChangeApLoopCallback") is just
  another (unrelated) ExitBootServices() handler's message.

I'm going to call these results successful.

Thanks,
Laszlo

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (6):
>   OvmfPkg/VirtioGpuDxe: map VRING for bus master common buffer operation
>   OvmfPkg/VirtioGpuDxe: map virtio GPU command objects to device
>     addresses
>   OvmfPkg/VirtioGpuDxe: take EFI_PHYSICAL_ADDRESS in
>     ResourceAttachBacking()
>   OvmfPkg/VirtioGpuDxe: helpers for backing store
>     (de)allocation+(un)mapping
>   OvmfPkg/VirtioGpuDxe: map backing store to bus master device address
>   OvmfPkg/VirtioGpuDxe: negotiate VIRTIO_F_IOMMU_PLATFORM
>
>  OvmfPkg/VirtioGpuDxe/VirtioGpu.h     |  89 ++++++-
>  OvmfPkg/VirtioGpuDxe/Commands.c      | 263 ++++++++++++++++++--
>  OvmfPkg/VirtioGpuDxe/DriverBinding.c |   1 -
>  OvmfPkg/VirtioGpuDxe/Gop.c           |  62 +++--
>  4 files changed, 370 insertions(+), 45 deletions(-)
>