Re: [PATCH for-6.0] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option

[Thomas Huth]

On 07/04/2021 14.48, Mark Cave-Ayland wrote:

If QEMU is launched with the -S option then the ESPState mig_version_id property
is left unset due to the ordering of the VMState fields in the 
VMStateDescription
for sysbusespscsi and pciespscsi. If the VM is migrated and restored in this
stopped state, the version tests in the vmstate_esp VMStateDescription and
esp_post_load() become confused causing the migration to fail.

Fix the ordering problem by moving the setting of mig_version_id to a common
esp_pre_save() function which is invoked first by both sysbusespscsi and
pciespscsi rather than at the point where ESPState is itself serialised into the
migration stream.

Buglink: https://bugs.launchpad.net/qemu/+bug/1922611
Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState")
Signed-off-by: Mark Cave-Ayland 
---
  hw/scsi/esp-pci.c | 1 +
  hw/scsi/esp.c | 7 ---
  include/hw/scsi/esp.h | 1 +
  3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index c3d3dab05e..9db10b1a48 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -332,6 +332,7 @@ static const VMStateDescription vmstate_esp_pci_scsi = {
  .name = "pciespscsi",
  .version_id = 2,
  .minimum_version_id = 1,
+.pre_save = esp_pre_save,
  .fields = (VMStateField[]) {
  VMSTATE_PCI_DEVICE(parent_obj, PCIESPState),
  VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)),
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3b9037e4f4..0037197bdb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1089,9 +1089,10 @@ static bool esp_is_version_5(void *opaque, int 
version_id)
  return version_id == 5;
  }
  
-static int esp_pre_save(void *opaque)

+int esp_pre_save(void *opaque)
  {
-ESPState *s = ESP(opaque);
+ESPState *s = ESP(object_resolve_path_component(
+  OBJECT(opaque), "esp"));
  
  s->mig_version_id = vmstate_esp.version_id;

  return 0;
@@ -1127,7 +1128,6 @@ const VMStateDescription vmstate_esp = {
  .name = "esp",
  .version_id = 5,
  .minimum_version_id = 3,
-.pre_save = esp_pre_save,
  .post_load = esp_post_load,
  .fields = (VMStateField[]) {
  VMSTATE_BUFFER(rregs, ESPState),
@@ -1317,6 +1317,7 @@ static const VMStateDescription vmstate_sysbus_esp_scsi = 
{
  .name = "sysbusespscsi",
  .version_id = 2,
  .minimum_version_id = 1,
+.pre_save = esp_pre_save,
  .fields = (VMStateField[]) {
  VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2),
  VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState),
diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h
index 95088490aa..aada3680b7 100644
--- a/include/hw/scsi/esp.h
+++ b/include/hw/scsi/esp.h
@@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s);
  uint64_t esp_reg_read(ESPState *s, uint32_t saddr);
  void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val);
  extern const VMStateDescription vmstate_esp;
+int esp_pre_save(void *opaque);


Reviewed-by: Thomas Huth 

Which tree should this patch go through? Your Sparc tree? Migration? Scsi? 
Trivial?

Re: Discussion: Patch series that adds disable-tcg option for ppc targets

[Thomas Huth]

On 06/04/2021 08.11, David Gibson wrote:

On Mon, Apr 05, 2021 at 01:32:18PM +, Bruno Piazera Larsen wrote:

[...]

 * exclude 8 files from the build (dfp_helper.c, excp_helper.c,
   fpu_helper.c, int_helper.c, mem_helper.c, misc_helper.c, *
   translate.c, timebase_helper.c);


That looks about right.


IIRC you don't only have to exclude translate.c from the build, you also 
have to separate translate_init.c.inc from it, i.e. turn 
translate_init.c.inc into a proper .c file and get rid of the #include 
"translate_init.c.inc" statement in translate.c, since many functions in the 
translate_init.c.inc file are still needed for the KVM-only builds, too. So 
maybe that's a good place to start as a first mini series.


 Thomas

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Thomas Huth]

On 09/04/2021 18.03, Greg Kurz wrote:

Despite its simple name and common usage of "getting a pointer to
the machine" in system-mode emulation, qdev_get_machine() has some
subtilities.

First, it can be called when running user-mode emulation : this is
because user-mode partly relies on qdev to instantiate its CPU
model.

Second, but not least, it has a side-effect : if it cannot find an
object at "/machine" in the QOM tree, it creates a dummy "container"
object and put it there. A simple check on the type returned by
qdev_get_machine() allows user-mode to run the common qdev code,
skipping the parts that only make sense for system-mode.

This side-effect turns out to complicate the use of qdev_get_machine()
for the system-mode case though. Most notably, qdev_get_machine() must
not be called before the machine object is added to the QOM tree by
qemu_create_machine(), otherwise the existing dummy "container" object
would cause qemu_create_machine() to fail with something like :

Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
qemu-system-ppc64: attempt to add duplicate property 'machine' to
  object (type 'container')
Aborted (core dumped)

This situation doesn't exist in the current code base, mostly because
of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
and e2fb3fbbf9c for details).

A new kind of breakage was spotted very recently though :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
/home/thuth/devel/qemu/include/hw/boards.h:24:
  MACHINE: Object 0x5635bd53af10 is not an instance of type machine
Aborted (core dumped)

This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
added a new condition for qdev_get_machine() to be called too early,
breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
time.

In order to avoid further subtle breakages like this, change the
implentation of qdev_get_machine() to:
- keep the existing behaviour of creating the dummy "container"
   object for the user-mode case only ;
- abort() if the machine doesn't exist yet in the QOM tree for
   the system-mode case. This gives a precise hint to developpers
   that calling qdev_get_machine() too early is a programming bug.

This is achieved with a new do_qdev_get_machine() function called
from qdev_get_machine(), with different implementations for system
and user mode.

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
  qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

Reported-by: Thomas Huth 
Signed-off-by: Greg Kurz 
---
  hw/core/machine.c| 14 ++
  hw/core/qdev.c   |  2 +-
  include/hw/qdev-core.h   |  1 +
  stubs/meson.build|  1 +
  stubs/qdev-get-machine.c | 11 +++
  5 files changed, 28 insertions(+), 1 deletion(-)
  create mode 100644 stubs/qdev-get-machine.c

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 40def78183a7..fecca4023105 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
  register_global_state();
  }
  
+Object *do_qdev_get_machine(void)

+{
+Object *machine;
+
+machine = object_resolve_path_component(object_get_root(), "machine");
+/*
+ * qdev_get_machine() shouldn't be called before qemu_create_machine()
+ * has created the "/machine" path.
+ */
+assert(machine != NULL);
+
+return machine;
+}
+
  static const TypeInfo machine_info = {
  .name = TYPE_MACHINE,
  .parent = TYPE_OBJECT,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a92..1122721b2bf0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
  static Object *dev;
  
  if (dev == NULL) {

-dev = container_get(object_get_root(), "/machine");
+dev = do_qdev_get_machine();
  }
  
  return dev;

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa1b..90e295e0bc1a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
  
  void qdev_assert_realized_properly(void);

  Object *qdev_get_machine(void);
+Object *do_qdev_get_machine(void);
  
  /* FIXME: make this a link<> */

  bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/stubs/meson.build b/stubs/meson.build
index be6f6d609e58..b99ee2b33e94 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -54,3 +54,4 @@ if have_system
  else
stub_ss.add(files('qdev.c'))
  endif
+stub_ss.add(files('qdev-get-machine.c'))
diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
new file mode 100644
index ..ed4cdaa01900
--- /dev/null
+++ b/stubs/qdev-get-machine.c
@@ -0,0 +1,11 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+
+Object *do_qdev_get_machine(void)
+{
+/*
+ * This will create a "container" and add it 

Re: [PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Thomas Huth]

On 09/04/2021 18.03, Greg Kurz wrote:

Calling qdev_get_machine() from a QOM instance_init function is
fragile because we can't be sure the machine object actually
exists. And this happens to break when passing ",help" on the
command line to get the list of properties for a CPU core
device types :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
  qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

This used to work before QEMU 5.0, but commit 3df261b6676b
unwillingly introduced a subtle regression : the above command
line needs to create an instance but the instance_init function
of the base class calls qdev_get_machine() before
qemu_create_machine() has been called, which is a programming bug.

Use current_machine instead. It is okay to skip the setting of
nr_thread in this case since only its type is displayed.

Reported-by: Thomas Huth 
Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no 
default machine'")
Cc: peter.mayd...@linaro.org
Signed-off-by: Greg Kurz 
---
  hw/cpu/core.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index 92d3b2fbad62..987607515574 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor 
*v, const char *name,
  
  static void cpu_core_instance_init(Object *obj)

  {
-MachineState *ms = MACHINE(qdev_get_machine());
  CPUCore *core = CPU_CORE(obj);
  
-core->nr_threads = ms->smp.threads;

+/*
+ * Only '-device something-cpu-core,help' can get us there before
+ * the machine has been created. We don't care to set nr_threads
+ * in this case since it isn't used afterwards.
+ */
+if (current_machine) {
+core->nr_threads = current_machine->smp.threads;
+}
  }


Ack for QEMU 6.0 to get rid of the crash. But note that using 
current_machine might also be wrong in some cases. It's e.g. possible that 
the user started QEMU with a different machine type (e.g. -M g3beige) and 
then uses some QOM commands to introspect the available devices - in that 
case, this instance_init function will be executed with current_machine 
pointing to a G3 Mac - which is certainly also not what we want here. It 
likely does not crash, but still ... using current_machine or 
qdev_get_machine() in an instance_init() function is just wrong. This 
nr_thread stuff should likely be done in the realize function instead.


 Thomas

Re: [PATCH 2/2] docs/devel/qgraph: add troubleshooting information

[Thomas Huth]

On 09/04/2021 21.01, Stefan Hajnoczi wrote:

It can be tricky to troubleshoot qos-test when a test won't execute. Add
an explanation of how to trace qgraph node connectivity and find which
node has the problem.

Cc: Emanuele Giuseppe Esposito 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
  docs/devel/qgraph.rst | 58 +++
  1 file changed, 58 insertions(+)

diff --git a/docs/devel/qgraph.rst b/docs/devel/qgraph.rst
index a9aff167ad..4635efb2c2 100644
--- a/docs/devel/qgraph.rst
+++ b/docs/devel/qgraph.rst
@@ -92,6 +92,64 @@ The basic framework steps are the following:
  Depending on the QEMU binary used, only some drivers/machines will be
  available and only test that are reached by them will be executed.
  
+Troubleshooting unavailable tests

+^
+If there is no path from an available machine to a test then that test will
+unavailable and cannot execute. This can happen if a test or driver did not set


"will be unavailable" ? or "will be marked as unavailable" ?

Apart from that, patch looks fine to me.

 Thomas

gitlab-ci check-dco test

[Richard Henderson]

On development branches, it's not uncommon to push
temporary --fixup patches, and normally one doesn't
sign those.  But then of course one get hate-mail
from the gitlab-ci job about the failing test.

Is there a way to make it fatal on staging, but
merely a warning on other branches (a-la checkpatch)?


r~

Re: [RFC PATCH-for-6.1 v2 6/6] hw/mips/loongson3_virt: Raise CPU clock to 2 GHz

[Huacai Chen]

Hi, Philippe,

On Fri, Apr 9, 2021 at 5:36 PM Philippe Mathieu-Daudé  wrote:
>
> Commit cd3a53b727d ("clock: Add clock_ns_to_ticks() function")
> removed the limitation of using clock with a frequency of 1 GHz
> or more.
>
> The previous commit converted the MIPS CP0 timer to use this
> new clock_ns_to_ticks() function. We can now use a clock of
> 2 GHz with the Loongson3 virt board.
Yes, we can do this, but why should we do this? I don't think this can
bring any advantages.

Huacai
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/mips/loongson3_virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
> index b15071defc6..0b72ef8a684 100644
> --- a/hw/mips/loongson3_virt.c
> +++ b/hw/mips/loongson3_virt.c
> @@ -152,7 +152,7 @@ static const MemoryRegionOps loongson3_pm_ops = {
>  }
>  };
>
> -#define DEF_LOONGSON3_FREQ (800 * 1000 * 1000)
> +#define DEF_LOONGSON3_FREQ (2000 * 1000 * 1000)
>
>  static uint64_t get_cpu_freq_hz(void)
>  {
> --
> 2.26.3
>

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 2:20 PM Philippe Mathieu-Daudé  wrote:
>
> +Paolo/Thomas
>
> On 4/9/21 7:21 PM, Patrick Venture wrote:
> > On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé  
> > wrote:
> >>
> >> Hi Patrick,
> >>
> >> On 4/9/21 6:25 PM, Patrick Venture wrote:
> >>> The pca954x is an i2c mux, and this adds support for two variants of
> >>> this device: the pca9546 and pca9548.
> >>>
> >>> This device is very common on BMCs to route a different channel to each
> >>> PCIe i2c bus downstream from the BMC.
> >>>
> >>> Signed-off-by: Patrick Venture 
> >>> Reviewed-by: Hao Wu 
> >>> Reviewed-by: Havard Skinnemoen 
> >>> ---
> >>>  MAINTAINERS  |   6 +
> >>>  hw/i2c/Kconfig   |   4 +
> >>>  hw/i2c/i2c_mux_pca954x.c | 290 +++
> >>>  hw/i2c/meson.build   |   1 +
> >>>  hw/i2c/trace-events  |   5 +
> >>>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >>>  6 files changed, 325 insertions(+)
> >>>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >>>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >>
> >>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> >>> index 09642a6dcb..8d120a25d5 100644
> >>> --- a/hw/i2c/Kconfig
> >>> +++ b/hw/i2c/Kconfig
> >>> @@ -28,3 +28,7 @@ config IMX_I2C
> >>>  config MPC_I2C
> >>>  bool
> >>>  select I2C
> >>> +
> >>> +config PCA954X
> >>> +bool
> >>> +select I2C
> >>
> >> Do you have a circular dependency when also using:
> >>
> >>depends on I2C
> >>
> >> ?
> >
> > I'm somewhat new to qemu -- I don't know what you mean, since I2C
> > doesn't depend on pca954x, I don't imagine there could be a circular
> > dependency.
>
> See
> https://qemu-project.gitlab.io/qemu/devel/kconfig.html#guidelines-for-writing-kconfig-files
>
> PCA954X is plugged on an I2C bus
> -> depends on I2C
>
> PCA954X provides I2C buses
> -> select I2C

So from the guide it looks like my KConfig should have _depends_ on
I2C.  My board that I'm testing with selects PCA954X and doesn't
explicitly select I2C.  My device _does_ provide I2C buses, as you
say.

>
> Your device is a particular case consuming and providing the Kconfig
> 'I2C' symbol. I expect a circular dependency problem. Easy to test with
> your series but I haven't.
>
> I suppose in this case, the "select" takes over on "depends on" so this
> is OK.

I have to imagine there is a similar situation for PCIe bridges, as
they depend on PCI but also provide it.

>
> Now (unrelated to your series) thinking at the graphical Kconfig tree
> representation (like this one generated 2 years ago:
> https://drive.google.com/open?id=1kvwl7guuAmCh2Y2UqeXynlA2HmjWcRs9),
> I'd rather see a circular dep.
>
> Regards,
>
> Phil.

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Philippe Mathieu-Daudé]

+Paolo/Thomas

On 4/9/21 7:21 PM, Patrick Venture wrote:
> On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé  wrote:
>>
>> Hi Patrick,
>>
>> On 4/9/21 6:25 PM, Patrick Venture wrote:
>>> The pca954x is an i2c mux, and this adds support for two variants of
>>> this device: the pca9546 and pca9548.
>>>
>>> This device is very common on BMCs to route a different channel to each
>>> PCIe i2c bus downstream from the BMC.
>>>
>>> Signed-off-by: Patrick Venture 
>>> Reviewed-by: Hao Wu 
>>> Reviewed-by: Havard Skinnemoen 
>>> ---
>>>  MAINTAINERS  |   6 +
>>>  hw/i2c/Kconfig   |   4 +
>>>  hw/i2c/i2c_mux_pca954x.c | 290 +++
>>>  hw/i2c/meson.build   |   1 +
>>>  hw/i2c/trace-events  |   5 +
>>>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>>>  6 files changed, 325 insertions(+)
>>>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>>>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
>>
>>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
>>> index 09642a6dcb..8d120a25d5 100644
>>> --- a/hw/i2c/Kconfig
>>> +++ b/hw/i2c/Kconfig
>>> @@ -28,3 +28,7 @@ config IMX_I2C
>>>  config MPC_I2C
>>>  bool
>>>  select I2C
>>> +
>>> +config PCA954X
>>> +bool
>>> +select I2C
>>
>> Do you have a circular dependency when also using:
>>
>>depends on I2C
>>
>> ?
> 
> I'm somewhat new to qemu -- I don't know what you mean, since I2C
> doesn't depend on pca954x, I don't imagine there could be a circular
> dependency.

See
https://qemu-project.gitlab.io/qemu/devel/kconfig.html#guidelines-for-writing-kconfig-files

PCA954X is plugged on an I2C bus
-> depends on I2C

PCA954X provides I2C buses
-> select I2C

Your device is a particular case consuming and providing the Kconfig
'I2C' symbol. I expect a circular dependency problem. Easy to test with
your series but I haven't.

I suppose in this case, the "select" takes over on "depends on" so this
is OK.

Now (unrelated to your series) thinking at the graphical Kconfig tree
representation (like this one generated 2 years ago:
https://drive.google.com/open?id=1kvwl7guuAmCh2Y2UqeXynlA2HmjWcRs9),
I'd rather see a circular dep.

Regards,

Phil.

Re: [PATCH 2/4] target/ppc: added solutions for building with disable-tcg

[Fabiano Rosas]

"Bruno Larsen (billionai)"  writes:

> this commit presents 2 possible solutions for substituting TCG emulation
> with KVM calls. One - used in machine.c and arch_dump.c - explicitly

As I mentioned in my reply to your introductory email I don't think we
should be replacing TCG routines with calls into KVM. Because that would
mean we have been up until now running KVM-supporting code but relying
on TCG routines which would (aside the most simple functions) be
wrong. The whole KVM runs natively, TCG is translated deal.

I don't see what in the vscr helpers makes them TCG-only. They seem like
they would work just fine with KVM code. The kvm_arch_get/put_registers
functions should already take care of synchronizing the CPUPPCState with
KVM.

Does that make sense? Tell me if I'm missing something. =)

> adds the KVM function and has the possibility of adding the TCG one
> for more generic compilation, prioritizing te KVM option. The second
> option, implemented in kvm_ppc.h, transparently changes the helper
> into the KVM call, if TCG is not enabled. I believe the first solution
> is better, but it is less readable, so I wanted to have some feedback
>
> Signed-off-by: Bruno Larsen (billionai) 
> ---
>  target/ppc/arch_dump.c | 17 +
>  target/ppc/kvm.c   | 30 ++
>  target/ppc/kvm_ppc.h   | 11 +++
>  target/ppc/machine.c   | 33 -
>  4 files changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
> index 9ab04b2c38..c53e01011a 100644
> --- a/target/ppc/arch_dump.c
> +++ b/target/ppc/arch_dump.c
> @@ -17,7 +17,10 @@
>  #include "elf.h"
>  #include "sysemu/dump.h"
>  #include "sysemu/kvm.h"
> +#include "kvm_ppc.h"
> +#if defined(CONFIG_TCG)
>  #include "exec/helper-proto.h"
> +#endif /* CONFIG_TCG */
>  
>  #ifdef TARGET_PPC64
>  #define ELFCLASS ELFCLASS64
> @@ -176,7 +179,21 @@ static void ppc_write_elf_vmxregset(NoteFuncArg *arg, 
> PowerPCCPU *cpu)
>  vmxregset->avr[i].u64[1] = avr->u64[1];
>  }
>  }
> +/* This is the first solution implemented. My personal favorite as it
> + * allows for explicit error handling, however it is much less readable 
> */
> +#if defined(CONFIG_KVM)
> +if(kvm_enabled()){
> +vmxregset->vscr.u32[3] = cpu_to_dump32(s, kvmppc_mfvscr(cpu));
> +}else
> +#endif
> +
> +#if defined(CONFIG_TCG)
>  vmxregset->vscr.u32[3] = cpu_to_dump32(s, helper_mfvscr(>env));
> +#else
> +{
> +/* TODO: add proper error handling, even tough this should never be 
> reached */
> +}
> +#endif
>  }
>  
>  static void ppc_write_elf_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 104a308abb..8ed54d12d8 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -51,6 +51,7 @@
>  #include "elf.h"
>  #include "sysemu/kvm_int.h"
>  
> +
>  #define PROC_DEVTREE_CPU  "/proc/device-tree/cpus/"
>  
>  #define DEBUG_RETURN_GUEST 0
> @@ -2947,3 +2948,32 @@ bool kvm_arch_cpu_check_are_resettable(void)
>  {
>  return true;
>  }
> +
> +/* Functions added to replace helper_m(t|f)vscr from int_helper.c */
> +int kvmppc_mtvscr(PowerPCCPU *cpu, uint32_t val){
> +CPUState *cs = CPU(cpu);
> +CPUPPCState *env = >env;
> +struct kvm_one_reg reg;
> +int ret;
> +reg.id = KVM_REG_PPC_VSCR;
> +reg.addr = (uintptr_t) >vscr;
> +ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, );
> +if(ret < 0){
> +fprintf(stderr, "Unable to set VSCR to KVM: %s", strerror(errno));
> +}
> +return ret;
> +}
> +
> +int kvmppc_mfvscr(PowerPCCPU *cpu){
> +CPUState *cs = CPU(cpu);
> +CPUPPCState *env = >env;
> +struct kvm_one_reg reg;
> +int ret;
> +reg.id = KVM_REG_PPC_VSCR;
> +reg.addr = (uintptr_t) >vscr;
> +ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, );
> +if(ret < 0){
> +fprintf(stderr, "Unable to get VSCR to KVM: %s", strerror(errno));
> +}
> +return ret;
> +}
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 989f61ace0..f618cb28b1 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -86,6 +86,17 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t 
> tb_offset);
>  
>  int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
>  
> +int kvmppc_mtvscr(PowerPCCPU*, uint32_t);
> +int kvmppc_mfvscr(PowerPCCPU*);
> +
> +/* This is the second (quick and dirty) solution. Not my personal favorite
> + * as it hides what is actually happening from the user and doesn't allow
> + * for error checking. but it requires less change in other files */
> +#ifndef CONFIG_TCG
> +#define helper_mtvscr(env, val) kvmppc_mtvscr(env_archcpu(env),val)
> +#define helper_mfvscr(env) kvmppc_mfvscr(env_archcpu(env))
> +#endif
> +
>  #else
>  
>  static inline uint32_t kvmppc_get_tbfreq(void)
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 283db1d28a..d92bc18859 100644
> --- 

[PULL 1/2] i386: Add missing cpu feature bits in EPYC-Rome model

[Eduardo Habkost]

From: Babu Moger 

Found the following cpu feature bits missing from EPYC-Rome model.
ibrs: Indirect Branch Restricted Speculation
ssbd: Speculative Store Bypass Disable

These new features will be added in EPYC-Rome-v2. The -cpu help output
after the change.

x86 EPYC-Rome (alias configured by machine type)
x86 EPYC-Rome-v1  AMD EPYC-Rome Processor
x86 EPYC-Rome-v2  AMD EPYC-Rome Processor

Reported-by: Pankaj Gupta 
Signed-off-by: Babu Moger 
Signed-off-by: Pankaj Gupta 
Signed-off-by: Eduardo Habkost 
Reviewed-by: David Edmondson 
Message-Id: <161478622280.16275.6399866734509127420.stgit@bmoger-ubuntu>
Signed-off-by: Eduardo Habkost 
---
 target/i386/cpu.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6b3e9467f17..ad99cad0e7c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4179,6 +4179,18 @@ static X86CPUDefinition builtin_x86_defs[] = {
 .xlevel = 0x801E,
 .model_id = "AMD EPYC-Rome Processor",
 .cache_info = _rome_cache_info,
+.versions = (X86CPUVersionDefinition[]) {
+{ .version = 1 },
+{
+.version = 2,
+.props = (PropValue[]) {
+{ "ibrs", "on" },
+{ "amd-ssbd", "on" },
+{ /* end of list */ }
+}
+},
+{ /* end of list */ }
+}
 },
 {
 .name = "EPYC-Milan",
-- 
2.30.2

[PULL 2/2] cpu/core: Fix "help" of CPU core device types

[Eduardo Habkost]

From: Greg Kurz 

Calling qdev_get_machine() from a QOM instance_init function is
fragile because we can't be sure the machine object actually
exists. And this happens to break when passing ",help" on the
command line to get the list of properties for a CPU core
device types :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
 qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

This used to work before QEMU 5.0, but commit 3df261b6676b
unwillingly introduced a subtle regression : the above command
line needs to create an instance but the instance_init function
of the base class calls qdev_get_machine() before
qemu_create_machine() has been called, which is a programming bug.

Use current_machine instead. It is okay to skip the setting of
nr_thread in this case since only its type is displayed.

Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' 
before 'no default machine'")
Reported-by: Thomas Huth 
Signed-off-by: Greg Kurz 
Cc: peter.mayd...@linaro.org
Message-Id: <20210409160339.500167-3-gr...@kaod.org>
Signed-off-by: Eduardo Habkost 
---
 hw/cpu/core.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index 92d3b2fbad6..98760751557 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor 
*v, const char *name,
 
 static void cpu_core_instance_init(Object *obj)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 CPUCore *core = CPU_CORE(obj);
 
-core->nr_threads = ms->smp.threads;
+/*
+ * Only '-device something-cpu-core,help' can get us there before
+ * the machine has been created. We don't care to set nr_threads
+ * in this case since it isn't used afterwards.
+ */
+if (current_machine) {
+core->nr_threads = current_machine->smp.threads;
+}
 }
 
 static void cpu_core_class_init(ObjectClass *oc, void *data)
