Re: [PATCH] hw/virtio: Add a protection against duplicate vu_scmi_stop calls

[Milan Zamazal]

Fabiano Rosas  writes:

> Milan Zamazal  writes:
>
>> The QEMU CI fails in virtio-scmi test occasionally.  As reported by
>> Thomas Huth, this happens most likely when the system is loaded and it
>> fails with the following error:
>>
>>   qemu-system-aarch64: ../../devel/qemu/hw/pci/msix.c:659:
>>   msix_unset_vector_notifiers: Assertion
>> `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier'
>> failed.
>>   ../../devel/qemu/tests/qtest/libqtest.c:200: kill_qemu() detected
>> QEMU death from signal 6 (Aborted) (core dumped)
>>
>> As discovered by Fabiano Rosas, the cause is a duplicate invocation of
>> msix_unset_vector_notifiers via duplicate vu_scmi_stop calls:
>>
>>   msix_unset_vector_notifiers
>>   virtio_pci_set_guest_notifiers
>>   vu_scmi_stop
>>   vu_scmi_disconnect
>>   ...
>>   qemu_chr_write_buffer
>>
>>   msix_unset_vector_notifiers
>>   virtio_pci_set_guest_notifiers
>>   vu_scmi_stop
>>   vu_scmi_set_status
>>   ...
>>   qemu_cleanup
>>
>> While vu_scmi_stop calls are protected by vhost_dev_is_started()
>> check, it's apparently not enough.  vhost-user-blk and vhost-user-gpio
>> use an extra protection, see f5b22d06fb (vhost: recheck dev state in
>> the vhost_migration_log routine) for the motivation.  Let's use the
>> same in vhost-user-scmi, which fixes the failure above.
>>
>> Fixes: a5dab090e142 ("hw/virtio: Add boilerplate for vhost-user-scmi device")
>> Signed-off-by: Milan Zamazal 
>
> Reviewed-by: Fabiano Rosas 

Please note that this bug fix should IMO definitely go to 8.1, to not
have a bug in vhost-user-scmi and to not have broken tests.  Any chance
to get it merged?

Thanks,
Milan

Re: sparc64 -- cannot get 'Hello World' to run

[Michael Tokarev]

03.08.2023 00:02, Philippe Mathieu-Daudé wrote:

Hi Frederick,

On 2/8/23 22:36, Frederick Virchanza Gotham wrote:

On Wed, Aug 2, 2023 at 11:04 AM Frederick Virchanza Gotham wrote:


I can't get sparc64 to work at all though. Even I make a simple 'Hello
World' program in C using only "puts", if I try to use qemu-user to
run it, it crashes.



You can try the following at the command line in Ubuntu 23.04:


Ubuntu 23.04, - I guess it is qemu 8.0, no?  8.0 has a few issues with
linux-user, some of them were there in 7.2 too.  And some of that has
additional patches (coming from upstream most of the time).

Please note also that there are quite a few fixes has been made to both
qemu 8.0 and 7.2 stable series, including linux-user area.

/mjt

Re: [PATCH] hw/virtio: Add a protection against duplicate vu_scmi_stop calls

[Thomas Huth]

On 03/08/2023 09.10, Milan Zamazal wrote:

Fabiano Rosas  writes:


Milan Zamazal  writes:


The QEMU CI fails in virtio-scmi test occasionally.  As reported by
Thomas Huth, this happens most likely when the system is loaded and it
fails with the following error:

   qemu-system-aarch64: ../../devel/qemu/hw/pci/msix.c:659:
   msix_unset_vector_notifiers: Assertion
`dev->msix_vector_use_notifier && dev->msix_vector_release_notifier'
failed.
   ../../devel/qemu/tests/qtest/libqtest.c:200: kill_qemu() detected
QEMU death from signal 6 (Aborted) (core dumped)

As discovered by Fabiano Rosas, the cause is a duplicate invocation of
msix_unset_vector_notifiers via duplicate vu_scmi_stop calls:

   msix_unset_vector_notifiers
   virtio_pci_set_guest_notifiers
   vu_scmi_stop
   vu_scmi_disconnect
   ...
   qemu_chr_write_buffer

   msix_unset_vector_notifiers
   virtio_pci_set_guest_notifiers
   vu_scmi_stop
   vu_scmi_set_status
   ...
   qemu_cleanup

While vu_scmi_stop calls are protected by vhost_dev_is_started()
check, it's apparently not enough.  vhost-user-blk and vhost-user-gpio
use an extra protection, see f5b22d06fb (vhost: recheck dev state in
the vhost_migration_log routine) for the motivation.  Let's use the
same in vhost-user-scmi, which fixes the failure above.

Fixes: a5dab090e142 ("hw/virtio: Add boilerplate for vhost-user-scmi device")
Signed-off-by: Milan Zamazal 


Reviewed-by: Fabiano Rosas 


Please note that this bug fix should IMO definitely go to 8.1, to not
have a bug in vhost-user-scmi and to not have broken tests.  Any chance
to get it merged?


If nobody else is planning a pull request with this patch included, I can 
take it for my next PR (since it is fixing the CI, I just saw another 
failure here:

https://gitlab.com/qemu-project/qemu/-/jobs/4790457938#L4784 )

 Thomas

[PATCH v2 1/2] block/blkio: close the fd when blkio_connect() fails

[Stefano Garzarella]

libblkio drivers take ownership of `fd` only after a successful
blkio_connect(), so if it fails, we are still the owners.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for 
virtio-blk")
Suggested-by: Hanna Czenczek 
Signed-off-by: Stefano Garzarella 
---

Notes:
v2:
- avoid to use `fd_supported` to track a valid fd [Hanna]

 block/blkio.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 8e7ce42c79..baba2f0b67 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -678,7 +678,7 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 const char *path = qdict_get_try_str(options, "path");
 BDRVBlkioState *s = bs->opaque;
 bool fd_supported = false;
-int fd, ret;
+int fd = -1, ret;
 
 if (!path) {
 error_setg(errp, "missing 'path' option");
@@ -719,6 +719,7 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 if (ret < 0) {
 fd_supported = false;
 qemu_close(fd);
+fd = -1;
 }
 }
 }
@@ -733,14 +734,18 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 }
 
 ret = blkio_connect(s->blkio);
+if (ret < 0 && fd >= 0) {
+/* Failed to give the FD to libblkio, close it */
+qemu_close(fd);
+fd = -1;
+}
+
 /*
  * If the libblkio driver doesn't support the `fd` property, 
blkio_connect()
  * will fail with -EINVAL. So let's try calling blkio_connect() again by
  * directly setting `path`.
  */
 if (fd_supported && ret == -EINVAL) {
-qemu_close(fd);
-
 /*
  * We need to clear the `fd` property we set previously by setting
  * it to -1.
-- 
2.41.0

[PATCH v2 2/2] block/blkio: add more comments on the fd passing handling

[Stefano Garzarella]

As Hanna pointed out, it is not clear in the code why qemu_open()
can fail, and why blkio_set_int("fd") is not enough to discover
the `fd` property support.

Let's fix them by adding more details in the code comments.

Suggested-by: Hanna Czenczek 
Reviewed-by: Hanna Czenczek 
Signed-off-by: Stefano Garzarella 
---
 block/blkio.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index baba2f0b67..1dd495617c 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -713,6 +713,12 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
  */
 fd = qemu_open(path, O_RDWR, NULL);
 if (fd < 0) {
+/*
+ * qemu_open() can fail if the user specifies a path that is not
+ * a file or device, for example in the case of Unix Domain Socket
+ * for the virtio-blk-vhost-user driver. In such cases let's have
+ * libblkio open the path directly.
+ */
 fd_supported = false;
 } else {
 ret = blkio_set_int(s->blkio, "fd", fd);
@@ -741,9 +747,12 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 }
 
 /*
- * If the libblkio driver doesn't support the `fd` property, 
blkio_connect()
- * will fail with -EINVAL. So let's try calling blkio_connect() again by
- * directly setting `path`.
+ * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
+ * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
+ * whether the driver supports the `fd` property or not. In that case,
+ * blkio_connect() will fail with -EINVAL.
+ * So let's try calling blkio_connect() again by directly setting `path`
+ * to cover this scenario.
  */
 if (fd_supported && ret == -EINVAL) {
 /*
-- 
2.41.0

[PATCH v2 0/2] block/blkio: fix fd leak and add more comments for the fd passing

[Stefano Garzarella]

Hanna discovered an fd leak in the error path, and a few comments to
improve in the code.

v2:
  - avoid to use `fd_supported` to track a valid fd [Hanna]

v1: 
https://lore.kernel.org/qemu-devel/20230801160332.122564-1-sgarz...@redhat.com/

Stefano Garzarella (2):
  block/blkio: close the fd when blkio_connect() fails
  block/blkio: add more comments on the fd passing handling

 block/blkio.c | 26 --
 1 file changed, 20 insertions(+), 6 deletions(-)

-- 
2.41.0

QEMU developers fortnightly conference for 2023-08-08

[juan . quintela]

Hi Do you have any topics for next week QEMU call? You can answer this  
email or use the wiki: https://wiki.qemu.org/QEMUCall Thanks, Juan.


QEMU developers fortnightly conference call
Tuesday 2023-08-08 ⋅ 15:00 – 16:00
Central European Time - Madrid

Location
https://meet.jit.si/kvmcallmeeting  
https://www.google.com/url?q=https%3A%2F%2Fmeet.jit.si%2Fkvmcallmeeting&sa=D&ust=169148358000&usg=AOvVaw1tC60zxlR0x9axjGwlU98Y



If you need call details, please contact me: quint...@redhat.com

Guests
Philippe Mathieu-Daudé
Joao Martins
quint...@redhat.com
Meirav Dean
Felipe Franciosi
afaer...@suse.de
bazu...@redhat.com
bbau...@redhat.com
c...@f00f.org
dustin.kirkl...@canonical.com
ebl...@redhat.com
edgar.igles...@gmail.com
eric.au...@redhat.com
i...@theiggy.com
jan.kis...@web.de
jidong.x...@gmail.com
jjhe...@linux.vnet.ibm.com
m...@linux.vnet.ibm.com
Peter Maydell
richard.hender...@linaro.org
stefa...@gmail.com
Warner Losh
z@139.com
zwu.ker...@gmail.com
Jason Gunthorpe
Neo Jia
David Edmondson
Elena Ufimtseva
Konrad Wilk
a...@rev.ng
a...@rev.ng
Shameerali Kolothum Thodi
Wang, Wei W
Chao Peng
qemu-devel@nongnu.org
mbur...@qti.qualcomm.com

Re: QEMU developers fortnightly conference for 2023-08-08

[Mark Burton]

Are too many people away right now?

Cheers
Mark.


On 3 Aug 2023, at 10:33, 
juan.quint...@gmail.com wrote:


WARNING: This email originated from outside of Qualcomm. Please be wary of any 
links or attachments, and do not enable macros.



Hi

Do you have any topics for next week QEMU call?
You can answer this email or use the wiki:

https://wiki.qemu.org/QEMUCall

Thanks, Juan.

QEMU developers fortnightly conference call
Tuesday 2023-08-08 ⋅ 15:00 – 16:00 (Central European Time - Madrid)
If you need call details, please contact me: 
quint...@redhat.com
Location
https://meet.jit.si/kvmcallmeeting
View 
map
Guests
Philippe Mathieu-Daudé
Joao Martins
quint...@redhat.com
Meirav Dean
Felipe Franciosi
afaer...@suse.de
bazu...@redhat.com
bbau...@redhat.com
c...@f00f.org
dustin.kirkl...@canonical.com
ebl...@redhat.com
edgar.igles...@gmail.com
eric.au...@redhat.com
i...@theiggy.com
jan.kis...@web.de
jidong.x...@gmail.com
jjhe...@linux.vnet.ibm.com
m...@linux.vnet.ibm.com
Peter Maydell
richard.hender...@linaro.org
stefa...@gmail.com
Warner Losh
z@139.com
zwu.ker...@gmail.com
Jason Gunthorpe
Neo Jia
David Edmondson
Elena Ufimtseva
Konrad Wilk
a...@rev.ng
a...@rev.ng
Shameerali Kolothum Thodi
Wang, Wei W
Chao Peng
qemu-devel@nongnu.org
mbur...@qti.qualcomm.com

Re: [RFC PATCH 05/19] kvm: Enable KVM_SET_USER_MEMORY_REGION2 for memslot

[Xiaoyao Li]

On 8/2/2023 1:10 AM, Claudio Fontana wrote:

On 7/31/23 18:21, Xiaoyao Li wrote:

From: Chao Peng 

Switch to KVM_SET_USER_MEMORY_REGION2 when supported by KVM.

With KVM_SET_USER_MEMORY_REGION2, QEMU can set up memory region that
backen'ed both by hva-based shared memory and gmem fd based private
memory.

Signed-off-by: Chao Peng 
Codeveloped-by: Xiaoyao Li 
Signed-off-by: Xiaoyao Li 
---
  accel/kvm/kvm-all.c  | 57 +---
  accel/kvm/trace-events   |  2 +-
  include/sysemu/kvm_int.h |  2 ++
  3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d8eee405de24..7b1818334ba7 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -288,35 +288,68 @@ int kvm_physical_memory_addr_from_host(KVMState *s, void 
*ram,
  static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot, 
bool new)
  {
  KVMState *s = kvm_state;
-struct kvm_userspace_memory_region mem;
+struct kvm_userspace_memory_region2 mem;
+static int cap_user_memory2 = -1;
  int ret;
  
+if (cap_user_memory2 == -1) {

+cap_user_memory2 = kvm_check_extension(s, KVM_CAP_USER_MEMORY2);
+}
+
+if (!cap_user_memory2 && slot->fd >= 0) {
+error_report("%s, KVM doesn't support gmem!", __func__);
+exit(1);
+}


We handle this special error case here,
while the existing callers of kvm_set_user_memory_region handle the other error 
cases in different places.

Not that the rest of kvm-all does an excellent job at error handling, but maybe 
we can avoid compounding on the issue.


I'm not sure how to align them. Do you have any suggestion?


+
  mem.slot = slot->slot | (kml->as_id << 16);
  mem.guest_phys_addr = slot->start_addr;
  mem.userspace_addr = (unsigned long)slot->ram;
  mem.flags = slot->flags;
+mem.gmem_fd = slot->fd;
+mem.gmem_offset = slot->ofs;
  
-if (slot->memory_size && !new && (mem.flags ^ slot->old_flags) & KVM_MEM_READONLY) {

+if (slot->memory_size && !new && (slot->flags ^ slot->old_flags) & 
KVM_MEM_READONLY) {


Why the change if mem.flags == slot->flags ?


I guess the goal is to make it clearer that it's comparing the (new) 
flags with old_flags of the slot.


Anyway, if this change is annoying, I can drop it. :)


  /* Set the slot size to 0 before setting the slot to the desired
   * value. This is needed based on KVM commit 75d61fbc. */
  mem.memory_size = 0;
-ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
+
+if (cap_user_memory2) {
+ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION2, &mem);
+} else {
+ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
+   }
  if (ret < 0) {
  goto err;
  }
  }
  mem.memory_size = slot->memory_size;
-ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
+if (cap_user_memory2) {
+ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION2, &mem);
+} else {
+ret = kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
+}
  slot->old_flags = mem.flags;
  err:
  trace_kvm_set_user_memory(mem.slot >> 16, (uint16_t)mem.slot, mem.flags,
mem.guest_phys_addr, mem.memory_size,
-  mem.userspace_addr, ret);
+  mem.userspace_addr, mem.gmem_fd,
+ mem.gmem_offset, ret);
  if (ret < 0) {
-error_report("%s: KVM_SET_USER_MEMORY_REGION failed, slot=%d,"
- " start=0x%" PRIx64 ", size=0x%" PRIx64 ": %s",
- __func__, mem.slot, slot->start_addr,
- (uint64_t)mem.memory_size, strerror(errno));
+if (cap_user_memory2) {
+error_report("%s: KVM_SET_USER_MEMORY_REGION2 failed, slot=%d,"
+" start=0x%" PRIx64 ", size=0x%" PRIx64 ","
+" flags=0x%" PRIx32 ","
+" gmem_fd=%" PRId32 ", gmem_offset=0x%" PRIx64 ": %s",
+__func__, mem.slot, slot->start_addr,
+(uint64_t)mem.memory_size, mem.flags,
+mem.gmem_fd, (uint64_t)mem.gmem_offset,
+strerror(errno));
+} else {
+error_report("%s: KVM_SET_USER_MEMORY_REGION failed, slot=%d,"
+" start=0x%" PRIx64 ", size=0x%" PRIx64 ": %s",
+__func__, mem.slot, slot->start_addr,
+(uint64_t)mem.memory_size, strerror(errno));
+}
  }
  return ret;
  }
@@ -472,6 +505,9 @@ static int kvm_mem_flags(MemoryRegion *mr)
  if (readonly && kvm_readonly_mem_allowed) {
  flags |= KVM_MEM_READONLY;
  }
+if (memory_region_can_be_private(mr)) {
+flags |= KVM_MEM_PRIVATE;
+}
  return flags;
  }
  
@@ -1402,6 +1438,9 @@ static void kvm_set_phys_mem(KVMMemoryList

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

[Andrew Jones]

On Wed, Aug 02, 2023 at 03:00:58PM -0300, Daniel Henrique Barboza wrote:
> cpu->cfg.mvendorid is a 32 bit field and kvm_set_one_reg() always write
> a target_ulong val, i.e. a 64 bit field in a 64 bit host.
> 
> Given that we're passing a pointer to the mvendorid field, the reg is
> reading 64 bits starting from mvendorid and going 32 bits in the next
> field, marchid. Here's an example:
> 
> $ ./qemu-system-riscv64 -machine virt,accel=kvm -m 2G -smp 1 \
>-cpu rv64,marchid=0xab,mvendorid=0xcd,mimpid=0xef(...)
> 
> (inside the guest)
>  # cat /proc/cpuinfo
> processor : 0
> hart  : 0
> isa   : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> mmu   : sv57
> mvendorid : 0xab00cd
> marchid   : 0xab
> mimpid: 0xef
> 
> 'mvendorid' was written as a combination of 0xab (the value from the
> adjacent field, marchid) and its intended value 0xcd.
> 
> Fix it by assigning cpu->cfg.mvendorid to a target_ulong var 'reg' and
> use it as input for kvm_set_one_reg(). Here's the result with this patch
> applied and using the same QEMU command line:
> 
>  # cat /proc/cpuinfo
> processor : 0
> hart  : 0
> isa   : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> mmu   : sv57
> mvendorid : 0xcd
> marchid   : 0xab
> mimpid: 0xef
> 
> This bug affects only the generic (rv64) CPUs when running with KVM in a
> 64 bit env since the 'host' CPU does not allow the machine IDs to be
> changed via command line.
> 
> Fixes: 1fb5a622f7 ("target/riscv: handle mvendorid/marchid/mimpid for KVM 
> CPUs")
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/riscv/kvm.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 9d8a8982f9..b1fd2233c0 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -852,12 +852,19 @@ void kvm_arch_init_irq_routing(KVMState *s)
>  static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
>  {
>  CPURISCVState *env = &cpu->env;
> +target_ulong reg;

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

 uint32_t reg = cpu->cfg.mvendorid;

and then...

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

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

> +reg = cpu->cfg.mvendorid;
> +ret = kvm_set_one_reg(cs, id, ®);
>  if (ret != 0) {
>  return ret;
>  }
> -- 
> 2.41.0
>

We should audit and fix all uses of &cpu->cfg.* with KVM ioctls. We can
also consider introducing wrappers like

#define kvm_set_one_reg_safe(cs, id, addr)  \
({  \
typeof(*(addr)) _addr = *(addr);\
kvm_set_one_reg(cs, id, &_addr) \
})

Thanks,
drew

Re: Format type of qemu NVMe virtual drive reverted back to its default (512 bytes block size) after performing hot plugout/plugin operation on that drive.

[Klaus Jensen]

On Jul 25 16:53, Ashutosh Sharma wrote:
> Hi,
> 
> I have a virtual system created using qemu 7.2. In that system, I
> attached/hot plugged a virtual NVMe drive. This drive had a default
> block size of 512 bytes.
> 
> admin@node-3:~$ sudo nvme list
> Node  SN   Model
>  Namespace Usage  Format   FW
> Rev
> - 
>  -
> --  
> /dev/nvme0n1  ashudev-6f34a1cf_13  QEMU NVMe Ctrl
>  1  34.36  GB /  34.36  GB512   B +  0 B
> 7.1.92
> 
> After that, I formatted this drive with 4k block size and it formatted
> successfully.
> 
> admin@node-3:~$ sudo nvme format /dev/nvme0n1 -f --lbaf 4
> Success formatting namespace:1
> admin@node-3:~$
> admin@node-3:~$ sudo nvme list
> Node  SN   Model
>  Namespace Usage  Format   FW
> Rev
> - 
>  -
> --  
> /dev/nvme0n1  ashudev-6f34a1cf_13  QEMU NVMe Ctrl
>  1  34.36  GB /  34.36  GB  4 KiB +  0 B
> 7.1.92
> 
> Then, I just performed the hot plugout and then plugin operation on
> that drive using qmp.execute's device_del and device_add cmd
> respectively.
> 
> But, after that, the default block size of that drive reverted to 512 bytes.
> 
> admin@node-3:~$ sudo nvme list
> Node  SN   Model
>  Namespace Usage  Format   FW
> Rev
> - 
>  -
> --  
> /dev/nvme0n1  ashudev-6f34a1cf_13  QEMU NVMe Ctrl
>  1  34.36  GB /  34.36  GB512   B +  0 B
> 7.1.92
> 
> So, I just wanted to know why the NVMe format type reverted back to
> 512 bytes, as I just performed the hot plugout/plugin operations only.
> Drive's block size (format type) should not be changed upon
> removal/insertion, right ? or am I missing something ?
> 
> Regards,
> Ashutosh
> 

The nvme device (or namespaces) does not have any persistent state. The
only way to specify boot-to-boot configuration is through device
parameters. In other words, if you reformat to 4k, then you need to
change logical_block_size on the device/namespace upon reboot.

The format command mostly exists for testing purposes.

I guess it's a caveat we should be more clear about in the
documentation.


signature.asc
Description: PGP signature

Re: [PATCH] hw/virtio: Add a protection against duplicate vu_scmi_stop calls

[Michael S. Tsirkin]

On Thu, Aug 03, 2023 at 09:38:20AM +0200, Thomas Huth wrote:
> On 03/08/2023 09.10, Milan Zamazal wrote:
> > Fabiano Rosas  writes:
> > 
> > > Milan Zamazal  writes:
> > > 
> > > > The QEMU CI fails in virtio-scmi test occasionally.  As reported by
> > > > Thomas Huth, this happens most likely when the system is loaded and it
> > > > fails with the following error:
> > > > 
> > > >qemu-system-aarch64: ../../devel/qemu/hw/pci/msix.c:659:
> > > >msix_unset_vector_notifiers: Assertion
> > > > `dev->msix_vector_use_notifier && dev->msix_vector_release_notifier'
> > > > failed.
> > > >../../devel/qemu/tests/qtest/libqtest.c:200: kill_qemu() detected
> > > > QEMU death from signal 6 (Aborted) (core dumped)
> > > > 
> > > > As discovered by Fabiano Rosas, the cause is a duplicate invocation of
> > > > msix_unset_vector_notifiers via duplicate vu_scmi_stop calls:
> > > > 
> > > >msix_unset_vector_notifiers
> > > >virtio_pci_set_guest_notifiers
> > > >vu_scmi_stop
> > > >vu_scmi_disconnect
> > > >...
> > > >qemu_chr_write_buffer
> > > > 
> > > >msix_unset_vector_notifiers
> > > >virtio_pci_set_guest_notifiers
> > > >vu_scmi_stop
> > > >vu_scmi_set_status
> > > >...
> > > >qemu_cleanup
> > > > 
> > > > While vu_scmi_stop calls are protected by vhost_dev_is_started()
> > > > check, it's apparently not enough.  vhost-user-blk and vhost-user-gpio
> > > > use an extra protection, see f5b22d06fb (vhost: recheck dev state in
> > > > the vhost_migration_log routine) for the motivation.  Let's use the
> > > > same in vhost-user-scmi, which fixes the failure above.
> > > > 
> > > > Fixes: a5dab090e142 ("hw/virtio: Add boilerplate for vhost-user-scmi 
> > > > device")
> > > > Signed-off-by: Milan Zamazal 
> > > 
> > > Reviewed-by: Fabiano Rosas 
> > 
> > Please note that this bug fix should IMO definitely go to 8.1, to not
> > have a bug in vhost-user-scmi and to not have broken tests.  Any chance
> > to get it merged?
> 
> If nobody else is planning a pull request with this patch included, I can
> take it for my next PR (since it is fixing the CI, I just saw another
> failure here:
> https://gitlab.com/qemu-project/qemu/-/jobs/4790457938#L4784 )
> 
>  Thomas
> 

I picked it up but if you like I can drop it.

-- 
MST

Re: [PATCH 0/3] target/m68k: Fix a few semihosting bugs

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via  wrote:
>
> The first two patches mirror similar patches I recently sent for nios2.
>
>  1. Use correct parameter for EXIT (d1 instead of d0)
>  2. Fix use of deposit64 in LSEEK (argument order was incorrect)
>
> The second patch has also been submitted by Peter Maydell, it's
> included here because it was required to get things working.

The usual way we do that is that you include my patch in your
series, and add your signed-off-by line (indicating it has
come via you).

thanks
-- PMM

Re: [PATCH 1/3] target/m68k: Pass semihosting arg to exit

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via  wrote:
>
> Instead of using d0 (the semihost function number), use d1 (the
> provide exit status).
>
> Signed-off-by: Keith Packard 
> ---
>  target/m68k/m68k-semi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
> index 88ad9ba814..12235759c7 100644
> --- a/target/m68k/m68k-semi.c
> +++ b/target/m68k/m68k-semi.c
> @@ -130,8 +130,8 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
>  args = env->dregs[1];
>  switch (nr) {
>  case HOSTED_EXIT:
> -gdb_exit(env->dregs[0]);
> -exit(env->dregs[0]);
> +gdb_exit(env->dregs[1]);
> +exit(env->dregs[1]);

I looked at this code but didn't spot the bug, because I
got confused by the 'args = env->dregs[1]' line above somehow.

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH] hw/virtio: Add a protection against duplicate vu_scmi_stop calls

[Thomas Huth]

On 03/08/2023 11.50, Michael S. Tsirkin wrote:

On Thu, Aug 03, 2023 at 09:38:20AM +0200, Thomas Huth wrote:

On 03/08/2023 09.10, Milan Zamazal wrote:

Fabiano Rosas  writes:


Milan Zamazal  writes:


The QEMU CI fails in virtio-scmi test occasionally.  As reported by
Thomas Huth, this happens most likely when the system is loaded and it
fails with the following error:

qemu-system-aarch64: ../../devel/qemu/hw/pci/msix.c:659:
msix_unset_vector_notifiers: Assertion
`dev->msix_vector_use_notifier && dev->msix_vector_release_notifier'
failed.
../../devel/qemu/tests/qtest/libqtest.c:200: kill_qemu() detected
QEMU death from signal 6 (Aborted) (core dumped)

As discovered by Fabiano Rosas, the cause is a duplicate invocation of
msix_unset_vector_notifiers via duplicate vu_scmi_stop calls:

msix_unset_vector_notifiers
virtio_pci_set_guest_notifiers
vu_scmi_stop
vu_scmi_disconnect
...
qemu_chr_write_buffer

msix_unset_vector_notifiers
virtio_pci_set_guest_notifiers
vu_scmi_stop
vu_scmi_set_status
...
qemu_cleanup

While vu_scmi_stop calls are protected by vhost_dev_is_started()
check, it's apparently not enough.  vhost-user-blk and vhost-user-gpio
use an extra protection, see f5b22d06fb (vhost: recheck dev state in
the vhost_migration_log routine) for the motivation.  Let's use the
same in vhost-user-scmi, which fixes the failure above.

Fixes: a5dab090e142 ("hw/virtio: Add boilerplate for vhost-user-scmi device")
Signed-off-by: Milan Zamazal 


Reviewed-by: Fabiano Rosas 


Please note that this bug fix should IMO definitely go to 8.1, to not
have a bug in vhost-user-scmi and to not have broken tests.  Any chance
to get it merged?


If nobody else is planning a pull request with this patch included, I can
take it for my next PR (since it is fixing the CI, I just saw another
failure here:
https://gitlab.com/qemu-project/qemu/-/jobs/4790457938#L4784 )

  Thomas



I picked it up but if you like I can drop it.


I think it better fits into your tree, so if you plan a PR before the next 
RC, then please go ahead. I'll drop it from my branch again.


 Thomas

Re: [PATCH 3/3] target/m68k: Support semihosting on non-ColdFire targets

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via  wrote:
>
> According to the m68k semihosting spec:
>
> "The instruction used to trigger a semihosting request depends on the
>  m68k processor variant.  On ColdFire, "halt" is used; on other processors
>  (which don't implement "halt"), "bkpt #0" may be used."
>
> Add support for non-CodeFire processors by matching BKPT #0
> instructions. When semihosting is disabled, convert those
> back to illegal op exceptions.
>
> Signed-off-by: Keith Packard 
> ---
>  target/m68k/cpu.h   |  1 +
>  target/m68k/op_helper.c | 16 
>  target/m68k/translate.c |  4 
>  3 files changed, 21 insertions(+)
>
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index cf70282717..b741c50a8f 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -67,6 +67,7 @@
>
>  #define EXCP_RTE0x100
>  #define EXCP_HALT_INSN  0x101
> +#define EXCP_BKPT_INSN  0x102
>
>  #define M68K_DTTR0   0
>  #define M68K_DTTR1   1
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 1ce850bbc5..2d89db6dde 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -295,6 +295,22 @@ static void m68k_interrupt_all(CPUM68KState *env, int 
> is_hw)
>  /* Return from an exception.  */
>  m68k_rte(env);
>  return;
> +case EXCP_BKPT_INSN:
> +if (semihosting_enabled((env->sr & SR_S) == 0)
> +&& (env->pc & 3) == 0
> +&& cpu_lduw_code(env, env->pc - 4) == 0x4e71
> +&& cpu_ldl_code(env, env->pc) == 0x4e7bf000) {

This long condition is identical to the one used for
semihosting-via-halt. It could usefully be moved out
to a function, which can then have a comment noting
what it's doing (i.e. checking for the required insns
preceding and following the bkpt or halt).

> +env->pc += 4;
> +do_m68k_semihosting(env, env->dregs[0]);
> +return;
> +}
> +/*
> + * When semihosting is not enabled, translate this back to
> + * an illegal op exception.
> + */
> +cs->exception_index = EXCP_ILLEGAL;
> +env->pc += 2;
> +break;
>  }
>  }
>
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index e07161d76f..d037c57453 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2640,6 +2640,10 @@ DISAS_INSN(bkpt)
>  #if defined(CONFIG_USER_ONLY)
>  gen_exception(s, s->base.pc_next, EXCP_DEBUG);
>  #else
> +if ((insn & 7) == 0) {
> +gen_exception(s, s->pc, EXCP_BKPT_INSN);
> +return;
> +}
>  gen_exception(s, s->base.pc_next, EXCP_ILLEGAL);

This means that semihosting-via-bpkt will only work
in system emulation, because in user mode the
EXCP_DEBUG will take precedence. To handle that you
need to also check in linux-user/m68k/cpu_loop.c. But
this effectively also implies reverting most of
a638af09b6c6b (where we took out a lot of "m68k
usermode semihosting" because it wasn't reachable).
So it's probably not worthwhile.

-- PMM

[PATCH v2] Fix SEGFAULT on getting physical address of MMIO region.

[Mikhail Tyutin]

Apply save_iotlb_data() to io_readx() as well as to io_writex().

Signed-off-by: Dmitriy Solovev 
Signed-off-by: Mikhail Tyutin 
---
 accel/tcg/cputlb.c | 36 +---
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ba44501a7c..addce3be38 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1363,6 +1363,21 @@ static inline void cpu_transaction_failed(CPUState *cpu, 
hwaddr physaddr,
 }
 }
 
+/*
+ * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
+ * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
+ * because of the side effect of io_writex changing memory layout.
+ */
+static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
+hwaddr mr_offset)
+{
+#ifdef CONFIG_PLUGIN
+SavedIOTLB *saved = &cs->saved_iotlb;
+saved->section = section;
+saved->mr_offset = mr_offset;
+#endif
+}
+
 static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
  int mmu_idx, vaddr addr, uintptr_t retaddr,
  MMUAccessType access_type, MemOp op)
