Re: [PATCH 2/2] riscv: zicond: make default

[Andrew Jones]

On Thu, Aug 10, 2023 at 02:07:17PM -0400, Alistair Francis wrote:
> On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta  wrote:
> >
> >
> >
> > On 8/8/23 14:06, Daniel Henrique Barboza wrote:
> > > (CCing Alistair and other reviewers)
> > >
> > > On 8/8/23 15:17, Vineet Gupta wrote:
> > >> Again this helps with better testing and something qemu has been doing
> > >> with newer features anyways.
> > >>
> > >> Signed-off-by: Vineet Gupta 
> > >> ---
> > >
> > > Even if we can reach a consensus about removing the experimental (x-
> > > prefix) status
> > > from an extension that is Frozen instead of ratified, enabling stuff
> > > in the default
> > > CPUs because it's easier to test is something we would like to avoid.
> > > The rv64
> > > CPU has a random set of extensions enabled for the most different and
> > > undocumented
> > > reasons, and users don't know what they'll get because we keep beefing
> > > up the
> > > generic CPUs arbitrarily.
> 
> The idea was to enable "most" extensions for the virt machine. It's a
> bit wishy-washy, but the idea was to enable as much as possible by
> default on the virt machine, as long as it doesn't conflict. The goal
> being to allow users to get the "best" experience as all their
> favourite extensions are enabled.
> 
> It's harder to do in practice, so we are in a weird state where users
> don't know what is and isn't enabled.
> 
> We probably want to revisit this. We should try to enable what is
> useful for users and make it clear what is and isn't enabled. I'm not
> clear on how best to do that though.
> 
> Again, I think this comes back to we need to version the virt machine.
> I might do that as a starting point, that allows us to make changes in
> a clear way.

While some extensions will impact the machine model, as well as cpu
models, versioning the machine model won't help much with ambiguity in
cpu model extension support. Daniel's proposal of having a base cpu mode,
which, on top, users can explicitly enable what they want (including with
profile support which work like a shorthand to enable many extensions at
once), is, IMO, the best way for users to know what they get. Also, the
'max' cpu model is the best way to "quickly get as much as possible" for
testing. To know what's in 'max', or named cpu models, we need to
implement qmp_query_cpu_model_expansion(). Something that could be used
from the command line would also be nice, but neither x86 nor arm provide
that (they have '-cpu help', but arm doesn't output anything for cpu
features and x86 dumps all features out without saying what's enabled for
any particular cpu model...)

I know x86 people have in the past discussed versioning cpu models, but
I don't think that should be necessary for riscv with the base+profile
approach. A profile would effectively be a versioned cpu model in that
case.

Finally, I'd discourage versioning the virt machine type until we need
to worry about users creating riscv guest images that they are unwilling
to modify, despite wanting to update their QEMU versions. And, even then,
we should only consider versioning when we're aware of problems for those
static guest images, i.e. we introduce a change to the virt machine model
which breaks that supported, old guest image. (NB: It was me that
advocated to start versioning Arm's virt machine type. In hindsight, I
think I may have advocated prematurely. I'm trying not to make that
mistake twice!)

> 
> >
> > I understand this position given the arbitrary nature of gazillion
> > extensions. However pragmatically things like bitmanip and zicond are so
> > fundamental it would be strange for designs to not have them, in a few
> > years. Besides these don't compete or conflict with other extensions.
> > But on face value it is indeed possible for vendors to drop them for
> > various reasons or no-reasons.
> >
> > But having the x- dropped is good enough for our needs as there's
> > already mechanisms to enable the toggles from elf attributes.
> >
> > >
> > > Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
> > > non-experimental
> > > and non-vendor extensions by default, making it easier for tooling to
> > > test new
> > > features/extensions. All tooling should consider changing their
> > > scripts to use the
> > > 'max' CPU when it's available.
> >
> > That would be great.
> 
> The max CPU helps, but I do feel that the default should allow users
> to experience as many RISC-V extensions/features as practical.
> 

If the virt machine type has a default cpu type, then why not set it to
'max'?

Thanks,
drew

[PATCH 2/3] virtio-net: added replay blocker for guest_announce

[pavel . dovgalyuk]

From: Pavel Dovgalyuk 

This patch blocks record/replay when guest_announce is enabled,
because this flag breaks loading the snapshots in deterministic
execution mode.

Signed-off-by: Pavel Dovgalyuk 
---
 hw/net/virtio-net.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 43fd34b0c8..2b6c246efb 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3580,6 +3580,10 @@ static void virtio_net_device_realize(DeviceState *dev, 
Error **errp)
 n->host_features |= (1ULL << VIRTIO_NET_F_MTU);
 }
 
+if (n->host_features & (1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE)) {
+replay_add_blocker("-device virtio-net-device,guest_announce=true");
+}
+
 if (n->net_conf.duplex_str) {
 if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
 n->net_conf.duplex = DUPLEX_HALF;
-- 
2.34.1

[PATCH 0/3] Record/replay patches

[pavel . dovgalyuk]

From: Pavel Dovgalyuk 

The set of patches include the following modifications:
 - fix for allowing record/replay with virtio-net
 - fix of the record/replay test

Pavel Dovgalyuk (3):
  replay: improve determinism of virtio-net
  virtio-net: added replay blocker for guest_announce
  tests/avocado: fix waiting for vm shutdown in replay_linux

 hw/net/virtio-net.c   | 13 +
 tests/avocado/replay_linux.py |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 3/3] tests/avocado: fix waiting for vm shutdown in replay_linux

[pavel . dovgalyuk]

From: Pavel Dovgalyuk 

This patch fixes the race condition in waiting for shutdown
of the replay linux test.

Signed-off-by: Pavel Dovgalyuk 
Suggested-by: John Snow 
---
 tests/avocado/replay_linux.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/replay_linux.py b/tests/avocado/replay_linux.py
index a76dd507fc..270ccc1eae 100644
--- a/tests/avocado/replay_linux.py
+++ b/tests/avocado/replay_linux.py
@@ -93,7 +93,7 @@ def launch_and_wait(self, record, args, shift):
 % os.path.getsize(replay_path))
 else:
 vm.event_wait('SHUTDOWN', self.timeout)
-vm.shutdown(True)
+vm.wait()
 logger.info('successfully fihished the replay')
 elapsed = time.time() - start_time
 logger.info('elapsed time %.2f sec' % elapsed)
-- 
2.34.1

[PATCH 1/3] replay: improve determinism of virtio-net

[pavel . dovgalyuk]

From: Pavel Dovgalyuk 

virtio-net device uses bottom halves for callbacks.
These callbacks should be deterministic, because they affect VM state.
This patch replaces BH invocations with corresponding replay functions,
making them deterministic in record/replay mode.

Signed-off-by: Pavel Dovgalyuk 
---
 hw/net/virtio-net.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7102ec4817..43fd34b0c8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -46,6 +46,7 @@
 #include "net_rx_pkt.h"
 #include "hw/virtio/vhost.h"
 #include "sysemu/qtest.h"
+#include "sysemu/replay.h"
 
 #define VIRTIO_NET_VM_VERSION11
 
@@ -417,7 +418,7 @@ static void virtio_net_set_status(struct VirtIODevice 
*vdev, uint8_t status)
 timer_mod(q->tx_timer,
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 
n->tx_timeout);
 } else {
-qemu_bh_schedule(q->tx_bh);
+replay_bh_schedule_event(q->tx_bh);
 }
 } else {
 if (q->tx_timer) {
@@ -2799,7 +2800,7 @@ static void virtio_net_handle_tx_bh(VirtIODevice *vdev, 
VirtQueue *vq)
 return;
 }
 virtio_queue_set_notification(vq, 0);
-qemu_bh_schedule(q->tx_bh);
+replay_bh_schedule_event(q->tx_bh);
 }
 
 static void virtio_net_tx_timer(void *opaque)
@@ -2882,7 +2883,7 @@ static void virtio_net_tx_bh(void *opaque)
 /* If we flush a full burst of packets, assume there are
  * more coming and immediately reschedule */
 if (ret >= n->tx_burst) {
-qemu_bh_schedule(q->tx_bh);
+replay_bh_schedule_event(q->tx_bh);
 q->tx_waiting = 1;
 return;
 }
@@ -2896,7 +2897,7 @@ static void virtio_net_tx_bh(void *opaque)
 return;
 } else if (ret > 0) {
 virtio_queue_set_notification(q->tx_vq, 0);
-qemu_bh_schedule(q->tx_bh);
+replay_bh_schedule_event(q->tx_bh);
 q->tx_waiting = 1;
 }
 }
-- 
2.34.1

Re: [QEMU PATCH v4 0/1] S3 support

[Chen, Jiqian]

Hi all,
Please forgive me for asking. Do you have any other comments on my latest 
version patches about virtio-gpu S3 on Xen? Looking forward to your reply.


On 2023/7/20 20:08, Jiqian Chen wrote:
> v4:
> 
> Hi all,
> Thanks for Gerd Hoffmann's advice. V4 makes below changes:
> * Use enum for freeze mode, so this can be extended with more
>   modes in the future.
> * Rename functions and paratemers with "_S3" postfix.
> And no functional changes.
> 
> latest version on kernel side:
> https://lore.kernel.org/lkml/20230720115805.8206-1-jiqian.c...@amd.com/T/#t
> 
> Best regards,
> Jiqian Chen.
> 
> 
> v3:
> link,
> https://lore.kernel.org/qemu-devel/20230719074726.1613088-1-jiqian.c...@amd.com/T/#t
> 
> Hi all,
> Thanks for Michael S. Tsirkin's advice. V3 makes below changes:
> * Remove changes in file include/standard-headers/linux/virtio_gpu.h
>   I am not supposed to edit this file and it will be imported after
>   the patches of linux kernel was merged.
> 
> 
> v2:
> link,
> https://lore.kernel.org/qemu-devel/20230630070016.841459-1-jiqian.c...@amd.com/T/#t
> 
> Hi all,
> Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for
> their advice and guidance. V2 makes below changes:
> 
> * Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
> * Add virtio_gpu_device_unrealize to destroy resources to solve
>   potential memory leak problem. This also needs hot-plug support.
> * Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
>   host can negotiate whenever freezing is supported or not.
> 
> v1:
> link,
> https://lore.kernel.org/qemu-devel/20230608025655.1674357-1-jiqian.c...@amd.com/
> 
> Hi all,
> I am working to implement virtgpu S3 function on Xen.
> 
> Currently on Xen, if we start a guest who enables virtgpu, and then
> run "echo mem > /sys/power/state" to suspend guest. And run
> "sudo xl trigger  s3resume" to resume guest. We can find that
> the guest kernel comes back, but the display doesn't. It just shown a
> black screen.
> 
> Through reading codes, I founded that when guest was during suspending,
> it called into Qemu to call virtio_gpu_gl_reset. In virtio_gpu_gl_reset,
> it destroyed all resources and reset renderer. This made the display
> gone after guest resumed.
> 
> I think we should keep resources or prevent they being destroyed when
> guest is suspending. So, I add a new status named freezing to virtgpu,
> and add a new ctrl message VIRTIO_GPU_CMD_STATUS_FREEZING to get
> notification from guest. If freezing is set to true, and then Qemu will
> realize that guest is suspending, it will not destroy resources and will
> not reset renderer. If freezing is set to false, Qemu will do its origin
> actions, and has no other impaction.
> 
> And now, display can come back and applications can continue their
> status after guest resumes.
> 
> Jiqian Chen (1):
>   virtgpu: do not destroy resources when guest suspend
> 
>  hw/display/virtio-gpu-base.c   |  3 ++
>  hw/display/virtio-gpu-gl.c | 10 ++-
>  hw/display/virtio-gpu-virgl.c  |  7 +
>  hw/display/virtio-gpu.c| 55 --
>  hw/virtio/virtio.c |  3 ++
>  include/hw/virtio/virtio-gpu.h |  6 
>  6 files changed, 81 insertions(+), 3 deletions(-)
> 

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v5 08/11] target/loongarch: Reject la64-only instructions in la32 mode

[gaosong]

Hi, Jiajie

在 2023/8/9 下午4:26, Jiajie Chen 写道:

LoongArch64-only instructions are marked with regard to the instruction
manual Table 2. LSX instructions are not marked for now for lack of
public manual.

Signed-off-by: Jiajie Chen 
---
  target/loongarch/insn_trans/trans_arith.c.inc | 30 
  .../loongarch/insn_trans/trans_atomic.c.inc   | 76 +--
  target/loongarch/insn_trans/trans_bit.c.inc   | 28 +++
  .../loongarch/insn_trans/trans_branch.c.inc   |  4 +-
  target/loongarch/insn_trans/trans_extra.c.inc | 16 ++--
  target/loongarch/insn_trans/trans_fmov.c.inc  |  4 +-
  .../loongarch/insn_trans/trans_memory.c.inc   | 68 -
  target/loongarch/insn_trans/trans_shift.c.inc | 14 ++--
  target/loongarch/translate.h  |  7 ++
  9 files changed, 127 insertions(+), 120 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_arith.c.inc 
b/target/loongarch/insn_trans/trans_arith.c.inc
index 43d6cf261d..4c21d8b037 100644
--- a/target/loongarch/insn_trans/trans_arith.c.inc
+++ b/target/loongarch/insn_trans/trans_arith.c.inc
@@ -249,9 +249,9 @@ static bool trans_addu16i_d(DisasContext *ctx, 
arg_addu16i_d *a)
  }
  
  TRANS(add_w, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_add_tl)

