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

[Laszlo Ersek <lersek@redhat.com>]

On 09/05/17 20:57, Brijesh Singh wrote:
> Hi Laszlo,
> 
> Thanks for quick reviews. I will go through each feedback address them
> in v2.
> 
> 
> 
> On 09/05/2017 06:47 AM, Laszlo Ersek wrote:
> 
> [...]
> 
>>
>> Please also modify the commit message similarly: "map the VRING system
>> physical address[es] to device address[es]".
>>
>>
>> Would it be OK with you to submit only these two patches next?
> 
> 
> I saw that you looked at other patches, just wondering, do you still
> want me
> to submit two patches and advance in small steps ?

I'm not so sure anymore...

I haven't looked at patch #3 yet. I guess I could review it tomorrow.

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


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


* Option #3: ignore pending mappings (= decrypted memory areas) at
  ExitBootServices(), leave them all to the OS.

  Problem: how will the OS find them? Should we introduce another UEFI
  configuration table? Config tables are generally allocated in
  BootServicesData type memory, so the boot loader has to save them. Who
  will write the grub2 code? Who will write the Linux kernel code to
  parse the table, and to re-encrypt the memory (inherited from the
  firmware) in-place?


* Option #4: ignore pending mappings (= decrypted memory areas) at
  ExitBootServices(). Ignore them in the OS too. Let's hope that "it's
  just a few pages of stale data, it won't compromise guest
  confidentiality when the hypervisor is breached". Doesn't sound like a
  good sales pitch for SEV.


Our approach thus far has been option #1 for the OvmfPkg VirtIo drivers,
and option #4 for all PCI drivers outside of OvmfPkg. If you are fine
with this, I am as well; we can continue this approach. (We should be
conscious of it though.)

My other series (see above) was inteded to extend option #1 to some
MdeModulePkg drivers (the main ones that we pull into OVMF, USB 1-2-3
and AHCI), and to start a discussion. If you believe that extending
option #1 to MdeModulePkg would be better for SEV, we should await the
outcome of that discussion. (Please do participate.)

Thanks,
Laszlo

>> I wouldn't like to look at the rest of the patches just now. I expect
>> quite a few tweaks -- especially because I would *really* like to keep
>> "TechNotes.txt" up to date! --, and I'd like to keep my focus, and
>> advance in small steps. I must re-read TechNotes.txt myself, in parallel
>> to the progress that we make with this series.
>>
>> Once we consider these patches complete, we can test them and commit
>> them in isolation. Further versions of the series won't have to repeat
>> these patches.
>>
>> I'll strive to be very responsive, so that you don't have to wait long
>> after every small step.
>>
>