Re: [PATCH v1 08/10] vhost: implement vhost_dev_start method

[Cindy Lu]

On Thu, Jun 25, 2020 at 10:35 PM Laurent Vivier  wrote:
>
> On 22/06/2020 17:37, Cindy Lu wrote:
> > use the vhost_dev_start callback to send the status to backend
>
> I agree with Jason, squash this patch with the previous one.
>
will fix this
> > Signed-off-by: Cindy Lu 
> > ---
> >  hw/virtio/vhost.c | 17 +
> >  include/hw/virtio/vhost.h |  2 ++
> >  2 files changed, 19 insertions(+)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 01ebe12f28..bfd7f9ce1f 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -744,6 +744,7 @@ static void vhost_iommu_region_del(MemoryListener 
> > *listener,
> >  }
> >  }
> >
> > +
> >  static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
> >  struct vhost_virtqueue *vq,
> >  unsigned idx, bool enable_log)
> > @@ -1661,6 +1662,11 @@ int vhost_dev_start(struct vhost_dev *hdev, 
> > VirtIODevice *vdev)
> >  }
> >  }
> >
> > +r = vhost_set_start(hdev, true);
>
> Perhaps you can use the same kind of name we have for the queue
> (queue_set_started()) and use something like vhost_dev_set_started()?
>
sure， will fix this
> > +if (r) {
> > +goto fail_log;
> > +}
> > +
> >  if (vhost_dev_has_iommu(hdev)) {
> >  hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true);
> >
> > @@ -1697,6 +1703,8 @@ void vhost_dev_stop(struct vhost_dev *hdev, 
> > VirtIODevice *vdev)
> >  /* should only be called after backend is connected */
> >  assert(hdev->vhost_ops);
> >
> > +vhost_set_start(hdev, false);
> > +
> >  for (i = 0; i < hdev->nvqs; ++i) {
> >  vhost_virtqueue_stop(hdev,
> >   vdev,
> > @@ -1722,3 +1730,12 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> >
> >  return -1;
> >  }
> > +
> > +int vhost_set_start(struct vhost_dev *hdev, bool started)
> > +{
> > +
> > +if (hdev->vhost_ops->vhost_dev_start) {
> > +hdev->vhost_ops->vhost_dev_start(hdev, started);
>
> The "return" is missing.
>
> And generally a function that only embeds a call to a hook has the same
> as the hook.
>
> > +}
> > +return 0;
> > +}
>
> so something like:
>
> int vhost_dev_set_started(struct vhost_dev *hdev, bool started)
> {
> if (hdev->vhost_ops->dev_set_started) {
> return hdev->vhost_ops->dev_set_started(hdev, started);
> }
> return 0;
> }
>
>
thanks will fix this
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 085450c6f8..59ea53f8c2 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -92,6 +92,7 @@ struct vhost_dev {
> >  const VhostDevConfigOps *config_ops;
> >  };
> >
> > +
> >  int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > VhostBackendType backend_type,
> > uint32_t busyloop_timeout);
> > @@ -137,4 +138,5 @@ int vhost_dev_set_inflight(struct vhost_dev *dev,
> > struct vhost_inflight *inflight);
> >  int vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size,
> > struct vhost_inflight *inflight);
> > +int vhost_set_start(struct vhost_dev *dev, bool started);
>
> There is no need to export it, so set it "static" in hw/virtio/vhost.c
> and move the definition before the use.
>
thanks will fix this
> Thanks,
> Laurent
>

Re: [PATCH v1 04/10] virtio-pci: implement queue_enabled method

[Cindy Lu]

On Wed, Jun 24, 2020 at 9:25 PM Laurent Vivier  wrote:
>
> On 22/06/2020 17:37, Cindy Lu wrote:
> > From: Jason Wang 
> >
> > With version 1, we can detect whether a queue is enabled via
> > queue_enabled.
> >
> > Signed-off-by: Jason Wang 
> > Signed-off-by: Cindy Lu 
> > ---
> >  hw/virtio/virtio-pci.c | 18 ++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 4cb784389c..3918aa9f6c 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1107,6 +1107,23 @@ static AddressSpace 
> > *virtio_pci_get_dma_as(DeviceState *d)
> >  return pci_get_address_space(dev);
> >  }
> >
> > +static bool  virtio_queue_check_enabled(VirtIODevice *vdev, int n)
> > +{
> > +return  virtio_queue_get_desc_addr(vdev, n) != 0;
> > +}
>
> This function is already defined under a different name in
> hw/virtio/virtio.c:
>
>
>3287 bool virtio_queue_enabled(VirtIODevice *vdev, int n)
>3288 {
>3289 return virtio_queue_get_desc_addr(vdev, n) != 0;
>3290 }
>
> As this file includes "hw/virtio/virtio.h" you can use it directly.
>
Thanks Laurent, Will fix this
> Thanks,
> Laurent
>

[PATCH v7 0/5] VIRTIO-IOMMU probe request support and MSI bypass on ARM

[Eric Auger]

By default the virtio-iommu translates MSI transactions. This
behavior is inherited from ARM SMMU. However the virt machine
code knows where the MSI doorbells are, so we can easily
declare those regions as VIRTIO_IOMMU_RESV_MEM_T_MSI. With that
setting the guest iommu subsystem will not need to map MSIs.
This setup will simplify the VFIO integration.

In this series, the ITS or GICV2M doorbells are declared as
HW MSI regions to be bypassed by the VIRTIO-IOMMU.

This also paves the way to the x86 integration where the MSI
region, [0xFEE0,0xFEEF], will be exposed by the q35
machine.  However this will be handled in a separate series
when not-DT support gets resolved.

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v5.0.0-virtio-iommu-msi-bypass-v7

History:

v6 -> v7:
- fix the hint message
- put the assert again on probe request

v5 -> v6:
- do not hardcode start/end addresses of doorbells
- check reserved region type on realize()

v4 -> v5:
- Take into account some additional comments from Markus:
  - reserved region type becomes an unsigned + some comment/desc
rewording
  - assert if the type is not RESERVED or MSI

v3 -> v4:
- collected Jean and markus's R-bs
- tool into account all Markus' comments in [1/5] (except removal of
  goto)
- use ':' as delimitor instead of commas
- add example in 4/5 commit message as suggested by Markus

v2 -> v3:
- Introduce VIRT_MSI_CTRL_NONE in VirtMSIControllerType
- do not fill the remainder of the probe buffer

v1 -> v2:
- check which MSI controller is in use and advertise the
  corresponding MSI doorbell
- managed for both ITS and GICv2M
- various fixes spotted by Peter and Jean-Philippe, see
  individual logs

v1: Most of those patches were respinned from
  [PATCH for-5.0 v11 00/20] VIRTIO-IOMMU device
  except the last one which is new


Eric Auger (5):
  qdev: Introduce DEFINE_PROP_RESERVED_REGION
  virtio-iommu: Implement RESV_MEM probe request
  virtio-iommu: Handle reserved regions in the translation process
  virtio-iommu-pci: Add array of Interval properties
  hw/arm/virt: Let the virtio-iommu bypass MSIs

 include/exec/memory.h|   6 ++
 include/hw/arm/virt.h|   7 ++
 include/hw/qdev-properties.h |   3 +
 include/hw/virtio/virtio-iommu.h |   2 +
 include/qemu/typedefs.h  |   1 +
 hw/arm/virt.c|  30 
 hw/core/qdev-properties.c|  89 
 hw/virtio/virtio-iommu-pci.c |  11 +++
 hw/virtio/virtio-iommu.c | 114 +--
 hw/virtio/trace-events   |   1 +
 10 files changed, 260 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH v7 5/5] hw/arm/virt: Let the virtio-iommu bypass MSIs

[Eric Auger]

At the moment the virtio-iommu translates MSI transactions.
This behavior is inherited from ARM SMMU. The virt machine
code knows where the guest MSI doorbells are so we can easily
declare those regions as VIRTIO_IOMMU_RESV_MEM_T_MSI. With that
setting the guest will not map MSIs through the IOMMU and those
transactions will be simply bypassed.

Depending on which MSI controller is in use (ITS or GICV2M),
we declare either:
- the ITS interrupt translation space (ITS_base + 0x1),
  containing the GITS_TRANSLATOR or
- The GICV2M single frame, containing the MSI_SETSP_NS register.

Signed-off-by: Eric Auger 

---
v5 -> v6:
- do not hardcode doorbell base and size
- removed Jean's R-b

v3 -> v4:
- use ':' as separators

v2 -> v3:
- Add a new value to VirtMSIControllerType

v1 -> v2:
- Test which MSI controller is instantiated
- If GICV2M is in use, declare its doorbell as an MSI doorbell too
---
 include/hw/arm/virt.h |  7 +++
 hw/arm/virt.c | 30 ++
 2 files changed, 37 insertions(+)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 31878ddc72..a18b6b397b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -96,6 +96,12 @@ typedef enum VirtIOMMUType {
 VIRT_IOMMU_VIRTIO,
 } VirtIOMMUType;
 
+typedef enum VirtMSIControllerType {
+VIRT_MSI_CTRL_NONE,
+VIRT_MSI_CTRL_GICV2M,
+VIRT_MSI_CTRL_ITS,
+} VirtMSIControllerType;
+
 typedef enum VirtGICType {
 VIRT_GIC_VERSION_MAX,
 VIRT_GIC_VERSION_HOST,
@@ -136,6 +142,7 @@ typedef struct {
 OnOffAuto acpi;
 VirtGICType gic_version;
 VirtIOMMUType iommu;
+VirtMSIControllerType msi_controller;
 uint16_t virtio_iommu_bdf;
 struct arm_boot_info bootinfo;
 MemMapEntry *memmap;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cd0834ce7f..eb4344cf6e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -602,6 +602,7 @@ static void create_its(VirtMachineState *vms)
 sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, vms->memmap[VIRT_GIC_ITS].base);
 
 fdt_add_its_gic_node(vms);
+vms->msi_controller = VIRT_MSI_CTRL_ITS;
 }
 
 static void create_v2m(VirtMachineState *vms)
@@ -622,6 +623,7 @@ static void create_v2m(VirtMachineState *vms)
 }
 
 fdt_add_v2m_gic_node(vms);
+vms->msi_controller = VIRT_MSI_CTRL_GICV2M;
 }
 
 static void create_gic(VirtMachineState *vms)
@@ -2200,8 +2202,36 @@ out:
 static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
 DeviceState *dev, Error **errp)
 {
+VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
 virt_memory_pre_plug(hotplug_dev, dev, errp);
+} else if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
+hwaddr db_start = 0, db_end = 0;
+char *resv_prop_str;
+
+switch (vms->msi_controller) {
+case VIRT_MSI_CTRL_NONE:
+return;
+case VIRT_MSI_CTRL_ITS:
+/* GITS_TRANSLATER page */
+db_start = base_memmap[VIRT_GIC_ITS].base + 0x1;
+db_end = base_memmap[VIRT_GIC_ITS].base +
+ base_memmap[VIRT_GIC_ITS].size - 1;
+break;
+case VIRT_MSI_CTRL_GICV2M:
+/* MSI_SETSPI_NS page */
+db_start = base_memmap[VIRT_GIC_V2M].base;
+db_end = db_start + base_memmap[VIRT_GIC_V2M].size - 1;
+break;
+}
+resv_prop_str = g_strdup_printf("0x%"PRIx64":0x%"PRIx64":%u",
+db_start, db_end,
+VIRTIO_IOMMU_RESV_MEM_T_MSI);
+
+qdev_prop_set_uint32(dev, "len-reserved-regions", 1);
+qdev_prop_set_string(dev, "reserved-regions[0]", resv_prop_str);
+g_free(resv_prop_str);
 }
 }
 
-- 
2.20.1

[PATCH v7 4/5] virtio-iommu-pci: Add array of Interval properties

[Eric Auger]

The machine may need to pass reserved regions to the
virtio-iommu-pci device (such as the MSI window on x86
or the MSI doorbells on ARM).

So let's add an array of Interval properties.

Note: if some reserved regions are already set by the
machine code - which should be the case in general -,
the length of the property array is already set and
prevents the end-user from modifying them. For example,
attempting to use:

-device virtio-iommu-pci,\
 len-reserved-regions=1,reserved-regions[0]=0xfee0:0xfeef:1

would result in the following error message:

qemu-system-aarch64: -device virtio-iommu-pci,addr=0xa,
len-reserved-regions=1,reserved-regions[0]=0xfee0:0xfeef:1:
array size property len-reserved-regions may not be set more than once

Otherwise, for example, adding two reserved regions is achieved
using the following options:

-device virtio-iommu-pci,addr=0xa,len-reserved-regions=2,\
 reserved-regions[0]=0xfee0:0xfeef:1,\
 reserved-regions[1]=0x100:100:1

Signed-off-by: Eric Auger 
Reviewed-by: Markus Armbruster 

---
v6 -> v7:
- fix hint message with correct valid values, ie. 0 and 1
- added Markus' R-b

v5 -> v6:
- check the type value
- removed Jean's R-b

v3 -> v4:
- added examples in the commit message as suggested by Markus
- added Jean's R-b
---
 hw/virtio/virtio-iommu-pci.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
