[libvirt PATCH] conf: allow virtio driver attributes for vhostuser disk

Pavel Hrdina posted 1 patch 3 years, 1 month ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/30bc3ff30bad62bccd603c0a17dae648f131805e.1612526846.git.phrdina@redhat.com
src/conf/domain_validate.c                    | 20 -------------------
.../disk-vhostuser.x86_64-latest.args         |  4 ++--
tests/qemuxml2argvdata/disk-vhostuser.xml     |  2 +-
.../disk-vhostuser.x86_64-latest.xml          |  2 +-
4 files changed, 4 insertions(+), 24 deletions(-)
[libvirt PATCH] conf: allow virtio driver attributes for vhostuser disk
Posted by Pavel Hrdina 3 years, 1 month ago
All of these options are actually supported by vhostuser disk so
we should allow them to be usable.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/conf/domain_validate.c                    | 20 -------------------
 .../disk-vhostuser.x86_64-latest.args         |  4 ++--
 tests/qemuxml2argvdata/disk-vhostuser.xml     |  2 +-
 .../disk-vhostuser.x86_64-latest.xml          |  2 +-
 4 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
index 222a9386f6..6b3e892332 100644
--- a/src/conf/domain_validate.c
+++ b/src/conf/domain_validate.c
@@ -322,26 +322,6 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef *disk)
 
     /* Unsupported driver elements */
 
-    if (disk->virtio) {
-        if (disk->virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("iommu is not supported with vhostuser disk"));
-            return -1;
-        }
-
-        if (disk->virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("ats is not supported with vhostuser disk"));
-            return -1;
-        }
-
-        if (disk->virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("packed is not supported with vhostuser disk"));
-            return -1;
-        }
-    }
-
     if (disk->src->metadataCacheMaxSize > 0) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("metadata_cache is not supported with vhostuser disk"));
diff --git a/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args b/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
index b24b2c0b4f..30acd7c78b 100644
--- a/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
+++ b/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
@@ -33,8 +33,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
 -device vhost-user-blk-pci,bus=pci.0,addr=0x2,chardev=chr-vu-virtio-disk0,\
 id=virtio-disk0,bootindex=1 \
 -chardev socket,id=chr-vu-virtio-disk1,path=/tmp/vhost1.sock,reconnect=10 \
--device vhost-user-blk-pci,bus=pci.0,addr=0x3,chardev=chr-vu-virtio-disk1,\
-id=virtio-disk1 \
+-device vhost-user-blk-pci,iommu_platform=on,ats=on,packed=on,bus=pci.0,\
+addr=0x3,chardev=chr-vu-virtio-disk1,id=virtio-disk1 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
 -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
 resourcecontrol=deny \
diff --git a/tests/qemuxml2argvdata/disk-vhostuser.xml b/tests/qemuxml2argvdata/disk-vhostuser.xml
index c96ef9119c..cbe2804e39 100644
--- a/tests/qemuxml2argvdata/disk-vhostuser.xml
+++ b/tests/qemuxml2argvdata/disk-vhostuser.xml
@@ -20,7 +20,7 @@
       <boot order='1'/>
     </disk>
     <disk type='vhostuser' device='disk'>
-      <driver name='qemu' type='raw'/>
+      <driver name='qemu' type='raw' iommu='on' ats='on' packed='on'/>
       <source type='unix' path='/tmp/vhost1.sock'>
         <reconnect enabled='yes' timeout='10'/>
       </source>
diff --git a/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml b/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
index 9712dc0b12..87f5ec46ac 100644
--- a/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
+++ b/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
@@ -28,7 +28,7 @@
       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
     </disk>
     <disk type='vhostuser' device='disk' snapshot='no'>
-      <driver name='qemu' type='raw'/>
+      <driver name='qemu' type='raw' iommu='on' ats='on' packed='on'/>
       <source type='unix' path='/tmp/vhost1.sock'>
         <reconnect enabled='yes' timeout='10'/>
       </source>
-- 
2.29.2

Re: [libvirt PATCH] conf: allow virtio driver attributes for vhostuser disk
Posted by Han Han 3 years, 1 month ago
On Fri, Feb 5, 2021 at 8:08 PM Pavel Hrdina <phrdina@redhat.com> wrote:

