Re: [PATCH 1/5] OvmfPkg/VirtioNetDxe: map VRING using VirtioRingMap()

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 5 September 2017 at 21:17, Laszlo Ersek <lersek@redhat.com> wrote:
[...]
> ... Based on the discussion in the other thread ("[edk2] [PATCH 0/4]
> MdeModulePkg: some PCI HC drivers: unmap common buffers at
> ExitBootServices()"), I'm worried that we might need another
> restructuring for the IOMMU driver, and for the ExitBootServices()
> handlers for the virtio drivers (removing the Unmap() calls).
>
> If that's the case, then it wouldn't be good if you wasted work on
> VirtioNetExitBoot() in v2 of this series.
>
> Also under patch v1 #4, I requested that you not use
> VirtioOperationBusMasterRead for DMA that remains pending after the
> protocol member function returns, because we cannot Unmap()
> BusMasterRead in VirtioNetExitBoot() without freeing memory. Dependent
> of the outcome of said other thread, this might not be good advice after
> all.
>
> I'd be pretty disappointed if we had to go back to the drawing board at
> this stage. In my opinion, the UEFI-2.7 spec doesn't offer any
> facilities to us that would let us reliably and safely Unmap() bus
> master operations (= re-encrypt memory) at ExitBootServices(), for such
> PCI device drivers that we cannot modify. Whatever we do at this point
> looks like a hack:
>
>
> * Option #1: modify Virtio and other PCI drivers to use only
>   CommonBuffer operations for asynch DMA, and manually Unmap() those
>   operations in the ExitBootServices() handlers of the drivers. In
>   addition, guarantee that Unmap() for CommonBuffer will not release
>   memory.
>
>   This is the approach I've been supporting. We could implement it for
>   OVMF, because the community controls most of the platform (QEMU,
>   KVM, OVMF), OVMF is 100% open source, and we can propose patches for
>   other (e.g. MdeModulePkg) PCI drivers in edk2, if necessary.
>
>   Problem: PCI drivers are not required by the spec, or the Driver
>   Writer's Guide, to Unmap() on ExitBootServices() (and then to Unmap()
>   only CommonBuffer). Also, PciIo implementations are not required by
>   the spec to behave like this (= not free memory when Unmap()ing
>   CommonBuffer). We can satisfy those assumptions in OvmfPkg, but
>   perhaps not in MdeModulePkg drivers.
>

I think you will have much bigger problems with out of tree PCI
drivers that never use Map/Unmap in the first place, because stuff
just works on PCs if you omit them.

This is actually the reason I am quite pleased with this SEV support:
it means x86 drivers will start breaking in the same way as they would
have on ARM systems with non-coherent DMA or non-1:1 mapped PCI, and
vendors will have to clean up their code anyway, not just for ARM
compatibility.

The only requirement imposed on DMA capable devices is that they cease
to perform DMA after ExitBootServices. Anything beyond that should not
be the responsibility of the device driver, simply because that
requirement did not exist before, and so we cannot go back in time and
add it.

> * Option #2: add an ExitBootServices() handler to the IOMMU driver, and
>   clean up mappings (= re-encrypt memory) after the PCI / Virtio drivers
>   are finalized in their ExitBootServices() handlers. Ignore mappings in
>   the drivers' ExitBootServices() handlers.
>
>   Problem: the keyword is "after". Under this approach, we *must* clean
>   up the mappings in the IOMMU driver, but we *must not* do that before
>   the device drivers are finished. And the UEFI spec does not allow us
>   to express a dependency order between ExitBootServices() handlers.
>
>   We could hack that around by deferring the actual cleanup to *another*
>   event handler, signaled from the IOMMU's "normal" ExitBootServices()
>   handler.
>
>   Problem: the UEFI spec does not promise that signaling events from
>   ExitBootServices() handlers is safe.
>
>   Problem: if some PCI driver does the same trick for whatever reason in
>   its ExitBootServices() handler, then we're back to square one.
>
>

I think this is the only feasible option. Fortunately, we don't have
to solve this at the spec level, but only have to deal with the
implementation.

What we need is a method in the IOMMU protocol that is invoked when
the ExitBootServices implementation is about to return EFI_SUCCESS
(which means it could be the second time it was called). This severely
limits what the method is actually capable of doing, and I think it
should not be allowed to call any boot or DXE services at all, but it
should still be sufficient for some linked list traversals and
CopyMem() operations, and whatever else is needed to re-encrypt the
memory.

And actually, this is not only useful for SEV, other IOMMU drivers
will have to deal with the same issue: in most cases, you'll want to
disable it before handing over to the OS, but this can never be done
safely in a EBS event handler for precisely the reasons you pointed
out. Most PCI devices probably deal with this gracefully, but tearing
down mappings should simply be avoided if a device could still be
accessing it.