Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Halil Pasic]

On Thu, 10 Feb 2022 12:19:25 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

[..]
> 
> Nope, that's not my problem. We make sure that the bit is persistent, we
> fail realization if the bit got removed by the callback when required,
> and we fail feature validation if the driver removes the bit, which is
> in a different code path. We should not talk about FEATURES_OK in this
> code.

I agree. I changed my mind. Expanation follows...

> 
> We force-add the bit, and then still might fail realization. The
> important condition is the has_iommu one, not the checks later on. I
> find it very confusing to talk about what a potential driver might do in
> that context.
> 

I assumed stuff like virtiofs+SE regressed with commit 04ceb61a40
("virtio: Fail if iommu_platform is requested, but unsupported") but I
think, I was wrong. It didn't work before that, because we did not
present ACCESS_PLATFORM to the guest. I operated under the assumption
that virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM)
does not impact the set of features offered to the driver by the device,
but it does.

So we need both this patch and "[PATCH v5 1/1] virtio: fix the condition
for iommu_platform not supported" to get virtiofs to work with SE/SEV/Secure VM.

I have to admit I only tested for the error message, and not with a full
SE setup.

I agree my comment was inadequate. Can we use

/* make sure that the device did not drop a required IOMMU_PLATFORM */

I will think some more though. This is again about the dual nature
of ACCESS_PLATFORM...

> What about moving the virtio_add_feature() after the if
> (klass->get_dma_as) check, and adding a comment
> 
> /* we want to always force IOMMU_PLATFORM here */
> 
> [I'll withdraw from this discussion for now, I fear I might just add
> confusion.]
> 
>