-TRANS(add_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
+TRANS_64(add_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
  TRANS(sub_w, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_sub_tl)
-TRANS(sub_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
+TRANS_64(sub_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
  TRANS(and, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_and_tl)
  TRANS(or, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_or_tl)
  TRANS(xor, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_xor_tl)
@@ -261,32 +261,32 @@ TRANS(orn, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, 
tcg_gen_orc_tl)
  TRANS(slt, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_slt)
  TRANS(sltu, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_sltu)
  TRANS(mul_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, tcg_gen_mul_tl)
-TRANS(mul_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_mul_tl)
+TRANS_64(mul_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_mul_tl)
  TRANS(mulh_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, gen_mulh_w)
  TRANS(mulh_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, gen_mulh_w)
-TRANS(mulh_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
-TRANS(mulh_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
-TRANS(mulw_d_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
-TRANS(mulw_d_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
+TRANS_64(mulh_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
+TRANS_64(mulh_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
+TRANS_64(mulw_d_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
+TRANS_64(mulw_d_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
  TRANS(div_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_div_w)
  TRANS(mod_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_rem_w)
  TRANS(div_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_div_du)
  TRANS(mod_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_rem_du)
-TRANS(div_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
-TRANS(mod_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
-TRANS(div_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
-TRANS(mod_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
+TRANS_64(div_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
+TRANS_64(mod_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
+TRANS_64(div_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
+TRANS_64(mod_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
  TRANS(slti, gen_rri_v, EXT_NONE, EXT_NONE, gen_slt)
  TRANS(sltui, gen_rri_v, EXT_NONE, EXT_NONE, gen_sltu)
  TRANS(addi_w, gen_rri_c, EXT_NONE, EXT_SIGN, tcg_gen_addi_tl)
-TRANS(addi_d, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_addi_tl)
+TRANS_64(addi_d, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_addi_tl)
  TRANS(alsl_w, gen_rrr_sa, EXT_NONE, EXT_SIGN, gen_alsl)
-TRANS(alsl_wu, gen_rrr_sa, EXT_NONE, EXT_ZERO, gen_alsl)
-TRANS(alsl_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_alsl)
+TRANS_64(alsl_wu, gen_rrr_sa, EXT_NONE, EXT_ZERO, gen_alsl)
+TRANS_64(alsl_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_alsl)
  TRANS(pcaddi, gen_pc, gen_pcaddi)
  TRANS(pcalau12i, gen_pc, gen_pcalau12i)
  TRANS(pcaddu12i, gen_pc, gen_pcaddu12i)
-TRANS(pcaddu18i, gen_pc, gen_pcaddu18i)
+TRANS_64(pcaddu18i, gen_pc, gen_pcaddu18i)
  TRANS(andi, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_andi_tl)
  TRANS(ori, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_ori_tl)
  TRANS(xori, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_xori_tl)
diff --git a/target/loongarch/insn_trans/trans_atomic.c.inc 
b/target/loongarch/insn_trans/trans_atomic.c.inc
index 612709f2a7..c69f31bc78 100644
--- a/target/loongarch/insn_trans/trans_atomic.c.inc
+++ b/target/loongarch/insn_trans/trans_atomic.c.inc
@@ -70,41 +70,41 @@ static

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajo...@ventanamicro.com wrote:
> > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > > > by recovering the immediate argument from the result of LUI with a
> > > > shift right by 12. However, the shift right will left-fill with the
> > > > sign. By applying a mask we recover an unsigned representation of the
> > > > 20-bit field (which includes a sign bit).
> > > > 
> > > > Example:
> > > > 0xf000 >> 12 = 0x
> > > > 0xf000 >> 12 & 0xf = 0x000f
> > > > 
> > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper immediates")
> > > > Signed-off-by: Richard Bagley 
> > > > ---
> > > >  disas/riscv.c | 9 ++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > index 4023e3fc65..690eb4a1ac 100644
> > > > --- a/disas/riscv.c
> > > > +++ b/disas/riscv.c
> > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t 
> > > > buflen, size_t tab, rv_decode *dec)
> > > >  break;
> > > >  case 'U':
> > > >  fmt++;
> > > > -snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > -append(buf, tmp, buflen);
> > > > -if (*fmt == 'o') {
> > > > +if (*fmt == 'i') {
> > > > +snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 
> > > > 0xf);
> > > 
> > > Why are we correcting LUI's output, but still outputting sign-extended
> > > values for AUIPC?
> > > 
> > > We can't assemble 'auipc a1, 0x' or 'auipc a1, -1' without getting
> > > 
> > >  Error: lui expression not in range 0..1048575
> > > 
> > > (and additionally for 0x)
> > > 
> > >  Error: value of 0000 too large for field of 4 bytes at 
> > > 
> > > 
> > > either.
> > > 
> > > (I see that the assembler's error messages state 'lui', but I was trying
> > > 'auipc'.)
> > > 
> > > I'm using as from gnu binutils 2.40.0.20230214.
> > > 
> > > (And, FWIW, I agree with Richard Henderson that these instructions should
> > > accept negative values.)
> > 
> > I'm kind of lost here, and you saying binutils rejects this syntax?  If
> > that's the case it's probably just an oversight, can you file a bug in
> > binutils land so folks can see?
> 
> Will do.
>

https://sourceware.org/bugzilla/show_bug.cgi?id=30746

Thanks,
drew

Re: [PATCH] hw/pci-host: Allow extended config space access for Designware PCIe host

[Peter Maydell]

On Thu, 10 Aug 2023 at 18:51, Michael S. Tsirkin  wrote:
>
> On Wed, Aug 09, 2023 at 10:22:50AM +, Jason Chien wrote:
> > In pcie_bus_realize(), a root bus is realized as a PCIe bus and a non-root
> > bus is realized as a PCIe bus if its parent bus is a PCIe bus. However,
> > the child bus "dw-pcie" is realized before the parent bus "pcie" which is
> > the root PCIe bus. Thus, the extended configuration space is not accessible
> > on "dw-pcie". The issue can be resolved by adding the
> > PCI_BUS_EXTENDED_CONFIG_SPACE flag to "pcie" before "dw-pcie" is realized.
> >
> > Signed-off-by: Jason Chien 
>
>
> Acked-by: Michael S. Tsirkin 
>
> I'm not planning another pull before release, hopefully
> another maintainer can pick it up? Peter?

At the moment I don't have anything intended for 8.1 either,
so whoever of us does it it would be a 1-patch pullreq...

-- PMM

Re: [PATCH v2 1/4] hw/i2c/aspeed: Fix I2CD_POOL_CTRL register bit field defination

[Cédric Le Goater]

Hello Hang,

It is good practice to send a cover letter giving a quick summary of the
changes.

On 8/11/23 07:42, Hang Yu wrote:

Fixed inconsistency between the regisiter bit field defination in headfile


Fixed inconsistency between the register bit field definition header file


and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
Register in new register mode. They share bit field
[12:8]:Transmit Data Byte Count and bit field
[29:24]:Actual Received Pool Buffer Size according to the datasheet.

Signed-off-by: Hang Yu 



The initial macros included a '+ 1'

 #define   I2CD_POOL_RX_SIZE(x) x) >> 16) & 0xff) + 1)
 #define   I2CD_POOL_TX_COUNT(x)x) >> 8) & 0xff) + 1)

which was not reported in commit 3be3d6ccf2ad ("aspeed: i2c: Migrate to
registerfields API"). I think patch 1 and 2 should be folded, could you
please respin a v3 and add a Fixes tag ?

It is too late for the 8.1 cycle but these changes would be good candidate
for QEMU stable. I will queue them for 8.2.

Also, they fix booting v08.06 SDK which is great. Here is an extra patch
to change the QEMU tests to the latest version :

  
https://github.com/legoater/qemu/commit/08243703f48f5e9c263773a846f7dc655cd64bfa

You can include it in your series if you want.

Thanks,

C.




---
  include/hw/i2c/aspeed_i2c.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 51c944efea..2e1e15aaf0 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
  REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
  SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
  REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
-SHARED_FIELD(RX_COUNT, 24, 5)
+SHARED_FIELD(RX_COUNT, 24, 6)
  SHARED_FIELD(RX_SIZE, 16, 5)
-SHARED_FIELD(TX_COUNT, 9, 5)
+SHARED_FIELD(TX_COUNT, 8, 5)
  FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
  REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
  SHARED_FIELD(RX_BUF, 8, 8)

[PATCH v2 5/8] target/loongarch: Add avail_LSPW to check LSPW instructions

[Song Gao]

Signed-off-by: Song Gao 
---
 target/loongarch/insn_trans/trans_privileged.c.inc | 8 
 target/loongarch/translate.h   | 1 +
 2 files changed, 9 insertions(+)

diff --git a/target/loongarch/insn_trans/trans_privileged.c.inc 
b/target/loongarch/insn_trans/trans_privileged.c.inc
index 684ff547a7..099cd871f0 100644
--- a/target/loongarch/insn_trans/trans_privileged.c.inc
+++ b/target/loongarch/insn_trans/trans_privileged.c.inc
@@ -437,6 +437,10 @@ static bool trans_ldpte(DisasContext *ctx, arg_ldpte *a)
 TCGv_i32 mem_idx = tcg_constant_i32(ctx->mem_idx);
 TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
 
+if (!avail_LSPW(ctx)) {
+return true;
+}
+
 if (check_plv(ctx)) {
 return false;
 }
@@ -450,6 +454,10 @@ static bool trans_lddir(DisasContext *ctx, arg_lddir *a)
 TCGv src = gpr_src(ctx, a->rj, EXT_NONE);
 TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
 
+if (!avail_LSPW(ctx)) {
+return true;
+}
+
 if (check_plv(ctx)) {
 return false;
 }
diff --git a/target/loongarch/translate.h b/target/loongarch/translate.h
index 0f244cd83b..f0d7b82932 100644
--- a/target/loongarch/translate.h
+++ b/target/loongarch/translate.h
@@ -20,6 +20,7 @@
 #define avail_FP(C)(FIELD_EX32((C)->cpucfg2, CPUCFG2, FP))
 #define avail_FP_SP(C) (FIELD_EX32((C)->cpucfg2, CPUCFG2, FP_SP))
 #define avail_FP_DP(C) (FIELD_EX32((C)->cpucfg2, CPUCFG2, FP_DP))
+#define avail_LSPW(C)  (FIELD_EX32((C)->cpucfg2, CPUCFG2, LSPW))
 
 /*
  * If an operation is being performed on less than TARGET_LONG_BITS,
-- 
2.39.1

[PATCH v2 7/8] target/loongarch: Add avail_LSX to check LSX instructions

[Song Gao]

Signed-off-by: Song Gao 
---
 target/loongarch/insn_trans/trans_lsx.c.inc | 1482 ++-
 target/loongarch/translate.h|2 +
 2 files changed, 823 insertions(+), 661 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_lsx.c.inc 
b/target/loongarch/insn_trans/trans_lsx.c.inc
index 45e0e738ad..5fbf2718f7 100644
--- a/target/loongarch/insn_trans/trans_lsx.c.inc
+++ b/target/loongarch/insn_trans/trans_lsx.c.inc
@@ -135,16 +135,20 @@ static bool gvec_subi(DisasContext *ctx, arg_vv_i *a, 
MemOp mop)
 return true;
 }
 
-TRANS(vadd_b, ALL, gvec_vvv, MO_8, tcg_gen_gvec_add)
-TRANS(vadd_h, ALL, gvec_vvv, MO_16, tcg_gen_gvec_add)
-TRANS(vadd_w, ALL, gvec_vvv, MO_32, tcg_gen_gvec_add)
-TRANS(vadd_d, ALL, gvec_vvv, MO_64, tcg_gen_gvec_add)
+TRANS(vadd_b, LSX, gvec_vvv, MO_8, tcg_gen_gvec_add)
+TRANS(vadd_h, LSX, gvec_vvv, MO_16, tcg_gen_gvec_add)
+TRANS(vadd_w, LSX, gvec_vvv, MO_32, tcg_gen_gvec_add)
+TRANS(vadd_d, LSX, gvec_vvv, MO_64, tcg_gen_gvec_add)
 
 #define VADDSUB_Q(NAME)\
 static bool trans_v## NAME ##_q(DisasContext *ctx, arg_vvv *a) \
 {  \
 TCGv_i64 rh, rl, ah, al, bh, bl;   \
\
+if (!avail_LSX(ctx)) { \
+return false;  \
+}  \
+   \
 CHECK_SXE; \
\
 rh = tcg_temp_new_i64();   \
@@ -170,58 +174,58 @@ static bool trans_v## NAME ##_q(DisasContext *ctx, 
arg_vvv *a) \
 VADDSUB_Q(add)
 VADDSUB_Q(sub)
 
-TRANS(vsub_b, ALL, gvec_vvv, MO_8, tcg_gen_gvec_sub)
-TRANS(vsub_h, ALL, gvec_vvv, MO_16, tcg_gen_gvec_sub)
-TRANS(vsub_w, ALL, gvec_vvv, MO_32, tcg_gen_gvec_sub)
-TRANS(vsub_d, ALL, gvec_vvv, MO_64, tcg_gen_gvec_sub)
-
-TRANS(vaddi_bu, ALL, gvec_vv_i, MO_8, tcg_gen_gvec_addi)
-TRANS(vaddi_hu, ALL, gvec_vv_i, MO_16, tcg_gen_gvec_addi)
-TRANS(vaddi_wu, ALL, gvec_vv_i, MO_32, tcg_gen_gvec_addi)
-TRANS(vaddi_du, ALL, gvec_vv_i, MO_64, tcg_gen_gvec_addi)
-TRANS(vsubi_bu, ALL, gvec_subi, MO_8)
-TRANS(vsubi_hu, ALL, gvec_subi, MO_16)
-TRANS(vsubi_wu, ALL, gvec_subi, MO_32)
-TRANS(vsubi_du, ALL, gvec_subi, MO_64)
-
-TRANS(vneg_b, ALL, gvec_vv, MO_8, tcg_gen_gvec_neg)
-TRANS(vneg_h, ALL, gvec_vv, MO_16, tcg_gen_gvec_neg)
-TRANS(vneg_w, ALL, gvec_vv, MO_32, tcg_gen_gvec_neg)
-TRANS(vneg_d, ALL, gvec_vv, MO_64, tcg_gen_gvec_neg)
-
-TRANS(vsadd_b, ALL, gvec_vvv, MO_8, tcg_gen_gvec_ssadd)
-TRANS(vsadd_h, ALL, gvec_vvv, MO_16, tcg_gen_gvec_ssadd)
-TRANS(vsadd_w, ALL, gvec_vvv, MO_32, tcg_gen_gvec_ssadd)
-TRANS(vsadd_d, ALL, gvec_vvv, MO_64, tcg_gen_gvec_ssadd)
-TRANS(vsadd_bu, ALL, gvec_vvv, MO_8, tcg_gen_gvec_usadd)
-TRANS(vsadd_hu, ALL, gvec_vvv, MO_16, tcg_gen_gvec_usadd)
-TRANS(vsadd_wu, ALL, gvec_vvv, MO_32, tcg_gen_gvec_usadd)
-TRANS(vsadd_du, ALL, gvec_vvv, MO_64, tcg_gen_gvec_usadd)
-TRANS(vssub_b, ALL, gvec_vvv, MO_8, tcg_gen_gvec_sssub)
-TRANS(vssub_h, ALL, gvec_vvv, MO_16, tcg_gen_gvec_sssub)
-TRANS(vssub_w, ALL, gvec_vvv, MO_32, tcg_gen_gvec_sssub)
-TRANS(vssub_d, ALL, gvec_vvv, MO_64, tcg_gen_gvec_sssub)
-TRANS(vssub_bu, ALL, gvec_vvv, MO_8, tcg_gen_gvec_ussub)
-TRANS(vssub_hu, ALL, gvec_vvv, MO_16, tcg_gen_gvec_ussub)
-TRANS(vssub_wu, ALL, gvec_vvv, MO_32, tcg_gen_gvec_ussub)
-TRANS(vssub_du, ALL, gvec_vvv, MO_64, tcg_gen_gvec_ussub)
-
-TRANS(vhaddw_h_b, ALL, gen_vvv, gen_helper_vhaddw_h_b)
-TRANS(vhaddw_w_h, ALL, gen_vvv, gen_helper_vhaddw_w_h)
-TRANS(vhaddw_d_w, ALL, gen_vvv, gen_helper_vhaddw_d_w)
-TRANS(vhaddw_q_d, ALL, gen_vvv, gen_helper_vhaddw_q_d)
-TRANS(vhaddw_hu_bu, ALL, gen_vvv, gen_helper_vhaddw_hu_bu)
-TRANS(vhaddw_wu_hu, ALL, gen_vvv, gen_helper_vhaddw_wu_hu)
-TRANS(vhaddw_du_wu, ALL, gen_vvv, gen_helper_vhaddw_du_wu)
-TRANS(vhaddw_qu_du, ALL, gen_vvv, gen_helper_vhaddw_qu_du)
-TRANS(vhsubw_h_b, ALL, gen_vvv, gen_helper_vhsubw_h_b)
-TRANS(vhsubw_w_h, ALL, gen_vvv, gen_helper_vhsubw_w_h)
-TRANS(vhsubw_d_w, ALL, gen_vvv, gen_helper_vhsubw_d_w)
-TRANS(vhsubw_q_d, ALL, gen_vvv, gen_helper_vhsubw_q_d)
-TRANS(vhsubw_hu_bu, ALL, gen_vvv, gen_helper_vhsubw_hu_bu)
-TRANS(vhsubw_wu_hu, ALL, gen_vvv, gen_helper_vhsubw_wu_hu)
-TRANS(vhsubw_du_wu, ALL, gen_vvv, gen_helper_vhsubw_du_wu)
-TRANS(vhsubw_qu_du, ALL, gen_vvv, gen_helper_vhsubw_qu_du)
+TRANS(vsub_b, LSX, gvec_vvv, MO_8, tcg_gen_gvec_sub)
+TRANS(vsub_h, LSX, gvec_vvv, MO_16, tcg_gen_gvec_sub)
+TRANS(vsub_w, LSX, gvec_vvv, MO_32, tcg_gen_gvec_sub)
+TRANS(vsub_d, LSX, gvec_vvv, MO_64, tcg_gen_gvec_sub)
+
+TRANS(vaddi_bu, LSX, gvec_vv_i, MO_8, tcg_gen_gvec_addi)
+TRANS(vaddi_hu, LSX, gvec_vv_i, MO_16, tcg_gen_gvec_addi)
+TRANS(vaddi_wu, LSX, gvec_vv_i, MO_32, tcg_gen_gv

[PATCH v2 4/8] target/loongarch: Add avail_FP/FP_SP/FP_DP to check fpu instructions

[Song Gao]

Signed-off-by: Song Gao 
---
 .../loongarch/insn_trans/trans_farith.c.inc   | 96 ---
 target/loongarch/insn_trans/trans_fcmp.c.inc  |  8 ++
 target/loongarch/insn_trans/trans_fcnv.c.inc  | 56 +--
 .../loongarch/insn_trans/trans_fmemory.c.inc  | 32 +++
 target/loongarch/insn_trans/trans_fmov.c.inc  | 48 --
 target/loongarch/translate.h  |  3 +
 6 files changed, 157 insertions(+), 86 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_farith.c.inc 
b/target/loongarch/insn_trans/trans_farith.c.inc
index b1a1dc7b01..a7ced99fd3 100644
--- a/target/loongarch/insn_trans/trans_farith.c.inc
+++ b/target/loongarch/insn_trans/trans_farith.c.inc
@@ -67,6 +67,10 @@ static bool trans_fcopysign_s(DisasContext *ctx, 
arg_fcopysign_s *a)
 TCGv src1 = get_fpr(ctx, a->fk);
 TCGv src2 = get_fpr(ctx, a->fj);
 
+if (!avail_FP_SP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_deposit_i64(dest, src1, src2, 0, 31);
@@ -81,6 +85,10 @@ static bool trans_fcopysign_d(DisasContext *ctx, 
arg_fcopysign_d *a)
 TCGv src1 = get_fpr(ctx, a->fk);
 TCGv src2 = get_fpr(ctx, a->fj);
 
+if (!avail_FP_DP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_deposit_i64(dest, src1, src2, 0, 63);
@@ -94,6 +102,10 @@ static bool trans_fabs_s(DisasContext *ctx, arg_fabs_s *a)
 TCGv dest = get_fpr(ctx, a->fd);
 TCGv src = get_fpr(ctx, a->fj);
 
+if (!avail_FP_SP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_andi_i64(dest, src, MAKE_64BIT_MASK(0, 31));
@@ -108,6 +120,10 @@ static bool trans_fabs_d(DisasContext *ctx, arg_fabs_d *a)
 TCGv dest = get_fpr(ctx, a->fd);
 TCGv src = get_fpr(ctx, a->fj);
 
+if (!avail_FP_DP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_andi_i64(dest, src, MAKE_64BIT_MASK(0, 63));
@@ -121,6 +137,10 @@ static bool trans_fneg_s(DisasContext *ctx, arg_fneg_s *a)
 TCGv dest = get_fpr(ctx, a->fd);
 TCGv src = get_fpr(ctx, a->fj);
 
+if (!avail_FP_SP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_xori_i64(dest, src, 0x8000);
@@ -135,6 +155,10 @@ static bool trans_fneg_d(DisasContext *ctx, arg_fneg_d *a)
 TCGv dest = get_fpr(ctx, a->fd);
 TCGv src = get_fpr(ctx, a->fj);
 
+if (!avail_FP_DP(ctx)) {
+return false;
+}
+
 CHECK_FPE;
 
 tcg_gen_xori_i64(dest, src, 0x8000LL);
@@ -143,41 +167,41 @@ static bool trans_fneg_d(DisasContext *ctx, arg_fneg_d *a)
 return true;
 }
 
-TRANS(fadd_s, ALL, gen_fff, gen_helper_fadd_s)
-TRANS(fadd_d, ALL, gen_fff, gen_helper_fadd_d)
-TRANS(fsub_s, ALL, gen_fff, gen_helper_fsub_s)
-TRANS(fsub_d, ALL, gen_fff, gen_helper_fsub_d)
-TRANS(fmul_s, ALL, gen_fff, gen_helper_fmul_s)
-TRANS(fmul_d, ALL, gen_fff, gen_helper_fmul_d)
-TRANS(fdiv_s, ALL, gen_fff, gen_helper_fdiv_s)
-TRANS(fdiv_d, ALL, gen_fff, gen_helper_fdiv_d)
-TRANS(fmax_s, ALL, gen_fff, gen_helper_fmax_s)
-TRANS(fmax_d, ALL, gen_fff, gen_helper_fmax_d)
-TRANS(fmin_s, ALL, gen_fff, gen_helper_fmin_s)
-TRANS(fmin_d, ALL, gen_fff, gen_helper_fmin_d)
-TRANS(fmaxa_s, ALL, gen_fff, gen_helper_fmaxa_s)
-TRANS(fmaxa_d, ALL, gen_fff, gen_helper_fmaxa_d)
-TRANS(fmina_s, ALL, gen_fff, gen_helper_fmina_s)
-TRANS(fmina_d, ALL, gen_fff, gen_helper_fmina_d)
-TRANS(fscaleb_s, ALL, gen_fff, gen_helper_fscaleb_s)
-TRANS(fscaleb_d, ALL, gen_fff, gen_helper_fscaleb_d)
-TRANS(fsqrt_s, ALL, gen_ff, gen_helper_fsqrt_s)
-TRANS(fsqrt_d, ALL, gen_ff, gen_helper_fsqrt_d)
-TRANS(frecip_s, ALL, gen_ff, gen_helper_frecip_s)
-TRANS(frecip_d, ALL, gen_ff, gen_helper_frecip_d)
-TRANS(frsqrt_s, ALL, gen_ff, gen_helper_frsqrt_s)
-TRANS(frsqrt_d, ALL, gen_ff, gen_helper_frsqrt_d)
-TRANS(flogb_s, ALL, gen_ff, gen_helper_flogb_s)
-TRANS(flogb_d, ALL, gen_ff, gen_helper_flogb_d)
-TRANS(fclass_s, ALL, gen_ff, gen_helper_fclass_s)
-TRANS(fclass_d, ALL, gen_ff, gen_helper_fclass_d)
-TRANS(fmadd_s, ALL, gen_muladd, gen_helper_fmuladd_s, 0)
-TRANS(fmadd_d, ALL, gen_muladd, gen_helper_fmuladd_d, 0)
-TRANS(fmsub_s, ALL, gen_muladd, gen_helper_fmuladd_s, float_muladd_negate_c)
-TRANS(fmsub_d, ALL, gen_muladd, gen_helper_fmuladd_d, float_muladd_negate_c)
-TRANS(fnmadd_s, ALL, gen_muladd, gen_helper_fmuladd_s, 
float_muladd_negate_result)
-TRANS(fnmadd_d, ALL, gen_muladd, gen_helper_fmuladd_d, 
float_muladd_negate_result)
-TRANS(fnmsub_s, ALL, gen_muladd, gen_helper_fmuladd_s,
+TRANS(fadd_s, FP_SP, gen_fff, gen_helper_fadd_s)
+TRANS(fadd_d, FP_DP, gen_fff, gen_helper_fadd_d)
+TRANS(fsub_s, FP_SP, gen_fff, gen_helper_fsub_s)
+TRANS(fsub_d, FP_DP, gen_fff, gen_helper_fsub_d)
+TRANS(fmul_s, FP_SP, gen_fff, gen_helper_fmul_s)
+TRANS(fmul_d, FP_DP, gen_fff, gen_helper_fmul_d)
+TRANS(fdiv_s, FP_SP, gen_fff, gen_helper_fdiv_s)
+TRANS(fdiv_d, FP_DP, gen_fff, gen_helper_fdiv_d)
+TRANS(fmax_s, FP_SP, gen_fff, gen_helper_fmax_s)
+TRANS(fmax_d, FP_DP, gen_fff, gen_helper_fmax_d)
+TRANS(fmin_s, FP_SP, gen_fff, gen_helper_fmin_

[PATCH v2 1/8] target/loongarch: Fix loongarch_la464_initfn() misses setting LSPW.

[Song Gao]

Reviewed-by: Richard Henderson 
Signed-off-by: Song Gao 
---
 target/loongarch/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
index dd1cd7d7d2..95e00a044c 100644
--- a/target/loongarch/cpu.c
+++ b/target/loongarch/cpu.c
@@ -391,6 +391,7 @@ static void loongarch_la464_initfn(Object *obj)
 data = FIELD_DP32(data, CPUCFG2, LSX, 1),
 data = FIELD_DP32(data, CPUCFG2, LLFTP, 1);
 data = FIELD_DP32(data, CPUCFG2, LLFTP_VER, 1);
+data = FIELD_DP32(data, CPUCFG2, LSPW, 1);
 data = FIELD_DP32(data, CPUCFG2, LAM, 1);
 env->cpucfg[2] = data;
 
-- 
2.39.1

[PATCH v2 0/8] Add some checks before translating instructions

[Song Gao]

Based-on: https://patchew.org/QEMU/20230809083258.1787464-...@jia.je/

Hi,

This series adds some checks before translating instructions

This includes:

CPUCFG[1].IOCSR

CPUCFG[2].FP
CPUCFG[2].FP_SP
CPUCFG[2].FP_DP
CPUCFG[2].LSPW
CPUCFG[2].LAM
CPUCFG[2].LSX

V2:
- Add a check parameter to the TRANS macro.
- remove TRANS_64.
- Add avail_ALL/64/FP/FP_SP/FP_DP/LSPW/LAM/LSX/IOCSR
  to check instructions.

Thanks.
Song Gao

Song Gao (8):
  target/loongarch: Fix loongarch_la464_initfn() misses setting LSPW.
  target/loongarch: Add a check parameter to the TRANS macro
  target/loongarch: Add avail_64 to check la64-only instructions
  target/loongarch: Add avail_FP/FP_SP/FP_DP to check fpu instructions
  target/loongarch: Add avail_LSPW to check LSPW instructions
  target/loongarch: Add avail_LAM to check atomic instructions
  target/loongarch: Add avail_LSX to check LSX instructions
  target/loongarch: Add avail_IOCSR to check iocsr instructions

 target/loongarch/cpu.c|1 +
 target/loongarch/insn_trans/trans_arith.c.inc |   96 +-
 .../loongarch/insn_trans/trans_atomic.c.inc   |   92 +-
 target/loongarch/insn_trans/trans_bit.c.inc   |   56 +-
 .../loongarch/insn_trans/trans_branch.c.inc   |   20 +-
 target/loongarch/insn_trans/trans_extra.c.inc |   28 +-
 .../loongarch/insn_trans/trans_farith.c.inc   |   96 +-
 target/loongarch/insn_trans/trans_fcmp.c.inc  |8 +
 target/loongarch/insn_trans/trans_fcnv.c.inc  |   56 +-
 .../loongarch/insn_trans/trans_fmemory.c.inc  |   32 +-
 target/loongarch/insn_trans/trans_fmov.c.inc  |   52 +-
 target/loongarch/insn_trans/trans_lsx.c.inc   | 1482 +
 .../loongarch/insn_trans/trans_memory.c.inc   |   84 +-
 .../insn_trans/trans_privileged.c.inc |   24 +-
 target/loongarch/insn_trans/trans_shift.c.inc |   34 +-
 target/loongarch/translate.c  |3 +
 target/loongarch/translate.h  |   24 +-
 17 files changed, 1237 insertions(+), 951 deletions(-)

-- 
2.39.1

[PATCH v2 8/8] target/loongarch: Add avail_IOCSR to check iocsr instructions

[Song Gao]

Signed-off-by: Song Gao 
---
 .../loongarch/insn_trans/trans_privileged.c.inc  | 16 
 target/loongarch/translate.h |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_privileged.c.inc 
b/target/loongarch/insn_trans/trans_privileged.c.inc
index 099cd871f0..4cb701b4b5 100644
--- a/target/loongarch/insn_trans/trans_privileged.c.inc
+++ b/target/loongarch/insn_trans/trans_privileged.c.inc
@@ -312,14 +312,14 @@ static bool gen_iocsrwr(DisasContext *ctx, arg_rr *a,
 return true;
 }
 
-TRANS(iocsrrd_b, ALL, gen_iocsrrd, gen_helper_iocsrrd_b)
-TRANS(iocsrrd_h, ALL, gen_iocsrrd, gen_helper_iocsrrd_h)
-TRANS(iocsrrd_w, ALL, gen_iocsrrd, gen_helper_iocsrrd_w)
-TRANS(iocsrrd_d, ALL, gen_iocsrrd, gen_helper_iocsrrd_d)
-TRANS(iocsrwr_b, ALL, gen_iocsrwr, gen_helper_iocsrwr_b)
-TRANS(iocsrwr_h, ALL, gen_iocsrwr, gen_helper_iocsrwr_h)
-TRANS(iocsrwr_w, ALL, gen_iocsrwr, gen_helper_iocsrwr_w)
-TRANS(iocsrwr_d, ALL, gen_iocsrwr, gen_helper_iocsrwr_d)
+TRANS(iocsrrd_b, IOCSR, gen_iocsrrd, gen_helper_iocsrrd_b)
+TRANS(iocsrrd_h, IOCSR, gen_iocsrrd, gen_helper_iocsrrd_h)
+TRANS(iocsrrd_w, IOCSR, gen_iocsrrd, gen_helper_iocsrrd_w)
+TRANS(iocsrrd_d, IOCSR, gen_iocsrrd, gen_helper_iocsrrd_d)
+TRANS(iocsrwr_b, IOCSR, gen_iocsrwr, gen_helper_iocsrwr_b)
+TRANS(iocsrwr_h, IOCSR, gen_iocsrwr, gen_helper_iocsrwr_h)
+TRANS(iocsrwr_w, IOCSR, gen_iocsrwr, gen_helper_iocsrwr_w)
+TRANS(iocsrwr_d, IOCSR, gen_iocsrwr, gen_helper_iocsrwr_d)
 
 static void check_mmu_idx(DisasContext *ctx)
 {
diff --git a/target/loongarch/translate.h b/target/loongarch/translate.h
index db46e9aa0f..89b49a859e 100644
--- a/target/loongarch/translate.h
+++ b/target/loongarch/translate.h
@@ -23,7 +23,7 @@
 #define avail_LSPW(C)  (FIELD_EX32((C)->cpucfg2, CPUCFG2, LSPW))
 #define avail_LAM(C)   (FIELD_EX32((C)->cpucfg2, CPUCFG2, LAM))
 #define avail_LSX(C)   (FIELD_EX32((C)->cpucfg2, CPUCFG2, LSX))
-
+#define avail_IOCSR(C) (FIELD_EX32((C)->cpucfg1, CPUCFG1, IOCSR))
 
 /*
  * If an operation is being performed on less than TARGET_LONG_BITS,
-- 
2.39.1

[PATCH v2 2/8] target/loongarch: Add a check parameter to the TRANS macro

[Song Gao]

The default check parmeter is ALL, remove TRANS_64 marco.

Suggested-by: Richard Henderson 
Signed-off-by: Song Gao 
---
 target/loongarch/insn_trans/trans_arith.c.inc |   84 +-
 .../loongarch/insn_trans/trans_atomic.c.inc   |   80 +-
 target/loongarch/insn_trans/trans_bit.c.inc   |   56 +-
 .../loongarch/insn_trans/trans_branch.c.inc   |   20 +-
 target/loongarch/insn_trans/trans_extra.c.inc |   16 +-
 .../loongarch/insn_trans/trans_farith.c.inc   |   72 +-
 target/loongarch/insn_trans/trans_fcnv.c.inc  |   56 +-
 .../loongarch/insn_trans/trans_fmemory.c.inc  |   32 +-
 target/loongarch/insn_trans/trans_fmov.c.inc  |   16 +-
 target/loongarch/insn_trans/trans_lsx.c.inc   | 1322 -
 .../loongarch/insn_trans/trans_memory.c.inc   |   84 +-
 .../insn_trans/trans_privileged.c.inc |   16 +-
 target/loongarch/insn_trans/trans_shift.c.inc |   30 +-
 target/loongarch/translate.c  |3 +
 target/loongarch/translate.h  |   13 +-
 15 files changed, 950 insertions(+), 950 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_arith.c.inc 
b/target/loongarch/insn_trans/trans_arith.c.inc
index e3b7979e15..d7f69a7553 100644
--- a/target/loongarch/insn_trans/trans_arith.c.inc
+++ b/target/loongarch/insn_trans/trans_arith.c.inc
@@ -248,45 +248,45 @@ static bool trans_addu16i_d(DisasContext *ctx, 
arg_addu16i_d *a)
 return true;
 }
 
-TRANS(add_w, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_add_tl)
-TRANS_64(add_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
-TRANS(sub_w, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_sub_tl)
-TRANS_64(sub_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
-TRANS(and, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_and_tl)
-TRANS(or, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_or_tl)
-TRANS(xor, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_xor_tl)
-TRANS(nor, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_nor_tl)
-TRANS(andn, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_andc_tl)
-TRANS(orn, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_orc_tl)
-TRANS(slt, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_slt)
-TRANS(sltu, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_sltu)
-TRANS(mul_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, tcg_gen_mul_tl)
-TRANS_64(mul_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_mul_tl)
-TRANS(mulh_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, gen_mulh_w)
-TRANS(mulh_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, gen_mulh_w)
-TRANS_64(mulh_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
-TRANS_64(mulh_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
-TRANS_64(mulw_d_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
-TRANS_64(mulw_d_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
-TRANS(div_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_div_w)
-TRANS(mod_w, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_rem_w)
-TRANS(div_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_div_du)
-TRANS(mod_wu, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_rem_du)
-TRANS_64(div_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
-TRANS_64(mod_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
-TRANS_64(div_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
-TRANS_64(mod_du, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
-TRANS(slti, gen_rri_v, EXT_NONE, EXT_NONE, gen_slt)
-TRANS(sltui, gen_rri_v, EXT_NONE, EXT_NONE, gen_sltu)
-TRANS(addi_w, gen_rri_c, EXT_NONE, EXT_SIGN, tcg_gen_addi_tl)
-TRANS_64(addi_d, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_addi_tl)
-TRANS(alsl_w, gen_rrr_sa, EXT_NONE, EXT_SIGN, gen_alsl)
-TRANS_64(alsl_wu, gen_rrr_sa, EXT_NONE, EXT_ZERO, gen_alsl)
-TRANS_64(alsl_d, gen_rrr_sa, EXT_NONE, EXT_NONE, gen_alsl)
-TRANS(pcaddi, gen_pc, gen_pcaddi)
-TRANS(pcalau12i, gen_pc, gen_pcalau12i)
-TRANS(pcaddu12i, gen_pc, gen_pcaddu12i)
-TRANS_64(pcaddu18i, gen_pc, gen_pcaddu18i)
-TRANS(andi, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_andi_tl)
-TRANS(ori, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_ori_tl)
-TRANS(xori, gen_rri_c, EXT_NONE, EXT_NONE, tcg_gen_xori_tl)
+TRANS(add_w, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_add_tl)
+TRANS(add_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
+TRANS(sub_w, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_sub_tl)
+TRANS(sub_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
+TRANS(and, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_and_tl)
+TRANS(or, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_or_tl)
+TRANS(xor, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_xor_tl)
+TRANS(nor, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_nor_tl)
+TRANS(andn, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_andc_tl)
+TRANS(orn, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_orc_tl)
+TRANS(slt, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_slt)
+TRANS(sltu, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_sltu)
+TRANS(mul_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, tcg_gen_mul_tl)

[PATCH v2 3/8] target/loongarch: Add avail_64 to check la64-only instructions

[Song Gao]

The la32 manual from [1], and it is not the final version.

[1]: 
https://www.loongson.cn/uploads/images/2023041918122813624.%E9%BE%99%E8%8A%AF%E6%9E%B6%E6%9E%8432%E4%BD%8D%E7%B2%BE%E7%AE%80%E7%89%88%E5%8F%82%E8%80%83%E6%89%8B%E5%86%8C_r1p03.pdf

Co-authored-by: Jiajie Chen 
Signed-off-by: Song Gao 
---
 target/loongarch/insn_trans/trans_arith.c.inc | 48 +++-
 .../loongarch/insn_trans/trans_atomic.c.inc   | 76 +--
 target/loongarch/insn_trans/trans_bit.c.inc   | 56 +++---
 .../loongarch/insn_trans/trans_branch.c.inc   |  4 +-
 target/loongarch/insn_trans/trans_extra.c.inc | 28 +--
 target/loongarch/insn_trans/trans_fmov.c.inc  |  4 +-
 .../loongarch/insn_trans/trans_memory.c.inc   | 68 -
 target/loongarch/insn_trans/trans_shift.c.inc | 24 +++---
 target/loongarch/translate.h  |  2 +
 9 files changed, 170 insertions(+), 140 deletions(-)

diff --git a/target/loongarch/insn_trans/trans_arith.c.inc 
b/target/loongarch/insn_trans/trans_arith.c.inc
index d7f69a7553..2c8b4e23cd 100644
--- a/target/loongarch/insn_trans/trans_arith.c.inc
+++ b/target/loongarch/insn_trans/trans_arith.c.inc
@@ -199,6 +199,10 @@ static bool trans_lu32i_d(DisasContext *ctx, arg_lu32i_d 
*a)
 TCGv src1 = gpr_src(ctx, a->rd, EXT_NONE);
 TCGv src2 = tcg_constant_tl(a->imm);
 
+if (!avail_64(ctx)) {
+return false;
+}
+
 tcg_gen_deposit_tl(dest, src1, src2, 32, 32);
 gen_set_gpr(a->rd, dest, EXT_NONE);
 
@@ -211,6 +215,10 @@ static bool trans_lu52i_d(DisasContext *ctx, arg_lu52i_d 
*a)
 TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
 TCGv src2 = tcg_constant_tl(a->imm);
 
+if (!avail_64(ctx)) {
+return false;
+}
+
 tcg_gen_deposit_tl(dest, src1, src2, 52, 12);
 gen_set_gpr(a->rd, dest, EXT_NONE);
 
@@ -242,6 +250,10 @@ static bool trans_addu16i_d(DisasContext *ctx, 
arg_addu16i_d *a)
 TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
 TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
 
+if (!avail_64(ctx)) {
+return false;
+}
+
 tcg_gen_addi_tl(dest, src1, a->imm << 16);
 gen_set_gpr(a->rd, dest, EXT_NONE);
 
@@ -249,9 +261,9 @@ static bool trans_addu16i_d(DisasContext *ctx, 
arg_addu16i_d *a)
 }
 
 TRANS(add_w, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_add_tl)
-TRANS(add_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
+TRANS(add_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_add_tl)
 TRANS(sub_w, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_SIGN, tcg_gen_sub_tl)
-TRANS(sub_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
+TRANS(sub_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_sub_tl)
 TRANS(and, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_and_tl)
 TRANS(or, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_or_tl)
 TRANS(xor, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_xor_tl)
@@ -261,32 +273,32 @@ TRANS(orn, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, 
tcg_gen_orc_tl)
 TRANS(slt, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_slt)
 TRANS(sltu, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_sltu)
 TRANS(mul_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, tcg_gen_mul_tl)
-TRANS(mul_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_mul_tl)
+TRANS(mul_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, tcg_gen_mul_tl)
 TRANS(mulh_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, gen_mulh_w)
 TRANS(mulh_wu, ALL, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, gen_mulh_w)
-TRANS(mulh_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
-TRANS(mulh_du, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
-TRANS(mulw_d_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
-TRANS(mulw_d_wu, ALL, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
+TRANS(mulh_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_d)
+TRANS(mulh_du, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_mulh_du)
+TRANS(mulw_d_w, 64, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_NONE, tcg_gen_mul_tl)
+TRANS(mulw_d_wu, 64, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_NONE, tcg_gen_mul_tl)
 TRANS(div_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_div_w)
 TRANS(mod_w, ALL, gen_rrr, EXT_SIGN, EXT_SIGN, EXT_SIGN, gen_rem_w)
 TRANS(div_wu, ALL, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_div_du)
 TRANS(mod_wu, ALL, gen_rrr, EXT_ZERO, EXT_ZERO, EXT_SIGN, gen_rem_du)
-TRANS(div_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
-TRANS(mod_d, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
-TRANS(div_du, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
-TRANS(mod_du, ALL, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
+TRANS(div_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_d)
+TRANS(mod_d, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_d)
+TRANS(div_du, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_div_du)
+TRANS(mod_du, 64, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_rem_du)
 TRANS(slti, ALL, gen_rri_v, EXT_NONE, EXT_NONE, gen_slt)
 TRANS(sltui, ALL,

[PATCH v2 6/8] target/loongarch: Add avail_LAM to check atomic instructions

[Song Gao]

Signed-off-by: Song Gao 
---
 target/loongarch/insn_trans/trans_atomic.c.inc | 12 
 target/loongarch/translate.h   |  1 +
 2 files changed, 13 insertions(+)

diff --git a/target/loongarch/insn_trans/trans_atomic.c.inc 
b/target/loongarch/insn_trans/trans_atomic.c.inc
index 194818d74d..867d09375a 100644
--- a/target/loongarch/insn_trans/trans_atomic.c.inc
+++ b/target/loongarch/insn_trans/trans_atomic.c.inc
@@ -9,6 +9,10 @@ static bool gen_ll(DisasContext *ctx, arg_rr_i *a, MemOp mop)
 TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
 TCGv t0 = make_address_i(ctx, src1, a->imm);
 
+if (!avail_LAM(ctx)) {
+return true;
+}
+
 tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, mop);
 tcg_gen_st_tl(t0, cpu_env, offsetof(CPULoongArchState, lladdr));
 tcg_gen_st_tl(dest, cpu_env, offsetof(CPULoongArchState, llval));
@@ -25,6 +29,10 @@ static bool gen_sc(DisasContext *ctx, arg_rr_i *a, MemOp mop)
 TCGv t0 = tcg_temp_new();
 TCGv val = tcg_temp_new();
 
+if (!avail_LAM(ctx)) {
+return true;
+}
+
 TCGLabel *l1 = gen_new_label();
 TCGLabel *done = gen_new_label();
 
@@ -53,6 +61,10 @@ static bool gen_am(DisasContext *ctx, arg_rrr *a,
 TCGv addr = gpr_src(ctx, a->rj, EXT_NONE);
 TCGv val = gpr_src(ctx, a->rk, EXT_NONE);
 
+if (!avail_LAM(ctx)) {
+return true;
+}
+
 if (a->rd != 0 && (a->rj == a->rd || a->rk == a->rd)) {
 qemu_log_mask(LOG_GUEST_ERROR,
   "Warning: source register overlaps destination register"
diff --git a/target/loongarch/translate.h b/target/loongarch/translate.h
index f0d7b82932..faf4ce87f9 100644
--- a/target/loongarch/translate.h
+++ b/target/loongarch/translate.h
@@ -21,6 +21,7 @@
 #define avail_FP_SP(C) (FIELD_EX32((C)->cpucfg2, CPUCFG2, FP_SP))
 #define avail_FP_DP(C) (FIELD_EX32((C)->cpucfg2, CPUCFG2, FP_DP))
 #define avail_LSPW(C)  (FIELD_EX32((C)->cpucfg2, CPUCFG2, LSPW))
+#define avail_LAM(C)   (FIELD_EX32((C)->cpucfg2, CPUCFG2, LAM))
 
 /*
  * If an operation is being performed on less than TARGET_LONG_BITS,
-- 
2.39.1

Re: [PATCH v2 4/4] hw/i2c/aspeed: Add support for BUFFER ORGANIZATION in new register mode

[Cédric Le Goater]

On 8/11/23 07:42, Hang Yu wrote:

Added support for the BUFFER ORGANIZATION option in reg I2CC_POOL_CTRL,


BUFFER ORGANIZATION could be lower case


when set to 1,The buffer is split into two parts: Lower 16 bytes for Tx
and higher 16 bytes for Rx.

Signed-off-by: Hang Yu 
---
  hw/i2c/aspeed_i2c.c | 7 ++-
  include/hw/i2c/aspeed_i2c.h | 1 +
  2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 44905d7899..26fefb8f9e 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -296,7 +296,12 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
  RX_SIZE) + 1;
  
  if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {

-uint8_t *pool_base = aic->bus_pool_base(bus);
+uint8_t *pool_base;
+if (ARRAY_FIELD_EX32(bus->regs, I2CC_POOL_CTRL, BUF_ORGANIZATION)) {
+pool_base = aic->bus_pool_base(bus) + 16;
+} else {
+pool_base = aic->bus_pool_base(bus);
+}


or simply add :

if (ARRAY_FIELD_EX32(bus->regs, I2CC_POOL_CTRL, BUF_ORGANIZATION)) {
pool_base += 16;
}


  
  for (i = 0; i < pool_rx_count; i++) {

  pool_base[i] = i2c_recv(bus->bus);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 2e1e15aaf0..88b144a599 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -162,6 +162,7 @@ REG32(I2CC_MS_TXRX_BYTE_BUF, 0x08)
  /* 15:0  shared with I2CD_BYTE_BUF[15:0] */
  REG32(I2CC_POOL_CTRL, 0x0c)
  /* 31:0 shared with I2CD_POOL_CTRL[31:0] */
+FIELD(I2CC_POOL_CTRL, BUF_ORGANIZATION, 0, 1) /* AST2600 */
  REG32(I2CM_INTR_CTRL, 0x10)
  REG32(I2CM_INTR_STS, 0x14)
  FIELD(I2CM_INTR_STS, PKT_STATE, 28, 4)

Re: [PATCH 17/24] tcg/i386: Merge tcg_out_brcond{32,64}

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:12, Richard Henderson
 wrote:
>
> Pass a rexw parameter instead of duplicating the functions.
>
> Signed-off-by: Richard Henderson 
> ---
>  tcg/i386/tcg-target.c.inc | 110 +-
>  1 file changed, 49 insertions(+), 61 deletions(-)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 18/24] tcg/i386: Merge tcg_out_setcond{32,64}

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:14, Richard Henderson
 wrote:
>
> Pass a rexw parameter instead of duplicating the functions.
>
> Signed-off-by: Richard Henderson 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 19/24] tcg/i386: Merge tcg_out_movcond{32,64}

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:16, Richard Henderson
 wrote:
>
> Pass a rexw parameter instead of duplicating the functions.
>
> Signed-off-by: Richard Henderson 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 20/24] tcg/i386: Add cf parameter to tcg_out_cmp

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:13, Richard Henderson
 wrote:
>
> Add the parameter to avoid TEST and pass along to tgen_arithi.
> All current users pass false.
>
> Signed-off-by: Richard Henderson 
> ---
>  tcg/i386/tcg-target.c.inc | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
> index b88fc14afd..56549ff2a0 100644
> --- a/tcg/i386/tcg-target.c.inc
> +++ b/tcg/i386/tcg-target.c.inc
> @@ -1418,15 +1418,15 @@ static void tcg_out_jxx(TCGContext *s, int opc, 
> TCGLabel *l, bool small)
>  }
>  }
>
> -static void tcg_out_cmp(TCGContext *s, TCGArg arg1, TCGArg arg2,
> -int const_arg2, int rexw)
> +static void tcg_out_cmp(TCGContext *s, int rexw, TCGArg arg1, TCGArg arg2,
> +int const_arg2, bool cf)
>  {
>  if (const_arg2) {
> -if (arg2 == 0) {
> +if (arg2 == 0 && !cf) {
>  /* test r, r */
>  tcg_out_modrm(s, OPC_TESTL + rexw, arg1, arg1);
>  } else {
> -tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, 0);
> +tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, cf);
>  }
>  } else {
>  tgen_arithr(s, ARITH_CMP + rexw, arg1, arg2);

I don't really understand the motivation here.
Why are some uses of this function fine with using the TEST
insn, but some must avoid it? What does 'cf' stand for?
A comment would help here if there isn't a clearer argument
name available...

thanks
-- PMM

Re: [PATCH for-8.2 v3 1/6] vfio/migration: Move from STOP_COPY to STOP in vfio_save_cleanup()

[Cédric Le Goater]

On 8/8/23 08:23, Avihai Horon wrote:


On 07/08/2023 18:53, Cédric Le Goater wrote:

External email: Use caution opening links or attachments


[ Adding Juan and Peter for their awareness ]

On 8/2/23 10:14, Avihai Horon wrote:

Changing the device state from STOP_COPY to STOP can take time as the
device may need to free resources and do other operations as part of the
transition. Currently, this is done in vfio_save_complete_precopy() and
therefore it is counted in the migration downtime.

To avoid this, change the device state from STOP_COPY to STOP in
vfio_save_cleanup(), which is called after migration has completed and
thus is not part of migration downtime.


What bothers me is that this looks like a device specific optimization


True, currently it helps mlx5, but this change is based on the assumption that, 
in general, VFIO devices are likely to free resources when transitioning from 
STOP_COPY to STOP.
So I think this is a good change to have in any case.



and we are loosing the error part.


I don't think we lose the error part.
AFAIU, the crucial part is transitioning to STOP_COPY and sending the final 
data.
If that's done successfully, then migration is successful.
The STOP_COPY->STOP transition is done as part of the cleanup flow, after the 
migration is completed -- i.e., failure in it does not affect the success of 
migration.
Further more, if there is an error in the STOP_COPY->STOP transition, then it's 
reported by vfio_migration_set_state().


It is indeed. I am nit-picking. Pushed on :

  https://github.com/legoater/qemu/tree/vfio-next

It can still be updated before I send a PR. I also provided custom
rpms to our QE team for extras tests.

Should follow Dynamic MSI-X allocation [1] and Joao's series regarding
vIOMMU [2] but first I will take some PTO. See you in a couple of weeks !

Cheers,

C.

[1] 
https://lore.kernel.org/qemu-devel/20230727072410.135743-1-jing2@intel.com/
[2] 
https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.mart...@oracle.com/

Re: [PATCH 20/24] tcg/i386: Add cf parameter to tcg_out_cmp

[Peter Maydell]

On Fri, 11 Aug 2023 at 11:26, Peter Maydell  wrote:
>
> On Tue, 8 Aug 2023 at 04:13, Richard Henderson
>  wrote:
> >
> > Add the parameter to avoid TEST and pass along to tgen_arithi.
> > All current users pass false.
> >
> > Signed-off-by: Richard Henderson 
> > ---
> >  tcg/i386/tcg-target.c.inc | 16 
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
> > index b88fc14afd..56549ff2a0 100644
> > --- a/tcg/i386/tcg-target.c.inc
> > +++ b/tcg/i386/tcg-target.c.inc
> > @@ -1418,15 +1418,15 @@ static void tcg_out_jxx(TCGContext *s, int opc, 
> > TCGLabel *l, bool small)
> >  }
> >  }
> >
> > -static void tcg_out_cmp(TCGContext *s, TCGArg arg1, TCGArg arg2,
> > -int const_arg2, int rexw)
> > +static void tcg_out_cmp(TCGContext *s, int rexw, TCGArg arg1, TCGArg arg2,
> > +int const_arg2, bool cf)
> >  {
> >  if (const_arg2) {
> > -if (arg2 == 0) {
> > +if (arg2 == 0 && !cf) {
> >  /* test r, r */
> >  tcg_out_modrm(s, OPC_TESTL + rexw, arg1, arg1);
> >  } else {
> > -tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, 0);
> > +tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, cf);
> >  }
> >  } else {
> >  tgen_arithr(s, ARITH_CMP + rexw, arg1, arg2);
>
> I don't really understand the motivation here.
> Why are some uses of this function fine with using the TEST
> insn, but some must avoid it? What does 'cf' stand for?
> A comment would help here if there isn't a clearer argument
> name available...

Looking at the following patch suggests perhaps:

/**
 * tcg_out_cmp: Emit a compare, setting the X, Y, Z flags accordingly.
 * @need_cf : true if the comparison must also set CF
 */

(fill in which XYZ flags you can rely on even if need_cf is false)

?

-- PMM

[PATCH trivial] qemu-img: omit errno value in error message

[Michael Tokarev]

I'm getting io-qcow2-244 test failure on mips*
due to output mismatch:

  Take an internal snapshot:
 -qemu-img: Could not create snapshot 'test': -95 (Operation not supported)
 +qemu-img: Could not create snapshot 'test': -122 (Operation not supported)
  No errors were found on the image.

This is because errno values might be different across
different architectures.

This error message in qemu-img.c is the only one which
prints errno directly, all the rest print strerror(errno)
only.  Fix this error message and the expected output
of the 3 test cases too.

Signed-off-by: Michael Tokarev 
---
 qemu-img.c | 4 ++--
 tests/qemu-iotests/080.out | 6 +++---
 tests/qemu-iotests/112.out | 6 +++---
 tests/qemu-iotests/244.out | 2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

(there are a few other places in the code which also print errno,
mostly in vhost area, but let's deal with this one first)


diff --git a/qemu-img.c b/qemu-img.c
index 27f48051b0..0756dbb835 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3468,8 +3468,8 @@ static int img_snapshot(int argc, char **argv)
 
 ret = bdrv_snapshot_create(bs, &sn);
 if (ret) {
-error_report("Could not create snapshot '%s': %d (%s)",
-snapshot_name, ret, strerror(-ret));
+error_report("Could not create snapshot '%s': %s",
+snapshot_name, strerror(-ret));
 }
 break;
 
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 45ab01db8e..d8acb3e723 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -33,7 +33,7 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Snapshot table 
offset invalid
 
 == Hitting snapshot table size limit ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-img: Could not create snapshot 'test': -27 (File too large)
+qemu-img: Could not create snapshot 'test': File too large
 read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
@@ -56,8 +56,8 @@ qemu-io: can't open device TEST_DIR/t.qcow2: Backing file 
name too long
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Could not create snapshot 'test': -27 (File too large)
-qemu-img: Could not create snapshot 'test': -11 (Resource temporarily 
unavailable)
+qemu-img: Could not create snapshot 'test': File too large
+qemu-img: Could not create snapshot 'test': Resource temporarily unavailable
 
 == Invalid snapshot L1 table offset ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index dd3cc4383c..ebf426febc 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -32,7 +32,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 refcount bits: 1
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Could not create snapshot 'foo': -22 (Invalid argument)
+qemu-img: Could not create snapshot 'foo': Invalid argument
 Leaked cluster 6 refcount=1 reference=0
 
 1 leaked clusters were found on the image.
@@ -44,7 +44,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 refcount bits: 2
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Could not create snapshot 'baz': -22 (Invalid argument)
+qemu-img: Could not create snapshot 'baz': Invalid argument
 Leaked cluster 7 refcount=1 reference=0
 
 1 leaked clusters were found on the image.
@@ -75,7 +75,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 refcount bits: 64
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: Could not create snapshot 'foo': -22 (Invalid argument)
+qemu-img: Could not create snapshot 'foo': Invalid argument
 Leaked cluster 5 refcount=18446744073709551615 reference=1
 Leaked cluster 6 refcount=1 reference=0
 
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index 5e03add054..4815a489b0 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -41,7 +41,7 @@ write failed: Operation not supported
 No errors were found on the image.
 
 Take an internal snapshot:
-qemu-img: Could not create snapshot 'test': -95 (Operation not supported)
+qemu-img: Could not create snapshot 'test': Operation not supported
 No errors were found on the image.
 
 === Standalone image with external data file (efficient) ===
-- 
2.39.2

Re: [PATCH 2/2] riscv: zicond: make default

[Daniel Henrique Barboza]

On 8/10/23 15:07, Alistair Francis wrote:

On Tue, Aug 8, 2023 at 6:10 PM Vineet Gupta  wrote:




On 8/8/23 14:06, Daniel Henrique Barboza wrote:

(CCing Alistair and other reviewers)

On 8/8/23 15:17, Vineet Gupta wrote:

Again this helps with better testing and something qemu has been doing
with newer features anyways.

Signed-off-by: Vineet Gupta 
---


Even if we can reach a consensus about removing the experimental (x-
prefix) status
from an extension that is Frozen instead of ratified, enabling stuff
in the default
CPUs because it's easier to test is something we would like to avoid.
The rv64
CPU has a random set of extensions enabled for the most different and
undocumented
reasons, and users don't know what they'll get because we keep beefing
up the
generic CPUs arbitrarily.


The idea was to enable "most" extensions for the virt machine. It's a
bit wishy-washy, but the idea was to enable as much as possible by
default on the virt machine, as long as it doesn't conflict. The goal
being to allow users to get the "best" experience as all their
favourite extensions are enabled.

It's harder to do in practice, so we are in a weird state where users
don't know what is and isn't enabled.

We probably want to revisit this. We should try to enable what is
useful for users and make it clear what is and isn't enabled. I'm not
clear on how best to do that though.


Yeah, thing is how we define 'useful for users'. If you take a look at this
thread where we discussed the 'max' CPU design:

https://lore.kernel.org/qemu-riscv/35a847a1-2720-14ab-61b0-c72d77d5f...@ventanamicro.com/

The 'max' CPU design is rather straightforward, the profile support is also
straightforward (I'll work on that soon), but the role of the rv64 CPU is
confusing.

IMO we should freeze the current rv64 extensions, document it, and the leave
it alone unless we have a very good reason to enable more stuff. We'll have the
max CPU for the use case where users want the maximum amount of stable features
and, with profile support, we can make rv64 operate with a predictable set of
extensions. Seems like a good coverage.


Thanks,

Daniel



Again, I think this comes back to we need to version the virt machine.
I might do that as a starting point, that allows us to make changes in
a clear way.



I understand this position given the arbitrary nature of gazillion
extensions. However pragmatically things like bitmanip and zicond are so
fundamental it would be strange for designs to not have them, in a few
years. Besides these don't compete or conflict with other extensions.
But on face value it is indeed possible for vendors to drop them for
various reasons or no-reasons.

But having the x- dropped is good enough for our needs as there's
already mechanisms to enable the toggles from elf attributes.



Starting on QEMU 8.2 we'll have a 'max' CPU type that will enable all
non-experimental
and non-vendor extensions by default, making it easier for tooling to
test new
features/extensions. All tooling should consider changing their
scripts to use the
'max' CPU when it's available.


That would be great.


The max CPU helps, but I do feel that the default should allow users
to experience as many RISC-V extensions/features as practical.

Alistair





For now, I fear that gcc and friends will still need to enable
'zicond' in the command
line via 'zicond=true'.  Thanks,


Thx,
-Vineet

Re: [PATCH] target/riscv/kvm.c: fix mvendorid size in vcpu_set_machine_ids()

[Daniel Henrique Barboza]

On 8/10/23 14:01, Alistair Francis wrote:

On Wed, Aug 9, 2023 at 6:17 PM Daniel Henrique Barboza
 wrote:


Drew,

On 8/3/23 09:05, Andrew Jones wrote:

On Thu, Aug 03, 2023 at 08:36:57AM -0300, Daniel Henrique Barboza wrote:



On 8/3/23 06:29, Andrew Jones wrote:

On Wed, Aug 02, 2023 at 03:00:58PM -0300, Daniel Henrique Barboza wrote:

cpu->cfg.mvendorid is a 32 bit field and kvm_set_one_reg() always write
a target_ulong val, i.e. a 64 bit field in a 64 bit host.

Given that we're passing a pointer to the mvendorid field, the reg is
reading 64 bits starting from mvendorid and going 32 bits in the next
field, marchid. Here's an example:

$ ./qemu-system-riscv64 -machine virt,accel=kvm -m 2G -smp 1 \
  -cpu rv64,marchid=0xab,mvendorid=0xcd,mimpid=0xef(...)

(inside the guest)
# cat /proc/cpuinfo
processor  : 0
hart   : 0
isa: rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
mmu: sv57
mvendorid  : 0xab00cd
marchid: 0xab
mimpid : 0xef

'mvendorid' was written as a combination of 0xab (the value from the
adjacent field, marchid) and its intended value 0xcd.

Fix it by assigning cpu->cfg.mvendorid to a target_ulong var 'reg' and
use it as input for kvm_set_one_reg(). Here's the result with this patch
applied and using the same QEMU command line:

# cat /proc/cpuinfo
processor  : 0
hart   : 0
isa: rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
mmu: sv57
mvendorid  : 0xcd
marchid: 0xab
mimpid : 0xef

This bug affects only the generic (rv64) CPUs when running with KVM in a
64 bit env since the 'host' CPU does not allow the machine IDs to be
changed via command line.

Fixes: 1fb5a622f7 ("target/riscv: handle mvendorid/marchid/mimpid for KVM CPUs")
Signed-off-by: Daniel Henrique Barboza 
---
target/riscv/kvm.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 9d8a8982f9..b1fd2233c0 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -852,12 +852,19 @@ void kvm_arch_init_irq_routing(KVMState *s)
static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
{
CPURISCVState *env = &cpu->env;
+target_ulong reg;


We can use the type of cfg since KVM just gets an address and uses the
KVM register type to determine the size. So here,

uint32_t reg = cpu->cfg.mvendorid;

and then...


uint64_t id;
int ret;
id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
  KVM_REG_RISCV_CONFIG_REG(mvendorid));
-ret = kvm_set_one_reg(cs, id, &cpu->cfg.mvendorid);
+/*
+ * cfg.mvendorid is an uint32 but a target_ulong will
+ * be written. Assign it to a target_ulong var to avoid
+ * writing pieces of other cpu->cfg fields in the reg.
+ */


...we don't need this comment since we're not doing anything special.


I tried it out and it doesn't seem to work. Here's the result:

/ # cat /proc/cpuinfo
processor: 0
hart : 0
isa  : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
mmu  : sv57
mvendorid: 0xaa00cd
marchid  : 0xab
mimpid   : 0xef


The issue here is that the kernel considers 'mvendorid' as an unsigned long (or
what QEMU calls target_ulong). kvm_set_one_reg() will write an unsigned long
regardless of the uint32_t typing of 'reg', meaning that it'll end up writing
32 bits of uninitialized stuff from the stack.


Indeed, I managed to reverse the problem in my head. We need to to worry
about KVM's notion of the type, not QEMU's. I feel like we still need some
sort of helper, but one that takes the type of the KVM register into
account. KVM_REG_RISCV_CONFIG registers are KVM ulongs, which may be
different than QEMU's ulongs, if we ever supported 32-bit userspace on
64-bit kernels. Also, not all KVM register types are ulong, some are
explicitly u32s and others u64s. I see kvm_riscv_reg_id() is used to try
and get the right KVM reg size set, but it's broken for RISCV_FP_F_REG(),
since those are all u32s, even when KVM has 64-bit ulong (I guess nobody
is testing get/set-one-reg with F registers using that helper, otherwise
we'd be getting EINVAL from KVM). And KVM_REG_RISCV_FP_D_REG(fcsr) is also
broken and RISCV_TIMER_REG() looks broken with 32-bit KVM, since it should
always be u64. I guess all that stuff needs an audit.

So, I think we need a helper that has a switch on the KVM register type
and provides the right sized buffer for each case.


Is this a suggestion to do this right now in this patch? I didn't understand
whether you're ok with the fix as is for 8.1 or if you want more things done
right away.


Do you want this in for 8.1? It might be a bit late. It helps a lot if
you put in the title [PATCH 8.1]


Yeah, sorry for not putting that in the subject.

At this point I don't think it's worth spinning a rc4 because of this patch. If 
you 

Re: [PATCH] disas/riscv: Further correction to LUI disassembly

[Andrew Jones]

On Fri, Aug 11, 2023 at 10:25:52AM +0200, Andrew Jones wrote:
> On Thu, Aug 10, 2023 at 06:27:50PM +0200, Andrew Jones wrote:
> > On Thu, Aug 10, 2023 at 09:12:42AM -0700, Palmer Dabbelt wrote:
> > > On Thu, 10 Aug 2023 08:31:46 PDT (-0700), ajo...@ventanamicro.com wrote:
> > > > On Mon, Jul 31, 2023 at 11:33:20AM -0700, Richard Bagley wrote:
> > > > > The recent commit 36df75a0a9 corrected one aspect of LUI disassembly
> > > > > by recovering the immediate argument from the result of LUI with a
> > > > > shift right by 12. However, the shift right will left-fill with the
> > > > > sign. By applying a mask we recover an unsigned representation of the
> > > > > 20-bit field (which includes a sign bit).
> > > > > 
> > > > > Example:
> > > > > 0xf000 >> 12 = 0x
> > > > > 0xf000 >> 12 & 0xf = 0x000f
> > > > > 
> > > > > Fixes: 36df75a0a9 ("riscv/disas: Fix disas output of upper 
> > > > > immediates")
> > > > > Signed-off-by: Richard Bagley 
> > > > > ---
> > > > >  disas/riscv.c | 9 ++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/disas/riscv.c b/disas/riscv.c
> > > > > index 4023e3fc65..690eb4a1ac 100644
> > > > > --- a/disas/riscv.c
> > > > > +++ b/disas/riscv.c
> > > > > @@ -4723,9 +4723,12 @@ static void format_inst(char *buf, size_t 
> > > > > buflen, size_t tab, rv_decode *dec)
> > > > >  break;
> > > > >  case 'U':
> > > > >  fmt++;
> > > > > -snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12);
> > > > > -append(buf, tmp, buflen);
> > > > > -if (*fmt == 'o') {
> > > > > +if (*fmt == 'i') {
> > > > > +snprintf(tmp, sizeof(tmp), "%d", dec->imm >> 12 & 
> > > > > 0xf);
> > > > 
> > > > Why are we correcting LUI's output, but still outputting sign-extended
> > > > values for AUIPC?
> > > > 
> > > > We can't assemble 'auipc a1, 0x' or 'auipc a1, -1' without 
> > > > getting
> > > > 
> > > >  Error: lui expression not in range 0..1048575
> > > > 
> > > > (and additionally for 0x)
> > > > 
> > > >  Error: value of 0000 too large for field of 4 bytes at 
> > > > 
> > > > 
> > > > either.
> > > > 
> > > > (I see that the assembler's error messages state 'lui', but I was trying
> > > > 'auipc'.)
> > > > 
> > > > I'm using as from gnu binutils 2.40.0.20230214.
> > > > 
> > > > (And, FWIW, I agree with Richard Henderson that these instructions 
> > > > should
> > > > accept negative values.)
> > > 
> > > I'm kind of lost here, and you saying binutils rejects this syntax?  If
> > > that's the case it's probably just an oversight, can you file a bug in
> > > binutils land so folks can see?
> > 
> > Will do.
> >
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=30746
>

But, to try to bring this thread back to the patch under review. While the
binutils BZ may address our preferred way of providing immediates to the
assembler, this patch is trying to make QEMU's output consistent with
objdump. Since objdump always outputs long immediate values as hex, then
it doesn't need to care about negative signs. QEMU seems to prefer
decimal, though, and so does llvm-objdump, which outputs values for these
instructions in the range 0..1048575. So, I guess this patch is making
QEMU consistent with llvm-objdump.

Back to making suggestions for this patch...

1. The commit message should probably say something along the lines of
   what I just wrote in the preceding paragraph to better explain the
   motivation.

2. Unless I'm missing something, then this patch should also address
   AUIPC.

Thanks,
drew

Re: [PATCH 21/24] tcg/i386: Use CMP+SBB in tcg_out_setcond

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:16, Richard Henderson
 wrote:
>
> Use the carry bit to optimize some forms of setcond.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 22/24] tcg/i386: Clear dest first in tcg_out_setcond if possible

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:16, Richard Henderson
 wrote:
>
> Using XOR first is both smaller and more efficient,
> though cannot be applied if it clobbers an input.
>
> Signed-off-by: Richard Henderson 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 23/24] tcg/i386: Use shift in tcg_out_setcond

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:13, Richard Henderson
 wrote:
>
> For LT/GE vs zero, shift down the sign bit.
>
> Signed-off-by: Richard Henderson 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 24/24] tcg/i386: Implement negsetcond_*

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:15, Richard Henderson
 wrote:
>
> Signed-off-by: Richard Henderson 
> ---
>  tcg/i386/tcg-target.h |  4 ++--
>  tcg/i386/tcg-target.c.inc | 27 +++
>  2 files changed, 21 insertions(+), 10 deletions(-)
>

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 16/24] tcg/sparc64: Implement negsetcond_*

[Peter Maydell]

On Tue, 8 Aug 2023 at 04:14, Richard Henderson
 wrote:
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors

[Ilya Maximets]

On 8/9/23 04:37, Jason Wang wrote:
> On Tue, Aug 8, 2023 at 6:28 AM Ilya Maximets  wrote:
>>
>> Lots of virtio functions that are on a hot path in data transmission
>> are initializing indirect descriptor cache at the point of stack
>> allocation.  It's a 112 byte structure that is getting zeroed out on
>> each call adding unnecessary overhead.  It's going to be correctly
>> initialized later via special init function.  The only reason to
>> actually initialize right away is the ability to safely destruct it.
>> However, we only need to destruct it when it was used, i.e. when a
>> desc_cache points to it.
>>
>> Removing these unnecessary stack initializations improves throughput
>> of virtio-net devices in terms of 64B packets per second by 6-14 %
>> depending on the case.  Tested with a proposed af-xdp network backend
>> and a dpdk testpmd application in the guest, but should be beneficial
>> for other virtio devices as well.
>>
>> Signed-off-by: Ilya Maximets 
> 
> Acked-by: Jason Wang 
> 
> Btw, we can probably remove MEMORY_REGION_CACHE_INVALID.

Good point.  I can include that in the patch.  Or just replace it
with a function, as Stefan suggested.

Best regards, Ilya Maximets.

Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors

[Ilya Maximets]

On 8/10/23 17:50, Stefan Hajnoczi wrote:
> On Tue, Aug 08, 2023 at 12:28:47AM +0200, Ilya Maximets wrote:
>> Lots of virtio functions that are on a hot path in data transmission
>> are initializing indirect descriptor cache at the point of stack
>> allocation.  It's a 112 byte structure that is getting zeroed out on
>> each call adding unnecessary overhead.  It's going to be correctly
>> initialized later via special init function.  The only reason to
>> actually initialize right away is the ability to safely destruct it.
>> However, we only need to destruct it when it was used, i.e. when a
>> desc_cache points to it.
>>
>> Removing these unnecessary stack initializations improves throughput
>> of virtio-net devices in terms of 64B packets per second by 6-14 %
>> depending on the case.  Tested with a proposed af-xdp network backend
>> and a dpdk testpmd application in the guest, but should be beneficial
>> for other virtio devices as well.
>>
>> Signed-off-by: Ilya Maximets 
>> ---
>>  hw/virtio/virtio.c | 42 +++---
>>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> Another option is to create an address_space_cache_init_invalid()
> function that only assigns mrs.mr = NULL instead of touching all bytes
> of the struct like = MEMORY_REGION_CACHE_INVALID. There would be less
> code and the existing mrs.mr check in address_space_cache_destroy()
> would serve the same function as the desc_cache == &indirect_desc_cache
> check added by this patch.

It does look simpler this way, indeed.  Though I'm not sure about
a function name.  We have address_space_cache_invalidate() that
does a completely different thing and the invalidated cache can
still be used, while the cache initialized with the newly proposed
address_space_cache_init_invalid() can not be safely used.

I suppose, the problem is not new, since the macro was named similarly,
but making it a function seems to make the issue worse.

Maybe address_space_cache_init_empty() will be a better name?
E.g.:

**
 * address_space_cache_init_empty: Initialize empty #MemoryRegionCache
 *
 * @cache: The #MemoryRegionCache to operate on.
 *
 * Initializes #MemoryRegionCache structure without memory region attached.
 * Cache initialized this way can only be safely destroyed, but not used.
 */
static inline void address_space_cache_init_empty(MemoryRegionCache *cache)
{
cache->mrs.mr = NULL;
}

What do you think?

Best regards, Ilya Maximets.

RFC: guest INTEL GDS mitigation status on patched host

[Jinpu Wang]

Hi folks on the list:

I'm testing the latest Downfall cpu vulnerability mitigation. what I
notice is when both host and guest are using patched kernel +
microcode eg kernel 5.15.125 +  intel-microcode 20230808 on affected
server eg Icelake server.

The mitigation status inside guest is:

Vulnerabilities:
  Gather data sampling:  Unknown: Dependent on hyp
 ervisor status
---> this one.
  Itlb multihit: Not affected
  L1tf:  Not affected
  Mds:   Not affected
  Meltdown:  Not affected
  Mmio stale data:   Vulnerable: Clear CPU buf
 fers attempted, no microc
 ode; SMT Host state unkno
 wn
  Retbleed:  Not affected
  Spec rstack overflow:  Not affected
  Spec store bypass: Mitigation; Speculative S
 tore Bypass disabled via
 prctl and seccomp
  Spectre v1:Mitigation; usercopy/swap
 gs barriers and __user po
 inter sanitization
  Spectre v2:Mitigation; Enhanced IBRS
 , IBPB conditional, RSB f
 illing, PBRSB-eIBRS SW se
 quence
  Srbds: Not affected
  Tsx async abort:   Not affected

According to kernel commit below
commit 81ac7e5d741742d650b4ed6186c4826c1a0631a7
Author: Daniel Sneddon 
Date:   Wed Jul 12 19:43:14 2023 -0700

KVM: Add GDS_NO support to KVM

Gather Data Sampling (GDS) is a transient execution attack using
gather instructions from the AVX2 and AVX512 extensions. This attack
allows malicious code to infer data that was previously stored in
vector registers. Systems that are not vulnerable to GDS will set the
GDS_NO bit of the IA32_ARCH_CAPABILITIES MSR. This is useful for VM
guests that may think they are on vulnerable systems that are, in
fact, not affected. Guests that are running on affected hosts where
the mitigation is enabled are protected as if they were running
on an unaffected system.

On all hosts that are not affected or that are mitigated, set the
GDS_NO bit.

Signed-off-by: Daniel Sneddon 
Signed-off-by: Dave Hansen 
Acked-by: Josh Poimboeuf 

KVM also has the support of GDS_NO, but seems qemu side doesn't pass
the info to guest, that's why it is unknown. IMO qemu should pass
GDS_NO if the host is already patched.

Is Intel or anyone already working on the qemu patch? I know it's not
a must, but good to do.

Thx!
Jinpu Wang @ IONOS Cloud

[PATCH 3/4] target/i386: Format feature_word_info.c.inc

[Tim Wiederhake]

Harmonize the formatting: Use trailing commas, fix indentation and
empty line usage, define "cpuid" fields one per line, unwind index-
assignment in arrays, and remove comments. The information in the
comments is preserved in the xml file.

Signed-off-by: Tim Wiederhake 
---
 target/i386/feature_word_info.c.inc | 230 
 1 file changed, 128 insertions(+), 102 deletions(-)

diff --git a/target/i386/feature_word_info.c.inc 
b/target/i386/feature_word_info.c.inc
index bba7975eab..040c3c4e56 100644
--- a/target/i386/feature_word_info.c.inc
+++ b/target/i386/feature_word_info.c.inc
@@ -6,47 +6,51 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 "tsc", "msr", "pae", "mce",
 "cx8", "apic", NULL, "sep",
 "mtrr", "pge", "mca", "cmov",
-"pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */,
-NULL, "ds" /* Intel dts */, "acpi", "mmx",
+"pat", "pse36", "pn", "clflush",
+NULL, "ds", "acpi", "mmx",
 "fxsr", "sse", "sse2", "ss",
-"ht" /* Intel htt */, "tm", "ia64", "pbe",
+"ht", "tm", "ia64", "pbe",
+},
+.cpuid = {
+.eax = 1,
+.reg = R_EDX,
 },
-.cpuid = {.eax = 1, .reg = R_EDX, },
 .tcg_features = TCG_FEATURES,
 },
 [FEAT_1_ECX] = {
 .type = CPUID_FEATURE_WORD,
 .feat_names = {
-"pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
+"pni", "pclmulqdq", "dtes64", "monitor",
 "ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
 NULL, "pcid", "dca", "sse4.1",
 "sse4.2", "x2apic", "movbe", "popcnt",
-"tsc-deadline", "aes", "xsave", NULL /* osxsave */,
+"tsc-deadline", "aes", "xsave", NULL,
 "avx", "f16c", "rdrand", "hypervisor",
 },
-.cpuid = { .eax = 1, .reg = R_ECX, },
+.cpuid = {
+.eax = 1,
+.reg = R_ECX,
+},
 .tcg_features = TCG_EXT_FEATURES,
 },
-/* Feature names that are already defined on feature_name[] but
- * are set on CPUID[8000_0001].EDX on AMD CPUs don't have their
- * names on feat_names below. They are copied automatically
- * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD.
- */
 [FEAT_8000_0001_EDX] = {
 .type = CPUID_FEATURE_WORD,
 .feat_names = {
-NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
-NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
-NULL /* cx8 */, NULL /* apic */, NULL, "syscall",
-NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
-NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
-"nx", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, "syscall",
+NULL, NULL, NULL, NULL,
+NULL, NULL, NULL, NULL,
+"nx", NULL, "mmxext", NULL,
+NULL, "fxsr-opt", "pdpe1gb", "rdtscp",
 NULL, "lm", "3dnowext", "3dnow",
 },
-.cpuid = { .eax = 0x8001, .reg = R_EDX, },
+.cpuid = {
+.eax = 0x8001,
+.reg = R_EDX,
+},
 .tcg_features = TCG_EXT2_FEATURES,
 },
 [FEAT_8000_0001_ECX] = {
@@ -61,13 +65,11 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 "perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
-.cpuid = { .eax = 0x8001, .reg = R_ECX, },
+.cpuid = {
+.eax = 0x8001,
+.reg = R_ECX,
+},
 .tcg_features = TCG_EXT3_FEATURES,
-/*
- * TOPOEXT is always allowed but can't be enabled blindly by
- * "-cpu host", as it requires consistent cache topology info
- * to be provided so it doesn't confuse guests.
- */
 .no_autoenable_flags = CPUID_EXT3_TOPOEXT,
 },
 [FEAT_C000_0001_EDX] = {
@@ -82,7 +84,10 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
-.cpuid = { .eax = 0xC001, .reg = R_EDX, },
+.cpuid = {
+.eax = 0xC001,
+.reg = R_EDX,
+},
 .tcg_features = TCG_EXT4_FEATURES,
 },
 [FEAT_KVM] = {
@@ -97,7 +102,10 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 "kvmclock-stable-bit", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 },
-.cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EAX, },
+.cpuid = {
+.eax = KVM_CPUID_FEATURES,
+.reg = R_EAX,
+},
 .tcg_features = TCG_

[PATCH 0/4] Generate x86 cpu features

[Tim Wiederhake]

Synchronizing the list of cpu features and models with qemu is a recurring
task in libvirt. For x86, this is done by reading qom-list-properties for
max-x86_64-cpu and manually filtering out everthing that does not look like
a feature name, as well as parsing target/i386/cpu.c for cpu models.

This is a flawed, tedious and error-prone procedure. Ideally, qemu
and libvirt would query a common source for cpu feature and model
related information. Meanwhile, converting this information into an easier
to parse format would help libvirt a lot.

This patch series converts the cpu feature information present in
target/i386/cpu.c (`feature_word_info`) into an xml file and adds a
script to generate the c code from this xml.

A patch set to convert the cpu model data (`builtin_x86_defs`) in the
same way will follow.

Tim Wiederhake (4):
  target/i386: Split out feature_word_info
  target/i386: Translate feature_word_info to xml
  target/i386: Format feature_word_info.c.inc
  target/i386: Autogenerate feature_word_info.c.inc

 target/i386/cpu.c   |  677 +--
 target/i386/feature_word_info.c.inc |  704 
 target/i386/feature_word_info.py|  110 ++
 target/i386/feature_word_info.xml   | 1610 +++
 4 files changed, 2425 insertions(+), 676 deletions(-)
 create mode 100644 target/i386/feature_word_info.c.inc
 create mode 100755 target/i386/feature_word_info.py
 create mode 100644 target/i386/feature_word_info.xml

-- 
2.39.2

[PATCH 1/4] target/i386: Split out feature_word_info

[Tim Wiederhake]

The isolated part will be generated by a script.

Signed-off-by: Tim Wiederhake 
---
 target/i386/cpu.c   | 677 +---
 target/i386/feature_word_info.c.inc | 676 +++
 2 files changed, 677 insertions(+), 676 deletions(-)
 create mode 100644 target/i386/feature_word_info.c.inc

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 97ad229d8b..d44a5b300e 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -762,682 +762,7 @@ void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
 #define TCG_8000_0008_EBX  (CPUID_8000_0008_EBX_XSAVEERPTR | \
   CPUID_8000_0008_EBX_WBNOINVD | CPUID_8000_0008_EBX_KERNEL_FEATURES)
 
-FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
-[FEAT_1_EDX] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-"fpu", "vme", "de", "pse",
-"tsc", "msr", "pae", "mce",
-"cx8", "apic", NULL, "sep",
-"mtrr", "pge", "mca", "cmov",
-"pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */,
-NULL, "ds" /* Intel dts */, "acpi", "mmx",
-"fxsr", "sse", "sse2", "ss",
-"ht" /* Intel htt */, "tm", "ia64", "pbe",
-},
-.cpuid = {.eax = 1, .reg = R_EDX, },
-.tcg_features = TCG_FEATURES,
-},
-[FEAT_1_ECX] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-"pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
-"ds-cpl", "vmx", "smx", "est",
-"tm2", "ssse3", "cid", NULL,
-"fma", "cx16", "xtpr", "pdcm",
-NULL, "pcid", "dca", "sse4.1",
-"sse4.2", "x2apic", "movbe", "popcnt",
-"tsc-deadline", "aes", "xsave", NULL /* osxsave */,
-"avx", "f16c", "rdrand", "hypervisor",
-},
-.cpuid = { .eax = 1, .reg = R_ECX, },
-.tcg_features = TCG_EXT_FEATURES,
-},
-/* Feature names that are already defined on feature_name[] but
- * are set on CPUID[8000_0001].EDX on AMD CPUs don't have their
- * names on feat_names below. They are copied automatically
- * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD.
- */
-[FEAT_8000_0001_EDX] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
-NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
-NULL /* cx8 */, NULL /* apic */, NULL, "syscall",
-NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
-NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
-"nx", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
-NULL, "lm", "3dnowext", "3dnow",
-},
-.cpuid = { .eax = 0x8001, .reg = R_EDX, },
-.tcg_features = TCG_EXT2_FEATURES,
-},
-[FEAT_8000_0001_ECX] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-"lahf-lm", "cmp-legacy", "svm", "extapic",
-"cr8legacy", "abm", "sse4a", "misalignsse",
-"3dnowprefetch", "osvw", "ibs", "xop",
-"skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid-msr",
-NULL, "tbm", "topoext", "perfctr-core",
-"perfctr-nb", NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-},
-.cpuid = { .eax = 0x8001, .reg = R_ECX, },
-.tcg_features = TCG_EXT3_FEATURES,
-/*
- * TOPOEXT is always allowed but can't be enabled blindly by
- * "-cpu host", as it requires consistent cache topology info
- * to be provided so it doesn't confuse guests.
- */
-.no_autoenable_flags = CPUID_EXT3_TOPOEXT,
-},
-[FEAT_C000_0001_EDX] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-NULL, NULL, "xstore", "xstore-en",
-NULL, NULL, "xcrypt", "xcrypt-en",
-"ace2", "ace2-en", "phe", "phe-en",
-"pmm", "pmm-en", NULL, NULL,
-NULL, NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-},
-.cpuid = { .eax = 0xC001, .reg = R_EDX, },
-.tcg_features = TCG_EXT4_FEATURES,
-},
-[FEAT_KVM] = {
-.type = CPUID_FEATURE_WORD,
-.feat_names = {
-"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
-"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
-NULL, "kvm-pv-tlb-flush", NULL, "kvm-pv-ipi",
-"kvm-poll-control", "kvm-pv-sched-yield", "kvm-asyncpf-int", 
"kvm-msi-ext-dest-id",
-NULL, NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-"kvmclock-stable-bit", NULL, NULL, NULL,
-NULL, NULL, NULL, NULL,
-},
-.cpuid = { .eax = KVM_CPU

[PATCH 4/4] target/i386: Autogenerate feature_word_info.c.inc

[Tim Wiederhake]

This introduces no semantic changes to the file.

Signed-off-by: Tim Wiederhake 
---
 target/i386/feature_word_info.c.inc |   2 +
 target/i386/feature_word_info.py| 110 
 target/i386/feature_word_info.xml   |   3 +
 3 files changed, 115 insertions(+)
 create mode 100755 target/i386/feature_word_info.py

diff --git a/target/i386/feature_word_info.c.inc 
b/target/i386/feature_word_info.c.inc
index 040c3c4e56..b8e77ab7e5 100644
--- a/target/i386/feature_word_info.c.inc
+++ b/target/i386/feature_word_info.c.inc
@@ -1,3 +1,5 @@
+/* This file is autogenerated by feature_word_info.py. */
+
 FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
 [FEAT_1_EDX] = {
 .type = CPUID_FEATURE_WORD,
diff --git a/target/i386/feature_word_info.py b/target/i386/feature_word_info.py
new file mode 100755
index 00..95f3931aa0
--- /dev/null
+++ b/target/i386/feature_word_info.py
@@ -0,0 +1,110 @@
+#!/bin/env python3
+
+import lxml.etree
+import os
+
+
+class FeatureWord:
+def __init__(self, xml):
+self.index = xml.values()[0]
+for node in xml:
+if node.tag == "cpuid":
+self.cpuid = dict()
+for child in node:
+self.cpuid[child.tag] = child.text
+elif node.tag == "feat_names":
+self.feat_names = [child.text for child in node]
+else:
+setattr(self, node.tag, node.text)
+
+
+def write_feat_names(f, data):
+f.write(".feat_names = {\n")
+for index, name in enumerate(data):
+if name is None:
+name = "NULL"
+if index % 4 == 0:
+f.write(" " * 11)
+f.write(" " + str(name) + ",")
+if index % 4 == 3:
+f.write("\n")
+f.write("},\n")
+
+
+def write_cpuid(f, data):
+f.write(".cpuid = {\n")
+f.write(".eax = {},\n".format(data["eax"]))
+if "ecx" in data:
+f.write(".needs_ecx = true,\n")
+f.write(".ecx = {},\n".format(data["ecx"]))
+f.write(".reg = {},\n".format(data["reg"]))
+f.write("},\n")
+
+
+def write_msr(f, data):
+f.write(".msr = {\n")
+f.write(".index = {},\n".format(data))
+f.write("},\n")
+
+
+def write_tcg_features(f, data):
+f.write(".tcg_features = {},\n".format(data))
+
+
+def write_unmigratable_flags(f, data):
+f.write(".unmigratable_flags = {},\n".format(data))
+
+
+def write_migratable_flags(f, data):
+f.write(".migratable_flags = {},\n".format(data))
+
+
+def write_no_autoenable_flags(f, data):
+f.write(".no_autoenable_flags = {},\n".format(data))
+
+
+def write_feature_word(f, data):
+f.write("[{}] = {{\n".format(data.index))
+f.write(".type = {},\n".format(data.type))
+if hasattr(data, "feat_names"):
+write_feat_names(f, data.feat_names)
+if hasattr(data, "cpuid"):
+write_cpuid(f, data.cpuid)
+if hasattr(data, "msr"):
+write_msr(f, data.msr)
+if hasattr(data, "tcg_features"):
+write_tcg_features(f, data.tcg_features)
+if hasattr(data, "unmigratable_flags"):
+write_unmigratable_flags(f, data.unmigratable_flags)
+if hasattr(data, "migratable_flags"):
+write_migratable_flags(f, data.migratable_flags)
+if hasattr(data, "no_autoenable_flags"):
+write_no_autoenable_flags(f, data.no_autoenable_flags)
+f.write("},\n")
+
+
+def write_feature_words(f, data):
+f.write("/* This file is autogenerated by feature_word_info.py. */\n\n")
+f.write("FeatureWordInfo feature_word_info[FEATURE_WORDS] = {\n")
+for feature_word in data:
+write_feature_word(f, feature_word)
+f.write("};\n")
+
+
+def main():
+dirname = os.path.dirname(__file__)
+ifile = os.path.join(dirname, "feature_word_info.xml")
+ofile = os.path.join(dirname, "feature_word_info.c.inc")
+
+parser = lxml.etree.XMLParser(remove_comments=True, remove_blank_text=True)
+with open(ifile, "tr") as f:
+doc = lxml.etree.parse(f, parser=parser)
+
+feature_words = [FeatureWord(node) for node in doc.getroot()]
+
+with open(ofile, "tw") as f:
+write_feature_words(f, feature_words)
+
+
+if __name__ == "__main__":
+main()
diff --git a/target/i386/feature_word_info.xml 
b/target/i386/feature_word_info.xml
index ff741b9f5a..662b8b1dfc 100644
--- a/target/i386/feature_word_info.xml
+++ b/target/i386/feature_word_info.xml
@@ -1,3 +1,6 @@
+
 
 
 CPUID_FEATURE_WORD
-- 
2.39.2

[PATCH 2/4] target/i386: Translate feature_word_info to xml

[Tim Wiederhake]

This is the data file that will be used to generate the C code.
All information, including the comments, is preserved.

Signed-off-by: Tim Wiederhake 
---
 target/i386/feature_word_info.xml | 1607 +
 1 file changed, 1607 insertions(+)
 create mode 100644 target/i386/feature_word_info.xml

diff --git a/target/i386/feature_word_info.xml 
b/target/i386/feature_word_info.xml
new file mode 100644
index 00..ff741b9f5a
--- /dev/null
+++ b/target/i386/feature_word_info.xml
@@ -0,0 +1,1607 @@
+
+
+CPUID_FEATURE_WORD
+
+"fpu"
+"vme"
+"de"
+"pse"
+"tsc"
+"msr"
+"pae"
+"mce"
+"cx8"
+"apic"
+
+"sep"
+"mtrr"
+"pge"
+"mca"
+"cmov"
+"pat"
+"pse36"
+"pn"
+"clflush"
+
+"ds"
+"acpi"
+"mmx"
+"fxsr"
+"sse"
+"sse2"
+"ss"
+"ht"
+"tm"
+"ia64"
+"pbe"
+
+
+1
+R_EDX
+
+TCG_FEATURES
+
+
+CPUID_FEATURE_WORD
+
+"pni"
+"pclmulqdq"
+"dtes64"
+"monitor"
+"ds-cpl"
+"vmx"
+"smx"
+"est"
+"tm2"
+"ssse3"
+"cid"
+
+"fma"
+"cx16"
+"xtpr"
+"pdcm"
+
+"pcid"
+"dca"
+"sse4.1"
+"sse4.2"
+"x2apic"
+"movbe"
+"popcnt"
+"tsc-deadline"
+"aes"
+"xsave"
+
+"avx"
+"f16c"
+"rdrand"
+"hypervisor"
+
+
+1
+R_ECX
+
+TCG_EXT_FEATURES
+
+
+
+CPUID_FEATURE_WORD
+
+
+
+
+
+
+
+
+
+
+
+
+"syscall"
+
+
+
+
+
+
+
+
+"nx"
+
+"mmxext"
+
+
+"fxsr-opt"
+"pdpe1gb"
+"rdtscp"
+
+"lm"
+"3dnowext"
+"3dnow"
+
+
+0x8001
+R_EDX
+
+TCG_EXT2_FEATURES
+
+
+CPUID_FEATURE_WORD
+
+"lahf-lm"
+"cmp-legacy"
+"svm"
+"extapic"
+"cr8legacy"
+"abm"
+"sse4a"
+"misalignsse"
+"3dnowprefetch"
+"osvw"
+"ibs"
+"xop"
+"skinit"
+"wdt"
+
+"lwp"
+"fma4"
+"tce"
+
+"nodeid-msr"
+
+"tbm"
+"topoext"
+"perfctr-core"
+"perfctr-nb"
+
+
+
+
+
+
+
+
+
+0x8001
+R_ECX
+
+TCG_EXT3_FEATURES
+
+CPUID_EXT3_TOPOEXT
+
+
+CPUID_FEATURE_WORD
+
+
+
+"xstore"
+"xstore-en"
+
+
+"xcrypt"
+"xcrypt-en"
+"ace2"
+"ace2-en"
+"phe"
+"phe-en"
+"pmm"
+"pmm-en"
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+0xC001
+R_EDX
+
+TCG_EXT4_FEATURES
+
+
+CPUID_FEATURE_WORD
+
+"kvmclock"
+"kvm-nopiodelay"
+"kvm-mmu"
+"kvmclock"
+"kvm-asyncpf"
+"kvm-steal-time"
+"kvm-pv-eoi"
+"kvm-pv-unhalt"
+
+"kvm-pv-tlb-flush"
+
+"kvm-pv-ipi"
+"kvm-poll-control"
+"kvm-pv-sched-yield"
+"kvm-asyncpf-int"
+"kvm-msi-ext-dest-id"
+
+
+
+
+
+
+
+
+"kvmclock-stable-bit"
+
+
+
+
+
+
+
+
+
+KVM_CPUID_FEATURES

Re: CXL volatile memory is not listed

[Jonathan Cameron via]

On Fri, 11 Aug 2023 08:04:26 +0530
Maverickk 78  wrote:

> Jonathan,
> 
> > More generally for the flow that would bring the memory up as system ram
> > you would typically need the bios to have done the CXL enumeration or
> > a bunch of scripts in the kernel to have done it.  In general it can't
> > be fully automated, because there are policy decisions to make on things 
> > like
> > interleaving.  
> 
> BIOS CXL enumeration? is CEDT not enough? or BIOS further needs to
> create an entry
> in the e820 table?
On intel platforms 'maybe' :)  I know how it works on those that just
use the nice standard EFI tables - less familiar with the e820 stuff :)

CEDT says where to find the the various bits of system related CXL stuff.
Nothing in there on the configuration that should be used such as interleaving
as that depends on what the administrator wants. Or on what the BIOS has
decided the users should have.

> 
> >
> > I'm not aware of any open source BIOSs that do it yet.  So you have
> > to rely on the same kernel paths as for persistent memory - manual 
> > configuration
> > etc in the kernel.
> >  
> Manual works with "cxl create regiton"  :)
Great.

Jonathan

> 
> On Thu, 10 Aug 2023 at 16:05, Jonathan Cameron
>  wrote:
> >
> > On Wed, 9 Aug 2023 04:21:47 +0530
> > Maverickk 78  wrote:
> >  
> > > Hello,
> > >
> > > I am running qemu-system-x86_64
> > >
> > > qemu-system-x86_64 --version
> > > QEMU emulator version 8.0.92 (v8.1.0-rc2-80-g0450cf0897)
> > >  
> > +Cc linux-cxl as the answer is more todo with linux than qemu.
> >  
> > > qemu-system-x86_64 \
> > > -m 2G,slots=4,maxmem=4G \
> > > -smp 4 \
> > > -machine type=q35,accel=kvm,cxl=on \
> > > -enable-kvm \
> > > -nographic \
> > > -device pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52 \
> > > -device cxl-rp,id=rp0,bus=cxl.0,chassis=0,port=0,slot=0 \
> > > -object memory-backend-file,id=mem0,mem-path=/tmp/mem0,size=1G,share=true 
> > > \
> > > -device cxl-type3,bus=rp0,volatile-memdev=mem0,id=cxl-mem0 \
> > > -M cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.size=1G  
> >
> > There are some problems upstream at the moment (probably not cxl related but
> > I'm digging). So today I can't boot an x86 machine. (goody)
> >
> >
> > More generally for the flow that would bring the memory up as system ram
> > you would typically need the bios to have done the CXL enumeration or
> > a bunch of scripts in the kernel to have done it.  In general it can't
> > be fully automated, because there are policy decisions to make on things 
> > like
> > interleaving.
> >
> > I'm not aware of any open source BIOSs that do it yet.  So you have
> > to rely on the same kernel paths as for persistent memory - manual 
> > configuration
> > etc in the kernel.
> >
> > There is support in ndctl for those enabling flows, so I'd look there
> > for more information
> >
> > Jonathan
> >
> >  
> > >
> > >
> > > I was expecting the CXL memory to be listed in "System Ram", the lsmem
> > > shows only 2G memory which is System RAM, it's not listing the CXL
> > > memory.
> > >
> > > Do I need to pass any particular parameter in the kernel command line?
> > >
> > > Is there any documentation available? I followed the inputs provided in
> > >
> > > https://lore.kernel.org/linux-mm/y+csoehvlkudn...@kroah.com/T/
> > >
> > > Is there any documentation/blog listed?  
> >  

Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors

[Stefan Hajnoczi]

On Fri, Aug 11, 2023, 08:50 Ilya Maximets  wrote:

> On 8/10/23 17:50, Stefan Hajnoczi wrote:
> > On Tue, Aug 08, 2023 at 12:28:47AM +0200, Ilya Maximets wrote:
> >> Lots of virtio functions that are on a hot path in data transmission
> >> are initializing indirect descriptor cache at the point of stack
> >> allocation.  It's a 112 byte structure that is getting zeroed out on
> >> each call adding unnecessary overhead.  It's going to be correctly
> >> initialized later via special init function.  The only reason to
> >> actually initialize right away is the ability to safely destruct it.
> >> However, we only need to destruct it when it was used, i.e. when a
> >> desc_cache points to it.
> >>
> >> Removing these unnecessary stack initializations improves throughput
> >> of virtio-net devices in terms of 64B packets per second by 6-14 %
> >> depending on the case.  Tested with a proposed af-xdp network backend
> >> and a dpdk testpmd application in the guest, but should be beneficial
> >> for other virtio devices as well.
> >>
> >> Signed-off-by: Ilya Maximets 
> >> ---
> >>  hw/virtio/virtio.c | 42 +++---
> >>  1 file changed, 27 insertions(+), 15 deletions(-)
> >
> > Another option is to create an address_space_cache_init_invalid()
> > function that only assigns mrs.mr = NULL instead of touching all bytes
> > of the struct like = MEMORY_REGION_CACHE_INVALID. There would be less
> > code and the existing mrs.mr check in address_space_cache_destroy()
> > would serve the same function as the desc_cache == &indirect_desc_cache
> > check added by this patch.
>
> It does look simpler this way, indeed.  Though I'm not sure about
> a function name.  We have address_space_cache_invalidate() that
> does a completely different thing and the invalidated cache can
> still be used, while the cache initialized with the newly proposed
> address_space_cache_init_invalid() can not be safely used.
>
> I suppose, the problem is not new, since the macro was named similarly,
> but making it a function seems to make the issue worse.
>
> Maybe address_space_cache_init_empty() will be a better name?
> E.g.:
>
> **
>  * address_space_cache_init_empty: Initialize empty #MemoryRegionCache
>  *
>  * @cache: The #MemoryRegionCache to operate on.
>  *
>  * Initializes #MemoryRegionCache structure without memory region attached.
>  * Cache initialized this way can only be safely destroyed, but not used.
>  */
> static inline void address_space_cache_init_empty(MemoryRegionCache *cache)
> {
> cache->mrs.mr = NULL;
> }
>
> What do you think?
>

init_empty() is good.

Stefan

>

Re: [PATCH] hw/pci-host: Allow extended config space access for Designware PCIe host

[Peter Maydell]

On Fri, 11 Aug 2023 at 10:55, Peter Maydell  wrote:
>
> On Thu, 10 Aug 2023 at 18:51, Michael S. Tsirkin  wrote:
> >
> > On Wed, Aug 09, 2023 at 10:22:50AM +, Jason Chien wrote:
> > > In pcie_bus_realize(), a root bus is realized as a PCIe bus and a non-root
> > > bus is realized as a PCIe bus if its parent bus is a PCIe bus. However,
> > > the child bus "dw-pcie" is realized before the parent bus "pcie" which is
> > > the root PCIe bus. Thus, the extended configuration space is not 
> > > accessible
> > > on "dw-pcie". The issue can be resolved by adding the
> > > PCI_BUS_EXTENDED_CONFIG_SPACE flag to "pcie" before "dw-pcie" is realized.
> > >
> > > Signed-off-by: Jason Chien 
> >
> >
> > Acked-by: Michael S. Tsirkin 
> >
> > I'm not planning another pull before release, hopefully
> > another maintainer can pick it up? Peter?
>
> At the moment I don't have anything intended for 8.1 either,
> so whoever of us does it it would be a 1-patch pullreq...

Also, at this stage in the release cycle it's always worth
asking: is this a regression, or was this bug also in 8.0?

thanks
-- PMM

Re: [PATCH 1/3] linux-user: Fix the build on systems without SOL_ALG

[Philippe Mathieu-Daudé]

On 10/8/23 23:51, Ilya Leoshkevich wrote:

Building QEMU on CentOS 7 fails, because there SOL_ALG is not defined.
There already exists #if defined(SOL_ALG) in do_setsockopt(); add it to
target_to_host_cmsg() as well.


Does including "crypto/afalgpriv.h" help?


Fixes: 27404b6c15c1 ("linux-user: Implement SOL_ALG encryption support")
Signed-off-by: Ilya Leoshkevich 
---
  linux-user/syscall.c | 2 ++
  1 file changed, 2 insertions(+)

Re: [PATCH v2] linux-user/riscv: Use abi type for target_ucontext

[Philippe Mathieu-Daudé]

On 11/8/23 07:54, LIU Zhiwei wrote:

We should not use types dependend on host arch for target_ucontext.
This bug is found when run rv32 applications.

Signed-off-by: LIU Zhiwei 
Reviewed-by: Richard Henderson 
Reviewed-by: Daniel Henrique Barboza 
---
v2:
- Use abi_ptr instead of abi_ulong for uc_link. (Suggest by Philippe 
Mathieu-Daudé)
---
  linux-user/riscv/signal.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH] virtio: don't zero out memory region cache for indirect descriptors

[Ilya Maximets]

On 8/11/23 15:58, Stefan Hajnoczi wrote:
> 
> 
> On Fri, Aug 11, 2023, 08:50 Ilya Maximets  > wrote:
> 
> On 8/10/23 17:50, Stefan Hajnoczi wrote:
> > On Tue, Aug 08, 2023 at 12:28:47AM +0200, Ilya Maximets wrote:
> >> Lots of virtio functions that are on a hot path in data transmission
> >> are initializing indirect descriptor cache at the point of stack
> >> allocation.  It's a 112 byte structure that is getting zeroed out on
> >> each call adding unnecessary overhead.  It's going to be correctly
> >> initialized later via special init function.  The only reason to
> >> actually initialize right away is the ability to safely destruct it.
> >> However, we only need to destruct it when it was used, i.e. when a
> >> desc_cache points to it.
> >>
> >> Removing these unnecessary stack initializations improves throughput
> >> of virtio-net devices in terms of 64B packets per second by 6-14 %
> >> depending on the case.  Tested with a proposed af-xdp network backend
> >> and a dpdk testpmd application in the guest, but should be beneficial
> >> for other virtio devices as well.
> >>
> >> Signed-off-by: Ilya Maximets  >
> >> ---
> >>  hw/virtio/virtio.c | 42 +++---
> >>  1 file changed, 27 insertions(+), 15 deletions(-)
> >
> > Another option is to create an address_space_cache_init_invalid()
> > function that only assigns mrs.mr  = NULL instead of 
> touching all bytes
> > of the struct like = MEMORY_REGION_CACHE_INVALID. There would be less
> > code and the existing mrs.mr  check in 
> address_space_cache_destroy()
> > would serve the same function as the desc_cache == &indirect_desc_cache
> > check added by this patch.
> 
> It does look simpler this way, indeed.  Though I'm not sure about
> a function name.  We have address_space_cache_invalidate() that
> does a completely different thing and the invalidated cache can
> still be used, while the cache initialized with the newly proposed
> address_space_cache_init_invalid() can not be safely used.
> 
> I suppose, the problem is not new, since the macro was named similarly,
> but making it a function seems to make the issue worse.
> 
> Maybe address_space_cache_init_empty() will be a better name?
> E.g.:
> 
> **
>  * address_space_cache_init_empty: Initialize empty #MemoryRegionCache
>  *
>  * @cache: The #MemoryRegionCache to operate on.
>  *
>  * Initializes #MemoryRegionCache structure without memory region 
> attached.
>  * Cache initialized this way can only be safely destroyed, but not used.
>  */
> static inline void address_space_cache_init_empty(MemoryRegionCache 
> *cache)
> {
>     cache->mrs.mr = NULL;
> }
> 
> What do you think?
> 
> 
> init_empty() is good.

I'll use it then.  Will send a v2 shortly.

Thanks!

> 
> Stefan
> 

Re: Re: Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[Peter Xu]

On Fri, Aug 11, 2023 at 01:49:52PM +0800, ThinerLogoer wrote:
> At 2023-08-11 05:24:43, "Peter Xu"  wrote:
> >On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:
> >> >I think we have the following options (there might be more)
> >> >
> >> >1) This patch.
> >> >
> >> >2) New flag for memory-backend-file. We already have "readonly" and 
> >> >"share=". I'm having a hard time coming up with a good name that really 
> >> >describes the subtle difference.
> >> >
> >> >3) Glue behavior to the QEMU machine
> >> >
> >> 
> >> 4) '-deny-private-discard' argv, or environment variable, or both
> >
> >I'd personally vote for (2).  How about "fdperm"?  To describe when we want
> >to use different rw permissions on the file (besides the access permission
> >of the memory we already provided with "readonly"=XXX).  IIUC the only sane
> >value will be ro/rw/default, where "default" should just use the same rw
> >permission as the memory ("readonly"=XXX).
> >
> >Would that be relatively clean and also work in this use case?
> >
> >(the other thing I'd wish we don't have that fallback is, as long as we
> > have any of that "fallback" we'll need to be compatible with it since
> > then, and for ever...)
> 
> If it must be (2), I would vote (2) + (4), with (4) adjust the default 
> behavior of said `fdperm`.
> Mainly because (private+discard) is itself not a good practice and (4) serves
> as a good tool to help catch existing (private+discard) problems.
> 
> Actually (readonly+private) is more reasonable than (private+discard), so I
> want at least one room for a default (readonly+private) behavior.

