date:20240422


On 22/04/2024 18:59, Philippe Mathieu-Daudé wrote:
> On 22/4/24 17:52, CLEMENT MATHIEU--DRIF wrote:
>> This piece of code can be shared by both IOTLB invalidation and
>> PASID-based IOTLB invalidation
>>
>> Signed-off-by: Clément Mathieu--Drif 
>> ---
>>   hw/i386/intel_iommu.c | 57 +--
>>   1 file changed, 33 insertions(+), 24 deletions(-)
>
>
>>   static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>>     VTDInvDesc *inv_desc)
>>   {
>>   VTDAddressSpace *vtd_dev_as;
>> -    IOMMUTLBEvent event;
>>   hwaddr addr;
>> -    uint64_t sz;
>>   uint16_t sid;
>>   bool size;
>>
>> @@ -2912,6 +2941,7 @@ static bool 
>> vtd_process_device_iotlb_desc(IntelIOMMUState *s,
>>   return false;
>>   }
>>
>> +
>
> Spurious newline ;)
Oups, sorry, it's fixed
>
> Reviewed-by: Philippe Mathieu-Daudé 
>
>>   /*
>

Re: [PATCH intel_iommu 3/7] intel_iommu: make types match


On 22/04/2024 19:03, Philippe Mathieu-Daudé wrote:
> On 22/4/24 17:52, CLEMENT MATHIEU--DRIF wrote:
>> The 'level' field in vtd_iotlb_key is an uint8_t.
>> We don't need to store level as an int in vtd_lookup_iotlb
>>
>> Signed-off-by: Clément Mathieu--Drif 
>> ---
>>   hw/i386/intel_iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 6f1364b3fd..ba545590b1 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -333,7 +333,7 @@ static VTDIOTLBEntry 
>> *vtd_lookup_iotlb(IntelIOMMUState *s, uint16_t source_id,
>>   {
>>   struct vtd_iotlb_key key;
>>   VTDIOTLBEntry *entry;
>> -    int level;
>> +    uint8_t level;
>
> Or simply 'unsigned' up to vtd_slpt_level_shift()?
vtd_iotlb_key.level is an uint8_t, just avoiding a warning here

[PATCH v10 3/6] ui/console: Use qemu_dmabuf_get_..() helpers instead

From: Dongwon Kim 

This commit updates all instances where fields within the QemuDmaBuf
struct are directly accessed, replacing them with calls to these new
helper functions.

v6: fix typos in helper names in ui/spice-display.c

v7: removed prefix, "dpy_gl_" from all helpers

v8: Introduction of helpers was removed as those were already added
by the previous commit

Suggested-by: Marc-André Lureau 
Reviewed-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 hw/display/vhost-user-gpu.c |  6 ++--
 hw/display/virtio-gpu-udmabuf.c |  7 +++--
 hw/vfio/display.c   | 15 +++---
 ui/console.c|  4 +--
 ui/dbus-console.c   |  9 --
 ui/dbus-listener.c  | 43 +---
 ui/egl-headless.c   | 23 ++-
 ui/egl-helpers.c| 47 ++-
 ui/gtk-egl.c| 48 ---
 ui/gtk-gl-area.c| 37 
 ui/gtk.c|  6 ++--
 ui/spice-display.c  | 50 +++--
 12 files changed, 187 insertions(+), 108 deletions(-)

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 709c8a02a1..ea9a6c5d10 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -249,6 +249,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
VhostUserGpuMsg *msg)
 case VHOST_USER_GPU_DMABUF_SCANOUT: {
 VhostUserGpuDMABUFScanout *m = >payload.dmabuf_scanout;
 int fd = qemu_chr_fe_get_msgfd(>vhost_chr);
+int old_fd;
 QemuDmaBuf *dmabuf;
 
 if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
@@ -262,8 +263,9 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
VhostUserGpuMsg *msg)
 g->parent_obj.enable = 1;
 con = g->parent_obj.scanout[m->scanout_id].con;
 dmabuf = >dmabuf[m->scanout_id];
-if (dmabuf->fd >= 0) {
-close(dmabuf->fd);
+old_fd = qemu_dmabuf_get_fd(dmabuf);
+if (old_fd >= 0) {
+close(old_fd);
 dmabuf->fd = -1;
 }
 dpy_gl_release_dmabuf(con, dmabuf);
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index d51184d658..c90eba281e 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -206,6 +206,7 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
 {
 struct virtio_gpu_scanout *scanout = >parent_obj.scanout[scanout_id];
 VGPUDMABuf *new_primary, *old_primary = NULL;
+uint32_t width, height;
 
 new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
 if (!new_primary) {
@@ -216,10 +217,10 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
 old_primary = g->dmabuf.primary[scanout_id];
 }
 
+width = qemu_dmabuf_get_width(_primary->buf);
+height = qemu_dmabuf_get_height(_primary->buf);
 g->dmabuf.primary[scanout_id] = new_primary;
-qemu_console_resize(scanout->con,
-new_primary->buf.width,
-new_primary->buf.height);
+qemu_console_resize(scanout->con, width, height);
 dpy_gl_scanout_dmabuf(scanout->con, _primary->buf);
 
 if (old_primary) {
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 1aa440c663..4861c8161d 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -259,9 +259,13 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice 
*vdev,
 
 static void vfio_display_free_one_dmabuf(VFIODisplay *dpy, VFIODMABuf *dmabuf)
 {
+int fd;
+
 QTAILQ_REMOVE(>dmabuf.bufs, dmabuf, next);
+
+fd = qemu_dmabuf_get_fd(>buf);
 dpy_gl_release_dmabuf(dpy->con, >buf);
-close(dmabuf->buf.fd);
+close(fd);
 g_free(dmabuf);
 }
 
@@ -286,6 +290,7 @@ static void vfio_display_dmabuf_update(void *opaque)
 VFIOPCIDevice *vdev = opaque;
 VFIODisplay *dpy = vdev->dpy;
 VFIODMABuf *primary, *cursor;
+uint32_t width, height;
 bool free_bufs = false, new_cursor = false;
 
 primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
@@ -296,10 +301,12 @@ static void vfio_display_dmabuf_update(void *opaque)
 return;
 }
 
+width = qemu_dmabuf_get_width(>buf);
+height = qemu_dmabuf_get_height(>buf);
+
 if (dpy->dmabuf.primary != primary) {
 dpy->dmabuf.primary = primary;
-qemu_console_resize(dpy->con,
-primary->buf.width, primary->buf.height);
+qemu_console_resize(dpy->con, width, height);
 dpy_gl_scanout_dmabuf(dpy->con, >buf);
 free_bufs = true;
 }
@@ -328,7 +335,7 @@ static void vfio_display_dmabuf_update(void *opaque)
 cursor->pos_updates = 0;
 }
 
-dpy_gl_update(dpy->con, 0, 0, primary->buf.width, primary->buf.height);
+dpy_gl_update(dpy->con, 0, 0, width, height);
 
 if (free_bufs) {

[PATCH v10 5/6] ui/console: Use qemu_dmabuf_new() and free() helpers instead

From: Dongwon Kim 

This commit introduces utility functions for the creation and deallocation
of QemuDmaBuf instances. Additionally, it updates all relevant sections
of the codebase to utilize these new utility functions.

v7: remove prefix, "dpy_gl_" from all helpers
qemu_dmabuf_free() returns without doing anything if input is null
(Daniel P. Berrangé )
call G_DEFINE_AUTOPTR_CLEANUP_FUNC for qemu_dmabuf_free()
(Daniel P. Berrangé )

v8: Introduction of helpers was removed as those were already added
by the previous commit

v9: set dmabuf->allow_fences to 'true' when dmabuf is created in
virtio_gpu_create_dmabuf()/virtio-gpu-udmabuf.c

removed unnecessary spaces were accidently added in the patch,
'ui/console: Use qemu_dmabuf_new() a...'

Suggested-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 include/hw/vfio/vfio-common.h   |  2 +-
 include/hw/virtio/virtio-gpu.h  |  4 ++--
 hw/display/vhost-user-gpu.c | 32 ++--
 hw/display/virtio-gpu-udmabuf.c | 24 +---
 hw/vfio/display.c   | 26 --
 ui/dbus-listener.c  | 28 
 6 files changed, 54 insertions(+), 62 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..d66e27db02 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -148,7 +148,7 @@ typedef struct VFIOGroup {
 } VFIOGroup;
 
 typedef struct VFIODMABuf {
-QemuDmaBuf buf;
+QemuDmaBuf *buf;
 uint32_t pos_x, pos_y, pos_updates;
 uint32_t hot_x, hot_y, hot_updates;
 int dmabuf_id;
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index ed44cdad6b..56d6e821bf 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -169,7 +169,7 @@ struct VirtIOGPUBaseClass {
 DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
 
 typedef struct VGPUDMABuf {
-QemuDmaBuf buf;
+QemuDmaBuf *buf;
 uint32_t scanout_id;
 QTAILQ_ENTRY(VGPUDMABuf) next;
 } VGPUDMABuf;
@@ -238,7 +238,7 @@ struct VhostUserGPU {
 VhostUserBackend *vhost;
 int vhost_gpu_fd; /* closed by the chardev */
 CharBackend vhost_chr;
-QemuDmaBuf dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
+QemuDmaBuf *dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
 bool backend_blocked;
 };
 
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index ea9a6c5d10..c91a800455 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -250,6 +250,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
VhostUserGpuMsg *msg)
 VhostUserGpuDMABUFScanout *m = >payload.dmabuf_scanout;
 int fd = qemu_chr_fe_get_msgfd(>vhost_chr);
 int old_fd;
+uint64_t modifier = 0;
 QemuDmaBuf *dmabuf;
 
 if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
@@ -262,31 +263,34 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, 
VhostUserGpuMsg *msg)
 
 g->parent_obj.enable = 1;
 con = g->parent_obj.scanout[m->scanout_id].con;
-dmabuf = >dmabuf[m->scanout_id];
-old_fd = qemu_dmabuf_get_fd(dmabuf);
-if (old_fd >= 0) {
-close(old_fd);
-dmabuf->fd = -1;
+dmabuf = g->dmabuf[m->scanout_id];
+if (dmabuf) {
+old_fd = qemu_dmabuf_get_fd(dmabuf);
+if (old_fd >= 0) {
+close(old_fd);
+qemu_dmabuf_set_fd(dmabuf, -1);
+}
 }
 dpy_gl_release_dmabuf(con, dmabuf);
+g_clear_pointer(, qemu_dmabuf_free);
 if (fd == -1) {
 dpy_gl_scanout_disable(con);
 break;
 }
-*dmabuf = (QemuDmaBuf) {
-.fd = fd,
-.width = m->fd_width,
-.height = m->fd_height,
-.stride = m->fd_stride,
-.fourcc = m->fd_drm_fourcc,
-.y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
-};
+
 if (msg->request == VHOST_USER_GPU_DMABUF_SCANOUT2) {
 VhostUserGpuDMABUFScanout2 *m2 = >payload.dmabuf_scanout2;
-dmabuf->modifier = m2->modifier;
+modifier = m2->modifier;
 }
 
+dmabuf = qemu_dmabuf_new(m->fd_width, m->fd_height,
+ m->fd_stride, 0, 0, 0, 0,
+ m->fd_drm_fourcc, modifier,
+ fd, false, m->fd_flags &
+ VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP);
+
 dpy_gl_scanout_dmabuf(con, dmabuf);
+g->dmabuf[m->scanout_id] = dmabuf;
 break;
 }
 case VHOST_USER_GPU_DMABUF_UPDATE: {
diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
index c90eba281e..c02ec6d37d 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -162,7

[PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers

From: Dongwon Kim 

New header and source files are added for containing QemuDmaBuf struct
definition and newly introduced helpers for creating/freeing the struct
and accessing its data.

v10: Change the license type for both dmabuf.h and dmabuf.c from MIT to
 GPL to be in line with QEMU's default license
 (Daniel P. Berrangé )

Suggested-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 include/ui/console.h |  20 +
 include/ui/dmabuf.h  |  64 +++
 ui/dmabuf.c  | 189 +++
 ui/meson.build   |   1 +
 4 files changed, 255 insertions(+), 19 deletions(-)
 create mode 100644 include/ui/dmabuf.h
 create mode 100644 ui/dmabuf.c

diff --git a/include/ui/console.h b/include/ui/console.h
index 0bc7a00ac0..a208a68b88 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -7,6 +7,7 @@
 #include "qapi/qapi-types-ui.h"
 #include "ui/input.h"
 #include "ui/surface.h"
+#include "ui/dmabuf.h"
 
 #define TYPE_QEMU_CONSOLE "qemu-console"
 OBJECT_DECLARE_TYPE(QemuConsole, QemuConsoleClass, QEMU_CONSOLE)
@@ -185,25 +186,6 @@ struct QEMUGLParams {
 int minor_ver;
 };
 
-typedef struct QemuDmaBuf {
-int   fd;
-uint32_t  width;
-uint32_t  height;
-uint32_t  stride;
-uint32_t  fourcc;
-uint64_t  modifier;
-uint32_t  texture;
-uint32_t  x;
-uint32_t  y;
-uint32_t  backing_width;
-uint32_t  backing_height;
-bool  y0_top;
-void  *sync;
-int   fence_fd;
-bool  allow_fences;
-bool  draw_submitted;
-} QemuDmaBuf;
-
 enum display_scanout {
 SCANOUT_NONE,
 SCANOUT_SURFACE,
diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h
new file mode 100644
index 00..7a60116ee6
--- /dev/null
+++ b/include/ui/dmabuf.h
@@ -0,0 +1,64 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * QemuDmaBuf struct and helpers used for accessing its data
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef DMABUF_H
+#define DMABUF_H
+
+typedef struct QemuDmaBuf {
+int   fd;
+uint32_t  width;
+uint32_t  height;
+uint32_t  stride;
+uint32_t  fourcc;
+uint64_t  modifier;
+uint32_t  texture;
+uint32_t  x;
+uint32_t  y;
+uint32_t  backing_width;
+uint32_t  backing_height;
+bool  y0_top;
+void  *sync;
+int   fence_fd;
+bool  allow_fences;
+bool  draw_submitted;
+} QemuDmaBuf;
+
+QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
+   uint32_t stride, uint32_t x,
+   uint32_t y, uint32_t backing_width,
+   uint32_t backing_height, uint32_t fourcc,
+   uint64_t modifier, int32_t dmabuf_fd,
+   bool allow_fences, bool y0_top);
+void qemu_dmabuf_free(QemuDmaBuf *dmabuf);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuDmaBuf, qemu_dmabuf_free);
+
+int32_t qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_width(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_height(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf);
+uint64_t qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_x(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_y(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf);
+uint32_t qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf);
+bool qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf);
+void *qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf);
+int32_t qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf);
+bool qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf);
+bool qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
+void qemu_dmabuf_set_texture(QemuDmaBuf *dmabuf, uint32_t texture);
+void qemu_dmabuf_set_fence_fd(QemuDmaBuf *dmabuf, int32_t fence_fd);
+void qemu_dmabuf_set_sync(QemuDmaBuf *dmabuf, void *sync);
+void qemu_dmabuf_set_draw_submitted(QemuDmaBuf *dmabuf, bool draw_submitted);
+void qemu_dmabuf_set_fd(QemuDmaBuf *dmabuf, int32_t fd);
+
+#endif
diff --git a/ui/dmabuf.c b/ui/dmabuf.c
new file mode 100644
index 00..f447cce4fe
--- /dev/null
+++ b/ui/dmabuf.c
@@ -0,0 +1,189 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * QemuDmaBuf struct and helpers used for accessing its data
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "ui/dmabuf.h"
+
+QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
+uint32_t stride, uint32_t x,
+uint32_t y, uint32_t backing_width,
+

[PATCH v10 4/6] ui/console: Use qemu_dmabuf_set_..() helpers instead

From: Dongwon Kim 

This commit updates all occurrences where these fields were
set directly have been updated to utilize helper functions.

v7: removed prefix, "dpy_gl_" from all helpers

v8: Introduction of helpers was removed as those were already added
by the previous commit

Suggested-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 ui/egl-helpers.c | 16 +---
 ui/gtk-egl.c |  4 ++--
 ui/gtk-gl-area.c |  4 ++--
 ui/gtk.c |  6 +++---
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 3f96e63d25..99b2ebbe23 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -348,8 +348,8 @@ void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf)
 return;
 }
 
-glGenTextures(1, >texture);
-texture = qemu_dmabuf_get_texture(dmabuf);
+glGenTextures(1, );
+qemu_dmabuf_set_texture(dmabuf, texture);
 glBindTexture(GL_TEXTURE_2D, texture);
 glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
 glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
@@ -368,7 +368,7 @@ void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf)
 }
 
 glDeleteTextures(1, );
-dmabuf->texture = 0;
+qemu_dmabuf_set_texture(dmabuf, 0);
 }
 
 void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
@@ -382,7 +382,7 @@ void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
 sync = eglCreateSyncKHR(qemu_egl_display,
 EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
 if (sync != EGL_NO_SYNC_KHR) {
-dmabuf->sync = sync;
+qemu_dmabuf_set_sync(dmabuf, sync);
 }
 }
 }
@@ -390,12 +390,14 @@ void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
 void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
 {
 void *sync = qemu_dmabuf_get_sync(dmabuf);
+int fence_fd;
 
 if (sync) {
-dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
-  sync);
+fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
+  sync);
+qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
 eglDestroySyncKHR(qemu_egl_display, sync);
-dmabuf->sync = NULL;
+qemu_dmabuf_set_sync(dmabuf, NULL);
 }
 }
 
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 7a45daefa1..ec0bf45482 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -87,7 +87,7 @@ void gd_egl_draw(VirtualConsole *vc)
 if (!qemu_dmabuf_get_draw_submitted(dmabuf)) {
 return;
 } else {
-dmabuf->draw_submitted = false;
+qemu_dmabuf_set_draw_submitted(dmabuf, false);
 }
 }
 #endif
@@ -381,7 +381,7 @@ void gd_egl_flush(DisplayChangeListener *dcl,
 if (vc->gfx.guest_fb.dmabuf &&
 !qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
 graphic_hw_gl_block(vc->gfx.dcl.con, true);
-vc->gfx.guest_fb.dmabuf->draw_submitted = true;
+qemu_dmabuf_set_draw_submitted(vc->gfx.guest_fb.dmabuf, true);
 gtk_egl_set_scanout_mode(vc, true);
 gtk_widget_queue_draw_area(area, x, y, w, h);
 return;
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 2d70280803..9a3f3d0d71 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -63,7 +63,7 @@ void gd_gl_area_draw(VirtualConsole *vc)
 if (!qemu_dmabuf_get_draw_submitted(dmabuf)) {
 return;
 } else {
-dmabuf->draw_submitted = false;
+qemu_dmabuf_set_draw_submitted(dmabuf, false);
 }
 }
 #endif
@@ -291,7 +291,7 @@ void gd_gl_area_scanout_flush(DisplayChangeListener *dcl,
 if (vc->gfx.guest_fb.dmabuf &&
 !qemu_dmabuf_get_draw_submitted(vc->gfx.guest_fb.dmabuf)) {
 graphic_hw_gl_block(vc->gfx.dcl.con, true);
-vc->gfx.guest_fb.dmabuf->draw_submitted = true;
+qemu_dmabuf_set_draw_submitted(vc->gfx.guest_fb.dmabuf, true);
 gtk_gl_area_set_scanout_mode(vc, true);
 }
 gtk_gl_area_queue_render(GTK_GL_AREA(vc->gfx.drawing_area));
diff --git a/ui/gtk.c b/ui/gtk.c
index 237c913b26..3a6832eb1b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -598,11 +598,11 @@ void gd_hw_gl_flushed(void *vcon)
 QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
 int fence_fd;
 
-if (dmabuf->fence_fd >= 0) {
-fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
+fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
+if (fence_fd >= 0) {
 qemu_set_fd_handler(fence_fd, NULL, NULL, NULL);
 close(fence_fd);
-dmabuf->fence_fd = -1;
+qemu_dmabuf_set_fence_fd(dmabuf, -1);
 graphic_hw_gl_block(vc->gfx.dcl.con, false);
 }
 }
-- 
2.34.1

[PATCH v10 0/6] ui/console: Private QemuDmaBuf struct

From: Dongwon Kim 

This series introduces privacy enhancements to the QemuDmaBuf struct
and its contained data to bolster security. it accomplishes this by
introducing of helper functions for allocating, deallocating, and
accessing individual fields within the struct and replacing all direct
references to individual fields in the struct with methods using helpers
throughout the codebase.

This change was made based on a suggestion from Marc-André Lureau


(Resumitting same patch series with this new cover-leter)

v6: fixed some typos in patch -
ui/console: Introduce dpy_gl_qemu_dmabuf_get_..() helpers)

v7: included minor fix (ui/gtk: Check if fence_fd is equal to or greater than 0)
(Marc-André Lureau )

migrated all helpers and QemuDmaBuf struct into dmabuf.c and their 
prototypes
to dmabuf.h for better encapsulation (ui/dmabuf: New dmabuf.c and 
dmabuf.h..)
(Daniel P. Berrangé  and
 Marc-André Lureau )

removed 'dpy_gl' from all helpers' names
Defined autoptr clean up function for QemuDmaBuf*
(Daniel P. Berrangé )

Minor corrections

v8: Introduce new dmabuf.c and dmabuf.h and all helper functions in the second
patch in the series (ui/console: new dmabuf.h and dmabuf.c for QemuDma)
(Philippe Mathieu-Daudé )

v9: set dmabuf->allow_fences true when it is created in virtio-gpu-udmabuf

removed unnecessary spaces were added in the patch,
'ui/console: Use qemu_dmabuf_new() a...'

v10: Change the license type for both dmabuf.h and dmabuf.c from MIT to
 GPL to be in line with QEMU's default license
 (Daniel P. Berrangé )

Dongwon Kim (6):
  ui/gtk: Check if fence_fd is equal to or greater than 0
  ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and
helpers
  ui/console: Use qemu_dmabuf_get_..() helpers instead
  ui/console: Use qemu_dmabuf_set_..() helpers instead
  ui/console: Use qemu_dmabuf_new() and free() helpers instead
  ui/console: move QemuDmaBuf struct def to dmabuf.c

 include/hw/vfio/vfio-common.h   |   2 +-
 include/hw/virtio/virtio-gpu.h  |   4 +-
 include/ui/console.h|  20 +--
 include/ui/dmabuf.h |  47 
 hw/display/vhost-user-gpu.c |  32 +++--
 hw/display/virtio-gpu-udmabuf.c |  27 ++---
 hw/vfio/display.c   |  35 +++---
 ui/console.c|   4 +-
 ui/dbus-console.c   |   9 +-
 ui/dbus-listener.c  |  71 ++-
 ui/dmabuf.c | 208 
 ui/egl-headless.c   |  23 ++--
 ui/egl-helpers.c|  59 +
 ui/gtk-egl.c|  52 +---
 ui/gtk-gl-area.c|  41 ---
 ui/gtk.c|  12 +-
 ui/spice-display.c  |  50 
 ui/meson.build  |   1 +
 18 files changed, 505 insertions(+), 192 deletions(-)
 create mode 100644 include/ui/dmabuf.h
 create mode 100644 ui/dmabuf.c

-- 
2.34.1

[PATCH v10 1/6] ui/gtk: Check if fence_fd is equal to or greater than 0

From: Dongwon Kim 

'fence_fd' needs to be validated always before being referenced
And the passing condition should include '== 0' as 0 is a valid
value for the file descriptor.

Suggested-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 ui/gtk-egl.c |  2 +-
 ui/gtk-gl-area.c |  2 +-
 ui/gtk.c | 10 ++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 3af5ac5bcf..955234429d 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -99,7 +99,7 @@ void gd_egl_draw(VirtualConsole *vc)
 #ifdef CONFIG_GBM
 if (dmabuf) {
 egl_dmabuf_create_fence(dmabuf);
-if (dmabuf->fence_fd > 0) {
+if (dmabuf->fence_fd >= 0) {
 qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, 
vc);
 return;
 }
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 52dcac161e..7fffd0544e 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -86,7 +86,7 @@ void gd_gl_area_draw(VirtualConsole *vc)
 #ifdef CONFIG_GBM
 if (dmabuf) {
 egl_dmabuf_create_fence(dmabuf);
-if (dmabuf->fence_fd > 0) {
+if (dmabuf->fence_fd >= 0) {
 qemu_set_fd_handler(dmabuf->fence_fd, gd_hw_gl_flushed, NULL, 
vc);
 return;
 }
diff --git a/ui/gtk.c b/ui/gtk.c
index 810d7fc796..7819a86321 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -597,10 +597,12 @@ void gd_hw_gl_flushed(void *vcon)
 VirtualConsole *vc = vcon;
 QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
 
-qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
-close(dmabuf->fence_fd);
-dmabuf->fence_fd = -1;
-graphic_hw_gl_block(vc->gfx.dcl.con, false);
+if (dmabuf->fence_fd >= 0) {
+qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
+close(dmabuf->fence_fd);
+dmabuf->fence_fd = -1;
+graphic_hw_gl_block(vc->gfx.dcl.con, false);
+}
 }
 
 /** DisplayState Callbacks (opengl version) **/
-- 
2.34.1

[PATCH v10 6/6] ui/console: move QemuDmaBuf struct def to dmabuf.c

From: Dongwon Kim 

To complete privatizing process of QemuDmaBuf, QemuDmaBuf struct def
is moved to dmabuf.c

Suggested-by: Marc-André Lureau 
Cc: Philippe Mathieu-Daudé 
Cc: Daniel P. Berrangé 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 include/ui/dmabuf.h | 19 +--
 ui/dmabuf.c | 19 +++
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h
index 7a60116ee6..80d89ac461 100644
--- a/include/ui/dmabuf.h
+++ b/include/ui/dmabuf.h
@@ -10,24 +10,7 @@
 #ifndef DMABUF_H
 #define DMABUF_H
 
-typedef struct QemuDmaBuf {
-int   fd;
-uint32_t  width;
-uint32_t  height;
-uint32_t  stride;
-uint32_t  fourcc;
-uint64_t  modifier;
-uint32_t  texture;
-uint32_t  x;
-uint32_t  y;
-uint32_t  backing_width;
-uint32_t  backing_height;
-bool  y0_top;
-void  *sync;
-int   fence_fd;
-bool  allow_fences;
-bool  draw_submitted;
-} QemuDmaBuf;
+typedef struct QemuDmaBuf QemuDmaBuf;
 
 QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
uint32_t stride, uint32_t x,
diff --git a/ui/dmabuf.c b/ui/dmabuf.c
index f447cce4fe..7f092333b4 100644
--- a/ui/dmabuf.c
+++ b/ui/dmabuf.c
@@ -10,6 +10,25 @@
 #include "qemu/osdep.h"
 #include "ui/dmabuf.h"
 
+struct QemuDmaBuf {
+int   fd;
+uint32_t  width;
+uint32_t  height;
+uint32_t  stride;
+uint32_t  fourcc;
+uint64_t  modifier;
+uint32_t  texture;
+uint32_t  x;
+uint32_t  y;
+uint32_t  backing_width;
+uint32_t  backing_height;
+bool  y0_top;
+void  *sync;
+int   fence_fd;
+bool  allow_fences;
+bool  draw_submitted;
+};
+
 QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
 uint32_t stride, uint32_t x,
 uint32_t y, uint32_t backing_width,
-- 
2.34.1

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

2024-04-22 Thread John Snow

On Mon, Apr 22, 2024 at 12:38 PM John Snow  wrote:
>
> On Mon, Apr 22, 2024 at 5:20 AM Markus Armbruster  wrote:
> >
> > John Snow  writes:
> >
> > > On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster  
> > > wrote:
> > >
> > >> John Snow  writes:
> > >>
> > >> > This series adds a new qapi-domain extension for Sphinx, which adds a
> > >> > series of custom directives for documenting QAPI definitions.
> > >> >
> > >> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> > >> >
> > >> > (Link to a demo HTML page at the end of this cover letter, but I want
> > >> > you to read the cover letter first to explain what you're seeing.)
> > >> >
> > >> > This adds a new QAPI Index page, cross-references for QMP commands,
> > >> > events, and data types, and improves the aesthetics of the QAPI/QMP
> > >> > documentation.
> > >>
> > >> Cross-references alone will be a *massive* improvement!  I'm sure
> > >> readers will appreciate better looks and an index, too.
> > >>
> > >> > This series adds only the new ReST syntax, *not* the autogenerator. The
> > >> > ReST syntax used in this series is, in general, not intended for anyone
> > >> > to actually write by hand. This mimics how Sphinx's own autodoc
> > >> > extension generates Python domain directives, which are then re-parsed
> > >> > to produce the final result.
> > >> >
> > >> > I have prototyped such a generator, but it isn't ready for inclusion
> > >> > yet. (Rest assured: error context reporting is preserved down to the
> > >> > line, even in generated ReST. There is no loss in usability for this
> > >> > approach. It will likely either supplant qapidoc.py or heavily alter
> > >> > it.) The generator requires only extremely minor changes to
> > >> > scripts/qapi/parser.py to preserve nested indentation and provide more
> > >> > accurate line information. It is less invasive than you may
> > >> > fear. Relying on a secondary ReST parse phase eliminates much of the
> > >> > complexity of qapidoc.py. Sleep soundly.
> > >>
> > >> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
> > >>
> > >> You proprose to generate formatted documentation in two steps:
> > >>
> > >> • First, the QAPI generator generates .rst from the QAPI schema.  The
> > >>   generated .rst makes use of a custom directives.
> > >>
> > >
> > > Yes, but this .rst file is built in-memory and never makes it to disk, 
> > > like
> > > Sphinx's autodoc for Python.
> >
> > Makes sense.
> >
> > > (We can add a debug knob to log it or save it out to disk if needed.)
> >
> > Likely useful at least occasionally.
>
> Yep, python's autodoc has such a knob to use the debugging log for
> this. I just want to point out that avoiding the intermediate file
> on-disk is actually the mechanism by which I can preserve source
> lines, so this is how it's gotta be.
>
> I build an intermediate doc in-memory with source filenames and source
> lines along with the modified doc block content so that ReST errors
> can be tracked back directly to the QAPI json files. If we saved to
> disk and parsed that, it'd obliterate that information.
>
> >
> > >> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
> > >>   qapi-domain extension implements the custom directives
> > >
> > > Yes.
> > >
> > >
> > >> This mirrors how Sphinx works for Python docs.  Which is its original
> > >> use case.
> > >>
> > >> Your series demonstrates the second step, with test input you wrote
> > >> manually.
> > >>
> > >> You have code for the first step, but you'd prefer to show it later.
> > >
> > > Right, it's not fully finished, although I have events, commands, and
> > > objects working. Unions, Alternates and Events need work.
> > >
> > >
> > >> Fair?
> > >
> > > Bingo!
> >
> > Thanks!
> >
> > >> > The purpose of sending this series in its current form is largely to
> > >> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> > >> > a wily beast, and feedback at this stage will dictate how and where
> > >> > certain features are implemented.
> > >>
> > >> I'd appreciate help with that.  Opinions?
> > >
> > >
> > >> > A goal for this syntax (and the generator) is to fully in-line all
> > >> > command/event/object members, inherited or local, boxed or not, 
> > >> > branched
> > >> > or not. This should provide a boon to using these docs as a reference,
> > >> > because users will not have to grep around the page looking for various
> > >> > types, branches, or inherited members. Any arguments types will be
> > >> > hyperlinked to their definition, further aiding usability. Commands can
> > >> > be hotlinked from anywhere else in the manual, and will provide a
> > >> > complete reference directly on the first screenful of information.
> > >>
> > >> Let me elaborate a bit here.
> > >>
> > >> A command's arguments can be specified inline, like so:
> > >>
> > >> { 'command': 'job-cancel', 'data': { 'id': 'str' } }
> > >>
> > >> The arguments are then documented right

Re: [PATCH RESEND 0/2] Fix crash of VMs configured with the CDROM device

2024-04-22 Thread Yong Huang

Ping.
I would appreciate comments on this series. Thanks,

Yong

On Mon, Apr 8, 2024 at 8:08 PM Hyman Huang  wrote:

> This patchset fixes the crash of VMs configured with the CDROM device
> on the destination during live migration. See the commit message for
> details.
>
> The previous patchset does not show up at https://patchew.org/QEMU.
> Just resend it to ensure the email gets to the inbox.
>
> Please review.
>
> Yong
>
> Hyman Huang (2):
>   scsi-disk: Introduce the migrate_emulate_scsi_request field
>   scsi-disk: Fix crash of VMs configured with the CDROM device
>
>  hw/scsi/scsi-disk.c | 35 ++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> --
> 2.39.3
>
>

-- 
Best regards

[PATCH 6.6.y] virtio_net: Do not send RSS key if it is not supported

2024-04-22 Thread Vlad Poenaru

From: Breno Leitao 

commit 059a49aa2e25c58f90b50151f109dd3c4cdb3a47 upstream.

There is a bug when setting the RSS options in virtio_net that can break
the whole machine, getting the kernel into an infinite loop.

Running the following command in any QEMU virtual machine with virtionet
will reproduce this problem:

# ethtool -X eth0  hfunc toeplitz

This is how the problem happens:

1) ethtool_set_rxfh() calls virtnet_set_rxfh()

2) virtnet_set_rxfh() calls virtnet_commit_rss_command()

3) virtnet_commit_rss_command() populates 4 entries for the rss
scatter-gather

4) Since the command above does not have a key, then the last
scatter-gatter entry will be zeroed, since rss_key_size == 0.
sg_buf_size = vi->rss_key_size;

