[Qemu-devel] [PATCH 0/5] Misc vhost-user fixes

[Qemu-devel] [PATCH 0/5] Misc vhost-user fixes

[Marc-André Lureau]

Hi,

Here is a few fixes spotted by Coverity in the recently landed
vhost-user-{input,gpu}.

Marc-André Lureau (5):
  vhost-user-gpu: do not send scanout update if no GPU socket
  vhost-user: check unix_listen() return value
  vhost-user: improve error report
  vhost-user-input: check ioctl(EVIOCGNAME) return value
  vhost-user-gpu: initialize msghdr & iov at declaration

 contrib/vhost-user-gpu/main.c   | 29 ++++++++++++++---------------
 contrib/vhost-user-input/main.c | 12 ++++++++++--
 2 files changed, 24 insertions(+), 17 deletions(-)

-- 
2.22.0.rc2.384.g1a9a72ea1d

[Qemu-devel] [PATCH 1/5] vhost-user-gpu: do not send scanout update if no GPU socket

[Marc-André Lureau]

Should fix coverity CID 1401760.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/vhost-user-gpu/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index f9e2146b69..9614c9422c 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -354,7 +354,7 @@ vg_disable_scanout(VuGpu *g, int scanout_id)
     scanout->width = 0;
     scanout->height = 0;
 
-    {
+    if (g->sock_fd >= 0) {
         VhostUserGpuMsg msg = {
             .request = VHOST_USER_GPU_SCANOUT,
             .size = sizeof(VhostUserGpuScanout),
-- 
2.22.0.rc2.384.g1a9a72ea1d

[Qemu-devel] [PULL 01/11] vhost-user-gpu: do not send scanout update if no GPU socket

[Michael S. Tsirkin]

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

Should fix coverity CID 1401760.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605145829.7674-2-marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 contrib/vhost-user-gpu/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index f9e2146b69..9614c9422c 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -354,7 +354,7 @@ vg_disable_scanout(VuGpu *g, int scanout_id)
     scanout->width = 0;
     scanout->height = 0;
 
-    {
+    if (g->sock_fd >= 0) {
         VhostUserGpuMsg msg = {
             .request = VHOST_USER_GPU_SCANOUT,
             .size = sizeof(VhostUserGpuScanout),
-- 
MST

[Qemu-devel] [PATCH 2/5] vhost-user: check unix_listen() return value

[Marc-André Lureau]

This check shouldn't be necessary, since &error_fatal is given as
argument and will exit() on failure. However, this change should
silence coverity CID 1401761 & 1401705.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/vhost-user-gpu/main.c   | 4 ++++
 contrib/vhost-user-input/main.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index 9614c9422c..e0b6df5b4d 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -1160,6 +1160,10 @@ main(int argc, char *argv[])
 
     if (opt_socket_path) {
         int lsock = unix_listen(opt_socket_path, &error_fatal);
+        if (lsock < 0) {
+            g_printerr("Failed to listen on %s.\n", opt_socket_path);
+            exit(EXIT_FAILURE);
+        }
         fd = accept(lsock, NULL, NULL);
         close(lsock);
     } else {
diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 8d493f598e..8b854117f5 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -367,6 +367,10 @@ main(int argc, char *argv[])
 
     if (opt_socket_path) {
         int lsock = unix_listen(opt_socket_path, &error_fatal);
+        if (lsock < 0) {
+            g_printerr("Failed to listen on %s.\n", opt_socket_path);
+            exit(EXIT_FAILURE);
+        }
         fd = accept(lsock, NULL, NULL);
         close(lsock);
     } else {
-- 
2.22.0.rc2.384.g1a9a72ea1d

Re: [Qemu-devel] [PATCH 2/5] vhost-user: check unix_listen() return value

[Peter Maydell]

On Wed, 5 Jun 2019 at 16:02, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> This check shouldn't be necessary, since &error_fatal is given as
> argument and will exit() on failure. However, this change should
> silence coverity CID 1401761 & 1401705.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/vhost-user-gpu/main.c   | 4 ++++
>  contrib/vhost-user-input/main.c | 4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
> index 9614c9422c..e0b6df5b4d 100644
> --- a/contrib/vhost-user-gpu/main.c
> +++ b/contrib/vhost-user-gpu/main.c
> @@ -1160,6 +1160,10 @@ main(int argc, char *argv[])
>
>      if (opt_socket_path) {
>          int lsock = unix_listen(opt_socket_path, &error_fatal);
> +        if (lsock < 0) {
> +            g_printerr("Failed to listen on %s.\n", opt_socket_path);
> +            exit(EXIT_FAILURE);
> +        }
>          fd = accept(lsock, NULL, NULL);
>          close(lsock);
>      } else {
> diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
> index 8d493f598e..8b854117f5 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -367,6 +367,10 @@ main(int argc, char *argv[])
>
>      if (opt_socket_path) {
>          int lsock = unix_listen(opt_socket_path, &error_fatal);
> +        if (lsock < 0) {
> +            g_printerr("Failed to listen on %s.\n", opt_socket_path);
> +            exit(EXIT_FAILURE);
> +        }
>          fd = accept(lsock, NULL, NULL);
>          close(lsock);
>      } else {
> --
> 2.22.0.rc2.384.g1a9a72ea1d
>

Maybe better to just assert(lsock >= 0) ?
(I hadn't noticed the &error_fatal -- it means this new code
is unreachable.)

thanks
-- PMM

[Qemu-devel] [PULL 02/11] vhost-user: check unix_listen() return value

[Michael S. Tsirkin]

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

This check shouldn't be necessary, since &error_fatal is given as
argument and will exit() on failure. However, this change should
silence coverity CID 1401761 & 1401705.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605145829.7674-3-marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 contrib/vhost-user-gpu/main.c   | 4 ++++
 contrib/vhost-user-input/main.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index 9614c9422c..e0b6df5b4d 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -1160,6 +1160,10 @@ main(int argc, char *argv[])
 
     if (opt_socket_path) {
         int lsock = unix_listen(opt_socket_path, &error_fatal);
+        if (lsock < 0) {
+            g_printerr("Failed to listen on %s.\n", opt_socket_path);
+            exit(EXIT_FAILURE);
+        }
         fd = accept(lsock, NULL, NULL);
         close(lsock);
     } else {
diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 8d493f598e..8b854117f5 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -367,6 +367,10 @@ main(int argc, char *argv[])
 
     if (opt_socket_path) {
         int lsock = unix_listen(opt_socket_path, &error_fatal);
+        if (lsock < 0) {
+            g_printerr("Failed to listen on %s.\n", opt_socket_path);
+            exit(EXIT_FAILURE);
+        }
         fd = accept(lsock, NULL, NULL);
         close(lsock);
     } else {
-- 
MST

Re: [Qemu-devel] [PULL 02/11] vhost-user: check unix_listen() return value

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1476 bytes --]

On 6/16/19 4:36 PM, Michael S. Tsirkin wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This check shouldn't be necessary, since &error_fatal is given as
> argument and will exit() on failure. However, this change should
> silence coverity CID 1401761 & 1401705.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Message-Id: <20190605145829.7674-3-marcandre.lureau@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  contrib/vhost-user-gpu/main.c   | 4 ++++
>  contrib/vhost-user-input/main.c | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
> index 9614c9422c..e0b6df5b4d 100644
> --- a/contrib/vhost-user-gpu/main.c
> +++ b/contrib/vhost-user-gpu/main.c
> @@ -1160,6 +1160,10 @@ main(int argc, char *argv[])
>  
>      if (opt_socket_path) {
>          int lsock = unix_listen(opt_socket_path, &error_fatal);
> +        if (lsock < 0) {
> +            g_printerr("Failed to listen on %s.\n", opt_socket_path);
> +            exit(EXIT_FAILURE);
> +        }

4 lines to appease Coverity; wouldn't the following one-liner also do
the trick?

int lsock = unix_listen(opt_socket_path, &error_fatal);
assert (lsock >= 0);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PULL 02/11] vhost-user: check unix_listen() return value

[Marc-André Lureau]

Hi

On Wed, Jun 26, 2019 at 7:56 PM Eric Blake <eblake@redhat.com> wrote:
>
> On 6/16/19 4:36 PM, Michael S. Tsirkin wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This check shouldn't be necessary, since &error_fatal is given as
> > argument and will exit() on failure. However, this change should
> > silence coverity CID 1401761 & 1401705.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Message-Id: <20190605145829.7674-3-marcandre.lureau@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  contrib/vhost-user-gpu/main.c   | 4 ++++
> >  contrib/vhost-user-input/main.c | 4 ++++
> >  2 files changed, 8 insertions(+)
> >
> > diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
> > index 9614c9422c..e0b6df5b4d 100644
> > --- a/contrib/vhost-user-gpu/main.c
> > +++ b/contrib/vhost-user-gpu/main.c
> > @@ -1160,6 +1160,10 @@ main(int argc, char *argv[])
> >
> >      if (opt_socket_path) {
> >          int lsock = unix_listen(opt_socket_path, &error_fatal);
> > +        if (lsock < 0) {
> > +            g_printerr("Failed to listen on %s.\n", opt_socket_path);
> > +            exit(EXIT_FAILURE);
> > +        }
>
> 4 lines to appease Coverity; wouldn't the following one-liner also do
> the trick?
>
> int lsock = unix_listen(opt_socket_path, &error_fatal);
> assert (lsock >= 0);

Probably, I didn't think too much about it. However looking at it now,
it would be worthwhile to report the error to the user in a friendlier
way than what error_fatal do. So I guess a follow-up patch should
introduce a local Error.

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
>


-- 
Marc-André Lureau

[Qemu-devel] [PATCH 3/5] vhost-user: improve error report

[Marc-André Lureau]

g_printerr() needs a trailing \n

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/vhost-user-gpu/main.c   | 2 +-
 contrib/vhost-user-input/main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index e0b6df5b4d..0ef649ffaa 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -1170,7 +1170,7 @@ main(int argc, char *argv[])
         fd = opt_fdnum;
     }
     if (fd == -1) {
-        g_printerr("Invalid socket");
+        g_printerr("Invalid vhost-user socket.\n");
         exit(EXIT_FAILURE);
     }
 
diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 8b854117f5..54f882602a 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -377,7 +377,7 @@ main(int argc, char *argv[])
         fd = opt_fdnum;
     }
     if (fd == -1) {
-        g_printerr("Invalid socket");
+        g_printerr("Invalid vhost-user socket.\n");
         exit(EXIT_FAILURE);
     }
     vug_init(&vi.dev, fd, vi_panic, &vuiface);
-- 
2.22.0.rc2.384.g1a9a72ea1d

[Qemu-devel] [PULL 03/11] vhost-user: improve error report

[Michael S. Tsirkin]

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

g_printerr() needs a trailing \n

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605145829.7674-4-marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 contrib/vhost-user-gpu/main.c   | 2 +-
 contrib/vhost-user-input/main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index e0b6df5b4d..0ef649ffaa 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -1170,7 +1170,7 @@ main(int argc, char *argv[])
         fd = opt_fdnum;
     }
     if (fd == -1) {
-        g_printerr("Invalid socket");
+        g_printerr("Invalid vhost-user socket.\n");
         exit(EXIT_FAILURE);
     }
 
diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 8b854117f5..54f882602a 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -377,7 +377,7 @@ main(int argc, char *argv[])
         fd = opt_fdnum;
     }
     if (fd == -1) {
-        g_printerr("Invalid socket");
+        g_printerr("Invalid vhost-user socket.\n");
         exit(EXIT_FAILURE);
     }
     vug_init(&vi.dev, fd, vi_panic, &vuiface);
-- 
MST

[Qemu-devel] [PATCH 4/5] vhost-user-input: check ioctl(EVIOCGNAME) return value

[Marc-André Lureau]

This should fix coverity CID 1401704.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/vhost-user-input/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 54f882602a..8b4e7d2536 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -342,7 +342,11 @@ main(int argc, char *argv[])
 
     vi.config = g_array_new(false, false, sizeof(virtio_input_config));
     memset(&id, 0, sizeof(id));
-    ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1), id.u.string);
+    if (ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1),
+              id.u.string) < 0) {
+        g_printerr("Failed to get evdev name: %s\n", g_strerror(errno));
+        exit(EXIT_FAILURE);
+    }
     id.select = VIRTIO_INPUT_CFG_ID_NAME;
     id.size = strlen(id.u.string);
     g_array_append_val(vi.config, id);
-- 
2.22.0.rc2.384.g1a9a72ea1d

Re: [Qemu-devel] [PATCH 4/5] vhost-user-input: check ioctl(EVIOCGNAME) return value

[Peter Maydell]

On Wed, 5 Jun 2019 at 16:04, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> This should fix coverity CID 1401704.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/vhost-user-input/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
> index 54f882602a..8b4e7d2536 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -342,7 +342,11 @@ main(int argc, char *argv[])
>
>      vi.config = g_array_new(false, false, sizeof(virtio_input_config));
>      memset(&id, 0, sizeof(id));
> -    ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1), id.u.string);
> +    if (ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1),
> +              id.u.string) < 0) {
> +        g_printerr("Failed to get evdev name: %s\n", g_strerror(errno));
> +        exit(EXIT_FAILURE);
> +    }


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PULL 04/11] vhost-user-input: check ioctl(EVIOCGNAME) return value