Just for purely discussion purpose: I think maybe someday private+discard
could work.  IIUC what we're missing is an syscall interface to install a
zero page for a MAP_PRIVATE, atomically freeing what's underneath: it seems
either punching a hole or DONTNEED won't suffice here.  It'll just be
another problem when having zero page involved in file mappings at least.

> 
> Also in my case I kind of have to use "-mem-path" despite it being considered
> to be close to deprecated. Only with this I can avoid knowledge of memory
> backend before migration. Actually there seems to be no equivalent working 
> after-migration
> setup of "-object memory-backend-file,... -machine q35,mem=..." that can match
> before-migration setup of "-machine q35" (specifying nothing). Therefore
> I must make a plan and choose a migration method BEFORE I boot the
> machine and prepare to migrate, reducing the operation freedom.
> Considering that, I have to use "-mem-path" which keeps the freedom but
> has no configurable argument and I have to rely on default config.
> 
> Are there any "-object memory-backend-file..." setup equivalent to "-machine 
> q35"
> that can migrate from and to each other? If there is, I want to try it out.
> By the way "-object memory-backend-file,id=pc.ram" has just been killed by an 
> earlier
> commit.

I'm actually not familiar enough on the interfaces here, but I just checked
up the man page; would this work for you, together with option (2)?