5) This buffer is passed to qemu, but qemu is not happy with a buffer
with zero length, and do the following in virtqueue_map_desc() (QEMU
function):

  if (!sz) {
  virtio_error(vdev, "virtio: zero sized buffers are not allowed");

6) virtio_error() (also QEMU function) set the device as broken

vdev->broken = true;

7) Qemu bails out, and do not repond this crazy kernel.

8) The kernel is waiting for the response to come back (function
virtnet_send_command())

9) The kernel is waiting doing the following :

  while (!virtqueue_get_buf(vi->cvq, ) &&
 !virtqueue_is_broken(vi->cvq))
  cpu_relax();

10) None of the following functions above is true, thus, the kernel
loops here forever. Keeping in mind that virtqueue_is_broken() does
not look at the qemu `vdev->broken`, so, it never realizes that the
vitio is broken at QEMU side.

Fix it by not sending RSS commands if the feature is not available in
the device.

Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Cc: sta...@vger.kernel.org
Cc: qemu-devel@nongnu.org
Signed-off-by: Breno Leitao 
Reviewed-by: Heng Qi 
Reviewed-by: Xuan Zhuo 
Signed-off-by: David S. Miller 
Signed-off-by: Vlad Poenaru 
---
 drivers/net/virtio_net.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7cb0548d17a3..56cbe00126bb 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3570,19 +3570,34 @@ static int virtnet_get_rxfh(struct net_device *dev, u32 
*indir, u8 *key, u8 *hfu
 static int virtnet_set_rxfh(struct net_device *dev, const u32 *indir, const u8 
*key, const u8 hfunc)
 {
struct virtnet_info *vi = netdev_priv(dev);
+   bool update = false;
int i;
 
if (hfunc != ETH_RSS_HASH_NO_CHANGE && hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
 
if (indir) {
+   if (!vi->has_rss)
+   return -EOPNOTSUPP;
+
for (i = 0; i < vi->rss_indir_table_size; ++i)
vi->ctrl->rss.indirection_table[i] = indir[i];
+   update = true;
}
-   if (key)
+   if (key) {
+   /* If either _F_HASH_REPORT or _F_RSS are negotiated, the
+* device provides hash calculation capabilities, that is,
+* hash_key is configured.
+*/
+   if (!vi->has_rss && !vi->has_rss_hash_report)
+   return -EOPNOTSUPP;
+
memcpy(vi->ctrl->rss.key, key, vi->rss_key_size);
+   update = true;
+   }
 
-   virtnet_commit_rss_command(vi);
+   if (update)
+   virtnet_commit_rss_command(vi);
 
return 0;
 }
@@ -4491,13 +4506,15 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
vi->has_rss_hash_report = true;
 
-   if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
+   if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
vi->has_rss = true;
 
-   if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
+   }
+
+   if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct virtio_net_config, 
rss_max_key_size));
 
-- 
2.43.0

[PATCH v4] hw/audio/virtio-snd: Always use little endian audio format

The VIRTIO Sound Device conforms with the Virtio spec v1.2,
thus only use little endianness.

Remove the suspicious target_words_bigendian() noticed during
code review.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
Supersedes: <20240422142056.3023-1-phi...@linaro.org>
v4: always LE (MST)
---
 hw/audio/virtio-snd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..ba4fff7302 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -24,7 +24,6 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "hw/audio/virtio-snd.h"
-#include "hw/core/cpu.h"
 
 #define VIRTIO_SOUND_VM_VERSION 1
 #define VIRTIO_SOUND_JACK_DEFAULT 0
@@ -401,7 +400,7 @@ static void virtio_snd_get_qemu_audsettings(audsettings *as,
 as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
 as->fmt = virtio_snd_get_qemu_format(params->format);
 as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = 0; /* Conforming to VIRTIO 1.0: always little endian. */
 }
 
 /*
-- 
2.41.0

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

2024-04-22 Thread Michael S. Tsirkin

On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > Since VirtIO devices can change endianness at runtime,
> > > we need to use the device endianness, not the target
> > > one.
> > > 
> > > Cc: qemu-sta...@nongnu.org
> > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > Signed-off-by: Philippe Mathieu-Daudé 
> > 
> > 
> > 
> > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > It is unconditionally little endian.
> 
> Oh, then the fix is as simple as:
> 
>  -as->endianness = target_words_bigendian() ? 1 : 0;
>  +as->endianness = 0; /* VIRTIO 1.0: always LE. */

Makes sense. Pls clarify in commit log whether the incorrect
value leads to any failures or this was found by code review.

> > If it's not it's a guest bug pls just fix it there.
> > 
> > 
> > > ---
> > > v2: Use virtio_is_big_endian()
> > > v3: Remove "hw/core/cpu.h
> > > ---
> > >   hw/audio/virtio-snd.c | 9 +
> > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> > > index c80b58bf5d..939cd78026 100644
> > > --- a/hw/audio/virtio-snd.c
> > > +++ b/hw/audio/virtio-snd.c
> > > @@ -24,7 +24,6 @@
> > >   #include "trace.h"
> > >   #include "qapi/error.h"
> > >   #include "hw/audio/virtio-snd.h"
> > > -#include "hw/core/cpu.h"
> > >   #define VIRTIO_SOUND_VM_VERSION 1
> > >   #define VIRTIO_SOUND_JACK_DEFAULT 0
> > > @@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t 
> > > rate)
> > >* Get QEMU Audiosystem compatible audsettings from virtio based pcm 
> > > stream
> > >* params.
> > >*/
> > > -static void virtio_snd_get_qemu_audsettings(audsettings *as,
> > > +static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings 
> > > *as,
> > >   virtio_snd_pcm_set_params 
> > > *params)
> > >   {
> > > +VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > > +
> > >   as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
> > >   as->fmt = virtio_snd_get_qemu_format(params->format);
> > >   as->freq = virtio_snd_get_qemu_freq(params->rate);
> > > -as->endianness = target_words_bigendian() ? 1 : 0;
> > > +as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
> > >   }
> > >   /*
> > > @@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound 
> > > *s, uint32_t stream_id)
> > >   s->pcm->streams[stream_id] = stream;
> > >   }
> > > -virtio_snd_get_qemu_audsettings(, params);
> > > +virtio_snd_get_qemu_audsettings(s, , params);
> > >   stream->info.direction = stream_id < s->snd_conf.streams / 2 +
> > >   (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : 
> > > VIRTIO_SND_D_INPUT;
> > >   stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
> > > -- 
> > > 2.41.0
> >

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one


On 22/4/24 23:02, Michael S. Tsirkin wrote:

On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 




This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.


Oh, then the fix is as simple as:

 -as->endianness = target_words_bigendian() ? 1 : 0;
 +as->endianness = 0; /* VIRTIO 1.0: always LE. */


If it's not it's a guest bug pls just fix it there.



---
v2: Use virtio_is_big_endian()
v3: Remove "hw/core/cpu.h
---
  hw/audio/virtio-snd.c | 9 +
  1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..939cd78026 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -24,7 +24,6 @@
  #include "trace.h"
  #include "qapi/error.h"
  #include "hw/audio/virtio-snd.h"
-#include "hw/core/cpu.h"
  
  #define VIRTIO_SOUND_VM_VERSION 1

  #define VIRTIO_SOUND_JACK_DEFAULT 0
@@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
   * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
   * params.
   */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
  virtio_snd_pcm_set_params *params)
  {
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
  as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
  as->fmt = virtio_snd_get_qemu_format(params->format);
  as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
  }
  
  /*

@@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
  s->pcm->streams[stream_id] = stream;
  }
  
-virtio_snd_get_qemu_audsettings(, params);

+virtio_snd_get_qemu_audsettings(s, , params);
  stream->info.direction = stream_id < s->snd_conf.streams / 2 +
  (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
  stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
--
2.41.0

Re: [PATCH] target/arm: Restrict translation disabled alignment check to VMSA


On 22/4/24 19:09, Richard Henderson wrote:

On 4/22/24 10:07, Richard Henderson wrote:

For cpus using PMSA, when the MPU is disabled, the default memory
type is Normal, Non-cachable.

Fixes: 59754f85ed3 ("target/arm: Do memory type alignment check when 
translation disabled")

Reported-by: Clément Chigot 
Signed-off-by: Richard Henderson 
---

Since v9 will likely be tagged tomorrow without this fixed,
Cc: qemu-sta...@nongnu.org

---
  target/arm/tcg/hflags.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 5da1b0fc1d..66de30b828 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -38,8 +38,16 @@ static bool aprofile_require_alignment(CPUARMState 
*env, int el, uint64_t sctlr)

  }
  /*
- * If translation is disabled, then the default memory type is
- * Device(-nGnRnE) instead of Normal, which requires that alignment
+ * With PMSA, when the MPU is disabled, all memory types in the
+ * default map is Normal.
+ */
+    if (arm_feature(env, ARM_FEATURE_PMSA)) {
+    return false;
+    }
+
+    /*
+ * With VMSA, if translation is disabled, then the default memory 
type
+ * is Device(-nGnRnE) instead of Normal, which requires that 
alignment

   * be enforced.  Since this affects all ram, it is most efficient
   * to handle this during translation.
   */


Oh, I meant to add: since the armv7 manual has both VMSA and PMSA 
sections, and the language about default Device type and alignment 
traps, is in the VMSA section.


To the best of my knowledge,
Reviewed-by: Philippe Mathieu-Daudé

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

2024-04-22 Thread Michael S. Tsirkin

On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> Since VirtIO devices can change endianness at runtime,
> we need to use the device endianness, not the target
> one.
> 
> Cc: qemu-sta...@nongnu.org
> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> Signed-off-by: Philippe Mathieu-Daudé 



This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
If it's not it's a guest bug pls just fix it there.


> ---
> v2: Use virtio_is_big_endian()
> v3: Remove "hw/core/cpu.h
> ---
>  hw/audio/virtio-snd.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index c80b58bf5d..939cd78026 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -24,7 +24,6 @@
>  #include "trace.h"
>  #include "qapi/error.h"
>  #include "hw/audio/virtio-snd.h"
> -#include "hw/core/cpu.h"
>  
>  #define VIRTIO_SOUND_VM_VERSION 1
>  #define VIRTIO_SOUND_JACK_DEFAULT 0
> @@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
>   * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
>   * params.
>   */
> -static void virtio_snd_get_qemu_audsettings(audsettings *as,
> +static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
>  virtio_snd_pcm_set_params 
> *params)
>  {
> +VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +
>  as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
>  as->fmt = virtio_snd_get_qemu_format(params->format);
>  as->freq = virtio_snd_get_qemu_freq(params->rate);
> -as->endianness = target_words_bigendian() ? 1 : 0;
> +as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
>  }
>  
>  /*
> @@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
> uint32_t stream_id)
>  s->pcm->streams[stream_id] = stream;
>  }
>  
> -virtio_snd_get_qemu_audsettings(, params);
> +virtio_snd_get_qemu_audsettings(s, , params);
>  stream->info.direction = stream_id < s->snd_conf.streams / 2 +
>  (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
>  stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
> -- 
> 2.41.0

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

On Mon, 22 Apr 2024 17:20, Philippe Mathieu-Daudé  
wrote:

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---


Reviewed-by: Manos Pitsidianakis 

Thanks for the explanation on v2 btw. virtio_is_big_endian()'s doc 
should probably reflect it's not just about legacy devices (virtio sound 
isn't legacy) but about target originating data streams too

[PATCH 3/4] hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()

Given that memory_region_set_readonly() is a no-op when the readonlyness is
already as requested it is possible to simplify the pattern

  if (condition) {
foo(true);
  }

to

  foo(condition);

which is shorter and allows to see the invariant of the code more easily.

Signed-off-by: Bernhard Beschow 
---
 hw/i386/x86.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ffbda48917..32cd22a4a8 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1171,9 +1171,7 @@ void x86_bios_rom_init(MachineState *ms, const char 
*default_firmware,
 load_image_size(filename, ptr, bios_size);
 x86_firmware_configure(ptr, bios_size);
 } else {
-if (!isapc_ram_fw) {
-memory_region_set_readonly(bios, true);
-}
+memory_region_set_readonly(bios, !isapc_ram_fw);
 ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1);
 if (ret != 0) {
 goto bios_error;
@@ -1190,9 +1188,7 @@ void x86_bios_rom_init(MachineState *ms, const char 
*default_firmware,
 0x10 - isa_bios_size,
 isa_bios,
 1);
-if (!isapc_ram_fw) {
-memory_region_set_readonly(isa_bios, true);
-}
+memory_region_set_readonly(isa_bios, !isapc_ram_fw);
 
 /* map all the bios at the top of memory */
 memory_region_add_subregion(rom_memory,
-- 
2.44.0

[PATCH 0/4] X86: Alias isa-bios area and clean up

This series changes the "isa-bios" MemoryRegion to be an alias rather than a
copy in the pflash case. This fixes issuing pflash commands in the isa-bios
region which matches real hardware and which some real-world legacy bioses I'm
running rely on. Furthermore, aliasing in the isa-bios area is already the
current behavior in the bios (a.k.a. ROM) case, so this series consolidates
behavior.

The consolidateion results in duplicate code which is resolved in the second
half (patches 3 and 4) in this series.

Question: AFAIU, patch 2 changes the behavior for SEV-enabled guests since the
isa-bios area is now encrypted. Does this need compat machinery or is it a
bugfix?

Testing done:
* `make check` with qemu-system-x86_64 (QEMU 8.2.2) installed. All tests
  including migration tests pass.
* `make check-avocado`

Best regards,
Bernhard

Bernhard Beschow (4):
  hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()
  hw/i386/pc_sysfw: Alias rather than copy isa-bios region
  hw/i386/x86: Eliminate two if statements in x86_bios_rom_init()
  hw/i386: Consolidate isa-bios creation

 include/hw/i386/x86.h |  2 ++
 hw/i386/pc_sysfw.c| 38 --
 hw/i386/x86.c | 35 +++
 3 files changed, 25 insertions(+), 50 deletions(-)

-- 
2.44.0

[PATCH 4/4] hw/i386: Consolidate isa-bios creation

Now that the -bios and -pflash code paths work the same it is possible to have a
common implementation.

While at it convert the magic number 0x10 (== 1MiB) to increase readability.

Signed-off-by: Bernhard Beschow 
---
 include/hw/i386/x86.h |  2 ++
 hw/i386/pc_sysfw.c| 28 
 hw/i386/x86.c | 29 ++---
 3 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 4dc30dcb4d..8e6ba4a726 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -116,6 +116,8 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
 void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp);
 
+void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios,
+   bool isapc_ram_fw);
 void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
MemoryRegion *rom_memory, bool isapc_ram_fw);
 
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 6e89671c26..e529182b48 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -28,7 +28,6 @@
 #include "sysemu/block-backend.h"
 #include "qemu/error-report.h"
 #include "qemu/option.h"
-#include "qemu/units.h"
 #include "hw/sysbus.h"
 #include "hw/i386/x86.h"
 #include "hw/i386/pc.h"
@@ -40,27 +39,6 @@
 
 #define FLASH_SECTOR_SIZE 4096
 
-static void pc_isa_bios_init(MemoryRegion *rom_memory,
- MemoryRegion *flash_mem)
-{
-int isa_bios_size;
-MemoryRegion *isa_bios;
-uint64_t flash_size;
-
-flash_size = memory_region_size(flash_mem);
-
-/* map the last 128KB of the BIOS in ISA space */
-isa_bios_size = MIN(flash_size, 128 * KiB);
-isa_bios = g_malloc(sizeof(*isa_bios));
-memory_region_init_alias(isa_bios, NULL, "isa-bios", flash_mem,
- flash_size - isa_bios_size, isa_bios_size);
-memory_region_add_subregion_overlap(rom_memory,
-0x10 - isa_bios_size,
-isa_bios,
-1);
-memory_region_set_readonly(isa_bios, true);
-}
-
 static PFlashCFI01 *pc_pflash_create(PCMachineState *pcms,
  const char *name,
  const char *alias_prop_name)
@@ -121,7 +99,7 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms)
  * pcms->max_fw_size.
  *
  * If pcms->flash[0] has a block backend, its memory is passed to