[Michael S. Tsirkin]

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

This should fix coverity CID 1401704.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605145829.7674-5-marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 contrib/vhost-user-input/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 54f882602a..8b4e7d2536 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -342,7 +342,11 @@ main(int argc, char *argv[])
 
     vi.config = g_array_new(false, false, sizeof(virtio_input_config));
     memset(&id, 0, sizeof(id));
-    ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1), id.u.string);
+    if (ioctl(vi.evdevfd, EVIOCGNAME(sizeof(id.u.string) - 1),
+              id.u.string) < 0) {
+        g_printerr("Failed to get evdev name: %s\n", g_strerror(errno));
+        exit(EXIT_FAILURE);
+    }
     id.select = VIRTIO_INPUT_CFG_ID_NAME;
     id.size = strlen(id.u.string);
     g_array_append_val(vi.config, id);
-- 
MST

[Qemu-devel] [PATCH 5/5] vhost-user-gpu: initialize msghdr & iov at declaration

[Marc-André Lureau]

This should fix uninitialized fields found by coverity CID 1401762.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 contrib/vhost-user-gpu/main.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index 0ef649ffaa..04b753046f 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -138,22 +138,20 @@ static int
 vg_sock_fd_write(int sock, const void *buf, ssize_t buflen, int fd)
 {
     ssize_t ret;
-    struct msghdr msg;
-    struct iovec iov;
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = buflen,
+    };
+    struct msghdr msg = {
+        .msg_iov = &iov,
+        .msg_iovlen = 1,
+    };
     union {
         struct cmsghdr cmsghdr;
         char control[CMSG_SPACE(sizeof(int))];
     } cmsgu;
     struct cmsghdr *cmsg;
 
-    iov.iov_base = (void *)buf;
-    iov.iov_len = buflen;
-
-    msg.msg_name = NULL;
-    msg.msg_namelen = 0;
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-
     if (fd != -1) {
         msg.msg_control = cmsgu.control;
         msg.msg_controllen = sizeof(cmsgu.control);
@@ -164,9 +162,6 @@ vg_sock_fd_write(int sock, const void *buf, ssize_t buflen, int fd)
         cmsg->cmsg_type = SCM_RIGHTS;
 
         *((int *)CMSG_DATA(cmsg)) = fd;
-    } else {
-        msg.msg_control = NULL;
-        msg.msg_controllen = 0;
     }
 
     do {
-- 
2.22.0.rc2.384.g1a9a72ea1d

Re: [Qemu-devel] [PATCH 5/5] vhost-user-gpu: initialize msghdr & iov at declaration

[Peter Maydell]

On Wed, 5 Jun 2019 at 16:01, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> This should fix uninitialized fields found by coverity CID 1401762.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/vhost-user-gpu/main.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PULL 05/11] vhost-user-gpu: initialize msghdr & iov at declaration

[Michael S. Tsirkin]

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

This should fix uninitialized fields found by coverity CID 1401762.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605145829.7674-6-marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 contrib/vhost-user-gpu/main.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/contrib/vhost-user-gpu/main.c b/contrib/vhost-user-gpu/main.c
index 0ef649ffaa..04b753046f 100644
--- a/contrib/vhost-user-gpu/main.c
+++ b/contrib/vhost-user-gpu/main.c
@@ -138,22 +138,20 @@ static int
 vg_sock_fd_write(int sock, const void *buf, ssize_t buflen, int fd)
 {
     ssize_t ret;
-    struct msghdr msg;
-    struct iovec iov;
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = buflen,
+    };
+    struct msghdr msg = {
+        .msg_iov = &iov,
+        .msg_iovlen = 1,
+    };
     union {
         struct cmsghdr cmsghdr;
         char control[CMSG_SPACE(sizeof(int))];
     } cmsgu;
     struct cmsghdr *cmsg;
 
