Re: [PATCH v4 0/3] NUMA: Apply cluster-NUMA-node boundary for aarch64 and riscv machines

[Gavin Shan]

On 4/12/23 7:42 PM, Peter Maydell wrote:

On Wed, 12 Apr 2023 at 02:08, Gavin Shan  wrote:

On 3/27/23 9:26 PM, Igor Mammedov wrote:

On Fri, 17 Mar 2023 14:25:39 +0800
Gavin Shan  wrote:


For arm64 and riscv architecture, the driver (/base/arch_topology.c) is
used to populate the CPU topology in the Linux guest. It's required that
the CPUs in one cluster can't span mutiple NUMA nodes. Otherwise, the Linux
scheduling domain can't be sorted out, as the following warning message
indicates. To avoid the unexpected confusion, this series attempts to
warn about such kind of irregular configurations.

 -smp 6,maxcpus=6,sockets=2,clusters=1,cores=3,threads=1 \
 -numa node,nodeid=0,cpus=0-1,memdev=ram0\
 -numa node,nodeid=1,cpus=2-3,memdev=ram1\
 -numa node,nodeid=2,cpus=4-5,memdev=ram2\

 [ cut here ]
 WARNING: CPU: 0 PID: 1 at kernel/sched/topology.c:2271 
build_sched_domains+0x284/0x910
 Modules linked in:
 CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-268.el9.aarch64 #1
 pstate: 0045 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : build_sched_domains+0x284/0x910
 lr : build_sched_domains+0x184/0x910
 sp : 8804bd50
 x29: 8804bd50 x28: 0002 x27: 
 x26: 89cf9a80 x25:  x24: 89cbf840
 x23: 80325000 x22: 005df800 x21: 8a4ce508
 x20:  x19: 80324440 x18: 0014
 x17: 388925c0 x16: 5386a066 x15: 9c10cc2e
 x14: 01c0 x13: 0001 x12: 7fffb1a0
 x11: 7fffb180 x10: 8a4ce508 x9 : 0041
 x8 : 8a4ce500 x7 : 8a4cf920 x6 : 0001
 x5 : 0001 x4 : 0007 x3 : 0002
 x2 : 1000 x1 : 8a4cf928 x0 : 0001
 Call trace:
  build_sched_domains+0x284/0x910
  sched_init_domains+0xac/0xe0
  sched_init_smp+0x48/0xc8
  kernel_init_freeable+0x140/0x1ac
  kernel_init+0x28/0x140
  ret_from_fork+0x10/0x20

PATCH[1] Warn about the irregular configuration if required
PATCH[2] Enable the validation for aarch64 machines
PATCH[3] Enable the validation for riscv machines

v3: https://lists.nongnu.org/archive/html/qemu-arm/2023-02/msg01226.html
v2: https://lists.nongnu.org/archive/html/qemu-arm/2023-02/msg01080.html
v1: https://lists.nongnu.org/archive/html/qemu-arm/2023-02/msg00886.html

Changelog
=
v4:
* Pick r-b and ack-b from Daniel/Philippe   (Gavin)
* Replace local variable @len with possible_cpus->len in
  validate_cpu_cluster_to_numa_boundary()   (Philippe)
v3:
* Validate cluster-to-NUMA instead of socket-to-NUMA
  boundary  (Gavin)
* Move the switch from MachineState to MachineClass (Philippe)
* Warning instead of rejecting the irregular configuration  (Daniel)
* Comments to mention cluster-to-NUMA is platform instead
  of architectural choice   (Drew)
* Drop PATCH[v2 1/4] related to qtests/numa-test(Gavin)
v2:
* Fix socket-NUMA-node boundary issues in qtests/numa-test  (Gavin)
* Add helper set_numa_socket_boundary() and validate the
  boundary in the generic path  (Philippe)

Gavin Shan (3):
numa: Validate cluster and NUMA node boundary if required
hw/arm: Validate cluster and NUMA node boundary
hw/riscv: Validate cluster and NUMA node boundary

   hw/arm/sbsa-ref.c   |  2 ++
   hw/arm/virt.c   |  2 ++
   hw/core/machine.c   | 42 ++
   hw/riscv/spike.c|  2 ++
   hw/riscv/virt.c |  2 ++
   include/hw/boards.h |  1 +
   6 files changed, 51 insertions(+)



Acked-by: Igor Mammedov 



Not sure if QEMU v8.0 is still available to integrate this series.
Otherwise, it should be something for QEMU v8.1. By the way, I'm
also uncertain who needs to be merge this series.


It barely touches arm specific boards, so I'm assuming it will
be reviewed and taken by whoever handles hw/core/machine.c

And yes, 8.0 is nearly out the door, this is 8.1 stuff.



Indeed. In this case, it needs to be merged via 'Machine core' tree,
which is being taken care by Eduardo Habkost or Marcel Apfelbaum.

Eduardo and  Marcel, could you please merge this to QEMU v8.1 when it's
ready? Thanks in advance.

Thanks,
Gavin

[PATCH] softmmu: Move dirtylimit.c into the target independent source set

[Thomas Huth]

dirtylimit.c just uses one TARGET_PAGE_SIZE macro - change it to
qemu_target_page_size() so we can move thefile into the target
independent source set. Then we only have to compile this file
once during the build instead of multiple times (one time for
each target).

Signed-off-by: Thomas Huth 
---
 softmmu/dirtylimit.c | 3 ++-
 softmmu/meson.build  | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
index c56f0f58c8..82986c1499 100644
--- a/softmmu/dirtylimit.c
+++ b/softmmu/dirtylimit.c
@@ -20,6 +20,7 @@
 #include "monitor/hmp.h"
 #include "monitor/monitor.h"
 #include "exec/memory.h"
+#include "exec/target_page.h"
 #include "hw/boards.h"
 #include "sysemu/kvm.h"
 #include "trace.h"
@@ -236,7 +237,7 @@ static inline int64_t 
dirtylimit_dirty_ring_full_time(uint64_t dirtyrate)
 static uint64_t max_dirtyrate;
 uint32_t dirty_ring_size = kvm_dirty_ring_size();
 uint64_t dirty_ring_size_meory_MB =
-dirty_ring_size * TARGET_PAGE_SIZE >> 20;
+dirty_ring_size * qemu_target_page_size() >> 20;
 
 if (max_dirtyrate < dirtyrate) {
 max_dirtyrate = dirtyrate;
diff --git a/softmmu/meson.build b/softmmu/meson.build
index b392f0bd35..974732b0f3 100644
--- a/softmmu/meson.build
+++ b/softmmu/meson.build
@@ -3,7 +3,6 @@ specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: [files(
   'ioport.c',
   'memory.c',
   'physmem.c',
-  'dirtylimit.c',
   'watchpoint.c',
 )])
 