- * pc_isa_bios_init().  Merging several flash devices for isa-bios is
+ * x86_isa_bios_init(). Merging several flash devices for isa-bios is
  * not supported.
  */
 static void pc_system_flash_map(PCMachineState *pcms,
@@ -176,7 +154,9 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
 if (i == 0) {
 flash_mem = pflash_cfi01_get_memory(system_flash);
-pc_isa_bios_init(rom_memory, flash_mem);
+
+/* Map the last 128KB of the BIOS in ISA space */
+x86_isa_bios_init(rom_memory, flash_mem, false);
 
 /* Encrypt the pflash boot ROM */
 if (sev_enabled()) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 32cd22a4a8..7366b0cee4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1136,13 +1136,28 @@ void x86_load_linux(X86MachineState *x86ms,
 nb_option_roms++;
 }
 
+void x86_isa_bios_init(MemoryRegion *rom_memory, MemoryRegion *bios,
+   bool isapc_ram_fw)
+{
+int bios_size = memory_region_size(bios);
+int isa_bios_size = MIN(bios_size, 128 * KiB);
+MemoryRegion *isa_bios;
+
+isa_bios = g_malloc(sizeof(*isa_bios));
+memory_region_init_alias(isa_bios, NULL, "isa-bios", bios,
+ bios_size - isa_bios_size, isa_bios_size);
+memory_region_add_subregion_overlap(rom_memory, 1 * MiB - isa_bios_size,
+isa_bios, 1);
+memory_region_set_readonly(isa_bios, !isapc_ram_fw);
+}
+
 void x86_bios_rom_init(MachineState *ms, const char *default_firmware,
MemoryRegion *rom_memory, bool isapc_ram_fw)
 {
 const char *bios_name;
 char *filename;
-MemoryRegion *bios, *isa_bios;
-int bios_size, isa_bios_size;
+MemoryRegion *bios;
+int bios_size;
 ssize_t ret;
 
 /* BIOS load */
@@ -1180,15 +1195,7 @@ void x86_bios_rom_init(MachineState *ms, const char 
*default_firmware,
 g_free(filename);
 
 /* map the last 128KB of the BIOS in ISA space */
-isa_bios_size = MIN(bios_size, 128 * KiB);
-isa_bios = g_malloc(sizeof(*isa_bios));
-memory_region_init_alias(isa_bios, NULL, "isa-bios", bios,
- bios_size - isa_bios_size, isa_bios_size);
-memory_region_add_subregion_overlap(rom_memory,
-0x10 - isa_bios_size,
-

[PATCH 2/4] hw/i386/pc_sysfw: Alias rather than copy isa-bios region

In the -bios case the "isa-bios" memory region is an alias to the BIOS mapped
to the top of the 4G memory boundary. Do the same in the -pflash case to have
common behavior. This also makes pflash commands work in the "isa-bios" region
which some real-world legacy bioses rely on.

Note that in the sev_enabled() case, the "isa-bios" memory region in the -pflash
case will now also point to encrypted memory, just like it already does in the
-bios case.

Signed-off-by: Bernhard Beschow 
---
 hw/i386/pc_sysfw.c | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 87b5bf59d6..6e89671c26 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -46,27 +46,18 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
 int isa_bios_size;
 MemoryRegion *isa_bios;
 uint64_t flash_size;
-void *flash_ptr, *isa_bios_ptr;
 
 flash_size = memory_region_size(flash_mem);
 
 /* map the last 128KB of the BIOS in ISA space */
 isa_bios_size = MIN(flash_size, 128 * KiB);
 isa_bios = g_malloc(sizeof(*isa_bios));
-memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size,
-   _fatal);
+memory_region_init_alias(isa_bios, NULL, "isa-bios", flash_mem,
+ flash_size - isa_bios_size, isa_bios_size);
 memory_region_add_subregion_overlap(rom_memory,
 0x10 - isa_bios_size,
 isa_bios,
 1);
-
-/* copy ISA rom image from top of flash memory */
-flash_ptr = memory_region_get_ram_ptr(flash_mem);
-isa_bios_ptr = memory_region_get_ram_ptr(isa_bios);
-memcpy(isa_bios_ptr,
-   ((uint8_t*)flash_ptr) + (flash_size - isa_bios_size),
-   isa_bios_size);
-
 memory_region_set_readonly(isa_bios, true);
 }
 
-- 
2.44.0

[PATCH 1/4] hw/i386/pc_sysfw: Remove unused parameter from pc_isa_bios_init()

Signed-off-by: Bernhard Beschow 
---
 hw/i386/pc_sysfw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 3efabbbab2..87b5bf59d6 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -41,8 +41,7 @@
 #define FLASH_SECTOR_SIZE 4096
 
 static void pc_isa_bios_init(MemoryRegion *rom_memory,
- MemoryRegion *flash_mem,
- int ram_size)
+ MemoryRegion *flash_mem)
 {
 int isa_bios_size;
 MemoryRegion *isa_bios;
@@ -186,7 +185,7 @@ static void pc_system_flash_map(PCMachineState *pcms,
 
 if (i == 0) {
 flash_mem = pflash_cfi01_get_memory(system_flash);
-pc_isa_bios_init(rom_memory, flash_mem, size);
+pc_isa_bios_init(rom_memory, flash_mem);
 
 /* Encrypt the pflash boot ROM */
 if (sev_enabled()) {
-- 
2.44.0

Re: [PATCH] target/riscv: change RISCV_EXCP_SEMIHOST exception number to 63

On 4/22/24 16:44, Richard Henderson wrote:

On 4/22/24 10:45, Daniel Henrique Barboza wrote:

Palmer, Anup,

On 4/22/24 10:58, Clément Léger wrote:

The current semihost exception number (16) is a reserved number (range
[16-17]). The upcoming double trap specification uses that number for
the double trap exception. Since the privileged spec (Table 22) defines
ranges for custom uses change the semihosting exception number to 63
which belongs to the range [48-63] in order to avoid any future
collisions with reserved exception.

I didn't find any reference to a number for the SEMIHOST exception here:

https://github.com/riscv-non-isa/riscv-semihosting

Do we have any potential candidates? I would like to avoid, if possible, setting
RISCV_EXCP_SEMIHOST to 63 as a band-aid just to replace it later on by the real
value.

RISCV_EXCP_SEMIHOST is internal to the qemu implementation and will never be
delivered to the guest.

I suggest using a number high in the >64 reserved range which will (likely) never be used
by any implementation, including ones that *do* define implementation-specific exceptions.
Which seems more likely than not within the "implementation defined" range.

E.g. target/i386 uses 0x100+n for qemu internal exceptions.

I'm not sure if we have a range for risc-v qemu internal exceptions only. IIRC
we don't.

If that's really the case I believe we could use whatever i386/ARM uses. At
least we'll have some
standardization.

Thanks,

Daniel

But in any case, the number can be redefined at will and not cause
compatibility issues.

Re: [PATCH] target/riscv: change RISCV_EXCP_SEMIHOST exception number to 63


On 4/22/24 10:45, Daniel Henrique Barboza wrote:

Palmer, Anup,

On 4/22/24 10:58, Clément Léger wrote:

The current semihost exception number (16) is a reserved number (range
[16-17]). The upcoming double trap specification uses that number for
the double trap exception. Since the privileged spec (Table 22) defines
ranges for custom uses change the semihosting exception number to 63
which belongs to the range [48-63] in order to avoid any future
collisions with reserved exception.



I didn't find any reference to a number for the SEMIHOST exception here:


https://github.com/riscv-non-isa/riscv-semihosting


Do we have any potential candidates? I would like to avoid, if possible, setting
RISCV_EXCP_SEMIHOST to 63 as a band-aid just to replace it later on by the real
value.


RISCV_EXCP_SEMIHOST is internal to the qemu implementation and will never be delivered to 
the guest.


I suggest using a number high in the >64 reserved range which will (likely) never be used 
by any implementation, including ones that *do* define implementation-specific exceptions. 
 Which seems more likely than not within the "implementation defined" range.


E.g. target/i386 uses 0x100+n for qemu internal exceptions.

But in any case, the number can be redefined at will and not cause 
compatibility issues.


r~

Re: [PATCH for-9.1] util/log: add cleanup function

2024-04-22 Thread Stefan Hajnoczi

On Wed, Apr 17, 2024 at 09:33:33PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We leak global_filename, and do not close global_file. Let's fix that.

What is the goal?

Leaking global_filename does not cause unbounded memory consumption. I
guess the goal in freeing global_filename is to keep leak checker
reports tidy?

Closing global_file doesn't improve anything AFAICT. It might cause
problems if another component still wants to log something from a
destructor function. I'm not sure if the order of destructors is
defined.

What about qemu_mutex_destroy(_mutex) to balance startup()?

What about debug_regions?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 
> Interesting: seems, nobody is maintainer of util/log.c
> 
>  util/log.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/util/log.c b/util/log.c
> index d36c98da0b..30de209210 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -85,6 +85,15 @@ static void qemu_log_thread_cleanup(Notifier *n, void 
> *unused)
>  }
>  }
>  
> +static void __attribute__((__destructor__)) cleanup(void)
> +{
> +g_free(global_filename);
> +if (global_file && global_file != stderr) {
> +fclose(global_file);
> +global_file = NULL;
> +}
> +}
> +
>  /* Lock/unlock output. */
>  
>  static FILE *qemu_log_trylock_with_err(Error **errp)
> -- 
> 2.34.1
> 

signature.asc
Description: PGP signature

Re: [PATCH] target/i386/translate.c: always write 32-bits for SGDT and SIDT

2024-04-22 Thread Volker Rümelin

Am 20.04.24 um 07:40 schrieb Mark Cave-Ayland:
> On 20/04/2024 02:21, Richard Henderson wrote:
>
>> On 4/19/24 12:51, Mark Cave-Ayland wrote:
>>> The various Intel CPU manuals claim that SGDT and SIDT can write
>>> either 24-bits
>>> or 32-bits depending upon the operand size, but this is incorrect.
>>> Not only do
>>> the Intel CPU manuals give contradictory information between processor
>>> revisions, but this information doesn't even match real-life behaviour.
>>>
>>> In fact, tests on real hardware show that the CPU always writes
>>> 32-bits for SGDT
>>> and SIDT, and this behaviour is required for at least OS/2 Warp and
>>> WFW 3.11 with
>>> Win32s to function correctly. Remove the masking applied due to the
>>> operand size
>>> for SGDT and SIDT so that the TCG behaviour matches the behaviour on
>>> real
>>> hardware.
>>>
>>> Signed-off-by: Mark Cave-Ayland 
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2198
>>>
>>> -- 
>>> MCA: Whilst I don't have a copy of OS/2 Warp handy, I've confirmed
>>> that this
>>> patch fixes the issue in WFW 3.11 with Win32s. For more technical
>>> information I
>>> highly recommend the excellent write-up at
>>> https://www.os2museum.com/wp/sgdtsidt-fiction-and-reality/.
>>> ---
>>>   target/i386/tcg/translate.c | 14 --
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
>>> index 76a42c679c..3026eb6774 100644
>>> --- a/target/i386/tcg/translate.c
>>> +++ b/target/i386/tcg/translate.c
>>> @@ -5846,9 +5846,10 @@ static bool disas_insn(DisasContext *s,
>>> CPUState *cpu)
>>>   gen_op_st_v(s, MO_16, s->T0, s->A0);
>>>   gen_add_A0_im(s, 2);
>>>   tcg_gen_ld_tl(s->T0, tcg_env, offsetof(CPUX86State,
>>> gdt.base));
>>> -    if (dflag == MO_16) {
>>> -    tcg_gen_andi_tl(s->T0, s->T0, 0xff);
>>> -    }
>>> +    /*
>>> + * NB: Despite claims to the contrary in Intel CPU
>>> documentation,
>>> + * all 32-bits are written regardless of operand size.
>>> + */
>>
>> Current documentation agrees that all 32 bits are written, so I don't
>> think you need this comment:
>
> Ah that's good to know the docs are now correct. I added the comment
> as there was a lot of conflicting information around for older CPUs so
> I thought it was worth an explicit mention.
>
> If everyone agrees a version without comments is preferable, I can
> re-send an updated version without them included.
>

Hi Mark,

I wouldn't remove the comment.

Quote from the Intel® 64 and IA-32 Architectures Software Developer’s
Manual Volume 2B: Instruction Set Reference, M-U March 2024:

IA-32 Architecture Compatibility
The 16-bit form of SGDT is compatible with the Intel 286 processor if
the upper 8 bits are not referenced. The Intel 286 processor fills these
bits with 1s; processor generations later than the Intel 286 processor
fill these bits with 0s.

Intel still claims the upper 8 bits are filled with 0s, but the
Operation pseudo code below is correct. The same is true for SIDT.

With best regards,
Volker

>>    IF OperandSize =16 or OperandSize = 32 (* Legacy or Compatibility
>> Mode *)
>>  THEN
>>    DEST[0:15] := GDTR(Limit);
>>    DEST[16:47] := GDTR(Base); (* Full 32-bit base address stored *)
>>    FI;
>>
>>
>> Anyway,
>> Reviewed-by: Richard Henderson 
>
> Thanks!
>
>
> ATB,
>
> Mark.
>
>
>

[PATCH v7 2/2] Implement SSH commands in QEMU GA for Windows

2024-04-22 Thread aidan_leuck

From: aidaleuc 

Signed-off-by: aidaleuc 
---
 qga/commands-windows-ssh.c | 712 +
 qga/commands-windows-ssh.h |  26 ++
 qga/meson.build|   7 +-
 qga/qapi-schema.json   |  17 +-
 4 files changed, 750 insertions(+), 12 deletions(-)
 create mode 100644 qga/commands-windows-ssh.c
 create mode 100644 qga/commands-windows-ssh.h

diff --git a/qga/commands-windows-ssh.c b/qga/commands-windows-ssh.c
new file mode 100644
index 00..6a642e3ba8
--- /dev/null
+++ b/qga/commands-windows-ssh.c
@@ -0,0 +1,712 @@
+/*
+ * QEMU Guest Agent win32-specific command implementations for SSH keys.
+ * The implementation is opinionated and expects the SSH implementation to
+ * be OpenSSH.
+ *
+ * Copyright Schweitzer Engineering Laboratories. 2024
+ *
+ * Authors:
+ *  Aidan Leuck 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+#include 
+
+#include "commands-common-ssh.h"
+#include "commands-windows-ssh.h"
+#include "guest-agent-core.h"
+#include "limits.h"
+#include "lmaccess.h"
+#include "lmapibuf.h"
+#include "lmerr.h"
+#include "qapi/error.h"
+
+#include "qga-qapi-commands.h"
+#include "sddl.h"
+#include "shlobj.h"
+#include "userenv.h"
+
+#define AUTHORIZED_KEY_FILE "authorized_keys"
+#define AUTHORIZED_KEY_FILE_ADMIN "administrators_authorized_keys"
+#define LOCAL_SYSTEM_SID "S-1-5-18"
+#define ADMIN_SID "S-1-5-32-544"
+
+/*
+ * Frees userInfo structure. This implements the g_auto cleanup
+ * for the structure.
+ */
+void free_userInfo(PWindowsUserInfo info)
+{
+g_free(info->sshDirectory);
+g_free(info->authorizedKeyFile);
+LocalFree(info->SSID);
+g_free(info->username);
+g_free(info);
+}
+
+/*
+ * Gets the admin SSH folder for OpenSSH. OpenSSH does not store
+ * the authorized_key file in the users home directory for security reasons and
+ * instead stores it at %PROGRAMDATA%/ssh. This function returns the path to
+ * that directory on the users machine
+ *
+ * parameters:
+ * errp -> error structure to set when an error occurs
+ * returns: The path to the ssh folder in %PROGRAMDATA% or NULL if an error
+ * occurred.
+ */
+static char *get_admin_ssh_folder(Error **errp)
+{
+/* Allocate memory for the program data path */
+g_autofree char *programDataPath = NULL;
+char *authkeys_path = NULL;
+PWSTR pgDataW = NULL;
+g_autoptr(GError) gerr = NULL;
+
+/* Get the KnownFolderPath on the machine. */
+HRESULT folderResult =
+SHGetKnownFolderPath(_ProgramData, 0, NULL, );
+if (folderResult != S_OK) {
+error_setg(errp, "Failed to retrieve ProgramData folder");
+return NULL;
+}
+
+/* Convert from a wide string back to a standard character string. */
+programDataPath = g_utf16_to_utf8(pgDataW, -1, NULL, NULL, );
+CoTaskMemFree(pgDataW);
+if (!programDataPath) {
+error_setg(errp,
+   "Failed converting ProgramData folder path to UTF-16 %s",
+   gerr->message);
+return NULL;
+}
+
+/* Build the path to the file. */
+authkeys_path = g_build_filename(programDataPath, "ssh", NULL);
+return authkeys_path;
+}
+
+/*
+ * Gets the path to the SSH folder for the specified user. If the user is an
+ * admin it returns the ssh folder located at %PROGRAMDATA%/ssh. If the user is
+ * not an admin it returns %USERPROFILE%/.ssh
+ *
+ * parameters:
+ * username -> Username to get the SSH folder for
+ * isAdmin -> Whether the user is an admin or not
+ * errp -> Error structure to set any errors that occur.
+ * returns: path to the ssh folder as a string.
+ */
+static char *get_ssh_folder(const char *username, const bool isAdmin,
+Error **errp)
+{
+DWORD maxSize = MAX_PATH;
+g_autofree char *profilesDir = g_new0(char, maxSize);
+
+if (isAdmin) {
+return get_admin_ssh_folder(errp);
+}
+
+/* If not an Admin the SSH key is in the user directory. */
+/* Get the user profile directory on the machine. */
+BOOL ret = GetProfilesDirectory(profilesDir, );
+if (!ret) {
+error_setg_win32(errp, GetLastError(),
+ "failed to retrieve profiles directory");
+return NULL;
+}
+
+/* Builds the filename */
+return g_build_filename(profilesDir, username, ".ssh", NULL);
+}
+
+/*
+ * Creates an entry for the user so they can access the ssh folder in their
+ * userprofile.
+ *
+ * parameters:
+ * userInfo -> Information about the current user
+ * pACL -> Pointer to an ACL structure
+ * errp -> Error structure to set any errors that occur
+ * returns -> 1 on success, 0 otherwise
+ */
+static bool create_acl_user(PWindowsUserInfo userInfo, PACL *pACL, Error 
**errp)
+{
+const int aclSize = 1;
+PACL newACL = NULL;
+EXPLICIT_ACCESS eAccess[1];
+PSID userPSID = NULL;
+
+/* Get a pointer to the internal

[PATCH v7 1/2] Refactor common functions between POSIX and Windows implementation

2024-04-22 Thread aidan_leuck

From: aidaleuc 

Signed-off-by: aidaleuc 
---
 qga/commands-common-ssh.c | 50 +++
 qga/commands-common-ssh.h | 10 
 qga/commands-posix-ssh.c  | 47 +---
 qga/meson.build   |  1 +
 4 files changed, 62 insertions(+), 46 deletions(-)
 create mode 100644 qga/commands-common-ssh.c
 create mode 100644 qga/commands-common-ssh.h

diff --git a/qga/commands-common-ssh.c b/qga/commands-common-ssh.c
new file mode 100644
index 00..537869fb98
--- /dev/null
+++ b/qga/commands-common-ssh.c
@@ -0,0 +1,50 @@
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "commands-common-ssh.h"
+
+GStrv read_authkeys(const char *path, Error **errp)
+{
+g_autoptr(GError) err = NULL;
+g_autofree char *contents = NULL;
+
+if (!g_file_get_contents(path, , NULL, )) {
+error_setg(errp, "failed to read '%s': %s", path, err->message);
+return NULL;
+}
+
+return g_strsplit(contents, "\n", -1);
+}
+
+bool check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
+{
+size_t n = 0;
+strList *k;
+
+for (k = keys; k != NULL; k = k->next) {
+if (!check_openssh_pub_key(k->value, errp)) {
+return false;
+}
+n++;
+}
+
+if (nkeys) {
+*nkeys = n;
+}
+return true;
+}
+
+bool check_openssh_pub_key(const char *key, Error **errp)
+{
+/* simple sanity-check, we may want more? */
+if (!key || key[0] == '#' || strchr(key, '\n')) {
+error_setg(errp, "invalid OpenSSH public key: '%s'", key);
+return false;
+}
+
+return true;
+}
diff --git a/qga/commands-common-ssh.h b/qga/commands-common-ssh.h
new file mode 100644
index 00..14d955fa84
--- /dev/null
+++ b/qga/commands-common-ssh.h
@@ -0,0 +1,10 @@
+/*
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qapi/qapi-builtin-types.h"
+
+GStrv read_authkeys(const char *path, Error **errp);
+bool check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp);
+bool check_openssh_pub_key(const char *key, Error **errp);
diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index 236f80de44..dd2ecb453a 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -9,6 +9,7 @@
 #include 
 #include 
 
+#include "commands-common-ssh.h"
 #include "qapi/error.h"
 #include "qga-qapi-commands.h"
 
@@ -80,37 +81,6 @@ mkdir_for_user(const char *path, const struct passwd *p,
 return true;
 }
 
-static bool
-check_openssh_pub_key(const char *key, Error **errp)
-{
-/* simple sanity-check, we may want more? */
-if (!key || key[0] == '#' || strchr(key, '\n')) {
-error_setg(errp, "invalid OpenSSH public key: '%s'", key);
-return false;
-}
-
-return true;
-}
-
-static bool
-check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
-{
-size_t n = 0;
-strList *k;
-
-for (k = keys; k != NULL; k = k->next) {
-if (!check_openssh_pub_key(k->value, errp)) {
-return false;
-}
-n++;
-}
-
-if (nkeys) {
-*nkeys = n;
-}
-return true;
-}
-
 static bool
 write_authkeys(const char *path, const GStrv keys,
const struct passwd *p, Error **errp)
@@ -139,21 +109,6 @@ write_authkeys(const char *path, const GStrv keys,
 return true;
 }
 
-static GStrv
-read_authkeys(const char *path, Error **errp)
-{
-g_autoptr(GError) err = NULL;
-g_autofree char *contents = NULL;
-
-if (!g_file_get_contents(path, , NULL, )) {
-error_setg(errp, "failed to read '%s': %s", path, err->message);
-return NULL;
-}
-
-return g_strsplit(contents, "\n", -1);
-
-}
-
 void
 qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
   bool has_reset, bool reset,
diff --git a/qga/meson.build b/qga/meson.build
index 1c3d2a3d1b..4c3899751b 100644
--- a/qga/meson.build
+++ b/qga/meson.build
@@ -66,6 +66,7 @@ qga_ss.add(files(
   'guest-agent-command-state.c',
   'main.c',
   'cutils.c',
+  'commands-common-ssh.c'
 ))
 if host_os == 'windows'
   qga_ss.add(files(
-- 
2.44.0

[PATCH v7 0/2] Implement SSH commands in QEMU GA for Windows

2024-04-22 Thread aidan_leuck

From: aidaleuc 

Changes since v6
* Fixed issue with file permissions causing public keys to be rejected by the 
SSH server. credit (kkostiuk)
Older versions of OpenSSH such as the one shipped with Windows Server 2022 are 
more stringent on 
file permissions.
* Removed everyone group from create_acl
* Disabled public key file inheritance from the parent folder to control 
permissions in a more granular fashion

Changes since v5
* Fixed spurious formatting 

Changes since v4
* Moved qapi/error.h to commands-common-ssh.c
* Changed  to "qapi/qapi-builtin-types.h" 
* Removed stbool.h from commands-common-ssh.h

Changes since v3
* Renamed commands-ssh-core.c/h to commands-common-ssh.c/h
* Fixed styling errors discovered by checkpatch.pl 
* Moved some header includes to the commands-common-ssh.h

Changes since v2
* Set indent to 4 spaces
* Moved all comments to C style comments
* Fixed a segfault bug in get_user_info function related to non zeroed memory 
when a user did not exist.
* Used g_new0 instead of g_malloc where applicable
* Modified newlines in qapi-schema.json
* Added newlines at the end of all files
* GError functions now use g_autoptr instead of being freed manually.
* Refactored get_ssh_folder to remove goto error statement
* Fixed uninitialized variable pgDataW
* Modified patch order so that the generalization patch is the first patch
* Removed unnecssary ZeroMemory calls

Changes since v1
* Fixed styling errors
* Moved from wcstombs to g_utf functions
* Removed unnecessary if checks on calls to free
* Fixed copyright headers
* Refactored create_acl functions into base function, admin function and user 
function
* Removed unused user count function
* Split up refactor of existing code into a separate patch

aidaleuc (2):
  Refactor common functions between POSIX and Windows implementation
  Implement SSH commands in QEMU GA for Windows

 qga/commands-common-ssh.c  |  50 +++
 qga/commands-common-ssh.h  |  10 +
 qga/commands-posix-ssh.c   |  47 +--
 qga/commands-windows-ssh.c | 712 +
 qga/commands-windows-ssh.h |  26 ++
 qga/meson.build|   8 +-
 qga/qapi-schema.json   |  17 +-
 7 files changed, 812 insertions(+), 58 deletions(-)
 create mode 100644 qga/commands-common-ssh.c
 create mode 100644 qga/commands-common-ssh.h
 create mode 100644 qga/commands-windows-ssh.c
 create mode 100644 qga/commands-windows-ssh.h

-- 
2.44.0

RE: [PATCH v6 0/2] Implement SSH commands in QEMU GA for Windows

2024-04-22 Thread Aidan Leuck

Hi Konstantin,

Thank you for taking the time to look over the patch and test it. I do expect 
this to work when installing OpenSSH by Windows features. I have been testing 
my implementation on Windows 11 which uses a newer version of OpenSSH server 
than the one shipped with Windows Server 2022. I was able to get a hold of a 
Windows Server VM and I was able to reproduce what you are describing. I will 
have a patch coming out shortly with a fix for the issue. I tested the new 
patch on both Windows Server 2022 (installed from Windows Features) Windows 11 
(Installed From Windows Features) and the latest beta release from OpenSSH on 
Github. Everything appears to be working now. Let me know if you are still 
running into issues.

Aidan Leuck


From: Konstantin Kostiuk 
Sent: Monday, April 22, 2024 2:51 AM
To: Aidan Leuck 
Cc: qemu-devel@nongnu.org; phi...@linaro.org; Dehan Meng 
Subject: Re: [PATCH v6 0/2] Implement SSH commands in QEMU GA for Windows

[Caution - External]
Hi Aidan,

I tried these patches with OpenSSH Server installed from Windows Features and 
public key
authorization does not work. Guest OS Windows Server 2022. Do you expect to use 
OpenSSH
from Windows Features or not?
As OpenSSH Server is a build feature for Server 2022 and new versions of 
Windows 10/11, I expect
that patch should work with it too.

Also from MSDN 
https://learn.microsoft.com/en-us/windows-server/administration/openssh/openssh_install_firstuse?tabs=gui
 
[learn.microsoft.com]
```
If you downloaded the OpenSSH beta from the GitHub repo at 
PowerShell/Win32-OpenSSH 
[github.com],
 follow the instructions listed there, not the ones in this article
```
So, why we should look at beta version behavior while MS provides a stable one 
from Features?


I debug the problem and the reason for ignoring SSH keys is the permissions of 
administrators_authorized_keys.
SSH server does not allow S-1-5-11 and S-1-1-0 permission.

4384 2024-04-22 01:19:57.763 debug1: trying public key file 
__PROGRAMDATA__/ssh/administrators_authorized_keys
4384 2024-04-22 01:19:57.763 debug3: Bad permissions. Try removing permissions 
for user: \\Everyone (S-1-1-0) on file 
C:/ProgramData/ssh/administrators_authorized_keys.
4384 2024-04-22 01:19:57.763 Authentication refused.

6824 2024-04-22 01:21:13.966 debug1: trying public key file 
__PROGRAMDATA__/ssh/administrators_authorized_keys
6824 2024-04-22 01:21:13.966 debug3: Bad permissions. Try removing permissions 
for user: NT AUTHORITY\\Authenticated Users (S-1-5-11) on file 
C:/ProgramData/ssh/administrators_authorized_keys.
6824 2024-04-22 01:21:13.966 Authentication refused.

I attached 2 screenshots of permissions. The first one with permission that 
file has after
guest-ssh-add-authorized-keys command and the second one with proper permissions
to make the SSH server happy.



[cid:image001.png@01DA94AF.6FB8CA60]
[cid:image002.png@01DA94AF.6FB8CA60]

Best Regards,
Konstantin Kostiuk.


On Fri, Mar 29, 2024 at 5:32 PM 
mailto:aidan_le...@selinc.com>> wrote:
From: aidaleuc mailto:aidan_le...@selinc.com>>

This patch aims to implement guest-ssh-add-authorized-keys, 
guest-ssh-remove-authorized-keys, and guest-ssh-get-authorized-keys
for Windows. This PR is based on Microsoft's OpenSSH implementation 
https://github.com/PowerShell/Win32-OpenSSH 
[github.com].
 The guest agents
will support Kubevirt and allow guest agent propagation to be used to 
dynamically inject SSH keys.
https://kubevirt.io/user-guide/virtual_machines/accessing_virtual_machines/#dynamic-ssh-public-key-injection-via-qemu-guest-agent
 
[kubevirt.io]

Changes since v5
* Fixed spurious formatting

Changes since v4
* Moved qapi/error.h to commands-common-ssh.c
* Changed  to "qapi/qapi-builtin-types.h"
* Removed stbool.h from commands-common-ssh.h

Changes since v3
* Renamed commands-ssh-core.c/h to commands-common-ssh.c/h
* Fixed styling errors discovered by checkpatch.pl 
[checkpatch.pl]
* Moved some header includes to the commands-common-ssh.h

Changes

Re: [PATCH] target/riscv/kvm: Fix exposure of Zkr





On 4/22/24 10:46, Andrew Jones wrote:

The Zkr extension may only be exposed to KVM guests if the VMM
implements the SEED CSR. Use the same implementation as TCG.

Without this patch, running with a KVM which does not forward the
SEED CSR access to QEMU will result in an ILL exception being
injected into the guest (this results in Linux guests crashing on
boot). And, when running with a KVM which does forward the access,
QEMU will crash, since QEMU doesn't know what to do with the exit.

Fixes: 3108e2f1c69d ("target/riscv/kvm: update KVM exts to Linux 6.8")
Signed-off-by: Andrew Jones 
---


Reviewed-by: Daniel Henrique Barboza 


  target/riscv/cpu.h |  3 +++
  target/riscv/csr.c | 18 ++
  target/riscv/kvm/kvm-cpu.c | 25 +
  3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3b1a02b9449a..52fb8c15d08f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -821,6 +821,9 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations 
*ops);
  
  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
  
+target_ulong riscv_new_csr_seed(target_ulong new_value,

+target_ulong write_mask);
+
  uint8_t satp_mode_max_from_map(uint32_t map);
  const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
  
diff --git a/target/riscv/csr.c b/target/riscv/csr.c

index 726096444fae..829d8346ed4e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -4267,10 +4267,8 @@ static RISCVException write_upmbase(CPURISCVState *env, 
int csrno,
  #endif
  
  /* Crypto Extension */

-static RISCVException rmw_seed(CPURISCVState *env, int csrno,
-   target_ulong *ret_value,
-   target_ulong new_value,
-   target_ulong write_mask)
+target_ulong riscv_new_csr_seed(target_ulong new_value,
+target_ulong write_mask)
  {
  uint16_t random_v;
  Error *random_e = NULL;
@@ -4294,6 +4292,18 @@ static RISCVException rmw_seed(CPURISCVState *env, int 
csrno,
  rval = random_v | SEED_OPST_ES16;
  }
  
+return rval;

+}
+
+static RISCVException rmw_seed(CPURISCVState *env, int csrno,
+   target_ulong *ret_value,
+   target_ulong new_value,
+   target_ulong write_mask)
+{
+target_ulong rval;
+
+rval = riscv_new_csr_seed(new_value, write_mask);
+
  if (ret_value) {
  *ret_value = rval;
  }
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80f1..50bdbd24a878 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1418,6 +1418,28 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct 
kvm_run *run)
  return ret;
  }
  
+static int kvm_riscv_handle_csr(CPUState *cs, struct kvm_run *run)

+{
+target_ulong csr_num = run->riscv_csr.csr_num;
+target_ulong new_value = run->riscv_csr.new_value;
+target_ulong write_mask = run->riscv_csr.write_mask;
+int ret = 0;
+
+switch (csr_num) {
+case CSR_SEED:
+run->riscv_csr.ret_value = riscv_new_csr_seed(new_value, write_mask);
+break;
+default:
+qemu_log_mask(LOG_UNIMP,
+  "%s: un-handled CSR EXIT for CSR %lx\n",
+  __func__, csr_num);
+ret = -1;
+break;
+}
+
+return ret;
+}
+
  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
  {
  int ret = 0;
@@ -1425,6 +1447,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
*run)
  case KVM_EXIT_RISCV_SBI:
  ret = kvm_riscv_handle_sbi(cs, run);
  break;
+case KVM_EXIT_RISCV_CSR:
+ret = kvm_riscv_handle_csr(cs, run);
+break;
  default:
  qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
__func__, run->exit_reason);

Re: [PATCH] target/riscv: change RISCV_EXCP_SEMIHOST exception number to 63


Palmer, Anup,

On 4/22/24 10:58, Clément Léger wrote:

The current semihost exception number (16) is a reserved number (range
[16-17]). The upcoming double trap specification uses that number for
the double trap exception. Since the privileged spec (Table 22) defines
ranges for custom uses change the semihosting exception number to 63
which belongs to the range [48-63] in order to avoid any future
collisions with reserved exception.



I didn't find any reference to a number for the SEMIHOST exception here:


https://github.com/riscv-non-isa/riscv-semihosting


Do we have any potential candidates? I would like to avoid, if possible, setting
RISCV_EXCP_SEMIHOST to 63 as a band-aid just to replace it later on by the real
value.


Thanks,

Daniel



Signed-off-by: Clément Léger 

---
  target/riscv/cpu_bits.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index fc2068ee4d..74318a925c 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -670,11 +670,11 @@ typedef enum RISCVException {
  RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
  RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
  RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
-RISCV_EXCP_SEMIHOST = 0x10,
  RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
  RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT = 0x15,
  RISCV_EXCP_VIRT_INSTRUCTION_FAULT = 0x16,
  RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT = 0x17,
+RISCV_EXCP_SEMIHOST = 0x3f,
  } RISCVException;
  
  #define RISCV_EXCP_INT_FLAG0x8000

[PATCH v2 1/1] target/riscv/kvm: tolerate KVM disable ext errors

Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
enabled, will fail with a kernel oops SIGILL right at the start. The
reason is that we can't expose zkr without implementing the SEED CSR.
Disabling zkr in the guest would be a workaround, but if the KVM doesn't
allow it we'll error out and never boot.

In hindsight this is too strict. If we keep proceeding, despite not
disabling the extension in the KVM vcpu, we'll not add the extension in
the riscv,isa. The guest kernel will be unaware of the extension, i.e.
it doesn't matter if the KVM vcpu has it enabled underneath or not. So
it's ok to keep booting in this case.

Change our current logic to not error out if we fail to disable an
extension in kvm_set_one_reg(), but show a warning and keep booting. It
is important to throw a warning because we must make the user aware that
the extension is still available in the vcpu, meaning that an
ill-behaved guest can ignore the riscv,isa settings and  use the
extension.

The case we're handling happens with an EINVAL error code. If we fail to
disable the extension in KVM for any other reason, error out.

We'll also keep erroring out when we fail to enable an extension in KVM,
since adding the extension in riscv,isa at this point will cause a guest
malfunction because the extension isn't enabled in the vcpu.

Suggested-by: Andrew Jones 
Signed-off-by: Daniel Henrique Barboza 
---
 target/riscv/kvm/kvm-cpu.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..03e3fee607 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU 
*cpu, CPUState *cs)
 reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
 ret = kvm_set_one_reg(cs, id, );
 if (ret != 0) {
-error_report("Unable to %s extension %s in KVM, error %d",
- reg ? "enable" : "disable",
- multi_ext_cfg->name, ret);
-exit(EXIT_FAILURE);
+if (!reg && ret == -EINVAL) {
+warn_report("KVM cannot disable extension %s",
+multi_ext_cfg->name);
+} else {
+error_report("Unable to enable extension %s in KVM, error %d",
+ multi_ext_cfg->name, ret);
+exit(EXIT_FAILURE);
+}
 }
 }
 }
-- 
2.44.0

[PATCH v2 0/1] target/riscv/kvm: tolerate KVM disable ext errors

Hi,

In this new version we changed the commit message a bit and we're now
only handling the case for EINVAL. Both were suggested by Drew in v1.

Changes from v1:
- added an extra paragraph explaining why we're throwing an warning
- changed the warning string
- warning is now being thrown only if EINVAL is returned. We'll keep
  erroring out otherwise.
- v1 link: 
https://lore.kernel.org/qemu-riscv/20240422131253.313869-1-dbarb...@ventanamicro.com/

Daniel Henrique Barboza (1):
  target/riscv/kvm: tolerate KVM disable ext errors

 target/riscv/kvm/kvm-cpu.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.44.0

Re: [PATCH] target/arm: Restrict translation disabled alignment check to VMSA


On 4/22/24 10:07, Richard Henderson wrote:

For cpus using PMSA, when the MPU is disabled, the default memory
type is Normal, Non-cachable.

Fixes: 59754f85ed3 ("target/arm: Do memory type alignment check when translation 
disabled")
Reported-by: Clément Chigot 
Signed-off-by: Richard Henderson 
---

Since v9 will likely be tagged tomorrow without this fixed,
Cc: qemu-sta...@nongnu.org

---
  target/arm/tcg/hflags.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 5da1b0fc1d..66de30b828 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -38,8 +38,16 @@ static bool aprofile_require_alignment(CPUARMState *env, int 
el, uint64_t sctlr)
  }
  
  /*

- * If translation is disabled, then the default memory type is
- * Device(-nGnRnE) instead of Normal, which requires that alignment
+ * With PMSA, when the MPU is disabled, all memory types in the
+ * default map is Normal.
+ */
+if (arm_feature(env, ARM_FEATURE_PMSA)) {
+return false;
+}
+
+/*
+ * With VMSA, if translation is disabled, then the default memory type
+ * is Device(-nGnRnE) instead of Normal, which requires that alignment
   * be enforced.  Since this affects all ram, it is most efficient
   * to handle this during translation.
   */


Oh, I meant to add: since the armv7 manual has both VMSA and PMSA sections, and the 
language about default Device type and alignment traps, is in the VMSA section.

This will at least fix our two r-profile cpus.

r~

[PATCH] target/arm: Restrict translation disabled alignment check to VMSA

For cpus using PMSA, when the MPU is disabled, the default memory
type is Normal, Non-cachable.

Fixes: 59754f85ed3 ("target/arm: Do memory type alignment check when 
translation disabled")
Reported-by: Clément Chigot 
Signed-off-by: Richard Henderson 
---

Since v9 will likely be tagged tomorrow without this fixed,
Cc: qemu-sta...@nongnu.org

---
 target/arm/tcg/hflags.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
index 5da1b0fc1d..66de30b828 100644
--- a/target/arm/tcg/hflags.c
+++ b/target/arm/tcg/hflags.c
@@ -38,8 +38,16 @@ static bool aprofile_require_alignment(CPUARMState *env, int 
el, uint64_t sctlr)
 }
 
 /*
- * If translation is disabled, then the default memory type is
- * Device(-nGnRnE) instead of Normal, which requires that alignment
+ * With PMSA, when the MPU is disabled, all memory types in the
+ * default map is Normal.
+ */
+if (arm_feature(env, ARM_FEATURE_PMSA)) {
+return false;
+}
+
+/*
+ * With VMSA, if translation is disabled, then the default memory type
+ * is Device(-nGnRnE) instead of Normal, which requires that alignment
  * be enforced.  Since this affects all ram, it is most efficient
  * to handle this during translation.
  */
-- 
2.34.1

Re: [PATCH intel_iommu 3/7] intel_iommu: make types match


On 22/4/24 17:52, CLEMENT MATHIEU--DRIF wrote:

