On Thu, 10 Feb 2022 10:55:13 +0100 Cornelia Huck <[email protected]> wrote:
> On Wed, Feb 09 2022, Halil Pasic <[email protected]> wrote: > > > On Wed, 09 Feb 2022 18:24:56 +0100 > > Cornelia Huck <[email protected]> wrote: > > > >> On Wed, Feb 09 2022, Halil Pasic <[email protected]> wrote: > >> > @@ -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; > >> > + if (has_iommu) { > >> > + vdev_has_iommu = virtio_host_has_feature(vdev, > >> > VIRTIO_F_IOMMU_PLATFORM); > >> > + /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM > >> > */ > >> > >> I must admit that the more I stare at this code, the more confused I > >> get. We run this function during device realization, and the reason that > >> the feature bit might have gotten lost is that the ->get_features() > >> device callback dropped it. This happens before the driver is actually > >> involved; the check whether the *driver* dropped the feature is done > >> during feature validation, which is another code path. > > [moved text from here] > >> > >> > virtio_add_feature(&vdev->host_features, > >> > VIRTIO_F_IOMMU_PLATFORM); [Mark 1] > > > > > > Let us have a look at > > static int virtio_validate_features(VirtIODevice *vdev) > > > > { > > > > VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev); > > > > > > > > if (virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM) && > > > > !virtio_vdev_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM)) { > > > > return -EFAULT; > > [Mark 2] > > } > > > > [..] > > > > So were it not of the [Mark 1] we could not hit [Mark 2] if the feature > > bit was lost because the ->get_features() callback dropped it. Yes, > > feature negotiation is another code path, but the two are interdependent > > in a non-trivial way. That is why I added that comment. > > Yes, of course we need to offer the bit to the driver in the first > place. My point is that the code here is not what makes us fail > FEATURES_OK; we won't even get to that point because the device will > fail realization. I disagree! Have you tested your hypothesis? Which line of code does cause the device realization to fail? Where is that check? > > > > > [moved here] > >> So what we do > >> here is failing device realization if a backend doesn't support > >> IOMMU_PLATFORM, isn't it? > > > > Not really. We fail the device realization if !vdev_has_iommu && > > vdev->dma_as != &address_space_memory, that is the device does not > > support address translation, but we need it to support address > > translation because ->dma_as != &address_space memory. If however > > ->dma_as == &address_space memory we carry on happily even if > > ->get_features() dropped > > IOMMU_PLATFORM, because we don't actually need an iova -> gpa > > translation. This is the case with virtiofs confidential guests for > > example. > > > > Well yes, that's what I meant, I just did not spell out all of the > conditions... > > > But we still don't want the guest dropping ACCESS_PLATFORM, because it is > > still mandatory, because the device won't operate correctly unless the > > driver grants access to the pieces of memory that the device needs to > > access. The underlying mechanism of granting access may not have > > anything to do with an IOMMU though. > > > > Does it make sense now? > > The code yes, the comment no. What we are actually doing is failing > realization so we don't end up offering a device without IOMMU_PLATFORM > that would need it. I don't understand. That is only one of the possible cases IMHO. Do you mean the check if (klass->get_dma_as) { 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"); return; } } or something different? If yo mean that check, it does not cover all cases where has_iommu. Please note that the line in question is if (has_iommu) { vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM); /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */ virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); only conditional on has_iommu. But we want the guest to *never* drop ACCESS_PLATFORM, regardless of vdev_has_iommu and ->dma_as. Please also note that the comment /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */ is intended to document why do we do virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); _only_ and is not intended to document the entire code that follows: virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM); if (klass->get_dma_as) { 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"); return; } } Is that the source of the confusion? If yes, maybe I should add a blank line after virtio_add_feature(). Regards, Halil > The code that fails FEATURES_OK if the driver > dropped it is already in place. > > I'd suggest a comment like > > /* make sure that the device did not drop a required IOMMU_PLATFORM */ > > or so. > >
