date:20200303

From: Bin Meng 

At present the board serial number is hard-coded to 1, and passed
to OTP model during initialization. Firmware (FSBL, U-Boot) uses
the serial number to generate a unique MAC address for the on-chip
ethernet controller. When multiple QEMU 'sifive_u' instances are
created and connected to the same subnet, they all have the same
MAC address hence it creates a unusable network.

A new "serial" property is introduced to specify the board serial
number. When not given, the default serial number 1 is used.

Signed-off-by: Bin Meng 
Reviewed-by: Palmer Dabbelt 
Reviewed-by: Alistair Francis 
Message-Id: <1573916930-19068-1-git-send-email-bmeng...@gmail.com>
[ Changed by AF:
 - Use the SoC's serial property to pass the info to the SoC
 - Fixup commit title
 - Rebase on file restructuring
]
Signed-off-by: Alistair Francis 
---
 hw/riscv/sifive_u.c | 20 
 include/hw/riscv/sifive_u.h |  1 +
 2 files changed, 21 insertions(+)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index e52f9d0bd4..f3f67cc0e3 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -34,6 +34,7 @@
 #include "qemu/log.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
+#include "qapi/visitor.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "hw/sysbus.h"
@@ -322,6 +323,8 @@ static void riscv_sifive_u_init(MachineState *machine)
 object_initialize_child(OBJECT(machine), "soc", >soc,
 sizeof(s->soc), TYPE_RISCV_U_SOC,
 _abort, NULL);
+object_property_set_uint(OBJECT(>soc), s->serial, "serial",
+_abort);
 object_property_set_bool(OBJECT(>soc), true, "realized",
 _abort);
 
@@ -413,6 +416,18 @@ static void sifive_u_set_start_in_flash(Object *obj, bool 
value, Error **errp)
 s->start_in_flash = value;
 }
 
+static void sifive_u_get_serial(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+visit_type_uint32(v, name, (uint32_t *)opaque, errp);
+}
+
+static void sifive_u_set_serial(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+visit_type_uint32(v, name, (uint32_t *)opaque, errp);
+}
+
 static void riscv_sifive_u_machine_instance_init(Object *obj)
 {
 SiFiveUState *s = RISCV_U_MACHINE(obj);
@@ -424,6 +439,11 @@ static void riscv_sifive_u_machine_instance_init(Object 
*obj)
 "Set on to tell QEMU's ROM to jump to " \
 "flash. Otherwise QEMU will jump to DRAM",
 NULL);
+
+s->serial = OTP_SERIAL;
+object_property_add(obj, "serial", "uint32", sifive_u_get_serial,
+sifive_u_set_serial, NULL, >serial, NULL);
+object_property_set_description(obj, "serial", "Board serial number", 
NULL);
 }
 
 
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index a2baa1de5f..16c297ec5f 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -61,6 +61,7 @@ typedef struct SiFiveUState {
 int fdt_size;
 
 bool start_in_flash;
+uint32_t serial;
 } SiFiveUState;
 
 enum {
-- 
2.25.1

[PATCH v1 2/3] riscv/sifive_u: Add a serial property to the sifive_u SoC

At present the board serial number is hard-coded to 1, and passed
to OTP model during initialization. Firmware (FSBL, U-Boot) uses
the serial number to generate a unique MAC address for the on-chip
ethernet controller. When multiple QEMU 'sifive_u' instances are
created and connected to the same subnet, they all have the same
MAC address hence it creates a unusable network.

A new "serial" property is introduced to the sifive_u SoC to specify
the board serial number. When not given, the default serial number
1 is used.

Suggested-by: Bin Meng 
Signed-off-by: Alistair Francis 
---
 hw/riscv/sifive_u.c | 8 +++-
 include/hw/riscv/sifive_u.h | 2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 9a0145b5b4..e52f9d0bd4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -488,7 +488,7 @@ static void riscv_sifive_u_soc_init(Object *obj)
   TYPE_SIFIVE_U_PRCI);
 sysbus_init_child_obj(obj, "otp", >otp, sizeof(s->otp),
   TYPE_SIFIVE_U_OTP);
-qdev_prop_set_uint32(DEVICE(>otp), "serial", OTP_SERIAL);
+qdev_prop_set_uint32(DEVICE(>otp), "serial", s->serial);
 sysbus_init_child_obj(obj, "gem", >gem, sizeof(s->gem),
   TYPE_CADENCE_GEM);
 }
@@ -607,10 +607,16 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
Error **errp)
 memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
 }
 
+static Property riscv_sifive_u_soc_props[] = {
+DEFINE_PROP_UINT32("serial", SiFiveUSoCState, serial, OTP_SERIAL),
+DEFINE_PROP_END_OF_LIST()
+};
+
 static void riscv_sifive_u_soc_class_init(ObjectClass *oc, void *data)
 {
 DeviceClass *dc = DEVICE_CLASS(oc);
 
+device_class_set_props(dc, riscv_sifive_u_soc_props);
 dc->realize = riscv_sifive_u_soc_realize;
 /* Reason: Uses serial_hds in realize function, thus can't be used twice */
 dc->user_creatable = false;
diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
index 82667b5746..a2baa1de5f 100644
--- a/include/hw/riscv/sifive_u.h
+++ b/include/hw/riscv/sifive_u.h
@@ -42,6 +42,8 @@ typedef struct SiFiveUSoCState {
 SiFiveUPRCIState prci;
 SiFiveUOTPState otp;
 CadenceGEMState gem;
+
+uint32_t serial;
 } SiFiveUSoCState;
 
 #define TYPE_RISCV_U_MACHINE MACHINE_TYPE_NAME("sifive_u")
-- 
2.25.1

[PATCH v1 1/3] riscv/sifive_u: Fix up file ordering

Split the file into clear machine and SoC sections.

Signed-off-by: Alistair Francis 
---
 hw/riscv/sifive_u.c | 107 ++--
 1 file changed, 54 insertions(+), 53 deletions(-)

diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
index 156a003642..9a0145b5b4 100644
--- a/hw/riscv/sifive_u.c
+++ b/hw/riscv/sifive_u.c
@@ -399,6 +399,60 @@ static void riscv_sifive_u_init(MachineState *machine)
   _space_memory);
 }
 
+static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
+{
+SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+return s->start_in_flash;
+}
+
+static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp)
+{
+SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+s->start_in_flash = value;
+}
+
+static void riscv_sifive_u_machine_instance_init(Object *obj)
+{
+SiFiveUState *s = RISCV_U_MACHINE(obj);
+
+s->start_in_flash = false;
+object_property_add_bool(obj, "start-in-flash", 
sifive_u_get_start_in_flash,
+ sifive_u_set_start_in_flash, NULL);
+object_property_set_description(obj, "start-in-flash",
+"Set on to tell QEMU's ROM to jump to " \
+"flash. Otherwise QEMU will jump to DRAM",
+NULL);
+}
+
+
+static void riscv_sifive_u_machine_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
+mc->desc = "RISC-V Board compatible with SiFive U SDK";
+mc->init = riscv_sifive_u_init;
+mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
+mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
+mc->default_cpus = mc->min_cpus;
+}
+
+static const TypeInfo riscv_sifive_u_machine_typeinfo = {
+.name   = MACHINE_TYPE_NAME("sifive_u"),
+.parent = TYPE_MACHINE,
+.class_init = riscv_sifive_u_machine_class_init,
+.instance_init = riscv_sifive_u_machine_instance_init,
+.instance_size = sizeof(SiFiveUState),
+};
+
+static void riscv_sifive_u_machine_init_register_types(void)
+{
+type_register_static(_sifive_u_machine_typeinfo);
+}
+
+type_init(riscv_sifive_u_machine_init_register_types)
+
 static void riscv_sifive_u_soc_init(Object *obj)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
@@ -439,33 +493,6 @@ static void riscv_sifive_u_soc_init(Object *obj)
   TYPE_CADENCE_GEM);
 }
 
-static bool sifive_u_get_start_in_flash(Object *obj, Error **errp)
-{
-SiFiveUState *s = RISCV_U_MACHINE(obj);
-
-return s->start_in_flash;
-}
-
-static void sifive_u_set_start_in_flash(Object *obj, bool value, Error **errp)
-{
-SiFiveUState *s = RISCV_U_MACHINE(obj);
-
-s->start_in_flash = value;
-}
-
-static void riscv_sifive_u_machine_instance_init(Object *obj)
-{
-SiFiveUState *s = RISCV_U_MACHINE(obj);
-
-s->start_in_flash = false;
-object_property_add_bool(obj, "start-in-flash", 
sifive_u_get_start_in_flash,
- sifive_u_set_start_in_flash, NULL);
-object_property_set_description(obj, "start-in-flash",
-"Set on to tell QEMU's ROM to jump to " \
-"flash. Otherwise QEMU will jump to DRAM",
-NULL);
-}
-
 static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp)
 {
 MachineState *ms = MACHINE(qdev_get_machine());
@@ -603,29 +630,3 @@ static void riscv_sifive_u_soc_register_types(void)
 }
 
 type_init(riscv_sifive_u_soc_register_types)
-
-static void riscv_sifive_u_machine_class_init(ObjectClass *oc, void *data)
-{
-MachineClass *mc = MACHINE_CLASS(oc);
-
-mc->desc = "RISC-V Board compatible with SiFive U SDK";
-mc->init = riscv_sifive_u_init;
-mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + SIFIVE_U_COMPUTE_CPU_COUNT;
-mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
-mc->default_cpus = mc->min_cpus;
-}
-
-static const TypeInfo riscv_sifive_u_machine_typeinfo = {
-.name   = MACHINE_TYPE_NAME("sifive_u"),
-.parent = TYPE_MACHINE,
-.class_init = riscv_sifive_u_machine_class_init,
-.instance_init = riscv_sifive_u_machine_instance_init,
-.instance_size = sizeof(SiFiveUState),
-};
-
-static void riscv_sifive_u_machine_init_register_types(void)
-{
-type_register_static(_sifive_u_machine_typeinfo);
-}
-
-type_init(riscv_sifive_u_machine_init_register_types)
-- 
2.25.1

[PATCH v1 0/3] hw/riscv: Add a serial property to the sifive_u machine