The 'level' field in vtd_iotlb_key is an uint8_t.
We don't need to store level as an int in vtd_lookup_iotlb

Signed-off-by: Clément Mathieu--Drif 
---
  hw/i386/intel_iommu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 6f1364b3fd..ba545590b1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -333,7 +333,7 @@ static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, 
uint16_t source_id,
  {
  struct vtd_iotlb_key key;
  VTDIOTLBEntry *entry;
-int level;
+uint8_t level;


Or simply 'unsigned' up to vtd_slpt_level_shift()?

Re: [PATCH intel_iommu 5/7] intel_iommu: extract device IOTLB invalidation logic


On 22/4/24 17:52, CLEMENT MATHIEU--DRIF wrote:

This piece of code can be shared by both IOTLB invalidation and
PASID-based IOTLB invalidation

Signed-off-by: Clément Mathieu--Drif 
---
  hw/i386/intel_iommu.c | 57 +--
  1 file changed, 33 insertions(+), 24 deletions(-)




  static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
VTDInvDesc *inv_desc)
  {
  VTDAddressSpace *vtd_dev_as;
-IOMMUTLBEvent event;
  hwaddr addr;
-uint64_t sz;
  uint16_t sid;
  bool size;
  
@@ -2912,6 +2941,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,

  return false;
  }
  
+


Spurious newline ;)

Reviewed-by: Philippe Mathieu-Daudé 


  /*

Re: [PATCH 0/7] plugins: Use unwind info for special gdb registers

2024-04-22 Thread Alex Bennée

Richard Henderson  writes:

> Based-on: 20240404230611.21231-1-richard.hender...@linaro.org
> ("[PATCH v2 00/21] Rewrite plugin code generation")

I'm getting code conflicts w.r.t to the above (which is already merged?)
so it would be helpful to get a re-base.

>
> This is an attempt to fix
> https://gitlab.com/qemu-project/qemu/-/issues/2208
> ("PC is not updated for each instruction in TCG plugins")

The issue raises another question about PCREL support which makes me
wonder if we need to deprecate get_vaddr at translation time and make it
a run time only value?

>
> I have only updated target/i386 so far, but basically all targets
> need updating for the new callbacks.  Extra points to anyone who
> sees how to avoid the extra code duplication.  :-)
>
>
> r~
>
>
> Richard Henderson (7):
>   tcg: Introduce INDEX_op_plugin_pc
>   accel/tcg: Set CPUState.plugin_ra before all plugin callbacks
>   accel/tcg: Return the TranslationBlock from cpu_unwind_state_data
>   plugins: Introduce TCGCPUOps callbacks for mid-tb register reads
>   target/i386: Split out gdb-internal.h
>   target/i386: Introduce cpu_compute_eflags_ccop
>   target/i386: Implement TCGCPUOps for plugin register reads
>
>  include/exec/cpu-common.h |  9 +++--
>  include/hw/core/cpu.h |  1 +
>  include/hw/core/tcg-cpu-ops.h | 13 +++
>  include/tcg/tcg-op-common.h   |  1 +
>  include/tcg/tcg-opc.h |  1 +
>  target/i386/cpu.h |  2 +
>  target/i386/gdb-internal.h| 65 +++
>  accel/tcg/plugin-gen.c| 50 +---
>  accel/tcg/translate-all.c |  9 +++--
>  plugins/api.c | 36 +-
>  target/i386/gdbstub.c |  1 +
>  target/i386/helper.c  |  6 ++-
>  target/i386/tcg/cc_helper.c   | 10 +
>  target/i386/tcg/tcg-cpu.c | 72 +++
>  tcg/tcg-op.c  |  5 +++
>  tcg/tcg.c | 10 +
>  16 files changed, 258 insertions(+), 33 deletions(-)
>  create mode 100644 target/i386/gdb-internal.h

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/6] Add ivshmem-flat device

2024-04-22 Thread Gustavo Romero

Hi Markus,

Thanks for interesting in the ivshmem-flat device.

Bill Mills (cc:ed) is the best person to answer your question,
so please find his answer below.

On 2/28/24 3:29 AM, Markus Armbruster wrote:

Gustavo Romero writes:

[...]

This patchset introduces a new device, ivshmem-flat, which is similar to the
current ivshmem device but does not require a PCI bus. It implements the ivshmem
status and control registers as MMRs and the shared memory as a directly
accessible memory region in the VM memory layout. It's meant to be used on
machines like those with Cortex-M MCUs, which usually lack a PCI bus, e.g.,
lm3s6965evb and mps2-an385. Additionally, it has the benefit of requiring a tiny
'device driver,' which is helpful on some RTOSes, like Zephyr, that run on
memory-constrained resource targets.

The patchset includes a QTest for the ivshmem-flat device, however, it's also
possible to experiment with it in two ways:

(a) using two Cortex-M VMs running Zephyr; or
(b) using one aarch64 VM running Linux with the ivshmem PCI device and another
arm (Cortex-M) VM running Zephyr with the new ivshmem-flat device.

Please note that for running the ivshmem-flat QTests the following patch, which
is not committed to the tree yet, must be applied:

https://lists.nongnu.org/archive/html/qemu-devel/2023-11/msg03176.html

What problem are you trying to solve with ivshmem?

Shared memory is not a solution to any communication problem, it's
merely a building block for building such solutions: you invariably have
to layer some protocol on top. What do you intend to put on top of
ivshmem?

Actually ivshmem is shared memory and bi-direction notifications (in this case
a doorbell register and an irq).

This is the fundamental requirement for many types of communication but our
interest is for the OpenAMP project [1].

All the OpenAMP project's communication is based on shared memory and
bi-directional notification. Often this is on a AMP SOC with Cortex-As and
Cortex-Ms or Rs. However we are now expanding into PCIe based AMP. One example
of this is an x86 host computer and a PCIe card with an ARM SOC. Other
examples include two systems with PCIe root complex connected via a
non-transparent bridge.

The existing PCI based ivshmem lets us model these types of systems in a simple
generic way without worrying about the details of the RC/EP relationship or the
details of a specific non-transparent bridge. In fact the ivshmem looks to the
two (or more) systems like a non-transparent bridge with its own memory (and no
other memory access is allowed).

Right now we are testing this with RPMSG between two QEMU system where both
systems are cortex-a53 and both running Zephyr. [2]

We will expand this by switching one of the QEMU systems to either arm64 Linux
or x86 Linux.

We (and others) are also working on a generic virtio transport that will work
between any two systems as long as they have shared memory and bi-directional
notifications.

Now for ivshmem-flat. We want to expand this model to include MCU like CPUs
and RTOS'es that don't have PCIe. We focus on Cortex-M because every open
source RTOS has an existing port for one of the Cortex-M machines already in
QEMU. However they don't normally pick the same one. If we added our own
custom machine for this, the QEMU project would push back and even if accepted
we would have to do a port for each RTOS. This would mean we would not test as
many RTOSes.

The ivshmem-flat is actually a good model for what a Cortex-M based PCIe card
would look like. The host system would see the connection as PCIe but to the
Cortex-M it would just appear as memory, MMR's for the doorbell, and an IRQ.

So even after we have a "roll your own machine definition from a file", I
expect ivshmem and ivshmem-flat to still be very useful.

[1] https://www.openampproject.org/
[2] Work in progress here:
https://github.com/OpenAMP/openamp-system-reference/tree/main/examples/zephyr/dual_qemu_ivshmem

Cheers,
Gustavo

Re: [PATCH 00/27] Add qapi-domain Sphinx extension

2024-04-22 Thread John Snow

On Mon, Apr 22, 2024 at 5:20 AM Markus Armbruster  wrote:
>
> John Snow  writes:
>
> > On Fri, Apr 19, 2024, 10:45 AM Markus Armbruster  wrote:
> >
> >> John Snow  writes:
> >>
> >> > This series adds a new qapi-domain extension for Sphinx, which adds a
> >> > series of custom directives for documenting QAPI definitions.
> >> >
> >> > GitLab CI: https://gitlab.com/jsnow/qemu/-/pipelines/1259566476
> >> >
> >> > (Link to a demo HTML page at the end of this cover letter, but I want
> >> > you to read the cover letter first to explain what you're seeing.)
> >> >
> >> > This adds a new QAPI Index page, cross-references for QMP commands,
> >> > events, and data types, and improves the aesthetics of the QAPI/QMP
> >> > documentation.
> >>
> >> Cross-references alone will be a *massive* improvement!  I'm sure
> >> readers will appreciate better looks and an index, too.
> >>
> >> > This series adds only the new ReST syntax, *not* the autogenerator. The
> >> > ReST syntax used in this series is, in general, not intended for anyone
> >> > to actually write by hand. This mimics how Sphinx's own autodoc
> >> > extension generates Python domain directives, which are then re-parsed
> >> > to produce the final result.
> >> >
> >> > I have prototyped such a generator, but it isn't ready for inclusion
> >> > yet. (Rest assured: error context reporting is preserved down to the
> >> > line, even in generated ReST. There is no loss in usability for this
> >> > approach. It will likely either supplant qapidoc.py or heavily alter
> >> > it.) The generator requires only extremely minor changes to
> >> > scripts/qapi/parser.py to preserve nested indentation and provide more
> >> > accurate line information. It is less invasive than you may
> >> > fear. Relying on a secondary ReST parse phase eliminates much of the
> >> > complexity of qapidoc.py. Sleep soundly.
> >>
> >> I'm a Sphinx noob.  Let me paraphrase you to make sure I understand.
> >>
> >> You proprose to generate formatted documentation in two steps:
> >>
> >> • First, the QAPI generator generates .rst from the QAPI schema.  The
> >>   generated .rst makes use of a custom directives.
> >>
> >
> > Yes, but this .rst file is built in-memory and never makes it to disk, like
> > Sphinx's autodoc for Python.
>
> Makes sense.
>
> > (We can add a debug knob to log it or save it out to disk if needed.)
>
> Likely useful at least occasionally.

Yep, python's autodoc has such a knob to use the debugging log for
this. I just want to point out that avoiding the intermediate file
on-disk is actually the mechanism by which I can preserve source
lines, so this is how it's gotta be.

I build an intermediate doc in-memory with source filenames and source
lines along with the modified doc block content so that ReST errors
can be tracked back directly to the QAPI json files. If we saved to
disk and parsed that, it'd obliterate that information.

>
> >> • Second, Sphinx turns the .rst into formatted documentation.  A Sphinx
> >>   qapi-domain extension implements the custom directives
> >
> > Yes.
> >
> >
> >> This mirrors how Sphinx works for Python docs.  Which is its original
> >> use case.
> >>
> >> Your series demonstrates the second step, with test input you wrote
> >> manually.
> >>
> >> You have code for the first step, but you'd prefer to show it later.
> >
> > Right, it's not fully finished, although I have events, commands, and
> > objects working. Unions, Alternates and Events need work.
> >
> >
> >> Fair?
> >
> > Bingo!
>
> Thanks!
>
> >> > The purpose of sending this series in its current form is largely to
> >> > solicit feedback on general aesthetics, layout, and features. Sphinx is
> >> > a wily beast, and feedback at this stage will dictate how and where
> >> > certain features are implemented.
> >>
> >> I'd appreciate help with that.  Opinions?
> >
> >
> >> > A goal for this syntax (and the generator) is to fully in-line all
> >> > command/event/object members, inherited or local, boxed or not, branched
> >> > or not. This should provide a boon to using these docs as a reference,
> >> > because users will not have to grep around the page looking for various
> >> > types, branches, or inherited members. Any arguments types will be
> >> > hyperlinked to their definition, further aiding usability. Commands can
> >> > be hotlinked from anywhere else in the manual, and will provide a
> >> > complete reference directly on the first screenful of information.
> >>
> >> Let me elaborate a bit here.
> >>
> >> A command's arguments can be specified inline, like so:
> >>
> >> { 'command': 'job-cancel', 'data': { 'id': 'str' } }
> >>
> >> The arguments are then documented right with the command.
> >>
> >> But they can also be specified by referencing an object type, like so:
> >>
> >> { 'command': 'block-dirty-bitmap-remove',
> >>   'data': 'BlockDirtyBitmap' }
> >>
> >> Reasons for doing it this way:
> >>
> >> • Several commands take the same arguments, and you don't

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

On Mon, 22 Apr 2024 at 16:48, Richard Henderson
 wrote:
>
> On 4/22/24 08:26, Clément Chigot wrote:
> > Hi Richard,
> >
> > While testing the future V9, I've some regressions on a custom board
> > using cortex-R5 CPUs.
> > Unaligned data accesses are no longer allowed because of that patch.
> >
> > I've dug into the various documentation and it seems that R-profile
> > CPUs don't have the same default memory type as A-profile. It depends
> > on a default memory map provided in the R-Profile RM in C1.3 [1], even
> > when PMU is disabled.
> >
> >> Each PMSAv8-32 MPU has an associated default memory map which is used when 
> >> the MPU is not enabled.
> >> ...
> >> Table C1-4 and Table C1-5 describe the default memory map defined for the 
> >> EL1 MPU.
> >
> > For our case, Table C1-5 can be simplified as:
> >   |  0x – 0x7FFF Normal
> >   |  0x8000 – 0xBFFF Device-nGnRE
> >   |  0xC000 – 0x Device-nGnRnE
> >
> > Therefore, we can't blindly enable strict alignment checking solely on
> > SCTLR bits. We should make it depend on the address targeted. But is
> > it possible to know that address in `aprofile_require_alignment` ?
> > with `mmu_idx` ?
>
> No, this would need to be handled in get_phys_addr_disabled.
>
> > By the way, are R-Profile CPUs the same as those having the `PMSA`
> > feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
> > with that, instead of create a new `ARM_FEATURE_R`
>
> No, some armv5 have PMSA.
>
> >
> > Note that the RM I've linked is for ARMv8. But this other link [2]
> > seems to show a similar behavior for arm-v7.
> >
> > cc Jonathan and Ard, though not sure this is the same bug you've
> > reported earlier.
> >
> > Thanks,
> > Clément
> > [1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
> > [2] 
> > https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ
>
> Ouch, thanks for the armv7 link.  At the moment it looks like my blanket 
> mmu-disabled
> change should be restricted to armv8.

Restricted to A-profile, probably. I haven't cross-checked, but IIRC
for v7A this is IMPDEF and we're OK to fault it.

thanks
-- PMM

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

On Mon, 22 Apr 2024 at 15:18, Peter Maydell  wrote:
> I imagine that value gets written into CNTFRQ by TF-A somewhere
> along the line (and then read by EDK2 later), though I haven't
> quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
> assumes that the generic-timer frequency and the watchdog
> timer frequency are the same, which is a bit dubious IMHO.

Checking the BSA spec, this is actually correct -- the system
watchdog is supposed to run at the generic counter frequency,
which will be the same as the one the CPU generic timers use.
So we need on the QEMU side to make the sbsa-watchdog device
be runtime configurable for frequency and arrange for it to
be set the same as the CPU.

(We could also arrange this by modelling the memory mapped
system counter properly; I have some slightly half-baked
patches to do that floating around somewhere. But I'm still
not quite sure it's worth the effort needed to try to get
them into a fully baked state :-))

thanks
-- PMM

[PATCH intel_iommu 4/7] intel_iommu: add support for first-stage translation

This translation mode will only be made available in scalable mode

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c  | 364 -
 hw/i386/intel_iommu_internal.h |  51 -
 2 files changed, 362 insertions(+), 53 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index ba545590b1..3b9f120dec 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -713,6 +713,21 @@ static uint64_t vtd_get_pte(dma_addr_t base_addr, uint32_t 
index)
 return pte;
 }
 
+static MemTxResult vtd_set_flag_in_pte(dma_addr_t base_addr, uint32_t index,
+   uint64_t pte, uint64_t flag)
+{
+assert(index < VTD_PT_ENTRY_NR);
+if (pte & flag) {
+return MEMTX_OK;
+}
+pte |= flag;
+pte = cpu_to_le64(pte);
+return dma_memory_write(_space_memory,
+base_addr + index * sizeof(pte),
+, sizeof(pte),
+MEMTXATTRS_UNSPECIFIED);
+}
+
 /* Given an iova and the level of paging structure, return the offset
  * of current level.
  */
@@ -730,11 +745,17 @@ static inline bool vtd_is_level_supported(IntelIOMMUState 
*s, uint32_t level)
 }
 
 /* Return true if check passed, otherwise false */