-    iov.iov_base = (void *)buf;
-    iov.iov_len = buflen;
-
-    msg.msg_name = NULL;
-    msg.msg_namelen = 0;
-    msg.msg_iov = &iov;
-    msg.msg_iovlen = 1;
-
     if (fd != -1) {
         msg.msg_control = cmsgu.control;
         msg.msg_controllen = sizeof(cmsgu.control);
@@ -164,9 +162,6 @@ vg_sock_fd_write(int sock, const void *buf, ssize_t buflen, int fd)
         cmsg->cmsg_type = SCM_RIGHTS;
 
         *((int *)CMSG_DATA(cmsg)) = fd;
-    } else {
-        msg.msg_control = NULL;
-        msg.msg_controllen = 0;
     }
 
     do {
-- 
MST

[Qemu-devel] [PULL 00/11] virtio, acpi: fixes, cleanups

[Michael S. Tsirkin]

The following changes since commit f3d0bec9f80e4ed7796fffa834ba0a53f2094f7f:

  Merge remote-tracking branch 'remotes/maxreitz/tags/pull-block-2019-06-14' into staging (2019-06-14 14:46:13 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 5f6b3561bf58395fd6c906d7064a1a5693a2e426:

  tests/rebuild-expected-aml.sh: blow out difflist (2019-06-16 16:44:44 -0400)

----------------------------------------------------------------
virtio, acpi: fixes, cleanups

A bunch of minor fixes all over the place.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Gerd Hoffmann (1):
      q35: fix mmconfig and PCI0._CRS

Li Hangjing (1):
      vhost: fix vhost_log size overflow during migration

Marc-André Lureau (6):
      vhost-user-gpu: do not send scanout update if no GPU socket
      vhost-user: check unix_listen() return value
      vhost-user: improve error report
      vhost-user-input: check ioctl(EVIOCGNAME) return value
      vhost-user-gpu: initialize msghdr & iov at declaration
      docs/vhost-user.json: some firmware.json copy leftovers

Michael S. Tsirkin (2):
      q35: update DSDT
      tests/rebuild-expected-aml.sh: blow out difflist

Wei Yang (1):
      hw/acpi: extract acpi_add_rom_blob()

 docs/interop/vhost-user.json            |   6 ++---
 include/hw/acpi/utils.h                 |   9 +++++++
 contrib/vhost-user-gpu/main.c           |  29 +++++++++++------------
 contrib/vhost-user-input/main.c         |  12 ++++++++--
 hw/acpi/utils.c                         |  35 ++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c                |  26 ++++++++-------------
 hw/i386/acpi-build.c                    |  40 +++++++++++++++++++-------------
 hw/pci-host/q35.c                       |  31 +++++++------------------
 hw/virtio/vhost.c                       |  10 ++++++++
 hw/acpi/Makefile.objs                   |   2 +-
 tests/data/acpi/q35/DSDT                | Bin 7815 -> 7841 bytes
 tests/data/acpi/q35/DSDT.bridge         | Bin 7832 -> 7858 bytes
 tests/data/acpi/q35/DSDT.cphp           | Bin 8278 -> 8304 bytes
 tests/data/acpi/q35/DSDT.dimmpxm        | Bin 9468 -> 9494 bytes
 tests/data/acpi/q35/DSDT.ipmibt         | Bin 7890 -> 7916 bytes
 tests/data/acpi/q35/DSDT.memhp          | Bin 9174 -> 9200 bytes
 tests/data/acpi/q35/DSDT.mmio64         | Bin 8945 -> 8971 bytes
 tests/data/acpi/q35/DSDT.numamem        | Bin 7821 -> 7847 bytes
 tests/data/acpi/rebuild-expected-aml.sh |   3 +++
 19 files changed, 127 insertions(+), 76 deletions(-)
 create mode 100644 include/hw/acpi/utils.h
 create mode 100644 hw/acpi/utils.c

[Qemu-devel] [PATCH] vhost: fix vhost_log size overflow during migration

[lihangjing]

From: Li Hangjing <lihangjing@baidu.com>

When a guest which doesn't support multiqueue is migrated with a multi queues
vhost-user-blk deivce, a crash will occur like:

0 qemu_memfd_alloc (name=<value optimized out>, size=562949953421312, seals=<value optimized out>, fd=0x7f87171fe8b4, errp=0x7f87171fe8a8) at util/memfd.c:153
1 0x00007f883559d7cf in vhost_log_alloc (size=70368744177664, share=true) at hw/virtio/vhost.c:186
2 0x00007f88355a0758 in vhost_log_get (listener=0x7f8838bd7940, enable=1) at qemu-2-12/hw/virtio/vhost.c:211
3 vhost_dev_log_resize (listener=0x7f8838bd7940, enable=1) at hw/virtio/vhost.c:263
4 vhost_migration_log (listener=0x7f8838bd7940, enable=1) at hw/virtio/vhost.c:787
5 0x00007f88355463d6 in memory_global_dirty_log_start () at memory.c:2503
6 0x00007f8835550577 in ram_init_bitmaps (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2173
7 ram_init_all (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2192
8 ram_save_setup (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2219
9 0x00007f88357a419d in qemu_savevm_state_setup (f=0x7f88384ce600) at migration/savevm.c:1002
10 0x00007f883579fc3e in migration_thread (opaque=0x7f8837530400) at migration/migration.c:2382
11 0x00007f8832447893 in start_thread () from /lib64/libpthread.so.0
12 0x00007f8832178bfd in clone () from /lib64/libc.so.6

This is because vhost_get_log_size() returns a overflowed vhost-log size.
In this function, it uses the uninitialized variable vqs->used_phys and
vqs->used_size to get the vhost-log size.

Signed-off-by: Li Hangjing <lihangjing@baidu.com>
Reviewed-by: Xie Yongji <xieyongji@baidu.com>
Reviewed-by: Chai Wen <chaiwen@baidu.com>
---
 hw/virtio/vhost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7f61018f2a..6d3a013f49 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -131,6 +131,11 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
     }
     for (i = 0; i < dev->nvqs; ++i) {
         struct vhost_virtqueue *vq = dev->vqs + i;
+
+        if (!vq->used_phys && !vq->used_size) {
+            continue;
+        }
+
         vhost_dev_sync_region(dev, section, start_addr, end_addr, vq->used_phys,
                               range_get_last(vq->used_phys, vq->used_size));
     }
@@ -168,6 +173,11 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
     }
     for (i = 0; i < dev->nvqs; ++i) {
         struct vhost_virtqueue *vq = dev->vqs + i;
+
+        if (!vq->used_phys && !vq->used_size) {
+            continue;
+        }
+
         uint64_t last = vq->used_phys + vq->used_size - 1;
         log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
     }
-- 
2.15.1.windows.2

[Qemu-devel] [PULL 07/11] vhost: fix vhost_log size overflow during migration

[Michael S. Tsirkin]

From: Li Hangjing <lihangjing@baidu.com>

When a guest which doesn't support multiqueue is migrated with a multi queues
vhost-user-blk deivce, a crash will occur like:

0 qemu_memfd_alloc (name=<value optimized out>, size=562949953421312, seals=<value optimized out>, fd=0x7f87171fe8b4, errp=0x7f87171fe8a8) at util/memfd.c:153
1 0x00007f883559d7cf in vhost_log_alloc (size=70368744177664, share=true) at hw/virtio/vhost.c:186
2 0x00007f88355a0758 in vhost_log_get (listener=0x7f8838bd7940, enable=1) at qemu-2-12/hw/virtio/vhost.c:211
3 vhost_dev_log_resize (listener=0x7f8838bd7940, enable=1) at hw/virtio/vhost.c:263
4 vhost_migration_log (listener=0x7f8838bd7940, enable=1) at hw/virtio/vhost.c:787
5 0x00007f88355463d6 in memory_global_dirty_log_start () at memory.c:2503
6 0x00007f8835550577 in ram_init_bitmaps (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2173
7 ram_init_all (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2192
8 ram_save_setup (f=0x7f88384ce600, opaque=0x7f8836024098) at migration/ram.c:2219
9 0x00007f88357a419d in qemu_savevm_state_setup (f=0x7f88384ce600) at migration/savevm.c:1002
10 0x00007f883579fc3e in migration_thread (opaque=0x7f8837530400) at migration/migration.c:2382
11 0x00007f8832447893 in start_thread () from /lib64/libpthread.so.0
12 0x00007f8832178bfd in clone () from /lib64/libc.so.6

This is because vhost_get_log_size() returns a overflowed vhost-log size.
In this function, it uses the uninitialized variable vqs->used_phys and
vqs->used_size to get the vhost-log size.

Signed-off-by: Li Hangjing <lihangjing@baidu.com>
Reviewed-by: Xie Yongji <xieyongji@baidu.com>
Reviewed-by: Chai Wen <chaiwen@baidu.com>
Message-Id: <20190603061524.24076-1-lihangjing@baidu.com>
Cc: qemu-stable@nongnu.org
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/vhost.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 60747a6f93..bc899fc60e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -131,6 +131,11 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
     }
     for (i = 0; i < dev->nvqs; ++i) {
         struct vhost_virtqueue *vq = dev->vqs + i;
+
+        if (!vq->used_phys && !vq->used_size) {
+            continue;
+        }
+
         vhost_dev_sync_region(dev, section, start_addr, end_addr, vq->used_phys,
                               range_get_last(vq->used_phys, vq->used_size));
     }
@@ -168,6 +173,11 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
     }
     for (i = 0; i < dev->nvqs; ++i) {
         struct vhost_virtqueue *vq = dev->vqs + i;
+
+        if (!vq->used_phys && !vq->used_size) {
+            continue;
+        }
+
         uint64_t last = vq->used_phys + vq->used_size - 1;
         log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
     }
-- 
MST

[Qemu-devel] [PATCH] docs/vhost-user.json: some firmware.json copy leftovers

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 docs/interop/vhost-user.json | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
index ae88c03117..da6aaf51c8 100644
--- a/docs/interop/vhost-user.json
+++ b/docs/interop/vhost-user.json
@@ -178,11 +178,11 @@
 #
 #   - /usr/share/qemu/vhost-user/50-crosvm-gpu.json
 #
-# then the sysadmin can prevent the default QEMU being used at all with
+# then the sysadmin can prevent the default QEMU GPU being used at all with
 #
 #   $ touch /etc/qemu/vhost-user/50-qemu-gpu.json
 #
-# The sysadmin can replace/alter the distro default OVMF with
+# The sysadmin can replace/alter the distro default QEMU GPU with
 #
 #   $ vim /etc/qemu/vhost-user/50-qemu-gpu.json
 #
@@ -190,7 +190,7 @@
 #
 #   $ vim /etc/qemu/vhost-user/10-qemu-gpu.json
 #
-# or they can provide a parallel OVMF with lower priority
+# or they can provide a parallel QEMU GPU with lower priority
 #
 #   $ vim /etc/qemu/vhost-user/99-qemu-gpu.json
 #
-- 
2.22.0.rc2.384.g1a9a72ea1d

Re: [Qemu-devel] [PATCH] docs/vhost-user.json: some firmware.json copy leftovers

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

On Wed, Jun 05, 2019 at 03:12:21PM +0200, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/interop/vhost-user.json | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PULL 06/11] docs/vhost-user.json: some firmware.json copy leftovers

[Michael S. Tsirkin]

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

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20190605131221.29432-1-marcandre.lureau@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 docs/interop/vhost-user.json | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
index ae88c03117..da6aaf51c8 100644
--- a/docs/interop/vhost-user.json
+++ b/docs/interop/vhost-user.json
@@ -178,11 +178,11 @@
 #
 #   - /usr/share/qemu/vhost-user/50-crosvm-gpu.json
 #
-# then the sysadmin can prevent the default QEMU being used at all with
+# then the sysadmin can prevent the default QEMU GPU being used at all with
 #
 #   $ touch /etc/qemu/vhost-user/50-qemu-gpu.json
 #
-# The sysadmin can replace/alter the distro default OVMF with
+# The sysadmin can replace/alter the distro default QEMU GPU with
 #
 #   $ vim /etc/qemu/vhost-user/50-qemu-gpu.json
 #
@@ -190,7 +190,7 @@
 #
 #   $ vim /etc/qemu/vhost-user/10-qemu-gpu.json
 #
-# or they can provide a parallel OVMF with lower priority
+# or they can provide a parallel QEMU GPU with lower priority
 #
 #   $ vim /etc/qemu/vhost-user/99-qemu-gpu.json
 #
-- 
MST

Re: [Qemu-devel] [Qemu-trivial] [PATCH] docs/vhost-user.json: some firmware.json copy leftovers

[Laurent Vivier]

Le 05/06/2019 à 15:12, Marc-André Lureau a écrit :
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/interop/vhost-user.json | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
> index ae88c03117..da6aaf51c8 100644
> --- a/docs/interop/vhost-user.json
> +++ b/docs/interop/vhost-user.json
> @@ -178,11 +178,11 @@
>  #
>  #   - /usr/share/qemu/vhost-user/50-crosvm-gpu.json
>  #
> -# then the sysadmin can prevent the default QEMU being used at all with
> +# then the sysadmin can prevent the default QEMU GPU being used at all with
>  #
>  #   $ touch /etc/qemu/vhost-user/50-qemu-gpu.json
>  #
> -# The sysadmin can replace/alter the distro default OVMF with
> +# The sysadmin can replace/alter the distro default QEMU GPU with
>  #
>  #   $ vim /etc/qemu/vhost-user/50-qemu-gpu.json
>  #
> @@ -190,7 +190,7 @@
>  #
>  #   $ vim /etc/qemu/vhost-user/10-qemu-gpu.json
>  #
> -# or they can provide a parallel OVMF with lower priority
> +# or they can provide a parallel QEMU GPU with lower priority
>  #
>  #   $ vim /etc/qemu/vhost-user/99-qemu-gpu.json
>  #
> 

Applied to my trivial-patches branch.

Thanks,
Laurent

Re: [Qemu-devel] [Qemu-trivial] [PATCH] docs/vhost-user.json: some firmware.json copy leftovers

[Laurent Vivier]

Le 26/06/2019 à 18:24, Laurent Vivier a écrit :
> Le 05/06/2019 à 15:12, Marc-André Lureau a écrit :
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  docs/interop/vhost-user.json | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/vhost-user.json b/docs/interop/vhost-user.json
>> index ae88c03117..da6aaf51c8 100644
>> --- a/docs/interop/vhost-user.json
>> +++ b/docs/interop/vhost-user.json
>> @@ -178,11 +178,11 @@
>>  #
>>  #   - /usr/share/qemu/vhost-user/50-crosvm-gpu.json
>>  #
>> -# then the sysadmin can prevent the default QEMU being used at all with
>> +# then the sysadmin can prevent the default QEMU GPU being used at all with
>>  #
>>  #   $ touch /etc/qemu/vhost-user/50-qemu-gpu.json
>>  #
>> -# The sysadmin can replace/alter the distro default OVMF with
>> +# The sysadmin can replace/alter the distro default QEMU GPU with
>>  #
>>  #   $ vim /etc/qemu/vhost-user/50-qemu-gpu.json
>>  #
>> @@ -190,7 +190,7 @@
>>  #
>>  #   $ vim /etc/qemu/vhost-user/10-qemu-gpu.json
>>  #
>> -# or they can provide a parallel OVMF with lower priority
>> +# or they can provide a parallel QEMU GPU with lower priority
>>  #
>>  #   $ vim /etc/qemu/vhost-user/99-qemu-gpu.json
>>  #
>>
> 
> Applied to my trivial-patches branch.

In fact, no, it has already been merged.

Thanks,
Laurent

[Qemu-devel] [PATCH v2] q35: fix mmconfig and PCI0._CRS

[Gerd Hoffmann]

This patch changes the handling of the mmconfig area.  Thanks to the
pci(e) expander devices we already have the logic to exclude address
ranges from PCI0._CRS.  We can simply add the mmconfig address range
to the list get it excluded as well.

With that in place we can go with a fixed pci hole which covers the
whole area from the end of (low) ram to the ioapic.

This will make the whole logic alot less fragile.  No matter where the
firmware places the mmconfig xbar, things should work correctly.  The
guest also gets a bit more PCI address space (seabios boot):

    # cat /proc/iomem
    [ ... ]
    7ffdd000-7fffffff : reserved
    80000000-afffffff : PCI Bus 0000:00            <<-- this is new
    b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
      b0000000-bfffffff : reserved
    c0000000-febfffff : PCI Bus 0000:00
      f8000000-fbffffff : 0000:00:01.0
    [ ... ]

So this is a guest visible change.

Cc: László Érsek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/bios-tables-test-allowed-diff.h |  8 +++++++
 hw/i386/acpi-build.c                  | 14 ++++++++++++
 hw/pci-host/q35.c                     | 31 +++++++--------------------
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
index dfb8523c8bf4..3bbd22c62a3b 100644
--- a/tests/bios-tables-test-allowed-diff.h
+++ b/tests/bios-tables-test-allowed-diff.h
@@ -1 +1,9 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT",
+"tests/data/acpi/q35/DSDT.bridge",
+"tests/data/acpi/q35/DSDT.mmio64",
+"tests/data/acpi/q35/DSDT.ipmibt",
+"tests/data/acpi/q35/DSDT.cphp",
+"tests/data/acpi/q35/DSDT.memhp",
+"tests/data/acpi/q35/DSDT.numamem",
+"tests/data/acpi/q35/DSDT.dimmpxm",
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 85dc1640bc67..8e4f26977619 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -122,6 +122,8 @@ typedef struct FwCfgTPMConfig {
     uint8_t tpmppi_version;
 } QEMU_PACKED FwCfgTPMConfig;
 
+static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
+
 static void init_common_fadt_data(Object *o, AcpiFadtData *data)
 {
     uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
@@ -1807,6 +1809,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     CrsRangeSet crs_range_set;
     PCMachineState *pcms = PC_MACHINE(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
+    AcpiMcfgInfo mcfg;
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *bus = NULL;
@@ -1921,6 +1924,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         }
     }
 
+    /*
+     * At this point crs_range_set has all the ranges used by pci
+     * busses *other* than PCI0.  These ranges will be excluded from
+     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
+     * too.
+     */
+    if (acpi_get_mcfg(&mcfg)) {
+        crs_range_insert(crs_range_set.mem_ranges,
+                         mcfg.base, mcfg.base + mcfg.size - 1);
+    }
+
     scope = aml_scope("\\_SB.PCI0");
     /* build PCI0._CRS */
     crs = aml_resource_template();
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 960939f5ed3e..72093320befe 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -258,15 +258,6 @@ static void q35_host_initfn(Object *obj)
     object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.address_space_io,
                              qdev_prop_allow_set_link_before_realize, 0, NULL);
-
-    /* Leave enough space for the biggest MCFG BAR */
-    /* TODO: this matches current bios behaviour, but
-     * it's not a power of two, which means an MTRR
-     * can't cover it exactly.
-     */
-    range_set_bounds(&s->mch.pci_hole,
-            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
-            IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
 static const TypeInfo q35_host_info = {
@@ -338,20 +329,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
     }
     addr = pciexbar & addr_mask;
     pcie_host_mmcfg_update(pehb, enable, addr, length);
-    /* Leave enough space for the MCFG BAR */
-    /*
-     * TODO: this matches current bios behaviour, but it's not a power of two,
-     * which means an MTRR can't cover it exactly.
-     */
-    if (enable) {
-        range_set_bounds(&mch->pci_hole,
-                         addr + length,
-                         IO_APIC_DEFAULT_ADDRESS - 1);
-    } else {
-        range_set_bounds(&mch->pci_hole,
-                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
-                         IO_APIC_DEFAULT_ADDRESS - 1);
-    }
 }
 
 /* PAM */
@@ -484,6 +461,14 @@ static void mch_update(MCHPCIState *mch)
     mch_update_pam(mch);
     mch_update_smram(mch);
     mch_update_ext_tseg_mbytes(mch);
+
+    /*
+     * pci hole goes from end-of-low-ram to io-apic.
+     * mmconfig will be excluded by the dsdt builder.
+     */
+    range_set_bounds(&mch->pci_hole,
+                     mch->below_4g_mem_size,
+                     IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
 static int mch_post_load(void *opaque, int version_id)
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2] q35: fix mmconfig and PCI0._CRS

[Laszlo Ersek]

On 06/07/19 09:34, Gerd Hoffmann wrote:
> This patch changes the handling of the mmconfig area.  Thanks to the
> pci(e) expander devices we already have the logic to exclude address
> ranges from PCI0._CRS.  We can simply add the mmconfig address range
> to the list get it excluded as well.
> 
> With that in place we can go with a fixed pci hole which covers the
> whole area from the end of (low) ram to the ioapic.
> 
> This will make the whole logic alot less fragile.  No matter where the
> firmware places the mmconfig xbar, things should work correctly.  The
> guest also gets a bit more PCI address space (seabios boot):
> 
>     # cat /proc/iomem
>     [ ... ]
>     7ffdd000-7fffffff : reserved
>     80000000-afffffff : PCI Bus 0000:00            <<-- this is new
>     b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
>       b0000000-bfffffff : reserved
>     c0000000-febfffff : PCI Bus 0000:00
>       f8000000-fbffffff : 0000:00:01.0
>     [ ... ]
> 
> So this is a guest visible change.
> 
> Cc: László Érsek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test-allowed-diff.h |  8 +++++++
>  hw/i386/acpi-build.c                  | 14 ++++++++++++
>  hw/pci-host/q35.c                     | 31 +++++++--------------------
>  3 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> index dfb8523c8bf4..3bbd22c62a3b 100644
> --- a/tests/bios-tables-test-allowed-diff.h
> +++ b/tests/bios-tables-test-allowed-diff.h
> @@ -1 +1,9 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT",
> +"tests/data/acpi/q35/DSDT.bridge",
> +"tests/data/acpi/q35/DSDT.mmio64",
> +"tests/data/acpi/q35/DSDT.ipmibt",
> +"tests/data/acpi/q35/DSDT.cphp",
> +"tests/data/acpi/q35/DSDT.memhp",
> +"tests/data/acpi/q35/DSDT.numamem",
> +"tests/data/acpi/q35/DSDT.dimmpxm",
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 85dc1640bc67..8e4f26977619 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -122,6 +122,8 @@ typedef struct FwCfgTPMConfig {
>      uint8_t tpmppi_version;
>  } QEMU_PACKED FwCfgTPMConfig;
>  
> +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
> +
>  static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>  {
>      uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> @@ -1807,6 +1809,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      CrsRangeSet crs_range_set;
>      PCMachineState *pcms = PC_MACHINE(machine);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> +    AcpiMcfgInfo mcfg;
>      uint32_t nr_mem = machine->ram_slots;
>      int root_bus_limit = 0xFF;
>      PCIBus *bus = NULL;
> @@ -1921,6 +1924,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          }
>      }
>  
> +    /*
> +     * At this point crs_range_set has all the ranges used by pci
> +     * busses *other* than PCI0.  These ranges will be excluded from
> +     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
> +     * too.
> +     */
> +    if (acpi_get_mcfg(&mcfg)) {
> +        crs_range_insert(crs_range_set.mem_ranges,
> +                         mcfg.base, mcfg.base + mcfg.size - 1);
> +    }
> +
>      scope = aml_scope("\\_SB.PCI0");
>      /* build PCI0._CRS */
>      crs = aml_resource_template();
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 960939f5ed3e..72093320befe 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -258,15 +258,6 @@ static void q35_host_initfn(Object *obj)
>      object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
>                               (Object **) &s->mch.address_space_io,
>                               qdev_prop_allow_set_link_before_realize, 0, NULL);
> -
> -    /* Leave enough space for the biggest MCFG BAR */
> -    /* TODO: this matches current bios behaviour, but
> -     * it's not a power of two, which means an MTRR
> -     * can't cover it exactly.
> -     */
> -    range_set_bounds(&s->mch.pci_hole,
> -            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
> -            IO_APIC_DEFAULT_ADDRESS - 1);
>  }
>  
>  static const TypeInfo q35_host_info = {
> @@ -338,20 +329,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>      }
>      addr = pciexbar & addr_mask;
>      pcie_host_mmcfg_update(pehb, enable, addr, length);
> -    /* Leave enough space for the MCFG BAR */
> -    /*
> -     * TODO: this matches current bios behaviour, but it's not a power of two,
> -     * which means an MTRR can't cover it exactly.
> -     */
> -    if (enable) {
> -        range_set_bounds(&mch->pci_hole,
> -                         addr + length,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    } else {
> -        range_set_bounds(&mch->pci_hole,
> -                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    }
>  }
>  
>  /* PAM */
> @@ -484,6 +461,14 @@ static void mch_update(MCHPCIState *mch)
>      mch_update_pam(mch);
>      mch_update_smram(mch);
>      mch_update_ext_tseg_mbytes(mch);
> +
> +    /*
> +     * pci hole goes from end-of-low-ram to io-apic.
> +     * mmconfig will be excluded by the dsdt builder.
> +     */
> +    range_set_bounds(&mch->pci_hole,
> +                     mch->below_4g_mem_size,
> +                     IO_APIC_DEFAULT_ADDRESS - 1);
>  }
>  
>  static int mch_post_load(void *opaque, int version_id)
> 

