On Wed, Feb 02, 2022 at 05:23:53PM +0100, Halil Pasic wrote:
> On Wed, 2 Feb 2022 10:24:51 -0300
> Daniel Henrique Barboza <danielhb...@gmail.com> wrote:
>
> > On 2/1/22 22:15, Halil Pasic wrote:
> > > On Tue, 1 Feb 2022 16:31:22 -0300
> > > Daniel Henrique Barboza <danielhb...@gmail.com> wrote:
> > >
> > >> On 2/1/22 15:33, Halil Pasic wrote:
> > >>> On Tue, 1 Feb 2022 12:36:25 -0300
> > >>> Daniel Henrique Barboza <danie...@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
I think I second that.
--
MST
