Re: [PATCH] s390: protvirt: virtio: Refuse device without IOMMU
From: Jason Wang <jasowang@redhat.com>
Date: 2020-06-15 03:02:11
Also in:
linux-s390, lkml, virtualization
On 2020/6/12 下午7:38, Pierre Morel wrote:
On 2020-06-12 11:21, Pierre Morel wrote:quoted
On 2020-06-11 05:10, Jason Wang wrote:quoted
On 2020/6/10 下午9:11, Pierre Morel wrote:quoted
Protected Virtualisation protects the memory of the guest and do not allow a the host to access all of its memory. Let's refuse a VIRTIO device which does not use IOMMU protected access. Signed-off-by: Pierre Morel <redacted> --- drivers/s390/virtio/virtio_ccw.c | 5 +++++ 1 file changed, 5 insertions(+)diff --git a/drivers/s390/virtio/virtio_ccw.cb/drivers/s390/virtio/virtio_ccw.c index 5730572b52cd..06ffbc96587a 100644--- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c@@ -986,6 +986,11 @@ static void virtio_ccw_set_status(structvirtio_device *vdev, u8 status) if (!ccw) return; + /* Protected Virtualisation guest needs IOMMU */ + if (is_prot_virt_guest() && + !__virtio_test_bit(vdev, VIRTIO_F_IOMMU_PLATFORM)) + status &= ~VIRTIO_CONFIG_S_FEATURES_OK; + /* Write the status to the host. */ vcdev->dma_area->status = status; ccw->cmd_code = CCW_CMD_WRITE_STATUS;I wonder whether we need move it to virtio core instead of ccw. I think the other memory protection technologies may suffer from this as well. ThanksWhat would you think of the following, also taking into account Connie's comment on where the test should be done: - declare a weak function in virtio.c code, returning that memory protection is not in use. - overwrite the function in the arch code - call this function inside core virtio_finalize_features() and if required fail if the device don't have VIRTIO_F_IOMMU_PLATFORM.
I think this is fine.
quoted hunk ↗ jump to hunk
quoted
Alternative could be to test a global variable that the architecture would overwrite if needed but I find the weak function solution more flexible. With a function, we also have the possibility to provide the device as argument and take actions depending it, this may answer Halil's concern. Regards, Pierrehum, in between I found another way which seems to me much better: We already have the force_dma_unencrypted() function available which AFAIU is what we want for encrypted memory protection and is already used by power and x86 SEV/SME in a way that seems AFAIU compatible with our problem. Even DMA and IOMMU are different things, I think they should be used together in our case. What do you think? The patch would then be something like:diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index a977e32a88f2..53476d5bbe35 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c@@ -4,6 +4,7 @@#include <linux/virtio_config.h> #include <linux/module.h> #include <linux/idr.h> +#include <linux/dma-direct.h> #include <uapi/linux/virtio_ids.h> /* Unique numbering for virtio devices. */@@ -179,6 +180,10 @@ int virtio_finalize_features(struct virtio_device*dev) if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) return 0; + if (force_dma_unencrypted(&dev->dev) && + !virtio_has_feature(dev, VIRTIO_F_IOMMU_PLATFORM)) + return -EIO; + virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); status = dev->config->get_status(dev); if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
I think this can work but need to listen from Michael. Thanks
Regards, Pierre