memory-backend='id'
An alternative to legacy -mem-path and mem-prealloc options.  
Allows to use a memory backend as main RAM.

For example:

-object 
memory-backend-file,id=pc.ram,size=512M,mem-path=/hugetlbfs,prealloc=on,share=on
-machine memory-backend=pc.ram
-m 512M

-- 
Peter Xu

[PATCH v2] virtio: don't zero out memory region cache for indirect descriptors

[Ilya Maximets]

Lots of virtio functions that are on a hot path in data transmission
are initializing indirect descriptor cache at the point of stack
allocation.  It's a 112 byte structure that is getting zeroed out on
each call adding unnecessary overhead.  It's going to be correctly
initialized later via special init function.  The only reason to
actually initialize right away is the ability to safely destruct it.
Replacing a designated initializer with a function to only initialize
what is necessary.

Removal of the unnecessary stack initializations improves throughput
of virtio-net devices in terms of 64B packets per second by 6-14 %
depending on the case.  Tested with a proposed af-xdp network backend
and a dpdk testpmd application in the guest, but should be beneficial
for other virtio devices as well.

Signed-off-by: Ilya Maximets 
---

Version 2:

  * Introduced an initialization function, so we don't need to compare
pointers in the end. [Stefan]
  * Removed the now unused macro. [Jason]

 hw/virtio/virtio.c| 20 +++-
 include/exec/memory.h | 16 +---
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 309038fd46..3d768fda5a 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1071,10 +1071,12 @@ static void virtqueue_split_get_avail_bytes(VirtQueue 
*vq,
 VirtIODevice *vdev = vq->vdev;
 unsigned int idx;
 unsigned int total_bufs, in_total, out_total;
-MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+MemoryRegionCache indirect_desc_cache;
 int64_t len = 0;
 int rc;
 
+address_space_cache_init_empty(&indirect_desc_cache);
+
 idx = vq->last_avail_idx;
 total_bufs = in_total = out_total = 0;
 
@@ -1207,12 +1209,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue 
*vq,
 VirtIODevice *vdev = vq->vdev;
 unsigned int idx;
 unsigned int total_bufs, in_total, out_total;
+MemoryRegionCache indirect_desc_cache;
 MemoryRegionCache *desc_cache;
-MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
 int64_t len = 0;
 VRingPackedDesc desc;
 bool wrap_counter;
 
+address_space_cache_init_empty(&indirect_desc_cache);
+
 idx = vq->last_avail_idx;
 wrap_counter = vq->last_avail_wrap_counter;
 total_bufs = in_total = out_total = 0;
@@ -1487,7 +1491,7 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
 {
 unsigned int i, head, max;
 VRingMemoryRegionCaches *caches;
-MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+MemoryRegionCache indirect_desc_cache;
 MemoryRegionCache *desc_cache;
 int64_t len;
 VirtIODevice *vdev = vq->vdev;
@@ -1498,6 +1502,8 @@ static void *virtqueue_split_pop(VirtQueue *vq, size_t sz)
 VRingDesc desc;
 int rc;
 
+address_space_cache_init_empty(&indirect_desc_cache);
+
 RCU_READ_LOCK_GUARD();
 if (virtio_queue_empty_rcu(vq)) {
 goto done;
@@ -1624,7 +1630,7 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t 
sz)
 {
 unsigned int i, max;
 VRingMemoryRegionCaches *caches;
-MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+MemoryRegionCache indirect_desc_cache;
 MemoryRegionCache *desc_cache;
 int64_t len;
 VirtIODevice *vdev = vq->vdev;
@@ -1636,6 +1642,8 @@ static void *virtqueue_packed_pop(VirtQueue *vq, size_t 
sz)
 uint16_t id;
 int rc;
 
+address_space_cache_init_empty(&indirect_desc_cache);
+
 RCU_READ_LOCK_GUARD();
 if (virtio_queue_packed_empty_rcu(vq)) {
 goto done;
@@ -3935,13 +3943,15 @@ VirtioQueueElement 
*qmp_x_query_virtio_queue_element(const char *path,
 } else {
 unsigned int head, i, max;
 VRingMemoryRegionCaches *caches;
-MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
+MemoryRegionCache indirect_desc_cache;
 MemoryRegionCache *desc_cache;
 VRingDesc desc;
 VirtioRingDescList *list = NULL;
 VirtioRingDescList *node;
 int rc; int ndescs;
 
+address_space_cache_init_empty(&indirect_desc_cache);
+
 RCU_READ_LOCK_GUARD();
 
 max = vq->vring.num;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 68284428f8..b1c4329d11 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2664,9 +2664,6 @@ struct MemoryRegionCache {
 bool is_write;
 };
 
-#define MEMORY_REGION_CACHE_INVALID ((MemoryRegionCache) { .mrs.mr = NULL })
-
-
 /* address_space_ld*_cached: load from a cached #MemoryRegion
  * address_space_st*_cached: store into a cached #MemoryRegion
  *
@@ -2755,6 +2752,19 @@ int64_t address_space_cache_init(MemoryRegionCache 
*cache,
  hwaddr len,
  bool is_write);
 
+/**
+ * address_space_cache_init_empty: Initialize empty #MemoryRegionCache
+ *
+ * @cache: The #MemoryRegionCache to operate

Re: [PATCH v2] linux-user/riscv: Use abi type for target_ucontext

[Alistair Francis]

On Fri, Aug 11, 2023 at 1:57 AM LIU Zhiwei  wrote:
>
> We should not use types dependend on host arch for target_ucontext.
> This bug is found when run rv32 applications.
>
> Signed-off-by: LIU Zhiwei 
> Reviewed-by: Richard Henderson 
> Reviewed-by: Daniel Henrique Barboza 

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
> v2:
> - Use abi_ptr instead of abi_ulong for uc_link. (Suggest by Philippe 
> Mathieu-Daudé)
> ---
>  linux-user/riscv/signal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/riscv/signal.c b/linux-user/riscv/signal.c
> index eaa168199a..f989f7f51f 100644
> --- a/linux-user/riscv/signal.c
> +++ b/linux-user/riscv/signal.c
> @@ -38,8 +38,8 @@ struct target_sigcontext {
>  }; /* cf. riscv-linux:arch/riscv/include/uapi/asm/ptrace.h */
>
>  struct target_ucontext {
> -unsigned long uc_flags;
> -struct target_ucontext *uc_link;
> +abi_ulong uc_flags;
> +abi_ptr uc_link;
>  target_stack_t uc_stack;
>  target_sigset_t uc_sigmask;
>  uint8_t   __unused[1024 / 8 - sizeof(target_sigset_t)];
> --
> 2.17.1
>
>

Re: [PATCH v7 0/5] Add RISC-V KVM AIA Support

[Alistair Francis]

On Thu, Jul 27, 2023 at 7:49 AM Yong-Xuan Wang  wrote:
>
> This series adds support for KVM AIA in RISC-V architecture.
>
> In order to test these patches, we require Linux with KVM AIA support which 
> can
> be found in the riscv_kvm_aia_hwaccel_v1 branch at
> https://github.com/avpatel/linux.git

Thanks!

Applied to riscv-to-apply.next

Alistair

>
> ---
> v7:
> - fix compiler warning in PATCH3
> - rename the "kvm-aia" property to "riscv-aia" and add it as a RISC-V KVM
> accelerator property. also move this setting from PATCH5 to PATCH3 in the 
> code.
>
> v6:
> - fix alignment
> - add hart index to the error message of ISMIC address setting in PATCH3
>
> v5:
> - remove the linux-header update patch since the riscv-to-apply.next QEMU has
> synced up to Linux 6.5-rc1 headers.
> - create the APLIC and IMSIC FDT helper functions in PATCH1
> - add the irqfd support in PATCH3
> - fix the comments and refine the code
>
> v4:
> - update the linux header by the scripts/update-linux-headers.sh in PATCH1
> - remove the checking for "aplic_m" before creating S-mode APLIC device in 
> PATCH2
> - add more setting when we initialize the KVM AIA chip in PATCH4
> - fix msi message delivery and the APLIC devices emulation in PATCH5
> - fix the AIA devices mapping with NUMA enabled in PATCH6
> - add "kvm-aia" parameter to sepecify the KVM AIA mode in PATCH6
>
> v3:
> - fix typo
> - tag the linux-header patch as placeholder
>
> v2:
> - rebase to riscv-to-apply.next
> - update the linux header by the scripts/update-linux-headers.sh
>
> Yong-Xuan Wang (5):
>   target/riscv: support the AIA device emulation with KVM enabled
>   target/riscv: check the in-kernel irqchip support
>   target/riscv: Create an KVM AIA irqchip
>   target/riscv: update APLIC and IMSIC to support KVM AIA
>   target/riscv: select KVM AIA in riscv virt machine
>
>  hw/intc/riscv_aplic.c|  56 --
>  hw/intc/riscv_imsic.c|  25 ++-
>  hw/riscv/virt.c  | 372 ---
>  target/riscv/kvm.c   | 196 -
>  target/riscv/kvm_riscv.h |   4 +
>  5 files changed, 454 insertions(+), 199 deletions(-)
>
> --
> 2.17.1
>
>

Re: [PATCH] hw/riscv: virt: Fix riscv,pmu DT node path

[Alistair Francis]

On Thu, Jul 27, 2023 at 10:37 AM Conor Dooley  wrote:
>
> From: Conor Dooley 
>
> On a dtb dumped from the virt machine, dt-validate complains:
> soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], 
> [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280]], 
> 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
> from schema $id: http://devicetree.org/schemas/simple-bus.yaml#
> That's pretty cryptic, but running the dtb back through dtc produces
> something a lot more reasonable:
> Warning (simple_bus_reg): /soc/pmu: missing or empty reg/ranges property
>
> Moving the riscv,pmu node out of the soc bus solves the problem.
>
> Signed-off-by: Conor Dooley 

Acked-by: Alistair Francis 

Alistair

> ---
> CC: Palmer Dabbelt 
> CC: Alistair Francis 
> CC: Bin Meng 
> CC: Weiwei Li 
> CC: Daniel Henrique Barboza 
> CC: Liu Zhiwei 
> CC: qemu-ri...@nongnu.org
> CC: qemu-devel@nongnu.org
> ---
>  hw/riscv/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d90286dc46..25dcc2616e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -732,7 +732,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
>  MachineState *ms = MACHINE(s);
>  RISCVCPU hart = s->soc[0].harts[0];
>
> -pmu_name = g_strdup_printf("/soc/pmu");
> +pmu_name = g_strdup_printf("/pmu");
>  qemu_fdt_add_subnode(ms->fdt, pmu_name);
>  qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
>  riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
> --
> 2.39.2
>
>

[RFC PATCH 0/2] Vhost-vdpa Shadow Virtqueue Hash calculation Support

[Hawkins Jiawei]

This series enables shadowed CVQ to intercept
VIRTIO_NET_CTRL_MQ_HASH_CONFIG command through shadowed CVQ,
update the virtio NIC device model so qemu send it in a
migration, and the restore of that Hash calculation state
in the destination.

Note that this patch should be based on
patch "vdpa: Send all CVQ state load commands in parallel" at [1].

[1]. https://lore.kernel.org/all/cover.1689748694.git.yin31...@gmail.com/

TestStep

1. test the migration using vp-vdpa device
  - For L0 guest, boot QEMU with two virtio-net-pci net device with
`ctrl_vq`, `mq`, `hash` features on, command line like:
-netdev tap,...
-device virtio-net-pci,disable-legacy=on,disable-modern=off,
iommu_platform=on,mq=on,ctrl_vq=on,hash=on,guest_announce=off,
indirect_desc=off,queue_reset=off,...

  - For L1 guest, apply the relative patch series and compile the
source code, start QEMU with two vdpa device with svq mode on,
enable the `ctrl_vq`, `mq`, `hash` features on, command line like:
  -netdev type=vhost-vdpa,x-svq=true,...
  -device virtio-net-pci,mq=on,guest_announce=off,ctrl_vq=on,
hash=on,...

  - For L2 source guest, run the following bash command:
```bash
#!/bin/sh

ethtool -K eth0 rxhash on
```
  - Gdb attach the destination VM and break at the
vhost_vdpa_net_load_rss()

  - Execute the live migration in L2 source monitor

  - Result
* with this series, gdb can hit the breakpoint and continue
the executing without triggering any error or warning.

Hawkins Jiawei (2):
  vdpa: Restore hash calculation state
  vdpa: Allow VIRTIO_NET_F_HASH_REPORT in SVQ

 net/vhost-vdpa.c | 89 
 1 file changed, 89 insertions(+)

-- 
2.25.1

Re: [PATCH 3/8] target/riscv/cpu.c: introduce cpu_cfg_ext_auto_update()

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:22 AM Daniel Henrique Barboza
 wrote:
>
> During realize() time we're activating a lot of extensions based on some
> criteria, e.g.:
>
> if (cpu->cfg.ext_zk) {
> cpu->cfg.ext_zkn = true;
> cpu->cfg.ext_zkr = true;
> cpu->cfg.ext_zkt = true;
> }
>
> This practice resulted in at least one case where we ended up enabling
> something we shouldn't: RVC enabling zca/zcd/zcf when using a CPU that
> has priv_spec older than 1.12.0.
>
> We're also not considering user choice. There's no way of doing it now
> but this is about to change in the next few patches.
>
> cpu_cfg_ext_auto_update() will check for priv version mismatches before
> enabling extensions. If we have a mismatch between the current priv
> version and the extension we want to enable, do not enable it. In the
> near future, this same function will also consider user choice when
> deciding if we're going to enable/disable an extension or not.
>
> For now let's use it to handle zca/zcd/zcf enablement if RVC is enabled.
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 44 +---
>  1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 3e62881d85..75dc83407e 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -168,6 +168,44 @@ static void isa_ext_update_enabled(RISCVCPU *cpu, 
> uint32_t ext_offset,
>  *ext_enabled = en;
>  }
>
> +static int cpu_cfg_ext_get_min_version(uint32_t ext_offset)
> +{
> +int i;
> +
> +for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> +if (isa_edata_arr[i].ext_enable_offset != ext_offset) {
> +continue;
> +}
> +
> +return isa_edata_arr[i].min_version;
> +}
> +
> +/* Default to oldest priv spec if no match found */
> +return PRIV_VERSION_1_10_0;
> +}
> +
> +static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
> +bool value)
> +{
> +CPURISCVState *env = &cpu->env;
> +bool prev_val = isa_ext_is_enabled(cpu, ext_offset);
> +int min_version;
> +
> +if (prev_val == value) {
> +return;
> +}
> +
> +if (value && env->priv_ver != PRIV_VERSION_LATEST) {
> +/* Do not enable it if priv_ver is older than min_version */
> +min_version = cpu_cfg_ext_get_min_version(ext_offset);
> +if (env->priv_ver < min_version) {
> +return;
> +}
> +}
> +
> +isa_ext_update_enabled(cpu, ext_offset, value);
> +}
> +
>  const char * const riscv_int_regnames[] = {
>  "x0/zero", "x1/ra",  "x2/sp",  "x3/gp",  "x4/tp",  "x5/t0",   "x6/t1",
>  "x7/t2",   "x8/s0",  "x9/s1",  "x10/a0", "x11/a1", "x12/a2",  "x13/a3",
> @@ -1248,12 +1286,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>
>  /* zca, zcd and zcf has a PRIV 1.12.0 restriction */
>  if (riscv_has_ext(env, RVC) && env->priv_ver >= PRIV_VERSION_1_12_0) {
> -cpu->cfg.ext_zca = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
>  if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
> -cpu->cfg.ext_zcf = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>  }
>  if (riscv_has_ext(env, RVD)) {
> -cpu->cfg.ext_zcd = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcd), true);
>  }
>  }
>
> --
> 2.41.0
>
>

