On Mon, 7 Feb 2022 08:46:34 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 2/3/22 13:45, Halil Pasic wrote:
Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e.
the driver must accept it if offered by the device. The virtio
specification says that the driver SHOULD accept the ACCESS_PLATFORM
feature if offered, and that the device MAY fail to operate if
ACCESS_PLATFORM was offered but not negotiated.
While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
the device when the driver fences ACCESS_PLATFORM. With commit
I believe a link to the virtio specification where this is being mentioned would
be good to have in the commit message.
I can add that if Michael agrees, and if the patch is deemed worthy.
2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
decision to do so whenever the get_dma_as() callback is implemented (by
the bus), which in practice means for the entirety of virtio-pci.
That means, if the device needs to translate I/O addresses, then
ACCESS_PLATFORM is mandatory. The aforementioned commit tells us
in the commit message that this is for security reasons.
If ACCESS_PLATFORM is offered not we want the device to utilize an
I think you meant "If ACCESS_PLATFORM is offered".
I'm missing because. I.e. s/not/not becasue/
IOMMU and do address translation, but because the device does not have
access to the entire guest RAM, and needs the driver to grant access
to the bits it needs access to (e.g. confidential guest support), we
still require the guest to have the corresponding logic and to accept
ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
things are bound to go wrong, and we may see failures much less graceful
than failing the device because the driver didn't negotiate
ACCESS_PLATFORM.
So let us make ACCESS_PLATFORM mandatory for the driver regardless
of whether the get_dma_as() callback is implemented or not.
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")
---
This patch is based on:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg866199.html
During the review of "virtio: fix the condition for iommu_platform not
supported" Daniel raised the question why do we "force IOMMU_PLATFORM"
iff has_iommu && !!klass->get_dma_as. My answer to that was, that
this logic ain't right.
While at it I used the opportunity to re-organize the code a little
and provide an explanatory comment.
---
hw/virtio/virtio-bus.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index fbf0dd14b8..359430eb1c 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error
**errp)
return;
}
- vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
- if (klass->get_dma_as != NULL && has_iommu) {
+ vdev->dma_as = &address_space_memory;
At this point you can also do:
if (!has_iommu) {
return;
}
and the rest of the code will have one less indentation level.
I have considered this and decided against it. The reason why is
if that approach is taken, we can't really add more code to the
end of the function. An early return is good if we want to
abort the function with an error. My point is !has_iommu does
not necessarily mean we are done: after a block that handles
the has_iommu situation, in future, there could be a block that
handles something different.