@@ -18,6 +17,7 @@ softmmu_ss.add(files(
   'cpu-throttle.c',
   'cpu-timers.c',
   'datadir.c',
+  'dirtylimit.c',
   'dma-helpers.c',
   'globals.c',
   'memory_mapping.c',
-- 
2.31.1

Re: [PATCH v11] audio/pwaudio.c: Add Pipewire audio backend for QEMU

[Volker Rümelin]

Am 12.04.23 um 15:59 schrieb Dorinda Bassey:
> Hi Volker,
>
> It seems that for some unknown reason using audio_pcm_info_clear_buf in 
> playback_process causes segmentation fault. Hence I moved the handling of 
> buffer underruns from the playback process to the qpw_write process because 
> that is the underlying cause of buffer underrun.
>

Hi Dorinda,

I guess you made a mistake somewhere if you see a segmentation fault. This 
patch for v10 works fine on my computer.

diff --git a/audio/pwaudio.c b/audio/pwaudio.c
index f9da86059f..0f272d6744 100644
--- a/audio/pwaudio.c
+++ b/audio/pwaudio.c
@@ -79,7 +79,7 @@ stream_destroy(void *data)
 static void
 playback_on_process(void *data)
 {
-PWVoice *v = (PWVoice *) data;
+PWVoice *v = data;
 void *p;
 struct pw_buffer *b;
 struct spa_buffer *buf;
@@ -108,19 +108,28 @@ playback_on_process(void *data)
 n_bytes = SPA_MIN(req, buf->datas[0].maxsize);
 
 /* get no of available bytes to read data from buffer */
-
 avail = spa_ringbuffer_get_read_index(>ring, );
+if (avail <= 0) {
+PWVoiceOut *vo = container_of(data, PWVoiceOut, v);
 
-if (avail < (int32_t) n_bytes) {
-n_bytes = avail;
-}
+audio_pcm_info_clear_buf(>hw.info, p, n_bytes / v->frame_size);
+} else {
+if ((uint32_t)avail < n_bytes) {
+/*
+ * PipeWire immediately calls this callback again if we provide
+ * less than n_bytes. Then audio_pcm_info_clear_buf() fills the
+ * rest of the buffer with silence.
+ */
+n_bytes = avail;
+}
 
-spa_ringbuffer_read_data(>ring,
-v->buffer, RINGBUFFER_SIZE,
-index & RINGBUFFER_MASK, p, n_bytes);
+spa_ringbuffer_read_data(>ring,
+ v->buffer, RINGBUFFER_SIZE,
+ index & RINGBUFFER_MASK, p, n_bytes);
 
-index += n_bytes;
-spa_ringbuffer_read_update(>ring, index);
+index += n_bytes;
+spa_ringbuffer_read_update(>ring, index);
+}
 
 buf->datas[0].chunk->offset = 0;
 buf->datas[0].chunk->stride = v->frame_size;
-- 
2.35.3

With best regards,
Volker

virtio-iommu hotplug issue

[Akihiko Odaki]

Hi,

Recently I encountered a problem with the combination of Linux's 
virtio-iommu driver and QEMU when a SR-IOV virtual function gets 
disabled. I'd like to ask you what kind of solution is appropriate here 
and implement the solution if possible.


A PCIe device implementing the SR-IOV specification exports a virtual 
function, and the guest can enable or disable it at runtime by writing 
to a configuration register. This effectively looks like a PCI device is 
hotplugged for the guest. In such a case, the kernel assumes the 
endpoint is detached from the virtio-iommu domain, but QEMU actually 
does not detach it.


This inconsistent view of the removed device sometimes prevents the VM 
from correctly performing the following procedure, for example:

1. Enable a VF.
2. Disable the VF.
3. Open a vfio container.
4. Open the group which the PF belongs to.
5. Add the group to the vfio container.
6. Map some memory region.
7. Close the group.
8. Close the vfio container.
9. Repeat 3-8

When the VF gets disabled, the kernel assumes the endpoint is detached 
from the IOMMU domain, but QEMU actually doesn't detach it. Later, the 
domain will be reused in step 3-8.


In step 7, the PF will be detached, and the kernel thinks there is no 
endpoint attached and the mapping the domain holds is cleared, but the 
VF endpoint is still attached and the mapping is kept intact.


In step 9, the same domain will be reused again, and the kernel requests 
to create a new mapping, but it will conflict with the existing mapping 
and result in -EINVAL.


This problem can be fixed by either of:
- requesting the detachment of the endpoint from the guest when the PCI 
device is unplugged (the VF is disabled)
- detecting that the PCI device is gone and automatically detach it on 
QEMU-side.


It is not completely clear for me which solution is more appropriate as 
the virtio-iommu specification is written in a way independent of the 
endpoint mechanism and does not say what should be done when a PCI 
device is unplugged.


Regards,
Akihiko Odaki

Re: [PATCH V2] intel_iommu: refine iotlb hash calculation

[Jason Wang]

On Wed, Apr 12, 2023 at 4:43 PM Alex Bennée  wrote:
>
>
> Jason Wang  writes:
>
> > Commit 1b2b12376c8 ("intel-iommu: PASID support") takes PASID into
> > account when calculating iotlb hash like:
> >
> > static guint vtd_iotlb_hash(gconstpointer v)
> > {
> > const struct vtd_iotlb_key *key = v;
> >
> > return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
> >(key->level) << VTD_IOTLB_LVL_SHIFT |
> >(key->pasid) << VTD_IOTLB_PASID_SHIFT;
> > }
> >
> > This turns out to be problematic since:
> >
> > - the shift will lose bits if not converting to uint64_t
> > - level should be off by one in order to fit into 2 bits
> > - VTD_IOTLB_PASID_SHIFT is 30 but PASID is 20 bits which will waste
> >   some bits
> > - the hash result is uint64_t so we will lose bits when converting to
> >   guint
> >
> > So this patch fixes them by
> >
> > - converting the keys into uint64_t before doing the shift
> > - off level by one to make it fit into two bits
> > - change the sid, lvl and pasid shift to 26, 42 and 44 in order to
> >   take the full width of uint64_t
> > - perform an XOR to the top 32bit with the bottom 32bit for the final
> >   result to fit guint
> >
> > Fixes: Coverity CID 1508100
> > Fixes: 1b2b12376c8 ("intel-iommu: PASID support")
> > Signed-off-by: Jason Wang 
> > ---
> > Changes since V1:
> > - perform XOR to avoid losing bits when converting to gint
> > ---
> >  hw/i386/intel_iommu.c  | 9 +
> >  hw/i386/intel_iommu_internal.h | 6 +++---
> >  2 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index a62896759c..94d52f4205 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -64,8 +64,8 @@ struct vtd_as_key {
> >  struct vtd_iotlb_key {
> >  uint64_t gfn;
> >  uint32_t pasid;
> > -uint32_t level;
> >  uint16_t sid;
> > +uint8_t level;
> >  };
> >
> >  static void vtd_address_space_refresh_all(IntelIOMMUState *s);
> > @@ -221,10 +221,11 @@ static gboolean vtd_iotlb_equal(gconstpointer v1, 
> > gconstpointer v2)
> >  static guint vtd_iotlb_hash(gconstpointer v)
> >  {
> >  const struct vtd_iotlb_key *key = v;
> > +uint64_t hash64 = key->gfn | ((uint64_t)(key->sid) << 
> > VTD_IOTLB_SID_SHIFT) |
> > +(uint64_t)(key->level - 1) << VTD_IOTLB_LVL_SHIFT |
> > +(uint64_t)(key->pasid) << VTD_IOTLB_PASID_SHIFT;
> >
> > -return key->gfn | ((key->sid) << VTD_IOTLB_SID_SHIFT) |
> > -   (key->level) << VTD_IOTLB_LVL_SHIFT |
> > -   (key->pasid) << VTD_IOTLB_PASID_SHIFT;
> > +return (guint)((hash64 >> 32) ^ (hash64 & 0xU));
>
> Have you measured the distribution this hash gives you? Otherwise
> consider using the qemu_xxhash() functions to return a well distributed
> 32 bit hash value.

It depends on a lot of factors and so it won't be even because the
individuals keys are not evenly distributed:

- gfn depends on guest DMA subsystems
- level depends on when huge pages are used
- pasid depends on whether PASID is being used

I'm ok to switch to use qemu_xxhash() if everyone agree. Or if as
Peter said, if it has been dealt with glibc, maybe it's not worth to
bother.

Thanks

>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

Re: [RFC PATCH v2 38/44] target/loongarch: Implement vbitsel vset

[gaosong]

在 2023/4/12 下午2:53, Richard Henderson 写道:



+#define SETANYEQZ(NAME, BIT, E) \
+void HELPER(NAME)(CPULoongArchState *env, uint32_t cd, uint32_t vj) \
+{   \
+    int i; \
+    bool ret = false;   \
+    VReg *Vj = &(env->fpr[vj].vreg); \
+    \
+    for (i = 0; i < LSX_LEN/BIT; i++) { \
+    ret |= (Vj->E(i) == 0); \
+ } \
+    env->cf[cd & 0x7] = ret;    \
+}
+SETANYEQZ(vsetanyeqz_b, 8, B)
+SETANYEQZ(vsetanyeqz_h, 16, H)
+SETANYEQZ(vsetanyeqz_w, 32, W)
+SETANYEQZ(vsetanyeqz_d, 64, D)


These could be inlined, though slightly harder.
C.f. target/arm/sve_helper.c, do_match2 (your n == 0).

Do you mean an inline like trans_vseteqz_v or just an inline helper 
function?


I meant inline tcg code generation, instead of a call to a helper.
But even if we keep this in a helper, see do_match2 for avoiding the 
loop over bytes. 

Ok,
e.g
#define SETANYEQZ(NAME, MO)          \
void HELPER(NAME)(CPULoongArchState *env, uint32_t cd, uint32_t vj) \
{     \
    int i;                                                            \
    bool ret = false; \
    VReg *Vj = &(env->fpr[vj].vreg); \
\
    ret = do_match2(0, (uint64_t)Vj->D(0), (uint64_t)Vj->D(1), 
MO);          \

    env->cf[cd & 0x7] = ret;      \
}
SETANYEQZ(vsetanyeqz_b, MO_8)
SETANYEQZ(vsetanyeqz_h, MO_16)
SETANYEQZ(vsetanyeqz_w, MO_32)
SETANYEQZ(vsetanyeqz_d, MO_64)

and
vsetanyeqz.b    $fcc5  $vr11
  v11    : {edc0004d576eef5b, ec03ec0fec03ea47}
--
do_match2
bits is 8
m1 is ec03ec0fec03ea47
m0 is edc0004d576eef5b
ones is 1010101
sings is 80808080
cmp1 is 0
cmp0 is edc0004d576eef5b
cmp1 is ec03ec0fec03ea47
cmp0 is 1
cmp1 is 3000100
ret is 0

but,  the results is not correct  for vsetanyeqz.b. :-)

Thanks.
Song Gao

Re: [PATCH v10 0/9] KVM: mm: fd-based approach for supporting KVM

[Sean Christopherson]

On Wed, Jan 25, 2023, Kirill A. Shutemov wrote:
> On Wed, Jan 25, 2023 at 12:20:26AM +, Sean Christopherson wrote:
> > On Tue, Jan 24, 2023, Liam Merwick wrote:
> > > On 14/01/2023 00:37, Sean Christopherson wrote:
> > > > On Fri, Dec 02, 2022, Chao Peng wrote:
> > > > > This patch series implements KVM guest private memory for confidential
> > > > > computing scenarios like Intel TDX[1]. If a TDX host accesses
> > > > > TDX-protected guest memory, machine check can happen which can further
> > > > > crash the running host system, this is terrible for multi-tenant
> > > > > configurations. The host accesses include those from KVM userspace 
> > > > > like
> > > > > QEMU. This series addresses KVM userspace induced crash by introducing
> > > > > new mm and KVM interfaces so KVM userspace can still manage guest 
> > > > > memory
> > > > > via a fd-based approach, but it can never access the guest memory
> > > > > content.
> > > > > 
> > > > > The patch series touches both core mm and KVM code. I appreciate
> > > > > Andrew/Hugh and Paolo/Sean can review and pick these patches. Any 
> > > > > other
> > > > > reviews are always welcome.
> > > > >- 01: mm change, target for mm tree
> > > > >- 02-09: KVM change, target for KVM tree
> > > > 
> > > > A version with all of my feedback, plus reworked versions of Vishal's 
> > > > selftest,
> > > > is available here:
> > > > 
> > > >g...@github.com:sean-jc/linux.git x86/upm_base_support
> > > > 
> > > > It compiles and passes the selftest, but it's otherwise barely tested.  
> > > > There are
> > > > a few todos (2 I think?) and many of the commits need changelogs, i.e. 
> > > > it's still
> > > > a WIP.
> > > > 
> > > 
> > > When running LTP (https://github.com/linux-test-project/ltp) on the v10
> > > bits (and also with Sean's branch above) I encounter the following NULL
> > > pointer dereference with testcases/kernel/syscalls/madvise/madvise01
> > > (100% reproducible).
> > > 
> > > It appears that in restrictedmem_error_page()
> > > inode->i_mapping->private_data is NULL in the
> > > list_for_each_entry_safe(inode, next, >s_inodes, i_sb_list) but I
> > > don't know why.
> > 
> > Kirill, can you take a look?  Or pass the buck to someone who can? :-)
> 
> The patch below should help.
> 
> diff --git a/mm/restrictedmem.c b/mm/restrictedmem.c
> index 15c52301eeb9..39ada985c7c0 100644
> --- a/mm/restrictedmem.c
> +++ b/mm/restrictedmem.c
> @@ -307,14 +307,29 @@ void restrictedmem_error_page(struct page *page, struct 
> address_space *mapping)
>  
>   spin_lock(>s_inode_list_lock);
>   list_for_each_entry_safe(inode, next, >s_inodes, i_sb_list) {
> - struct restrictedmem *rm = inode->i_mapping->private_data;
>   struct restrictedmem_notifier *notifier;
> - struct file *memfd = rm->memfd;
> + struct restrictedmem *rm;
>   unsigned long index;
> + struct file *memfd;
>  
> - if (memfd->f_mapping != mapping)
> + if (atomic_read(>i_count))

Kirill, should this be

if (!atomic_read(>i_count))
continue;

i.e. skip unreferenced inodes, not skip referenced inodes?

Re: [PATCH 10/16] hw/i3c/aspeed_i3c: Add IBI handling

[Joe Komlodi]

Hi jeremy,

On Tue, Apr 11, 2023 at 2:17 AM Jeremy Kerr  wrote:
>
> Hi Joe,
>
> > +static int aspeed_i3c_device_ibi_finish(I3CBus *bus)
> > +{
> > +AspeedI3CDevice *s = ASPEED_I3C_DEVICE(bus->qbus.parent);
> > +bool nack_and_disable_hj = ARRAY_FIELD_EX32(s->regs, DEVICE_CTRL,
> > +HOT_JOIN_ACK_NACK_CTRL);
> > +if (nack_and_disable_hj || s->ibi_data.send_direct_disec) {
> > +aspeed_i3c_device_send_disec(s);
> > +}
>
> Shouldn't this be conditional on the ibi being a HJ request? With this,
> I'm seeing the DISEC happen on *all* IBIs.

Good catch! Yep it should be on HJs only.
Will fix in v2 as well.

Thanks,
Joe

>
> Cheers,
>
>
> Jeremy
>

Re: [PATCH v10 0/9] KVM: mm: fd-based approach for supporting KVM

[Sean Christopherson]

On Wed, Mar 22, 2023, Michael Roth wrote:
> On Tue, Feb 21, 2023 at 08:11:35PM +0800, Chao Peng wrote:
> > >   *fixup (upm_base_support): KVM: use inclusive ranges for restrictedmem 
> > > binding/unbinding
> > >   *fixup (upm_base_support): mm: restrictedmem: use inclusive ranges for 
> > > issuing invalidations
> > 
> > As many kernel APIs treat 'end' as exclusive, I would rather keep using
> > exclusive 'end' for these APIs(restrictedmem_bind/restrictedmem_unbind
> > and notifier callbacks) but fix it internally in the restrictedmem. E.g.
> > all the places where xarray API needs a 'last'/'max' we use 'end - 1'.
> > See below for the change.
> 
> Yes I did feel like I was fighting the kernel a bit on that; your
> suggestion seems like it would be a better fit.

Comically belated +1, XArray is the odd one here.

[PULL 0/5] Migration 20230412 patches

[Juan Quintela]

The following changes since commit abb02ce0e76a8e00026699a863ab2d11d88f56d4:

  Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging 
(2023-04-11 16:19:06 +0100)

are available in the Git repository at:

  https://gitlab.com/juan.quintela/qemu.git tags/migration-20230412-pull-request

for you to fetch changes up to 28ef5339c37f1f78c2fa4df2295bc0cd73a0abfd:

  migration: fix ram_state_pending_exact() (2023-04-12 22:47:50 +0200)


Migration Pull request for 8.0

Last patches found:
- peter xu preempt channel fixes.
  needed for backward compatibility with old machine types.
- lukas fix to get compress working again.

- fix ram on s390x.  Get back to the old code, even when it shouldn't
  be needed, but as it fails on s390x, just revert.

Later, Juan.



Juan Quintela (1):
  migration: fix ram_state_pending_exact()

Lukas Straub (1):
  migration/ram.c: Fix migration with compress enabled

Peter Xu (3):
  io: tls: Inherit QIO_CHANNEL_FEATURE_SHUTDOWN on server side
  migration: Fix potential race on postcopy_qemufile_src
  migration: Recover behavior of preempt channel creation for pre-7.2

 migration/migration.h| 41 +++-
 hw/core/machine.c|  1 +
 io/channel-tls.c |  3 +++
 migration/migration.c| 19 +--
 migration/postcopy-ram.c | 30 ++---
 migration/ram.c  | 27 +-
 6 files changed, 97 insertions(+), 24 deletions(-)

-- 
2.39.2

[PULL 3/5] migration: Recover behavior of preempt channel creation for pre-7.2

[Juan Quintela]

From: Peter Xu 

In 8.0 devel window we reworked preempt channel creation, so that there'll
be no race condition when the migration channel and preempt channel got
established in the wrong order in commit 5655aab079.

However no one noticed that the change will also be not compatible with
older qemus, majorly 7.1/7.2 versions where preempt mode started to be
supported.

Leverage the same pre-7.2 flag introduced in the previous patch to recover
the behavior hopefully before 8.0 releases, so we don't break migration
when we migrate from 8.0 to older qemu binaries.

Fixes: 5655aab079 ("migration: Postpone postcopy preempt channel to be after 
main")
Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/migration.h|  7 +++
 migration/migration.c|  9 +
 migration/postcopy-ram.c | 10 --
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 67baba2184..310ae8901b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -384,12 +384,19 @@ struct MigrationState {
  * - postcopy preempt src QEMU instance will generate an EOS message at
  *   the end of migration to shut the preempt channel on dest side.
  *
+ * - postcopy preempt channel will be created at the setup phase on src
+ QEMU.
+ *
  * When clear:
  *
  * - postcopy preempt src QEMU instance will _not_ generate an EOS
  *   message at the end of migration, the dest qemu will shutdown the
  *   channel itself.
  *
+ * - postcopy preempt channel will be created at the switching phase
+ *   from precopy -> postcopy (to avoid race condtion of misordered
+ *   creation of channels).
+ *
  * NOTE: See message-id  on qemu-devel
  * mailing list for more information on the possible race.  Everyone
  * should probably just keep this value untouched after set by the
diff --git a/migration/migration.c b/migration/migration.c
index 37fc4fb3e2..bda4789193 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4388,6 +4388,15 @@ void migrate_fd_connect(MigrationState *s, Error 
*error_in)
 }
 }
 
+/*
+ * This needs to be done before resuming a postcopy.  Note: for newer
+ * QEMUs we will delay the channel creation until postcopy_start(), to
+ * avoid disorder of channel creations.
+ */
+if (migrate_postcopy_preempt() && s->preempt_pre_7_2) {
+postcopy_preempt_setup(s);
+}
+
 if (resume) {
 /* Wakeup the main migration thread to do the recovery */
 migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_PAUSED,
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 263bab75ec..93f39f8e06 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -1630,8 +1630,14 @@ int postcopy_preempt_establish_channel(MigrationState *s)
 return 0;
 }
 
-/* Kick off async task to establish preempt channel */
-postcopy_preempt_setup(s);
+/*
+ * Kick off async task to establish preempt channel.  Only do so with
+ * 8.0+ machines, because 7.1/7.2 require the channel to be created in
+ * setup phase of migration (even if racy in an unreliable network).
+ */
+if (!s->preempt_pre_7_2) {
+postcopy_preempt_setup(s);
+}
 
 /*
  * We need the postcopy preempt channel to be established before
-- 
2.39.2

[PULL 2/5] migration: Fix potential race on postcopy_qemufile_src

[Juan Quintela]

From: Peter Xu 

postcopy_qemufile_src object should be owned by one thread, either the main
thread (e.g. when at the beginning, or at the end of migration), or by the
return path thread (when during a preempt enabled postcopy migration).  If
that's not the case the access to the object might be racy.

postcopy_preempt_shutdown_file() can be potentially racy, because it's
called at the end phase of migration on the main thread, however during
which the return path thread hasn't yet been recycled; the recycle happens
in await_return_path_close_on_source() which is after this point.

It means, logically it's posslbe the main thread and the return path thread
are both operating on the same qemufile.  While I don't think qemufile is
thread safe at all.

postcopy_preempt_shutdown_file() used to be needed because that's where we
send EOS to dest so that dest can safely shutdown the preempt thread.

To avoid the possible race, remove this only place that a race can happen.
Instead we figure out another way to safely close the preempt thread on
dest.

The core idea during postcopy on deciding "when to stop" is that dest will
send a postcopy SHUT message to src, telling src that all data is there.
Hence to shut the dest preempt thread maybe better to do it directly on
dest node.

This patch proposed such a way that we change postcopy_prio_thread_created
into PreemptThreadStatus, so that we kick the preempt thread on dest qemu
by a sequence of:

  mis->preempt_thread_status = PREEMPT_THREAD_QUIT;
  qemu_file_shutdown(mis->postcopy_qemufile_dst);

While here shutdown() is probably so far the easiest way to kick preempt
thread from a blocked qemu_get_be64().  Then it reads preempt_thread_status
to make sure it's not a network failure but a willingness to quit the
thread.

We could have avoided that extra status but just rely on migration status.
The problem is postcopy_ram_incoming_cleanup() is just called early enough
so we're still during POSTCOPY_ACTIVE no matter what.. So just make it
simple to have the status introduced.

One flag x-preempt-pre-7-2 is added to keep old pre-7.2 behaviors of
postcopy preempt.

Fixes: 9358982744 ("migration: Send requested page directly in rp-return 
thread")
Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/migration.h| 34 +-
 hw/core/machine.c|  1 +
 migration/migration.c| 10 --
 migration/postcopy-ram.c | 20 +++-
 4 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/migration/migration.h b/migration/migration.h
index 2da2f8a164..67baba2184 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -65,6 +65,12 @@ typedef struct {
 bool all_zero;
 } PostcopyTmpPage;
 
+typedef enum {
+PREEMPT_THREAD_NONE = 0,
+PREEMPT_THREAD_CREATED,
+PREEMPT_THREAD_QUIT,
+} PreemptThreadStatus;
+
 /* State for the incoming migration */
 struct MigrationIncomingState {
 QEMUFile *from_src_file;
@@ -124,7 +130,12 @@ struct MigrationIncomingState {
 QemuSemaphore postcopy_qemufile_dst_done;
 /* Postcopy priority thread is used to receive postcopy requested pages */
 QemuThread postcopy_prio_thread;
-bool postcopy_prio_thread_created;
+/*
+ * Always set by the main vm load thread only, but can be read by the
+ * postcopy preempt thread.  "volatile" makes sure all reads will be
+ * uptodate across cores.
+ */
+volatile PreemptThreadStatus preempt_thread_status;
 /*
  * Used to sync between the ram load main thread and the fast ram load
  * thread.  It protects postcopy_qemufile_dst, which is the postcopy
@@ -364,6 +375,27 @@ struct MigrationState {
  * do not trigger spurious decompression errors.
  */
 bool decompress_error_check;
+/*
+ * This variable only affects behavior when postcopy preempt mode is
+ * enabled.
+ *
+ * When set:
+ *
+ * - postcopy preempt src QEMU instance will generate an EOS message at
+ *   the end of migration to shut the preempt channel on dest side.
+ *
+ * When clear:
+ *
+ * - postcopy preempt src QEMU instance will _not_ generate an EOS
+ *   message at the end of migration, the dest qemu will shutdown the
+ *   channel itself.
+ *
+ * NOTE: See message-id  on qemu-devel
+ * mailing list for more information on the possible race.  Everyone
+ * should probably just keep this value untouched after set by the
+ * machine type (or the default).
+ */
+bool preempt_pre_7_2;
 
 /*
  * This decides the size of guest memory chunk that will be used
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 45e3d24fdc..cd13b8b0a3 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -42,6 +42,7 @@
 GlobalProperty hw_compat_7_2[] = {
 { "e1000e", "migrate-timadj", "off" },
 { "virtio-mem", "x-early-migration", "false" },
+{ "migration", "x-preempt-pre-7-2", 

[PULL 5/5] migration: fix ram_state_pending_exact()

[Juan Quintela]

I removed that bit on commit:

commit c8df4a7aeffcb46020f610526eea621fa5b0cd47
Author: Juan Quintela 
Date:   Mon Oct 3 02:00:03 2022 +0200

migration: Split save_live_pending() into state_pending_*

Fixes: c8df4a7aeffcb46020f610526eea621fa5b0cd47
Suggested-by: Nina Schoetterl-Glausch 
Signed-off-by: Juan Quintela 
---
 migration/ram.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 9d1817ab7b..79d881f735 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3506,12 +3506,13 @@ static void ram_state_pending_estimate(void *opaque, 
uint64_t *must_precopy,
 static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
 uint64_t *can_postcopy)
 {
+MigrationState *s = migrate_get_current();
 RAMState **temp = opaque;
 RAMState *rs = *temp;
 
 uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
 
-if (!migration_in_postcopy()) {
+if (!migration_in_postcopy() && remaining_size < s->threshold_size) {
 qemu_mutex_lock_iothread();
 WITH_RCU_READ_LOCK_GUARD() {
 migration_bitmap_sync_precopy(rs);
-- 
2.39.2

[PULL 1/5] io: tls: Inherit QIO_CHANNEL_FEATURE_SHUTDOWN on server side

[Juan Quintela]

From: Peter Xu 

TLS iochannel will inherit io_shutdown() from the master ioc, however we
missed to do that on the server side.

This will e.g. allow qemu_file_shutdown() to work on dest QEMU too for
migration.

Acked-by: Daniel P. Berrangé 
Signed-off-by: Peter Xu 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 io/channel-tls.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 5a7a3d48d6..9805dd0a3f 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -74,6 +74,9 @@ qio_channel_tls_new_server(QIOChannel *master,
 ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
 
 ioc->master = master;
+if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
+qio_channel_set_feature(QIO_CHANNEL(ioc), 
QIO_CHANNEL_FEATURE_SHUTDOWN);
+}
 object_ref(OBJECT(master));
 
 ioc->session = qcrypto_tls_session_new(
-- 
2.39.2

[PULL 4/5] migration/ram.c: Fix migration with compress enabled

[Juan Quintela]

From: Lukas Straub 

Since ec6f3ab9, migration with compress enabled was broken, because
the compress threads use a dummy QEMUFile which just acts as a
buffer and that commit accidentally changed it to use the outgoing
migration channel instead.

Fix this by using the dummy file again in the compress threads.

Signed-off-by: Lukas Straub 
Reviewed-by: Juan Quintela 
Signed-off-by: Juan Quintela 
---
 migration/ram.c | 24 +++-
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 96e8a19a58..9d1817ab7b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -688,12 +688,11 @@ exit:
  * @offset: offset inside the block for the page
  *  in the lower bits, it contains flags
  */
-static size_t save_page_header(PageSearchStatus *pss, RAMBlock *block,
-   ram_addr_t offset)
+static size_t save_page_header(PageSearchStatus *pss, QEMUFile *f,
+   RAMBlock *block, ram_addr_t offset)
 {
 size_t size, len;
 bool same_block = (block == pss->last_sent_block);
-QEMUFile *f = pss->pss_channel;
 
 if (same_block) {
 offset |= RAM_SAVE_FLAG_CONTINUE;
@@ -867,7 +866,7 @@ static int save_xbzrle_page(RAMState *rs, PageSearchStatus 
*pss,
 }
 
 /* Send XBZRLE based compressed page */
-bytes_xbzrle = save_page_header(pss, block,
+bytes_xbzrle = save_page_header(pss, pss->pss_channel, block,
 offset | RAM_SAVE_FLAG_XBZRLE);
 qemu_put_byte(file, ENCODING_FLAG_XBZRLE);
 qemu_put_be16(file, encoded_len);
@@ -1302,15 +1301,14 @@ void ram_release_page(const char *rbname, uint64_t 
offset)
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page_to_file(PageSearchStatus *pss,
+static int save_zero_page_to_file(PageSearchStatus *pss, QEMUFile *file,
   RAMBlock *block, ram_addr_t offset)
 {
 uint8_t *p = block->host + offset;
-QEMUFile *file = pss->pss_channel;
 int len = 0;
 
 if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
-len += save_page_header(pss, block, offset | RAM_SAVE_FLAG_ZERO);
+len += save_page_header(pss, file, block, offset | RAM_SAVE_FLAG_ZERO);
 qemu_put_byte(file, 0);
 len += 1;
 ram_release_page(block->idstr, offset);
@@ -1327,10 +1325,10 @@ static int save_zero_page_to_file(PageSearchStatus *pss,
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
+static int save_zero_page(PageSearchStatus *pss, QEMUFile *f, RAMBlock *block,
   ram_addr_t offset)
 {
-int len = save_zero_page_to_file(pss, block, offset);
+int len = save_zero_page_to_file(pss, f, block, offset);
 
 if (len) {
 stat64_add(_atomic_counters.duplicate, 1);
@@ -1394,7 +1392,7 @@ static int save_normal_page(PageSearchStatus *pss, 
RAMBlock *block,
 {
 QEMUFile *file = pss->pss_channel;
 
-ram_transferred_add(save_page_header(pss, block,
+ram_transferred_add(save_page_header(pss, pss->pss_channel, block,
  offset | RAM_SAVE_FLAG_PAGE));
 if (async) {
 qemu_put_buffer_async(file, buf, TARGET_PAGE_SIZE,
@@ -1473,11 +1471,11 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream 
*stream, RAMBlock *block,
 uint8_t *p = block->host + offset;
 int ret;
 
-if (save_zero_page_to_file(pss, block, offset)) {
+if (save_zero_page_to_file(pss, f, block, offset)) {
 return true;
 }
 
-save_page_header(pss, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
+save_page_header(pss, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
 
 /*
  * copy it to a internal buffer to avoid it being modified by VM
@@ -2355,7 +2353,7 @@ static int ram_save_target_page_legacy(RAMState *rs, 
PageSearchStatus *pss)
 return 1;
 }
 
-res = save_zero_page(pss, block, offset);
+res = save_zero_page(pss, pss->pss_channel, block, offset);
 if (res > 0) {
 /* Must let xbzrle know, otherwise a previous (now 0'd) cached
  * page would be stale
-- 
2.39.2

Re: [PATCH 3/4] vhost: Add high-level state save/load functions

[Stefan Hajnoczi]

On Tue, Apr 11, 2023 at 05:05:14PM +0200, Hanna Czenczek wrote:
> vhost_save_backend_state() and vhost_load_backend_state() can be used by
> vhost front-ends to easily save and load the back-end's state to/from
> the migration stream.
> 
> Because we do not know the full state size ahead of time,
> vhost_save_backend_state() simply reads the data in 1 MB chunks, and
> writes each chunk consecutively into the migration stream, prefixed by
> its length.  EOF is indicated by a 0-length chunk.
> 
> Signed-off-by: Hanna Czenczek 
> ---
>  include/hw/virtio/vhost.h |  35 +++
>  hw/virtio/vhost.c | 196 ++
>  2 files changed, 231 insertions(+)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 29449e0fe2..d1f1e9e1f3 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -425,4 +425,39 @@ int vhost_set_device_state_fd(struct vhost_dev *dev,
>   */
>  int vhost_check_device_state(struct vhost_dev *dev, Error **errp);
>  
> +/**
> + * vhost_save_backend_state(): High-level function to receive a vhost
> + * back-end's state, and save it in `f`.  Uses
> + * `vhost_set_device_state_fd()` to get the data from the back-end, and
> + * stores it in consecutive chunks that are each prefixed by their
> + * respective length (be32).  The end is marked by a 0-length chunk.
> + *
> + * Must only be called while the device and all its vrings are stopped
> + * (`VHOST_TRANSFER_STATE_PHASE_STOPPED`).
> + *
> + * @dev: The vhost device from which to save the state
> + * @f: Migration stream in which to save the state
> + * @errp: Potential error message
> + *
> + * Returns 0 on success, and -errno otherwise.
> + */
> +int vhost_save_backend_state(struct vhost_dev *dev, QEMUFile *f, Error 
> **errp);
> +
> +/**
> + * vhost_load_backend_state(): High-level function to load a vhost
> + * back-end's state from `f`, and send it over to the back-end.  Reads
> + * the data from `f` in the format used by `vhost_save_state()`, and
> + * uses `vhost_set_device_state_fd()` to transfer it to the back-end.
> + *
> + * Must only be called while the device and all its vrings are stopped
> + * (`VHOST_TRANSFER_STATE_PHASE_STOPPED`).
> + *
> + * @dev: The vhost device to which to send the sate
> + * @f: Migration stream from which to load the state
> + * @errp: Potential error message
> + *
> + * Returns 0 on success, and -errno otherwise.
> + */
> +int vhost_load_backend_state(struct vhost_dev *dev, QEMUFile *f, Error 
> **errp);
> +
>  #endif
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 90099d8f6a..d08849c691 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -2125,3 +2125,199 @@ int vhost_check_device_state(struct vhost_dev *dev, 
> Error **errp)
> "vhost transport does not support migration state transfer");
>  return -ENOSYS;
>  }
> +
> +int vhost_save_backend_state(struct vhost_dev *dev, QEMUFile *f, Error 
> **errp)
> +{
> +/* Maximum chunk size in which to transfer the state */
> +const size_t chunk_size = 1 * 1024 * 1024;
> +void *transfer_buf = NULL;
> +g_autoptr(GError) g_err = NULL;
> +int pipe_fds[2], read_fd = -1, write_fd = -1, reply_fd = -1;
> +int ret;
> +
> +/* [0] for reading (our end), [1] for writing (back-end's end) */
> +if (!g_unix_open_pipe(pipe_fds, FD_CLOEXEC, _err)) {
> +error_setg(errp, "Failed to set up state transfer pipe: %s",
> +   g_err->message);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +read_fd = pipe_fds[0];
> +write_fd = pipe_fds[1];
> +
> +/* VHOST_TRANSFER_STATE_PHASE_STOPPED means the device must be stopped */
> +assert(!dev->started && !dev->enable_vqs);
> +
> +/* Transfer ownership of write_fd to the back-end */
> +ret = vhost_set_device_state_fd(dev,
> +VHOST_TRANSFER_STATE_DIRECTION_SAVE,
> +VHOST_TRANSFER_STATE_PHASE_STOPPED,
> +write_fd,
> +_fd,
> +errp);
> +if (ret < 0) {
> +error_prepend(errp, "Failed to initiate state transfer: ");
> +goto fail;
> +}
> +
> +/* If the back-end wishes to use a different pipe, switch over */
> +if (reply_fd >= 0) {
> +close(read_fd);
> +read_fd = reply_fd;
> +}
> +
> +transfer_buf = g_malloc(chunk_size);
> +
> +while (true) {
> +ssize_t read_ret;
> +
> +read_ret = read(read_fd, transfer_buf, chunk_size);
> +if (read_ret < 0) {
> +ret = -errno;
> +error_setg_errno(errp, -ret, "Failed to receive state");
> +goto fail;
> +}
> +
> +assert(read_ret <= chunk_size);
> +qemu_put_be32(f, read_ret);
> +
> +if (read_ret == 0) {
> +/* EOF */
> +break;
> +}
> +
> +

Re: [PATCH 1/3] ui/sdl2: Grab Alt+Tab also in fullscreen mode

[Bernhard Beschow]

Am 12. April 2023 20:34:23 UTC schrieb Bernhard Beschow :
>By default, SDL grabs Alt+Tab only in non-fullscreen mode. This causes Alt+Tab
>to switch tasks on the host rather than in the VM in fullscreen mode while it
>switches tasks in non-fullscreen mode in the VM. Fix this confusing behavior
>by grabbing Alt+F4 in fullscreen mode, always causing tasks to be switched in

Typo: s/Alt+F4/Alt+Tab/

>the VM.
>
>Signed-off-by: Bernhard Beschow 
>---
> ui/sdl2.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/ui/sdl2.c b/ui/sdl2.c
>index b12dec4caf..91705e4bce 100644
>--- a/ui/sdl2.c
>+++ b/ui/sdl2.c
>@@ -856,6 +856,7 @@ static void sdl2_display_init(DisplayState *ds, 
>DisplayOptions *o)
> SDL_SetHint(SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, "0");
> #endif
> SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
>+SDL_SetHint(SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED, "0");
> memset(, 0, sizeof(info));
> SDL_VERSION();
> 

Re: [PATCH for 8.1] intel_iommu: refine iotlb hash calculation

[Peter Maydell]

On Wed, 12 Apr 2023 at 09:40, Alex Bennée  wrote:
> Peter Maydell  writes:
> > Whoops, hadn't noticed that guint type... (glib's
> > g_int64_hash()'s approach to this is to XOR the top
> > 32 bits with the bottom 32 bits to produce the 32-bit
> > hash value.)
>
> This is less of a hash and more just concatting a bunch of fields. BTW
> if the glib built-in hash isn't suitable we also have the qemu_xxhash()
> functions which claim a good distribution of values and we use in a
> number of places throughout the code.

Is that really necessary? If glib doesn't do anything complex
for "my keys are just integers" I don't see that we need to
do anything complex for "my keys are a handful of integers".
glib does do a bit on its end to counteract suboptimal hash functions:

https://github.com/GNOME/glib/blob/main/glib/ghash.c#L429

static inline guint
g_hash_table_hash_to_index (GHashTable *hash_table, guint hash)
{
  /* Multiply the hash by a small prime before applying the modulo. This
   * prevents the table from becoming densely packed, even with a poor hash
   * function. A densely packed table would have poor performance on
   * workloads with many failed lookups or a high degree of churn. */
  return (hash * 11) % hash_table->mod;
}

I figure if glib thought that users of hash tables should be
doing more complex stuff then they would (a) provide helpers
for that and (b) call it out in the docs. They don't do either.

thanks
-- PMM

Re: [PATCH 2/4] vhost-user: Interface for migration state transfer

[Stefan Hajnoczi]

On Tue, Apr 11, 2023 at 05:05:13PM +0200, Hanna Czenczek wrote:
> So-called "internal" virtio-fs migration refers to transporting the
> back-end's (virtiofsd's) state through qemu's migration stream.  To do
> this, we need to be able to transfer virtiofsd's internal state to and
> from virtiofsd.
> 
> Because virtiofsd's internal state will not be too large, we believe it
> is best to transfer it as a single binary blob after the streaming
> phase.  Because this method should be useful to other vhost-user
> implementations, too, it is introduced as a general-purpose addition to
> the protocol, not limited to vhost-user-fs.
> 
> These are the additions to the protocol:
> - New vhost-user protocol feature VHOST_USER_PROTOCOL_F_MIGRATORY_STATE:
>   This feature signals support for transferring state, and is added so
>   that migration can fail early when the back-end has no support.
> 
> - SET_DEVICE_STATE_FD function: Front-end and back-end negotiate a pipe
>   over which to transfer the state.  The front-end sends an FD to the
>   back-end into/from which it can write/read its state, and the back-end
>   can decide to either use it, or reply with a different FD for the
>   front-end to override the front-end's choice.
>   The front-end creates a simple pipe to transfer the state, but maybe
>   the back-end already has an FD into/from which it has to write/read
>   its state, in which case it will want to override the simple pipe.
>   Conversely, maybe in the future we find a way to have the front-end
>   get an immediate FD for the migration stream (in some cases), in which
>   case we will want to send this to the back-end instead of creating a
>   pipe.
>   Hence the negotiation: If one side has a better idea than a plain
>   pipe, we will want to use that.
> 
> - CHECK_DEVICE_STATE: After the state has been transferred through the
>   pipe (the end indicated by EOF), the front-end invokes this function
>   to verify success.  There is no in-band way (through the pipe) to
>   indicate failure, so we need to check explicitly.
> 
> Once the transfer pipe has been established via SET_DEVICE_STATE_FD
> (which includes establishing the direction of transfer and migration
> phase), the sending side writes its data into the pipe, and the reading
> side reads it until it sees an EOF.  Then, the front-end will check for
> success via CHECK_DEVICE_STATE, which on the destination side includes
> checking for integrity (i.e. errors during deserialization).
> 
> Suggested-by: Stefan Hajnoczi 
> Signed-off-by: Hanna Czenczek 
> ---
>  include/hw/virtio/vhost-backend.h |  24 +
>  include/hw/virtio/vhost.h |  79 
>  hw/virtio/vhost-user.c| 147 ++
>  hw/virtio/vhost.c |  37 
>  4 files changed, 287 insertions(+)
> 
> diff --git a/include/hw/virtio/vhost-backend.h 
> b/include/hw/virtio/vhost-backend.h
> index ec3fbae58d..5935b32fe3 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -26,6 +26,18 @@ typedef enum VhostSetConfigType {
>  VHOST_SET_CONFIG_TYPE_MIGRATION = 1,
>  } VhostSetConfigType;
>  
> +typedef enum VhostDeviceStateDirection {
> +/* Transfer state from back-end (device) to front-end */
> +VHOST_TRANSFER_STATE_DIRECTION_SAVE = 0,
> +/* Transfer state from front-end to back-end (device) */
> +VHOST_TRANSFER_STATE_DIRECTION_LOAD = 1,
> +} VhostDeviceStateDirection;
> +
> +typedef enum VhostDeviceStatePhase {
> +/* The device (and all its vrings) is stopped */
> +VHOST_TRANSFER_STATE_PHASE_STOPPED = 0,
> +} VhostDeviceStatePhase;

vDPA has:

  /* Suspend a device so it does not process virtqueue requests anymore
   *
   * After the return of ioctl the device must preserve all the necessary state
   * (the virtqueue vring base plus the possible device specific states) that is
   * required for restoring in the future. The device must not change its
   * configuration after that point.
   */
  #define VHOST_VDPA_SUSPEND  _IO(VHOST_VIRTIO, 0x7D)

  /* Resume a device so it can resume processing virtqueue requests
   *
   * After the return of this ioctl the device will have restored all the
   * necessary states and it is fully operational to continue processing the
   * virtqueue descriptors.
   */
  #define VHOST_VDPA_RESUME   _IO(VHOST_VIRTIO, 0x7E)

I wonder if it makes sense to import these into vhost-user so that the
difference between kernel vhost and vhost-user is minimized. It's okay
if one of them is ahead of the other, but it would be nice to avoid
overlapping/duplicated functionality.

(And I hope vDPA will import the device state vhost-user messages
introduced in this series.)

> +
>  struct vhost_inflight;
>  struct vhost_dev;
>  struct vhost_log;
> @@ -133,6 +145,15 @@ typedef int (*vhost_set_config_call_op)(struct vhost_dev 
> *dev,
>  
>  typedef void (*vhost_reset_status_op)(struct vhost_dev *dev);
>  
> +typedef bool 

Re: s390x TCG migration failure

[Juan Quintela]

Nina Schoetterl-Glausch  wrote:
> Hi,
>
> We're seeing failures running s390x migration kvm-unit-tests tests with TCG.

As this is tcg, could you tell the exact command that you are running?
Does it needs to be in s390x host, rigth?

$ time ./tests/qtest/migration-test
# random seed: R02S940c4f22abc48b14868566639d3d6c77
# Skipping test: s390x host with KVM is required
1..0

real0m0.003s
user0m0.002s
sys 0m0.001s


> Some initial findings:
> What seems to be happening is that after migration a control block
> header accessed by the test code is all zeros which causes an
> unexpected exception.

What exception?

What do you mean here by control block header?

> I did a bisection which points to c8df4a7aef ("migration: Split 
> save_live_pending() into state_pending_*") as the culprit.
> The migration issue persists after applying the fix e264705012 ("migration: I 
> messed state_pending_exact/estimate") on top of c8df4a7aef.
>
> Applying
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 56ff9cd29d..2dc546cf28 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3437,7 +3437,7 @@ static void ram_state_pending_exact(void *opaque, 
> uint64_t max_size,
>  
>  uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
>  
> -if (!migration_in_postcopy()) {
> +if (!migration_in_postcopy() && remaining_size < max_size) {

If block is all zeros, then remaining_size should be zero, so always
smaller than max_size.

I don't really fully understand what is going here.

>  qemu_mutex_lock_iothread();
>  WITH_RCU_READ_LOCK_GUARD() {
>  migration_bitmap_sync_precopy(rs);
>
> on top fixes or hides the issue. (The comparison was removed by c8df4a7aef.)
> I arrived at this by experimentation, I haven't looked into why this makes a 
> difference.
>
> Any thoughts on the matter appreciated.

Later, Juan.

Re: [PATCH] tests/vm: use the default system python for NetBSD

[John Snow]

On Wed, Mar 29, 2023 at 8:47 AM Daniel P. Berrangé  wrote:
>
> Currently our NetBSD VM recipe requests instal of the python37 package
> and explicitly tells QEMU to use that version of python. Since the
> NetBSD base ISO was updated to version 9.3 though, the default system
> python version is 3.9 which is sufficiently new for QEMU to rely on.
> Rather than requesting an older python, just test against the default
> system python which is what most users will have.

Is this the default Python, or does it just happen to be the python
that one of our other dependencies claims to require? From my notes on
the mkvenv.py work, I had actually changed this over to requiring
Python 3.10, because it appeared at the time that NetBSD only shipped
pip for 3.10.

e.g. https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/index-all.html
you can see here we've got "py310-pip" but I don't see "py39-pip". The
only other pip I see is py27-pip.

The impression I got was:

1) There's no such thing as a "default" python for NetBSD,
2) The best Python to use on this platform is currently 3.10.

I'm not very familiar with NetBSD though, so it's definitely possible
I misunderstood something.

--js

>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  tests/vm/netbsd | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/tests/vm/netbsd b/tests/vm/netbsd
> index aa54338dfa..0b9536ca17 100755
> --- a/tests/vm/netbsd
> +++ b/tests/vm/netbsd
> @@ -30,7 +30,6 @@ class NetBSDVM(basevm.BaseVM):
>  "git-base",
>  "pkgconf",
>  "xz",
> -"python37",
>  "ninja-build",
>
>  # gnu tools
> @@ -66,7 +65,7 @@ class NetBSDVM(basevm.BaseVM):
>  mkdir src build; cd src;
>  tar -xf /dev/rld1a;
>  cd ../build
> -../src/configure --python=python3.7 --disable-opengl 
> {configure_opts};
> +../src/configure --disable-opengl {configure_opts};
>  gmake --output-sync -j{jobs} {target} {verbose};
>  """
>  poweroff = "/sbin/poweroff"
> --
> 2.39.1
>
>

Re: [PATCH 0/4] vhost-user-fs: Internal migration

[Stefan Hajnoczi]

Hi,
Is there a vhost-user.rst spec patch?

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [PATCH 1/4] vhost: Re-enable vrings after setting features

[Stefan Hajnoczi]

On Tue, Apr 11, 2023 at 05:05:12PM +0200, Hanna Czenczek wrote:
> If the back-end supports the VHOST_USER_F_PROTOCOL_FEATURES feature,
> setting the vhost features will set this feature, too.  Doing so
> disables all vrings, which may not be intended.
> 
> For example, enabling or disabling logging during migration requires
> setting those features (to set or unset VHOST_F_LOG_ALL), which will
> automatically disable all vrings.  In either case, the VM is running
> (disabling logging is done after a failed or cancelled migration, and
> only once the VM is running again, see comment in
> memory_global_dirty_log_stop()), so the vrings should really be enabled.
> As a result, the back-end seems to hang.
> 
> To fix this, we must remember whether the vrings are supposed to be
> enabled, and, if so, re-enable them after a SET_FEATURES call that set
> VHOST_USER_F_PROTOCOL_FEATURES.
> 
> It seems less than ideal that there is a short period in which the VM is
> running but the vrings will be stopped (between SET_FEATURES and
> SET_VRING_ENABLE).  To fix this, we would need to change the protocol,
> e.g. by introducing a new flag or vhost-user protocol feature to disable
> disabling vrings whenever VHOST_USER_F_PROTOCOL_FEATURES is set, or add
> new functions for setting/clearing singular feature bits (so that
> F_LOG_ALL can be set/cleared without touching F_PROTOCOL_FEATURES).
> 
> Even with such a potential addition to the protocol, we still need this
> fix here, because we cannot expect that back-ends will implement this
> addition.
> 
> Signed-off-by: Hanna Czenczek 
> ---
>  include/hw/virtio/vhost.h | 10 ++
>  hw/virtio/vhost.c | 13 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index a52f273347..2fe02ed5d4 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -90,6 +90,16 @@ struct vhost_dev {
>  int vq_index_end;
>  /* if non-zero, minimum required value for max_queues */
>  int num_queues;
> +
> +/*
> + * Whether the virtqueues are supposed to be enabled (via
> + * SET_VRING_ENABLE).  Setting the features (e.g. for
> + * enabling/disabling logging) will disable all virtqueues if
> + * VHOST_USER_F_PROTOCOL_FEATURES is set, so then we need to
> + * re-enable them if this field is set.
> + */
> +bool enable_vqs;
> +
>  /**
>   * vhost feature handling requires matching the feature set
>   * offered by a backend which may be a subset of the total
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index a266396576..cbff589efa 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -50,6 +50,8 @@ static unsigned int used_memslots;
>  static QLIST_HEAD(, vhost_dev) vhost_devices =
>  QLIST_HEAD_INITIALIZER(vhost_devices);
>  
> +static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable);
> +
>  bool vhost_has_free_slot(void)
>  {
>  unsigned int slots_limit = ~0U;
> @@ -899,6 +901,15 @@ static int vhost_dev_set_features(struct vhost_dev *dev,
>  }
>  }
>  
> +if (dev->enable_vqs) {
> +/*
> + * Setting VHOST_USER_F_PROTOCOL_FEATURES would have disabled all
> + * virtqueues, even if that was not intended; re-enable them if
> + * necessary.
> + */
> +vhost_dev_set_vring_enable(dev, true);
> +}
> +
>  out:
>  return r;
>  }
> @@ -1896,6 +1907,8 @@ int vhost_dev_get_inflight(struct vhost_dev *dev, 
> uint16_t queue_size,
>  
>  static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable)
>  {
> +hdev->enable_vqs = enable;
> +
>  if (!hdev->vhost_ops->vhost_set_vring_enable) {
>  return 0;
>  }

The vhost-user spec doesn't say that VHOST_F_LOG_ALL needs to be toggled
at runtime and I don't think VHOST_USER_SET_PROTOCOL_FEATURES is
intended to be used like that. This issue shows why doing so is a bad
idea.

VHOST_F_LOG_ALL does not need to be toggled to control logging. Logging
is controlled at runtime by the presence of the dirty log
(VHOST_USER_SET_LOG_BASE) and the per-vring logging flag
(VHOST_VRING_F_LOG).

I suggest permanently enabling VHOST_F_LOG_ALL upon connection when the
the backend supports it. No spec changes are required.

libvhost-user looks like it will work. I didn't look at DPDK/SPDK, but
checking that it works there is important too.

I have CCed people who may be interested in this issue. This is the
first time I've looked at vhost-user logging, so this idea may not work.

Stefan


signature.asc
Description: PGP signature

Re: s390x TCG migration failure

[Juan Quintela]

Nina Schoetterl-Glausch  wrote:
> Hi,
>
> We're seeing failures running s390x migration kvm-unit-tests tests with TCG.
> Some initial findings:
> What seems to be happening is that after migration a control block header 
> accessed by the test code is all zeros which causes an unexpected exception.
> I did a bisection which points to c8df4a7aef ("migration: Split 
> save_live_pending() into state_pending_*") as the culprit.
> The migration issue persists after applying the fix e264705012 ("migration: I 
> messed state_pending_exact/estimate") on top of c8df4a7aef.
>
> Applying
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 56ff9cd29d..2dc546cf28 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3437,7 +3437,7 @@ static void ram_state_pending_exact(void *opaque, 
> uint64_t max_size,
>  
>  uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
>  
> -if (!migration_in_postcopy()) {
> +if (!migration_in_postcopy() && remaining_size < max_size) {
>  qemu_mutex_lock_iothread();
>  WITH_RCU_READ_LOCK_GUARD() {
>  migration_bitmap_sync_precopy(rs);
>
> on top fixes or hides the issue. (The comparison was removed by c8df4a7aef.)
> I arrived at this by experimentation, I haven't looked into why this makes a 
> difference.

> Any thoughts on the matter appreciated.

This shouldn't be happening.  Famous last words.
I am still applying the patch, to get back to old behaviour, but we
shouldn't be needing this.

Basically when we call ram_state_pending_exact() we know that we want to
sync the bitmap.  But I guess that dirty block bitmap can be
"interesting" to say the less.

Later, Juan.

[PATCH 1/3] ui/sdl2: Grab Alt+Tab also in fullscreen mode

[Bernhard Beschow]

By default, SDL grabs Alt+Tab only in non-fullscreen mode. This causes Alt+Tab
to switch tasks on the host rather than in the VM in fullscreen mode while it
switches tasks in non-fullscreen mode in the VM. Fix this confusing behavior
by grabbing Alt+F4 in fullscreen mode, always causing tasks to be switched in
the VM.

Signed-off-by: Bernhard Beschow 
---
 ui/sdl2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index b12dec4caf..91705e4bce 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -856,6 +856,7 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
 SDL_SetHint(SDL_HINT_VIDEO_X11_NET_WM_BYPASS_COMPOSITOR, "0");
 #endif
 SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
+SDL_SetHint(SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED, "0");
 memset(, 0, sizeof(info));
 SDL_VERSION();
 
-- 
2.40.0

[PATCH 0/3] SDL2 usability fixes

[Bernhard Beschow]

I'm trying to use QEMU on Windows hosts for fun and for profit. While the GTK
GUI doesn't seem to support OpenGL under Windows the SDL2 GUI does. Hence I
used the SDL2 GUI where I ran into several issues of which three are fixed in
this series, which are:

* Alt+Tab switches tasks on the host rather than in the guest in fullscreen mode
* Alt+F4 closes QEMU rather than a graphical task in the guest
* AltGr keyboard modifier isn't recognized by a Linux guest

More information about each issue is provided in the patches.

Bernhard Beschow (3):
  ui/sdl2: Grab Alt+Tab also in fullscreen mode
  ui/sdl2: Grab Alt+F4 also under Windows
  ui/sdl2-input: Fix AltGr modifier on Windows hosts

 ui/sdl2-input.c | 13 +
 ui/sdl2.c   |  2 ++
 2 files changed, 15 insertions(+)

-- 
2.40.0

[PATCH 3/3] ui/sdl2-input: Fix AltGr modifier on Windows hosts

[Bernhard Beschow]

Windows generates Ctrl + Alt_R for AltGr. By removing the Ctrl modifier Linux
guests see AltGr. This fixes e.g. the '~' key on german keyboards.

Signed-off-by: Bernhard Beschow 
---
 ui/sdl2-input.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/ui/sdl2-input.c b/ui/sdl2-input.c
index f068382209..a6538b56d2 100644
--- a/ui/sdl2-input.c
+++ b/ui/sdl2-input.c
@@ -39,6 +39,19 @@ void sdl2_process_key(struct sdl2_console *scon,
 return;
 }
 qcode = qemu_input_map_usb_to_qcode[ev->keysym.scancode];
+
+#ifdef CONFIG_WIN32
+if (qcode == Q_KEY_CODE_ALT_R &&
+qkbd_state_modifier_get(scon->kbd, QKBD_MOD_CTRL)) {
+/*
+ * Windows generates Ctrl + Alt_R for AltGr. By removing the Ctrl
+ * modifier (Linux) guests see AltGr.
+ */
+trace_sdl2_process_key(ev->keysym.scancode, Q_KEY_CODE_CTRL, "up");
+qkbd_state_key_event(scon->kbd, Q_KEY_CODE_CTRL, false);
+}
+#endif
+
 trace_sdl2_process_key(ev->keysym.scancode, qcode,
ev->type == SDL_KEYDOWN ? "down" : "up");
 qkbd_state_key_event(scon->kbd, qcode, ev->type == SDL_KEYDOWN);
-- 
2.40.0

[PATCH 2/3] ui/sdl2: Grab Alt+F4 also under Windows

[Bernhard Beschow]

SDL doesn't grab Alt+F4 under Windows by default. Pressing Alt+F4 thus closes
the VM immediately without confirmation, possibly leading to data loss. Fix
this by always grabbing Alt+F4 on Windows hosts, too.

Signed-off-by: Bernhard Beschow 
---
 ui/sdl2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 91705e4bce..0031f9bca2 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -857,6 +857,7 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
 #endif
 SDL_SetHint(SDL_HINT_GRAB_KEYBOARD, "1");
 SDL_SetHint(SDL_HINT_ALLOW_ALT_TAB_WHILE_GRABBED, "0");
+SDL_SetHint(SDL_HINT_WINDOWS_NO_CLOSE_ON_ALT_F4, "1");
 memset(, 0, sizeof(info));
 SDL_VERSION();
 
-- 
2.40.0

Re: s390x TCG migration failure

[Juan Quintela]

Nina Schoetterl-Glausch  wrote:
> Hi,
>
> We're seeing failures running s390x migration kvm-unit-tests tests with TCG.
> Some initial findings:
> What seems to be happening is that after migration a control block header 
> accessed by the test code is all zeros which causes an unexpected exception.
> I did a bisection which points to c8df4a7aef ("migration: Split 
> save_live_pending() into state_pending_*") as the culprit.
> The migration issue persists after applying the fix e264705012 ("migration: I 
> messed state_pending_exact/estimate") on top of c8df4a7aef.
>
> Applying
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 56ff9cd29d..2dc546cf28 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3437,7 +3437,7 @@ static void ram_state_pending_exact(void *opaque, 
> uint64_t max_size,
>  
>  uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
>  
> -if (!migration_in_postcopy()) {
> +if (!migration_in_postcopy() && remaining_size < max_size) {
>  qemu_mutex_lock_iothread();
>  WITH_RCU_READ_LOCK_GUARD() {
>  migration_bitmap_sync_precopy(rs);
>
> on top fixes or hides the issue. (The comparison was removed by c8df4a7aef.)
> I arrived at this by experimentation, I haven't looked into why this makes a 
> difference.
>
> Any thoughts on the matter appreciated.

Ouch, you are right.
Good catch.

Queued the fix.

Later, Juan.

Re: [RFC 5/5] target/riscv: Expose properties for BF16 extensions

[Daniel Henrique Barboza]

On 4/11/23 23:33, Weiwei Li wrote:

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---


Reviewed-by: Daniel Henrique Barboza 


  target/riscv/cpu.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index c19bbb41fb..0265fae46f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -83,6 +83,7 @@ static const struct isa_ext_data isa_edata_arr[] = {
  ISA_EXT_DATA_ENTRY(zifencei, true, PRIV_VERSION_1_10_0, ext_ifencei),
  ISA_EXT_DATA_ENTRY(zihintpause, true, PRIV_VERSION_1_10_0, 
ext_zihintpause),
  ISA_EXT_DATA_ENTRY(zawrs, true, PRIV_VERSION_1_12_0, ext_zawrs),
+ISA_EXT_DATA_ENTRY(zfbfmin, true, PRIV_VERSION_1_12_0, ext_zfbfmin),
  ISA_EXT_DATA_ENTRY(zfh, true, PRIV_VERSION_1_11_0, ext_zfh),
  ISA_EXT_DATA_ENTRY(zfhmin, true, PRIV_VERSION_1_12_0, ext_zfhmin),
  ISA_EXT_DATA_ENTRY(zfinx, true, PRIV_VERSION_1_12_0, ext_zfinx),
@@ -107,6 +108,8 @@ static const struct isa_ext_data isa_edata_arr[] = {
  ISA_EXT_DATA_ENTRY(zve32f, true, PRIV_VERSION_1_12_0, ext_zve32f),
  ISA_EXT_DATA_ENTRY(zve64f, true, PRIV_VERSION_1_12_0, ext_zve64f),
  ISA_EXT_DATA_ENTRY(zve64d, true, PRIV_VERSION_1_12_0, ext_zve64d),
+ISA_EXT_DATA_ENTRY(zvfbfmin, true, PRIV_VERSION_1_12_0, ext_zvfbfmin),
+ISA_EXT_DATA_ENTRY(zvfbfwma, true, PRIV_VERSION_1_12_0, ext_zvfbfwma),
  ISA_EXT_DATA_ENTRY(zvfh, true, PRIV_VERSION_1_12_0, ext_zvfh),
  ISA_EXT_DATA_ENTRY(zvfhmin, true, PRIV_VERSION_1_12_0, ext_zvfhmin),
  ISA_EXT_DATA_ENTRY(zhinx, true, PRIV_VERSION_1_12_0, ext_zhinx),
@@ -1469,6 +1472,10 @@ static Property riscv_cpu_extensions[] = {
  DEFINE_PROP_BOOL("x-zvfh", RISCVCPU, cfg.ext_zvfh, false),
  DEFINE_PROP_BOOL("x-zvfhmin", RISCVCPU, cfg.ext_zvfhmin, false),
  
+DEFINE_PROP_BOOL("x-zfbfmin", RISCVCPU, cfg.ext_zfbfmin, false),

+DEFINE_PROP_BOOL("x-zvfbfmin", RISCVCPU, cfg.ext_zvfbfmin, false),
+DEFINE_PROP_BOOL("x-zvfbfwma", RISCVCPU, cfg.ext_zvfbfwma, false),
+
  DEFINE_PROP_END_OF_LIST(),
  };
  

Re: [RFC 1/5] target/riscv: Add properties for BF16 extensions

[Daniel Henrique Barboza]

On 4/11/23 23:33, Weiwei Li wrote:

Add ext_zfbfmin/zvfbfmin/zvfbfwma properties.
Add require check for BF16 extensions.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---


Reviewed-by: Daniel Henrique Barboza 


  target/riscv/cpu.c | 13 +
  target/riscv/cpu.h |  3 +++
  2 files changed, 16 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 1e97473af2..c19bbb41fb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -874,6 +874,12 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  return;
  }
  
+if ((cpu->cfg.ext_zfbfmin | cpu->cfg.ext_zvfbfmin |

+ cpu->cfg.ext_zvfbfwma) && !cpu->cfg.ext_f) {
+error_setg(errp, "BF16 extensions require F extension");
+return;
+}
+
  if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
  error_setg(errp, "D extension requires F extension");
  return;
@@ -918,6 +924,13 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
*cpu, Error **errp)
  return;
  }
  
+if ((cpu->cfg.ext_zvfbfmin | cpu->cfg.ext_zvfbfwma) &&

+!cpu->cfg.ext_zve32f) {
+error_setg(errp, "Zvfbfmin/Zvfbfwma extensions require Zve32f "
+ "extension");
+return;
+}
+
  /* Set the ISA extensions, checks should have happened above */
  if (cpu->cfg.ext_zhinx) {
  cpu->cfg.ext_zhinxmin = true;
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..6c99a82624 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -462,6 +462,7 @@ struct RISCVCPUConfig {
  bool ext_svpbmt;
  bool ext_zdinx;
  bool ext_zawrs;
+bool ext_zfbfmin;
  bool ext_zfh;
  bool ext_zfhmin;
  bool ext_zfinx;
@@ -471,6 +472,8 @@ struct RISCVCPUConfig {
  bool ext_zve64f;
  bool ext_zve64d;
  bool ext_zmmul;
+bool ext_zvfbfmin;
+bool ext_zvfbfwma;
  bool ext_zvfh;
  bool ext_zvfhmin;
  bool ext_smaia;

Re: [PATCH v2 52/54] tcg/riscv: Simplify constraints on qemu_ld/st

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

The softmmu tlb uses TCG_REG_TMP[0-2], not any of the normally available
registers.  Now that we handle overlap betwen inputs and helper arguments,
we can allow any allocatable reg.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/riscv/tcg-target-con-set.h |  2 --
  tcg/riscv/tcg-target-con-str.h |  1 -
  tcg/riscv/tcg-target.c.inc | 16 +++-
  3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/tcg/riscv/tcg-target-con-set.h b/tcg/riscv/tcg-target-con-set.h
index c11710d117..1a8b8e9f2b 100644
--- a/tcg/riscv/tcg-target-con-set.h
+++ b/tcg/riscv/tcg-target-con-set.h
@@ -10,11 +10,9 @@
   * tcg-target-con-str.h; the constraint combination is inclusive or.
   */
  C_O0_I1(r)
-C_O0_I2(LZ, L)
  C_O0_I2(rZ, r)
  C_O0_I2(rZ, rZ)
  C_O0_I4(rZ, rZ, rZ, rZ)
-C_O1_I1(r, L)
  C_O1_I1(r, r)
  C_O1_I2(r, r, ri)
  C_O1_I2(r, r, rI)
diff --git a/tcg/riscv/tcg-target-con-str.h b/tcg/riscv/tcg-target-con-str.h
index 8d8afaee53..6f1cfb976c 100644
--- a/tcg/riscv/tcg-target-con-str.h
+++ b/tcg/riscv/tcg-target-con-str.h
@@ -9,7 +9,6 @@
   * REGS(letter, register_mask)
   */
  REGS('r', ALL_GENERAL_REGS)
-REGS('L', ALL_GENERAL_REGS & ~SOFTMMU_RESERVE_REGS)
  
  /*

   * Define constraint letters for constants:
diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 425ea8902e..35f04ddda9 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -125,17 +125,7 @@ static TCGReg tcg_target_call_oarg_reg(TCGCallReturnKind 
kind, int slot)
  #define TCG_CT_CONST_N12   0x400
  #define TCG_CT_CONST_M12   0x800
  
-#define ALL_GENERAL_REGS  MAKE_64BIT_MASK(0, 32)

-/*
- * For softmmu, we need to avoid conflicts with the first 5
- * argument registers to call the helper.  Some of these are
- * also used for the tlb lookup.
- */
-#ifdef CONFIG_SOFTMMU
-#define SOFTMMU_RESERVE_REGS  MAKE_64BIT_MASK(TCG_REG_A0, 5)
-#else
-#define SOFTMMU_RESERVE_REGS  0
-#endif
+#define ALL_GENERAL_REGS   MAKE_64BIT_MASK(0, 32)
  
  #define sextreg  sextract64
  
@@ -1653,10 +1643,10 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode op)
  
  case INDEX_op_qemu_ld_i32:

  case INDEX_op_qemu_ld_i64:
-return C_O1_I1(r, L);
+return C_O1_I1(r, r);
  case INDEX_op_qemu_st_i32:
  case INDEX_op_qemu_st_i64:
-return C_O0_I2(LZ, L);
+return C_O0_I2(rZ, r);
  
  default:

  g_assert_not_reached();

Re: [PATCH v2 43/54] tcg/riscv: Convert tcg_out_qemu_{ld,st}_slow_path

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

Use tcg_out_ld_helper_args, tcg_out_ld_helper_ret,
and tcg_out_st_helper_args.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/riscv/tcg-target.c.inc | 37 ++---
  1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index d4134bc86f..425ea8902e 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -994,14 +994,14 @@ static void add_qemu_ldst_label(TCGContext *s, int is_ld, 
MemOpIdx oi,
  label->label_ptr[0] = label_ptr[0];
  }
  
+/* We have three temps, we might as well expose them. */

+static const TCGLdstHelperParam ldst_helper_param = {
+.ntmp = 3, .tmp = { TCG_REG_TMP0, TCG_REG_TMP1, TCG_REG_TMP2 }
+};
+
  static bool tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *l)
  {
-MemOpIdx oi = l->oi;
-MemOp opc = get_memop(oi);
-TCGReg a0 = tcg_target_call_iarg_regs[0];
-TCGReg a1 = tcg_target_call_iarg_regs[1];
-TCGReg a2 = tcg_target_call_iarg_regs[2];
-TCGReg a3 = tcg_target_call_iarg_regs[3];
+MemOp opc = get_memop(l->oi);
  
  /* resolve label address */

  if (!reloc_sbimm12(l->label_ptr[0], tcg_splitwx_to_rx(s->code_ptr))) {
@@ -1009,13 +1009,9 @@ static bool tcg_out_qemu_ld_slow_path(TCGContext *s, 
TCGLabelQemuLdst *l)
  }
  
  /* call load helper */

-tcg_out_mov(s, TCG_TYPE_PTR, a0, TCG_AREG0);
-tcg_out_mov(s, TCG_TYPE_PTR, a1, l->addrlo_reg);
-tcg_out_movi(s, TCG_TYPE_PTR, a2, oi);
-tcg_out_movi(s, TCG_TYPE_PTR, a3, (tcg_target_long)l->raddr);
-
+tcg_out_ld_helper_args(s, l, _helper_param);
  tcg_out_call_int(s, qemu_ld_helpers[opc & MO_SSIZE], false);
-tcg_out_mov(s, (opc & MO_SIZE) == MO_64, l->datalo_reg, a0);
+tcg_out_ld_helper_ret(s, l, true, _helper_param);
  
  tcg_out_goto(s, l->raddr);

  return true;
@@ -1023,14 +1019,7 @@ static bool tcg_out_qemu_ld_slow_path(TCGContext *s, 
TCGLabelQemuLdst *l)
  
  static bool tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *l)

  {
-MemOpIdx oi = l->oi;
-MemOp opc = get_memop(oi);
-MemOp s_bits = opc & MO_SIZE;
-TCGReg a0 = tcg_target_call_iarg_regs[0];
-TCGReg a1 = tcg_target_call_iarg_regs[1];
-TCGReg a2 = tcg_target_call_iarg_regs[2];
-TCGReg a3 = tcg_target_call_iarg_regs[3];
-TCGReg a4 = tcg_target_call_iarg_regs[4];
+MemOp opc = get_memop(l->oi);
  
  /* resolve label address */

  if (!reloc_sbimm12(l->label_ptr[0], tcg_splitwx_to_rx(s->code_ptr))) {
@@ -1038,13 +1027,7 @@ static bool tcg_out_qemu_st_slow_path(TCGContext *s, 
TCGLabelQemuLdst *l)
  }
  
  /* call store helper */

-tcg_out_mov(s, TCG_TYPE_PTR, a0, TCG_AREG0);
-tcg_out_mov(s, TCG_TYPE_PTR, a1, l->addrlo_reg);
-tcg_out_movext(s, s_bits == MO_64 ? TCG_TYPE_I64 : TCG_TYPE_I32, a2,
-   l->type, s_bits, l->datalo_reg);
-tcg_out_movi(s, TCG_TYPE_PTR, a3, oi);
-tcg_out_movi(s, TCG_TYPE_PTR, a4, (tcg_target_long)l->raddr);
-
+tcg_out_st_helper_args(s, l, _helper_param);
  tcg_out_call_int(s, qemu_st_helpers[opc & MO_SIZE], false);
  
  tcg_out_goto(s, l->raddr);

Re: [PATCH v2 28/54] tcg/riscv: Rationalize args to tcg_out_qemu_{ld,st}

[Daniel Henrique Barboza]

On 4/10/23 22:04, Richard Henderson wrote:

Interpret the variable argument placement in the caller.
Mark the argument registers const, because they must be passed to
add_qemu_ldst_label unmodified.

Pass data_type instead of is64 -- there are several places where
we already convert back from bool to type.  Clean things up by
using type throughout.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/riscv/tcg-target.c.inc | 68 +++---
  1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 1edc3b1c4d..d4134bc86f 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -1101,7 +1101,7 @@ static bool tcg_out_qemu_st_slow_path(TCGContext *s, 
TCGLabelQemuLdst *l)
  #endif /* CONFIG_SOFTMMU */
  
  static void tcg_out_qemu_ld_direct(TCGContext *s, TCGReg val,

-   TCGReg base, MemOp opc, bool is_64)
+   TCGReg base, MemOp opc, TCGType type)
  {
  /* Byte swapping is left to middle-end expansion. */
  tcg_debug_assert((opc & MO_BSWAP) == 0);
@@ -1120,7 +1120,7 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGReg 
val,
  tcg_out_opc_imm(s, OPC_LH, val, base, 0);
  break;
  case MO_UL:
-if (is_64) {
+if (type == TCG_TYPE_I64) {
  tcg_out_opc_imm(s, OPC_LWU, val, base, 0);
  break;
  }
@@ -1136,30 +1136,22 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, 
TCGReg val,
  }
  }
  
-static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)

+static void tcg_out_qemu_ld(TCGContext *s, const TCGReg data_reg,
+const TCGReg addr_reg, const MemOpIdx oi,
+TCGType data_type)
  {
-TCGReg addr_reg, data_reg;
-MemOpIdx oi;
-MemOp opc;
-#if defined(CONFIG_SOFTMMU)
-tcg_insn_unit *label_ptr[1];
-#else
-unsigned a_bits;
-#endif
+MemOp opc = get_memop(oi);
  TCGReg base;
  
-data_reg = *args++;

-addr_reg = *args++;
-oi = *args++;
-opc = get_memop(oi);
-
  #if defined(CONFIG_SOFTMMU)
+tcg_insn_unit *label_ptr[1];
+
  base = tcg_out_tlb_load(s, addr_reg, oi, label_ptr, 1);
-tcg_out_qemu_ld_direct(s, data_reg, base, opc, is_64);
-add_qemu_ldst_label(s, 1, oi, (is_64 ? TCG_TYPE_I64 : TCG_TYPE_I32),
-data_reg, addr_reg, s->code_ptr, label_ptr);
+tcg_out_qemu_ld_direct(s, data_reg, base, opc, data_type);
+add_qemu_ldst_label(s, true, oi, data_type, data_reg, addr_reg,
+s->code_ptr, label_ptr);
  #else
-a_bits = get_alignment_bits(opc);
+unsigned a_bits = get_alignment_bits(opc);
  if (a_bits) {
  tcg_out_test_alignment(s, true, addr_reg, a_bits);
  }
@@ -1172,7 +1164,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg 
*args, bool is_64)
  tcg_out_opc_reg(s, OPC_ADD, TCG_REG_TMP0, TCG_GUEST_BASE_REG, base);
  base = TCG_REG_TMP0;
  }
-tcg_out_qemu_ld_direct(s, data_reg, base, opc, is_64);
+tcg_out_qemu_ld_direct(s, data_reg, base, opc, data_type);
  #endif
  }
  
@@ -1200,30 +1192,22 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGReg val,

  }
  }
  
-static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64)

+static void tcg_out_qemu_st(TCGContext *s, const TCGReg data_reg,
+const TCGReg addr_reg, const MemOpIdx oi,
+TCGType data_type)
  {
-TCGReg addr_reg, data_reg;
-MemOpIdx oi;
-MemOp opc;
-#if defined(CONFIG_SOFTMMU)
-tcg_insn_unit *label_ptr[1];
-#else
-unsigned a_bits;
-#endif
+MemOp opc = get_memop(oi);
  TCGReg base;
  
-data_reg = *args++;

-addr_reg = *args++;
-oi = *args++;
-opc = get_memop(oi);
-
  #if defined(CONFIG_SOFTMMU)
+tcg_insn_unit *label_ptr[1];
+
  base = tcg_out_tlb_load(s, addr_reg, oi, label_ptr, 0);
  tcg_out_qemu_st_direct(s, data_reg, base, opc);
-add_qemu_ldst_label(s, 0, oi, (is_64 ? TCG_TYPE_I64 : TCG_TYPE_I32),
-data_reg, addr_reg, s->code_ptr, label_ptr);
+add_qemu_ldst_label(s, false, oi, data_type, data_reg, addr_reg,
+s->code_ptr, label_ptr);
  #else
-a_bits = get_alignment_bits(opc);
+unsigned a_bits = get_alignment_bits(opc);
  if (a_bits) {
  tcg_out_test_alignment(s, false, addr_reg, a_bits);
  }
@@ -1528,16 +1512,16 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
  break;
  
  case INDEX_op_qemu_ld_i32:

-tcg_out_qemu_ld(s, args, false);
+tcg_out_qemu_ld(s, a0, a1, a2, TCG_TYPE_I32);
  break;
  case INDEX_op_qemu_ld_i64:
-tcg_out_qemu_ld(s, args, true);
+tcg_out_qemu_ld(s, a0, a1, a2, TCG_TYPE_I64);
  break;
  case INDEX_op_qemu_st_i32:
-

Re: [PATCH v2 27/54] tcg/riscv: Require TCG_TARGET_REG_BITS == 64

[Daniel Henrique Barboza]

On 4/10/23 22:04, Richard Henderson wrote:

The port currently does not support "oversize" guests, which
means riscv32 can only target 32-bit guests.  We will soon be
building TCG once for all guests.  This implies that we can
only support riscv64.

Since all Linux distributions target riscv64 not riscv32,
this is not much of a restriction and simplifies the code.


Code looks good but I got confused about the riscv32 implications you cited.

Does this means that if someone happens to have a risc-v 32 bit host, with a
special Linux sauce that runs on that 32 bit risc-v host, this person won't be
able to build the riscv32 TCG target in that machine?


Daniel



Signed-off-by: Richard Henderson 
---
  tcg/riscv/tcg-target-con-set.h |   6 -
  tcg/riscv/tcg-target.h |  22 ++--
  tcg/riscv/tcg-target.c.inc | 206 ++---
  3 files changed, 72 insertions(+), 162 deletions(-)

diff --git a/tcg/riscv/tcg-target-con-set.h b/tcg/riscv/tcg-target-con-set.h
index cf0ac4d751..c11710d117 100644
--- a/tcg/riscv/tcg-target-con-set.h
+++ b/tcg/riscv/tcg-target-con-set.h
@@ -13,18 +13,12 @@ C_O0_I1(r)
  C_O0_I2(LZ, L)
  C_O0_I2(rZ, r)
  C_O0_I2(rZ, rZ)
-C_O0_I3(LZ, L, L)
-C_O0_I3(LZ, LZ, L)
-C_O0_I4(LZ, LZ, L, L)
  C_O0_I4(rZ, rZ, rZ, rZ)
  C_O1_I1(r, L)
  C_O1_I1(r, r)
-C_O1_I2(r, L, L)
  C_O1_I2(r, r, ri)
  C_O1_I2(r, r, rI)
  C_O1_I2(r, rZ, rN)
  C_O1_I2(r, rZ, rZ)
  C_O1_I4(r, rZ, rZ, rZ, rZ)
-C_O2_I1(r, r, L)
-C_O2_I2(r, r, L, L)
  C_O2_I4(r, r, rZ, rZ, rM, rM)
diff --git a/tcg/riscv/tcg-target.h b/tcg/riscv/tcg-target.h
index 0deb33701f..dddf2486c1 100644
--- a/tcg/riscv/tcg-target.h
+++ b/tcg/riscv/tcg-target.h
@@ -25,11 +25,14 @@
  #ifndef RISCV_TCG_TARGET_H
  #define RISCV_TCG_TARGET_H
  
-#if __riscv_xlen == 32

-# define TCG_TARGET_REG_BITS 32
-#elif __riscv_xlen == 64
-# define TCG_TARGET_REG_BITS 64
+/*
+ * We don't support oversize guests.
+ * Since we will only build tcg once, this in turn requires a 64-bit host.
+ */
+#if __riscv_xlen != 64
+#error "unsupported code generation mode"
  #endif
+#define TCG_TARGET_REG_BITS 64
  
  #define TCG_TARGET_INSN_UNIT_SIZE 4

  #define TCG_TARGET_TLB_DISPLACEMENT_BITS 20
@@ -83,13 +86,8 @@ typedef enum {
  #define TCG_TARGET_STACK_ALIGN  16
  #define TCG_TARGET_CALL_STACK_OFFSET0
  #define TCG_TARGET_CALL_ARG_I32 TCG_CALL_ARG_NORMAL
-#if TCG_TARGET_REG_BITS == 32
-#define TCG_TARGET_CALL_ARG_I64 TCG_CALL_ARG_EVEN
-#define TCG_TARGET_CALL_ARG_I128TCG_CALL_ARG_EVEN
-#else
  #define TCG_TARGET_CALL_ARG_I64 TCG_CALL_ARG_NORMAL
  #define TCG_TARGET_CALL_ARG_I128TCG_CALL_ARG_NORMAL
-#endif
  #define TCG_TARGET_CALL_RET_I128TCG_CALL_RET_NORMAL
  
  /* optional instructions */

@@ -106,8 +104,8 @@ typedef enum {
  #define TCG_TARGET_HAS_sub2_i32 1
  #define TCG_TARGET_HAS_mulu2_i320
  #define TCG_TARGET_HAS_muls2_i320
-#define TCG_TARGET_HAS_muluh_i32(TCG_TARGET_REG_BITS == 32)
-#define TCG_TARGET_HAS_mulsh_i32(TCG_TARGET_REG_BITS == 32)
+#define TCG_TARGET_HAS_muluh_i320
+#define TCG_TARGET_HAS_mulsh_i320
  #define TCG_TARGET_HAS_ext8s_i321
  #define TCG_TARGET_HAS_ext16s_i32   1
  #define TCG_TARGET_HAS_ext8u_i321
@@ -128,7 +126,6 @@ typedef enum {
  #define TCG_TARGET_HAS_setcond2 1
  #define TCG_TARGET_HAS_qemu_st8_i32 0
  
-#if TCG_TARGET_REG_BITS == 64

  #define TCG_TARGET_HAS_movcond_i64  0
  #define TCG_TARGET_HAS_div_i64  1
  #define TCG_TARGET_HAS_rem_i64  1
@@ -165,7 +162,6 @@ typedef enum {
  #define TCG_TARGET_HAS_muls2_i640
  #define TCG_TARGET_HAS_muluh_i641
  #define TCG_TARGET_HAS_mulsh_i641
-#endif
  
  #define TCG_TARGET_DEFAULT_MO (0)
  
diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc

index 266fe1433d..1edc3b1c4d 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -137,15 +137,7 @@ static TCGReg tcg_target_call_oarg_reg(TCGCallReturnKind 
kind, int slot)
  #define SOFTMMU_RESERVE_REGS  0
  #endif
  
-

-static inline tcg_target_long sextreg(tcg_target_long val, int pos, int len)
-{
-if (TCG_TARGET_REG_BITS == 32) {
-return sextract32(val, pos, len);
-} else {
-return sextract64(val, pos, len);
-}
-}
+#define sextreg  sextract64
  
  /* test if a constant matches the constraint */

  static bool tcg_target_const_match(int64_t val, TCGType type, int ct)
@@ -235,7 +227,6 @@ typedef enum {
  OPC_XOR = 0x4033,
  OPC_XORI = 0x4013,
  
-#if TCG_TARGET_REG_BITS == 64

  OPC_ADDIW = 0x1b,
  OPC_ADDW = 0x3b,
  OPC_DIVUW = 0x200503b,
@@ -250,23 +241,6 @@ typedef enum {
  OPC_SRLIW = 0x501b,
  OPC_SRLW = 0x503b,
  OPC_SUBW = 0x403b,
-#else
-/* Simplify code throughout by defining aliases for RV32.  */
-OPC_ADDIW = OPC_ADDI,
-OPC_ADDW = OPC_ADD,
-OPC_DIVUW = OPC_DIVU,
-OPC_DIVW = OPC_DIV,
-OPC_MULW = OPC_MUL,

Re: [PATCH for-8.0 0/5] Xen emulation build/Coverity fixes

[Stefano Stabellini]

On Wed, 11 Apr 2023, David Woodhouse wrote:
> Some Coverity fixes and minor cleanups. And most notably, dropping
> support for Xen libraries older than 4.7.1.

I just wanted to say that I am fine with this

Re: [PATCH v2 12/54] tcg/riscv: Conditionalize tcg_out_exts_i32_i64

[Daniel Henrique Barboza]

On 4/10/23 22:04, Richard Henderson wrote:

Since TCG_TYPE_I32 values are kept sign-extended in registers,
via "w" instructions, we need not extend if the register matches.


Perhaps "we don't need to extend if ..." ?


This is already relied upon by comparisons.

Signed-off-by: Richard Henderson 
---



Reviewed-by: Daniel Henrique Barboza 




  tcg/riscv/tcg-target.c.inc | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
index 7bd3b421ad..2b9aab29ec 100644
--- a/tcg/riscv/tcg-target.c.inc
+++ b/tcg/riscv/tcg-target.c.inc
@@ -604,7 +604,9 @@ static void tcg_out_ext32s(TCGContext *s, TCGReg ret, 
TCGReg arg)
  
  static void tcg_out_exts_i32_i64(TCGContext *s, TCGReg ret, TCGReg arg)

  {
-tcg_out_ext32s(s, ret, arg);
+if (ret != arg) {
+tcg_out_ext32s(s, ret, arg);
+}
  }
  
  static void tcg_out_ldst(TCGContext *s, RISCVInsn opc, TCGReg data,

Re: [PATCH 2/2] migration/ram.c: Fix migration with compress enabled

[Juan Quintela]

Lukas Straub  wrote:
> Since ec6f3ab9, migration with compress enabled was broken, because
> the compress threads use a dummy QEMUFile which just acts as a
> buffer and that commit accidentally changed it to use the outgoing
> migration channel instead.
>
> Fix this by using the dummy file again in the compress threads.
>
> Signed-off-by: Lukas Straub 

Reviewed-by: Juan Quintela 
queued.

Ouch, ouch.

Good catch.

The other patch (the test) needs to wait util 8.1 is open.

Later, Juan.

Re: [PULL for-8.0 0/1] NFS changes for 2023-04-12

[Peter Maydell]

On Wed, 12 Apr 2023 at 17:30, Paolo Bonzini  wrote:
>
> The following changes since commit abb02ce0e76a8e00026699a863ab2d11d88f56d4:
>
>   Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging 
> (2023-04-11 16:19:06 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 3fe64abcde55cf6f4ea5883106301baad219a7cc:
>
>   block/nfs: do not poll within a coroutine (2023-04-12 18:26:51 +0200)
>
> 
> Fix NFS driver issue.
>
> 


Applied, thanks.

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

-- PMM

Re: [PATCH for-8.0 2/3] migration: Fix potential race on postcopy_qemufile_src

[Juan Quintela]

Peter Xu  wrote:
> postcopy_qemufile_src object should be owned by one thread, either the main
> thread (e.g. when at the beginning, or at the end of migration), or by the
> return path thread (when during a preempt enabled postcopy migration).  If
> that's not the case the access to the object might be racy.
>
> postcopy_preempt_shutdown_file() can be potentially racy, because it's
> called at the end phase of migration on the main thread, however during
> which the return path thread hasn't yet been recycled; the recycle happens
> in await_return_path_close_on_source() which is after this point.
>
> It means, logically it's posslbe the main thread and the return path thread
> are both operating on the same qemufile.  While I don't think qemufile is
> thread safe at all.
>
> postcopy_preempt_shutdown_file() used to be needed because that's where we
> send EOS to dest so that dest can safely shutdown the preempt thread.
>
> To avoid the possible race, remove this only place that a race can happen.
> Instead we figure out another way to safely close the preempt thread on
> dest.
>
> The core idea during postcopy on deciding "when to stop" is that dest will
> send a postcopy SHUT message to src, telling src that all data is there.
> Hence to shut the dest preempt thread maybe better to do it directly on
> dest node.
>
> This patch proposed such a way that we change postcopy_prio_thread_created
> into PreemptThreadStatus, so that we kick the preempt thread on dest qemu
> by a sequence of:
>
>   mis->preempt_thread_status = PREEMPT_THREAD_QUIT;
>   qemu_file_shutdown(mis->postcopy_qemufile_dst);
>
> While here shutdown() is probably so far the easiest way to kick preempt
> thread from a blocked qemu_get_be64().  Then it reads preempt_thread_status
> to make sure it's not a network failure but a willingness to quit the
> thread.
>
> We could have avoided that extra status but just rely on migration status.
> The problem is postcopy_ram_incoming_cleanup() is just called early enough
> so we're still during POSTCOPY_ACTIVE no matter what.. So just make it
> simple to have the status introduced.
>
> One flag x-preempt-pre-7-2 is added to keep old pre-7.2 behaviors of
> postcopy preempt.
>
> Fixes: 9358982744 ("migration: Send requested page directly in rp-return 
> thread")
> Signed-off-by: Peter Xu 
> ---
>  hw/core/machine.c|  1 +
>  migration/migration.c| 10 --
>  migration/migration.h| 34 +-
>  migration/postcopy-ram.c | 20 +++-
>  4 files changed, 57 insertions(+), 8 deletions(-)
>

As preempt is pretty new I will 

Reviewed-by: Juan Quintela 

But code is quite subtle.
queued.

Re: [PATCH for-8.0 3/3] migration: Recover behavior of preempt channel creation for pre-7.2

[Juan Quintela]

Peter Xu  wrote:
> In 8.0 devel window we reworked preempt channel creation, so that there'll
> be no race condition when the migration channel and preempt channel got
> established in the wrong order in commit 5655aab079.
>
> However no one noticed that the change will also be not compatible with
> older qemus, majorly 7.1/7.2 versions where preempt mode started to be
> supported.
>
> Leverage the same pre-7.2 flag introduced in the previous patch to recover
> the behavior hopefully before 8.0 releases, so we don't break migration
> when we migrate from 8.0 to older qemu binaries.
>
> Fixes: 5655aab079 ("migration: Postpone postcopy preempt channel to be after 
> main")
> Signed-off-by: Peter Xu 

Reviewed-by: Juan Quintela 
queued.

Re: [PATCH for-8.0 1/3] io: tls: Inherit QIO_CHANNEL_FEATURE_SHUTDOWN on server side

[Juan Quintela]

Peter Xu  wrote:
> TLS iochannel will inherit io_shutdown() from the master ioc, however we
> missed to do that on the server side.
>
> This will e.g. allow qemu_file_shutdown() to work on dest QEMU too for
> migration.
>
> Acked-by: Daniel P. Berrangé 
> Signed-off-by: Peter Xu 

Reviewed-by: Juan Quintela 


queued

Re: [PATCH v2 51/54] tcg/ppc: Remove unused constraints A, B, C, D

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

These constraints have not been used for quite some time.

Fixes: 77b73de67632 ("Use rem/div[u]_i32 drop div[u]2_i32")
Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/ppc/tcg-target-con-str.h | 4 
  1 file changed, 4 deletions(-)

diff --git a/tcg/ppc/tcg-target-con-str.h b/tcg/ppc/tcg-target-con-str.h
index f3bf030bc3..9dcbc3df50 100644
--- a/tcg/ppc/tcg-target-con-str.h
+++ b/tcg/ppc/tcg-target-con-str.h
@@ -10,10 +10,6 @@
   */
  REGS('r', ALL_GENERAL_REGS)
  REGS('v', ALL_VECTOR_REGS)
-REGS('A', 1u << TCG_REG_R3)
-REGS('B', 1u << TCG_REG_R4)
-REGS('C', 1u << TCG_REG_R5)
-REGS('D', 1u << TCG_REG_R6)
  
  /*

   * Define constraint letters for constants:

Re: [PATCH v2 49/54] tcg/ppc: Reorg tcg_out_tlb_read

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

Allocate TCG_REG_TMP2.  Use R0, TMP1, TMP2 instead of any of
the normally allocated registers for the tlb load.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/ppc/tcg-target.c.inc | 84 +++-
  1 file changed, 49 insertions(+), 35 deletions(-)

diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index 1b60166d2f..613cd73583 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -68,6 +68,7 @@
  #else
  # define TCG_REG_TMP1   TCG_REG_R12
  #endif
+#define TCG_REG_TMP2TCG_REG_R11
  
  #define TCG_VEC_TMP1TCG_REG_V0

  #define TCG_VEC_TMP2TCG_REG_V1
@@ -2007,10 +2008,11 @@ static void * const qemu_st_helpers[(MO_SIZE | 
MO_BSWAP) + 1] = {
  QEMU_BUILD_BUG_ON(TLB_MASK_TABLE_OFS(0) > 0);
  QEMU_BUILD_BUG_ON(TLB_MASK_TABLE_OFS(0) < -32768);
  
-/* Perform the TLB load and compare.  Places the result of the comparison

-   in CR7, loads the addend of the TLB into R3, and returns the register
-   containing the guest address (zero-extended into R4).  Clobbers R0 and R2. 
*/
-
+/*
+ * Perform the TLB load and compare.  Places the result of the comparison
+ * in CR7, loads the addend of the TLB into TMP1, and returns the register
+ * containing the guest address (zero-extended into TMP2).  Clobbers R0.
+ */
  static TCGReg tcg_out_tlb_read(TCGContext *s, MemOp opc,
 TCGReg addrlo, TCGReg addrhi,
 int mem_index, bool is_read)
@@ -2026,40 +2028,44 @@ static TCGReg tcg_out_tlb_read(TCGContext *s, MemOp opc,
  unsigned a_bits = get_alignment_bits(opc);
  
  /* Load tlb_mask[mmu_idx] and tlb_table[mmu_idx].  */

-tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_R3, TCG_AREG0, mask_off);
-tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_R4, TCG_AREG0, table_off);
+tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP1, TCG_AREG0, mask_off);
+tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP2, TCG_AREG0, table_off);
  
  /* Extract the page index, shifted into place for tlb index.  */

  if (TCG_TARGET_REG_BITS == 32) {
-tcg_out_shri32(s, TCG_REG_TMP1, addrlo,
+tcg_out_shri32(s, TCG_REG_R0, addrlo,
 TARGET_PAGE_BITS - CPU_TLB_ENTRY_BITS);
  } else {
-tcg_out_shri64(s, TCG_REG_TMP1, addrlo,
+tcg_out_shri64(s, TCG_REG_R0, addrlo,
 TARGET_PAGE_BITS - CPU_TLB_ENTRY_BITS);
  }
-tcg_out32(s, AND | SAB(TCG_REG_R3, TCG_REG_R3, TCG_REG_TMP1));
+tcg_out32(s, AND | SAB(TCG_REG_TMP1, TCG_REG_TMP1, TCG_REG_R0));
  
-/* Load the TLB comparator.  */

+/* Load the (low part) TLB comparator into TMP2. */
  if (cmp_off == 0 && TCG_TARGET_REG_BITS >= TARGET_LONG_BITS) {
  uint32_t lxu = (TCG_TARGET_REG_BITS == 32 || TARGET_LONG_BITS == 32
  ? LWZUX : LDUX);
-tcg_out32(s, lxu | TAB(TCG_REG_TMP1, TCG_REG_R3, TCG_REG_R4));
+tcg_out32(s, lxu | TAB(TCG_REG_TMP2, TCG_REG_TMP1, TCG_REG_TMP2));
  } else {
-tcg_out32(s, ADD | TAB(TCG_REG_R3, TCG_REG_R3, TCG_REG_R4));
+tcg_out32(s, ADD | TAB(TCG_REG_TMP1, TCG_REG_TMP1, TCG_REG_TMP2));
  if (TCG_TARGET_REG_BITS < TARGET_LONG_BITS) {
-tcg_out_ld(s, TCG_TYPE_I32, TCG_REG_TMP1, TCG_REG_R3, cmp_off + 4);
-tcg_out_ld(s, TCG_TYPE_I32, TCG_REG_R4, TCG_REG_R3, cmp_off);
+tcg_out_ld(s, TCG_TYPE_I32, TCG_REG_TMP2,
+   TCG_REG_TMP1, cmp_off + 4);
  } else {
-tcg_out_ld(s, TCG_TYPE_TL, TCG_REG_TMP1, TCG_REG_R3, cmp_off);
+tcg_out_ld(s, TCG_TYPE_TL, TCG_REG_TMP2, TCG_REG_TMP1, cmp_off);
  }
  }
  
-/* Load the TLB addend for use on the fast path.  Do this asap

-   to minimize any load use delay.  */
-tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_R3, TCG_REG_R3,
-   offsetof(CPUTLBEntry, addend));
+/*
+ * Load the TLB addend for use on the fast path.
+ * Do this asap to minimize any load use delay.
+ */
+if (TCG_TARGET_REG_BITS >= TARGET_LONG_BITS) {
+tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP1, TCG_REG_TMP1,
+   offsetof(CPUTLBEntry, addend));
+}
  
-/* Clear the non-page, non-alignment bits from the address */

+/* Clear the non-page, non-alignment bits from the address into R0. */
  if (TCG_TARGET_REG_BITS == 32) {
  /* We don't support unaligned accesses on 32-bits.
   * Preserve the bottom bits and thus trigger a comparison
@@ -2090,9 +2096,6 @@ static TCGReg tcg_out_tlb_read(TCGContext *s, MemOp opc,
  if (TARGET_LONG_BITS == 32) {
  tcg_out_rlw(s, RLWINM, TCG_REG_R0, t, 0,
  (32 - a_bits) & 31, 31 - TARGET_PAGE_BITS);
-/* Zero-extend the address for use in the final address.  */
-tcg_out_ext32u(s, TCG_REG_R4, addrlo);
-addrlo = TCG_REG_R4;
  } else 

Re: [PATCH v2 50/54] tcg/ppc: Adjust constraints on qemu_ld/st

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

The softmmu tlb uses TCG_REG_{TMP1,TMP2,R0}, not any of the normally
available registers.  Now that we handle overlap betwen inputs and
helper arguments, we can allow any allocatable reg.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/ppc/tcg-target-con-set.h | 11 ---
  tcg/ppc/tcg-target-con-str.h |  2 --
  tcg/ppc/tcg-target.c.inc | 32 ++--
  3 files changed, 14 insertions(+), 31 deletions(-)

diff --git a/tcg/ppc/tcg-target-con-set.h b/tcg/ppc/tcg-target-con-set.h
index a1a345883d..f206b29205 100644
--- a/tcg/ppc/tcg-target-con-set.h
+++ b/tcg/ppc/tcg-target-con-set.h
@@ -12,18 +12,15 @@
  C_O0_I1(r)
  C_O0_I2(r, r)
  C_O0_I2(r, ri)
-C_O0_I2(S, S)
  C_O0_I2(v, r)
-C_O0_I3(S, S, S)
+C_O0_I3(r, r, r)
  C_O0_I4(r, r, ri, ri)
-C_O0_I4(S, S, S, S)
-C_O1_I1(r, L)
+C_O0_I4(r, r, r, r)
  C_O1_I1(r, r)
  C_O1_I1(v, r)
  C_O1_I1(v, v)
  C_O1_I1(v, vr)
  C_O1_I2(r, 0, rZ)
-C_O1_I2(r, L, L)
  C_O1_I2(r, rI, ri)
  C_O1_I2(r, rI, rT)
  C_O1_I2(r, r, r)
@@ -36,7 +33,7 @@ C_O1_I2(v, v, v)
  C_O1_I3(v, v, v, v)
  C_O1_I4(r, r, ri, rZ, rZ)
  C_O1_I4(r, r, r, ri, ri)
-C_O2_I1(L, L, L)
-C_O2_I2(L, L, L, L)
+C_O2_I1(r, r, r)
+C_O2_I2(r, r, r, r)
  C_O2_I4(r, r, rI, rZM, r, r)
  C_O2_I4(r, r, r, r, rI, rZM)
diff --git a/tcg/ppc/tcg-target-con-str.h b/tcg/ppc/tcg-target-con-str.h
index 298ca20d5b..f3bf030bc3 100644
--- a/tcg/ppc/tcg-target-con-str.h
+++ b/tcg/ppc/tcg-target-con-str.h
@@ -14,8 +14,6 @@ REGS('A', 1u << TCG_REG_R3)
  REGS('B', 1u << TCG_REG_R4)
  REGS('C', 1u << TCG_REG_R5)
  REGS('D', 1u << TCG_REG_R6)
-REGS('L', ALL_QLOAD_REGS)
-REGS('S', ALL_QSTORE_REGS)
  
  /*

   * Define constraint letters for constants:
diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index 613cd73583..e94f3131a3 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -93,18 +93,6 @@
  #define ALL_GENERAL_REGS  0xu
  #define ALL_VECTOR_REGS   0xull
  
-#ifdef CONFIG_SOFTMMU

-#define ALL_QLOAD_REGS \
-(ALL_GENERAL_REGS & \
- ~((1 << TCG_REG_R3) | (1 << TCG_REG_R4) | (1 << TCG_REG_R5)))
-#define ALL_QSTORE_REGS \
-(ALL_GENERAL_REGS & ~((1 << TCG_REG_R3) | (1 << TCG_REG_R4) | \
-  (1 << TCG_REG_R5) | (1 << TCG_REG_R6)))
-#else
-#define ALL_QLOAD_REGS  (ALL_GENERAL_REGS & ~(1 << TCG_REG_R3))
-#define ALL_QSTORE_REGS ALL_QLOAD_REGS
-#endif
-
  TCGPowerISA have_isa;
  static bool have_isel;
  bool have_altivec;
@@ -3791,23 +3779,23 @@ static TCGConstraintSetIndex 
tcg_target_op_def(TCGOpcode op)
  
  case INDEX_op_qemu_ld_i32:

  return (TCG_TARGET_REG_BITS == 64 || TARGET_LONG_BITS == 32
-? C_O1_I1(r, L)
-: C_O1_I2(r, L, L));
+? C_O1_I1(r, r)
+: C_O1_I2(r, r, r));
  
  case INDEX_op_qemu_st_i32:

  return (TCG_TARGET_REG_BITS == 64 || TARGET_LONG_BITS == 32
-? C_O0_I2(S, S)
-: C_O0_I3(S, S, S));
+? C_O0_I2(r, r)
+: C_O0_I3(r, r, r));
  
  case INDEX_op_qemu_ld_i64:

-return (TCG_TARGET_REG_BITS == 64 ? C_O1_I1(r, L)
-: TARGET_LONG_BITS == 32 ? C_O2_I1(L, L, L)
-: C_O2_I2(L, L, L, L));
+return (TCG_TARGET_REG_BITS == 64 ? C_O1_I1(r, r)
+: TARGET_LONG_BITS == 32 ? C_O2_I1(r, r, r)
+: C_O2_I2(r, r, r, r));
  
  case INDEX_op_qemu_st_i64:

-return (TCG_TARGET_REG_BITS == 64 ? C_O0_I2(S, S)
-: TARGET_LONG_BITS == 32 ? C_O0_I3(S, S, S)
-: C_O0_I4(S, S, S, S));
+return (TCG_TARGET_REG_BITS == 64 ? C_O0_I2(r, r)
+: TARGET_LONG_BITS == 32 ? C_O0_I3(r, r, r)
+: C_O0_I4(r, r, r, r));
  
  case INDEX_op_add_vec:

  case INDEX_op_sub_vec:

Re: [PATCH for-8.0 0/5] Xen emulation build/Coverity fixes

[David Woodhouse]

On Wed, 2023-04-12 at 20:01 +0100, David Woodhouse wrote:
> On Wed, 2023-04-12 at 19:55 +0100, Peter Maydell wrote:
> > On Wed, 12 Apr 2023 at 19:52, David Woodhouse  wrote:
> > > 
> > > Some Coverity fixes and minor cleanups. And most notably, dropping
> > > support for Xen libraries older than 4.7.1.
> > > 
> > > I believe there are two issues that remain to be fixed. The x32 build
> > > fails, and I've seen patches which attempt to detect x32 and disable
> > > the Xen emulation. Along with assertions that we just shouldn't care.
> > > I don't have a strong opinion either way but it seems to be in hand.
> > > 
> > > The other is the question of what Xen *actually* does if you try to
> > > unmap an IRQ_MSI_EMU PIRQ. I don't think Linux guests try that, and
> > > I'm fairly sure Windows doesn't even use MSI→PIRQ mappings in the
> > > first place, and I doubt any other guests care either. I'd like to
> > > establish the 'correct' behaviour and implement it, ideally before
> > > the 8.0 release, but it's going to take me a few days more.
> > > 
> > > David Woodhouse (5):
> > >   hw/xen: Simplify emulated Xen platform init
> > >   hw/xen: Fix memory leak in libxenstore_open() for Xen
> > >   xen: Drop support for Xen versions below 4.7.1
> > >   hw/xen: Fix double-free in xen_console store_con_info()
> > >   hw/xen: Fix broken check for invalid state in xs_be_open()
> > > 
> > 
> > This is highly unlikely to make 8.0 at this point, FYI.
> > If there's anything in this you think is super-critical we
> > might be able to sneak it in.
> 
> Nothing is super-critical except maybe the double-free in
> store_con_info(). That could lead to a crash on startup if the QEMU Xen
> console is being used.

Although we could just do the one-liner that drops the extra 'free'
instead of converting to g_autoptr.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v2 42/54] tcg/ppc: Convert tcg_out_qemu_{ld,st}_slow_path

[Daniel Henrique Barboza]

On 4/10/23 22:05, Richard Henderson wrote:

Use tcg_out_ld_helper_args, tcg_out_ld_helper_ret,
and tcg_out_st_helper_args.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/ppc/tcg-target.c.inc | 88 
  1 file changed, 26 insertions(+), 62 deletions(-)

diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index 90093a6509..1b60166d2f 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -2137,44 +2137,38 @@ static void add_qemu_ldst_label(TCGContext *s, bool 
is_ld,
  label->label_ptr[0] = lptr;
  }
  
+static TCGReg ldst_ra_gen(TCGContext *s, const TCGLabelQemuLdst *l, int arg)

+{
+if (arg < 0) {
+arg = TCG_REG_TMP1;
+}
+tcg_out32(s, MFSPR | RT(arg) | LR);
+return arg;
+}
+
+/*
+ * For the purposes of ppc32 sorting 4 input registers into 4 argument
+ * registers, there is an outside chance we would require 3 temps.
+ * Because of constraints, no inputs are in r3, and env will not be
+ * placed into r3 until after the sorting is done, and is thus free.
+ */
+static const TCGLdstHelperParam ldst_helper_param = {
+.ra_gen = ldst_ra_gen,
+.ntmp = 3,
+.tmp = { TCG_REG_TMP1, TCG_REG_R0, TCG_REG_R3 }
+};
+
  static bool tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
  {
-MemOpIdx oi = lb->oi;
-MemOp opc = get_memop(oi);
-TCGReg hi, lo, arg = TCG_REG_R3;
+MemOp opc = get_memop(lb->oi);
  
  if (!reloc_pc14(lb->label_ptr[0], tcg_splitwx_to_rx(s->code_ptr))) {

  return false;
  }
  
-tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);

-
-lo = lb->addrlo_reg;
-hi = lb->addrhi_reg;
-if (TCG_TARGET_REG_BITS < TARGET_LONG_BITS) {
-arg |= (TCG_TARGET_CALL_ARG_I64 == TCG_CALL_ARG_EVEN);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, hi);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, lo);
-} else {
-/* If the address needed to be zero-extended, we'll have already
-   placed it in R4.  The only remaining case is 64-bit guest.  */
-tcg_out_mov(s, TCG_TYPE_TL, arg++, lo);
-}
-
-tcg_out_movi(s, TCG_TYPE_I32, arg++, oi);
-tcg_out32(s, MFSPR | RT(arg) | LR);
-
+tcg_out_ld_helper_args(s, lb, _helper_param);
  tcg_out_call_int(s, LK, qemu_ld_helpers[opc & (MO_BSWAP | MO_SIZE)]);
-
-lo = lb->datalo_reg;
-hi = lb->datahi_reg;
-if (TCG_TARGET_REG_BITS == 32 && (opc & MO_SIZE) == MO_64) {
-tcg_out_mov(s, TCG_TYPE_I32, lo, TCG_REG_R4);
-tcg_out_mov(s, TCG_TYPE_I32, hi, TCG_REG_R3);
-} else {
-tcg_out_movext(s, lb->type, lo,
-   TCG_TYPE_REG, opc & MO_SSIZE, TCG_REG_R3);
-}
+tcg_out_ld_helper_ret(s, lb, false, _helper_param);
  
  tcg_out_b(s, 0, lb->raddr);

  return true;
@@ -2182,43 +2176,13 @@ static bool tcg_out_qemu_ld_slow_path(TCGContext *s, 
TCGLabelQemuLdst *lb)
  
  static bool tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)

  {
-MemOpIdx oi = lb->oi;
-MemOp opc = get_memop(oi);
-MemOp s_bits = opc & MO_SIZE;
-TCGReg hi, lo, arg = TCG_REG_R3;
+MemOp opc = get_memop(lb->oi);
  
  if (!reloc_pc14(lb->label_ptr[0], tcg_splitwx_to_rx(s->code_ptr))) {

  return false;
  }
  
-tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);

-
-lo = lb->addrlo_reg;
-hi = lb->addrhi_reg;
-if (TCG_TARGET_REG_BITS < TARGET_LONG_BITS) {
-arg |= (TCG_TARGET_CALL_ARG_I64 == TCG_CALL_ARG_EVEN);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, hi);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, lo);
-} else {
-/* If the address needed to be zero-extended, we'll have already
-   placed it in R4.  The only remaining case is 64-bit guest.  */
-tcg_out_mov(s, TCG_TYPE_TL, arg++, lo);
-}
-
-lo = lb->datalo_reg;
-hi = lb->datahi_reg;
-if (TCG_TARGET_REG_BITS == 32 && s_bits == MO_64) {
-arg |= (TCG_TARGET_CALL_ARG_I64 == TCG_CALL_ARG_EVEN);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, hi);
-tcg_out_mov(s, TCG_TYPE_I32, arg++, lo);
-} else {
-tcg_out_movext(s, s_bits == MO_64 ? TCG_TYPE_I64 : TCG_TYPE_I32,
-   arg++, lb->type, s_bits, lo);
-}
-
-tcg_out_movi(s, TCG_TYPE_I32, arg++, oi);
-tcg_out32(s, MFSPR | RT(arg) | LR);
-
+tcg_out_st_helper_args(s, lb, _helper_param);
  tcg_out_call_int(s, LK, qemu_st_helpers[opc & (MO_BSWAP | MO_SIZE)]);
  
  tcg_out_b(s, 0, lb->raddr);

Re: [PATCH v2 25/54] tcg/ppc: Rationalize args to tcg_out_qemu_{ld,st}

[Daniel Henrique Barboza]

On 4/10/23 22:04, Richard Henderson wrote:

Interpret the variable argument placement in the caller.
Mark the argument register const, because they must be passed to
add_qemu_ldst_label unmodified.  This requires a bit of local
variable renaming, because addrlo was being modified.

Pass data_type instead of is64 -- there are several places where
we already convert back from bool to type.  Clean things up by
using type throughout.

Signed-off-by: Richard Henderson 
---


Reviewed-by: Daniel Henrique Barboza 


  tcg/ppc/tcg-target.c.inc | 164 +--
  1 file changed, 89 insertions(+), 75 deletions(-)

diff --git a/tcg/ppc/tcg-target.c.inc b/tcg/ppc/tcg-target.c.inc
index 77abb7d20c..90093a6509 100644
--- a/tcg/ppc/tcg-target.c.inc
+++ b/tcg/ppc/tcg-target.c.inc
@@ -2118,7 +2118,8 @@ static TCGReg tcg_out_tlb_read(TCGContext *s, MemOp opc,
  /* Record the context of a call to the out of line helper code for the slow
 path for a load or store, so that we can later generate the correct
 helper code.  */
-static void add_qemu_ldst_label(TCGContext *s, bool is_ld, MemOpIdx oi,
+static void add_qemu_ldst_label(TCGContext *s, bool is_ld,
+TCGType type, MemOpIdx oi,
  TCGReg datalo_reg, TCGReg datahi_reg,
  TCGReg addrlo_reg, TCGReg addrhi_reg,
  tcg_insn_unit *raddr, tcg_insn_unit *lptr)
@@ -2126,6 +2127,7 @@ static void add_qemu_ldst_label(TCGContext *s, bool 
is_ld, MemOpIdx oi,
  TCGLabelQemuLdst *label = new_ldst_label(s);
  
  label->is_ld = is_ld;

+label->type = type;
  label->oi = oi;
  label->datalo_reg = datalo_reg;
  label->datahi_reg = datahi_reg;
@@ -2288,30 +2290,19 @@ static bool tcg_out_qemu_st_slow_path(TCGContext *s, 
TCGLabelQemuLdst *l)
  
  #endif /* SOFTMMU */
  
-static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)

+static void tcg_out_qemu_ld(TCGContext *s,
+const TCGReg datalo, const TCGReg datahi,
+const TCGReg addrlo, const TCGReg addrhi,
+const MemOpIdx oi, TCGType data_type)
  {
-TCGReg datalo, datahi, addrlo, rbase;
-TCGReg addrhi __attribute__((unused));
-MemOpIdx oi;
-MemOp opc, s_bits;
+MemOp opc = get_memop(oi);
+MemOp s_bits = opc & MO_SIZE;
+TCGReg rbase, index;
+
  #ifdef CONFIG_SOFTMMU
-int mem_index;
  tcg_insn_unit *label_ptr;
-#else
-unsigned a_bits;
-#endif
  
-datalo = *args++;

-datahi = (TCG_TARGET_REG_BITS == 32 && is_64 ? *args++ : 0);
-addrlo = *args++;
-addrhi = (TCG_TARGET_REG_BITS < TARGET_LONG_BITS ? *args++ : 0);
-oi = *args++;
-opc = get_memop(oi);
-s_bits = opc & MO_SIZE;
-
-#ifdef CONFIG_SOFTMMU
-mem_index = get_mmuidx(oi);
-addrlo = tcg_out_tlb_read(s, opc, addrlo, addrhi, mem_index, true);
+index = tcg_out_tlb_read(s, opc, addrlo, addrhi, get_mmuidx(oi), true);
  
  /* Load a pointer into the current opcode w/conditional branch-link. */

  label_ptr = s->code_ptr;
@@ -2319,80 +2310,71 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg 
*args, bool is_64)
  
  rbase = TCG_REG_R3;

  #else  /* !CONFIG_SOFTMMU */
-a_bits = get_alignment_bits(opc);
+unsigned a_bits = get_alignment_bits(opc);
  if (a_bits) {
  tcg_out_test_alignment(s, true, addrlo, addrhi, a_bits);
  }
  rbase = guest_base ? TCG_GUEST_BASE_REG : 0;
  if (TCG_TARGET_REG_BITS > TARGET_LONG_BITS) {
  tcg_out_ext32u(s, TCG_REG_TMP1, addrlo);
-addrlo = TCG_REG_TMP1;
+index = TCG_REG_TMP1;
+} else {
+index = addrlo;
  }
  #endif
  
  if (TCG_TARGET_REG_BITS == 32 && s_bits == MO_64) {

  if (opc & MO_BSWAP) {
-tcg_out32(s, ADDI | TAI(TCG_REG_R0, addrlo, 4));
-tcg_out32(s, LWBRX | TAB(datalo, rbase, addrlo));
+tcg_out32(s, ADDI | TAI(TCG_REG_R0, index, 4));
+tcg_out32(s, LWBRX | TAB(datalo, rbase, index));
  tcg_out32(s, LWBRX | TAB(datahi, rbase, TCG_REG_R0));
  } else if (rbase != 0) {
-tcg_out32(s, ADDI | TAI(TCG_REG_R0, addrlo, 4));
-tcg_out32(s, LWZX | TAB(datahi, rbase, addrlo));
+tcg_out32(s, ADDI | TAI(TCG_REG_R0, index, 4));
+tcg_out32(s, LWZX | TAB(datahi, rbase, index));
  tcg_out32(s, LWZX | TAB(datalo, rbase, TCG_REG_R0));
-} else if (addrlo == datahi) {
-tcg_out32(s, LWZ | TAI(datalo, addrlo, 4));
-tcg_out32(s, LWZ | TAI(datahi, addrlo, 0));
+} else if (index == datahi) {
+tcg_out32(s, LWZ | TAI(datalo, index, 4));
+tcg_out32(s, LWZ | TAI(datahi, index, 0));
  } else {
-tcg_out32(s, LWZ | TAI(datahi, addrlo, 0));
-tcg_out32(s, LWZ | TAI(datalo, addrlo, 4));
+tcg_out32(s, LWZ | 

Re: [PATCH for-8.0 0/5] Xen emulation build/Coverity fixes

[David Woodhouse]

On Wed, 2023-04-12 at 19:55 +0100, Peter Maydell wrote:
> On Wed, 12 Apr 2023 at 19:52, David Woodhouse  wrote:
> > 
> > Some Coverity fixes and minor cleanups. And most notably, dropping
> > support for Xen libraries older than 4.7.1.
> > 
> > I believe there are two issues that remain to be fixed. The x32 build
> > fails, and I've seen patches which attempt to detect x32 and disable
> > the Xen emulation. Along with assertions that we just shouldn't care.
> > I don't have a strong opinion either way but it seems to be in hand.
> > 
> > The other is the question of what Xen *actually* does if you try to
> > unmap an IRQ_MSI_EMU PIRQ. I don't think Linux guests try that, and
> > I'm fairly sure Windows doesn't even use MSI→PIRQ mappings in the
> > first place, and I doubt any other guests care either. I'd like to
> > establish the 'correct' behaviour and implement it, ideally before
> > the 8.0 release, but it's going to take me a few days more.
> > 
> > David Woodhouse (5):
> >   hw/xen: Simplify emulated Xen platform init
> >   hw/xen: Fix memory leak in libxenstore_open() for Xen
> >   xen: Drop support for Xen versions below 4.7.1
> >   hw/xen: Fix double-free in xen_console store_con_info()
> >   hw/xen: Fix broken check for invalid state in xs_be_open()
> > 
> 
> This is highly unlikely to make 8.0 at this point, FYI.
> If there's anything in this you think is super-critical we
> might be able to sneak it in.

Nothing is super-critical except maybe the double-free in
store_con_info(). That could lead to a crash on startup if the QEMU Xen
console is being used.


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 4/5] hw/xen: Fix double-free in xen_console store_con_info()

[Peter Maydell]

On Wed, 12 Apr 2023 at 19:52, David Woodhouse  wrote:
>
> From: David Woodhouse 
>
> Coverity spotted a double-free (CID 1508254); we g_string_free(path) and
> then for some reason immediately call free(path) too.
>
> We should just use g_autoptr() for it anyway, which simplifies the code
> a bit.
>
> Fixes: 7a8a749da7d3 ("hw/xen: Move xenstore_store_pv_console_info to 
> xen_console.c")
> Signed-off-by: David Woodhouse 

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 5/5] hw/xen: Fix broken check for invalid state in xs_be_open()

[Peter Maydell]

On Wed, 12 Apr 2023 at 19:52, David Woodhouse  wrote:
>
> From: David Woodhouse 
>
> Coverity points out that if (!s && !s->impl) isn't really what we intended
> to do here. CID 1508131.
>
> Fixes: 032475127225 ("hw/xen: Add emulated implementation of XenStore 
> operations")
> Signed-off-by: David Woodhouse 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH for-8.0 0/5] Xen emulation build/Coverity fixes

[Peter Maydell]

On Wed, 12 Apr 2023 at 19:52, David Woodhouse  wrote:
>
> Some Coverity fixes and minor cleanups. And most notably, dropping
> support for Xen libraries older than 4.7.1.
>
> I believe there are two issues that remain to be fixed. The x32 build
> fails, and I've seen patches which attempt to detect x32 and disable
> the Xen emulation. Along with assertions that we just shouldn't care.
> I don't have a strong opinion either way but it seems to be in hand.
>
> The other is the question of what Xen *actually* does if you try to
> unmap an IRQ_MSI_EMU PIRQ. I don't think Linux guests try that, and
> I'm fairly sure Windows doesn't even use MSI→PIRQ mappings in the
> first place, and I doubt any other guests care either. I'd like to
> establish the 'correct' behaviour and implement it, ideally before
> the 8.0 release, but it's going to take me a few days more.
>
> David Woodhouse (5):
>   hw/xen: Simplify emulated Xen platform init
>   hw/xen: Fix memory leak in libxenstore_open() for Xen
>   xen: Drop support for Xen versions below 4.7.1
>   hw/xen: Fix double-free in xen_console store_con_info()
>   hw/xen: Fix broken check for invalid state in xs_be_open()
>

This is highly unlikely to make 8.0 at this point, FYI.
If there's anything in this you think is super-critical we
might be able to sneak it in.

thanks
-- PMM

Re: [PULL 22/27] hw/xen: Add emulated implementation of XenStore operations

[Peter Maydell]

On Wed, 12 Apr 2023 at 19:22, David Woodhouse  wrote:
>
> On Tue, 2023-04-11 at 19:07 +0100, Peter Maydell wrote:
> >
> >
> > > +static void xs_be_unwatch(struct qemu_xs_handle *h, struct
> > > qemu_xs_watch *w)
> > > +{
> > > +xs_impl_unwatch(h->impl, DOMID_QEMU, w->path, NULL,
> > > xs_be_watch_cb, w);
> >
> > Coverity points out that this is the only call to xs_impl_unwatch()
> > where we don't check the return value. Is there some useful way
> > we can report the error, or is it a "we're closing everything down
> > anyway, no way to report anything" situation? (This particular
> > Coverity heuristic is quite prone to false positives, so if that's
> > the way it is I'll just mark it as a f-p in the coverity UI.)
>
> This is because the Xen libxenstore API doesn't return an error, and
> this is the ops function which emulates that same API. I suppose we
> could explicitly cast to void with a comment to that effect, to avoid
> having it linger in Coverity? I think that's sufficient to make
> Coverity shut up, isn't it?

I've just marked it a false-positive in the UI. Coverity's generally
good at not resurfacing old false-positives, so don't bother
changing the code unless you think it would improve clarity for
a human reader.

thanks
-- PMM

[PATCH 1/5] hw/xen: Simplify emulated Xen platform init

[David Woodhouse]

From: David Woodhouse 

I initially put the basic platform init (overlay pages, grant tables,
event channels) into mc->kvm_type because that was the earliest place
that could sensibly test for xen_mode==XEN_EMULATE.

The intent was to do this early enough that we could then initialise the
XenBus and other parts which would have depended on them, from a generic
location for both Xen and KVM/Xen in the PC-specific code, as seen in
https://lore.kernel.org/qemu-devel/20230116221919.1124201-16-dw...@infradead.org/

However, then the Xen on Arm patches came along, and *they* wanted to
do the XenBus init from a 'generic' Xen-specific location instead:
https://lore.kernel.org/qemu-devel/20230210222729.957168-4-sstabell...@kernel.org/

Since there's no generic location that covers all three, I conceded to
do it for XEN_EMULATE mode in pc_basic_devices_init().

And now there's absolutely no point in having some of the platform init
done from pc_machine_kvm_type(); we can move it all up to live in a
single place in pc_basic_devices_init(). This has the added benefit that
we can drop the separate xen_evtchn_connect_gsis() function completely,
and pass just the system GSIs in directly to xen_evtchn_create().

While I'm at it, it does no harm to explicitly pass in the *number* of
said GSIs, because it does make me twitch a bit to pass an array of
impicit size. During the lifetime of the KVM/Xen patchset, that had
already changed (albeit just cosmetically) from GSI_NUM_PINS to
IOAPIC_NUM_PINS.

And document a bit better that this is for the *output* GSI for raising
CPU0's events when the per-CPU vector isn't available. The fact that
we create a whole set of them and then only waggle the one we're told
to, instead of having a single output and only *connecting* it to the
GSI that it should be connected to, is still non-intuitive for me.

Signed-off-by: David Woodhouse 
Reviewed-by: Paul Durrant 
---
 hw/i386/kvm/xen_evtchn.c | 40 
 hw/i386/kvm/xen_evtchn.h |  3 +--
 hw/i386/pc.c | 13 -
 3 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 3048329474..3d810dbd59 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -147,7 +147,10 @@ struct XenEvtchnState {
 QemuMutex port_lock;
 uint32_t nr_ports;
 XenEvtchnPort port_table[EVTCHN_2L_NR_CHANNELS];
-qemu_irq gsis[IOAPIC_NUM_PINS];
+
+/* Connected to the system GSIs for raising callback as GSI / INTx */
+unsigned int nr_callback_gsis;
+qemu_irq *callback_gsis;
 
 struct xenevtchn_handle *be_handles[EVTCHN_2L_NR_CHANNELS];
 
@@ -299,7 +302,7 @@ static void gsi_assert_bh(void *opaque)
 }
 }
 
-void xen_evtchn_create(void)
+void xen_evtchn_create(unsigned int nr_gsis, qemu_irq *system_gsis)
 {
 XenEvtchnState *s = XEN_EVTCHN(sysbus_create_simple(TYPE_XEN_EVTCHN,
 -1, NULL));
@@ -310,8 +313,19 @@ void xen_evtchn_create(void)
 qemu_mutex_init(>port_lock);
 s->gsi_bh = aio_bh_new(qemu_get_aio_context(), gsi_assert_bh, s);
 
-for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-sysbus_init_irq(SYS_BUS_DEVICE(s), >gsis[i]);
+/*
+ * These are the *output* GSI from event channel support, for
+ * signalling CPU0's events via GSI or PCI INTx instead of the
+ * per-CPU vector. We create a *set* of irqs and connect one to
+ * each of the system GSIs which were passed in from the platform
+ * code, and then just trigger the right one as appropriate from
+ * xen_evtchn_set_callback_level().
+ */
+s->nr_callback_gsis = nr_gsis;
+s->callback_gsis = g_new0(qemu_irq, nr_gsis);
+for (i = 0; i < nr_gsis; i++) {
+sysbus_init_irq(SYS_BUS_DEVICE(s), >callback_gsis[i]);
+sysbus_connect_irq(SYS_BUS_DEVICE(s), i, system_gsis[i]);
 }
 
 /*
@@ -336,20 +350,6 @@ void xen_evtchn_create(void)
 xen_evtchn_ops = _evtchn_backend_ops;
 }
 
-void xen_evtchn_connect_gsis(qemu_irq *system_gsis)
-{
-XenEvtchnState *s = xen_evtchn_singleton;
-int i;
-
-if (!s) {
-return;
-}
-
-for (i = 0; i < IOAPIC_NUM_PINS; i++) {
-sysbus_connect_irq(SYS_BUS_DEVICE(s), i, system_gsis[i]);
-}
-}
-
 static void xen_evtchn_register_types(void)
 {
 type_register_static(_evtchn_info);
@@ -430,8 +430,8 @@ void xen_evtchn_set_callback_level(int level)
 return;
 }
 
-if (s->callback_gsi && s->callback_gsi < IOAPIC_NUM_PINS) {
-qemu_set_irq(s->gsis[s->callback_gsi], level);
+if (s->callback_gsi && s->callback_gsi < s->nr_callback_gsis) {
+qemu_set_irq(s->callback_gsis[s->callback_gsi], level);
 if (level) {
 /* Ensure the vCPU polls for deassertion */
 kvm_xen_set_callback_asserted();
diff --git a/hw/i386/kvm/xen_evtchn.h b/hw/i386/kvm/xen_evtchn.h
index bfb67ac2bc..b740acfc0d 100644
--- 

[PATCH 4/5] hw/xen: Fix double-free in xen_console store_con_info()

[David Woodhouse]

From: David Woodhouse 

Coverity spotted a double-free (CID 1508254); we g_string_free(path) and
then for some reason immediately call free(path) too.

We should just use g_autoptr() for it anyway, which simplifies the code
a bit.

Fixes: 7a8a749da7d3 ("hw/xen: Move xenstore_store_pv_console_info to 
xen_console.c")
Signed-off-by: David Woodhouse 
---
 hw/char/xen_console.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index c7a19c0e7c..810dae3f44 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -178,8 +178,7 @@ static int store_con_info(struct XenConsole *con)
 Chardev *cs = qemu_chr_fe_get_driver(>chr);
 char *pts = NULL;
 char *dom_path;
-GString *path;
-int ret = -1;
+g_autoptr(GString) path = NULL;
 
 /* Only continue if we're talking to a pty. */
 if (!CHARDEV_IS_PTY(cs)) {
@@ -204,15 +203,9 @@ static int store_con_info(struct XenConsole *con)
 
 if (xenstore_write_str(con->console, path->str, pts)) {
 fprintf(stderr, "xenstore_write_str for '%s' fail", path->str);
-goto out;
+return -1;
 }
-ret = 0;
-
-out:
-g_string_free(path, true);
-free(path);
-
-return ret;
+return 0;
 }
 
 static int con_init(struct XenLegacyDevice *xendev)
-- 
2.39.2

[PATCH for-8.0 0/5] Xen emulation build/Coverity fixes

[David Woodhouse]

Some Coverity fixes and minor cleanups. And most notably, dropping
support for Xen libraries older than 4.7.1.

I believe there are two issues that remain to be fixed. The x32 build
fails, and I've seen patches which attempt to detect x32 and disable
the Xen emulation. Along with assertions that we just shouldn't care.
I don't have a strong opinion either way but it seems to be in hand.

The other is the question of what Xen *actually* does if you try to
unmap an IRQ_MSI_EMU PIRQ. I don't think Linux guests try that, and
I'm fairly sure Windows doesn't even use MSI→PIRQ mappings in the
first place, and I doubt any other guests care either. I'd like to
establish the 'correct' behaviour and implement it, ideally before
the 8.0 release, but it's going to take me a few days more.

David Woodhouse (5):
  hw/xen: Simplify emulated Xen platform init
  hw/xen: Fix memory leak in libxenstore_open() for Xen
  xen: Drop support for Xen versions below 4.7.1
  hw/xen: Fix double-free in xen_console store_con_info()
  hw/xen: Fix broken check for invalid state in xs_be_open()

 hw/char/xen_console.c   |  13 ++
 hw/i386/kvm/xen_evtchn.c|  40 -
 hw/i386/kvm/xen_evtchn.h|   3 +-
 hw/i386/kvm/xen_xenstore.c  |   2 +-
 hw/i386/pc.c|  13 ++
 hw/xen/xen-operations.c |  59 +---
 include/hw/xen/xen_native.h | 107 +---
 meson.build |   5 +--
 scripts/xen-detect.c|  60 -
 9 files changed, 33 insertions(+), 269 deletions(-)

[PATCH 5/5] hw/xen: Fix broken check for invalid state in xs_be_open()

[David Woodhouse]

From: David Woodhouse 

Coverity points out that if (!s && !s->impl) isn't really what we intended
to do here. CID 1508131.

Fixes: 032475127225 ("hw/xen: Add emulated implementation of XenStore 
operations")
Signed-off-by: David Woodhouse 
---
 hw/i386/kvm/xen_xenstore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 900679af8a..65f91e87d7 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1688,7 +1688,7 @@ static struct qemu_xs_handle *xs_be_open(void)
 XenXenstoreState *s = xen_xenstore_singleton;
 struct qemu_xs_handle *h;
 
-if (!s && !s->impl) {
+if (!s || !s->impl) {
 errno = -ENOSYS;
 return NULL;
 }
-- 
2.39.2

[PATCH 3/5] xen: Drop support for Xen versions below 4.7.1

[David Woodhouse]

From: David Woodhouse 

In restructuring to allow for internal emulation of Xen functionality,
I broke compatibility for Xen 4.6 and earlier. Fix this by explicitly
removing support for anything older than 4.7.1, which is also ancient
but it does still build, and the compatibility support for it is fairly
unintrusive.

Fixes: 15e283c5b684 ("hw/xen: Add foreignmem operations to allow redirection to 
internal emulation")
Signed-off-by: David Woodhouse 
---
 hw/xen/xen-operations.c |  57 +--
 include/hw/xen/xen_native.h | 107 +---
 meson.build |   5 +-
 scripts/xen-detect.c|  60 
 4 files changed, 3 insertions(+), 226 deletions(-)

diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
index 3d213d28df..e00983ec44 100644
--- a/hw/xen/xen-operations.c
+++ b/hw/xen/xen-operations.c
@@ -28,46 +28,13 @@
 #include 
 
 /*
- * We don't support Xen prior to 4.2.0.
+ * We don't support Xen prior to 4.7.1.
  */
 
-/* Xen 4.2 through 4.6 */
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
-
-typedef xc_evtchn xenevtchn_handle;
-typedef evtchn_port_or_error_t xenevtchn_port_or_error_t;
-
-#define xenevtchn_open(l, f) xc_evtchn_open(l, f);
-#define xenevtchn_close(h) xc_evtchn_close(h)
-#define xenevtchn_fd(h) xc_evtchn_fd(h)
-#define xenevtchn_pending(h) xc_evtchn_pending(h)
-#define xenevtchn_notify(h, p) xc_evtchn_notify(h, p)
-#define xenevtchn_bind_interdomain(h, d, p) xc_evtchn_bind_interdomain(h, d, p)
-#define xenevtchn_unmask(h, p) xc_evtchn_unmask(h, p)
-#define xenevtchn_unbind(h, p) xc_evtchn_unbind(h, p)
-
-typedef xc_gnttab xengnttab_handle;
-
-#define xengnttab_open(l, f) xc_gnttab_open(l, f)
-#define xengnttab_close(h) xc_gnttab_close(h)
-#define xengnttab_set_max_grants(h, n) xc_gnttab_set_max_grants(h, n)
-#define xengnttab_map_grant_ref(h, d, r, p) xc_gnttab_map_grant_ref(h, d, r, p)
-#define xengnttab_unmap(h, a, n) xc_gnttab_munmap(h, a, n)
-#define xengnttab_map_grant_refs(h, c, d, r, p) \
-xc_gnttab_map_grant_refs(h, c, d, r, p)
-#define xengnttab_map_domain_grant_refs(h, c, d, r, p) \
-xc_gnttab_map_domain_grant_refs(h, c, d, r, p)
-
-typedef xc_interface xenforeignmemory_handle;
-
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
-
 #include 
 #include 
 #include 
 
-#endif
-
 /* Xen before 4.8 */
 
 static int libxengnttab_fallback_grant_copy(xengnttab_handle *xgt,
@@ -223,26 +190,6 @@ static struct gnttab_backend_ops libxengnttab_backend_ops 
= {
 .unmap = libxengnttab_backend_unmap,
 };
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
-
-static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot,
-  size_t pages, xfn_pfn_t *pfns,
-  int *errs)
-{
-if (errs) {
-return xc_map_foreign_bulk(xen_xc, dom, prot, pfns, errs, pages);
-} else {
-return xc_map_foreign_pages(xen_xc, dom, prot, pfns, pages);
-}
-}
-
-static int libxenforeignmem_backend_unmap(void *addr, size_t pages)
-{
-return munmap(addr, pages * XC_PAGE_SIZE);
-}
-
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
-
 static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot,
   size_t pages, xen_pfn_t *pfns,
   int *errs)
@@ -256,8 +203,6 @@ static int libxenforeignmem_backend_unmap(void *addr, 
size_t pages)
 return xenforeignmemory_unmap(xen_fmem, addr, pages);
 }
 
-#endif
-
 struct foreignmem_backend_ops libxenforeignmem_backend_ops = {
 .map = libxenforeignmem_backend_map,
 .unmap = libxenforeignmem_backend_unmap,
diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h
index 6bcc83baf9..f11eb423e3 100644
--- a/include/hw/xen/xen_native.h
+++ b/include/hw/xen/xen_native.h
@@ -24,23 +24,11 @@
 extern xc_interface *xen_xc;
 
 /*
- * We don't support Xen prior to 4.2.0.
+ * We don't support Xen prior to 4.7.1.
  */
 
-/* Xen 4.2 through 4.6 */
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
-
-typedef xc_interface xenforeignmemory_handle;
-
-#define xenforeignmemory_open(l, f) xen_xc
-#define xenforeignmemory_close(h)
-
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
-
 #include 
 
-#endif
-
 extern xenforeignmemory_handle *xen_fmem;
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -148,8 +136,6 @@ static inline xendevicemodel_handle *xendevicemodel_open(
 return xen_xc;
 }
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40500
-
 static inline int xendevicemodel_create_ioreq_server(
 xendevicemodel_handle *dmod, domid_t domid, int handle_bufioreq,
 ioservid_t *id)
@@ -211,8 +197,6 @@ static inline int xendevicemodel_set_ioreq_server_state(
 return xc_hvm_set_ioreq_server_state(dmod, domid, id, enabled);
 }
 
-#endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40500 */
-
 static inline int xendevicemodel_set_pci_intx_level(
 

[PATCH 2/5] hw/xen: Fix memory leak in libxenstore_open() for Xen

[David Woodhouse]

From: David Woodhouse 

There was a superfluous allocation of the XS handle, leading to it
being leaked on both the error path and the success path (where it gets
allocated again).

Spotted by Coverity (CID 1508098).

Fixes: ba2a92db1ff6 ("hw/xen: Add xenstore operations to allow redirection to 
internal emulation")
Suggested-by: Peter Maydell 
Signed-off-by: David Woodhouse 
Reviewed-by: Peter Maydell 
---
 hw/xen/xen-operations.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
index 4b78fbf4bd..3d213d28df 100644
--- a/hw/xen/xen-operations.c
+++ b/hw/xen/xen-operations.c
@@ -287,7 +287,7 @@ static void watch_event(void *opaque)
 static struct qemu_xs_handle *libxenstore_open(void)
 {
 struct xs_handle *xsh = xs_open(0);
-struct qemu_xs_handle *h = g_new0(struct qemu_xs_handle, 1);
+struct qemu_xs_handle *h;
 
 if (!xsh) {
 return NULL;
-- 
2.39.2

Re: [PULL 22/27] hw/xen: Add emulated implementation of XenStore operations

[David Woodhouse]

On Tue, 2023-04-11 at 19:07 +0100, Peter Maydell wrote:
> 
> 
> > +static void xs_be_unwatch(struct qemu_xs_handle *h, struct
> > qemu_xs_watch *w)
> > +{
> > +    xs_impl_unwatch(h->impl, DOMID_QEMU, w->path, NULL,
> > xs_be_watch_cb, w);
> 
> Coverity points out that this is the only call to xs_impl_unwatch()
> where we don't check the return value. Is there some useful way
> we can report the error, or is it a "we're closing everything down
> anyway, no way to report anything" situation? (This particular
> Coverity heuristic is quite prone to false positives, so if that's
> the way it is I'll just mark it as a f-p in the coverity UI.)

This is because the Xen libxenstore API doesn't return an error, and
this is the ops function which emulates that same API. I suppose we
could explicitly cast to void with a comment to that effect, to avoid
having it linger in Coverity? I think that's sufficient to make
Coverity shut up, isn't it?


smime.p7s
Description: S/MIME cryptographic signature

Re: qemu v8.0-rc3 fails to compile with Xen

[David Woodhouse]

On Wed, 2023-04-12 at 18:32 +0100, Peter Maydell wrote:
> On Wed, 12 Apr 2023 at 17:58, David Woodhouse 
> wrote:
> > 
> > On Wed, 2023-04-12 at 18:41 +0200, Olaf Hering wrote:
> > > The error with this patch applied looks like this:
> > 
> > Thanks. So I think I fixed that one but there are others. I'll make
> > myself a build environment rather than getting you to help me play
> > whack-a-mole.
> 
> Is it worth the effort if this version of Xen
> is old enough that it's outside our standard supported
> build platforms policy?
> https://www.qemu.org/docs/master/about/build-platforms.html
> You could just remove all the no-longer-needed compat code
> instead...

Indeed. I'll rip out everything before 4.7.1, which is plenty ancient
enough and the compat support for it is fairly minimal (and builds fine
here).


smime.p7s
Description: S/MIME cryptographic signature

Re: qemu v8.0-rc3 fails to compile with Xen

[Peter Maydell]

On Wed, 12 Apr 2023 at 17:58, David Woodhouse  wrote:
>
> On Wed, 2023-04-12 at 18:41 +0200, Olaf Hering wrote:
> > The error with this patch applied looks like this:
>
> Thanks. So I think I fixed that one but there are others. I'll make
> myself a build environment rather than getting you to help me play
> whack-a-mole.

Is it worth the effort if this version of Xen
is old enough that it's outside our standard supported
build platforms policy?
https://www.qemu.org/docs/master/about/build-platforms.html
You could just remove all the no-longer-needed compat code
instead...

thanks
-- PMM

Re: [PATCH v12 02/10] target/riscv: add support for Zca extension

[Daniel Henrique Barboza]

On 4/12/23 08:35, Weiwei Li wrote:


On 2023/4/12 18:55, Alistair Francis wrote:

On Wed, Apr 12, 2023 at 12:55 PM Weiwei Li  wrote:


On 2023/4/12 10:12, Alistair Francis wrote:

On Fri, Apr 7, 2023 at 6:23 AM Daniel Henrique Barboza
 wrote:

Hi,

This patch is going to break the sifive_u boot if I rebase

"[PATCH v6 0/9] target/riscv: rework CPU extensions validation"

on top of it, as it is the case today with the current riscv-to-apply.next.

The reason is that the priv spec version for Zca is marked as 1_12_0, and
the priv spec version for both sifive CPUs is 1_10_0, and both are enabling
RVC.

This patch from that series above:

"[PATCH v6 5/9] target/riscv/cpu.c: add priv_spec validate/disable_exts helpers"

Makes the disabling of the extension based on priv version to happen *after* we
do all the validations, instead of before as we're doing today. Zca (and Zcd) 
will
be manually enabled just to be disabled shortly after by the priv spec code. And
this will happen:

qemu-system-riscv64: warning: disabling zca extension for hart 
0x because privilege spec version does not match
qemu-system-riscv64: warning: disabling zca extension for hart 
0x0001 because privilege spec version does not match
qemu-system-riscv64: warning: disabling zcd extension for hart 
0x0001 because privilege spec version does not match
(--- hangs ---)

This means that the assumption made in this patch - that Zca implies RVC - is no
longer valid, and all these translations won't work.

Thanks for catching and reporting this


Some possible solutions:

- Do not use Zca as a synonym for RVC, i.e. drop this patch. We would need to 
convert
all Zca checks to RVC checks in all translation code.

This to me seems like the best solution

I had also implemented a patch for this solution. I'll sent it later.

Thanks!


However, this will introduce additional check. i.e. check both Zca and C
or , both Zcf/d and CF/CD.

Is there a large performance penalty for that?


May not very large.  Just one or two additional check in instruction 
translation. You can see the patch at:

https://lore.kernel.org/qemu-devel/20230412030648.80470-1-liwei...@iscas.ac.cn/




I think this is not very necessary. Implcitly-enabled extensions is
often the subsets of existed extension.

Yeah, I see what you are saying. It just becomes difficult to manage
though. It all worked fine until there are conflicts between the priv
specs.


So enabling them will introduce no additional function to the cpus.

We can just make them invisible to user(mask them in the isa string)
instead of disabling them  to be compatible with lower priv version.

I'm open to other options, but masking them like this seems like more
work and also seems confusing. The extension will end up enabled in
QEMU and potentially visible to external tools, but then just not
exposed to the guest. It seems prone to future bugs.


Yeah. they may be visible to external tools if they read cfg directly.

Another way is to introduce another parameter instead of cfg.ext_zca to 
indicate whether Zca/Zcf/Zcd

instructions are enabled. This is be done by patchset:

https://lore.kernel.org/qemu-devel/20230410033526.31708-1-liwei...@iscas.ac.cn/

All of the three ways are acceptable to me.


Earlier today I grabbed two Weiwei Li patches (isa_string changes and 
Zca/Zcf/Zcd
changes) in the "rework CPU extensions validation" series, but after reading 
these
last messages I guess I jumped the gun.

Alistair, I'm considering drop the patch that disables extensions via priv_spec 
later
during realize() (the one I mentioned in my first message) from that series 
until we
figure the best way of dealing with priv spec and Z-extensions being used as 
MISA bits
and so on. We can merge those cleanups and write_misa() changes that are 
already reviewed
in the meantime. Are you ok with that?


Thanks,


Daniel
 



Regards,

Weiwei Li



Alistair


Regards,

Weiwei Li



- Do not apply patch 5/9 from that series that moves the disable_ext code to 
the end
of validation. Also a possibility, but we would be sweeping the problem under 
the rug.
Zca still can't be used as a RVC replacement due to priv spec version 
constraints, but
we just won't disable Zca because we'll keep validating exts too early (which 
is the
problem that the patch addresses).

- change the priv spec of the sifive CPUs - and everyone that uses RVC -  to 
1_12_0. Not
sure if this makes sense.

- do not disable any extensions due to privilege spec version mismatch. This 
would make
all the priv_version related artifacts to be more "educational" than to be an 
actual
configuration we want to enforce. Not sure if that would do any good in the 
end, but
it's also a possibility.

This also seems like something we can do. Printing a warning but
continuing on seems reasonable to me. That allows users to
enable/disable features even if the versions don't match.

I don't think this is the solution for this problem though

Re: [PATCH] target/riscv: Update check for Zca/zcf/zcd

[Daniel Henrique Barboza]

On 4/12/23 00:06, Weiwei Li wrote:

Even though Zca/Zcf/Zcd can be included by C/F/D, however, their priv
version is higher than the priv version of C/F/D. So if we use check
for them instead of check for C/F/D totally, it will trigger new
problem when we try to disable the extensions based on the configured
priv version.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---


Two things:

- the patch fails checkpatch.pl. I fixed it in my tree, but in case the patch
needs a new version entirely here's the error:

v7-0005-target-riscv-Mask-the-implicitly-enabled-extensio.patch has no obvious 
style problems and is ready for submission.

Checking v7-0006-target-riscv-Update-check-for-Zca-zcf-zcd.patch...
ERROR: space required before the open parenthesis '('
#36: FILE: target/riscv/insn_trans/trans_rvd.c.inc:36:
+if(!has_ext(ctx, RVD) || !has_ext(ctx, RVC)) { \

ERROR: space required before the open parenthesis '('
#72: FILE: target/riscv/insn_trans/trans_rvf.c.inc:35:
+if(!has_ext(ctx, RVF) || !has_ext(ctx, RVC)) { \



- yesterday Richard sent the following review in the patch "[RFC PATCH 3/4]
target/riscv: check smstateen fcsr flag":





+#define REQUIRE_ZFINX_OR_F(ctx) do { \
+if (!has_ext(ctx, RVF)) { \
+if (!ctx->cfg_ptr->ext_zfinx) { \
+return false; \
+} \
+smstateen_fcsr_check(ctx); \
  } \
  } while (0)


As a matter of style, I strongly object to a *nested* macro returning from the 
calling function.  These should all be changed to normal functions of the form

if (!require_xyz(ctx) || !require_abc(ctx)) {
return something;
}

etc.  insn_trans/trans_rvv.c.inc is much much cleaner in this respect.



I believe his comment is also valid for this patch as well due to how
REQUIRE_ZCD_OR_DC(ctx) and REQUIRE_ZCF_OR_FC(ctx) is implemented. Before
re-sending this patch as is it's better to check with him now.

Richard, does this patch use the nested macro style you strongly object?


Thanks,


Daniel



  target/riscv/insn_trans/trans_rvd.c.inc | 12 +++-
  target/riscv/insn_trans/trans_rvf.c.inc | 14 --
  target/riscv/insn_trans/trans_rvi.c.inc |  5 +++--
  target/riscv/translate.c|  5 +++--
  4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
b/target/riscv/insn_trans/trans_rvd.c.inc
index 2c51e01c40..f8d0ae48c7 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -31,9 +31,11 @@
  } \
  } while (0)
  
-#define REQUIRE_ZCD(ctx) do { \

-if (!ctx->cfg_ptr->ext_zcd) {  \
-return false; \
+#define REQUIRE_ZCD_OR_DC(ctx) do { \
+if (!ctx->cfg_ptr->ext_zcd) { \
+if(!has_ext(ctx, RVD) || !has_ext(ctx, RVC)) { \
+return false; \
+} \
  } \
  } while (0)
  
@@ -67,13 +69,13 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
  
  static bool trans_c_fld(DisasContext *ctx, arg_fld *a)

  {
-REQUIRE_ZCD(ctx);
+REQUIRE_ZCD_OR_DC(ctx);
  return trans_fld(ctx, a);
  }
  
  static bool trans_c_fsd(DisasContext *ctx, arg_fsd *a)

  {
-REQUIRE_ZCD(ctx);
+REQUIRE_ZCD_OR_DC(ctx);
  return trans_fsd(ctx, a);
  }
  
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc

index 9e9fa2087a..58467eb409 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -30,10 +30,12 @@
  } \
  } while (0)
  
-#define REQUIRE_ZCF(ctx) do {  \

-if (!ctx->cfg_ptr->ext_zcf) {  \
-return false;  \
-}  \
+#define REQUIRE_ZCF_OR_FC(ctx) do {\
+if (!ctx->cfg_ptr->ext_zcf) {  \
+if(!has_ext(ctx, RVF) || !has_ext(ctx, RVC)) { \
+return false;  \
+}  \
+}  \
  } while (0)
  
  static bool trans_flw(DisasContext *ctx, arg_flw *a)

@@ -69,13 +71,13 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
  
  static bool trans_c_flw(DisasContext *ctx, arg_flw *a)

  {
-REQUIRE_ZCF(ctx);
+REQUIRE_ZCF_OR_FC(ctx);
  return trans_flw(ctx, a);
  }
  
  static bool trans_c_fsw(DisasContext *ctx, arg_fsw *a)

  {
-REQUIRE_ZCF(ctx);
+REQUIRE_ZCF_OR_FC(ctx);
  return trans_fsw(ctx, a);
  }
  
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc

index c70c495fc5..e33f63bea1 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
  tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
  
  gen_set_pc(ctx, cpu_pc);

Re: PCIe atomics in pcie-root-port

[Robin Voetter]

On 4/6/23 20:40, Alex Williamson wrote:


I think the typical approach for QEMU would be expose options in the
downstream ports that would then need to be enabled by the user or
management tool, but that's where the complication begins.  At some
point we would want management tools to "do the right thing"
themselves.  Is support for PCIe atomics pervasive enough to default to
enabling support?


Apparently this is supported from Haswell or newer on Intel, and 
Ryzen/Threadripper/Epyc on AMD hardware. I don't have any official data 
for this, though.



How do we handle hotplugged endpoints where the
interconnects do not expose atomics support, or perhaps when they
expose support that doesn't exist?  At some point in the future when we
have migration of devices with atomics, do we need to test all endpoint
to endpoint paths for equivalent atomic support on the migration target?
Are there capabilities on the endpoint that we can virtualize to
disable use of atomics if the host and guest topologies are out of sync?
It seems automatic detection is a lot of added complication for this 
feature. For the time being, I think its best to allow enabling PCIe 
atomics using a device property for the pcie-root-port that is disabled 
by default. If general hardware support is good enough, it can be 
enabled by default. Compatibility with older QEMU versions can then be 
added using a newer hw compat in hw/core/machine.c.


Kind regards,

Robin

Re: qemu v8.0-rc3 fails to compile with Xen

[David Woodhouse]

On Wed, 2023-04-12 at 18:41 +0200, Olaf Hering wrote:
> The error with this patch applied looks like this:

Thanks. So I think I fixed that one but there are others. I'll make
myself a build environment rather than getting you to help me play
whack-a-mole.

Thanks.


smime.p7s
Description: S/MIME cryptographic signature

Re: qemu v8.0-rc3 fails to compile with Xen

[Olaf Hering]

Wed, 12 Apr 2023 15:05:06 +0100 David Woodhouse :

> On Wed, 2023-04-12 at 14:20 +0200, Olaf Hering wrote:
> > Wed, 12 Apr 2023 12:46:23 +0100 Alex Bennée :
> > 
> > > Olaf Hering  writes:
> > > > Qemu v7.2.1 can be compiled with Xen 4.6, but v8.0.0-rc3 needs now at 
> > > > least Xen 4.7.  
> > > Was this caused by the addition of the KVM Xen target support or some 
> > > other churn since?
> > 
> > I did not bisect this failure, just checking if bisect is worth the effort.
> 
> It'll be something like this. I haven't tested this yet because I can't
> even get Xen that old to build locally.

The error with this patch applied looks like this:

FAILED: libcommon.fa.p/hw_xen_xen-operations.c.o 
/usr/bin/gcc-7 -m64 -mcx16 -Ilibcommon.fa.p -I/usr/include/pixman-1 
-I/usr/include/libpng16 -I/usr/include/spice-server -I/usr/include/cacard 
-I/usr/include/nss3 -I/usr/include/nspr4 -I/usr/include/glib-2.0 
-I/usr/lib64/glib-2.0/include -I/usr/include/spice-1 -I/usr/include/p11-kit-1 
-I/usr/include/libusb-1.0 -I/usr/include/libmount -I/usr/include/blkid 
-I/usr/include/gio-unix-2.0 -I/usr/include/ncursesw -fdiagnostics-color=auto 
-Wall -Winvalid-pch -std=gnu11 -O2 -isystem /Qc6f3cbca32/linux-headers -isystem 
linux-headers -iquote . -iquote /Qc6f3cbca32 -iquote /Qc6f3cbca32/include 
-iquote /Qc6f3cbca32/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common 
-fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes 
-Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits 
-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body 
-Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 
-Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value 
-Wno-psabi -fstack-protector-strong -fmessage-length=0 -grecord-gcc-switches 
-O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables 
-fasynchronous-unwind-tables -fstack-clash-protection -fPIE -DOPENSSL_LOAD_CONF 
-MD -MQ libcommon.fa.p/hw_xen_xen-operations.c.o -MF 
libcommon.fa.p/hw_xen_xen-operations.c.o.d -o 
libcommon.fa.p/hw_xen_xen-operations.c.o -c ../hw/xen/xen-operations.c
../hw/xen/xen-operations.c:37:19: error: conflicting types for 
'xenevtchn_handle'
 typedef xc_evtchn xenevtchn_handle;
   ^~~~
In file included from ../hw/xen/xen-operations.c:17:0:
/Qc6f3cbca32/include/hw/xen/xen_backend_ops.h:33:33: note: previous declaration 
of 'xenevtchn_handle' was here
 typedef struct xenevtchn_handle xenevtchn_handle;
 ^~~~
../hw/xen/xen-operations.c:49:19: error: conflicting types for 
'xengnttab_handle'
 typedef xc_gnttab xengnttab_handle;
   ^~~~
In file included from ../hw/xen/xen-operations.c:17:0:
/Qc6f3cbca32/include/hw/xen/xen_backend_ops.h:136:33: note: previous 
declaration of 'xengnttab_handle' was here
 typedef struct xengntdev_handle xengnttab_handle;
 ^~~~
../hw/xen/xen-operations.c:193:13: warning: initialization from incompatible 
pointer type [-Wincompatible-pointer-types]
 .open = libxenevtchn_backend_open,
 ^
../hw/xen/xen-operations.c:193:13: note: (near initialization for 
'libxenevtchn_backend_ops.open')
../hw/xen/xen-operations.c:194:14: error: 'xenevtchn_close' undeclared here 
(not in a function); did you mean 'xc_evtchn_close'?
 .close = xenevtchn_close,
  ^~~
  xc_evtchn_close
../hw/xen/xen-operations.c:195:25: error: 'xenevtchn_bind_interdomain' 
undeclared here (not in a function); did you mean 'xc_evtchn_bind_interdomain'?
 .bind_interdomain = xenevtchn_bind_interdomain,
 ^~
 xc_evtchn_bind_interdomain
../hw/xen/xen-operations.c:196:15: error: 'xenevtchn_unbind' undeclared here 
(not in a function); did you mean 'xc_evtchn_unbind'?
 .unbind = xenevtchn_unbind,
   ^~~~
   xc_evtchn_unbind
../hw/xen/xen-operations.c:197:15: error: 'xenevtchn_fd' undeclared here (not 
in a function); did you mean 'xc_evtchn_fd'?
 .get_fd = xenevtchn_fd,
   ^~~~
   xc_evtchn_fd
../hw/xen/xen-operations.c:198:15: error: 'xenevtchn_notify' undeclared here 
(not in a function); did you mean 'xc_evtchn_notify'?
 .notify = xenevtchn_notify,
   ^~~~
   xc_evtchn_notify
../hw/xen/xen-operations.c:199:15: error: 'xenevtchn_unmask' undeclared here 
(not in a function); did you mean 'xc_evtchn_unmask'?
 .unmask = xenevtchn_unmask,
   ^~~~
   xc_evtchn_unmask
../hw/xen/xen-operations.c:200:16: error: 'xenevtchn_pending' undeclared here 
(not in a function); did you mean 'xc_evtchn_pending'?
 .pending = xenevtchn_pending,

[PATCH for-8.1] hw/display: Compile vga.c as target-independent code

[Thomas Huth]

The target checks here are only during the initialization, so they
are not performance critical. We can switch these to runtime checks
to avoid that we have to compile this file multiple times during
the build, and make the code ready for an universal build one day.

Signed-off-by: Thomas Huth 
---
 hw/display/vga.c   | 31 ++-
 hw/display/meson.build |  2 +-
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 7a5fdff649..37557c3442 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -26,7 +26,9 @@
 #include "qemu/units.h"
 #include "sysemu/reset.h"
 #include "qapi/error.h"
+#include "hw/core/cpu.h"
 #include "hw/display/vga.h"
+#include "hw/i386/x86.h"
 #include "hw/pci/pci.h"
 #include "vga_int.h"
 #include "vga_regs.h"
@@ -2244,11 +2246,8 @@ bool vga_common_init(VGACommonState *s, Object *obj, 
Error **errp)
  * into a device attribute set by the machine/platform to remove
  * all target endian dependencies from this file.
  */
-#if TARGET_BIG_ENDIAN
-s->default_endian_fb = true;
-#else
-s->default_endian_fb = false;
-#endif
+s->default_endian_fb = target_words_bigendian();
+
 vga_dirty_log_start(s);
 
 return true;
@@ -2263,11 +2262,15 @@ static const MemoryRegionPortio vga_portio_list[] = {
 PORTIO_END_OF_LIST(),
 };
 
-static const MemoryRegionPortio vbe_portio_list[] = {
+static const MemoryRegionPortio vbe_portio_list_x86[] = {
 { 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index 
},
-# ifdef TARGET_I386
 { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
-# endif
+{ 2, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionPortio vbe_portio_list_no_x86[] = {
+{ 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index 
},
 { 2, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
 PORTIO_END_OF_LIST(),
 };
@@ -2278,9 +2281,19 @@ MemoryRegion *vga_init_io(VGACommonState *s, Object *obj,
   const MemoryRegionPortio **vbe_ports)
 {
 MemoryRegion *vga_mem;
+MachineState *ms = MACHINE(qdev_get_machine());
+
+/*
+ * We unfortunately need two VBE lists since non-x86 machines might
+ * not be able to do 16-bit accesses at unaligned addresses (0x1cf)
+ */
+if (object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
+*vbe_ports = vbe_portio_list_x86;
+} else {
+*vbe_ports = vbe_portio_list_no_x86;
+}
 
 *vga_ports = vga_portio_list;
-*vbe_ports = vbe_portio_list;
 
 vga_mem = g_malloc(sizeof(*vga_mem));
 memory_region_init_io(vga_mem, obj, _mem_ops, s,
diff --git a/hw/display/meson.build b/hw/display/meson.build
index 4191694380..17165bd536 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -36,7 +36,7 @@ softmmu_ss.add(when: 'CONFIG_CG3', if_true: files('cg3.c'))
 softmmu_ss.add(when: 'CONFIG_MACFB', if_true: files('macfb.c'))
 softmmu_ss.add(when: 'CONFIG_NEXTCUBE', if_true: files('next-fb.c'))
 
-specific_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
+softmmu_ss.add(when: 'CONFIG_VGA', if_true: files('vga.c'))
 
 if (config_all_devices.has_key('CONFIG_VGA_CIRRUS') or
 config_all_devices.has_key('CONFIG_VGA_PCI') or
-- 
2.31.1

[PULL for-8.0 1/1] block/nfs: do not poll within a coroutine

[Paolo Bonzini]

Since the former nfs_get_allocated_file_size is now a coroutine
function, it must suspend rather than poll.  Switch BDRV_POLL_WHILE()
to a qemu_coroutine_yield() loop and schedule nfs_co_generic_bh_cb()
in place of the call to bdrv_wakeup().

Fixes: 82618d7bc341 ("block: Convert bdrv_get_allocated_file_size() to 
co_wrapper", 2023-02-01)
Signed-off-by: Paolo Bonzini 
Reviewed-by: Eric Blake 
Reviewed-by: Kevin Wolf 
Message-Id: <20230412112606.80983-1-pbonz...@redhat.com>
Signed-off-by: Paolo Bonzini 
---
 block/nfs.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 351dc6ec8d14..006045d71a6f 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -726,10 +726,8 @@ nfs_get_allocated_file_size_cb(int ret, struct nfs_context 
*nfs, void *data,
 if (task->ret < 0) {
 error_report("NFS Error: %s", nfs_get_error(nfs));
 }
-
-/* Set task->complete before reading bs->wakeup.  */
-qatomic_mb_set(>complete, 1);
-bdrv_wakeup(task->bs);
+replay_bh_schedule_oneshot_event(task->client->aio_context,
+ nfs_co_generic_bh_cb, task);
 }
 
 static int64_t coroutine_fn nfs_co_get_allocated_file_size(BlockDriverState 
*bs)
@@ -743,15 +741,19 @@ static int64_t coroutine_fn 
nfs_co_get_allocated_file_size(BlockDriverState *bs)
 return client->st_blocks * 512;
 }
 
-task.bs = bs;
+nfs_co_init_task(bs, );
 task.st = 
-if (nfs_fstat_async(client->context, client->fh, 
nfs_get_allocated_file_size_cb,
-) != 0) {
-return -ENOMEM;
-}
+WITH_QEMU_LOCK_GUARD(>mutex) {
+if (nfs_fstat_async(client->context, client->fh, 
nfs_get_allocated_file_size_cb,
+) != 0) {
+return -ENOMEM;
+}
 
-nfs_set_events(client);
-BDRV_POLL_WHILE(bs, !task.complete);
+nfs_set_events(client);
+}
+while (!task.complete) {
+qemu_coroutine_yield();
+}
 
 return (task.ret < 0 ? task.ret : st.st_blocks * 512);
 }
-- 
2.39.2

[PULL for-8.0 0/1] NFS changes for 2023-04-12

[Paolo Bonzini]

The following changes since commit abb02ce0e76a8e00026699a863ab2d11d88f56d4:

  Merge tag 'for-upstream' of https://repo.or.cz/qemu/kevin into staging 
(2023-04-11 16:19:06 +0100)

are available in the Git repository at:

  https://gitlab.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 3fe64abcde55cf6f4ea5883106301baad219a7cc:

  block/nfs: do not poll within a coroutine (2023-04-12 18:26:51 +0200)


Fix NFS driver issue.


Paolo Bonzini (1):
  block/nfs: do not poll within a coroutine

 block/nfs.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)
-- 
2.39.2

Re: [PATCH 8.0 regression] block/nfs: do not poll within a coroutine

[Kevin Wolf]

Am 12.04.2023 um 13:26 hat Paolo Bonzini geschrieben:
> Since the former nfs_get_allocated_file_size is now a coroutine
> function, it must suspend rather than poll.  Switch BDRV_POLL_WHILE()
> to a qemu_coroutine_yield() loop and schedule nfs_co_generic_bh_cb()
> in place of the call to bdrv_wakeup().
> 
> Fixes: 82618d7bc341 ("block: Convert bdrv_get_allocated_file_size() to 
> co_wrapper", 2023-02-01)
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nfs.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index 351dc6ec8d14..417defc0cfef 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -726,10 +726,8 @@ nfs_get_allocated_file_size_cb(int ret, struct 
> nfs_context *nfs, void *data,
>  if (task->ret < 0) {
>  error_report("NFS Error: %s", nfs_get_error(nfs));
>  }
> -
> -/* Set task->complete before reading bs->wakeup.  */
> -qatomic_mb_set(>complete, 1);
> -bdrv_wakeup(task->bs);
> +replay_bh_schedule_oneshot_event(task->client->aio_context,
> + nfs_co_generic_bh_cb, task);
>  }
>  
>  static int64_t coroutine_fn nfs_co_get_allocated_file_size(BlockDriverState 
> *bs)
> @@ -743,15 +741,19 @@ static int64_t coroutine_fn 
> nfs_co_get_allocated_file_size(BlockDriverState *bs)
>  return client->st_blocks * 512;
>  }
>  
> -task.bs = bs;
> +nfs_co_init_task(bs, );
>  task.st = 
> -if (nfs_fstat_async(client->context, client->fh, 
> nfs_get_allocated_file_size_cb,
> -) != 0) {
> -return -ENOMEM;
> -}
> +WITH_QEMU_LOCK_GUARD(>mutex) {
> +if (nfs_fstat_async(client->context, client->fh, 
> nfs_get_allocated_file_size_cb,
> +) != 0) {
> +return -ENOMEM;
> +}
>  
> -nfs_set_events(client);
> -BDRV_POLL_WHILE(bs, !task.complete);
> + nfs_set_events(client);

Tab damage in this line.

> +}
> +while (!task.complete) {
> +qemu_coroutine_yield();
> +}
>  
>  return (task.ret < 0 ? task.ret : st.st_blocks * 512);
>  }

With the indentation above fixed:

Reviewed-by: Kevin Wolf 

Re: [QEMU][PATCH] gitlab-ci.d/crossbuilds: Drop the '--disable-tcg' configuration for xen

[Vikram Garhwal]

Hi all,

Yes, gdbstub build issue is only for for aarch64-softmmu target and that 
was the reason for this patch. x86 targets build fine with 
''disable-tcg" option.


Thanks Fabiano & Philippe for sharing the existing patch series for this.

Regards,

Vikram



On 4/12/23 5:51 AM, Fabiano Rosas wrote:

Alex Bennée  writes:


Vikram Garhwal  writes:


Xen is supported for aarch64 via xenpvh machine. disable-tcg option fails the
build for aarch64 target.

Link for xen on arm patch series: 
https://mail.gnu.org/archive/html/qemu-devel/2023-02/msg03979.html

Signed-off-by: Vikram Garhwal 
---
  .gitlab-ci.d/crossbuilds.yml | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.d/crossbuilds.yml b/.gitlab-ci.d/crossbuilds.yml
index 61b8ac86ee..6867839248 100644
--- a/.gitlab-ci.d/crossbuilds.yml
+++ b/.gitlab-ci.d/crossbuilds.yml
@@ -186,7 +186,7 @@ cross-amd64-xen-only:
variables:
  IMAGE: debian-amd64-cross
  ACCEL: xen
-EXTRA_CONFIGURE_OPTS: --disable-tcg --disable-kvm
+EXTRA_CONFIGURE_OPTS: --disable-kvm

x86 should handle --disable-tcg fine.

  
  cross-arm64-xen-only:

extends: .cross_accel_build_job
@@ -195,4 +195,4 @@ cross-arm64-xen-only:
variables:
  IMAGE: debian-arm64-cross
  ACCEL: xen
-EXTRA_CONFIGURE_OPTS: --disable-tcg --disable-kvm
+EXTRA_CONFIGURE_OPTS: --disable-kvm

Currently this builds qemu-system-i386, but with your changes and the
work Fabiano is doing:

   Message-Id: <20230313151058.19645-1-faro...@suse.de>
   Date: Mon, 13 Mar 2023 12:10:48 -0300
   Subject: [PATCH v9 00/10] target/arm: Allow CONFIG_TCG=n builds
   From: Fabiano Rosas 

We should be able to have a qemu-system-aarch64 supporting Xen without TCG

The build should already be working on current master after Philippe
fixed the gdbstub issues. My remaining patches fix tests and general
runtime issues. I just sent v10 to the list.

Re: [PATCH 8.0 regression] block/nfs: do not poll within a coroutine

[Eric Blake]

On Wed, Apr 12, 2023 at 01:26:06PM +0200, Paolo Bonzini wrote:
> Since the former nfs_get_allocated_file_size is now a coroutine
> function, it must suspend rather than poll.  Switch BDRV_POLL_WHILE()
> to a qemu_coroutine_yield() loop and schedule nfs_co_generic_bh_cb()
> in place of the call to bdrv_wakeup().
> 
> Fixes: 82618d7bc341 ("block: Convert bdrv_get_allocated_file_size() to 
> co_wrapper", 2023-02-01)
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nfs.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

Reviewed-by: Eric Blake 

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

Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation

[Jonathan Cameron via]

On Tue, 11 Apr 2023 16:52:58 +0100
Peter Maydell  wrote:

> On Mon, 7 Nov 2022 at 22:49, Michael S. Tsirkin  wrote:
> >
> > From: Huai-Cheng Kuo 
> >
> > The Data Object Exchange implementation of CXL Coherent Device Attribute
> > Table (CDAT). This implementation is referring to "Coherent Device
> > Attribute Table Specification, Rev. 1.03, July. 2022" and "Compute
> > Express Link Specification, Rev. 3.0, July. 2022"
> >
> > This patch adds core support that will be shared by both
> > end-points and switch port emulation.  
> 
> > +static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> > +{
> > +g_autofree CDATEntry *cdat_st = NULL;
> > +uint8_t sum = 0;
> > +int num_ent;
> > +int i = 0, ent = 1, file_size = 0;
> > +CDATSubHeader *hdr;
> > +FILE *fp = NULL;
> > +
> > +/* Read CDAT file and create its cache */
> > +fp = fopen(cdat->filename, "r");
> > +if (!fp) {
> > +error_setg(errp, "CDAT: Unable to open file");
> > +return;
> > +}
> > +
> > +fseek(fp, 0, SEEK_END);
> > +file_size = ftell(fp);  
> 
> Coverity points out that ftell() can fail and return -1...
> 
> > +fseek(fp, 0, SEEK_SET);
> > +cdat->buf = g_malloc0(file_size);  
> 
> ...which would cause an attempt to allocate a very large
> amount of memory, since you aren't checking for errors.
> CID 1508185.
> 
> Below, some other issues I saw in a quick scan through the code.
> 
> > +
> > +if (fread(cdat->buf, file_size, 1, fp) == 0) {
> > +error_setg(errp, "CDAT: File read failed");
> > +return;
> > +}  
> 
> (The issues in this bit of code I've mentioned in a
> different thread.)

I'll carry a patch locally using g_file_get_contents()
that gets rid of this mess. My assumption being that similar
will emerge from other thread and I can drop my patch.

> 
> > +
> > +fclose(fp);
> > +
> > +if (file_size < sizeof(CDATTableHeader)) {
> > +error_setg(errp, "CDAT: File too short");
> > +return;
> > +}  
> 
> > +i = sizeof(CDATTableHeader);
> > +num_ent = 1;
> > +while (i < file_size) {
> > +hdr = (CDATSubHeader *)(cdat->buf + i);  
> 
> If the file is not a complete number of records in
> size, then this can index off the end of the buffer.
> You should check for that.

> 
> > +cdat_len_check(hdr, errp);
> > +i += hdr->length;
> > +num_ent++;
> > +}
> > +if (i != file_size) {
> > +error_setg(errp, "CDAT: File length missmatch");  
> 
> Typo: "mismatch"
> 

That's fixed in the tree.

> > +return;
> > +}
> > +
> > +cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);  
> 
> To allocate an array of N lots of a structure, use
> g_new0(), which will take care to avoid possible
> overflow in the multiply.
> 
> > +if (!cdat_st) {
> > +error_setg(errp, "CDAT: Failed to allocate entry array");  
> 
> g_malloc0() and g_new0() can never fail, so you don't need
> to check for a NULL pointer return.

dropped.  I'll remember this one day ;)

> 
> > +return;
> > +}
> > +
> > +/* Set CDAT header, Entry = 0 */
> > +cdat_st[0].base = cdat->buf;
> > +cdat_st[0].length = sizeof(CDATTableHeader);
> > +i = 0;
> > +
> > +while (i < cdat_st[0].length) {
> > +sum += cdat->buf[i++];
> > +}
> > +
> > +/* Read CDAT structures */
> > +while (i < file_size) {
> > +hdr = (CDATSubHeader *)(cdat->buf + i);
> > +cdat_len_check(hdr, errp);  
> 
> We already did this check the first time through the data...
dropped
> 
> > +
> > +cdat_st[ent].base = hdr;
> > +cdat_st[ent].length = hdr->length;
> > +
> > +while (cdat->buf + i <
> > +   (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
> > +assert(i < file_size);
> > +sum += cdat->buf[i++];
> > +}
> > +
> > +ent++;
> > +}
> > +
> > +if (sum != 0) {
> > +warn_report("CDAT: Found checksum mismatch in %s", cdat->filename);
> > +}
> > +cdat->entry_len = num_ent;
> > +cdat->entry = g_steal_pointer(_st);
> > +}
> > +
> > +void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
> > +{
> > +CDATObject *cdat = _cstate->cdat;
> > +
> > +if (cdat->filename) {
> > +ct3_load_cdat(cdat, errp);
> > +} else {
> > +ct3_build_cdat(cdat, errp);
> > +}
> > +}  
> 
> None of the callsites to this function check for it
> failing. In particular they do not assume "if I call
> this and it fails then I need to call cxl_doe_cdata_release()
> to have it clean up". It would probably be less confusing
> if the init function cleans up after itself, i.e. does not
> leave allocated memory pointed to by cdat->buf and so on.
Agreed.  Will make it cleanup and add the error checks at the two
callers.

Thanks,

Jonathan

> 
> thanks
> -- PMM

Re: [PATCH 3/3] block-backend: delay application of request queuing

[Hanna Czenczek]

On 12.04.23 14:03, Paolo Bonzini wrote:

On Wed, Apr 12, 2023 at 1:54 PM Hanna Czenczek  wrote:

On 05.04.23 18:31, Paolo Bonzini wrote:

+if (busy || blk->in_flight) {
+return true;
+}
+
+if (qatomic_read(>request_queuing) == BLK_QUEUE_READY) {
+qatomic_set(>request_queuing, BLK_QUEUE_QUIESCENT);
+}
+return false;
   }

This implicitly relies on nobody increasing blk->in_flight (or
dev_ops->drained_poll() returning `true` again) while the BB is starting
to be drained, because if the function were to be called again after it
has returned `false` once per drained section (not sure if that’s
possible![1]), then we’d end up in the original situation, with
in_flight elevated and queuing enabled.

Yes, it does.


Is that really strictly guaranteed somehow or is it rather a complex
conglomerate of many cases that in the end happen to work out
individually?  I mean, I could imagine that running
BlockDevOps.drained_begin() is supposed to guarantee that, but it can’t,
because only NBD seems to implement it.  The commit message talks about
IDE being fine (by accident?) because it needs BQL availability to
submit new requests.  But that’s very complex and I’d rather have a
strict requirement to guarantee correctness.

It's a conglomerate of three cases each of which is sufficient (BQL,
aio_disable_external, bdrv_drained_begin---plus just not using
blk_inc_in_flight could be a fourth, of course). Of these,
aio_disable_external() is going away in favor of the
.bdrv_drained_begin callback; and blk_inc_in_flight() is used rarely
in the first place so I thought it'd be not too hard to have this
requirement.


Does IDE’s BQL requirement work for nested drains, though, i.e. when you 
have a drained_begin, followed by another?  The commit message doesn’t 
say whether it’s impossible for IDE to create a new request in between 
the two.


I’m a bit afraid that these cases are too complicated for me to fully 
comprehend.



[1] If the blk_root_drained_poll() isn’t called anymore after returning
`false`, all will be good, but I assume it will be, because we have a
quiesce_counter, not a quiesce_bool.  We could kind of emulate this by
continuing to return `false` after blk_root_drained_poll() has returned
`false` once, until the quiesce_counter becomes 0.
We could also have blk_root_drained_poll(), if it sees in_flight > 0 &&
request_queuing == BLK_QUEUE_QUIESCENT, revert request_queuing to
BLK_QUEUE_READY and resume all queued requests.

The intended long term fix is to remove request queuing and, if a
request is submitted while BLK_QUEUE_QUIESCENT, give an assertion
failure.


Yep, that would be a nice obvious requirement.


But since the hang requires blk_inc_in_flight() in the device, perhaps
in the short term documenting it in blk_inc_in_flight() may be enough?


Technically it needs a blk_inc_in_flight() whose blk_dec_in_flight() 
depends on a different request that can be queued (which is only the 
case in IDE), so I suppose we could document exactly that in those 
functions’ interfaces, i.e. that users must take care not to use 
blk_inc_flight() while the BlockBackend is (being) drained, when the 
associated blk_dec_in_flight() may depend on an I/O request to the BB.


I think that should be enough, yes.  Well, as long as you can guarantee 
that IDE will indeed fulfill that requirement, because I find it 
difficult to see/prove...


Hanna

Re: source fails to compile on msys2

[Howard Spoelstra]

Yep, fixed

No idea how I got that wrong ;-)

Best,
Howard

Re: [PATCH v3 18/20] bsd-user: Automatically generate syscall_nr.h

[Warner Losh]

On Wed, Apr 12, 2023 at 4:10 AM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 4/11/23 19:09, Warner Losh wrote:
> > +++ b/bsd-user/syscallhdr.sh
> > @@ -0,0 +1,7 @@
> > +#!/bin/sh
> > +
> > +in="$1"
> > +out="$2"
> > +bsd="$3"
> > +
> > +awk -v bsd="$3" '{sub("SYS_", "TARGET_" bsd "_NR_", $0); print;}' < $in
> > $out
>
> If the host/guest syscall numbers always match, there's no point in using
> TARGET_freebsd_NR_foo at all -- just use the original SYS_foo symbol from
> .
>

long term, this is likely correct. Short term, though, changing to SYS_foo
would cause quite a bit
of churn that I'm looking to avoid. bsd-user has two branches, and the
newest branch has problems
with threads we've not been able to completely track down, so we can't
switch to using it just yet.
So we have to still add new system calls to the old code base, which is
made harder as the number
of differences proliferate.

This is the first step, though, towards that goal: not updating the system
call tables as much, and
generating more code where possible to reduce the load we have on
hand-coded stuff.

Warner

[PATCH 0/2] tests/migration: Fix migration-test slowdown

[Juan Quintela]

Since commit:

commit 1bfc8dde505f1e6a92697c52aa9b09e81b54c78f
Author: Dr. David Alan Gilbert 
Date:   Mon Mar 6 15:26:12 2023 +

tests/migration: Tweek auto converge limits check

Thomas found an autoconverge test failure where the
migration completed before the autoconverge had kicked in.
[...]

migration-test has become very slow.
On my laptop, before that commit migration-test takes 2min10seconds
After that commit, it takes around 11minutes

We can't revert it because it fixes a real problem when the host
machine is overloaded.  See the comment on test_migrate_auto_converge().

So I did two things here:

- Once that we have setup up precopy, we can move to full speed, no
  need to continue at 3MB/s.  That slowed everything a lot.

- Only run auto_converge tests when we are asking for slow tests.

  Only that test on my hardware requires more than 1min to run.  We
  need to run it at 3MB/s, but we are asking it to do 15 iterations
  throgh 150MB of RAM.  We can have a test that is (reasonably) fast,
  or one that also works when machine is very loaded.

To test that things still works over load, I used my desktop (ancient
core i9900), and run migration-test in a loop in 20 terminals (load
was 40) and didn't see a single failure in more than 1 hour run.

Please, review.

PD.  Yes, I am still looking at the dreaded multifd_cancel test, but
even on this setup, I am unable to get a failure.  I have never seen a
failure on it when I am running it, but I am only running x86 kvm
linux.  Moving to arm or tcg and see how well that goes.

Juan Quintela (2):
  tests/migration: Make precopy fast
  tests/migration: Only run auto_converge in slow mode

 tests/qtest/migration-test.c | 26 ++
 1 file changed, 22 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH 1/2] tests/migration: Make precopy fast

[Juan Quintela]

Otherwise we do the 1st migration iteration at a too slow speed.

Signed-off-by: Juan Quintela 
---
 tests/qtest/migration-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 3b615b0da9..7b05b0b7dd 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1348,6 +1348,7 @@ static void test_precopy_common(MigrateCommon *args)
 migrate_qmp(from, args->connect_uri, "{}");
 }
 
+migrate_ensure_converge(from);
 
 if (args->result != MIG_TEST_SUCCEED) {
 bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1365,8 +1366,6 @@ static void test_precopy_common(MigrateCommon *args)
 wait_for_migration_pass(from);
 }
 
-migrate_ensure_converge(from);
-
 /* We do this first, as it has a timeout to stop us
  * hanging forever if migration didn't converge */
 wait_for_migration_complete(from);
-- 
2.39.2

[PATCH 2/2] tests/migration: Only run auto_converge in slow mode

[Juan Quintela]

Signed-off-by: Juan Quintela 
---
 tests/qtest/migration-test.c | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7b05b0b7dd..6317131b50 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1795,6 +1795,21 @@ static void test_validate_uuid_dst_not_set(void)
 do_test_validate_uuid(, false);
 }
 
+/*
+ * The way auto_converge works, we need to do too many passes to
+ * run this test.  Auto_converge logic is only run once every
+ * three iterations, so:
+ *
+ * - 3 iterations without auto_converge enabled
+ * - 3 iterations with pct = 5
+ * - 3 iterations with pct = 30
+ * - 3 iterations with pct = 55
+ * - 3 iterations with pct = 80
+ * - 3 iterations with pct = 95 (max(95, 80 + 25))
+ *
+ * To make things even worse, we need to run the initial stage at
+ * 3MB/s so we enter autoconverge even when host is (over)loaded.
+ */
 static void test_migrate_auto_converge(void)
 {
 g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -2574,8 +2589,12 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
 qtest_add_func("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
-
-qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
+/*
+ * See explanation why this test is slow on function definition
+ */
+if (g_test_slow()) {
+qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
+}
 qtest_add_func("/migration/multifd/tcp/plain/none",
test_multifd_tcp_none);
 /*
-- 
2.39.2

Re: [PULL for-8.0 0/2] last minute hw/nvme coverity fixes

[Peter Maydell]

On Wed, 12 Apr 2023 at 11:04, Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Hi Peter,
>
> The following changes since commit 6c50845a9183610cfd4cfffd48dfc704cd340882:
>
>   hw/i2c/allwinner-i2c: Fix subclassing of TYPE_AW_I2C_SUN6I (2023-04-11 
> 14:13:29 +0100)
>
> are available in the Git repository at:
>
>   git://git.infradead.org/qemu-nvme.git tags/coverity-fixes-pull-request
>
> for you to fetch changes up to 4b32319cdacd99be983e1a74128289ef52c5964e:
>
>   hw/nvme: fix memory leak in nvme_dsm (2023-04-12 12:03:09 +0200)
>
> 
> hw/nvme coverity fixes
>
> Fix two issues reported by coverity (CID 1451080 and 1451082).
>
> 
>
> Klaus Jensen (2):
>   hw/nvme: fix memory leak in fdp ruhid parsing
>   hw/nvme: fix memory leak in nvme_dsm


Applied, thanks.

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

-- PMM

Re: qemu v8.0-rc3 fails to compile with Xen

[David Woodhouse]

On Wed, 2023-04-12 at 14:20 +0200, Olaf Hering wrote:
> Wed, 12 Apr 2023 12:46:23 +0100 Alex Bennée :
> 
> > Olaf Hering  writes:
> > > Qemu v7.2.1 can be compiled with Xen 4.6, but v8.0.0-rc3 needs now at 
> > > least Xen 4.7.  
> > Was this caused by the addition of the KVM Xen target support or some other 
> > churn since?
> 
> I did not bisect this failure, just checking if bisect is worth the effort.

It'll be something like this. I haven't tested this yet because I can't
even get Xen that old to build locally.

It's reasonable to have a discussion about whether we should
intentionally drop support for older versions of Xen, but less
reasonable for me to break it by accident... :)



From 84125c787737eecc4d90e3e4ace574453eb1085d Mon Sep 17 00:00:00 2001
From: David Woodhouse 
Date: Wed, 12 Apr 2023 14:57:33 +0100
Subject: [PATCH] hw/xen: Define xenforeignmemory_map() for Xen < 4.7.1

In restructuring to allow for internal emulation of Xen functionality,
some of the compatibility for older Xen libraries was lost. The actual
backend ops were using the older xc_map_foreign_bulk() function directly,
but the xenforeignmemory_map2() compatibiity wrapper still needed a
definition of xenforeignmemory_map(). Put it back. And in that case the
ops might as well use it instead of doing their own.

Fixes: 15e283c5b684 ("hw/xen: Add foreignmem operations to allow redirection to 
internal emulation")
Signed-off-by: David Woodhouse 
---
 hw/xen/xen-operations.c | 22 --
 include/hw/xen/xen_native.h | 13 +
 2 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/hw/xen/xen-operations.c b/hw/xen/xen-operations.c
index 3d213d28df..a8e7bda7b0 100644
--- a/hw/xen/xen-operations.c
+++ b/hw/xen/xen-operations.c
@@ -223,26 +223,6 @@ static struct gnttab_backend_ops libxengnttab_backend_ops 
= {
 .unmap = libxengnttab_backend_unmap,
 };
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40701
-
-static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot,
-  size_t pages, xfn_pfn_t *pfns,
-  int *errs)
-{
-if (errs) {
-return xc_map_foreign_bulk(xen_xc, dom, prot, pfns, errs, pages);
-} else {
-return xc_map_foreign_pages(xen_xc, dom, prot, pfns, pages);
-}
-}
-
-static int libxenforeignmem_backend_unmap(void *addr, size_t pages)
-{
-return munmap(addr, pages * XC_PAGE_SIZE);
-}
-
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
-
 static void *libxenforeignmem_backend_map(uint32_t dom, void *addr, int prot,
   size_t pages, xen_pfn_t *pfns,
   int *errs)
@@ -256,8 +236,6 @@ static int libxenforeignmem_backend_unmap(void *addr, 
size_t pages)
 return xenforeignmemory_unmap(xen_fmem, addr, pages);
 }
 
-#endif
-
 struct foreignmem_backend_ops libxenforeignmem_backend_ops = {
 .map = libxenforeignmem_backend_map,
 .unmap = libxenforeignmem_backend_unmap,
diff --git a/include/hw/xen/xen_native.h b/include/hw/xen/xen_native.h
index 6bcc83baf9..db550cf703 100644
--- a/include/hw/xen/xen_native.h
+++ b/include/hw/xen/xen_native.h
@@ -35,6 +35,19 @@ typedef xc_interface xenforeignmemory_handle;
 #define xenforeignmemory_open(l, f) xen_xc
 #define xenforeignmemory_close(h)
 
+static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
+ int prot, size_t pages,
+ const xen_pfn_t arr[/*pages*/],
+ int err[/*pages*/])
+{
+if (err)
+return xc_map_foreign_bulk(h, dom, prot, arr, err, pages);
+else
+return xc_map_foreign_pages(h, dom, prot, arr, pages);
+}
+
+#define xenforeignmemory_unmap(h, p, s) munmap(p, s * XC_PAGE_SIZE)
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40701 */
 
 #include 
-- 
2.34.1




smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH v11] audio/pwaudio.c: Add Pipewire audio backend for QEMU

[Dorinda Bassey]

Hi Volker,

It seems that for some unknown reason using audio_pcm_info_clear_buf in
playback_process causes segmentation fault. Hence I moved the handling of
buffer underruns from the playback process to the qpw_write process because
that is the underlying cause of buffer underrun.

Regards,
Dorinda.

On Wed, Apr 12, 2023 at 3:57 PM Dorinda Bassey  wrote:

> This commit adds a new audiodev backend to allow QEMU to use Pipewire as
> both an audio sink and source. This backend is available on most systems
>
> Add Pipewire entry points for QEMU Pipewire audio backend
> Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
> qpw_write function returns the current state of the stream to pwaudio
> and Writes some data to the server for playback streams using pipewire
> spa_ringbuffer implementation.
> qpw_read function returns the current state of the stream to pwaudio and
> reads some data from the server for capture streams using pipewire
> spa_ringbuffer implementation. These functions qpw_write and qpw_read
> are called during playback and capture.
> Added some functions that convert pw audio formats to QEMU audio format
> and vice versa which would be needed in the pipewire audio sink and
> source functions qpw_init_in() & qpw_init_out().
> These methods that implement playback and recording will create streams
> for playback and capture that will start processing and will result in
> the on_process callbacks to be called.
> Built a connection to the Pipewire sound system server in the
> qpw_audio_init() method.
>
> Signed-off-by: Dorinda Bassey 
> ---
> v11:
> handle buffer underruns in qpw_write
> use local variable
> change param name frame_size
> fix format specifier
> change trace value to trace quantum
>
>  audio/audio.c |   3 +
>  audio/audio_template.h|   4 +
>  audio/meson.build |   1 +
>  audio/pwaudio.c   | 913 ++
>  audio/trace-events|   8 +
>  meson.build   |   8 +
>  meson_options.txt |   4 +-
>  qapi/audio.json   |  44 ++
>  qemu-options.hx   |  21 +
>  scripts/meson-buildoptions.sh |   8 +-
>  10 files changed, 1011 insertions(+), 3 deletions(-)
>  create mode 100644 audio/pwaudio.c
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 70b096713c..90c7c49d11 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2061,6 +2061,9 @@ void audio_create_pdos(Audiodev *dev)
>  #ifdef CONFIG_AUDIO_PA
>  CASE(PA, pa, Pa);
>  #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +CASE(PIPEWIRE, pipewire, Pipewire);
> +#endif
>  #ifdef CONFIG_AUDIO_SDL
>  CASE(SDL, sdl, Sdl);
>  #endif
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index e42326c20d..dc0c74aa74 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -362,6 +362,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> TYPE)(Audiodev *dev)
>  case AUDIODEV_DRIVER_PA:
>  return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
>  #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +case AUDIODEV_DRIVER_PIPEWIRE:
> +return
> qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
> +#endif
>  #ifdef CONFIG_AUDIO_SDL
>  case AUDIODEV_DRIVER_SDL:
>  return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> diff --git a/audio/meson.build b/audio/meson.build
> index 074ba9..65a49c1a10 100644
> --- a/audio/meson.build
> +++ b/audio/meson.build
> @@ -19,6 +19,7 @@ foreach m : [
>['sdl', sdl, files('sdlaudio.c')],
>['jack', jack, files('jackaudio.c')],
>['sndio', sndio, files('sndioaudio.c')],
> +  ['pipewire', pipewire, files('pwaudio.c')],
>['spice', spice, files('spiceaudio.c')]
>  ]
>if m[1].found()
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> new file mode 100644
> index 00..adf1a538c0
> --- /dev/null
> +++ b/audio/pwaudio.c
> @@ -0,0 +1,913 @@
> +/*
> + * QEMU Pipewire audio driver
> + *
> + * Copyright (c) 2023 Red Hat Inc.
> + *
> + * Author: Dorinda Bassey   
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "audio.h"
> +#include 
> +#include "qemu/error-report.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include "trace.h"
> +
> +#define AUDIO_CAP "pipewire"
> +#define RINGBUFFER_SIZE(1u << 22)
> +#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
> +
> +#include "audio_int.h"
> +
> +typedef struct pwvolume {
> +uint32_t channels;
> +float values[SPA_AUDIO_MAX_CHANNELS];
> +} pwvolume;
> +
> +typedef struct pwaudio {
> +Audiodev *dev;
> +struct pw_thread_loop *thread_loop;
> +struct pw_context *context;
> +
> +struct pw_core *core;
> +struct spa_hook core_listener;
> +int last_seq, pending_seq, error;
> +} pwaudio;
> +
> +typedef struct PWVoice {
> +pwaudio *g;
> +struct pw_stream *stream;
> 

Re: [PATCH 8.0 regression] block/nfs: do not poll within a coroutine

[Peter Maydell]

If you would like this in 8.0 then it needs to be reviewed and in a pullreq
today...

thanks
-- PMM

On Wed, 12 Apr 2023 at 12:27, Paolo Bonzini  wrote:
>
> Since the former nfs_get_allocated_file_size is now a coroutine
> function, it must suspend rather than poll.  Switch BDRV_POLL_WHILE()
> to a qemu_coroutine_yield() loop and schedule nfs_co_generic_bh_cb()
> in place of the call to bdrv_wakeup().
>
> Fixes: 82618d7bc341 ("block: Convert bdrv_get_allocated_file_size() to 
> co_wrapper", 2023-02-01)
> Signed-off-by: Paolo Bonzini 
> ---
>  block/nfs.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/block/nfs.c b/block/nfs.c
> index 351dc6ec8d14..417defc0cfef 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -726,10 +726,8 @@ nfs_get_allocated_file_size_cb(int ret, struct 
> nfs_context *nfs, void *data,
>  if (task->ret < 0) {
>  error_report("NFS Error: %s", nfs_get_error(nfs));
>  }
> -
> -/* Set task->complete before reading bs->wakeup.  */
> -qatomic_mb_set(>complete, 1);
> -bdrv_wakeup(task->bs);
> +replay_bh_schedule_oneshot_event(task->client->aio_context,
> + nfs_co_generic_bh_cb, task);
>  }
>
>  static int64_t coroutine_fn nfs_co_get_allocated_file_size(BlockDriverState 
> *bs)
> @@ -743,15 +741,19 @@ static int64_t coroutine_fn 
> nfs_co_get_allocated_file_size(BlockDriverState *bs)
>  return client->st_blocks * 512;
>  }
>
> -task.bs = bs;
> +nfs_co_init_task(bs, );
>  task.st = 
> -if (nfs_fstat_async(client->context, client->fh, 
> nfs_get_allocated_file_size_cb,
> -) != 0) {
> -return -ENOMEM;
> -}
> +WITH_QEMU_LOCK_GUARD(>mutex) {
> +if (nfs_fstat_async(client->context, client->fh, 
> nfs_get_allocated_file_size_cb,
> +) != 0) {
> +return -ENOMEM;
> +}
>
> -nfs_set_events(client);
> -BDRV_POLL_WHILE(bs, !task.complete);
> +   nfs_set_events(client);
> +}
> +while (!task.complete) {
> +qemu_coroutine_yield();
> +}
>
>  return (task.ret < 0 ? task.ret : st.st_blocks * 512);
>  }

[PATCH v11] audio/pwaudio.c: Add Pipewire audio backend for QEMU

[Dorinda Bassey]

This commit adds a new audiodev backend to allow QEMU to use Pipewire as
both an audio sink and source. This backend is available on most systems

Add Pipewire entry points for QEMU Pipewire audio backend
Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
qpw_write function returns the current state of the stream to pwaudio
and Writes some data to the server for playback streams using pipewire
spa_ringbuffer implementation.
qpw_read function returns the current state of the stream to pwaudio and
reads some data from the server for capture streams using pipewire
spa_ringbuffer implementation. These functions qpw_write and qpw_read
are called during playback and capture.
Added some functions that convert pw audio formats to QEMU audio format
and vice versa which would be needed in the pipewire audio sink and
source functions qpw_init_in() & qpw_init_out().
These methods that implement playback and recording will create streams
for playback and capture that will start processing and will result in
the on_process callbacks to be called.
Built a connection to the Pipewire sound system server in the
qpw_audio_init() method.

Signed-off-by: Dorinda Bassey 
---
v11:
handle buffer underruns in qpw_write
use local variable
change param name frame_size
fix format specifier
change trace value to trace quantum

 audio/audio.c |   3 +
 audio/audio_template.h|   4 +
 audio/meson.build |   1 +
 audio/pwaudio.c   | 913 ++
 audio/trace-events|   8 +
 meson.build   |   8 +
 meson_options.txt |   4 +-
 qapi/audio.json   |  44 ++
 qemu-options.hx   |  21 +
 scripts/meson-buildoptions.sh |   8 +-
 10 files changed, 1011 insertions(+), 3 deletions(-)
 create mode 100644 audio/pwaudio.c

diff --git a/audio/audio.c b/audio/audio.c
index 70b096713c..90c7c49d11 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2061,6 +2061,9 @@ void audio_create_pdos(Audiodev *dev)
 #ifdef CONFIG_AUDIO_PA
 CASE(PA, pa, Pa);
 #endif
+#ifdef CONFIG_AUDIO_PIPEWIRE
+CASE(PIPEWIRE, pipewire, Pipewire);
+#endif
 #ifdef CONFIG_AUDIO_SDL
 CASE(SDL, sdl, Sdl);
 #endif
diff --git a/audio/audio_template.h b/audio/audio_template.h
index e42326c20d..dc0c74aa74 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -362,6 +362,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, 
TYPE)(Audiodev *dev)
 case AUDIODEV_DRIVER_PA:
 return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
 #endif
+#ifdef CONFIG_AUDIO_PIPEWIRE
+case AUDIODEV_DRIVER_PIPEWIRE:
+return 
qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
+#endif
 #ifdef CONFIG_AUDIO_SDL
 case AUDIODEV_DRIVER_SDL:
 return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
diff --git a/audio/meson.build b/audio/meson.build
index 074ba9..65a49c1a10 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -19,6 +19,7 @@ foreach m : [
   ['sdl', sdl, files('sdlaudio.c')],
   ['jack', jack, files('jackaudio.c')],
   ['sndio', sndio, files('sndioaudio.c')],
+  ['pipewire', pipewire, files('pwaudio.c')],
   ['spice', spice, files('spiceaudio.c')]
 ]
   if m[1].found()
diff --git a/audio/pwaudio.c b/audio/pwaudio.c
new file mode 100644
index 00..adf1a538c0
--- /dev/null
+++ b/audio/pwaudio.c
@@ -0,0 +1,913 @@
+/*
+ * QEMU Pipewire audio driver
+ *
+ * Copyright (c) 2023 Red Hat Inc.
+ *
+ * Author: Dorinda Bassey   
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "audio.h"
+#include 
+#include "qemu/error-report.h"
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include "trace.h"
+
+#define AUDIO_CAP "pipewire"
+#define RINGBUFFER_SIZE(1u << 22)
+#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
+
+#include "audio_int.h"
+
+typedef struct pwvolume {
+uint32_t channels;
+float values[SPA_AUDIO_MAX_CHANNELS];
+} pwvolume;
+
+typedef struct pwaudio {
+Audiodev *dev;
+struct pw_thread_loop *thread_loop;
+struct pw_context *context;
+
+struct pw_core *core;
+struct spa_hook core_listener;
+int last_seq, pending_seq, error;
+} pwaudio;
+
+typedef struct PWVoice {
+pwaudio *g;
+struct pw_stream *stream;
+struct spa_hook stream_listener;
+struct spa_audio_info_raw info;
+uint32_t highwater_mark;
+uint32_t sample_size, req;
+struct spa_ringbuffer ring;
+uint8_t buffer[RINGBUFFER_SIZE];
+
+pwvolume volume;
+bool muted;
+} PWVoice;
+
+typedef struct PWVoiceOut {
+HWVoiceOut hw;
+PWVoice v;
+} PWVoiceOut;
+
+typedef struct PWVoiceIn {
+HWVoiceIn hw;
+PWVoice v;
+} PWVoiceIn;
+
+static void
+stream_destroy(void *data)
+{
+PWVoice *v = (PWVoice *) data;
+spa_hook_remove(>stream_listener);
+v->stream = NULL;
+}
+
+/* output data processing function to read stuffs from the 

Re: AVX-512 instruction set

[Paolo Bonzini]

On Wed, Apr 12, 2023 at 3:41 PM Walid Ghandour
 wrote:
> I will try to work on this.

Even if the individual tasks I listed are not enough to implement
AVX512, I suggest that you post individual pieces of work so that you
can somewhat pipeline the work.

Also, please review the design of the AVX decoder and the
implementation of the translation and the tests. You can find the
patches at https://patchew.org/QEMU/20221013214651.672114-1-pbonz...@redhat.com/
(patches 3-4-6-8 especially).

Paolo

> Regards,
> Walid
>
> Le mer. 12 avr. 2023 à 15:30, Paolo Bonzini  a écrit :
>>
>> On Wed, Apr 12, 2023 at 2:17 PM Alex Bennée  wrote:
>> > I don't think there is currently any active effort to add AVX512
>> > support. There have been various GSoC projects to improve the x86 SIMD
>> > emulation but I don't think they got merged.
>>
>> No, there isn't. However, the recent implementation of AVX in QEMU 7.2
>> is designed to make AVX512 at least doable.
>>
>> Adding support for AVX512 would be a very large work (at least 1
>> months full time plus time to get it merged), but not impossible. The
>> tasks could be something like this:
>>
>> 1. EVEX prefix decoding for AVX and AVX2 instructions
>> 2. operand broadcast
>> 3. VEX encoded mask instruction: kmov, kadd, kxnor, etc
>> 4. other instructions and permutations with mask operands and permutations
>> 5. opmask support including merging and zeroing
>> 6. Disp8*N addressing mode
>> 7. rounding control and exception suppression
>>
>> It's important that, at any given step, the new functionality is
>> tested comprehensively.
>>
>> > > 62 f2 7d 48 18 0d fa 0c 00 00 vbroadcastss 0xcfa(%rip),%zmm1
>> > >
>> > > qemu: uncaught target signal 4 (Illegal instruction) - core dumped
>> > >
>> > > I like to add support for broadcast and fmadd avx 512 instructions such 
>> > > as the following one:
>> > >
>> > > 62 e2 7d 48 b8 c9 vfmadd231ps %zmm1,%zmm0,%zmm17
>>
>> Both of these are using a small subset of AVX512 (step 1 above). Both
>> vbroadcastss and vfmadd231ps are already implemented in QEMU, but not
>> using ZMM registers.
>>
>> Unfortunately the base AVX512 functionality is large, and therefore
>> the above tasks are all needed to claim support for AVX512. On the
>> other hand, for historical reasons AVX512BW and AVX512VL extensions
>> are separate but in practice they are easier to just implement at the
>> same time as basic AVX512; my expectation is that they would come up
>> almost for free with the rest of the work.
>>
>> Paolo
>>

Re: [PATCH 1/2] accel/tcg/plugin: export host insn size

[Wu, Fei]

On 4/12/2023 9:28 PM, Alex Bennée wrote:
> 
> "Wu, Fei"  writes:
> 
>> On 4/11/2023 3:27 PM, Alex Bennée wrote:
>>>
>>> "Wu, Fei"  writes:
>>>
 On 4/10/2023 6:36 PM, Alex Bennée wrote:
>
> Richard Henderson  writes:
>
>> On 4/6/23 00:46, Alex Bennée wrote:
>>> If your aim is to examine JIT efficiency what is wrong with the current
>>> "info jit" that you can access via the HMP? Also I'm wondering if its
>>> time to remove the #ifdefs from CONFIG_PROFILER because I doubt the
>>> extra data it collects is that expensive.
>>> Richard, what do you think?
>>
>> What is it that you want from CONFIG_PROFILER that you can't get from 
>> perf?
>> I've been tempted to remove CONFIG_PROFILER entirely.
>
> I think perf is pretty good at getting the hot paths in the translator
> and pretty much all of the timer related stuff in CONFIG_PROFILER could
> be dropped. However some of the additional information about TCG ops
> usage and distribution is useful. That said last time I had a tilt at
> this on the back of a GSoC project:
>
>   Subject: [PATCH  v9 00/13] TCG code quality tracking and perf 
> integration
>   Date: Mon,  7 Oct 2019 16:28:26 +0100
>   Message-Id: <20191007152839.30804-1-alex.ben...@linaro.org>
>
> The series ended up moving all the useful bits of CONFIG_PROFILER into
> tb stats which was dynamically controlled on a per TB basis. Now that
> the perf integration stuff was merged maybe there is a simpler series to
> be picked out of the remains?
>
> Fei Wu,
>
> Have you looked at the above series? Is that gathering the sort of
> things you need? Is this all in service of examining the translation
> quality of hot code?
>
 Yes, it does have what I want, I suppose this wiki is for the series:
 https://wiki.qemu.org/Features/TCGCodeQuality
>>>
>>> Yes.
>>>

 btw, the archive seems broken and cannot show the whole series:
 https://www.mail-archive.com/qemu-devel@nongnu.org/msg650258.html
>>>
>>> I have a v10 branch here:
>>>
>>>   https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>>>
>>> I think the top two patches can be dropped on a re-base as the JIT/perf
>>> integration is already merged. It might be a tricky re-base though.
>>> Depends on how much churn there has been in the tree since.
>>>
>> I'd like to try it. Why has it not been merged upstream?
> 
> Bits have been merged (the perf jit support) but the original GSoC
> student moved on and I ran out of time to work on it. It became yet another
> back burner series that awaits some spare hacking time.
> 
Got it, let's see if I can help.

Thanks,
Fei.

Re: AVX-512 instruction set

[Walid Ghandour]

I will try to work on this.

Regards,
Walid

Le mer. 12 avr. 2023 à 15:30, Paolo Bonzini  a écrit :

> On Wed, Apr 12, 2023 at 2:17 PM Alex Bennée 
> wrote:
> > I don't think there is currently any active effort to add AVX512
> > support. There have been various GSoC projects to improve the x86 SIMD
> > emulation but I don't think they got merged.
>
> No, there isn't. However, the recent implementation of AVX in QEMU 7.2
> is designed to make AVX512 at least doable.
>
> Adding support for AVX512 would be a very large work (at least 1
> months full time plus time to get it merged), but not impossible. The
> tasks could be something like this:
>
> 1. EVEX prefix decoding for AVX and AVX2 instructions
> 2. operand broadcast
> 3. VEX encoded mask instruction: kmov, kadd, kxnor, etc
> 4. other instructions and permutations with mask operands and permutations
> 5. opmask support including merging and zeroing
> 6. Disp8*N addressing mode
> 7. rounding control and exception suppression
>
> It's important that, at any given step, the new functionality is
> tested comprehensively.
>
> > > 62 f2 7d 48 18 0d fa 0c 00 00 vbroadcastss 0xcfa(%rip),%zmm1
> > >
> > > qemu: uncaught target signal 4 (Illegal instruction) - core dumped
> > >
> > > I like to add support for broadcast and fmadd avx 512 instructions
> such as the following one:
> > >
> > > 62 e2 7d 48 b8 c9 vfmadd231ps %zmm1,%zmm0,%zmm17
>
> Both of these are using a small subset of AVX512 (step 1 above). Both
> vbroadcastss and vfmadd231ps are already implemented in QEMU, but not
> using ZMM registers.
>
> Unfortunately the base AVX512 functionality is large, and therefore
> the above tasks are all needed to claim support for AVX512. On the
> other hand, for historical reasons AVX512BW and AVX512VL extensions
> are separate but in practice they are easier to just implement at the
> same time as basic AVX512; my expectation is that they would come up
> almost for free with the rest of the work.
>
> Paolo
>
>

Re: source fails to compile on msys2

[Thomas Huth]

On 12/04/2023 15.13, Howard Spoelstra wrote:

Hello Peter,

My source was cloned today. I just cloned again and I still see the tokens 
reversed:
git clone https://www.gitlab.com/qemu/qemu 


You're using the wrong repository. The official QEMU repo is here:

 https://gitlab.com/qemu-project/qemu.git

 HTH,
  Thomas

Re: source fails to compile on msys2

[Peter Maydell]

On Wed, 12 Apr 2023 at 14:21, Stefan Weil via  wrote:
>
> Am 12.04.23 um 15:13 schrieb Howard Spoelstra:
> > Hello Peter,
> >
> > My source was cloned today. I just cloned again and I still see the
> > tokens reversed:
> > git clone https://www.gitlab.com/qemu/qemu
> >  qemu-master-clean
>
> The official URL is https://gitlab.com/qemu-project/qemu/.

Yep. I have no idea who that other gitlab repo is, but it's
not us, and the repo could contain anything...

https://www.qemu.org/download/ is where to start for the
instructions on how to clone the official QEMU repo.

thanks
-- PMM

Re: [QEMU][PATCH v6 10/10] meson.build: enable xenpv machine build for ARM

[Alex Bennée]

Fabiano Rosas  writes:

> Vikram Garhwal  writes:
>
>> Add CONFIG_XEN for aarch64 device to support build for ARM targets.
>>
>> Signed-off-by: Vikram Garhwal 
>> Signed-off-by: Stefano Stabellini 
>> Reviewed-by: Alex Bennée 
>> ---
>>  meson.build | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 52c3995c9d..eb5bb305ae 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -135,7 +135,7 @@ endif
>>  if cpu in ['x86', 'x86_64', 'arm', 'aarch64']
>># i386 emulator provides xenpv machine type for multiple architectures
>>accelerator_targets += {
>> -'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu'],
>> +'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu',
>> 'aarch64-softmmu'],
>
> I'm not familiar with Xen, so pardon my ignorance, but would it (ever)
> make sense to do a 1:1 map of host architecture and qemu target? So we
> don't have to deal with having a build on x86 pulling aarch64-softmmu
> and vice-versa.
>
> Do we expect both x86_64-softmmu and aarch64-softmmu binaries to be used
> in the same host?

Xen is different from the other accelerators as it isn't really guest
CPU aware. It is merely io device emulation backend albeit one that
supports a non-paravirtualised guest on x86. But you are right that
using qemu-system-i386 as a backend on aarch64 hosts does cause some
cognitive dissonance for users. For aarch64 hosts we would only support
the VirtIO guests.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 1/2] accel/tcg/plugin: export host insn size

[Alex Bennée]

"Wu, Fei"  writes:

> On 4/11/2023 3:27 PM, Alex Bennée wrote:
>> 
>> "Wu, Fei"  writes:
>> 
>>> On 4/10/2023 6:36 PM, Alex Bennée wrote:

 Richard Henderson  writes:

> On 4/6/23 00:46, Alex Bennée wrote:
>> If your aim is to examine JIT efficiency what is wrong with the current
>> "info jit" that you can access via the HMP? Also I'm wondering if its
>> time to remove the #ifdefs from CONFIG_PROFILER because I doubt the
>> extra data it collects is that expensive.
>> Richard, what do you think?
>
> What is it that you want from CONFIG_PROFILER that you can't get from 
> perf?
> I've been tempted to remove CONFIG_PROFILER entirely.

 I think perf is pretty good at getting the hot paths in the translator
 and pretty much all of the timer related stuff in CONFIG_PROFILER could
 be dropped. However some of the additional information about TCG ops
 usage and distribution is useful. That said last time I had a tilt at
 this on the back of a GSoC project:

   Subject: [PATCH  v9 00/13] TCG code quality tracking and perf integration
   Date: Mon,  7 Oct 2019 16:28:26 +0100
   Message-Id: <20191007152839.30804-1-alex.ben...@linaro.org>

 The series ended up moving all the useful bits of CONFIG_PROFILER into
 tb stats which was dynamically controlled on a per TB basis. Now that
 the perf integration stuff was merged maybe there is a simpler series to
 be picked out of the remains?

 Fei Wu,

 Have you looked at the above series? Is that gathering the sort of
 things you need? Is this all in service of examining the translation
 quality of hot code?

>>> Yes, it does have what I want, I suppose this wiki is for the series:
>>> https://wiki.qemu.org/Features/TCGCodeQuality
>> 
>> Yes.
>> 
>>>
>>> btw, the archive seems broken and cannot show the whole series:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg650258.html
>> 
>> I have a v10 branch here:
>> 
>>   https://github.com/stsquad/qemu/tree/tcg/tbstats-and-perf-v10
>> 
>> I think the top two patches can be dropped on a re-base as the JIT/perf
>> integration is already merged. It might be a tricky re-base though.
>> Depends on how much churn there has been in the tree since.
>> 
> I'd like to try it. Why has it not been merged upstream?

Bits have been merged (the perf jit support) but the original GSoC
student moved on and I ran out of time to work on it. It became yet another
back burner series that awaits some spare hacking time.

>
> Thanks,
> Fei.
>
>>>
>>> Thanks,
>>> Fei.
>>>
>
>
> r~


>> 
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: AVX-512 instruction set

[Paolo Bonzini]

On Wed, Apr 12, 2023 at 2:17 PM Alex Bennée  wrote:
> I don't think there is currently any active effort to add AVX512
> support. There have been various GSoC projects to improve the x86 SIMD
> emulation but I don't think they got merged.

No, there isn't. However, the recent implementation of AVX in QEMU 7.2
is designed to make AVX512 at least doable.

Adding support for AVX512 would be a very large work (at least 1
months full time plus time to get it merged), but not impossible. The
tasks could be something like this:

1. EVEX prefix decoding for AVX and AVX2 instructions
2. operand broadcast
3. VEX encoded mask instruction: kmov, kadd, kxnor, etc
4. other instructions and permutations with mask operands and permutations
5. opmask support including merging and zeroing
6. Disp8*N addressing mode
7. rounding control and exception suppression

It's important that, at any given step, the new functionality is
tested comprehensively.

> > 62 f2 7d 48 18 0d fa 0c 00 00 vbroadcastss 0xcfa(%rip),%zmm1
> >
> > qemu: uncaught target signal 4 (Illegal instruction) - core dumped
> >
> > I like to add support for broadcast and fmadd avx 512 instructions such as 
> > the following one:
> >
> > 62 e2 7d 48 b8 c9 vfmadd231ps %zmm1,%zmm0,%zmm17

Both of these are using a small subset of AVX512 (step 1 above). Both
vbroadcastss and vfmadd231ps are already implemented in QEMU, but not
using ZMM registers.

Unfortunately the base AVX512 functionality is large, and therefore
the above tasks are all needed to claim support for AVX512. On the
other hand, for historical reasons AVX512BW and AVX512VL extensions
are separate but in practice they are easier to just implement at the
same time as basic AVX512; my expectation is that they would come up
almost for free with the rest of the work.

Paolo

[Patch] SDHCI: fixing extend sdhci controller ops usage

[Hake Huang (OSS)]

[Patch] SDHCI: fixing extend sdhci controller ops usage

1. ops bug fixing
For IMX and S3C sdhci control the ops are given in inst_init
but it will be overwritten by class_init which calls the class_realize
fix it by checking whether ops has value of not.

2. CLKCON updates
According to usdhc RM for imx, the lower byte is reserved
when the card is present, we need set the CLKCON CLK related bit
and when write to the CLKCON register the lower byte shall be masked

Co-authored-by: Jiamin Ma 423337...@qq.com
Signed-off-by: Hake Huang hake.hu...@oss.nxp.com
---
hw/sd/sdhci.c | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6811f0f1a8..df45f08da4 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -259,6 +259,10 @@ static void sdhci_set_inserted(DeviceState *dev, bool 
level)
 if (s->norintstsen & SDHC_NISEN_INSERT) {
 s->norintsts |= SDHC_NIS_INSERT;
 }
+/*always ensure the clock is ready */
+s->clkcon |= SDHC_CLOCK_SDCLK_EN;
+s->clkcon |= SDHC_CLOCK_INT_STABLE;
+s->clkcon |= SDHC_CLOCK_INT_EN;
 } else {
 s->prnsts = 0x1fa;
 s->pwrcon &= ~SDHC_POWER_ON;
@@ -1397,16 +1401,18 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
{
 ERRP_GUARD();
-switch (s->endianness) {
-case DEVICE_LITTLE_ENDIAN:
-s->io_ops = _mmio_le_ops;
-break;
-case DEVICE_BIG_ENDIAN:
-s->io_ops = _mmio_be_ops;
-break;
-default:
-error_setg(errp, "Incorrect endianness");
-return;
+if (s->io_ops == NULL) {
+switch (s->endianness) {
+case DEVICE_LITTLE_ENDIAN:
+s->io_ops = _mmio_le_ops;
+break;
+case DEVICE_BIG_ENDIAN:
+s->io_ops = _mmio_be_ops;
+break;
+default:
+error_setg(errp, "Incorrect endianness");
+return;
+}
 }
 sdhci_init_readonly_registers(s, errp);
@@ -1669,10 +1675,12 @@ static uint64_t usdhc_read(void *opaque, hwaddr offset, 
unsigned size)
 case USDHC_TUNE_CTRL_STATUS:
 case USDHC_UNDOCUMENTED_REG27:
 case USDHC_TUNING_CTRL:
-case USDHC_MIX_CTRL:
 case USDHC_WTMK_LVL:
 ret = 0;
 break;
+case USDHC_MIX_CTRL:
+ret = s->trnmod;
+break;
 }
 return ret;
@@ -1822,6 +1830,10 @@ usdhc_write(void *opaque, hwaddr offset, uint64_t val, 
unsigned size)
  */
 sdhci_write(opaque, offset, val | s->trnmod, size);
 break;
+case SDHC_CLKCON:
+/* imx usdhc low 7 bits of CLKCON is always 1  */
+sdhci_write(opaque, offset, val | 0xf, size);
+break;
 case SDHC_BLKSIZE:
 /*
  * ESDHCI does not implement "Host SDMA Buffer Boundary", and
--
2.34.1

Regards,
Hake

Re: source fails to compile on msys2

[Stefan Weil via]

Am 12.04.23 um 15:13 schrieb Howard Spoelstra:

Hello Peter,

My source was cloned today. I just cloned again and I still see the 
tokens reversed:
git clone https://www.gitlab.com/qemu/qemu 
 qemu-master-clean


The official URL is https://gitlab.com/qemu-project/qemu/.

Regards,
Stefan

Re: [QEMU][PATCH v6 10/10] meson.build: enable xenpv machine build for ARM

[Fabiano Rosas]

Vikram Garhwal  writes:

> Add CONFIG_XEN for aarch64 device to support build for ARM targets.
>
> Signed-off-by: Vikram Garhwal 
> Signed-off-by: Stefano Stabellini 
> Reviewed-by: Alex Bennée 
> ---
>  meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 52c3995c9d..eb5bb305ae 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -135,7 +135,7 @@ endif
>  if cpu in ['x86', 'x86_64', 'arm', 'aarch64']
># i386 emulator provides xenpv machine type for multiple architectures
>accelerator_targets += {
> -'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu'],
> +'CONFIG_XEN': ['i386-softmmu', 'x86_64-softmmu',
> 'aarch64-softmmu'],

I'm not familiar with Xen, so pardon my ignorance, but would it (ever)
make sense to do a 1:1 map of host architecture and qemu target? So we
don't have to deal with having a build on x86 pulling aarch64-softmmu
and vice-versa.

Do we expect both x86_64-softmmu and aarch64-softmmu binaries to be used
in the same host?

Re: source fails to compile on msys2

[Howard Spoelstra]

Hello Peter,

My source was cloned today. I just cloned again and I still see the tokens
reversed:
git clone https://www.gitlab.com/qemu/qemu qemu-master-clean

/**
 * qemu_build_not_reached()
 *
 * The compiler, during optimization, is expected to prove that a call
 * to this function cannot be reached and remove it.  If the compiler
 * supports QEMU_ERROR, this will be reported at compile time; otherwise
 * this will be reported at link time due to the missing symbol.
 */
extern G_NORETURN
void QEMU_ERROR("code path is reachable")
qemu_build_not_reached_always(void);
#if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)
#define qemu_build_not_reached()  qemu_build_not_reached_always()
#else
#define qemu_build_not_reached()  g_assert_not_reached()
#endif


The source on gitlab shows the correct order:

/** * qemu_build_not_reached() * * The compiler, during optimization,
is expected to prove that a call * to this function cannot be reached
and remove it.  If the compiler * supports QEMU_ERROR, this will be
reported at compile time; otherwise * this will be reported at link
time due to the missing symbol. */G_NORETURN externvoid
QEMU_ERROR("code path is reachable")
qemu_build_not_reached_always(void);#if defined(__OPTIMIZE__) &&
!defined(__NO_INLINE__)#define qemu_build_not_reached()
qemu_build_not_reached_always()#else#define qemu_build_not_reached()
g_assert_not_reached()#endif

Re: [PULL v4 23/83] hw/cxl/cdat: CXL CDAT Data Object Exchange implementation

[Jonathan Cameron via]

On Tue, 11 Apr 2023 16:52:58 +0100
Peter Maydell  wrote:

> On Mon, 7 Nov 2022 at 22:49, Michael S. Tsirkin  wrote:
> >
> > From: Huai-Cheng Kuo 
> >
> > The Data Object Exchange implementation of CXL Coherent Device Attribute
> > Table (CDAT). This implementation is referring to "Coherent Device
> > Attribute Table Specification, Rev. 1.03, July. 2022" and "Compute
> > Express Link Specification, Rev. 3.0, July. 2022"
> >
> > This patch adds core support that will be shared by both
> > end-points and switch port emulation.  
> 
> > +static void ct3_load_cdat(CDATObject *cdat, Error **errp)
> > +{
> > +g_autofree CDATEntry *cdat_st = NULL;
> > +uint8_t sum = 0;
> > +int num_ent;
> > +int i = 0, ent = 1, file_size = 0;
> > +CDATSubHeader *hdr;
> > +FILE *fp = NULL;
> > +
> > +/* Read CDAT file and create its cache */
> > +fp = fopen(cdat->filename, "r");
> > +if (!fp) {
> > +error_setg(errp, "CDAT: Unable to open file");
> > +return;
> > +}
> > +
> > +fseek(fp, 0, SEEK_END);
> > +file_size = ftell(fp);  
> 
> Coverity points out that ftell() can fail and return -1...
> 
> > +fseek(fp, 0, SEEK_SET);
> > +cdat->buf = g_malloc0(file_size);  
> 
> ...which would cause an attempt to allocate a very large
> amount of memory, since you aren't checking for errors.
> CID 1508185.
> 
> Below, some other issues I saw in a quick scan through the code.
> 
> > +
> > +if (fread(cdat->buf, file_size, 1, fp) == 0) {
> > +error_setg(errp, "CDAT: File read failed");
> > +return;
> > +}  
> 
> (The issues in this bit of code I've mentioned in a
> different thread.)
> 
> > +
> > +fclose(fp);
> > +
> > +if (file_size < sizeof(CDATTableHeader)) {
> > +error_setg(errp, "CDAT: File too short");
> > +return;
> > +}  
> 
> > +i = sizeof(CDATTableHeader);
> > +num_ent = 1;
> > +while (i < file_size) {
> > +hdr = (CDATSubHeader *)(cdat->buf + i);  
> 
> If the file is not a complete number of records in
> size, then this can index off the end of the buffer.
> You should check for that.
> 
> > +cdat_len_check(hdr, errp);
> > +i += hdr->length;
> > +num_ent++;
> > +}
> > +if (i != file_size) {
> > +error_setg(errp, "CDAT: File length missmatch");  
> 
> Typo: "mismatch"
> 
> > +return;
> > +}
> > +
> > +cdat_st = g_malloc0(sizeof(*cdat_st) * num_ent);  
> 
> To allocate an array of N lots of a structure, use
> g_new0(), which will take care to avoid possible
> overflow in the multiply.
> 
> > +if (!cdat_st) {
> > +error_setg(errp, "CDAT: Failed to allocate entry array");  
> 
> g_malloc0() and g_new0() can never fail, so you don't need
> to check for a NULL pointer return.
> 
> > +return;
> > +}
> > +
> > +/* Set CDAT header, Entry = 0 */
> > +cdat_st[0].base = cdat->buf;
> > +cdat_st[0].length = sizeof(CDATTableHeader);
> > +i = 0;
> > +
> > +while (i < cdat_st[0].length) {
> > +sum += cdat->buf[i++];
> > +}
> > +
> > +/* Read CDAT structures */
> > +while (i < file_size) {
> > +hdr = (CDATSubHeader *)(cdat->buf + i);
> > +cdat_len_check(hdr, errp);  
> 
> We already did this check the first time through the data...
> 
> > +
> > +cdat_st[ent].base = hdr;
> > +cdat_st[ent].length = hdr->length;
> > +
> > +while (cdat->buf + i <
> > +   (uint8_t *)cdat_st[ent].base + cdat_st[ent].length) {
> > +assert(i < file_size);
> > +sum += cdat->buf[i++];
> > +}
> > +
> > +ent++;
> > +}
> > +
> > +if (sum != 0) {
> > +warn_report("CDAT: Found checksum mismatch in %s", cdat->filename);
> > +}
> > +cdat->entry_len = num_ent;
> > +cdat->entry = g_steal_pointer(_st);
> > +}
> > +
> > +void cxl_doe_cdat_init(CXLComponentState *cxl_cstate, Error **errp)
> > +{
> > +CDATObject *cdat = _cstate->cdat;
> > +
> > +if (cdat->filename) {
> > +ct3_load_cdat(cdat, errp);
> > +} else {
> > +ct3_build_cdat(cdat, errp);
> > +}
> > +}  
> 
> None of the callsites to this function check for it
> failing. In particular they do not assume "if I call
> this and it fails then I need to call cxl_doe_cdata_release()
> to have it clean up". It would probably be less confusing
> if the init function cleans up after itself, i.e. does not
> leave allocated memory pointed to by cdat->buf and so on.

Thanks Peter,

I'll wait for the other thread to resolve the follow up with
a patch set cleaning up remaining issues you've pointed out.

Jonathan

> 
> thanks
> -- PMM

Re: [PATCH] target/riscv: Update check for Zca/zcf/zcd

[Daniel Henrique Barboza]

On 4/12/23 00:06, Weiwei Li wrote:

Even though Zca/Zcf/Zcd can be included by C/F/D, however, their priv
version is higher than the priv version of C/F/D. So if we use check
for them instead of check for C/F/D totally, it will trigger new
problem when we try to disable the extensions based on the configured
priv version.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---



Reviewed-by: Daniel Henrique Barboza 


And I'll fold it into the next version of "[PATCH v6 0/9] target/riscv: rework
CPU extensions validation​" to fix the sifive break I'm experiencing there.



Thanks,


Daniel


  target/riscv/insn_trans/trans_rvd.c.inc | 12 +++-
  target/riscv/insn_trans/trans_rvf.c.inc | 14 --
  target/riscv/insn_trans/trans_rvi.c.inc |  5 +++--
  target/riscv/translate.c|  5 +++--
  4 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/target/riscv/insn_trans/trans_rvd.c.inc 
b/target/riscv/insn_trans/trans_rvd.c.inc
index 2c51e01c40..f8d0ae48c7 100644
--- a/target/riscv/insn_trans/trans_rvd.c.inc
+++ b/target/riscv/insn_trans/trans_rvd.c.inc
@@ -31,9 +31,11 @@
  } \
  } while (0)
  
-#define REQUIRE_ZCD(ctx) do { \

-if (!ctx->cfg_ptr->ext_zcd) {  \
-return false; \
+#define REQUIRE_ZCD_OR_DC(ctx) do { \
+if (!ctx->cfg_ptr->ext_zcd) { \
+if(!has_ext(ctx, RVD) || !has_ext(ctx, RVC)) { \
+return false; \
+} \
  } \
  } while (0)
  
@@ -67,13 +69,13 @@ static bool trans_fsd(DisasContext *ctx, arg_fsd *a)
  
  static bool trans_c_fld(DisasContext *ctx, arg_fld *a)

  {
-REQUIRE_ZCD(ctx);
+REQUIRE_ZCD_OR_DC(ctx);
  return trans_fld(ctx, a);
  }
  
  static bool trans_c_fsd(DisasContext *ctx, arg_fsd *a)

  {
-REQUIRE_ZCD(ctx);
+REQUIRE_ZCD_OR_DC(ctx);
  return trans_fsd(ctx, a);
  }
  
diff --git a/target/riscv/insn_trans/trans_rvf.c.inc b/target/riscv/insn_trans/trans_rvf.c.inc

index 9e9fa2087a..58467eb409 100644
--- a/target/riscv/insn_trans/trans_rvf.c.inc
+++ b/target/riscv/insn_trans/trans_rvf.c.inc
@@ -30,10 +30,12 @@
  } \
  } while (0)
  
-#define REQUIRE_ZCF(ctx) do {  \

-if (!ctx->cfg_ptr->ext_zcf) {  \
-return false;  \
-}  \
+#define REQUIRE_ZCF_OR_FC(ctx) do {\
+if (!ctx->cfg_ptr->ext_zcf) {  \
+if(!has_ext(ctx, RVF) || !has_ext(ctx, RVC)) { \
+return false;  \
+}  \
+}  \
  } while (0)
  
  static bool trans_flw(DisasContext *ctx, arg_flw *a)

@@ -69,13 +71,13 @@ static bool trans_fsw(DisasContext *ctx, arg_fsw *a)
  
  static bool trans_c_flw(DisasContext *ctx, arg_flw *a)

  {
-REQUIRE_ZCF(ctx);
+REQUIRE_ZCF_OR_FC(ctx);
  return trans_flw(ctx, a);
  }
  
  static bool trans_c_fsw(DisasContext *ctx, arg_fsw *a)

  {
-REQUIRE_ZCF(ctx);
+REQUIRE_ZCF_OR_FC(ctx);
  return trans_fsw(ctx, a);
  }
  
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc

index c70c495fc5..e33f63bea1 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -56,7 +56,7 @@ static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
  tcg_gen_andi_tl(cpu_pc, cpu_pc, (target_ulong)-2);
  
  gen_set_pc(ctx, cpu_pc);

-if (!ctx->cfg_ptr->ext_zca) {
+if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca) {
  TCGv t0 = tcg_temp_new();
  
  misaligned = gen_new_label();

@@ -169,7 +169,8 @@ static bool gen_branch(DisasContext *ctx, arg_b *a, TCGCond 
cond)
  
  gen_set_label(l); /* branch taken */
  
-if (!ctx->cfg_ptr->ext_zca && ((ctx->base.pc_next + a->imm) & 0x3)) {

+if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca &&
+((ctx->base.pc_next + a->imm) & 0x3)) {
  /* misaligned */
  gen_exception_inst_addr_mis(ctx);
  } else {
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index d0094922b6..661e29ab39 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -551,7 +551,7 @@ static void gen_jal(DisasContext *ctx, int rd, target_ulong 
imm)
  
  /* check misaligned: */

  next_pc = ctx->base.pc_next + imm;
-if (!ctx->cfg_ptr->ext_zca) {
+if (!has_ext(ctx, RVC) && !ctx->cfg_ptr->ext_zca) {
  if ((next_pc & 0x3) != 0) {
  gen_exception_inst_addr_mis(ctx);
  return;
@@ -1137,7 +1137,8 @@ static void decode_opc(CPURISCVState *env, DisasContext 
*ctx, uint16_t opcode)
   * The Zca extension is added as way to refer to instructions in the C
   * extension that do not include the floating-point loads and stores
   */
-if (ctx->cfg_ptr->ext_zca && decode_insn16(ctx, opcode)) {
+if ((has_ext(ctx, RVC) || 

Re: [PATCH] target/riscv: Mask the implicitly enabled extensions in isa_string based on priv version

[Daniel Henrique Barboza]

On 4/7/23 00:30, Weiwei Li wrote:

Using implicitly enabled extensions such as Zca/Zcf/Zcd instead of their
super extensions can simplify the extension related check. However, they
may have higher priv version than their super extensions. So we should mask
them in the isa_string based on priv version to make them invisible to user
if the specified priv version is lower than their minimal priv version.

Signed-off-by: Weiwei Li 
Signed-off-by: Junqiang Wang 
---


Reviewed-by: Daniel Henrique Barboza 


And I'll fold it into the next version of "[PATCH v6 0/9] target/riscv: rework
CPU extensions validation​" to fix the sifive break I'm experiencing there.



Thanks,


Daniel


  target/riscv/cpu.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index cb68916fce..1a5099382c 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1709,6 +1709,7 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char 
**isa_str,
  
  for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {

  if (isa_edata_arr[i].multi_letter &&
+(cpu->env.priv_ver >= isa_edata_arr[i].min_version) &&
  isa_ext_is_enabled(cpu, _edata_arr[i])) {
  new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
  g_free(old);