[RFC PATCH 1/2] vdpa: Restore hash calculation state

[Hawkins Jiawei]

This patch introduces vhost_vdpa_net_load_rss() to restore
the hash calculation state at device's startup.

Note that vhost_vdpa_net_load_rss() has `do_rss` argument,
which allows future code to reuse this function to restore
the receive-side scaling state when the VIRTIO_NET_F_RSS
feature is enabled in SVQ. Currently, vhost_vdpa_net_load_rss()
could only be invoked when `do_rss` is set to false.

Signed-off-by: Hawkins Jiawei 
---
Question:

It seems that virtio_net_handle_rss() currently does not restore the
hash key length parsed from the CVQ command sent from the guest into
n->rss_data and uses the maximum key length in other code.

So for `hash_key_length` field in VIRTIO_NET_CTRL_MQ_HASH_CONFIG command
sent to device, is it okay to also use the maximum key length as its value?
Or should we introduce the `hash_key_length` field in n->rss_data
structure to record the key length from guest and use this value?

 net/vhost-vdpa.c | 88 
 1 file changed, 88 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 7bb29f6009..bd51020771 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -788,6 +788,85 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, 
const VirtIONet *n,
 return 0;
 }
 
+static int vhost_vdpa_net_load_rss(VhostVDPAState *s, const VirtIONet *n,
+   void **out_cursor, void **in_cursor,
+   bool do_rss)
+{
+struct virtio_net_rss_config cfg;
+ssize_t r;
+
+/*
+ * According to VirtIO standard, "Initially the device has all hash
+ * types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.".
+ *
+ * Therefore, there is no need to send this CVQ command if the
+ * driver disable the all hash types, which aligns with
+ * the device's defaults.
+ *
+ * Note that the device's defaults can mismatch the driver's
+ * configuration only at live migration.
+ */
+if (!n->rss_data.enabled ||
+n->rss_data.hash_types == VIRTIO_NET_HASH_REPORT_NONE) {
+return 0;
+}
+
+cfg.hash_types = cpu_to_le32(n->rss_data.hash_types);
+/*
+ * According to VirtIO standard, "Field reserved MUST contain zeroes.
+ * It is defined to make the structure to match the layout of
+ * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
+ *
+ * Therefore, we need to zero the fields in struct virtio_net_rss_config,
+ * which corresponds the `reserved` field in
+ * struct virtio_net_hash_config.
+ */
+memset(&cfg.indirection_table_mask, 0,
+   sizeof_field(struct virtio_net_hash_config, reserved));
+/*
+ * Consider that virtio_net_handle_rss() currently does not restore the
+ * hash key length parsed from the CVQ command sent from the guest into
+ * n->rss_data and uses the maximum key length in other code, so we also
+ * employthe the maxium key length here.
+ */
+cfg.hash_key_length = sizeof(n->rss_data.key);
+
+g_autofree uint16_t *table = g_malloc_n(n->rss_data.indirections_len,
+sizeof(n->rss_data.indirections_table[0]));
+for (int i = 0; i < n->rss_data.indirections_len; ++i) {
+table[i] = cpu_to_le16(n->rss_data.indirections_table[i]);
+}
+
+const struct iovec data[] = {
+{
+.iov_base = &cfg,
+.iov_len = offsetof(struct virtio_net_rss_config,
+indirection_table),
+}, {
+.iov_base = table,
+.iov_len = n->rss_data.indirections_len *
+   sizeof(n->rss_data.indirections_table[0]),
+}, {
+.iov_base = &cfg.max_tx_vq,
+.iov_len = offsetof(struct virtio_net_rss_config, hash_key_data) -
+   offsetof(struct virtio_net_rss_config, max_tx_vq),
+}, {
+.iov_base = (void *)n->rss_data.key,
+.iov_len = sizeof(n->rss_data.key),
+}
+};
+
+r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
+VIRTIO_NET_CTRL_MQ,
+VIRTIO_NET_CTRL_MQ_HASH_CONFIG,
+data, ARRAY_SIZE(data));
+if (unlikely(r < 0)) {
+return r;
+}
+
+return 0;
+}
+
 static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
   const VirtIONet *n,
   void **out_cursor, void **in_cursor)
