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

[Daniel Henrique Barboza]

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.


Thanks,


Daniel





> And there is a good reason for virtio-ccw devices to use
> F_IOMMU_PLATFORM (secure execution).
>
> Other opinions?
>
> Regards,
> Halil