-static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
+static inline bool vtd_pe_type_check(IntelIOMMUState *s,
  VTDPASIDEntry *pe)
 {
+X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
+
 switch (VTD_PE_GET_TYPE(pe)) {
 case VTD_SM_PASID_ENTRY_FLT:
+if (!(s->ecap & VTD_ECAP_FLTS)) {
+return false;
+}
+break;
 case VTD_SM_PASID_ENTRY_SLT:
 case VTD_SM_PASID_ENTRY_NESTED:
 break;
@@ -784,6 +805,11 @@ static inline bool vtd_pe_present(VTDPASIDEntry *pe)
 return pe->val[0] & VTD_PASID_ENTRY_P;
 }
 
+static inline bool vtd_fl_pte_present(uint64_t pte)
+{
+return pte & VTD_FL_PTE_P;
+}
+
 static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
   uint32_t pasid,
   dma_addr_t addr,
@@ -791,7 +817,6 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState 
*s,
 {
 uint32_t index;
 dma_addr_t entry_size;
-X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
 
 index = VTD_PASID_TABLE_INDEX(pasid);
 entry_size = VTD_PASID_ENTRY_SIZE;
@@ -805,7 +830,7 @@ static int vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState 
*s,
 }
 
 /* Do translation type check */
-if (!vtd_pe_type_check(x86_iommu, pe)) {
+if (!vtd_pe_type_check(s, pe)) {
 return -VTD_FR_PASID_TABLE_INV;
 }
 
@@ -1027,6 +1052,34 @@ static inline bool 
vtd_iova_sl_range_check(IntelIOMMUState *s,
 return !(iova & ~(vtd_iova_limit(s, ce, aw, pasid) - 1));
 }
 
+/* Return true if IOVA is canonical, otherwise false. */
+static bool vtd_iova_fl_check_canonical(IntelIOMMUState *s,
+uint64_t iova, VTDContextEntry *ce,
+uint8_t aw, uint32_t pasid)
+{
+uint64_t iova_limit = vtd_iova_limit(s, ce, aw, pasid);
+uint64_t upper_bits_mask = ~(iova_limit - 1);
+uint64_t upper_bits = iova & upper_bits_mask;
+bool msb = ((iova & (iova_limit >> 1)) != 0);
+return !(
+ (!msb && (upper_bits != 0)) ||
+ (msb && (upper_bits != upper_bits_mask))
+);
+}
+
+/* Return the page table base address corresponding to the translation type. */
+static dma_addr_t vtd_pe_get_pgtbl_base(VTDPASIDEntry *pe)
+{
+uint16_t pgtt = VTD_PE_GET_TYPE(pe);
+if (pgtt == VTD_SM_PASID_ENTRY_FLT) {
+return pe->val[2] & VTD_SM_PASID_ENTRY_PTPTR;
+} else if (pgtt == VTD_SM_PASID_ENTRY_SLT) {
+return pe->val[0] & VTD_SM_PASID_ENTRY_PTPTR;
+}
+
+return 0; /* Not supported */
+}
+
 static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
   VTDContextEntry *ce,
   uint32_t pasid)
@@ -1035,7 +1088,7 @@ static dma_addr_t vtd_get_iova_pgtbl_base(IntelIOMMUState 
*s,
 
 if (s->root_scalable) {
 vtd_ce_get_rid2pasid_entry(s, ce, , pasid);
-return pe.val[0] & VTD_SM_PASID_ENTRY_SLPTPTR;
+return vtd_pe_get_pgtbl_base();
 }
 
 return vtd_ce_get_slpt_base(ce);
@@ -1053,6 +1106,10 @@ static dma_addr_t 
vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
 static uint64_t vtd_spte_rsvd[VTD_SPTE_RSVD_LEN];
 static uint64_t vtd_spte_rsvd_large[VTD_SPTE_RSVD_LEN];
 
+#define VTD_FPTE_RSVD_LEN 5
+static uint64_t vtd_fpte_rsvd[VTD_FPTE_RSVD_LEN];
+static uint64_t vtd_fpte_rsvd_large[VTD_FPTE_RSVD_LEN];
+
 static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
 {
 uint64_t rsvd_mask;
@@ -1079,21 +1136,140 @@ static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, 
uint32_t level)
 return slpte & rsvd_mask;
 }
 
-/*

[PATCH intel_iommu 7/7] intel_iommu: add a CLI option to enable FLTS

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c | 6 ++
 include/hw/i386/intel_iommu.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4b54a45107..c35ccc3a98 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3704,6 +3704,7 @@ static Property vtd_properties[] = {
 DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
 DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
 DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
+DEFINE_PROP_BOOL("flts", IntelIOMMUState, flts, false),
 DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
 DEFINE_PROP_BOOL("dma-translation", IntelIOMMUState, dma_translation, 
true),
 DEFINE_PROP_END_OF_LIST(),
@@ -4413,6 +4414,11 @@ static void vtd_init(IntelIOMMUState *s)
 s->ecap |= VTD_ECAP_PASID;
 }
 
+if (s->flts) {
+s->ecap |= VTD_ECAP_FLTS;
+s->cap |= VTD_CAP_FS1GP;
+}
+
 vtd_reset_caches(s);
 
 /* Define registers with default values and bit semantics */
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b9a01556ec..6ecc8bb8a9 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -263,6 +263,7 @@ struct IntelIOMMUState {
 bool caching_mode;  /* RO - is cap CM enabled? */
 bool scalable_mode; /* RO - is Scalable Mode supported? */
 bool snoop_control; /* RO - is SNP filed supported? */
+bool flts;  /* RO - is FS translation supported? */
 
 dma_addr_t root;/* Current root table pointer */
 bool root_scalable; /* Type of root table (scalable or not) */
-- 
2.44.0

[PATCH intel_iommu 2/7] intel_iommu: rename slpte to pte before adding FLTS

Some variables struct fields and functions can be used for both
slpte and flpte. We can modify certain identifiers to make them
more generic.

- slpte in IOMMUTLBEntry becomes pte and will be used for both FL and SL
- VTD_SL_PT_LEVEL, VTD_SL_PT_PAGE_SIZE_MASK and VTD_SL_LEVEL_BITS can be
  renamed and considered as a common constants
- vtd_iova_range_check becomes vtd_iova_sl_range_check because the range
  check depends on the translation type
- vtd_do_iommu_translate now handles both FL and SL so we can rename
  slpte to pte
- VTD_SL_PT_BASE_ADDR_MASK becomes VTD_PT_BASE_ADDR_MASK because the
  address offset within a 64bits word of a Scalable-Mode PASID Table
  Entry is the same for FL and SL. As a consequence, vtd_get_slpte_addr
  is also renamed to vtd_get_pte_addr.
- vtd_is_last_slpte becomes vtd_is_last_slpte because the same bit is
  used for FL and SL.
- vtd_slpt_level_page_mask becomes vtd_pt_level_page_mask
- vtd_get_slpte becomes vtd_get_pte

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c  | 106 -
 hw/i386/intel_iommu_internal.h |  10 ++--
 include/hw/i386/intel_iommu.h  |   2 +-
 3 files changed, 60 insertions(+), 58 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index cc8e59674e..6f1364b3fd 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -259,15 +259,15 @@ static gboolean vtd_hash_remove_by_domain(gpointer key, 
gpointer value,
 }
 
 /* The shift of an addr for a certain level of paging structure */
-static inline uint32_t vtd_slpt_level_shift(uint32_t level)
+static inline uint32_t vtd_pt_level_shift(uint32_t level)
 {
 assert(level != 0);
-return VTD_PAGE_SHIFT_4K + (level - 1) * VTD_SL_LEVEL_BITS;
+return VTD_PAGE_SHIFT_4K + (level - 1) * VTD_LEVEL_BITS;
 }
 
-static inline uint64_t vtd_slpt_level_page_mask(uint32_t level)
+static inline uint64_t vtd_pt_level_page_mask(uint32_t level)
 {
-return ~((1ULL << vtd_slpt_level_shift(level)) - 1);
+return ~((1ULL << vtd_pt_level_shift(level)) - 1);
 }
 
 static gboolean vtd_hash_remove_by_page(gpointer key, gpointer value,
@@ -324,7 +324,7 @@ static void vtd_reset_caches(IntelIOMMUState *s)
 
 static uint64_t vtd_get_iotlb_gfn(hwaddr addr, uint32_t level)
 {
-return (addr & vtd_slpt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;
+return (addr & vtd_pt_level_page_mask(level)) >> VTD_PAGE_SHIFT_4K;
 }
 
 /* Must be called with IOMMU lock held */
@@ -352,7 +352,7 @@ out:
 
 /* Must be with IOMMU lock held */
 static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
- uint16_t domain_id, hwaddr addr, uint64_t slpte,
+ uint16_t domain_id, hwaddr addr, uint64_t pte,
  uint8_t access_flags, uint32_t level,
  uint32_t pasid)
 {
@@ -360,7 +360,7 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t 
source_id,
 struct vtd_iotlb_key *key = g_malloc(sizeof(*key));
 uint64_t gfn = vtd_get_iotlb_gfn(addr, level);
 
-trace_vtd_iotlb_page_update(source_id, addr, slpte, domain_id);
+trace_vtd_iotlb_page_update(source_id, addr, pte, domain_id);
 if (g_hash_table_size(s->iotlb) >= VTD_IOTLB_MAX_SIZE) {
 trace_vtd_iotlb_reset("iotlb exceeds size limit");
 vtd_reset_iotlb_locked(s);
@@ -368,9 +368,9 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t 
source_id,
 
 entry->gfn = gfn;
 entry->domain_id = domain_id;
-entry->slpte = slpte;
+entry->pte = pte;
 entry->access_flags = access_flags;
-entry->mask = vtd_slpt_level_page_mask(level);
+entry->mask = vtd_pt_level_page_mask(level);
 entry->pasid = pasid;
 
 key->gfn = gfn;
@@ -685,32 +685,32 @@ static inline dma_addr_t 
vtd_ce_get_slpt_base(VTDContextEntry *ce)
 return ce->lo & VTD_CONTEXT_ENTRY_SLPTPTR;
 }
 
-static inline uint64_t vtd_get_slpte_addr(uint64_t slpte, uint8_t aw)
+static inline uint64_t vtd_get_pte_addr(uint64_t pte, uint8_t aw)
 {
-return slpte & VTD_SL_PT_BASE_ADDR_MASK(aw);
+return pte & VTD_PT_BASE_ADDR_MASK(aw);
 }
 
 /* Whether the pte indicates the address of the page frame */
-static inline bool vtd_is_last_slpte(uint64_t slpte, uint32_t level)
+static inline bool vtd_is_last_pte(uint64_t pte, uint32_t level)
 {
-return level == VTD_SL_PT_LEVEL || (slpte & VTD_SL_PT_PAGE_SIZE_MASK);
+return level == VTD_COMMON_PT_LEVEL || (pte & VTD_PT_PAGE_SIZE_MASK);
 }
 
-/* Get the content of a spte located in @base_addr[@index] */
-static uint64_t vtd_get_slpte(dma_addr_t base_addr, uint32_t index)
+/* Get the content of a pte located in @base_addr[@index] */
+static uint64_t vtd_get_pte(dma_addr_t base_addr, uint32_t index)
 {
-uint64_t slpte;
+uint64_t pte;
 
-assert(index < VTD_SL_PT_ENTRY_NR);
+assert(index < VTD_PT_ENTRY_NR);
 
 if (dma_memory_read(_space_memory,
-base_addr + index * sizeof(slpte),
-

[PATCH intel_iommu 0/7] FLTS for VT-d

This series is the first of a list that add support for SVM in the Intel IOMMU.

Here, we implement support for first-stage translation in VT-d.
The PASID-based IOTLB invalidation is also added in this series as it is a
requirement of FLTS.

The last patch introduces the 'flts' option to enable the feature from
the command line.
Once enabled, several drivers of the Linux kernel use this feature.

This work is based on the VT-d specification version 4.1 (March 2023)

Here is a link to a GitHub repository where you can find the following elements 
:
- Qemu with all the patches for SVM
- ATS
- PRI
- PASID based IOTLB invalidation
- Device IOTLB invalidations
- First-stage translations
- Requests with already translated addresses
- A demo device
- A simple driver for the demo device
- A userspace program (for testing and demonstration purposes)

https://github.com/BullSequana/Qemu-in-guest-SVM-demo

Clément Mathieu--Drif (7):
  intel_iommu: fix FRCD construction macro.
  intel_iommu: rename slpte to pte before adding FLTS
  intel_iommu: make types match
  intel_iommu: add support for first-stage translation
  intel_iommu: extract device IOTLB invalidation logic
  intel_iommu: add PASID-based IOTLB invalidation
  intel_iommu: add a CLI option to enable FLTS

 hw/i386/intel_iommu.c  | 655 ++---
 hw/i386/intel_iommu_internal.h | 114 --
 include/hw/i386/intel_iommu.h  |   3 +-
 3 files changed, 609 insertions(+), 163 deletions(-)

-- 
2.44.0

[PATCH intel_iommu 6/7] intel_iommu: add PASID-based IOTLB invalidation

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c  | 130 ++---
 hw/i386/intel_iommu_internal.h |  51 +++--
 2 files changed, 150 insertions(+), 31 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index aaac61bf6a..4b54a45107 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -277,9 +277,22 @@ static gboolean vtd_hash_remove_by_page(gpointer key, 
gpointer value,
 VTDIOTLBPageInvInfo *info = (VTDIOTLBPageInvInfo *)user_data;
 uint64_t gfn = (info->addr >> VTD_PAGE_SHIFT_4K) & info->mask;
 uint64_t gfn_tlb = (info->addr & entry->mask) >> VTD_PAGE_SHIFT_4K;
-return (entry->domain_id == info->domain_id) &&
-(((entry->gfn & info->mask) == gfn) ||
- (entry->gfn == gfn_tlb));
+return (
+(entry->domain_id == info->domain_id) &&
+(info->pasid == entry->pasid)
+) && (
+((entry->gfn & info->mask) == gfn) ||
+(entry->gfn == gfn_tlb)
+);
+}
+
+static gboolean vtd_hash_remove_by_pasid(gpointer key, gpointer value,
+gpointer user_data)
+{
+VTDIOTLBEntry *entry = (VTDIOTLBEntry *)value;
+VTDIOTLBPasidEntryInvInfo *info = (VTDIOTLBPasidEntryInvInfo *)user_data;
+return ((entry->domain_id == info->domain_id) &&
+(info->pasid == entry->pasid));
 }
 
 /* Reset all the gen of VTDAddressSpace to zero and set the gen of
@@ -1287,8 +1300,10 @@ static int vtd_iova_to_pte_sl(IntelIOMMUState *s,  
VTDContextEntry *ce,
 if (ret != 0) {
 return ret;
 }
+
 *reads = (*reads) && (slpte & VTD_SL_R);
 *writes = (*writes) && (slpte & VTD_SL_W);
+
 if ((slpte & access_right_check) != access_right_check) {
 error_report_once("%s: detected slpte permission error "
   "(iova=0x%" PRIx64 ", level=0x%" PRIx32 ", "
@@ -2484,23 +2499,61 @@ static void 
vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s,
 }
 }
 
+static VTDIOTLBPageInvInfo vtd_build_tlb_page_inv_info(uint16_t domain_id,
+   hwaddr addr, uint8_t am,
+   uint32_t pasid)
+{
+assert(am <= VTD_MAMV);
+VTDIOTLBPageInvInfo info = {
+.domain_id = domain_id,
+.addr = addr,
+.mask = ~((1ULL << am) - 1),
+.pasid = pasid
+};
+return info;
+}
+
 static void vtd_iotlb_page_invalidate(IntelIOMMUState *s, uint16_t domain_id,
   hwaddr addr, uint8_t am)
 {
-VTDIOTLBPageInvInfo info;
+VTDIOTLBPageInvInfo info = vtd_build_tlb_page_inv_info(domain_id, addr,
+   am, PCI_NO_PASID);
 
 trace_vtd_inv_desc_iotlb_pages(domain_id, addr, am);
 
-assert(am <= VTD_MAMV);
-info.domain_id = domain_id;
-info.addr = addr;
-info.mask = ~((1 << am) - 1);
 vtd_iommu_lock(s);
 g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, );
 vtd_iommu_unlock(s);
+
 vtd_iotlb_page_invalidate_notify(s, domain_id, addr, am, PCI_NO_PASID);
 }
 
+static void vtd_pasid_based_iotlb_page_invalidate(IntelIOMMUState *s,
+  uint16_t domain_id,
+  hwaddr addr,
+  uint8_t am, uint32_t pasid)
+{
+VTDIOTLBPageInvInfo info = vtd_build_tlb_page_inv_info(domain_id, addr,
+   am, pasid);
+vtd_iommu_lock(s);
+g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_page, );
+vtd_iommu_unlock(s);
+}
+
+static void vtd_pasid_based_iotlb_invalidate(IntelIOMMUState *s,
+ uint16_t domain_id,
+ uint32_t pasid)
+{
+assert(pasid != PCI_NO_PASID);
+VTDIOTLBPasidEntryInvInfo info = {
+.domain_id = domain_id,
+.pasid = pasid
+};
+vtd_iommu_lock(s);
+g_hash_table_foreach_remove(s->iotlb, vtd_hash_remove_by_pasid, );
+vtd_iommu_unlock(s);
+}
+
 /* Flush IOTLB
  * Returns the IOTLB Actual Invalidation Granularity.
  * @val: the content of the IOTLB_REG
@@ -2759,7 +2812,7 @@ static bool vtd_get_inv_desc(IntelIOMMUState *s,
 static bool vtd_process_wait_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
 {
 if ((inv_desc->hi & VTD_INV_DESC_WAIT_RSVD_HI) ||
-(inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO)) {
+(inv_desc->lo & VTD_INV_DESC_WAIT_RSVD_LO(s->ecap))) {
 error_report_once("%s: invalid wait desc: hi=%"PRIx64", lo=%"PRIx64
   " (reserved nonzero)", __func__, inv_desc->hi,
   inv_desc->lo);
@@ -2785,6 +2838,11 @@ static bool vtd_process_wait_desc(IntelIOMMUState *s, 
VTDInvDesc *inv_desc)
 } else

[PATCH intel_iommu 5/7] intel_iommu: extract device IOTLB invalidation logic

This piece of code can be shared by both IOTLB invalidation and
PASID-based IOTLB invalidation

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c | 57 +--
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 3b9f120dec..aaac61bf6a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2890,13 +2890,42 @@ static bool vtd_process_inv_iec_desc(IntelIOMMUState *s,
 return true;
 }
 
+static void do_invalidate_device_tlb(VTDAddressSpace *vtd_dev_as,
+ bool size, hwaddr addr)
+{
+/*
+ * According to ATS spec table 2.4:
+ * S = 0, bits 15:12 =  range size: 4K
+ * S = 1, bits 15:12 = xxx0 range size: 8K
+ * S = 1, bits 15:12 = xx01 range size: 16K
+ * S = 1, bits 15:12 = x011 range size: 32K
+ * S = 1, bits 15:12 = 0111 range size: 64K
+ * ...
+ */
+
+IOMMUTLBEvent event;
+uint64_t sz;
+
+if (size) {
+sz = (VTD_PAGE_SIZE * 2) << cto64(addr >> VTD_PAGE_SHIFT);
+addr &= ~(sz - 1);
+} else {
+sz = VTD_PAGE_SIZE;
+}
+
+event.type = IOMMU_NOTIFIER_DEVIOTLB_UNMAP;
+event.entry.target_as = _dev_as->as;
+event.entry.addr_mask = sz - 1;
+event.entry.iova = addr;
+event.entry.perm = IOMMU_NONE;
+event.entry.translated_addr = 0;
+memory_region_notify_iommu(_dev_as->iommu, 0, event);
+}
 static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s,
   VTDInvDesc *inv_desc)
 {
 VTDAddressSpace *vtd_dev_as;
-IOMMUTLBEvent event;
 hwaddr addr;
-uint64_t sz;
 uint16_t sid;
 bool size;
 
@@ -2912,6 +2941,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState 
*s,
 return false;
 }
 
+
 /*
  * Using sid is OK since the guest should have finished the
  * initialization of both the bus and device.
@@ -2921,28 +2951,7 @@ static bool 
vtd_process_device_iotlb_desc(IntelIOMMUState *s,
 goto done;
 }
 
-/* According to ATS spec table 2.4:
- * S = 0, bits 15:12 =  range size: 4K
- * S = 1, bits 15:12 = xxx0 range size: 8K
- * S = 1, bits 15:12 = xx01 range size: 16K
- * S = 1, bits 15:12 = x011 range size: 32K
- * S = 1, bits 15:12 = 0111 range size: 64K
- * ...
- */
-if (size) {
-sz = (VTD_PAGE_SIZE * 2) << cto64(addr >> VTD_PAGE_SHIFT);
-addr &= ~(sz - 1);
-} else {
-sz = VTD_PAGE_SIZE;
-}
-
-event.type = IOMMU_NOTIFIER_DEVIOTLB_UNMAP;
-event.entry.target_as = _dev_as->as;
-event.entry.addr_mask = sz - 1;
-event.entry.iova = addr;
-event.entry.perm = IOMMU_NONE;
-event.entry.translated_addr = 0;
-memory_region_notify_iommu(_dev_as->iommu, 0, event);
+do_invalidate_device_tlb(vtd_dev_as, size, addr);
 
 done:
 return true;
-- 
2.44.0

[PATCH intel_iommu 3/7] intel_iommu: make types match

The 'level' field in vtd_iotlb_key is an uint8_t.
We don't need to store level as an int in vtd_lookup_iotlb

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 6f1364b3fd..ba545590b1 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -333,7 +333,7 @@ static VTDIOTLBEntry *vtd_lookup_iotlb(IntelIOMMUState *s, 
uint16_t source_id,
 {
 struct vtd_iotlb_key key;
 VTDIOTLBEntry *entry;
-int level;
+uint8_t level;
 
 for (level = VTD_SL_PT_LEVEL; level < VTD_SL_PML4_LEVEL; level++) {
 key.gfn = vtd_get_iotlb_gfn(addr, level);
-- 
2.44.0

[PATCH intel_iommu 1/7] intel_iommu: fix FRCD construction macro.

The constant must be unsigned, otherwise the two's complement
overrides the other fields when a PASID is present

Signed-off-by: Clément Mathieu--Drif 
---
 hw/i386/intel_iommu_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index f8cf99bddf..cbc4030031 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -267,7 +267,7 @@
 /* For the low 64-bit of 128-bit */
 #define VTD_FRCD_FI(val)((val) & ~0xfffULL)
 #define VTD_FRCD_PV(val)(((val) & 0xULL) << 40)
-#define VTD_FRCD_PP(val)(((val) & 0x1) << 31)
+#define VTD_FRCD_PP(val)(((val) & 0x1ULL) << 31)
 #define VTD_FRCD_IR_IDX(val)(((val) & 0xULL) << 48)
 
 /* DMA Remapping Fault Conditions */
-- 
2.44.0

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

On 4/22/24 08:26, Clément Chigot wrote:

Hi Richard,

While testing the future V9, I've some regressions on a custom board
using cortex-R5 CPUs.
Unaligned data accesses are no longer allowed because of that patch.

I've dug into the various documentation and it seems that R-profile
CPUs don't have the same default memory type as A-profile. It depends
on a default memory map provided in the R-Profile RM in C1.3 [1], even
when PMU is disabled.

Each PMSAv8-32 MPU has an associated default memory map which is used when the
MPU is not enabled.
...
Table C1-4 and Table C1-5 describe the default memory map defined for the EL1
MPU.

For our case, Table C1-5 can be simplified as:
| 0x – 0x7FFF Normal
| 0x8000 – 0xBFFF Device-nGnRE
| 0xC000 – 0x Device-nGnRnE

Therefore, we can't blindly enable strict alignment checking solely on
SCTLR bits. We should make it depend on the address targeted. But is
it possible to know that address in `aprofile_require_alignment` ?
with `mmu_idx` ?

No, this would need to be handled in get_phys_addr_disabled.

By the way, are R-Profile CPUs the same as those having the `PMSA`
feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
with that, instead of create a new `ARM_FEATURE_R`

No, some armv5 have PMSA.

Note that the RM I've linked is for ARMv8. But this other link [2]
seems to show a similar behavior for arm-v7.

cc Jonathan and Ard, though not sure this is the same bug you've
reported earlier.

Thanks,
Clément
[1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
[2]
https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ

Ouch, thanks for the armv7 link. At the moment it looks like my blanket mmu-disabled
change should be restricted to armv8.

Re: [PATCH v7 00/12] Enabling DCD emulation support in Qemu

2024-04-22 Thread Gregory Price

On Mon, Apr 22, 2024 at 01:04:48PM +0100, Jonathan Cameron wrote:
> On Sat, 20 Apr 2024 16:35:46 -0400
> Gregory Price  wrote:
> 
> > On Fri, Apr 19, 2024 at 11:43:14AM -0700, fan wrote:
> > > On Fri, Apr 19, 2024 at 02:24:36PM -0400, Gregory Price wrote:  
> > > > 
> > > > added review to all patches, will hopefully be able to add a Tested-by
> > > > tag early next week, along with a v1 RFC for MHD bit-tracking.
> > > > 
> > > > We've been testing v5/v6 for a bit, so I expect as soon as we get the
> > > > MHD code ported over to v7 i'll ship a tested-by tag pretty quick.
> > > > 
> > > > The super-set release will complicate a few things but this doesn't
> > > > look like a blocker on our end, just a change to how we track bits in a
> > > > shared bit/bytemap.
> > > >   
> > > 
> > > Hi Gregory,
> > > Thanks for reviewing the patches so quickly. 
> > > 
> > > No pressure, but look forward to your MHD work. :)
> > > 
> > > Fan  
> > 
> > Starting to get into versioniong hell a bit, since the Niagara work was
> > based off of jonathan's branch and the mhd-dcd work needs some of the
> > extentions from that branch - while this branch is based on master.
> > 
> > Probably we'll need to wait for a new cxl dated branch to try and sus
> > out the pain points before we push an RFC.  I would not want to have
> > conflicting commits for something like this for example:
> > 
> > https://lore.kernel.org/qemu-devel/20230901012914.226527-2-gregory.pr...@memverge.com/
> > 
> > We get merge conflicts here because this is behind that patch. So
> > pushing up an RFC in this state would be mostly useless to everyone
> 
> Subtle hint noted ;) 
>

Gentle nudge/poke/prod :P

Got your updates, thank you!  We should have something cleaned up today 
hopefully.

> I'll build a fresh tree - any remaining rebases until QEMU 9.0 should be
> straight forward anyway.   My ideal is that the NUMA GP series lands early
> in 9.1 cycle and this can go in parallel.  I'd really like to
> get this in early if possible so we can start clearing some of the other
> stuff that ended up built on top of it!
> 
> Jonathan
> 
> > 
> > ~Gregory
>

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

2024-04-22 Thread Marcin Juszkiewicz


W dniu 22.04.2024 o 16:18, Peter Maydell pisze:

On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz



  From what I see in EDK2 code we read CNTFREQ_EL0:

GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
ArmReadCntFrq() -> "mrs x0, cntfrq_el0"


Yeah, it looks like it's TF-A that hardcodes the frequency:
https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269

I imagine that value gets written into CNTFRQ by TF-A somewhere
along the line (and then read by EDK2 later), though I haven't
quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
assumes that the generic-timer frequency and the watchdog
timer frequency are the same, which is a bit dubious IMHO.

It also seems to be hardcoded in TF-A's support for the virt
board too, annoyingly.


I looked at it and it seems that TF-A can just read what is in 
CNTFRQ_EL0 instead of hardcoding the value.


Submitted patch:

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/28313
refactor(qemu): do not hardcode counter frequency [NEW]

Re: [PATCH v2] tests/unit: Remove debug statements in test-nested-aio-poll.c

2024-04-22 Thread Stefan Hajnoczi

On Mon, Apr 22, 2024 at 01:22:46PM +0200, Philippe Mathieu-Daudé wrote:
> We have been running this test for almost a year; it
> is safe to remove its debug statements, which clutter
> CI jobs output:
> 
>   ▶  88/100 /nested-aio-poll  OK
>   io_read 0x16bb26158
>   io_poll_true 0x16bb26158
>   > io_poll_ready
>   io_read 0x16bb26164
>   < io_poll_ready
>   io_poll_true 0x16bb26158
>   io_poll_false 0x16bb26164
>   > io_poll_ready
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_poll_false 0x16bb26164
>   io_read 0x16bb26164
>   < io_poll_ready
>   88/100 qemu:unit / test-nested-aio-pollOK
> 
> Reviewed-by: Eric Blake 
> Reviewed-by: Richard Henderson 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  tests/unit/test-nested-aio-poll.c | 7 ---
>  1 file changed, 7 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

Re: [PATCH v3 5/6] target/arm: Do memory type alignment check when translation disabled

2024-04-22 Thread Clément Chigot

Hi Richard,

While testing the future V9, I've some regressions on a custom board
using cortex-R5 CPUs.
Unaligned data accesses are no longer allowed because of that patch.

I've dug into the various documentation and it seems that R-profile
CPUs don't have the same default memory type as A-profile. It depends
on a default memory map provided in the R-Profile RM in C1.3 [1], even
when PMU is disabled.

> Each PMSAv8-32 MPU has an associated default memory map which is used when 
> the MPU is not enabled.
> ...
> Table C1-4 and Table C1-5 describe the default memory map defined for the EL1 
> MPU.

For our case, Table C1-5 can be simplified as:
 |  0x – 0x7FFF Normal
 |  0x8000 – 0xBFFF Device-nGnRE
 |  0xC000 – 0x Device-nGnRnE

Therefore, we can't blindly enable strict alignment checking solely on
SCTLR bits. We should make it depend on the address targeted. But is
it possible to know that address in `aprofile_require_alignment` ?
with `mmu_idx` ?

By the way, are R-Profile CPUs the same as those having the `PMSA`
feature ? That could mean we can use the `ARM_FEATURE_PMSA` to deal
with that, instead of create a new `ARM_FEATURE_R`

Note that the RM I've linked is for ARMv8. But this other link [2]
seems to show a similar behavior for arm-v7.

cc Jonathan and Ard, though not sure this is the same bug you've
reported earlier.

Thanks,
Clément
[1] https://developer.arm.com/documentation/ddi0568/a-c/?lang=en
[2] 
https://developer.arm.com/documentation/ddi0406/cb/System-Level-Architecture/Protected-Memory-System-Architecture--PMSA-/About-the-PMSA/Enabling-and-disabling-the-MPU?lang=en#BEIJEFCJ

On Fri, Mar 1, 2024 at 9:43 PM Richard Henderson
 wrote:
>
> If translation is disabled, the default memory type is Device, which
> requires alignment checking.  This is more optimally done early via
> the MemOp given to the TCG memory operation.
>
> Reviewed-by: Philippe Mathieu-Daudé 
> Reported-by: Idan Horowitz 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1204
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/tcg/hflags.c | 34 --
>  1 file changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/tcg/hflags.c b/target/arm/tcg/hflags.c
> index 8e5d35d922..5da1b0fc1d 100644
> --- a/target/arm/tcg/hflags.c
> +++ b/target/arm/tcg/hflags.c
> @@ -26,6 +26,35 @@ static inline bool fgt_svc(CPUARMState *env, int el)
>  FIELD_EX64(env->cp15.fgt_exec[FGTREG_HFGITR], HFGITR_EL2, SVC_EL1);
>  }
>
> +/* Return true if memory alignment should be enforced. */
> +static bool aprofile_require_alignment(CPUARMState *env, int el, uint64_t 
> sctlr)
> +{
> +#ifdef CONFIG_USER_ONLY
> +return false;
> +#else
> +/* Check the alignment enable bit. */
> +if (sctlr & SCTLR_A) {
> +return true;
> +}
> +
> +/*
> + * If translation is disabled, then the default memory type is
> + * Device(-nGnRnE) instead of Normal, which requires that alignment
> + * be enforced.  Since this affects all ram, it is most efficient
> + * to handle this during translation.
> + */
> +if (sctlr & SCTLR_M) {
> +/* Translation enabled: memory type in PTE via MAIR_ELx. */
> +return false;
> +}
> +if (el < 2 && (arm_hcr_el2_eff(env) & (HCR_DC | HCR_VM))) {
> +/* Stage 2 translation enabled: memory type in PTE. */
> +return false;
> +}
> +return true;
> +#endif
> +}
> +
>  static CPUARMTBFlags rebuild_hflags_common(CPUARMState *env, int fp_el,
> ARMMMUIdx mmu_idx,
> CPUARMTBFlags flags)
> @@ -121,8 +150,9 @@ static CPUARMTBFlags rebuild_hflags_a32(CPUARMState *env, 
> int fp_el,
>  {
>  CPUARMTBFlags flags = {};
>  int el = arm_current_el(env);
> +uint64_t sctlr = arm_sctlr(env, el);
>
> -if (arm_sctlr(env, el) & SCTLR_A) {
> +if (aprofile_require_alignment(env, el, sctlr)) {
>  DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
>  }
>
> @@ -223,7 +253,7 @@ static CPUARMTBFlags rebuild_hflags_a64(CPUARMState *env, 
> int el, int fp_el,
>
>  sctlr = regime_sctlr(env, stage1);
>
> -if (sctlr & SCTLR_A) {
> +if (aprofile_require_alignment(env, el, sctlr)) {
>  DP_TBFLAG_ANY(flags, ALIGN_MEM, 1);
>  }
>
> --
> 2.34.1
>
>

Re: [PATCH] target/arm: fix MPIDR value for ARM CPUs with SMT


On 4/22/24 08:21, Richard Henderson wrote:

For Arm's CPUs they fall into two categories:
  * older ones don't set MT in their MPIDR, and the Aff0
    field is effectively the CPU number
  * newer ones do set MT in their MPIDR, but don't have
    SMT, so their Aff0 is always 0 and their Aff1
    is the CPU number

Of all the CPUs we model, none of them are the
architecturally-permitted "MT is set, CPU implements
actual SMT, Aff0 indicates the thread in the CPU" type.


Looking at the TRM, Neoverse-E1 is "MT is set, actual SMT,
Aff0 is the thread" (Aff0 can be 0 or 1). We just don't
model that CPU type yet. But we should probably make
sure we don't block ourselves into a corner where that
would be awkward -- I'll have a think about this and
look at what x86 does with the topology info.


I'm suggesting that we set things up per -smp, and if the user chooses a -cpu value for 
which that topology doesn't make sense, we do it anyway and let them keep both pieces.


... but more practically, it allows experimentation at -cpu max, without having to build 
in anything cpu-specific.  Good to know about the E1 though...



r~

Re: [PATCH] target/arm: fix MPIDR value for ARM CPUs with SMT


On 4/22/24 04:26, Peter Maydell wrote:

On Mon, 22 Apr 2024 at 11:46, Peter Maydell  wrote:


On Sun, 21 Apr 2024 at 06:40, Richard Henderson
 wrote:

--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1314,8 +1314,18 @@ static void arm_cpu_dump_state(CPUState *cs, FILE *f, 
int flags)
   }
   }

-uint64_t arm_build_mp_affinity(int idx, uint8_t clustersz)
+uint64_t arm_build_mp_affinity(ARMCPU *cpu, int idx, uint8_t clustersz)
   {
+if (cpu->has_smt) {
+/*
+ * Right now, the ARM CPUs with SMT supported by QEMU only have
+ * one thread per core. So Aff0 is always 0.
+ */


Well, this isn't true.

  -smp 
[[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]
 
[,dies=dies][,clusters=clusters][,cores=cores][,threads=threads]

I would expect all of Aff[0-3] to be settable with the proper topology 
parameters.


As I understand it the MPIDR value is more or less independent
of the topology information as presented to the guest OS.
The options to the -smp command set the firmware topology
information, which doesn't/shouldn't affect the reported
MPIDR values, and in particular shouldn't change whether
the CPU selected has the MT bit set or not.

For Arm's CPUs they fall into two categories:
  * older ones don't set MT in their MPIDR, and the Aff0
field is effectively the CPU number
  * newer ones do set MT in their MPIDR, but don't have
SMT, so their Aff0 is always 0 and their Aff1
is the CPU number

Of all the CPUs we model, none of them are the
architecturally-permitted "MT is set, CPU implements
actual SMT, Aff0 indicates the thread in the CPU" type.


Looking at the TRM, Neoverse-E1 is "MT is set, actual SMT,
Aff0 is the thread" (Aff0 can be 0 or 1). We just don't
model that CPU type yet. But we should probably make
sure we don't block ourselves into a corner where that
would be awkward -- I'll have a think about this and
look at what x86 does with the topology info.


I'm suggesting that we set things up per -smp, and if the user chooses a -cpu value for 
which that topology doesn't make sense, we do it anyway and let them keep both pieces.



r~

Re: [PATCH v7 00/12] Enabling DCD emulation support in Qemu

2024-04-22 Thread Jonathan Cameron via

On Mon, 22 Apr 2024 15:23:16 +0100
Jonathan Cameron  wrote:

> On Mon, 22 Apr 2024 13:04:48 +0100
> Jonathan Cameron  wrote:
> 
> > On Sat, 20 Apr 2024 16:35:46 -0400
> > Gregory Price  wrote:
> >   
> > > On Fri, Apr 19, 2024 at 11:43:14AM -0700, fan wrote:
> > > > On Fri, Apr 19, 2024 at 02:24:36PM -0400, Gregory Price wrote:  
> > > > > 
> > > > > added review to all patches, will hopefully be able to add a Tested-by
> > > > > tag early next week, along with a v1 RFC for MHD bit-tracking.
> > > > > 
> > > > > We've been testing v5/v6 for a bit, so I expect as soon as we get the
> > > > > MHD code ported over to v7 i'll ship a tested-by tag pretty quick.
> > > > > 
> > > > > The super-set release will complicate a few things but this doesn't
> > > > > look like a blocker on our end, just a change to how we track bits in 
> > > > > a
> > > > > shared bit/bytemap.
> > > > >   
> > > > 
> > > > Hi Gregory,
> > > > Thanks for reviewing the patches so quickly. 
> > > > 
> > > > No pressure, but look forward to your MHD work. :)
> > > > 
> > > > Fan  
> > > 
> > > Starting to get into versioniong hell a bit, since the Niagara work was
> > > based off of jonathan's branch and the mhd-dcd work needs some of the
> > > extentions from that branch - while this branch is based on master.
> > > 
> > > Probably we'll need to wait for a new cxl dated branch to try and sus
> > > out the pain points before we push an RFC.  I would not want to have
> > > conflicting commits for something like this for example:
> > > 
> > > https://lore.kernel.org/qemu-devel/20230901012914.226527-2-gregory.pr...@memverge.com/
> > > 
> > > We get merge conflicts here because this is behind that patch. So
> > > pushing up an RFC in this state would be mostly useless to everyone
> > 
> > Subtle hint noted ;) 
> > 
> > I'll build a fresh tree - any remaining rebases until QEMU 9.0 should be
> > straight forward anyway.   My ideal is that the NUMA GP series lands early
> > in 9.1 cycle and this can go in parallel.  I'd really like to
> > get this in early if possible so we can start clearing some of the other
> > stuff that ended up built on top of it!  
> 
> I've pushed to gitlab.com/jic23/qemu cxl-2024-04-22-draft
> Its extremely lightly tested so far.
> 
> To save time, I've temporarily dropped the fm-api DCD initiate
> dynamic capacity add patch as that needs non trivial updates.
> 
> I've not yet caught up with some other outstanding series, but
> I will almost certainly put them on top of DCD.

If anyone pulled in meantime... I failed to push down a fix from
my working tree on top of this.
Goes to show I shouldn't ignore patches simply named "Push down" :(

Updated on same branch.

Jonathan
> 
> Jonathan
> 
> > 
> > Jonathan
> >   
> > > 
> > > ~Gregory
> > 
> >   
> 
>

Re: [PATCH v2 5/6] hw/ppc: SPI controller wiring to P10 chip and create seeprom device

2024-04-22 Thread Cédric Le Goater


On 4/9/24 19:56, Chalapathi V wrote:

In this commit
Creates SPI controller on p10 chip.
Create the keystore seeprom of type "seeprom-25csm04"
Connect the cs of seeprom to PIB_SPIC[2] cs irq.

The QOM tree of spi controller and seeprom are.
/machine (powernv10-machine)
   /chip[0] (power10_v2.0-pnv-chip)
 /pib_spic[2] (pnv-spi-controller)
   /bus (pnv-spi-bus)
 /pnv-spi-bus.2 (SSI)
   /xscom-spi-controller-regs[0] (memory-region)

/machine (powernv10-machine)
   /unattached (container)
 /device[7] (seeprom-25csm04)
   /ssi-gpio-cs[0] (irq)

(qemu) qom-get /machine/unattached/device[7] "parent_bus"
"/machine/chip[0]/pib_spic[2]/bus/pnv-spi-bus.2"

Signed-off-by: Chalapathi V 
---
  include/hw/ppc/pnv_chip.h |  3 +++
  hw/ppc/pnv.c  | 36 +++-
  2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/include/hw/ppc/pnv_chip.h b/include/hw/ppc/pnv_chip.h
index 8589f3291e..3edf13e8f9 100644
--- a/include/hw/ppc/pnv_chip.h
+++ b/include/hw/ppc/pnv_chip.h
@@ -6,6 +6,7 @@
  #include "hw/ppc/pnv_core.h"
  #include "hw/ppc/pnv_homer.h"
  #include "hw/ppc/pnv_n1_chiplet.h"
+#include "hw/ppc/pnv_spi_controller.h"
  #include "hw/ppc/pnv_lpc.h"
  #include "hw/ppc/pnv_occ.h"
  #include "hw/ppc/pnv_psi.h"
@@ -118,6 +119,8 @@ struct Pnv10Chip {
  PnvSBE   sbe;
  PnvHomer homer;
  PnvN1Chiplet n1_chiplet;
+#define PNV10_CHIP_MAX_PIB_SPIC 6
+PnvSpiController pib_spic[PNV10_CHIP_MAX_PIB_SPIC];
  
  uint32_t nr_quads;

  PnvQuad  *quads;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 6e3a5ccdec..eeb2d650bd 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -46,6 +46,7 @@
  #include "hw/pci-host/pnv_phb.h"
  #include "hw/pci-host/pnv_phb3.h"
  #include "hw/pci-host/pnv_phb4.h"
+#include "hw/ssi/ssi.h"
  
  #include "hw/ppc/xics.h"

  #include "hw/qdev-properties.h"
@@ -1829,6 +1830,11 @@ static void pnv_chip_power10_instance_init(Object *obj)
  for (i = 0; i < pcc->i2c_num_engines; i++) {
  object_initialize_child(obj, "i2c[*]", >i2c[i], TYPE_PNV_I2C);
  }
+
+for (i = 0; i < PNV10_CHIP_MAX_PIB_SPIC ; i++) {
+object_initialize_child(obj, "pib_spic[*]", >pib_spic[i],
+TYPE_PNV_SPI_CONTROLLER);
+}
  }
  
  static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)

@@ -2043,7 +2049,35 @@ static void pnv_chip_power10_realize(DeviceState *dev, 
Error **errp)
qdev_get_gpio_in(DEVICE(>psi),
 PSIHB9_IRQ_SBE_I2C));
  }
-
+/* PIB SPI Controller */
+for (i = 0; i < PNV10_CHIP_MAX_PIB_SPIC; i++) {
+object_property_set_int(OBJECT(>pib_spic[i]), "spic_num",
+i , _fatal);
+/*
+ * The TPM attached SPIC needs to reverse the bit order in each byte
+ * it sends to the TPM.
+ */
+if (i == 4) {
+object_property_set_bool(OBJECT(>pib_spic[i]),
+"reverse_bits", true, _fatal);
+}


or

  object_property_set_bool(OBJECT(>pib_spic[i]),
"reverse_bits", (i == 4) , _fatal);


That said. This setting looks weird to me.

Why do we need to reverse the bits ? is it an endian issue ?

Are there other SPI devices on the buses ?


+if (!qdev_realize(DEVICE(>pib_spic[i]), NULL, errp)) {
+return;
+}
+pnv_xscom_add_subregion(chip, PNV10_XSCOM_PIB_SPIC_BASE +
+i * PNV10_XSCOM_PIB_SPIC_SIZE,
+>pib_spic[i].xscom_spic_regs);
+}



The devices below belong to the rainer machine it seems. We should introduce
a per-machine handler to create them like it was done for the I2C devices.
For this purpose, the PnvMachineClass::i2c_init) handler could be changed
to create all machine specific devices.


+/* Primary MEAS/MVPD/Keystore SEEPROM connected to pib_spic[2] */
+DeviceState *seeprom = qdev_new("seeprom-25csm04");
+qdev_prop_set_string(seeprom, "filename",
+ "sbe_measurement_seeprom.bin.ecc");


This should be done differently. Here is a command line example :

$ qemu-system-arm -M ast2600-evb \
  -blockdev node-name=fmc0,driver=file,filename=/path/to/fmc0.img \
  -device mx66u51235f,bus=ssi.0,cs=0x0,drive=fmc0 \
  -blockdev node-name=fmc1,driver=file,filename=/path/to/fmc1.img \
  -device mx66u51235f,bus=ssi.0,cs=0x1,drive=fmc1 \
  -blockdev node-name=spi1,driver=file,filename=/path/to/spi1.img \
  -device mx66u51235f,cs=0x0,bus=ssi.1,drive=spi1 \
  ...

Please try to rework "seeprom-25csm04" on top of "m25p80". It should help.



+ssi_realize_and_unref(seeprom, ((>pib_spic[2])->bus).ssi_bus,
+  _fatal);
+qemu_irq seeprom_cs = qdev_get_gpio_in_named(seeprom, SSI_GPIO_CS, 0);
+Object *bus = OBJECT(&(>pib_spic[2])->bus);
+

Re: [PATCH v3 43/49] qapi, i386: Move kernel-hashes to SevCommonProperties

Daniel P. Berrangé  writes:

> On Wed, Mar 20, 2024 at 03:39:39AM -0500, Michael Roth wrote:
>> From: Dov Murik 
>> 
>> In order to enable kernel-hashes for SNP, pull it from
>> SevGuestProperties to its parent SevCommonProperties so
>> it will be available for both SEV and SNP.
>> 
>> Signed-off-by: Dov Murik 
>> Signed-off-by: Michael Roth 
>> ---
>>  qapi/qom.json | 14 +++---
>>  target/i386/sev.c | 44 ++--
>>  2 files changed, 25 insertions(+), 33 deletions(-)
>
> This change ought to be squashed into the earlier patch
> that introduce sev-guest-common.

Concur.

[...]

Re: [PATCH v3 36/49] i386/sev: Add KVM_EXIT_VMGEXIT handling for Extended Guest Requests

Michael Roth  writes:

> The GHCB specification[1] defines a VMGEXIT-based Guest Request
> hypercall to allow an SNP guest to issue encrypted requests directly to
> SNP firmware to do things like query the attestation report for the
> guest. These are generally handled purely in the kernel.
>
> In some some cases, it's useful for the host to be able to additionally
> supply the certificate chain for the signing key that SNP firmware uses
> to sign these attestation reports. To allow for, the GHCB specification
> defines an Extended Guest Request where this certificate data can be
> provided in a special format described in the GHCB spec. This
> certificate data may be global or guest-specific depending on how the
> guest was configured. Rather than providing interfaces to manage these
> within the kernel, KVM handles this by forward the Extended Guest
> Requests on to userspace so the certificate data can be provided in the
> expected format.
>
> Add a certs-path parameter to the sev-snp-guest object so that it can
> be used to inject any certificate data into these Extended Guest
> Requests.
>
> Signed-off-by: Michael Roth 
> ---
>  qapi/qom.json |  7 +++-
>  target/i386/sev.c | 85 +++
>  2 files changed, 91 insertions(+), 1 deletion(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index b25a3043da..7ba778af91 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -957,6 +957,10 @@
>  # SNP_LAUNCH_FINISH command in the SEV-SNP firmware ABI
>  # (default: all-zero)
>  #
> +# @certs-path: path to certificate data that can be passed to guests via
> +#  SNP Extended Guest Requests. File should be in the format
> +#  described in the GHCB specification. (default: none)

Is this a filename, or is it a search path of sorts?

> +#
>  # Since: 7.2
>  ##
>  { 'struct': 'SevSnpGuestProperties',
> @@ -967,7 +971,8 @@
>  '*id-block': 'str',
>  '*id-auth': 'str',
>  '*auth-key-enabled': 'bool',
> -'*host-data': 'str' } }
> +'*host-data': 'str',
> +'*certs-path': 'str' } }
>  
>  ##
>  # @ThreadContextProperties:

[...]

Re: [PATCH v3 31/49] i386/sev: Update query-sev QAPI format to handle SEV-SNP

Michael Roth  writes:

> Most of the current 'query-sev' command is relevant to both legacy
> SEV/SEV-ES guests and SEV-SNP guests, with 2 exceptions:
>
>   - 'policy' is a 64-bit field for SEV-SNP, not 32-bit, and
> the meaning of the bit positions has changed
>   - 'handle' is not relevant to SEV-SNP
>
> To address this, this patch adds a new 'sev-type' field that can be
> used as a discriminator to select between SEV and SEV-SNP-specific
> fields/formats without breaking compatibility for existing management
> tools (so long as management tools that add support for launching
> SEV-SNP guest update their handling of query-sev appropriately).
>
> The corresponding HMP command has also been fixed up similarly.
>
> Signed-off-by: Michael Roth 
> ---
>  qapi/misc-target.json | 71 ++-
>  target/i386/sev.c | 50 --
>  target/i386/sev.h |  3 ++
>  3 files changed, 94 insertions(+), 30 deletions(-)
>
> diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> index 4e0a6492a9..daceb85d95 100644
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -47,6 +47,49 @@
> 'send-update', 'receive-update' ],
>'if': 'TARGET_I386' }
>  
> +##
> +# @SevGuestType:
> +#
> +# An enumeration indicating the type of SEV guest being run.
> +#
> +# @sev: The guest is a legacy SEV or SEV-ES guest.

Single space after ':', please.

Recommend a blank line between argument descriptions.

> +# @sev-snp: The guest is an SEV-SNP guest.
> +#
> +# Since: 6.2

The type is since 9.1, but its members become results of query-sev,
where they are since 2.12.  See also my reply to Daniel's question on
PATCH 21.

> +##
> +{ 'enum': 'SevGuestType',
> +  'data': [ 'sev', 'sev-snp' ],
> +  'if': 'TARGET_I386' }
> +
> +##
> +# @SevGuestInfo:
> +#
> +# Information specific to legacy SEV/SEV-ES guests.
> +#
> +# @policy: SEV policy value

I know you're just moving existing documentation.  Still: this is rather
sparse.  Where would I find what numbers to pass for @policy?

> +#
> +# @handle: SEV firmware handle
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'SevGuestInfo',
> +  'data': { 'policy': 'uint32',
> +'handle': 'uint32' },
> +  'if': 'TARGET_I386' }
> +
> +##
> +# @SevSnpGuestInfo:
> +#
> +# Information specific to SEV-SNP guests.
> +#
> +# @snp-policy: SEV-SNP policy value

Same question.

> +#
> +# Since: 6.2

9.1

> +##
> +{ 'struct': 'SevSnpGuestInfo',
> +  'data': { 'snp-policy': 'uint64' },
> +  'if': 'TARGET_I386' }
> +
>  ##
>  # @SevInfo:
>  #
> @@ -60,25 +103,25 @@
>  #
>  # @build-id: SEV FW build id
>  #
> -# @policy: SEV policy value
> -#
>  # @state: SEV guest state
>  #
> -# @handle: SEV firmware handle
> +# @sev-type: Type of SEV guest being run
>  #
>  # Since: 2.12
>  ##
> -{ 'struct': 'SevInfo',
> -'data': { 'enabled': 'bool',
> -  'api-major': 'uint8',
> -  'api-minor' : 'uint8',
> -  'build-id' : 'uint8',
> -  'policy' : 'uint32',
> -  'state' : 'SevState',
> -  'handle' : 'uint32'
> -},
> -  'if': 'TARGET_I386'
> -}
> +{ 'union': 'SevInfo',
> +  'base': { 'enabled': 'bool',
> +'api-major': 'uint8',
> +'api-minor' : 'uint8',
> +'build-id' : 'uint8',
> +'state' : 'SevState',
> +'sev-type' : 'SevGuestType' },
> +  'discriminator': 'sev-type',
> +  'data': {
> +  'sev': 'SevGuestInfo',
> +  'sev-snp': 'SevSnpGuestInfo' },
> +  'if': 'TARGET_I386' }
> +
>  
>  ##
>  # @query-sev:

[...]

[PATCH] hvf: arm: Remove PL1_WRITE_MASK

2024-04-22 Thread Zenghui Yu

As it had never been used since the first commit a1477da3ddeb ("hvf: Add
Apple Silicon support").

Signed-off-by: Zenghui Yu 
---
 target/arm/hvf/hvf.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 65a5601804..015e96a6d3 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -150,7 +150,6 @@ void hvf_arm_init_debug(void)
 
 #define HVF_SYSREG(crn, crm, op0, op1, op2) \
 ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
-#define PL1_WRITE_MASK 0x4
 
 #define SYSREG_OP0_SHIFT  20
 #define SYSREG_OP0_MASK   0x3
-- 
2.34.1

Re: [PATCH v2 4/6] hw/misc: Microchip's 25CSM04 SEEPROM model

2024-04-22 Thread Cédric Le Goater


Hello Chalapathi

On 4/9/24 19:56, Chalapathi V wrote:

This commit implements a Serial EEPROM utilizing the Serial Peripheral
Interface (SPI) compatible bus.
Currently implemented SEEPROM is Microchip's 25CSM04 which provides 4 Mbits
of Serial EEPROM utilizing the Serial Peripheral Interface (SPI) compatible
bus. The device is organized as 524288 bytes of 8 bits each (512Kbyte) and
is optimized for use in consumer and industrial applications where reliable
and dependable nonvolatile memory storage is essential.

This seeprom device is created from a parent "ssi-peripheral".



Can the hw/block/m25p80c model be extented instead ?


Thanks,

C.






Signed-off-by: Chalapathi V 
---
  include/hw/misc/seeprom_25csm04.h |  48 ++
  hw/misc/seeprom_25csm04.c | 780 ++
  hw/misc/Kconfig   |   3 +
  hw/misc/meson.build   |   1 +
  hw/ppc/Kconfig|   1 +
  5 files changed, 833 insertions(+)
  create mode 100644 include/hw/misc/seeprom_25csm04.h
  create mode 100644 hw/misc/seeprom_25csm04.c

diff --git a/include/hw/misc/seeprom_25csm04.h 
b/include/hw/misc/seeprom_25csm04.h
new file mode 100644
index 00..0343530354
--- /dev/null
+++ b/include/hw/misc/seeprom_25csm04.h
@@ -0,0 +1,48 @@
+/*
+ * 25CSM04 Serial EEPROM model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * The Microchip Technology Inc. 25CSM04 provides 4 Mbits of Serial EEPROM
+ * utilizing the Serial Peripheral Interface (SPI) compatible bus. The device
+ * is organized as 524288 bytes of 8 bits each (512Kbyte) and is optimized
+ * for use in consumer and industrial applications where reliable and
+ * dependable nonvolatile memory storage is essential
+ */
+
+#ifndef SEEPROM_25CSM04_H
+#define SEEPROM_25CSM04_H
+
+#include "hw/ssi/ssi.h"
+#include "qom/object.h"
+
+#define TYPE_SEEPROM_25CSM04 "seeprom-25csm04"
+
+OBJECT_DECLARE_SIMPLE_TYPE(SeepromCsm04, SEEPROM_25CSM04)
+
+typedef struct SeepromCsm04 {
+SSIPeripheral parent_object;
+
+char*file;
+char*file_name;
+uint8_t opcode;
+uint32_taddr;
+uint8_t rd_state;
+boollocked;
+boolcommand_byte;
+/* device registers */
+uint8_t status0;
+uint8_t status1;
+uint8_t dsn[16];
+uint8_t uplid[256];
+uint8_t mpr[8];
+uint8_t idr[5];
+} SeepromCsm04;
+
+uint32_t seeprom_transfer(SSIPeripheral *ss, uint32_t tx);
+void seeprom_realize(SSIPeripheral *dev, Error **errp);
+bool compute_addr(SeepromCsm04 *s, uint32_t tx);
+bool validate_addr(SeepromCsm04 *s);
+#endif /* PPC_PNV_SPI_SEEPROM_H */
diff --git a/hw/misc/seeprom_25csm04.c b/hw/misc/seeprom_25csm04.c
new file mode 100644
index 00..45df66e4b0
--- /dev/null
+++ b/hw/misc/seeprom_25csm04.c
@@ -0,0 +1,780 @@
+/*
+ * 25CSM04 Serial EEPROM model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/misc/seeprom_25csm04.h"
+#include "hw/qdev-properties.h"
+#include "qemu/datadir.h"
+#include 
+
+#define SPI_DEBUG(x)
+
+/*
+ * 2-byte STATUS register which is a combination of six nonvolatile bits of
+ * EEPROM and five volatile latches.
+ *
+ * status 0:
+ * bit 7 WPEN: Write-Protect Enable bit
+ * 1 = Write-Protect pin is enabled, 0 = Write-Protect pin is ignored
+ *
+ * bit 3-2 BP<1:0>: Block Protection bits
+ * 00 = No array write protection
+ * 01 = Upper quarter memory array protection
+ * 10 = Upper half memory array protection
+ * 11 = Entire memory array protection
+ *
+ * bit 1 WEL: Write Enable Latch bit
+ * 1 = WREN has been executed and device is enabled for writing
+ * 0 = Device is not write-enabled
+ *
+ * bit 0 RDY/BSY: Ready/Busy Status Latch bit
+ * 1 = Device is busy with an internal write cycle
+ * 0 = Device is ready for a new sequence
+ */
+#define STATUS0_WPEN0x7
+#define STATUS0_BP  0x2
+#define STATUS0_WEL 0x1
+#define STATUS0_BUSY0x0
+
+/*
+ * status 1:
+ * bit 7 WPM: Write Protection Mode bit(1)
+ * 1 = Enhanced Write Protection mode selected (factory default)
+ * 0 = Legacy Write Protection mode selected
+ *
+ * bit 6 ECS: Error Correction State Latch bit
+ * 1 = The previously executed read sequence did require the ECC
+ * 0 = The previous executed read sequence did not require the ECC
+ *
+ * bit 5 FMPC: Freeze Memory Protection Configuration bit(2)
+ * 1 = Memory Partition registers and write protection mode are permanently
+ * frozen and cannot be modified
+ * 0 = Memory Partition registers and write protection mode are not frozen
+ * and are modifiable
+ *
+ * bit 4 PREL: Partition Register Write Enable Latch bit
+ * 1 = PRWE has been executed and WMPR, FRZR and PPAB instructions are enabled
+ * 0 = WMPR, FRZR and PPAB instructions are disabled
+ *
+ * bit 3 PABP: Partition Address Boundary Protection bit
+ *

Re: [PATCH] target/riscv/kvm: tolerate KVM disable ext errors

2024-04-22 Thread Andrew Jones

On Mon, Apr 22, 2024 at 11:08:31AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 4/22/24 10:43, Andrew Jones wrote:
> > On Mon, Apr 22, 2024 at 10:12:53AM -0300, Daniel Henrique Barboza wrote:
> > > Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
> > > enabled, will fail with a kernel oops SIGILL right at the start. The
> > > reason is that we can't expose zkr without implementing the SEED CSR.
> > > Disabling zkr in the guest would be a workaround, but if the KVM doesn't
> > > allow it we'll error out and never boot.
> > > 
> > > In hindsight this is too strict. If we keep proceeding, despite not
> > > disabling the extension in the KVM vcpu, we'll not add extension in
> >  ^ the
> > > riscv,isa. The guest kernel will be unaware of the extension, i.e. it
> >   ^ the
> > 
> > > doesn't matter if the KVM vcpu has it enabled underneath or not. So it's
> > > ok to keep booting in this case.
> > > 
> > > Change our current logic to not error out if we fail to disable an
> > > extension in kvm_set_one_reg(): throw an warning instead and keep
> > > booting.  We'll keep the existing behavior when we fail to enable an
> > > extension in KVM, since adding the extension in riscv,isa at this point
> > > will cause a guest malfunction because the extension isn't enabled in
> > > the vcpu.
> > 
> > I'd probably add a sentence or two explaining why we still want to warn,
> > which is because KVM is stating that while you may think you're disabling
> > an extension, that extension's behavior (instructions, etc.) will still
> > be exposed to the guest since there's no way not to expose it.  The user
> > should be aware that a guest could use it anyway, since that means the
> > attempt to disable it can't be relied upon for migration compatibility of
> > arbitrary guests. The guest needs to be well behaved, i.e. one that won't
> > try to use any extensions which aren't in the ISA string. So, disabling
> > these types of extensions either shouldn't generally be done (so a noisy
> > warning helps prohibit that) or done for debug purposes (where a noisy
> > warning is fine).
> 
> I'll add this in the next version.
> 
> > 
> > > 
> > > Suggested-by: Andrew Jones 
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   target/riscv/kvm/kvm-cpu.c | 12 
> > >   1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > > index 6a6c6cae80..261ca24504 100644
> > > --- a/target/riscv/kvm/kvm-cpu.c
> > > +++ b/target/riscv/kvm/kvm-cpu.c
> > > @@ -427,10 +427,14 @@ static void 
> > > kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU *cpu, CPUState *cs)
> > >   reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
> > >   ret = kvm_set_one_reg(cs, id, );
> > >   if (ret != 0) {
> > > -error_report("Unable to %s extension %s in KVM, error %d",
> > > - reg ? "enable" : "disable",
> > > - multi_ext_cfg->name, ret);
> > > -exit(EXIT_FAILURE);
> > > +if (reg) {
> > > +error_report("Unable to enable extension %s in KVM, 
> > > error %d",
> > > + multi_ext_cfg->name, ret);
> > > +exit(EXIT_FAILURE);
> > > +} else {
> > 
> > Maybe we should check for a specific error. Is it EINVAL? If we do, then
> > the message below would drop the (error %d) part and instead state
> > explicitly that it cannot disable this extension since it's not possible.
> 
> It's throwing a 22 (EINVAL).
> 
> > 
> > > +warn_report("KVM did not disable extension %s (error 
> > > %d)",
> > 
> > s/did not/cannot/
> > 
> > > +multi_ext_cfg->name, ret);
> > > +}
> 
> 
> I don't mind rephrasing it to "The KVM module is not able to disable 
> extension %s"
> But I'm unsure what we gain from not showing the error code.
> 
> Aside EINVAL the other (very unlikely) error code being thrown would be 
> EBUSY. Should
> we handle EBUSY differently in this case? I don't think so. If we're already 
> throwing a
> warning, with error code, the user is well informed about the guest having a 
> flaky
> start and can proceed with discretion. Regardless of the extension 
> disablement failing
> due to EINVAL or EBUSY.

But EBUSY means the VMM messed up and tried to do something it shouldn't,
which is to change the configuration of the VCPU after it already ran.
EINVAL isn't a VMM mess up. It wanted to disable the extension and will
remove it from the ISA string, KVM just can't guarantee that the guest
won't be able to use it anyway (making that a warn case). So, EBUSY isn't
a warn case (but it's also not something we'd expect to see in a function
called from kvm_arch_init_vcpu()). However, KVM is free to add new errors,
so I'd only warn for the ones which translate to warn cases (EINVAL).

Thanks,
drew

Re: [PATCH v2] hw/audio/virtio-snd: Use device endianness instead of target one


On 22/4/24 15:45, Manos Pitsidianakis wrote:
On Mon, 22 Apr 2024 16:13, Philippe Mathieu-Daudé  
wrote:

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.


Hey Philippe, can you clarify what do you mean by they can change 
endianness at runtime?


The target's one is used because that's the one it will be using to do 
I/O with its kernel's audio interface.


See for example in SysemuCPUOps:

/**
 * @virtio_is_big_endian: Callback to return %true if a CPU which
 * supports runtime configurable endianness is currently big-endian.
 * Non-configurable CPUs can use the default implementation of this
 * method.
 * This method should not be used by any callers other than the
 * pre-1.0 virtio devices.
 */
bool (*virtio_is_big_endian)(CPUState *cpu);

and:

/**
 * cpu_virtio_is_big_endian:
 * @cpu: CPU

 * Returns %true if a CPU which supports runtime configurable endianness
 * is currently big-endian.
 */
bool cpu_virtio_is_big_endian(CPUState *cpu);

From commit 616a655219 ("virtio: add endian-ambivalent support to 
VirtIODevice"):


Some CPU families can dynamically change their endianness. This
means we can have little endian ppc or big endian arm guests for
example. This has an impact on legacy virtio data structures since
they are target endian.
We hence introduce a new property to track the endianness of each
virtio device. It is reasonnably assumed that endianness won't
change while the device is in use : we hence capture the device
endianness when it gets reset.





Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/audio/virtio-snd.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..82c5647660 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -395,13 +395,15 @@ static uint32_t 
virtio_snd_get_qemu_freq(uint32_t rate)
 * Get QEMU Audiosystem compatible audsettings from virtio based pcm 
stream

 * params.
 */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, 
audsettings *as,
    virtio_snd_pcm_set_params 
*params)

{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
    as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
    as->fmt = virtio_snd_get_qemu_format(params->format);
    as->freq = virtio_snd_get_qemu_freq(params->rate);
-    as->endianness = target_words_bigendian() ? 1 : 0;
+    as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
}

/*
@@ -464,7 +466,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound 
*s, uint32_t stream_id)

    s->pcm->streams[stream_id] = stream;
    }

-    virtio_snd_get_qemu_audsettings(, params);
+    virtio_snd_get_qemu_audsettings(s, , params);
    stream->info.direction = stream_id < s->snd_conf.streams / 2 +
    (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : 
VIRTIO_SND_D_INPUT;

    stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
--
2.41.0

Re: [PATCH v7 00/12] Enabling DCD emulation support in Qemu

2024-04-22 Thread Jonathan Cameron via

On Mon, 22 Apr 2024 13:04:48 +0100
Jonathan Cameron  wrote:

> On Sat, 20 Apr 2024 16:35:46 -0400
> Gregory Price  wrote:
> 
> > On Fri, Apr 19, 2024 at 11:43:14AM -0700, fan wrote:  
> > > On Fri, Apr 19, 2024 at 02:24:36PM -0400, Gregory Price wrote:
> > > > 
> > > > added review to all patches, will hopefully be able to add a Tested-by
> > > > tag early next week, along with a v1 RFC for MHD bit-tracking.
> > > > 
> > > > We've been testing v5/v6 for a bit, so I expect as soon as we get the
> > > > MHD code ported over to v7 i'll ship a tested-by tag pretty quick.
> > > > 
> > > > The super-set release will complicate a few things but this doesn't
> > > > look like a blocker on our end, just a change to how we track bits in a
> > > > shared bit/bytemap.
> > > > 
> > > 
> > > Hi Gregory,
> > > Thanks for reviewing the patches so quickly. 
> > > 
> > > No pressure, but look forward to your MHD work. :)
> > > 
> > > Fan
> > 
> > Starting to get into versioniong hell a bit, since the Niagara work was
> > based off of jonathan's branch and the mhd-dcd work needs some of the
> > extentions from that branch - while this branch is based on master.
> > 
> > Probably we'll need to wait for a new cxl dated branch to try and sus
> > out the pain points before we push an RFC.  I would not want to have
> > conflicting commits for something like this for example:
> > 
> > https://lore.kernel.org/qemu-devel/20230901012914.226527-2-gregory.pr...@memverge.com/
> > 
> > We get merge conflicts here because this is behind that patch. So
> > pushing up an RFC in this state would be mostly useless to everyone  
> 
> Subtle hint noted ;) 
> 
> I'll build a fresh tree - any remaining rebases until QEMU 9.0 should be
> straight forward anyway.   My ideal is that the NUMA GP series lands early
> in 9.1 cycle and this can go in parallel.  I'd really like to
> get this in early if possible so we can start clearing some of the other
> stuff that ended up built on top of it!

I've pushed to gitlab.com/jic23/qemu cxl-2024-04-22-draft
Its extremely lightly tested so far.

To save time, I've temporarily dropped the fm-api DCD initiate
dynamic capacity add patch as that needs non trivial updates.

I've not yet caught up with some other outstanding series, but
I will almost certainly put them on top of DCD.

Jonathan

> 
> Jonathan
> 
> > 
> > ~Gregory  
> 
>

[PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Use virtio_is_big_endian()
v3: Remove "hw/core/cpu.h
---
 hw/audio/virtio-snd.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..939cd78026 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -24,7 +24,6 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "hw/audio/virtio-snd.h"
-#include "hw/core/cpu.h"
 
 #define VIRTIO_SOUND_VM_VERSION 1
 #define VIRTIO_SOUND_JACK_DEFAULT 0
@@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
  * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
  * params.
  */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
 virtio_snd_pcm_set_params *params)
 {
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
 as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
 as->fmt = virtio_snd_get_qemu_format(params->format);
 as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
 }
 
 /*
@@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 s->pcm->streams[stream_id] = stream;
 }
 
-virtio_snd_get_qemu_audsettings(, params);
+virtio_snd_get_qemu_audsettings(s, , params);
 stream->info.direction = stream_id < s->snd_conf.streams / 2 +
 (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
 stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
-- 
2.41.0

Re: [PATCH v2 1/1] ebpf: Added traces back. Changed source set for eBPF to 'system'.

2024-04-22 Thread Andrew Melnichenko

Hello, everyone.
Was added missed "trace.h"

Best regards.

On Mon, Apr 22, 2024 at 5:17 PM Andrew Melnychenko  wrote:
>
> There was an issue with Qemu build with "--disable-system".
> The traces could be generated and the build fails.
> The traces were 'cut out' for previous patches, and overall,
> the 'system' source set should be used like in pre-'eBPF blob' patches.
>
> Signed-off-by: Andrew Melnychenko 
> ---
>  ebpf/ebpf_rss.c  | 7 +++
>  ebpf/meson.build | 2 +-
>  ebpf/trace.h | 1 +
>  3 files changed, 9 insertions(+), 1 deletion(-)
>  create mode 100644 ebpf/trace.h
>
> diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> index d102f3dd092..87f0714910e 100644
> --- a/ebpf/ebpf_rss.c
> +++ b/ebpf/ebpf_rss.c
> @@ -25,6 +25,8 @@
>  #include "ebpf/rss.bpf.skeleton.h"
>  #include "ebpf/ebpf.h"
>
> +#include "trace.h"
> +
>  void ebpf_rss_init(struct EBPFRSSContext *ctx)
>  {
>  if (ctx != NULL) {
> @@ -55,18 +57,21 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> PROT_READ | PROT_WRITE, MAP_SHARED,
> ctx->map_configuration, 0);
>  if (ctx->mmap_configuration == MAP_FAILED) {
> +trace_ebpf_error("eBPF RSS", "can not mmap eBPF configuration 
> array");
>  return false;
>  }
>  ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
> PROT_READ | PROT_WRITE, MAP_SHARED,
> ctx->map_toeplitz_key, 0);
>  if (ctx->mmap_toeplitz_key == MAP_FAILED) {
> +trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz key");
>  goto toeplitz_fail;
>  }
>  ctx->mmap_indirections_table = mmap(NULL, qemu_real_host_page_size(),
> PROT_READ | PROT_WRITE, MAP_SHARED,
> ctx->map_indirections_table, 0);
>  if (ctx->mmap_indirections_table == MAP_FAILED) {
> +trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection table");
>  goto indirection_fail;
>  }
>
> @@ -108,12 +113,14 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
>
>  rss_bpf_ctx = rss_bpf__open();
>  if (rss_bpf_ctx == NULL) {
> +trace_ebpf_error("eBPF RSS", "can not open eBPF RSS object");
>  goto error;
>  }
>
>  bpf_program__set_type(rss_bpf_ctx->progs.tun_rss_steering_prog, 
> BPF_PROG_TYPE_SOCKET_FILTER);
>
>  if (rss_bpf__load(rss_bpf_ctx)) {
> +trace_ebpf_error("eBPF RSS", "can not load RSS program");
>  goto error;
>  }
>
> diff --git a/ebpf/meson.build b/ebpf/meson.build
> index c5bf9295a20..bff6156f518 100644
> --- a/ebpf/meson.build
> +++ b/ebpf/meson.build
> @@ -1 +1 @@
> -common_ss.add(when: libbpf, if_true: files('ebpf.c', 'ebpf_rss.c'), 
> if_false: files('ebpf_rss-stub.c'))
> +system_ss.add(when: libbpf, if_true: files('ebpf.c', 'ebpf_rss.c'), 
> if_false: files('ebpf_rss-stub.c'))
> diff --git a/ebpf/trace.h b/ebpf/trace.h
> new file mode 100644
> index 000..abefc46ab10
> --- /dev/null
> +++ b/ebpf/trace.h
> @@ -0,0 +1 @@
> +#include "trace/trace-ebpf.h"
> --
> 2.44.0
>

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

On Mon, 22 Apr 2024 at 14:38, Marcin Juszkiewicz
 wrote:
>
> W dniu 22.04.2024 o 14:56, Peter Maydell pisze:
> > On Fri, 19 Apr 2024 at 19:46, Peter Maydell  
> > wrote:
>
> >> The upshot is that the only CPU type that changes is 'max'; but any
> >> new type we add in future (whether v8.6 or not) will also get the new
> >> 1GHz default (assuming we spot in code review any attempts to set
> >> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> >> of cut-n-paste from an older CPU initfn ;-)).
> >>
> >> It remains the case that the machine model can override the default
> >> value via the 'cntfrq' QOM property (regardless of the CPU type).
> >
> > This patchset turns out to break the sbsa-ref board: the
> > Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
> > avocado test both (a) takes rather longer to boot and (b) when
> > running thinks that time is advancing very fast.
> >
> > I suspect this may be because the firmware hard-codes the
> > generic timer clock frequency it is expecting. I've cc'd the
> > sbsa-ref maintainers: is that correct?
> >
> > If so, we can deal with it by making the sbsa-ref board set the
> > cntfrq QOM property on its CPUs to force them to the old
> > frequency. However this will produce a technically-out-of-spec
> > CPU when used with a v8.6-compliant CPU type, so probably we
> > should do something to be able to tell the firmware the clock
> > frequency (if it doesn't want to just read CNTFRQ_EL0 itself).
>
>  From what I see in EDK2 code we read CNTFREQ_EL0:
>
> GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
> sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
> ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

Yeah, it looks like it's TF-A that hardcodes the frequency:
https://github.com/ARM-software/arm-trusted-firmware/blob/c8be7c08c3b3a2330d54b58651faa9438ff34f6e/plat/qemu/qemu_sbsa/include/platform_def.h#L269

I imagine that value gets written into CNTFRQ by TF-A somewhere
along the line (and then read by EDK2 later), though I haven't
quite found where. Plus I notice that the TF-A sbsa-watchdog-timer
assumes that the generic-timer frequency and the watchdog
timer frequency are the same, which is a bit dubious IMHO.

It also seems to be hardcoded in TF-A's support for the virt
board too, annoyingly.

thanks
-- PMM

[PATCH v2 1/1] ebpf: Added traces back. Changed source set for eBPF to 'system'.

2024-04-22 Thread Andrew Melnychenko

There was an issue with Qemu build with "--disable-system".
The traces could be generated and the build fails.
The traces were 'cut out' for previous patches, and overall,
the 'system' source set should be used like in pre-'eBPF blob' patches.

Signed-off-by: Andrew Melnychenko 
---
 ebpf/ebpf_rss.c  | 7 +++
 ebpf/meson.build | 2 +-
 ebpf/trace.h | 1 +
 3 files changed, 9 insertions(+), 1 deletion(-)
 create mode 100644 ebpf/trace.h

diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
index d102f3dd092..87f0714910e 100644
--- a/ebpf/ebpf_rss.c
+++ b/ebpf/ebpf_rss.c
@@ -25,6 +25,8 @@
 #include "ebpf/rss.bpf.skeleton.h"
 #include "ebpf/ebpf.h"
 
+#include "trace.h"
+
 void ebpf_rss_init(struct EBPFRSSContext *ctx)
 {
 if (ctx != NULL) {
@@ -55,18 +57,21 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
PROT_READ | PROT_WRITE, MAP_SHARED,
ctx->map_configuration, 0);
 if (ctx->mmap_configuration == MAP_FAILED) {
+trace_ebpf_error("eBPF RSS", "can not mmap eBPF configuration array");
 return false;
 }
 ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
PROT_READ | PROT_WRITE, MAP_SHARED,
ctx->map_toeplitz_key, 0);
 if (ctx->mmap_toeplitz_key == MAP_FAILED) {
+trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz key");
 goto toeplitz_fail;
 }
 ctx->mmap_indirections_table = mmap(NULL, qemu_real_host_page_size(),
PROT_READ | PROT_WRITE, MAP_SHARED,
ctx->map_indirections_table, 0);
 if (ctx->mmap_indirections_table == MAP_FAILED) {
+trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection table");
 goto indirection_fail;
 }
 
@@ -108,12 +113,14 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx)
 
 rss_bpf_ctx = rss_bpf__open();
 if (rss_bpf_ctx == NULL) {
+trace_ebpf_error("eBPF RSS", "can not open eBPF RSS object");
 goto error;
 }
 
 bpf_program__set_type(rss_bpf_ctx->progs.tun_rss_steering_prog, 
BPF_PROG_TYPE_SOCKET_FILTER);
 
 if (rss_bpf__load(rss_bpf_ctx)) {
+trace_ebpf_error("eBPF RSS", "can not load RSS program");
 goto error;
 }
 
diff --git a/ebpf/meson.build b/ebpf/meson.build
index c5bf9295a20..bff6156f518 100644
--- a/ebpf/meson.build
+++ b/ebpf/meson.build
@@ -1 +1 @@
-common_ss.add(when: libbpf, if_true: files('ebpf.c', 'ebpf_rss.c'), if_false: 
files('ebpf_rss-stub.c'))
+system_ss.add(when: libbpf, if_true: files('ebpf.c', 'ebpf_rss.c'), if_false: 
files('ebpf_rss-stub.c'))
diff --git a/ebpf/trace.h b/ebpf/trace.h
new file mode 100644
index 000..abefc46ab10
--- /dev/null
+++ b/ebpf/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-ebpf.h"
-- 
2.44.0

Re: [PATCH] target/riscv/kvm: tolerate KVM disable ext errors





On 4/22/24 10:43, Andrew Jones wrote:

On Mon, Apr 22, 2024 at 10:12:53AM -0300, Daniel Henrique Barboza wrote:

Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
enabled, will fail with a kernel oops SIGILL right at the start. The
reason is that we can't expose zkr without implementing the SEED CSR.
Disabling zkr in the guest would be a workaround, but if the KVM doesn't
allow it we'll error out and never boot.

In hindsight this is too strict. If we keep proceeding, despite not
disabling the extension in the KVM vcpu, we'll not add extension in

 ^ the

riscv,isa. The guest kernel will be unaware of the extension, i.e. it

  ^ the


doesn't matter if the KVM vcpu has it enabled underneath or not. So it's
ok to keep booting in this case.

Change our current logic to not error out if we fail to disable an
extension in kvm_set_one_reg(): throw an warning instead and keep
booting.  We'll keep the existing behavior when we fail to enable an
extension in KVM, since adding the extension in riscv,isa at this point
will cause a guest malfunction because the extension isn't enabled in
the vcpu.


I'd probably add a sentence or two explaining why we still want to warn,
which is because KVM is stating that while you may think you're disabling
an extension, that extension's behavior (instructions, etc.) will still
be exposed to the guest since there's no way not to expose it.  The user
should be aware that a guest could use it anyway, since that means the
attempt to disable it can't be relied upon for migration compatibility of
arbitrary guests. The guest needs to be well behaved, i.e. one that won't
try to use any extensions which aren't in the ISA string. So, disabling
these types of extensions either shouldn't generally be done (so a noisy
warning helps prohibit that) or done for debug purposes (where a noisy
warning is fine).


I'll add this in the next version.





Suggested-by: Andrew Jones 
Signed-off-by: Daniel Henrique Barboza 
---
  target/riscv/kvm/kvm-cpu.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..261ca24504 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU 
*cpu, CPUState *cs)
  reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
  ret = kvm_set_one_reg(cs, id, );
  if (ret != 0) {
-error_report("Unable to %s extension %s in KVM, error %d",
- reg ? "enable" : "disable",
- multi_ext_cfg->name, ret);
-exit(EXIT_FAILURE);
+if (reg) {
+error_report("Unable to enable extension %s in KVM, error %d",
+ multi_ext_cfg->name, ret);
+exit(EXIT_FAILURE);
+} else {


Maybe we should check for a specific error. Is it EINVAL? If we do, then
the message below would drop the (error %d) part and instead state
explicitly that it cannot disable this extension since it's not possible.


It's throwing a 22 (EINVAL).




+warn_report("KVM did not disable extension %s (error %d)",


s/did not/cannot/


+multi_ext_cfg->name, ret);
+}



I don't mind rephrasing it to "The KVM module is not able to disable extension 
%s"
But I'm unsure what we gain from not showing the error code.

Aside EINVAL the other (very unlikely) error code being thrown would be EBUSY. 
Should
we handle EBUSY differently in this case? I don't think so. If we're already 
throwing a
warning, with error code, the user is well informed about the guest having a 
flaky
start and can proceed with discretion. Regardless of the extension disablement 
failing
due to EINVAL or EBUSY.


Thanks,

Daniel


  }
  }
  }
--
2.44.0



Thanks,
drew

Re: [PATCH v2 2/6] hw/ppc: SPI controller model - registers implementation

2024-04-22 Thread Cédric Le Goater


On 4/16/24 19:02, Chalapathi V wrote:


On 15-04-2024 20:44, Cédric Le Goater wrote:

Hello Chalapathi

The subject could be rephrased to : "ppc/pnv: Add SPI controller model".

On 4/9/24 19:56, Chalapathi V wrote:

SPI controller device model supports a connection to a single SPI responder.
This provide access to SPI seeproms, TPM, flash device and an ADC controller.

All SPI function control is mapped into the SPI register space to enable full
control by firmware. In this commit SPI configuration component is modelled
which contains all SPI configuration and status registers as well as the hold
registers for data to be sent or having been received.

An existing QEMU SSI framework is used and SSI_BUS is created.

Signed-off-by: Chalapathi V 
---
  include/hw/ppc/pnv_spi_controller.h  |  55 +
  include/hw/ppc/pnv_spi_controller_regs.h | 114 ++


These two files should be under hw/ssi/ and include/hw/ssi/. Please
remove '_controller'.

Sure. Thank You.



  include/hw/ppc/pnv_xscom.h |   3 +
  hw/ppc/pnv_spi_controller.c  | 278 +++
  hw/ppc/Kconfig   |   1 +
  hw/ppc/meson.build   |   1 +
  6 files changed, 452 insertions(+)
  create mode 100644 include/hw/ppc/pnv_spi_controller.h
  create mode 100644 include/hw/ppc/pnv_spi_controller_regs.h
  create mode 100644 hw/ppc/pnv_spi_controller.c

diff --git a/include/hw/ppc/pnv_spi_controller.h 
b/include/hw/ppc/pnv_spi_controller.h
new file mode 100644
index 00..5ec50fb14c
--- /dev/null
+++ b/include/hw/ppc/pnv_spi_controller.h
@@ -0,0 +1,55 @@
+/*
+ * QEMU PowerPC SPI Controller model
+ *
+ * Copyright (c) 2024, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * This model Supports a connection to a single SPI responder.
+ * Introduced for P10 to provide access to SPI seeproms, TPM, flash device
+ * and an ADC controller.
+ */
+#include "hw/ssi/ssi.h"
+
+#ifndef PPC_PNV_SPI_CONTROLLER_H
+#define PPC_PNV_SPI_CONTROLLER_H
+
+#define TYPE_PNV_SPI_CONTROLLER "pnv-spi-controller"
+#define PNV_SPICONTROLLER(obj) \
+    OBJECT_CHECK(PnvSpiController, (obj), TYPE_PNV_SPI_CONTROLLER)


You could use OBJECT_DECLARE_SIMPLE_TYPE ? Anyhow, I would prefer
naming the macro PNV_SPI_CONTROLLER.


+#define SPI_CONTROLLER_REG_SIZE 8
+
+typedef struct SSIBus SSIBus;


why ?

I might have got compile time errors. I will recheck and update. Thank You.




+
+#define TYPE_PNV_SPI_BUS "pnv-spi-bus"
+OBJECT_DECLARE_SIMPLE_TYPE(PnvSPIBus, PNV_SPI_BUS)
+
+typedef struct PnvSPIBus {


I don't think this extra PnvSPIBus model is useful.


+    SysBusDevice parent_obj;
+
+    SSIBus *ssi_bus;
+    qemu_irq *cs_line;


These two attributes could live under PnvSpiController.

This is added to have a SysBusDevice parent so that I can use the busname in 
command line for TPM. I will add these in PnvSpiController with SysBusDevice 
parent and test.


You could still compute the bus name from pnv_spi_controller_realize()
and move all PnvSPIBus attributes under PnvSpiController. The PnvSPIBus
is not required.




+    uint32_t id;


and this one would become useless.


+} PnvSPIBus;

+typedef struct PnvSpiController {
+    DeviceState parent;
+
+    PnvSPIBus   bus;
+    MemoryRegion    xscom_spic_regs;
+    /* SPI controller object number */
+    uint32_t    spic_num;
+
+    /* SPI Controller registers */
+    uint64_t    error_reg;
+    uint64_t    counter_config_reg;
+    uint64_t    config_reg1;
+    uint64_t    clock_config_reset_control;
+    uint64_t    memory_mapping_reg;
+    uint64_t    transmit_data_reg;
+    uint64_t    receive_data_reg;
+    uint8_t sequencer_operation_reg[SPI_CONTROLLER_REG_SIZE];
+    uint64_t    status_reg;


You could use an array of uint64_t also.

Sure. I will try and check.


That's not a must have. Both approach work but since the memops use
the MMIO offest to address the register, it is sometime simpler to
use an array of uint64_t.







+} PnvSpiController;
+#endif /* PPC_PNV_SPI_CONTROLLER_H */
diff --git a/include/hw/ppc/pnv_spi_controller_regs.h 
b/include/hw/ppc/pnv_spi_controller_regs.h
new file mode 100644
index 00..6f613aca5e
--- /dev/null
+++ b/include/hw/ppc/pnv_spi_controller_regs.h
@@ -0,0 +1,114 @@
+/*
+ * QEMU PowerPC SPI Controller model
+ *
+ * Copyright (c) 2023, IBM Corporation.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef SPI_CONTROLLER_REGS_H
+#define SPI_CONTROLLER_REGS_H
+
+/* Error Register */
+#define ERROR_REG   0x00
+
+/* counter_config_reg */
+#define COUNTER_CONFIG_REG  0x01
+#define COUNTER_CONFIG_REG_SHIFT_COUNT_N1   PPC_BITMASK(0, 7)
+#define COUNTER_CONFIG_REG_SHIFT_COUNT_N2   PPC_BITMASK(8, 15)
+#define COUNTER_CONFIG_REG_COUNT_COMPARE1   PPC_BITMASK(24, 31)
+#define COUNTER_CONFIG_REG_COUNT_COMPARE2   PPC_BITMASK(32, 39)
+#define COUNTER_CONFIG_REG_N1_COUNT_CONTROL

[PATCH] target/riscv: change RISCV_EXCP_SEMIHOST exception number to 63

2024-04-22 Thread Clément Léger

The current semihost exception number (16) is a reserved number (range
[16-17]). The upcoming double trap specification uses that number for
the double trap exception. Since the privileged spec (Table 22) defines
ranges for custom uses change the semihosting exception number to 63
which belongs to the range [48-63] in order to avoid any future
collisions with reserved exception.

Signed-off-by: Clément Léger 

---
 target/riscv/cpu_bits.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index fc2068ee4d..74318a925c 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -670,11 +670,11 @@ typedef enum RISCVException {
 RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
 RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
 RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
-RISCV_EXCP_SEMIHOST = 0x10,
 RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
 RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT = 0x15,
 RISCV_EXCP_VIRT_INSTRUCTION_FAULT = 0x16,
 RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT = 0x17,
+RISCV_EXCP_SEMIHOST = 0x3f,
 } RISCVException;
 
 #define RISCV_EXCP_INT_FLAG0x8000
-- 
2.43.0

Re: [PATCH v3 22/49] i386/sev: Introduce 'sev-snp-guest' object

Michael Roth  writes:

> From: Brijesh Singh 
>
> SEV-SNP support relies on a different set of properties/state than the
> existing 'sev-guest' object. This patch introduces the 'sev-snp-guest'
> object, which can be used to configure an SEV-SNP guest. For example,
> a default-configured SEV-SNP guest with no additional information
> passed in for use with attestation:
>
>   -object sev-snp-guest,id=sev0
>
> or a fully-specified SEV-SNP guest where all spec-defined binary
> blobs are passed in as base64-encoded strings:
>
>   -object sev-snp-guest,id=sev0, \
> policy=0x3, \
> init-flags=0, \
> id-block=YWFhYWFhYWFhYWFhYWFhCg==, \
> id-auth=CxHK/OKLkXGn/KpAC7Wl1FSiisWDbGTEKz..., \
> auth-key-enabled=on, \
> host-data=LNkCWBRC5CcdGXirbNUV1OrsR28s..., \
> guest-visible-workarounds=AA==, \
>
> See the QAPI schema updates included in this patch for more usage
> details.
>
> In some cases these blobs may be up to 4096 characters, but this is
> generally well below the default limit for linux hosts where
> command-line sizes are defined by the sysconf-configurable ARG_MAX
> value, which defaults to 2097152 characters for Ubuntu hosts, for
> example.
>
> Signed-off-by: Brijesh Singh 
> Co-developed-by: Michael Roth 
> Acked-by: Markus Armbruster  (for QAPI schema)
> Signed-off-by: Michael Roth 

[...]

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 66b5781ca6..b25a3043da 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -920,6 +920,55 @@
>  '*handle': 'uint32',
>  '*kernel-hashes': 'bool' } }
>  
> +##
> +# @SevSnpGuestProperties:
> +#
> +# Properties for sev-snp-guest objects. Most of these are direct arguments
> +# for the KVM_SNP_* interfaces documented in the linux kernel source