@@ -1382,6 +1397,12 @@ static uint64_t io_readx(CPUArchState *env, 
CPUTLBEntryFull *full,
 cpu_io_recompile(cpu, retaddr);
 }
 
+/*
+ * The memory_region_dispatch may trigger a flush/resize
+ * so for plugins we save the iotlb_data just in case.
+ */
+save_iotlb_data(cpu, section, mr_offset);
+
 {
 QEMU_IOTHREAD_LOCK_GUARD();
 r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
@@ -1398,21 +1419,6 @@ static uint64_t io_readx(CPUArchState *env, 
CPUTLBEntryFull *full,
 return val;
 }
 
-/*
- * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
- * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
- * because of the side effect of io_writex changing memory layout.
- */
-static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
-hwaddr mr_offset)
-{
-#ifdef CONFIG_PLUGIN
-SavedIOTLB *saved = &cs->saved_iotlb;
-saved->section = section;
-saved->mr_offset = mr_offset;
-#endif
-}
-
 static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
   int mmu_idx, uint64_t val, vaddr addr,
   uintptr_t retaddr, MemOp op)
-- 
2.34.1

RE: [RFC PATCH] arm/kvm: Enable support for KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE

[Shameerali Kolothum Thodi via]

> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: 27 July 2023 16:43
> To: Shameerali Kolothum Thodi 
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; ricar...@google.com;
> k...@vger.kernel.org; Jonathan Cameron ;
> Linuxarm 
> Subject: Re: [RFC PATCH] arm/kvm: Enable support for
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE
> 
> On Tue, 25 Jul 2023 at 16:01, Shameer Kolothum
>  wrote:
> >
> > Now that we have Eager Page Split support added for ARM in the kernel[0],
> > enable it in Qemu. This adds,
> >  -eager-split-size to Qemu options to set the eager page split chunk size.
> >  -enable KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE.
> 
> It looks from the code like you've added a new sub-option
> to -accel, not a new global option. This is the right thing,
> but your commit message should document the actual option syntax
> to avoid confusion.

Ok. Will update the commit message.

> > The chunk size specifies how many pages to break at a time, using a
> > single allocation. Bigger the chunk size, more pages need to be
> > allocated ahead of time.
> >
> > Notes:
> >  - I am not sure whether we need to call kvm_vm_check_extension() for
> >KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE or not as kernel seems to
> disable
> >eager page size by default and it will return zero always.
> >
> >   -ToDo: Update qemu-options.hx
> >
> > [0]:
> https://lore.kernel.org/all/168426111477.3193133.1074810619984378093
> 0.b4...@linux.dev/
> 
> Speaking of confusion, this message says "It's an optimization used
> in Google Cloud since 2016 on x86, and for the last couple of months
> on ARM." so I'm not sure why we've ended up with an Arm-specific
> KVM_CAP and code in target/arm/kvm.c rather than something more
> generic ?
> 
> If this is going to arrive for other architectures in the future
> we should probably think about whether some of this code should
> be generic, not arm-specific.
> 
> Also this seems to be an obscure tuning parameter -- it could
> use good documentation so users have some idea when it can help.
> 
> As a more specific case of that: the kernel patchset says it
> makes Arm do the same thing that x86 already does, and split
> the huge pages automatically based on use of the dirty log.
> If the kernel can do this automatically and we never felt
> the need to provide a manual tuning knob for x86, do we even
> need to expose the Arm manual control via QEMU?

From the history of the above series, it looks like, the main argument
for making this a user adjustable knob for ARM is because of the upfront
extra memory allocations required in kernel associated with splitting the
block page. 

https://lore.kernel.org/kvmarm/86v8ktkqfx.wl-...@kernel.org/

https://lore.kernel.org/kvmarm/86h6w70zhc.wl-...@kernel.org/

And the knob for x86 case is a kvm module_param(eager_page_split).
Not clear to me why x86 opted for a module_param per KVM but not
per VM user space one. The discussion can be found here,
https://lore.kernel.org/kvm/YaDrmNVsXSMXR72Z@xz-m1.local/#t


> Other than that, I have a few minor coding things below.
> 
> > +static bool kvm_arm_eager_split_size_valid(uint64_t req_size, uint32_t
> sizes)
> > +{
> > +int i;
> > +
> > +for (i = 0; i < sizeof(uint32_t) * BITS_PER_BYTE; i++) {
> > +if (!(sizes & (1 << i))) {
> > +continue;
> > +}
> > +
> > +if (req_size == (1 << i)) {
> > +return true;
> > +}
> > +}
> 
> We know req_size is a power of 2 here, so if you also explicitly
> rule out 0 then you can do
>  return sizes & (1 << ctz64(req_size));
> rather than having to loop through. (Need to rule out 0
> because otherwise ctz64() returns 64 and the shift is UB.)

Yes, missed that we already handled the != power of 2 case.
Will update as per your next comment on this patch. That
is much simpler. Thanks.

> 
> > +
> > +return false;
> > +}
> > +
> >  int kvm_arch_init(MachineState *ms, KVMState *s)
> >  {
> >  int ret = 0;
> > @@ -280,6 +298,21 @@ int kvm_arch_init(MachineState *ms, KVMState
> *s)
> >  }
> >  }
> >
> > +if (s->kvm_eager_split_size) {
> > +uint32_t sizes;
> > +
> > +sizes = kvm_vm_check_extension(s,
> KVM_CAP_ARM_SUPPORTED_BLOCK_SIZES);
> > +if (!sizes) {
> > +error_report("Eager Page Split not supported on host");
> > +} else if
> (!kvm_arm_eager_split_size_valid(s->kvm_eager_split_size,
> > +   sizes)) {
> > +error_report("Eager Page Split requested chunk size not
> valid");
> > +} else if (kvm_vm_enable_cap(s,
> KVM_CAP_ARM_EAGER_SPLIT_CHUNK_SIZE, 0,
> > + s->kvm_eager_split_size)) {
> > +error_report("Failed to set Eager Page Split chunk size");
> > +}
> > +}
> > +
> >  kvm_arm_init_debug(s);
> >
> >  return ret;
> > @@ -1062,6 +1095,46 @@ bool
> kvm_arch_cpu_check_are_resettable(void)
> > 

[PULL 1/9] util/oslib-win32: Fix compiling with Clang from MSYS2

[Thomas Huth]

Clang complains:

../util/oslib-win32.c:483:56: error: omitting the parameter name in a
 function definition is a C2x extension [-Werror,-Wc2x-extensions]
win32_close_exception_handler(struct _EXCEPTION_RECORD*,
   ^
Fix it by adding parameter names.

Message-Id: <20230728142748.305341-4-th...@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Thomas Huth 
---
 util/oslib-win32.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 429542face..19a0ea7fbe 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -480,8 +480,9 @@ int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
 }
 
 EXCEPTION_DISPOSITION
-win32_close_exception_handler(struct _EXCEPTION_RECORD*,
-  void*, struct _CONTEXT*, void*)
+win32_close_exception_handler(struct _EXCEPTION_RECORD *exception_record,
+  void *registration, struct _CONTEXT *context,
+  void *dispatcher)
 {
 return EXCEPTION_EXECUTE_HANDLER;
 }
-- 
2.39.3

[PULL 0/9] Fixes for 8.1-rc3

[Thomas Huth]

The following changes since commit fb695ae3fdfe34ce7bf2eaa4595d48ca809c8841:

  Merge tag 'pull-qapi-2023-08-02' of https://repo.or.cz/qemu/armbru into 
staging (2023-08-02 06:51:53 -0700)

are available in the Git repository at:

  https://gitlab.com/thuth/qemu.git tags/pull-request-2023-08-03

for you to fetch changes up to f54ba56dad0e9cea275e9802915a293f1a8c7d22:

  gitlab: disable FF_SCRIPT_SECTIONS on msys jobs (2023-08-03 13:04:48 +0200)


* Fix timeout problems in the MSYS Gitlab CI jobs
* Fix a problem when compiling with Clang on Windows


Daniel P. Berrangé (8):
  gitlab: remove duplication between msys jobs
  gitlab: print timestamps during windows msys jobs
  gitlab: always use updated msys installer
  gitlab: drop $CI_PROJECT_DIR from cache path
  gitlab: always populate cache for windows msys jobs
  configure: support passthrough of -Dxxx args to meson
  gitlab: disable optimization and debug symbols in msys build
  gitlab: disable FF_SCRIPT_SECTIONS on msys jobs

Thomas Huth (1):
  util/oslib-win32: Fix compiling with Clang from MSYS2

 configure|   4 ++
 util/oslib-win32.c   |   5 +-
 .gitlab-ci.d/windows.yml | 174 +++
 3 files changed, 94 insertions(+), 89 deletions(-)

[PULL 4/9] gitlab: always use updated msys installer

[Thomas Huth]

From: Daniel P. Berrangé 

We current reference an msys installer binary from mid-2022, which means
after installation, it immediately has to re-download a bunch of newer
content. This wastes precious CI time.

The msys project publishes an installer binary with a fixed URL that
always references the latest content. We cache the downloads in gitlab
though and so once downloaded we would never re-fetch the installer
leading back to the same problem.

To deal with this we also fetch the pgp signature for the installer
on every run, and compare that to the previously cached signature. If
the signature changes, we re-download the full installer.

This ensures we always have the latest installer for msys, while also
maximising use of the gitlab cache.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Thomas Huth 
Message-Id: <20230801130403.164060-4-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 30 +++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 831b080d12..0bc04ad068 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -23,10 +23,34 @@
   - If ( !(Test-Path -Path msys64\var\cache ) ) {
   mkdir msys64\var\cache
 }
-  - If ( !(Test-Path -Path msys64\var\cache\msys2.exe ) ) {
+  - Invoke-WebRequest
+"https://repo.msys2.org/distrib/msys2-x86_64-latest.sfx.exe.sig";
+-outfile "msys2.exe.sig"
+  - if ( Test-Path -Path msys64\var\cache\msys2.exe.sig ) {
+  Write-Output "Cached installer sig" ;
+  if ( ((Get-FileHash msys2.exe.sig).Hash -ne (Get-FileHash 
msys64\var\cache\msys2.exe.sig).Hash) ) {
+Write-Output "Mis-matched installer sig, new installer download 
required" ;
+Remove-Item -Path msys64\var\cache\msys2.exe.sig ;
+if ( Test-Path -Path msys64\var\cache\msys2.exe ) {
+  Remove-Item -Path msys64\var\cache\msys2.exe
+}
+  } else {
+Write-Output "Matched installer sig, cached installer still valid"
+  }
+} else {
+  Write-Output "No cached installer sig, new installer download required" ;
+  if ( Test-Path -Path msys64\var\cache\msys2.exe ) {
+Remove-Item -Path msys64\var\cache\msys2.exe
+  }
+}
+  - if ( !(Test-Path -Path msys64\var\cache\msys2.exe ) ) {
+  Write-Output "Fetching latest installer" ;
   Invoke-WebRequest
-  
"https://github.com/msys2/msys2-installer/releases/download/2022-06-03/msys2-base-x86_64-20220603.sfx.exe";
-  -outfile "msys64\var\cache\msys2.exe"
+  "https://repo.msys2.org/distrib/msys2-x86_64-latest.sfx.exe";
+  -outfile "msys64\var\cache\msys2.exe" ;
+  Copy-Item -Path msys2.exe.sig -Destination msys64\var\cache\msys2.exe.sig
+} else {
+  Write-Output "Using cached installer"
 }
   - Write-Output "Invoking msys2.exe installer at $(Get-Date -Format u)"
   - msys64\var\cache\msys2.exe -y
-- 
2.39.3

[PULL 2/9] gitlab: remove duplication between msys jobs

[Thomas Huth]

From: Daniel P. Berrangé 

Although they share a common parent, the two msys jobs still have
massive duplication in their script definitions that can easily be
collapsed.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Thomas Huth 
Message-Id: <20230801130403.164060-2-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 132 +++
 1 file changed, 49 insertions(+), 83 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index f889a468b5..f086540e40 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -35,97 +35,63 @@
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
   - taskkill /F /FI "MODULES eq msys-2.0.dll"
-
-msys2-64bit:
-  extends: .shared_msys2_builder
   script:
   - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
   bison diffutils flex
   git grep make sed
-  mingw-w64-x86_64-capstone
-  mingw-w64-x86_64-curl
-  mingw-w64-x86_64-cyrus-sasl
-  mingw-w64-x86_64-dtc
-  mingw-w64-x86_64-gcc
-  mingw-w64-x86_64-glib2
-  mingw-w64-x86_64-gnutls
-  mingw-w64-x86_64-gtk3
-  mingw-w64-x86_64-libgcrypt
-  mingw-w64-x86_64-libjpeg-turbo
-  mingw-w64-x86_64-libnfs
-  mingw-w64-x86_64-libpng
-  mingw-w64-x86_64-libssh
-  mingw-w64-x86_64-libtasn1
-  mingw-w64-x86_64-libusb
-  mingw-w64-x86_64-lzo2
-  mingw-w64-x86_64-nettle
-  mingw-w64-x86_64-ninja
-  mingw-w64-x86_64-pixman
-  mingw-w64-x86_64-pkgconf
-  mingw-w64-x86_64-python
-  mingw-w64-x86_64-SDL2
-  mingw-w64-x86_64-SDL2_image
-  mingw-w64-x86_64-snappy
-  mingw-w64-x86_64-spice
-  mingw-w64-x86_64-usbredir
-  mingw-w64-x86_64-zstd "
+  $MINGW_TARGET-capstone
+  $MINGW_TARGET-curl
+  $MINGW_TARGET-cyrus-sasl
+  $MINGW_TARGET-dtc
+  $MINGW_TARGET-gcc
+  $MINGW_TARGET-glib2
+  $MINGW_TARGET-gnutls
+  $MINGW_TARGET-gtk3
+  $MINGW_TARGET-libgcrypt
+  $MINGW_TARGET-libjpeg-turbo
+  $MINGW_TARGET-libnfs
+  $MINGW_TARGET-libpng
+  $MINGW_TARGET-libssh
+  $MINGW_TARGET-libtasn1
+  $MINGW_TARGET-libusb
+  $MINGW_TARGET-lzo2
+  $MINGW_TARGET-nettle
+  $MINGW_TARGET-ninja
+  $MINGW_TARGET-pixman
+  $MINGW_TARGET-pkgconf
+  $MINGW_TARGET-python
+  $MINGW_TARGET-SDL2
+  $MINGW_TARGET-SDL2_image
+  $MINGW_TARGET-snappy
+  $MINGW_TARGET-spice
+  $MINGW_TARGET-usbredir
+  $MINGW_TARGET-zstd "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
-  - $env:MSYSTEM = 'MINGW64' # Start a 64-bit MinGW environment
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
   - mkdir build
   - cd build
-  # Note: do not remove "--without-default-devices"!
-  # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using 
--without-default-devices"
-  # changed to compile QEMU with the --without-default-devices switch
-  # for the msys2 64-bit job, due to the build could not complete within
-  # the project timeout.
-  - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
-  --without-default-devices --enable-fdt=system'
-  - ..\msys64\usr\bin\bash -lc 'make'
-  # qTests don't run successfully with "--without-default-devices",
-  # so let's exclude the qtests from CI for now.
-  - ..\msys64\usr\bin\bash -lc 'make check MTESTARGS=\"--no-suite qtest\" || { 
cat meson-logs/testlog.txt; exit 1; } ;'
+  - ..\msys64\usr\bin\bash -lc "../configure --enable-fdt=system 
$CONFIGURE_ARGS"
+  - ..\msys64\usr\bin\bash -lc "make"
+  - ..\msys64\usr\bin\bash -lc "make check MTESTARGS='$TEST_ARGS' || { cat 
meson-logs/testlog.txt; exit 1; } ;"
+
+msys2-64bit:
+  extends: .shared_msys2_builder
+  variables:
+MINGW_TARGET: mingw-w64-x86_64
+MSYSTEM: MINGW64
+# do not remove "--without-default-devices"!
+# commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using 
--without-default-devices"
+# changed to compile QEMU with the --without-default-devices switch
+# for the msys2 64-bit job, due to the build could not complete within
+CONFIGURE_ARGS:  --target-list=x86_64-softmmu --without-default-devices
+# qTests don't run successfully with "--without-default-devices",
+# so let's exclude the qtests from CI for now.
+TEST_ARGS: --no-suite qtest
 
 msys2-32bit:
   extends: .shared_msys2_builder
-  script:
-  - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
-  bison diffutils flex
-  git grep make sed
-  mingw-w64-i686-capstone
-  mingw-w64-i686-curl
-  mingw-w64-i686-cyrus-sasl
-  mingw-w64-i686-dtc
-  mingw-w64-i686-gcc
-  mingw-w64-i686-glib2
-  mingw-w64-i686-gnutls
-  mingw-w64-i686-gtk3
-  mingw-w64-i686-libgcrypt
-  mingw-w64-i686-libjpeg-turbo
-  mingw-w64-i686-libnfs
-

[PULL 8/9] gitlab: disable optimization and debug symbols in msys build

[Thomas Huth]

From: Daniel P. Berrangé 

Building at -O2, adds 33% to the build time, over -O2. IOW a build that
takes 45 minutes at -O0, takes 60 minutes at -O2. Turning off debug
symbols drops it further, down to 38 minutes.

IOW, a "-O2 -g" build is 58% slower than a "-O0" build on msys in the
gitlab CI windows shared runners.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20230801130403.164060-8-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 34109a80f2..552e3b751d 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -113,7 +113,7 @@ msys2-64bit:
 # commit 9f8e6cad65a6 ("gitlab-ci: Speed up the msys2-64bit job by using 
--without-default-devices"
 # changed to compile QEMU with the --without-default-devices switch
 # for the msys2 64-bit job, due to the build could not complete within
-CONFIGURE_ARGS:  --target-list=x86_64-softmmu --without-default-devices
+CONFIGURE_ARGS:  --target-list=x86_64-softmmu --without-default-devices 
-Ddebug=false -Doptimization=0
 # qTests don't run successfully with "--without-default-devices",
 # so let's exclude the qtests from CI for now.
 TEST_ARGS: --no-suite qtest
@@ -123,5 +123,5 @@ msys2-32bit:
   variables:
 MINGW_TARGET: mingw-w64-i686
 MSYSTEM: MINGW32
-CONFIGURE_ARGS:  --target-list=ppc64-softmmu
+CONFIGURE_ARGS:  --target-list=ppc64-softmmu -Ddebug=false -Doptimization=0
 TEST_ARGS: --no-suite qtest
-- 
2.39.3

[PULL 6/9] gitlab: always populate cache for windows msys jobs

[Thomas Huth]

From: Daniel P. Berrangé 

The cache is used to hold the msys installer. Even if the build phase
fails, we should still populate the cache as the installer will be
valid for next time.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Thomas Huth 
Message-Id: <20230801130403.164060-6-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 6454880cb7..34109a80f2 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -8,6 +8,7 @@
 key: "${CI_JOB_NAME}-cache"
 paths:
   - msys64/var/cache
+when: always
   needs: []
   stage: build
   timeout: 80m
-- 
2.39.3

[PULL 7/9] configure: support passthrough of -Dxxx args to meson

[Thomas Huth]

From: Daniel P. Berrangé 

This can be useful for setting some meson global options, such as the
optimization level or debug state.xs

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20230801130403.164060-7-berra...@redhat.com>
[thuth: Move the help text into the section with the other --... options]
Signed-off-by: Thomas Huth 
---
 configure | 4 
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 26ec5e4f54..afb25fd558 100755
--- a/configure
+++ b/configure
@@ -757,6 +757,9 @@ for opt do
   # everything else has the same name in configure and meson
   --*) meson_option_parse "$opt" "$optarg"
   ;;
+  # Pass through -D options to meson
+  -D*) meson_options="$meson_options $opt"
+  ;;
   esac
 done
 
@@ -846,6 +849,7 @@ $(echo Available targets: $default_target_list | \
   --target-list-exclude=LIST exclude a set of targets from the default 
target-list
 
 Advanced options (experts only):
+  -Dmesonoptname=val   passthrough option to meson unmodified
   --cross-prefix=PREFIXuse PREFIX for compile tools, PREFIX can be blank 
[$cross_prefix]
   --cc=CC  use C compiler CC [$cc]
   --host-cc=CC use C compiler CC [$host_cc] for code run at
-- 
2.39.3

[PULL 5/9] gitlab: drop $CI_PROJECT_DIR from cache path

[Thomas Huth]

From: Daniel P. Berrangé 

The gitlab cache is limited to only handle content within the
$CI_PROJECT_DIR hierarchy, and as such relative paths are always
implicitly relative to $CI_PROJECT_DIR.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Thomas Huth 
Message-Id: <20230801130403.164060-5-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 0bc04ad068..6454880cb7 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -7,7 +7,7 @@
   cache:
 key: "${CI_JOB_NAME}-cache"
 paths:
-  - ${CI_PROJECT_DIR}/msys64/var/cache
+  - msys64/var/cache
   needs: []
   stage: build
   timeout: 80m
-- 
2.39.3

[PULL 9/9] gitlab: disable FF_SCRIPT_SECTIONS on msys jobs

[Thomas Huth]

From: Daniel P. Berrangé 

The FF_SCRIPT_SECTIONS=1 variable should ordinarily cause output from
each line of the job script to be presented in a collapsible section
with execution time listed.

While it works on Linux shared runners, when used with Windows runners
with PowerShell, this option does not create any sections, and actually
causes echo'ing of commands to be disabled, making it even worse to
debug the jobs.

Signed-off-by: Daniel P. Berrangé 
Acked-by: Thomas Huth 
Message-Id: <20230801130403.164060-9-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 4 
 1 file changed, 4 insertions(+)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 552e3b751d..cd7622a761 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -12,6 +12,10 @@
   needs: []
   stage: build
   timeout: 80m
+  variables:
+# This feature doesn't (currently) work with PowerShell, it stops
+# the echo'ing of commands being run and doesn't show any timing
+FF_SCRIPT_SECTIONS: 0
   artifacts:
 name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
 expire_in: 7 days
-- 
2.39.3

[PULL 3/9] gitlab: print timestamps during windows msys jobs

[Thomas Huth]

From: Daniel P. Berrangé 

It is hard to get visibility into where time is consumed in our Windows
msys jobs. Adding a few log console messages with the timestamp will
aid in our debugging.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Thomas Huth 
Message-Id: <20230801130403.164060-3-berra...@redhat.com>
Signed-off-by: Thomas Huth 
---
 .gitlab-ci.d/windows.yml | 5 +
 1 file changed, 5 insertions(+)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index f086540e40..831b080d12 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -19,6 +19,7 @@
 reports:
   junit: "build/meson-logs/testlog.junit.xml"
   before_script:
+  - Write-Output "Acquiring msys2.exe installer at $(Get-Date -Format u)"
   - If ( !(Test-Path -Path msys64\var\cache ) ) {
   mkdir msys64\var\cache
 }
@@ -27,6 +28,7 @@
   
"https://github.com/msys2/msys2-installer/releases/download/2022-06-03/msys2-base-x86_64-20220603.sfx.exe";
   -outfile "msys64\var\cache\msys2.exe"
 }
+  - Write-Output "Invoking msys2.exe installer at $(Get-Date -Format u)"
   - msys64\var\cache\msys2.exe -y
   - ((Get-Content -path .\msys64\etc\\post-install\\07-pacman-key.post -Raw)
   -replace '--refresh-keys', '--version') |
@@ -36,6 +38,7 @@
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
   - taskkill /F /FI "MODULES eq msys-2.0.dll"
   script:
+  - Write-Output "Installing mingw packages at $(Get-Date -Format u)"
   - .\msys64\usr\bin\bash -lc "pacman -Sy --noconfirm --needed
   bison diffutils flex
   git grep make sed
@@ -66,6 +69,7 @@
   $MINGW_TARGET-spice
   $MINGW_TARGET-usbredir
   $MINGW_TARGET-zstd "
+  - Write-Output "Running build at $(Get-Date -Format u)"
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
   - mkdir build
@@ -73,6 +77,7 @@
   - ..\msys64\usr\bin\bash -lc "../configure --enable-fdt=system 
$CONFIGURE_ARGS"
   - ..\msys64\usr\bin\bash -lc "make"
   - ..\msys64\usr\bin\bash -lc "make check MTESTARGS='$TEST_ARGS' || { cat 
meson-logs/testlog.txt; exit 1; } ;"
+  - Write-Output "Finished build at $(Get-Date -Format u)"
 
 msys2-64bit:
   extends: .shared_msys2_builder
-- 
2.39.3

Re: [PATCH 3/8] gitlab: always use updated msys installer

[Daniel P . Berrangé]

On Wed, Aug 02, 2023 at 05:49:42PM +0200, Thomas Huth wrote:
> On 01/08/2023 15.03, Daniel P. Berrangé wrote:
> > We current reference an msys installer binary from mid-2022, which means
> > after installation, it immediately has to re-download a bunch of newer
> > content. This wastes precious CI time.
> > 
> > The msys project publishes an installer binary with a fixed URL that
> > always references the latest content. We cache the downloads in gitlab
> > though and so once downloaded we would never re-fetch the installer
> > leading back to the same problem.
> > 
> > To deal with this we also fetch the pgp signature for the installer
> > on every run, and compare that to the previously cached signature. If
> > the signature changes, we re-download the full installer.
> > 
> > This ensures we always have the latest installer for msys, while also
> > maximising use of the gitlab cache.
> > 
> > Signed-off-by: Daniel P. Berrangé 


> Reviewed-by: Thomas Huth 
> 
> ... I think the original idea was to use a "tagged" version to avoid that we
> have some kind of "rolling release" here, but since the latest content is
> fetched anyway during the following update, that idea was likely not working
> as expected...

I think using the "latest" installer is also more in keeping with what
we do for other distros, where we'll always pick up the latest content
when the containers get rebuilt, or macOS/FreeBSD where we pick the
latest from Ports/HomeBrew.  

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: QEMU developers fortnightly conference for 2023-08-08

[Juan Quintela]

Mark Burton  wrote:
> Are too many people away right now?

Hi

Some of us are still working O:-)

But I think that the easiest way to cancel it is if nobody puts an
agenda.

Later, Juan.

[PATCH] Add api to read CPU registers in TCG plugins

[Mikhail Tyutin]

From: Aleksandr Anenkov 

This commit:
- adds a plugin API to read the registers of the current CPU
- introduces qemu_plugin_reg_ctx structure for faster data gathering of
  a set of registers without memory reallocation
- adds an example plugin showing how to work with this API
- fixes for GDB that read wrong register 'eflags' value
- minor performance improvements when reading registers from GDB

