Re: [PATCH 4/6] target/mips: Convert Loongson [D]DIVU.G opcodes to decodetree

[Philippe Mathieu-Daudé]

On 1/21/21 8:58 PM, Richard Henderson wrote:
> On 1/12/21 11:55 AM, Philippe Mathieu-Daudé wrote:
>> Convert DIVU.G (divide 32-bit unsigned integers) and DDIVU.G
>> (divide 64-bit unsigned integers) opcodes to decodetree.
>>
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  target/mips/godson2.decode|  2 ++
>>  target/mips/loong-ext.decode  |  2 ++
>>  target/mips/loong_translate.c | 55 +++
>>  target/mips/translate.c   | 37 ---
>>  4 files changed, 59 insertions(+), 37 deletions(-)

>> +static bool gen_lext_DIVU_G(DisasContext *s, int rd, int rs, int rt,
>> +bool is_double)
>> +{
>> +TCGv t0, t1;
>> +TCGLabel *l1, *l2;
>> +
>> +if (is_double) {
>> +if (TARGET_LONG_BITS != 64) {
>> +return false;
>> +}
>> +check_mips_64(s);
>> +}
>> +
>> +if (rd == 0) {
>> +/* Treat as NOP. */
>> +return true;
>> +}
>> +
>> +t0 = tcg_temp_local_new();
>> +t1 = tcg_temp_local_new();
>> +l1 = gen_new_label();
>> +l2 = gen_new_label();
>> +
>> +gen_load_gpr(t0, rs);
>> +gen_load_gpr(t1, rt);
>> +
>> +if (!is_double) {
>> +tcg_gen_ext32u_tl(t0, t0);
>> +tcg_gen_ext32u_tl(t1, t1);
>> +}
>> +tcg_gen_brcondi_tl(TCG_COND_NE, t1, 0, l1);
>> +tcg_gen_movi_tl(cpu_gpr[rd], 0);
>> +
>> +tcg_gen_br(l2);
>> +gen_set_label(l1);
>> +tcg_gen_divu_tl(cpu_gpr[rd], t0, t1);
>> +tcg_gen_ext32s_tl(cpu_gpr[rd], cpu_gpr[rd]);
> 
> this extend should be conditional on !is_double.

Oops, thanks!

> 
> Otherwise,
> Reviewed-by: Richard Henderson 
> 
> r~
> 

Re: [PATCH] ui/sdl2: Check return value from g_setenv()

[Gerd Hoffmann]

  Hi,

> Setting environment variables can fail; check the return value
> from g_setenv() and bail out if we couldn't set SDL_VIDEODRIVER.

Hmm, looking at the comment I'm wondering whenever we maybe should just
drop the setenv (after 6.1).  It's quite old, I doubt svgalib talking
directly to the hardware is still a thing these days, and we have the
x11 -> wayland shift happening ...

take care,
  Gerd

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Gerd Hoffmann]

  Hi,

> this part actually works. .needed is only evaluated on the sending side. For
> the receiving side subsections are optional.  Migration doesn't fail if a
> subsection isn't loaded. Before I sent this patch series one of the
> migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the
> command reply queue and 3 bytes in the scancode queue. The migration didn't
> fail.

Hmm, ok.  If you actually tested it you are probably right.  My memory
tells me ->needed() is evaluated on both sending and receiving side as
the migration data stream does not carry the information whenever a
subsection is present or not.  But maybe my memories are wrong, or
things have changed, I don't follow migration changes that closely.

> > If we can't find something we can add a property simliar to the one
> > for the extended keyboard state.
> 
> What is the best way to add such a compat property? The ps2 keyboard isn't a
> qdev device. I can't just add a property to the device class. Do I have to
> add a property to the i8042 and the pl050 device and propagate the property
> value with the ps2_kbd_init() call to the PS2KbdState?

Yes, I think so.  But double-check the migration thing first, if your
approach works that is the easier way of course.

take care,
  Gerd

Re: [PATCH] fsl-imx6ul: add SAI1/2/3 and ASRC as unimplemented devices

[Philippe Mathieu-Daudé]

Hi Guenter,

On 8/10/21 6:10 AM, Guenter Roeck wrote:
> Define SAI1/2/3 and ASRC as unimplemented devices to avoid random
> Linux kernel crashes.

Relevant dmesg output could be useful.

> Signed-off-by: Guenter Roeck 
> ---
>  hw/arm/fsl-imx6ul.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
> index e0128d7316..48b60eb3ce 100644
> --- a/hw/arm/fsl-imx6ul.c
> +++ b/hw/arm/fsl-imx6ul.c
> @@ -534,6 +534,13 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
> **errp)
>   */
>  create_unimplemented_device("sdma", FSL_IMX6UL_SDMA_ADDR, 0x4000);
>  
> +/*
> + * SAI

"Audio SSI (Synchronous Serial Interface)"

> + */
> +create_unimplemented_device("sai1", FSL_IMX6UL_SAI1_ADDR, 0x4000);
> +create_unimplemented_device("sai2", FSL_IMX6UL_SAI2_ADDR, 0x4000);
> +create_unimplemented_device("sai3", FSL_IMX6UL_SAI3_ADDR, 0x4000);

Hmm I see these named SSI[123] in the datasheet.

  The Synchronous Serial Interface (SSI) is a full-duplex serial
  port that allows communication with external devices using a
  variety of serial protocols. The SSI supports a wide variety of
  protocols (SSI normal, SSI network, I2S, and AC-97), bit depths
  (up to 24 bits per word), and clock/frame sync options.

  The three SSIs may support three audio streams (possibly at
  different sample rates) simultaneously. SSI1, SSI2 and SSI3 are
  located on the Shared Peripheral Bus. Since the SDMA can directly
  access SSI1...SSI3 (being on the Shared Peripheral Bus), they can
  be used for high-bandwidth data transfers in order to optimize
  bus bandwidth consumption.

Since QEMU models SPI devices in hw/ssi/, having the devices named
"sai*" is OK.

> +
>  /*
>   * PWM
>   */
> @@ -542,6 +549,11 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
> **errp)
>  create_unimplemented_device("pwm3", FSL_IMX6UL_PWM3_ADDR, 0x4000);
>  create_unimplemented_device("pwm4", FSL_IMX6UL_PWM4_ADDR, 0x4000);
>  
> +/*
> + * ASRC

"Audio ASRC (asynchronous sample rate converter)"

Preferably updating the descriptions:
Reviewed-by: Philippe Mathieu-Daudé 

> + */
> +create_unimplemented_device("asrc", FSL_IMX6UL_ASRC_ADDR, 0x4000);
> +
>  /*
>   * CAN
>   */
> 

Re: [PATCH] monitor/hmp: schedule qemu_chr_fe_accept_input() after read

[Volker Rümelin]

Since commit 584af1f1d9 (ui/gtk: add a keyboard fifo to the VTE
consoles) a GTK VTE console chardev backend relies on the
connected chardev frontend to call qemu_chr_fe_accept_input()
whenever it can receive new characters. The HMP monitor doesn't
do this. It only schedules a call to qemu_chr_fe_accept_input()
after it handled a HMP command in monitor_command_cb().

This is a problem if you paste a few characters into the GTK
monitor console. Even entering a UTF-8 multibyte character leads
to the same problem.

Schedule a call to qemu_chr_fe_accept_input() after handling the
received bytes to fix the HMP monitor.

Signed-off-by: Volker Rümelin mailto:vr_q...@t-online.de>>


Wouldn't it work to make gd_vc_send_chars() write in a loop (until it 
fails)?


Hi Marc-André,

yes that works. I found similar code in the udp_chr_flush_buffer() 
function in chardev/char-udp.c. I will send a new patch within the next 
few hours.


With best regards,
Volker



If not, I think monitor/qmp.c should be adjusted too.

---
 monitor/hmp.c              |  1 +
 monitor/monitor-internal.h |  1 +
 monitor/monitor.c          | 19 +--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index d50c3124e1..470f56a71d 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1349,6 +1349,7 @@ static void monitor_read(void *opaque, const
uint8_t *buf, int size)
         for (i = 0; i < size; i++) {
             readline_handle_byte(mon->rs, buf[i]);
         }
+        monitor_accept_input(>common);
     } else {
         if (size == 0 || buf[size - 1] != 0) {
             monitor_printf(>common, "corrupted command\n");
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 9c3a09cb01..af33c3c617 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -170,6 +170,7 @@ int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
                        bool use_io_thread);
 void monitor_data_destroy(Monitor *mon);
+void monitor_accept_input(Monitor *mon);
 int monitor_can_read(void *opaque);
 void monitor_list_append(Monitor *mon);
 void monitor_fdsets_cleanup(void);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 46a171bca6..8e3cf4ad98 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -519,13 +519,28 @@ int monitor_suspend(Monitor *mon)
     return 0;
 }

-static void monitor_accept_input(void *opaque)
+static void monitor_accept_input_bh(void *opaque)
 {
     Monitor *mon = opaque;

     qemu_chr_fe_accept_input(>chr);
 }

+void monitor_accept_input(Monitor *mon)
+{
+    if (!qatomic_mb_read(>suspend_cnt)) {
+        AioContext *ctx;
+
+        if (mon->use_io_thread) {
+            ctx = iothread_get_aio_context(mon_iothread);
+        } else {
+            ctx = qemu_get_aio_context();
+        }
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
+    }
+}
+
 void monitor_resume(Monitor *mon)
 {
     if (monitor_is_hmp_non_interactive(mon)) {
@@ -547,7 +562,7 @@ void monitor_resume(Monitor *mon)
             readline_show_prompt(hmp_mon->rs);
         }

-        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input_bh, mon);
     }

     trace_monitor_suspend(mon, -1);
-- 
2.26.2





--
Marc-André Lureau

Re: [PULL 24/30] spapr_pci: populate ibm,loc-code

[Philippe Mathieu-Daudé]

On 8/10/21 6:29 AM, David Gibson wrote:
> On Mon, Aug 09, 2021 at 10:57:00AM +0100, Peter Maydell wrote:
>> On Tue, 7 Jul 2015 at 16:49, Alexander Graf  wrote:
>>>
>>> From: Nikunj A Dadhania 
>>>
>>> Each hardware instance has a platform unique location code.  The OF
>>> device tree that describes a part of a hardware entity must include
>>> the “ibm,loc-code” property with a value that represents the location
>>> code for that hardware entity.
>>>
>>> Populate ibm,loc-code.
>>
>> Ancient patch, but Coverity has just noticed a bug in it
>> which is still present in current QEMU (CID 1460454):
>>
>>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice 
>>> *pdev)
>>> +{
>>> +char *path = NULL, *buf = NULL, *host = NULL;
>>> +
>>> +/* Get the PCI VFIO host id */
>>> +host = object_property_get_str(OBJECT(pdev), "host", NULL);
>>> +if (!host) {
>>> +goto err_out;
>>> +}
>>> +
>>> +/* Construct the path of the file that will give us the DT location */
>>> +path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
>>> +g_free(host);
>>> +if (!path || !g_file_get_contents(path, , NULL, NULL)) {
>>> +goto err_out;
>>> +}
>>> +g_free(path);
>>
>> Here we create a 'path' string, use it as the argument to
>> g_file_get_contents() and then free it (either here or in the err_out 
>> path)...
>>
>>> +
>>> +/* Construct and read from host device tree the loc-code */
>>> +path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
>>> +g_free(buf);
>>> +if (!path || !g_file_get_contents(path, , NULL, NULL)) {
>>> +goto err_out;
>>> +}
>>> +return buf;
>>
>> ...but here we forget to free it before returning in the success case.
>>
>>> +
>>> +err_out:
>>> +g_free(path);
>>> +return NULL;
>>> +}
>>
>> Cleanest fix would be to declare 'path' and 'host' as
>>g_autofree char *path = NULL;
>>g_autofree char *host = NULL;
>> and then you can remove all the manual g_free(path) and g_free(host) calls.
> 
> Thanks for the report.  I've committed the fix (I hope) below to ppc-for-6.1:
> 
> From 70ae61b510dc571c407b28c46498cae60e60ca66 Mon Sep 17 00:00:00 2001
> From: David Gibson 
> Date: Tue, 10 Aug 2021 14:28:19 +1000
> Subject: [PATCH] spapr_pci: Fix leak in spapr_phb_vfio_get_loc_code() with
>  g_autofree
> 
> This uses g_autofree to simplify logic in spapr_phb_vfio_get_loc_code(),
> in the process fixing a leak in one of the paths.  I'm told this fixes
> Coverity error CID 1460454
> 
> Reported-by: Peter Maydell 
> Fixes: 16b0ea1d852 ("spapr_pci: populate ibm,loc-code")
> Signed-off-by: David Gibson 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/ppc/spapr_pci.c | 17 ++---
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7a725855f9..13d806f390 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -782,33 +782,28 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, 
> void *opaque, int devfn)
>  
>  static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb,  PCIDevice 
> *pdev)
>  {
> -char *path = NULL, *buf = NULL, *host = NULL;
> +g_autofree char *path = NULL;
> +g_autofree char *host = NULL;
> +char *buf = NULL;
>  
>  /* Get the PCI VFIO host id */
>  host = object_property_get_str(OBJECT(pdev), "host", NULL);
>  if (!host) {
> -goto err_out;
> +return NULL;
>  }
>  
>  /* Construct the path of the file that will give us the DT location */
>  path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
> -g_free(host);
>  if (!g_file_get_contents(path, , NULL, NULL)) {
> -goto err_out;
> +return NULL;
>  }
> -g_free(path);
>  
>  /* Construct and read from host device tree the loc-code */
>  path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> -g_free(buf);
>  if (!g_file_get_contents(path, , NULL, NULL)) {
> -goto err_out;
> +return NULL;
>  }
>  return buf;
> -
> -err_out:
> -g_free(path);
> -return NULL;
>  }
>  
>  static char *spapr_phb_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
> 

Re: [PATCH] xive: Remove extra '0x' prefix in trace events

[Philippe Mathieu-Daudé]

On 8/10/21 2:56 AM, David Gibson wrote:
> On Mon, Aug 09, 2021 at 11:39:49AM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/9/21 10:52 AM, Cédric Le Goater wrote:
>>> Cc: th...@redhat.com
>>> Fixes: 4e960974d4ee ("xive: Add trace events")
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/519
>>> Signed-off-by: Cédric Le Goater 
>>> ---
>>>  hw/intc/trace-events | 10 +-
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>> index e56e7dd3b667..6a17d38998d9 100644
>>> --- a/hw/intc/trace-events
>>> +++ b/hw/intc/trace-events
>>> @@ -219,14 +219,14 @@ kvm_xive_source_reset(uint32_t srcno) "IRQ 0x%x"
>>>  xive_tctx_accept(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr, 
>>> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x 
>>> CPPR=0x%02x NSR=0x%02x ACK"
>>>  xive_tctx_notify(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr, 
>>> uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x 
>>> CPPR=0x%02x NSR=0x%02x raise !"
>>>  xive_tctx_set_cppr(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t 
>>> pipr, uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x 
>>> PIPR=0x%02x new CPPR=0x%02x NSR=0x%02x"
>>> -xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value) 
>>> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
>>> -xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value) 
>>> "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
>>> +xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value) 
>>> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
>>> +xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value) 
>>> "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
>>>  xive_router_end_notify(uint8_t end_blk, uint32_t end_idx, uint32_t 
>>> end_data) "END 0x%02x/0x%04x -> enqueue 0x%08x"
>>>  xive_router_end_escalate(uint8_t end_blk, uint32_t end_idx, uint8_t 
>>> esc_blk, uint32_t esc_idx, uint32_t end_data) "END 0x%02x/0x%04x -> 
>>> escalate END 0x%02x/0x%04x data 0x%08x"
>>> -xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) 
>>> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
>>> -xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) 
>>> "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
>>> +xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) 
>>> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
>>> +xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) 
>>> "@0x%"PRIx64" sz=%d val=0x%" PRIx64
>>>  xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring) 
>>> "found NVT 0x%x/0x%x ring=0x%x"
>>> -xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) 
>>> "END 0x%x/0x%x @0x0x%"PRIx64
>>> +xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) 
>>> "END 0x%x/0x%x @0x%"PRIx64
>>>  
>>>  # pnv_xive.c
>>>  pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" 
>>> val=0x%"PRIx64
>>>
>>
>> Acceptable for 6.1 IMHO.
> 
> Acceptable for, but also not vital for.  I've applied this to
> ppc-for-6.1, but I'll probably only bother sending a PR if some more
> crucial fixes come along.

Fair enough :)

Re: [PATCH 3/3] ps2: migration support for command reply queue

[Volker Rümelin]

   Hi,


+static bool ps2_keyboard_cqueue_needed(void *opaque)
+{
+PS2KbdState *s = opaque;
+
+return s->common.queue.cwptr != -1; /* the queue is mostly empty */
+}
+
+static const VMStateDescription vmstate_ps2_keyboard_cqueue = {
+.name = "ps2kbd/command_reply_queue",
+.needed = ps2_keyboard_cqueue_needed,
+.fields = (VMStateField[]) {
+VMSTATE_INT32(common.queue.cwptr, PS2KbdState),
+VMSTATE_END_OF_LIST()
+}
+};

Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has
the same problem btw).  Chicken and egg problem on the receiving side:
How you properly evaluate ps2_keyboard_cqueue_needed() without having
common.queue.cwptr received yet?

With "cqueue not needed" being the common case migration will work
nicely in most cases nevertheless, but when the source machine actually
sends cqueue state things will break ...


Hi Gerd,

this part actually works. .needed is only evaluated on the sending side. 
For the receiving side subsections are optional. Migration doesn't fail 
if a subsection isn't loaded. Before I sent this patch series one of the 
migration tests was a migration from 6.0.92 to 6.0.92 with one byte in 
the command reply queue and 3 bytes in the scancode queue. The migration 
didn't fail.


Migration will fail in the rare case when a new QEMU sends the 
command_reply_queue subsection to an old QEMU version.



Looking at data sent via vmstate works but ordering is important.  You
could check write_cmd because that is sent in the migration data stream
before ps2_keyboard_cqueue_needed() will be evaluated (just an random
example for the ordering, probably wouldn't help much in this case).

There is some redundancy in the data stream (wptr + rptr would be
enough, but we also send count).  Maybe that can be used somehow.
Of course the tricky part is to not confuse old qemu versions on
the receiving end.

If we can't find something we can add a property simliar to the one
for the extended keyboard state.


What is the best way to add such a compat property? The ps2 keyboard 
isn't a qdev device. I can't just add a property to the device class. Do 
I have to add a property to the i8042 and the pl050 device and propagate 
the property value with the ps2_kbd_init() call to the PS2KbdState?


With best regards,
Volker


take care,
   Gerd

Re: [PATCH] fuzz: avoid building twice, when running on gitlab

[Philippe Mathieu-Daudé]

+Coiby Xu & qemu-block@

On 8/9/21 9:36 PM, Peter Maydell wrote:
> On Mon, 9 Aug 2021 at 20:30, Alexander Bulekov  wrote:
>>
>> On 210809 1506, Alexander Bulekov wrote:
>>> On 210809 1925, Peter Maydell wrote:
 On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov  wrote:
>
> On oss-fuzz, we build twice, to put together a build that is portable to
> the runner containers. On gitlab ci, this is wasteful and contributes to
> timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at
> the remote cost of potentially missing some cases that break oss-fuzz
> builds.
>
> Signed-off-by: Alexander Bulekov 
> ---
>
> From a couple test runs it looks like this can shave off 15-20 minutes.
>
>  scripts/oss-fuzz/build.sh | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

 I tried a test run with this, but it still hit the 1 hour timeout:

 https://gitlab.com/qemu-project/qemu/-/pipelines/350387482
>>>
>>> It also timed out for me with a 120 minute timeout:
>>> https://gitlab.com/a1xndr/qemu/-/jobs/1488160601
>>>
>>> The log has almost exactly the same number of lines as yours, so I'm
>>> guessing one of the qtests is timing out with --enable-sanitizers .
> 
>>
>> Building locally:
>> $ CC=clang-11 CXX=clang++-11 ../configure --enable-fuzzing \
>> --enable-debug --enable-sanitizers
>> $ make check-qtest-i386 check-unit
>>
>> Same as on gitlab, this times out shortly after outputting
>> "sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found"
>>
>> Manually running qos-test, the same way check-qtest-i386 invokes it:
>>
>> $ QTEST_QEMU_BINARY=./qemu-system-i386 
>> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
>> tests/qtest/qos-test --tap -k -m quick < /dev/null
>>
>> # starting vhost-user backend: exec ./storage-daemon/qemu-storage-daemon 
>> --blockdev driver=file,node-name=disk0,filename=qtest.XRAzzu --export 
>> type=vhost-user-blk,id=disk0,addr.type=unix,addr.path=/tmp/qtest-94561-sock.NdKWpt,node-name=disk0,writable=on,num-queues=1
>> sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found
>> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-94561.sock 
>> -qtest-log /dev/null -chardev socket,path=/tmp/qtest-94561.qmp,id=char0 -mon 
>> chardev=char0,mode=control -display none -M pc  -device 
>> vhost-user-blk-pci,id=drv0,chardev=char1,addr=4.0 -object 
>> memory-backend-memfd,id=mem,size=256M,share=on  -M memory-backend=mem -m 
>> 256M -chardev socket id=char1,path=/tmp/qtest-94561-sock.NdKWpt  -accel qtest
>>
>> *timeout*
> 
> vhost-user timing out in realize I suspect. I see that as
> an intermittent hang in non-sanitizer configs.
> 
> vhost-user folk: Can we either look at fixing this or else disable
> the test ? (Stack backtraces available in the other email thread.)
> 
> thanks
> -- PMM
> 

Re: [PATCH] audio: Never send migration section

[Philippe Mathieu-Daudé]

On 8/9/21 7:12 PM, Daniel P. Berrangé wrote:
> On Mon, Aug 09, 2021 at 06:09:56PM +0100, Dr. David Alan Gilbert (git) wrote:
>> From: "Dr. David Alan Gilbert" 
>>
>> The audio migration vmstate is empty, and always has been; we can't
>> just remove it though because an old qemu might send it us.
>> Changes with -audiodev now mean it's sometimes created when it didn't
>> used to be, and can confuse migration to old qemu.

Not a 6.1 regression but easy change for rc3 IMO.

>> Change it so that vmstate_audio is never sent; if it's received it
>> should still be accepted, and old qemu's shouldn't be too upset if it's
>> missing.

Worth Cc: stable@ maybe?

>> Signed-off-by: Dr. David Alan Gilbert 
>> ---
>>  audio/audio.c | 10 ++
>>  1 file changed, 10 insertions(+)
> 
> Reviewed-by: Daniel P. Berrangé 
> Tested-by: Daniel P. Berrangé 
> 
> 
> For testing I have a VM with -audiodev, but *WITHOUT* any sound
> frontend devices. Live migrating to a QEMU using QEMU_AUDIO_DRV
> would previously fail. With this applied it now works, showing
> that we dont uncessarily send this.
> 
> I also tested migration to a QEMU with -audiodev, but without
> this patch, and that still works as before, showing that QEMU
> is happy if this section is not sent.
> 
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index 59453ef856..54a153c0ef 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -1622,10 +1622,20 @@ void audio_cleanup(void)
>>  }
>>  }
>>  
>> +static bool vmstate_audio_needed(void *opaque)
>> +{
>> +/*
>> + * Never needed, this vmstate only exists in case
>> + * an old qemu sends it to us.
>> + */
>> +return false;
>> +}
>> +
>>  static const VMStateDescription vmstate_audio = {
>>  .name = "audio",
>>  .version_id = 1,
>>  .minimum_version_id = 1,
>> +.needed = vmstate_audio_needed,
>>  .fields = (VMStateField[]) {
>>  VMSTATE_END_OF_LIST()
>>  }
>> -- 
>> 2.31.1
>>
> 
> Regards,
> Daniel
> 

Re: [PULL 24/30] spapr_pci: populate ibm,loc-code

[David Gibson]

On Mon, Aug 09, 2021 at 10:57:00AM +0100, Peter Maydell wrote:
> On Tue, 7 Jul 2015 at 16:49, Alexander Graf  wrote:
> >
> > From: Nikunj A Dadhania 
> >
> > Each hardware instance has a platform unique location code.  The OF
> > device tree that describes a part of a hardware entity must include
> > the “ibm,loc-code” property with a value that represents the location
> > code for that hardware entity.
> >
> > Populate ibm,loc-code.
> 
> Ancient patch, but Coverity has just noticed a bug in it
> which is still present in current QEMU (CID 1460454):
> 
> > +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice 
> > *pdev)
> > +{
> > +char *path = NULL, *buf = NULL, *host = NULL;
> > +
> > +/* Get the PCI VFIO host id */
> > +host = object_property_get_str(OBJECT(pdev), "host", NULL);
> > +if (!host) {
> > +goto err_out;
> > +}
> > +
> > +/* Construct the path of the file that will give us the DT location */
> > +path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
> > +g_free(host);
> > +if (!path || !g_file_get_contents(path, , NULL, NULL)) {
> > +goto err_out;
> > +}
> > +g_free(path);
> 
> Here we create a 'path' string, use it as the argument to
> g_file_get_contents() and then free it (either here or in the err_out path)...
> 
> > +
> > +/* Construct and read from host device tree the loc-code */
> > +path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
> > +g_free(buf);
> > +if (!path || !g_file_get_contents(path, , NULL, NULL)) {
> > +goto err_out;
> > +}
> > +return buf;
> 
> ...but here we forget to free it before returning in the success case.
> 
> > +
> > +err_out:
> > +g_free(path);
> > +return NULL;
> > +}
> 
> Cleanest fix would be to declare 'path' and 'host' as
>g_autofree char *path = NULL;
>g_autofree char *host = NULL;
> and then you can remove all the manual g_free(path) and g_free(host) calls.

Thanks for the report.  I've committed the fix (I hope) below to ppc-for-6.1:

From 70ae61b510dc571c407b28c46498cae60e60ca66 Mon Sep 17 00:00:00 2001
From: David Gibson 
Date: Tue, 10 Aug 2021 14:28:19 +1000
Subject: [PATCH] spapr_pci: Fix leak in spapr_phb_vfio_get_loc_code() with
 g_autofree

This uses g_autofree to simplify logic in spapr_phb_vfio_get_loc_code(),
in the process fixing a leak in one of the paths.  I'm told this fixes
Coverity error CID 1460454

Reported-by: Peter Maydell 
Fixes: 16b0ea1d852 ("spapr_pci: populate ibm,loc-code")
Signed-off-by: David Gibson 
---
 hw/ppc/spapr_pci.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7a725855f9..13d806f390 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -782,33 +782,28 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus, 
void *opaque, int devfn)
 
 static char *spapr_phb_vfio_get_loc_code(SpaprPhbState *sphb,  PCIDevice *pdev)
 {
-char *path = NULL, *buf = NULL, *host = NULL;
+g_autofree char *path = NULL;
+g_autofree char *host = NULL;
+char *buf = NULL;
 
 /* Get the PCI VFIO host id */
 host = object_property_get_str(OBJECT(pdev), "host", NULL);
 if (!host) {
-goto err_out;
+return NULL;
 }
 
 /* Construct the path of the file that will give us the DT location */
 path = g_strdup_printf("/sys/bus/pci/devices/%s/devspec", host);
-g_free(host);
 if (!g_file_get_contents(path, , NULL, NULL)) {
-goto err_out;
+return NULL;
 }
-g_free(path);
 
 /* Construct and read from host device tree the loc-code */
 path = g_strdup_printf("/proc/device-tree%s/ibm,loc-code", buf);
-g_free(buf);
 if (!g_file_get_contents(path, , NULL, NULL)) {
-goto err_out;
+return NULL;
 }
 return buf;
-
-err_out:
-g_free(path);
-return NULL;
 }
 
 static char *spapr_phb_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
-- 
2.31.1



-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for 6.2 31/49] bsd-user: Remove dead #ifdefs from elfload.c

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh

LOW_ELF_STACK doesn't exist on FreeBSD and likely never will. Remove it.
Likewise, remove an #if 0 block that's not useful

Signed-off-by: Warner Losh
---
  bsd-user/elfload.c | 20 
  1 file changed, 20 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH 14/19] target/ppc/pmu_book3s_helper.c: add generic timeout helpers

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:52AM -0300, Daniel Henrique Barboza wrote:
> Before adding counter negative condition support for the other 5
> counters, create generic helpers that retrieves the elapsed timeout to
> counter negative based on the event being sampled.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/pmu_book3s_helper.c | 41 +-
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index e7af273cb6..7126e9b3d5 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -111,23 +111,52 @@ static void update_PMCs(CPUPPCState *env, uint64_t 
> icount_delta)
>  update_PMC_PM_CYC(env, SPR_POWER_PMC6, icount_delta);
>  }
>  
> +static int64_t get_INST_CMPL_timeout(CPUPPCState *env, int sprn)
> +{
> +int64_t remaining_insns;
> +
> +if (env->spr[sprn] == 0) {

Why do you need to special case the PMC being 0?

> +return icount_to_ns(COUNTER_NEGATIVE_VAL);
> +}
> +
> +if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> +return 0;
> +}
> +
> +remaining_insns = COUNTER_NEGATIVE_VAL - env->spr[sprn];
> +return icount_to_ns(remaining_insns);
> +}
> +
> +static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
> +{
> +int64_t remaining_cyc;
> +
> +if (env->spr[sprn] == 0) {
> +return icount_to_ns(COUNTER_NEGATIVE_VAL);

Why is icount involved in the CYC timeout logic?  AFAICT it wasn't
before this change.

> +}
> +
> +if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> +return 0;
> +}
> +
> +remaining_cyc = COUNTER_NEGATIVE_VAL - env->spr[sprn];
> +return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, PPC_CPU_FREQ);
> +}
> +
>  static void set_PMU_excp_timer(CPUPPCState *env)
>  {
> -uint64_t timeout, now, remaining_val;
> +uint64_t timeout, now;
>  
>  if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
>  return;
>  }
>  
> -remaining_val = COUNTER_NEGATIVE_VAL - env->spr[SPR_POWER_PMC1];
> -
>  switch (get_PMC_event(env, SPR_POWER_PMC1)) {
>  case 0x2:
> -timeout = icount_to_ns(remaining_val);
> +timeout = get_INST_CMPL_timeout(env, SPR_POWER_PMC1);
>  break;
>  case 0x1e:
> -timeout = muldiv64(remaining_val, NANOSECONDS_PER_SECOND,
> -   PPC_CPU_FREQ);
> +timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
>  break;
>  default:
>  return;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for 6.2 32/49] bsd-user: *BSD specific siginfo defintions

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh

Add FreeBSD, NetBSD and OpenBSD values for the various signal info types
and defines to decode different signals to discover more information
about the specific signal types.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---


Acked-by: Richard Henderson 

r~

Re: [PATCH 13/19] target/ppc/translate: PMU: handle setting of PMCs while running

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:51AM -0300, Daniel Henrique Barboza wrote:
> The initial PMU support were made under the assumption that the counters
> would be set before running the PMU and read after either freezing the
> PMU manually or via a performance monitor alert.
> 
> Turns out that some EBB powerpc kernel tests set the counters after
> unfreezing the counters. Setting a PMC value when the PMU is running
> means that, at that moment, the baseline for calculating the events (set
> in env->pmu_base_icount) needs to be updated. Updating this baseline
> means that we need to update all the PMCs with their actual value at
> that moment. Any existing counter negative timer needs to be discarded
> an a new one, with the updated values, must be set again.
> 
> This patch does that via a new 'helper_store_pmc()' that is called in
> the mtspr() callbacks of the PMU registers, spr_write_pmu_ureg() and
> spr_write_pmu_generic() in target/ppc/translate.c
> 
> With this change, EBB powerpc kernel tests such as  'no_handler_test'
> are now passing.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/helper.h|  1 +
>  target/ppc/pmu_book3s_helper.c | 36 --
>  target/ppc/translate.c | 27 +
>  3 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 5122632784..757665b360 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, void, env)
>  DEF_HELPER_2(store_lpcr, void, env, tl)
>  DEF_HELPER_2(store_pcr, void, env, tl)
>  DEF_HELPER_2(store_mmcr0, void, env, tl)
> +DEF_HELPER_3(store_pmc, void, env, i32, i64)
>  #endif
>  DEF_HELPER_1(check_tlb_flush_local, void, env)
>  DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index 58ae65e22b..e7af273cb6 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -173,7 +173,7 @@ void cpu_ppc_pmu_timer_init(CPUPPCState *env)
>  env->pmu_intr_timer = timer;
>  }
>  
> -static bool mmcr0_counter_neg_cond_enabled(uint64_t mmcr0)
> +static bool counter_negative_cond_enabled(uint64_t mmcr0)

Can you fold this rename into the patch which introduces the function
please.

>  {
>  return mmcr0 & MMCR0_PMC1CE;
>  }
> @@ -219,9 +219,41 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong 
> value)
>   * Start performance monitor alert timer for counter negative
>   * events, if needed.
>   */
> -if (mmcr0_counter_neg_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
> +if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
>  set_PMU_excp_timer(env);
>  }
>  }
>  }
>  }
> +
> +void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> +{
> +bool pmu_frozen = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
> +uint64_t curr_icount, icount_delta;
> +
> +if (pmu_frozen) {
> +env->spr[sprn] = value;
> +return;
> +}
> +
> +curr_icount = (uint64_t)icount_get_raw();
> +icount_delta = curr_icount - env->pmu_base_icount;
> +
> +/* Update the counter with the events counted so far */
> +update_PMCs(env, icount_delta);
> +
> +/* Set the counter to the desirable value after update_PMCs() */
> +env->spr[sprn] = value;
> +
> +/*
> + * Delete the current timer and restart a new one with the
> + * updated values.
> + */
> +timer_del(env->pmu_intr_timer);
> +
> +env->pmu_base_icount = curr_icount;

I'd expect some of this code to be shared with the unfreeze path using
a helper.  Is there a reason that's not the case?

Do you also need to deal with any counter interrupts that have already
been generated by the old counter?  Are the counter overflow events
edge-triggered or level-triggered?

> +if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
> +set_PMU_excp_timer(env);
> +}
> +}
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index afc254a03f..3e890cc4d8 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -409,11 +409,25 @@ void spr_write_generic(DisasContext *ctx, int sprn, int 
> gprn)
>  
>  void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn)
>  {
> +TCGv_i32 t_sprn;
> +
>  switch (sprn) {
>  case SPR_POWER_MMCR0:
>  gen_icount_io_start(ctx);
>  gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>  break;
> +case SPR_POWER_PMC1:
> +case SPR_POWER_PMC2:
> +case SPR_POWER_PMC3:
> +case SPR_POWER_PMC4:
> +case SPR_POWER_PMC5:
> +case SPR_POWER_PMC6:
> +gen_icount_io_start(ctx);
> +
> +t_sprn = tcg_const_i32(sprn);
> +gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
> +tcg_temp_free_i32(t_sprn);
> +break;
>  default:

Re: [PATCH 18/19] target/ppc/pmu_book3s_helper.c: add PM_CMPLU_STALL mock events

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:56AM -0300, Daniel Henrique Barboza wrote:
> EBB powerpc kernel test 'multi_counter_test' uses PM_CMPLU_STALL events
> that we do not support. These events are related to CPU stalled/wasted
> cycles while waiting for resources, cache misses and so on.
> 
> Unlike the 0xFA event added previously, there's no available equivalent
> for us to use, and at this moment we can't sample those events as well.
> What we can do is mock those events as if we were calculating them.
> 
> This patch implements PM_CMPLU_STALL, PM_CMPLU_STALL_FXU,
> PM_CMPLU_STALL_OTHER_CMPL and PM_CMPLU_STALL_THRD mock events by giving
> them a fixed amount of the total elapsed cycles.
> 
> The chosen sample values for these events (25% of total cycles for
> PM_CMPLU_STALL and 5% for the other three) were chosen at random and has
> no intention of being truthful with what a real PowerPC hardware would
> give us. Our intention here is to make 'multi_counter_test' EBB test
> pass.

Hmm.  I guess these mock values make sense for getting the kernel
tests to pass, but I'm not sure if it's a good idea in general.  Would
we be better off just reporting 0 always - that would be a strong hint
to someone attempting to analyze results that something is fishy (in
this case that they don't actually have a real CPU).

> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/pmu_book3s_helper.c | 81 +-
>  1 file changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index ae7050cd62..32cf76b77f 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -92,16 +92,54 @@ static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>  env->spr[sprn] += get_cycles(icount_delta);
>  }
>  
> +static int get_stall_ratio(uint8_t stall_event)
> +{
> +int stall_ratio = 0;
> +
> +switch (stall_event) {
> +case 0xA:
> +stall_ratio = 25;
> +break;
> +case 0x6:
> +case 0x16:
> +case 0x1C:
> +stall_ratio = 5;
> +break;
> +default:
> +break;
> +}
> +
> +return stall_ratio;
> +}
> +
> +static void update_PMC_PM_STALL(CPUPPCState *env, int sprn,
> +uint64_t icount_delta,
> +uint8_t stall_event)
> +{
> +int stall_ratio = get_stall_ratio(stall_event);
> +uint64_t cycles = muldiv64(get_cycles(icount_delta), stall_ratio, 100);
> +
> +env->spr[sprn] += cycles;
> +}
> +
>  static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
>  uint64_t icount_delta)
>  {
> -switch (get_PMC_event(env, sprn)) {
> +uint8_t event = get_PMC_event(env, sprn);
> +
> +switch (event) {
>  case 0x2:
>  update_PMC_PM_INST_CMPL(env, sprn, icount_delta);
>  break;
>  case 0x1E:
>  update_PMC_PM_CYC(env, sprn, icount_delta);
>  break;
> +case 0xA:
> +case 0x6:
> +case 0x16:
> +case 0x1C:
> +update_PMC_PM_STALL(env, sprn, icount_delta, event);
> +break;
>  default:
>  return;
>  }
> @@ -163,6 +201,34 @@ static int64_t get_CYC_timeout(CPUPPCState *env, int 
> sprn)
>  return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, PPC_CPU_FREQ);
>  }
>  
> +static int64_t get_stall_timeout(CPUPPCState *env, int sprn,
> + uint8_t stall_event)
> +{
> +uint64_t remaining_cyc;
> +int stall_multiplier;
> +
> +if (env->spr[sprn] == 0) {
> +return icount_to_ns(COUNTER_NEGATIVE_VAL);
> +}
> +
> +if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> +return 0;
> +}
> +
> +remaining_cyc = COUNTER_NEGATIVE_VAL - env->spr[sprn];
> +
> +/*
> + * Consider that for this stall event we'll advance the counter
> + * in a lower rate, thus requiring more cycles to overflow.
> + * E.g. for PM_CMPLU_STALL (0xA), ratio 25, it'll require
> + * 100/25 = 4 times the same amount of cycles to overflow.
> + */
> +stall_multiplier = 100 / get_stall_ratio(stall_event);
> +remaining_cyc *= stall_multiplier;
> +
> +return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, PPC_CPU_FREQ);
> +}
> +
>  static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
>  {
>  bool PMC14_running = !(env->spr[SPR_POWER_MMCR0] & MMCR0_FC14);
> @@ -191,6 +257,7 @@ static bool pmc_counter_negative_enabled(CPUPPCState 
> *env, int sprn)
>  static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
>  {
>  int64_t timeout = -1;
> +uint8_t event;
>  
>  if (!pmc_counter_negative_enabled(env, sprn)) {
>  return -1;
> @@ -205,13 +272,23 @@ static int64_t get_counter_neg_timeout(CPUPPCState 
> *env, int sprn)
>  case SPR_POWER_PMC2:
>  case SPR_POWER_PMC3:
>  case SPR_POWER_PMC4:
> -switch (get_PMC_event(env, sprn)) {
> +event 

Re: [PATCH 15/19] target/ppc/pmu_book3s_helper: enable counter negative for all PMCs

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:53AM -0300, Daniel Henrique Barboza wrote:
> All performance monitor counters can trigger a counter negative
> condition if the proper MMCR0 bits are set. This patch does that by
> doing the following:
> 
> - pmc_counter_negative_enabled() will check whether a given PMC is
> eligible to trigger the counter negative alert;
> 
> - get_counter_neg_timeout() will return the timeout for the counter
> negative condition for a given PMC, or -1 if the PMC is not able to
> trigger this alert;
> 
> - the existing counter_negative_cond_enabled() now must consider the
> counter negative bit for PMCs 2-6, MMCR0_PMCjCE;
> 
> - set_PMU_excp_timer() will now search all existing PMCs for the
> shortest counter negative timeout. The shortest timeout will be used to
> set the PMC interrupt timer.
> 
> This change makes most EBB powepc kernel tests pass, validating that the
> existing EBB logic is consistent. There are a few tests that aren't passing
> due to additional PMU bits and perf events that aren't covered yet.
> We'll attempt to cover some of those in the next patches.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   |  1 +
>  target/ppc/pmu_book3s_helper.c | 96 ++
>  2 files changed, 87 insertions(+), 10 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5c81d459f4..1aa1fd42af 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -351,6 +351,7 @@ typedef struct ppc_v3_pate_t {
>  #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
>  #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
>  #define MMCR0_PMC1CE PPC_BIT(48)
> +#define MMCR0_PMCjCE PPC_BIT(49)
>  
>  #define MMCR1_PMC1SEL_SHIFT (63 - 39)
>  #define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index 7126e9b3d5..c5c5ab38c9 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -143,22 +143,98 @@ static int64_t get_CYC_timeout(CPUPPCState *env, int 
> sprn)
>  return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, PPC_CPU_FREQ);
>  }
>  
> -static void set_PMU_excp_timer(CPUPPCState *env)
> +static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
>  {
> -uint64_t timeout, now;
> +switch (sprn) {
> +case SPR_POWER_PMC1:
> +return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
>  
> -if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
> -return;
> +case SPR_POWER_PMC2:
> +case SPR_POWER_PMC3:
> +case SPR_POWER_PMC4:
> +case SPR_POWER_PMC5:
> +case SPR_POWER_PMC6:
> +return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
> +
> +default:
> +break;
>  }
>  
> -switch (get_PMC_event(env, SPR_POWER_PMC1)) {
> -case 0x2:
> -timeout = get_INST_CMPL_timeout(env, SPR_POWER_PMC1);
> +return false;
> +}
> +
> +static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
> +{
> +int64_t timeout = -1;
> +
> +if (!pmc_counter_negative_enabled(env, sprn)) {
> +return -1;
> +}
> +
> +if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> +return 0;
> +}
> +
> +switch (sprn) {
> +case SPR_POWER_PMC1:
> +case SPR_POWER_PMC2:
> +case SPR_POWER_PMC3:
> +case SPR_POWER_PMC4:
> +switch (get_PMC_event(env, sprn)) {
> +case 0x2:
> +timeout = get_INST_CMPL_timeout(env, sprn);
> +break;
> +case 0x1E:
> +timeout = get_CYC_timeout(env, sprn);
> +break;
> +}
> +
>  break;
> -case 0x1e:
> -timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
> +case SPR_POWER_PMC5:
> +timeout = get_INST_CMPL_timeout(env, sprn);
> +break;
> +case SPR_POWER_PMC6:
> +timeout = get_CYC_timeout(env, sprn);
>  break;
>  default:
> +break;
> +}
> +
> +return timeout;
> +}
> +
> +static void set_PMU_excp_timer(CPUPPCState *env)
> +{
> +int64_t timeout = -1;
> +uint64_t now;
> +int i;
> +
> +/*
> + * Scroll through all PMCs and check which one is closer to a
> + * counter negative timeout.

I'm wondering if it would be simpler to use a separate timer for each
PMC: after all the core timer logic must have already implemented this
"who fires first" logic.

> + */
> +for (i = SPR_POWER_PMC1; i <= SPR_POWER_PMC6; i++) {
> +int64_t curr_timeout = get_counter_neg_timeout(env, i);
> +
> +if (curr_timeout == -1) {
> +continue;
> +}
> +
> +if (curr_timeout == 0) {
> +timeout = 0;
> +break;
> +}
> +
> +if (timeout == -1 || timeout > curr_timeout) {
> +timeout = curr_timeout;
> +}
> +}
> +
> +/*
> + * This can happen if counter negative conditions were enabled
> + * without any events to be sampled.
> + */
> 

Re: [PATCH 05/19] target/ppc/pmu_book3s_helper.c: eliminate code repetition

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:43AM -0300, Daniel Henrique Barboza wrote:
> We don't need a base_icount value in CPUPPCState for each PMC. All the
> calculation done after freeze will use the same base start value. Use a
> single 'pmu_base_icount' attribute that can be use to all PMCs.
> 
> Likewise, the freeze count operations are going to be done for all
> available PMCs, so eliminate both freeze_PMC5_value() and
> freeze_PMC6_value() and use the new update_PMCs_on_freeze() that will
> update all PMCs.
> 
> Signed-off-by: Daniel Henrique Barboza 

Please fold this simplification into the initial patch.

> ---
>  target/ppc/cpu.h   |  8 +---
>  target/ppc/pmu_book3s_helper.c | 33 +
>  2 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 229abfe7ee..8cea8f2aca 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1176,9 +1176,11 @@ struct CPUPPCState {
>  uint64_t tm_dscr;
>  uint64_t tm_tar;
>  
> -/* PMU registers icount state */
> -uint64_t pmc5_base_icount;
> -uint64_t pmc6_base_icount;
> +/*
> + * PMU icount base value used by the PMU to calculate
> + * instructions and cycles.
> + */
> +uint64_t pmu_base_icount;
>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)  \
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index fe16fcfce0..0994531f65 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -28,22 +28,19 @@ static uint64_t get_cycles(uint64_t insns)
>  return insns * 4;
>  }
>  
> -/* PMC5 always count instructions */
> -static void freeze_PMC5_value(CPUPPCState *env)
> -{
> -uint64_t insns = get_insns() - env->pmc5_base_icount;
> -
> -env->spr[SPR_POWER_PMC5] += insns;
> -env->pmc5_base_icount += insns;
> -}
> -
> -/* PMC6 always count cycles */
> -static void freeze_PMC6_value(CPUPPCState *env)
> +/*
> + * Set all PMCs values after a PMU freeze via MMCR0_FC.
> + *
> + * There is no need to update the base icount of each PMC since
> + * the PMU is not running.
> + */
> +static void update_PMCs_on_freeze(CPUPPCState *env)
>  {
> -uint64_t insns = get_insns() - env->pmc6_base_icount;
> +uint64_t curr_icount = get_insns();
>  
> -env->spr[SPR_POWER_PMC6] += get_cycles(insns);
> -env->pmc6_base_icount += insns;
> +env->spr[SPR_POWER_PMC5] += curr_icount - env->pmu_base_icount;
> +env->spr[SPR_POWER_PMC6] += get_cycles(curr_icount -
> +   env->pmu_base_icount);
>  }
>  
>  void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> @@ -64,13 +61,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong 
> value)
>   */
>  if (curr_FC != new_FC) {
>  if (!curr_FC) {
> -freeze_PMC5_value(env);
> -freeze_PMC6_value(env);
> +update_PMCs_on_freeze(env);
>  } else {
> -uint64_t curr_icount = get_insns();
> -
> -env->pmc5_base_icount = curr_icount;
> -env->pmc6_base_icount = curr_icount;
> +env->pmu_base_icount = get_insns();
>  }
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 12/19] target/ppc/pmu_book3s_helper.c: enable PMC1 counter negative EBB

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:50AM -0300, Daniel Henrique Barboza wrote:
> This patch starts the counter negative EBB support by enabling PMC1
> counter negative condition.
> 
> A counter negative condition happens when a performance monitor counter
> reaches the value 0x8000. When that happens, if a counter negative
> condition is enabled in that counter, a performance monitor alert is
> triggered. For PMC1, this condition is enabled by MMCR0_PMC1CE.
> 
> An icount-based logic is used to predict when we need to wake up the timer
> to trigger the alert in both PM_INST_CMPL (0x2) and PM_CYC (0x1E) events.
> The timer callback will then trigger a PPC_INTERRUPT_PMC which will become a
> event-based exception later.
> 
> Some EBB powerpc kernel selftests are passing after this patch, but a
> substancial amount of them relies on other PMCs to be enabled and events
> that we don't support at this moment. We'll address that in the next
> patches.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   |   1 +
>  target/ppc/pmu_book3s_helper.c | 127 +++--
>  2 files changed, 92 insertions(+), 36 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 1d38b8cf7a..5c81d459f4 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -350,6 +350,7 @@ typedef struct ppc_v3_pate_t {
>  #define MMCR0_EBE   PPC_BIT(43) /* Perf Monitor EBB Enable */
>  #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
>  #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> +#define MMCR0_PMC1CE PPC_BIT(48)
>  
>  #define MMCR1_PMC1SEL_SHIFT (63 - 39)
>  #define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index 43cc0eb722..58ae65e22b 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -25,6 +25,7 @@
>   * and SPAPR code.
>   */
>  #define PPC_CPU_FREQ 10
> +#define COUNTER_NEGATIVE_VAL 0x8000
>  
>  static uint64_t get_cycles(uint64_t icount_delta)
>  {
> @@ -32,22 +33,9 @@ static uint64_t get_cycles(uint64_t icount_delta)
>  NANOSECONDS_PER_SECOND);
>  }
>  
> -static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
> -uint64_t icount_delta)
> -{
> -env->spr[sprn] += icount_delta;
> -}
> -
> -static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> -  uint64_t icount_delta)
> -{
> -env->spr[sprn] += get_cycles(icount_delta);
> -}
> -
> -static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> -uint64_t icount_delta)
> +static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
>  {
> -int event;
> +int event = 0x0;
>  
>  switch (sprn) {
>  case SPR_POWER_PMC1:
> @@ -65,11 +53,35 @@ static void update_programmable_PMC_reg(CPUPPCState *env, 
> int sprn,
>  case SPR_POWER_PMC4:
>  event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
>  break;
> +case SPR_POWER_PMC5:
> +event = 0x2;
> +break;
> +case SPR_POWER_PMC6:
> +event = 0x1E;
> +break;

This looks like a nice cleanup that would be better folded into an
earlier patch.

>  default:
> -return;
> +break;
>  }
>  
> -switch (event) {
> +return event;
> +}
> +
> +static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
> +uint64_t icount_delta)
> +{
> +env->spr[sprn] += icount_delta;
> +}
> +
> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> +  uint64_t icount_delta)
> +{
> +env->spr[sprn] += get_cycles(icount_delta);
> +}
> +
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> +uint64_t icount_delta)
> +{
> +switch (get_PMC_event(env, sprn)) {
>  case 0x2:
>  update_PMC_PM_INST_CMPL(env, sprn, icount_delta);
>  break;
> @@ -99,30 +111,57 @@ static void update_PMCs(CPUPPCState *env, uint64_t 
> icount_delta)
>  update_PMC_PM_CYC(env, SPR_POWER_PMC6, icount_delta);
>  }
>  
> +static void set_PMU_excp_timer(CPUPPCState *env)
> +{
> +uint64_t timeout, now, remaining_val;
> +
> +if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
> +return;
> +}
> +
> +remaining_val = COUNTER_NEGATIVE_VAL - env->spr[SPR_POWER_PMC1];
> +
> +switch (get_PMC_event(env, SPR_POWER_PMC1)) {
> +case 0x2:
> +timeout = icount_to_ns(remaining_val);
> +break;
> +case 0x1e:
> +timeout = muldiv64(remaining_val, NANOSECONDS_PER_SECOND,
> +   PPC_CPU_FREQ);

So.. this appears to be simulating to the guest that cycles are
occurring at a constant rate, consistent with the advertised CPU
frequency.  Which sounds right, except... it's not clear to me that
you're using the 

Re: [PATCH 09/19] PPC64/TCG: Implement 'rfebb' instruction

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:47AM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero 
> 
> An Event-Based Branch (EBB) allows applications to change the NIA when a
> event-based exception occurs. Event-based exceptions are enabled by
> setting the Branch Event Status and Control Register (BESCR). If the
> event-based exception is enabled when the exception occurs, an EBB
> happens.
> 
> The EBB will:
> 
> - set the Global Enable (GE) bit of BESCR to 0;
> - set bits 0-61 of the Event-Based Branch Return Register (EBBRR) to the
>   effective address of the NIA that would have executed if the EBB
>   didn't happen;
> - Instruction fetch and execution will continue in the effective address
>   contained in the Event-Based Branch Handler Register (EBBHR).
> 
> The EBB Handler will process the event and then execute the Return From
> Event-Based Branch (rfebb) instruction. rfebb sets BESCR_GE and then
> redirects execution to the address pointed in EBBRR. This process is
> described in the PowerISA v3.1, Book II, Chapter 6 [1].
> 
> This patch implements the rfebb instruction. Descriptions of all
> relevant BESCR bits are also added - this patch is only using BESCR_GE,
> but next patches will use the remaining bits.
> 
> Note that we're implementing the extended rfebb mnemonic (BESCR_GE is
> being always set to 1). The basic rfebb instruction would accept an
> operand that would be used to set GE.
> 
> [1] https://wiki.raptorcs.com/w/images/f/f5/PowerISA_public.v3.1.pdf
> 
> CC: Gustavo Romero 
> Signed-off-by: Gustavo Romero 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   | 12 
>  target/ppc/translate.c | 21 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index afd9cd402b..ae431e65be 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -358,6 +358,18 @@ typedef struct ppc_v3_pate_t {
>  #define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
>  #define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
>  
> +/* EBB/BESCR bits */
> +/* Global Enable */
> +#define BESCR_GE PPC_BIT(0)
> +/* External Event-based Exception Enable */
> +#define BESCR_EE PPC_BIT(30)
> +/* Performance Monitor Event-based Exception Enable */
> +#define BESCR_PME PPC_BIT(31)
> +/* External Event-based Exception Occurred */
> +#define BESCR_EEO PPC_BIT(62)
> +/* Performance Monitor Event-based Exception Occurred */
> +#define BESCR_PMEO PPC_BIT(63)
> +
>  /* LPCR bits */
>  #define LPCR_VPM0 PPC_BIT(0)
>  #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 62356cfadf..afc254a03f 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -2701,6 +2701,26 @@ static void gen_darn(DisasContext *ctx)
>  }
>  }
>  }
> +
> +/* rfebb */
> +static void gen_rfebb(DisasContext *ctx)

Oof.. not necessarily a nack, but it would be nice to implement any
new instructions using the disastree path rather than the old ppc
specific decode logic.

> +{
> +TCGv target = tcg_temp_new();
> +TCGv bescr = tcg_temp_new();
> +
> +gen_load_spr(target, SPR_EBBRR);
> +tcg_gen_mov_tl(cpu_nip, target);
> +
> +gen_load_spr(bescr, SPR_BESCR);
> +tcg_gen_ori_tl(bescr, bescr, BESCR_GE);
> +gen_store_spr(SPR_BESCR, bescr);
> +
> +ctx->base.is_jmp = DISAS_EXIT;
> +
> +tcg_temp_free(target);
> +tcg_temp_free(bescr);
> +}
> +
>  #endif
>  
>  /*** Integer rotate
> ***/
> @@ -7724,6 +7744,7 @@ GEN_HANDLER(popcntd, 0x1F, 0x1A, 0x0F, 0xF801, 
> PPC_POPCNTWD),
>  GEN_HANDLER(cntlzd, 0x1F, 0x1A, 0x01, 0x, PPC_64B),
>  GEN_HANDLER_E(cnttzd, 0x1F, 0x1A, 0x11, 0x, PPC_NONE, PPC2_ISA300),
>  GEN_HANDLER_E(darn, 0x1F, 0x13, 0x17, 0x001CF801, PPC_NONE, PPC2_ISA300),
> +GEN_HANDLER_E(rfebb, 0x13, 0x12, 0x04, 0x03FFF001, PPC_NONE, PPC2_ISA207S),
>  GEN_HANDLER_E(prtyd, 0x1F, 0x1A, 0x05, 0xF801, PPC_NONE, PPC2_ISA205),
>  GEN_HANDLER_E(bpermd, 0x1F, 0x1C, 0x07, 0x0001, PPC_NONE, 
> PPC2_PERM_ISA206),
>  #endif

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 16/19] target/ppc/pmu_book3s_helper: adding 0xFA event

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:54AM -0300, Daniel Henrique Barboza wrote:
> The PowerISA 3.1 defines the 0xFA event as instructions completed when
> the thread's CTRL register is set. Some EBB powerpc kernel tests use
> this event to exercise both the PMU and the EBB support.

Couldn't you implement this more accurately by snapshotting the count
at each CTRL write, and either adding the delta to the PMC or not
depending on the previous CTRL value?

