diff mbox series

[v2,14/16] qemu: build vhost-user GPU devices

Message ID 98d3ca86997a6171090d1a6cc544fcafc31305b7.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>

For each vhost-user GPUs,
- build a socket chardev, and pass the vhost-user socket to it
- build a vhost-user video device and associate it with the chardev

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

Comments

Ján Tomko Aug. 27, 2019, 9:17 a.m. UTC | #1 
On Fri, Aug 23, 2019 at 12:21:58PM -0400, Cole Robinson wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>

>

>For each vhost-user GPUs,

>- build a socket chardev, and pass the vhost-user socket to it

>- build a vhost-user video device and associate it with the chardev

>

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

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

>---

> src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++++--

> 1 file changed, 44 insertions(+), 2 deletions(-)

>

>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

>index 8bef103f68..0e1d9510e5 100644

>--- a/src/qemu/qemu_command.c

>+++ b/src/qemu/qemu_command.c

>@@ -4672,8 +4672,15 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,

>         goto error;

>     }

>

>-    if (STREQ(model, "virtio-gpu")) {

>-        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,


Eww, why do we do a string comparison here when we store the model as an
enum?

>+    if (video->vhostuser) {

>+        if (STREQ(model, "virtio-vga"))

>+            model = "vhost-user-vga";

>+        if (STREQ(model, "virtio-gpu"))

>+            model = "vhost-user-gpu";

>+    }


Not sure why we need to reassign model here instead of getting it right
the first time.

>+


How different is the vhost-user-gpu device from virtio-gpu,
don't we new a new VIDEO_TYPE_VHOST_USER instead?


if (video->type == VIRTIO && !primary) for the condition below:

>+    if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) {

>+        if (qemuBuildVirtioDevStr(&buf, model, qemuCaps,

>                                   VIR_DOMAIN_DEVICE_VIDEO, video) < 0) {

>             goto error;

>         }


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

On Tue, Aug 27, 2019 at 1:17 PM Ján Tomko <jtomko@redhat.com> wrote:
>
> On Fri, Aug 23, 2019 at 12:21:58PM -0400, Cole Robinson wrote:
> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >For each vhost-user GPUs,
> >- build a socket chardev, and pass the vhost-user socket to it
> >- build a vhost-user video device and associate it with the chardev
> >
> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >Signed-off-by: Cole Robinson <crobinso@redhat.com>
> >---
> > src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 44 insertions(+), 2 deletions(-)
> >
> >diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> >index 8bef103f68..0e1d9510e5 100644
> >--- a/src/qemu/qemu_command.c
> >+++ b/src/qemu/qemu_command.c
> >@@ -4672,8 +4672,15 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
> >         goto error;
> >     }
> >
> >-    if (STREQ(model, "virtio-gpu")) {
> >-        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,
>
> Eww, why do we do a string comparison here when we store the model as an
> enum?

Yup, pre-existing. I suppose that can be fixed.

>
> >+    if (video->vhostuser) {
> >+        if (STREQ(model, "virtio-vga"))
> >+            model = "vhost-user-vga";
> >+        if (STREQ(model, "virtio-gpu"))
> >+            model = "vhost-user-gpu";
> >+    }
>
> Not sure why we need to reassign model here instead of getting it right
> the first time.
>
> >+
>
> How different is the vhost-user-gpu device from virtio-gpu,
> don't we new a new VIDEO_TYPE_VHOST_USER instead?

>From guest PoV, it's actually the same device. So we shouldn't
introduce a new virDomainVideoType, right? But we need a mapping to
-device, it has to be in libvirt qemu driver.


>
>
> if (video->type == VIRTIO && !primary) for the condition below:
>
> >+    if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) {
> >+        if (qemuBuildVirtioDevStr(&buf, model, qemuCaps,
> >                                   VIR_DOMAIN_DEVICE_VIDEO, video) < 0) {
> >             goto error;
> >         }
>
> Jano
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Aug. 29, 2019, 2:13 p.m. UTC | #3 
On 8/29/19 8:43 AM, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 27, 2019 at 1:17 PM Ján Tomko <jtomko@redhat.com> wrote:
>>
>> On Fri, Aug 23, 2019 at 12:21:58PM -0400, Cole Robinson wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> For each vhost-user GPUs,
>>> - build a socket chardev, and pass the vhost-user socket to it
>>> - build a vhost-user video device and associate it with the chardev
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>> src/qemu/qemu_command.c | 46 +++++++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>>> index 8bef103f68..0e1d9510e5 100644
>>> --- a/src/qemu/qemu_command.c
>>> +++ b/src/qemu/qemu_command.c
>>> @@ -4672,8 +4672,15 @@ qemuBuildDeviceVideoStr(const virDomainDef *def,
>>>         goto error;
>>>     }
>>>
>>> -    if (STREQ(model, "virtio-gpu")) {
>>> -        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,
>>
>> Eww, why do we do a string comparison here when we store the model as an
>> enum?
> 
> Yup, pre-existing. I suppose that can be fixed.
> 

virDomainVideoType only has VIRTIO and not virtio-gpu vs virtio-vga;
that distinction depends on a few different criteria like arch and
primary vs secondary. So video->type isn't a drop in replacement here

>>
>>> +    if (video->vhostuser) {
>>> +        if (STREQ(model, "virtio-vga"))
>>> +            model = "vhost-user-vga";
>>> +        if (STREQ(model, "virtio-gpu"))
>>> +            model = "vhost-user-gpu";
>>> +    }
>>
>> Not sure why we need to reassign model here instead of getting it right
>> the first time.
>>
>>> +
>>
>> How different is the vhost-user-gpu device from virtio-gpu,
>> don't we new a new VIDEO_TYPE_VHOST_USER instead?
> 
> From guest PoV, it's actually the same device. So we shouldn't
> introduce a new virDomainVideoType, right? But we need a mapping to
> -device, it has to be in libvirt qemu driver.
> 

I agree that existing model='virtio' should cover it

- Cole

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

Patch

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 8bef103f68..0e1d9510e5 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4672,8 +4672,15 @@  qemuBuildDeviceVideoStr(const virDomainDef *def,
         goto error;
     }
 
-    if (STREQ(model, "virtio-gpu")) {
-        if (qemuBuildVirtioDevStr(&buf, "virtio-gpu", qemuCaps,
+    if (video->vhostuser) {
+        if (STREQ(model, "virtio-vga"))
+            model = "vhost-user-vga";
+        if (STREQ(model, "virtio-gpu"))
+            model = "vhost-user-gpu";
+    }
+
+    if (STREQ(model, "virtio-gpu") || STREQ(model, "vhost-user-gpu")) {
+        if (qemuBuildVirtioDevStr(&buf, model, qemuCaps,
                                   VIR_DOMAIN_DEVICE_VIDEO, video) < 0) {
             goto error;
         }
@@ -4715,6 +4722,10 @@  qemuBuildDeviceVideoStr(const virDomainDef *def,
             if (video->heads)
                 virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
         }
+    } else if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && video->vhostuser) {
+        if (video->heads)
+            virBufferAsprintf(&buf, ",max_outputs=%u", video->heads);
+        virBufferAsprintf(&buf, ",chardev=chr-vu-%s", video->info.alias);
     } else if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO) {
         if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS)) {
             if (video->heads)
@@ -4830,6 +4841,23 @@  qemuBuildVgaVideoCommand(virCommandPtr cmd,
 }
 
 
+static char *
+qemuBuildVhostUserChardevStr(const char *alias,
+                             int *fd,
+                             virCommandPtr cmd)
+{
+    char *chardev = NULL;
+
+    if (virAsprintf(&chardev, "socket,id=chr-vu-%s,fd=%d", alias, *fd) < 0)
+        return NULL;
+
+    virCommandPassFD(cmd, *fd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);
+    *fd = -1;
+
+    return chardev;
+}
+
+
 static int
 qemuBuildVideoCommandLine(virCommandPtr cmd,
                           const virDomainDef *def,
@@ -4837,6 +4865,20 @@  qemuBuildVideoCommandLine(virCommandPtr cmd,
 {
     size_t i;
 
+    for (i = 0; i < def->nvideos; i++) {
+        VIR_AUTOFREE(char *) chardev = NULL;
+        virDomainVideoDefPtr video = def->videos[i];
+
+        if (video->type == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && video->vhostuser) {
+            if (!(chardev = qemuBuildVhostUserChardevStr(video->info.alias,
+                                                         &video->info.vhost_user_fd,
+                                                         cmd)))
+                return -1;
+
+            virCommandAddArgList(cmd, "-chardev", chardev, NULL);
+        }
+    }
+
     for (i = 0; i < def->nvideos; i++) {
         char *str = NULL;
         virDomainVideoDefPtr video = def->videos[i];