@@ -812,6 +891,15 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
 return r;
 }
 
+if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_HASH_REPORT)) {
+return 0;
+}
+
+r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor, false);
+if (unlikely(r < 0)) {
+return r;
+}
+
 return 0;
 }
 
-- 
2.25.1

Re: [PATCH 4/8] target/riscv/cpu.c: use cpu_cfg_ext_auto_update() during realize()

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:08 AM Daniel Henrique Barboza
 wrote:
>
> Let's change the other instances in realize() where we're enabling an
> extension based on a certain criteria (e.g. it's a dependency of another
> extension).
>
> We're leaving icsr and ifencei being enabled during RVG for later -
> we'll want to error out in that case. Every other extension enablement
> during realize is now done via cpu_cfg_ext_auto_update().
>
> The end goal is that only cpu init() functions will handle extension
> flags directly via "cpu->cfg.ext_N = true|false".
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 50 +++---
>  1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 75dc83407e..88b263e830 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1174,7 +1174,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  }
>
>  if (cpu->cfg.ext_zfh) {
> -cpu->cfg.ext_zfhmin = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zfhmin), true);
>  }
>
>  if (cpu->cfg.ext_zfhmin && !riscv_has_ext(env, RVF)) {
> @@ -1200,17 +1200,17 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  }
>
>  /* The V vector extension depends on the Zve64d extension */
> -cpu->cfg.ext_zve64d = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64d), true);
>  }
>
>  /* The Zve64d extension depends on the Zve64f extension */
>  if (cpu->cfg.ext_zve64d) {
> -cpu->cfg.ext_zve64f = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve64f), true);
>  }
>
>  /* The Zve64f extension depends on the Zve32f extension */
>  if (cpu->cfg.ext_zve64f) {
> -cpu->cfg.ext_zve32f = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zve32f), true);
>  }
>
>  if (cpu->cfg.ext_zve64d && !riscv_has_ext(env, RVD)) {
> @@ -1224,7 +1224,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  }
>
>  if (cpu->cfg.ext_zvfh) {
> -cpu->cfg.ext_zvfhmin = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zvfhmin), true);
>  }
>
>  if (cpu->cfg.ext_zvfhmin && !cpu->cfg.ext_zve32f) {
> @@ -1254,7 +1254,7 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>
>  /* Set the ISA extensions, checks should have happened above */
>  if (cpu->cfg.ext_zhinx) {
> -cpu->cfg.ext_zhinxmin = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
>  }
>
>  if ((cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinxmin) && 
> !cpu->cfg.ext_zfinx) {
> @@ -1275,12 +1275,12 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  }
>
>  if (cpu->cfg.ext_zce) {
> -cpu->cfg.ext_zca = true;
> -cpu->cfg.ext_zcb = true;
> -cpu->cfg.ext_zcmp = true;
> -cpu->cfg.ext_zcmt = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zca), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcb), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmp), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcmt), true);
>  if (riscv_has_ext(env, RVF) && env->misa_mxl_max == MXL_RV32) {
> -cpu->cfg.ext_zcf = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zcf), true);
>  }
>  }
>
> @@ -1329,26 +1329,26 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>  }
>
>  if (cpu->cfg.ext_zk) {
> -cpu->cfg.ext_zkn = true;
> -cpu->cfg.ext_zkr = true;
> -cpu->cfg.ext_zkt = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkn), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkr), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkt), true);
>  }
>
>  if (cpu->cfg.ext_zkn) {
> -cpu->cfg.ext_zbkb = true;
> -cpu->cfg.ext_zbkc = true;
> -cpu->cfg.ext_zbkx = true;
> -cpu->cfg.ext_zkne = true;
> -cpu->cfg.ext_zknd = true;
> -cpu->cfg.ext_zknh = true;
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkb), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkc), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zbkx), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zkne), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknd), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_zknh), true);
>  }
>
>  if (cpu->cfg.ext_zks) {
> -cpu->cfg.ext_zbkb = true;
> -cpu->cfg.ext_zbkc = true;
> -cpu->cfg.ext_zbkx = true;
> -cpu->cfg.ext_zksed = true;
> -cpu->cfg.ext_zksh = true;
> +   

Re: [PATCH] hw/riscv: virt: Fix riscv,pmu DT node path

[Alistair Francis]

On Thu, Jul 27, 2023 at 10:37 AM Conor Dooley  wrote:
>
> From: Conor Dooley 
>
> On a dtb dumped from the virt machine, dt-validate complains:
> soc: pmu: {'riscv,event-to-mhpmcounters': [[1, 1, 524281], [2, 2, 524284], 
> [65561, 65561, 524280], [65563, 65563, 524280], [65569, 65569, 524280]], 
> 'compatible': ['riscv,pmu']} should not be valid under {'type': 'object'}
> from schema $id: http://devicetree.org/schemas/simple-bus.yaml#
> That's pretty cryptic, but running the dtb back through dtc produces
> something a lot more reasonable:
> Warning (simple_bus_reg): /soc/pmu: missing or empty reg/ranges property
>
> Moving the riscv,pmu node out of the soc bus solves the problem.
>
> Signed-off-by: Conor Dooley 

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
> CC: Palmer Dabbelt 
> CC: Alistair Francis 
> CC: Bin Meng 
> CC: Weiwei Li 
> CC: Daniel Henrique Barboza 
> CC: Liu Zhiwei 
> CC: qemu-ri...@nongnu.org
> CC: qemu-devel@nongnu.org
> ---
>  hw/riscv/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d90286dc46..25dcc2616e 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -732,7 +732,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
>  MachineState *ms = MACHINE(s);
>  RISCVCPU hart = s->soc[0].harts[0];
>
> -pmu_name = g_strdup_printf("/soc/pmu");
> +pmu_name = g_strdup_printf("/pmu");
>  qemu_fdt_add_subnode(ms->fdt, pmu_name);
>  qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
>  riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
> --
> 2.39.2
>
>

pci: Fix the update of interrupt disable bit in PCI_COMMAND register

[Guoyi Tu]

The PCI_COMMAND register is located at offset 4 within
the PCI configuration space and occupies 2 bytes. The
interrupt disable bit is at the 10th bit, which corresponds
to the byte at offset 5 in the PCI configuration space.

In our testing environment, the guest driver may directly
updates the byte at offset 5 in the PCI configuration space.
The backtrace looks like as following:
#0  pci_default_write_config (d=0x5580bbfc6230, addr=5, val_in=5, l=1)
at hw/pci/pci.c:1442
#1  0x5580b8f3156a in virtio_write_config (pci_dev=0x5580bbfc6230, 
address=5, val=5, len=1)

at hw/virtio/virtio-pci.c:605
#2  0x5580b8ed2f3b in pci_host_config_write_common 
(pci_dev=0x5580bbfc6230, addr=5, limit=256,

val=5, len=1) at hw/pci/pci_host.c:81

In this situation, the range_covers_byte function called
by the pci_default_write_config function will return false,
resulting in the inability to handle the interrupt disable
update event.

To fix this issue, we can use the ranges_overlap function
instead of range_covers_byte to determine whether the interrupt
bit has been updated.

Signed-off-by: Guoyi Tu 
Signed-off-by: yuanminghao 
---
 hw/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b8d22e2e74..881d774fb6 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1613,7 +1613,7 @@ void pci_default_write_config(PCIDevice *d, 
uint32_t addr, uint32_t val_in, int

 range_covers_byte(addr, l, PCI_COMMAND))
 pci_update_mappings(d);

-if (range_covers_byte(addr, l, PCI_COMMAND)) {
+if (ranges_overlap(addr, l, PCI_COMMAND, 2)) {
 pci_update_irq_disabled(d, was_irq_disabled);
 memory_region_set_enabled(&d->bus_master_enable_region,
   (pci_get_word(d->config + PCI_COMMAND)
--
2.27.0

--
Guoyi

[RFC PATCH 2/2] vdpa: Allow VIRTIO_NET_F_HASH_REPORT in SVQ

[Hawkins Jiawei]

Enable SVQ with VIRTIO_NET_F_HASH_REPORT feature.

Signed-off-by: Hawkins Jiawei 
---
 net/vhost-vdpa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bd51020771..a13b267250 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -118,6 +118,7 @@ static const uint64_t vdpa_svq_device_features =
 BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
 /* VHOST_F_LOG_ALL is exposed by SVQ */
 BIT_ULL(VHOST_F_LOG_ALL) |
+BIT_ULL(VIRTIO_NET_F_HASH_REPORT) |
 BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
 BIT_ULL(VIRTIO_NET_F_STANDBY) |
 BIT_ULL(VIRTIO_NET_F_SPEED_DUPLEX);
-- 
2.25.1

Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand]

On 10.08.23 23:24, Peter Xu wrote:

On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:

I think we have the following options (there might be more)

1) This patch.

2) New flag for memory-backend-file. We already have "readonly" and
"share=". I'm having a hard time coming up with a good name that really
describes the subtle difference.

3) Glue behavior to the QEMU machine



4) '-deny-private-discard' argv, or environment variable, or both


I'd personally vote for (2).  How about "fdperm"?  To describe when we want
to use different rw permissions on the file (besides the access permission
of the memory we already provided with "readonly"=XXX).  IIUC the only sane
value will be ro/rw/default, where "default" should just use the same rw
permission as the memory ("readonly"=XXX).


Hmm, I'm not particularly happy about that.



Would that be relatively clean and also work in this use case?



I get the feeling that we are over-engineering something that probably 
should never have been allowed: MAP_PRIVATE mapping of a file and 
opening it rw because someone might punch holes into it.


Once we start adding new parameters just for that, I get a bit skeptical 
that this is what we want. The number of people that care about that are 
probably close to 0.


The only real use case where this used to make sense (by accident I 
assume) was with hugetlb. And somehow, we decided that it was a good 
idea for "-mem-path" to use MAP_PRIVATE.


So, what stops us from

a) Leaving -mem-path alone. Keep opening files rw.
b) Make memory-backend-file with shared=off,readonly=off open the file
   read-only
c) Gluing that behavior to a QEMU compat machine

fallocate(PUNCH_HOLE) will fail, and we can probably let 
virtio-mem/virtio-balloon and postcopy refuse to even start (virtio-mem 
already does that) as early as possible.


People that care about any such use case would already get a warning 
when punching a hole today.


If we ever support discarding RAM in that configuration, we can simply 
unlock it again.


Am I missing any important use case?

--
Cheers,

David / dhildenb

Re: [PATCH 5/8] target/riscv/cpu.c: introduce RISCVCPUMultiExtConfig

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:06 AM Daniel Henrique Barboza
 wrote:
>
> If we want to make better decisions when auto-enabling extensions during
> realize() we need a way to tell if an user set an extension manually.
> The RISC-V KVM driver has its own solution via a KVMCPUConfig struct
> that has an 'user_set' flag that is set during the Property set()
> callback. The set() callback also does init() time validations based on
> the current KVM driver capabilities.
>
> For TCG we would want a 'user_set' mechanic too, but we would look
> ad-hoc via cpu_cfg_ext_auto_update() if a certain extension was user set
> or not. If we copy what was made in the KVM side we would look for
> 'user_set' for one into 60+ extension structs spreaded in 3 arrays
> (riscv_cpu_extensions, riscv_cpu_experimental_exts,
> riscv_cpu_vendor_exts).
>
> We'll still need an extension struct but we won't be using the
> 'user_set' flag:
>
> - 'RISCVCPUMultiExtConfig' will be our specialized structure, similar to what
> we're already doing with the MISA extensions in 'RISCVCPUMisaExtConfig'.
> DEFINE_PROP_BOOL() for all 3 extensions arrays were replaced by
> MULTI_EXT_CFG_BOOL(), a macro that will init our specialized struct;
>
> - the 'multi_ext_user_opts' hash will be used to store the offset of each
> extension that the user set via the set() callback, cpu_set_multi_ext_cfg().
> For now we're just initializing and populating it - next patch will use
> it to determine if a certain extension was user set;
>
> - cpu_add_multi_ext_prop() is a new helper that will replace the
> qdev_property_add_static() calls that our macros are doing to populate
> user properties. The macro was renamed to ADD_CPU_MULTIEXT_PROPS_ARRAY()
> for clarity. Note that the non-extension properties in
> riscv_cpu_options[] still need to be declared via qdev().
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 231 -
>  1 file changed, 145 insertions(+), 86 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 88b263e830..b588f6969f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -153,6 +153,9 @@ static const struct isa_ext_data isa_edata_arr[] = {
>  ISA_EXT_DATA_ENTRY(xventanacondops, PRIV_VERSION_1_12_0, 
> ext_XVentanaCondOps),
>  };
>
> +/* Hash that stores user set extensions */
> +static GHashTable *multi_ext_user_opts;
> +
>  static bool isa_ext_is_enabled(RISCVCPU *cpu, uint32_t ext_offset)
>  {
>  bool *ext_enabled = (void *)&cpu->cfg + ext_offset;
> @@ -1668,6 +1671,8 @@ static void riscv_cpu_init(Object *obj)
>  qdev_init_gpio_in(DEVICE(cpu), riscv_cpu_set_irq,
>IRQ_LOCAL_MAX + IRQ_LOCAL_GUEST_MAX);
>  #endif /* CONFIG_USER_ONLY */
> +
> +multi_ext_user_opts = g_hash_table_new(NULL, g_direct_equal);
>  }
>
>  typedef struct RISCVCPUMisaExtConfig {
> @@ -1819,94 +1824,104 @@ static void riscv_cpu_add_misa_properties(Object 
> *cpu_obj)
>  }
>  }
>
> -static Property riscv_cpu_extensions[] = {
> +typedef struct RISCVCPUMultiExtConfig {
> +const char *name;
> +uint32_t offset;
> +bool enabled;
> +} RISCVCPUMultiExtConfig;
> +
> +#define MULTI_EXT_CFG_BOOL(_name, _prop, _defval) \
> +{.name = _name, .offset = CPU_CFG_OFFSET(_prop), \
> + .enabled = _defval}
> +
> +static RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>  /* Defaults for standard extensions */
> -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),
> -DEFINE_PROP_BOOL("Zihintpause", RISCVCPU, cfg.ext_zihintpause, true),
> -DEFINE_PROP_BOOL("Zawrs", RISCVCPU, cfg.ext_zawrs, true),
> -DEFINE_PROP_BOOL("Zfa", RISCVCPU, cfg.ext_zfa, true),
> -DEFINE_PROP_BOOL("Zfh", RISCVCPU, cfg.ext_zfh, false),
> -DEFINE_PROP_BOOL("Zfhmin", RISCVCPU, cfg.ext_zfhmin, false),
> -DEFINE_PROP_BOOL("Zve32f", RISCVCPU, cfg.ext_zve32f, false),
> -DEFINE_PROP_BOOL("Zve64f", RISCVCPU, cfg.ext_zve64f, false),
> -DEFINE_PROP_BOOL("Zve64d", RISCVCPU, cfg.ext_zve64d, false),
> -DEFINE_PROP_BOOL("sstc", RISCVCPU, cfg.ext_sstc, true),
> -
> -DEFINE_PROP_BOOL("smstateen", RISCVCPU, cfg.ext_smstateen, false),
> -DEFINE_PROP_BOOL("svadu", RISCVCPU, cfg.ext_svadu, true),
> -DEFINE_PROP_BOOL("svinval", RISCVCPU, cfg.ext_svinval, false),
> -DEFINE_PROP_BOOL("svnapot", RISCVCPU, cfg.ext_svnapot, false),
> -DEFINE_PROP_BOOL("svpbmt", RISCVCPU, cfg.ext_svpbmt, false),
> -
> -DEFINE_PROP_BOOL("zba", RISCVCPU, cfg.ext_zba, true),
> -DEFINE_PROP_BOOL("zbb", RISCVCPU, cfg.ext_zbb, true),
> -DEFINE_PROP_BOOL("zbc", RISCVCPU, cfg.ext_zbc, true),
> -DEFINE_PROP_BOOL("zbkb", RISCVCPU, cfg.ext_zbkb, false),
> -DEFINE_PROP_BOOL("zbkc", RISCVCPU, cfg.ext_zbkc, false),
> -DEFINE_PROP_BOOL("zbkx", RISCVCPU,

Re: [PATCH 6/8] target/riscv: use isa_ext_update_enabled() in init_max_cpu_extensions()

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:51 AM Daniel Henrique Barboza
 wrote:
>
> Before adding support to detect if an extension was user set we need to
> handle how we're enabling extensions in riscv_init_max_cpu_extensions().
> object_property_set_bool() calls the set() callback for the property,
> and we're going to use this callback to set the 'multi_ext_user_opts'
> hash.
>
> This means that, as is today, all extensions we're setting for the 'max'
> CPU will be seen as user set in the future. Let's change set_bool() to
> isa_ext_update_enabled() that will just enable/disable the flag on a
> certain offset.
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 19 +--
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b588f6969f..a40dc865a0 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -2096,25 +2096,24 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>  set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
>
>  for (int i = 0; i < ARRAY_SIZE(riscv_cpu_extensions); i++) {
> -object_property_set_bool(obj, riscv_cpu_extensions[i].name,
> - true, NULL);
> +isa_ext_update_enabled(cpu, riscv_cpu_extensions[i].offset, true);
>  }
>
>  /* set vector version */
>  env->vext_ver = VEXT_VERSION_1_00_0;
>
>  /* Zfinx is not compatible with F. Disable it */
> -object_property_set_bool(obj, "zfinx", false, NULL);
> -object_property_set_bool(obj, "zdinx", false, NULL);
> -object_property_set_bool(obj, "zhinx", false, NULL);
> -object_property_set_bool(obj, "zhinxmin", false, NULL);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zfinx), false);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zdinx), false);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinx), false);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zhinxmin), false);
>
> -object_property_set_bool(obj, "zce", false, NULL);
> -object_property_set_bool(obj, "zcmp", false, NULL);
> -object_property_set_bool(obj, "zcmt", false, NULL);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zce), false);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmp), false);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcmt), false);
>
>  if (env->misa_mxl != MXL_RV32) {
> -object_property_set_bool(obj, "zcf", false, NULL);
> +isa_ext_update_enabled(cpu, CPU_CFG_OFFSET(ext_zcf), false);
>  }
>  }
>
> --
> 2.41.0
>
>

Re: [PATCH 7/8] target/riscv/cpu.c: honor user choice in cpu_cfg_ext_auto_update()

[Alistair Francis]

On Fri, Jul 28, 2023 at 10:32 AM Daniel Henrique Barboza
 wrote:
>
> Add a new cpu_cfg_ext_is_user_set() helper to check if an extension was
> set by the user in the command line. Use it inside
> cpu_cfg_ext_auto_update() to verify if the user set a certain extension
> and, if that's the case, do not change its value.
>
> This will make us honor user choice instead of overwriting the values.
> Users will then be informed whether they're using an incompatible set of
> extensions instead of QEMU setting a magic value that works.
>
> For example, we'll now error out if the user explictly set 'zce' to true
> and 'zca' to false:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,zce=true,zca=false -nographic
> qemu-system-riscv64: Zcf/Zcd/Zcb/Zcmp/Zcmt extensions require Zca extension
>
> This didn't happen before because we were enabling 'zca' if 'zce' was enabled
> regardless if the user explictly set 'zca' to false.
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index a40dc865a0..644d0fdad2 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -187,6 +187,12 @@ static int cpu_cfg_ext_get_min_version(uint32_t 
> ext_offset)
>  return PRIV_VERSION_1_10_0;
>  }
>
> +static bool cpu_cfg_ext_is_user_set(uint32_t ext_offset)
> +{
> +return g_hash_table_contains(multi_ext_user_opts,
> + GUINT_TO_POINTER(ext_offset));
> +}
> +
>  static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, uint32_t ext_offset,
>  bool value)
>  {
> @@ -198,6 +204,10 @@ static void cpu_cfg_ext_auto_update(RISCVCPU *cpu, 
> uint32_t ext_offset,
>  return;
>  }
>
> +if (cpu_cfg_ext_is_user_set(ext_offset)) {
> +return;
> +}
> +
>  if (value && env->priv_ver != PRIV_VERSION_LATEST) {
>  /* Do not enable it if priv_ver is older than min_version */
>  min_version = cpu_cfg_ext_get_min_version(ext_offset);
> --
> 2.41.0
>
>

Re: [PATCH 8/8] target/riscv/cpu.c: consider user option with RVG

[Alistair Francis]

On Fri, Jul 28, 2023 at 9:39 AM Daniel Henrique Barboza
 wrote:
>
> Enabling RVG will enable a set of extensions that we're not checking if
> the user was okay enabling or not. And in this case we want to error
> out, instead of ignoring, otherwise we will be inconsistent enabling RVG
> without all its extensions.
>
> After this patch, disabling ifencei or icsr while enabling RVG will
> result in error:
>
> $ ./build/qemu-system-riscv64 -M virt -cpu rv64,g=true,Zifencei=false 
> --nographic
> qemu-system-riscv64: warning: Setting G will also set IMAFD_Zicsr_Zifencei
> qemu-system-riscv64: RVG requires Zifencei but user set Zifencei to false
>
> Signed-off-by: Daniel Henrique Barboza 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu.c | 18 --
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 644d0fdad2..72a36b47ed 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1135,8 +1135,22 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, 
> Error **errp)
>riscv_has_ext(env, RVD) &&
>cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
>  warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
> -cpu->cfg.ext_icsr = true;
> -cpu->cfg.ext_ifencei = true;
> +
> +if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_icsr)) &&
> +!cpu->cfg.ext_icsr) {
> +error_setg(errp, "RVG requires Zicsr but user set Zicsr to 
> false");
> +return;
> +}
> +
> +if (cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_ifencei)) &&
> +!cpu->cfg.ext_ifencei) {
> +error_setg(errp, "RVG requires Zifencei but user set "
> +   "Zifencei to false");
> +return;
> +}
> +
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_icsr), true);
> +cpu_cfg_ext_auto_update(cpu, CPU_CFG_OFFSET(ext_ifencei), true);
>
>  env->misa_ext |= RVI | RVM | RVA | RVF | RVD;
>  env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
> --
> 2.41.0
>
>

Re: [PATCH 20/24] tcg/i386: Add cf parameter to tcg_out_cmp

[Richard Henderson]

On 8/11/23 03:45, Peter Maydell wrote:

On Fri, 11 Aug 2023 at 11:26, Peter Maydell  wrote:


On Tue, 8 Aug 2023 at 04:13, Richard Henderson
 wrote:


Add the parameter to avoid TEST and pass along to tgen_arithi.
All current users pass false.

Signed-off-by: Richard Henderson 
---
  tcg/i386/tcg-target.c.inc | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
index b88fc14afd..56549ff2a0 100644
--- a/tcg/i386/tcg-target.c.inc
+++ b/tcg/i386/tcg-target.c.inc
@@ -1418,15 +1418,15 @@ static void tcg_out_jxx(TCGContext *s, int opc, 
TCGLabel *l, bool small)
  }
  }