In this commit, we rely on the already written register
reading code that QEMU for GDB has.

Why is the GDB code used
Each CPU architecture contains its own set of registers.
At the same time, QEMU does not contain a unified code architecture
for working with registers in various CPU architectures.
Each implementation of the CPU architecture in QEMU locates, reads,
and writes registers differently. In addition, each register has
its own specifics for reading and writing. Fortunately,
the GDB part of QEMU code already contains something unified
and complete. So in terms of simplicity and minimal code
changes of QEMU, we're just reusing what's already in GDB.
It works without having to run the GDB server.

How it works
The existing GDB code in QEMU already knows how to read registers
by register number, but cannot do it by register name.
QEMU has xml files using GDB Target Description Format to describe
targets sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html
from where GDB gets information about all registers. It only remained
to "teach" QEMU to read register names from these files the way GDB
itself does it and remember them in order to convert back to numbers
from the user API.

Signed-off-by: Aleksandr Anenkov 
---
 contrib/plugins/Makefile |   1 +
 contrib/plugins/registers.c  | 231 +
 gdb-xml/i386-32bit.xml   |   9 ++
 gdb-xml/i386-64bit.xml   |  17 +++
 gdbstub/gdbstub.c| 280 +--
 include/exec/gdbstub.h   |  36 +
 include/hw/core/cpu.h|   3 +
 include/qemu/qemu-plugin.h   | 105 -
 plugins/api.c| 223 
 plugins/qemu-plugins.symbols |  10 ++
 target/arm/cpu.c |   2 +
 target/arm/gdbstub.c |   2 +-
 target/i386/cpu.c|   6 +-
 target/i386/gdbstub.c|  25 +++-
 target/riscv/cpu.c   |   3 +
 target/riscv/gdbstub.c   |   3 +-
 16 files changed, 933 insertions(+), 23 deletions(-)
 create mode 100644 contrib/plugins/registers.c

diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
index b2b9db9f51..e14c07ddcc 100644
--- a/contrib/plugins/Makefile
+++ b/contrib/plugins/Makefile
@@ -21,6 +21,7 @@ NAMES += lockstep
 NAMES += hwprofile
 NAMES += cache
 NAMES += drcov
+NAMES += registers
 
 SONAMES := $(addsuffix .so,$(addprefix lib,$(NAMES)))
 
diff --git a/contrib/plugins/registers.c b/contrib/plugins/registers.c
new file mode 100644
index 00..ae40a27f5f
--- /dev/null
+++ b/contrib/plugins/registers.c
@@ -0,0 +1,231 @@
+/*
+ * Log register states
+ *
+ * Copyright (c) 2022 YADRO.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#include 
+#include 
+#include 
+
+#include 
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+/* Print report to file every N instructions */
+#define REPORT_BUF_N_INSN 100
+
+typedef enum target_t {
+UNKNOWN_TARGET,
+X86_64_TARGET,
+RISCV64_TARGET
+} target_t;
+
+target_t target = UNKNOWN_TARGET;
+bool system_emulation = false;
+
+const char *const X86_64_REGS[] = { "rax", "rbx", "rcx", "rdx", "rsi", "rdi",
+"rbp", "rsp", "rip", "eflags", "xmm0", 
"ymm0h" };
+const char *const RISCV64_REGS[] = { "zero", "ra", "sp", "gp", "tp", "t0",
+ "a0", "a1", "ft0", "vstart" };
+
+/*
+ * Each vcpu has its own independent data set, which is only initialized once
+ */
+typedef struct vcpu_cache {
+struct qemu_plugin_reg_ctx *reg_ctx;
+GString *report;
+size_t report_counter;
+unsigned int vcpu_index;
+} vcpu_cache;
+
+vcpu_cache *caches = NULL;
+
+static void print_register_values(GString *report, const void *data, size_t 
size)
+{
+if (size == 4) {
+g_string_append_printf(report, "%08x", *(uint32_t *)data);
+}
+else if (size == 8) {
+g_string_append_printf(report, "%016" PRIx64, *(uint64_t *)data);
+}
+else if (size % sizeof(uint64_t) == 0) {
+

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

[Daniel Henrique Barboza]

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

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

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

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

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

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

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

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

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

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

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

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


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

  uint32_t reg = cpu->cfg.mvendorid;

and then...


  uint64_t id;
  int ret;
  
  id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,

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


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


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

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


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

target_ulong seems that the right choice here. We could perhaps work with
uint64_t (other parts of the code does that) but target_ulong is nicer with
32-bit setups.


Thanks,

Daniel




+reg = cpu->cfg.mvendorid;
+ret = kvm_set_one_reg(cs, id, ®);
  if (ret != 0) {
  return ret;
  }
--
2.41.0



We should audit and fix all uses of &cpu->cfg.* with KVM ioctls. We can
also consider introducing wrappers like

#define kvm_set_one_reg_safe(cs, id, addr)  \
({  \
typeof(*(addr)) _addr = *(addr);\
kvm_set_one_reg(cs, id, &_addr) \
})

Thanks,
drew

[PATCH v9 0/4] hw/ufs: Add Universal Flash Storage (UFS) support

[Jeuk Kim]

Dear Stefan,
I'm really sorry, but could you please put this patch series
instead of v8, which was previously merged into block-next?
The fixes from v8 are below.
Please let me know if you have any comments or issues.

Thank you very much,
Jeuk

Since v8:
- Fix compilation warnings (Mike Maslenkin reported. Thanks so much Mike!)
- Skip ufs-test for qemu-system-ppc64
I have confirmed that the dma in ufs-test does not work well
for qemu-system-ppc64.
This seems to be because qemu-system-ppc64 is the only big-endian system test.
Since there are currently no ufs devices supported on big-endian systems,
I just skip the test for now, and leave it as a TODO.

Jeuk Kim (4):
  hw/ufs: Initial commit for emulated Universal-Flash-Storage
  hw/ufs: Support for Query Transfer Requests
  hw/ufs: Support for UFS logical unit
  tests/qtest: Introduce tests for UFS

 MAINTAINERS  |7 +
 docs/specs/pci-ids.rst   |2 +
 hw/Kconfig   |1 +
 hw/meson.build   |1 +
 hw/ufs/Kconfig   |4 +
 hw/ufs/lu.c  | 1445 
 hw/ufs/meson.build   |1 +
 hw/ufs/trace-events  |   58 ++
 hw/ufs/trace.h   |1 +
 hw/ufs/ufs.c | 1494 ++
 hw/ufs/ufs.h |  131 
 include/block/ufs.h  | 1090 +++
 include/hw/pci/pci.h |1 +
 include/hw/pci/pci_ids.h |1 +
 include/scsi/constants.h |1 +
 meson.build  |1 +
 tests/qtest/meson.build  |1 +
 tests/qtest/ufs-test.c   |  584 +++
 18 files changed, 4824 insertions(+)
 create mode 100644 hw/ufs/Kconfig
 create mode 100644 hw/ufs/lu.c
 create mode 100644 hw/ufs/meson.build
 create mode 100644 hw/ufs/trace-events
 create mode 100644 hw/ufs/trace.h
 create mode 100644 hw/ufs/ufs.c
 create mode 100644 hw/ufs/ufs.h
 create mode 100644 include/block/ufs.h
 create mode 100644 tests/qtest/ufs-test.c

-- 
2.34.1

[PATCH v9 1/4] hw/ufs: Initial commit for emulated Universal-Flash-Storage

[Jeuk Kim]

Universal Flash Storage (UFS) is a high-performance mass storage device
with a serial interface. It is primarily used as a high-performance
data storage device for embedded applications.

This commit contains code for UFS device to be recognized
as a UFS PCI device.
Patches to handle UFS logical unit and Transfer Request will follow.

Signed-off-by: Jeuk Kim 
Reviewed-by: Stefan Hajnoczi 
---
 MAINTAINERS  |6 +
 docs/specs/pci-ids.rst   |2 +
 hw/Kconfig   |1 +
 hw/meson.build   |1 +
 hw/ufs/Kconfig   |4 +
 hw/ufs/meson.build   |1 +
 hw/ufs/trace-events  |   32 ++
 hw/ufs/trace.h   |1 +
 hw/ufs/ufs.c |  278 ++
 hw/ufs/ufs.h |   42 ++
 include/block/ufs.h  | 1090 ++
 include/hw/pci/pci.h |1 +
 include/hw/pci/pci_ids.h |1 +
 meson.build  |1 +
 14 files changed, 1461 insertions(+)
 create mode 100644 hw/ufs/Kconfig
 create mode 100644 hw/ufs/meson.build
 create mode 100644 hw/ufs/trace-events
 create mode 100644 hw/ufs/trace.h
 create mode 100644 hw/ufs/ufs.c
 create mode 100644 hw/ufs/ufs.h
 create mode 100644 include/block/ufs.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6111b6b4d9..7da40dabc2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2256,6 +2256,12 @@ F: tests/qtest/nvme-test.c
 F: docs/system/devices/nvme.rst
 T: git git://git.infradead.org/qemu-nvme.git nvme-next
 
+ufs
+M: Jeuk Kim 
+S: Supported
+F: hw/ufs/*
+F: include/block/ufs.h
+
 megasas
 M: Hannes Reinecke 
 L: qemu-bl...@nongnu.org
diff --git a/docs/specs/pci-ids.rst b/docs/specs/pci-ids.rst
index e302bea484..d6707fa069 100644
--- a/docs/specs/pci-ids.rst
+++ b/docs/specs/pci-ids.rst
@@ -92,6 +92,8 @@ PCI devices (other than virtio):
   PCI PVPanic device (``-device pvpanic-pci``)
 1b36:0012
   PCI ACPI ERST device (``-device acpi-erst``)
+1b36:0013
+  PCI UFS device (``-device ufs``)
 
 All these devices are documented in :doc:`index`.
 
diff --git a/hw/Kconfig b/hw/Kconfig
index ba62ff6417..9ca7b38c31 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -38,6 +38,7 @@ source smbios/Kconfig
 source ssi/Kconfig
 source timer/Kconfig
 source tpm/Kconfig
+source ufs/Kconfig
 source usb/Kconfig
 source virtio/Kconfig
 source vfio/Kconfig
diff --git a/hw/meson.build b/hw/meson.build
index c7ac7d3d75..f01fac4617 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -37,6 +37,7 @@ subdir('smbios')
 subdir('ssi')
 subdir('timer')
 subdir('tpm')
+subdir('ufs')
 subdir('usb')
 subdir('vfio')
 subdir('virtio')
diff --git a/hw/ufs/Kconfig b/hw/ufs/Kconfig
new file mode 100644
index 00..b7b3392e85
--- /dev/null
+++ b/hw/ufs/Kconfig
@@ -0,0 +1,4 @@
+config UFS_PCI
+bool
+default y if PCI_DEVICES
+depends on PCI
diff --git a/hw/ufs/meson.build b/hw/ufs/meson.build
new file mode 100644
index 00..eb5164bde9
--- /dev/null
+++ b/hw/ufs/meson.build
@@ -0,0 +1 @@
+system_ss.add(when: 'CONFIG_UFS_PCI', if_true: files('ufs.c'))
diff --git a/hw/ufs/trace-events b/hw/ufs/trace-events
new file mode 100644
index 00..d1badcad10
--- /dev/null
+++ b/hw/ufs/trace-events
@@ -0,0 +1,32 @@
+# ufs.c
+ufs_irq_raise(void) "INTx"
+ufs_irq_lower(void) "INTx"
+ufs_mmio_read(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" 
data 0x%"PRIx64" size %d"
+ufs_mmio_write(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" 
data 0x%"PRIx64" size %d"
+ufs_process_db(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_process_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_complete_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_sendback_req(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_exec_nop_cmd(uint32_t slot) "UTRLDBR slot %"PRIu32""
+ufs_exec_scsi_cmd(uint32_t slot, uint8_t lun, uint8_t opcode) "slot %"PRIu32", 
lun 0x%"PRIx8", opcode 0x%"PRIx8""
+ufs_exec_query_cmd(uint32_t slot, uint8_t opcode) "slot %"PRIu32", opcode 
0x%"PRIx8""
+ufs_process_uiccmd(uint32_t uiccmd, uint32_t ucmdarg1, uint32_t ucmdarg2, 
uint32_t ucmdarg3) "uiccmd 0x%"PRIx32", ucmdarg1 0x%"PRIx32", ucmdarg2 
0x%"PRIx32", ucmdarg3 0x%"PRIx32""
+
+# error condition
+ufs_err_dma_read_utrd(uint32_t slot, uint64_t addr) "failed to read utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
+ufs_err_dma_read_req_upiu(uint32_t slot, uint64_t addr) "failed to read req 
upiu. UTRLDBR slot %"PRIu32", request upiu addr %"PRIu64""
+ufs_err_dma_read_prdt(uint32_t slot, uint64_t addr) "failed to read prdt. 
UTRLDBR slot %"PRIu32", prdt addr %"PRIu64""
+ufs_err_dma_write_utrd(uint32_t slot, uint64_t addr) "failed to write utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
+ufs_err_dma_write_rsp_upiu(uint32_t slot, uint64_t addr) "failed to write rsp 
upiu. UTRLDBR slot %"PRIu32", response upiu addr %"PRIu64""
+ufs_err_utrl_slot_busy(uint32_t slot) "UTRLDBR slot %"PRIu32" is busy"
+ufs_err_unsupport_register_offset(uint32_t offset) "Register offset 
0x%"PRIx32" is not yet supported"
+ufs_err_inva

[PATCH v9 4/4] tests/qtest: Introduce tests for UFS

[Jeuk Kim]

This patch includes the following tests
  Test mmio read
  Test ufs device initialization and ufs-lu recognition
  Test I/O (Performs a write followed by a read to verify)

Signed-off-by: Jeuk Kim 
Acked-by: Thomas Huth 
Reviewed-by: Stefan Hajnoczi 
---
 MAINTAINERS |   1 +
 tests/qtest/meson.build |   1 +
 tests/qtest/ufs-test.c  | 584 
 3 files changed, 586 insertions(+)
 create mode 100644 tests/qtest/ufs-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7da40dabc2..a45bac20c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2261,6 +2261,7 @@ M: Jeuk Kim 
 S: Supported
 F: hw/ufs/*
 F: include/block/ufs.h
+F: tests/qtest/ufs-test.c
 
 megasas
 M: Hannes Reinecke 
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index b071d400b3..2b1d589a87 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -269,6 +269,7 @@ qos_test_ss.add(
   'virtio-iommu-test.c',
   'vmxnet3-test.c',
   'igb-test.c',
+  'ufs-test.c',
 )
 
 if config_all_devices.has_key('CONFIG_VIRTIO_SERIAL')
diff --git a/tests/qtest/ufs-test.c b/tests/qtest/ufs-test.c
new file mode 100644
index 00..34c97bdf1b
--- /dev/null
+++ b/tests/qtest/ufs-test.c
@@ -0,0 +1,584 @@
+/*
+ * QTest testcase for UFS
+ *
+ * Copyright (c) 2023 Samsung Electronics Co., Ltd. All rights reserved.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu/units.h"
+#include "libqtest.h"
+#include "libqos/qgraph.h"
+#include "libqos/pci.h"
+#include "scsi/constants.h"
+#include "include/block/ufs.h"
+
+/* Test images sizes in Bytes */
+#define TEST_IMAGE_SIZE (64 * 1024 * 1024)
+/* Timeout for various operations, in seconds. */
+#define TIMEOUT_SECONDS 10
+/* Maximum PRD entry count */
+#define MAX_PRD_ENTRY_COUNT 10
+#define PRD_ENTRY_DATA_SIZE 4096
+/* Constants to build upiu */
+#define UTP_COMMAND_DESCRIPTOR_SIZE 4096
+#define UTP_RESPONSE_UPIU_OFFSET 1024
+#define UTP_PRDT_UPIU_OFFSET 2048
+
+typedef struct QUfs QUfs;
+
+struct QUfs {
+QOSGraphObject obj;
+QPCIDevice dev;
+QPCIBar bar;
+
+uint64_t utrlba;
+uint64_t utmrlba;
+uint64_t cmd_desc_addr;
+uint64_t data_buffer_addr;
+
+bool enabled;
+};
+
+static inline uint32_t ufs_rreg(QUfs *ufs, size_t offset)
+{
+return qpci_io_readl(&ufs->dev, ufs->bar, offset);
+}
+
+static inline void ufs_wreg(QUfs *ufs, size_t offset, uint32_t value)
+{
+qpci_io_writel(&ufs->dev, ufs->bar, offset, value);
+}
+
+static void ufs_wait_for_irq(QUfs *ufs)
+{
+uint64_t end_time;
+uint32_t is;
+/* Wait for device to reset as the linux driver does. */
+end_time = g_get_monotonic_time() + TIMEOUT_SECONDS * G_TIME_SPAN_SECOND;
+do {
+qtest_clock_step(ufs->dev.bus->qts, 100);
+is = ufs_rreg(ufs, A_IS);
+} while (is == 0 && g_get_monotonic_time() < end_time);
+}
+
+static UtpTransferReqDesc ufs_build_req_utrd(uint64_t cmd_desc_addr,
+ uint8_t slot,
+ uint32_t data_direction,
+ uint16_t prd_table_length)
+{
+UtpTransferReqDesc req = { 0 };
+uint64_t command_desc_base_addr =
+cmd_desc_addr + slot * UTP_COMMAND_DESCRIPTOR_SIZE;
+
+req.header.dword_0 =
+cpu_to_le32(1 << 28 | data_direction | UTP_REQ_DESC_INT_CMD);
+req.header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+
+req.command_desc_base_addr_hi = cpu_to_le32(command_desc_base_addr >> 32);
+req.command_desc_base_addr_lo =
+cpu_to_le32(command_desc_base_addr & 0x);
+req.response_upiu_offset =
+cpu_to_le16(UTP_RESPONSE_UPIU_OFFSET / sizeof(uint32_t));
+req.response_upiu_length = cpu_to_le16(sizeof(UtpUpiuRsp));
+req.prd_table_offset = cpu_to_le16(UTP_PRDT_UPIU_OFFSET / 
sizeof(uint32_t));
+req.prd_table_length = cpu_to_le16(prd_table_length);
+return req;
+}
+
+static void ufs_send_nop_out(QUfs *ufs, uint8_t slot,
+ UtpTransferReqDesc *utrd_out, UtpUpiuRsp *rsp_out)
+{
+/* Build up utp transfer request descriptor */
+UtpTransferReqDesc utrd =
+ufs_build_req_utrd(ufs->cmd_desc_addr, slot, UTP_NO_DATA_TRANSFER, 0);
+uint64_t utrd_addr = ufs->utrlba + slot * sizeof(UtpTransferReqDesc);
+uint64_t req_upiu_addr =
+ufs->cmd_desc_addr + slot * UTP_COMMAND_DESCRIPTOR_SIZE;
+uint64_t rsp_upiu_addr = req_upiu_addr + UTP_RESPONSE_UPIU_OFFSET;
+qtest_memwrite(ufs->dev.bus->qts, utrd_addr, &utrd, sizeof(utrd));
+
+/* Build up request upiu */
+UtpUpiuReq req_upiu = { 0 };
+req_upiu.header.trans_type = UPIU_TRANSACTION_NOP_OUT;
+req_upiu.header.task_tag = slot;
+qtest_memwrite(ufs->dev.bus->qts, req_upiu_addr, &req_upiu,
+   sizeof(req_upiu));
+
+/* Ring Doorbell */
+ufs_wreg(ufs, A_UTRLDBR, 1);
+ufs_wait_for_irq(ufs);
+g_assert_true(FIEL

[PATCH v9 2/4] hw/ufs: Support for Query Transfer Requests

[Jeuk Kim]

This commit makes the UFS device support query
and nop out transfer requests.

The next patch would be support for UFS logical
unit and scsi command transfer request.

Signed-off-by: Jeuk Kim 
Reviewed-by: Stefan Hajnoczi 
---
 hw/ufs/trace-events |   1 +
 hw/ufs/ufs.c| 980 +++-
 hw/ufs/ufs.h|  46 +++
 3 files changed, 1025 insertions(+), 2 deletions(-)

diff --git a/hw/ufs/trace-events b/hw/ufs/trace-events
index d1badcad10..665e1a942b 100644
--- a/hw/ufs/trace-events
+++ b/hw/ufs/trace-events
@@ -18,6 +18,7 @@ ufs_err_dma_read_req_upiu(uint32_t slot, uint64_t addr) 
"failed to read req upiu
 ufs_err_dma_read_prdt(uint32_t slot, uint64_t addr) "failed to read prdt. 
UTRLDBR slot %"PRIu32", prdt addr %"PRIu64""
 ufs_err_dma_write_utrd(uint32_t slot, uint64_t addr) "failed to write utrd. 
UTRLDBR slot %"PRIu32", UTRD dma addr %"PRIu64""
 ufs_err_dma_write_rsp_upiu(uint32_t slot, uint64_t addr) "failed to write rsp 
upiu. UTRLDBR slot %"PRIu32", response upiu addr %"PRIu64""
+ufs_err_utrl_slot_error(uint32_t slot) "UTRLDBR slot %"PRIu32" is in error"
 ufs_err_utrl_slot_busy(uint32_t slot) "UTRLDBR slot %"PRIu32" is busy"
 ufs_err_unsupport_register_offset(uint32_t offset) "Register offset 
0x%"PRIx32" is not yet supported"
 ufs_err_invalid_register_offset(uint32_t offset) "Register offset 0x%"PRIx32" 
is invalid"
diff --git a/hw/ufs/ufs.c b/hw/ufs/ufs.c
index 101082a8a3..c771f8e0b4 100644
--- a/hw/ufs/ufs.c
+++ b/hw/ufs/ufs.c
@@ -15,10 +15,221 @@
 #include "ufs.h"
 
 /* The QEMU-UFS device follows spec version 3.1 */
-#define UFS_SPEC_VER 0x0310
+#define UFS_SPEC_VER 0x0310
 #define UFS_MAX_NUTRS 32
 #define UFS_MAX_NUTMRS 8
 
+static MemTxResult ufs_addr_read(UfsHc *u, hwaddr addr, void *buf, int size)
+{
+hwaddr hi = addr + size - 1;
+
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_read(PCI_DEVICE(u), addr, buf, size);
+}
+
+static MemTxResult ufs_addr_write(UfsHc *u, hwaddr addr, const void *buf,
+  int size)
+{
+hwaddr hi = addr + size - 1;
+if (hi < addr) {
+return MEMTX_DECODE_ERROR;
+}
+
+if (!FIELD_EX32(u->reg.cap, CAP, 64AS) && (hi >> 32)) {
+return MEMTX_DECODE_ERROR;
+}
+
+return pci_dma_write(PCI_DEVICE(u), addr, buf, size);
+}
+
+static void ufs_complete_req(UfsRequest *req, UfsReqResult req_result);
+
+static inline hwaddr ufs_get_utrd_addr(UfsHc *u, uint32_t slot)
+{
+hwaddr utrl_base_addr = (((hwaddr)u->reg.utrlbau) << 32) + u->reg.utrlba;
+hwaddr utrd_addr = utrl_base_addr + slot * sizeof(UtpTransferReqDesc);
+
+return utrd_addr;
+}
+
+static inline hwaddr ufs_get_req_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+uint32_t cmd_desc_base_addr_lo =
+le32_to_cpu(utrd->command_desc_base_addr_lo);
+uint32_t cmd_desc_base_addr_hi =
+le32_to_cpu(utrd->command_desc_base_addr_hi);
+
+return (((hwaddr)cmd_desc_base_addr_hi) << 32) + cmd_desc_base_addr_lo;
+}
+
+static inline hwaddr ufs_get_rsp_upiu_base_addr(const UtpTransferReqDesc *utrd)
+{
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(utrd);
+uint32_t rsp_upiu_byte_off =
+le16_to_cpu(utrd->response_upiu_offset) * sizeof(uint32_t);
+return req_upiu_base_addr + rsp_upiu_byte_off;
+}
+
+static MemTxResult ufs_dma_read_utrd(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr utrd_addr = ufs_get_utrd_addr(u, req->slot);
+MemTxResult ret;
+
+ret = ufs_addr_read(u, utrd_addr, &req->utrd, sizeof(req->utrd));
+if (ret) {
+trace_ufs_err_dma_read_utrd(req->slot, utrd_addr);
+}
+return ret;
+}
+
+static MemTxResult ufs_dma_read_req_upiu(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+hwaddr req_upiu_base_addr = ufs_get_req_upiu_base_addr(&req->utrd);
+UtpUpiuReq *req_upiu = &req->req_upiu;
+uint32_t copy_size;
+uint16_t data_segment_length;
+MemTxResult ret;
+
+/*
+ * To know the size of the req_upiu, we need to read the
+ * data_segment_length in the header first.
+ */
+ret = ufs_addr_read(u, req_upiu_base_addr, &req_upiu->header,
+sizeof(UtpUpiuHeader));
+if (ret) {
+trace_ufs_err_dma_read_req_upiu(req->slot, req_upiu_base_addr);
+return ret;
+}
+data_segment_length = be16_to_cpu(req_upiu->header.data_segment_length);
+
+copy_size = sizeof(UtpUpiuHeader) + UFS_TRANSACTION_SPECIFIC_FIELD_SIZE +
+data_segment_length;
+
+ret = ufs_addr_read(u, req_upiu_base_addr, &req->req_upiu, copy_size);
+if (ret) {
+trace_ufs_err_dma_read_req_upiu(req->slot, req_upiu_base_addr);
+}
+return ret;
+}
+
+static MemTxResult ufs_dma_read_prdt(UfsRequest *req)
+{
+UfsHc *u = req->hc;
+uint16_t prdt_len = le16_to_cpu(req->utrd.prd_table_length);
+uint16_t p

[PATCH v9 3/4] hw/ufs: Support for UFS logical unit

[Jeuk Kim]

This commit adds support for ufs logical unit.
The LU handles processing for the SCSI command,
unit descriptor query request.

This commit enables the UFS device to process
IO requests.

Signed-off-by: Jeuk Kim 
Reviewed-by: Stefan Hajnoczi 
---
 hw/ufs/lu.c  | 1445 ++
 hw/ufs/meson.build   |2 +-
 hw/ufs/trace-events  |   25 +
 hw/ufs/ufs.c |  252 ++-
 hw/ufs/ufs.h |   43 ++
 include/scsi/constants.h |1 +
 6 files changed, 1761 insertions(+), 7 deletions(-)
 create mode 100644 hw/ufs/lu.c

diff --git a/hw/ufs/lu.c b/hw/ufs/lu.c
new file mode 100644
index 00..ec8932ff86
--- /dev/null
+++ b/hw/ufs/lu.c
@@ -0,0 +1,1445 @@
+/*
+ * QEMU UFS Logical Unit
+ *
+ * Copyright (c) 2023 Samsung Electronics Co., Ltd. All rights reserved.
+ *
+ * Written by Jeuk Kim 
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/units.h"
+#include "qapi/error.h"
+#include "qemu/memalign.h"
+#include "hw/scsi/scsi.h"
+#include "scsi/constants.h"
+#include "sysemu/block-backend.h"
+#include "qemu/cutils.h"
+#include "trace.h"
+#include "ufs.h"
+
+/*
+ * The code below handling SCSI commands is copied from hw/scsi/scsi-disk.c,
+ * with minor adjustments to make it work for UFS.
+ */
+
+#define SCSI_DMA_BUF_SIZE (128 * KiB)
+#define SCSI_MAX_INQUIRY_LEN 256
+#define SCSI_INQUIRY_DATA_SIZE 36
+#define SCSI_MAX_MODE_LEN 256
+
+typedef struct UfsSCSIReq {
+SCSIRequest req;
+/* Both sector and sector_count are in terms of BDRV_SECTOR_SIZE bytes.  */
+uint64_t sector;
+uint32_t sector_count;
+uint32_t buflen;
+bool started;
+bool need_fua_emulation;
+struct iovec iov;
+QEMUIOVector qiov;
+BlockAcctCookie acct;
+} UfsSCSIReq;
+
+static void ufs_scsi_free_request(SCSIRequest *req)
+{
+UfsSCSIReq *r = DO_UPCAST(UfsSCSIReq, req, req);
+
+qemu_vfree(r->iov.iov_base);
+}
+
+static void scsi_check_condition(UfsSCSIReq *r, SCSISense sense)
+{
+trace_ufs_scsi_check_condition(r->req.tag, sense.key, sense.asc,
+   sense.ascq);
+scsi_req_build_sense(&r->req, sense);
+scsi_req_complete(&r->req, CHECK_CONDITION);
+}
+
+static int ufs_scsi_emulate_vpd_page(SCSIRequest *req, uint8_t *outbuf,
+ uint32_t outbuf_len)
+{
+UfsHc *u = UFS(req->bus->qbus.parent);
+UfsLu *lu = DO_UPCAST(UfsLu, qdev, req->dev);
+uint8_t page_code = req->cmd.buf[2];
+int start, buflen = 0;
+
+if (outbuf_len < SCSI_INQUIRY_DATA_SIZE) {
+return -1;
+}
+
+outbuf[buflen++] = lu->qdev.type & 0x1f;
+outbuf[buflen++] = page_code;
+outbuf[buflen++] = 0x00;
+outbuf[buflen++] = 0x00;
+start = buflen;
+
+switch (page_code) {
+case 0x00: /* Supported page codes, mandatory */
+{
+trace_ufs_scsi_emulate_vpd_page_00(req->cmd.xfer);
+outbuf[buflen++] = 0x00; /* list of supported pages (this page) */
+if (u->params.serial) {
+outbuf[buflen++] = 0x80; /* unit serial number */
+}
+outbuf[buflen++] = 0x87; /* mode page policy */
+break;
+}
+case 0x80: /* Device serial number, optional */
+{
+int l;
+
+if (!u->params.serial) {
+trace_ufs_scsi_emulate_vpd_page_80_not_supported();
+return -1;
+}
+
+l = strlen(u->params.serial);
+if (l > SCSI_INQUIRY_DATA_SIZE) {
+l = SCSI_INQUIRY_DATA_SIZE;
+}
+
+trace_ufs_scsi_emulate_vpd_page_80(req->cmd.xfer);
+memcpy(outbuf + buflen, u->params.serial, l);
+buflen += l;
+break;
+}
+case 0x87: /* Mode Page Policy, mandatory */
+{
+trace_ufs_scsi_emulate_vpd_page_87(req->cmd.xfer);
+outbuf[buflen++] = 0x3f; /* apply to all mode pages and subpages */
+outbuf[buflen++] = 0xff;
+outbuf[buflen++] = 0; /* shared */
+outbuf[buflen++] = 0;
+break;
+}
+default:
+return -1;
+}
+/* done with EVPD */
+assert(buflen - start <= 255);
+outbuf[start - 1] = buflen - start;
+return buflen;
+}
+
+static int ufs_scsi_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf,
+uint32_t outbuf_len)
+{
+int buflen = 0;
+
+if (outbuf_len < SCSI_INQUIRY_DATA_SIZE) {
+return -1;
+}
+
+if (req->cmd.buf[1] & 0x1) {
+/* Vital product data */
+return ufs_scsi_emulate_vpd_page(req, outbuf, outbuf_len);
+}
+
+/* Standard INQUIRY data */
+if (req->cmd.buf[2] != 0) {
+return -1;
+}
+
+/* PAGE CODE == 0 */
+buflen = req->cmd.xfer;
+if (buflen > SCSI_MAX_INQUIRY_LEN) {
+buflen = SCSI_MAX_INQUIRY_LEN;
+}
+
+if (is_wlun(req->lun)) {
+outbuf[0] = TYPE_WLUN;
+} else {
+outbuf[0] = 0;
+}
+outbuf[1] = 0;
+
+strpadcpy((char *)&outbuf[16], 16

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

[Andrew Jones]

On Thu, Aug 03, 2023 at 08:36:57AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/3/23 06:29, Andrew Jones wrote:
> > On Wed, Aug 02, 2023 at 03:00:58PM -0300, Daniel Henrique Barboza wrote:
> > > cpu->cfg.mvendorid is a 32 bit field and kvm_set_one_reg() always write
> > > a target_ulong val, i.e. a 64 bit field in a 64 bit host.
> > > 
> > > Given that we're passing a pointer to the mvendorid field, the reg is
> > > reading 64 bits starting from mvendorid and going 32 bits in the next
> > > field, marchid. Here's an example:
> > > 
> > > $ ./qemu-system-riscv64 -machine virt,accel=kvm -m 2G -smp 1 \
> > > -cpu rv64,marchid=0xab,mvendorid=0xcd,mimpid=0xef(...)
> > > 
> > > (inside the guest)
> > >   # cat /proc/cpuinfo
> > > processor : 0
> > > hart  : 0
> > > isa   : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> > > mmu   : sv57
> > > mvendorid : 0xab00cd
> > > marchid   : 0xab
> > > mimpid: 0xef
> > > 
> > > 'mvendorid' was written as a combination of 0xab (the value from the
> > > adjacent field, marchid) and its intended value 0xcd.
> > > 
> > > Fix it by assigning cpu->cfg.mvendorid to a target_ulong var 'reg' and
> > > use it as input for kvm_set_one_reg(). Here's the result with this patch
> > > applied and using the same QEMU command line:
> > > 
> > >   # cat /proc/cpuinfo
> > > processor : 0
> > > hart  : 0
> > > isa   : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> > > mmu   : sv57
> > > mvendorid : 0xcd
> > > marchid   : 0xab
> > > mimpid: 0xef
> > > 
> > > This bug affects only the generic (rv64) CPUs when running with KVM in a
> > > 64 bit env since the 'host' CPU does not allow the machine IDs to be
> > > changed via command line.
> > > 
> > > Fixes: 1fb5a622f7 ("target/riscv: handle mvendorid/marchid/mimpid for KVM 
> > > CPUs")
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   target/riscv/kvm.c | 9 -
> > >   1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> > > index 9d8a8982f9..b1fd2233c0 100644
> > > --- a/target/riscv/kvm.c
> > > +++ b/target/riscv/kvm.c
> > > @@ -852,12 +852,19 @@ void kvm_arch_init_irq_routing(KVMState *s)
> > >   static int kvm_vcpu_set_machine_ids(RISCVCPU *cpu, CPUState *cs)
> > >   {
> > >   CPURISCVState *env = &cpu->env;
> > > +target_ulong reg;
> > 
> > We can use the type of cfg since KVM just gets an address and uses the
> > KVM register type to determine the size. So here,
> > 
> >   uint32_t reg = cpu->cfg.mvendorid;
> > 
> > and then...
> > 
> > >   uint64_t id;
> > >   int ret;
> > >   id = kvm_riscv_reg_id(env, KVM_REG_RISCV_CONFIG,
> > > KVM_REG_RISCV_CONFIG_REG(mvendorid));
> > > -ret = kvm_set_one_reg(cs, id, &cpu->cfg.mvendorid);
> > > +/*
> > > + * cfg.mvendorid is an uint32 but a target_ulong will
> > > + * be written. Assign it to a target_ulong var to avoid
> > > + * writing pieces of other cpu->cfg fields in the reg.
> > > + */
> > 
> > ...we don't need this comment since we're not doing anything special.
> 
> I tried it out and it doesn't seem to work. Here's the result:
> 
> / # cat /proc/cpuinfo
> processor : 0
> hart  : 0
> isa   : rv64imafdc_zicbom_zicboz_zihintpause_zbb_sstc
> mmu   : sv57
> mvendorid : 0xaa00cd
> marchid   : 0xab
> mimpid: 0xef
> 
> 
> The issue here is that the kernel considers 'mvendorid' as an unsigned long 
> (or
> what QEMU calls target_ulong). kvm_set_one_reg() will write an unsigned long
> regardless of the uint32_t typing of 'reg', meaning that it'll end up writing
> 32 bits of uninitialized stuff from the stack.

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

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

Thanks,
drew


> 
> target_ulong seems that the right choice here. We could perhaps work with
> uint64_t (other parts of the

Re: [PATCH v2 08/11] hw/loongarch/virt: add plug handler for TPM on SysBus

[Stefan Berger]

On 7/20/23 13:57, Stefan Berger wrote:



On 7/14/23 03:09, Joelle van Dyne wrote:

TPM needs to know its own base address in order to generate its DSDT
device entry.

Signed-off-by: Joelle van Dyne 



It would be great to also cover the crb-device  with tests:

from tests/qtest/meson.build:

   (config_all.has_key('CONFIG_TCG') and 
config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ?    \
     ['tpm-tis-device-test', 'tpm-tis-device-swtpm-test'] : []) +   
  \

It should be easy to make a copy of these two tis-device tests and rename them 
to crb-device tests, adapt them, and run them at least on aarch64.


... actually make a copy of the files tests/qtest/tpm-crb-swtpm-test.c & 
tests/qtest/tpm-crb-test.c.



Regards,
    Stefan

Re: [PATCH v7 13/14] linux-user: Adjust initial brk when interpreter is close to executable

[Helge Deller]

Hi Richard,

Thanks for putting this all together!
I'll test asap.

I haven't checked yet, but Akihiko did send a revised v2 patch
series, while my v6 series included his older v1 patches.
We should consider his latest series...

One other thing below

On 8/3/23 03:53, Richard Henderson wrote:

From: Helge Deller 

While we attempt to load a ET_DYN executable far away from
TASK_UNMAPPED_BASE, we are not completely in control of the
address space layout.  If the interpreter lands close to
the executable, leaving insufficient heap space, move brk.

Signed-off-by: Helge Deller 
[rth: Re-order after ELF_ET_DYN_BASE patch so that we do not
  "temporarily break" tsan, and also to minimize the changes required.
  Remove image_info.reserve_brk as unused.]
Signed-off-by: Richard Henderson 
---
  linux-user/qemu.h|  1 -
  linux-user/elfload.c | 51 +---
  2 files changed, 15 insertions(+), 37 deletions(-)
...
@@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, int 
image_fd,
  info->end_code = 0;
  info->start_data = -1;
  info->end_data = 0;
-info->brk = 0;
+/* Usual start for brk is after all sections of the main executable. */
+info->brk = TARGET_PAGE_ALIGN(hiaddr);


This is from my original patch, and is probably wrong.
I think this needs to be:
info->brk = HOST_PAGE_ALIGN(hiaddr);

The brk page needs to be aligned to the host page size variable (which
is always >= target page size).
The page will be mapped +rw (on host), so may need the distance to code/shared
libs below it, and for that distance target-alignment may not be sufficient.

I think this fixes the problem which joel faced with armel static binary
on ppc64le.

Helge

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

[Daniel Henrique Barboza]

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

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



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

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

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

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

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

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

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

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

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

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

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

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


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

   uint32_t reg = cpu->cfg.mvendorid;

and then...


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


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


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

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


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


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


KVM_REG_RISCV_FP_D_REG(fcsr) does not work ATM because we're missing the 
ptrace.h
linux header (same problem as RVV I mentioned in private).

But otherwise, yeah, we need an audit for the other cases to see if we're 
messing
stuff up by accident.


Daniel




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

Thanks,
drew




target_ulong seems that the right choice here. We could perhaps work with
uint64_t (other parts of the code does that) but target_ulong is nicer with
32-bit setups.


Thanks,

Daniel




+reg = cpu->cfg.mvendorid;
+ret = kvm_set_one_reg(cs, id, ®);
   

Re: [RFC PATCH 08/19] HostMem: Add private property to indicate to use kvm gmem

[David Hildenbrand]

On 03.08.23 00:53, Isaku Yamahata wrote:

On Wed, Aug 02, 2023 at 04:14:29PM +0200,
David Hildenbrand  wrote:


On 02.08.23 10:03, Xiaoyao Li wrote:

On 8/2/2023 1:21 AM, David Hildenbrand wrote:

On 31.07.23 18:21, Xiaoyao Li wrote:

From: Isaku Yamahata 

Signed-off-by: Isaku Yamahata 
Signed-off-by: Xiaoyao Li 
---
    backends/hostmem.c   | 18 ++
    include/sysemu/hostmem.h |  2 +-
    qapi/qom.json    |  4 
    3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 747e7838c031..dbdbb0aafd45 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -461,6 +461,20 @@ static void
host_memory_backend_set_reserve(Object *o, bool value, Error **errp)
    }
    backend->reserve = value;
    }
+
+static bool host_memory_backend_get_private(Object *o, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    return backend->private;
+}
+
+static void host_memory_backend_set_private(Object *o, bool value,
Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+
+    backend->private = value;
+}
    #endif /* CONFIG_LINUX */
    static bool
@@ -541,6 +555,10 @@ host_memory_backend_class_init(ObjectClass *oc,
void *data)
    host_memory_backend_get_reserve,
host_memory_backend_set_reserve);
    object_class_property_set_description(oc, "reserve",
    "Reserve swap space (or huge pages) if applicable");
+    object_class_property_add_bool(oc, "private",
+    host_memory_backend_get_private,
host_memory_backend_set_private);
+    object_class_property_set_description(oc, "private",
+    "Use KVM gmem private memory");
    #endif /* CONFIG_LINUX */
    /*
     * Do not delete/rename option. This option must be considered
stable
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 39326f1d4f9c..d88970395618 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -65,7 +65,7 @@ struct HostMemoryBackend {
    /* protected */
    uint64_t size;
    bool merge, dump, use_canonical_path;
-    bool prealloc, is_mapped, share, reserve;
+    bool prealloc, is_mapped, share, reserve, private;
    uint32_t prealloc_threads;
    ThreadContext *prealloc_context;
    DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
diff --git a/qapi/qom.json b/qapi/qom.json
index 7f92ea43e8e1..e0b2044e3d20 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -605,6 +605,9 @@
    # @reserve: if true, reserve swap space (or huge pages) if applicable
    # (default: true) (since 6.1)
    #
+# @private: if true, use KVM gmem private memory
+#   (default: false) (since 8.1)
+#


But that's not what any of this does.

This patch only adds a property and doesn't even explain what it intends
to achieve with that.

How will it be used from a user? What will it affect internally? What
will it modify in regards of the memory backend?


How it will be used is in the next patch, patch 09.

for kvm_x86_sw_protected_vm type VM, it will allocate private gmem with
KVM ioctl if the memory backend has property "private" on.


It feels wired up the wrong way.

When creating/initializing the memory backend, we should also take care of
allocating the gmem_fd, for example, by doing some gmem allocation callback,
ideally *internally* creating the RAM memory region / RAMBlock.

And we should fail if that is impossible (gmem does not apply to the VM) or
creating the gmem_fd failed for other reason.

Like passing a RAM_GMEM flag to memory_region_init_ram_flags_nomigrate() in
ram_backend_memory_alloc(), to then handle it internally, failing if there
is an error.


KVM gmem is tied to VM. not before creating VM. We have to delay of the
allocation of kvm gmem until VM initialization.


In vl.c, the flow is

1) Create machine: qemu_create_machine()
2) Configure KVM: configure_accelerators()
3) Create backends: qemu_create_late_backends()

Staring at object_create_early(), "memory-backend-" area always late.

So maybe, at the time memory backends are created+initialized, 
everything you need  is already in place.




Hmm, one options is to move gmem_fd from RAMBlock to KVMSlot.  Handle the
allocation of KVM gmem (issuing KVM gmem ioctl) there. i.e. in
kvm_set_phys_mem() or kvm_region_add() (or whatever functions of KVM memory
listener).  Maybe we can drop ram_block_convert_range() and can have KVM
specific logic instead.


Might be doable as well.



We still need a way for user to specify which guest memory region is subject
to KVM gmem, though.


Let's minimize the gmem hacks and come up with something clean.

--
Cheers,

David / dhildenb

[PATCH trivial for-8.1] stubs/colo.c: spelling

[Michael Tokarev]

Signed-off-by: Michael Tokarev 
---
 stubs/colo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stubs/colo.c b/stubs/colo.c
index f33379d0fd..08c9f982d5 100644
--- a/stubs/colo.c
+++ b/stubs/colo.c
@@ -24 +24 @@ void migrate_start_colo_process(MigrationState *s)
-error_report("Impossible happend: trying to start COLO when COLO "
+error_report("Impossible happened: trying to start COLO when COLO "
-- 
2.39.2

Re: [PATCH] target/arm: Do not use gen_mte_checkN in trans_STGP

[Peter Maydell]

On Thu, 27 Jul 2023 at 17:33, Richard Henderson
 wrote:
>
> STGP writes to tag memory, it does not check it.
> This happened to work because we wrote tag memory first
> so that the check always succeeded.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/tcg/translate-a64.c | 41 +-
>  1 file changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
> index 5fa1257d32..dfd18e19ca 100644
> --- a/target/arm/tcg/translate-a64.c
> +++ b/target/arm/tcg/translate-a64.c
> @@ -3020,37 +3020,17 @@ static bool trans_STGP(DisasContext *s, arg_ldstpair 
> *a)
>  tcg_gen_addi_i64(dirty_addr, dirty_addr, offset);
>  }
>
> -if (!s->ata) {
> -/*
> - * TODO: We could rely on the stores below, at least for
> - * system mode, if we arrange to add MO_ALIGN_16.
> - */
> -gen_helper_stg_stub(cpu_env, dirty_addr);
> -} else if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> -gen_helper_stg_parallel(cpu_env, dirty_addr, dirty_addr);
> -} else {
> -gen_helper_stg(cpu_env, dirty_addr, dirty_addr);
> -}
> -
> -mop = finalize_memop(s, MO_64);
> -clean_addr = gen_mte_checkN(s, dirty_addr, true, false, 2 << MO_64, mop);
> -
> +clean_addr = clean_data_tbi(s, dirty_addr);
>  tcg_rt = cpu_reg(s, a->rt);
>  tcg_rt2 = cpu_reg(s, a->rt2);
>
>  /*
> - * STGP is defined as two 8-byte memory operations and one tag operation.
> - * We implement it as one single 16-byte memory operation for 
> convenience.
> - * Rebuild mop as for STP.
> - * TODO: The atomicity with LSE2 is stronger than required.
> - * Need a form of MO_ATOM_WITHIN16_PAIR that never requires
> - * 16-byte atomicity.
> + * STGP is defined as two 8-byte memory operations, aligned to 
> TAG_GRANULE,
> + * and one tag operation.  We implement it as one single aligned 16-byte
> + * memory operation for convenience.  Note that the alignment ensures
> + * MO_ATOM_IFALIGN_PAIR produces 8-byte atomicity for the memory store.
>   */
> -mop = MO_128;
> -if (s->align_mem) {
> -mop |= MO_ALIGN_8;
> -}
> -mop = finalize_memop_pair(s, mop);
> +mop = MO_128 | MO_ALIGN | MO_ATOM_IFALIGN_PAIR;

So here we're implicitly assuming TAG_GRANULE is 16 and
then relying on the codegen for a MO_128 | MO_ALIGN
operation to give us the alignment fault if the guest
address isn't aligned to the tag granule, right ?

Previously we also put s->be_data into the MemOp
(via finalize_memop_pair() calling finalize_memop_atom()).
Don't we still need to do that ? (We do explicitly swap
the two i64s into the i128 in different orders depending
on the be_data setting, but I think that is to handle
MO_128 | MO_BE giving a true BE 128 bit store, whereas
what we're implementing is two BE 64 bit stores.)


>  tmp = tcg_temp_new_i128();
>  if (s->be_data == MO_LE) {
> @@ -3060,6 +3040,15 @@ static bool trans_STGP(DisasContext *s, arg_ldstpair 
> *a)
>  }
>  tcg_gen_qemu_st_i128(tmp, clean_addr, get_mem_index(s), mop);

thanks
-- PMM

Re: [PATCH v7 00/14] linux-user: brk fixes

[Joel Stanley]

Hi Richard,

On Thu, 3 Aug 2023 at 01:53, Richard Henderson
 wrote:
>
> Builds on Helge's v6, incorporating my feedback plus
> some other minor cleanup.

This succeeds for the armhf static binary on ppc64le host that was
previously segfaulting.

However, the arm static binary on ppc64le host now segfaults:

$ gdb -q -ex r --args ./build/qemu-arm -d guest_errors,page,strace ~/hello
Reading symbols from ./build/qemu-arm...
Starting program: /scratch/joel/qemu/build/qemu-arm -d
guest_errors,page,strace /home/joel/hello
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/powerpc64le-linux-gnu/libthread_db.so.1".
[New Thread 0x7762ece0 (LWP 143553)]
host mmap_min_addr=0x1
pgb_find_hole: base @ 14042 for 4294967296 bytes
pgb_static: base @ 14042 for 4294967295 bytes
pgb_reserved_va: base @ 0x14042 for 4294967296 bytes
Locating guest address space @ 0x14042
page layout changed following mmap
startend  size prot
0001-0009 0008 ---
0009-0009b000 b000 ---
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-0009b000 b000 ---
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4081 0081 rw-
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4001 0001 ---
4001-40811000 00801000 rw-
- 0001 r-x
guest_base  0x14042
page layout changed following binary load
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4001 0001 ---
4001-4081 0080 rw-
4081-40811000 1000 r-x
- 0001 r-x
end_code0x00084f7c
start_code  0x0001
start_data  0x00095098
end_data0x00098394
start_stack 0x4080f410
brk 0x0009b000
entry   0x00010418
argv_start  0x4080f414
env_start   0x4080f41c
auxv_start  0x4080f4a0
143551 brk(NULL) = 0x0009b000
143551 brk(0x0009b8fc) = 0x0009b000

Thread 1 "qemu-arm" received signal SIGSEGV, Segmentation fault.
0x7fffeed9bb74 in code_gen_buffer ()
(gdb) bt
#0  0x7fffeed9bb74 in code_gen_buffer ()
#1  0x000100169fdc in cpu_tb_exec (cpu=cpu@entry=0x1003d4a90,
itb=itb@entry=0x7fffeed9ba60 ,
tb_exit=tb_exit@entry=0x7fffe51c)
at ../accel/tcg/cpu-exec.c:457
#2  0x00010016a704 in cpu_loop_exec_tb (tb_exit=0x7fffe51c,
last_tb=,
pc=, tb=0x7fffeed9ba60 ,
cpu=)
at ../accel/tcg/cpu-exec.c:919
#3  cpu_exec_loop (cpu=cpu@entry=0x1003d4a90, sc=) at
../accel/tcg/cpu-exec.c:1040
#4  0x00010016abac in cpu_exec_setjmp (cpu=cpu@entry=0x1003d4a90,
sc=)
at ../accel/tcg/cpu-exec.c:1057
#5  0x00010016b270 in cpu_exec (cpu=0x1003d4a90) at
../accel/tcg/cpu-exec.c:1083
#6  0x00010004d7b0 in cpu_loop (env=0x1003d4fa0) at
../linux-user/arm/cpu_loop.c:328
#7  0x000100047548 in main (argc=,
argv=0x7188, envp=)
at ../linux-user/main.c:1012
(gdb)