For other reviewers: the only change relative to v1 is the
"tests/bios-tables-test-allowed-diff.h" hunk, which is new in v2.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

Re: [Qemu-devel] [PATCH v2] q35: fix mmconfig and PCI0._CRS

[Marcel Apfelbaum]

On 6/7/19 10:34 AM, Gerd Hoffmann wrote:
> This patch changes the handling of the mmconfig area.  Thanks to the
> pci(e) expander devices we already have the logic to exclude address
> ranges from PCI0._CRS.  We can simply add the mmconfig address range
> to the list get it excluded as well.
>
> With that in place we can go with a fixed pci hole which covers the
> whole area from the end of (low) ram to the ioapic.
>
> This will make the whole logic alot less fragile.  No matter where the
> firmware places the mmconfig xbar, things should work correctly.  The
> guest also gets a bit more PCI address space (seabios boot):
>
>      # cat /proc/iomem
>      [ ... ]
>      7ffdd000-7fffffff : reserved
>      80000000-afffffff : PCI Bus 0000:00            <<-- this is new
>      b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
>        b0000000-bfffffff : reserved
>      c0000000-febfffff : PCI Bus 0000:00
>        f8000000-fbffffff : 0000:00:01.0
>      [ ... ]
>
> So this is a guest visible change.
>
> Cc: László Érsek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   tests/bios-tables-test-allowed-diff.h |  8 +++++++
>   hw/i386/acpi-build.c                  | 14 ++++++++++++
>   hw/pci-host/q35.c                     | 31 +++++++--------------------
>   3 files changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> index dfb8523c8bf4..3bbd22c62a3b 100644
> --- a/tests/bios-tables-test-allowed-diff.h
> +++ b/tests/bios-tables-test-allowed-diff.h
> @@ -1 +1,9 @@
>   /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT",
> +"tests/data/acpi/q35/DSDT.bridge",
> +"tests/data/acpi/q35/DSDT.mmio64",
> +"tests/data/acpi/q35/DSDT.ipmibt",
> +"tests/data/acpi/q35/DSDT.cphp",
> +"tests/data/acpi/q35/DSDT.memhp",
> +"tests/data/acpi/q35/DSDT.numamem",
> +"tests/data/acpi/q35/DSDT.dimmpxm",
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 85dc1640bc67..8e4f26977619 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -122,6 +122,8 @@ typedef struct FwCfgTPMConfig {
>       uint8_t tpmppi_version;
>   } QEMU_PACKED FwCfgTPMConfig;
>   
> +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
> +
>   static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>   {
>       uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> @@ -1807,6 +1809,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>       CrsRangeSet crs_range_set;
>       PCMachineState *pcms = PC_MACHINE(machine);
>       PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> +    AcpiMcfgInfo mcfg;
>       uint32_t nr_mem = machine->ram_slots;
>       int root_bus_limit = 0xFF;
>       PCIBus *bus = NULL;
> @@ -1921,6 +1924,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>           }
>       }
>   
> +    /*
> +     * At this point crs_range_set has all the ranges used by pci
> +     * busses *other* than PCI0.  These ranges will be excluded from
> +     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
> +     * too.
> +     */
> +    if (acpi_get_mcfg(&mcfg)) {
> +        crs_range_insert(crs_range_set.mem_ranges,
> +                         mcfg.base, mcfg.base + mcfg.size - 1);
> +    }
> +
>       scope = aml_scope("\\_SB.PCI0");
>       /* build PCI0._CRS */
>       crs = aml_resource_template();
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 960939f5ed3e..72093320befe 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -258,15 +258,6 @@ static void q35_host_initfn(Object *obj)
>       object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
>                                (Object **) &s->mch.address_space_io,
>                                qdev_prop_allow_set_link_before_realize, 0, NULL);
> -
> -    /* Leave enough space for the biggest MCFG BAR */
> -    /* TODO: this matches current bios behaviour, but
> -     * it's not a power of two, which means an MTRR
> -     * can't cover it exactly.
> -     */
> -    range_set_bounds(&s->mch.pci_hole,
> -            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
> -            IO_APIC_DEFAULT_ADDRESS - 1);
>   }
>   
>   static const TypeInfo q35_host_info = {
> @@ -338,20 +329,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>       }
>       addr = pciexbar & addr_mask;
>       pcie_host_mmcfg_update(pehb, enable, addr, length);
> -    /* Leave enough space for the MCFG BAR */
> -    /*
> -     * TODO: this matches current bios behaviour, but it's not a power of two,
> -     * which means an MTRR can't cover it exactly.
> -     */
> -    if (enable) {
> -        range_set_bounds(&mch->pci_hole,
> -                         addr + length,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    } else {
> -        range_set_bounds(&mch->pci_hole,
> -                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    }
>   }
>   
>   /* PAM */
> @@ -484,6 +461,14 @@ static void mch_update(MCHPCIState *mch)
>       mch_update_pam(mch);
>       mch_update_smram(mch);
>       mch_update_ext_tseg_mbytes(mch);
> +
> +    /*
> +     * pci hole goes from end-of-low-ram to io-apic.
> +     * mmconfig will be excluded by the dsdt builder.
> +     */
> +    range_set_bounds(&mch->pci_hole,
> +                     mch->below_4g_mem_size,
> +                     IO_APIC_DEFAULT_ADDRESS - 1);
>   }
>   
>   static int mch_post_load(void *opaque, int version_id)