"Linux", please.

> +# under Documentation/virt/kvm/amd-memory-encryption.rst, which are in

Does not seem to exist.  Do you mean
Documentation/arch/x86/amd-memory-encryption.rst?

> +# turn closely coupled with the SNP_INIT/SNP_LAUNCH_* firmware commands
> +# documented in the SEV-SNP Firmware ABI Specification (Rev 0.9).

docs/devel/qapi-code-gen.rst:

For legibility, wrap text paragraphs so every line is at most 70
characters long.

Separate sentences with two spaces.

> +#
> +# More usage information is also available in the QEMU source tree under
> +# docs/amd-memory-encryption.
> +#
> +# @policy: the 'POLICY' parameter to the SNP_LAUNCH_START command, as
> +#  defined in the SEV-SNP firmware ABI (default: 0x3)

docs/devel/qapi-code-gen.rst:

Descriptions start with '\@name:'.  The description text must be
indented like this::

 # @name: Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed
 # do eiusmod tempor incididunt ut labore et dolore magna aliqua.

> +#
> +# @guest-visible-workarounds: 16-byte, base64-encoded blob to report
> +# hypervisor-defined workarounds, corresponding
> +# to the 'GOSVW' parameter of the
> +# SNP_LAUNCH_START command defined in the
> +# SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @id-block: 96-byte, base64-encoded blob to provide the 'ID Block'
> +#structure for the SNP_LAUNCH_FINISH command defined in the
> +#SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @id-auth: 4096-byte, base64-encoded blob to provide the 'ID Authentication
> +#   Information Structure' for the SNP_LAUNCH_FINISH command defined
> +#   in the SEV-SNP firmware ABI (default: all-zero)
> +#
> +# @auth-key-enabled: true if 'id-auth' blob contains the 'AUTHOR_KEY' field
> +#defined SEV-SNP firmware ABI (default: false)
> +#
> +# @host-data: 32-byte, base64-encoded, user-defined blob to provide to the
> +# guest, as documented for the 'HOST_DATA' parameter of the
> +# SNP_LAUNCH_FINISH command in the SEV-SNP firmware ABI
> +# (default: all-zero)
> +#
> +# Since: 7.2

