Re: [RFC 0/4] Virtio uses DMA API for all devices

[Jiandi An]

On 08/09/2018 12:40 AM, Christoph Hellwig wrote:
> On Thu, Aug 09, 2018 at 08:13:32AM +1000, Benjamin Herrenschmidt wrote:
>>>> -	if (xen_domain())
>>>> +	if (xen_domain() || pseries_secure_vm())
>>>> 		return true;
>>>
>>> I don't think it's pseries specific actually. E.g. I suspect AMD SEV
>>> might benefit from the same kind of hack.
>>
>> As long as they can provide the same guarantee that the DMA ops are
>> completely equivalent between virtio and other PCI devices, at least on
>> the same bus, ie, we don't have to go hack special DMA ops.
>>
>> I think the latter is really what Christoph wants to avoid for good
>> reasons.
> 
> Yes.  I also generally want to avoid too much arch specific magic.
> 
> FYI, I'm off to a week-long vacation today, don't expect quick replies.
> 
> 
> 

I've been following this RFC series as this has impact on AMD SEV.
Could you guys keep us in the loop on this (thomas.lendacky@xxxxxxx,
brijesh.singh@xxxxxxx, jiandi.an@xxxxxxx are on cc).

AMD SEV today sets swiotlb_force to SWIOTLB_FORCE early on in
x86_64_start_kernel.  During start_kernel, mem_encrypt_init() sets
dma_ops to swiotlb_dma_ops if SEV is on as it uses SWIOTLB to bounce
buffer DMA operation and it's marked as decrypted.

For virtio device we have to pass in iommu_platform=true flag for
this to set the VIRTIO_F_IOMMU_PLATFORM flag. But for example
QEMU has the use of iommu_platform attribute disabled for virtio-gpu
device.  So would also like to move towards not having to specify
the VIRTIO_F_IOMMU_PLATFORM flag.

Anshuman's patch [RFC,2/4] virtio: Override device's DMA OPS with
virtio_direct_dma_ops selectively sets the default dma ops of
virtio device's parent pci to direct_dma_ops if VIRTIO_F_IOMMU_PLATFORM
is set.  And later platform specific code can override the dma ops again.

 int virtio_finalize_features(struct virtio_device *dev)
 {
 	int ret = dev->config->finalize_features(dev);
@@ -174,6 +176,9 @@  int virtio_finalize_features(struct virtio_device *dev)
 	if (ret)
 		return ret;
 
+	if (virtio_has_iommu_quirk(dev))
+		set_dma_ops(dev->dev.parent, &virtio_direct_dma_ops);
+
 	if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1))
 		return 0;

Would like to be in the loop and put in the platform specific code for
AMD SEV at the same time along with this.  Or does it make sense that if
swiotlb force is set don't override virtio device's parent dma_ops to
direct_dma_ops?  What if someone passes in swiotlb=force from kernel
boot command line parameter, this would override the parent pci device's
dma ops to direct_dma_ops.

So where is this RFC standing currently.  I saw Michael's comment mentioning
this RFC is blocked by its performance overhead for now for switching to
using DMA ops unconditionally.

-Jiandi