>
>
> r~
>
>
> Akihiko Odaki (6):
>   linux-user: Unset MAP_FIXED_NOREPLACE for host
>   linux-user: Fix MAP_FIXED_NOREPLACE on old kernels
>   linux-user: Do not call get_errno() in do_brk()
>   linux-user: Use MAP_FIXED_NOREPLACE for do_brk()
>   linux-user: Do nothing if too small brk is specified
>   linux-user: Do not align brk with host page size
>
> Helge Deller (1):
>   linux-user: Adjust initial brk when interpreter is close to executable
>
> Richard Henderson (7):
>   linux-user: Remove last_brk
>   bsd-user: Remove last_brk
>   linux-user: Adjust task_unmapped_base for reserved_va
>   linux-user: Define TASK_UNMAPPED_BASE in $guest/target_mman.h
>   linux-user: Add ELF_ET_DYN_BASE
>   linux-user: Use elf_et_dyn_base for ET_DYN with interpreter
>   linux-user: Properly set image_info.brk in flatload
>
>  bsd-user/qemu.h  |  1 -
>  linux-user/aarch64/target_mman.h | 13 
>  linux-user/alpha/target_mman.h   | 11 
>  linux-user/arm/target_mman.h | 11 
>  linux-user/cris/target_mman.h| 12 
>  linux-user/hexagon/target_mman.h | 13 
>  linux-user/hppa/target_mman.h|  6 ++
>  linux-user/i386/target_mman.h| 16 +
>  linux-user/loongarch64/target_mman.h | 11 
>  linux-user/m68k/target_mman.h|  5 ++
>  linux-user/microblaze/target_mman.h  | 11 
>  linux-user/mips/target_mman.h| 10 +++
>  linux-user/nios2/target_mman.h   | 10 +++
>  linux-user/openrisc/target_mman.h| 10 +++
>  linux-user/ppc/target_mman.h | 20 ++
>  linux-user/qemu.h|  2 -
>  linux-user/riscv/target_mman.h  

[PATCH] linux-user/elfload: Set V in ELF_HWCAP for RISC-V

[Nathan Egge]

From: "Nathan Egge" 

Set V bit for hwcap if misa is set.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1793
Signed-off-by: Nathan Egge 
---
 linux-user/elfload.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 861ec07abc..a299ba7300 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1710,7 +1710,8 @@ static uint32_t get_elf_hwcap(void)
 #define MISA_BIT(EXT) (1 << (EXT - 'A'))
 RISCVCPU *cpu = RISCV_CPU(thread_cpu);
 uint32_t mask = MISA_BIT('I') | MISA_BIT('M') | MISA_BIT('A')
-| MISA_BIT('F') | MISA_BIT('D') | MISA_BIT('C');
+| MISA_BIT('F') | MISA_BIT('D') | MISA_BIT('C')
+| MISA_BIT('V');
 
 return cpu->env.misa_ext & mask;
 #undef MISA_BIT
-- 
2.35.1

Re: [PATCH v3 0/6] Add nRF51 DETECT signal with test

[Peter Maydell]

On Fri, 28 Jul 2023 at 17:04, Chris Laplante  wrote:
>
> This patch series implements the nRF51 DETECT signal
> in the GPIO peripheral. A qtest is added exercising the signal.
>
> To implement the test, named out-GPIO IRQ interception had to be added
> to the qtest framework. I also took the opportunity to improve IRQ
> interception a bit by adding 'FAIL' responses when interception fails.
> Otherwise, it is frustrating to troubleshoot why calls to
> qtest_irq_intercept_out and friends appears to do nothing.
>
> v2: https://patchwork.kernel.org/project/qemu-devel/list/?series=769532

I've applied this to my target-arm-for-8.2 branch; as
the name suggests this will be to go in after we release
8.1 and development reopens for 8.2.

Thanks for the patches!

-- PMM

Re: [PATCH v1 2/8] hw/misc: Introduce a model of Xilinx Versal's CFU_APB

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Introduce a model of the software programming interface (CFU_APB) of
> Xilinx Versal's Configuration Frame Unit.
>
> Signed-off-by: Francisco Iglesias 
> ---

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v3 1/5] vhost: fix the fd leak

[Fiona Ebner]

Am 31.07.23 um 14:10 schrieb Li Feng:
> When the vhost-user reconnect to the backend, the notifer should be
> cleanup. Otherwise, the fd resource will be exhausted.
> 
> Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> 
> Signed-off-by: Li Feng 
> Reviewed-by: Raphael Norwitz 

Tested-by: Fiona Ebner 

Thank you for the fix! CC-ing stable, because the issue is present since
8.0.0.

Re: [PATCH v1 3/8] hw/misc/xlnx-versal-cfu: Introduce a model of Xilinx Versal CFU_FDRO

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame Unit's data out
> port (CFU_FDRO).
>
> Signed-off-by: Francisco Iglesias 
> ---
>  hw/misc/xlnx-versal-cfu.c | 105 ++
>  include/hw/misc/xlnx-versal-cfu.h |  11 
>  2 files changed, 116 insertions(+)
>
> diff --git a/hw/misc/xlnx-versal-cfu.c b/hw/misc/xlnx-versal-cfu.c
> index cbd17d2351..528090ef1b 100644
> --- a/hw/misc/xlnx-versal-cfu.c
> +++ b/hw/misc/xlnx-versal-cfu.c
> @@ -257,6 +257,26 @@ static void cfu_stream_write(void *opaque, hwaddr addr, 
> uint64_t value,
>  }
>  }
>
> +static uint64_t cfu_fdro_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> +uint64_t ret = 0;
> +
> +if (s->fdro_data->len) {
> +ret = g_array_index(s->fdro_data, uint32_t, 0);
> +g_array_remove_index(s->fdro_data, 0);

This is pretty expensive because everything in the GArray
after element 0 must be copied downwards. Are you sure you
don't want a different data structure ?

What actually is this, and what are the operations we want
to do on it ?

> +}
> +
> +return ret;
> +}
> +
> +static void cfu_fdro_write(void *opaque, hwaddr addr, uint64_t value,
> +   unsigned size)
> +{
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: Unsupported write from addr=%"
> +  HWADDR_PRIx "\n", __func__, addr);
> +}
> +
>  static const MemoryRegionOps cfu_stream_ops = {
>  .read = cfu_stream_read,
>  .write = cfu_stream_write,
> @@ -267,6 +287,16 @@ static const MemoryRegionOps cfu_stream_ops = {
>  },
>  };
>
> +static const MemoryRegionOps cfu_fdro_ops = {
> +.read = cfu_fdro_read,
> +.write = cfu_fdro_write,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +.valid = {
> +.min_access_size = 4,
> +.max_access_size = 4,
> +},
> +};
> +
>  static void cfu_apb_init(Object *obj)
>  {
>  XlnxVersalCFUAPB *s = XLNX_VERSAL_CFU_APB(obj);
> @@ -298,6 +328,24 @@ static void cfu_apb_init(Object *obj)
>  sysbus_init_irq(sbd, &s->irq_cfu_imr);
>  }
>
> +static void cfu_fdro_init(Object *obj)
> +{
> +XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(obj);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +memory_region_init_io(&s->iomem_fdro, obj, &cfu_fdro_ops, s,
> +  TYPE_XLNX_VERSAL_CFU_FDRO, KEYHOLE_STREAM_4K);
> +sysbus_init_mmio(sbd, &s->iomem_fdro);
> +s->fdro_data = g_array_new(FALSE, FALSE, sizeof(uint32_t));
> +}
> +
> +static void cfu_fdro_cfi_transfer_packet(XlnxCfiIf *cfi_if, XlnxCfiPacket 
> *pkt)
> +{
> +XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(cfi_if);
> +
> +g_array_append_vals(s->fdro_data, &pkt->data[0], 4);
> +}
> +
>  static Property cfu_props[] = {
>  DEFINE_PROP_LINK("cframe0", XlnxVersalCFUAPB, cfg.cframe[0],
>   TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> @@ -344,6 +392,41 @@ static const VMStateDescription vmstate_cfu_apb = {
>  }
>  };
>
> +static int cfdro_reg_pre_save(void *opaque)
> +{
> +XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> +
> +if (s->fdro_data->len) {
> +s->ro_data = (uint32_t *) s->fdro_data->data;
> +s->ro_dlen = s->fdro_data->len;
> +}

I think we need to initialise ro_data and ro_dlen in
the else case here as well. Otherwise they might have old
stale stuff in them that then goes into the migration stream.

> +
> +return 0;
> +}
> +
> +static int cfdro_reg_post_load(void *opaque, int version_id)
> +{
> +XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(opaque);
> +
> +if (s->ro_dlen) {
> +g_array_append_vals(s->fdro_data, s->ro_data, s->ro_dlen);
> +}
> +return 0;
> +}
> +
> +static const VMStateDescription vmstate_cfu_fdro = {
> +.name = TYPE_XLNX_VERSAL_CFU_FDRO,
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.pre_save = cfdro_reg_pre_save,
> +.post_load = cfdro_reg_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_VARRAY_UINT32_ALLOC(ro_data, XlnxVersalCFUFDRO, ro_dlen,
> +0, vmstate_info_uint32, uint32_t),

This kind of _ALLOC vmstate will cause the migration core
code to g_malloc() you a buffer for the data. We don't
free that anywhere (and if we have a subsequent vmsave
then we will overwrite the ro-data pointer, and leak the
memory).

It might be better to avoid the GArray and just directly
work with a g_malloc()'d buffer of our own, to fit better
with how the _ALLOC vmstate wants to work.

> +VMSTATE_END_OF_LIST(),
> +}
> +};
> +
>  static void cfu_apb_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -353,6 +436,15 @@ static void cfu_apb_class_init(ObjectClass *klass, void 
> *data)
>  device_class_set_props(dc, cfu_props);
>  }
>
> +static void cfu_fdro_class_init(ObjectClass *klass, void *data)
>