> We don't have a way at this moment to tell whether an instruction was
> completed under those conditions. What we can do is to make it
> equivalent to the existing PM_INST_COMPL event that counts all
> instructions completed. For our current purposes with the PMU support
> this is enough.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/pmu_book3s_helper.c | 14 ++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index c5c5ab38c9..388263688b 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -52,6 +52,20 @@ static uint8_t get_PMC_event(CPUPPCState *env, int sprn)
>  break;
>  case SPR_POWER_PMC4:
>  event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
> +
> +/*
> + * Event 0xFA for PMC4SEL is described as follows in
> + * PowerISA v3.1:
> + *
> + * "The thread has completed an instruction when the RUN bit of
> + * the thread’s CTRL register contained 1"
> + *
> + * Our closest equivalent for this event at this moment is plain
> + * INST_CMPL (event 0x2)
> + */
> +if (event == 0xFA) {
> +event = 0x2;
> +}
>  break;
>  case SPR_POWER_PMC5:
>  event = 0x2;

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 03/19] target/ppc: add exclusive user write function for PMU regs

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:41AM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero 
> 
> Similar to the previous patch, write read on PowerPC PMU regs
> requires extra handling than the generic write ureg functions.
> 
> This patch adds a specific write function for user PMU SPRs,
> spr_write_pmu_ureg(). MMCR0 and PMC1 are being treated before
> write, all other registers will be default to what is done in
> spr_write_ureg(), for now.
> 
> Since spr_write_pmu_ureg() needs to have a look in SPR_POWER_MMCR0
> to validate if the write is valid or not, we're adding a 'spr'
> array in DisasContext that points to env->spr.
> 
> CC: Gustavo Romero 
> Signed-off-by: Gustavo Romero 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu_init.c  | 26 +-
>  target/ppc/spr_tcg.h   |  1 +
>  target/ppc/translate.c | 42 ++
>  3 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index d30aa0fe1e..71062809c8 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,47 +6868,47 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>  {
>  spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC2, "UPMC2",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC3, "UPMC3",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC4, "UPMC4",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC5, "UPMC5",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC6, "UPMC6",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_USIAR, "USIAR",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_USDAR, "USDAR",
> - _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
>   0x);
>  }
> @@ -6976,8 +6976,8 @@ static void register_power8_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_power8_pmu_user_sprs(CPUPPCState *env)
>  {
>  spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> - _read_pmu_ureg, SPR_NOACCESS,
> - _read_pmu_ureg, _write_ureg,
> + _read_pmu_ureg, _write_pmu_ureg,
> + _read_pmu_ureg, _write_pmu_ureg,
>   0x);
>  spr_register(env, SPR_POWER_USIER, "USIER",
>   _read_generic, SPR_NOACCESS,
> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
> index 84ecba220f..40b5de34b9 100644
> --- a/target/ppc/spr_tcg.h
> +++ b/target/ppc/spr_tcg.h
> @@ -28,6 +28,7 @@ void spr_write_generic(DisasContext *ctx, int sprn, int 
> gprn);
>  void spr_read_pmu_generic(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn);
>  void spr_read_pmu_ureg(DisasContext *ctx, int gprn, int sprn);
> +void spr_write_pmu_ureg(DisasContext *ctx, int gprn, int sprn);
>  void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
>  void spr_write_xer(DisasContext 

Re: [PATCH 06/19] target/ppc/pmu_book3s_helper: enable PMC1-PMC4 events

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:44AM -0300, Daniel Henrique Barboza wrote:
> So far the PMU logic was using PMC5 for instruction counting (linux
> kernel PM_INST_CMPL) and PMC6 to count cycles (PM_CYC). We aren't using
> PMCs 1-4.
> 
> Let's enable all PMCs to count these 2 events we already provide. The
> logic used to calculate PMC5 is now being provided by
> update_PMC_PM_INST_CMPL() and PMC6 logic is now implemented in
> update_PMC_PM_CYC().
> 
> The enablement of these 2 events for all PMUs are done by using the
> Linux kernel definition of those events: 0x02 for PM_INST_CMPL and 0x1e
> for PM_CYC,

I'm confused by this.  Surely the specific values here should be
defined by the hardware, not by Linux.

> all of those defined by specific bits in MMCR1 for each PMC.
> PMCs 1-4 relies on the correct event to be defined in MMCR1. PMC5 and
> PMC6 will count PM_INST_CMPL and PMC_CYC, respectively, regardless of
> MMCR1 setup.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   |  8 +
>  target/ppc/pmu_book3s_helper.c | 60 --
>  2 files changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 8cea8f2aca..afd9cd402b 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -350,6 +350,14 @@ typedef struct ppc_v3_pate_t {
>  #define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
>  #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
>  
> +#define MMCR1_PMC1SEL_SHIFT (63 - 39)
> +#define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> +#define MMCR1_PMC2SEL_SHIFT (63 - 47)
> +#define MMCR1_PMC2SEL PPC_BITMASK(40, 47)
> +#define MMCR1_PMC3SEL_SHIFT (63 - 55)
> +#define MMCR1_PMC3SEL PPC_BITMASK(48, 55)
> +#define MMCR1_PMC4SEL PPC_BITMASK(56, 63)
> +
>  /* LPCR bits */
>  #define LPCR_VPM0 PPC_BIT(0)
>  #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index 0994531f65..99e62bd37b 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -28,6 +28,56 @@ static uint64_t get_cycles(uint64_t insns)
>  return insns * 4;
>  }
>  
> +static void update_PMC_PM_INST_CMPL(CPUPPCState *env, int sprn,
> +uint64_t curr_icount)
> +{
> +env->spr[sprn] += curr_icount - env->pmu_base_icount;
> +}
> +
> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
> +  uint64_t curr_icount)
> +{
> +uint64_t insns = curr_icount - env->pmu_base_icount;
> +env->spr[sprn] += get_cycles(insns);
> +}
> +
> +static void update_programmable_PMC_reg(CPUPPCState *env, int sprn,
> +uint64_t curr_icount)
> +{
> +int event;
> +
> +switch (sprn) {
> +case SPR_POWER_PMC1:
> +event = MMCR1_PMC1SEL & env->spr[SPR_POWER_MMCR1];
> +event = event >> MMCR1_PMC1SEL_SHIFT;
> +break;
> +case SPR_POWER_PMC2:
> +event = MMCR1_PMC2SEL & env->spr[SPR_POWER_MMCR1];
> +event = event >> MMCR1_PMC2SEL_SHIFT;
> +break;
> +case SPR_POWER_PMC3:
> +event = MMCR1_PMC3SEL & env->spr[SPR_POWER_MMCR1];
> +event = event >> MMCR1_PMC3SEL_SHIFT;
> +break;
> +case SPR_POWER_PMC4:
> +event = MMCR1_PMC4SEL & env->spr[SPR_POWER_MMCR1];
> +break;
> +default:
> +return;
> +}
> +
> +switch (event) {
> +case 0x2:
> +update_PMC_PM_INST_CMPL(env, sprn, curr_icount);
> +break;
> +case 0x1E:
> +update_PMC_PM_CYC(env, sprn, curr_icount);
> +break;
> +default:
> +return;
> +}
> +}
> +
>  /*
>   * Set all PMCs values after a PMU freeze via MMCR0_FC.
>   *
> @@ -37,10 +87,14 @@ static uint64_t get_cycles(uint64_t insns)
>  static void update_PMCs_on_freeze(CPUPPCState *env)
>  {
>  uint64_t curr_icount = get_insns();
> +int sprn;
> +
> +for (sprn = SPR_POWER_PMC1; sprn < SPR_POWER_PMC5; sprn++) {
> +update_programmable_PMC_reg(env, sprn, curr_icount);
> +}
>  
> -env->spr[SPR_POWER_PMC5] += curr_icount - env->pmu_base_icount;
> -env->spr[SPR_POWER_PMC6] += get_cycles(curr_icount -
> -   env->pmu_base_icount);
> +update_PMC_PM_INST_CMPL(env, SPR_POWER_PMC5, curr_icount);
> +update_PMC_PM_CYC(env, SPR_POWER_PMC6, curr_icount);
>  }
>  
>  void helper_store_mmcr0(CPUPPCState *env, target_ulong value)

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for-6.2 v6 6/7] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors

[David Gibson]

On Mon, Aug 09, 2021 at 03:47:14PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/7/21 11:06 AM, Markus Armbruster wrote:
> > Daniel Henrique Barboza  writes:
> > 
> > > Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
> > > error path, signalling that the hotunplug process wasn't successful.
> > > This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
> > > to signal this error to the management layer.
> > > 
> > > We also have another error path in spapr_memory_unplug_rollback() for
> > > configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
> > > in the hotunplug error path, but it will reconfigure them. Let's send
> > > the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
> > > case of older kernels.
> > > 
> > > Reviewed-by: Greg Kurz 
> > > Signed-off-by: Daniel Henrique Barboza 
> > > ---
> > >   hw/ppc/spapr.c |  9 -
> > >   hw/ppc/spapr_drc.c | 18 --
> > >   2 files changed, 20 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 1611d7ab05..5459f9a7e9 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -29,6 +29,7 @@
> > >   #include "qemu/datadir.h"
> > >   #include "qapi/error.h"
> > >   #include "qapi/qapi-events-machine.h"
> > > +#include "qapi/qapi-events-qdev.h"
> > >   #include "qapi/visitor.h"
> > >   #include "sysemu/sysemu.h"
> > >   #include "sysemu/hostmem.h"
> > > @@ -3686,13 +3687,19 @@ void 
> > > spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
> > >   /*
> > >* Tell QAPI that something happened and the memory
> > > - * hotunplug wasn't successful.
> > > + * hotunplug wasn't successful. Keep sending
> > > + * MEM_UNPLUG_ERROR even while sending DEVICE_UNPLUG_ERROR
> > > + * until the deprecation MEM_UNPLUG_ERROR is due.
> > >*/
> > >   if (dev->id) {
> > >   qapi_error = g_strdup_printf("Memory hotunplug rejected by the 
> > > guest "
> > >"for device %s", dev->id);
> > >   qapi_event_send_mem_unplug_error(dev->id, qapi_error);
> > >   }
> > > +
> > > +qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> > > +dev->canonical_path,
> > > +qapi_error != NULL, qapi_error);
> > >   }
> > 
> > When dev->id is null, we send something like
> > 
> >  {"event": "DEVICE_UNPLUG_ERROR",
> >   "data": {"path": "/machine/..."},
> >   "timestamp": ...}
> > 
> > Unless I'm missing something, this is all the information the management
> > application really needs.
> > 
> > When dev->id is non-null, we add to "data":
> > 
> >"device": "dev123",
> >"msg": "Memory hotunplug rejected by the guest for device 
> > dev123",
> > 
> > I'm fine with emitting the device ID when we have it.
> > 
> > What's the intended use of "msg"?
> > 
> > Could DEVICE_UNPLUG_ERROR ever be emitted for this device with a
> > different "msg"?
> 
> 
> It won't have a different 'msg' for the current use of the event in both ppc64
> and x86. It'll always be the same ' hotunplug rejected by the guest'
> message.
> 
> The idea is that a future caller might want to insert a more informative
> message, such as "hotunplug failed: memory is being used by kernel space"
> or any other more specific condition. But then I guess we can argue that,
> if that time comes, one can just add this new optional 'msg' member in this
> event, and for now we can live without it.

Right.  We could also consider making the current message more
specific about why we chose to cancel the unplug: e.g. "guest
unisolated DRC after unplug request" for PAPR, and something
appropriate to the ACPI specifics for x86.  Not sure if that's useful
enough to justify it.

> Would you oppose to renaming this new event to "DEVICE_UNPLUG_GUEST_ERROR"
> and then remove the 'msg' member? I guess this rename would make it clearer
> for management that we're reporting a guest side error, making any further
> clarifications via 'msg' unneeded.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
> > 
> > If "msg" is useful when dev->id is non-null, then it's likely useful
> > when dev->id is null.  Why not
> > 
> >"msg": "Memory hotunplug rejected by the guest",
> > 
> > always?
> > 
> > If we do that here, we'll likely do it everywhere, and then member @msg
> > isn't actually optional.
> > 
> > >   /* Callback to be called during DRC release. */
> > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > > index a4d9496f76..8f0479631f 100644
> > > --- a/hw/ppc/spapr_drc.c
> > > +++ b/hw/ppc/spapr_drc.c
> > > @@ -17,6 +17,8 @@
> > >   #include "hw/ppc/spapr_drc.h"
> > >   #include "qom/object.h"
> > >   #include "migration/vmstate.h"
> > > +#include "qapi/error.h"
> > > +#include "qapi/qapi-events-qdev.h"
> > >   #include "qapi/visitor.h"
> > >   

Re: [PATCH 10/19] target/ppc: PMU Event-Based exception support

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:48AM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero 
> 
> Following up the rfebb implementation, this patch adds the EBB exception
> support that are triggered by Performance Monitor alerts. This exception
> occurs when an enabled PMU condition or event happens and both MMCR0_EBE
> and BESCR_PME are set.
> 
> The supported PM alerts will consist of counter negative conditions of
> the PMU counters. This will be achieved by a timer mechanism that will
> predict when a counter becomes negative. The PMU timer callback will set
> the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
> exception code will then set the appropriate BESCR bits, set the next
> instruction pointer to the address pointed by the return register
> (SPR_EBBRR), and redirect execution to the handler (pointed by
> SPR_EBBHR).
> 
> This patch sets the basic structure of interrupts and the timer. The
> following patches will add the counter negative logic for the registers.
> The goal is to use the EBB selftests of the powerpc kernel to validate
> the EBB implementation, thus we'll add more PMU bits as we go along.
> 
> CC: Gustavo Romero 
> Signed-off-by: Gustavo Romero 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  hw/ppc/spapr_cpu_core.c|  6 ++
>  target/ppc/cpu.h   |  9 +++-
>  target/ppc/excp_helper.c   | 28 +
>  target/ppc/pmu_book3s_helper.c | 38 ++
>  4 files changed, 80 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 4f316a6f9d..41b443bde2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,7 @@
>  #include "target/ppc/kvm_ppc.h"
>  #include "hw/ppc/ppc.h"
>  #include "target/ppc/mmu-hash64.h"
> +#include "target/ppc/pmu_book3s_helper.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/hw_accel.h"
> @@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>  return false;
>  }
>  
> +/* Init PMU interrupt timer (TCG only) */
> +if (!kvm_enabled()) {

I'd prefer this check to be inside the called function.

> +cpu_ppc_pmu_timer_init(env);
> +}
> +
>  if (!sc->pre_3_0_migration) {
>  vmstate_register(NULL, cs->cpu_index, _spapr_cpu_state,
>   cpu->machine_data);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index ae431e65be..1d38b8cf7a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -129,8 +129,9 @@ enum {
>  /* ISA 3.00 additions */
>  POWERPC_EXCP_HVIRT= 101,
>  POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception
>  */
> +POWERPC_EXCP_EBB = 103, /* Event-based branch exception  
> */
>  /* EOL   
> */
> -POWERPC_EXCP_NB   = 103,
> +POWERPC_EXCP_NB   = 104,
>  /* QEMU exceptions: special cases we want to stop translation
> */
>  POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only  
> */
>  };
> @@ -1201,6 +1202,11 @@ struct CPUPPCState {
>   * instructions and cycles.
>   */
>  uint64_t pmu_base_icount;
> +
> +/*
> + * Timer used to fire performance monitor alerts and interrupts.
> + */
> +QEMUTimer *pmu_intr_timer;
>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)  \
> @@ -2417,6 +2423,7 @@ enum {
>  PPC_INTERRUPT_HMI,/* Hypervisor Maintenance interrupt*/
>  PPC_INTERRUPT_HDOORBELL,  /* Hypervisor Doorbell interrupt*/
>  PPC_INTERRUPT_HVIRT,  /* Hypervisor virtualization interrupt  */
> +PPC_INTERRUPT_PMC,/* Performance Monitor Counter interrupt */
>  };
>  
>  /* Processor Compatibility mask (PCR) */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index a79a0ed465..b866209b6e 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -821,6 +821,22 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>  cpu_abort(cs, "Non maskable external exception "
>"is not implemented yet !\n");
>  break;
> +case POWERPC_EXCP_EBB:   /* Event-based branch exception 
> */
> +if ((env->spr[SPR_BESCR] & BESCR_GE) &&
> +(env->spr[SPR_BESCR] & BESCR_PME)) {
> +target_ulong nip;
> +
> +env->spr[SPR_BESCR] &= ~BESCR_GE;   /* Clear GE */
> +env->spr[SPR_BESCR] |= BESCR_PMEO;  /* Set PMEO */
> +env->spr[SPR_EBBRR] = env->nip; /* Save NIP for rfebb insn */
> +nip = env->spr[SPR_EBBHR];  /* EBB handler */
> +powerpc_set_excp_state(cpu, nip, env->msr);
> +}
> +/*
> + * This interrupt is handled by userspace. No need
> + * to proceed.

Re: [PATCH 04/19] target/ppc: PMU Book3s basic insns count for pseries TCG

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:42AM -0300, Daniel Henrique Barboza wrote:
> The PMCC (PMC Control) bit in the MMCR0 register controls whether the
> counters PMC5 and PMC6 are being part of the performance monitor
> facility in a specific time. If PMCC allows it, PMC5 and PMC6 will
> always be used to measure instructions completed and cycles,
> respectively.
> 
> This patch adds the barebones of the Book3s PMU logic by enabling
> instruction counting, using the icount framework, using the performance
> monitor counters 5 and 6. The overall logic goes as follows:
> 
> - a helper is added to control the PMU state on each MMCR0 write. This
> allows for the PMU to start/stop as quick as possible;

Um.. why does a helper accomplish that goal?

> 
> - only PMC5 and PMC6 are being set. PMC6 (cycles) is default to 4*insns
> (for cycles per instruction) for now;

What's the justification for that value?  With a superscalar core, I'd
expect average (median) cycles per instruction to be < 1 a lot of the
time.  Mean cycles per instruction could be higher since certain
instructions could take a lot.

> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting via MMCR1 (not implemented yet) and setting
> initial counter values,  and enable the PMU by zeroing MMCR0_FC. Software
> must freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.

Is that the way hardware behaves?  Or is it just a limitation of this
software implementation?  Either is fine, we should just be clear on
what it is.

> 
> Since there will be more PMU exclusive code to be added next, let's also
> put the PMU logic in its own helper to keep all in the same place. The
> code is also repetitive and not really extensible to add more PMCs, but
> we'll handle this in the next patches.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   |  4 ++
>  target/ppc/cpu_init.c  |  4 +-
>  target/ppc/helper.h|  1 +
>  target/ppc/meson.build |  1 +
>  target/ppc/pmu_book3s_helper.c | 78 ++
>  target/ppc/translate.c | 14 --
>  6 files changed, 97 insertions(+), 5 deletions(-)
>  create mode 100644 target/ppc/pmu_book3s_helper.c
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4d96015f81..229abfe7ee 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1175,6 +1175,10 @@ struct CPUPPCState {
>  uint32_t tm_vscr;
>  uint64_t tm_dscr;
>  uint64_t tm_tar;
> +
> +/* PMU registers icount state */
> +uint64_t pmc5_base_icount;
> +uint64_t pmc6_base_icount;
>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)  \
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 71062809c8..fce89ee994 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6822,7 +6822,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState 
> *env)
>  spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
>   SPR_NOACCESS, SPR_NOACCESS,
>   _read_pmu_generic, _write_pmu_generic,
> - KVM_REG_PPC_MMCR0, 0x);
> + KVM_REG_PPC_MMCR0, 0x8000);
>  spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
>   SPR_NOACCESS, SPR_NOACCESS,
>   _read_pmu_generic, _write_pmu_generic,
> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState 
> *env)
>  spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>   _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
> - 0x);
> + 0x8000);
>  spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>   _read_pmu_ureg, _write_pmu_ureg,
>   _read_ureg, _write_ureg,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 4076aa281e..5122632784 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
>  DEF_HELPER_1(hrfid, void, env)
>  DEF_HELPER_2(store_lpcr, void, env, tl)
>  DEF_HELPER_2(store_pcr, void, env, tl)
> +DEF_HELPER_2(store_mmcr0, void, env, tl)
>  #endif
>  DEF_HELPER_1(check_tlb_flush_local, void, env)
>  DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index b85f295703..bf252ca3ac 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
>'int_helper.c',
>'mem_helper.c',
>'misc_helper.c',
> +  'pmu_book3s_helper.c',
>'timebase_helper.c',
>'translate.c',
>  ))
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> new file mode 100644
> index 00..fe16fcfce0
> --- /dev/null
> +++ b/target/ppc/pmu_book3s_helper.c

I'd prefer to call this just book3s_pmu.c.  Or better yet

Re: [PATCH 01/19] target/ppc: add exclusive Book3S PMU reg read/write functions

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:39AM -0300, Daniel Henrique Barboza wrote:
> The PowerPC PMU, as described by PowerISA v3.1, has a lot of functions
> that freezes, resets and sets counters to specific values depending on
> the circuntances. Some of these are trigged based on read/value of the
> PMU registers (MMCR0, MMCR1, MMCR2, MMCRA and PMC counters).
> 
> Having to handle the PMU logic using the generic read/write functions
> can impact all other registers that has nothing to do with the PMU that
> uses these functions. This patch creates two new functions,
> spr_read_pmu_generic() and spr_write_pmu_generic, that will be used later
> on to handle PMU logic together with the read/write of PMU registers.
> 
> We're not ready to add specific PMU logic in these new functions yet, so
> for now these are just stubs that calls spr_read/write_generic(). No
> functional change is made.
> 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu_init.c  | 24 
>  target/ppc/spr_tcg.h   |  2 ++
>  target/ppc/translate.c | 12 
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 505a0ed6ac..021c1bc750 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6821,47 +6821,47 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState 
> *env)
>  {
>  spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,

So... this seems dubiousd to me.  Surely when you go to implement the
specifics of these registers you'll need separate logic for each of
them.

Why call a common "read_pmu" function that will then have to multiplex
to different logic for each register, when you could just dispatch
directly to a helper for that specific register.

>   KVM_REG_PPC_MMCR0, 0x);
>  spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_MMCR1, 0x);
>  spr_register_kvm(env, SPR_POWER_MMCRA, "MMCRA",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_MMCRA, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC1, "PMC1",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_PMC1, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC2, "PMC2",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_PMC2, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC3, "PMC3",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_PMC3, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC4, "PMC4",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_PMC4, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC5, "PMC5",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, spr_write_pmu_generic,
>   KVM_REG_PPC_PMC5, 0x);
>  spr_register_kvm(env, SPR_POWER_PMC6, "PMC6",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_PMC6, 0x);
>  spr_register_kvm(env, SPR_POWER_SIAR, "SIAR",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_SIAR, 0x);
>  spr_register_kvm(env, SPR_POWER_SDAR, "SDAR",
>   SPR_NOACCESS, SPR_NOACCESS,
> - _read_generic, _write_generic,
> + _read_pmu_generic, _write_pmu_generic,
>   KVM_REG_PPC_SDAR, 0x);
>  }
>  
> @@ -6941,7 +6941,7 @@ static void register_power8_pmu_sup_sprs(CPUPPCState 
> *env)
>  {
>  spr_register_kvm(env, SPR_POWER_MMCR2, "MMCR2",
>   SPR_NOACCESS, SPR_NOACCESS,
> 

Re: [PATCH 00/26] ppc/pnv: Extend the powernv10 machine

[David Gibson]

On Mon, Aug 09, 2021 at 03:45:21PM +0200, Cédric Le Goater wrote:
> Hi,
> 
> This series adds the minimum set of models (XIVE2, PHB5) to boot a
> baremetal POWER10 machine using the OpenPOWER firmware images.
> 
> The major change is the support for the interrupt controller of the
> POWER10 processor. XIVE2 is very much like XIVE but the register
> interface, the different MMIO regions, the XIVE internal descriptors
> have gone through a major cleanup. It was easier to duplicate the
> models then to try to adapt the current models. XIVE2 adds some new
> set of features. Not all are modeled here but we add the
> "Address-based trigger" mode which is activated by default on the
> PHB5. When using ABT, the PHB5 offloads all interrupt management on
> the IC, this to improve latency.

5..8/26 applied to ppc-for-6.2, continuing to look at the rest.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 00/26] ppc/pnv: Extend the powernv10 machine

[David Gibson]

On Mon, Aug 09, 2021 at 03:45:21PM +0200, Cédric Le Goater wrote:
> Hi,
> 
> This series adds the minimum set of models (XIVE2, PHB5) to boot a
> baremetal POWER10 machine using the OpenPOWER firmware images.
> 
> The major change is the support for the interrupt controller of the
> POWER10 processor. XIVE2 is very much like XIVE but the register
> interface, the different MMIO regions, the XIVE internal descriptors
> have gone through a major cleanup. It was easier to duplicate the
> models then to try to adapt the current models. XIVE2 adds some new
> set of features. Not all are modeled here but we add the
> "Address-based trigger" mode which is activated by default on the
> PHB5. When using ABT, the PHB5 offloads all interrupt management on
> the IC, this to improve latency.

1..4/26 applied to ppc-for-6.2, continuing to look at the rest.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH 02/19] target/ppc: add exclusive user read function for PMU regs

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:40AM -0300, Daniel Henrique Barboza wrote:
> From: Gustavo Romero 
> 
> User read on PowerPC PMU regs requires extra handling in some
> instances. Instead of changing the existing read ureg function
> (spr_read_ureg) this patch adds a specific read function for
> user PMU SPRs, spr_read_pmu_ureg().
> 
> This function does extra handling of UMMCR0 and UMMCR2 and falls
> back to the default behavior for the not yet handled regs. Aside
> for UMMCR0 and UMMCR2 reads, no functional change is made.
> 
> CC: Gustavo Romero 
> Signed-off-by: Gustavo Romero 
> Signed-off-by: Daniel Henrique Barboza 
> ---
>  target/ppc/cpu.h   |  8 
>  target/ppc/cpu_init.c  | 26 +-
>  target/ppc/spr_tcg.h   |  1 +
>  target/ppc/translate.c | 41 +++--
>  4 files changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 500205229c..4d96015f81 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -342,6 +342,14 @@ typedef struct ppc_v3_pate_t {
>  #define MSR_RI   1  /* Recoverable interrupt1
> */
>  #define MSR_LE   0  /* Little-endian mode   1 hflags 
> */
>  
> +/* PMU bits */
> +#define MMCR0_FCPPC_BIT(32) /* Freeze Counters  */
> +#define MMCR0_PMAO  PPC_BIT(56) /* Perf Monitor Alert Ocurred */
> +#define MMCR0_PMAE  PPC_BIT(37) /* Perf Monitor Alert Enable */
> +#define MMCR0_EBE   PPC_BIT(43) /* Perf Monitor EBB Enable */
> +#define MMCR0_FCECE PPC_BIT(38) /* FC on Enabled Cond or Event */
> +#define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> +
>  /* LPCR bits */
>  #define LPCR_VPM0 PPC_BIT(0)
>  #define LPCR_VPM1 PPC_BIT(1)
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 021c1bc750..d30aa0fe1e 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6868,47 +6868,47 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>  {
>  spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UMMCRA, "UMMCRA",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC1, "UPMC1",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC2, "UPMC2",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC3, "UPMC3",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC4, "UPMC4",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC5, "UPMC5",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_UPMC6, "UPMC6",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_USIAR, "USIAR",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  spr_register(env, SPR_POWER_USDAR, "USDAR",
> - _read_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, SPR_NOACCESS,
>   _read_ureg, _write_ureg,
>   0x);
>  }
> @@ -6976,8 +6976,8 @@ static void register_power8_pmu_sup_sprs(CPUPPCState 
> *env)
>  static void register_power8_pmu_user_sprs(CPUPPCState *env)
>  {
>  spr_register(env, SPR_POWER_UMMCR2, "UMMCR2",
> - _read_ureg, SPR_NOACCESS,
> - _read_ureg, _write_ureg,
> + _read_pmu_ureg, SPR_NOACCESS,
> + _read_pmu_ureg, 

[PATCH] fsl-imx6ul: add SAI1/2/3 and ASRC as unimplemented devices

[Guenter Roeck]

Define SAI1/2/3 and ASRC as unimplemented devices to avoid random
Linux kernel crashes.

Signed-off-by: Guenter Roeck 
---
 hw/arm/fsl-imx6ul.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index e0128d7316..48b60eb3ce 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -534,6 +534,13 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
**errp)
  */
 create_unimplemented_device("sdma", FSL_IMX6UL_SDMA_ADDR, 0x4000);
 
+/*
+ * SAI
+ */
+create_unimplemented_device("sai1", FSL_IMX6UL_SAI1_ADDR, 0x4000);
+create_unimplemented_device("sai2", FSL_IMX6UL_SAI2_ADDR, 0x4000);
+create_unimplemented_device("sai3", FSL_IMX6UL_SAI3_ADDR, 0x4000);
+
 /*
  * PWM
  */
@@ -542,6 +549,11 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error 
**errp)
 create_unimplemented_device("pwm3", FSL_IMX6UL_PWM3_ADDR, 0x4000);
 create_unimplemented_device("pwm4", FSL_IMX6UL_PWM4_ADDR, 0x4000);
 
+/*
+ * ASRC
+ */
+create_unimplemented_device("asrc", FSL_IMX6UL_ASRC_ADDR, 0x4000);
+
 /*
  * CAN
  */
-- 
2.25.1

[PATCH] hw: arm: aspeed: Enable mac0/1 instead of mac1/2 for g220a

[Guenter Roeck]

According to its dts file in the Linux kernel, we need mac0 and mac1 enabled
instead of mac1 and mac2. Also, g220a is based on aspeed-g5 (ast2500) which
doesn't even have the third interface.

Signed-off-by: Guenter Roeck 
---
 hw/arm/aspeed.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ecf0c9cfac..20e3a77160 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -997,7 +997,7 @@ static void aspeed_machine_g220a_class_init(ObjectClass 
*oc, void *data)
 amc->fmc_model = "n25q512a";
 amc->spi_model = "mx25l25635e";
 amc->num_cs= 2;
-amc->macs_mask  = ASPEED_MAC1_ON | ASPEED_MAC2_ON;
+amc->macs_mask  = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
 amc->i2c_init  = g220a_bmc_i2c_init;
 mc->default_ram_size = 1024 * MiB;
 mc->default_cpus = mc->min_cpus = mc->max_cpus =
-- 
2.25.1

[PATCH] target/riscv: Don't wrongly overide isa version

[LIU Zhiwei]

For some cpu, the isa version has already been set in cpu init function.
Thus only overide the isa version when isa version is not set, or
users set different isa version explicitly by cpu parameters.

Signed-off-by: LIU Zhiwei 
---
 target/riscv/cpu.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 991a6bb760..425efba0c8 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -392,9 +392,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 RISCVCPU *cpu = RISCV_CPU(dev);
 CPURISCVState *env = >env;
 RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(dev);
-int priv_version = PRIV_VERSION_1_11_0;
-int bext_version = BEXT_VERSION_0_93_0;
-int vext_version = VEXT_VERSION_0_07_1;
+int priv_version = env->priv_ver;
 target_ulong target_misa = env->misa;
 Error *local_err = NULL;
 
@@ -417,9 +415,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 }
 }
 
