On Wed, 2 Feb 2022 10:24:51 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 2/1/22 22:15, Halil Pasic wrote:
On Tue, 1 Feb 2022 16:31:22 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
On 2/1/22 15:33, Halil Pasic wrote:
On Tue, 1 Feb 2022 12:36:25 -0300
Daniel Henrique Barboza <danielhb@linux.ibm.com> wrote:
+ vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
if (klass->get_dma_as != NULL && has_iommu) {
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
vdev->dma_as = klass->get_dma_as(qbus->parent);
+ if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
+ error_setg(errp,
+ "iommu_platform=true is not supported by the device");
+ }
} else {
vdev->dma_as = &address_space_memory;
}
I struggled to understand what this 'else' clause was doing and I assumed that
it was
wrong. Searching through the ML I learned that this 'else' clause is intended
to handle
legacy virtio devices that doesn't support the DMA API (introduced in
8607f5c3072caeebb)
and thus shouldn't set VIRTIO_F_IOMMU_PLATFORM.
My suggestion, if a v4 is required for any other reason, is to add a small
comment in this
'else' clause explaining that this is the legacy virtio devices condition and
those devices
don't set F_IOMMU_PLATFORM. This would make the code easier to read for a
virtio casual like
myself.
I do not agree that this is about legacy virtio. In my understanding
virtio-ccw simply does not need translation because CCW devices use
guest physical addresses as per architecture. It may be considered
legacy stuff form PCI perspective, but I don't think it is legacy
in general.
I wasn't talking about virtio-ccw. I was talking about this piece of code:
if (klass->get_dma_as != NULL && has_iommu) {
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
vdev->dma_as = klass->get_dma_as(qbus->parent);
} else {
vdev->dma_as = &address_space_memory;
}
I suggested something like this:
if (klass->get_dma_as != NULL && has_iommu) {
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
vdev->dma_as = klass->get_dma_as(qbus->parent);
} else {
/*
* We don't force VIRTIO_F_IOMMU_PLATFORM for legacy devices, i.e.
* devices that don't implement klass->get_dma_as, regardless of
* 'has_iommu' setting.
*/
vdev->dma_as = &address_space_memory;
}
At least from my reading of commits 8607f5c3072 and 2943b53f682 this seems to be
the case. I spent some time thinking that this IF/ELSE was wrong because I
wasn't
aware of this history.
With virtio-ccw we take the else branch because we don't implement
->get_dma_as(). I don't consider all the virtio-ccw to be legacy.
IMHO there are two ways to think about this:
a) The commit that introduced this needs a fix which implemets
get_dma_as() for virtio-ccw in a way that it simply returns
address_space_memory.
b) The presence of ->get_dma_as() is not indicative of "legacy".
BTW in virtospeak "legacy" has a special meaning: pre-1.0 virtio. Do you
mean that legacy. And if I read the virtio-pci code correctly
->get_dma_as is set for legacy, transitional and modern devices alike.
Oh ok. I'm not well versed into virtiospeak. My "legacy" comment was a poor
choice of
word for the situation.
We can ignore the "legacy" bit. My idea/suggestion is to put a comment at that
point
explaining the logic behind into not forcing VIRTIO_F_IOMMU_PLATFORM in devices
that
doesn't implement ->get_dma_as().
I am assuming that this is an intended design that was introduced by 2943b53f682
("virtio: force VIRTIO_F_IOMMU_PLATFORM"), meaning that the implementation of
the
->get_dma_as is being used as a parameter to force the feature in the device.
And with
this code:
if (klass->get_dma_as != NULL && has_iommu) {
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
vdev->dma_as = klass->get_dma_as(qbus->parent);
} else {
vdev->dma_as = &address_space_memory;
}
It is possible that we have 2 vdev devices where ->dma_as =
&address_space_memory, but one
of them is sitting in a bus where "klass->get_dma_as(qbus->parent) =
&address_space_memory",
and this device will have VIRTIO_F_IOMMU_PLATFORM forced onto it and the former
won't.
If this is not an intended design I can only speculate how to fix it. Forcing
VIRTIO_F_IOMMU_PLATFORM
in all devices, based only on has_iommu, can break stuff. Setting
VIRTIO_F_IOMMU_PLATFORM only
if "vdev->dma_as != &address_space_memory" make some sense but I am fairly
certain it will
break stuff the other way. Or perhaps the fix is something else entirely.
IMHO the important thing to figure out is what impact that
virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
in the first branch (of the if-else) has. IMHO if one examines the
commits 8607f5c307 ("virtio: convert to use DMA api") and 2943b53f68
("virtio: force VIRTIO_F_IOMMU_PLATFORM") very carefully, one will
probably reach the conclusion that the objective of the latter, is
to prevent the guest form not negotiating the IOMMU_PLATFORM feature
(clearing it as part of the feature negotiation) and trying to use
the device without that feature. In other words, virtio features are
usually optional for the guest for the sake of compatibility, but
IOMMU_PLATFORM is not: for very good reasons. Neither the commit message
nor the patch does mention legacy anywhere.
In my opinion not forcing the guest to negotiate IOMMU_PLATFORM when
->get_dma_as() is not set is at least unfortunate. Please observe, that
virtio-pci is not affected by this omission because for virtio-pci
devices ->get_dma_as != NULL always holds. And what is the deal for
devices that don't implement get_dma_as() (and don't need address
translation)? If iommu_platform=on is justified (no user error) then
the device does not have access to the entire guest memory. Which
means it more than likely needs cooperation form the guest (driver).
So detecting that the guest does not support IOMMU_PLATFORM and failing
gracefully via virtio_validate_features() instead of carrying on
in good faith and failing in ugly ways when the host attempts to access
guest memory to which it does not have access to. If we assume user
error, that is the host can access at least all the memory it needs
to access to make that device work, then it is probably still a
good idea to fail the device and thus help the user correct his
error.
Yeah, this go back on what I've said about 2943b53f682 up there. There are
assumptions
being made on the ->get_dma_as() existence that aren't clear.
I agree. The commit message does not explain.
IMHO the best course of action is
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 34f5a0a664..1d0eb16d1c 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -80,7 +80,6 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error
**errp)
vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
if (klass->get_dma_as != NULL && has_iommu) {
- virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
vdev->dma_as = klass->get_dma_as(qbus->parent);
if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
error_setg(errp,
@@ -89,6 +88,7 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error
**errp)
} else {
vdev->dma_as = &address_space_memory;
}
+ virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
}
I am fairly confident that forcing VIRTIO_F_IOMMU_PLATFORM all around, based on
has_iommu
Yes I should have made that conditional on has_iommu. It was very late
when I finished that email.
alone, will have consequences all around. This code has been around for almost
5 years and a
lot of stuff has been developed on top of it.
Do you have any particular examples in mind?
All that said, if this is the proper way of fixing it I'd say to do it now,
document it properly
and fix the breakages as they come along. The alternative - hacking around and
around a codebase
that might not be solid - is worse in the long run.
IMHO this is a good discussion you triggered. But I see it out of scope
for the bug I'm trying to fix.
I can post a proper patch for "IOMMU_PLATFORM is non-negotiable for
all guests" and we can have proper review and discussion on that. But
I would like the bug I'm working on here fixed first. There are
people that want to use virtiofs with confidential guests, and
we should really make sure they can.
Regards,
Halil