Re: [PATCH v1 4/8] hw/misc/xlnx-versal-cfu: Introduce a model of Xilinx Versal's CFU_SFR

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame Unit's Single
> Frame Read port (CFU_SFR).
>
> Signed-off-by: Francisco Iglesias 
> ---
>  hw/misc/xlnx-versal-cfu.c | 88 +++
>  include/hw/misc/xlnx-versal-cfu.h | 15 ++
>  2 files changed, 103 insertions(+)

> +static void cfu_sfr_init(Object *obj)
> +{
> +XlnxVersalCFUSFR *s = XLNX_VERSAL_CFU_SFR(obj);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +memory_region_init_io(&s->iomem_sfr, obj, &cfu_sfr_ops, s,
> +  TYPE_XLNX_VERSAL_CFU_SFR, KEYHOLE_STREAM_4K);
> +sysbus_init_mmio(sbd, &s->iomem_sfr);
> +}
> +
>  static void cfu_fdro_init(Object *obj)
>  {
>  XlnxVersalCFUFDRO *s = XLNX_VERSAL_CFU_FDRO(obj);
> @@ -380,6 +435,12 @@ static Property cfu_props[] = {
>  DEFINE_PROP_END_OF_LIST(),
>  };
>
> +static Property cfu_sfr_props[] = {
> +DEFINE_PROP_LINK("cfu", XlnxVersalCFUSFR, cfg.cfu,
> + TYPE_XLNX_VERSAL_CFU_APB, XlnxVersalCFUAPB *),
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static const VMStateDescription vmstate_cfu_apb = {
>  .name = TYPE_XLNX_VERSAL_CFU_APB,
>  .version_id = 1,
> @@ -427,6 +488,16 @@ static const VMStateDescription vmstate_cfu_fdro = {
>  }
>  };
>
> +static const VMStateDescription vmstate_cfu_sfr = {
> +.name = TYPE_XLNX_VERSAL_CFU_SFR,
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32_ARRAY(wfifo, XlnxVersalCFUSFR, 4),
> +VMSTATE_END_OF_LIST(),
> +}
> +};
> +
>  static void cfu_apb_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -445,6 +516,14 @@ static void cfu_fdro_class_init(ObjectClass *klass, void 
> *data)
>  xcic->cfi_transfer_packet = cfu_fdro_cfi_transfer_packet;
>  }
>
> +static void cfu_sfr_class_init(ObjectClass *klass, void *data)
> +{
> +DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +device_class_set_props(dc, cfu_sfr_props);
> +dc->vmsd = &vmstate_cfu_sfr;

Missing reset again ?

> +}

thanks
-- PMM

Re: [PATCH v1 7/8] hw/arm/xlnx-versal: Connect the CFU_APB, CFU_FDRO and CFU_SFR

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Connect the Configuration Frame Unit (CFU_APB, CFU_FDRO and CFU_SFR) to
> the Versal machine.
>
> Signed-off-by: Francisco Iglesias 
> ---
>  hw/arm/xlnx-versal.c | 42 
>  include/hw/arm/xlnx-versal.h | 16 ++
>  2 files changed, 58 insertions(+)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v2 1/2] block/blkio: close the fd when blkio_connect() fails

[Hanna Czenczek]

On 03.08.23 10:28, Stefano Garzarella wrote:

libblkio drivers take ownership of `fd` only after a successful
blkio_connect(), so if it fails, we are still the owners.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for 
virtio-blk")
Suggested-by: Hanna Czenczek 
Signed-off-by: Stefano Garzarella 
---

Notes:
 v2:
 - avoid to use `fd_supported` to track a valid fd [Hanna]

  block/blkio.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)


Thanks!

Reviewed-by: Hanna Czenczek 

Re: [PATCH v1 8/8] hw/arm/versal: Connect the CFRAME_REG and CFRAME_BCAST_REG

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Connect the Configuration Frame controller (CFRAME_REG) and the
> Configuration Frame broadcast controller (CFRAME_BCAST_REG) to the
> Versal machine.
>
> Signed-off-by: Francisco Iglesias 

> +/* CFRAME REG */
> +for (i = 0; i < ARRAY_SIZE(s->pmc.cframe); i++) {
> +char *name = g_strdup_printf("cframe%d", i);
> +
> +object_initialize_child(OBJECT(s), name, &s->pmc.cframe[i],
> +TYPE_XLNX_VERSAL_CFRAME_REG);
[...]

> +g_free(name);

g_autofree is nicer than this kind of explicit free, I think
(here and below)
> +}

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH v7 00/14] linux-user: brk fixes

[Helge Deller]

On 8/3/23 15:11, Joel Stanley wrote:

Hi Richard,

On Thu, 3 Aug 2023 at 01:53, Richard Henderson
 wrote:


Builds on Helge's v6, incorporating my feedback plus
some other minor cleanup.


This succeeds for the armhf static binary on ppc64le host that was
previously segfaulting.

However, the arm static binary on ppc64le host now segfaults:

$ gdb -q -ex r --args ./build/qemu-arm -d guest_errors,page,strace ~/hello
Reading symbols from ./build/qemu-arm...
Starting program: /scratch/joel/qemu/build/qemu-arm -d
guest_errors,page,strace /home/joel/hello
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/powerpc64le-linux-gnu/libthread_db.so.1".
[New Thread 0x7762ece0 (LWP 143553)]
host mmap_min_addr=0x1
pgb_find_hole: base @ 14042 for 4294967296 bytes
pgb_static: base @ 14042 for 4294967295 bytes
pgb_reserved_va: base @ 0x14042 for 4294967296 bytes
Locating guest address space @ 0x14042
page layout changed following mmap
startend  size prot
0001-0009 0008 ---
0009-0009b000 b000 ---
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-0009b000 b000 ---
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4081 0081 rw-
- 0001 r-x
page layout changed following mmap
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4001 0001 ---
4001-40811000 00801000 rw-
- 0001 r-x
guest_base  0x14042
page layout changed following binary load
startend  size prot
0001-0009 0008 r-x
0009-000a 0001 rw-
4000-4001 0001 ---
4001-4081 0080 rw-
4081-40811000 1000 r-x
- 0001 r-x
end_code0x00084f7c
start_code  0x0001
start_data  0x00095098
end_data0x00098394
start_stack 0x4080f410
brk 0x0009b000
entry   0x00010418
argv_start  0x4080f414
env_start   0x4080f41c
auxv_start  0x4080f4a0
143551 brk(NULL) = 0x0009b000
143551 brk(0x0009b8fc) = 0x0009b000


I think the problem is the brk with 9b000 here.
It's not 64k aligned (=pages size of your ppc64le).

Please try with this patch on top of Richard's series:


@@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, int 
image_fd,
  info->end_code = 0;
  info->start_data = -1;
  info->end_data = 0;
-info->brk = .

change that to become:
info->brk = HOST_PAGE_ALIGN(hiaddr);

Helge

Re: [PATCH v1 5/8] hw/misc: Introduce a model of Xilinx Versal's CFRAME_REG

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame controller
> (CFRAME_REG).
>
> Signed-off-by: Francisco Iglesias 
> ---
>  MAINTAINERS  |   2 +
>  hw/misc/meson.build  |   1 +
>  hw/misc/xlnx-versal-cframe-reg.c | 714 +++
>  include/hw/misc/xlnx-versal-cframe-reg.h | 288 +
>  4 files changed, 1005 insertions(+)
>  create mode 100644 hw/misc/xlnx-versal-cframe-reg.c
>  create mode 100644 include/hw/misc/xlnx-versal-cframe-reg.h
>



> +#define KEYHOLE_STREAM_4K 0x1000

You could define this as (4 * KiB) which would then be more
clearly 4K.

> +
> +#define MAX_BLOCKTYPE 6
> +#define MAX_BLOCKTYPE_FRAMES 0xF
> +
> +enum {
> +CFRAME_CMD_WCFG = 1,
> +CFRAME_CMD_ROWON = 2,
> +CFRAME_CMD_ROWOFF = 3,
> +CFRAME_CMD_RCFG = 4,
> +CFRAME_CMD_DLPARK = 5

minor style nit -- leaving the trailing ',' on the last
item in an enum or similar means that the next person
to add another entry doesn't have to edit the previous
line to put the comma in.

> +};

> +static void cfrm_fdri_post_write(RegisterInfo *reg, uint64_t val)
> +{
> +XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(reg->opaque);
> +
> +if (s->row_configured && s->rowon && s->wcfg) {
> +XlnxCFrame *new_f = &s->new_f;
> +
> +new_f->data[new_f->idx++] = s->regs[R_FDRI0];
> +new_f->data[new_f->idx++] = s->regs[R_FDRI1];
> +new_f->data[new_f->idx++] = s->regs[R_FDRI2];
> +new_f->data[new_f->idx++] = s->regs[R_FDRI3];
> +
> +assert(new_f->idx <= FRAME_NUM_WORDS);

We should assert that we're not overrunning the array
*before* we write the data, not afterwards.

More generally, this kind of "we have a data array and
an index into it and we store stuff in at the index"
is prime territory for bugs resulting in array overruns.
If there's a way to write it that makes it clear that
that can't happen (esp. abstracting out what the operations
on the data structure are, documenting the invariants, etc)
then we should take it.

> +
> +if (new_f->idx == FRAME_NUM_WORDS) {
> +XlnxCFrame *cur_f;
> +
> +/* Include block type and frame address */
> +new_f->addr = extract32(s->regs[R_FAR0], 0, 23);
> +
> +cur_f = cframes_get_frame(s, new_f->addr);
> +
> +if (cur_f) {
> +/* Overwrite current frame */
> +cur_f[0] = new_f[0];
> +} else {
> +g_array_append_val(s->cframes, new_f[0]);
> +}
> +
> +cframe_incr_far(s);
> +
> +/* Clear out new_f */
> +memset(new_f, 0, sizeof(*new_f));
> +}
> +}
> +}

> +static int cframes_reg_pre_save(void *opaque)
> +{
> +XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(opaque);
> +
> +if (s->cframes->len) {
> +s->cf_data = (uint32_t *) s->cframes->data;
> +s->cf_dlen = s->cframes->len * g_array_get_element_size(s->cframes) 
> / 4;
> +}
> +return 0;
> +}
> +
> +static int cframes_reg_post_load(void *opaque, int version_id)
> +{
> +XlnxVersalCFrameReg *s = XLNX_VERSAL_CFRAME_REG(opaque);
> +
> +if (s->cf_dlen) {
> +uint32_t num_frames = s->cf_dlen /
> +  (g_array_get_element_size(s->cframes) / 4);
> +g_array_append_vals(s->cframes, s->cf_data, num_frames);
> +}
> +return 0;
> +}

Same remarks here about handling of GArray vs VMSTATE ALLOC.

(Or we could implement a VMSTATE macro/support for GArrays,
I guess. That would arguably be neater.)