-- 
2.30.2

[PULL 0/2] x86 and CPU bug fixes for 6.0-rc3

[Eduardo Habkost]

The following changes since commit 471387aa1446e2583f372f79327cc0a8c802b4b4:

  Merge remote-tracking branch 'remotes/cohuck-gitlab/tags/s390x-20210409' into 
staging (2021-04-09 17:21:18 +0100)

are available in the Git repository at:

  https://gitlab.com/ehabkost/qemu.git tags/x86-next-pull-request

for you to fetch changes up to 0b47ec4b95ad1952e55e639711d442f8ec6e1345:

  cpu/core: Fix "help" of CPU core device types (2021-04-09 16:05:16 -0400)


x86 and CPU bug fixes for 6.0-rc3

* Add missing features to EPYC-Rome CPU model (Babu Moger)
* Fix crash with "-device ...-cpu-core,help" (Greg Kurz)



Babu Moger (1):
  i386: Add missing cpu feature bits in EPYC-Rome model

Greg Kurz (1):
  cpu/core: Fix "help" of CPU core device types

 hw/cpu/core.c | 10 --
 target/i386/cpu.c | 12 
 2 files changed, 20 insertions(+), 2 deletions(-)

-- 
2.30.2

[PATCH v3 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

The pca954x is an i2c mux, and this adds support for two variants of
this device: the pca9546 and pca9548.

This device is very common on BMCs to route a different channel to each
PCIe i2c bus downstream from the BMC.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
Reviewed-by: Havard Skinnemoen 
---
 MAINTAINERS  |   6 +
 hw/i2c/Kconfig   |   4 +
 hw/i2c/i2c_mux_pca954x.c | 289 +++
 hw/i2c/meson.build   |   1 +
 hw/i2c/trace-events  |   5 +
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 6 files changed, 324 insertions(+)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 58f342108e..5ea0b60b8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2039,6 +2039,12 @@ S: Maintained
 F: hw/net/tulip.c
 F: hw/net/tulip.h
 
+pca954x
+M: Patrick Venture 
+S: Maintained
+F: hw/i2c/i2c_mux_pca954x.c
+F: include/hw/i2c/i2c_mux_pca954x.h
+
 Generic Loader
 M: Alistair Francis 
 S: Maintained
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 09642a6dcb..8d120a25d5 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -28,3 +28,7 @@ config IMX_I2C
 config MPC_I2C
 bool
 select I2C
+
+config PCA954X
+bool
+select I2C
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
new file mode 100644
index 00..a716810e02
--- /dev/null
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -0,0 +1,289 @@
+/*
+ * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c_mux_pca954x.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/queue.h"
+#include "qom/object.h"
+#include "trace.h"
+
+#define PCA9548_CHANNEL_COUNT 8
+#define PCA9546_CHANNEL_COUNT 4
+
+/*
+ * struct Pca954xChannel - The i2c mux device will have N of these states
+ * that own the i2c channel bus.
+ * @bus: The owned channel bus.
+ * @enabled: Is this channel active?
+ */
+typedef struct Pca954xChannel {
+SysBusDevice parent;
+
+I2CBus   *bus;
+
+bool enabled;
+} Pca954xChannel;
+
+#define TYPE_PCA954X_CHANNEL "pca954x-channel"
+#define PCA954X_CHANNEL(obj) \
+OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
+
+/*
+ * struct Pca954xState - The pca954x state object.
+ * @control: The value written to the mux control.
+ * @channel: The set of i2c channel buses that act as channels which own the
+ * i2c children.
+ */
+typedef struct Pca954xState {
+SMBusDevice parent;
+
+uint8_t control;
+
+/* The channel i2c buses. */
+Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+} Pca954xState;
+
+/*
+ * struct Pca954xClass - The pca954x class object.
+ * @nchans: The number of i2c channels this device has.
+ */
+typedef struct Pca954xClass {
+SMBusDeviceClass parent;
+
+uint8_t nchans;
+} Pca954xClass;
+
+#define TYPE_PCA954X "pca954x"
+OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
+
+/*
+ * For each channel, if it's enabled, recursively call match on those children.
+ */
+static bool pca954x_match(I2CSlave *candidate, uint8_t address,
+  bool broadcast,
+  I2CNodeList *current_devs)
+{
+Pca954xState *mux = PCA954X(candidate);
+Pca954xClass *mc = PCA954X_GET_CLASS(mux);
+int i;
+
+/* They are talking to the mux itself (or all devices enabled). */
+if ((candidate->address == address) || broadcast) {
+I2CNode *node = g_malloc(sizeof(struct I2CNode));
+node->elt = candidate;
+QLIST_INSERT_HEAD(current_devs, node, next);
+if (!broadcast) {
+return true;
+}
+}
+
+for (i = 0; i < mc->nchans; i++) {
+if (!mux->channel[i].enabled) {
+continue;
+}
+
+if (i2c_scan_bus(mux->channel[i].bus, address, broadcast)) {
+if (!broadcast) {
+return true;
+}
+}
+}
+
+/* If we arrived here we didn't find a match, return broadcast. */
+return broadcast;
+}
+
+static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
+{
+Pca954xClass *mc = PCA954X_GET_CLASS(s);
+int i;
+
+/*
+ * For each channel, check if their bit is set in enable_mask 

[PATCH v3 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Patrick Venture]

The i2c mux device pca954x implements two devices:
 - the pca9546 and pca9548.

v3:
 - fixup comment with missing end parenthesis.
 - removed superfluous object cast.

v2:
 - the core i2c bus now calls a match method on each i2c child, which
 by default will only check for a match against itself.
 - the pca954x device overrides the i2c device match method to search
 the children for each of its buses that are active.
 - the pca954x device now owns an i2c bus for each channel, allowing
 the normal device model to attach devices to the channels.

Patrick Venture (4):
  hw/i2c: name I2CNode list in I2CBus
  hw/i2c: add match method for device search
  hw/i2c: move search to i2c_scan_bus method
  hw/i2c: add pca954x i2c-mux switch

 MAINTAINERS  |   6 +
 hw/i2c/Kconfig   |   4 +
 hw/i2c/core.c|  55 --
 hw/i2c/i2c_mux_pca954x.c | 289 +++
 hw/i2c/meson.build   |   1 +
 hw/i2c/trace-events  |   5 +
 include/hw/i2c/i2c.h |  16 +-
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 8 files changed, 381 insertions(+), 14 deletions(-)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v3 3/4] hw/i2c: move search to i2c_scan_bus method

[Patrick Venture]

Moves the search for matching devices on an i2c bus into a separate
method.  This allows for an object that owns an I2CBus can avoid
duplicating this method.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
---
 hw/i2c/core.c| 38 ++
 include/hw/i2c/i2c.h |  1 +
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d03b0eea5c..12981cea2d 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -77,6 +77,30 @@ int i2c_bus_busy(I2CBus *bus)
 return !QLIST_EMPTY(>current_devs);
 }
 
+bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast)
+{
+BusChild *kid;
+
+QTAILQ_FOREACH(kid, >qbus.children, sibling) {
+DeviceState *qdev = kid->child;
+I2CSlave *candidate = I2C_SLAVE(qdev);
+I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(candidate);
+
+if (sc->match_and_add(candidate, address, broadcast,
+  >current_devs)) {
+if (!broadcast) {
+return true;
+}
+}
+}
+
+/*
+ * If broadcast was true, and the list was full or empty, return true. If
+ * broadcast was false, return false.
+ */
+return broadcast;
+}
+
 /* TODO: Make this handle multiple masters.  */
 /*
  * Start or continue an i2c transaction.  When this is called for the
@@ -93,7 +117,6 @@ int i2c_bus_busy(I2CBus *bus)
  */
 int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 {
-BusChild *kid;
 I2CSlaveClass *sc;
 I2CNode *node;
 bool bus_scanned = false;
@@ -115,17 +138,8 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
recv)
  * terminating the previous transaction.
  */
 if (QLIST_EMPTY(>current_devs)) {
-QTAILQ_FOREACH(kid, >qbus.children, sibling) {
-DeviceState *qdev = kid->child;
-I2CSlave *candidate = I2C_SLAVE(qdev);
-sc = I2C_SLAVE_GET_CLASS(candidate);
-if (sc->match_and_add(candidate, address, bus->broadcast,
-  >current_devs)) {
-if (!bus->broadcast) {
-break;
-}
-}
-}
+/* Disregard whether devices were found. */
+(void)i2c_scan_bus(bus, address, bus->broadcast);
 bus_scanned = true;
 }
 
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b8b95ff4a..45915f3303 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -87,6 +87,7 @@ void i2c_nack(I2CBus *bus);
 int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
 int i2c_send(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
+bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast);
 
 /**
  * Create an I2C slave device on the heap.
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v3 2/4] hw/i2c: add match method for device search

[Patrick Venture]

At the start of an i2c transaction, the i2c bus searches its list of
children to identify which devices correspond to the address (or
broadcast).  Now the I2CSlave device has a method "match" that
encapsulates the lookup behavior. This allows the behavior to be changed
to support devices, such as i2c muxes.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
---
 hw/i2c/core.c| 23 +++
 include/hw/i2c/i2c.h | 11 +++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 21ec52ac5a..d03b0eea5c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -118,10 +118,9 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
recv)
 QTAILQ_FOREACH(kid, >qbus.children, sibling) {
 DeviceState *qdev = kid->child;
 I2CSlave *candidate = I2C_SLAVE(qdev);
-if ((candidate->address == address) || (bus->broadcast)) {
-node = g_malloc(sizeof(struct I2CNode));
-node->elt = candidate;
-QLIST_INSERT_HEAD(>current_devs, node, next);
+sc = I2C_SLAVE_GET_CLASS(candidate);
+if (sc->match_and_add(candidate, address, bus->broadcast,
+  >current_devs)) {
 if (!bus->broadcast) {
 break;
 }
@@ -290,12 +289,28 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char 
*name, uint8_t addr)
 return dev;
 }
 
+static bool i2c_slave_match(I2CSlave *candidate, uint8_t address,
+bool broadcast, I2CNodeList *current_devs)
+{
+if ((candidate->address == address) || (broadcast)) {
+I2CNode *node = g_malloc(sizeof(struct I2CNode));
+node->elt = candidate;
+QLIST_INSERT_HEAD(current_devs, node, next);
+return true;
+}
+
+/* Not found and not broadcast. */
+return false;
+}
+
 static void i2c_slave_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
+I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
 set_bit(DEVICE_CATEGORY_MISC, k->categories);
 k->bus_type = TYPE_I2C_BUS;
 device_class_set_props(k, i2c_props);
+sc->match_and_add = i2c_slave_match;
 }
 
 static const TypeInfo i2c_slave_type_info = {
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 1f7c268c86..9b8b95ff4a 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -16,6 +16,7 @@ enum i2c_event {
 I2C_NACK /* Masker NACKed a receive byte.  */
 };
 
+typedef struct I2CNodeList I2CNodeList;
 
 #define TYPE_I2C_SLAVE "i2c-slave"
 OBJECT_DECLARE_TYPE(I2CSlave, I2CSlaveClass,
@@ -39,6 +40,16 @@ struct I2CSlaveClass {
  * return code is not used and should be zero.
  */
 int (*event)(I2CSlave *s, enum i2c_event event);
+
+/*
+ * Check if this device matches the address provided.  Returns bool of
+ * true if it matches (or broadcast), and updates the device list, false
+ * otherwise.
+ *
+ * If broadcast is true, match should add the device and return true.
+ */
+bool (*match_and_add)(I2CSlave *candidate, uint8_t address, bool broadcast,
+  I2CNodeList *current_devs);
 };
 
 struct I2CSlave {
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v3 1/4] hw/i2c: name I2CNode list in I2CBus

[Patrick Venture]

To enable passing the current_devs field as a parameter, we need to use
a named struct type.

Tested: BMC firmware with i2c devices booted to userspace.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/hw/i2c/i2c.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 277dd9f2d6..1f7c268c86 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -58,9 +58,11 @@ struct I2CNode {
 QLIST_ENTRY(I2CNode) next;
 };
 
+typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
+
 struct I2CBus {
 BusState qbus;
-QLIST_HEAD(, I2CNode) current_devs;
+I2CNodeList current_devs;
 uint8_t saved_address;
 bool broadcast;
 };
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Eduardo Habkost]

On Fri, Apr 09, 2021 at 06:03:38PM +0200, Greg Kurz wrote:
[...]
> In order to avoid further subtle breakages like this, change the
> implentation of qdev_get_machine() to:
> - keep the existing behaviour of creating the dummy "container"
>   object for the user-mode case only ;
> - abort() if the machine doesn't exist yet in the QOM tree for
>   the system-mode case. This gives a precise hint to developpers
>   that calling qdev_get_machine() too early is a programming bug.
> 
> This is achieved with a new do_qdev_get_machine() function called
> from qdev_get_machine(), with different implementations for system
> and user mode.
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)

Should this to be considered for 6.0?  It doesn't seem to be a
bug fix, but just a preventive measure.

-- 
Eduardo

Re: [PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Eduardo Habkost]

On Fri, Apr 09, 2021 at 06:03:39PM +0200, Greg Kurz wrote:
> Calling qdev_get_machine() from a QOM instance_init function is
> fragile because we can't be sure the machine object actually
> exists. And this happens to break when passing ",help" on the
> command line to get the list of properties for a CPU core
> device types :
> 
> $ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
> qemu-system-ppc64: ../../hw/core/machine.c:1290:
>  qdev_get_machine: Assertion `machine != NULL' failed.
> Aborted (core dumped)
> 
> This used to work before QEMU 5.0, but commit 3df261b6676b
> unwillingly introduced a subtle regression : the above command
> line needs to create an instance but the instance_init function
> of the base class calls qdev_get_machine() before
> qemu_create_machine() has been called, which is a programming bug.
> 
> Use current_machine instead. It is okay to skip the setting of
> nr_thread in this case since only its type is displayed.
> 
> Reported-by: Thomas Huth 
> Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' 
> before 'no default machine'")
> Cc: peter.mayd...@linaro.org
> Signed-off-by: Greg Kurz 

Thanks!  I'm queueing this one (without patch 1/2) for QEMU 6.0.

-- 
Eduardo

Re: [PATCH 1/2] libqos/qgraph: fix "UNAVAILBLE" typo

[Philippe Mathieu-Daudé]

On 4/9/21 9:01 PM, Stefan Hajnoczi wrote:
> Cc: Emanuele Giuseppe Esposito 
> Cc: Paolo Bonzini 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tests/qtest/libqos/qgraph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 1/4] target/ppc: Code motion required to build disabling tcg

[Fabiano Rosas]

"Bruno Larsen (billionai)"  writes:

A general advice for this whole series is: make sure you add in some
words explaining why you decided to make a particular change. It will be
much easier to review if we know what were the logical steps leading to
the change.

> This commit does the necessary code motion from translate_init.c.inc

For instance, I don't immediately see why these changes are necessary. I
see that translate_init.c.inc already has some `#ifdef CONFIG_TCG`, so
why do we need to move a bunch of code into cpu.c instead of just adding
more code under ifdef CONFIG_TCG? (I'm not saying it's wrong, just trying to
understand the reasoning).

Is translate_init.c.inc intended to be TCG only? But then I see you
moved TCG-only functions out of it (ppc_fixup_cpu) and left not TCG-only
functions (gen_spr_generic).

> This moves all functions that start with gdb_* into target/ppc/gdbstub.c
> and creates a new function that calls those and is called by ppc_cpu_realize

This looks like it makes sense regardless of disable-tcg, could we have
it in a standalone patch?

> All functions related to realizing the cpu have been moved to cpu.c, which
> may call functions from gdbstub or translate_init

Again, I don't disagree with this, but at first sight it doesn't seem
entirely related to disabling TCG.

Re: Mac OS real USB device support issue

[Programmingkid]

> On Apr 7, 2021, at 1:28 AM, Howard Spoelstra  wrote:
> 
> On Wed, Apr 7, 2021 at 7:26 AM Howard Spoelstra  wrote:
>> 
>> On Wed, Apr 7, 2021 at 3:53 AM Programmingkid  
>> wrote:
>>> 
>>> 
>>> 
 On Apr 6, 2021, at 7:18 PM, BALATON Zoltan  wrote:
 
 On Tue, 6 Apr 2021, Programmingkid wrote:
>> On Apr 6, 2021, at 12:53 PM, BALATON Zoltan  wrote:
>> On Tue, 6 Apr 2021, Programmingkid wrote:
 On Apr 6, 2021, at 10:01 AM, Howard Spoelstra  
 wrote:
 On Tue, Apr 6, 2021 at 3:44 PM Programmingkid 
  wrote:
> 
> Hi Gerd,
> 
> I was wondering if you had access to a Mac OS 10 or Mac OS 11 machine 
> to test USB support. I am on Mac OS 11.1 and cannot make USB devices 
> work with any of my guests. So far these are the guests I have tested 
> with:
> 
> - Windows 7
> - Mac OS 9.2
> - Windows 2000
> 
> I have tried using USB flash drives, USB sound cards, and an USB 
> headset. They all show up under 'info usb', but cannot be used in the 
> guest. My setup does use a USB-C hub so I'm not sure if this is a bug 
> with QEMU or an issue with the hub. Would you have any information on 
> this issue?
 
 Hi John,
 
 As far as the Mac OS 9.2 guest is concerned on a mac OS host, it does
 not support USB 2.0. I was successful only in passing through a USB
 flash drive that was forced into USB 1.1 mode by connecting it to a
 real USB 1.1 hub and unloading the kext it used.
 
 Best,
 Howard
>>> 
>>> Hi Howard, I was actually thinking about CC'ing you for this email. 
>>> Glad you found it. Unloading kext files does not sound pleasant. Maybe 
>>> there is some better way of doing it.
>> 
>> In any case, until you make sure nothing tries to drive the device on 
>> the host, passing it to a guest likely will fail because then two 
>> drivers from two OSes would try to access it simultaneously which likely 
>> creates a mess as the device and drivers don't expect this. So you can't 
>> just pass a device through that the host has recognised and is driving 
>> without somehow getting the host to leave it alone first before you can 
>> pass it through. Unloading the driver is one way to do that (although it 
>> probably breaks all other similar devices too). Maybe there's another 
>> way to unbind a device from the host such as ejecting it first but then 
>> I'm not sure if the low level USB needed for accessing the device still 
>> works after that or it's completely forgotten. There's probably a doc 
>> somewhere that describes how it works and how can you plug a device 
>> without also getting higher level drivers to load or if there's no 
>> official ways for that then you'll need to do some configuration on the 
>> host t
 o avoid it grabbing devices that you want to pass through. On Linux you 
 can add an udev rule to ignore the device (maybe also adding 
 TAG+="uaccess" to allow console users to use it without needing root 
 access) but not sure how USB works on macOS.
>> 
>> Regards,
>> BALATON Zoltan
> 
> Being able to dissociate a real USB device from its Mac OS driver would 
> be very useful in this situation. IOKit might be one place to look for 
> such a feature. The Mach kernel documentation is another place that might 
> have what we want.
 
 Those might be a good place to start. IOKit provides the drivers and also 
 the io registry which is probably where you can get if a driver is bound 
 to a device and which one is it. How to dissociate the driver from the 
 device though I don't know.
>>> 
>>> https://developer.apple.com/library/archive/documentation/DeviceDrivers/Conceptual/IOKitFundamentals/DeviceRemoval/DeviceRemoval.html
>>> According to this article a driver has a stop() and detach() method that is 
>>> called by the IOKit to remove a device. I'm thinking QEMU can be the one 
>>> that calls these methods for a certain device.
>>> 
 
> I have one theory. What if we introduce a middleman. A pseudo-USB device 
> that the guest operating system could apply its configuration data to and 
> will also talk directly with to the real USB device.
> So this:
> 
> USB device <-> Host <-> QEMU USB middleman <-> Guest
 
 Isn't this middleman the QEMU usb-host device that we already have?
>>> 
>>> It could be. I need to research this issue some more.
>>> 
 
> This could make USB 2.0 and 3.0 flash drives compatible with an older 
> operating system like Mac OS 9. The USB middleman could fully accept Mac 
> OS 9's configuration and make it think it is talking to a USB 1.1 device. 
> Parameters like data packet payload size would no longer be a problem. 

Re: [PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Patrick Venture]

On Fri, Apr 9, 2021 at 11:31 AM Corey Minyard  wrote:
>
> On Fri, Apr 09, 2021 at 09:25:41AM -0700, Patrick Venture wrote:
> > The i2c mux device pca954x implements two devices:
> >  - the pca9546 and pca9548.
> >
> > v2:
> >  - the core i2c bus now calls a match method on each i2c child, which
> >  by default will only check for a match against itself.
> >  - the pca954x device overrides the i2c device match method to search
> >  the children for each of its buses that are active.
> >  - the pca954x device now owns an i2c bus for each channel, allowing
> >  the normal device model to attach devices to the channels.
>
> I like this design.  Avoiding hacking into the bus code is a bonus.
>
> Can these devices really have multiple channels enabled at the same
> time?  That seems strange, but I guess that could be useful.

I believe I saw that in the datasheet, and it seems reasonable that
someone might want to do that.

>
> I'm not sure if you need to add a vmstate structure for this.  In
> general most new devices have them; if it's ever included on an x86
> system (or a system with vmstate transfer capability, probably more than
> x86) that will become an issue.  I'm not sure what the expectations are,
> though.

I am perfectly willing to add the vmstate structure in a future
patchset.  My team is actively developing Qemu now for BMC automated
testing support, and we will be adding other pca mux configurations,
and other support, so this will be active.  I don't anticipate a host
system including this device yet, but that's a consideration I had not
considered.

>
> -corey
>
> >
> > Patrick Venture (4):
> >   hw/i2c: name I2CNode list in I2CBus
> >   hw/i2c: add match method for device search
> >   hw/i2c: move search to i2c_scan_bus method
> >   hw/i2c: add pca954x i2c-mux switch
> >
> >  MAINTAINERS  |   6 +
> >  hw/i2c/Kconfig   |   4 +
> >  hw/i2c/core.c|  55 --
> >  hw/i2c/i2c_mux_pca954x.c | 290 +++
> >  hw/i2c/meson.build   |   1 +
> >  hw/i2c/trace-events  |   5 +
> >  include/hw/i2c/i2c.h |  16 +-
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  8 files changed, 382 insertions(+), 14 deletions(-)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >
> > --
> > 2.31.1.295.g9ea45b61b8-goog
> >

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 11:34 AM Corey Minyard  wrote:
>
> On Fri, Apr 09, 2021 at 09:25:45AM -0700, Patrick Venture wrote:
> > The pca954x is an i2c mux, and this adds support for two variants of
> > this device: the pca9546 and pca9548.
> >
> > This device is very common on BMCs to route a different channel to each
> > PCIe i2c bus downstream from the BMC.
> >
> > Signed-off-by: Patrick Venture 
> > Reviewed-by: Hao Wu 
> > Reviewed-by: Havard Skinnemoen 
> > ---
> >  MAINTAINERS  |   6 +
> >  hw/i2c/Kconfig   |   4 +
> >  hw/i2c/i2c_mux_pca954x.c | 290 +++
> >  hw/i2c/meson.build   |   1 +
> >  hw/i2c/trace-events  |   5 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  6 files changed, 325 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 58f342108e..5ea0b60b8a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2039,6 +2039,12 @@ S: Maintained
> >  F: hw/net/tulip.c
> >  F: hw/net/tulip.h
> >
> > +pca954x
> > +M: Patrick Venture 
> > +S: Maintained
> > +F: hw/i2c/i2c_mux_pca954x.c
> > +F: include/hw/i2c/i2c_mux_pca954x.h
> > +
> >  Generic Loader
> >  M: Alistair Francis 
> >  S: Maintained
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 09642a6dcb..8d120a25d5 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -28,3 +28,7 @@ config IMX_I2C
> >  config MPC_I2C
> >  bool
> >  select I2C
> > +
> > +config PCA954X
> > +bool
> > +select I2C
> > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > new file mode 100644
> > index 00..9aa1a8872f
> > --- /dev/null
> > +++ b/hw/i2c/i2c_mux_pca954x.c
> > @@ -0,0 +1,290 @@
> > +/*
> > + * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
> > + *
> > + * Copyright 2021 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful, but 
> > WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> > + * for more details.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/i2c/i2c_mux_pca954x.h"
> > +#include "hw/i2c/smbus_slave.h"
> > +#include "hw/qdev-core.h"
> > +#include "hw/sysbus.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "qemu/queue.h"
> > +#include "qom/object.h"
> > +#include "trace.h"
> > +
> > +#define PCA9548_CHANNEL_COUNT 8
> > +#define PCA9546_CHANNEL_COUNT 4
> > +
> > +/*
> > + * struct Pca954xChannel - The i2c mux device will have N of these states
> > + * that own the i2c channel bus.
> > + * @bus: The owned channel bus.
> > + * @enabled: Is this channel active?
> > + */
> > +typedef struct Pca954xChannel {
> > +SysBusDevice parent;
> > +
> > +I2CBus   *bus;
> > +
> > +bool enabled;
> > +} Pca954xChannel;
> > +
> > +#define TYPE_PCA954X_CHANNEL "pca954x-channel"
> > +#define PCA954X_CHANNEL(obj) \
> > +OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
> > +
> > +/*
> > + * struct Pca954xState - The pca954x state object.
> > + * @control: The value written to the mux control.
> > + * @channel: The set of i2c channel buses that act as channels which own 
> > the
> > + * i2c children.
> > + */
> > +typedef struct Pca954xState {
> > +SMBusDevice parent;
> > +
> > +uint8_t control;
> > +
> > +/* The channel i2c buses. */
> > +Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
> > +} Pca954xState;
> > +
> > +/*
> > + * struct Pca954xClass - The pca954x class object.
> > + * @nchans: The number of i2c channels this device has.
> > + */
> > +typedef struct Pca954xClass {
> > +SMBusDeviceClass parent;
> > +
> > +uint8_t nchans;
> > +} Pca954xClass;
> > +
> > +#define TYPE_PCA954X "pca954x"
> > +OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
> > +
> > +/*
> > + * For each channel, if it's enabled, recursively call match on those 
> > children.
> > + */
> > +static bool pca954x_match(I2CSlave *candidate, uint8_t address,
> > +  bool broadcast,
> > +  I2CNodeList *current_devs)
> > +{
> > +Pca954xState *mux = PCA954X(candidate);
> > +Pca954xClass *mc = PCA954X_GET_CLASS(mux);
> > +int i;
> > +
> > +/* They are talking to the mux itself (or all devices enabled. */
>
> Missing close paren in the comment above.  Really minor nit :)

