Re: [PATCH v1 1/2] hw/net:ftgmac100: fix coding style

[Cédric Le Goater]

On 6/19/24 12:01 PM, Jamin Lin wrote:

Fix coding style issues from checkpatch.pl

Test command:
./scripts/checkpatch.pl --no-tree -f hw/net/ftgmac100.c

Signed-off-by: Jamin Lin 



Reviewed-by: Cédric Le Goater 

Thanks,

C.



---
  hw/net/ftgmac100.c | 12 
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 74b6c3d9a7..25e4c0cd5b 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -238,7 +238,8 @@ typedef struct {
   */
  #define FTGMAC100_MAX_FRAME_SIZE9220
  
-/* Limits depending on the type of the frame

+/*
+ * Limits depending on the type of the frame
   *
   *   9216 for Jumbo frames (+ 4 for VLAN)
   *   1518 for other frames (+ 4 for VLAN)
@@ -533,8 +534,10 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t 
tx_ring,
  break;
  }
  
-/* record transmit flags as they are valid only on the first

- * segment */
+/*
+ * record transmit flags as they are valid only on the first
+ * segment
+ */
  if (bd.des0 & FTGMAC100_TXDES0_FTS) {
  flags = bd.des1;
  }
@@ -639,7 +642,8 @@ static bool ftgmac100_can_receive(NetClientState *nc)
   */
  static uint32_t ftgmac100_rxpoll(FTGMAC100State *s)
  {
-/* Polling times :
+/*
+ * Polling times :
   *
   * Speed  TIME_SEL=0TIME_SEL=1
   *

Re: [PATCH v2 2/2] tests/avocado: update firmware for sbsa-ref

[Philippe Mathieu-Daudé]

Hi Marcin,

On 20/6/24 08:00, Marcin Juszkiewicz wrote:

Update firmware to have graphics card memory fix from EDK2 commit
c1d1910be6e04a8b1a73090cf2881fb698947a6e:

 OvmfPkg/QemuVideoDxe: add feature PCD to remap framebuffer W/C

 Some platforms (such as SBSA-QEMU on recent builds of the emulator) only
 tolerate misaligned accesses to normal memory, and raise alignment
 faults on such accesses to device memory, which is the default for PCIe
 MMIO BARs.

 When emulating a PCIe graphics controller, the framebuffer is typically
 exposed via a MMIO BAR, while the disposition of the region is closer to
 memory (no side effects on reads or writes, except for the changing
 picture on the screen; direct random access to any pixel in the image).

 In order to permit the use of such controllers on platforms that only
 tolerate these types of accesses for normal memory, it is necessary to
 remap the memory. Use the DXE services to set the desired capabilities
 and attributes.

 Hide this behavior under a feature PCD so only platforms that really
 need it can enable it. (OVMF on x86 has no need for this)

With this fix enabled we can boot sbsa-ref with more than one cpu core.


To keep bisection working, don't we want this patch first, then the
previous one on top?


Signed-off-by: Marcin Juszkiewicz 
---
  tests/avocado/machine_aarch64_sbsaref.py | 14 +++---
  1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/avocado/machine_aarch64_sbsaref.py 
b/tests/avocado/machine_aarch64_sbsaref.py
index 136b495096..e920bbf08c 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -37,18 +37,18 @@ def fetch_firmware(self):
  
  Used components:
  
-- Trusted Firmware 2.11.0

-- Tianocore EDK2 stable202405
-- Tianocore EDK2-platforms commit 4bbd0ed
+- Trusted Firmware v2.11.0
+- Tianocore EDK2   4d4f569924
+- Tianocore EDK2-platforms 3f08401
  
  """
  
  # Secure BootRom (TF-A code)

  fs0_xz_url = (
  
"https://artifacts.codelinaro.org/artifactory/linaro-419-sbsa-ref/";
-"20240528-140808/edk2/SBSA_FLASH0.fd.xz"
+"20240619-148232/edk2/SBSA_FLASH0.fd.xz"
  )
-fs0_xz_hash = 
"fa6004900b67172914c908b78557fec4d36a5f784f4c3dd08f49adb75e1892a9"
+fs0_xz_hash = 
"0c954842a590988f526984de22e21ae0ab9cb351a0c99a8a58e928f0c7359cf7"
  tar_xz_path = self.fetch_asset(fs0_xz_url, asset_hash=fs0_xz_hash,
algorithm='sha256')
  archive.extract(tar_xz_path, self.workdir)
@@ -57,9 +57,9 @@ def fetch_firmware(self):
  # Non-secure rom (UEFI and EFI variables)
  fs1_xz_url = (
  
"https://artifacts.codelinaro.org/artifactory/linaro-419-sbsa-ref/";
-"20240528-140808/edk2/SBSA_FLASH1.fd.xz"
+"20240619-148232/edk2/SBSA_FLASH1.fd.xz"
  )
-fs1_xz_hash = 
"5f3747d4000bc416d9641e33ff4ac60c3cc8cb74ca51b6e932e58531c62eb6f7"
+fs1_xz_hash = 
"c6ec39374c4d79bb9e9cdeeb6db44732d90bb4a334cec92002b3f4b9cac4b5ee"
  tar_xz_path = self.fetch_asset(fs1_xz_url, asset_hash=fs1_xz_hash,
algorithm='sha256')
  archive.extract(tar_xz_path, self.workdir)

Re: [PATCH 22/32] hw/sd: Add emmc_cmd_SEND_EXT_CSD() handler

[Cédric Le Goater]

Hello

On 6/19/24 7:40 PM, Philippe Mathieu-Daudé wrote:

Hi,

On 3/7/23 15:24, Cédric Le Goater wrote:

The parameters mimick a real 4GB eMMC, but it can be set to various
sizes. Initially from Vincent Palatin 

Signed-off-by: Cédric Le Goater 
---
  hw/sd/sdmmc-internal.h |  97 
  include/hw/sd/sd.h |   1 +
  hw/sd/sd.c | 109 -
  3 files changed, 206 insertions(+), 1 deletion(-)


First pass review, this will take time...


+static void mmc_set_ext_csd(SDState *sd, uint64_t size)
+{
+    uint32_t sectcount = size >> HWBLOCK_SHIFT;
+
+    memset(sd->ext_csd, 0, sizeof(sd->ext_csd));
+
+    sd->ext_csd[EXT_CSD_S_CMD_SET] = 0x1; /* supported command sets */
+    sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features  */
+    sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background operations */
+    sd->ext_csd[241] = 0xA; /* 1st initialization time after partitioning */
+    sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
+    sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */


We do not support (and are not interested in) that. I'll use 0x0 for
"do not support".


+    sd->ext_csd[EXT_CSD_SEC_ERASE_MULT] = 0x96; /* Secure erase support */


This value is obsolete, so I'd use 0x0 to avoid confusions.


+    sd->ext_csd[EXT_CSD_SEC_TRIM_MULT] = 0x96; /* Secure TRIM multiplier */


Again, 0x0 for "not defined".


+    sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
+    sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 128KB unit */
+    sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */


16KB of super_page_size hmm. Simpler could be the underlying block
retrieved with bdrv_nb_sectors() or simply BDRV_SECTOR_SIZE (0x1).


+    sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */


2MB of erase size hmmm why not.


+    sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x1; /* HC erase timeout */


We don't implement timeout, can we use 0?


+    sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write sector count */
+    sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect group size 
*/
+    sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC  */
+    sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */
+    sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */
+    sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */
+    sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */
+    sd->ext_csd[213] = (sectcount >> 8) & 0xff;  /* ... */
+    sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff);   /* ... */
+    sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */
+    sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz  */
+    sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */
+    sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */
+    sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */
+    sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */


Class B at 3MB/s. I suppose announcing up to J at 21MB/s is safe (0x46).


+    sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1;


SWITCH command isn't implemented so far. We could use 0x0 for "not
defined".


+    sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1;


Similarly, 0x0 for "undefined" is legal.


+    sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7;


You anounce dual data rate. Could we just use High-Speed mode (0x3)
to ease modelling?


+    sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2;
+    sd->ext_csd[EXT_CSD_REV] = 0x5;


This is Revision 1.5 (for MMC v4.41)... The first QEMU implementation
was based on Revision 1.3 (for MMC v4.3) and I'm seeing some features
from Revision 1.6 (for MMC v4.5)...

Do we want to implement all of them? Since we are adding from
scratch, I suggest we directly start with v4.5 (0x6).

Note, EXT_CSD_BUS_WIDTH is not set (0x0) meaning 1-bit data bus.
I'd set it to 0x2 (8-bit):

    sd->ext_csd[EXT_CSD_BUS_WIDTH] = EXT_CSD_BUS_WIDTH_8_MASK;



I applied the proposed changes from above and the rainier-bmc boots fine.
Here are the mmc related logs :


  U-Boot SPL 2019.04 (Jun 17 2024 - 07:49:13 +)
  Trying to boot from MMC1
  
  
  U-Boot 2019.04 (Jun 17 2024 - 07:49:13 +)
  
  SOC: AST2600-A3

  eMMC 2nd Boot (ABR): Enable, boot partition: 1
  LPC Mode: SIO:Disable
  Eth: MAC0: RMII/NCSI, MAC1: RMII/NCSI, MAC2: RMII/NCSI, MAC3: RMII/NCSI
  Model: IBM P10 BMC
  DRAM:  already initialized, 896 MiB (capacity:1024 MiB, VGA:64 MiB, ECC:on, 
ECC size:896 MiB)
  MMC:   emmc_slot0@100: 0
  Loading Environment from MMC... OK
  In:serial@1e784000
  Out:   serial@1e784000
  Err:   serial@1e784000
  Model: IBM P10 BMC
  Net:   No MDIO found.
  ftgmac100_probe - NCSI detected
  
  ...
  
  [0.640650] mmc0: SDHCI controller on 1e750100.sdhci [1e750100.sdhci] using ADMA

  [0.658402] mmc0: unspecified timeout for CMD6 - use generic
  [0.659014] mmc0: unspecified timeout for CMD6 -

Re: [RFC PATCH v4 1/5] accel/tcg: Avoid unnecessary call overhead from qemu_plugin_vcpu_mem_cb

[Frank Chang]

Reviewed-by: Frank Chang 

Max Chou  於 2024年6月14日 週五 上午1:52寫道：
>
> If there are not any QEMU plugin memory callback functions, checking
> before calling the qemu_plugin_vcpu_mem_cb function can reduce the
> function call overhead.
>
> Signed-off-by: Max Chou 
> ---
>  accel/tcg/ldst_common.c.inc | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/accel/tcg/ldst_common.c.inc b/accel/tcg/ldst_common.c.inc
> index c82048e377e..87ceb954873 100644
> --- a/accel/tcg/ldst_common.c.inc
> +++ b/accel/tcg/ldst_common.c.inc
> @@ -125,7 +125,9 @@ void helper_st_i128(CPUArchState *env, uint64_t addr, 
> Int128 val, MemOpIdx oi)
>
>  static void plugin_load_cb(CPUArchState *env, abi_ptr addr, MemOpIdx oi)
>  {
> -qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_R);
> +if (cpu_plugin_mem_cbs_enabled(env_cpu(env))) {
> +qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_R);
> +}
>  }
>
>  uint8_t cpu_ldb_mmu(CPUArchState *env, abi_ptr addr, MemOpIdx oi, uintptr_t 
> ra)
> @@ -188,7 +190,9 @@ Int128 cpu_ld16_mmu(CPUArchState *env, abi_ptr addr,
>
>  static void plugin_store_cb(CPUArchState *env, abi_ptr addr, MemOpIdx oi)
>  {
> -qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_W);
> +if (cpu_plugin_mem_cbs_enabled(env_cpu(env))) {
> +qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_W);
> +}
>  }
>
>  void cpu_stb_mmu(CPUArchState *env, abi_ptr addr, uint8_t val,
> --
> 2.34.1
>
>

[PATCH v2] tests/qtest/fuzz/virtio_net_fuzz.c: fix virtio_net_fuzz_multi

[Dmitry Frolov]

The main loop is executed during flush_events(), where virtio error may occur.
This behavior is legit and should not produce any crash report.
But the test is waiting on used descriptors w/o a check, and, in case of error
fails with message: "assertion timer != NULL failed".
Thus, any invalid input data produces a meaningless crash report.
Debuging the problem, I found that in case of virtio error in the main loop,
dev->bus->get_status(dev) is 0 in most cases.
In rare cases VIRTIO_CONFIG_S_NEEDS_RESET bit is set.
So, checking only for VIRTIO_CONFIG_S_NEEDS_RESET bit is not enough.

Also, the second qvirtqueue_add() call with corresponding comment are redundant.

v1: https://patchew.org/QEMU/20240523102813.396750-2-fro...@swemel.ru/
v2: modified error-check & clean-up

Signed-off-by: Dmitry Frolov 
---
 tests/qtest/fuzz/virtio_net_fuzz.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/fuzz/virtio_net_fuzz.c 
b/tests/qtest/fuzz/virtio_net_fuzz.c
index e239875e3b..f62d2b9478 100644
--- a/tests/qtest/fuzz/virtio_net_fuzz.c
+++ b/tests/qtest/fuzz/virtio_net_fuzz.c
@@ -65,22 +65,21 @@ static void virtio_net_fuzz_multi(QTestState *s,
 } else {
 vqa.rx = 0;
 uint64_t req_addr = guest_alloc(t_alloc, vqa.length);
-/*
- * If checking used ring, ensure that the fuzzer doesn't trigger
- * trivial asserion failure on zero-zied buffer
- */
 qtest_memwrite(s, req_addr, Data, vqa.length);
 
-
 free_head = qvirtqueue_add(s, q, req_addr, vqa.length,
 vqa.write, vqa.next);
-qvirtqueue_add(s, q, req_addr, vqa.length, vqa.write , vqa.next);
 qvirtqueue_kick(s, dev, q, free_head);
 }
 
 /* Run the main loop */
 qtest_clock_step(s, 100);
 flush_events(s);
+/* Input led to a virtio_error */
+if (dev->bus->get_status(dev) & VIRTIO_CONFIG_S_NEEDS_RESET ||
+  !(dev->bus->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) {
+return;
+}
 
 /* Wait on used descriptors */
 if (check_used && !vqa.rx) {
@@ -92,10 +91,6 @@ static void virtio_net_fuzz_multi(QTestState *s,
  */
 while (!vqa.rx && q != net_if->queues[QVIRTIO_RX_VQ]) {
 uint32_t got_desc_idx;
-/* Input led to a virtio_error */
-if (dev->bus->get_status(dev) & VIRTIO_CONFIG_S_NEEDS_RESET) {
-break;
-}
 if (dev->bus->get_queue_isr_status(dev, q) &&
 qvirtqueue_get_buf(s, q, &got_desc_idx, NULL)) {
 g_assert_cmpint(got_desc_idx, ==, free_head);
@@ -107,6 +102,11 @@ static void virtio_net_fuzz_multi(QTestState *s,
 /* Run the main loop */
 qtest_clock_step(s, 100);
 flush_events(s);
+/* Input led to a virtio_error */
+if (dev->bus->get_status(dev) & VIRTIO_CONFIG_S_NEEDS_RESET ||
+  !(dev->bus->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) {
+return;
+}
 }
 }
 Data += vqa.length;
-- 
2.43.0

Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic

[Thomas Huth]

On 17/06/2024 01.44, Jared Rossi wrote:



On 6/7/24 1:57 AM, Thomas Huth wrote:

On 05/06/2024 16.48, Jared Rossi wrote:



diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c977a52b50..de3d1f0d5a 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -43,6 +43,7 @@ typedef unsigned long long u64;
  #include "iplb.h"
    /* start.s */
+extern char _start[];
  void disabled_wait(void) __attribute__ ((__noreturn__));
  void consume_sclp_int(void);
  void consume_io_int(void);
@@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
  static inline void panic(const char *string)
  {
  sclp_print(string);
+    if (load_next_iplb()) {
+    sclp_print("\nTrying next boot device...");
+    jump_to_IPL_code((long)_start);
+    }
+
  disabled_wait();
  }


Honestly, I am unsure whether this is a really cool idea or a very ugly 
hack ... but I think I tend towards the latter, sorry. Jumping back to 
the startup code might cause various problem, e.g. pre-initialized 
variables don't get their values reset, causing different behavior when 
the s390-ccw bios runs a function a second time this way. Thus this 
sounds very fragile. Could we please try to get things cleaned up 
correctly, so that functions return with error codes instead of 
panicking when we can continue with another boot device? Even if its 
more work right now, I think this will be much more maintainable in the 
future.


 Thomas



Thanks Thomas, I appreciate your insight.  Your hesitation is perfectly 
understandable as well.  My initial design was like you suggest, where 
the functions return instead of panic, but the issue I ran into is that 
netboot uses a separate image, which we jump in to at the start of IPL 
from a network device (see zipl_load() in pc-bios/s390-ccw/bootmap.c). I 
wasn't able to come up with a simple way to return to the main BIOS code 
if a netboot fails other than by jumping back.  So, it seems to me that 
netboot kind of throws a monkeywrench into the basic idea of reworking 
the panics into returns.


I'm open to suggestions on a better way to recover from a failed netboot, 
and it's certainly possible I've overlooked something, but as far as I 
can tell a jump is necessary in that particular case at least. Netboot 
could perhaps be handled as a special case where the jump back is 
permitted whereas other device types return, but I don't think that 
actually solves the main issue.


What are your thoughts on this?


Yes, I agree that jumping is currently required to get back from the 
netboot code. So if you could rework your patches in a way that limits the 
jumping to a failed netboot, that would be acceptable, I think.


Apart from that: We originally decided to put the netboot code into a 
separate binary since the required roms/SLOF module might not always have 
been checked out (it needed to be done manually), so we were not able to 
compile it in all cases. But nowadays, this is handled in a much nicer 
way, the submodule is automatically checked out once you compile the 
s390x-softmmu target and have a s390x compiler available, so I wonder 
whether we should maybe do the next step and integrate the netboot code 
into the main s390-ccw.img now? Anybody got an opinion on this?


 Thomas



Hi Thomas,

I would generally defer the decision about integrating the netboot code to 
someone with more insight than me, but for what it's worth, I am of the 
opinion that if we want to rework all of panics into returns, then it would 
make the most sense to also do the integration now so that we can avoid 
using jump altogether.  Unless I'm missing something simple, I don't think 
the panic/return conversion will be trivial, and actually I think it will be 
quite invasive since there are dozens of calls to panic and assert that will 
need to be changed.   It doesn't seem worthwhile to do all of these 
conversions in order to avoid using jump, but then still being exposed to 
possible problems caused by jumping due to netboot requiring it anyway.


Agreed, we should either do it right and merge the two binaries, or it does 
not make too much sense to only partly convert the code.


I can look into merging the two binaries, but it might also take some time. 
So for the time being, I'm fine if we include the panic-jumping hack for 
now, we can still then clean it up later.


 Thomas

Re: [PATCH 5/5] s390x: Enable and document boot device fallback on panic

[Thomas Huth]

On 17/06/2024 16.49, Christian Borntraeger wrote:



Am 05.06.24 um 15:37 schrieb Thomas Huth:

On 29/05/2024 17.43, jro...@linux.ibm.com wrote:

From: Jared Rossi 

On a panic during IPL (i.e. a device failed to boot) check for another 
device

to boot from, as indicated by the presence of an unused IPLB.

If an IPLB is successfully loaded, then jump to the start of BIOS, 
restarting

IPL using the updated IPLB.  Otherwise enter disabled wait.

Signed-off-by: Jared Rossi 
---
  docs/system/bootindex.rst | 7 ---
  docs/system/s390x/bootdevices.rst | 9 ++---
  pc-bios/s390-ccw/s390-ccw.h   | 6 ++
  3 files changed, 16 insertions(+), 6 deletions(-)


Could you please split the documentation changes into a separate patch in 
v2 ? ... I think that would be cleaner.



diff --git a/docs/system/bootindex.rst b/docs/system/bootindex.rst
index 8b057f812f..de597561bd 100644
--- a/docs/system/bootindex.rst
+++ b/docs/system/bootindex.rst
@@ -50,9 +50,10 @@ Limitations
  Some firmware has limitations on which devices can be considered for
  booting.  For instance, the PC BIOS boot specification allows only one
-disk to be bootable.  If boot from disk fails for some reason, the BIOS
-won't retry booting from other disk.  It can still try to boot from
-floppy or net, though.
+disk to be bootable, except for on s390x machines. If boot from disk 
fails for
+some reason, the BIOS won't retry booting from other disk.  It can still 
try to
+boot from floppy or net, though.  In the case of s390x, the BIOS will 
try up to

+8 total devices, any number of which may be disks.


Since the old text was already talking about "PC BIOS", I'd rather leave 
that paragraph as it is (maybe just replace "PC BIOS" with "x86 PC BIOS"), 
and add a separate paragraph afterwards about s390x instead.



diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c977a52b50..de3d1f0d5a 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -43,6 +43,7 @@ typedef unsigned long long u64;
  #include "iplb.h"
  /* start.s */
+extern char _start[];
  void disabled_wait(void) __attribute__ ((__noreturn__));
  void consume_sclp_int(void);
  void consume_io_int(void);
@@ -88,6 +89,11 @@ __attribute__ ((__noreturn__))
  static inline void panic(const char *string)
  {
  sclp_print(string);
+    if (load_next_iplb()) {
+    sclp_print("\nTrying next boot device...");
+    jump_to_IPL_code((long)_start);
+    }
+
  disabled_wait();
  }


Honestly, I am unsure whether this is a really cool idea or a very ugly 
hack ... but I think I tend towards the latter, sorry. Jumping back to the 
startup code might cause various problem, e.g. pre-initialized variables 
don't get their values reset, causing different behavior when the s390-ccw 
bios runs a function a second time this way. 


We jump back to _start and to me it looks like that this code does the 
resetting of bss segment.
So anything that has a zero value this should be fine. But static variables 
!= 0 are indeed tricky.

As far as I can see we do have some of those :-(

So instead of jumping, is there a way that remember somewhere at which 
device we are and then trigger a re-ipl to reload the BIOS?


If there is an easy way, this could maybe an option, but in the long run, 
I'd really prefer if we'd merge the binaries and get rid of such tricks, 
since this makes the code flow quite hard to understand and maybe also more 
difficult to debug if you run into problems later.


 Thomas

[PATCH] plugins/execlog.c: correct dump of registers values

[Frédéric Pétrot]

Register values are dumped as 'sz' chunks of two nibbles in the execlog
plugin, sz was 1 too big.

Signed-off-by: Frédéric Pétrot 
---
 contrib/plugins/execlog.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index 371db97eb1..1c1601cc0b 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -101,7 +101,7 @@ static void insn_check_regs(CPU *cpu)
 GByteArray *temp = reg->last;
 g_string_append_printf(cpu->last_exec, ", %s -> 0x", reg->name);
 /* TODO: handle BE properly */
-for (int i = sz; i >= 0; i--) {
+for (int i = sz - 1; i >= 0; i--) {
 g_string_append_printf(cpu->last_exec, "%02x",
reg->new->data[i]);
 }
-- 
2.39.2

Re: [PATCH v2 1/2] tests/avocado: use default amount of cores on sbsa-ref

[Peter Maydell]

On Thu, 20 Jun 2024 at 07:00, Marcin Juszkiewicz
 wrote:
>
> I was wondering why avocado tests passed with firmware which crashes
> when anyone else is using it.
>
> Turned out that amount of cores matters. Have to find out why still.

This commit message confuses me. It reads like "running with
two cores will make the guest crash", i.e. "apply this patch
and the test suite will stop passing". I assume that's not
the case, but what's actually going on here?

thanks
-- PMM

Re: [PATCH] hw/net: Fix Coverity Issue for npcm-gmac

[Peter Maydell]

On Wed, 19 Jun 2024 at 10:13, Alex Bennée  wrote:
>
> Nabih Estefan  writes:
>
> > There is an extra `buf=` set that is not used by npcm-gmac. Remove it
> > for coverity to be happy.

By the way, Nabih, it looks like the mailing list received five copies
of this patch email. You might want to look at what happened on your
end that resulted in all the duplicates.

> Have you go the coverity reference to include in the commit message?

This is CID 1534027.

> > Signed-off-by: Nabih Estefan 
> > ---
> >  hw/net/npcm_gmac.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
> > index 1b71e2526e..b397fd5064 100644
> > --- a/hw/net/npcm_gmac.c
> > +++ b/hw/net/npcm_gmac.c
> > @@ -614,7 +614,6 @@ static void gmac_try_send_next_packet(NPCMGMACState 
> > *gmac)
> >  net_checksum_calculate(tx_send_buffer, length, csum);
> >  qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, 
> > length);
> >  trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, 
> > length);
> > -buf = tx_send_buffer;
>
> So coverity is saying that buf starts at tx_send_buffer and none of the
> other legs that can mess with it are possible for the tx_desc.tdes1 &
> TX_DESC_TDES1_LAST_SEG_MASK leg?

Coverity is saying that in the loop body, we unconditionally
(in "step 4") set "buf = &tx_send_buffer[prev_buf_size]" before
we ever try to use "buf". This assignment "buf = tx_send_buffer"
happens later in the loop body, but there is no further reference
to buf either inside the loop body or after the loop ends. So we
will never look at the value we assign to "buf" here (either we
finish the loop and the function, or else we loop back around
again and overwrite this value), and this assignment is dead code.

What I'm wondering is whether this code for "last segment,
send the packet" should be setting "prev_buf_size = 0" instead
of "buf = tx_send_buffer" (meaning, I think "we've sent this packet,
there is nothing currently in the tx_send_buffer, the next descriptor
can start filling tx_send_buffer from byte 0".) Otherwise I think
we will continue to accumulate data from the following descriptor
into tx_send_buffer after the data from this packet, but when we
send that second packet we will do it from the start of
tx_send_buffer and so we will send the wrong data.

thanks
-- PMM

[PATCH 01/10] target/i386: use cpu_cc_dst for CC_OP_POPCNT

[Paolo Bonzini]

It is the only POPCNT that computes ZF from one of the cc_op_* registers,
but it uses cpu_cc_src instead of cpu_cc_dst like the others.  Do not
make it the odd one off.

Signed-off-by: Paolo Bonzini 
---
 target/i386/cpu.h   | 2 +-
 target/i386/tcg/cc_helper.c | 2 +-
 target/i386/tcg/translate.c | 2 +-
 target/i386/tcg/emit.c.inc  | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 7e2a9b56aea..f54cd93b3f9 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1332,7 +1332,7 @@ typedef enum {
 CC_OP_BMILGQ,
 
 CC_OP_CLR, /* Z set, all other flags clear.  */
-CC_OP_POPCNT, /* Z via CC_SRC, all other flags clear.  */
+CC_OP_POPCNT, /* Z via CC_DST, all other flags clear.  */
 
 CC_OP_NB,
 } CCOp;
diff --git a/target/i386/tcg/cc_helper.c b/target/i386/tcg/cc_helper.c
index f76e9cb8cfb..301ed954064 100644
--- a/target/i386/tcg/cc_helper.c
+++ b/target/i386/tcg/cc_helper.c
@@ -107,7 +107,7 @@ target_ulong helper_cc_compute_all(target_ulong dst, 
target_ulong src1,
 case CC_OP_CLR:
 return CC_Z | CC_P;
 case CC_OP_POPCNT:
-return src1 ? 0 : CC_Z;
+return dst ? 0 : CC_Z;
 
 case CC_OP_MULB:
 return compute_all_mulb(dst, src1);
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index ad1819815ab..f32cda4e169 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -324,7 +324,7 @@ static const uint8_t cc_op_live[CC_OP_NB] = {
 [CC_OP_ADOX] = USES_CC_SRC | USES_CC_SRC2,
 [CC_OP_ADCOX] = USES_CC_DST | USES_CC_SRC | USES_CC_SRC2,
 [CC_OP_CLR] = 0,
-[CC_OP_POPCNT] = USES_CC_SRC,
+[CC_OP_POPCNT] = USES_CC_DST,
 };
 
 static void set_cc_op_1(DisasContext *s, CCOp op, bool dirty)
diff --git a/target/i386/tcg/emit.c.inc b/target/i386/tcg/emit.c.inc
index 11faa70b5e2..fc7477833bc 100644
--- a/target/i386/tcg/emit.c.inc
+++ b/target/i386/tcg/emit.c.inc
@@ -2804,10 +2804,10 @@ static void gen_POPA(DisasContext *s, X86DecodedInsn 
*decode)
 
 static void gen_POPCNT(DisasContext *s, X86DecodedInsn *decode)
 {
-decode->cc_src = tcg_temp_new();
+decode->cc_dst = tcg_temp_new();
 decode->cc_op = CC_OP_POPCNT;
 
-tcg_gen_mov_tl(decode->cc_src, s->T0);
+tcg_gen_mov_tl(decode->cc_dst, s->T0);
 tcg_gen_ctpop_tl(s->T0, s->T0);
 }
 
-- 
2.45.2

[PATCH 06/10] target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder

[Paolo Bonzini]

This moves the last LOCK-enabled instructions to the new decoder.  It is now
possible to assume that PREFIX_LOCK gen_multi0F is called only after checking
that LOCK was not specified.

The gen_cmpxchg8b and gen_cmpxchg16b functions even have the correct
prototype already; the only thing that needs to be done is removing the
gen_lea_modrm() call.

Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/decode-new.h |   2 +
 target/i386/tcg/translate.c  | 121 +--
 target/i386/tcg/decode-new.c.inc |  34 ++---
 target/i386/tcg/emit.c.inc   |  96 
 4 files changed, 124 insertions(+), 129 deletions(-)

diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index bebc77bd54b..7f23d373ea7 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -114,6 +114,8 @@ typedef enum X86CPUIDFeature {
 X86_FEAT_CLWB,
 X86_FEAT_CMOV,
 X86_FEAT_CMPCCXADD,
+X86_FEAT_CX8,
+X86_FEAT_CX16,
 X86_FEAT_F16C,
 X86_FEAT_FMA,
 X86_FEAT_FSGSBASE,
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 1d845ff66bb..c60f18c7482 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2298,104 +2298,6 @@ static void gen_sty_env_A0(DisasContext *s, int offset, 
bool align)
 tcg_gen_qemu_st_i128(t, s->tmp0, mem_index, mop);
 }
 
-static void gen_cmpxchg8b(DisasContext *s, X86DecodedInsn *decode)
-{
-TCGv_i64 cmp, val, old;
-TCGv Z;
-
-gen_lea_modrm(s, decode);
-
-cmp = tcg_temp_new_i64();
-val = tcg_temp_new_i64();
-old = tcg_temp_new_i64();
-
-/* Construct the comparison values from the register pair. */
-tcg_gen_concat_tl_i64(cmp, cpu_regs[R_EAX], cpu_regs[R_EDX]);
-tcg_gen_concat_tl_i64(val, cpu_regs[R_EBX], cpu_regs[R_ECX]);
-
-/* Only require atomic with LOCK; non-parallel handled in generator. */
-if (s->prefix & PREFIX_LOCK) {
-tcg_gen_atomic_cmpxchg_i64(old, s->A0, cmp, val, s->mem_index, 
MO_TEUQ);
-} else {
-tcg_gen_nonatomic_cmpxchg_i64(old, s->A0, cmp, val,
-  s->mem_index, MO_TEUQ);
-}
-
-/* Set tmp0 to match the required value of Z. */
-tcg_gen_setcond_i64(TCG_COND_EQ, cmp, old, cmp);
-Z = tcg_temp_new();
-tcg_gen_trunc_i64_tl(Z, cmp);
-
-/*
- * Extract the result values for the register pair.
- * For 32-bit, we may do this unconditionally, because on success (Z=1),
- * the old value matches the previous value in EDX:EAX.  For x86_64,
- * the store must be conditional, because we must leave the source
- * registers unchanged on success, and zero-extend the writeback
- * on failure (Z=0).
- */
-if (TARGET_LONG_BITS == 32) {
-tcg_gen_extr_i64_tl(cpu_regs[R_EAX], cpu_regs[R_EDX], old);
-} else {
-TCGv zero = tcg_constant_tl(0);
-
-tcg_gen_extr_i64_tl(s->T0, s->T1, old);
-tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EAX], Z, zero,
-   s->T0, cpu_regs[R_EAX]);
-tcg_gen_movcond_tl(TCG_COND_EQ, cpu_regs[R_EDX], Z, zero,
-   s->T1, cpu_regs[R_EDX]);
-}
-
-/* Update Z. */
-gen_compute_eflags(s);
-tcg_gen_deposit_tl(cpu_cc_src, cpu_cc_src, Z, ctz32(CC_Z), 1);
-}
-
-#ifdef TARGET_X86_64
-static void gen_cmpxchg16b(DisasContext *s, X86DecodedInsn *decode)
-{
-MemOp mop = MO_TE | MO_128 | MO_ALIGN;
-TCGv_i64 t0, t1;
-TCGv_i128 cmp, val;
-
-gen_lea_modrm(s, decode);
-
-cmp = tcg_temp_new_i128();
-val = tcg_temp_new_i128();
-tcg_gen_concat_i64_i128(cmp, cpu_regs[R_EAX], cpu_regs[R_EDX]);
-tcg_gen_concat_i64_i128(val, cpu_regs[R_EBX], cpu_regs[R_ECX]);
-
-/* Only require atomic with LOCK; non-parallel handled in generator. */
-if (s->prefix & PREFIX_LOCK) {
-tcg_gen_atomic_cmpxchg_i128(val, s->A0, cmp, val, s->mem_index, mop);
-} else {
-tcg_gen_nonatomic_cmpxchg_i128(val, s->A0, cmp, val, s->mem_index, 
mop);
-}
-
-tcg_gen_extr_i128_i64(s->T0, s->T1, val);
-
-/* Determine success after the fact. */
-t0 = tcg_temp_new_i64();
-t1 = tcg_temp_new_i64();
-tcg_gen_xor_i64(t0, s->T0, cpu_regs[R_EAX]);
-tcg_gen_xor_i64(t1, s->T1, cpu_regs[R_EDX]);
-tcg_gen_or_i64(t0, t0, t1);
-
-/* Update Z. */
-gen_compute_eflags(s);
-tcg_gen_setcondi_i64(TCG_COND_EQ, t0, t0, 0);
-tcg_gen_deposit_tl(cpu_cc_src, cpu_cc_src, t0, ctz32(CC_Z), 1);
-
-/*
- * Extract the result values for the register pair.  We may do this
- * unconditionally, because on success (Z=1), the old value matches
- * the previous value in RDX:RAX.
- */
-tcg_gen_mov_i64(cpu_regs[R_EAX], s->T0);
-tcg_gen_mov_i64(cpu_regs[R_EDX], s->T1);
-}
-#endif
-
 #include "emit.c.inc"
 
 static void gen_x87(DisasContext *s, X86DecodedInsn *decode)
@@ -2971,29 +2873,10 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 
 

[PATCH 08/10] target/i386: list instructions still in translate.c

[Paolo Bonzini]

Group them so that it is easier to figure out which two-byte opcodes to
tackle together.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/decode-new.c.inc | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
index fa51aadfcf2..f01a4f1f1fe 100644
--- a/target/i386/tcg/decode-new.c.inc
+++ b/target/i386/tcg/decode-new.c.inc
@@ -129,6 +129,37 @@
  *
  *(^)  these are the two cases in which Intel and AMD disagree on the
  * primary exception class
+ *
+ * Instructions still in translate.c
+ * -
+ * Generation of TCG opcodes for almost all instructions is in emit.c.inc;
+ * this file interprets the prefixes and opcode bytes down to individual
+ * instruction mnemonics.  There is only a handful of opcodes still using
+ * a switch statement to decode modrm bits 3-5 and prefixes after decoding
+ * is complete; these are relics of the older x86 decoder and their code
+ * generation is performed in translate.c.
+ *
+ * These unconverted opcodes also perform their own effective address
+ * generation using the gen_lea_modrm() function.
+ *
+ * There is nothing particularly complicated about them; simply, they don't
+ * need any nasty hacks in the decoder, and they shouldn't get in the way
+ * of the implementation of new x86 instructions, so they are left alone
+ * for the time being.
+ *
+ * x87:
+ * 0xD8 - 0xDF
+ *
+ * privileged/system:
+ * 0x0F 0x00   group 6 (SLDT, STR, LLDT, LTR, VERR, VERW)
+ * 0x0F 0x01   group 7 (SGDT, SIDT, LGDT, LIDT, SMSW, LMSW, INVLPG,
+ *  MONITOR, MWAIT, CLAC, STAC, XGETBV, XSETBV,
+ *  SWAPGS, RDTSCP)
+ * 0x0F 0xC7 (reg operand) group 9 (RDRAND, RDSEED, RDPID)
+ *
+ * MPX:
+ * 0x0F 0x1A   BNDLDX, BNDMOV, BNDCL, BNDCU
+ * 0x0F 0x1B   BNDSTX, BNDMOV, BNDMK, BNDCN
  */
 
 #define X86_OP_NONE { 0 },
-- 
2.45.2

[PATCH 04/10] target/i386: try not to force EFLAGS computation for CC_OP_ADOX/ADCX

[Paolo Bonzini]

When computing the "other" flag (CF for CC_OP_ADOX, OF for CC_OP_ADCX),
take into account that it is already in the right position of cpu_cc_src,
just like for CC_OP_EFLAGS.  There is no need to call gen_compute_eflags().

Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/translate.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 257110ac703..08db40681fa 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -928,6 +928,7 @@ static CCPrepare gen_prepare_eflags_c(DisasContext *s, TCGv 
reg)
  .no_setcond = true };
 
 case CC_OP_EFLAGS:
+case CC_OP_ADOX:
 case CC_OP_SARB ... CC_OP_SARQ:
 /* CC_SRC & 1 */
 return (CCPrepare) { .cond = TCG_COND_TSTNE,
@@ -994,6 +995,9 @@ static CCPrepare gen_prepare_eflags_o(DisasContext *s, TCGv 
reg)
 return (CCPrepare) { .cond = TCG_COND_NE, .reg = cpu_cc_src };
 default:
 gen_compute_eflags(s);
+/* fallthrough */
+case CC_OP_EFLAGS:
+case CC_OP_ADCX:
 return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = cpu_cc_src,
  .imm = CC_O };
 }
-- 
2.45.2

[PATCH 03/10] target/i386: convert bit test instructions to new decoder

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/decode-new.h |   3 +
 target/i386/tcg/translate.c  | 147 +-
 target/i386/tcg/decode-new.c.inc |  40 ++---
 target/i386/tcg/emit.c.inc   | 149 ++-
 4 files changed, 181 insertions(+), 158 deletions(-)

diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index f9bf9a60411..e4cdf5e3c4f 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -190,6 +190,9 @@ typedef enum X86InsnSpecial {
 /* Always locked if it has a memory operand (XCHG) */
 X86_SPECIAL_Locked,
 
+/* Like HasLock, but also operand 2 provides bit displacement into memory. 
 */
+X86_SPECIAL_BitTest,
+
 /* Do not load effective address in s->A0 */
 X86_SPECIAL_NoLoadEA,
 
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 934c514e64f..257110ac703 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -708,11 +708,6 @@ static TCGv gen_ext_tl(TCGv dst, TCGv src, MemOp size, 
bool sign)
 return dst;
 }
 
-static void gen_exts(MemOp ot, TCGv reg)
-{
-gen_ext_tl(reg, reg, ot, true);
-}
-
 static void gen_op_j_ecx(DisasContext *s, TCGCond cond, TCGLabel *label1)
 {
 TCGv tmp = gen_ext_tl(NULL, cpu_regs[R_ECX], s->aflag, false);
@@ -2985,7 +2980,7 @@ static void disas_insn_old(DisasContext *s, CPUState 
*cpu, int b)
 int prefixes = s->prefix;
 MemOp dflag = s->dflag;
 MemOp ot;
-int modrm, reg, rm, mod, op, val;
+int modrm, reg, rm, mod, op;
 
 /* now check op code */
 switch (b) {
@@ -3051,146 +3046,6 @@ static void disas_insn_old(DisasContext *s, CPUState 
*cpu, int b)
 }
 break;
 
-//
-/* bit operations */
-case 0x1ba: /* bt/bts/btr/btc Gv, im */
-ot = dflag;
-modrm = x86_ldub_code(env, s);
-op = (modrm >> 3) & 7;
-mod = (modrm >> 6) & 3;
-rm = (modrm & 7) | REX_B(s);
-if (mod != 3) {
-s->rip_offset = 1;
-gen_lea_modrm(env, s, modrm);
-if (!(s->prefix & PREFIX_LOCK)) {
-gen_op_ld_v(s, ot, s->T0, s->A0);
-}
-} else {
-gen_op_mov_v_reg(s, ot, s->T0, rm);
-}
-/* load shift */
-val = x86_ldub_code(env, s);
-tcg_gen_movi_tl(s->T1, val);
-if (op < 4)
-goto unknown_op;
-op -= 4;
-goto bt_op;
-case 0x1a3: /* bt Gv, Ev */
-op = 0;
-goto do_btx;
-case 0x1ab: /* bts */
-op = 1;
-goto do_btx;
-case 0x1b3: /* btr */
-op = 2;
-goto do_btx;
-case 0x1bb: /* btc */
-op = 3;
-do_btx:
-ot = dflag;
-modrm = x86_ldub_code(env, s);
-reg = ((modrm >> 3) & 7) | REX_R(s);
-mod = (modrm >> 6) & 3;
-rm = (modrm & 7) | REX_B(s);
-gen_op_mov_v_reg(s, MO_32, s->T1, reg);
-if (mod != 3) {
-AddressParts a = gen_lea_modrm_0(env, s, modrm);
-/* specific case: we need to add a displacement */
-gen_exts(ot, s->T1);
-tcg_gen_sari_tl(s->tmp0, s->T1, 3 + ot);
-tcg_gen_shli_tl(s->tmp0, s->tmp0, ot);
-tcg_gen_add_tl(s->A0, gen_lea_modrm_1(s, a, false), s->tmp0);
-gen_lea_v_seg(s, s->A0, a.def_seg, s->override);
-if (!(s->prefix & PREFIX_LOCK)) {
-gen_op_ld_v(s, ot, s->T0, s->A0);
-}
-} else {
-gen_op_mov_v_reg(s, ot, s->T0, rm);
-}
-bt_op:
-tcg_gen_andi_tl(s->T1, s->T1, (1 << (3 + ot)) - 1);
-tcg_gen_movi_tl(s->tmp0, 1);
-tcg_gen_shl_tl(s->tmp0, s->tmp0, s->T1);
-if (s->prefix & PREFIX_LOCK) {
-switch (op) {
-case 0: /* bt */
-/* Needs no atomic ops; we suppressed the normal
-   memory load for LOCK above so do it now.  */
-gen_op_ld_v(s, ot, s->T0, s->A0);
-break;
-case 1: /* bts */
-tcg_gen_atomic_fetch_or_tl(s->T0, s->A0, s->tmp0,
-   s->mem_index, ot | MO_LE);
-break;
-case 2: /* btr */
-tcg_gen_not_tl(s->tmp0, s->tmp0);
-tcg_gen_atomic_fetch_and_tl(s->T0, s->A0, s->tmp0,
-s->mem_index, ot | MO_LE);
-break;
-default:
-case 3: /* btc */
-tcg_gen_atomic_fetch_xor_tl(s->T0, s->A0, s->tmp0,
-s->mem_index, ot | MO_LE);
-break;
-}
-tcg_gen_shr_tl(s->tmp4, s->T0, s->T1);
-} else {
-tcg_gen_shr_tl(s->tmp4, s->T0, s->T1);
-switch (op) {
-case 0: /* bt */
-/* Data already loaded; nothing to do.  */
-

[PATCH 09/10] target/i386: assert that cc_op* and pc_save are preserved

[Paolo Bonzini]

Now all decoding has been done before any code generation.
There is no need anymore to save and restore cc_op* and
pc_save but, for the time being, assert that this is indeed
the case.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/translate.c | 12 +++-
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 501a1ef9313..d11c5e1dc13 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3709,15 +3709,9 @@ static void i386_tr_translate_insn(DisasContextBase 
*dcbase, CPUState *cpu)
 case 2:
 /* Restore state that may affect the next instruction. */
 dc->pc = dc->base.pc_next;
-/*
- * TODO: These save/restore can be removed after the table-based
- * decoder is complete; we will be decoding the insn completely
- * before any code generation that might affect these variables.
- */
-dc->cc_op_dirty = orig_cc_op_dirty;
-dc->cc_op = orig_cc_op;
-dc->pc_save = orig_pc_save;
-/* END TODO */
+assert(dc->cc_op_dirty == orig_cc_op_dirty);
+assert(dc->cc_op == orig_cc_op);
+assert(dc->pc_save == orig_pc_save);
 dc->base.num_insns--;
 tcg_remove_ops_after(dc->prev_insn_end);
 dc->base.insn_start = dc->prev_insn_start;
-- 
2.45.2

Re: [PATCH 22/32] hw/sd: Add emmc_cmd_SEND_EXT_CSD() handler

[Philippe Mathieu-Daudé]

On 3/7/23 15:24, Cédric Le Goater wrote:

The parameters mimick a real 4GB eMMC, but it can be set to various
sizes. Initially from Vincent Palatin 

Signed-off-by: Cédric Le Goater 
---
  hw/sd/sdmmc-internal.h |  97 
  include/hw/sd/sd.h |   1 +
  hw/sd/sd.c | 109 -
  3 files changed, 206 insertions(+), 1 deletion(-)




diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 51e2254728a6..212658050441 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -141,6 +141,7 @@ struct SDState {
  uint64_t data_start;
  uint32_t data_offset;
  uint8_t data[512];
+uint8_t ext_csd[512];


Since the SWITCH command writes to EXT_CSD, this array must be
migrated.


  qemu_irq readonly_cb;
  qemu_irq inserted_cb;
  QEMUTimer *ocr_power_timer;
@@ -414,8 +415,85 @@ static const uint8_t sd_csd_rw_mask[16] = {
  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
  };
  
+static void mmc_set_ext_csd(SDState *sd, uint64_t size)

+{
+uint32_t sectcount = size >> HWBLOCK_SHIFT;
+
+memset(sd->ext_csd, 0, sizeof(sd->ext_csd));
+
+sd->ext_csd[EXT_CSD_S_CMD_SET] = 0x1; /* supported command sets */
+sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features  */
+sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background operations */
+sd->ext_csd[241] = 0xA; /* 1st initialization time after partitioning */
+sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
+sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure feature */
+sd->ext_csd[EXT_CSD_SEC_ERASE_MULT] = 0x96; /* Secure erase support */
+sd->ext_csd[EXT_CSD_SEC_TRIM_MULT] = 0x96; /* Secure TRIM multiplier */
+sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
+sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 128KB unit */
+sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */
+sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit size */
+sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x1; /* HC erase timeout */
+sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write sector count */
+sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect group size 
*/
+sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC  */
+sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */
+sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */
+sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */
+sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */
+sd->ext_csd[213] = (sectcount >> 8) & 0xff;  /* ... */
+sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff);   /* ... */
+sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */
+sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz  */
+sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */
+sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */
+sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */
+sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */
+sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1;
+sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1;
+sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7;
+sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2;
+sd->ext_csd[EXT_CSD_REV] = 0x5;
+sd->ext_csd[EXT_CSD_RPMB_MULT] = 0x1; /* RPMB size */
+sd->ext_csd[EXT_CSD_PARTITION_SUPPORT] = 0x3;
+sd->ext_csd[159] = 0x00; /* Max enhanced area size */
+sd->ext_csd[158] = 0x00; /* ... */
+sd->ext_csd[157] = 0xEC; /* ... */
+}

[PATCH 10/10] target/i386: remove gen_ext_tl

[Paolo Bonzini]

With the introduction of tcg_gen_ext_tl, most uses can be converted directly
because they do not have a NULL destination.  tcg_gen_ext_tl is able to drop
no-ops like "tcg_gen_ext_tl(tcgv, tcgv, MO_TL)" just fine, and the only thing
that gen_ext_tl was adding on top was avoiding the creation of a useless
temporary.  This can be done in the only place where it matters, which is
gen_op_j_ecx.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/translate.c | 41 +++--
 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index d11c5e1dc13..5c9c992400e 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -697,23 +697,16 @@ static inline TCGv gen_compute_Dshift(DisasContext *s, 
MemOp ot)
 return dshift;
 };
 
-static TCGv gen_ext_tl(TCGv dst, TCGv src, MemOp size, bool sign)
-{
-if (size == MO_TL) {
-return src;
-}
-if (!dst) {
-dst = tcg_temp_new();
-}
-tcg_gen_ext_tl(dst, src, size | (sign ? MO_SIGN : 0));
-return dst;
-}
-
 static void gen_op_j_ecx(DisasContext *s, TCGCond cond, TCGLabel *label1)
 {
-TCGv tmp = gen_ext_tl(NULL, cpu_regs[R_ECX], s->aflag, false);
-
-tcg_gen_brcondi_tl(cond, tmp, 0, label1);
+TCGv lhs;
+if (s->aflag == MO_TL) {
+lhs = cpu_regs[R_ECX];
+} else {
+lhs = tcg_temp_new();
+tcg_gen_ext_tl(lhs, cpu_regs[R_ECX], s->aflag);
+}
+tcg_gen_brcondi_tl(cond, lhs, 0, label1);
 }
 
 static inline void gen_op_jz_ecx(DisasContext *s, TCGLabel *label1)
@@ -886,16 +879,16 @@ static CCPrepare gen_prepare_eflags_c(DisasContext *s, 
TCGv reg)
 case CC_OP_SUBB ... CC_OP_SUBQ:
 /* (DATA_TYPE)CC_SRCT < (DATA_TYPE)CC_SRC */
 size = s->cc_op - CC_OP_SUBB;
-gen_ext_tl(s->cc_srcT, s->cc_srcT, size, false);
-gen_ext_tl(cpu_cc_src, cpu_cc_src, size, false);
+tcg_gen_ext_tl(s->cc_srcT, s->cc_srcT, size);
+tcg_gen_ext_tl(cpu_cc_src, cpu_cc_src, size);
 return (CCPrepare) { .cond = TCG_COND_LTU, .reg = s->cc_srcT,
  .reg2 = cpu_cc_src, .use_reg2 = true };
 
 case CC_OP_ADDB ... CC_OP_ADDQ:
 /* (DATA_TYPE)CC_DST < (DATA_TYPE)CC_SRC */
 size = s->cc_op - CC_OP_ADDB;
-gen_ext_tl(cpu_cc_dst, cpu_cc_dst, size, false);
-gen_ext_tl(cpu_cc_src, cpu_cc_src, size, false);
+tcg_gen_ext_tl(cpu_cc_dst, cpu_cc_dst, size);
+tcg_gen_ext_tl(cpu_cc_src, cpu_cc_src, size);
 return (CCPrepare) { .cond = TCG_COND_LTU, .reg = cpu_cc_dst,
  .reg2 = cpu_cc_src, .use_reg2 = true };
 
@@ -920,7 +913,7 @@ static CCPrepare gen_prepare_eflags_c(DisasContext *s, TCGv 
reg)
 
 case CC_OP_BMILGB ... CC_OP_BMILGQ:
 size = s->cc_op - CC_OP_BMILGB;
-gen_ext_tl(cpu_cc_src, cpu_cc_src, size, false);
+tcg_gen_ext_tl(cpu_cc_src, cpu_cc_src, size);
 return (CCPrepare) { .cond = TCG_COND_EQ, .reg = cpu_cc_src };
 
 case CC_OP_ADCX:
@@ -1050,8 +1043,8 @@ static CCPrepare gen_prepare_cc(DisasContext *s, int b, 
TCGv reg)
 size = s->cc_op - CC_OP_SUBB;
 switch (jcc_op) {
 case JCC_BE:
-gen_ext_tl(s->cc_srcT, s->cc_srcT, size, false);
-gen_ext_tl(cpu_cc_src, cpu_cc_src, size, false);
+tcg_gen_ext_tl(s->cc_srcT, s->cc_srcT, size);
+tcg_gen_ext_tl(cpu_cc_src, cpu_cc_src, size);
 cc = (CCPrepare) { .cond = TCG_COND_LEU, .reg = s->cc_srcT,
.reg2 = cpu_cc_src, .use_reg2 = true };
 break;
@@ -1061,8 +1054,8 @@ static CCPrepare gen_prepare_cc(DisasContext *s, int b, 
TCGv reg)
 case JCC_LE:
 cond = TCG_COND_LE;
 fast_jcc_l:
-gen_ext_tl(s->cc_srcT, s->cc_srcT, size, true);
-gen_ext_tl(cpu_cc_src, cpu_cc_src, size, true);
+tcg_gen_ext_tl(s->cc_srcT, s->cc_srcT, size | MO_SIGN);
+tcg_gen_ext_tl(cpu_cc_src, cpu_cc_src, size | MO_SIGN);
 cc = (CCPrepare) { .cond = cond, .reg = s->cc_srcT,
.reg2 = cpu_cc_src, .use_reg2 = true };
 break;
-- 
2.45.2

[PATCH 05/10] target/i386: decode address before going back to translate.c

[Paolo Bonzini]

There are now relatively few unconverted opcodes in translate.c (there
are 13 of them including 8 for x87), and all of them have the same
format with a mod/rm byte and no immediate.  A good next step is
to remove the early bail out to disas_insn_x87/disas_insn_old,
instead giving these legacy translator functions the same prototype
as the other gen_* functions.

To do this, the X86DecodeInsn can be passed down to the places that
used to fetch address bytes from the instruction stream.  To make
sure that everything is done cleanly, the CPUX86State* argument is
removed.

As part of the unification, the gen_lea_modrm() name is now free,
so rename gen_load_ea() to gen_lea_modrm().  This is as good a name
and it makes the changes to translate.c easier to review.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/decode-new.h |  14 ++-
 target/i386/tcg/translate.c  | 152 +--
 target/i386/tcg/decode-new.c.inc |  53 ++-
 target/i386/tcg/emit.c.inc   |   2 +-
 4 files changed, 103 insertions(+), 118 deletions(-)

diff --git a/target/i386/tcg/decode-new.h b/target/i386/tcg/decode-new.h
index e4cdf5e3c4f..bebc77bd54b 100644
--- a/target/i386/tcg/decode-new.h
+++ b/target/i386/tcg/decode-new.h
@@ -264,12 +264,13 @@ typedef enum X86VEXSpecial {
 
 typedef struct X86OpEntry  X86OpEntry;
 typedef struct X86DecodedInsn X86DecodedInsn;
+struct DisasContext;
 
 /* Decode function for multibyte opcodes.  */
-typedef void (*X86DecodeFunc)(DisasContext *s, CPUX86State *env, X86OpEntry 
*entry, uint8_t *b);
+typedef void (*X86DecodeFunc)(struct DisasContext *s, CPUX86State *env, 
X86OpEntry *entry, uint8_t *b);
 
 /* Code generation function.  */
-typedef void (*X86GenFunc)(DisasContext *s, X86DecodedInsn *decode);
+typedef void (*X86GenFunc)(struct DisasContext *s, X86DecodedInsn *decode);
 
 struct X86OpEntry {
 /* Based on the is_decode flags.  */
@@ -316,6 +317,14 @@ typedef struct X86DecodedOp {
 };
 } X86DecodedOp;
 
+typedef struct AddressParts {
+int def_seg;
+int base;
+int index;
+int scale;
+target_long disp;
+} AddressParts;
+
 struct X86DecodedInsn {
 X86OpEntry e;
 X86DecodedOp op[3];
@@ -333,3 +342,4 @@ struct X86DecodedInsn {
 uint8_t b;
 };
 
+static void gen_lea_modrm(struct DisasContext *s, X86DecodedInsn *decode);
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 08db40681fa..1d845ff66bb 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -29,6 +29,7 @@
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
 #include "helper-tcg.h"
+#include "decode-new.h"
 
 #include "exec/log.h"
 
@@ -1529,14 +1530,6 @@ static inline uint64_t x86_ldq_code(CPUX86State *env, 
DisasContext *s)
 
 /* Decompose an address.  */
 
-typedef struct AddressParts {
-int def_seg;
-int base;
-int index;
-int scale;
-target_long disp;
-} AddressParts;
-
 static AddressParts gen_lea_modrm_0(CPUX86State *env, DisasContext *s,
 int modrm)
 {
@@ -1695,24 +1688,11 @@ static TCGv gen_lea_modrm_1(DisasContext *s, 
AddressParts a, bool is_vsib)
 return ea;
 }
 
-static void gen_lea_modrm(CPUX86State *env, DisasContext *s, int modrm)
-{
-AddressParts a = gen_lea_modrm_0(env, s, modrm);
-TCGv ea = gen_lea_modrm_1(s, a, false);
-gen_lea_v_seg(s, ea, a.def_seg, s->override);
-}
-
-static void gen_nop_modrm(CPUX86State *env, DisasContext *s, int modrm)
-{
-(void)gen_lea_modrm_0(env, s, modrm);
-}
-
 /* Used for BNDCL, BNDCU, BNDCN.  */
-static void gen_bndck(CPUX86State *env, DisasContext *s, int modrm,
+static void gen_bndck(DisasContext *s, X86DecodedInsn *decode,
   TCGCond cond, TCGv_i64 bndv)
 {
-AddressParts a = gen_lea_modrm_0(env, s, modrm);
-TCGv ea = gen_lea_modrm_1(s, a, false);
+TCGv ea = gen_lea_modrm_1(s, decode->mem, false);
 
 tcg_gen_extu_tl_i64(s->tmp1_i64, ea);
 if (!CODE64(s)) {
@@ -1724,8 +1704,9 @@ static void gen_bndck(CPUX86State *env, DisasContext *s, 
int modrm,
 }
 
 /* generate modrm load of memory or register. */
-static void gen_ld_modrm(CPUX86State *env, DisasContext *s, int modrm, MemOp 
ot)
+static void gen_ld_modrm(DisasContext *s, X86DecodedInsn *decode, MemOp ot)
 {
+int modrm = s->modrm;
 int mod, rm;
 
 mod = (modrm >> 6) & 3;
@@ -1733,14 +1714,15 @@ static void gen_ld_modrm(CPUX86State *env, DisasContext 
*s, int modrm, MemOp ot)
 if (mod == 3) {
 gen_op_mov_v_reg(s, ot, s->T0, rm);
 } else {
-gen_lea_modrm(env, s, modrm);
+gen_lea_modrm(s, decode);
 gen_op_ld_v(s, ot, s->T0, s->A0);
 }
 }
 
 /* generate modrm store of memory or register. */
-static void gen_st_modrm(CPUX86State *env, DisasContext *s, int modrm, MemOp 
ot)
+static void gen_st_modrm(DisasContext *s, X86DecodedInsn *decode, MemOp ot)
 {
+int modrm = s->modrm;
 int mod, rm;
 
 mod = (modrm

Re: [PATCH v2 1/2] tests/avocado: use default amount of cores on sbsa-ref

[Marcin Juszkiewicz]

W dniu 20.06.2024 o 11:34, Peter Maydell pisze:
On Thu, 20 Jun 2024 at 07:00, Marcin Juszkiewicz 
 wrote:


I was wondering why avocado tests passed with firmware which
crashes when anyone else is using it.

Turned out that amount of cores matters. Have to find out why
still.


This commit message confuses me.


Had no idea how to write in more readable form. Will reword it for v3 
(with reverse order of patches as recommended by Philippe.



It reads like "running with two cores will make the guest crash",
i.e. "apply this patch and the test suite will stop passing". I
assume that's not the case, but what's actually going on here?


That's exactly the case. With sbsa-ref firmware which qemu uses now we 
have crash if more than 1 core is used. Avocado test hardcoded "-smp 1" 
and was passing fine.


And I forgot to mail qemu-devel when I got hit by that crash.

This week Rebecca Cran pointed me that crash is in BootLogoLib in EDK2 
and I wrote some workaround for make things work. Then Ard Biesheuvel 
found the real reason, fixed QemuVideoDxe in EDK2 and we got sbsa-ref 
running with any amount of cores.


The commit message of fix:

commit c1d1910be6e04a8b1a73090cf2881fb698947a6e
Author: Ard Biesheuvel 
Date:   Mon Jun 17 17:07:41 2024 +0200

OvmfPkg/QemuVideoDxe: add feature PCD to remap framebuffer W/C

Some platforms (such as SBSA-QEMU on recent builds of the emulator) only
tolerate misaligned accesses to normal memory, and raise alignment
faults on such accesses to device memory, which is the default for PCIe
MMIO BARs.

When emulating a PCIe graphics controller, the framebuffer is typically
exposed via a MMIO BAR, while the disposition of the region is closer to
memory (no side effects on reads or writes, except for the changing
picture on the screen; direct random access to any pixel in the image).

In order to permit the use of such controllers on platforms that only
tolerate these types of accesses for normal memory, it is necessary to
remap the memory. Use the DXE services to set the desired capabilities
and attributes.

Hide this behavior under a feature PCD so only platforms that really
need it can enable it. (OVMF on x86 has no need for this)

[PATCH 02/10] target/i386: give CC_OP_POPCNT low bits corresponding to MO_TL

[Paolo Bonzini]

Handle it like the other arithmetic cc_ops.  This simplifies a
bit the implementation of bit test instructions.

Signed-off-by: Paolo Bonzini 
---
 target/i386/cpu.h   | 13 +++--
 target/i386/tcg/translate.c |  3 +--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f54cd93b3f9..8504a7998fd 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1275,6 +1275,7 @@ typedef enum {
 CC_OP_ADCX, /* CC_DST = C, CC_SRC = rest.  */
 CC_OP_ADOX, /* CC_SRC2 = O, CC_SRC = rest.  */
 CC_OP_ADCOX, /* CC_DST = C, CC_SRC2 = O, CC_SRC = rest.  */
+CC_OP_CLR, /* Z and P set, all other flags clear.  */
 
 CC_OP_MULB, /* modify all flags, C, O = (CC_SRC != 0) */
 CC_OP_MULW,
@@ -1331,8 +1332,16 @@ typedef enum {
 CC_OP_BMILGL,
 CC_OP_BMILGQ,
 
-CC_OP_CLR, /* Z set, all other flags clear.  */
-CC_OP_POPCNT, /* Z via CC_DST, all other flags clear.  */
+/*
+ * Note that only CC_OP_POPCNT (i.e. the one with MO_TL size)
+ * is used or implemented, because the translation needs
+ * to zero-extend CC_DST anyway.
+ */
+CC_OP_POPCNTB__, /* Z via CC_DST, all other flags clear.  */
+CC_OP_POPCNTW__,
+CC_OP_POPCNTL__,
+CC_OP_POPCNTQ__,
+CC_OP_POPCNT = sizeof(target_ulong) == 8 ? CC_OP_POPCNTQ__ : 
CC_OP_POPCNTL__,
 
 CC_OP_NB,
 } CCOp;
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index f32cda4e169..934c514e64f 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1019,8 +1019,6 @@ static CCPrepare gen_prepare_eflags_z(DisasContext *s, 
TCGv reg)
  .imm = CC_Z };
 case CC_OP_CLR:
 return (CCPrepare) { .cond = TCG_COND_ALWAYS };
-case CC_OP_POPCNT:
-return (CCPrepare) { .cond = TCG_COND_EQ, .reg = cpu_cc_src };
 default:
 {
 MemOp size = (s->cc_op - CC_OP_ADDB) & 3;
@@ -3177,6 +3175,7 @@ static void disas_insn_old(DisasContext *s, CPUState 
*cpu, int b)
 case CC_OP_SHLB ... CC_OP_SHLQ:
 case CC_OP_SARB ... CC_OP_SARQ:
 case CC_OP_BMILGB ... CC_OP_BMILGQ:
+case CC_OP_POPCNT:
 /* Z was going to be computed from the non-zero status of CC_DST.
We can get that same Z value (and the new C value) by leaving
CC_DST alone, setting CC_SRC, and using a CC_OP_SAR of the
-- 
2.45.2

[PATCH 07/10] target/i386: do not check PREFIX_LOCK in old-style decoder

[Paolo Bonzini]

It is already checked before getting there.

Reviewed-by: Richard Henderson 
Signed-off-by: Paolo Bonzini 
---
 target/i386/tcg/translate.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index c60f18c7482..501a1ef9313 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2878,7 +2878,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 switch ((modrm >> 3) & 7) {
 case 7:
 if (mod != 3 ||
-(s->prefix & (PREFIX_LOCK | PREFIX_REPNZ))) {
+(s->prefix & PREFIX_REPNZ)) {
 goto illegal_op;
 }
 if (s->prefix & PREFIX_REPZ) {
@@ -2898,7 +2898,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 
 case 6: /* RDRAND */
 if (mod != 3 ||
-(s->prefix & (PREFIX_LOCK | PREFIX_REPZ | PREFIX_REPNZ)) ||
+(s->prefix & (PREFIX_REPZ | PREFIX_REPNZ)) ||
 !(s->cpuid_ext_features & CPUID_EXT_RDRAND)) {
 goto illegal_op;
 }
@@ -3058,8 +3058,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 
 case 0xd0: /* xgetbv */
 if ((s->cpuid_ext_features & CPUID_EXT_XSAVE) == 0
-|| (s->prefix & (PREFIX_LOCK | PREFIX_DATA
- | PREFIX_REPZ | PREFIX_REPNZ))) {
+|| (s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ))) {
 goto illegal_op;
 }
 tcg_gen_trunc_tl_i32(s->tmp2_i32, cpu_regs[R_ECX]);
@@ -3069,8 +3068,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 
 case 0xd1: /* xsetbv */
 if ((s->cpuid_ext_features & CPUID_EXT_XSAVE) == 0
-|| (s->prefix & (PREFIX_LOCK | PREFIX_DATA
- | PREFIX_REPZ | PREFIX_REPNZ))) {
+|| (s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ))) {
 goto illegal_op;
 }
 gen_svm_check_intercept(s, SVM_EXIT_XSETBV);
@@ -3237,8 +3235,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 gen_st_modrm(s, decode, ot);
 break;
 case 0xee: /* rdpkru */
-if (s->prefix & (PREFIX_LOCK | PREFIX_DATA
- | PREFIX_REPZ | PREFIX_REPNZ)) {
+if (s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ)) {
 goto illegal_op;
 }
 tcg_gen_trunc_tl_i32(s->tmp2_i32, cpu_regs[R_ECX]);
@@ -3246,8 +3243,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 tcg_gen_extr_i64_tl(cpu_regs[R_EAX], cpu_regs[R_EDX], s->tmp1_i64);
 break;
 case 0xef: /* wrpkru */
-if (s->prefix & (PREFIX_LOCK | PREFIX_DATA
- | PREFIX_REPZ | PREFIX_REPNZ)) {
+if (s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ)) {
 goto illegal_op;
 }
 tcg_gen_concat_tl_i64(s->tmp1_i64, cpu_regs[R_EAX],
@@ -3323,7 +3319,6 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 if (prefixes & PREFIX_REPZ) {
 /* bndcl */
 if (reg >= 4
-|| (prefixes & PREFIX_LOCK)
 || s->aflag == MO_16) {
 goto illegal_op;
 }
@@ -3331,7 +3326,6 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 } else if (prefixes & PREFIX_REPNZ) {
 /* bndcu */
 if (reg >= 4
-|| (prefixes & PREFIX_LOCK)
 || s->aflag == MO_16) {
 goto illegal_op;
 }
@@ -3345,7 +3339,7 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 }
 if (mod == 3) {
 int reg2 = (modrm & 7) | REX_B(s);
-if (reg2 >= 4 || (prefixes & PREFIX_LOCK)) {
+if (reg2 >= 4) {
 goto illegal_op;
 }
 if (s->flags & HF_MPX_IU_MASK) {
@@ -3374,7 +3368,6 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 /* bndldx */
 AddressParts a = decode->mem;
 if (reg >= 4
-|| (prefixes & PREFIX_LOCK)
 || s->aflag == MO_16
 || a.base < -1) {
 goto illegal_op;
@@ -3410,7 +3403,6 @@ static void gen_multi0F(DisasContext *s, X86DecodedInsn 
*decode)
 if (mod != 3 && (prefixes & PREFIX_REPZ)) {
 /* bndmk */
 if (reg >= 4
-|| (prefixes & PREFIX_LOCK)
 || s->aflag == MO_16) {
 

[PATCH 00/10] target/i386: make decoding entirely table based

[Paolo Bonzini]

The trailing part of the previous series I sent; with fixes for
BT/BTS/BTR/BTC, plus moving code generation of CMPXCHG8B/CMPXCHG16B to
the new decoder.  This way all LOCKable instructions are converted, and
the patch "target/i386: do not check PREFIX_LOCK in old-style decoder"
is correct.

Sneak in a couple cleanups for CC_OP_POPCNT.  They don't really make
the generated code any more efficient, but they simplify a bit the
logic for the BT/BTS/BTR/BTC flags.

Supersedes: <20240608084113.2770363-1-pbonz...@redhat.com>


Paolo Bonzini (10):
  target/i386: use cpu_cc_dst for CC_OP_POPCNT
  target/i386: give CC_OP_POPCNT low bits corresponding to MO_TL
  target/i386: convert bit test instructions to new decoder
  target/i386: try not to force EFLAGS computation for CC_OP_ADOX/ADCX
  target/i386: decode address before going back to translate.c
  target/i386: convert CMPXCHG8B/CMPXCHG16B to new decoder
  target/i386: do not check PREFIX_LOCK in old-style decoder
  target/i386: list instructions still in translate.c
  target/i386: assert that cc_op* and pc_save are preserved
  target/i386: remove gen_ext_tl

 target/i386/cpu.h|  13 +-
 target/i386/tcg/decode-new.h |  19 +-
 target/i386/tcg/cc_helper.c  |   2 +-
 target/i386/tcg/translate.c  | 492 ++-
 target/i386/tcg/decode-new.c.inc | 136 ++---
 target/i386/tcg/emit.c.inc   | 249 +++-
 6 files changed, 467 insertions(+), 444 deletions(-)

-- 
2.45.2

Re: [PATCH v2 1/2] tests/avocado: use default amount of cores on sbsa-ref

[Peter Maydell]

On Thu, 20 Jun 2024 at 10:55, Marcin Juszkiewicz
 wrote:
>
> W dniu 20.06.2024 o 11:34, Peter Maydell pisze:
> > On Thu, 20 Jun 2024 at 07:00, Marcin Juszkiewicz
> >  wrote:
> >>
> >> I was wondering why avocado tests passed with firmware which
> >> crashes when anyone else is using it.
> >>
> >> Turned out that amount of cores matters. Have to find out why
> >> still.
> >
> > This commit message confuses me.
>
> Had no idea how to write in more readable form. Will reword it for v3
> (with reverse order of patches as recommended by Philippe.
>
> > It reads like "running with two cores will make the guest crash",
> > i.e. "apply this patch and the test suite will stop passing". I
> > assume that's not the case, but what's actually going on here?
>
> That's exactly the case. With sbsa-ref firmware which qemu uses now we
> have crash if more than 1 core is used. Avocado test hardcoded "-smp 1"
> and was passing fine.
>
> And I forgot to mail qemu-devel when I got hit by that crash.
>
> This week Rebecca Cran pointed me that crash is in BootLogoLib in EDK2
> and I wrote some workaround for make things work. Then Ard Biesheuvel
> found the real reason, fixed QemuVideoDxe in EDK2 and we got sbsa-ref
> running with any amount of cores.

Oh, OK, so it's just random bad luck that enabling the second
CPU means that we end up doing an unaligned access to the
framebuffer, I guess.

Then, yes, Philippe is right and we need to update our sbsa-ref
firmware we're using for the test first, to avoid breaking bisection.

For a commit message for this patch, maybe something like:

 The version of the sbsa-ref EDK2 firmware we used to use in this
 test had a bug where it might make an unaligned access to the
 framebuffer, which causes a guest crash on newer versions of
 QEMU where we enforce the architectural requirement that
 unaligned accesses to Device memory should take an exception.
 We happened to not notice this because our test was booting with
 "-smp 1" and through luck this didn't write the boot logo to
 the framebuffer at an unaligned address; but trying to boot the
 same firmware with two CPUs would result in a guest crash.
 Now we have updated the firmware we're using for the test, we can
 make the test use all the cores on the board, so we are testing the
 SMP boot path.

?

thanks
-- PMM

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

[Peter Maydell]

On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé  wrote:
>
> On 18/6/24 16:40, Zheyu Ma wrote:
> > This commit updates the a9_gtimer_get_current_cpu() function to handle
> > cases where QTest is enabled. When QTest is used, it returns 0 instead
> > of dereferencing the current_cpu, which can be NULL. This prevents the
> > program from crashing during QTest runs.
> >
> > Reproducer:
> > cat << EOF | qemu-system-aarch64 -display \
> > none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> > writel 0xf03fe20c 0x26d7468c
> > EOF
> >
> > Signed-off-by: Zheyu Ma 
> > ---
> >   hw/timer/a9gtimer.c | 5 +
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> > index a2ac5bdfb9..64d80cdf6a 100644
> > --- a/hw/timer/a9gtimer.c
> > +++ b/hw/timer/a9gtimer.c
> > @@ -32,6 +32,7 @@
> >   #include "qemu/log.h"
> >   #include "qemu/module.h"
> >   #include "hw/core/cpu.h"
> > +#include "sysemu/qtest.h"
> >
> >   #ifndef A9_GTIMER_ERR_DEBUG
> >   #define A9_GTIMER_ERR_DEBUG 0
> > @@ -48,6 +49,10 @@
> >
> >   static inline int a9_gtimer_get_current_cpu(A9GTimerState *s)
> >   {
> > +if (qtest_enabled()) {
> > +return 0;
>
> Indeed this is how we fixed hw/intc/arm_gic in commit 09bbdb89bc,
> so:
>
> Reviewed-by: Philippe Mathieu-Daudé 
>
> > +}
> > +
> >   if (current_cpu->cpu_index >= s->num_cpu) {
>
> That said, such accesses of @current_cpu from hw/ are dubious.

True, but I'm not sure we ever settled on the right way to avoid
them, did we?

Anyway, I've applied this patch to target-arm.next.

-- PMM

Re: [PATCH] hw/usb/hcd-dwc2: Handle invalid address access in read and write functions

[Peter Maydell]

On Tue, 18 Jun 2024 at 22:33, Paul Zimmerman  wrote:
>
> On Tue, Jun 18, 2024 at 1:37 PM Philippe Mathieu-Daudé  
> wrote:
>>
>> Hi Paul,
>>
>> On 18/6/24 20:58, Paul Zimmerman wrote:
>> > On Tue, Jun 18, 2024 at 6:56 AM Zheyu Ma > > > wrote:
>> >  >
>> >  > This commit modifies the dwc2_hsotg_read() and dwc2_hsotg_write()
>> > functions
>> >  > to handle invalid address access gracefully. Instead of using
>> >  > g_assert_not_reached(), which causes the program to abort, the functions
>> >  > now log an error message and return a default value for reads or do
>> >  > nothing for writes.
>> >  >
>> >  > This change prevents the program from aborting and provides clear log
>> >  > messages indicating when an invalid memory address is accessed.
>> >  >
>> >  > Reproducer:
>> >  > cat << EOF | qemu-system-aarch64 -display none \
>> >  > -machine accel=qtest, -m 512M -machine raspi2b -m 1G -nodefaults \
>> >  > -usb -drive file=null-co://,if=none,format=raw,id=disk0 -device \
>> >  > usb-storage,port=1,drive=disk0 -qtest stdio
>> >  > readl 0x3f980dfb
>> >  > EOF
>> >  >
>> >  > Signed-off-by: Zheyu Ma > > >
>> >  > ---
>> >  >  hw/usb/hcd-dwc2.c | 9 +++--
>> >  >  1 file changed, 7 insertions(+), 2 deletions(-)
>> >  >
>> >  > diff --git a/hw/usb/hcd-dwc2.c b/hw/usb/hcd-dwc2.c
>> >  > index 8cac9c0a06..b4f0652c7d 100644
>> >  > --- a/hw/usb/hcd-dwc2.c
>> >  > +++ b/hw/usb/hcd-dwc2.c
>> >  > @@ -1128,7 +1128,10 @@ static uint64_t dwc2_hsotg_read(void *ptr, 
>> > hwaddr addr, unsigned size)
>> >  >  val = dwc2_pcgreg_read(ptr, addr, (addr - HSOTG_REG(0xe00)) >> 
>> > 2, size);
>> >  >  break;
>> >  >  default:
>> >  > -g_assert_not_reached();
>> >  > +qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 
>> > 0x%"HWADDR_PRIx"\n",
>> >  > +  __func__, addr);
>> >  > +val = 0;
>> >  > +break;
>> >  >  }
>> >  >
>> >  >  return val;
>> >  > @@ -1160,7 +1163,9 @@ static void dwc2_hsotg_write(void *ptr, hwaddr 
>> > addr, uint64_t val,
>> >  >  dwc2_pcgreg_write(ptr, addr, (addr - HSOTG_REG(0xe00)) >> 2, 
>> > val, size);
>> >  >  break;
>> >  >  default:
>> >  > -g_assert_not_reached();
>> >  > +qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 
>> > 0x%"HWADDR_PRIx"\n",
>> >  > +  __func__, addr);
>> >  > +break;
>> >  >  }
>> >  >  }
>> >  >
>> >  > --
>> >  > 2.34.1
>> >
>> > Looks good to me.
>> >
>> > Reviewed-by: Paul Zimmerman > > >
>> >
>>
>> Does that mean on real HW the access to unassigned registers are
>> silently ignored as RAZ/WI like this patch? (I don't have access
>> to the specs -- IIRC you don't neither, but you might have real
>> HW to test).

> I have an old raspi around somewhere I could probably dig up and
> test with, but I'm not familiar with qtest, so I don't know how I
> would reproduce the failure on real hw.
>
> Besides, isn't it always better to fail and log an error than just crash?

Yes, assert is definitely the wrong thing here. RAZ/WI and log a
guest-error is what we typically do for devices where the spec doesn't
give a behaviour for accesses to register offsets that aren't documented
as having registers.

I've applied this to target-arm.next; thanks.

-- PMM

[PATCH v3 1/2] tests/avocado: update firmware for sbsa-ref

[Marcin Juszkiewicz]

Update firmware to have graphics card memory fix from EDK2 commit
c1d1910be6e04a8b1a73090cf2881fb698947a6e:

OvmfPkg/QemuVideoDxe: add feature PCD to remap framebuffer W/C

Some platforms (such as SBSA-QEMU on recent builds of the emulator) only
tolerate misaligned accesses to normal memory, and raise alignment
faults on such accesses to device memory, which is the default for PCIe
MMIO BARs.

When emulating a PCIe graphics controller, the framebuffer is typically
exposed via a MMIO BAR, while the disposition of the region is closer to
memory (no side effects on reads or writes, except for the changing
picture on the screen; direct random access to any pixel in the image).

In order to permit the use of such controllers on platforms that only
tolerate these types of accesses for normal memory, it is necessary to
remap the memory. Use the DXE services to set the desired capabilities
and attributes.

Hide this behavior under a feature PCD so only platforms that really
need it can enable it. (OVMF on x86 has no need for this)

With this fix enabled we can boot sbsa-ref with more than one cpu core.

Signed-off-by: Marcin Juszkiewicz 
---
 tests/avocado/machine_aarch64_sbsaref.py | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/avocado/machine_aarch64_sbsaref.py 
b/tests/avocado/machine_aarch64_sbsaref.py
index 6bb82f2a03..e854ec6a1a 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -37,18 +37,18 @@ def fetch_firmware(self):
 
 Used components:
 
-- Trusted Firmware 2.11.0
-- Tianocore EDK2 stable202405
-- Tianocore EDK2-platforms commit 4bbd0ed
+- Trusted Firmware v2.11.0
+- Tianocore EDK2   4d4f569924
+- Tianocore EDK2-platforms 3f08401
 
 """
 
 # Secure BootRom (TF-A code)
 fs0_xz_url = (
 "https://artifacts.codelinaro.org/artifactory/linaro-419-sbsa-ref/";
-"20240528-140808/edk2/SBSA_FLASH0.fd.xz"
+"20240619-148232/edk2/SBSA_FLASH0.fd.xz"
 )
-fs0_xz_hash = 
"fa6004900b67172914c908b78557fec4d36a5f784f4c3dd08f49adb75e1892a9"
+fs0_xz_hash = 
"0c954842a590988f526984de22e21ae0ab9cb351a0c99a8a58e928f0c7359cf7"
 tar_xz_path = self.fetch_asset(fs0_xz_url, asset_hash=fs0_xz_hash,
   algorithm='sha256')
 archive.extract(tar_xz_path, self.workdir)
@@ -57,9 +57,9 @@ def fetch_firmware(self):
 # Non-secure rom (UEFI and EFI variables)
 fs1_xz_url = (
 "https://artifacts.codelinaro.org/artifactory/linaro-419-sbsa-ref/";
-"20240528-140808/edk2/SBSA_FLASH1.fd.xz"
+"20240619-148232/edk2/SBSA_FLASH1.fd.xz"
 )
-fs1_xz_hash = 
"5f3747d4000bc416d9641e33ff4ac60c3cc8cb74ca51b6e932e58531c62eb6f7"
+fs1_xz_hash = 
"c6ec39374c4d79bb9e9cdeeb6db44732d90bb4a334cec92002b3f4b9cac4b5ee"
 tar_xz_path = self.fetch_asset(fs1_xz_url, asset_hash=fs1_xz_hash,
   algorithm='sha256')
 archive.extract(tar_xz_path, self.workdir)

-- 
2.45.1

[PATCH v3 0/2] tests/avocado: make sbsa-ref working with >1 core

[Marcin Juszkiewicz]

Recent changes made sbsa-ref crash when more than 1 cpu core was used.
We handle it in firmware now so one patch updates it to the working
snapshot (TF-A 2.11 + EDK2 snapshot + EDK2-platforms snapshot).

Other change drops "-smp 1" from CI to make sure we test default setup
of sbsa-ref.

Previous firmware worked with 1 cpu by pure luck probably.

To: qemu-devel@nongnu.org
Cc: qemu-...@nongnu.org,
Cc: Peter Maydell ,
Cc: Leif Lindholm ,
Cc: Radoslaw Biernacki ,
Cc: Cleber Rosa ,
Cc: Philippe Mathieu-Daudé 
Cc: Wainer dos Santos Moschetta ,
Cc: Beraldo Leal ,
Cc: Ard Biesheuvel 
Cc: Rebecca Cran 

Signed-off-by: Marcin Juszkiewicz 
---
Changes in v3:
- first update firmware, then use all cores (for bisecting)
- changed commit message in 'use all cores' patch

---
Marcin Juszkiewicz (2):
  tests/avocado: update firmware for sbsa-ref
  tests/avocado: use default amount of cores on sbsa-ref

 tests/avocado/machine_aarch64_sbsaref.py | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)
---
base-commit: 02d9c38236cf8c9826e5c5be61780c4444cb4ae0
change-id: 20240620-b4-new-firmware-177daccc9d76

Best regards,
-- 
Marcin Juszkiewicz 

[PATCH v3 2/2] tests/avocado: use default amount of cores on sbsa-ref

[Marcin Juszkiewicz]

The version of the sbsa-ref EDK2 firmware we used to use in this test
had a bug where it might make an unaligned access to the framebuffer,
which causes a guest crash on newer versions of QEMU where we enforce
the architectural requirement that unaligned accesses to Device memory
should take an exception.

We happened to not notice this because our test was booting with "-smp
1" and through luck this didn't write the boot logo to the framebuffer
at an unaligned address; but trying to boot the same firmware with two
CPUs would result in a guest crash. Now we have updated the firmware
we're using for the test, we can make the test use all the cores on the
board, so we are testing the SMP boot path.

Signed-off-by: Marcin Juszkiewicz 
---
 tests/avocado/machine_aarch64_sbsaref.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tests/avocado/machine_aarch64_sbsaref.py 
b/tests/avocado/machine_aarch64_sbsaref.py
index e854ec6a1a..e920bbf08c 100644
--- a/tests/avocado/machine_aarch64_sbsaref.py
+++ b/tests/avocado/machine_aarch64_sbsaref.py
@@ -75,8 +75,6 @@ def fetch_firmware(self):
 f"if=pflash,file={fs0_path},format=raw",
 "-drive",
 f"if=pflash,file={fs1_path},format=raw",
-"-smp",
-"1",
 "-machine",
 "sbsa-ref",
 )

-- 
2.45.1

Re: [PATCH 22/32] hw/sd: Add emmc_cmd_SEND_EXT_CSD() handler

[Philippe Mathieu-Daudé]

On 20/6/24 09:23, Cédric Le Goater wrote:

Hello

On 6/19/24 7:40 PM, Philippe Mathieu-Daudé wrote:

Hi,

On 3/7/23 15:24, Cédric Le Goater wrote:

The parameters mimick a real 4GB eMMC, but it can be set to various
sizes. Initially from Vincent Palatin 

Signed-off-by: Cédric Le Goater 
---
  hw/sd/sdmmc-internal.h |  97 
  include/hw/sd/sd.h |   1 +
  hw/sd/sd.c | 109 -
  3 files changed, 206 insertions(+), 1 deletion(-)


First pass review, this will take time...


+static void mmc_set_ext_csd(SDState *sd, uint64_t size)
+{
+    uint32_t sectcount = size >> HWBLOCK_SHIFT;
+
+    memset(sd->ext_csd, 0, sizeof(sd->ext_csd));
+
+    sd->ext_csd[EXT_CSD_S_CMD_SET] = 0x1; /* supported command sets */
+    sd->ext_csd[EXT_CSD_HPI_FEATURES] = 0x3; /* HPI features  */
+    sd->ext_csd[EXT_CSD_BKOPS_SUPPORT] = 0x1; /* Background 
operations */
+    sd->ext_csd[241] = 0xA; /* 1st initialization time after 
partitioning */

+    sd->ext_csd[EXT_CSD_TRIM_MULT] = 0x1; /* Trim multiplier */
+    sd->ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] = 0x15; /* Secure 
feature */


We do not support (and are not interested in) that. I'll use 0x0 for
"do not support".

+    sd->ext_csd[EXT_CSD_SEC_ERASE_MULT] = 0x96; /* Secure erase 
support */


This value is obsolete, so I'd use 0x0 to avoid confusions.

+    sd->ext_csd[EXT_CSD_SEC_TRIM_MULT] = 0x96; /* Secure TRIM 
multiplier */


Again, 0x0 for "not defined".


+    sd->ext_csd[EXT_CSD_BOOT_INFO] = 0x7; /* Boot information */
+    sd->ext_csd[EXT_CSD_BOOT_MULT] = 0x8; /* Boot partition size. 
128KB unit */

+    sd->ext_csd[EXT_CSD_ACC_SIZE] = 0x6; /* Access size */


16KB of super_page_size hmm. Simpler could be the underlying block
retrieved with bdrv_nb_sectors() or simply BDRV_SECTOR_SIZE (0x1).

+    sd->ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] = 0x4; /* HC Erase unit 
size */


2MB of erase size hmmm why not.

+    sd->ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] = 0x1; /* HC erase 
timeout */


We don't implement timeout, can we use 0?

+    sd->ext_csd[EXT_CSD_REL_WR_SEC_C] = 0x1; /* Reliable write 
sector count */
+    sd->ext_csd[EXT_CSD_HC_WP_GRP_SIZE] = 0x4; /* HC write protect 
group size */

+    sd->ext_csd[EXT_CSD_S_C_VCC] = 0x8; /* Sleep current VCC  */
+    sd->ext_csd[EXT_CSD_S_C_VCCQ] = 0x7; /* Sleep current VCCQ */
+    sd->ext_csd[EXT_CSD_S_A_TIMEOUT] = 0x11; /* Sleep/Awake timeout */
+    sd->ext_csd[215] = (sectcount >> 24) & 0xff; /* Sector count */
+    sd->ext_csd[214] = (sectcount >> 16) & 0xff; /* ... */
+    sd->ext_csd[213] = (sectcount >> 8) & 0xff;  /* ... */
+    sd->ext_csd[EXT_CSD_SEC_CNT] = (sectcount & 0xff);   /* ... */
+    sd->ext_csd[210] = 0xa; /* Min write perf for 8bit@52Mhz */
+    sd->ext_csd[209] = 0xa; /* Min read perf for 8bit@52Mhz  */
+    sd->ext_csd[208] = 0xa; /* Min write perf for 4bit@52Mhz */
+    sd->ext_csd[207] = 0xa; /* Min read perf for 4bit@52Mhz */
+    sd->ext_csd[206] = 0xa; /* Min write perf for 4bit@26Mhz */
+    sd->ext_csd[205] = 0xa; /* Min read perf for 4bit@26Mhz */


Class B at 3MB/s. I suppose announcing up to J at 21MB/s is safe (0x46).


+    sd->ext_csd[EXT_CSD_PART_SWITCH_TIME] = 0x1;


SWITCH command isn't implemented so far. We could use 0x0 for "not
defined".


+    sd->ext_csd[EXT_CSD_OUT_OF_INTERRUPT_TIME] = 0x1;


Similarly, 0x0 for "undefined" is legal.


+    sd->ext_csd[EXT_CSD_CARD_TYPE] = 0x7;


You anounce dual data rate. Could we just use High-Speed mode (0x3)
to ease modelling?


+    sd->ext_csd[EXT_CSD_STRUCTURE] = 0x2;
+    sd->ext_csd[EXT_CSD_REV] = 0x5;


This is Revision 1.5 (for MMC v4.41)... The first QEMU implementation
was based on Revision 1.3 (for MMC v4.3) and I'm seeing some features
from Revision 1.6 (for MMC v4.5)...

Do we want to implement all of them? Since we are adding from
scratch, I suggest we directly start with v4.5 (0x6).

Note, EXT_CSD_BUS_WIDTH is not set (0x0) meaning 1-bit data bus.
I'd set it to 0x2 (8-bit):

    sd->ext_csd[EXT_CSD_BUS_WIDTH] = EXT_CSD_BUS_WIDTH_8_MASK;



I applied the proposed changes from above and the rainier-bmc boots fine.
Here are the mmc related logs :


   U-Boot SPL 2019.04 (Jun 17 2024 - 07:49:13 +)
   Trying to boot from MMC1
   U-Boot 2019.04 (Jun 17 2024 - 07:49:13 +)
   SOC: AST2600-A3
   eMMC 2nd Boot (ABR): Enable, boot partition: 1
   LPC Mode: SIO:Disable
   Eth: MAC0: RMII/NCSI, MAC1: RMII/NCSI, MAC2: RMII/NCSI, MAC3: RMII/NCSI
   Model: IBM P10 BMC
   DRAM:  already initialized, 896 MiB (capacity:1024 MiB, VGA:64 MiB, 
ECC:on, ECC size:896 MiB)

   MMC:   emmc_slot0@100: 0
   Loading Environment from MMC... OK
   In:    serial@1e784000
   Out:   serial@1e784000
   Err:   serial@1e784000
   Model: IBM P10 BMC
   Net:   No MDIO found.
   ftgmac100_probe - NCSI detected
   ...
   [    0.640650] mmc0: SDHCI controller on 1e750100.sdhci 
[1e750100.sdhci] using ADMA

   [    0.658402] mmc0: unspecified timeout for CMD6 - use gener

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

[Philippe Mathieu-Daudé]

On 20/6/24 12:10, Peter Maydell wrote:

On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé  wrote:


On 18/6/24 16:40, Zheyu Ma wrote:

This commit updates the a9_gtimer_get_current_cpu() function to handle
cases where QTest is enabled. When QTest is used, it returns 0 instead
of dereferencing the current_cpu, which can be NULL. This prevents the
program from crashing during QTest runs.

Reproducer:
cat << EOF | qemu-system-aarch64 -display \
none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
writel 0xf03fe20c 0x26d7468c
EOF

Signed-off-by: Zheyu Ma 
---
   hw/timer/a9gtimer.c | 5 +
   1 file changed, 5 insertions(+)




   if (current_cpu->cpu_index >= s->num_cpu) {


That said, such accesses of @current_cpu from hw/ are dubious.


True, but I'm not sure we ever settled on the right way to avoid
them, did we?


No we didn't, it is still in my TODO list; we might discuss it
when I post my RFC.

Regards,

Phil.

[PATCH v3 0/3] Add boot-mode property for zynq

[Sai Pavan Boddu]

Add a way to update the boot-mode via machine properties.

Changes for V2:
Make boot-mode property work with string
Fixed few code style issues
Added zynq board doc.
Changes for V3:
Mentioned about zynq doc in MAINTAINERS file
Stick to small case for mentioning boot modes in doc
fixed commit message to mention right property name.


Sai Pavan Boddu (3):
  hw/misc/zynq_slcr: Add boot-mode property
  hw/arm/xilinx_zynq: Add boot-mode property
  docs/system/arm: Add a doc for zynq board

 MAINTAINERS   |  1 +
 docs/system/arm/xlnx-zynq.rst | 47 +++
 docs/system/target-arm.rst|  1 +
 hw/arm/xilinx_zynq.c  | 31 +++
 hw/misc/zynq_slcr.c   | 22 +++-
 5 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 docs/system/arm/xlnx-zynq.rst

-- 
2.34.1

[PATCH v3 2/3] hw/arm/xilinx_zynq: Add boot-mode property

[Sai Pavan Boddu]

Read boot-mode value as machine property and propagate that to
SLCR.BOOT_MODE register.

Signed-off-by: Sai Pavan Boddu 
Acked-by: Edgar E. Iglesias 
---
 hw/arm/xilinx_zynq.c | 31 +++
 1 file changed, 31 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 7f7a3d23fbe..39f07e6dfd8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -38,6 +38,7 @@
 #include "qom/object.h"
 #include "exec/tswap.h"
 #include "target/arm/cpu-qom.h"
+#include "qapi/visitor.h"
 
 #define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
 OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
@@ -90,6 +91,7 @@ struct ZynqMachineState {
 MachineState parent;
 Clock *ps_clk;
 ARMCPU *cpu[ZYNQ_MAX_CPUS];
+uint8_t boot_mode;
 };
 
 static void zynq_write_board_setup(ARMCPU *cpu,
@@ -176,6 +178,27 @@ static inline int zynq_init_spi_flashes(uint32_t 
base_addr, qemu_irq irq,
 return unit;
 }
 
+static void zynq_set_boot_mode(Object *obj, const char *str,
+   Error **errp)
+{
+ZynqMachineState *m = ZYNQ_MACHINE(obj);
+uint8_t mode = 0;
+
+if (!strcasecmp(str, "QSPI")) {
+mode = 1;
+} else if (!strcasecmp(str, "SD")) {
+mode = 5;
+} else if (!strcasecmp(str, "NOR")) {
+mode = 2;
+} else if (!strcasecmp(str, "JTAG")) {
+mode = 0;
+} else {
+error_setg(errp, "bootmode %s not supported", str);
+return;
+}
+m->boot_mode = mode;
+}
+
 static void zynq_init(MachineState *machine)
 {
 ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
@@ -241,6 +264,7 @@ static void zynq_init(MachineState *machine)
 /* Create slcr, keep a pointer to connect clocks */
 slcr = qdev_new("xilinx-zynq_slcr");
 qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
+qdev_prop_set_uint8(slcr, "boot-mode", zynq_machine->boot_mode);
 sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
 sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF800);
 
@@ -372,6 +396,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void 
*data)
 NULL
 };
 MachineClass *mc = MACHINE_CLASS(oc);
+ObjectProperty *prop;
 mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
 mc->init = zynq_init;
 mc->max_cpus = ZYNQ_MAX_CPUS;
@@ -379,6 +404,12 @@ static void zynq_machine_class_init(ObjectClass *oc, void 
*data)
 mc->ignore_memory_transaction_failures = true;
 mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_id = "zynq.ext_ram";
+prop = object_class_property_add_str(oc, "boot-mode", NULL,
+  zynq_set_boot_mode);
+object_class_property_set_description(oc, "boot-mode",
+  "Supported boot modes:"
+  " JTAG QSPI SD NOR");
+object_property_set_default_str(prop, "QSPI");
 }
 
 static const TypeInfo zynq_machine_type = {
-- 
2.34.1

[PATCH v3 3/3] docs/system/arm: Add a doc for zynq board

[Sai Pavan Boddu]

Added the supported device list and an example command.

Signed-off-by: Sai Pavan Boddu 
Reviewed-by: Edgar E. Iglesias 
---
 MAINTAINERS   |  1 +
 docs/system/arm/xlnx-zynq.rst | 47 +++
 docs/system/target-arm.rst|  1 +
 3 files changed, 49 insertions(+)
 create mode 100644 docs/system/arm/xlnx-zynq.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index 951556224a1..2f06febc676 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1033,6 +1033,7 @@ F: hw/adc/zynq-xadc.c
 F: include/hw/misc/zynq_slcr.h
 F: include/hw/adc/zynq-xadc.h
 X: hw/ssi/xilinx_*
+F: docs/system/arm/xlnx-zynq.rst
 
 Xilinx ZynqMP and Versal
 M: Alistair Francis 
diff --git a/docs/system/arm/xlnx-zynq.rst b/docs/system/arm/xlnx-zynq.rst
new file mode 100644
index 000..ade18a3fe13
--- /dev/null
+++ b/docs/system/arm/xlnx-zynq.rst
@@ -0,0 +1,47 @@
+Xilinx Zynq board (``xilinx-zynq-a9``)
+==
+The Zynq 7000 family is based on the AMD SoC architecture. These products
+integrate a feature-rich dual or single-core Arm Cortex-A9 MPCore based
+processing system (PS) and AMD programmable logic (PL) in a single device.
+
+More details here:
+https://docs.amd.com/r/en-US/ug585-zynq-7000-SoC-TRM/Zynq-7000-SoC-Technical-Reference-Manual
+
+QEMU xilinx-zynq-a9 board supports following devices:
+- A9 MPCORE
+- cortex-a9
+- GIC v1
+- Generic timer
+- wdt
+- OCM 256KB
+- SMC SRAM@0xe200 64MB
+- Zynq SLCR
+- SPI x2
+- QSPI
+- UART
+- TTC x2
+- Gigabit Ethernet Controller x2
+- SD Controller x2
+- XADC
+- Arm PrimeCell DMA Controller
+- DDR Memory
+- USB 2.0 x2
+
+Running
+"""
+Direct Linux boot of a generic ARM upstream Linux kernel:
+
+.. code-block:: bash
+
+  $ qemu-system-aarch64 -M xilinx-zynq-a9 \
+-dtb zynq-zc702.dtb  -serial null -serial mon:stdio \
+-display none  -m 1024 \
+-initrd rootfs.cpio.gz -kernel zImage
+
+For configuring the boot-mode provide the following on the command line:
+
+.. code-block:: bash
+
+   -machine boot-mode=qspi
+
+Supported values are jtag, sd, qspi, nor.
diff --git a/docs/system/target-arm.rst b/docs/system/target-arm.rst
index 870d30e3502..7b992722846 100644
--- a/docs/system/target-arm.rst
+++ b/docs/system/target-arm.rst
@@ -109,6 +109,7 @@ undocumented; you can get a complete list by running
arm/virt
arm/xenpvh
arm/xlnx-versal-virt
+   arm/xlnx-zynq
 
 Emulated CPU architecture support
 =
-- 
2.34.1

[PATCH v3 1/3] hw/misc/zynq_slcr: Add boot-mode property

[Sai Pavan Boddu]

boot-mode property sets user values into BOOT_MODE register, on hardware
these are derived from board switches.

Signed-off-by: Sai Pavan Boddu 
Reviewed-by: Edgar E. Iglesias 
---
 hw/misc/zynq_slcr.c | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index 3412ff099ea..ad814c3a79b 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -24,6 +24,8 @@
 #include "hw/registerfields.h"
 #include "hw/qdev-clock.h"
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
 
 #ifndef ZYNQ_SLCR_ERR_DEBUG
 #define ZYNQ_SLCR_ERR_DEBUG 0
@@ -121,6 +123,7 @@ REG32(RST_REASON, 0x250)
 
 REG32(REBOOT_STATUS, 0x258)
 REG32(BOOT_MODE, 0x25c)
+FIELD(BOOT_MODE, BOOT_MODE, 0, 4)
 
 REG32(APU_CTRL, 0x300)
 REG32(WDT_CLK_SEL, 0x304)
@@ -195,6 +198,7 @@ struct ZynqSLCRState {
 Clock *ps_clk;
 Clock *uart0_ref_clk;
 Clock *uart1_ref_clk;
+uint8_t boot_mode;
 };
 
 /*
@@ -371,7 +375,7 @@ static void zynq_slcr_reset_init(Object *obj, ResetType 
type)
 s->regs[R_FPGA_RST_CTRL]  = 0x01F33F0F;
 s->regs[R_RST_REASON] = 0x0040;
 
-s->regs[R_BOOT_MODE]  = 0x0001;
+s->regs[R_BOOT_MODE]  = s->boot_mode & R_BOOT_MODE_BOOT_MODE_MASK;
 
 /* 0x700 - 0x7D4 */
 for (i = 0; i < 54; i++) {
@@ -588,6 +592,15 @@ static const ClockPortInitArray zynq_slcr_clocks = {
 QDEV_CLOCK_END
 };
 
+static void zynq_slcr_realize(DeviceState *dev, Error **errp)
+{
+ZynqSLCRState *s = ZYNQ_SLCR(dev);
+
+if (s->boot_mode > 0xF) {
+error_setg(errp, "Invalid boot mode %d specified", s->boot_mode);
+}
+}
+
 static void zynq_slcr_init(Object *obj)
 {
 ZynqSLCRState *s = ZYNQ_SLCR(obj);
@@ -610,15 +623,22 @@ static const VMStateDescription vmstate_zynq_slcr = {
 }
 };
 
+static Property zynq_slcr_props[] = {
+DEFINE_PROP_UINT8("boot-mode", ZynqSLCRState, boot_mode, 1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
 static void zynq_slcr_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 ResettableClass *rc = RESETTABLE_CLASS(klass);
 
 dc->vmsd = &vmstate_zynq_slcr;
+dc->realize = zynq_slcr_realize;
 rc->phases.enter = zynq_slcr_reset_init;
 rc->phases.hold  = zynq_slcr_reset_hold;
 rc->phases.exit  = zynq_slcr_reset_exit;
+device_class_set_props(dc, zynq_slcr_props);
 }
 
 static const TypeInfo zynq_slcr_info = {
-- 
2.34.1

Re: [PATCH] target/arm/helper: Fix timer interrupt masking when HCR_EL2.E2H == 0

[Peter Maydell]

On Sat, 15 Jun 2024 at 19:56, Florian Lugou  wrote:
>
> CNTHCTL_EL2 based masking of timer interrupts was introduced in
> f6fc36deef6abcee406211f3e2f11ff894b87fa4. This masking was however
> effective no matter whether EL2 was enabled in the current security
> state or not, contrary to arm specification.
>
> Signed-off-by: Florian Lugou 
> ---
>  target/arm/helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index ce31957235..60e2344c68 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2684,7 +2684,8 @@ static void gt_update_irq(ARMCPU *cpu, int timeridx)
>   * If bit CNTHCTL_EL2.CNT[VP]MASK is set, it overrides IMASK.
>   * It is RES0 in Secure and NonSecure state.
>   */
> -if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
> +if ((arm_hcr_el2_eff(env) & HCR_E2H) &&
> +(ss == ARMSS_Root || ss == ARMSS_Realm) &&

When the architecture says "is EL2 enabled in the current security state"
it doesn't mean "is HCR_EL2.E2H set?", it means "is this either NonSecure/Realm
or else is SCR_EL2.EEL2 set?". Compare the pseudocode EL2Enabled()
and QEMU's arm_is_el2_enabled() and arm_is_el2_enabled_secstate() functions.
This doesn't mean much in Root state, and for Realm state EL2 is always
enabled (assuming it is implemented).

For this timer check, we're doing I think the same thing as the
pseudocode AArch64.CheckTimerConditions(), which does:

  if (IsFeatureImplemented(FEAT_RME) && ss IN {SS_Root, SS_Realm} &&
  CNTHCTL_EL2.CNTPMASK == '1') then
 imask = '1';

so I'm inclined to say that our current implementation in QEMU is correct.

>  ((timeridx == GTIMER_VIRT && (cnthctl & R_CNTHCTL_CNTVMASK_MASK)) ||
>   (timeridx == GTIMER_PHYS && (cnthctl & R_CNTHCTL_CNTPMASK_MASK {
>  irqstate = 0;
> --

thanks
-- PMM

Re: [PATCH v3] hw/gpio/aspeed: Add reg_table_size to AspeedGPIOClass

[Philippe Mathieu-Daudé]

Hi,

On 19/6/24 20:36, Zheyu Ma wrote:

ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
function. This issue occurred when reading beyond the bounds of the
reg_table.

To enhance the safety and maintainability of the Aspeed GPIO code, this commit
introduces a reg_table_size member to the AspeedGPIOClass structure. This
change ensures that the size of the GPIO register table is explicitly tracked
and initialized, reducing the risk of errors if new register tables are
introduced in the future.

Reproducer:
cat << EOF | qemu-system-aarch64 -display none \
-machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
readq 0x7e780272
EOF

ASAN log indicating the issue:
==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 
0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
READ of size 2 at 0x55a5da29e128 thread T0
 #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
 #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
 #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
 #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
 #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
 #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
 #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
 #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12

Signed-off-by: Zheyu Ma 
---
Changes in v3:
- Add the reproducer

Changes in v2:
- Introduce the reg_table_size to AspeedGPIOClass
---
  hw/gpio/aspeed_gpio.c | 17 +
  include/hw/gpio/aspeed_gpio.h |  1 +
  2 files changed, 18 insertions(+)




  static const TypeInfo aspeed_gpio_info = {
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 904eecf62c..e66036ac39 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -75,6 +75,7 @@ struct AspeedGPIOClass {
  uint32_t nr_gpio_pins;
  uint32_t nr_gpio_sets;
  const AspeedGPIOReg *reg_table;
+uint32_t reg_table_size;
  };


- "reg_table_size" is a number of registers, using s/size/count/ might
  be clearer.
- No point in specifying 32-bit, "unsigned" is sufficient.

(Cédric, if you agree, you might update your tree).

Unrelated to this patch but figured out while reviewing, in
aspeed_gpio_read/write 'idx' is
- pointlessly assigned to -1
- of type 'uint64_t', also pointless, 'unsigned' is clearer.

Regards,

Phil.

Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency

[Alex Bennée]

Manos Pitsidianakis  writes:

> Add mechanism to generate rust hw targets that depend on a custom
> bindgen target for rust bindings to C.
>
> This way bindings will be created before the rust crate is compiled.
>
> The bindings will end up in BUILDDIR/{target}-generated.rs and have the same 
> name
> as a target:
>
> ninja aarch64-softmmu-generated.rs
>

> +
> +# FIXME: These are the latest stable versions, refine to actual minimum ones.
> +msrv = {
> +  'rustc': '1.79.0',
> +  'cargo': '1.79.0',
> +  'bindgen': '0.69.4',
> +}

So for Debian Bookworm this comes out as:

  msrv = {
'rustc': '1.79.0',
'cargo': '1.79.0',
'bindgen': '0.69.4',
  }

I shall have to see how close Trixie is ;-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/riscv/virt.c: Make block devices default to virtio

[Daniel Henrique Barboza]

On 6/20/24 3:47 AM, Sunil V L wrote:

RISC-V virt is currently missing default type for block devices. Without
this being set, proper backend is not created when option like -cdrom
is used. So, make the virt board's default block device type be
IF_VIRTIO similar to other architectures.

We also need to set no_cdrom to avoid getting a default cdrom device.

Signed-off-by: Sunil V L 
---


Reviewed-by: Daniel Henrique Barboza 


  hw/riscv/virt.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8675c3a7d1..b0871b7f81 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1764,6 +1764,8 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
  mc->init = virt_machine_init;
  mc->max_cpus = VIRT_CPUS_MAX;
  mc->default_cpu_type = TYPE_RISCV_CPU_BASE;
+mc->block_default_type = IF_VIRTIO;
+mc->no_cdrom = 1;
  mc->pci_allow_0_address = true;
  mc->possible_cpu_arch_ids = riscv_numa_possible_cpu_arch_ids;
  mc->cpu_index_to_instance_props = riscv_numa_cpu_index_to_props;

Re: [PATCH 0/4] hw/m68k/virt: Add some devices

[Laurent Vivier]

Le 27/05/2024 à 19:15, Jiaxun Yang a écrit :

Hi all,

This series added some devices that I found lacking when
I was trying to port U-Boot to m68k virt machine.


I have a branch with a bootloader based on petitboot.

See https://github.com/vivier/qemu-m68k/commits/m68k-virt/

Thanks
Laurent



Please review.
Thanks

Signed-off-by: Jiaxun Yang 
---
Jiaxun Yang (4):
   hw/m68k/virt: Add a XHCI controller
   hw/m68k/virt: Add fw_cfg controller
   hw/m68k/virt: Add a pflash controller for BIOS firmware
   hw/m68k/virt: Supply bootinfo for BIOS

  hw/m68k/Kconfig   |   3 +
  hw/m68k/virt.c| 231 --
  include/standard-headers/asm-m68k/bootinfo-virt.h |   4 +
  3 files changed, 176 insertions(+), 62 deletions(-)
---
base-commit: 60b54b67c63d8f076152e0f7dccf39854dfc6a77
change-id: 20240527-m68k-bios-a0a2370181f5

Best regards,

Re: [PATCH v4 00/15] vfio: VFIO migration support with vIOMMU

[Cédric Le Goater]

[ ... ]


* [v4] vfio: VFIO migration support with vIOMMU
  
https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.mart...@oracle.com/

     Refreshed the patchset on upstream and pushed on vfio-9.1 branch.


/me nods Probably deserves an item on the list too related to this subject of
vIOMMU and migration after the vIOMMU series is done:

*
https://lore.kernel.org/qemu-devel/20230908120521.50903-1-joao.m.mart...@oracle.com/


   * [RFCv2] vfio/iommufd: IOMMUFD Dirty Tracking
  
https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.mart...@oracle.com/



I plan on still submitting a follow-up targetting 9.1 likely next week with
Avihai's comments on top of the vfio-9.1 branch after I sent some dirty tracking
fixes in kernel side. Though it is mostly to progress review as I think I am
still dependent on Zhenzhong prep series for merging because of this patch:
https://lore.kernel.org/all/20240605083043.317831-8-zhenzhong.d...@intel.com/


This is ready to be pushed.

As soon as I get an ack, a nod, a smoke sign, from the PCI maintainers
regarding the new PCIIOMMUOps callbacks I will send a PR for:

  https://lore.kernel.org/all/20240522170107.289532-1-...@redhat.com
  https://lore.kernel.org/all/20240605083043.317831-1-zhenzhong.d...@intel.com
  https://lore.kernel.org/all/20240614095402.904691-1-eric.au...@redhat.com
  https://lore.kernel.org/all/20240617063409.34393-1-...@redhat.com

Thanks,

C.

Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency

[Alex Bennée]

Manos Pitsidianakis  writes:

> Add mechanism to generate rust hw targets that depend on a custom
> bindgen target for rust bindings to C.
>
> This way bindings will be created before the rust crate is compiled.
>
> The bindings will end up in BUILDDIR/{target}-generated.rs and have the same 
> name
> as a target:
>
> ninja aarch64-softmmu-generated.rs
>

> +
> +
> +rust_targets = {}
> +
> +cargo_wrapper = [
> +  find_program(meson.global_source_root() / 'scripts/cargo_wrapper.py'),
> +  '--config-headers', meson.project_build_root() / 'config-host.h',
> +  '--meson-build-root', meson.project_build_root(),
> +  '--meson-build-dir', meson.current_build_dir(),
> +  '--meson-source-dir', meson.current_source_dir(),
> +]

I'm unclear what the difference between meson-build-root and
meson-build-dir is?

We also end up defining crate-dir and outdir. Aren't these all
derivable from whatever module we are building?

> +
> +if get_option('b_colorout') != 'never'
> +  cargo_wrapper += ['--color', 'always']
> +endif
> +
> +if get_option('optimization') in ['0', '1', 'g']
> +  rs_build_type = 'debug'
> +else
> +  rs_build_type = 'release'
> +endif
> +
> +# Collect metadata for each (crate,qemu-target,compiler-target) combination.
> +# Rust meson targets cannot be defined a priori because they depend on 
> bindgen
> +# generation that is created for each emulation target separately. Thus Rust
> +# meson targets will be defined for each target after the target-specific
> +# bindgen dependency is declared.
> +rust_hw_target_list = {}
> +
> +foreach rust_hw_target, rust_hws: rust_hw_target_list
> +  foreach rust_hw_dev: rust_hws
> +output = meson.current_build_dir() / rust_target_triple / rs_build_type 
> / rust_hw_dev['output']
> +crate_metadata = {
> +  'name': rust_hw_dev['name'],
> +  'output': [rust_hw_dev['output']],
> +  'output-path': output,
> +  'command': [cargo_wrapper,
> +'--crate-dir', meson.current_source_dir() / rust_hw_dev['dirname'],
> +'--profile', rs_build_type,
> +'--target-triple', rust_target_triple,
> +'--outdir', '@OUTDIR@',
> +'build-lib'
> +]
> +  }
> +rust_targets += { rust_hw_target: [crate_metadata] }
> +  endforeach
> +endforeach
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> new file mode 100644
> index 00..bcf808c8d7
> --- /dev/null
> +++ b/rust/wrapper.h
> @@ -0,0 +1,39 @@
> +/*
> + * QEMU System Emulator
> + *
> + * Copyright (c) 2003-2020 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "qemu-io.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/sysbus.h"
> +#include "exec/memory.h"
> +#include "chardev/char-fe.h"
> +#include "hw/clock.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/qdev-properties-system.h"
> +#include "hw/irq.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "chardev/char-serial.h"
> diff --git a/scripts/cargo_wrapper.py b/scripts/cargo_wrapper.py
> index 927336f80e..833e0e55f8 100644
> --- a/scripts/cargo_wrapper.py
> +++ b/scripts/cargo_wrapper.py
> @@ -111,6 +111,8 @@ def get_cargo_rustc(args: argparse.Namespace) -> 
> tuple[Dict[str, Any], List[str]
>  
>  env = os.environ
>  env["CARGO_ENCODED_RUSTFLAGS"] = cfg
> +env["MESON_BUILD_DIR"] = str(target_dir)
> +env["MESON_BUILD_ROOT"] = str(args.meson_build_root)
>  
>  return (env, cargo_cmd)
>  
> @@ -234,6 +236,14 @@ def main() -> None:
>  required=True,
>  )
>  parser.add_argument(
> +"--meson-build-root",
> +metavar="BUILD_ROOT",
> +help="meson.project_build_root()",
> +type=Path,
> +dest="meson_build_root",
> +required=True,
> +)
> +parser.add_argument(
>  "--meson-source-dir",
>  metavar="SOURC

Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency

[Paolo Bonzini]

On Thu, Jun 20, 2024 at 1:10 PM Alex Bennée  wrote:
> > +# FIXME: These are the latest stable versions, refine to actual minimum 
> > ones.
> > +msrv = {
> > +  'rustc': '1.79.0',
> > +  'cargo': '1.79.0',
> > +  'bindgen': '0.69.4',
> > +}
>
> So for Debian Bookworm this comes out as:
>
>   msrv = {
> 'rustc': '1.79.0',
> 'cargo': '1.79.0',
> 'bindgen': '0.69.4',
>   }

I think it's 0.60.1 bindgen and 1.63.0 rustc/cargo? That means we
don't have generic associated types (1.65), which are nice to have but
not absolutely necessary.

The only other one with an old version is Ubuntu 22.04 (1.58.1), but
it has 1.75.0 in updates

Paolo

Re: [PATCH v3] hw/gpio/aspeed: Add reg_table_size to AspeedGPIOClass

[Cédric Le Goater]

@@ -75,6 +75,7 @@ struct AspeedGPIOClass {
  uint32_t nr_gpio_pins;
  uint32_t nr_gpio_sets;
  const AspeedGPIOReg *reg_table;
+    uint32_t reg_table_size;
  };


- "reg_table_size" is a number of registers, using s/size/count/ might
   be clearer.
- No point in specifying 32-bit, "unsigned" is sufficient.

(Cédric, if you agree, you might update your tree).

Unrelated to this patch but figured out while reviewing, in
aspeed_gpio_read/write 'idx' is
- pointlessly assigned to -1
- of type 'uint64_t', also pointless, 'unsigned' is clearer.


Zheyu, could you please send a v4 ? Thanks,

C.

[PATCH 0/6] host/i386: allow configuring the x86-64 baseline

[Paolo Bonzini]

As discussed, add a Meson option to configure which x86-64 instruction
set to use.  QEMU will now default to x86-64-v1 + cmpxchg16b for
64-bit builds (that corresponds to a Pentium 4 for 32-bit builds).

The baseline can be tuned down to Pentium Pro for 32-bit builds (with
-Dx86_version=0), or up as desired.

Patch "host/i386: assume presence of CMOV" is not reverted because
CMOV appeared first in the Pentium Pro.

Paolo

Paolo Bonzini (6):
  Revert "host/i386: assume presence of POPCNT"
  Revert "host/i386: assume presence of SSSE3"
  Revert "host/i386: assume presence of SSE2"
  meson: allow configuring the x86-64 baseline
  meson: remove dead optimization option
  meson: require compiler support for chosen x86-64 instructions

 meson.build  | 56 
 host/include/i386/host/cpuinfo.h |  2 ++
 tcg/i386/tcg-target.h|  5 +--
 util/bufferiszero.c  |  4 +--
 util/cpuinfo-i386.c  |  6 ++--
 meson_options.txt|  5 +--
 scripts/meson-buildoptions.sh|  3 ++
 7 files changed, 52 insertions(+), 29 deletions(-)

-- 
2.45.2

[PATCH 2/6] Revert "host/i386: assume presence of SSSE3"

[Paolo Bonzini]

This reverts commit 433cd6d94a8256af70a5200f236dc8047c3c1468.
The x86-64 instruction set can now be tuned down to x86-64 v1
or i386 Pentium Pro.

Signed-off-by: Paolo Bonzini 
---
 util/cpuinfo-i386.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/cpuinfo-i386.c b/util/cpuinfo-i386.c
index 6d474a6259a..ca74ef04f54 100644
--- a/util/cpuinfo-i386.c
+++ b/util/cpuinfo-i386.c
@@ -38,8 +38,8 @@ unsigned __attribute__((constructor)) cpuinfo_init(void)
 info |= (c & bit_POPCNT ? CPUINFO_POPCNT : 0);
 info |= (c & bit_PCLMUL ? CPUINFO_PCLMUL : 0);
 
-/* NOTE: our AES support requires SSSE3 (PSHUFB) as well. */
-info |= (c & bit_AES) ? CPUINFO_AES : 0;
+/* Our AES support requires PSHUFB as well. */
+info |= ((c & bit_AES) && (c & bit_SSSE3) ? CPUINFO_AES : 0);
 
 /* For AVX features, we must check available and usable. */
 if ((c & bit_AVX) && (c & bit_OSXSAVE)) {
-- 
2.45.2

[PATCH 4/6] meson: allow configuring the x86-64 baseline

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 meson.build   | 41 ---
 meson_options.txt |  3 +++
 scripts/meson-buildoptions.sh |  3 +++
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/meson.build b/meson.build
index 97e00d6f59b..6e694ecd9fe 100644
--- a/meson.build
+++ b/meson.build
@@ -336,15 +336,40 @@ if host_arch == 'i386' and not cc.links('''
   qemu_common_flags = ['-march=i486'] + qemu_common_flags
 endif
 
-# Assume x86-64-v2 (minus CMPXCHG16B for 32-bit code)
-if host_arch == 'i386'
-  qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
-endif
+# Pick x86-64 baseline version
 if host_arch in ['i386', 'x86_64']
-  qemu_common_flags = ['-mpopcnt', '-msse4.2'] + qemu_common_flags
-endif
-if host_arch == 'x86_64'
-  qemu_common_flags = ['-mcx16'] + qemu_common_flags
+  if get_option('x86_version') == '0' and host_arch == 'x86_64'
+error('x86_64-v1 required for x86-64 hosts')
+  endif
+
+  # add flags for individual instruction set extensions
+  if get_option('x86_version') >= '1'
+if host_arch == 'i386'
+  qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
+else
+  # present on basically all processors but technically not part of
+  # x86-64-v1, so only include -mneeded for x86-64 version 2 and above
+  qemu_common_flags = ['-mcx16'] + qemu_common_flags
+endif
+  endif
+  if get_option('x86_version') >= '2'
+qemu_common_flags = ['-mpopcnt'] + qemu_common_flags
+qemu_common_flags = cc.get_supported_arguments('-mneeded') + 
qemu_common_flags
+  endif
+  if get_option('x86_version') >= '3'
+qemu_common_flags = ['-mmovbe', '-mabm', '-mbmi1', '-mbmi2', '-mfma', 
'-mf16c'] + qemu_common_flags
+  endif
+
+  # add required vector instruction set (each level implies those below)
+  if get_option('x86_version') == '1'
+qemu_common_flags = ['-msse2'] + qemu_common_flags
+  elif get_option('x86_version') == '2'
+qemu_common_flags = ['-msse4.2'] + qemu_common_flags
+  elif get_option('x86_version') == '3'
+qemu_common_flags = ['-mavx2'] + qemu_common_flags
+  elif get_option('x86_version') == '4'
+qemu_common_flags = ['-mavx512f', '-mavx512bw', '-mavx512cd', 
'-mavx512dq', '-mavx512vl'] + qemu_common_flags
+  endif
 endif
 
 if get_option('prefer_static')
diff --git a/meson_options.txt b/meson_options.txt
index 7a79dd89706..6065ed2d352 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -370,3 +370,6 @@ option('qemu_ga_version', type: 'string', value: '',
 
 option('hexagon_idef_parser', type : 'boolean', value : true,
description: 'use idef-parser to automatically generate TCG code for 
the Hexagon frontend')
+
+option('x86_version', type : 'combo', choices : ['0', '1', '2', '3', '4'], 
value: '1',
+   description: 'tweak required x86_64 architecture version beyond 
compiler default')
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 58d49a447d5..62842d47e88 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -82,6 +82,8 @@ meson_options_help() {
   printf "%s\n" '  --with-suffix=VALUE  Suffix for QEMU 
data/modules/config directories'
   printf "%s\n" '   (can be empty) [qemu]'
   printf "%s\n" '  --with-trace-file=VALUE  Trace file prefix for simple 
backend [trace]'
+  printf "%s\n" '  --x86-version=CHOICE tweak required x86_64 architecture 
version beyond'
+  printf "%s\n" '   compiler default [1] (choices: 
0/1/2/3)'
   printf "%s\n" ''
   printf "%s\n" 'Optional features, enabled with --enable-FEATURE and'
   printf "%s\n" 'disabled with --disable-FEATURE, default is enabled if 
available'
@@ -552,6 +554,7 @@ _meson_option_parse() {
 --disable-werror) printf "%s" -Dwerror=false ;;
 --enable-whpx) printf "%s" -Dwhpx=enabled ;;
 --disable-whpx) printf "%s" -Dwhpx=disabled ;;
+--x86-version=*) quote_sh "-Dx86_version=$2" ;;
 --enable-xen) printf "%s" -Dxen=enabled ;;
 --disable-xen) printf "%s" -Dxen=disabled ;;
 --enable-xen-pci-passthrough) printf "%s" -Dxen_pci_passthrough=enabled ;;
-- 
2.45.2

[PATCH 5/6] meson: remove dead optimization option

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 meson.build   | 13 -
 meson_options.txt |  2 --
 2 files changed, 15 deletions(-)

diff --git a/meson.build b/meson.build
index 6e694ecd9fe..54e6b09f4fb 100644
--- a/meson.build
+++ b/meson.build
@@ -2874,18 +2874,6 @@ config_host_data.set('CONFIG_AVX2_OPT', 
get_option('avx2') \
 int main(int argc, char *argv[]) { return bar(argv[argc - 1]); }
   '''), error_message: 'AVX2 not available').allowed())
 
-config_host_data.set('CONFIG_AVX512F_OPT', get_option('avx512f') \
-  .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable 
AVX512F') \
-  .require(cc.links('''
-#include 
-#include 
-static int __attribute__((target("avx512f"))) bar(void *a) {
-  __m512i x = *(__m512i *)a;
-  return _mm512_test_epi64_mask(x, x);
-}
-int main(int argc, char *argv[]) { return bar(argv[argc - 1]); }
-  '''), error_message: 'AVX512F not available').allowed())
-
 config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \
   .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable 
AVX512BW') \
   .require(cc.links('''
@@ -4283,7 +4271,6 @@ summary_info += {'mutex debugging':   
get_option('debug_mutex')}
 summary_info += {'memory allocator':  get_option('malloc')}
 summary_info += {'avx2 optimization': config_host_data.get('CONFIG_AVX2_OPT')}
 summary_info += {'avx512bw optimization': 
config_host_data.get('CONFIG_AVX512BW_OPT')}
-summary_info += {'avx512f optimization': 
config_host_data.get('CONFIG_AVX512F_OPT')}
 summary_info += {'gcov':  get_option('b_coverage')}
 summary_info += {'thread sanitizer':  get_option('tsan')}
 summary_info += {'CFI support':   get_option('cfi')}
diff --git a/meson_options.txt b/meson_options.txt
index 6065ed2d352..0269fa0f16e 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -119,8 +119,6 @@ option('membarrier', type: 'feature', value: 'disabled',
 
 option('avx2', type: 'feature', value: 'auto',
description: 'AVX2 optimizations')
-option('avx512f', type: 'feature', value: 'disabled',
-   description: 'AVX512F optimizations')
 option('avx512bw', type: 'feature', value: 'auto',
description: 'AVX512BW optimizations')
 option('keyring', type: 'feature', value: 'auto',
-- 
2.45.2

[PATCH 1/6] Revert "host/i386: assume presence of POPCNT"

[Paolo Bonzini]

This reverts commit 45ccdbcb24baf99667997fac5cf60318e5e7db51.
The x86-64 instruction set can now be tuned down to x86-64 v1
or i386 Pentium Pro.

Signed-off-by: Paolo Bonzini 
---
 host/include/i386/host/cpuinfo.h | 1 +
 tcg/i386/tcg-target.h| 5 +++--
 util/cpuinfo-i386.c  | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/host/include/i386/host/cpuinfo.h b/host/include/i386/host/cpuinfo.h
index c1e94d75ce1..72f6fad61e5 100644
--- a/host/include/i386/host/cpuinfo.h
+++ b/host/include/i386/host/cpuinfo.h
@@ -11,6 +11,7 @@
 #define CPUINFO_ALWAYS  (1u << 0)  /* so cpuinfo is nonzero */
 #define CPUINFO_MOVBE   (1u << 2)
 #define CPUINFO_LZCNT   (1u << 3)
+#define CPUINFO_POPCNT  (1u << 4)
 #define CPUINFO_BMI1(1u << 5)
 #define CPUINFO_BMI2(1u << 6)
 #define CPUINFO_AVX1(1u << 9)
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index ecc69827287..2f67a97e059 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -111,6 +111,7 @@ typedef enum {
 #endif
 
 #define have_bmi1 (cpuinfo & CPUINFO_BMI1)
+#define have_popcnt   (cpuinfo & CPUINFO_POPCNT)
 #define have_avx1 (cpuinfo & CPUINFO_AVX1)
 #define have_avx2 (cpuinfo & CPUINFO_AVX2)
 #define have_movbe(cpuinfo & CPUINFO_MOVBE)
@@ -142,7 +143,7 @@ typedef enum {
 #define TCG_TARGET_HAS_nor_i32  0
 #define TCG_TARGET_HAS_clz_i32  1
 #define TCG_TARGET_HAS_ctz_i32  1
-#define TCG_TARGET_HAS_ctpop_i321
+#define TCG_TARGET_HAS_ctpop_i32have_popcnt
 #define TCG_TARGET_HAS_deposit_i32  1
 #define TCG_TARGET_HAS_extract_i32  1
 #define TCG_TARGET_HAS_sextract_i32 1
@@ -177,7 +178,7 @@ typedef enum {
 #define TCG_TARGET_HAS_nor_i64  0
 #define TCG_TARGET_HAS_clz_i64  1
 #define TCG_TARGET_HAS_ctz_i64  1
-#define TCG_TARGET_HAS_ctpop_i641
+#define TCG_TARGET_HAS_ctpop_i64have_popcnt
 #define TCG_TARGET_HAS_deposit_i64  1
 #define TCG_TARGET_HAS_extract_i64  1
 #define TCG_TARGET_HAS_sextract_i64 0
diff --git a/util/cpuinfo-i386.c b/util/cpuinfo-i386.c
index 8f2694d88f2..6d474a6259a 100644
--- a/util/cpuinfo-i386.c
+++ b/util/cpuinfo-i386.c
@@ -35,6 +35,7 @@ unsigned __attribute__((constructor)) cpuinfo_init(void)
 __cpuid(1, a, b, c, d);
 
 info |= (c & bit_MOVBE ? CPUINFO_MOVBE : 0);
+info |= (c & bit_POPCNT ? CPUINFO_POPCNT : 0);
 info |= (c & bit_PCLMUL ? CPUINFO_PCLMUL : 0);
 
 /* NOTE: our AES support requires SSSE3 (PSHUFB) as well. */
-- 
2.45.2

[PATCH 6/6] meson: require compiler support for chosen x86-64 instructions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 meson.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/meson.build b/meson.build
index 54e6b09f4fb..c5360fbd299 100644
--- a/meson.build
+++ b/meson.build
@@ -2863,6 +2863,7 @@ have_cpuid_h = cc.links('''
 config_host_data.set('CONFIG_CPUID_H', have_cpuid_h)
 
 config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
+  .enable_auto_if(get_option('x86_version') >= '3') \
   .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable 
AVX2') \
   .require(cc.links('''
 #include 
@@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', 
get_option('avx2') \
   '''), error_message: 'AVX2 not available').allowed())
 
 config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \
+  .enable_auto_if(get_option('x86_version') >= '4') \
   .require(have_cpuid_h, error_message: 'cpuid.h not available, cannot enable 
AVX512BW') \
   .require(cc.links('''
 #include 
-- 
2.45.2

[PATCH 3/6] Revert "host/i386: assume presence of SSE2"

[Paolo Bonzini]

This reverts commit b18236897ca15c3db1506d8edb9a191dfe51429c.
The x86-64 instruction set can now be tuned down to x86-64 v1
or i386 Pentium Pro.

Signed-off-by: Paolo Bonzini 
---
 host/include/i386/host/cpuinfo.h | 1 +
 util/bufferiszero.c  | 4 ++--
 util/cpuinfo-i386.c  | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/host/include/i386/host/cpuinfo.h b/host/include/i386/host/cpuinfo.h
index 72f6fad61e5..81771733eaa 100644
--- a/host/include/i386/host/cpuinfo.h
+++ b/host/include/i386/host/cpuinfo.h
@@ -14,6 +14,7 @@
 #define CPUINFO_POPCNT  (1u << 4)
 #define CPUINFO_BMI1(1u << 5)
 #define CPUINFO_BMI2(1u << 6)
+#define CPUINFO_SSE2(1u << 7)
 #define CPUINFO_AVX1(1u << 9)
 #define CPUINFO_AVX2(1u << 10)
 #define CPUINFO_AVX512F (1u << 11)
diff --git a/util/bufferiszero.c b/util/bufferiszero.c
index 11c080e02cf..74864f7b782 100644
--- a/util/bufferiszero.c
+++ b/util/bufferiszero.c
@@ -188,14 +188,14 @@ static biz_accel_fn const accel_table[] = {
 
 static unsigned best_accel(void)
 {
-#ifdef CONFIG_AVX2_OPT
 unsigned info = cpuinfo_init();
 
+#ifdef CONFIG_AVX2_OPT
 if (info & CPUINFO_AVX2) {
 return 2;
 }
 #endif
-return 1;
+return info & CPUINFO_SSE2 ? 1 : 0;
 }
 
 #elif defined(__aarch64__) && defined(__ARM_NEON)
diff --git a/util/cpuinfo-i386.c b/util/cpuinfo-i386.c
index ca74ef04f54..90f92a42dc8 100644
--- a/util/cpuinfo-i386.c
+++ b/util/cpuinfo-i386.c
@@ -34,6 +34,7 @@ unsigned __attribute__((constructor)) cpuinfo_init(void)
 if (max >= 1) {
 __cpuid(1, a, b, c, d);
 
+info |= (d & bit_SSE2 ? CPUINFO_SSE2 : 0);
 info |= (c & bit_MOVBE ? CPUINFO_MOVBE : 0);
 info |= (c & bit_POPCNT ? CPUINFO_POPCNT : 0);
 info |= (c & bit_PCLMUL ? CPUINFO_PCLMUL : 0);
-- 
2.45.2

Re: [PATCH] vfio: container: Fix missing allocation of VFIOSpaprContainer

[Cédric Le Goater]

Shivaprasad,

On 5/9/24 9:14 PM, Shivaprasad G Bhat wrote:

The commit 6ad359ec29 "(vfio/spapr: Move prereg_listener into
spapr container)" began to use the newly introduced VFIOSpaprContainer
structure.

After several refactors, today the container_of(container,
VFIOSpaprContainer, ABC) is used when VFIOSpaprContainer is actually
not allocated. On PPC64 systems, this dereference is leading to corruption
showing up as glibc malloc assertion during guest start when using vfio.

Patch adds the missing allocation while also making the structure movement
to vfio common header file.

Fixes: 6ad359ec29 "(vfio/spapr: Move prereg_listener into spapr container)"
Signed-off-by: Shivaprasad G Bhat 


Could you please give vfio-9.1 a try ? Thanks,

C.

https://github.com/legoater/qemu/commits/vfio-9.1


---
  hw/vfio/container.c   |6 --
  hw/vfio/spapr.c   |6 --
  include/hw/vfio/vfio-common.h |6 ++
  3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276e..ecaf5786d9 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -539,6 +539,7 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
  {
  VFIOContainer *container;
  VFIOContainerBase *bcontainer;
+VFIOSpaprContainer *scontainer;
  int ret, fd;
  VFIOAddressSpace *space;

@@ -611,7 +612,8 @@ static int vfio_connect_container(VFIOGroup *group, 
AddressSpace *as,
  goto close_fd_exit;
  }

-container = g_malloc0(sizeof(*container));
+scontainer = g_malloc0(sizeof(*scontainer));
+container = &scontainer->container;
  container->fd = fd;
  bcontainer = &container->bcontainer;

@@ -675,7 +677,7 @@ unregister_container_exit:
  vfio_cpr_unregister_container(bcontainer);

  free_container_exit:
-g_free(container);
+g_free(scontainer);

  close_fd_exit:
  close(fd);
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 0d949bb728..78d218b7e7 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -24,12 +24,6 @@
  #include "qapi/error.h"
  #include "trace.h"

-typedef struct VFIOSpaprContainer {
-VFIOContainer container;
-MemoryListener prereg_listener;
-QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
-} VFIOSpaprContainer;
-
  static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
  {
  if (memory_region_is_iommu(section->mr)) {
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..010fa68ac6 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -82,6 +82,12 @@ typedef struct VFIOContainer {
  QLIST_HEAD(, VFIOGroup) group_list;
  } VFIOContainer;

+typedef struct VFIOSpaprContainer {
+VFIOContainer container;
+MemoryListener prereg_listener;
+QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
+} VFIOSpaprContainer;
+
  typedef struct VFIOHostDMAWindow {
  hwaddr min_iova;
  hwaddr max_iova;

Re: [PATCH v3 09/11] tests/migration-tests: migration_event_wait()

[Fabiano Rosas]

Peter Xu  writes:

> Introduce a small helper to wait for a migration event, generalized from
> the incoming migration path.  Make the helper easier to use by allowing it
> to keep waiting until the expected event is received.
>
> Signed-off-by: Peter Xu 

Reviewed-by: Fabiano Rosas 

Re: [PATCH v3 10/11] tests/migration-tests: Verify postcopy-recover-setup status

[Fabiano Rosas]

Peter Xu  writes:

> Making sure the postcopy-recover-setup status is present in the postcopy
> failure unit test.  Note that it only applies to src QEMU not dest.
>
> This also introduces the tiny but helpful migration_event_wait() helper.

Not anymore. I'll drop this line.

Reviewed-by: Fabiano Rosas 

Re: [PATCH v3 08/11] tests/migration-tests: Always enable migration events

[Fabiano Rosas]

Peter Xu  writes:

> Libvirt should always enable it, so it'll be nice qtest also cover that for
> all tests on both sides.  migrate_incoming_qmp() used to enable it only on
> dst, now we enable them on both, as we'll start to sanity check events even
> on the src QEMU.
>
> We'll need to leave the one in migrate_incoming_qmp(), because
> virtio-net-failover test uses that one only, and it relies on the events to
> work.
>
> Signed-off-by: Peter Xu 

Reviewed-by: Fabiano Rosas 

Re: [RFC PATCH v3 1/5] build-sys: Add rust feature option

[Paolo Bonzini]

On 6/19/24 22:13, Manos Pitsidianakis wrote:

Add options for Rust in meson_options.txt, meson.build, configure to
prepare for adding Rust code in the followup commits.

`rust` is a reserved meson name, so we have to use an alternative.
`with_rust` was chosen.

A cargo_wrapper.py script is added that is heavily based on the work of
Marc-André Lureau from 2021.

https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/

Signed-off-by: Marc-André Lureau 
Signed-off-by: Manos Pitsidianakis 


The cargo_wrapper.py script is not used yet, so it should be
delayed until it's used.

For the detection of the toolchain, I'd rather do everything in
configure since that's where the cross file is built.  Something like:

diff --git a/configure b/configure
index 8b6a2f16ceb..6412a1021c3 100755
--- a/configure
+++ b/configure
@@ -173,6 +173,8 @@ fi
 
 # default parameters

 container_engine="auto"
+rust_target_triple=""
+with_rust="no"
 cpu=""
 cross_compile="no"
 cross_prefix=""
@@ -201,6 +202,8 @@ for opt do
   --cross-prefix=*) cross_prefix="$optarg"
 cross_compile="yes"
   ;;
+  --cargo=*) CARGO="$optarg"
+  ;;
   --cc=*) CC="$optarg"
   ;;
   --cxx=*) CXX="$optarg"
@@ -317,6 +322,8 @@ windmc="${WINDMC-${cross_prefix}windmc}"
 pkg_config="${PKG_CONFIG-${cross_prefix}pkg-config}"
 sdl2_config="${SDL2_CONFIG-${cross_prefix}sdl2-config}"
 
+cargo="${CARGO-cargo}"

+
 check_define() {
 cat > $TMPC < 
+##

+# detect rust triples
+
+if test "$with_rust" = yes; then
+  $CARGO -vV > "${TMPDIR1}/${TMPB}.out"
+  if test $? != 0; then
+error_exit "could not execute cargo binary \"$CARGO\""
+  fi
+  rust_host_triple=$(sed -n 's/^host: //p' "${TMPDIR1}/${TMPB}.out")
+  if test "$rust_target_triple" = ""; then
+rust_target_triple=$rust_host_triple
+  fi
+fi
+
 ##
 # functions to probe cross compilers
 
@@ -1604,6 +1639,10 @@ if test "$container" != no; then

 echo "RUNC=$runc" >> $config_host_mak
 fi
 echo "SUBDIRS=$subdirs" >> $config_host_mak
+if test "$with_rust" = yes; then
+  echo "RUST_HOST_TRIPLE=$rust_host_triple" >> $config_host_mak
+  echo "RUST_TARGET_TRIPLE=$rust_target_triple" >> $config_host_mak
+fi
 echo "PYTHON=$python" >> $config_host_mak
 echo "MKVENV_ENSUREGROUP=$mkvenv ensuregroup $mkvenv_online_flag" >> 
$config_host_mak
 echo "GENISOIMAGE=$genisoimage" >> $config_host_mak
@@ -1731,6 +1770,13 @@ if test "$skip_meson" = no; then
   echo "c = [$(meson_quote $cc $CPU_CFLAGS)]" >> $cross
   test -n "$cxx" && echo "cpp = [$(meson_quote $cxx $CPU_CFLAGS)]" >> $cross
   test -n "$objcc" && echo "objc = [$(meson_quote $objcc $CPU_CFLAGS)]" >> 
$cross
+  if test "$with_rust" = yes; then
+if test "$rust_host_triple" != "$rust_target_triple"; then
+  echo "cargo = [$(meson_quote $cargo --target "$rust_target_triple")]" >> 
$cross
+else
+  echo "cargo = [$(meson_quote $cargo)]" >> $cross
+fi
+  fi
   echo "ar = [$(meson_quote $ar)]" >> $cross
   echo "dlltool = [$(meson_quote $dlltool)]" >> $cross
   echo "nm = [$(meson_quote $nm)]" >> $cross
diff --git a/meson.build b/meson.build
index c5360fbd299..ad7dbc0d641 100644
--- a/meson.build
+++ b/meson.build
@@ -290,6 +290,11 @@ foreach lang : all_languages
   endif
 endforeach
 
+cargo = not_found

+if 'RUST_TARGET_TRIPLE' in config_host
+  cargo = find_program('cargo', required: true)
+endif
+
 # default flags for all hosts
 # We use -fwrapv to tell the compiler that we require a C dialect where
 # left shift of signed integers is well defined and has the expected
@@ -4239,6 +4244,10 @@ if 'objc' in all_languages
 else
   summary_info += {'Objective-C compiler': false}
 endif
+summary_info += {'Rust support':  cargo.found()}
+if cargo.found() and config_host['RUST_TARGET_TRIPLE']) != 
config_host['RUST_HOST_TRIPLE']
+  summary_info += {'Rust target': config_host['RUST_TARGET_TRIPLE']}
+endif
 option_cflags = (get_option('debug') ? ['-g'] : [])
 if get_option('optimization') != 'plain'
   option_cflags += ['-O' + get_option('optimization')]

How to use designware-root-port and designware-root-host devices ?

[Arthur Tumanyan]

Hi all,

My question may sound stupid, however... Currently I'm trying to make
available designware-root-{port,host} devices  in linux when I run it in
qemu.

I try the following way to run:

qemu-system-arm -M virt -m 2G \
 -kernel images/Image \
 -append "rootwait root=/dev/vda ro" \
 -drive file=images/rootfs.ext2,format=raw,id=hd0 \
 -device designware-root-port,id=rp0,chassis=1,slot=0,bus=pcie.0,addr=1
\
 -device e1000,netdev=net0,mac=52:54:00:12:34:56,bus=rp0,addr=0 \
 -netdev user,id=net0

but it seems designware device is not enabled by default: qemu-system-arm:
-device designware-root-port,id=rp0,chassis=1,slot=0,bus=pcie.0,addr=1:
'designware-root-port' is not a valid device model name

when I enable it from Kconfig/meson.build it says the device is already
registered and exits with abort().

>From the other hand the device is declared as non pluggable: dc->user_creatable
= false;

Can you please help me to use designware-root-host/port devices ?

Thanks in advance,
Arthur

Re: [PATCH] hw/timer/a9gtimer: Handle QTest mode in a9_gtimer_get_current_cpu

[Edgar E. Iglesias]

On Thu, Jun 20, 2024 at 12:25:51PM +0200, Philippe Mathieu-Daudé wrote:
> On 20/6/24 12:10, Peter Maydell wrote:
> > On Tue, 18 Jun 2024 at 15:51, Philippe Mathieu-Daudé  
> > wrote:
> > > 
> > > On 18/6/24 16:40, Zheyu Ma wrote:
> > > > This commit updates the a9_gtimer_get_current_cpu() function to handle
> > > > cases where QTest is enabled. When QTest is used, it returns 0 instead
> > > > of dereferencing the current_cpu, which can be NULL. This prevents the
> > > > program from crashing during QTest runs.
> > > > 
> > > > Reproducer:
> > > > cat << EOF | qemu-system-aarch64 -display \
> > > > none -machine accel=qtest, -m 512M -machine npcm750-evb -qtest stdio
> > > > writel 0xf03fe20c 0x26d7468c
> > > > EOF
> > > > 
> > > > Signed-off-by: Zheyu Ma 
> > > > ---
> > > >hw/timer/a9gtimer.c | 5 +
> > > >1 file changed, 5 insertions(+)
> 
> 
> > > >if (current_cpu->cpu_index >= s->num_cpu) {
> > > 
> > > That said, such accesses of @current_cpu from hw/ are dubious.
> > 
> > True, but I'm not sure we ever settled on the right way to avoid
> > them, did we?
> 
> No we didn't, it is still in my TODO list; we might discuss it
> when I post my RFC.
>

Yeah, this way of getting the core id is a problem when having multiple
ARM CPU subsystems (and for heterogenous cores).

IIRC, when I looked at what the GIC v2 HW does, the GIC exposes an AMBA
port for each CPU. In my mental model that would translate to exposing
multiple Memory Reginos (sysbus_init_mmio) and mapping the appropriate
device MR to each CPU AddressSpace.

We could also do it with memory attributes but I don't think the
master Ids are standardised enough to extract a core-index from
with out having SoC specific code,, at least not accross Xilinx devices.

I never looked at newer GIC versions nor the mmio mapped timers
though...

Cheers,
Edgar

[PATCH 1/2] migration: Implement dirty ring

[Shota Imamura]

This commit implements the dirty ring as an alternative dirty tracking
method to the dirty bitmap.

While the dirty ring has already been implemented in accel/kvm using KVM's
dirty ring, it was designed to set bits in the ramlist and ramblock bitmap.
This commit introduces a new dirty ring to replace the bitmap, allowing the
use of the dirty ring even without KVM. When using KVM's dirty ring, this
implementation maximizes its effectiveness.

To enable the dirty ring, specify the startup option
"-migration dirty-logging=ring,dirty-ring-size=N". To use the bitmap,
either specify nothing or "-migration dirty-logging=bitmap". If the dirty
ring becomes full, it falls back to the bitmap for that round.

Signed-off-by: Shota Imamura 
---
 accel/kvm/kvm-all.c|  36 -
 include/exec/ram_addr.h| 131 +++--
 include/exec/ramlist.h |  48 
 include/migration/misc.h   |   4 +-
 include/qemu/bitops.h  |  23 ++
 migration/migration-hmp-cmds.c |   2 +
 migration/migration.c  |  27 ++-
 migration/migration.h  |   6 ++
 migration/ram.c| 127 
 qemu-options.hx|  29 
 system/physmem.c   | 128 +++-
 system/vl.c|  63 +++-
 12 files changed, 597 insertions(+), 27 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 854cb86b22..91410d682f 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -667,7 +667,13 @@ static void kvm_dirty_ring_mark_page(KVMState *s, uint32_t 
as_id,
 return;
 }
 
-set_bit(offset, mem->dirty_bmap);
+if (!test_and_set_bit(offset, mem->dirty_bmap) &&
+mem->flags & KVM_MEM_LOG_DIRTY_PAGES &&
+migration_has_dirty_ring()) {
+unsigned long pfn =
+(mem->ram_start_offset >> TARGET_PAGE_BITS) + offset;
+ram_list_enqueue_dirty(pfn);
+}
 }
 
 static bool dirty_gfn_is_dirtied(struct kvm_dirty_gfn *gfn)
@@ -1675,6 +1681,34 @@ static void kvm_log_sync_global(MemoryListener *l, bool 
last_stage)
 /* Flush all kernel dirty addresses into KVMSlot dirty bitmap */
 kvm_dirty_ring_flush();
 
+if (!ram_list_enqueue_dirty_full()) {
+cpu_physical_memory_set_dirty_ring(ram_list_get_enqueue_dirty());
+
+if (s->kvm_dirty_ring_with_bitmap && last_stage) {
+kvm_slots_lock();
+for (i = 0; i < s->nr_slots; i++) {
+mem = &kml->slots[i];
+if (mem->memory_size &&
+mem->flags & KVM_MEM_LOG_DIRTY_PAGES &&
+kvm_slot_get_dirty_log(s, mem)) {
+kvm_slot_sync_dirty_pages(mem);
+}
+}
+kvm_slots_unlock();
+}
+
+kvm_slots_lock();
+for (i = 0; i < s->nr_slots; i++) {
+mem = &kml->slots[i];
+if (mem->memory_size && mem->flags & KVM_MEM_LOG_DIRTY_PAGES) {
+kvm_slot_reset_dirty_pages(mem);
+}
+}
+kvm_slots_unlock();
+
+return;
+}
+
 /*
  * TODO: make this faster when nr_slots is big while there are
  * only a few used slots (small VMs).
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 891c44cf2d..1eaebcf22f 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -27,6 +27,7 @@
 #include "exec/ramblock.h"
 #include "exec/exec-all.h"
 #include "qemu/rcu.h"
+#include "migration/misc.h"
 
 extern uint64_t total_dirty_pages;
 
@@ -282,7 +283,11 @@ static inline void 
cpu_physical_memory_set_dirty_flag(ram_addr_t addr,
 
 blocks = qatomic_rcu_read(&ram_list.dirty_memory[client]);
 
-set_bit_atomic(offset, blocks->blocks[idx]);
+if (!test_and_set_bit_atomic(offset, blocks->blocks[idx]) &&
+migration_has_dirty_ring() &&
+client == DIRTY_MEMORY_MIGRATION) {
+ram_list_enqueue_dirty(page);
+}
 }
 
 static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
@@ -313,8 +318,24 @@ static inline void 
cpu_physical_memory_set_dirty_range(ram_addr_t start,
 unsigned long next = MIN(end, base + DIRTY_MEMORY_BLOCK_SIZE);
 
 if (likely(mask & (1 << DIRTY_MEMORY_MIGRATION))) {
-bitmap_set_atomic(blocks[DIRTY_MEMORY_MIGRATION]->blocks[idx],
-  offset, next - page);
+if (!migration_has_dirty_ring() ||
+ram_list_enqueue_dirty_full()) {
+use_dirty_bmap:
+bitmap_set_atomic(
+blocks[DIRTY_MEMORY_MIGRATION]->blocks[idx],
+offset,
+next - page);
+} else {
+for (unsigned long p = page; p < next; p++) {
+if (!test_and_set_bit_atomic(
+p % DIRTY_MEMORY_BL

[PATCH 2/2] qtest/migration: Add dirty ring tests

[Shota Imamura]

This commit adds tests for migration using the dirty ring. To avoid
confusion with KVM's dirty ring, use_dirty_ring has been changed to
use_kvm_dirty_ring, and use_qemu_dirty_ring has been added.

Signed-off-by: Shota Imamura 
---
 tests/qtest/migration-test.c | 78 
 1 file changed, 70 insertions(+), 8 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 0dccb4beff..a8151b9470 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -556,7 +556,8 @@ typedef struct {
 /* only launch the target process */
 bool only_target;
 /* Use dirty ring if true; dirty logging otherwise */
-bool use_dirty_ring;
+bool use_kvm_dirty_ring;
+bool use_qemu_dirty_ring;
 const char *opts_source;
 const char *opts_target;
 /* suspend the src before migrating to dest. */
@@ -675,6 +676,7 @@ static int test_migrate_start(QTestState **from, QTestState 
**to,
 g_autofree char *shmem_opts = NULL;
 g_autofree char *shmem_path = NULL;
 const char *kvm_opts = NULL;
+const char *migration_ops = NULL;
 const char *arch = qtest_get_arch();
 const char *memory_size;
 const char *machine_alias, *machine_opts = "";
@@ -754,10 +756,16 @@ static int test_migrate_start(QTestState **from, 
QTestState **to,
 memory_size, shmem_path);
 }
 
-if (args->use_dirty_ring) {
+if (args->use_kvm_dirty_ring) {
 kvm_opts = ",dirty-ring-size=4096";
 }
 
+if (args->use_qemu_dirty_ring) {
+migration_ops = "dirty-logging=ring,dirty-ring-size=32768";
+} else {
+migration_ops = "dirty-logging=bitmap";
+}
+
 if (!qtest_has_machine(machine_alias)) {
 g_autofree char *msg = g_strdup_printf("machine %s not supported", 
machine_alias);
 g_test_skip(msg);
@@ -774,10 +782,12 @@ static int test_migrate_start(QTestState **from, 
QTestState **to,
  "-name source,debug-threads=on "
  "-m %s "
  "-serial file:%s/src_serial "
+ "-migration %s "
  "%s %s %s %s %s",
  kvm_opts ? kvm_opts : "",
  machine, machine_opts,
  memory_size, tmpfs,
+ migration_ops,
  arch_opts ? arch_opts : "",
  arch_source ? arch_source : "",
  shmem_opts ? shmem_opts : "",
@@ -1796,12 +1806,27 @@ static void test_precopy_unix_suspend_notlive(void)
 test_precopy_common(&args);
 }
 
-static void test_precopy_unix_dirty_ring(void)
+static void test_precopy_unix_qemu_dirty_ring(void)
 {
 g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
 MigrateCommon args = {
 .start = {
-.use_dirty_ring = true,
+.use_qemu_dirty_ring = true,
+},
+.listen_uri = uri,
+.connect_uri = uri,
+.live = true,
+};
+
+test_precopy_common(&args);
+}
+
+static void test_precopy_unix_kvm_dirty_ring(void)
+{
+g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+MigrateCommon args = {
+.start = {
+.use_kvm_dirty_ring = true,
 },
 .listen_uri = uri,
 .connect_uri = uri,
@@ -1815,6 +1840,22 @@ static void test_precopy_unix_dirty_ring(void)
 test_precopy_common(&args);
 }
 
+static void test_precopy_unix_kvm_and_qemu_dirty_ring(void)
+{
+g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+MigrateCommon args = {
+.start = {
+.use_kvm_dirty_ring = true,
+.use_qemu_dirty_ring = true,
+},
+.listen_uri = uri,
+.connect_uri = uri,
+.live = true,
+};
+
+test_precopy_common(&args);
+}
+
 #ifdef CONFIG_GNUTLS
 static void test_precopy_unix_tls_psk(void)
 {
@@ -1942,6 +1983,21 @@ static void test_precopy_file(void)
 test_file_common(&args, true);
 }
 
+static void test_precopy_file_dirty_ring(void)
+{
+g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
+   FILE_TEST_FILENAME);
+MigrateCommon args = {
+.start = {
+.use_qemu_dirty_ring = true,
+},
+.connect_uri = uri,
+.listen_uri = "defer",
+};
+
+test_file_common(&args, true);
+}
+
 static void file_offset_finish_hook(QTestState *from, QTestState *to,
 void *opaque)
 {
@@ -3298,7 +3354,7 @@ static void test_migrate_dirty_limit(void)
 MigrateCommon args = {
 .start = {
 .hide_stderr = true,
-.use_dirty_ring = true,
+.use_kvm_dirty_ring = true,
 },
 .listen_uri = uri,
 .connect_uri = uri,
@@ -3342,7 

[PATCH 0/2] Implement dirty ring for pre-copy migration

[Shota Imamura]

This patch series introduces the dirty ring as an additional method for
dirty tracking, alongside the existing dirty bitmap.

Shota Imamura (2):
  migration: Implement dirty ring
  qtest/migration: Add dirty ring tests

 accel/kvm/kvm-all.c|  36 -
 include/exec/ram_addr.h| 131 +++--
 include/exec/ramlist.h |  48 
 include/migration/misc.h   |   4 +-
 include/qemu/bitops.h  |  23 ++
 migration/migration-hmp-cmds.c |   2 +
 migration/migration.c  |  27 ++-
 migration/migration.h  |   6 ++
 migration/ram.c| 127 
 qemu-options.hx|  29 
 system/physmem.c   | 128 +++-
 system/vl.c|  63 +++-
 tests/qtest/migration-test.c   |  78 ++--
 13 files changed, 667 insertions(+), 35 deletions(-)

-- 
2.34.1

Re: [PATCH v4 2/5] ppc/pnv: Extend SPI model

[Chalapathi V]

On 20-06-2024 03:44, Miles Glenn wrote:

Hi Chalapathi,

I can't say I have a great understanding of this IBM SPI controller,
but I did find some places for improvement, mostly dealing with the use
of "magic numbers" throughout the code.  Please see comments below.

Thanks,

Glenn


Hello Glenn,

Thank You for the review and suggestions. I will address them and update 
in next revision ASAP.


Thank You,

Chalapathi



On Mon, 2024-06-17 at 11:54 -0500, Chalapathi V wrote:

In this commit SPI shift engine and sequencer logic is implemented.
Shift engine performs serialization and de-serialization according to
the
control by the sequencer and according to the setup defined in the
configuration registers. Sequencer implements the main control logic
and
FSM to handle data transmit and data receive control of the shift
engine.

Signed-off-by: Chalapathi V 
---
  include/hw/ssi/pnv_spi.h |   27 +
  hw/ssi/pnv_spi.c | 1039
++
  hw/ssi/trace-events  |   15 +
  3 files changed, 1081 insertions(+)

diff --git a/include/hw/ssi/pnv_spi.h b/include/hw/ssi/pnv_spi.h
index 71c53d4a17..21fbfcb69c 100644
--- a/include/hw/ssi/pnv_spi.h
+++ b/include/hw/ssi/pnv_spi.h
@@ -8,6 +8,14 @@
   * This model Supports a connection to a single SPI responder.
   * Introduced for P10 to provide access to SPI seeproms, TPM, flash
device
   * and an ADC controller.
+ *
+ * All SPI function control is mapped into the SPI register space to
enable
+ * full control by firmware.
+ *
+ * SPI Controller has sequencer and shift engine. The SPI shift
engine
+ * performs serialization and de-serialization according to the
control by
+ * the sequencer and according to the setup defined in the
configuration
+ * registers and the SPI sequencer implements the main control
logic.
   */
  #include "hw/ssi/ssi.h"
  #include "hw/sysbus.h"
@@ -50,6 +58,25 @@ typedef struct PnvSpi {
  MemoryRegionxscom_spic_regs;
  /* SPI object number */
  uint32_tspic_num;
+uint8_t transfer_len;
+uint8_t responder_select;
+/* To verify if shift_n1 happens prior to shift_n2 */
+boolshift_n1_done;
+/* Loop counter for branch operation opcode Ex/Fx */
+uint8_t loop_counter_1;
+uint8_t loop_counter_2;
+/* N1/N2_bits specifies the size of the N1/N2 segment of a frame
in bits.*/
+uint8_t N1_bits;
+uint8_t N2_bits;
+/* Number of bytes in a payload for the N1/N2 frame segment.*/
+uint8_t N1_bytes;
+uint8_t N2_bytes;
+/* Number of N1/N2 bytes marked for transmit */
+uint8_t N1_tx;
+uint8_t N2_tx;
+/* Number of N1/N2 bytes marked for receive */
+uint8_t N1_rx;
+uint8_t N2_rx;
  
  /* SPI registers */

  uint64_tregs[PNV_SPI_REGS];
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index da9e3925dd..b8f4370525 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -14,9 +14,1040 @@
  #include "hw/ssi/pnv_spi_regs.h"
  #include "hw/ssi/ssi.h"
  #include 
+#include 

I think the only reason you are including math.h is because you're
using the `ceil` function below.  And, since you are only using it to
operate on integers, it is not really necessary.  See comment below on
how to do the same thing with integer math.


  #include "hw/irq.h"
  #include "trace.h"
  
+/* PnvXferBuffer */

+typedef struct PnvXferBuffer {
+
+uint32_tlen;
+uint8_t*data;
+
+} PnvXferBuffer;
+
+/* pnv_spi_xfer_buffer_methods */
+static PnvXferBuffer *pnv_spi_xfer_buffer_new(void)
+{
+PnvXferBuffer *payload = g_malloc0(sizeof(*payload));
+
+return payload;
+}
+
+static void pnv_spi_xfer_buffer_free(PnvXferBuffer *payload)
+{
+free(payload->data);
+free(payload);
+}
+
+static uint8_t *pnv_spi_xfer_buffer_write_ptr(PnvXferBuffer
*payload,
+uint32_t offset, uint32_t length)
+{
+if (payload->len < (offset + length)) {
+payload->len = offset + length;
+payload->data = g_realloc(payload->data, payload->len);
+}
+return &payload->data[offset];
+}
+
+static bool does_rdr_match(PnvSpi *s)
+{
+/*
+ * According to spec, the mask bits that are 0 are compared and
the
+ * bits that are 1 are ignored.
+ */
+uint16_t rdr_match_mask = GETFIELD(SPI_MM_RDR_MATCH_MASK,
+s->regs[SPI_MM_REG]);
+uint16_t rdr_match_val = GETFIELD(SPI_MM_RDR_MATCH_VAL,
+s->regs[SPI_MM_REG]);
+
+if ((~rdr_match_mask & rdr_match_val) == ((~rdr_match_mask) &
+GETFIELD(PPC_BITMASK(48, 63), s-

regs[SPI_RCV_DATA_REG]))) {

+return true;
+}
+return false;
+}
+
+static uint8_t get_from_offset(PnvSpi *s, uint8_t offset)
+{
+uint8_t byte;
+
+/*
+ * Offset is an index between 0 and PNV_SPI_REG_SIZE - 1
+ * Check the offset before using it.
+ */
+if (offset < PNV_SP

Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST

[Markus Armbruster]

Markus Armbruster  writes:

> John Snow  writes:

[...]

>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index b3de1fb6b3a..57598331c5c 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json

[...]

>> @@ -631,8 +632,8 @@
>>  # Errors:
>>  # - If hybrid suspend is not supported, Unsupported
>>  #
>> -# Notes: It's strongly recommended to issue the guest-sync command
>> -# before sending commands when the guest resumes
>> +# .. note:: It's strongly recommended to issue the guest-sync command
>> +#before sending commands when the guest resumes.
>>  #
>>  # Since: 1.1
>>  ##
>> @@ -1461,16 +1462,15 @@
>>  # * POSIX: as defined by os-release(5)
>>  # * Windows: contains string "server" or "client"
>>  #
>> -# Notes: On POSIX systems the fields @id, @name, @pretty-name,
>> -# @version, @version-id, @variant and @variant-id follow the
>> -# definition specified in os-release(5). Refer to the manual page
>> -# for exact description of the fields.  Their values are taken
>> -# from the os-release file.  If the file is not present in the
>> -# system, or the values are not present in the file, the fields
>> -# are not included.
>> +# .. note:: On POSIX systems the fields @id, @name, @pretty-name,
>> +#@version, @version-id, @variant and @variant-id follow the
>> +#definition specified in os-release(5). Refer to the manual page for
>> +#exact description of the fields.  Their values are taken from the
>> +#os-release file.  If the file is not present in the system, or the
>> +#values are not present in the file, the fields are not included.
>>  #
>> -# On Windows the values are filled from information gathered from
>> -# the system.
>> +#On Windows the values are filled from information gathered from
>> +#the system.
>
> Please don't change the indentation here.  I get the same output with
>
>   @@ -1461,7 +1462,7 @@
># * POSIX: as defined by os-release(5)
># * Windows: contains string "server" or "client"
>#
>   -# Notes: On POSIX systems the fields @id, @name, @pretty-name,
>   +# .. note:: On POSIX systems the fields @id, @name, @pretty-name,
># @version, @version-id, @variant and @variant-id follow the
># definition specified in os-release(5). Refer to the manual page
># for exact description of the fields.  Their values are taken

I'm blind.  Actually, you change indentation of subsequent lines from 4
to 3 *everywhere*.  I guess you do that to make subsequent lines line up
with the directive, here "note".

Everywhere else, we indent such lines by 4.  Hmm.  How terrible would it
be not to mess with the alignment?

If we want to use 3 for directives, is it worth pointing out in the
commit message?

[...]

Re: [RFC PATCH v3 1/5] build-sys: Add rust feature option

[Alex Bennée]

Manos Pitsidianakis  writes:

> Add options for Rust in meson_options.txt, meson.build, configure to
> prepare for adding Rust code in the followup commits.
>
> `rust` is a reserved meson name, so we have to use an alternative.
> `with_rust` was chosen.
>
> A cargo_wrapper.py script is added that is heavily based on the work of
> Marc-André Lureau from 2021.
>
> https://patchew.org/QEMU/20210907121943.3498701-1-marcandre.lur...@redhat.com/
>
> Signed-off-by: Marc-André Lureau 
> Signed-off-by: Manos Pitsidianakis 

>  
> +with_rust="auto"
> +with_rust_target_triple=""
> +
>  ar="${AR-${cross_prefix}ar}"
>  as="${AS-${cross_prefix}as}"
>  ccas="${CCAS-$cc}"
> @@ -760,6 +763,12 @@ for opt do
>;;
>--gdb=*) gdb_bin="$optarg"
>;;
> +  --enable-with-rust) with_rust=enabled
> +  ;;
> +  --disable-with-rust) with_rust=disabled
> +  ;;
> +  --with-rust-target-triple=*) with_rust_target_triple="$optarg"
> +  ;;
># everything else has the same name in configure and meson
>--*) meson_option_parse "$opt" "$optarg"
>;;
> @@ -1796,6 +1805,8 @@ if test "$skip_meson" = no; then
>test -n "${LIB_FUZZING_ENGINE+xxx}" && meson_option_add 
> "-Dfuzzing_engine=$LIB_FUZZING_ENGINE"
>test "$plugins" = yes && meson_option_add "-Dplugins=true"
>test "$tcg" != enabled && meson_option_add "-Dtcg=$tcg"
> +  test "$with_rust" != enabled && meson_option_add "-Dwith_rust=$with_rust"
> +  test "$with_rust_target_triple" != "" && meson_option_add 
> "-Dwith_rust_target_triple=$with_rust_target_triple"
>run_meson() {
>  NINJA=$ninja $meson setup "$@" "$PWD" "$source_path"
>}


> +summary_info += {'Rust support':  with_rust}
> +if with_rust and get_option('with_rust_target_triple') != ''
> +  summary_info += {'Rust target': get_option('with_rust_target_triple')}
> +endif


I wonder if we should display the auto-probed triple here as well, not
just when its been overridden?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 06/12] tests/data/acpi/virt: Move ACPI tables under aarch64

[Igor Mammedov]

On Wed, 19 Jun 2024 23:30:35 +0530
Sunil V L  wrote:

> On Wed, Jun 19, 2024 at 05:20:50AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 19, 2024 at 11:17:43AM +0200, Igor Mammedov wrote:  
> > > On Mon, 27 May 2024 20:46:29 +0530
> > > Sunil V L  wrote:
> > >   
> > > > On Mon, May 27, 2024 at 12:12:10PM +0200, Philippe Mathieu-Daudé wrote: 
> > > >  
> > > > > Hi Sunil,
> > > > > 
> > > > > On 24/5/24 08:14, Sunil V L wrote:
> > > > > > Since virt is a common machine name across architectures like ARM64 
> > > > > > and
> > > > > > RISC-V, move existing ARM64 ACPI tables under aarch64 folder so that
> > > > > > RISC-V tables can be added under riscv64 folder in future.
> > > > > > 
> > > > > > Signed-off-by: Sunil V L 
> > > > > > Reviewed-by: Alistair Francis 
> > > > > > ---
> > > > > >   tests/data/acpi/virt/{ => aarch64}/APIC | Bin
> > > > > 
> > > > > The usual pattern is {target}/{machine}, so instead of:
> > > > > 
> > > > >   microvm/
> > > > >   pc/
> > > > >   q35/
> > > > >   virt/aarch64/
> > > > >   virt/riscv64/
> > > > > 
> > > > > (which is odd because q35 is the x86 'virt'), I'd rather see:
> > > > > 
> > > > >   x86/microvm/
> > > > >   x86/pc/
> > > > >   x86/q35/
> > > > >   aarch64/virt/
> > > > >   riscv64/virt/
> > > > > 
> > > > > Anyhow just my 2 cents, up to the ACPI maintainers :)
> > > > > 
> > > > Hi Phil,
> > > > 
> > > > Your suggestion does make sense to me. Let me wait for feedback from
> > > > ARM/ACPI maintainers.  
> > > 
> > > I'd prefer  {target}/{machine} hierarchy like Philippe suggests  
> > 
> > Agreed.
> >   
> Thanks for the confirmation!. Let me send the updated version soon.
> 
> Moving pc/q35/microvm also under new x86 would need many changes in
> bios-table-test.c. So, the question is, are you ok to combine x86
> changes as well in this series or prefer to it later in separate series?

it should be fine ok to include x86 changes here as well.

I'd basically split previous patch on path altering part and a 2nd adding
 .arch = "aarch64"

then 3rd doing the same for x86

as for this patch, I'd include all blobs movement here.

> 
> Thanks,
> Sunil
> 

Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST

[Markus Armbruster]

John Snow  writes:

> We do not need a dedicated section for notes. By eliminating a specially
> parsed section, these notes can be treated as normal rST paragraphs in
> the new QMP reference manual, and can be placed and styled much more
> flexibly.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting
> a new syntax. The exact formatting to use is a matter of taste, but a
> good candidate is simply:
>
> .. note:: lorem ipsum ...
>
> ... but there are other choices, too. The Sphinx readthedocs theme
> offers theming for the following forms (capitalization unimportant); all
> are adorned with a (!) symbol in the title bar for rendered HTML docs.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> These are rendered in orange:
>
> .. Attention:: ...
> .. Caution:: ...
> .. WARNING:: ...
>
> These are rendered in red:
>
> .. DANGER:: ...
> .. Error:: ...
>
> These are rendered in green:
>
> .. Hint:: ...
> .. Important:: ...
> .. Tip:: ...
>
> These are rendered in blue:
>
> .. Note:: ...
> .. admonition:: custom title
>
>admonition body text
>
> This patch uses ".. note::" almost everywhere,

Not mentioned, and may or may not be worth mentioning: both "Note:" and
"Notes:" become ".. note::", which renders as "Note".  One instance
quoted below.

No objection to the change; you obviously double-checked it reads okay
that way.

>with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow 
> Acked-by: Stefan Hajnoczi  [for block*.json]

[...]

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index df5e07debd2..cacedfb771c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json

[...]

> @@ -6048,9 +6048,9 @@
>  #
>  # @name: the name of the internal snapshot to be created
>  #
> -# Notes: In transaction, if @name is empty, or any snapshot matching
> -# @name exists, the operation will fail.  Only some image formats
> -# support it, for example, qcow2, and rbd.
> +# .. note:: In transaction, if @name is empty, or any snapshot matching
> +#@name exists, the operation will fail.  Only some image formats
> +#support it, for example, qcow2, and rbd.
>  #
>  # Since: 1.7
>  ##

[...]

Re: [PATCH] target/arm/helper: Fix timer interrupt masking when HCR_EL2.E2H == 0

[Florian Lugou]

On Thu, Jun 20, 2024 at 11:43:17AM +0100, Peter Maydell wrote:
> On Sat, 15 Jun 2024 at 19:56, Florian Lugou  
> wrote:
> >
> > CNTHCTL_EL2 based masking of timer interrupts was introduced in
> > f6fc36deef6abcee406211f3e2f11ff894b87fa4. This masking was however
> > effective no matter whether EL2 was enabled in the current security
> > state or not, contrary to arm specification.
> >
> > Signed-off-by: Florian Lugou 
> > ---
> >  target/arm/helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/arm/helper.c b/target/arm/helper.c
> > index ce31957235..60e2344c68 100644
> > --- a/target/arm/helper.c
> > +++ b/target/arm/helper.c
> > @@ -2684,7 +2684,8 @@ static void gt_update_irq(ARMCPU *cpu, int timeridx)
> >   * If bit CNTHCTL_EL2.CNT[VP]MASK is set, it overrides IMASK.
> >   * It is RES0 in Secure and NonSecure state.
> >   */
> > -if ((ss == ARMSS_Root || ss == ARMSS_Realm) &&
> > +if ((arm_hcr_el2_eff(env) & HCR_E2H) &&
> > +(ss == ARMSS_Root || ss == ARMSS_Realm) &&
> 
> When the architecture says "is EL2 enabled in the current security state"
> it doesn't mean "is HCR_EL2.E2H set?", it means "is this either 
> NonSecure/Realm
> or else is SCR_EL2.EEL2 set?". Compare the pseudocode EL2Enabled()
> and QEMU's arm_is_el2_enabled() and arm_is_el2_enabled_secstate() functions.
> This doesn't mean much in Root state, and for Realm state EL2 is always
> enabled (assuming it is implemented).
> 
> For this timer check, we're doing I think the same thing as the
> pseudocode AArch64.CheckTimerConditions(), which does:
> 
>   if (IsFeatureImplemented(FEAT_RME) && ss IN {SS_Root, SS_Realm} &&
>   CNTHCTL_EL2.CNTPMASK == '1') then
>  imask = '1';
> 
> so I'm inclined to say that our current implementation in QEMU is correct.

Indeed. I got confused with the specification, my apologies.

I am facing an issue with QEMU freezing waiting for a timer interrupt when
running with -icount shift=0,sleep=off. Bissection has shown that the issue
appeared with f6fc36deef6abcee406211f3e2f11ff894b87fa4.

Further testing suggests that the issue may come from gt_recalc_timer. Calling
gt_update_irq before timer_mod (as it was done before f6fc36deef6a) rather than
at the end of the function solves the issue. Is it possible that timer_mod
relies on cpu->gt_timer_outputs, which has not been modified at this point to
reflect the timer triggering?

> 
> >  ((timeridx == GTIMER_VIRT && (cnthctl & R_CNTHCTL_CNTVMASK_MASK)) 
> > ||
> >   (timeridx == GTIMER_PHYS && (cnthctl & 
> > R_CNTHCTL_CNTPMASK_MASK {
> >  irqstate = 0;
> > --
> 
> thanks
> -- PMM

Best,

-- 
Florian


signature.asc
Description: PGP signature

[PATCH] docs: add precision about capstone for execlog plugin

[Alexandre Iooss]

Some people are wondering why they get an empty string as disassembly.
Most of the time, they configured QEMU without Capstone support.
Let's document this behaviour to help users.

Signed-off-by: Alexandre Iooss 
---
 docs/devel/tcg-plugins.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index 9cc09d8c3d..f7d7b9e3a4 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -539,7 +539,9 @@ which will output an execution trace following this 
structure::
   0, 0xd34, 0xf9c8f000, "bl #0x10c8"
   0, 0x10c8, 0xfff96c43, "ldr r3, [r0, #0x44]", load, 0x20e4, RAM
 
-the output can be filtered to only track certain instructions or
+Please note that you need to configure QEMU with Capstone support to get 
disassembly.
+
+The output can be filtered to only track certain instructions or
 addresses using the ``ifilter`` or ``afilter`` options. You can stack the
 arguments if required::
 
-- 
2.30.2

Re: [PATCH-for-9.0? 0/2] hw/sd/sdcard: Avoid OOB in sd_read_byte()

[Philippe Mathieu-Daudé]

On 8/4/24 16:17, Philippe Mathieu-Daudé wrote:

Since this is Fix day, I went over this old bug:
https://gitlab.com/qemu-project/qemu/-/issues/487
It happens to be a QEMU implementation detail not
really related to the spec.

Philippe Mathieu-Daudé (2):
   hw/sd/sdcard: Avoid OOB in sd_read_byte() during unexpected CMD switch


First patch queued.

Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency

[Richard Henderson]

On 6/19/24 13:13, Manos Pitsidianakis wrote:

+# FIXME: These are the latest stable versions, refine to actual minimum ones.
+msrv = {
+  'rustc': '1.79.0',
+  'cargo': '1.79.0',
+  'bindgen': '0.69.4',
+}


A note for other rust newbies:

These versions are pretty darn close to actual minima.  Ubuntu 24.04 packages rust 1.77, 
which does not support (but has a warning reserving syntax for)



+println!("cargo::rerun-if-env-changed=MESON_BUILD_DIR");


Since even the newest distros will not have current enough rust versions, we must rely on 
'rustup'.  This may be available even on older distros; for instance Ubuntu 22.04 has 
rustup via 'snap'.


I think this is good enough for rust development within qemu, but it may require that the 
configure switch be opt-in: default no rather than default auto.



r~

[PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Zheyu Ma]

ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
function. This issue occurred when reading beyond the bounds of the
reg_table.

To enhance the safety and maintainability of the Aspeed GPIO code, this commit
introduces a reg_table_count member to the AspeedGPIOClass structure. This
change ensures that the size of the GPIO register table is explicitly tracked
and initialized, reducing the risk of errors if new register tables are
introduced in the future.

Reproducer:
cat << EOF | qemu-system-aarch64 -display none \
-machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
readq 0x7e780272
EOF

ASAN log indicating the issue:
==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 
0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
READ of size 2 at 0x55a5da29e128 thread T0
#0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
#1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
#2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
#3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
#4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
#5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
#6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
#7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12

Signed-off-by: Zheyu Ma 
---
Changes in v4:
- Change the variable name to 'reg_table_count'
- Change the 'reg_table_count' type to unsigned
Changes in v3:
- Add the reproducer
---
 hw/gpio/aspeed_gpio.c | 17 +
 include/hw/gpio/aspeed_gpio.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c1781e2ba3..6474bb8de5 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -559,6 +559,12 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr 
offset, uint32_t size)
 return debounce_value;
 }
 
+if (idx >= agc->reg_table_count) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
+  __func__, idx);
+return 0;
+}
+
 reg = &agc->reg_table[idx];
 if (reg->set_idx >= agc->nr_gpio_sets) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
@@ -785,6 +791,12 @@ static void aspeed_gpio_write(void *opaque, hwaddr offset, 
uint64_t data,
 return;
 }
 
+if (idx >= agc->reg_table_count) {
+qemu_log_mask(LOG_GUEST_ERROR, "%s: idx 0x%" PRIx64 " out of bounds\n",
+  __func__, idx);
+return;
+}
+
 reg = &agc->reg_table[idx];
 if (reg->set_idx >= agc->nr_gpio_sets) {
 qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
@@ -1117,6 +1129,7 @@ static void aspeed_gpio_ast2400_class_init(ObjectClass 
*klass, void *data)
 agc->nr_gpio_pins = 216;
 agc->nr_gpio_sets = 7;
 agc->reg_table = aspeed_3_3v_gpios;
+agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_2500_class_init(ObjectClass *klass, void *data)
@@ -1127,6 +1140,7 @@ static void aspeed_gpio_2500_class_init(ObjectClass 
*klass, void *data)
 agc->nr_gpio_pins = 228;
 agc->nr_gpio_sets = 8;
 agc->reg_table = aspeed_3_3v_gpios;
+agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
@@ -1137,6 +1151,7 @@ static void 
aspeed_gpio_ast2600_3_3v_class_init(ObjectClass *klass, void *data)
 agc->nr_gpio_pins = 208;
 agc->nr_gpio_sets = 7;
 agc->reg_table = aspeed_3_3v_gpios;
+agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
@@ -1147,6 +1162,7 @@ static void 
aspeed_gpio_ast2600_1_8v_class_init(ObjectClass *klass, void *data)
 agc->nr_gpio_pins = 36;
 agc->nr_gpio_sets = 2;
 agc->reg_table = aspeed_1_8v_gpios;
+agc->reg_table_count = GPIO_1_8V_REG_ARRAY_SIZE;
 }
 
 static void aspeed_gpio_1030_class_init(ObjectClass *klass, void *data)
@@ -1157,6 +1173,7 @@ static void aspeed_gpio_1030_class_init(ObjectClass 
*klass, void *data)
 agc->nr_gpio_pins = 151;
 agc->nr_gpio_sets = 6;
 agc->reg_table = aspeed_3_3v_gpios;
+agc->reg_table_count = GPIO_3_3V_REG_ARRAY_SIZE;
 }
 
 static const TypeInfo aspeed_gpio_info = {
diff --git a/include/hw/gpio/aspeed_gpio.h b/include/hw/gpio/aspeed_gpio.h
index 904eecf62c..90a12ae318 100644
--- a/include/hw/gpio/aspeed_gpio.h
+++ b/include/hw/gpio/aspeed_gpio.h
@@ -75,6 +75,7 @@ struct AspeedGPIOClass {
 uint32_t nr_gpio_pins;
 uint32_t nr_gpio_sets;
 const AspeedGPIOReg *reg_table;
+unsigned reg_table_count;
 };
 
 struct AspeedGPIOState {
-- 
2.34.1

Re: [PATCH v3] hw/gpio/aspeed: Add reg_table_size to AspeedGPIOClass

[Zheyu Ma]

On Thu, Jun 20, 2024 at 2:35 PM Cédric Le Goater  wrote:

>
> >> @@ -75,6 +75,7 @@ struct AspeedGPIOClass {
> >>   uint32_t nr_gpio_pins;
> >>   uint32_t nr_gpio_sets;
> >>   const AspeedGPIOReg *reg_table;
> >> +uint32_t reg_table_size;
> >>   };
> >
> > - "reg_table_size" is a number of registers, using s/size/count/ might
> >be clearer.
> > - No point in specifying 32-bit, "unsigned" is sufficient.
> >
> > (Cédric, if you agree, you might update your tree).
> >
> > Unrelated to this patch but figured out while reviewing, in
> > aspeed_gpio_read/write 'idx' is
> > - pointlessly assigned to -1
> > - of type 'uint64_t', also pointless, 'unsigned' is clearer.
>
> Zheyu, could you please send a v4 ? Thanks,
>

Sure. I've sent it.

Zheyu

Re: [PATCH v4 5/5] iotests: add backup-discard-source

[Vladimir Sementsov-Ogievskiy]

On 13.06.24 11:02, Kevin Wolf wrote:

Am 12.06.2024 um 21:21 hat Vladimir Sementsov-Ogievskiy geschrieben:

On 11.06.24 20:49, Kevin Wolf wrote:

Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben:

Add test for a new backup option: discard-source.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Fiona Ebner 
Tested-by: Fiona Ebner 


This test fails for me, and it already does so after this commit that
introduced it. I haven't checked what get_actual_size(), but I'm running
on XFS, so its preallocation could be causing this. We generally avoid
checking the number of allocated blocks in image files for this reason.



Hmm right, I see that relying on allocated size is bad thing. Better
is to check block status, to see how many qcow2 clusters are
allocated.

Do we have some qmp command to get such information? The simplest way
I see is to add dirty to temp block-node, and then check its dirty
count through query-named-block-nodes


Hm, does it have to be QMP in a running QEMU process?


hmm, yes, seems in test_discard_written() we do want to examine running 
process. I'll try to go with bitmap


I'm not sure what
the best way would be there.

Otherwise, my approach would be 'qemu-img check' or even 'qemu-img map',
depending on what you want to verify with it.

Kevin



--
Best regards,
Vladimir

Re: [PATCH v4] hw/gpio/aspeed: Add reg_table_count to AspeedGPIOClass

[Philippe Mathieu-Daudé]

On 20/6/24 16:02, Zheyu Ma wrote:

ASan detected a global-buffer-overflow error in the aspeed_gpio_read()
function. This issue occurred when reading beyond the bounds of the
reg_table.

To enhance the safety and maintainability of the Aspeed GPIO code, this commit
introduces a reg_table_count member to the AspeedGPIOClass structure. This
change ensures that the size of the GPIO register table is explicitly tracked
and initialized, reducing the risk of errors if new register tables are
introduced in the future.

Reproducer:
cat << EOF | qemu-system-aarch64 -display none \
-machine accel=qtest, -m 512M -machine ast1030-evb -qtest stdio
readq 0x7e780272
EOF

ASAN log indicating the issue:
==2602930==ERROR: AddressSanitizer: global-buffer-overflow on address 
0x55a5da29e128 at pc 0x55a5d700dc62 bp 0x7fff096c4e90 sp 0x7fff096c4e88
READ of size 2 at 0x55a5da29e128 thread T0
 #0 0x55a5d700dc61 in aspeed_gpio_read hw/gpio/aspeed_gpio.c:564:14
 #1 0x55a5d933f3ab in memory_region_read_accessor system/memory.c:445:11
 #2 0x55a5d92fba40 in access_with_adjusted_size system/memory.c:573:18
 #3 0x55a5d92f842c in memory_region_dispatch_read1 system/memory.c:1426:16
 #4 0x55a5d92f7b68 in memory_region_dispatch_read system/memory.c:1459:9
 #5 0x55a5d9376ad1 in flatview_read_continue_step system/physmem.c:2836:18
 #6 0x55a5d9376399 in flatview_read_continue system/physmem.c:2877:19
 #7 0x55a5d93775b8 in flatview_read system/physmem.c:2907:12

Signed-off-by: Zheyu Ma 
---
Changes in v4:
- Change the variable name to 'reg_table_count'
- Change the 'reg_table_count' type to unsigned


Thanks,

Reviewed-by: Philippe Mathieu-Daudé 


Changes in v3:
- Add the reproducer
---
  hw/gpio/aspeed_gpio.c | 17 +
  include/hw/gpio/aspeed_gpio.h |  1 +
  2 files changed, 18 insertions(+)

Re: [RFC PATCH v4 1/5] accel/tcg: Avoid unnecessary call overhead from qemu_plugin_vcpu_mem_cb

[Alex Bennée]

Max Chou  writes:

> If there are not any QEMU plugin memory callback functions, checking
> before calling the qemu_plugin_vcpu_mem_cb function can reduce the
> function call overhead.
>
> Signed-off-by: Max Chou 

Queued this patch to maintainer/june-2024-omnibus, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] linux-user: open_self_stat: Implement num_threads

[Alex Bennée]

"Fabio D'Urso"  writes:

> The num_threads field reports the total number of threads in the
> process. In QEMU, this is equal to the number of CPU instances.
>
> Signed-off-by: Fabio D'Urso 
> ---
>  linux-user/syscall.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b9b5a387b3..a47b2eeb65 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8171,6 +8171,16 @@ static int open_self_stat(CPUArchState *cpu_env, int 
> fd)
>  } else if (i == 3) {
>  /* ppid */
>  g_string_printf(buf, FMT_pid " ", getppid());
> +} else if (i == 19) {
> +/* num_threads */
> +int cpus = 0;
> +WITH_RCU_READ_LOCK_GUARD() {
> +CPUState *cpu_iter;
> +CPU_FOREACH(cpu_iter) {
> +cpus++;
> +}
> +}
> +g_string_printf(buf, "%d ", cpus);

Looks ok to me.

Reviewed-by: Alex Bennée 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/2] iotests/backup-discard-source: don't use actual-size

[Vladimir Sementsov-Ogievskiy]

Relying on disk usage is bad thing, and test just doesn't work on XFS.

Let's instead add a dirty bitmap to track writes to test image.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 .../qemu-iotests/tests/backup-discard-source  | 29 +--
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/tests/backup-discard-source 
b/tests/qemu-iotests/tests/backup-discard-source
index 05fbe5d26b..17fef9c6d3 100755
--- a/tests/qemu-iotests/tests/backup-discard-source
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -31,12 +31,6 @@ target_img = os.path.join(iotests.test_dir, 'target')
 size = 1024 * 1024
 
 
-def get_actual_size(vm, node_name):
-nodes = vm.cmd('query-named-block-nodes', flat=True)
-node = next(n for n in nodes if n['node-name'] == node_name)
-return node['image']['actual-size']
-
-
 class TestBackup(iotests.QMPTestCase):
 def setUp(self):
 qemu_img_create('-f', iotests.imgfmt, source_img, str(size))
@@ -84,7 +78,12 @@ class TestBackup(iotests.QMPTestCase):
 }
 })
 
-self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+self.bitmap = {
+'node': 'temp',
+'name': 'bitmap0'
+}
+
+self.vm.cmd('block-dirty-bitmap-add', self.bitmap)
 
 def tearDown(self):
 # That should fail, because region is discarded
@@ -113,6 +112,13 @@ class TestBackup(iotests.QMPTestCase):
 
 self.vm.event_wait(name='BLOCK_JOB_COMPLETED')
 
+def get_bitmap_count(self):
+nodes = self.vm.cmd('query-named-block-nodes', flat=True)
+temp = next(n for n in nodes if n['node-name'] == 'temp')
+bitmap = temp['dirty-bitmaps'][0]
+assert bitmap['name'] == self.bitmap['name']
+return bitmap['count']
+
 def test_discard_written(self):
 """
 1. Guest writes
@@ -125,7 +131,7 @@ class TestBackup(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', '')
 
 # Check that data is written to temporary image
-self.assertGreater(get_actual_size(self.vm, 'temp'), size)
+self.assertEqual(self.get_bitmap_count(), size)
 
 self.do_backup()
 
@@ -138,13 +144,18 @@ class TestBackup(iotests.QMPTestCase):
 """
 self.do_backup()
 
+# backup job did discard operation and pollute the bitmap,
+# we have to clean the bitmap, to check next write
+self.assertEqual(self.get_bitmap_count(), size)
+self.vm.cmd('block-dirty-bitmap-clear', self.bitmap)
+
 # Try trigger copy-before-write operation
 result = self.vm.hmp_qemu_io('cbw', 'write 0 1M')
 self.assert_qmp(result, 'return', '')
 
 # Check that data is not written to temporary image, as region
 # is discarded from copy-before-write process
-self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024)
+self.assertEqual(self.get_bitmap_count(), 0)
 
 
 if __name__ == '__main__':
-- 
2.34.1

[PATCH 1/2] iotests/backup-discard-source: convert size variable to be int

[Vladimir Sementsov-Ogievskiy]

Make variable reusable in code for checks. Don't care to change "512 *
1024" invocations as they will be dropped in the next commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/tests/backup-discard-source | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/tests/backup-discard-source 
b/tests/qemu-iotests/tests/backup-discard-source
index 2391b12acd..05fbe5d26b 100755
--- a/tests/qemu-iotests/tests/backup-discard-source
+++ b/tests/qemu-iotests/tests/backup-discard-source
@@ -28,7 +28,7 @@ from iotests import qemu_img_create, qemu_img_map, qemu_io
 temp_img = os.path.join(iotests.test_dir, 'temp')
 source_img = os.path.join(iotests.test_dir, 'source')
 target_img = os.path.join(iotests.test_dir, 'target')
-size = '1M'
+size = 1024 * 1024
 
 
 def get_actual_size(vm, node_name):
@@ -39,9 +39,9 @@ def get_actual_size(vm, node_name):
 
 class TestBackup(iotests.QMPTestCase):
 def setUp(self):
-qemu_img_create('-f', iotests.imgfmt, source_img, size)
-qemu_img_create('-f', iotests.imgfmt, temp_img, size)
-qemu_img_create('-f', iotests.imgfmt, target_img, size)
+qemu_img_create('-f', iotests.imgfmt, source_img, str(size))
+qemu_img_create('-f', iotests.imgfmt, temp_img, str(size))
+qemu_img_create('-f', iotests.imgfmt, target_img, str(size))
 qemu_io('-c', 'write 0 1M', source_img)
 
 self.vm = iotests.VM()
@@ -98,7 +98,7 @@ class TestBackup(iotests.QMPTestCase):
 mapping = qemu_img_map(temp_img)
 self.assertEqual(len(mapping), 1)
 self.assertEqual(mapping[0]['start'], 0)
-self.assertEqual(mapping[0]['length'], 1024 * 1024)
+self.assertEqual(mapping[0]['length'], size)
 self.assertEqual(mapping[0]['data'], False)
 
 os.remove(temp_img)
@@ -125,7 +125,7 @@ class TestBackup(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', '')
 
 # Check that data is written to temporary image
-self.assertGreater(get_actual_size(self.vm, 'temp'), 1024 * 1024)
+self.assertGreater(get_actual_size(self.vm, 'temp'), size)
 
 self.do_backup()
 
-- 
2.34.1

[PATCH 0/2] fix backup-discard-source test for XFS

[Vladimir Sementsov-Ogievskiy]

Hi all!

As Kevin reported, the test doesn't work on XFS, as it rely on disk
usage.

Fix it, switching to dirty bitmap for guest write tracking.

Vladimir Sementsov-Ogievskiy (2):
  iotests/backup-discard-source: convert size variable to be int
  iotests/backup-discard-source: don't use actual-size

 .../qemu-iotests/tests/backup-discard-source  | 39 ---
 1 file changed, 25 insertions(+), 14 deletions(-)

-- 
2.34.1

Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections

[John Snow]

On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > Change get_doc_indented() to preserve indentation on all subsequent text
> > lines, and create a compatibility dedent() function for qapidoc.py to
> > remove that indentation. This is being done for the benefit of a new
>
> Suggest "remove indentation the same way get_doc_indented() did."
>

Aight.


> > qapidoc generator which requires that indentation in argument and
> > features sections are preserved.
> >
> > Prior to this patch, a section like this:
> >
> > ```
> > @name: lorem ipsum
> >dolor sit amet
> >  consectetur adipiscing elit
> > ```
> >
> > would have its body text be parsed as:
>
> Suggest "parsed into".
>

Why? (I mean, I'll do it, but I don't see the semantic difference
personally)


> > (first and final newline only for presentation)
> >
> > ```
> > lorem ipsum
> > dolor sit amet
> >   consectetur adipiscing elit
> > ```
> >
> > We want to preserve the indentation for even the first body line so that
> > the entire block can be parsed directly as rST. This patch would now
> > parse that segment as:
>
> If you change "parsed as" to "parsed into" above, then do it here, too.
>
> >
> > ```
> > lorem ipsum
> >dolor sit amet
> >  consectetur adipiscing elit
> > ```
> >
> > This is helpful for formatting arguments and features as field lists in
> > rST, where the new generator will format this information as:
> >
> > ```
> > :arg type name: lorem ipsum
> >dolor sit amet
> >  consectetur apidiscing elit
> > ```
> >
> > ...and can be formed by the simple concatenation of the field list
> > construct and the body text. The indents help preserve the continuation
> > of a block-level element, and further allow the use of additional rST
> > block-level constructs such as code blocks, lists, and other such
> > markup. Avoiding reflowing the text conditionally also helps preserve
> > source line context for better rST error reporting from sphinx through
> > generated source, too.
>
> What do you mean by "reflowing"?
>

Poorly phrased, was thinking about emacs too much. I mean munging the text
post-hoc for the doc generator such that newlines are added or removed in
the process of re-formatting text to get the proper indentation for the new
rST form.

In prototyping, this got messy very quickly and was difficult to correlate
source line numbers across the transformation.

It was easier to just not munge the text at all instead of munging it and
then un-munging it.

(semantic satiation: munge munge munge munge.)


> > This understandably breaks the existing qapidoc.py; so a new function is
> > added there to dedent the text for compatibility. Once the new generator
> > is merged, this function will not be needed any longer and can be
> > dropped.
> >
> > I verified this patch changes absolutely nothing by comparing the
> > md5sums of the QMP ref html pages both before and after the change, so
> > it's certified inert. QAPI test output has been updated to reflect the
> > new strategy of preserving indents for rST.
>
> I think the remainder is unnecessary detail.  Drop?
>

As long as you're convinced it's safe, it's done its job and we thank it
for its service

🫡


> > before:
> >
> > 69cde3d6f18b0f324badbb447d4381ce  manual_before/interop/qemu-ga-ref.html
> > 446e9381833def2adc779f1b90f2215f  manual_before/interop/qemu-qmp-ref.html
> > df0ad6c26cb4c28b85d663fe44609c12
> manual_before/interop/qemu-storage-daemon-qmp-ref.html
> >
> > after:
> >
> > 69cde3d6f18b0f324badbb447d4381ce  manual/interop/qemu-ga-ref.html
> > 446e9381833def2adc779f1b90f2215f  manual/interop/qemu-qmp-ref.html
> > df0ad6c26cb4c28b85d663fe44609c12
> manual/interop/qemu-storage-daemon-qmp-ref.html
> >
> > Signed-off-by: John Snow 
> > ---
> >  docs/sphinx/qapidoc.py | 29 -
> >  scripts/qapi/parser.py |  5 +++--
> >  tests/qapi-schema/doc-good.out | 32 
> >  3 files changed, 43 insertions(+), 23 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index e675966defa..f2f2005dd5f 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -26,6 +26,7 @@
> >
> >  import os
> >  import re
> > +import textwrap
> >
> >  from docutils import nodes
> >  from docutils.parsers.rst import Directive, directives
> > @@ -53,6 +54,21 @@
> >  __version__ = "1.0"
> >
> >
> > +def dedent(text: str) -> str:
> > +# Temporary: In service of the new QAPI Sphinx domain, the QAPI doc
> > +# parser now preserves indents in args/members/features text.
> > +# QAPIDoc does not handle this well, so undo that change here.
>
> A comment should explain how things are.  This one explains how things
> have changed.  Suggest:
>
># Adjust indentation to make description text parse as paragraph.
>
> If we planned to keep this, we might want to explain in more detail, as
> I did in review of v1.  But we don't.
>
> > +
> > 

Re: [PATCH 4/6] meson: allow configuring the x86-64 baseline

[Daniel P . Berrangé]

On Thu, Jun 20, 2024 at 03:02:52PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  meson.build   | 41 ---
>  meson_options.txt |  3 +++
>  scripts/meson-buildoptions.sh |  3 +++
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 97e00d6f59b..6e694ecd9fe 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -336,15 +336,40 @@ if host_arch == 'i386' and not cc.links('''
>qemu_common_flags = ['-march=i486'] + qemu_common_flags
>  endif
>  
> -# Assume x86-64-v2 (minus CMPXCHG16B for 32-bit code)
> -if host_arch == 'i386'
> -  qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
> -endif
> +# Pick x86-64 baseline version
>  if host_arch in ['i386', 'x86_64']
> -  qemu_common_flags = ['-mpopcnt', '-msse4.2'] + qemu_common_flags
> -endif
> -if host_arch == 'x86_64'
> -  qemu_common_flags = ['-mcx16'] + qemu_common_flags
> +  if get_option('x86_version') == '0' and host_arch == 'x86_64'
> +error('x86_64-v1 required for x86-64 hosts')
> +  endif
> +
> +  # add flags for individual instruction set extensions
> +  if get_option('x86_version') >= '1'
> +if host_arch == 'i386'
> +  qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
> +else
> +  # present on basically all processors but technically not part of
> +  # x86-64-v1, so only include -mneeded for x86-64 version 2 and above
> +  qemu_common_flags = ['-mcx16'] + qemu_common_flags
> +endif
> +  endif
> +  if get_option('x86_version') >= '2'
> +qemu_common_flags = ['-mpopcnt'] + qemu_common_flags
> +qemu_common_flags = cc.get_supported_arguments('-mneeded') + 
> qemu_common_flags
> +  endif
> +  if get_option('x86_version') >= '3'
> +qemu_common_flags = ['-mmovbe', '-mabm', '-mbmi1', '-mbmi2', '-mfma', 
> '-mf16c'] + qemu_common_flags
> +  endif
> +
> +  # add required vector instruction set (each level implies those below)
> +  if get_option('x86_version') == '1'
> +qemu_common_flags = ['-msse2'] + qemu_common_flags
> +  elif get_option('x86_version') == '2'
> +qemu_common_flags = ['-msse4.2'] + qemu_common_flags
> +  elif get_option('x86_version') == '3'
> +qemu_common_flags = ['-mavx2'] + qemu_common_flags
> +  elif get_option('x86_version') == '4'
> +qemu_common_flags = ['-mavx512f', '-mavx512bw', '-mavx512cd', 
> '-mavx512dq', '-mavx512vl'] + qemu_common_flags
> +  endif
>  endif


Any particular reason you chose to list various instructions individually
rather than just ask GCC for the full ABI ? I'd think all of the above
condences down to just

  # add flags for individual instruction set extensions
  if get_option('x86_version') >= '1'
if host_arch == 'i386'
  qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
else
  # present on basically all processors but technically not part of
  # x86-64-v1, so only include -mneeded for x86-64 version 2 and above
  qemu_common_flags = ['-mcx16'] + qemu_common_flags
endif
  endif
  if get_option('x86_version') >= '2'
qemu_common_flags = ['-march=x86-64-v' + get_option('x86_version'), 
'-mneeded'] + qemu_common_flags
  endif


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 6/6] meson: require compiler support for chosen x86-64 instructions

[Daniel P . Berrangé]

On Thu, Jun 20, 2024 at 03:02:54PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini 
> ---
>  meson.build | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index 54e6b09f4fb..c5360fbd299 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2863,6 +2863,7 @@ have_cpuid_h = cc.links('''
>  config_host_data.set('CONFIG_CPUID_H', have_cpuid_h)
>  
>  config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
> +  .enable_auto_if(get_option('x86_version') >= '3') \
>.require(have_cpuid_h, error_message: 'cpuid.h not available, cannot 
> enable AVX2') \
>.require(cc.links('''
>  #include 
> @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', 
> get_option('avx2') \
>'''), error_message: 'AVX2 not available').allowed())
>  
>  config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \
> +  .enable_auto_if(get_option('x86_version') >= '4') \
>.require(have_cpuid_h, error_message: 'cpuid.h not available, cannot 
> enable AVX512BW') \
>.require(cc.links('''
>  #include 

I'm not sure this makes sense. The CONFIG_AVX* options are used only
to validate whether the toolchain has support for this. The QEMU
code then has a runtime, so it automagically uses AVX2/AVX512
if-and-only-if running on a suitably new CPU.  IOW, we want this
enabled always when the toolchain supports it, regardless of what
x86_version is set.

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 01/10] target/i386: use cpu_cc_dst for CC_OP_POPCNT

[Richard Henderson]

On 6/20/24 02:54, Paolo Bonzini wrote:

It is the only POPCNT that computes ZF from one of the cc_op_* registers,
but it uses cpu_cc_src instead of cpu_cc_dst like the others.  Do not
make it the odd one off.

Signed-off-by: Paolo Bonzini
---
  target/i386/cpu.h   | 2 +-
  target/i386/tcg/cc_helper.c | 2 +-
  target/i386/tcg/translate.c | 2 +-
  target/i386/tcg/emit.c.inc  | 4 ++--
  4 files changed, 5 insertions(+), 5 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 4/6] meson: allow configuring the x86-64 baseline

[Paolo Bonzini]

On Thu, Jun 20, 2024 at 4:55 PM Daniel P. Berrangé  wrote:
> Any particular reason you chose to list various instructions individually
> rather than just ask GCC for the full ABI ? I'd think all of the above
> condences down to just

To avoid that the default ('1') forces a lower level than the compiler default.

Something like what you propose below could work by adding a 'default'
value to the x86_version option, that leaves the flags entirely alone
apart from -mcx16.

However, doing so would prevent QEMU from changing the default to
x86-64-v2 in meson_options.txt, because then even a compiler that
defaults to x86-64-v3 would build a QEMU with AVX2 disabled.

For AVX2 specifically this is not a huge deal because the decision to
use AVX2 code is mostly done at runtime; but it would be a problem for
future integer instruction set extensions---for example if the distro
compiler uses APX you don't want to disable it.

>   # add flags for individual instruction set extensions
>   if get_option('x86_version') >= '1'
> if host_arch == 'i386'
>   qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags

Also -msse2 here, but yes.

Paolo

Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections

[Markus Armbruster]

John Snow  writes:

> On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster  wrote:
>
>> John Snow  writes:
>>
>> > Change get_doc_indented() to preserve indentation on all subsequent text
>> > lines, and create a compatibility dedent() function for qapidoc.py to
>> > remove that indentation. This is being done for the benefit of a new
>>
>> Suggest "remove indentation the same way get_doc_indented() did."
>>
>
> Aight.
>
>
>> > qapidoc generator which requires that indentation in argument and
>> > features sections are preserved.
>> >
>> > Prior to this patch, a section like this:
>> >
>> > ```
>> > @name: lorem ipsum
>> >dolor sit amet
>> >  consectetur adipiscing elit
>> > ```
>> >
>> > would have its body text be parsed as:
>>
>> Suggest "parsed into".
>>
>
> Why? (I mean, I'll do it, but I don't see the semantic difference
> personally)
>

"Parse as " vs. "Parse into ".

>> > (first and final newline only for presentation)
>> >
>> > ```
>> > lorem ipsum
>> > dolor sit amet
>> >   consectetur adipiscing elit
>> > ```
>> >
>> > We want to preserve the indentation for even the first body line so that
>> > the entire block can be parsed directly as rST. This patch would now
>> > parse that segment as:
>>
>> If you change "parsed as" to "parsed into" above, then do it here, too.
>>
>> >
>> > ```
>> > lorem ipsum
>> >dolor sit amet
>> >  consectetur adipiscing elit
>> > ```
>> >
>> > This is helpful for formatting arguments and features as field lists in
>> > rST, where the new generator will format this information as:
>> >
>> > ```
>> > :arg type name: lorem ipsum
>> >dolor sit amet
>> >  consectetur apidiscing elit
>> > ```
>> >
>> > ...and can be formed by the simple concatenation of the field list
>> > construct and the body text. The indents help preserve the continuation
>> > of a block-level element, and further allow the use of additional rST
>> > block-level constructs such as code blocks, lists, and other such
>> > markup. Avoiding reflowing the text conditionally also helps preserve
>> > source line context for better rST error reporting from sphinx through
>> > generated source, too.
>>
>> What do you mean by "reflowing"?
>>
>
> Poorly phrased, was thinking about emacs too much. I mean munging the text
> post-hoc for the doc generator such that newlines are added or removed in
> the process of re-formatting text to get the proper indentation for the new
> rST form.
>
> In prototyping, this got messy very quickly and was difficult to correlate
> source line numbers across the transformation.
>
> It was easier to just not munge the text at all instead of munging it and
> then un-munging it.
>
> (semantic satiation: munge munge munge munge.)

Is this about a possible alternative solution you explored?  Keeping
.get_doc_indented() as is, and then try to undo its damage?

[...]

Re: [PATCH 6/6] meson: require compiler support for chosen x86-64 instructions

[Paolo Bonzini]

On Thu, Jun 20, 2024 at 5:01 PM Daniel P. Berrangé  wrote:
> >  config_host_data.set('CONFIG_AVX2_OPT', get_option('avx2') \
> > +  .enable_auto_if(get_option('x86_version') >= '3') \
> >.require(have_cpuid_h, error_message: 'cpuid.h not available, cannot 
> > enable AVX2') \
> >.require(cc.links('''
> >  #include 
> > @@ -2875,6 +2876,7 @@ config_host_data.set('CONFIG_AVX2_OPT', 
> > get_option('avx2') \
> >'''), error_message: 'AVX2 not available').allowed())
> >
> >  config_host_data.set('CONFIG_AVX512BW_OPT', get_option('avx512bw') \
> > +  .enable_auto_if(get_option('x86_version') >= '4') \
> >.require(have_cpuid_h, error_message: 'cpuid.h not available, cannot 
> > enable AVX512BW') \
> >.require(cc.links('''
> >  #include 
>
> I'm not sure this makes sense. The CONFIG_AVX* options are used only
> to validate whether the toolchain has support for this. The QEMU
> code then has a runtime, so it automagically uses AVX2/AVX512
> if-and-only-if running on a suitably new CPU.  IOW, we want this
> enabled always when the toolchain supports it, regardless of what
> x86_version is set.

The difference is that if the toolchain does not support AVX2/AVX512
intrinsics for some reason, and you require -Dx86_version={3,4}, meson
would report an error with this patch.

Paolo

Re: [PATCH 3/3] exec: use char* for pointer arithmetic

[Alex Bennée]

Daniel P. Berrangé  writes:

> On Tue, Jun 18, 2024 at 04:05:36PM -0700, Richard Henderson wrote:
>> On 6/18/24 15:46, Roman Kiryanov wrote:
>> > @@ -2839,7 +2839,7 @@ static inline uint8_t 
>> > address_space_ldub_cached(MemoryRegionCache *cache,
>> >   {
>> >   assert(addr < cache->len);
>> >   if (likely(cache->ptr)) {
>> > -return ldub_p(cache->ptr + addr);
>> > +return ldub_p((char*)cache->ptr + addr);
>> 
>> We require "char *" with a space.
>> 
>> With all of those fixed,
>> Reviewed-by: Richard Henderson 
>> 
>> PS: I'm annoyed that standards never adopted arithmetic on void *.
>
> NB, QEMU is explicitly *NOT* targetting the C standard, we are
> targetting the C dialect supported by GCC and CLang only. IOW,
> if they have well defined behaviour for arithmetic on void *,
> then we are free to use it.

It looks like GNU C does support it:

  https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

>
> With regards,
> Daniel

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 02/10] target/i386: give CC_OP_POPCNT low bits corresponding to MO_TL

[Richard Henderson]

On 6/20/24 02:54, Paolo Bonzini wrote:

Handle it like the other arithmetic cc_ops.  This simplifies a
bit the implementation of bit test instructions.

Signed-off-by: Paolo Bonzini 
---
  target/i386/cpu.h   | 13 +++--
  target/i386/tcg/translate.c |  3 +--
  2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f54cd93b3f9..8504a7998fd 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1275,6 +1275,7 @@ typedef enum {
  CC_OP_ADCX, /* CC_DST = C, CC_SRC = rest.  */
  CC_OP_ADOX, /* CC_SRC2 = O, CC_SRC = rest.  */
  CC_OP_ADCOX, /* CC_DST = C, CC_SRC2 = O, CC_SRC = rest.  */
+CC_OP_CLR, /* Z and P set, all other flags clear.  */
  
  CC_OP_MULB, /* modify all flags, C, O = (CC_SRC != 0) */

  CC_OP_MULW,
@@ -1331,8 +1332,16 @@ typedef enum {
  CC_OP_BMILGL,
  CC_OP_BMILGQ,
  
-CC_OP_CLR, /* Z set, all other flags clear.  */

-CC_OP_POPCNT, /* Z via CC_DST, all other flags clear.  */
+/*
+ * Note that only CC_OP_POPCNT (i.e. the one with MO_TL size)
+ * is used or implemented, because the translation needs
+ * to zero-extend CC_DST anyway.
+ */
+CC_OP_POPCNTB__, /* Z via CC_DST, all other flags clear.  */
+CC_OP_POPCNTW__,
+CC_OP_POPCNTL__,
+CC_OP_POPCNTQ__,
+CC_OP_POPCNT = sizeof(target_ulong) == 8 ? CC_OP_POPCNTQ__ : 
CC_OP_POPCNTL__,
  
  CC_OP_NB,

  } CCOp;
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index f32cda4e169..934c514e64f 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1019,8 +1019,6 @@ static CCPrepare gen_prepare_eflags_z(DisasContext *s, 
TCGv reg)
   .imm = CC_Z };
  case CC_OP_CLR:
  return (CCPrepare) { .cond = TCG_COND_ALWAYS };
-case CC_OP_POPCNT:
-return (CCPrepare) { .cond = TCG_COND_EQ, .reg = cpu_cc_src };


The previous patch needs to have changed this to dst.

Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH 04/13] qapi/parser: preserve indentation in QAPIDoc sections

[John Snow]

On Thu, Jun 20, 2024, 11:07 AM Markus Armbruster  wrote:

> John Snow  writes:
>
> > On Wed, Jun 19, 2024, 8:03 AM Markus Armbruster 
> wrote:
> >
> >> John Snow  writes:
> >>
> >> > Change get_doc_indented() to preserve indentation on all subsequent
> text
> >> > lines, and create a compatibility dedent() function for qapidoc.py to
> >> > remove that indentation. This is being done for the benefit of a new
> >>
> >> Suggest "remove indentation the same way get_doc_indented() did."
> >>
> >
> > Aight.
> >
> >
> >> > qapidoc generator which requires that indentation in argument and
> >> > features sections are preserved.
> >> >
> >> > Prior to this patch, a section like this:
> >> >
> >> > ```
> >> > @name: lorem ipsum
> >> >dolor sit amet
> >> >  consectetur adipiscing elit
> >> > ```
> >> >
> >> > would have its body text be parsed as:
> >>
> >> Suggest "parsed into".
> >>
> >
> > Why? (I mean, I'll do it, but I don't see the semantic difference
> > personally)
> >
>
> "Parse as " vs. "Parse into ".
>
> >> > (first and final newline only for presentation)
> >> >
> >> > ```
> >> > lorem ipsum
> >> > dolor sit amet
> >> >   consectetur adipiscing elit
> >> > ```
> >> >
> >> > We want to preserve the indentation for even the first body line so
> that
> >> > the entire block can be parsed directly as rST. This patch would now
> >> > parse that segment as:
> >>
> >> If you change "parsed as" to "parsed into" above, then do it here, too.
> >>
> >> >
> >> > ```
> >> > lorem ipsum
> >> >dolor sit amet
> >> >  consectetur adipiscing elit
> >> > ```
> >> >
> >> > This is helpful for formatting arguments and features as field lists
> in
> >> > rST, where the new generator will format this information as:
> >> >
> >> > ```
> >> > :arg type name: lorem ipsum
> >> >dolor sit amet
> >> >  consectetur apidiscing elit
> >> > ```
> >> >
> >> > ...and can be formed by the simple concatenation of the field list
> >> > construct and the body text. The indents help preserve the
> continuation
> >> > of a block-level element, and further allow the use of additional rST
> >> > block-level constructs such as code blocks, lists, and other such
> >> > markup. Avoiding reflowing the text conditionally also helps preserve
> >> > source line context for better rST error reporting from sphinx through
> >> > generated source, too.
> >>
> >> What do you mean by "reflowing"?
> >>
> >
> > Poorly phrased, was thinking about emacs too much. I mean munging the
> text
> > post-hoc for the doc generator such that newlines are added or removed in
> > the process of re-formatting text to get the proper indentation for the
> new
> > rST form.
> >
> > In prototyping, this got messy very quickly and was difficult to
> correlate
> > source line numbers across the transformation.
> >
> > It was easier to just not munge the text at all instead of munging it and
> > then un-munging it.
> >
> > (semantic satiation: munge munge munge munge.)
>
> Is this about a possible alternative solution you explored?  Keeping
> .get_doc_indented() as is, and then try to undo its damage?
>

precisamente. That solution was categorically worse.


> [...]
>
>

Re: [PATCH 03/10] target/i386: convert bit test instructions to new decoder

[Richard Henderson]

On 6/20/24 02:54, Paolo Bonzini wrote:

Signed-off-by: Paolo Bonzini
---
  target/i386/tcg/decode-new.h |   3 +
  target/i386/tcg/translate.c  | 147 +-
  target/i386/tcg/decode-new.c.inc |  40 ++---
  target/i386/tcg/emit.c.inc   | 149 ++-
  4 files changed, 181 insertions(+), 158 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PATCH v2 06/12] sysemu: generalise qtest_warp_clock as qemu_clock_advance_virtual_time

[Alex Bennée]

Move the key functionality of moving time forward into the clock
sub-system itself. This will allow us to plumb in time control into
plugins.

Signed-off-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
Message-Id: <20240530220610.1245424-4-pierrick.bouv...@linaro.org>

--
v2
  - use target_ns in docs and signature
---
 include/qemu/timer.h | 15 +++
 system/qtest.c   | 25 +++--
 util/qemu-timer.c| 26 ++
 3 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 9a366e551f..5ce83c7911 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -245,6 +245,21 @@ bool qemu_clock_run_timers(QEMUClockType type);
  */
 bool qemu_clock_run_all_timers(void);
 
+/**
+ * qemu_clock_advance_virtual_time(): advance the virtual time tick
+ * @target_ns: target time in nanoseconds
+ *
+ * This function is used where the control of the flow of time has
+ * been delegated to outside the clock subsystem (be it qtest, icount
+ * or some other external source). You can ask the clock system to
+ * return @early at the first expired timer.
+ *
+ * Time can only move forward, attempts to reverse time would lead to
+ * an error.
+ *
+ * Returns: new virtual time.
+ */
+int64_t qemu_clock_advance_virtual_time(int64_t target_ns);
 
 /*
  * QEMUTimerList
diff --git a/system/qtest.c b/system/qtest.c
index 5be66b0140..8cb98966b4 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -337,26 +337,6 @@ void qtest_set_virtual_clock(int64_t count)
 qatomic_set_i64(&qtest_clock_counter, count);
 }
 
-static void qtest_clock_warp(int64_t dest)
-{
-int64_t clock = cpus_get_virtual_clock();
-AioContext *aio_context;
-assert(qtest_enabled());
-aio_context = qemu_get_aio_context();
-while (clock < dest) {
-int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
-  QEMU_TIMER_ATTR_ALL);
-int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
-
-cpus_set_virtual_clock(cpus_get_virtual_clock() + warp);
-
-qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
-timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
-clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-}
-qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
-}
-
 static bool (*process_command_cb)(CharBackend *chr, gchar **words);
 
 void qtest_set_command_cb(bool (*pc_cb)(CharBackend *chr, gchar **words))
@@ -751,7 +731,8 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 ns = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
 QEMU_TIMER_ATTR_ALL);
 }
-qtest_clock_warp(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns);
+qemu_clock_advance_virtual_time(
+qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns);
 qtest_send_prefix(chr);
 qtest_sendf(chr, "OK %"PRIi64"\n",
 (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
@@ -777,7 +758,7 @@ static void qtest_process_command(CharBackend *chr, gchar 
**words)
 g_assert(words[1]);
 ret = qemu_strtoi64(words[1], NULL, 0, &ns);
 g_assert(ret == 0);
-qtest_clock_warp(ns);
+qemu_clock_advance_virtual_time(ns);
 qtest_send_prefix(chr);
 qtest_sendf(chr, "OK %"PRIi64"\n",
 (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index 6a0de33dd2..213114be68 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -645,6 +645,11 @@ int64_t qemu_clock_get_ns(QEMUClockType type)
 }
 }
 
+static void qemu_virtual_clock_set_ns(int64_t time)
+{
+return cpus_set_virtual_clock(time);
+}
+
 void init_clocks(QEMUTimerListNotifyCB *notify_cb)
 {
 QEMUClockType type;
@@ -675,3 +680,24 @@ bool qemu_clock_run_all_timers(void)
 
 return progress;
 }
+
+int64_t qemu_clock_advance_virtual_time(int64_t dest)
+{
+int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+AioContext *aio_context;
+aio_context = qemu_get_aio_context();
+while (clock < dest) {
+int64_t deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL,
+  QEMU_TIMER_ATTR_ALL);
+int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
+
+qemu_virtual_clock_set_ns(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 
warp);
+
+qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
+timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
+clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+}
+qemu_clock_notify(QEMU_CLOCK_VIRTUAL);
+
+return clock;
+}
-- 
2.39.2

[PATCH v2 01/12] include/exec: add missing include guard comment

[Alex Bennée]

Message-Id: <20240612153508.1532940-2-alex.ben...@linaro.org>
Reviewed-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
---
 include/exec/gdbstub.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index eb14b91139..008a92198a 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -144,4 +144,4 @@ void gdb_set_stop_cpu(CPUState *cpu);
 /* in gdbstub-xml.c, generated by scripts/feature_to_c.py */
 extern const GDBFeature gdb_static_features[];
 
-#endif
+#endif /* GDBSTUB_H */
-- 
2.39.2

[PATCH v2 00/12] maintainer updates pre-PR (gdbstub, plugins, time control)

[Alex Bennée]

Hi,

This is the current state of my maintainer trees. The gdbstub patches
are just minor clean-ups. The main feature this brings in is the
ability for plugins to control time. This has been discussed before
but represents the first time plugins can "control" the execution of
the core. The idea would be to eventually deprecate the icount auto
modes in favour of a plugin and just use icount for deterministic
execution and record/replay.

v2
  - merged in Pierrick's fixes
  - added migration blocker
  - added Max's plugin tweak

I'll send the PR on Monday if nothing comes up. The following still need review:

  plugins: add migration blocker

Alex.

Akihiko Odaki (1):
  plugins: Ensure register handles are not NULL

Alex Bennée (7):
  include/exec: add missing include guard comment
  gdbstub: move enums into separate header
  sysemu: add set_virtual_time to accel ops
  qtest: use cpu interface in qtest_clock_warp
  sysemu: generalise qtest_warp_clock as qemu_clock_advance_virtual_time
  plugins: add time control API
  plugins: add migration blocker

Max Chou (1):
  accel/tcg: Avoid unnecessary call overhead from
qemu_plugin_vcpu_mem_cb

Pierrick Bouvier (3):
  qtest: move qtest_{get, set}_virtual_clock to accel/qtest/qtest.c
  contrib/plugins: add Instructions Per Second (IPS) example for cost
modeling
  plugins: fix inject_mem_cb rw masking

 include/exec/gdbstub.h|  11 +-
 include/gdbstub/enums.h   |  21 +++
 include/qemu/qemu-plugin.h|  27 +++
 include/qemu/timer.h  |  15 ++
 include/sysemu/accel-ops.h|  18 +-
 include/sysemu/cpu-timers.h   |   3 +-
 include/sysemu/qtest.h|   2 -
 accel/hvf/hvf-accel-ops.c |   2 +-
 accel/kvm/kvm-all.c   |   2 +-
 accel/qtest/qtest.c   |  13 ++
 accel/tcg/plugin-gen.c|   4 +-
 accel/tcg/tcg-accel-ops.c |   2 +-
 contrib/plugins/ips.c | 164 ++
 gdbstub/user.c|   1 +
 monitor/hmp-cmds.c|   3 +-
 plugins/api.c |  47 -
 plugins/core.c|   4 +-
 ...t-virtual-clock.c => cpus-virtual-clock.c} |   5 +
 system/cpus.c |  11 ++
 system/qtest.c|  37 +---
 system/vl.c   |   1 +
 target/arm/hvf/hvf.c  |   2 +-
 target/arm/hyp_gdbstub.c  |   2 +-
 target/arm/kvm.c  |   2 +-
 target/i386/kvm/kvm.c |   2 +-
 target/ppc/kvm.c  |   2 +-
 target/s390x/kvm/kvm.c|   2 +-
 util/qemu-timer.c |  26 +++
 accel/tcg/ldst_common.c.inc   |   8 +-
 contrib/plugins/Makefile  |   1 +
 plugins/qemu-plugins.symbols  |   2 +
 stubs/meson.build |   2 +-
 32 files changed, 377 insertions(+), 67 deletions(-)
 create mode 100644 include/gdbstub/enums.h
 create mode 100644 contrib/plugins/ips.c
 rename stubs/{cpus-get-virtual-clock.c => cpus-virtual-clock.c} (68%)

-- 
2.39.2

[PATCH v2 09/12] plugins: add migration blocker

[Alex Bennée]

If the plugin in controlling time there is some state that might be
missing from the plugin tracking it. Migration is unlikely to work in
this case so lets put a migration blocker in to let the user know if
they try.

Signed-off-by: Alex Bennée 
Suggested-by: "Dr. David Alan Gilbert" 
---
 plugins/api.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/plugins/api.c b/plugins/api.c
index 4431a0ea7e..c4239153af 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -47,6 +47,8 @@
 #include "disas/disas.h"
 #include "plugin.h"
 #ifndef CONFIG_USER_ONLY
+#include "qapi/error.h"
+#include "migration/blocker.h"
 #include "exec/ram_addr.h"
 #include "qemu/plugin-memory.h"
 #include "hw/boards.h"
@@ -589,11 +591,17 @@ uint64_t qemu_plugin_u64_sum(qemu_plugin_u64 entry)
  * Time control
  */
 static bool has_control;
+Error *migration_blocker;
 
 const void *qemu_plugin_request_time_control(void)
 {
 if (!has_control) {
 has_control = true;
+#ifdef CONFIG_SOFTMMU
+error_setg(&migration_blocker,
+   "TCG plugin time control does not support migration");
+migrate_add_blocker(&migration_blocker, NULL);
+#endif
 return &has_control;
 }
 return NULL;
-- 
2.39.2

[PATCH v2 10/12] contrib/plugins: add Instructions Per Second (IPS) example for cost modeling

[Alex Bennée]

From: Pierrick Bouvier 

This plugin uses the new time control interface to make decisions
about the state of time during the emulation. The algorithm is
currently very simple. The user specifies an ips rate which applies
per core. If the core runs ahead of its allocated execution time the
plugin sleeps for a bit to let real time catch up. Either way time is
updated for the emulation as a function of total executed instructions
with some adjustments for cores that idle.

Examples


Slow down execution of /bin/true:
$ num_insn=$(./build/qemu-x86_64 -plugin ./build/tests/plugin/libinsn.so -d 
plugin /bin/true |& grep total | sed -e 's/.*: //')
$ time ./build/qemu-x86_64 -plugin 
./build/contrib/plugins/libips.so,ips=$(($num_insn/4)) /bin/true
real 4.000s

Boot a Linux kernel simulating a 250MHz cpu:
$ /build/qemu-system-x86_64 -kernel /boot/vmlinuz-6.1.0-21-amd64 -append 
"console=ttyS0" -plugin 
./build/contrib/plugins/libips.so,ips=$((250*1000*1000)) -smp 1 -m 512
check time until kernel panic on serial0

Tested in system mode by booting a full debian system, and using:
$ sysbench cpu run
Performance decrease linearly with the given number of ips.

Signed-off-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
Message-Id: <20240530220610.1245424-7-pierrick.bouv...@linaro.org>

---
v2
  - more explicit Instructions Per Second (IPS)!
---
 contrib/plugins/ips.c| 164 +++
 contrib/plugins/Makefile |   1 +
 2 files changed, 165 insertions(+)
 create mode 100644 contrib/plugins/ips.c

diff --git a/contrib/plugins/ips.c b/contrib/plugins/ips.c
new file mode 100644
index 00..29fa556d0f
--- /dev/null
+++ b/contrib/plugins/ips.c
@@ -0,0 +1,164 @@
+/*
+ * Instructions Per Second (IPS) rate limiting plugin.
+ *
+ * This plugin can be used to restrict the execution of a system to a
+ * particular number of Instructions Per Second (IPS). This controls
+ * time as seen by the guest so while wall-clock time may be longer
+ * from the guests point of view time will pass at the normal rate.
+ *
+ * This uses the new plugin API which allows the plugin to control
+ * system time.
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include 
+#include 
+#include 
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+/* how many times do we update time per sec */
+#define NUM_TIME_UPDATE_PER_SEC 10
+#define NSEC_IN_ONE_SEC (1000 * 1000 * 1000)
+
+static GMutex global_state_lock;
+
+static uint64_t max_insn_per_second = 1000 * 1000 * 1000; /* ips per core, per 
second */
+static uint64_t max_insn_per_quantum; /* trap every N instructions */
+static int64_t virtual_time_ns; /* last set virtual time */
+
+static const void *time_handle;
+
+typedef struct {
+uint64_t total_insn;
+uint64_t quantum_insn; /* insn in last quantum */
+int64_t last_quantum_time; /* time when last quantum started */
+} vCPUTime;
+
+struct qemu_plugin_scoreboard *vcpus;
+
+/* return epoch time in ns */
+static int64_t now_ns(void)
+{
+return g_get_real_time() * 1000;
+}
+
+static uint64_t num_insn_during(int64_t elapsed_ns)
+{
+double num_secs = elapsed_ns / (double) NSEC_IN_ONE_SEC;
+return num_secs * (double) max_insn_per_second;
+}
+
+static int64_t time_for_insn(uint64_t num_insn)
+{
+double num_secs = (double) num_insn / (double) max_insn_per_second;
+return num_secs * (double) NSEC_IN_ONE_SEC;
+}
+
+static void update_system_time(vCPUTime *vcpu)
+{
+int64_t elapsed_ns = now_ns() - vcpu->last_quantum_time;
+uint64_t max_insn = num_insn_during(elapsed_ns);
+
+if (vcpu->quantum_insn >= max_insn) {
+/* this vcpu ran faster than expected, so it has to sleep */
+uint64_t insn_advance = vcpu->quantum_insn - max_insn;
+uint64_t time_advance_ns = time_for_insn(insn_advance);
+int64_t sleep_us = time_advance_ns / 1000;
+g_usleep(sleep_us);
+}
+
+vcpu->total_insn += vcpu->quantum_insn;
+vcpu->quantum_insn = 0;
+vcpu->last_quantum_time = now_ns();
+
+/* based on total number of instructions, what should be the new time? */
+int64_t new_virtual_time = time_for_insn(vcpu->total_insn);
+
+g_mutex_lock(&global_state_lock);
+
+/* Time only moves forward. Another vcpu might have updated it already. */
+if (new_virtual_time > virtual_time_ns) {
+qemu_plugin_update_ns(time_handle, new_virtual_time);
+virtual_time_ns = new_virtual_time;
+}
+
+g_mutex_unlock(&global_state_lock);
+}
+
+static void vcpu_init(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+vCPUTime *vcpu = qemu_plugin_scoreboard_find(vcpus, cpu_index);
+vcpu->total_insn = 0;
+vcpu->quantum_insn = 0;
+vcpu->last_quantum_time = now_ns();
+}
+
+static void vcpu_exit(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+vCPUTime *vcpu = qemu_plugin_scoreboard_find(vcpus, cpu_index);
+update_system_time(vcpu);
+}
+
+static void every_quant

[PATCH v2 08/12] plugins: add time control API

[Alex Bennée]

Expose the ability to control time through the plugin API. Only one
plugin can control time so it has to request control when loaded.
There are probably more corner cases to catch here.

Signed-off-by: Pierrick Bouvier 
[AJB: tweaked user-mode handling, merged QEMU_PLUGIN_API fix]
Signed-off-by: Alex Bennée 
Message-Id: <20240530220610.1245424-6-pierrick.bouv...@linaro.org>

---
plugins/next
  - make qemu_plugin_update_ns a NOP in user-mode
v2
  - remove From: header
  - merged in plugins: missing QEMU_PLUGIN_API for time control
---
 include/qemu/qemu-plugin.h   | 27 +++
 plugins/api.c| 35 +++
 plugins/qemu-plugins.symbols |  2 ++
 3 files changed, 64 insertions(+)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 95703d8fec..c71c705b69 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -661,6 +661,33 @@ void qemu_plugin_register_vcpu_mem_inline_per_vcpu(
 qemu_plugin_u64 entry,
 uint64_t imm);
 
+/**
+ * qemu_plugin_request_time_control() - request the ability to control time
+ *
+ * This grants the plugin the ability to control system time. Only one
+ * plugin can control time so if multiple plugins request the ability
+ * all but the first will fail.
+ *
+ * Returns an opaque handle or NULL if fails
+ */
+QEMU_PLUGIN_API
+const void *qemu_plugin_request_time_control(void);
+
+/**
+ * qemu_plugin_update_ns() - update system emulation time
+ * @handle: opaque handle returned by qemu_plugin_request_time_control()
+ * @time: time in nanoseconds
+ *
+ * This allows an appropriately authorised plugin (i.e. holding the
+ * time control handle) to move system time forward to @time. For
+ * user-mode emulation the time is not changed by this as all reported
+ * time comes from the host kernel.
+ *
+ * Start time is 0.
+ */
+QEMU_PLUGIN_API
+void qemu_plugin_update_ns(const void *handle, int64_t time);
+
 typedef void
 (*qemu_plugin_vcpu_syscall_cb_t)(qemu_plugin_id_t id, unsigned int vcpu_index,
  int64_t num, uint64_t a1, uint64_t a2,
diff --git a/plugins/api.c b/plugins/api.c
index 6bdb26bbe3..4431a0ea7e 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -39,6 +39,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/plugin.h"
 #include "qemu/log.h"
+#include "qemu/timer.h"
 #include "tcg/tcg.h"
 #include "exec/exec-all.h"
 #include "exec/gdbstub.h"
@@ -583,3 +584,37 @@ uint64_t qemu_plugin_u64_sum(qemu_plugin_u64 entry)
 }
 return total;
 }
+
+/*
+ * Time control
+ */
+static bool has_control;
+
+const void *qemu_plugin_request_time_control(void)
+{
+if (!has_control) {
+has_control = true;
+return &has_control;
+}
+return NULL;
+}
+
+#ifdef CONFIG_SOFTMMU
+static void advance_virtual_time__async(CPUState *cpu, run_on_cpu_data data)
+{
+int64_t new_time = data.host_ulong;
+qemu_clock_advance_virtual_time(new_time);
+}
+#endif
+
+void qemu_plugin_update_ns(const void *handle, int64_t new_time)
+{
+#ifdef CONFIG_SOFTMMU
+if (handle == &has_control) {
+/* Need to execute out of cpu_exec, so bql can be locked. */
+async_run_on_cpu(current_cpu,
+ advance_virtual_time__async,
+ RUN_ON_CPU_HOST_ULONG(new_time));
+}
+#endif
+}
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index aa0a77a319..ca773d8d9f 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -38,6 +38,7 @@
   qemu_plugin_register_vcpu_tb_exec_cond_cb;
   qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu;
   qemu_plugin_register_vcpu_tb_trans_cb;
+  qemu_plugin_request_time_control;
   qemu_plugin_reset;
   qemu_plugin_scoreboard_free;
   qemu_plugin_scoreboard_find;
@@ -51,5 +52,6 @@
   qemu_plugin_u64_set;
   qemu_plugin_u64_sum;
   qemu_plugin_uninstall;
+  qemu_plugin_update_ns;
   qemu_plugin_vcpu_for_each;
 };
-- 
2.39.2

[PATCH v2 02/12] gdbstub: move enums into separate header

[Alex Bennée]

This is an experiment to further reduce the amount we throw into the
exec headers. It might not be as useful as I initially thought because
just under half of the users also need gdbserver_start().

Message-Id: <20240612153508.1532940-3-alex.ben...@linaro.org>
Reviewed-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
---
 include/exec/gdbstub.h|  9 -
 include/gdbstub/enums.h   | 21 +
 accel/hvf/hvf-accel-ops.c |  2 +-
 accel/kvm/kvm-all.c   |  2 +-
 accel/tcg/tcg-accel-ops.c |  2 +-
 gdbstub/user.c|  1 +
 monitor/hmp-cmds.c|  3 ++-
 system/vl.c   |  1 +
 target/arm/hvf/hvf.c  |  2 +-
 target/arm/hyp_gdbstub.c  |  2 +-
 target/arm/kvm.c  |  2 +-
 target/i386/kvm/kvm.c |  2 +-
 target/ppc/kvm.c  |  2 +-
 target/s390x/kvm/kvm.c|  2 +-
 14 files changed, 34 insertions(+), 19 deletions(-)
 create mode 100644 include/gdbstub/enums.h

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 008a92198a..1bd2c4ec2a 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -1,15 +1,6 @@
 #ifndef GDBSTUB_H
 #define GDBSTUB_H
 
-#define DEFAULT_GDBSTUB_PORT "1234"
-
-/* GDB breakpoint/watchpoint types */
-#define GDB_BREAKPOINT_SW0
-#define GDB_BREAKPOINT_HW1
-#define GDB_WATCHPOINT_WRITE 2
-#define GDB_WATCHPOINT_READ  3
-#define GDB_WATCHPOINT_ACCESS4
-
 typedef struct GDBFeature {
 const char *xmlname;
 const char *xml;
diff --git a/include/gdbstub/enums.h b/include/gdbstub/enums.h
new file mode 100644
index 00..c4d54a1d08
--- /dev/null
+++ b/include/gdbstub/enums.h
@@ -0,0 +1,21 @@
+/*
+ * gdbstub enums
+ *
+ * Copyright (c) 2024 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef GDBSTUB_ENUMS_H
+#define GDBSTUB_ENUMS_H
+
+#define DEFAULT_GDBSTUB_PORT "1234"
+
+/* GDB breakpoint/watchpoint types */
+#define GDB_BREAKPOINT_SW0
+#define GDB_BREAKPOINT_HW1
+#define GDB_WATCHPOINT_WRITE 2
+#define GDB_WATCHPOINT_READ  3
+#define GDB_WATCHPOINT_ACCESS4
+
+#endif /* GDBSTUB_ENUMS_H */
diff --git a/accel/hvf/hvf-accel-ops.c b/accel/hvf/hvf-accel-ops.c
index b2a37a2229..ac08cfb9f3 100644
--- a/accel/hvf/hvf-accel-ops.c
+++ b/accel/hvf/hvf-accel-ops.c
@@ -52,7 +52,7 @@
 #include "qemu/main-loop.h"
 #include "exec/address-spaces.h"
 #include "exec/exec-all.h"
-#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 #include "sysemu/cpus.h"
 #include "sysemu/hvf.h"
 #include "sysemu/hvf_int.h"
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 854cb86b22..2b4ab89679 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -27,7 +27,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "hw/s390x/adapter.h"
-#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 #include "sysemu/kvm_int.h"
 #include "sysemu/runstate.h"
 #include "sysemu/cpus.h"
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index 1433e38f40..3c19e68a79 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -35,7 +35,7 @@
 #include "exec/exec-all.h"
 #include "exec/hwaddr.h"
 #include "exec/tb-flush.h"
-#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 
 #include "hw/core/cpu.h"
 
diff --git a/gdbstub/user.c b/gdbstub/user.c
index edeb72efeb..e34b58b407 100644
--- a/gdbstub/user.c
+++ b/gdbstub/user.c
@@ -18,6 +18,7 @@
 #include "exec/gdbstub.h"
 #include "gdbstub/syscalls.h"
 #include "gdbstub/user.h"
+#include "gdbstub/enums.h"
 #include "hw/core/cpu.h"
 #include "trace.h"
 #include "internals.h"
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 45ee3a9e1f..f601d06ab8 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -15,8 +15,9 @@
 
 #include "qemu/osdep.h"
 #include "exec/address-spaces.h"
-#include "exec/gdbstub.h"
 #include "exec/ioport.h"
+#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 #include "monitor/hmp.h"
 #include "qemu/help_option.h"
 #include "monitor/monitor-internal.h"
diff --git a/system/vl.c b/system/vl.c
index a3eede5fa5..cfcb674425 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -68,6 +68,7 @@
 #include "sysemu/numa.h"
 #include "sysemu/hostmem.h"
 #include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 #include "qemu/timer.h"
 #include "chardev/char.h"
 #include "qemu/bitmap.h"
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 45e2218be5..ef9bc42738 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -33,7 +33,7 @@
 #include "trace/trace-target_arm_hvf.h"
 #include "migration/vmstate.h"
 
-#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 
 #define MDSCR_EL1_SS_SHIFT  0
 #define MDSCR_EL1_MDE_SHIFT 15
diff --git a/target/arm/hyp_gdbstub.c b/target/arm/hyp_gdbstub.c
index ebde2899cd..f120d55caa 100644
--- a/target/arm/hyp_gdbstub.c
+++ b/target/arm/hyp_gdbstub.c
@@ -12,7 +12,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "internals.h"
-#include "exec/gdbstub.h"
+#include "gdbstub/enums.h"
 
 /* Ma

[PATCH v2 04/12] sysemu: add set_virtual_time to accel ops

[Alex Bennée]

We are about to remove direct calls to individual accelerators for
this information and will need a central point for plugins to hook
into time changes.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
Reviewed-by: Richard Henderson 
Message-Id: <20240530220610.1245424-2-pierrick.bouv...@linaro.org>
---
 include/sysemu/accel-ops.h | 18 +-
 include/sysemu/cpu-timers.h|  3 ++-
 ...et-virtual-clock.c => cpus-virtual-clock.c} |  5 +
 system/cpus.c  | 11 +++
 stubs/meson.build  |  2 +-
 5 files changed, 36 insertions(+), 3 deletions(-)
 rename stubs/{cpus-get-virtual-clock.c => cpus-virtual-clock.c} (68%)

diff --git a/include/sysemu/accel-ops.h b/include/sysemu/accel-ops.h
index ef91fc28bb..a088672230 100644
--- a/include/sysemu/accel-ops.h
+++ b/include/sysemu/accel-ops.h
@@ -20,7 +20,12 @@
 typedef struct AccelOpsClass AccelOpsClass;
 DECLARE_CLASS_CHECKERS(AccelOpsClass, ACCEL_OPS, TYPE_ACCEL_OPS)
 
-/* cpus.c operations interface */
+/**
+ * struct AccelOpsClass - accelerator interfaces
+ *
+ * This structure is used to abstract accelerator differences from the
+ * core CPU code. Not all have to be implemented.
+ */
 struct AccelOpsClass {
 /*< private >*/
 ObjectClass parent_class;
@@ -44,7 +49,18 @@ struct AccelOpsClass {
 
 void (*handle_interrupt)(CPUState *cpu, int mask);
 
+/**
+ * @get_virtual_clock: fetch virtual clock
+ * @set_virtual_clock: set virtual clock
+ *
+ * These allow the timer subsystem to defer to the accelerator to
+ * fetch time. The set function is needed if the accelerator wants
+ * to track the changes to time as the timer is warped through
+ * various timer events.
+ */
 int64_t (*get_virtual_clock)(void);
+void (*set_virtual_clock)(int64_t time);
+
 int64_t (*get_elapsed_ticks)(void);
 
 /* gdbstub hooks */
diff --git a/include/sysemu/cpu-timers.h b/include/sysemu/cpu-timers.h
index d86738a378..7bfa960fbd 100644
--- a/include/sysemu/cpu-timers.h
+++ b/include/sysemu/cpu-timers.h
@@ -96,8 +96,9 @@ int64_t cpu_get_clock(void);
 
 void qemu_timer_notify_cb(void *opaque, QEMUClockType type);
 
-/* get the VIRTUAL clock and VM elapsed ticks via the cpus accel interface */
+/* get/set VIRTUAL clock and VM elapsed ticks via the cpus accel interface */
 int64_t cpus_get_virtual_clock(void);
+void cpus_set_virtual_clock(int64_t new_time);
 int64_t cpus_get_elapsed_ticks(void);
 
 #endif /* SYSEMU_CPU_TIMERS_H */
diff --git a/stubs/cpus-get-virtual-clock.c b/stubs/cpus-virtual-clock.c
similarity index 68%
rename from stubs/cpus-get-virtual-clock.c
rename to stubs/cpus-virtual-clock.c
index fd447d53f3..af7c1a1d40 100644
--- a/stubs/cpus-get-virtual-clock.c
+++ b/stubs/cpus-virtual-clock.c
@@ -6,3 +6,8 @@ int64_t cpus_get_virtual_clock(void)
 {
 return cpu_get_clock();
 }
+
+void cpus_set_virtual_clock(int64_t new_time)
+{
+/* do nothing */
+}
diff --git a/system/cpus.c b/system/cpus.c
index f8fa78f33d..d3640c9503 100644
--- a/system/cpus.c
+++ b/system/cpus.c
@@ -229,6 +229,17 @@ int64_t cpus_get_virtual_clock(void)
 return cpu_get_clock();
 }
 
+/*
+ * Signal the new virtual time to the accelerator. This is only needed
+ * by accelerators that need to track the changes as we warp time.
+ */
+void cpus_set_virtual_clock(int64_t new_time)
+{
+if (cpus_accel && cpus_accel->set_virtual_clock) {
+cpus_accel->set_virtual_clock(new_time);
+}
+}
+
 /*
  * return the time elapsed in VM between vm_start and vm_stop.  Unless
  * icount is active, cpus_get_elapsed_ticks() uses units of the host CPU cycle
diff --git a/stubs/meson.build b/stubs/meson.build
index f15b48d01f..772a3e817d 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -29,7 +29,7 @@ endif
 if have_block or have_ga
   stub_ss.add(files('replay-tools.c'))
   # stubs for hooks in util/main-loop.c, util/async.c etc.
-  stub_ss.add(files('cpus-get-virtual-clock.c'))
+  stub_ss.add(files('cpus-virtual-clock.c'))
   stub_ss.add(files('icount.c'))
   stub_ss.add(files('graph-lock.c'))
   if linux_io_uring.found()
-- 
2.39.2

[PATCH v2 05/12] qtest: use cpu interface in qtest_clock_warp

[Alex Bennée]

This generalises the qtest_clock_warp code to use the AccelOps
handlers for updating its own sense of time. This will make the next
patch which moves the warp code closer to pure code motion.

From: Alex Bennée 
Acked-by: Thomas Huth 
Signed-off-by: Pierrick Bouvier 
Signed-off-by: Alex Bennée 
Message-Id: <20240530220610.1245424-3-pierrick.bouv...@linaro.org>
---
 include/sysemu/qtest.h | 1 +
 accel/qtest/qtest.c| 1 +
 system/qtest.c | 6 +++---
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
index b5d5fd3463..45f3b7e1df 100644
--- a/include/sysemu/qtest.h
+++ b/include/sysemu/qtest.h
@@ -36,6 +36,7 @@ void qtest_server_set_send_handler(void (*send)(void *, const 
char *),
 void qtest_server_inproc_recv(void *opaque, const char *buf);
 
 int64_t qtest_get_virtual_clock(void);
+void qtest_set_virtual_clock(int64_t count);
 #endif
 
 #endif
diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index f6056ac836..53182e6c2a 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -52,6 +52,7 @@ static void qtest_accel_ops_class_init(ObjectClass *oc, void 
*data)
 
 ops->create_vcpu_thread = dummy_start_vcpu_thread;
 ops->get_virtual_clock = qtest_get_virtual_clock;
+ops->set_virtual_clock = qtest_set_virtual_clock;
 };
 
 static const TypeInfo qtest_accel_ops_type = {
diff --git a/system/qtest.c b/system/qtest.c
index 507a358f3b..5be66b0140 100644
--- a/system/qtest.c
+++ b/system/qtest.c
@@ -332,14 +332,14 @@ int64_t qtest_get_virtual_clock(void)
 return qatomic_read_i64(&qtest_clock_counter);
 }
 
-static void qtest_set_virtual_clock(int64_t count)
+void qtest_set_virtual_clock(int64_t count)
 {
 qatomic_set_i64(&qtest_clock_counter, count);
 }
 
 static void qtest_clock_warp(int64_t dest)
 {
-int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t clock = cpus_get_virtual_clock();
 AioContext *aio_context;
 assert(qtest_enabled());
 aio_context = qemu_get_aio_context();
@@ -348,7 +348,7 @@ static void qtest_clock_warp(int64_t dest)
   QEMU_TIMER_ATTR_ALL);
 int64_t warp = qemu_soonest_timeout(dest - clock, deadline);
 
-qtest_set_virtual_clock(qtest_get_virtual_clock() + warp);
+cpus_set_virtual_clock(cpus_get_virtual_clock() + warp);
 
 qemu_clock_run_timers(QEMU_CLOCK_VIRTUAL);
 timerlist_run_timers(aio_context->tlg.tl[QEMU_CLOCK_VIRTUAL]);
-- 
2.39.2

[PATCH v2 11/12] plugins: fix inject_mem_cb rw masking

[Alex Bennée]

From: Pierrick Bouvier 

These are not booleans, but masks.
Issue found by Richard Henderson.

Fixes: f86fd4d8721 ("plugins: distinct types for callbacks")
Signed-off-by: Richard Henderson 
Signed-off-by: Pierrick Bouvier 
Message-Id: <20240612195147.93121-3-pierrick.bouv...@linaro.org>
Signed-off-by: Alex Bennée 
---
 accel/tcg/plugin-gen.c | 4 ++--
 plugins/core.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index cc1634e7a6..b6bae32b99 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -240,13 +240,13 @@ static void inject_mem_cb(struct qemu_plugin_dyn_cb *cb,
 {
 switch (cb->type) {
 case PLUGIN_CB_MEM_REGULAR:
-if (rw && cb->regular.rw) {
+if (rw & cb->regular.rw) {
 gen_mem_cb(&cb->regular, meminfo, addr);
 }
 break;
 case PLUGIN_CB_INLINE_ADD_U64:
 case PLUGIN_CB_INLINE_STORE_U64:
-if (rw && cb->inline_insn.rw) {
+if (rw & cb->inline_insn.rw) {
 inject_cb(cb);
 }
 break;
diff --git a/plugins/core.c b/plugins/core.c
index badede28cf..9d737d8278 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -589,7 +589,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
 
 switch (cb->type) {
 case PLUGIN_CB_MEM_REGULAR:
-if (rw && cb->regular.rw) {
+if (rw & cb->regular.rw) {
 cb->regular.f.vcpu_mem(cpu->cpu_index,
make_plugin_meminfo(oi, rw),
vaddr, cb->regular.userp);
@@ -597,7 +597,7 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
 break;
 case PLUGIN_CB_INLINE_ADD_U64:
 case PLUGIN_CB_INLINE_STORE_U64:
-if (rw && cb->inline_insn.rw) {
+if (rw & cb->inline_insn.rw) {
 exec_inline_op(cb->type, &cb->inline_insn, cpu->cpu_index);
 }
 break;
-- 
2.39.2