> All of these options are actually supported by vhostuser disk so
> we should allow them to be usable.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/conf/domain_validate.c                    | 20 -------------------
>  .../disk-vhostuser.x86_64-latest.args         |  4 ++--
>  tests/qemuxml2argvdata/disk-vhostuser.xml     |  2 +-
>  .../disk-vhostuser.x86_64-latest.xml          |  2 +-
>  4 files changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> index 222a9386f6..6b3e892332 100644
> --- a/src/conf/domain_validate.c
> +++ b/src/conf/domain_validate.c
> @@ -322,26 +322,6 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef
> *disk)
>
>      /* Unsupported driver elements */
>
> -    if (disk->virtio) {
> -        if (disk->virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("iommu is not supported with vhostuser
> disk"));
> -            return -1;
> -        }
> -
> -        if (disk->virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("ats is not supported with vhostuser disk"));
> -            return -1;
> -        }
> -
> -        if (disk->virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("packed is not supported with vhostuser
> disk"));
> -            return -1;
> -        }
> -    }
> -
>
 Tested on qemu-5.2.0-5.fc34.1.x86_64 libvirt v7.0.0-337-ga58edc602e
VM could be start with the virtio properties of vhostuser-blk:
<disk type="vhostuser" device="disk" model="virtio-non-transitional"
snapshot="no">
      <driver name="qemu" type="raw" iommu="on" ats="on" packed="on"/>
      <source type="unix" path="/tmp/vhost-user-blk0.sock">
        <reconnect enabled="yes" timeout="10"/>
      </source>
      <target dev="vdb" bus="virtio"/>
      <address type="pci" domain="0x0000" bus="0x00" slot="0x0b"
function="0x0"/>
    </disk>

Note that the model="virtio-non-transitional" should be set otherwise it
will report an error:
2021-02-07T05:33:25.737469Z qemu-system-x86_64: -device
vhost-user-blk-pci,iommu_platform=on,ats=on,packed=on,bus=pci.0,addr=0xb,chardev=chr-vu-virtio-disk1,id=virtio-disk1:
VIRTIO_F_IOMMU_P
LATFORM was supported by neither legacy nor transitional device

>      if (disk->src->metadataCacheMaxSize > 0) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                         _("metadata_cache is not supported with vhostuser
> disk"));
> diff --git a/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
> b/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
> index b24b2c0b4f..30acd7c78b 100644
> --- a/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
> +++ b/tests/qemuxml2argvdata/disk-vhostuser.x86_64-latest.args
> @@ -33,8 +33,8 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
>  -device
> vhost-user-blk-pci,bus=pci.0,addr=0x2,chardev=chr-vu-virtio-disk0,\
>  id=virtio-disk0,bootindex=1 \
>  -chardev socket,id=chr-vu-virtio-disk1,path=/tmp/vhost1.sock,reconnect=10
> \
> --device
> vhost-user-blk-pci,bus=pci.0,addr=0x3,chardev=chr-vu-virtio-disk1,\
> -id=virtio-disk1 \
> +-device vhost-user-blk-pci,iommu_platform=on,ats=on,packed=on,bus=pci.0,\
> +addr=0x3,chardev=chr-vu-virtio-disk1,id=virtio-disk1 \
>  -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 \
>  -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\
>  resourcecontrol=deny \
> diff --git a/tests/qemuxml2argvdata/disk-vhostuser.xml
> b/tests/qemuxml2argvdata/disk-vhostuser.xml
> index c96ef9119c..cbe2804e39 100644
> --- a/tests/qemuxml2argvdata/disk-vhostuser.xml
> +++ b/tests/qemuxml2argvdata/disk-vhostuser.xml
> @@ -20,7 +20,7 @@
>        <boot order='1'/>
>      </disk>
>      <disk type='vhostuser' device='disk'>
> -      <driver name='qemu' type='raw'/>
> +      <driver name='qemu' type='raw' iommu='on' ats='on' packed='on'/>
>        <source type='unix' path='/tmp/vhost1.sock'>
>          <reconnect enabled='yes' timeout='10'/>
>        </source>
> diff --git a/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
> b/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
> index 9712dc0b12..87f5ec46ac 100644
> --- a/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
> +++ b/tests/qemuxml2xmloutdata/disk-vhostuser.x86_64-latest.xml
> @@ -28,7 +28,7 @@
>        <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> function='0x0'/>
>      </disk>
>      <disk type='vhostuser' device='disk' snapshot='no'>
> -      <driver name='qemu' type='raw'/>
> +      <driver name='qemu' type='raw' iommu='on' ats='on' packed='on'/>
>        <source type='unix' path='/tmp/vhost1.sock'>
>          <reconnect enabled='yes' timeout='10'/>
>        </source>
> --
> 2.29.2
>
>

-- 
Reviewed-by: Han Han <hhan@redhat.com>
Tested-by: Han Han <hhan@redhat.com>
Re: [libvirt PATCH] conf: allow virtio driver attributes for vhostuser disk
Posted by Pavel Hrdina 3 years, 1 month ago
On Sun, Feb 07, 2021 at 01:42:33PM +0800, Han Han wrote:
> On Fri, Feb 5, 2021 at 8:08 PM Pavel Hrdina <phrdina@redhat.com> wrote:
> 
> > All of these options are actually supported by vhostuser disk so
> > we should allow them to be usable.
> >
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/conf/domain_validate.c                    | 20 -------------------
> >  .../disk-vhostuser.x86_64-latest.args         |  4 ++--
> >  tests/qemuxml2argvdata/disk-vhostuser.xml     |  2 +-
> >  .../disk-vhostuser.x86_64-latest.xml          |  2 +-
> >  4 files changed, 4 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c
> > index 222a9386f6..6b3e892332 100644
> > --- a/src/conf/domain_validate.c
> > +++ b/src/conf/domain_validate.c
> > @@ -322,26 +322,6 @@ virDomainDiskVhostUserValidate(const virDomainDiskDef
> > *disk)
> >
> >      /* Unsupported driver elements */
> >
> > -    if (disk->virtio) {
> > -        if (disk->virtio->iommu != VIR_TRISTATE_SWITCH_ABSENT) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                           _("iommu is not supported with vhostuser
> > disk"));
> > -            return -1;
> > -        }
> > -
> > -        if (disk->virtio->ats != VIR_TRISTATE_SWITCH_ABSENT) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                           _("ats is not supported with vhostuser disk"));
> > -            return -1;
> > -        }
> > -
> > -        if (disk->virtio->packed != VIR_TRISTATE_SWITCH_ABSENT) {
> > -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > -                           _("packed is not supported with vhostuser
> > disk"));
> > -            return -1;
> > -        }
> > -    }
> > -
> >
>  Tested on qemu-5.2.0-5.fc34.1.x86_64 libvirt v7.0.0-337-ga58edc602e
> VM could be start with the virtio properties of vhostuser-blk:
> <disk type="vhostuser" device="disk" model="virtio-non-transitional"
> snapshot="no">
>       <driver name="qemu" type="raw" iommu="on" ats="on" packed="on"/>
>       <source type="unix" path="/tmp/vhost-user-blk0.sock">
>         <reconnect enabled="yes" timeout="10"/>
>       </source>
>       <target dev="vdb" bus="virtio"/>
>       <address type="pci" domain="0x0000" bus="0x00" slot="0x0b"
> function="0x0"/>
>     </disk>
> 
> Note that the model="virtio-non-transitional" should be set otherwise it
> will report an error:
> 2021-02-07T05:33:25.737469Z qemu-system-x86_64: -device
> vhost-user-blk-pci,iommu_platform=on,ats=on,packed=on,bus=pci.0,addr=0xb,chardev=chr-vu-virtio-disk1,id=virtio-disk1:
> VIRTIO_F_IOMMU_P
> LATFORM was supported by neither legacy nor transitional device

This is not exclusive to vhostuser disk. The same happens with disk
type='file' and my guess is that it will happen to other
virtio/vhost-user devices. This is a separate issue that would be
probably nice to fix but I guess it's not critical because QEMU gives
reasonable error message.

Thanks for testing.

Pavel
Re: [libvirt PATCH] conf: allow virtio driver attributes for vhostuser disk
Posted by Peter Krempa 3 years, 1 month ago
On Fri, Feb 05, 2021 at 13:07:41 +0100, Pavel Hrdina wrote:
> All of these options are actually supported by vhostuser disk so
> we should allow them to be usable.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/conf/domain_validate.c                    | 20 -------------------
>  .../disk-vhostuser.x86_64-latest.args         |  4 ++--
>  tests/qemuxml2argvdata/disk-vhostuser.xml     |  2 +-
>  .../disk-vhostuser.x86_64-latest.xml          |  2 +-
>  4 files changed, 4 insertions(+), 24 deletions(-)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>