Ack, will fix in v3.

>
> > +if ((candidate->address == address) || broadcast) {
> > +  

[PATCH 2/2] docs/devel/qgraph: add troubleshooting information

[Stefan Hajnoczi]

It can be tricky to troubleshoot qos-test when a test won't execute. Add
an explanation of how to trace qgraph node connectivity and find which
node has the problem.

Cc: Emanuele Giuseppe Esposito 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
 docs/devel/qgraph.rst | 58 +++
 1 file changed, 58 insertions(+)

diff --git a/docs/devel/qgraph.rst b/docs/devel/qgraph.rst
index a9aff167ad..4635efb2c2 100644
--- a/docs/devel/qgraph.rst
+++ b/docs/devel/qgraph.rst
@@ -92,6 +92,64 @@ The basic framework steps are the following:
 Depending on the QEMU binary used, only some drivers/machines will be
 available and only test that are reached by them will be executed.
 
+Troubleshooting unavailable tests
+^
+If there is no path from an available machine to a test then that test will
+unavailable and cannot execute. This can happen if a test or driver did not set
+up its qgraph node correctly. It can also happen if the necessary machine type
+or device is missing from the QEMU binary because it was compiled out or
+otherwise.
+
+It is possible to troubleshoot unavailable tests by running::
+
+  $ QTEST_QEMU_BINARY=build/qemu-system-x86_64 build/tests/qtest/qos-test 
--verbose
+  # ALL QGRAPH EDGES: {
+  #   src='virtio-net'
+  #  |-> dest='virtio-net-tests/vhost-user/multiqueue' type=2 
(node=0x559142109e30)
+  #  |-> dest='virtio-net-tests/vhost-user/migrate' type=2 
(node=0x559142109d00)
+  #   src='virtio-net-pci'
+  #  |-> dest='virtio-net' type=1 (node=0x55914210d740)
+  #   src='pci-bus'
+  #  |-> dest='virtio-net-pci' type=2 (node=0x55914210d880)
+  #   src='pci-bus-pc'
+  #  |-> dest='pci-bus' type=1 (node=0x559142103f40)
+  #   src='i440FX-pcihost'
+  #  |-> dest='pci-bus-pc' type=0 (node=0x55914210ac70)
+  #   src='x86_64/pc'
+  #  |-> dest='i440FX-pcihost' type=0 (node=0x5591421117f0)
+  #   src=''
+  #  |-> dest='x86_64/pc' type=0 (node=0x559142111600)
+  #  |-> dest='arm/raspi2' type=0 (node=0x559142110740)
+  ...
+  # }
+  # ALL QGRAPH NODES: {
+  #   name='virtio-net-tests/announce-self' type=3 cmd_line='(null)' 
[available]
+  #   name='arm/raspi2' type=0 cmd_line='-M raspi2 ' [UNAVAILABLE]
+  ...
+  # }
+
+The ``virtio-net-tests/announce-self`` test is listed as "available" in the
+"ALL QGRAPH NODES" output. This means the test will execute. We can follow the
+qgraph path in the "ALL QGRAPH EDGES" output as follows: '' -> 'x86_64/pc' ->
+'i440FX-pcihost' -> 'pci-bus-pc' -> 'pci-bus' -> 'virtio-net-pci' ->
+'virtio-net'. The root of the qgraph is '' and the depth first search begins
+there.
+
+The ``arm/raspi`` machine node is listed as "UNAVAILABLE". Although it is
+reachable from the root via '' -> 'arm/raspi2' the node is unavailable because
+the QEMU binary did not list it when queried by the framework. This is expected
+because we used the ``qemu-system-x86_64`` binary which does not support ARM
+machine types.
+
+If a test is unexpectedly listed as "UNAVAILABLE", first check that the "ALL
+QGRAPH EDGES" output reports edge connectivity from the root ('') to the test.
+If there is no connectivity then the qgraph nodes were not set up correctly and
+the driver or test code is incorrect. If there is connectivity, check the
+availability of each node in the path in the "ALL QGRAPH NODES" output. The
+first unavailable node in the path is the reason why the test is unavailable.
+Typically this is because the QEMU binary lacks support for the necessary
+machine type or device.
+
 Creating a new driver and its interface
 "
 
-- 
2.30.2

[PATCH 1/2] libqos/qgraph: fix "UNAVAILBLE" typo

[Stefan Hajnoczi]

Cc: Emanuele Giuseppe Esposito 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
 tests/qtest/libqos/qgraph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/libqos/qgraph.c b/tests/qtest/libqos/qgraph.c
index b3b1a31f81..d1dc491930 100644
--- a/tests/qtest/libqos/qgraph.c
+++ b/tests/qtest/libqos/qgraph.c
@@ -844,7 +844,7 @@ void qos_dump_graph(void)
 }
 qos_printf_literal("type=%d cmd_line='%s' [%s]\n",
node->type, node->command_line,
-   node->available ? "available" : "UNAVAILBLE"
+   node->available ? "available" : "UNAVAILABLE"
 );
 }
 g_list_free(keys);
-- 
2.30.2

[PATCH 0/2] docs/devel/qgraph: add troubleshooting information

[Stefan Hajnoczi]

I recently needed to troubleshoot a case where qos-test terminated immediately
with no output. In other words, qos-test decided that no tests are runnable.

After lots of head scratching and some help from Emanuele it turned out that
the machine types weren't being detected as expected.

These patches add documentation about how to troubleshoot similar cases in the
future.

Stefan Hajnoczi (2):
  libqos/qgraph: fix "UNAVAILBLE" typo
  docs/devel/qgraph: add troubleshooting information

 docs/devel/qgraph.rst   | 58 +
 tests/qtest/libqos/qgraph.c |  2 +-
 2 files changed, 59 insertions(+), 1 deletion(-)

-- 
2.30.2

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Corey Minyard]

On Fri, Apr 09, 2021 at 09:25:45AM -0700, Patrick Venture wrote:
> The pca954x is an i2c mux, and this adds support for two variants of
> this device: the pca9546 and pca9548.
> 
> This device is very common on BMCs to route a different channel to each
> PCIe i2c bus downstream from the BMC.
> 
> Signed-off-by: Patrick Venture 
> Reviewed-by: Hao Wu 
> Reviewed-by: Havard Skinnemoen 
> ---
>  MAINTAINERS  |   6 +
>  hw/i2c/Kconfig   |   4 +
>  hw/i2c/i2c_mux_pca954x.c | 290 +++
>  hw/i2c/meson.build   |   1 +
>  hw/i2c/trace-events  |   5 +
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  6 files changed, 325 insertions(+)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58f342108e..5ea0b60b8a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2039,6 +2039,12 @@ S: Maintained
>  F: hw/net/tulip.c
>  F: hw/net/tulip.h
>  
> +pca954x
> +M: Patrick Venture 
> +S: Maintained
> +F: hw/i2c/i2c_mux_pca954x.c
> +F: include/hw/i2c/i2c_mux_pca954x.h
> +
>  Generic Loader
>  M: Alistair Francis 
>  S: Maintained
> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index 09642a6dcb..8d120a25d5 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -28,3 +28,7 @@ config IMX_I2C
>  config MPC_I2C
>  bool
>  select I2C
> +
> +config PCA954X
> +bool
> +select I2C
> diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> new file mode 100644
> index 00..9aa1a8872f
> --- /dev/null
> +++ b/hw/i2c/i2c_mux_pca954x.c
> @@ -0,0 +1,290 @@
> +/*
> + * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
> + *
> + * Copyright 2021 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/i2c_mux_pca954x.h"
> +#include "hw/i2c/smbus_slave.h"
> +#include "hw/qdev-core.h"
> +#include "hw/sysbus.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/queue.h"
> +#include "qom/object.h"
> +#include "trace.h"
> +
> +#define PCA9548_CHANNEL_COUNT 8
> +#define PCA9546_CHANNEL_COUNT 4
> +
> +/*
> + * struct Pca954xChannel - The i2c mux device will have N of these states
> + * that own the i2c channel bus.
> + * @bus: The owned channel bus.
> + * @enabled: Is this channel active?
> + */
> +typedef struct Pca954xChannel {
> +SysBusDevice parent;
> +
> +I2CBus   *bus;
> +
> +bool enabled;
> +} Pca954xChannel;
> +
> +#define TYPE_PCA954X_CHANNEL "pca954x-channel"
> +#define PCA954X_CHANNEL(obj) \
> +OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
> +
> +/*
> + * struct Pca954xState - The pca954x state object.
> + * @control: The value written to the mux control.
> + * @channel: The set of i2c channel buses that act as channels which own the
> + * i2c children.
> + */
> +typedef struct Pca954xState {
> +SMBusDevice parent;
> +
> +uint8_t control;
> +
> +/* The channel i2c buses. */
> +Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
> +} Pca954xState;
> +
> +/*
> + * struct Pca954xClass - The pca954x class object.
> + * @nchans: The number of i2c channels this device has.
> + */
> +typedef struct Pca954xClass {
> +SMBusDeviceClass parent;
> +
> +uint8_t nchans;
> +} Pca954xClass;
> +
> +#define TYPE_PCA954X "pca954x"
> +OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
> +
> +/*
> + * For each channel, if it's enabled, recursively call match on those 
> children.
> + */
> +static bool pca954x_match(I2CSlave *candidate, uint8_t address,
> +  bool broadcast,
> +  I2CNodeList *current_devs)
> +{
> +Pca954xState *mux = PCA954X(candidate);
> +Pca954xClass *mc = PCA954X_GET_CLASS(mux);
> +int i;
> +
> +/* They are talking to the mux itself (or all devices enabled. */

Missing close paren in the comment above.  Really minor nit :)

> +if ((candidate->address == address) || broadcast) {
> +I2CNode *node = g_malloc(sizeof(struct I2CNode));
> +node->elt = candidate;
> +QLIST_INSERT_HEAD(current_devs, node, next);
> +if (!broadcast) {
> +return true;
> +}
> +}
> +
> +for (i = 0; i < mc->nchans; i++) {
> +if (!mux->channel[i].enabled) {
> +continue;
> +}
> +
> +if 

Re: [PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Corey Minyard]

On Fri, Apr 09, 2021 at 09:25:41AM -0700, Patrick Venture wrote:
> The i2c mux device pca954x implements two devices:
>  - the pca9546 and pca9548.
> 
> v2:
>  - the core i2c bus now calls a match method on each i2c child, which
>  by default will only check for a match against itself.
>  - the pca954x device overrides the i2c device match method to search
>  the children for each of its buses that are active.
>  - the pca954x device now owns an i2c bus for each channel, allowing
>  the normal device model to attach devices to the channels.

I like this design.  Avoiding hacking into the bus code is a bonus.

Can these devices really have multiple channels enabled at the same
time?  That seems strange, but I guess that could be useful.

I'm not sure if you need to add a vmstate structure for this.  In
general most new devices have them; if it's ever included on an x86
system (or a system with vmstate transfer capability, probably more than
x86) that will become an issue.  I'm not sure what the expectations are,
though.

-corey

> 
> Patrick Venture (4):
>   hw/i2c: name I2CNode list in I2CBus
>   hw/i2c: add match method for device search
>   hw/i2c: move search to i2c_scan_bus method
>   hw/i2c: add pca954x i2c-mux switch
> 
>  MAINTAINERS  |   6 +
>  hw/i2c/Kconfig   |   4 +
>  hw/i2c/core.c|  55 --
>  hw/i2c/i2c_mux_pca954x.c | 290 +++
>  hw/i2c/meson.build   |   1 +
>  hw/i2c/trace-events  |   5 +
>  include/hw/i2c/i2c.h |  16 +-
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  8 files changed, 382 insertions(+), 14 deletions(-)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> 
> -- 
> 2.31.1.295.g9ea45b61b8-goog
> 

[Bug 1923197] Re: RISC-V priviledged instruction error

[Teodori Serge]

** Changed in: qemu
   Status: New => Confirmed

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1923197

Title:
  RISC-V priviledged instruction error

Status in QEMU:
  Confirmed

Bug description:
  Hello when performing an MRET with MPP set to something else than 0b11
  in MSTATUS, 'Invalid Instruction' exception will be triggered. The
  problem appeared in code after version 5.2.0. Use following code to
  test.

    # setup interrupt handling for monitor mode
    la t0, entry_loop
    la t1, entry_trap
    li t2, 0x888
    li t3, 0x1880
    csrw mepc, t0
    csrw mtvec, t1
    csrs mie, t2
    csrs mstatus, t3

    # if supervisor mode not supported, then loop forever
    csrr t0, misa
    li t1, 0x4
    and t2, t1, t0
    beqz t2, 1f

    # setup interrupt i& exception delegation for supervisor mode
    li t0, 0xc000 # 3 GiB (entry address of supervisor)
    li t1, 0x1000
    li t2, 0x300
    li t3, 0x222
    csrw mepc, t0
    csrc mstatus, t1
    csrs medeleg, t2
    csrs mideleg, t3

    # pass mhartid as first parameter to supervisor
    csrr a0, mhartid

  1:
    mret

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1923197/+subscriptions

Re: [PULL for-6.0 0/1] s390x fix

[Peter Maydell]

On Fri, 9 Apr 2021 at 15:49, Cornelia Huck  wrote:
>
> The following changes since commit d8724020dd13c88a72fc391a6a2cf63abbd3dcca:
>
>   Merge remote-tracking branch 
> 'remotes/dgilbert/tags/pull-migration-20210407b' into staging (2021-04-08 
> 14:00:57 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/cohuck/qemu.git tags/s390x-20210409
>
> for you to fetch changes up to d895d25ae2bb8519aa715dd2a97f09d4a66b189d:
>
>   s390x: css: report errors from ccw_dstream_read/write (2021-04-09 10:52:13 
> +0200)
>
> 
> One s390x fix:
> - correctly handle the case where the guest ccw payload points to
>   invalid memory areas
>
> 


Applied, thanks.

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

-- PMM

Resetting non-qdev children in a 3-phase reset device

[Peter Maydell]

I wanted to convert the hw/misc/mps2-scc.c device from old-style
to 3-phase reset as a prerequisite for another change to the device,
but I ran into a problem because currently it has some TYPE_DEVICE
QOM child objects, the LEDs. Because TYPE_DEVICE don't live in the
qbus hierarchy, the device resets them manually in its DeviceClass::reset
method:

for (i = 0; i < ARRAY_SIZE(s->led); i++) {
device_cold_reset(DEVICE(s->led[i]));
}

This makes converting to 3-phase reset awkward. The obvious "natural"
approach would be to say "in this device's phase X, invoke phase X
for these objects", but we have no API for that. (The functions which
would do it, resettable_phase_enter() etc, are static inside resettable.c.)

Ignoring the phasing and trying to just call device_cold_reset() in
the 'enter' phase results in an assertion failure, because we trip
the assert(!enter_phase_in_progress) in resettable_assert_reset(),
which doesn't expect us to be triggering a reset inside a reset.

Ideally one would want to be able to add the LEDs to the list of
things which are children of this object for purposes of reset
(so they are iterated as part of the resettable_child_foreach()
logic and their phases are automatically invoked at the right point).
But for a subclass of DeviceState that's device_reset_child_foreach()
and it only iterates any qbus children of this device.

Any clever ideas?

Maybe some mechanism for marking "these things which are my
QOM children I want to be reset when I am reset (so make them
reset children of me and don't reset them as part of the
qbus-tree-walking)" would be useful. I do think that in a
lot of cases we want to be doing something closer to "reset
along the QOM tree". I really do need to spend some time working
out what the right thing with reset is and how we might get
from where we are now to there...

thanks
-- PMM

Re: [PATCH] checkpatch: Fix use of uninitialized value

[Isaku Yamahata]

On Fri, Apr 09, 2021 at 07:40:11AM +0200,
Greg Kurz  wrote:

> On Thu, 8 Apr 2021 10:49:13 -0700
> Isaku Yamahata  wrote:
> 
> > 
> > How about initializing them explicitly as follows?
> > ($realfile ne '') prevents the case realfile eq '' && acpi_testexpted eq ''.
> > Anyway your patch also should fix it. So
> > Reviewed-by: Isaku Yamahata 
> > 
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 8f7053ec9b..2eb894a628 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1325,8 +1325,8 @@ sub process {
> > my %suppress_whiletrailers;
> > my %suppress_export;
> >  
> > -my $acpi_testexpected;
> > -my $acpi_nontestexpected;
> > +my $acpi_testexpected = '';
> > +my $acpi_nontestexpected = '';
> >  
> 
> Hmm... I haven't tried but I believe this will break when these are
> passed to checkfilename() :
> 
> sub checkfilename {
> my ($name, $acpi_testexpected, $acpi_nontestexpected) = @_;
> [...]
> if (defined $$acpi_testexpected and defined $$acpi_nontestexpected) {
> ERROR("Do not add expected files together with tests, " .
> 
> 
> > # Pre-scan the patch sanitizing the lines.

Oops. You're right. I scratch it.

-- 
Isaku Yamahata 

Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Thomas Huth]

On 06/04/2021 09.24, Klaus Jensen wrote:

On Apr  6 09:10, Philippe Mathieu-Daudé wrote:

On 4/5/21 7:54 PM, Klaus Jensen wrote:

From: Klaus Jensen 

The Non-MDTS DMSRL limit must be recomputed when namespaces are
detached.

Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
Signed-off-by: Klaus Jensen 
Reviewed-by: Gollu Appalanaidu 
---
  hw/block/nvme.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index de0e726dfdd8..3dc51f407671 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
  return NVME_NO_COMPLETE;
  }
  
+static void __nvme_update_dmrsl(NvmeCtrl *n)

+{
+int nsid;
+
+for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
+NvmeNamespace *ns = nvme_ns(n, nsid);
+if (!ns) {
+continue;
+}
+
+n->dmrsl = MIN_NON_ZERO(n->dmrsl,
+BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
+}
+}
+
  static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
  static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
  {
@@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
NvmeRequest *req)
  }
  
  nvme_ns_detach(ctrl, ns);

+
+__nvme_update_dmrsl(ctrl);
  }


Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
a convention, it makes me think of a internal macro expansion use
for preprocessor).

There are very few uses of this prefix:

hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
*path, V9fsString *buf)
hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
NvmeZone *zone,
hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
*ns, NvmeZone *zone,
hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
NvmeNamespace *ns)
hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
*flic,
hw/net/rocker/rocker_desc.c:199:static bool
__desc_ring_post_desc(DescRing *ring, int err)
hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
uint8_t phy_addr,
hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
uint64_t *nextp,
hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
*cmdname, void *data)
pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
uint32_t ccw_addr, int fmt, Irb *irb)
target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
target_ulong addr,

Thomas, Eric, is it worth cleaning these and updating the
'CODESTYLE.rst'?



Yeah ok, I think you are right that there is no clear convention on when
to use this or not. I typically just use it for functions that are
normally not supposed to be called directly.

But I don't even think its consistent in the nvme device. For my sake,
we can clean it up, I'll drop it in this case since there is no good
reason for it other than my own idea of "style".


IIRC all identifiers that start with two underscores are reserved by the C 
standard:


 https://busybox.net/~landley/c99-draft.html#7.1.3

Thus you should not use two underscores at the beginning here at all.

 HTH,
  Thomas

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé  wrote:
>
> Hi Patrick,
>
> On 4/9/21 6:25 PM, Patrick Venture wrote:
> > The pca954x is an i2c mux, and this adds support for two variants of
> > this device: the pca9546 and pca9548.
> >
> > This device is very common on BMCs to route a different channel to each
> > PCIe i2c bus downstream from the BMC.
> >
> > Signed-off-by: Patrick Venture 
> > Reviewed-by: Hao Wu 
> > Reviewed-by: Havard Skinnemoen 
> > ---
> >  MAINTAINERS  |   6 +
> >  hw/i2c/Kconfig   |   4 +
> >  hw/i2c/i2c_mux_pca954x.c | 290 +++
> >  hw/i2c/meson.build   |   1 +
> >  hw/i2c/trace-events  |   5 +
> >  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >  6 files changed, 325 insertions(+)
> >  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
>
> > diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> > index 09642a6dcb..8d120a25d5 100644
> > --- a/hw/i2c/Kconfig
> > +++ b/hw/i2c/Kconfig
> > @@ -28,3 +28,7 @@ config IMX_I2C
> >  config MPC_I2C
> >  bool
> >  select I2C
> > +
> > +config PCA954X
> > +bool
> > +select I2C
>
> Do you have a circular dependency when also using:
>
>depends on I2C
>
> ?

I'm somewhat new to qemu -- I don't know what you mean, since I2C
doesn't depend on pca954x, I don't imagine there could be a circular
dependency.

>
> > +static void pca954x_realize(DeviceState *dev, Error **errp)
> > +{
> > +Pca954xState *s = PCA954X(dev);
> > +Pca954xClass *c = PCA954X_GET_CLASS(s);
> > +int i;
> > +
> > +/* SMBus modules. Cannot fail. */
> > +for (i = 0; i < c->nchans; i++) {
> > +Object *obj = OBJECT(>channel[i]);
> > +sysbus_realize(SYS_BUS_DEVICE(obj), _abort);
>
> No need to cast to Object:
>
>sysbus_realize(SYS_BUS_DEVICE(>channel[i]), _abort);
>

Ack, will fix in v3.

> > +}
> > +}

Re: [PATCH v4 02/19] flake8: Enforce shorter line length for comments and docstrings

[John Snow]

On 4/9/21 5:33 AM, Markus Armbruster wrote:

Daniel P. Berrangé  writes:


On Fri, Mar 26, 2021 at 04:44:25PM +, Peter Maydell wrote:

On Fri, 26 Mar 2021 at 16:33, John Snow  wrote:

Being less terse about it: Mostly, I don't like how it enforces this
column width even for indented structures. Generally, we claim that 72
columns is "comfortable to read" and I agree.

 However, when we start in a margin, I
 am not convinced that this is
 actually more readable than the
 alternative. We aren't using our full
 72 characters here.


I agree, and I don't see any strong reason to hold our Python
code to a different standard to the rest of our codebase as
regards line length and comment standards.


There's one small difference with python vs the rest of the codebase when
it comes to API doc strings specifically. eg we have a docstring API comment
in python/qemu/machine.py:

class QEMUMachine:
 """
 A QEMU VM.

 Use this object as a context manager to ensure
 the QEMU process terminates::

 with VM(binary) as vm:
 ...
 # vm is guaranteed to be shut down here
 """

This formatting, including line breaks, is preserved as-is when a user
requests viewing of the help:


print(help(qemu.machine.QEMUMachine))


Help on class QEMUMachine in module qemu.machine:

class QEMUMachine(builtins.object)
  |  QEMUMachine(binary: str, args: Sequence[str] = (), wrapper: Sequence[str] 
= (), name: Optional[str] = None, test_dir: str = '/var/tmp', monitor_address: 
Union[Tuple[str, str], str, NoneType] = None, socket_scm_helper: Optional[str] 
= None, sock_dir: Optional[str] = None, drain_console: bool = False, 
console_log: Optional[str] = None)
  |
  |  A QEMU VM.
  |
  |  Use this object as a context manager to ensure
  |  the QEMU process terminates::
  |
  |  with VM(binary) as vm:
  |  ...
  |  # vm is guaranteed to be shut down here
  |
  |  Methods defined here:
  |


IOW, while we as QEMU maintainers may not care about keeping to a narrow
line width, with API docstrings, we're also declaring that none of the
users of the python APIs can care either. These docstrings are never
reflowed, so they can end up wrapping if the user's terminal is narrow
which looks very ugly.


So this python API docstring scenario is slightly different from our
main codebase, where majority of comments are only ever going to be seen
by QEMU maintainers, and where C API doc strings don't preserve formatting,
because they're turned into HTML and re-flowed.

Having said all that, I still don't think we need to restrict ourselves
to 72 characters. This is not the 1980's with people using text terminals
with physical size constraints. I think it is fine if we let python
docstrings get larger - especially if the docstrings are already indented
4/8/12 spaces due to the code indent context, because the code indent is
removed when comments are displayed. I think a 100 char line limit would
be fine and still not cause wrapping when using python live help().


The trouble with long lines is not text terminals, it's humans.  Humans
tend to have trouble following long lines with their eyes (I sure do).
Typographic manuals suggest to limit columns to roughly 60 characters
for exactly that reason[*].

Most doc strings are indented once (classes, functions) or twice
(methods).  72 - 8 is roughly 60.