> +
> +static const VMStateDescription vmstate_cframe_reg = {
> +.name = TYPE_XLNX_VERSAL_CFRAME_REG,
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.pre_save = cframes_reg_pre_save,
> +.post_load = cframes_reg_post_load,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT32_ARRAY(wfifo, XlnxVersalCFrameReg, 4),
> +VMSTATE_UINT32_ARRAY(regs, XlnxVersalCFrameReg, CFRAME_REG_R_MAX),
> +VMSTATE_BOOL(rowon, XlnxVersalCFrameReg),
> +VMSTATE_BOOL(wcfg, XlnxVersalCFrameReg),
> +VMSTATE_BOOL(rcfg, XlnxVersalCFrameReg),
> +VMSTATE_VARRAY_UINT32_ALLOC(cf_data, XlnxVersalCFrameReg, cf_dlen,
> +0, vmstate_info_uint32, uint32_t),
> +VMSTATE_END_OF_LIST(),
> +}
> +};
> +
> +static Property cframe_regs_props[] = {
> +/* Kept for backwards compatibility */

Backwards compatibility with who? This is the first implementation
of this as far as upstream is concerned...

> +DEFINE_PROP_LINK("cfu", XlnxVersalCFrameReg, cfg.cfu_fdro,
> + TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> +DEFINE_PROP_LINK("cfu-fdro", XlnxVersalCFrameReg, cfg.cfu_fdro,
> + TYPE_XLNX_CFI_IF, XlnxCfiIf *),
> +DEFINE_PROP_UINT32("blktype0-frames", XlnxVersalCFrameReg,
> +   cfg.blktype_nu

Re: [RFC PATCH for-8.2 00/18] crypto: Provide clmul.h and host accel

[Ard Biesheuvel]

On Thu, 13 Jul 2023 at 23:14, Richard Henderson
 wrote:
>
> Inspired by Ard Biesheuvel's RFC patches [1] for accelerating
> carry-less multiply under emulation.
>
> This is less polished than the AES patch set:
>
> (1) Should I split HAVE_CLMUL_ACCEL into per-width HAVE_CLMUL{N}_ACCEL?
> The "_generic" and "_accel" split is different from aes-round.h
> because of the difference in support for different widths, and it
> means that each host accel has more boilerplate.
>
> (2) Should I bother trying to accelerate anything other than 64x64->128?

That is the only compelling use case afaict.

> That seems to be the one that GSM really wants anyway.  I'd keep all
> of the sizes implemented generically, since that centralizes the 3
> target implementations.
>
> (3) The use of Int128 isn't fantastic -- better would be a vector type,
> though that has its own special problems for ppc64le (see the
> endianness hoops within aes-round.h).  Perhaps leave things in
> env memory, like I was mostly able to do with AES?
>
> (4) No guest test case(s).
>
>
> r~
>
>
> [1] https://patchew.org/QEMU/20230601123332.3297404-1-a...@kernel.org/
>
> Richard Henderson (18):
>   crypto: Add generic 8-bit carry-less multiply routines
>   target/arm: Use clmul_8* routines
>   target/s390x: Use clmul_8* routines
>   target/ppc: Use clmul_8* routines
>   crypto: Add generic 16-bit carry-less multiply routines
>   target/arm: Use clmul_16* routines
>   target/s390x: Use clmul_16* routines
>   target/ppc: Use clmul_16* routines
>   crypto: Add generic 32-bit carry-less multiply routines
>   target/arm: Use clmul_32* routines
>   target/s390x: Use clmul_32* routines
>   target/ppc: Use clmul_32* routines
>   crypto: Add generic 64-bit carry-less multiply routine
>   target/arm: Use clmul_64
>   target/s390x: Use clmul_64
>   target/ppc: Use clmul_64
>   host/include/i386: Implement clmul.h
>   host/include/aarch64: Implement clmul.h
>
>  host/include/aarch64/host/cpuinfo.h  |   1 +
>  host/include/aarch64/host/crypto/clmul.h | 230 +++
>  host/include/generic/host/crypto/clmul.h |  28 +++
>  host/include/i386/host/cpuinfo.h |   1 +
>  host/include/i386/host/crypto/clmul.h| 187 ++
>  host/include/x86_64/host/crypto/clmul.h  |   1 +
>  include/crypto/clmul.h   | 123 
>  target/arm/tcg/vec_internal.h|  11 --
>  crypto/clmul.c   | 163 
>  target/arm/tcg/mve_helper.c  |  16 +-
>  target/arm/tcg/vec_helper.c  | 112 ++-
>  target/ppc/int_helper.c  |  63 +++
>  target/s390x/tcg/vec_int_helper.c| 175 +++--
>  util/cpuinfo-aarch64.c   |   4 +-
>  util/cpuinfo-i386.c  |   1 +
>  crypto/meson.build   |   9 +-
>  16 files changed, 865 insertions(+), 260 deletions(-)
>  create mode 100644 host/include/aarch64/host/crypto/clmul.h
>  create mode 100644 host/include/generic/host/crypto/clmul.h
>  create mode 100644 host/include/i386/host/crypto/clmul.h
>  create mode 100644 host/include/x86_64/host/crypto/clmul.h
>  create mode 100644 include/crypto/clmul.h
>  create mode 100644 crypto/clmul.c
>
> --
> 2.34.1
>

Re: [PATCH v1 6/8] hw/misc: Introduce a model of Xilinx Versal's CFRAME_BCAST_REG

[Peter Maydell]

On Mon, 10 Jul 2023 at 15:03, Francisco Iglesias
 wrote:
>
> Introduce a model of Xilinx Versal's Configuration Frame broadcast
> controller (CFRAME_BCAST_REG).
>
> Signed-off-by: Francisco Iglesias 
> ---
>  hw/misc/xlnx-versal-cframe-reg.c | 173 +++
>  include/hw/misc/xlnx-versal-cframe-reg.h |  17 +++
>  2 files changed, 190 insertions(+)

Missing reset again.

> +static uint64_t cframes_bcast_reg_read(void *opaque, hwaddr addr, unsigned 
> size)
> +{
> +qemu_log_mask(LOG_GUEST_ERROR, "%s: Unsupported read from addr=%"
> +  HWADDR_PRIx "\n", __func__, addr);
> +return 0;
> +}
> +
> +static void cframes_bcast_reg_write(void *opaque, hwaddr addr, uint64_t 
> value,
> +  unsigned size)
> +{
> +XlnxVersalCFrameBcastReg *s = XLNX_VERSAL_CFRAME_BCAST_REG(opaque);
> +unsigned int idx;
> +
> +/* 4 32bit words. */
> +idx = (addr >> 2) & 3;
> +
> +s->wfifo[idx] = value;
> +
> +/* Writing to the top word triggers the transmit onto CFI. */
> +if (idx == 3) {
> +uint32_t reg_addr = extract32(addr, 4, 6);
> +XlnxCfiPacket pkt = {
> +.reg_addr = reg_addr,
> +.data[0] = s->wfifo[0],
> +.data[1] = s->wfifo[1],
> +.data[2] = s->wfifo[2],
> +.data[3] = s->wfifo[3]
> +};
> +
> +for (int i = 0; i < ARRAY_SIZE(s->cfg.cframe); i++) {
> +if (s->cfg.cframe[i]) {
> +xlnx_cfi_transfer_packet(s->cfg.cframe[i], &pkt);
> +}
> +}
> +
> +memset(s->wfifo, 0, 4 * sizeof(uint32_t));
> +}
> +}

> +static void cframes_bcast_fdri_write(void *opaque, hwaddr addr, uint64_t 
> value,
> +  unsigned size)
> +{
> +XlnxVersalCFrameBcastReg *s = XLNX_VERSAL_CFRAME_BCAST_REG(opaque);
> +unsigned int idx;
> +
> +/* 4 32bit words. */
> +idx = (addr >> 2) & 3;
> +
> +s->wfifo[idx] = value;
> +
> +/* Writing to the top word triggers the transmit onto CFI. */
> +if (idx == 3) {
> +XlnxCfiPacket pkt = {
> +.reg_addr = CFRAME_FDRI,
> +.data[0] = s->wfifo[0],
> +.data[1] = s->wfifo[1],
> +.data[2] = s->wfifo[2],
> +.data[3] = s->wfifo[3]
> +};
> +
> +for (int i = 0; i < ARRAY_SIZE(s->cfg.cframe); i++) {
> +if (s->cfg.cframe[i]) {
> +xlnx_cfi_transfer_packet(s->cfg.cframe[i], &pkt);
> +}
> +}
> +
> +memset(s->wfifo, 0, 4 * sizeof(uint32_t));
> +}
> +}

I feel like I've seen this code structure in several patches:
opportunity to share code ?

thanks
-- PMM

Re: [PATCH v5 4/5] qmp: Added new command to retrieve eBPF blob.

[Daniel P . Berrangé]

On Wed, Aug 02, 2023 at 11:41:22PM +0300, Andrew Melnychenko wrote:
> Now, the binary objects may be retrieved by id.
> It would require for future qmp commands that may require specific
> eBPF blob.
> 
> Added command "request-ebpf". This command returns
> eBPF program encoded base64. The program taken from the
> skeleton and essentially is an ELF object that can be
> loaded in the future with libbpf.
> 
> The reason to use the command to provide the eBPF object
> instead of a separate artifact was to avoid issues related
> to finding the eBPF itself. eBPF object is an ELF binary
> that contains the eBPF program and eBPF map description(BTF).
> Overall, eBPF object should contain the program and enough
> metadata to create/load eBPF with libbpf. As the eBPF
> maps/program should correspond to QEMU, the eBPF can't
> be used from different QEMU build.
> 
> The first solution was a helper that comes with QEMU
> and loads appropriate eBPF objects. And the issue is
> to find a proper helper if the system has several
> different QEMUs installed and/or built from the source,
> which helpers may not be compatible.
> 
> Another issue is QEMU updating while there is a running
> QEMU instance. With an updated helper, it may not be
> possible to hotplug virtio-net device to the already
> running QEMU. Overall, requesting the eBPF object from
> QEMU itself solves possible failures with acceptable effort.
> 
> Links:
> [PATCH 3/5] qmp: Added the helper stamp check.
> https://lore.kernel.org/all/20230219162100.174318-4-and...@daynix.com/
> 
> Signed-off-by: Andrew Melnychenko 
> ---
>  ebpf/ebpf.c   | 70 +++
>  ebpf/ebpf.h   | 31 +++
>  ebpf/ebpf_rss.c   |  6 
>  ebpf/meson.build  |  2 +-
>  qapi/ebpf.json| 57 +++
>  qapi/meson.build  |  1 +
>  qapi/qapi-schema.json |  1 +
>  7 files changed, 167 insertions(+), 1 deletion(-)
>  create mode 100644 ebpf/ebpf.c
>  create mode 100644 ebpf/ebpf.h
>  create mode 100644 qapi/ebpf.json

> diff --git a/qapi/ebpf.json b/qapi/ebpf.json
> new file mode 100644
> index 000..40851f8c177
> --- /dev/null
> +++ b/qapi/ebpf.json
> @@ -0,0 +1,57 @@
> +# -*- Mode: Python -*-
> +# vim: filetype=python
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +##
> +# = eBPF Objects
> +##
> +
> +{ 'include': 'common.json' }
> +
> +##
> +# @EbpfObject:
> +#
> +# Structure that holds eBPF ELF object encoded in base64.
> +#
> +# Since: 8.3

We're just releasing 8.1, so next will be 8.2

There will never be any 8.3 because we reset to next major
version in the first release of each year.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v7 00/14] linux-user: brk fixes

[Joel Stanley]

On Thu, 3 Aug 2023 at 13:55, Helge Deller  wrote:
> > 143551 brk(NULL) = 0x0009b000
> > 143551 brk(0x0009b8fc) = 0x0009b000
>
> I think the problem is the brk with 9b000 here.
> It's not 64k aligned (=pages size of your ppc64le).
>
> Please try with this patch on top of Richard's series:
>
> > @@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, 
> > int image_fd,
> >   info->end_code = 0;
> >   info->start_data = -1;
> >   info->end_data = 0;
> > -info->brk = .
> change that to become:
>  info->brk = HOST_PAGE_ALIGN(hiaddr);

That stopped the crashing, and the binaries seem to run fine. I tested
on two hosts: ppc64le (64K) and arm64 (16K).

Cheers,

Joel

Re: [PATCH] linux-user/elfload: Set V in ELF_HWCAP for RISC-V

[Daniel Henrique Barboza]

On 8/3/23 10:14, Nathan Egge wrote:

From: "Nathan Egge" 

Set V bit for hwcap if misa is set.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1793
Signed-off-by: Nathan Egge 
---


Tested with the example program described in the bug:

===
#include 
#include 

#define ISA_V_HWCAP (1 << ('v' - 'a'))

void main() {
  unsigned long hw_cap = getauxval(AT_HWCAP);
  printf("RVV %s\n", hw_cap & ISA_V_HWCAP ? "detected" : "not found");
}
===

$ ./qemu-riscv64 -cpu rv64,vext_spec=v1.0,v=true,vlen=128 -B 0x10 ./a.out
RVV detected
$ ./qemu-riscv64 -cpu rv64,vext_spec=v1.0,vlen=128 -B 0x10 ./a.out
RVV not found


Reviewed-by: Daniel Henrique Barboza 
Tested-by: Daniel Henrique Barboza 


Looks like 8.1 material to me. Thanks,


Daniel


  linux-user/elfload.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 861ec07abc..a299ba7300 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1710,7 +1710,8 @@ static uint32_t get_elf_hwcap(void)
  #define MISA_BIT(EXT) (1 << (EXT - 'A'))
  RISCVCPU *cpu = RISCV_CPU(thread_cpu);
  uint32_t mask = MISA_BIT('I') | MISA_BIT('M') | MISA_BIT('A')
-| MISA_BIT('F') | MISA_BIT('D') | MISA_BIT('C');
+| MISA_BIT('F') | MISA_BIT('D') | MISA_BIT('C')
+| MISA_BIT('V');
  
  return cpu->env.misa_ext & mask;

  #undef MISA_BIT

Re: [PATCH] target/arm: Do not use gen_mte_checkN in trans_STGP

[Richard Henderson]

On 8/3/23 06:10, Peter Maydell wrote:

On Thu, 27 Jul 2023 at 17:33, Richard Henderson

-mop = MO_128;
-if (s->align_mem) {
-mop |= MO_ALIGN_8;
-}
-mop = finalize_memop_pair(s, mop);
+mop = MO_128 | MO_ALIGN | MO_ATOM_IFALIGN_PAIR;


So here we're implicitly assuming TAG_GRANULE is 16 and
then relying on the codegen for a MO_128 | MO_ALIGN
operation to give us the alignment fault if the guest
address isn't aligned to the tag granule, right ?


Yes.



Previously we also put s->be_data into the MemOp
(via finalize_memop_pair() calling finalize_memop_atom()).
Don't we still need to do that ?


Whoops, yes.


r~

Re: [PATCH v2 1/2] migration: Split await_return_path_close_on_source

[Fabiano Rosas]

Peter Xu  writes:

> On Wed, Aug 02, 2023 at 04:58:38PM -0300, Fabiano Rosas wrote:
>> Peter Xu  writes:
>> 
>> > On Wed, Aug 02, 2023 at 11:36:43AM -0300, Fabiano Rosas wrote:
>> >> This function currently has a straight-forward part which is waiting
>> >> for the thread to join and a complicated part which is doing a
>> >> qemu_file_shutdown() on the return path file.
>> >> 
>> >> The shutdown is tricky because all calls to qemu_file_shutdown() set
>> >> f->last_error to -EIO, which means we can never know if an error is an
>> >> actual error or if we cleanly shutdown the file previously.
>> >> 
>> >> This is particularly bothersome for postcopy because it would send the
>> >> return path thread into the retry routine which would wait on the
>> >> postcopy_pause_rp_sem and consequently block the main thread. We
>> >> haven't had reports of this so I must presume we never reach here with
>> >> postcopy.
>> >> 
>> >> The shutdown call is also racy because since it doesn't take the
>> >> qemu_file_lock, it could NULL-dereference if the return path thread
>> >> happens to be in the middle of the critical region at
>> >> migration_release_dst_files().
>> >
>> > After you rework the thread model on resume, shall we move
>> > migration_release_dst_files() into the migration thread to be after the
>> > pthread_join()?  I assume then we don't even need a mutex to protect it?
>> >
>> 
>> I just need to figure out if it's ok to move the postcopy_qemufile_src
>> cleanup along. No idea why it is there in the first place. I see you
>> moved it from postcopy_pause and we're about to move it back to the
>> exact same place =D
>
> It was there because the old postcopy-preempt was sending data via
> postcopy_qemufile_src from the migration thread, while postcopy_pause is
> also the migration thread context.
>
> Then we had 9358982744 ("migration: Send requested page directly in
> rp-return thread") where we moved that "send page" operation into the
> return path thread to reduce latencies.  After moving there it also means
> the file handle can be accessed in >1 threads, so I just moved it over to
> operate that always in the return path thread, then no race should happen.
>

Thanks for the context.

> With your change, return path will vanish before migration thread accesses
> it later (so as mentioned above, it must be after pthread_join()
> succeeded), then I assume it'll be fine too to have it back in migration
> thread.
>
> Or perhaps just take the file lock?
>

There's also migrate_fd_cleanup and migrate_fd_cancel that can touch
these files. We might need to lock anyway, let's see.

In general I'd like to drop all of these "ok not to lock, because...",
it's too easy for code to change and the assumptions to stop being
true. IMHO it's not worth it to gain performance by not taking a lock
when the data is still shared and there's nothing stopping someone in
the future from accessing it concurrently.

>> 
>> >> 
>> >> Move this more complicated part of the code to a separate routine so
>> >> we can wait on the thread without all of this baggage.
>> >
>> > I think you mentioned "some nuance" on having mark_source_rp_bad() in
>> > await_return_path_close_on_source(), I did remember I tried to look into
>> > that "nuance" too a long time ago but I just forgot what was that.  Great
>> > if you can share some details.
>> >
>> 
>> Well, mark_source_rp_bad() at await_return_path_close_on_source() is
>> basically useless:
>> 
>> - We only call mark_source_rp_bad() if s->to_dst_file has an error and the
>>   migration_completion() already checks that condition and fails the
>>   migration anyway.
>> 
>> - If to_dst_file has an error, chances are the destination already did
>>   cleanup by this point, so from_dst_file would already have an errno,
>>   due to that. At qemu_fill_buffer(), the len == 0 case basically means
>>   "the other end finished cleanly". We still set -EIO in that case, I
>>   don't know why. Possibly because not all backends will have the same
>>   semantics for len == 0.
>
> I don't know either, but I checked it's from a555b8092a ("qemu-file: Don't
> do IO after shutdown").  Maybe there's better way to do so we could
> identify the difference, but yes can be for later.
>

That is not the -EIO I'm talking about, but I'm glad you mentioned it,
because I just noticed we might need to revert ac7d25b816 ("qemu-file:
remove shutdown member").

It did a purely logical change which is to drop the f->shutdown flag,
but that has the side-effect that now we cannot differentiate an orderly
shutdown from an IO error.

I get that perhaps ideally all of the code would be resilient to be able
to handle a shutdown in the same way as an IO error, but I'm not sure we
are prepared for that.


Now, the actual -EIO I was mentioning is this one in qemu-file.c:

static ssize_t coroutine_mixed_fn qemu_fill_buffer(QEMUFile *f)
{
...
...
do {
len = qio_channel_read(f->ioc,
   (char *)f-

Re: [PATCH] target/m68k: Map FPU exceptions to FPSR register

[Richard Henderson]

On 8/2/23 20:52, Keith Packard via wrote:

Add helpers for reading/writing the 68881 FPSR register so that
changes in floating point exception state can be seen by the
application.

Call these helpers in pre_load/post_load hooks to synchronize
exception state.

Signed-off-by: Keith Packard
---
  target/m68k/cpu.c| 12 +++
  target/m68k/cpu.h|  2 ++
  target/m68k/fpu_helper.c | 72 
  target/m68k/helper.c |  4 +--
  target/m68k/helper.h |  2 ++
  target/m68k/translate.c  |  4 +--
  6 files changed, 92 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 


r~

[PATCH v3 0/3] qmp, vhost-user: Remove virtio_list & update virtio introspection

[Jonah Palmer]

These patches update a few things related to virtio introspection via.
QMP/HMP commands.

1. Remove 'virtio_list' and instead query the QOM composition tree to
find any active & realized virtio devices.

The 'virtio_list' was duplicating information about virtio devices that
was already available in the QOM composition tree, so there was no need
to keep this list.

2. Add new transport, protocol, and device features as well as support
to introspect vhost-user-gpio devices.

Vhost-user-gpio previously had no support for introspection. Support for
introspecting its vhost-user device is now available in these patches.

3. Move VhostUserProtocolFeature definition to its corresponding header
file (vhost-user.h). Cleanup previous definitions in other files.

VhostUserProtocolFeature was being defined in 3 separate files. Instead
of 3 separate definitions, use one instead and add it to the
vhost-user.h header file.

New virtio transport feature:
-
 - VIRTIO_F_RING_RESET

New vhost-user protocol feature:

 - VHOST_USER_PROTOCOL_F_STATUS

New virtio device features:
---
virtio-blk:
 - VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
 - VIRTIO_NET_F_NOTF_COAL
 - VIRTIO_NET_F_GUEST_USO4
 - VIRTIO_NET_F_GUEST_USO6
 - VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
 - VIRTIO_GPIO_F_IRQ
 - VHOST_F_LOG_ALL
 - VHOST_USER_F_PROTOCOL_FEATURES

v3: use recursion and type casting to find realized virtio devices
remove virtio scmi & bluetooth feature mappings
revert virtio scmi & bluetooth case changes in qmp_decode_features
change config define for VIRTIO_GPIO to CONFIG_VHOST_USER_GPIO
move VhostUserProtocolFeature definition to header file

v2: verify virtio devices via. 'TYPE_VIRTIO_DEVICES'
verify path is a virtio device before checking if it's realized
remove 'VIRTIO_BLK_F_ZONED' update (already exists)
add cover letter

Jonah Palmer (3):
  qmp: remove virtio_list, search QOM tree instead
  qmp: update virtio feature maps, vhost-user-gpio introspection
  vhost-user: move VhostUserProtocolFeature definition to header file

 hw/scsi/vhost-user-scsi.c  |   4 -
 hw/virtio/vhost-user-gpio.c|   7 ++
 hw/virtio/vhost-user.c |  21 -
 hw/virtio/virtio-qmp.c | 154 -
 hw/virtio/virtio-qmp.h |   7 --
 hw/virtio/virtio.c |   6 --
 include/hw/virtio/vhost-user.h |  21 +
 7 files changed, 105 insertions(+), 115 deletions(-)

-- 
2.39.3

[PATCH v3 1/3] qmp: remove virtio_list, search QOM tree instead

[Jonah Palmer]

The virtio_list duplicates information about virtio devices that already
exist in the QOM composition tree. Instead of creating this list of
realized virtio devices, search the QOM composition tree instead.

This patch modifies the QMP command qmp_x_query_virtio to instead
recursively search the QOM composition tree for devices of type
'TYPE_VIRTIO_DEVICE'. The device is also checked to ensure it's
realized.

[Jonah: In the previous commit the qmp_x_query_virtio function was
 iterating through devices found via. qmp_qom_list and appending
 "/virtio-backend" to devices' paths to check if they were a virtio
 device.

 This method was messy and involved unneeded string manipulation.

 Instead, we can use recursion with object_get_root to iterate through
 all parent and child device paths to find virtio devices.

 The qmp_find_virtio_device function was also updated to simplify the
 method of determining if a path is to a valid and realized virtio
 device.]

Signed-off-by: Jonah Palmer 
---
 hw/virtio/virtio-qmp.c | 96 ++
 hw/virtio/virtio-qmp.h |  7 ---
 hw/virtio/virtio.c |  6 ---
 3 files changed, 40 insertions(+), 69 deletions(-)

diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index 3d32dbec8d..baec351c4f 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -665,70 +665,54 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t 
device_id, uint64_t bitmap)
 return features;
 }
 
-VirtioInfoList *qmp_x_query_virtio(Error **errp)
+static int query_dev_child(Object *child, void *opaque)
 {
-VirtioInfoList *list = NULL;
-VirtioInfo *node;
-VirtIODevice *vdev;
+VirtioInfoList **vdevs = opaque;
+Object *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
+if (dev != NULL && DEVICE(dev)->realized) {
+VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+
+VirtioInfo *info = g_new(VirtioInfo, 1);
+
+/* Get canonical path of device */
+gchar *path = object_get_canonical_path(dev);
+
+info->path = g_strdup(path);
+info->name = g_strdup(vdev->name);
+QAPI_LIST_PREPEND(*vdevs, info);
 
-QTAILQ_FOREACH(vdev, &virtio_list, next) {
-DeviceState *dev = DEVICE(vdev);
-Error *err = NULL;
-QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
-
-if (err == NULL) {
-GString *is_realized = qobject_to_json_pretty(obj, true);
-/* virtio device is NOT realized, remove it from list */
-if (!strncmp(is_realized->str, "false", 4)) {
-QTAILQ_REMOVE(&virtio_list, vdev, next);
-} else {
-node = g_new(VirtioInfo, 1);
-node->path = g_strdup(dev->canonical_path);
-node->name = g_strdup(vdev->name);
-QAPI_LIST_PREPEND(list, node);
-}
-   g_string_free(is_realized, true);
-}
-qobject_unref(obj);
+g_free(path);
+} else {
+object_unref(dev);
 }
 
-return list;
+return 0;
 }
 
-VirtIODevice *qmp_find_virtio_device(const char *path)
+VirtioInfoList *qmp_x_query_virtio(Error **errp)
 {
-VirtIODevice *vdev;
+VirtioInfoList *vdevs = NULL;
 
-QTAILQ_FOREACH(vdev, &virtio_list, next) {
-DeviceState *dev = DEVICE(vdev);
-
-if (strcmp(dev->canonical_path, path) != 0) {
-continue;
-}
-
-Error *err = NULL;
-QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
-if (err == NULL) {
-GString *is_realized = qobject_to_json_pretty(obj, true);
-/* virtio device is NOT realized, remove it from list */
-if (!strncmp(is_realized->str, "false", 4)) {
-g_string_free(is_realized, true);
-qobject_unref(obj);
-QTAILQ_REMOVE(&virtio_list, vdev, next);
-return NULL;
-}
-g_string_free(is_realized, true);
-} else {
-/* virtio device doesn't exist in QOM tree */
-QTAILQ_REMOVE(&virtio_list, vdev, next);
-qobject_unref(obj);
-return NULL;
-}
-/* device exists in QOM tree & is realized */
-qobject_unref(obj);
-return vdev;
+/* Query the QOM composition tree recursively for virtio devices */
+object_child_foreach_recursive(object_get_root(), query_dev_child, &vdevs);
+if (vdevs == NULL) {
+error_setg(errp, "No virtio devices found");
+return NULL;
 }
-return NULL;
+return vdevs;
+}
+
+VirtIODevice *qmp_find_virtio_device(const char *path)
+{
+/* Verify the canonical path is a realized virtio device */
+Object *dev = object_dynamic_cast(object_resolve_path(path, NULL),
+  TYPE_VIRTIO_DEVICE);
+if (!dev || !DEVICE(dev)->realized) {
+object_unref(dev);
+return NULL;
+}
+
+return VIRTIO_DEVICE(d

[PATCH v3 2/3] qmp: update virtio feature maps, vhost-user-gpio introspection

[Jonah Palmer]

Add new virtio transport feature to transport feature map:
 - VIRTIO_F_RING_RESET

Add new vhost-user protocol feature to vhost-user protocol feature map
and enumeration:
 - VHOST_USER_PROTOCOL_F_STATUS

Add new virtio device features for several virtio devices to their
respective feature mappings:

virtio-blk:
 - VIRTIO_BLK_F_SECURE_ERASE

virtio-net:
 - VIRTIO_NET_F_NOTF_COAL
 - VIRTIO_NET_F_GUEST_USO4
 - VIRTIO_NET_F_GUEST_USO6
 - VIRTIO_NET_F_HOST_USO

virtio/vhost-user-gpio:
 - VIRTIO_GPIO_F_IRQ
 - VHOST_F_LOG_ALL
 - VHOST_USER_F_PROTOCOL_FEATURES

Add support for introspection on vhost-user-gpio devices.

Signed-off-by: Jonah Palmer 
---
 hw/virtio/vhost-user-gpio.c |  7 +++
 hw/virtio/virtio-qmp.c  | 38 -
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3b013f2d0f..3d7fae3984 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -205,6 +205,12 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice 
*vdev, int idx, bool mask)
 vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
 }
 
+static struct vhost_dev *vu_gpio_get_vhost(VirtIODevice *vdev)
+{
+VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+return &gpio->vhost_dev;
+}
+
 static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
 {
 virtio_delete_queue(gpio->command_vq);
@@ -413,6 +419,7 @@ static void vu_gpio_class_init(ObjectClass *klass, void 
*data)
 vdc->get_config = vu_gpio_get_config;
 vdc->set_status = vu_gpio_set_status;
 vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+vdc->get_vhost = vu_gpio_get_vhost;
 }
 
 static const TypeInfo vu_gpio_info = {
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index baec351c4f..a38b49af8a 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -30,6 +30,7 @@
 #include "standard-headers/linux/virtio_iommu.h"
 #include "standard-headers/linux/virtio_mem.h"
 #include "standard-headers/linux/virtio_vsock.h"
+#include "standard-headers/linux/virtio_gpio.h"
 
 #include CONFIG_DEVICES
 
@@ -53,6 +54,7 @@ enum VhostUserProtocolFeature {
 VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
 VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
 VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+VHOST_USER_PROTOCOL_F_STATUS = 16,
 VHOST_USER_PROTOCOL_F_MAX
 };
 
@@ -79,6 +81,8 @@ static const qmp_virtio_feature_map_t virtio_transport_map[] 
= {
 "VIRTIO_F_ORDER_PLATFORM: Memory accesses ordered by platform"),
 FEATURE_ENTRY(VIRTIO_F_SR_IOV, \
 "VIRTIO_F_SR_IOV: Device supports single root I/O virtualization"),
+FEATURE_ENTRY(VIRTIO_F_RING_RESET, \
+"VIRTIO_F_RING_RESET: Driver can reset individual VQs"),
 /* Virtio ring transport features */
 FEATURE_ENTRY(VIRTIO_RING_F_INDIRECT_DESC, \
 "VIRTIO_RING_F_INDIRECT_DESC: Indirect descriptors supported"),
@@ -134,6 +138,9 @@ static const qmp_virtio_feature_map_t 
vhost_user_protocol_map[] = {
 FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS, \
 "VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS: Configuration for "
 "memory slots supported"),
+FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
+"VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
+"device status supported"),
 { -1, "" }
 };
 
@@ -176,6 +183,8 @@ static const qmp_virtio_feature_map_t 
virtio_blk_feature_map[] = {
 "VIRTIO_BLK_F_DISCARD: Discard command supported"),
 FEATURE_ENTRY(VIRTIO_BLK_F_WRITE_ZEROES, \
 "VIRTIO_BLK_F_WRITE_ZEROES: Write zeroes command supported"),
+FEATURE_ENTRY(VIRTIO_BLK_F_SECURE_ERASE, \
+"VIRTIO_BLK_F_SECURE_ERASE: Secure erase supported"),
 FEATURE_ENTRY(VIRTIO_BLK_F_ZONED, \
 "VIRTIO_BLK_F_ZONED: Zoned block devices"),
 #ifndef VIRTIO_BLK_NO_LEGACY
@@ -299,6 +308,14 @@ static const qmp_virtio_feature_map_t 
virtio_net_feature_map[] = {
 FEATURE_ENTRY(VIRTIO_NET_F_CTRL_MAC_ADDR, \
 "VIRTIO_NET_F_CTRL_MAC_ADDR: MAC address set through control "
 "channel"),
+FEATURE_ENTRY(VIRTIO_NET_F_NOTF_COAL, \
+"VIRTIO_NET_F_NOTF_COAL: Device supports coalescing 
notifications"),
+FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO4, \
+"VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv4"),
+FEATURE_ENTRY(VIRTIO_NET_F_GUEST_USO6, \
+"VIRTIO_NET_F_GUEST_USO4: Driver can receive USOv6"),
+FEATURE_ENTRY(VIRTIO_NET_F_HOST_USO, \
+"VIRTIO_NET_F_HOST_USO: Device can receive USO"),
 FEATURE_ENTRY(VIRTIO_NET_F_HASH_REPORT, \
 "VIRTIO_NET_F_HASH_REPORT: Hash reporting supported"),
 FEATURE_ENTRY(VIRTIO_NET_F_RSS, \
@@ -469,6 +486,20 @@ static const qmp_virtio_feature_map_t 
virtio_rng_feature_map[] = {
 };
 #endif
 
+/* virtio/vhost-gpio features mapping */
+#ifdef CONFIG_VHOST_USER

[PATCH v3 3/3] vhost-user: move VhostUserProtocolFeature definition to header file

[Jonah Palmer]

Move the definition of VhostUserProtocolFeature to
include/hw/virtio/vhost-user.h.

Remove previous definitions in hw/scsi/vhost-user-scsi.c,
hw/virtio/vhost-user.c, and hw/virtio/virtio-qmp.c.

Previously there were 3 separate definitions of this over 3 different
files. Now only 1 definition of this will be present for these 3 files.

Signed-off-by: Jonah Palmer 
---
 hw/scsi/vhost-user-scsi.c  |  4 
 hw/virtio/vhost-user.c | 21 -
 hw/virtio/virtio-qmp.c | 22 +-
 include/hw/virtio/vhost-user.h | 21 +
 4 files changed, 22 insertions(+), 46 deletions(-)

diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index ee99b19e7a..df6b66cc1a 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -39,10 +39,6 @@ static const int user_feature_bits[] = {
 VHOST_INVALID_FEATURE_BIT
 };
 
-enum VhostUserProtocolFeature {
-VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
-};
-
 static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
 {
 VHostUserSCSI *s = (VHostUserSCSI *)vdev;
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8dcf049d42..a096335921 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -56,27 +56,6 @@
  */
 #define VHOST_USER_MAX_CONFIG_SIZE 256
 
-enum VhostUserProtocolFeature {
-VHOST_USER_PROTOCOL_F_MQ = 0,
-VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
-VHOST_USER_PROTOCOL_F_RARP = 2,
-VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
-VHOST_USER_PROTOCOL_F_NET_MTU = 4,
-VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5,
-VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
-VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
-VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
-VHOST_USER_PROTOCOL_F_CONFIG = 9,
-VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10,
-VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11,
-VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
-VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
-/* Feature 14 reserved for VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS. */
-VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
-VHOST_USER_PROTOCOL_F_STATUS = 16,
-VHOST_USER_PROTOCOL_F_MAX
-};
-
 #define VHOST_USER_PROTOCOL_FEATURE_MASK ((1 << VHOST_USER_PROTOCOL_F_MAX) - 1)
 
 typedef enum VhostUserRequest {
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index a38b49af8a..69880fe7c5 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -17,6 +17,7 @@
 #include "qapi/qapi-commands-qom.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp/qjson.h"
+#include "hw/virtio/vhost-user.h"
 
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/vhost_types.h"
@@ -37,27 +38,6 @@
 #define FEATURE_ENTRY(name, desc) (qmp_virtio_feature_map_t) \
 { .virtio_bit = name, .feature_desc = desc }
 
-enum VhostUserProtocolFeature {
-VHOST_USER_PROTOCOL_F_MQ = 0,
-VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
-VHOST_USER_PROTOCOL_F_RARP = 2,
-VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
-VHOST_USER_PROTOCOL_F_NET_MTU = 4,
-VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5,
-VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
-VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
-VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
-VHOST_USER_PROTOCOL_F_CONFIG = 9,
-VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10,
-VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11,
-VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
-VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
-VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
-VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
-VHOST_USER_PROTOCOL_F_STATUS = 16,
-VHOST_USER_PROTOCOL_F_MAX
-};
-
 /* Virtio transport features mapping */
 static const qmp_virtio_feature_map_t virtio_transport_map[] = {
 /* Virtio device transport features */
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index 191216a74f..80e2b4a463 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -11,6 +11,27 @@
 #include "chardev/char-fe.h"
 #include "hw/virtio/virtio.h"
 
+enum VhostUserProtocolFeature {
+VHOST_USER_PROTOCOL_F_MQ = 0,
+VHOST_USER_PROTOCOL_F_LOG_SHMFD = 1,
+VHOST_USER_PROTOCOL_F_RARP = 2,
+VHOST_USER_PROTOCOL_F_REPLY_ACK = 3,
+VHOST_USER_PROTOCOL_F_NET_MTU = 4,
+VHOST_USER_PROTOCOL_F_BACKEND_REQ = 5,
+VHOST_USER_PROTOCOL_F_CROSS_ENDIAN = 6,
+VHOST_USER_PROTOCOL_F_CRYPTO_SESSION = 7,
+VHOST_USER_PROTOCOL_F_PAGEFAULT = 8,
+VHOST_USER_PROTOCOL_F_CONFIG = 9,
+VHOST_USER_PROTOCOL_F_BACKEND_SEND_FD = 10,
+VHOST_USER_PROTOCOL_F_HOST_NOTIFIER = 11,
+VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD = 12,
+VHOST_USER_PROTOCOL_F_RESET_DEVICE = 13,
+VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14,
+VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15,
+VHOST_USER_PROTOCOL_F_STATUS = 16,
+VHOST_USER_PROTOCOL_F_MAX
+};
+
 /**
  * VhostUserHostNotifier - notifier information for one queue
  * @rcu: rcu_head for cleanup

Re: [PATCH v2] Fix SEGFAULT on getting physical address of MMIO region.

[Richard Henderson]

On 8/3/23 03:58, Mikhail Tyutin wrote:

Apply save_iotlb_data() to io_readx() as well as to io_writex().

Signed-off-by: Dmitriy Solovev 
Signed-off-by: Mikhail Tyutin 
---
  accel/tcg/cputlb.c | 36 +---
  1 file changed, 21 insertions(+), 15 deletions(-)


Reviewed-by: Richard Henderson 


r~



diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index ba44501a7c..addce3be38 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1363,6 +1363,21 @@ static inline void cpu_transaction_failed(CPUState *cpu, 
hwaddr physaddr,
  }
  }
  
+/*

+ * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
+ * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
+ * because of the side effect of io_writex changing memory layout.
+ */
+static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
+hwaddr mr_offset)
+{
+#ifdef CONFIG_PLUGIN
+SavedIOTLB *saved = &cs->saved_iotlb;
+saved->section = section;
+saved->mr_offset = mr_offset;
+#endif
+}
+
  static uint64_t io_readx(CPUArchState *env, CPUTLBEntryFull *full,
   int mmu_idx, vaddr addr, uintptr_t retaddr,
   MMUAccessType access_type, MemOp op)
@@ -1382,6 +1397,12 @@ static uint64_t io_readx(CPUArchState *env, 
CPUTLBEntryFull *full,
  cpu_io_recompile(cpu, retaddr);
  }
  
+/*

+ * The memory_region_dispatch may trigger a flush/resize
+ * so for plugins we save the iotlb_data just in case.
+ */
+save_iotlb_data(cpu, section, mr_offset);
+
  {
  QEMU_IOTHREAD_LOCK_GUARD();
  r = memory_region_dispatch_read(mr, mr_offset, &val, op, full->attrs);
@@ -1398,21 +1419,6 @@ static uint64_t io_readx(CPUArchState *env, 
CPUTLBEntryFull *full,
  return val;
  }
  
-/*

- * Save a potentially trashed CPUTLBEntryFull for later lookup by plugin.
- * This is read by tlb_plugin_lookup if the fulltlb entry doesn't match
- * because of the side effect of io_writex changing memory layout.
- */
-static void save_iotlb_data(CPUState *cs, MemoryRegionSection *section,
-hwaddr mr_offset)
-{
-#ifdef CONFIG_PLUGIN
-SavedIOTLB *saved = &cs->saved_iotlb;
-saved->section = section;
-saved->mr_offset = mr_offset;
-#endif
-}
-
  static void io_writex(CPUArchState *env, CPUTLBEntryFull *full,
int mmu_idx, uint64_t val, vaddr addr,
uintptr_t retaddr, MemOp op)

Re: [PATCH v7 00/14] linux-user: brk fixes

[Helge Deller]

* Joel Stanley :
> On Thu, 3 Aug 2023 at 13:55, Helge Deller  wrote:
> > > 143551 brk(NULL) = 0x0009b000
> > > 143551 brk(0x0009b8fc) = 0x0009b000
> >
> > I think the problem is the brk with 9b000 here.
> > It's not 64k aligned (=pages size of your ppc64le).
> >
> > Please try with this patch on top of Richard's series:
> >
> > > @@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, 
> > > int image_fd,
> > >   info->end_code = 0;
> > >   info->start_data = -1;
> > >   info->end_data = 0;
> > > -info->brk = .
> > change that to become:
> >  info->brk = HOST_PAGE_ALIGN(hiaddr);
>
> That stopped the crashing, and the binaries seem to run fine. I tested
> on two hosts: ppc64le (64K) and arm64 (16K).

Great!

That made re-read Akihiko's patch:

Author: Akihiko Odaki 
linux-user: Do not align brk with host page size

do_brk() minimizes calls into target_mmap() by aligning the address
with host page size, which is potentially larger than the target page
size. However, the current implementation of this optimization has two
bugs:

- The start of brk is rounded up with the host page size while brk
  advertises an address aligned with the target page size as the
  beginning of brk. This makes the beginning of brk unmapped.

this patch has wrong assumptions.

The start of brk always needs to be host page aligned.
It's not an optimization, but a requirement, since brk needs to be
located on a host-aligned page which may get different permissions
than the page before it (where code from the binary may be located).

I wonder if we need that patch at all.


Joel, could you give the patch below on top of git head (no other
patches applied) a spin?
(I just tested it here locally on a full range of linux-user chroots)

I think this is ALL what's needed for git head to fix the static binary
issues, has a nice preparation for Richard's ELF_ET_DYN_BASE patches.

If it does, it replaces patches 1,2 & 4-6 from Richard's v7 patch
series.

Helge



diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 861ec07abc..88d9e4056e 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -3021,8 +3021,10 @@ static void load_elf_image(const char *image_name, int 
image_fd,
 struct elfhdr *ehdr = (struct elfhdr *)bprm_buf;
 struct elf_phdr *phdr;
 abi_ulong load_addr, load_bias, loaddr, hiaddr, error;
+unsigned long load_offset = 0;
 int i, retval, prot_exec;
 Error *err = NULL;
+bool is_main_executable;

 /* First of all, some simple consistency checks */
 if (!elf_check_ident(ehdr)) {
@@ -3106,28 +3108,8 @@ static void load_elf_image(const char *image_name, int 
image_fd,
 }
 }

-if (pinterp_name != NULL) {
-/*
- * This is the main executable.
- *
- * Reserve extra space for brk.
- * We hold on to this space while placing the interpreter
- * and the stack, lest they be placed immediately after
- * the data segment and block allocation from the brk.
- *
- * 16MB is chosen as "large enough" without being so large as
- * to allow the result to not fit with a 32-bit guest on a
- * 32-bit host. However some 64 bit guests (e.g. s390x)
- * attempt to place their heap further ahead and currently
- * nothing stops them smashing into QEMUs address space.
- */
-#if TARGET_LONG_BITS == 64
-info->reserve_brk = 32 * MiB;
-#else
-info->reserve_brk = 16 * MiB;
-#endif
-hiaddr += info->reserve_brk;
-
+is_main_executable = (pinterp_name != NULL);
+if (is_main_executable) {
 if (ehdr->e_type == ET_EXEC) {
 /*
  * Make sure that the low address does not conflict with
@@ -3136,10 +3118,11 @@ static void load_elf_image(const char *image_name, int 
image_fd,
 probe_guest_base(image_name, loaddr, hiaddr);
 } else {
 /*
- * The binary is dynamic, but we still need to
+ * The binary is dynamic (pie-executabe), but we still need to
  * select guest_base.  In this case we pass a size.
  */
 probe_guest_base(image_name, 0, hiaddr - loaddr);
+load_offset = 0 /* TODO: should be ELF_ET_DYN_BASE */;
 }
 }

@@ -3157,9 +3140,9 @@ static void load_elf_image(const char *image_name, int 
image_fd,
  * In both cases, we will overwrite pages in this range with mappings
  * from the executable.
  */
-load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE,
+load_addr = target_mmap(loaddr + load_offset, (size_t)hiaddr - loaddr + 1, 
PROT_NONE,
 MAP_PRIVATE | MAP_ANON | MAP_NORESERVE |
-(ehdr->e_type == ET_EXEC ? MAP_FIXED : 0),
+(is_main_executable ? MAP_FIXED : 0),
 -1, 0);
 if (load_addr == -1)

Re: [PATCH v2 2/2] migration: Replace the return path retry logic

[Fabiano Rosas]

Peter Xu  writes:

> On Wed, Aug 02, 2023 at 05:04:45PM -0300, Fabiano Rosas wrote:
>> Peter Xu  writes:
>> 
>> >> +if (await_return_path_close_on_source(s)) {
>> >> +trace_migration_return_path_pause_err();
>> >> +return MIG_THR_ERR_FATAL;
>> >> +}
>> >
>> > I see that here on return path failures we'll bail out, and actually it's
>> > against the instinction (that when pause it should have failed, so it's
>> > weird why it's returning 0).
>> >
>> > So how about above suggestion, plus here we just call
>> > await_return_path_close_on_source(), without caring about the retval?
>> 
>> So you are suggesting to remove the knowledge of the retry entirely from
>> the thread. It just reports the error and the postcopy_pause takes the
>> responsibility of ignoring it when we want to retry... It could be
>> clearer that way indeed.
>
> That error doesn't really important IMHO here, because the to-dst-file
> should have already errored out anyway.
>
> I just think it cleaner if we reset rp_error only until the new thread
> created.
>

ok

>> 
>> It would trigger when a rp error happened that wasn't related to the
>> QEMUFile. If we go with your suggestion above, then this goes away.
>
> With your current patch where rp_error seems to be always reset when thread
> quit, if that's true then it'll 100% happen that this will not trigger.
>
> But yeah this is a trivial spot, feel free to choose the best if you plan
> to reorganize this patch a bit.  Thanks.

My patch just resets the error when doing postcopy and the error is a
QEMUFile error. The header validation at the start of the loop could
still set rp_state.error and return without going through the postcopy
retry:

if (header_type >= MIG_RP_MSG_MAX ||
header_type == MIG_RP_MSG_INVALID) {
error_report("RP: Received invalid message 0x%04x length 0x%04x",
 header_type, header_len);
mark_source_rp_bad(ms);
goto out;
}

Re: [PATCH v3 1/3] qmp: remove virtio_list, search QOM tree instead

[Daniel P . Berrangé]

On Thu, Aug 03, 2023 at 10:54:58AM -0400, Jonah Palmer wrote:
> The virtio_list duplicates information about virtio devices that already
> exist in the QOM composition tree. Instead of creating this list of
> realized virtio devices, search the QOM composition tree instead.
> 
> This patch modifies the QMP command qmp_x_query_virtio to instead
> recursively search the QOM composition tree for devices of type
> 'TYPE_VIRTIO_DEVICE'. The device is also checked to ensure it's
> realized.
> 
> [Jonah: In the previous commit the qmp_x_query_virtio function was
>  iterating through devices found via. qmp_qom_list and appending
>  "/virtio-backend" to devices' paths to check if they were a virtio
>  device.
> 
>  This method was messy and involved unneeded string manipulation.
> 
>  Instead, we can use recursion with object_get_root to iterate through
>  all parent and child device paths to find virtio devices.
> 
>  The qmp_find_virtio_device function was also updated to simplify the
>  method of determining if a path is to a valid and realized virtio
>  device.]

FWIW, this "history" would typically go after the '---' but before
the diffstat, as it is relevant to reviewers of this new v3, but
doesn't need to get into the permanent git log once merged.

> Signed-off-by: Jonah Palmer 
> ---
>  hw/virtio/virtio-qmp.c | 96 ++
>  hw/virtio/virtio-qmp.h |  7 ---
>  hw/virtio/virtio.c |  6 ---
>  3 files changed, 40 insertions(+), 69 deletions(-)

Reviewed-by: Daniel P. Berrangé 


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH] hw/riscv: split RAM into low and high memory

[Andrew Jones]

On Mon, Jul 31, 2023 at 09:53:17AM +0800, Fei Wu wrote:
> riscv virt platform's memory started at 0x8000 and
> straddled the 4GiB boundary. Curiously enough, this choice
> of a memory layout will prevent from launching a VM with
> a bit more than 2000MiB and PCIe pass-thru on an x86 host, due
> to identity mapping requirements for the MSI doorbell on x86,
> and these (APIC/IOAPIC) live right below 4GiB.
> 
> So just split the RAM range into two portions:
> - 1 GiB range from 0x8000 to 0xc000.
> - The remainder at 0x1
> 
> ...leaving a hole between the ranges.

Can you elaborate on the use case? Maybe provide details of the host
system and the QEMU command line? I'm wondering why we didn't have
any problems with the arm virt machine type. Has nobody tried this
use case with that? Is the use case something valid for riscv, but
not arm?

Thanks,
drew

Re: [PATCH v7 00/14] linux-user: brk fixes

[Richard Henderson]

On 8/3/23 08:01, Helge Deller wrote:

* Joel Stanley :

On Thu, 3 Aug 2023 at 13:55, Helge Deller  wrote:

143551 brk(NULL) = 0x0009b000
143551 brk(0x0009b8fc) = 0x0009b000


I think the problem is the brk with 9b000 here.
It's not 64k aligned (=pages size of your ppc64le).

Please try with this patch on top of Richard's series:


@@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, int 
image_fd,
   info->end_code = 0;
   info->start_data = -1;
   info->end_data = 0;
-info->brk = .

change that to become:
  info->brk = HOST_PAGE_ALIGN(hiaddr);


That stopped the crashing, and the binaries seem to run fine. I tested
on two hosts: ppc64le (64K) and arm64 (16K).


Great!

That made re-read Akihiko's patch:

Author: Akihiko Odaki 
 linux-user: Do not align brk with host page size

 do_brk() minimizes calls into target_mmap() by aligning the address
 with host page size, which is potentially larger than the target page
 size. However, the current implementation of this optimization has two
 bugs:

 - The start of brk is rounded up with the host page size while brk
   advertises an address aligned with the target page size as the
   beginning of brk. This makes the beginning of brk unmapped.

this patch has wrong assumptions.

The start of brk always needs to be host page aligned.



There is a bunch of code in target_mmap that attempts to manage adjacent guest pages that 
fall into the same host page.  Akihiko's patch assumes that code actually works.  Which I 
think is entirely reasonable.


You can't move brk up like this either (without other adjustments to the binary mapping), 
since that will leave a hole in the guest address space, which can get filled with 
something else later, which will definitely cause problems.



r~

[PATCH v2] Allowing setting and overriding parameters in smb.conf

[Henrik Carlqvist]

>From a6dfb322a88965281e3bba00a92f8d5e437bfa95 Mon Sep 17 00:00:00 2001
From: Henrik Carlqvist 
Date: Thu, 3 Aug 2023 16:52:25 +0200
Subject: [PATCH] Allowing setting and overriding parameters in smb.conf,
 moving some parameters from the [qemu] section to the [global] section to
 allow them to get overridden by custom user settings.

Signed-off-by: Henrik Carlqvist 
---

In this second version of the patch I have moved also the "force user" 
parameter to the global section of smb.conf. Even though I do not self see the
usefullness of altering that parameter we might just as well give the users
the freedom to alter anything in smb.conf. Maybe someone else will see the 
need to alter that parameter.

Best regards Henrik

 net/slirp.c | 50 -
 qapi/net.json   |  3 +++
 qemu-options.hx | 15 ---
 3 files changed, 56 insertions(+), 12 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..e27d115bc4 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -106,7 +106,8 @@ static int slirp_guestfwd(SlirpState *s, const char 
*config_str, Error **errp);
 
 #if defined(CONFIG_SMBD_COMMAND)
 static int slirp_smb(SlirpState *s, const char *exported_dir,
- struct in_addr vserver_addr, Error **errp);
+ struct in_addr vserver_addr, const char *smbparams,
+ Error **errp);
 static void slirp_smb_cleanup(SlirpState *s);
 #else
 static inline void slirp_smb_cleanup(SlirpState *s) { }
@@ -424,6 +425,7 @@ static int net_slirp_init(NetClientState *peer, const char 
*model,
   const char *bootfile, const char *vdhcp_start,
   const char *vnameserver, const char *vnameserver6,
   const char *smb_export, const char *vsmbserver,
+  const char *smbparams,
   const char **dnssearch, const char *vdomainname,
   const char *tftp_server_name,
   Error **errp)
@@ -678,7 +680,7 @@ static int net_slirp_init(NetClientState *peer, const char 
*model,
 }
 #if defined(CONFIG_SMBD_COMMAND)
 if (smb_export) {
-if (slirp_smb(s, smb_export, smbsrv, errp) < 0) {
+if (slirp_smb(s, smb_export, smbsrv, smbparams, errp) < 0) {
 goto error;
 }
 }
@@ -891,7 +893,8 @@ static void slirp_smb_cleanup(SlirpState *s)
 }
 
 static int slirp_smb(SlirpState* s, const char *exported_dir,
- struct in_addr vserver_addr, Error **errp)
+ struct in_addr vserver_addr, const char *smbparams,
+ Error **errp)
 {
 char *smb_conf;
 char *smb_cmdline;
@@ -950,11 +953,12 @@ static int slirp_smb(SlirpState* s, const char 
*exported_dir,
 "printing = bsd\n"
 "disable spoolss = yes\n"
 "usershare max shares = 0\n"
-"[qemu]\n"
-"path=%s\n"
 "read only=no\n"
 "guest ok=yes\n"
-"force user=%s\n",
+"force user=%s\n"
+   "%s"
+"[qemu]\n"
+"path=%s\n",
 s->smb_dir,
 s->smb_dir,
 s->smb_dir,
@@ -963,8 +967,9 @@ static int slirp_smb(SlirpState* s, const char 
*exported_dir,
 s->smb_dir,
 s->smb_dir,
 s->smb_dir,
-exported_dir,
-passwd->pw_name
+passwd->pw_name,
+smbparams,
+exported_dir
 );
 fclose(f);
 
@@ -1143,6 +1148,29 @@ static const char **slirp_dnssearch(const StringList 
*dnsname)
 return ret;
 }
 
+static char *slirp_smbparams(const StringList *smbparam)
+{
+const StringList *c = smbparam;
+size_t i = 1; /* for string terminating 0 */
+char *ret;
+
+while (c) {
+i += strlen(c->value->str);
+i++; /* for \n */
+c = c->next;
+}
+ret = g_malloc(i * sizeof(*ret));
+ret[0]=0; /* Start with empty string */
+
+c = smbparam;
+while (c) {
+pstrcat(ret, i * sizeof(*ret), c->value->str);
+pstrcat(ret, i * sizeof(*ret), "\n");
+c = c->next;
+}
+return ret;
+}
+
 int net_init_slirp(const Netdev *netdev, const char *name,
NetClientState *peer, Error **errp)
 {
@@ -1151,6 +1179,7 @@ int net_init_slirp(const Netdev *netdev, const char *name,
 int ret;
 const NetdevUserOptions *user;
 const char **dnssearch;
+char *smbparams;
 bool ipv4 = true, ipv6 = true;
 
 assert(netdev->type == NET_CLIENT_DRIVER_USER);
@@ -1170,6 +1199,7 @@ int net_init_slirp(const Netdev *netdev, const char *name,
NULL;
 
 dnssearch = slirp_dnssearch(user->dnssearch);
+smbparams = slirp_smbparams(user->smbparam);
 
 /* all optional fields are initialized to "all bits zero" */
 
@@ -1182,7 +1212,8 @@ int net_init_slirp(const Netdev *netdev, const char *name,

Re: [PATCH v2] Fix SEGFAULT on getting physical address of MMIO region.

[Peter Maydell]

On Thu, 3 Aug 2023 at 12:00, Mikhail Tyutin  wrote:
>
> Apply save_iotlb_data() to io_readx() as well as to io_writex().

Could we have a bit more detail in the commit message about
when you can get this segfault?

In particular, does this happen only if you're using plugins?

thanks
-- PMM

Re: [PATCH v2 1/2] migration: Split await_return_path_close_on_source

[Peter Xu]

On Thu, Aug 03, 2023 at 11:45:38AM -0300, Fabiano Rosas wrote:
> Peter Xu  writes:
> 
> > On Wed, Aug 02, 2023 at 04:58:38PM -0300, Fabiano Rosas wrote:
> >> Peter Xu  writes:
> >> 
> >> > On Wed, Aug 02, 2023 at 11:36:43AM -0300, Fabiano Rosas wrote:
> >> >> This function currently has a straight-forward part which is waiting
> >> >> for the thread to join and a complicated part which is doing a
> >> >> qemu_file_shutdown() on the return path file.
> >> >> 
> >> >> The shutdown is tricky because all calls to qemu_file_shutdown() set
> >> >> f->last_error to -EIO, which means we can never know if an error is an
> >> >> actual error or if we cleanly shutdown the file previously.
> >> >> 
> >> >> This is particularly bothersome for postcopy because it would send the
> >> >> return path thread into the retry routine which would wait on the
> >> >> postcopy_pause_rp_sem and consequently block the main thread. We
> >> >> haven't had reports of this so I must presume we never reach here with
> >> >> postcopy.
> >> >> 
> >> >> The shutdown call is also racy because since it doesn't take the
> >> >> qemu_file_lock, it could NULL-dereference if the return path thread
> >> >> happens to be in the middle of the critical region at
> >> >> migration_release_dst_files().
> >> >
> >> > After you rework the thread model on resume, shall we move
> >> > migration_release_dst_files() into the migration thread to be after the
> >> > pthread_join()?  I assume then we don't even need a mutex to protect it?
> >> >
> >> 
> >> I just need to figure out if it's ok to move the postcopy_qemufile_src
> >> cleanup along. No idea why it is there in the first place. I see you
> >> moved it from postcopy_pause and we're about to move it back to the
> >> exact same place =D
> >
> > It was there because the old postcopy-preempt was sending data via
> > postcopy_qemufile_src from the migration thread, while postcopy_pause is
> > also the migration thread context.
> >
> > Then we had 9358982744 ("migration: Send requested page directly in
> > rp-return thread") where we moved that "send page" operation into the
> > return path thread to reduce latencies.  After moving there it also means
> > the file handle can be accessed in >1 threads, so I just moved it over to
> > operate that always in the return path thread, then no race should happen.
> >
> 
> Thanks for the context.
> 
> > With your change, return path will vanish before migration thread accesses
> > it later (so as mentioned above, it must be after pthread_join()
> > succeeded), then I assume it'll be fine too to have it back in migration
> > thread.
> >
> > Or perhaps just take the file lock?
> >
> 
> There's also migrate_fd_cleanup and migrate_fd_cancel that can touch
> these files. We might need to lock anyway, let's see.

The cancel path shouldn't clear the QEMUFile*, then I assume it's fine.
That's based on the assumption that qemu_file_shutdown() is actually thread
safe (say, shutdown() syscall is thread-safe for sockets).

But yeah that depends on some more knowledge, it'll be good as you said
below to just always take the lock because that shouldn't hurt.

> 
> In general I'd like to drop all of these "ok not to lock, because...",
> it's too easy for code to change and the assumptions to stop being
> true. IMHO it's not worth it to gain performance by not taking a lock
> when the data is still shared and there's nothing stopping someone in
> the future from accessing it concurrently.

Yes I agree with you.

If you want we can move that from rp thread to migration thread, but with
the lock added altogether.  Then rp thread can always guarantee to have the
file there which should still be helpful; sometimes even with a lock there
we still need to take care of when QEMUFile*==NULL, but then not needed to
care for rp thread.

> 
> >> 
> >> >> 
> >> >> Move this more complicated part of the code to a separate routine so
> >> >> we can wait on the thread without all of this baggage.
> >> >
> >> > I think you mentioned "some nuance" on having mark_source_rp_bad() in
> >> > await_return_path_close_on_source(), I did remember I tried to look into
> >> > that "nuance" too a long time ago but I just forgot what was that.  Great
> >> > if you can share some details.
> >> >
> >> 
> >> Well, mark_source_rp_bad() at await_return_path_close_on_source() is
> >> basically useless:
> >> 
> >> - We only call mark_source_rp_bad() if s->to_dst_file has an error and the
> >>   migration_completion() already checks that condition and fails the
> >>   migration anyway.
> >> 
> >> - If to_dst_file has an error, chances are the destination already did
> >>   cleanup by this point, so from_dst_file would already have an errno,
> >>   due to that. At qemu_fill_buffer(), the len == 0 case basically means
> >>   "the other end finished cleanly". We still set -EIO in that case, I
> >>   don't know why. Possibly because not all backends will have the same
> >>   semantics for len == 0.
> >
> > I don't know e

Re: [PATCH v7 00/14] linux-user: brk fixes

[Richard Henderson]

On 8/3/23 08:01, Helge Deller wrote:

If it does, it replaces patches 1,2 & 4-6 from Richard's v7 patch
series.


The patch you gave below has no overlap with 1,2,4,5 at all.


r~

Re: [PATCH v2 1/2] migration: Split await_return_path_close_on_source

[Daniel P . Berrangé]

On Thu, Aug 03, 2023 at 11:15:41AM -0400, Peter Xu wrote:
> On Thu, Aug 03, 2023 at 11:45:38AM -0300, Fabiano Rosas wrote:
> > Peter Xu  writes:
> > 
> > > On Wed, Aug 02, 2023 at 04:58:38PM -0300, Fabiano Rosas wrote:
> > >> Peter Xu  writes:
> > >> 
> > >> > On Wed, Aug 02, 2023 at 11:36:43AM -0300, Fabiano Rosas wrote:
> > >> >> This function currently has a straight-forward part which is waiting
> > >> >> for the thread to join and a complicated part which is doing a
> > >> >> qemu_file_shutdown() on the return path file.
> > >> >> 
> > >> >> The shutdown is tricky because all calls to qemu_file_shutdown() set
> > >> >> f->last_error to -EIO, which means we can never know if an error is an
> > >> >> actual error or if we cleanly shutdown the file previously.
> > >> >> 
> > >> >> This is particularly bothersome for postcopy because it would send the
> > >> >> return path thread into the retry routine which would wait on the
> > >> >> postcopy_pause_rp_sem and consequently block the main thread. We
> > >> >> haven't had reports of this so I must presume we never reach here with
> > >> >> postcopy.
> > >> >> 
> > >> >> The shutdown call is also racy because since it doesn't take the
> > >> >> qemu_file_lock, it could NULL-dereference if the return path thread
> > >> >> happens to be in the middle of the critical region at
> > >> >> migration_release_dst_files().
> > >> >
> > >> > After you rework the thread model on resume, shall we move
> > >> > migration_release_dst_files() into the migration thread to be after the
> > >> > pthread_join()?  I assume then we don't even need a mutex to protect 
> > >> > it?
> > >> >
> > >> 
> > >> I just need to figure out if it's ok to move the postcopy_qemufile_src
> > >> cleanup along. No idea why it is there in the first place. I see you
> > >> moved it from postcopy_pause and we're about to move it back to the
> > >> exact same place =D
> > >
> > > It was there because the old postcopy-preempt was sending data via
> > > postcopy_qemufile_src from the migration thread, while postcopy_pause is
> > > also the migration thread context.
> > >
> > > Then we had 9358982744 ("migration: Send requested page directly in
> > > rp-return thread") where we moved that "send page" operation into the
> > > return path thread to reduce latencies.  After moving there it also means
> > > the file handle can be accessed in >1 threads, so I just moved it over to
> > > operate that always in the return path thread, then no race should happen.
> > >
> > 
> > Thanks for the context.
> > 
> > > With your change, return path will vanish before migration thread accesses
> > > it later (so as mentioned above, it must be after pthread_join()
> > > succeeded), then I assume it'll be fine too to have it back in migration
> > > thread.
> > >
> > > Or perhaps just take the file lock?
> > >
> > 
> > There's also migrate_fd_cleanup and migrate_fd_cancel that can touch
> > these files. We might need to lock anyway, let's see.
> 
> The cancel path shouldn't clear the QEMUFile*, then I assume it's fine.
> That's based on the assumption that qemu_file_shutdown() is actually thread
> safe (say, shutdown() syscall is thread-safe for sockets).

The shutdown() syscall and qio_channel_shutdown() method are intended
to be safe to call from any thread *PROVIDED* you can ensure no other
thread is concurrently going to call close() on the FD (or unref the
QIOChannel object).

There is no locking in qemu_file_shutdown() to guarantee this, but
maybe something else in migration code is guaranteeing that the
QIOChannel object is not going to be closed (or unref'd), while a
thread is invoking qemu_file_shutdown().

IOW, in theory qemu_file_shutdown() could be safe to use but
I'm not seeing a clearly expressed guarantee of safety in the
code. If it is safe, the reasons are very subtle and rationale
ought to be documented in the comment for qemu_file_shutdown


With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH v2 0/2] block/blkio: fix fd leak and add more comments for the fd passing

[Stefan Hajnoczi]

On Thu, Aug 03, 2023 at 10:28:23AM +0200, Stefano Garzarella wrote:
> Hanna discovered an fd leak in the error path, and a few comments to
> improve in the code.
> 
> v2:
>   - avoid to use `fd_supported` to track a valid fd [Hanna]
> 
> v1: 
> https://lore.kernel.org/qemu-devel/20230801160332.122564-1-sgarz...@redhat.com/
> 
> Stefano Garzarella (2):
>   block/blkio: close the fd when blkio_connect() fails
>   block/blkio: add more comments on the fd passing handling
> 
>  block/blkio.c | 26 --
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> -- 
> 2.41.0
> 

Thanks, applied to my block tree:
https://gitlab.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

[RFC PATCH post-8.1] hw/xen: Clean up event channel 'type_val' handling to use union

[David Woodhouse]

From: David Woodhouse 

A previous implementation of this stuff used a 64-bit field for all of
the port information (vcpu/type/type_val) and did atomic exchanges on
them. When I implemented that in Qemu I regretted my life choices and
just kept it simple with locking instead.

So there's no need for the XenEvtchnPort to be so simplistic. We can
use a union for the pirq/virq/interdomain information, which lets us
keep a separate bit for the 'remote domain' in interdomain ports. A
single bit is enough since the only possible targets are loopback or
qemu itself.

So now we can ditch PORT_INFO_TYPEVAL_REMOTE_QEMU and the horrid
manual masking, although the in-memory representation is identical
so there's no change in the saved state ABI.

Signed-off-by: David Woodhouse 
---
Thought this would be a nice cleanup to avoid abusing `type_val` for
various different purposes, and especially the top bit of it for
interdomain ports. But having done it I find myself fairly ambivalent
about it. Does anyone feel strongly either way?

 hw/i386/kvm/xen_evtchn.c | 124 ---
 1 file changed, 64 insertions(+), 60 deletions(-)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index a731738411..446ae46022 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -58,7 +58,15 @@ OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
 typedef struct XenEvtchnPort {
 uint32_t vcpu;  /* Xen/ACPI vcpu_id */
 uint16_t type;  /* EVTCHNSTAT_ */
-uint16_t type_val;  /* pirq# / virq# / remote port according to type */
+union {
+uint16_t type_val;  /* pirq# / virq# / remote port according to type */
+uint16_t pirq;
+uint16_t virq;
+struct {
+uint16_t port:15;
+uint16_t to_qemu:1; /* Only two targets; qemu or loopback */
+} interdomain;
+} u;
 } XenEvtchnPort;
 
 /* 32-bit compatibility definitions, also used natively in 32-bit build */
@@ -210,16 +218,16 @@ static int xen_evtchn_post_load(void *opaque, int 
version_id)
 XenEvtchnPort *p = &s->port_table[i];
 
 if (p->type == EVTCHNSTAT_pirq) {
-assert(p->type_val);
-assert(p->type_val < s->nr_pirqs);
+assert(p->u.pirq);
+assert(p->u.pirq < s->nr_pirqs);
 
 /*
  * Set the gsi to IRQ_UNBOUND; it may be changed to an actual
  * GSI# below, or to IRQ_MSI_EMU when the MSI table snooping
  * catches up with it.
  */
-s->pirq[p->type_val].gsi = IRQ_UNBOUND;
-s->pirq[p->type_val].port = i;
+s->pirq[p->u.pirq].gsi = IRQ_UNBOUND;
+s->pirq[p->u.pirq].port = i;
 }
 }
 /* Rebuild s->pirq[].gsi mapping */
@@ -243,7 +251,7 @@ static const VMStateDescription xen_evtchn_port_vmstate = {
 .fields = (VMStateField[]) {
 VMSTATE_UINT32(vcpu, XenEvtchnPort),
 VMSTATE_UINT16(type, XenEvtchnPort),
-VMSTATE_UINT16(type_val, XenEvtchnPort),
+VMSTATE_UINT16(u.type_val, XenEvtchnPort),
 VMSTATE_END_OF_LIST()
 }
 };
@@ -599,14 +607,13 @@ static void unbind_backend_ports(XenEvtchnState *s)
 
 for (i = 1; i < s->nr_ports; i++) {
 p = &s->port_table[i];
-if (p->type == EVTCHNSTAT_interdomain &&
-(p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU)) {
-evtchn_port_t be_port = p->type_val & 
PORT_INFO_TYPEVAL_REMOTE_PORT_MASK;
+if (p->type == EVTCHNSTAT_interdomain && p->u.interdomain.to_qemu) {
+evtchn_port_t be_port = p->u.interdomain.port;
 
 if (s->be_handles[be_port]) {
 /* This part will be overwritten on the load anyway. */
 p->type = EVTCHNSTAT_unbound;
-p->type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU;
+p->u.interdomain.port = 0;
 
 /* Leave the backend port open and unbound too. */
 if (kvm_xen_has_cap(EVTCHN_SEND)) {
@@ -644,7 +651,7 @@ int xen_evtchn_status_op(struct evtchn_status *status)
 
 switch (p->type) {
 case EVTCHNSTAT_unbound:
-if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
+if (p->u.interdomain.to_qemu) {
 status->u.unbound.dom = DOMID_QEMU;
 } else {
 status->u.unbound.dom = xen_domid;
@@ -652,22 +659,21 @@ int xen_evtchn_status_op(struct evtchn_status *status)
 break;
 
 case EVTCHNSTAT_interdomain:
-if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
+if (p->u.interdomain.to_qemu) {
 status->u.interdomain.dom = DOMID_QEMU;
 } else {
 status->u.interdomain.dom = xen_domid;
 }
 
-status->u.interdomain.port = p->type_val &
-PORT_INFO_TYPEVAL_REMOTE_PORT_MASK;
+status->u.interdomain.port = p->u.interdomain.port;
 break;
 
 case EVTCHNSTAT_pirq:
-status->u.pirq = p->type_val;
+

Re: [PATCH v2 1/2] migration: Split await_return_path_close_on_source

[Peter Xu]

On Thu, Aug 03, 2023 at 04:24:16PM +0100, Daniel P. Berrangé wrote:
> On Thu, Aug 03, 2023 at 11:15:41AM -0400, Peter Xu wrote:
> > On Thu, Aug 03, 2023 at 11:45:38AM -0300, Fabiano Rosas wrote:
> > > Peter Xu  writes:
> > > 
> > > > On Wed, Aug 02, 2023 at 04:58:38PM -0300, Fabiano Rosas wrote:
> > > >> Peter Xu  writes:
> > > >> 
> > > >> > On Wed, Aug 02, 2023 at 11:36:43AM -0300, Fabiano Rosas wrote:
> > > >> >> This function currently has a straight-forward part which is waiting
> > > >> >> for the thread to join and a complicated part which is doing a
> > > >> >> qemu_file_shutdown() on the return path file.
> > > >> >> 
> > > >> >> The shutdown is tricky because all calls to qemu_file_shutdown() set
> > > >> >> f->last_error to -EIO, which means we can never know if an error is 
> > > >> >> an
> > > >> >> actual error or if we cleanly shutdown the file previously.
> > > >> >> 
> > > >> >> This is particularly bothersome for postcopy because it would send 
> > > >> >> the
> > > >> >> return path thread into the retry routine which would wait on the
> > > >> >> postcopy_pause_rp_sem and consequently block the main thread. We
> > > >> >> haven't had reports of this so I must presume we never reach here 
> > > >> >> with
> > > >> >> postcopy.
> > > >> >> 
> > > >> >> The shutdown call is also racy because since it doesn't take the
> > > >> >> qemu_file_lock, it could NULL-dereference if the return path thread
> > > >> >> happens to be in the middle of the critical region at
> > > >> >> migration_release_dst_files().
> > > >> >
> > > >> > After you rework the thread model on resume, shall we move
> > > >> > migration_release_dst_files() into the migration thread to be after 
> > > >> > the
> > > >> > pthread_join()?  I assume then we don't even need a mutex to protect 
> > > >> > it?
> > > >> >
> > > >> 
> > > >> I just need to figure out if it's ok to move the postcopy_qemufile_src
> > > >> cleanup along. No idea why it is there in the first place. I see you
> > > >> moved it from postcopy_pause and we're about to move it back to the
> > > >> exact same place =D
> > > >
> > > > It was there because the old postcopy-preempt was sending data via
> > > > postcopy_qemufile_src from the migration thread, while postcopy_pause is
> > > > also the migration thread context.
> > > >
> > > > Then we had 9358982744 ("migration: Send requested page directly in
> > > > rp-return thread") where we moved that "send page" operation into the
> > > > return path thread to reduce latencies.  After moving there it also 
> > > > means
> > > > the file handle can be accessed in >1 threads, so I just moved it over 
> > > > to
> > > > operate that always in the return path thread, then no race should 
> > > > happen.
> > > >
> > > 
> > > Thanks for the context.
> > > 
> > > > With your change, return path will vanish before migration thread 
> > > > accesses
> > > > it later (so as mentioned above, it must be after pthread_join()
> > > > succeeded), then I assume it'll be fine too to have it back in migration
> > > > thread.
> > > >
> > > > Or perhaps just take the file lock?
> > > >
> > > 
> > > There's also migrate_fd_cleanup and migrate_fd_cancel that can touch
> > > these files. We might need to lock anyway, let's see.
> > 
> > The cancel path shouldn't clear the QEMUFile*, then I assume it's fine.
> > That's based on the assumption that qemu_file_shutdown() is actually thread
> > safe (say, shutdown() syscall is thread-safe for sockets).
> 
> The shutdown() syscall and qio_channel_shutdown() method are intended
> to be safe to call from any thread *PROVIDED* you can ensure no other
> thread is concurrently going to call close() on the FD (or unref the
> QIOChannel object).
> 
> There is no locking in qemu_file_shutdown() to guarantee this, but
> maybe something else in migration code is guaranteeing that the
> QIOChannel object is not going to be closed (or unref'd), while a
> thread is invoking qemu_file_shutdown().

It should currently be guaranteed by the qemu_file_lock I think.

> 
> IOW, in theory qemu_file_shutdown() could be safe to use but
> I'm not seeing a clearly expressed guarantee of safety in the
> code. If it is safe, the reasons are very subtle and rationale
> ought to be documented in the comment for qemu_file_shutdown

I agree.

For now for this specific use case of the migration qemufile, we can simply
always take the mutex.

Thanks,

-- 
Peter Xu

Ping: Re: [PATCH v2] softmmu/physmem: try opening file readonly before failure in file_ram_open

[ThinerLogoer]

At 2023-07-28 18:45:20, "David Hildenbrand"  wrote:
>
>
>Whatever you prefer! If I resend the patch, I would keep you the author 
>and only add my Co-authored-by: Signed-off-by:.
>
>Just let me know.
>

Hello,

I wonder whether you have planned to resubmit the current patch anytime soon, 
or is it already
inside the patch queue?

---

Regards,

logoerthiner

Re: [PATCH 01/44] Split out common part of BCM283X classes

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:43, Sergey Kambalin  wrote:
>
> Signed-off-by: Sergey Kambalin 
> ---
>  hw/arm/bcm2836.c | 102 ++-
>  hw/arm/raspi.c   |   2 +-
>  include/hw/arm/bcm2836.h |  26 +-
>  3 files changed, 83 insertions(+), 47 deletions(-)

> @@ -230,11 +238,17 @@ static const TypeInfo bcm283x_types[] = {
>  #endif
>  }, {
>  .name   = TYPE_BCM283X,
> -.parent = TYPE_DEVICE,
> +.parent = TYPE_BCM283X_BASE,
>  .instance_size  = sizeof(BCM283XState),
> -.instance_init  = bcm2836_init,
> -.class_size = sizeof(BCM283XClass),
> -.class_init = bcm283x_class_init,
> +.instance_init  = bcm283x_init,
> +.abstract   = true,
> +}, {
> +.name   = TYPE_BCM283X_BASE,
> +.parent = TYPE_DEVICE,
> +.instance_size  = sizeof(BCM283XBaseState),
> +.instance_init  = bcm283x_base_init,
> +.class_size = sizeof(BCM283XBaseClass),
> +.class_init = bcm283x_base_class_init,
>  .abstract   = true,
>  }
>  };
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> +
> +struct BCM283XBaseClass {
> +/*< private >*/
> +DeviceClass parent_class;
> +/*< public >*/
> +const char *name;
> +const char *cpu_type;
> +unsigned core_count;
> +hwaddr peri_base; /* Peripheral base address seen by the CPU */
> +hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
> +int clusterid;
> +};
> +
> +struct BCM283XState {
> +/*< private >*/
> +BCM283XBaseState parent_obj;
> +/*< public >*/
>  BCM2835PeripheralState peripherals;
>  };
>

This gives us a slightly odd class hierarchy where we have
two "common between bcm SoCs" types:

   TYPE_BCM283X_BASE --> TYPE_BCM283X --> TYPE_BCM2835
 ||-> TYPE_BCM2836
 |\-> TYPE_BCM2837
 \-> TYPE_BCM2838

The only thing TYPE_BCM283X seems to be doing here that
TYPE_BCM283X_BASE is not is handling the BCM2835PeripheralState
object. Would it be clearer to keep the existing
class hierarchy where everything inherits from
TYPE_BCM283X, and accept a little code duplication for
the 3 subclasses that use the same BCM2835PeripheralState?
I'm not sure...

thanks
-- PMM

Re: [PATCH 02/44] Split out common part of peripherals

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:44, Sergey Kambalin  wrote:
>
> Signed-off-by: Sergey Kambalin 
> ---
>  hw/arm/bcm2835_peripherals.c | 198 +++
>  hw/arm/bcm2836.c |  24 ++--
>  include/hw/arm/bcm2835_peripherals.h |  29 +++-
>  include/hw/arm/bcm2836.h |   3 +-
>  4 files changed, 154 insertions(+), 100 deletions(-)
>
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index 0233038b95..4c0c0b1e7d 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -30,9 +30,9 @@
>  #define SEPARATE_DMA_IRQ_MAX 10
>  #define ORGATED_DMA_IRQ_COUNT 4
>
> -static void create_unimp(BCM2835PeripheralState *ps,
> - UnimplementedDeviceState *uds,
> - const char *name, hwaddr ofs, hwaddr size)
> +void create_unimp(RaspiPeripheralBaseState *ps,

Something has gone wrong with the naming here. This is
the SoC implementation, and its peripherals should be
the same whether the BCM2838 is being used on the raspi,
or on any other board. The type names and so on should
be SoC-specific, not board-specific.

thanks
-- PMM

[PATCH for-8.2 v2 0/2] migration: Add max-switchover-bandwidth parameter

[Peter Xu]

v2:
- Fix wordings, reindent qapi doc [Markus]
- Added a pre-requisite patch to dedup documents in qapi/migration
- Rename available-bandwidth to max-switchover-bandwidth [Dan]

This is the v2 series to add the new parameter to guide migration
switchover calculations.

For more information on the new parameter and why we need it, please read
commit message in patch 2.

Please have a look, thanks.

Peter Xu (2):
  qapi/migration: Deduplicate migration parameter field comments
  migration: Allow user to specify migration switchover bandwidth

 qapi/migration.json| 297 +++--
 migration/migration.h  |   2 +-
 migration/options.h|   1 +
 migration/migration-hmp-cmds.c |  14 ++
 migration/migration.c  |  19 ++-
 migration/options.c|  28 
 migration/trace-events |   2 +-
 7 files changed, 80 insertions(+), 283 deletions(-)

-- 
2.41.0

[PATCH for-8.2 v2 1/2] qapi/migration: Deduplicate migration parameter field comments

[Peter Xu]

We used to have three objects that have always the same list of parameters
and comments are always duplicated:

  - @MigrationParameter
  - @MigrationParameters
  - @MigrateSetParameters

Before we can deduplicate the code, it's fairly straightforward to
deduplicate the comments first, so for each time we add a new migration
parameter we don't need to copy the same paragraphs three times.

Make the @MigrationParameter the major source of truth, while leaving the
rest two to reference to it.

We do have a slight problem in the man/html pages generated, that for the
latter two objects we'll get a list of Members but with all of them saying
"Not documented":

   Members
   announce-initial: int (optional)
  Not documented

   announce-max: int (optional)
  Not documented

   announce-rounds: int (optional)
  Not documented

   [...]

Even though we'll have a reference there telling the reader to jump over to
read the @MigrationParameter sections instead, for example:

   MigrationParameters (Object)

   The object structure to represent a list of migration parameters.
   The optional members aren't actually optional.  For detailed
   explanation for each of the field, please refer to the documentation
   of MigrationParameter.

So hopefully that's not too bad.. and we can leave it for later to make it
even better.

Signed-off-by: Peter Xu 
---
 qapi/migration.json | 283 ++--
 1 file changed, 7 insertions(+), 276 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index 8843e74b59..bb798f87a5 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -854,142 +854,9 @@
 ##
 # @MigrateSetParameters:
 #
-# @announce-initial: Initial delay (in milliseconds) before sending
-# the first announce (Since 4.0)
-#
-# @announce-max: Maximum delay (in milliseconds) between packets in
-# the announcement (Since 4.0)
-#
-# @announce-rounds: Number of self-announce packets sent after
-# migration (Since 4.0)
-#
-# @announce-step: Increase in delay (in milliseconds) between
-# subsequent packets in the announcement (Since 4.0)
-#
-# @compress-level: compression level
-#
-# @compress-threads: compression thread count
-#
-# @compress-wait-thread: Controls behavior when all compression
-# threads are currently busy.  If true (default), wait for a free
-# compression thread to become available; otherwise, send the page
-# uncompressed.  (Since 3.1)
-#
-# @decompress-threads: decompression thread count
-#
-# @throttle-trigger-threshold: The ratio of bytes_dirty_period and
-# bytes_xfer_period to trigger throttling.  It is expressed as
-# percentage.  The default value is 50. (Since 5.0)
-#
-# @cpu-throttle-initial: Initial percentage of time guest cpus are
-# throttled when migration auto-converge is activated.  The
-# default value is 20. (Since 2.7)
-#
-# @cpu-throttle-increment: throttle percentage increase each time
-# auto-converge detects that migration is not making progress.
-# The default value is 10. (Since 2.7)
-#
-# @cpu-throttle-tailslow: Make CPU throttling slower at tail stage At
-# the tail stage of throttling, the Guest is very sensitive to CPU
-# percentage while the @cpu-throttle -increment is excessive
-# usually at tail stage.  If this parameter is true, we will
-# compute the ideal CPU percentage used by the Guest, which may
-# exactly make the dirty rate match the dirty rate threshold.
-# Then we will choose a smaller throttle increment between the one
-# specified by @cpu-throttle-increment and the one generated by
-# ideal CPU percentage.  Therefore, it is compatible to
-# traditional throttling, meanwhile the throttle increment won't
-# be excessive at tail stage.  The default value is false.  (Since
-# 5.1)
-#
-# @tls-creds: ID of the 'tls-creds' object that provides credentials
-# for establishing a TLS connection over the migration data
-# channel.  On the outgoing side of the migration, the credentials
-# must be for a 'client' endpoint, while for the incoming side the
-# credentials must be for a 'server' endpoint.  Setting this to a
-# non-empty string enables TLS for all migrations.  An empty
-# string means that QEMU will use plain text mode for migration,
-# rather than TLS (Since 2.9) Previously (since 2.7), this was
-# reported by omitting tls-creds instead.
-#
-# @tls-hostname: hostname of the target host for the migration.  This
-# is required when using x509 based TLS credentials and the
-# migration URI does not already include a hostname.  For example
-# if using fd: or exec: based migration, the hostname must be
-# provided so that the server's x509 certificate identity can be
-# validated.  (Since 2.7) An empty string means that QEMU will use
-# the hostname associated with the migration URI, if any.  (Since
-# 

[PATCH for-8.2 v2 2/2] migration: Allow user to specify migration switchover bandwidth

[Peter Xu]

Migration bandwidth is a very important value to live migration.  It's
because it's one of the major factors that we'll make decision on when to
switchover to destination in a precopy process.

This value is currently estimated by QEMU during the whole live migration
process by monitoring how fast we were sending the data.  This can be the
most accurate bandwidth if in the ideal world, where we're always feeding
unlimited data to the migration channel, and then it'll be limited to the
bandwidth that is available.

However in reality it may be very different, e.g., over a 10Gbps network we
can see query-migrate showing migration bandwidth of only a few tens of
MB/s just because there are plenty of other things the migration thread
might be doing.  For example, the migration thread can be busy scanning
zero pages, or it can be fetching dirty bitmap from other external dirty
sources (like vhost or KVM).  It means we may not be pushing data as much
as possible to migration channel, so the bandwidth estimated from "how many
data we sent in the channel" can be dramatically inaccurate sometimes,
e.g., that a few tens of MB/s even if 10Gbps available, and then the
decision to switchover will be further affected by this.

The migration may not even converge at all with the downtime specified,
with that wrong estimation of bandwidth.

The issue is QEMU itself may not be able to avoid those uncertainties on
measuing the real "available migration bandwidth".  At least not something
I can think of so far.

One way to fix this is when the user is fully aware of the available
bandwidth, then we can allow the user to help providing an accurate value.

For example, if the user has a dedicated channel of 10Gbps for migration
for this specific VM, the user can specify this bandwidth so QEMU can
always do the calculation based on this fact, trusting the user as long as
specified.

A new parameter "max-switchover-bandwidth" is introduced just for this. So
when the user specified this parameter, instead of trusting the estimated
value from QEMU itself (based on the QEMUFile send speed), let's trust the
user more by using this value to decide when to switchover, assuming that
we'll have such bandwidth available then.

When the user wants to have migration only use 5Gbps out of that 10Gbps,
one can set max-bandwidth to 5Gbps, along with max-switchover-bandwidth to
5Gbps so it'll never use over 5Gbps too (so the user can have the rest
5Gbps for other things).  So it can be useful even if the network is not
dedicated, but as long as the user can know a solid value.

This can resolve issues like "unconvergence migration" which is caused by
hilarious low "migration bandwidth" detected for whatever reason.

Reported-by: Zhiyi Guo 
Signed-off-by: Peter Xu 
---
 qapi/migration.json| 14 +-
 migration/migration.h  |  2 +-
 migration/options.h|  1 +
 migration/migration-hmp-cmds.c | 14 ++
 migration/migration.c  | 19 +++
 migration/options.c| 28 
 migration/trace-events |  2 +-
 7 files changed, 73 insertions(+), 7 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index bb798f87a5..6a04fb7d36 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -759,6 +759,16 @@
 # @max-bandwidth: to set maximum speed for migration.  maximum speed
 # in bytes per second.  (Since 2.8)
 #
+# @max-switchover-bandwidth: to set available bandwidth for migration.
+# By default, this value is zero, means the user is not aware of
+# the available bandwidth that can be used by QEMU migration, so
+# QEMU will estimate the bandwidth automatically.  This can be set
+# when the estimated value is not accurate, while the user is able
+# to guarantee such bandwidth is available for migration purpose
+# during the migration procedure.  When specified correctly, this
+# can make the switchover decision much more accurate, which will
+# also be based on the max downtime specified.  (Since 8.2)
+#
 # @downtime-limit: set maximum tolerated downtime for migration.
 # maximum downtime in milliseconds (Since 2.8)
 #
@@ -840,7 +850,7 @@
'cpu-throttle-initial', 'cpu-throttle-increment',
'cpu-throttle-tailslow',
'tls-creds', 'tls-hostname', 'tls-authz', 'max-bandwidth',
-   'downtime-limit',
+   'max-switchover-bandwidth', 'downtime-limit',
{ 'name': 'x-checkpoint-delay', 'features': [ 'unstable' ] },
'block-incremental',
'multifd-channels',
@@ -885,6 +895,7 @@
 '*tls-hostname': 'StrOrNull',
 '*tls-authz': 'StrOrNull',
 '*max-bandwidth': 'size',
+'*max-switchover-bandwidth': 'size',
 '*downtime-limit': 'uint64',
 '*x-checkpoint-delay': { 'type': 'uint32',
  'features': [ 'unstable' ] },
@@ -949,6 

[PULL for-8.1 1/2] block/blkio: close the fd when blkio_connect() fails

[Stefan Hajnoczi]

From: Stefano Garzarella 

libblkio drivers take ownership of `fd` only after a successful
blkio_connect(), so if it fails, we are still the owners.

Fixes: cad2ccc395 ("block/blkio: use qemu_open() to support fd passing for 
virtio-blk")
Suggested-by: Hanna Czenczek 
Signed-off-by: Stefano Garzarella 
Reviewed-by: Hanna Czenczek 
Message-id: 20230803082825.25293-2-sgarz...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/blkio.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 8e7ce42c79..baba2f0b67 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -678,7 +678,7 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 const char *path = qdict_get_try_str(options, "path");
 BDRVBlkioState *s = bs->opaque;
 bool fd_supported = false;
-int fd, ret;
+int fd = -1, ret;
 
 if (!path) {
 error_setg(errp, "missing 'path' option");
@@ -719,6 +719,7 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 if (ret < 0) {
 fd_supported = false;
 qemu_close(fd);
+fd = -1;
 }
 }
 }
@@ -733,14 +734,18 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 }
 
 ret = blkio_connect(s->blkio);
+if (ret < 0 && fd >= 0) {
+/* Failed to give the FD to libblkio, close it */
+qemu_close(fd);
+fd = -1;
+}
+
 /*
  * If the libblkio driver doesn't support the `fd` property, 
blkio_connect()
  * will fail with -EINVAL. So let's try calling blkio_connect() again by
  * directly setting `path`.
  */
 if (fd_supported && ret == -EINVAL) {
-qemu_close(fd);
-
 /*
  * We need to clear the `fd` property we set previously by setting
  * it to -1.
-- 
2.41.0

[PULL for-8.1 0/2] Block patches

[Stefan Hajnoczi]

The following changes since commit 9ba37026fcf6b7f3f096c0cca3e1e7307802486b:

  Update version for v8.1.0-rc2 release (2023-08-02 08:22:45 -0700)

are available in the Git repository at:

  https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 9b06d0d076271d76e5384d767ef94a676f0a9efd:

  block/blkio: add more comments on the fd passing handling (2023-08-03 
11:28:43 -0400)


Pull request

Fix for an fd leak in the blkio block driver.



Stefano Garzarella (2):
  block/blkio: close the fd when blkio_connect() fails
  block/blkio: add more comments on the fd passing handling

 block/blkio.c | 28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

-- 
2.41.0

[PULL for-8.1 2/2] block/blkio: add more comments on the fd passing handling

[Stefan Hajnoczi]

From: Stefano Garzarella 

As Hanna pointed out, it is not clear in the code why qemu_open()
can fail, and why blkio_set_int("fd") is not enough to discover
the `fd` property support.

Let's fix them by adding more details in the code comments.

Suggested-by: Hanna Czenczek 
Reviewed-by: Hanna Czenczek 
Signed-off-by: Stefano Garzarella 
Message-id: 20230803082825.25293-3-sgarz...@redhat.com
Signed-off-by: Stefan Hajnoczi 
---
 block/blkio.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index baba2f0b67..1dd495617c 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -713,6 +713,12 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
  */
 fd = qemu_open(path, O_RDWR, NULL);
 if (fd < 0) {
+/*
+ * qemu_open() can fail if the user specifies a path that is not
+ * a file or device, for example in the case of Unix Domain Socket
+ * for the virtio-blk-vhost-user driver. In such cases let's have
+ * libblkio open the path directly.
+ */
 fd_supported = false;
 } else {
 ret = blkio_set_int(s->blkio, "fd", fd);
@@ -741,9 +747,12 @@ static int blkio_virtio_blk_connect(BlockDriverState *bs, 
QDict *options,
 }
 
 /*
- * If the libblkio driver doesn't support the `fd` property, 
blkio_connect()
- * will fail with -EINVAL. So let's try calling blkio_connect() again by
- * directly setting `path`.
+ * Before https://gitlab.com/libblkio/libblkio/-/merge_requests/208
+ * (libblkio <= v1.3.0), setting the `fd` property is not enough to check
+ * whether the driver supports the `fd` property or not. In that case,
+ * blkio_connect() will fail with -EINVAL.
+ * So let's try calling blkio_connect() again by directly setting `path`
+ * to cover this scenario.
  */
 if (fd_supported && ret == -EINVAL) {
 /*
-- 
2.41.0

Re: [PATCH v9 0/4] hw/ufs: Add Universal Flash Storage (UFS) support

[Stefan Hajnoczi]

On Thu, Aug 03, 2023 at 08:48:22PM +0900, Jeuk Kim wrote:
> Dear Stefan,
> I'm really sorry, but could you please put this patch series
> instead of v8, which was previously merged into block-next?
> The fixes from v8 are below.
> Please let me know if you have any comments or issues.

I hope you have time to solve the remaining endianness issues, but that
can be done as a separate series.

Thanks, applied to my block-next tree:
https://gitlab.com/stefanha/qemu/commits/block-next

Stefan


signature.asc
Description: PGP signature

RE: [PATCH v2] Fix SEGFAULT on getting physical address of MMIO region.

[Mikhail Tyutin]

> > Apply save_iotlb_data() to io_readx() as well as to io_writex().
> 
> Could we have a bit more detail in the commit message about
> when you can get this segfault?
> 
> In particular, does this happen only if you're using plugins?
> 

I think so. It crashes on specific addresses when plugin calls 
qemu_plugin_hwaddr_phys_addr(). I haven't seen this crash without a plugin.

Re: [PATCH v7 00/14] linux-user: brk fixes

[Helge Deller]

On 8/3/23 17:11, Richard Henderson wrote:

On 8/3/23 08:01, Helge Deller wrote:

* Joel Stanley :

On Thu, 3 Aug 2023 at 13:55, Helge Deller  wrote:

143551 brk(NULL) = 0x0009b000
143551 brk(0x0009b8fc) = 0x0009b000


I think the problem is the brk with 9b000 here.
It's not 64k aligned (=pages size of your ppc64le).

Please try with this patch on top of Richard's series:


@@ -3229,7 +3208,8 @@ static void load_elf_image(const char *image_name, int 
image_fd,
   info->end_code = 0;
   info->start_data = -1;
   info->end_data = 0;
-    info->brk = .

change that to become:
  info->brk = HOST_PAGE_ALIGN(hiaddr);


That stopped the crashing, and the binaries seem to run fine. I tested
on two hosts: ppc64le (64K) and arm64 (16K).


Great!

That made re-read Akihiko's patch:

Author: Akihiko Odaki 
 linux-user: Do not align brk with host page size

 do_brk() minimizes calls into target_mmap() by aligning the address
 with host page size, which is potentially larger than the target page
 size. However, the current implementation of this optimization has two
 bugs:

 - The start of brk is rounded up with the host page size while brk
   advertises an address aligned with the target page size as the
   beginning of brk. This makes the beginning of brk unmapped.

this patch has wrong assumptions.

The start of brk always needs to be host page aligned.



There is a bunch of code in target_mmap that attempts to manage
adjacent guest pages that fall into the same host page.  Akihiko's
patch assumes that code actually works.  Which I think is entirely
reasonable.


Ok.


You can't move brk up like this either (without other adjustments to
the binary mapping), since that will leave a hole in the guest
address space, which can get filled with something else later, which
will definitely cause problems.


Ah, true. I have to admit that I didn't thought of that yet.
What is a possible solution to increase brk then (if you agree
that it should be increased here if necessary) ?
Call target_mmap() on the area from current brk to new brk?

Helge

Re: [PATCH v7 00/14] linux-user: brk fixes

[Helge Deller]

On 8/3/23 17:20, Richard Henderson wrote:

On 8/3/23 08:01, Helge Deller wrote:

If it does, it replaces patches 1,2 & 4-6 from Richard's v7 patch
series.


The patch you gave below has no overlap with 1,2,4,5 at all.


Yes, ignore this Your patch series is fine as-is
(the brk()-host-page-alignment issue is the only one left I think)

Helge

Re: [PATCH 01/44] Split out common part of BCM283X classes

[Peter Maydell]

On Thu, 3 Aug 2023 at 16:48, Peter Maydell  wrote:
>
> On Wed, 26 Jul 2023 at 14:43, Sergey Kambalin  wrote:
> >
> > Signed-off-by: Sergey Kambalin 
> > ---
> >  hw/arm/bcm2836.c | 102 ++-
> >  hw/arm/raspi.c   |   2 +-
> >  include/hw/arm/bcm2836.h |  26 +-
> >  3 files changed, 83 insertions(+), 47 deletions(-)
>
> > @@ -230,11 +238,17 @@ static const TypeInfo bcm283x_types[] = {
> >  #endif
> >  }, {
> >  .name   = TYPE_BCM283X,
> > -.parent = TYPE_DEVICE,
> > +.parent = TYPE_BCM283X_BASE,
> >  .instance_size  = sizeof(BCM283XState),
> > -.instance_init  = bcm2836_init,
> > -.class_size = sizeof(BCM283XClass),
> > -.class_init = bcm283x_class_init,
> > +.instance_init  = bcm283x_init,
> > +.abstract   = true,
> > +}, {
> > +.name   = TYPE_BCM283X_BASE,
> > +.parent = TYPE_DEVICE,
> > +.instance_size  = sizeof(BCM283XBaseState),
> > +.instance_init  = bcm283x_base_init,
> > +.class_size = sizeof(BCM283XBaseClass),
> > +.class_init = bcm283x_base_class_init,
> >  .abstract   = true,
> >  }
> >  };
> > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> > +
> > +struct BCM283XBaseClass {
> > +/*< private >*/
> > +DeviceClass parent_class;
> > +/*< public >*/
> > +const char *name;
> > +const char *cpu_type;
> > +unsigned core_count;
> > +hwaddr peri_base; /* Peripheral base address seen by the CPU */
> > +hwaddr ctrl_base; /* Interrupt controller and mailboxes etc. */
> > +int clusterid;
> > +};
> > +
> > +struct BCM283XState {
> > +/*< private >*/
> > +BCM283XBaseState parent_obj;
> > +/*< public >*/
> >  BCM2835PeripheralState peripherals;
> >  };
> >
>
> This gives us a slightly odd class hierarchy where we have
> two "common between bcm SoCs" types:
>
>TYPE_BCM283X_BASE --> TYPE_BCM283X --> TYPE_BCM2835
>  ||-> TYPE_BCM2836
>  |\-> TYPE_BCM2837
>  \-> TYPE_BCM2838
>
> The only thing TYPE_BCM283X seems to be doing here that
> TYPE_BCM283X_BASE is not is handling the BCM2835PeripheralState
> object. Would it be clearer to keep the existing
> class hierarchy where everything inherits from
> TYPE_BCM283X, and accept a little code duplication for
> the 3 subclasses that use the same BCM2835PeripheralState?
> I'm not sure...

Ah, looking at the later parts of the patchset I think I
see the issue -- because the board code wants to
embed the SoC object in the machine struct, not having
a common type for the BCM283[567] which is the same size
for all of them would mean that the board code would
also have to split rather than being common for those
SoC types. This is the downside of our "embed the structs"
style of board and SoC code, I guess.

So it's a bit swings-and-roundabouts, and since you've
written the code this way, let's go with doing it this
way. I can always come back and try a refactoring
later if it bothers me too much :-)

-- PMM

Re: [PATCH 01/44] Split out common part of BCM283X classes

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:43, Sergey Kambalin  wrote:
>
> Signed-off-by: Sergey Kambalin 
> ---
>  hw/arm/bcm2836.c | 102 ++-
>  hw/arm/raspi.c   |   2 +-
>  include/hw/arm/bcm2836.h |  26 +-
>  3 files changed, 83 insertions(+), 47 deletions(-)

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PULL 0/9] Fixes for 8.1-rc3

[Richard Henderson]

On 8/3/23 04:09, Thomas Huth wrote:

The following changes since commit fb695ae3fdfe34ce7bf2eaa4595d48ca809c8841:

   Merge tag 'pull-qapi-2023-08-02' of https://repo.or.cz/qemu/armbru into 
staging (2023-08-02 06:51:53 -0700)

are available in the Git repository at:

   https://gitlab.com/thuth/qemu.git tags/pull-request-2023-08-03

for you to fetch changes up to f54ba56dad0e9cea275e9802915a293f1a8c7d22:

   gitlab: disable FF_SCRIPT_SECTIONS on msys jobs (2023-08-03 13:04:48 +0200)


* Fix timeout problems in the MSYS Gitlab CI jobs
* Fix a problem when compiling with Clang on Windows


Nice: msys2-64bit finished in 63 minutes.

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/8.1 as 
appropriate.


r~

Re: [RFC PATCH 5/6] include/qemu/compiler: Fix problem with gcc_struct and Clang

[Daniel P . Berrangé]

On Mon, Jul 31, 2023 at 02:04:42PM +0200, Thomas Huth wrote:
> On 31/07/2023 11.32, Daniel P. Berrangé wrote:
> > On Mon, Jul 31, 2023 at 11:10:58AM +0200, Thomas Huth wrote:
> ...
> > > Or do you see another possibility how we could fix that timeout problem in
> > > the 64-bit MSYS2 job? Still switching to clang, but compiling with
> > > --extra-cflags="-Wno-unknown-attributes" maybe?
> > 
> > I was surprised to see that we're not using ccache in gitlab CI. It wouldn't
> > help the from-clean compile time, but thereafter it ought to help. I'm doing
> > some tests with that to see the impact.
> 
> I tried that two years ago, but the results were rather disappointing:
> 
>  https://lists.nongnu.org/archive/html/qemu-devel/2021-04/msg02189.html
> 
> ... but that was only with the Linux runners. Maybe it helps with the MSYS
> runners?

I've done some tests over the last few days and my results are
rather different.  The speed up is *spectacular* if the hit rate
is high.

eg, consider you are retriggering broadly the same pipeline over &
over with iterations of your patches. eg 99% of the QEMU code
probably doesn't change as you're just re-trying to fix some edge
case in 1 file, and thus you'll get near 100% hit rate.

With such a case I see:

The msys64 job can complete in 24 minutes, instead of 50-60 minutes

The build-system-fedora job can complete in 6 minutes instead of 20
minutes

The build-user-hexagon job can cmplete in 3 minutes instead of 6

Basically a 50% cut in time on all compile jobs I've looked at
so far.

NB, these are both shared runners, since the pipeline is in my
fork. I've no idea what the impact will be on the Azure private
runners for upstream.

Given the CI limitations these days, such a speedup is massively
beneficial for our contributors though.

The downside is storage for the cache. In theory gitlab has defined
a quota for storage per account. In practice they are still yet to
enforce that. If they do ever enforce it, we're likely doomed as
our container images alone will exceed it several times over, let
alone leave room for build logs, job caches, artifacts, etc. While
storage quota isn't enforced though, we might as well use ccache.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 04/44] Introduce BCM2838 SoC

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:52, Sergey Kambalin  wrote:
>
> Signed-off-by: Sergey Kambalin 
> ---
>  hw/arm/bcm2838.c | 110 +++
>  hw/arm/bcm2838_peripherals.c |  72 ++
>  hw/arm/meson.build   |   2 +
>  include/hw/arm/bcm2838.h |  26 +++
>  include/hw/arm/bcm2838_peripherals.h |  36 +
>  5 files changed, 246 insertions(+)
>  create mode 100644 hw/arm/bcm2838.c
>  create mode 100644 hw/arm/bcm2838_peripherals.c
>  create mode 100644 include/hw/arm/bcm2838.h
>  create mode 100644 include/hw/arm/bcm2838_peripherals.h
>
> diff --git a/hw/arm/bcm2838.c b/hw/arm/bcm2838.c
> new file mode 100644
> index 00..dd650c8148
> --- /dev/null
> +++ b/hw/arm/bcm2838.c
> @@ -0,0 +1,110 @@
> +/*
> + * BCM2838 SoC emulation
> + *
> + * Copyright (C) 2022 Ovchinnikov Vitalii 
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/module.h"
> +#include "hw/arm/raspi_platform.h"
> +#include "hw/sysbus.h"
> +#include "hw/arm/bcm2838.h"
> +#include "trace.h"
> +
> +struct BCM2838Class {
> +/*< private >*/
> +BCM283XBaseClass parent_class;
> +/*< public >*/
> +hwaddr peri_low_base; /* Lower peripheral base address seen by the CPU */
> +hwaddr gic_base; /* GIC base address inside ARM local peripherals region 
> */

Are these actually variable across different BCM2838 versions?
If not, don't bother making them configurable like this, just
have a #define of the address values and use it directly.
The BCM283[567] code only does this for peri_base and
ctrl_base so it can have one class that handles all three SoCs.

> +};

thanks
-- PMM