index 632533abaf..33f3f5b03e 100644
--- a/hw/virtio/virtio-iommu-pci.c
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -33,6 +33,9 @@ struct VirtIOIOMMUPCI {
 
 static Property virtio_iommu_pci_properties[] = {
 DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
+DEFINE_PROP_ARRAY("reserved-regions", VirtIOIOMMUPCI,
+  vdev.nb_reserved_regions, vdev.reserved_regions,
+  qdev_prop_reserved_region, ReservedRegion),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -40,6 +43,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 {
 VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev);
 DeviceState *vdev = DEVICE(&dev->vdev);
+VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
 
 if (!qdev_get_machine_hotplug_handler(DEVICE(vpci_dev))) {
 MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
@@ -54,6 +58,13 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
   "-no-acpi\n");
 return;
 }
+for (int i = 0; i < s->nb_reserved_regions; i++) {
+if (s->reserved_regions[i].type != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
+s->reserved_regions[i].type != VIRTIO_IOMMU_RESV_MEM_T_MSI) {
+error_setg(errp, "reserved region %d has an invalid type", i);
+error_append_hint(errp, "Valid values are 0 and 1\n");
+}
+}
 object_property_set_link(OBJECT(dev),
  OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
  "primary-bus", errp);
-- 
2.20.1

[PATCH v7 2/5] virtio-iommu: Implement RESV_MEM probe request

[Eric Auger]

This patch implements the PROBE request. At the moment,
only THE RESV_MEM property is handled. The first goal is
to report iommu wide reserved regions such as the MSI regions
set by the machine code. On x86 this will be the IOAPIC MSI
region, [0xFEE0 - 0xFEEF], on ARM this may be the ITS
doorbell.

In the future we may introduce per device reserved regions.
This will be useful when protecting host assigned devices
which may expose their own reserved regions

Signed-off-by: Eric Auger 
Reviewed-by: Jean-Philippe Brucker 

---

v6 -> v7:
- put the assert again to make it clear there is no risk
  of truncation

v5 -> v6:
- removed validation of s->reserved_regions[i].type in the
  probe request as it should rather happen in the realize()

v4 -> v5:
- assert if reserved region type is different from RESERVED or
  MSI

v3 -> v4:
- removed any reference to the NONE property that does not
  exist anymore.

v2 -> v3:
- on probe, do not fill the reminder of the buffer with zeroes
  as the buffer was already zero initialized (Bharat)

v1 -> v2:
- move the unlock back to the same place
- remove the push label and factorize the code after the out label
- fix a bunch of cpu_to_leX according to the latest spec revision
- do not remove sizeof(last) from free space
- check the ep exists
---
 include/hw/virtio/virtio-iommu.h |  2 +
 hw/virtio/virtio-iommu.c | 94 ++--
 hw/virtio/trace-events   |  1 +
 3 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index e653004d7c..49eb105cd8 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -53,6 +53,8 @@ typedef struct VirtIOIOMMU {
 GHashTable *as_by_busptr;
 IOMMUPciBus *iommu_pcibus_by_bus_num[PCI_BUS_MAX];
 PCIBus *primary_bus;
+ReservedRegion *reserved_regions;
+uint32_t nb_reserved_regions;
 GTree *domains;
 QemuMutex mutex;
 GTree *endpoints;
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 483883ec1d..2cdaa1969b 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -38,6 +38,7 @@
 
 /* Max size */
 #define VIOMMU_DEFAULT_QUEUE_SIZE 256
+#define VIOMMU_PROBE_SIZE 512
 
 typedef struct VirtIOIOMMUDomain {
 uint32_t id;
@@ -378,6 +379,65 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
 return ret;
 }
 
+static ssize_t virtio_iommu_fill_resv_mem_prop(VirtIOIOMMU *s, uint32_t ep,
+   uint8_t *buf, size_t free)
+{
+struct virtio_iommu_probe_resv_mem prop = {};
+size_t size = sizeof(prop), length = size - sizeof(prop.head), total;
+int i;
+
+total = size * s->nb_reserved_regions;
+
+if (total > free) {
+return -ENOSPC;
+}
+
+for (i = 0; i < s->nb_reserved_regions; i++) {
+unsigned subtype = s->reserved_regions[i].type;
+
+assert(subtype == VIRTIO_IOMMU_RESV_MEM_T_RESERVED ||
+   subtype == VIRTIO_IOMMU_RESV_MEM_T_MSI);
+prop.head.type = cpu_to_le16(VIRTIO_IOMMU_PROBE_T_RESV_MEM);
+prop.head.length = cpu_to_le16(length);
+prop.subtype = subtype;
+prop.start = cpu_to_le64(s->reserved_regions[i].low);
+prop.end = cpu_to_le64(s->reserved_regions[i].high);
+
+memcpy(buf, &prop, size);
+
+trace_virtio_iommu_fill_resv_property(ep, prop.subtype,
+  prop.start, prop.end);
+buf += size;
+}
+return total;
+}
+
+/**
+ * virtio_iommu_probe - Fill the probe request buffer with
+ * the properties the device is able to return
+ */
+static int virtio_iommu_probe(VirtIOIOMMU *s,
+  struct virtio_iommu_req_probe *req,
+  uint8_t *buf)
+{
+uint32_t ep_id = le32_to_cpu(req->endpoint);
+size_t free = VIOMMU_PROBE_SIZE;
+ssize_t count;
+
+if (!virtio_iommu_mr(s, ep_id)) {
+return VIRTIO_IOMMU_S_NOENT;
+}
+
+count = virtio_iommu_fill_resv_mem_prop(s, ep_id, buf, free);
+if (count < 0) {
+return VIRTIO_IOMMU_S_INVAL;
+}
+buf += count;
+free -= count;
+
+return VIRTIO_IOMMU_S_OK;
+}
+
 static int virtio_iommu_iov_to_req(struct iovec *iov,
unsigned int iov_cnt,
void *req, size_t req_sz)
@@ -407,15 +467,27 @@ virtio_iommu_handle_req(detach)
 virtio_iommu_handle_req(map)
 virtio_iommu_handle_req(unmap)
 
+static int virtio_iommu_handle_probe(VirtIOIOMMU *s,
+ struct iovec *iov,
+ unsigned int iov_cnt,
+ uint8_t *buf)
+{
+struct virtio_iommu_req_probe req;
+int ret = virtio_iommu_iov_to_req(iov, iov_cnt, &req, sizeof(req));
+
+return ret ? ret : virtio_iommu_probe(s, &req, buf);
+}
+
 static void virtio_iommu_handle_command(VirtIODevice *vdev, 

[PATCH v7 1/5] qdev: Introduce DEFINE_PROP_RESERVED_REGION

[Eric Auger]

Introduce a new property defining a reserved region:
::.

This will be used to encode reserved IOVA regions.

For instance, in virtio-iommu use case, reserved IOVA regions
will be passed by the machine code to the virtio-iommu-pci
device (an array of those). The type of the reserved region
will match the virtio_iommu_probe_resv_mem subtype value:
- VIRTIO_IOMMU_RESV_MEM_T_RESERVED (0)
- VIRTIO_IOMMU_RESV_MEM_T_MSI (1)

on PC/Q35 machine, this will be used to inform the
virtio-iommu-pci device it should bypass the MSI region.
The reserved region will be: 0xfee0:0xfeef:1.

On ARM, we can declare the ITS MSI doorbell as an MSI
region to prevent MSIs from being mapped on guest side.

Signed-off-by: Eric Auger 
Reviewed-by: Markus Armbruster 
Reviewed-by: Michael S. Tsirkin 

---

v4 -> v5:
- type becomes an unsigned + comment/error rewording

v3 -> v4:
- use ':' instead of commas as separators.
- rearrange error messages
- check snprintf returned value
- dared to keep Markus' R-b despite those changes
---
 include/exec/memory.h|  6 +++
 include/hw/qdev-properties.h |  3 ++
 include/qemu/typedefs.h  |  1 +
 hw/core/qdev-properties.c| 89 
 4 files changed, 99 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 7207025bd4..84ee5b7a01 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -51,6 +51,12 @@ extern bool global_dirty_log;
 
 typedef struct MemoryRegionOps MemoryRegionOps;
 
+struct ReservedRegion {
+hwaddr low;
+hwaddr high;
+unsigned type;
+};
+
 typedef struct IOMMUTLBEntry IOMMUTLBEntry;
 
 /* See address_space_translate: bit 0 is read, bit 1 is write.  */
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 49c6cd2460..944e3f2e0c 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -19,6 +19,7 @@ extern const PropertyInfo qdev_prop_string;
 extern const PropertyInfo qdev_prop_chr;
 extern const PropertyInfo qdev_prop_tpm;
 extern const PropertyInfo qdev_prop_macaddr;
+extern const PropertyInfo qdev_prop_reserved_region;
 extern const PropertyInfo qdev_prop_on_off_auto;
 extern const PropertyInfo qdev_prop_multifd_compression;
 extern const PropertyInfo qdev_prop_losttickpolicy;
@@ -184,6 +185,8 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
 DEFINE_PROP(_n, _s, _f, qdev_prop_drive_iothread, BlockBackend *)
 #define DEFINE_PROP_MACADDR(_n, _s, _f) \
 DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
+#define DEFINE_PROP_RESERVED_REGION(_n, _s, _f) \
+DEFINE_PROP(_n, _s, _f, qdev_prop_reserved_region, ReservedRegion)
 #define DEFINE_PROP_ON_OFF_AUTO(_n, _s, _f, _d) \
 DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_on_off_auto, OnOffAuto)
 #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index ce4a78b687..15f5047bf1 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -58,6 +58,7 @@ typedef struct ISABus ISABus;
 typedef struct ISADevice ISADevice;
 typedef struct IsaDma IsaDma;
 typedef struct MACAddr MACAddr;
+typedef struct ReservedRegion ReservedRegion;
 typedef struct MachineClass MachineClass;
 typedef struct MachineState MachineState;
 typedef struct MemoryListener MemoryListener;
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 71f8aca7c6..ca7771f307 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -15,6 +15,7 @@
 #include "chardev/char.h"
 #include "qemu/uuid.h"
 #include "qemu/units.h"
+#include "qemu/cutils.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
   Error **errp)
@@ -578,6 +579,94 @@ const PropertyInfo qdev_prop_macaddr = {
 .set   = set_mac,
 };
 
+/* --- Reserved Region --- */
+
+/*
+ * Accepted syntax:
+ *   ::
+ *   where low/high addresses are uint64_t in hexadecimal
+ *   and type is a non-negative decimal integer
+ */
+static void get_reserved_region(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+DeviceState *dev = DEVICE(obj);
+Property *prop = opaque;
+ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
+char buffer[64];
+char *p = buffer;
+int rc;
+
+rc = snprintf(buffer, sizeof(buffer), "0x%"PRIx64":0x%"PRIx64":%u",
+  rr->low, rr->high, rr->type);
+assert(rc < sizeof(buffer));
+
+visit_type_str(v, name, &p, errp);
+}
+
+static void set_reserved_region(Object *obj, Visitor *v, const char *name,
+void *opaque, Error **errp)
+{
+DeviceState *dev = DEVICE(obj);
+Property *prop = opaque;
+ReservedRegion *rr = qdev_get_prop_ptr(dev, prop);
+Error *local_err = NULL;
+const char *endptr;
+char *str;
+int ret;
+
+if (dev->realized) {
+qdev_prop_set_after_realize(dev, name, errp);
+return;
+   

[PATCH v7 3/5] virtio-iommu: Handle reserved regions in the translation process

[Eric Auger]

When translating an address we need to check if it belongs to
a reserved virtual address range. If it does, there are 2 cases:

- it belongs to a RESERVED region: the guest should neither use
  this address in a MAP not instruct the end-point to DMA on
  them. We report an error

- It belongs to an MSI region: we bypass the translation.

Signed-off-by: Eric Auger 
Reviewed-by: Peter Xu 
Reviewed-by: Jean-Philippe Brucker 
Reviewed-by: Michael S. Tsirkin 

---

v1 -> v2:
- use addr when testing addr belongs to the reserved region
  and use a block local variable
---
 hw/virtio/virtio-iommu.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 2cdaa1969b..b39e836181 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -607,6 +607,7 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 uint32_t sid, flags;
 bool bypass_allowed;
 bool found;
+int i;
 
 interval.low = addr;
 interval.high = addr + 1;
@@ -640,6 +641,25 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 goto unlock;
 }
 
+for (i = 0; i < s->nb_reserved_regions; i++) {
+ReservedRegion *reg = &s->reserved_regions[i];
+
+if (addr >= reg->low && addr <= reg->high) {
+switch (reg->type) {
+case VIRTIO_IOMMU_RESV_MEM_T_MSI:
+entry.perm = flag;
+break;
+case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
+default:
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
+break;
+}
+goto unlock;
+}
+}
+
 if (!ep->domain) {
 if (!bypass_allowed) {
 error_report_once("%s %02x:%02x.%01x not attached to any domain",
-- 
2.20.1

[PATCH v2] util/qemu-option: Document the get_opt_value() function

[Philippe Mathieu-Daudé]

Coverity noticed commit 950c4e6c94 introduced a dereference before
null check in get_opt_value (CID1391003):

  In get_opt_value: All paths that lead to this null pointer
  comparison already dereference the pointer earlier (CWE-476)

We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
for the 'value' parameter".

Since this function is publicly exposed, it risks new users to do
the same error again. Avoid that documenting the 'value' argument
must not be NULL.

Signed-off-by: Philippe Mathieu-Daudé 
---
v2: Drop confuse comment (Damien Hedde)
---
 include/qemu/option.h | 13 +
 1 file changed, 13 insertions(+)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index eb4097889d..ac50d25774 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -28,6 +28,19 @@
 
 #include "qemu/queue.h"
 
+/**
+ * get_opt_value
+ * @p: a pointer to the option name, delimited by commas
+ * @value: a non-NULL pointer that will received the delimited options
+ *
+ * The @value char pointer will be allocated and filled with
+ * the delimited options.
+ *
+ * Returns the position of the comma delimiter/zero byte after the
+ * option name in @p.
+ * The memory pointer in @value must be released with a call to g_free()
+ * when no longer required.
+ */
 const char *get_opt_value(const char *p, char **value);
 
 void parse_option_size(const char *name, const char *value,
-- 
2.21.3

Re: [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct

[Philippe Mathieu-Daudé]

On 6/28/20 9:54 PM, Peter Maydell wrote:
> In bcm2835_fb_mbox_push(), Coverity complains (CID 1429989) that we
> pass a pointer to a local struct to another function without
> initializing all its fields.  This is a real bug:
> bcm2835_fb_reconfigure() copies the whole of our new BCM2385FBConfig
> struct into s->config, so any fields we don't initialize will corrupt
> the state of the device.
> 
> Copy the two fields which we don't want to update (pixo and alpha)
> from the existing config so we don't accidentally change them.
> 
> Fixes: cfb7ba983857e40e88
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
> Not sure why this wasn't a visible bug -- alpha isn't used,
> but if pixo changes from zero to non-zero we flip from
> RGB to BGR...
> ---
>  hw/display/bcm2835_fb.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index c6263808a27..7c0e5eef2d5 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -282,6 +282,10 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, 
> uint32_t value)
>  newconf.base = s->vcram_base | (value & 0xc000);
>  newconf.base += BCM2835_FB_OFFSET;
>  
> +/* Copy fields which we don't want to change from the existing config */
> +newconf.pixo = s->config.pixo;
> +newconf.alpha = s->config.alpha;
> +
>  bcm2835_fb_validate_config(&newconf);
>  
>  pitch = bcm2835_fb_get_pitch(&newconf);
> 

Re: [PULL 00/84] QOM patches for 2020-06-15

[Philippe Mathieu-Daudé]

On 6/27/20 5:11 PM, Peter Maydell wrote:
> On Sat, 27 Jun 2020 at 12:53, Markus Armbruster  wrote:
>> Peter Maydell  writes:
>>> Hi. I've just noticed that this commit added new global-scope
>>> functions to header files without documentation comments, eg
>>>
>>> bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
>>
>> They actually have doc comments: next to their definition, just like the
>> functions they replace.
> 
>>> Please could you provide some follow-up patches that document them?
>>> I don't think we have any hope of getting people to follow whatever
>>> the correct new way to create/configure/realize devices is if we
>>> don't document it :-(   [Concrete example: I now have no idea
>>> how this is supposed to work after this patchset.]
>>
>> Please check out my function comments, and if they leave you confused,
>> let's talk about improvements.
> 
> I will have a look at them, but we should move them (wholesale
> with other documentation comments for qdev) to the header files.
> (That we are having this discussion at all demonstrates why -- people
> don't look in the C files for API documentation of functions.)
> The headers are where the API that faces the rest of QEMU should
> be documented; comments in the C files are for people who care
> about the internals of the implementation. "New prototype in a header
> file should have a doc comment" is a standard part of my code review
> I apply to any code which I see going past. We absolutely have not
> been good about documenting our facing-the-rest-of-QEMU functions
> in the past but this is an area where I think we should be raising the bar.
> 
>> I'm content to use comment placement / formatting I dislike to make my
>> code blend in, but I'm not willing to do conversion work from a style I
>> like to style I dislike.  That's a job for someone who won't feel icky
>> afterwards :)
> 
> Fair enough. I'm happy to write some patches to consistently put
> all the qdev doc info into the headers.

As a start I can respin
https://www.mail-archive.com/qemu-devel@nongnu.org/msg593422.html if you
want.

Re: Crash when running Qemu.

[Philippe Mathieu-Daudé]

On 6/28/20 7:05 PM, Peter Maydell wrote:
> On Sun, 28 Jun 2020 at 17:46, Jean-Christophe DUBOIS
>  wrote:
>>
>> Le 28/06/2020 à 16:38, Peter Maydell a écrit :
>>> On Sun, 28 Jun 2020 at 11:56, Jean-Christophe DUBOIS
>>>  wrote:
 jcd@jcd-UX305CA:~/Projects/µCOS/work$
 ../../qemu/qemu/arm-softmmu/qemu-system-arm -machine mcimx6ul-evk -m
 128M -display none -serial stdio -kernel ./OS.elf
 double free or corruption (!prev)
 Abandon (core dumped)
>>> I can't repro using your command line but without the -kernel option,
>>> so it's probably specific to something your guest code is doing.
>>> I tested with git commit e7651153a8801dad6; which commit are you
>>> using?
>>
>> I was on master (553cf5d7c47bee05a3dec9461c1f8430316d516b)
>>
>> Reverting the June 23rd commit series on PCA9552 fixed the problem for me.
>>
>>> Can you provide either the elf file or a repro example that
>>> doesn't need it ?
>>
>> Please, find the OS.elf file as attachment.
> 
> Ah, thanks for tracking that down. Philippe, the valgrind
> error in Jean-Christophe's other email in this thread suggests
> something's wrong in the typeinfo or the class init for the
> pca9552 changes you've made -- would you mind having a look at it?

Oops... sure!

[Bug 1883268] Re: random errors on aarch64 when executing __aarch64_cas8_acq_rel

[Christophe Lyon]

Hi Richard,

Thanks for taking a look and confirming that you managed to reproduce the 
problem.
I forgot to mention that I'm using x86_64 hosts, not i686. I hope there are not 
two unrelated issues...

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

Title:
  random errors on aarch64 when executing __aarch64_cas8_acq_rel

Status in QEMU:
  In Progress

Bug description:
  Hello,

  Since I upgraded to qemu-5.0 when executing the GCC testsuite,
  I've noticed random failures of g++.dg/ext/sync-4.C.

  I'm attaching the source of the testcase, the binary executable and
  the qemu traces (huge, 111MB!) starting at main (with qemu-aarch64
  -cpu cortex-a57 -R 0 -d
  in_asm,int,exec,cpu,unimp,guest_errors,nochain)

  The traces where generated by a CI build, I built the executable
  manually but I expect it to be the same as the one executed by CI.

  In seems the problem occurs in f13, which leads to a call to abort()

  The preprocessed version of f13/t13 are as follows:
  static bool f13 (void *p) __attribute__ ((noinline));
  static bool f13 (void *p)
  {
return (__sync_bool_compare_and_swap((ditype*)p, 1, 2));
  }
  static void t13 ()
  {
try {
  f13(0);
}
catch (...) {
  return;
}
abort();
  }

  
  When looking at the execution traces at address 0x00400c9c, main calls f13, 
which in turn calls __aarch64_cas8_acq_rel (at 0x00401084)
  __aarch64_cas8_acq_rel returns to f13 (address 0x0040113c), then f13 returns 
to main (0x0040108c) which then calls abort (0x00400ca0)

  I'm not quite sure what's wrong :-(

  I've not noticed such random problems with native aarch64 hardware.

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

[PATCH] hw/misc/pca9552: Add missing TypeInfo::class_size field

[Philippe Mathieu-Daudé]

When adding the generic PCA955xClass in commit 736132e455, we
forgot to set the class_size field. Fill it now to avoid:

  (gdb) run -machine mcimx6ul-evk -m 128M -display none -serial stdio -kernel 
./OS.elf
  Starting program: ../../qemu/qemu/arm-softmmu/qemu-system-arm -machine 
mcimx6ul-evk -m 128M -display none -serial stdio -kernel ./OS.elf
  double free or corruption (!prev)
  Thread 1 "qemu-system-arm" received signal SIGABRT, Aborted.
  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
  (gdb) where
  #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
  #1  0x775d8859 in __GI_abort () at abort.c:79
  #2  0x776433ee in __libc_message
  (action=action@entry=do_abort, fmt=fmt@entry=0x7776d285 "%s\n")
  at ../sysdeps/posix/libc_fatal.c:155
  #3  0x7764b47c in malloc_printerr
  (str=str@entry=0x7776f690 "double free or corruption (!prev)")
  at malloc.c:5347
  #4  0x7764d12c in _int_free
  (av=0x7779eb80 , p=0x567a3990, have_lock=) at malloc.c:4317
  #5  0x55c906c3 in type_initialize_interface
  (ti=ti@entry=0x565b8f40, interface_type=0x56597ad0, 
parent_type=0x5662ca10) at qom/object.c:259
  #6  0x55c902da in type_initialize (ti=ti@entry=0x565b8f40)
  at qom/object.c:323
  #7  0x55c90d20 in type_initialize (ti=0x565b8f40)
  at qom/object.c:1028

  $ valgrind --track-origins=yes qemu-system-arm -M mcimx6ul-evk -m 128M 
-display none -serial stdio -kernel ./OS.elf
  ==77479== Memcheck, a memory error detector
  ==77479== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
  ==77479== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
  ==77479== Command: qemu-system-arm -M mcimx6ul-evk -m 128M -display none 
-serial stdio -kernel ./OS.elf
  ==77479==
  ==77479== Invalid write of size 2
  ==77479==at 0x6D8322: pca9552_class_init (pca9552.c:424)
  ==77479==by 0x844D1F: type_initialize (object.c:1029)
  ==77479==by 0x844D1F: object_class_foreach_tramp (object.c:1016)
  ==77479==by 0x4AE1057: g_hash_table_foreach (in 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.2)
  ==77479==by 0x8453A4: object_class_foreach (object.c:1038)
  ==77479==by 0x8453A4: object_class_get_list (object.c:1095)
  ==77479==by 0x556194: select_machine (vl.c:2416)
  ==77479==by 0x556194: qemu_init (vl.c:3828)
  ==77479==by 0x40AF9C: main (main.c:48)
  ==77479==  Address 0x583f108 is 0 bytes after a block of size 200 alloc'd
  ==77479==at 0x483DD99: calloc (in 
/usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==77479==by 0x4AF8D30: g_malloc0 (in 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.2)
  ==77479==by 0x844258: type_initialize.part.0 (object.c:306)
  ==77479==by 0x844D1F: type_initialize (object.c:1029)
  ==77479==by 0x844D1F: object_class_foreach_tramp (object.c:1016)
  ==77479==by 0x4AE1057: g_hash_table_foreach (in 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.6400.2)
  ==77479==by 0x8453A4: object_class_foreach (object.c:1038)
  ==77479==by 0x8453A4: object_class_get_list (object.c:1095)
  ==77479==by 0x556194: select_machine (vl.c:2416)
  ==77479==by 0x556194: qemu_init (vl.c:3828)
  ==77479==by 0x40AF9C: main (main.c:48)

Fixes: 736132e455 ("hw/misc/pca9552: Add generic PCA955xClass")
Reported-by: Jean-Christophe DUBOIS 
Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/misc/pca9552.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index 80caa9ec8f..68b574d084 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -410,6 +410,7 @@ static const TypeInfo pca955x_info = {
 .instance_init = pca955x_initfn,
 .instance_size = sizeof(PCA955xState),
 .class_init= pca955x_class_init,
+.class_size= sizeof(PCA955xClass),
 .abstract  = true,
 };
 
-- 
2.21.3

Re: [PATCH 3/4] nbd: make client_close synchronous

[Vladimir Sementsov-Ogievskiy]

25.06.2020 17:25, Vladimir Sementsov-Ogievskiy wrote:

client_close doesn't guarantee that client is closed: nbd_trip() keeps
reference to it. Let's wait for nbd_trip to finish.

Without this fix, the following crash is possible:

- export bitmap through unternal Qemu NBD server
- connect a client
- shutdown Qemu

On shutdown nbd_export_close_all is called, but it actually don't wait
for nbd_trip() to finish and to release its references. So, export is
not release, and exported bitmap remains busy, and on try to remove the
bitmap (which is part of bdrv_close()) the assertion fairs:

bdrv_release_dirty_bitmap_locked: Assertion `!bdrv_dirty_bitmap_busy(bitmap)' 
failed

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  nbd/server.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index 20754e9ebc..5e27a8d31a 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1419,6 +1419,8 @@ static void client_close(NBDClient *client, bool 
negotiated)
  qio_channel_shutdown(client->ioc, QIO_CHANNEL_SHUTDOWN_BOTH,
   NULL);
  
+AIO_WAIT_WHILE(client->exp->ctx, client->recv_coroutine);

+
  /* Also tell the client, so that they release their reference.  */
  if (client->close_fn) {
  client->close_fn(client, negotiated);
@@ -2450,6 +2452,7 @@ static coroutine_fn void nbd_trip(void *opaque)
  
  trace_nbd_trip();

  if (client->closing) {
+client->recv_coroutine = NULL;
  nbd_client_put(client);
  return;
  }



I have a doubt, isn't it possible to hang in AIO_WAIT_WHILE() after this patch?

Do we need aio_wait_kick() in the nbd_trip()? Or may be, something like
aio_wait_kick(), but to wake exactly client->exp->ctx aio context?

Also, why in block/io.c we kick the main context, but not bs->aio_context?


--
Best regards,
Vladimir

[Bug 1882065] Re: Could this cause OOB bug ?

[Thomas Huth]

Fix has been included:
https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ee760ac80ac1f1

** Changed in: qemu
   Status: New => Fix Committed

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

Title:
  Could this cause OOB bug ?

Status in QEMU:
  Fix Committed

Bug description:

  In function megasas_handle_scsi(hw/scsi/megasas.c):

  ```c
  static int megasas_handle_scsi(MegasasState *s, MegasasCmd *cmd,
 int frame_cmd)
  {
  

  cdb = cmd->frame->pass.cdb;
  target_id = cmd->frame->header.target_id;
  lun_id = cmd->frame->header.lun_id;
  cdb_len = cmd->frame->header.cdb_len;
  

  if (cdb_len > 16) {
  trace_megasas_scsi_invalid_cdb_len(
  mfi_frame_desc[frame_cmd], is_logical,
  target_id, lun_id, cdb_len);
  megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
  cmd->frame->header.scsi_status = CHECK_CONDITION;
  s->event_count++;
  return MFI_STAT_SCSI_DONE_WITH_ERROR;
  }
  }
  ```

  Two variables, frame_cmd and cdb_len, can be controlled by guest os.
  So can mfi_frame_desc[frame_cmd] cause OOB bug ?

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

[PATCH v2] virtio-balloon: always indicate S_DONE when migration fails

[David Hildenbrand]

If something goes wrong during precopy, before stopping the VM, we will
never send a S_DONE indication to the VM, resulting in the hinted pages
not getting released to be used by the guest OS (e.g., Linux).

Easy to reproduce:
1. Start migration (e.g., HMP "migrate -d 'exec:gzip -c > STATEFILE.gz'")
2. Cancel migration (e.g., HMP "migrate_cancel")
3. Oberve in the guest (e.g., cat /proc/meminfo) that there is basically
   no free memory left.

While at it, add similar locking to virtio_balloon_free_page_done() as
done in virtio_balloon_free_page_stop. Locking is still weird, but that
has to be sorted out separately.

There is nothing to do in the PRECOPY_NOTIFY_COMPLETE case. Add some
comments regarding S_DONE handling.

Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Reviewed-by: Alexander Duyck 
Cc: Wei Wang 
Cc: Alexander Duyck 
Signed-off-by: David Hildenbrand 
---
 hw/virtio/virtio-balloon.c | 26 --
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 10507b2a43..8a84718490 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -628,8 +628,13 @@ static void virtio_balloon_free_page_done(VirtIOBalloon *s)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
-virtio_notify_config(vdev);
+if (s->free_page_report_status != FREE_PAGE_REPORT_S_DONE) {
+/* See virtio_balloon_free_page_stop() */
+qemu_mutex_lock(&s->free_page_lock);
+s->free_page_report_status = FREE_PAGE_REPORT_S_DONE;
+qemu_mutex_unlock(&s->free_page_lock);
+virtio_notify_config(vdev);
+}
 }
 
 static int
@@ -653,17 +658,26 @@ virtio_balloon_free_page_report_notify(NotifierWithReturn 
*n, void *data)
 case PRECOPY_NOTIFY_SETUP:
 precopy_enable_free_page_optimization();
 break;
-case PRECOPY_NOTIFY_COMPLETE:
-case PRECOPY_NOTIFY_CLEANUP:
 case PRECOPY_NOTIFY_BEFORE_BITMAP_SYNC:
 virtio_balloon_free_page_stop(dev);
 break;
 case PRECOPY_NOTIFY_AFTER_BITMAP_SYNC:
 if (vdev->vm_running) {
 virtio_balloon_free_page_start(dev);
-} else {
-virtio_balloon_free_page_done(dev);
+break;
 }
+/*
+ * Set S_DONE before migrating the vmstate, so the guest will reuse
+ * all hinted pages once running on the destination. Fall through.
+ */
+case PRECOPY_NOTIFY_CLEANUP:
+/*
+ * Especially, if something goes wrong during precopy or if migration
+ * is canceled, we have to properly communicate S_DONE to the VM.
+ */
+virtio_balloon_free_page_done(dev);
+break;
+case PRECOPY_NOTIFY_COMPLETE:
 break;
 default:
 virtio_error(vdev, "%s: %d reason unknown", __func__, pnd->reason);
-- 
2.26.2

Re: [PULL 3/6] MAINTAINERS: Add Loongson-3 maintainer and reviewer

[Philippe Mathieu-Daudé]

On 6/27/20 9:51 PM, Aleksandar Markovic wrote:
> From: Huacai Chen 
> 
> Add myself as the maintainer for Loongson-3 virtual platforms, and
> also add Jiaxun Yang as the reviewer.
> 
> Signed-off-by: Huacai Chen 
> Co-developed-by: Jiaxun Yang 
> Reviewed-by: Aleksandar Markovic 
> Signed-off-by: Aleksandar Markovic 
> Message-Id: <1592995531-32600-5-git-send-email-che...@lemote.com>
> ---
>  MAINTAINERS | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b40446..fe925ea 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1096,6 +1096,13 @@ F: hw/isa/vt82c686.c
>  F: hw/pci-host/bonito.c
>  F: include/hw/isa/vt82c686.h
>  
> +Loongson-3 Virtual Platform
> +M: Huacai Chen 
> +R: Jiaxun Yang 
> +S: Maintained
> +F: hw/mips/loongson3_virt.c

This file has not been commited, is this pull request incomplete?

> +F: hw/intc/loongson_liointc.c

How can we test this device?

> +
>  Boston
>  M: Paul Burton 
>  R: Aleksandar Rikalo 
> 

Re: [PULL 6/6] MAINTAINERS: Add 'Performance Tools and Tests' subsection

[Philippe Mathieu-Daudé]

On 6/27/20 9:51 PM, Aleksandar Markovic wrote:
> This commit creates a new 'Miscellaneous' section which hosts a new
> 'Performance Tools and Tests' subsection. This subsection will contain
> the the performance scripts and benchmarks written as a part of the
> 'TCG Continuous Benchmarking' project. Also, it will be a placeholder
> for follow-ups to this project, if any.
> 
> Signed-off-by: Ahmed Karaman 
> Reviewed-by: Alex Bennée 
> Reviewed-by: Aleksandar Markovic 
> Signed-off-by: Aleksandar Markovic 
> Message-Id: <20200626164546.22102-4-ahmedkhaledkara...@gmail.com>
> ---
>  MAINTAINERS | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe925ea..dec252f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1096,11 +1096,10 @@ F: hw/isa/vt82c686.c
>  F: hw/pci-host/bonito.c
>  F: include/hw/isa/vt82c686.h
>  
> -Loongson-3 Virtual Platform
> +Loongson-3 virtual platforms
>  M: Huacai Chen 
>  R: Jiaxun Yang 
>  S: Maintained
> -F: hw/mips/loongson3_virt.c

Ah, now I see, here you unlist the uncommited file.

It might be easier to manage sending 2 different pull requests,
on for MIPS and one for the performance tools.

>  F: hw/intc/loongson_liointc.c
>  
>  Boston
> @@ -3026,3 +3025,10 @@ M: Peter Maydell 
>  S: Maintained
>  F: docs/conf.py
>  F: docs/*/conf.py
> +
> +Miscellaneous
> +-
> +Performance Tools and Tests
> +M: Ahmed Karaman 

Aleksandar, don't you want to be listed here with Ahmed?

> +S: Maintained
> +F: scripts/performance/
> 

Re: [PATCH v8 1/2] target/arm: kvm: Handle DABT with no valid ISS

[Andrew Jones]

On Sun, Jun 28, 2020 at 04:04:58PM +0100, Beata Michalska wrote:
> On ARMv7 & ARMv8 some load/store instructions might trigger a data abort
> exception with no valid ISS info to be decoded. The lack of decode info
> makes it at least tricky to emulate those instruction which is one of the
> (many) reasons why KVM will not even try to do so.
> 
> Add support for handling those by requesting KVM to inject external
> dabt into the quest.
> 
> Signed-off-by: Beata Michalska 
> ---
>  target/arm/kvm.c | 57 
> +++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index eef3bbd..2dd8a9a 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -39,6 +39,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>  
>  static bool cap_has_mp_state;
>  static bool cap_has_inject_serror_esr;
> +static bool cap_has_inject_ext_dabt;
>  
>  static ARMHostCPUFeatures arm_host_cpu_features;
>  
> @@ -245,6 +246,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>  ret = -EINVAL;
>  }
>  
> +if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
> +if (kvm_vm_enable_cap(s, KVM_CAP_ARM_NISV_TO_USER, 0)) {
> +error_report("Failed to enable KVM_CAP_ARM_NISV_TO_USER cap");
> +} else {
> +/* Set status for supporting the external dabt injection */
> +cap_has_inject_ext_dabt = kvm_check_extension(s,
> +KVM_CAP_ARM_INJECT_EXT_DABT);
> +}
> +}
> +
>  return ret;
>  }
>  
> @@ -810,6 +821,45 @@ void kvm_arm_vm_state_change(void *opaque, int running, 
> RunState state)
>  }
>  }
>  
> +/**
> + * kvm_arm_handle_dabt_nisv:
> + * @cs: CPUState
> + * @esr_iss: ISS encoding (limited) for the exception from Data Abort
> + *   ISV bit set to '0b0' -> no valid instruction syndrome
> + * @fault_ipa: faulting address for the synchronous data abort
> + *
> + * Returns: 0 if the exception has been handled, < 0 otherwise
> + */
> +static int kvm_arm_handle_dabt_nisv(CPUState *cs, uint64_t esr_iss,
> + uint64_t fault_ipa)

The indent of 'uint64_t fault_ipa' is off. Should be under CPUState.

> +{
> +/*
> + * Request KVM to inject the external data abort into the guest
> + */
> +if (cap_has_inject_ext_dabt) {
> +struct kvm_vcpu_events events = { };
> +/*
> + * The external data abort event will be handled immediately by KVM
> + * using the address fault that triggered the exit on given VCPU.
> + * Requesting injection of the external data abort does not rely
> + * on any other VCPU state. Therefore, in this particular case, the 
> VCPU
> + * synchronization can be exceptionally skipped.
> + */
> +events.exception.ext_dabt_pending = 1;
> +/*
> + * KVM_CAP_ARM_INJECT_EXT_DABT implies KVM_CAP_VCPU_EVENTS
> + */

Single line comments may be done with /* ... */ 

> +return kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events);
> +

Extra blank line here.

> +} else {
> +error_report("Data abort exception triggered by guest memory access "
> + "at physical address: 0x"  TARGET_FMT_lx,
> + (target_ulong)fault_ipa);
> +error_printf("KVM unable to emulate faulting instruction.\n");
> +}
> +return -1;
> +}
> +
>  int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>  {
>  int ret = 0;
> @@ -820,7 +870,12 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run 
> *run)
>  ret = EXCP_DEBUG;
>  } /* otherwise return to guest */
>  break;
> -default:
> +case KVM_EXIT_ARM_NISV:
> +/* External DABT with no valid iss to decode */
> +ret = kvm_arm_handle_dabt_nisv(cs, run->arm_nisv.esr_iss,
> +   run->arm_nisv.fault_ipa);
> +break;
> + default:

An extra space got added in front of 'default:'

>  qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
>__func__, run->exit_reason);
>  break;
> -- 
> 2.7.4
> 
> 

Besides the format changes

Reviewed-by: Andrew Jones 

[PATCH 2/3] target/nios2: in line the semantics of DISAS_UPDATE with other targets

[Wentong Wu]

In line the semantics of DISAS_UPDATE on nios2 target with other targets
which is to explicitly write the PC back into the cpu state before doing
a tcg_gen_exit_tb().

Signed-off-by: Wentong Wu 
---
 target/nios2/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index b052be85..83c10eb2 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -865,6 +865,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock 
*tb, int max_insns)
 /* Indicate where the next block should start */
 switch (dc->is_jmp) {
 case DISAS_NEXT:
+case DISAS_UPDATE:
 /* Save the current PC back into the CPU register */
 tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
 tcg_gen_exit_tb(NULL, 0);
@@ -872,7 +873,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock 
*tb, int max_insns)
 
 default:
 case DISAS_JUMP:
-case DISAS_UPDATE:
 /* The jump will already have updated the PC register */
 tcg_gen_exit_tb(NULL, 0);
 break;
-- 
2.21.3

[PATCH 1/3] target/nios2: add DISAS_NORETURN case for nothing more to generate

[Wentong Wu]

Add DISAS_NORETURN case for nothing more to generate because at runtime
execution will never return from some helper call. And at the same time
replace DISAS_UPDATE in t_gen_helper_raise_exception and gen_exception
with the newly added DISAS_NORETURN.

Signed-off-by: Wentong Wu 
---
 target/nios2/translate.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index e17656e6..b052be85 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -149,7 +149,7 @@ static void t_gen_helper_raise_exception(DisasContext *dc,
 tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc);
 gen_helper_raise_exception(dc->cpu_env, tmp);
 tcg_temp_free_i32(tmp);
-dc->is_jmp = DISAS_UPDATE;
+dc->is_jmp = DISAS_NORETURN;
 }
 
 static bool use_goto_tb(DisasContext *dc, uint32_t dest)
@@ -802,7 +802,7 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
 tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
 gen_helper_raise_exception(cpu_env, tmp);
 tcg_temp_free_i32(tmp);
-dc->is_jmp = DISAS_UPDATE;
+dc->is_jmp = DISAS_NORETURN;
 }
 
 /* generate intermediate code for basic block 'tb'.  */
@@ -877,6 +877,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock 
*tb, int max_insns)
 tcg_gen_exit_tb(NULL, 0);
 break;
 
+case DISAS_NORETURN:
 case DISAS_TB_JUMP:
 /* nothing more to generate */
 break;
-- 
2.21.3

[PATCH 3/3] target/nios2: Use gen_io_start around wrctl instruction

[Wentong Wu]

wrctl instruction on nios2 target will cause checking cpu
interrupt but tcg_handle_interrupt() will call cpu_abort()
if the CPU gets an interrupt while it's not in 'can do IO'
state, so add gen_io_start around wrctl instruction. Also
at the same time, end the onging TB with DISAS_UPDATE.

Signed-off-by: Wentong Wu 
---
 target/nios2/translate.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 83c10eb2..51347ada 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -32,6 +32,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/translator.h"
 #include "qemu/qemu-print.h"
+#include "exec/gen-icount.h"
 
 /* is_jmp field values */
 #define DISAS_JUMPDISAS_TARGET_0 /* only pc was modified dynamically */
@@ -518,7 +519,11 @@ static void wrctl(DisasContext *dc, uint32_t code, 
uint32_t flags)
 /* If interrupts were enabled using WRCTL, trigger them. */
 #if !defined(CONFIG_USER_ONLY)
 if ((instr.imm5 + CR_BASE) == CR_STATUS) {
+if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
+gen_io_start();
+}
 gen_helper_check_interrupts(dc->cpu_env);
+dc->is_jmp = DISAS_UPDATE;
 }
 #endif
 }
-- 
2.21.3

Re: [PATCH v8 2/2] target/arm: kvm: Handle misconfigured dabt injection

[Andrew Jones]

On Sun, Jun 28, 2020 at 04:04:59PM +0100, Beata Michalska wrote:
> Injecting external data abort through KVM might trigger
> an issue on kernels that do not get updated to include the KVM fix.
> For those and aarch32 guests, the injected abort gets misconfigured
> to be an implementation defined exception. This leads to the guest
> repeatedly re-running the faulting instruction.
> 
> Add support for handling that case.
> 
> [
>   Fixed-by: 018f22f95e8a
>   ('KVM: arm: Fix DFSR setting for non-LPAE aarch32 guests')
>   Fixed-by: 21aecdbd7f3a
>   ('KVM: arm: Make inject_abt32() inject an external abort instead')
> ]
> 
> Signed-off-by: Beata Michalska 

Not sure why you didn't pick up my a-b tag on this patch, as I had no
comments at all on it from the previous review. Actually, the last patch
could have picked up my r-b tag too, despite some comments needing
rework. Anyway, here's this tag again.

Acked-by: Andrew Jones 

> ---
>  target/arm/cpu.h |  2 ++
>  target/arm/kvm.c | 30 +-
>  target/arm/kvm32.c   | 34 ++
>  target/arm/kvm64.c   | 49 +
>  target/arm/kvm_arm.h | 10 ++
>  5 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 677584e..ed0ff09 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -570,6 +570,8 @@ typedef struct CPUARMState {
>  uint64_t esr;
>  } serror;
>  
> +uint8_t ext_dabt_raised; /* Tracking/verifying injection of ext DABT */
> +
>  /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
>  uint32_t irq_line_state;
>  
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 2dd8a9a..e7a596e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -749,6 +749,29 @@ int kvm_get_vcpu_events(ARMCPU *cpu)
>  
>  void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
>  {
> +ARMCPU *cpu = ARM_CPU(cs);
> +CPUARMState *env = &cpu->env;
> +
> +if (unlikely(env->ext_dabt_raised)) {
> +/*
> + * Verifying that the ext DABT has been properly injected,
> + * otherwise risking indefinitely re-running the faulting instruction
> + * Covering a very narrow case for kernels 5.5..5.5.4
> + * when injected abort was misconfigured to be
> + * an IMPLEMENTATION DEFINED exception (for 32-bit EL1)
> + */
> +if (!arm_feature(env, ARM_FEATURE_AARCH64) &&
> +unlikely(!kvm_arm_verify_ext_dabt_pending(cs))) {
> +
> +error_report("Data abort exception with no valid ISS generated 
> by "
> +   "guest memory access. KVM unable to emulate faulting "
> +   "instruction. Failed to inject an external data abort "
> +   "into the guest.");
> +abort();
> +   }
> +   /* Clear the status */
> +   env->ext_dabt_raised = 0;
> +}
>  }
>  
>  MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> @@ -833,6 +856,8 @@ void kvm_arm_vm_state_change(void *opaque, int running, 
> RunState state)
>  static int kvm_arm_handle_dabt_nisv(CPUState *cs, uint64_t esr_iss,
>   uint64_t fault_ipa)
>  {
> +ARMCPU *cpu = ARM_CPU(cs);
> +CPUARMState *env = &cpu->env;
>  /*
>   * Request KVM to inject the external data abort into the guest
>   */
> @@ -849,7 +874,10 @@ static int kvm_arm_handle_dabt_nisv(CPUState *cs, 
> uint64_t esr_iss,
>  /*
>   * KVM_CAP_ARM_INJECT_EXT_DABT implies KVM_CAP_VCPU_EVENTS
>   */
> -return kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events);
> +if (!kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events)) {
> +env->ext_dabt_raised = 1;
> +return 0;
> +}
>  
>  } else {
>  error_report("Data abort exception triggered by guest memory access "
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 7b3a19e..0af46b4 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -559,3 +559,37 @@ void kvm_arm_pmu_init(CPUState *cs)
>  {
>  qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
>  }
> +
> +#define ARM_REG_DFSR  ARM_CP15_REG32(0, 5, 0, 0)
> +#define ARM_REG_TTBCR ARM_CP15_REG32(0, 2, 0, 2)
> +/*
> + *DFSR:
> + *  TTBCR.EAE == 0
> + *  FS[4]   - DFSR[10]
> + *  FS[3:0] - DFSR[3:0]
> + *  TTBCR.EAE == 1
> + *  FS, bits [5:0]
> + */
> +#define DFSR_FSC(lpae, v) \
> +((lpae) ? ((v) & 0x3F) : (((v) >> 6) | ((v) & 0x1F)))
> +
> +#define DFSC_EXTABT(lpae) ((lpae) ? 0x10 : 0x08)
> +
> +bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
> +{
> +uint32_t dfsr_val;
> +
> +if (!kvm_get_one_reg(cs, ARM_REG_DFSR, &dfsr_val)) {
> +ARMCPU *cpu = ARM_CPU(cs);
> +CPUARMState *env = &cpu->env;
> +uint32_t ttbcr;
> +int lpae = 0;
> +
> +if (!kvm_get_one_reg(cs, ARM_REG_TTBCR, &ttbcr)) {
> +

Re: [PATCH v4 00/16] python: add mypy support to python/qemu

[Philippe Mathieu-Daudé]

+Cleber/Wainer.

On 6/26/20 10:41 PM, John Snow wrote:
> Based-on: 20200626202350.11060-1-js...@redhat.com
> 
> This series modifies the python/qemu library to comply with mypy --strict.
> This requires my "refactor shutdown" patch as a pre-requisite.
> 
> v4:
> 001/16:[] [--] 'python/qmp.py: Define common types'
> 002/16:[] [--] 'iotests.py: use qemu.qmp type aliases'
> 003/16:[] [--] 'python/qmp.py: re-absorb MonitorResponseError'
> 004/16:[] [--] 'python/qmp.py: Do not return None from cmd_obj'
> 005/16:[] [--] 'python/qmp.py: add casts to JSON deserialization'
> 006/16:[] [--] 'python/qmp.py: add QMPProtocolError'
> 007/16:[] [--] 'python/machine.py: Fix monitor address typing'
> 008/16:[] [--] 'python/machine.py: reorder __init__'
> 009/16:[] [--] 'python/machine.py: Don't modify state in _base_args()'
> 010/16:[] [--] 'python/machine.py: Handle None events in events_wait'
> 011/16:[] [--] 'python/machine.py: use qmp.command'
> 012/16:[0010] [FC] 'python/machine.py: Add _qmp access shim'
> 013/16:[] [-C] 'python/machine.py: fix _popen access'
> 014/16:[] [--] 'python/qemu: make 'args' style arguments immutable'
> 015/16:[] [--] 'iotests.py: Adjust HMP kwargs typing'
> 016/16:[] [-C] 'python/qemu: Add mypy type annotations'
> 
>  - Rebased on "refactor shutdown" v4
>  - Fixed _qmp access for scripts that disable QMP

See:

https://travis-ci.org/github/philmd/qemu/jobs/702507625#L5439

/ # uname -a
Linux buildroot 4.5.0-2-4kc-malta #1 Debian 4.5.5-1 (2016-05-29) mips
GNU/Linux
/ # reboot
/ # reboot: Restarting system
>>> {'execute': 'quit'}
qemu received signal 9; command: "mips-softmmu/qemu-system-mips -display
none -vga none -chardev
socket,id=mon,path=/tmp/tmpcojsc5u3/qemu-14540-monitor.sock -mon
chardev=mon,mode=control -machine malta -chardev
socket,id=console,path=/tmp/tmpcojsc5u3/qemu-14540-console.sock,server,nowait
-serial chardev:console -kernel
/tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpio/boot/vmlinux-4.5.0-2-4kc-malta
-initrd
/tmp/avocado_xgmck2k5/avocado_job_83wkzs2f/12-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_mips_malta_cpiorootfs.cpio
-append printk.time=0 console=ttyS0 console=tty rdinit=/sbin/init
noreboot -no-reboot"

Reproduced traceback from:
/home/travis/build/philmd/qemu/build/tests/venv/lib/python3.6/site-packages/avocado/core/test.py:886
Traceback (most recent call last):
  File
"/home/travis/build/philmd/qemu/build/tests/acceptance/avocado_qemu/__init__.py",
line 195, in tearDown
vm.shutdown()
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
457, in shutdown
self._do_shutdown(has_quit)
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
434, in _do_shutdown
self._soft_shutdown(has_quit, timeout)
  File "/home/travis/build/philmd/qemu/python/qemu/machine.py", line
414, in _soft_shutdown
self._qmp.cmd('quit')
  File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 271, in cmd
return self.cmd_obj(qmp_cmd)
  File "/home/travis/build/philmd/qemu/python/qemu/qmp.py", line 249, in
cmd_obj
self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
BrokenPipeError: [Errno 32] Broken pipe

Re: [PATCH 09/46] error: Avoid error_propagate() after migrate_add_blocker()

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

When migrate_add_blocker(blocker, &errp) is followed by
error_propagate(errp, err), we can often just as well do
migrate_add_blocker(..., errp).

Do that with this Coccinelle script:

 @@
 expression blocker, err, errp;
 expression ret;
 @@
 -ret = migrate_add_blocker(blocker, &err);
 -if (err) {
 +ret = migrate_add_blocker(blocker, errp);
 +if (ret < 0) {
 ... when != err;
 -error_propagate(errp, err);
 ...
 }

 @@
 expression blocker, err, errp;
 @@
 -migrate_add_blocker(blocker, &err);
 -if (err) {
 +if (migrate_add_blocker(blocker, errp) < 0) {
 ... when != err;
 -error_propagate(errp, err);
 ...
 }

Double-check @err is not used afterwards.  Dereferencing it would be
use after free, but checking whether it's null would be legitimate.

Signed-off-by: Markus Armbruster 
---
  block/parallels.c| 5 ++---
  block/qcow.c | 6 ++
  block/vdi.c  | 6 ++
  block/vhdx.c | 5 ++---
  block/vmdk.c | 6 ++
  block/vpc.c  | 5 ++---
  block/vvfat.c| 5 ++---
  hw/display/virtio-gpu-base.c | 5 +
  hw/intc/arm_gic_kvm.c| 4 +---
  hw/intc/arm_gicv3_its_kvm.c  | 4 +---
  hw/intc/arm_gicv3_kvm.c  | 4 +---
  hw/misc/ivshmem.c| 4 +---
  hw/scsi/vhost-scsi.c | 4 +---
  13 files changed, 20 insertions(+), 43 deletions(-)



[..]


diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index ad0ebabc87..87bc4aeca1 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -113,9 +113,7 @@ static void kvm_arm_its_realize(DeviceState *dev, Error 
**errp)


Remove unused local_err

with fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


  GITS_CTLR)) {
  error_setg(&s->migration_blocker, "This operating system kernel "
 "does not support vITS migration");
-migrate_add_blocker(s->migration_blocker, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
+if (migrate_add_blocker(s->migration_blocker, errp) < 0) {
  error_free(s->migration_blocker);
  return;
  }


[..]


--
Best regards,
Vladimir

Re: [PATCH 11/17] hw/misc/max111x: Create header file for documentation, TYPE_ macros

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Create a header file for the hw/misc/max111x device, in the
> usual modern style for QOM devices:
>  * definition of the TYPE_ constants and macros
>  * definition of the device's state struct so that it can
>be embedded in other structs if desired
>  * documentation of the interface
> 
> This allows us to use TYPE_MAX_ in the spitz.c code rather
> than the string "max".
> 
> Signed-off-by: Peter Maydell 
> ---
> To be honest the main driver here is that I wanted a header
> file to put the documentation comment in :-)
> ---
>  include/hw/misc/max111x.h | 57 +++
>  hw/arm/spitz.c|  3 ++-
>  hw/misc/max111x.c | 25 +
>  MAINTAINERS   |  1 +
>  4 files changed, 61 insertions(+), 25 deletions(-)
>  create mode 100644 include/hw/misc/max111x.h
> 
> diff --git a/include/hw/misc/max111x.h b/include/hw/misc/max111x.h
> new file mode 100644
> index 000..337ba63d588
> --- /dev/null
> +++ b/include/hw/misc/max111x.h
> @@ -0,0 +1,57 @@
> +/*
> + * Maxim MAX1110/ ADC chip emulation.
> + *
> + * Copyright (c) 2006 Openedhand Ltd.
> + * Written by Andrzej Zaborowski 
> + *
> + * This code is licensed under the GNU GPLv2.
> + *
> + * Contributions after 2012-01-13 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + */
> +
> +#ifndef HW_MISC_MAX111X_H
> +#define HW_MISC_MAX111X_H
> +
> +#include "hw/ssi/ssi.h"
> +#include "hw/irq.h"

"hw/irq.h" not needed as qemu_irq is forward-declared in
"qemu/typedefs.h".

> +
> +/*
> + * This is a model of the Maxim MAX1110/ ADC chip, which for QEMU
> + * is an SSI slave device. It has either 4 (max1110) or 8 (max)
> + * 8-bit ADC channels.
> + *
> + * QEMU interface:
> + *  + GPIO inputs 0..3 (for max1110) or 0..7 (for max): set the value
> + *of each ADC input, as an unsigned 8-bit value
> + *  + GPIO output 0: interrupt line
> + *  + Properties "input0" to "input3" (max1110) or "input0" to "input7"
> + *(max): initial reset values for ADC inputs.
> + *
> + * Known bugs:
> + *  + the interrupt line is not correctly implemented, and will never
> + *be lowered once it has been asserted.
> + */
> +typedef struct {
> +SSISlave parent_obj;
> +
> +qemu_irq interrupt;
> +/* Values of inputs at system reset (settable by QOM property) */
> +uint8_t reset_input[8];
> +
> +uint8_t tb1, rb2, rb3;
> +int cycle;
> +
> +uint8_t input[8];
> +int inputs, com;
> +} MAX111xState;
> +
> +#define TYPE_MAX_111X "max111x"
> +
> +#define MAX_111X(obj) \
> +OBJECT_CHECK(MAX111xState, (obj), TYPE_MAX_111X)

Nitpick, we can keep MAX_111X() + MAX111xState in "hw/misc/max111x.c"
until we get a consumer.

> +
> +#define TYPE_MAX_1110 "max1110"
> +#define TYPE_MAX_ "max"
> +
> +#endif
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index fa592aad6d6..1400a56729d 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -29,6 +29,7 @@
>  #include "audio/audio.h"
>  #include "hw/boards.h"
>  #include "hw/sysbus.h"
> +#include "hw/misc/max111x.h"
>  #include "migration/vmstate.h"
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
> @@ -732,7 +733,7 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>qdev_get_gpio_in(sms->mpu->gpio, 
> SPITZ_GPIO_TP_INT));
>  
>  bus = qdev_get_child_bus(sms->mux, "ssi2");
> -sms->max = qdev_new("max");
> +sms->max = qdev_new(TYPE_MAX_);
>  max = sms->max;
>  qdev_prop_set_uint8(sms->max, "input1" /* BATT_VOLT */,
>  SPITZ_BATTERY_VOLT);
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index 3a5cb838445..d41cfd92a8d 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -11,34 +11,11 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "hw/irq.h"

You still need "hw/irq.h" for qemu_irq_raise().

> -#include "hw/ssi/ssi.h"
> +#include "hw/misc/max111x.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
>  #include "hw/qdev-properties.h"
>  
> -typedef struct {
> -SSISlave parent_obj;
> -
> -qemu_irq interrupt;
> -/* Values of inputs at system reset (settable by QOM property) */
> -uint8_t reset_input[8];
> -
> -uint8_t tb1, rb2, rb3;
> -int cycle;
> -
> -uint8_t input[8];
> -int inputs, com;
> -} MAX111xState;
> -
> -#define TYPE_MAX_111X "max111x"
> -
> -#define MAX_111X(obj) \
> -OBJECT_CHECK(MAX111xState, (obj), TYPE_MAX_111X)
> -
> -#define TYPE_MAX_1110 "max1110"
> -#define TYPE_MAX_ "max"
> -
>  /* Control-byte bitfields */
>  #define CB_PD0   (1 << 0)
>  #define CB_PD1   (1 << 1)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1b40446c739..550844d1514 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -787,6 +787,7 @@ F: hw/gpio/max7310.c
>  F: hw/gpio/zaurus.c
>  F: hw/misc/mst_fpga.c
>  F: hw/misc/max11

Re: [PATCH v3 1/2] qom: Introduce object_property_try_add_child()

[Auger Eric]

Hi Markus

On 6/25/20 11:24 AM, Markus Armbruster wrote:
> Eric Auger  writes:
> 
>> object_property_add() does not allow object_property_try_add()
>> to gracefully fail as &error_abort is passed as an error handle.
>>
>> However such failure can easily be triggered from the QMP shell when,
>> for instance, one attempts to create an object with an id that already
>> exists. This is achived from the following call path:
>>
>> user_creatable_add_type -> object_property_add_child ->
>> object_property_add
>>
>> For instance, call twice:
>> object-add qom-type=memory-backend-ram id=mem1 props.size=1073741824
>> and QEMU aborts.
> 
> qmp_object_add -> user_creatable_add_dict -> user_creatable_add_type ->
> ...
OK
> 
>> This behavior is undesired as a user/management application mistake
>> in reusing a property ID shouldn't result in loss of the VM and live
>> data within.
>>
>> This patch introduces a new function, object_property_try_add_child()
>> which takes an error handle and turn object_property_try_add() into
>> a non-static one.
>>
>> Now the call path becomes:
>>
>> user_creatable_add_type -> object_property_try_add_child ->
>> object_property_try_add
>>
>> and the error is returned gracefully to the QMP client.
>>
>> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
>> {"return": {}}
>> (QEMU) object-add qom-type=memory-backend-ram id=mem2  props.size=4294967296
>> {"error": {"class": "GenericError", "desc": "attempt to add duplicate 
>> property
>> 'mem2' to object (type 'container')"}}
> 
> What's this?  qmp-shell?
yes qmp-shell, I will add this info
> 
>>
>> Signed-off-by: Eric Auger 
>> Fixes: d2623129a7de ("qom: Drop parameter @errp of object_property_add() & 
>> friends")
>>
>> ---
>>
>> v2 -> v3:
>> - don't take the object reference on failure in
>>   object_property_try_add_child
>> ---
>>  include/qom/object.h| 24 ++--
>>  qom/object.c| 21 -
>>  qom/object_interfaces.c |  7 +--
>>  3 files changed, 43 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 94a61ccc3f..91cf058d86 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1039,7 +1039,7 @@ Object *object_ref(Object *obj);
>>  void object_unref(Object *obj);
>>  
>>  /**
>> - * object_property_add:
>> + * object_property_try_add:
>>   * @obj: the object to add a property to
>>   * @name: the name of the property.  This can contain any character except 
>> for
>>   *  a forward slash.  In general, you should use hyphens '-' instead of
>> @@ -1056,10 +1056,22 @@ void object_unref(Object *obj);
>>   *   meant to allow a property to free its opaque upon object
>>   *   destruction.  This may be NULL.
>>   * @opaque: an opaque pointer to pass to the callbacks for the property
>> + * @errp: error handle
> 
> We have several descriptions of @errp parameters in this file already,
> and you're inventing a new one :)
> 
> Suggest to pick one of the existing ones instead:
> 
> * @errp: a pointer to an Error that is filled if getting/setting fails.
> * @errp: If an error occurs, a pointer to an area to store the error
> * @errp: pointer to error object
OK
> * @errp: returns an error if this function fails
> 
>>   *
>>   * Returns: The #ObjectProperty; this can be used to set the @resolve
>>   * callback for child and link properties.
>>   */
>> +ObjectProperty *object_property_try_add(Object *obj, const char *name,
>> +const char *type,
>> +ObjectPropertyAccessor *get,
>> +ObjectPropertyAccessor *set,
>> +ObjectPropertyRelease *release,
>> +void *opaque, Error **errp);
>> +
>> +/**
>> + * object_property_add: same as object_property_try_add with
>> + * errp hardcoded to &error_abort
>> + */
> 
> Style:
> 
>/**
> * object_property_add:
> * Same as object_property_try_add() with @errp hardcoded to
> * &error_abort.
> */
> 
> The line break after ':' matches the rest of the file (personally, I
> think the whole line is a complete waste then, but let's go with the
> flow).  The @ in @errp and the () in object_property_try_add() help
> tools with highlighting and linking.  Sentences start with a capital
> letter, and end with punctuation.
OK
> 
>>  ObjectProperty *object_property_add(Object *obj, const char *name,
>>  const char *type,
>>  ObjectPropertyAccessor *get,
>> @@ -1495,10 +1507,11 @@ Object *object_resolve_path_type(const char *path, 
>> const char *typename,
>>  Object *object_resolve_path_component(Object *parent, const char *part);
>>  
>>  /**
>> - * object_property_add_child:
>> + * object_property_try_add_child:
>>   * @obj: the object to add a property to
>>   * @name: the name of

Re: [PATCH v2 00/16] Crazy shit around -global (pardon my french)

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Cc: David for insurance against me spewing nonsense about migration.
> 
> John Snow  writes:
> 
> > On 6/25/20 12:45 AM, Markus Armbruster wrote:
> >> John Snow  writes:
> >> 
> >>> On 6/22/20 5:42 AM, Markus Armbruster wrote:
>  There are three ways to configure backends:
> 
>  * -nic, -serial, -drive, ... (onboard devices)
> 
>  * Set the property with -device, or, if you feel masochistic, with
>    -set device (pluggable devices)
> 
>  * Set the property with -global (both)
> 
>  The trouble is -global is terrible.
> 
>  It gets applied in object_new(), which can't fail.  We treat failure
>  to apply -global as fatal error, except when hot-plugging, where we
>  treat it as warning *boggle*.  I'm not addressing that today.
> 
>  Some code falls apart when you use both -global and the other way.
> 
>  To make life more interesting, we gave -drive two roles: with
>  interface type other than none, it's for configuring onboard devices,
>  and with interface type none, it's for defining backends for use with
>  -device and such.  Since we neglect to require interface type none for
>  the latter, you can use one -drive in both roles.  This confuses the
>  code about as much as you, dear reader, probably are by now.
> 
>  Because this still isn't interesting enough, there's yet another way
>  to configure backends, just for floppies: set the floppy controller's
>  property.  Goes back to the time when floppy wasn't a separate device,
>  and involves some Bad Magic.  Now -global can interact with itself!
> 
>  Digging through all this took me an embarrassing amount of time.
>  Hair, too.
> 
>  My patches reject some the silliest uses outright, and deprecate some
>  not so silly ones that have replacements.
> 
>  Apply on top of my "[PATCH v2 00/58] qdev: Rework how we plug into the
>  parent bus".
> 
> >>>
> >>> Oof. Thank you for your work in fixing our darkest corners. I sincerely
> >>> appreciate it.
> >>>
> >>> The qdev tree ordering problems don't cause any issues for migration, do
> >>> they?
> >> 
> >> This series should only change device configuration, not device state or
> >> its encoding in the migration stream.
> >> 
> >> I'm not sure what you mean by "qdev tree ordering problems".  Ist it
> >> commit e8c9e65816 'qom: Make "info qom-tree" show children sorted'?
> >> 
> >>> (I see you already sent a PR, so whatever!)
> >> 
> >> A question that might avoid a later migration debugging session is
> >> *never* "whatever"!
> >> 
> >
> > I thought I had read that one of these patches changes the order in
> > which devices get instantiated, which I thought might change their QOM
> > paths. Which I thought *might* have some ramifications for migration,
> > but wasn't sure.
> 
> Device instantiation order changes should not break migration.

They shouldn't; although I only narrowly stopped a new device from
making a mistake that would have made it dependent.
Of course you do have to explicitly state PCI/USB slot IDs otherwise the
allocation of those is order dependent.

> The order in which devices appear in the migration stream should not
> matter.

Order in the stream is a separate issue; we have ways to enforce that;
for example you want the interrupt controller to arrive before a device
that will raise an interrupt.

Dave


Dave

> > If it's just showing the same path outputs *sorted*, then there's no
> > problem.
> >
> > Likely misread.
> >
> > --js
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 04/19] iotests.py: Add qemu_img_pipe_and_status()

[Maxim Levitsky]

On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> This function will be used by the next patch, which intends to check
> both the exit code and qemu-img's output.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py | 34 +-
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5ea4c4df8b..eee94e18cc 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -29,7 +29,7 @@ import struct
>  import subprocess
>  import sys
>  from typing import (Any, Callable, Dict, Iterable,
> -List, Optional, Sequence, TypeVar)
> +List, Optional, Sequence, Tuple, TypeVar)
>  import unittest
>  
>  # pylint: disable=import-error, wrong-import-position
> @@ -90,15 +90,23 @@ luks_default_secret_object = 'secret,id=keysec0,data=' + \
>  luks_default_key_secret_opt = 'key-secret=keysec0'
>  
>  
> -def qemu_img(*args):
> -'''Run qemu-img and return the exit code'''
> -devnull = open('/dev/null', 'r+')
> -exitcode = subprocess.call(qemu_img_args + list(args),
> -   stdin=devnull, stdout=devnull)
> +def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
> +"""
> +Run qemu-img and return both its output and its exit code
> +"""
> +subp = subprocess.Popen(qemu_img_args + list(args),
> +stdout=subprocess.PIPE,
> +stderr=subprocess.STDOUT,
> +universal_newlines=True)
> +exitcode = subp.wait()
>  if exitcode < 0:
>  sys.stderr.write('qemu-img received signal %i: %s\n'
>   % (-exitcode, ' '.join(qemu_img_args + list(args
> -return exitcode
> +return (subp.communicate()[0], exitcode)
> +
> +def qemu_img(*args: str) -> int:
> +'''Run qemu-img and return the exit code'''
> +return qemu_img_pipe_and_status(*args)[1]
>  
>  def ordered_qmp(qmsg, conv_keys=True):
>  # Dictionaries are not ordered prior to 3.6, therefore:
> @@ -140,17 +148,9 @@ def qemu_img_verbose(*args):
>   % (-exitcode, ' '.join(qemu_img_args + list(args
>  return exitcode
>  
> -def qemu_img_pipe(*args):
> +def qemu_img_pipe(*args: str) -> str:
>  '''Run qemu-img and return its output'''
> -subp = subprocess.Popen(qemu_img_args + list(args),
> -stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT,
> -universal_newlines=True)
> -exitcode = subp.wait()
> -if exitcode < 0:
> -sys.stderr.write('qemu-img received signal %i: %s\n'
> - % (-exitcode, ' '.join(qemu_img_args + list(args
> -return subp.communicate()[0]
> +return qemu_img_pipe_and_status(*args)[0]
>  
>  def qemu_img_log(*args):
>  result = qemu_img_pipe(*args)

You made me learn a bit about python type hints, and I don't regret it :-)
Looks OK.

Reviewed-by: Maxim Levitsky 

Best regards,
Maxim Levitsky

Re: [PATCH 01/17] hw/arm/spitz: Detabify

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The spitz board has been around a long time, and still has a fair number
> of hard-coded tab characters in it. We're about to do some work on
> this source file, so start out by expanding out the tabs.
> 
> This commit is a pure whitespace only change.
> 
> Signed-off-by: Peter Maydell 
> ---
> Couple of checkpatch errors due to the QUEUE_KEY macro which can
> be ignored as this is just a detabify.
> ---
>  hw/arm/spitz.c | 156 -
>  1 file changed, 78 insertions(+), 78 deletions(-)

'git-diff -w' -> no change.

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 02/17] hw/arm/spitz: Create SpitzMachineClass abstract base class

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> For the four Spitz-family machines (akita, borzoi, spitz, terrier)
> create a proper abstract class SpitzMachineClass which encapsulates
> the common behaviour, rather than having them all derive directly
> from TYPE_MACHINE:
>  * instead of each machine class setting mc->init to a wrapper
>function which calls spitz_common_init() with parameters,
>put that data in the SpitzMachineClass and make spitz_common_init
>the SpitzMachineClass machine-init function
>  * move the settings of mc->block_default_type and
>mc->ignore_memory_transaction_failures into the SpitzMachineClass
>class init rather than repeating them in each machine's class init
> 
> (The motivation is that we're going to want to keep some state in
> the SpitzMachineState so we can connect GPIOs between devices created
> in one sub-function of the machine init to devices created in a
> different sub-function.)
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/arm/spitz.c | 91 ++
>  1 file changed, 55 insertions(+), 36 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 9eaedab79b5..c70e912a33d 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -33,6 +33,26 @@
>  #include "exec/address-spaces.h"
>  #include "cpu.h"
>  
> +enum spitz_model_e { spitz, akita, borzoi, terrier };
> +
> +typedef struct {
> +MachineClass parent;
> +enum spitz_model_e model;

Nitpick, I'd drop the not very useful typedef and use
directly ...:

   enum { spitz, akita, borzoi, terrier } model

> +int arm_id;
> +} SpitzMachineClass;
> +
> +typedef struct {
> +MachineState parent;
> +} SpitzMachineState;
> +
> +#define TYPE_SPITZ_MACHINE "spitz-common"
> +#define SPITZ_MACHINE(obj) \
> +OBJECT_CHECK(SpitzMachineState, obj, TYPE_SPITZ_MACHINE)
> +#define SPITZ_MACHINE_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(SpitzMachineClass, obj, TYPE_SPITZ_MACHINE)
> +#define SPITZ_MACHINE_CLASS(klass) \
> +OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
> +
>  #undef REG_FMT
>  #define REG_FMT "0x%02lx"
>  
> @@ -905,8 +925,6 @@ static void spitz_gpio_setup(PXA2xxState *cpu, int slots)
>  }
>  
>  /* Board init.  */
> -enum spitz_model_e { spitz, akita, borzoi, terrier };
> -
>  #define SPITZ_RAM   0x0400
>  #define SPITZ_ROM   0x0080
>  
> @@ -915,9 +933,10 @@ static struct arm_boot_info spitz_binfo = {
>  .ram_size = 0x0400,
>  };
>  
> -static void spitz_common_init(MachineState *machine,
> -  enum spitz_model_e model, int arm_id)
> +static void spitz_common_init(MachineState *machine)
>  {
> +SpitzMachineClass *smc = SPITZ_MACHINE_GET_CLASS(machine);
> +enum spitz_model_e model = smc->model;

... and use 'smc->model' in place.

Patch easier to review with 'git-diff -W' [*].

Reviewed-by: Philippe Mathieu-Daudé 

[*] Content of my .gitattributes:

$ cat .gitattributes
*.c diff=c
*.cpp   diff=cpp
*.m text diff=objc
*.h diff=c
*.pydiff=python
*.json  text
*.pltext diff=perl
*.shtext eol=lf
*.xml   text
*.yml   text
*.bz2   binary

Re: [PATCH 06/17] hw/misc/max111x: provide QOM properties for setting initial values

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Add some QOM properties to the max111x ADC device to allow the
> initial values to be configured. Currently this is done by
> board code calling max111x_set_input() after it creates the
> device, which doesn't work on system reset.
> 
> This requires us to implement a reset method for this device,
> so while we're doing that make sure we reset the other parts
> of the device state.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/misc/max111x.c | 57 ++-
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index 2b87bdee5b7..d0e5534e4f5 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -15,11 +15,15 @@
>  #include "hw/ssi/ssi.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> +#include "hw/qdev-properties.h"
>  
>  typedef struct {
>  SSISlave parent_obj;
>  
>  qemu_irq interrupt;
> +/* Values of inputs at system reset (settable by QOM property) */
> +uint8_t reset_input[8];
> +
>  uint8_t tb1, rb2, rb3;
>  int cycle;
>  

An eventual improvement is to make 'inputs' a class property:

  MAX111xClass {
  SSISlaveClass parent_obj;

  unsigned input_count;
  }

"eventual" because from a QOM point it is cleaner, but we'd have
to add more boiler-plate casts, so code less clear.

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 07/17] hw/misc/max111x: Don't use vmstate_register()

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The max111x is a proper qdev device; we can use dc->vmsd rather than
> directly calling vmstate_register().
> 
> It's possible that this is a migration compat break, but the only
> boards that use this device are the spitz-family ('akita', 'borzoi',
> 'spitz', 'terrier').
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/misc/max111x.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/misc/max111x.c b/hw/misc/max111x.c
> index d0e5534e4f5..abddfa3c660 100644
> --- a/hw/misc/max111x.c
> +++ b/hw/misc/max111x.c
> @@ -140,8 +140,6 @@ static int max111x_init(SSISlave *d, int inputs)
>  
>  s->inputs = inputs;
>  
> -vmstate_register(VMSTATE_IF(dev), VMSTATE_INSTANCE_ID_ANY,
> - &vmstate_max111x, s);
>  return 0;
>  }
>  
> @@ -206,6 +204,7 @@ static void max111x_class_init(ObjectClass *klass, void 
> *data)
>  
>  k->transfer = max111x_transfer;
>  dc->reset = max111x_reset;
> +dc->vmsd = &vmstate_max111x;
>  }
>  
>  static const TypeInfo max111x_info = {
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2] util/qemu-option: Document the get_opt_value() function

[Daniel P . Berrangé]

On Mon, Jun 29, 2020 at 09:08:58AM +0200, Philippe Mathieu-Daudé wrote:
> Coverity noticed commit 950c4e6c94 introduced a dereference before
> null check in get_opt_value (CID1391003):
> 
>   In get_opt_value: All paths that lead to this null pointer
>   comparison already dereference the pointer earlier (CWE-476)
> 
> We fixed this in commit 6e3ad3f0e31, but relaxed the check in commit
> 0c2f6e7ee99 because "No callers of get_opt_value() pass in a NULL
> for the 'value' parameter".
> 
> Since this function is publicly exposed, it risks new users to do
> the same error again. Avoid that documenting the 'value' argument
> must not be NULL.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> v2: Drop confuse comment (Damien Hedde)
> ---
>  include/qemu/option.h | 13 +
>  1 file changed, 13 insertions(+)

Reviewed-by: Daniel P. Berrangé 


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

Re: [PATCH 08/17] ssi: Add ssi_realize_and_unref()

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Add an ssi_realize_and_unref(), for the benefit of callers
> who want to be able to create an SSI device, set QOM properties
> on it, and then do the realize-and-unref afterwards.
> 
> The API works on the same principle as the recently added
> qdev_realize_and_undef(), sysbus_realize_and_undef(), etc.
> 
> Signed-off-by: Peter Maydell 
> ---
>  include/hw/ssi/ssi.h | 26 ++
>  hw/ssi/ssi.c |  7 ++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 93f2b8b0beb..4be5325e654 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -79,6 +79,32 @@ extern const VMStateDescription vmstate_ssi_slave;
>  }
>  
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name);
> +/**
> + * ssi_realize_and_unref: realize and unref an SSI slave device
> + * @dev: SSI slave device to realize
> + * @bus: SSI bus to put it on
> + * @errp: error pointer
> + *
> + * Call 'realize' on @dev, put it on the specified @bus, and drop the
> + * reference to it. Errors are reported via @errp and by returning
> + * false.
> + *
> + * This function is useful if you have created @dev via qdev_new()
> + * (which takes a reference to the device it returns to you), so that
> + * you can set properties on it before realizing it. If you don't need
> + * to set properties then ssi_create_slave() is probably better (as it
> + * does the create, init and realize in one step).
> + *
> + * If you are embedding the SSI slave into another QOM device and
> + * initialized it via some variant on object_initialize_child() then
> + * do not use this function, because that family of functions arrange
> + * for the only reference to the child device to be held by the parent
> + * via the child<> property, and so the reference-count-drop done here
> + * would be incorrect.  (Instead you would want ssi_realize(), which
> + * doesn't currently exist but would be trivial to create if we had
> + * any code that wanted it.)
> + */
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp);
>  
>  /* Master interface.  */
>  SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 67b48c31cd6..a35d7ebb266 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -90,11 +90,16 @@ static const TypeInfo ssi_slave_info = {
>  .abstract = true,
>  };
>  
> +bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp)
> +{
> +return qdev_realize_and_unref(dev, &bus->parent_obj, errp);
> +}
> +
>  DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
>  {
>  DeviceState *dev = qdev_new(name);
>  
> -qdev_realize_and_unref(dev, &bus->parent_obj, &error_fatal);
> +ssi_realize_and_unref(dev, bus, &error_fatal);
>  return dev;
>  }
>  

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 09/17] hw/arm/spitz: Use max111x properties to set initial values

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Use the new max111x qdev properties to set the initial input
> values rather than calling max111x_set_input(); this means that
> on system reset the inputs will correctly return to their initial
> values.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/arm/spitz.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 11e413723f4..93a25edcb5b 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -731,11 +731,14 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>qdev_get_gpio_in(sms->mpu->gpio, 
> SPITZ_GPIO_TP_INT));
>  
>  bus = qdev_get_child_bus(sms->mux, "ssi2");
> -sms->max = ssi_create_slave(bus, "max");
> +sms->max = qdev_new("max");
>  max = sms->max;
> -max111x_set_input(sms->max, MAX_BATT_VOLT, SPITZ_BATTERY_VOLT);
> -max111x_set_input(sms->max, MAX_BATT_TEMP, 0);
> -max111x_set_input(sms->max, MAX_ACIN_VOLT, SPITZ_CHARGEON_ACIN);
> +qdev_prop_set_uint8(sms->max, "input1" /* BATT_VOLT */,
> +SPITZ_BATTERY_VOLT);
> +qdev_prop_set_uint8(sms->max, "input2" /* BATT_TEMP */, 0);
> +qdev_prop_set_uint8(sms->max, "input3" /* ACIN_VOLT */,
> +SPITZ_CHARGEON_ACIN);

Actually for arrays it would be nice to use:

DEFINE_PROP_ARRAY("input", MAX111xState, nr_inputs, reset_input,
  qdev_prop_uint8, uint8_t),

Then something like:

qdev_prop_set_uint8_indexed(sms->max, "input", 2 /*BATT_TEMP*/, 0);

Anyway,
Reviewed-by: Philippe Mathieu-Daudé 

> +ssi_realize_and_unref(sms->max, bus, &error_fatal);
>  
>  qdev_connect_gpio_out(sms->mpu->gpio, SPITZ_GPIO_LCDCON_CS,
>  qdev_get_gpio_in(sms->mux, 0));
> 

Re: [PATCH 10/46] qemu-option: Check return value instead of @err where convenient

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

Convert uses like

 opts = qemu_opts_create(..., &err);
 if (err) {
 ...
 }

to

 opts = qemu_opts_create(..., &err);
 if (!opts) {
 ...
 }

Eliminate error_propagate() that are now unnecessary.  Delete @err
that are now unused.

Signed-off-by: Markus Armbruster 
---
  block/parallels.c  |  4 ++--
  blockdev.c |  5 ++---
  qdev-monitor.c |  6 ++
  util/qemu-config.c | 10 --
  util/qemu-option.c | 12 
  5 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 860dbb80a2..b15c9ac28d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -823,8 +823,8 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
  }
  }
  
-opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, &local_err);

-if (local_err != NULL) {
+opts = qemu_opts_create(¶llels_runtime_opts, NULL, 0, errp);
+if (!opts) {
  goto fail_options;
  }


Honestly, I don't like this hunk. as already complicated code (crossing gotos) 
becomes more
complicated (add one more pattern to fail_options path: no-op error_propagate).

At least, we'll need a follow-up patch, refactoring parallels_open() to drop 
"fail_options"
label completely.

Still, it should work and the rest is fine, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 12/17] hw/arm/spitz: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Currently we have a free-floating set of IRQs and a function
> spitz_out_switch() which handle some miscellaneous GPIO lines for the
> spitz board.  Encapsulate this behaviour in a simple QOM device.
> 
> At this point we can finally remove the 'max' global, because the
> ADC battery-temperature value is now handled by the misc-gpio device
> writing the value to its outbound "adc-temp" GPIO, which the board
> code wires up to the appropriate inbound GPIO line on the max.
> 
> This commit also fixes Coverity issue CID 1421913 (which pointed out
> that the 'outsignals' in spitz_scoop_gpio_setup() were leaked),
> because it removes the use of the qemu_allocate_irqs() API from this
> code entirely.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/arm/spitz.c | 129 +
>  1 file changed, 87 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 1400a56729d..bab9968ccee 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -51,6 +51,7 @@ typedef struct {
>  DeviceState *max;
>  DeviceState *scp0;
>  DeviceState *scp1;
> +DeviceState *misc_gpio;
>  } SpitzMachineState;
>  
>  #define TYPE_SPITZ_MACHINE "spitz-common"
> @@ -658,8 +659,6 @@ static void spitz_lcdtg_realize(SSISlave *ssi, Error 
> **errp)
>  #define SPITZ_GPIO_MAX_CS   20
>  #define SPITZ_GPIO_TP_INT   11
>  
> -static DeviceState *max;
> -
>  /* "Demux" the signal based on current chipselect */
>  typedef struct {
>  SSISlave ssidev;
> @@ -695,18 +694,6 @@ static void corgi_ssp_gpio_cs(void *opaque, int line, 
> int level)
>  #define SPITZ_BATTERY_VOLT  0xd0/* About 4.0V */
>  #define SPITZ_CHARGEON_ACIN 0x80/* About 5.0V */
>  
> -static void spitz_adc_temp_on(void *opaque, int line, int level)
> -{
> -int batt_temp;
> -
> -if (!max)
> -return;
> -
> -batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> -
> -qemu_set_irq(qdev_get_gpio_in(max, MAX_BATT_TEMP), batt_temp);
> -}
> -
>  static void corgi_ssp_realize(SSISlave *d, Error **errp)
>  {
>  DeviceState *dev = DEVICE(d);
> @@ -734,7 +721,6 @@ static void spitz_ssp_attach(SpitzMachineState *sms)
>  
>  bus = qdev_get_child_bus(sms->mux, "ssi2");
>  sms->max = qdev_new(TYPE_MAX_);
> -max = sms->max;
>  qdev_prop_set_uint8(sms->max, "input1" /* BATT_VOLT */,
>  SPITZ_BATTERY_VOLT);
>  qdev_prop_set_uint8(sms->max, "input2" /* BATT_TEMP */, 0);
> @@ -810,27 +796,66 @@ static void spitz_akita_i2c_setup(PXA2xxState *cpu)
>  
>  /* Other peripherals */
>  
> -static void spitz_out_switch(void *opaque, int line, int level)
> +/*
> + * Encapsulation of some miscellaneous GPIO line behaviour for the Spitz 
> boards.
> + *
> + * QEMU interface:
> + *  + named GPIO inputs "green-led", "orange-led", "charging", "discharging":
> + *these currently just print messages that the line has been signalled
> + *  + named GPIO input "adc-temp-on": set to cause the battery-temperature
> + *value to be passed to the max111x ADC
> + *  + named GPIO output "adc-temp": the ADC value, to be wired up to the 
> max111x
> + */
> +#define TYPE_SPITZ_MISC_GPIO "spitz-misc-gpio"
> +#define SPITZ_MISC_GPIO(obj) \
> +OBJECT_CHECK(SpitzMiscGPIOState, (obj), TYPE_SPITZ_MISC_GPIO)
> +
> +typedef struct SpitzMiscGPIOState {
> +SysBusDevice parent_obj;
> +
> +qemu_irq adc_value;
> +} SpitzMiscGPIOState;
> +
> +static void spitz_misc_charging(void *opaque, int n, int level)
>  {
> -switch (line) {
> -case 0:
> -zaurus_printf("Charging %s.\n", level ? "off" : "on");
> -break;
> -case 1:
> -zaurus_printf("Discharging %s.\n", level ? "on" : "off");
> -break;
> -case 2:
> -zaurus_printf("Green LED %s.\n", level ? "on" : "off");
> -break;
> -case 3:
> -zaurus_printf("Orange LED %s.\n", level ? "on" : "off");
> -break;
> -case 6:
> -spitz_adc_temp_on(opaque, line, level);
> -break;
> -default:
> -g_assert_not_reached();
> -}
> +zaurus_printf("Charging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_discharging(void *opaque, int n, int level)
> +{
> +zaurus_printf("Discharging %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_green_led(void *opaque, int n, int level)
> +{
> +zaurus_printf("Green LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_orange_led(void *opaque, int n, int level)
> +{
> +zaurus_printf("Orange LED %s.\n", level ? "off" : "on");
> +}
> +
> +static void spitz_misc_adc_temp(void *opaque, int n, int level)
> +{
> +SpitzMiscGPIOState *s = SPITZ_MISC_GPIO(opaque);
> +int batt_temp = level ? SPITZ_BATTERY_TEMP : 0;
> +
> +qemu_set_irq(s->adc_value, batt_temp);
> +}
> +
> +static void spitz_

Re: [PATCH 13/17] hw/gpio/zaurus.c: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of logging guest accesses to invalid register offsets in this
> device using zaurus_printf() (which just prints to stderr), use the
> usual qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> Since this was the only use of the zaurus_printf() macro outside
> spitz.c, we can move the definition of that macro from sharpsl.h
> to spitz.c.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  include/hw/arm/sharpsl.h |  3 ---
>  hw/arm/spitz.c   |  3 +++
>  hw/gpio/zaurus.c | 12 +++-
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/arm/sharpsl.h b/include/hw/arm/sharpsl.h
> index 89e168fbff3..e986b28c527 100644
> --- a/include/hw/arm/sharpsl.h
> +++ b/include/hw/arm/sharpsl.h
> @@ -9,9 +9,6 @@
>  
>  #include "exec/hwaddr.h"
>  
> -#define zaurus_printf(format, ...)   \
> -fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> -
>  /* zaurus.c */
>  
>  #define SL_PXA_PARAM_BASE0xaa00
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index bab9968ccee..6eb46869157 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -62,6 +62,9 @@ typedef struct {
>  #define SPITZ_MACHINE_CLASS(klass) \
>  OBJECT_CLASS_CHECK(SpitzMachineClass, klass, TYPE_SPITZ_MACHINE)
>  
> +#define zaurus_printf(format, ...)  \
> +fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
> +
>  #undef REG_FMT
>  #define REG_FMT "0x%02lx"
>  
> diff --git a/hw/gpio/zaurus.c b/hw/gpio/zaurus.c
> index 9a12c683420..258e9264930 100644
> --- a/hw/gpio/zaurus.c
> +++ b/hw/gpio/zaurus.c
> @@ -22,9 +22,7 @@
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "qemu/module.h"
> -
> -#undef REG_FMT
> -#define REG_FMT  "0x%02lx"
> +#include "qemu/log.h"
>  
>  /* SCOOP devices */
>  
> @@ -104,7 +102,9 @@ static uint64_t scoop_read(void *opaque, hwaddr addr,
>  case SCOOP_GPRR:
>  return s->gpio_level;
>  default:
> -zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned 
> long)addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "scoop_read: bad register offset 0x%02" HWADDR_PRIx 
> "\n",
> +  addr);
>  }
>  
>  return 0;
> @@ -150,7 +150,9 @@ static void scoop_write(void *opaque, hwaddr addr,
>  scoop_gpio_handler_update(s);
>  break;
>  default:
> -zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned 
> long)addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "scoop_write: bad register offset 0x%02" HWADDR_PRIx 
> "\n",
> +  addr);
>  }
>  }
>  
> 

Re: [PATCH 14/17] hw/arm/spitz: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of logging guest accesses to invalid register offsets in the
> Spitz flash device with zaurus_printf() (which just prints to stderr),
> use the usual qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  hw/arm/spitz.c | 12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
> index 6eb46869157..49eae3fce4e 100644
> --- a/hw/arm/spitz.c
> +++ b/hw/arm/spitz.c
> @@ -23,6 +23,7 @@
>  #include "hw/ssi/ssi.h"
>  #include "hw/block/flash.h"
>  #include "qemu/timer.h"
> +#include "qemu/log.h"
>  #include "hw/arm/sharpsl.h"
>  #include "ui/console.h"
>  #include "hw/audio/wm8750.h"
> @@ -65,9 +66,6 @@ typedef struct {
>  #define zaurus_printf(format, ...)  \
>  fprintf(stderr, "%s: " format, __func__, ##__VA_ARGS__)
>  
> -#undef REG_FMT
> -#define REG_FMT "0x%02lx"
> -
>  /* Spitz Flash */
>  #define FLASH_BASE  0x0c00
>  #define FLASH_ECCLPLB   0x00/* Line parity 7 - 0 bit */
> @@ -137,7 +135,9 @@ static uint64_t sl_read(void *opaque, hwaddr addr, 
> unsigned size)
>  return ecc_digest(&s->ecc, nand_getio(s->nand));
>  
>  default:
> -zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned 
> long)addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "sl_read: bad register offset 0x%02" HWADDR_PRIx "\n",
> +  addr);
>  }
>  return 0;
>  }
> @@ -168,7 +168,9 @@ static void sl_write(void *opaque, hwaddr addr,
>  break;
>  
>  default:
> -zaurus_printf("Bad register offset " REG_FMT "\n", (unsigned 
> long)addr);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "sl_write: bad register offset 0x%02" HWADDR_PRIx "\n",
> +  addr);
>  }
>  }
>  
> 

Re: [PATCH 15/17] hw/arm/pxa2xx_pic: Use LOG_GUEST_ERROR for bad guest register accesses

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> Instead of using printf() for logging guest accesses to invalid
> register offsets in the pxa2xx PIC device, use the usual
> qemu_log_mask(LOG_GUEST_ERROR,...).
> 
> This was the only user of the REG_FMT macro in pxa.h, so we can
> remove that.
> 
> Signed-off-by: Peter Maydell 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  include/hw/arm/pxa.h | 1 -
>  hw/arm/pxa2xx_pic.c  | 9 +++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/arm/pxa.h b/include/hw/arm/pxa.h
> index f6dfb5c0cf0..8843e5f9107 100644
> --- a/include/hw/arm/pxa.h
> +++ b/include/hw/arm/pxa.h
> @@ -184,7 +184,6 @@ struct PXA2xxI2SState {
>  };
>  
>  # define PA_FMT  "0x%08lx"
> -# define REG_FMT "0x" TARGET_FMT_plx
>  
>  PXA2xxState *pxa270_init(MemoryRegion *address_space, unsigned int 
> sdram_size,
>   const char *revision);
> diff --git a/hw/arm/pxa2xx_pic.c b/hw/arm/pxa2xx_pic.c
> index 105c5e63f2f..ceee6aa48db 100644
> --- a/hw/arm/pxa2xx_pic.c
> +++ b/hw/arm/pxa2xx_pic.c
> @@ -11,6 +11,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>  #include "cpu.h"
>  #include "hw/arm/pxa.h"
>  #include "hw/sysbus.h"
> @@ -166,7 +167,9 @@ static uint64_t pxa2xx_pic_mem_read(void *opaque, hwaddr 
> offset,
>  case ICHP:   /* Highest Priority register */
>  return pxa2xx_pic_highest(s);
>  default:
> -printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "pxa2xx_pic_mem_read: bad register offset 0x%" 
> HWADDR_PRIx
> +  "\n", offset);
>  return 0;
>  }
>  }
> @@ -199,7 +202,9 @@ static void pxa2xx_pic_mem_write(void *opaque, hwaddr 
> offset,
>  s->priority[32 + ((offset - IPR32) >> 2)] = value & 0x803f;
>  break;
>  default:
> -printf("%s: Bad register offset " REG_FMT "\n", __func__, offset);
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "pxa2xx_pic_mem_write: bad register offset 0x%"
> +  HWADDR_PRIx "\n", offset);
>  return;
>  }
>  pxa2xx_pic_update(opaque);
> 

Re: [PATCH 16/17] hw/arm/spitz: Provide usual QOM macros for corgi-ssp and spitz-lcdtg

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The QOM types "spitz-lcdtg" and "corgi-ssp" are missing the
> usual QOM TYPE and casting macros; provide and use them.
> 
> In particular, we can safely use the QOM cast macros instead of
> FROM_SSI_SLAVE() because in both cases the 'ssidev' field of
> the instance state struct is the first field in it.
> 
> Signed-off-by: Peter Maydell 
> ---
>  hw/arm/spitz.c | 23 +++
>  1 file changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 17/17] Replace uses of FROM_SSI_SLAVE() macro with QOM casts

[Philippe Mathieu-Daudé]

On 6/28/20 4:24 PM, Peter Maydell wrote:
> The FROM_SSI_SLAVE() macro predates QOM and is used as a typesafe way
> to cast from an SSISlave* to the instance struct of a subtype of
> TYPE_SSI_SLAVE.  Switch to using the QOM cast macros instead, which
> have the same effect (by writing the QOM macros if the types were
> previously missing them.)
> 
> (The FROM_SSI_SLAVE() macro allows the SSISlave member of the
> subtype's struct to be anywhere as long as it is named "ssidev",
> whereas a QOM cast macro insists that it is the first thing in the
> subtype's struct.  This is true for all the types we convert here.)
> 
> This removes all the uses of FROM_SSI_SLAVE() so we can delete the
> definition.
> 
> Signed-off-by: Peter Maydell 
> ---
>  include/hw/ssi/ssi.h |  2 --
>  hw/arm/z2.c  | 11 +++
>  hw/display/ads7846.c |  9 ++---
>  hw/display/ssd0323.c | 10 +++---
>  hw/sd/ssi-sd.c   |  4 ++--
>  5 files changed, 22 insertions(+), 14 deletions(-)
> 
[...]
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 25cec2ddeaa..25cdf4c966d 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -74,7 +74,7 @@ typedef struct {
>  
>  static uint32_t ssi_sd_transfer(SSISlave *dev, uint32_t val)
>  {
> -ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, dev);
> +ssi_sd_state *s = SSI_SD(dev);
>  
>  /* Special case: allow CMD12 (STOP TRANSMISSION) while reading data.  */
>  if (s->mode == SSI_SD_DATA_READ && val == 0x4d) {
> @@ -241,7 +241,7 @@ static const VMStateDescription vmstate_ssi_sd = {
>  
>  static void ssi_sd_realize(SSISlave *d, Error **errp)
>  {
> -ssi_sd_state *s = FROM_SSI_SLAVE(ssi_sd_state, d);
> +ssi_sd_state *s = SSI_SD(d);
>  DeviceState *carddev;
>  DriveInfo *dinfo;
>  Error *err = NULL;
> 

Reviewed-by: Philippe Mathieu-Daudé 

回复: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Lin Ma]

> -邮件原件-
> 发件人: Stefan Hajnoczi 
> 发送时间: 2020年6月22日 20:14
> 收件人: Lin Ma 
> 抄送: qemu-devel@nongnu.org; f...@euphon.net; kw...@redhat.com;
> mre...@redhat.com; pbonz...@redhat.com
> 主题: Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16
> command
> 
> On Wed, Jun 17, 2020 at 06:30:18PM +0800, Lin Ma wrote:
> > Signed-off-by: Lin Ma 
> > ---
> >  hw/scsi/scsi-disk.c| 90
> ++
> >  include/block/accounting.h |  1 +
> >  include/scsi/constants.h   |  1 +
> >  3 files changed, 92 insertions(+)
> >
> > diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index
> > 387503e11b..9e3002ddaf 100644
> > --- a/hw/scsi/scsi-disk.c
> > +++ b/hw/scsi/scsi-disk.c
> > @@ -1866,6 +1866,89 @@ static void
> scsi_disk_emulate_write_data(SCSIRequest *req)
> >  }
> >  }
> >
> > +typedef struct GetLbaStatusCBData {
> > +uint32_t num_blocks;
> > +uint32_t is_deallocated;
> > +SCSIDiskReq *r;
> > +} GetLbaStatusCBData;
> > +
> > +static void scsi_get_lba_status_complete(void *opaque, int ret);
> > +
> > +static void scsi_get_lba_status_complete_noio(GetLbaStatusCBData
> > +*data, int ret) {
> > +SCSIDiskReq *r = data->r;
> > +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> > +
> > +assert(r->req.aiocb == NULL);
> > +
> > +block_acct_start(blk_get_stats(s->qdev.conf.blk), &r->acct,
> > + s->qdev.blocksize,
> BLOCK_ACCT_GET_LBA_STATUS);
> > +
> > +r->req.aiocb = blk_aio_get_lba_status(s->qdev.conf.blk,
> > +  r->req.cmd.lba *
> s->qdev.blocksize,
> > +  s->qdev.blocksize,
> > +
> > +scsi_get_lba_status_complete, data); }
> > +
> > +static void scsi_get_lba_status_complete(void *opaque, int ret) {
> > +GetLbaStatusCBData *data = opaque;
> > +SCSIDiskReq *r = data->r;
> > +SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, r->req.dev);
> > +
> > +assert(r->req.aiocb != NULL);
> > +r->req.aiocb = NULL;
> > +
> > +aio_context_acquire(blk_get_aio_context(s->qdev.conf.blk));
> > +if (scsi_disk_req_check_error(r, ret, true)) {
> > +g_free(data);
> > +goto done;
> > +}
> > +
> > +block_acct_done(blk_get_stats(s->qdev.conf.blk), &r->acct);
> > +scsi_req_unref(&r->req);
> > +g_free(data);
> > +
> > +done:
> > +aio_context_release(blk_get_aio_context(s->qdev.conf.blk));
> > +}
> > +
> > +static void scsi_disk_emulate_get_lba_status(SCSIRequest *req,
> > +uint8_t *outbuf) {
> > +SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> > +GetLbaStatusCBData *data;
> > +uint32_t *num_blocks;
> > +uint32_t *is_deallocated;
> > +
> > +data = g_new0(GetLbaStatusCBData, 1);
> > +data->r = r;
> > +num_blocks = &(data->num_blocks);
> > +is_deallocated = &(data->is_deallocated);
> > +
> > +scsi_req_ref(&r->req);
> > +scsi_get_lba_status_complete_noio(data, 0);
> 
> scsi_get_lba_status_complete_noio() looks asynchronous. If the BlockDriver
> yields in .bdrv_co_block_status() then the operation has not completed yet
> when scsi_get_lba_status_complete_noio() returns. It is not safe to access the
> GetLbaStatusCBData data until the async operation is complete.
> 
> Also, scsi_get_lba_status_complete() calls g_free(data) so there is a
> use-after-free when *num_blocks and *is_deallocated are accessed.

Got it, I'll fill the outbuf[] in the completion function in V3.

> These issues can be solved by making this code asynchronous (similar to
> read/write/flush/discard_zeroes/ioctl). outbuf[] will be filled in in the 
> completion
> function before g_free(data) is called.

I looked into block/io.c, The 'bdrv_co_pdiscard()', the 'bdrv_co_block_status' 
and the
'bdrv_co_flush()', They look similiar, They called corresponding 
bs->drv->bdrv_co_*()
or the bs->drv->bdrv_aio_*() between pair of blk_inc/dec_in_flight():
The 'bdrv_co_pdiscard()' calls bs->drv->bdrv_co_pdiscard() or 
bs->drv->bdrv_aio_pdiscard()
The 'bdrv_co_flush()' calls bs->drv->bdrv_co_flush*() or 
bs->drv->bdrv_aio_flush().
The 'bdrv_co_block_status' calls bs->drv->bdrv_co_block_status(). qemu contains 
the
coroutine version of block_status, no aio version of block_status.

About "making this code asynchronous", Well, In fact I havn't realized yet 
where the issue is.
If what you mean is that make the 'bdrv_co_get_lba_status()' asynchronous, How 
about
directly calling coroutine-based 'bdrv_co_block_status()' instead of 
'bdrv_block_status()' in it?
Or could you please suggest more detailed information?
BTW, IMO the existing BlockDriver->bdrv_co_block_status() is enough, It's not 
necessary to
implement a drv->bdrv_aio_get_block_status() in BlockDrivers(say qcow2 or raw), 
Am I right?

I'm not familiar with qemu block layer and coroutine, Sorry for any 
inconvenience.

> > +
> > +/*
> > + * 8 + 16 is the length in bytes of response header and
> > + * one LBA status descr

Re: [PATCH v2 1/9] hw/pci-host: add pci-intack write method

[Li Qiang]

P J P  于2020年6月25日周四 上午2:59写道：
>
> From: Prasad J Pandit 
>
> Add pci-intack mmio write method to avoid NULL pointer dereference
> issue.
>
> Reported-by: Lei Sun 
> Signed-off-by: Prasad J Pandit 

Reviewed-by: Li Qiang 

> ---
>  hw/pci-host/prep.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> index 367e408b91..3c8ff6af03 100644
> --- a/hw/pci-host/prep.c
> +++ b/hw/pci-host/prep.c
> @@ -26,6 +26,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/units.h"
> +#include "qemu/log.h"
>  #include "qapi/error.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_bus.h"
> @@ -119,8 +120,15 @@ static uint64_t raven_intack_read(void *opaque, hwaddr 
> addr,
>  return pic_read_irq(isa_pic);
>  }
>
> +static void raven_intack_write(void *opaque, hwaddr addr,
> +uint64_t data, unsigned size)
> +{
> +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
> +}
> +
>  static const MemoryRegionOps raven_intack_ops = {
>  .read = raven_intack_read,
> +.write = raven_intack_write,
>  .valid = {
>  .max_access_size = 1,
>  },
> --
> 2.26.2
>

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Christophe de Dinechin]

On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
>> If we want to build spice as a separately loadable module, we need to
>> put all the spice code in one loadable module, because the build
>> system does not know how to deal with dependencies yet.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  audio/Makefile.objs   | 2 +-
>>  chardev/Makefile.objs | 3 +--
>>  ui/Makefile.objs  | 8 
>>  3 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
>> index b4a4c11f31..298c895ff5 100644
>> --- a/audio/Makefile.objs
>> +++ b/audio/Makefile.objs
>> @@ -1,5 +1,5 @@
>>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
>> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
>> +spice-app.mo-objs += ../audio/spiceaudio.o
>
> Explicitly showing paths in the variables doesn't look right. The
> make recipes are supposed to automatically expand bare file names
> to add the right path. This is usually dealt with by a call to
> the "unnest-vars" function.

I agree. I mentioned this in the cover letter:

> - Adding various non-UI files into spice-app.so, which required a
>   couple of ../pwd/foo.o references in the makefile. Not very nice,
>   but I did not want to spend too much time fixing the makefiles.

I did not find an elegant way to integrate a non-UI file into a .mo that is
declared in the ui section.

I considered various solutions:

a) Having separate load modules for different source directories.
   This exposes details of the build system into the generated libs.

b) Moving the source
   This breaks the logical organization of the sources

c) Manually managing this specific .o one level up
   This seems even harder to read / understand

So I thought I would dedicate a bit more time to add that capability to the
unnest-vars function. I did not want to defer review for that, and vaguely
hoped that there was an existing correct way to do it that someone more
experienced in the build system could point me to.

>
>>  common-obj-$(CONFIG_AUDIO_COREAUDIO) += coreaudio.o
>>  common-obj-$(CONFIG_AUDIO_DSOUND) += dsoundaudio.o
>>  common-obj-$(CONFIG_AUDIO_WIN_INT) += audio_win_int.o
>> diff --git a/chardev/Makefile.objs b/chardev/Makefile.objs
>> index fc9910d4f2..955fac0cf9 100644
>> --- a/chardev/Makefile.objs
>> +++ b/chardev/Makefile.objs
>> @@ -22,5 +22,4 @@ common-obj-$(CONFIG_BRLAPI) += baum.o
>>  baum.o-cflags := $(SDL_CFLAGS)
>>  baum.o-libs := $(BRLAPI_LIBS)
>>
>> -common-obj-$(CONFIG_SPICE) += spice.mo
>> -spice.mo-objs := spice.o
>> +spice-app.mo-objs += ../chardev/spice.o
>> diff --git a/ui/Makefile.objs b/ui/Makefile.objs
>> index 504b196479..1ab515e23d 100644
>> --- a/ui/Makefile.objs
>> +++ b/ui/Makefile.objs
>> @@ -11,7 +11,6 @@ common-obj-y += keymaps.o console.o cursor.o qemu-pixman.o
>>  common-obj-y += input.o input-keymap.o input-legacy.o kbd-state.o
>>  common-obj-y += input-barrier.o
>>  common-obj-$(CONFIG_LINUX) += input-linux.o
>> -common-obj-$(CONFIG_SPICE) += spice-core.o spice-input.o spice-display.o
>>  common-obj-$(CONFIG_COCOA) += cocoa.o
>>  common-obj-$(CONFIG_VNC) += $(vnc-obj-y)
>>  common-obj-$(call lnot,$(CONFIG_VNC)) += vnc-stubs.o
>> @@ -53,10 +52,11 @@ curses.mo-objs := curses.o
>>  curses.mo-cflags := $(CURSES_CFLAGS) $(ICONV_CFLAGS)
>>  curses.mo-libs := $(CURSES_LIBS) $(ICONV_LIBS)
>>
>> -ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),yy)
>> -common-obj-$(if $(CONFIG_MODULES),m,y) += spice-app.mo
>> +common-obj-$(CONFIG_SPICE) += spice-app.mo
>> +spice-app.mo-objs += spice-core.o spice-input.o spice-display.o
>> +ifeq ($(CONFIG_GIO)$(CONFIG_SPICE),ym)
>> +spice-app.mo-objs += spice-app.o
>>  endif
>> -spice-app.mo-objs := spice-app.o
>>  spice-app.mo-cflags := $(GIO_CFLAGS)
>>  spice-app.mo-libs := $(GIO_LIBS)
>>
>> --
>> 2.26.2
>>
>>
>
> Regards,
> Daniel


--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v3 16/30] .gitignore: un-ignore .gitlab-ci.d

[Thomas Huth]

On 26/06/2020 20.13, Alex Bennée wrote:

The sooner we deprecate in-tree builds the sooner this mess of regexes
can be thrown away.

Signed-off-by: Alex Bennée 

---
v2
   - just use explicit !/.gitlab-ci.d
---
  .gitignore | 1 +
  1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 90acb4347d4..2992d15931a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -93,6 +93,7 @@
  *.tp
  *.vr
  *.d
+!/.gitlab-ci.d
  !/scripts/qemu-guest-agent/fsfreeze-hook.d
  *.o
  .sdk



Reviewed-by: Thomas Huth 

Re: [PATCH 11/46] qemu-option: Make uses of find_desc_by_name() more similar

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

This is to make the next commit easier to review.

Signed-off-by: Markus Armbruster


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 08/10] build: Add SPICE_CFLAGS and SPICE_LIBS to relevant files

[Christophe de Dinechin]

On 2020-06-26 at 19:26 CEST, Daniel P. Berrangé wrote...
> On Fri, Jun 26, 2020 at 06:43:05PM +0200, Christophe de Dinechin wrote:
>> Instead of adding the spice build flags to the top-level build
>> options, add them where they are necessary. This is a step to move the
>> burden of linking with spice libraries away from the top-level qemu.
>>
>> Signed-off-by: Christophe de Dinechin 
>> ---
>>  configure|  4 ++--
>>  hw/display/Makefile.objs |  1 +
>>  hw/i386/Makefile.objs|  1 +
>>  monitor/Makefile.objs|  3 +++
>>  softmmu/Makefile.objs|  2 +-
>>  stubs/Makefile.objs  |  2 +-
>>  ui/Makefile.objs |  4 ++--
>>  util/module.c| 13 +++--
>>  8 files changed, 22 insertions(+), 8 deletions(-)
>
>> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
>> index f32b9e47a3..1df8bb3814 100644
>> --- a/stubs/Makefile.objs
>> +++ b/stubs/Makefile.objs
>> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>>  stub-obj-y += runstate-check.o
>>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>>  stub-obj-y += set-fd-handler.o
>> -stub-obj-y += vmgenid.o
>>  stub-obj-y += sysbus.o
>>  stub-obj-y += tpm.o
>>  stub-obj-y += trace-control.o
>> +stub-obj-y += vmgenid.o
>>  stub-obj-y += vmstate.o
>>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
>>
>
> This looks unrelated to this series.

I'll send a separate trivial patch to fix the alphabetical ordering.
I used to have a spice.c stub here, which conflicted every time. This is how
I noticed the alphabetical order was not respected here.

>
>
>
>> diff --git a/util/module.c b/util/module.c
>> index 2fa93561fe..29b4806520 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -22,11 +22,11 @@
>>  #ifdef CONFIG_MODULE_UPGRADES
>>  #include "qemu-version.h"
>>  #endif
>> -#ifdef CONFIG_TRACE_RECORDER
>>  #include "trace/recorder.h"
>> -#endif
>>
>>
>> +RECORDER(modules, 16, "QEMU load modules");
>> +
>>  typedef struct ModuleEntry
>>  {
>>  void (*init)(void);
>> @@ -85,6 +85,15 @@ void register_dso_module_init(void (*fn)(void), 
>> module_init_type type)
>>  {
>>  ModuleEntry *e;
>>
>> +#ifdef CONFIG_TRACE_RECORDER
>> +static const char *name[] = {
>> +"MIGRATION", "BLOCK", "OPTS", "QOM",
>> +"TRACE", "XEN_BACKEND", "LIBQOS", "FUZZ_TARGET",
>> +"MAX"
>> +};
>> +#endif
>> +record(modules, "Register DSO module init %p type %u %+s",
>> +   fn, type, name[type]);
>>  init_lists();
>
> This looks unrelated too, but in general debugging should go via QEMU's
> standard trace backends.
>

Yes. I apparently botched a fixup. That was supposed to be a private patch
for my own use.



--
Cheers,
Christophe de Dinechin (IRC c3d)

Re: [PATCH v2 2/9] pci-host: add pcie-msi read method

[Li Qiang]

P J P  于2020年6月25日周四 上午2:59写道：

>
> From: Prasad J Pandit 
>
> Add pcie-msi mmio read method to avoid NULL pointer dereference
> issue.
>
> Reported-by: Lei Sun 
> Signed-off-by: Prasad J Pandit 

Reviewed-by: Li Qiang 

> ---
>  hw/pci-host/designware.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
> index 8492c18991..82262bdfdf 100644
> --- a/hw/pci-host/designware.c
> +++ b/hw/pci-host/designware.c
> @@ -21,6 +21,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>  #include "hw/pci/msi.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_host.h"
> @@ -63,6 +64,13 @@ designware_pcie_root_to_host(DesignwarePCIERoot *root)
>  return DESIGNWARE_PCIE_HOST(bus->parent);
>  }
>
> +static uint64_t designware_pcie_root_msi_read(void *opaque, hwaddr addr,
> +  unsigned size)
> +{
> +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
> +return 0;
> +}
> +
>  static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
> uint64_t val, unsigned len)
>  {
> @@ -77,6 +85,7 @@ static void designware_pcie_root_msi_write(void *opaque, 
> hwaddr addr,
>  }
>
>  static const MemoryRegionOps designware_pci_host_msi_ops = {
> +.read = designware_pcie_root_msi_read,
>  .write = designware_pcie_root_msi_write,
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  .valid = {
> --
> 2.26.2
>

[PATCH 1/3] softmmu: move softmmu only files from root

[Claudio Fontana]

move arch_init, balloon, cpus, ioport, memory, memory_mapping, qtest.

They are all specific to CONFIG_SOFTMMU.

Signed-off-by: Claudio Fontana 
Reviewed-by: Alex Bennée 
Reviewed-by: Laurent Vivier 
Reviewed-by: Thomas Huth 
---
 MAINTAINERS  | 12 ++--
 Makefile.target  |  7 ++-
 softmmu/Makefile.objs| 10 ++
 arch_init.c => softmmu/arch_init.c   |  0
 balloon.c => softmmu/balloon.c   |  0
 cpus.c => softmmu/cpus.c |  0
 ioport.c => softmmu/ioport.c |  0
 memory.c => softmmu/memory.c |  0
 memory_mapping.c => softmmu/memory_mapping.c |  0
 qtest.c => softmmu/qtest.c   |  0
 10 files changed, 18 insertions(+), 11 deletions(-)
 rename arch_init.c => softmmu/arch_init.c (100%)
 rename balloon.c => softmmu/balloon.c (100%)
 rename cpus.c => softmmu/cpus.c (100%)
 rename ioport.c => softmmu/ioport.c (100%)
 rename memory.c => softmmu/memory.c (100%)
 rename memory_mapping.c => softmmu/memory_mapping.c (100%)
 rename qtest.c => softmmu/qtest.c (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index dec252f38b..7214f59383 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -115,7 +115,7 @@ Overall TCG CPUs
 M: Richard Henderson 
 R: Paolo Bonzini 
 S: Maintained
-F: cpus.c
+F: softmmu/cpus.c
 F: cpus-common.c
 F: exec.c
 F: accel/tcg/
@@ -1708,7 +1708,7 @@ M: David Hildenbrand 
 S: Maintained
 F: hw/virtio/virtio-balloon*.c
 F: include/hw/virtio/virtio-balloon.h
-F: balloon.c
+F: softmmu/balloon.c
 F: include/sysemu/balloon.h
 
 virtio-9p
@@ -2177,12 +2177,12 @@ Memory API
 M: Paolo Bonzini 
 S: Supported
 F: include/exec/ioport.h
-F: ioport.c
 F: include/exec/memop.h
 F: include/exec/memory.h
 F: include/exec/ram_addr.h
 F: include/exec/ramblock.h
-F: memory.c
+F: softmmu/ioport.c
+F: softmmu/memory.c
 F: include/exec/memory-internal.h
 F: exec.c
 F: scripts/coccinelle/memory-region-housekeeping.cocci
@@ -2214,13 +2214,13 @@ F: ui/cocoa.m
 Main loop
 M: Paolo Bonzini 
 S: Maintained
-F: cpus.c
 F: include/qemu/main-loop.h
 F: include/sysemu/runstate.h
 F: util/main-loop.c
 F: util/qemu-timer.c
 F: softmmu/vl.c
 F: softmmu/main.c
+F: softmmu/cpus.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
@@ -2375,7 +2375,7 @@ M: Thomas Huth 
 M: Laurent Vivier 
 R: Paolo Bonzini 
 S: Maintained
-F: qtest.c
+F: softmmu/qtest.c
 F: accel/qtest.c
 F: tests/qtest/
 X: tests/qtest/bios-tables-test-allowed-diff.h
diff --git a/Makefile.target b/Makefile.target
index 8ed1eba95b..7fbf5d8b92 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -152,16 +152,13 @@ endif #CONFIG_BSD_USER
 #
 # System emulator target
 ifdef CONFIG_SOFTMMU
-obj-y += arch_init.o cpus.o gdbstub.o balloon.o ioport.o
-obj-y += qtest.o
+obj-y += softmmu/
+obj-y += gdbstub.o
 obj-y += dump/
 obj-y += hw/
 obj-y += monitor/
 obj-y += qapi/
-obj-y += memory.o
-obj-y += memory_mapping.o
 obj-y += migration/ram.o
-obj-y += softmmu/
 LIBS := $(libs_softmmu) $(LIBS)
 
 # Hardware support
diff --git a/softmmu/Makefile.objs b/softmmu/Makefile.objs
index dd15c24346..a4bd9f2f52 100644
--- a/softmmu/Makefile.objs
+++ b/softmmu/Makefile.objs
@@ -1,3 +1,13 @@
 softmmu-main-y = softmmu/main.o
+
+obj-y += arch_init.o
+obj-y += cpus.o
+obj-y += balloon.o
+obj-y += ioport.o
+obj-y += memory.o
+obj-y += memory_mapping.o
+
+obj-y += qtest.o
+
 obj-y += vl.o
 vl.o-cflags := $(GPROF_CFLAGS) $(SDL_CFLAGS)
diff --git a/arch_init.c b/softmmu/arch_init.c
similarity index 100%
rename from arch_init.c
rename to softmmu/arch_init.c
diff --git a/balloon.c b/softmmu/balloon.c
similarity index 100%
rename from balloon.c
rename to softmmu/balloon.c
diff --git a/cpus.c b/softmmu/cpus.c
similarity index 100%
rename from cpus.c
rename to softmmu/cpus.c
diff --git a/ioport.c b/softmmu/ioport.c
similarity index 100%
rename from ioport.c
rename to softmmu/ioport.c
diff --git a/memory.c b/softmmu/memory.c
similarity index 100%
rename from memory.c
rename to softmmu/memory.c
diff --git a/memory_mapping.c b/softmmu/memory_mapping.c
similarity index 100%
rename from memory_mapping.c
rename to softmmu/memory_mapping.c
diff --git a/qtest.c b/softmmu/qtest.c
similarity index 100%
rename from qtest.c
rename to softmmu/qtest.c
-- 
2.16.4

[PATCH 3/3] cpu-timers, icount: new modules

[Claudio Fontana]

refactoring of cpus.c continues with cpu timer state extraction.

cpu-timers: responsible for the cpu timers state, and for access to
cpu clocks and ticks.

icount: counts the TCG instructions executed. As such it is specific to
the TCG accelerator. Therefore, it is built only under CONFIG_TCG.

One complication is due to qtest, which misuses icount to warp time
(qtest_clock_warp). In order to solve this problem, detach instead qtest
from icount, and use a trivial separate counter for it.

This requires fixing assumptions scattered in the code that
qtest_enabled() implies icount_enabled().

No functionality change.

Signed-off-by: Claudio Fontana 
Reviewed-by: Alex Bennée 
---
 MAINTAINERS  |   2 +
 accel/qtest.c|   6 +-
 accel/tcg/cpu-exec.c |  43 ++-
 accel/tcg/tcg-all.c  |   7 +-
 accel/tcg/translate-all.c|   3 +-
 dma-helpers.c|   4 +-
 docs/replay.txt  |   6 +-
 exec.c   |   4 -
 hw/core/ptimer.c |   8 +-
 hw/i386/x86.c|   1 +
 include/exec/cpu-all.h   |   4 +
 include/exec/exec-all.h  |   4 +-
 include/qemu/timer.h |  22 +-
 include/sysemu/cpu-timers.h  |  81 +
 include/sysemu/cpus.h|  12 +-
 include/sysemu/qtest.h   |   2 +
 include/sysemu/replay.h  |   4 +-
 replay/replay.c  |   6 +-
 softmmu/Makefile.objs|   2 +
 softmmu/cpu-timers.c | 284 
 softmmu/cpus.c   | 750 +--
 softmmu/icount.c | 499 
 softmmu/qtest.c  |  34 +-
 softmmu/timers-state.h   |  69 
 softmmu/vl.c |   8 +-
 stubs/Makefile.objs  |   3 +-
 stubs/clock-warp.c   |   4 +-
 stubs/cpu-get-clock.c|   3 +-
 stubs/cpu-get-icount.c   |  21 --
 stubs/icount.c   |  22 ++
 stubs/qemu-timer-notify-cb.c |   8 +
 stubs/qtest.c|   5 +
 target/alpha/translate.c |   3 +-
 target/arm/helper.c  |   7 +-
 target/riscv/csr.c   |   8 +-
 tests/ptimer-test-stubs.c|   7 +-
 tests/test-timed-average.c   |   2 +-
 util/main-loop.c |   4 +-
 util/qemu-timer.c|  12 +-
 39 files changed, 1121 insertions(+), 853 deletions(-)
 create mode 100644 include/sysemu/cpu-timers.h
 create mode 100644 softmmu/cpu-timers.c
 create mode 100644 softmmu/icount.c
 create mode 100644 softmmu/timers-state.h
 delete mode 100644 stubs/cpu-get-icount.c
 create mode 100644 stubs/icount.c
 create mode 100644 stubs/qemu-timer-notify-cb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 827668fa8a..d2a5f20963 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -,6 +,8 @@ F: softmmu/vl.c
 F: softmmu/main.c
 F: softmmu/cpus.c
 F: softmmu/cpu-throttle.c
+F: softmmu/cpu-timers.c
+F: softmmu/icount.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
diff --git a/accel/qtest.c b/accel/qtest.c
index 5b88f55921..119d0f16a4 100644
--- a/accel/qtest.c
+++ b/accel/qtest.c
@@ -19,14 +19,10 @@
 #include "sysemu/accel.h"
 #include "sysemu/qtest.h"
 #include "sysemu/cpus.h"
+#include "sysemu/cpu-timers.h"
 
 static int qtest_init_accel(MachineState *ms)
 {
-QemuOpts *opts = qemu_opts_create(qemu_find_opts("icount"), NULL, 0,
-  &error_abort);
-qemu_opt_set(opts, "shift", "0", &error_abort);
-configure_icount(opts, &error_abort);
-qemu_opts_del(opts);
 return 0;
 }
 
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index d95c4848a4..82155c1db3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -19,6 +19,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu-common.h"
+#include "qemu/qemu-print.h"
 #include "cpu.h"
 #include "trace.h"
 #include "disas/disas.h"
@@ -36,6 +37,8 @@
 #include "hw/i386/apic.h"
 #endif
 #include "sysemu/cpus.h"
+#include "exec/cpu-all.h"
+#include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
 
 /* -icount align implementation. */
@@ -56,6 +59,9 @@ typedef struct SyncClocks {
 #define MAX_DELAY_PRINT_RATE 20LL
 #define MAX_NB_PRINTS 100
 
+static int64_t max_delay;
+static int64_t max_advance;
+
 static void align_clocks(SyncClocks *sc, CPUState *cpu)
 {
 int64_t cpu_icount;
@@ -65,7 +71,7 @@ static void align_clocks(SyncClocks *sc, CPUState *cpu)
 }
 
 cpu_icount = cpu->icount_extra + cpu_neg(cpu)->icount_decr.u16.low;
-sc->diff_clk += cpu_icount_to_ns(sc->last_cpu_icount - cpu_icount);
+sc->diff_clk += icount_to_ns(sc->last_cpu_icount - cpu_icount);
 sc->last_cpu_icount = cpu_icount;
 
 if (sc->diff_clk > VM_CLOCK_ADVANCE) {
@@ -98,9 +104,9 @@ static void print_delay(const SyncClocks *sc)
 (-sc->diff_clk / (float)10LL <
  (threshold_delay - THRESHOLD_REDUCE))) {
 threshold_delay = (-sc->diff_clk / 10LL) + 1;
-printf("Warning: The guest is now late by %.1f to %.1f seconds\n",

[PATCH 0/3] QEMU cpus.c refactoring part1

[Claudio Fontana]

Motivation and higher level steps:

https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

Previous series: [RFC RESEND v7 0/4] QEMU cpus.c refactoring

This series is already reviewed, and is a split of the first three patches
from the previous series (RFC). The forth and last of the previous series
will then be posted separately.

PREVIOUS DISCUSSIONS:

* should we reorder patches or moves inside patches to avoid code going
  from cpus.c to softmmu/cpus.c and then again to softmmu/somethingelse.c ?
  (Philippe)

* some questions about headers in include/softmmu (Philippe)


[SPLIT into TWO series, changed from RFC to PATCH]


v6 -> v7:

* rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
  "icount: make dma reads deterministic"



v5 -> v6:

* rebased changes on top of Emilio G. Cota changes to cpus.c
  "cpu: convert queued work to a QSIMPLEQ"

* keep a pointer in cpus.c instead of a copy of CpusAccel
  (Alex)




v4 -> v5: rebase on latest master

* rebased changes on top of roman series to remove one of the extra states for 
hvf.
  (Is the result now functional for HVF?)

* rebased changes on top of icount changes and fixes to icount_configure and
  the new shift vmstate. (Markus)

v3 -> v4:

* overall: added copyright headers to all files that were missing them
  (used copyright and license of the module the stuff was extracted from).
  For the new interface files, added SUSE LLC.

* 1/4 (move softmmu only files from root):

  MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)

* 2/4 (cpu-throttle):

  MAINTAINERS (to patch 1),
  copyright Fabrice Bellard and license from cpus.c

* 3/4 (cpu-timers, icount):

  - MAINTAINERS: add cpu-timers.c and icount.c to Paolo

  - break very long lines (patchew)

  - add copyright SUSE LLC, GPLv2 to cpu-timers.h

  - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
as it is lifted from cpus.c

  - vl.c: in configure_accelerators bail out if icount_enabled()
and !tcg_enabled() as qtest does not enable icount anymore.

* 4/4 (accel stuff to accel):

  - add copyright SUSE LLC to files that mostly only consist of the
new interface. Add whatever copyright was in the accelerator code
if instead they mostly consist of accelerator code.

  - change a comment to mention the result of the AccelClass experiment

  - moved qtest accelerator into accel/qtest/ , make it like the others.

  - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)

  - rename accel_int to cpus_accel

  - rename CpusAccel functions from cpu_synchronize_* to synchronize_*




v2 -> v3:

* turned into a 4 patch series, adding a first patch moving
  softmmu code currently in top_srcdir to softmmu/

* cpu-throttle: moved to softmmu/

* cpu-timers, icount:

  - moved to softmmu/

  - fixed assumption of qtest_enabled() => icount_enabled()
  causing the failure of check-qtest-arm goal, in test-arm-mptimer.c

  Fix is in hw/core/ptimer.c,

  where the artificial timeout rate limit should not be applied
  under qtest_enabled(), in a similar way to how it is not applied
  for icount_enabled().

* CpuAccelInterface: no change.





v1 -> v2:

* 1/3 (cpu-throttle): provide a description in the commit message

* 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers,
  as icount is actually TCG-specific. Only build it under CONFIG_TCG.

  To do this, qtest had to be detached from icount. To this end, a
  trivial global counter for qtest has been introduced.

* 3/3 (CpuAccelInterface): provided a description.

This is point 8) in that plan. The idea is to extract the unrelated parts
in cpus, and register interfaces from each single accelerator to the main
cpus module (cpus.c).

While doing this RFC, I noticed some assumptions about Windows being
either TCG or HAX (not considering WHPX) that might need to be revisited.
I added a comment there.

The thing builds successfully based on Linux cross-compilations for
windows/hax, windows/whpx, and I got a good build on Darwin/hvf.

Tests run successully for tcg and kvm configurations, but did not test on
windows or darwin.

Welcome your feedback and help on this,

Claudio

Claudio Fontana (3):
  softmmu: move softmmu only files from root
  cpu-throttle: new module, extracted from cpus.c
  cpu-timers, icount: new modules

 MAINTAINERS  |  15 +-
 Makefile.target  |   7 +-
 accel/qtest.c|   6 +-
 accel/tcg/cpu-exec.c |  43 +-
 accel/tcg/tcg-all.c  |   7 +-
 accel/tcg/translate-all.c|   3 +-
 dma-helpers.c|   4 +-
 docs/replay.txt  |   6 +-
 exec.c   |   4 -
 hw/core/ptimer.c |   8 +-
 hw/i386/x86.c|   1 +
 include/

[PATCH 2/3] cpu-throttle: new module, extracted from cpus.c

[Claudio Fontana]

move the vcpu throttling functionality into its own module.

This functionality is not specific to any accelerator,
and it is used currently by migration to slow down guests to try to
have migrations converge, and by the cocoa MacOS UI to throttle speed.

cpu-throttle contains the controls to adjust and inspect throttle
settings, start (set) and stop vcpu throttling, and the throttling
function itself that is run periodically on vcpus to make them take a nap.

Execution of the throttling function on all vcpus is triggered by a timer,
registered at module initialization.

No functionality change.

Signed-off-by: Claudio Fontana 
Reviewed-by: Alex Bennée 
Reviewed-by: Laurent Vivier 
---
 MAINTAINERS   |   1 +
 include/hw/core/cpu.h |  37 -
 include/qemu/main-loop.h  |   5 ++
 include/sysemu/cpu-throttle.h |  68 +++
 migration/migration.c |   1 +
 migration/ram.c   |   1 +
 softmmu/Makefile.objs |   1 +
 softmmu/cpu-throttle.c| 122 ++
 softmmu/cpus.c|  95 +++-
 9 files changed, 207 insertions(+), 124 deletions(-)
 create mode 100644 include/sysemu/cpu-throttle.h
 create mode 100644 softmmu/cpu-throttle.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7214f59383..827668fa8a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2221,6 +2221,7 @@ F: util/qemu-timer.c
 F: softmmu/vl.c
 F: softmmu/main.c
 F: softmmu/cpus.c
+F: softmmu/cpu-throttle.c
 F: qapi/run-state.json
 
 Human Monitor (HMP)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b79318..5542577d2b 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -822,43 +822,6 @@ bool cpu_exists(int64_t id);
  */
 CPUState *cpu_by_arch_id(int64_t id);
 
-/**
- * cpu_throttle_set:
- * @new_throttle_pct: Percent of sleep time. Valid range is 1 to 99.
- *
- * Throttles all vcpus by forcing them to sleep for the given percentage of
- * time. A throttle_percentage of 25 corresponds to a 75% duty cycle roughly.
- * (example: 10ms sleep for every 30ms awake).
- *
- * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
- * Once the throttling starts, it will remain in effect until cpu_throttle_stop
- * is called.
- */
-void cpu_throttle_set(int new_throttle_pct);
-
-/**
- * cpu_throttle_stop:
- *
- * Stops the vcpu throttling started by cpu_throttle_set.
- */
-void cpu_throttle_stop(void);
-
-/**
- * cpu_throttle_active:
- *
- * Returns: %true if the vcpus are currently being throttled, %false otherwise.
- */
-bool cpu_throttle_active(void);
-
-/**
- * cpu_throttle_get_percentage:
- *
- * Returns the vcpu throttle percentage. See cpu_throttle_set for details.
- *
- * Returns: The throttle percentage in range 1 to 99.
- */
-int cpu_throttle_get_percentage(void);
-
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index a6d20b0719..8e98613656 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -303,6 +303,11 @@ void qemu_mutex_unlock_iothread(void);
  */
 void qemu_cond_wait_iothread(QemuCond *cond);
 
+/*
+ * qemu_cond_timedwait_iothread: like the previous, but with timeout
+ */
+void qemu_cond_timedwait_iothread(QemuCond *cond, int ms);
+
 /* internal interfaces */
 
 void qemu_fd_register(int fd);
diff --git a/include/sysemu/cpu-throttle.h b/include/sysemu/cpu-throttle.h
new file mode 100644
index 00..d65bdef6d0
--- /dev/null
+++ b/include/sysemu/cpu-throttle.h
@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2012 SUSE LINUX Products 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
+ * 
+ */
+
+#ifndef SYSEMU_CPU_THROTTLE_H
+#define SYSEMU_CPU_THROTTLE_H
+
+#include "qemu/timer.h"
+
+/**
+ * cpu_throttle_init:
+ *
+ * Initialize the CPU throttling API.
+ */
+void cpu_throttle_init(void);
+
+/**
+ * cpu_throttle_set:
+ * @new_throttle_pct: Percent of sleep time. Valid range is 1 to 99.
+ *
+ * Throttles all vcpus by forcing them to sleep for the given percentage of
+ * time. A throttle_percentage of 25 corresponds to a 75% duty cycle roughly.
+ * (example: 10ms sleep for every 30ms awake).
+ *
+ * cpu_throttle_set can be called as needed to adjust new_throttle_pct.
+ * Once the throttling starts, it will remain in effect u

Re: [PATCH v2 3/9] vfio: add quirk device write method

[Li Qiang]

P J P  于2020年6月25日周四 上午2:59写道：
>
> From: Prasad J Pandit 
>
> Add vfio quirk device mmio write method to avoid NULL pointer
> dereference issue.
>
> Reported-by: Lei Sun 
> Signed-off-by: Prasad J Pandit 

Reviewed-by: Li Qiang 

> ---
>  hw/vfio/pci-quirks.c | 8 
>  1 file changed, 8 insertions(+)
>
> Update v2: use LOG_GUEST_ERROR
>   -> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg04962.html
>
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index d304c81148..cc6d5dbc23 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -14,6 +14,7 @@
>  #include "config-devices.h"
>  #include "exec/memop.h"
>  #include "qemu/units.h"
> +#include "qemu/log.h"
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
>  #include "qemu/module.h"
> @@ -264,8 +265,15 @@ static uint64_t vfio_ati_3c3_quirk_read(void *opaque,
>  return data;
>  }
>
> +static void vfio_ati_3c3_quirk_write(void *opaque, hwaddr addr,
> +uint64_t data, unsigned size)
> +{
> +qemu_log_mask(LOG_GUEST_ERROR, "%s not implemented\n", __func__);
> +}
> +
>  static const MemoryRegionOps vfio_ati_3c3_quirk = {
>  .read = vfio_ati_3c3_quirk_read,
> +.write = vfio_ati_3c3_quirk_write,
>  .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
> --
> 2.26.2
>

Re: [PATCH 11/46] qemu-option: Make uses of find_desc_by_name() more similar

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

This is to make the next commit easier to review.

Signed-off-by: Markus Armbruster 
---
  util/qemu-option.c | 32 ++--
  1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6119f971a4..9941005c91 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -270,6 +270,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char 
*name)
  const char *qemu_opt_get(QemuOpts *opts, const char *name)
  {
  QemuOpt *opt;
+const QemuOptDesc *desc;
  

Honestly, I don't see how this hunk helps with the following patch, which is 
simple anyway.
Keeping desc variable scope smaller seems better for me, as well as further 
scope of
def_val. (Still, keep my r-b if you don't want to change it).

--
Best regards,
Vladimir

Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

Hi Peter,

On 6/28/20 10:37 PM, Peter Maydell wrote:
> Currently we have a free-floating set of IRQs and a function
> tosa_out_switch() which handle the GPIO lines on the tosa board which
> connect to LEDs, and another free-floating IRQ and tosa_reset()
> function to handle the GPIO line that resets the system.  Encapsulate
> this behaviour in a simple QOM device.
> 
> This commit fixes Coverity issue CID 1421929 (which pointed out that
> the 'outsignals' in tosa_gpio_setup() were leaked), because it
> removes the use of the qemu_allocate_irqs() API from this code
> entirely.
> 
> Signed-off-by: Peter Maydell 
> ---
> This is simpler than the spitz changes because the new device
> doesn't need to refer to any of the other devices on the board.
> ---
>  hw/arm/tosa.c | 88 +--
>  1 file changed, 64 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 06ecf1e7824..383b3b22e24 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -65,24 +65,39 @@ static void tosa_microdrive_attach(PXA2xxState *cpu)
>  pxa2xx_pcmcia_attach(cpu->pcmcia[0], md);
>  }
>  
> -static void tosa_out_switch(void *opaque, int line, int level)
> +/*
> + * Encapsulation of some GPIO line behaviour for the Tosa board
> + *
> + * QEMU interface:
> + *  + named GPIO inputs "leds[0..3]": assert to light LEDs
> + *  + named GPIO input "reset": when asserted, resets the system
> + */
> +
> +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio"
> +#define TOSA_MISC_GPIO(obj) \
> +OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO)
> +
> +typedef struct TosaMiscGPIOState {
> +SysBusDevice parent_obj;
> +} TosaMiscGPIOState;

Since we don't really use this type, can we avoid declaring it?

Like:

  #define TOSA_MISC_GPIO(obj) \
  OBJECT_CHECK(SysBusDevice, (obj), TYPE_TOSA_MISC_GPIO)

And in tosa_misc_gpio_info:

.instance_size = sizeof(SysBusDevice)

> +
> +static void tosa_gpio_leds(void *opaque, int line, int level)
>  {
>  switch (line) {
> -case 0:
> -fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> -break;
> -case 1:
> -fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> -break;
> -case 2:
> -fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> -break;
> -case 3:
> -fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> -break;
> -default:
> -fprintf(stderr, "Uhandled out event: %d = %d\n", line, level);
> -break;
> +case 0:
> +fprintf(stderr, "blue LED %s.\n", level ? "on" : "off");
> +break;
> +case 1:
> +fprintf(stderr, "green LED %s.\n", level ? "on" : "off");
> +break;
> +case 2:
> +fprintf(stderr, "amber LED %s.\n", level ? "on" : "off");
> +break;
> +case 3:
> +fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off");
> +break;

Nitpicking, the indentation change might go in the previous patch.

> +default:
> +g_assert_not_reached();
>  }
>  }
>  
> @@ -93,13 +108,22 @@ static void tosa_reset(void *opaque, int line, int level)
>  }
>  }
>  
> +static void tosa_misc_gpio_init(Object *obj)
> +{
> +DeviceState *dev = DEVICE(obj);
> +

Ah, MachineClass does not inherit from DeviceClass, so we can use
it to create GPIOs.

Something is bugging me here, similar with the LEDs series I sent
recently.

GPIOs are not specific to a bus. I see ResettableClass takes Object
arguments.

We should be able to wire GPIO lines to generic Objects like LEDs.
Parents don't have to be qdev.

Actually looking at qdev_init_gpio_in_named_with_opaque(), the
function only accesses the QOM API, not the QDEV one. The only
field stored in the state is the gpio list:

struct DeviceState {
...
QLIST_HEAD(, NamedGPIOList) gpios;

Having to create a container to wire GPIOs or hold a reference
to a MemoryRegion sounds wrong.

If the MachineState can not do that, can we create a generic
BoardState (like PCB to route signals) so all machines can use it?

> +qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4);
> +qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1);
> +}
> +
>  static void tosa_gpio_setup(PXA2xxState *cpu,
>  DeviceState *scp0,
>  DeviceState *scp1,
>  TC6393xbState *tmio)
>  {
> -qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4);
> -qemu_irq reset;
> +DeviceState *misc_gpio;
> +
> +misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL);
>  
>  /* MMC/SD host */
>  pxa2xx_mmci_handlers(cpu->mmc,
> @@ -107,8 +131,8 @@ static void tosa_gpio_setup(PXA2xxState *cpu,
>  qemu_irq_invert(qdev_get_gpio_in(cpu->gpio, 
> TOSA_GPIO_nSD_DETECT)));
>  
>  /* Handle reset */
> -reset = qemu_allocate_irq(tosa_reset, cpu, 0);
> -qdev_

Re: [PATCH 12/46] qemu-option: Factor out helper find_default_by_name()

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster


Reviewed-by: Vladimir Sementsov-Ogievskiy 

--
Best regards,
Vladimir

Re: [PATCH 11/46] qemu-option: Make uses of find_desc_by_name() more similar

[Vladimir Sementsov-Ogievskiy]

29.06.2020 12:36, Vladimir Sementsov-Ogievskiy wrote:

24.06.2020 19:43, Markus Armbruster wrote:

This is to make the next commit easier to review.

Signed-off-by: Markus Armbruster 
---
  util/qemu-option.c | 32 ++--
  1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6119f971a4..9941005c91 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -270,6 +270,7 @@ static void qemu_opt_del_all(QemuOpts *opts, const char 
*name)
  const char *qemu_opt_get(QemuOpts *opts, const char *name)
  {
  QemuOpt *opt;
+    const QemuOptDesc *desc;

Honestly, I don't see how this hunk helps with the following patch, which is 
simple anyway.
Keeping desc variable scope smaller seems better for me, as well as further 
scope of
def_val. (Still, keep my r-b if you don't want to change it).



Aha, I see, we have more similar patterns and you want them to look similarly. 
Still, it's
better to keep scope of variable smaller. May be a follow-up.

--
Best regards,
Vladimir

Re: [PATCH 04/10] spice: Move all the spice-related code in spice-app.so

[Daniel P . Berrangé]

On Mon, Jun 29, 2020 at 11:22:40AM +0200, Christophe de Dinechin wrote:
> 
> On 2020-06-26 at 19:20 CEST, Daniel P. Berrangé wrote...
> > On Fri, Jun 26, 2020 at 06:43:01PM +0200, Christophe de Dinechin wrote:
> >> If we want to build spice as a separately loadable module, we need to
> >> put all the spice code in one loadable module, because the build
> >> system does not know how to deal with dependencies yet.
> >>
> >> Signed-off-by: Christophe de Dinechin 
> >> ---
> >>  audio/Makefile.objs   | 2 +-
> >>  chardev/Makefile.objs | 3 +--
> >>  ui/Makefile.objs  | 8 
> >>  3 files changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/audio/Makefile.objs b/audio/Makefile.objs
> >> index b4a4c11f31..298c895ff5 100644
> >> --- a/audio/Makefile.objs
> >> +++ b/audio/Makefile.objs
> >> @@ -1,5 +1,5 @@
> >>  common-obj-y = audio.o audio_legacy.o noaudio.o wavaudio.o mixeng.o
> >> -common-obj-$(CONFIG_SPICE) += spiceaudio.o
> >> +spice-app.mo-objs += ../audio/spiceaudio.o
> >
> > Explicitly showing paths in the variables doesn't look right. The
> > make recipes are supposed to automatically expand bare file names
> > to add the right path. This is usually dealt with by a call to
> > the "unnest-vars" function.
> 
> I agree. I mentioned this in the cover letter:
> 
> > - Adding various non-UI files into spice-app.so, which required a
> >   couple of ../pwd/foo.o references in the makefile. Not very nice,
> >   but I did not want to spend too much time fixing the makefiles.
> 
> I did not find an elegant way to integrate a non-UI file into a .mo that is
> declared in the ui section.
> 
> I considered various solutions:
> 
> a) Having separate load modules for different source directories.
>This exposes details of the build system into the generated libs.
> 
> b) Moving the source
>This breaks the logical organization of the sources
> 
> c) Manually managing this specific .o one level up
>This seems even harder to read / understand

I don't think any of these three should be required. The problem isn't
the structure of the makefiles or layout of the source, rather it is
a matter of expanding paths correctly in the build rules.

> So I thought I would dedicate a bit more time to add that capability to the
> unnest-vars function. I did not want to defer review for that, and vaguely
> hoped that there was an existing correct way to do it that someone more
> experienced in the build system could point me to.

With QEMU's build system, regardless of where the rules are defined,
they should get run in the context of the top level makefile, as all
the sub-dir Makefiles are just includes. The unnest-vars function
takes relative filenames and adds the correct directory prefix to
them.

IIUC, common-obj-m gets  spice-app.mo added. common-obj-m is processed
by unnest-vars, but spice-app.mo-objs is not, so all its .o files are
relative. So we just need to fix the way the *.mo-objs variables are
processed, such that unnest-vars is used to add the directory prefixes.


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

[PATCH] trivial: Respect alphabetical order of .o files in Makefile.objs

[Christophe de Dinechin]

The vmgenid.o is the only file that is not in alphabetical order.

Signed-off-by: Christophe de Dinechin 
---
 stubs/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f32b9e47a3..1df8bb3814 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -19,10 +19,10 @@ stub-obj-y += replay.o
 stub-obj-y += runstate-check.o
 stub-obj-$(CONFIG_SOFTMMU) += semihost.o
 stub-obj-y += set-fd-handler.o
-stub-obj-y += vmgenid.o
 stub-obj-y += sysbus.o
 stub-obj-y += tpm.o
 stub-obj-y += trace-control.o
+stub-obj-y += vmgenid.o
 stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
 
-- 
2.26.2

Re: [PATCH rc4 06/29] target/avr: Add defintions of AVR core types

[Philippe Mathieu-Daudé]

Hi Michael,

On 3/6/20 2:34 PM, Michael Rolnik wrote:
> Hi all.
> 
> are there any action items for me?

FYI QEMU 5.1 soft freeze is next Tuesday, so even if Aleksandar
respin this week, it is very unlikely Richard and myself have
time to review it again before next week.

TBH let face it, AVR won't be in the next release.

When QEMU 5.2 development windows opens, you should start pinging
early and insist every week.

I'm sorry you have been waiting so long without any updates. If
that can comfort you a bit, you are not the only one waiting for
this to be merged. Now if the AVR users were more active in the
QEMU community (simply by showing their presence, sharing how they
would use it) this would raise the interest in this port.

Regards,

Phil.

> 
> Regards,
> Michael Rolnik
> 
> On Thu, Feb 27, 2020 at 10:38 AM Michael Rolnik  > wrote:
> 
> Hi all.
> 
> I don't see how the fact that some MCUs have an instruction and some
> don't within same AVR family.
> Think about gnu binutils (not GCC) these utils have no idea about
> MCUs all they are aware of AVR CPU only.
> However, I don't mind removing -cpu flag.
> 
> Regards,
> Michael Ronik
> 
> On Fri, Feb 21, 2020 at 5:31 PM Aleksandar Markovic
> mailto:aleksandar.m.m...@gmail.com>>
> wrote:
> 
> On Fri, Feb 21, 2020 at 12:04 PM Michael Rolnik
> mailto:mrol...@gmail.com>> wrote:
> >
> > Hi all.
> >
> > How is it going?
> >
> > Regards,
> > Michael.
> >
> 
> Michael,
> 
> I think we are very close to merging.
> 
> There is absolutely no need to support ALL AVR mcus or AVR core
> types
> in the first version that will be merged.
> 
> But this issue (recently discovered during Jaoquin's review)
> about the
> fact that an avr core type doesn't determine in a complete way the
> instruction set of a particular MCU is thorny.
> 
> Should we switch from "-cpu " to "-cpu "
> (but keep
> the current avr core type organization for internal purpose)? Or
> something else?
> 
> This is a high-level AVR suppot design issue. Let's think about it
> without a rush.
> 
> The problem is that once one organization/meaning of that switch is
> upstreamed, it is very difficult to switch to other. There is a
> procedural rule that deprecation process lasts at least 8
> months, plus
> there may be some technical obstacles and difficulties.
> 
> In short, "-cpu " is not enough to emulate
> accurately a
> givem program.
> 
> Regards,
> Aleksandar
> 
> > On Mon, Feb 10, 2020 at 9:39 AM Michael Rolnik
> mailto:mrol...@gmail.com>> wrote:
> >>
> >> Hi all.
> >>
> >> When I decided to implement AVR 8 bit CPU support for QEMU I
> found this document which listed all AVR instructions.
> >> After that I learned that there are several CPU flavours, I
> looked into this GCC file to figure out what are they as I could
> not find any official document explaining it.
> >> Then I downloaded several datasheets and created a list of
> instructions by CPU type (attached).It turned out that there are
> some variations
> >> e.g.
> >> - AVTTINY - some have LDS, some don't
> >> - AVR1, AVR25 - some have short SP, some don't
> >> - AVRXMEGA2, AVRXMEGA4, AVRXMEGA5, AVRXMEGA6, AVRXMEGA7 -
> some have RMW, some don't
> >> - AVRXMEGA3 - some have RCALL, some don't
> >>
> >> I decided to leave CPU flavour definition as suggested by GCC
> gcc/config/avr/avr-devices.c file and when a specific MCU is
> created it will set / reset CPU features relevant to it.
> >>
> >> I hope this helps.
> >>
> >> Best Regards,
> >> Michael Rolnik
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> On Sat, Feb 8, 2020 at 9:35 AM Aleksandar Markovic
>  > wrote:
> >>>
> >>>
> >>>
> >>> On Sunday, February 2, 2020, Joaquin de Andres
> mailto:m...@xcancerberox.com.ar>> wrote:
> 
>  On 1/31/20 1:02 AM, Aleksandar Markovic wrote:
> >
> > From: Michael Rolnik  >
> >
> > AVR core types are:
> >
> >    - avr1
> >    - avr2
> >    - avr25
> >    - avr3
> >    - avr31
> >    - avr35
> >    - avr4
> >    - avr5
> >    - avr51
> >    - avr6
> >    - avrtiny
> >    - xmega2
> 

[RFC 1/1] cpus: extract out accel-specific code to each accel

[Claudio Fontana]

each accelerator registers a new "CpusAccel" interface
implementation on initialization, providing functions for
starting a vcpu, kicking a vcpu, and sychronizing state.

This way the code in cpus.c is now all general softmmu code,
nothing accelerator-specific anymore.

There is still some ifdeffery for WIN32 though.

Signed-off-by: Claudio Fontana 
---
 MAINTAINERS   |   1 +
 accel/Makefile.objs   |   2 +-
 accel/kvm/Makefile.objs   |   2 +
 accel/kvm/kvm-all.c   |  15 +-
 accel/kvm/kvm-cpus.c  |  94 +
 accel/kvm/kvm-cpus.h  |  17 +
 accel/qtest/Makefile.objs |   2 +
 accel/qtest/qtest-cpus.c  | 105 +
 accel/qtest/qtest-cpus.h  |  17 +
 accel/{ => qtest}/qtest.c |   7 +
 accel/stubs/kvm-stub.c|   3 +-
 accel/tcg/Makefile.objs   |   1 +
 accel/tcg/tcg-all.c   |  12 +-
 accel/tcg/tcg-cpus.c  | 532 +
 accel/tcg/tcg-cpus.h  |  17 +
 hw/core/cpu.c |   1 +
 include/sysemu/cpus.h |  33 ++
 include/sysemu/hw_accel.h |  57 +--
 include/sysemu/kvm.h  |   2 +-
 softmmu/cpus.c| 908 +++---
 stubs/Makefile.objs   |   1 +
 stubs/cpu-synchronize-state.c |  15 +
 target/i386/Makefile.objs |   7 +-
 target/i386/hax-all.c |   6 +-
 target/i386/hax-cpus.c|  85 
 target/i386/hax-cpus.h|  17 +
 target/i386/hax-i386.h|   2 +
 target/i386/hax-posix.c   |  12 +
 target/i386/hax-windows.c |  20 +
 target/i386/hvf/Makefile.objs |   2 +-
 target/i386/hvf/hvf-cpus.c| 141 +++
 target/i386/hvf/hvf-cpus.h|  17 +
 target/i386/hvf/hvf.c |   3 +
 target/i386/whpx-all.c|   3 +
 target/i386/whpx-cpus.c   |  96 +
 target/i386/whpx-cpus.h   |  17 +
 36 files changed, 1357 insertions(+), 915 deletions(-)
 create mode 100644 accel/kvm/kvm-cpus.c
 create mode 100644 accel/kvm/kvm-cpus.h
 create mode 100644 accel/qtest/Makefile.objs
 create mode 100644 accel/qtest/qtest-cpus.c
 create mode 100644 accel/qtest/qtest-cpus.h
 rename accel/{ => qtest}/qtest.c (86%)
 create mode 100644 accel/tcg/tcg-cpus.c
 create mode 100644 accel/tcg/tcg-cpus.h
 create mode 100644 stubs/cpu-synchronize-state.c
 create mode 100644 target/i386/hax-cpus.c
 create mode 100644 target/i386/hax-cpus.h
 create mode 100644 target/i386/hvf/hvf-cpus.c
 create mode 100644 target/i386/hvf/hvf-cpus.h
 create mode 100644 target/i386/whpx-cpus.c
 create mode 100644 target/i386/whpx-cpus.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d2a5f20963..c876cb39cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -427,6 +427,7 @@ WHPX CPUs
 M: Sunil Muthuswamy 
 S: Supported
 F: target/i386/whpx-all.c
+F: target/i386/whpx-cpus.c
 F: target/i386/whp-dispatch.h
 F: accel/stubs/whpx-stub.c
 F: include/sysemu/whpx.h
diff --git a/accel/Makefile.objs b/accel/Makefile.objs
index ff72f0d030..c5e58eb53d 100644
--- a/accel/Makefile.objs
+++ b/accel/Makefile.objs
@@ -1,5 +1,5 @@
 common-obj-$(CONFIG_SOFTMMU) += accel.o
-obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest.o
+obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest/
 obj-$(CONFIG_KVM) += kvm/
 obj-$(CONFIG_TCG) += tcg/
 obj-$(CONFIG_XEN) += xen/
diff --git a/accel/kvm/Makefile.objs b/accel/kvm/Makefile.objs
index fdfa481578..ce0f492b8d 100644
--- a/accel/kvm/Makefile.objs
+++ b/accel/kvm/Makefile.objs
@@ -1,2 +1,4 @@
 obj-y += kvm-all.o
+obj-y += kvm-cpus.o
+
 obj-$(call lnot,$(CONFIG_SEV)) += sev-stub.o
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d54a8701d8..f521a4d10a 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -45,6 +45,10 @@
 #include "qapi/qapi-types-common.h"
 #include "qapi/qapi-visit-common.h"
 #include "sysemu/reset.h"
+#include "qemu/guest-random.h"
+
+#include "sysemu/hw_accel.h"
+#include "kvm-cpus.h"
 
 #include "hw/boards.h"
 
@@ -379,7 +383,7 @@ err:
 return ret;
 }
 
-int kvm_destroy_vcpu(CPUState *cpu)
+static int do_kvm_destroy_vcpu(CPUState *cpu)
 {
 KVMState *s = kvm_state;
 long mmap_size;
@@ -413,6 +417,14 @@ err:
 return ret;
 }
 
+void kvm_destroy_vcpu(CPUState *cpu)
+{
+if (do_kvm_destroy_vcpu(cpu) < 0) {
+error_report("kvm_destroy_vcpu failed");
+exit(EXIT_FAILURE);
+}
+}
+
 static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
 {
 struct KVMParkedVcpu *cpu;
@@ -2232,6 +2244,7 @@ static int kvm_init(MachineState *ms)
 qemu_balloon_inhibit(true);
 }
 
+cpus_register_accel(&kvm_cpus);
 return 0;
 
 err:
diff --git a/accel/kvm/kvm-cpus.c b/accel/kvm/kvm-cpus.c
new file mode 100644
index 00..ac6945a9e6
--- /dev/null
+++ b/accel/kvm/kvm-cpus.c
@@ -0,0 +1,94 @@
+/*
+ * QEMU KVM support
+ *
+ * Copyright IBM, Corp. 2008
+ *   Red Hat, Inc. 2008
+ *
+ * Authors:
+ *  Anthony Liguori   
+ *  Glauber Costa 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 

[RFC 0/1] QEMU cpus.c refactoring part2

[Claudio Fontana]

Motivation and higher level steps:

https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

This is based on and requires "QEMU cpus.c refactoring part1":

https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg09401.html

OPEN ITEMS:

* needs review
* requires HVF patches before this can be committed (Roman).
* investigate splitting into multiple patches



* Rebased on commit 255ae6e2158c743717bed76c9a2365ee4bcd326e,
"replay: notify the main loop when there are no instructions"

[SPLIT into part1 and part2]



v6 -> v7:

* rebased changes on top of Pavel Dovgalyuk changes to dma-helpers.c
  "icount: make dma reads deterministic"



v5 -> v6:

* rebased changes on top of Emilio G. Cota changes to cpus.c
  "cpu: convert queued work to a QSIMPLEQ"

* keep a pointer in cpus.c instead of a copy of CpusAccel
  (Alex)




v4 -> v5: rebase on latest master

* rebased changes on top of roman series to remove one of the extra states for 
hvf.
  (Is the result now functional for HVF?)

* rebased changes on top of icount changes and fixes to icount_configure and
  the new shift vmstate. (Markus)

v3 -> v4:

* overall: added copyright headers to all files that were missing them
  (used copyright and license of the module the stuff was extracted from).
  For the new interface files, added SUSE LLC.

* 1/4 (move softmmu only files from root):

  MAINTAINERS: moved softmmu/cpus.c to its final location (from patch 2)

* 2/4 (cpu-throttle):

  MAINTAINERS (to patch 1),
  copyright Fabrice Bellard and license from cpus.c

* 3/4 (cpu-timers, icount):

  - MAINTAINERS: add cpu-timers.c and icount.c to Paolo

  - break very long lines (patchew)

  - add copyright SUSE LLC, GPLv2 to cpu-timers.h

  - add copyright Fabrice Bellard and license from cpus.c to timers-state.h
as it is lifted from cpus.c

  - vl.c: in configure_accelerators bail out if icount_enabled()
and !tcg_enabled() as qtest does not enable icount anymore.

* 4/4 (accel stuff to accel):

  - add copyright SUSE LLC to files that mostly only consist of the
new interface. Add whatever copyright was in the accelerator code
if instead they mostly consist of accelerator code.

  - change a comment to mention the result of the AccelClass experiment

  - moved qtest accelerator into accel/qtest/ , make it like the others.

  - rename xxx-cpus-interface to xxx-cpus (remove "interface" from names)

  - rename accel_int to cpus_accel

  - rename CpusAccel functions from cpu_synchronize_* to synchronize_*




v2 -> v3:

* turned into a 4 patch series, adding a first patch moving
  softmmu code currently in top_srcdir to softmmu/

* cpu-throttle: moved to softmmu/

* cpu-timers, icount:

  - moved to softmmu/

  - fixed assumption of qtest_enabled() => icount_enabled()
  causing the failure of check-qtest-arm goal, in test-arm-mptimer.c

  Fix is in hw/core/ptimer.c,

  where the artificial timeout rate limit should not be applied
  under qtest_enabled(), in a similar way to how it is not applied
  for icount_enabled().

* CpuAccelInterface: no change.





v1 -> v2:

* 1/3 (cpu-throttle): provide a description in the commit message

* 2/3 (cpu-timers, icount): in this v2 separate icount from cpu-timers,
  as icount is actually TCG-specific. Only build it under CONFIG_TCG.

  To do this, qtest had to be detached from icount. To this end, a
  trivial global counter for qtest has been introduced.

* 3/3 (CpuAccelInterface): provided a description.

This is point 8) in that plan. The idea is to extract the unrelated parts
in cpus, and register interfaces from each single accelerator to the main
cpus module (cpus.c).

While doing this RFC, I noticed some assumptions about Windows being
either TCG or HAX (not considering WHPX) that might need to be revisited.
I added a comment there.

The thing builds successfully based on Linux cross-compilations for
windows/hax, windows/whpx, and I got a good build on Darwin/hvf.

Tests run successully for tcg and kvm configurations, but did not test on
windows or darwin.

Welcome your feedback and help on this,

Claudio

Claudio Fontana (1):
  cpus: extract out accel-specific code to each accel

 MAINTAINERS   |   1 +
 accel/Makefile.objs   |   2 +-
 accel/kvm/Makefile.objs   |   2 +
 accel/kvm/kvm-all.c   |  15 +-
 accel/kvm/kvm-cpus.c  |  94 +
 accel/kvm/kvm-cpus.h  |  17 +
 accel/qtest/Makefile.objs |   2 +
 accel/qtest/qtest-cpus.c  | 105 +
 accel/qtest/qtest-cpus.h  |  17 +
 accel/{ => qtest}/qtest.c |   7 +
 accel/stubs/kvm-stub.c|   3 +-
 accel/tcg/Makefile.objs   |   1 +
 accel/tcg/tcg-all.c   |  12 +-
 accel/tcg/tcg-cpus.c  | 532 +
 accel/tcg/tcg-cpus.h  |  17 +
 hw/core/cpu.c |   1 +
 include/sysemu/cpus.h |  33 ++
 include/sysemu/hw_accel.h |  57 +--
 include/sysemu/kvm.h  |   2 +-
 softmmu/cpus.c

Re: [PATCH v3 19/30] gitlab: build all container images during CI

[Thomas Huth]

On 26/06/2020 20.13, Alex Bennée wrote:

From: Daniel P. Berrangé 

We have a number of container images in tests/docker/dockerfiles
that are intended to provide well defined environments for doing
test builds. We want our CI system to use these containers too.

This introduces builds of all of them as the first stage in the
CI, so that the built containers are available for later build
jobs. The containers are setup to use the GitLab container
registry as the cache, so we only pay the penalty of the full
build when the dockerfiles change. The main qemu-project/qemu
repo is used as a second cache, so that users forking QEMU will
see a fast turnaround time on their CI jobs.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20200622153318.751107-3-berra...@redhat.com>
[AJB: tweak the tag format]
Signed-off-by: Alex Bennée 
Acked-by: Thomas Huth 
---
  .gitlab-ci.d/containers.yml | 248 
  .gitlab-ci.yml  |   3 +
  2 files changed, 251 insertions(+)
  create mode 100644 .gitlab-ci.d/containers.yml

diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
new file mode 100644
index 000..580439647ef
--- /dev/null
+++ b/.gitlab-ci.d/containers.yml
@@ -0,0 +1,248 @@
+
+


Remove one or two empty lines here?


+.container_job_template: &container_job_definition
+  image: docker:stable
+  stage: containers
+  services:
+- docker:dind
+  before_script:
+- export TAG="$CI_REGISTRY_IMAGE/qemu/$NAME:latest"
+- export COMMON_TAG="$CI_REGISTRY/qemu-project/qemu/$NAME:latest"
+- docker info
+- docker login registry.gitlab.com -u "$CI_REGISTRY_USER" -p 
"$CI_REGISTRY_PASSWORD"
+  script:
+- docker pull "$TAG" || docker pull "$COMMON_TAG" || true
+- sed -i -e "s,FROM qemu/,FROM $CI_REGISTRY_IMAGE/qemu/," 
tests/docker/dockerfiles/$NAME.docker
+- docker build --cache-from "$TAG" --cache-from "$COMMON_TAG" --tag "$TAG" -f 
"tests/docker/dockerfiles/$NAME.docker" tests/docker/dockerfiles
+- docker push "$TAG"
+  after_script:
+- docker logout

[...]

+
+amd64-ubuntu-container:
+  <<: *container_job_definition
+  variables:
+NAME: ubuntu
+


"git am" complains:

Applying: gitlab: build all container images during CI
.git/rebase-apply/patch:260: new blank line at EOF.
+

... thus remove the trailing empty line?

 Thomas

Re: [PATCH v2 6/9] spapr_pci: add spapr msi read method

[Li Qiang]

David Gibson  于2020年6月25日周四 上午10:09写道：
>
> On Thu, Jun 25, 2020 at 12:25:20AM +0530, P J P wrote:
> > From: Prasad J Pandit 
> >
> > Add spapr msi mmio read method to avoid NULL pointer dereference
> > issue.
> >
> > Reported-by: Lei Sun 
> > Signed-off-by: Prasad J Pandit 
>

Reviewed-by: Li Qiang 

> Acked-by: David Gibson 
>
> > ---
> >  hw/ppc/spapr_pci.c | 13 +++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 329002ac04..7033352834 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -52,6 +52,7 @@
> >  #include "sysemu/kvm.h"
> >  #include "sysemu/hostmem.h"
> >  #include "sysemu/numa.h"
> > +#include "qemu/log.h"
> >
> >  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> >  #define RTAS_QUERY_FN   0
> > @@ -738,6 +739,12 @@ static PCIINTxRoute spapr_route_intx_pin_to_irq(void 
> > *opaque, int pin)
> >  return route;
> >  }
> >
> > +static uint64_t spapr_msi_read(void *opaque, hwaddr addr, unsigned size)
> > +{
> > +qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
> > +return 0;
> > +}
> > +
> >  /*
> >   * MSI/MSIX memory region implementation.
> >   * The handler handles both MSI and MSIX.
> > @@ -755,8 +762,10 @@ static void spapr_msi_write(void *opaque, hwaddr addr,
> >  }
> >
> >  static const MemoryRegionOps spapr_msi_ops = {
> > -/* There is no .read as the read result is undefined by PCI spec */
> > -.read = NULL,
> > +/* .read result is undefined by PCI spec
> > + * define .read method to avoid assert failure in memory_region_init_io
> > + */
> > +.read = spapr_msi_read,
> >  .write = spapr_msi_write,
> >  .endianness = DEVICE_LITTLE_ENDIAN
> >  };
>
> --
> David Gibson| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson

Re: [PULL 3/6] MAINTAINERS: Add Loongson-3 maintainer and reviewer

[Aleksandar Markovic]

понедељак, 29. јун 2020., Philippe Mathieu-Daudé  је
написао/ла:

> On 6/27/20 9:51 PM, Aleksandar Markovic wrote:
> > From: Huacai Chen 
> >
> > Add myself as the maintainer for Loongson-3 virtual platforms, and
> > also add Jiaxun Yang as the reviewer.
> >
> > Signed-off-by: Huacai Chen 
> > Co-developed-by: Jiaxun Yang 
> > Reviewed-by: Aleksandar Markovic 
> > Signed-off-by: Aleksandar Markovic 
> > Message-Id: <1592995531-32600-5-git-send-email-che...@lemote.com>
> > ---
> >  MAINTAINERS | 7 +++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1b40446..fe925ea 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1096,6 +1096,13 @@ F: hw/isa/vt82c686.c
> >  F: hw/pci-host/bonito.c
> >  F: include/hw/isa/vt82c686.h
> >
> > +Loongson-3 Virtual Platform
> > +M: Huacai Chen 
> > +R: Jiaxun Yang 
> > +S: Maintained
> > +F: hw/mips/loongson3_virt.c
>
> This file has not been commited, is this pull request incomplete?
>
> > +F: hw/intc/loongson_liointc.c
>
>
This line, in this patch, was my mistake. I did delete the line, but forgot
to do "git add MAINTAINERS". So, the deletion happened in the next patch
dealing with MAINTAINERS.

So, my bad. An honest mistake. I apoligize.

However, the final outcome of the pull request is 100% as desired, and as I
informed other in my responses to patches.  No need to do any follow ups.

How can we test this device?
>
>
Please read the commit message of patch 3/4 of Huacai's recent series v6.


> > +
> >  Boston
> >  M: Paul Burton 
> >  R: Aleksandar Rikalo 
> >
>
>

Re: [PATCH 06/10] trivial: Remove extra trailing whitespace

[Philippe Mathieu-Daudé]

On 6/26/20 6:43 PM, Christophe de Dinechin wrote:
> Signed-off-by: Christophe de Dinechin 
> ---
>  hw/display/qxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index d5627119ec..28caf878cd 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -51,7 +51,7 @@
>  #undef ALIGN
>  #define ALIGN(a, b) (((a) + ((b) - 1)) & ~((b) - 1))
>  
> -#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9" 
> +#define PIXEL_SIZE 0.2936875 //1280x1024 is 14.8" x 11.9"

>From CODING_STYLE.rst:

  Comment style
  =

  We use traditional C-style /``*`` ``*``/ comments and avoid // comments.

Let's switch to the C-style format :)

>  
>  #define QXL_MODE(_x, _y, _b, _o)  \
>  {   .x_res = _x,  \
> 

Re: [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR)

[Philippe Mathieu-Daudé]

Hi Yoshinori,

On 6/25/20 11:25 AM, Peter Maydell wrote:
> On Sun, 21 Jun 2020 at 13:54, Philippe Mathieu-Daudé  wrote:
>>
>> From: Yoshinori Sato 
>>
>> renesas_tmr: 8bit timer modules.
> 
> Hi; the recent Coverity run reports a potential bug in this
> code: (CID 1429976)
> 
> 
>> +static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch)
>> +{
>> +int64_t delta, now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +int elapsed, ovf = 0;
>> +uint16_t tcnt[2];
> 
> Here we declare tcnt[] but do not initialize its contents...
> 
>> +uint32_t ret;
>> +
>> +delta = (now - tmr->tick) * NANOSECONDS_PER_SECOND / tmr->input_freq;
>> +if (delta > 0) {
>> +tmr->tick = now;
>> +
>> +if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == INTERNAL) {
>> +/* timer1 count update */
>> +elapsed = elapsed_time(tmr, 1, delta);
>> +if (elapsed >= 0x100) {
>> +ovf = elapsed >> 8;
>> +}
>> +tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff);
>> +}
>> +switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) {
>> +case INTERNAL:
>> +elapsed = elapsed_time(tmr, 0, delta);
>> +tcnt[0] = tmr->tcnt[0] + elapsed;
>> +break;
>> +case CASCADING:
>> +if (ovf > 0) {
>> +tcnt[0] = tmr->tcnt[0] + ovf;
>> +}
>> +break;
>> +}
> 
> ...but not all cases here set both tcnt[0] and tcnt[1]
> (for instance in the "case CASCADING:" if ovf <=0 we
> won't set either of them)...
> 
>> +} else {
>> +tcnt[0] = tmr->tcnt[0];
>> +tcnt[1] = tmr->tcnt[1];
>> +}
>> +if (size == 1) {
>> +return tcnt[ch];
>> +} else {
>> +ret = 0;
>> +ret = deposit32(ret, 0, 8, tcnt[1]);
>> +ret = deposit32(ret, 8, 8, tcnt[0]);
>> +return ret;
> 
> ...and so here we will end up returning uninitialized
> data. Presumably the spec says what value is actually
> supposed to be returned in these cases?
> 
> PS: the "else" branch with the deposit32() calls could be
> rewritten more simply as
>   return lduw_be_p(tcnt);
> 
>> +static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
>> +{
> 
> In this function Coverity reports a missing "break" (CID 1429977):
> 
>> +case A_TCORA:
>> +if (size == 1) {
>> +return tmr->tcora[ch];
>> +} else if (ch == 0) {
>> +return concat_reg(tmr->tcora);
>> +}
> 
> Here execution can fall through but there is no 'break' or '/* fallthrough 
> */'.
> 
>> +case A_TCORB:
>> +if (size == 1) {
>> +return tmr->tcorb[ch];
>> +} else {
>> +return concat_reg(tmr->tcorb);
>> +}
> 
> Is it correct that the A_TCORA and A_TCORB code is different?
> It looks odd, so if this is intentional then a comment describing
> why it is so might be helpful to readers.

Can you address Peter's comments please?

> 
> thanks
> -- PMM
> 

Re: [PATCH QEMU v25 03/17] vfio: Add save and load functions for VFIO PCI devices

[Dr. David Alan Gilbert]

* Alex Williamson (alex.william...@redhat.com) wrote:
> On Fri, 26 Jun 2020 13:16:13 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Alex Williamson (alex.william...@redhat.com) wrote:
> > > On Wed, 24 Jun 2020 19:59:39 +0530
> > > Kirti Wankhede  wrote:
> > >   
> > > > On 6/23/2020 1:58 AM, Alex Williamson wrote:  
> > > > > On Sun, 21 Jun 2020 01:51:12 +0530
> > > > > Kirti Wankhede  wrote:
> > > > > 
> > > > >> These functions save and restore PCI device specific data - config
> > > > >> space of PCI device.
> > > > >> Tested save and restore with MSI and MSIX type.
> > > > >>
> > > > >> Signed-off-by: Kirti Wankhede 
> > > > >> Reviewed-by: Neo Jia 
> > > > >> ---
> > > > >>   hw/vfio/pci.c | 95 
> > > > >> +++
> > > > >>   include/hw/vfio/vfio-common.h |  2 +
> > > > >>   2 files changed, 97 insertions(+)
> > > > >>
> > > > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > > >> index 27f8872db2b1..5ba340aee1d4 100644
> > > > >> --- a/hw/vfio/pci.c
> > > > >> +++ b/hw/vfio/pci.c
> > > > >> @@ -41,6 +41,7 @@
> > > > >>   #include "trace.h"
> > > > >>   #include "qapi/error.h"
> > > > >>   #include "migration/blocker.h"
> > > > >> +#include "migration/qemu-file.h"
> > > > >>   
> > > > >>   #define TYPE_VFIO_PCI "vfio-pci"
> > > > >>   #define PCI_VFIO(obj)OBJECT_CHECK(VFIOPCIDevice, obj, 
> > > > >> TYPE_VFIO_PCI)
> > > > >> @@ -2407,11 +2408,105 @@ static Object 
> > > > >> *vfio_pci_get_object(VFIODevice *vbasedev)
> > > > >>   return OBJECT(vdev);
> > > > >>   }
> > > > >>   
> > > > >> +static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> > > > >> +{
> > > > >> +VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, 
> > > > >> vbasedev);
> > > > >> +PCIDevice *pdev = &vdev->pdev;
> > > > >> +
> > > > >> +qemu_put_buffer(f, vdev->emulated_config_bits, 
> > > > >> vdev->config_size);
> > > > >> +qemu_put_buffer(f, vdev->pdev.wmask, vdev->config_size);
> > > > >> +pci_device_save(pdev, f);
> > > > >> +
> > > > >> +qemu_put_be32(f, vdev->interrupt);
> > > > >> +if (vdev->interrupt == VFIO_INT_MSIX) {
> > > > >> +msix_save(pdev, f);
> > > > > 
> > > > > msix_save() checks msix_present() so shouldn't we include this
> > > > > unconditionally?  Can't there also be state in the vector table
> > > > > regardless of whether we're currently running in MSI-X mode?
> > > > > 
> > > > >> +}
> > > > >> +}
> > > > >> +
> > > > >> +static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> > > > >> +{
> > > > >> +VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, 
> > > > >> vbasedev);
> > > > >> +PCIDevice *pdev = &vdev->pdev;
> > > > >> +uint32_t interrupt_type;
> > > > >> +uint16_t pci_cmd;
> > > > >> +int i, ret;
> > > > >> +
> > > > >> +qemu_get_buffer(f, vdev->emulated_config_bits, 
> > > > >> vdev->config_size);
> > > > >> +qemu_get_buffer(f, vdev->pdev.wmask, vdev->config_size);
> > > > > 
> > > > > This doesn't seem safe, why is it ok to indiscriminately copy these
> > > > > arrays that are configured via support or masking of various device
> > > > > features from the source to the target?
> > > > > 
> > > > 
> > > > Ideally, software state at host should be restrored at destination - 
> > > > this is the attempt to do that.  
> > > 
> > > Or is it the case that both source and target should initialize these
> > > and come up with the same result and they should be used for
> > > validation, not just overwriting the target with the source?  
> > 
> > Is the request to have something similar to get_pci_config_device's
> > check where it compares the configs and c/w/w1c masks (see
> > hw/pci/pci.c:520 ish) - we get errors like:
> >Bad config data: i=0x read: ... device: ... cmask...
> > 
> > this is pretty good at spotting things where the source and destination
> > device are configured differently, but to allow other dynamic
> > configuration values to be passed through OK.
> 
> Yeah, except instead of validating we're just overwriting the
> destination currently.  Maybe we should make use of that directly.
> 
> I'm also not sure what the current best practice is for including
> device/feature specific information into the migration stream.  For
> example, if a new feature that's potentially only present on some
> devices includes emulation state that needs to be migrated we'd need a
> way to include that in the migration stream such that a target can fail
> if it doesn't understand that data or fail if it requires that data and
> it's not present.  What we have here seems very rigid, I don't see how
> we iterate on it with any chance of maintaining compatibility.  Any
> specific pointers to relevant examples?  Thanks,

That's what the 'subsection' mechanism in vmsd's allows.
It's a named part of a devices migration stream; if the destination
finds it receiving it without knowing what it is, it fails an

Re: [PATCH v3 29/30] gitlab: limit re-builds of the containers

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> Most of the time we are just rebuilding the same things. We can skip
> this although currently there is no mechanism for picking up new
> distro releases.
> 
> Rather than try to be too fine grained allow any change to trigger all
> the images being rebuilt.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  .gitlab-ci.d/containers.yml | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
> index 20b2fb1de5d..f56aa44f711 100644
> --- a/.gitlab-ci.d/containers.yml
> +++ b/.gitlab-ci.d/containers.yml
> @@ -19,6 +19,10 @@
>  - docker push "$TAG"
>after_script:
>  - docker logout
> +  only:
> +changes:
> +  - .gitlab-ci.d/containers.yml
> +  - tests/docker/*
>  
>  amd64-centos7-container:
><<: *container_job_definition
> 

Re: [PATCH v3 28/30] gitlab: split build-disabled into two phases

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> As we run check-qtest in "SLOW" mode this can timeout so split into
> two jobs.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  .gitlab-ci.yml | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index eb5b335c1e9..c6f1addc2f3 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -110,7 +110,7 @@ check:system-fedora-alt:
>  IMAGE: fedora
>  MAKE_CHECK_ARGS: check-acceptance
>  
> -build-disabled:
> +build:system-fedora-disabled:
><<: *native_build_job_definition
>variables:
>  IMAGE: fedora
> @@ -121,6 +121,17 @@ build-disabled:
>--disable-qom-cast-debug --disable-spice --disable-vhost-vsock
>--disable-vhost-net --disable-vhost-crypto --disable-vhost-user
>  TARGETS: i386-softmmu ppc64-softmmu mips64-softmmu i386-linux-user
> +  artifacts:
> +paths:
> +  - build
> +
> +qtest:system-fedora-disabled:
> +  <<: *native_test_job_definition
> +  needs:
> +- job: build:system-fedora-disabled
> +  artifacts: true
> +  variables:
> +IMAGE: fedora
>  MAKE_CHECK_ARGS: check-qtest SPEED=slow
>  
>  build-tcg-disabled:
> 

Re: [PATCH 13/46] qemu-option: Simplify around find_default_by_name()

[Vladimir Sementsov-Ogievskiy]

25.06.2020 16:12, Markus Armbruster wrote:

Eric Blake  writes:


On 6/24/20 11:43 AM, Markus Armbruster wrote:

Signed-off-by: Markus Armbruster 
---
   util/qemu-option.c | 13 -
   1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index ddcf3072c5..d9293814b4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -286,11 +286,9 @@ const char *qemu_opt_get(QemuOpts *opts, const char *name)
   opt = qemu_opt_find(opts, name);
   if (!opt) {
   def_val = find_default_by_name(opts, name);
-if (def_val) {
-return def_val;
-}
+return def_val;
   }
-return opt ? opt->str : NULL;
+return opt->str;
   }


You could go with even fewer lines and variables by inverting the logic:

if (opt) {
 return opt->str;
}
return find_default_by_name(opts, name);


Yes, that's better.


 void qemu_opt_iter_init(QemuOptsIter *iter, QemuOpts *opts,
const char *name)
@@ -320,7 +318,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
   {
   QemuOpt *opt;
   const char *def_val;
-char *str = NULL;
+char *str;
 if (opts == NULL) {
   return NULL;
@@ -329,10 +327,7 @@ char *qemu_opt_get_del(QemuOpts *opts, const char *name)
   opt = qemu_opt_find(opts, name);
   if (!opt) {
   def_val = find_default_by_name(opts, name);
-if (def_val) {
-str = g_strdup(def_val);
-}
-return str;
+return g_strdup(def_val);


Similarly, you could drop def_val with:
  return g_strdup(find_default_by_name(opts, name));


Your contracted version is still readable; sold.


Either way,
Reviewed-by: Eric Blake 




with suggested improvements:
Reviewed-by: Vladimir Sementsov-Ogievskiy 



--
Best regards,
Vladimir

Re: [PATCH v3 26/30] gitlab: enable check-tcg for linux-user tests

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> Switch to building in the new debian-all-test-cross image which has
> most of the cross compilers inline.
> 
> Signed-off-by: Alex Bennée 
> ---
>  .gitlab-ci.yml | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 5ae8130bd1a..17c3349dd9e 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -140,10 +140,9 @@ build-tcg-disabled:
>  build-user:
><<: *native_build_job_definition
>variables:
> -IMAGE: ubuntu2004
> -CONFIGURE_ARGS: --disable-system --disable-guest-agent
> -  --disable-capstone --disable-slirp --disable-fdt
> -MAKE_CHECK_ARGS:  run-tcg-tests-i386-linux-user 
> run-tcg-tests-x86_64-linux-user
> +IMAGE: debian-all-test-cross
> +CONFIGURE_ARGS: --disable-tools --disable-system
> +MAKE_CHECK_ARGS: check-tcg
>  
>  build-clang:
><<: *native_build_job_definition
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 16/30] .gitignore: un-ignore .gitlab-ci.d

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> The sooner we deprecate in-tree builds the sooner this mess of regexes
> can be thrown away.

:S

Reviewed-by: Philippe Mathieu-Daudé 

> 
> Signed-off-by: Alex Bennée 
> 
> ---
> v2
>   - just use explicit !/.gitlab-ci.d
> ---
>  .gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/.gitignore b/.gitignore
> index 90acb4347d4..2992d15931a 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -93,6 +93,7 @@
>  *.tp
>  *.vr
>  *.d
> +!/.gitlab-ci.d
>  !/scripts/qemu-guest-agent/fsfreeze-hook.d
>  *.o
>  .sdk
> 

Re: [PATCH v3 29/30] gitlab: limit re-builds of the containers

[Daniel P . Berrangé]

On Fri, Jun 26, 2020 at 07:13:56PM +0100, Alex Bennée wrote:
> Most of the time we are just rebuilding the same things. We can skip
> this although currently there is no mechanism for picking up new
> distro releases.
> 
> Rather than try to be too fine grained allow any change to trigger all
> the images being rebuilt.
> 
> Signed-off-by: Alex Bennée 
> ---
>  .gitlab-ci.d/containers.yml | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
> index 20b2fb1de5d..f56aa44f711 100644
> --- a/.gitlab-ci.d/containers.yml
> +++ b/.gitlab-ci.d/containers.yml
> @@ -19,6 +19,10 @@
>  - docker push "$TAG"
>after_script:
>  - docker logout
> +  only:
> +changes:
> +  - .gitlab-ci.d/containers.yml
> +  - tests/docker/*

How does this work for a person who forks the QEMU git repo and pushes
a change which doesn't touch the containers.yml file ?  AFAICT, all
their jobs will fail due to not having previously built any container
images in their brand new fork.

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

Re: [PATCH v3 15/30] tests/docker: change tag naming scheme of our images

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> We've been misusing the tag naming scheme for some time by overloading
> the post : section with the image type. Really it should be saved for
> the revision of that particular build. Move the details to the other
> side so we have:
> 
>   qemu/image-name
> 
> with the implied :latest version added by the tooling.
> 
> Suggested-by: Daniel P. Berrangé 
> Signed-off-by: Alex Bennée 
> 
> ---
> v2
>   - fix RUN invocation as well
>   - don't include :latest tag for debian-arm64-cross (implied)
> ---
>  tests/docker/Makefile.include   | 4 ++--
>  tests/docker/docker.py  | 2 +-
>  tests/docker/dockerfiles/debian-alpha-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-amd64-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-amd64.docker| 2 +-
>  tests/docker/dockerfiles/debian-arm64-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-arm64-test-cross.docker | 2 +-
>  tests/docker/dockerfiles/debian-armel-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-armhf-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-hppa-cross.docker   | 2 +-
>  tests/docker/dockerfiles/debian-m68k-cross.docker   | 2 +-
>  tests/docker/dockerfiles/debian-mips-cross.docker   | 2 +-
>  tests/docker/dockerfiles/debian-mips64-cross.docker | 2 +-
>  tests/docker/dockerfiles/debian-mips64el-cross.docker   | 2 +-
>  tests/docker/dockerfiles/debian-mipsel-cross.docker | 2 +-
>  tests/docker/dockerfiles/debian-powerpc-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-ppc64-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-ppc64el-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-riscv64-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-s390x-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-sh4-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-sparc64-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-tricore-cross.docker| 2 +-
>  tests/docker/dockerfiles/debian-win32-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian-win64-cross.docker  | 2 +-
>  tests/docker/dockerfiles/debian9-mxe.docker | 2 +-
>  26 files changed, 27 insertions(+), 27 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v3 09/30] tests/vm: change scripts to use self._config

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> From: Robert Foley 
> 
> This change converts existing scripts to using for example self.ROOT_PASS,
> to self._config['root_pass'].
> We made similar changes for GUEST_USER, and GUEST_PASS.
> This allows us also to remove the change in basevm.py,
> which adds __getattr__ for backwards compatibility.
> 
> Signed-off-by: Robert Foley 
> Reviewed-by: Peter Puhov 
> Signed-off-by: Alex Bennée 
> Message-Id: <20200601211421.1277-8-robert.fo...@linaro.org>
> ---
>  tests/vm/basevm.py | 11 ++-
>  tests/vm/fedora| 17 +
>  tests/vm/freebsd   | 16 
>  tests/vm/netbsd| 19 ++-
>  tests/vm/openbsd   | 17 +
>  5 files changed, 38 insertions(+), 42 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH 14/46] qemu-option: Factor out helper opt_create()

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

There is just one use so far.  The next commit will add more.

Signed-off-by: Markus Armbruster 
---
  util/qemu-option.c | 27 ++-
  1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index d9293814b4..3cdf0c0800 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -502,6 +502,23 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
  }
  }
  
+static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,

+   bool prepend)
+{
+QemuOpt *opt = g_malloc0(sizeof(*opt));


I'd prefer g_new0(QemuOpt, 1)

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy 


+
+opt->name = g_strdup(name);
+opt->str = value;
+opt->opts = opts;
+if (prepend) {
+QTAILQ_INSERT_HEAD(&opts->head, opt, next);
+} else {
+QTAILQ_INSERT_TAIL(&opts->head, opt, next);
+}
+
+return opt;
+}
+
  static void opt_set(QemuOpts *opts, const char *name, char *value,
  bool prepend, bool *help_wanted, Error **errp)
  {
@@ -519,16 +536,8 @@ static void opt_set(QemuOpts *opts, const char *name, char 
*value,
  return;
  }
  
-opt = g_malloc0(sizeof(*opt));

-opt->name = g_strdup(name);
-opt->opts = opts;
-if (prepend) {
-QTAILQ_INSERT_HEAD(&opts->head, opt, next);
-} else {
-QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-}
+opt = opt_create(opts, name, value, prepend);
  opt->desc = desc;
-opt->str = value;
  qemu_opt_parse(opt, &local_err);
  if (local_err) {
  error_propagate(errp, local_err);




--
Best regards,
Vladimir

Re: [PATCH] trivial: Respect alphabetical order of .o files in Makefile.objs

[Alex Bennée]

Christophe de Dinechin  writes:

> The vmgenid.o is the only file that is not in alphabetical order.
>
> Signed-off-by: Christophe de Dinechin 

Reviewed-by: Alex Bennée 

> ---
>  stubs/Makefile.objs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f32b9e47a3..1df8bb3814 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>  stub-obj-y += runstate-check.o
>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>  stub-obj-y += set-fd-handler.o
> -stub-obj-y += vmgenid.o
>  stub-obj-y += sysbus.o
>  stub-obj-y += tpm.o
>  stub-obj-y += trace-control.o
> +stub-obj-y += vmgenid.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o


-- 
Alex Bennée

Re: [PATCH v3 29/30] gitlab: limit re-builds of the containers

[Alex Bennée]

Daniel P. Berrangé  writes:

> On Fri, Jun 26, 2020 at 07:13:56PM +0100, Alex Bennée wrote:
>> Most of the time we are just rebuilding the same things. We can skip
>> this although currently there is no mechanism for picking up new
>> distro releases.
>> 
>> Rather than try to be too fine grained allow any change to trigger all
>> the images being rebuilt.
>> 
>> Signed-off-by: Alex Bennée 
>> ---
>>  .gitlab-ci.d/containers.yml | 4 
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
>> index 20b2fb1de5d..f56aa44f711 100644
>> --- a/.gitlab-ci.d/containers.yml
>> +++ b/.gitlab-ci.d/containers.yml
>> @@ -19,6 +19,10 @@
>>  - docker push "$TAG"
>>after_script:
>>  - docker logout
>> +  only:
>> +changes:
>> +  - .gitlab-ci.d/containers.yml
>> +  - tests/docker/*
>
> How does this work for a person who forks the QEMU git repo and pushes
> a change which doesn't touch the containers.yml file ?  AFAICT, all
> their jobs will fail due to not having previously built any container
> images in their brand new fork.

Hmm what we really need is a condition check to see if there is a local
registry with images in it.

>
> Regards,
> Daniel


-- 
Alex Bennée

Re: [PATCH v3 23/30] tests/docker: add packages needed for check-acceptance

[Philippe Mathieu-Daudé]

Hi Alex,

On 6/26/20 8:13 PM, Alex Bennée wrote:
> We need additional python packages to run check-acceptance. Add them
> to the docker images we will be using later.

I'm not sure everybody is interested by that. Maybe we could add an
extra 'acceptance-testing' Docker layer on top and use these images
when running acceptance tests.

Anyway this works so:
Reviewed-by: Philippe Mathieu-Daudé 

> 
> Signed-off-by: Alex Bennée 
> ---
>  tests/docker/dockerfiles/fedora.docker |  7 +++
>  tests/docker/dockerfiles/ubuntu2004.docker | 10 +-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/docker/dockerfiles/fedora.docker 
> b/tests/docker/dockerfiles/fedora.docker
> index 798ddd2c3e0..70b6186bd3e 100644
> --- a/tests/docker/dockerfiles/fedora.docker
> +++ b/tests/docker/dockerfiles/fedora.docker
> @@ -80,7 +80,12 @@ ENV PACKAGES \
>  pixman-devel \
>  python3 \
>  python3-PyYAML \
> +python3-numpy \
> +python3-opencv \
> +python3-pillow \
> +python3-pip \
>  python3-sphinx \
> +python3-virtualenv \
>  rdma-core-devel \
>  SDL2-devel \
>  snappy-devel \
> @@ -89,6 +94,8 @@ ENV PACKAGES \
>  systemd-devel \
>  systemtap-sdt-devel \
>  tar \
> +tesseract \
> +tesseract-langpack-eng \
>  texinfo \
>  usbredir-devel \
>  virglrenderer-devel \
> diff --git a/tests/docker/dockerfiles/ubuntu2004.docker 
> b/tests/docker/dockerfiles/ubuntu2004.docker
> index 6050ce7e8a8..f7aac840bf8 100644
> --- a/tests/docker/dockerfiles/ubuntu2004.docker
> +++ b/tests/docker/dockerfiles/ubuntu2004.docker
> @@ -46,9 +46,17 @@ ENV PACKAGES flex bison \
>  libxen-dev \
>  libzstd-dev \
>  make \
> -python3-yaml \
> +python3-numpy \
> +python3-opencv \
> +python3-pil \
> +python3-pip \
>  python3-sphinx \
> +python3-venv \
> +python3-yaml \
> +rpm2cpio \
>  sparse \
> +tesseract-ocr \
> +tesseract-ocr-eng \
>  texinfo \
>  xfslibs-dev\
>  vim
> 

Re: [PATCH 05/19] iotests.py: Add (verify|has)_working_luks()

[Maxim Levitsky]

On Thu, 2020-06-25 at 14:55 +0200, Max Reitz wrote:
> Similar to _require_working_luks for bash tests, these functions can be
> used to check whether our luks driver can actually create images.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py | 39 +++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index eee94e18cc..039170a8a3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -1052,6 +1052,45 @@ def verify_quorum():
>  if not supports_quorum():
>  notrun('quorum support missing')
>  
> +def has_working_luks() -> Tuple[bool, str]:
> +"""
> +Check whether our LUKS driver can actually create images
> +(this extends to LUKS encryption for qcow2).
> +
> +If not, return the reason why.
> +"""
> +
> +img_file = f'{test_dir}/luks-test.luks'
> +(output, status) = \
> +qemu_img_pipe_and_status('create', '-f', 'luks',
> + '--object', luks_default_secret_object,
> + '-o', luks_default_key_secret_opt,
> + '-o', 'iter-time=10',
> + img_file, '1G')
> +try:
> +os.remove(img_file)
> +except OSError:
> +pass
> +
> +if status != 0:
> +reason = output
> +for line in output.splitlines():
> +if img_file + ':' in line:
> +reason = line.split(img_file + ':', 1)[1].strip()
> +break
> +
> +return (False, reason)
> +else:
> +return (True, '')
> +
> +def verify_working_luks():
> +"""
> +Skip test suite if LUKS does not work
> +"""
> +(working, reason) = has_working_luks()
> +if not working:
> +notrun(reason)
> +
>  def qemu_pipe(*args):
>  """
>  Run qemu with an option to print something and exit (e.g. a help option).

It just occured to me that we can have a situation when luks driver is 
blacklisted
(via block driver blacklist "--block-drv-whitelist=") then this test.

THe whilelist only affects the qmp it seems so this check doesn't catch it, 
plus you could have case when luks driver is blacklisted but qcow2 isn't

When I build qemu with
'--block-drv-whitelist='raw,qcow2' I was able to break iotests 295 296
But this is such a non issue, that I am just noting for reference.

Reviewed-by: Maxim Levitsky 


Best regards,
Maxim Levitsky

Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Claudio Fontana]

Hello Christophe,

On 6/26/20 6:42 PM, Christophe de Dinechin wrote:
> In order to facilitate the move of large chunks of functionality to
> load modules, it is simpler to create a wrapper with the same name
> that simply relays the implementation. For efficiency, this is
> typically done using inline functions in the header for the
> corresponding functionality. In that case, we rename the actual
> implementation by appending _implementation to its name. This makes it
> easier to select which function you want to put a breakpoint on.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  include/qemu/module.h | 24 
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/qemu/module.h b/include/qemu/module.h
> index 011ae1ae76..1922a0293c 100644
> --- a/include/qemu/module.h
> +++ b/include/qemu/module.h
> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
> function(void)\
>  }
>  #endif
>  
> +#ifdef CONFIG_MODULES
> +/* Identify which functions are replaced by a callback stub */
> +#ifdef MODULE_STUBS
> +#define MODIFACE(Ret,Name,Args) \
> +Ret (*Name)Args;\
> +extern Ret Name##_implementation Args
> +#else /* !MODULE_STUBS */
> +#define MODIFACE(Ret,Name,Args) \
> +extern Ret (*Name)Args; \
> +extern Ret Name##_implementation Args
> +#endif /* MODULE_STUBS */
> +
> +#define MODIMPL(Ret,Name,Args)  \
> +static void __attribute__((constructor)) Name##_register(void)  \
> +{   \
> +Name = Name##_implementation;   \
> +}   \
> +Ret Name##_implementation Args
> +#else /* !CONFIG_MODULES */
> +/* When not using a module, such functions are called directly */
> +#define MODIFACE(Ret,Name,Args) Ret Name Args
> +#define MODIMPL(Ret,Name,Args)  Ret Name Args
> +#endif /* CONFIG_MODULES */
> +
>  typedef enum {
>  MODULE_INIT_MIGRATION,
>  MODULE_INIT_BLOCK,
> 

Just as background, I am interested in all modules-related work, because of my 
long term plan to have target-specific modules as well:

 https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html

I am not 100% clear on what is the goal and expected usage of this preprocessor 
code, despite the commit message,
maybe you could clarify a bit with more verbosity?

Additionally if you happen to be interested, maybe you know already or could 
think about what this could mean for target-specific modules,
which will require some improvements to the modules "subsystem"(?) as well.

In my experimentation I didn't have to do this preprocessor work, instead I had 
to fine tune a bit the makefile support (rules.mak and makefiles)
to be able to accomodate for (even large) modules in target/ as well.

Thanks!

CLaudio

Re: [PATCH v3 20/30] gitlab: convert jobs to use custom built containers

[Thomas Huth]

On 26/06/2020 20.13, Alex Bennée wrote:

From: Daniel P. Berrangé 

Now that we're building standard container images from
dockerfiles in tests/docker/dockerfiles, we can convert
the build jobs to use them. The key benefit of this is
that a contributor can now more easily replicate the CI
environment on their local machine. The container images
are cached too, so we are not spending time waiting for
the apt-get/dnf package installs to complete.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20200622153318.751107-4-berra...@redhat.com>
[AJB: tweak naming convention]
Signed-off-by: Alex Bennée 
---
  .gitlab-ci.yml | 187 +
  1 file changed, 81 insertions(+), 106 deletions(-)


Acked-by: Thomas Huth 

Re: [PATCH v3 24/30] gitlab: add acceptance testing to system builds

[Philippe Mathieu-Daudé]

Cc'ing Wainer

On 6/26/20 8:13 PM, Alex Bennée wrote:
> As part of migrating things from Travis to GitLab add the acceptance
> tests. To do this:
> 
>   - rename system1 to system-ubuntu-main
>   - rename system2 to system-fedora-misc
>   - split into build/check/acceptance
>   - remove -j from check stages
>   - use artifacts to save build stage
>   - add post acceptance template and use
> 
> Signed-off-by: Alex Bennée 
> Message-Id: <20200622143204.12921-16-alex.ben...@linaro.org>
> 
> ---
> v2
>   - updated with danp's docker changes
>   - use needs instead of dependancies
>   - touch all the build files to prevent rebuild
> ---
>  .gitlab-ci.yml | 66 +++---
>  .travis.yml| 23 --
>  2 files changed, 63 insertions(+), 26 deletions(-)
> 
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index a7abc55a5c6..5ae8130bd1a 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -1,8 +1,12 @@
> +# Currently we have two build stages after our containers are built:
> +#  - build (for traditional build and test or first stage build)
> +#  - test (for test stages, using build artefacts from a build stage)
>  stages:
>- containers
>- containers-layer2
>- containers-layer3
>- build
> +  - test
>  
>  include:
>- local: '/.gitlab-ci.d/edk2.yml'
> @@ -24,26 +28,82 @@ include:
>  ../configure --enable-werror $CONFIGURE_ARGS ;
>fi
>  - make -j"$JOBS"
> -- make -j"$JOBS" $MAKE_CHECK_ARGS
> +- if test -n "$MAKE_CHECK_ARGS";
> +  then
> +make $MAKE_CHECK_ARGS ;
> +  fi
> +
> +.native_test_job_template: &native_test_job_definition
> +  stage: test
> +  image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
> +  script:
> +- cd build
> +- find . -type f -exec touch {} +
> +- make $MAKE_CHECK_ARGS
> +
> +.post_acceptance_template: &post_acceptance
> +  after_script:
> +- python3 -c 'import json; r = 
> json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) 
> for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat

Wainer, I guess remember you said you'd send a script to do that
instead, right?

> +- du -chs $HOME/avocado/data/cache
>  
> -build-system1:
> +build:system-ubuntu-main:
><<: *native_build_job_definition
>variables:
>  IMAGE: ubuntu2004
>  TARGETS: aarch64-softmmu alpha-softmmu cris-softmmu hppa-softmmu 
> lm32-softmmu
>moxie-softmmu microblazeel-softmmu mips64el-softmmu m68k-softmmu 
> ppc-softmmu
>riscv64-softmmu sparc-softmmu
> +  artifacts:
> +paths:
> +  - build
> +
> +check:system-ubuntu-main:
> +  <<: *native_test_job_definition
> +  needs:
> +- job: build:system-ubuntu-main
> +  artifacts: true
> +  variables:
> +IMAGE: ubuntu2004
>  MAKE_CHECK_ARGS: check
>  
> -build-system2:
> +acceptance:system-ubuntu-main:
> +  <<: *native_test_job_definition
> +  needs:
> +- job: build:system-ubuntu-main
> +  artifacts: true
> +  variables:
> +IMAGE: ubuntu2004

(here I'd use ubuntu2004:acceptance)

> +MAKE_CHECK_ARGS: check-acceptance
> +
> +build:system-fedora-alt:
><<: *native_build_job_definition
>variables:
>  IMAGE: fedora
>  TARGETS: tricore-softmmu unicore32-softmmu microblaze-softmmu 
> mips-softmmu
>riscv32-softmmu s390x-softmmu sh4-softmmu sparc64-softmmu 
> x86_64-softmmu
>xtensa-softmmu nios2-softmmu or1k-softmmu
> +  artifacts:
> +paths:
> +  - build
> +
> +check:system-fedora-alt:
> +  <<: *native_test_job_definition
> +  needs:
> +- job: build:system-fedora-alt
> +  artifacts: true
> +  variables:
> +IMAGE: fedora
>  MAKE_CHECK_ARGS: check
>  
> +check:system-fedora-alt:
> +  <<: *native_test_job_definition
> +  needs:
> +- job: build:system-fedora-alt
> +  artifacts: true
> +  variables:
> +IMAGE: fedora

(here I'd use fedora:acceptance)

> +MAKE_CHECK_ARGS: check-acceptance
> +
>  build-disabled:
><<: *native_build_job_definition
>variables:
> diff --git a/.travis.yml b/.travis.yml
> index 74158f741b1..c24dfbe377f 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -289,29 +289,6 @@ jobs:
>python: 3.6
>  
>  
> -# Acceptance (Functional) tests
> -- name: "GCC check-acceptance"
> -  dist: bionic
> -  env:
> -- CONFIG="--enable-tools 
> --target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sh4-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
> -- TEST_CMD="make check-acceptance"
> -- CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-acceptance"
> -  after_script:
> -- python3 -c 'import json; r = 
> json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) 
> for t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
> -- du -chs $HOME/avocado/data/cache
> - 

Re: [PATCH v3 23/30] tests/docker: add packages needed for check-acceptance

[Thomas Huth]

On 26/06/2020 20.13, Alex Bennée wrote:

We need additional python packages to run check-acceptance. Add them
to the docker images we will be using later.

Signed-off-by: Alex Bennée 
---
  tests/docker/dockerfiles/fedora.docker |  7 +++
  tests/docker/dockerfiles/ubuntu2004.docker | 10 +-
  2 files changed, 16 insertions(+), 1 deletion(-)


Reviewed-by: Thomas Huth 

Re: [PATCH v3 25/30] tests/docker: add a linux-user testing focused image

[Philippe Mathieu-Daudé]

On 6/26/20 8:13 PM, Alex Bennée wrote:
> We happily use all the cross images for both cross-building QEMU as
> well as building the linux-user tests. However calling docker from
> within docker seems not to work. As we can build in Debian anyway why
> not include an image that has all the compilers available for
> non-docker invocation.
> 
> Signed-off-by: Alex Bennée 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  .gitlab-ci.d/containers.yml   |  7 +++
>  tests/docker/Makefile.include |  1 +
>  .../dockerfiles/debian-all-test-cross.docker  | 53 +++
>  3 files changed, 61 insertions(+)
>  create mode 100644 tests/docker/dockerfiles/debian-all-test-cross.docker
> 
> diff --git a/.gitlab-ci.d/containers.yml b/.gitlab-ci.d/containers.yml
> index ba9c24e98f1..20b2fb1de5d 100644
> --- a/.gitlab-ci.d/containers.yml
> +++ b/.gitlab-ci.d/containers.yml
> @@ -66,6 +66,13 @@ amd64-debian-cross-container:
>variables:
>  NAME: debian-amd64-cross
>  
> +amd64-debian-user-cross-container:
> +  <<: *container_job_definition
> +  stage: containers-layer2
> +  needs: ['amd64-debian10-container']
> +  variables:
> +NAME: debian-all-test-cross
> +
>  amd64-debian-container:
><<: *container_job_definition
>stage: containers-layer2
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 8139e8467d4..079ceb6ff33 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -133,6 +133,7 @@ docker-image-travis: NOUSER=1
>  
>  # Specialist build images, sometimes very limited tools
>  docker-image-debian-tricore-cross: docker-image-debian9
> +docker-image-debian-all-test-cross: docker-image-debian10
>  docker-image-debian-arm64-test-cross: docker-image-debian11
>  
>  # These images may be good enough for building tests but not for test builds
> diff --git a/tests/docker/dockerfiles/debian-all-test-cross.docker 
> b/tests/docker/dockerfiles/debian-all-test-cross.docker
> new file mode 100644
> index 000..dedcea58b46
> --- /dev/null
> +++ b/tests/docker/dockerfiles/debian-all-test-cross.docker
> @@ -0,0 +1,53 @@
> +#
> +# Docker all cross-compiler target (tests only)
> +#
> +# While the normal cross builds take care to setup proper multiarch
> +# build environments which can cross build QEMU this just installs the
> +# basic compilers for as many targets as possible. We shall use this
> +# to build and run linux-user tests on GitLab
> +#
> +FROM qemu/debian10
> +
> +# What we need to build QEMU itself
> +RUN apt update && \
> +DEBIAN_FRONTEND=noninteractive eatmydata \
> +apt build-dep -yy qemu
> +
> +# Add the foreign architecture we want and install dependencies
> +RUN DEBIAN_FRONTEND=noninteractive eatmydata \
> +apt install -y --no-install-recommends \
> +gcc-aarch64-linux-gnu \
> +libc6-dev-arm64-cross \
> +gcc-alpha-linux-gnu \
> +libc6.1-dev-alpha-cross \
> +gcc-arm-linux-gnueabihf \
> +libc6-dev-armhf-cross \
> +gcc-hppa-linux-gnu \
> +libc6-dev-hppa-cross \
> +gcc-m68k-linux-gnu \
> +libc6-dev-m68k-cross \
> +gcc-mips-linux-gnu \
> +libc6-dev-mips-cross \
> +gcc-mips64-linux-gnuabi64 \
> +libc6-dev-mips64-cross \
> +gcc-mips64el-linux-gnuabi64 \
> +libc6-dev-mips64el-cross \
> +gcc-mipsel-linux-gnu \
> +libc6-dev-mipsel-cross \
> +gcc-powerpc-linux-gnu \
> +libc6-dev-powerpc-cross \
> +gcc-powerpc64-linux-gnu \
> +libc6-dev-ppc64-cross \
> +gcc-powerpc64le-linux-gnu \
> +libc6-dev-ppc64el-cross \
> +gcc-riscv64-linux-gnu \
> +libc6-dev-riscv64-cross \
> +gcc-s390x-linux-gnu \
> +libc6-dev-s390x-cross \
> +gcc-sh4-linux-gnu \
> +libc6-dev-sh4-cross \
> +gcc-sparc64-linux-gnu \
> +libc6-dev-sparc64-cross
> +
> +ENV QEMU_CONFIGURE_OPTS --disable-system --disable-docs --disable-tools
> +ENV DEF_TARGET_LIST 
> aarch64-linux-user,alpha-linux-user,arm-linux-user,hppa-linux-user,i386-linux-user,m68k-linux-user,mips-linux-user,mips64-linux-user,mips64el-linux-user,mipsel-linux-user,ppc-linux-user,ppc64-linux-user,ppc64le-linux-user,riscv64-linux-user,s390x-linux-user,sh4-linux-user,sparc64-linux-user
> 

Re: [PATCH] trivial: Respect alphabetical order of .o files in Makefile.objs

[Philippe Mathieu-Daudé]

On 6/29/20 11:49 AM, Christophe de Dinechin wrote:
> The vmgenid.o is the only file that is not in alphabetical order.
> 
> Signed-off-by: Christophe de Dinechin 
> ---
>  stubs/Makefile.objs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f32b9e47a3..1df8bb3814 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -19,10 +19,10 @@ stub-obj-y += replay.o
>  stub-obj-y += runstate-check.o
>  stub-obj-$(CONFIG_SOFTMMU) += semihost.o
>  stub-obj-y += set-fd-handler.o
> -stub-obj-y += vmgenid.o
>  stub-obj-y += sysbus.o
>  stub-obj-y += tpm.o
>  stub-obj-y += trace-control.o
> +stub-obj-y += vmgenid.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_SOFTMMU) += win32-kbd-hook.o
>  
> 

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/3] block: Add bdrv_co_get_lba_status

[Stefan Hajnoczi]

On Thu, Jun 25, 2020 at 01:07:14PM +, Lin Ma wrote:
> On 2020-06-25 20:59, Lin Ma wrote:
> > Please initialize is_deallocated to false at the beginning of the
> > function to avoid accidental uninitialized variable accesses in the
> > caller.
> 
> It has already been initialized to 0 by 'data = g_new0(GetLbaStatusCBData, 
> 1);'
> in function scsi_disk_emulate_get_lba_status of patch 3/3, Do I still need to
> initialize it at the beginning of this function?

It's safer to set output arguments in all code paths. That way callers
don't need to remember to initialize the variable to a specific value
(false in this case).

You can leave this assumptnion but please include it in the doc comment
so it's clear that callers are responsible for setting it to false.

Stefan


signature.asc
Description: PGP signature

[REPORT] [GSoC - TCG Continuous Benchmarking] [#2] Dissecting QEMU Into Three Main Parts

[Ahmed Karaman]

Hi,

The second report of the TCG Continuous Benchmarking series builds
upon the QEMU performance metrics calculated in the previous report.
This report presents a method to dissect the number of instructions
executed by a QEMU invocation into three main phases:
- Code Generation
- JIT Execution
- Helpers Execution
It devises a Python script that automates this process.

After that, the report presents an experiment for comparing the
output of running the script on 17 different targets. Many conclusions
can be drawn from the results and two of them are discussed in the
analysis section.

Report link:
https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/Dissecting-QEMU-Into-Three-Main-Parts/

Previous reports:
Report 1 - Measuring Basic Performance Metrics of QEMU:
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06692.html

Best regards,
Ahmed Karaman

[PATCH RFC] virtio-fs: force virtio 1.x usage

[Cornelia Huck]

virtio-fs devices are only specified for virtio-1, so it is unclear
how a legacy or transitional device should behave.

Signed-off-by: Cornelia Huck 
---

Forcing off legacy now (after the virtio-fs device has already been
available) may have unintended consequences, therefore RFC.

By default, a virtio-pci device uses 'AUTO' for disable_legacy, which
will resolve to different values based upon which bus the device has
been plugged. Therefore, forcing disable_legacy may result in the same
device or a quite different one.

Even though pre-virtio-1 behaviour of virtio-fs devices is simply not
specified, toggling disable_legacy will have implications for the BAR
layout, IIRC, and therefore a guest might end up getting a different
device, even if it always used it with virtio-1 anyway.

Not sure what the best way to solve this problem is. Adding a compat
property for disable_legacy=AUTO may be the right thing to do, but I'm
not quite clear if there are any further implications here.

Whatever we do here, we should make sure that the ccw incarnation of
this device indeed forces virtio-1.

---
 hw/virtio/vhost-user-fs-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-user-fs-pci.c b/hw/virtio/vhost-user-fs-pci.c
index e11c889d82b3..244205edf765 100644
--- a/hw/virtio/vhost-user-fs-pci.c
+++ b/hw/virtio/vhost-user-fs-pci.c
@@ -44,6 +44,7 @@ static void vhost_user_fs_pci_realize(VirtIOPCIProxy 
*vpci_dev, Error **errp)
 vpci_dev->nvectors = dev->vdev.conf.num_request_queues + 2;
 }
 
+virtio_pci_force_virtio_1(vpci_dev);
 qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
 }
 
-- 
2.25.4

Re: [PULL 6/6] MAINTAINERS: Add 'Performance Tools and Tests' subsection

[Aleksandar Markovic]

понедељак, 29. јун 2020., Philippe Mathieu-Daudé  је
написао/ла:

> On 6/27/20 9:51 PM, Aleksandar Markovic wrote:
> > This commit creates a new 'Miscellaneous' section which hosts a new
> > 'Performance Tools and Tests' subsection. This subsection will contain
> > the the performance scripts and benchmarks written as a part of the
> > 'TCG Continuous Benchmarking' project. Also, it will be a placeholder
> > for follow-ups to this project, if any.
> >
> > Signed-off-by: Ahmed Karaman 
> > Reviewed-by: Alex Bennée 
> > Reviewed-by: Aleksandar Markovic 
> > Signed-off-by: Aleksandar Markovic 
> > Message-Id: <20200626164546.22102-4-ahmedkhaledkara...@gmail.com>
> > ---
> >  MAINTAINERS | 10 --
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index fe925ea..dec252f 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1096,11 +1096,10 @@ F: hw/isa/vt82c686.c
> >  F: hw/pci-host/bonito.c
> >  F: include/hw/isa/vt82c686.h
> >
> > -Loongson-3 Virtual Platform
> > +Loongson-3 virtual platforms
> >  M: Huacai Chen 
> >  R: Jiaxun Yang 
> >  S: Maintained
> > -F: hw/mips/loongson3_virt.c
>
> Ah, now I see, here you unlist the uncommited file.
>
>
This file/machine, for multiple reasons, will not be accepted in 5.1. The
end user will not be able to see, let alone use the machine in question in
5.1. This is on purpose.

This will give enough development time to Huacai to implement missing bits
and pieces, and to possibly improve the whole idea during 5.2 development
cycle.

Even though the support that Huacai introduced in 5.1 is, for many reasons,
not completed, and even though Huacai and I had at times opposing views and
fierce discussions and disagreements - I salute and support his work, and
consider it, by far, the best and the most important contribution to QEMU
for MIPS in years.



> It might be easier to manage sending 2 different pull requests,
> on for MIPS and one for the performance tools.
>
> >  F: hw/intc/loongson_liointc.c
> >
> >  Boston
> > @@ -3026,3 +3025,10 @@ M: Peter Maydell 
> >  S: Maintained
> >  F: docs/conf.py
> >  F: docs/*/conf.py
> > +
> > +Miscellaneous
> > +-
> > +Performance Tools and Tests
> > +M: Ahmed Karaman 
>
> Aleksandar, don't you want to be listed here with Ahmed?
>
>
Of course not. The project is student's. The mentor just helps and leads
the student. The fruits of the project belong to the community and to the
student - and not to the mentor.

If you were the mentor, my impression is that you would leave your name in
MAINTAINERS whenever you see the slightest opportunity (I remember an
occasion where you were saying "I worked six hours on this" and then
proposing yourself as the maitainer for a particular segment (??)). I don't
have such approach, and I oppose your approach, and I appeal to you to
control your apetite for maintainership.

Regards,
Aleksandar



> > +S: Maintained
> > +F: scripts/performance/
> >
>
>

Re: [PATCH v3 24/30] gitlab: add acceptance testing to system builds

[Thomas Huth]

On 26/06/2020 20.13, Alex Bennée wrote:

As part of migrating things from Travis to GitLab add the acceptance
tests. To do this:

   - rename system1 to system-ubuntu-main
   - rename system2 to system-fedora-misc
   - split into build/check/acceptance
   - remove -j from check stages
   - use artifacts to save build stage
   - add post acceptance template and use

Signed-off-by: Alex Bennée 
Message-Id: <20200622143204.12921-16-alex.ben...@linaro.org>

---
v2
   - updated with danp's docker changes
   - use needs instead of dependancies
   - touch all the build files to prevent rebuild
---
  .gitlab-ci.yml | 66 +++---
  .travis.yml| 23 --
  2 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a7abc55a5c6..5ae8130bd1a 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -1,8 +1,12 @@
+# Currently we have two build stages after our containers are built:
+#  - build (for traditional build and test or first stage build)
+#  - test (for test stages, using build artefacts from a build stage)
  stages:
- containers
- containers-layer2
- containers-layer3
- build
+  - test
  
  include:

- local: '/.gitlab-ci.d/edk2.yml'
@@ -24,26 +28,82 @@ include:
  ../configure --enable-werror $CONFIGURE_ARGS ;
fi
  - make -j"$JOBS"
-- make -j"$JOBS" $MAKE_CHECK_ARGS
+- if test -n "$MAKE_CHECK_ARGS";
+  then
+make $MAKE_CHECK_ARGS ;
+  fi
+
+.native_test_job_template: &native_test_job_definition
+  stage: test
+  image: $CI_REGISTRY_IMAGE/qemu/$IMAGE:latest
+  script:
+- cd build
+- find . -type f -exec touch {} +
+- make $MAKE_CHECK_ARGS
+
+.post_acceptance_template: &post_acceptance
+  after_script:
+- python3 -c 'import json; r = json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for t in 
r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
+- du -chs $HOME/avocado/data/cache
  
-build-system1:

+build:system-ubuntu-main:
<<: *native_build_job_definition
variables:
  IMAGE: ubuntu2004
  TARGETS: aarch64-softmmu alpha-softmmu cris-softmmu hppa-softmmu 
lm32-softmmu
moxie-softmmu microblazeel-softmmu mips64el-softmmu m68k-softmmu 
ppc-softmmu
riscv64-softmmu sparc-softmmu
+  artifacts:
+paths:
+  - build
+
+check:system-ubuntu-main:
+  <<: *native_test_job_definition
+  needs:
+- job: build:system-ubuntu-main
+  artifacts: true
+  variables:
+IMAGE: ubuntu2004
  MAKE_CHECK_ARGS: check
  
-build-system2:

+acceptance:system-ubuntu-main:
+  <<: *native_test_job_definition
+  needs:
+- job: build:system-ubuntu-main
+  artifacts: true
+  variables:
+IMAGE: ubuntu2004
+MAKE_CHECK_ARGS: check-acceptance
+
+build:system-fedora-alt:
<<: *native_build_job_definition
variables:
  IMAGE: fedora
  TARGETS: tricore-softmmu unicore32-softmmu microblaze-softmmu mips-softmmu
riscv32-softmmu s390x-softmmu sh4-softmmu sparc64-softmmu x86_64-softmmu
xtensa-softmmu nios2-softmmu or1k-softmmu
+  artifacts:
+paths:
+  - build
+
+check:system-fedora-alt:
+  <<: *native_test_job_definition
+  needs:
+- job: build:system-fedora-alt
+  artifacts: true
+  variables:
+IMAGE: fedora
  MAKE_CHECK_ARGS: check
  
+check:system-fedora-alt:

+  <<: *native_test_job_definition
+  needs:
+- job: build:system-fedora-alt
+  artifacts: true
+  variables:
+IMAGE: fedora
+MAKE_CHECK_ARGS: check-acceptance


Why is Ubuntu "-main" and "Fedora "-alt" ? ... that does not make sense 
to me.


We also might want to rework the list of targets. To speed up the 
testing, I originally omitted some targets like sh4eb-softmmu which did 
not seem very interesting, but now that we add more and more builds in 
parallel, we could maybe split the two system targets into three or even 
four instead, and then add these targets, too. It would also be nice to 
have some tests with "centos8" and a debian container, too.


And "rx-softmmu" is also still missing in the target list.

Ok, it's quite a bit of change that still needs to be done here ... 
maybe that's rather something for a separate patch later.


 Thomas

Re: [PATCH 15/46] qemu-option: Tidy up opt_set() not to free arguments on failure

[Vladimir Sementsov-Ogievskiy]

24.06.2020 19:43, Markus Armbruster wrote:

opt_set() frees its argument @value on failure.  Slightly unclean;
functions ideally do nothing on failure.

To tidy this up, move opt_create() from opt_set() into its callers,
along with the cleanup.


Hmm, let me think a bit..

So, prior to this patch:

opt_set gets name/value pair and sets the option in opts object, it
seems absolutely obvious and standard behavior for Map-like object.

The fact that for setting an option we create a QemuOpt object, and
somehow register it inside opts object is an implementation detail.

after the patch:

opt_set gets opt object, which is already registered in opts. So,
it seems like option is "partly" set already, and opt_set only
finalize the processing.

And, as opt_set() only finalize the "set" operation, on opt_set
failure we need additional roll-back of "set" operation first step.

Additional fact, indirectly showing that something is unclear here
is that we pass "opts" to opt_set twice: as "opts" parameter and
inside opt: (opt->opts must be the same, assertion won't hurt if
you decide to keep the patch).

=

Semantics before the patch seems clearer to me.

To improve the situation around "value", we can just g_strdup it
in opt_create as well as "name" argument (and use const char*
type for "value" argument as well)



Signed-off-by: Markus Armbruster 
---
  util/qemu-option.c | 33 ++---
  1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 3cdf0c0800..14946e81f2 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -519,36 +519,39 @@ static QemuOpt *opt_create(QemuOpts *opts, const char 
*name, char *value,
  return opt;
  }
  
-static void opt_set(QemuOpts *opts, const char *name, char *value,

-bool prepend, bool *help_wanted, Error **errp)
+static bool opt_set(QemuOpts *opts, QemuOpt *opt, bool *help_wanted,
+Error **errp)
  {
-QemuOpt *opt;
  const QemuOptDesc *desc;
  Error *local_err = NULL;
  
-desc = find_desc_by_name(opts->list->desc, name);

+desc = find_desc_by_name(opts->list->desc, opt->name);
  if (!desc && !opts_accepts_any(opts)) {
-g_free(value);
-error_setg(errp, QERR_INVALID_PARAMETER, name);
-if (help_wanted && is_help_option(name)) {
+error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
+if (help_wanted && is_help_option(opt->name)) {
  *help_wanted = true;
  }
-return;
+return false;
  }
  
-opt = opt_create(opts, name, value, prepend);

  opt->desc = desc;
  qemu_opt_parse(opt, &local_err);
  if (local_err) {
  error_propagate(errp, local_err);
-qemu_opt_del(opt);
+return false;
  }
+
+return true;
  }
  
  void qemu_opt_set(QemuOpts *opts, const char *name, const char *value,

Error **errp)
  {
-opt_set(opts, name, g_strdup(value), false, NULL, errp);
+QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
+
+if (!opt_set(opts, opt, NULL, errp)) {
+qemu_opt_del(opt);
+}
  }
  
  void qemu_opt_set_bool(QemuOpts *opts, const char *name, bool val,

@@ -820,9 +823,9 @@ static void opts_do_parse(QemuOpts *opts, const char 
*params,
const char *firstname, bool prepend,
bool *help_wanted, Error **errp)
  {
-Error *local_err = NULL;
  char *option, *value;
  const char *p;
+QemuOpt *opt;
  
  for (p = params; *p;) {

  p = get_opt_name_value(p, firstname, &option, &value);
@@ -834,10 +837,10 @@ static void opts_do_parse(QemuOpts *opts, const char 
*params,
  continue;
  }
  
-opt_set(opts, option, value, prepend, help_wanted, &local_err);

+opt = opt_create(opts, option, value, prepend);
  g_free(option);
-if (local_err) {
-error_propagate(errp, local_err);
+if (!opt_set(opts, opt, help_wanted, errp)) {
+qemu_opt_del(opt);
  return;
  }
  }




--
Best regards,
Vladimir

Re: [PATCH v2 3/3] scsi-disk: Add support for the GET LBA STATUS 16 command

[Stefan Hajnoczi]

On Thu, Jun 25, 2020 at 01:31:56PM +, Lin Ma wrote:
> On 2020-06-25 21:25, Lin Ma wrote:
> >> +/*
> >> + * 8 + 16 is the length in bytes of response header and
> >> + * one LBA status descriptor
> >> + */
> >> +memset(outbuf, 0, 8 + 16);
> >> +outbuf[3] = 20;
> >> +outbuf[8] = (req->cmd.lba >> 56) & 0xff;
> >> +outbuf[9] = (req->cmd.lba >> 48) & 0xff;
> >> +outbuf[10] = (req->cmd.lba >> 40) & 0xff;
> >> +outbuf[11] = (req->cmd.lba >> 32) & 0xff;
> >> +outbuf[12] = (req->cmd.lba >> 24) & 0xff;
> >> +outbuf[13] = (req->cmd.lba >> 16) & 0xff;
> >> +outbuf[14] = (req->cmd.lba >> 8) & 0xff;
> >> +outbuf[15] = req->cmd.lba & 0xff;
> >> +outbuf[16] = (*num_blocks >> 24) & 0xff;
> >> +outbuf[17] = (*num_blocks >> 16) & 0xff;
> >> +outbuf[18] = (*num_blocks >> 8) & 0xff;
> >> +outbuf[19] = *num_blocks & 0xff;
> >> +outbuf[20] = *is_deallocated ? 1 : 0;
> > 
> > SCSI defines 3 values and QEMU can represent all of them:
> > 
> > 0 - mapped or unknown
> > 1 - deallocated
> > 2 - anchored
> > 
> > See the BDRV_BLOCK_* constants in include/block/block.h. The
> > is_deallocated boolean is not enough to represent this state, but the
> > bdrv_block_status() return value can be used instead.
> 
> I don't know which one in BDRV_BLOCK_* can be used to represent 'anchored'.
> It seems that I need to use BDRV_BLOCK_* combination to recognized 'anchored',
> 
> I'd like to use these combinations to analyze the bdrv_block_status() return 
> value:
> 'deallocated': BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | 
> BDRV_BLOCK_ZERO
> 'anchored':BDRV_BLOCK_ALLOCATED | BDRV_BLOCK_OFFSET_VALID | ! 
> BDRV_BLOCK_ZERO | ! BDRV_BLOCK_DATA
> Am I right?

My understanding is that the SCSI status are mapped to QEMU block status
as follows:

allocated: BDRV_BLOCK_DATA | !BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
anchored: BDRV_BLOCK_DATA | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID
deallocated: !BDRV_BLOCK_DATA

I have CCed Eric Blake, who is familiar with block status.

> >> +}
> >> +
> >>  static int32_t scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >>  {
> >>  SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req);
> >> @@ -2076,6 +2159,13 @@ static int32_t 
> >> scsi_disk_emulate_command(SCSIRequest *req, uint8_t *buf)
> >>
> >>  /* Protection, exponent and lowest lba field left blank. */
> >>  break;
> >> +} else if ((req->cmd.buf[1] & 31) == SAI_GET_LBA_STATUS) {
> >> +if (req->cmd.lba > s->qdev.max_lba) {
> >> +goto illegal_lba;
> >> +}
> >> +scsi_disk_emulate_get_lba_status(req, outbuf);
> >> +r->iov.iov_len = req->cmd.xfer;
> >> +return r->iov.iov_len;
> > 
> > Is there something tricky going on here with iov_len that prevents us
> > from using break here and sharing the functions normal return code path?
> 
> Using 'break' here and following the normal return code path cause the 
> assertion
> 'assert(!r->req.aiocb)'.
> 
> In fact, There is a 'return' statement in the 'case SYNCHRONIZE_CACHE' in 
> function
> scsi_disk_emulate_command, It causes the same assertion if no this 'return' 
> statement.
> I borrowed this idea to avoid the assertion.

The assertion shows that this patch isn't asynchronous. I think the
assertion will pass once you convert GET_LBA_STATUS to async and then
a break statement will work.


signature.asc
Description: PGP signature

Re: [REPORT] [GSoC - TCG Continuous Benchmarking] [#2] Dissecting QEMU Into Three Main Parts

[Aleksandar Markovic]

понедељак, 29. јун 2020., Ahmed Karaman  је
написао/ла:

> Hi,
>
> The second report of the TCG Continuous Benchmarking series builds
> upon the QEMU performance metrics calculated in the previous report.
> This report presents a method to dissect the number of instructions
> executed by a QEMU invocation into three main phases:
> - Code Generation
> - JIT Execution
> - Helpers Execution
> It devises a Python script that automates this process.
>
> After that, the report presents an experiment for comparing the
> output of running the script on 17 different targets. Many conclusions
> can be drawn from the results and two of them are discussed in the
> analysis section.
>
> Report link:
> https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/
> Dissecting-QEMU-Into-Three-Main-Parts/
>
> Previous reports:
> Report 1 - Measuring Basic Performance Metrics of QEMU:
> https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06692.html
>
>
My sincere congratulations on the Report 2!!

And, on top of that, this is an excellent idea to list previous reports, as
you did in the paragraph above.

Keep reports coming!!

Aleksandar



> Best regards,
> Ahmed Karaman
>

Re: [PATCH 03/46] qdev: Smooth error checking of qdev_realize() & friends

[Greg Kurz]

On Wed, 24 Jun 2020 18:43:01 +0200
Markus Armbruster  wrote:

> Convert
> 
> foo(..., &err);
> if (err) {
> ...
> }
> 
> to
> 
> if (!foo(..., &err)) {
> ...
> }
> 
> for qdev_realize(), qdev_realize_and_unref(), qbus_realize() and their
> wrappers isa_realize_and_unref(), pci_realize_and_unref(),
> sysbus_realize(), sysbus_realize_and_unref(), usb_realize_and_unref().
> Coccinelle script:
> 
> @@
> identifier fun = {isa_realize_and_unref, pci_realize_and_unref, 
> qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, 
> sysbus_realize_and_unref, usb_realize_and_unref};
> expression list args, args2;
> typedef Error;
> Error *err;
> identifier errp;
> @@
> -  fun(args, &err, args2);
> -  if (err) {
> +  if (!fun(args, errp, args2)) {
>  ... when != err
> -error_propagate(errp, err);
>  ...
>  }
> 
> @@
> identifier fun = {isa_realize_and_unref, pci_realize_and_unref, 
> qbus_realize, qdev_realize, qdev_realize_and_unref, sysbus_realize, 
> sysbus_realize_and_unref, usb_realize_and_unref};
> expression list args, args2;
> typedef Error;
> Error *err;
> @@
> -  fun(args, &err, args2);
> -  if (err) {
> +  if (!fun(args, &err, args2)) {
>  ...
>  }
> 
> Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
> ARMSSE being used both as typedef and function-like macro there.
> Convert manually.
> 
> Eliminate error_propagate() that are now unnecessary.  Delete @err
> that are now unused.  Clean up whitespace.
> 
> Signed-off-by: Markus Armbruster 
> ---

Reviewed-by: Greg Kurz 

>  hw/arm/allwinner-a10.c  |  21 ++-
>  hw/arm/armsse.c | 104 
>  hw/arm/armv7m.c |  12 +---
>  hw/arm/aspeed_ast2600.c |  68 ++---
>  hw/arm/aspeed_soc.c |  60 +-
>  hw/arm/bcm2835_peripherals.c|  60 +-
>  hw/arm/bcm2836.c|  12 +---
>  hw/arm/cubieboard.c |   3 +-
>  hw/arm/digic.c  |  12 +---
>  hw/arm/digic_boards.c   |   3 +-
>  hw/arm/fsl-imx25.c  |  44 --
>  hw/arm/fsl-imx31.c  |  32 +++---
>  hw/arm/fsl-imx6.c   |  48 ---
>  hw/arm/msf2-soc.c   |  21 ++-
>  hw/arm/nrf51_soc.c  |  24 ++--
>  hw/arm/stm32f205_soc.c  |  29 +++--
>  hw/arm/stm32f405_soc.c  |  32 +++---
>  hw/arm/xlnx-zynqmp.c|  61 +--
>  hw/block/fdc.c  |   4 +-
>  hw/block/xen-block.c|   3 +-
>  hw/char/serial-pci-multi.c  |   5 +-
>  hw/char/serial-pci.c|   5 +-
>  hw/char/serial.c|  10 +--
>  hw/core/cpu.c   |   3 +-
>  hw/cpu/a15mpcore.c  |   5 +-
>  hw/cpu/a9mpcore.c   |  21 ++-
>  hw/cpu/arm11mpcore.c|  17 ++
>  hw/cpu/realview_mpcore.c|   9 +--
>  hw/display/virtio-gpu-pci.c |   6 +-
>  hw/display/virtio-vga.c |   5 +-
>  hw/intc/armv7m_nvic.c   |   9 +--
>  hw/intc/pnv_xive.c  |   8 +--
>  hw/intc/realview_gic.c  |   5 +-
>  hw/intc/spapr_xive.c|   8 +--
>  hw/intc/xics.c  |   5 +-
>  hw/intc/xive.c  |   3 +-
>  hw/isa/piix4.c  |   5 +-
>  hw/microblaze/xlnx-zynqmp-pmu.c |   9 +--
>  hw/mips/cps.c   |  17 ++
>  hw/misc/macio/cuda.c|   5 +-
>  hw/misc/macio/macio.c   |  25 ++--
>  hw/misc/macio/pmu.c |   5 +-
>  hw/pci-host/pnv_phb3.c  |  13 +---
>  hw/pci-host/pnv_phb4.c  |   5 +-
>  hw/pci-host/pnv_phb4_pec.c  |   5 +-
>  hw/ppc/e500.c   |   5 +-
>  hw/ppc/pnv.c|  53 
>  hw/ppc/pnv_core.c   |   4 +-
>  hw/ppc/pnv_psi.c|   9 +--
>  hw/ppc/spapr_cpu_core.c |   3 +-
>  hw/ppc/spapr_irq.c  |   5 +-
>  hw/riscv/opentitan.c|   9 +--
>  hw/riscv/sifive_e.c |   6 +-
>  hw/riscv/sifive_u.c |   5 +-
>  hw/s390x/event-facility.c   |  13 ++--
>  hw/s390x/s390-pci-bus.c |   3 +-
>  hw/s390x/sclp.c |   3 +-
>  hw/s390x/virtio-ccw-crypto.c|   5 +-
>  hw/s390x/virtio-ccw-rng.c   |   5 +-
>  hw/scsi/scsi-bus.c  |   4 +-
>  hw/sd/aspeed_sdhci.c|   4 +-
>  hw/sd/ssi-sd.c  |   3 +-
>  hw/usb/bus.c|   3 +-
>  hw/virtio/virtio-rng-pci.c  |   5 +-
>  qdev-monitor.c  |   3 +-
>  65 files changed, 248 insertions(+), 768 deletions(-)
> 
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index 52e0d83760..3e45aa4141 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw

Re: [Bug 1884719] [NEW] Function not implemented when using libaio

[Stefan Hajnoczi]

On Tue, Jun 23, 2020 at 07:39:58AM -, Martin Grigorov wrote:
> Public bug reported:

CCing Filip and Laurent, who may be interested in adding Linux AIO
syscalls to qemu-user.

> 
> Hello
> 
> I experience "Function not implemented" errors when trying to use Linux
> libaio library in foreign architecture, e.g. aarch64.
> 
> I've faced this problem while using 
> https://github.com/multiarch/qemu-user-static, i.e. Docker+QEMU. 
> I understand that I do not use plain QEMU and you may count this report as a 
> "distribution of QEMU"! Just let me know what are the steps to test it with 
> plain QEMU and I will test and update this ticket!
> 
> 
> Here are the steps to reproduce the issue:
> 
> 1) On x86_64 machine register QEMU:
> 
> `docker run -it --rm --privileged multiarch/qemu-user-static --reset
> --credential yes --persistent yes`
> 
> 2) Start a Docker image with foreign CPU architecture, e.g. aarch64
> 
> `docker run -it arm64v8/centos:8 bash`
> 
> 3) Inside the Docker container install GCC and libaio
> 
> `yum install gcc libaio libaio-devel`
> 
> 4) Compile the following C program
> 
> ```
> #include 
> #include 
> #include 
> #include 
> 
> struct io_control {
> io_context_t ioContext;
> };
> 
> int main() {
> int queueSize = 10;
> 
> struct io_control * theControl = (struct io_control *) 
> malloc(sizeof(struct io_control));
> if (theControl == NULL) {
> printf("theControl is NULL");
> return 123;
> }
> 
> int res = io_queue_init(queueSize, &theControl->ioContext);
> io_queue_release(theControl->ioContext);
> free(theControl);
> printf("res is: %d", res);
> }
> ```
> 
> ```
> cat > test.c
> [PASTE THE CODE ABOVE HERE]
> ^D
> ```
> 
> `gcc test.c -o out -laio && ./out`
> 
> 
> When executed directly on aarch64 machine (i.e. without emulation) or on 
> x86_64 Docker image (e.g. centos:8) it prints `res is: 0`, i.e. it 
> successfully initialized a LibAIO queue.
> 
> But when executed on Docker image with foreign/emulated CPU architecture
> it prints `res is: -38` (ENOSYS). `man io_queue_init` says that error
> ENOSYS is returned when "Not implemented."
> 
> Environment:
> 
> QEMU version: 5.0.0.2  
> (https://github.com/multiarch/qemu-user-static/blob/master/.travis.yml#L24-L28)
> Container application: Docker
> Output of `docker --version`:
> 
> ```
> Client:
>  Version:   19.03.8
>  API version:   1.40
>  Go version:go1.13.8
>  Git commit:afacb8b7f0
>  Built: Wed Mar 11 23:42:35 2020
>  OS/Arch:   linux/amd64
>  Experimental:  false
> 
> Server:
>  Engine:
>   Version:  19.03.8
>   API version:  1.40 (minimum version 1.12)
>   Go version:   go1.13.8
>   Git commit:   afacb8b7f0
>   Built:Wed Mar 11 22:48:33 2020
>   OS/Arch:  linux/amd64
>   Experimental: false
>  containerd:
>   Version:  1.3.3-0ubuntu2
>   GitCommit:
>  runc:
>   Version:  spec: 1.0.1-dev
>   GitCommit:
>  docker-init:
>   Version:  0.18.0
>   GitCommit:
> ```
> 
> Same happens with Ubuntu (arm64v8/ubuntu:focal).
> 
> I've tried to `strace` it but :
> 
> ```
> /usr/bin/strace: ptrace(PTRACE_TRACEME, ...): Function not implemented
> /usr/bin/strace: PTRACE_SETOPTIONS: Function not implemented
> /usr/bin/strace: detach: waitpid(112): No child processes
> /usr/bin/strace: Process 112 detached
> ```
> 
> Here are the steps to reproduce the problem with strace:
> 
>  ```
>  docker run --rm -it --security-opt seccomp:unconfined --security-opt 
> apparmor:unconfined --privileged --cap-add ALL arm64v8/centos:8 bash
> 
>  yum install -y strace`
> 
>  strace echo Test
>  ```
> 
> Note: I used --privileged, disabled seccomp and apparmor, and added all
> capabilities
> 
> Disabling security solves the "Permission denied" problem but then comes
> the "Not implemented" one.
> 
> 
> Any idea what could be the problem and how to work it around ?
> I've googled a lot but I wasn't able to find any problems related to libaio 
> on QEMU.
> 
> Thank you!
> Martin
> 
> ** Affects: qemu
>  Importance: Undecided
>  Status: New
> 
> -- 
> You received this bug notification because you are a member of qemu-
> devel-ml, which is subscribed to QEMU.
> https://bugs.launchpad.net/bugs/1884719
> 
> Title:
>   Function not implemented when using libaio
> 
> Status in QEMU:
>   New
> 
> Bug description:
>   Hello
> 
>   I experience "Function not implemented" errors when trying to use
>   Linux libaio library in foreign architecture, e.g. aarch64.
> 
>   I've faced this problem while using 
> https://github.com/multiarch/qemu-user-static, i.e. Docker+QEMU. 
>   I understand that I do not use plain QEMU and you may count this report as 
> a "distribution of QEMU"! Just let me know what are the steps to test it with 
> plain QEMU and I will test and update this ticket!
> 
>   
>   Here are the ste