Reviewed-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH v2] q35: fix mmconfig and PCI0._CRS

[Paolo Bonzini]

On 07/06/19 09:34, Gerd Hoffmann wrote:
> This patch changes the handling of the mmconfig area.  Thanks to the
> pci(e) expander devices we already have the logic to exclude address
> ranges from PCI0._CRS.  We can simply add the mmconfig address range
> to the list get it excluded as well.
> 
> With that in place we can go with a fixed pci hole which covers the
> whole area from the end of (low) ram to the ioapic.
> 
> This will make the whole logic alot less fragile.  No matter where the
> firmware places the mmconfig xbar, things should work correctly.  The
> guest also gets a bit more PCI address space (seabios boot):
> 
>     # cat /proc/iomem
>     [ ... ]
>     7ffdd000-7fffffff : reserved
>     80000000-afffffff : PCI Bus 0000:00            <<-- this is new
>     b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
>       b0000000-bfffffff : reserved
>     c0000000-febfffff : PCI Bus 0000:00
>       f8000000-fbffffff : 0000:00:01.0
>     [ ... ]
> 
> So this is a guest visible change.
> 
> Cc: László Érsek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/bios-tables-test-allowed-diff.h |  8 +++++++
>  hw/i386/acpi-build.c                  | 14 ++++++++++++
>  hw/pci-host/q35.c                     | 31 +++++++--------------------
>  3 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> index dfb8523c8bf4..3bbd22c62a3b 100644
> --- a/tests/bios-tables-test-allowed-diff.h
> +++ b/tests/bios-tables-test-allowed-diff.h
> @@ -1 +1,9 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/q35/DSDT",
> +"tests/data/acpi/q35/DSDT.bridge",
> +"tests/data/acpi/q35/DSDT.mmio64",
> +"tests/data/acpi/q35/DSDT.ipmibt",
> +"tests/data/acpi/q35/DSDT.cphp",
> +"tests/data/acpi/q35/DSDT.memhp",
> +"tests/data/acpi/q35/DSDT.numamem",
> +"tests/data/acpi/q35/DSDT.dimmpxm",
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 85dc1640bc67..8e4f26977619 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -122,6 +122,8 @@ typedef struct FwCfgTPMConfig {
>      uint8_t tpmppi_version;
>  } QEMU_PACKED FwCfgTPMConfig;
>  
> +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
> +
>  static void init_common_fadt_data(Object *o, AcpiFadtData *data)
>  {
>      uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> @@ -1807,6 +1809,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      CrsRangeSet crs_range_set;
>      PCMachineState *pcms = PC_MACHINE(machine);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> +    AcpiMcfgInfo mcfg;
>      uint32_t nr_mem = machine->ram_slots;
>      int root_bus_limit = 0xFF;
>      PCIBus *bus = NULL;
> @@ -1921,6 +1924,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          }
>      }
>  
> +    /*
> +     * At this point crs_range_set has all the ranges used by pci
> +     * busses *other* than PCI0.  These ranges will be excluded from
> +     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
> +     * too.
> +     */
> +    if (acpi_get_mcfg(&mcfg)) {
> +        crs_range_insert(crs_range_set.mem_ranges,
> +                         mcfg.base, mcfg.base + mcfg.size - 1);
> +    }
> +
>      scope = aml_scope("\\_SB.PCI0");
>      /* build PCI0._CRS */
>      crs = aml_resource_template();
> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> index 960939f5ed3e..72093320befe 100644
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c
> @@ -258,15 +258,6 @@ static void q35_host_initfn(Object *obj)
>      object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
>                               (Object **) &s->mch.address_space_io,
>                               qdev_prop_allow_set_link_before_realize, 0, NULL);
> -
> -    /* Leave enough space for the biggest MCFG BAR */
> -    /* TODO: this matches current bios behaviour, but
> -     * it's not a power of two, which means an MTRR
> -     * can't cover it exactly.
> -     */
> -    range_set_bounds(&s->mch.pci_hole,
> -            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
> -            IO_APIC_DEFAULT_ADDRESS - 1);
>  }
>  
>  static const TypeInfo q35_host_info = {
> @@ -338,20 +329,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
>      }
>      addr = pciexbar & addr_mask;
>      pcie_host_mmcfg_update(pehb, enable, addr, length);
> -    /* Leave enough space for the MCFG BAR */
> -    /*
> -     * TODO: this matches current bios behaviour, but it's not a power of two,
> -     * which means an MTRR can't cover it exactly.
> -     */
> -    if (enable) {
> -        range_set_bounds(&mch->pci_hole,
> -                         addr + length,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    } else {
> -        range_set_bounds(&mch->pci_hole,
> -                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
> -                         IO_APIC_DEFAULT_ADDRESS - 1);
> -    }
>  }
>  
>  /* PAM */
> @@ -484,6 +461,14 @@ static void mch_update(MCHPCIState *mch)
>      mch_update_pam(mch);
>      mch_update_smram(mch);
>      mch_update_ext_tseg_mbytes(mch);
> +
> +    /*
> +     * pci hole goes from end-of-low-ram to io-apic.
> +     * mmconfig will be excluded by the dsdt builder.
> +     */
> +    range_set_bounds(&mch->pci_hole,
> +                     mch->below_4g_mem_size,
> +                     IO_APIC_DEFAULT_ADDRESS - 1);
>  }
>  
>  static int mch_post_load(void *opaque, int version_id)
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH v2] q35: fix mmconfig and PCI0._CRS

[Michael S. Tsirkin]

