Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support

[Laszlo Ersek <lersek@redhat.com>]

On 07/26/17 05:32, Jason Wang wrote:
> 
> 
> On 2017年07月26日 02:17, Laszlo Ersek wrote:
>> Adding Ard
>>
>> On 07/20/17 00:09, Brijesh Singh wrote:
>>> I have found that OVMF fails to detect the disk when iommu_platform
>>> is set from
>>> qemu cli. The failure occurs during the feature bit negotiation.
>>>
>>> Recently, EDKII introduced IOMMU protocol d1fddc4533bf. SEV patch
>>> series introduced
>>> a IoMmu protocol driver f9d129e68a45 to set a DMA access attribute
>>> and methods to
>>> allocate, free, map and unmap the DMA memory for the master bus devices
>>>
>>> In this patch series, I have tried to enable the IOMMU_PLATFORM
>>> feature for
>>> VirtioBlkDevice. I am sending this as RFC to seek feedback before I
>>> extend the support
>>> for other Virtio devices. The patch has been tested in SEV guest -
>>> mainly because
>>> IoMmuDxe driver installs the IOMMU protocol for SEV guest only. If
>>> needed, I can
>>> extend the IoMmuDxe driver to install IOMMU protocol for non SEV guests.
>>>
>>> qemu cli used for testing:
>>>
>>> # $QEMU \
>>>    ...
>>>    -drive file=${HDA_FILE},if=none,id=disk0,format=qcow2 \
>>>    -device
>>> virtio-blk-pci,drive=disk0,disable-legacy=on,iommu_platform=true,disable-modern=off,scsi=off
>>>
>>>    ...
>>>
>>> Cc: Jordan Justen<jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek<lersek@redhat.com>
>>> Cc: Jason Wang<jasowang@redhat.com>
>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Brijesh Singh<brijesh.singh@amd.com>
>>>
>>> Brijesh Singh (3):
>>>    OvmfPkg/Include/Virtio10: Define VIRTIO_F_IOMMU_PLATFORM feature bit
>>>    OvmfPkg/VirtioLib: Add IOMMU_PLATFORM support
>>>    OvmfPkg/VirtioBlkDxe: Add VIRITO_F_IOMMU_PLATFORM support
>> In the course of processing my post-PTO email backlog, yesterday I
>> skimmed this topic very quickly, and thought about it for an hour or so
>> (while away for the computer). Here's my take on it.
>>
>>
>> (1) The closest to any formal language that I found for
>> VIRTIO_F_IOMMU_PLATFORM are:
>>
>>    https://issues.oasis-open.org/browse/VIRTIO-154
>>    https://lists.oasis-open.org/archives/virtio-dev/201610/msg00121.html
>>
>> Are these references up-to-date? The ticket also references SVN r587 as
>> the resolution, but that link is dead.
>>
>>
>> (2) A few weeks back I saw Jason's SeaBIOS patch (also pointed out to me
>> more recently by Brijesh, in private):
>>
>>    [SeaBIOS] [PATCH] virtio: IOMMU support
>>
>> I don't understand this patch. (I also don't understand the
>> "iommu_platform" device property on the QEMU command line.) According to
>> the spec language quoted from the mailing list above, we have four cases:
>>
>> (2.1) device does not offer VIRITO_F_IOMMU_PLATFORM --> everything works
>> like before
>>
>> (2.2) device offers VIRITO_F_IOMMU_PLATFORM, but the driver does not
>> negotiate it --> device is allowed to reject functioning
>>
>> * device offers VIRITO_F_IOMMU_PLATFORM and the driver negotiates it:
>>    there are two possibilities:
>>    (2.3) the driver*disables*  the IOMMU, and works like before
>>    (2.4) the driver*configures*  the IOMMU and works accordingly
>>
>> The SeaBIOS patch negotiates VIRITO_F_IOMMU_PLATFORM, but it*neither*
>> disables*nor*  configures the IOMMU. It simply*ignores*  the IOMMU. I
>> don't see how that is any different*in effect*  from simply not
>> negotiating VIRITO_F_IOMMU_PLATFORM -- case (2.2) --, and I don't
>> understand why QEMU tolerates "ignoring the IOMMU" differently from "not
>> negotiating the flag".
> 
> I think the difference is we don't want give the ability of bypassing
> IOMMU at driver level. That's why we forbid it in the case of 2.2.
> 
> For 2.3 IOMMU was disabled by e.g guest os not driver.

Ah! That makes a lot of sense. I guess this is again motivated by the
DPDK use case -- the guest kernel would decide about IOMMU setup, but
the virtio driver (which is in guest userspace) is required to comply
with the IOMMU requirement, even if the guest kernel does not actually
program the IOMMU.

I hope my above interpretation is correct, because the recommendations
in my other (long) email match it 100%. Namely, in UEFI, IOMMU setup (or
non-setup) is separated to various platform drivers, and the virtio
device drivers should be abstracted from the actual IOMMU presence (this
is the gist of my long email). Hence simply confirming
VIRITO_F_IOMMU_PLATFORM is right for the device drivers.

Thanks
Laszlo