On Wed, Feb 02, 2022 at 02:15:47AM +0100, 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.
>
> 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.
>
> 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);
> }
>
> which would be a separate patch, as this is a separate issue. Jason,
> Michael, Connie, what do you think?
Do you mean just force VIRTIO_F_IOMMU_PLATFORM for everyone?
Or am I misreading the patch?
> Regards,
> Halil