My problem with this patch isn't actually the docstrings -- it's 
one-line comments.


If you can teach flake8 to allow this:

# Pretend this is a single-line comment that's 73 chars

but disallow this:

# Pretend this is a two-line comment that's 73 chars,
# and continues to a new line that's also pretty long,
# and maybe keeps going, too.

I will happily accept that patch. Without the ability to enforce the 
style though, I am reluctant to pretend that it's even a preference that 
we have. I think it's a waste to hunt down and re-flow single-line 
comments that just barely squeak over a limit. They look worse.


We can discuss this more when we go to propose a style guide for the 
Python folder; I think it's maybe a misprioritization of our energies in 
the present context.


(I still have the style guide on my TODO list, and even began writing a 
draft at one point, but I think we'd both like to press forward on the 
Typing bits first.)



With nesting, doc strings can become indented more.  Nesting sufficient
to squeeze the doc string width to column 72 under roughly 60 is pretty
rare.  Going beyond 72 colums to keep such doc strings readable is
exactly what PEP 8 wants you to do.

Again, I see no reason to deviate from PEP 8.


[*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style

Re: General question about parsing an rbd filename

[Connor Kuehl]

On 4/9/21 9:27 AM, Markus Armbruster wrote:

Connor Kuehl  writes:

block/rbd.c hints that:


  * Configuration values containing :, @, or = can be escaped with a
  * leading "\".


Right now, much of the parsing code will allow anyone to escape
_anything_ so long as it's preceded by '\'.

Is this the intended behavior? Or should the parser be updated to
allow escaping only certain sequences.


I can't answer this question, but perhaps I can get us a bit closer to
an answer.

The commend you quoted in part is about "rbd:" pseudo-filenames.

By "parsing code", you probably mean qemu_rbd_parse_filename().  It uses
qemu_rbd_next_tok() to split off one part after the other, stopping at a
special delimiter character, and qemu_rbd_unescape() to unescape most,
but not all parts.

Both treat '\' followed by a character other than '\0' specially.
qemu_rbd_next_tok() doesn't stop at an escaped delimiter character.
qemu_rbd_unescape() unescapes escaped characters.

I believe the comment you quoted is basically trying to say "to use a
character that would normally be a delimiter, escape it with '\'".  It
doesn't say these are the only characters you may escape.


I agree with your interpretation here.


Not unescaping some parts feels iffy to me.


I wonder if my reading of it placed too much emphasis on the 
"Configuration values" part of it, which I understand to be the optional 
key,value pairs that come after the image name.


Thank you,

Connor

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Klaus Jensen]

On Apr 10 00:30, Keith Busch wrote:

On Fri, Apr 09, 2021 at 01:55:01PM +0200, Klaus Jensen wrote:

On Apr  9 20:05, Minwoo Im wrote:
> On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > NSZE is the total size of the namespace in logical blocks. So the max
> > addressable logical block is NLB minus 1. So your starting logical
> > block is equal to NSZE it is a out of range.
> >
> > Signed-off-by: Gollu Appalanaidu 
> > ---
> >  hw/block/nvme.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 953ec64729..be9edb1158 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest 
*req)
> >  uint64_t slba = le64_to_cpu(range[i].slba);
> >  uint32_t nlb = le32_to_cpu(range[i].nlb);
> >
> > -if (nvme_check_bounds(ns, slba, nlb)) {
> > +if (nvme_check_bounds(ns, slba, nlb) || slba == 
ns->id_ns.nsze) {
>
> This patch also looks like check the boundary about slba.  Should it be
> also checked inside of nvme_check_bounds() ?

The catch here is that DSM is like the only command where the number of
logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
means that nvme_check_bounds() will always "do the right thing".

My main gripe here is that (in my mind), by definition, a "zero length
range" does not reference any LBAs at all. So how can it result in LBA Out
of Range?


So what's the problem? If the request is to discard 0 blocks starting
from the last block, then that's valid. Is this patch actually fixing
anything?



If SLBA == NSZE we are out of bounds since the last addressable block is 
NSZE-1. But, I don't consider the current behavior buggy or wrong, the 
devices correctly handles the zero length range by just not discarding 
anything anywhere.


The spec is pretty unclear on how invalid ranges in DSM are handled. My 
interpretation is that the advisory nature of DSM allows it to do best 
effort, but as Gollu is suggesting here, a device could just as well 
decide to validate the ranges and return an appropriate status code if 
it wanted to.


signature.asc
Description: PGP signature

Re: [PATCH for-6.0? 1/3] job: Add job_wait_unpaused() for block-job-complete

[John Snow]

On 4/9/21 5:57 AM, Max Reitz wrote:


Just as a PS, in a reply to one of Vladimir’s mails 
(da048f58-43a6-6811-6ad2-0d7899737...@redhat.com) I was wondering 
whether it even makes sense for mirror to do all the stuff it does in 
mirror_complete() to do it there.  Aren’t all of those things that 
should really be done in job-finalize (i.e. mirror_exit_common())?


Max


Yes, I think so -- admittedly, when I added that finalize logic, I was 
just very confused about what was safe to move where in the mirror code 
and never got my patches off the ground to do a more vigorous refactoring.


We've got, I think, three different user-initiated "This job should 
finish now" mechanisms:


- Cancelling the mirror job after it reaches READY
- Issuing "complete" to the mirror job after it reaches READY
- Issuing "finalize" to a job

Maybe these could all be integrated into a single mechanism somehow. I 
think I just lack the knowledge of the draining/threading/aio models to 
do it safely myself, and we'd need some compatibility shims for a while, 
etc.


Would have to look at this stuff again to know for certain what we'd be 
able to change compatibly.


--js

Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Philippe Mathieu-Daudé]

Hi Patrick,

On 4/9/21 6:25 PM, Patrick Venture wrote:
> The pca954x is an i2c mux, and this adds support for two variants of
> this device: the pca9546 and pca9548.
> 
> This device is very common on BMCs to route a different channel to each
> PCIe i2c bus downstream from the BMC.
> 
> Signed-off-by: Patrick Venture 
> Reviewed-by: Hao Wu 
> Reviewed-by: Havard Skinnemoen 
> ---
>  MAINTAINERS  |   6 +
>  hw/i2c/Kconfig   |   4 +
>  hw/i2c/i2c_mux_pca954x.c | 290 +++
>  hw/i2c/meson.build   |   1 +
>  hw/i2c/trace-events  |   5 +
>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
>  6 files changed, 325 insertions(+)
>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> index 09642a6dcb..8d120a25d5 100644
> --- a/hw/i2c/Kconfig
> +++ b/hw/i2c/Kconfig
> @@ -28,3 +28,7 @@ config IMX_I2C
>  config MPC_I2C
>  bool
>  select I2C
> +
> +config PCA954X
> +bool
> +select I2C

Do you have a circular dependency when also using:

   depends on I2C

?

> +static void pca954x_realize(DeviceState *dev, Error **errp)
> +{
> +Pca954xState *s = PCA954X(dev);
> +Pca954xClass *c = PCA954X_GET_CLASS(s);
> +int i;
> +
> +/* SMBus modules. Cannot fail. */
> +for (i = 0; i < c->nchans; i++) {
> +Object *obj = OBJECT(>channel[i]);
> +sysbus_realize(SYS_BUS_DEVICE(obj), _abort);

No need to cast to Object:

   sysbus_realize(SYS_BUS_DEVICE(>channel[i]), _abort);

> +}
> +}

Re: [PATCH] Document qemu-img options data_file and data_file_raw

[Connor Kuehl]

On 3/23/21 6:15 PM, John Snow wrote:

On 3/1/21 12:28 PM, Connor Kuehl wrote:

[..]
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b615aa8419..5cc585dc27 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -866,6 +866,18 @@ Supported image file formats:
  issue ``lsattr filename`` to check if the NOCOW flag is set or not
  (Capital 'C' is NOCOW flag).
+  ``data_file``
+    Pathname that refers to a file that will store all guest data. If
+    this option is used, the qcow2 file will only contain the image's
+    metadata.
+


Might recommend "filename" simply for parity with *FILENAME* argument.

(This is the first appearance of "Pathname" in this file without spaces, 
though "Path name" is indeed used several times.)



+  ``data_file_raw``
+    If this option is set to ``on``, QEMU will always keep the external
+    data file consistent as a standalone read-only raw image. The 
default

+    value is ``off``.
+
+    This option can only be enabled if ``data_file`` is set.
+


How does this interact with caching options, if it does? What happens in 
the negative case -- how does the file become inconsistent?


Hi,

First, just wanted to share the same note I'm sending to Max's review:


Hey, I just wanted to leave a note indicating that I'm not ignoring this 
review; I've incorporated it in my next version but I am waiting until after 
the freeze to send it.


Second: I've been trying to figure out if and how this affects caching. 
I do not know yet. I will keep digging. My next version of the patch 
does touch on the negative case though, describing how it becomes 
inconsistent.


Thank you!

Connor

Re: [PATCH] Document qemu-img options data_file and data_file_raw

[Connor Kuehl]

On 3/26/21 4:24 AM, Max Reitz wrote:

On 01.03.21 18:28, Connor Kuehl wrote:

[..]
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index b615aa8419..5cc585dc27 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -866,6 +866,18 @@ Supported image file formats:
  issue ``lsattr filename`` to check if the NOCOW flag is set or not
  (Capital 'C' is NOCOW flag).
+  ``data_file``
+    Pathname that refers to a file that will store all guest data. If
+    this option is used, the qcow2 file will only contain the image's
+    metadata.


I think I would like a note here about the fact that when passing this 
option to qemu-img create, the given data file will be newly created, 
i.e. if it already contains data, all that data will be lost.  And 
perhaps also note that qemu-img amend on the other hand will only change 
the reference in the qcow2 file, so the given file should already exist 
and will not be overwritten.


(“Pathname that refers to a file” sounds like the file may already exist 
before this operation, which may give people ideas.  (Not that the ideas 
were bad, it’s just that they have to take care.  Referencing qemu-img 
amend should give them a hint on how to do it right.))


Hey, I just wanted to leave a note indicating that I'm not ignoring this 
review; I've incorporated it in my next version but I am waiting until 
after the freeze to send it.


Thank you!

Connor

Re: [RFC PATCH-for-6.1 v2 4/6] target/mips/cp0_timer: Add ns_substract_to_count() helper

[Richard Henderson]

On 4/9/21 2:36 AM, Philippe Mathieu-Daudé wrote:

Factor ns_substract_to_count() out to simplify a bit.

Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/cp0_timer.c | 12 +---
  1 file changed, 9 insertions(+), 3 deletions(-)


s/substract/subtract/g
Also, one usually subtracts "from".

That said, I'm not sure I see this as clearer...


r~

Re: [RFC PATCH-for-6.1 v2 5/6] target/mips/cp0_timer: Use new clock_ns_to_ticks()

[Richard Henderson]

On 4/9/21 2:36 AM, Philippe Mathieu-Daudé wrote:

-static uint32_t ns_to_count(CPUMIPSState *env, uint64_t ns)
+static uint32_t tick_to_count(MIPSCPU *cpu, uint64_t ticks)
  {
-return ns / env->cp0_count_ns;
+return ticks / cpu->cp0_count_rate;
  }


I'm not clear on the difference between ticks and counts, and this change looks 
suspicious in terms of units.  Hasn't cp0_count_rate been used to initialize 
the clock in the first place?  And if so, why didn't clock_ns_to_ticks do the 
entire job?



r~

[PATCH v2 3/4] hw/i2c: move search to i2c_scan_bus method

[Patrick Venture]

Moves the search for matching devices on an i2c bus into a separate
method.  This allows for an object that owns an I2CBus can avoid
duplicating this method.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
---
 hw/i2c/core.c| 38 ++
 include/hw/i2c/i2c.h |  1 +
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d03b0eea5c..12981cea2d 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -77,6 +77,30 @@ int i2c_bus_busy(I2CBus *bus)
 return !QLIST_EMPTY(>current_devs);
 }
 
+bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast)
+{
+BusChild *kid;
+
+QTAILQ_FOREACH(kid, >qbus.children, sibling) {
+DeviceState *qdev = kid->child;
+I2CSlave *candidate = I2C_SLAVE(qdev);
+I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(candidate);
+
+if (sc->match_and_add(candidate, address, broadcast,
+  >current_devs)) {
+if (!broadcast) {
+return true;
+}
+}
+}
+
+/*
+ * If broadcast was true, and the list was full or empty, return true. If
+ * broadcast was false, return false.
+ */
+return broadcast;
+}
+
 /* TODO: Make this handle multiple masters.  */
 /*
  * Start or continue an i2c transaction.  When this is called for the
@@ -93,7 +117,6 @@ int i2c_bus_busy(I2CBus *bus)
  */
 int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 {
-BusChild *kid;
 I2CSlaveClass *sc;
 I2CNode *node;
 bool bus_scanned = false;
@@ -115,17 +138,8 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
recv)
  * terminating the previous transaction.
  */
 if (QLIST_EMPTY(>current_devs)) {
-QTAILQ_FOREACH(kid, >qbus.children, sibling) {
-DeviceState *qdev = kid->child;
-I2CSlave *candidate = I2C_SLAVE(qdev);
-sc = I2C_SLAVE_GET_CLASS(candidate);
-if (sc->match_and_add(candidate, address, bus->broadcast,
-  >current_devs)) {
-if (!bus->broadcast) {
-break;
-}
-}
-}
+/* Disregard whether devices were found. */
+(void)i2c_scan_bus(bus, address, bus->broadcast);
 bus_scanned = true;
 }
 
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 9b8b95ff4a..45915f3303 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -87,6 +87,7 @@ void i2c_nack(I2CBus *bus);
 int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
 int i2c_send(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
+bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast);
 
 /**
  * Create an I2C slave device on the heap.
-- 
2.31.1.295.g9ea45b61b8-goog

Re: [PATCH v2 1/4] hw/i2c: name I2CNode list in I2CBus

[Philippe Mathieu-Daudé]

On 4/9/21 6:25 PM, Patrick Venture wrote:
> To enable passing the current_devs field as a parameter, we need to use
> a named struct type.
> 
> Tested: BMC firmware with i2c devices booted to userspace.
> 
> Signed-off-by: Patrick Venture 
> Reviewed-by: Hao Wu 
> ---
>  include/hw/i2c/i2c.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [RFC PATCH v2 02/11] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[John Snow]

On 4/9/21 12:07 PM, Emanuele Giuseppe Esposito wrote:



diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c721e07d63..18d32ebe45 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -109,7 +109,7 @@ def __init__(self,
  self._binary = binary
  self._args = list(args)
-    self._wrapper = wrapper
+    self._wrapper = list(wrapper)



Unrelated change?

(I'm assuming you want to copy the user's input to explicitly avoid 
sharing state. Commit message blurb for this would be good.)


Yes, unrelated change. I do not see the benefit of copying the user 
state. I will drop it.






ACK


(Apologies for the ignorance, is this an Acked-by?)

Emanuele



Ah, yes. I just mean to say: "Minor style stuff, but looks good to me 
and I'll [likely] approve the non-RFC versions of these."


(Pending tests passing via flake8/pylint/mypy. Sorry those aren't 
formalized in the tree yet.)


--js

Re: [PULL 0/1] Linux user for 6.0 patches

[Peter Maydell]

On Fri, 9 Apr 2021 at 14:11, Laurent Vivier  wrote:
>
> The following changes since commit d0d3dd401b70168a353450e031727affee828527:
>
>   Update version for v6.0.0-rc2 release (2021-04-06 18:34:34 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/vivier/qemu.git tags/linux-user-for-6.0-pull-request
>
> for you to fetch changes up to 360f0abdc51652b06a3718ed43a8688562e69ca4:
>
>   linux-user: Use signed lengths in uaccess.c (2021-04-07 18:55:27 +0200)
>
> ----
> linux-user pull request 20210409
>
> Fix lock_user()/unlock_user()
>
> 


Applied, thanks.

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

-- PMM

[PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

[Patrick Venture]

The pca954x is an i2c mux, and this adds support for two variants of
this device: the pca9546 and pca9548.

This device is very common on BMCs to route a different channel to each
PCIe i2c bus downstream from the BMC.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
Reviewed-by: Havard Skinnemoen 
---
 MAINTAINERS  |   6 +
 hw/i2c/Kconfig   |   4 +
 hw/i2c/i2c_mux_pca954x.c | 290 +++
 hw/i2c/meson.build   |   1 +
 hw/i2c/trace-events  |   5 +
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 6 files changed, 325 insertions(+)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 58f342108e..5ea0b60b8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2039,6 +2039,12 @@ S: Maintained
 F: hw/net/tulip.c
 F: hw/net/tulip.h
 
+pca954x
+M: Patrick Venture 
+S: Maintained
+F: hw/i2c/i2c_mux_pca954x.c
+F: include/hw/i2c/i2c_mux_pca954x.h
+
 Generic Loader
 M: Alistair Francis 
 S: Maintained
diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
index 09642a6dcb..8d120a25d5 100644
--- a/hw/i2c/Kconfig
+++ b/hw/i2c/Kconfig
@@ -28,3 +28,7 @@ config IMX_I2C
 config MPC_I2C
 bool
 select I2C
+
+config PCA954X
+bool
+select I2C
diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
new file mode 100644
index 00..9aa1a8872f
--- /dev/null
+++ b/hw/i2c/i2c_mux_pca954x.c
@@ -0,0 +1,290 @@
+/*
+ * I2C multiplexer for PCA954x series of I2C multiplexer/switch chips.
+ *
+ * Copyright 2021 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * for more details.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c_mux_pca954x.h"
+#include "hw/i2c/smbus_slave.h"
+#include "hw/qdev-core.h"
+#include "hw/sysbus.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/queue.h"
+#include "qom/object.h"
+#include "trace.h"
+
+#define PCA9548_CHANNEL_COUNT 8
+#define PCA9546_CHANNEL_COUNT 4
+
+/*
+ * struct Pca954xChannel - The i2c mux device will have N of these states
+ * that own the i2c channel bus.
+ * @bus: The owned channel bus.
+ * @enabled: Is this channel active?
+ */
+typedef struct Pca954xChannel {
+SysBusDevice parent;
+
+I2CBus   *bus;
+
+bool enabled;
+} Pca954xChannel;
+
+#define TYPE_PCA954X_CHANNEL "pca954x-channel"
+#define PCA954X_CHANNEL(obj) \
+OBJECT_CHECK(Pca954xChannel, (obj), TYPE_PCA954X_CHANNEL)
+
+/*
+ * struct Pca954xState - The pca954x state object.
+ * @control: The value written to the mux control.
+ * @channel: The set of i2c channel buses that act as channels which own the
+ * i2c children.
+ */
+typedef struct Pca954xState {
+SMBusDevice parent;
+
+uint8_t control;
+
+/* The channel i2c buses. */
+Pca954xChannel channel[PCA9548_CHANNEL_COUNT];
+} Pca954xState;
+
+/*
+ * struct Pca954xClass - The pca954x class object.
+ * @nchans: The number of i2c channels this device has.
+ */
+typedef struct Pca954xClass {
+SMBusDeviceClass parent;
+
+uint8_t nchans;
+} Pca954xClass;
+
+#define TYPE_PCA954X "pca954x"
+OBJECT_DECLARE_TYPE(Pca954xState, Pca954xClass, PCA954X)
+
+/*
+ * For each channel, if it's enabled, recursively call match on those children.
+ */
+static bool pca954x_match(I2CSlave *candidate, uint8_t address,
+  bool broadcast,
+  I2CNodeList *current_devs)
+{
+Pca954xState *mux = PCA954X(candidate);
+Pca954xClass *mc = PCA954X_GET_CLASS(mux);
+int i;
+
+/* They are talking to the mux itself (or all devices enabled. */
+if ((candidate->address == address) || broadcast) {
+I2CNode *node = g_malloc(sizeof(struct I2CNode));
+node->elt = candidate;
+QLIST_INSERT_HEAD(current_devs, node, next);
+if (!broadcast) {
+return true;
+}
+}
+
+for (i = 0; i < mc->nchans; i++) {
+if (!mux->channel[i].enabled) {
+continue;
+}
+
+if (i2c_scan_bus(mux->channel[i].bus, address, broadcast)) {
+if (!broadcast) {
+return true;
+}
+}
+}
+
+/* If we arrived here we didn't find a match, return broadcast. */
+return broadcast;
+}
+
+static void pca954x_enable_channel(Pca954xState *s, uint8_t enable_mask)
+{
+Pca954xClass *mc = PCA954X_GET_CLASS(s);
+int i;
+
+/*
+ * For each channel, check if their bit is set in enable_mask 

Re: trace_FOO_tcg bit-rotted?

[Alex Bennée]

Laurent Vivier  writes:

> Le 06/04/2021 à 18:00, Alex Bennée a écrit :
>> Hi,
>> 
>> It's been awhile since I last played with this but I think we are
>> suffering from not having some test cases for tracing code
>> generation/execution in the tree. I tried adding a simple trace point to
>> see if I could track ERET calls:
>> 
>> --8<---cut here---start->8---
>> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
>> index 0b42e53500..0d643f78fe 100644
>> --- a/target/arm/translate-a64.c
>> +++ b/target/arm/translate-a64.c
>> @@ -36,6 +36,7 @@
>>  #include "exec/log.h"
>>  
>>  #include "trace-tcg.h"
>> +#include "trace.h"
>>  #include "translate-a64.h"
>>  #include "qemu/atomic128.h"
>>  
>> @@ -2302,6 +2303,9 @@ static void disas_uncond_b_reg(DisasContext *s, 
>> uint32_t insn)
>>  default:
>>  goto do_unallocated;
>>  }
>> +
>> +trace_eret_tcg(s->current_el, dst);
>> +
>>  if (tb_cflags(s->base.tb) & CF_USE_ICOUNT) {
>>  gen_io_start();
>>  }
>> diff --git a/target/arm/trace-events b/target/arm/trace-events
>> index 41c63d7570..2d4fca16a1 100644
>> --- a/target/arm/trace-events
>> +++ b/target/arm/trace-events
>> @@ -1,5 +1,10 @@
>>  # See docs/devel/tracing.txt for syntax documentation.
>>  
>> +# translate-a64.c
>> +# Mode: softmmu
>> +# Targets: TCG(aarch64-softmmu)
>> +tcg eret(int current_el, TCGv target_el) "trans_eret: from EL%d", 
>> "exec_eret: EL%d to EL%"PRId64
>
> If I read correctly, the name should be eret_tcg()
> And I'm not sure TCGv will be accepted as a parameter type, use
> uint64_t instead (and %PRIu64)

This was my confusion. I thought the trace-events file was prefixed with
tcg like guest_mem_before:

  vcpu tcg guest_mem_before(TCGv vaddr, uint16_t info) "info=%d", 
"vaddr=0x%016"PRIx64" info=%d"

and that signalled the tools to generate _trans, _exec and _tcg hooks in
the generated files. The trace code (see other patch) also has logic to
translate natural TCG types into the natives types as well signalling
which values are only visible for the _exec portion.

Maybe I'm over thinking this. Perhaps all the TCG tracing use cases are
just as easily supported with TCG plugins now and we should deprecate
this unused bit of complexity. I certainly understand the plugin
interactions better ;-)

>
> Thanks,
> Laurent


-- 
Alex Bennée

[PATCH v2 1/4] hw/i2c: name I2CNode list in I2CBus

[Patrick Venture]

To enable passing the current_devs field as a parameter, we need to use
a named struct type.

Tested: BMC firmware with i2c devices booted to userspace.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
---
 include/hw/i2c/i2c.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 277dd9f2d6..1f7c268c86 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -58,9 +58,11 @@ struct I2CNode {
 QLIST_ENTRY(I2CNode) next;
 };
 
+typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
+
 struct I2CBus {
 BusState qbus;
-QLIST_HEAD(, I2CNode) current_devs;
+I2CNodeList current_devs;
 uint8_t saved_address;
 bool broadcast;
 };
-- 
2.31.1.295.g9ea45b61b8-goog

[PULL 07/10] mirror: Move open_backing_file to exit_common

[Kevin Wolf]

From: Max Reitz 

This is a graph change and therefore should be done in job-finalize
(which is what invokes mirror_exit_common()).

Signed-off-by: Max Reitz 
Message-Id: <20210409120422.144040-2-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 22 --
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index d7e54c0ff7..f1f936bf1a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -689,6 +689,14 @@ static int mirror_exit_common(Job *job)
 ret = -EPERM;
 }
 }
+} else if (!abort && s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
+assert(!bdrv_backing_chain_next(target_bs));
+ret = bdrv_open_backing_file(bdrv_skip_filters(target_bs), NULL,
+ "backing", _err);
+if (ret < 0) {
+error_report_err(local_err);
+local_err = NULL;
+}
 }
 
 if (s->to_replace) {
@@ -1107,9 +1115,6 @@ immediate_exit:
 static void mirror_complete(Job *job, Error **errp)
 {
 MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
-BlockDriverState *target;
-
-target = blk_bs(s->target);
 
 if (!s->synced) {
 error_setg(errp, "The active block job '%s' cannot be completed",
@@ -1117,17 +1122,6 @@ static void mirror_complete(Job *job, Error **errp)
 return;
 }
 
-if (s->backing_mode == MIRROR_OPEN_BACKING_CHAIN) {
-int ret;
-
-assert(!bdrv_backing_chain_next(target));
-ret = bdrv_open_backing_file(bdrv_skip_filters(target), NULL,
- "backing", errp);
-if (ret < 0) {
-return;
-}
-}
-
 /* block all operations on to_replace bs */
 if (s->replaces) {
 AioContext *replace_aio_context;
-- 
2.30.2

[PATCH v2 2/4] hw/i2c: add match method for device search

[Patrick Venture]

At the start of an i2c transaction, the i2c bus searches its list of
children to identify which devices correspond to the address (or
broadcast).  Now the I2CSlave device has a method "match" that
encapsulates the lookup behavior. This allows the behavior to be changed
to support devices, such as i2c muxes.

Tested: A BMC firmware was booted to userspace and i2c devices were
detected.

Signed-off-by: Patrick Venture 
Reviewed-by: Hao Wu 
---
 hw/i2c/core.c| 23 +++
 include/hw/i2c/i2c.h | 11 +++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 21ec52ac5a..d03b0eea5c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -118,10 +118,9 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int 
recv)
 QTAILQ_FOREACH(kid, >qbus.children, sibling) {
 DeviceState *qdev = kid->child;
 I2CSlave *candidate = I2C_SLAVE(qdev);
-if ((candidate->address == address) || (bus->broadcast)) {
-node = g_malloc(sizeof(struct I2CNode));
-node->elt = candidate;
-QLIST_INSERT_HEAD(>current_devs, node, next);
+sc = I2C_SLAVE_GET_CLASS(candidate);
+if (sc->match_and_add(candidate, address, bus->broadcast,
+  >current_devs)) {
 if (!bus->broadcast) {
 break;
 }
@@ -290,12 +289,28 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char 
*name, uint8_t addr)
 return dev;
 }
 
+static bool i2c_slave_match(I2CSlave *candidate, uint8_t address,
+bool broadcast, I2CNodeList *current_devs)
+{
+if ((candidate->address == address) || (broadcast)) {
+I2CNode *node = g_malloc(sizeof(struct I2CNode));
+node->elt = candidate;
+QLIST_INSERT_HEAD(current_devs, node, next);
+return true;
+}
+
+/* Not found and not broadcast. */
+return false;
+}
+
 static void i2c_slave_class_init(ObjectClass *klass, void *data)
 {
 DeviceClass *k = DEVICE_CLASS(klass);
+I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
 set_bit(DEVICE_CATEGORY_MISC, k->categories);
 k->bus_type = TYPE_I2C_BUS;
 device_class_set_props(k, i2c_props);
+sc->match_and_add = i2c_slave_match;
 }
 
 static const TypeInfo i2c_slave_type_info = {
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 1f7c268c86..9b8b95ff4a 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -16,6 +16,7 @@ enum i2c_event {
 I2C_NACK /* Masker NACKed a receive byte.  */
 };
 
+typedef struct I2CNodeList I2CNodeList;
 
 #define TYPE_I2C_SLAVE "i2c-slave"
 OBJECT_DECLARE_TYPE(I2CSlave, I2CSlaveClass,
@@ -39,6 +40,16 @@ struct I2CSlaveClass {
  * return code is not used and should be zero.
  */
 int (*event)(I2CSlave *s, enum i2c_event event);
+
+/*
+ * Check if this device matches the address provided.  Returns bool of
+ * true if it matches (or broadcast), and updates the device list, false
+ * otherwise.
+ *
+ * If broadcast is true, match should add the device and return true.
+ */
+bool (*match_and_add)(I2CSlave *candidate, uint8_t address, bool broadcast,
+  I2CNodeList *current_devs);
 };
 
 struct I2CSlave {
-- 
2.31.1.295.g9ea45b61b8-goog

[PATCH v2 0/4] hw/i2c: Adds pca954x i2c mux switch device

[Patrick Venture]

The i2c mux device pca954x implements two devices:
 - the pca9546 and pca9548.

v2:
 - the core i2c bus now calls a match method on each i2c child, which
 by default will only check for a match against itself.
 - the pca954x device overrides the i2c device match method to search
 the children for each of its buses that are active.
 - the pca954x device now owns an i2c bus for each channel, allowing
 the normal device model to attach devices to the channels.

Patrick Venture (4):
  hw/i2c: name I2CNode list in I2CBus
  hw/i2c: add match method for device search
  hw/i2c: move search to i2c_scan_bus method
  hw/i2c: add pca954x i2c-mux switch

 MAINTAINERS  |   6 +
 hw/i2c/Kconfig   |   4 +
 hw/i2c/core.c|  55 --
 hw/i2c/i2c_mux_pca954x.c | 290 +++
 hw/i2c/meson.build   |   1 +
 hw/i2c/trace-events  |   5 +
 include/hw/i2c/i2c.h |  16 +-
 include/hw/i2c/i2c_mux_pca954x.h |  19 ++
 8 files changed, 382 insertions(+), 14 deletions(-)
 create mode 100644 hw/i2c/i2c_mux_pca954x.c
 create mode 100644 include/hw/i2c/i2c_mux_pca954x.h

-- 
2.31.1.295.g9ea45b61b8-goog

[PULL 06/10] hw/block/fdc: Fix 'fallback' property on sysbus floppy disk controllers

[Kevin Wolf]

From: Philippe Mathieu-Daudé 

Setting the 'fallback' property corrupts the QOM instance state
(FDCtrlSysBus) because it accesses an incorrect offset (it uses
the offset of the FDCtrlISABus state).

Cc: qemu-sta...@nongnu.org
Fixes: a73275dd6fc ("fdc: Add fallback option")
Signed-off-by: Philippe Mathieu-Daudé 
Message-Id: <20210407133742.1680424-1-f4...@amsat.org>
Reviewed-by: Markus Armbruster 
Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 82afda7f3a..a825c2acba 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2893,7 +2893,7 @@ static Property sysbus_fdc_properties[] = {
 DEFINE_PROP_SIGNED("fdtypeB", FDCtrlSysBus, state.qdev_for_drives[1].type,
 FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
 FloppyDriveType),
-DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
+DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
 FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
 FloppyDriveType),
 DEFINE_PROP_END_OF_LIST(),
@@ -2918,7 +2918,7 @@ static Property sun4m_fdc_properties[] = {
 DEFINE_PROP_SIGNED("fdtype", FDCtrlSysBus, state.qdev_for_drives[0].type,
 FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
 FloppyDriveType),
-DEFINE_PROP_SIGNED("fallback", FDCtrlISABus, state.fallback,
+DEFINE_PROP_SIGNED("fallback", FDCtrlSysBus, state.fallback,
 FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
 FloppyDriveType),
 DEFINE_PROP_END_OF_LIST(),
-- 
2.30.2

[PULL 08/10] mirror: Do not enter a paused job on completion

[Kevin Wolf]

From: Max Reitz 

Currently, it is impossible to complete jobs on standby (i.e. paused
ready jobs), but actually the only thing in mirror_complete() that does
not work quite well with a paused job is the job_enter() at the end.

If we make it conditional, this function works just fine even if the
mirror job is paused.

So technically this is a no-op, but obviously the intention is to accept
block-job-complete even for jobs on standby, which we need this patch
for first.

Signed-off-by: Max Reitz 
Message-Id: <20210409120422.144040-3-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 block/mirror.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index f1f936bf1a..5a71bd8bbc 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1148,7 +1148,11 @@ static void mirror_complete(Job *job, Error **errp)
 }
 
 s->should_complete = true;
-job_enter(job);
+
+/* If the job is paused, it will be re-entered when it is resumed */
+if (!job->paused) {
+job_enter(job);
+}
 }
 
 static void coroutine_fn mirror_pause(Job *job)
-- 
2.30.2

[PULL 02/10] block/rbd: fix memory leak in qemu_rbd_co_create_opts()

[Kevin Wolf]

From: Stefano Garzarella 

When we allocate 'q_namespace', we forgot to set 'has_q_namespace'
to true. This can cause several issues, including a memory leak,
since qapi_free_BlockdevCreateOptions() does not deallocate that
memory, as reported by valgrind:

  13 bytes in 1 blocks are definitely lost in loss record 7 of 96
 at 0x4839809: malloc (vg_replace_malloc.c:307)
 by 0x48CEBB8: g_malloc (in /usr/lib64/libglib-2.0.so.0.6600.8)
 by 0x48E3FE3: g_strdup (in /usr/lib64/libglib-2.0.so.0.6600.8)
 by 0x180010: qemu_rbd_co_create_opts (rbd.c:446)
 by 0x1AE72C: bdrv_create_co_entry (block.c:492)
 by 0x241902: coroutine_trampoline (coroutine-ucontext.c:173)
 by 0x57530AF: ??? (in /usr/lib64/libc-2.32.so)
 by 0x1FFEFFFA6F: ???

Fix setting 'has_q_namespace' to true when we allocate 'q_namespace'.

Fixes: 19ae9ae014 ("block/rbd: Add support for ceph namespaces")
Signed-off-by: Stefano Garzarella 
Message-Id: <20210329150129.121182-3-sgarz...@redhat.com>
Reviewed-by: Markus Armbruster 
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block/rbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/rbd.c b/block/rbd.c
index 24cefcd0dc..f098a89c7b 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -444,6 +444,7 @@ static int coroutine_fn qemu_rbd_co_create_opts(BlockDriver 
*drv,
 loc->user= g_strdup(qdict_get_try_str(options, "user"));
 loc->has_user= !!loc->user;
 loc->q_namespace = g_strdup(qdict_get_try_str(options, "namespace"));
+loc->has_q_namespace = !!loc->q_namespace;
 loc->image   = g_strdup(qdict_get_try_str(options, "image"));
 keypairs = qdict_get_try_str(options, "=keyvalue-pairs");
 
-- 
2.30.2

[PULL 05/10] iotests: Test mirror-top filter permissions

[Kevin Wolf]

From: Max Reitz 

Add a test accompanying commit 53431b9086b2832ca1aeff0c55e186e9ed79bd11
("block/mirror: Fix mirror_top's permissions").

Signed-off-by: Max Reitz 
Message-Id: <20210331122815.51491-1-mre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/tests/mirror-top-perms | 121 ++
 tests/qemu-iotests/tests/mirror-top-perms.out |   5 +
 2 files changed, 126 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/mirror-top-perms
 create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out

diff --git a/tests/qemu-iotests/tests/mirror-top-perms 
b/tests/qemu-iotests/tests/mirror-top-perms
new file mode 100755
index 00..451a0666f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -0,0 +1,121 @@
+#!/usr/bin/env python3
+# group: rw
+#
+# Test permissions taken by the mirror-top filter
+#
+# Copyright (C) 2021 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import iotests
+from iotests import qemu_img
+
+# Import qemu after iotests.py has amended sys.path
+# pylint: disable=wrong-import-order
+import qemu
+
+
+image_size = 1 * 1024 * 1024
+source = os.path.join(iotests.test_dir, 'source.img')
+
+
+class TestMirrorTopPerms(iotests.QMPTestCase):
+def setUp(self):
+assert qemu_img('create', '-f', iotests.imgfmt, source,
+str(image_size)) == 0
+self.vm = iotests.VM()
+self.vm.add_drive(source)
+self.vm.add_blockdev(f'null-co,node-name=null,size={image_size}')
+self.vm.launch()
+
+# Will be created by the test function itself
+self.vm_b = None
+
+def tearDown(self):
+try:
+self.vm.shutdown()
+except qemu.machine.AbnormalShutdown:
+pass
+
+if self.vm_b is not None:
+self.vm_b.shutdown()
+
+os.remove(source)
+
+def test_cancel(self):
+"""
+Before commit 53431b9086b28, mirror-top used to not take any
+permissions but WRITE and share all permissions.  Because it
+is inserted between the source's original parents and the
+source, there generally was no parent that would have taken or
+unshared any permissions on the source, which means that an
+external process could access the image unhindered by locks.
+(Unless there was a parent above the protocol node that would
+take its own locks, e.g. a format driver.)
+This is bad enough, but if the mirror job is then cancelled,
+the mirroring VM tries to take back the image, restores the
+original permissions taken and unshared, and assumes this must
+just work.  But it will not, and so the VM aborts.
+
+Commit 53431b9086b28 made mirror keep the original permissions
+and so no other process can "steal" the image.
+
+(Note that you cannot really do the same with the target image
+and then completing the job, because the mirror job always
+took/unshared the correct permissions on the target.  For
+example, it does not share READ_CONSISTENT, which makes it
+difficult to let some other qemu process open the image.)
+"""
+
+result = self.vm.qmp('blockdev-mirror',
+ job_id='mirror',
+ device='drive0',
+ target='null',
+ sync='full')
+self.assert_qmp(result, 'return', {})
+
+self.vm.event_wait('BLOCK_JOB_READY')
+
+# We want this to fail because the image cannot be locked.
+# If it does not fail, continue still and see what happens.
+self.vm_b = iotests.VM(path_suffix='b')
+# Must use -blockdev -device so we can use share-rw.
+# (And we need share-rw=on because mirror-top was always
+# forced to take the WRITE permission so it can write to the
+# source image.)
+self.vm_b.add_blockdev(f'file,node-name=drive0,filename={source}')
+self.vm_b.add_device('virtio-blk,drive=drive0,share-rw=on')
+try:
+self.vm_b.launch()
+print('ERROR: VM B launched successfully, this should not have '
+  'happened')
+except qemu.qmp.QMPConnectError:
+assert 

[PULL 10/10] test-blockjob: Test job_wait_unpaused()

[Kevin Wolf]

From: Max Reitz 

Create a job that remains on STANDBY after a drained section, and see
that invoking job_wait_unpaused() will get it unstuck.

Signed-off-by: Max Reitz 
Message-Id: <20210409120422.144040-5-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 tests/unit/test-blockjob.c | 121 +
 1 file changed, 121 insertions(+)

diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
index 7519847912..dcacfa6c7c 100644
--- a/tests/unit/test-blockjob.c
+++ b/tests/unit/test-blockjob.c
@@ -16,6 +16,7 @@
 #include "block/blockjob_int.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qmp/qdict.h"
+#include "iothread.h"
 
 static const BlockJobDriver test_block_job_driver = {
 .job_driver = {
@@ -375,6 +376,125 @@ static void test_cancel_concluded(void)
 cancel_common(s);
 }
 
+/* (See test_yielding_driver for the job description) */
+typedef struct YieldingJob {
+BlockJob common;
+bool should_complete;
+} YieldingJob;
+
+static void yielding_job_complete(Job *job, Error **errp)
+{
+YieldingJob *s = container_of(job, YieldingJob, common.job);
+s->should_complete = true;
+job_enter(job);
+}
+
+static int coroutine_fn yielding_job_run(Job *job, Error **errp)
+{
+YieldingJob *s = container_of(job, YieldingJob, common.job);
+
+job_transition_to_ready(job);
+
+while (!s->should_complete) {
+job_yield(job);
+}
+
+return 0;
+}
+
+/*
+ * This job transitions immediately to the READY state, and then
+ * yields until it is to complete.
+ */
+static const BlockJobDriver test_yielding_driver = {
+.job_driver = {
+.instance_size  = sizeof(YieldingJob),
+.free   = block_job_free,
+.user_resume= block_job_user_resume,
+.run= yielding_job_run,
+.complete   = yielding_job_complete,
+},
+};
+
+/*
+ * Test that job_complete() works even on jobs that are in a paused
+ * state (i.e., STANDBY).
+ *
+ * To do this, run YieldingJob in an IO thread, get it into the READY
+ * state, then have a drained section.  Before ending the section,
+ * acquire the context so the job will not be entered and will thus
+ * remain on STANDBY.
+ *
+ * job_complete() should still work without error.
+ *
+ * Note that on the QMP interface, it is impossible to lock an IO
+ * thread before a drained section ends.  In practice, the
+ * bdrv_drain_all_end() and the aio_context_acquire() will be
+ * reversed.  However, that makes for worse reproducibility here:
+ * Sometimes, the job would no longer be in STANDBY then but already
+ * be started.  We cannot prevent that, because the IO thread runs
+ * concurrently.  We can only prevent it by taking the lock before
+ * ending the drained section, so we do that.
+ *
+ * (You can reverse the order of operations and most of the time the
+ * test will pass, but sometimes the assert(status == STANDBY) will
+ * fail.)
+ */
+static void test_complete_in_standby(void)
+{
+BlockBackend *blk;
+IOThread *iothread;
+AioContext *ctx;
+Job *job;
+BlockJob *bjob;
+
+/* Create a test drive, move it to an IO thread */
+blk = create_blk(NULL);
+iothread = iothread_new();
+
+ctx = iothread_get_aio_context(iothread);
+blk_set_aio_context(blk, ctx, _abort);
+
+/* Create our test job */
+bjob = mk_job(blk, "job", _yielding_driver, true,
+  JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
+job = >job;
+assert(job->status == JOB_STATUS_CREATED);
+
+/* Wait for the job to become READY */
+job_start(job);
+aio_context_acquire(ctx);
+AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY);
+aio_context_release(ctx);
+
+/* Begin the drained section, pausing the job */
+bdrv_drain_all_begin();
+assert(job->status == JOB_STATUS_STANDBY);
+/* Lock the IO thread to prevent the job from being run */
+aio_context_acquire(ctx);
+/* This will schedule the job to resume it */
+bdrv_drain_all_end();
+
+/* But the job cannot run, so it will remain on standby */
+assert(job->status == JOB_STATUS_STANDBY);
+
+/* Even though the job is on standby, this should work */
+job_complete(job, _abort);
+
+/* The test is done now, clean up. */
+job_finish_sync(job, NULL, _abort);
+assert(job->status == JOB_STATUS_PENDING);
+
+job_finalize(job, _abort);
+assert(job->status == JOB_STATUS_CONCLUDED);
+
+job_dismiss(, _abort);
+
+destroy_blk(blk);
+aio_context_release(ctx);
+iothread_join(iothread);
+}
+
 int main(int argc, char **argv)
 {
 qemu_init_main_loop(_abort);
@@ -389,5 +509,6 @@ int main(int argc, char **argv)
 g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
 g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
 g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
+g_test_add_func("/blockjob/complete_in_standby", test_complete_in_standby);
 

Re: [PATCH 0/4] job: Allow complete for jobs on standby

[Kevin Wolf]

Am 09.04.2021 um 14:04 hat Max Reitz geschrieben:
> Hi,
> 
> We sometimes have a problem with jobs remaining on STANDBY after a drain
> (for a short duration), so if the user immediately issues a
> block-job-complete, that will fail.
> 
> (See also
> https://lists.nongnu.org/archive/html/qemu-block/2021-04/msg00215.html,
> which this series is an alternative for.)
> 
> Looking at the only implementation of .complete(), which is
> mirror_complete(), it looks like there is basically nothing that would
> prevent it from being run while mirror is paused.  Really only the
> job_enter() at the end, which we should not and need not do when the job
> is paused.
> 
> So make that conditional (patch 2), clean up the function on the way
> (patch 1, which moves one of its blocks to mirror_exit_common()), and
> then we can allow job_complete() on jobs that are on standby (patch 3).
> 
> Patch 4 is basically the same test as in
> https://lists.nongnu.org/archive/html/qemu-block/2021-04/msg00214.html,
> except some comments are different and, well, job_complete() just works
> on STANDBY jobs.
> 
> Patch 5 is an iotest that may or may not show the problem for you.  I’ve
> tuned the numbers so that on my machine, it fails about 50/50 without
> this series (i.e., the job is still on STANDBY and job_complete()
> refuses to do anything).
> 
> I’m not sure we want that iotest, because it does quite a bit of I/O and
> it’s unreliable, and I don’t think there’s anything I can do to make it
> reliable.

Thanks, applied patches 1-4 to the block branch.

Kevin

Re: [RFC PATCH-for-6.1 v2 3/6] target/mips/cp0_timer: Add ns_to_count() helper

[Richard Henderson]

On 4/9/21 2:36 AM, Philippe Mathieu-Daudé wrote:

Factor ns_to_count() out to simplify a bit.

Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/cp0_timer.c | 18 ++
  1 file changed, 10 insertions(+), 8 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PULL 04/10] iotests: add test for removing persistent bitmap from backing file

[Kevin Wolf]

From: Vladimir Sementsov-Ogievskiy 

Just demonstrate one of x-blockdev-reopen usecases. We can't simply
remove persistent bitmap from RO node (for example from backing file),
as we need to remove it from the image too. So, we should reopen the
node first.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20210401161522.8001-1-vsement...@virtuozzo.com>
Signed-off-by: Kevin Wolf 
---
 .../tests/remove-bitmap-from-backing  | 69 +++
 .../tests/remove-bitmap-from-backing.out  |  6 ++
 2 files changed, 75 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/remove-bitmap-from-backing
 create mode 100644 tests/qemu-iotests/tests/remove-bitmap-from-backing.out

diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing 
b/tests/qemu-iotests/tests/remove-bitmap-from-backing
new file mode 100755
index 00..0ea4c36507
--- /dev/null
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing
@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+#
+# Test removing persistent bitmap from backing
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import log, qemu_img_create, qemu_img, qemu_img_pipe
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+top, base = iotests.file_path('top', 'base')
+size = '1M'
+
+assert qemu_img_create('-f', iotests.imgfmt, base, size) == 0
+assert qemu_img_create('-f', iotests.imgfmt, '-b', base,
+   '-F', iotests.imgfmt, top, size) == 0
+
+assert qemu_img('bitmap', '--add', base, 'bitmap0') == 0
+# Just assert that our method of checking bitmaps in the image works.
+assert 'bitmaps' in qemu_img_pipe('info', base)
+
+vm = iotests.VM().add_drive(top, 'backing.node-name=base')
+vm.launch()
+
+log('Trying to remove persistent bitmap from r-o base node, should fail:')
+vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
+
+new_base_opts = {
+'node-name': 'base',
+'driver': 'qcow2',
+'file': {
+'driver': 'file',
+'filename':  base
+},
+'read-only': False
+}
+
+# Don't want to bother with filtering qmp_log for reopen command
+result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+if result != {'return': {}}:
+log('Failed to reopen: ' + str(result))
+
+log('Remove persistent bitmap from base node reopened to RW:')
+vm.qmp_log('block-dirty-bitmap-remove', node='base', name='bitmap0')
+
+new_base_opts['read-only'] = True
+result = vm.qmp('x-blockdev-reopen', **new_base_opts)
+if result != {'return': {}}:
+log('Failed to reopen: ' + str(result))
+
+vm.shutdown()
+
+if 'bitmaps' in qemu_img_pipe('info', base):
+log('ERROR: Bitmap is still in the base image')
diff --git a/tests/qemu-iotests/tests/remove-bitmap-from-backing.out 
b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
new file mode 100644
index 00..c28af82c75
--- /dev/null
+++ b/tests/qemu-iotests/tests/remove-bitmap-from-backing.out
@@ -0,0 +1,6 @@
+Trying to remove persistent bitmap from r-o base node, should fail:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", 
"node": "base"}}
+{"error": {"class": "GenericError", "desc": "Bitmap 'bitmap0' is readonly and 
cannot be modified"}}
+Remove persistent bitmap from base node reopened to RW:
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmap0", 
"node": "base"}}
+{"return": {}}
-- 
2.30.2

Re: [RFC PATCH-for-6.1 v2 2/6] target/mips/cpu: Update CP0 clock when CPU clock is propagated

[Richard Henderson]

On 4/9/21 2:36 AM, Philippe Mathieu-Daudé wrote:

Setting the CP0 clock simply on CPU init is incorrect. First
because the clock can not be yet propagated. Second because
we aimed to support dynamic frequencies in commit a0713e85bfa,
so the CPU frequency can be changed*after*  init time.

The correct way is to register a ClockCallback, which will
update the CP0 period when the clock changes.

Fixes: a0713e85bfa ("target/mips/cpu: Allow the CPU to use dynamic frequencies")
Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/cpu.c | 11 +--
  1 file changed, 9 insertions(+), 2 deletions(-)


Reviewed-by: Richard Henderson 

r~

[PULL 09/10] job: Allow complete for jobs on standby

[Kevin Wolf]

From: Max Reitz 

The only job that implements .complete is the mirror job, and it can
handle completion requests just fine while the job is paused.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1945635
Signed-off-by: Max Reitz 
Message-Id: <20210409120422.144040-4-mre...@redhat.com>
Signed-off-by: Kevin Wolf 
---
 job.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/job.c b/job.c
index 289edee143..4aff13d95a 100644
--- a/job.c
+++ b/job.c
@@ -56,7 +56,7 @@ bool JobVerbTable[JOB_VERB__MAX][JOB_STATUS__MAX] = {
 [JOB_VERB_PAUSE]= {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
 [JOB_VERB_RESUME]   = {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
 [JOB_VERB_SET_SPEED]= {0, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0},
-[JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0},
+[JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0},
 [JOB_VERB_FINALIZE] = {0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0},
 [JOB_VERB_DISMISS]  = {0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0},
 };
@@ -991,7 +991,7 @@ void job_complete(Job *job, Error **errp)
 if (job_apply_verb(job, JOB_VERB_COMPLETE, errp)) {
 return;
 }
-if (job->pause_count || job_is_cancelled(job) || !job->driver->complete) {
+if (job_is_cancelled(job) || !job->driver->complete) {
 error_setg(errp, "The active block job '%s' cannot be completed",
job->id);
 return;
-- 
2.30.2

[PULL 03/10] iotests/qsd-jobs: Filter events in the first test

[Kevin Wolf]

From: Max Reitz 

The job may or may not be ready before the 'quit' is issued.  Whether it
is is irrelevant; for the purpose of the test, it only needs to still be
there.  Filter the job status change and READY events from the output so
it becomes reliable.

Reported-by: Peter Maydell 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
Message-Id: <20210401132839.139939-1-mre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/tests/qsd-jobs |  5 -
 tests/qemu-iotests/tests/qsd-jobs.out | 10 --
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/tests/qsd-jobs 
b/tests/qemu-iotests/tests/qsd-jobs
index 972b6b3898..510bf0a9dc 100755
--- a/tests/qemu-iotests/tests/qsd-jobs
+++ b/tests/qemu-iotests/tests/qsd-jobs
@@ -52,9 +52,12 @@ echo "=== Job still present at shutdown ==="
 echo
 
 # Just make sure that this doesn't crash
+# (Filter job status and READY events, because their order may differ
+# between runs, particularly around when 'quit' is issued)
 $QSD --chardev stdio,id=stdio --monitor chardev=stdio \
 --blockdev node-name=file0,driver=file,filename="$TEST_IMG" \
---blockdev node-name=fmt0,driver=qcow2,file=file0 <

Re: [RFC PATCH-for-6.1 v2 1/6] target/mips/cpu: Use clock_has_source() instead of clock_get()

[Richard Henderson]

On 4/9/21 2:36 AM, Philippe Mathieu-Daudé wrote:

clock_get() returns the clock period. With an unconnected clock
we get 0. This is not what we want here. Use the API properly
by using the clock_has_source() function instead.

Fixes: a0713e85bfa ("target/mips/cpu: Allow the CPU to use dynamic frequencies")
Signed-off-by: Philippe Mathieu-Daudé
---
  target/mips/cpu.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Richard Henderson 

r~

[PULL 01/10] block/rbd: fix memory leak in qemu_rbd_connect()

[Kevin Wolf]

From: Stefano Garzarella 

In qemu_rbd_connect(), 'mon_host' is allocated by qemu_rbd_mon_host()
using g_strjoinv(), but it's only freed in the error path, leaking
memory in the success path as reported by valgrind:

  80 bytes in 4 blocks are definitely lost in loss record 5,028 of 6,516
 at 0x4839809: malloc (vg_replace_malloc.c:307)
 by 0x5315BB8: g_malloc (in /usr/lib64/libglib-2.0.so.0.6600.8)
 by 0x532B6FF: g_strjoinv (in /usr/lib64/libglib-2.0.so.0.6600.8)
 by 0x87D07E: qemu_rbd_mon_host (rbd.c:538)
 by 0x87D07E: qemu_rbd_connect (rbd.c:562)
 by 0x87E1CE: qemu_rbd_open (rbd.c:740)
 by 0x840EB1: bdrv_open_driver (block.c:1528)
 by 0x8453A9: bdrv_open_common (block.c:1802)
 by 0x8453A9: bdrv_open_inherit (block.c:3444)
 by 0x8464C2: bdrv_open (block.c:3537)
 by 0x8108CD: qmp_blockdev_add (blockdev.c:3569)
 by 0x8EA61B: qmp_marshal_blockdev_add (qapi-commands-block-core.c:1086)
 by 0x90B528: do_qmp_dispatch_bh (qmp-dispatch.c:131)
 by 0x907EA4: aio_bh_poll (async.c:164)

Fix freeing 'mon_host' also when qemu_rbd_connect() ends correctly.

Fixes: 0a55679b4a5061f4d74bdb1a0e81611ba3390b00
Signed-off-by: Stefano Garzarella 
Message-Id: <20210329150129.121182-2-sgarz...@redhat.com>
Reviewed-by: Markus Armbruster 
Reviewed-by: Max Reitz 
Signed-off-by: Kevin Wolf 
---
 block/rbd.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 9071a00e3f..24cefcd0dc 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -563,13 +563,13 @@ static int qemu_rbd_connect(rados_t *cluster, 
rados_ioctx_t *io_ctx,
 if (local_err) {
 error_propagate(errp, local_err);
 r = -EINVAL;
-goto failed_opts;
+goto out;
 }
 
 r = rados_create(cluster, opts->user);
 if (r < 0) {
 error_setg_errno(errp, -r, "error initializing");
-goto failed_opts;
+goto out;
 }
 
 /* try default location when conf=NULL, but ignore failure */
@@ -626,11 +626,12 @@ static int qemu_rbd_connect(rados_t *cluster, 
rados_ioctx_t *io_ctx,
  */
 rados_ioctx_set_namespace(*io_ctx, opts->q_namespace);
 
-return 0;
+r = 0;
+goto out;
 
 failed_shutdown:
 rados_shutdown(*cluster);
-failed_opts:
+out:
 g_free(mon_host);
 return r;
 }
-- 
2.30.2

[PULL 00/10] Block layer fixes for 6.0-rc3

[Kevin Wolf]

The following changes since commit ce69aa92d71e13db9c3702a8e8305e8d2463aeb8:

  Merge remote-tracking branch 'remotes/jasowang/tags/net-pull-request' into 
staging (2021-04-08 16:45:31 +0100)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to c2c731a4d35062295cd3260e66b3754588a2fad4:

  test-blockjob: Test job_wait_unpaused() (2021-04-09 18:00:29 +0200)


Block layer fixes

- mirror: Fix job-complete race condition causing unexpected errors
- fdc: Fix 'fallback' property on sysbus floppy disk controllers
- rbd: Fix memory leaks
- iotest improvements


Max Reitz (6):
  iotests/qsd-jobs: Filter events in the first test
  iotests: Test mirror-top filter permissions
  mirror: Move open_backing_file to exit_common
  mirror: Do not enter a paused job on completion
  job: Allow complete for jobs on standby
  test-blockjob: Test job_wait_unpaused()

Philippe Mathieu-Daudé (1):
  hw/block/fdc: Fix 'fallback' property on sysbus floppy disk controllers

Stefano Garzarella (2):
  block/rbd: fix memory leak in qemu_rbd_connect()
  block/rbd: fix memory leak in qemu_rbd_co_create_opts()

Vladimir Sementsov-Ogievskiy (1):
  iotests: add test for removing persistent bitmap from backing file

 block/mirror.c |  28 +++--
 block/rbd.c|  10 +-
 hw/block/fdc.c |   4 +-
 job.c  |   4 +-
 tests/unit/test-blockjob.c | 121 +
 tests/qemu-iotests/tests/mirror-top-perms  | 121 +
 tests/qemu-iotests/tests/mirror-top-perms.out  |   5 +
 tests/qemu-iotests/tests/qsd-jobs  |   5 +-
 tests/qemu-iotests/tests/qsd-jobs.out  |  10 --
 .../qemu-iotests/tests/remove-bitmap-from-backing  |  69 
 .../tests/remove-bitmap-from-backing.out   |   6 +
 11 files changed, 349 insertions(+), 34 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/mirror-top-perms
 create mode 100644 tests/qemu-iotests/tests/mirror-top-perms.out
 create mode 100755 tests/qemu-iotests/tests/remove-bitmap-from-backing
 create mode 100644 tests/qemu-iotests/tests/remove-bitmap-from-backing.out

Re: [RFC PATCH v2 02/11] python: qemu: pass the wrapper field from QEMUQtestmachine to QEMUMachine

[Emanuele Giuseppe Esposito]

diff --git a/python/qemu/machine.py b/python/qemu/machine.py
index c721e07d63..18d32ebe45 100644
--- a/python/qemu/machine.py
+++ b/python/qemu/machine.py
@@ -109,7 +109,7 @@ def __init__(self,
  self._binary = binary
  self._args = list(args)
-    self._wrapper = wrapper
+    self._wrapper = list(wrapper)



Unrelated change?

(I'm assuming you want to copy the user's input to explicitly avoid 
sharing state. Commit message blurb for this would be good.)


Yes, unrelated change. I do not see the benefit of copying the user 
state. I will drop it.






ACK


(Apologies for the ignorance, is this an Acked-by?)

Emanuele

Re: [RFC PATCH v2 04/11] qemu-iotests: delay QMP socket timers

[Emanuele Giuseppe Esposito]

On 08/04/2021 21:03, Paolo Bonzini wrote:



Il gio 8 apr 2021, 18:06 Emanuele Giuseppe Esposito > ha scritto:




On 08/04/2021 17:40, Paolo Bonzini wrote:
 > On 07/04/21 15:50, Emanuele Giuseppe Esposito wrote:
 >>   def get_qmp_events_filtered(self, wait=60.0):
 >>   result = []
 >> -    for ev in self.get_qmp_events(wait=wait):
 >> +    qmp_wait = wait
 >> +    if qemu_gdb:
 >> +    qmp_wait = 0.0
 >> +    for ev in self.get_qmp_events(wait=qmp_wait):
 >>   result.append(filter_qmp_event(ev))
 >>   return result
 >
 > Should this be handled in get_qmp_events instead, since you're
basically
 > changing all the callers?

get_qmp_events is in python/machine.py, which as I understand might be
used also by some other scripts, so I want to keep the changes there to
the minimum. Also, machine.py has no access to qemu_gdb or
qemu_valgrind, so passing a boolean or something to delay the timer
would still require to add a similar check in all sections.

Or do you have a cleaner way to do this?


Maybe a subclass IotestsMachine?



I actually figured that I could override get_qmp_events and put the 
check there. Something like (simplified):


class VM(qtest.QEMUQtestMachine):

...

def get_qmp_events(self, wait)
if qemu_gdb or qemu_valgrind:
wait = 0.0
return super().get_qmp_events(wait)

Emanuele

[PATCH 1/2] qdev: Separate implementations of qdev_get_machine() for user and system

[Greg Kurz]

Despite its simple name and common usage of "getting a pointer to
the machine" in system-mode emulation, qdev_get_machine() has some
subtilities.

First, it can be called when running user-mode emulation : this is
because user-mode partly relies on qdev to instantiate its CPU
model.

Second, but not least, it has a side-effect : if it cannot find an
object at "/machine" in the QOM tree, it creates a dummy "container"
object and put it there. A simple check on the type returned by
qdev_get_machine() allows user-mode to run the common qdev code,
skipping the parts that only make sense for system-mode.

This side-effect turns out to complicate the use of qdev_get_machine()
for the system-mode case though. Most notably, qdev_get_machine() must
not be called before the machine object is added to the QOM tree by
qemu_create_machine(), otherwise the existing dummy "container" object
would cause qemu_create_machine() to fail with something like :

Unexpected error in object_property_try_add() at ../../qom/object.c:1223:
qemu-system-ppc64: attempt to add duplicate property 'machine' to
 object (type 'container')
Aborted (core dumped)

This situation doesn't exist in the current code base, mostly because
of preventive fixing of some "latent bugs" in QEMU 4.0 (see 1a3ec8c1564
and e2fb3fbbf9c for details).

A new kind of breakage was spotted very recently though :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
/home/thuth/devel/qemu/include/hw/boards.h:24:
 MACHINE: Object 0x5635bd53af10 is not an instance of type machine
Aborted (core dumped)

This comes from the change 3df261b6676b in QEMU 5.0. It unwillingly
added a new condition for qdev_get_machine() to be called too early,
breaking MACHINE(qdev_get_machine()) in generic cpu-core code this
time.

In order to avoid further subtle breakages like this, change the
implentation of qdev_get_machine() to:
- keep the existing behaviour of creating the dummy "container"
  object for the user-mode case only ;
- abort() if the machine doesn't exist yet in the QOM tree for
  the system-mode case. This gives a precise hint to developpers
  that calling qdev_get_machine() too early is a programming bug.

This is achieved with a new do_qdev_get_machine() function called
from qdev_get_machine(), with different implementations for system
and user mode.

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
 qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

Reported-by: Thomas Huth 
Signed-off-by: Greg Kurz 
---
 hw/core/machine.c| 14 ++
 hw/core/qdev.c   |  2 +-
 include/hw/qdev-core.h   |  1 +
 stubs/meson.build|  1 +
 stubs/qdev-get-machine.c | 11 +++
 5 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 stubs/qdev-get-machine.c

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 40def78183a7..fecca4023105 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1293,6 +1293,20 @@ void qdev_machine_creation_done(void)
 register_global_state();
 }
 
+Object *do_qdev_get_machine(void)
+{
+Object *machine;
+
+machine = object_resolve_path_component(object_get_root(), "machine");
+/*
+ * qdev_get_machine() shouldn't be called before qemu_create_machine()
+ * has created the "/machine" path.
+ */
+assert(machine != NULL);
+
+return machine;
+}
+
 static const TypeInfo machine_info = {
 .name = TYPE_MACHINE,
 .parent = TYPE_OBJECT,
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cefc5eaa0a92..1122721b2bf0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -1131,7 +1131,7 @@ Object *qdev_get_machine(void)
 static Object *dev;
 
 if (dev == NULL) {
-dev = container_get(object_get_root(), "/machine");
+dev = do_qdev_get_machine();
 }
 
 return dev;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bafc311bfa1b..90e295e0bc1a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -782,6 +782,7 @@ const char *qdev_fw_name(DeviceState *dev);
 
 void qdev_assert_realized_properly(void);
 Object *qdev_get_machine(void);
+Object *do_qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
 bool qdev_set_parent_bus(DeviceState *dev, BusState *bus, Error **errp);
diff --git a/stubs/meson.build b/stubs/meson.build
index be6f6d609e58..b99ee2b33e94 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -54,3 +54,4 @@ if have_system
 else
   stub_ss.add(files('qdev.c'))
 endif
+stub_ss.add(files('qdev-get-machine.c'))
diff --git a/stubs/qdev-get-machine.c b/stubs/qdev-get-machine.c
new file mode 100644
index ..ed4cdaa01900
--- /dev/null
+++ b/stubs/qdev-get-machine.c
@@ -0,0 +1,11 @@
+#include "qemu/osdep.h"
+#include "hw/qdev-core.h"
+
+Object *do_qdev_get_machine(void)
+{
+/*
+ * This will create a "container" and add it to the QOM tree, if there
+ * isn't one already.
+ */
+return 

[PATCH for-6.0 2/2] block/nbd: ensure ->connection_thread is always valid

[Roman Kagan]

Simplify lifetime management of BDRVNBDState->connection_thread by
delaying the possible cleanup of it until the BDRVNBDState itself goes
away.

This also fixes possible use-after-free in nbd_co_establish_connection
when it races with nbd_co_establish_connection_cancel.

Signed-off-by: Roman Kagan 
---
This is a more future-proof version of the fix for use-after-free but
less intrusive than Vladimir's series so that it can go in 6.0.

 block/nbd.c | 58 ++---
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index d86df3afcb..6e3879c0c5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -144,16 +144,31 @@ typedef struct BDRVNBDState {
 NBDConnectThread *connect_thread;
 } BDRVNBDState;
 
+static void nbd_free_connect_thread(NBDConnectThread *thr);
 static int nbd_establish_connection(BlockDriverState *bs, SocketAddress *saddr,
 Error **errp);
 static int nbd_co_establish_connection(BlockDriverState *bs, Error **errp);
-static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
-   bool detach);
+static void nbd_co_establish_connection_cancel(BlockDriverState *bs);
 static int nbd_client_handshake(BlockDriverState *bs, Error **errp);
 static void nbd_yank(void *opaque);
 
 static void nbd_clear_bdrvstate(BDRVNBDState *s)
 {
+NBDConnectThread *thr = s->connect_thread;
+bool thr_running;
+
+qemu_mutex_lock(>mutex);
+thr_running = thr->state == CONNECT_THREAD_RUNNING;
+if (thr_running) {
+thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+}
+qemu_mutex_unlock(>mutex);
+
+/* the runaway thread will clean it up itself */
+if (!thr_running) {
+nbd_free_connect_thread(thr);
+}
+
 object_unref(OBJECT(s->tlscreds));
 qapi_free_SocketAddress(s->saddr);
 s->saddr = NULL;
@@ -293,7 +308,7 @@ static void coroutine_fn 
nbd_client_co_drain_begin(BlockDriverState *bs)
 qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
 }
 
-nbd_co_establish_connection_cancel(bs, false);
+nbd_co_establish_connection_cancel(bs);
 
 reconnect_delay_timer_del(s);
 
@@ -333,7 +348,7 @@ static void nbd_teardown_connection(BlockDriverState *bs)
 if (s->connection_co_sleep_ns_state) {
 qemu_co_sleep_wake(s->connection_co_sleep_ns_state);
 }
-nbd_co_establish_connection_cancel(bs, true);
+nbd_co_establish_connection_cancel(bs);
 }
 if (qemu_in_coroutine()) {
 s->teardown_co = qemu_coroutine_self();
@@ -534,18 +549,12 @@ nbd_co_establish_connection(BlockDriverState *bs, Error 
**errp)
  * nbd_co_establish_connection_cancel
  * Cancel nbd_co_establish_connection asynchronously: it will finish soon, to
  * allow drained section to begin.
- *
- * If detach is true, also cleanup the state (or if thread is running, move it
- * to CONNECT_THREAD_RUNNING_DETACHED state). s->connect_thread becomes NULL if
- * detach is true.
  */
-static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
-   bool detach)
+static void nbd_co_establish_connection_cancel(BlockDriverState *bs)
 {
 BDRVNBDState *s = bs->opaque;
 NBDConnectThread *thr = s->connect_thread;
 bool wake = false;
-bool do_free = false;
 
 qemu_mutex_lock(>mutex);
 
@@ -556,21 +565,10 @@ static void 
nbd_co_establish_connection_cancel(BlockDriverState *bs,
 s->wait_connect = false;
 wake = true;
 }
-if (detach) {
-thr->state = CONNECT_THREAD_RUNNING_DETACHED;
-s->connect_thread = NULL;
-}
-} else if (detach) {
-do_free = true;
 }
 
 qemu_mutex_unlock(>mutex);
 
-if (do_free) {
-nbd_free_connect_thread(thr);
-s->connect_thread = NULL;
-}
-
 if (wake) {
 aio_co_wake(s->connection_co);
 }
@@ -2294,31 +2292,33 @@ static int nbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 return -EEXIST;
 }
 
+nbd_init_connect_thread(s);
+
 /*
  * establish TCP connection, return error if it fails
  * TODO: Configurable retry-until-timeout behaviour.
  */
 if (nbd_establish_connection(bs, s->saddr, errp) < 0) {
-yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name));
-return -ECONNREFUSED;
+ret = -ECONNREFUSED;
+goto fail;
 }
 
 ret = nbd_client_handshake(bs, errp);
 if (ret < 0) {
-yank_unregister_instance(BLOCKDEV_YANK_INSTANCE(bs->node_name));
-nbd_clear_bdrvstate(s);
-return ret;
+goto fail;
 }
 /* successfully connected */
 s->state = NBD_CLIENT_CONNECTED;
 
-nbd_init_connect_thread(s);
-
 s->connection_co = qemu_coroutine_create(nbd_connection_entry, s);
 bdrv_inc_in_flight(bs);
 aio_co_schedule(bdrv_get_aio_context(bs), 

[PATCH 2/2] cpu/core: Fix "help" of CPU core device types

[Greg Kurz]

Calling qdev_get_machine() from a QOM instance_init function is
fragile because we can't be sure the machine object actually
exists. And this happens to break when passing ",help" on the
command line to get the list of properties for a CPU core
device types :

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
qemu-system-ppc64: ../../hw/core/machine.c:1290:
 qdev_get_machine: Assertion `machine != NULL' failed.
Aborted (core dumped)

This used to work before QEMU 5.0, but commit 3df261b6676b
unwillingly introduced a subtle regression : the above command
line needs to create an instance but the instance_init function
of the base class calls qdev_get_machine() before
qemu_create_machine() has been called, which is a programming bug.

Use current_machine instead. It is okay to skip the setting of
nr_thread in this case since only its type is displayed.

Reported-by: Thomas Huth 
Fixes: 3df261b6676b ("softmmu/vl.c: Handle '-cpu help' and '-device help' 
before 'no default machine'")
Cc: peter.mayd...@linaro.org
Signed-off-by: Greg Kurz 
---
 hw/cpu/core.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index 92d3b2fbad62..987607515574 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -66,10 +66,16 @@ static void core_prop_set_nr_threads(Object *obj, Visitor 
*v, const char *name,
 
 static void cpu_core_instance_init(Object *obj)
 {
-MachineState *ms = MACHINE(qdev_get_machine());
 CPUCore *core = CPU_CORE(obj);
 
-core->nr_threads = ms->smp.threads;
+/*
+ * Only '-device something-cpu-core,help' can get us there before
+ * the machine has been created. We don't care to set nr_threads
+ * in this case since it isn't used afterwards.
+ */
+if (current_machine) {
+core->nr_threads = current_machine->smp.threads;
+}
 }
 
 static void cpu_core_class_init(ObjectClass *oc, void *data)
-- 
2.26.3

[PATCH for-6.0 1/2] block/nbd: fix channel object leak

[Roman Kagan]

nbd_free_connect_thread leaks the channel object if it hasn't been
stolen.

Unref it and fix the leak.

Signed-off-by: Roman Kagan 
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index c26dc5a54f..d86df3afcb 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -385,6 +385,7 @@ static void nbd_free_connect_thread(NBDConnectThread *thr)
 {
 if (thr->sioc) {
 qio_channel_close(QIO_CHANNEL(thr->sioc), NULL);
+object_unref(OBJECT(thr->sioc));
 }
 error_free(thr->err);
 qapi_free_SocketAddress(thr->saddr);
-- 
2.30.2

[PATCH for-6.0 0/2] block/nbd: assorted bugfixes

[Roman Kagan]

A couple of bugfixes to block/nbd that look appropriate for 6.0.

Roman Kagan (2):
  block/nbd: fix channel object leak
  block/nbd: ensure ->connection_thread is always valid

 block/nbd.c | 59 +++--
 1 file changed, 30 insertions(+), 29 deletions(-)

-- 
2.30.2

Re: [RFC PATCH v2 01/11] python: qemu: add timer parameter for qmp.accept socket

[Emanuele Giuseppe Esposito]

--- a/python/qemu/qtest.py
+++ b/python/qemu/qtest.py
@@ -138,9 +138,9 @@ def _pre_launch(self) -> None:
  super()._pre_launch()
  self._qtest = QEMUQtestProtocol(self._qtest_path, server=True)
-    def _post_launch(self) -> None:
+    def _post_launch(self, timer) -> None:
  assert self._qtest is not None
-    super()._post_launch()
+    super()._post_launch(timer)
  self._qtest.accept()
  def _post_shutdown(self) -> None:



Are you forgetting to change _launch() to provide some default value for 
what timer needs to be?


I think for the "event" callbacks here, I'd prefer configuring the 
behavior as a property instead of passing it around as a parameter.


I agree, I changed it in a field of the QEMUMachine class called 
_qmp_timer that defaults to 15 seconds.


Emanuele

[PATCH 0/2] cpu/core: Fix "help" of CPU core device types

[Greg Kurz]

Thomas Huth recently reported an annoying crash:

$ ./qemu-system-ppc64 -device power8_v2.0-spapr-cpu-core,help
/home/thuth/devel/qemu/include/hw/boards.h:24:
 MACHINE: Object 0x5635bd53af10 is not an instance of type machine
Aborted (core dumped)

This is caused by an early use of qdev_get_machine(), before the
machine creation, which triggers a side-effect of creating a
dummy "container" object instead of the machine. This is needed
by user mode emulation, which doesn't really care about the
type of the parent of the CPU model. This is toxic for system
mode though because the system mode specific code usually assume
MACHINE(qdev_get_machine()).

This series brings separate implementations between user and
system mode. The breakage with "cpu-code,help" is fixed by using
current_machine.

Greg Kurz (2):
  qdev: Separate implementations of qdev_get_machine() for user and
system
  cpu/core: Fix "help" of CPU core device types

 hw/core/machine.c| 14 ++
 hw/core/qdev.c   |  2 +-
 hw/cpu/core.c| 10 --
 include/hw/qdev-core.h   |  1 +
 stubs/meson.build|  1 +
 stubs/qdev-get-machine.c | 11 +++
 6 files changed, 36 insertions(+), 3 deletions(-)
 create mode 100644 stubs/qdev-get-machine.c

-- 
2.26.3

Re: [RFC PATCH 0/4] target/ppc: add disable-tcg option

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20210409151916.97326-1-bruno.lar...@eldorado.org.br/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210409151916.97326-1-bruno.lar...@eldorado.org.br
Subject: [RFC PATCH 0/4] target/ppc: add disable-tcg option

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  
patchew/161786467973.295167.5612704777283969903.st...@bahia.lan -> 
patchew/161786467973.295167.5612704777283969903.st...@bahia.lan
 - [tag update]  patchew/20210409150527.15053-1-peter.mayd...@linaro.org -> 
patchew/20210409150527.15053-1-peter.mayd...@linaro.org
 * [new tag] 
patchew/20210409151916.97326-1-bruno.lar...@eldorado.org.br -> 
patchew/20210409151916.97326-1-bruno.lar...@eldorado.org.br
Switched to a new branch 'test'
0250bc9 target/ppc: updated build rules for disable-tcg option
e36c2a7 target/ppc: Add stubs for tcg functions, so it builds
4e6d44d target/ppc: added solutions for building with disable-tcg
38ccad3 target/ppc: Code motion required to build disabling tcg

=== OUTPUT BEGIN ===
1/4 Checking commit 38ccad308a44 (target/ppc: Code motion required to build 
disabling tcg)
2/4 Checking commit 4e6d44d2a68a (target/ppc: added solutions for building with 
disable-tcg)
WARNING: Block comments use a leading /* on a separate line
#43: FILE: target/ppc/arch_dump.c:182:
+/* This is the first solution implemented. My personal favorite as it

WARNING: Block comments use a trailing */ on a separate line
#44: FILE: target/ppc/arch_dump.c:183:
+ * allows for explicit error handling, however it is much less readable */

ERROR: space required before the open brace '{'
#46: FILE: target/ppc/arch_dump.c:185:
+if(kvm_enabled()){

ERROR: space required before the open parenthesis '('
#46: FILE: target/ppc/arch_dump.c:185:
+if(kvm_enabled()){

ERROR: space required after that close brace '}'
#48: FILE: target/ppc/arch_dump.c:187:
+}else

WARNING: line over 80 characters
#55: FILE: target/ppc/arch_dump.c:194:
+/* TODO: add proper error handling, even tough this should never be 
reached */

ERROR: space required before the open brace '{'
#79: FILE: target/ppc/kvm.c:2953:
+int kvmppc_mtvscr(PowerPCCPU *cpu, uint32_t val){

ERROR: space required before the open brace '{'
#87: FILE: target/ppc/kvm.c:2961:
+if(ret < 0){

ERROR: space required before the open parenthesis '('
#87: FILE: target/ppc/kvm.c:2961:
+if(ret < 0){

ERROR: space required before the open brace '{'
#93: FILE: target/ppc/kvm.c:2967:
+int kvmppc_mfvscr(PowerPCCPU *cpu){

ERROR: space required before the open brace '{'
#101: FILE: target/ppc/kvm.c:2975:
+if(ret < 0){

ERROR: space required before the open parenthesis '('
#101: FILE: target/ppc/kvm.c:2975:
+if(ret < 0){

ERROR: "(foo*)" should be "(foo *)"
#115: FILE: target/ppc/kvm_ppc.h:90:
+int kvmppc_mfvscr(PowerPCCPU*);

WARNING: Block comments use a leading /* on a separate line
#117: FILE: target/ppc/kvm_ppc.h:92:
+/* This is the second (quick and dirty) solution. Not my personal favorite

WARNING: Block comments use a trailing */ on a separate line
#119: FILE: target/ppc/kvm_ppc.h:94:
+ * for error checking. but it requires less change in other files */

ERROR: space required after that ',' (ctx:VxV)
#121: FILE: target/ppc/kvm_ppc.h:96:
+#define helper_mtvscr(env, val) kvmppc_mtvscr(env_archcpu(env),val)
   ^

ERROR: space required before the open brace '{'
#148: FILE: target/ppc/machine.c:101:
+if(kvm_enabled()){

ERROR: space required before the open parenthesis '('
#148: FILE: target/ppc/machine.c:101:
+if(kvm_enabled()){

ERROR: space required after that close brace '}'
#150: FILE: target/ppc/machine.c:103:
+}else

WARNING: line over 80 characters
#156: FILE: target/ppc/machine.c:109:
+/* TODO: Add correct error handling, even though this should never be 
reached */

ERROR: space required before the open brace '{'
#167: FILE: target/ppc/machine.c:467:
+if(kvm_enabled()){

ERROR: space required before the open parenthesis '('
#167: FILE: target/ppc/machine.c:467:
+if(kvm_enabled()){

ERROR: space required before the open brace '{'
#184: FILE: target/ppc/machine.c:484:
+if(kvm_enabled()){

ERROR: space required before the open parenthesis '('
#184: FILE: target/ppc/machine.c:484:
+if(kvm_enabled()){

total: 18 errors, 6 warnings, 147 lines checked

Patch 2/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/4 Checking commit e36c2a70087a 

Re: [Virtio-fs] [PATCH] vhost-user-fs: fix features handling

[Vivek Goyal]

On Thu, Apr 08, 2021 at 10:55:34PM +0300, Anton Kuchin wrote:
> Make virtio-fs take into account server capabilities.
> 
> Just returning requested features assumes they all of then are implemented
> by server and results in setting unsupported configuration if some of them
> are absent.
> 
> Signed-off-by: Anton Kuchin 

[CC stefan and qemu-devel.]

Can you give more details of what problem exactly you are facing. Or
this fix is about avoiding a future problem where device can refuse
to support a feature qemu is requesting for.

IIUC, this patch is preparing a list of features vhost-user-fs device
can support. Then it calls vhost_get_features() which makes sure that
all these features are support by real vhost-user device (hdev->features).
If not, then corresponding feature is reset and remaining features
are returned to caller.

This feature negotion bit is called in so many places that I am kind of
lost that who should be doing what. Will leave it to Stefan who
understands it much better.


> ---
>  hw/virtio/vhost-user-fs.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index ac4fc34b36..6cf983ba0e 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -24,6 +24,14 @@
>  #include "monitor/monitor.h"
>  #include "sysemu/sysemu.h"
>  
> +static const int user_feature_bits[] = {
> +VIRTIO_F_VERSION_1,
> +VIRTIO_RING_F_INDIRECT_DESC,
> +VIRTIO_RING_F_EVENT_IDX,
> +VIRTIO_F_NOTIFY_ON_EMPTY,
> +VHOST_INVALID_FEATURE_BIT
> +};
> +
>  static void vuf_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>  VHostUserFS *fs = VHOST_USER_FS(vdev);
> @@ -129,11 +137,12 @@ static void vuf_set_status(VirtIODevice *vdev, uint8_t 
> status)
>  }
>  
>  static uint64_t vuf_get_features(VirtIODevice *vdev,
> -  uint64_t requested_features,
> -  Error **errp)
> + uint64_t features,

Will it make sense to keep the name requested_features. This kind of
makes it clear that caller is requesting these features.

I feel there should be few lines of comments also to make it clear
what this function is actually doing.

Vivek

> + Error **errp)
>  {
> -/* No feature bits used yet */
> -return requested_features;
> +VHostUserFS *fs = VHOST_USER_FS(vdev);
> +
> +return vhost_get_features(>vhost_dev, user_feature_bits, features);
>  }
>  
>  static void vuf_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> -- 
> 2.25.1
> 
> 
> ___
> Virtio-fs mailing list
> virtio...@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs

Re: [PATCH v2 3/7] ACPI ERST: support for ACPI ERST feature

[Eric DeVolder]

Hi Igor,
Thank you for reviewing. I've responded inline below.
eric


From: Igor Mammedov 
Sent: Tuesday, April 6, 2021 2:31 PM
To: Eric DeVolder 
Cc: m...@redhat.com ; marcel.apfelb...@gmail.com 
; pbonz...@redhat.com ; 
r...@twiddle.net ; ehabk...@redhat.com ; 
qemu-devel@nongnu.org ; Boris Ostrovsky 
; kw...@oracle.com 
Subject: Re: [PATCH v2 3/7] ACPI ERST: support for ACPI ERST feature

On Mon,  8 Feb 2021 15:57:55 -0500
Eric DeVolder  wrote:

> This change implements the support for the ACPI ERST feature[1,2].
>
> The size of the ACPI ERST storage is declared via the QEMU
> global parameter acpi-erst.size. The size can range from 64KiB
> to to 64MiB. The default is 64KiB.
>
> The location of the ACPI ERST storage backing file is delared
> via the QEMU global parameter acpi-erst.filename. The default
> is acpi-erst.backing.
>
> [1] "Advanced Configuration and Power Interface Specification",
> version 6.2, May 2017.
> https://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf
>
> [2] "Unified Extensible Firmware Interface Specification",
> version 2.8, March 2019.
> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
>
> Signed-off-by: Eric DeVolder 

items 2/4/5 from v1 review still need to be addressed.

>
> 2. patch is too big to review, please split it up in smaller chunks.
>
> EJD: Done.

(separating a header and a makefile rule doesn't make much sense)

it should be split at least on part that implements device model and ACPI parts

EJD: I'll rebase this patch set on qemu-6 and accommodate your suggestions with 
how to split/organize the patch set.

[...]
>
> 4. Maybe instead of SYSBUS device, implement it as a PCI device and
>use its BAR/control registers for pstore storage and control interface.
>It could save you headache of picking address where to map it +
>it would take care of migration part automatically, as firmware
>would do it for you and then QEMU could pickup firmware programmed
>address and put it into ERST table.
> EJD: Thanks for the idea. For now I've left it as a SYSBUS device; we can 
> revisit as needed.

I would really prefer to see a PCI version (current way is just a hack)

EJD: I understand, I don't like the base address problem either. Is there an 
example PCI device that gets its base address assigned during ACPI setup that I 
could reference and pattern this work after? I've been using SYSBUS as that 
most closely mimics the real hardware implementations I've studied in order to 
produce this code.
EJD: I thought my inexperience with authoring QEMU devices was the primary 
problem in establishing a solution for the base address. Otherwise, this thing 
only needs a single 4KiB page (for the 2 registers + exchange buffer) exposed.

> 5. instead of dealing with file for storage directly, reuse hostmem backend
>to provide it to for your device. ex: pc-dimm. i.e. split device
>on frontend and backend
>
> EJD: I had looked into that prior to posting v1. The entire ERST storage is 
> not memory mapped, just an exchange buffer. So the hostmem backend is not 
> suitable for this purpose.

Is there a compelling reason why it can't be memory mapped?

EJD: Well, this ERST device I've coded pretty much follows the ACPI ERST spec 
verbatim. As it stands today, the spec doesn't provide a way to report the 
total size of the persistent storage behind the interface; you know when 
storage is full only when you receive an Out Of Storage error code upon write. 
In a sense, that allows the size of the storage to vary greatly and be 
implemented in any way needed (ie actual hardware, this has tended to be in the 
64KiB range when it is carved out of system parallel flash memory, but some 
hardware uses serial flash as well). In virtual environments, it can be of any 
size, and we at Oracle have intentions of heavily utilizing ACPI ERST to stuff 
all kinds of diagnostic information into it, thus wanting the storage to be 
very large. By not actually exposing/memory-mapping the storage, the issue of 
where to drop it in the memory map goes away (yes a PCI BAR could solve this).
EJD: But at the end of the day, could this storage be memory mapped? I suppose 
it could be, but then that rather circumvents the entire need for the ACPI ERST 
interface to start with. Linux and Windows both already know how to utilize 
ACPI ERST.




> ---
>  hw/acpi/erst.c | 952 
> +
>  1 file changed, 952 insertions(+)
>  create mode 100644 hw/acpi/erst.c
>
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> new file mode 100644
> index 000..3a342f9
> --- /dev/null
> +++ b/hw/acpi/erst.c
> @@ -0,0 +1,952 @@
> +/*
> + * ACPI Error Record Serialization Table, ERST, Implementation
> + *
> + * Copyright (c) 2020 Oracle and/or its affiliates.
> + *
> + * See ACPI specification,
> + * "ACPI Platform Error Interfaces" : "Error Serialization"
> + *
> + * This library 

[RFC PATCH 0/4] target/ppc: add disable-tcg option

[Bruno Larsen (billionai)]

This patch series aims to add the option to build without TCG for the
powerpc target. This RFC shows mostly the strategies employed when
dealing with compilation problems, and ask for input on the bits
we don't quite understand yet.

The first patch mostly code motion, as referenced in 2021-04/msg0717.
The second patch shows the 2 strategies we've considered, and hope to
get feedback on. The third patch contains the stubs we haven't decided
on how to deal with yet, but needed to exist to compile the project.
The final patch just changes the meson.build rules

Bruno Larsen (billionai) (4):
  target/ppc: Code motion required to build disabling tcg
  target/ppc: added solutions for problems encountered when building
with disable-tcg
  target/ppc: Add stubs for tcg functions, so it build with disable-tcg
  target/ppc: updated build rules for disable-tcg option

 target/ppc/arch_dump.c  |   17 +
 target/ppc/cpu.c|  859 +++
 target/ppc/cpu.h|   15 +
 target/ppc/gdbstub.c|  253 +++
 target/ppc/kvm.c|   30 +
 target/ppc/kvm_ppc.h|   11 +
 target/ppc/machine.c|   33 +-
 target/ppc/meson.build  |   22 +-
 target/ppc/tcg-stub.c   |  139 
 target/ppc/translate_init.c.inc | 1148 +--
 10 files changed, 1407 insertions(+), 1120 deletions(-)
 create mode 100644 target/ppc/tcg-stub.c

-- 
2.17.1

[PATCH 3/4] target/ppc: Add stubs for tcg functions, so it builds

[Bruno Larsen (billionai)]

This file basically adds all stubs required to build the project
with disable-tcg. most of these are not going to remain stubs by the
end, but this part is where it got complicated, and I wanted to get
an RFC ASAP. Most of these have to do with mmu emulation, so they'll
probably be replaced by a KVM implementation in the final product,
but I'm not sure which ones have to be replace, which can remain
stubs, and which should not be called at all. Input in general is
very much welcome.

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/tcg-stub.c | 139 ++
 1 file changed, 139 insertions(+)
 create mode 100644 target/ppc/tcg-stub.c

diff --git a/target/ppc/tcg-stub.c b/target/ppc/tcg-stub.c
new file mode 100644
index 00..5dc8cf8911
--- /dev/null
+++ b/target/ppc/tcg-stub.c
@@ -0,0 +1,139 @@
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "mmu-hash64.h"
+
+/* STUFF FOR FIRST LINKER ERROR */
+/* This stuff happens in target/ppc files */
+
+#if !defined(CONFIG_USER_ONLY)
+
+void ppc_store_sdr1(CPUPPCState *env, target_ulong value) {
+/* stub to make things compile */
+return;
+}
+
+void ppc_store_ptcr(CPUPPCState *env, target_ulong value) {
+/* stub to make things compile */
+return;
+}
+
+#endif /* !defined(CONFIG_USER_ONLY) */
+void ppc_store_msr(CPUPPCState *env, target_ulong value) {
+/* stub to make things compile */
+return;
+}
+
+void dump_mmu(CPUPPCState *env){
+/* stub to make things compile */
+return;
+}
+
+void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask) {
+/* stub to make things compile */
+return;
+}
+
+void ppc_cpu_do_interrupt(CPUState *cpu) {
+/* stub to make things compile */
+return;
+}
+
+/* STUFF FOR SECOND LINKER ERROR*/
+/* these errors happen mostly in hw/ppc */
+
+#ifdef TARGET_PPC64
+int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
+  target_ulong esid, target_ulong vsid) {
+/* rquired by kvm.c and machine.c */
+return 0;
+}
+
+void ppc_hash64_filter_pagesizes(PowerPCCPU *cpu,
+ bool (*cb)(void *, uint32_t, uint32_t),
+ void *opaque) {
+/* required by spapr_caps.c */
+return; 
+}
+
+void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val) {
+/* required by spapr_* */
+return;
+}
+
+const ppc_hash_pte64_t *ppc_hash64_map_hptes(PowerPCCPU *cpu,
+ hwaddr ptex, int n) {
+/* used by spapr_hcall a bunch */
+return NULL;
+}
+
+void ppc_hash64_unmap_hptes(PowerPCCPU *cpu, const ppc_hash_pte64_t *hptes,
+hwaddr ptex, int n) {
+/* used a bunch by spapr_hcall */
+return; 
+}
+
+void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
+   target_ulong pte_index,
+   target_ulong pte0, target_ulong pte1){
+return; 
+}
+
+unsigned ppc_hash64_hpte_page_shift_noslb(PowerPCCPU *cpu,
+  uint64_t pte0, uint64_t pte1) {
+return 0;
+}
+#endif
+
+void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector) {
+/* required by spapr_events spapr_mce_dispatch_elog */
+return;
+}
+#ifndef CONFIG_USER_ONLY
+void ppc_cpu_do_system_reset(CPUState *cs){
+/* required by pnv and spapr */
+return;
+}
+#endif
+
+bool ppc64_v3_get_pate(PowerPCCPU *cpu, target_ulong lpid,
+   ppc_v3_pate_t *entry);
+
+bool ppc64_v3_get_pate(PowerPCCPU *cpu, target_ulong lpid,
+   ppc_v3_pate_t *entry) {
+/* used by spapr_hcall: ppc_hash64_hpt_mask */
+return true;
+}
+
+/* THIRD BATCH OF ERRORS, AFTER MOVING STUFF FROM TRANSLATE TO CPU.C */
+
+/* they are all coming from cpu.c, probably */
+
+void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp) {
+return;
+}
+
+void init_ppc_proc(PowerPCCPU *cpu) {
+return;
+}
+
+void destroy_ppc_opcodes(PowerPCCPU *cpu) {
+return;
+}
+
+void ppc_tlb_invalidate_all(CPUPPCState *env) {
+return;
+}
+
+void ppc_cpu_dump_state(CPUState *cpu, FILE *f, int flags) {
+return;
+}
+
+void ppc_cpu_dump_statistics(CPUState *cpu, int flags) {
+return;
+}
+
+#include "exec/hwaddr.h"
+
+hwaddr ppc_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr) {
+return 0;
+}
-- 
2.17.1

[PATCH 2/4] target/ppc: added solutions for building with disable-tcg

[Bruno Larsen (billionai)]

this commit presents 2 possible solutions for substituting TCG emulation
with KVM calls. One - used in machine.c and arch_dump.c - explicitly
adds the KVM function and has the possibility of adding the TCG one
for more generic compilation, prioritizing te KVM option. The second
option, implemented in kvm_ppc.h, transparently changes the helper
into the KVM call, if TCG is not enabled. I believe the first solution
is better, but it is less readable, so I wanted to have some feedback

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/arch_dump.c | 17 +
 target/ppc/kvm.c   | 30 ++
 target/ppc/kvm_ppc.h   | 11 +++
 target/ppc/machine.c   | 33 -
 4 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/target/ppc/arch_dump.c b/target/ppc/arch_dump.c
index 9ab04b2c38..c53e01011a 100644
--- a/target/ppc/arch_dump.c
+++ b/target/ppc/arch_dump.c
@@ -17,7 +17,10 @@
 #include "elf.h"
 #include "sysemu/dump.h"
 #include "sysemu/kvm.h"
+#include "kvm_ppc.h"
+#if defined(CONFIG_TCG)
 #include "exec/helper-proto.h"
+#endif /* CONFIG_TCG */
 
 #ifdef TARGET_PPC64
 #define ELFCLASS ELFCLASS64
@@ -176,7 +179,21 @@ static void ppc_write_elf_vmxregset(NoteFuncArg *arg, 
PowerPCCPU *cpu)
 vmxregset->avr[i].u64[1] = avr->u64[1];
 }
 }
+/* This is the first solution implemented. My personal favorite as it
+ * allows for explicit error handling, however it is much less readable */
+#if defined(CONFIG_KVM)
+if(kvm_enabled()){
+vmxregset->vscr.u32[3] = cpu_to_dump32(s, kvmppc_mfvscr(cpu));
+}else
+#endif
+
+#if defined(CONFIG_TCG)
 vmxregset->vscr.u32[3] = cpu_to_dump32(s, helper_mfvscr(>env));
+#else
+{
+/* TODO: add proper error handling, even tough this should never be 
reached */
+}
+#endif
 }
 
 static void ppc_write_elf_vsxregset(NoteFuncArg *arg, PowerPCCPU *cpu)
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 104a308abb..8ed54d12d8 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -51,6 +51,7 @@
 #include "elf.h"
 #include "sysemu/kvm_int.h"
 
+
 #define PROC_DEVTREE_CPU  "/proc/device-tree/cpus/"
 
 #define DEBUG_RETURN_GUEST 0
@@ -2947,3 +2948,32 @@ bool kvm_arch_cpu_check_are_resettable(void)
 {
 return true;
 }
+
+/* Functions added to replace helper_m(t|f)vscr from int_helper.c */
+int kvmppc_mtvscr(PowerPCCPU *cpu, uint32_t val){
+CPUState *cs = CPU(cpu);
+CPUPPCState *env = >env;
+struct kvm_one_reg reg;
+int ret;
+reg.id = KVM_REG_PPC_VSCR;
+reg.addr = (uintptr_t) >vscr;
+ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, );
+if(ret < 0){
+fprintf(stderr, "Unable to set VSCR to KVM: %s", strerror(errno));
+}
+return ret;
+}
+
+int kvmppc_mfvscr(PowerPCCPU *cpu){
+CPUState *cs = CPU(cpu);
+CPUPPCState *env = >env;
+struct kvm_one_reg reg;
+int ret;
+reg.id = KVM_REG_PPC_VSCR;
+reg.addr = (uintptr_t) >vscr;
+ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, );
+if(ret < 0){
+fprintf(stderr, "Unable to get VSCR to KVM: %s", strerror(errno));
+}
+return ret;
+}
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 989f61ace0..f618cb28b1 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -86,6 +86,17 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t 
tb_offset);
 
 int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
 
+int kvmppc_mtvscr(PowerPCCPU*, uint32_t);
+int kvmppc_mfvscr(PowerPCCPU*);
+
+/* This is the second (quick and dirty) solution. Not my personal favorite
+ * as it hides what is actually happening from the user and doesn't allow
+ * for error checking. but it requires less change in other files */
+#ifndef CONFIG_TCG
+#define helper_mtvscr(env, val) kvmppc_mtvscr(env_archcpu(env),val)
+#define helper_mfvscr(env) kvmppc_mfvscr(env_archcpu(env))
+#endif
+
 #else
 
 static inline uint32_t kvmppc_get_tbfreq(void)
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 283db1d28a..d92bc18859 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -8,7 +8,9 @@
 #include "qapi/error.h"
 #include "qemu/main-loop.h"
 #include "kvm_ppc.h"
+#ifdef CONFIG_TCG
 #include "exec/helper-proto.h"
+#endif /*CONFIG_TCG*/
 
 static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
 {
@@ -95,7 +97,18 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
version_id)
 ppc_store_sdr1(env, sdr1);
 }
 qemu_get_be32s(f, );
-helper_mtvscr(env, vscr);
+#if defined(CONFIG_KVM)
+if(kvm_enabled()){
+kvmppc_mtvscr(cpu, vscr);
+}else
+#endif
+#if defined(CONFIG_TCG)
+helper_mtvscr(env, vscr);
+#else
+{
+/* TODO: Add correct error handling, even though this should never be 
reached */
+}
+#endif
 qemu_get_be64s(f, >spe_acc);
 qemu_get_be32s(f, >spe_fscr);
 qemu_get_betls(f, >msr_mask);
@@ -450,7 +463,16 @@ static int get_vscr(QEMUFile *f, 

[PATCH 4/4] target/ppc: updated build rules for disable-tcg option

[Bruno Larsen (billionai)]

updated the meson file to respect the disable-tcg option and only add
relevant files to the build process

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/meson.build | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/target/ppc/meson.build b/target/ppc/meson.build
index bbfef90e08..23f9346a6e 100644
--- a/target/ppc/meson.build
+++ b/target/ppc/meson.build
@@ -2,32 +2,40 @@ ppc_ss = ss.source_set()
 ppc_ss.add(files(
   'cpu-models.c',
   'cpu.c',
+  'gdbstub.c',
+))
+
+ppc_ss.add(libdecnumber)
+
+ppc_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'), if_false: 
files('kvm-stub.c'))
+ppc_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user_only_helper.c'))
+ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
   'dfp_helper.c',
   'excp_helper.c',
   'fpu_helper.c',
-  'gdbstub.c',
   'int_helper.c',
   'mem_helper.c',
   'misc_helper.c',
   'timebase_helper.c',
   'translate.c',
-))
+), if_false: files('tcg-stub.c'))
 
-ppc_ss.add(libdecnumber)
-
-ppc_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'), if_false: 
files('kvm-stub.c'))
-ppc_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user_only_helper.c'))
 
 ppc_softmmu_ss = ss.source_set()
 ppc_softmmu_ss.add(files(
   'arch_dump.c',
   'machine.c',
+  'monitor.c',
+))
+ppc_softmmu_ss.add(when: 'CONFIG_TCG', if_true: files(
   'mmu-hash32.c',
   'mmu_helper.c',
-  'monitor.c',
 ))
+
 ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
   'compat.c',
+))
+ppc_softmmu_ss.add(when: ['TARGET_PPC64', 'CONFIG_TCG'], if_true: files(
   'mmu-book3s-v3.c',
   'mmu-hash64.c',
   'mmu-radix64.c',
-- 
2.17.1

[PATCH 1/4] target/ppc: Code motion required to build disabling tcg

[Bruno Larsen (billionai)]

This commit does the necessary code motion from translate_init.c.inc
This moves all functions that start with gdb_* into target/ppc/gdbstub.c
and creates a new function that calls those and is called by ppc_cpu_realize
All functions related to realizing the cpu have been moved to cpu.c, which
may call functions from gdbstub or translate_init

Signed-off-by: Bruno Larsen (billionai) 
---
 target/ppc/cpu.c|  859 +++
 target/ppc/cpu.h|   15 +
 target/ppc/gdbstub.c|  253 +++
 target/ppc/translate_init.c.inc | 1148 +--
 4 files changed, 1163 insertions(+), 1112 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index e501a7ff6f..b77ea1c943 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -20,6 +20,21 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "cpu-models.h"
+#include "kvm_ppc.h"
+#include "qemu/qemu-print.h"
+#include "qemu/cutils.h"
+#include "qapi/qapi-commands-machine-target.h"
+
+#include "qapi/error.h"
+#include "sysemu/tcg.h"
+#include "helper_regs.h"
+#include "hw/ppc/ppc.h"
+#include "fpu/softfloat-helpers.h"
+#include "sysemu/hw_accel.h"
+#include "disas/capstone.h"
+#include "hw/qdev-properties.h"
+#include "internal.h"
+#include "mmu-hash64.h"
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
@@ -45,3 +60,847 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer)
(1ul << XER_OV) | (1ul << XER_CA) |
(1ul << XER_OV32) | (1ul << XER_CA32));
 }
+
+
+static const char *ppc_cpu_lookup_alias(const char *alias)
+{
+int ai;
+
+for (ai = 0; ppc_cpu_aliases[ai].alias != NULL; ai++) {
+if (strcmp(ppc_cpu_aliases[ai].alias, alias) == 0) {
+return ppc_cpu_aliases[ai].model;
+}
+}
+
+return NULL;
+}
+
+static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
+{
+ObjectClass *oc = (ObjectClass *)a;
+uint32_t pvr = *(uint32_t *)b;
+PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
+
+/* -cpu host does a PVR lookup during construction */
+if (unlikely(strcmp(object_class_get_name(oc),
+TYPE_HOST_POWERPC_CPU) == 0)) {
+return -1;
+}
+
+return pcc->pvr == pvr ? 0 : -1;
+}
+
+static gint ppc_cpu_compare_class_pvr_mask(gconstpointer a, gconstpointer b)
+{
+ObjectClass *oc = (ObjectClass *)a;
+uint32_t pvr = *(uint32_t *)b;
+PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
+
+/* -cpu host does a PVR lookup during construction */
+if (unlikely(strcmp(object_class_get_name(oc),
+TYPE_HOST_POWERPC_CPU) == 0)) {
+return -1;
+}
+
+if (pcc->pvr_match(pcc, pvr)) {
+return 0;
+}
+
+return -1;
+}
+
+PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
+{
+GSList *list, *item;
+PowerPCCPUClass *pcc = NULL;
+
+list = object_class_get_list(TYPE_POWERPC_CPU, false);
+item = g_slist_find_custom(list, , ppc_cpu_compare_class_pvr);
+if (item != NULL) {
+pcc = POWERPC_CPU_CLASS(item->data);
+}
+g_slist_free(list);
+
+return pcc;
+}
+
+PowerPCCPUClass *ppc_cpu_class_by_pvr_mask(uint32_t pvr)
+{
+GSList *list, *item;
+PowerPCCPUClass *pcc = NULL;
+
+list = object_class_get_list(TYPE_POWERPC_CPU, true);
+item = g_slist_find_custom(list, , ppc_cpu_compare_class_pvr_mask);
+if (item != NULL) {
+pcc = POWERPC_CPU_CLASS(item->data);
+}
+g_slist_free(list);
+
+return pcc;
+}
+
+PowerPCCPUClass *ppc_cpu_get_family_class(PowerPCCPUClass *pcc)
+{
+ObjectClass *oc = OBJECT_CLASS(pcc);
+
+while (oc && !object_class_is_abstract(oc)) {
+oc = object_class_get_parent(oc);
+}
+assert(oc);
+
+return POWERPC_CPU_CLASS(oc);
+}
+
+/* Sort by PVR, ordering special case "host" last. */
+static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+ObjectClass *oc_a = (ObjectClass *)a;
+ObjectClass *oc_b = (ObjectClass *)b;
+PowerPCCPUClass *pcc_a = POWERPC_CPU_CLASS(oc_a);
+PowerPCCPUClass *pcc_b = POWERPC_CPU_CLASS(oc_b);
+const char *name_a = object_class_get_name(oc_a);
+const char *name_b = object_class_get_name(oc_b);
+
+if (strcmp(name_a, TYPE_HOST_POWERPC_CPU) == 0) {
+return 1;
+} else if (strcmp(name_b, TYPE_HOST_POWERPC_CPU) == 0) {
+return -1;
+} else {
+/* Avoid an integer overflow during subtraction */
+if (pcc_a->pvr < pcc_b->pvr) {
+return -1;
+} else if (pcc_a->pvr > pcc_b->pvr) {
+return 1;
+} else {
+return 0;
+}
+}
+}
+#ifndef CONFIG_USER_ONLY
+static const TypeInfo ppc_vhyp_type_info = {
+.name = TYPE_PPC_VIRTUAL_HYPERVISOR,
+.parent = TYPE_INTERFACE,
+.class_size = sizeof(PPCVirtualHypervisorClass),
+};
+#endif
+
+static ObjectClass *ppc_cpu_class_by_name(const char *name)
+{
+char *cpu_model, *typename;
+ObjectClass 

Re: [PATCH v3] hw/block/nvme: add device self test command support

[Keith Busch]

On Wed, Mar 31, 2021 at 02:54:27PM +0530, Gollu Appalanaidu wrote:
> This is to add support for Device Self Test Command (DST) and
> DST Log Page. Refer NVM Express specification 1.4b section 5.8
> ("Device Self-test command")

Please don't write change logs that just say what you did. I can read
the code to see that. Explain why this is useful because this frankly
looks like another useless feature. We don't need to implement every
optional spec feature here. There should be a real value proposition.

Re: [PATCH v2] monitor/qmp: fix race on CHR_EVENT_CLOSED without OOB

[Markus Armbruster]

I considered this for 6.0, but decided not to rock the boat at this late
stage.

Queued for 6.1, thanks!

Re: [PATCH] hw/block/nvme: slba equal to nsze is out of bounds if nlb is 1-based

[Keith Busch]

On Fri, Apr 09, 2021 at 01:55:01PM +0200, Klaus Jensen wrote:
> On Apr  9 20:05, Minwoo Im wrote:
> > On 21-04-09 13:14:02, Gollu Appalanaidu wrote:
> > > NSZE is the total size of the namespace in logical blocks. So the max
> > > addressable logical block is NLB minus 1. So your starting logical
> > > block is equal to NSZE it is a out of range.
> > > 
> > > Signed-off-by: Gollu Appalanaidu 
> > > ---
> > >  hw/block/nvme.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 953ec64729..be9edb1158 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -2527,7 +2527,7 @@ static uint16_t nvme_dsm(NvmeCtrl *n, NvmeRequest 
> > > *req)
> > >  uint64_t slba = le64_to_cpu(range[i].slba);
> > >  uint32_t nlb = le32_to_cpu(range[i].nlb);
> > > 
> > > -if (nvme_check_bounds(ns, slba, nlb)) {
> > > +if (nvme_check_bounds(ns, slba, nlb) || slba == 
> > > ns->id_ns.nsze) {
> > 
> > This patch also looks like check the boundary about slba.  Should it be
> > also checked inside of nvme_check_bounds() ?
> 
> The catch here is that DSM is like the only command where the number of
> logical blocks is a 1s-based value. Otherwise we always have nlb > 0, which
> means that nvme_check_bounds() will always "do the right thing".
> 
> My main gripe here is that (in my mind), by definition, a "zero length
> range" does not reference any LBAs at all. So how can it result in LBA Out
> of Range?

So what's the problem? If the request is to discard 0 blocks starting
from the last block, then that's valid. Is this patch actually fixing
anything?

Re: [PATCH v4 cxl-2.0-doe 3/3] PCIe standard header for DOE

[Jonathan Cameron]

On Wed, 31 Mar 2021 12:37:08 -0400
Chris Browy  wrote:

> From: hchkuo 
> 
> Signed-off-by: hchkuo 
Code must build after each patch, so this needs to go first in the series,
not last.  git rebase -i HEAD~3 and reorder the patches should be an easy
way to do it.

+ add a note to say standard-headers at least should come via scripts
(break that one out to a separate patch to make life easier)

Jonathan

> ---
>  include/hw/pci/pci_ids.h  | 2 ++
>  include/hw/pci/pcie_regs.h| 3 +++
>  include/standard-headers/linux/pci_regs.h | 3 ++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index 95f92d9..471c915 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -157,6 +157,8 @@
>  
>  /* Vendors and devices.  Sort key: vendor first, device next. */
>  
> +#define PCI_VENDOR_ID_PCI_SIG0x0001
> +
>  #define PCI_VENDOR_ID_LSI_LOGIC  0x1000
>  #define PCI_DEVICE_ID_LSI_53C810 0x0001
>  #define PCI_DEVICE_ID_LSI_53C895A0x0012
> diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
> index 1db86b0..5ec7014 100644
> --- a/include/hw/pci/pcie_regs.h
> +++ b/include/hw/pci/pcie_regs.h
> @@ -179,4 +179,7 @@ typedef enum PCIExpLinkWidth {
>  #define PCI_ACS_VER 0x1
>  #define PCI_ACS_SIZEOF  8
>  
> +/* DOE Capability Register Fields */
> +#define PCI_DOE_SIZEOF  24
> +
>  #endif /* QEMU_PCIE_REGS_H */
> diff --git a/include/standard-headers/linux/pci_regs.h 
> b/include/standard-headers/linux/pci_regs.h
> index e709ae8..2a8df63 100644
> --- a/include/standard-headers/linux/pci_regs.h
> +++ b/include/standard-headers/linux/pci_regs.h
> @@ -730,7 +730,8 @@
>  #define PCI_EXT_CAP_ID_DVSEC 0x23/* Designated Vendor-Specific */
>  #define PCI_EXT_CAP_ID_DLF   0x25/* Data Link Feature */
>  #define PCI_EXT_CAP_ID_PL_16GT   0x26/* Physical Layer 16.0 GT/s */
> -#define PCI_EXT_CAP_ID_MAX   PCI_EXT_CAP_ID_PL_16GT
> +#define PCI_EXT_CAP_ID_DOE   0x2E/* Data Object Exchange */
> +#define PCI_EXT_CAP_ID_MAX   PCI_EXT_CAP_ID_DOE
>  
>  #define PCI_EXT_CAP_DSN_SIZEOF   12
>  #define PCI_EXT_CAP_MCAST_ENDPOINT_SIZEOF 40

Re: [PATCH for-6.0 1/2] hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block

[Philippe Mathieu-Daudé]

On 4/9/21 5:05 PM, Peter Maydell wrote:
> The AN524 has three MPCs: one for the BRAM, one for the QSPI flash,
> and one for the DDR.  We incorrectly set the .mpc field in the
> RAMInfo struct for the SRAM block to 1, giving it the same MPC we are
> using for the QSPI.  The effect of this was that the QSPI didn't get
> mapped into the system address space at all, via an MPC or otherwise,
> and guest programs which tried to read from the QSPI would get a bus
> error.  Correct the SRAM RAMInfo to indicate that it does not have an
> associated MPC.
> 

Fixes: 25ff112a8cc ("hw/arm/mps2-tz: Add new mps3-an524 board")

> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/arm/mps2-tz.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH v4 cxl-2.0-doe 2/3] CXL Data Object Exchange implementation

[Jonathan Cameron]

On Wed, 31 Mar 2021 12:36:58 -0400
Chris Browy  wrote:

> From: hchkuo 

Again, must have a description, plus a sign off from Chris if Chris is
the person sending the patch out.

> 
> Signed-off-by: hchkuo 

Please split this into 2 patches.  Add the DOE + CDAT in first patch, and
compliance in the second.  That will make them easier to review.

Also break out the test data as another separate patch.

A few other things inline, particularly around multiple instances of
this device causing interesting issues given static non const data.


> ---
>  hw/cxl/cxl-cdat.c   | 220 +
>  hw/cxl/meson.build  |   1 +
>  hw/mem/cxl_type3.c  | 200 +++
>  include/hw/cxl/cxl_cdat.h   | 149 
>  include/hw/cxl/cxl_compliance.h | 297 
> 
>  include/hw/cxl/cxl_component.h  |   7 +
>  include/hw/cxl/cxl_device.h |   4 +
>  include/hw/cxl/cxl_pci.h|   2 +
>  tests/data/cdat/cdat.dat| Bin 0 -> 148 bytes
>  9 files changed, 880 insertions(+)
>  create mode 100644 hw/cxl/cxl-cdat.c
>  create mode 100644 include/hw/cxl/cxl_cdat.h
>  create mode 100644 include/hw/cxl/cxl_compliance.h
>  create mode 100644 tests/data/cdat/cdat.dat
> 
> diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> new file mode 100644
> index 000..fa54506
> --- /dev/null
> +++ b/hw/cxl/cxl-cdat.c
> @@ -0,0 +1,220 @@
> +/*
> + * CXL CDAT Structure
> + *
> + * Copyright (C) 2021 Avery Design Systems, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/pci/pci.h"
> +#include "hw/cxl/cxl.h"
> +#include "qapi/error.h"
> +
> +static struct cdat_dsmas dsmas = {
> +.header = {
> +.type = CDAT_TYPE_DSMAS,
> +.length = sizeof(dsmas),
> +},
> +.DSMADhandle = 0,
> +.flags = 0,
> +.DPA_base = 0,
> +.DPA_length = 0,

From a testing point of view would be helpful if you put some
plausible values in these :)

> +};
> +
> +static struct cdat_dslbis dslbis = {
> +.header = {
> +.type = CDAT_TYPE_DSLBIS,
> +.length = sizeof(dslbis),
> +},
> +.handle = 0,
> +.flags = 0,
> +.data_type = 0,
> +.entry_base_unit = 0,
> +};
> +
> +static struct cdat_dsmscis dsmscis = {
> +.header = {
> +.type = CDAT_TYPE_DSMSCIS,
> +.length = sizeof(dsmscis),
> +},
> +.DSMAS_handle = 0,
> +.memory_side_cache_size = 0,
> +.cache_attributes = 0,
> +};
> +
> +static struct cdat_dsis dsis = {
> +.header = {
> +.type = CDAT_TYPE_DSIS,
> +.length = sizeof(dsis),
> +},
> +.flags = 0,
> +.handle = 0,
> +};
> +
> +static struct cdat_dsemts dsemts = {
> +.header = {
> +.type = CDAT_TYPE_DSEMTS,
> +.length = sizeof(dsemts),
> +},
> +.DSMAS_handle = 0,
> +.EFI_memory_type_attr = 0,
> +.DPA_offset = 0,
> +.DPA_length = 0,
> +};
> +
> +struct cdat_sslbis {
> +struct cdat_sslbis_header sslbis_header;
> +struct cdat_sslbe sslbe[];
> +};
> +
> +static struct cdat_sslbis sslbis = {
> +.sslbis_header = {
> +.header = {
> +.type = CDAT_TYPE_SSLBIS,
> +.length = sizeof(sslbis.sslbis_header) +
> +  sizeof(struct cdat_sslbe) * 2,
> +},
> +.data_type = 0,
> +.entry_base_unit = 0,
> +},
> +.sslbe[0] = {
> +.port_x_id = 0,
> +.port_y_id = 0,
> +.latency_bandwidth = 0,
> +},
> +.sslbe[1] = {
> +.port_x_id = 0,
> +.port_y_id = 0,
> +.latency_bandwidth = 0,
> +},
> +};
> +
> +static void *cdat_table[] = {
> +(void *) ,

You should never need to cast to a (void *) as the c spec never requires
it.

> +(void *) ,
> +(void *) ,
> +(void *) ,
> +(void *) ,
> +(void *) ,
> +};

If I instantiate two CXL mem devices, They will currently share this
table.  That is definitely not a good idea as an architecture as they may
well have different parameters.  You need to ensure each instance has it's
own copy.  Or if you want them to just be constant values and not handle
that complexity, then make them const and ensure the code is happy to
work with that.

> +
> +static void cdat_len_check(struct cdat_sub_header *hdr, Error **errp)
> +{
> +assert(hdr->length);
> +assert(hdr->reserved == 0);
> +
> +switch (hdr->type) {
> +case CDAT_TYPE_DSMAS:
> +assert(hdr->length == sizeof(struct cdat_dsmas));
> +break;
> +case CDAT_TYPE_DSLBIS:
> +assert(hdr->length == sizeof(struct cdat_dslbis));
> +break;
> +case CDAT_TYPE_DSMSCIS:
> +assert(hdr->length == sizeof(struct cdat_dsmscis));
> +break;
> +case CDAT_TYPE_DSIS:
> +assert(hdr->length == sizeof(struct cdat_dsis));
> +break;
> +

Re: [PATCH] virtiofsd: Fix side-effect in assert()

[Philippe Mathieu-Daudé]

Hi Stefan,

On 4/9/21 5:11 PM, Stefan Weil wrote:
> Am 09.04.21 um 12:06 schrieb Greg Kurz:
> 
>> It is bad practice to put an expression with a side-effect in
>> assert() because the side-effect won't happen if the code is
>> compiled with -DNDEBUG.
>>
>> Use an intermediate variable. Consolidate this in an macro to
>> have proper line numbers when the assertion is hit.
>>
>> virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr:
>>   Assertion `fchdir_res == 0' failed.
>> Aborted
>>
>>    2796  /* fchdir should not fail here */
>> =>2797  FCHDIR_NOFAIL(lo->proc_self_fd);
>>    2798  ret = getxattr(procname, name, value, size);
>>    2799  FCHDIR_NOFAIL(lo->root.fd);
>>
>> Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations")
>> Cc: misono.tomoh...@jp.fujitsu.com
>> Signed-off-by: Greg Kurz 
>> ---
>>   tools/virtiofsd/passthrough_ll.c | 21 +
>>   1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c
>> b/tools/virtiofsd/passthrough_ll.c
>> index 1553d2ef454f..6592f96f685e 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data 
> *lo, const char *server_name,
>>   return -ENODATA;
>>   }
>>   +#define FCHDIR_NOFAIL(fd) do { \
>> +    int fchdir_res = fchdir(fd);   \
>> +    assert(fchdir_res == 0);   \
>> +    } while (0)
>> +
>>   static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char
>> *in_name,
>>   
> 
> 
> I am afraid that this will raise a compiler warning (or error with
> -Werror) when NDEBUG is defined because the variable is unused in that
> case ([-Wunused-variable]).
> 
> I suggest to use different implementations of the macro depending on
> NDEBUG.

QEMU doesn't build with NDEBUG, see commit 262a69f4282
("osdep.h: Prohibit disabling assert() in supported builds").

Regards,

Phil.

Re: [PATCH for-6.0 0/2] mps3-an524: Fix MPC setting for SRAM block

[Richard Henderson]

On 4/9/21 8:05 AM, Peter Maydell wrote:

Peter Maydell (2):
   hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block
   hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC


Reviewed-by: Richard Henderson 

r~

Re: [PATCH] virtiofsd: Fix side-effect in assert()

[Stefan Weil]

Am 09.04.21 um 12:06 schrieb Greg Kurz:


It is bad practice to put an expression with a side-effect in
assert() because the side-effect won't happen if the code is
compiled with -DNDEBUG.

Use an intermediate variable. Consolidate this in an macro to
have proper line numbers when the assertion is hit.

virtiofsd: ../../tools/virtiofsd/passthrough_ll.c:2797: lo_getxattr:
  Assertion `fchdir_res == 0' failed.
Aborted

   2796  /* fchdir should not fail here */
=>2797  FCHDIR_NOFAIL(lo->proc_self_fd);
   2798  ret = getxattr(procname, name, value, size);
   2799  FCHDIR_NOFAIL(lo->root.fd);

Fixes: bdfd66788349 ("virtiofsd: Fix xattr operations")
Cc: misono.tomoh...@jp.fujitsu.com
Signed-off-by: Greg Kurz 
---
  tools/virtiofsd/passthrough_ll.c | 21 +
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 1553d2ef454f..6592f96f685e 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -2723,6 +2723,11 @@ static int xattr_map_server(const struct lo_data 

*lo, const char *server_name,

  return -ENODATA;
  }
  
+#define FCHDIR_NOFAIL(fd) do { \

+int fchdir_res = fchdir(fd);   \
+assert(fchdir_res == 0);   \
+} while (0)
+
  static void lo_getxattr(fuse_req_t req, fuse_ino_t ino, const char *in_name,
  



I am afraid that this will raise a compiler warning (or error with 
-Werror) when NDEBUG is defined because the variable is unused in that 
case ([-Wunused-variable]).


I suggest to use different implementations of the macro depending on NDEBUG.

Stefan

[Bug 1893040] Re: External modules retreval using Go1.15 on s390x appears to have checksum and ECDSA verification issues

[Alex Bennée]

** Tags added: s390x

** Tags added: tcg

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1893040

Title:
   External modules retreval using Go1.15 on s390x appears to have
  checksum and ECDSA verification issues

Status in QEMU:
  New

Bug description:
  We are observing issue while building go-runner image and we suspect it is 
due to QEMU version being used. As referred in below issue:
  https://github.com/golang/go/issues/40949

  We tried to build go-runner image using go1.15 and register QEMU
  (docker run --rm --privileged multiarch/qemu-user-
  static@sha256:c772ee1965aa0be9915ee1b018a0dd92ea361b4fa1bcab5bbc033517749b2af4
  --reset -p yes) as mentioned in PR
  https://github.com/kubernetes/release/pull/1499. We observed below
  failure during build:

  
-
  ERROR: executor failed running [/bin/sh -c CGO_ENABLED=0 GOOS=linux 
GOARCH=${ARCH} go build -ldflags '-s -w -buildid= -extldflags "-static"'
 -o go-runner ${package}]: buildkit-runc did not terminate successfully
  --
   > [builder 7/7] RUN CGO_ENABLED=0 GOOS=linux GOARCH=${ARCH} go build 
-ldflags '-s -w -buildid= -extldflags "-static"' -o go-runner .:
  --
  failed to solve: rpc error: code = Unknown desc = executor failed running 
[/bin/sh -c CGO_ENABLED=0 GOOS=linux GOARCH=${ARCH} go build -ldflags '-s 
-w -buildid= -extldflags "-static"' -o go-runner ${package}]: buildkit-runc 
did not terminate successfully
  Makefile:52: recipe for target 'container' failed
  make: *** [container] Error 1
  
-

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1893040/+subscriptions

Re: [PATCH] checkpatch: Fix use of uninitialized value

[Alex Bennée]

Greg Kurz  writes:

> checkfilename() doesn't always set $acpi_testexpected. Fix the following
> warning:
>
> Use of uninitialized value $acpi_testexpected in string eq at
>  ./scripts/checkpatch.pl line 1529.
>
> Fixes: d2f1af0e4120 ("checkpatch: don't emit warning on newly created acpi 
> data files")
> Cc: isaku.yamah...@intel.com
> Signed-off-by: Greg Kurz 

jinx ;-)
 
  Subject: [RFC PATCH] scripts/checkpatch: fix uninitialised value check
  Date: Thu,  8 Apr 2021 17:46:10 +0100
  Message-Id: <20210408164610.14229-1-alex.ben...@linaro.org>

but as I failed to check the list first have a:

Reviewed-by: Alex Bennée 

> ---
>  scripts/checkpatch.pl |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 8f7053ec9b26..3d185cceac94 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1532,6 +1532,7 @@ sub process {
>($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ 
> &&
> (defined($1) || defined($2 &&
>!(($realfile ne '') &&
> +defined($acpi_testexpected) &&
>  ($realfile eq $acpi_testexpected))) {
>   $reported_maintainer_file = 1;
>   WARN("added, moved or deleted file(s), does MAINTAINERS 
> need updating?\n" . $herecurr);


-- 
Alex Bennée

[PATCH for-6.0 1/2] hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block

[Peter Maydell]

The AN524 has three MPCs: one for the BRAM, one for the QSPI flash,
and one for the DDR.  We incorrectly set the .mpc field in the
RAMInfo struct for the SRAM block to 1, giving it the same MPC we are
using for the QSPI.  The effect of this was that the QSPI didn't get
mapped into the system address space at all, via an MPC or otherwise,
and guest programs which tried to read from the QSPI would get a bus
error.  Correct the SRAM RAMInfo to indicate that it does not have an
associated MPC.

Signed-off-by: Peter Maydell 
---
 hw/arm/mps2-tz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 3fbe3d29f95..5ebd671bf83 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -238,7 +238,7 @@ static const RAMInfo an524_raminfo[] = { {
 .name = "sram",
 .base = 0x2000,
 .size = 32 * 4 * KiB,
-.mpc = 1,
+.mpc = -1,
 .mrindex = 1,
 }, {
 /* We don't model QSPI flash yet; for now expose it as simple ROM */
-- 
2.20.1

[PATCH for-6.0 2/2] hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC

[Peter Maydell]

Each board in mps2-tz.c specifies a RAMInfo[] array providing
information about each RAM in the board.  The .mpc field of the
RAMInfo struct specifies which MPC, if any, the RAM is attached to.
We already assert if the array doesn't have any entry for an MPC, but
we don't diagnose the error of using the same MPC number twice (which
is quite easy to do by accident if copy-and-pasting structure
entries).

Enhance find_raminfo_for_mpc() so that it detects multiple entries
for the MPC as well as missing entries.

Signed-off-by: Peter Maydell 
---
 hw/arm/mps2-tz.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 5ebd671bf83..25016e464d9 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -306,14 +306,18 @@ static const RAMInfo 
*find_raminfo_for_mpc(MPS2TZMachineState *mms, int mpc)
 {
 MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_GET_CLASS(mms);
 const RAMInfo *p;
+const RAMInfo *found = NULL;
 
 for (p = mmc->raminfo; p->name; p++) {
 if (p->mpc == mpc && !(p->flags & IS_ALIAS)) {
-return p;
+/* There should only be one entry in the array for this MPC */
+g_assert(!found);
+found = p;
 }
 }
 /* if raminfo array doesn't have an entry for each MPC this is a bug */
-g_assert_not_reached();
+assert(found);
+return found;
 }
 
 static MemoryRegion *mr_for_raminfo(MPS2TZMachineState *mms,
-- 
2.20.1

[PATCH for-6.0 0/2] mps3-an524: Fix MPC setting for SRAM block

[Peter Maydell]

The AN524 FPGA image has three MPCs: one for the BRAM, one for
the QSPI flash, and one for the DDR. In the an524_raminfo[] array
that defines the various RAM blocks on the board, we incorrectly
set the .mpc field for the SRAM to 1 as well as for the QSPI flash.
The effect of this was to cause the QSPI flash not to be mapped
at all (because when we mapped the 'upstream' end of each MPC,
we found the incorrectly marked SRAM entry before the QSPI one
when scanning through the raminfo array, and so put the upstream
end of MPC1 at the SRAM address).

Patch 1 fixes the SRAM block to use '.mpc = -1' indicating that
there is no associated MPC. Patch 2 adds an assert() that would
have caught this programming error (which is quite easy to make
if you're constructing the raminfo array for a new board by
copying and modifying entries from existing boards).

I think this makes sense to put into 6.0, it's a pretty safe change.

Peter Maydell (2):
  hw/arm/mps2-tz: Fix MPC setting for AN524 SRAM block
  hw/arm/mps2-tz: Assert if more than one RAM is attached to an MPC

 hw/arm/mps2-tz.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.20.1