[PULL 6/6] hw/display: fix virgl reset regression
From: Marc-André Lureau
Before commit 49afbca3b00e8e517d54964229a794b51768deaf ("virtio-gpu: drop
use_virgl_renderer"), use_virgl_renderer was preventing calling GL
functions from non-GL context threads. The innocuously looking
g->parent_obj.use_virgl_renderer = false;
was set the first time virtio_gpu_gl_reset() was called, during
pc_machine_reset() in the main thread. Further virtio_gpu_gl_reset()
calls in IO threads, without associated GL context, were thus skipping
GL calls and avoided warnings or crashes (see also
https://gitlab.freedesktop.org/virgl/virglrenderer/-/issues/226).
Signed-off-by: Marc-André Lureau
Message-Id: <20210702123221.942432-1-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann
---
include/hw/virtio/virtio-gpu.h | 1 +
hw/display/virtio-gpu-gl.c | 22 +++---
hw/display/virtio-gpu-virgl.c | 8 ++--
3 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index bcf54d970f24..24c6628944ea 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -279,6 +279,7 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd);
void virtio_gpu_virgl_fence_poll(VirtIOGPU *g);
+void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g);
void virtio_gpu_virgl_reset(VirtIOGPU *g);
int virtio_gpu_virgl_init(VirtIOGPU *g);
int virtio_gpu_virgl_get_num_capsets(VirtIOGPU *g);
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index b1035e119c3b..6cc4313b1af2 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -51,12 +51,7 @@ static void virtio_gpu_gl_update_cursor_data(VirtIOGPU *g,
static void virtio_gpu_gl_flushed(VirtIOGPUBase *b)
{
VirtIOGPU *g = VIRTIO_GPU(b);
-VirtIOGPUGL *gl = VIRTIO_GPU_GL(b);
-if (gl->renderer_reset) {
-gl->renderer_reset = false;
-virtio_gpu_virgl_reset(g);
-}
virtio_gpu_process_cmdq(g);
}
@@ -74,6 +69,10 @@ static void virtio_gpu_gl_handle_ctrl(VirtIODevice *vdev,
VirtQueue *vq)
virtio_gpu_virgl_init(g);
gl->renderer_inited = true;
}
+if (gl->renderer_reset) {
+gl->renderer_reset = false;
+virtio_gpu_virgl_reset(g);
+}
cmd = virtqueue_pop(vq, sizeof(struct virtio_gpu_ctrl_command));
while (cmd) {
@@ -95,12 +94,13 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
virtio_gpu_reset(vdev);
-if (gl->renderer_inited) {
-if (g->parent_obj.renderer_blocked) {
-gl->renderer_reset = true;
-} else {
-virtio_gpu_virgl_reset(g);
-}
+/*
+ * GL functions must be called with the associated GL context in main
+ * thread, and when the renderer is unblocked.
+ */
+if (gl->renderer_inited && !gl->renderer_reset) {
+virtio_gpu_virgl_reset_scanout(g);
+gl->renderer_reset = true;
}
}
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 092c6dc380d9..18d054922fea 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -588,17 +588,21 @@ void virtio_gpu_virgl_fence_poll(VirtIOGPU *g)
virtio_gpu_fence_poll(g);
}
-void virtio_gpu_virgl_reset(VirtIOGPU *g)
+void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
{
int i;
-virgl_renderer_reset();
for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
dpy_gl_scanout_disable(g->parent_obj.scanout[i].con);
}
}
+void virtio_gpu_virgl_reset(VirtIOGPU *g)
+{
+virgl_renderer_reset();
+}
+
int virtio_gpu_virgl_init(VirtIOGPU *g)
{
int ret;
--
2.31.1
[PULL 5/6] vl: add virtio-vga-gl to the default_list
From: Marc-André Lureau
Do not instantiate an extra default VGA device if -device virtio-vga-gl
is provided.
Related to commit b36eb8860f8f4a9c6f131c3fd380116a3017e022 ("virtio-gpu:
add virtio-vga-gl")
Signed-off-by: Marc-André Lureau
Message-Id: <20210701062421.721414-1-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann
---
softmmu/vl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4df149610185..a6c17fa39c38 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -202,6 +202,7 @@ static struct {
{ .driver = "virtio-vga", .flag = _vga },
{ .driver = "ati-vga", .flag = _vga },
{ .driver = "vhost-user-vga", .flag = _vga },
+{ .driver = "virtio-vga-gl",.flag = _vga },
};
static QemuOptsList qemu_rtc_opts = {
--
2.31.1
[PULL 4/6] hw/display: fail early when multiple virgl devices are requested
From: Marc-André Lureau
This avoids failing to initialize virgl and crashing later on, and clear
the user expectations.
Signed-off-by: Marc-André Lureau
Reviewed-by: Mark Cave-Ayland
Message-Id: <20210705104218.1161101-1-marcandre.lur...@redhat.com>
Signed-off-by: Gerd Hoffmann
---
hw/display/virtio-gpu-gl.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index 7ab93bf8c829..b1035e119c3b 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -113,6 +113,11 @@ static void virtio_gpu_gl_device_realize(DeviceState
*qdev, Error **errp)
return;
#endif
+if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
+error_setg(errp, "at most one %s device is permitted",
TYPE_VIRTIO_GPU_GL);
+return;
+}
+
if (!display_opengl) {
error_setg(errp, "opengl is not available");
return;
--
2.31.1
[PULL 3/6] Revert "qxl: add migration blocker to avoid pre-save assert"
This reverts commit 86dbcdd9c7590d06db89ca256c5eaf0b4aba8858.
The pre-save assert is gone now, so the migration blocker
is not needed any more.
Signed-off-by: Gerd Hoffmann
Reviewed-by: Marc-André Lureau
Message-Id: <20210721093347.338536-3-kra...@redhat.com>
---
hw/display/qxl.h | 1 -
hw/display/qxl.c | 31 ---
2 files changed, 32 deletions(-)
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 379d3304abc1..30d21f4d0bdc 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -39,7 +39,6 @@ struct PCIQXLDevice {
uint32_t cmdlog;
uint32_t guest_bug;
-Error *migration_blocker;
enum qxl_mode mode;
uint32_t cmdflags;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 3867b94fe236..43482d4364ba 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -30,7 +30,6 @@
#include "qemu/module.h"
#include "hw/qdev-properties.h"
#include "sysemu/runstate.h"
-#include "migration/blocker.h"
#include "migration/vmstate.h"
#include "trace.h"
@@ -666,30 +665,6 @@ static int interface_get_command(QXLInstance *sin, struct
QXLCommandExt *ext)
qxl->guest_primary.commands++;
qxl_track_command(qxl, ext);
qxl_log_command(qxl, "cmd", ext);
-{
-/*
- * Windows 8 drivers place qxl commands in the vram
- * (instead of the ram) bar. We can't live migrate such a
- * guest, so add a migration blocker in case we detect
- * this, to avoid triggering the assert in pre_save().
- *
- *
https://cgit.freedesktop.org/spice/win32/qxl-wddm-dod/commit/?id=f6e099db39e7d0787f294d5fd0dce328b5210faa
- */
-void *msg = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
-if (msg != NULL && (
-msg < (void *)qxl->vga.vram_ptr ||
-msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
-if (!qxl->migration_blocker) {
-Error *local_err = NULL;
-error_setg(>migration_blocker,
- "qxl: guest bug: command not in ram bar");
-migrate_add_blocker(qxl->migration_blocker, _err);
-if (local_err) {
-error_report_err(local_err);
-}
-}
-}
-}
trace_qxl_ring_command_get(qxl->id, qxl_mode_to_string(qxl->mode));
return true;
default:
@@ -1283,12 +1258,6 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
qemu_spice_create_host_memslot(>ssd);
qxl_soft_reset(d);
-if (d->migration_blocker) {
-migrate_del_blocker(d->migration_blocker);
-error_free(d->migration_blocker);
-d->migration_blocker = NULL;
-}
-
if (startstop) {
qemu_spice_display_start();
}
--
2.31.1
[PULL 2/6] qxl: remove assert in qxl_pre_save.
Since commit 551dbd0846d2 ("migration: check pre_save return in
vmstate_save_state") the pre_save hook can fail. So lets finally
use that to drop the guest-triggerable assert in qxl_pre_save().
Signed-off-by: Gerd Hoffmann
Reviewed-by: Marc-André Lureau
Message-Id: <20210721093347.338536-2-kra...@redhat.com>
---
hw/display/qxl.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 84f99088e0a0..3867b94fe236 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
} else {
d->last_release_offset = (uint8_t *)d->last_release - ram_start;
}
-assert(d->last_release_offset < d->vga.vram_size);
+if (d->last_release_offset < d->vga.vram_size) {
+return 1;
+}
return 0;
}
--
2.31.1
[PULL 0/6] Vga 20210723 patches
The following changes since commit e77c8b8b8e933414ef07dbed04e02973fccffeb0: Update version for v6.1.0-rc0 release (2021-07-21 17:10:15 +0100) are available in the Git repository at: git://git.kraxel.org/qemu tags/vga-20210723-pull-request for you to fetch changes up to 8a13b9bc0f283caff4333c75bc396a963f47ce5c: hw/display: fix virgl reset regression (2021-07-22 15:46:54 +0200) vga: fixes for qxl and virtio-gpu Gerd Hoffmann (2): qxl: remove assert in qxl_pre_save. Revert "qxl: add migration blocker to avoid pre-save assert" Marc-André Lureau (3): hw/display: fail early when multiple virgl devices are requested vl: add virtio-vga-gl to the default_list hw/display: fix virgl reset regression Philippe Mathieu-Daudé (1): hw/display/virtio-gpu: Fix memory leak (CID 1453811) hw/display/qxl.h | 1 - include/hw/virtio/virtio-gpu.h | 1 + hw/display/qxl.c | 35 +++--- hw/display/virtio-gpu-gl.c | 27 +++--- hw/display/virtio-gpu-virgl.c | 8 ++-- hw/display/virtio-gpu.c| 26 ++--- softmmu/vl.c | 1 + 7 files changed, 37 insertions(+), 62 deletions(-) -- 2.31.1
[PULL 1/6] hw/display/virtio-gpu: Fix memory leak (CID 1453811)
From: Philippe Mathieu-Daudé
To avoid leaking memory on the error path, reorder the
code as:
- check the parameters first
- check resource already existing
- finally allocate memory
Reported-by: Coverity (CID 1453811: RESOURCE_LEAK)
Fixes: e0933d91b1c ("virtio-gpu: Add virtio_gpu_resource_create_blob")
Signed-off-by: Philippe Mathieu-Daudé
Message-Id: <20210531101928.1662732-1-phi...@redhat.com>
Signed-off-by: Gerd Hoffmann
---
hw/display/virtio-gpu.c | 26 ++
1 file changed, 10 insertions(+), 16 deletions(-)
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 6b7f643951fe..990e71fd4062 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -340,37 +340,31 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
return;
}
-res = virtio_gpu_find_resource(g, cblob.resource_id);
-if (res) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
- __func__, cblob.resource_id);
-cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
-return;
-}
-
-res = g_new0(struct virtio_gpu_simple_resource, 1);
-res->resource_id = cblob.resource_id;
-res->blob_size = cblob.size;
-
if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_GUEST &&
cblob.blob_flags != VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid memory type\n",
__func__);
cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
-g_free(res);
return;
}
-if (res->iov) {
-cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+if (virtio_gpu_find_resource(g, cblob.resource_id)) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: resource already exists %d\n",
+ __func__, cblob.resource_id);
+cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
return;
}
+res = g_new0(struct virtio_gpu_simple_resource, 1);
+res->resource_id = cblob.resource_id;
+res->blob_size = cblob.size;
+
ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
cmd, >addrs, >iov,
>iov_cnt);
-if (ret != 0) {
+if (ret != 0 || res->iov) {
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
+g_free(res);
return;
}
--
2.31.1
Re: [PATCH v2 13/22] target/loongarch: Add floating point arithmetic instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
+uint64_t helper_fp_sqrt_d(CPULoongArchState *env, uint64_t fp)
+{
+fp = float64_sqrt(fp, >active_fpu.fp_status);
+update_fcsr0(env, GETPC());
+return fp;
+}
+
+uint32_t helper_fp_sqrt_s(CPULoongArchState *env, uint32_t fp)
+{
+fp = float32_sqrt(fp, >active_fpu.fp_status);
+update_fcsr0(env, GETPC());
+return fp;
+}
I believe you will find it easier to take and return uint64_t, even for 32-bit operations.
The manual says that the high bits may contain any value, so in my opinion you should
not work hard to preserve the high bits, as you currently do with
+gen_load_fpr32(fp0, a->fj);
+gen_load_fpr32(fp1, a->fk);
+gen_helper_fp_add_s(fp0, cpu_env, fp0, fp1);
+gen_store_fpr32(fp0, a->fd);
I think this should be as simple as
gen_helper_fp_add_s(cpu_fpu[a->fd], cpu_env,
cpu_fpu[a->fj], cpu_fpu[a->fk]);
I also think that loongarch should learn from risc-v and change the architecture to
"nan-box" single-precision results -- fill the high 32-bits with 1s. This is an SNaN
representation for double-precision and will immediately fail when incorrectly using a
single-precision value as a double-precision input.
Thankfully the current architecture is backward compatible with nan-boxing.
+/* Floating point arithmetic operation instruction translation */
+static bool trans_fadd_s(DisasContext *ctx, arg_fadd_s * a)
+{
+TCGv_i32 fp0, fp1;
+
+fp0 = tcg_temp_new_i32();
+fp1 = tcg_temp_new_i32();
+
+check_fpu_enabled(ctx);
+gen_load_fpr32(fp0, a->fj);
+gen_load_fpr32(fp1, a->fk);
+gen_helper_fp_add_s(fp0, cpu_env, fp0, fp1);
+gen_store_fpr32(fp0, a->fd);
+
+tcg_temp_free_i32(fp0);
+tcg_temp_free_i32(fp1);
+
+return true;
+}
Again, you should use some helper functions to reduce the repetition.
+static bool trans_fmadd_d(DisasContext *ctx, arg_fmadd_d *a)
+{
+TCGv_i64 fp0, fp1, fp2, fp3;
+
+fp0 = tcg_temp_new_i64();
+fp1 = tcg_temp_new_i64();
+fp2 = tcg_temp_new_i64();
+fp3 = tcg_temp_new_i64();
+
+check_fpu_enabled(ctx);
+gen_load_fpr64(fp0, a->fj);
+gen_load_fpr64(fp1, a->fk);
+gen_load_fpr64(fp2, a->fa);
+check_fpu_enabled(ctx);
Repeating check_fpu_enabled.
+gen_helper_fp_madd_d(fp3, cpu_env, fp0, fp1, fp2);
+gen_store_fpr64(fp3, a->fd);
I think you might as well pass in the float_muladd_* constant to a single helper rather
than having 4 different helpers.
r~
Re: [PATCH v3 0/2] Add more 64-bit utilities
On Wed, Jul 21, 2021 at 4:31 AM Joe Komlodi wrote: > > Changelog: > v2 -> v3 > - 1/2: register block init should also be uint64_t > v1 -> v2 > - 2/2: Use uint64_t for 64-bit value > > Hi all, > > This adds more utilities for 64-bit registers. > As part of it, it also fixes FIELD_DP64 to work with bit fields wider than > 32-bits. > > Thanks! > Joe > > Joe Komlodi (2): > hw/core/register: Add more 64-bit utilities > hw/registerfields: Use 64-bit bitfield for FIELD_DP64 Thanks! Applied to riscv-to-apply.next Alistair > > hw/core/register.c | 12 > include/hw/register.h | 8 > include/hw/registerfields.h | 10 +- > 3 files changed, 29 insertions(+), 1 deletion(-) > > -- > 2.7.4 >
Re: [PATCH v2 12/22] target/loongarch: Add fixed point extra instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
+target_ulong helper_cpucfg(CPULoongArchState *env, target_ulong rj)
+{
+target_ulong r = 0;
+
+switch (rj) {
+case 0:
+r = env->CSR_MCSR0 & 0x;
+break;
+case 1:
+r = (env->CSR_MCSR0 & 0x) >> 32;
+break;
Why do you represent all of these as high and low portions of a 64-bit internal value,
when the manual describes them as 32-bit values?
+/* Fixed point extra instruction translation */
+static bool trans_crc_w_b_w(DisasContext *ctx, arg_crc_w_b_w *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+TCGv_i32 tsz = tcg_const_i32(1 << 1);
This size is wrong. It should be 1, not 1 << 1 (2).
+static bool trans_crc_w_w_w(DisasContext *ctx, arg_crc_w_w_w *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+TCGv_i32 tsz = tcg_const_i32(1 << 4);
Because this size most certainly should not be 16...
+static bool trans_crc_w_d_w(DisasContext *ctx, arg_crc_w_d_w *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+TCGv_i32 tsz = tcg_const_i32(1 << 8);
... and this size should not be 256. Both well larger than the 8 byte buffer that you've
allocated.
Also, you need a helper so that you don't have 8 copies of this code.
+static bool trans_asrtle_d(DisasContext *ctx, arg_asrtle_d * a)
+{
+TCGv t0, t1;
+
+t0 = get_gpr(a->rj);
+t1 = get_gpr(a->rk);
+
+gen_helper_asrtle_d(cpu_env, t0, t1);
+
+return true;
+}
+
+static bool trans_asrtgt_d(DisasContext *ctx, arg_asrtgt_d * a)
+{
+TCGv t0, t1;
+
+t0 = get_gpr(a->rj);
+t1 = get_gpr(a->rk);
+
+gen_helper_asrtgt_d(cpu_env, t0, t1);
+
+return true;
+}
I'm not sure why both of these instructions are in the ISA, since
ASRTLE X,Y <-> ASRTGT Y,X
but we certainly don't need two different helpers.
Just swap the arguments for one of them.
+static bool trans_rdtimel_w(DisasContext *ctx, arg_rdtimel_w *a)
+{
+/* Nop */
+return true;
+}
+
+static bool trans_rdtimeh_w(DisasContext *ctx, arg_rdtimeh_w *a)
+{
+/* Nop */
+return true;
+}
+
+static bool trans_rdtime_d(DisasContext *ctx, arg_rdtime_d *a)
+{
+/* Nop */
+return true;
+}
If you don't want to implement these right now, you should at least initialize the
destination register to 0, or something.
r~
Re: [PATCH v3 1/2] hw/core/register: Add more 64-bit utilities
On Wed, Jul 21, 2021 at 4:31 AM Joe Komlodi wrote:
>
> We already have some utilities to handle 64-bit wide registers, so this just
> adds some more for:
> - Initializing 64-bit registers
> - Extracting and depositing to an array of 64-bit registers
>
> Signed-off-by: Joe Komlodi
Reviewed-by: Alistair Francis
Alistair
> ---
> hw/core/register.c | 12
> include/hw/register.h | 8
> include/hw/registerfields.h | 8
> 3 files changed, 28 insertions(+)
>
> diff --git a/hw/core/register.c b/hw/core/register.c
> index d6f8c20..95b0150 100644
> --- a/hw/core/register.c
> +++ b/hw/core/register.c
> @@ -300,6 +300,18 @@ RegisterInfoArray *register_init_block32(DeviceState
> *owner,
> data, ops, debug_enabled, memory_size, 32);
> }
>
> +RegisterInfoArray *register_init_block64(DeviceState *owner,
> + const RegisterAccessInfo *rae,
> + int num, RegisterInfo *ri,
> + uint64_t *data,
> + const MemoryRegionOps *ops,
> + bool debug_enabled,
> + uint64_t memory_size)
> +{
> +return register_init_block(owner, rae, num, ri, (void *)
> + data, ops, debug_enabled, memory_size, 64);
> +}
> +
> void register_finalize_block(RegisterInfoArray *r_array)
> {
> object_unparent(OBJECT(_array->mem));
> diff --git a/include/hw/register.h b/include/hw/register.h
> index b480e38..6a076cf 100644
> --- a/include/hw/register.h
> +++ b/include/hw/register.h
> @@ -204,6 +204,14 @@ RegisterInfoArray *register_init_block32(DeviceState
> *owner,
> bool debug_enabled,
> uint64_t memory_size);
>
> +RegisterInfoArray *register_init_block64(DeviceState *owner,
> + const RegisterAccessInfo *rae,
> + int num, RegisterInfo *ri,
> + uint64_t *data,
> + const MemoryRegionOps *ops,
> + bool debug_enabled,
> + uint64_t memory_size);
> +
> /**
> * This function should be called to cleanup the registers that were
> initialized
> * when calling register_init_block32(). This function should only be called
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> index 93fa4a8..9a03ac5 100644
> --- a/include/hw/registerfields.h
> +++ b/include/hw/registerfields.h
> @@ -30,6 +30,10 @@
> enum { A_ ## reg = (addr) }; \
> enum { R_ ## reg = (addr) / 2 };
>
> +#define REG64(reg, addr) \
> +enum { A_ ## reg = (addr) }; \
> +enum { R_ ## reg = (addr) / 8 };
> +
> /* Define SHIFT, LENGTH and MASK constants for a field within a register */
>
> /* This macro will define R_FOO_BAR_MASK, R_FOO_BAR_SHIFT and
> R_FOO_BAR_LENGTH
> @@ -58,6 +62,8 @@
> /* Extract a field from an array of registers */
> #define ARRAY_FIELD_EX32(regs, reg, field)\
> FIELD_EX32((regs)[R_ ## reg], reg, field)
> +#define ARRAY_FIELD_EX64(regs, reg, field)\
> +FIELD_EX64((regs)[R_ ## reg], reg, field)
>
> /* Deposit a register field.
> * Assigning values larger then the target field will result in
> @@ -99,5 +105,7 @@
> /* Deposit a field to array of registers. */
> #define ARRAY_FIELD_DP32(regs, reg, field, val) \
> (regs)[R_ ## reg] = FIELD_DP32((regs)[R_ ## reg], reg, field, val);
> +#define ARRAY_FIELD_DP64(regs, reg, field, val) \
> +(regs)[R_ ## reg] = FIELD_DP64((regs)[R_ ## reg], reg, field, val);
>
> #endif
> --
> 2.7.4
>
Re: [PATCH 14/17] target/riscv: Tidy trans_rvh.c.inc
On Fri, Jul 9, 2021 at 2:52 PM Richard Henderson
wrote:
>
> Exit early if check_access fails.
> Split out do_hlv, do_hsv, do_hlvx subroutines.
> Use gpr_src, gpr_dst in the new subroutines.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/insn32.decode | 1 +
> target/riscv/insn_trans/trans_rvh.c.inc | 264 +---
> 2 files changed, 55 insertions(+), 210 deletions(-)
>
> diff --git a/target/riscv/insn32.decode b/target/riscv/insn32.decode
> index f09f8d5faf..2cd921d51c 100644
> --- a/target/riscv/insn32.decode
> +++ b/target/riscv/insn32.decode
> @@ -42,6 +42,7 @@
> imm rd
> rd rs1 rs2
> rd rs1
> +_s rs1 rs2
> imm rs1 rs2
> imm rd
> shamt rs1 rd
> diff --git a/target/riscv/insn_trans/trans_rvh.c.inc
> b/target/riscv/insn_trans/trans_rvh.c.inc
> index 6b5edf82b7..dac732024b 100644
> --- a/target/riscv/insn_trans/trans_rvh.c.inc
> +++ b/target/riscv/insn_trans/trans_rvh.c.inc
> @@ -17,281 +17,137 @@
> */
>
> #ifndef CONFIG_USER_ONLY
> -static void check_access(DisasContext *ctx) {
> +static bool check_access(DisasContext *ctx)
> +{
> if (!ctx->hlsx) {
> if (ctx->virt_enabled) {
> generate_exception(ctx, RISCV_EXCP_VIRT_INSTRUCTION_FAULT);
> } else {
> generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
> }
> +return false;
> }
> +return true;
> }
> #endif
>
> +static bool do_hlv(DisasContext *ctx, arg_r2 *a, MemOp mop)
> +{
> +#ifdef CONFIG_USER_ONLY
> +return false;
> +#else
> +if (check_access(ctx)) {
> +TCGv dest = gpr_dst(ctx, a->rd);
> +TCGv addr = gpr_src(ctx, a->rs1);
> +int mem_idx = ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> +tcg_gen_qemu_ld_tl(dest, addr, mem_idx, mop);
> +}
> +return true;
> +#endif
> +}
> +
> static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
> {
> REQUIRE_EXT(ctx, RVH);
> -#ifndef CONFIG_USER_ONLY
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> -
> -check_access(ctx);
> -
> -gen_get_gpr(t0, a->rs1);
> -
> -tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK,
> MO_SB);
> -gen_set_gpr(a->rd, t1);
> -
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -return true;
> -#else
> -return false;
> -#endif
> +return do_hlv(ctx, a, MO_SB);
> }
>
> static bool trans_hlv_h(DisasContext *ctx, arg_hlv_h *a)
> {
> REQUIRE_EXT(ctx, RVH);
> -#ifndef CONFIG_USER_ONLY
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> -
> -check_access(ctx);
> -
> -gen_get_gpr(t0, a->rs1);
> -
> -tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK,
> MO_TESW);
> -gen_set_gpr(a->rd, t1);
> -
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -return true;
> -#else
> -return false;
> -#endif
> +return do_hlv(ctx, a, MO_TESW);
> }
>
> static bool trans_hlv_w(DisasContext *ctx, arg_hlv_w *a)
> {
> REQUIRE_EXT(ctx, RVH);
> -#ifndef CONFIG_USER_ONLY
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> -
> -check_access(ctx);
> -
> -gen_get_gpr(t0, a->rs1);
> -
> -tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK,
> MO_TESL);
> -gen_set_gpr(a->rd, t1);
> -
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -return true;
> -#else
> -return false;
> -#endif
> +return do_hlv(ctx, a, MO_TESL);
> }
>
> static bool trans_hlv_bu(DisasContext *ctx, arg_hlv_bu *a)
> {
> REQUIRE_EXT(ctx, RVH);
> -#ifndef CONFIG_USER_ONLY
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> -
> -check_access(ctx);
> -
> -gen_get_gpr(t0, a->rs1);
> -
> -tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK,
> MO_UB);
> -gen_set_gpr(a->rd, t1);
> -
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -return true;
> -#else
> -return false;
> -#endif
> +return do_hlv(ctx, a, MO_UB);
> }
>
> static bool trans_hlv_hu(DisasContext *ctx, arg_hlv_hu *a)
> {
> REQUIRE_EXT(ctx, RVH);
> -#ifndef CONFIG_USER_ONLY
> -TCGv t0 = tcg_temp_new();
> -TCGv t1 = tcg_temp_new();
> +return do_hlv(ctx, a, MO_TEUW);
> +}
>
> -check_access(ctx);
> -
> -gen_get_gpr(t0, a->rs1);
> -tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK,
> MO_TEUW);
> -gen_set_gpr(a->rd, t1);
> -
> -tcg_temp_free(t0);
> -tcg_temp_free(t1);
> -return true;
> -#else
> +static bool do_hsv(DisasContext *ctx, arg_r2_s *a, MemOp mop)
> +{
> +#ifdef CONFIG_USER_ONLY
> return false;
> +#else
> +if (check_access(ctx)) {
> +TCGv addr = gpr_src(ctx, a->rs1);
> +TCGv data = gpr_src(ctx, a->rs2);
> +int mem_idx = ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> +tcg_gen_qemu_ld_tl(data, addr, mem_idx, mop);
> +}
> +return true;
> #endif
> }
Re: [PATCH 09/17] target/riscv: Reorg csr instructions
On Fri, Jul 9, 2021 at 2:46 PM Richard Henderson
wrote:
>
> Introduce csrr and csrw helpers, for read-only and write-only insns.
>
> Note that we do not properly implement this in riscv_csrrw, in that
> we cannot distinguish true read-only (rs1 == 0) from any other zero
> write_mask another source register -- this should still raise an
> exception for read-only registers.
>
> Only issue gen_io_start for CF_USE_ICOUNT.
> Use ctx->zero for csrrc.
>
> Signed-off-by: Richard Henderson
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/helper.h | 6 +-
> target/riscv/op_helper.c| 18 +--
> target/riscv/insn_trans/trans_rvi.c.inc | 170 +---
> 3 files changed, 129 insertions(+), 65 deletions(-)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 415e37bc37..460eee9988 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -65,9 +65,9 @@ DEF_HELPER_FLAGS_2(gorc, TCG_CALL_NO_RWG_SE, tl, tl, tl)
> DEF_HELPER_FLAGS_2(gorcw, TCG_CALL_NO_RWG_SE, tl, tl, tl)
>
> /* Special functions */
> -DEF_HELPER_3(csrrw, tl, env, tl, tl)
> -DEF_HELPER_4(csrrs, tl, env, tl, tl, tl)
> -DEF_HELPER_4(csrrc, tl, env, tl, tl, tl)
> +DEF_HELPER_2(csrr, tl, env, int)
> +DEF_HELPER_3(csrw, void, env, int, tl)
> +DEF_HELPER_4(csrrw, tl, env, int, tl, tl)
> #ifndef CONFIG_USER_ONLY
> DEF_HELPER_2(sret, tl, env, tl)
> DEF_HELPER_2(mret, tl, env, tl)
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 3c48e739ac..ee7c24efe7 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -37,11 +37,10 @@ void helper_raise_exception(CPURISCVState *env, uint32_t
> exception)
> riscv_raise_exception(env, exception, 0);
> }
>
> -target_ulong helper_csrrw(CPURISCVState *env, target_ulong src,
> -target_ulong csr)
> +target_ulong helper_csrr(CPURISCVState *env, int csr)
> {
> target_ulong val = 0;
> -RISCVException ret = riscv_csrrw(env, csr, , src, -1);
> +RISCVException ret = riscv_csrrw(env, csr, , 0, 0);
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
> @@ -49,23 +48,20 @@ target_ulong helper_csrrw(CPURISCVState *env,
> target_ulong src,
> return val;
> }
>
> -target_ulong helper_csrrs(CPURISCVState *env, target_ulong src,
> -target_ulong csr, target_ulong rs1_pass)
> +void helper_csrw(CPURISCVState *env, int csr, target_ulong src)
> {
> -target_ulong val = 0;
> -RISCVException ret = riscv_csrrw(env, csr, , -1, rs1_pass ? src : 0);
> +RISCVException ret = riscv_csrrw(env, csr, NULL, src, -1);
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
> }
> -return val;
> }
>
> -target_ulong helper_csrrc(CPURISCVState *env, target_ulong src,
> -target_ulong csr, target_ulong rs1_pass)
> +target_ulong helper_csrrw(CPURISCVState *env, int csr,
> + target_ulong src, target_ulong write_mask)
> {
> target_ulong val = 0;
> -RISCVException ret = riscv_csrrw(env, csr, , 0, rs1_pass ? src : 0);
> +RISCVException ret = riscv_csrrw(env, csr, , src, write_mask);
>
> if (ret != RISCV_EXCP_NONE) {
> riscv_raise_exception(env, ret, GETPC());
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc
> b/target/riscv/insn_trans/trans_rvi.c.inc
> index 840187a4d6..3705aad380 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -452,80 +452,148 @@ static bool trans_fence_i(DisasContext *ctx,
> arg_fence_i *a)
> return true;
> }
>
> -#define RISCV_OP_CSR_PRE do {\
> -source1 = tcg_temp_new(); \
> -csr_store = tcg_temp_new(); \
> -dest = tcg_temp_new(); \
> -rs1_pass = tcg_temp_new(); \
> -gen_get_gpr(source1, a->rs1); \
> -tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next); \
> -tcg_gen_movi_tl(rs1_pass, a->rs1); \
> -tcg_gen_movi_tl(csr_store, a->csr); \
> -gen_io_start();\
> -} while (0)
> +static bool do_csr_post(DisasContext *ctx)
> +{
> +/* We may have changed important cpu state -- exit to main loop. */
> +tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
> +exit_tb(ctx);
> +ctx->base.is_jmp = DISAS_NORETURN;
> +return true;
> +}
>
> -#define RISCV_OP_CSR_POST do {\
> -gen_set_gpr(a->rd, dest); \
> -tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn); \
> -exit_tb(ctx); \
> -ctx->base.is_jmp = DISAS_NORETURN; \
> -tcg_temp_free(source1); \
> -tcg_temp_free(csr_store); \
> -tcg_temp_free(dest); \
> -tcg_temp_free(rs1_pass); \
> -} while (0)
> +static bool do_csrr(DisasContext *ctx, int rd, int rc)
> +{
> +TCGv dest = gpr_dst(ctx, rd);
> +TCGv_i32 csr = tcg_constant_i32(rc);
>
> +if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> +gen_io_start();
> +}
> +gen_helper_csrr(dest, cpu_env, csr);
> +return do_csr_post(ctx);
> +}
> +
> +static bool do_csrw(DisasContext
[PATCH] misc/pca9552: Fix LED status register indexing in pca955x_get_led()
There was a bit of a thinko in the state calculation where every odd pin
in was reported in e.g. "pwm0" mode rather than "off". This was the
result of an incorrect bit shift for the 2-bit field representing each
LED state.
Fixes: a90d8f84674d ("misc/pca9552: Add qom set and get")
Signed-off-by: Andrew Jeffery
---
hw/misc/pca9552.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index b7686e27d7fa..fff19e369a39 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -272,7 +272,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const
char *name,
* reading the INPUTx reg
*/
reg = PCA9552_LS0 + led / 4;
-state = (pca955x_read(s, reg) >> (led % 8)) & 0x3;
+state = (pca955x_read(s, reg) >> ((led % 4) * 2)) & 0x3;
visit_type_str(v, name, (char **)_state[state], errp);
}
--
2.30.2
Re: [PATCH v1 2/5] hw/intc: sifive_clint: Use RISC-V CPU GPIO lines
On Wed, Jul 14, 2021 at 11:41 AM Alistair Francis wrote:
>
> On Tue, Jul 13, 2021 at 2:06 PM Anup Patel wrote:
> >
> > On Fri, Jul 9, 2021 at 9:01 AM Alistair Francis
> > wrote:
> > >
> > > Instead of using riscv_cpu_update_mip() let's instead use the new RISC-V
> > > CPU GPIO lines to set the timer and soft MIP bits.
> > >
> > > Signed-off-by: Alistair Francis
> > > ---
> > > include/hw/intc/sifive_clint.h | 2 +
> > > hw/intc/sifive_clint.c | 72 --
> > > 2 files changed, 54 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/hw/intc/sifive_clint.h
> > > b/include/hw/intc/sifive_clint.h
> > > index a30be0f3d6..921b1561dd 100644
> > > --- a/include/hw/intc/sifive_clint.h
> > > +++ b/include/hw/intc/sifive_clint.h
> > > @@ -40,6 +40,8 @@ typedef struct SiFiveCLINTState {
> > > uint32_t time_base;
> > > uint32_t aperture_size;
> > > uint32_t timebase_freq;
> > > +qemu_irq *timer_irqs;
> > > +qemu_irq *soft_irqs;
> > > } SiFiveCLINTState;
> > >
> > > DeviceState *sifive_clint_create(hwaddr addr, hwaddr size,
> > > diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> > > index 0f41e5ea1c..c635a47507 100644
> > > --- a/hw/intc/sifive_clint.c
> > > +++ b/hw/intc/sifive_clint.c
> > > @@ -28,6 +28,12 @@
> > > #include "hw/qdev-properties.h"
> > > #include "hw/intc/sifive_clint.h"
> > > #include "qemu/timer.h"
> > > +#include "hw/irq.h"
> > > +
> > > +typedef struct sifive_clint_callback {
> > > +SiFiveCLINTState *s;
> > > +int num;
> > > +} sifive_clint_callback;
> > >
> > > static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> > > {
> > > @@ -39,7 +45,9 @@ static uint64_t cpu_riscv_read_rtc(uint32_t
> > > timebase_freq)
> > > * Called when timecmp is written to update the QEMU timer or immediately
> > > * trigger timer interrupt if mtimecmp <= current timer value.
> > > */
> > > -static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value,
> > > +static void sifive_clint_write_timecmp(SiFiveCLINTState *s, RISCVCPU
> > > *cpu,
> > > + int hartid,
> > > + uint64_t value,
> > > uint32_t timebase_freq)
> > > {
> > > uint64_t next;
> > > @@ -51,12 +59,12 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> > > uint64_t value,
> > > if (cpu->env.timecmp <= rtc_r) {
> > > /* if we're setting an MTIMECMP value in the "past",
> > > immediately raise the timer interrupt */
> > > -riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
> > > +qemu_irq_raise(s->timer_irqs[hartid]);
> >
> > This breaks multi-socket support.
> >
> > Please use "hartid - s->hartid_base" as an index.
> >
> > > return;
> > > }
> > >
> > > /* otherwise, set up the future timer interrupt */
> > > -riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
> > > +qemu_irq_lower(s->timer_irqs[hartid]);
> > > diff = cpu->env.timecmp - rtc_r;
> > > /* back to ns (note args switched in muldiv64) */
> > > next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > > @@ -70,8 +78,9 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> > > uint64_t value,
> > > */
> > > static void sifive_clint_timer_cb(void *opaque)
> > > {
> > > -RISCVCPU *cpu = opaque;
> > > -riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(1));
> > > +sifive_clint_callback *state = opaque;
> > > +
> > > +qemu_irq_raise(state->s->timer_irqs[state->num]);
> > > }
> > >
> > > /* CPU wants to read rtc or timecmp register */
> > > @@ -137,7 +146,11 @@ static void sifive_clint_write(void *opaque, hwaddr
> > > addr, uint64_t value,
> > > if (!env) {
> > > error_report("clint: invalid timecmp hartid: %zu", hartid);
> > > } else if ((addr & 0x3) == 0) {
> > > -riscv_cpu_update_mip(RISCV_CPU(cpu), MIP_MSIP,
> > > BOOL_TO_MASK(value));
> > > +if (value) {
> > > +qemu_irq_raise(clint->soft_irqs[hartid]);
> > > +} else {
> > > +qemu_irq_lower(clint->soft_irqs[hartid]);
> > > +}
> >
> > Eventthis is broken for multi-socket.
> >
> > Use "hartid - clint->hartid_base" as index.
>
> Thanks for testing this. I have fixed this and will send a v2. I also
> added multi-socket tests to my automated tests.
Thanks, I will rebase the ACLINT series on your v2 series.
Regards,
Anup
>
> Alistair
>
> >
> > > } else {
> > > error_report("clint: invalid sip write: %08x",
> > > (uint32_t)addr);
> > > }
> > > @@ -153,13 +166,13 @@ static void sifive_clint_write(void *opaque, hwaddr
> > > addr, uint64_t value,
> > > } else if ((addr & 0x7) == 0) {
> > > /* timecmp_lo */
> > > uint64_t timecmp_hi = env->timecmp >> 32;
> > > -sifive_clint_write_timecmp(RISCV_CPU(cpu),
> > > +
Re: [PULL V3 for 6.2 0/6] COLO-Proxy patches for 2021-06-25
在 2021/7/19 下午5:00, Zhang Chen 写道: Hi Jason, Please help to queue COLO-proxy patches to net branch. Thanks Chen Queued for 6.2 Thanks The following changes since commit fd79f89c76c8e2f409dd9db5d7a367b1f64b6dc6: Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20210718' into staging (2021-07-18 13:46:39 +0100) are available in the Git repository at: https://github.com/zhangckid/qemu.git master-colo-21jun25-pull-request-v3 for you to fetch changes up to 91176794e3a72c74b01e149638ac1a7e2dee73fc: net/net.c: Add handler for passthrough filter command (2021-07-19 16:50:44 +0800) This series add passthrough support frame to object with network processing function. The first object is colo-compare. V3: Fix memory leak issue. V2: Optimize HMP code from Dave's comment. Zhang Chen (6): qapi/net: Add IPFlowSpec and QMP command for filter passthrough util/qemu-sockets.c: Add inet_parse_base to handle InetSocketAddressBase hmp-commands: Add new HMP command for filter passthrough net/colo-compare: Move data structure and define to .h file. net/colo-compare: Add passthrough list to CompareState net/net.c: Add handler for passthrough filter command hmp-commands.hx| 26 include/monitor/hmp.h | 2 + include/qemu/sockets.h | 1 + monitor/hmp-cmds.c | 63 +++ net/colo-compare.c | 160 net/colo-compare.h | 98 ++ net/net.c | 205 ++ qapi/net.json | 78 util/qemu-sockets.c| 14 + 9 files changed, 538 insertions(+), 109 deletions(-)
[PATCH for-6.1 v3 1/1] machine: Disallow specifying topology parameters as zero
In the SMP configuration, we should either specify a topology
parameter with a reasonable value (equal to or greater than 1)
or just leave it omitted and QEMU will calculate its value.
Configurations which explicitly specify the topology parameters
as zero like "sockets=0" are meaningless, so disallow them.
However, the commit 1e63fe685804d
(machine: pass QAPI struct to mc->smp_parse) has documented that
'0' has the same semantics as omitting a parameter in the qapi
comment for SMPConfiguration. So this patch fixes the doc and
also adds the corresponding sanity check in the smp parsers.
Reviewed-by: Andrew Jones
Reviewed-by: Daniel P. Berrange
Tested-by: Daniel P. Berrange
Suggested-by: Andrew Jones
Signed-off-by: Yanan Wang
---
hw/core/machine.c | 15 +++
qapi/machine.json | 6 +++---
qemu-options.hx | 12 +++-
3 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 775add0795..d95e8b6903 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -829,6 +829,21 @@ static void machine_set_smp(Object *obj, Visitor *v, const
char *name,
return;
}
+/*
+ * The topology parameters must be specified equal to or great than one
+ * or just omitted, explicit configuration like "cpus=0" is not allowed.
+ */
+if ((config->has_cpus && config->cpus == 0) ||
+(config->has_sockets && config->sockets == 0) ||
+(config->has_dies && config->dies == 0) ||
+(config->has_cores && config->cores == 0) ||
+(config->has_threads && config->threads == 0) ||
+(config->has_maxcpus && config->maxcpus == 0)) {
+error_setg(errp, "CPU topology parameters must be equal to "
+ "or greater than one if provided");
+goto out_free;
+}
+
mc->smp_parse(ms, config, errp);
if (errp) {
goto out_free;
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb..9272cb3cf8 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1288,8 +1288,8 @@
##
# @SMPConfiguration:
#
-# Schema for CPU topology configuration. "0" or a missing value lets
-# QEMU figure out a suitable value based on the ones that are provided.
+# Schema for CPU topology configuration. A missing value lets QEMU
+# figure out a suitable value based on the ones that are provided.
#
# @cpus: number of virtual CPUs in the virtual machine
#
@@ -1297,7 +1297,7 @@
#
# @dies: number of dies per socket in the CPU topology
#
-# @cores: number of cores per thread in the CPU topology
+# @cores: number of cores per die in the CPU topology
#
# @threads: number of threads per core in the CPU topology
#
diff --git a/qemu-options.hx b/qemu-options.hx
index 99ed5ec5f1..b0168f8c48 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -223,11 +223,13 @@ SRST
of computing the CPU maximum count.
Either the initial CPU count, or at least one of the topology parameters
-must be specified. Values for any omitted parameters will be computed
-from those which are given. Historically preference was given to the
-coarsest topology parameters when computing missing values (ie sockets
-preferred over cores, which were preferred over threads), however, this
-behaviour is considered liable to change.
+must be specified. The specified parameters must be equal to or great
+than one, explicit configuration like "cpus=0" is not allowed. Values
+for any omitted parameters will be computed from those which are given.
+Historically preference was given to the coarsest topology parameters
+when computing missing values (ie sockets preferred over cores, which
+were preferred over threads), however, this behaviour is considered
+liable to change.
ERST
DEF("numa", HAS_ARG, QEMU_OPTION_numa,
--
2.19.1
[PATCH for-6.1 v3 0/1] machine: Disallow specifying topology parameters as zero
In the SMP configuration, we should either specify a topology parameter with a reasonable value (equal to or greater than 1) or just leave it omitted and QEMU will calculate its value. Configurations which explicitly specify the topology parameters as zero like "sockets=0" are meaningless, so disallow them. However; the commit 1e63fe685804d (machine: pass QAPI struct to mc->smp_parse) has documented that '0' has the same semantics as omitting a parameter in the qapi comment for SMPConfiguration. So this patch fixes the doc and also adds the corresponding sanity check in the smp parsers. This patch originly comes form [1], and it was suggested that this patch fixing the doc should be sent for 6.1 to avoid a deprecation process in the future. [1] https://lore.kernel.org/qemu-devel/ypwsthpiza3mf...@redhat.com/ v2->v3: - improve the error message - v2: https://lore.kernel.org/qemu-devel/20210722154326.1464-1-wangyana...@huawei.com/ v1->v2: - move the check to machine_set_smp - update qemu-option.hx - v1: https://lore.kernel.org/qemu-devel/20210722021512.2600-1-wangyana...@huawei.com/ Yanan Wang (1): machine: Disallow specifying topology parameters as zero hw/core/machine.c | 15 +++ qapi/machine.json | 6 +++--- qemu-options.hx | 12 +++- 3 files changed, 25 insertions(+), 8 deletions(-) -- 2.19.1
Re: [PATCH for-6.1 v2] machine: Disallow specifying topology parameters as zero
Hi Cleber, On 2021/7/23 6:25, Cleber Rosa wrote: Yanan Wang writes: In the SMP configuration, we should either specify a topology parameter with a reasonable value (equal to or greater than 1) or just leave it omitted and QEMU will calculate its value. Configurations which explicitly specify the topology parameters as zero like "sockets=0" are meaningless, so disallow them. However, the commit 1e63fe685804d (machine: pass QAPI struct to mc->smp_parse) has documented that '0' has the same semantics as omitting a parameter in the qapi comment for SMPConfiguration. So this patch fixes the doc and also adds the corresponding sanity check in the smp parsers. Suggested-by: Andrew Jones Signed-off-by: Yanan Wang --- hw/core/machine.c | 14 ++ qapi/machine.json | 6 +++--- qemu-options.hx | 12 +++- 3 files changed, 24 insertions(+), 8 deletions(-) Hi Yanan, This looks somewhat similar to this very old patch of mine: https://mail.gnu.org/archive/html/qemu-devel/2020-10/msg03039.html I'm putting a reference here because I believe the test can be salvaged and slightly adapted for this patch of yours. Let me know if I can help anyhow. Thanks for this. I was introducing an unit test for the smp parsing in [1], in which all possible valid and invalid smp configs were covered, and actually the "parameter=0" stuff was also covered. You can have a look, and suggestions are welcome. I'm not sure we need two different tests for the same part. :) [1] https://lore.kernel.org/qemu-devel/20210719032043.25416-12-wangyana...@huawei.com/ Thanks, Yanan .
Re: [PATCH v2 11/22] target/loongarch: Add fixed point atomic instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
+#define TRANS_AM_W(name, op) \
+static bool trans_ ## name(DisasContext *ctx, arg_ ## name * a) \
+{ \
+TCGv addr, val, ret; \
+TCGv Rd = cpu_gpr[a->rd]; \
+int mem_idx = ctx->mem_idx; \
+ \
+if (a->rd == 0) { \
+return true; \
+} \
+if ((a->rd != 0) && ((a->rj == a->rd) || (a->rk == a->rd))) { \
+printf("%s: warning, register equal\n", __func__);\
+return false; \
+} \
+ \
+addr = get_gpr(a->rj);\
+val = get_gpr(a->rk); \
+ret = tcg_temp_new(); \
+ \
+tcg_gen_atomic_##op##_tl(ret, addr, val, mem_idx, MO_TESL | \
+ctx->default_tcg_memop_mask); \
+tcg_gen_mov_tl(Rd, ret); \
+ \
+tcg_temp_free(ret); \
+ \
+return true; \
+}
No printf. Use a common routine instead of macros.
r~
Re: [PATCH v2 10/22] target/loongarch: Add fixed point load/store instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
This patch implement fixed point load/store instruction translation.
This includes:
- LD.{B[U]/H[U]/W[U]/D}, ST.{B/H/W/D}
- LDX.{B[U]/H[U]/W[U]/D}, STX.{B/H/W/D}
- LDPTR.{W/D}, STPTR.{W/D}
- PRELD
- LD{GT/LE}.{B/H/W/D}, ST{GT/LE}.{B/H/W/D}
- DBAR, IBAR
Signed-off-by: Song Gao
---
target/loongarch/helper.h | 3 +
target/loongarch/insns.decode | 58
target/loongarch/op_helper.c | 15 +
target/loongarch/trans.inc.c | 758 ++
target/loongarch/translate.c | 29 ++
5 files changed, 863 insertions(+)
diff --git a/target/loongarch/helper.h b/target/loongarch/helper.h
index bbbcc26..5cd38c8 100644
--- a/target/loongarch/helper.h
+++ b/target/loongarch/helper.h
@@ -18,3 +18,6 @@ DEF_HELPER_2(bitrev_d, tl, env, tl)
DEF_HELPER_FLAGS_1(loongarch_bitswap, TCG_CALL_NO_RWG_SE, tl, tl)
DEF_HELPER_FLAGS_1(loongarch_dbitswap, TCG_CALL_NO_RWG_SE, tl, tl)
+
+DEF_HELPER_3(asrtle_d, void, env, tl, tl)
+DEF_HELPER_3(asrtgt_d, void, env, tl, tl)
diff --git a/target/loongarch/insns.decode b/target/loongarch/insns.decode
index ec599a9..08fd232 100644
--- a/target/loongarch/insns.decode
+++ b/target/loongarch/insns.decode
@@ -24,6 +24,9 @@
%lsbw10:5
%msbd16:6
%lsbd10:6
+%si1410:s14
+%hint0:5
+%whint 0:15
#
# Argument sets
@@ -40,6 +43,9 @@
_rdrjrksa3 rd rj rk sa3
_rdrjmsbwlsbw rd rj msbw lsbw
_rdrjmsbdlsbd rd rj msbd lsbd
+_rdrjsi14 rd rj si14
+_hintrjsi12 hint rj si12
+_whint whint
#
# Formats
@@ -56,6 +62,9 @@
@fmt_rdrjmsbwlsbw ... . . . . .
_rdrjmsbwlsbw %rd %rj %msbw %lsbw
@fmt_rdrjmsbdlsbd .. .. .. . .
_rdrjmsbdlsbd %rd %rj %msbd %lsbd
@fmt_rdrjrksa3 .. ... . . . _rdrjrksa3
%rd %rj %rk %sa3
+@fmt_hintrjsi12 .. . . _hintrjsi12
%hint %rj %si12
+@fmt_whint . ... _whint
%whint
+@fmt_rdrjsi14 .. . . _rdrjsi14
%rd %rj %si14
#
# Fixed point arithmetic operation instruction
@@ -158,3 +167,52 @@ bstrins_w 011 . 0 . . .
@fmt_rdrjmsbwlsbw
bstrpick_w 011 . 1 . . . @fmt_rdrjmsbwlsbw
bstrins_d 10 .. .. . .@fmt_rdrjmsbdlsbd
bstrpick_d 11 .. .. . .@fmt_rdrjmsbdlsbd
+
+#
+# Fixed point load/store instruction
+#
+ld_b 0010 10 . . @fmt_rdrjsi12
+ld_h 0010 11 . . @fmt_rdrjsi12
+ld_w 0010 100010 . . @fmt_rdrjsi12
+ld_d 0010 100011 . . @fmt_rdrjsi12
+st_b 0010 100100 . . @fmt_rdrjsi12
+st_h 0010 100101 . . @fmt_rdrjsi12
+st_w 0010 100110 . . @fmt_rdrjsi12
+st_d 0010 100111 . . @fmt_rdrjsi12
+ld_bu0010 101000 . . @fmt_rdrjsi12
+ld_hu0010 101001 . . @fmt_rdrjsi12
+ld_wu0010 101010 . . @fmt_rdrjsi12
+ldx_b0011 1000 0 . . .@fmt_rdrjrk
+ldx_h0011 1000 01000 . . .@fmt_rdrjrk
+ldx_w0011 1000 1 . . .@fmt_rdrjrk
+ldx_d0011 1000 11000 . . .@fmt_rdrjrk
+stx_b0011 1001 0 . . .@fmt_rdrjrk
+stx_h0011 1001 01000 . . .@fmt_rdrjrk
+stx_w0011 1001 1 . . .@fmt_rdrjrk
+stx_d0011 1001 11000 . . .@fmt_rdrjrk
+ldx_bu 0011 1010 0 . . .@fmt_rdrjrk
+ldx_hu 0011 1010 01000 . . .@fmt_rdrjrk
+ldx_wu 0011 1010 1 . . .@fmt_rdrjrk
+preld0010 101011 . . @fmt_hintrjsi12
+dbar 0011 1111 00100 ... @fmt_whint
+ibar 0011 1111 00101 ... @fmt_whint
+ldptr_w 0010 0100 .. . . @fmt_rdrjsi14
+stptr_w 0010 0101 .. . . @fmt_rdrjsi14
+ldptr_d 0010 0110 .. . . @fmt_rdrjsi14
+stptr_d 0010 0111 .. . . @fmt_rdrjsi14
+ldgt_b 0011 1111 1 . . .@fmt_rdrjrk
+ldgt_h 0011 1111 10001 . . .@fmt_rdrjrk
+ldgt_w 0011 1111 10010 . . .@fmt_rdrjrk
+ldgt_d 0011 1111 10011 . . .@fmt_rdrjrk
+ldle_b
Re: [PATCH v2 09/22] target/loongarch: Add fixed point bit instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
This patch implement fixed point bit instruction translation.
This includes:
- EXT.W.{B/H}
- CL{O/Z}.{W/D}, CT{O/Z}.{W/D}
- BYTEPICK.{W/D}
- REVB.{2H/4H/2W/D}
- REVH.{2W/D}
- BITREV.{4B/8B}, BITREV.{W/D}
- BSTRINS.{W/D}, BSTRPICK.{W/D}
- MASKEQZ, MASKNEZ
Signed-off-by: Song Gao
---
target/loongarch/helper.h | 10 +
target/loongarch/insns.decode | 45 +++
target/loongarch/op_helper.c | 119
target/loongarch/trans.inc.c | 665 ++
4 files changed, 839 insertions(+)
diff --git a/target/loongarch/helper.h b/target/loongarch/helper.h
index 6c7e19b..bbbcc26 100644
--- a/target/loongarch/helper.h
+++ b/target/loongarch/helper.h
@@ -8,3 +8,13 @@
DEF_HELPER_3(raise_exception_err, noreturn, env, i32, int)
DEF_HELPER_2(raise_exception, noreturn, env, i32)
+
+DEF_HELPER_2(cto_w, tl, env, tl)
+DEF_HELPER_2(ctz_w, tl, env, tl)
+DEF_HELPER_2(cto_d, tl, env, tl)
+DEF_HELPER_2(ctz_d, tl, env, tl)
The count leading and trailing zero operations are built into tcg. Count leading and
trailing one simply needs a NOT operation to convert it to zero.
+DEF_HELPER_2(bitrev_w, tl, env, tl)
+DEF_HELPER_2(bitrev_d, tl, env, tl)
These should use TCG_CALL_NO_RWG_SE.
+target_ulong helper_bitrev_w(CPULoongArchState *env, target_ulong rj)
+{
+int32_t v = (int32_t)rj;
+const int SIZE = 32;
+uint8_t bytes[SIZE];
+
+int i;
+for (i = 0; i < SIZE; i++) {
+bytes[i] = v & 0x1;
+v = v >> 1;
+}
+/* v == 0 */
+for (i = 0; i < SIZE; i++) {
+v = v | ((uint32_t)bytes[i] << (SIZE - 1 - i));
+}
+
+return (target_ulong)(int32_t)v;
+}
return (int32_t)revbit32(rj);
+target_ulong helper_bitrev_d(CPULoongArchState *env, target_ulong rj)
+{
+uint64_t v = rj;
+const int SIZE = 64;
+uint8_t bytes[SIZE];
+
+int i;
+for (i = 0; i < SIZE; i++) {
+bytes[i] = v & 0x1;
+v = v >> 1;
+}
+/* v == 0 */
+for (i = 0; i < SIZE; i++) {
+v = v | ((uint64_t)bytes[i] << (SIZE - 1 - i));
+}
+
+return (target_ulong)v;
+}
return revbit64(rj);
+static inline target_ulong bitswap(target_ulong v)
+{
+v = ((v >> 1) & (target_ulong)0xULL) |
+((v & (target_ulong)0xULL) << 1);
+v = ((v >> 2) & (target_ulong)0xULL) |
+((v & (target_ulong)0xULL) << 2);
+v = ((v >> 4) & (target_ulong)0x0F0F0F0F0F0F0F0FULL) |
+((v & (target_ulong)0x0F0F0F0F0F0F0F0FULL) << 4);
+return v;
+}
+
+target_ulong helper_loongarch_dbitswap(target_ulong rj)
+{
+return bitswap(rj);
+}
+
+target_ulong helper_loongarch_bitswap(target_ulong rt)
+{
+return (int32_t)bitswap(rt);
+}
I assume these are fpr the bitrev.4b and bitrev.8b insns?
It would be better to name them correctly.
+/* Fixed point bit operation instruction translation */
+static bool trans_ext_w_h(DisasContext *ctx, arg_ext_w_h *a)
+{
+TCGv t0;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = get_gpr(a->rj);
+
+tcg_gen_ext16s_tl(Rd, t0);
Again, you should have a common routine for handling these unary operations.
+static bool trans_clo_w(DisasContext *ctx, arg_clo_w *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+gen_load_gpr(Rd, a->rj);
+
+tcg_gen_not_tl(Rd, Rd);
+tcg_gen_ext32u_tl(Rd, Rd);
+tcg_gen_clzi_tl(Rd, Rd, TARGET_LONG_BITS);
+tcg_gen_subi_tl(Rd, Rd, TARGET_LONG_BITS - 32);
So, you're actually using the tcg builtins here, and the helper you created
isn't used.
+static bool trans_cto_w(DisasContext *ctx, arg_cto_w *a)
+{
+TCGv t0;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+gen_load_gpr(t0, a->rj);
+
+gen_helper_cto_w(Rd, cpu_env, t0);
Here you should have used the tcg builtin.
+static bool trans_ctz_w(DisasContext *ctx, arg_ctz_w *a)
+{
+TCGv t0;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+gen_load_gpr(t0, a->rj);
+
+gen_helper_ctz_w(Rd, cpu_env, t0);
Likewise.
+static bool trans_revb_2w(DisasContext *ctx, arg_revb_2w *a)
+{
+TCGv_i64 t0, t1, t2;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new_i64();
+t1 = tcg_temp_new_i64();
+t2 = get_gpr(a->rj);
+
+gen_load_gpr(t0, a->rd);
+
+tcg_gen_ext32u_i64(t1, t2);
+tcg_gen_bswap32_i64(t0, t1);
+tcg_gen_shri_i64(t1, t2, 32);
+tcg_gen_bswap32_i64(t1, t1);
+tcg_gen_concat32_i64(Rd, t0, t1);
tcg_gen_bswap64_i64(Rd, Rj)
tcg_gen_rotri_i64(Rd, Rd, 32);
+static bool trans_bytepick_d(DisasContext *ctx, arg_bytepick_d *a)
+{
+TCGv t0;
+TCGv Rd
Re: [PATCH v2 08/22] target/loongarch: Add fixed point shift instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
+/* Fixed point shift operation instruction translation */
+static bool trans_sll_w(DisasContext *ctx, arg_sll_w *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+t1 = get_gpr(a->rj);
+
+gen_load_gpr(t0, a->rk);
+
+tcg_gen_andi_tl(t0, t0, 0x1f);
+tcg_gen_shl_tl(t0, t1, t0);
+tcg_gen_ext32s_tl(Rd, t0);
+
+tcg_temp_free(t0);
+
+return true;
+}
Again, you should be using common helper functions for this instead of replicating the
same pattern 16 times.
r~
Re: [PATCH v2 07/22] target/loongarch: Add fixed point arithmetic instruction translation
On 7/20/21 11:53 PM, Song Gao wrote:
+/* Fixed point arithmetic operation instruction translation */
+static bool trans_add_w(DisasContext *ctx, arg_add_w *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+TCGv Rj = cpu_gpr[a->rj];
+TCGv Rk = cpu_gpr[a->rk];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+if (a->rj != 0 && a->rk != 0) {
+tcg_gen_add_tl(Rd, Rj, Rk);
+tcg_gen_ext32s_tl(Rd, Rd);
+} else if (a->rj == 0 && a->rk != 0) {
+tcg_gen_mov_tl(Rd, Rk);
+} else if (a->rj != 0 && a->rk == 0) {
+tcg_gen_mov_tl(Rd, Rj);
+} else {
+tcg_gen_movi_tl(Rd, 0);
+}
+
+return true;
+}
Do not do all of this "if reg(n) zero" testing.
Use a common function to perform the gpr lookup, and a small callback function for the
operation. Often, the callback function already exists within include/tcg/tcg-op.h.
Please see my riscv cleanup patch set I referenced vs patch 6.
+static bool trans_orn(DisasContext *ctx, arg_orn *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+TCGv Rj = cpu_gpr[a->rj];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+TCGv t0 = tcg_temp_new();
+gen_load_gpr(t0, a->rk);
+
+tcg_gen_not_tl(t0, t0);
+tcg_gen_or_tl(Rd, Rj, t0);
tcg_gen_orc_tl.
+static bool trans_andn(DisasContext *ctx, arg_andn *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+TCGv Rj = cpu_gpr[a->rj];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+TCGv t0 = tcg_temp_new();
+gen_load_gpr(t0, a->rk);
+
+tcg_gen_not_tl(t0, t0);
+tcg_gen_and_tl(Rd, Rj, t0);
tcg_gen_andc_tl.
+static bool trans_mul_d(DisasContext *ctx, arg_mul_d *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = get_gpr(a->rj);
+t1 = get_gpr(a->rk);
+
+check_loongarch_64(ctx);
Architecture checks go first, before you've decided the operation is a nop.
+static bool trans_mulh_d(DisasContext *ctx, arg_mulh_d *a)
+{
+TCGv t0, t1, t2;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = get_gpr(a->rj);
+t1 = get_gpr(a->rk);
+t2 = tcg_temp_new();
+
+check_loongarch_64(ctx);
+tcg_gen_muls2_i64(t2, Rd, t0, t1);
If you actually supported LA32, you'd notice this doesn't compile. Are you planning to
support LA32 in the future?
+static bool trans_lu32i_d(DisasContext *ctx, arg_lu32i_d *a)
+{
+TCGv_i64 t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new_i64();
+t1 = tcg_temp_new_i64();
+
+tcg_gen_movi_tl(t0, a->si20);
+tcg_gen_concat_tl_i64(t1, Rd, t0);
+tcg_gen_mov_tl(Rd, t1);
Hmm. Better as
tcg_gen_deposit_tl(Rd, Rd, tcg_constant_tl(a->si20), 32, 32);
+static bool trans_lu52i_d(DisasContext *ctx, arg_lu52i_d *a)
+{
+TCGv t0, t1;
+TCGv Rd = cpu_gpr[a->rd];
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+t0 = tcg_temp_new();
+t1 = tcg_temp_new();
+
+gen_load_gpr(t1, a->rj);
+
+tcg_gen_movi_tl(t0, a->si12);
+tcg_gen_shli_tl(t0, t0, 52);
+tcg_gen_andi_tl(t1, t1, 0xfU);
+tcg_gen_or_tl(Rd, t0, t1);
Definitely better as
tcg_gen_deposit_tl(Rd, Rd, tcg_constant_tl(a->si12), 52, 12);
+static bool trans_addi_w(DisasContext *ctx, arg_addi_w *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+TCGv Rj = cpu_gpr[a->rj];
+target_ulong uimm = (target_long)(a->si12);
+
+if (a->rd == 0) {
+/* Nop */
+return true;
+}
+
+if (a->rj != 0) {
+tcg_gen_addi_tl(Rd, Rj, uimm);
+tcg_gen_ext32s_tl(Rd, Rd);
+} else {
+tcg_gen_movi_tl(Rd, uimm);
+}
+
+return true;
+}
Again, there should be a common function for all of the two-register-immediate operations.
The callback here is exactly the same as for trans_add_w.
+static bool trans_xori(DisasContext *ctx, arg_xori *a)
+{
+TCGv Rd = cpu_gpr[a->rd];
+TCGv Rj = cpu_gpr[a->rj];
+
+target_ulong uimm = (uint16_t)(a->ui12);
You shouldn't need these sorts of casts.
r~
Re: [PATCH v2 06/22] target/loongarch: Add main translation routines
On 7/20/21 11:53 PM, Song Gao wrote:
+/* General purpose registers moves. */
+void gen_load_gpr(TCGv t, int reg)
+{
+if (reg == 0) {
+tcg_gen_movi_tl(t, 0);
+} else {
+tcg_gen_mov_tl(t, cpu_gpr[reg]);
+}
+}
Please have a look at
https://patchew.org/QEMU/20210709042608.883256-1-richard.hender...@linaro.org/
for a better way to handle the zero register.
+static inline void save_cpu_state(DisasContext *ctx, int do_save_pc)
+{
+if (do_save_pc && ctx->base.pc_next != ctx->saved_pc) {
+gen_save_pc(ctx->base.pc_next);
+ctx->saved_pc = ctx->base.pc_next;
+}
+if (ctx->hflags != ctx->saved_hflags) {
+tcg_gen_movi_i32(hflags, ctx->hflags);
+ctx->saved_hflags = ctx->hflags;
+switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
+case LOONGARCH_HFLAG_BR:
+break;
+case LOONGARCH_HFLAG_BC:
+case LOONGARCH_HFLAG_B:
+tcg_gen_movi_tl(btarget, ctx->btarget);
+break;
+}
+}
+}
Drop all the hflags handling.
It's all copied from mips delay slot handling.
+
+static inline void restore_cpu_state(CPULoongArchState *env, DisasContext *ctx)
+{
+ctx->saved_hflags = ctx->hflags;
+switch (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
+case LOONGARCH_HFLAG_BR:
+break;
+case LOONGARCH_HFLAG_BC:
+case LOONGARCH_HFLAG_B:
+ctx->btarget = env->btarget;
+break;
+}
+}
Likewise.
+static void gen_load_fpr32h(TCGv_i32 t, int reg)
+{
+tcg_gen_extrh_i64_i32(t, fpu_f64[reg]);
+}
+
+static void gen_store_fpr32h(TCGv_i32 t, int reg)
+{
+TCGv_i64 t64 = tcg_temp_new_i64();
+tcg_gen_extu_i32_i64(t64, t);
+tcg_gen_deposit_i64(fpu_f64[reg], fpu_f64[reg], t64, 32, 32);
+tcg_temp_free_i64(t64);
+}
There is no general-purpose high-part fpr stuff. There's only movgr2frh and movfrh2gr,
and you can simplify both if you drop the transition through TCGv_i32.
+void gen_op_addr_add(TCGv ret, TCGv arg0, TCGv arg1)
+{
+tcg_gen_add_tl(ret, arg0, arg1);
+}
No point in this, since loongarch has no 32-bit address mode.
+void gen_base_offset_addr(TCGv addr, int base, int offset)
+{
+if (base == 0) {
+tcg_gen_movi_tl(addr, offset);
+} else if (offset == 0) {
+gen_load_gpr(addr, base);
+} else {
+tcg_gen_movi_tl(addr, offset);
+gen_op_addr_add(addr, cpu_gpr[base], addr);
+}
+}
Using the interfaces I quote above from my riscv cleanup,
this can be tidied to
tcg_gen_addi_tl(addr, gpr_src(base), offset);
+static inline bool use_goto_tb(DisasContext *ctx, target_ulong dest)
+{
+return true;
+}
You must now use translate_use_goto_tb, which will not always return true. You will see
assertion failures otherwise.
+static inline void clear_branch_hflags(DisasContext *ctx)
+{
+ctx->hflags &= ~LOONGARCH_HFLAG_BMASK;
+if (ctx->base.is_jmp == DISAS_NEXT) {
+save_cpu_state(ctx, 0);
+} else {
+/*
+ * It is not safe to save ctx->hflags as hflags may be changed
+ * in execution time.
+ */
+tcg_gen_andi_i32(hflags, hflags, ~LOONGARCH_HFLAG_BMASK);
+}
+}
Not required.
+static void gen_branch(DisasContext *ctx, int insn_bytes)
+{
+if (ctx->hflags & LOONGARCH_HFLAG_BMASK) {
+int proc_hflags = ctx->hflags & LOONGARCH_HFLAG_BMASK;
+/* Branches completion */
+clear_branch_hflags(ctx);
+ctx->base.is_jmp = DISAS_NORETURN;
+switch (proc_hflags & LOONGARCH_HFLAG_BMASK) {
+case LOONGARCH_HFLAG_B:
+/* unconditional branch */
+gen_goto_tb(ctx, 0, ctx->btarget);
+break;
+case LOONGARCH_HFLAG_BC:
+/* Conditional branch */
+{
+TCGLabel *l1 = gen_new_label();
+
+tcg_gen_brcondi_tl(TCG_COND_NE, bcond, 0, l1);
+gen_goto_tb(ctx, 1, ctx->base.pc_next + insn_bytes);
+gen_set_label(l1);
+gen_goto_tb(ctx, 0, ctx->btarget);
+}
+break;
+case LOONGARCH_HFLAG_BR:
+/* unconditional branch to register */
+tcg_gen_mov_tl(cpu_PC, btarget);
+tcg_gen_lookup_and_goto_ptr();
+break;
+default:
+fprintf(stderr, "unknown branch 0x%x\n", proc_hflags);
+abort();
+}
+}
+}
Split this up into the various trans_* branch routines, without the setting of
HFLAG.
+static void loongarch_tr_init_disas_context(DisasContextBase *dcbase,
+CPUState *cs)
+{
+DisasContext *ctx = container_of(dcbase, DisasContext, base);
+CPULoongArchState *env = cs->env_ptr;
+
+ctx->page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
+ctx->saved_pc = -1;
+ctx->btarget = 0;
+/* Restore state from the tb context. */
+ctx->hflags = (uint32_t)ctx->base.tb->flags;
+restore_cpu_state(env, ctx);
+
Re: [PATCH v2 05/22] target/loongarch: Add memory management support
On 7/20/21 11:53 PM, Song Gao wrote:
This patch introduces one memory-management-related functions
- loongarch_cpu_tlb_fill()
Signed-off-by: Song Gao
---
target/loongarch/cpu.c| 1 +
target/loongarch/cpu.h| 9
target/loongarch/tlb_helper.c | 103 ++
3 files changed, 113 insertions(+)
create mode 100644 target/loongarch/tlb_helper.c
diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index 8eaa778..6269dd9 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -269,6 +269,7 @@ static struct TCGCPUOps loongarch_tcg_ops = {
.initialize = loongarch_tcg_init,
.synchronize_from_tb = loongarch_cpu_synchronize_from_tb,
.cpu_exec_interrupt = loongarch_cpu_exec_interrupt,
+.tlb_fill = loongarch_cpu_tlb_fill,
};
#endif /* CONFIG_TCG */
diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h
index 1db8bb5..5c06122 100644
--- a/target/loongarch/cpu.h
+++ b/target/loongarch/cpu.h
@@ -287,4 +287,13 @@ static inline void compute_hflags(CPULoongArchState *env)
const char *loongarch_exception_name(int32_t exception);
+/* tlb_helper.c */
+bool loongarch_cpu_tlb_fill(CPUState *cs,
+vaddr address,
+int size,
+MMUAccessType access_type,
+int mmu_idx,
+bool probe,
+uintptr_t retaddr);
+
#endif /* LOONGARCH_CPU_H */
diff --git a/target/loongarch/tlb_helper.c b/target/loongarch/tlb_helper.c
new file mode 100644
index 000..b59a995
--- /dev/null
+++ b/target/loongarch/tlb_helper.c
@@ -0,0 +1,103 @@
+/*
+ * LoongArch tlb emulation helpers for qemu.
+ *
+ * Copyright (c) 2021 Loongson Technology Corporation Limited
+ *
+ * SPDX-License-Identifier: LGPL-2.1+
+ */
+
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "cpu-csr.h"
+#include "exec/helper-proto.h"
+#include "exec/exec-all.h"
+#include "exec/cpu_ldst.h"
+#include "exec/log.h"
+
+enum {
+TLBRET_PE = -7,
+TLBRET_XI = -6,
+TLBRET_RI = -5,
+TLBRET_DIRTY = -4,
+TLBRET_INVALID = -3,
+TLBRET_NOMATCH = -2,
+TLBRET_BADADDR = -1,
+TLBRET_MATCH = 0
+};
+
+static void raise_mmu_exception(CPULoongArchState *env, target_ulong address,
+MMUAccessType access_type, int tlb_error)
+{
+CPUState *cs = env_cpu(env);
+int exception = 0, error_code = 0;
+
+if (access_type == MMU_INST_FETCH) {
+error_code |= INST_INAVAIL;
+}
+
+switch (tlb_error) {
+default:
+case TLBRET_BADADDR:
+exception = EXCP_ADE;
+break;
+case TLBRET_NOMATCH:
+/* No TLB match for a mapped address */
+if (access_type == MMU_DATA_STORE) {
+exception = EXCP_TLBS;
+} else {
+exception = EXCP_TLBL;
+}
+error_code |= TLB_NOMATCH;
+break;
+case TLBRET_INVALID:
+/* TLB match with no valid bit */
+if (access_type == MMU_DATA_STORE) {
+exception = EXCP_TLBS;
+} else {
+exception = EXCP_TLBL;
+}
+break;
+case TLBRET_DIRTY:
+exception = EXCP_TLBM;
+break;
+case TLBRET_XI:
+/* Execute-Inhibit Exception */
+exception = EXCP_TLBXI;
+break;
+case TLBRET_RI:
+/* Read-Inhibit Exception */
+exception = EXCP_TLBRI;
+break;
+case TLBRET_PE:
+/* Privileged Exception */
+exception = EXCP_TLBPE;
+break;
+}
+
+if (tlb_error == TLBRET_NOMATCH) {
+env->CSR_TLBRBADV = address;
+env->CSR_TLBREHI = address & (TARGET_PAGE_MASK << 1);
+cs->exception_index = exception;
+env->error_code = error_code;
+return;
+}
+
+/* Raise exception */
+env->CSR_BADV = address;
+cs->exception_index = exception;
+env->error_code = error_code;
+env->CSR_TLBEHI = address & (TARGET_PAGE_MASK << 1);
+}
+
+bool loongarch_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
+ MMUAccessType access_type, int mmu_idx,
+ bool probe, uintptr_t retaddr)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+CPULoongArchState *env = >env;
+int ret = TLBRET_BADADDR;
+
+/* data access */
+raise_mmu_exception(env, address, access_type, ret);
+do_raise_exception_err(env, cs->exception_index, env->error_code, retaddr);
+}
Again, almost all of this does not apply for user-only.
r~
Re: [PATCH v2 04/22] target/loongarch: Add interrupt handling support
On 7/20/21 11:53 PM, Song Gao wrote:
+bool loongarch_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
+{
+if (interrupt_request & CPU_INTERRUPT_HARD) {
+LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+CPULoongArchState *env = >env;
+
+if (cpu_loongarch_hw_interrupts_enabled(env) &&
+cpu_loongarch_hw_interrupts_pending(env)) {
+cs->exception_index = EXCP_INTE;
+env->error_code = 0;
+loongarch_cpu_do_interrupt(cs);
+return true;
+}
+}
+return false;
+}
Not sure what you're doing here, with user-only. None of these conditions
apply.
r~
Re: [PATCH v2 03/22] target/loongarch: Add core definition
On 7/20/21 11:52 PM, Song Gao wrote:
This patch add target state header, target definitions
and initialization routines.
Signed-off-by: Song Gao
---
target/loongarch/cpu-param.h | 21
target/loongarch/cpu-qom.h | 40 ++
target/loongarch/cpu.c | 293 +++
target/loongarch/cpu.h | 265 ++
4 files changed, 619 insertions(+)
create mode 100644 target/loongarch/cpu-param.h
create mode 100644 target/loongarch/cpu-qom.h
create mode 100644 target/loongarch/cpu.c
create mode 100644 target/loongarch/cpu.h
diff --git a/target/loongarch/cpu-param.h b/target/loongarch/cpu-param.h
new file mode 100644
index 000..582ee29
--- /dev/null
+++ b/target/loongarch/cpu-param.h
@@ -0,0 +1,21 @@
+/*
+ * LoongArch cpu parameters for qemu.
+ *
+ * Copyright (c) 2021 Loongson Technology Corporation Limited
+ *
+ * SPDX-License-Identifier: LGPL-2.1+
+ */
+
+#ifndef LOONGARCH_CPU_PARAM_H
+#define LOONGARCH_CPU_PARAM_H 1
+
+#ifdef TARGET_LOONGARCH64
+#define TARGET_LONG_BITS 64
Why the ifdef for TARGET_LOONGARCH64?
Nothing will compile without that set.
+#ifdef CONFIG_TCG
+static void loongarch_cpu_synchronize_from_tb(CPUState *cs,
+ const TranslationBlock *tb)
+{
+LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+CPULoongArchState *env = >env;
+
+env->active_tc.PC = tb->pc;
+env->hflags &= ~LOONGARCH_HFLAG_BMASK;
+env->hflags |= tb->flags & LOONGARCH_HFLAG_BMASK;
+}
Loongarch has no branch delay slots, so you should not have replicated the mips branch
delay slot handling. There should be no BMASK at all.
+#ifdef CONFIG_TCG
+#include "hw/core/tcg-cpu-ops.h"
+
+static struct TCGCPUOps loongarch_tcg_ops = {
+.initialize = loongarch_tcg_init,
+.synchronize_from_tb = loongarch_cpu_synchronize_from_tb,
+};
+#endif /* CONFIG_TCG */
May I presume that Loongarch has virtualization hardware, and will eventually support KVM?
If not, there is no need for CONFIG_TCG anywhere.
+#define TCG_GUEST_DEFAULT_MO (0)
+#define UNASSIGNED_CPU_ID 0x
+
+typedef union fpr_t fpr_t;
+union fpr_t {
+float64 fd; /* ieee double precision */
+float32 fs[2];/* ieee single precision */
+uint64_t d;/* binary double fixed-point */
+uint32_t w[2]; /* binary single fixed-point */
+};
For what it's worth, we already have a CPU_DoubleU type that could be used. But frankly,
float64 *is* uint64_t, so there's very little use in putting them together into a union.
It would seem that you don't even use fs and w for more than fpu_dump_state, and you're
even doing it wrong there.
+typedef struct CPULoongArchFPUContext CPULoongArchFPUContext;
+struct CPULoongArchFPUContext {
+/* Floating point registers */
+fpr_t fpr[32];
+float_status fp_status;
+
+bool cf[8];
+/*
+ * fcsr0
+ * 31:29 |28:24 |23:21 |20:16 |15:10 |9:8 |7 |6 |5 |4:0
+ *Cause Flags RM DAE TM Enables
+ */
+uint32_t fcsr0;
+uint32_t fcsr0_mask;
+uint32_t vcsr16;
+
+#define FCSR0_M10xdf /* FCSR1 mask, DAE, TM and Enables */
+#define FCSR0_M20x1f1f /* FCSR2 mask, Cause and Flags */
+#define FCSR0_M30x300/* FCSR3 mask, Round Mode */
+#define FCSR0_RM8/* Round Mode bit num on fcsr0 */
+#define GET_FP_CAUSE(reg)(((reg) >> 24) & 0x1f)
+#define GET_FP_ENABLE(reg) (((reg) >> 0) & 0x1f)
+#define GET_FP_FLAGS(reg)(((reg) >> 16) & 0x1f)
+#define SET_FP_CAUSE(reg, v) do { (reg) = ((reg) & ~(0x1f << 24)) | \
+ ((v & 0x1f) << 24); \
+ } while (0)
+#define SET_FP_ENABLE(reg, v) do { (reg) = ((reg) & ~(0x1f << 0)) | \
+ ((v & 0x1f) << 0);\
+ } while (0)
+#define SET_FP_FLAGS(reg, v) do { (reg) = ((reg) & ~(0x1f << 16)) | \
+ ((v & 0x1f) << 16); \
+ } while (0)
+#define UPDATE_FP_FLAGS(reg, v) do { (reg) |= ((v & 0x1f) << 16); } while (0)
+#define FP_INEXACT1
+#define FP_UNDERFLOW 2
+#define FP_OVERFLOW 4
+#define FP_DIV0 8
+#define FP_INVALID16
+};
+
+#define TARGET_INSN_START_EXTRA_WORDS 2
+#define LOONGARCH_FPU_MAX 1
+#define N_IRQS 14
+
+enum loongarch_feature {
+LA_FEATURE_3A5000,
+};
+
+typedef struct TCState TCState;
+struct TCState {
+target_ulong gpr[32];
+target_ulong PC;
+};
+
+typedef struct CPULoongArchState CPULoongArchState;
+struct CPULoongArchState {
+TCState active_tc;
+CPULoongArchFPUContext active_fpu;
Please don't replicate the mips foolishness with active_tc and active_fpu. There is no
inactive_fpu with which to contrast this. Just include these fields directly into the
main
Re: [PATCH for-6.1 v2] machine: Disallow specifying topology parameters as zero
Yanan Wang writes: > In the SMP configuration, we should either specify a topology > parameter with a reasonable value (equal to or greater than 1) > or just leave it omitted and QEMU will calculate its value. > Configurations which explicitly specify the topology parameters > as zero like "sockets=0" are meaningless, so disallow them. > > However, the commit 1e63fe685804d > (machine: pass QAPI struct to mc->smp_parse) has documented that > '0' has the same semantics as omitting a parameter in the qapi > comment for SMPConfiguration. So this patch fixes the doc and > also adds the corresponding sanity check in the smp parsers. > > Suggested-by: Andrew Jones > Signed-off-by: Yanan Wang > --- > hw/core/machine.c | 14 ++ > qapi/machine.json | 6 +++--- > qemu-options.hx | 12 +++- > 3 files changed, 24 insertions(+), 8 deletions(-) Hi Yanan, This looks somewhat similar to this very old patch of mine: https://mail.gnu.org/archive/html/qemu-devel/2020-10/msg03039.html I'm putting a reference here because I believe the test can be salvaged and slightly adapted for this patch of yours. Let me know if I can help anyhow. Thanks, - Cleber.
Re: [PATCH v2 1/1] hmp: synchronize cpu state for lapic info
May I get feedback for this bugfix?
So far the "info lapic " returns stale data and could not accurate reflect
the status in KVM.
Thank you very much!
Dongli Zhang
On 7/1/21 2:40 PM, Dongli Zhang wrote:
> While the default "info lapic" always synchronizes cpu state ...
>
> mon_get_cpu()
> -> mon_get_cpu_sync(mon, true)
>-> cpu_synchronize_state(cpu)
> -> ioctl KVM_GET_LAPIC (taking KVM as example)
>
> ... the cpu state is not synchronized when the apic-id is available as
> argument.
>
> The cpu state should be synchronized when apic-id is available. Otherwise
> the "info lapic " always returns stale data.
>
> Signed-off-by: Dongli Zhang
> ---
> Changed since v1:
> - I sent out wrong patch version in v1
>
> target/i386/monitor.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 119211f0b0..d833ab5b8e 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -28,6 +28,7 @@
> #include "monitor/hmp-target.h"
> #include "monitor/hmp.h"
> #include "qapi/qmp/qdict.h"
> +#include "sysemu/hw_accel.h"
> #include "sysemu/kvm.h"
> #include "sysemu/sev.h"
> #include "qapi/error.h"
> @@ -656,7 +657,11 @@ void hmp_info_local_apic(Monitor *mon, const QDict
> *qdict)
>
> if (qdict_haskey(qdict, "apic-id")) {
> int id = qdict_get_try_int(qdict, "apic-id", 0);
> +
> cs = cpu_by_arch_id(id);
> +if (cs) {
> +cpu_synchronize_state(cs);
> +}
> } else {
> cs = mon_get_cpu(mon);
> }
>
[PATCH v2 1/1] modules: Improve error message when module is not found
When a module is not found, specially accelerators, QEMU displays
a error message that not easy to understand[1]. This patch improves
the readability by offering a user-friendly message[2].
This patch also moves the accelerator ops check to runtine (instead
of the original g_assert) because it works better with dynamic
modules.
[1] qemu-system-x86_64 -accel tcg
ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed:
(ops != NULL)
Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces:
assertion failed: (ops != NULL)
31964 IOT instruction (core dumped) ./qemu-system-x86_64 ...
[2] qemu-system-x86_64 -accel tcg
accel-tcg-x86_64 module is missing, install the package or config the library
path correctly.
Signed-off-by: Jose R. Ziviani
---
accel/accel-softmmu.c | 5 -
util/module.c | 14 --
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/accel/accel-softmmu.c b/accel/accel-softmmu.c
index 67276e4f52..52449ac2d0 100644
--- a/accel/accel-softmmu.c
+++ b/accel/accel-softmmu.c
@@ -79,7 +79,10 @@ void accel_init_ops_interfaces(AccelClass *ac)
* all accelerators need to define ops, providing at least a mandatory
* non-NULL create_vcpu_thread operation.
*/
-g_assert(ops != NULL);
+if (ops == NULL) {
+exit(1);
+}
+
if (ops->ops_init) {
ops->ops_init(ops);
}
diff --git a/util/module.c b/util/module.c
index 6bb4ad915a..268a8563fd 100644
--- a/util/module.c
+++ b/util/module.c
@@ -206,13 +206,10 @@ static int module_load_file(const char *fname, bool
mayfail, bool export_symbols
out:
return ret;
}
-#endif
bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
{
bool success = false;
-
-#ifdef CONFIG_MODULES
char *fname = NULL;
#ifdef CONFIG_MODULE_UPGRADES
char *version_dir;
@@ -300,6 +297,9 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
if (!success) {
g_hash_table_remove(loaded_modules, module_name);
+fprintf(stderr, "%s module is missing, install the "
+"package or config the library path "
+"correctly.\n", module_name);
g_free(module_name);
}
@@ -307,12 +307,9 @@ bool module_load_one(const char *prefix, const char
*lib_name, bool mayfail)
g_free(dirs[i]);
}
-#endif
return success;
}
-#ifdef CONFIG_MODULES
-
static bool module_loaded_qom_all;
void module_load_qom_one(const char *type)
@@ -384,4 +381,9 @@ void qemu_load_module_for_opts(const char *group) {}
void module_load_qom_one(const char *type) {}
void module_load_qom_all(void) {}
+bool module_load_one(const char *prefix, const char *lib_name, bool mayfail)
+{
+return false;
+}
+
#endif
--
2.32.0
[PATCH v2 0/1] Improve module accelerator error message
v1 -> v2: * Moved the code to module.c * Simplified a lot by using current module DB to get info The main objective is to improve the error message when trying to load a not found/not installed module TCG. For example: $ qemu-system-x86_64 -accel tcg ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL) Bail out! ERROR:../accel/accel-softmmu.c:82:accel_init_ops_interfaces: assertion failed: (ops != NULL) [1]31964 IOT instruction (core dumped) ./qemu-system-x86_64 ... To: $ qemu-system-x86_64 -accel tcg accel-tcg-x86_64 module is missing, install the package or config the library path correctly. Jose R. Ziviani (1): modules: Improve error message when module is not found accel/accel-softmmu.c | 5 - util/module.c | 14 -- 2 files changed, 12 insertions(+), 7 deletions(-) -- 2.32.0
Re: [PATCH-for-6.1 v2] gitlab-ci: Extract OpenSBI job rules and fix 'when' condition
Philippe Mathieu-Daudé writes:
> First, all jobs depending on 'docker-opensbi' job must use at most
> all the rules that triggers it. The simplest way to ensure that is
> to always use the same rules. Extract all the rules to a reusable
> section, and include this section (with the 'extends' keyword) in
> both 'docker-opensbi' and 'build-opensbi' jobs.
>
> Second, jobs depending on another should not use the 'when: always'
> condition, because if a dependency failed we should not keep running
> jobs depending on it. The correct condition is 'when: on_success'.
>
> The problems were introduced in commit c6fc0fc1a71 ("gitlab-ci.yml:
> Add jobs to build OpenSBI firmware binaries"), but were revealed in
> commit 91e9c47e50a ("docker: OpenSBI build job depends on OpenSBI
> container").
>
> This fix is similar to the one used with the EDK2 firmware job in
> commit ac0595cf6b3 ("gitlab-ci: Extract EDK2 job rules to reusable
> section").
>
> Reported-by: Daniel P. Berrangé
> Reviewed-by: Daniel P. Berrangé
> Reviewed-by: Willian Rampazzo
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> v2: when 'always' -> 'on_success' & reworded (danpb)
>
> Supersedes: <20210720164829.3949558-1-phi...@redhat.com>
> ---
> .gitlab-ci.d/opensbi.yml | 30 ++
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
Reviewed-by: Cleber Rosa
Re: [PATCH for-6.1 2/2] docs: Move licence/copyright from HTML output to rST comments
Peter Maydell writes: > Our built HTML documentation now has a standard footer which > gives the license for QEMU (and its documentation as a whole). > In almost all pages, we either don't bother to state the > copyright/license for the individual rST sources, or we put > it in an rST comment. There are just three pages which render > copyright or license information into the user-visible HTML. > > Quoting a specific (different) license for an individual HTML > page within the manual is confusing. Downgrade the license > and copyright info to a comment within the rST source, bringing > these pages in line with the rest of our documents. > > Suggested-by: Markus Armbruster > Signed-off-by: Peter Maydell > --- > docs/interop/vhost-user-gpu.rst | 7 --- > docs/interop/vhost-user.rst | 12 +++- > docs/system/generic-loader.rst | 4 ++-- > 3 files changed, 13 insertions(+), 10 deletions(-) Reviewed-by: Cleber Rosa
Re: [PATCH for-6.1 1/2] docs: Remove stale TODO comments about license and version
Peter Maydell writes: > Since commits 13f934e79fa and 3a50c8f3067aaf, our HTML docs include a > footer to all pages stating the license and version. We can > therefore delete the TODO comments suggesting we should do that from > our .rst files. > > Signed-off-by: Peter Maydell > --- > docs/interop/qemu-ga-ref.rst | 9 - > docs/interop/qemu-qmp-ref.rst| 9 - > docs/interop/qemu-storage-daemon-qmp-ref.rst | 9 - > 3 files changed, 27 deletions(-) > Reviewed-by: Cleber Rosa
Re: [PATCH for-6.1 0/3] docs: Document arm mainstone, kzm, imx25-pdk
On 7/22/21 7:52 AM, Peter Maydell wrote: Peter Maydell (3): docs: Add documentation of Arm 'mainstone' board docs: Add documentation of Arm 'kzm' board docs: Add documentation of Arm 'imx25-pdk' board Reviewed-by: Richard Henderson r~
Re: [PATCH] MAINTAINERS: Don't list Andrzej Zaborowski for various components
On 7/22/21 8:09 AM, Peter Maydell wrote: Andrzej Zaborowski is listed as an "Odd Fixes" maintainer for the nSeries, Palm and PXA2XX boards, as well as the "Maintained" status Arm 32-bit TCG backend. Andrzej's last email to qemu-devel was back in 2017, and the email before that was all the way back in 2013. We don't really need to fill his email up with CCs on QEMU patches any more... Remove Andrzej from the various boards sections (leaving them still Odd Fixes with me as the backup patch reviewer). Add Richard Henderson as the maintainer for the Arm TCG backend, since removing Andrzej would otherwise leave that section with no M: line at all. Signed-off-by: Peter Maydell --- Andrzej: if you're reading this, thanks for all the work you did on QEMU back in the day; and if you do want to still be CCd on patches let me know and I'll happily drop this MAINTAINERS update. Richard: are you happy with (a) being listed for Arm TCG and (b) it being "Maintained" status? Yep. Reviewed-by: Richard Henderson r~
Re: [PATCH for-6.1 0/2] docs license footer followon cleanups
On Thu, Jul 22, 2021 at 11:23 PM Peter Maydell wrote: > This patchset makes a couple of followon cleanups now that > commits 13f934e79fa and 3a50c8f3067aaf are in master and our HTML > documentation has a footer to all pages stating the QEMU license > and version: > * it removes the TODO comments, because we've now done them > * three .rst files were rendering their own copyright or >license information into the user-visible HTML, which is >a bit confusing now that we also do this in the page footer; >patch 2 brings those files into line with the others by having >the comment/license only in a rST comment > > thanks > -- PMM > > Peter Maydell (2): > docs: Remove stale TODO comments about license and version > docs: Move licence/copyright from HTML output to rST comments > Reviewed-by: Marc-André Lureau > docs/interop/qemu-ga-ref.rst | 9 - > docs/interop/qemu-qmp-ref.rst| 9 - > docs/interop/qemu-storage-daemon-qmp-ref.rst | 9 - > docs/interop/vhost-user-gpu.rst | 7 --- > docs/interop/vhost-user.rst | 12 +++- > docs/system/generic-loader.rst | 4 ++-- > 6 files changed, 13 insertions(+), 37 deletions(-) > > -- > 2.20.1 > > > -- Marc-André Lureau
Re: [PATCH v3 2/5] migration: Make from_dst_file accesses thread-safe
On Thu, Jul 22, 2021 at 01:58:38PM -0400, Peter Xu wrote:
> Accessing from_dst_file is potentially racy in current code base like below:
>
> if (s->from_dst_file)
> do_something(s->from_dst_file);
>
> Because from_dst_file can be reset right after the check in another
> thread (rp_thread). One example is migrate_fd_cancel().
>
> Use the same qemu_file_lock to protect it too, just like to_dst_file.
>
> When it's safe to access without lock, comment it.
>
> There's one special reference in migration_thread() that can be replaced by
> the newly introduced rp_thread_created flag.
>
> Reported-by: Dr. David Alan Gilbert
> Reviewed-by: Dr. David Alan Gilbert
> Signed-off-by: Peter Xu
> ---
(Dave should have helped fixing this which I appreciated a lot, but just make
it be together with the record..)
Below needs to be squashed into the patch:
diff --git a/migration/migration.c b/migration/migration.c
index a50330016c..041b8451a6 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2696,7 +2696,7 @@ static void
migration_release_from_dst_file(MigrationState *ms)
{
QEMUFile *file;
-WITH_QEMU_LOCK_GUARD(>qemu_file_lock) {
+WITH_QEMU_LOCK_GUARD(>qemu_file_lock) {
/*
* Reset the from_dst_file pointer first before releasing it, as we
* can't block within lock section
--
Peter Xu
[PATCH for-6.1 0/2] docs license footer followon cleanups
This patchset makes a couple of followon cleanups now that commits 13f934e79fa and 3a50c8f3067aaf are in master and our HTML documentation has a footer to all pages stating the QEMU license and version: * it removes the TODO comments, because we've now done them * three .rst files were rendering their own copyright or license information into the user-visible HTML, which is a bit confusing now that we also do this in the page footer; patch 2 brings those files into line with the others by having the comment/license only in a rST comment thanks -- PMM Peter Maydell (2): docs: Remove stale TODO comments about license and version docs: Move licence/copyright from HTML output to rST comments docs/interop/qemu-ga-ref.rst | 9 - docs/interop/qemu-qmp-ref.rst| 9 - docs/interop/qemu-storage-daemon-qmp-ref.rst | 9 - docs/interop/vhost-user-gpu.rst | 7 --- docs/interop/vhost-user.rst | 12 +++- docs/system/generic-loader.rst | 4 ++-- 6 files changed, 13 insertions(+), 37 deletions(-) -- 2.20.1
[PATCH for-6.1 2/2] docs: Move licence/copyright from HTML output to rST comments
Our built HTML documentation now has a standard footer which gives the license for QEMU (and its documentation as a whole). In almost all pages, we either don't bother to state the copyright/license for the individual rST sources, or we put it in an rST comment. There are just three pages which render copyright or license information into the user-visible HTML. Quoting a specific (different) license for an individual HTML page within the manual is confusing. Downgrade the license and copyright info to a comment within the rST source, bringing these pages in line with the rest of our documents. Suggested-by: Markus Armbruster Signed-off-by: Peter Maydell --- docs/interop/vhost-user-gpu.rst | 7 --- docs/interop/vhost-user.rst | 12 +++- docs/system/generic-loader.rst | 4 ++-- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/docs/interop/vhost-user-gpu.rst b/docs/interop/vhost-user-gpu.rst index 3268bf405ce..71a2c52b313 100644 --- a/docs/interop/vhost-user-gpu.rst +++ b/docs/interop/vhost-user-gpu.rst @@ -2,9 +2,10 @@ Vhost-user-gpu Protocol === -:Licence: This work is licensed under the terms of the GNU GPL, - version 2 or later. See the COPYING file in the top-level - directory. +.. + Licence: This work is licensed under the terms of the GNU GPL, + version 2 or later. See the COPYING file in the top-level + directory. .. contents:: Table of Contents diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index d6085f70452..d63f8d3f93a 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -1,11 +1,13 @@ === Vhost-user Protocol === -:Copyright: 2014 Virtual Open Systems Sarl. -:Copyright: 2019 Intel Corporation -:Licence: This work is licensed under the terms of the GNU GPL, - version 2 or later. See the COPYING file in the top-level - directory. + +.. + Copyright 2014 Virtual Open Systems Sarl. + Copyright 2019 Intel Corporation + Licence: This work is licensed under the terms of the GNU GPL, + version 2 or later. See the COPYING file in the top-level + directory. .. contents:: Table of Contents diff --git a/docs/system/generic-loader.rst b/docs/system/generic-loader.rst index 531ddbc8e34..4f9fb005f1d 100644 --- a/docs/system/generic-loader.rst +++ b/docs/system/generic-loader.rst @@ -1,8 +1,8 @@ .. Copyright (c) 2016, Xilinx Inc. -This work is licensed under the terms of the GNU GPL, version 2 or later. See -the COPYING file in the top-level directory. + This work is licensed under the terms of the GNU GPL, version 2 or later. See + the COPYING file in the top-level directory. Generic Loader -- -- 2.20.1
[PATCH for-6.1 1/2] docs: Remove stale TODO comments about license and version
Since commits 13f934e79fa and 3a50c8f3067aaf, our HTML docs include a footer to all pages stating the license and version. We can therefore delete the TODO comments suggesting we should do that from our .rst files. Signed-off-by: Peter Maydell --- docs/interop/qemu-ga-ref.rst | 9 - docs/interop/qemu-qmp-ref.rst| 9 - docs/interop/qemu-storage-daemon-qmp-ref.rst | 9 - 3 files changed, 27 deletions(-) diff --git a/docs/interop/qemu-ga-ref.rst b/docs/interop/qemu-ga-ref.rst index db1e9461249..032d4924552 100644 --- a/docs/interop/qemu-ga-ref.rst +++ b/docs/interop/qemu-ga-ref.rst @@ -1,15 +1,6 @@ QEMU Guest Agent Protocol Reference === -.. - TODO: the old Texinfo manual used to note that this manual - is GPL-v2-or-later. We should make that reader-visible - both here and in our Sphinx manuals more generally. - -.. - TODO: display the QEMU version, both here and in our Sphinx manuals - more generally. - .. contents:: :depth: 3 diff --git a/docs/interop/qemu-qmp-ref.rst b/docs/interop/qemu-qmp-ref.rst index b5bebf6b9a9..357effd64f3 100644 --- a/docs/interop/qemu-qmp-ref.rst +++ b/docs/interop/qemu-qmp-ref.rst @@ -1,15 +1,6 @@ QEMU QMP Reference Manual = -.. - TODO: the old Texinfo manual used to note that this manual - is GPL-v2-or-later. We should make that reader-visible - both here and in our Sphinx manuals more generally. - -.. - TODO: display the QEMU version, both here and in our Sphinx manuals - more generally. - .. contents:: :depth: 3 diff --git a/docs/interop/qemu-storage-daemon-qmp-ref.rst b/docs/interop/qemu-storage-daemon-qmp-ref.rst index d0ebb42ebd5..9fed68152f5 100644 --- a/docs/interop/qemu-storage-daemon-qmp-ref.rst +++ b/docs/interop/qemu-storage-daemon-qmp-ref.rst @@ -1,15 +1,6 @@ QEMU Storage Daemon QMP Reference Manual -.. - TODO: the old Texinfo manual used to note that this manual - is GPL-v2-or-later. We should make that reader-visible - both here and in our Sphinx manuals more generally. - -.. - TODO: display the QEMU version, both here and in our Sphinx manuals - more generally. - .. contents:: :depth: 3 -- 2.20.1
Re: Prefetches in buffer_zero_*
* Richard Henderson (richard.hender...@linaro.org) wrote: > On 7/22/21 12:02 AM, Dr. David Alan Gilbert wrote: > > Hi Richard, > >I think you were the last person to fiddle with the prefetching > > in buffer_zero_avx2 and friends; Joe (cc'd) wondered if explicit > > prefetching still made sense on modern CPUs, and that their hardware > > generally figures stuff out better on simple increments. > > > >What was your thinking on this, and did you actually measure > > any improvement? > > Ah, well, that was 5 years ago so I have no particular memory of this. It > wouldn't surprise me if you can't measure any improvement on modern > hardware. > > Do you now measure an improvement with the prefetches gone? Not tried, it just came from Joe's suggestion that it was generally a bad idea these days; I do remember that the behaviour of those functions is quite tricky because there performance is VERY data dependent - many VMs actually have pages that are quite dirty so you never iterate the loop, but then you hit others with big zero pages and you spend your entire life in the loop. Dave > > r~ > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH 1/1] hw/i2c: add remote I2C device
This patch adds the remote I2C device, which supports the usage of
external I2C devices.
Signed-off-by: Shengtan Mao
---
hw/arm/Kconfig| 1 +
hw/i2c/Kconfig| 4 +
hw/i2c/meson.build| 1 +
hw/i2c/remote-i2c.c | 117 ++
tests/qtest/meson.build | 1 +
tests/qtest/remote-i2c-test.c | 216 ++
6 files changed, 340 insertions(+)
create mode 100644 hw/i2c/remote-i2c.c
create mode 100644 tests/qtest/remote-i2c-test.c
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 90b19c0861..58fdfab90d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -392,6 +392,7 @@ config NPCM7XX
select MAX34451
select PL310 # cache controller
select PMBUS
+select REMOTE_I2C
select SERIAL
select SSI
select UNIMP
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 8217cb5041..278156991d 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -1,6 +1,10 @@
config I2C
bool
+config REMOTE_I2C
+bool
+select I2C
+
config SMBUS
bool
select I2C
diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build
index d3df273251..ba0215db61 100644
--- a/hw/i2c/meson.build
+++ b/hw/i2c/meson.build
@@ -6,6 +6,7 @@ i2c_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true:
files('smbus_ich9.c'))
i2c_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_i2c.c'))
i2c_ss.add(when: 'CONFIG_BITBANG_I2C', if_true: files('bitbang_i2c.c'))
i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c'))
+i2c_ss.add(when: 'CONFIG_REMOTE_I2C', if_true: files('remote-i2c.c'))
i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c'))
i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c'))
i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c'))
diff --git a/hw/i2c/remote-i2c.c b/hw/i2c/remote-i2c.c
new file mode 100644
index 00..69be80fd3c
--- /dev/null
+++ b/hw/i2c/remote-i2c.c
@@ -0,0 +1,117 @@
+/*
+ * Remote I2C controller
+ *
+ * Copyright (c) 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+
+#include "chardev/char-fe.h"
+#include "hw/i2c/i2c.h"
+#include "hw/qdev-properties-system.h"
+
+#define TYPE_REMOTE_I2C "remote-i2c"
+#define REMOTE_I2C(obj) OBJECT_CHECK(RemoteI2CState, (obj), TYPE_REMOTE_I2C)
+#define ONE_BYTE 1
+
+typedef struct {
+I2CSlave parent_obj;
+CharBackend chr;
+} RemoteI2CState;
+
+typedef enum {
+REMOTE_I2C_START_RECV = 0,
+REMOTE_I2C_START_SEND = 1,
+REMOTE_I2C_FINISH = 2,
+REMOTE_I2C_NACK = 3,
+REMOTE_I2C_RECV = 4,
+REMOTE_I2C_SEND = 5,
+} RemoteI2CCommand;
+
+static uint8_t remote_i2c_recv(I2CSlave *s)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t resp = 0;
+uint8_t type = REMOTE_I2C_RECV;
+qemu_chr_fe_write_all(>chr, , ONE_BYTE);
+
+qemu_chr_fe_read_all(>chr, , ONE_BYTE);
+return resp;
+}
+
+static int remote_i2c_send(I2CSlave *s, uint8_t data)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t type = REMOTE_I2C_SEND;
+uint8_t resp = 1;
+qemu_chr_fe_write_all(>chr, , ONE_BYTE);
+qemu_chr_fe_write_all(>chr, , ONE_BYTE);
+
+qemu_chr_fe_read_all(>chr, , ONE_BYTE);
+return resp ? -1 : 0;
+}
+
+/* Returns non-zero when no response from the device. */
+static int remote_i2c_event(I2CSlave *s, enum i2c_event event)
+{
+RemoteI2CState *i2c = REMOTE_I2C(s);
+uint8_t type;
+uint8_t resp = 1;
+switch (event) {
+case I2C_START_RECV:
+type = REMOTE_I2C_START_RECV;
+break;
+case I2C_START_SEND:
+type = REMOTE_I2C_START_SEND;
+break;
+case I2C_FINISH:
+type = REMOTE_I2C_FINISH;
+break;
+case I2C_NACK:
+type = REMOTE_I2C_NACK;
+}
+qemu_chr_fe_write_all(>chr, , ONE_BYTE);
+qemu_chr_fe_read_all(>chr, , ONE_BYTE);
+return resp ? -1 : 0;
+}
+
+static Property remote_i2c_props[] = {
+DEFINE_PROP_CHR("chardev", RemoteI2CState, chr),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void remote_i2c_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+k->recv = _i2c_recv;
+k->send = _i2c_send;
+k->event = _i2c_event;
+device_class_set_props(dc, remote_i2c_props);
+}
+
+static const TypeInfo remote_i2c_type = {
+.name = TYPE_REMOTE_I2C,
+.parent = TYPE_I2C_SLAVE,
+.instance_size = sizeof(RemoteI2CState),
+.class_size = sizeof(I2CSlaveClass),
+
[PATCH 0/1] Add remote I2C device to support external I2C device
This patch implements the remote I2C device. The remote I2C device allows an external I2C device to communicate with the I2C controller in QEMU through the remote I2C protocol. Users no longer have to directly modify QEMU to add new I2C devices and can instead implement the emulated device externally and connect it to the remote I2C device. Previous work by Wolfram Sang (https://git.kernel.org/pub/scm/virt/qemu/wsa/qemu.git/commit/?h=i2c-passthrough) was referenced. It shares the similar idea of redirecting the actual I2C device functionalities, but Sang focuses on physical devices, and we focus on emulated devices. The work by Sang mainly utilizes file descriptors while ours utilizes character devices, which offers better support for emulated devices. The work by Sang is not meant to offer full I2C device support; it only implements the receive functionality. Our work implements full support for I2C devices: send, recv, and event (match_and_add is not applicable for external devices). Shengtan Mao (1): hw/i2c: add remote I2C device hw/arm/Kconfig| 1 + hw/i2c/Kconfig| 4 + hw/i2c/meson.build| 1 + hw/i2c/remote-i2c.c | 117 ++ tests/qtest/meson.build | 1 + tests/qtest/remote-i2c-test.c | 216 ++ 6 files changed, 340 insertions(+) create mode 100644 hw/i2c/remote-i2c.c create mode 100644 tests/qtest/remote-i2c-test.c -- 2.32.0.402.g57bb445576-goog
Re: Prefetches in buffer_zero_*
On 7/22/21 12:02 AM, Dr. David Alan Gilbert wrote: Hi Richard, I think you were the last person to fiddle with the prefetching in buffer_zero_avx2 and friends; Joe (cc'd) wondered if explicit prefetching still made sense on modern CPUs, and that their hardware generally figures stuff out better on simple increments. What was your thinking on this, and did you actually measure any improvement? Ah, well, that was 5 years ago so I have no particular memory of this. It wouldn't surprise me if you can't measure any improvement on modern hardware. Do you now measure an improvement with the prefetches gone? r~
Re: [PATCH v3 5/5] migration: Move the yank unregister of channel_close out
* Peter Xu (pet...@redhat.com) wrote:
> It's efficient, but hackish to call yank unregister calls in channel_close(),
> especially it'll be hard to debug when qemu crashed with some yank function
> leaked.
>
> Remove that hack, but instead explicitly unregister yank functions at the
> places where needed, they are:
>
> (on src)
> - migrate_fd_cleanup
> - postcopy_pause
>
> (on dst)
> - migration_incoming_state_destroy
> - postcopy_pause_incoming
>
> Signed-off-by: Peter Xu
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/migration.c | 14 +-
> migration/qemu-file-channel.c | 3 ---
> migration/savevm.c| 7 +++
> migration/yank_functions.c| 14 ++
> migration/yank_functions.h| 1 +
> 5 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b2c48b7e17..a50330016c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -59,6 +59,7 @@
> #include "multifd.h"
> #include "qemu/yank.h"
> #include "sysemu/cpus.h"
> +#include "yank_functions.h"
>
> #define MAX_THROTTLE (128 << 20) /* Migration transfer speed
> throttling */
>
> @@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
> }
>
> if (mis->from_src_file) {
> +migration_ioc_unregister_yank_from_file(mis->from_src_file);
> qemu_fclose(mis->from_src_file);
> mis->from_src_file = NULL;
> }
> @@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
> * Close the file handle without the lock to make sure the
> * critical section won't block for long.
> */
> +migration_ioc_unregister_yank_from_file(tmp);
> qemu_fclose(tmp);
> }
>
> @@ -3351,8 +3354,17 @@ static MigThrError postcopy_pause(MigrationState *s)
> while (true) {
> QEMUFile *file;
>
> -/* Current channel is possibly broken. Release it. */
> +/*
> + * Current channel is possibly broken. Release it. Note that this is
> + * guaranteed even without lock because to_dst_file should only be
> + * modified by the migration thread. That also guarantees that the
> + * unregister of yank is safe too without the lock. It should be
> safe
> + * even to be within the qemu_file_lock, but we didn't do that to
> avoid
> + * taking more mutex (yank_lock) within qemu_file_lock. TL;DR: we
> make
> + * the qemu_file_lock critical section as small as possible.
> + */
> assert(s->to_dst_file);
> +migration_ioc_unregister_yank_from_file(s->to_dst_file);
> qemu_mutex_lock(>qemu_file_lock);
> file = s->to_dst_file;
> s->to_dst_file = NULL;
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 2f8b1fcd46..bb5a5752df 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
> int ret;
> QIOChannel *ioc = QIO_CHANNEL(opaque);
> ret = qio_channel_close(ioc, errp);
> -if (OBJECT(ioc)->ref == 1) {
> -migration_ioc_unregister_yank(ioc);
> -}
> object_unref(OBJECT(ioc));
> return ret;
> }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 96b5e5d639..7b7b64bd13 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -65,6 +65,7 @@
> #include "qemu/bitmap.h"
> #include "net/announce.h"
> #include "qemu/yank.h"
> +#include "yank_functions.h"
>
> const unsigned int postcopy_ram_discard_version;
>
> @@ -2568,6 +2569,12 @@ static bool
> postcopy_pause_incoming(MigrationIncomingState *mis)
> /* Clear the triggered bit to allow one recovery */
> mis->postcopy_recover_triggered = false;
>
> +/*
> + * Unregister yank with either from/to src would work, since ioc behind
> it
> + * is the same
> + */
> +migration_ioc_unregister_yank_from_file(mis->from_src_file);
> +
> assert(mis->from_src_file);
> qemu_file_shutdown(mis->from_src_file);
> qemu_fclose(mis->from_src_file);
> diff --git a/migration/yank_functions.c b/migration/yank_functions.c
> index 23697173ae..8c08aef14a 100644
> --- a/migration/yank_functions.c
> +++ b/migration/yank_functions.c
> @@ -14,6 +14,7 @@
> #include "qemu/yank.h"
> #include "io/channel-socket.h"
> #include "io/channel-tls.h"
> +#include "qemu-file.h"
>
> void migration_yank_iochannel(void *opaque)
> {
> @@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
> QIO_CHANNEL(ioc));
> }
> }
> +
> +void migration_ioc_unregister_yank_from_file(QEMUFile *file)
> +{
> +QIOChannel *ioc = qemu_file_get_ioc(file);
> +
> +if (ioc) {
> +/*
> + * For migration qemufiles, we'll always reach here. Though we'll
> skip
> + * calls from e.g. savevm/loadvm
[PATCH] MAINTAINERS: Don't list Andrzej Zaborowski for various components
Andrzej Zaborowski is listed as an "Odd Fixes" maintainer for the nSeries, Palm and PXA2XX boards, as well as the "Maintained" status Arm 32-bit TCG backend. Andrzej's last email to qemu-devel was back in 2017, and the email before that was all the way back in 2013. We don't really need to fill his email up with CCs on QEMU patches any more... Remove Andrzej from the various boards sections (leaving them still Odd Fixes with me as the backup patch reviewer). Add Richard Henderson as the maintainer for the Arm TCG backend, since removing Andrzej would otherwise leave that section with no M: line at all. Signed-off-by: Peter Maydell --- Andrzej: if you're reading this, thanks for all the work you did on QEMU back in the day; and if you do want to still be CCd on patches let me know and I'll happily drop this MAINTAINERS update. Richard: are you happy with (a) being listed for Arm TCG and (b) it being "Maintained" status? --- MAINTAINERS | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 4256ad1adbb..8c44a26bcce 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -786,7 +786,6 @@ F: roms/vbootrom F: docs/system/arm/nuvoton.rst nSeries -M: Andrzej Zaborowski M: Peter Maydell L: qemu-...@nongnu.org S: Odd Fixes @@ -804,7 +803,6 @@ F: tests/acceptance/machine_arm_n8x0.py F: docs/system/arm/nseries.rst Palm -M: Andrzej Zaborowski M: Peter Maydell L: qemu-...@nongnu.org S: Odd Fixes @@ -837,7 +835,6 @@ F: include/hw/intc/realview_gic.h F: docs/system/arm/realview.rst PXA2XX -M: Andrzej Zaborowski M: Peter Maydell L: qemu-...@nongnu.org S: Odd Fixes @@ -3037,7 +3034,7 @@ F: disas/arm-a64.cc F: disas/libvixl/ ARM TCG target -M: Andrzej Zaborowski +M: Richard Henderson S: Maintained L: qemu-...@nongnu.org F: tcg/arm/ -- 2.20.1
[PATCH v3 5/5] migration: Move the yank unregister of channel_close out
It's efficient, but hackish to call yank unregister calls in channel_close(),
especially it'll be hard to debug when qemu crashed with some yank function
leaked.
Remove that hack, but instead explicitly unregister yank functions at the
places where needed, they are:
(on src)
- migrate_fd_cleanup
- postcopy_pause
(on dst)
- migration_incoming_state_destroy
- postcopy_pause_incoming
Signed-off-by: Peter Xu
---
migration/migration.c | 14 +-
migration/qemu-file-channel.c | 3 ---
migration/savevm.c| 7 +++
migration/yank_functions.c| 14 ++
migration/yank_functions.h| 1 +
5 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index b2c48b7e17..a50330016c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -59,6 +59,7 @@
#include "multifd.h"
#include "qemu/yank.h"
#include "sysemu/cpus.h"
+#include "yank_functions.h"
#define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling
*/
@@ -273,6 +274,7 @@ void migration_incoming_state_destroy(void)
}
if (mis->from_src_file) {
+migration_ioc_unregister_yank_from_file(mis->from_src_file);
qemu_fclose(mis->from_src_file);
mis->from_src_file = NULL;
}
@@ -1811,6 +1813,7 @@ static void migrate_fd_cleanup(MigrationState *s)
* Close the file handle without the lock to make sure the
* critical section won't block for long.
*/
+migration_ioc_unregister_yank_from_file(tmp);
qemu_fclose(tmp);
}
@@ -3351,8 +3354,17 @@ static MigThrError postcopy_pause(MigrationState *s)
while (true) {
QEMUFile *file;
-/* Current channel is possibly broken. Release it. */
+/*
+ * Current channel is possibly broken. Release it. Note that this is
+ * guaranteed even without lock because to_dst_file should only be
+ * modified by the migration thread. That also guarantees that the
+ * unregister of yank is safe too without the lock. It should be safe
+ * even to be within the qemu_file_lock, but we didn't do that to avoid
+ * taking more mutex (yank_lock) within qemu_file_lock. TL;DR: we make
+ * the qemu_file_lock critical section as small as possible.
+ */
assert(s->to_dst_file);
+migration_ioc_unregister_yank_from_file(s->to_dst_file);
qemu_mutex_lock(>qemu_file_lock);
file = s->to_dst_file;
s->to_dst_file = NULL;
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 2f8b1fcd46..bb5a5752df 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,9 +107,6 @@ static int channel_close(void *opaque, Error **errp)
int ret;
QIOChannel *ioc = QIO_CHANNEL(opaque);
ret = qio_channel_close(ioc, errp);
-if (OBJECT(ioc)->ref == 1) {
-migration_ioc_unregister_yank(ioc);
-}
object_unref(OBJECT(ioc));
return ret;
}
diff --git a/migration/savevm.c b/migration/savevm.c
index 96b5e5d639..7b7b64bd13 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -65,6 +65,7 @@
#include "qemu/bitmap.h"
#include "net/announce.h"
#include "qemu/yank.h"
+#include "yank_functions.h"
const unsigned int postcopy_ram_discard_version;
@@ -2568,6 +2569,12 @@ static bool
postcopy_pause_incoming(MigrationIncomingState *mis)
/* Clear the triggered bit to allow one recovery */
mis->postcopy_recover_triggered = false;
+/*
+ * Unregister yank with either from/to src would work, since ioc behind it
+ * is the same
+ */
+migration_ioc_unregister_yank_from_file(mis->from_src_file);
+
assert(mis->from_src_file);
qemu_file_shutdown(mis->from_src_file);
qemu_fclose(mis->from_src_file);
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 23697173ae..8c08aef14a 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -14,6 +14,7 @@
#include "qemu/yank.h"
#include "io/channel-socket.h"
#include "io/channel-tls.h"
+#include "qemu-file.h"
void migration_yank_iochannel(void *opaque)
{
@@ -46,3 +47,16 @@ void migration_ioc_unregister_yank(QIOChannel *ioc)
QIO_CHANNEL(ioc));
}
}
+
+void migration_ioc_unregister_yank_from_file(QEMUFile *file)
+{
+QIOChannel *ioc = qemu_file_get_ioc(file);
+
+if (ioc) {
+/*
+ * For migration qemufiles, we'll always reach here. Though we'll skip
+ * calls from e.g. savevm/loadvm as they don't use yank.
+ */
+migration_ioc_unregister_yank(ioc);
+}
+}
diff --git a/migration/yank_functions.h b/migration/yank_functions.h
index 74c7f18c91..a7577955ed 100644
--- a/migration/yank_functions.h
+++ b/migration/yank_functions.h
@@ -17,3 +17,4 @@
void migration_yank_iochannel(void *opaque);
void
Re: [PATCH for-6.1? 3/6] jobs: Give Job.force_cancel more meaning
22.07.2021 15:26, Max Reitz wrote: We largely have two cancel modes for jobs: First, there is actual cancelling. The job is terminated as soon as possible, without trying to reach a consistent result. Second, we have mirror in the READY state. Technically, the job is not really cancelled, but it just is a different completion mode. The job can still run for an indefinite amount of time while it tries to reach a consistent result. We want to be able to clearly distinguish which cancel mode a job is in (when it has been cancelled). We can use Job.force_cancel for this, but right now it only reflects cancel requests from the user with force=true, but clearly, jobs that do not even distinguish between force=false and force=true are effectively always force-cancelled. So this patch has Job.force_cancel signify whether the job will terminate as soon as possible (force_cancel=true) or whether it will effectively remain running despite being "cancelled" (force_cancel=false). To this end, we let jobs that provide JobDriver.cancel() tell the generic job code whether they will terminate as soon as possible or not, and for jobs that do not provide that method we assume they will. Signed-off-by: Max Reitz In isolation this patch is rather strange: force_cancel is used only by mirror. But we keep in generic job layer. And make a handler to set a value to this variable. So in generic layer we ask mirror which value it want to set to generic variable, which is used only by mirror.. This probably shows that this feature of mirror should be mirror only and generic layer shouldn't take care of it (see also my answer to next commit). But at the end of the series the variable is not more used by mirror directly. So, technically the commit is not wrong, and it is a preparation for the following ones. Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
[PATCH v3 2/5] migration: Make from_dst_file accesses thread-safe
Accessing from_dst_file is potentially racy in current code base like below:
if (s->from_dst_file)
do_something(s->from_dst_file);
Because from_dst_file can be reset right after the check in another
thread (rp_thread). One example is migrate_fd_cancel().
Use the same qemu_file_lock to protect it too, just like to_dst_file.
When it's safe to access without lock, comment it.
There's one special reference in migration_thread() that can be replaced by
the newly introduced rp_thread_created flag.
Reported-by: Dr. David Alan Gilbert
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 39 +--
migration/migration.h | 8 +---
migration/ram.c | 1 +
3 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 21b94f75a3..b2c48b7e17 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1879,9 +1879,11 @@ static void migrate_fd_cancel(MigrationState *s)
QEMUFile *f = migrate_get_current()->to_dst_file;
trace_migrate_fd_cancel();
-if (s->rp_state.from_dst_file) {
-/* shutdown the rp socket, so causing the rp thread to shutdown */
-qemu_file_shutdown(s->rp_state.from_dst_file);
+WITH_QEMU_LOCK_GUARD(>qemu_file_lock) {
+if (s->rp_state.from_dst_file) {
+/* shutdown the rp socket, so causing the rp thread to shutdown */
+qemu_file_shutdown(s->rp_state.from_dst_file);
+}
}
do {
@@ -2686,6 +2688,23 @@ static int migrate_handle_rp_resume_ack(MigrationState
*s, uint32_t value)
return 0;
}
+/* Release ms->rp_state.from_dst_file in a safe way */
+static void migration_release_from_dst_file(MigrationState *ms)
+{
+QEMUFile *file;
+
+WITH_QEMU_LOCK_GUARD(>qemu_file_lock) {
+/*
+ * Reset the from_dst_file pointer first before releasing it, as we
+ * can't block within lock section
+ */
+file = ms->rp_state.from_dst_file;
+ms->rp_state.from_dst_file = NULL;
+}
+
+qemu_fclose(file);
+}
+
/*
* Handles messages sent on the return path towards the source VM
*
@@ -2827,11 +2846,13 @@ out:
* Maybe there is something we can do: it looks like a
* network down issue, and we pause for a recovery.
*/
-qemu_fclose(rp);
-ms->rp_state.from_dst_file = NULL;
+migration_release_from_dst_file(ms);
rp = NULL;
if (postcopy_pause_return_path_thread(ms)) {
-/* Reload rp, reset the rest */
+/*
+ * Reload rp, reset the rest. Referencing it is safe since
+ * it's reset only by us above, or when migration completes
+ */
rp = ms->rp_state.from_dst_file;
ms->rp_state.error = false;
goto retry;
@@ -2843,8 +2864,7 @@ out:
}
trace_source_return_path_thread_end();
-ms->rp_state.from_dst_file = NULL;
-qemu_fclose(rp);
+migration_release_from_dst_file(ms);
rcu_unregister_thread();
return NULL;
}
@@ -2852,7 +2872,6 @@ out:
static int open_return_path_on_source(MigrationState *ms,
bool create_thread)
{
-
ms->rp_state.from_dst_file = qemu_file_get_return_path(ms->to_dst_file);
if (!ms->rp_state.from_dst_file) {
return -1;
@@ -3746,7 +3765,7 @@ static void *migration_thread(void *opaque)
* If we opened the return path, we need to make sure dst has it
* opened as well.
*/
-if (s->rp_state.from_dst_file) {
+if (s->rp_state.rp_thread_created) {
/* Now tell the dest that it should open its end so it can reply */
qemu_savevm_send_open_return_path(s->to_dst_file);
diff --git a/migration/migration.h b/migration/migration.h
index c302879fad..7a5aa8c2fd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -154,12 +154,13 @@ struct MigrationState {
QemuThread thread;
QEMUBH *vm_start_bh;
QEMUBH *cleanup_bh;
+/* Protected by qemu_file_lock */
QEMUFile *to_dst_file;
QIOChannelBuffer *bioc;
/*
- * Protects to_dst_file pointer. We need to make sure we won't
- * yield or hang during the critical section, since this lock will
- * be used in OOB command handler.
+ * Protects to_dst_file/from_dst_file pointers. We need to make sure we
+ * won't yield or hang during the critical section, since this lock will be
+ * used in OOB command handler.
*/
QemuMutex qemu_file_lock;
@@ -192,6 +193,7 @@ struct MigrationState {
/* State related to return path */
struct {
+/* Protected by qemu_file_lock */
QEMUFile *from_dst_file;
QemuThreadrp_thread;
bool error;
diff --git a/migration/ram.c b/migration/ram.c
index b5fc454b2f..f728f5072f 100644
---
Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges
On 22/07/2021 19:49, Michael S. Tsirkin wrote:
> On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote:
...
>>
>> Fixes: 17858a169508609ca9063c544833e5a1adeb7b52
>
> use short hash and include subject within ("subject here") please
Tips:
some people use
.gitconfig
[pretty]
fixes = Fixes: %h (\"%s\")%nCc: %aE
[alias]
showfix = log -1 --format=fixes
$ git showfix 17858a169508609ca9063c544833e5a1adeb7b52
Fixes: 17858a169508 ("hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35")
Cc: jus...@redhat.com
Thanks,
Laurent
[PATCH-for-6.1 v2] gitlab-ci: Extract OpenSBI job rules and fix 'when' condition
First, all jobs depending on 'docker-opensbi' job must use at most
all the rules that triggers it. The simplest way to ensure that is
to always use the same rules. Extract all the rules to a reusable
section, and include this section (with the 'extends' keyword) in
both 'docker-opensbi' and 'build-opensbi' jobs.
Second, jobs depending on another should not use the 'when: always'
condition, because if a dependency failed we should not keep running
jobs depending on it. The correct condition is 'when: on_success'.
The problems were introduced in commit c6fc0fc1a71 ("gitlab-ci.yml:
Add jobs to build OpenSBI firmware binaries"), but were revealed in
commit 91e9c47e50a ("docker: OpenSBI build job depends on OpenSBI
container").
This fix is similar to the one used with the EDK2 firmware job in
commit ac0595cf6b3 ("gitlab-ci: Extract EDK2 job rules to reusable
section").
Reported-by: Daniel P. Berrangé
Reviewed-by: Daniel P. Berrangé
Reviewed-by: Willian Rampazzo
Signed-off-by: Philippe Mathieu-Daudé
---
v2: when 'always' -> 'on_success' & reworded (danpb)
Supersedes: <20210720164829.3949558-1-phi...@redhat.com>
---
.gitlab-ci.d/opensbi.yml | 30 ++
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/.gitlab-ci.d/opensbi.yml b/.gitlab-ci.d/opensbi.yml
index f66cd1d9089..5e0a2477c5d 100644
--- a/.gitlab-ci.d/opensbi.yml
+++ b/.gitlab-ci.d/opensbi.yml
@@ -1,10 +1,23 @@
-docker-opensbi:
- stage: containers
- rules: # Only run this job when the Dockerfile is modified
+# All jobs needing docker-opensbi must use the same rules it uses.
+.opensbi_job_rules:
+ rules: # Only run this job when ...
- changes:
+ # this file is modified
- .gitlab-ci.d/opensbi.yml
+ # or the Dockerfile is modified
- .gitlab-ci.d/opensbi/Dockerfile
- when: always
+ when: on_success
+ - changes: # or roms/opensbi/ is modified (submodule updated)
+ - roms/opensbi/*
+ when: on_success
+ - if: '$CI_COMMIT_REF_NAME =~ /^opensbi/' # or the branch/tag starts with
'opensbi'
+ when: on_success
+ - if: '$CI_COMMIT_MESSAGE =~ /opensbi/i' # or last commit description
contains 'OpenSBI'
+ when: on_success
+
+docker-opensbi:
+ extends: .opensbi_job_rules
+ stage: containers
image: docker:19.03.1
services:
- docker:19.03.1-dind
@@ -24,16 +37,9 @@ docker-opensbi:
- docker push $IMAGE_TAG
build-opensbi:
+ extends: .opensbi_job_rules
stage: build
needs: ['docker-opensbi']
- rules: # Only run this job when ...
- - changes: # ... roms/opensbi/ is modified (submodule updated)
- - roms/opensbi/*
- when: always
- - if: '$CI_COMMIT_REF_NAME =~ /^opensbi/' # or the branch/tag starts with
'opensbi'
- when: always
- - if: '$CI_COMMIT_MESSAGE =~ /opensbi/i' # or last commit description
contains 'OpenSBI'
- when: always
artifacts:
paths: # 'artifacts.zip' will contains the following files:
- pc-bios/opensbi-riscv32-generic-fw_dynamic.bin
--
2.31.1
[PATCH v3 1/5] migration: Fix missing join() of rp_thread
It's possible that the migration thread skip the join() of the rp_thread in
below race and crash on src right at finishing migration:
migration_thread rp_thread
-
migration_completion()
(before rp_thread quits)
from_dst_file=NULL
[thread got scheduled out]
s->rp_state.from_dst_file==NULL
(skip join() of rp_thread)
migrate_fd_cleanup()
qemu_fclose(s->to_dst_file)
yank_unregister_instance()
assert(yank_find_entry()) <--- crash
It could mostly happen with postcopy, but that shouldn't be required, e.g., I
think it could also trigger with MIGRATION_CAPABILITY_RETURN_PATH set.
It's suspected that above race could be the root cause of a recent (but rare)
migration-test break reported by either Dave or PMM:
https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/
The issue is: from_dst_file is reset in the rp_thread, so if the thread reset
it to NULL fast enough then the migration thread will assume there's no
rp_thread at all.
This could potentially cause more severe issue (e.g. crash) after the yank code.
Fix it by using a boolean to keep "whether we've created rp_thread".
Cc: Dr. David Alan Gilbert
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/migration.c | 4 +++-
migration/migration.h | 7 +++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/migration/migration.c b/migration/migration.c
index 2d306582eb..21b94f75a3 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2867,6 +2867,7 @@ static int open_return_path_on_source(MigrationState *ms,
qemu_thread_create(>rp_state.rp_thread, "return path",
source_return_path_thread, ms, QEMU_THREAD_JOINABLE);
+ms->rp_state.rp_thread_created = true;
trace_open_return_path_on_source_continue();
@@ -2891,6 +2892,7 @@ static int
await_return_path_close_on_source(MigrationState *ms)
}
trace_await_return_path_close_on_source_joining();
qemu_thread_join(>rp_state.rp_thread);
+ms->rp_state.rp_thread_created = false;
trace_await_return_path_close_on_source_close();
return ms->rp_state.error;
}
@@ -3170,7 +3172,7 @@ static void migration_completion(MigrationState *s)
* it will wait for the destination to send it's status in
* a SHUT command).
*/
-if (s->rp_state.from_dst_file) {
+if (s->rp_state.rp_thread_created) {
int rp_error;
trace_migration_return_path_end_before();
rp_error = await_return_path_close_on_source(s);
diff --git a/migration/migration.h b/migration/migration.h
index 2ebb740dfa..c302879fad 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -195,6 +195,13 @@ struct MigrationState {
QEMUFile *from_dst_file;
QemuThreadrp_thread;
bool error;
+/*
+ * We can also check non-zero of rp_thread, but there's no "official"
+ * way to do this, so this bool makes it slightly more elegant.
+ * Checking from_dst_file for this is racy because from_dst_file will
+ * be cleared in the rp_thread!
+ */
+bool rp_thread_created;
QemuSemaphore rp_sem;
} rp_state;
--
2.31.1
[PATCH v3 0/5] migrations: Fix potential rare race of migration-test after yank
v3: - Use WITH_QEMU_LOCK_GUARD() for patch 2 [Eric] (potentially I can also replace other existing uses of qemu_file_lock into WITH_QEMU_LOCK_GUARD, but I decided to took Dave's r-b first and leave that for later) - Added r-bs for Dave on patch 2/4 - Add a comment in patch 5 to explain why it's safe to unregister yank without qemu_file_lock, in postcopy_pause() [Dave] v2: - Pick r-b for Dave on patch 1/3 - Move migration_file_get_ioc() from patch 5 to patch 4, meanwhile rename it to qemu_file_get_ioc(). [Dave] - Patch 2 "migration: Shutdown src in await_return_path_close_on_source()" is replaced by patch "migration: Make from_dst_file accesses thread-safe" [Dave] Patch 1 fixes a possible race that migration thread can accidentally skip join() of rp_thread even if the return thread is enabled. Patch 1 is suspected to also be the root cause of the recent hard-to-reproduce migration-test failure here reported by PMM: https://lore.kernel.org/qemu-devel/YPamXAHwan%2FPPXLf@work-vm/ I didn't reproduce it myself; but after co-debugged with Dave it's suspected that the race of rp_thread could be the cause. It's not exposed before because yank is soo strict on releasing instances, while we're not that strict before, and didn't join() on rp_thread wasn't so dangerous after all when migration succeeded before. Patch 2 fixes another theoretical race on accessing from_dst_file spotted by Dave. I don't think there's known issues with it, but may still worth fixing. Patch 3 should be a cleanup on yank that I think would be nice to have. Patch 4-5 are further cleanups to remove the ref==1 check in channel_close(), finally, as I always thought that's a bit hackish. So I used explicit unregister of the yank function at necessary places to replace that ref==1 one. I still think having patch 3-5 altogether would be great, however I think patch 1 should still be the most important to be reviewed. Also it would be great to know whether patch 1 could fix the known yank crash. Please review, thanks. Peter Xu (5): migration: Fix missing join() of rp_thread migration: Make from_dst_file accesses thread-safe migration: Introduce migration_ioc_[un]register_yank() migration: Teach QEMUFile to be QIOChannel-aware migration: Move the yank unregister of channel_close out migration/channel.c | 15 ++--- migration/migration.c | 57 +++ migration/migration.h | 15 +++-- migration/multifd.c | 8 ++--- migration/qemu-file-channel.c | 11 ++- migration/qemu-file.c | 17 ++- migration/qemu-file.h | 4 ++- migration/ram.c | 3 +- migration/savevm.c| 11 +-- migration/yank_functions.c| 42 ++ migration/yank_functions.h| 3 ++ 11 files changed, 138 insertions(+), 48 deletions(-) -- 2.31.1
Re: [PATCH 1/2] acpi: x86: pcihp: cleanup devfn usage in build_append_pci_bus_devices()
I would add a description: we want to scan all functions
not just function 0 to describe hotplug into bridges
at function != 0. in preparation for this, refactor code to not
skip functions != 0.
On Thu, Jul 22, 2021 at 06:59:44AM -0400, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov
> ---
> hw/i386/acpi-build.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 17836149fe..b40e284b72 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -374,7 +374,7 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> Aml *dev, *notify_method = NULL, *method;
> QObject *bsel;
> PCIBus *sec;
> -int i;
> +int devfn;
>
> bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> NULL);
> if (bsel) {
> @@ -384,11 +384,11 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> notify_method = aml_method("DVNT", 2, AML_NOTSERIALIZED);
> }
>
> -for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> +for (devfn = 0; devfn < ARRAY_SIZE(bus->devices); devfn++) {
> DeviceClass *dc;
> PCIDeviceClass *pc;
> -PCIDevice *pdev = bus->devices[i];
> -int slot = PCI_SLOT(i);
> +PCIDevice *pdev = bus->devices[devfn];
> +int slot = PCI_SLOT(devfn);
> bool hotplug_enabled_dev;
> bool bridge_in_acpi;
> bool cold_plugged_bridge;
I am a bit puzzled about why this is equivalent. so we used to scan just
function 0 on each slot. now we are scanning them all.
won't this generate a different AML code? in fact duplicate
descriptions?
I suspect you need to move the check for slot == 0 from the next patch
to this one otherwise bisect will be broken.
Or just squash this part into next patch. up to you.
> @@ -525,13 +525,12 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> /* Notify about child bus events in any case */
> if (pcihp_bridge_en) {
> QLIST_FOREACH(sec, >child, sibling) {
> -int32_t devfn = sec->parent_dev->devfn;
> -
> if (pci_bus_is_root(sec)) {
> continue;
> }
>
> -aml_append(method, aml_name("^S%.02X.PCNT", devfn));
> +aml_append(method, aml_name("^S%.02X.PCNT",
> +sec->parent_dev->devfn));
> }
> }
>
this is a refactor, sure.
> --
> 2.27.0
Re: -only-migrate and the two different uses of migration blockers
* David Gibson (da...@gibson.dropbear.id.au) wrote: > On Tue, Jul 20, 2021 at 07:30:16AM +0200, Markus Armbruster wrote: > > "Dr. David Alan Gilbert" writes: > > > > > * Markus Armbruster (arm...@redhat.com) wrote: > > >> We appear to use migration blockers in two ways: > > >> > > >> (1) Prevent migration for an indefinite time, typically due to use of > > >> some feature that isn't compatible with migration. > > >> > > >> (2) Delay migration for a short time. > > >> > > >> Option -only-migrate is designed for (1). It interferes with (2). > > >> > > >> Example for (1): device "x-pci-proxy-dev" doesn't support migration. It > > >> adds a migration blocker on realize, and deletes it on unrealize. With > > >> -only-migrate, device realize fails. Works as designed. > > >> > > >> Example for (2): spapr_mce_req_event() makes an effort to prevent > > >> migration degrate the reporting of FWNMIs. It adds a migration blocker > > >> when it receives one, and deletes it when it's done handling it. This > > >> is a best effort; if migration is already in progress by the time FWNMI > > >> is received, we simply carry on, and that's okay. However, option > > >> -only-migrate sabotages the best effort entirely. > > > > > > That's interesting; it's the first time I've heard of anyone using it as > > > 'best effort'. I've always regarded blockers as blocking. > > > > Me too, until I found this one. > > Right, it may well have been the first usage this way, this fwnmi > stuff isn't super old. > > > >> While this isn't exactly terrible, it may be a weakness in our thinking > > >> and our infrastructure. I'm bringing it up so the people in charge are > > >> aware :) > > > > > > Thanks. > > > > > > It almost feels like they need a way to temporarily hold off > > > 'completion' of migratio - i.e. the phase where we stop the CPU and > > > write the device data; mind you you'd also probably want it to stop > > > cold-migrates/snapshots? > > > > Yes, a proper way to delay 'completion' for a bit would be clearer, and > > wouldn't let -only-migrate interfere. > > Right. If that becomes a thing, we should use it here. Note that > this one use case probably isn't a very strong argument for it, > though. The only problem here is slightly less that optimal error > reporting in a rare edge case (hardware fault occurs by chance at the > same time as a migration). Can you at least put a scary comment in to say why it's so odd. If you wanted a choice of a different bad way to do this, since you have savevm_htab_handlers, you might be able to make htab_save_iterate claim there's always more to do. > > and, also, I half-suspect that the whole fwnmi feature exists > more to tick IBM RAS check boxes than because anyone will actually use > it. Ah at least it's always reliable Dave > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au| minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH v3 3/4] vhost-user-rng: backend: Add RNG vhost-user daemon implementation
On Wed, Jul 21, 2021 at 09:14:31PM +0100, Alex Bennée wrote:
>
> Mathieu Poirier writes:
>
> > This patch provides the vhost-user backend implementation to work
> > in tandem with the vhost-user-rng implementation of the QEMU VMM.
> >
> > It uses the vhost-user API so that other VMM can re-use the interface
> > without having to write the driver again.
> >
> > Signed-off-by: Mathieu Poirier
>
> Try the following patch which creates a nested main loop and runs it
> until the g_timeout_add fires again.
>
> --8<---cut here---start->8---
> tools/virtio/vhost-user-rng: avoid mutex by using nested main loop
>
> As we are blocking anyway all we really need to do is run a main loop
> until the timer fires and the data is consumed.
>
Right, I made the implemenation blocking to be as close as possible to what
virtio-rng does.
I took a look at your patch below and it should do the trick. Testing yielded
the same results as my solution so this is good. To me the nested loop is a
little unorthodox to solve this kind of problem but it has less lines of code
and avoids spinning a new thread to deal with the timer.
I'll send another revision.
Thanks for the review,
Mathieu
> Signed-off-by: Alex Bennée
>
> 1 file changed, 30 insertions(+), 76 deletions(-)
> tools/vhost-user-rng/main.c | 106 +---
>
> modified tools/vhost-user-rng/main.c
> @@ -42,13 +42,10 @@
>
> typedef struct {
> VugDev dev;
> -struct itimerspec ts;
> -timer_t rate_limit_timer;
> -pthread_mutex_t rng_mutex;
> -pthread_cond_t rng_cond;
> int64_t quota_remaining;
> -bool activate_timer;
> +guint timer;
> GMainLoop *loop;
> +GMainLoop *blocked;
> } VuRNG;
>
> static gboolean print_cap, verbose;
> @@ -59,66 +56,26 @@ static gint source_fd, socket_fd = -1;
> static uint32_t period_ms = 1 << 16;
> static uint64_t max_bytes = INT64_MAX;
>
> -static void check_rate_limit(union sigval sv)
> +static gboolean check_rate_limit(gpointer user_data)
> {
> -VuRNG *rng = sv.sival_ptr;
> -bool wakeup = false;
> +VuRNG *rng = (VuRNG *) user_data;
>
> -pthread_mutex_lock(>rng_mutex);
> -/*
> - * The timer has expired and the guest has used all available
> - * entropy, which means function vu_rng_handle_request() is waiting
> - * on us. As such wake it up once we're done here.
> - */
> -if (rng->quota_remaining == 0) {
> -wakeup = true;
> +if (rng->blocked) {
> +g_info("%s: called while blocked", __func__);
> +g_main_loop_quit(rng->blocked);
> }
> -
> /*
> * Reset the entropy available to the guest and tell function
> * vu_rng_handle_requests() to start the timer before using it.
> */
> rng->quota_remaining = max_bytes;
> -rng->activate_timer = true;
> -pthread_mutex_unlock(>rng_mutex);
> -
> -if (wakeup) {
> -pthread_cond_signal(>rng_cond);
> -}
> -}
> -
> -static void setup_timer(VuRNG *rng)
> -{
> -struct sigevent sev;
> -int ret;
> -
> -memset(>ts, 0, sizeof(struct itimerspec));
> -rng->ts.it_value.tv_sec = period_ms / 1000;
> -rng->ts.it_value.tv_nsec = (period_ms % 1000) * 100;
> -
> -/*
> - * Call function check_rate_limit() as if it was the start of
> - * a new thread when the timer expires.
> - */
> -sev.sigev_notify = SIGEV_THREAD;
> -sev.sigev_notify_function = check_rate_limit;
> -sev.sigev_value.sival_ptr = rng;
> -/* Needs to be NULL if defaults attributes are to be used. */
> -sev.sigev_notify_attributes = NULL;
> -ret = timer_create(CLOCK_MONOTONIC, , >rate_limit_timer);
> -if (ret < 0) {
> -fprintf(stderr, "timer_create() failed\n");
> -}
> -
> +return true;
> }
>
> -
> /* Virtio helpers */
> static uint64_t rng_get_features(VuDev *dev)
> {
> -if (verbose) {
> -g_info("%s: replying", __func__);
> -}
> +g_info("%s: replying", __func__);
> return 0;
> }
>
> @@ -137,7 +94,7 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
> VuVirtq *vq = vu_get_queue(dev, qidx);
> VuVirtqElement *elem;
> size_t to_read;
> -int len, ret;
> +int len;
>
> for (;;) {
> /* Get element in the vhost virtqueue */
> @@ -149,24 +106,21 @@ static void vu_rng_handle_requests(VuDev *dev, int qidx)
> /* Get the amount of entropy to read from the vhost server */
> to_read = elem->in_sg[0].iov_len;
>
> -pthread_mutex_lock(>rng_mutex);
> -
> /*
> * We have consumed all entropy available for this time slice.
> * Wait for the timer (check_rate_limit()) to tell us about the
> * start of a new time slice.
> */
> if (rng->quota_remaining == 0) {
> -pthread_cond_wait(>rng_cond, >rng_mutex);
> -}
> -
> -/* Start the timer if the last time slice has
Re: [RFC PATCH v2 11/44] i386/tdx: Implement user specified tsc frequency
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote:
From: Xiaoyao Li
Reuse -cpu,tsc-frequency= to get user wanted tsc frequency and pass it
to KVM_TDX_INIT_VM.
Besides, sanity check the tsc frequency to be in the legal range and
legal granularity (required by SEAM module).
Signed-off-by: Xiaoyao Li
Signed-off-by: Isaku Yamahata
---
[..]
+if (env->tsc_khz && (env->tsc_khz < TDX1_MIN_TSC_FREQUENCY_KHZ ||
+ env->tsc_khz > TDX1_MAX_TSC_FREQUENCY_KHZ)) {
+error_report("Invalid TSC %ld KHz, must specify cpu_frequecy between [%d,
%d] kHz\n",
s/frequecy/frequency
+ env->tsc_khz, TDX1_MIN_TSC_FREQUENCY_KHZ,
+ TDX1_MAX_TSC_FREQUENCY_KHZ);
+exit(1);
+}
+
+if (env->tsc_khz % (25 * 1000)) {
+error_report("Invalid TSC %ld KHz, it must be multiple of 25MHz\n",
env->tsc_khz);
Should this be 25KHz instead of 25MHz?
Re: [PATCH-for-6.1] gitlab-ci: Extract OpenSBI job rules to reusable section
On 7/22/21 5:49 PM, Daniel P. Berrangé wrote:
> On Tue, Jul 20, 2021 at 06:48:29PM +0200, Philippe Mathieu-Daudé wrote:
>> All jobs depending on 'docker-opensbi' job must use at most all
>> the rules that triggers it. The simplest way to ensure that
>> is to always use the same rules. Extract all the rules to a
>> reusable section, and include this section (with the 'extends'
>> keyword) in both 'docker-opensbi' and 'build-opensbi' jobs.
>>
>> The problem was introduced in commit c6fc0fc1a71 ("gitlab-ci.yml:
>> Add jobs to build OpenSBI firmware binaries"), but was revealed in
>> commit 91e9c47e50a ("docker: OpenSBI build job depends on OpenSBI
>> container").
>>
>> This fix is similar to the one used with the EDK2 firmware job in
>> commit ac0595cf6b3 ("gitlab-ci: Extract EDK2 job rules to reusable
>> section").
>>
>> Reported-by: Daniel P. Berrangé
>> Signed-off-by: Philippe Mathieu-Daudé
>> ---
>> Latent bug on CI, safe for 6.1.
>> ---
>> .gitlab-ci.d/opensbi.yml | 28 +---
>> 1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/.gitlab-ci.d/opensbi.yml b/.gitlab-ci.d/opensbi.yml
>> index f66cd1d9089..d8a0456679e 100644
>> --- a/.gitlab-ci.d/opensbi.yml
>> +++ b/.gitlab-ci.d/opensbi.yml
>> @@ -1,10 +1,23 @@
>> -docker-opensbi:
>> - stage: containers
>> - rules: # Only run this job when the Dockerfile is modified
>> +# All jobs needing docker-opensbi must use the same rules it uses.
>> +.opensbi_job_rules:
>> + rules: # Only run this job when ...
>> - changes:
>> + # this file is modified
>> - .gitlab-ci.d/opensbi.yml
>> + # or the Dockerfile is modified
>> - .gitlab-ci.d/opensbi/Dockerfile
>> when: always
>> + - changes: # or roms/opensbi/ is modified (submodule updated)
>> + - roms/opensbi/*
>> + when: always
>> + - if: '$CI_COMMIT_REF_NAME =~ /^opensbi/' # or the branch/tag starts with
>> 'opensbi'
>> + when: always
>> + - if: '$CI_COMMIT_MESSAGE =~ /opensbi/i' # or last commit description
>> contains 'OpenSBI'
>> + when: always
>
> In debugging why the acceptance jobs rnu despite their prerequisite
> jobs failing, I've realized we've been making a mistake in most of
> 'rules' sections.
>
> 'when: always' will make the job run regardless of status of any
> dependant jobs. IOW, if you have a 'needs: []', it is almost
> always wrong to use 'when: always'. Instead we need 'when: on_success'
Indeed, when the first job fail, the second is marked "skipped" with
no "retry" option.
> So this patch needs to make that change, and likewise the edk2 patch
> with the same logic.
>
> Alex has queued this one, but I don't see it in a PULL yet, so I
> guess we can just do a v2 of this.
Sure, thanks for updating me.
[PATCH v3 4/5] migration: Teach QEMUFile to be QIOChannel-aware
migration uses QIOChannel typed qemufiles. In follow up patches, we'll need
the capability to identify this fact, so that we can get the backing QIOChannel
from a QEMUFile.
We can also define types for QEMUFile but so far since we only need to be able
to identify QIOChannel, introduce a boolean which is simpler.
Introduce another helper qemu_file_get_ioc() to return the ioc backend of a
qemufile if has_ioc is set.
No functional change.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/qemu-file-channel.c | 4 ++--
migration/qemu-file.c | 17 -
migration/qemu-file.h | 4 +++-
migration/ram.c | 2 +-
migration/savevm.c| 4 ++--
5 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 867a5ed0c3..2f8b1fcd46 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -187,11 +187,11 @@ static const QEMUFileOps channel_output_ops = {
QEMUFile *qemu_fopen_channel_input(QIOChannel *ioc)
{
object_ref(OBJECT(ioc));
-return qemu_fopen_ops(ioc, _input_ops);
+return qemu_fopen_ops(ioc, _input_ops, true);
}
QEMUFile *qemu_fopen_channel_output(QIOChannel *ioc)
{
object_ref(OBJECT(ioc));
-return qemu_fopen_ops(ioc, _output_ops);
+return qemu_fopen_ops(ioc, _output_ops, true);
}
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 1eacf9e831..6338d8e2ff 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -55,6 +55,8 @@ struct QEMUFile {
Error *last_error_obj;
/* has the file has been shutdown */
bool shutdown;
+/* Whether opaque points to a QIOChannel */
+bool has_ioc;
};
/*
@@ -101,7 +103,7 @@ bool qemu_file_mode_is_not_valid(const char *mode)
return false;
}
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc)
{
QEMUFile *f;
@@ -109,6 +111,7 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps
*ops)
f->opaque = opaque;
f->ops = ops;
+f->has_ioc = has_ioc;
return f;
}
@@ -851,3 +854,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
f->ops->set_blocking(f->opaque, block, NULL);
}
}
+
+/*
+ * Return the ioc object if it's a migration channel. Note: it can return NULL
+ * for callers passing in a non-migration qemufile. E.g. see qemu_fopen_bdrv()
+ * and its usage in e.g. load_snapshot(). So we need to check against NULL
+ * before using it. If without the check, migration_incoming_state_destroy()
+ * could fail for load_snapshot().
+ */
+QIOChannel *qemu_file_get_ioc(QEMUFile *file)
+{
+return file->has_ioc ? QIO_CHANNEL(file->opaque) : NULL;
+}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a9b6d6ccb7..3f36d4dc8c 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -27,6 +27,7 @@
#include
#include "exec/cpu-common.h"
+#include "io/channel.h"
/* Read a chunk of data from a file at the given position. The pos argument
* can be ignored if the file is only be used for streaming. The number of
@@ -119,7 +120,7 @@ typedef struct QEMUFileHooks {
QEMURamSaveFunc *save_page;
} QEMUFileHooks;
-QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
+QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops, bool has_ioc);
void qemu_file_set_hooks(QEMUFile *f, const QEMUFileHooks *hooks);
int qemu_get_fd(QEMUFile *f);
int qemu_fclose(QEMUFile *f);
@@ -179,5 +180,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags,
void *data);
size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
ram_addr_t offset, size_t size,
uint64_t *bytes_sent);
+QIOChannel *qemu_file_get_ioc(QEMUFile *file);
#endif
diff --git a/migration/ram.c b/migration/ram.c
index f728f5072f..08b3cb7a4a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -550,7 +550,7 @@ static int compress_threads_save_setup(void)
/* comp_param[i].file is just used as a dummy buffer to save data,
* set its ops to empty.
*/
-comp_param[i].file = qemu_fopen_ops(NULL, _ops);
+comp_param[i].file = qemu_fopen_ops(NULL, _ops, false);
comp_param[i].done = true;
comp_param[i].quit = false;
qemu_mutex_init(_param[i].mutex);
diff --git a/migration/savevm.c b/migration/savevm.c
index 72848b946c..96b5e5d639 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -168,9 +168,9 @@ static const QEMUFileOps bdrv_write_ops = {
static QEMUFile *qemu_fopen_bdrv(BlockDriverState *bs, int is_writable)
{
if (is_writable) {
-return qemu_fopen_ops(bs, _write_ops);
+return qemu_fopen_ops(bs, _write_ops, false);
}
-return qemu_fopen_ops(bs, _read_ops);
+return qemu_fopen_ops(bs, _read_ops, false);
}
--
2.31.1
Re: [RFC PATCH v2 12/44] target/i386/tdx: Finalize the TD's measurement when machine is done
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Xiaoyao Li Invoke KVM_TDX_FINALIZEMR to finalize the TD's measurement and make the TD vCPUs runnable once machine initialization is complete. Signed-off-by: Xiaoyao Li Signed-off-by: Isaku Yamahata --- target/i386/kvm/kvm.c | 7 +++ target/i386/kvm/tdx.c | 21 + target/i386/kvm/tdx.h | 3 +++ 3 files changed, 31 insertions(+) diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index be0b96b120..5742fa4806 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -53,6 +53,7 @@ #include "migration/blocker.h" #include "exec/memattrs.h" #include "trace.h" +#include "tdx.h" //#define DEBUG_KVM @@ -2246,6 +2247,12 @@ int kvm_arch_init(MachineState *ms, KVMState *s) return ret; } This is probably a good place in the series to update the comment preceding the sev_kvm_init call since TDX is now here and otherwise the comment seems untimely. Reviewed-by: Connor Kuehl
Re: [RFC PATCH v2 06/44] hw/i386: Introduce kvm-type for TDX guest
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Xiaoyao Li Introduce a machine property, kvm-type, to allow the user to create a Trusted Domain eXtensions (TDX) VM, a.k.a. a Trusted Domain (TD), e.g.: # $QEMU \ -machine ...,kvm-type=tdx \ ... Only two types are supported: "legacy" and "tdx", with "legacy" being the default. Signed-off-by: Xiaoyao Li Co-developed-by: Sean Christopherson Signed-off-by: Sean Christopherson Signed-off-by: Isaku Yamahata I am not a QEMU command line expert, so my mental model of this may be wrong, but: This seems to have a very broad scope on the command line and I am wondering if it's possible to associate it with a TDX command line object specifically to narrow its scope. I.e., is it possible to express this on the command line when launching something that is _not_ meant to be powered by TDX, such as an SEV guest? If it doesn't make sense to express that command line argument in a situation like that, perhaps it could be constrained only to the TDX-specific commandline objects.
Re: [PATCH for-6.1? 4/6] job: Add job_cancel_requested()
22.07.2021 15:26, Max Reitz wrote:
Most callers of job_is_cancelled() actually want to know whether the job
is on its way to immediate termination. For example, we refuse to pause
jobs that are cancelled; but this only makes sense for jobs that are
really actually cancelled.
A mirror job that is cancelled during READY with force=false should
absolutely be allowed to pause. This "cancellation" (which is actually
a kind of completion)
You have to repeat that this "cancel" is not "cancel".
So, the whole problem is that feature of mirror, on cancel in READY state do
not cancel but do some specific kind of completion.
You try to make this thing correctly handled on generic layer..
Did you consider instead just drop the feature from generic layer? So that all
*cancel* functions always do force-cancel. Then the internal implementation
become a lot clearer.
But we have to support the qmp block-job-cancel of READY mirror (and commit)
with force=false.
We can do it as an exclusion in qmp_block_job_cancel, something like:
if (job is mirror or commit AND it's ready AND force = false)
mirror_soft_cancel(...);
else
job_cancel(...);
may take an indefinite amount of time, and so
should behave like any job during normal operation. For example, with
on-target-error=stop, the job should stop on write errors. (In
contrast, force-cancelled jobs should not get write errors, as they
should just terminate and not do further I/O.)
Therefore, redefine job_is_cancelled() to only return true for jobs that
are force-cancelled (which as of HEAD^ means any job that interprets the
cancellation request as a request for immediate termination), and add
job_cancel_request() as the general variant, which returns true for any
jobs which have been requested to be cancelled, whether it be
immediately or after an arbitrarily long completion phase.
Buglink: https://gitlab.com/qemu-project/qemu/-/issues/462
Signed-off-by: Max Reitz
---
[..]
--- a/job.c
+++ b/job.c
@@ -216,6 +216,11 @@ const char *job_type_str(const Job *job)
}
bool job_is_cancelled(Job *job)
+{
+return job->cancelled && job->force_cancel;
+}
+
+bool job_cancel_requested(Job *job)
{
return job->cancelled;
}
@@ -650,7 +655,7 @@ static void job_conclude(Job *job)
static void job_update_rc(Job *job)
{
-if (!job->ret && job_is_cancelled(job)) {
+if (!job->ret && job_cancel_requested(job)) {
Why not job_is_cancelled() here?
So in case of mirror other kind of completion we set ret to -ECANCELED?
job->ret = -ECANCELED;
}
if (job->ret) {
@@ -704,7 +709,7 @@ static int job_finalize_single(Job *job)
/* Emit events only if we actually started */
if (job_started(job)) {
-if (job_is_cancelled(job)) {
+if (job_cancel_requested(job)) {
job_event_cancelled(job);
Same question here.. Shouldn't mirror report COMPLETED event in case of
not-force cancelled in READY state?
} else {
job_event_completed(job);
@@ -1015,7 +1020,7 @@ void job_complete(Job *job, Error **errp)
if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) {
return;
}
-if (job_is_cancelled(job) || !job->driver->complete) {
+if (job_cancel_requested(job) || !job->driver->complete) {
error_setg(errp, "The active block job '%s' cannot be completed",
job->id);
return;
@@ -1043,7 +1048,7 @@ int job_finish_sync(Job *job, void (*finish)(Job *, Error
**errp), Error **errp)
AIO_WAIT_WHILE(job->aio_context,
(job_enter(job), !job_is_completed(job)));
-ret = (job_is_cancelled(job) && job->ret == 0) ? -ECANCELED : job->ret;
+ret = (job_cancel_requested(job) && job->ret == 0) ? -ECANCELED : job->ret;
job_unref(job);
return ret;
}
--
Best regards,
Vladimir
[PATCH v3 3/5] migration: Introduce migration_ioc_[un]register_yank()
There're plenty of places in migration/* that checks against either socket or
tls typed ioc for yank operations. Provide two helpers to hide all these
information.
Reviewed-by: Dr. David Alan Gilbert
Signed-off-by: Peter Xu
---
migration/channel.c | 15 ++-
migration/multifd.c | 8 ++--
migration/qemu-file-channel.c | 8 ++--
migration/yank_functions.c| 28
migration/yank_functions.h| 2 ++
5 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/migration/channel.c b/migration/channel.c
index 01275a9162..c4fc000a1a 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -44,13 +44,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
TYPE_QIO_CHANNEL_TLS)) {
migration_tls_channel_process_incoming(s, ioc, _err);
} else {
-if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
-object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
-yank_register_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
-}
-
+migration_ioc_register_yank(ioc);
migration_ioc_process_incoming(ioc, _err);
}
@@ -94,12 +88,7 @@ void migration_channel_connect(MigrationState *s,
} else {
QEMUFile *f = qemu_fopen_channel_output(ioc);
-if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
-object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
-yank_register_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
-}
+migration_ioc_register_yank(ioc);
qemu_mutex_lock(>qemu_file_lock);
s->to_dst_file = f;
diff --git a/migration/multifd.c b/migration/multifd.c
index ab41590e71..377da78f5b 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -987,12 +987,8 @@ int multifd_load_cleanup(Error **errp)
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDRecvParams *p = _recv_state->params[i];
-if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
-&& OBJECT(p->c)->ref == 1) {
-yank_unregister_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(p->c));
+if (OBJECT(p->c)->ref == 1) {
+migration_ioc_unregister_yank(p->c);
}
object_unref(OBJECT(p->c));
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index fad340ea7a..867a5ed0c3 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -107,12 +107,8 @@ static int channel_close(void *opaque, Error **errp)
int ret;
QIOChannel *ioc = QIO_CHANNEL(opaque);
ret = qio_channel_close(ioc, errp);
-if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
- object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
-&& OBJECT(ioc)->ref == 1) {
-yank_unregister_function(MIGRATION_YANK_INSTANCE,
- migration_yank_iochannel,
- QIO_CHANNEL(ioc));
+if (OBJECT(ioc)->ref == 1) {
+migration_ioc_unregister_yank(ioc);
}
object_unref(OBJECT(ioc));
return ret;
diff --git a/migration/yank_functions.c b/migration/yank_functions.c
index 96c90e17dc..23697173ae 100644
--- a/migration/yank_functions.c
+++ b/migration/yank_functions.c
@@ -11,6 +11,9 @@
#include "qapi/error.h"
#include "io/channel.h"
#include "yank_functions.h"
+#include "qemu/yank.h"
+#include "io/channel-socket.h"
+#include "io/channel-tls.h"
void migration_yank_iochannel(void *opaque)
{
@@ -18,3 +21,28 @@ void migration_yank_iochannel(void *opaque)
qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
}
+
+/* Return whether yank is supported on this ioc */
+static bool migration_ioc_yank_supported(QIOChannel *ioc)
+{
+return object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
+}
+
+void migration_ioc_register_yank(QIOChannel *ioc)
+{
+if (migration_ioc_yank_supported(ioc)) {
+yank_register_function(MIGRATION_YANK_INSTANCE,
+ migration_yank_iochannel,
+ QIO_CHANNEL(ioc));
+}
+}
+
+void migration_ioc_unregister_yank(QIOChannel *ioc)
+{
+if (migration_ioc_yank_supported(ioc)) {
+yank_unregister_function(MIGRATION_YANK_INSTANCE,
+ migration_yank_iochannel,
+ QIO_CHANNEL(ioc));
+}
+}
diff --git
Re: [RFC PATCH v2 09/44] target/i386: kvm: don't synchronize guest tsc for TD guest
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Make kvm_synchronize_all_tsc() nop for TD-guest. s/nop/noop TDX module specification, 9.11.1 TSC Virtualization This appears in 9.12.1 of the latest revision as of this writing. https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1eas-v0.85.039.pdf "Virtual TSC values are consistent among all the TD;s VCPUs at the s/TD;s/TDs level suppored by the CPU". s/suppored/supported There is no need for qemu to synchronize tsc and VMM can't access to guest TSC. Actually do_kvm_synchronize_tsc() hits assert due to failure to write to guest tsc. qemu/target/i386/kvm.c:235: kvm_get_tsc: Assertion `ret == 1' failed. Signed-off-by: Isaku Yamahata Reviewed-by: Connor Kuehl
Re: [PATCH 0/2] acpi: pcihp: fix hotplug when bridge is wired to function > 0
On Thu, Jul 22, 2021 at 06:59:43AM -0400, Igor Mammedov wrote: > For full description see 2/2. > Tested hotplug on Q35 (see 2/2 for reproducer) and PC (with pci-bridge) > machines > > Igor Mammedov (2): > acpi: x86: pcihp: cleanup devfn usage in > build_append_pci_bus_devices() > acpi: x86: pcihp: add support hotplug on multifunction bridges > > hw/i386/acpi-build.c | 42 +- > 1 file changed, 29 insertions(+), 13 deletions(-) > In fact I think this fixes a bug in acpi hotplug on pc too. have some questions though, posted. Thanks! > 2.27.0
Re: [PATCH for-6.2 34/34] target/arm: Implement MVE interleaving loads/stores
On 7/13/21 3:37 AM, Peter Maydell wrote:
+const int off[4] = { O1, O2, O3, O4 }; \
static? uint8_t?
Otherwise, with the help of your little print program, I can confirm that these offsets
match what the pseudocode produces. I hope this while beat execution thing really pays
off in hw, because this description is nuts.
Reviewed-by: Richard Henderson
r~
Re: [RFC PATCH v2 32/44] tdx: add kvm_tdx_enabled() accessor for later use
On 7/7/21 7:55 PM, isaku.yamah...@gmail.com wrote:
From: Isaku Yamahata
Signed-off-by: Isaku Yamahata
---
include/sysemu/tdx.h | 1 +
target/i386/kvm/kvm.c | 5 +
2 files changed, 6 insertions(+)
diff --git a/include/sysemu/tdx.h b/include/sysemu/tdx.h
index 70eb01348f..f3eced10f9 100644
--- a/include/sysemu/tdx.h
+++ b/include/sysemu/tdx.h
@@ -6,6 +6,7 @@
#include "hw/i386/pc.h"
bool kvm_has_tdx(KVMState *s);
+bool kvm_tdx_enabled(void);
int tdx_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
#endif
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index af6b5f350e..76c3ea9fac 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -152,6 +152,11 @@ int kvm_set_vm_type(MachineState *ms, int kvm_type)
return -ENOTSUP;
}
+bool kvm_tdx_enabled(void)
+{
+return vm_type == KVM_X86_TDX_VM;
+}
+
Is this the whole story? Does this guarantee that the VM QEMU is
responsible to bring up is a successfully initialized TD?
From my reading of the series as it unfolded, this looks like the
function proves that KVM can support TDs and that the user requested
a TDX kvm-type, not that we have a fully-formed TD.
Is it possible to associate this with a more verifiable metric that
the TD has been or will be created successfully? I.e., once the VM
has successfully called the TDX INIT ioctl or has finalized setup?
My question mainly comes from a later patch in the series, where the
"query-tdx-capabilities" and "query-tdx" QMP commands are added.
Forgive me if I am misinterpreting the semantics of each of these
commands:
"query-tdx-capabilities" sounds like it answers the question of
"can it run a TD?"
and "query-tdx" sounds like it answers the question of "is it a TD?"
Is the assumption with "query-tdx" that anything that's gone wrong
with developing a TD will have resulted in the QEMU process exiting
and therefore if we get to a point where we can run "query-tdx" then
we know the TD was successfully formed?
[PATCH for-6.1 2/3] docs: Add documentation of Arm 'kzm' board
Add brief documentation of the Arm 'kzm' board. Signed-off-by: Peter Maydell --- docs/system/arm/kzm.rst| 18 ++ docs/system/target-arm.rst | 1 + MAINTAINERS| 1 + 3 files changed, 20 insertions(+) create mode 100644 docs/system/arm/kzm.rst diff --git a/docs/system/arm/kzm.rst b/docs/system/arm/kzm.rst new file mode 100644 index 000..bb018fbdf7c --- /dev/null +++ b/docs/system/arm/kzm.rst @@ -0,0 +1,18 @@ +Kyoto Microcomputer KZM-ARM11-01 (``kzm``) +== + +The ``kzm`` board emulates the Kyoto Microcomputer KZM-ARM11-01 +evaluation board, which is based on an NXP i.MX32 SoC +which uses an ARM1136 CPU. + +Emulated devices: + +- UARTs +- LAN9118 ethernet +- AVIC +- CCM +- GPT +- EPIT timers +- I2C +- GPIO controllers +- Watchdog timer diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst index ad3f5f435d6..d423782d661 100644 --- a/docs/system/target-arm.rst +++ b/docs/system/target-arm.rst @@ -91,6 +91,7 @@ undocumented; you can get a complete list by running arm/musicpal arm/gumstix arm/mainstone + arm/kzm arm/nrf arm/nseries arm/nuvoton diff --git a/MAINTAINERS b/MAINTAINERS index 47ddcbb7f7a..063d8e07b75 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -694,6 +694,7 @@ F: hw/*/imx_* F: hw/*/*imx31* F: include/hw/*/imx_* F: include/hw/*/*imx31* +F: docs/system/arm/kzm.rst Integrator CP M: Peter Maydell -- 2.20.1
Re: [RFC PATCH v2 34/44] target/i386/tdx: set reboot action to shutdown when tdx
On 7/7/21 7:55 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata In TDX CPU state is also protected, thus vcpu state can't be reset by VMM. It assumes -action reboot=shutdown instead of silently ignoring vcpu reset. TDX module spec version 344425-002US doesn't support vcpu reset by VMM. VM needs to be destroyed and created again to emulate REBOOT_ACTION_RESET. For simplicity, put its responsibility to management system like libvirt because it's difficult for the current qemu implementation to destroy and re-create KVM VM resources with keeping other resources. If management system wants reboot behavior for its users, it needs to - set reboot_action to REBOOT_ACTION_SHUTDOWN, - set shutdown_action to SHUTDOWN_ACTION_PAUSE optionally and, - subscribe VM state change and on reboot, (destroy qemu if SHUTDOWN_ACTION_PAUSE and) start new qemu. Signed-off-by: Isaku Yamahata --- target/i386/kvm/tdx.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c index 1316d95209..0621317b0a 100644 --- a/target/i386/kvm/tdx.c +++ b/target/i386/kvm/tdx.c @@ -25,6 +25,7 @@ #include "qapi/qapi-types-misc-target.h" #include "standard-headers/asm-x86/kvm_para.h" #include "sysemu/sysemu.h" +#include "sysemu/runstate-action.h" #include "sysemu/kvm.h" #include "sysemu/kvm_int.h" #include "sysemu/tdx.h" @@ -363,6 +364,19 @@ static void tdx_guest_init(Object *obj) qemu_mutex_init(>lock); +/* + * TDX module spec version 344425-002US doesn't support reset of vcpu by + * VMM. VM needs to be destroyed and created again to emulate + * REBOOT_ACTION_RESET. For simplicity, put its responsibility to + * management system like libvirt. + * + * Management system should + * - set reboot_action to REBOOT_ACTION_SHUTDOWN + * - set shutdown_action to SHUTDOWN_ACTION_PAUSE + * - subscribe VM state and on reboot, destroy qemu and start new qemu + */ +reboot_action = REBOOT_ACTION_SHUTDOWN; + tdx->debug = false; object_property_add_bool(obj, "debug", tdx_guest_get_debug, tdx_guest_set_debug); I think the same effect could be accomplished with modifying kvm_arch_cpu_check_are_resettable.
Re: [RFC PATCH v2 04/44] vl: Introduce machine_init_done_late notifier
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Introduce a new notifier, machine_init_done_late, that is notified after machine_init_done. This will be used by TDX to generate the HOB for its virtual firmware, which needs to be done after all guest memory has been added, i.e. after machine_init_done notifiers have run. Some code registers memory by machine_init_done(). Signed-off-by: Isaku Yamahata --- hw/core/machine.c | 26 ++ include/sysemu/sysemu.h | 2 ++ 2 files changed, 28 insertions(+) diff --git a/hw/core/machine.c b/hw/core/machine.c index ffc076ae84..66c39cf72a 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -1278,6 +1278,31 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify) notifier_remove(notify); } +static NotifierList machine_init_done_late_notifiers = +NOTIFIER_LIST_INITIALIZER(machine_init_done_late_notifiers); I think a comment here describing the difference between machine_init_done and machine_init_done_late would go a long way for other developers so they don't have to hunt through the git log. Connor
Re: [RFC PATCH v2 01/44] target/i386: Expose x86_cpu_get_supported_feature_word() for TDX
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Sean Christopherson Expose x86_cpu_get_supported_feature_word() outside of cpu.c so that it can be used by TDX to setup the VM-wide CPUID configuration. Signed-off-by: Sean Christopherson Signed-off-by: Isaku Yamahata Reviewed-by: Connor Kuehl
[PATCH for-6.1 1/3] docs: Add documentation of Arm 'mainstone' board
Add brief documentation of the Arm 'mainstone' board. Signed-off-by: Peter Maydell --- docs/system/arm/mainstone.rst | 25 + docs/system/target-arm.rst| 1 + MAINTAINERS | 1 + 3 files changed, 27 insertions(+) create mode 100644 docs/system/arm/mainstone.rst diff --git a/docs/system/arm/mainstone.rst b/docs/system/arm/mainstone.rst new file mode 100644 index 000..05310f42c7f --- /dev/null +++ b/docs/system/arm/mainstone.rst @@ -0,0 +1,25 @@ +Intel Mainstone II board (``mainstone``) + + +The ``mainstone`` board emulates the Intel Mainstone II development +board, which uses a PXA270 CPU. + +Emulated devices: + +- Flash memory +- Keypad +- MMC controller +- 91C111 ethernet +- PIC +- Timer +- DMA +- GPIO +- FIR +- Serial +- LCD controller +- SSP +- USB controller +- RTC +- PCMCIA +- I2C +- I2S diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst index c0c2585c0ad..ad3f5f435d6 100644 --- a/docs/system/target-arm.rst +++ b/docs/system/target-arm.rst @@ -90,6 +90,7 @@ undocumented; you can get a complete list by running arm/highbank arm/musicpal arm/gumstix + arm/mainstone arm/nrf arm/nseries arm/nuvoton diff --git a/MAINTAINERS b/MAINTAINERS index 4256ad1adbb..47ddcbb7f7a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -856,6 +856,7 @@ F: include/hw/arm/pxa.h F: include/hw/arm/sharpsl.h F: include/hw/display/tc6393xb.h F: docs/system/arm/xscale.rst +F: docs/system/arm/mainstone.rst SABRELITE / i.MX6 M: Peter Maydell -- 2.20.1
Re: [RFC PATCH v2 02/44] kvm: Switch KVM_CAP_READONLY_MEM to a per-VM ioctl()
On 7/7/21 7:54 PM, isaku.yamah...@gmail.com wrote: From: Isaku Yamahata Switch to making a VM ioctl() call for KVM_CAP_READONLY_MEM, which may be conditional on VM type in recent versions of KVM, e.g. when TDX is supported. kvm_vm_check_extension() has fallback from kvm_vm_ioctl() to kvm_check_extension(). fallback from VM ioctl to System ioctl for compatibility for old kernel. Signed-off-by: Isaku Yamahata --- accel/kvm/kvm-all.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index e5b10dd129..fdbe24bf59 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2531,7 +2531,7 @@ static int kvm_init(MachineState *ms) } kvm_readonly_mem_allowed = -(kvm_check_extension(s, KVM_CAP_READONLY_MEM) > 0); +(kvm_vm_check_extension(s, KVM_CAP_READONLY_MEM) > 0); kvm_eventfds_allowed = (kvm_check_extension(s, KVM_CAP_IOEVENTFD) > 0); Reviewed-by: Connor Kuehl
[PATCH for-6.1 0/3] docs: Document arm mainstone, kzm, imx25-pdk
This patchset adds brief documentation for another set of Arm boards. As usual, people familiar with these boards are welcome to provide more detail for the docs -- I just did the minimum "name the board and list emulated devices identified from a quick scan through the source code". (After this we still have 9 undocumented Arm machines.) thanks -- PMM Peter Maydell (3): docs: Add documentation of Arm 'mainstone' board docs: Add documentation of Arm 'kzm' board docs: Add documentation of Arm 'imx25-pdk' board docs/system/arm/imx25-pdk.rst | 19 +++ docs/system/arm/kzm.rst | 18 ++ docs/system/arm/mainstone.rst | 25 + docs/system/target-arm.rst| 3 +++ MAINTAINERS | 3 +++ 5 files changed, 68 insertions(+) create mode 100644 docs/system/arm/imx25-pdk.rst create mode 100644 docs/system/arm/kzm.rst create mode 100644 docs/system/arm/mainstone.rst -- 2.20.1
[PATCH for-6.1 3/3] docs: Add documentation of Arm 'imx25-pdk' board
Add brief documentation of the Arm 'imx25-pdk' board. Signed-off-by: Peter Maydell --- docs/system/arm/imx25-pdk.rst | 19 +++ docs/system/target-arm.rst| 1 + MAINTAINERS | 1 + 3 files changed, 21 insertions(+) create mode 100644 docs/system/arm/imx25-pdk.rst diff --git a/docs/system/arm/imx25-pdk.rst b/docs/system/arm/imx25-pdk.rst new file mode 100644 index 000..2a9711e8a79 --- /dev/null +++ b/docs/system/arm/imx25-pdk.rst @@ -0,0 +1,19 @@ +NXP i.MX25 PDK board (``imx25-pdk``) + + +The ``imx25-pdk`` board emulates the NXP i.MX25 Product Development Kit +board, which is based on an i.MX25 SoC which uses an ARM926 CPU. + +Emulated devices: + +- SD controller +- AVIC +- CCM +- GPT +- EPIT timers +- FEC +- RNGC +- I2C +- GPIO controllers +- Watchdog timer +- USB controllers diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst index d423782d661..91ebc26c6db 100644 --- a/docs/system/target-arm.rst +++ b/docs/system/target-arm.rst @@ -95,6 +95,7 @@ undocumented; you can get a complete list by running arm/nrf arm/nseries arm/nuvoton + arm/imx25-pdk arm/orangepi arm/palm arm/raspi diff --git a/MAINTAINERS b/MAINTAINERS index 063d8e07b75..2ea17d68f6e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -684,6 +684,7 @@ F: hw/watchdog/wdt_imx2.c F: include/hw/arm/fsl-imx25.h F: include/hw/misc/imx25_ccm.h F: include/hw/watchdog/wdt_imx2.h +F: docs/system/arm/imx25-pdk.rst i.MX31 (kzm) M: Peter Maydell -- 2.20.1
Re: [PATCH 2/2] acpi: x86: pcihp: add support hotplug on multifunction bridges
On Thu, Jul 22, 2021 at 06:59:45AM -0400, Igor Mammedov wrote:
> Commit 17858a1695 (hw/acpi/ich9: Set ACPI PCI hot-plug as default on Q35)
> switched PCI hotplug from native to ACPI one by default.
>
> That however breaks ihotplug on following CLI that used to work:
s/ihotplug/hotplug/ ?
>-nodefaults -machine q35 \
>-device
> pcie-root-port,id=pcie-root-port-0,multifunction=on,bus=pcie.0,addr=0x1,chassis=1
> \
>-device
> pcie-root-port,id=pcie-root-port-1,port=0x1,addr=0x1.0x1,bus=pcie.0,chassis=2
>
> where PCI device is hotplugged to pcie-root-port-1 with error on guest side:
>
> ACPI BIOS Error (bug): Could not resolve symbol [^S0B.PCNT], AE_NOT_FOUND
> (20201113/psargs-330)
> ACPI Error: Aborting method \_SB.PCI0.PCNT due to previous error
> (AE_NOT_FOUND) (20201113/psparse-531)
> ACPI Error: Aborting method \_GPE._E01 due to previous error (AE_NOT_FOUND)
> (20201113/psparse-531)
> ACPI Error: AE_NOT_FOUND, while evaluating GPE method [_E01]
> (20201113/evgpe-515)
>
> cause is that QEMU's ACPI hotplug never supported functions other then 0
> and due to bug it was generating notification entries for not described
> functions.
>
> Technically there is no reason not to describe cold-plugged bridges
> (root ports) on functions other then 0, as they similaraly to bridge
> on function 0 are unpluggable.
>
> Fix consists of describing cold-plugged bridges[root ports] on functions
> other than 0.
I would add: since we need to describe multifunction devices
>
> Fixes: 17858a169508609ca9063c544833e5a1adeb7b52
use short hash and include subject within ("subject here") please
> Signed-off-by: Igor Mammedov
> Reported-by: Laurent Vivier
> ---
> hw/i386/acpi-build.c | 31 ---
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b40e284b72..77cb7a338a 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -364,7 +364,7 @@ static void build_append_pcihp_notify_entry(Aml *method,
> int slot)
> int32_t devfn = PCI_DEVFN(slot, 0);
>
> if_ctx = aml_if(aml_and(aml_arg(0), aml_int(0x1U << slot), NULL));
> -aml_append(if_ctx, aml_notify(aml_name("S%.02X", devfn), aml_arg(1)));
> +aml_append(if_ctx, aml_notify(aml_name("^S%.03X", devfn), aml_arg(1)));
> aml_append(method, if_ctx);
> }
>
why are you adding a parent prefix here? anything wrong with
relying on search rules?
Seems like an unrelated change.
Also there's always 8 bit in devfn. Why do we need an extra 0?
> @@ -389,18 +389,26 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> PCIDeviceClass *pc;
> PCIDevice *pdev = bus->devices[devfn];
> int slot = PCI_SLOT(devfn);
> +int func = PCI_FUNC(devfn);
> +/* ACPI spec: 1.0b: Table 6-2 _ADR Object Bus Types, PCI type */
> +int adr = slot << 16 | func;
> bool hotplug_enabled_dev;
> bool bridge_in_acpi;
> bool cold_plugged_bridge;
>
> if (!pdev) {
> -if (bsel) { /* add hotplug slots for non present devices */
> +/*
> + * add hotplug slots for non present devices.
> + * hotplug is supported only for non-multifunction device
> + * so generate device description only for function 0
> + */
> +if (bsel && !PCI_FUNC(devfn)) {
replace with func here and elsewhere?
> if (pci_bus_is_express(bus) && slot > 0) {
> break;
> }
> -dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> +dev = aml_device("S%.03X", devfn);
> aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
> +aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
> method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> aml_append(method,
> aml_call2("PCEJ", aml_name("BSEL"), aml_name("_SUN"))
> @@ -436,9 +444,18 @@ static void build_append_pci_bus_devices(Aml
> *parent_scope, PCIBus *bus,
> continue;
> }
>
> +/*
> + * allow describing coldplugged bridges in ACPI even if they are not
> + * on function 0, as they are not unpluggale, for all other devices
unpluggable
> + * generate description only for function 0 per slot
> + */
> +if (PCI_FUNC(devfn) && !bridge_in_acpi) {
> +continue;
> +}
> +
> /* start to compose PCI slot descriptor */
> -dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> -aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
> +dev = aml_device("S%.03X", devfn);
> +aml_append(dev, aml_name_decl("_ADR", aml_int(adr)));
>
> if (bsel) {
> /*
> @@ -529,7 +546,7 @@
[PATCH v2 3/5] s390x: topology: CPU topology objects and structures
We use new objects to have a dynamic administration of the CPU topology.
The highier level object is the S390 book. In a first implementation
we will have only a single S390 book.
The book is built as a SYSBUS bridge during the CPU initialisation.
Every object under this single book will be build dynamically
immediately after a CPU has be realized if it is needed.
The CPU will fill the sockets once after the other, according to the
number of core per socket defined during the smp parsing.
Each CPU inside a socket will be represented by a bit in a 64bit
unsigned long. Set on plug and clear on unplug of a CPU.
Signed-off-by: Pierre Morel
---
hw/s390x/cpu-topology.c | 334
hw/s390x/meson.build| 1 +
hw/s390x/s390-virtio-ccw.c | 4 +
include/hw/s390x/cpu-topology.h | 67 +++
4 files changed, 406 insertions(+)
create mode 100644 hw/s390x/cpu-topology.c
create mode 100644 include/hw/s390x/cpu-topology.h
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 00..73f91d5334
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,334 @@
+/*
+ * CPU Topology
+ *
+ * Copyright 2021 IBM Corp.
+ * Author(s): Pierre Morel
+
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/sysbus.h"
+#include "hw/s390x/cpu-topology.h"
+#include "hw/qdev-properties.h"
+#include "hw/boards.h"
+#include "qemu/typedefs.h"
+#include "target/s390x/cpu.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+
+static S390TopologyCores *s390_create_cores(S390TopologySocket *socket,
+int origin)
+{
+DeviceState *dev;
+S390TopologyCores *cores;
+const MachineState *ms = MACHINE(qdev_get_machine());
+
+if (socket->bus->num_children >= ms->smp.cores) {
+return NULL;
+}
+
+dev = qdev_new(TYPE_S390_TOPOLOGY_CORES);
+qdev_realize_and_unref(dev, socket->bus, _fatal);
+
+cores = S390_TOPOLOGY_CORES(dev);
+cores->origin = origin;
+socket->cnt += 1;
+
+return cores;
+}
+
+static S390TopologySocket *s390_create_socket(S390TopologyBook *book, int id)
+{
+DeviceState *dev;
+S390TopologySocket *socket;
+const MachineState *ms = MACHINE(qdev_get_machine());
+
+if (book->bus->num_children >= ms->smp.sockets) {
+return NULL;
+}
+
+dev = qdev_new(TYPE_S390_TOPOLOGY_SOCKET);
+qdev_realize_and_unref(dev, book->bus, _fatal);
+
+socket = S390_TOPOLOGY_SOCKET(dev);
+socket->socket_id = id;
+book->cnt++;
+
+return socket;
+}
+
+static S390TopologyCores *s390_get_cores(S390TopologySocket *socket, int
origin)
+{
+S390TopologyCores *cores;
+BusChild *kid;
+
+QTAILQ_FOREACH(kid, >bus->children, sibling) {
+cores = S390_TOPOLOGY_CORES(kid->child);
+if (cores->origin == origin) {
+return cores;
+}
+}
+return s390_create_cores(socket, origin);
+}
+
+static S390TopologySocket *s390_get_socket(S390TopologyBook *book,
+ int socket_id)
+{
+S390TopologySocket *socket;
+BusChild *kid;
+
+QTAILQ_FOREACH(kid, >bus->children, sibling) {
+socket = S390_TOPOLOGY_SOCKET(kid->child);
+if (socket->socket_id == socket_id) {
+return socket;
+}
+}
+return s390_create_socket(book, socket_id);
+}
+
+/*
+ * s390_topology_new_cpu:
+ * @core_id: the core ID is machine wide
+ *
+ * We have a single book returned by s390_get_topology(),
+ * then we build the hierarchy on demand.
+ * Note that we do not destroy the hierarchy on error creating
+ * an entry in the topology, we just keept it empty.
+ * We do not need to worry about not finding a topology level
+ * entry this would have been catched during smp parsing.
+ */
+void s390_topology_new_cpu(int core_id)
+{
+const MachineState *ms = MACHINE(qdev_get_machine());
+S390TopologyBook *book;
+S390TopologySocket *socket;
+S390TopologyCores *cores;
+int cores_per_socket, sock_idx;
+int origin, bit;
+
+book = s390_get_topology();
+
+cores_per_socket = ms->smp.max_cpus / ms->smp.sockets;
+
+sock_idx = (core_id / cores_per_socket);
+socket = s390_get_socket(book, sock_idx);
+
+/*
+ * We assert that all CPU are identical IFL, shared and
+ * horizontal topology, the only reason to have several
+ * S390TopologyCores is to have more than 64 CPUs.
+ */
+origin = 64 * (core_id / 64);
+
+cores = s390_get_cores(socket, origin);
+
+bit = 63 - (core_id - origin);
+set_bit(bit, >mask);
+cores->origin = origin;
+}
+
+/*
+ * Setting the first topology: 1 book, 1 socket
+ * This is enough for 64 cores if the topology is flat (single socket)
+ */
+void
[PATCH v2 1/5] s390x: kvm: topology: Linux header update
Just as information, the linux header update patch is inside the
Linux patch series.
Signed-off-by: Pierre Morel
---
linux-headers/linux/kvm.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index bcaf66cc4d..38e96ea6f7 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1112,6 +1112,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_BINARY_STATS_FD 203
#define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
#define KVM_CAP_ARM_MTE 205
+#define KVM_CAP_S390_CPU_TOPOLOGY 206
#ifdef KVM_CAP_IRQ_ROUTING
--
2.25.1
[PATCH v2 0/5] s390x: CPU Topology
Hi, This series is a first part of the implementation of CPU topology for S390 greatly reduced from the first spin. In particular, we reduced the scope to the S390x specificities, removing all code touching to SMP or NUMA, with the goal to: - facilitate review and acceptance - let for later the SMP part currently actively discussed in mainline - be able despite the reduction of code to handle CPU topology for S390 using the current S390 topology provided by QEMU with cores and sockets only. To use these patches you will need the Linux series. You find it there: https://marc.info/?l=kvm=162697338719109=3 Currently this code is for KVM only, I have no idea if it is interesting to provide a TCG patch. If ever it will be done in another series. A short introduction CPU Topology is described in the S390 POP with essentially the description of two instructions: PTF Perform Topology function used to poll for topology change and used to set the polarization but this part is not part of this item. STSI Store System Information and the SYSIB 15.1.x providing the Topology configuration. S390 Topology is a 6 levels hierarchical topology with up to 5 level of containers. The last topology level, specifying the CPU cores. This patch series only uses the two lower levels sockets and cores. To get the information on the topology, S390 provides the STSI instruction, which stores a structures providing the list of the containers used in the Machine topology: the SYSIB. A selector within the STSI instruction allow to chose how many topology levels will be provide in the SYSIB. Using the Topology List Entries (TLE) provided inside the SYSIB we the Linux kernel is able to compute the information about the cache distance between two cores and can use this information to take scheduling decisions. Note: - Z15 reports 3 levels of containers, drawers, book, sockets as Container-TLEs above the core description inside CPU-TLEs. The Topology can be seen at several places inside zLinux: - sysfs: /sys/devices/system/cpu/cpuX/topology - procfs: /proc/sysinfo and /proc/cpuinfo - lscpu -e : gives toplogy information The different Topology levels have names: - Node - Drawer - Book - sockets or physical package - core Threads: Multithreading, is not part of the topology as described by the SYSIB 15.1.x The interest of the guest to know the CPU topology is obviously to be able to optimise the load balancing and the migration of threads. KVM will have the same interest concerning vCPUs scheduling and cache optimisation. == The design == 1) To be ready for hotplug, I chose an Object oriented design of the topology containers: - A node is a bridge on the SYSBUS and defines a "node bus" - A drawer is hotplug on the "node bus" - A book on the "drawer bus" - A socket on the "book bus" - And the CPU Topology List Entry (CPU-TLE)sits on the socket bus. These objects will be enhanced with the cache information when NUMA is implemented. This also allows for easy retrieval when building the different SYSIB for Store Topology System Information (STSI) 2) Perform Topology Function (PTF) instruction is made available to the guest with a new KVM capability and intercepted in QEMU, allowing the guest to pool for topology changes. = Features and TBD list = - There is no direct match between IDs shown by: - lscpu (unrelated numbered list), - SYSIB 15.1.x (topology ID) - The CPU number, left column of lscpu, is used to reference a CPU by Linux tools While the CPU address is used by QEMU for hotplug. - Effect of -smp parsing on the topology with an example: -smp 9,sockets=4,cores=4,maxcpus=16 We have 4 socket each holding 4 cores so that we have a maximum of 16 CPU, 9 of them are active on boot. (Should be obvious) # lscpu -e CPU NODE DRAWER BOOK SOCKET CORE L1d:L1i:L2d:L2i ONLINE CONFIGURED POLARIZATION ADDRESS 00 00 00 0:0:0:0yes yeshorizontal 0 10 00 01 1:1:1:1yes yeshorizontal 1 20 00 02 2:2:2:2yes yeshorizontal 2 30 00 03 3:3:3:3yes yeshorizontal 3 40 00 14 4:4:4:4yes yeshorizontal 4 50 00 15 5:5:5:5yes yeshorizontal 5 60 00 16 6:6:6:6yes yeshorizontal 6 70 00 17 7:7:7:7yes yeshorizontal 7 80 00 28 8:8:8:8yes yeshorizontal 8 # - To plug a new CPU inside the topology one can simply use the CPU address like in: (qemu) device_add host-s390x-cpu,core-id=12 # lscpu -e CPU NODE DRAWER
Re: [PATCH v3] migration: clear the memory region dirty bitmap when skipping free pages
On Thu, Jul 22, 2021 at 04:51:48PM +0200, David Hildenbrand wrote: > I'll give it a churn. Thanks, David. -- Peter Xu
[PATCH v2 4/5] s390x: topology: Topology list entries and SYSIB 15.x.x
We define the CPU type Topology List Entry and the
Container type Topology List Entry to implement SYSIB 15.1.x
This patch will be squatched with the next patch.
Signed-off-by: Pierre Morel
---
target/s390x/cpu.h | 44
1 file changed, 44 insertions(+)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index b26ae8fff2..d573ba205e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -564,6 +564,50 @@ typedef union SysIB {
} SysIB;
QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
+/* CPU type Topology List Entry */
+typedef struct SysIBTl_cpu {
+uint8_t nl;
+uint8_t reserved0[3];
+uint8_t reserved1:5;
+uint8_t dedicated:1;
+uint8_t polarity:2;
+uint8_t type;
+uint16_t origin;
+uint64_t mask;
+} QEMU_PACKED SysIBTl_cpu;
+
+/* Container type Topology List Entry */
+typedef struct SysIBTl_container {
+uint8_t nl;
+uint8_t reserved[6];
+uint8_t id;
+} QEMU_PACKED SysIBTl_container;
+
+/* Generic Topology List Entry */
+typedef union SysIBTl_entry {
+uint8_t nl;
+SysIBTl_container container;
+SysIBTl_cpu cpu;
+} QEMU_PACKED SysIBTl_entry;
+
+#define TOPOLOGY_NR_MAG 6
+#define TOPOLOGY_NR_MAG6 0
+#define TOPOLOGY_NR_MAG5 1
+#define TOPOLOGY_NR_MAG4 2
+#define TOPOLOGY_NR_MAG3 3
+#define TOPOLOGY_NR_MAG2 4
+#define TOPOLOGY_NR_MAG1 5
+/* Configuration topology */
+typedef struct SysIB_151x {
+uint8_t res0[2];
+uint16_t length;
+uint8_t mag[TOPOLOGY_NR_MAG];
+uint8_t res1;
+uint8_t mnest;
+uint32_t res2;
+SysIBTl_entry tle[0];
+} QEMU_PACKED SysIB_151x;
+
/* MMU defines */
#define ASCE_ORIGIN (~0xfffULL) /* segment table origin
*/
#define ASCE_SUBSPACE 0x200 /* subspace group control
*/
--
2.25.1
[PATCH v2 5/5] s390x: topology: implementating Store Topology System Information
The handling of STSI is enhenced with the interception of the
function code 15 for storing CPU topology.
Using the objects built during the pluging of CPU, we build the
SYSIB 15_1_x structures.
With this patch the maximum MNEST level is 2, this is also
the only level allowed and only SYSIB 15_1_2 will be built.
Signed-off-by: Pierre Morel
---
target/s390x/kvm/kvm.c | 101 +
1 file changed, 101 insertions(+)
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 9a0c13d4ac..0194756e6a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -52,6 +52,7 @@
#include "hw/s390x/s390-virtio-ccw.h"
#include "hw/s390x/s390-virtio-hcall.h"
#include "hw/s390x/pv.h"
+#include "hw/s390x/cpu-topology.h"
#ifndef DEBUG_KVM
#define DEBUG_KVM 0
@@ -1907,6 +1908,102 @@ static void insert_stsi_3_2_2(S390CPU *cpu, __u64 addr,
uint8_t ar)
}
}
+static int stsi_15_container(void *p, int nl, int id)
+{
+SysIBTl_container *tle = (SysIBTl_container *)p;
+
+tle->nl = nl;
+tle->id = id;
+
+return sizeof(*tle);
+}
+
+static int stsi_15_cpus(void *p, S390TopologyCores *cd)
+{
+SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
+
+tle->nl = 0;
+tle->dedicated = cd->dedicated;
+tle->polarity = cd->polarity;
+tle->type = cd->cputype;
+tle->origin = cd->origin;
+tle->mask = cd->mask;
+
+return sizeof(*tle);
+}
+
+static int set_socket(const MachineState *ms, void *p,
+ S390TopologySocket *socket)
+{
+BusChild *kid;
+int l, len = 0;
+
+len += stsi_15_container(p, 1, socket->socket_id);
+p += len;
+
+QTAILQ_FOREACH_REVERSE(kid, >bus->children, sibling) {
+l = stsi_15_cpus(p, S390_TOPOLOGY_CORES(kid->child));
+p += l;
+len += l;
+}
+return len;
+}
+
+static void insert_stsi_15_1_2(const MachineState *ms, void *p)
+{
+S390TopologyBook *book;
+SysIB_151x *sysib;
+BusChild *kid;
+int level = 2;
+int len, l;
+
+sysib = (SysIB_151x *)p;
+sysib->mnest = level;
+sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
+sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores;
+
+book = s390_get_topology();
+len = sizeof(SysIB_151x);
+p += len;
+
+QTAILQ_FOREACH_REVERSE(kid, >bus->children, sibling) {
+l = set_socket(ms, p, S390_TOPOLOGY_SOCKET(kid->child));
+p += l;
+len += l;
+}
+
+sysib->length = len;
+}
+
+static void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
+{
+const MachineState *machine = MACHINE(qdev_get_machine());
+void *p;
+int ret, cc;
+
+/*
+ * Until the SCLP STSI Facility reporting the MNEST value is used,
+ * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
+ */
+if (sel2 != 2) {
+setcc(cpu, 3);
+return;
+}
+
+p = g_malloc0(4096);
+
+ insert_stsi_15_1_2(machine, p);
+
+if (s390_is_pv()) {
+ret = s390_cpu_pv_mem_write(cpu, 0, p, 4096);
+} else {
+ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, 4096);
+}
+cc = ret ? 3 : 0;
+setcc(cpu, cc);
+g_free(p);
+}
+
static int handle_stsi(S390CPU *cpu)
{
CPUState *cs = CPU(cpu);
@@ -1920,6 +2017,10 @@ static int handle_stsi(S390CPU *cpu)
/* Only sysib 3.2.2 needs post-handling for now. */
insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
return 0;
+case 15:
+insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
+ run->s390_stsi.ar);
+return 0;
default:
return 0;
}
--
2.25.1
[PATCH v2 2/5] s390x: kvm: topology: interception of PTF instruction
Interception of the PTF instruction depending on the new
KVM_CAP_S390_CPU_TOPOLOGY KVM extension.
Signed-off-by: Pierre Morel
---
hw/s390x/s390-virtio-ccw.c | 45 ++
include/hw/s390x/s390-virtio-ccw.h | 7 +
target/s390x/kvm/kvm.c | 21 ++
3 files changed, 73 insertions(+)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e4b18aef49..500e856974 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -404,6 +404,49 @@ static void s390_pv_prepare_reset(S390CcwMachineState *ms)
s390_pv_prep_reset();
}
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra)
+{
+S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+CPUS390XState *env = >env;
+uint64_t reg = env->regs[r1];
+uint8_t fc = reg & S390_TOPO_FC_MASK;
+
+if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+s390_program_interrupt(env, PGM_OPERAND, ra);
+return 0;
+}
+
+if (env->psw.mask & PSW_MASK_PSTATE) {
+s390_program_interrupt(env, PGM_PRIVILEGED, ra);
+return 0;
+}
+
+if (reg & ~S390_TOPO_FC_MASK) {
+s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+return 0;
+}
+
+switch (fc) {
+case 0:/* Horizontal polarization is already set */
+env->regs[r1] = S390_PTF_REASON_DONE;
+return 2;
+case 1:/* Vertical polarization is not supported */
+env->regs[r1] = S390_PTF_REASON_NONE;
+return 2;
+case 2:/* Report if a topology change report is pending */
+if (ms->topology_change_report_pending) {
+ms->topology_change_report_pending = false;
+return 1;
+}
+return 0;
+default:
+s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+break;
+}
+
+return 0;
+}
+
static void s390_machine_reset(MachineState *machine)
{
S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
@@ -433,6 +476,8 @@ static void s390_machine_reset(MachineState *machine)
run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
break;
case S390_RESET_MODIFIED_CLEAR:
+/* clear topology_change_report pending condition on subsystem reset */
+ms->topology_change_report_pending = false;
/*
* Susbsystem reset needs to be done before we unshare memory
* and lose access to VIRTIO structures in guest memory.
diff --git a/include/hw/s390x/s390-virtio-ccw.h
b/include/hw/s390x/s390-virtio-ccw.h
index 3331990e02..fbde357332 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -27,9 +27,16 @@ struct S390CcwMachineState {
bool aes_key_wrap;
bool dea_key_wrap;
bool pv;
+bool topology_change_report_pending;
uint8_t loadparm[8];
};
+#define S390_PTF_REASON_NONE (0x00 << 8)
+#define S390_PTF_REASON_DONE (0x01 << 8)
+#define S390_PTF_REASON_BUSY (0x02 << 8)
+#define S390_TOPO_FC_MASK 0xffUL
+int s390_handle_ptf(S390CPU *cpu, uint8_t r1, uintptr_t ra);
+
struct S390CcwMachineClass {
/*< private >*/
MachineClass parent_class;
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b1fdb55c4..9a0c13d4ac 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -97,6 +97,7 @@
#define PRIV_B9_EQBS0x9c
#define PRIV_B9_CLP 0xa0
+#define PRIV_B9_PTF 0xa2
#define PRIV_B9_PCISTG 0xd0
#define PRIV_B9_PCILG 0xd2
#define PRIV_B9_RPCIT 0xd3
@@ -1452,6 +1453,16 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct
kvm_run *run)
}
}
+static int kvm_handle_ptf(S390CPU *cpu, struct kvm_run *run)
+{
+uint8_t r1 = (run->s390_sieic.ipb >> 20) & 0x0f;
+uint8_t ret;
+
+ret = s390_handle_ptf(cpu, r1, RA_IGNORED);
+setcc(cpu, ret);
+return 0;
+}
+
static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)
{
int r = 0;
@@ -1469,6 +1480,9 @@ static int handle_b9(S390CPU *cpu, struct kvm_run *run,
uint8_t ipa1)
case PRIV_B9_RPCIT:
r = kvm_rpcit_service_call(cpu, run);
break;
+case PRIV_B9_PTF:
+r = kvm_handle_ptf(cpu, run);
+break;
case PRIV_B9_EQBS:
/* just inject exception */
r = -1;
@@ -2470,6 +2484,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model,
Error **errp)
set_bit(S390_FEAT_DIAG_318, model->features);
}
+/*
+ * Configuration topology is partially handled in KVM
+ */
+if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
+set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
+}
+
/* strip of features that are not part of the maximum model */
bitmap_and(model->features, model->features, model->def->full_feat,
S390_FEAT_MAX);
--
2.25.1
Re: [PATCH v2 5/5] migration: Move the yank unregister of channel_close out
On Thu, Jul 22, 2021 at 06:09:03PM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (pet...@redhat.com) wrote: > > On Thu, Jul 22, 2021 at 04:27:35PM +0100, Dr. David Alan Gilbert wrote: > > > > @@ -3352,6 +3355,8 @@ static MigThrError postcopy_pause(MigrationState > > > > *s) > > > > > > > > /* Current channel is possibly broken. Release it. */ > > > > assert(s->to_dst_file); > > > > +/* Unregister yank for current channel */ > > > > +migration_ioc_unregister_yank_from_file(s->to_dst_file); > > > > > > Should this go inside the lock? > > > > Shouldn't need to; as we've got the assert() right above so otherwise we'll > > abrt otherwise :) > > Hmm OK; although with out always having to think about it then you worry > about something getting in after the assert. Right; the point is still that to_dst_file shouldn't be changed by other thread, or it'll bring some more challenge. If it can be mnodified when reaching this line, it means it can also reach earlier at assert(), then we coredump. So we should guaratee it won't happen, or we'd better also remove that assertion.. I think the challenge here is, we do have a mutex to protect the files and from that pov it's the same as other mutex. However it's different in that this mutex is also used in the oob handlers so we want to "keep it non-blocking and as small as possible for the critical sections". I didn't put it into the mutex protection because the yank code will further go to take the yank_lock so it'll make things slightly more complicated (then the file mutex is correlated to yank_lock too!). I don't think that's a problem because yank_lock is also "non-blocking" (ah! it can still block, got scheduled out, but there's no explicit things that will proactively sleep..). So since I think it's safe without the lock then I kept it outside of the lock then we don't need to discuss the safely of having it inside the critical section. (However then I noticed we still need justification on why it can be moved out..) > > > The mutex lock/unlock right below this one is not protecting us from someone > > changing it but really for being able to wait until someone finished using > > it > > then we won't crash someone else. > > > > I think the rational is to_dst_file is managed by migration thread while > > from_dst_file is managed by rp_thread. > > > > Maybe I add a comment above? > > OK, I almost pushed this further, but then I started to get worried that > we'd trace off a race with ordering on two locks with yank_lock; so yeh > lets just add a comment. Agreed. I think ordering is not a huge problem as yank_lock is very limitedly used in yank and protect yank instance/functions only, so there shouldn't be a path we take file mutex within it. Will repost shortly, thanks. -- Peter Xu
Re: [PULL 04/15] chardev-spice: add missing module_obj directive
On 7/22/21 5:36 PM, Paolo Bonzini wrote:
> The chardev-spicevmc class was not listed in chardev/spice.c, causing
> "-chardev spicevmc" to fail when modules are enabled.
>
> Reported-by: Frederic Bezies
> Fixes: 9f4a0f0978 ("modules: use modinfo for qom load", 2021-07-09)
> Resolves: //gitlab.com/qemu-project/qemu/-/issues/488
Thanks for the detail of updating to full url, however "https:'
got lost ;) Gitlab doesn't notice because of the leading '//'
I suppose. Not worth respining the pullreq.
> Signed-off-by: Paolo Bonzini
> Reviewed-by: Daniel P. Berrangé
> Message-Id: <20210719164435.1227794-1-pbonz...@redhat.com>
> Signed-off-by: Paolo Bonzini
> ---
> chardev/spice.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/chardev/spice.c b/chardev/spice.c
> index 3ffb3fdc0d..bbffef4913 100644
> --- a/chardev/spice.c
> +++ b/chardev/spice.c
> @@ -382,6 +382,7 @@ static const TypeInfo char_spicevmc_type_info = {
> .parent = TYPE_CHARDEV_SPICE,
> .class_init = char_spicevmc_class_init,
> };
> +module_obj(TYPE_CHARDEV_SPICEVMC);
>
> static void char_spiceport_class_init(ObjectClass *oc, void *data)
> {
>
Re: [PULL 0/3] SIGSEGV fixes
On Wed, 21 Jul 2021 at 22:19, Taylor Simpson wrote: > > The following changes since commit 7457b407edd6e8555e4b46488aab2f13959fccf8: > > Merge remote-tracking branch > 'remotes/thuth-gitlab/tags/pull-request-2021-07-19' into staging (2021-07-19 > 11:34:08 +0100) > > are available in the git repository at: > > https://github.com/quic/qemu tags/pull-hex-20210721 > > for you to fetch changes up to 953ea3e4b426ee0c8349343c53e3358cfec720f2: > > linux-test (tests/tcg/multiarch/linux-test.c) add check (2021-07-21 > 15:54:28 -0500) > > > The Hexagon target was silently failing the SIGSEGV test because > the signal handler was not called. > > Patch 1/3 fixes the Hexagon target > Patch 2/3 drops include qemu.h from target/hexagon/op_helper.c > Patch 3/3 adds a check that the signal handler is called > > Hi; the check added in patch 2 seems to fire about 50% of the time for qemu-riscv64, causing 'make check-tcg' to fail. $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500: SIGSEGV handler not called $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500: SIGSEGV handler not called $ ./qemu-riscv64 ./tests/tcg/riscv64-linux-user/linux-test /mnt/nvmedisk/linaro/qemu-for-merges/tests/tcg/multiarch/linux-test.c:500: SIGSEGV handler not called thanks -- PMM
Re: [PATCH v2 5/5] migration: Move the yank unregister of channel_close out
* Peter Xu (pet...@redhat.com) wrote: > On Thu, Jul 22, 2021 at 04:27:35PM +0100, Dr. David Alan Gilbert wrote: > > > @@ -3352,6 +3355,8 @@ static MigThrError postcopy_pause(MigrationState *s) > > > > > > /* Current channel is possibly broken. Release it. */ > > > assert(s->to_dst_file); > > > +/* Unregister yank for current channel */ > > > +migration_ioc_unregister_yank_from_file(s->to_dst_file); > > > > Should this go inside the lock? > > Shouldn't need to; as we've got the assert() right above so otherwise we'll > abrt otherwise :) Hmm OK; although with out always having to think about it then you worry about something getting in after the assert. > The mutex lock/unlock right below this one is not protecting us from someone > changing it but really for being able to wait until someone finished using it > then we won't crash someone else. > > I think the rational is to_dst_file is managed by migration thread while > from_dst_file is managed by rp_thread. > > Maybe I add a comment above? OK, I almost pushed this further, but then I started to get worried that we'd trace off a race with ordering on two locks with yank_lock; so yeh lets just add a comment. Dave > Thanks, > > -- > Peter Xu > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH for-6.1? 2/6] job: @force parameter for job_cancel_sync{,_all}()
22.07.2021 15:26, Max Reitz wrote: Callers should be able to specify whether they want job_cancel_sync() to force-cancel the job or not. In fact, almost all invocations do not care about consistency of the result and just want the job to terminate as soon as possible, so they should pass force=true. The replication block driver is the exception. This changes some iotest outputs, because quitting qemu while a mirror job is active will now lead to it being cancelled instead of completed, which is what we want. (Cancelling a READY mirror job with force=false may take an indefinite amount of time, which we do not want when quitting. If users want consistent results, they must have all jobs be done before they quit qemu.) Buglink:https://gitlab.com/qemu-project/qemu/-/issues/462 Signed-off-by: Max Reitz Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v3 1/4] vhost-user-rng: Add vhost-user-rng implementation
Hi Alex,
On Wed, Jul 21, 2021 at 09:52:50AM +0100, Alex Bennée wrote:
>
> Mathieu Poirier writes:
>
> > Following in the footsteps of what whas done for vhost-user-i2c
> > and virtiofsd, introduce a random number generator (RNG) backend
> > that communicates with a vhost-user server to retrieve entropy.
> > That way another VMM could be using the same vhost-user daemon and
> > avoid having to write yet another RNG driver.
> >
> > Signed-off-by: Mathieu Poirier
> > ---
> > hw/virtio/Kconfig | 5 +
> > hw/virtio/meson.build | 1 +
>
> FWIW there are simple merge failures for the meson and Kconfig parts due
> to I2C being merged.
>
Right, merging I2C before this set will definitely break things.
>
> > +
> > +rng->vhost_dev.nvqs = 1;
> > +rng->vhost_dev.vqs = g_new0(struct vhost_virtqueue,
> > rng->vhost_dev.nvqs);
> > +if (!rng->vhost_dev.vqs) {
> > +error_setg_errno(errp, -1, "memory allocation failed");
> > +goto vhost_dev_init_failed;
> > +}
>
> g_new0 will abort on memory allocation failure (which is fine for
> userspace ;-) so you don't need the check and erro handling exit here.
>
Ok, I'll fix this.
> > +
> > +ret = vhost_dev_init(>vhost_dev, >vhost_user,
> > + VHOST_BACKEND_TYPE_USER, 0, errp);
> > +if (ret < 0) {
> > +error_setg_errno(errp, -ret, "vhost_dev_init() failed");
> > +goto vhost_dev_init_failed;
> > +}
> > +
> > +qemu_chr_fe_set_handlers(>chardev, NULL, NULL, vu_rng_event, NULL,
> > + dev, NULL, true);
> > +
> > +return;
> > +
> > +vhost_dev_init_failed:
> > +virtio_delete_queue(rng->req_vq);
> > +virtio_add_queue_failed:
> > +virtio_cleanup(vdev);
> > +vhost_user_cleanup(>vhost_user);
> > +}
> > +
> > +static void vu_rng_device_unrealize(DeviceState *dev)
> > +{
> > +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > +VHostUserRNG *rng = VHOST_USER_RNG(dev);
> > +
> > +vu_rng_set_status(vdev, 0);
> > +
> > +vhost_dev_cleanup(>vhost_dev);
> > +g_free(rng->vhost_dev.vqs);
> > +rng->vhost_dev.vqs = NULL;
> > +virtio_delete_queue(rng->req_vq);
> > +virtio_cleanup(vdev);
> > +vhost_user_cleanup(>vhost_user);
> > +}
> > +
> > +static const VMStateDescription vu_rng_vmstate = {
> > +.name = "vhost-user-rng",
> > +.unmigratable = 1,
> > +};
> > +
> > +static Property vu_rng_properties[] = {
> > +DEFINE_PROP_CHR("chardev", VHostUserRNG, chardev),
> > +DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void vu_rng_class_init(ObjectClass *klass, void *data)
> > +{
> > +DeviceClass *dc = DEVICE_CLASS(klass);
> > +VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> > +
> > +device_class_set_props(dc, vu_rng_properties);
> > +dc->vmsd = _rng_vmstate;
> > +set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> > +
> > +vdc->realize = vu_rng_device_realize;
> > +vdc->unrealize = vu_rng_device_unrealize;
> > +vdc->get_features = vu_rng_get_features;
> > +vdc->set_status = vu_rng_set_status;
> > +vdc->guest_notifier_mask = vu_rng_guest_notifier_mask;
> > +vdc->guest_notifier_pending = vu_rng_guest_notifier_pending;
> > +}
> > +
> > +static const TypeInfo vu_rng_info = {
> > +.name = TYPE_VHOST_USER_RNG,
> > +.parent = TYPE_VIRTIO_DEVICE,
> > +.instance_size = sizeof(VHostUserRNG),
> > +.class_init = vu_rng_class_init,
> > +};
> > +
> > +static void vu_rng_register_types(void)
> > +{
> > +type_register_static(_rng_info);
> > +}
> > +
> > +type_init(vu_rng_register_types)
> > diff --git a/include/hw/virtio/vhost-user-rng.h
> > b/include/hw/virtio/vhost-user-rng.h
> > new file mode 100644
> > index ..071539996d1d
> > --- /dev/null
> > +++ b/include/hw/virtio/vhost-user-rng.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Vhost-user RNG virtio device
> > + *
> > + * Copyright (c) 2021 Mathieu Poirier
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#ifndef _QEMU_VHOST_USER_RNG_H
> > +#define _QEMU_VHOST_USER_RNG_H
> > +
> > +#include "hw/virtio/virtio.h"
> > +#include "hw/virtio/vhost.h"
> > +#include "hw/virtio/vhost-user.h"
> > +#include "chardev/char-fe.h"
> > +
> > +#define TYPE_VHOST_USER_RNG "vhost-user-rng"
> > +OBJECT_DECLARE_SIMPLE_TYPE(VHostUserRNG, VHOST_USER_RNG)
> > +
> > +struct VHostUserRNG {
> > +/*< private >*/
> > +VirtIODevice parent;
> > +CharBackend chardev;
> > +struct vhost_virtqueue *vhost_vq;
> > +struct vhost_dev vhost_dev;
> > +VhostUserState vhost_user;
> > +VirtQueue *req_vq;
> > +bool connected;
> > +
> > +/*< public >*/
> > +};
> > +
> > +#endif /* _QEMU_VHOST_USER_RNG_H */
>
> Otherwise:
>
> Reviewed-by: Alex Bennée
Thanks for taking the time to review this set.
>
> --
> Alex Bennée
Re: [PATCH] nbd/server: Add --selinux-label option
On Thu, Jul 22, 2021 at 05:32:40PM +0100, Richard W.M. Jones wrote:
> Under SELinux, Unix domain sockets have two labels. One is on the
> disk and can be set with commands such as chcon(1). There is a
> different label stored in memory (called the process label). This can
> only be set by the process creating the socket. When using SELinux +
> SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
> you must set both labels correctly first.
>
> For qemu-nbd the options to set the second label are awkward. You can
> create the socket in a wrapper program and then exec into qemu-nbd.
> Or you could try something with LD_PRELOAD.
>
> This commit adds the ability to set the label straightforwardly on the
> command line, via the new --selinux-label flag. (The name of the flag
> is the same as the equivalent nbdkit option.)
>
> A worked example showing how to use the new option can be found in
> this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
> Signed-off-by: Richard W.M. Jones
> ---
> configure | 9 -
> meson.build | 10 +-
> meson_options.txt | 3 +++
> qemu-nbd.c| 33 +
> 4 files changed, 53 insertions(+), 2 deletions(-)
> diff --git a/meson.build b/meson.build
> index 2f377098d7..2d7206233e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1064,6 +1064,11 @@ keyutils = dependency('libkeyutils', required: false,
>
> has_gettid = cc.has_function('gettid')
>
> +# libselinux
> +selinux = dependency('libselinux',
> + required: get_option('selinux'),
> + method: 'pkg-config', kwargs: static_kwargs)
> +
For the new build dep we'll need updated package lists in
tests/docker/dockerfiles. For centos, fedra ,opensuse
add libselinux-devel and for ubuntu 18/20 add libselinux-dev
That'll make sure this new code gets tested in CI.
The rest looks ok to me
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[PATCH] nbd/server: Add --selinux-label option
Under SELinux, Unix domain sockets have two labels. One is on the
disk and can be set with commands such as chcon(1). There is a
different label stored in memory (called the process label). This can
only be set by the process creating the socket. When using SELinux +
SVirt and wanting qemu to be able to connect to a qemu-nbd instance,
you must set both labels correctly first.
For qemu-nbd the options to set the second label are awkward. You can
create the socket in a wrapper program and then exec into qemu-nbd.
Or you could try something with LD_PRELOAD.
This commit adds the ability to set the label straightforwardly on the
command line, via the new --selinux-label flag. (The name of the flag
is the same as the equivalent nbdkit option.)
A worked example showing how to use the new option can be found in
this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938
Signed-off-by: Richard W.M. Jones
---
configure | 9 -
meson.build | 10 +-
meson_options.txt | 3 +++
qemu-nbd.c| 33 +
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/configure b/configure
index b5965b159f..7e04bd485f 100755
--- a/configure
+++ b/configure
@@ -443,6 +443,7 @@ fuse="auto"
fuse_lseek="auto"
multiprocess="auto"
slirp_smbd="$default_feature"
+selinux="auto"
malloc_trim="auto"
gio="$default_feature"
@@ -1578,6 +1579,10 @@ for opt do
;;
--disable-slirp-smbd) slirp_smbd=no
;;
+ --enable-selinux) selinux="enabled"
+ ;;
+ --disable-selinux) selinux="disabled"
+ ;;
*)
echo "ERROR: unknown option $opt"
echo "Try '$0 --help' for more information"
@@ -1965,6 +1970,7 @@ disabled with --disable-FEATURE, default is enabled if
available
multiprocessOut of process device emulation support
gio libgio support
slirp-smbd use smbd (at path --smbd=*) in slirp networking
+ selinux SELinux support in qemu-nbd
NOTE: The object files are built at the place where configure is launched
EOF
@@ -5220,7 +5226,8 @@ if test "$skip_meson" = no; then
-Dattr=$attr -Ddefault_devices=$default_devices
-Dvirglrenderer=$virglrenderer \
-Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \
-Dvhost_user_blk_server=$vhost_user_blk_server
-Dmultiprocess=$multiprocess \
--Dfuse=$fuse -Dfuse_lseek=$fuse_lseek
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\
+-Dfuse=$fuse -Dfuse_lseek=$fuse_lseek
-Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf \
+-Dselinux=$selinux \
$(if test "$default_features" = no; then echo
"-Dauto_features=disabled"; fi) \
-Dtcg_interpreter=$tcg_interpreter \
$cross_arg \
diff --git a/meson.build b/meson.build
index 2f377098d7..2d7206233e 100644
--- a/meson.build
+++ b/meson.build
@@ -1064,6 +1064,11 @@ keyutils = dependency('libkeyutils', required: false,
has_gettid = cc.has_function('gettid')
+# libselinux
+selinux = dependency('libselinux',
+ required: get_option('selinux'),
+ method: 'pkg-config', kwargs: static_kwargs)
+
# Malloc tests
malloc = []
@@ -1291,6 +1296,7 @@ config_host_data.set('CONFIG_FUSE', fuse.found())
config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found())
config_host_data.set('CONFIG_X11', x11.found())
config_host_data.set('CONFIG_CFI', get_option('cfi'))
+config_host_data.set('CONFIG_SELINUX', selinux.found())
config_host_data.set('QEMU_VERSION', '"@0@"'.format(meson.project_version()))
config_host_data.set('QEMU_VERSION_MAJOR',
meson.project_version().split('.')[0])
config_host_data.set('QEMU_VERSION_MINOR',
meson.project_version().split('.')[1])
@@ -2739,7 +2745,8 @@ if have_tools
qemu_io = executable('qemu-io', files('qemu-io.c'),
dependencies: [block, qemuutil], install: true)
qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
- dependencies: [blockdev, qemuutil, gnutls], install: true)
+ dependencies: [blockdev, qemuutil, gnutls, selinux],
+ install: true)
subdir('storage-daemon')
subdir('contrib/rdmacm-mux')
@@ -3104,6 +3111,7 @@ summary_info += {'libpmem support': libpmem.found()}
summary_info += {'libdaxctl support': libdaxctl.found()}
summary_info += {'libudev': libudev.found()}
summary_info += {'FUSE lseek':fuse_lseek.found()}
+summary_info += {'selinux': selinux.found()}
summary(summary_info, bool_yn: true, section: 'Dependencies')
if not supported_cpus.contains(cpu)
diff --git a/meson_options.txt b/meson_options.txt
index a9a9b8f4c6..a5938500a3 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -155,3 +155,6 @@ option('slirp', type: 'combo', value: 'auto',
option('fdt', type: 'combo', value: 'auto',
choices: ['disabled', 'enabled', 'auto', 'system', 'internal'],
description: 'Whether and how to
[PATCH] nbd/server: Add --selinux-label option
https://bugzilla.redhat.com/show_bug.cgi?id=1984938 The purpose of the patch is explained in the commit message / bug. In the cover I want to explain a couple of design choices. If libselinux isn't available at build time then the --selinux-label option is still present. It does not appear in the qemu-nbd --help output. If you still use it, it is ignored. (By contrast nbdkit will give an error if you try to use the option without having SELinux support. It's not clear which is better.) We give an error if setsockcreatecon_raw fails. In theory we could ignore this error (warning?) and keep going. Either SELinux would later reject clients or it wouldn't. Rich.
Re: [PATCH for-6.1 0/2] gitlab: misc tweaks to job execution rules
Self-nack. Sent the wrong version of the code - this one is broken, ignore it. On Thu, Jul 22, 2021 at 05:20:33PM +0100, Daniel P. Berrangé wrote: > - Fixes a problem with acceptance jobs running when build jobs fail > - Fixes a problem with pages job publishing website from undesirable >branches. > > Daniel P. Berrangé (2): > gitlab: only let pages be published from default branch > gitlab: don't run acceptance jobs if build jobs fail > > .gitlab-ci.d/buildtest-template.yml | 4 ++-- > .gitlab-ci.d/buildtest.yml | 18 ++ > 2 files changed, 20 insertions(+), 2 deletions(-) > > -- > 2.31.1 > > Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH for-6.1? 1/6] mirror: Keep s->synced on error
22.07.2021 15:26, Max Reitz wrote:
An error does not take us out of the READY phase, which is what
s->synced signifies. It does of course mean that source and target are
no longer in sync, but that is what s->actively_sync is for -- s->synced
never meant that source and target are in sync, only that they were at
some point (and at that point we transitioned into the READY phase).
The tangible problem is that we transition to READY once we are in sync
and s->synced is false. By resetting s->synced here, we will transition
from READY to READY once the error is resolved (if the job keeps
running), and that transition is not allowed.
Signed-off-by: Max Reitz
---
block/mirror.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/mirror.c b/block/mirror.c
index 98fc66eabf..d73b704473 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -121,7 +121,6 @@ typedef enum MirrorMethod {
static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read,
int error)
{
-s->synced = false;
s->actively_synced = false;
if (read) {
return block_job_error_action(>common, s->on_source_error,
Looked through.. Yes, seems s->synced used as "is ready". Isn't it better to drop
s->synced at all and use job_is_read() instead?
Hmm, s->actively_synced used only for assertion in active_write_settle()..
That's not wrong, just interesting.
--
Best regards,
Vladimir
[PATCH 1/2] gitlab: only let pages be published from default branch
GitLab will happily publish pages generated by the latest CI pipeline from any branch: https://docs.gitlab.com/ee/user/project/pages/introduction.html "Remember that GitLab Pages are by default branch/tag agnostic and their deployment relies solely on what you specify in .gitlab-ci.yml. You can limit the pages job with the only parameter, whenever a new commit is pushed to a branch used specifically for your pages." The current "pages" job is not limited, so it is happily publishing docs content from any branch/tag in qemu.git that gets pushed to. This means we're potentially publishing from the "staging" branch or worse from outdated "stable-NNN" branches This change restricts it to only publish from the default branch in the main repository. For contributor forks, however, we allow it to publish from any branch, since users will have arbitrarily named topic branches in flight at any time. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.d/buildtest.yml | 18 ++ 1 file changed, 18 insertions(+) diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml index 89df51517c..eaaf1189d8 100644 --- a/.gitlab-ci.d/buildtest.yml +++ b/.gitlab-ci.d/buildtest.yml @@ -663,6 +663,17 @@ build-tools-and-docs-debian: # Prepare for GitLab pages deployment. Anything copied into the # "public" directory will be deployed to $USER.gitlab.io/$PROJECT +# +# GitLab publishes from any branch that triggers a CI pipeline +# +# For the main repo we don't want to publish from 'staging' +# since that content may not be pushed, nor do we wish to +# publish from 'stable-NNN' branches as that content is outdated. +# Thus we restrict to just the default branch +# +# For contributor forks we want to publish from any repo so +# that users can see the results of their commits, regardless +# of what topic branch they're currently using pages: image: $CI_REGISTRY_IMAGE/qemu/debian-amd64:latest stage: test @@ -681,3 +692,10 @@ pages: artifacts: paths: - public + rules: +- if '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH' + when: on_success +- if '$CI_PROJECT_NAMESPACE == "qemu-project"' + when: never +- if '$CI_PROJECT_NAMESPACE != "qemu-project"' + when: on_success -- 2.31.1
[PATCH for-6.1 0/2] gitlab: misc tweaks to job execution rules
- Fixes a problem with acceptance jobs running when build jobs fail - Fixes a problem with pages job publishing website from undesirable branches. Daniel P. Berrangé (2): gitlab: only let pages be published from default branch gitlab: don't run acceptance jobs if build jobs fail .gitlab-ci.d/buildtest-template.yml | 4 ++-- .gitlab-ci.d/buildtest.yml | 18 ++ 2 files changed, 20 insertions(+), 2 deletions(-) -- 2.31.1
[PATCH 2/2] gitlab: don't run acceptance jobs if build jobs fail
The 'when: always' clause tells gitlab to run the job even if the dependant jobs have failed. This is wrong for the acceptance jobs, since we need the build artifacts from the dependant jobs. This results in the acceptance jobs given extra failures with wierd messages about missing files. Signed-off-by: Daniel P. Berrangé --- .gitlab-ci.d/buildtest-template.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml index 3e3e19d96b..fcbcc4e627 100644 --- a/.gitlab-ci.d/buildtest-template.yml +++ b/.gitlab-ci.d/buildtest-template.yml @@ -73,9 +73,9 @@ # in its namespace setting or via git-push option, see documentation # in /.gitlab-ci.yml of this repository). - if: '$CI_PROJECT_NAMESPACE == "qemu-project"' - when: always + when: on_success - if: '$QEMU_CI_AVOCADO_TESTING' - when: always + when: on_success # Otherwise, set to manual (the jobs are created but not run). - when: manual allow_failure: true -- 2.31.1
Re: [PATCH v2 5/5] migration: Move the yank unregister of channel_close out
On Thu, Jul 22, 2021 at 04:27:35PM +0100, Dr. David Alan Gilbert wrote: > > @@ -3352,6 +3355,8 @@ static MigThrError postcopy_pause(MigrationState *s) > > > > /* Current channel is possibly broken. Release it. */ > > assert(s->to_dst_file); > > +/* Unregister yank for current channel */ > > +migration_ioc_unregister_yank_from_file(s->to_dst_file); > > Should this go inside the lock? Shouldn't need to; as we've got the assert() right above so otherwise we'll abrt otherwise :) The mutex lock/unlock right below this one is not protecting us from someone changing it but really for being able to wait until someone finished using it then we won't crash someone else. I think the rational is to_dst_file is managed by migration thread while from_dst_file is managed by rp_thread. Maybe I add a comment above? Thanks, -- Peter Xu