-static void tcg_out_cmp(TCGContext *s, TCGArg arg1, TCGArg arg2,
-int const_arg2, int rexw)
+static void tcg_out_cmp(TCGContext *s, int rexw, TCGArg arg1, TCGArg arg2,
+int const_arg2, bool cf)
  {
  if (const_arg2) {
-if (arg2 == 0) {
+if (arg2 == 0 && !cf) {
  /* test r, r */
  tcg_out_modrm(s, OPC_TESTL + rexw, arg1, arg1);
  } else {
-tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, 0);
+tgen_arithi(s, ARITH_CMP + rexw, arg1, arg2, cf);
  }
  } else {
  tgen_arithr(s, ARITH_CMP + rexw, arg1, arg2);


I don't really understand the motivation here.
Why are some uses of this function fine with using the TEST
insn, but some must avoid it? What does 'cf' stand for?
A comment would help here if there isn't a clearer argument
name available...


Looking at the following patch suggests perhaps:

/**
  * tcg_out_cmp: Emit a compare, setting the X, Y, Z flags accordingly.
  * @need_cf : true if the comparison must also set CF
  */

(fill in which XYZ flags you can rely on even if need_cf is false)


I can add that, yes.

Basically, test sets SZ flags, where cmp sets SZCO.  I want to add an optimizaton using C, 
so "cmp 0,x" should not be silently replaced by "test x,x".



r~

[PATCH v3 06/10] migration: Consolidate return path closing code

[Fabiano Rosas]

We'll start calling the await_return_path_close_on_source() function
from other parts of the code, so move all of the related checks and
tracepoints into it.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 195726eb4a..4edbee3a5d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2049,6 +2049,14 @@ static int open_return_path_on_source(MigrationState *ms,
 /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
 static int await_return_path_close_on_source(MigrationState *ms)
 {
+int ret;
+
+if (!ms->rp_state.rp_thread_created) {
+return 0;
+}
+
+trace_migration_return_path_end_before();
+
 /*
  * If this is a normal exit then the destination will send a SHUT
  * and the rp_thread will exit, however if there's an error we
@@ -2066,7 +2074,10 @@ static int 
await_return_path_close_on_source(MigrationState *ms)
 qemu_thread_join(&ms->rp_state.rp_thread);
 ms->rp_state.rp_thread_created = false;
 trace_await_return_path_close_on_source_close();
-return ms->rp_state.error;
+
+ret = ms->rp_state.error;
+trace_migration_return_path_end_after(ret);
+return ret;
 }
 
 static inline void
@@ -2362,20 +2373,8 @@ static void migration_completion(MigrationState *s)
 goto fail;
 }
 
-/*
- * If rp was opened we must clean up the thread before
- * cleaning everything else up (since if there are no failures
- * it will wait for the destination to send it's status in
- * a SHUT command).
- */
-if (s->rp_state.rp_thread_created) {
-int rp_error;
-trace_migration_return_path_end_before();
-rp_error = await_return_path_close_on_source(s);
-trace_migration_return_path_end_after(rp_error);
-if (rp_error) {
-goto fail;
-}
+if (await_return_path_close_on_source(s)) {
+goto fail;
 }
 
 if (qemu_file_get_error(s->to_dst_file)) {
-- 
2.35.3

[PATCH v3 07/10] migration: Replace the return path retry logic

[Fabiano Rosas]

Replace the return path retry logic with finishing and restarting the
thread. This fixes a race when resuming the migration that leads to a
segfault.

Currently when doing postcopy we consider that an IO error on the
return path file could be due to a network intermittency. We then keep
the thread alive but have it do cleanup of the 'from_dst_file' and
wait on the 'postcopy_pause_rp' semaphore. When the user issues a
migrate resume, a new return path is opened and the thread is allowed
to continue.

There's a race condition in the above mechanism. It is possible for
the new return path file to be setup *before* the cleanup code in the
return path thread has had a chance to run, leading to the *new* file
being closed and the pointer set to NULL. When the thread is released
after the resume, it tries to dereference 'from_dst_file' and crashes:

Thread 7 "return path" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd1dbf700 (LWP 9611)]
0x560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at 
../migration/qemu-file.c:154
154 return f->last_error;

(gdb) bt
 #0  0x560e4893 in qemu_file_get_error_obj (f=0x0, errp=0x0) at 
../migration/qemu-file.c:154
 #1  0x560e4983 in qemu_file_get_error (f=0x0) at 
../migration/qemu-file.c:206
 #2  0x55b9a1df in source_return_path_thread (opaque=0x56e06000) at 
../migration/migration.c:1876
 #3  0x5602e14f in qemu_thread_start (args=0x5782e780) at 
../util/qemu-thread-posix.c:541
 #4  0x738d76ea in start_thread (arg=0x7fffd1dbf700) at 
pthread_create.c:477
 #5  0x735efa6f in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Here's the race (important bit is open_return_path happening before
migration_release_dst_files):

migration | qmp | return path
--+-+-
qmp_migrate_pause()
 shutdown(ms->to_dst_file)
  f->last_error = -EIO
migrate_detect_error()
 postcopy_pause()
  set_state(PAUSED)
  wait(postcopy_pause_sem)
qmp_migrate(resume)
migrate_fd_connect()
 resume = state == PAUSED
 open_return_path <-- TOO SOON!
 set_state(RECOVER)
 post(postcopy_pause_sem)
(incoming closes 
to_src_file)
res = 
qemu_file_get_error(rp)

migration_release_dst_files()

ms->rp_state.from_dst_file = NULL
  post(postcopy_pause_rp_sem)

postcopy_pause_return_path_thread()
  
wait(postcopy_pause_rp_sem)
rp = 
ms->rp_state.from_dst_file
goto retry
qemu_file_get_error(rp)
SIGSEGV
---

We can keep the retry logic without having the thread alive and
waiting. The only piece of data used by it is the 'from_dst_file' and
it is only allowed to proceed after a migrate resume is issued and the
semaphore released at migrate_fd_connect().

Move the retry logic to outside the thread by waiting for the thread
to finish before pausing the migration.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 60 ---
 migration/migration.h |  1 -
 2 files changed, 11 insertions(+), 50 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4edbee3a5d..7dfcbc3634 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1775,18 +1775,6 @@ static void migrate_handle_rp_req_pages(MigrationState 
*ms, const char* rbname,
 }
 }
 
-/* Return true to retry, false to quit */
-static bool postcopy_pause_return_path_thread(MigrationState *s)
-{
-trace_postcopy_pause_return_path();
-
-qemu_sem_wait(&s->postcopy_pause_rp_sem);
-
-trace_postcopy_pause_return_path_continued();
-
-return true;
-}
-
 static int migrate_handle_rp_recv_bitmap(MigrationState *s, char *block_name)
 {
 RAMBlock *block = qemu_ram_block_by_name(block_name);
@@ -1870,7 +1858,6 @@ static void *source_return_path_thread(void *opaque)
 trace_source_return_path_thread_entry();
 rcu_register_thread();
 
-retry:
 while (!ms->rp_state.error && !qemu_file_get_error(rp) &&
migration_is_setup_or_active(ms->state)) {
 trace_source_retur

[PATCH v3 01/10] migration: Fix possible race when setting rp_state.error

[Fabiano Rosas]

We don't need to set the rp_state.error right after a shutdown because
qemu_file_shutdown() always sets the QEMUFile error, so the return
path thread would have seen it and set the rp error itself.

Setting the error outside of the thread is also racy because the
thread could clear it after we set it.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5528acb65e..f88c86079c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2062,7 +2062,6 @@ static int 
await_return_path_close_on_source(MigrationState *ms)
  * waiting for the destination.
  */
 qemu_file_shutdown(ms->rp_state.from_dst_file);
-mark_source_rp_bad(ms);
 }
 trace_await_return_path_close_on_source_joining();
 qemu_thread_join(&ms->rp_state.rp_thread);
-- 
2.35.3

[PATCH v3 00/10] Fix segfault on migration return path

[Fabiano Rosas]

I decided to fix the issues with the shutdown instead of complaining
about them. First 5 patches address all of the possible races I
found. The only problem left to figure out is the -EIO on shutdown
which will need more thought.

Patches 6 & 7 fix the original segfault.

Patches 8-10 make the cleanup of migration files more predictable and
centralized.

I also adjusted some small things that were mentioned by Peter:

- moved the rp.error = false outside of the thread;
- stopped checking for errors during postcopy_pause();
- dropped the tracepoint;

CI run: https://gitlab.com/farosas/qemu/-/pipelines/963407228

v2:
https://lore.kernel.org/r/20230802143644.7534-1-faro...@suse.de

- moved the await into postcopy_pause() as Peter suggested;

- brought back the mark_source_rp_bad call. Turns out that piece of
code is filled with nuance. I just moved it aside since it doesn't
make sense during pause/resume. We can tackle that when we get the
chance.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/953420150
Also ran the switchover and preempt tests for 1000 times each on
x86_64.

v1:
https://lore.kernel.org/r/20230728121516.16258-1-faro...@suse.de

The /x86_64/migration/postcopy/preempt/recovery/plain test is
sometimes failing due a segmentation fault on the migration return
path. There is a race involving the retry logic of the return path and
the migration resume command.

The issue happens when the retry logic tries to cleanup the current
return path file, but ends up cleaning the new one and trying to use
it right after. Tracing shows it clearly:

open_return_path_on_source  <-- at migration start
open_return_path_on_source_continue <-- rp thread created
postcopy_pause_incoming
postcopy_pause_fast_load
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused. 
(incoming)
postcopy_pause_fault_thread
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused. (source)
postcopy_pause_incoming_continued
open_return_path_on_source   <-- NOK, too soon
postcopy_pause_continued
postcopy_pause_return_path   <-- too late, already operating on the new 
from_dst_file
postcopy_pause_return_path_continued <-- will continue and crash
postcopy_pause_incoming
qemu-system-x86_64: Detected IO failure for postcopy. Migration paused.
postcopy_pause_incoming_continued

We could solve this by adding some form of synchronization to ensure
that we always do the cleanup before setting up the new file, but I
find it more straight-forward to move the retry logic outside of the
thread by letting it finish and starting a new thread when resuming
the migration.

More details on the commit message.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/947875609

Fabiano Rosas (10):
  migration: Fix possible race when setting rp_state.error
  migration: Fix possible race when shutting return path
  migration: Fix possible race when checking to_dst_file for errors
  migration: Fix possible race when shutting down to_dst_file
  migration: Remove redundant cleanup of postcopy_qemufile_src
  migration: Consolidate return path closing code
  migration: Replace the return path retry logic
  migration: Move return path cleanup to main migration thread
  migration: Be consistent about shutdown of source shared files
  migration: Add a wrapper to cleanup migration files

 migration/migration.c | 228 +++---
 migration/migration.h |   1 -
 util/yank.c   |   2 -
 3 files changed, 79 insertions(+), 152 deletions(-)

-- 
2.35.3

[PATCH v3 04/10] migration: Fix possible race when shutting down to_dst_file

[Fabiano Rosas]

It's not safe to call qemu_file_shutdown() on the to_dst_file without
first checking for the file's presence under the lock. The cleanup of
this file happens at postcopy_pause() and migrate_fd_cleanup() which
are not necessarily running in the same thread as migrate_fd_cancel().

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 18 +++---
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 85c171f32c..5e6a766235 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1245,7 +1245,7 @@ static void migrate_fd_error(MigrationState *s, const 
Error *error)
 static void migrate_fd_cancel(MigrationState *s)
 {
 int old_state ;
-QEMUFile *f = migrate_get_current()->to_dst_file;
+
 trace_migrate_fd_cancel();
 
 WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
@@ -1271,11 +1271,13 @@ static void migrate_fd_cancel(MigrationState *s)
  * If we're unlucky the migration code might be stuck somewhere in a
  * send/write while the network has failed and is waiting to timeout;
  * if we've got shutdown(2) available then we can force it to quit.
- * The outgoing qemu file gets closed in migrate_fd_cleanup that is
- * called in a bh, so there is no race against this cancel.
  */
-if (s->state == MIGRATION_STATUS_CANCELLING && f) {
-qemu_file_shutdown(f);
+if (s->state == MIGRATION_STATUS_CANCELLING) {
+WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
+if (s->to_dst_file) {
+qemu_file_shutdown(s->to_dst_file);
+}
+}
 }
 if (s->state == MIGRATION_STATUS_CANCELLING && s->block_inactive) {
 Error *local_err = NULL;
@@ -1519,12 +1521,14 @@ void qmp_migrate_pause(Error **errp)
 {
 MigrationState *ms = migrate_get_current();
 MigrationIncomingState *mis = migration_incoming_get_current();
-int ret;
+int ret = 0;
 
 if (ms->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
 /* Source side, during postcopy */
 qemu_mutex_lock(&ms->qemu_file_lock);
-ret = qemu_file_shutdown(ms->to_dst_file);
+if (ms->to_dst_file) {
+ret = qemu_file_shutdown(ms->to_dst_file);
+}
 qemu_mutex_unlock(&ms->qemu_file_lock);
 if (ret) {
 error_setg(errp, "Failed to pause source migration");
-- 
2.35.3

[PATCH v3 02/10] migration: Fix possible race when shutting return path

[Fabiano Rosas]

We cannot call qemu_file_shutdown() on the return path file without
taking the file lock. The return path thread could be running it's
cleanup code and have just cleared the pointer.

This was caught by inspection, it should be rare, but the next patches
will start calling this code from other places, so let's do the
correct thing.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index f88c86079c..0067c927fa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2052,17 +2052,19 @@ static int open_return_path_on_source(MigrationState 
*ms,
 static int await_return_path_close_on_source(MigrationState *ms)
 {
 /*
- * If this is a normal exit then the destination will send a SHUT and the
- * rp_thread will exit, however if there's an error we need to cause
- * it to exit.
+ * If this is a normal exit then the destination will send a SHUT
+ * and the rp_thread will exit, however if there's an error we
+ * need to cause it to exit. shutdown(2), if we have it, will
+ * cause it to unblock if it's stuck waiting for the destination.
  */
-if (qemu_file_get_error(ms->to_dst_file) && ms->rp_state.from_dst_file) {
-/*
- * shutdown(2), if we have it, will cause it to unblock if it's stuck
- * waiting for the destination.
- */
-qemu_file_shutdown(ms->rp_state.from_dst_file);
+if (qemu_file_get_error(ms->to_dst_file)) {
+WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+if (ms->rp_state.from_dst_file) {
+qemu_file_shutdown(ms->rp_state.from_dst_file);
+}
+}
 }
+
 trace_await_return_path_close_on_source_joining();
 qemu_thread_join(&ms->rp_state.rp_thread);
 ms->rp_state.rp_thread_created = false;
-- 
2.35.3

[PATCH v3 03/10] migration: Fix possible race when checking to_dst_file for errors

[Fabiano Rosas]

Checking ms->to_dst_file for errors when cleaning up the return path
could race with migrate_fd_cleanup() which clears the pointer.

Since migrate_fd_cleanup() is reachable via qmp_migrate(), which is
issued by the user, it is safer if we take the lock when reading
ms->to_dst_file.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0067c927fa..85c171f32c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2057,11 +2057,10 @@ static int 
await_return_path_close_on_source(MigrationState *ms)
  * need to cause it to exit. shutdown(2), if we have it, will
  * cause it to unblock if it's stuck waiting for the destination.
  */
-if (qemu_file_get_error(ms->to_dst_file)) {
-WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
-if (ms->rp_state.from_dst_file) {
-qemu_file_shutdown(ms->rp_state.from_dst_file);
-}
+WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+if (ms->to_dst_file && ms->rp_state.from_dst_file &&
+qemu_file_get_error(ms->to_dst_file)) {
+qemu_file_shutdown(ms->rp_state.from_dst_file);
 }
 }
 
-- 
2.35.3

[PATCH v3 05/10] migration: Remove redundant cleanup of postcopy_qemufile_src

[Fabiano Rosas]

This file is owned by the return path thread which is already doing
cleanup.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 5e6a766235..195726eb4a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1177,12 +1177,6 @@ static void migrate_fd_cleanup(MigrationState *s)
 qemu_fclose(tmp);
 }
 
-if (s->postcopy_qemufile_src) {
-migration_ioc_unregister_yank_from_file(s->postcopy_qemufile_src);
-qemu_fclose(s->postcopy_qemufile_src);
-s->postcopy_qemufile_src = NULL;
-}
-
 assert(!migration_is_active(s));
 
 if (s->state == MIGRATION_STATUS_CANCELLING) {
-- 
2.35.3

[PATCH v3 08/10] migration: Move return path cleanup to main migration thread

[Fabiano Rosas]

Now that the return path thread is allowed to finish during a paused
migration, we can move the cleanup of the QEMUFiles to the main
migration thread.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7dfcbc3634..7fec57ad7f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -98,6 +98,7 @@ static int migration_maybe_pause(MigrationState *s,
  int *current_active_state,
  int new_state);
 static void migrate_fd_cancel(MigrationState *s);
+static int await_return_path_close_on_source(MigrationState *s);
 
 static bool migration_needs_multiple_sockets(void)
 {
@@ -1177,6 +1178,12 @@ static void migrate_fd_cleanup(MigrationState *s)
 qemu_fclose(tmp);
 }
 
+/*
+ * We already cleaned up to_dst_file, so errors from the return
+ * path might be due to that, ignore them.
+ */
+await_return_path_close_on_source(s);
+
 assert(!migration_is_active(s));
 
 if (s->state == MIGRATION_STATUS_CANCELLING) {
@@ -1985,7 +1992,6 @@ out:
 }
 
 trace_source_return_path_thread_end();
-migration_release_dst_files(ms);
 rcu_unregister_thread();
 return NULL;
 }
@@ -2039,6 +2045,9 @@ static int 
await_return_path_close_on_source(MigrationState *ms)
 
 ret = ms->rp_state.error;
 ms->rp_state.error = false;
+
+migration_release_dst_files(ms);
+
 trace_migration_return_path_end_after(ret);
 return ret;
 }
-- 
2.35.3

[PATCH v3 09/10] migration: Be consistent about shutdown of source shared files

[Fabiano Rosas]

When doing cleanup, we currently close() some of the shared migration
files and shutdown() + close() others. Be consistent by always calling
shutdown() before close().

Do this only for the source files for now because the source runs
multiple threads which could cause races between the two calls. Having
them together allows us to move them to a centralized place under the
protection of a lock the next patch.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 7fec57ad7f..4df5ca25c1 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1175,6 +1175,7 @@ static void migrate_fd_cleanup(MigrationState *s)
  * critical section won't block for long.
  */
 migration_ioc_unregister_yank_from_file(tmp);
+qemu_file_shutdown(tmp);
 qemu_fclose(tmp);
 }
 
@@ -1844,6 +1845,7 @@ static void migration_release_dst_files(MigrationState 
*ms)
 ms->postcopy_qemufile_src = NULL;
 }
 
+qemu_file_shutdown(file);
 qemu_fclose(file);
 }
 
-- 
2.35.3

[PATCH v3 10/10] migration: Add a wrapper to cleanup migration files

[Fabiano Rosas]

We currently have a pattern for cleaning up a migration QEMUFile:

  qemu_mutex_lock(&s->qemu_file_lock);
  file = s->file_name;
  s->file_name = NULL;
  qemu_mutex_unlock(&s->qemu_file_lock);

  migration_ioc_unregister_yank_from_file(file);
  qemu_file_shutdown(file);
  qemu_fclose(file);

There are some considerations for this sequence:

- we must clear the pointer under the lock, to avoid TOC/TOU bugs;
- the shutdown() and close() expect be given a non-null parameter;
- a close() in one thread should not race with a shutdown() in another;

Create a wrapper function to make sure everything works correctly.

Note: the return path did not used to call
  migration_ioc_unregister_yank_from_file(), but I added it
  nonetheless for uniformity.

Signed-off-by: Fabiano Rosas 
---
 migration/migration.c | 92 ---
 util/yank.c   |  2 -
 2 files changed, 26 insertions(+), 68 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4df5ca25c1..3c33e4fae4 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -217,6 +217,27 @@ MigrationIncomingState 
*migration_incoming_get_current(void)
 return current_incoming;
 }
 
+static void migration_file_release(QEMUFile **file)
+{
+MigrationState *ms = migrate_get_current();
+QEMUFile *tmp;
+
+/*
+ * Reset the pointer before releasing it to avoid holding the lock
+ * for too long.
+ */
+WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+tmp = *file;
+*file = NULL;
+}
+
+if (tmp) {
+migration_ioc_unregister_yank_from_file(tmp);
+qemu_file_shutdown(tmp);
+qemu_fclose(tmp);
+}
+}
+
 void migration_incoming_transport_cleanup(MigrationIncomingState *mis)
 {
 if (mis->socket_address_list) {
@@ -1155,8 +1176,6 @@ static void migrate_fd_cleanup(MigrationState *s)
 qemu_savevm_state_cleanup();
 
 if (s->to_dst_file) {
-QEMUFile *tmp;
-
 trace_migrate_fd_cleanup();
 qemu_mutex_unlock_iothread();
 if (s->migration_thread_running) {
@@ -1166,17 +1185,7 @@ static void migrate_fd_cleanup(MigrationState *s)
 qemu_mutex_lock_iothread();
 
 multifd_save_cleanup();
-qemu_mutex_lock(&s->qemu_file_lock);
-tmp = s->to_dst_file;
-s->to_dst_file = NULL;
-qemu_mutex_unlock(&s->qemu_file_lock);
-/*
- * Close the file handle without the lock to make sure the
- * critical section won't block for long.
- */
-migration_ioc_unregister_yank_from_file(tmp);
-qemu_file_shutdown(tmp);
-qemu_fclose(tmp);
+migration_file_release(&s->to_dst_file);
 }
 
 /*
@@ -1816,39 +1825,6 @@ static int migrate_handle_rp_resume_ack(MigrationState 
*s, uint32_t value)
 return 0;
 }
 
-/*
- * Release ms->rp_state.from_dst_file (and postcopy_qemufile_src if
- * existed) in a safe way.
- */
-static void migration_release_dst_files(MigrationState *ms)
-{
-QEMUFile *file;
-
-WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
-/*
- * Reset the from_dst_file pointer first before releasing it, as we
- * can't block within lock section
- */
-file = ms->rp_state.from_dst_file;
-ms->rp_state.from_dst_file = NULL;
-}
-
-/*
- * Do the same to postcopy fast path socket too if there is.  No
- * locking needed because this qemufile should only be managed by
- * return path thread.
- */
-if (ms->postcopy_qemufile_src) {
-migration_ioc_unregister_yank_from_file(ms->postcopy_qemufile_src);
-qemu_file_shutdown(ms->postcopy_qemufile_src);
-qemu_fclose(ms->postcopy_qemufile_src);
-ms->postcopy_qemufile_src = NULL;
-}
-
-qemu_file_shutdown(file);
-qemu_fclose(file);
-}
-
 /*
  * Handles messages sent on the return path towards the source VM
  *
@@ -2048,7 +2024,8 @@ static int 
await_return_path_close_on_source(MigrationState *ms)
 ret = ms->rp_state.error;
 ms->rp_state.error = false;
 
-migration_release_dst_files(ms);
+migration_file_release(&ms->rp_state.from_dst_file);
+migration_file_release(&ms->postcopy_qemufile_src);
 
 trace_migration_return_path_end_after(ret);
 return ret;
@@ -2504,26 +2481,9 @@ static MigThrError postcopy_pause(MigrationState *s)
 assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
 
 while (true) {
-QEMUFile *file;
-
-/*
- * Current channel is possibly broken. Release it.  Note that this is
- * guaranteed even without lock because to_dst_file should only be
- * modified by the migration thread.  That also guarantees that the
- * unregister of yank is safe too without the lock.  It should be safe
- * even to be within the qemu_file_lock, but we didn't do that to avoid
- * taking more mutex (yank_lock) within qemu_file_lock.  TL;DR: we make
- * the qemu_file_lock c

Re: [PATCH v5 08/11] target/loongarch: Reject la64-only instructions in la32 mode

[Richard Henderson]

On 8/11/23 01:12, gaosong wrote:

+TRANS_64(sra_d, gen_rrr, EXT_NONE, EXT_NONE, EXT_NONE, gen_sra_d)
  TRANS(rotr_w, gen_rrr, EXT_ZERO, EXT_NONE, EXT_SIGN, gen_rotr_w)

TRANS_64(rotr_w, ...)

...

  TRANS(rotri_w, gen_rri_v, EXT_NONE, EXT_NONE, gen_rotr_w)

TRANS_64(rotri_w, ...)

I see the manual from https://www.loongson.cn/download/index

insn cpucfg also not support on la32.



I see all 3 of these, ROTR.W, ROTRI.W and CPUCFG listed in Table 2 at

https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#overview-of-basic-integer-instructions


r~

Re: [PATCH v2 2/8] target/loongarch: Add a check parameter to the TRANS macro

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

The default check parmeter is ALL, remove TRANS_64 marco.

Suggested-by: Richard Henderson 
Signed-off-by: Song Gao 


If you're going to remove TRANS_64, you should simply drop the patch that added it, and be 
careful about the final patch ordering such that the LA32 cpu is created after your patch 
3 (which adds avail_64()).


Otherwise there will be a bisection point at which LA32 is enabled, but avail_64 is not 
enforced.



r~

Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand]

On 11.08.23 16:59, David Hildenbrand wrote:

On 10.08.23 23:24, Peter Xu wrote:

On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:

I think we have the following options (there might be more)

1) This patch.

2) New flag for memory-backend-file. We already have "readonly" and
"share=". I'm having a hard time coming up with a good name that really
describes the subtle difference.

3) Glue behavior to the QEMU machine



4) '-deny-private-discard' argv, or environment variable, or both


I'd personally vote for (2).  How about "fdperm"?  To describe when we want
to use different rw permissions on the file (besides the access permission
of the memory we already provided with "readonly"=XXX).  IIUC the only sane
value will be ro/rw/default, where "default" should just use the same rw
permission as the memory ("readonly"=XXX).


Hmm, I'm not particularly happy about that.



Would that be relatively clean and also work in this use case?



I get the feeling that we are over-engineering something that probably
should never have been allowed: MAP_PRIVATE mapping of a file and
opening it rw because someone might punch holes into it.

Once we start adding new parameters just for that, I get a bit skeptical
that this is what we want. The number of people that care about that are
probably close to 0.

The only real use case where this used to make sense (by accident I
assume) was with hugetlb. And somehow, we decided that it was a good
idea for "-mem-path" to use MAP_PRIVATE.

So, what stops us from

a) Leaving -mem-path alone. Keep opening files rw.
b) Make memory-backend-file with shared=off,readonly=off open the file
 read-only
c) Gluing that behavior to a QEMU compat machine

fallocate(PUNCH_HOLE) will fail, and we can probably let
virtio-mem/virtio-balloon and postcopy refuse to even start (virtio-mem
already does that) as early as possible.

People that care about any such use case would already get a warning
when punching a hole today.

If we ever support discarding RAM in that configuration, we can simply
unlock it again.

Am I missing any important use case?



I just started looking into the origins of "-mem-path".

Originally c902760fb2 ("Add option to use file backed guest memory"):

* Without MAP_POPULATE support, we use MAP_PRIVATE
* With MAP_POPULATE support we use MAP_PRIVATE if mem_prealloc was not
  defined.

It was only used for hugetlb. The shared memory case didn't really 
matter: they just needed a way to get hugetlb pages into the VM. Opening 
the file R/W even with MAP_PRIVATE kind-of made sense in that case, it 
was an exclusive owner.


Discarding of RAM was not very popular back then I guess: virtio-mem 
didn't exist, virtio-balloon doesn't even handle hugetlb today really, 
postcopy didn't exist.



I guess that's why nobody really cared about "-mem-path" MAP_PRIVATE vs. 
MAP_SHARED semantics: just get hugetlb pages into the VM somehow.


Nowadays, "-mem-path" always defaults to MAP_PRIVATE. For the original 
hugetlb use case, it's still good enough. For anything else, I'm not so 
sure.



--
Cheers,

David / dhildenb

[RFC PATCH 1/3] vdpa: Add SetSteeringEBPF method for NetClientState

[Hawkins Jiawei]

At present, to enable the VIRTIO_NET_F_RSS feature, eBPF must
be loaded for the vhost backend.

Given that vhost-vdpa is one of the vhost backend, we need to
implement the SetSteeringEBPF method to support RSS for vhost-vdpa,
even if vhost-vdpa calculates the rss hash in the hardware device
instead of in the kernel by eBPF.

Although this requires QEMU to be compiled with `--enable-bpf`
configuration even if the vdpa device does not use eBPF to
calculate the rss hash, this can avoid adding the specific
conditional statements for vDPA case to enable the VIRTIO_NET_F_RSS
feature, which reduces code maintainbility.

Suggested-by: Eugenio Pérez 
Signed-off-by: Hawkins Jiawei 
---
 net/vhost-vdpa.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index a13b267250..4c8e4b19f6 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -238,6 +238,12 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
 }
 }
 
+/** Dummy SetSteeringEBPF to support RSS for vhost-vdpa backend  */
+static bool vhost_vdpa_set_steering_ebpf(NetClientState *nc, int prog_fd)
+{
+return true;
+}
+
 static bool vhost_vdpa_has_vnet_hdr(NetClientState *nc)
 {
 assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
@@ -400,6 +406,7 @@ static NetClientInfo net_vhost_vdpa_info = {
 .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
 .has_ufo = vhost_vdpa_has_ufo,
 .check_peer_type = vhost_vdpa_check_peer_type,
+.set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
 };
 
 static int64_t vhost_vdpa_get_vring_group(int device_fd, unsigned vq_index,
@@ -1215,6 +1222,7 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
 .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
 .has_ufo = vhost_vdpa_has_ufo,
 .check_peer_type = vhost_vdpa_check_peer_type,
+.set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
 };
 
 /*
-- 
2.25.1

[RFC PATCH 2/3] vdpa: Restore receive-side scaling state

[Hawkins Jiawei]

This patch reuses vhost_vdpa_net_load_rss() with some
refactorings to restore the receive-side scaling state
at device's startup.

Signed-off-by: Hawkins Jiawei 
---
 net/vhost-vdpa.c | 53 
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4c8e4b19f6..7870cbe142 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -820,17 +820,28 @@ static int vhost_vdpa_net_load_rss(VhostVDPAState *s, 
const VirtIONet *n,
 }
 
 cfg.hash_types = cpu_to_le32(n->rss_data.hash_types);
-/*
- * According to VirtIO standard, "Field reserved MUST contain zeroes.
- * It is defined to make the structure to match the layout of
- * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
- *
- * Therefore, we need to zero the fields in struct virtio_net_rss_config,
- * which corresponds the `reserved` field in
- * struct virtio_net_hash_config.
- */
-memset(&cfg.indirection_table_mask, 0,
-   sizeof_field(struct virtio_net_hash_config, reserved));
+if (do_rss) {
+/*
+ * According to VirtIO standard, "Number of entries in 
indirection_table
+ * is (indirection_table_mask + 1)".
+ */
+cfg.indirection_table_mask = cpu_to_le16(n->rss_data.indirections_len -
+ 1);
+cfg.unclassified_queue = cpu_to_le16(n->rss_data.default_queue);
+cfg.max_tx_vq = cpu_to_le16(n->curr_queue_pairs);
+} else {
+/*
+ * According to VirtIO standard, "Field reserved MUST contain zeroes.
+ * It is defined to make the structure to match the layout of
+ * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
+ *
+ * Therefore, we need to zero the fields in
+ * struct virtio_net_rss_config, which corresponds the `reserved` field
+ * in struct virtio_net_hash_config.
+ */
+memset(&cfg.indirection_table_mask, 0,
+   sizeof_field(struct virtio_net_hash_config, reserved));
+}
 /*
  * Consider that virtio_net_handle_rss() currently does not restore the
  * hash key length parsed from the CVQ command sent from the guest into
@@ -866,6 +877,7 @@ static int vhost_vdpa_net_load_rss(VhostVDPAState *s, const 
VirtIONet *n,
 
 r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
 VIRTIO_NET_CTRL_MQ,
+do_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG :
 VIRTIO_NET_CTRL_MQ_HASH_CONFIG,
 data, ARRAY_SIZE(data));
 if (unlikely(r < 0)) {
@@ -899,13 +911,18 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
 return r;
 }
 
-if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_HASH_REPORT)) {
-return 0;
-}
-
-r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor, false);
-if (unlikely(r < 0)) {
-return r;
+if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_RSS)) {
+/* Load the receive-side scaling state */
+r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor, true);
+if (unlikely(r < 0)) {
+return r;
+}
+} else if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_RSS)) {
+/* Load the hash calculation state */
+r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor, false);
+if (unlikely(r < 0)) {
+return r;
+}
 }
 
 return 0;
-- 
2.25.1

[RFC PATCH 0/3] Vhost-vdpa Shadow Virtqueue RSS Support

[Hawkins Jiawei]

This series enables shadowed CVQ to intercept RSS command
through shadowed CVQ, update the virtio NIC device model
so qemu send it in a migration, and the restore of that
RSS state in the destination.

Note that this patch should be based on
patch "Vhost-vdpa Shadow Virtqueue Hash calculation Support" at [1].

[1]. https://lore.kernel.org/all/cover.1691762906.git.yin31...@gmail.com/

TestStep

1. test the migration using vp-vdpa device
  - For L0 guest, boot QEMU with two virtio-net-pci net device with
`in-qemu` RSS, command line like:
-netdev tap,vhost=off...
-device virtio-net-pci,disable-legacy=on,disable-modern=off,
iommu_platform=on,mq=on,ctrl_vq=on,hash=on,rss=on,guest_announce=off,
indirect_desc=off,queue_reset=off,...

  - For L1 guest, apply the relative patch series and compile the
source code, start QEMU with two vdpa device with svq mode on,
enable the `ctrl_vq`, `mq`, `rss` features on, command line like:
  -netdev type=vhost-vdpa,x-svq=true,...
  -device virtio-net-pci,mq=on,guest_announce=off,ctrl_vq=on,
rss=on,...

  - For L2 source guest, run the following bash command:
```bash
#!/bin/sh

ethtool -K eth0 rxhash on
```

  - Execute the live migration in L2 source monitor

  - Result
* with this series, L2 QEMU can execute without
triggering any error or warning. L0 QEMU echo
"Can't load eBPF RSS - fallback to software RSS".

Hawkins Jiawei (3):
  vdpa: Add SetSteeringEBPF method for NetClientState
  vdpa: Restore receive-side scaling state
  vdpa: Allow VIRTIO_NET_F_RSS in SVQ

 net/vhost-vdpa.c | 62 ++--
 1 file changed, 44 insertions(+), 18 deletions(-)

-- 
2.25.1

[RFC PATCH 3/3] vdpa: Allow VIRTIO_NET_F_RSS in SVQ

[Hawkins Jiawei]

Enable SVQ with VIRTIO_NET_F_RSS feature.

Signed-off-by: Hawkins Jiawei 
---
 net/vhost-vdpa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 7870cbe142..eb08530396 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -119,6 +119,7 @@ static const uint64_t vdpa_svq_device_features =
 /* VHOST_F_LOG_ALL is exposed by SVQ */
 BIT_ULL(VHOST_F_LOG_ALL) |
 BIT_ULL(VIRTIO_NET_F_HASH_REPORT) |
+BIT_ULL(VIRTIO_NET_F_RSS) |
 BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
 BIT_ULL(VIRTIO_NET_F_STANDBY) |
 BIT_ULL(VIRTIO_NET_F_SPEED_DUPLEX);
-- 
2.25.1

Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[Peter Xu]

On Fri, Aug 11, 2023 at 04:59:56PM +0200, David Hildenbrand wrote:
> On 10.08.23 23:24, Peter Xu wrote:
> > On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:
> > > > I think we have the following options (there might be more)
> > > > 
> > > > 1) This patch.
> > > > 
> > > > 2) New flag for memory-backend-file. We already have "readonly" and
> > > > "share=". I'm having a hard time coming up with a good name that really
> > > > describes the subtle difference.
> > > > 
> > > > 3) Glue behavior to the QEMU machine
> > > > 
> > > 
> > > 4) '-deny-private-discard' argv, or environment variable, or both
> > 
> > I'd personally vote for (2).  How about "fdperm"?  To describe when we want
> > to use different rw permissions on the file (besides the access permission
> > of the memory we already provided with "readonly"=XXX).  IIUC the only sane
> > value will be ro/rw/default, where "default" should just use the same rw
> > permission as the memory ("readonly"=XXX).
> 
> Hmm, I'm not particularly happy about that.
> 
> > 
> > Would that be relatively clean and also work in this use case?
> > 
> 
> I get the feeling that we are over-engineering something that probably
> should never have been allowed: MAP_PRIVATE mapping of a file and opening it
> rw because someone might punch holes into it.
> 
> Once we start adding new parameters just for that, I get a bit skeptical
> that this is what we want. The number of people that care about that are
> probably close to 0.
> 
> The only real use case where this used to make sense (by accident I assume)
> was with hugetlb. And somehow, we decided that it was a good idea for
> "-mem-path" to use MAP_PRIVATE.
> 
> So, what stops us from
> 
> a) Leaving -mem-path alone. Keep opening files rw.
> b) Make memory-backend-file with shared=off,readonly=off open the file
>read-only
> c) Gluing that behavior to a QEMU compat machine