9.1

> +##

Together:

##
# @SevSnpGuestProperties:
#
# Properties for sev-snp-guest objects.  Most of these are direct
# arguments for the KVM_SNP_* interfaces documented in the Linux
# kernel source under
# Documentation/arch/x86/amd-memory-encryption.rst, which are in turn
# closely coupled with the SNP_INIT/SNP_LAUNCH_* firmware commands
# documented in the SEV-SNP Firmware ABI Specification (Rev 0.9).
#
# More usage information is also available in the QEMU source tree
# under docs/amd-memory-encryption.
#
# @policy: the 'POLICY' parameter to the SNP_LAUNCH_START command, as
# defined in the SEV-SNP firmware ABI (default: 0x3)
#
# @guest-visible-workarounds: 16-byte, base64-encoded blob to report
# hypervisor-defined workarounds, corresponding to the 'GOSVW'
# parameter of the SNP_LAUNCH_START command defined in the SEV-SNP
# firmware

Re: [PATCH v2] hw/audio/virtio-snd: Use device endianness instead of target one


On Mon, 22 Apr 2024 16:13, Philippe Mathieu-Daudé  wrote:

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.


Hey Philippe, can you clarify what do you mean by they can change 
endianness at runtime?


The target's one is used because that's the one it will be using to do 
I/O with its kernel's audio interface.








Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
hw/audio/virtio-snd.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..82c5647660 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -395,13 +395,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
 * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
 * params.
 */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
virtio_snd_pcm_set_params *params)
{
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
as->fmt = virtio_snd_get_qemu_format(params->format);
as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
}

/*
@@ -464,7 +466,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
s->pcm->streams[stream_id] = stream;
}

-virtio_snd_get_qemu_audsettings(, params);
+virtio_snd_get_qemu_audsettings(s, , params);
stream->info.direction = stream_id < s->snd_conf.streams / 2 +
(s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
--
2.41.0

[PATCH] target/riscv/kvm: Fix exposure of Zkr

2024-04-22 Thread Andrew Jones

The Zkr extension may only be exposed to KVM guests if the VMM
implements the SEED CSR. Use the same implementation as TCG.

Without this patch, running with a KVM which does not forward the
SEED CSR access to QEMU will result in an ILL exception being
injected into the guest (this results in Linux guests crashing on
boot). And, when running with a KVM which does forward the access,
QEMU will crash, since QEMU doesn't know what to do with the exit.

Fixes: 3108e2f1c69d ("target/riscv/kvm: update KVM exts to Linux 6.8")
Signed-off-by: Andrew Jones 
---
 target/riscv/cpu.h |  3 +++
 target/riscv/csr.c | 18 ++
 target/riscv/kvm/kvm-cpu.c | 25 +
 3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 3b1a02b9449a..52fb8c15d08f 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -821,6 +821,9 @@ void riscv_set_csr_ops(int csrno, riscv_csr_operations 
*ops);
 
 void riscv_cpu_register_gdb_regs_for_features(CPUState *cs);
 
+target_ulong riscv_new_csr_seed(target_ulong new_value,
+target_ulong write_mask);
+
 uint8_t satp_mode_max_from_map(uint32_t map);
 const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit);
 
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 726096444fae..829d8346ed4e 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -4267,10 +4267,8 @@ static RISCVException write_upmbase(CPURISCVState *env, 
int csrno,
 #endif
 
 /* Crypto Extension */
-static RISCVException rmw_seed(CPURISCVState *env, int csrno,
-   target_ulong *ret_value,
-   target_ulong new_value,
-   target_ulong write_mask)
+target_ulong riscv_new_csr_seed(target_ulong new_value,
+target_ulong write_mask)
 {
 uint16_t random_v;
 Error *random_e = NULL;
@@ -4294,6 +4292,18 @@ static RISCVException rmw_seed(CPURISCVState *env, int 
csrno,
 rval = random_v | SEED_OPST_ES16;
 }
 
+return rval;
+}
+
+static RISCVException rmw_seed(CPURISCVState *env, int csrno,
+   target_ulong *ret_value,
+   target_ulong new_value,
+   target_ulong write_mask)
+{
+target_ulong rval;
+
+rval = riscv_new_csr_seed(new_value, write_mask);
+
 if (ret_value) {
 *ret_value = rval;
 }
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80f1..50bdbd24a878 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1418,6 +1418,28 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct 
kvm_run *run)
 return ret;
 }
 
+static int kvm_riscv_handle_csr(CPUState *cs, struct kvm_run *run)
+{
+target_ulong csr_num = run->riscv_csr.csr_num;
+target_ulong new_value = run->riscv_csr.new_value;
+target_ulong write_mask = run->riscv_csr.write_mask;
+int ret = 0;
+
+switch (csr_num) {
+case CSR_SEED:
+run->riscv_csr.ret_value = riscv_new_csr_seed(new_value, write_mask);
+break;
+default:
+qemu_log_mask(LOG_UNIMP,
+  "%s: un-handled CSR EXIT for CSR %lx\n",
+  __func__, csr_num);
+ret = -1;
+break;
+}
+
+return ret;
+}
+
 int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
 {
 int ret = 0;
@@ -1425,6 +1447,9 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
*run)
 case KVM_EXIT_RISCV_SBI:
 ret = kvm_riscv_handle_sbi(cs, run);
 break;
+case KVM_EXIT_RISCV_CSR:
+ret = kvm_riscv_handle_csr(cs, run);
+break;
 default:
 qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
   __func__, run->exit_reason);
-- 
2.44.0

Re: [PATCH] target/riscv/kvm: tolerate KVM disable ext errors

2024-04-22 Thread Andrew Jones

On Mon, Apr 22, 2024 at 10:12:53AM -0300, Daniel Henrique Barboza wrote:
> Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
> enabled, will fail with a kernel oops SIGILL right at the start. The
> reason is that we can't expose zkr without implementing the SEED CSR.
> Disabling zkr in the guest would be a workaround, but if the KVM doesn't
> allow it we'll error out and never boot.
> 
> In hindsight this is too strict. If we keep proceeding, despite not
> disabling the extension in the KVM vcpu, we'll not add extension in
^ the
> riscv,isa. The guest kernel will be unaware of the extension, i.e. it
 ^ the

> doesn't matter if the KVM vcpu has it enabled underneath or not. So it's
> ok to keep booting in this case.
> 
> Change our current logic to not error out if we fail to disable an
> extension in kvm_set_one_reg(): throw an warning instead and keep
> booting.  We'll keep the existing behavior when we fail to enable an
> extension in KVM, since adding the extension in riscv,isa at this point
> will cause a guest malfunction because the extension isn't enabled in
> the vcpu.

I'd probably add a sentence or two explaining why we still want to warn,
which is because KVM is stating that while you may think you're disabling
an extension, that extension's behavior (instructions, etc.) will still
be exposed to the guest since there's no way not to expose it.  The user
should be aware that a guest could use it anyway, since that means the
attempt to disable it can't be relied upon for migration compatibility of
arbitrary guests. The guest needs to be well behaved, i.e. one that won't
try to use any extensions which aren't in the ISA string. So, disabling
these types of extensions either shouldn't generally be done (so a noisy
warning helps prohibit that) or done for debug purposes (where a noisy
warning is fine).

> 
> Suggested-by: Andrew Jones 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/riscv/kvm/kvm-cpu.c | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 6a6c6cae80..261ca24504 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU 
> *cpu, CPUState *cs)
>  reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
>  ret = kvm_set_one_reg(cs, id, );
>  if (ret != 0) {
> -error_report("Unable to %s extension %s in KVM, error %d",
> - reg ? "enable" : "disable",
> - multi_ext_cfg->name, ret);
> -exit(EXIT_FAILURE);
> +if (reg) {
> +error_report("Unable to enable extension %s in KVM, error 
> %d",
> + multi_ext_cfg->name, ret);
> +exit(EXIT_FAILURE);
> +} else {

Maybe we should check for a specific error. Is it EINVAL? If we do, then
the message below would drop the (error %d) part and instead state
explicitly that it cannot disable this extension since it's not possible.

> +warn_report("KVM did not disable extension %s (error %d)",

s/did not/cannot/

> +multi_ext_cfg->name, ret);
> +}
>  }
>  }
>  }
> -- 
> 2.44.0
>

Thanks,
drew

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

2024-04-22 Thread Marcin Juszkiewicz


W dniu 22.04.2024 o 14:56, Peter Maydell pisze:

On Fri, 19 Apr 2024 at 19:46, Peter Maydell  wrote:



The upshot is that the only CPU type that changes is 'max'; but any
new type we add in future (whether v8.6 or not) will also get the new
1GHz default (assuming we spot in code review any attempts to set
the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
of cut-n-paste from an older CPU initfn ;-)).

It remains the case that the machine model can override the default
value via the 'cntfrq' QOM property (regardless of the CPU type).


This patchset turns out to break the sbsa-ref board: the
Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
avocado test both (a) takes rather longer to boot and (b) when
running thinks that time is advancing very fast.

I suspect this may be because the firmware hard-codes the
generic timer clock frequency it is expecting. I've cc'd the
sbsa-ref maintainers: is that correct?