At present the board serial number is hard-coded to 1, and passed
to OTP model during initialization. Firmware (FSBL, U-Boot) uses
the serial number to generate a unique MAC address for the on-chip
ethernet controller. When multiple QEMU 'sifive_u' instances are
created and connected to the same subnet, they all have the same
MAC address hence it creates a unusable network.

A new "serial" property is introduced to specify the board serial
number. When not given, the default serial number 1 is used.

Alistair Francis (2):
  riscv/sifive_u: Fix up file ordering
  riscv/sifive_u: Add a serial property to the sifive_u SoC

Bin Meng (1):
  riscv/sifive_u: Add a serial property to the sifive_u machine

 hw/riscv/sifive_u.c | 135 +---
 include/hw/riscv/sifive_u.h |   3 +
 2 files changed, 84 insertions(+), 54 deletions(-)

-- 
2.25.1

[PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs

The RISC-V spec specifies that when a write happens and the D bit is
clear the implementation will set the bit in the PTE. It does not
describe that the PTE being dirty means that we should provide write
access. This patch removes the write access granted to pages when the
dirty bit is set.

Following the prot variable we can see that it affects all of these
functions:
 riscv_cpu_tlb_fill()
   tlb_set_page()
 tlb_set_page_with_attrs()
   address_space_translate_for_iotlb()

Looking at the cputlb code (tlb_set_page_with_attrs() and
address_space_translate_for_iotlb()) it looks like the main affect of
setting write permissions is that the page can be marked as TLB_NOTDIRTY.

I don't see any other impacts (related to the dirty bit) for giving a
page write permissions.

Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be because it
ends up with write permission in the second stage translation in cases
where we aren't doing a store.

Signed-off-by: Alistair Francis 
Reviewed-by: Bin Meng 
---
 target/riscv/cpu_helper.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 5ea5d133aa..cc9f20b471 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -572,10 +572,8 @@ restart:
 if ((pte & PTE_X)) {
 *prot |= PAGE_EXEC;
 }
-/* add write permission on stores or if the page is already dirty,
-   so that we TLB miss on later writes to update the dirty bit */
-if ((pte & PTE_W) &&
-(access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+/* add write permission on stores */
+if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
 *prot |= PAGE_WRITE;
 }
 return TRANSLATE_SUCCESS;
-- 
2.25.1

[PATCH 2/2] misc: Replace zero-length arrays with flexible array member (manual)

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following command (then manual analysis):

  git grep -F '[0];'

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Signed-off-by: Philippe Mathieu-Daudé 
---
 docs/interop/vhost-user.rst   | 4 ++--
 block/qed.h   | 2 +-
 include/hw/acpi/acpi-defs.h   | 4 ++--
 include/hw/boards.h   | 2 +-
 include/hw/s390x/event-facility.h | 2 +-
 include/hw/s390x/sclp.h   | 8 
 block/vmdk.c  | 2 +-
 hw/char/sclpconsole-lm.c  | 2 +-
 hw/char/sclpconsole.c | 2 +-
 hw/s390x/virtio-ccw.c | 2 +-
 target/s390x/ioinst.c | 2 +-
 11 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 401652397c..3b1b6602c7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -568,7 +568,7 @@ For split virtqueue, queue region can be implemented as:
   uint16_t used_idx;
 
   /* Used to track the state of each descriptor in descriptor table */
-  DescStateSplit desc[0];
+  DescStateSplit desc[];
   } QueueRegionSplit;
 
 To track inflight I/O, the queue region should be processed as follows:
@@ -690,7 +690,7 @@ For packed virtqueue, queue region can be implemented as:
   uint8_t padding[7];
 
   /* Used to track the state of each descriptor fetched from descriptor 
ring */
-  DescStatePacked desc[0];
+  DescStatePacked desc[];
   } QueueRegionPacked;
 
 To track inflight I/O, the queue region should be processed as follows:
diff --git a/block/qed.h b/block/qed.h
index 42c115d822..87428ba00e 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -103,7 +103,7 @@ typedef struct {
 } QEMU_PACKED QEDHeader;
 
 typedef struct {
-uint64_t offsets[0];/* in bytes */
+uint64_t offsets[]; /* in bytes */
 } QEDTable;
 
 /* The L2 cache is a simple write-through cache for L2 structures */
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 19f7ba7b70..c13327fa78 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -152,7 +152,7 @@ typedef struct AcpiSerialPortConsoleRedirection
  */
 struct AcpiRsdtDescriptorRev1 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint32_t table_offset_entry[0];  /* Array of pointers to other */