So we want to make all users with shared=off + readonly=off to only open
the file RO always, failing file write ops rather than crashing others.

Sounds reasonable to me.

In that case, do we even need a compat bit, if we're 100% sure it won't
break anyone but only help, with the fact that everything should be
transparent?

-- 
Peter Xu

Re: [PATCH v2 3/8] target/loongarch: Add avail_64 to check la64-only instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

The la32 manual from [1], and it is not the final version.

[1]: 
https://www.loongson.cn/uploads/images/2023041918122813624.%E9%BE%99%E8%8A%AF%E6%9E%B6%E6%9E%8432%E4%BD%8D%E7%B2%BE%E7%AE%80%E7%89%88%E5%8F%82%E8%80%83%E6%89%8B%E5%86%8C_r1p03.pdf


I really hope this manual will be changed before final.



-TRANS(pcaddi, ALL, gen_pc, gen_pcaddi)
-TRANS(pcalau12i, ALL, gen_pc, gen_pcalau12i)
+TRANS(pcaddi, 64, gen_pc, gen_pcaddi)
+TRANS(pcalau12i, 64, gen_pc, gen_pcalau12i)
  TRANS(pcaddu12i, ALL, gen_pc, gen_pcaddu12i)
-TRANS(pcaddu18i, ALL, gen_pc, gen_pcaddu18i)
+TRANS(pcaddu18i, 64, gen_pc, gen_pcaddu18i)


For the compiler, PCALAU12I is much more useful than PCADDU12I.

Because PCALAU12I produces zeros in the lower 12 bits, the high-part pc-relative 
relocation does not need to be paired with a corresponding low-part pc-relative relocation.


Whereas PCADDU12I produces a full 32-bit result, and the low-part pc-relative relocation 
needs to know where the high-part was produced in order to compensate.  This fundamental 
error was made by the RISC-V ISA, and their toolchain is still paying the price.




@@ -69,6 +77,10 @@ static bool trans_cpucfg(DisasContext *ctx, arg_cpucfg *a)
  TCGv dest = gpr_dst(ctx, a->rd, EXT_NONE);
  TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
  
+if (!avail_64(ctx)) {

+return false;
+}


For the operating system running on LA32, lack of CPUCFG means that you now have to 
provide the cpu configuration in another way:


(1) Via compilation options, such that one operating system build will only run on a 
single cpu.


(2) Via external data, like device tree.

Either option complicates the usage of LA32.

I would hope that a few words of rom for CPUCFG to read is not too expensive to 
incorporate in even the smallest cpu implementation.



r~

[PATCH for-8.1 1/1] hw/riscv/virt.c: change 'aclint' TCG check

[Daniel Henrique Barboza]

The 'aclint' property is being conditioned with tcg acceleration in
virt_machine_class_init(). But acceleration code starts later than the
class init of the board, meaning that tcg_enabled() will be always be
false during class_init(), and the option is never being declared even
when declaring TCG accel:

$ ./build/qemu-system-riscv64 -M virt,accel=tcg,aclint=on
qemu-system-riscv64: Property 'virt-machine.aclint' not found

Fix it by moving the check from class_init() to machine_init(). Tune the
description to mention that the option is TCG only.

Cc: Philippe Mathieu-Daudé 
Fixes: c0716c81b ("hw/riscv/virt: Restrict ACLINT to TCG")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1823
Signed-off-by: Daniel Henrique Barboza 
---
 hw/riscv/virt.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d90286dc46..99c4e6314b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1350,6 +1350,11 @@ static void virt_machine_init(MachineState *machine)
 exit(1);
 }
 
+if (!tcg_enabled() && s->have_aclint) {
+error_report("'aclint' is only available with TCG acceleration");
+exit(1);
+}
+
 /* Initialize sockets */
 mmio_irqchip = virtio_irqchip = pcie_irqchip = NULL;
 for (i = 0; i < socket_count; i++) {
@@ -1683,13 +1688,14 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
 machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
 #endif
 
-if (tcg_enabled()) {
-object_class_property_add_bool(oc, "aclint", virt_get_aclint,
-   virt_set_aclint);
-object_class_property_set_description(oc, "aclint",
-  "Set on/off to enable/disable "
-  "emulating ACLINT devices");
-}
+
+object_class_property_add_bool(oc, "aclint", virt_get_aclint,
+   virt_set_aclint);
+object_class_property_set_description(oc, "aclint",
+  "(TCG only) Set on/off to "
+  "enable/disable emulating "
+  "ACLINT devices");
+
 object_class_property_add_str(oc, "aia", virt_get_aia,
   virt_set_aia);
 object_class_property_set_description(oc, "aia",
-- 
2.41.0

[PATCH for-8.1 0/1] hw/riscv/virt.c: fix 'aclint' prop regression

[Daniel Henrique Barboza]

Richard, Alistair,

I came across this gitlab bug earlier today. The bug itself was opened
yesterday:

https://gitlab.com/qemu-project/qemu/-/issues/1823

And turns out that this is a regression in the 'aclint' option that was
introduced in 8.1.

I'm aware that we're already in rc3 and kind of late, but even so I
marked this patch as 8.1 to let you decide whether it's worth spinning
rc4 or not.

Daniel Henrique Barboza (1):
  hw/riscv/virt.c: change 'aclint' TCG check

 hw/riscv/virt.c | 20 +---
 1 file changed, 13 insertions(+), 7 deletions(-)

-- 
2.41.0

Re: [RFC v1 0/3] Initial support for SPDM

[Alistair Francis]

On Thu, Aug 10, 2023 at 6:18 AM Jonathan Cameron
 wrote:
>
> On Wed, 9 Aug 2023 12:45:35 -0400
> Alistair Francis  wrote:
>
> > On Wed, Aug 9, 2023 at 8:11 AM Jonathan Cameron
> >  wrote:
> > >
> > > On Tue,  8 Aug 2023 11:51:21 -0400
> > > Alistair Francis  wrote:
> > >
> > > > The Security Protocol and Data Model (SPDM) Specification defines
> > > > messages, data objects, and sequences for performing message exchanges
> > > > over a variety of transport and physical media.
> > > >  - 
> > > > https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.3.0.pdf
> > > >
> > > > This series is a very initial start on adding SPDM support to QEMU.
> > > >
> > > > This series uses libspdm [1] which is a reference implementation of
> > > > SPDM.
> > > >
> > > > The series starts by adding support for building and linking with
> > > > libspdm. It then progresses to handling SPDM requests in the NVMe device
> > > > over the PCIe DOE protocol.
> > > >
> > > > This is a very early attempt. The code quality is not super high, the C
> > > > code hasn't been tested at all. This is really just an RFC to see if
> > > > QEMU will accept linking with libspdm.
> > > >
> > > > So, the main question is, how do people feel about linking with libspdm
> > > > to support SPDM?
> > > >
> > > > 1: https://github.com/DMTF/libspdm
> > >
> > > Hi Alastair,
> > >
> > > For reference / background we've had SPDM support for CXL emulated devices
> > > in our staging tree for quite a while - it's not upstream because of
> > > exactly this question (+ no one had upstreaming this as a high priority
> > > as out of tree was fine for developing the kernel stack - it's well
> > > isolated in the code so there was little cost in rebasing this feature)
> > > - and because libspdm is packaged by almost no one. There were problems
> > > building it with external crypto libraries etc.
> >
> > Thanks for pointing that out! I didn't realise there was existing QEMU
> > work. I'm glad others are looking into it
> >
> > Building with libspdm is difficult. Even this series does have weird
> > issues with openssl's crypto library. I have been working towards
> > packaging libspdm into buildroot, which has helped cleanup libspdm to
> > be more user friendly. libspdm 3.0 for example is now a lot easier to
> > build/package, but I expect to continue to improve things.
> >
> > There will need to be more improvements to libspdm before this series
> > could be merged.
> >
> > >
> > > Looks like you have had a lot more success than I ever did in getting that
> > > to work. I tried a few times in the past and ended up sticking with
> > > the Avery design folks approach of a socket connection to spdm-emu
> > > Given you cc'd them I'm guessing you came across that work which is what
> > > we used for testing the kernel code Lukas is working on currently.
> > >
> > > https://lore.kernel.org/qemu-devel/20210629132520.0...@huawei.com/
> > >
> > > https://gitlab.com/jic23/qemu/-/commit/9864fb29979e55c1e37c20edf00907d9524036e8
> >
> > Thanks for that!
> >
> > In the past the QEMU community has refused to accept upstream changes
> > that expose QEMU internals via sockets. So I suspect linking with
> > libspdm will be a more upstreamable approach.
> >
> > On top of that requiring user to run an external socket application is 
> > clunky.
> >
> > >
> > > So I think we have 2 choices here.
> > > 1) Do what you have done and build the library as you are doing.
> > >  - Can fix a version - so don't care if they change the ABI etc other
> > >than needing to move the version QEMU uses forwards when we need
> > >to for new features.
> >
> > I agree
> >
> > >  - Cert management is going to add a lot of complexity into QEMU.
> > >I've not looked at how much infrastructure we can reuse from elsewhere.
> > >Maybe this is a solved problem.
> >
> > Could we not just point to a cert when running QEMU?
>
> Yes, but it's going to be multiple cert trees per PCI device with all the
> association with particular SPDM instances etc. Not too bad I guess.
>
> >
> > >
> > > 2) Keep the SPDM emulation external.
> > >  - I'm not sure the socket protocol used by spdm-emu is fixed in stone
> > >as people tend to change both ends.
> > >  - Cert management and protocol options etc are already available.
> >
> > I suspect this will never get upstream though. My aim is to get
> > something upstream, so this is probably a no go (unless someone jumps
> > in and says this is ok).
>
> There is precedence with the TPM stuff using swtpm as the provider.
> https://qemu.readthedocs.io/en/latest/specs/tpm.html#the-qemu-tpm-emulator-device

Good point. I forgot about the TPM.

I have CCed Stefan in case they have any comments

>
> >
> > >
> > > Personally I prefer the control option 1 gives us, even if it's a lot more
> > > code.  Option 2 also rather limits our ability to do anything with
> > > the encrypted data as well. It's fine if the aim is just verification
> > > of simp

Re: [PATCH v2 4/8] target/loongarch: Add avail_FP/FP_SP/FP_DP to check fpu instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

Signed-off-by: Song Gao
---
  .../loongarch/insn_trans/trans_farith.c.inc   | 96 ---
  target/loongarch/insn_trans/trans_fcmp.c.inc  |  8 ++
  target/loongarch/insn_trans/trans_fcnv.c.inc  | 56 +--
  .../loongarch/insn_trans/trans_fmemory.c.inc  | 32 +++
  target/loongarch/insn_trans/trans_fmov.c.inc  | 48 --
  target/loongarch/translate.h  |  3 +
  6 files changed, 157 insertions(+), 86 deletions(-)


Since the manual does not explicitly list "basic floating point" vs single vs double, I 
can only give you an


Acked-by: Richard Henderson 


r~

Re: [PATCH v2 5/8] target/loongarch: Add avail_LSPW to check LSPW instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

Signed-off-by: Song Gao 
---
  target/loongarch/insn_trans/trans_privileged.c.inc | 8 
  target/loongarch/translate.h   | 1 +
  2 files changed, 9 insertions(+)


Reviewed-by: Richard Henderson 


r~

Re: pci: Fix the update of interrupt disable bit in PCI_COMMAND register

[Michael S. Tsirkin]

On Fri, Aug 11, 2023 at 10:46:51PM +0800, Guoyi Tu wrote:
> The PCI_COMMAND register is located at offset 4 within
> the PCI configuration space and occupies 2 bytes. The
> interrupt disable bit is at the 10th bit, which corresponds
> to the byte at offset 5 in the PCI configuration space.
> 
> In our testing environment, the guest driver may directly
> updates the byte at offset 5 in the PCI configuration space.
> The backtrace looks like as following:
> #0  pci_default_write_config (d=0x5580bbfc6230, addr=5, val_in=5, l=1)
> at hw/pci/pci.c:1442
> #1  0x5580b8f3156a in virtio_write_config (pci_dev=0x5580bbfc6230,
> address=5, val=5, len=1)
> at hw/virtio/virtio-pci.c:605
> #2  0x5580b8ed2f3b in pci_host_config_write_common
> (pci_dev=0x5580bbfc6230, addr=5, limit=256,
> val=5, len=1) at hw/pci/pci_host.c:81
> 
> In this situation, the range_covers_byte function called
> by the pci_default_write_config function will return false,
> resulting in the inability to handle the interrupt disable
> update event.
> 
> To fix this issue, we can use the ranges_overlap function
> instead of range_covers_byte to determine whether the interrupt
> bit has been updated.
> 
> Signed-off-by: Guoyi Tu 
> Signed-off-by: yuanminghao 

Oh wow good catch!

Fixes: b6981cb57be5 ("pci: interrupt disable bit support")

clearly stable material too.


> ---
>  hw/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b8d22e2e74..881d774fb6 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1613,7 +1613,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t
> addr, uint32_t val_in, int
>  range_covers_byte(addr, l, PCI_COMMAND))
>  pci_update_mappings(d);
> 
> -if (range_covers_byte(addr, l, PCI_COMMAND)) {
> +if (ranges_overlap(addr, l, PCI_COMMAND, 2)) {
>  pci_update_irq_disabled(d, was_irq_disabled);
>  memory_region_set_enabled(&d->bus_master_enable_region,
>(pci_get_word(d->config + PCI_COMMAND)
> -- 
> 2.27.0
> 
> --
> Guoyi

Re: [PATCH v2 6/8] target/loongarch: Add avail_LAM to check atomic instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

Signed-off-by: Song Gao 
---
  target/loongarch/insn_trans/trans_atomic.c.inc | 12 
  target/loongarch/translate.h   |  1 +
  2 files changed, 13 insertions(+)

diff --git a/target/loongarch/insn_trans/trans_atomic.c.inc 
b/target/loongarch/insn_trans/trans_atomic.c.inc
index 194818d74d..867d09375a 100644
--- a/target/loongarch/insn_trans/trans_atomic.c.inc
+++ b/target/loongarch/insn_trans/trans_atomic.c.inc
@@ -9,6 +9,10 @@ static bool gen_ll(DisasContext *ctx, arg_rr_i *a, MemOp mop)
  TCGv src1 = gpr_src(ctx, a->rj, EXT_NONE);
  TCGv t0 = make_address_i(ctx, src1, a->imm);
  
+if (!avail_LAM(ctx)) {

+return true;
+}
+
  tcg_gen_qemu_ld_i64(dest, t0, ctx->mem_idx, mop);
  tcg_gen_st_tl(t0, cpu_env, offsetof(CPULoongArchState, lladdr));
  tcg_gen_st_tl(dest, cpu_env, offsetof(CPULoongArchState, llval));
@@ -25,6 +29,10 @@ static bool gen_sc(DisasContext *ctx, arg_rr_i *a, MemOp mop)
  TCGv t0 = tcg_temp_new();
  TCGv val = tcg_temp_new();
  
+if (!avail_LAM(ctx)) {

+return true;
+}
+
  TCGLabel *l1 = gen_new_label();
  TCGLabel *done = gen_new_label();


I think these two are wrong.  LAM says "AM* atomic memory instructions".
I think LL/SC are always available.

  
@@ -53,6 +61,10 @@ static bool gen_am(DisasContext *ctx, arg_rrr *a,

  TCGv addr = gpr_src(ctx, a->rj, EXT_NONE);
  TCGv val = gpr_src(ctx, a->rk, EXT_NONE);
  
+if (!avail_LAM(ctx)) {

+return true;
+}


While correct, I think it would be better style to use LAM instead of ALL in the 
TRANS(am*) instructions.



r~

Re: [PATCH v2 7/8] target/loongarch: Add avail_LSX to check LSX instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

Signed-off-by: Song Gao 
---
  target/loongarch/insn_trans/trans_lsx.c.inc | 1482 ++-
  target/loongarch/translate.h|2 +
  2 files changed, 823 insertions(+), 661 deletions(-)



Reviewed-by: Richard Henderson 


r~

Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[Peter Xu]

On Fri, Aug 11, 2023 at 05:26:24PM +0200, David Hildenbrand wrote:
> I just started looking into the origins of "-mem-path".
> 
> Originally c902760fb2 ("Add option to use file backed guest memory"):
> 
> * Without MAP_POPULATE support, we use MAP_PRIVATE
> * With MAP_POPULATE support we use MAP_PRIVATE if mem_prealloc was not
>   defined.
> 
> It was only used for hugetlb. The shared memory case didn't really matter:
> they just needed a way to get hugetlb pages into the VM. Opening the file
> R/W even with MAP_PRIVATE kind-of made sense in that case, it was an
> exclusive owner.
> 
> Discarding of RAM was not very popular back then I guess: virtio-mem didn't
> exist, virtio-balloon doesn't even handle hugetlb today really, postcopy
> didn't exist.
> 
> 
> I guess that's why nobody really cared about "-mem-path" MAP_PRIVATE vs.
> MAP_SHARED semantics: just get hugetlb pages into the VM somehow.
> 
> Nowadays, "-mem-path" always defaults to MAP_PRIVATE. For the original
> hugetlb use case, it's still good enough. For anything else, I'm not so
> sure.

Ok this answers my other question then on the compat bit.. thanks.  Feel
free to ignore there.

But then I'd lean back towards simply adding a fdperm=; that seems the
simplest, since even if with a compat bit, we still face risk of breaking
-mem-path users for anyone using new machine types.

-- 
Peter Xu

Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand]

On 11.08.23 18:16, Peter Xu wrote:

On Fri, Aug 11, 2023 at 05:26:24PM +0200, David Hildenbrand wrote:

I just started looking into the origins of "-mem-path".

Originally c902760fb2 ("Add option to use file backed guest memory"):

* Without MAP_POPULATE support, we use MAP_PRIVATE
* With MAP_POPULATE support we use MAP_PRIVATE if mem_prealloc was not
   defined.

It was only used for hugetlb. The shared memory case didn't really matter:
they just needed a way to get hugetlb pages into the VM. Opening the file
R/W even with MAP_PRIVATE kind-of made sense in that case, it was an
exclusive owner.

Discarding of RAM was not very popular back then I guess: virtio-mem didn't
exist, virtio-balloon doesn't even handle hugetlb today really, postcopy
didn't exist.


I guess that's why nobody really cared about "-mem-path" MAP_PRIVATE vs.
MAP_SHARED semantics: just get hugetlb pages into the VM somehow.

Nowadays, "-mem-path" always defaults to MAP_PRIVATE. For the original
hugetlb use case, it's still good enough. For anything else, I'm not so
sure.


Ok this answers my other question then on the compat bit.. thanks.  Feel
free to ignore there.

But then I'd lean back towards simply adding a fdperm=; that seems the
simplest, since even if with a compat bit, we still face risk of breaking
-mem-path users for anyone using new machine types.


We wouldn't touch "-mem-path".

--
Cheers,

David / dhildenb

Re: [PATCH v2 8/8] target/loongarch: Add avail_IOCSR to check iocsr instructions

[Richard Henderson]

On 8/11/23 03:02, Song Gao wrote:

Signed-off-by: Song Gao
---
  .../loongarch/insn_trans/trans_privileged.c.inc  | 16 
  target/loongarch/translate.h |  2 +-
  2 files changed, 9 insertions(+), 9 deletions(-)


Reviewed-by: Richard Henderson 


r~

[PULL 0/2] pci: last minute bugfixes

[Michael S. Tsirkin]

The following changes since commit 15b11a1da6a4b7c6b8bb37883f52b544dee2b8fd:

  cryptodev: Handle unexpected request to avoid crash (2023-08-03 16:16:17 
-0400)

are available in the Git repository at:

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

for you to fetch changes up to 0f936247e8ed0ab5fb7e75827dd8c8f73d5ef4b5:

  pci: Fix the update of interrupt disable bit in PCI_COMMAND register 
(2023-08-11 12:15:24 -0400)


pci: last minute bugfixes

two fixes that seem very safe and important enough to sneak
in before the release.

Signed-off-by: Michael S. Tsirkin 


Guoyi Tu (1):
  pci: Fix the update of interrupt disable bit in PCI_COMMAND register

Jason Chien (1):
  hw/pci-host: Allow extended config space access for Designware PCIe host

 hw/pci-host/designware.c | 1 +
 hw/pci/pci.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

[PULL 1/2] hw/pci-host: Allow extended config space access for Designware PCIe host

[Michael S. Tsirkin]

From: Jason Chien 

In pcie_bus_realize(), a root bus is realized as a PCIe bus and a non-root
bus is realized as a PCIe bus if its parent bus is a PCIe bus. However,
the child bus "dw-pcie" is realized before the parent bus "pcie" which is
the root PCIe bus. Thus, the extended configuration space is not accessible
on "dw-pcie". The issue can be resolved by adding the
PCI_BUS_EXTENDED_CONFIG_SPACE flag to "pcie" before "dw-pcie" is realized.

Signed-off-by: Jason Chien 
Message-Id: <20230809102257.25121-1-jason.ch...@sifive.com>
Reviewed-by: Michael S. Tsirkin 
Signed-off-by: Michael S. Tsirkin 
Reviewed-by: Frank Chang 
Signed-off-by: Jason Chien jason.ch...@sifive.com>
---
 hw/pci-host/designware.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index 9e183caa48..388d252ee2 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -694,6 +694,7 @@ static void designware_pcie_host_realize(DeviceState *dev, 
Error **errp)
  &s->pci.io,
  0, 4,
  TYPE_PCIE_BUS);
+pci->bus->flags |= PCI_BUS_EXTENDED_CONFIG_SPACE;
 
 memory_region_init(&s->pci.address_space_root,
OBJECT(s),
-- 
MST