If so, we can deal with it by making the sbsa-ref board set the
cntfrq QOM property on its CPUs to force them to the old
frequency. However this will produce a technically-out-of-spec
CPU when used with a v8.6-compliant CPU type, so probably we
should do something to be able to tell the firmware the clock
frequency (if it doesn't want to just read CNTFRQ_EL0 itself).


From what I see in EDK2 code we read CNTFREQ_EL0:

GetPlatformTimerFreq() checks for PcdArmArchTimerFreqInHz variable which
sbsa-ref has set to 0. So it calls ArmGenericTimerGetTimerFreq() ->
ArmReadCntFrq() -> "mrs x0, cntfrq_el0"

I added debug output to firmware and it shows me:

HRW: GetPlatformTimerFreq TimerFreq = 6250

Local tree:
ed0604e99c (HEAD -> test-counter) target/arm: Default to 1GHz cntfrq for 'max' 
and new CPUs
c0a8c341f5 target/arm: Refactor default generic timer frequency handling
592c01312b hw: Add compat machines for 9.1
62dbe54c24 (tag: v9.0.0-rc4, origin/master, origin/HEAD) Update version for 
v9.0.0-rc4 release
a12214d1c4 (origin/staging) usb-storage: Fix BlockConf defaults

sbsa-ref with "-cpu max" used. All cpu cores give me same value.

Re: [PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()


On 22/4/24 14:52, Manos Pitsidianakis wrote:

Extract audio card removal logic out of the device unrealize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
  hw/audio/virtio-snd.c | 20 ++--
  1 file changed, 14 insertions(+), 6 deletions(-)




-static void virtio_snd_unrealize(DeviceState *dev)
+/* Remove audio card and cleanup streams. */
+static void virtio_snd_unsetup(VirtIOSound *vsnd)


Maybe s/unsetup/cleanup/?


  {
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-VirtIOSound *vsnd = VIRTIO_SND(dev);
  VirtIOSoundPCMStream *stream;
  
-qemu_del_vm_change_state_handler(vsnd->vmstate);

-trace_virtio_snd_unrealize(vsnd);
-
  if (vsnd->pcm) {
  if (vsnd->pcm->streams) {
  for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
@@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
  vsnd->pcm = NULL;
  }
  AUD_remove_card(>card);
+}

Re: [PATCH v1 2/4] virtio-snd: factor card setup out of realize func


On 22/4/24 14:52, Manos Pitsidianakis wrote:

Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
  hw/audio/virtio-snd.c | 72 ---
  1 file changed, 41 insertions(+), 31 deletions(-)




+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+ERRP_GUARD();
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+trace_virtio_snd_realize(vsnd);
+
+vsnd->vmstate =
+qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+virtio_add_feature(>features, VIRTIO_F_VERSION_1);
+
  vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
  virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
  vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
  QTAILQ_INIT(>cmdq);
  QSIMPLEQ_INIT(>invalid);
  
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {

-status = virtio_snd_set_pcm_params(vsnd, i, _params);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't initialize stream params, device responded with 
%s.",
-   print_code(status));
-goto error_cleanup;
-}
-status = virtio_snd_pcm_prepare(vsnd, i);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't prepare streams, device responded with %s.",
-   print_code(status));
-goto error_cleanup;
-}
+if (virtio_snd_setup(vsnd, errp)) {
+return;
  }
  
-return;

-
-error_cleanup:
  virtio_snd_unrealize(dev);


We usually handle failure as:

  if (!virtio_snd_setup(vsnd, errp)) {
virtio_snd_unrealize(dev);
  }


  }
  


Otherwise LGTM.

Re: [PATCH v1 1/4] virtio-snd: add virtio_snd_is_config_valid()


Hi Manos,

On 22/4/24 14:52, Manos Pitsidianakis wrote:

Factor out virtio_snd_config value validation in a separate function, in
order to re-use it in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
  hw/audio/virtio-snd.c | 47 ++-
  1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..7ca9ed251c 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1045,6 +1045,34 @@ virtio_snd_vm_state_change(void *opaque, bool running,
  }
  }
  
+static bool

+virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
+{
+if (snd_conf.jacks > 8) {
+error_setg(errp,
+   "Invalid number of jacks: %"PRIu32
+   ": maximum value is 8", snd_conf.jacks);
+return false;
+}
+if (snd_conf.streams < 1 || snd_conf.streams > 64) {


Why the undocumented 10 -> 64 change?


+error_setg(errp,
+   "Invalid number of streams: %"PRIu32
+   ": minimum value is 1, maximum value is 64",
+   snd_conf.streams);
+return false;
+}
+
+if (snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
+error_setg(errp,
+  "Invalid number of channel maps: %"PRIu32
+  ": VIRTIO v1.2 sets the maximum value at %"PRIu32,
+  snd_conf.chmaps, VIRTIO_SND_CHMAP_MAX_SIZE);
+return false;
+}
+
+return true;
+}
+
  static void virtio_snd_realize(DeviceState *dev, Error **errp)
  {
  ERRP_GUARD();
@@ -1055,24 +1083,7 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
  
  trace_virtio_snd_realize(vsnd);
  
-/* check number of jacks and streams */

-if (vsnd->snd_conf.jacks > 8) {
-error_setg(errp,
-   "Invalid number of jacks: %"PRIu32,
-   vsnd->snd_conf.jacks);
-return;
-}
-if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
-error_setg(errp,
-   "Invalid number of streams: %"PRIu32,
-vsnd->snd_conf.streams);
-return;
-}
-
-if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
-error_setg(errp,
-   "Invalid number of channel maps: %"PRIu32,
-   vsnd->snd_conf.chmaps);
+if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
  return;
  }

[PATCH v2] hw/audio/virtio-snd: Use device endianness instead of target one

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/audio/virtio-snd.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..82c5647660 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -395,13 +395,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
  * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
  * params.
  */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
 virtio_snd_pcm_set_params *params)
 {
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
 as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
 as->fmt = virtio_snd_get_qemu_format(params->format);
 as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
 }
 
 /*
@@ -464,7 +466,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 s->pcm->streams[stream_id] = stream;
 }
 
-virtio_snd_get_qemu_audsettings(, params);
+virtio_snd_get_qemu_audsettings(s, , params);
 stream->info.direction = stream_id < s->snd_conf.streams / 2 +
 (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
 stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
-- 
2.41.0

[PATCH] target/riscv/kvm: tolerate KVM disable ext errors

Running a KVM guest using a 6.9-rc3 kernel, in a 6.8 host that has zkr
enabled, will fail with a kernel oops SIGILL right at the start. The
reason is that we can't expose zkr without implementing the SEED CSR.
Disabling zkr in the guest would be a workaround, but if the KVM doesn't
allow it we'll error out and never boot.

In hindsight this is too strict. If we keep proceeding, despite not
disabling the extension in the KVM vcpu, we'll not add extension in
riscv,isa. The guest kernel will be unaware of the extension, i.e. it
doesn't matter if the KVM vcpu has it enabled underneath or not. So it's
ok to keep booting in this case.

Change our current logic to not error out if we fail to disable an
extension in kvm_set_one_reg(): throw an warning instead and keep
booting.  We'll keep the existing behavior when we fail to enable an
extension in KVM, since adding the extension in riscv,isa at this point
will cause a guest malfunction because the extension isn't enabled in
the vcpu.

Suggested-by: Andrew Jones 
Signed-off-by: Daniel Henrique Barboza 
---
 target/riscv/kvm/kvm-cpu.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 6a6c6cae80..261ca24504 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -427,10 +427,14 @@ static void kvm_riscv_update_cpu_cfg_isa_ext(RISCVCPU 
*cpu, CPUState *cs)
 reg = kvm_cpu_cfg_get(cpu, multi_ext_cfg);
 ret = kvm_set_one_reg(cs, id, );
 if (ret != 0) {
-error_report("Unable to %s extension %s in KVM, error %d",
- reg ? "enable" : "disable",
- multi_ext_cfg->name, ret);
-exit(EXIT_FAILURE);
+if (reg) {
+error_report("Unable to enable extension %s in KVM, error %d",
+ multi_ext_cfg->name, ret);
+exit(EXIT_FAILURE);
+} else {
+warn_report("KVM did not disable extension %s (error %d)",
+multi_ext_cfg->name, ret);
+}
 }
 }
 }
-- 
2.44.0

Re: [PATCH] hw/audio/virtio-snd: Use device endianness instead of target one


On 22/4/24 15:04, Philippe Mathieu-Daudé wrote:

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/audio/virtio-snd.c | 8 +---
  1 file changed, 5 insertions(+), 3 deletions(-)




-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
  virtio_snd_pcm_set_params *params)
  {
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
  as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
  as->fmt = virtio_snd_get_qemu_format(params->format);
  as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = vdev->device_endian ? 1 : 0;


Err, I neglected to consider VIRTIO_DEVICE_ENDIAN_UNKNOWN :/


  }

Re: [PATCH v3 21/49] i386/sev: Introduce "sev-common" type to encapsulate common SEV state

Michael Roth  writes:

> Currently all SEV/SEV-ES functionality is managed through a single
> 'sev-guest' QOM type. With upcoming support for SEV-SNP, taking this
> same approach won't work well since some of the properties/state
> managed by 'sev-guest' is not applicable to SEV-SNP, which will instead
> rely on a new QOM type with its own set of properties/state.
>
> To prepare for this, this patch moves common state into an abstract
> 'sev-common' parent type to encapsulate properties/state that are
> common to both SEV/SEV-ES and SEV-SNP, leaving only SEV/SEV-ES-specific
> properties/state in the current 'sev-guest' type. This should not
> affect current behavior or command-line options.

QAPI schema refactoring except for the misleading "since" documentation
pointed out by Daniel
Acked-by: Markus Armbruster 

[...]

[PATCH] hw/audio/virtio-snd: Use device endianness instead of target one

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-sta...@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/audio/virtio-snd.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..796e0753d6 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -395,13 +395,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
  * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
  * params.
  */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
 virtio_snd_pcm_set_params *params)
 {
+VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
 as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
 as->fmt = virtio_snd_get_qemu_format(params->format);
 as->freq = virtio_snd_get_qemu_freq(params->rate);
-as->endianness = target_words_bigendian() ? 1 : 0;
+as->endianness = vdev->device_endian ? 1 : 0;
 }
 
 /*
@@ -464,7 +466,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
uint32_t stream_id)
 s->pcm->streams[stream_id] = stream;
 }
 
-virtio_snd_get_qemu_audsettings(, params);
+virtio_snd_get_qemu_audsettings(s, , params);
 stream->info.direction = stream_id < s->snd_conf.streams / 2 +
 (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
 stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
-- 
2.41.0

Re: [RFC PATCH 1/3] target/riscv: change RISCV_EXCP_SEMIHOST exception number

2024-04-22 Thread Clément Léger




On 22/04/2024 05:25, Alistair Francis wrote:
> On Thu, Apr 18, 2024 at 11:40 PM Clément Léger  wrote:
>>
>> The double trap specification defines the double trap exception number
>> to be 16 which is actually used by the internal semihosting one. Change
>> it to some other value.
>>
>> Signed-off-by: Clément Léger 
> 
> Reviewed-by: Alistair Francis 

Hi Alistair,

Ved actually told me that even 17 is reserved so I'll move the semihost
one to 63, which is in a range designated for custom use.

Regards,

Clément

> 
> Alistair
> 
>> ---
>>  target/riscv/cpu_bits.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index fc2068ee4d..9ade72ff31 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -670,7 +670,7 @@ typedef enum RISCVException {
>>  RISCV_EXCP_INST_PAGE_FAULT = 0xc, /* since: priv-1.10.0 */
>>  RISCV_EXCP_LOAD_PAGE_FAULT = 0xd, /* since: priv-1.10.0 */
>>  RISCV_EXCP_STORE_PAGE_FAULT = 0xf, /* since: priv-1.10.0 */
>> -RISCV_EXCP_SEMIHOST = 0x10,
>> +RISCV_EXCP_SEMIHOST = 0x11,
>>  RISCV_EXCP_INST_GUEST_PAGE_FAULT = 0x14,
>>  RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT = 0x15,
>>  RISCV_EXCP_VIRT_INSTRUCTION_FAULT = 0x16,
>> --
>> 2.43.0
>>
>>

[PATCH] hw/arm/npcm7xx: Store derivative OTP fuse key in little endian

Use little endian for derivative OTP fuse key.

Cc: qemu-sta...@nongnu.org
Fixes: c752bb079b ("hw/nvram: NPCM7xx OTP device model")
Suggested-by: Avi Fishman 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/arm/npcm7xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index cc68b5d8f1..9f2d96c733 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -24,6 +24,7 @@
 #include "hw/qdev-clock.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
+#include "qemu/bswap.h"
 #include "qemu/units.h"
 #include "sysemu/sysemu.h"
 #include "target/arm/cpu-qom.h"
@@ -386,7 +387,7 @@ static void npcm7xx_init_fuses(NPCM7xxState *s)
  * The initial mask of disabled modules indicates the chip derivative (e.g.
  * NPCM750 or NPCM730).
  */
-value = tswap32(nc->disabled_modules);
+value = cpu_to_le32(nc->disabled_modules);
 npcm7xx_otp_array_write(>fuse_array, , NPCM7XX_FUSE_DERIVATIVE,
 sizeof(value));
 }
-- 
2.41.0

Re: [PATCH 0/3] target/arm: Make the counter frequency default 1GHz for new CPUs, machines

On Fri, 19 Apr 2024 at 19:46, Peter Maydell  wrote:
>
> In previous versions of the Arm architecture, the frequency of the
> generic timers as reported in CNTFRQ_EL0 could be any IMPDEF value,
> and for QEMU we picked 62.5MHz, giving a timer tick period of 16ns.
> In Armv8.6, the architecture standardized this frequency to 1GHz.
>
> Because there is no ID register feature field that indicates whether a
> CPU is v8.6 or that it ought to have this counter frequency, we
> implement this by changing our default CNTFRQ value for all CPUs, with
> exceptions for backwards compatibility:
>
>  * CPU types which we already implement will retain the old
>default value. None of these are v8.6 CPUs, so this is
>architecturally OK.
>  * CPUs used in versioned machine types with a version of 9.0
>or earlier will retain the old default value.
>
> The upshot is that the only CPU type that changes is 'max'; but any
> new type we add in future (whether v8.6 or not) will also get the new
> 1GHz default (assuming we spot in code review any attempts to set
> the ARM_FEATURE_BACKCOMPAT_CNTFRQ flag on new CPU types as a result
> of cut-n-paste from an older CPU initfn ;-)).
>
> It remains the case that the machine model can override the default
> value via the 'cntfrq' QOM property (regardless of the CPU type).

This patchset turns out to break the sbsa-ref board: the
Aarch64SbsarefMachine.test_sbsaref_alpine_linux_max_pauth_impdef
avocado test both (a) takes rather longer to boot and (b) when
running thinks that time is advancing very fast.

I suspect this may be because the firmware hard-codes the
generic timer clock frequency it is expecting. I've cc'd the
sbsa-ref maintainers: is that correct?

If so, we can deal with it by making the sbsa-ref board set the
cntfrq QOM property on its CPUs to force them to the old
frequency. However this will produce a technically-out-of-spec
CPU when used with a v8.6-compliant CPU type, so probably we
should do something to be able to tell the firmware the clock
frequency (if it doesn't want to just read CNTFRQ_EL0 itself).

thanks
-- PMM

[PATCH v1 3/4] virtio-snd: factor card removal out of unrealize()

Extract audio card removal logic out of the device unrealize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
 hw/audio/virtio-snd.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 82dd320ebe..a9cfaea046 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1343,15 +1343,11 @@ static inline void 
virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream)
 }
 }
 
-static void virtio_snd_unrealize(DeviceState *dev)
+/* Remove audio card and cleanup streams. */
+static void virtio_snd_unsetup(VirtIOSound *vsnd)
 {
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-VirtIOSound *vsnd = VIRTIO_SND(dev);
 VirtIOSoundPCMStream *stream;
 
-qemu_del_vm_change_state_handler(vsnd->vmstate);
-trace_virtio_snd_unrealize(vsnd);
-
 if (vsnd->pcm) {
 if (vsnd->pcm->streams) {
 for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
@@ -1370,6 +1366,18 @@ static void virtio_snd_unrealize(DeviceState *dev)
 vsnd->pcm = NULL;
 }
 AUD_remove_card(>card);
+}
+
+static void virtio_snd_unrealize(DeviceState *dev)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+
+qemu_del_vm_change_state_handler(vsnd->vmstate);
+trace_virtio_snd_unrealize(vsnd);
+
+virtio_snd_unsetup(vsnd);
+
 qemu_mutex_destroy(>cmdq_mutex);
 virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
 virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_EVENT]);
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 2/4] virtio-snd: factor card setup out of realize func

Extract audio card setup logic out of the device realize callback so
that it can be re-used in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
 hw/audio/virtio-snd.c | 72 ---
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index 7ca9ed251c..82dd320ebe 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1073,27 +1073,21 @@ virtio_snd_is_config_valid(virtio_snd_config snd_conf, 
Error **errp)
 return true;
 }
 
-static void virtio_snd_realize(DeviceState *dev, Error **errp)
+/* Registers card and sets up streams according to configuration. */
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp)
 {
 ERRP_GUARD();
-VirtIOSound *vsnd = VIRTIO_SND(dev);
-VirtIODevice *vdev = VIRTIO_DEVICE(dev);
 virtio_snd_pcm_set_params default_params = { 0 };
 uint32_t status;
 
-trace_virtio_snd_realize(vsnd);
-
 if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
-return;
+return false;
 }
 
 if (!AUD_register_card("virtio-sound", >card, errp)) {
-return;
+return false;
 }
 
-vsnd->vmstate =
-qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
-
 vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
 vsnd->pcm->snd = vsnd;
 vsnd->pcm->streams =
@@ -1101,9 +1095,6 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 vsnd->pcm->pcm_params =
 g_new0(virtio_snd_pcm_set_params, vsnd->snd_conf.streams);
 
-virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
-virtio_add_feature(>features, VIRTIO_F_VERSION_1);
-
 /* set default params for all streams */
 default_params.features = 0;
 default_params.buffer_bytes = cpu_to_le32(8192);
@@ -,6 +1102,41 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 default_params.channels = 2;
 default_params.format = VIRTIO_SND_PCM_FMT_S16;
 default_params.rate = VIRTIO_SND_PCM_RATE_48000;
+
+for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
+status = virtio_snd_set_pcm_params(vsnd, i, _params);
+if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+error_setg(errp,
+   "Can't initialize stream params, device responded with 
%s.",
+   print_code(status));
+return false;
+}
+status = virtio_snd_pcm_prepare(vsnd, i);
+if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
+error_setg(errp,
+   "Can't prepare streams, device responded with %s.",
+   print_code(status));
+return false;
+}
+}
+
+return true;
+}
+
+static void virtio_snd_realize(DeviceState *dev, Error **errp)
+{
+ERRP_GUARD();
+VirtIOSound *vsnd = VIRTIO_SND(dev);
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+trace_virtio_snd_realize(vsnd);
+
+vsnd->vmstate =
+qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
+
+virtio_init(vdev, VIRTIO_ID_SOUND, sizeof(virtio_snd_config));
+virtio_add_feature(>features, VIRTIO_F_VERSION_1);
+
 vsnd->queues[VIRTIO_SND_VQ_CONTROL] =
 virtio_add_queue(vdev, 64, virtio_snd_handle_ctrl);
 vsnd->queues[VIRTIO_SND_VQ_EVENT] =
@@ -1123,26 +1149,10 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 QTAILQ_INIT(>cmdq);
 QSIMPLEQ_INIT(>invalid);
 
-for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
-status = virtio_snd_set_pcm_params(vsnd, i, _params);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't initialize stream params, device responded with 
%s.",
-   print_code(status));
-goto error_cleanup;
-}
-status = virtio_snd_pcm_prepare(vsnd, i);
-if (status != cpu_to_le32(VIRTIO_SND_S_OK)) {
-error_setg(errp,
-   "Can't prepare streams, device responded with %s.",
-   print_code(status));
-goto error_cleanup;
-}
+if (virtio_snd_setup(vsnd, errp)) {
+return;
 }
 
-return;
-
-error_cleanup:
 virtio_snd_unrealize(dev);
 }
 
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 4/4] virtio_snd_set_config: validate and re-setup card

Validate new configuration values and re-setup audio card.

Changing the number of streams via virtio_snd_set_config() did not
re-configure the audio card, leaving it in an invalid state. This can be
demonstrated by this heap buffer overflow:

```shell
cat << EOF | qemu-system-x86_64 -display none \
-machine accel=qtest -m 512M -machine q35 -device \
virtio-sound,audiodev=my_audiodev,streams=2 -audiodev \
alsa,id=my_audiodev -qtest stdio
outl 0xcf8 0x80001804
outw 0xcfc 0x06
outl 0xcf8 0x80001820
outl 0xcfc 0xe0008000
write 0xe0008016 0x1 0x03
write 0xe0008020 0x4 0x00901000
write 0xe0008028 0x4 0x00a01000
write 0xe000801c 0x1 0x01
write 0xe000a004 0x1 0x40
write 0x10c000 0x1 0x02
write 0x109001 0x1 0xc0
write 0x109002 0x1 0x10
write 0x109008 0x1 0x04
write 0x10a002 0x1 0x01
write 0xe000b00d 0x1 0x00
EOF
```

When built with `--enable-sanitizers`, QEMU prints this error:

  ERROR: AddressSanitizer: heap-buffer-overflow [..snip..] in
  virtio_snd_handle_rx_xfer().

Closes #2296.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2296
Reported-by: Zheyu Ma 
Suggested-by: Zheyu Ma 
Signed-off-by: Manos Pitsidianakis 
---
 hw/audio/virtio-snd.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index a9cfaea046..d47af2ed69 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -36,7 +36,11 @@ static void virtio_snd_pcm_out_cb(void *data, int available);
 static void virtio_snd_process_cmdq(VirtIOSound *s);
 static void virtio_snd_pcm_flush(VirtIOSoundPCMStream *stream);
 static void virtio_snd_pcm_in_cb(void *data, int available);
+static bool virtio_snd_setup(VirtIOSound *vsnd, Error **errp);
+static void virtio_snd_unsetup(VirtIOSound *vsnd);
 static void virtio_snd_unrealize(DeviceState *dev);
+static bool virtio_snd_is_config_valid(virtio_snd_config snd_conf,
+   Error **errp);
 
 static uint32_t supported_formats = BIT(VIRTIO_SND_PCM_FMT_S8)
   | BIT(VIRTIO_SND_PCM_FMT_U8)
@@ -111,23 +115,26 @@ static void
 virtio_snd_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
 VirtIOSound *s = VIRTIO_SND(vdev);
-const virtio_snd_config *sndconfig =
-(const virtio_snd_config *)config;
+virtio_snd_config new_value =
+*(const virtio_snd_config *)config;
 
+le32_to_cpus(_value.jacks);
+le32_to_cpus(_value.streams);
+le32_to_cpus(_value.chmaps);
 
-   trace_virtio_snd_set_config(vdev,
-   s->snd_conf.jacks,
-   sndconfig->jacks,
-   s->snd_conf.streams,
-   sndconfig->streams,
-   s->snd_conf.chmaps,
-   sndconfig->chmaps);
-
-memcpy(>snd_conf, sndconfig, sizeof(virtio_snd_config));
-le32_to_cpus(>snd_conf.jacks);
-le32_to_cpus(>snd_conf.streams);
-le32_to_cpus(>snd_conf.chmaps);
+trace_virtio_snd_set_config(vdev,
+s->snd_conf.jacks,
+new_value.jacks,
+s->snd_conf.streams,
+new_value.streams,
+s->snd_conf.chmaps,
+new_value.chmaps);
 
+if (virtio_snd_is_config_valid(new_value, _warn)) {
+virtio_snd_unsetup(s);
+s->snd_conf = new_value;
+virtio_snd_setup(s, _fatal);
+}
 }
 
 static void
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 1/4] virtio-snd: add virtio_snd_is_config_valid()

Factor out virtio_snd_config value validation in a separate function, in
order to re-use it in follow up commits.

Signed-off-by: Manos Pitsidianakis 
---
 hw/audio/virtio-snd.c | 47 ++-
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..7ca9ed251c 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -1045,6 +1045,34 @@ virtio_snd_vm_state_change(void *opaque, bool running,
 }
 }
 
+static bool
+virtio_snd_is_config_valid(virtio_snd_config snd_conf, Error **errp)
+{
+if (snd_conf.jacks > 8) {
+error_setg(errp,
+   "Invalid number of jacks: %"PRIu32
+   ": maximum value is 8", snd_conf.jacks);
+return false;
+}
+if (snd_conf.streams < 1 || snd_conf.streams > 64) {
+error_setg(errp,
+   "Invalid number of streams: %"PRIu32
+   ": minimum value is 1, maximum value is 64",
+   snd_conf.streams);
+return false;
+}
+
+if (snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
+error_setg(errp,
+  "Invalid number of channel maps: %"PRIu32
+  ": VIRTIO v1.2 sets the maximum value at %"PRIu32,
+  snd_conf.chmaps, VIRTIO_SND_CHMAP_MAX_SIZE);
+return false;
+}
+
+return true;
+}
+
 static void virtio_snd_realize(DeviceState *dev, Error **errp)
 {
 ERRP_GUARD();
@@ -1055,24 +1083,7 @@ static void virtio_snd_realize(DeviceState *dev, Error 
**errp)
 
 trace_virtio_snd_realize(vsnd);
 
-/* check number of jacks and streams */
-if (vsnd->snd_conf.jacks > 8) {
-error_setg(errp,
-   "Invalid number of jacks: %"PRIu32,
-   vsnd->snd_conf.jacks);
-return;
-}
-if (vsnd->snd_conf.streams < 1 || vsnd->snd_conf.streams > 10) {
-error_setg(errp,
-   "Invalid number of streams: %"PRIu32,
-vsnd->snd_conf.streams);
-return;
-}
-
-if (vsnd->snd_conf.chmaps > VIRTIO_SND_CHMAP_MAX_SIZE) {
-error_setg(errp,
-   "Invalid number of channel maps: %"PRIu32,
-   vsnd->snd_conf.chmaps);
+if (!virtio_snd_is_config_valid(vsnd->snd_conf, errp)) {
 return;
 }
 
-- 
γαῖα πυρί μιχθήτω

[PATCH v1 0/4] virtio_snd_set_config: Fix #2296

Changing the number of streams via virtio_snd_set_config() did not
re-configure the audio card, leaving it in an invalid state.

Reported in https://gitlab.com/qemu-project/qemu/-/issues/2296

Manos Pitsidianakis (4):
  virtio-snd: add virtio_snd_is_config_valid()
  virtio-snd: factor card setup out of realize func
  virtio-snd: factor card removal out of unrealize()
  virtio_snd_set_config: validate and re-setup card

 hw/audio/virtio-snd.c | 174 +-
 1 file changed, 105 insertions(+), 69 deletions(-)


base-commit: 62dbe54c24dbf77051bafe1039c31ddc8f37602d
-- 
γαῖα πυρί μιχθήτω

Re: [PATCH] backends/cryptodev-builtin: Fix local_error leaks

2024-04-22 Thread 皮振伟

Hi, Please add the following message: Fixes: 2fda101de07("virtio-crypto:
Support asynchronous mode") LGTM. Reviewed-by: zhenwei pi <
pizhen...@bytedance.com> > From:"Li Zhijian" >
Date:Mon, Apr 22, 2024, 16:50 > Subject:[External] [PATCH]
backends/cryptodev-builtin: Fix local_error leaks > To:<
arei.gong...@huawei.com>, > Cc:,,"Li Zhijian" > It seems
that this error does not need to be propagated to the upper, > directly
output the error to avoid the leaks >  > Closes:
https://gitlab.com/qemu-project/qemu/-/issues/2283 > Signed-off-by: Li
Zhijian  > --- >  backends/cryptodev-builtin.c | 9
+ >  1 file changed, 5 insertions(+), 4 deletions(-) >  > diff
--git a/backends/cryptodev-builtin.c b/backends/cryptodev-builtin.c > index
a514bbb310..940104ee55 100644 > --- a/backends/cryptodev-builtin.c > +++
b/backends/cryptodev-builtin.c > @@ -23,6 +23,7 @@ >   >  #include
"qemu/osdep.h" >  #include "sysemu/cryptodev.h" > +#include
"qemu/error-report.h" >  #include "qapi/error.h" >  #include
"standard-headers/linux/virtio_crypto.h" >  #include "crypto/cipher.h" > @@
-396,8 +397,8 @@ static int cryptodev_builtin_create_session( >  case
VIRTIO_CRYPTO_HASH_CREATE_SESSION: >  case
VIRTIO_CRYPTO_MAC_CREATE_SESSION: >  default: > -
error_setg(_error, "Unsupported opcode :%" PRIu32 "", > -
  sess_info->op_code); > +error_report("Unsupported opcode :%"
PRIu32 "", > + sess_info->op_code); >  return
-VIRTIO_CRYPTO_NOTSUPP; >  } >   > @@ -554,8 +555,8 @@ static int
cryptodev_builtin_operation( >   >  if (op_info->session_id >=
MAX_NUM_SESSIONS || >builtin->sessions[op_info->session_id]
== NULL) { > -error_setg(_error, "Cannot find a valid session
id: %" PRIu64 "", > -   op_info->session_id); > +
error_report("Cannot find a valid session id: %" PRIu64 "", > +
op_info->session_id); >  return -VIRTIO_CRYPTO_INVSESS; >
   } >   > --  > 2.31.1

Re: [RFC PATCH-for-8.0 3/3] hw/net/xilinx_ethlite: Replace tswap32() by be32_to_cpu()


On 13/12/22 14:53, Peter Maydell wrote:

On Tue, 13 Dec 2022 at 12:52, Philippe Mathieu-Daudé  wrote:


This partly revert commit d48751ed4f ("xilinx-ethlite:
Simplify byteswapping to/from brams") which states the
packet data is stored in big-endian.

Signed-off-by: Philippe Mathieu-Daudé 



@@ -102,8 +102,8 @@ eth_read(void *opaque, hwaddr addr, unsigned int size)
  D(qemu_log("%s " TARGET_FMT_plx "=%x\n", __func__, addr * 4, r));
  break;

-default:
-r = tswap32(s->regs[addr]);
+default: /* Packet data */
+r = be32_to_cpu(s->regs[addr]);
  break;
  }
  return r;
@@ -160,8 +160,8 @@ eth_write(void *opaque, hwaddr addr,
  s->regs[addr] = value;
  break;

-default:
-s->regs[addr] = tswap32(value);
+default: /* Packet data */
+s->regs[addr] = cpu_to_be32(value);
  break;
  }
  }


This is a change of behaviour for this device in the
qemu-system-microblazeel petalogix-s3adsp1800 board, because
previously on that system the bytes of the rx buffer would
appear in the registers in little-endian order and now they
will appear in big-endian order.


Maybe to simplify we could choose to only model the Big
Endian variant of this device?

-- >8 --
@@ -169,7 +169,7 @@ eth_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps eth_ops = {
 .read = eth_read,
 .write = eth_write,
-.endianness = DEVICE_NATIVE_ENDIAN,
+.endianness = DEVICE_BIG_ENDIAN,
 .valid = {
 .min_access_size = 4,
 .max_access_size = 4
---

Re: [PATCH] docs/devel: fix minor typo in submitting-a-patch.rst