+uint32_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
@@ -162,7 +162,7 @@ typedef struct AcpiRsdtDescriptorRev1 
AcpiRsdtDescriptorRev1;
  */
 struct AcpiXsdtDescriptorRev2 {
 ACPI_TABLE_HEADER_DEF   /* ACPI common table header */
-uint64_t table_offset_entry[0];  /* Array of pointers to other */
+uint64_t table_offset_entry[];  /* Array of pointers to other */
 /* ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiXsdtDescriptorRev2 AcpiXsdtDescriptorRev2;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9bc42dfb22..c96120d15f 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -71,7 +71,7 @@ typedef struct CPUArchId {
  */
 typedef struct {
 int len;
-CPUArchId cpus[0];
+CPUArchId cpus[];
 } CPUArchIdList;
 
 /**
diff --git a/include/hw/s390x/event-facility.h 
b/include/hw/s390x/event-facility.h
index bdc32a3c09..700a610f33 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -122,7 +122,7 @@ typedef struct MDBO {
 
 typedef struct MDB {
 MdbHeader header;
-MDBO mdbo[0];
+MDBO mdbo[];
 } QEMU_PACKED MDB;
 
 typedef struct SclpMsg {
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b78c..cd7b24359f 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -132,7 +132,7 @@ typedef struct ReadInfo {
 uint16_t highest_cpu;
 uint8_t  _reserved5[124

Re: [PATCH] hw/ide: Remove status register read side effect

2020-03-03 Thread jasper.lowell

I'm happy to do this. It will take a while for me to collect the
results though. I'll chime in once I have the results.

> That should give the ultimate answer to our guessing.
Agreed.

Thanks,
Jasper Lowell.

On Thu, 2020-02-27 at 12:38 +0100, BALATON Zoltan wrote:
> On Thu, 27 Feb 2020, jasper.low...@bt.com wrote:
> > I've since looked at a Ultra 5 board and can confirm that it
> > shipped
> > with a CMD646U chip like the Ultra 10.
> 
> If you have access to an Ultra 5 maybe you could try testing what it
> does 
> with irqs. That should give the ultimate answer to our guessing. It
> may 
> need patching a Linux driver to log more info and recompile the
> kernel so 
> not sure you have time for that but maybe it would help if you can do
> it.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH] hw/ide: Remove status register read side effect

2020-03-03 Thread jasper.lowell

I haven't.

I did a pull this morning on master and everything seems to be working
again. The problem was likely the same.

Thanks,
Jasper Lowell.

On Thu, 2020-02-27 at 12:35 +0100, BALATON Zoltan wrote:
> On Thu, 27 Feb 2020, jasper.low...@bt.com wrote:
> > > I'll submit a RFC V2 patch with a proposed fix.
> > 
> > This will have to wait.
> > 
> > Recent commits have caused Solaris 10 to error out of booting much
> > earlier than previously.
> 
> Can you bisect which commit broke it? Is it the same that caused
> slowness 
> for arm and ppc? For that one there are patches that should fix it on
> the 
> list.
> 
> Regards,
> BALATON Zoltan
>

[PATCH v4 3/3] linux-user/riscv: Update the syscall_nr's to the 5.5 kernel

Signed-off-by: Alistair Francis 
---
 linux-user/riscv/syscall32_nr.h | 295 +++
 linux-user/riscv/syscall64_nr.h | 301 
 linux-user/riscv/syscall_nr.h   | 294 +--
 3 files changed, 598 insertions(+), 292 deletions(-)
 create mode 100644 linux-user/riscv/syscall32_nr.h
 create mode 100644 linux-user/riscv/syscall64_nr.h

diff --git a/linux-user/riscv/syscall32_nr.h b/linux-user/riscv/syscall32_nr.h
new file mode 100644
index 00..4fef73e954
--- /dev/null
+++ b/linux-user/riscv/syscall32_nr.h
@@ -0,0 +1,295 @@
+/*
+ * This file contains the system call numbers.
+ */
+#ifndef LINUX_USER_RISCV_SYSCALL32_NR_H
+#define LINUX_USER_RISCV_SYSCALL32_NR_H
+
+#define TARGET_NR_io_setup 0
+#define TARGET_NR_io_destroy 1
+#define TARGET_NR_io_submit 2
+#define TARGET_NR_io_cancel 3
+#define TARGET_NR_setxattr 5
+#define TARGET_NR_lsetxattr 6
+#define TARGET_NR_fsetxattr 7
+#define TARGET_NR_getxattr 8
+#define TARGET_NR_lgetxattr 9
+#define TARGET_NR_fgetxattr 10
+#define TARGET_NR_listxattr 11
+#define TARGET_NR_llistxattr 12
+#define TARGET_NR_flistxattr 13
+#define TARGET_NR_removexattr 14
+#define TARGET_NR_lremovexattr 15
+#define TARGET_NR_fremovexattr 16
+#define TARGET_NR_getcwd 17
+#define TARGET_NR_lookup_dcookie 18
+#define TARGET_NR_eventfd2 19
+#define TARGET_NR_epoll_create1 20
+#define TARGET_NR_epoll_ctl 21
+#define TARGET_NR_epoll_pwait 22
+#define TARGET_NR_dup 23
+#define TARGET_NR_dup3 24
+#define TARGET_NR_fcntl64 25
+#define TARGET_NR_inotify_init1 26
+#define TARGET_NR_inotify_add_watch 27
+#define TARGET_NR_inotify_rm_watch 28
+#define TARGET_NR_ioctl 29
+#define TARGET_NR_ioprio_set 30
+#define TARGET_NR_ioprio_get 31
+#define TARGET_NR_flock 32
+#define TARGET_NR_mknodat 33
+#define TARGET_NR_mkdirat 34
+#define TARGET_NR_unlinkat 35
+#define TARGET_NR_symlinkat 36
+#define TARGET_NR_linkat 37
+#define TARGET_NR_umount2 39
+#define TARGET_NR_mount 40
+#define TARGET_NR_pivot_root 41
+#define TARGET_NR_nfsservctl 42
+#define TARGET_NR_statfs64 43
+#define TARGET_NR_fstatfs64 44
+#define TARGET_NR_truncate64 45
+#define TARGET_NR_ftruncate64 46
+#define TARGET_NR_fallocate 47
+#define TARGET_NR_faccessat 48
+#define TARGET_NR_chdir 49
+#define TARGET_NR_fchdir 50
+#define TARGET_NR_chroot 51
+#define TARGET_NR_fchmod 52
+#define TARGET_NR_fchmodat 53
+#define TARGET_NR_fchownat 54
+#define TARGET_NR_fchown 55
+#define TARGET_NR_openat 56
+#define TARGET_NR_close 57
+#define TARGET_NR_vhangup 58
+#define TARGET_NR_pipe2 59
+#define TARGET_NR_quotactl 60
+#define TARGET_NR_getdents64 61
+#define TARGET_NR_llseek 62
+#define TARGET_NR_read 63
+#define TARGET_NR_write 64
+#define TARGET_NR_readv 65
+#define TARGET_NR_writev 66
+#define TARGET_NR_pread64 67
+#define TARGET_NR_pwrite64 68
+#define TARGET_NR_preadv 69
+#define TARGET_NR_pwritev 70
+#define TARGET_NR_sendfile64 71
+#define TARGET_NR_signalfd4 74
+#define TARGET_NR_vmsplice 75
+#define TARGET_NR_splice 76
+#define TARGET_NR_tee 77
+#define TARGET_NR_readlinkat 78
+#define TARGET_NR_fstatat64 79
+#define TARGET_NR_fstat64 80
+#define TARGET_NR_sync 81
+#define TARGET_NR_fsync 82
+#define TARGET_NR_fdatasync 83
+#define TARGET_NR_sync_file_range 84
+#define TARGET_NR_timerfd_create 85
+#define TARGET_NR_acct 89
+#define TARGET_NR_capget 90
+#define TARGET_NR_capset 91
+#define TARGET_NR_personality 92
+#define TARGET_NR_exit 93
+#define TARGET_NR_exit_group 94
+#define TARGET_NR_waitid 95
+#define TARGET_NR_set_tid_address 96
+#define TARGET_NR_unshare 97
+#define TARGET_NR_set_robust_list 99
+#define TARGET_NR_get_robust_list 100
+#define TARGET_NR_getitimer 102
+#define TARGET_NR_setitimer 103
+#define TARGET_NR_kexec_load 104
+#define TARGET_NR_init_module 105
+#define TARGET_NR_delete_module 106
+#define TARGET_NR_timer_create 107
+#define TARGET_NR_timer_getoverrun 109
+#define TARGET_NR_timer_delete 111
+#define TARGET_NR_syslog 116
+#define TARGET_NR_ptrace 117
+#define TARGET_NR_sched_setparam 118
+#define TARGET_NR_sched_setscheduler 119
+#define TARGET_NR_sched_getscheduler 120
+#define TARGET_NR_sched_getparam 121
+#define TARGET_NR_sched_setaffinity 122
+#define TARGET_NR_sched_getaffinity 123
+#define TARGET_NR_sched_yield 124
+#define TARGET_NR_sched_get_priority_max 125
+#define TARGET_NR_sched_get_priority_min 126
+#define TARGET_NR_restart_syscall 128
+#define TARGET_NR_kill 129
+#define TARGET_NR_tkill 130
+#define TARGET_NR_tgkill 131
+#define TARGET_NR_sigaltstack 132
+#define TARGET_NR_rt_sigsuspend 133
+#define TARGET_NR_rt_sigaction 134
+#define TARGET_NR_rt_sigprocmask 135
+#define TARGET_NR_rt_sigpending 136
+#define TARGET_NR_rt_sigqueueinfo 138
+#define TARGET_NR_rt_sigreturn 139
+#define TARGET_NR_setpriority 140
+#define TARGET_NR_getpriority 141
+#define TARGET_NR_reboot 142
+#define TARGET_NR_setregid 143
+#define TARGET_NR_setgid 144
+#define TARGET_NR_setreuid 145
+#define TARGET_NR_setuid 146
+#define

[PATCH v4 1/3] linux-user: Protect more syscalls

New y2038 safe 32-bit architectures (like RISC-V) don't support old
syscalls with a 32-bit time_t. The kernel defines new *_time64 versions
of these syscalls. Add some more #ifdefs to syscall.c in linux-user to
allow us to compile without these old syscalls.

Signed-off-by: Alistair Francis 
---
 linux-user/strace.c  |  2 ++
 linux-user/syscall.c | 77 ++--
 2 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index 4f7130b2ff..6420ccd97b 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -775,6 +775,7 @@ print_syscall_ret_newselect(const struct syscallname *name, 
abi_long ret)
 #define TARGET_TIME_OOP  3   /* leap second in progress */
 #define TARGET_TIME_WAIT 4   /* leap second has occurred */
 #define TARGET_TIME_ERROR5   /* clock not synchronized */
+#ifdef TARGET_NR_adjtimex
 static void
 print_syscall_ret_adjtimex(const struct syscallname *name, abi_long ret)
 {
@@ -813,6 +814,7 @@ print_syscall_ret_adjtimex(const struct syscallname *name, 
abi_long ret)
 
 qemu_log("\n");
 }
+#endif
 
 UNUSED static struct flags access_flags[] = {
 FLAG_GENERIC(F_OK),
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 8d27d10807..c000fb07c5 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -245,7 +245,11 @@ static type name (type1 arg1,type2 arg2,type3 arg3,type4 
arg4,type5 arg5,  \
 #define __NR_sys_rt_sigqueueinfo __NR_rt_sigqueueinfo
 #define __NR_sys_rt_tgsigqueueinfo __NR_rt_tgsigqueueinfo
 #define __NR_sys_syslog __NR_syslog
-#define __NR_sys_futex __NR_futex
+#ifdef __NR_futex
+# define __NR_sys_futex __NR_futex
+#elif defined(__NR_futex_time64)
+# define __NR_sys_futex __NR_futex_time64
+#endif
 #define __NR_sys_inotify_init __NR_inotify_init
 #define __NR_sys_inotify_add_watch __NR_inotify_add_watch
 #define __NR_sys_inotify_rm_watch __NR_inotify_rm_watch
@@ -295,7 +299,9 @@ _syscall1(int,exit_group,int,error_code)
 #if defined(TARGET_NR_set_tid_address) && defined(__NR_set_tid_address)
 _syscall1(int,set_tid_address,int *,tidptr)
 #endif
-#if defined(TARGET_NR_futex) && defined(__NR_futex)
+#if ((defined(TARGET_NR_futex) || defined(TARGET_NR_futex_time64)) && \
+ defined(__NR_futex)) || \
+(defined(TARGET_NR_futex_time64) && defined(__NR_futex_time64))
 _syscall6(int,sys_futex,int *,uaddr,int,op,int,val,
   const struct timespec *,timeout,int *,uaddr2,int,val3)
 #endif
@@ -742,21 +748,30 @@ safe_syscall3(ssize_t, read, int, fd, void *, buff, 
size_t, count)
 safe_syscall3(ssize_t, write, int, fd, const void *, buff, size_t, count)
 safe_syscall4(int, openat, int, dirfd, const char *, pathname, \
   int, flags, mode_t, mode)
+#if defined(TARGET_NR_wait4) || defined(TARGET_NR_waitpid)
 safe_syscall4(pid_t, wait4, pid_t, pid, int *, status, int, options, \
   struct rusage *, rusage)
+#endif
 safe_syscall5(int, waitid, idtype_t, idtype, id_t, id, siginfo_t *, infop, \
   int, options, struct rusage *, rusage)
 safe_syscall3(int, execve, const char *, filename, char **, argv, char **, 
envp)
+#if defined(TARGET_NR_select) || defined(TARGET_NR__newselect) || \
+defined(TARGET_NR_pselect6)
 safe_syscall6(int, pselect6, int, nfds, fd_set *, readfds, fd_set *, writefds, 
\
   fd_set *, exceptfds, struct timespec *, timeout, void *, sig)
+#endif
+#if defined(TARGET_NR_ppoll) || defined(TARGET_NR_poll)
 safe_syscall5(int, ppoll, struct pollfd *, ufds, unsigned int, nfds,
   struct timespec *, tsp, const sigset_t *, sigmask,
   size_t, sigsetsize)
+#endif
 safe_syscall6(int, epoll_pwait, int, epfd, struct epoll_event *, events,
   int, maxevents, int, timeout, const sigset_t *, sigmask,
   size_t, sigsetsize)
+#ifdef TARGET_NR_futex
 safe_syscall6(int,futex,int *,uaddr,int,op,int,val, \
   const struct timespec *,timeout,int *,uaddr2,int,val3)
+#endif
 safe_syscall2(int, rt_sigsuspend, sigset_t *, newset, size_t, sigsetsize)
 safe_syscall2(int, kill, pid_t, pid, int, sig)
 safe_syscall2(int, tkill, int, tid, int, sig)
@@ -776,12 +791,16 @@ safe_syscall6(ssize_t, recvfrom, int, fd, void *, buf, 
size_t, len,
 safe_syscall3(ssize_t, sendmsg, int, fd, const struct msghdr *, msg, int, 
flags)
 safe_syscall3(ssize_t, recvmsg, int, fd, struct msghdr *, msg, int, flags)
 safe_syscall2(int, flock, int, fd, int, operation)
+#ifdef TARGET_NR_rt_sigtimedwait
 safe_syscall4(int, rt_sigtimedwait, const sigset_t *, these, siginfo_t *, 
uinfo,
   const struct timespec *, uts, size_t, sigsetsize)
+#endif
 safe_syscall4(int, accept4, int, fd, struct sockaddr *, addr, socklen_t *, len,
   int, flags)
+#if defined(TARGET_NR_nanosleep)
 safe_syscall2(int, nanosleep, const struct timespec *, req,
   struct timespec *, rem)
+#endif
 #ifdef TARGET_NR_clock_nanosleep
 safe_syscall4(int, clock_nanosleep, const clockid_t,

[PATCH v4 2/3] linux-user/syscall: Add support for clock_gettime64/clock_settime64

Add support for the clock_gettime64/clock_settime64 syscalls. Currently
we only support these syscalls when running on 64-bit hosts.

Signed-off-by: Alistair Francis 
---
 linux-user/syscall.c | 43 +++
 1 file changed, 43 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c000fb07c5..82468e018d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1236,6 +1236,22 @@ static inline abi_long target_to_host_timespec(struct 
timespec *host_ts,
 }
 #endif
 
+#if defined(TARGET_NR_clock_settime64) && HOST_LONG_BITS == 64
+static inline abi_long target_to_host_timespec64(struct timespec *host_ts,
+ abi_ulong target_addr)
+{
+struct target_timespec *target_ts;
+
+if (!lock_user_struct(VERIFY_READ, target_ts, target_addr, 1)) {
+return -TARGET_EFAULT;
+}
+__get_user(host_ts->tv_sec, _ts->tv_sec);
+__get_user(host_ts->tv_nsec, _ts->tv_nsec);
+unlock_user_struct(target_ts, target_addr, 0);
+return 0;
+}
+#endif
+
 static inline abi_long host_to_target_timespec(abi_ulong target_addr,
struct timespec *host_ts)
 {
@@ -11465,6 +11481,20 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
 return ret;
 }
 #endif
+#ifdef TARGET_NR_clock_settime64
+# if HOST_LONG_BITS == 64
+case TARGET_NR_clock_settime64:
+{
+struct timespec ts;
+
+ret = target_to_host_timespec64(, arg2);
+if (!is_error(ret)) {
+ret = get_errno(clock_settime(arg1, ));
+}
+return ret;
+}
+# endif
+#endif
 #ifdef TARGET_NR_clock_gettime
 case TARGET_NR_clock_gettime:
 {
@@ -11476,6 +11506,19 @@ static abi_long do_syscall1(void *cpu_env, int num, 
abi_long arg1,
 return ret;
 }
 #endif
+#ifdef TARGET_NR_clock_gettime64
+# if HOST_LONG_BITS == 64
+case TARGET_NR_clock_gettime64:
+{
+struct timespec ts;
+ret = get_errno(clock_gettime(arg1, ));
+if (!is_error(ret)) {
+ret = host_to_target_timespec64(arg2, );
+}
+return ret;
+}
+# endif
+#endif
 #ifdef TARGET_NR_clock_getres
 case TARGET_NR_clock_getres:
 {
-- 
2.25.1

[PATCH 1/2] misc: Replace zero-length arrays with flexible array member (automatic)

Description copied from Linux kernel commit from Gustavo A. R. Silva
(see [3]):

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

All these instances of code were found with the help of the
following Coccinelle script:

  @@
  identifier s, a;
  type T;
  @@
   struct s {
  ...
  -   T a[0];
  +   T a[];
  };
  @@
  identifier s, a;
  type T;
  @@
   struct s {
  ...
  -   T a[0];
  +   T a[];
   } QEMU_PACKED;

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76497732932f
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/commit/?id=17642a2fbd2c1

Inspired-by: Gustavo A. R. Silva 
Signed-off-by: Philippe Mathieu-Daudé 
---
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 12 ++--
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 25 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed9c7..f8bb1e5459 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -95,7 +95,7 @@ typedef struct TaskState {
 struct sigqueue *first_free; /* first free siginfo queue entry */
 int signal_pending; /* non zero if a signal may be pending */
 
-uint8_t stack[0];
+uint8_t stack[];
 } __attribute__((aligned(16))) TaskState;
 
 void init_task_state(TaskState *ts);
diff --git a/contrib/libvhost-user/libvhost-user.h 
b/contrib/libvhost-user/libvhost-user.h
index 6fc8000e99..f30394fab6 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -286,7 +286,7 @@ typedef struct VuVirtqInflight {
 uint16_t used_idx;
 
 /* Used to track the state of each descriptor in descriptor table */
-VuDescStateSplit desc[0];
+VuDescStateSplit desc[];
 } VuVirtqInflight;
 
 typedef struct VuVirtqInflightDesc {
diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index 5f8ded2686..c954270aad 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -14,7 +14,7 @@
 struct bi_record {
 uint16_t tag;/* tag ID */
 uint16_t size;   /* size of record */
-uint32_t data[0];/* data */
+uint32_t data[]; /* data */
 };
 
 /* machine independent tags */
diff --git a/hw/scsi/srp.h b/hw/scsi/srp.h
index d27f31d2d5..54c954badd 100644
--- a/hw/scsi/srp.h
+++ b/hw/scsi/srp.h
@@ -112,7 +112,7 @@ struct srp_direct_buf {
 struct srp_indirect_buf {
 struct srp_direct_buftable_desc;
 uint32_t len;
-struct srp_direct_bufdesc_list[0];
+struct srp_direct_bufdesc_list[];
 } QEMU_PACKED;
 
 enum {
@@ -211,7 +211,7 @@ struct srp_cmd {
 uint8_treserved4;
 uint8_tadd_cdb_len;
 uint8_tcdb[16];
-uint8_tadd_data[0];
+uint8_tadd_data[];
 } QEMU_PACKED;
 
 enum {
@@ -241,7 +241,7 @@ struct srp_rsp {
 uint32_t   data_in_res_cnt;
 uint32_t   sense_data_len;
 uint32_t   resp_data_len;
-uint8_tdata[0];
+uint8_tdata[];
 } QEMU_PACKED;
 
 #endif /* SCSI_SRP_H */
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 9167bbaf6d..179775db7b 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -203,7 +203,7 @@ typedef struct XenPTMSIX {
 uint64_t

[PATCH v4 0/3] linux-user: generate syscall_nr.sh for RISC-V

This series updates the RISC-V syscall_nr.sh based on the 5.5 kernel.

There are two parts to this. One is just adding the new syscalls, the
other part is updating the RV32 syscalls to match the fact that RV32 is
a 64-bit time_t architectures (y2038) safe.

We need to make some changes to syscall.c to avoid warnings/errors
during compliling with the new syscall.

I did some RV32 user space testing after applying these patches. I ran the
glibc testsuite in userspace and I don't see any regressions.

Alistair Francis (3):
  linux-user: Protect more syscalls
  linux-user/syscall: Add support for clock_gettime64/clock_settime64
  linux-user/riscv: Update the syscall_nr's to the 5.5 kernel

 linux-user/riscv/syscall32_nr.h | 295 +++
 linux-user/riscv/syscall64_nr.h | 301 
 linux-user/riscv/syscall_nr.h   | 294 +--
 linux-user/strace.c |   2 +
 linux-user/syscall.c| 120 -
 5 files changed, 717 insertions(+), 295 deletions(-)
 create mode 100644 linux-user/riscv/syscall32_nr.h
 create mode 100644 linux-user/riscv/syscall64_nr.h

-- 
2.25.1

[PATCH 0/2] misc: Replace zero-length arrays with flexible array member

This is a tree-wide cleanup inspired by a Linux kernel commit
(from Gustavo A. R. Silva).

--v-- description start --v--

  The current codebase makes use of the zero-length array language
  extension to the C90 standard, but the preferred mechanism to
  declare variable-length types such as these ones is a flexible
  array member [1], introduced in C99:

  struct foo {
  int stuff;
  struct boo array[];
  };

  By making use of the mechanism above, we will get a compiler
  warning in case the flexible array does not occur last in the
  structure, which will help us prevent some kind of undefined
  behavior bugs from being unadvertenly introduced [2] to the
  Linux codebase from now on.

--^-- description end --^--

Do the similar housekeeping in the QEMU codebase (which uses
C99 since commit 7be41675f7cb).

The first patch is done with the help of a coccinelle semantic
patch. However Coccinelle does not recognize:

  struct foo {
  int stuff;
  struct boo array[];
  } QEMU_PACKED;

but does recognize:

  struct QEMU_PACKED foo {
  int stuff;
  struct boo array[];
  };

I'm not sure why, neither it is worth refactoring all QEMU
structures to use the attributes before the structure name,
so I did the 2nd patch manually.

Anyway this is annoying, because many structures are not handled
by coccinelle. Maybe this needs to be reported to upstream
coccinelle?

I used spatch 1.0.8 with:

  -I include --include-headers \
  --macro-file scripts/cocci-macro-file.h \
  --keep-comments --indent 4

Regards,

Phil.

Philippe Mathieu-Daudé (2):
  misc: Replace zero-length arrays with flexible array member
(automatic)
  misc: Replace zero-length arrays with flexible array member (manual)

 docs/interop/vhost-user.rst   |  4 ++--
 block/qed.h   |  2 +-
 bsd-user/qemu.h   |  2 +-
 contrib/libvhost-user/libvhost-user.h |  2 +-
 hw/m68k/bootinfo.h|  2 +-
 hw/scsi/srp.h |  6 +++---
 hw/xen/xen_pt.h   |  2 +-
 include/hw/acpi/acpi-defs.h   | 16 
 include/hw/arm/smmu-common.h  |  2 +-
 include/hw/boards.h   |  2 +-
 include/hw/i386/intel_iommu.h |  3 ++-
 include/hw/s390x/event-facility.h |  2 +-
 include/hw/s390x/sclp.h   |  8 
 include/hw/virtio/virtio-iommu.h  |  2 +-
 include/sysemu/cryptodev.h|  2 +-
 include/tcg/tcg.h |  2 +-
 pc-bios/s390-ccw/bootmap.h|  2 +-
 pc-bios/s390-ccw/sclp.h   |  2 +-
 tests/qtest/libqos/ahci.h |  2 +-
 block/linux-aio.c |  2 +-
 block/vmdk.c  |  2 +-
 hw/acpi/nvdimm.c  |  6 +++---
 hw/char/sclpconsole-lm.c  |  2 +-
 hw/char/sclpconsole.c |  2 +-
 hw/dma/soc_dma.c  |  2 +-
 hw/i386/x86.c |  2 +-
 hw/misc/omap_l4.c |  2 +-
 hw/nvram/eeprom93xx.c |  2 +-
 hw/rdma/vmw/pvrdma_qp_ops.c   |  4 ++--
 hw/s390x/virtio-ccw.c |  2 +-
 hw/usb/dev-network.c  |  2 +-
 hw/usb/dev-smartcard-reader.c |  4 ++--
 hw/virtio/virtio.c|  4 ++--
 net/queue.c   |  2 +-
 target/s390x/ioinst.c |  2 +-
 35 files changed, 54 insertions(+), 53 deletions(-)

-- 
2.21.1

Re: [PATCH 2/2] via-ide: Also emulate non 100% native mode

2020-03-03 Thread BALATON Zoltan

On Tue, 3 Mar 2020, Mark Cave-Ayland wrote:

On 02/03/2020 21:40, BALATON Zoltan wrote:

I had a quick look at the schematics linked from the page above, and they
confirm
that the VIA IDE interface is connected directly to IRQs 14 and 15 and not to
the PCI
interrupt pins.

Where did you see that? What I got from trying to look this up in the
schematics was
that VT8231 has two pins named IRQ14 and IRQ15 (but described as Primary and
Secondary Channel Interrupt Request in doc) where the interrupt lines of the
two IDE
ports/channels are connected but how they are routed within the chip after that
was
not clear. The chip doc says that in native mode the interrupt should be
configurable
and use a single interrupt for both channels and in legacy mode they use the
usual 14
and 15 but this is not what guest drivers expect so I think not how really
works on
PegasosII. It is true however that connection to PCI interrupts aren't
mentioned so
it always uses ISA IRQ numbers, it just depends on legacy vs. native mode which
line
is raised. But that was never really a question for VT8231 and maybe only CMD646
could have such interconnection with PCI interrupts. (Proabable reason is that
via-ide is part of a southbridge chip where it has connections to ISA bus while
CMD646 is a PCI IDE controller but I could be wrong as my knowledge is limited
about
these.)

Presumably the VIA southbridge has its own internal pair of cascaded 8259s so
the IRQ
line from the drive is connected to IRQ14/15 as per an typical ISA PC. You can
see
this in the "Common Hardware Reference Platform: I/O Device Reference" PDF
section 4.1.

Yes the VIA southbridge chip integrates all usual PC devices including
PICs so these may have connection internally. I haven't checked these docs
before but now I think another chapter in the doc you referenced and its
companion "Common Hardware Reference Platform: A System Architecture" may
explain the unusal combination of PCI like io regs with legacy interrupts
the Pegasos2 seems to have. In the I/O Device Reference, chapter 11 about
IDE says that the device must be addressed only with PCI I/O addresses
where io addresses are completely relocatable and that when OpenFirmware
passes control to the OS IDE must be configured as a PCI device. So this
probably means io addresses coming from PCI BARs. But the CHRP System
Architecture, chapter 9.2 about ISA devices says that "ISA devices
included in a PCI part must route their interrupt signals to the legacy
interrupt controller" and this is what the Pegasos2 seems to do. This may
be a misinterpretation of the spec because elsewhere (I've lost the exact
reference and have no time to find it again) it also says something about
native devices must be using OpenPIC but Pegasos2 does not have OpenPIC so
that part surely cannot apply anyway.

So on that basis the best explanation as to what is happening is the
comment in the link you provided here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/powerpc/platforms/chrp/pci.c?h=v4.14.172#n353

/* Pegasos2 firmware version 20040810 configures the built-in IDE controller
* in legacy mode, but sets the PCI registers to PCI native mode.
* The chip can only operate in legacy mode, so force the PCI class into legacy
* mode as well. The same fixup must be done to the class-code property in
* the IDE node /pci@8000/ide@C,1
*/

I'm not sure that it makes much sense that the firmware configures the chip one
way
then puts info about a different way in the device tree. There could be bugs
but this
is not likely. Especially because I see in traces that the firmware does try to
configure the device in native mode. These are the first few accesses of the
firmware
to via-ide:

But that is exactly what is happening! The comment above clearly indicates the
firmware incorrectly sets the IDE controller in native mode which is in exact
agreement with the trace you provide below. And in fact if you look at

I may be reading the comment wrong but to me that says that "firmware
configures IDE in _legacy_ mode" whereas the trace shows it actually
configures it in _native_ mode which is complying to the CHRP doc above.
But since it cannot comply to the "native devices using OpenPIC" part it
probably tries to apply the "ISA devices embedded in PCI" part and locks
IRQ to 14 and 15. Or it just wants to avoid sharing IRQs as much as
possible and the designers decided they will use IRQ14 and 15 for IDE.

https://www.powerdeveloper.org/platforms/pegasos/devicetree you can see the
nvramrc
hack that was released in order to fix the device tree to boot Linux which
alters the
class-code and sets interrupts to 14 (which I believe is invalid, but seemingly
good
enough here).

Isn't this the same fixup that newer Linux kernels already include? Maybe
this was needed before Linux properly supported Pegasos2 but later kernels
will do this anyway. This does not give us any new info

Re: [PATCH] cpu: Use DeviceClass reset instead of a special CPUClass reset


On 3/3/20 7:36 PM, Peter Maydell wrote:

On Tue, 3 Mar 2020 at 18:33, Philippe Mathieu-Daudé  wrote:

Nitpick: you don't need to include the bracket symbol in the diff:

@@
-resetfn(CPUState *cpu)
+resetfn(DeviceState *dev)
 {

(simply indent it with a space).


I think this was probably leftover from trying to get Coccinelle
to not rewrap the '{' onto the previous line, before I found
--smpl-spacing.

In general I don't find it terribly useful to spend a great
deal of time streamlining Coccinelle scripts -- I think they
are basically one-shot uses almost all of the time, so once
they're producing the right changes I prefer to move on.


I agree in this case it is not useful to add the script to the 
repository as a file, because the cleanup is done (as you said, 
'one-shot script'). The script is however valuable as in the commit 
description.


Cleaning it up is not for performance, but as the script is buried into 
the git repository, it can (and will) be used as reference/example by 
other developers.


Anyway this patch is queued now, so let's move on :)

Re: [PATCH v6 7/9] iotests: ignore import warnings from pylint


On 3/3/20 8:57 PM, John Snow wrote:

On 2/27/20 9:14 AM, Philippe Mathieu-Daudé wrote:

On 2/27/20 1:06 AM, John Snow wrote:

The right way to solve this is to come up with a virtual environment
infrastructure that sets all the paths correctly, and/or to create
installable python modules that can be imported normally.

That's hard, so just silence this error for now.


I'm tempted to NAck this and require an "installable python module"...

Let's discuss why it is that hard!



I've been tricked into this before. It's not work I am interested in
doing right now; it's WAY beyond the scope of what I am doing here.

It involves properly factoring all of our python code, deciding which
portions are meant to be installed separately from QEMU itself, coming
up with a versioning scheme, packaging code, and moving code around in
many places.

Then it involves coming up with tooling and infrastructure for creating
virtual environments, installing the right packages to it, and using it
to run our python tests.

No, that's way too invasive. I'm not doing it and I will scream loudly
if you make me.

A less invasive hack involves setting the python import path in a
consolidated spot so that python knows where it can import from. This
works, but might break the ability to run such tests as one-offs without
executing the environment setup.

Again, not work I care to do right now and so I won't. The benefit of
these patches is to provide some minimum viable CI CQA for Python where
we had none before, NOT fix every possible Python problem in one shot.


OK I guess we misunderstood each other :)

I didn't understood your comment as personal to you for this patch, but 
generic. It makes sense it is not your priority and it is obvious this 
task will take a single developer a lot of time resources. I am 
certainly NOT asking you to do it.


My question was rather community-oriented.
(Cc'ing Eduardo because we talked about this after the last KVM forum).



--js



Signed-off-by: John Snow 
---
   tests/qemu-iotests/iotests.py | 1 +
   1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/iotests.py
b/tests/qemu-iotests/iotests.py
index 60c4c7f736..214f59995e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@
   from collections import OrderedDict
   from typing import Collection
   +# pylint: disable=import-error, wrong-import-position
   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..',
'python'))
   from qemu import qtest

Re: [PATCH] Fixed integer overflow in e1000e


Hi Andrew,

Please Cc all the maintainers:

  ./scripts/get_maintainer.pl -f hw/net/e1000e.c
  Dmitry Fleytman  (maintainer:e1000e)
  Jason Wang  (odd fixer:Network devices)
  qemu-devel@nongnu.org (open list:All patches CC here)

On 3/3/20 7:29 PM, and...@daynix.com wrote:

From: Andrew Melnychenko 

https://bugzilla.redhat.com/show_bug.cgi?id=1737400


The BZ backtrace lists e1000e_tx_pkt_send() which you fixes,
but you also fixes e1000e_start_recv(). Good.

Please include the BZ backtrace.


Fixed setting max_queue_num if there are no peers in NICConf. qemu_new_nic() 
creates NICState with 1 NetClientState(index 0) without peers, set 
max_queue_num to 0 - It prevents undefined behavior and possible crashes, 
especially during pcie hotplug.



Please add:

Cc: qemu-sta...@nongnu.org
Fixes: 6f3fbe4ed06
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1737400


Signed-off-by: Andrew Melnychenko 
---
  hw/net/e1000e.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index a91dbdca3c..f2cc1552c5 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -328,7 +328,7 @@ e1000e_init_net_peer(E1000EState *s, PCIDevice *pci_dev, 
uint8_t *macaddr)
  s->nic = qemu_new_nic(_e1000e_info, >conf,
  object_get_typename(OBJECT(s)), dev->id, s);
  
-s->core.max_queue_num = s->conf.peers.queues - 1;

+s->core.max_queue_num = s->conf.peers.queues ? s->conf.peers.queues - 1 : 
0;
  
  trace_e1000e_mac_set_permanent(MAC_ARG(macaddr));

  memcpy(s->core.permanent_mac, macaddr, sizeof(s->core.permanent_mac));

Re: [PATCH v2] linux-user: Add AT_EXECFN auxval

2020-03-03 Thread Lirong Yuan

On Tue, Mar 3, 2020 at 1:40 AM Laurent Vivier  wrote:
>
> Le 02/03/2020 à 20:31, Lirong Yuan a écrit :
> > This change adds the support for AT_EXECFN auxval.
> >
> > Signed-off-by: Lirong Yuan 
> > ---
> > Changelog since v1:
> > - remove implementation for AT_EXECFD auxval.
> >
> >  linux-user/elfload.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index db748c5877..8198be0446 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -1573,7 +1573,7 @@ struct exec
> >   ~(abi_ulong)(TARGET_ELF_EXEC_PAGESIZE-1))
> >  #define TARGET_ELF_PAGEOFFSET(_v) ((_v) & (TARGET_ELF_EXEC_PAGESIZE-1))
> >
> > -#define DLINFO_ITEMS 15
> > +#define DLINFO_ITEMS 16
> >
> >  static inline void memcpy_fromfs(void * to, const void * from, unsigned 
> > long n)
> >  {
> > @@ -2037,6 +2037,7 @@ static abi_ulong create_elf_tables(abi_ulong p, int 
> > argc, int envc,
> >  NEW_AUX_ENT(AT_CLKTCK, (abi_ulong) sysconf(_SC_CLK_TCK));
> >  NEW_AUX_ENT(AT_RANDOM, (abi_ulong) u_rand_bytes);
> >  NEW_AUX_ENT(AT_SECURE, (abi_ulong) qemu_getauxval(AT_SECURE));
> > +NEW_AUX_ENT(AT_EXECFN, info->file_string);
> >
> >  #ifdef ELF_HWCAP2
> >  NEW_AUX_ENT(AT_HWCAP2, (abi_ulong) ELF_HWCAP2);
> >
>
> Applied to my linux-user branch.
>
> Thanks,
> Laurent

Awesome, thanks for the review! :)

Regards,
Lirong

Re: [PULL V2 01/23] dp8393x: Mask EOL bit from descriptor addresses

2020-03-03 Thread Finn Thain

Hi Jason,

The patch in this pull request (since merged) differs from the patch that 
I sent. In particular, the change below is missing from commit 88f632fbb1 
("dp8393x: Mask EOL bit from descriptor addresses") in mainline.

--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -525,8 +525,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
  * (4 + 3 * s->regs[SONIC_TFC]),
MEMTXATTRS_UNSPECIFIED, s->data,
size);
-s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
 /* EOL detected */
 break;
 }

Please compare with "[PATCH v4 01/14] dp8393x: Mask EOL bit from 
descriptor addresses" in the mailing list archives: 
https://lore.kernel.org/qemu-devel/d6e8d06ad4d02f4a30c4caa6001967f806f21a1a.1580290069.git.fth...@telegraphics.com.au/

It appears that this portion of my patch went missing when merge conflicts 
were resolved. The conflicts were apparently caused by commit 19f7034773 
("Avoid address_space_rw() with a constant is_write argument").

Regards,
Finn

On Tue, 3 Mar 2020, Jason Wang wrote:

> From: Finn Thain 
> 
> The Least Significant bit of a descriptor address register is used as
> an EOL flag. It has to be masked when the register value is to be used
> as an actual address for copying memory around. But when the registers
> are to be updated the EOL bit should not be masked.
> 
> Signed-off-by: Finn Thain 
> Tested-by: Laurent Vivier 
> Signed-off-by: Jason Wang 
> ---
>  hw/net/dp8393x.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 7045193..216d44b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ## 
> __VA_ARGS__); } while (0)
>  #define SONIC_ISR_PINT   0x0800
>  #define SONIC_ISR_LCD0x1000
>  
> +#define SONIC_DESC_EOL   0x0001
> +#define SONIC_DESC_ADDR  0xFFFE
> +
>  #define TYPE_DP8393X "dp8393x"
>  #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
>  
> @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
>  
>  static uint32_t dp8393x_crda(dp8393xState *s)
>  {
> -return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> +return (s->regs[SONIC_URDA] << 16) |
> +   (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
>  }
>  
>  static uint32_t dp8393x_rbwc(dp8393xState *s)
> @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
>  
>  static uint32_t dp8393x_ttda(dp8393xState *s)
>  {
> -return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> +return (s->regs[SONIC_UTDA] << 16) |
> +   (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
>  }
>  
>  static uint32_t dp8393x_wt(dp8393xState *s)
> @@ -509,7 +514,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> MEMTXATTRS_UNSPECIFIED, s->data,
> size);
>  s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> -if (dp8393x_get(s, width, 0) & 0x1) {
> +if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>  /* EOL detected */
>  break;
>  }
> @@ -765,13 +770,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, 
> const uint8_t * buf,
>  /* XXX: Check byte ordering */
>  
>  /* Check for EOL */
> -if (s->regs[SONIC_LLFA] & 0x1) {
> +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>  /* Are we still in resource exhaustion? */
>  size = sizeof(uint16_t) * 1 * width;
>  address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
>  address_space_read(>as, address, MEMTXATTRS_UNSPECIFIED,
> s->data, size);
> -if (dp8393x_get(s, width, 0) & 0x1) {
> +if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
>  /* Still EOL ; stop reception */
>  return -1;
>  } else {
> @@ -831,7 +836,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const 
> uint8_t * buf,
> dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> MEMTXATTRS_UNSPECIFIED, s->data, size);
>  s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> -if (s->regs[SONIC_LLFA] & 0x1) {
> +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>  /* EOL detected */
>  s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>  } else {
>

Re: [PATCH] cpu: Use DeviceClass reset instead of a special CPUClass reset

2020-03-03 Thread David Gibson

On Tue, Mar 03, 2020 at 10:05:11AM +, Peter Maydell wrote:
> The CPUClass has a 'reset' method.  This is a legacy from when
> TYPE_CPU used not to inherit from TYPE_DEVICE.  We don't need it any
> more, as we can simply use the TYPE_DEVICE reset.  The 'cpu_reset()'
> function is kept as the API which most places use to reset a CPU; it
> is now a wrapper which calls device_cold_reset() and then the
> tracepoint function.
> 
> This change should not cause CPU objects to be reset more often
> than they are at the moment, because:
>  * nobody is directly calling device_cold_reset() or
>qdev_reset_all() on CPU objects
>  * no CPU object is on a qbus, so they will not be reset either
>by somebody calling qbus_reset_all()/bus_cold_reset(), or
>by the main "reset sysbus and everything in the qbus tree"
>reset that most devices are reset by
> 
> Note that this does not change the need for each machine or whatever
> to use qemu_register_reset() to arrange to call cpu_reset() -- that
> is necessary because CPU objects are not on any qbus, so they don't
> get reset when the qbus tree rooted at the sysbus bus is reset, and
> this isn't being changed here.
> 
> All the changes to the files under target/ were made using the
> included Coccinelle script, except:
> 
> (1) the deletion of the now-inaccurate and not terribly useful
> "CPUClass::reset" comments was done with a perl one-liner afterwards:
>   perl -n -i -e '/ CPUClass::reset/ or print' target/*/*.c
> 
> (2) this bit of the s390 change was done by hand, because the
> Coccinelle script is not sophisticated enough to handle the
> parent_reset call being inside another function:
> 
> | @@ -96,8 +96,9 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
> type)
> | S390CPU *cpu = S390_CPU(s);
> | S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> | CPUS390XState *env = >env;
> |+DeviceState *dev = DEVICE(s);
> |
> |-scc->parent_reset(s);
> |+scc->parent_reset(dev);
> | cpu->env.sigp_order = 0;
> | s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
> 
> Signed-off-by: Peter Maydell 

ppc parts

Acked-by: David Gibson 

> ---
> Testing was by 'make check' and 'make check-acceptance'.
> 
> I need this patch as a preliminary to some arm stuff I'm
> doing, but I think it makes sense as a cleanup in its own
> right so I'm sending it out early for review. If it's not
> yet in master before I get round to finishing the stuff
> that depends on it I'll resend it as part of that series.
> ---
>  include/hw/core/cpu.h  |  6 
>  target/alpha/cpu-qom.h |  2 +-
>  target/arm/cpu-qom.h   |  2 +-
>  target/cris/cpu-qom.h  |  2 +-
>  target/hppa/cpu-qom.h  |  2 +-
>  target/i386/cpu-qom.h  |  2 +-
>  target/lm32/cpu-qom.h  |  2 +-
>  target/m68k/cpu-qom.h  |  2 +-
>  target/microblaze/cpu-qom.h|  2 +-
>  target/mips/cpu-qom.h  |  2 +-
>  target/moxie/cpu.h |  2 +-
>  target/nios2/cpu.h |  2 +-
>  target/openrisc/cpu.h  |  2 +-
>  target/ppc/cpu-qom.h   |  2 +-
>  target/riscv/cpu.h |  2 +-
>  target/s390x/cpu-qom.h |  2 +-
>  target/sh4/cpu-qom.h   |  2 +-
>  target/sparc/cpu-qom.h |  2 +-
>  target/tilegx/cpu.h|  2 +-
>  target/tricore/cpu-qom.h   |  2 +-
>  target/xtensa/cpu-qom.h|  2 +-
>  hw/core/cpu.c  | 19 +++-
>  target/arm/cpu.c   |  8 ++---
>  target/cris/cpu.c  |  8 ++---
>  target/i386/cpu.c  |  8 ++---
>  target/lm32/cpu.c  |  8 ++---
>  target/m68k/cpu.c  |  8 ++---
>  target/microblaze/cpu.c|  8 ++---
>  target/mips/cpu.c  |  8 ++---
>  target/moxie/cpu.c |  7 +++--
>  target/nios2/cpu.c |  8 ++---
>  target/openrisc/cpu.c  |  8 ++---
>  target/ppc/translate_init.inc.c|  8 ++---
>  target/riscv/cpu.c |  7 +++--
>  target/s390x/cpu.c |  8 +++--
>  target/sh4/cpu.c   |  8 ++---
>  target/sparc/cpu.c |  8 ++---
>  target/tilegx/cpu.c|  7 +++--
>  target/tricore/cpu.c   |  7 +++--
>  target/xtensa/cpu.c|  8 ++---
>  scripts/coccinelle/cpu-reset.cocci | 47 ++
>  41 files changed, 144 insertions(+), 108 deletions(-)
>  create mode 100644 scripts/coccinelle/cpu-reset.cocci
> 
> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> index 73e9a869a41..88ee543722d 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -79,7 +79,6 @@ struct TranslationBlock;
>   * @class_by_name: Callback to map -cpu command line model name to an
>   * instantiatable CPU type.
>   * @parse_features: Callback to parse command line arguments.
> - *

Re: [PATCH 4/4] qapi: Brush off some (py)lint




On 2/27/20 9:45 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 

I wrote some pylint cleanup for iotests recently, too. Are you targeting
a subset of pylint errors to clean here?

(Do any files pass 100%?)

Consider checking in a pylintrc file that lets others run the same
subset of pylint tests as you are doing so that we can prevent future
regressions.

Take a peek at [PATCH v6 0/9] iotests: use python logging

Thanks for this series. I had a very similar series sitting waiting to
go out, but this goes further in a few places.

--js

> ---
>  scripts/qapi/commands.py   | 2 +-
>  scripts/qapi/expr.py   | 3 +--
>  scripts/qapi/gen.py| 9 ++---
>  scripts/qapi/introspect.py | 2 --
>  scripts/qapi/parser.py | 6 ++
>  scripts/qapi/schema.py | 9 -
>  6 files changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 8bb6316061..0e13e82989 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -274,7 +274,7 @@ class 
> QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor):
>  
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -   c_prefix=c_name(self._prefix, protect=False)))
> + c_prefix=c_name(self._prefix, protect=False)))
>  self._genc.preamble_add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "%(prefix)sqapi-commands.h"
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index d7a289eded..fecf466fa7 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -35,7 +35,6 @@ def check_name_is_str(name, info, source):
>  def check_name_str(name, info, source,
> allow_optional=False, enum_member=False,
> permit_upper=False):
> -global valid_name
>  membername = name
>  
>  if allow_optional and name.startswith('*'):
> @@ -249,7 +248,7 @@ def check_union(expr, info):
>  def check_alternate(expr, info):
>  members = expr['data']
>  
> -if len(members) == 0:
> +if not members:
>  raise QAPISemError(info, "'data' must not be empty")
>  for (key, value) in members.items():
>  source = "'data' member '%s'" % key
> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> index e17354392b..33690bfa3b 100644
> --- a/scripts/qapi/gen.py
> +++ b/scripts/qapi/gen.py
> @@ -45,10 +45,10 @@ class QAPIGen:
>  
>  def write(self, output_dir):
>  pathname = os.path.join(output_dir, self.fname)
> -dir = os.path.dirname(pathname)
> -if dir:
> +odir = os.path.dirname(pathname)
> +if odir:
>  try:
> -os.makedirs(dir)
> +os.makedirs(odir)
>  except os.error as e:
>  if e.errno != errno.EEXIST:
>  raise
> @@ -261,6 +261,9 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor):
>  genc.write(output_dir)
>  genh.write(output_dir)
>  
> +def _begin_system_module(self, name):
> +pass
> +
>  def _begin_user_module(self, name):
>  pass
>  
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 0cc655fd9f..b5537eddc0 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,8 +10,6 @@ This work is licensed under the terms of the GNU GPL, 
> version 2.
>  See the COPYING file in the top-level directory.
>  """
>  
> -import string
> -
>  from qapi.common import *
>  from qapi.gen import QAPISchemaMonolithicCVisitor
>  from qapi.schema import (QAPISchemaArrayType, QAPISchemaBuiltinType,
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 340f7c4633..abadacbb0e 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -282,8 +282,7 @@ class QAPISchemaParser:
>  doc.end_comment()
>  self.accept()
>  return doc
> -else:
> -doc.append(self.val)
> +doc.append(self.val)
>  self.accept(False)
>  
>  raise QAPIParseError(self, "documentation comment must end with 
> '##'")
> @@ -492,7 +491,7 @@ class QAPIDoc:
>  raise QAPIParseError(self._parser,
>   "'%s' can't follow '%s' section"
>   % (name, self.sections[0].name))
> -elif self._is_section_tag(name):
> +if self._is_section_tag(name):
>  line = line[len(name)+1:]
>  self._start_section(name[:-1])
>  
> @@ -556,7 +555,6 @@ class QAPIDoc:
>  raise QAPISemError(feature.info,
> "feature '%s' lacks documentation"
> % feature.name)
> -self.features[feature.name] = QAPIDoc.ArgSection(feature.name)
>  self.features[feature.name].connect(feature)
>  
>  def check_expr(self, expr):
> diff --git

Re: [PATCH 5/6] qmp.py: change event_wait to use a dict




On 2/27/20 6:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2020 3:56, John Snow wrote:
>> It's easier to work with than a list of tuples, because we can check the
>> keys for membership.
>>
>> Signed-off-by: John Snow 
>> ---
>>   python/qemu/machine.py    | 10 +-
>>   tests/qemu-iotests/040    | 12 ++--
>>   tests/qemu-iotests/260    |  5 +++--
>>   tests/qemu-iotests/iotests.py | 16 
>>   4 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index 183d8f3d38..748de5f322 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -476,21 +476,21 @@ def event_wait(self, name, timeout=60.0,
>> match=None):
>>   timeout: QEMUMonitorProtocol.pull_event timeout parameter.
>>   match: Optional match criteria. See event_match for details.
>>   """
>> -    return self.events_wait([(name, match)], timeout)
>> +    return self.events_wait({name: match}, timeout)
>>     def events_wait(self, events, timeout=60.0):
>>   """
>>   events_wait waits for and returns a named event from QMP
>> with a timeout.
>>   -    events: a sequence of (name, match_criteria) tuples.
>> +    events: a mapping containing {name: match_criteria}.
>>   The match criteria are optional and may be None.
>>   See event_match for details.
>>   timeout: QEMUMonitorProtocol.pull_event timeout parameter.
>>   """
>>   def _match(event):
>> -    for name, match in events:
>> -    if event['event'] == name and self.event_match(event,
>> match):
>> -    return True
>> +    name = event['event']
>> +    if name in events:
>> +    return self.event_match(event, events[name])
>>   return False
>>     # Search cached events
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 32c82b4ec6..90b59081ff 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -485,12 +485,12 @@ class TestErrorHandling(iotests.QMPTestCase):
>>     def run_job(self, expected_events, error_pauses_job=False):
>>   match_device = {'data': {'device': 'job0'}}
>> -    events = [
>> -    ('BLOCK_JOB_COMPLETED', match_device),
>> -    ('BLOCK_JOB_CANCELLED', match_device),
>> -    ('BLOCK_JOB_ERROR', match_device),
>> -    ('BLOCK_JOB_READY', match_device),
>> -    ]
>> +    events = {
>> +    'BLOCK_JOB_COMPLETED': match_device,
>> +    'BLOCK_JOB_CANCELLED': match_device,
>> +    'BLOCK_JOB_ERROR': match_device,
>> +    'BLOCK_JOB_READY': match_device,
>> +    }
>>     completed = False
>>   log = []
>> diff --git a/tests/qemu-iotests/260 b/tests/qemu-iotests/260
>> index 30c0de380d..b2fb045ddd 100755
>> --- a/tests/qemu-iotests/260
>> +++ b/tests/qemu-iotests/260
>> @@ -65,8 +65,9 @@ def test(persistent, restart):
>>     vm.qmp_log('block-commit', device='drive0', top=top,
>>  filters=[iotests.filter_qmp_testfiles])
>> -    ev = vm.events_wait((('BLOCK_JOB_READY', None),
>> - ('BLOCK_JOB_COMPLETED', None)))
>> +    ev = vm.events_wait({
>> +    'BLOCK_JOB_READY': None,
>> +    'BLOCK_JOB_COMPLETED': None })
> 
> may be better to keep it 2-lines. Or, otherwise, put closing "})" on a
> separate line too.
> 

Sure.

>>   log(filter_qmp_event(ev))
>>   if (ev['event'] == 'BLOCK_JOB_COMPLETED'):
>>   vm.shutdown()
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 5d2990a0e4..3390fab021 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -604,14 +604,14 @@ def run_job(self, job, auto_finalize=True,
>> auto_dismiss=False,
>>   """
>>   match_device = {'data': {'device': job}}
>>   match_id = {'data': {'id': job}}
>> -    events = [
>> -    ('BLOCK_JOB_COMPLETED', match_device),
>> -    ('BLOCK_JOB_CANCELLED', match_device),
>> -    ('BLOCK_JOB_ERROR', match_device),
>> -    ('BLOCK_JOB_READY', match_device),
>> -    ('BLOCK_JOB_PENDING', match_id),
>> -    ('JOB_STATUS_CHANGE', match_id)
>> -    ]
>> +    events = {
>> +    'BLOCK_JOB_COMPLETED': match_device,
>> +    'BLOCK_JOB_CANCELLED': match_device,
>> +    'BLOCK_JOB_ERROR': match_device,
>> +    'BLOCK_JOB_READY': match_device,
>> +    'BLOCK_JOB_PENDING': match_id,
>> +    'JOB_STATUS_CHANGE': match_id,
>> +    }
>>   error = None
>>   while True:
>>   ev = filter_qmp_event(self.events_wait(events,
>> timeout=wait))
>>
> 
> 
> Not sure that I like new interface more (neither that it is faster), but
> it works too:
> Reviewed-by: Vladimir

Re: [PATCH 3/6] iotests: move bitmap helpers into their own file

On 2/27/20 5:54 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> Clean code movement, no changes. If test passes, it should be correct :)
> 
> The only thing: I'd prefer not exporting global variables and use
> bitmaps.GROUPS instead (even then, it's not very good interface but..)
> 
> with or without it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy 

Hm, yeah -- it's not great, it's definitely just a bit of a quick
shuffle instead of a more serious effort at a refactor to provide bitmap
services for all tests.

I don't think it's worth the time just yet to make a serious attempt at
making this module bigger -- but maybe there is work that can be done at
providing bitmap management services targeted for use outside of testing
code. (i.e. into the python/ folder somewhere.)

Well, I can just do this:
"
import bitmaps
from bitmaps import EmulatedBitmap
"

and bitmaps.GROUPS makes it a little more obvious in-context at the
expense of one extra import line, so I'll do that.

Re: [PATCH 2/6] qmp: expose block-dirty-bitmap-populate




On 2/27/20 5:44 AM, Vladimir Sementsov-Ogievskiy wrote:
> 25.02.2020 3:56, John Snow wrote:
>> This is a new job-creating command.
>>
>> Signed-off-by: John Snow 
>> ---
>>   qapi/block-core.json  | 18 ++
>>   qapi/transaction.json |  2 ++
>>   blockdev.c    | 78 +++
>>   3 files changed, 98 insertions(+)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index df1797681a..a8be1fb36b 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2293,6 +2293,24 @@
>>   '*auto-finalize': 'bool',
>>   '*auto-dismiss': 'bool' } }
>>   +##
>> +# @block-dirty-bitmap-populate:
>> +#
>> +# Creates a new job that writes a pattern into a dirty bitmap.
>> +#
>> +# Since: 5.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "block-dirty-bitmap-populate",
>> +#  "arguments": { "node": "drive0", "target": "bitmap0",
>> +# "job-id": "job0", "pattern": "allocate-top" } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
>> +  'data': 'BlockDirtyBitmapPopulate' }
>> +
>>   ##
>>   # @BlockDirtyBitmapSha256:
>>   #
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index 04301f1be7..28521d5c7f 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -50,6 +50,7 @@
>>   # - @block-dirty-bitmap-enable: since 4.0
>>   # - @block-dirty-bitmap-disable: since 4.0
>>   # - @block-dirty-bitmap-merge: since 4.0
>> +# - @block-dirty-bitmap-populate: since 5.0
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -67,6 +68,7 @@
>>  'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>>  'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>  'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
>> +   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
>>  'blockdev-backup': 'BlockdevBackup',
>>  'blockdev-snapshot': 'BlockdevSnapshot',
>>  'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 011dcfec27..33c0e35399 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -2314,6 +2314,67 @@ static void
>> block_dirty_bitmap_remove_commit(BlkActionState *common)
>>   bdrv_release_dirty_bitmap(state->bitmap);
>>   }
>>   +static void block_dirty_bitmap_populate_prepare(BlkActionState
>> *common, Error **errp)
> 
> over80 line (not the only)
> 

OK, will fix all instances.

>> +{
>> +    BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState,
>> common, common);
> 
> At first glance using *Backup* looks like a mistake. May be rename it,
> or at least add a comment.
> 

You're right, it's tricky. A rename would be helpful.

>> +    BlockDirtyBitmapPopulate *bitpop;
>> +    BlockDriverState *bs;
>> +    AioContext *aio_context;
>> +    BdrvDirtyBitmap *bmap = NULL;
>> +    int job_flags = JOB_DEFAULT;
>> +
>> +    assert(common->action->type ==
>> TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
>> +    bitpop = common->action->u.block_dirty_bitmap_populate.data;
>> +
>> +    bs = bdrv_lookup_bs(bitpop->node, bitpop->node, errp);
>> +    if (!bs) {
>> +    return;
>> +    }
>> +
>> +    aio_context = bdrv_get_aio_context(bs);
>> +    aio_context_acquire(aio_context);
>> +    state->bs = bs;
>> +
>> +    bmap = bdrv_find_dirty_bitmap(bs, bitpop->name);
> 
> Could we use block_dirty_bitmap_lookup ?
> 

Yes.

>> +    if (!bmap) {
>> +    error_setg(errp, "Bitmap '%s' could not be found",
>> bitpop->name);
>> +    return;
> 
> aio context lock leaked
> 

Good spot.

>> +    }
>> +
>> +    /* Paired with .clean() */
>> +    bdrv_drained_begin(state->bs);
>> +
>> +    if (!bitpop->has_on_error) {
>> +    bitpop->on_error = BLOCKDEV_ON_ERROR_REPORT;
>> +    }
>> +    if (!bitpop->has_auto_finalize) {
>> +    bitpop->auto_finalize = true;
>> +    }
>> +    if (!bitpop->has_auto_dismiss) {
>> +    bitpop->auto_dismiss = true;
>> +    }
>> +
>> +    if (!bitpop->auto_finalize) {
>> +    job_flags |= JOB_MANUAL_FINALIZE;
>> +    }
>> +    if (!bitpop->auto_dismiss) {
>> +    job_flags |= JOB_MANUAL_DISMISS;
>> +    }
>> +
>> +    state->job = bitpop_job_create(
>> +    bitpop->job_id,
>> +    bs,
>> +    bmap,
>> +    bitpop->pattern,
>> +    bitpop->on_error,
>> +    job_flags,
>> +    NULL, NULL,
>> +    common->block_job_txn,
>> +    errp);
>> +
>> +    aio_context_release(aio_context);
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>   error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2397,6 +2458,13 @@ static const BlkActionOps actions[] = {
>>   .commit = block_dirty_bitmap_remove_commit,
>>   .abort = block_dirty_bitmap_remove_abort,
>>   },
>> +

Re: [PATCH 1/6] block: add bitmap-populate job

On 2/27/20 1:11 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> Still, if user pass disabled bitmap, it will be invalid immediately after
> job finish.

Yes ... In truth, I really want to augment this job to provide a defined
point-in-time semantic so it can be fully useful in all circumstances.

At the moment, the point-in-time it provides is "at finalize", which may
or may not be the single most useful semantic. (It matches mirror --
because that was easier.)

The other option is "at start" which matches backup, but will require a
new filter and some changes to permissions. That was more invasive, so I
avoided it for now.

(In the interest of a speedy resolution for libvirt, can we add a
critical_moment=[START|FINALIZE] option to this job *later*?)

Re: [PATCH 1/2] iotests: add JobRunner class

On 2/27/20 6:44 AM, Max Reitz wrote:
>> I'll just clean up the logging series I had to do it at a more
>> fundamental level.
> OK.  So you’re looking to basically get VM.qmp() to log automatically if
> necessary?  (or maybe qmp_log() to not log unless necessary)
> 
> Max
> 

Yes. If this series looks good I will just rebase it on top of the
logging series. Then self.logging goes away and the calls to qmp_log et
al just become unconditional.

Then we can enable/disable "all QMP logging" globally instead of
individually throughout the iotests shared codebase.

--js

Re: [PATCH v6 1/9] iotests: do a light delinting




On 2/27/20 7:59 AM, Max Reitz wrote:
> On 27.02.20 01:06, John Snow wrote:
>> This doesn't fix everything in here, but it does help clean up the
>> pylint report considerably.
>>
>> This should be 100% style changes only; the intent is to make pylint
>> more useful by working on establishing a baseline for iotests that we
>> can gate against in the future. This will be important if (when?) we
>> begin adding type hints to our code base.
>>
>> Signed-off-by: John Snow 
>> ---
>>  tests/qemu-iotests/iotests.py | 88 ++-
>>  1 file changed, 45 insertions(+), 43 deletions(-)
> 
> I feel like I’m the wrongest person there is for reviewing a Python
> style-fixing patch, but here I am and so here I go:
> 

No, it's good.

>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 8815052eb5..e8a0ea14fc 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
> 
> [...]
> 
>> @@ -245,8 +243,7 @@ def qemu_nbd_early_pipe(*args):
>>' '.join(qemu_nbd_args + ['--fork'] + 
>> list(args
>>  if exitcode == 0:
>>  return exitcode, ''
>> -else:
>> -return exitcode, subp.communicate()[0]
>> +return exitcode, subp.communicate()[0]
> 
> If we want to make such a change (which I don’t doubt we want), I think
> it should be the other way around: Make the condition “exitcode != 0”,
> so the final return is the return for the successful case.  (Just
> because I think that’s how we usually do it, at least in the qemu code?)
> 
> [...]
> 

Yes, makes sense. I was behaving a little more mechanically.

>> @@ -455,10 +452,9 @@ def file_path(*names, base_dir=test_dir):
>>  def remote_filename(path):
>>  if imgproto == 'file':
>>  return path
>> -elif imgproto == 'ssh':
>> +if imgproto == 'ssh':
> 
> This seems like a weird thing to complain about for a coding style
> check, but whatever.
> 
> (As in, I prefer the elif form)
> 

Honestly, I do too. We can silence the warning instead.

This warning option doesn't like "if return else return" constructs,
preferring instead:

if x:
return 0
return 1

but I have to admit that I often like to see the branches laid out as
branches, too.

Other Pythonistas (Eduardo, Philippe, Markus?) -- strong feelings one
way or the other?

>>  return "ssh://%s@127.0.0.1:22%s" % (os.environ.get('USER'), path)
>> -else:
>> -raise Exception("Protocol %s not supported" % (imgproto))
>> +raise Exception("Protocol %s not supported" % (imgproto))
>>  
>>  class VM(qtest.QEMUQtestMachine):
>>  '''A QEMU VM'''
> 
> [...]
> 
>> @@ -756,12 +750,13 @@ def assert_block_path(self, root, path, expected_node, 
>> graph=None):
>>  assert node is not None, 'Cannot follow path %s%s' % (root, 
>> path)
>>  
>>  try:
>> -node_id = next(edge['child'] for edge in graph['edges'] \
>> - if edge['parent'] == 
>> node['id'] and
>> -edge['name'] == child_name)
>> +node_id = next(edge['child'] for edge in graph['edges']
>> +   if edge['parent'] == node['id'] and
>> +   edge['name'] == child_name)
> 
> I don’t mind the if alignment, but I do mind not aligning the third line
> to the “edge” above it (i.e. the third line is part of the condition, so
> I’d align it to the “if” condition).
> 
> But then again it’s nothing new that I like to disagree with commonly
> agreed-upon Python coding styles, so.
> 
> [...]
> 

OK, that can be addressed by highlighting the sub-expression with
parentheses:

node_id = next(edge['child'] for edge in graph['edges']
   if (edge['parent'] == node['id'] and
   edge['name'] == child_name))


>> @@ -891,13 +892,14 @@ def wait_until_completed(self, drive='drive0', 
>> check_offset=True, wait=60.0,
>>  self.assert_qmp(event, 'data/error', error)
>>  self.assert_no_active_block_jobs()
>>  return event
>> -elif event['event'] == 'JOB_STATUS_CHANGE':
>> +if event['event'] == 'JOB_STATUS_CHANGE':
>>  self.assert_qmp(event, 'data/id', drive)
>>  
>>  def wait_ready(self, drive='drive0'):
>>  '''Wait until a block job BLOCK_JOB_READY event'''
>> -f = {'data': {'type': 'mirror', 'device': drive } }
>> +f = {'data': {'type': 'mirror', 'device': drive}}
>>  event = self.vm.event_wait(name='BLOCK_JOB_READY', match=f)
>> +return event
> 
> Why not just “return self.vm.event_wait…”?
> 

Shrug. Sometimes I name my return variables when working in Python to
give some semantic clue over what exactly I'm even returning.

I can change it; but the docstring will grow to describe what it returns
to re-document the same.

Re: [PATCH v6 2/9] iotests: add script_initialize