On Tue, Jun 11, 2019 at 01:37:35PM +0200, Paolo Bonzini wrote:
> On 07/06/19 09:34, Gerd Hoffmann wrote:
> > This patch changes the handling of the mmconfig area.  Thanks to the
> > pci(e) expander devices we already have the logic to exclude address
> > ranges from PCI0._CRS.  We can simply add the mmconfig address range
> > to the list get it excluded as well.
> > 
> > With that in place we can go with a fixed pci hole which covers the
> > whole area from the end of (low) ram to the ioapic.
> > 
> > This will make the whole logic alot less fragile.  No matter where the
> > firmware places the mmconfig xbar, things should work correctly.  The
> > guest also gets a bit more PCI address space (seabios boot):
> > 
> >     # cat /proc/iomem
> >     [ ... ]
> >     7ffdd000-7fffffff : reserved
> >     80000000-afffffff : PCI Bus 0000:00            <<-- this is new
> >     b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
> >       b0000000-bfffffff : reserved
> >     c0000000-febfffff : PCI Bus 0000:00
> >       f8000000-fbffffff : 0000:00:01.0
> >     [ ... ]
> > 
> > So this is a guest visible change.
> > 
> > Cc: László Érsek <lersek@redhat.com>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  tests/bios-tables-test-allowed-diff.h |  8 +++++++
> >  hw/i386/acpi-build.c                  | 14 ++++++++++++
> >  hw/pci-host/q35.c                     | 31 +++++++--------------------
> >  3 files changed, 30 insertions(+), 23 deletions(-)
> > 
> > diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> > index dfb8523c8bf4..3bbd22c62a3b 100644
> > --- a/tests/bios-tables-test-allowed-diff.h
> > +++ b/tests/bios-tables-test-allowed-diff.h
> > @@ -1 +1,9 @@
> >  /* List of comma-separated changed AML files to ignore */
> > +"tests/data/acpi/q35/DSDT",
> > +"tests/data/acpi/q35/DSDT.bridge",
> > +"tests/data/acpi/q35/DSDT.mmio64",
> > +"tests/data/acpi/q35/DSDT.ipmibt",
> > +"tests/data/acpi/q35/DSDT.cphp",
> > +"tests/data/acpi/q35/DSDT.memhp",
> > +"tests/data/acpi/q35/DSDT.numamem",
> > +"tests/data/acpi/q35/DSDT.dimmpxm",
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 85dc1640bc67..8e4f26977619 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -122,6 +122,8 @@ typedef struct FwCfgTPMConfig {
> >      uint8_t tpmppi_version;
> >  } QEMU_PACKED FwCfgTPMConfig;
> >  
> > +static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
> > +
> >  static void init_common_fadt_data(Object *o, AcpiFadtData *data)
> >  {
> >      uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
> > @@ -1807,6 +1809,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      CrsRangeSet crs_range_set;
> >      PCMachineState *pcms = PC_MACHINE(machine);
> >      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
> > +    AcpiMcfgInfo mcfg;
> >      uint32_t nr_mem = machine->ram_slots;
> >      int root_bus_limit = 0xFF;
> >      PCIBus *bus = NULL;
> > @@ -1921,6 +1924,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          }
> >      }
> >  
> > +    /*
> > +     * At this point crs_range_set has all the ranges used by pci
> > +     * busses *other* than PCI0.  These ranges will be excluded from
> > +     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
> > +     * too.
> > +     */
> > +    if (acpi_get_mcfg(&mcfg)) {
> > +        crs_range_insert(crs_range_set.mem_ranges,
> > +                         mcfg.base, mcfg.base + mcfg.size - 1);
> > +    }
> > +
> >      scope = aml_scope("\\_SB.PCI0");
> >      /* build PCI0._CRS */
> >      crs = aml_resource_template();
> > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> > index 960939f5ed3e..72093320befe 100644
> > --- a/hw/pci-host/q35.c
> > +++ b/hw/pci-host/q35.c
> > @@ -258,15 +258,6 @@ static void q35_host_initfn(Object *obj)
> >      object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
> >                               (Object **) &s->mch.address_space_io,
> >                               qdev_prop_allow_set_link_before_realize, 0, NULL);
> > -
> > -    /* Leave enough space for the biggest MCFG BAR */
> > -    /* TODO: this matches current bios behaviour, but
> > -     * it's not a power of two, which means an MTRR
> > -     * can't cover it exactly.
> > -     */
> > -    range_set_bounds(&s->mch.pci_hole,
> > -            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
> > -            IO_APIC_DEFAULT_ADDRESS - 1);
> >  }
> >  
> >  static const TypeInfo q35_host_info = {
> > @@ -338,20 +329,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
> >      }
> >      addr = pciexbar & addr_mask;
> >      pcie_host_mmcfg_update(pehb, enable, addr, length);
> > -    /* Leave enough space for the MCFG BAR */
> > -    /*
> > -     * TODO: this matches current bios behaviour, but it's not a power of two,
> > -     * which means an MTRR can't cover it exactly.
> > -     */
> > -    if (enable) {
> > -        range_set_bounds(&mch->pci_hole,
> > -                         addr + length,
> > -                         IO_APIC_DEFAULT_ADDRESS - 1);
> > -    } else {
> > -        range_set_bounds(&mch->pci_hole,
> > -                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
> > -                         IO_APIC_DEFAULT_ADDRESS - 1);
> > -    }
> >  }
> >  
> >  /* PAM */
> > @@ -484,6 +461,14 @@ static void mch_update(MCHPCIState *mch)
> >      mch_update_pam(mch);
> >      mch_update_smram(mch);
> >      mch_update_ext_tseg_mbytes(mch);
> > +
> > +    /*
> > +     * pci hole goes from end-of-low-ram to io-apic.
> > +     * mmconfig will be excluded by the dsdt builder.
> > +     */
> > +    range_set_bounds(&mch->pci_hole,
> > +                     mch->below_4g_mem_size,
> > +                     IO_APIC_DEFAULT_ADDRESS - 1);
> >  }
> >  
> >  static int mch_post_load(void *opaque, int version_id)
> > 
> 
> Queued, thanks.
> 
> Paolo

Paolo I would prefer to queue ACPI changes through my tree.
In particular there's a dance with expected tables I am
still working out.

-- 
MST

[Qemu-devel] [PULL 09/11] q35: fix mmconfig and PCI0._CRS

[Michael S. Tsirkin]

From: Gerd Hoffmann <kraxel@redhat.com>

This patch changes the handling of the mmconfig area.  Thanks to the
pci(e) expander devices we already have the logic to exclude address
ranges from PCI0._CRS.  We can simply add the mmconfig address range
to the list get it excluded as well.

With that in place we can go with a fixed pci hole which covers the
whole area from the end of (low) ram to the ioapic.

This will make the whole logic alot less fragile.  No matter where the
firmware places the mmconfig xbar, things should work correctly.  The
guest also gets a bit more PCI address space (seabios boot):

    # cat /proc/iomem
    [ ... ]
    7ffdd000-7fffffff : reserved
    80000000-afffffff : PCI Bus 0000:00            <<-- this is new
    b0000000-bfffffff : PCI MMCONFIG 0000 [bus 00-ff]
      b0000000-bfffffff : reserved
    c0000000-febfffff : PCI Bus 0000:00
      f8000000-fbffffff : 0000:00:01.0
    [ ... ]

So this is a guest visible change.

Cc: László Érsek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20190607073429.3436-1-kraxel@redhat.com>
---
 tests/bios-tables-test-allowed-diff.h |  8 +++++++
 hw/i386/acpi-build.c                  | 14 ++++++++++++
 hw/pci-host/q35.c                     | 31 +++++++--------------------
 3 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
index dfb8523c8b..3bbd22c62a 100644
--- a/tests/bios-tables-test-allowed-diff.h
+++ b/tests/bios-tables-test-allowed-diff.h
@@ -1 +1,9 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/DSDT",
+"tests/data/acpi/q35/DSDT.bridge",
+"tests/data/acpi/q35/DSDT.mmio64",
+"tests/data/acpi/q35/DSDT.ipmibt",
+"tests/data/acpi/q35/DSDT.cphp",
+"tests/data/acpi/q35/DSDT.memhp",
+"tests/data/acpi/q35/DSDT.numamem",
+"tests/data/acpi/q35/DSDT.dimmpxm",
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f2db7c1e22..31a1c1e3ad 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -121,6 +121,8 @@ typedef struct FwCfgTPMConfig {
     uint8_t tpmppi_version;
 } QEMU_PACKED FwCfgTPMConfig;
 
+static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
+
 static void init_common_fadt_data(Object *o, AcpiFadtData *data)
 {
     uint32_t io = object_property_get_uint(o, ACPI_PM_PROP_PM_IO_BASE, NULL);
@@ -1806,6 +1808,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     CrsRangeSet crs_range_set;
     PCMachineState *pcms = PC_MACHINE(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
+    AcpiMcfgInfo mcfg;
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *bus = NULL;
@@ -1920,6 +1923,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         }
     }
 
+    /*
+     * At this point crs_range_set has all the ranges used by pci
+     * busses *other* than PCI0.  These ranges will be excluded from
+     * the PCI0._CRS.  Add mmconfig to the set so it will be excluded
+     * too.
+     */
+    if (acpi_get_mcfg(&mcfg)) {
+        crs_range_insert(crs_range_set.mem_ranges,
+                         mcfg.base, mcfg.base + mcfg.size - 1);
+    }
+
     scope = aml_scope("\\_SB.PCI0");
     /* build PCI0._CRS */
     crs = aml_resource_template();
diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
index 172b0bc435..0a010be4cf 100644
--- a/hw/pci-host/q35.c
+++ b/hw/pci-host/q35.c
@@ -260,15 +260,6 @@ static void q35_host_initfn(Object *obj)
     object_property_add_link(obj, MCH_HOST_PROP_IO_MEM, TYPE_MEMORY_REGION,
                              (Object **) &s->mch.address_space_io,
                              qdev_prop_allow_set_link_before_realize, 0, NULL);
-
-    /* Leave enough space for the biggest MCFG BAR */
-    /* TODO: this matches current bios behaviour, but
-     * it's not a power of two, which means an MTRR
-     * can't cover it exactly.
-     */
-    range_set_bounds(&s->mch.pci_hole,
-            MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX,
-            IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
 static const TypeInfo q35_host_info = {
@@ -340,20 +331,6 @@ static void mch_update_pciexbar(MCHPCIState *mch)
     }
     addr = pciexbar & addr_mask;
     pcie_host_mmcfg_update(pehb, enable, addr, length);
-    /* Leave enough space for the MCFG BAR */
-    /*
-     * TODO: this matches current bios behaviour, but it's not a power of two,
-     * which means an MTRR can't cover it exactly.
-     */
-    if (enable) {
-        range_set_bounds(&mch->pci_hole,
-                         addr + length,
-                         IO_APIC_DEFAULT_ADDRESS - 1);
-    } else {
-        range_set_bounds(&mch->pci_hole,
-                         MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT,
-                         IO_APIC_DEFAULT_ADDRESS - 1);
-    }
 }
 
 /* PAM */
@@ -486,6 +463,14 @@ static void mch_update(MCHPCIState *mch)
     mch_update_pam(mch);
     mch_update_smram(mch);
     mch_update_ext_tseg_mbytes(mch);
+
+    /*
+     * pci hole goes from end-of-low-ram to io-apic.
+     * mmconfig will be excluded by the dsdt builder.
+     */
+    range_set_bounds(&mch->pci_hole,
+                     mch->below_4g_mem_size,
+                     IO_APIC_DEFAULT_ADDRESS - 1);
 }
 
 static int mch_post_load(void *opaque, int version_id)
-- 
MST

[Qemu-devel] [PATCH v7] hw/acpi: extract acpi_add_rom_blob()

[Wei Yang]

arm and i386 has almost the same function acpi_add_rom_blob(), except
giving different FWCfgCallback function.

This patch moves acpi_add_rom_blob() to utils.c by passing
FWCfgCallback to it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

v7:
  * rebase on top of current master because of conflict
v6:
  * change author from Igor to Michael
v5:
  * remove unnecessary header glib/gprintf.h
  * rearrange include header to make it more suitable
v4:
  * extract -> moves
  * adjust comment in source to make checkpatch happy
v3:
  * put acpi_add_rom_blob() to hw/acpi/utils.c
v2:
  * remove unused header in original source file
