diff mbox series

[v2,02/16] domain: add "vhostuser" attribute to virtio video model

Message ID 76dadadc00b9c519e4b25f321776347672781cb9.1566576129.git.crobinso@redhat.com
State New
Headers show
Series Add vhost-user-gpu support | expand

Commit Message

Cole Robinson Aug. 23, 2019, 4:21 p.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Accept a new attribute to specify usage of helper process, ex:

  <video>
    <model type='virtio' vhostuser='yes'/>
  </video>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 docs/formatdomain.html.in     |  6 ++++++
 docs/schemas/domaincommon.rng | 11 ++++++++++-
 src/conf/domain_conf.c        | 14 ++++++++++++++
 src/conf/domain_conf.h        |  1 +
 4 files changed, 31 insertions(+), 1 deletion(-)

Comments

Cole Robinson Aug. 23, 2019, 4:51 p.m. UTC | #1 
On 8/23/19 12:21 PM, Cole Robinson wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Accept a new attribute to specify usage of helper process, ex:
> 
>   <video>
>     <model type='virtio' vhostuser='yes'/>
>   </video>
> 

For other devices, we have

<interface type='vhostuser'>
<interface><driver name='vhost'/>
<hostdev type='scsi_host'><source protocol='vhost'>

Which is an attempt to make this more generic. IMO using vhostuser='yes'
and is the simplest match to qemu terminology but maybe other people
have stronger opinions.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  docs/formatdomain.html.in     |  6 ++++++
>  docs/schemas/domaincommon.rng | 11 ++++++++++-
>  src/conf/domain_conf.c        | 14 ++++++++++++++
>  src/conf/domain_conf.h        |  1 +
>  4 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index fcb7c59c00..ec650fbe17 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7039,6 +7039,12 @@ qemu-kvm -net nic,model=? /dev/null
>            Attribute <code>vram64</code> (<span class="since">since 1.3.3</span>)
>            extends secondary bar and makes it addressable as 64bit memory.
>          </p>
> +        <p>
> +          For guest type "kvm" and model type "virtio" there are
> +          optional attributes. Attribute <code>vhost-user</code>
> +          (<span class="since">since 5.5.0</span>) specify that a
> +          vhost-user helper process should be associated with the GPU.
> +        </p>
>        </dd>
>  
>        <dt><code>acceleration</code></dt>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index c48f8c4f56..bac566855d 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3581,7 +3581,6 @@
>                  <value>vmvga</value>
>                  <value>xen</value>
>                  <value>vbox</value>
> -                <value>virtio</value>
>                  <value>gop</value>
>                  <value>none</value>
>                  <value>bochs</value>
> @@ -3607,6 +3606,16 @@
>                  </attribute>
>                </optional>
>              </group>
> +            <group>
> +              <attribute name="type">
> +                <value>virtio</value>
> +              </attribute>
> +              <optional>
> +                <attribute name="vhostuser">
> +                  <ref name="virYesNo"/>
> +                </attribute>
> +              </optional>
> +            </group>
>            </choice>
>            <optional>
>              <attribute name="vram">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b7a342bb91..f51575d57d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15358,6 +15358,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>      xmlNodePtr cur;
>      VIR_XPATH_NODE_AUTORESTORE(ctxt);
>      VIR_AUTOFREE(char *) type = NULL;
> +    VIR_AUTOFREE(char *) vhostuser = NULL;
>      VIR_AUTOFREE(char *) heads = NULL;
>      VIR_AUTOFREE(char *) vram = NULL;
>      VIR_AUTOFREE(char *) vram64 = NULL;
> @@ -15376,6 +15377,7 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>              if (!type && !vram && !ram && !heads &&
>                  virXMLNodeNameEqual(cur, "model")) {
>                  type = virXMLPropString(cur, "type");
> +                vhostuser = virXMLPropString(cur, "vhostuser");
>                  ram = virXMLPropString(cur, "ram");
>                  vram = virXMLPropString(cur, "vram");
>                  vram64 = virXMLPropString(cur, "vram64");
> @@ -15408,6 +15410,16 @@ virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
>          def->type = virDomainVideoDefaultType(dom);
>      }
>  
> +    if (vhostuser != NULL) {
> +        if (virStringParseYesNo(vhostuser, &def->vhostuser) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unknown vhostuser value '%s'"), vhostuser);
> +            goto cleanup;
> +        }
> +    } else {
> +        def->vhostuser = false;
> +    }
> +
>      if (ram) {
>          if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
>              virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -26486,6 +26498,8 @@ virDomainVideoDefFormat(virBufferPtr buf,
>          virBufferAsprintf(buf, " heads='%u'", def->heads);
>      if (def->primary)
>          virBufferAddLit(buf, " primary='yes'");
> +    if (def->vhostuser)
> +        virBufferAddLit(buf, " vhostuser='yes'");
>      if (def->accel) {
>          virBufferAddLit(buf, ">\n");
>          virBufferAdjustIndent(buf, 2);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 33cef5b75c..bc2450f25e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1424,6 +1424,7 @@ struct _virDomainVideoDef {
>      virDomainVideoDriverDefPtr driver;
>      virDomainDeviceInfo info;
>      virDomainVirtioOptionsPtr virtio;
> +    bool vhostuser;

I assume you followed the example of 'primary', but this should be a
virTristateBool. Follow 'accel3d' parsing/formatting as an example

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko Aug. 27, 2019, 7:20 a.m. UTC | #2 
On Fri, Aug 23, 2019 at 12:51:56PM -0400, Cole Robinson wrote:
>On 8/23/19 12:21 PM, Cole Robinson wrote:

>> From: Marc-André Lureau <marcandre.lureau@redhat.com>

>>

>> Accept a new attribute to specify usage of helper process, ex:

>>

>>   <video>

>>     <model type='virtio' vhostuser='yes'/>

>>   </video>

>>

>

>For other devices, we have

>

><interface type='vhostuser'>

><interface><driver name='vhost'/>

><hostdev type='scsi_host'><source protocol='vhost'>

>

>Which is an attempt to make this more generic.


>IMO using vhostuser='yes'


missing verb?

>and is the simplest match to qemu terminology but maybe other people

>have stronger opinions.


Yes.

This does not belong in the model - it's a backend thing.

<driver name='vhost'/> sounds best to me (and similarly to
what commit 175077fd707db6ad87d6e2a079e82bc290ac2421 did,
we can also expose <driver name='qemu'/>)

Jano

>

>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>>  docs/formatdomain.html.in     |  6 ++++++

>>  docs/schemas/domaincommon.rng | 11 ++++++++++-

>>  src/conf/domain_conf.c        | 14 ++++++++++++++

>>  src/conf/domain_conf.h        |  1 +

>>  4 files changed, 31 insertions(+), 1 deletion(-)

>>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko Aug. 27, 2019, 7:35 a.m. UTC | #3 
On Fri, Aug 23, 2019 at 12:21:46PM -0400, Cole Robinson wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>

>

>Accept a new attribute to specify usage of helper process, ex:

>

>  <video>

>    <model type='virtio' vhostuser='yes'/>

>  </video>

>

>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>Signed-off-by: Cole Robinson <crobinso@redhat.com>

>---

> docs/formatdomain.html.in     |  6 ++++++

> docs/schemas/domaincommon.rng | 11 ++++++++++-


Also, parser additions should include an addition to the XML test cases
(virschematest will automatically pick them up and test them against the
schema) and either genericxml2xmltest or qemuxml2xmltest.

Jano

> src/conf/domain_conf.c        | 14 ++++++++++++++

> src/conf/domain_conf.h        |  1 +

> 4 files changed, 31 insertions(+), 1 deletion(-)

>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Marc-André Lureau Aug. 29, 2019, 12:30 p.m. UTC | #4 
Hi

On Tue, Aug 27, 2019 at 11:19 AM Ján Tomko <jtomko@redhat.com> wrote:
>
> On Fri, Aug 23, 2019 at 12:51:56PM -0400, Cole Robinson wrote:
> >On 8/23/19 12:21 PM, Cole Robinson wrote:
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> Accept a new attribute to specify usage of helper process, ex:
> >>
> >>   <video>
> >>     <model type='virtio' vhostuser='yes'/>
> >>   </video>
> >>
> >
> >For other devices, we have
> >
> ><interface type='vhostuser'>
> ><interface><driver name='vhost'/>
> ><hostdev type='scsi_host'><source protocol='vhost'>
> >
> >Which is an attempt to make this more generic.
>
> >IMO using vhostuser='yes'
>
> missing verb?
>
> >and is the simplest match to qemu terminology but maybe other people
> >have stronger opinions.
>
> Yes.
>
> This does not belong in the model - it's a backend thing.
>
> <driver name='vhost'/> sounds best to me (and similarly to
> what commit 175077fd707db6ad87d6e2a079e82bc290ac2421 did,
> we can also expose <driver name='qemu'/>)

Shouldn't it be <driver name='vhost-user'/>?

('vhost' would be for a kernel backend)

>
> Jano
>
> >
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> >> ---
> >>  docs/formatdomain.html.in     |  6 ++++++
> >>  docs/schemas/domaincommon.rng | 11 ++++++++++-
> >>  src/conf/domain_conf.c        | 14 ++++++++++++++
> >>  src/conf/domain_conf.h        |  1 +
> >>  4 files changed, 31 insertions(+), 1 deletion(-)
> >>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Ján Tomko Aug. 29, 2019, 12:46 p.m. UTC | #5 
On Thu, Aug 29, 2019 at 04:30:29PM +0400, Marc-André Lureau wrote:
>Hi

>

>On Tue, Aug 27, 2019 at 11:19 AM Ján Tomko <jtomko@redhat.com> wrote:

>>

>> On Fri, Aug 23, 2019 at 12:51:56PM -0400, Cole Robinson wrote:

>> >On 8/23/19 12:21 PM, Cole Robinson wrote:

>> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>

>> >>

>> >> Accept a new attribute to specify usage of helper process, ex:

>> >>

>> >>   <video>

>> >>     <model type='virtio' vhostuser='yes'/>

>> >>   </video>

>> >>

>> >

>> >For other devices, we have

>> >

>> ><interface type='vhostuser'>

>> ><interface><driver name='vhost'/>

>> ><hostdev type='scsi_host'><source protocol='vhost'>

>> >

>> >Which is an attempt to make this more generic.

>>

>> >IMO using vhostuser='yes'

>>

>> missing verb?

>>

>> >and is the simplest match to qemu terminology but maybe other people

>> >have stronger opinions.

>>

>> Yes.

>>

>> This does not belong in the model - it's a backend thing.

>>

>> <driver name='vhost'/> sounds best to me (and similarly to

>> what commit 175077fd707db6ad87d6e2a079e82bc290ac2421 did,

>> we can also expose <driver name='qemu'/>)

>

>Shouldn't it be <driver name='vhost-user'/>?

>


Even better :)

Jano

>('vhost' would be for a kernel backend)

>

>>

>> Jano

>>

>> >
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
diff mbox series

Patch

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index fcb7c59c00..ec650fbe17 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7039,6 +7039,12 @@  qemu-kvm -net nic,model=? /dev/null
           Attribute <code>vram64</code> (<span class="since">since 1.3.3</span>)
           extends secondary bar and makes it addressable as 64bit memory.
         </p>
+        <p>
+          For guest type "kvm" and model type "virtio" there are
+          optional attributes. Attribute <code>vhost-user</code>
+          (<span class="since">since 5.5.0</span>) specify that a
+          vhost-user helper process should be associated with the GPU.
+        </p>
       </dd>
 
       <dt><code>acceleration</code></dt>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index c48f8c4f56..bac566855d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3581,7 +3581,6 @@ 
                 <value>vmvga</value>
                 <value>xen</value>
                 <value>vbox</value>
-                <value>virtio</value>
                 <value>gop</value>
                 <value>none</value>
                 <value>bochs</value>
@@ -3607,6 +3606,16 @@ 
                 </attribute>
               </optional>
             </group>
+            <group>
+              <attribute name="type">
+                <value>virtio</value>
+              </attribute>
+              <optional>
+                <attribute name="vhostuser">
+                  <ref name="virYesNo"/>
+                </attribute>
+              </optional>
+            </group>
           </choice>
           <optional>
             <attribute name="vram">
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b7a342bb91..f51575d57d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -15358,6 +15358,7 @@  virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
     xmlNodePtr cur;
     VIR_XPATH_NODE_AUTORESTORE(ctxt);
     VIR_AUTOFREE(char *) type = NULL;
+    VIR_AUTOFREE(char *) vhostuser = NULL;
     VIR_AUTOFREE(char *) heads = NULL;
     VIR_AUTOFREE(char *) vram = NULL;
     VIR_AUTOFREE(char *) vram64 = NULL;
@@ -15376,6 +15377,7 @@  virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
             if (!type && !vram && !ram && !heads &&
                 virXMLNodeNameEqual(cur, "model")) {
                 type = virXMLPropString(cur, "type");
+                vhostuser = virXMLPropString(cur, "vhostuser");
                 ram = virXMLPropString(cur, "ram");
                 vram = virXMLPropString(cur, "vram");
                 vram64 = virXMLPropString(cur, "vram64");
@@ -15408,6 +15410,16 @@  virDomainVideoDefParseXML(virDomainXMLOptionPtr xmlopt,
         def->type = virDomainVideoDefaultType(dom);
     }
 
+    if (vhostuser != NULL) {
+        if (virStringParseYesNo(vhostuser, &def->vhostuser) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("unknown vhostuser value '%s'"), vhostuser);
+            goto cleanup;
+        }
+    } else {
+        def->vhostuser = false;
+    }
+
     if (ram) {
         if (def->type != VIR_DOMAIN_VIDEO_TYPE_QXL) {
             virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -26486,6 +26498,8 @@  virDomainVideoDefFormat(virBufferPtr buf,
         virBufferAsprintf(buf, " heads='%u'", def->heads);
     if (def->primary)
         virBufferAddLit(buf, " primary='yes'");
+    if (def->vhostuser)
+        virBufferAddLit(buf, " vhostuser='yes'");
     if (def->accel) {
         virBufferAddLit(buf, ">\n");
         virBufferAdjustIndent(buf, 2);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 33cef5b75c..bc2450f25e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1424,6 +1424,7 @@  struct _virDomainVideoDef {
     virDomainVideoDriverDefPtr driver;
     virDomainDeviceInfo info;
     virDomainVirtioOptionsPtr virtio;
+    bool vhostuser;
 };
 
 /* graphics console modes */