Re: [PATCH v2 1/1] virtio: fix the condition for iommu_platform not supported

[Halil Pasic]

On Fri, 28 Jan 2022 08:02:39 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> > We may be able to differentiate between the two using ->dma_as, but for
> > that it needs to be set up correctly: whenever you require translation
> > it should be something different than address_space_memory. The question
> > is why do you require translation but don't have your ->dma_as set up
> > properly? It can be a guest thing, i.e. guest just assumes it has to do
> > bus addresses, while it actually does not have to, or we indeed do have
> > an IOMMU which polices the devices access to the guest memory, but for
> > some strange reason we failed to set up ->dma_as to reflect that.  
> 
> 
> I have 2 suggestions. First is to separate how we interpret iommu_platform. I 
> find it
> hard to do this properly without creating a new flag/command line option.
> 

A new command line option looks problematic to me because of the
existing setups. We could tie that to a compat machine, but it looks
ugly and also a little wrong from where I stand.
> 
> My second suggestion is, well .... I think it's proved that s390x-PV and AMD 
> SEV are
> being impacted (and probably Power secure guests as well), so why not check 
> for
> confidential guest support to skip that check entirely? Something like this 
> patch:
> 

This is not acceptable for s390x and it should not be acceptable for SEV
or Power secure guests, because s390x Secure Execution ()support predates
the confidential guest support patches and "->cgs", and thus you don't
have to turn on CGS to use SE. Just providing the iommu_platform=on
manually on each device is perfectly fine! Should be the same for SEV

[..]
> +    if (!machine->cgs && has_iommu &&
> +        !virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) {
>           error_setg(errp, "iommu_platform=true is not supported by the 
> device");
>           return;
>       }
[..]

> This will not break anything for non-secure guests and, granted that 
> machine->cgs is already
> set at this point, this will fix the problem for s390x-PV and AMD SEV. And we 
> won't have to
> dive deep into a virtio-bus feature negotiation saga because of something 
> that can be easily
> handled for machine->cgs guests only.

Your assumption does not hold. See above. Unfortunately my assumption of 
->dma_as == & address_space_memory implies does not need translation
does not hold either. But IMHO we should really get to the bottom of
that, because it just does not make sense.

> 
> If this patch works for you and Brijesh I believe this is a good option.

I don't believe it is a good option. @Brijesh can you confirm that SEV
has the same problem with this approach s390x has, and that it would
break existing setups?

I have another idea, but my problem is that I don't understand enough of
the Power and PCI stuff. Anyway if for your plattform iommu_platform=on
devices can not work in a VM that does not have an IOMMU you could
error out on that. You could express that via a machine property, and
then make sure your dma address space is not address_space_memory, if
that machine property is set.

Regards,
Halil