---
 hw/acpi/Makefile.objs    |  2 +-
 hw/acpi/utils.c          | 35 +++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c | 26 ++++++++++----------------
 hw/i386/acpi-build.c     | 26 ++++++++++----------------
 include/hw/acpi/utils.h  |  9 +++++++++
 5 files changed, 65 insertions(+), 33 deletions(-)
 create mode 100644 hw/acpi/utils.c
 create mode 100644 include/hw/acpi/utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 661a9b8c2f..9bb2101e3b 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -10,7 +10,7 @@ common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
 common-obj-y += bios-linker-loader.o
-common-obj-y += aml-build.o
+common-obj-y += aml-build.o utils.o
 common-obj-$(CONFIG_ACPI_PCI) += pci.o
 common-obj-$(CONFIG_TPM) += tpm.o
 
diff --git a/hw/acpi/utils.c b/hw/acpi/utils.c
new file mode 100644
index 0000000000..a134a4d554
--- /dev/null
+++ b/hw/acpi/utils.c
@@ -0,0 +1,35 @@
+/*
+ * Utilities for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2019 Intel Corporation
+ * Copyright (C) 2019 Red Hat Inc
+ *
+ * Author: Wei Yang <richardw.yang@linux.intel.com>
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
+#include "hw/loader.h"
+
+MemoryRegion *acpi_add_rom_blob(FWCfgCallback update, void *opaque,
+                                GArray *blob, const char *name,
+                                uint64_t max_size)
+{
+    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
+                        name, update, opaque, NULL, true);
+}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 4a64f9985c..e3353de9e4 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -37,9 +37,9 @@
 #include "hw/acpi/acpi.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
-#include "hw/loader.h"
 #include "hw/hw.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
 #include "hw/acpi/pci.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
@@ -866,14 +866,6 @@ static void virt_acpi_build_reset(void *build_opaque)
     build_state->patched = false;
 }
 
-static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
-                                       GArray *blob, const char *name,
-                                       uint64_t max_size)
-{
-    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, virt_acpi_build_update, build_state, NULL, true);
-}
-
 static const VMStateDescription vmstate_virt_acpi_build = {
     .name = "virt_acpi_build",
     .version_id = 1,
@@ -905,20 +897,22 @@ void virt_acpi_setup(VirtMachineState *vms)
     virt_acpi_build(vms, &tables);
 
     /* Now expose it all to Guest */
-    build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-                                               ACPI_BUILD_TABLE_FILE,
-                                               ACPI_BUILD_TABLE_MAX_SIZE);
+    build_state->table_mr = acpi_add_rom_blob(virt_acpi_build_update,
+                                              build_state, tables.table_data,
+                                              ACPI_BUILD_TABLE_FILE,
+                                              ACPI_BUILD_TABLE_MAX_SIZE);
     assert(build_state->table_mr != NULL);
 
     build_state->linker_mr =
-        acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
-                          "etc/table-loader", 0);
+        acpi_add_rom_blob(virt_acpi_build_update, build_state,
+                          tables.linker->cmd_blob, "etc/table-loader", 0);
 
     fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data,
                     acpi_data_len(tables.tcpalog));
 
-    build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
-                                              ACPI_BUILD_RSDP_FILE, 0);
+    build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
+                                             build_state, tables.rsdp,
+                                             ACPI_BUILD_RSDP_FILE, 0);
 
     qemu_register_reset(virt_acpi_build_reset, build_state);
     virt_acpi_build_reset(build_state);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 85dc1640bc..5fdf7e2da7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -38,7 +38,6 @@
 #include "hw/acpi/piix4.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
-#include "hw/loader.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
@@ -59,6 +58,7 @@
 #include "hw/i386/x86-iommu.h"
 
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
 #include "hw/acpi/pci.h"
 
 #include "qom/qom-qobject.h"
@@ -2824,14 +2824,6 @@ static void acpi_build_reset(void *build_opaque)
     build_state->patched = 0;
 }
 
-static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
-                                       GArray *blob, const char *name,
-                                       uint64_t max_size)
-{
-    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, acpi_build_update, build_state, NULL, true);
-}
-
 static const VMStateDescription vmstate_acpi_build = {
     .name = "acpi_build",
     .version_id = 1,
@@ -2873,14 +2865,15 @@ void acpi_setup(void)
     acpi_build(&tables, MACHINE(pcms));
 
     /* Now expose it all to Guest */
-    build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-                                               ACPI_BUILD_TABLE_FILE,
-                                               ACPI_BUILD_TABLE_MAX_SIZE);
+    build_state->table_mr = acpi_add_rom_blob(acpi_build_update,
+                                              build_state, tables.table_data,
+                                              ACPI_BUILD_TABLE_FILE,
+                                              ACPI_BUILD_TABLE_MAX_SIZE);
     assert(build_state->table_mr != NULL);
 
     build_state->linker_mr =
-        acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
-                          "etc/table-loader", 0);
+        acpi_add_rom_blob(acpi_build_update, build_state,
+                          tables.linker->cmd_blob, "etc/table-loader", 0);
 
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
@@ -2917,8 +2910,9 @@ void acpi_setup(void)
         build_state->rsdp_mr = NULL;
     } else {
         build_state->rsdp = NULL;
-        build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
-                                                  ACPI_BUILD_RSDP_FILE, 0);
+        build_state->rsdp_mr = acpi_add_rom_blob(acpi_build_update,
+                                                 build_state, tables.rsdp,
+                                                 ACPI_BUILD_RSDP_FILE, 0);
     }
 
     qemu_register_reset(acpi_build_reset, build_state);
diff --git a/include/hw/acpi/utils.h b/include/hw/acpi/utils.h
new file mode 100644
index 0000000000..140b4de603
--- /dev/null
+++ b/include/hw/acpi/utils.h
@@ -0,0 +1,9 @@
+#ifndef HW_ACPI_UTILS_H
+#define HW_ACPI_UTILS_H
+
+#include "hw/nvram/fw_cfg.h"
+
+MemoryRegion *acpi_add_rom_blob(FWCfgCallback update, void *opaque,
+                                GArray *blob, const char *name,
+                                uint64_t max_size);
+#endif
-- 
2.19.1

[Qemu-devel] [PULL 08/11] hw/acpi: extract acpi_add_rom_blob()

[Michael S. Tsirkin]

From: Wei Yang <richardw.yang@linux.intel.com>

arm and i386 has almost the same function acpi_add_rom_blob(), except
giving different FWCfgCallback function.

This patch moves acpi_add_rom_blob() to utils.c by passing
FWCfgCallback to it.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

v7:
  * rebase on top of current master because of conflict
v6:
  * change author from Igor to Michael
v5:
  * remove unnecessary header glib/gprintf.h
  * rearrange include header to make it more suitable
v4:
  * extract -> moves
  * adjust comment in source to make checkpatch happy
v3:
  * put acpi_add_rom_blob() to hw/acpi/utils.c
v2:
  * remove unused header in original source file
Message-Id: <20190610011830.28398-1-richardw.yang@linux.intel.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/utils.h  |  9 +++++++++
 hw/acpi/utils.c          | 35 +++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c | 26 ++++++++++----------------
 hw/i386/acpi-build.c     | 26 ++++++++++----------------
 hw/acpi/Makefile.objs    |  2 +-
 5 files changed, 65 insertions(+), 33 deletions(-)
 create mode 100644 include/hw/acpi/utils.h
 create mode 100644 hw/acpi/utils.c

diff --git a/include/hw/acpi/utils.h b/include/hw/acpi/utils.h
new file mode 100644
index 0000000000..140b4de603
--- /dev/null
+++ b/include/hw/acpi/utils.h
@@ -0,0 +1,9 @@
+#ifndef HW_ACPI_UTILS_H
+#define HW_ACPI_UTILS_H
+
+#include "hw/nvram/fw_cfg.h"
+
+MemoryRegion *acpi_add_rom_blob(FWCfgCallback update, void *opaque,
+                                GArray *blob, const char *name,
+                                uint64_t max_size);
+#endif
diff --git a/hw/acpi/utils.c b/hw/acpi/utils.c
new file mode 100644
index 0000000000..a134a4d554
--- /dev/null
+++ b/hw/acpi/utils.c
@@ -0,0 +1,35 @@
+/*
+ * Utilities for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2019 Intel Corporation
+ * Copyright (C) 2019 Red Hat Inc
+ *
+ * Author: Wei Yang <richardw.yang@linux.intel.com>
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
+#include "hw/loader.h"
+
+MemoryRegion *acpi_add_rom_blob(FWCfgCallback update, void *opaque,
+                                GArray *blob, const char *name,
+                                uint64_t max_size)
+{
+    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
+                        name, update, opaque, NULL, true);
+}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9e8b84988d..0afb372769 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -36,9 +36,9 @@
 #include "hw/acpi/acpi.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
-#include "hw/loader.h"
 #include "hw/hw.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
 #include "hw/acpi/pci.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
@@ -865,14 +865,6 @@ static void virt_acpi_build_reset(void *build_opaque)
     build_state->patched = false;
 }
 
-static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
-                                       GArray *blob, const char *name,
-                                       uint64_t max_size)
-{
-    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, virt_acpi_build_update, build_state, NULL, true);
-}
-
 static const VMStateDescription vmstate_virt_acpi_build = {
     .name = "virt_acpi_build",
     .version_id = 1,
@@ -904,20 +896,22 @@ void virt_acpi_setup(VirtMachineState *vms)
     virt_acpi_build(vms, &tables);
 
     /* Now expose it all to Guest */
-    build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-                                               ACPI_BUILD_TABLE_FILE,
-                                               ACPI_BUILD_TABLE_MAX_SIZE);
+    build_state->table_mr = acpi_add_rom_blob(virt_acpi_build_update,
+                                              build_state, tables.table_data,
+                                              ACPI_BUILD_TABLE_FILE,
+                                              ACPI_BUILD_TABLE_MAX_SIZE);
     assert(build_state->table_mr != NULL);
 
     build_state->linker_mr =
-        acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
-                          "etc/table-loader", 0);
+        acpi_add_rom_blob(virt_acpi_build_update, build_state,
+                          tables.linker->cmd_blob, "etc/table-loader", 0);
 
     fw_cfg_add_file(vms->fw_cfg, ACPI_BUILD_TPMLOG_FILE, tables.tcpalog->data,
                     acpi_data_len(tables.tcpalog));
 
-    build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
-                                              ACPI_BUILD_RSDP_FILE, 0);
+    build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
+                                             build_state, tables.rsdp,
+                                             ACPI_BUILD_RSDP_FILE, 0);
 
     qemu_register_reset(virt_acpi_build_reset, build_state);
     virt_acpi_build_reset(build_state);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 45c369c923..f2db7c1e22 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -37,7 +37,6 @@
 #include "hw/acpi/piix4.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/acpi/bios-linker-loader.h"
-#include "hw/loader.h"
 #include "hw/isa/isa.h"
 #include "hw/block/fdc.h"
 #include "hw/acpi/memory_hotplug.h"
@@ -58,6 +57,7 @@
 #include "hw/i386/x86-iommu.h"
 
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/utils.h"
 #include "hw/acpi/pci.h"
 
 #include "qom/qom-qobject.h"
