Re: [PATCH] hw/virtio/virtio-gpu: Fix compiler warning when compiling with -Wshadow
On Fri, Oct 6, 2023 at 8:46 PM Thomas Huth wrote:
>
> Avoid using trivial variable names in macros, otherwise we get
> the following compiler warning when compiling with -Wshadow=local:
>
> In file included from ../../qemu/hw/display/virtio-gpu-virgl.c:19:
> ../../home/thuth/devel/qemu/hw/display/virtio-gpu-virgl.c:
> In function ‘virgl_cmd_submit_3d’:
> ../../qemu/include/hw/virtio/virtio-gpu.h:228:16: error: declaration of ‘s’
> shadows a previous local [-Werror=shadow=compatible-local]
> 228 | size_t s;
> |^
> ../../qemu/hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro
> ‘VIRTIO_GPU_FILL_CMD’
> 215 | VIRTIO_GPU_FILL_CMD(cs);
> | ^~~
> ../../qemu/hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration
> is here
> 213 | size_t s;
> |^
> cc1: all warnings being treated as errors
>
> Signed-off-by: Thomas Huth
Reviewed-by: Marc-André Lureau
> ---
> include/hw/virtio/virtio-gpu.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 390c4642b8..8b7e3faf01 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -225,13 +225,13 @@ struct VhostUserGPU {
> };
>
> #define VIRTIO_GPU_FILL_CMD(out) do { \
> -size_t s; \
> -s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> +size_t s_; \
> +s_ = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> &out, sizeof(out)); \
> -if (s != sizeof(out)) { \
> +if (s_ != sizeof(out)) {\
> qemu_log_mask(LOG_GUEST_ERROR, \
>"%s: command size incorrect %zu vs %zu\n",\
> - __func__, s, sizeof(out));\
> + __func__, s_, sizeof(out)); \
> return; \
> } \
> } while (0)
> --
> 2.41.0
>
>
--
Marc-André Lureau
[PULL 2/2] target/loongarch: Add preldx instruction
Resolve the issue of starting the Loongnix 20.5[1] system failure.
Logs:
Loading Linux 4.19.0-19-loongson-3 ...
Loading initial ramdisk ...
PROGRESS CODE: V02010004 I0
PROGRESS CODE: V03101019 I0
Error: unknown opcode. 903a3e6c: 0x382c6d82
[1]
http://pkg.loongnix.cn/loongnix/isos/Loongnix-20.5/Loongnix-20.5.cartoon.gui.loongarch64.en.qcow2
Reviewed-by: Philippe Mathieu-Daudé
Signed-off-by: Song Gao
Message-Id: <20230905123910.3052023-1-gaos...@loongson.cn>
---
target/loongarch/disas.c | 7 +++
target/loongarch/insn_trans/trans_memory.c.inc | 5 +
target/loongarch/insns.decode | 3 +++
3 files changed, 15 insertions(+)
diff --git a/target/loongarch/disas.c b/target/loongarch/disas.c
index c8a29eac2b..2040f3e44d 100644
--- a/target/loongarch/disas.c
+++ b/target/loongarch/disas.c
@@ -190,6 +190,12 @@ static void output_hint_r_i(DisasContext *ctx,
arg_hint_r_i *a,
output(ctx, mnemonic, "%d, r%d, %d", a->hint, a->rj, a->imm);
}
+static void output_hint_rr(DisasContext *ctx, arg_hint_rr *a,
+ const char *mnemonic)
+{
+output(ctx, mnemonic, "%d, r%d, r%d", a->hint, a->rj, a->rk);
+}
+
static void output_i(DisasContext *ctx, arg_i *a, const char *mnemonic)
{
output(ctx, mnemonic, "%d", a->imm);
@@ -549,6 +555,7 @@ INSN(ld_bu,rr_i)
INSN(ld_hu,rr_i)
INSN(ld_wu,rr_i)
INSN(preld,hint_r_i)
+INSN(preldx, hint_rr)
INSN(fld_s,fr_i)
INSN(fst_s,fr_i)
INSN(fld_d,fr_i)
diff --git a/target/loongarch/insn_trans/trans_memory.c.inc
b/target/loongarch/insn_trans/trans_memory.c.inc
index c3de1404ea..42f4e74012 100644
--- a/target/loongarch/insn_trans/trans_memory.c.inc
+++ b/target/loongarch/insn_trans/trans_memory.c.inc
@@ -110,6 +110,11 @@ static bool trans_preld(DisasContext *ctx, arg_preld *a)
return true;
}
+static bool trans_preldx(DisasContext *ctx, arg_preldx * a)
+{
+return true;
+}
+
static bool trans_dbar(DisasContext *ctx, arg_dbar * a)
{
tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
diff --git a/target/loongarch/insns.decode b/target/loongarch/insns.decode
index 64b308f9fb..62f58cc541 100644
--- a/target/loongarch/insns.decode
+++ b/target/loongarch/insns.decode
@@ -24,6 +24,7 @@
&rrr rd rj rk
&rr_i rd rj imm
&hint_r_i hint rj imm
+&hint_rr hint rj rk
&rrr_sa rd rj rk sa
&rr_ms_ls rd rj ms ls
&ff fd fj
@@ -69,6 +70,7 @@
@rr_i16 .. imm:s16 rj:5 rd:5&rr_i
@rr_i16s2 .. rj:5 rd:5&rr_i imm=%offs16
@hint_r_i12 .. imm:s12 rj:5 hint:5&hint_r_i
+@hint_rr . rk:5 rj:5 hint:5&hint_rr
@rrr_sa2p1 ... .. rk:5 rj:5 rd:5&rrr_sa sa=%sa2p1
@rrr_sa2 ... sa:2 rk:5 rj:5 rd:5&rrr_sa
@rrr_sa3 .. sa:3 rk:5 rj:5 rd:5&rrr_sa
@@ -228,6 +230,7 @@ ldx_bu 0011 1010 0 . . .
@rrr
ldx_hu 0011 1010 01000 . . .@rrr
ldx_wu 0011 1010 1 . . .@rrr
preld 0010 101011 . . @hint_r_i12
+preldx 0011 1010 11000 . . .@hint_rr
dbar0011 1111 00100 ... @i15
ibar0011 1111 00101 ... @i15
ldptr_w 0010 0100 .. . . @rr_i14s2
--
2.25.1
[PULL 0/2] loongarch-to-apply queue
The following changes since commit 2f3913f4b2ad74baeb5a6f1d36efbd9ecdf1057d: Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging (2023-10-05 09:01:01 -0400) are available in the Git repository at: https://gitlab.com/gaosong/qemu.git tags/pull-loongarch-20231008 for you to fetch changes up to e1fc0cf1fb65c5f049bef4661d0e3278e51e2560: target/loongarch: Add preldx instruction (2023-10-08 15:02:15 +0800) pull-loongarch-20231008 Jiajie Chen (1): target/loongarch: fix ASXE flag conflict Song Gao (1): target/loongarch: Add preldx instruction target/loongarch/cpu.h | 4 ++-- target/loongarch/disas.c | 7 +++ target/loongarch/insn_trans/trans_memory.c.inc | 5 + target/loongarch/insns.decode | 3 +++ 4 files changed, 17 insertions(+), 2 deletions(-)
[PULL 1/2] target/loongarch: fix ASXE flag conflict
From: Jiajie Chen HW_FLAGS_EUEN_ASXE acccidentally conflicts with HW_FLAGS_CRMD_PG, enabling LASX instructions even when CSR_EUEN.ASXE=0. Closes: https://gitlab.com/qemu-project/qemu/-/issues/1907 Signed-off-by: Jiajie Chen Reviewed-by: Richard Henderson Reviewed-by: Song Gao Message-Id: <20230930112837.1871691-...@jia.je> Signed-off-by: Song Gao --- target/loongarch/cpu.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/target/loongarch/cpu.h b/target/loongarch/cpu.h index 40e70a8119..8b54cf109c 100644 --- a/target/loongarch/cpu.h +++ b/target/loongarch/cpu.h @@ -458,11 +458,11 @@ static inline void set_pc(CPULoongArchState *env, uint64_t value) * LoongArch CPUs hardware flags. */ #define HW_FLAGS_PLV_MASK R_CSR_CRMD_PLV_MASK /* 0x03 */ -#define HW_FLAGS_CRMD_PGR_CSR_CRMD_PG_MASK /* 0x10 */ #define HW_FLAGS_EUEN_FPE 0x04 #define HW_FLAGS_EUEN_SXE 0x08 -#define HW_FLAGS_EUEN_ASXE 0x10 +#define HW_FLAGS_CRMD_PGR_CSR_CRMD_PG_MASK /* 0x10 */ #define HW_FLAGS_VA32 0x20 +#define HW_FLAGS_EUEN_ASXE 0x40 static inline void cpu_get_tb_cpu_state(CPULoongArchState *env, vaddr *pc, uint64_t *cs_base, uint32_t *flags) -- 2.25.1
Re: [PATCH v6 3/5] vhost-user-scsi: support reconnect to backend
Sorry, the reply is late due to being on vacation for half a month.
On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
wrote:
>
> One comment on the logging stuff in vhost-scsi. As far as I can tell the
> logging in vhost-user-scsi looks good.
>
> Markus - does this look better to you? Otherwise do you think we should also
> fix up the vhost-user-blk realize function?
>
> > On Sep 22, 2023, at 7:46 AM, Li Feng wrote:
> >
> > If the backend crashes and restarts, the device is broken.
> > This patch adds reconnect for vhost-user-scsi.
> >
> > This patch also improves the error messages, and reports some silent errors.
> >
> > Tested with spdk backend.
> >
> > Signed-off-by: Li Feng
> > ---
> > hw/scsi/vhost-scsi-common.c | 16 +-
> > hw/scsi/vhost-scsi.c | 5 +-
> > hw/scsi/vhost-user-scsi.c | 204 +++---
> > include/hw/virtio/vhost-scsi-common.h | 2 +-
> > include/hw/virtio/vhost-user-scsi.h | 4 +
> > 5 files changed, 201 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > index a61cd0e907..4c8637045d 100644
> > --- a/hw/scsi/vhost-scsi-common.c
> > +++ b/hw/scsi/vhost-scsi-common.c
> > @@ -16,6 +16,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > #include "qemu/error-report.h"
> > #include "qemu/module.h"
> > #include "hw/virtio/vhost.h"
> > @@ -25,7 +26,7 @@
> > #include "hw/virtio/virtio-access.h"
> > #include "hw/fw-path-provider.h"
> >
> > -int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > +int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
> > {
> > int ret, i;
> > VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
> > @@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
> >
> > if (!k->set_guest_notifiers) {
> > -error_report("binding does not support guest notifiers");
> > +error_setg(errp, "binding does not support guest notifiers");
> > return -ENOSYS;
> > }
> >
> > ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
> > if (ret < 0) {
> > +error_setg_errno(errp, -ret, "Error enabling host notifiers");
> > return ret;
> > }
> >
> > ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
> > if (ret < 0) {
> > -error_report("Error binding guest notifier");
> > +error_setg_errno(errp, -ret, "Error binding guest notifier");
> > goto err_host_notifiers;
> > }
> >
> > @@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> >
> > ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > if (ret < 0) {
> > -error_report("Error setting inflight format: %d", -ret);
> > +error_setg_errno(errp, -ret, "Error setting inflight format");
> > goto err_guest_notifiers;
> > }
> >
> > @@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > vs->conf.virtqueue_size,
> > vsc->inflight);
> > if (ret < 0) {
> > -error_report("Error getting inflight: %d", -ret);
> > +error_setg_errno(errp, -ret, "Error getting inflight");
> > goto err_guest_notifiers;
> > }
> > }
> >
> > ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > if (ret < 0) {
> > -error_report("Error setting inflight: %d", -ret);
> > +error_setg_errno(errp, -ret, "Error setting inflight");
> > goto err_guest_notifiers;
> > }
> > }
> >
> > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > if (ret < 0) {
> > -error_report("Error start vhost dev");
> > +error_setg_errno(errp, -ret, "Error starting vhost dev");
> > goto err_guest_notifiers;
> > }
> >
> > diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> > index 443f67daa4..01a3ab4277 100644
> > --- a/hw/scsi/vhost-scsi.c
> > +++ b/hw/scsi/vhost-scsi.c
> > @@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
> > int ret, abi_version;
> > VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
> > const VhostOps *vhost_ops = vsc->dev.vhost_ops;
> > +Error *local_err = NULL;
> >
> > ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
> > if (ret < 0) {
> > @@ -88,14 +89,14 @@ static int vhost_scsi_start(VHostSCSI *s)
> > return -ENOSYS;
> > }
> >
> > -ret = vhost_scsi_common_start(vsc);
> > +ret = vhost_scsi_common_start(vsc, &local_err);
> > if (ret < 0) {
>
> Why aren’t you reporting the error here?
I will add reporting the error in the next version.
>
> > return ret;
> > }
> >
> > ret = vhost_scsi_set_endpoint(s);
> > if (ret < 0) {
> > -error_report("Error setting vhost-scsi endpoint");
> > +error_reportf_err(local_err, "Error setting vh
Re: [PATCH v6 0/5] Implement reconnect for vhost-user-scsi
On Fri, Sep 22, 2023 at 07:46:10PM +0800, Li Feng wrote: > Changes for v6: > - [PATCH] vhost-user: fix lost reconnect > - Fix missing assign event_cb. Pls don't make vN+1 a reply to vN - start a new thread with each version please. > Changes for v5: > - No logic has been changed, just move part of the code from patch 4 to patch > 5. > > Changes for v4: > - Merge > https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to > this series. > - Add ERRP_GUARD in vhost_user_scsi_realize; > - Reword the commit messages. > > Changes for v3: > - Split the vhost_user_scsi_handle_output to a separate patch; > - Move the started_vu from vhost scsi common header to vhost-user-scsi header; > - Fix a log print error; > > Changes for v2: > - Split the v1 patch to small separate patchset; > - New patch for fixing fd leak, which has sent to reviewers in another > mail; > - Implement the `vhost_user_scsi_handle_output`; > - Add the started_vu safe check; > - Fix error handler; > - Check the inflight before set/get inflight fd. > > Li Feng (5): > vhost-user-common: send get_inflight_fd once > vhost: move and rename the conn retry times > vhost-user-scsi: support reconnect to backend > vhost-user-scsi: start vhost when guest kicks > vhost-user: fix lost reconnect > > hw/block/vhost-user-blk.c | 6 +- > hw/scsi/vhost-scsi-common.c | 47 ++--- > hw/scsi/vhost-scsi.c | 5 +- > hw/scsi/vhost-user-scsi.c | 253 +++--- > hw/virtio/vhost-user-gpio.c | 5 +- > hw/virtio/vhost-user.c| 10 +- > include/hw/virtio/vhost-scsi-common.h | 2 +- > include/hw/virtio/vhost-user-scsi.h | 4 + > include/hw/virtio/vhost-user.h| 3 +- > include/hw/virtio/vhost.h | 2 + > 10 files changed, 277 insertions(+), 60 deletions(-) > > -- > 2.41.0
Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once
On Sun, Oct 08, 2023 at 04:49:05PM +0800, Li Feng wrote:
> On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
> wrote:
> >
> >
> >
> > > On Sep 22, 2023, at 7:46 AM, Li Feng wrote:
> > >
> > > Currently the get_inflight_fd will be sent every time the device is
> > > started, and
> > > the backend will allocate shared memory to save the inflight state. If the
> > > backend finds that it receives the second get_inflight_fd, it will
> > > release the
> > > previous shared memory, which breaks inflight working logic.
> > >
> > > This patch is a preparation for the following patches.
> >
> > This looks identical to the v3 patch I reviewed? If I’ve missed something
> > can you please point it out?
> Yes, nothing changed in this patch.
Then you should include tags such as reviewed/acked by for previous
version. if you drop tags you indicate to people they have to
re-review.
also, mentioning which patches changed in the cover letter is
a courtesy to reviewers.
> >
> >
> > > Signed-off-by: Li Feng
> > > ---
> > > hw/scsi/vhost-scsi-common.c | 37 ++---
> > > 1 file changed, 18 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > index a06f01af26..a61cd0e907 100644
> > > --- a/hw/scsi/vhost-scsi-common.c
> > > +++ b/hw/scsi/vhost-scsi-common.c
> > > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > >
> > > vsc->dev.acked_features = vdev->guest_features;
> > >
> > > -assert(vsc->inflight == NULL);
> > > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > > -ret = vhost_dev_get_inflight(&vsc->dev,
> > > - vs->conf.virtqueue_size,
> > > - vsc->inflight);
> > > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > > if (ret < 0) {
> > > -error_report("Error get inflight: %d", -ret);
> > > +error_report("Error setting inflight format: %d", -ret);
> > > goto err_guest_notifiers;
> > > }
> > >
> > > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > -if (ret < 0) {
> > > -error_report("Error set inflight: %d", -ret);
> > > -goto err_guest_notifiers;
> > > +if (vsc->inflight) {
> > > +if (!vsc->inflight->addr) {
> > > +ret = vhost_dev_get_inflight(&vsc->dev,
> > > +vs->conf.virtqueue_size,
> > > +vsc->inflight);
> > > +if (ret < 0) {
> > > +error_report("Error getting inflight: %d", -ret);
> > > +goto err_guest_notifiers;
> > > +}
> > > +}
> > > +
> > > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > +if (ret < 0) {
> > > +error_report("Error setting inflight: %d", -ret);
> > > +goto err_guest_notifiers;
> > > +}
> > > }
> > >
> > > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > return ret;
> > >
> > > err_guest_notifiers:
> > > -g_free(vsc->inflight);
> > > -vsc->inflight = NULL;
> > > -
> > > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > err_host_notifiers:
> > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > > }
> > > assert(ret >= 0);
> > >
> > > -if (vsc->inflight) {
> > > -vhost_dev_free_inflight(vsc->inflight);
> > > -g_free(vsc->inflight);
> > > -vsc->inflight = NULL;
> > > -}
> > > -
> > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > }
> > >
> > > --
> > > 2.41.0
> > >
> >
Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once
On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
wrote:
>
>
>
> > On Sep 22, 2023, at 7:46 AM, Li Feng wrote:
> >
> > Currently the get_inflight_fd will be sent every time the device is
> > started, and
> > the backend will allocate shared memory to save the inflight state. If the
> > backend finds that it receives the second get_inflight_fd, it will release
> > the
> > previous shared memory, which breaks inflight working logic.
> >
> > This patch is a preparation for the following patches.
>
> This looks identical to the v3 patch I reviewed? If I’ve missed something can
> you please point it out?
Yes, nothing changed in this patch.
>
>
> > Signed-off-by: Li Feng
> > ---
> > hw/scsi/vhost-scsi-common.c | 37 ++---
> > 1 file changed, 18 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > index a06f01af26..a61cd0e907 100644
> > --- a/hw/scsi/vhost-scsi-common.c
> > +++ b/hw/scsi/vhost-scsi-common.c
> > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> >
> > vsc->dev.acked_features = vdev->guest_features;
> >
> > -assert(vsc->inflight == NULL);
> > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > -ret = vhost_dev_get_inflight(&vsc->dev,
> > - vs->conf.virtqueue_size,
> > - vsc->inflight);
> > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > if (ret < 0) {
> > -error_report("Error get inflight: %d", -ret);
> > +error_report("Error setting inflight format: %d", -ret);
> > goto err_guest_notifiers;
> > }
> >
> > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > -if (ret < 0) {
> > -error_report("Error set inflight: %d", -ret);
> > -goto err_guest_notifiers;
> > +if (vsc->inflight) {
> > +if (!vsc->inflight->addr) {
> > +ret = vhost_dev_get_inflight(&vsc->dev,
> > +vs->conf.virtqueue_size,
> > +vsc->inflight);
> > +if (ret < 0) {
> > +error_report("Error getting inflight: %d", -ret);
> > +goto err_guest_notifiers;
> > +}
> > +}
> > +
> > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > +if (ret < 0) {
> > +error_report("Error setting inflight: %d", -ret);
> > +goto err_guest_notifiers;
> > +}
> > }
> >
> > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > return ret;
> >
> > err_guest_notifiers:
> > -g_free(vsc->inflight);
> > -vsc->inflight = NULL;
> > -
> > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > err_host_notifiers:
> > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > }
> > assert(ret >= 0);
> >
> > -if (vsc->inflight) {
> > -vhost_dev_free_inflight(vsc->inflight);
> > -g_free(vsc->inflight);
> > -vsc->inflight = NULL;
> > -}
> > -
> > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > }
> >
> > --
> > 2.41.0
> >
>
Re: [PATCH] contrib/vhost-user-gpu: Fix compiler warning when compiling with -Wshadow
On Fri, Oct 06, 2023 at 07:35:18PM +0200, Thomas Huth wrote:
> Rename some variables to avoid compiler warnings when compiling
> with -Wshadow=local.
>
> Signed-off-by: Thomas Huth
Reviewed-by: Michael S. Tsirkin
> ---
> contrib/vhost-user-gpu/vugpu.h | 8
> contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++---
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
> index 509b679f03..5cede45134 100644
> --- a/contrib/vhost-user-gpu/vugpu.h
> +++ b/contrib/vhost-user-gpu/vugpu.h
> @@ -164,12 +164,12 @@ struct virtio_gpu_ctrl_command {
> };
>
> #define VUGPU_FILL_CMD(out) do {\
> -size_t s; \
> -s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> +size_t s_; \
> +s_ = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> &out, sizeof(out)); \
> -if (s != sizeof(out)) { \
> +if (s_ != sizeof(out)) {\
> g_critical("%s: command size incorrect %zu vs %zu", \
> - __func__, s, sizeof(out)); \
> + __func__, s_, sizeof(out)); \
> return; \
> } \
> } while (0)
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index aa304475a0..bb41758e34 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -834,7 +834,7 @@ vg_resource_flush(VuGpu *g,
> .width = width,
> .height = height,
> };
> -pixman_image_t *i =
> +pixman_image_t *img =
> pixman_image_create_bits(pixman_image_get_format(res->image),
> msg->payload.update.width,
> msg->payload.update.height,
> @@ -842,11 +842,11 @@ vg_resource_flush(VuGpu *g,
>payload.update.data),
> width * bpp);
> pixman_image_composite(PIXMAN_OP_SRC,
> - res->image, NULL, i,
> + res->image, NULL, img,
> extents->x1, extents->y1,
> 0, 0, 0, 0,
> width, height);
> -pixman_image_unref(i);
> +pixman_image_unref(img);
> vg_send_msg(g, msg, -1);
> g_free(msg);
> }
> --
> 2.41.0
Re: [PATCH v6 1/5] vhost-user-common: send get_inflight_fd once
On Sun, Oct 8, 2023 at 4:51 PM Michael S. Tsirkin wrote:
>
> On Sun, Oct 08, 2023 at 04:49:05PM +0800, Li Feng wrote:
> > On Fri, Sep 29, 2023 at 8:55 AM Raphael Norwitz
> > wrote:
> > >
> > >
> > >
> > > > On Sep 22, 2023, at 7:46 AM, Li Feng wrote:
> > > >
> > > > Currently the get_inflight_fd will be sent every time the device is
> > > > started, and
> > > > the backend will allocate shared memory to save the inflight state. If
> > > > the
> > > > backend finds that it receives the second get_inflight_fd, it will
> > > > release the
> > > > previous shared memory, which breaks inflight working logic.
> > > >
> > > > This patch is a preparation for the following patches.
> > >
> > > This looks identical to the v3 patch I reviewed? If I’ve missed something
> > > can you please point it out?
> > Yes, nothing changed in this patch.
>
>
> Then you should include tags such as reviewed/acked by for previous
> version. if you drop tags you indicate to people they have to
> re-review.
> also, mentioning which patches changed in the cover letter is
> a courtesy to reviewers.
OK.
>
> > >
> > >
> > > > Signed-off-by: Li Feng
> > > > ---
> > > > hw/scsi/vhost-scsi-common.c | 37 ++---
> > > > 1 file changed, 18 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
> > > > index a06f01af26..a61cd0e907 100644
> > > > --- a/hw/scsi/vhost-scsi-common.c
> > > > +++ b/hw/scsi/vhost-scsi-common.c
> > > > @@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > >
> > > > vsc->dev.acked_features = vdev->guest_features;
> > > >
> > > > -assert(vsc->inflight == NULL);
> > > > -vsc->inflight = g_new0(struct vhost_inflight, 1);
> > > > -ret = vhost_dev_get_inflight(&vsc->dev,
> > > > - vs->conf.virtqueue_size,
> > > > - vsc->inflight);
> > > > +ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
> > > > if (ret < 0) {
> > > > -error_report("Error get inflight: %d", -ret);
> > > > +error_report("Error setting inflight format: %d", -ret);
> > > > goto err_guest_notifiers;
> > > > }
> > > >
> > > > -ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > > -if (ret < 0) {
> > > > -error_report("Error set inflight: %d", -ret);
> > > > -goto err_guest_notifiers;
> > > > +if (vsc->inflight) {
> > > > +if (!vsc->inflight->addr) {
> > > > +ret = vhost_dev_get_inflight(&vsc->dev,
> > > > +vs->conf.virtqueue_size,
> > > > +vsc->inflight);
> > > > +if (ret < 0) {
> > > > +error_report("Error getting inflight: %d", -ret);
> > > > +goto err_guest_notifiers;
> > > > +}
> > > > +}
> > > > +
> > > > +ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
> > > > +if (ret < 0) {
> > > > +error_report("Error setting inflight: %d", -ret);
> > > > +goto err_guest_notifiers;
> > > > +}
> > > > }
> > > >
> > > > ret = vhost_dev_start(&vsc->dev, vdev, true);
> > > > @@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
> > > > return ret;
> > > >
> > > > err_guest_notifiers:
> > > > -g_free(vsc->inflight);
> > > > -vsc->inflight = NULL;
> > > > -
> > > > k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
> > > > err_host_notifiers:
> > > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > > @@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
> > > > }
> > > > assert(ret >= 0);
> > > >
> > > > -if (vsc->inflight) {
> > > > -vhost_dev_free_inflight(vsc->inflight);
> > > > -g_free(vsc->inflight);
> > > > -vsc->inflight = NULL;
> > > > -}
> > > > -
> > > > vhost_dev_disable_notifiers(&vsc->dev, vdev);
> > > > }
> > > >
> > > > --
> > > > 2.41.0
> > > >
> > >
>
Re: [PATCH v6 0/5] Implement reconnect for vhost-user-scsi
On Sun, Oct 8, 2023 at 4:49 PM Michael S. Tsirkin wrote: > > On Fri, Sep 22, 2023 at 07:46:10PM +0800, Li Feng wrote: > > Changes for v6: > > - [PATCH] vhost-user: fix lost reconnect > > - Fix missing assign event_cb. > > > Pls don't make vN+1 a reply to vN - start a new thread > with each version please. OK. > > > Changes for v5: > > - No logic has been changed, just move part of the code from patch 4 to > > patch 5. > > > > Changes for v4: > > - Merge > > https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to > > this series. > > - Add ERRP_GUARD in vhost_user_scsi_realize; > > - Reword the commit messages. > > > > Changes for v3: > > - Split the vhost_user_scsi_handle_output to a separate patch; > > - Move the started_vu from vhost scsi common header to vhost-user-scsi > > header; > > - Fix a log print error; > > > > Changes for v2: > > - Split the v1 patch to small separate patchset; > > - New patch for fixing fd leak, which has sent to reviewers in another > > mail; > > - Implement the `vhost_user_scsi_handle_output`; > > - Add the started_vu safe check; > > - Fix error handler; > > - Check the inflight before set/get inflight fd. > > > > Li Feng (5): > > vhost-user-common: send get_inflight_fd once > > vhost: move and rename the conn retry times > > vhost-user-scsi: support reconnect to backend > > vhost-user-scsi: start vhost when guest kicks > > vhost-user: fix lost reconnect > > > > hw/block/vhost-user-blk.c | 6 +- > > hw/scsi/vhost-scsi-common.c | 47 ++--- > > hw/scsi/vhost-scsi.c | 5 +- > > hw/scsi/vhost-user-scsi.c | 253 +++--- > > hw/virtio/vhost-user-gpio.c | 5 +- > > hw/virtio/vhost-user.c| 10 +- > > include/hw/virtio/vhost-scsi-common.h | 2 +- > > include/hw/virtio/vhost-user-scsi.h | 4 + > > include/hw/virtio/vhost-user.h| 3 +- > > include/hw/virtio/vhost.h | 2 + > > 10 files changed, 277 insertions(+), 60 deletions(-) > > > > -- > > 2.41.0 >
Re: [PATCH] hw/virtio/virtio-gpu: Fix compiler warning when compiling with -Wshadow
On Fri, Oct 06, 2023 at 06:45:08PM +0200, Thomas Huth wrote:
> Avoid using trivial variable names in macros, otherwise we get
> the following compiler warning when compiling with -Wshadow=local:
>
> In file included from ../../qemu/hw/display/virtio-gpu-virgl.c:19:
> ../../home/thuth/devel/qemu/hw/display/virtio-gpu-virgl.c:
> In function ‘virgl_cmd_submit_3d’:
> ../../qemu/include/hw/virtio/virtio-gpu.h:228:16: error: declaration of ‘s’
> shadows a previous local [-Werror=shadow=compatible-local]
> 228 | size_t s;
> |^
> ../../qemu/hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro
> ‘VIRTIO_GPU_FILL_CMD’
> 215 | VIRTIO_GPU_FILL_CMD(cs);
> | ^~~
> ../../qemu/hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration
> is here
> 213 | size_t s;
> |^
> cc1: all warnings being treated as errors
>
> Signed-off-by: Thomas Huth
> ---
> include/hw/virtio/virtio-gpu.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 390c4642b8..8b7e3faf01 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -225,13 +225,13 @@ struct VhostUserGPU {
> };
>
> #define VIRTIO_GPU_FILL_CMD(out) do { \
> -size_t s; \
> -s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> +size_t s_; \
> +s_ = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> &out, sizeof(out)); \
> -if (s != sizeof(out)) { \
> +if (s_ != sizeof(out)) {\
> qemu_log_mask(LOG_GUEST_ERROR, \
>"%s: command size incorrect %zu vs %zu\n",\
> - __func__, s, sizeof(out));\
> + __func__, s_, sizeof(out)); \
> return; \
> } \
> } while (0)
This is not really enough I think. Someone might
use another macro as parameter to this macro and we'll get
a mess. We want something that's specific to this macro.
How about VIRTIO_GPU_FILL_CMD_s ?
> --
> 2.41.0
Re: [PATCH] contrib/vhost-user-gpu: Fix compiler warning when compiling with -Wshadow
On Fri, Oct 06, 2023 at 07:35:18PM +0200, Thomas Huth wrote:
> Rename some variables to avoid compiler warnings when compiling
> with -Wshadow=local.
>
> Signed-off-by: Thomas Huth
> ---
> contrib/vhost-user-gpu/vugpu.h | 8
> contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +++---
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
> index 509b679f03..5cede45134 100644
> --- a/contrib/vhost-user-gpu/vugpu.h
> +++ b/contrib/vhost-user-gpu/vugpu.h
> @@ -164,12 +164,12 @@ struct virtio_gpu_ctrl_command {
> };
>
> #define VUGPU_FILL_CMD(out) do {\
> -size_t s; \
> -s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> +size_t s_; \
> +s_ = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
> &out, sizeof(out)); \
> -if (s != sizeof(out)) { \
> +if (s_ != sizeof(out)) {\
> g_critical("%s: command size incorrect %zu vs %zu", \
> - __func__, s, sizeof(out)); \
> + __func__, s_, sizeof(out)); \
> return; \
> } \
> } while (0)
Oh wait this is the same issue. Fixes it now but can start failing
down the road. Let's use something specific to this macro.
VUGPU_FILL_CMD_s ?
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index aa304475a0..bb41758e34 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -834,7 +834,7 @@ vg_resource_flush(VuGpu *g,
> .width = width,
> .height = height,
> };
> -pixman_image_t *i =
> +pixman_image_t *img =
> pixman_image_create_bits(pixman_image_get_format(res->image),
> msg->payload.update.width,
> msg->payload.update.height,
> @@ -842,11 +842,11 @@ vg_resource_flush(VuGpu *g,
>payload.update.data),
> width * bpp);
> pixman_image_composite(PIXMAN_OP_SRC,
> - res->image, NULL, i,
> + res->image, NULL, img,
> extents->x1, extents->y1,
> 0, 0, 0, 0,
> width, height);
> -pixman_image_unref(i);
> +pixman_image_unref(img);
> vg_send_msg(g, msg, -1);
> g_free(msg);
> }
> --
> 2.41.0
[PATCH v7 3/5] vhost-user-scsi: support reconnect to backend
If the backend crashes and restarts, the device is broken.
This patch adds reconnect for vhost-user-scsi.
This patch also improves the error messages, and reports some silent errors.
Tested with spdk backend.
Signed-off-by: Li Feng
---
hw/scsi/vhost-scsi-common.c | 16 +-
hw/scsi/vhost-scsi.c | 6 +-
hw/scsi/vhost-user-scsi.c | 204 +++---
include/hw/virtio/vhost-scsi-common.h | 2 +-
include/hw/virtio/vhost-user-scsi.h | 4 +
5 files changed, 202 insertions(+), 30 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a61cd0e907..4c8637045d 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "hw/virtio/vhost.h"
@@ -25,7 +26,7 @@
#include "hw/virtio/virtio-access.h"
#include "hw/fw-path-provider.h"
-int vhost_scsi_common_start(VHostSCSICommon *vsc)
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
{
int ret, i;
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
@@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
if (!k->set_guest_notifiers) {
-error_report("binding does not support guest notifiers");
+error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
}
ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
if (ret < 0) {
+error_setg_errno(errp, -ret, "Error enabling host notifiers");
return ret;
}
ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
if (ret < 0) {
-error_report("Error binding guest notifier");
+error_setg_errno(errp, -ret, "Error binding guest notifier");
goto err_host_notifiers;
}
@@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error setting inflight format: %d", -ret);
+error_setg_errno(errp, -ret, "Error setting inflight format");
goto err_guest_notifiers;
}
@@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vs->conf.virtqueue_size,
vsc->inflight);
if (ret < 0) {
-error_report("Error getting inflight: %d", -ret);
+error_setg_errno(errp, -ret, "Error getting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
-error_report("Error setting inflight: %d", -ret);
+error_setg_errno(errp, -ret, "Error setting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
if (ret < 0) {
-error_report("Error start vhost dev");
+error_setg_errno(errp, -ret, "Error starting vhost dev");
goto err_guest_notifiers;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 443f67daa4..95cadb93e7 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
int ret, abi_version;
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
const VhostOps *vhost_ops = vsc->dev.vhost_ops;
+Error *local_err = NULL;
ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
if (ret < 0) {
@@ -88,14 +89,15 @@ static int vhost_scsi_start(VHostSCSI *s)
return -ENOSYS;
}
-ret = vhost_scsi_common_start(vsc);
+ret = vhost_scsi_common_start(vsc, &local_err);
if (ret < 0) {
+error_reportf_err(local_err, "Error starting vhost-scsi");
return ret;
}
ret = vhost_scsi_set_endpoint(s);
if (ret < 0) {
-error_report("Error setting vhost-scsi endpoint");
+error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
vhost_scsi_common_stop(vsc);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+int ret;
+
+ret = vhost_scsi_common_start(vsc, errp);
+s->started_vu = (ret < 0 ? false : true);
+
+return ret;
+}
+
+static void vhost_user_scsi_stop(VHostUserSCSI *s)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+
+if (!s->started_vu) {
+return;
+}
+s->started_vu = false;
+
+vhost_scsi_common_stop(vsc);
+}
+
static void vhost_user_scsi_se
[PATCH v7 2/5] vhost: move and rename the conn retry times
Multiple devices need this macro, move it to a common header.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/block/vhost-user-blk.c | 4 +---
hw/virtio/vhost-user-gpio.c | 3 +--
include/hw/virtio/vhost.h | 2 ++
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..3c69fa47d5 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -32,8 +32,6 @@
#include "sysemu/sysemu.h"
#include "sysemu/runstate.h"
-#define REALIZE_CONNECTION_RETRIES 3
-
static const int user_feature_bits[] = {
VIRTIO_BLK_F_SIZE_MAX,
VIRTIO_BLK_F_SEG_MAX,
@@ -482,7 +480,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
s->inflight = g_new0(struct vhost_inflight, 1);
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
assert(!*errp);
do {
if (*errp) {
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3d7fae3984..fc784e4213 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -15,7 +15,6 @@
#include "standard-headers/linux/virtio_ids.h"
#include "trace.h"
-#define REALIZE_CONNECTION_RETRIES 3
#define VHOST_NVQS 2
/* Features required from VirtIO */
@@ -365,7 +364,7 @@ static void vu_gpio_device_realize(DeviceState *dev, Error
**errp)
qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
dev, NULL, true);
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
g_assert(!*errp);
do {
if (*errp) {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 6a173cb9fa..ca3131b1af 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -8,6 +8,8 @@
#define VHOST_F_DEVICE_IOTLB 63
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VU_REALIZE_CONN_RETRIES 3
+
/* Generic structures common for any vhost based device. */
struct vhost_inflight {
--
2.41.0
[PATCH v7 5/5] vhost-user: fix lost reconnect
When the vhost-user is reconnecting to the backend, and if the vhost-user fails
at the get_features in vhost_dev_init(), then the reconnect will fail
and it will not be retriggered forever.
The reason is:
When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
immediately.
vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
The reconnect path is:
vhost_user_blk_event
vhost_user_async_close(.. vhost_user_blk_disconnect ..)
qemu_chr_fe_set_handlers <- clear the notifier callback
schedule vhost_user_async_close_bh
The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
called, then the event fd callback will not be reinstalled.
All vhost-user devices have this issue, including vhost-user-blk/scsi.
With this patch, if the vdev->vdev is null, the fd callback will still
be reinstalled.
Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/block/vhost-user-blk.c | 2 +-
hw/scsi/vhost-user-scsi.c | 3 ++-
hw/virtio/vhost-user-gpio.c| 2 +-
hw/virtio/vhost-user.c | 10 --
include/hw/virtio/vhost-user.h | 3 ++-
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3c69fa47d5..95c758200d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent
event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &s->chardev, &s->dev,
- vhost_user_blk_disconnect);
+ vhost_user_blk_disconnect,
vhost_user_blk_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 5afb514398..dbe864c0e5 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -234,7 +234,8 @@ static void vhost_user_scsi_event(void *opaque,
QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
- vhost_user_scsi_disconnect);
+ vhost_user_scsi_disconnect,
+ vhost_user_scsi_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index fc784e4213..aff2d7eff6 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -289,7 +289,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
- vu_gpio_disconnect);
+ vu_gpio_disconnect, vu_gpio_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 3766b415f8..7395bfc531 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2765,6 +2765,7 @@ typedef struct {
DeviceState *dev;
CharBackend *cd;
struct vhost_dev *vhost;
+IOEventHandler *event_cb;
} VhostAsyncCallback;
static void vhost_user_async_close_bh(void *opaque)
@@ -2779,7 +2780,10 @@ static void vhost_user_async_close_bh(void *opaque)
*/
if (vhost->vdev) {
data->cb(data->dev);
-}
+} else if (data->event_cb) {
+qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
+ NULL, data->dev, NULL, true);
+ }
g_free(data);
}
@@ -2791,7 +2795,8 @@ static void vhost_user_async_close_bh(void *opaque)
*/
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
-vu_async_close_fn cb)
+vu_async_close_fn cb,
+IOEventHandler *event_cb)
{
if (!runstate_check(RUN_STATE_SHUTDOWN)) {
/*
@@ -2807,6 +2812,7 @@ void vhost_user_async_close(DeviceState *d,
data->dev = d;
data->cd = chardev;
data->vhost = vhost;
+data->event_cb = event_cb;
/* Disable any further notifications on the chardev */
qemu_chr_fe_set_handlers(chardev,
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 9f9ddf878d..6b06ecb1bd 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -106,6 +106,7 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
-
[PATCH v7 4/5] vhost-user-scsi: start vhost when guest kicks
Let's keep the same behavior as vhost-user-blk.
Some old guests kick virtqueue before setting VIRTIO_CONFIG_S_DRIVER_OK.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/scsi/vhost-user-scsi.c | 48 +++
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 5df24faff4..5afb514398 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -111,8 +111,48 @@ static void vhost_user_scsi_reset(VirtIODevice *vdev)
}
}
-static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
{
+VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+
+Error *local_err = NULL;
+int i, ret;
+
+if (!vdev->start_on_kick) {
+return;
+}
+
+if (!s->connected) {
+return;
+}
+
+if (vhost_dev_is_started(&vsc->dev)) {
+return;
+}
+
+/*
+ * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
+ * vhost here instead of waiting for .set_status().
+ */
+ret = vhost_user_scsi_start(s, &local_err);
+if (ret < 0) {
+error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
+qemu_chr_fe_disconnect(&vs->conf.chardev);
+return;
+}
+
+/* Kick right away to begin processing requests already in vring */
+for (i = 0; i < vsc->dev.nvqs; i++) {
+VirtQueue *kick_vq = virtio_get_queue(vdev, i);
+
+if (!virtio_queue_get_desc_addr(vdev, i)) {
+continue;
+}
+event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
+}
}
static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
@@ -242,9 +282,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error
**errp)
return;
}
-virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
- vhost_dummy_handle_output,
- vhost_dummy_handle_output, &err);
+virtio_scsi_common_realize(dev, vhost_user_scsi_handle_output,
+ vhost_user_scsi_handle_output,
+ vhost_user_scsi_handle_output, &err);
if (err != NULL) {
error_propagate(errp, err);
return;
--
2.41.0
[PATCH v7 0/5] Implement reconnect for vhost-user-scsi
Changes for v7: - [PATCH 3/5] vhost-user-scsi: support reconnect to backend - Add reporting the error in vhost-scsi; - Rebase to master and fix the conflict. - Add "Reviewed-by" tags. Changes for v6: - [PATCH] vhost-user: fix lost reconnect - Fix missing assign event_cb. Changes for v5: - No logic has been changed, just move part of the code from patch 4 to patch 5. Changes for v4: - Merge https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to this series. - Add ERRP_GUARD in vhost_user_scsi_realize; - Reword the commit messages. Changes for v3: - Split the vhost_user_scsi_handle_output to a separate patch; - Move the started_vu from vhost scsi common header to vhost-user-scsi header; - Fix a log print error; Changes for v2: - Split the v1 patch to small separate patchset; - New patch for fixing fd leak, which has sent to reviewers in another mail; - Implement the `vhost_user_scsi_handle_output`; - Add the started_vu safe check; - Fix error handler; - Check the inflight before set/get inflight fd. Li Feng (5): vhost-user-common: send get_inflight_fd once vhost: move and rename the conn retry times vhost-user-scsi: support reconnect to backend vhost-user-scsi: start vhost when guest kicks vhost-user: fix lost reconnect hw/block/vhost-user-blk.c | 6 +- hw/scsi/vhost-scsi-common.c | 47 ++--- hw/scsi/vhost-scsi.c | 6 +- hw/scsi/vhost-user-scsi.c | 253 +++--- hw/virtio/vhost-user-gpio.c | 5 +- hw/virtio/vhost-user.c| 10 +- include/hw/virtio/vhost-scsi-common.h | 2 +- include/hw/virtio/vhost-user-scsi.h | 4 + include/hw/virtio/vhost-user.h| 3 +- include/hw/virtio/vhost.h | 2 + 10 files changed, 278 insertions(+), 60 deletions(-) -- 2.41.0
[PATCH v7 1/5] vhost-user-common: send get_inflight_fd once
Currently the get_inflight_fd will be sent every time the device is started, and
the backend will allocate shared memory to save the inflight state. If the
backend finds that it receives the second get_inflight_fd, it will release the
previous shared memory, which breaks inflight working logic.
This patch is a preparation for the following patches.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/scsi/vhost-scsi-common.c | 37 ++---
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a06f01af26..a61cd0e907 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vsc->dev.acked_features = vdev->guest_features;
-assert(vsc->inflight == NULL);
-vsc->inflight = g_new0(struct vhost_inflight, 1);
-ret = vhost_dev_get_inflight(&vsc->dev,
- vs->conf.virtqueue_size,
- vsc->inflight);
+ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error get inflight: %d", -ret);
+error_report("Error setting inflight format: %d", -ret);
goto err_guest_notifiers;
}
-ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
-if (ret < 0) {
-error_report("Error set inflight: %d", -ret);
-goto err_guest_notifiers;
+if (vsc->inflight) {
+if (!vsc->inflight->addr) {
+ret = vhost_dev_get_inflight(&vsc->dev,
+vs->conf.virtqueue_size,
+vsc->inflight);
+if (ret < 0) {
+error_report("Error getting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
+}
+
+ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
+if (ret < 0) {
+error_report("Error setting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
@@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
return ret;
err_guest_notifiers:
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-
k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
err_host_notifiers:
vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
}
assert(ret >= 0);
-if (vsc->inflight) {
-vhost_dev_free_inflight(vsc->inflight);
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-}
-
vhost_dev_disable_notifiers(&vsc->dev, vdev);
}
--
2.41.0
RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
Hi Eric,
>-Original Message-
>From: Eric Auger
>Sent: Wednesday, October 4, 2023 11:44 PM
>Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>
>Let the vfio-ccw device use vfio_attach_device() and
>vfio_detach_device(), hence hiding the details of the used
>IOMMU backend.
>
>Note that the migration reduces the following trace
>"vfio: subchannel %s has already been attached" (featuring
>cssid.ssid.devid) into "device is already attached"
>
>Also now all the devices have been migrated to use the new
>vfio_attach_device/vfio_detach_device API, let's turn the
>legacy functions into static functions, local to container.c.
>
>Signed-off-by: Eric Auger
>Signed-off-by: Yi Liu
>Signed-off-by: Zhenzhong Duan
>Reviewed-by: Matthew Rosato
>
>---
>
>v3:
>- simplified vbasedev->dev setting
>
>v2 -> v3:
>- Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev
> while keeping into account Matthew's comment
> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>2b6b31678...@linux.ibm.com/
>---
> include/hw/vfio/vfio-common.h | 5 --
> hw/vfio/ccw.c | 122 +-
> hw/vfio/common.c | 10 +--
> 3 files changed, 37 insertions(+), 100 deletions(-)
>
>diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>index 12fbfbc37d..c486bdef2a 100644
>--- a/include/hw/vfio/vfio-common.h
>+++ b/include/hw/vfio/vfio-common.h
>@@ -202,7 +202,6 @@ typedef struct {
> hwaddr pages;
> } VFIOBitmap;
>
>-void vfio_put_base_device(VFIODevice *vbasedev);
> void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>@@ -220,11 +219,7 @@ void vfio_region_unmap(VFIORegion *region);
> void vfio_region_exit(VFIORegion *region);
> void vfio_region_finalize(VFIORegion *region);
> void vfio_reset_handler(void *opaque);
>-VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
>-void vfio_put_group(VFIOGroup *group);
> struct vfio_device_info *vfio_get_device_info(int fd);
>-int vfio_get_device(VFIOGroup *group, const char *name,
>-VFIODevice *vbasedev, Error **errp);
> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>AddressSpace *as, Error **errp);
> void vfio_detach_device(VFIODevice *vbasedev);
>diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>index 1e2fce83b0..6ec35fedc9 100644
>--- a/hw/vfio/ccw.c
>+++ b/hw/vfio/ccw.c
>@@ -572,88 +572,15 @@ static void vfio_ccw_put_region(VFIOCCWDevice
>*vcdev)
> g_free(vcdev->io_region);
> }
>
>-static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
>-{
>-g_free(vcdev->vdev.name);
>-vfio_put_base_device(&vcdev->vdev);
>-}
>-
>-static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
>-Error **errp)
>-{
>-S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
>-char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>- cdev->hostid.ssid,
>- cdev->hostid.devid);
>-VFIODevice *vbasedev;
>-
>-QLIST_FOREACH(vbasedev, &group->device_list, next) {
>-if (strcmp(vbasedev->name, name) == 0) {
>-error_setg(errp, "vfio: subchannel %s has already been attached",
>- name);
>-goto out_err;
>-}
>-}
>-
>-/*
>- * All vfio-ccw devices are believed to operate in a way compatible with
>- * discarding of memory in RAM blocks, ie. pages pinned in the host are
>- * in the current working set of the guest driver and therefore never
>- * overlap e.g., with pages available to the guest balloon driver. This
>- * needs to be set before vfio_get_device() for vfio common to handle
>- * ram_block_discard_disable().
>- */
>-vcdev->vdev.ram_block_discard_allowed = true;
>-
>-if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) {
>-goto out_err;
>-}
>-
>-vcdev->vdev.ops = &vfio_ccw_ops;
>-vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
>-vcdev->vdev.name = name;
>-vcdev->vdev.dev = DEVICE(vcdev);
>-
>-return;
>-
>-out_err:
>-g_free(name);
>-}
>-
>-static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
>-{
>-char *tmp, group_path[PATH_MAX];
>-ssize_t len;
>-int groupid;
>-
>-tmp = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s/iommu_group",
>- cdev->hostid.cssid, cdev->hostid.ssid,
>- cdev->hostid.devid, cdev->mdevid);
>-len = readlink(tmp, group_path, sizeof(group_path));
>-g_free(tmp);
>-
>-if (len <= 0 || len >= sizeof(group_path)) {
>-error_setg(errp, "vfio: no iommu_group found");
>-return NULL;
>-}
>-
>-group_path[len] = 0;
>-
>-if (sscanf(basename(group_path), "%d", &groupid) != 1) {
>-error_setg(errp, "
Re: [PATCH 1/3] via-ide: Fix legacy mode emulation
On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
On 05/10/2023 23:13, BALATON Zoltan wrote:
The initial value for BARs were set in reset method for emulating
legacy mode at start but this does not work because PCI code resets
BARs after calling device reset method.
This is certainly something I've noticed when testing previous versions of
the VIA patches. Perhaps it's worth a separate thread to the PCI devs?
I think I brought up this back then but was told current PCI code won't
change and since that could break everything else that makes sense so this
is something that we should take as given and accomodate that.
Additionally the values
written to BARs were also wrong.
I don't believe this is correct: according to the datasheet the values on
reset are the ones given in the current reset code, so even if the reset
function is overridden at a later data during PCI bus reset, I would leave
these for now since it is a different issue.
Those values are missing the IO space bit for one so they can't be correct
as a BAR value no matter what the datasheet says. And since they are
ineffective now I think it's best to remove them to avoid confusion.
Move setting the BARs to a callback on writing the PCI config regsiter
that sets the compatibility mode (which firmwares needing this mode
seem to do) and fix their values to program it to use legacy port
numbers. As noted in a comment, we only do this when the BARs were
unset before, because logs from real machine show this is how real
chip works, even if it contradicts the data sheet which is not very
clear about this.
Signed-off-by: BALATON Zoltan
---
hw/ide/via.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/hw/ide/via.c b/hw/ide/via.c
index fff23803a6..8186190207 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -132,11 +132,6 @@ static void via_ide_reset(DeviceState *dev)
pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_FAST_BACK |
PCI_STATUS_DEVSEL_MEDIUM);
-pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, 0x01f0);
-pci_set_long(pci_conf + PCI_BASE_ADDRESS_1, 0x03f4);
-pci_set_long(pci_conf + PCI_BASE_ADDRESS_2, 0x0170);
-pci_set_long(pci_conf + PCI_BASE_ADDRESS_3, 0x0374);
-pci_set_long(pci_conf + PCI_BASE_ADDRESS_4, 0xcc01); /* BMIBA:
20-23h */
pci_set_long(pci_conf + PCI_INTERRUPT_LINE, 0x010e);
/* IDE chip enable, IDE configuration 1/2, IDE FIFO Configuration*/
@@ -159,6 +154,35 @@ static void via_ide_reset(DeviceState *dev)
pci_set_long(pci_conf + 0xc0, 0x00020001);
}
+static void via_ide_cfg_write(PCIDevice *pd, uint32_t addr,
+ uint32_t val, int len)
+{
+pci_default_write_config(pd, addr, val, len);
+/*
+ * Only set BARs if they are unset. Logs from real hardware show that
+ * writing class_prog to enable compatibility mode after BARs were set
+ * (possibly by firmware) it will use ports set by BARs not ISA ports
+ * (e.g. pegasos2 Linux does this and calls it non-100% native mode).
Can you remind me again where the references are to non-100% native mode? The
only thing I can find in Linux is
https://github.com/torvalds/linux/blob/master/arch/powerpc/platforms/chrp/pci.c#L360
but that simply forces a switch to legacy mode, with no mention of "non-100%
native mode".
It was discussed somewhere in the via-ide thread we had when this was last
touched for pegasos2 in March 2020. Basically the non-100% native mode is
when ports are set by BARs but IRQs are still hard coded to 14-15. Linux
can work with all 3 possible modes: legacy (both ports and IRQs are hard
coded to ISA values), native (using BARs and PCI config 0x3c for a single
interrupt for both channels, vt82c686 data sheet does not document this
but vt8231 has a comment saying native mode only) and non-100% native mode
where BARs are effective to set port addresses but IRQs don't respect 0x3c
but use 14-15 as in legacy mode. Some machines only work in non-100%
native mode such as pegasos2 and Linux has some quirks for this. Other
OSes running on this machine work with what the firmware has set up and
can't work with anything else so we need to emulate what those OSes want
(which matches real hardware) because Linux can usually cope anyway. On
pegasso2 MorphOS uses BARs but expects IRQ 14-15 which is what the
firmware also sets up by setting mode to native in the PCI config of the
IDE func yet IRQs are fixed at 14-15. Linux forces its driver to use
legacy interrupts by setting mode back to legacy but it still uses BARs
and this is what it calls non-100% native mode. On amigaone firmware sets
legacy mode and AmigaOS does not change this but uses it with legacy ports
and IRQs. Linux finds the same and uses legacy mode on amigaone.
+ * But if 0x8a is written after reset without setting BARs then we
want
+ * legacy ports (this is done by AmigaOne fi
Re: [PATCH v3 03/10] migration: Refactor error handling in source return path
On 5/10/23 18:05, Peter Xu wrote:
On Thu, Oct 05, 2023 at 08:11:33AM +0200, Philippe Mathieu-Daudé wrote:
Hi Peter,
On 5/10/23 00:02, Peter Xu wrote:
rp_state.error was a boolean used to show error happened in return path
thread. That's not only duplicating error reporting (migrate_set_error),
but also not good enough in that we only do error_report() and set it to
true, we never can keep a history of the exact error and show it in
query-migrate.
To make this better, a few things done:
- Use error_setg() rather than error_report() across the whole lifecycle
of return path thread, keeping the error in an Error*.
- Use migrate_set_error() to apply that captured error to the global
migration object when error occured in this thread.
- With above, no need to have mark_source_rp_bad(), remove it, alongside
with rp_state.error itself.
Signed-off-by: Peter Xu
---
migration/migration.h | 1 -
migration/ram.h| 5 +-
migration/migration.c | 123 ++---
migration/ram.c| 41 +++---
migration/trace-events | 4 +-
5 files changed, 79 insertions(+), 95 deletions(-)
-int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block)
+int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *block, Error **errp)
{
int ret = -EINVAL;
/* from_dst_file is always valid because we're within rp_thread */
@@ -4193,16 +4194,16 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock
*block)
ret = qemu_file_get_error(file);
if (ret || size != local_size) {
-error_report("%s: read bitmap failed for ramblock '%s': %d"
- " (size 0x%"PRIx64", got: 0x%"PRIx64")",
- __func__, block->idstr, ret, local_size, size);
+error_setg(errp, "read bitmap failed for ramblock '%s': %d"
+ " (size 0x%"PRIx64", got: 0x%"PRIx64")",
+ block->idstr, ret, local_size, size);
ret = -EIO;
goto out;
}
if (end_mark != RAMBLOCK_RECV_BITMAP_ENDING) {
-error_report("%s: ramblock '%s' end mark incorrect: 0x%"PRIx64,
- __func__, block->idstr, end_mark);
+error_setg(errp, "ramblock '%s' end mark incorrect: 0x%"PRIx64,
+ block->idstr, end_mark);
ret = -EINVAL;
goto out;
}
This function returns -EIO/-EINVAL errors, propagated to its 2 callers
- migrate_handle_rp_recv_bitmap()
- migrate_handle_rp_resume_ack()
It was only called in migrate_handle_rp_recv_bitmap(), but I think I see
what you meant..
which are only used in source_return_path_thread() where the return
value is only checked as boolean.
Could we simplify them returning a boolean (which is the pattern with
functions taking an Error** as last parameter)?
Yes, with errp passed in, the "int" retcode is slightly duplicated. I can
add one more patch on top of this as further cleanup, as below.
Thanks,
===8<===
From b1052befd72beb129012afddf5647339fe4e257c Mon Sep 17 00:00:00 2001
From: Peter Xu
Date: Thu, 5 Oct 2023 12:03:44 -0400
Subject: [PATCH] migration: Change ram_dirty_bitmap_reload() retval to bool
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Now we have a Error** passed into the return path thread stack, which is
even clearer than an int retval. Change ram_dirty_bitmap_reload() and the
callers to use a bool instead to replace errnos.
Suggested-by: Philippe Mathieu-Daudé
Signed-off-by: Peter Xu
---
migration/ram.h | 2 +-
migration/migration.c | 18 +-
migration/ram.c | 24 +++-
3 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/migration/ram.h b/migration/ram.h
index 14ed666d58..af0290f8ab 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -72,7 +72,7 @@ void ramblock_recv_bitmap_set(RAMBlock *rb, void *host_addr);
void ramblock_recv_bitmap_set_range(RAMBlock *rb, void *host_addr, size_t nr);
int64_t ramblock_recv_bitmap_send(QEMUFile *file,
const char *block_name);
-int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *rb, Error **errp);
+bool ram_dirty_bitmap_reload(MigrationState *s, RAMBlock *rb, Error **errp);
bool ramblock_page_is_discarded(RAMBlock *rb, ram_addr_t start);
void postcopy_preempt_shutdown_file(MigrationState *s);
void *postcopy_preempt_thread(void *opaque);
diff --git a/migration/migration.c b/migration/migration.c
index 1a7f214fcf..e7375810be 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1837,29 +1837,29 @@ static void migrate_handle_rp_req_pages(MigrationState
*ms, const char* rbname,
ram_save_queue_pages(rbname, start, len, errp);
}
-static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name,
- Error **errp)
+static bool migrate_handle_rp_recv_b
[PATCH] migration: Use g_autofree to simplify ram_dirty_bitmap_reload()
Signed-off-by: Philippe Mathieu-Daudé
---
Based-on:
---
migration/ram.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 982fbbeee1..4cb948cfd0 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4164,11 +4164,11 @@ bool ram_dirty_bitmap_reload(MigrationState *s,
RAMBlock *block, Error **errp)
{
/* from_dst_file is always valid because we're within rp_thread */
QEMUFile *file = s->rp_state.from_dst_file;
-unsigned long *le_bitmap, nbits = block->used_length >> TARGET_PAGE_BITS;
+unsigned long *le_bitmap = NULL;
+unsigned long nbits = block->used_length >> TARGET_PAGE_BITS;
uint64_t local_size = DIV_ROUND_UP(nbits, 8);
uint64_t size, end_mark;
RAMState *rs = ram_state;
-bool result = false;
trace_ram_dirty_bitmap_reload_begin(block->idstr);
@@ -4193,7 +4193,7 @@ bool ram_dirty_bitmap_reload(MigrationState *s, RAMBlock
*block, Error **errp)
if (size != local_size) {
error_setg(errp, "ramblock '%s' bitmap size mismatch (0x%"PRIx64
" != 0x%"PRIx64")", block->idstr, size, local_size);
-goto out;
+return false;
}
size = qemu_get_buffer(file, (uint8_t *)le_bitmap, local_size);
@@ -4203,13 +4203,13 @@ bool ram_dirty_bitmap_reload(MigrationState *s,
RAMBlock *block, Error **errp)
error_setg(errp, "read bitmap failed for ramblock '%s': "
"(size 0x%"PRIx64", got: 0x%"PRIx64")",
block->idstr, local_size, size);
-goto out;
+return false;
}
if (end_mark != RAMBLOCK_RECV_BITMAP_ENDING) {
error_setg(errp, "ramblock '%s' end mark incorrect: 0x%"PRIx64,
block->idstr, end_mark);
-goto out;
+return false;
}
/*
@@ -4241,10 +4241,7 @@ bool ram_dirty_bitmap_reload(MigrationState *s, RAMBlock
*block, Error **errp)
*/
migration_rp_kick(s);
-result = true;
-out:
-g_free(le_bitmap);
-return result;
+return true;
}
static int ram_resume_prepare(MigrationState *s, void *opaque)
--
2.41.0
Re: [PATCH] hw/virtio/virtio-gpu: Fix compiler warning when compiling with -Wshadow
On 8/10/23 10:57, Michael S. Tsirkin wrote:
On Fri, Oct 06, 2023 at 06:45:08PM +0200, Thomas Huth wrote:
Avoid using trivial variable names in macros, otherwise we get
the following compiler warning when compiling with -Wshadow=local:
In file included from ../../qemu/hw/display/virtio-gpu-virgl.c:19:
../../home/thuth/devel/qemu/hw/display/virtio-gpu-virgl.c:
In function ‘virgl_cmd_submit_3d’:
../../qemu/include/hw/virtio/virtio-gpu.h:228:16: error: declaration of ‘s’
shadows a previous local [-Werror=shadow=compatible-local]
228 | size_t s;
|^
../../qemu/hw/display/virtio-gpu-virgl.c:215:5: note: in expansion of macro
‘VIRTIO_GPU_FILL_CMD’
215 | VIRTIO_GPU_FILL_CMD(cs);
| ^~~
../../qemu/hw/display/virtio-gpu-virgl.c:213:12: note: shadowed declaration
is here
213 | size_t s;
|^
cc1: all warnings being treated as errors
Signed-off-by: Thomas Huth
---
include/hw/virtio/virtio-gpu.h | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 390c4642b8..8b7e3faf01 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -225,13 +225,13 @@ struct VhostUserGPU {
};
#define VIRTIO_GPU_FILL_CMD(out) do { \
-size_t s; \
-s = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
+size_t s_; \
+s_ = iov_to_buf(cmd->elem.out_sg, cmd->elem.out_num, 0, \
&out, sizeof(out)); \
-if (s != sizeof(out)) { \
+if (s_ != sizeof(out)) {\
qemu_log_mask(LOG_GUEST_ERROR, \
"%s: command size incorrect %zu vs %zu\n",\
- __func__, s, sizeof(out));\
+ __func__, s_, sizeof(out)); \
return; \
} \
} while (0)
This is not really enough I think. Someone might
use another macro as parameter to this macro and we'll get
a mess. We want something that's specific to this macro.
How about VIRTIO_GPU_FILL_CMD_s ?
Or unmacroize as:
virtio_gpu_fill_cmd(struct virtio_gpu_ctrl_command *cmd,
const void *data, size_t size);
Re: [PATCH v7 3/5] vhost-user-scsi: support reconnect to backend
Hello Li, I have some trivial style comments you could possibly address
in a next version:
On Sun, 08 Oct 2023 12:12, Li Feng wrote:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..5df24faff4 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+int ret;
+
+ret = vhost_scsi_common_start(vsc, errp);
+s->started_vu = (ret < 0 ? false : true);
-+s->started_vu = (ret < 0 ? false : true);
++s->started_vu = !(ret < 0);
static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t
status)
{
VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
-+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(vdev);
+static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
+{
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+VHostUserSCSI *s = VHOST_USER_SCSI(vdev);
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+int ret = 0;
+
+if (s->connected) {
+return 0;
+}
+s->connected = true;
+
+vsc->dev.num_queues = vs->conf.num_queues;
+vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
+vsc->dev.vqs = s->vhost_vqs;
+vsc->dev.vq_index = 0;
+vsc->dev.backend_features = 0;
+
+ret = vhost_dev_init(&vsc->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
+ errp);
+if (ret < 0) {
+return ret;
+}
+
+/* restore vhost state */
+if (virtio_device_started(vdev, vdev->status)) {
+ret = vhost_user_scsi_start(s, errp);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
-+if (virtio_device_started(vdev, vdev->status)) {
-+ret = vhost_user_scsi_start(s, errp);
-+if (ret < 0) {
-+return ret;
-+}
-+}
-+
-+return 0;
-+}
++if (virtio_device_started(vdev, vdev->status)) {
++ret = vhost_user_scsi_start(s, errp);
++}
++
++return ret;
++}
[skipping..]
+static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
+{
+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
-+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
++DeviceState *dev = DEVICE(s);
diff --git a/include/hw/virtio/vhost-user-scsi.h
b/include/hw/virtio/vhost-user-scsi.h
index 521b08e559..b405ec952a 100644
--- a/include/hw/virtio/vhost-user-scsi.h
+++ b/include/hw/virtio/vhost-user-scsi.h
@@ -29,6 +29,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserSCSI, VHOST_USER_SCSI)
struct VHostUserSCSI {
VHostSCSICommon parent_obj;
VhostUserState vhost_user;
+bool connected;
+bool started_vu;
+
+struct vhost_virtqueue *vhost_vqs;
+bool connected;
+bool started_vu;
-+
+struct vhost_virtqueue *vhost_vqs;
See
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations
The definition should look like:
struct VHostUserSCSI {
VHostSCSICommon parent_obj;
/* Properties */
bool connected;
bool started_vu;
VhostUserState vhost_user;
struct vhost_virtqueue *vhost_vqs;
}
[PATCH QEMU] tulip: Fix LXT970 PHY registers
From: Dmitry Borisov
Fix incorrect MII status value (0xf02c).
Use default values from a 21143-based board:
https://www.beowulf.org/pipermail/tulip-bug/2000-February/000485.html
Signed-off-by: Dmitry Borisov
---
hw/net/tulip.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
mode change 100644 => 100755 hw/net/tulip.c
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
old mode 100644
new mode 100755
index 915e5fb595..43e8f4bcb5
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -415,14 +415,15 @@ static void tulip_update_rs(TULIPState *s, int state)
trace_tulip_rx_state(tulip_rx_state_name(state));
}
+/* LEVEL1 LXT970 PHY registers */
static uint16_t tulip_mdi_default[] = {
/* MDI Registers 0 - 6, 7 */
-0x3100, 0xf02c, 0x7810, 0x, 0x0501, 0x4181, 0x, 0x,
+0x1000, 0x782d, 0x7810, 0x0001, 0x01e1, 0x41e1, 0x0001, 0x,
/* MDI Registers 8 - 15 */
0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
/* MDI Registers 16 - 31 */
-0x0003, 0x, 0x0001, 0x, 0x, 0x, 0x, 0x,
-0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x4000, 0x, 0x38c8, 0x0010, 0x, 0x0002,
+0x0001, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
};
/* Readonly mask for MDI (PHY) registers */
--
2.38.5
[PATCH QEMU] tulip: Fix LXT970 PHY registers
From: Dmitry Borisov
Fix incorrect MII status value (0xf02c).
Use default values from a 21143-based board:
https://www.beowulf.org/pipermail/tulip-bug/2000-February/000485.html
Signed-off-by: Dmitry Borisov
---
hw/net/tulip.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
mode change 100644 => 100755 hw/net/tulip.c
diff --git a/hw/net/tulip.c b/hw/net/tulip.c
old mode 100644
new mode 100755
index 915e5fb595..43e8f4bcb5
--- a/hw/net/tulip.c
+++ b/hw/net/tulip.c
@@ -415,14 +415,15 @@ static void tulip_update_rs(TULIPState *s, int state)
trace_tulip_rx_state(tulip_rx_state_name(state));
}
+/* LEVEL1 LXT970 PHY registers */
static uint16_t tulip_mdi_default[] = {
/* MDI Registers 0 - 6, 7 */
-0x3100, 0xf02c, 0x7810, 0x, 0x0501, 0x4181, 0x, 0x,
+0x1000, 0x782d, 0x7810, 0x0001, 0x01e1, 0x41e1, 0x0001, 0x,
/* MDI Registers 8 - 15 */
0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
/* MDI Registers 16 - 31 */
-0x0003, 0x, 0x0001, 0x, 0x, 0x, 0x, 0x,
-0x, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x4000, 0x, 0x38c8, 0x0010, 0x, 0x0002,
+0x0001, 0x, 0x, 0x, 0x, 0x, 0x, 0x,
};
/* Readonly mask for MDI (PHY) registers */
--
2.38.5
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: ltaylorsimp...@gmail.com > Sent: Friday, October 6, 2023 11:01 AM > To: Brian Cain ; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > WARNING: This email originated from outside of Qualcomm. Please be wary of > any links or attachments, and do not enable macros. > > > -Original Message- > > From: Brian Cain > > Sent: Thursday, October 5, 2023 4:22 PM > > To: qemu-devel@nongnu.org > > Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org; > > phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the > > identifiers to avoid shadowing the type name. > > > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > > rename the function arguments to avoid shadowing the global. > > > > Signed-off-by: Brian Cain > > --- > > target/hexagon/genptr.c | 56 - > > target/hexagon/genptr.h | 18 > > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > > target/hexagon/op_helper.c | 4 +- > > 5 files changed, 42 insertions(+), 42 deletions(-) > > > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > > 217bc7bb5a..11377ac92b 100644 > > --- a/target/hexagon/genptr.c > > +++ b/target/hexagon/genptr.c > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, > TCGv > > src) > > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > > mem_index) > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > > +mem_index) > > I'd recommend moving both the type and the arg name to the new line, also > indent the new line. > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, > int > mem_index) > > > > > > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int > > mem_index) > > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int > > +mem_index) > > Ditto > > > static inline void gen_store_conditional4(DisasContext *ctx, > > - TCGv pred, TCGv vaddr, TCGv src) > > + TCGv pred, TCGv v_addr, TCGv > > + src) > > Ditto > > > zero = tcg_constant_tl(0); > > @@ -374,13 +374,13 @@ static inline void > > gen_store_conditional4(DisasContext *ctx, } > > > > static inline void gen_store_conditional8(DisasContext *ctx, > > - TCGv pred, TCGv vaddr, TCGv_i64 > > src) > > + TCGv pred, TCGv v_addr, > > + TCGv_i64 src) > > Indent > > > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > > slot) > > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > > +slot) > > Ditto > > > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > > slot); > > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > > +slot); > > Ditto I could be mistaken but AFAICT none of these lines are wrapped in the way they're quoted above in my patch (nor the baseline). I don't think any of these lines exceed 80 columns, so they shouldn't need wrapping, either. I double checked how it's displayed at the archive https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to make sure that it wasn't a misconfiguration of my mailer. For another perspective - refer to the commit used to create this patch: https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121a3eef71e6 Is your review comment that "these lines should be wrapped and when you do, make sure you do it like this"? Or "if you are going to wrap them, wrap them like this"? Or something else? > Otherwise, > Reviewed-by: Taylor Simpson >
Re: [PATCH 15/19] parallels: Remove unnecessary data_end field
On 10/7/23 19:54, Mike Maslenkin wrote:
On Sat, Oct 7, 2023 at 5:30 PM Alexander Ivanov
wrote:
On 10/7/23 13:21, Mike Maslenkin wrote:
On Sat, Oct 7, 2023 at 1:18 PM Alexander Ivanov
wrote:
On 10/6/23 21:43, Mike Maslenkin wrote:
On Mon, Oct 2, 2023 at 12:01 PM Alexander Ivanov
wrote:
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48ea5b3f03..80a7171b84 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,6 +265,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -275,7 +282,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = prealloc_clusters * s->cluster_size;
@@ -297,9 +304,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
first_free);
@@ -315,8 +319,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
* branch. In the other case we are likely re-using hole.
Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -757,13 +760,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -806,7 +803,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
@@ -1361,8 +1357,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between
BAT
* and actual data. We can't avoid read-modify-write...
@@ -1403,11 +1398,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
@@ -1461,7 +1455,6 @@ static int
parallels_truncate_unused_clusters(BlockDriverState *bs)
end_off = (end_off + 1) * s->cluster_size;
}
end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0,
NULL);
}
diff --git a/block/parallels.h b/block/parallels.h
index 18b4f8068e..a6a048d890 100644
--- a/block/parallels.h
+++ b/block/parallel
Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
Hi Zhenzhong,
On 10/8/23 12:21, Duan, Zhenzhong wrote:
> Hi Eric,
>
>> -Original Message-
>> From: Eric Auger
>> Sent: Wednesday, October 4, 2023 11:44 PM
>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>>
>> Let the vfio-ccw device use vfio_attach_device() and
>> vfio_detach_device(), hence hiding the details of the used
>> IOMMU backend.
>>
>> Note that the migration reduces the following trace
>> "vfio: subchannel %s has already been attached" (featuring
>> cssid.ssid.devid) into "device is already attached"
>>
>> Also now all the devices have been migrated to use the new
>> vfio_attach_device/vfio_detach_device API, let's turn the
>> legacy functions into static functions, local to container.c.
>>
>> Signed-off-by: Eric Auger
>> Signed-off-by: Yi Liu
>> Signed-off-by: Zhenzhong Duan
>> Reviewed-by: Matthew Rosato
>>
>> ---
>>
>> v3:
>> - simplified vbasedev->dev setting
>>
>> v2 -> v3:
>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev
>> while keeping into account Matthew's comment
>> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>> 2b6b31678...@linux.ibm.com/
>> ---
>> include/hw/vfio/vfio-common.h | 5 --
>> hw/vfio/ccw.c | 122 +-
>> hw/vfio/common.c | 10 +--
>> 3 files changed, 37 insertions(+), 100 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 12fbfbc37d..c486bdef2a 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -202,7 +202,6 @@ typedef struct {
>> hwaddr pages;
>> } VFIOBitmap;
>>
>> -void vfio_put_base_device(VFIODevice *vbasedev);
>> void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>> @@ -220,11 +219,7 @@ void vfio_region_unmap(VFIORegion *region);
>> void vfio_region_exit(VFIORegion *region);
>> void vfio_region_finalize(VFIORegion *region);
>> void vfio_reset_handler(void *opaque);
>> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
>> -void vfio_put_group(VFIOGroup *group);
>> struct vfio_device_info *vfio_get_device_info(int fd);
>> -int vfio_get_device(VFIOGroup *group, const char *name,
>> -VFIODevice *vbasedev, Error **errp);
>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>AddressSpace *as, Error **errp);
>> void vfio_detach_device(VFIODevice *vbasedev);
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 1e2fce83b0..6ec35fedc9 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -572,88 +572,15 @@ static void vfio_ccw_put_region(VFIOCCWDevice
>> *vcdev)
>> g_free(vcdev->io_region);
>> }
>>
>> -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
>> -{
>> -g_free(vcdev->vdev.name);
>> -vfio_put_base_device(&vcdev->vdev);
>> -}
>> -
>> -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
>> -Error **errp)
>> -{
>> -S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
>> -char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>> - cdev->hostid.ssid,
>> - cdev->hostid.devid);
>> -VFIODevice *vbasedev;
>> -
>> -QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> -if (strcmp(vbasedev->name, name) == 0) {
>> -error_setg(errp, "vfio: subchannel %s has already been
>> attached",
>> - name);
>> -goto out_err;
>> -}
>> -}
>> -
>> -/*
>> - * All vfio-ccw devices are believed to operate in a way compatible with
>> - * discarding of memory in RAM blocks, ie. pages pinned in the host are
>> - * in the current working set of the guest driver and therefore never
>> - * overlap e.g., with pages available to the guest balloon driver. This
>> - * needs to be set before vfio_get_device() for vfio common to handle
>> - * ram_block_discard_disable().
>> - */
>> -vcdev->vdev.ram_block_discard_allowed = true;
>> -
>> -if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) {
>> -goto out_err;
>> -}
>> -
>> -vcdev->vdev.ops = &vfio_ccw_ops;
>> -vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
>> -vcdev->vdev.name = name;
>> -vcdev->vdev.dev = DEVICE(vcdev);
>> -
>> -return;
>> -
>> -out_err:
>> -g_free(name);
>> -}
>> -
>> -static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
>> -{
>> -char *tmp, group_path[PATH_MAX];
>> -ssize_t len;
>> -int groupid;
>> -
>> -tmp = g_strdup_printf("/sys/bus/css/devices/%x.%x.%04x/%s/iommu_group",
>> - cdev->hostid.cssid, cdev->hostid.ssid,
>> - cdev->hostid.devid, cdev->mdevid);
>> -len = readlink(tmp, group_path, sizeof(group_path
Re: [PATCH v8 00/29] Consolidate PIIX south bridges
On 10/7/23 8:38 AM, Bernhard Beschow wrote: > This series consolidates the implementations of the PIIX3 and PIIX4 south > bridges and makes PIIX4 usable in the PC machine via an experimental command > line parameter. The motivation is to resolve duplicate code between the device > models as well as resolving the "Frankenstein" PIIX4-PM problem in PIIX3 > discussed on this list before. > > The series is structured as follows: > > Patches 1-8 are preparational patches necessary for moving all sub devices > into > PIIX3, like was done for PIIX4. In isolation these patches can also be seen as > general x86 machine cleanup sub series which has merit in its own right -- and > could be applied to master if the remainder of the series takes longer to > review. > > Patches 9-13 move PIIX3 sub devices into one device model like already > done for PIIX4. Together with the previous sub series these patches form a > bigger sub series which also has merit in its own right, and could be applied > independent of the remainder of this series as well. > > The remainder of this series consolidates the PIIX3 and PIIX4 device models. > The culmination point is the last commit which makes PIIX4 usable in the PC > machine. > > One challenge was dealing with optional devices where Peter already gave > advice > in [1] which this series implements. Although PIIX4 is now usable in the PC > machine it still has a different binary layout in its VM state. > > Testing done: > * `make check` > * `qemu-system-x86_64 -M pc -m 2G -accel kvm -cdrom > manjaro-kde-21.3.2-220704-linux515.iso` > * `qemu-system-x86_64 -M pc,x-south-bridge=piix4-isa -m 2G -accel kvm -cdrom > manjaro-kde-21.3.2-220704-linux515.iso` > * `qemu-system-x86_64 -M q35 -m 2G -accel kvm -cdrom > manjaro-kde-21.3.2-220704-linux515.iso` > * `qemu-system-mips64el -M malta -cpu 5KEc -m 1G -kernel kernel -initrd initrd > -append "root=LABEL=rootfs console=ttyS0" -drive file=image.qcow2` > * `qemu-system-mips64el -M malta -bios yamon-02.22.bin` > * Run HVM domU guest under Xen with manjaro-kde-21.3.2-220704-linux515.iso > image I did some preliminary tests of this patch series on some Xen HVM domU guests I have that use the xenfv / pc machine and depend on the current PIIX3 implementation. So far there are no regressions in my tests. I use libxl or libvirt to manage the Xen guests. I have not (yet) tested the experimental option that makes PIIX4 useable in the xenfv / pc machines. IIUC, that would require a patch to hvmloader/pci.c in Xen tools so Xen's hvmloader recognizes the PIIX4 pci device id [1], and a patch to libxl so libxl can optionally launch qemu with the new experimental option enabled. Since this patch series affects the xenfv machine, I added the Xen x86 maintainers to the Cc list and Jason Andryuk who is credited with discovering the necessary patch to hvmloader/pci.c. [1] https://lore.kernel.org/qemu-devel/b0ff78f4-1193-495b-919c-84a1ff8ad...@gmail.com/ > > v8: > - Wire ISA interrupts before device realization > - Optionally allow a PIC and PIT to be instantiated in PIIX3 for compatiblity > with PIIX4 > - Touch ICH9 LPC as far as required for PIIX consolidation > - Make PIIX4 usable in the PC machine via an experimental option > - Review and rework history, touching every commit and drop R-b tags when > changes became too large > > v7: > - Rebase onto master > - Avoid the PIC proxy (Phil) > The motivation for the PIC proxy was to allow for wiring up ISA interrupts > in > the south bridges. ISA interrupt wiring requires the GPIO lines to be > populated already but pc_piix assigned the interrupts only after realizing > PIIX3. By shifting interrupt assignment before realizing, the ISA interrupts > are already populated during PIIX3's realize phase where the ISA interrupts > are wired up. > - New patches: > * hw/isa/piix4: Reuse struct PIIXState from PIIX3 > * hw/isa/piix4: Create the "intr" property during init() already > - Patches with substantial changes (Reviewed-by dropped): > * hw/isa/piix3: Move ISA bus IRQ assignments into host device > > v6: > - Fix some comments about TYPE_ISA_PIC (Mark) ... and use it consistently > within the patch series. > - Incorporate series "[PATCH v2 0/3] Decouple INTx-to-LNKx routing from south > bridges" [2] for maintainer convenience. > - Merge v5's 'hw/i386/pc_piix: Associate pci_map_irq_fn as soon as PCI bus is > created' into > https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03312.html . Do > similar for Malta. > - Rebase onto latest master (d6271b657286 "Merge tag 'for_upstream' of > https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging") > > v5: > - Pick up Reviewed-by tags from > https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg00116.html > - Add patch to make usage of the isa_pic global more type-safe > - Re-introduce isa-pic as PIC specific proxy (Mark) > > v4: > - Rebase onto "[PATCH v2 0/3] Decouple IN
Re: [PATCH 2/3] hw/pci-host: Add emulation of Mai Logic Articia S
On Sun, 8 Oct 2023, Mark Cave-Ayland wrote:
On 05/10/2023 23:13, BALATON Zoltan wrote:
The Articia S is a generic chipset supporting several different CPUs
that were used on some PPC boards. This is a minimal emulation of the
parts needed for emulating the AmigaOne board.
Signed-off-by: BALATON Zoltan
---
hw/pci-host/Kconfig | 5 +
hw/pci-host/articia.c | 266 ++
hw/pci-host/meson.build | 2 +
include/hw/pci-host/articia.h | 17 +++
4 files changed, 290 insertions(+)
create mode 100644 hw/pci-host/articia.c
create mode 100644 include/hw/pci-host/articia.h
diff --git a/hw/pci-host/Kconfig b/hw/pci-host/Kconfig
index a07070eddf..33014c80a4 100644
--- a/hw/pci-host/Kconfig
+++ b/hw/pci-host/Kconfig
@@ -73,6 +73,11 @@ config SH_PCI
bool
select PCI
+config ARTICIA
+bool
+select PCI
+select I8259
+
config MV64361
bool
select PCI
diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
new file mode 100644
index 00..80558e1c47
--- /dev/null
+++ b/hw/pci-host/articia.c
@@ -0,0 +1,266 @@
+/*
+ * Mai Logic Articia S emulation
+ *
+ * Copyright (c) 2023 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "hw/pci/pci_device.h"
+#include "hw/pci/pci_host.h"
+#include "hw/irq.h"
+#include "hw/i2c/bitbang_i2c.h"
+#include "hw/intc/i8259.h"
+#include "hw/pci-host/articia.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
+
+OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
+struct ArticiaHostState {
+PCIDevice parent_obj;
+
+ArticiaState *as;
+};
+
+/* TYPE_ARTICIA */
+
+struct ArticiaState {
+PCIHostState parent_obj;
+
+qemu_irq irq[PCI_NUM_PINS];
+MemoryRegion io;
+MemoryRegion mem;
+MemoryRegion reg;
+
+bitbang_i2c_interface smbus;
+uint32_t gpio; /* bits 0-7 in, 8-15 out, 16-23 direction (0 in, 1 out)
*/
+hwaddr gpio_base;
+MemoryRegion gpio_reg;
+};
These types above should be in the header file and not in the C file, as per
our current QOM guidelines.
I don't think there's such a guideline, at least I did not find any
mention of it in style and qom docs. It was necessary to move some type
declarations to headers for types that are embedded in other objects
because C needs the struct size for that, but I don't think that should be
a general thing when it's not needed.
The reason for that is that moving these to the header exposes internal
object structure to users that should not need to know that so it breaks
object encapsulation and also needs moving a bunch of includes to the
header which then makes the users of this type also include those headers
when they don't really need them but only need the type defines to
instantiate the object and that's all they should have access to. So I
think declaring types in the header should only be done for types that
aren't full devices and are meant to be embedded as part of another device
or a SoC but otherwise it's better to keep implementation closed and local
to the object and not expose it unless really needed, that's why these
are here.
If you insist I can move these but I don't think there's really such
recommendation and I don't think that's a good idea because of the above.
Regards,
BALATON Zoltan
[PATCH v2] target/riscv: Use a direct cast for better performance
v1 was here: https://lists.nongnu.org/archive/html/qemu-devel/2023-10/msg02021.html v2 is functionally exactly the same, except I changed s/qemu cast/QOM cast/ in the comment, and added the Reviewed-by tag received for the first version. Rich.
[PATCH v2] target/riscv: Use a direct cast for better performance
RISCV_CPU(cs) uses a checked cast. When QOM cast debugging is enabled
this adds about 5% total overhead when emulating RV64 on x86-64 host.
Using a RISC-V guest with 16 vCPUs, 16 GB of guest RAM, virtio-blk
disk. The guest has a copy of the qemu source tree. The test
involves compiling the qemu source tree with 'make clean; time make -j16'.
Before making this change the compile step took 449 & 447 seconds over
two consecutive runs.
After making this change, 428 & 422 seconds.
The saving is about 5%.
Thanks: Paolo Bonzini
Signed-off-by: Richard W.M. Jones
Reviewed-by: Daniel Henrique Barboza
---
target/riscv/cpu_helper.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 3a02079290..479d9863ae 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -66,7 +66,11 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *pflags)
{
CPUState *cs = env_cpu(env);
-RISCVCPU *cpu = RISCV_CPU(cs);
+/*
+ * Using the checked cast RISCV_CPU(cs) imposes ~ 5% overhead when
+ * QOM cast debugging is enabled, so use a direct cast instead.
+ */
+RISCVCPU *cpu = (RISCVCPU *)cs;
RISCVExtStatus fs, vs;
uint32_t flags = 0;
--
2.41.0
[PATCH v3 1/3] target/hexagon: move GETPC() calls to top level helpers
From: Matheus Tavares Bernardino
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
- HELPER(probe_hvx_stores), when called from another helper, ends up
using its own GETPC() expansion instead of the top level caller.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
Message-Id:
<2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathb...@quicinc.com>-ne
Reviewed-by: Brian Cain
Signed-off-by: Brian Cain
---
target/hexagon/macros.h| 19 +-
target/hexagon/op_helper.c | 75 +++---
target/hexagon/op_helper.h | 9 -
3 files changed, 38 insertions(+), 65 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index b356d85792..9a51b5709b 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(tcg_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+do { \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+} while (0)
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..8ca3976a65 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_num].va;
@@ -119,6 +118,12 @@ void HELPER(commit_store)(CPUHexagonState *env, int
slot_num)
}
}
+void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+{
+uintptr_t ra = GETPC();
+commit_store(env, slot_num, ra);
+}
+
void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
{
mem_gather_store(env, addr, slot);
@@ -467,13 +472,12 @@ int32_t HELPER(cabacdecbin_pred)(int64_t RssV, int64_t
RttV)
}
static void probe_store(CPUHexagonState *env, int slot, int mmu_idx,
-bool is_predicated)
+bool is_predicated, uintptr_t retaddr)
{
if (!is_predicated || !(env->slot_cancelled & (1 << slot))) {
size1u_t width = env->mem_log_stores[slot].width;
target_ulong va = env->mem_log_stores[slot].va;
-uintptr_t ra = GETPC();
-probe_write(env, va, width, mmu_idx, ra);
+probe_write(env, va, width, mmu_idx, retaddr);
}
}
@@ -494,12 +498,13 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState
*env, int args)
int mmu_idx = FIELD_EX32(args, PROBE_PKT_
[PATCH v3 0/3] hexagon: GETPC() fixes, shadowing fixes
Changes since v2: - rebased, suggested by Markus - s/cpu_env/tcg_env/ - For local shadows: s/tcg_env/tcg_env_/ Brian Cain (2): target/hexagon: fix some occurrences of -Wshadow=local target/hexagon: avoid shadowing globals Matheus Tavares Bernardino (1): target/hexagon: move GETPC() calls to top level helpers target/hexagon/genptr.c | 56 - target/hexagon/genptr.h | 18 +++--- target/hexagon/imported/alu.idef| 6 +- target/hexagon/macros.h | 19 +++--- target/hexagon/mmvec/macros.h | 2 +- target/hexagon/mmvec/system_ext_mmvec.c | 4 +- target/hexagon/mmvec/system_ext_mmvec.h | 2 +- target/hexagon/op_helper.c | 84 ++--- target/hexagon/op_helper.h | 9 --- target/hexagon/translate.c | 10 +-- 10 files changed, 90 insertions(+), 120 deletions(-) -- 2.25.1
[PATCH v3 2/3] target/hexagon: fix some occurrences of -Wshadow=local
Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
are less obvious. They are required because of some macro invocations like
SCATTER_OP_WRITE_TO_MEM().
e.g.:
In file included from ../target/hexagon/op_helper.c:31:
../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows
a previous local [-Werror=shadow=compatible-local]
205 | for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
| ^
../target/hexagon/op_helper.c:157:17: note: in expansion of macro
‘SCATTER_OP_WRITE_TO_MEM’
157 | SCATTER_OP_WRITE_TO_MEM(uint16_t);
| ^~~
../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
135 | int i;
| ^
In file included from ../target/hexagon/op_helper.c:31:
../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’ shadows
a previous local [-Werror=shadow=compatible-local]
204 | uintptr_t ra = GETPC(); \
| ^~
../target/hexagon/op_helper.c:160:17: note: in expansion of macro
‘SCATTER_OP_WRITE_TO_MEM’
160 | SCATTER_OP_WRITE_TO_MEM(uint32_t);
| ^~~
../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
134 | uintptr_t ra = GETPC();
| ^~
Reviewed-by: Matheus Tavares Bernardino
Signed-off-by: Brian Cain
---
target/hexagon/imported/alu.idef | 6 +++---
target/hexagon/mmvec/macros.h| 2 +-
target/hexagon/op_helper.c | 9 +++--
target/hexagon/translate.c | 10 +-
4 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/target/hexagon/imported/alu.idef b/target/hexagon/imported/alu.idef
index 12d2aac5d4..b855676989 100644
--- a/target/hexagon/imported/alu.idef
+++ b/target/hexagon/imported/alu.idef
@@ -1142,9 +1142,9 @@
Q6INSN(A4_cround_rr,"Rd32=cround(Rs32,Rt32)",ATTRIBS(),"Convergent Round", {RdV
tmp128 = fSHIFTR128(tmp128, SHIFT);\
DST = fCAST16S_8S(tmp128);\
} else {\
-size16s_t rndbit_128 = fCAST8S_16S((1LL << (SHIFT - 1))); \
-size16s_t src_128 = fCAST8S_16S(SRC); \
-size16s_t tmp128 = fADD128(src_128, rndbit_128);\
+rndbit_128 = fCAST8S_16S((1LL << (SHIFT - 1))); \
+src_128 = fCAST8S_16S(SRC); \
+tmp128 = fADD128(src_128, rndbit_128);\
tmp128 = fSHIFTR128(tmp128, SHIFT);\
DST = fCAST16S_8S(tmp128);\
}
diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index a655634fd1..1ceb9453ee 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -201,7 +201,7 @@
} while (0)
#define SCATTER_OP_WRITE_TO_MEM(TYPE) \
do { \
-uintptr_t ra = GETPC(); \
+ra = GETPC(); \
for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
if (test_bit(i, env->vtcm_log.mask)) { \
TYPE dst = 0; \
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 8ca3976a65..da10ac5847 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -132,10 +132,9 @@ void HELPER(gather_store)(CPUHexagonState *env, uint32_t
addr, int slot)
void HELPER(commit_hvx_stores)(CPUHexagonState *env)
{
uintptr_t ra = GETPC();
-int i;
/* Normal (possibly masked) vector store */
-for (i = 0; i < VSTORES_MAX; i++) {
+for (int i = 0; i < VSTORES_MAX; i++) {
if (env->vstore_pending[i]) {
env->vstore_pending[i] = 0;
target_ulong va = env->vstore[i].va;
@@ -162,7 +161,7 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
g_assert_not_reached();
}
} else {
-for (i = 0; i < sizeof(MMVector); i++) {
+for (int i = 0; i < sizeof(MMVector); i++) {
if (test_bit(i, env->vtcm_log.mask)) {
cpu_stb_data_ra(env, env->vtcm_log.va[i],
env->vtcm_log.data.ub[i], ra);
@@ -505,10 +504,8 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState
*env, int args)
static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
uintptr_t retaddr)
{
-int i;
-
/* Normal (possibly masked) vector store */
-for (i = 0; i < VSTORES_MAX; i++) {
+for (int i = 0; i < VSTORES_MAX; i++) {
if (env->vstore_pending[i]) {
target_ulong va = env->vstore[i].va;
int size = env->vstore[i].size;
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index 663b7bbc3a..666c061180 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -553,7 +553,7 @@ static void gen_start_packet(DisasContext *ctx)
/* Preload the predicated registers into get_result_gpr(ctx
[PATCH v3 3/3] target/hexagon: avoid shadowing globals
The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
identifiers to avoid shadowing the type name.
The global `tcg_env` is shadowed by local `tcg_env` arguments, so we
rename the function arguments to avoid shadowing the global.
Signed-off-by: Brian Cain
---
target/hexagon/genptr.c | 56 -
target/hexagon/genptr.h | 18
target/hexagon/mmvec/system_ext_mmvec.c | 4 +-
target/hexagon/mmvec/system_ext_mmvec.h | 2 +-
target/hexagon/op_helper.c | 4 +-
5 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index dbae6c570a..fa577e05dc 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
tcg_gen_deposit_i64(result, result, src64, N * 8, 8);
}
-static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index)
{
-tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
-tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+tcg_gen_qemu_ld_tl(dest, v_addr, mem_index, MO_TEUL);
+tcg_gen_mov_tl(hex_llsc_addr, v_addr);
tcg_gen_mov_tl(hex_llsc_val, dest);
}
-static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int mem_index)
{
-tcg_gen_qemu_ld_i64(dest, vaddr, mem_index, MO_TEUQ);
-tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+tcg_gen_qemu_ld_i64(dest, v_addr, mem_index, MO_TEUQ);
+tcg_gen_mov_tl(hex_llsc_addr, v_addr);
tcg_gen_mov_i64(hex_llsc_val_i64, dest);
}
static inline void gen_store_conditional4(DisasContext *ctx,
- TCGv pred, TCGv vaddr, TCGv src)
+ TCGv pred, TCGv v_addr, TCGv src)
{
TCGLabel *fail = gen_new_label();
TCGLabel *done = gen_new_label();
TCGv one, zero, tmp;
-tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail);
+tcg_gen_brcond_tl(TCG_COND_NE, v_addr, hex_llsc_addr, fail);
one = tcg_constant_tl(0xff);
zero = tcg_constant_tl(0);
@@ -374,13 +374,13 @@ static inline void gen_store_conditional4(DisasContext
*ctx,
}
static inline void gen_store_conditional8(DisasContext *ctx,
- TCGv pred, TCGv vaddr, TCGv_i64 src)
+ TCGv pred, TCGv v_addr, TCGv_i64 src)
{
TCGLabel *fail = gen_new_label();
TCGLabel *done = gen_new_label();
TCGv_i64 one, zero, tmp;
-tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail);
+tcg_gen_brcond_tl(TCG_COND_NE, v_addr, hex_llsc_addr, fail);
one = tcg_constant_i64(0xff);
zero = tcg_constant_i64(0);
@@ -407,57 +407,57 @@ static TCGv gen_slotval(DisasContext *ctx)
}
#endif
-void gen_store32(TCGv vaddr, TCGv src, int width, uint32_t slot)
+void gen_store32(TCGv v_addr, TCGv src, int width, uint32_t slot)
{
-tcg_gen_mov_tl(hex_store_addr[slot], vaddr);
+tcg_gen_mov_tl(hex_store_addr[slot], v_addr);
tcg_gen_movi_tl(hex_store_width[slot], width);
tcg_gen_mov_tl(hex_store_val32[slot], src);
}
-void gen_store1(TCGv_env tcg_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store1(TCGv_env tcg_env_, TCGv v_addr, TCGv src, uint32_t slot)
{
-gen_store32(vaddr, src, 1, slot);
+gen_store32(v_addr, src, 1, slot);
}
-void gen_store1i(TCGv_env tcg_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store1i(TCGv_env tcg_env_, TCGv v_addr, int32_t src, uint32_t slot)
{
TCGv tmp = tcg_constant_tl(src);
-gen_store1(tcg_env, vaddr, tmp, slot);
+gen_store1(tcg_env_, v_addr, tmp, slot);
}
-void gen_store2(TCGv_env tcg_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store2(TCGv_env tcg_env_, TCGv v_addr, TCGv src, uint32_t slot)
{
-gen_store32(vaddr, src, 2, slot);
+gen_store32(v_addr, src, 2, slot);
}
-void gen_store2i(TCGv_env tcg_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store2i(TCGv_env tcg_env_, TCGv v_addr, int32_t src, uint32_t slot)
{
TCGv tmp = tcg_constant_tl(src);
-gen_store2(tcg_env, vaddr, tmp, slot);
+gen_store2(tcg_env_, v_addr, tmp, slot);
}
-void gen_store4(TCGv_env tcg_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store4(TCGv_env tcg_env_, TCGv v_addr, TCGv src, uint32_t slot)
{
-gen_store32(vaddr, src, 4, slot);
+gen_store32(v_addr, src, 4, slot);
}
-void gen_store4i(TCGv_env tcg_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store4i(TCGv_env tcg_env_, TCGv v_addr, int32_t src, uint32_t slot)
{
TCGv tmp = tcg_constant_tl(src);
-gen_store4(tcg_env, vaddr, tmp, slot);
+gen_store4(tcg_env_, v_addr, tmp, slot);
}
-void gen_store8(TCGv_env tcg_env, TCGv vaddr, TCGv_i64 src, uint32_t slot)
+void gen_store8(TCGv_env tcg_env_, TCGv v_addr
Re: [PATCH 1/3] target/riscv: Propagate error from PMU setup
On Tue, Oct 3, 2023 at 10:53 PM Rob Bradford wrote:
>
> More closely follow the QEMU style by returning an Error and propagating
> it there is an error relating to the PMU setup.
>
> Further simplify the function by removing the num_counters parameter as
> this is available from the passed in cpu pointer.
>
> Signed-off-by: Rob Bradford
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/cpu.c | 8 +++-
> target/riscv/pmu.c | 19 +--
> target/riscv/pmu.h | 3 ++-
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 4140899c52..9d79c20c1a 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1488,7 +1488,13 @@ static void riscv_cpu_realize_tcg(DeviceState *dev,
> Error **errp)
> }
>
> if (cpu->cfg.pmu_num) {
> -if (!riscv_pmu_init(cpu, cpu->cfg.pmu_num) && cpu->cfg.ext_sscofpmf)
> {
> +riscv_pmu_init(cpu, &local_err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> +if (cpu->cfg.ext_sscofpmf) {
> cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>riscv_pmu_timer_cb, cpu);
> }
> diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> index 36f6307d28..13801ccb78 100644
> --- a/target/riscv/pmu.c
> +++ b/target/riscv/pmu.c
> @@ -434,22 +434,21 @@ int riscv_pmu_setup_timer(CPURISCVState *env, uint64_t
> value, uint32_t ctr_idx)
> }
>
>
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters)
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> {
> -if (num_counters > (RV_MAX_MHPMCOUNTERS - 3)) {
> -return -1;
> +uint8_t pmu_num = cpu->cfg.pmu_num;
> +
> +if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> +error_setg(errp, "Number of counters exceeds maximum available");
> +return;
> }
>
> cpu->pmu_event_ctr_map = g_hash_table_new(g_direct_hash, g_direct_equal);
> if (!cpu->pmu_event_ctr_map) {
> -/* PMU support can not be enabled */
> -qemu_log_mask(LOG_UNIMP, "PMU events can't be supported\n");
> -cpu->cfg.pmu_num = 0;
> -return -1;
> +error_setg(errp, "Unable to allocate PMU event hash table");
> +return;
> }
>
> /* Create a bitmask of available programmable counters */
> -cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, num_counters);
> -
> -return 0;
> +cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> }
> diff --git a/target/riscv/pmu.h b/target/riscv/pmu.h
> index 2bfb71ba87..88e0713296 100644
> --- a/target/riscv/pmu.h
> +++ b/target/riscv/pmu.h
> @@ -17,13 +17,14 @@
> */
>
> #include "cpu.h"
> +#include "qapi/error.h"
>
> bool riscv_pmu_ctr_monitor_instructions(CPURISCVState *env,
> uint32_t target_ctr);
> bool riscv_pmu_ctr_monitor_cycles(CPURISCVState *env,
>uint32_t target_ctr);
> void riscv_pmu_timer_cb(void *priv);
> -int riscv_pmu_init(RISCVCPU *cpu, int num_counters);
> +void riscv_pmu_init(RISCVCPU *cpu, Error **errp);
> int riscv_pmu_update_event_map(CPURISCVState *env, uint64_t value,
> uint32_t ctr_idx);
> int riscv_pmu_incr_ctr(RISCVCPU *cpu, enum riscv_pmu_event_idx event_idx);
> --
> 2.41.0
>
>
Re: [PATCH 2/3] target/riscv: Support discontinuous PMU counters
On Wed, Oct 4, 2023 at 7:36 PM Rob Bradford wrote:
>
> Hi Atish,
>
> On Tue, 2023-10-03 at 13:25 -0700, Atish Kumar Patra wrote:
> > On Tue, Oct 3, 2023 at 5:51 AM Rob Bradford
> > wrote:
> > >
> > > There is no requirement that the enabled counters in the platform
> > > are
> > > continuously numbered. Add a "pmu-mask" property that, if
> > > specified, can
> > > be used to specify the enabled PMUs. In order to avoid ambiguity if
> > > "pmu-mask" is specified then "pmu-num" must also match the number
> > > of
> > > bits set in the mask.
> > >
> > > Signed-off-by: Rob Bradford
> > > ---
> > > target/riscv/cpu.c | 1 +
> > > target/riscv/cpu_cfg.h | 1 +
> > > target/riscv/pmu.c | 15 +--
> > > 3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9d79c20c1a..b89b006a76 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -1817,6 +1817,7 @@ static void
> > > riscv_cpu_add_misa_properties(Object *cpu_obj)
> > > static Property riscv_cpu_extensions[] = {
> > > /* Defaults for standard extensions */
> > > DEFINE_PROP_UINT8("pmu-num", RISCVCPU, cfg.pmu_num, 16),
> > > +DEFINE_PROP_UINT32("pmu-mask", RISCVCPU, cfg.pmu_mask, 0),
> > > DEFINE_PROP_BOOL("sscofpmf", RISCVCPU, cfg.ext_sscofpmf,
> > > false),
> > > DEFINE_PROP_BOOL("Zifencei", RISCVCPU, cfg.ext_ifencei, true),
> > > DEFINE_PROP_BOOL("Zicsr", RISCVCPU, cfg.ext_icsr, true),
> > > diff --git a/target/riscv/cpu_cfg.h b/target/riscv/cpu_cfg.h
> > > index 0e6a0f245c..40f7d970bc 100644
> > > --- a/target/riscv/cpu_cfg.h
> > > +++ b/target/riscv/cpu_cfg.h
> > > @@ -124,6 +124,7 @@ struct RISCVCPUConfig {
> > > bool ext_XVentanaCondOps;
> > >
> > > uint8_t pmu_num;
> > > +uint32_t pmu_mask;
> > > char *priv_spec;
> > > char *user_spec;
> > > char *bext_spec;
> > > diff --git a/target/riscv/pmu.c b/target/riscv/pmu.c
> > > index 13801ccb78..f97e25a1f6 100644
> > > --- a/target/riscv/pmu.c
> > > +++ b/target/riscv/pmu.c
> > > @@ -437,6 +437,13 @@ int riscv_pmu_setup_timer(CPURISCVState *env,
> > > uint64_t value, uint32_t ctr_idx)
> > > void riscv_pmu_init(RISCVCPU *cpu, Error **errp)
> > > {
> > > uint8_t pmu_num = cpu->cfg.pmu_num;
> > > +uint32_t pmu_mask = cpu->cfg.pmu_mask;
> > > +
> > > +if (pmu_mask && ctpop32(pmu_mask) != pmu_num) {
> > > +error_setg(errp, "Mismatch between number of enabled
> > > counters in "
> > > + "\"pmu-mask\" and \"pmu-num\"");
> > > +return;
> > > +}
> > >
> >
> > Is that necessary for the default case? I am thinking of marking
> > pmu-num as deprecated and pmu-mask
> > as the preferred way of doing things as it is more flexible. There is
> > no real benefit carrying both.
> > The default pmu-mask value will change in that case.
> > We can just overwrite pmu-num with ctpop32(pmu_mask) if pmu-mask is
> > available. Thoughts ?
> >
>
> I agree it makes sense to me that there is only one way for the user to
> adjust the PMU count. However i'm not sure how we can handle the
> transition if we choose to deprecate "pmu-num".
>
> If we change the default "pmu-mask" to MAKE_32BIT_MASK(3, 16) then that
> value in the config will always be set - you propose that we overwrite
> "pmu-num" with the popcount of that property. But that will break if
Couldn't we deprecate "pmu-num" and then throw an error if both are
set? Then we can migrate away from "pmu-num"
Alistair
> the user has an existing setup that changes the value of "pmu-num"
> (either as a property at runtime or in the CPU init code).
>
> One option would be to not make the mask configurable as property and
> make choosing the layout of the counters something that the specialised
> CPU init can choose to do.
>
> Cheers,
>
> Rob
>
> > > if (pmu_num > (RV_MAX_MHPMCOUNTERS - 3)) {
> > > error_setg(errp, "Number of counters exceeds maximum
> > > available");
> > > @@ -449,6 +456,10 @@ void riscv_pmu_init(RISCVCPU *cpu, Error
> > > **errp)
> > > return;
> > > }
> > >
> > > -/* Create a bitmask of available programmable counters */
> > > -cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > +/* Create a bitmask of available programmable counters if none
> > > supplied */
> > > +if (pmu_mask) {
> > > +cpu->pmu_avail_ctrs = pmu_mask;
> > > +} else {
> > > +cpu->pmu_avail_ctrs = MAKE_32BIT_MASK(3, pmu_num);
> > > +}
> > > }
> > > --
> > > 2.41.0
> > >
>
>
Re: [PATCH v2 1/2] target/riscv/kvm: improve 'init_multiext_cfg' error msg
On Wed, Oct 4, 2023 at 12:46 AM Daniel Henrique Barboza
wrote:
>
> Our error message is returning the value of 'ret', which will be always
> -1 in case of error, and will not be that useful:
>
> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error -1
>
> Improve the error message by outputting 'errno' instead of 'ret'. Use
> strerrorname_np() to output the error name instead of the error code.
> This will give us what we need to know right away:
>
> qemu-system-riscv64: Unable to read ISA_EXT KVM register ssaia, error code:
> ENOENT
>
> Given that we're going to exit(1) in this condition instead of
> attempting to recover, remove the 'kvm_riscv_destroy_scratch_vcpu()'
> call.
>
> Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/kvm/kvm-cpu.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index c6615cb807..c3daf74fe9 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -792,8 +792,8 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu,
> KVMScratchCPU *kvmcpu)
> val = false;
> } else {
> error_report("Unable to read ISA_EXT KVM register %s, "
> - "error %d", multi_ext_cfg->name, ret);
> -kvm_riscv_destroy_scratch_vcpu(kvmcpu);
> + "error code: %s", multi_ext_cfg->name,
> + strerrorname_np(errno));
> exit(EXIT_FAILURE);
> }
> } else {
> --
> 2.41.0
>
>
Re: [PATCH v2 2/2] target/riscv/kvm: support KVM_GET_REG_LIST
On Wed, Oct 4, 2023 at 12:48 AM Daniel Henrique Barboza
wrote:
>
> KVM for RISC-V started supporting KVM_GET_REG_LIST in Linux 6.6. It
> consists of a KVM ioctl() that retrieves a list of all available regs
> for get_one_reg/set_one_reg. Regs that aren't present in the list aren't
> supported in the host.
>
> This simplifies our lives when initing the KVM regs since we don't have
> to always attempt a KVM_GET_ONE_REG for all regs QEMU knows. We'll only
> attempt a get_one_reg() if we're sure the reg is supported, i.e. it was
> retrieved by KVM_GET_REG_LIST. Any error in get_one_reg() will then
> always considered fatal, instead of having to handle special error codes
> that might indicate a non-fatal failure.
>
> Start by moving the current kvm_riscv_init_multiext_cfg() logic into a
> new kvm_riscv_read_multiext_legacy() helper. We'll prioritize using
> KVM_GET_REG_LIST, so check if we have it available and, in case we
> don't, use the legacy() logic.
>
> Otherwise, retrieve the available reg list and use it to check if the
> host supports our known KVM regs, doing the usual get_one_reg() for
> the supported regs and setting cpu->cfg accordingly.
>
> Signed-off-by: Daniel Henrique Barboza
Acked-by: Alistair Francis
Alistair
> ---
> target/riscv/kvm/kvm-cpu.c | 96 +-
> 1 file changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index c3daf74fe9..090d617627 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -771,7 +771,8 @@ static void kvm_riscv_read_cbomz_blksize(RISCVCPU *cpu,
> KVMScratchCPU *kvmcpu,
> }
> }
>
> -static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
> +static void kvm_riscv_read_multiext_legacy(RISCVCPU *cpu,
> + KVMScratchCPU *kvmcpu)
> {
> CPURISCVState *env = &cpu->env;
> uint64_t val;
> @@ -812,6 +813,99 @@ static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu,
> KVMScratchCPU *kvmcpu)
> }
> }
>
> +static int uint64_cmp(const void *a, const void *b)
> +{
> +uint64_t val1 = *(const uint64_t *)a;
> +uint64_t val2 = *(const uint64_t *)b;
> +
> +if (val1 < val2) {
> +return -1;
> +}
> +
> +if (val1 > val2) {
> +return 1;
> +}
> +
> +return 0;
> +}
> +
> +static void kvm_riscv_init_multiext_cfg(RISCVCPU *cpu, KVMScratchCPU *kvmcpu)
> +{
> +KVMCPUConfig *multi_ext_cfg;
> +struct kvm_one_reg reg;
> +struct kvm_reg_list rl_struct;
> +struct kvm_reg_list *reglist;
> +uint64_t val, reg_id, *reg_search;
> +int i, ret;
> +
> +rl_struct.n = 0;
> +ret = ioctl(kvmcpu->cpufd, KVM_GET_REG_LIST, &rl_struct);
> +
> +/*
> + * If KVM_GET_REG_LIST isn't supported we'll get errno 22
> + * (EINVAL). Use read_legacy() in this case.
> + */
> +if (errno == EINVAL) {
> +return kvm_riscv_read_multiext_legacy(cpu, kvmcpu);
> +} else if (errno != E2BIG) {
> +/*
> + * E2BIG is an expected error message for the API since we
> + * don't know the number of registers. The right amount will
> + * be written in rl_struct.n.
> + *
> + * Error out if we get any other errno.
> + */
> +error_report("Error when accessing get-reg-list, code: %s",
> + strerrorname_np(errno));
> +exit(EXIT_FAILURE);
> +}
> +
> +reglist = g_malloc(sizeof(struct kvm_reg_list) +
> + rl_struct.n * sizeof(uint64_t));
> +reglist->n = rl_struct.n;
> +ret = ioctl(kvmcpu->cpufd, KVM_GET_REG_LIST, reglist);
> +if (ret) {
> +error_report("Error when reading KVM_GET_REG_LIST, code %s ",
> + strerrorname_np(errno));
> +exit(EXIT_FAILURE);
> +}
> +
> +/* sort reglist to use bsearch() */
> +qsort(®list->reg, reglist->n, sizeof(uint64_t), uint64_cmp);
> +
> +for (i = 0; i < ARRAY_SIZE(kvm_multi_ext_cfgs); i++) {
> +multi_ext_cfg = &kvm_multi_ext_cfgs[i];
> +reg_id = kvm_riscv_reg_id(&cpu->env, KVM_REG_RISCV_ISA_EXT,
> + multi_ext_cfg->kvm_reg_id);
> +reg_search = bsearch(®_id, reglist->reg, reglist->n,
> + sizeof(uint64_t), uint64_cmp);
> +if (!reg_search) {
> +continue;
> +}
> +
> +reg.id = reg_id;
> +reg.addr = (uint64_t)&val;
> +ret = ioctl(kvmcpu->cpufd, KVM_GET_ONE_REG, ®);
> +if (ret != 0) {
> +error_report("Unable to read ISA_EXT KVM register %s, "
> + "error code: %s", multi_ext_cfg->name,
> + strerrorname_np(errno));
> +exit(EXIT_FAILURE);
> +}
> +
> +multi_ext_cfg->supported = true;
> +kvm_cpu_cfg_set(cpu, multi_ext_cfg, val);
> +}
> +
> +if (cpu->cfg.ext_icbom) {
> +kvm_riscv_read_cbomz
Re: [PATCH 0/2] riscv, kvm: support KVM_GET_REG_LIST
On Tue, Oct 3, 2023 at 9:34 PM Daniel Henrique Barboza wrote: > > Hi, > > Starting on Linux 6.6 the QEMU RISC-V KVM driver now supports > KMV_GET_REG_LIST. This API will make it simpler for the QEMU KVM driver > to determine whether a KVM reg is present or not. > > We'll use this API to fetch ISA_EXT regs during init(). The current > logic will be put in a legacy() helper and will still be used in case > the host KVM module does not support get-reg-list. > > Patch 1 contains error handling changes in kvm_riscv_init_multiext_cfg() > where we're using &error_fatal and errno. > > > Daniel Henrique Barboza (2): > target/riscv/kvm: improve 'init_multiext_cfg' error msg > target/riscv/kvm: support KVM_GET_REG_LIST Thanks! Applied to riscv-to-apply.next Alistair > > target/riscv/kvm/kvm-cpu.c | 100 +++-- > 1 file changed, 95 insertions(+), 5 deletions(-) > > -- > 2.41.0 > >
Re: [PATCH] target/riscv/tcg: remove RVG warning
On Tue, Oct 3, 2023 at 10:26 PM Daniel Henrique Barboza
wrote:
>
> Vendor CPUs that set RVG are displaying user warnings about other
> extensions that RVG must enable, one warning per CPU. E.g.:
>
> $ ./build/qemu-system-riscv64 -smp 8 -M virt -cpu veyron-v1 -nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>
> This happens because we decided a while ago that, for simplicity, vendor
> CPUs could set RVG instead of setting each G extension individually in
> their cpu_init(). Our warning isn't taking that into account, and we're
> bugging users with a warning that we're causing ourselves.
>
> In a closer look we conclude that this warning is not warranted in any
> other circumstance since we're just following the ISA [1], which states
> in chapter 24:
>
> "One goal of the RISC-V project is that it be used as a stable software
> development target. For this purpose, we define a combination of a base
> ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr,
> Zifencei) as a 'general-purpose' ISA, and we use the abbreviation G for
> the IMAFDZicsr Zifencei combination of instruction-set extensions."
>
> With this in mind, enabling IMAFD_Zicsr_Zifencei if the user explicitly
> enables 'G' is an expected behavior and the warning is unneeded. Any
> user caught by surprise should refer to the ISA.
>
> Remove the warning when handling RVG.
>
> [1]
> https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf
>
> Reported-by: Paul A. Clarke
> Suggested-by: Andrew Jones
> Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/tcg/tcg-cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 08b806dc07..f50ce57602 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -293,7 +293,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu,
> Error **errp)
> return;
> }
>
> -warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
> --
> 2.41.0
>
>
Re: [PATCH] target/riscv/tcg: remove RVG warning
On Tue, Oct 3, 2023 at 10:26 PM Daniel Henrique Barboza
wrote:
>
> Vendor CPUs that set RVG are displaying user warnings about other
> extensions that RVG must enable, one warning per CPU. E.g.:
>
> $ ./build/qemu-system-riscv64 -smp 8 -M virt -cpu veyron-v1 -nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
>
> This happens because we decided a while ago that, for simplicity, vendor
> CPUs could set RVG instead of setting each G extension individually in
> their cpu_init(). Our warning isn't taking that into account, and we're
> bugging users with a warning that we're causing ourselves.
>
> In a closer look we conclude that this warning is not warranted in any
> other circumstance since we're just following the ISA [1], which states
> in chapter 24:
>
> "One goal of the RISC-V project is that it be used as a stable software
> development target. For this purpose, we define a combination of a base
> ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr,
> Zifencei) as a 'general-purpose' ISA, and we use the abbreviation G for
> the IMAFDZicsr Zifencei combination of instruction-set extensions."
>
> With this in mind, enabling IMAFD_Zicsr_Zifencei if the user explicitly
> enables 'G' is an expected behavior and the warning is unneeded. Any
> user caught by surprise should refer to the ISA.
>
> Remove the warning when handling RVG.
>
> [1]
> https://github.com/riscv/riscv-isa-manual/releases/download/Ratified-IMAFDQC/riscv-spec-20191213.pdf
>
> Reported-by: Paul A. Clarke
> Suggested-by: Andrew Jones
> Signed-off-by: Daniel Henrique Barboza
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> target/riscv/tcg/tcg-cpu.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 08b806dc07..f50ce57602 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -293,7 +293,6 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu,
> Error **errp)
> return;
> }
>
> -warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
> --
> 2.41.0
>
>
Re: [PATCH v3 4/5] hw/char: riscv_htif: replace exit calls with proper shutdown
On Mon, Oct 2, 2023 at 7:32 PM Clément Chigot wrote:
>
> On Fri, Sep 22, 2023 at 7:20 AM Alistair Francis wrote:
> >
> > On Thu, Sep 7, 2023 at 9:26 PM Clément Chigot wrote:
> > >
> > > This replaces the exit calls by shutdown requests, ensuring a proper
> > > cleanup of Qemu. Otherwise, some connections like gdb could be broken
> > > before its final packet ("Wxx") is being sent. This part, being done
> > > inside qemu_cleanup function, can be reached only when the main loop
> > > exits after a shutdown request.
> > >
> > > Signed-off-by: Clément Chigot
> >
> > Do you mind rebasing this on:
> > https://github.com/alistair23/qemu/tree/riscv-to-apply.next
>
> Thanks for the review.
> Just a quick question on the procedure side, is there any special tag
> or something to say in the cover letter to state that it has been
> rebased on riscv-to-apply instead of the usual master?
You can use the "Based-on" tag. There is some more details here:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#id33
Alistair
>
> Clément
>
> > Alistair
> >
> > > ---
> > > hw/char/riscv_htif.c | 5 -
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> > > index 37d3ccc76b..7e9b6fcc98 100644
> > > --- a/hw/char/riscv_htif.c
> > > +++ b/hw/char/riscv_htif.c
> > > @@ -31,6 +31,7 @@
> > > #include "qemu/error-report.h"
> > > #include "exec/address-spaces.h"
> > > #include "sysemu/dma.h"
> > > +#include "sysemu/runstate.h"
> > >
> > > #define RISCV_DEBUG_HTIF 0
> > > #define HTIF_DEBUG(fmt, ...)
> > > \
> > > @@ -205,7 +206,9 @@ static void htif_handle_tohost_write(HTIFState *s,
> > > uint64_t val_written)
> > > g_free(sig_data);
> > > }
> > >
> > > -exit(exit_code);
> > > +qemu_system_shutdown_request_with_code(
> > > +SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
> > > +return;
> > > } else {
> > > uint64_t syscall[8];
> > > cpu_physical_memory_read(payload, syscall,
> > > sizeof(syscall));
> > > --
> > > 2.25.1
> > >
Re: [PATCH v2] target/riscv: pmp: Ignore writes when RW=01
On Mon, Sep 25, 2023 at 9:11 PM Mayuresh Chitale
wrote:
>
> As per the Priv spec: "The R, W, and X fields form a collective WARL
> field for which the combinations with R=0 and W=1 are reserved."
> However currently such writes are not ignored as ought to be. The
> combinations with RW=01 are allowed only when the Smepmp extension
> is enabled and mseccfg.MML is set.
>
> Signed-off-by: Mayuresh Chitale
Reviewed-by: Alistair Francis
Alistair
> ---
> target/riscv/pmp.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 5b14eb511a..8e25f145e0 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -120,6 +120,11 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t
> pmp_index, uint8_t val)
> if (locked) {
> qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write -
> locked\n");
> } else {
> +/* If !mseccfg.MML then ignore writes with encoding RW=01 */
> +if ((val & PMP_WRITE) && !(val & PMP_READ) &&
> +!MSECCFG_MML_ISSET(env)) {
> +val &= ~(PMP_WRITE | PMP_READ);
> +}
> env->pmp_state.pmp[pmp_index].cfg_reg = val;
> pmp_update_rule(env, pmp_index);
> }
> --
> 2.34.1
>
>
RE: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>-Original Message-
>From: Eric Auger
>Sent: Monday, October 9, 2023 1:46 AM
>Subject: Re: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>
>Hi Zhenzhong,
>On 10/8/23 12:21, Duan, Zhenzhong wrote:
>> Hi Eric,
>>
>>> -Original Message-
>>> From: Eric Auger
>>> Sent: Wednesday, October 4, 2023 11:44 PM
>>> Subject: [PATCH v4 10/15] vfio/ccw: Use vfio_[attach/detach]_device
>>>
>>> Let the vfio-ccw device use vfio_attach_device() and
>>> vfio_detach_device(), hence hiding the details of the used
>>> IOMMU backend.
>>>
>>> Note that the migration reduces the following trace
>>> "vfio: subchannel %s has already been attached" (featuring
>>> cssid.ssid.devid) into "device is already attached"
>>>
>>> Also now all the devices have been migrated to use the new
>>> vfio_attach_device/vfio_detach_device API, let's turn the
>>> legacy functions into static functions, local to container.c.
>>>
>>> Signed-off-by: Eric Auger
>>> Signed-off-by: Yi Liu
>>> Signed-off-by: Zhenzhong Duan
>>> Reviewed-by: Matthew Rosato
>>>
>>> ---
>>>
>>> v3:
>>> - simplified vbasedev->dev setting
>>>
>>> v2 -> v3:
>>> - Hopefully fix confusion beteen vbasedev->name, mdevid and sysfsdev
>>> while keeping into account Matthew's comment
>>> https://lore.kernel.org/qemu-devel/6e04ab8f-dc84-e9c2-deea-
>>> 2b6b31678...@linux.ibm.com/
>>> ---
>>> include/hw/vfio/vfio-common.h | 5 --
>>> hw/vfio/ccw.c | 122 +-
>>> hw/vfio/common.c | 10 +--
>>> 3 files changed, 37 insertions(+), 100 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index 12fbfbc37d..c486bdef2a 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -202,7 +202,6 @@ typedef struct {
>>> hwaddr pages;
>>> } VFIOBitmap;
>>>
>>> -void vfio_put_base_device(VFIODevice *vbasedev);
>>> void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>>> void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>>> void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
>>> @@ -220,11 +219,7 @@ void vfio_region_unmap(VFIORegion *region);
>>> void vfio_region_exit(VFIORegion *region);
>>> void vfio_region_finalize(VFIORegion *region);
>>> void vfio_reset_handler(void *opaque);
>>> -VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp);
>>> -void vfio_put_group(VFIOGroup *group);
>>> struct vfio_device_info *vfio_get_device_info(int fd);
>>> -int vfio_get_device(VFIOGroup *group, const char *name,
>>> -VFIODevice *vbasedev, Error **errp);
>>> int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>AddressSpace *as, Error **errp);
>>> void vfio_detach_device(VFIODevice *vbasedev);
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index 1e2fce83b0..6ec35fedc9 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -572,88 +572,15 @@ static void vfio_ccw_put_region(VFIOCCWDevice
>>> *vcdev)
>>> g_free(vcdev->io_region);
>>> }
>>>
>>> -static void vfio_ccw_put_device(VFIOCCWDevice *vcdev)
>>> -{
>>> -g_free(vcdev->vdev.name);
>>> -vfio_put_base_device(&vcdev->vdev);
>>> -}
>>> -
>>> -static void vfio_ccw_get_device(VFIOGroup *group, VFIOCCWDevice *vcdev,
>>> -Error **errp)
>>> -{
>>> -S390CCWDevice *cdev = S390_CCW_DEVICE(vcdev);
>>> -char *name = g_strdup_printf("%x.%x.%04x", cdev->hostid.cssid,
>>> - cdev->hostid.ssid,
>>> - cdev->hostid.devid);
>>> -VFIODevice *vbasedev;
>>> -
>>> -QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>> -if (strcmp(vbasedev->name, name) == 0) {
>>> -error_setg(errp, "vfio: subchannel %s has already been
>>> attached",
>>> - name);
>>> -goto out_err;
>>> -}
>>> -}
>>> -
>>> -/*
>>> - * All vfio-ccw devices are believed to operate in a way compatible
>>> with
>>> - * discarding of memory in RAM blocks, ie. pages pinned in the host are
>>> - * in the current working set of the guest driver and therefore never
>>> - * overlap e.g., with pages available to the guest balloon driver.
>>> This
>>> - * needs to be set before vfio_get_device() for vfio common to handle
>>> - * ram_block_discard_disable().
>>> - */
>>> -vcdev->vdev.ram_block_discard_allowed = true;
>>> -
>>> -if (vfio_get_device(group, cdev->mdevid, &vcdev->vdev, errp)) {
>>> -goto out_err;
>>> -}
>>> -
>>> -vcdev->vdev.ops = &vfio_ccw_ops;
>>> -vcdev->vdev.type = VFIO_DEVICE_TYPE_CCW;
>>> -vcdev->vdev.name = name;
>>> -vcdev->vdev.dev = DEVICE(vcdev);
>>> -
>>> -return;
>>> -
>>> -out_err:
>>> -g_free(name);
>>> -}
>>> -
>>> -static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp)
>>> -{
>>> -char *tmp, group_path[PATH_MAX];
>>> -ssize_t l
Re: [PATCH v2] target/riscv: pmp: Ignore writes when RW=01
On Mon, Sep 25, 2023 at 9:11 PM Mayuresh Chitale
wrote:
>
> As per the Priv spec: "The R, W, and X fields form a collective WARL
> field for which the combinations with R=0 and W=1 are reserved."
> However currently such writes are not ignored as ought to be. The
> combinations with RW=01 are allowed only when the Smepmp extension
> is enabled and mseccfg.MML is set.
>
> Signed-off-by: Mayuresh Chitale
Do you mind rebasing this on
https://github.com/alistair23/qemu/tree/riscv-to-apply.next
Alistair
> ---
> target/riscv/pmp.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c
> index 5b14eb511a..8e25f145e0 100644
> --- a/target/riscv/pmp.c
> +++ b/target/riscv/pmp.c
> @@ -120,6 +120,11 @@ static void pmp_write_cfg(CPURISCVState *env, uint32_t
> pmp_index, uint8_t val)
> if (locked) {
> qemu_log_mask(LOG_GUEST_ERROR, "ignoring pmpcfg write -
> locked\n");
> } else {
> +/* If !mseccfg.MML then ignore writes with encoding RW=01 */
> +if ((val & PMP_WRITE) && !(val & PMP_READ) &&
> +!MSECCFG_MML_ISSET(env)) {
> +val &= ~(PMP_WRITE | PMP_READ);
> +}
> env->pmp_state.pmp[pmp_index].cfg_reg = val;
> pmp_update_rule(env, pmp_index);
> }
> --
> 2.34.1
>
>
Re: [PATCH] target/riscv: deprecate capital 'Z' CPU properties
On Sun, Oct 8, 2023 at 3:15 AM Daniel Henrique Barboza
wrote:
>
> At this moment there are eleven CPU extension properties that starts
> with capital 'Z': Zifencei, Zicsr, Zihintntl, Zihintpause, Zawrs, Zfa,
> Zfh, Zfhmin, Zve32f, Zve64f and Zve64d. All other extensions are named
> with lower-case letters.
>
> We want all properties to be named with lower-case letters since it's
> consistent with the riscv-isa string that we create in the FDT. Having
> these 11 properties to be exceptions can be confusing.
>
> Deprecate all of them. Create their lower-case counterpart to be used as
> maintained CPU properties. When trying to use any deprecated property a
> warning message will be displayed, recommending users to switch to the
> lower-case variant:
>
> ./build/qemu-system-riscv64 -M virt -cpu rv64,Zifencei=true --nographic
> qemu-system-riscv64: warning: CPU property 'Zifencei' is deprecated. Please
> use 'zifencei' instead
>
> This will give users some time to change their scripts before we remove
> the capital 'Z' properties entirely.
>
> Signed-off-by: Daniel Henrique Barboza
Reviewed-by: Alistair Francis
Alistair
> ---
> docs/about/deprecated.rst | 23 ++
> target/riscv/cpu.c | 39 +++---
> target/riscv/cpu.h | 1 +
> target/riscv/tcg/tcg-cpu.c | 31 +-
> 4 files changed, 82 insertions(+), 12 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 694b878f36..331f10f930 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -378,6 +378,29 @@ of generic CPUs: rv32 and rv64 as default CPUs and 'max'
> as a feature complete
> CPU for both 32 and 64 bit builds. Users are then discouraged to use the
> 'any'
> CPU type starting in 8.2.
>
> +RISC-V CPU properties which start with with capital 'Z' (since 8.2)
> +^^
> +
> +All RISC-V CPU properties which start with capital 'Z' are being deprecated
> +starting in 8.2. The reason is that they were wrongly added with capital 'Z'
> +in the past. CPU properties were later added with lower-case names, which
> +is the format we want to use from now on.
> +
> +Users which try to use these deprecated properties will receive a warning
> +recommending to switch to their stable counterparts:
> +
> +- "Zifencei" should be replaced with "zifencei"
> +- "Zicsr" should be replaced with "zicsr"
> +- "Zihintntl" should be replaced with "zihintntl"
> +- "Zihintpause" should be replaced with "zihintpause"
> +- "Zawrs" should be replaced with "zawrs"
> +- "Zfa" should be replaced with "zfa"
> +- "Zfh" should be replaced with "zfh"
> +- "Zfhmin" should be replaced with "zfhmin"
> +- "Zve32f" should be replaced with "zve32f"
> +- "Zve64f" should be replaced with "zve64f"
> +- "Zve64d" should be replaced with "zve64d"
> +
> Block device options
>
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 521bb88538..1cdc3d2609 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1246,17 +1246,17 @@ const char *riscv_get_misa_ext_description(uint32_t
> bit)
> const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
> /* Defaults for standard extensions */
> MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
> -MULTI_EXT_CFG_BOOL("Zifencei", ext_ifencei, true),
> -MULTI_EXT_CFG_BOOL("Zicsr", ext_icsr, true),
> -MULTI_EXT_CFG_BOOL("Zihintntl", ext_zihintntl, true),
> -MULTI_EXT_CFG_BOOL("Zihintpause", ext_zihintpause, true),
> -MULTI_EXT_CFG_BOOL("Zawrs", ext_zawrs, true),
> -MULTI_EXT_CFG_BOOL("Zfa", ext_zfa, true),
> -MULTI_EXT_CFG_BOOL("Zfh", ext_zfh, false),
> -MULTI_EXT_CFG_BOOL("Zfhmin", ext_zfhmin, false),
> -MULTI_EXT_CFG_BOOL("Zve32f", ext_zve32f, false),
> -MULTI_EXT_CFG_BOOL("Zve64f", ext_zve64f, false),
> -MULTI_EXT_CFG_BOOL("Zve64d", ext_zve64d, false),
> +MULTI_EXT_CFG_BOOL("zifencei", ext_ifencei, true),
> +MULTI_EXT_CFG_BOOL("zicsr", ext_icsr, true),
> +MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
> +MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
> +MULTI_EXT_CFG_BOOL("zawrs", ext_zawrs, true),
> +MULTI_EXT_CFG_BOOL("zfa", ext_zfa, true),
> +MULTI_EXT_CFG_BOOL("zfh", ext_zfh, false),
> +MULTI_EXT_CFG_BOOL("zfhmin", ext_zfhmin, false),
> +MULTI_EXT_CFG_BOOL("zve32f", ext_zve32f, false),
> +MULTI_EXT_CFG_BOOL("zve64f", ext_zve64f, false),
> +MULTI_EXT_CFG_BOOL("zve64d", ext_zve64d, false),
> MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>
> MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> @@ -1349,6 +1349,23 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_experimental_exts[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +/* Deprecated entries marked for future removal */
> +const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
> +MULTI_EXT_CFG_BOOL("Zifencei", e
Re: [PATCH] target/riscv: deprecate capital 'Z' CPU properties
On Sun, Oct 8, 2023 at 3:15 AM Daniel Henrique Barboza
wrote:
>
> At this moment there are eleven CPU extension properties that starts
> with capital 'Z': Zifencei, Zicsr, Zihintntl, Zihintpause, Zawrs, Zfa,
> Zfh, Zfhmin, Zve32f, Zve64f and Zve64d. All other extensions are named
> with lower-case letters.
>
> We want all properties to be named with lower-case letters since it's
> consistent with the riscv-isa string that we create in the FDT. Having
> these 11 properties to be exceptions can be confusing.
>
> Deprecate all of them. Create their lower-case counterpart to be used as
> maintained CPU properties. When trying to use any deprecated property a
> warning message will be displayed, recommending users to switch to the
> lower-case variant:
>
> ./build/qemu-system-riscv64 -M virt -cpu rv64,Zifencei=true --nographic
> qemu-system-riscv64: warning: CPU property 'Zifencei' is deprecated. Please
> use 'zifencei' instead
>
> This will give users some time to change their scripts before we remove
> the capital 'Z' properties entirely.
>
> Signed-off-by: Daniel Henrique Barboza
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> docs/about/deprecated.rst | 23 ++
> target/riscv/cpu.c | 39 +++---
> target/riscv/cpu.h | 1 +
> target/riscv/tcg/tcg-cpu.c | 31 +-
> 4 files changed, 82 insertions(+), 12 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 694b878f36..331f10f930 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -378,6 +378,29 @@ of generic CPUs: rv32 and rv64 as default CPUs and 'max'
> as a feature complete
> CPU for both 32 and 64 bit builds. Users are then discouraged to use the
> 'any'
> CPU type starting in 8.2.
>
> +RISC-V CPU properties which start with with capital 'Z' (since 8.2)
> +^^
> +
> +All RISC-V CPU properties which start with capital 'Z' are being deprecated
> +starting in 8.2. The reason is that they were wrongly added with capital 'Z'
> +in the past. CPU properties were later added with lower-case names, which
> +is the format we want to use from now on.
> +
> +Users which try to use these deprecated properties will receive a warning
> +recommending to switch to their stable counterparts:
> +
> +- "Zifencei" should be replaced with "zifencei"
> +- "Zicsr" should be replaced with "zicsr"
> +- "Zihintntl" should be replaced with "zihintntl"
> +- "Zihintpause" should be replaced with "zihintpause"
> +- "Zawrs" should be replaced with "zawrs"
> +- "Zfa" should be replaced with "zfa"
> +- "Zfh" should be replaced with "zfh"
> +- "Zfhmin" should be replaced with "zfhmin"
> +- "Zve32f" should be replaced with "zve32f"
> +- "Zve64f" should be replaced with "zve64f"
> +- "Zve64d" should be replaced with "zve64d"
> +
> Block device options
>
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 521bb88538..1cdc3d2609 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1246,17 +1246,17 @@ const char *riscv_get_misa_ext_description(uint32_t
> bit)
> const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
> /* Defaults for standard extensions */
> MULTI_EXT_CFG_BOOL("sscofpmf", ext_sscofpmf, false),
> -MULTI_EXT_CFG_BOOL("Zifencei", ext_ifencei, true),
> -MULTI_EXT_CFG_BOOL("Zicsr", ext_icsr, true),
> -MULTI_EXT_CFG_BOOL("Zihintntl", ext_zihintntl, true),
> -MULTI_EXT_CFG_BOOL("Zihintpause", ext_zihintpause, true),
> -MULTI_EXT_CFG_BOOL("Zawrs", ext_zawrs, true),
> -MULTI_EXT_CFG_BOOL("Zfa", ext_zfa, true),
> -MULTI_EXT_CFG_BOOL("Zfh", ext_zfh, false),
> -MULTI_EXT_CFG_BOOL("Zfhmin", ext_zfhmin, false),
> -MULTI_EXT_CFG_BOOL("Zve32f", ext_zve32f, false),
> -MULTI_EXT_CFG_BOOL("Zve64f", ext_zve64f, false),
> -MULTI_EXT_CFG_BOOL("Zve64d", ext_zve64d, false),
> +MULTI_EXT_CFG_BOOL("zifencei", ext_ifencei, true),
> +MULTI_EXT_CFG_BOOL("zicsr", ext_icsr, true),
> +MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
> +MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
> +MULTI_EXT_CFG_BOOL("zawrs", ext_zawrs, true),
> +MULTI_EXT_CFG_BOOL("zfa", ext_zfa, true),
> +MULTI_EXT_CFG_BOOL("zfh", ext_zfh, false),
> +MULTI_EXT_CFG_BOOL("zfhmin", ext_zfhmin, false),
> +MULTI_EXT_CFG_BOOL("zve32f", ext_zve32f, false),
> +MULTI_EXT_CFG_BOOL("zve64f", ext_zve64f, false),
> +MULTI_EXT_CFG_BOOL("zve64d", ext_zve64d, false),
> MULTI_EXT_CFG_BOOL("sstc", ext_sstc, true),
>
> MULTI_EXT_CFG_BOOL("smstateen", ext_smstateen, false),
> @@ -1349,6 +1349,23 @@ const RISCVCPUMultiExtConfig
> riscv_cpu_experimental_exts[] = {
> DEFINE_PROP_END_OF_LIST(),
> };
>
> +/* Deprecated entries marked for future removal */
> +const RISCVCPUMultiExtConfig riscv_cpu_deprecated_exts[] = {
> +MULTI_EXT_CFG_BOOL("Zif
[PATCH 3/3] vfio/ccw: Remove redundant definition of TYPE_VFIO_CCW
No functional changes. Signed-off-by: Zhenzhong Duan --- include/hw/s390x/vfio-ccw.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h index 63a909eb7e..4209d27657 100644 --- a/include/hw/s390x/vfio-ccw.h +++ b/include/hw/s390x/vfio-ccw.h @@ -22,6 +22,4 @@ #define TYPE_VFIO_CCW "vfio-ccw" OBJECT_DECLARE_SIMPLE_TYPE(VFIOCCWDevice, VFIO_CCW) -#define TYPE_VFIO_CCW "vfio-ccw" - #endif -- 2.34.1
[PATCH 0/3] vfio: memory leak fix and code cleanup
Hi, This trivial patchset fixes a incremental memory leak in rare case, and some cleanup on ap/ccw. This patchset is based on vfio-next. Thanks Zhenzhong Zhenzhong Duan (3): vfio/pci: Fix a potential memory leak in vfio_listener_region_add vfio/ap: Remove pointless apdev variable vfio/ccw: Remove redundant definition of TYPE_VFIO_CCW hw/vfio/ap.c| 9 +++-- hw/vfio/common.c| 2 +- include/hw/s390x/vfio-ccw.h | 2 -- 3 files changed, 4 insertions(+), 9 deletions(-) -- 2.34.1
[PATCH 2/3] vfio/ap: Remove pointless apdev variable
No need to double-cast, call VFIO_AP_DEVICE() on DeviceState.
No functional changes.
Signed-off-by: Zhenzhong Duan
---
hw/vfio/ap.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 22e564f4f7..e083a19eac 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -156,8 +156,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
{
int ret;
Error *err = NULL;
-APDevice *apdev = AP_DEVICE(dev);
-VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
VFIODevice *vbasedev = &vapdev->vdev;
vbasedev->name = g_path_get_basename(vbasedev->sysfsdev);
@@ -195,8 +194,7 @@ error:
static void vfio_ap_unrealize(DeviceState *dev)
{
-APDevice *apdev = AP_DEVICE(dev);
-VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
vfio_ap_unregister_irq_notifier(vapdev, VFIO_AP_REQ_IRQ_INDEX);
vfio_detach_device(&vapdev->vdev);
@@ -211,8 +209,7 @@ static Property vfio_ap_properties[] = {
static void vfio_ap_reset(DeviceState *dev)
{
int ret;
-APDevice *apdev = AP_DEVICE(dev);
-VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
+VFIOAPDevice *vapdev = VFIO_AP_DEVICE(dev);
ret = ioctl(vapdev->vdev.fd, VFIO_DEVICE_RESET);
if (ret) {
--
2.34.1
[PATCH 1/3] vfio/pci: Fix a potential memory leak in vfio_listener_region_add
When there is an failure in vfio_listener_region_add() and the section
belongs to a ram device, there is an inaccurate error report which should
never be related to vfio_dma_map failure. The memory holding err is also
incrementally leaked in each failure.
Fix it by reporting the real error and free it.
Fixes: 567b5b309ab ("vfio/pci: Relax DMA map errors for MMIO regions")
Signed-off-by: Zhenzhong Duan
---
hw/vfio/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9e61de03ee..5ff5acf1d8 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -763,7 +763,7 @@ static void vfio_listener_region_add(MemoryListener
*listener,
fail:
if (memory_region_is_ram_device(section->mr)) {
-error_report("failed to vfio_dma_map. pci p2p may not work");
+error_reportf_err(err, "PCI p2p may not work: ");
return;
}
/*
--
2.34.1
Re: [PATCH 0/6] riscv: RVA22U64 profile support
On Fri, Sep 29, 2023 at 10:54 PM Daniel P. Berrangé wrote:
>
> On Fri, Sep 29, 2023 at 09:49:47AM -0300, Daniel Henrique Barboza wrote:
> >
> >
> > On 9/29/23 08:55, Daniel P. Berrangé wrote:
> > > On Fri, Sep 29, 2023 at 08:29:08AM -0300, Daniel Henrique Barboza wrote:
> > > >
> > > >
> > > > On 9/29/23 07:46, Daniel P. Berrangé wrote:
> > > > > On Tue, Sep 26, 2023 at 04:49:44PM -0300, Daniel Henrique Barboza
> > > > > wrote:
> > > > > > Based-on: 20230926183109.165878-1-dbarb...@ventanamicro.com
> > > > > > ("[PATCH 0/2] riscv: add extension properties for all cpus")
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > These patches implements the base profile support for qemu-riscv
> > > > > > and the
> > > > > > first profile, RVA22U64.
> > > > > >
> > > > > > As discussed in this thread [1] we're aiming for a flag that
> > > > > > enables all
> > > > > > mandatory extensions of a profile. Optional extensions were left
> > > > > > behind
> > > > > > and must be enabled by hand if desired. Since this is the first
> > > > > > profile
> > > > > > we're adding, we'll need to add the base framework as well.
> > > > > >
> > > > > > The RVA22U64 profile was chosen because qemu-riscv implements all
> > > > > > its
> > > > > > extensions, both mandatory and optional. That includes 'zicntr' and
> > > > > > 'zihpm', which we support for awhile but aren't adverting to
> > > > > > userspace.
> > > > > >
> > > > > > Other design decisions made:
> > > > > >
> > > > > > - disabling a profile flag does nothing, i.e. we won't mass disable
> > > > > > mandatory extensions of the rva22U64 profile if the user sets
> > > > > > rva22u64=false;
> > > > >
> > > > > Why shouldn't this be allowed ?
> > > > >
> > > > > IIUC, a profile is syntactic sugar for a group of features. If
> > > > > we can disable individual features explicitly, why should we
> > > > > not allow use of the profile as sugar to disable them en-mass ?
> > > >
> > > > In theory there's no harm in allowing mass disabling of extensions but,
> > > > given
> > > > it's a whole profile, we would end up disabling most/all CPU extensions
> > > > and
> > > > the guest would do nothing.
> > >
> > > True, that is just user error though. They could disable a profile
> > > and then manually re-enable individual features, and thus get a
> > > working system.
> > >
> > > > There is a thread in the ML:
> > > >
> > > > https://lore.kernel.org/qemu-riscv/cabjz62nyvnu4z1qmcg7myjkgg_9ywxjufhhwjmoqep6unrr...@mail.gmail.com/
> > > >
> > > > Where we discussed the possibility of having a minimal CPU extension
> > > > set. We didn't
> > > > reach a consensus because the definition of "minimal CPU extension set"
> > > > vary between
> > > > OSes (Linux requires IMAFD, FreeBSD might require something differ).
> > > >
> > > > Assuming we reach a consensus on what a minimal set is, we could allow
> > > > disabling mass
> > > > extensions via probile but keeping this minimal set, for example. At
> > > > very least we
> > > > shouldn't allow users to disable 'I' because that would kill the CPU,
> > > > so RV64I is
> > > > the minimum set that I would assume for now.
> > >
> > > I'd probably just call that user error too.
> > >
> > > > >
> > > > > TL;DR: feature groups are pretty error prone if more than
> > > > > one is listed by the user, or they're combined with individual
> > > > > features.
> > > > >
> > > > > >
> > > > > > - profile support for vendor CPUs consists into checking if the CPU
> > > > > > happens to have the mandatory extensions required for it. In
> > > > > > case it
> > > > > > doesn't we'll error out. This is done to follow the same
> > > > > > prerogative
> > > > > > we always had of not allowing extensions being enabled for
> > > > > > vendor
> > > > > > CPUs;
> > > > >
> > > > > Why shouldn't this be allowed ?
> > > >
> > > > There's no technical reason to not allow it. The reason it's forbid is
> > > > to be
> > > > closer to what the real hardware would do. E.g. the real hardware
> > > > doesn't allow
> > > > users to enable Vector if the hardware doesn't support it. Vendor CPUs
> > > > also has
> > > > a privileged spec restriction as well, so if a CPU is running in an
> > > > older spec
> > > > it can't enable extensions that were added later.
> > >
> > > Real hardware is constrained in not being able to invent arbitrary
> > > new features on chip. Virtual machines are not constrained, so
> > > I don't think the inability of hardware todo this, is an especially
> > > strong reason to limit software emulation.
I think exposing flexibility in vendor CPUs just creates confusion.
As a user if I start QEMU with "-cpu company-cpu" then I am expecting
to get an emulation of company-cpu.
> > >
> > > What I don't like about this, is that (IIUC) the '$profile=on' option
> > > now has different semantics depending on what CPU it is used with.
> > >
> > > ie using it with a vendor CPU, $profile=on becomes an assertion
Re: [PATCH 2/2] target/riscv/tcg-cpu.c: add extension properties for all cpus
On Fri, Sep 29, 2023 at 8:39 PM Daniel P. Berrangé wrote: > > On Tue, Sep 26, 2023 at 03:31:09PM -0300, Daniel Henrique Barboza wrote: > > At this moment we do not expose extension properties for vendor CPUs > > because that would allow users to change them via command line. The > > drawback is that if we were to add an API that shows all CPU properties, > > e.g. qmp-query-cpu-model-expansion, we won't be able to show extensions > > state of vendor CPUs. > > > > We have the required machinery to create extension properties for vendor > > CPUs while not allowing users to enable extensions. Disabling existing > > extensions is allowed since it can be useful for debugging. > > > > Change the set() callback cpu_set_multi_ext_cfg() to allow enabling > > extensions only for generic CPUs. In cpu_add_multi_ext_prop() let's not > > set the default values for the properties if we're not dealing with > > generic CPUs, otherwise the values set in cpu_init() of vendor CPUs will > > be overwritten. And finally, in tcg_cpu_instance_init(), add cpu user > > properties for all CPUs. > > > > For the veyron-v1 CPU, we're now able to disable existing extensions > > like smstateen: > > > > $ ./build/qemu-system-riscv64 --nographic -M virt \ > > -cpu veyron-v1,smstateen=false > > > > But setting extensions that the CPU didn't set during cpu_init(), like > > V, is not allowed: > > > > $ ./build/qemu-system-riscv64 --nographic -M virt \ > > -cpu veyron-v1,v=true > > qemu-system-riscv64: can't apply global veyron-v1-riscv-cpu.v=true: > > 'veyron-v1' CPU does not allow enabling extensions > > Why should we block the user if they want to enable an extra > feature, over and above what is built-in to the CPU model ? It ends up being tricky to maintain. On top of that we can report a specific vendor CPU to guests, but then we don't correctly model it (as extensions might be disabled). Alistair > Is there some technical reason that prevents this from working ? > > With 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 v7 3/5] vhost-user-scsi: support reconnect to backend
Thanks for your comments, I will submit the v8.
> On 8 Oct 2023, at 6:46 PM, Manos Pitsidianakis
> wrote:
>
> Hello Li, I have some trivial style comments you could possibly address in a
> next version:
>
> On Sun, 08 Oct 2023 12:12, Li Feng wrote:
>> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
>> index df6b66cc1a..5df24faff4 100644
>> --- a/hw/scsi/vhost-user-scsi.c
>> +++ b/hw/scsi/vhost-user-scsi.c
>> @@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
>>VHOST_INVALID_FEATURE_BIT
>> };
>> +static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
>> +{
>> +VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>> +int ret;
>> +
>> +ret = vhost_scsi_common_start(vsc, errp);
>> +s->started_vu = (ret < 0 ? false : true);
>
> -+s->started_vu = (ret < 0 ? false : true);
> ++s->started_vu = !(ret < 0);
>
>> static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>>VHostUserSCSI *s = (VHostUserSCSI *)vdev;
>> +DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
>
> -+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
> ++DeviceState *dev = DEVICE(vdev);
>
>> +static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
>> +{
>> +VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> +VHostUserSCSI *s = VHOST_USER_SCSI(vdev);
>> +VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
>> +VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>> +int ret = 0;
>> +
>> +if (s->connected) {
>> +return 0;
>> +}
>> +s->connected = true;
>> +
>> +vsc->dev.num_queues = vs->conf.num_queues;
>> +vsc->dev.nvqs = VIRTIO_SCSI_VQ_NUM_FIXED + vs->conf.num_queues;
>> +vsc->dev.vqs = s->vhost_vqs;
>> +vsc->dev.vq_index = 0;
>> +vsc->dev.backend_features = 0;
>> +
>> +ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
>> VHOST_BACKEND_TYPE_USER, 0,
>> + errp);
>> +if (ret < 0) {
>> +return ret;
>> +}
>> +
>> +/* restore vhost state */
>> +if (virtio_device_started(vdev, vdev->status)) {
>> +ret = vhost_user_scsi_start(s, errp);
>> +if (ret < 0) {
>> +return ret;
>> +}
>> +}
>> +
>> +return 0;
>> +}
>
>
> -+if (virtio_device_started(vdev, vdev->status)) {
> -+ret = vhost_user_scsi_start(s, errp);
> -+if (ret < 0) {
> -+return ret;
> -+}
> -+}
> -+
> -+return 0;
> -+}
> ++if (virtio_device_started(vdev, vdev->status)) {
> ++ret = vhost_user_scsi_start(s, errp);
> ++}
> ++
> ++return ret;
> ++}
>
> [skipping..]
>
>> +static int vhost_user_scsi_realize_connect(VHostUserSCSI *s, Error **errp)
>> +{
>> +DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
>
>
> -+DeviceState *dev = &s->parent_obj.parent_obj.parent_obj.parent_obj;
> ++DeviceState *dev = DEVICE(s);
>
>> diff --git a/include/hw/virtio/vhost-user-scsi.h
>> b/include/hw/virtio/vhost-user-scsi.h
>> index 521b08e559..b405ec952a 100644
>> --- a/include/hw/virtio/vhost-user-scsi.h
>> +++ b/include/hw/virtio/vhost-user-scsi.h
>> @@ -29,6 +29,10 @@ OBJECT_DECLARE_SIMPLE_TYPE(VHostUserSCSI, VHOST_USER_SCSI)
>> struct VHostUserSCSI {
>>VHostSCSICommon parent_obj;
>>VhostUserState vhost_user;
>> +bool connected;
>> +bool started_vu;
>> +
>> +struct vhost_virtqueue *vhost_vqs;
>
> +bool connected;
> +bool started_vu;
> -+
> +struct vhost_virtqueue *vhost_vqs;
>
> See
> https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations
>
> The definition should look like:
>
> struct VHostUserSCSI {
> VHostSCSICommon parent_obj;
>
> /* Properties */
> bool connected;
> bool started_vu;
>
> VhostUserState vhost_user;
> struct vhost_virtqueue *vhost_vqs;
> }
[PATCH v8 2/5] vhost: move and rename the conn retry times
Multiple devices need this macro, move it to a common header.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/block/vhost-user-blk.c | 4 +---
hw/virtio/vhost-user-gpio.c | 3 +--
include/hw/virtio/vhost.h | 2 ++
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index eecf3f7a81..3c69fa47d5 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -32,8 +32,6 @@
#include "sysemu/sysemu.h"
#include "sysemu/runstate.h"
-#define REALIZE_CONNECTION_RETRIES 3
-
static const int user_feature_bits[] = {
VIRTIO_BLK_F_SIZE_MAX,
VIRTIO_BLK_F_SEG_MAX,
@@ -482,7 +480,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev,
Error **errp)
s->inflight = g_new0(struct vhost_inflight, 1);
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
assert(!*errp);
do {
if (*errp) {
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3d7fae3984..fc784e4213 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -15,7 +15,6 @@
#include "standard-headers/linux/virtio_ids.h"
#include "trace.h"
-#define REALIZE_CONNECTION_RETRIES 3
#define VHOST_NVQS 2
/* Features required from VirtIO */
@@ -365,7 +364,7 @@ static void vu_gpio_device_realize(DeviceState *dev, Error
**errp)
qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
dev, NULL, true);
-retries = REALIZE_CONNECTION_RETRIES;
+retries = VU_REALIZE_CONN_RETRIES;
g_assert(!*errp);
do {
if (*errp) {
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 6a173cb9fa..ca3131b1af 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -8,6 +8,8 @@
#define VHOST_F_DEVICE_IOTLB 63
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VU_REALIZE_CONN_RETRIES 3
+
/* Generic structures common for any vhost based device. */
struct vhost_inflight {
--
2.41.0
[PATCH v8 0/5] Implement reconnect for vhost-user-scsi
Changes for v8: - [PATCH 3/5] vhost-user-scsi: support reconnect to backend - Fix code style suggested by Manos Pitsidianakis - [PATCH 4/5] vhost-user-scsi: start vhost when guest kicks - Use 'DEVICE()' macro in vhost_user_scsi_handle_output to replace the 'parent_obj.parent_obj.parent_obj.parent_obj'. Changes for v7: - [PATCH 3/5] vhost-user-scsi: support reconnect to backend - Add reporting the error in vhost-scsi; - Rebase to master and fix the conflict. - Add "Reviewed-by" tags. Changes for v6: - [PATCH] vhost-user: fix lost reconnect - Fix missing assign event_cb. Changes for v5: - No logic has been changed, just move part of the code from patch 4 to patch 5. Changes for v4: - Merge https://lore.kernel.org/all/20230830045722.611224-1-fen...@smartx.com/ to this series. - Add ERRP_GUARD in vhost_user_scsi_realize; - Reword the commit messages. Changes for v3: - Split the vhost_user_scsi_handle_output to a separate patch; - Move the started_vu from vhost scsi common header to vhost-user-scsi header; - Fix a log print error; Changes for v2: - Split the v1 patch to small separate patchset; - New patch for fixing fd leak, which has sent to reviewers in another mail; - Implement the `vhost_user_scsi_handle_output`; - Add the started_vu safe check; - Fix error handler; - Check the inflight before set/get inflight fd. Li Feng (5): vhost-user-common: send get_inflight_fd once vhost: move and rename the conn retry times vhost-user-scsi: support reconnect to backend vhost-user-scsi: start vhost when guest kicks vhost-user: fix lost reconnect hw/block/vhost-user-blk.c | 6 +- hw/scsi/vhost-scsi-common.c | 47 ++--- hw/scsi/vhost-scsi.c | 6 +- hw/scsi/vhost-user-scsi.c | 250 +++--- hw/virtio/vhost-user-gpio.c | 5 +- hw/virtio/vhost-user.c| 10 +- include/hw/virtio/vhost-scsi-common.h | 2 +- include/hw/virtio/vhost-user-scsi.h | 6 + include/hw/virtio/vhost-user.h| 3 +- include/hw/virtio/vhost.h | 2 + 10 files changed, 277 insertions(+), 60 deletions(-) -- 2.41.0
[PATCH v8 4/5] vhost-user-scsi: start vhost when guest kicks
Let's keep the same behavior as vhost-user-blk.
Some old guests kick virtqueue before setting VIRTIO_CONFIG_S_DRIVER_OK.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/scsi/vhost-user-scsi.c | 48 +++
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 1e5d853cdd..2457b950e1 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -111,8 +111,48 @@ static void vhost_user_scsi_reset(VirtIODevice *vdev)
}
}
-static void vhost_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+static void vhost_user_scsi_handle_output(VirtIODevice *vdev, VirtQueue *vq)
{
+VHostUserSCSI *s = (VHostUserSCSI *)vdev;
+DeviceState *dev = DEVICE(vdev);
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
+
+Error *local_err = NULL;
+int i, ret;
+
+if (!vdev->start_on_kick) {
+return;
+}
+
+if (!s->connected) {
+return;
+}
+
+if (vhost_dev_is_started(&vsc->dev)) {
+return;
+}
+
+/*
+ * Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
+ * vhost here instead of waiting for .set_status().
+ */
+ret = vhost_user_scsi_start(s, &local_err);
+if (ret < 0) {
+error_reportf_err(local_err, "vhost-user-scsi: vhost start failed: ");
+qemu_chr_fe_disconnect(&vs->conf.chardev);
+return;
+}
+
+/* Kick right away to begin processing requests already in vring */
+for (i = 0; i < vsc->dev.nvqs; i++) {
+VirtQueue *kick_vq = virtio_get_queue(vdev, i);
+
+if (!virtio_queue_get_desc_addr(vdev, i)) {
+continue;
+}
+event_notifier_set(virtio_queue_get_host_notifier(kick_vq));
+}
}
static int vhost_user_scsi_connect(DeviceState *dev, Error **errp)
@@ -239,9 +279,9 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error
**errp)
return;
}
-virtio_scsi_common_realize(dev, vhost_dummy_handle_output,
- vhost_dummy_handle_output,
- vhost_dummy_handle_output, &err);
+virtio_scsi_common_realize(dev, vhost_user_scsi_handle_output,
+ vhost_user_scsi_handle_output,
+ vhost_user_scsi_handle_output, &err);
if (err != NULL) {
error_propagate(errp, err);
return;
--
2.41.0
[PATCH v8 1/5] vhost-user-common: send get_inflight_fd once
Currently the get_inflight_fd will be sent every time the device is started, and
the backend will allocate shared memory to save the inflight state. If the
backend finds that it receives the second get_inflight_fd, it will release the
previous shared memory, which breaks inflight working logic.
This patch is a preparation for the following patches.
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/scsi/vhost-scsi-common.c | 37 ++---
1 file changed, 18 insertions(+), 19 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a06f01af26..a61cd0e907 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -52,20 +52,28 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vsc->dev.acked_features = vdev->guest_features;
-assert(vsc->inflight == NULL);
-vsc->inflight = g_new0(struct vhost_inflight, 1);
-ret = vhost_dev_get_inflight(&vsc->dev,
- vs->conf.virtqueue_size,
- vsc->inflight);
+ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error get inflight: %d", -ret);
+error_report("Error setting inflight format: %d", -ret);
goto err_guest_notifiers;
}
-ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
-if (ret < 0) {
-error_report("Error set inflight: %d", -ret);
-goto err_guest_notifiers;
+if (vsc->inflight) {
+if (!vsc->inflight->addr) {
+ret = vhost_dev_get_inflight(&vsc->dev,
+vs->conf.virtqueue_size,
+vsc->inflight);
+if (ret < 0) {
+error_report("Error getting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
+}
+
+ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
+if (ret < 0) {
+error_report("Error setting inflight: %d", -ret);
+goto err_guest_notifiers;
+}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
@@ -85,9 +93,6 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
return ret;
err_guest_notifiers:
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-
k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, false);
err_host_notifiers:
vhost_dev_disable_notifiers(&vsc->dev, vdev);
@@ -111,12 +116,6 @@ void vhost_scsi_common_stop(VHostSCSICommon *vsc)
}
assert(ret >= 0);
-if (vsc->inflight) {
-vhost_dev_free_inflight(vsc->inflight);
-g_free(vsc->inflight);
-vsc->inflight = NULL;
-}
-
vhost_dev_disable_notifiers(&vsc->dev, vdev);
}
--
2.41.0
[PATCH v8 5/5] vhost-user: fix lost reconnect
When the vhost-user is reconnecting to the backend, and if the vhost-user fails
at the get_features in vhost_dev_init(), then the reconnect will fail
and it will not be retriggered forever.
The reason is:
When the vhost-user fails at get_features, the vhost_dev_cleanup will be called
immediately.
vhost_dev_cleanup calls 'memset(hdev, 0, sizeof(struct vhost_dev))'.
The reconnect path is:
vhost_user_blk_event
vhost_user_async_close(.. vhost_user_blk_disconnect ..)
qemu_chr_fe_set_handlers <- clear the notifier callback
schedule vhost_user_async_close_bh
The vhost->vdev is null, so the vhost_user_blk_disconnect will not be
called, then the event fd callback will not be reinstalled.
All vhost-user devices have this issue, including vhost-user-blk/scsi.
With this patch, if the vdev->vdev is null, the fd callback will still
be reinstalled.
Fixes: 71e076a07d ("hw/virtio: generalise CHR_EVENT_CLOSED handling")
Signed-off-by: Li Feng
Reviewed-by: Raphael Norwitz
---
hw/block/vhost-user-blk.c | 2 +-
hw/scsi/vhost-user-scsi.c | 3 ++-
hw/virtio/vhost-user-gpio.c| 2 +-
hw/virtio/vhost-user.c | 10 --
include/hw/virtio/vhost-user.h | 3 ++-
5 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3c69fa47d5..95c758200d 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -391,7 +391,7 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent
event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &s->chardev, &s->dev,
- vhost_user_blk_disconnect);
+ vhost_user_blk_disconnect,
vhost_user_blk_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 2457b950e1..61e4050682 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -231,7 +231,8 @@ static void vhost_user_scsi_event(void *opaque,
QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &vs->conf.chardev, &vsc->dev,
- vhost_user_scsi_disconnect);
+ vhost_user_scsi_disconnect,
+ vhost_user_scsi_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index fc784e4213..aff2d7eff6 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -289,7 +289,7 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
case CHR_EVENT_CLOSED:
/* defer close until later to avoid circular close */
vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
- vu_gpio_disconnect);
+ vu_gpio_disconnect, vu_gpio_event);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 3766b415f8..7395bfc531 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2765,6 +2765,7 @@ typedef struct {
DeviceState *dev;
CharBackend *cd;
struct vhost_dev *vhost;
+IOEventHandler *event_cb;
} VhostAsyncCallback;
static void vhost_user_async_close_bh(void *opaque)
@@ -2779,7 +2780,10 @@ static void vhost_user_async_close_bh(void *opaque)
*/
if (vhost->vdev) {
data->cb(data->dev);
-}
+} else if (data->event_cb) {
+qemu_chr_fe_set_handlers(data->cd, NULL, NULL, data->event_cb,
+ NULL, data->dev, NULL, true);
+ }
g_free(data);
}
@@ -2791,7 +2795,8 @@ static void vhost_user_async_close_bh(void *opaque)
*/
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
-vu_async_close_fn cb)
+vu_async_close_fn cb,
+IOEventHandler *event_cb)
{
if (!runstate_check(RUN_STATE_SHUTDOWN)) {
/*
@@ -2807,6 +2812,7 @@ void vhost_user_async_close(DeviceState *d,
data->dev = d;
data->cd = chardev;
data->vhost = vhost;
+data->event_cb = event_cb;
/* Disable any further notifications on the chardev */
qemu_chr_fe_set_handlers(chardev,
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 9f9ddf878d..6b06ecb1bd 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -106,6 +106,7 @@ typedef void (*vu_async_close_fn)(DeviceState *cb);
void vhost_user_async_close(DeviceState *d,
CharBackend *chardev, struct vhost_dev *vhost,
-
[PATCH v8 3/5] vhost-user-scsi: support reconnect to backend
If the backend crashes and restarts, the device is broken.
This patch adds reconnect for vhost-user-scsi.
This patch also improves the error messages, and reports some silent errors.
Tested with spdk backend.
Signed-off-by: Li Feng
---
hw/scsi/vhost-scsi-common.c | 16 +-
hw/scsi/vhost-scsi.c | 6 +-
hw/scsi/vhost-user-scsi.c | 201 +++---
include/hw/virtio/vhost-scsi-common.h | 2 +-
include/hw/virtio/vhost-user-scsi.h | 6 +
5 files changed, 201 insertions(+), 30 deletions(-)
diff --git a/hw/scsi/vhost-scsi-common.c b/hw/scsi/vhost-scsi-common.c
index a61cd0e907..4c8637045d 100644
--- a/hw/scsi/vhost-scsi-common.c
+++ b/hw/scsi/vhost-scsi-common.c
@@ -16,6 +16,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/module.h"
#include "hw/virtio/vhost.h"
@@ -25,7 +26,7 @@
#include "hw/virtio/virtio-access.h"
#include "hw/fw-path-provider.h"
-int vhost_scsi_common_start(VHostSCSICommon *vsc)
+int vhost_scsi_common_start(VHostSCSICommon *vsc, Error **errp)
{
int ret, i;
VirtIODevice *vdev = VIRTIO_DEVICE(vsc);
@@ -35,18 +36,19 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
VirtIOSCSICommon *vs = (VirtIOSCSICommon *)vsc;
if (!k->set_guest_notifiers) {
-error_report("binding does not support guest notifiers");
+error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
}
ret = vhost_dev_enable_notifiers(&vsc->dev, vdev);
if (ret < 0) {
+error_setg_errno(errp, -ret, "Error enabling host notifiers");
return ret;
}
ret = k->set_guest_notifiers(qbus->parent, vsc->dev.nvqs, true);
if (ret < 0) {
-error_report("Error binding guest notifier");
+error_setg_errno(errp, -ret, "Error binding guest notifier");
goto err_host_notifiers;
}
@@ -54,7 +56,7 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
ret = vhost_dev_prepare_inflight(&vsc->dev, vdev);
if (ret < 0) {
-error_report("Error setting inflight format: %d", -ret);
+error_setg_errno(errp, -ret, "Error setting inflight format");
goto err_guest_notifiers;
}
@@ -64,21 +66,21 @@ int vhost_scsi_common_start(VHostSCSICommon *vsc)
vs->conf.virtqueue_size,
vsc->inflight);
if (ret < 0) {
-error_report("Error getting inflight: %d", -ret);
+error_setg_errno(errp, -ret, "Error getting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_set_inflight(&vsc->dev, vsc->inflight);
if (ret < 0) {
-error_report("Error setting inflight: %d", -ret);
+error_setg_errno(errp, -ret, "Error setting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_start(&vsc->dev, vdev, true);
if (ret < 0) {
-error_report("Error start vhost dev");
+error_setg_errno(errp, -ret, "Error starting vhost dev");
goto err_guest_notifiers;
}
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 443f67daa4..95cadb93e7 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -75,6 +75,7 @@ static int vhost_scsi_start(VHostSCSI *s)
int ret, abi_version;
VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
const VhostOps *vhost_ops = vsc->dev.vhost_ops;
+Error *local_err = NULL;
ret = vhost_ops->vhost_scsi_get_abi_version(&vsc->dev, &abi_version);
if (ret < 0) {
@@ -88,14 +89,15 @@ static int vhost_scsi_start(VHostSCSI *s)
return -ENOSYS;
}
-ret = vhost_scsi_common_start(vsc);
+ret = vhost_scsi_common_start(vsc, &local_err);
if (ret < 0) {
+error_reportf_err(local_err, "Error starting vhost-scsi");
return ret;
}
ret = vhost_scsi_set_endpoint(s);
if (ret < 0) {
-error_report("Error setting vhost-scsi endpoint");
+error_reportf_err(local_err, "Error setting vhost-scsi endpoint");
vhost_scsi_common_stop(vsc);
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index df6b66cc1a..1e5d853cdd 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,26 +39,56 @@ static const int user_feature_bits[] = {
VHOST_INVALID_FEATURE_BIT
};
+static int vhost_user_scsi_start(VHostUserSCSI *s, Error **errp)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+int ret;
+
+ret = vhost_scsi_common_start(vsc, errp);
+s->started_vu = !(ret < 0);
+
+return ret;
+}
+
+static void vhost_user_scsi_stop(VHostUserSCSI *s)
+{
+VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
+
+if (!s->started_vu) {
+return;
+}
+s->started_vu = false;
+
+vhost_scsi_common_stop(vsc);
+}
+
static void vhost_user_scsi_set_status(VirtI
Re: [PATCH] hw/virtio/vhost: Silence compiler warnings in vhost code when using -Wshadow
Michael Tokarev writes: > 06.10.2023 14:24, Michael S. Tsirkin: >> On Fri, Oct 06, 2023 at 01:45:51PM +0300, Michael Tokarev wrote: >>> 06.10.2023 11:55, Markus Armbruster пишет: Michael Tokarev writes: > Applied to my trivial-patches tree, thanks! > > Marcus, you picked up previous patches of this theme, -- > you can continue this tradition if you like :) I intend to post a pull request for the -Wshadow patches that have R-bys. I'm also tracking the unreviewed ones, and hope they get reviewed. >>> >>> Ahh, ok. >>> >>> I've added my own R-bys for the ones I picked up, and for this one >>> by Thomas too: >>> >>> Reviewed-By: Michael Tokarev >> >> it would be better to deal with all of them in one place tbh. > > This is exactly why I asked Marcus about this in the first place, > being confused a bit with the stuff being especially sent to > qemu-trivial@ :) This patch is in my latest PR[*], along with all the other shadow patches that were ready at the time. A few more are coming down the pipe, which I can take through my tree, too. [*] [PULL 00/32] -Wshadow=local patches patches for 2023-10-06 Message-ID: <20231006113657.3803180-1-arm...@redhat.com>
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
Hi Brian,
On 6/10/23 00:22, Brian Cain wrote:
The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
identifiers to avoid shadowing the type name.
This one surprises me, since we have other occurences:
include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry
*iotlb, void **vaddr,
include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState *cpu,
uint64_t vaddr,
target/arm/internals.h:643:G_NORETURN void
arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
target/i386/tcg/helper-tcg.h:70:G_NORETURN void
handle_unaligned_access(CPUX86State *env, vaddr vaddr,
...
$ git grep -w vaddr, | wc -l
207
What is the error/warning like?
The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
rename the function arguments to avoid shadowing the global.
Signed-off-by: Brian Cain
---
target/hexagon/genptr.c | 56 -
target/hexagon/genptr.h | 18
target/hexagon/mmvec/system_ext_mmvec.c | 4 +-
target/hexagon/mmvec/system_ext_mmvec.h | 2 +-
target/hexagon/op_helper.c | 4 +-
5 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 217bc7bb5a..11377ac92b 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
tcg_gen_deposit_i64(result, result, src64, N * 8, 8);
}
-static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index)
{
-tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
-tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+tcg_gen_qemu_ld_tl(dest, v_addr, mem_index, MO_TEUL);
+tcg_gen_mov_tl(hex_llsc_addr, v_addr);
tcg_gen_mov_tl(hex_llsc_val, dest);
}
Re: [PATCH v3 1/3] target/hexagon: move GETPC() calls to top level helpers
On 9/10/23 00:09, Brian Cain wrote: From: Matheus Tavares Bernardino As docs/devel/loads-stores.rst states: ``GETPC()`` should be used with great care: calling it in other functions that are *not* the top level ``HELPER(foo)`` will cause unexpected behavior. Instead, the value of ``GETPC()`` should be read from the helper and passed if needed to the functions that the helper calls. Let's fix the GETPC() usage in Hexagon, making sure it's always called from top level helpers and passed down to the places where it's needed. There are a few snippets where that is not currently the case: - probe_store(), which is only called from two helpers, so it's easy to move GETPC() up. - mem_load*() functions, which are also called directly from helpers, but through the MEM_LOAD*() set of macros. Note that this are only used when compiling with --disable-hexagon-idef-parser. In this case, we also take this opportunity to simplify the code, unifying the mem_load*() functions. - HELPER(probe_hvx_stores), when called from another helper, ends up using its own GETPC() expansion instead of the top level caller. Signed-off-by: Matheus Tavares Bernardino Reviewed-by: Taylor Simpson Message-Id: <2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathb...@quicinc.com>-ne Again suspicious '-ne' trailing. Reviewed-by: Brian Cain Signed-off-by: Brian Cain --- target/hexagon/macros.h| 19 +- target/hexagon/op_helper.c | 75 +++--- target/hexagon/op_helper.h | 9 - 3 files changed, 38 insertions(+), 65 deletions(-)
Re: [PATCH v3 2/3] target/hexagon: fix some occurrences of -Wshadow=local
On 9/10/23 00:09, Brian Cain wrote:
Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
are less obvious. They are required because of some macro invocations like
SCATTER_OP_WRITE_TO_MEM().
e.g.:
In file included from ../target/hexagon/op_helper.c:31:
../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows
a previous local [-Werror=shadow=compatible-local]
205 | for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) {
\
| ^
../target/hexagon/op_helper.c:157:17: note: in expansion of macro
‘SCATTER_OP_WRITE_TO_MEM’
157 | SCATTER_OP_WRITE_TO_MEM(uint16_t);
| ^~~
../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
135 | int i;
| ^
In file included from ../target/hexagon/op_helper.c:31:
../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’
shadows a previous local [-Werror=shadow=compatible-local]
204 | uintptr_t ra = GETPC(); \
| ^~
../target/hexagon/op_helper.c:160:17: note: in expansion of macro
‘SCATTER_OP_WRITE_TO_MEM’
160 | SCATTER_OP_WRITE_TO_MEM(uint32_t);
| ^~~
../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
134 | uintptr_t ra = GETPC();
| ^~
Reviewed-by: Matheus Tavares Bernardino
Signed-off-by: Brian Cain
---
target/hexagon/imported/alu.idef | 6 +++---
target/hexagon/mmvec/macros.h| 2 +-
target/hexagon/op_helper.c | 9 +++--
target/hexagon/translate.c | 10 +-
4 files changed, 12 insertions(+), 15 deletions(-)
No change since v2:
https://lore.kernel.org/qemu-devel/45707b6e-6835-421f-e89b-6c1b0c50e...@linaro.org/
Please carry reviewer tags so we don't re-review patches.
Again,
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v3 0/3] hexagon: GETPC() fixes, shadowing fixes
I applied this series to my shadow-next in my public Git repository at https://repo.or.cz/qemu/armbru.git. I don't intend to include it in pull requests, because it does more than just fix -Wshadow issues. Don't think that'll be a problem. Thanks!
Re: [v3] Help wanted for enabling -Wshadow=local
Warner Losh writes: > On Fri, Oct 6, 2023, 11:55 AM Thomas Huth wrote: > >> On 06/10/2023 18.18, Thomas Huth wrote: >> > On 06/10/2023 16.45, Markus Armbruster wrote: >> >> Local variables shadowing other local variables or parameters make the >> >> code needlessly hard to understand. Bugs love to hide in such code. >> >> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail >> >> on polling error". >> >> >> >> Enabling -Wshadow would prevent bugs like this one. But we have to >> >> clean up all the offenders first. >> >> >> >> Quite a few people responded to my calls for help. Thank you so much! >> >> >> >> I'm collecting patches in my git repo at >> >> https://repo.or.cz/qemu/armbru.git in branch shadow-next. All but the >> >> last two are in a pending pull request. [...] >> >> More warnings may lurk in code my test build doesn't compile. Need a >> >> full CI build with -Wshadow=local to find them. Anybody care to kick >> >> one off? >> > >> > I ran a build here (with -Werror enabled, so that it's easier to see where >> > it breaks): >> > >> > https://gitlab.com/thuth/qemu/-/pipelines/1028023489 >> > >> > ... but I didn't see any additional spots in the logs beside the ones that >> > you already listed. >> >> After adding two more patches to fix the above warnings, things look >> pretty >> good: >> >> https://gitlab.com/thuth/qemu/-/pipelines/1028413030 >> >> There are just some warnings left in the BSD code, as Warner already >> mentioned in his reply to v2 of your mail: >> >> https://gitlab.com/thuth/qemu/-/jobs/5241420713 > > > I think I have fixes for these. I need to merge what just landed into > bsd-user fork, rebase, test, the apply them to qemu master branch, retest > and send them off... > > My illness has hung on longer than I thought so I'm still behind... Get well, and looking forward to your patches!
Re: [PATCH 3/3] hw/ppc: Add emulation of AmigaOne XE board
Hi Zoltan,
On 6/10/23 00:13, BALATON Zoltan wrote:
The AmigaOne is a rebranded MAI Teron board that uses U-Boot firmware
with patches to support AmigaOS and is very similar to pegasos2 so can
be easily emulated sharing most code with pegasos2. The reason to
emulate it is that AmigaOS comes in different versions for AmigaOne
and PegasosII which only have drivers for one machine and firmware so
these only run on the specific machine. Adding this board allows
another AmigaOS version to be used reusing already existing peagasos2
emulation. (The AmigaOne was the first of these boards so likely most
widespread which then inspired Pegasos that was later replaced with
PegasosII due to problems with Articia S, so these have a lot of
similarity. Pegasos mainly ran MorphOS while the PegasosII version of
AmigaOS was added later and therefore less common than the AmigaOne
version.)
Signed-off-by: BALATON Zoltan
---
MAINTAINERS | 8 ++
configs/devices/ppc-softmmu/default.mak | 1 +
hw/ppc/Kconfig | 7 +
hw/ppc/amigaone.c | 164
hw/ppc/meson.build | 2 +
5 files changed, 182 insertions(+)
create mode 100644 hw/ppc/amigaone.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f0e20fde6..03f908c153 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1490,6 +1490,14 @@ F: hw/pci-host/mv64361.c
F: hw/pci-host/mv643xx.h
F: include/hw/pci-host/mv64361.h
+amigaone
'AmigaOne' like in subject and description?
+M: BALATON Zoltan
+L: qemu-...@nongnu.org
+S: Maintained
+F: hw/ppc/amigaone.c
+F: hw/pci-host/articia.c
+F: include/hw/pci-host/articia.h
+
Virtual Open Firmware (VOF)
M: Alexey Kardashevskiy
+static void amigaone_init(MachineState *machine)
+{
+PowerPCCPU *cpu;
+CPUPPCState *env;
+MemoryRegion *rom, *pci_mem, *mr;
+const char *fwname = machine->firmware ?: PROM_FILENAME;
+char *filename;
+ssize_t sz;
+PCIBus *pci_bus;
+Object *via;
+DeviceState *dev;
+I2CBus *i2c_bus;
+uint8_t *spd_data;
+int i;
+
+/* init CPU */
+cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
+env = &cpu->env;
+if (PPC_INPUT(env) != PPC_FLAGS_INPUT_6xx) {
+error_report("Incompatible CPU, only 6xx bus supported");
+exit(1);
+}
+cpu_ppc_tb_init(env, BUS_FREQ_HZ / 4);
+qemu_register_reset(amigaone_cpu_reset, cpu);
+
+/* RAM */
+if (machine->ram_size > 2 * GiB) {
+error_report("RAM size more than 2 GiB is not supported");
+exit(1);
+}
+memory_region_add_subregion(get_system_memory(), 0, machine->ram);
+if (machine->ram_size < 1 * GiB + 32 * KiB) {
+/* Firmware uses this area for startup */
This is odd. Does this machine really support 2GiB?
Could it be 1GiB max, mapped twice?
+mr = g_new(MemoryRegion, 1);
+memory_region_init_ram(mr, NULL, "init-cache", 32 * KiB, &error_fatal);
+memory_region_add_subregion(get_system_memory(), 0x4000, mr);
+}
+
+/* allocate and load firmware */
+filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, fwname);
+if (!filename) {
+error_report("Could not find firmware '%s'", fwname);
+exit(1);
+}
+rom = g_new(MemoryRegion, 1);
+memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
+memory_region_add_subregion(get_system_memory(), PROM_ADDR, rom);
+sz = load_image_targphys(filename, PROM_ADDR, PROM_SIZE);
+if (sz <= 0 || sz > PROM_SIZE) {
+error_report("Could not load firmware '%s'", filename);
+exit(1);
+}
+g_free(filename);
+
+/* Articia S */
+dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe00, NULL);
+
+i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
+if (machine->ram_size > 512 * MiB) {
+spd_data = spd_data_generate(SDR, machine->ram_size / 2);
+} else {
+spd_data = spd_data_generate(SDR, machine->ram_size);
+}
+fix_spd_data(spd_data);
+smbus_eeprom_init_one(i2c_bus, 0x51, spd_data);
+if (machine->ram_size > 512 * MiB) {
+smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
+}
This seems to confirm my doubts, you use at most 2 SPD of 512MiB DIMMs,
so max for this machine is 1 GiB.
+pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
+mr = g_new(MemoryRegion, 1);
+memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
+ 0, 0x100);
+memory_region_add_subregion(get_system_memory(), 0xfd00, mr);
+mr = g_new(MemoryRegion, 1);
+memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
+ 0x8000, 0x7d00);
+memory_region_add_subregion(get_system_memory(), 0x8000, mr);
+pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
+
+/* VIA VT82c686B South Bridge (multifunction PCI device) */
+via = OBJECT(pci_create_simple_multifunct
Re: [PATCH 2/3] vfio/ap: Remove pointless apdev variable
On 9/10/23 04:20, Zhenzhong Duan wrote: No need to double-cast, call VFIO_AP_DEVICE() on DeviceState. No functional changes. Signed-off-by: Zhenzhong Duan --- hw/vfio/ap.c | 9 +++-- 1 file changed, 3 insertions(+), 6 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 3/3] vfio/ccw: Remove redundant definition of TYPE_VFIO_CCW
On 9/10/23 04:20, Zhenzhong Duan wrote: No functional changes. Signed-off-by: Zhenzhong Duan --- include/hw/s390x/vfio-ccw.h | 2 -- 1 file changed, 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH 0/3] vfio: memory leak fix and code cleanup
On 10/9/23 04:20, Zhenzhong Duan wrote: Hi, This trivial patchset fixes a incremental memory leak in rare case, and some cleanup on ap/ccw. This patchset is based on vfio-next. Will this apply on the future v5 of Eric ? Thanks, C. Thanks Zhenzhong Zhenzhong Duan (3): vfio/pci: Fix a potential memory leak in vfio_listener_region_add vfio/ap: Remove pointless apdev variable vfio/ccw: Remove redundant definition of TYPE_VFIO_CCW hw/vfio/ap.c| 9 +++-- hw/vfio/common.c| 2 +- include/hw/s390x/vfio-ccw.h | 2 -- 3 files changed, 4 insertions(+), 9 deletions(-)
[PATCH v5 0/3] ramfb: migration support
From: Marc-André Lureau Hi, Implement RAMFB migration, and add properties to enable it only on >= 8.2 machines, + a few related cleanups. thanks v5: - add missing VMSTATE_END_OF_LIST - changed ramfb=off & x-mig=on user config error to a warning - add r-b tags v4: (Laszlo review and suggestions) - change migrate_needed() to assert(ramfb_exists) - rename vfio_display_needed() to vfio_display_migration_needed(), update the condition and associated comment - move the ramfb-migrate option check and add a check for ramfb=on - add a stub to fix compilation on some architectures v3: - add a "x-" prefix to properties, as they are not meant for users. - RAMFB now exports a ramfb_vmstate for actual devices to include - VFIOPCIDevice now has a VFIODisplay optional subsection whenever ramfb migration is required (untested) Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1859424 Marc-André Lureau (3): ramfb: add migration support ramfb-standalone: add migration support hw/vfio: add ramfb migration support hw/vfio/pci.h | 3 +++ include/hw/display/ramfb.h| 4 hw/core/machine.c | 2 ++ hw/display/ramfb-standalone.c | 27 + hw/display/ramfb.c| 19 +++ hw/vfio/display.c | 21 + hw/vfio/pci.c | 44 +++ stubs/ramfb.c | 2 ++ 8 files changed, 122 insertions(+) -- 2.41.0
[PATCH v5 2/3] ramfb-standalone: add migration support
From: Marc-André Lureau
Add a "ramfb-dev" section whenever "x-migrate" is turned on. Turn it off
by default on machines <= 8.1 for compatibility reasons.
Signed-off-by: Marc-André Lureau
Reviewed-by: Laszlo Ersek
---
hw/core/machine.c | 1 +
hw/display/ramfb-standalone.c | 27 +++
2 files changed, 28 insertions(+)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index cfd1edfe20..6305f2d7a4 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@
GlobalProperty hw_compat_8_1[] = {
{ TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
+{ "ramfb", "x-migrate", "off" },
};
const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
index 8c0094397f..a96e7ebcd9 100644
--- a/hw/display/ramfb-standalone.c
+++ b/hw/display/ramfb-standalone.c
@@ -1,4 +1,5 @@
#include "qemu/osdep.h"
+#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qemu/module.h"
#include "hw/loader.h"
@@ -15,6 +16,7 @@ struct RAMFBStandaloneState {
SysBusDevice parent_obj;
QemuConsole *con;
RAMFBState *state;
+bool migrate;
};
static void display_update_wrapper(void *dev)
@@ -40,14 +42,39 @@ static void ramfb_realizefn(DeviceState *dev, Error **errp)
ramfb->state = ramfb_setup(errp);
}
+static bool migrate_needed(void *opaque)
+{
+RAMFBStandaloneState *ramfb = RAMFB(opaque);
+
+return ramfb->migrate;
+}
+
+static const VMStateDescription ramfb_dev_vmstate = {
+.name = "ramfb-dev",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = migrate_needed,
+.fields = (VMStateField[]) {
+VMSTATE_STRUCT_POINTER(state, RAMFBStandaloneState, ramfb_vmstate,
RAMFBState),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static Property ramfb_properties[] = {
+DEFINE_PROP_BOOL("x-migrate", RAMFBStandaloneState, migrate, true),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void ramfb_class_initfn(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
+dc->vmsd = &ramfb_dev_vmstate;
dc->realize = ramfb_realizefn;
dc->desc = "ram framebuffer standalone device";
dc->user_creatable = true;
+device_class_set_props(dc, ramfb_properties);
}
static const TypeInfo ramfb_info = {
--
2.41.0
[PATCH v5 1/3] ramfb: add migration support
From: Marc-André Lureau
Implementing RAMFB migration is quite straightforward. One caveat is to
treat the whole RAMFBCfg as a blob, since that's what is exposed to the
guest directly. This avoid having to fiddle with endianness issues if we
were to migrate fields individually as integers.
The devices using RAMFB will have to include ramfb_vmstate in their
migration description.
Signed-off-by: Marc-André Lureau
Reviewed-by: Laszlo Ersek
---
include/hw/display/ramfb.h | 4
hw/display/ramfb.c | 19 +++
2 files changed, 23 insertions(+)
diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
index b33a2c467b..a7e0019144 100644
--- a/include/hw/display/ramfb.h
+++ b/include/hw/display/ramfb.h
@@ -1,11 +1,15 @@
#ifndef RAMFB_H
#define RAMFB_H
+#include "migration/vmstate.h"
+
/* ramfb.c */
typedef struct RAMFBState RAMFBState;
void ramfb_display_update(QemuConsole *con, RAMFBState *s);
RAMFBState *ramfb_setup(Error **errp);
+extern const VMStateDescription ramfb_vmstate;
+
/* ramfb-standalone.c */
#define TYPE_RAMFB_DEVICE "ramfb"
diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
index c2b002d534..477ef7272a 100644
--- a/hw/display/ramfb.c
+++ b/hw/display/ramfb.c
@@ -28,6 +28,8 @@ struct QEMU_PACKED RAMFBCfg {
uint32_t stride;
};
+typedef struct RAMFBCfg RAMFBCfg;
+
struct RAMFBState {
DisplaySurface *ds;
uint32_t width, height;
@@ -116,6 +118,23 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
dpy_gfx_update_full(con);
}
+static int ramfb_post_load(void *opaque, int version_id)
+{
+ramfb_fw_cfg_write(opaque, 0, 0);
+return 0;
+}
+
+const VMStateDescription ramfb_vmstate = {
+.name = "ramfb",
+.version_id = 1,
+.minimum_version_id = 1,
+.post_load = ramfb_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_BUFFER_UNSAFE(cfg, RAMFBState, 0, sizeof(RAMFBCfg)),
+VMSTATE_END_OF_LIST()
+}
+};
+
RAMFBState *ramfb_setup(Error **errp)
{
FWCfgState *fw_cfg = fw_cfg_find();
--
2.41.0
[PATCH v5 3/3] hw/vfio: add ramfb migration support
From: Marc-André Lureau
Add a "VFIODisplay" subsection whenever "x-ramfb-migrate" is turned on.
Turn it off by default on machines <= 8.1 for compatibility reasons.
Signed-off-by: Marc-André Lureau
Reviewed-by: Laszlo Ersek
---
hw/vfio/pci.h | 3 +++
hw/core/machine.c | 1 +
hw/vfio/display.c | 21 +
hw/vfio/pci.c | 44
stubs/ramfb.c | 2 ++
5 files changed, 71 insertions(+)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 2d836093a8..fd06695542 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -173,6 +173,7 @@ struct VFIOPCIDevice {
bool no_kvm_ioeventfd;
bool no_vfio_ioeventfd;
bool enable_ramfb;
+OnOffAuto ramfb_migrate;
bool defer_kvm_irq_routing;
bool clear_parent_atomics_on_exit;
VFIODisplay *dpy;
@@ -226,4 +227,6 @@ void vfio_display_reset(VFIOPCIDevice *vdev);
int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
void vfio_display_finalize(VFIOPCIDevice *vdev);
+extern const VMStateDescription vfio_display_vmstate;
+
#endif /* HW_VFIO_VFIO_PCI_H */
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 6305f2d7a4..05aef2cf9f 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -35,6 +35,7 @@
GlobalProperty hw_compat_8_1[] = {
{ TYPE_PCI_BRIDGE, "x-pci-express-writeable-slt-bug", "true" },
{ "ramfb", "x-migrate", "off" },
+{ "vfio-pci-nohotplug", "x-ramfb-migrate", "off" }
};
const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index bec864f482..2739ba56ec 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -542,3 +542,24 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
vfio_display_edid_exit(vdev->dpy);
g_free(vdev->dpy);
}
+
+static bool migrate_needed(void *opaque)
+{
+VFIODisplay *dpy = opaque;
+bool ramfb_exists = dpy->ramfb != NULL;
+
+/* see vfio_display_migration_needed() */
+assert(ramfb_exists);
+return ramfb_exists;
+}
+
+const VMStateDescription vfio_display_vmstate = {
+.name = "VFIODisplay",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = migrate_needed,
+.fields = (VMStateField[]) {
+VMSTATE_STRUCT_POINTER(ramfb, VFIODisplay, ramfb_vmstate, RAMFBState),
+VMSTATE_END_OF_LIST(),
+}
+};
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 3b2ca3c24c..e44ed21180 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2608,6 +2608,32 @@ static bool vfio_msix_present(void *opaque, int
version_id)
return msix_present(pdev);
}
+static bool vfio_display_migration_needed(void *opaque)
+{
+VFIOPCIDevice *vdev = opaque;
+
+/*
+ * We need to migrate the VFIODisplay object if ramfb *migration* was
+ * explicitly requested (in which case we enforced both ramfb=on and
+ * display=on), or ramfb migration was left at the default "auto"
+ * setting, and *ramfb* was explicitly requested (in which case we
+ * enforced display=on).
+ */
+return vdev->ramfb_migrate == ON_OFF_AUTO_ON ||
+(vdev->ramfb_migrate == ON_OFF_AUTO_AUTO && vdev->enable_ramfb);
+}
+
+const VMStateDescription vmstate_vfio_display = {
+.name = "VFIOPCIDevice/VFIODisplay",
+.version_id = 1,
+.minimum_version_id = 1,
+.needed = vfio_display_migration_needed,
+.fields = (VMStateField[]){
+VMSTATE_STRUCT_POINTER(dpy, VFIOPCIDevice, vfio_display_vmstate,
VFIODisplay),
+VMSTATE_END_OF_LIST()
+}
+};
+
const VMStateDescription vmstate_vfio_pci_config = {
.name = "VFIOPCIDevice",
.version_id = 1,
@@ -2616,6 +2642,10 @@ const VMStateDescription vmstate_vfio_pci_config = {
VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
VMSTATE_END_OF_LIST()
+},
+.subsections = (const VMStateDescription*[]) {
+&vmstate_vfio_display,
+NULL
}
};
@@ -3271,6 +3301,19 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
}
}
+if (vdev->ramfb_migrate == ON_OFF_AUTO_ON && !vdev->enable_ramfb) {
+warn_report("x-ramfb-migrate=on but ramfb=off");
+vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
+}
+if (vbasedev->enable_migration == ON_OFF_AUTO_OFF) {
+if (vdev->ramfb_migrate == ON_OFF_AUTO_AUTO) {
+vdev->ramfb_migrate = ON_OFF_AUTO_OFF;
+} else if (vdev->ramfb_migrate == ON_OFF_AUTO_ON) {
+error_setg(errp, "x-ramfb-migrate requires enable-migration");
+goto out_deregister;
+}
+}
+
if (!pdev->failover_pair_id) {
if (!vfio_migration_realize(vbasedev, errp)) {
goto out_deregister;
@@ -3484,6 +3527,7 @@ static const TypeInfo vfio_pci_dev_info = {
static Property vfio_pci_dev_nohotplug_properties[] = {
DEFINE_PROP_BOOL("ramfb", VFIOPCIDevice, enable_ramfb, false),
+DEFINE_PROP_ON_OFF_AUTO("x-ramfb-migrate", VFIOPCIDevice, ramfb_migrate,
[PULL 09/25] tcg: Correct invalid mentions of 'softmmu' by 'system-mode'
From: Philippe Mathieu-Daudé
Signed-off-by: Philippe Mathieu-Daudé
Message-ID: <20231004090629.37473-6-phi...@linaro.org>
Signed-off-by: Paolo Bonzini
---
accel/tcg/user-exec.c | 2 +-
docs/devel/testing.rst | 2 +-
include/qemu/atomic128.h| 4 ++--
include/tcg/tcg-op-common.h | 2 +-
target/riscv/vector_helper.c| 2 +-
tcg/aarch64/tcg-target.c.inc| 4 ++--
tcg/arm/tcg-target.c.inc| 2 +-
tcg/i386/tcg-target.c.inc | 2 +-
tcg/loongarch64/tcg-target.c.inc| 4 ++--
tcg/meson.build | 6 +++---
tcg/mips/tcg-target.c.inc | 4 ++--
tcg/ppc/tcg-target.c.inc| 4 ++--
tcg/region.c| 4 ++--
tcg/riscv/tcg-target.c.inc | 4 ++--
tcg/s390x/tcg-target.c.inc | 4 ++--
tcg/sparc64/tcg-target.c.inc| 4 ++--
tcg/tcg.c | 11 ++-
tests/tcg/Makefile.target | 2 +-
tests/tcg/multiarch/gdbstub/interrupt.py| 2 +-
tests/tcg/multiarch/gdbstub/memory.py | 2 +-
tests/tcg/multiarch/system/memory.c | 4 ++--
tests/tcg/s390x/pgm-specification-softmmu.S | 2 +-
tests/tcg/s390x/pgm-specification.mak | 2 +-
tests/tcg/s390x/softmmu.ld | 2 +-
tests/tcg/xtensa/Makefile.softmmu-target| 2 +-
tests/tcg/xtensaeb/Makefile.softmmu-target | 2 +-
26 files changed, 43 insertions(+), 42 deletions(-)
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 5bf2761bf48..68b252cb8e8 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -940,7 +940,7 @@ void *page_get_target_data(target_ulong address)
void page_reset_target_data(target_ulong start, target_ulong last) { }
#endif /* TARGET_PAGE_DATA_SIZE */
-/* The softmmu versions of these helpers are in cputlb.c. */
+/* The system-mode versions of these helpers are in cputlb.c. */
static void *cpu_mmu_lookup(CPUState *cpu, vaddr addr,
MemOp mop, uintptr_t ra, MMUAccessType type)
diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 5d1fc0aa95f..f3e24721890 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -1458,7 +1458,7 @@ TCG test dependencies
~
The TCG tests are deliberately very light on dependencies and are
-either totally bare with minimal gcc lib support (for softmmu tests)
+either totally bare with minimal gcc lib support (for system-mode tests)
or just glibc (for linux-user tests). This is because getting a cross
compiler to work with additional libraries can be challenging.
diff --git a/include/qemu/atomic128.h b/include/qemu/atomic128.h
index 34554bf0acc..88af6d4ea3f 100644
--- a/include/qemu/atomic128.h
+++ b/include/qemu/atomic128.h
@@ -43,8 +43,8 @@
* See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80878
*
* This interpretation is not especially helpful for QEMU.
- * For softmmu, all RAM is always read/write from the hypervisor.
- * For user-only, if the guest doesn't implement such an __atomic_read
+ * For system-mode, all RAM is always read/write from the hypervisor.
+ * For user-mode, if the guest doesn't implement such an __atomic_read
* then the host need not worry about it either.
*
* Moreover, using libatomic is not an option, because its interface is
diff --git a/include/tcg/tcg-op-common.h b/include/tcg/tcg-op-common.h
index a53b15933bb..2048f92b5e1 100644
--- a/include/tcg/tcg-op-common.h
+++ b/include/tcg/tcg-op-common.h
@@ -265,7 +265,7 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned
idx);
*
* See tcg/README for more info about this TCG operation.
*
- * NOTE: In softmmu emulation, direct jumps with goto_tb are only safe within
+ * NOTE: In system emulation, direct jumps with goto_tb are only safe within
* the pages this TB resides in because we don't take care of direct jumps when
* address mapping changes, e.g. in tlb_flush(). In user mode, there's only a
* static address translation, so the destination address is always valid, TBs
diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index cba02c13203..c9b39fb67f4 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -100,7 +100,7 @@ static inline target_ulong adjust_addr(CPURISCVState *env,
target_ulong addr)
/*
* This function checks watchpoint before real load operation.
*
- * In softmmu mode, the TLB API probe_access is enough for watchpoint check.
+ * In system mode, the TLB API probe_access is enough for watchpoint check.
* In user mode, there is no watchpoint support now.
*
* It will trigger an exception if there is no mapping in TLB
diff --git a/tcg/aarch64/tcg-target.c.inc b/tcg/aarch64/tcg-target.c.inc
index 69f2daf2c2b..3afb896a3a5 100644
--- a/tcg/aarch64/tcg-targe
[PULL 16/25] system: Rename softmmu/ directory as system/
From: Philippe Mathieu-Daudé
The softmmu/ directory contains files specific to system
emulation. Rename it as system/. Update meson rules, the
MAINTAINERS file and all the documentation and comments.
Signed-off-by: Philippe Mathieu-Daudé
Message-ID: <20231004090629.37473-14-phi...@linaro.org>
Signed-off-by: Paolo Bonzini
---
MAINTAINERS | 42 +--
accel/tcg/icount-common.c | 2 +-
docs/devel/qtest.rst | 2 +-
.../sysemu/cpu-timers-internal.h | 0
include/sysemu/runstate-action.h | 2 +-
meson.build | 8 ++--
scripts/checkpatch.pl | 2 +-
scripts/coverity-scan/COMPONENTS.md | 2 +-
scripts/get_maintainer.pl | 2 +-
scripts/oss-fuzz/build.sh | 6 +--
softmmu/trace.h | 1 -
{softmmu => system}/arch_init.c | 0
{softmmu => system}/async-teardown.c | 0
{softmmu => system}/balloon.c | 0
{softmmu => system}/bootdevice.c | 0
{softmmu => system}/cpu-throttle.c| 0
{softmmu => system}/cpu-timers.c | 2 +-
{softmmu => system}/cpus.c| 0
{softmmu => system}/datadir.c | 0
{softmmu => system}/device_tree.c | 0
{softmmu => system}/dirtylimit.c | 0
{softmmu => system}/dma-helpers.c | 0
{softmmu => system}/globals.c | 0
{softmmu => system}/ioport.c | 0
{softmmu => system}/main.c| 0
{softmmu => system}/memory.c | 0
{softmmu => system}/memory_mapping.c | 0
{softmmu => system}/meson.build | 0
{softmmu => system}/physmem.c | 6 ++-
{softmmu => system}/qdev-monitor.c| 0
{softmmu => system}/qemu-seccomp.c| 0
{softmmu => system}/qtest.c | 0
{softmmu => system}/rtc.c | 0
{softmmu => system}/runstate-action.c | 0
{softmmu => system}/runstate-hmp-cmds.c | 0
{softmmu => system}/runstate.c| 0
{softmmu => system}/tpm-hmp-cmds.c| 0
{softmmu => system}/tpm.c | 0
{softmmu => system}/trace-events | 0
system/trace.h| 1 +
{softmmu => system}/vl.c | 0
{softmmu => system}/watchpoint.c | 0
tests/unit/meson.build| 2 +-
43 files changed, 41 insertions(+), 39 deletions(-)
rename softmmu/timers-state.h => include/sysemu/cpu-timers-internal.h (100%)
delete mode 100644 softmmu/trace.h
rename {softmmu => system}/arch_init.c (100%)
rename {softmmu => system}/async-teardown.c (100%)
rename {softmmu => system}/balloon.c (100%)
rename {softmmu => system}/bootdevice.c (100%)
rename {softmmu => system}/cpu-throttle.c (100%)
rename {softmmu => system}/cpu-timers.c (99%)
rename {softmmu => system}/cpus.c (100%)
rename {softmmu => system}/datadir.c (100%)
rename {softmmu => system}/device_tree.c (100%)
rename {softmmu => system}/dirtylimit.c (100%)
rename {softmmu => system}/dma-helpers.c (100%)
rename {softmmu => system}/globals.c (100%)
rename {softmmu => system}/ioport.c (100%)
rename {softmmu => system}/main.c (100%)
rename {softmmu => system}/memory.c (100%)
rename {softmmu => system}/memory_mapping.c (100%)
rename {softmmu => system}/meson.build (100%)
rename {softmmu => system}/physmem.c (99%)
rename {softmmu => system}/qdev-monitor.c (100%)
rename {softmmu => system}/qemu-seccomp.c (100%)
rename {softmmu => system}/qtest.c (100%)
rename {softmmu => system}/rtc.c (100%)
rename {softmmu => system}/runstate-action.c (100%)
rename {softmmu => system}/runstate-hmp-cmds.c (100%)
rename {softmmu => system}/runstate.c (100%)
rename {softmmu => system}/tpm-hmp-cmds.c (100%)
rename {softmmu => system}/tpm.c (100%)
rename {softmmu => system}/trace-events (100%)
create mode 100644 system/trace.h
rename {softmmu => system}/vl.c (100%)
rename {softmmu => system}/watchpoint.c (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index ea91f9e8048..a5ce4c08562 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -137,8 +137,8 @@ Overall TCG CPUs
M: Richard Henderson
R: Paolo Bonzini
S: Maintained
-F: softmmu/cpus.c
-F: softmmu/watchpoint.c
+F: system/cpus.c
+F: system/watchpoint.c
F: cpu-common.c
F: cpu-target.c
F: page-vary-target.c
@@ -2108,7 +2108,7 @@ S: Maintained
F: docs/interop/virtio-balloon-stats.rst
F: hw/virtio/virtio-balloon*.c
F: include/hw/virtio/virtio-balloon.h
-F: softmmu/balloon.c
+F: system/balloon.c
F: include/sysemu/balloon.h
virtio-9p
@@ -2795,7 +2795,7 @@ Device Tree
M: Alistair Francis
R: David Gibson
S: Maintained
-F: softmmu/device_tree.c
+F: system/device_tree.c
F: include/sysemu/device_tree.h
Du
[PULL v2 00/25] Audio, source reorg, HVF changes for 2023-10-06
The following changes since commit 2f3913f4b2ad74baeb5a6f1d36efbd9ecdf1057d:
Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu
into staging (2023-10-05 09:01:01 -0400)
are available in the Git repository at:
https://gitlab.com/bonzini/qemu.git tags/for-upstream
for you to fetch changes up to 912eef205ae9ccfd477c343a51a7c2dcfae2ba43:
audio, qtest: get rid of QEMU_AUDIO_DRV (2023-10-08 21:08:27 +0200)
* util/log: re-allow switching away from stderr log file
* finish audio configuration rework
* cleanup HVF stubs
* remove more mentions of softmmu
v1->v2: fixed conflicts, moved system/timers-state.h to
include/sysemu/cpu-timers-internal.h
Fiona Ebner (1):
util/log: re-allow switching away from stderr log file
Paolo Bonzini (9):
configure: change $softmmu to $system
cutils: squelch compiler warnings with custom paths
audio: error hints need a trailing \n
audio: disable default backends if -audio/-audiodev is used
audio: extract audio_define_default
audio: extend -audio to allow creating a default backend
audio: do not use first -audiodev as default audio device
audio: reintroduce default audio backend for VNC
audio, qtest: get rid of QEMU_AUDIO_DRV
Philippe Mathieu-Daudé (15):
target/i386/hvf: Remove unused includes in 'hvf-i386.h'
sysemu/kvm: Restrict hvf_get_supported_cpuid() to x86 targets
target/i386: Check for USER_ONLY definition instead of SOFTMMU one
softmmu/trace-events: Fix a typo
travis-ci: Correct invalid mentions of 'softmmu' by 'system'
cpu: Correct invalid mentions of 'softmmu' by 'system-mode'
fuzz: Correct invalid mentions of 'softmmu' by 'system'
tcg: Correct invalid mentions of 'softmmu' by 'system-mode'
accel: Rename accel_softmmu* -> accel_system*
gdbstub: Rename 'softmmu' -> 'system'
semihosting: Rename softmmu_FOO_user() -> uaccess_FOO_user()
target/i386: Rename i386_softmmu_kvm_ss -> i386_kvm_ss
meson: Rename softmmu_mods -> system_mods
meson: Rename target_softmmu_arch -> target_system_arch
system: Rename softmmu/ directory as system/
.travis.yml| 4 +-
MAINTAINERS| 42 +--
accel/{accel-softmmu.c => accel-system.c} | 6 +-
accel/{accel-softmmu.h => accel-system.h} | 6 +-
accel/accel-target.c | 2 +-
accel/meson.build | 2 +-
accel/stubs/meson.build| 10 +--
accel/tcg/icount-common.c | 2 +-
accel/tcg/user-exec.c | 2 +-
audio/audio.c | 85 +-
audio/audio.h | 3 +
configure | 10 +--
cpu-target.c | 2 +-
docs/about/deprecated.rst | 6 --
docs/about/removed-features.rst| 20 -
docs/devel/build-system.rst| 4 +-
docs/devel/qtest.rst | 2 +-
docs/devel/testing.rst | 2 +-
gdbstub/internals.h| 4 +-
gdbstub/meson.build| 10 +--
gdbstub/{softmmu.c => system.c}| 2 +-
gdbstub/trace-events | 2 +-
hw/core/cpu-common.c | 4 +-
include/qemu/atomic128.h | 4 +-
.../semihosting/{softmmu-uaccess.h => uaccess.h} | 24 +++---
.../sysemu/cpu-timers-internal.h | 0
include/sysemu/hvf.h | 3 -
include/sysemu/runstate-action.h | 2 +-
include/tcg/tcg-op-common.h| 2 +-
meson.build| 22 +++---
qemu-options.hx| 29 ++--
scripts/checkpatch.pl | 2 +-
scripts/coverity-scan/COMPONENTS.md| 2 +-
scripts/get_maintainer.pl | 2 +-
scripts/oss-fuzz/build.sh | 6 +-
semihosting/arm-compat-semi.c | 4 +-
semihosting/config.c | 2 +-
semihosting/guestfd.c | 2 +-
semihosting/syscalls.c | 2 +-
semihosting/uaccess.c | 14 ++--
softmmu/trace.h| 1 -
stubs/semihost.c | 4 +-
{softmmu => system}/arch_init.c| 0
{softmmu => system}/async-teardown.c |
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
On 9/10/23 08:09, Philippe Mathieu-Daudé wrote: Hi Brian, On 6/10/23 00:22, Brian Cain wrote: The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the identifiers to avoid shadowing the type name. This one surprises me, since we have other occurences: include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr, include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr, target/arm/internals.h:643:G_NORETURN void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, target/i386/tcg/helper-tcg.h:70:G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr, ... $ git grep -w vaddr, | wc -l 207 What is the error/warning like? OK I could reproduce, I suppose you are building with Clang which doesn't support shadow-local so you get global warnings too (as mentioned in this patch subject...): In file included from ../../gdbstub/trace.h:1, from ../../gdbstub/softmmu.c:30: trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint': trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow] 903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr) | ~^ In file included from include/sysemu/accel-ops.h:13, from include/sysemu/cpus.h:4, from ../../gdbstub/softmmu.c:21: include/exec/cpu-common.h:21:18: note: shadowed declaration is here 21 | typedef uint64_t vaddr; | ^ trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint': trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow] 923 | static inline void trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr) | ~^ include/exec/cpu-common.h:21:18: note: shadowed declaration is here 21 | typedef uint64_t vaddr; | ^ Clang users got confused by this, IIUC Markus and Thomas idea is to only enable these warnings for GCC, enforcing them for Clang users via CI (until Clang get this option supported). Personally I'd rather enable the warning once for all, waiting for Clang support (or clean/enable global shadowing for GCC too). See this thread: https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-b2b58d351...@redhat.com/ Regards, Phil.
Re: [PULL 00/21] vfio queue
On 10/6/23 12:33, Cédric Le Goater wrote: On 10/6/23 08:19, Cédric Le Goater wrote: The following changes since commit 2f3913f4b2ad74baeb5a6f1d36efbd9ecdf1057d: Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging (2023-10-05 09:01:01 -0400) are available in the Git repository at: https://github.com/legoater/qemu/ tags/pull-vfio-20231006 for you to fetch changes up to 6e86aaef9ac57066aa923211a164df95b7b3cdf7: vfio/common: Move legacy VFIO backend code into separate container.c (2023-10-05 22:04:52 +0200) vfio queue: * Fix for VFIO display when using Intel vGPUs * Support for dynamic MSI-X * Preliminary work for IOMMUFD support Stefan, I just did some tests on z with passthough devices (PCI and AP) and the series is not bisectable. QEMU crashes at patch : "vfio/pci: Introduce vfio_[attach/detach]_device". Also, with everything applied, the guest fails to start with : vfio: IRQ 0 not available (number of irqs 0) So, please hold on and sorry for the noise. I will start digging on my side. We found a couple of issues after further testing. Fixes are under way. Let's drop this PR. I will send a reduced one today. Thanks, C.
[PULL v2 0/6] vfio queue
The following changes since commit 2f3913f4b2ad74baeb5a6f1d36efbd9ecdf1057d: Merge tag 'for_upstream' of https://git.kernel.org/pub/scm/virt/kvm/mst/qemu into staging (2023-10-05 09:01:01 -0400) are available in the Git repository at: https://github.com/legoater/qemu/ tags/pull-vfio-20231009 for you to fetch changes up to eaadba6f9b14823e52ee154d0052d69907deee8a: vfio/pci: enable MSI-X in interrupt restoring on dynamic allocation (2023-10-05 22:04:51 +0200) vfio queue: * Fix for VFIO display when using Intel vGPUs * Support for dynamic MSI-X Alex Williamson (1): vfio/display: Fix missing update to set backing fields Jing Liu (4): vfio/pci: detect the support of dynamic MSI-X allocation vfio/pci: enable vector on dynamic MSI-X allocation vfio/pci: use an invalid fd to enable MSI-X vfio/pci: enable MSI-X in interrupt restoring on dynamic allocation Zhenzhong Duan (1): vfio/pci: rename vfio_put_device to vfio_pci_put_device hw/vfio/pci.h| 1 + hw/vfio/display.c| 2 + hw/vfio/pci.c| 127 +++ hw/vfio/trace-events | 2 +- 4 files changed, 101 insertions(+), 31 deletions(-)
[PULL 1/6] vfio/display: Fix missing update to set backing fields
From: Alex Williamson
The below referenced commit renames scanout_width/height to
backing_width/height, but also promotes these fields in various portions
of the egl interface. Meanwhile vfio dmabuf support has never used the
previous scanout fields and is therefore missed in the update. This
results in a black screen when transitioning from ramfb to dmabuf display
when using Intel vGPU with these features.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1891
Link: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02726.html
Fixes: 9ac06df8b684 ("virtio-gpu-udmabuf: correct naming of QemuDmaBuf size
properties")
Signed-off-by: Alex Williamson
Tested-by: Cédric Le Goater
Signed-off-by: Cédric Le Goater
---
hw/vfio/display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index
bec864f482f46866bf292a5a63c31753a7c84eef..837d9e6a309e834601c125e36faadf81c1c5172e
100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -243,6 +243,8 @@ static VFIODMABuf *vfio_display_get_dmabuf(VFIOPCIDevice
*vdev,
dmabuf->dmabuf_id = plane.dmabuf_id;
dmabuf->buf.width = plane.width;
dmabuf->buf.height = plane.height;
+dmabuf->buf.backing_width = plane.width;
+dmabuf->buf.backing_height = plane.height;
dmabuf->buf.stride = plane.stride;
dmabuf->buf.fourcc = plane.drm_format;
dmabuf->buf.modifier = plane.drm_format_mod;
--
2.41.0
[PULL 6/6] vfio/pci: enable MSI-X in interrupt restoring on dynamic allocation
From: Jing Liu
During migration restoring, vfio_enable_vectors() is called to restore
enabling MSI-X interrupts for assigned devices. It sets the range from
0 to nr_vectors to kernel to enable MSI-X and the vectors unmasked in
guest. During the MSI-X enabling, all the vectors within the range are
allocated according to the VFIO_DEVICE_SET_IRQS ioctl.
When dynamic MSI-X allocation is supported, we only want the guest
unmasked vectors being allocated and enabled. Use vector 0 with an
invalid fd to get MSI-X enabled, after that, all the vectors can be
allocated in need.
Signed-off-by: Jing Liu
Reviewed-by: Cédric Le Goater
Reviewed-by: Alex Williamson
Signed-off-by: Cédric Le Goater
---
hw/vfio/pci.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
ad508abd6f382d518871191017859c4c4c8fd4f9..898296fd5441b07fded7e50a53b2927683dd4b1a
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -402,6 +402,23 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool
msix)
int ret = 0, i, argsz;
int32_t *fds;
+/*
+ * If dynamic MSI-X allocation is supported, the vectors to be allocated
+ * and enabled can be scattered. Before kernel enabling MSI-X, setting
+ * nr_vectors causes all these vectors to be allocated on host.
+ *
+ * To keep allocation as needed, use vector 0 with an invalid fd to get
+ * MSI-X enabled first, then set vectors with a potentially sparse set of
+ * eventfds to enable interrupts only when enabled in guest.
+ */
+if (msix && !vdev->msix->noresize) {
+ret = vfio_enable_msix_no_vec(vdev);
+
+if (ret) {
+return ret;
+}
+}
+
argsz = sizeof(*irq_set) + (vdev->nr_vectors * sizeof(*fds));
irq_set = g_malloc0(argsz);
--
2.41.0
[PULL 5/6] vfio/pci: use an invalid fd to enable MSI-X
From: Jing Liu
Guests typically enable MSI-X with all of the vectors masked in the MSI-X
vector table. To match the guest state of device, QEMU enables MSI-X by
enabling vector 0 with userspace triggering and immediately release.
However the release function actually does not release it due to already
using userspace mode.
It is no need to enable triggering on host and rely on the mask bit to
avoid spurious interrupts. Use an invalid fd (i.e. fd = -1) is enough
to get MSI-X enabled.
After dynamic MSI-X allocation is supported, the interrupt restoring
also need use such way to enable MSI-X, therefore, create a function
for that.
Suggested-by: Alex Williamson
Signed-off-by: Jing Liu
Reviewed-by: Cédric Le Goater
Reviewed-by: Alex Williamson
Signed-off-by: Cédric Le Goater
---
hw/vfio/pci.c | 44
1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
eb7627c5f9fcd67f346595ebe73011cc3205b58e..ad508abd6f382d518871191017859c4c4c8fd4f9
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -369,6 +369,33 @@ static void vfio_msi_interrupt(void *opaque)
notify(&vdev->pdev, nr);
}
+/*
+ * Get MSI-X enabled, but no vector enabled, by setting vector 0 with an
invalid
+ * fd to kernel.
+ */
+static int vfio_enable_msix_no_vec(VFIOPCIDevice *vdev)
+{
+g_autofree struct vfio_irq_set *irq_set = NULL;
+int ret = 0, argsz;
+int32_t *fd;
+
+argsz = sizeof(*irq_set) + sizeof(*fd);
+
+irq_set = g_malloc0(argsz);
+irq_set->argsz = argsz;
+irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+ VFIO_IRQ_SET_ACTION_TRIGGER;
+irq_set->index = VFIO_PCI_MSIX_IRQ_INDEX;
+irq_set->start = 0;
+irq_set->count = 1;
+fd = (int32_t *)&irq_set->data;
+*fd = -1;
+
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+
+return ret;
+}
+
static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix)
{
struct vfio_irq_set *irq_set;
@@ -618,6 +645,8 @@ static void vfio_commit_kvm_msi_virq_batch(VFIOPCIDevice
*vdev)
static void vfio_msix_enable(VFIOPCIDevice *vdev)
{
+int ret;
+
vfio_disable_interrupts(vdev);
vdev->msi_vectors = g_new0(VFIOMSIVector, vdev->msix->entries);
@@ -640,8 +669,6 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
vfio_commit_kvm_msi_virq_batch(vdev);
if (vdev->nr_vectors) {
-int ret;
-
ret = vfio_enable_vectors(vdev, true);
if (ret) {
error_report("vfio: failed to enable vectors, %d", ret);
@@ -655,13 +682,14 @@ static void vfio_msix_enable(VFIOPCIDevice *vdev)
* MSI-X capability, but leaves the vector table masked. We therefore
* can't rely on a vector_use callback (from request_irq() in the
guest)
* to switch the physical device into MSI-X mode because that may come
a
- * long time after pci_enable_msix(). This code enables vector 0 with
- * triggering to userspace, then immediately release the vector,
leaving
- * the physical device with no vectors enabled, but MSI-X enabled, just
- * like the guest view.
+ * long time after pci_enable_msix(). This code sets vector 0 with an
+ * invalid fd to make the physical device MSI-X enabled, but with no
+ * vectors enabled, just like the guest view.
*/
-vfio_msix_vector_do_use(&vdev->pdev, 0, NULL, NULL);
-vfio_msix_vector_release(&vdev->pdev, 0);
+ret = vfio_enable_msix_no_vec(vdev);
+if (ret) {
+error_report("vfio: failed to enable MSI-X, %d", ret);
+}
}
trace_vfio_msix_enable(vdev->vbasedev.name);
--
2.41.0
[PULL 4/6] vfio/pci: enable vector on dynamic MSI-X allocation
From: Jing Liu
The vector_use callback is used to enable vector that is unmasked in
guest. The kernel used to only support static MSI-X allocation. When
allocating a new interrupt using "static MSI-X allocation" kernels,
QEMU first disables all previously allocated vectors and then
re-allocates all including the new one. The nr_vectors of VFIOPCIDevice
indicates that all vectors from 0 to nr_vectors are allocated (and may
be enabled), which is used to loop all the possibly used vectors when
e.g., disabling MSI-X interrupts.
Extend the vector_use function to support dynamic MSI-X allocation when
host supports the capability. QEMU therefore can individually allocate
and enable a new interrupt without affecting others or causing interrupts
lost during runtime.
Utilize nr_vectors to calculate the upper bound of enabled vectors in
dynamic MSI-X allocation mode since looping all msix_entries_nr is not
efficient and unnecessary.
Signed-off-by: Jing Liu
Tested-by: Reinette Chatre
Reviewed-by: Alex Williamson
Signed-off-by: Cédric Le Goater
---
hw/vfio/pci.c | 46 --
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
86c92c51a46e2d1e9856c3cbcd7c47548daab422..eb7627c5f9fcd67f346595ebe73011cc3205b58e
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -470,6 +470,7 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
unsigned int nr,
VFIOPCIDevice *vdev = VFIO_PCI(pdev);
VFIOMSIVector *vector;
int ret;
+bool resizing = !!(vdev->nr_vectors < nr + 1);
trace_vfio_msix_vector_do_use(vdev->vbasedev.name, nr);
@@ -512,33 +513,42 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev,
unsigned int nr,
}
/*
- * We don't want to have the host allocate all possible MSI vectors
- * for a device if they're not in use, so we shutdown and incrementally
- * increase them as needed.
+ * When dynamic allocation is not supported, we don't want to have the
+ * host allocate all possible MSI vectors for a device if they're not
+ * in use, so we shutdown and incrementally increase them as needed.
+ * nr_vectors represents the total number of vectors allocated.
+ *
+ * When dynamic allocation is supported, let the host only allocate
+ * and enable a vector when it is in use in guest. nr_vectors represents
+ * the upper bound of vectors being enabled (but not all of the ranges
+ * is allocated or enabled).
*/
-if (vdev->nr_vectors < nr + 1) {
+if (resizing) {
vdev->nr_vectors = nr + 1;
-if (!vdev->defer_kvm_irq_routing) {
+}
+
+if (!vdev->defer_kvm_irq_routing) {
+if (vdev->msix->noresize && resizing) {
vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
ret = vfio_enable_vectors(vdev, true);
if (ret) {
error_report("vfio: failed to enable vectors, %d", ret);
}
-}
-} else {
-Error *err = NULL;
-int32_t fd;
-
-if (vector->virq >= 0) {
-fd = event_notifier_get_fd(&vector->kvm_interrupt);
} else {
-fd = event_notifier_get_fd(&vector->interrupt);
-}
+Error *err = NULL;
+int32_t fd;
-if (vfio_set_irq_signaling(&vdev->vbasedev,
- VFIO_PCI_MSIX_IRQ_INDEX, nr,
- VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
-error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+if (vector->virq >= 0) {
+fd = event_notifier_get_fd(&vector->kvm_interrupt);
+} else {
+fd = event_notifier_get_fd(&vector->interrupt);
+}
+
+if (vfio_set_irq_signaling(&vdev->vbasedev,
+ VFIO_PCI_MSIX_IRQ_INDEX, nr,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err))
{
+error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+}
}
}
--
2.41.0
[PULL 3/6] vfio/pci: detect the support of dynamic MSI-X allocation
From: Jing Liu
Kernel provides the guidance of dynamic MSI-X allocation support of
passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
guide user space.
Fetch the flags from host to determine if dynamic MSI-X allocation is
supported.
Originally-by: Reinette Chatre
Signed-off-by: Jing Liu
Reviewed-by: Cédric Le Goater
Reviewed-by: Alex Williamson
Signed-off-by: Cédric Le Goater
---
hw/vfio/pci.h| 1 +
hw/vfio/pci.c| 16 ++--
hw/vfio/trace-events | 2 +-
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index
2d836093a83d3df5f94dc8981ee6c61cfb6d3373..0d89eb761ece426ca1df40f63461b2bb1bcf1ab0
100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
uint32_t table_offset;
uint32_t pba_offset;
unsigned long *pending;
+bool noresize;
} VFIOMSIXInfo;
#define TYPE_VFIO_PCI "vfio-pci"
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
b2d5010b9f0effbe85a3f57b43e282e837b6383f..86c92c51a46e2d1e9856c3cbcd7c47548daab422
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1493,7 +1493,9 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev,
Error **errp)
uint8_t pos;
uint16_t ctrl;
uint32_t table, pba;
-int fd = vdev->vbasedev.fd;
+int ret, fd = vdev->vbasedev.fd;
+struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
+ .index = VFIO_PCI_MSIX_IRQ_INDEX };
VFIOMSIXInfo *msix;
pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX);
@@ -1530,6 +1532,15 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev,
Error **errp)
msix->pba_offset = pba & ~PCI_MSIX_FLAGS_BIRMASK;
msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
+ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+if (ret < 0) {
+error_setg_errno(errp, -ret, "failed to get MSI-X irq info");
+g_free(msix);
+return;
+}
+
+msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
+
/*
* Test the size of the pba_offset variable and catch if it extends outside
* of the specified BAR. If it is the case, we need to apply a hardware
@@ -1562,7 +1573,8 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev,
Error **errp)
}
trace_vfio_msix_early_setup(vdev->vbasedev.name, pos, msix->table_bar,
-msix->table_offset, msix->entries);
+msix->table_offset, msix->entries,
+msix->noresize);
vdev->msix = msix;
vfio_pci_fixup_msix_region(vdev);
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index
e64ca4a01961b34dc4b9b7d78917f1d8847d887d..0ba3c5a0e26bc82f40a0d7737e68128d2c9334fe
100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -27,7 +27,7 @@ vfio_vga_read(uint64_t addr, int size, uint64_t data) "
(0x%"PRIx64", %d) = 0x%"
vfio_pci_read_config(const char *name, int addr, int len, int val) " (%s,
@0x%x, len=0x%x) 0x%x"
vfio_pci_write_config(const char *name, int addr, int val, int len) " (%s,
@0x%x, 0x%x, len=0x%x)"
vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
-vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset,
int entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
+vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset,
int entries, bool noresize) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x,
entries %d, noresize %d"
vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
vfio_check_pm_reset(const char *name) "%s Supports PM reset"
vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"
--
2.41.0
[PULL 2/6] vfio/pci: rename vfio_put_device to vfio_pci_put_device
From: Zhenzhong Duan
vfio_put_device() is a VFIO PCI specific function, rename it with
'vfio_pci' prefix to avoid confusing.
No functional change.
Suggested-by: Cédric Le Goater
Signed-off-by: Zhenzhong Duan
Signed-off-by: Cédric Le Goater
---
hw/vfio/pci.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index
3b2ca3c24ca2ed91c03b5c6bef65bb5a0fb0298a..b2d5010b9f0effbe85a3f57b43e282e837b6383f
100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2826,7 +2826,7 @@ static void vfio_populate_device(VFIOPCIDevice *vdev,
Error **errp)
}
}
-static void vfio_put_device(VFIOPCIDevice *vdev)
+static void vfio_pci_put_device(VFIOPCIDevice *vdev)
{
g_free(vdev->vbasedev.name);
g_free(vdev->msix);
@@ -3317,7 +3317,7 @@ static void vfio_instance_finalize(Object *obj)
*
* g_free(vdev->igd_opregion);
*/
-vfio_put_device(vdev);
+vfio_pci_put_device(vdev);
vfio_put_group(group);
}
--
2.41.0
RE: [PATCH 0/3] vfio: memory leak fix and code cleanup
>-Original Message- >From: Cédric Le Goater >Sent: Monday, October 9, 2023 2:32 PM >Subject: Re: [PATCH 0/3] vfio: memory leak fix and code cleanup > >On 10/9/23 04:20, Zhenzhong Duan wrote: >> Hi, >> >> This trivial patchset fixes a incremental memory leak in rare case, >> and some cleanup on ap/ccw. >> >> This patchset is based on vfio-next. > >Will this apply on the future v5 of Eric ? I think so, though this patchset is applied on v4 of Eric. Let me know if you want a new version without basing on the prerequisite patchset, it’s easy to rebase. Thanks Zhenzhong