-set_priv_version(env, priv_version);
-set_bext_version(env, bext_version);
-set_vext_version(env, vext_version);
+if (!env->priv_ver) {
+set_priv_version(env, PRIV_VERSION_1_11_0);
+} else if (env->priv_ver != priv_version) {
+set_priv_version(env, priv_version);
+}
 
 if (cpu->cfg.mmu) {
 set_feature(env, RISCV_FEATURE_MMU);
@@ -497,6 +497,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 target_misa |= RVH;
 }
 if (cpu->cfg.ext_b) {
+int bext_version = BEXT_VERSION_0_93_0;
 target_misa |= RVB;
 
 if (cpu->cfg.bext_spec) {
@@ -515,6 +516,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error 
**errp)
 set_bext_version(env, bext_version);
 }
 if (cpu->cfg.ext_v) {
+int vext_version = VEXT_VERSION_0_07_1;
 target_misa |= RVV;
 if (!is_power_of_2(cpu->cfg.vlen)) {
 error_setg(errp,
-- 
2.17.1

Re: [PATCH for 6.2 26/49] bsd-user: Create target specific vmparam.h

[Warner Losh]

On Mon, Aug 9, 2021 at 2:39 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/7/21 11:42 AM, Warner Losh wrote:
> > +#define TARGET_MAXTSIZ  (128UL*1024*1024)   /* max text size */
> > +#define TARGET_DFLDSIZ  (128UL*1024*1024)   /* initial data size limit
> */
> > +#define TARGET_MAXDSIZ  (512UL*1024*1024)   /* max data size */
> > +#define TARGET_DFLSSIZ  (8UL*1024*1024) /* initial stack size limit
> */
> > +#define TARGET_MAXSSIZ  (64UL*1024*1024)/* max stack size */
> > +#define TARGET_SGROWSIZ (128UL*1024)/* amount to grow stack */
>
> To-do list: KiB and MiB from units.h.
>

Easy enough to do now and merge to our main branch


> > +++ b/bsd-user/qemu.h
> > @@ -44,7 +44,7 @@ extern enum BSDType bsd_type;
> >   #include "target_arch.h"
> >   #include "syscall_defs.h"
> >   #include "target_syscall.h"
> > -//#include "target_os_vmparam.h"
> > +#include "target_os_vmparam.h"
> >   //#include "target_os_signal.h"
> >   //#include "hostdep.h"
>
> Ah, I see.  Well, perhaps just squash the addition of the include to the
> patch that
> introduces the include?
>

Good idea.

Reviewed-by: Richard Henderson 
>

thanks!

Warner

Re: [PATCH for 6.2 25/49] bsd-user: define max args in terms of pages

[Warner Losh]

On Mon, Aug 9, 2021 at 2:33 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/7/21 11:42 AM, Warner Losh wrote:
> > From: Warner Losh 
> >
> > For 32-bit platforms, pass in up to 256k of args. For 64-bit, bump that
> > to 512k.
> >
> > Signed-off-by: Kyle Evans 
> > Signed-off-by: Warner Losh 
> > ---
> >   bsd-user/qemu.h | 14 ++
> >   1 file changed, 10 insertions(+), 4 deletions(-)
>
> Reviewed-by: Richard Henderson 
>
> > +#define TARGET_ARG_MAX (512 * 1024)
> > +#else
> > +#define TARGET_ARG_MAX (256 * 1024)
>
> For the to-do list: qemu/units.h has KiB for clarity.
>

OK. Will change. Thanks!

Warner

Re: [PATCH for 6.2 24/49] bsd-user: Include more things in qemu.h

[Warner Losh]

On Mon, Aug 9, 2021 at 2:31 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 8/7/21 11:42 AM, Warner Losh wrote:
> > From: Warner Losh 
> >
> > Include more header files to match bsd-user fork.
> >
> > Signed-off-by: Warner Losh 
> > ---
> >   bsd-user/qemu.h | 9 +
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
> > index 6c4ec61d76..02e6e8327a 100644
> > --- a/bsd-user/qemu.h
> > +++ b/bsd-user/qemu.h
> > @@ -18,11 +18,15 @@
> >   #define QEMU_H
> >
> >
> > +#include "qemu/osdep.h"
> >   #include "cpu.h"
> >   #include "exec/cpu_ldst.h"
> > +#include "exec/exec-all.h"
> > +//#include "trace/trace-bsd_user.h"
> >
> >   #undef DEBUG_REMAP
> >   #ifdef DEBUG_REMAP
> > +#include 
> >   #endif /* DEBUG_REMAP */
>
> osdep.h will have included stdio.h.
>
> > +//#include "target_os_vmparam.h"
> > +//#include "target_os_signal.h"
> > +//#include "hostdep.h"
>
> Delete these?
>

Done. I'll post in v2.  Thanks!

Warner

[PULL for-6.1 1/1] qga: fix leak of base64 decoded data on command error

[Michael Roth]

From: Daniel P. Berrangé 

If the guest command fails to be spawned, then we would leak the decoded
base64 input used for the command's stdin feed.

Signed-off-by: Daniel P. Berrangé 
Reviewed-by: Marc-André Lureau 
Signed-off-by: Michael Roth 
---
 qga/commands.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/commands.c b/qga/commands.c
index a6491d2cf8..80501e4a73 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -402,7 +402,7 @@ GuestExec *qmp_guest_exec(const char *path,
 GIOChannel *in_ch, *out_ch, *err_ch;
 GSpawnFlags flags;
 bool has_output = (has_capture_output && capture_output);
-uint8_t *input = NULL;
+g_autofree uint8_t *input = NULL;
 size_t ninput = 0;
 
 arglist.value = (char *)path;
@@ -441,7 +441,7 @@ GuestExec *qmp_guest_exec(const char *path,
 g_child_watch_add(pid, guest_exec_child_watch, gei);
 
 if (has_input_data) {
-gei->in.data = input;
+gei->in.data = g_steal_pointer();
 gei->in.size = ninput;
 #ifdef G_OS_WIN32
 in_ch = g_io_channel_win32_new_fd(in_fd);
-- 
2.25.1

[PATCH v2 1/1] target/riscv: Add User CSRs read-only check

[LIU Zhiwei]

For U-mode CSRs, read-only check is also needed.

Signed-off-by: LIU Zhiwei 
Reviewed-by: Bin Meng 
---
 target/riscv/csr.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 9a4ed18ac5..5499cae94a 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1422,11 +1422,11 @@ RISCVException riscv_csrrw(CPURISCVState *env, int 
csrno,
 RISCVException ret;
 target_ulong old_value;
 RISCVCPU *cpu = env_archcpu(env);
+int read_only = get_field(csrno, 0xC00) == 3;
 
 /* check privileges and return -1 if check fails */
 #if !defined(CONFIG_USER_ONLY)
 int effective_priv = env->priv;
-int read_only = get_field(csrno, 0xC00) == 3;
 
 if (riscv_has_ext(env, RVH) &&
 env->priv == PRV_S &&
@@ -1439,11 +1439,13 @@ RISCVException riscv_csrrw(CPURISCVState *env, int 
csrno,
 effective_priv++;
 }
 
-if ((write_mask && read_only) ||
-(!env->debugger && (effective_priv < get_field(csrno, 0x300 {
+if (!env->debugger && (effective_priv < get_field(csrno, 0x300))) {
 return RISCV_EXCP_ILLEGAL_INST;
 }
 #endif
+if (write_mask && read_only) {
+return RISCV_EXCP_ILLEGAL_INST;
+}
 
 /* ensure the CSR extension is enabled. */
 if (!cpu->cfg.ext_icsr) {
-- 
2.17.1

[PULL for-6.1 0/1] qemu-ga patch queue for hard-freeze/rc3

[Michael Roth]

Hi Peter,

This is a single fix for a potentially recurring memory leak in the guest
agent. If you don't think it is rc3 material I can save it for 6.2, but if
possible this would be good to have in.

The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d:

  Merge remote-tracking branch 
'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging (2021-08-06 
10:28:33 +0100)

are available in the Git repository at:

  git://github.com/mdroth/qemu.git tags/qga-pull-2021-08-09-tag

for you to fetch changes up to 057489dd1586612b99b4b98d211bf7f0a9d6f0e4:

  qga: fix leak of base64 decoded data on command error (2021-08-09 20:18:43 
-0500)


qemu-ga patch queue for hard-freeze

* fix memory leak in guest_exec


Daniel P. Berrangé (1):
  qga: fix leak of base64 decoded data on command error

 qga/commands.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.

[David Gibson]

On Mon, Aug 09, 2021 at 10:14:05AM +0200, Cédric Le Goater wrote:
> On 8/9/21 10:06 AM, Philippe Mathieu-Daudé wrote:
> > On 8/9/21 9:55 AM, Cédric Le Goater wrote:
> >> Hello Phil,
> >>
> >> On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote:
> >>> Hi Cédric,
> >>>
> >>> On 8/6/21 8:00 PM, Cédric Le Goater wrote:
>  It includes support for the POWER10 processor and the QEMU platform.
> >>>
> >>> 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here?
> >>
> >> OK. See attachement.
> > 
> > By "here" I meant in the commit description ;)
> 
> yeah I know but David queued the patch already.

I can replace it with a new version.  Including the shortlog is
probably worth it.

> 
> > 
> 
>  Built from submodule.
> >>>
> >>> 2/ Could we have a CI job building this, during 6.2 cycle?
> >>>(See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml)
> >>
> >> Sure. It doesn't look too complex. 
> >>
> >> I plan to add acceptance tests for the QEMU powernv machines also 
> >> once the OpenPOWER files (zImage.epapr and rootfs.cpio.xz) are 
> >> published on GH.
> >>  
> 
>  Signed-off-by: Cédric Le Goater 
>  ---
>   pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes
>   roms/skiboot|   2 +-
>   2 files changed, 1 insertion(+), 1 deletion(-)
> 
>  diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid
>  index 
>  504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0
>   100644
>  GIT binary patch
>  literal 2528128
> >>>
> >>> Consider using 'git-format-patch --no-binary' and a reference
> >>> to your repository to fetch a such big binary patch.
> >>
> >> David would pull from my tree then ? So that's like doing a PR. 
> >> We can do that next time I send an update if David is OK with 
> >> that. I should send an update for v7.0 tag. 
> > 
> > As you wish. Big patches gave us troubles, i.e. they make crash
> > the 'patches' instance. 2.5MiB is probably borderline and I'm
> > being nit-picky.
> > 
> 
> If we can do a PR next time, all should be fine.
> 
> Thanks,
> 
> C.
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] xive: Remove extra '0x' prefix in trace events

[David Gibson]

On Mon, Aug 09, 2021 at 11:39:49AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/9/21 10:52 AM, Cédric Le Goater wrote:
> > Cc: th...@redhat.com
> > Fixes: 4e960974d4ee ("xive: Add trace events")
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/519
> > Signed-off-by: Cédric Le Goater 
> > ---
> >  hw/intc/trace-events | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/intc/trace-events b/hw/intc/trace-events
> > index e56e7dd3b667..6a17d38998d9 100644
> > --- a/hw/intc/trace-events
> > +++ b/hw/intc/trace-events
> > @@ -219,14 +219,14 @@ kvm_xive_source_reset(uint32_t srcno) "IRQ 0x%x"
> >  xive_tctx_accept(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr, 
> > uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x 
> > CPPR=0x%02x NSR=0x%02x ACK"
> >  xive_tctx_notify(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t pipr, 
> > uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x PIPR=0x%02x 
> > CPPR=0x%02x NSR=0x%02x raise !"
> >  xive_tctx_set_cppr(uint32_t index, uint8_t ring, uint8_t ipb, uint8_t 
> > pipr, uint8_t cppr, uint8_t nsr) "target=%d ring=0x%x IBP=0x%02x 
> > PIPR=0x%02x new CPPR=0x%02x NSR=0x%02x"
> > -xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value) 
> > "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> > -xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value) 
> > "@0x0x%"PRIx64" IRQ 0x%x val=0x0x%"PRIx64
> > +xive_source_esb_read(uint64_t addr, uint32_t srcno, uint64_t value) 
> > "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> > +xive_source_esb_write(uint64_t addr, uint32_t srcno, uint64_t value) 
> > "@0x%"PRIx64" IRQ 0x%x val=0x%"PRIx64
> >  xive_router_end_notify(uint8_t end_blk, uint32_t end_idx, uint32_t 
> > end_data) "END 0x%02x/0x%04x -> enqueue 0x%08x"
> >  xive_router_end_escalate(uint8_t end_blk, uint32_t end_idx, uint8_t 
> > esc_blk, uint32_t esc_idx, uint32_t end_data) "END 0x%02x/0x%04x -> 
> > escalate END 0x%02x/0x%04x data 0x%08x"
> > -xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) 
> > "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> > -xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) 
> > "@0x0x%"PRIx64" sz=%d val=0x%" PRIx64
> > +xive_tctx_tm_write(uint64_t offset, unsigned int size, uint64_t value) 
> > "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> > +xive_tctx_tm_read(uint64_t offset, unsigned int size, uint64_t value) 
> > "@0x%"PRIx64" sz=%d val=0x%" PRIx64
> >  xive_presenter_notify(uint8_t nvt_blk, uint32_t nvt_idx, uint8_t ring) 
> > "found NVT 0x%x/0x%x ring=0x%x"
> > -xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) 
> > "END 0x%x/0x%x @0x0x%"PRIx64
> > +xive_end_source_read(uint8_t end_blk, uint32_t end_idx, uint64_t addr) 
> > "END 0x%x/0x%x @0x%"PRIx64
> >  
> >  # pnv_xive.c
> >  pnv_xive_ic_hw_trigger(uint64_t addr, uint64_t val) "@0x%"PRIx64" 
> > val=0x%"PRIx64
> > 
> 
> Acceptable for 6.1 IMHO.

Acceptable for, but also not vital for.  I've applied this to
ppc-for-6.1, but I'll probably only bother sending a PR if some more
crucial fixes come along.

> 
> Reviewed-by: Philippe Mathieu-Daudé 
> 

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH] ppc/pnv: update skiboot to commit 820d43c0a775.

[David Gibson]

On Mon, Aug 09, 2021 at 09:55:35AM +0200, Cédric Le Goater wrote:
> Hello Phil,
> 
> On 8/9/21 9:06 AM, Philippe Mathieu-Daudé wrote:
> > Hi Cédric,
> > 
> > On 8/6/21 8:00 PM, Cédric Le Goater wrote:
> >> It includes support for the POWER10 processor and the QEMU platform.
> > 
> > 1/ Can you include the output of 'git shortlog v6.4..820d43c0' here?
> 
> OK. See attachement.
> >>
> >> Built from submodule.
> > 
> > 2/ Could we have a CI job building this, during 6.2 cycle?
> >(See .gitlab-ci.d/edk2.yml and .gitlab-ci.d/opensbi.yml)
> 
> Sure. It doesn't look too complex. 
> 
> I plan to add acceptance tests for the QEMU powernv machines also 
> once the OpenPOWER files (zImage.epapr and rootfs.cpio.xz) are 
> published on GH.
>  
> >>
> >> Signed-off-by: Cédric Le Goater 
> >> ---
> >>  pc-bios/skiboot.lid | Bin 1667280 -> 2528128 bytes
> >>  roms/skiboot|   2 +-
> >>  2 files changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/pc-bios/skiboot.lid b/pc-bios/skiboot.lid
> >> index 
> >> 504b95e8b6611aff3a934ff10f789934680591f9..8a3c278512a428a034ed5b1ddbed017ae8c0a9d0
> >>  100644
> >> GIT binary patch
> >> literal 2528128
> > 
> > Consider using 'git-format-patch --no-binary' and a reference
> > to your repository to fetch a such big binary patch.
> 
> David would pull from my tree then ? So that's like doing a PR. 
> We can do that next time I send an update if David is OK with 
> that. I should send an update for v7.0 tag.

Yes, I'm fine with that.  That's currently how Alexey sends SLOF
updates.

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [PATCH for 6.2 30/49] bsd-user: elf cleanup

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh

Move OS-dependent defines into target_os_elf.h. Move the architectural
dependent stuff into target_arch_elf.h. Adjust elfload.c to use
target_create_elf_tables instead of create_elf_tables.

Signed-off-by: Warner Losh
Signed-off-by: Stacey Son
Signed-off-by: Kyle Evans
Signed-off-by: Justin Hibbits
Signed-off-by: Alexander Kabaev

Sponsored by:   Netflix
---
  bsd-user/elfload.c   | 190 ---
  bsd-user/freebsd/target_os_elf.h | 149 
  bsd-user/netbsd/target_os_elf.h  | 143 +++
  bsd-user/openbsd/target_os_elf.h | 143 +++
  bsd-user/qemu.h  |   1 +
  5 files changed, 459 insertions(+), 167 deletions(-)
  create mode 100644 bsd-user/freebsd/target_os_elf.h
  create mode 100644 bsd-user/netbsd/target_os_elf.h
  create mode 100644 bsd-user/openbsd/target_os_elf.h


Acked-by: Richard Henderson 

r~

Re: [PATCH v1] s390x/ioinst: Fix wrong MSCH alignment check on little endian

[Halil Pasic]

On Thu,  5 Aug 2021 16:37:53 +0200
David Hildenbrand  wrote:

> schib->pmcw.chars is 32bit, not 16bit. This fixes the kvm-unit-tests
> "css" test, which fails with:
> 
>   FAIL: Channel Subsystem: measurement block format1: Unaligned MB origin:
>   Program interrupt: expected(21) == received(0)
> 
> Because we end up not injecting an operand program exception.
> 
> Fixes: a54b8ac340c2 ("css: SCHIB measurement block origin must be aligned")
> Cc: Halil Pasic 
> Cc: Cornelia Huck 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: Thomas Huth 
> Cc: Pierre Morel 
> Cc: qemu-s3...@nongnu.org
> Signed-off-by: David Hildenbrand 

Reviewed-by: Halil Pasic 

> ---
>  target/s390x/ioinst.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/ioinst.c b/target/s390x/ioinst.c
> index 4eb0a7a9f8..bdae5090bc 100644
> --- a/target/s390x/ioinst.c
> +++ b/target/s390x/ioinst.c
> @@ -123,7 +123,7 @@ static int ioinst_schib_valid(SCHIB *schib)
>  }
>  /* for MB format 1 bits 26-31 of word 11 must be 0 */
>  /* MBA uses words 10 and 11, it means align on 2**6 */
> -if ((be16_to_cpu(schib->pmcw.chars) & PMCW_CHARS_MASK_MBFC) &&
> +if ((be32_to_cpu(schib->pmcw.chars) & PMCW_CHARS_MASK_MBFC) &&
>  (be64_to_cpu(schib->mba) & 0x03fUL)) {
>  return 0;
>  }

Re: Re: [PATCH v4] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Mon, Aug 9, 2021 at 3:58 AM Stefano Garzarella  wrote:
>
> On Thu, Aug 05, 2021 at 12:07:02PM -0700, Jiang Wang . wrote:
> >On Wed, Aug 4, 2021 at 1:13 AM Stefano Garzarella  
> >wrote:
> >>
> >> On Tue, Aug 03, 2021 at 11:41:32PM +, Jiang Wang wrote:
> >> >Datagram sockets are connectionless and unreliable.
> >> >The sender does not know the capacity of the receiver
> >> >and may send more packets than the receiver can handle.
> >> >
> >> >Add two more dedicate virtqueues for datagram sockets,
> >> >so that it will not unfairly steal resources from
> >> >stream and future connection-oriented sockets.
> >> >
> >> >Signed-off-by: Jiang Wang 
> >> >---
> >> >v1 -> v2: use qemu cmd option to control number of queues,
> >> >   removed configuration settings for dgram.
> >> >v2 -> v3: use ioctl to get features and decide number of
> >> >virt queues, instead of qemu cmd option.
> >> >v3 -> v4: change DGRAM feature bit value to 2. Add an argument
> >> >   in vhost_vsock_common_realize to indicate dgram is supported or 
> >> > not.
> >> >
> >> > hw/virtio/vhost-user-vsock.c  |  2 +-
> >> > hw/virtio/vhost-vsock-common.c| 58 ++-
> >> > hw/virtio/vhost-vsock.c   |  5 +-
> >> > include/hw/virtio/vhost-vsock-common.h|  6 +-
> >> > include/hw/virtio/vhost-vsock.h   |  4 ++
> >> > include/standard-headers/linux/virtio_vsock.h |  1 +
> >> > 6 files changed, 69 insertions(+), 7 deletions(-)
> >> >
> >> >diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> >> >index 6095ed7349..e9ec0e1c00 100644
> >> >--- a/hw/virtio/vhost-user-vsock.c
> >> >+++ b/hw/virtio/vhost-user-vsock.c
> >> >@@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, 
> >> >Error **errp)
> >> > return;
> >> > }
> >> >
> >> >-vhost_vsock_common_realize(vdev, "vhost-user-vsock");
> >> >+vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
> >> >
> >> > vhost_dev_set_config_notifier(>vhost_dev, _ops);
> >> >
> >> >diff --git a/hw/virtio/vhost-vsock-common.c 
> >> >b/hw/virtio/vhost-vsock-common.c
> >> >index 4ad6e234ad..c78536911a 100644
> >> >--- a/hw/virtio/vhost-vsock-common.c
> >> >+++ b/hw/virtio/vhost-vsock-common.c
> >> >@@ -17,6 +17,8 @@
> >> > #include "hw/virtio/vhost-vsock.h"
> >> > #include "qemu/iov.h"
> >> > #include "monitor/monitor.h"
> >> >+#include 
> >> >+#include 
> >> >
> >> > int vhost_vsock_common_start(VirtIODevice *vdev)
> >> > {
> >> >@@ -196,9 +198,39 @@ int vhost_vsock_common_post_load(void *opaque, int 
> >> >version_id)
> >> > return 0;
> >> > }
> >> >
> >> >-void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name)
> >> >+static int vhost_vsock_get_max_qps(bool enable_dgram)
> >> >+{
> >> >+uint64_t features;
> >> >+int ret;
> >> >+int fd = -1;
> >> >+
> >> >+if (!enable_dgram)
> >> >+return MAX_VQS_WITHOUT_DGRAM;
> >> >+
> >> >+fd = qemu_open_old("/dev/vhost-vsock", O_RDONLY);
> >>
> >>
> >> As I said in the previous version, we cannot directly open
> >> /dev/vhost-vsock, for two reasons:
> >>
> >>1. this code is common with vhost-user-vsock which does not use
> >>/dev/vhost-vsock.
> >>
> >>2. the fd may have been passed from the management layer and qemu may
> >>not be able to directly open /dev/vhost-vsock.
> >>
> >> I think is better to move this function in hw/virtio/vhost-vsock.c,
> >> using the `vhostfd`, returning true or false if dgram is supported, then
> >> you can use it for `enable_dgram` param ...
> >>
> >
> >Yes, you are right. Now I remember you said that before but I forgot about 
> >that
> >when I changed the code. I will fix it. Sorry about that.
>
> No problem :-)
>
> >
> >> >+if (fd == -1) {
> >> >+error_report("vhost-vsock: failed to open device. %s", 
> >> >strerror(errno));
> >> >+return -1;
> >> >+}
> >> >+
> >> >+ret = ioctl(fd, VHOST_GET_FEATURES, );
> >> >+if (ret) {
> >> >+error_report("vhost-vsock: failed to read  device. %s", 
> >> >strerror(errno));
> >> >+qemu_close(fd);
> >> >+return ret;
> >> >+}
> >> >+
> >> >+qemu_close(fd);
> >> >+if (features & (1 << VIRTIO_VSOCK_F_DGRAM))
> >> >+return MAX_VQS_WITH_DGRAM;
> >> >+
> >> >+return MAX_VQS_WITHOUT_DGRAM;
> >> >+}
> >> >+
> >> >+void vhost_vsock_common_realize(VirtIODevice *vdev, const char *name, 
> >> >bool enable_dgram)
> >> > {
> >> > VHostVSockCommon *vvc = VHOST_VSOCK_COMMON(vdev);
> >> >+int nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> >
> >> > virtio_init(vdev, name, VIRTIO_ID_VSOCK,
> >> > sizeof(struct virtio_vsock_config));
> >> >@@ -209,12 +241,24 @@ void vhost_vsock_common_realize(VirtIODevice
> >> >*vdev, const char *name)
> >> > vvc->trans_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >> >vhost_vsock_common_handle_output);
> >> >
> >> >+nvqs 

Re: Re: [RFC v3] virtio/vsock: add two more queues for datagram types

[Jiang Wang .]

On Mon, Aug 9, 2021 at 3:49 AM Stefano Garzarella  wrote:
>
> On Thu, Aug 05, 2021 at 12:00:05PM -0700, Jiang Wang . wrote:
> >On Tue, Aug 3, 2021 at 11:49 PM Stefano Garzarella  
> >wrote:
> >>
> >> On Wed, Aug 4, 2021 at 8:41 AM Stefano Garzarella 
> >> wrote:
> >> >
> >> > On Tue, Aug 03, 2021 at 11:58:27AM -0700, Jiang Wang . wrote:
> >> > >On Wed, Jul 7, 2021 at 10:27 AM Stefano Garzarella 
> >> > > wrote:
> >> > >> On Wed, Jul 07, 2021 at 09:52:46AM -0700, Jiang Wang . wrote:
> >> > >> >On Wed, Jul 7, 2021 at 1:33 AM Stefano Garzarella 
> >> > >> > wrote:
> >> > >> >> On Tue, Jul 06, 2021 at 10:26:07PM +, Jiang Wang wrote:
> >> >
> >> > [...]
> >> >
> >> > >> >> >+
> >> > >> >> >+if (nvqs < 0)
> >> > >> >> >+nvqs = MAX_VQS_WITHOUT_DGRAM;
> >> > >> >> >+
> >> > >> >> >+if (nvqs == MAX_VQS_WITH_DGRAM) {
> >> > >> >> >+vvc->dgram_recv_vq = virtio_add_queue(vdev, 
> >> > >> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> > >> >> >+  
> >> > >> >> >vhost_vsock_common_handle_output);
> >> > >> >> >+vvc->dgram_trans_vq = virtio_add_queue(vdev, 
> >> > >> >> >VHOST_VSOCK_QUEUE_SIZE,
> >> > >> >> >+   
> >> > >> >> >vhost_vsock_common_handle_output);
> >> > >> >> >+}
> >> > >> >> >+
> >> > >> >> > /* The event queue belongs to QEMU */
> >> > >> >> > vvc->event_vq = virtio_add_queue(vdev, 
> >> > >> >> > VHOST_VSOCK_QUEUE_SIZE,
> >> > >> >> >
> >> > >> >> > vhost_vsock_common_handle_output);
> >> > >> >>
> >> > >> >> Did you do a test with a guest that doesn't support datagram with 
> >> > >> >> QEMU
> >> > >> >> and hosts that do?
> >> > >> >>
> >> > >> >Yes, and it works.
> >> > >> >
> >> > >> >> I repost my thoughts that I had on v2:
> >> > >> >>
> >> > >> >>  What happen if the guest doesn't support dgram?
> >> > >> >>
> >> > >> >>  I think we should dynamically use the 3rd queue or the 5th 
> >> > >> >> queue for
> >> > >> >>  the events at runtime after the guest acked the features.
> >> > >> >>
> >> > >> >>  Maybe better to switch to an array of VirtQueue.
> >> > >> >>
> >> > >> >I think in current V3, it  already dynamically use 3rd or 5th queue
> >> > >> >depending
> >> > >> >on the feature bit.
> >> > >>
> >> > >> I'm not sure. IIUC when vhost_vsock_common_realize() is called, we 
> >> > >> don't
> >> > >> know the features acked by the guest, so how can it be dynamic?
> >> > >>
> >> > >> Here we should know only if the host kernel supports it.
> >> > >>
> >> > >> Maybe it works, because in QEMU we use the event queue only after a
> >> > >> migration to send a reset event, so you can try to migrate a guest to
> >> > >> check this path.
> >> > >>
> >> > >I tried VM migration and didn't see any problems. The migration looks 
> >> > >fine
> >> > >and vsock dgram still works after migration. Is there any more specific 
> >> > >test
> >> > >you want to run to check for this code path?
> >> > >
> >> >
> >> > I meant a migration of a guest from QEMU without this patch to a QEMU
> >> > with this patch. Of course in that case testing a socket stream.
> >> >
> >>
> >> Sorry, I meant the opposite.
> >>
> >> You should try to migrate a guest that does not support dgrams starting
> >> from a QEMU with this patch (and kernel that supports dgram, so qemu
> >> uses the 5th queue for event), to a QEMU without this patch.
> >>
> >Got it. I tried what you said and saw errors on the destination qemu. Then
> >I moved event queue up to be number 3 (before adding dgram vqs),
> >as the following:
> >
> >+/* The event queue belongs to QEMU */
> >+vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >+   vhost_vsock_common_handle_output);
> >+
> >
> > nvqs = vhost_vsock_get_max_qps(enable_dgram);
> >
> >@@ -253,10 +257,6 @@ void vhost_vsock_common_realize(VirtIODevice
> >*vdev, const char *name, bool enabl
> >
> >vhost_vsock_common_handle_output);
> > }
> >
> >-/* The event queue belongs to QEMU */
> >-vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> >-   vhost_vsock_common_handle_output);
> >-
> >
> >But I still see following errors on the destination qemu:
> >qemu-system-x86_64: Error starting vhost vsock: 14
> >
> >Any idea if my above code change is wrong or missing something?
>
> No, sorry.
> Do you have some kernel messages in the host?
>

I checked dmesg but did not find any errors. I will debug more.

> >
> >Take one step back, what should be the host kernel version? With or
> >without dgram support? I tried both.  The new dest kernel shows the above 
> >error.
> >The old dest kernel shows a msr error probably not related to vsock.
>
> I think the best is to try the host kernel with DGRAM support, so QEMU
> will allocate all the queues.
>
> >
> >To explain the above qemu 14 error, I think the issue is that the
> >source 

Re: [PATCH for 6.2 29/49] bsd-user: Add system independent stack, data and text limiting

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh

Eliminate the x86 specific stack stuff in favor of more generic control
over the process size:
 target_maxtsiz  max text size
 target_dfldsiz  initial data size limit
 target_maxdsiz  max data size
 target_dflssiz  initial stack size limit
 target_maxssiz  max stack size
 target_sgrowsiz amount to grow stack
These can be set on a per-arch basis, and the stack size can be set
on the command line. Adjust the stack size parameters at startup.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh

Sponsored by:   Netflix
---
  bsd-user/elfload.c |  2 +-
  bsd-user/main.c| 51 +-
  bsd-user/qemu.h|  7 ++-
  3 files changed, 44 insertions(+), 16 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for 6.2 28/49] bsd-user: Move stack initializtion into a per-os file.

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

+static inline int setup_initial_stack(struct bsd_binprm *bprm,
+abi_ulong *ret_addr, abi_ulong *stringp)
+{
+int i;
+abi_ulong stack_hi_addr;
+size_t execpath_len, stringspace;
+abi_ulong destp, argvp, envp, p;
+struct target_ps_strings ps_strs;
+char canary[sizeof(abi_long) * 8];
+
+stack_hi_addr = p = target_stkbas + target_stksiz;
+
+/* Save some space for ps_strings. */
+p -= sizeof(struct target_ps_strings);
+
+#ifdef TARGET_SZSIGCODE
+/* Add machine depedent sigcode. */
+p -= TARGET_SZSIGCODE;
+if (setup_sigtramp(p, (unsigned)offsetof(struct target_sigframe, sf_uc),
+TARGET_FREEBSD_NR_sigreturn)) {
+errno = EFAULT;
+return -1;
+}
+#endif


Hmm.  The x86 version you just added always returns -EOPNOTSUPP.  Therefore I conclude 
that TARGET_SZSIGCODE is unset.


Perhaps a better interface would be

  p = setup_sigtramp(p, ...);
  if (p == 0) {
// fail
  }

then you don't need to expose TARGET_SZSIGCODE or have conditional compilation 
here.

Perhaps for the to-do list...


+/* Add canary for SSP. */
+arc4random_buf(canary, sizeof(canary));


You should use qemu_guest_getrandom_nofail here.


+/*
+ * Deviate from FreeBSD stack layout: force stack to new page here
+ * so that signal trampoline is not sharing the page with user stack
+ * frames. This is actively harmful in qemu as it marks pages with
+ * code it translated as read-only, which is somewhat problematic
+ * for user trying to use the stack as intended.
+ */


A decent short-term solution.

I'll draw your attention to my vdso patch set for linux-user:
https://patchew.org/QEMU/20210706234932.356913-1-richard.hender...@linaro.org/

Modulo randomness,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH for 6.2 27/49] bsd-user: Add architecture specific signal tramp code

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh

Add a stubbed out version of setup_sigtramp. This is not yet used for
x86, but is used for other architectures. This will be connected in
future commits.

Signed-off-by: Stacey Son
Signed-off-by: Warner Losh
---
  bsd-user/i386/target_arch_sigtramp.h   | 29 ++
  bsd-user/x86_64/target_arch_sigtramp.h | 29 ++
  2 files changed, 58 insertions(+)
  create mode 100644 bsd-user/i386/target_arch_sigtramp.h
  create mode 100644 bsd-user/x86_64/target_arch_sigtramp.h


Reviewed-by: Richard Henderson 

r~

Re: [PATCH for 6.2 26/49] bsd-user: Create target specific vmparam.h

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

+#define TARGET_MAXTSIZ  (128UL*1024*1024)   /* max text size */
+#define TARGET_DFLDSIZ  (128UL*1024*1024)   /* initial data size limit */
+#define TARGET_MAXDSIZ  (512UL*1024*1024)   /* max data size */
+#define TARGET_DFLSSIZ  (8UL*1024*1024) /* initial stack size limit */
+#define TARGET_MAXSSIZ  (64UL*1024*1024)/* max stack size */
+#define TARGET_SGROWSIZ (128UL*1024)/* amount to grow stack */


To-do list: KiB and MiB from units.h.


+++ b/bsd-user/qemu.h
@@ -44,7 +44,7 @@ extern enum BSDType bsd_type;
  #include "target_arch.h"
  #include "syscall_defs.h"
  #include "target_syscall.h"
-//#include "target_os_vmparam.h"
+#include "target_os_vmparam.h"
  //#include "target_os_signal.h"
  //#include "hostdep.h"


Ah, I see.  Well, perhaps just squash the addition of the include to the patch that 
introduces the include?


Otherwise,
Reviewed-by: Richard Henderson 


r~

Re: [PATCH for 6.2 25/49] bsd-user: define max args in terms of pages

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh 

For 32-bit platforms, pass in up to 256k of args. For 64-bit, bump that
to 512k.

Signed-off-by: Kyle Evans 
Signed-off-by: Warner Losh 
---
  bsd-user/qemu.h | 14 ++
  1 file changed, 10 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 


+#define TARGET_ARG_MAX (512 * 1024)
+#else
+#define TARGET_ARG_MAX (256 * 1024)


For the to-do list: qemu/units.h has KiB for clarity.


r~

Re: [PATCH for 6.2 24/49] bsd-user: Include more things in qemu.h

[Richard Henderson]

On 8/7/21 11:42 AM, Warner Losh wrote:

From: Warner Losh 

Include more header files to match bsd-user fork.

Signed-off-by: Warner Losh 
---
  bsd-user/qemu.h | 9 +
  1 file changed, 9 insertions(+)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 6c4ec61d76..02e6e8327a 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -18,11 +18,15 @@
  #define QEMU_H
  
  
+#include "qemu/osdep.h"

  #include "cpu.h"
  #include "exec/cpu_ldst.h"
+#include "exec/exec-all.h"
+//#include "trace/trace-bsd_user.h"
  
  #undef DEBUG_REMAP

  #ifdef DEBUG_REMAP
+#include 
  #endif /* DEBUG_REMAP */


osdep.h will have included stdio.h.


+//#include "target_os_vmparam.h"
+//#include "target_os_signal.h"
+//#include "hostdep.h"


Delete these?


r~

Re: [PULL 0/2] Block patches for 6.1-rc3

[Peter Maydell]

On Mon, 9 Aug 2021 at 18:03, Hanna Reitz  wrote:
>
> Hi Peter,
>
> Let me prefix this by saying that it's me, Max.  I've changed my name
> and email address.  I understand freeze may not be the best of times for
> this, but it looks like I can no longer send mails from the mreitz@
> address for now (only receive them).
>
> I've tried to create and sign the tag as Max, so I hope this pull
> request won't run into any issues from that perspective.
>
> (For the future, I'll create a new key and hope signing it with my old
> key will make it sufficiently trustworthy...)
>
>
> The following changes since commit 632eda54043d6f26ff87dac16233e14b4708b967:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' 
> into staging (2021-08-09 11:04:27 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2021-08-09
>
> for you to fetch changes up to a6d2bb25cf945cd16f29a575055c6f1a1f9cf6c9:
>
>   tests: filter out TLS distinguished name in certificate checks (2021-08-09 
> 17:32:43 +0200)
>
> 
> Block patches for 6.1-rc3:
> - Build fix for FUSE block exports
> - iotest 233 fix
>

Applied, thanks.

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

-- PMM

[PATCH for-6.2 v3 1/2] qom: DECLARE_INTERFACE_CHECKER macro

[Eduardo Habkost]

The new macro will be similar to DECLARE_INSTANCE_CHECKER,
but for interface types (that use INTEFACE_CHECK instead of
OBJECT_CHECK).

Signed-off-by: Eduardo Habkost 
---
Changes series v1 -> v2: none
Change series v2 -> v3:
* Rebased on top of
  [PATCH for-6.2 00/12] qom: Get rid of all manual usage of OBJECT_CHECK & 
friends
---
Cc: Paolo Bonzini 
Cc: "Daniel P. Berrangé" 
Cc: Eduardo Habkost 
Cc: qemu-devel@nongnu.org
---
 include/qom/object.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index faae0d841fe..4a9d7017d9f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -178,6 +178,20 @@ struct Object
 OBJ_NAME(const void *obj) \
 { return OBJECT_CHECK(InstanceType, obj, TYPENAME); }
 
+/**
+ * DECLARE_INTERFACE_CHECKER:
+ * @InstanceType: instance type
+ * @OBJ_NAME: the object name in uppercase with underscore separators
+ * @TYPENAME: type name
+ *
+ * This macro will provide the instance type cast functions for a
+ * QOM interface type.
+ */
+#define DECLARE_INTERFACE_CHECKER(InstanceType, OBJ_NAME, TYPENAME) \
+static inline G_GNUC_UNUSED InstanceType * \
+OBJ_NAME(const void *obj) \
+{ return INTERFACE_CHECK(InstanceType, obj, TYPENAME); }
+
 /**
  * DECLARE_CLASS_CHECKERS:
  * @ClassType: class struct name
-- 
2.31.1

[PATCH for-6.2 v3 2/2] [autoamted] Use DECLARE_INTERFACE_CHECKER macro

[Eduardo Habkost]

Replace manual INTERFACE_CHECK macros with
DECLARE_INTERFACE_CHECKER, which is less error prone.

Generated using:

 $ ./scripts/codeconverter/converter.py -i \
   --pattern=InterfaceCheckMacro $(g grep -l '' -- '*.[ch]')

Signed-off-by: Eduardo Habkost 
---
 include/hw/acpi/acpi_dev_interface.h |  5 ++---
 include/hw/arm/linux-boot-if.h   |  4 ++--
 include/hw/fw-path-provider.h|  4 ++--
 include/hw/hotplug.h |  4 ++--
 include/hw/intc/intc.h   |  5 ++---
 include/hw/ipmi/ipmi.h   |  4 ++--
 include/hw/isa/isa.h |  4 ++--
 include/hw/mem/memory-device.h   |  4 ++--
 include/hw/nmi.h |  4 ++--
 include/hw/ppc/pnv_xscom.h   |  4 ++--
 include/hw/ppc/spapr_irq.h   |  4 ++--
 include/hw/ppc/xics.h|  4 ++--
 include/hw/ppc/xive.h| 12 ++--
 include/hw/rdma/rdma.h   |  5 ++---
 include/hw/rtc/m48t59.h  |  4 ++--
 include/hw/stream.h  |  4 ++--
 include/hw/vmstate-if.h  |  4 ++--
 include/qom/object_interfaces.h  |  5 ++---
 include/sysemu/tpm.h |  4 ++--
 target/arm/idau.h|  4 ++--
 tests/unit/check-qom-interface.c |  4 ++--
 21 files changed, 46 insertions(+), 50 deletions(-)

diff --git a/include/hw/acpi/acpi_dev_interface.h 
b/include/hw/acpi/acpi_dev_interface.h
index c9c7c17e043..f99b8b380eb 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -22,9 +22,8 @@ typedef struct AcpiDeviceIfClass AcpiDeviceIfClass;
 DECLARE_CLASS_CHECKERS(AcpiDeviceIfClass, ACPI_DEVICE_IF,
TYPE_ACPI_DEVICE_IF)
 typedef struct AcpiDeviceIf AcpiDeviceIf;
-#define ACPI_DEVICE_IF(obj) \
- INTERFACE_CHECK(AcpiDeviceIf, (obj), \
- TYPE_ACPI_DEVICE_IF)
+DECLARE_INTERFACE_CHECKER(AcpiDeviceIf, ACPI_DEVICE_IF,
+  TYPE_ACPI_DEVICE_IF)
 
 
 void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
diff --git a/include/hw/arm/linux-boot-if.h b/include/hw/arm/linux-boot-if.h
index 295e282c36e..17b8083f954 100644
--- a/include/hw/arm/linux-boot-if.h
+++ b/include/hw/arm/linux-boot-if.h
@@ -13,8 +13,8 @@ typedef struct ARMLinuxBootIfClass ARMLinuxBootIfClass;
 DECLARE_CLASS_CHECKERS(ARMLinuxBootIfClass, ARM_LINUX_BOOT_IF,
TYPE_ARM_LINUX_BOOT_IF)
 typedef struct ARMLinuxBootIf ARMLinuxBootIf;
-#define ARM_LINUX_BOOT_IF(obj) \
-INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
+DECLARE_INTERFACE_CHECKER(ARMLinuxBootIf, ARM_LINUX_BOOT_IF,
+  TYPE_ARM_LINUX_BOOT_IF)
 
 
 struct ARMLinuxBootIfClass {
diff --git a/include/hw/fw-path-provider.h b/include/hw/fw-path-provider.h
index 33d91daed52..639fe9d821a 100644
--- a/include/hw/fw-path-provider.h
+++ b/include/hw/fw-path-provider.h
@@ -26,8 +26,8 @@ typedef struct FWPathProviderClass FWPathProviderClass;
 DECLARE_CLASS_CHECKERS(FWPathProviderClass, FW_PATH_PROVIDER,
TYPE_FW_PATH_PROVIDER)
 typedef struct FWPathProvider FWPathProvider;
-#define FW_PATH_PROVIDER(obj) \
- INTERFACE_CHECK(FWPathProvider, (obj), TYPE_FW_PATH_PROVIDER)
+DECLARE_INTERFACE_CHECKER(FWPathProvider, FW_PATH_PROVIDER,
+  TYPE_FW_PATH_PROVIDER)
 
 
 struct FWPathProviderClass {
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index 75d32d69e2b..5dc7435a4c5 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -20,8 +20,8 @@ typedef struct HotplugHandlerClass HotplugHandlerClass;
 DECLARE_CLASS_CHECKERS(HotplugHandlerClass, HOTPLUG_HANDLER,
TYPE_HOTPLUG_HANDLER)
 typedef struct HotplugHandler HotplugHandler;
-#define HOTPLUG_HANDLER(obj) \
- INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
+DECLARE_INTERFACE_CHECKER(HotplugHandler, HOTPLUG_HANDLER,
+  TYPE_HOTPLUG_HANDLER)
 
 
 /**
diff --git a/include/hw/intc/intc.h b/include/hw/intc/intc.h
index 7c57c3a0379..a31b5341acb 100644
--- a/include/hw/intc/intc.h
+++ b/include/hw/intc/intc.h
@@ -9,9 +9,8 @@ typedef struct InterruptStatsProviderClass 
InterruptStatsProviderClass;
 DECLARE_CLASS_CHECKERS(InterruptStatsProviderClass, INTERRUPT_STATS_PROVIDER,
TYPE_INTERRUPT_STATS_PROVIDER)
 typedef struct InterruptStatsProvider InterruptStatsProvider;
-#define INTERRUPT_STATS_PROVIDER(obj) \
-INTERFACE_CHECK(InterruptStatsProvider, (obj), \
-TYPE_INTERRUPT_STATS_PROVIDER)
+DECLARE_INTERFACE_CHECKER(InterruptStatsProvider, INTERRUPT_STATS_PROVIDER,
+  TYPE_INTERRUPT_STATS_PROVIDER)
 
 
 struct InterruptStatsProviderClass {
diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
index d655352fa95..983e7366e75 100644
--- a/include/hw/ipmi/ipmi.h
+++ b/include/hw/ipmi/ipmi.h
@@ -110,8 +110,8 @@ uint32_t ipmi_next_uuid(void);
  */
 #define TYPE_IPMI_INTERFACE 

Re: [PATCH] docs/devel: fix missing antislash

[Eduardo Habkost]

On Mon, Aug 09, 2021 at 07:31:41PM +0200, Alexandre Iooss wrote:
> Signed-off-by: Alexandre Iooss 
> ---
>  docs/devel/qom.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
> index e5fe3597cd..b9568c0fb8 100644
> --- a/docs/devel/qom.rst
> +++ b/docs/devel/qom.rst
> @@ -309,7 +309,7 @@ This is equivalent to the following:
> OBJECT_GET_CLASS(MyDeviceClass, obj, TYPE_MY_DEVICE)
> #define MY_DEVICE_CLASS(void *klass) \
> OBJECT_CLASS_CHECK(MyDeviceClass, klass, TYPE_MY_DEVICE)
> -   #define MY_DEVICE(void *obj)
> +   #define MY_DEVICE(void *obj) \
> OBJECT_CHECK(MyDevice, obj, TYPE_MY_DEVICE)

Oops, nice catch!

However, the code above is already going to be deleted by:
https://lore.kernel.org/qemu-devel/20210729175554.686474-9-ehabk...@redhat.com

-- 
Eduardo

[PATCH for-6.2 v3 0/2] qom: DECLARE_INTERFACE_CHECKER macro

[Eduardo Habkost]

This is an alternative to the series:
  Subject: [PATCH 0/3] qom: Replace INTERFACE_CHECK with OBJECT_CHECK
  
https://lore.kernel.org/qemu-devel/20200916193101.511600-1-ehabk...@redhat.com/

Instead of removing INTERFACE_CHECK completely, keep it but use a
DECLARE_INTERFACE_CHECKER macro to define the type checking functions for
interface types.

Maybe one day INTERFACE_CHECK/DECLARE_INTERFACE_CHECKER will be
completely replaced with OBJECT_CHECK/DECLARE_INSTANCE_CHECKER,
but by now it might be useful to keep the distinction between
regular objects and interface types.  See discussion at
https://lore.kernel.org/qemu-devel/20200916221347.gl7...@habkost.net

Based-on: <20210806211127.646908-1-ehabk...@redhat.com>

Changes v2 -> v3:
* Rebased on top of
  Subject: [PATCH for-6.2 00/12] qom: Get rid of all manual usage of 
OBJECT_CHECK & friends
  Date: Fri,  6 Aug 2021 17:11:15 -0400
  Message-Id: <20210806211127.646908-1-ehabk...@redhat.com>
  https://lore.kernel.org/qemu-devel/20210806211127.646908-1-ehabk...@redhat.com

Changes v1 -> v2:
* Move declaration after typedefs, so the code actually compiles

Links to previous versions:
v1: 
https://lore.kernel.org/qemu-devel/20200916223258.599367-1-ehabk...@redhat.com
v2: 
https://lore.kernel.org/qemu-devel/20200917024947.707586-1-ehabk...@redhat.com

Eduardo Habkost (2):
  qom: DECLARE_INTERFACE_CHECKER macro
  [autoamted] Use DECLARE_INTERFACE_CHECKER macro

 include/hw/acpi/acpi_dev_interface.h |  5 ++---
 include/hw/arm/linux-boot-if.h   |  4 ++--
 include/hw/fw-path-provider.h|  4 ++--
 include/hw/hotplug.h |  4 ++--
 include/hw/intc/intc.h   |  5 ++---
 include/hw/ipmi/ipmi.h   |  4 ++--
 include/hw/isa/isa.h |  4 ++--
 include/hw/mem/memory-device.h   |  4 ++--
 include/hw/nmi.h |  4 ++--
 include/hw/ppc/pnv_xscom.h   |  4 ++--
 include/hw/ppc/spapr_irq.h   |  4 ++--
 include/hw/ppc/xics.h|  4 ++--
 include/hw/ppc/xive.h| 12 ++--
 include/hw/rdma/rdma.h   |  5 ++---
 include/hw/rtc/m48t59.h  |  4 ++--
 include/hw/stream.h  |  4 ++--
 include/hw/vmstate-if.h  |  4 ++--
 include/qom/object.h | 14 ++
 include/qom/object_interfaces.h  |  5 ++---
 include/sysemu/tpm.h |  4 ++--
 target/arm/idau.h|  4 ++--
 tests/unit/check-qom-interface.c |  4 ++--
 22 files changed, 60 insertions(+), 50 deletions(-)

-- 
2.31.1

Re: [PATCH for-6.2 00/12] qom: Get rid of all manual usage of OBJECT_CHECK & friends

[Eduardo Habkost]

On Sat, Aug 07, 2021 at 10:15:52AM +0200, Philippe Mathieu-Daudé wrote:
> On 8/6/21 11:11 PM, Eduardo Habkost wrote:
> > This series gets rid of all manual usage of OBJECT_CHECK,
> > OBJECT_CLASS_CHECK, and OBJECT_GET_CLASS.
> > 
> > All type check macros defined manually are replaced with
> > DEFINE_*CHECKER* or OBJECT_DECLARE* macros.
> > 
> > All manual usage of OBJECT_CHECK/OBJECT_CLASS_CHECK/OBJECT_GET_CLASS
> > is manually replaced with the corresponding type-specific wrappers.
> 
> Is INTERFACE_CHECK already converted / in good shape?

Not yet.  I need to refresh my memory by looking at mailing list
archives, but I have a work in progress branch that I haven't
touched in a while (except for rebasing it) at:

https://gitlab.com/ehabkost/qemu/-/commits/work/qom-declare-interface-type/

Basically it introduces a DECLARE_INTERFACE_CHECKER macro instead
of reusing OBJECT_CHECK/DECLARE_INSTANCE_CHECKER.

-- 
Eduardo

migration-test, intermittent hang, aarch64 host, i386 guest

[Peter Maydell]

migration-test hung on me again in merge testing. Here are the
backtraces; note that one of the qemu-system-i386 processes is a
zombie that its parent isn't reaping.

Process tree:
migration-test(786453)-+-qemu-system-i38(802719)
   |-qemu-system-i38(802846)
   `-qemu-system-i38(807045)
===
PROCESS: 786453
pm786453  786451 19 18:29 ?00:12:30
tests/qtest/migration-test --tap -k -m quick
[New LWP 786457]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
0x997d178c in __GI___clock_nanosleep (clock_id=, clock_id@entry=0, flags=flags@entry=0,
req=req@entry=0xc3ba2e38, rem=rem@entry=0x0) at
../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
78  ../sysdeps/unix/sysv/linux/clock_nanosleep.c: No such file or directory.

Thread 2 (Thread 0x996bd1b0 (LWP 786457)):
#0  syscall () at ../sysdeps/unix/sysv/linux/aarch64/syscall.S:38
#1  0xb4c93ab0 in qemu_futex_wait (val=,
f=) at /home/pm/qemu/include/qemu/futex.h:29
#2  qemu_event_wait (ev=ev@entry=0xb4cd29b0
) at ../../util/qemu-thread-posix.c:480
#3  0xb4c94434 in call_rcu_thread (opaque=opaque@entry=0x0) at
../../util/rcu.c:258
#4  0xb4c92a08 in qemu_thread_start (args=) at
../../util/qemu-thread-posix.c:541
#5  0x998ab4fc in start_thread (arg=0xc3ba352f) at
pthread_create.c:477
#6  0x9980467c in thread_start () at
../sysdeps/unix/sysv/linux/aarch64/clone.S:78

Thread 1 (Thread 0x99af9690 (LWP 786453)):
#0  0x997d178c in __GI___clock_nanosleep (clock_id=, clock_id@entry=0, flags=flags@entry=0,
req=req@entry=0xc3ba2e38, rem=rem@entry=0x0) at
../sysdeps/unix/sysv/linux/clock_nanosleep.c:78
#1  0x997d7134 in __GI___nanosleep
(requested_time=requested_time@entry=0xc3ba2e38,
remaining=remaining@entry=0x0) at nanosleep.c:27
#2  0x997fe0e0 in usleep (useconds=useconds@entry=1000) at
../sysdeps/posix/usleep.c:32
#3  0xb4c6ba58 in wait_for_migration_status
(who=0xd57af470, goal=0xb4c9ea68 "cancelled", ungoals=0x0) at
../../tests/qtest/migration-helpers.c:157
#4  0xb4c6aeb0 in test_multifd_tcp_cancel () at
../../tests/qtest/migration-test.c:1376
#5  0x999f9f5c in ?? () from /lib/aarch64-linux-gnu/libglib-2.0.so.0
#6  0x999f9d60 in ?? () from /lib/aarch64-linux-gnu/libglib-2.0.so.0
#7  0x999f9d60 in ?? () from /lib/aarch64-linux-gnu/libglib-2.0.so.0
#8  0x999f9d60 in ?? () from /lib/aarch64-linux-gnu/libglib-2.0.so.0
#9  0x999f9d60 in ?? () from /lib/aarch64-linux-gnu/libglib-2.0.so.0
#10 0x999fa3e4 in g_test_run_suite () from
/lib/aarch64-linux-gnu/libglib-2.0.so.0
#11 0x999fa410 in g_test_run () from
/lib/aarch64-linux-gnu/libglib-2.0.so.0
#12 0xb4c683fc in main (argc=, argv=) at ../../tests/qtest/migration-test.c:1495
[Inferior 1 (process 786453) detached]

===
PROCESS: 802719
pm802719  786453 99 18:29 ?01:15:08 ./qemu-system-i386
-qtest unix:/tmp/qtest-786453.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-786453.qmp,id=char0 -mon
chardev=char0,mode=control -display none -accel kvm -accel tcg -name
source,debug-threads=on -m 150M -serial
file:/tmp/migration-test-LndGy5/src_serial -drive
file=/tmp/migration-test-LndGy5/bootsect,format=raw -accel qtest
[New LWP 802811]
[New LWP 802813]
[New LWP 802815]
[New LWP 802816]
[New LWP 804033]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
0x7cff2234 in __ppoll (fds=0xd5513040, nfds=5,
timeout=, timeout@entry=0xff698338,
sigmask=sigmask@entry=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:44
44  ../sysdeps/unix/sysv/linux/ppoll.c: No such file or directory.

Thread 6 (Thread 0x01fef560 (LWP 804033)):
#0  futex_abstimed_wait_cancelable (private=0, abstime=0x0, clockid=0,
expected=0, futex_word=0xd563bcd8) at
../sysdeps/nptl/futex-internal.h:320
#1  do_futex_wait (sem=sem@entry=0xd563bcd8, abstime=0x0,
clockid=0) at sem_waitcommon.c:112
#2  0x7d0abdcc in __new_sem_wait_slow
(sem=sem@entry=0xd563bcd8, abstime=0x0, clockid=0) at
sem_waitcommon.c:184
#3  0x7d0abe70 in __new_sem_wait
(sem=sem@entry=0xd563bcd8) at sem_wait.c:42
#4  0xafbc5f5c in qemu_sem_wait (sem=sem@entry=0xd563bcd8)
at ../../util/qemu-thread-posix.c:357
#5  0xaf7a970c in multifd_send_sync_main (f=)
at ../../migration/multifd.c:617
#6  0xaf984244 in ram_save_iterate (f=0xd4795a60,
opaque=) at ../../migration/ram.c:2951
#7  0xaf671294 in qemu_savevm_state_iterate (f=0xd4795a60,
postcopy=postcopy@entry=false) at ../../migration/savevm.c:1296
#8  0xaf75279c in migration_iteration_run (s=0xd4487200)
at ../../migration/migration.c:3576
#9  

Re: [PATCH] fuzz: avoid building twice, when running on gitlab

[Peter Maydell]

On Mon, 9 Aug 2021 at 20:30, Alexander Bulekov  wrote:
>
> On 210809 1506, Alexander Bulekov wrote:
> > On 210809 1925, Peter Maydell wrote:
> > > On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov  wrote:
> > > >
> > > > On oss-fuzz, we build twice, to put together a build that is portable to
> > > > the runner containers. On gitlab ci, this is wasteful and contributes to
> > > > timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at
> > > > the remote cost of potentially missing some cases that break oss-fuzz
> > > > builds.
> > > >
> > > > Signed-off-by: Alexander Bulekov 
> > > > ---
> > > >
> > > > From a couple test runs it looks like this can shave off 15-20 minutes.
> > > >
> > > >  scripts/oss-fuzz/build.sh | 24 +---
> > > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > I tried a test run with this, but it still hit the 1 hour timeout:
> > >
> > > https://gitlab.com/qemu-project/qemu/-/pipelines/350387482
> >
> > It also timed out for me with a 120 minute timeout:
> > https://gitlab.com/a1xndr/qemu/-/jobs/1488160601
> >
> > The log has almost exactly the same number of lines as yours, so I'm
> > guessing one of the qtests is timing out with --enable-sanitizers .

>
> Building locally:
> $ CC=clang-11 CXX=clang++-11 ../configure --enable-fuzzing \
> --enable-debug --enable-sanitizers
> $ make check-qtest-i386 check-unit
>
> Same as on gitlab, this times out shortly after outputting
> "sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found"
>
> Manually running qos-test, the same way check-qtest-i386 invokes it:
>
> $ QTEST_QEMU_BINARY=./qemu-system-i386 
> QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
> tests/qtest/qos-test --tap -k -m quick < /dev/null
>
> # starting vhost-user backend: exec ./storage-daemon/qemu-storage-daemon 
> --blockdev driver=file,node-name=disk0,filename=qtest.XRAzzu --export 
> type=vhost-user-blk,id=disk0,addr.type=unix,addr.path=/tmp/qtest-94561-sock.NdKWpt,node-name=disk0,writable=on,num-queues=1
> sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found
> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-94561.sock 
> -qtest-log /dev/null -chardev socket,path=/tmp/qtest-94561.qmp,id=char0 -mon 
> chardev=char0,mode=control -display none -M pc  -device 
> vhost-user-blk-pci,id=drv0,chardev=char1,addr=4.0 -object 
> memory-backend-memfd,id=mem,size=256M,share=on  -M memory-backend=mem -m 256M 
> -chardev socket id=char1,path=/tmp/qtest-94561-sock.NdKWpt  -accel qtest
>
> *timeout*

vhost-user timing out in realize I suspect. I see that as
an intermittent hang in non-sanitizer configs.

vhost-user folk: Can we either look at fixing this or else disable
the test ? (Stack backtraces available in the other email thread.)

thanks
-- PMM

Re: [RFC PATCH 02/13] target/riscv: Support UXL32 for branch instructions

[Richard Henderson]

On 8/8/21 3:45 PM, LIU Zhiwei wrote:


On 2021/8/6 上午3:06, Richard Henderson wrote:

On 8/4/21 4:53 PM, LIU Zhiwei wrote:

+static TCGv gpr_src_u(DisasContext *ctx, int reg_num)
+{
+    if (reg_num == 0) {
+    return ctx->zero;
+    }
+    if (ctx->uxl32) {
+    tcg_gen_ext32u_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]);
+    }
+    return cpu_gpr[reg_num];
+}
+
+static TCGv gpr_src_s(DisasContext *ctx, int reg_num)
+{
+    if (reg_num == 0) {
+    return ctx->zero;
+    }
+    if (ctx->uxl32) {
+    tcg_gen_ext32s_tl(cpu_gpr[reg_num], cpu_gpr[reg_num]);
+    }
+    return cpu_gpr[reg_num];
+}


This is bad: you cannot modify the source registers like this.


In my opinion, when uxl32, the only meaningful part is the low 32 bits, and it doesn't 
matter to modify the high parts.


Then why does the architecture manual specify that when registers are modified the value 
written sign-extended?  This effect should be visible...






These incorrect modifications will be visible to the kernel on transition back 
to S-mode.


When transition back to S-mode, I think the kernel will save the U-mode 
registers to memory.


... here.  Once we're in S-mode, we have SXLEN, and if SXLEN > UXLEN, the high part of the 
register will be visible.  It really must be either (1) sign-extended because U-mode wrote 
to the register or (2) unmodified from the last time S-mode wrote to the register.



r~

Re: [PATCH] fuzz: avoid building twice, when running on gitlab

[Alexander Bulekov]

On 210809 1506, Alexander Bulekov wrote:
> On 210809 1925, Peter Maydell wrote:
> > On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov  wrote:
> > >
> > > On oss-fuzz, we build twice, to put together a build that is portable to
> > > the runner containers. On gitlab ci, this is wasteful and contributes to
> > > timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at
> > > the remote cost of potentially missing some cases that break oss-fuzz
> > > builds.
> > >
> > > Signed-off-by: Alexander Bulekov 
> > > ---
> > >
> > > From a couple test runs it looks like this can shave off 15-20 minutes.
> > >
> > >  scripts/oss-fuzz/build.sh | 24 +---
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > I tried a test run with this, but it still hit the 1 hour timeout:
> > 
> > https://gitlab.com/qemu-project/qemu/-/pipelines/350387482
> 
> It also timed out for me with a 120 minute timeout:
> https://gitlab.com/a1xndr/qemu/-/jobs/1488160601
> 
> The log has almost exactly the same number of lines as yours, so I'm
> guessing one of the qtests is timing out with --enable-sanitizers .
> 
> -Alex
> 

Building locally:
$ CC=clang-11 CXX=clang++-11 ../configure --enable-fuzzing \
--enable-debug --enable-sanitizers
$ make check-qtest-i386 check-unit

Same as on gitlab, this times out shortly after outputting
"sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found"

Manually running qos-test, the same way check-qtest-i386 invokes it:

$ QTEST_QEMU_BINARY=./qemu-system-i386 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
tests/qtest/qos-test --tap -k -m quick < /dev/null

# starting vhost-user backend: exec ./storage-daemon/qemu-storage-daemon 
--blockdev driver=file,node-name=disk0,filename=qtest.XRAzzu --export 
type=vhost-user-blk,id=disk0,addr.type=unix,addr.path=/tmp/qtest-94561-sock.NdKWpt,node-name=disk0,writable=on,num-queues=1
sh: 1: exec: ./storage-daemon/qemu-storage-daemon: not found
# starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-94561.sock 
-qtest-log /dev/null -chardev socket,path=/tmp/qtest-94561.qmp,id=char0 -mon 
chardev=char0,mode=control -display none -M pc  -device 
vhost-user-blk-pci,id=drv0,chardev=char1,addr=4.0 -object 
memory-backend-memfd,id=mem,size=256M,share=on  -M memory-backend=mem -m 256M 
-chardev socket id=char1,path=/tmp/qtest-94561-sock.NdKWpt  -accel qtest

*timeout*

Ok, lets try to manually build ./storage-daemon/qemu-storage-daemon
$ make ./storage-daemon/qemu-storage-daemon

And rerun the tests...
$ QTEST_QEMU_BINARY=./qemu-system-i386 
QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon 
tests/qtest/qos-test --tap -k -m quick < /dev/null

No timeout... Still not sure why ./storage-daemon/qemu-storage-daemon isn't
being built by make check, and how to fix that.
-Alex

> > 
> > -- PMM

[PATCH] Acceptance tests: add myself as a reviewer for the acceptance tests.

[Willian Rampazzo]

Signed-off-by: Willian Rampazzo 
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 37b1a8e442..3f8ad63165 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3422,6 +3422,7 @@ W: https://trello.com/b/6Qi1pxVn/avocado-qemu
 R: Cleber Rosa 
 R: Philippe Mathieu-Daudé 
 R: Wainer dos Santos Moschetta 
+R: Willian Rampazzo 
 S: Odd Fixes
 F: tests/acceptance/
 
-- 
2.31.1

Re: [PATCH-for-6.2 v3 1/7] target/mips: Introduce generic TRANS() macro for decodetree helpers

[Richard Henderson]

On 8/8/21 7:30 AM, Philippe Mathieu-Daudé wrote:

Plain copy/paste of the TRANS() macro introduced in the PPC
commit f2aabda8ac9 ("target/ppc: Move D/DS/X-form integer
loads to decodetree") to the MIPS target.

Suggested-by: Richard Henderson
Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/tcg/translate.h | 8 
  1 file changed, 8 insertions(+)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH-for-6.2 2/2] target/mips/mxu: Use tcg_constant_i32()

[Richard Henderson]

On 8/8/21 6:08 AM, Philippe Mathieu-Daudé wrote:

Replace uses of tcg_const_i32() with the allocate and free close together.

Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/tcg/mxu_translate.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)


In my opinion these functions should be simplified by dropping all of the special casing 
for the zero register and simply let the tcg optimizer do its thing.


But failing that, this is fine.
Reviewed-by: Richard Henderson 


r~

Re: [PATCH-for-6.2 1/2] target/mips/tx79: Use tcg_constant_tl()

[Richard Henderson]

On 8/8/21 6:08 AM, Philippe Mathieu-Daudé wrote:

Replace uses of tcg_const_tl() with the allocate and free close together.

Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/tcg/tx79_translate.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)


Reviewed-by: Richard Henderson 

r~

Re: [PATCH v3 10/10] virtiofsd: Add lazy lo_do_find()

[Vivek Goyal]

On Fri, Jul 30, 2021 at 05:01:34PM +0200, Max Reitz wrote:
> lo_find() right now takes two lookup keys for two maps, namely the file
> handle for inodes_by_handle and the statx information for inodes_by_ids.
> However, we only need the statx information if looking up the inode by
> the file handle failed.
> 
> There are two callers of lo_find(): The first one, lo_do_lookup(), has
> both keys anyway, so passing them does not incur any additional cost.
> The second one, lookup_name(), though, needs to explicitly invoke
> name_to_handle_at() (through get_file_handle()) and statx() (through
> do_statx()).  We need to try to get a file handle as the primary key, so
> we cannot get rid of get_file_handle(), but we only need the statx
> information if looking up an inode by handle failed; so we can defer
> that until the lookup has indeed failed.

So IIUC, this patch seems to be all about avoiding do_statx()
call in lookup_name() if file handle could be successfully
generated.

So can't we just not modify lookup_name() to not call statx()
if file handle could be generated. And also modfiy lo_find()
to use st/mnt_id only if fhandle==NULL.

That probably is much simpler change as compared to passing function
pointers around.

Vivek

> 
> To this end, replace lo_find()'s st/mnt_id parameters by a get_ids()
> closure that is invoked to fill the lo_key struct if necessary.
> 
> Also, lo_find() is renamed to lo_do_find(), so we can add a new
> lo_find() wrapper whose closure just initializes the lo_key from the
> st/mnt_id parameters, just like the old lo_find() did.
> 
> lookup_name() directly calls lo_do_find() now and passes its own
> closure, which performs the do_statx() call.
> 
> Signed-off-by: Max Reitz 
> ---
>  tools/virtiofsd/passthrough_ll.c | 93 ++--
>  1 file changed, 76 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index ac95961d12..41e9f53878 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1200,22 +1200,23 @@ out_err:
>  fuse_reply_err(req, saverr);
>  }
>  
> -static struct lo_inode *lo_find(struct lo_data *lo,
> -const struct lo_fhandle *fhandle,
> -struct stat *st, uint64_t mnt_id)
> +/*
> + * get_ids() will be called to get the key for lo->inodes_by_ids if
> + * the lookup by file handle has failed.
> + */
> +static struct lo_inode *lo_do_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +int (*get_ids)(struct lo_key *, const void *),
> +const void *get_ids_opaque)
>  {
>  struct lo_inode *p = NULL;
> -struct lo_key ids_key = {
> -.ino = st->st_ino,
> -.dev = st->st_dev,
> -.mnt_id = mnt_id,
> -};
> +struct lo_key ids_key;
>  
>  pthread_mutex_lock(>mutex);
>  if (fhandle) {
>  p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
>  }
> -if (!p) {
> +if (!p && get_ids(_key, get_ids_opaque) == 0) {
>  p = g_hash_table_lookup(lo->inodes_by_ids, _key);
>  /*
>   * When we had to fall back to looking up an inode by its
> @@ -1244,6 +1245,36 @@ static struct lo_inode *lo_find(struct lo_data *lo,
>  return p;
>  }
>  
> +struct lo_find_get_ids_key_opaque {
> +const struct stat *st;
> +uint64_t mnt_id;
> +};
> +
> +static int lo_find_get_ids_key(struct lo_key *ids_key, const void *opaque)
> +{
> +const struct lo_find_get_ids_key_opaque *stat_info = opaque;
> +
> +*ids_key = (struct lo_key){
> +.ino = stat_info->st->st_ino,
> +.dev = stat_info->st->st_dev,
> +.mnt_id = stat_info->mnt_id,
> +};
> +
> +return 0;
> +}
> +
> +static struct lo_inode *lo_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +struct stat *st, uint64_t mnt_id)
> +{
> +const struct lo_find_get_ids_key_opaque stat_info = {
> +.st = st,
> +.mnt_id = mnt_id,
> +};
> +
> +return lo_do_find(lo, fhandle, lo_find_get_ids_key, _info);
> +}
> +
>  /* value_destroy_func for posix_locks GHashTable */
>  static void posix_locks_value_destroy(gpointer data)
>  {
> @@ -1769,14 +1800,41 @@ out_err:
>  fuse_reply_err(req, saverr);
>  }
>  
> +struct lookup_name_get_ids_key_opaque {
> +struct lo_data *lo;
> +int parent_fd;
> +const char *name;
> +};
> +
> +static int lookup_name_get_ids_key(struct lo_key *ids_key, const void 
> *opaque)
> +{
> +const struct lookup_name_get_ids_key_opaque *stat_params = opaque;
> +uint64_t mnt_id;
> +struct stat attr;
> +int res;
> +
> +res = do_statx(stat_params->lo, stat_params->parent_fd, 
> stat_params->name,
> +   , AT_SYMLINK_NOFOLLOW, _id);
> +if (res < 0) {
> +return -errno;
> +}
> +
> +*ids_key = (struct lo_key){
> +.ino = attr.st_ino,
> +  

Re: [PATCH] fuzz: avoid building twice, when running on gitlab

[Alexander Bulekov]

On 210809 1925, Peter Maydell wrote:
> On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov  wrote:
> >
> > On oss-fuzz, we build twice, to put together a build that is portable to
> > the runner containers. On gitlab ci, this is wasteful and contributes to
> > timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at
> > the remote cost of potentially missing some cases that break oss-fuzz
> > builds.
> >
> > Signed-off-by: Alexander Bulekov 
> > ---
> >
> > From a couple test runs it looks like this can shave off 15-20 minutes.
> >
> >  scripts/oss-fuzz/build.sh | 24 +---
> >  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> I tried a test run with this, but it still hit the 1 hour timeout:
> 
> https://gitlab.com/qemu-project/qemu/-/pipelines/350387482

It also timed out for me with a 120 minute timeout:
https://gitlab.com/a1xndr/qemu/-/jobs/1488160601

The log has almost exactly the same number of lines as yours, so I'm
guessing one of the qtests is timing out with --enable-sanitizers .

-Alex

> 
> -- PMM

Re: [PATCH for-6.2 v6 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Daniel Henrique Barboza]

On 8/7/21 10:38 AM, Markus Armbruster wrote:

I apologize for the not reviewing this promptly.

Daniel Henrique Barboza  writes:


qapi_event_send_mem_unplug_error() deals with @device being NULL by
replacing it with an empty string ("") when emitting the event. Aside
from the fact that this is a side effect that can be patched someday,


I guess by "side effect" you allude to the output visitor's misfeature
to map null pointer to "".


there's also the lack of utility that the event brings to listeners,
e.g.  "a memory unplug error happened somewhere".


True.


We're better of not emitting the event if dev->id is NULL.


On a green field, yes.  But is it worth an incompatible change now?  I
doubt it.


Next patches
will introduce a new device unplug error event that is better suited to
deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be
emitted to avoid breaking existing APIs, but it'll be deprecated and
removed in the future.

Suggested-by: Markus Armbruster 
Reviewed-by: Greg Kurz 
Signed-off-by: Daniel Henrique Barboza 
---
  hw/acpi/memory_hotplug.c | 9 +++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..e37acb0367 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -177,9 +177,14 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
  /* call pc-dimm unplug cb */
  hotplug_handler_unplug(hotplug_ctrl, dev, _err);
  if (local_err) {
+const char *error_pretty = error_get_pretty(local_err);
+
  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
-qapi_event_send_mem_unplug_error(dev->id,
- error_get_pretty(local_err));
+
+if (dev->id) {
+qapi_event_send_mem_unplug_error(dev->id, error_pretty);
+}
+
  error_free(local_err);
  break;
  }


Three options:

1. Make the incompatible change.  Keep this patch.  Needs a release
note.

2. Continue to rely on the output visitor's misfeature.  Drop this
patch.

Relying on the misfeature is best avoided, at least in new code.

3. Make the mapping explicit here, and avoid relying on the misfeature.
Something like the appended patch.


I like (3) more. If no one opposes I'll respin patches 1 and 2 with this
approach.


Daniel



I don't like 1.  I think the choice between 2. and 3. depends on how it
fits with the remainder of this series.


diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..89c411dd5c 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -178,7 +178,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr 
addr, uint64_t data,
  hotplug_handler_unplug(hotplug_ctrl, dev, _err);
  if (local_err) {
  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
-qapi_event_send_mem_unplug_error(dev->id,
+qapi_event_send_mem_unplug_error(dev->id ?: "",
   error_get_pretty(local_err));
  error_free(local_err);
  break;

Re: [PATCH for-6.2 v6 6/7] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors

[Daniel Henrique Barboza]

On 8/7/21 11:06 AM, Markus Armbruster wrote:

Daniel Henrique Barboza  writes:


Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
error path, signalling that the hotunplug process wasn't successful.
This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
to signal this error to the management layer.

We also have another error path in spapr_memory_unplug_rollback() for
configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
in the hotunplug error path, but it will reconfigure them. Let's send
the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
case of older kernels.

Reviewed-by: Greg Kurz 
Signed-off-by: Daniel Henrique Barboza 
---
  hw/ppc/spapr.c |  9 -
  hw/ppc/spapr_drc.c | 18 --
  2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1611d7ab05..5459f9a7e9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -29,6 +29,7 @@
  #include "qemu/datadir.h"
  #include "qapi/error.h"
  #include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-qdev.h"
  #include "qapi/visitor.h"
  #include "sysemu/sysemu.h"
  #include "sysemu/hostmem.h"
@@ -3686,13 +3687,19 @@ void spapr_memory_unplug_rollback(SpaprMachineState 
*spapr, DeviceState *dev)
  
  /*

   * Tell QAPI that something happened and the memory
- * hotunplug wasn't successful.
+ * hotunplug wasn't successful. Keep sending
+ * MEM_UNPLUG_ERROR even while sending DEVICE_UNPLUG_ERROR
+ * until the deprecation MEM_UNPLUG_ERROR is due.
   */
  if (dev->id) {
  qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest "
   "for device %s", dev->id);
  qapi_event_send_mem_unplug_error(dev->id, qapi_error);
  }
+
+qapi_event_send_device_unplug_error(!!dev->id, dev->id,
+dev->canonical_path,
+qapi_error != NULL, qapi_error);
  }
  


When dev->id is null, we send something like

 {"event": "DEVICE_UNPLUG_ERROR",
  "data": {"path": "/machine/..."},
  "timestamp": ...}

Unless I'm missing something, this is all the information the management
application really needs.

When dev->id is non-null, we add to "data":

   "device": "dev123",
   "msg": "Memory hotunplug rejected by the guest for device 
dev123",

I'm fine with emitting the device ID when we have it.

What's the intended use of "msg"?

Could DEVICE_UNPLUG_ERROR ever be emitted for this device with a
different "msg"?



It won't have a different 'msg' for the current use of the event in both ppc64
and x86. It'll always be the same ' hotunplug rejected by the guest'
message.

The idea is that a future caller might want to insert a more informative
message, such as "hotunplug failed: memory is being used by kernel space"
or any other more specific condition. But then I guess we can argue that,
if that time comes, one can just add this new optional 'msg' member in this
event, and for now we can live without it.

Would you oppose to renaming this new event to "DEVICE_UNPLUG_GUEST_ERROR"
and then remove the 'msg' member? I guess this rename would make it clearer
for management that we're reporting a guest side error, making any further
clarifications via 'msg' unneeded.


Thanks,


Daniel






If "msg" is useful when dev->id is non-null, then it's likely useful
when dev->id is null.  Why not

   "msg": "Memory hotunplug rejected by the guest",

always?

If we do that here, we'll likely do it everywhere, and then member @msg
isn't actually optional.


  /* Callback to be called during DRC release. */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a4d9496f76..8f0479631f 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -17,6 +17,8 @@
  #include "hw/ppc/spapr_drc.h"
  #include "qom/object.h"
  #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "qapi/qapi-events-qdev.h"
  #include "qapi/visitor.h"
  #include "qemu/error-report.h"
  #include "hw/ppc/spapr.h" /* for RTAS return codes */
@@ -160,6 +162,11 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
   * means that the kernel is refusing the removal.
   */
  if (drc->unplug_requested && drc->dev) {
+const char qapi_error_fmt[] = \


Drop the superfluous \


+"Device hotunplug rejected by the guest for device %s";


Unusual indentation.


+
+g_autofree char *qapi_error = NULL;
+
  if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
  spapr = SPAPR_MACHINE(qdev_get_machine());
  
@@ -169,14 +176,13 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)

  drc->unplug_requested = false;
  
  if (drc->dev->id) {

-error_report("Device hotunplug rejected by the guest "
- "for device %s", 

Re: [PULL 0/2] Block patches for 6.1-rc3

[Peter Maydell]

On Mon, 9 Aug 2021 at 18:03, Hanna Reitz  wrote:
>
> Hi Peter,
>
> Let me prefix this by saying that it's me, Max.  I've changed my name
> and email address.  I understand freeze may not be the best of times for
> this, but it looks like I can no longer send mails from the mreitz@
> address for now (only receive them).
>
> I've tried to create and sign the tag as Max, so I hope this pull
> request won't run into any issues from that perspective.
>
> (For the future, I'll create a new key and hope signing it with my old
> key will make it sufficiently trustworthy...)

Yep, that's fine. This pullreq is going through OK (will send the
usual applied/errors email once the builds have completed). I'm happy
to treat signed-by-old-key as trusted enough. (In fact we already
are working basically on trust-on-first-use for new keys, but if
you're OK with signing your new key with the old one that does
simplify things a bit.)

Do you plan to send a patch to MAINTAINERS ? It has some
mreitz@redhat lines in it currently.

thanks
-- PMM

Re: [PATCH v3 09/10] virtiofsd: Optionally fill lo_inode.fhandle

[Vivek Goyal]

On Fri, Jul 30, 2021 at 05:01:33PM +0200, Max Reitz wrote:
> When the inode_file_handles option is set, try to generate a file handle
> for new inodes instead of opening an O_PATH FD.
> 
> Being able to open these again will require CAP_DAC_READ_SEARCH, so the
> description text tells the user they will also need to specify
> -o modcaps=+dac_read_search.
> 
> Generating a file handle returns the mount ID it is valid for.  Opening
> it will require an FD instead.  We have mount_fds to map an ID to an FD.
> get_file_handle() fills the hash map by opening the file we have
> generated a handle for.  To verify that the resulting FD indeed
> represents the handle's mount ID, we use statx().  Therefore, using file
> handles requires statx() support.

So opening the file and storing that fd in mount_fds table might be
a potential problem with inotify work Ioannis is doing.

So say a file foo.txt was opened O_RDONLY and fd stored in mount_fs. Now
say user unlinks foo.txt. If notifications are enabled, final notification
will not be generated till this mount_fds fd is closed.

Now question is when will this fd be closed? If it closed at some
later point and then notification is generated, that will break
notificaitons.

In fact even O_PATH fd is delaying notifications due to same reason.
But its not too bad as we close O_PATH fd pretty quickly after
unlinking. And we were hoping that file handle support will get rid
of this problem because we will not keep O_PATH fd open.

But, IIUC, mount_fds stuff will make it even worse. I did not see 
the code which removes this fd from mount_fds. So I am not sure what's
the life time of this fd.

Thanks
Vivek

> 
> Signed-off-by: Max Reitz 
> ---
>  tools/virtiofsd/helper.c  |   3 +
>  tools/virtiofsd/passthrough_ll.c  | 194 --
>  tools/virtiofsd/passthrough_seccomp.c |   1 +
>  3 files changed, 190 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/virtiofsd/helper.c b/tools/virtiofsd/helper.c
> index a8295d975a..aa63a21d43 100644
> --- a/tools/virtiofsd/helper.c
> +++ b/tools/virtiofsd/helper.c
> @@ -187,6 +187,9 @@ void fuse_cmdline_help(void)
> "   default: no_allow_direct_io\n"
> "-o announce_submounts  Announce sub-mount points to the 
> guest\n"
> "-o posix_acl/no_posix_acl  Enable/Disable posix_acl. 
> (default: disabled)\n"
> +   "-o inode_file_handles  Use file handles to reference 
> inodes\n"
> +   "   instead of O_PATH file 
> descriptors\n"
> +   "   (requires -o 
> modcaps=+dac_read_search)\n"
> );
>  }
>  
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index f9d8b2f134..ac95961d12 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -194,6 +194,7 @@ struct lo_data {
>  /* If set, virtiofsd is responsible for setting umask during creation */
>  bool change_umask;
>  int user_posix_acl, posix_acl;
> +int inode_file_handles;
>  };
>  
>  /**
> @@ -250,6 +251,10 @@ static const struct fuse_opt lo_opts[] = {
>  { "no_killpriv_v2", offsetof(struct lo_data, user_killpriv_v2), 0 },
>  { "posix_acl", offsetof(struct lo_data, user_posix_acl), 1 },
>  { "no_posix_acl", offsetof(struct lo_data, user_posix_acl), 0 },
> +{ "inode_file_handles", offsetof(struct lo_data, inode_file_handles), 1 
> },
> +{ "no_inode_file_handles",
> +  offsetof(struct lo_data, inode_file_handles),
> +  0 },
>  FUSE_OPT_END
>  };
>  static bool use_syslog = false;
> @@ -321,6 +326,135 @@ static int temp_fd_steal(TempFd *temp_fd)
>  }
>  }
>  
> +/**
> + * Generate a file handle for the given dirfd/name combination.
> + *
> + * If mount_fds does not yet contain an entry for the handle's mount
> + * ID, (re)open dirfd/name in O_RDONLY mode and add it to mount_fds
> + * as the FD for that mount ID.  (That is the file that we have
> + * generated a handle for, so it should be representative for the
> + * mount ID.  However, to be sure (and to rule out races), we use
> + * statx() to verify that our assumption is correct.)
> + */
> +static struct lo_fhandle *get_file_handle(struct lo_data *lo,
> +  int dirfd, const char *name)
> +{
> +/* We need statx() to verify the mount ID */
> +#if defined(CONFIG_STATX) && defined(STATX_MNT_ID)
> +struct lo_fhandle *fh;
> +int ret;
> +
> +if (!lo->use_statx || !lo->inode_file_handles) {
> +return NULL;
> +}
> +
> +fh = g_new0(struct lo_fhandle, 1);
> +
> +fh->handle.handle_bytes = sizeof(fh->padding) - sizeof(fh->handle);
> +ret = name_to_handle_at(dirfd, name, >handle, >mount_id,
> +AT_EMPTY_PATH);
> +if (ret < 0) {
> +goto fail;
> +}
> +
> +if (pthread_rwlock_rdlock(_fds_lock)) {
> +

Re: [PATCH] fuzz: avoid building twice, when running on gitlab

[Peter Maydell]

On Mon, 9 Aug 2021 at 12:18, Alexander Bulekov  wrote:
>
> On oss-fuzz, we build twice, to put together a build that is portable to
> the runner containers. On gitlab ci, this is wasteful and contributes to
> timeouts on the build-oss-fuzz job. Avoid building twice on gitlab, at
> the remote cost of potentially missing some cases that break oss-fuzz
> builds.
>
> Signed-off-by: Alexander Bulekov 
> ---
>
> From a couple test runs it looks like this can shave off 15-20 minutes.
>
>  scripts/oss-fuzz/build.sh | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)

I tried a test run with this, but it still hit the 1 hour timeout:

https://gitlab.com/qemu-project/qemu/-/pipelines/350387482

-- PMM

Re: [PATCH for 6.2 23/49] bsd-user: pull in target_arch_thread.h update target_arch_elf.h

[Richard Henderson]

On 8/8/21 11:43 AM, Warner Losh wrote:

BTW, I've started to notice that many of the
items flagged by the style commitcheck.pl  script originated in the 
linux-user

code and it's still that way today...  Do you have any advice for what I should
do about that, if anything?


Fix em on the bsd side.

We have so far resisted style fixes for their own sake, only fixing them up locally when 
we have to make nearby changes for some other reason.



r~

[PATCH] docs/devel: fix missing antislash

[Alexandre Iooss]

Signed-off-by: Alexandre Iooss 
---
 docs/devel/qom.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index e5fe3597cd..b9568c0fb8 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -309,7 +309,7 @@ This is equivalent to the following:
OBJECT_GET_CLASS(MyDeviceClass, obj, TYPE_MY_DEVICE)
#define MY_DEVICE_CLASS(void *klass) \
OBJECT_CLASS_CHECK(MyDeviceClass, klass, TYPE_MY_DEVICE)
-   #define MY_DEVICE(void *obj)
+   #define MY_DEVICE(void *obj) \
OBJECT_CHECK(MyDevice, obj, TYPE_MY_DEVICE)
 
struct MyDeviceClass {
-- 
2.31.1

[PATCH v2 0/1] virtio: failover: allow to keep the VFIO device rather than the virtio-net one

[Laurent Vivier]

v2: use validate_features() to disable the guest driver rather
than setting vring.num to 0.

With failover, when the guest virtio-net driver doesn't support the
STANDBY feature, the primary device is not plugged and only the virtio-net
device is kept. Doing like that we can migrate the machine and
keep the network connection.

But in some cases, when performance is more important than availability
we would prefer to keep the VFIO device rather than the virtio-net one,
even if it means to lose the network connection during the migration of
the machine.

To do that we can't simply unplug the virtio-net device and plug the
VFIO one because for the migration the initial state must be kept
(virtio-net plugged, VFIO unplugged) but we can try to disable the
virtio-net driver and plug the VFIO card, so the initial state is
correct (the virtio-net card is plugged, but disabled in guest, and
the VFIO card is unplugged before migration).

This change doesn't impact the case when guest and host support
the STANDBY feature.

I've introduced the "failover-default" property to virtio-net device
to set which device to keep (failover-default=true keeps the virtio-net
device, =off the other one).

For example, with a guest driver that doesn't support STANDBY:

  ...
  -device virtio-net-pci,id=virtio0,failover=on,failover-default=on \
  -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
  ...

  [root@localhost ~]# ip a
  1: lo:  mtu 65536 qdisc noqueue state UNKNOWN qlen 1
  link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
  inet 127.0.0.1/8 scope host lo
 valid_lft forever preferred_lft forever
  inet6 ::1/128 scope host
 valid_lft forever preferred_lft forever
  2: eth0:  mtu 1500 qdisc pfifo_fast state UP 
q0
  link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  inet 192.168.20.2/24 brd 192.168.20.255 scope global eth0
 valid_lft forever preferred_lft forever
  inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
 valid_lft forever preferred_lft forever
  # ethtool -i eth0
  driver: virtio_net
  version: 1.0.0
  firmware-version:
  expansion-rom-version:
  bus-info: :04:00.0
  supports-statistics: no
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no

  ...
  -device virtio-net-pci,id=virtio0,failover=on,failover-default=off \
  -device vfio-pci,host=$PCI,id=hostdev0,failover_pair_id=virtio0 \
  ...

  [root@localhost ~]# ip a
  1: lo:  mtu 65536 qdisc noqueue state UNKNOWN qlen 1
  link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
  inet 127.0.0.1/8 scope host lo
 valid_lft forever preferred_lft forever
  inet6 ::1/128 scope host
 valid_lft forever preferred_lft forever
  2: enp2s0:  mtu 1500 qdisc mq state UP qlen 
100
  link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  inet 192.168.20.2/24 brd 192.168.20.255 scope global enp2s0
 valid_lft forever preferred_lft forever
  inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
 valid_lft forever preferred_lft forever
  [root@localhost ~]# ethtool -i enp2s0
  driver: i40evf
  version: 1.6.27-k
  firmware-version: N/A
  expansion-rom-version:
  bus-info: :02:00.0
  supports-statistics: yes
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no

With guest driver that supports STANDBY, we would always have:

  [root@localhost ~]# ip a
  1: lo:  mtu 65536 qdisc noqueue state UNKNOWN group 
defau0
  link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
  inet 127.0.0.1/8 scope host lo
 valid_lft forever preferred_lft forever
  inet6 ::1/128 scope host
 valid_lft forever preferred_lft forever
  2: enp4s0:  mtu 1500 qdisc noqueue state UP 
gr0
  link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  inet 192.168.20.2/24 brd 192.168.20.255 scope global noprefixroute enp4s0
 valid_lft forever preferred_lft forever
  inet6 fe80::2428:c5ff:fe7f:1424/64 scope link
 valid_lft forever preferred_lft forever
  3: enp4s0nsby:  mtu 1500 qdisc fq_codel 
master0
  link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  4: enp2s0:  mtu 1500 qdisc mq master enp4s0 
st0
  link/ether 26:28:c5:7f:14:24 brd ff:ff:ff:ff:ff:ff
  [root@localhost ~]# ethtool -i enp4s0
  driver: net_failover
  version: 0.1
  firmware-version:
  expansion-rom-version:
  bus-info:
  supports-statistics: no
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no
  [root@localhost ~]# ethtool -i enp4s0nsby
  driver: virtio_net
  version: 1.0.0
  firmware-version:
  expansion-rom-version:
  bus-info: :04:00.0
  supports-statistics: yes
  supports-test: no
  supports-eeprom-access: no
  supports-register-dump: no
  supports-priv-flags: no
  [root@localhost ~]# ethtool -i enp2s0
  driver: iavf
  version: 4.18.0-310.el8.x86_64
  firmware-version: N/A
  expansion-rom-version:
  

[PATCH v2 1/1] virtio: failover: define the default device to use in case of error

[Laurent Vivier]

If the guest driver doesn't support the STANDBY feature, by default
we keep the virtio-net device and don't hotplug the VFIO device,
but in some cases, user can prefer to use the VFIO device rather
than the virtio-net one. We can't unplug the virtio-net device
(because on migration it is expected on the destination side)
but we can force the guest driver to be disabled. Then, we can
hotplug the VFIO device that will be unplugged before the migration
like in the normal failover migration but without the failover device.

This patch adds a new property to virtio-net device: "failover-default".

By default, "failover-default" is set to true and thus the default NIC
to use if the failover cannot be enabled is the virtio-net device
(this is what is done until now with the virtio-net failover).

If "failover-default" is set to false, in case of error, the virtio-net
device is not the default anymore and the failover primary device
is used instead.

If the STANDBY feature is supported by guest and host, the virtio-net
failover acts as usual.

Signed-off-by: Laurent Vivier 
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c| 49 +-
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f06..ab77930a327e 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -208,6 +208,7 @@ struct VirtIONet {
 /* primary failover device is hidden*/
 bool failover_primary_hidden;
 bool failover;
+bool failover_default;
 DeviceListener primary_listener;
 Notifier migration_state;
 VirtioNetRssData rss_data;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 16d20cdee52a..972c03232a96 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -935,12 +935,23 @@ static void virtio_net_set_features(VirtIODevice *vdev, 
uint64_t features)
 memset(n->vlans, 0xff, MAX_VLAN >> 3);
 }
 
-if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY)) {
-qapi_event_send_failover_negotiated(n->netclient_name);
-qatomic_set(>failover_primary_hidden, false);
-failover_add_primary(n, );
-if (err) {
-warn_report_err(err);
+/*
+ * if the virtio-net driver has the STANDBY feature, we can plug the 
primary
+ * if not but is not the default failover device,
+ * we need to plug the primary alone and the virtio-net driver will
+ * be disabled in the validate_features() function but validate_features()
+ * is only available with virtio 1.0 spec
+ */
+if (n->failover) {
+if (virtio_has_feature(features, VIRTIO_NET_F_STANDBY) ||
+   (virtio_has_feature(features, VIRTIO_F_VERSION_1) &&
+!n->failover_default)) {
+qapi_event_send_failover_negotiated(n->netclient_name);
+qatomic_set(>failover_primary_hidden, false);
+failover_add_primary(n, );
+if (err) {
+warn_report_err(err);
+}
 }
 }
 }
@@ -3625,9 +3636,34 @@ static Property virtio_net_properties[] = {
 DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
 DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
 DEFINE_PROP_BOOL("failover", VirtIONet, failover, false),
+DEFINE_PROP_BOOL("failover-default", VirtIONet, failover_default, true),
 DEFINE_PROP_END_OF_LIST(),
 };
 
+/* validate_features() is only available with VIRTIO_F_VERSION_1 */
+static int failover_validate_features(VirtIODevice *vdev)
+{
+VirtIONet *n = VIRTIO_NET(vdev);
+
+/*
+ * If the guest driver doesn't support the STANDBY feature, by default
+ * we keep the virtio-net device and don't hotplug the VFIO device,
+ * but in some cases, user can prefer to use the VFIO device rather
+ * than the virtio-net one. We can't unplug the virtio-net device
+ * (because on migration it is expected on the destination side)
+ * but we can force the guest driver to be disabled. In this case, We can
+ * hotplug the VFIO device that will be unplugged before the migration
+ * like in the normal failover migration but without the failover device.
+ */
+if (n->failover && !n->failover_default &&
+!virtio_vdev_has_feature(vdev, VIRTIO_NET_F_STANDBY)) {
+/* disable virtio-net */
+return -ENODEV;
+}
+
+return 0;
+}
+
 static void virtio_net_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
@@ -3651,6 +3687,7 @@ static void virtio_net_class_init(ObjectClass *klass, 
void *data)
 vdc->post_load = virtio_net_post_load_virtio;
 vdc->vmsd = _virtio_net_device;
 vdc->primary_unplug_pending = primary_unplug_pending;
+vdc->validate_features = failover_validate_features;
 }
 
 static const TypeInfo virtio_net_info = {
-- 
2.31.1

Re: [PATCH] audio: Never send migration section

[Daniel P . Berrangé]

On Mon, Aug 09, 2021 at 06:09:56PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> The audio migration vmstate is empty, and always has been; we can't
> just remove it though because an old qemu might send it us.
> Changes with -audiodev now mean it's sometimes created when it didn't
> used to be, and can confuse migration to old qemu.
> 
> Change it so that vmstate_audio is never sent; if it's received it
> should still be accepted, and old qemu's shouldn't be too upset if it's
> missing.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> ---
>  audio/audio.c | 10 ++
>  1 file changed, 10 insertions(+)

Reviewed-by: Daniel P. Berrangé 
Tested-by: Daniel P. Berrangé 


For testing I have a VM with -audiodev, but *WITHOUT* any sound
frontend devices. Live migrating to a QEMU using QEMU_AUDIO_DRV
would previously fail. With this applied it now works, showing
that we dont uncessarily send this.

I also tested migration to a QEMU with -audiodev, but without
this patch, and that still works as before, showing that QEMU
is happy if this section is not sent.

> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 59453ef856..54a153c0ef 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1622,10 +1622,20 @@ void audio_cleanup(void)
>  }
>  }
>  
> +static bool vmstate_audio_needed(void *opaque)
> +{
> +/*
> + * Never needed, this vmstate only exists in case
> + * an old qemu sends it to us.
> + */
> +return false;
> +}
> +
>  static const VMStateDescription vmstate_audio = {
>  .name = "audio",
>  .version_id = 1,
>  .minimum_version_id = 1,
> +.needed = vmstate_audio_needed,
>  .fields = (VMStateField[]) {
>  VMSTATE_END_OF_LIST()
>  }
> -- 
> 2.31.1
> 

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 :|

[PATCH] audio: Never send migration section

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" 

The audio migration vmstate is empty, and always has been; we can't
just remove it though because an old qemu might send it us.
Changes with -audiodev now mean it's sometimes created when it didn't
used to be, and can confuse migration to old qemu.

Change it so that vmstate_audio is never sent; if it's received it
should still be accepted, and old qemu's shouldn't be too upset if it's
missing.

Signed-off-by: Dr. David Alan Gilbert 
---
 audio/audio.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index 59453ef856..54a153c0ef 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1622,10 +1622,20 @@ void audio_cleanup(void)
 }
 }
 
+static bool vmstate_audio_needed(void *opaque)
+{
+/*
+ * Never needed, this vmstate only exists in case
+ * an old qemu sends it to us.
+ */
+return false;
+}
+
 static const VMStateDescription vmstate_audio = {
 .name = "audio",
 .version_id = 1,
 .minimum_version_id = 1,
+.needed = vmstate_audio_needed,
 .fields = (VMStateField[]) {
 VMSTATE_END_OF_LIST()
 }
-- 
2.31.1

[PULL 0/2] Block patches for 6.1-rc3

[Hanna Reitz]

Hi Peter,

Let me prefix this by saying that it's me, Max.  I've changed my name
and email address.  I understand freeze may not be the best of times for
this, but it looks like I can no longer send mails from the mreitz@
address for now (only receive them).

I've tried to create and sign the tag as Max, so I hope this pull
request won't run into any issues from that perspective.

(For the future, I'll create a new key and hope signing it with my old
key will make it sufficiently trustworthy...)


The following changes since commit 632eda54043d6f26ff87dac16233e14b4708b967:

  Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into 
staging (2021-08-09 11:04:27 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2021-08-09

for you to fetch changes up to a6d2bb25cf945cd16f29a575055c6f1a1f9cf6c9:

  tests: filter out TLS distinguished name in certificate checks (2021-08-09 
17:32:43 +0200)


Block patches for 6.1-rc3:
- Build fix for FUSE block exports
- iotest 233 fix


Daniel P. Berrangé (1):
  tests: filter out TLS distinguished name in certificate checks

Fabrice Fontaine (1):
  block/export/fuse.c: fix musl build

 block/export/fuse.c  | 8 ++--
 tests/qemu-iotests/233   | 2 +-
 tests/qemu-iotests/233.out   | 4 ++--
 tests/qemu-iotests/common.filter | 5 +
 4 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.31.1

[PULL 2/2] tests: filter out TLS distinguished name in certificate checks

[Hanna Reitz]

From: Daniel P. Berrangé 

The version of GNUTLS in Fedora 34 has changed the order in which encodes
fields when generating new TLS certificates. This in turn changes the
order seen when querying the distinguished name. This ultimately breaks
the expected output in the NBD TLS iotests. We don't need to be
comparing the exact distinguished name text for the purpose of the test
though, so it is fine to filter it out.

Reported-by: Eric Blake 
Signed-off-by: Daniel P. Berrangé 
Message-Id: <20210804180330.3469683-1-berra...@redhat.com>
Reviewed-by: Eric Blake 
Tested-by: Eric Blake 
Signed-off-by: Hanna Reitz 
---
 tests/qemu-iotests/233   | 2 +-
 tests/qemu-iotests/233.out   | 4 ++--
 tests/qemu-iotests/common.filter | 5 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index da150cd27b..9ca7b68f42 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -148,7 +148,7 @@ $QEMU_IMG info --image-opts \
 
 echo
 echo "== final server log =="
-cat "$TEST_DIR/server.log"
+cat "$TEST_DIR/server.log" | _filter_authz_check_tls
 rm -f "$TEST_DIR/server.log"
 
 # success, all done
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index c3c344811b..4b1f6a0e15 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -65,6 +65,6 @@ qemu-img: Could not open 
'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': F
 == final server log ==
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
 qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
-qemu-nbd: option negotiation failed: TLS x509 authz check for 
CN=localhost,O=Cthulhu Dark Lord Enterprises client1,L=R'lyeh,C=South Pacific 
is denied
-qemu-nbd: option negotiation failed: TLS x509 authz check for 
CN=localhost,O=Cthulhu Dark Lord Enterprises client3,L=R'lyeh,C=South Pacific 
is denied
+qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
+qemu-nbd: option negotiation failed: TLS x509 authz check for 
DISTINGUISHED-NAME is denied
 *** done
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 268b749e2f..2b2b53946c 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -332,5 +332,10 @@ for fname in fnames:
 sys.stdout.write(result)'
 }
 
+_filter_authz_check_tls()
+{
+$SED -e 's/TLS x509 authz check for .* is denied/TLS x509 authz check for 
DISTINGUISHED-NAME is denied/'
+}
+
 # make sure this script returns success
 true
-- 
2.31.1

[PULL 1/2] block/export/fuse.c: fix musl build

[Hanna Reitz]

From: Fabrice Fontaine 

Fix the following build failure on musl raised since version 6.0.0 and
https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
because musl does not define FALLOC_FL_ZERO_RANGE:

../block/export/fuse.c: In function 'fuse_fallocate':
../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared (first 
use in this function)
  563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
  |   ^~~~

Fixes:
 - 
http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64

Signed-off-by: Fabrice Fontaine 
Message-Id: <20210809095101.1101336-1-fontaine.fabr...@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Denis V. Lunev 
Signed-off-by: Hanna Reitz 
---
 block/export/fuse.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index ada9e263eb..fc7b07d2b5 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -635,7 +635,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t 
inode, int mode,
 offset += size;
 length -= size;
 } while (ret == 0 && length > 0);
-} else if (mode & FALLOC_FL_ZERO_RANGE) {
+}
+#ifdef CONFIG_FALLOCATE_ZERO_RANGE
+else if (mode & FALLOC_FL_ZERO_RANGE) {
 if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length > blk_len) {
 /* No need for zeroes, we are going to write them ourselves */
 ret = fuse_do_truncate(exp, offset + length, false,
@@ -654,7 +656,9 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t 
inode, int mode,
 offset += size;
 length -= size;
 } while (ret == 0 && length > 0);
-} else if (!mode) {
+}
+#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
+else if (!mode) {
 /* We can only fallocate at the EOF with a truncate */
 if (offset < blk_len) {
 fuse_reply_err(req, EOPNOTSUPP);
-- 
2.31.1

Re: [PATCH v3 08/10] virtiofsd: Add inodes_by_handle hash table

[Hanna Reitz]

On 09.08.21 18:10, Vivek Goyal wrote:

On Fri, Jul 30, 2021 at 05:01:32PM +0200, Max Reitz wrote:

Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
FD in lo_inode.fd.  Therefore, when the respective inode is unlinked,
its inode ID will remain in use until we drop our lo_inode (and
lo_inode_put() thus closes the FD).  Therefore, lo_find() can safely use
the inode ID as an lo_inode key, because any inode with an inode ID we
find in lo_data.inodes (on the same filesystem) must be the exact same
file.

This will change when we start setting lo_inode.fhandle so we do not
have to keep an O_PATH FD open.  Then, unlinking such an inode will
immediately remove it, so its ID can then be reused by newly created
files, even while the lo_inode object is still there[1].

So creating a new file can then reuse the old file's inode ID, and
looking up the new file would lead to us finding the old file's
lo_inode, which is not ideal.

Luckily, just as file handles cause this problem, they also solve it:  A
file handle contains a generation ID, which changes when an inode ID is
reused, so the new file can be distinguished from the old one.  So all
we need to do is to add a second map besides lo_data.inodes that maps
file handles to lo_inodes, namely lo_data.inodes_by_handle.  For
clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.

Unfortunately, we cannot rely on being able to generate file handles
every time.  Therefore, we still enter every lo_inode object into
inodes_by_ids, but having an entry in inodes_by_handle is optional.  A
potential inodes_by_handle entry then has precedence, the inodes_by_ids
entry is just a fallback.

Note that we do not generate lo_fhandle objects yet, and so we also do
not enter anything into the inodes_by_handle map yet.  Also, all lookups
skip that map.  We might manually create file handles with some code
that is immediately removed by the next patch again, but that would
break the assumption in lo_find() that every lo_inode with a non-NULL
.fhandle must have an entry in inodes_by_handle and vice versa.  So we
leave actually using the inodes_by_handle map for the next patch.

[1] If some application in the guest still has the file open, there is
going to be a corresponding FD mapping in lo_data.fd_map.  In such a
case, the inode will only go away once every application in the guest
has closed it.  The problem described only applies to cases where the
guest does not have the file open, and it is just in the dentry cache,
basically.

Signed-off-by: Max Reitz 
---
  tools/virtiofsd/passthrough_ll.c | 81 +---
  1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 487448d666..f9d8b2f134 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -180,7 +180,8 @@ struct lo_data {
  int announce_submounts;
  bool use_statx;
  struct lo_inode root;
-GHashTable *inodes; /* protected by lo->mutex */
+GHashTable *inodes_by_ids; /* protected by lo->mutex */
+GHashTable *inodes_by_handle; /* protected by lo->mutex */
  struct lo_map ino_map; /* protected by lo->mutex */
  struct lo_map dirp_map; /* protected by lo->mutex */
  struct lo_map fd_map; /* protected by lo->mutex */
@@ -263,8 +264,9 @@ static struct {
  /* That we loaded cap-ng in the current thread from the saved */
  static __thread bool cap_loaded = 0;
  
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,

-uint64_t mnt_id);
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id);
  static int xattr_map_client(const struct lo_data *lo, const char *client_name,
  char **out_name);
  
@@ -1064,18 +1066,40 @@ out_err:

  fuse_reply_err(req, saverr);
  }
  
-static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,

-uint64_t mnt_id)
+static struct lo_inode *lo_find(struct lo_data *lo,
+const struct lo_fhandle *fhandle,
+struct stat *st, uint64_t mnt_id)
  {
-struct lo_inode *p;
-struct lo_key key = {
+struct lo_inode *p = NULL;
+struct lo_key ids_key = {
  .ino = st->st_ino,
  .dev = st->st_dev,
  .mnt_id = mnt_id,
  };
  
  pthread_mutex_lock(>mutex);

-p = g_hash_table_lookup(lo->inodes, );
+if (fhandle) {
+p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
+}
+if (!p) {
+p = g_hash_table_lookup(lo->inodes_by_ids, _key);

So even if fhandle is not NULL, we will still lookup the inode
object in lo->inodes_by_ids? I thought fallback was only required
if we could not generate file handle to begin with and in that case
fhandle will be NULL?

[PATCH v3] blog: add a post for the new TCG cache modelling plugin

[Mahmoud Mandour]

This post introduces the new TCG plugin `cache` that's used for cache
modelling. This plugin is a part of my GSoC 2021 participation.

Signed-off-by: Mahmoud Mandour 
---

v2 -> v3:
Added a prologue briefly explaining the importance of caching.

Explained the multi-threaded linux-user appoximation we use briefly in the
Mult-core caching section.

Dropped using the directory "./x86_64-linux-user" from CLI commands.

Updated the list of patches to include all patches posted related to the
cache plugin.

v1 -> v2:
Elaborated some more in the introduction and broken up some
sentences.

Changed the example of blocked matrix multiplication to a simpler
solution that requires less explanation since matrix multiplication
itself is out of the scope of the post.

Added the code for the `mm` function but did not directly give away
the cache-problematic portion and preferred to "investigate" the
problem since it's the job of the plugin to help knowing where the
problem is.

Added an epilogue in which I summarize the work and the patches
posted until now. 

 .../2021-08-09-tcg-cache-modelling-plugin.md  | 288 ++
 screenshots/2021-06-17-cache-structure.png| Bin 0 -> 19675 bytes
 2 files changed, 288 insertions(+)
 create mode 100644 _posts/2021-08-09-tcg-cache-modelling-plugin.md
 create mode 100644 screenshots/2021-06-17-cache-structure.png

diff --git a/_posts/2021-08-09-tcg-cache-modelling-plugin.md 
b/_posts/2021-08-09-tcg-cache-modelling-plugin.md
new file mode 100644
index 000..ae75c93
--- /dev/null
+++ b/_posts/2021-08-09-tcg-cache-modelling-plugin.md
@@ -0,0 +1,288 @@
+---
+layout: post
+title:  "Cache Modelling TCG Plugin"
+date:   2021-08-09 15:00:00 +0200
+author: Mahmoud Mandour
+categories: [TCG plugins, GSOC]
+---
+
+[Caches](https://en.wikipedia.org/wiki/CPU_cache) are a key way that enables
+modern CPUs to keep running at full speed by avoiding the need to fetch data
+and instructions from the comparatively slow system memory. As a result
+understanding cache behaviour is a key part of performance optimisation.
+
+TCG plugins provide means to instrument generated code for both user-mode and
+full system emulation. This includes the ability to intercept every memory
+access and instruction execution. This post introduces a new TCG plugin that's
+used to simulate configurable L1 separate instruction cache and data cache.
+
+While different microarchitectures often have different approaches at the very
+low level, the core concepts of caching are universal. As QEMU is not a
+microarchitectural emulator we model an ideal caching system with a few simple
+parameters. By doing so, we can adequately simulate the behaviour of L1 private
+(per-core) caches.
+
+## Overview
+
+The plugin simulates how L1 user-configured caches would behave when given a
+working set defined by a program in user-mode, or system-wide working set.
+Subsequently, it logs performance statistics along with the most N
+cache-thrashing instructions.
+
+### Configurability
+
+The plugin is configurable in terms of:
+
+* icache size parameters: `icachesize`, `iblksize`, `iassoc`, All of which take
+  a numeric value
+* dcache size parameters: `dcachesize`, `dblksize`, `dassoc`. All of which take
+  a numeric value
+* Eviction policy: `evict=lru|rand|fifo`
+* How many top-most thrashing instructions to log: `limit=TOP_N`
+* How many core caches to keep track of: `cores=N_CORES`
+
+### Multicore caching
+
+Multicore caching is achieved by having independent L1 caches for each 
available
+core.
+
+In __full-system emulation__, the number of available vCPUs is known to the
+plugin at plugin installation time, so separate caches are maintained for 
those.
+
+In __user-space emulation__, the index of the vCPU initiating memory access
+monotonically increases and is limited with however much the kernel allows
+creating. The approach used is that we allocate a static number of caches, and
+fit all memory accesses into those cores. This approximation is sufficiently
+similar to real systems since having more threads than cores will result in
+interleaving those threads between the available cores so they might thrash 
each
+other anyway.
+
+## Design and implementation
+
+### General structure
+
+A generic cache data structure, `Cache`, is used to model either an icache or
+dcache. For each known core, the plugin maintains an icache and a dcache. On a
+memory access coming from a core, the corresponding cache is interrogated.
+
+Each cache has a number of cache sets that are used to store the actual cached
+locations alongside metadata that backs eviction algorithms. The structure of a
+cache with `n` sets, and `m` blocks per sets is summarized in the following
+figure:
+
+![cache structure](/screenshots/2021-06-17-cache-structure.png)
+
+### Eviction algorithms
+
+The plugin supports three eviction algorithms:
+
+* Random eviction
+* Least recently used 

Re: [PATCH v3 07/10] virtiofsd: Add lo_inode.fhandle

[Hanna Reitz]

On 09.08.21 17:21, Vivek Goyal wrote:

On Fri, Jul 30, 2021 at 05:01:31PM +0200, Max Reitz wrote:

This new field is an alternative to lo_inode.fd: Either of the two must
be set.  In case an O_PATH FD is needed for some lo_inode, it is either
taken from lo_inode.fd, if valid, or a temporary FD is opened with
open_by_handle_at().

Using a file handle instead of an FD has the advantage of keeping the
number of open file descriptors low.

Because open_by_handle_at() requires a mount FD (i.e. a non-O_PATH FD
opened on the filesystem to which the file handle refers), but every
lo_fhandle only has a mount ID (as returned by name_to_handle_at()), we
keep a hash map of such FDs in mount_fds (mapping ID to FD).
get_file_handle(), which is added by a later patch, will ensure that
every mount ID for which we have generated a handle has a corresponding
entry in mount_fds.

Signed-off-by: Max Reitz 
Reviewed-by: Connor Kuehl 
---
  tools/virtiofsd/passthrough_ll.c  | 116 ++
  tools/virtiofsd/passthrough_seccomp.c |   1 +
  2 files changed, 102 insertions(+), 15 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 292b7f7e27..487448d666 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -88,8 +88,25 @@ struct lo_key {
  uint64_t mnt_id;
  };
  
+struct lo_fhandle {

+union {
+struct file_handle handle;
+char padding[sizeof(struct file_handle) + MAX_HANDLE_SZ];
+};
+int mount_id;
+};
+
+/* Maps mount IDs to an FD that we can pass to open_by_handle_at() */
+static GHashTable *mount_fds;
+pthread_rwlock_t mount_fds_lock = PTHREAD_RWLOCK_INITIALIZER;
+

How about if we move this hash table inside "struct lo_data". That seems to
be one global data structure keeping all the info. Also it can be
cleaned up during lo_destroy().


Yes, sounds good and right, will do.

Hanna

[CXL volatile MEM] - Qemu command to turn on HMAT and NUMA fails with assertion

[Samarth Saxena]

Hi All,

I am trying the following command to run Qemu with Kernel 5.14.

qemu-system-x86_64 -M q35,accel=kvm,nvdimm=on,cxl=on,hmat=on -m 
8448M,slots=2,maxmem=16G -smp 8,sockets=2,cores=2,threads=2 -hda 
/lan/dscratch/singhabh/shradha/ubuntu-20.10-64_with_orig_driver_backup.qcow2 
-enable-kvm -usbdevice tablet -L $VB_ROOT/etc/vm/common/ -object 
memory-backend-ram,id=cxl-ram,share=on,size=256M -device 
"pxb-cxl,id=cxl.0,bus=pcie.0,bus_nr=52,uid=0,len-window-base=1,window-base[0]=0x4c000,memdev[0]=cxl-ram"
 -device cxl-rp,id=rp0,bus=cxl.0,addr=0.0,chassis=0,slot=0 -device 
cxl-type3,bus=rp0,memdev=cxl-ram,id=cxl-vmem0,size=256M -numa 
node,nodeid=0,memdev=cxl-ram -object 
memory-backend-ram,id=other-ram,size=8G,prealloc=n,share=off -numa 
node,nodeid=1,memdev=other-ram,initiator=0 -numa cpu,node-id=0,socket-id=0 
-numa cpu,node-id=0,socket-id=1

I get the following crash before the machine starts to boot.

qemu-system-x86_64: ../softmmu/memory.c:2443: 
memory_region_add_subregion_common: Assertion `!subregion->container' failed.


Please help me find what's wrong here.

Regards,
[CadenceLogoRed185Regcopy1583174817new51584636989.png]
Samarth Saxena
Sr Principal Software Engineer
T: +911204308300
[UIcorrectsize1583179003.png]
[16066EmailSignatureFortune100Best2021White92x1271617625037.png]

Re: [PATCH v3] hw/acpi: add an assertion check for non-null return from acpi_get_i386_pci_host

[Ani Sinha]

On Fri, 6 Aug 2021, Philippe Mathieu-Daudé wrote:

> On 8/6/21 4:01 PM, Philippe Mathieu-Daudé wrote:
> > On 8/6/21 12:52 PM, Ani Sinha wrote:
> >> On Fri, 6 Aug 2021, Igor Mammedov wrote:
> >>> On Thu, 5 Aug 2021 19:42:35 +0530 (IST)
> >>> Ani Sinha  wrote:
>  On Thu, 5 Aug 2021, Ani Sinha wrote:
> > On Thu, 5 Aug 2021, Ani Sinha wrote:
> >> On Thu, 5 Aug 2021, Igor Mammedov wrote:
> >>> On Mon, 26 Jul 2021 22:27:43 +0530
> >>> Ani Sinha  wrote:
> >>>
>  All existing code using acpi_get_i386_pci_host() checks for a 
>  non-null
>  return value from this function call. Instead of returning early 
>  when the value
>  returned is NULL, assert instead. Since there are only two possible 
>  host buses
>  for i386 - q35 and i440fx, a null value return from the function 
>  does not make
>  sense in most cases and is likely an error situation.
> 
>  Fixes: c0e427d6eb5fef ("hw/acpi/ich9: Enable ACPI PCI hot-plug")
> 
>  Signed-off-by: Ani Sinha 
>  ---
>   hw/acpi/pcihp.c  |  8 
>   hw/i386/acpi-build.c | 15 ++-
>   2 files changed, 14 insertions(+), 9 deletions(-)
> 
>  changelog:
>  v1: initial patch
>  v2: removed comment addition - that can be sent as a separate patch.
>  v3: added assertion for null host values for all cases except one.
> 
>  diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
>  index f4d706e47d..054ee8cbc5 100644
>  --- a/hw/acpi/pcihp.c
>  +++ b/hw/acpi/pcihp.c
>  @@ -116,6 +116,12 @@ static void acpi_set_pci_info(void)
>   bsel_is_set = true;
> 
>   if (!host) {
>  +/*
>  + * This function can be eventually called from
>  + * qemu_devices_reset() -> acpi_pcihp_reset() even
>  + * for architectures other than i386. Hence, we need
>  + * to ignore null values for host here.
>  + */
>   return;
>   }
> >>>
> >>> I suspect it's a MIPS target that call this code unnecessarily.
> >>> It would be better to get rid of this condition altogether.
> >>> Frr that I can suggest to make acpi_pcihp_reset() stub and
> >>> replace pcihp.c with stub (perhaps use acpi-x86-stub.c) when building
> >>> for MIPS.
> >>>
> >>> then a bunch of asserts/ifs won't be necessary,
> >>> just one in acpi_get_i386_pci_host() will be sufficient.
> >>>
> >>
> >> OK this is a good idea.
> >> I can see that mips-softmmu-config-devices.h has
> >> CONFIG_ACPI_X86 turned on for mips. This does not seem right.
> >>
> >> The issue here is:
> >>
> >> $ grep -R CONFIG_ACPI_X86 *
> >> devices/mips-softmmu/common.mak:CONFIG_ACPI_X86=y
> >>
> >> So after
> >>
> >> -CONFIG_ACPI_X86=y
> >> -CONFIG_PIIX4=y
> >>
> >> (the second one is needed because after removing first one we get:
> >>
> >> /usr/bin/ld: libcommon.fa.p/hw_isa_piix4.c.o: in function 
> >> `piix4_create':
> >> /home/anisinha/workspace/qemu/build/../hw/isa/piix4.c:269: undefined
> >> reference to `piix4_pm_init'
> >>
> >> This is because in hw/acpi/meson.build, piix4.c is conditional on
> >> CONFIG_ACPI_X86. )
> >>
> >> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_gt64xxx_pci.c.o: in
> >> function `gt64120_pci_set_irq':
> >> /home/anisinha/workspace/qemu/build/../hw/mips/gt64xxx_pci.c:1020:
> >> undefined reference to `piix4_dev'
> >> /usr/bin/ld: libqemu-mips-softmmu.fa.p/hw_mips_malta.c.o: in function
> >> `mips_malta_init':
> >> /home/anisinha/workspace/qemu/build/../hw/mips/malta.c:1404: undefined
> >> reference to `piix4_create'
> >>
> >> So should mips be doing piix stuff anyway? Is Piix4 etc not x86 
> >> specific?
> >
> > PIIX, PIIX3 and PIIX4 are generic chipsets, not X86-specific.
> >
> > QEMU's PIIX3 is a Frankenstein to support virtualization to a chipset
> > not designed for it.
> > If you look at it, the X86 machine use a PIIX3 but the PIIX3 doesn't
> > even provide an ACPI function. It appeared in the PIIX4. The kludge is
> > to instanciate the PIIX4.acpi from the PIIX3 and X86 ppl are happy with
> > it, but it makes it ugly for the other architectures.
> >
> > Apparently this is by design:
> > https://qemu.readthedocs.io/en/stable/system/target-mips.html
> >
> > What do you mean "by design"? The Malta uses a PIIX4 chipset for its
> > southbridge indeed.
> >
> > which means mips malta will continue to use the x86 specific functions
> > like acpi_pcihp_reset(). Creating a stub for this with acpi-x86-stub.c
> > will result in a double symbol definition because CONFIG_PC is off for
> > mips.
> >
> 
>  Also to be noted that 

[PATCH v13] qapi: introduce 'query-x86-cpuid' QMP command.

[Valeriy Vdovin]

From: Valeriy Vdovin 

Introducing new QMP command 'query-x86-cpuid'. This command can be used to
get virtualized cpu model info generated by QEMU during VM initialization in
the form of cpuid representation.

Diving into more details about virtual CPU generation: QEMU first parses '-cpu'
command line option. From there it takes the name of the model as the basis for
feature set of the new virtual CPU. After that it uses trailing '-cpu' options,
that state if additional cpu features should be present on the virtual CPU or
excluded from it (tokens '+'/'-' or '=on'/'=off').
After that QEMU checks if the host's cpu can actually support the derived
feature set and applies host limitations to it.
After this initialization procedure, virtual CPU has it's model and
vendor names, and a working feature set and is ready for identification
instructions such as CPUID.

To learn exactly how virtual CPU is presented to the guest machine via CPUID
instruction, new QMP command can be used. By calling 'query-x86-cpuid'
command, one can get a full listing of all CPUID leaves with subleaves which are
supported by the initialized virtual CPU.

Other than debug, the command is useful in cases when we would like to
utilize QEMU's virtual CPU initialization routines and put the retrieved
values into kernel CPUID overriding mechanics for more precise control
over how various processes perceive its underlying hardware with
container processes as a good example.

The command is specific to x86. It is currenly only implemented for KVM 
acceleator.

Output format:
The output is a plain list of leaf/subleaf argument combinations, that
return 4 words in registers EAX, EBX, ECX, EDX.

Use example:
qmp_request: {
  "execute": "query-x86-cpuid"
}

qmp_response: {
  "return": [
{
  "eax": 1073741825,
  "edx": 77,
  "in-eax": 1073741824,
  "ecx": 1447775574,
  "ebx": 1263359563
},
{
  "eax": 16777339,
  "edx": 0,
  "in-eax": 1073741825,
  "ecx": 0,
  "ebx": 0
},
{
  "eax": 13,
  "edx": 1231384169,
  "in-eax": 0,
  "ecx": 1818588270,
  "ebx": 1970169159
},
{
  "eax": 198354,
  "edx": 126614527,
  "in-eax": 1,
  "ecx": 2176328193,
  "ebx": 2048
},

{
  "eax": 12328,
  "edx": 0,
  "in-eax": 2147483656,
  "ecx": 0,
  "ebx": 0
}
  ]
}

Signed-off-by: Valeriy Vdovin 
---
v2: - Removed leaf/subleaf iterators.
- Modified cpu_x86_cpuid to return false in cases when count is
  greater than supported subleaves.
v3: - Fixed structure name coding style.
- Added more comments
- Ensured buildability for non-x86 targets.
v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf.
- Fixed comments.
- Removed target check in qmp_query_cpu_model_cpuid.
v5: - Added error handling code in qmp_query_cpu_model_cpuid
v6: - Fixed error handling code. Added method to query_error_class
v7: - Changed implementation in favor of cached cpuid_data for
  KVM_SET_CPUID2
v8: - Renamed qmp method to query-kvm-cpuid and some fields in response.
- Modified documentation to qmp method
- Removed helper struct declaration
v9: - Renamed 'in_eax' / 'in_ecx' fields to 'in-eax' / 'in-ecx'
- Pasted more complete response to commit message.
v10:
- Subject changed
- Fixes in commit message
- Small fixes in QMP command docs
v11:
- Added explanation about CONFIG_KVM to the commit message.
v12:
- Changed title from query-kvm-cpuid to query-x86-cpuid
- Removed CONFIG_KVM ifdefs
- Added detailed error messages for some stub/unimplemented cases.
v13:
- Tagged with since 6.2

 qapi/machine-target.json   | 44 
 softmmu/cpus.c |  2 +-
 target/i386/kvm/kvm-stub.c | 10 
 target/i386/kvm/kvm.c  | 51 ++
 tests/qtest/qmp-cmd-test.c |  1 +
 5 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..599394d067 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,47 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || 
defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+
+##
+# @CpuidEntry:
+#
+# A single entry of a CPUID response.
+#
+# One entry holds full set of information (leaf) returned to the guest
+# in response to it calling a CPUID instruction with eax, ecx used as
+# the agruments to that instruction. ecx is an optional argument as
+# not all of the leaves support it.
+#
+# @in-eax: CPUID argument in eax
+# @in-ecx: CPUID argument in ecx
+# @eax: CPUID result in eax
+# @ebx: CPUID result in ebx
+# @ecx: CPUID result in ecx
+# @edx: CPUID result in edx
+#
+# Since: 6.2
+##
+{ 'struct': 'CpuidEntry',
+  'data': { 'in-eax' : 'uint32',
+'*in-ecx' : 'uint32',
+ 

[PATCH] ui/sdl2: Check return value from g_setenv()

[Peter Maydell]

Setting environment variables can fail; check the return value
from g_setenv() and bail out if we couldn't set SDL_VIDEODRIVER.

Fixes: Coverity 1458798
Signed-off-by: Peter Maydell 
---
I followed existing practice in this function for how it
deals with errors (ie, fprintf to stderr and exit).
---
 ui/sdl2.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ui/sdl2.c b/ui/sdl2.c
index 36d9010cb6c..17c0ec30ebf 100644
--- a/ui/sdl2.c
+++ b/ui/sdl2.c
@@ -817,7 +817,10 @@ static void sdl2_display_init(DisplayState *ds, 
DisplayOptions *o)
  * This is a bit hackish but saves us from bigger problem.
  * Maybe it's a good idea to fix this in SDL instead.
  */
-g_setenv("SDL_VIDEODRIVER", "x11", 0);
+if (!g_setenv("SDL_VIDEODRIVER", "x11", 0)) {
+fprintf(stderr, "Could not set SDL_VIDEODRIVER environment 
variable\n");
+exit(1);
+}
 #endif
 
 if (SDL_Init(SDL_INIT_VIDEO)) {
-- 
2.20.1

Re: [PATCH 1/2] virtio: add a way to disable a queue

[Laurent Vivier]

On 09/08/2021 05:01, Jason Wang wrote:
> 
> 在 2021/8/6 下午3:27, Laurent Vivier 写道:
>> On 06/08/2021 08:25, Jason Wang wrote:
>>> 在 2021/8/2 下午4:42, Laurent Vivier 写道:
 On 02/08/2021 06:50, Jason Wang wrote:
> 在 2021/7/30 上午3:19, Laurent Vivier 写道:
>> Add virtio_queue_disable()/virtio_queue_enable() to disable/enable a 
>> queue
>> by setting vring.num to 0 (or num_default).
>> This is needed to be able to disable a guest driver from the host side
> I suspect this won't work correclty for vhost.
 With my test it seems to work with vhost too.
>>>
>>> So setting 0 will lead -EINVAL to be returned during VHOST_SET_VRING_NUM. I 
>>> think qemu
>>> will warn the failure in this case.
>> I didn't see any error when I tried. I will check the code.
>>
>>> What's more important, it's not guaranteed to work for the case of 
>>> vhost-user or
>>> vhost-vDPA.
>> Perhaps we can target only the vhost host case, as this is used for failover 
>> and usually
>> the virtio-net device is backed by a bridge on same network as the VFIO 
>> device?
> 
> 
> Probably not, it should be a general feature that can work for all types of 
> virtio/vhost
> backends.
> 
> 
>>
>>>
> And I believe we should only do this after the per queue 
> enabling/disabling is supported
> by the spec.
>
> (only MMIO support that AFAIK)
 I don't want to modify the spec.

 I need something that works without modifying existing (old) drivers.

 The idea is to be able to disable the virtio-net kernel driver from QEMU 
 if the driver is
 too old (i.e. it doesn't support STANDBY feature).

 Setting vring.num to 0 forces the kernel driver to exit on error in the 
 probe function.
 It's what I want: the device is present but disabled (the driver is not 
 loaded).

 Any other suggestion?
>>>
>>> I think we should probably disable the device instead of doing it per 
>>> virtqueue.
>>>
>> I tried to use virtio_set_disabled() but it doesn't work.
>> Perhaps it's too late when I call the function (I need to do that in
>> virtio_net_set_features()). What I want is to prevent the load of the driver 
>> in the guest
>> kernel to hide the virtio-net device. Setting vring.num to 0 triggers an 
>> error in the
>> driver probe function and prevents the load of the driver.
> 
> 
> How about fail the validate_features() in this case?

It's a good suggestion and it seems to work.

I'm going to send an updated patch.

Thanks,
Laurent

Re: [PATCH v3 08/10] virtiofsd: Add inodes_by_handle hash table

[Vivek Goyal]

On Fri, Jul 30, 2021 at 05:01:32PM +0200, Max Reitz wrote:
> Currently, lo_inode.fhandle is always NULL and so always keep an O_PATH
> FD in lo_inode.fd.  Therefore, when the respective inode is unlinked,
> its inode ID will remain in use until we drop our lo_inode (and
> lo_inode_put() thus closes the FD).  Therefore, lo_find() can safely use
> the inode ID as an lo_inode key, because any inode with an inode ID we
> find in lo_data.inodes (on the same filesystem) must be the exact same
> file.
> 
> This will change when we start setting lo_inode.fhandle so we do not
> have to keep an O_PATH FD open.  Then, unlinking such an inode will
> immediately remove it, so its ID can then be reused by newly created
> files, even while the lo_inode object is still there[1].
> 
> So creating a new file can then reuse the old file's inode ID, and
> looking up the new file would lead to us finding the old file's
> lo_inode, which is not ideal.
> 
> Luckily, just as file handles cause this problem, they also solve it:  A
> file handle contains a generation ID, which changes when an inode ID is
> reused, so the new file can be distinguished from the old one.  So all
> we need to do is to add a second map besides lo_data.inodes that maps
> file handles to lo_inodes, namely lo_data.inodes_by_handle.  For
> clarity, lo_data.inodes is renamed to lo_data.inodes_by_ids.
> 
> Unfortunately, we cannot rely on being able to generate file handles
> every time.  Therefore, we still enter every lo_inode object into
> inodes_by_ids, but having an entry in inodes_by_handle is optional.  A
> potential inodes_by_handle entry then has precedence, the inodes_by_ids
> entry is just a fallback.
> 
> Note that we do not generate lo_fhandle objects yet, and so we also do
> not enter anything into the inodes_by_handle map yet.  Also, all lookups
> skip that map.  We might manually create file handles with some code
> that is immediately removed by the next patch again, but that would
> break the assumption in lo_find() that every lo_inode with a non-NULL
> .fhandle must have an entry in inodes_by_handle and vice versa.  So we
> leave actually using the inodes_by_handle map for the next patch.
> 
> [1] If some application in the guest still has the file open, there is
> going to be a corresponding FD mapping in lo_data.fd_map.  In such a
> case, the inode will only go away once every application in the guest
> has closed it.  The problem described only applies to cases where the
> guest does not have the file open, and it is just in the dentry cache,
> basically.
> 
> Signed-off-by: Max Reitz 
> ---
>  tools/virtiofsd/passthrough_ll.c | 81 +---
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 487448d666..f9d8b2f134 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -180,7 +180,8 @@ struct lo_data {
>  int announce_submounts;
>  bool use_statx;
>  struct lo_inode root;
> -GHashTable *inodes; /* protected by lo->mutex */
> +GHashTable *inodes_by_ids; /* protected by lo->mutex */
> +GHashTable *inodes_by_handle; /* protected by lo->mutex */
>  struct lo_map ino_map; /* protected by lo->mutex */
>  struct lo_map dirp_map; /* protected by lo->mutex */
>  struct lo_map fd_map; /* protected by lo->mutex */
> @@ -263,8 +264,9 @@ static struct {
>  /* That we loaded cap-ng in the current thread from the saved */
>  static __thread bool cap_loaded = 0;
>  
> -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> -uint64_t mnt_id);
> +static struct lo_inode *lo_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +struct stat *st, uint64_t mnt_id);
>  static int xattr_map_client(const struct lo_data *lo, const char 
> *client_name,
>  char **out_name);
>  
> @@ -1064,18 +1066,40 @@ out_err:
>  fuse_reply_err(req, saverr);
>  }
>  
> -static struct lo_inode *lo_find(struct lo_data *lo, struct stat *st,
> -uint64_t mnt_id)
> +static struct lo_inode *lo_find(struct lo_data *lo,
> +const struct lo_fhandle *fhandle,
> +struct stat *st, uint64_t mnt_id)
>  {
> -struct lo_inode *p;
> -struct lo_key key = {
> +struct lo_inode *p = NULL;
> +struct lo_key ids_key = {
>  .ino = st->st_ino,
>  .dev = st->st_dev,
>  .mnt_id = mnt_id,
>  };
>  
>  pthread_mutex_lock(>mutex);
> -p = g_hash_table_lookup(lo->inodes, );
> +if (fhandle) {
> +p = g_hash_table_lookup(lo->inodes_by_handle, fhandle);
> +}
> +if (!p) {
> +p = g_hash_table_lookup(lo->inodes_by_ids, _key);

So even if fhandle is not NULL, we will still lookup the inode
object in 

Re: [PULL for-6.1 0/1] hw/nvme fixes

[Peter Maydell]

On Mon, 9 Aug 2021 at 11:56, Klaus Jensen  wrote:
>
> From: Klaus Jensen 
>
> Hi Peter,
>
> The following changes since commit dee64246ded3aa7dbada68b96ce1c64e5bea327d:
>
>   Merge remote-tracking branch 
> 'remotes/vivier2/tags/linux-user-for-6.1-pull-request' into staging 
> (2021-08-06 10:28:33 +0100)
>
> are available in the Git repository at:
>
>   git://git.infradead.org/qemu-nvme.git tags/nvme-next-pull-request
>
> for you to fetch changes up to 5f4884c4412318a1adc105dea9cc28f7625ce730:
>
>   hw/nvme: fix missing variable initializers (2021-08-09 12:52:16 +0200)
>
> 
> hw/nvme fixes
>
> * coverity fixes
>
> 


Applied, thanks.

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

-- PMM

Re: [PATCH V6 00/27] Live Update

[Steven Sistare]

I forgot to mention in the changes list: I added a new mechanism to save fd 
values,
in lieu of the environment.  See [PATCH V6 13/27] cpr: preserve extra state

- Steve

On 8/6/2021 5:43 PM, Steve Sistare wrote:
> Provide the cpr-save, cpr-exec, and cpr-load commands for live update.
> These save and restore VM state, with minimal guest pause time, so that
> qemu may be updated to a new version in between.
> 
> cpr-save stops the VM and saves vmstate to an ordinary file.  It supports
> any type of guest image and block device, but the caller must not modify
> guest block devices between cpr-save and cpr-load.  It supports two modes:
> reboot and restart.
> 
> In reboot mode, the caller invokes cpr-save and then terminates qemu.
> The caller may then update the host kernel and system software and reboot.
> The caller resumes the guest by running qemu with the same arguments as the
> original process and invoking cpr-load.  To use this mode, guest ram must be
> mapped to a persistent shared memory file such as /dev/dax0.0, or /dev/shm
> PKRAM as proposed in 
> https://lore.kernel.org/lkml/1617140178-8773-1-git-send-email-anthony.yzn...@oracle.com.
> 
> The reboot mode supports vfio devices if the caller first suspends the
> guest, such as by issuing guest-suspend-ram to the qemu guest agent.  The
> guest drivers' suspend methods flush outstanding requests and re-initialize
> the devices, and thus there is no device state to save and restore.
> 
> Restart mode preserves the guest VM across a restart of the qemu process.
> After cpr-save, the caller passes qemu command-line arguments to cpr-exec,
> which directly exec's the new qemu binary.  The arguments must include -S
> so new qemu starts in a paused state and waits for the cpr-load command.
> The restart mode supports vfio devices by preserving the vfio container,
> group, device, and event descriptors across the qemu re-exec, and by
> updating DMA mapping virtual addresses using VFIO_DMA_UNMAP_FLAG_VADDR and
> VFIO_DMA_MAP_FLAG_VADDR as defined in 
> https://lore.kernel.org/kvm/1611939252-7240-1-git-send-email-steven.sist...@oracle.com/
> and integrated in Linux kernel 5.12.
> 
> To use the restart mode, qemu must be started with the memfd-alloc option,
> which allocates guest ram using memfd_create.  The memfd's are saved to
> the environment and kept open across exec, after which they are found from
> the environment and re-mmap'd.  Hence guest ram is preserved in place,
> albeit with new virtual addresses in the qemu process.
> 
> The caller resumes the guest by invoking cpr-load, which loads state from
> the file. If the VM was running at cpr-save time, then VM execution resumes.
> If the VM was suspended at cpr-save time (reboot mode), then the caller must
> issue a system_wakeup command to resume.
> 
> The first patches add reboot mode:
>   - memory: qemu_check_ram_volatile
>   - migration: fix populate_vfio_info
>   - migration: qemu file wrappers
>   - migration: simplify savevm
>   - vl: start on wakeup request
>   - cpr: reboot mode
>   - cpr: reboot HMP interfaces
> 
> The next patches add restart mode:
>   - memory: flat section iterator
>   - oslib: qemu_clear_cloexec
>   - machine: memfd-alloc option
>   - qapi: list utility functions
>   - vl: helper to request re-exec
>   - cpr: preserve extra state
>   - cpr: restart mode
>   - cpr: restart HMP interfaces
>   - hostmem-memfd: cpr for memory-backend-memfd
> 
> The next patches add vfio support for restart mode:
>   - pci: export functions for cpr
>   - vfio-pci: refactor for cpr
>   - vfio-pci: cpr part 1 (fd and dma)
>   - vfio-pci: cpr part 2 (msi)
>   - vfio-pci: cpr part 3 (intx)
> 
> The next patches preserve various descriptor-based backend devices across
> cprexec:
>   - vhost: reset vhost devices for cpr
>   - chardev: cpr framework
>   - chardev: cpr for simple devices
>   - chardev: cpr for pty
>   - chardev: cpr for sockets
>   - cpr: only-cpr-capable option
> 
> Here is an example of updating qemu from v4.2.0 to v4.2.1 using
> restart mode.  The software update is performed while the guest is
> running to minimize downtime.
> 
> window 1| window 2
> |
> # qemu-system-x86_64 ...|
> QEMU 4.2.0 monitor - type 'help' ...|
> (qemu) info status  |
> VM status: running  |
> | # yum update qemu
> (qemu) cpr-save /tmp/qemu.sav restart   |
> (qemu) cpr-exec qemu-system-x86_64 -S ...   |
> QEMU 4.2.1 monitor - type 'help' ...|
> (qemu) info status  |
> VM status: paused (prelaunch)   |
> (qemu) cpr-load /tmp/qemu.sav   |
> (qemu) info status  |
> VM status: running  |
> 
> 
> Here is an example of updating the host kernel 

Re: [PATCH] linux-user: Check lock_user result for ip_mreq_source sockopts

[Philippe Mathieu-Daudé]

On 8/9/21 5:54 PM, Peter Maydell wrote:
> In do_setsockopt(), the code path for the options which take a struct
> ip_mreq_source (IP_BLOCK_SOURCE, IP_UNBLOCK_SOURCE,
> IP_ADD_SOURCE_MEMBERSHIP and IP_DROP_SOURCE_MEMBERSHIP) fails to
> check the return value from lock_user().  Handle this in the usual
> way by returning -TARGET_EFAULT.
> 
> (In practice this was probably harmless because we'd pass a NULL
> pointer to setsockopt() and the kernel would then return EFAULT.)
> 
> Fixes: Coverity CID 1459987
> Signed-off-by: Peter Maydell 
> ---
> Compile-tested only; I don't have a test case to hand that
> uses these socket options.
> 
>  linux-user/syscall.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 09/10] hw/misc: Add Infineon DPS310 sensor model

[Philippe Mathieu-Daudé]

On 8/9/21 3:15 PM, Cédric Le Goater wrote:
> From: Joel Stanley 
> 
> This contains some hardcoded register values that were obtained from the
> hardware after reading the temperature.
> 
> It does enough to test the Linux kernel driver. The FIFO mode, IRQs and
> operation modes other than the default as used by Linux are not modelled.
> 
> Signed-off-by: Joel Stanley 
> [ clg: Fix sequential reading ]
> Message-Id: <20210616073358.750472-2-j...@jms.id.au>
> Signed-off-by: Cédric Le Goater 
> Message-Id: <20210629142336.750058-4-...@kaod.org>
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/misc/dps310.c| 227 
>  hw/arm/Kconfig  |   1 +
>  hw/misc/Kconfig |   4 +
>  hw/misc/meson.build |   1 +
>  4 files changed, 233 insertions(+)
>  create mode 100644 hw/misc/dps310.c
> 
> diff --git a/hw/misc/dps310.c b/hw/misc/dps310.c
> new file mode 100644
> index ..893521ab8516
> --- /dev/null
> +++ b/hw/misc/dps310.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2017-2021 Joel Stanley , IBM Corporation
> + *
> + * Infineon DPS310 temperature and humidity sensor
> + *
> + * 
> https://www.infineon.com/cms/en/product/sensor/pressure-sensors/pressure-sensors-for-iot/dps310/
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/hw.h"
> +#include "hw/i2c/i2c.h"
> +#include "qapi/error.h"
> +#include "qapi/visitor.h"
> +#include "migration/vmstate.h"
> +
> +#define NUM_REGISTERS   0x33
> +
> +typedef struct DPS310State {
> +/*< private >*/
> +I2CSlave i2c;
> +
> +/*< public >*/
> +uint8_t regs[NUM_REGISTERS];
> +
> +uint8_t len;
> +uint8_t pointer;
> +
> +} DPS310State;

> +static void dps310_reset(DeviceState *dev)
> +{
> +DPS310State *s = DPS310(dev);
> +
> +static const uint8_t regs_reset_state[] = {

   static const uint8_t regs_reset_state[NUM_REGISTERS] = {

> +0xfe, 0x2f, 0xee, 0x02, 0x69, 0xa6, 0x00, 0x80, 0xc7, 0x00, 0x00, 
> 0x00,
> +0x00, 0x10, 0x00, 0x00, 0x0e, 0x1e, 0xdd, 0x13, 0xca, 0x5f, 0x21, 
> 0x52,
> +0xf9, 0xc6, 0x04, 0xd1, 0xdb, 0x47, 0x00, 0x5b, 0xfb, 0x3a, 0x00, 
> 0x00,
> +0x20, 0x49, 0x4e, 0xa5, 0x90, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> 0x00,
> +0x60, 0x15, 0x02
> +};
> +
> +QEMU_BUILD_BUG_ON(sizeof(regs_reset_state) != sizeof(s->regs));

and drop QEMU_BUILD_BUG_ON?

> +
> +memcpy(s->regs, regs_reset_state, sizeof(s->regs));
> +s->pointer = 0;
> +
> +/* TODO: assert these after some timeout ? */
> +s->regs[DPS310_MEAS_CFG] = DPS310_COEF_RDY | DPS310_SENSOR_RDY
> +| DPS310_TMP_RDY | DPS310_PRS_RDY;
> +}

Re: [PATCH 08/10] aspeed: Emulate the AST2600A3

[Philippe Mathieu-Daudé]

On 8/9/21 3:15 PM, Cédric Le Goater wrote:
> From: Joel Stanley 
> 
> This is the latest revision of the ASPEED 2600 SoC. As there is no
> need to model multiple revisions of the same SoC for the moment,
> update the SCU AST2600 to model the A3 revision instead of the A1 and
> adapt the AST2600 SoC and machines.
> 
> Reset values are taken from v8 of the datasheet.
> 
> Signed-off-by: Joel Stanley 
> [ clg: - Introduced an Aspeed "ast2600-a3" SoC class
>- Commit log update ]
> Message-Id: <20210407171637.43-21-...@kaod.org>
> Signed-off-by: Cédric Le Goater 
> Message-Id: <20210629142336.750058-3-...@kaod.org>
> Signed-off-by: Cédric Le Goater 
> ---
>  include/hw/misc/aspeed_scu.h |  2 ++
>  hw/arm/aspeed.c  |  6 +++---
>  hw/arm/aspeed_ast2600.c  |  6 +++---
>  hw/misc/aspeed_scu.c | 36 +---
>  4 files changed, 37 insertions(+), 13 deletions(-)

> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 40a38ebd8549..05edebedeb46 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -101,14 +101,24 @@
>  #define AST2600_CLK_STOP_CTRL_CLR TO_REG(0x84)
>  #define AST2600_CLK_STOP_CTRL2 TO_REG(0x90)
>  #define AST2600_CLK_STOP_CTRL2_CLR TO_REG(0x94)
> +#define AST2600_DEBUG_CTRLTO_REG(0xC8)
> +#define AST2600_DEBUG_CTRL2   TO_REG(0xD8)
>  #define AST2600_SDRAM_HANDSHAKE   TO_REG(0x100)
>  #define AST2600_HPLL_PARAMTO_REG(0x200)
>  #define AST2600_HPLL_EXT  TO_REG(0x204)
> +#define AST2600_APLL_PARAMTO_REG(0x210)
> +#define AST2600_APLL_EXT  TO_REG(0x214)
> +#define AST2600_MPLL_PARAMTO_REG(0x220)
>  #define AST2600_MPLL_EXT  TO_REG(0x224)
> +#define AST2600_EPLL_PARAMTO_REG(0x240)
>  #define AST2600_EPLL_EXT  TO_REG(0x244)
> +#define AST2600_DPLL_PARAMTO_REG(0x260)
> +#define AST2600_DPLL_EXT  TO_REG(0x264)
>  #define AST2600_CLK_SEL   TO_REG(0x300)
>  #define AST2600_CLK_SEL2  TO_REG(0x304)
> -#define AST2600_CLK_SEL3  TO_REG(0x310)
> +#define AST2600_CLK_SEL3  TO_REG(0x308)

Is it a bugfix? Otherwise this is annoying.

Maybe:

 #define AST2600A1_CLK_SEL3  TO_REG(0x310)
 #define AST2600A3_CLK_SEL3  TO_REG(0x308)

and...

> +#define AST2600_CLK_SEL4  TO_REG(0x310)
> +#define AST2600_CLK_SEL5  TO_REG(0x314)
>  #define AST2600_HW_STRAP1 TO_REG(0x500)
>  #define AST2600_HW_STRAP1_CLR TO_REG(0x504)
>  #define AST2600_HW_STRAP1_PROTTO_REG(0x508)
> @@ -433,6 +443,8 @@ static uint32_t aspeed_silicon_revs[] = {
>  AST2500_A1_SILICON_REV,
>  AST2600_A0_SILICON_REV,
>  AST2600_A1_SILICON_REV,
> +AST2600_A2_SILICON_REV,
> +AST2600_A3_SILICON_REV,
>  };
>  
>  bool is_supported_silicon_rev(uint32_t silicon_rev)
> @@ -651,16 +663,26 @@ static const MemoryRegionOps aspeed_ast2600_scu_ops = {
>  .valid.unaligned = false,
>  };
>  
> -static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
> +static const uint32_t ast2600_a3_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>  [AST2600_SYS_RST_CTRL]  = 0xF7C3FED8,
> -[AST2600_SYS_RST_CTRL2] = 0xFFFC,
> +[AST2600_SYS_RST_CTRL2] = 0x0DFC,
>  [AST2600_CLK_STOP_CTRL] = 0x7F8A,
>  [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
> +[AST2600_DEBUG_CTRL]= 0x0FFF,
> +[AST2600_DEBUG_CTRL2]   = 0x00FF,
>  [AST2600_SDRAM_HANDSHAKE]   = 0x,
> -[AST2600_HPLL_PARAM]= 0x1000405F,
> +[AST2600_HPLL_PARAM]= 0x1000408F,
> +[AST2600_APLL_PARAM]= 0x1000405F,
> +[AST2600_MPLL_PARAM]= 0x1008405F,
> +[AST2600_EPLL_PARAM]= 0x1004077F,
> +[AST2600_DPLL_PARAM]= 0x1078405F,
> +[AST2600_CLK_SEL]   = 0xF394,
> +[AST2600_CLK_SEL2]  = 0x0070,
> +[AST2600_CLK_SEL3]  = 0x,

... use AST2600A3_CLK_SEL3 here?

So someone wanting the emulate the A1 doesn't get
the nasty bug of having CLK_SEL3 misplaced.

> +[AST2600_CLK_SEL4]  = 0xF3F4,
> +[AST2600_CLK_SEL5]  = 0x3000,
>  [AST2600_CHIP_ID0]  = 0x1234ABCD,
>  [AST2600_CHIP_ID1]  = 0x,
> -
>  };

[PATCH] linux-user: Check lock_user result for ip_mreq_source sockopts

[Peter Maydell]

In do_setsockopt(), the code path for the options which take a struct
ip_mreq_source (IP_BLOCK_SOURCE, IP_UNBLOCK_SOURCE,
IP_ADD_SOURCE_MEMBERSHIP and IP_DROP_SOURCE_MEMBERSHIP) fails to
check the return value from lock_user().  Handle this in the usual
way by returning -TARGET_EFAULT.

(In practice this was probably harmless because we'd pass a NULL
pointer to setsockopt() and the kernel would then return EFAULT.)

Fixes: Coverity CID 1459987
Signed-off-by: Peter Maydell 
---
Compile-tested only; I don't have a test case to hand that
uses these socket options.

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ccd3892b2df..d2b062ea5a9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2121,6 +2121,9 @@ static abi_long do_setsockopt(int sockfd, int level, int 
optname,
 return -TARGET_EINVAL;
 
 ip_mreq_source = lock_user(VERIFY_READ, optval_addr, optlen, 1);
+if (!ip_mreq_source) {
+return -TARGET_EFAULT;
+}
 ret = get_errno(setsockopt(sockfd, level, optname, ip_mreq_source, 
optlen));
 unlock_user (ip_mreq_source, optval_addr, 0);
 break;
-- 
2.20.1

Re: [PATCH 03/10] watchdog: aspeed: Fix sequential control writes

[Philippe Mathieu-Daudé]

On 8/9/21 3:15 PM, Cédric Le Goater wrote:
> From: Andrew Jeffery 
> 
> The logic in the handling for the control register required toggling the
> enable state for writes to stick. Rework the condition chain to allow
> sequential writes that do not update the enable state.
> 
> Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> Signed-off-by: Andrew Jeffery 
> Reviewed-by: Cédric Le Goater 
> Message-Id: <20210709053107.1829304-3-and...@aj.id.au>
> Signed-off-by: Cédric Le Goater 
> ---
>  hw/watchdog/wdt_aspeed.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index faa3d35fdf21..69c37af9a6e9 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset, 
> uint64_t data,
>  } else if (!enable && aspeed_wdt_is_enabled(s)) {
>  s->regs[WDT_CTRL] = data;
>  timer_del(s->timer);
> +} else {
> +s->regs[WDT_CTRL] = data;
>  }

Alternatively easier to review:

   } else {
   if (!enable && aspeed_wdt_is_enabled(s)) {
   timer_del(s->timer);
   }
   s->regs[WDT_CTRL] = data;
   }

>  break;
>  case WDT_RESET_WIDTH:
> 

Re: [PATCH for 6.1] tests: filter out TLS distinguished name in certificate checks

[Hanna Reitz]

On 04.08.21 20:03, Daniel P. Berrangé wrote:

The version of GNUTLS in Fedora 34 has changed the order in which encodes
fields when generating new TLS certificates. This in turn changes the
order seen when querying the distinguished name. This ultimately breaks
the expected output in the NBD TLS iotests. We don't need to be
comparing the exact distinguished name text for the purpose of the test
though, so it is fine to filter it out.

Reported-by: Eric Blake 
Signed-off-by: Daniel P. Berrangé 
---
  tests/qemu-iotests/233   | 2 +-
  tests/qemu-iotests/233.out   | 4 ++--
  tests/qemu-iotests/common.filter | 5 +
  3 files changed, 8 insertions(+), 3 deletions(-)


Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

(Given my email address change today, I don’t know yet how well the pull 
request will go.  Perhaps I’ll have to ask Kevin or Eric to step in on 
this one.)


Hanna

Re: [PATCH v2] block/export/fuse.c: fix musl build

[Hanna Reitz]

On 09.08.21 11:51, Fabrice Fontaine wrote:

Fix the following build failure on musl raised since version 6.0.0 and
https://gitlab.com/qemu-project/qemu/-/commit/4ca37a96a75aafe7a37ba51ab1912b09b7190a6b
because musl does not define FALLOC_FL_ZERO_RANGE:

../block/export/fuse.c: In function 'fuse_fallocate':
../block/export/fuse.c:563:23: error: 'FALLOC_FL_ZERO_RANGE' undeclared (first 
use in this function)
   563 | } else if (mode & FALLOC_FL_ZERO_RANGE) {
   |   ^~~~

Fixes:
  - 
http://autobuild.buildroot.org/results/b96e3d364fd1f8bbfb18904a742e73327d308f64

Signed-off-by: Fabrice Fontaine 
---
Changes v1 -> v2 (after review of Philippe Mathieu-Daudé):
  - Use CONFIG_FALLOCATE_ZERO_RANGE and make safer #ifdef'ry

  block/export/fuse.c | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)


Thanks, applied to my block branch:

https://github.com/XanClic/qemu/commits/block

(Of note: I’ve had an email address change today, so sending the pull 
request may become a bit hairy.  If it doesn’t work out, I’ll have to 
ask Kevin (or Peter directly, given this is a build fix) tomorrow.)