@@ -2823,14 +2823,6 @@ static void acpi_build_reset(void *build_opaque)
     build_state->patched = 0;
 }
 
-static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state,
-                                       GArray *blob, const char *name,
-                                       uint64_t max_size)
-{
-    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1,
-                        name, acpi_build_update, build_state, NULL, true);
-}
-
 static const VMStateDescription vmstate_acpi_build = {
     .name = "acpi_build",
     .version_id = 1,
@@ -2872,14 +2864,15 @@ void acpi_setup(void)
     acpi_build(&tables, MACHINE(pcms));
 
     /* Now expose it all to Guest */
-    build_state->table_mr = acpi_add_rom_blob(build_state, tables.table_data,
-                                               ACPI_BUILD_TABLE_FILE,
-                                               ACPI_BUILD_TABLE_MAX_SIZE);
+    build_state->table_mr = acpi_add_rom_blob(acpi_build_update,
+                                              build_state, tables.table_data,
+                                              ACPI_BUILD_TABLE_FILE,
+                                              ACPI_BUILD_TABLE_MAX_SIZE);
     assert(build_state->table_mr != NULL);
 
     build_state->linker_mr =
-        acpi_add_rom_blob(build_state, tables.linker->cmd_blob,
-                          "etc/table-loader", 0);
+        acpi_add_rom_blob(acpi_build_update, build_state,
+                          tables.linker->cmd_blob, "etc/table-loader", 0);
 
     fw_cfg_add_file(pcms->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
                     tables.tcpalog->data, acpi_data_len(tables.tcpalog));
@@ -2916,8 +2909,9 @@ void acpi_setup(void)
         build_state->rsdp_mr = NULL;
     } else {
         build_state->rsdp = NULL;
-        build_state->rsdp_mr = acpi_add_rom_blob(build_state, tables.rsdp,
-                                                  ACPI_BUILD_RSDP_FILE, 0);
+        build_state->rsdp_mr = acpi_add_rom_blob(acpi_build_update,
+                                                 build_state, tables.rsdp,
+                                                 ACPI_BUILD_RSDP_FILE, 0);
     }
 
     qemu_register_reset(acpi_build_reset, build_state);
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 661a9b8c2f..9bb2101e3b 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -10,7 +10,7 @@ common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
 common-obj-y += bios-linker-loader.o
-common-obj-y += aml-build.o
+common-obj-y += aml-build.o utils.o
 common-obj-$(CONFIG_ACPI_PCI) += pci.o
 common-obj-$(CONFIG_TPM) += tpm.o
 
-- 
MST

[Qemu-devel] [PULL 10/11] q35: update DSDT

[Michael S. Tsirkin]

update expected files and drop them from allowed diff list.

Fixes: 4a4418369d6 ("q35: fix mmconfig and PCI0._CRS")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/bios-tables-test-allowed-diff.h |   8 --------
 tests/data/acpi/q35/DSDT              | Bin 7815 -> 7841 bytes
 tests/data/acpi/q35/DSDT.bridge       | Bin 7832 -> 7858 bytes
 tests/data/acpi/q35/DSDT.cphp         | Bin 8278 -> 8304 bytes
 tests/data/acpi/q35/DSDT.dimmpxm      | Bin 9468 -> 9494 bytes
 tests/data/acpi/q35/DSDT.ipmibt       | Bin 7890 -> 7916 bytes
 tests/data/acpi/q35/DSDT.memhp        | Bin 9174 -> 9200 bytes
 tests/data/acpi/q35/DSDT.mmio64       | Bin 8945 -> 8971 bytes
 tests/data/acpi/q35/DSDT.numamem      | Bin 7821 -> 7847 bytes
 9 files changed, 8 deletions(-)

diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
index 3bbd22c62a..dfb8523c8b 100644
--- a/tests/bios-tables-test-allowed-diff.h
+++ b/tests/bios-tables-test-allowed-diff.h
@@ -1,9 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/DSDT",
-"tests/data/acpi/q35/DSDT.bridge",
-"tests/data/acpi/q35/DSDT.mmio64",
-"tests/data/acpi/q35/DSDT.ipmibt",
-"tests/data/acpi/q35/DSDT.cphp",
-"tests/data/acpi/q35/DSDT.memhp",
-"tests/data/acpi/q35/DSDT.numamem",
-"tests/data/acpi/q35/DSDT.dimmpxm",
diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index 7576ffcd05991ad5a3901c0f7698a52fffc6d6e2..f9f36d1645c9b57aea38350d67dfaa143845697d 100644
GIT binary patch
delta 65
zcmZp-U1-bY66_MPP>z9t(QYHxMHw+i!5F>xV5j&1XHNr;c;}#CK`(BuZIeIB#Pf3e
R|NnnI0|czt>@4fT2mo%L69@nR

delta 51
zcmZ2z+iuI{66_MvF2}&Y*szi7qKue3e~eyyuv2`1v!?+^ymL^npaU1zoXH<$;x}i>
Hnll0bdb|$6

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index c623cc5d72a2e346793fa9128e7e88b6781241b2..29176832ca9842c6654273ae1246321aa38b2821 100644
GIT binary patch
delta 65
zcmbPXyUCWzCD<iolN<vB<Gqbs7iGj81!MH$gPr07oIMRV;+=zn1--brwoU#h6VJ=>
R|NsB>3=ptlv$Jd{BLJG%6YKy0

delta 51
zcmdmFJHwXCCD<ioh8zO}qryh6i!x&F{4sj*!A|i3&YlJw@y<cPf(~3<b0&Y3iQk+h
H>%j;Bc5Du$

diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index 7ac526e4669fd84048b2d8ec6af8661503e1a9fa..19bdb5d21050f24aaacbafb1f84d6e1d541876c6 100644
GIT binary patch
delta 65
zcmccS@WFx0CD<jTK!JgQar;KD1#)7Jf-!pW!A|i3&YlJw@y<cPf?nKQ+a@27i|6I|
R|NsAb1_)TOSxDZ65dfFX6KMbd

delta 51
zcmez1aLs|sCD<h-Oo4%c@#jXa1#)8U{4sj*!A|i3&YlJw@y<cPf(~3<b0!~<i{ET1
HZ_Wq+oC^;j

diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 7177116a21969fa3649053ec83a8704b0d84b9c5..727fe489b4c8cdd39476ff61e7d7664c816f5291 100644
GIT binary patch
delta 65
zcmez4In9g9CD<iIOqGFwv0)>Zo3farV2oaTuv2`1v!?+^ymL^npcgmSw#jMA@w^=W
R|Nmdl00ApD?@)GO1OQ#b61)Ha

delta 51
zcmbQ{^~aOTCD<k8j|u|=<Kc~5Zpvcr{4sj*!A|i3&YlJw@y<cPf(~3<b0()L$8Wx`
HY|aP(oxu-u

diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt
index c7f431f058bcb54e364be4edfd9d4609bc9bb602..9634930e6125de4375d87a56a353f636985599d4 100644
GIT binary patch
delta 65
zcmca)`^J{bCD<k8jT{35WAa8W8Cfw$!5F>xV5j&1XHNr;c;}#CK`(BuZIf+e<9Rv$
R|Np<90RmQR?vr(41OR*267c{4

delta 51
zcmaE3d&!o|CD<k8k{kmA<I;^>GO}Xs{4sj*!A|i3&YlJw@y<cPf(~3<b0*u!#&6y#
HYt9G&jZqG%

diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index 0235461391c90d577c3d533c2fd29ce50f1e2981..dad5dc8db2f13bdb0de001da42c13b18286c3061 100644
GIT binary patch
delta 65
zcmccS{=uEgCD<k8gE9jHqrpZl1tl>@!5F>xV5j&1XHNr;c;}#CK`(BuZId08;(0m#
R|Np<90RmQRo}}c$2mpN;68-=H

delta 51
zcmez1e$AcBCD<k8nlb|eW6?$~1tl?e{usUZV5j&1XHNr;c;}#CK?g3bIg=ff;y3S9
HGG_z;j#>`3

diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index f60ee77fb4d655e77c7ef1e8c205d741cc288939..20f627ed08a0cae4e144f3e4dd7dd5f1d8d0318c 100644
GIT binary patch
delta 65
zcmez9+U>^W66_Mft<1o{_<AFkmZF%WV2oaTuv2`1v!?+^ymL^npcgmSw#i<K@w^=W
R|Nmdl00ApD&ru9z1OQ+{60-mR

delta 51
zcmeBn`{>H$66_N4QHg<pQEwxcmZF$De~eyyuv2`1v!?+^ymL^npaU1zoXK8_@tY4T
HdN2Y2dk79|

diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index 6c0d4f2bcb384cc3f3919b7472021ed51b93cbf5..7b96a972804e95e191d9d3bf9a965e90f6f7e555 100644
GIT binary patch
delta 65
zcmeCRU2e<e66_MPT#kW(@x?~2>oQ`Ff-!pW!A|i3&YlJw@y<cPf?nKQ+a`aPiRb0`
R|NsAb1_)TO*;Ceq5dfBm6V?C#

delta 51
zcmZ2(+iT0^66_MvE62dVsI`&nx{R1Re~eyyuv2`1v!?+^ymL^npaU1zoXOv1;x`w_
Hnll0ba<&eR

-- 
MST

[Qemu-devel] [PULL 11/11] tests/rebuild-expected-aml.sh: blow out difflist

[Michael S. Tsirkin]

As expected files have been updated, make sure we
do not forget to remove them from the allowed
diff list.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/data/acpi/rebuild-expected-aml.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/data/acpi/rebuild-expected-aml.sh b/tests/data/acpi/rebuild-expected-aml.sh
index d2853218dd..f89d4624bc 100755
--- a/tests/data/acpi/rebuild-expected-aml.sh
+++ b/tests/data/acpi/rebuild-expected-aml.sh
@@ -29,5 +29,8 @@ for qemu in $qemu_bins; do
     TEST_ACPI_REBUILD_AML=y QTEST_QEMU_BINARY=$qemu tests/bios-tables-test
 done
 
+eval `grep SRC_PATH= config-host.mak`
+
+echo '/* List of comma-separated changed AML files to ignore */' > ${SRC_PATH}/tests/bios-tables-test-allowed-diff.h
 
 echo "The files were rebuilt and can be added to git."
-- 
MST

Re: [Qemu-devel] [PULL 00/11] virtio, acpi: fixes, cleanups

[Peter Maydell]

On Sun, 16 Jun 2019 at 22:36, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit f3d0bec9f80e4ed7796fffa834ba0a53f2094f7f:
>
>   Merge remote-tracking branch 'remotes/maxreitz/tags/pull-block-2019-06-14' into staging (2019-06-14 14:46:13 +0100)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 5f6b3561bf58395fd6c906d7064a1a5693a2e426:
>
>   tests/rebuild-expected-aml.sh: blow out difflist (2019-06-16 16:44:44 -0400)
>
> ----------------------------------------------------------------
> virtio, acpi: fixes, cleanups
>
> A bunch of minor fixes all over the place.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.1
for any user-visible changes.

-- PMM