Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov]

On Wed, 18 Oct 2023 09:32:47 +0100
David Woodhouse  wrote:

> On Wed, 2023-10-18 at 09:32 +0200, Igor Mammedov wrote:
> > On Mon, 16 Oct 2023 16:19:08 +0100
> > David Woodhouse  wrote:
> >   
> > > From: David Woodhouse 
> > >   
> > 
> > is this index a user (guest) visible?  
> 
> Yes. It defines what block device (e.g. /dev/xvda) the disk appears as
> in the guest. In the common case, it literally encodes the Linux
> major/minor numbers. So xvda (major 202) is 0xca00, xvdb is 0xca10 etc.

that makes 'index' an implicit ABI and a subject to versioning
when the way it's assigned changes (i.e. one has to use versioned
machine types to keep older versions working the they used to).

>From what I remember it's discouraged to make QEMU invent
various IDs that are part of ABI (guest or mgmt side).
Instead it's preferred for mgmt side/user to provide that explicitly.

Basically you are trading off manageability/simplicity at QEMU
level with CLI usability for human user.
I don't care much as long as it is hidden within xen code base,
but maybe libvirt does.

> Previously we had to explicitly set it for each disk in Qemu:
> 
>   -drive file=disk1.img,id=drv1 -device xen-disk,drive=drv1,vdev=xvda
>   -drive file=disk2.img,id=drv2 -device xen-disk,drive=drv2,vdev=xvdb
> 
> Now we can just do
> 
>   -drive file=disk1.img,if=xen -drive file-disk2.img,if=xen
>
> (We could go further and make if=xen the default for Xen guests too,
> but that doesn't work right now because xen-block will barf on the
> default provided CD-ROM when it's empty. It doesn't handle empty
> devices. And if I work around that, then `-hda disk1.img` would work on
> the command line... but would make it appear as /dev/xvda instead of
> /dev/hda, and I don't know how I feel about that.)
>
> [root@localhost ~]# xenstore-ls  -f device/vbd
> device/vbd = ""
> device/vbd/51712 = ""
> device/vbd/51712/backend = "/local/domain/0/backend/qdisk/1/51712"
> device/vbd/51712/backend-id = "0"
> device/vbd/51712/device-type = "disk"
> device/vbd/51712/event-channel = "8"
> device/vbd/51712/feature-persistent = "1"
> device/vbd/51712/protocol = "x86_64-abi"
> device/vbd/51712/ring-ref = "8"
> device/vbd/51712/state = "4"
> device/vbd/51712/virtual-device = "51712"
> 
> >   
> > > There's no need to force the user to assign a vdev. We can automatically
> > > assign one, starting at xvda and searching until we find the first disk
> > > name that's unused.
> > > 
> > > This means we can now allow '-drive if=xen,file=xxx' to work without an
> > > explicit separate -driver argument, just like if=virtio.
> > > 
> > > Signed-off-by: David Woodhouse 
> > > ---
> > >  blockdev.c   | 15 ---
> > >  hw/block/xen-block.c | 25 +
> > >  2 files changed, 37 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 325b7a3bef..9dec4c9c74 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
> > >   * Ignore default drives, because we create certain default
> > >   * drives unconditionally, then leave them unclaimed.  Not the
> > >   * users fault.
> > > - * Ignore IF_VIRTIO, because it gets desugared into -device,
> > > - * so we can leave failing to -device.
> > > + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> > > + * -device, so we can leave failing to -device.
> > >   * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
> > >   * available for device_add is a feature.
> > >   */
> > >  if (dinfo->is_default || dinfo->type == IF_VIRTIO
> > > -    || dinfo->type == IF_NONE) {
> > > +    || dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
> > >  continue;
> > >  }
> > >  if (!blk_get_attached_dev(blk)) {
> > > @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> > > BlockInterfaceType block_default_type,
> > >  qemu_opt_set(devopts, "driver", "virtio-blk", _abort);
> > >  qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> > >   _abort);
> > > +    } else if (type == IF_XEN) {
> > > +    QemuOpts *devopts;
> > > +    devopts = 

Re: [PATCH 11/12] hw/xen: automatically assign device index to block devices

[Igor Mammedov]

On Mon, 16 Oct 2023 16:19:08 +0100
David Woodhouse  wrote:

> From: David Woodhouse 
> 

is this index a user (guest) visible?

> There's no need to force the user to assign a vdev. We can automatically
> assign one, starting at xvda and searching until we find the first disk
> name that's unused.
> 
> This means we can now allow '-drive if=xen,file=xxx' to work without an
> explicit separate -driver argument, just like if=virtio.
> 
> Signed-off-by: David Woodhouse 
> ---
>  blockdev.c   | 15 ---
>  hw/block/xen-block.c | 25 +
>  2 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 325b7a3bef..9dec4c9c74 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -255,13 +255,13 @@ void drive_check_orphaned(void)
>   * Ignore default drives, because we create certain default
>   * drives unconditionally, then leave them unclaimed.  Not the
>   * users fault.
> - * Ignore IF_VIRTIO, because it gets desugared into -device,
> - * so we can leave failing to -device.
> + * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
> + * -device, so we can leave failing to -device.
>   * Ignore IF_NONE, because leaving unclaimed IF_NONE remains
>   * available for device_add is a feature.
>   */
>  if (dinfo->is_default || dinfo->type == IF_VIRTIO
> -|| dinfo->type == IF_NONE) {
> +|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
>  continue;
>  }
>  if (!blk_get_attached_dev(blk)) {
> @@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type,
>  qemu_opt_set(devopts, "driver", "virtio-blk", _abort);
>  qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
>   _abort);
> +} else if (type == IF_XEN) {
> +QemuOpts *devopts;
> +devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
> +   _abort);
> +qemu_opt_set(devopts, "driver",
> + (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
> + _abort);
> +qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
> + _abort);
>  }
>  
>  filename = qemu_opt_get(legacy_opts, "file");
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 9262338535..20fa783cbe 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -34,6 +34,31 @@ static char *xen_block_get_name(XenDevice *xendev, Error 
> **errp)
>  XenBlockDevice *blockdev = XEN_BLOCK_DEVICE(xendev);
>  XenBlockVdev *vdev = >props.vdev;
>  
> +if (blockdev->props.vdev.type == XEN_BLOCK_VDEV_TYPE_INVALID) {
> +char name[11];
> +int disk = 0;
> +unsigned long idx;
> +
> +/* Find an unoccupied device name */
> +while (disk < (1 << 20)) {
> +if (disk < (1 << 4)) {
> +idx = (202 << 8) | (disk << 4);
> +} else {
> +idx = (1 << 28) | (disk << 8);
> +}
> +snprintf(name, sizeof(name), "%lu", idx);
> +if (!xen_backend_exists("qdisk", name)) {
> +vdev->type = XEN_BLOCK_VDEV_TYPE_XVD;
> +vdev->partition = 0;
> +vdev->disk = disk;
> +vdev->number = idx;
> +return g_strdup(name);
> +}
> +disk++;
> +}
> +error_setg(errp, "cannot find device vdev for block device");
> +return NULL;
> +}
>  return g_strdup_printf("%lu", vdev->number);
>  }
>  

Re: [PATCH v4 2/2] pcie: Specify 0 for ARI next function numbers

[Igor Mammedov]

On Tue,  4 Jul 2023 21:22:14 +0900
Akihiko Odaki  wrote:

> The current implementers of ARI are all SR-IOV devices. The ARI next
> function number field is undefined for VF .
   ^
add a reference to a spec (spec name, rev, chapter) where it's declared
so reviewer or whoever reads it later could easily find relevant
documentation.

>The PF should end the linked
> list formed with the field by specifying 0.
ditto

> 
> For migration, the field will keep having 1 as its value on the old
> virt models.
> 
> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in 
> docs/pcie_sriov.txt")
> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV")
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki 
> ---
>  include/hw/pci/pci.h | 2 ++
>  hw/core/machine.c| 1 +
>  hw/pci/pci.c | 2 ++
>  hw/pci/pcie.c| 2 +-
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index e6d0574a29..9c5b5eb206 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -209,6 +209,8 @@ enum {
>  QEMU_PCIE_CAP_CXL = (1 << QEMU_PCIE_CXL_BITNR),
>  #define QEMU_PCIE_ERR_UNC_MASK_BITNR 11
>  QEMU_PCIE_ERR_UNC_MASK = (1 << QEMU_PCIE_ERR_UNC_MASK_BITNR),
> +#define QEMU_PCIE_ARI_NEXTFN_1_BITNR 12
> +QEMU_PCIE_ARI_NEXTFN_1 = (1 << QEMU_PCIE_ARI_NEXTFN_1_BITNR),
>  };
>  
>  typedef struct PCIINTxRoute {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 46f8f9a2b0..f0d35c6401 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -41,6 +41,7 @@
>  
>  GlobalProperty hw_compat_8_0[] = {
>  { "migration", "multifd-flush-after-each-section", "on"},
> +{ TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
>  };
>  const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
>  
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e2eb4c3b4a..45a9bc0da8 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -82,6 +82,8 @@ static Property pci_props[] = {
>  DEFINE_PROP_UINT32("acpi-index",  PCIDevice, acpi_index, 0),
>  DEFINE_PROP_BIT("x-pcie-err-unc-mask", PCIDevice, cap_present,
>  QEMU_PCIE_ERR_UNC_MASK_BITNR, true),
> +DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
> +QEMU_PCIE_ARI_NEXTFN_1_BITNR, false),
>  DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 9a3f6430e8..cf09e03a10 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
>  /* ARI */
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset)
>  {
> -uint16_t nextfn = 1;
> +uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0;
>  
>  pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER,
>  offset, PCI_ARI_SIZEOF);

Re: [PATCH v3 2/2] pcie: Specify 0 for ARI next function numbers

[Igor Mammedov]

On Sun,  2 Jul 2023 21:02:27 +0900
Akihiko Odaki  wrote:

> The current implementers of ARI are all SR-IOV devices. The ARI next
> function number field is undefined for VF. The PF should end the linked
> list formed with the field by specifying 0.

this should also describe compat behavior changes.


> Fixes: 2503461691 ("pcie: Add some SR/IOV API documentation in 
> docs/pcie_sriov.txt")
> Fixes: 44c2c09488 ("hw/nvme: Add support for SR-IOV")
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki 
> ---
>  include/hw/pci/pci.h | 2 ++
>  hw/core/machine.c| 1 +
>  hw/pci/pci.c | 2 ++
>  hw/pci/pcie.c| 2 +-
>  4 files changed, 6 insertions(+), 1 deletion(-)
> 

>  GlobalProperty hw_compat_8_0[] = {
>  { "migration", "multifd-flush-after-each-section", "on"},
> +{ TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
>  };

[...]

> +DEFINE_PROP_BIT("x-pcie-ari-nextfn-1", PCIDevice, cap_present,
> +QEMU_PCIE_ARI_NEXTFN_1_BITNR, true),

now, I'm confused a bit. So above line says that default
x-pcie-ari-nextfn-1=on

then compat also sets it to 'on', so question is why do
we have compat entry at all?
If default state doesn't change why do we need involve compat
machinery and add "x-pcie-ari-nextfn-1" property?

>  DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 9a3f6430e8..cf09e03a10 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -1030,7 +1030,7 @@ void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
>  /* ARI */
>  void pcie_ari_init(PCIDevice *dev, uint16_t offset)
>  {
> -uint16_t nextfn = 1;
> +uint16_t nextfn = dev->cap_present & QEMU_PCIE_ARI_NEXTFN_1 ? 1 : 0;
>  
>  pcie_add_capability(dev, PCI_EXT_CAP_ID_ARI, PCI_ARI_VER,
>  offset, PCI_ARI_SIZEOF);

Re: [PATCH 01/32] hw/mips/malta: Introduce PIIX4_PCI_DEVFN definition

[Igor Mammedov]

On Sun,  4 Dec 2022 20:05:22 +0100
Bernhard Beschow  wrote:

> From: Philippe Mathieu-Daudé 
> 
> The PIIX4 PCI-ISA bridge function is always located at 10:0.
> Since we want to re-use its address, add the PIIX4_PCI_DEVFN
> definition.
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> Message-Id: <20221027204720.33611-2-phi...@linaro.org>

Reviewed-by: Igor Mammedov 

> ---
>  hw/mips/malta.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index c0a2e0ab04..9bffa1b128 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -71,6 +71,8 @@
>  
>  #define FLASH_SIZE  0x40
>  
> +#define PIIX4_PCI_DEVFN PCI_DEVFN(10, 0)
> +
>  typedef struct {
>  MemoryRegion iomem;
>  MemoryRegion iomem_lo; /* 0 - 0x900 */
> @@ -1401,7 +1403,7 @@ void mips_malta_init(MachineState *machine)
>  empty_slot_init("GT64120", 0, 0x2000);
>  
>  /* Southbridge */
> -piix4 = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
> +piix4 = pci_create_simple_multifunction(pci_bus, PIIX4_PCI_DEVFN, true,
>  TYPE_PIIX4_PCI_DEVICE);
>  isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(piix4), "isa.0"));
>  

Re: [PATCH v7 12/12] hw/acpi: Make the PCI hot-plug aware of SR-IOV

[Igor Mammedov]

On Fri, 18 Mar 2022 20:18:19 +0100
Lukasz Maniak  wrote:

> From: Łukasz Gieryk 
> 
> PCI device capable of SR-IOV support is a new, still-experimental
> feature with only a single working example of the Nvme device.
> 
> This patch in an attempt to fix a double-free problem when a
> SR-IOV-capable Nvme device is hot-unplugged. The problem and the
> reproduction steps can be found in this thread:
> 
> https://patchew.org/QEMU/20220217174504.1051716-1-lukasz.man...@linux.intel.com/20220217174504.1051716-14-lukasz.man...@linux.intel.com/

pls include that in patch description.

> Details of the proposed solution are, for convenience, included below.
> 
> 1) The current SR-IOV implementation assumes it’s the PhysicalFunction
>that creates and deletes VirtualFunctions.
> 2) It’s a design decision (the Nvme device at least) for the VFs to be
>of the same class as PF. Effectively, they share the dc->hotpluggable
>value.
> 3) When a VF is created, it’s added as a child node to PF’s PCI bus
>slot.
> 4) Monitor/device_del triggers the ACPI mechanism. The implementation is
>not aware of SR/IOV and ejects PF’s PCI slot, directly unrealizing all
>hot-pluggable (!acpi_pcihp_pc_no_hotplug) children nodes.
> 5) VFs are unrealized directly, and it doesn’t work well with (1).
>SR/IOV structures are not updated, so when it’s PF’s turn to be
>unrealized, it works on stale pointers to already-deleted VFs.
it's unclear what's bing hotpluged and unplugged, it would be better if
you included QEMU CLI and relevan qmp/monito commands to reproduce it.

> 
> Signed-off-by: Łukasz Gieryk 
> ---
>  hw/acpi/pcihp.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 6351bd3424d..248839e1110 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -192,8 +192,12 @@ static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, 
> PCIDevice *dev)
>   * ACPI doesn't allow hotplug of bridge devices.  Don't allow
>   * hot-unplug of bridge devices unless they were added by hotplug
>   * (and so, not described by acpi).
> + *
> + * Don't allow hot-unplug of SR-IOV Virtual Functions, as they
> + * will be removed implicitly, when Physical Function is unplugged.
>   */
> -return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable;
> +return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable ||
> +   pci_is_vf(dev);
>  }
>  
>  static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned 
> slots)

Re: [PATCH v2 3/3] Use g_new() & friends where that makes obvious sense

[Igor Mammedov]

On Tue, 15 Mar 2022 15:41:56 +0100
Markus Armbruster  wrote:

> g_new(T, n) is neater than g_malloc(sizeof(T) * n).  It's also safer,
> for two reasons.  One, it catches multiplication overflowing size_t.
> Two, it returns T * rather than void *, which lets the compiler catch
> more type errors.
> 
> This commit only touches allocations with size arguments of the form
> sizeof(T).
> 
> Patch created mechanically with:
> 
> $ spatch --in-place --sp-file scripts/coccinelle/use-g_new-etc.cocci \
>--macro-file scripts/cocci-macro-file.h FILES...
> 
> Signed-off-by: Markus Armbruster 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Cédric Le Goater 
> Reviewed-by: Alex Bennée 
> Acked-by: Dr. David Alan Gilbert 


for */i386/*
Reviewed-by: Igor Mammedov 


nit:
possible miss, see below 

[...]
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index cf8e500514..0731f70410 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c

missed:

 pfn_list = g_malloc(sizeof (*pfn_list) * nr_pfn);


> @@ -396,7 +396,7 @@ go_physmap:
>  
>  mr_name = memory_region_name(mr);
>  
> -physmap = g_malloc(sizeof(XenPhysmap));
> +physmap = g_new(XenPhysmap, 1);
>  
>  physmap->start_addr = start_addr;
>  physmap->size = size;
> @@ -1281,7 +1281,7 @@ static void xen_read_physmap(XenIOState *state)
>  return;
>  
>  for (i = 0; i < num; i++) {
> -physmap = g_malloc(sizeof (XenPhysmap));
> +physmap = g_new(XenPhysmap, 1);
>  physmap->phys_offset = strtoull(entries[i], NULL, 16);
>  snprintf(path, sizeof(path),
>  "/local/domain/0/device-model/%d/physmap/%s/start_addr",
> @@ -1410,7 +1410,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
> **ram_memory)
>  xen_pfn_t ioreq_pfn;
>  XenIOState *state;
>  
> -state = g_malloc0(sizeof (XenIOState));
> +state = g_new0(XenIOState, 1);
>  
>  state->xce_handle = xenevtchn_open(NULL, 0);
>  if (state->xce_handle == NULL) {
> @@ -1463,7 +1463,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
> **ram_memory)
>  }
>  
>  /* Note: cpus is empty at this point in init */
> -state->cpu_by_vcpu_id = g_malloc0(max_cpus * sizeof(CPUState *));
> +state->cpu_by_vcpu_id = g_new0(CPUState *, max_cpus);
>  
>  rc = xen_set_ioreq_server_state(xen_domid, state->ioservid, true);
>  if (rc < 0) {
> @@ -1472,7 +1472,7 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion 
> **ram_memory)
>  goto err;
>  }
>  
> -state->ioreq_local_port = g_malloc0(max_cpus * sizeof (evtchn_port_t));
> +state->ioreq_local_port = g_new0(evtchn_port_t, max_cpus);

[...]

Re: [PATCH 09/28] hw/acpi: Replace g_memdup() by g_memdup2_qemu()

[Igor Mammedov]

On Fri,  3 Sep 2021 13:06:43 +0200
Philippe Mathieu-Daudé  wrote:

> Per 
> https://discourse.gnome.org/t/port-your-module-from-g-memdup-to-g-memdup2-now/5538
> 
>   The old API took the size of the memory to duplicate as a guint,
>   whereas most memory functions take memory sizes as a gsize. This
>   made it easy to accidentally pass a gsize to g_memdup(). For large
>   values, that would lead to a silent truncation of the size from 64
>   to 32 bits, and result in a heap area being returned which is
>   significantly smaller than what the caller expects. This can likely
>   be exploited in various modules to cause a heap buffer overflow.
> 
> Replace g_memdup() by the safer g_memdup2_qemu() wrapper.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Igor Mammedov 


> ---
>  hw/acpi/core.c   | 3 ++-
>  hw/i386/acpi-build.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078d..9dd2cf09a0b 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -637,7 +637,8 @@ void acpi_pm1_cnt_init(ACPIREGS *ar, MemoryRegion *parent,
>  suspend[3] = 1 | ((!disable_s3) << 7);
>  suspend[4] = s4_val | ((!disable_s4) << 7);
>  
> -fw_cfg_add_file(fw_cfg, "etc/system-states", g_memdup(suspend, 6), 
> 6);
> +fw_cfg_add_file(fw_cfg, "etc/system-states",
> +g_memdup2_qemu(suspend, 6), 6);
>  }
>  }
>  
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index aa269914b49..54494ca1f65 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2785,7 +2785,7 @@ void acpi_setup(void)
>   */
>  unsigned rsdp_size = acpi_data_len(tables.rsdp);
>  
> -build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
> +build_state->rsdp = g_memdup2_qemu(tables.rsdp->data, rsdp_size);
>  fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE,
>   acpi_build_update, NULL, build_state,
>   build_state->rsdp, rsdp_size, true);

Re: [PATCH 08/28] hw/acpi: Avoid truncating acpi_data_len() to 32-bit

[Igor Mammedov]

On Fri,  3 Sep 2021 13:06:42 +0200
Philippe Mathieu-Daudé  wrote:

> acpi_data_len() returns an unsigned type, which might be bigger
> than 32-bit (although it is unlikely such value is returned).
> Hold the returned value in an 'unsigned' type to avoid unlikely
> size truncation.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Igor Mammedov 

> ---
>  hw/arm/virt-acpi-build.c | 2 +-
>  hw/i386/acpi-build.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 037cc1fd82c..95543d43e2a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -885,7 +885,7 @@ void virt_acpi_build(VirtMachineState *vms, 
> AcpiBuildTables *tables)
>  
>  static void acpi_ram_update(MemoryRegion *mr, GArray *data)
>  {
> -uint32_t size = acpi_data_len(data);
> +unsigned size = acpi_data_len(data);
>  
>  /* Make sure RAM size is correct - in case it got changed
>   * e.g. by migration */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a33ac8b91e1..aa269914b49 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2660,7 +2660,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState 
> *machine)
>  
>  static void acpi_ram_update(MemoryRegion *mr, GArray *data)
>  {
> -uint32_t size = acpi_data_len(data);
> +unsigned size = acpi_data_len(data);
>  
>  /* Make sure RAM size is correct - in case it got changed e.g. by 
> migration */
>  memory_region_ram_resize(mr, size, _abort);
> @@ -2783,7 +2783,7 @@ void acpi_setup(void)
>   * Though RSDP is small, its contents isn't immutable, so
>   * we'll update it along with the rest of tables on guest access.
>   */
> -uint32_t rsdp_size = acpi_data_len(tables.rsdp);
> +unsigned rsdp_size = acpi_data_len(tables.rsdp);
>  
>  build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
>  fw_cfg_add_file_callback(x86ms->fw_cfg, ACPI_BUILD_RSDP_FILE,

Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()

[Igor Mammedov]

On Mon, 14 Dec 2020 12:24:18 -0500
Eduardo Habkost  wrote:

> On Mon, Dec 14, 2020 at 03:55:30PM +0100, Igor Mammedov wrote:
> > On Fri, 11 Dec 2020 17:05:20 -0500
> > Eduardo Habkost  wrote:
> >   
> > > Every single qdev property setter function manually checks
> > > dev->realized.  We can just check dev->realized inside
> > > qdev_property_set() instead.
> > > 
> > > The check is being added as a separate function
> > > (qdev_prop_allow_set()) because it will become a callback later.  
> > 
> > is callback added within this series?
> > and I'd add here what's the purpose of it.  
> 
> It will be added in part 2 of the series.  See v3:
> https://lore.kernel.org/qemu-devel/20201112214350.872250-35-ehabk...@redhat.com/
> 
> I don't know what else I could say about its purpose, in addition
> to what I wrote above, and the comment below[1].
> 
> If you are just curious about the callback and confused because
> it is not anywhere in this series, I can just remove the
> paragraph above from the commit message.  Would that be enough?
lets remove it for now to avoid confusion

Reviewed-by: Igor Mammedov 

> 
> >   
> [...]
> > > +/* returns: true if property is allowed to be set, false otherwise */  
> 
> [1] ^^^
> 
> > > +static bool qdev_prop_allow_set(Object *obj, const char *name,
> > > +Error **errp)
> > > +{
> > > +DeviceState *dev = DEVICE(obj);
> > > +
> > > +if (dev->realized) {
> > > +qdev_prop_set_after_realize(dev, name, errp);
> > > +return false;
> > > +}
> > > +return true;
> > > +}
> > > +  
> 

Re: [PATCH v4 30/32] qdev: Rename qdev_get_prop_ptr() to object_field_prop_ptr()

[Igor Mammedov]

On Fri, 11 Dec 2020 17:05:27 -0500
Eduardo Habkost  wrote:

> The function will be moved to common QOM code, as it is not
> specific to TYPE_DEVICE anymore.
> 
> Reviewed-by: Stefan Berger 
> Signed-off-by: Eduardo Habkost 

Reviewed-by: Igor Mammedov 

> ---
> Changes v1 -> v2:
> * Rename to object_field_prop_ptr() instead of object_static_prop_ptr()
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Thomas Huth 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/hw/qdev-properties.h |  2 +-
>  backends/tpm/tpm_util.c  |  6 ++--
>  hw/block/xen-block.c |  4 +--
>  hw/core/qdev-properties-system.c | 50 +-
>  hw/core/qdev-properties.c| 60 
>  hw/s390x/css.c   |  4 +--
>  hw/s390x/s390-pci-bus.c  |  4 +--
>  hw/vfio/pci-quirks.c |  4 +--
>  8 files changed, 67 insertions(+), 67 deletions(-)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 90222822f1..97bb9494ae 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -193,7 +193,7 @@ void qdev_prop_set_macaddr(DeviceState *dev, const char 
> *name,
> const uint8_t *value);
>  void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>  
> -void *qdev_get_prop_ptr(Object *obj, Property *prop);
> +void *object_field_prop_ptr(Object *obj, Property *prop);
>  
>  void qdev_prop_register_global(GlobalProperty *prop);
>  const GlobalProperty *qdev_find_global_prop(Object *obj,
> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
> index 39b45fa46d..a6e6d3e72f 100644
> --- a/backends/tpm/tpm_util.c
> +++ b/backends/tpm/tpm_util.c
> @@ -35,7 +35,7 @@
>  static void get_tpm(Object *obj, Visitor *v, const char *name, void *opaque,
>  Error **errp)
>  {
> -TPMBackend **be = qdev_get_prop_ptr(obj, opaque);
> +TPMBackend **be = object_field_prop_ptr(obj, opaque);
>  char *p;
>  
>  p = g_strdup(*be ? (*be)->id : "");
> @@ -47,7 +47,7 @@ static void set_tpm(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  Error **errp)
>  {
>  Property *prop = opaque;
> -TPMBackend *s, **be = qdev_get_prop_ptr(obj, prop);
> +TPMBackend *s, **be = object_field_prop_ptr(obj, prop);
>  char *str;
>  
>  if (!visit_type_str(v, name, , errp)) {
> @@ -67,7 +67,7 @@ static void set_tpm(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  static void release_tpm(Object *obj, const char *name, void *opaque)
>  {
>  Property *prop = opaque;
> -TPMBackend **be = qdev_get_prop_ptr(obj, prop);
> +TPMBackend **be = object_field_prop_ptr(obj, prop);
>  
>  if (*be) {
>  tpm_backend_reset(*be);
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index bd1aef63a7..718d886e5c 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -336,7 +336,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, 
> const char *name,
> void *opaque, Error **errp)
>  {
>  Property *prop = opaque;
> -XenBlockVdev *vdev = qdev_get_prop_ptr(obj, prop);
> +XenBlockVdev *vdev = object_field_prop_ptr(obj, prop);
>  char *str;
>  
>  switch (vdev->type) {
> @@ -396,7 +396,7 @@ static void xen_block_set_vdev(Object *obj, Visitor *v, 
> const char *name,
> void *opaque, Error **errp)
>  {
>  Property *prop = opaque;
> -XenBlockVdev *vdev = qdev_get_prop_ptr(obj, prop);
> +XenBlockVdev *vdev = object_field_prop_ptr(obj, prop);
>  char *str, *p;
>  const char *end;
>  
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 590c5f3d97..e6d378a34e 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -62,7 +62,7 @@ static void get_drive(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>Error **errp)
>  {
>  Property *prop = opaque;
> -void **ptr = qdev_get_prop_ptr(obj, prop);
> +void **ptr = object_field_prop_ptr(obj, pr

Re: [PATCH v4 23/32] qdev: Move dev->realized check to qdev_property_set()

[Igor Mammedov]

On Fri, 11 Dec 2020 17:05:20 -0500
Eduardo Habkost  wrote:

> Every single qdev property setter function manually checks
> dev->realized.  We can just check dev->realized inside
> qdev_property_set() instead.
> 
> The check is being added as a separate function
> (qdev_prop_allow_set()) because it will become a callback later.

is callback added within this series?
and I'd add here what's the purpose of it.

> 
> Reviewed-by: Stefan Berger 
> Signed-off-by: Eduardo Habkost 
> ---
> Changes v1 -> v2:
> * Removed unused variable at xen_block_set_vdev()
> * Redone patch after changes in the previous patches in the
>   series
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Thomas Huth 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: Mark Cave-Ayland 
> Cc: Artyom Tarasenko 
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  backends/tpm/tpm_util.c  |   6 --
>  hw/block/xen-block.c |   6 --
>  hw/core/qdev-properties-system.c |  70 --
>  hw/core/qdev-properties.c| 100 ++-
>  hw/s390x/css.c   |   6 --
>  hw/s390x/s390-pci-bus.c  |   6 --
>  hw/vfio/pci-quirks.c |   6 --
>  target/sparc/cpu.c   |   6 --
>  8 files changed, 18 insertions(+), 188 deletions(-)
> 
> diff --git a/backends/tpm/tpm_util.c b/backends/tpm/tpm_util.c
> index a5d997e7dc..39b45fa46d 100644
> --- a/backends/tpm/tpm_util.c
> +++ b/backends/tpm/tpm_util.c
> @@ -46,16 +46,10 @@ static void get_tpm(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  static void set_tpm(Object *obj, Visitor *v, const char *name, void *opaque,
>  Error **errp)
>  {
> -DeviceState *dev = DEVICE(obj);
>  Property *prop = opaque;
>  TPMBackend *s, **be = qdev_get_prop_ptr(obj, prop);
>  char *str;
>  
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
>  if (!visit_type_str(v, name, , errp)) {
>  return;
>  }
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 905e4acd97..bd1aef63a7 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -395,17 +395,11 @@ static int vbd_name_to_disk(const char *name, const 
> char **endp,
>  static void xen_block_set_vdev(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
>  {
> -DeviceState *dev = DEVICE(obj);
>  Property *prop = opaque;
>  XenBlockVdev *vdev = qdev_get_prop_ptr(obj, prop);
>  char *str, *p;
>  const char *end;
>  
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
>  if (!visit_type_str(v, name, , errp)) {
>  return;
>  }
> diff --git a/hw/core/qdev-properties-system.c 
> b/hw/core/qdev-properties-system.c
> index 42529c3b65..f31aea3de1 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -94,11 +94,6 @@ static void set_drive_helper(Object *obj, Visitor *v, 
> const char *name,
>  bool blk_created = false;
>  int ret;
>  
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
>  if (!visit_type_str(v, name, , errp)) {
>  return;
>  }
> @@ -230,17 +225,11 @@ static void get_chr(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
>  Error **errp)
>  {
> -DeviceState *dev = DEVICE(obj);
>  Property *prop = opaque;
>  CharBackend *be = qdev_get_prop_ptr(obj, prop);
>  Chardev *s;
>  char *str;
>  
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
>  if (!visit_type_str(v, name, , errp)) {
>  return;
>  }
> @@ -311,18 +300,12 @@ static void get_mac(Object *obj, Visitor *v, const char 
> *name, void *opaque,
>  static void set_mac(Object *obj, Visitor *v, const char *name, void *opaque,
>  Error **errp)
>  {
> -DeviceState *dev = DEVICE(obj);
>  Property *prop = opaque;
>  MACAddr *mac = qdev_get_prop_ptr(obj, prop);
>  int i, pos;
>  char *str;
>  const char *p;
>  
> -if (dev->realized) {
> -qdev_prop_set_after_realize(dev, name, errp);
> -return;
> -}
> -
>  if (!visit_type_str(v, name, , errp)) {
>  return;
>  }
> @@ -390,7 +373,6 @@ static void get_netdev(Object *obj, Visitor *v, const 
> char *name,
>  static void 

Re: [PATCH 3/5] qom: Remove module_obj_name parameter from OBJECT_DECLARE* macros

[Igor Mammedov]

On Wed, 16 Sep 2020 14:25:17 -0400
Eduardo Habkost  wrote:

> One of the goals of having less boilerplate on QOM declarations
> is to avoid human error.  Requiring an extra argument that is
> never used is an opportunity for mistakes.
> 
> Remove the unused argument from OBJECT_DECLARE_TYPE and
> OBJECT_DECLARE_SIMPLE_TYPE.
> 
> Coccinelle patch used to convert all users of the macros:
> 
>   @@
>   declarer name OBJECT_DECLARE_TYPE;
>   identifier InstanceType, ClassType, lowercase, UPPERCASE;
>   @@
>OBJECT_DECLARE_TYPE(InstanceType, ClassType,
>   -lowercase,
>UPPERCASE);
> 
>   @@
>   declarer name OBJECT_DECLARE_SIMPLE_TYPE;
>   identifier InstanceType, lowercase, UPPERCASE;
>   @@
>OBJECT_DECLARE_SIMPLE_TYPE(InstanceType,
>   -lowercase,
>UPPERCASE);
> 
> Signed-off-by: Eduardo Habkost 

for x86/virtio/hostmem/QOM
Acked-by: Igor Mammedov 

> ---
> Cc: "Marc-André Lureau" 
> Cc: Gerd Hoffmann 
> Cc: "Michael S. Tsirkin" 
> Cc: "Daniel P. Berrangé" 
> Cc: Peter Maydell 
> Cc: Corey Minyard 
> Cc: "Cédric Le Goater" 
> Cc: David Gibson 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: "Philippe Mathieu-Daudé" 
> Cc: Alistair Francis 
> Cc: David Hildenbrand 
> Cc: Laurent Vivier 
> Cc: Amit Shah 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Paolo Bonzini 
> Cc: Eduardo Habkost 
> Cc: Fam Zheng 
> Cc: "Gonglei (Arei)" 
> Cc: Igor Mammedov 
> Cc: Stefan Berger 
> Cc: Richard Henderson 
> Cc: Michael Rolnik 
> Cc: Sarah Harris 
> Cc: "Edgar E. Iglesias" 
> Cc: Michael Walle 
> Cc: Aleksandar Markovic 
> Cc: Aurelien Jarno 
> Cc: Jiaxun Yang 
> Cc: Aleksandar Rikalo 
> Cc: Anthony Green 
> Cc: Chris Wulff 
> Cc: Marek Vasut 
> Cc: Stafford Horne 
> Cc: Palmer Dabbelt 
> Cc: Sagar Karandikar 
> Cc: Bastian Koppelmann 
> Cc: Yoshinori Sato 
> Cc: Mark Cave-Ayland 
> Cc: Artyom Tarasenko 
> Cc: Guan Xuetao 
> Cc: Max Filippov 
> Cc: qemu-de...@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: qemu-...@nongnu.org
> Cc: qemu-s3...@nongnu.org
> Cc: qemu-block@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-ri...@nongnu.org
> ---
>  hw/audio/intel-hda.h| 2 +-
>  hw/display/virtio-vga.h | 2 +-
>  include/authz/base.h| 2 +-
>  include/authz/list.h| 2 +-
>  include/authz/listfile.h| 2 +-
>  include/authz/pamacct.h | 2 +-
>  include/authz/simple.h  | 2 +-
>  include/crypto/secret_common.h  | 2 +-
>  include/crypto/secret_keyring.h | 2 +-
>  include/hw/arm/armsse.h | 2 +-
>  include/hw/hyperv/vmbus.h   | 2 +-
>  include/hw/i2c/i2c.h| 2 +-
>  include/hw/i2c/smbus_slave.h| 2 +-
>  include/hw/ipack/ipack.h| 2 +-
>  include/hw/ipmi/ipmi.h  | 2 +-
>  include/hw/mem/pc-dimm.h| 2 +-
>  include/hw/ppc/pnv.h| 2 +-
>  include/hw/ppc/pnv_core.h   | 2 +-
>  include/hw/ppc/pnv_homer.h  | 2 +-
>  include/hw/ppc/pnv_occ.h| 2 +-
>  include/hw/ppc/pnv_psi.h| 2 +-
>  include/hw/ppc/pnv_xive.h   | 2 +-
>  include/hw/ppc/spapr_cpu_core.h | 2 +-
>  include/hw/ppc/spapr_vio.h  | 2 +-
>  include/hw/ppc/xics.h   | 2 +-
>  include/hw/ppc/xive.h   | 2 +-
>  include/hw/s390x/event-facility.h   | 2 +-
>  include/hw/s390x/s390_flic.h| 2 +-
>  include/hw/s390x/sclp.h | 2 +-
>  include/hw/sd/sd.h  | 2 +-
>  include/hw/ssi/ssi.h| 2 +-
>  include/hw/sysbus.h | 2 +-
>  include/hw/virtio/virtio-gpu.h  | 2 +-
>  include/hw/virtio/virtio-input.h| 2 +-
>  include/hw/virtio/virtio-mem.h  | 2 +-
>  include/hw/virtio/virtio-pmem.h | 2 +-
>  include/hw/virtio/virtio-serial.h   | 2 +-
>  include/hw/xen/xen-bus.h| 2 +-
>  include/io/channel.h| 2 +-
>  include/io/dns-resolver.h   | 2 +-
>  include/io/net-listener.h   | 2 +-
>  include/qom/object.h| 6 ++
>  include/scsi/pr-manager.h   | 2 +-
>  include/sysemu/cryptodev.h  | 2 +-
>  include/sysemu/hostmem.h| 2 +-
>  include/sysemu/rng.h| 2 +-
>  include/sysemu/tpm_backend.h| 2 +-
>  include/sysemu/vhost-user-backend.h | 2 +-
>  target/alpha/cpu-qom.h  | 2 +-
>  target/arm/cpu-qom.h 

Re: [PATCH v6 2/7] hw: add 5.2 machine types and 5.1 compat options

[Igor Mammedov]

On Tue, 18 Aug 2020 15:33:43 +0100
Stefan Hajnoczi  wrote:

> arm, i386, ppc, and s390x have versioned machine types and associated
> compatibility options. Introduce new ones now that QEMU 5.1 has been
> released.
> 
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/boards.h|  3 +++
>  include/hw/i386/pc.h   |  3 +++
>  hw/arm/virt.c  |  9 -
>  hw/core/machine.c  |  4 
>  hw/i386/pc.c   |  4 
>  hw/i386/pc_piix.c  | 14 +-
>  hw/i386/pc_q35.c   | 13 -
>  hw/ppc/spapr.c | 15 +--
>  hw/s390x/s390-virtio-ccw.c | 14 +-
>  9 files changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 426ce5f625..bc5b82ad20 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -319,6 +319,9 @@ struct MachineState {
>  } \
>  type_init(machine_initfn##_register_types)
>  
> +extern GlobalProperty hw_compat_5_1[];
> +extern const size_t hw_compat_5_1_len;
> +
>  extern GlobalProperty hw_compat_5_0[];
>  extern const size_t hw_compat_5_0_len;
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 3d7ed3a55e..fe52e165b2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -193,6 +193,9 @@ void pc_system_firmware_init(PCMachineState *pcms, 
> MemoryRegion *rom_memory);
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
> const CPUArchIdList *apic_ids, GArray *entry);
>  
> +extern GlobalProperty pc_compat_5_1[];
> +extern const size_t pc_compat_5_1_len;
> +
>  extern GlobalProperty pc_compat_5_0[];
>  extern const size_t pc_compat_5_0_len;
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ecfee362a1..acf9bfbece 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2546,10 +2546,17 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> +static void virt_machine_5_2_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(5, 2)
> +
>  static void virt_machine_5_1_options(MachineClass *mc)
>  {
> +virt_machine_5_2_options(mc);
> +compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
> +DEFINE_VIRT_MACHINE(5, 1)
>  
>  static void virt_machine_5_0_options(MachineClass *mc)
>  {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 8d1a90c6cf..a6f7e4e8d7 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -28,6 +28,10 @@
>  #include "hw/mem/nvdimm.h"
>  #include "migration/vmstate.h"
>  
> +GlobalProperty hw_compat_5_1[] = {
> +};
> +const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
> +
>  GlobalProperty hw_compat_5_0[] = {
>  { "pci-host-bridge", "x-config-reg-migration-enabled", "off" },
>  { "virtio-balloon-device", "page-poison", "false" },
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 47c5ca3e34..4afcf17f99 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -97,6 +97,10 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>  
> +GlobalProperty pc_compat_5_1[] = {
> +};
> +const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
> +
>  GlobalProperty pc_compat_5_0[] = {
>  };
>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index b789e83f9a..9ce1f9c5d6 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -426,7 +426,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>  machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
>  }
>  
> -static void pc_i440fx_5_1_machine_options(MachineClass *m)
> +static void pc_i440fx_5_2_machine_options(MachineClass *m)
>  {
>  PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>  pc_i440fx_machine_options(m);
> @@ -435,6 +435,18 @@ static void pc_i440fx_5_1_machine_options(MachineClass 
> *m)
>  pcmc->default_cpu_version = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
> +  pc_i440fx_5_2_machine_options);
> +
> +static void pc_i440fx_5_1_machine_options(MachineClass *m)
> +{
> +pc_i440fx_machine_options(m);
> +m->alias = NULL;
> +m->is_default = false;
> +compat_props_add(m->compat_props, hw_compat_5_1, hw_compat_5_1_len);
> +compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
> +}
> +
>  DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1

Re: [PATCH v6 2/7] hw: add 5.2 machine types and 5.1 compat options

[Igor Mammedov]

On Tue, 18 Aug 2020 17:11:32 +0200
Cornelia Huck  wrote:

> On Tue, 18 Aug 2020 15:33:43 +0100
> Stefan Hajnoczi  wrote:
> 
> > arm, i386, ppc, and s390x have versioned machine types and associated
> > compatibility options. Introduce new ones now that QEMU 5.1 has been
> > released.
> > 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> >  include/hw/boards.h|  3 +++
> >  include/hw/i386/pc.h   |  3 +++
> >  hw/arm/virt.c  |  9 -
> >  hw/core/machine.c  |  4 
> >  hw/i386/pc.c   |  4 
> >  hw/i386/pc_piix.c  | 14 +-
> >  hw/i386/pc_q35.c   | 13 -
> >  hw/ppc/spapr.c | 15 +--
> >  hw/s390x/s390-virtio-ccw.c | 14 +-
> >  9 files changed, 73 insertions(+), 6 deletions(-)  
> 
> https://lore.kernel.org/qemu-devel/20200728094645.272149-1-coh...@redhat.com/
> is already out there :)

That one doesn't apply anymore (or I did something wrong when applying it),

but Stefan's version applies cleanly

> 
> 

Re: [PATCH v7 0/9] acpi: i386 tweaks

[Igor Mammedov]

On Wed, 10 Jun 2020 17:53:46 +0200
Gerd Hoffmann  wrote:

> On Wed, Jun 10, 2020 at 10:54:26AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jun 10, 2020 at 01:40:02PM +0200, Igor Mammedov wrote:  
> > > On Wed, 10 Jun 2020 11:41:22 +0200
> > > Gerd Hoffmann  wrote:
> > >   
> > > > First batch of microvm patches, some generic acpi stuff.
> > > > Split the acpi-build.c monster, specifically split the
> > > > pc and q35 and pci bits into a separate file which we
> > > > can skip building at some point in the future.
> > > >   
> > > It looks like series is missing patch to whitelist changed ACPI tables in
> > > bios-table-test.  
> > 
> > Right. Does it pass make check?  
> 
> No, but after 'git cherry-pick 9b20a3365d73dad4ad144eab9c5827dbbb2e9f21' it 
> does.
> 
> > > Do we already have test case for microvm in bios-table-test,
> > > if not it's probably time to add it.  
> > 
> > Separately :)  
> 
> Especially as this series is just preparing cleanups and doesn't
> actually add acpi support to microvm yet.
> 
> But, yes, adding a testcase sounds useful.
Sorry for confusion, I didn't mean to do it within this series

> 
> take care,
>   Gerd
> 

Re: [PATCH v7 0/9] acpi: i386 tweaks

[Igor Mammedov]

On Wed, 10 Jun 2020 11:41:22 +0200
Gerd Hoffmann  wrote:

> First batch of microvm patches, some generic acpi stuff.
> Split the acpi-build.c monster, specifically split the
> pc and q35 and pci bits into a separate file which we
> can skip building at some point in the future.
> 
It looks like series is missing patch to whitelist changed ACPI tables in
bios-table-test.

Do we already have test case for microvm in bios-table-test,
if not it's probably time to add it.

> v2 changes: leave acpi-build.c largely as-is, move useful
> bits to other places to allow them being reused, specifically:
> 
>  * move isa device generator functions to individual isa devices.
>  * move fw_cfg generator function to fw_cfg.c
> 
> v3 changes: fix rtc, support multiple lpt devices.
> 
> v4 changes:
>  * drop merged patches.
>  * split rtc crs change to separata patch.
>  * added two cleanup patches.
>  * picked up ack & review tags.
> 
> v5 changes:
>  * add comment for rtc crs update.
>  * add even more cleanup patches.
>  * picked up ack & review tags.
> 
> v6 changes:
>  * floppy: move cmos_get_fd_drive_type.
>  * picked up ack & review tags.
> 
> v7 changes:
>  * rebased to mst/pci branch, resolved stubs conflict.
>  * dropped patches already queued up in mst/pci.
>  * added missing sign-off.
>  * picked up ack & review tags.
> 
> take care,
>   Gerd
> 
> Gerd Hoffmann (9):
>   acpi: move aml builder code for floppy device
>   floppy: make isa_fdc_get_drive_max_chs static
>   floppy: move cmos_get_fd_drive_type() from pc
>   acpi: move aml builder code for i8042 (kbd+mouse) device
>   acpi: factor out fw_cfg_add_acpi_dsdt()
>   acpi: simplify build_isa_devices_aml()
>   acpi: drop serial/parallel enable bits from dsdt
>   acpi: drop build_piix4_pm()
>   acpi: q35: drop _SB.PCI0.ISA.LPCD opregion.
> 
>  hw/i386/fw_cfg.h   |   1 +
>  include/hw/block/fdc.h |   3 +-
>  include/hw/i386/pc.h   |   1 -
>  hw/block/fdc.c | 111 +-
>  hw/i386/acpi-build.c   | 211 ++---
>  hw/i386/fw_cfg.c   |  28 ++
>  hw/i386/pc.c   |  25 -
>  hw/input/pckbd.c   |  31 ++
>  stubs/cmos.c   |   7 ++
>  stubs/Makefile.objs|   1 +
>  10 files changed, 184 insertions(+), 235 deletions(-)
>  create mode 100644 stubs/cmos.c
> 

Re: [PATCH v2 2/8] qapi/misc: Restrict LostTickPolicy enum to machine code

[Igor Mammedov]

On Mon, 16 Mar 2020 01:03:42 +0100
Philippe Mathieu-Daudé  wrote:

> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Igor Mammedov 

> ---
>  qapi/machine.json| 32 
>  qapi/misc.json   | 32 
>  include/hw/rtc/mc146818rtc.h |  2 +-
>  hw/core/qdev-properties.c|  1 +
>  hw/i386/kvm/i8254.c  |  2 +-
>  5 files changed, 35 insertions(+), 34 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index de05730704..07ffc07ba2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -415,6 +415,38 @@
>  ##
>  { 'command': 'query-target', 'returns': 'TargetInfo' }
>  
> +##
> +# @LostTickPolicy:
> +#
> +# Policy for handling lost ticks in timer devices.  Ticks end up getting
> +# lost when, for example, the guest is paused.
> +#
> +# @discard: throw away the missed ticks and continue with future injection
> +#   normally.  The guest OS will see the timer jump ahead by a
> +#   potentially quite significant amount all at once, as if the
> +#   intervening chunk of time had simply not existed; needless to
> +#   say, such a sudden jump can easily confuse a guest OS which is
> +#   not specifically prepared to deal with it.  Assuming the guest
> +#   OS can deal correctly with the time jump, the time in the guest
> +#   and in the host should now match.
> +#
> +# @delay: continue to deliver ticks at the normal rate.  The guest OS will
> +# not notice anything is amiss, as from its point of view time will
> +# have continued to flow normally.  The time in the guest should now
> +# be behind the time in the host by exactly the amount of time during
> +# which ticks have been missed.
> +#
> +# @slew: deliver ticks at a higher rate to catch up with the missed ticks.
> +#The guest OS will not notice anything is amiss, as from its point
> +#of view time will have continued to flow normally.  Once the timer
> +#has managed to catch up with all the missing ticks, the time in
> +#the guest and in the host should match.
> +#
> +# Since: 2.0
> +##
> +{ 'enum': 'LostTickPolicy',
> +  'data': ['discard', 'delay', 'slew' ] }
> +
>  ##
>  # @NumaOptionsType:
>  #
> diff --git a/qapi/misc.json b/qapi/misc.json
> index c18fe681fb..2725d835ad 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -7,38 +7,6 @@
>  
>  { 'include': 'common.json' }
>  
> -##
> -# @LostTickPolicy:
> -#
> -# Policy for handling lost ticks in timer devices.  Ticks end up getting
> -# lost when, for example, the guest is paused.
> -#
> -# @discard: throw away the missed ticks and continue with future injection
> -#   normally.  The guest OS will see the timer jump ahead by a
> -#   potentially quite significant amount all at once, as if the
> -#   intervening chunk of time had simply not existed; needless to
> -#   say, such a sudden jump can easily confuse a guest OS which is
> -#   not specifically prepared to deal with it.  Assuming the guest
> -#   OS can deal correctly with the time jump, the time in the guest
> -#   and in the host should now match.
> -#
> -# @delay: continue to deliver ticks at the normal rate.  The guest OS will
> -# not notice anything is amiss, as from its point of view time will
> -# have continued to flow normally.  The time in the guest should now
> -# be behind the time in the host by exactly the amount of time during
> -# which ticks have been missed.
> -#
> -# @slew: deliver ticks at a higher rate to catch up with the missed ticks.
> -#The guest OS will not notice anything is amiss, as from its point
> -#of view time will have continued to flow normally.  Once the timer
> -#has managed to catch up with all the missing ticks, the time in
> -#the guest and in the host should match.
> -#
> -# Since: 2.0
> -##
> -{ 'enum': 'LostTickPolicy',
> -  'data': ['discard', 'delay', 'slew' ] }
> -
>  ##
>  # @add_client:
>  #
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 10c93a096a..58a7748c66 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -9,7 +9,7 @@
>  #ifndef HW_RTC_MC146818RTC_H
>  #define HW_RTC_MC146818RTC_H
>  
> -#include "qapi/qapi-types-misc.h"
> +#include "qapi/qapi-types-machine.h"
>  #include "qemu/queue.h"
>  #include "qemu/timer.h"
>  #include "hw/isa/isa.h"
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-

Re: [PATCH v5 13/15] acpi: drop build_piix4_pm()

[Igor Mammedov]

On Tue, 12 May 2020 13:16:05 +0200
Gerd Hoffmann  wrote:

> On Mon, May 11, 2020 at 09:37:32PM +0200, Igor Mammedov wrote:
> > On Thu,  7 May 2020 15:16:38 +0200
> > Gerd Hoffmann  wrote:
> >   
> > > The _SB.PCI0.PX13.P13C opregion (holds isa device enable bits)
> > > is not used any more, remove it from DSDT.
> > > 
> > > Signed-off-by: Gerd Hoffmann 
> > > ---
> > >  hw/i386/acpi-build.c | 16 
> > >  1 file changed, 16 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 765409a90eb6..c1e63cce5e8e 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1441,21 +1441,6 @@ static void build_q35_isa_bridge(Aml *table)
> > >  aml_append(table, scope);
> > >  }
> > >  
> > > -static void build_piix4_pm(Aml *table)
> > > -{
> > > -Aml *dev;
> > > -Aml *scope;
> > > -
> > > -scope =  aml_scope("_SB.PCI0");
> > > -dev = aml_device("PX13");
> > > -aml_append(dev, aml_name_decl("_ADR", aml_int(0x00010003)));  
> > I agree about removing P13C but I'm not sure if it's safe to remove
> > whole isa bridge  
> 
> The isa bridge is _SB0.PCI0.ISA (piix4 function 0).
> _SB.PCI0.PX13 is the acpi pm device (piix4 function 3).
> 
> So this does _not_ remove the isa bridge.  And given that PX13 has
> nothing declared beside the opregion we are removing and I have not
> found any other references to the PX13 device I assumed it is ok to drop
> that one too.  Didn't notice any odd side effects in testing.
I don't see HID or some other way to tell guest that it should load a driver
for it so probably we can remove it.
If it breaks some legacy OS we can always add it back.


> 
> take care,
>   Gerd
> 

Re: [PATCH v5 14/15] acpi: q35: drop _SB.PCI0.ISA.LPCD opregion.

[Igor Mammedov]

On Thu,  7 May 2020 15:16:39 +0200
Gerd Hoffmann  wrote:

> Seems to be unused.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index c1e63cce5e8e..1afb47b09ee9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1417,7 +1417,6 @@ static void build_q35_isa_bridge(Aml *table)
>  {
>  Aml *dev;
>  Aml *scope;
> -Aml *field;
>  
>  scope =  aml_scope("_SB.PCI0");
>  dev = aml_device("ISA");
> @@ -1427,16 +1426,6 @@ static void build_q35_isa_bridge(Aml *table)
>  aml_append(dev, aml_operation_region("PIRQ", AML_PCI_CONFIG,
>   aml_int(0x60), 0x0C));
>  
> -aml_append(dev, aml_operation_region("LPCD", AML_PCI_CONFIG,
> - aml_int(0x80), 0x02));
> -field = aml_field("LPCD", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> -aml_append(field, aml_named_field("COMA", 3));
> -aml_append(field, aml_reserved_field(1));
> -aml_append(field, aml_named_field("COMB", 3));
> -aml_append(field, aml_reserved_field(1));
> -aml_append(field, aml_named_field("LPTD", 2));
> -aml_append(dev, field);
> -
>  aml_append(scope, dev);
>  aml_append(table, scope);
>  }

Re: [PATCH v5 13/15] acpi: drop build_piix4_pm()

[Igor Mammedov]

On Thu,  7 May 2020 15:16:38 +0200
Gerd Hoffmann  wrote:

> The _SB.PCI0.PX13.P13C opregion (holds isa device enable bits)
> is not used any more, remove it from DSDT.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/i386/acpi-build.c | 16 
>  1 file changed, 16 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 765409a90eb6..c1e63cce5e8e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1441,21 +1441,6 @@ static void build_q35_isa_bridge(Aml *table)
>  aml_append(table, scope);
>  }
>  
> -static void build_piix4_pm(Aml *table)
> -{
> -Aml *dev;
> -Aml *scope;
> -
> -scope =  aml_scope("_SB.PCI0");
> -dev = aml_device("PX13");
> -aml_append(dev, aml_name_decl("_ADR", aml_int(0x00010003)));
I agree about removing P13C but I'm not sure if it's safe to remove
whole isa bridge

> -
> -aml_append(dev, aml_operation_region("P13C", AML_PCI_CONFIG,
> - aml_int(0x00), 0xff));
> -aml_append(scope, dev);
> -aml_append(table, scope);
> -}
> -
>  static void build_piix4_isa_bridge(Aml *table)
>  {
>  Aml *dev;
> @@ -1607,7 +1592,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  aml_append(dsdt, sb_scope);
>  
>  build_hpet_aml(dsdt);
> -build_piix4_pm(dsdt);
>  build_piix4_isa_bridge(dsdt);
>  build_isa_devices_aml(dsdt);
>  build_piix4_pci_hotplug(dsdt);

Re: [PATCH v5 12/15] acpi: drop serial/parallel enable bits from dsdt

[Igor Mammedov]

On Thu,  7 May 2020 15:16:37 +0200
Gerd Hoffmann  wrote:

> The _STA methods for COM+LPT used to reference them,
> but that isn't the case any more.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 


> ---
>  hw/i386/acpi-build.c | 23 ---
>  1 file changed, 23 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1922868f3401..765409a90eb6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1437,15 +1437,6 @@ static void build_q35_isa_bridge(Aml *table)
>  aml_append(field, aml_named_field("LPTD", 2));
>  aml_append(dev, field);
>  
> -aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
> - aml_int(0x82), 0x02));
> -/* enable bits */
> -field = aml_field("LPCE", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> -aml_append(field, aml_named_field("CAEN", 1));
> -aml_append(field, aml_named_field("CBEN", 1));
> -aml_append(field, aml_named_field("LPEN", 1));
> -aml_append(dev, field);
> -
>  aml_append(scope, dev);
>  aml_append(table, scope);
>  }
> @@ -1469,7 +1460,6 @@ static void build_piix4_isa_bridge(Aml *table)
>  {
>  Aml *dev;
>  Aml *scope;
> -Aml *field;
>  
>  scope =  aml_scope("_SB.PCI0");
>  dev = aml_device("ISA");
> @@ -1478,19 +1468,6 @@ static void build_piix4_isa_bridge(Aml *table)
>  /* PIIX PCI to ISA irq remapping */
>  aml_append(dev, aml_operation_region("P40C", AML_PCI_CONFIG,
>   aml_int(0x60), 0x04));
> -/* enable bits */
> -field = aml_field("^PX13.P13C", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> -/* Offset(0x5f),, 7, */
> -aml_append(field, aml_reserved_field(0x2f8));
> -aml_append(field, aml_reserved_field(7));
> -aml_append(field, aml_named_field("LPEN", 1));
> -/* Offset(0x67),, 3, */
> -aml_append(field, aml_reserved_field(0x38));
> -aml_append(field, aml_reserved_field(3));
> -aml_append(field, aml_named_field("CAEN", 1));
> -aml_append(field, aml_reserved_field(3));
> -aml_append(field, aml_named_field("CBEN", 1));
> -aml_append(dev, field);
>  
>  aml_append(scope, dev);
>  aml_append(table, scope);

Re: [PATCH v5 03/15] acpi: rtc: use a single crs range

[Igor Mammedov]

On Thu,  7 May 2020 15:16:28 +0200
Gerd Hoffmann  wrote:

> Use a single io range for _CRS instead of two,
> following what real hardware does.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/rtc/mc146818rtc.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 2104e0aa3b14..ab0cc59973b3 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -1013,12 +1013,14 @@ static void rtc_build_aml(ISADevice *isadev, Aml 
> *scope)
>  Aml *dev;
>  Aml *crs;
>  
> +/*
> + * Reserving 8 io ports here, following what physical hardware
> + * does, even though qemu only responds to the first two ports.
> + */
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> -   0x10, 0x02));
> +   0x01, 0x08));
>  aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
> -aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE + 2,
> -   0x02, 0x06));
>  
>  dev = aml_device("RTC");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));

Re: [PATCH v4 03/13] acpi: rtc: use a single crs range

[Igor Mammedov]

On Wed, 6 May 2020 10:39:02 +0200
Gerd Hoffmann  wrote:

>   Hi,
> 
> > >  crs = aml_resource_template();
> > >  aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> > > -   0x10, 0x02));
> > > +   0x10, 0x08));
> > >  aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
> > > -aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE 
> > > + 2,
> > > -   0x02, 0x06));  
> > can we just drop the later range as unused? (I don't see where it's 
> > actually initialized)  
> 
> I'd rather follow what physical hardware is doing here
> for better compatibility ...

maybe add comment here why it doesn't match IO range that RTC actualy provides,
otherwise it's looks very confusing

> 
> take care,
>   Gerd
> 

Re: [PATCH v4 13/13] floppy: make isa_fdc_get_drive_max_chs static

[Igor Mammedov]

On Tue,  5 May 2020 13:38:43 +0200
Gerd Hoffmann  wrote:

> acpi aml generator needs this, but it is in floppy code now
> so we can make the function static.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/block/fdc.h | 2 --
>  hw/block/fdc.c | 4 ++--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
> index c15ff4c62315..5d71cf972268 100644
> --- a/include/hw/block/fdc.h
> +++ b/include/hw/block/fdc.h
> @@ -16,7 +16,5 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
> DriveInfo **fds, qemu_irq *fdc_tc);
>  
>  FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
> -void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> -   uint8_t *maxc, uint8_t *maxh, uint8_t *maxs);
>  
>  #endif
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 40faa088b5f7..499a580b993c 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2744,8 +2744,8 @@ FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, 
> int i)
>  return isa->state.drives[i].drive;
>  }
>  
> -void isa_fdc_get_drive_max_chs(FloppyDriveType type,
> -   uint8_t *maxc, uint8_t *maxh, uint8_t *maxs)
> +static void isa_fdc_get_drive_max_chs(FloppyDriveType type, uint8_t *maxc,
> +  uint8_t *maxh, uint8_t *maxs)
>  {
>  const FDFormat *fdf;
>  

Re: [PATCH v4 12/13] acpi: drop serial/parallel enable bits from dsdt

[Igor Mammedov]

On Tue,  5 May 2020 13:38:42 +0200
Gerd Hoffmann  wrote:

> The _STA methods for COM+LPT used to reference them,
> but that isn't the case any more.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/i386/acpi-build.c | 23 ---
>  1 file changed, 23 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1922868f3401..765409a90eb6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1437,15 +1437,6 @@ static void build_q35_isa_bridge(Aml *table)
>  aml_append(field, aml_named_field("LPTD", 2));
  
not related to this patch but it seems that above are also unused fields.
it was this way in Seabios and probably even earlier wherever it was copied 
from.

>  aml_append(dev, field);
>  
> -aml_append(dev, aml_operation_region("LPCE", AML_PCI_CONFIG,
> - aml_int(0x82), 0x02));
> -/* enable bits */
> -field = aml_field("LPCE", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
> -aml_append(field, aml_named_field("CAEN", 1));
> -aml_append(field, aml_named_field("CBEN", 1));
> -aml_append(field, aml_named_field("LPEN", 1));
> -aml_append(dev, field);
> -
>  aml_append(scope, dev);
>  aml_append(table, scope);
>  }
> @@ -1469,7 +1460,6 @@ static void build_piix4_isa_bridge(Aml *table)
>  {
>  Aml *dev;
>  Aml *scope;
> -Aml *field;
>  
>  scope =  aml_scope("_SB.PCI0");
>  dev = aml_device("ISA");
> @@ -1478,19 +1468,6 @@ static void build_piix4_isa_bridge(Aml *table)
>  /* PIIX PCI to ISA irq remapping */
>  aml_append(dev, aml_operation_region("P40C", AML_PCI_CONFIG,
>   aml_int(0x60), 0x04));
> -/* enable bits */
> -field = aml_field("^PX13.P13C", AML_ANY_ACC, AML_NOLOCK, AML_PRESERVE);
^^^ should we drop this as well as it becomes 
unused?

> -/* Offset(0x5f),, 7, */
> -aml_append(field, aml_reserved_field(0x2f8));
> -aml_append(field, aml_reserved_field(7));
> -aml_append(field, aml_named_field("LPEN", 1));
> -/* Offset(0x67),, 3, */
> -aml_append(field, aml_reserved_field(0x38));
> -aml_append(field, aml_reserved_field(3));
> -aml_append(field, aml_named_field("CAEN", 1));
> -aml_append(field, aml_reserved_field(3));
> -aml_append(field, aml_named_field("CBEN", 1));
> -aml_append(dev, field);
>  
>  aml_append(scope, dev);
>  aml_append(table, scope);

Re: [PATCH v4 11/13] acpi: simplify build_isa_devices_aml()

[Igor Mammedov]

On Tue,  5 May 2020 13:38:41 +0200
Gerd Hoffmann  wrote:

> x86 machines can have a single ISA bus only.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 15 +--
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index cb3913d2ee76..1922868f3401 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1061,19 +1061,14 @@ static void build_hpet_aml(Aml *table)
>  static void build_isa_devices_aml(Aml *table)
>  {
>  bool ambiguous;
> -
> -Aml *scope = aml_scope("_SB.PCI0.ISA");
>  Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, );
> +Aml *scope;
>  
> -if (ambiguous) {
> -error_report("Multiple ISA busses, unable to define IPMI ACPI data");
> -} else if (!obj) {
> -error_report("No ISA bus, unable to define IPMI ACPI data");
> -} else {
> -build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
> -isa_build_aml(ISA_BUS(obj), scope);
> -}
> +assert(obj && !ambiguous);
>  
> +scope = aml_scope("_SB.PCI0.ISA");
> +build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
> +isa_build_aml(ISA_BUS(obj), scope);
>  aml_append(table, scope);
>  }
>  

Re: [PATCH v4 03/13] acpi: rtc: use a single crs range

[Igor Mammedov]

On Tue,  5 May 2020 13:38:33 +0200
Gerd Hoffmann  wrote:

> Use a single io range for _CRS instead of two,
> following what real hardware does.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/rtc/mc146818rtc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index 2104e0aa3b14..47fafcfb7c1d 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -1015,10 +1015,8 @@ static void rtc_build_aml(ISADevice *isadev, Aml 
> *scope)
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> -   0x10, 0x02));
> +   0x10, 0x08));
>  aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
> -aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE + 2,
> -   0x02, 0x06));
can we just drop the later range as unused? (I don't see where it's actually 
initialized)

>  
>  dev = aml_device("RTC");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));

Re: [PATCH v4 02/13] acpi: move aml builder code for rtc device

[Igor Mammedov]

On Tue,  5 May 2020 13:38:32 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 17 -
>  hw/rtc/mc146818rtc.c | 22 ++
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2e15f6848e7e..0bfa2dd23fcc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1137,22 +1137,6 @@ static Aml *build_fdc_device_aml(ISADevice *fdc)
>  return dev;
>  }
>  
> -static Aml *build_rtc_device_aml(void)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -dev = aml_device("RTC");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
> -aml_append(crs, aml_irq_no_flags(8));
> -aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -return dev;
> -}
> -
>  static Aml *build_kbd_device_aml(void)
>  {
>  Aml *dev;
> @@ -1278,7 +1262,6 @@ static void build_isa_devices_aml(Aml *table)
>  Aml *scope = aml_scope("_SB.PCI0.ISA");
>  Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, );
>  
> -aml_append(scope, build_rtc_device_aml());
>  aml_append(scope, build_kbd_device_aml());
>  aml_append(scope, build_mouse_device_aml());
>  if (fdc) {
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index d18c09911be2..2104e0aa3b14 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -27,6 +27,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/module.h"
>  #include "qemu/bcd.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>  #include "qemu/timer.h"
> @@ -1007,13 +1008,34 @@ static void rtc_resetdev(DeviceState *d)
>  }
>  }
>  
> +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +Aml *dev;
> +Aml *crs;
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> +   0x10, 0x02));
> +aml_append(crs, aml_irq_no_flags(RTC_ISA_IRQ));
> +aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE + 2, RTC_ISA_BASE + 2,
> +   0x02, 0x06));
> +
> +dev = aml_device("RTC");
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +}
> +
>  static void rtc_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = rtc_realizefn;
>  dc->reset = rtc_resetdev;
>  dc->vmsd = _rtc;
> +isa->build_aml = rtc_build_aml;
>  device_class_set_props(dc, mc146818rtc_properties);
>  }
>  

Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method

[Igor Mammedov]

On Mon, 4 May 2020 15:25:16 +0200
Gerd Hoffmann  wrote:

> On Thu, Apr 30, 2020 at 06:25:24PM +0200, Igor Mammedov wrote:
> > On Wed, 29 Apr 2020 15:59:58 +0200
> > Gerd Hoffmann  wrote:
> >   
> > > The _STA method dates back to the days where we had a static DSDT.  The
> > > device is listed in the DSDT table unconditionally and the _STA method
> > > checks a bit in the isa bridge pci config space to figure whenever a
> > > given is isa device is present or not, then evaluates to 0x0f (present)
> > > or 0x00 (absent).
> > > 
> > > These days the DSDT is generated by qemu anyway, so if a device is not
> > > present we can simply drop it from the DSDT instead.
> > > 
> > > Signed-off-by: Gerd Hoffmann 
> > > Reviewed-by: Igor Mammedov   
> > 
> > looking more at it, we should also cleanup no longer used LPEN field
> > the same applies to similar fields for serial and ...   
> 
> IIRC these bits are in the chipset specs so we should not remove them
> from pci config space.
we also can't touch device model due to migration resons
(ACPI code that checks STA bits is in the wild, and us removing ACPI part
of it won't change that)

> 
> Removing from DSDT should be possible indeed (I guess this is what you
> mean?)
yep

> 
> take care,
>   Gerd
> 

Re: [PATCH v3 14/15] acpi: factor out fw_cfg_add_acpi_dsdt()

[Igor Mammedov]

On Wed, 29 Apr 2020 16:00:02 +0200
Gerd Hoffmann  wrote:

> Add helper function to add fw_cfg device,
> also move code to hw/i386/fw_cfg.c.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Philippe Mathieu-Daudé 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/fw_cfg.h |  1 +
>  hw/i386/acpi-build.c | 24 +---
>  hw/i386/fw_cfg.c | 28 
>  3 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/i386/fw_cfg.h b/hw/i386/fw_cfg.h
> index 9e742787792b..275f15c1c5e8 100644
> --- a/hw/i386/fw_cfg.h
> +++ b/hw/i386/fw_cfg.h
> @@ -25,5 +25,6 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
> uint16_t apic_id_limit);
>  void fw_cfg_build_smbios(MachineState *ms, FWCfgState *fw_cfg);
>  void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg);
> +void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg);
>  
>  #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 643e875c05a5..a8b790021974 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1874,30 +1874,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>  
>  /* create fw_cfg node, unconditionally */
>  {
> -/* when using port i/o, the 8-bit data register *always* overlaps
> - * with half of the 16-bit control register. Hence, the total size
> - * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
> - * DMA control register is located at FW_CFG_DMA_IO_BASE + 4 */
> -uint8_t io_size = object_property_get_bool(OBJECT(x86ms->fw_cfg),
> -   "dma_enabled", NULL) ?
> -  ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
> -  FW_CFG_CTL_SIZE;
> -
>  scope = aml_scope("\\_SB.PCI0");
> -dev = aml_device("FWCF");
> -
> -aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> -
> -/* device present, functioning, decoding, not shown in UI */
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> -
> -crs = aml_resource_template();
> -aml_append(crs,
> -aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, 
> io_size)
> -);
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -aml_append(scope, dev);
> +fw_cfg_add_acpi_dsdt(scope, x86ms->fw_cfg);
>  aml_append(dsdt, scope);
>  }
>  
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index da60ada59462..c55abfb01abb 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -15,6 +15,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/numa.h"
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/firmware/smbios.h"
>  #include "hw/i386/fw_cfg.h"
>  #include "hw/timer/hpet.h"
> @@ -179,3 +180,30 @@ void fw_cfg_build_feature_control(MachineState *ms, 
> FWCfgState *fw_cfg)
>  *val = cpu_to_le64(feature_control_bits | FEATURE_CONTROL_LOCKED);
>  fw_cfg_add_file(fw_cfg, "etc/msr_feature_control", val, sizeof(*val));
>  }
> +
> +void fw_cfg_add_acpi_dsdt(Aml *scope, FWCfgState *fw_cfg)
> +{
> +/*
> + * when using port i/o, the 8-bit data register *always* overlaps
> + * with half of the 16-bit control register. Hence, the total size
> + * of the i/o region used is FW_CFG_CTL_SIZE; when using DMA, the
> + * DMA control register is located at FW_CFG_DMA_IO_BASE + 4
> + */
> +Object *obj = OBJECT(fw_cfg);
> +uint8_t io_size = object_property_get_bool(obj, "dma_enabled", NULL) ?
> +ROUND_UP(FW_CFG_CTL_SIZE, 4) + sizeof(dma_addr_t) :
> +FW_CFG_CTL_SIZE;
> +Aml *dev = aml_device("FWCF");
> +Aml *crs = aml_resource_template();
> +
> +aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +
> +/* device present, functioning, decoding, not shown in UI */
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> +aml_append(crs,
> +aml_io(AML_DECODE16, FW_CFG_IO_BASE, FW_CFG_IO_BASE, 0x01, io_size));
> +
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +aml_append(scope, dev);
> +}

Re: [PATCH v3 13/15] acpi: move aml builder code for i8042 (kbd+mouse) device

[Igor Mammedov]

On Wed, 29 Apr 2020 16:00:01 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 39 ---
>  hw/input/pckbd.c | 31 +++
>  2 files changed, 31 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 7415bd5fdce0..643e875c05a5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1052,42 +1052,6 @@ static void build_hpet_aml(Aml *table)
>  aml_append(table, scope);
>  }
>  
> -static Aml *build_kbd_device_aml(void)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -dev = aml_device("KBD");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0303")));
> -
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> -
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
> -aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
> -aml_append(crs, aml_irq_no_flags(1));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -return dev;
> -}
> -
> -static Aml *build_mouse_device_aml(void)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -dev = aml_device("MOU");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0F13")));
> -
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> -
> -crs = aml_resource_template();
> -aml_append(crs, aml_irq_no_flags(12));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -return dev;
> -}
> -
>  static void build_isa_devices_aml(Aml *table)
>  {
>  bool ambiguous;
> @@ -1095,9 +1059,6 @@ static void build_isa_devices_aml(Aml *table)
>  Aml *scope = aml_scope("_SB.PCI0.ISA");
>  Object *obj = object_resolve_path_type("", TYPE_ISA_BUS, );
>  
> -aml_append(scope, build_kbd_device_aml());
> -aml_append(scope, build_mouse_device_aml());
> -
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");
>  } else if (!obj) {
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index 60a41303203a..29d633ca9478 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -26,6 +26,7 @@
>  #include "qemu/log.h"
>  #include "hw/isa/isa.h"
>  #include "migration/vmstate.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/input/ps2.h"
>  #include "hw/irq.h"
>  #include "hw/input/i8042.h"
> @@ -561,12 +562,42 @@ static void i8042_realizefn(DeviceState *dev, Error 
> **errp)
>  qemu_register_reset(kbd_reset, s);
>  }
>  
> +static void i8042_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +Aml *kbd;
> +Aml *mou;
> +Aml *crs;
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
> +aml_append(crs, aml_io(AML_DECODE16, 0x0064, 0x0064, 0x01, 0x01));
> +aml_append(crs, aml_irq_no_flags(1));
> +
> +kbd = aml_device("KBD");
> +aml_append(kbd, aml_name_decl("_HID", aml_eisaid("PNP0303")));
> +aml_append(kbd, aml_name_decl("_STA", aml_int(0xf)));
> +aml_append(kbd, aml_name_decl("_CRS", crs));
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_irq_no_flags(12));
> +
> +mou = aml_device("MOU");
> +aml_append(mou, aml_name_decl("_HID", aml_eisaid("PNP0F13")));
> +aml_append(mou, aml_name_decl("_STA", aml_int(0xf)));
> +aml_append(mou, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, kbd);
> +aml_append(scope, mou);
> +}
> +
>  static void i8042_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = i8042_realizefn;
>  dc->vmsd = _kbd_isa;
> +isa->build_aml = i8042_build_aml;
>  set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
>  

Re: [PATCH v3 10/15] acpi: parallel: don't use _STA method

[Igor Mammedov]

On Wed, 29 Apr 2020 15:59:58 +0200
Gerd Hoffmann  wrote:

> The _STA method dates back to the days where we had a static DSDT.  The
> device is listed in the DSDT table unconditionally and the _STA method
> checks a bit in the isa bridge pci config space to figure whenever a
> given is isa device is present or not, then evaluates to 0x0f (present)
> or 0x00 (absent).
> 
> These days the DSDT is generated by qemu anyway, so if a device is not
> present we can simply drop it from the DSDT instead.
> 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Igor Mammedov 

looking more at it, we should also cleanup no longer used LPEN field
the same applies to similar fields for serial and ... 


> ---
>  hw/i386/acpi-build.c | 29 -
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index fea83352e6ab..e01afbd011d9 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1167,39 +1167,26 @@ static Aml *build_mouse_device_aml(void)
>  return dev;
>  }
>  
> -static Aml *build_lpt_device_aml(void)
> +static void build_lpt_device_aml(Aml *scope)
>  {
>  Aml *dev;
>  Aml *crs;
> -Aml *method;
> -Aml *if_ctx;
> -Aml *else_ctx;
> -Aml *zero = aml_int(0);
> -Aml *is_present = aml_local(0);
> +
> +if (!memory_region_present(get_system_io(), 0x0378)) {
> +return;
> +}
>  
>  dev = aml_device("LPT");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_store(aml_name("LPEN"), is_present));
> -if_ctx = aml_if(aml_equal(is_present, zero));
> -{
> -aml_append(if_ctx, aml_return(aml_int(0x00)));
> -}
> -aml_append(method, if_ctx);
> -else_ctx = aml_else();
> -{
> -aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -}
> -aml_append(method, else_ctx);
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
>  aml_append(crs, aml_irq_no_flags(7));
>  aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -return dev;
> +aml_append(scope, dev);
>  }
>  
>  static void build_isa_devices_aml(Aml *table)
> @@ -1215,7 +1202,7 @@ static void build_isa_devices_aml(Aml *table)
>  if (fdc) {
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
> -aml_append(scope, build_lpt_device_aml());
> +build_lpt_device_aml(scope);
>  
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");

Re: [PATCH v3 12/15] acpi: move aml builder code for floppy device

[Igor Mammedov]

On Wed, 29 Apr 2020 16:00:00 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

see small nit below

> ---
>  hw/block/fdc.c   | 83 
>  hw/i386/acpi-build.c | 83 
>  stubs/cmos.c |  7 
>  stubs/Makefile.objs  |  1 +
>  4 files changed, 91 insertions(+), 83 deletions(-)
>  create mode 100644 stubs/cmos.c
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 33bc9e2f9249..56ddc26c7065 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -32,6 +32,8 @@
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qemu/timer.h"
> +#include "hw/i386/pc.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/irq.h"
>  #include "hw/isa/isa.h"
>  #include "hw/qdev-properties.h"
> @@ -2764,6 +2766,85 @@ void isa_fdc_get_drive_max_chs(FloppyDriveType type,
>  (*maxc)--;
>  }
>  
> +static Aml *build_fdinfo_aml(int idx, FloppyDriveType type)
> +{
> +Aml *dev, *fdi;
> +uint8_t maxc, maxh, maxs;
> +
> +isa_fdc_get_drive_max_chs(type, , , );
   ^^^ can be made static now


> +
> +dev = aml_device("FLP%c", 'A' + idx);
> +
> +aml_append(dev, aml_name_decl("_ADR", aml_int(idx)));
> +
> +fdi = aml_package(16);
> +aml_append(fdi, aml_int(idx));  /* Drive Number */
> +aml_append(fdi,
> +aml_int(cmos_get_fd_drive_type(type)));  /* Device Type */
> +/*
> + * the values below are the limits of the drive, and are thus independent
> + * of the inserted media
> + */
> +aml_append(fdi, aml_int(maxc));  /* Maximum Cylinder Number */
> +aml_append(fdi, aml_int(maxs));  /* Maximum Sector Number */
> +aml_append(fdi, aml_int(maxh));  /* Maximum Head Number */
> +/*
> + * SeaBIOS returns the below values for int 0x13 func 0x08 regardless of
> + * the drive type, so shall we
> + */
> +aml_append(fdi, aml_int(0xAF));  /* disk_specify_1 */
> +aml_append(fdi, aml_int(0x02));  /* disk_specify_2 */
> +aml_append(fdi, aml_int(0x25));  /* disk_motor_wait */
> +aml_append(fdi, aml_int(0x02));  /* disk_sector_siz */
> +aml_append(fdi, aml_int(0x12));  /* disk_eot */
> +aml_append(fdi, aml_int(0x1B));  /* disk_rw_gap */
> +aml_append(fdi, aml_int(0xFF));  /* disk_dtl */
> +aml_append(fdi, aml_int(0x6C));  /* disk_formt_gap */
> +aml_append(fdi, aml_int(0xF6));  /* disk_fill */
> +aml_append(fdi, aml_int(0x0F));  /* disk_head_sttl */
> +aml_append(fdi, aml_int(0x08));  /* disk_motor_strt */
> +
> +aml_append(dev, aml_name_decl("_FDI", fdi));
> +return dev;
> +}
> +
> +static void fdc_isa_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +Aml *dev;
> +Aml *crs;
> +int i;
> +
> +#define ACPI_FDE_MAX_FD 4
> +uint32_t fde_buf[5] = {
> +0, 0, 0, 0, /* presence of floppy drives #0 - #3 */
> +cpu_to_le32(2)  /* tape presence (2 == never present) */
> +};
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, 0x03F2, 0x03F2, 0x00, 0x04));
> +aml_append(crs, aml_io(AML_DECODE16, 0x03F7, 0x03F7, 0x00, 0x01));
> +aml_append(crs, aml_irq_no_flags(6));
> +aml_append(crs,
> +aml_dma(AML_COMPATIBILITY, AML_NOTBUSMASTER, AML_TRANSFER8, 2));
> +
> +dev = aml_device("FDC0");
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0700")));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +for (i = 0; i < MIN(MAX_FD, ACPI_FDE_MAX_FD); i++) {
> +FloppyDriveType type = isa_fdc_get_drive_type(isadev, i);
> +
> +if (type < FLOPPY_DRIVE_TYPE_NONE) {
> +fde_buf[i] = cpu_to_le32(1);  /* drive present */
> +aml_append(dev, build_fdinfo_aml(i, type));
> +}
> +}
> +aml_append(dev, aml_name_decl("_FDE",
> +   aml_buffer(sizeof(fde_buf), (uint8_t *)fde_buf)));
> +
> +aml_append(scope, dev);
> +}
> +
>  static const VMStateDescription vmstate_isa_fdc ={
>  .name = "fdc",
>  .version_id = 2,
> @@ -2797,11 +2878,13 @@ static Property isa_fdc_properties[] = {
>  static void isabus_fdc_class_init(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = isabus_fdc_realize;
>  dc->fw_name = "fdc";
>  dc->reset = fdctrl_external_reset_isa;
>  dc

Re: [PATCH v3 11/15] acpi: move aml builder code for parallel device

[Igor Mammedov]

On Wed, 29 Apr 2020 15:59:59 +0200
Gerd Hoffmann  wrote:

> Also adds support for multiple LPT devices.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/char/parallel.c   | 32 
>  hw/i386/acpi-build.c | 23 ---
>  2 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index 8dd67d13759b..bc6b55b3b910 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -28,6 +28,7 @@
>  #include "qemu/module.h"
>  #include "chardev/char-parallel.h"
>  #include "chardev/char-fe.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/irq.h"
>  #include "hw/isa/isa.h"
>  #include "hw/qdev-properties.h"
> @@ -568,6 +569,35 @@ static void parallel_isa_realizefn(DeviceState *dev, 
> Error **errp)
>   s, "parallel");
>  }
>  
> +static void parallel_isa_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +ISAParallelState *isa = ISA_PARALLEL(isadev);
> +int i, uid = 0;
> +Aml *dev;
> +Aml *crs;
> +
> +for (i = 0; i < ARRAY_SIZE(isa_parallel_io); i++) {
> +if (isa->iobase == isa_parallel_io[i]) {
> +uid = i + 1;
> +}
> +}
> +if (!uid) {
> +return;
> +}
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, isa->iobase, isa->iobase, 0x08, 
> 0x08));
> +aml_append(crs, aml_irq_no_flags(isa->isairq));
> +
> +dev = aml_device("LPT%d", uid);
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
> +aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +}
> +
>  /* Memory mapped interface */
>  static uint64_t parallel_mm_readfn(void *opaque, hwaddr addr, unsigned size)
>  {
> @@ -624,9 +654,11 @@ static Property parallel_isa_properties[] = {
>  static void parallel_isa_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = parallel_isa_realizefn;
>  dc->vmsd = _parallel_isa;
> +isa->build_aml = parallel_isa_build_aml;
>  device_class_set_props(dc, parallel_isa_properties);
>  set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e01afbd011d9..12a017e1f45b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1167,28 +1167,6 @@ static Aml *build_mouse_device_aml(void)
>  return dev;
>  }
>  
> -static void build_lpt_device_aml(Aml *scope)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -if (!memory_region_present(get_system_io(), 0x0378)) {
> -return;
> -}
> -
> -dev = aml_device("LPT");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
> -
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> -
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
> -aml_append(crs, aml_irq_no_flags(7));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -aml_append(scope, dev);
> -}
> -
>  static void build_isa_devices_aml(Aml *table)
>  {
>  ISADevice *fdc = pc_find_fdc0();
> @@ -1202,7 +1180,6 @@ static void build_isa_devices_aml(Aml *table)
>  if (fdc) {
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
> -build_lpt_device_aml(scope);
>  
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");

Re: [PATCH v3 07/15] acpi: move aml builder code for rtc device

[Igor Mammedov]

On Wed, 29 Apr 2020 15:59:55 +0200
Gerd Hoffmann  wrote:

> Also use a single io range instead of two,
> following what real hardware does.
I'd make a separate patch for this comment.
Also qemu maps only the first range (0x70) if I'm not mistaken,
so we perhaps should drop the second (0x72) instead of merging it into the first

[...]

> -static Aml *build_rtc_device_aml(void)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -dev = aml_device("RTC");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
> -aml_append(crs, aml_irq_no_flags(8));
> -aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));
[...]
> +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +Aml *dev;
> +Aml *crs;
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, RTC_ISA_BASE, RTC_ISA_BASE,
> +   0x10, 0x08));

[...]

Re: [PATCH v3 06/15] rtc: add RTC_ISA_BASE

[Igor Mammedov]

On Wed, 29 Apr 2020 15:59:54 +0200
Gerd Hoffmann  wrote:

> Add and use RTC_ISA_BASE define instead of hardcoding 0x70.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/rtc/mc146818rtc.h | 1 +
>  hw/rtc/mc146818rtc.c | 5 ++---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/rtc/mc146818rtc.h b/include/hw/rtc/mc146818rtc.h
> index 10c93a096a1d..3713181b56fe 100644
> --- a/include/hw/rtc/mc146818rtc.h
> +++ b/include/hw/rtc/mc146818rtc.h
> @@ -47,6 +47,7 @@ typedef struct RTCState {
>  } RTCState;
>  
>  #define RTC_ISA_IRQ 8
> +#define RTC_ISA_BASE 0x70
>  
>  ISADevice *mc146818_rtc_init(ISABus *bus, int base_year,
>   qemu_irq intercept_irq);
> diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
> index dc4269cc55cb..d18c09911be2 100644
> --- a/hw/rtc/mc146818rtc.c
> +++ b/hw/rtc/mc146818rtc.c
> @@ -908,7 +908,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  {
>  ISADevice *isadev = ISA_DEVICE(dev);
>  RTCState *s = MC146818_RTC(dev);
> -int base = 0x70;
>  
>  s->cmos_data[RTC_REG_A] = 0x26;
>  s->cmos_data[RTC_REG_B] = 0x02;
> @@ -951,7 +950,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  qemu_register_suspend_notifier(>suspend_notifier);
>  
>  memory_region_init_io(>io, OBJECT(s), _ops, s, "rtc", 2);
> -isa_register_ioport(isadev, >io, base);
> +isa_register_ioport(isadev, >io, RTC_ISA_BASE);
>  
>  /* register rtc 0x70 port for coalesced_pio */
>  memory_region_set_flush_coalesced(>io);
> @@ -960,7 +959,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
>  memory_region_add_subregion(>io, 0, >coalesced_io);
>  memory_region_add_coalescing(>coalesced_io, 0, 1);
>  
> -qdev_set_legacy_instance_id(dev, base, 3);
> +qdev_set_legacy_instance_id(dev, RTC_ISA_BASE, 3);
>  qemu_register_reset(rtc_reset, s);
>  
>  object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);

Re: [PATCH v2 07/12] acpi: move aml builder code for rtc device

[Igor Mammedov]

On Tue, 7 Apr 2020 12:26:58 +0200
Gerd Hoffmann  wrote:

> On Mon, Apr 06, 2020 at 02:17:05PM +0200, Igor Mammedov wrote:
> > On Mon, 6 Apr 2020 10:25:17 +0200
> > Gerd Hoffmann  wrote:
> >   
> > > On Fri, Apr 03, 2020 at 12:09:21PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  3 Apr 2020 10:04:57 +0200
> > > > Gerd Hoffmann  wrote:
> > > > 
> > > > > Signed-off-by: Gerd Hoffmann 
> > > > > ---
> > > > [...]
> > > > > +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> > > > > +{
> > > > > +Aml *dev;
> > > > > +Aml *crs;
> > > > > +
> > > > > +crs = aml_resource_template();
> > > > > +aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 
> > > > > 0x02));
maybe replace magic 0x0070 with macro
  RTC_BASE_ADDR

and do the same in realize

diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c
index dc4269cc55..a1f27f4883 100644
--- a/hw/rtc/mc146818rtc.c
+++ b/hw/rtc/mc146818rtc.c
@@ -908,7 +908,6 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 {
 ISADevice *isadev = ISA_DEVICE(dev);
 RTCState *s = MC146818_RTC(dev);
-int base = 0x70;
 
 s->cmos_data[RTC_REG_A] = 0x26;
 s->cmos_data[RTC_REG_B] = 0x02;
@@ -951,7 +950,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 qemu_register_suspend_notifier(>suspend_notifier);
 
 memory_region_init_io(>io, OBJECT(s), _ops, s, "rtc", 2);
-isa_register_ioport(isadev, >io, base);
+isa_register_ioport(isadev, >io, RTC_BASE_ADDR);
 
 /* register rtc 0x70 port for coalesced_pio */
 memory_region_set_flush_coalesced(>io);
@@ -960,7 +959,7 @@ static void rtc_realizefn(DeviceState *dev, Error **errp)
 memory_region_add_subregion(>io, 0, >coalesced_io);
 memory_region_add_coalescing(>coalesced_io, 0, 1);
 
-qdev_set_legacy_instance_id(dev, base, 3);
+qdev_set_legacy_instance_id(dev, RTC_BASE_ADDR, 3);
 qemu_register_reset(rtc_reset, s);
 
 object_property_add_tm(OBJECT(s), "date", rtc_get_date, NULL);


> > > > > +aml_append(crs, aml_irq_no_flags(8));
> > > > > +aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 
> > > > > 0x06));

one more comment, is this last io record correct?
(looking at realize it maps only 2 bytes at 0x70)

> > > > 
> > > > since this is made a part of device, can we fetch io port values from
> > > > device instead of hard-codding values here?
> > > 
> > > No, the rtc device hasn't a configurable io port address.  
> > what I'm after is consistent code, so if we switch to taking
> > io_base/irq from ISA device, then do it everywhere.  
> 
> The patch series does it consistently where it makes sense.
> That IMHO isn't the case for the rtc.  It has a fixed address.
> You can't have multiple instances if it.  And because of that
> there isn't a variable in the device state struct where I could
> read the iobase from ...

ok

> 
> > So we don't have a zoo of devices doing the same thing in multiple
> > ways.  
> 
> It's two ways: hardcoded for devices which can't move and
> read-from-device for devices which can move.
> 
> cheers,
>   Gerd
> 

Re: hotplug issue of vhost-user-blk

[Igor Mammedov]

On Wed, 8 Apr 2020 10:25:42 +0800
Li Feng  wrote:

> Hi all,
> 
> Hotplug of vhost-user-blk doesn't not work in qemu master branch and
> all previous version.
> 
> The action I insert a vhost-user-blk disk is:
> (qemu) chardev-add socket,id=spdk_vhost_blk2,path=/vhost-blk.0,reconnect=1
> (qemu) device_add
> vhost-user-blk-pci,chardev=spdk_vhost_blk2,id=spdk_vhost_blk2,num-queues=4
> 
> Until here, it's well.
> 
> Then I unplug it from qemu:
> (qemu) device_del spdk_vhost_blk2
> (qemu) chardev-remove spdk_vhost_blk2
> Error: Chardev 'spdk_vhost_blk2' is busy
> 
> The related code is here:
> qmp_chardev_remove
> -> qemu_chr_is_busy
> -> object_unparent(OBJECT(chr));  
> 
>  330 static bool qemu_chr_is_busy(Chardev *s)
>  331 {
>  332 if (CHARDEV_IS_MUX(s)) {
>  333 MuxChardev *d = MUX_CHARDEV(s);
>  334 return d->mux_cnt >= 0;
>  335 } else {
>  336 return s->be != NULL;
>  337 }
>  338 }
> 
> My question is:
> 1. s->be is set to NULL when qemu_chr_fe_deinit is called.
> However, the qmp_chardev_remove is blocked at qemu_chr_is_busy check,
> then the object_unparent will not be called.
> 2. Is there a path that device_del will trigger the s->be that been set to 
> NULL?

device_del is request for guest to eject device and once it's done backend
could be removed. 

what's you command line? (so I could point out entry point where ejection 
happens
for you to troubleshoot it further)

> 
> How should I fix this issue?
> I have tested that comment the qemu_chr_is_busy works well.
> 
> Thanks in advance.
> 
> Feng Li
> 

Re: [PATCH v2 07/12] acpi: move aml builder code for rtc device

[Igor Mammedov]

On Mon, 6 Apr 2020 10:25:17 +0200
Gerd Hoffmann  wrote:

> On Fri, Apr 03, 2020 at 12:09:21PM +0200, Igor Mammedov wrote:
> > On Fri,  3 Apr 2020 10:04:57 +0200
> > Gerd Hoffmann  wrote:
> >   
> > > Signed-off-by: Gerd Hoffmann 
> > > ---  
> > [...]  
> > > +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> > > +{
> > > +Aml *dev;
> > > +Aml *crs;
> > > +
> > > +crs = aml_resource_template();
> > > +aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
> > > +aml_append(crs, aml_irq_no_flags(8));
> > > +aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));  
> > 
> > since this is made a part of device, can we fetch io port values from
> > device instead of hard-codding values here?  
> 
> No, the rtc device hasn't a configurable io port address.
what I'm after is consistent code, so if we switch to taking
io_base/irq from ISA device, then do it everywhere.
So we don't have a zoo of devices doing the same thing in multiple
ways.

> 
> cheers,
>   Gerd
> 

Re: [PATCH v2 09/12] acpi: move aml builder code for parallel device

[Igor Mammedov]

On Mon, 6 Apr 2020 12:26:52 +0200
Gerd Hoffmann  wrote:

> On Fri, Apr 03, 2020 at 12:16:01PM +0200, Igor Mammedov wrote:
> > On Fri, 3 Apr 2020 12:12:10 +0200
> > Igor Mammedov  wrote:
> >   
> > > On Fri,  3 Apr 2020 10:04:59 +0200
> > > Gerd Hoffmann  wrote:
> > >   
> > [...]  
> > > > +static void parallel_isa_build_aml(ISADevice *isadev, Aml *scope)
> > > > +{
> > > > +ISAParallelState *isa = ISA_PARALLEL(isadev);
> > > > +Aml *dev;
> > > > +Aml *crs;
> > > > +
> > > > +if (isa->iobase != 0x0378) {
> > > > +return;
> > > > +}  
> > if device is present why should we skip adding it to DSDT?  
> 
> Well, that is the current state of affairs, only the first parallel
> ports shows up in the dsdt.  And given how rare parallel ports are these
> days I didn't bother changing that ...
> 
> We can handle this simliar to serial lines though, incremental below.
> Do you prefer that?

yep (with Paolo's comment addressed)

> 
> take care,
>   Gerd
> 
> === cut here ===
> From 617797cf42e56e18d5d62cb171af00c28589caba Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann 
> Date: Mon, 6 Apr 2020 12:17:59 +0200
> Subject: [PATCH] [fixup] parallel
> 
> ---
>  hw/char/parallel.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index 2bff1f17fda7..7157d6816b77 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -572,10 +572,16 @@ static void parallel_isa_realizefn(DeviceState *dev, 
> Error **errp)
>  static void parallel_isa_build_aml(ISADevice *isadev, Aml *scope)
>  {
>  ISAParallelState *isa = ISA_PARALLEL(isadev);
> +int i, uid = 0;
>  Aml *dev;
>  Aml *crs;
>  
> -if (isa->iobase != 0x0378) {
> +for (i = 0; i < ARRAY_SIZE(isa_parallel_io); i++) {
> +if (isa->iobase == isa_parallel_io[i]) {
> +uid = i + 1;
> +}
> +}
> +if (!uid) {
>  return;
>  }
>  
> @@ -583,8 +589,9 @@ static void parallel_isa_build_aml(ISADevice *isadev, Aml 
> *scope)
>  aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
>  aml_append(crs, aml_irq_no_flags(7));
>  
> -dev = aml_device("LPT");
> +dev = aml_device("LPT%d", uid);
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
> +aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
>  aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  aml_append(dev, aml_name_decl("_CRS", crs));
>  

Re: [PATCH v2 06/12] acpi: add ISADeviceClass->build_aml()

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:56 +0200
Gerd Hoffmann  wrote:

> Also add isa_aml_build() function which walks all isa devices.
> This allows to move aml builder code to isa devices.
> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  include/hw/isa/isa.h |  2 ++
>  hw/i386/acpi-build.c |  1 +
>  hw/isa/isa-bus.c | 15 +++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index e9ac1f1205a4..1534f8826453 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -70,6 +70,7 @@ typedef struct IsaDmaClass {
>  
>  typedef struct ISADeviceClass {
>  DeviceClass parent_class;
> +void (*build_aml)(ISADevice *dev, Aml *scope);
>  } ISADeviceClass;
>  
>  struct ISABus {
> @@ -108,6 +109,7 @@ ISADevice *isa_try_create(ISABus *bus, const char *name);
>  ISADevice *isa_create_simple(ISABus *bus, const char *name);
>  
>  ISADevice *isa_vga_init(ISABus *bus);
> +void isa_build_aml(ISABus *bus, Aml *scope);
>  
>  /**
>   * isa_register_ioport: Install an I/O port region on the ISA bus.
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 5d2b9b099684..77fc9df74735 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1259,6 +1259,7 @@ static void build_isa_devices_aml(Aml *table)
>  error_report("No ISA bus, unable to define IPMI ACPI data");
>  } else {
>  build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
> +isa_build_aml(ISA_BUS(obj), scope);

is it possible to have more than 1 ISA bus on pc/q35 machine?

I'm asking because there is following clause
  if (ambiguous) {
just above this context while devices you are moving here all defined
unconditionally before that

>  }
>  
>  aml_append(table, scope);
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 798dd9194e8f..1f2189f4d5db 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -207,6 +207,21 @@ ISADevice *isa_vga_init(ISABus *bus)
>  }
>  }
>  
> +void isa_build_aml(ISABus *bus, Aml *scope)
> +{
> +BusChild *kid;
> +ISADevice *dev;
> +ISADeviceClass *dc;
> +
> +QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
> +dev = ISA_DEVICE(kid->child);
> +dc = ISA_DEVICE_GET_CLASS(dev);
> +if (dc->build_aml) {
> +dc->build_aml(dev, scope);
> +}
> +}
> +}
> +
>  static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>  {
>  ISADevice *d = ISA_DEVICE(dev);

Re: [PATCH v2 09/12] acpi: move aml builder code for parallel device

[Igor Mammedov]

On Fri, 3 Apr 2020 12:12:10 +0200
Igor Mammedov  wrote:

> On Fri,  3 Apr 2020 10:04:59 +0200
> Gerd Hoffmann  wrote:
> 
[...]
> > +static void parallel_isa_build_aml(ISADevice *isadev, Aml *scope)
> > +{
> > +ISAParallelState *isa = ISA_PARALLEL(isadev);
> > +Aml *dev;
> > +Aml *crs;
> > +
> > +if (isa->iobase != 0x0378) {
> > +return;
> > +}
if device is present why should we skip adding it to DSDT?

[..]

Re: [PATCH v2 09/12] acpi: move aml builder code for parallel device

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:59 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/char/parallel.c   | 25 +
>  hw/i386/acpi-build.c | 23 ---
>  2 files changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index 8dd67d13759b..2bff1f17fda7 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -28,6 +28,7 @@
>  #include "qemu/module.h"
>  #include "chardev/char-parallel.h"
>  #include "chardev/char-fe.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/irq.h"
>  #include "hw/isa/isa.h"
>  #include "hw/qdev-properties.h"
> @@ -568,6 +569,28 @@ static void parallel_isa_realizefn(DeviceState *dev, 
> Error **errp)
>   s, "parallel");
>  }
>  
> +static void parallel_isa_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +ISAParallelState *isa = ISA_PARALLEL(isadev);
> +Aml *dev;
> +Aml *crs;
> +
> +if (isa->iobase != 0x0378) {
> +return;
> +}
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
> +aml_append(crs, aml_irq_no_flags(7));
> +
> +dev = aml_device("LPT");
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +}
> +
>  /* Memory mapped interface */
>  static uint64_t parallel_mm_readfn(void *opaque, hwaddr addr, unsigned size)
>  {
> @@ -624,9 +647,11 @@ static Property parallel_isa_properties[] = {
>  static void parallel_isa_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = parallel_isa_realizefn;
>  dc->vmsd = _parallel_isa;
> +isa->build_aml = parallel_isa_build_aml;
>  device_class_set_props(dc, parallel_isa_properties);
>  set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 81805bf85f8d..0539620ddff5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1167,28 +1167,6 @@ static Aml *build_mouse_device_aml(void)
>  return dev;
>  }
>  
> -static void build_lpt_device_aml(Aml *scope)
> -{
> -Aml *dev;
> -Aml *crs;
> -
> -if (!memory_region_present(get_system_io(), 0x0378)) {
> -return;
> -}
> -
> -dev = aml_device("LPT");
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
> -
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> -
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
> -aml_append(crs, aml_irq_no_flags(7));
perhaps fetch values from device instead of hard-coding them

> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -aml_append(scope, dev);
> -}
> -
>  static void build_isa_devices_aml(Aml *table)
>  {
>  ISADevice *fdc = pc_find_fdc0();
> @@ -1202,7 +1180,6 @@ static void build_isa_devices_aml(Aml *table)
>  if (fdc) {
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
> -build_lpt_device_aml(scope);
>  
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");

Re: [PATCH v2 08/12] acpi: move aml builder code for serial device

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:58 +0200
Gerd Hoffmann  wrote:

> The code uses the isa_serial_io array to figure what the device uid is.
> Side effect is that acpi antries are not limited to port 1+2 any more,
> we'll also get entries for ports 3+4.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/char/serial-isa.c | 32 
>  hw/i386/acpi-build.c | 32 
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/char/serial-isa.c b/hw/char/serial-isa.c
> index f9b6eed7833d..f7c19a398ced 100644
> --- a/hw/char/serial-isa.c
> +++ b/hw/char/serial-isa.c
> @@ -27,6 +27,7 @@
>  #include "qapi/error.h"
>  #include "qemu/module.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/acpi/aml-build.h"
>  #include "hw/char/serial.h"
>  #include "hw/isa/isa.h"
>  #include "hw/qdev-properties.h"
> @@ -81,6 +82,35 @@ static void serial_isa_realizefn(DeviceState *dev, Error 
> **errp)
>  isa_register_ioport(isadev, >io, isa->iobase);
>  }
>  
> +static void serial_isa_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +ISASerialState *isa = ISA_SERIAL(isadev);
> +int i, uid = 0;
> +Aml *dev;
> +Aml *crs;
> +
> +for (i = 0; i < ARRAY_SIZE(isa_serial_io); i++) {
> +if (isa->iobase == isa_serial_io[i]) {
> +uid = i + 1;
> +}
> +}
> +if (!uid) {
> +return;
> +}
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, isa->iobase, isa->iobase, 0x00, 
> 0x08));
> +aml_append(crs, aml_irq_no_flags(isa->isairq));
> +
> +dev = aml_device("COM%d", uid);
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0501")));
> +aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +}
> +
>  static const VMStateDescription vmstate_isa_serial = {
>  .name = "serial",
>  .version_id = 3,
> @@ -103,9 +133,11 @@ static Property serial_isa_properties[] = {
>  static void serial_isa_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = serial_isa_realizefn;
>  dc->vmsd = _isa_serial;
> +isa->build_aml = serial_isa_build_aml;
>  device_class_set_props(dc, serial_isa_properties);
>  set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a5bc7764e611..81805bf85f8d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1189,36 +1189,6 @@ static void build_lpt_device_aml(Aml *scope)
>  aml_append(scope, dev);
>  }
>  
> -static void build_com_device_aml(Aml *scope, uint8_t uid)
> -{
> -Aml *dev;
> -Aml *crs;
> -uint8_t irq = 4;
> -uint16_t io_port = 0x03F8;
> -
> -assert(uid == 1 || uid == 2);
> -if (uid == 2) {
> -irq = 3;
> -io_port = 0x02F8;
> -}
> -if (!memory_region_present(get_system_io(), io_port)) {
> -return;
> -}
> -
> -dev = aml_device("COM%d", uid);
> -aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0501")));
> -aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
> -
> -aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
> -
> -crs = aml_resource_template();
> -aml_append(crs, aml_io(AML_DECODE16, io_port, io_port, 0x00, 0x08));
> -aml_append(crs, aml_irq_no_flags(irq));
> -aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -aml_append(scope, dev);
> -}
> -
>  static void build_isa_devices_aml(Aml *table)
>  {
>  ISADevice *fdc = pc_find_fdc0();
> @@ -1233,8 +1203,6 @@ static void build_isa_devices_aml(Aml *table)
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
>  build_lpt_device_aml(scope);
> -build_com_device_aml(scope, 1);
> -build_com_device_aml(scope, 2);
>  
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");

Re: [PATCH v2 07/12] acpi: move aml builder code for rtc device

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:57 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 
> ---
[...]
> +static void rtc_build_aml(ISADevice *isadev, Aml *scope)
> +{
> +Aml *dev;
> +Aml *crs;
> +
> +crs = aml_resource_template();
> +aml_append(crs, aml_io(AML_DECODE16, 0x0070, 0x0070, 0x10, 0x02));
> +aml_append(crs, aml_irq_no_flags(8));
> +aml_append(crs, aml_io(AML_DECODE16, 0x0072, 0x0072, 0x02, 0x06));

since this is made a part of device, can we fetch io port values from
device instead of hard-codding values here?

> +
> +dev = aml_device("RTC");
> +aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0B00")));
> +aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +aml_append(scope, dev);
> +}
> +
>  static void rtc_class_initfn(ObjectClass *klass, void *data)
>  {
>  DeviceClass *dc = DEVICE_CLASS(klass);
> +ISADeviceClass *isa = ISA_DEVICE_CLASS(klass);
>  
>  dc->realize = rtc_realizefn;
>  dc->reset = rtc_resetdev;
>  dc->vmsd = _rtc;
> +isa->build_aml = rtc_build_aml;
>  device_class_set_props(dc, mc146818rtc_properties);
>  }
>  

Re: [PATCH v2 06/12] acpi: add ISADeviceClass->build_aml()

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:56 +0200
Gerd Hoffmann  wrote:

> Also add isa_aml_build() function which walks all isa devices.
> This allows to move aml builder code to isa devices.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/isa/isa.h |  2 ++
>  hw/i386/acpi-build.c |  1 +
>  hw/isa/isa-bus.c | 15 +++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index e9ac1f1205a4..1534f8826453 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -70,6 +70,7 @@ typedef struct IsaDmaClass {
>  
>  typedef struct ISADeviceClass {
>  DeviceClass parent_class;
> +void (*build_aml)(ISADevice *dev, Aml *scope);
>  } ISADeviceClass;
>  
>  struct ISABus {
> @@ -108,6 +109,7 @@ ISADevice *isa_try_create(ISABus *bus, const char *name);
>  ISADevice *isa_create_simple(ISABus *bus, const char *name);
>  
>  ISADevice *isa_vga_init(ISABus *bus);
> +void isa_build_aml(ISABus *bus, Aml *scope);
>  
>  /**
>   * isa_register_ioport: Install an I/O port region on the ISA bus.
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 5d2b9b099684..77fc9df74735 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1259,6 +1259,7 @@ static void build_isa_devices_aml(Aml *table)
>  error_report("No ISA bus, unable to define IPMI ACPI data");
>  } else {
>  build_acpi_ipmi_devices(scope, BUS(obj), "\\_SB.PCI0.ISA");
> +isa_build_aml(ISA_BUS(obj), scope);
>  }
>  
>  aml_append(table, scope);
> diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
> index 798dd9194e8f..1f2189f4d5db 100644
> --- a/hw/isa/isa-bus.c
> +++ b/hw/isa/isa-bus.c
> @@ -207,6 +207,21 @@ ISADevice *isa_vga_init(ISABus *bus)
>  }
>  }
>  
> +void isa_build_aml(ISABus *bus, Aml *scope)
> +{
> +BusChild *kid;
> +ISADevice *dev;
> +ISADeviceClass *dc;
> +
> +QTAILQ_FOREACH(kid, >parent_obj.children, sibling) {
> +dev = ISA_DEVICE(kid->child);
> +dc = ISA_DEVICE_GET_CLASS(dev);
> +if (dc->build_aml) {
> +dc->build_aml(dev, scope);
> +}
> +}
> +}
> +
>  static void isabus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>  {
>  ISADevice *d = ISA_DEVICE(dev);

Re: [PATCH v2 05/12] acpi: parallel: don't use _STA method

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:55 +0200
Gerd Hoffmann  wrote:

> The _STA method dates back to the days where we had a static DSDT.  The
> device is listed in the DSDT table unconditionally and the _STA method
> checks a bit in the isa bridge pci config space to figure whenever a
> given is isa device is present or not, then evaluates to 0x0f (present)
> or 0x00 (absent).
> 
> These days the DSDT is generated by qemu anyway, so if a device is not
> present we can simply drop it from the DSDT instead.
same as 4/12 applies

Reviewed-by: Igor Mammedov 

> 
> Signed-off-by: Gerd Hoffmann 
> ---
>  hw/i386/acpi-build.c | 29 -
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 08433a06039f..5d2b9b099684 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1183,39 +1183,26 @@ static Aml *build_mouse_device_aml(void)
>  return dev;
>  }
>  
> -static Aml *build_lpt_device_aml(void)
> +static void build_lpt_device_aml(Aml *scope)
>  {
>  Aml *dev;
>  Aml *crs;
> -Aml *method;
> -Aml *if_ctx;
> -Aml *else_ctx;
> -Aml *zero = aml_int(0);
> -Aml *is_present = aml_local(0);
> +
> +if (!memory_region_present(get_system_io(), 0x0378)) {
> +return;
> +}
>  
>  dev = aml_device("LPT");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0400")));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_store(aml_name("LPEN"), is_present));
> -if_ctx = aml_if(aml_equal(is_present, zero));
> -{
> -aml_append(if_ctx, aml_return(aml_int(0x00)));
> -}
> -aml_append(method, if_ctx);
> -else_ctx = aml_else();
> -{
> -aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -}
> -aml_append(method, else_ctx);
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, 0x0378, 0x0378, 0x08, 0x08));
>  aml_append(crs, aml_irq_no_flags(7));
>  aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -return dev;
> +aml_append(scope, dev);
>  }
>  
>  static void build_com_device_aml(Aml *scope, uint8_t uid)
> @@ -1262,7 +1249,7 @@ static void build_isa_devices_aml(Aml *table)
>  if (fdc) {
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
> -aml_append(scope, build_lpt_device_aml());
> +build_lpt_device_aml(scope);
>  build_com_device_aml(scope, 1);
>  build_com_device_aml(scope, 2);
>  

Re: [PATCH v2 04/12] acpi: serial: don't use _STA method

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:54 +0200
Gerd Hoffmann  wrote:

> The _STA method dates back to the days where we had a static DSDT.  The
> device is listed in the DSDT table unconditionally and the _STA method
> checks a bit in the isa bridge pci config space to figure whenever a
> given is isa device is present or not, then evaluates to 0x0f (present)
> or 0x00 (absent).
> 
> These days the DSDT is generated by qemu anyway, so if a device is not
> present we can simply drop it from the DSDT instead.
> 
> Signed-off-by: Gerd Hoffmann 
I'd drop this patch and squash commit message into 8/12,
but I don't insist since get_system_io() added here is removed by 8/12,
so either way 

Reviewed-by: Igor Mammedov 


also for patches that modify DSDT, see comment at the start of 
tests/qtest/bios-tables-test.c
to make 'make check' happy and don't break tests during bisection 

> ---
>  hw/i386/acpi-build.c | 32 
>  1 file changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 214b98671bf2..08433a06039f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1218,50 +1218,34 @@ static Aml *build_lpt_device_aml(void)
>  return dev;
>  }
>  
> -static Aml *build_com_device_aml(uint8_t uid)
> +static void build_com_device_aml(Aml *scope, uint8_t uid)
>  {
>  Aml *dev;
>  Aml *crs;
> -Aml *method;
> -Aml *if_ctx;
> -Aml *else_ctx;
> -Aml *zero = aml_int(0);
> -Aml *is_present = aml_local(0);
> -const char *enabled_field = "CAEN";
>  uint8_t irq = 4;
>  uint16_t io_port = 0x03F8;
>  
>  assert(uid == 1 || uid == 2);
>  if (uid == 2) {
> -enabled_field = "CBEN";
>  irq = 3;
>  io_port = 0x02F8;
>  }
> +if (!memory_region_present(get_system_io(), io_port)) {
> +return;
> +}
>  
>  dev = aml_device("COM%d", uid);
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0501")));
>  aml_append(dev, aml_name_decl("_UID", aml_int(uid)));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_store(aml_name("%s", enabled_field), is_present));
> -if_ctx = aml_if(aml_equal(is_present, zero));
> -{
> -aml_append(if_ctx, aml_return(aml_int(0x00)));
> -}
> -aml_append(method, if_ctx);
> -else_ctx = aml_else();
> -{
> -aml_append(else_ctx, aml_return(aml_int(0x0f)));
> -}
> -aml_append(method, else_ctx);
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, io_port, io_port, 0x00, 0x08));
>  aml_append(crs, aml_irq_no_flags(irq));
>  aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -return dev;
> +aml_append(scope, dev);
>  }
>  
>  static void build_isa_devices_aml(Aml *table)
> @@ -1279,8 +1263,8 @@ static void build_isa_devices_aml(Aml *table)
>  aml_append(scope, build_fdc_device_aml(fdc));
>  }
>  aml_append(scope, build_lpt_device_aml());
> -aml_append(scope, build_com_device_aml(1));
> -aml_append(scope, build_com_device_aml(2));
> +build_com_device_aml(scope, 1);
> +build_com_device_aml(scope, 2);
>  
>  if (ambiguous) {
>  error_report("Multiple ISA busses, unable to define IPMI ACPI data");

Re: [PATCH v2 03/12] acpi: drop pointless _STA method

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:53 +0200
Gerd Hoffmann  wrote:

> When returning a constant there is no point in having a method
> in the first place, _STA can be a simple integer instead.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/i386/acpi-build.c | 14 +++---
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9a19c14e661b..214b98671bf2 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1151,14 +1151,11 @@ static Aml *build_kbd_device_aml(void)
>  {
>  Aml *dev;
>  Aml *crs;
> -Aml *method;
>  
>  dev = aml_device("KBD");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0303")));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_return(aml_int(0x0f)));
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_io(AML_DECODE16, 0x0060, 0x0060, 0x01, 0x01));
> @@ -1173,14 +1170,11 @@ static Aml *build_mouse_device_aml(void)
>  {
>  Aml *dev;
>  Aml *crs;
> -Aml *method;
>  
>  dev = aml_device("MOU");
>  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0F13")));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_return(aml_int(0x0f)));
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  crs = aml_resource_template();
>  aml_append(crs, aml_irq_no_flags(12));
> @@ -2238,9 +2232,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> TPM_CRB_ADDR_SIZE, 
> AML_READ_WRITE));
>  aml_append(dev, aml_name_decl("_CRS", crs));
>  
> -method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_return(aml_int(0x0f)));
> -aml_append(dev, method);
> +aml_append(dev, aml_name_decl("_STA", aml_int(0xf)));
>  
>  tpm_build_ppi_acpi(tpm, dev);
>  

Re: [PATCH v2 02/12] acpi: add aml builder stubs

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:52 +0200
Gerd Hoffmann  wrote:

> Needed when moving aml builder code to devices.
> 
> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  hw/acpi/aml-build-stub.c | 79 
>  hw/acpi/Makefile.objs|  4 +-
>  2 files changed, 81 insertions(+), 2 deletions(-)
>  create mode 100644 hw/acpi/aml-build-stub.c
> 
> diff --git a/hw/acpi/aml-build-stub.c b/hw/acpi/aml-build-stub.c
> new file mode 100644
> index ..58b2e162277f
> --- /dev/null
> +++ b/hw/acpi/aml-build-stub.c
> @@ -0,0 +1,79 @@
> +/*
> + * ACPI aml builder stubs for platforms that don't support ACPI.
> + *
> + * Copyright (c) 2006 Fabrice Bellard
> + * Copyright (c) 2016 Red Hat, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +
> +void aml_append(Aml *parent_ctx, Aml *child)
> +{
> +}
> +
> +Aml *aml_resource_template(void)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_device(const char *name_format, ...)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_eisaid(const char *str)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_name_decl(const char *name, Aml *val)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_io(AmlIODecode dec, uint16_t min_base, uint16_t max_base,
> +uint8_t aln, uint8_t len)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_irq_no_flags(uint8_t irq)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_int(const uint64_t val)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_package(uint8_t num_elements)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_dma(AmlDmaType typ, AmlDmaBusMaster bm, AmlTransferSize sz,
> + uint8_t channel)
> +{
> +return NULL;
> +}
> +
> +Aml *aml_buffer(int buffer_size, uint8_t *byte_list)
> +{
> +return NULL;
> +}
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 777da07f4d70..cab9bcd457dc 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -20,6 +20,6 @@ common-obj-$(CONFIG_TPM) += tpm.o
>  common-obj-$(CONFIG_IPMI) += ipmi.o
>  common-obj-$(call lnot,$(CONFIG_IPMI)) += ipmi-stub.o
>  else
> -common-obj-y += acpi-stub.o
> +common-obj-y += acpi-stub.o aml-build-stub.o
>  endif
> -common-obj-$(CONFIG_ALL) += acpi-stub.o acpi-x86-stub.o ipmi-stub.o
> +common-obj-$(CONFIG_ALL) += acpi-stub.o aml-build-stub.o acpi-x86-stub.o 
> ipmi-stub.o

Re: [PATCH v2 01/12] move 'typedef Aml' to qemu/types.h

[Igor Mammedov]

On Fri,  3 Apr 2020 10:04:51 +0200
Gerd Hoffmann  wrote:

> Signed-off-by: Gerd Hoffmann 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/acpi/aml-build.h | 1 -
>  include/qemu/typedefs.h | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index de4a4065682c..1bfe5844e984 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -32,7 +32,6 @@ struct Aml {
>  uint8_t op;
>  AmlBlockFlags block_flags;
>  };
> -typedef struct Aml Aml;
>  
>  typedef enum {
>  AML_COMPATIBILITY = 0,
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 375770a80f06..ecf3cde26c3c 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -24,6 +24,7 @@
>  typedef struct AdapterInfo AdapterInfo;
>  typedef struct AddressSpace AddressSpace;
>  typedef struct AioContext AioContext;
> +typedef struct Aml Aml;
>  typedef struct AnnounceTimer AnnounceTimer;
>  typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
>  typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;

Re: [PATCH v2 5/8] qapi/misc: Restrict query-vm-generation-id command to machine code

[Igor Mammedov]

On Tue, 17 Mar 2020 10:44:33 +0100
Philippe Mathieu-Daudé  wrote:

> On 3/16/20 1:45 PM, Igor Mammedov wrote:
> > On Mon, 16 Mar 2020 01:03:45 +0100
> > Philippe Mathieu-Daudé  wrote:
> >   
> >> Signed-off-by: Philippe Mathieu-Daudé   
> > 
> > Acked-by: Igor Mammedov   
> >> ---
> >>   qapi/machine.json | 20 
> >>   qapi/misc.json| 21 -
> >>   hw/acpi/vmgenid.c |  2 +-
> >>   stubs/vmgenid.c   |  2 +-
> >>   4 files changed, 22 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/qapi/machine.json b/qapi/machine.json
> >> index c096efbea3..1a2a4b0d48 100644
> >> --- a/qapi/machine.json
> >> +++ b/qapi/machine.json
> >> @@ -415,6 +415,26 @@
> >>   ##
> >>   { 'command': 'query-target', 'returns': 'TargetInfo' }
> >>   
> >> +##
> >> +# @GuidInfo:
> >> +#
> >> +# GUID information.
> >> +#
> >> +# @guid: the globally unique identifier
> >> +#
> >> +# Since: 2.9
> >> +##
> >> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
> >> +
> >> +##
> >> +# @query-vm-generation-id:
> >> +#
> >> +# Show Virtual Machine Generation ID
> >> +#
> >> +# Since: 2.9
> >> +##
> >> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
> >> +
> >>   ##
> >>   # @LostTickPolicy:
> >>   #
> >> diff --git a/qapi/misc.json b/qapi/misc.json
> >> index f70025f34c..8c02870227 100644
> >> --- a/qapi/misc.json
> >> +++ b/qapi/misc.json
> >> @@ -1383,24 +1383,3 @@
> >>   #
> >>   ##
> >>   { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
> >> -
> >> -##
> >> -# @GuidInfo:
> >> -#
> >> -# GUID information.
> >> -#
> >> -# @guid: the globally unique identifier
> >> -#
> >> -# Since: 2.9
> >> -##
> >> -{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
> >> -
> >> -##
> >> -# @query-vm-generation-id:
> >> -#
> >> -# Show Virtual Machine Generation ID
> >> -#
> >> -# Since: 2.9
> >> -##
> >> -{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }  
> 
> Daniel explained on IRC the GUID structure is "standardized by microsoft 
> as a way to detect when a guest has certain operations applied" to a 
> saved snapshot.
> 
> https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/virtual-machine-generation-identifier
>  
> 
> 
> So this one goes to qapi/block-core.json, right?

I think it belongs to machine

> 
> >> -
> >> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> >> index 2df7623d74..2b26bacaf8 100644
> >> --- a/hw/acpi/vmgenid.c
> >> +++ b/hw/acpi/vmgenid.c
> >> @@ -12,7 +12,7 @@
> >>   
> >>   #include "qemu/osdep.h"
> >>   #include "qapi/error.h"
> >> -#include "qapi/qapi-commands-misc.h"
> >> +#include "qapi/qapi-commands-machine.h"
> >>   #include "qemu/module.h"
> >>   #include "hw/acpi/acpi.h"
> >>   #include "hw/acpi/aml-build.h"
> >> diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
> >> index 568e42b064..bfad656c6c 100644
> >> --- a/stubs/vmgenid.c
> >> +++ b/stubs/vmgenid.c
> >> @@ -1,6 +1,6 @@
> >>   #include "qemu/osdep.h"
> >>   #include "qapi/error.h"
> >> -#include "qapi/qapi-commands-misc.h"
> >> +#include "qapi/qapi-commands-machine.h"
> >>   #include "qapi/qmp/qerror.h"
> >>   
> >>   GuidInfo *qmp_query_vm_generation_id(Error **errp)  
> >   
> 

Re: [PATCH v2 5/8] qapi/misc: Restrict query-vm-generation-id command to machine code

[Igor Mammedov]

On Mon, 16 Mar 2020 01:03:45 +0100
Philippe Mathieu-Daudé  wrote:

> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Igor Mammedov 
> ---
>  qapi/machine.json | 20 
>  qapi/misc.json| 21 -
>  hw/acpi/vmgenid.c |  2 +-
>  stubs/vmgenid.c   |  2 +-
>  4 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c096efbea3..1a2a4b0d48 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -415,6 +415,26 @@
>  ##
>  { 'command': 'query-target', 'returns': 'TargetInfo' }
>  
> +##
> +# @GuidInfo:
> +#
> +# GUID information.
> +#
> +# @guid: the globally unique identifier
> +#
> +# Since: 2.9
> +##
> +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
> +
> +##
> +# @query-vm-generation-id:
> +#
> +# Show Virtual Machine Generation ID
> +#
> +# Since: 2.9
> +##
> +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
> +
>  ##
>  # @LostTickPolicy:
>  #
> diff --git a/qapi/misc.json b/qapi/misc.json
> index f70025f34c..8c02870227 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1383,24 +1383,3 @@
>  #
>  ##
>  { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
> -
> -##
> -# @GuidInfo:
> -#
> -# GUID information.
> -#
> -# @guid: the globally unique identifier
> -#
> -# Since: 2.9
> -##
> -{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
> -
> -##
> -# @query-vm-generation-id:
> -#
> -# Show Virtual Machine Generation ID
> -#
> -# Since: 2.9
> -##
> -{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
> -
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index 2df7623d74..2b26bacaf8 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -12,7 +12,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-machine.h"
>  #include "qemu/module.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
> diff --git a/stubs/vmgenid.c b/stubs/vmgenid.c
> index 568e42b064..bfad656c6c 100644
> --- a/stubs/vmgenid.c
> +++ b/stubs/vmgenid.c
> @@ -1,6 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> -#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-machine.h"
>  #include "qapi/qmp/qerror.h"
>  
>  GuidInfo *qmp_query_vm_generation_id(Error **errp)

Re: [PATCH v2 8/8] qapi/misc: Restrict device memory commands to machine code

[Igor Mammedov]

On Mon, 16 Mar 2020 01:03:48 +0100
Philippe Mathieu-Daudé  wrote:

> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Igor Mammedov 

> ---
>  qapi/machine.json   | 131 +++
>  qapi/misc.json  | 132 
>  include/hw/mem/memory-device.h  |   1 +
>  include/hw/virtio/virtio-pmem.h |   2 +-
>  4 files changed, 133 insertions(+), 133 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 33b259dbd0..17ccebda14 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1476,3 +1476,134 @@
>  #
>  ##
>  { 'command': 'query-pci', 'returns': ['PciInfo'] }
> +
> +##
> +# @MemoryInfo:
> +#
> +# Actual memory information in bytes.
> +#
> +# @base-memory: size of "base" memory specified with command line
> +#   option -m.
> +#
> +# @plugged-memory: size of memory that can be hot-unplugged. This field
> +#  is omitted if target doesn't support memory hotplug
> +#  (i.e. CONFIG_MEM_DEVICE not defined at build time).
> +#
> +# Since: 2.11.0
> +##
> +{ 'struct': 'MemoryInfo',
> +  'data'  : { 'base-memory': 'size', '*plugged-memory': 'size' } }
> +
> +##
> +# @query-memory-size-summary:
> +#
> +# Return the amount of initially allocated and present hotpluggable (if
> +# enabled) memory in bytes.
> +#
> +# Example:
> +#
> +# -> { "execute": "query-memory-size-summary" }
> +# <- { "return": { "base-memory": 4294967296, "plugged-memory": 0 } }
> +#
> +# Since: 2.11.0
> +##
> +{ 'command': 'query-memory-size-summary', 'returns': 'MemoryInfo' }
> +
> +##
> +# @PCDIMMDeviceInfo:
> +#
> +# PCDIMMDevice state information
> +#
> +# @id: device's ID
> +#
> +# @addr: physical address, where device is mapped
> +#
> +# @size: size of memory that the device provides
> +#
> +# @slot: slot number at which device is plugged in
> +#
> +# @node: NUMA node number where device is plugged in
> +#
> +# @memdev: memory backend linked with device
> +#
> +# @hotplugged: true if device was hotplugged
> +#
> +# @hotpluggable: true if device if could be added/removed while machine is 
> running
> +#
> +# Since: 2.1
> +##
> +{ 'struct': 'PCDIMMDeviceInfo',
> +  'data': { '*id': 'str',
> +'addr': 'int',
> +'size': 'int',
> +'slot': 'int',
> +'node': 'int',
> +'memdev': 'str',
> +'hotplugged': 'bool',
> +'hotpluggable': 'bool'
> +  }
> +}
> +
> +##
> +# @VirtioPMEMDeviceInfo:
> +#
> +# VirtioPMEM state information
> +#
> +# @id: device's ID
> +#
> +# @memaddr: physical address in memory, where device is mapped
> +#
> +# @size: size of memory that the device provides
> +#
> +# @memdev: memory backend linked with device
> +#
> +# Since: 4.1
> +##
> +{ 'struct': 'VirtioPMEMDeviceInfo',
> +  'data': { '*id': 'str',
> +'memaddr': 'size',
> +'size': 'size',
> +'memdev': 'str'
> +  }
> +}
> +
> +##
> +# @MemoryDeviceInfo:
> +#
> +# Union containing information about a memory device
> +#
> +# nvdimm is included since 2.12. virtio-pmem is included since 4.1.
> +#
> +# Since: 2.1
> +##
> +{ 'union': 'MemoryDeviceInfo',
> +  'data': { 'dimm': 'PCDIMMDeviceInfo',
> +'nvdimm': 'PCDIMMDeviceInfo',
> +'virtio-pmem': 'VirtioPMEMDeviceInfo'
> +  }
> +}
> +
> +##
> +# @query-memory-devices:
> +#
> +# Lists available memory devices and their state
> +#
> +# Since: 2.1
> +#
> +# Example:
> +#
> +# -> { "execute": "query-memory-devices" }
> +# <- { "return": [ { "data":
> +#   { "addr": 5368709120,
> +# "hotpluggable": true,
> +# "hotplugged": true,
> +# "id": "d1",
> +# "memdev": "/objects/memX",
> +# "node": 0,
> +# "size": 1073741824,
> +# "slot": 0},
> +#"type": "dimm"
> +#  } ] }
> +#
> +##
> +{ 'command': 'query-memory-devices', 'returns': ['MemoryDeviceInfo'] }
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 92b3926c6b..670b902999 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -518,39 +518,6 @@
>  ##
>  { 'comma

Re: [PATCH v2 4/8] qapi/misc: Move query-uuid command with block code

[Igor Mammedov]

On Mon, 16 Mar 2020 01:03:44 +0100
Philippe Mathieu-Daudé  wrote:

here should be why

PS:
 I don't see a reason to move it to block code at all
if this command is moved then it should be machine code

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  qapi/block-core.json | 30 ++
>  qapi/misc.json   | 30 --
>  block/iscsi.c|  2 +-
>  stubs/uuid.c |  2 +-
>  4 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 91586fb1fb..5c3fa6c5d0 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -5415,3 +5415,33 @@
>  { 'command': 'blockdev-snapshot-delete-internal-sync',
>'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
>'returns': 'SnapshotInfo' }
> +
> +##
> +# @UuidInfo:
> +#
> +# Guest UUID information (Universally Unique Identifier).
> +#
> +# @UUID: the UUID of the guest
> +#
> +# Since: 0.14.0
> +#
> +# Notes: If no UUID was specified for the guest, a null UUID is returned.
> +##
> +{ 'struct': 'UuidInfo', 'data': {'UUID': 'str'} }
> +
> +##
> +# @query-uuid:
> +#
> +# Query the guest UUID information.
> +#
> +# Returns: The @UuidInfo for the guest
> +#
> +# Since: 0.14.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-uuid" }
> +# <- { "return": { "UUID": "550e8400-e29b-41d4-a716-44665544" } }
> +#
> +##
> +{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
> diff --git a/qapi/misc.json b/qapi/misc.json
> index ed28e41229..f70025f34c 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -97,36 +97,6 @@
>  ##
>  { 'command': 'query-kvm', 'returns': 'KvmInfo' }
>  
> -##
> -# @UuidInfo:
> -#
> -# Guest UUID information (Universally Unique Identifier).
> -#
> -# @UUID: the UUID of the guest
> -#
> -# Since: 0.14.0
> -#
> -# Notes: If no UUID was specified for the guest, a null UUID is returned.
> -##
> -{ 'struct': 'UuidInfo', 'data': {'UUID': 'str'} }
> -
> -##
> -# @query-uuid:
> -#
> -# Query the guest UUID information.
> -#
> -# Returns: The @UuidInfo for the guest
> -#
> -# Since: 0.14.0
> -#
> -# Example:
> -#
> -# -> { "execute": "query-uuid" }
> -# <- { "return": { "UUID": "550e8400-e29b-41d4-a716-44665544" } }
> -#
> -##
> -{ 'command': 'query-uuid', 'returns': 'UuidInfo', 'allow-preconfig': true }
> -
>  ##
>  # @IOThreadInfo:
>  #
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 682abd8e09..68ed5cf3f8 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -42,7 +42,7 @@
>  #include "qemu/uuid.h"
>  #include "sysemu/replay.h"
>  #include "qapi/error.h"
> -#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-block-core.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qstring.h"
>  #include "crypto/secret.h"
> diff --git a/stubs/uuid.c b/stubs/uuid.c
> index 67f182fa3a..9ef75fdae4 100644
> --- a/stubs/uuid.c
> +++ b/stubs/uuid.c
> @@ -1,5 +1,5 @@
>  #include "qemu/osdep.h"
> -#include "qapi/qapi-commands-misc.h"
> +#include "qapi/qapi-commands-block-core.h"
>  #include "qemu/uuid.h"
>  
>  UuidInfo *qmp_query_uuid(Error **errp)

Re: [PATCH v2 6/8] qapi/misc: Restrict ACPI commands to machine code

[Igor Mammedov]

On Mon, 16 Mar 2020 01:03:46 +0100
Philippe Mathieu-Daudé  wrote:

> Signed-off-by: Philippe Mathieu-Daudé 

Acked-by: Igor Mammedov 

> ---
>  qapi/machine.json| 154 +++
>  qapi/misc.json   | 154 ---
>  include/hw/acpi/acpi_dev_interface.h |   2 +-
>  hw/acpi/core.c   |   2 +-
>  hw/acpi/cpu.c|   2 +-
>  hw/acpi/memory_hotplug.c |   2 +-
>  6 files changed, 158 insertions(+), 158 deletions(-)
> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 1a2a4b0d48..f77ee63730 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1018,3 +1018,157 @@
>  ##
>  { 'event': 'BALLOON_CHANGE',
>'data': { 'actual': 'int' } }
> +
> +##
> +# @AcpiTableOptions:
> +#
> +# Specify an ACPI table on the command line to load.
> +#
> +# At most one of @file and @data can be specified. The list of files 
> specified
> +# by any one of them is loaded and concatenated in order. If both are 
> omitted,
> +# @data is implied.
> +#
> +# Other fields / optargs can be used to override fields of the generic ACPI
> +# table header; refer to the ACPI specification 5.0, section 5.2.6 System
> +# Description Table Header. If a header field is not overridden, then the
> +# corresponding value from the concatenated blob is used (in case of @file), 
> or
> +# it is filled in with a hard-coded value (in case of @data).
> +#
> +# String fields are copied into the matching ACPI member from lowest address
> +# upwards, and silently truncated / NUL-padded to length.
> +#
> +# @sig: table signature / identifier (4 bytes)
> +#
> +# @rev: table revision number (dependent on signature, 1 byte)
> +#
> +# @oem_id: OEM identifier (6 bytes)
> +#
> +# @oem_table_id: OEM table identifier (8 bytes)
> +#
> +# @oem_rev: OEM-supplied revision number (4 bytes)
> +#
> +# @asl_compiler_id: identifier of the utility that created the table
> +#   (4 bytes)
> +#
> +# @asl_compiler_rev: revision number of the utility that created the
> +#table (4 bytes)
> +#
> +# @file: colon (:) separated list of pathnames to load and
> +#concatenate as table data. The resultant binary blob is expected to
> +#have an ACPI table header. At least one file is required. This field
> +#excludes @data.
> +#
> +# @data: colon (:) separated list of pathnames to load and
> +#concatenate as table data. The resultant binary blob must not have 
> an
> +#ACPI table header. At least one file is required. This field 
> excludes
> +#@file.
> +#
> +# Since: 1.5
> +##
> +{ 'struct': 'AcpiTableOptions',
> +  'data': {
> +'*sig':   'str',
> +'*rev':   'uint8',
> +'*oem_id':'str',
> +'*oem_table_id':  'str',
> +'*oem_rev':   'uint32',
> +'*asl_compiler_id':   'str',
> +'*asl_compiler_rev':  'uint32',
> +'*file':  'str',
> +'*data':  'str' }}
> +
> +##
> +# @MEM_UNPLUG_ERROR:
> +#
> +# Emitted when memory hot unplug error occurs.
> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 2.4
> +#
> +# Example:
> +#
> +# <- { "event": "MEM_UNPLUG_ERROR"
> +#  "data": { "device": "dimm1",
> +#"msg": "acpi: device unplug for unsupported device"
> +#  },
> +#  "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'MEM_UNPLUG_ERROR',
> +  'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @ACPISlotType:
> +#
> +# @DIMM: memory slot
> +# @CPU: logical CPU slot (since 2.7)
> +##
> +{ 'enum': 'ACPISlotType', 'data': [ 'DIMM', 'CPU' ] }
> +
> +##
> +# @ACPIOSTInfo:
> +#
> +# OSPM Status Indication for a device
> +# For description of possible values of @source and @status fields
> +# see "_OST (OSPM Status Indication)" chapter of ACPI5.0 spec.
> +#
> +# @device: device ID associated with slot
> +#
> +# @slot: slot ID, unique per slot of a given @slot-type
> +#
> +# @slot-type: type of the slot
> +#
> +# @source: an integer containing the source event
> +#
> +# @status: an integer containing the status code
> +#
> +# Since: 2.1
> +##
> +{ 'struct': 'ACPIOSTInfo',
> +  'data'  : { '*device': 'str',
> +  'slot': 'str',
> +  'slot-type': 'ACPISlotType',
> +  'source': 'int',
> +  'status': '

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

[Igor Mammedov]

On Wed,  4 Mar 2020 16:35:59 +0100
Philippe Mathieu-Daudé  wrote:

> v2:
> - do not modify qed.h (structure with single member)
> - based on hw/scsi/spapr_vscsi fix series
> 
> This is a tree-wide cleanup inspired by a Linux kernel commit
> (from Gustavo A. R. Silva).
> 
> --v-- description start --v--
> 
>   The current codebase makes use of the zero-length array language
>   extension to the C90 standard, but the preferred mechanism to
>   declare variable-length types such as these ones is a flexible
>   array member [1], introduced in C99:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   };
> 
>   By making use of the mechanism above, we will get a compiler
>   warning in case the flexible array does not occur last in the
>   structure, which will help us prevent some kind of undefined
>   behavior bugs from being unadvertenly introduced [2] to the
>   Linux codebase from now on.
> 
> --^-- description end --^--
> 
> Do the similar housekeeping in the QEMU codebase (which uses
> C99 since commit 7be41675f7cb).
> 
> The first patch is done with the help of a coccinelle semantic
> patch. However Coccinelle does not recognize:
> 
>   struct foo {
>   int stuff;
>   struct boo array[];
>   } QEMU_PACKED;
> 
> but does recognize:
> 
>   struct QEMU_PACKED foo {
>   int stuff;
>   struct boo array[];
>   };
> 
> I'm not sure why, neither it is worth refactoring all QEMU
> structures to use the attributes before the structure name,
> so I did the 2nd patch manually.
> 
> Anyway this is annoying, because many structures are not handled
> by coccinelle. Maybe this needs to be reported to upstream
> coccinelle?
> 
> I used spatch 1.0.8 with:
> 
>   -I include --include-headers \
>   --macro-file scripts/cocci-macro-file.h \
>   --keep-comments --indent 4
> 
> Regards,
> 
> Phil.
> 
> Based-on: <20200304153311.22959-1-phi...@redhat.com>
> Supersedes: <20200304005105.27454-1-phi...@redhat.com>

For acpi parts
Acked-by: Igor Mammedov 

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

Re: [Qemu-block] [PATCH v2 08/18] tests/bios-tables: Improve portability by searching bash in the $PATH

[Igor Mammedov]

On Tue, 29 Jan 2019 18:53:53 +0100
Philippe Mathieu-Daudé  wrote:

> Bash is not always installed as /bin/bash. In particular on OpenBSD,
> the package installs it in /usr/local/bin.
> Use the 'env' shebang to search bash in the $PATH.
> 
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Igor Mammedov 

> ---
>  tests/data/acpi/rebuild-expected-aml.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/data/acpi/rebuild-expected-aml.sh 
> b/tests/data/acpi/rebuild-expected-aml.sh
> index bf9ba242ad..ff77a751c8 100755
> --- a/tests/data/acpi/rebuild-expected-aml.sh
> +++ b/tests/data/acpi/rebuild-expected-aml.sh
> @@ -1,4 +1,4 @@
> -#! /bin/bash
> +#! /usr/bin/env bash
>  
>  #
>  # Rebuild expected AML files for acpi unit-test

Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Wed, 19 Dec 2018 14:00:37 +0100
Andrew Jones  wrote:

> On Wed, Dec 19, 2018 at 01:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> > Hi Drew,
> > 
> > On 12/19/18 11:10 AM, Andrew Jones wrote:
> > > On Tue, Dec 18, 2018 at 06:51:20PM +0100, Philippe Mathieu-Daudé wrote:
> > >> GCC 8 added a -Wstringop-truncation warning:
> > >>
> > >>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> > >>   bug 81117 is specifically intended to highlight likely unintended
> > >>   uses of the strncpy function that truncate the terminating NUL
> > >>   character from the source string.
> > >>
> > >> This new warning leads to compilation failures:
> > >>
> > >> CC  hw/acpi/core.o
> > >>   In function 'acpi_table_install', inlined from 'acpi_table_add' at 
> > >> qemu/hw/acpi/core.c:296:5:
> > >>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals 
> > >> destination size [-Werror=stringop-truncation]
> > >>strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> > >>^
> > >>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> > >>
> > >> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > >> strings to be NUL-terminated.
> > > 
> > > Aren't we always starting with zero-initialized structures in ACPI code?
> > > If so, then we should be able to change the strncpy's to memcpy's.
> > 
> > The first call zero-initializes, but then we call realloc():
> > 
> > /* We won't fail from here on. Initialize / extend the globals. */
> > if (acpi_tables == NULL) {
> > acpi_tables_len = sizeof(uint16_t);
> > acpi_tables = g_malloc0(acpi_tables_len);
> > }
> > 
> > acpi_tables = g_realloc(acpi_tables, acpi_tables_len +
> >  ACPI_TABLE_PFX_SIZE +
> >  sizeof dfl_hdr + body_size);
> > 
> > ext_hdr = (struct acpi_table_header *)(acpi_tables +
> >acpi_tables_len);
> > 
> > So memcpy() isn't enough.
> 
> Ah, thanks.
> 
> > 
> > I can resend the previous patch which uses strpadcpy() if you prefer,
> > Igor already reviewed it:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04406.html
> >
> 
> I do like strpadcpy() better, but I'm not going to lose sleep about
> this either way it goes.
I'm ok with both ways, but v2 consensus was to use QEMU_NONSTRING if I got it 
right

> 
> Thanks,
> drew 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Wed, 19 Dec 2018 10:20:36 +0100
Philippe Mathieu-Daudé  wrote:

> Le mer. 19 déc. 2018 10:16, Igor Mammedov  a écrit :
> 
> > On Tue, 18 Dec 2018 18:51:20 +0100
> > Philippe Mathieu-Daudé  wrote:
> >  
> > > GCC 8 added a -Wstringop-truncation warning:
> > >
> > >   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
> > >   bug 81117 is specifically intended to highlight likely unintended
> > >   uses of the strncpy function that truncate the terminating NUL
> > >   character from the source string.
> > >
> > > This new warning leads to compilation failures:
> > >
> > > CC  hw/acpi/core.o
> > >   In function 'acpi_table_install', inlined from 'acpi_table_add' at  
> > qemu/hw/acpi/core.c:296:5:  
> > >   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals  
> > destination size [-Werror=stringop-truncation]  
> > >strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> > >^
> > >   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> > >
> > > Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> > > strings to be NUL-terminated.
> > >
> > > Suggested-by: Michael S. Tsirkin 
> > > Signed-off-by: Philippe Mathieu-Daudé 
> > > ---
> > >  hw/acpi/core.c  | 8 
> > >  include/hw/acpi/acpi-defs.h | 8 
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> > > index aafdc61648..f60f750c3d 100644
> > > --- a/hw/acpi/core.c
> > > +++ b/hw/acpi/core.c
> > > @@ -35,14 +35,14 @@
> > >  struct acpi_table_header {
> > >  uint16_t _length; /* our length, not actual part of the hdr  
> > */  
> > >/* allows easier parsing for fw_cfg  
> > clients */  
> > > -char sig[4];  /* ACPI signature (4 ASCII characters) */
> > > +char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters)  
> > */  
> > >  uint32_t length;  /* Length of table, in bytes, including  
> > header */  
> > >  uint8_t revision; /* ACPI Specification minor version # */
> > >  uint8_t checksum; /* To make sum of entire table == 0 */
> > > -char oem_id[6];   /* OEM identification */
> > > -char oem_table_id[8]; /* OEM table identification */
> > > +char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> > > +char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
> > >  uint32_t oem_revision;/* OEM revision number */
> > > -char asl_compiler_id[4];  /* ASL compiler vendor ID */
> > > +char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
> > >  uint32_t asl_compiler_revision; /* ASL compiler revision number */
> > >  } QEMU_PACKED;
> > >
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index af8e023968..3bf0bec8ba 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -43,7 +43,7 @@ enum {
> > >  struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */
> > >  uint64_t signature;  /* ACPI signature, contains "RSD  
> > PTR " */  
> > >  uint8_t  checksum;   /* To make sum of struct == 0 */
> > > -uint8_t  oem_id [6]; /* OEM identification */
> > > +uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
> > >  uint8_t  revision;   /* Must be 0 for 1.0, 2 for 2.0 */
> > >  uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT  
> > */  
> > >  uint32_t length; /* XSDT Length in bytes including  
> > hdr */
> >
> > you'll need to rebase this on top the latest Michael's pull request.
> > [PULL v2 25/30] hw: arm: Carry RSDP specific data through  AcpiRsdpData
> > [PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests
> >  
> 
> OK. Can I add your Ack-by then?
pls note that new AcpiRsdpData has oem_id field that needs the same treatment

with rebase
Reviewed-by: Igor Mammedov 

> 
> > @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;  
> > >  uint32_t length; /* Length of table, in bytes,  
> >

Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] hw/acpi: Use QEMU_NONSTRING for non NUL-terminated arrays

[Igor Mammedov]

On Tue, 18 Dec 2018 18:51:20 +0100
Philippe Mathieu-Daudé  wrote:

> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
> CC  hw/acpi/core.o
>   In function 'acpi_table_install', inlined from 'acpi_table_add' at 
> qemu/hw/acpi/core.c:296:5:
>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals 
> destination size [-Werror=stringop-truncation]
>strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
>^
>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> 
> Use the QEMU_NONSTRING attribute, since ACPI tables don't require the
> strings to be NUL-terminated.
> 
> Suggested-by: Michael S. Tsirkin 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/acpi/core.c  | 8 
>  include/hw/acpi/acpi-defs.h | 8 
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..f60f750c3d 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -35,14 +35,14 @@
>  struct acpi_table_header {
>  uint16_t _length; /* our length, not actual part of the hdr */
>/* allows easier parsing for fw_cfg clients */
> -char sig[4];  /* ACPI signature (4 ASCII characters) */
> +char sig[4] QEMU_NONSTRING; /* ACPI signature (4 ASCII characters) */
>  uint32_t length;  /* Length of table, in bytes, including header 
> */
>  uint8_t revision; /* ACPI Specification minor version # */
>  uint8_t checksum; /* To make sum of entire table == 0 */
> -char oem_id[6];   /* OEM identification */
> -char oem_table_id[8]; /* OEM table identification */
> +char oem_id[6] QEMU_NONSTRING; /* OEM identification */
> +char oem_table_id[8] QEMU_NONSTRING; /* OEM table identification */
>  uint32_t oem_revision;/* OEM revision number */
> -char asl_compiler_id[4];  /* ASL compiler vendor ID */
> +char asl_compiler_id[4] QEMU_NONSTRING; /* ASL compiler vendor ID */
>  uint32_t asl_compiler_revision; /* ASL compiler revision number */
>  } QEMU_PACKED;
>  
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index af8e023968..3bf0bec8ba 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -43,7 +43,7 @@ enum {
>  struct AcpiRsdpDescriptor {/* Root System Descriptor Pointer */
>  uint64_t signature;  /* ACPI signature, contains "RSD PTR " 
> */
>  uint8_t  checksum;   /* To make sum of struct == 0 */
> -uint8_t  oem_id [6]; /* OEM identification */
> +uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */
>  uint8_t  revision;   /* Must be 0 for 1.0, 2 for 2.0 */
>  uint32_t rsdt_physical_address;  /* 32-bit physical address of RSDT */
>  uint32_t length; /* XSDT Length in bytes including hdr */

you'll need to rebase this on top the latest Michael's pull request.
[PULL v2 25/30] hw: arm: Carry RSDP specific data through  AcpiRsdpData
[PULL v2 29/30] hw: acpi: Remove AcpiRsdpDescriptor and fix tests

> @@ -62,10 +62,10 @@ typedef struct AcpiRsdpDescriptor AcpiRsdpDescriptor;
>  uint32_t length; /* Length of table, in bytes, including 
> header */ \
>  uint8_t  revision;   /* ACPI Specification minor version # 
> */ \
>  uint8_t  checksum;   /* To make sum of entire table == 0 */ \
> -uint8_t  oem_id [6]; /* OEM identification */ \
> -uint8_t  oem_table_id [8];   /* OEM table identification */ \
> +uint8_t  oem_id [6] QEMU_NONSTRING; /* OEM identification */ \
> +uint8_t  oem_table_id [8] QEMU_NONSTRING; /* OEM table identification */ 
> \
>  uint32_t oem_revision;   /* OEM revision number */ \
> -uint8_t  asl_compiler_id [4];/* ASL compiler vendor ID */ \
> +uint8_t  asl_compiler_id [4] QEMU_NONSTRING; /* ASL compiler vendor ID 
> */ \
>  uint32_t asl_compiler_revision;  /* ASL compiler revision number */
>  
>  

Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/3] hw/acpi: Replace strncpy() by strpadcpy(pad='\0')

[Igor Mammedov]

On Tue, 18 Dec 2018 12:03:31 +0100
Philippe Mathieu-Daudé  wrote:

> From: Marc-André Lureau 
> 
> GCC 8 added a -Wstringop-truncation warning:
> 
>   The -Wstringop-truncation warning added in GCC 8.0 via r254630 for
>   bug 81117 is specifically intended to highlight likely unintended
>   uses of the strncpy function that truncate the terminating NUL
>   character from the source string.
> 
> This new warning leads to compilation failures:
> 
> CC  hw/acpi/core.o
>   In function 'acpi_table_install', inlined from 'acpi_table_add' at 
> qemu/hw/acpi/core.c:296:5:
>   qemu/hw/acpi/core.c:184:9: error: 'strncpy' specified bound 4 equals 
> destination size [-Werror=stringop-truncation]
>strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
>^
>   make: *** [qemu/rules.mak:69: hw/acpi/core.o] Error 1
> 
> The ACPI tables don't require the strings to be NUL-terminated,
> therefore strncpy is the right function to use here.
> 
> We could add a #pragma GCC diagnostic ignored "-Wstringop-truncation"
> around, disable the warning globally using -Wno-stringop-truncation,
> but since QEMU provides the strpadcpy() which does the same purpose,
> simply use it to avoid the annoying warning.
> 
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Eric Blake 
> Reviewed-by: Philippe Mathieu-Daudé 
> [PMD: reword commit subject and description]
> Signed-off-by: Philippe Mathieu-Daudé 

Reviewed-by: Igor Mammedov 

> ---
>  hw/acpi/aml-build.c |  6 --
>  hw/acpi/core.c  | 13 +++--
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..397833462a 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/aml-build.h"
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
> +#include "qemu/cutils.h"
>  #include "sysemu/numa.h"
>  
>  static GArray *build_alloc_array(void)
> @@ -1532,13 +1533,14 @@ build_header(BIOSLinker *linker, GArray *table_data,
>  h->revision = rev;
>  
>  if (oem_id) {
> -strncpy((char *)h->oem_id, oem_id, sizeof h->oem_id);
> +strpadcpy((char *)h->oem_id, sizeof h->oem_id, oem_id, '\0');
>  } else {
>  memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>  }
>  
>  if (oem_table_id) {
> -strncpy((char *)h->oem_table_id, oem_table_id, 
> sizeof(h->oem_table_id));
> +strpadcpy((char *)h->oem_table_id, sizeof(h->oem_table_id),
> +  oem_table_id, '\0');
>  } else {
>  memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>  memcpy(h->oem_table_id + 4, sig, 4);
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index aafdc61648..6e8f4e5713 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -31,6 +31,7 @@
>  #include "qapi/qapi-visit-misc.h"
>  #include "qemu/error-report.h"
>  #include "qemu/option.h"
> +#include "qemu/cutils.h"
>  
>  struct acpi_table_header {
>  uint16_t _length; /* our length, not actual part of the hdr */
> @@ -181,7 +182,7 @@ static void acpi_table_install(const char unsigned *blob, 
> size_t bloblen,
>  ext_hdr->_length = cpu_to_le16(acpi_payload_size);
>  
>  if (hdrs->has_sig) {
> -strncpy(ext_hdr->sig, hdrs->sig, sizeof ext_hdr->sig);
> +strpadcpy(ext_hdr->sig, sizeof ext_hdr->sig, hdrs->sig, '\0');
>  ++changed_fields;
>  }
>  
> @@ -200,12 +201,12 @@ static void acpi_table_install(const char unsigned 
> *blob, size_t bloblen,
>  ext_hdr->checksum = 0;
>  
>  if (hdrs->has_oem_id) {
> -strncpy(ext_hdr->oem_id, hdrs->oem_id, sizeof ext_hdr->oem_id);
> +strpadcpy(ext_hdr->oem_id, sizeof ext_hdr->oem_id, hdrs->oem_id, 
> '\0');
>  ++changed_fields;
>  }
>  if (hdrs->has_oem_table_id) {
> -strncpy(ext_hdr->oem_table_id, hdrs->oem_table_id,
> -sizeof ext_hdr->oem_table_id);
> +strpadcpy(ext_hdr->oem_table_id, sizeof ext_hdr->oem_table_id,
> +  hdrs->oem_table_id, '\0');
>  ++changed_fields;
>  }
>  if (hdrs->has_oem_rev) {
> @@ -213,8 +214,8 @@ static void acpi_table_install(const char unsigned *blob, 
> size_t bloblen,
>  ++changed_fields;
>  }
>  if (hdrs->has_asl_compiler_id) {
> -strncpy(ext_hdr->asl_compiler_id, hdrs->asl_compiler_id,
> -sizeof ext_hdr->asl_compiler_id);
> +strpadcpy(ext_hdr->asl_compiler_id, sizeof ext_hdr->asl_compiler_id,
> +  hdrs->asl_compiler_id, '\0');
>  ++changed_fields;
>  }
>  if (hdrs->has_asl_compiler_rev) {

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] nvme: don't unref ctrl_mem when device unrealized

[Igor Mammedov]

On Sun, 28 Oct 2018 23:29:40 -0700
Li Qiang  wrote:

> Currently, when hotplug/unhotplug nvme device, it will cause an
> assert in object.c. Following is the backtrack:
> 
> ERROR:qom/object.c:981:object_unref: assertion failed: (obj->ref > 0)
> 
> Thread 2 "qemu-system-x86" received signal SIGABRT, Aborted.
> [Switching to Thread 0x7fffcbd32700 (LWP 18844)]
> 0x7fffdb9e4fff in raise () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) bt
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> qom/object.c:981
> /home/liqiang02/qemu-upstream/qemu/memory.c:1732
> /home/liqiang02/qemu-upstream/qemu/memory.c:285
> util/qemu-thread-posix.c:504
> /lib/x86_64-linux-gnu/libpthread.so.0
> 
> This is caused by memory_region_unref in nvme_exit.
> 
> Remove it to make the PCIdevice refcount correct.
> 
> Signed-off-by: Li Qiang 
nvme device holds a reference to ctrl_mem MemoryRegion as a parent
so MemoryRegion will be destroyed later during destruction of
nvme object when its cildren are un-parented.

Reviewed-by: Igor Mammedov 

> ---
>  hw/block/nvme.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index fc7dacb816..359a06d0ad 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1331,9 +1331,6 @@ static void nvme_exit(PCIDevice *pci_dev)
>  g_free(n->namespaces);
>  g_free(n->cq);
>  g_free(n->sq);
> -if (n->cmbsz) {
> -memory_region_unref(>ctrl_mem);
> -}
>  
>  msix_uninit_exclusive_bar(pci_dev);
>  }

[Qemu-block] [PATCH for-2.11] ide: ahci: unparent children buses before freeing their memory

[Igor Mammedov]

Fixes read after freeing error reported
  https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04243.html
  Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>

ich9-ahci device creates ide buses and attaches them as QOM children
at realize time, however it forgets to properly clean them up
at unrealize time and frees memory containing these children,
with following call-chain:

   qdev_device_add()
 object_property_set_bool('realized', true)
   device_set_realized()
  ...
  pci_qdev_realize() -> pci_ich9_ahci_realize() -> ahci_realize()
   ...
   s->dev = g_new0(AHCIDevice, ports);
   ...
  AHCIDevice *ad = >dev[i];
  ide_bus_new(>port, sizeof(ad->port), qdev, i, 1);
  ^^^ creates bus in memory allocated by above gnew()
  and adds it as child propety to ahci device
  ...
  hotplug_handler_plug(); -> goto post_realize_fail;
  pci_qdev_unrealize() -> pci_ich9_uninit() -> ahci_uninit()
  ...
   g_free(s->dev);
   ^^^ free memory that holds children busses

  return with error from device_set_realized()

As result later when qdev_device_add() tries to unparent ich9-ahci
after failed device_set_realized(),
object_unparent() -> object_property_del_child()
iterates over existing QOM children including buses added by
ide_bus_new() and tries to unparent them, which causes access to
freed memory where they where located.

Reported-by: Thomas Huth <th...@redhat.com>
Signed-off-by: Igor Mammedov <imamm...@redhat.com>
---
 hw/ide/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 406a1b5..ccbe091 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1495,6 +1495,7 @@ void ahci_uninit(AHCIState *s)
 
 ide_exit(s);
 }
+object_unparent(OBJECT(>port));
 }
 
 g_free(s->dev);
-- 
2.7.4

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Igor Mammedov]

On Wed, 23 Aug 2017 08:04:06 +0200
Thomas Huth  wrote:

> On 23.08.2017 07:40, Thomas Huth wrote:
> > On 23.08.2017 02:10, Philippe Mathieu-Daudé wrote:  
> >> On 08/22/2017 07:42 PM, Michael S. Tsirkin wrote:  
> >>> On Tue, Aug 22, 2017 at 06:43:43PM -0300, Philippe Mathieu-Daudé wrote:  
>  9e047b982452 "piix4: add acpi pci hotplug support" introduced a new
>  property
>  'use_acpi_pci_hotplug' for pc-1.7 and older machines.
>  c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API"
>  added the
>  qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
>  property.
> 
>  Check for use_acpi_pci_hotplug before calling
>  acpi_pcihp_device_[un]plug_cb().  
> > [...]  
>  Reported-by: Thomas Huth 
>  Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
>  Signed-off-by: Philippe Mathieu-Daudé   
> >>>
> >>> Looks like this is a very old bug, isn't it?
> >>> Objections to merging this after the release?  
> >>
> >> Yes, I'm also inclined to delay it so we can release 2.10, I tagged
> >> "2.10-rc4" since Thomas sent it as a bug within the 2.10 window so I'll
> >> let him decide if it is worth crying wolf :) It's very likely no-one but
> >> him used pre-pc-i440fx-1.7 the last 3 years, not even thinking about hot
> >> plugging AHCI devices :D  
> > 
> > I'm fine if this gets included in 2.11 - it's quite unlikely that a user
> > tries hot-plug ahci on such an old machine type, I think. But we maybe
question is should be ahci device by hotpluggable at all?

Re: [Qemu-block] [PATCH for-2.10-rc4?] acpi: pcihp: fix use-after-free for machines previous pc-1.7 compat

[Igor Mammedov]

On Tue, 22 Aug 2017 18:43:43 -0300
Philippe Mathieu-Daudé  wrote:

> 9e047b982452 "piix4: add acpi pci hotplug support" introduced a new property
> 'use_acpi_pci_hotplug' for pc-1.7 and older machines.
> c24d5e0b91d1 "convert ACPI PCI hotplug to use hotplug-handler API" added the
> qbus hotplug handlers but forgot to check for the 'use_acpi_pci_hotplug'
> property.
> 
> Check for use_acpi_pci_hotplug before calling acpi_pcihp_device_[un]plug_cb().
> 
> If Xen is enabled, piix4_pm_init() disables use_acpi_pci_hotplug.
> 
> The following valgrind Trace equivs:
> 
>   qdev_device_add( "ich9-ahci" )
>-> device_set_realized()
> -> hotplug_handler_plug()
>  -> piix4_device_plug_cb()
>   -> acpi_pcihp_device_plug_cb()
>-> acpi_pcihp_get_bsel() "Property ACPI_PCIHP_PROP_BSEL not found"
>-> object_unparent()  
>  <- "Bus doesn't have property ACPI_PCIHP_PROP_BSEL set"
> 
> $ valgrind x86_64-softmmu/qemu-system-x86_64 -M pc-1.2 -nographic -S
> (qemu) device_add ich9-ahci,id=ich9-ahci
> ==6604== Invalid read of size 8
> ==6604==at 0x609AB0: object_unparent (object.c:445)
> ==6604==by 0x4C4478: device_unparent (qdev.c:1095)
> ==6604==by 0x60A364: object_finalize_child_property (object.c:1396)
> ==6604==by 0x6092A6: object_property_del_child.isra.7 (object.c:427)
> ==6604==by 0x451728: qdev_device_add (qdev-monitor.c:634)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==by 0x365439: monitor_command_cb (monitor.c:3922)
> ==6604==by 0x6E5D27: readline_handle_byte (readline.c:393)
> ==6604==by 0x364311: monitor_read (monitor.c:3905)
> ==6604==by 0x67C573: mux_chr_read (char-mux.c:216)
> ==6604==  Address 0x15fc5448 is 30,328 bytes inside a block of size 36,288 
> free'd
> ==6604==at 0x4C2ACDD: free (vg_replace_malloc.c:530)
> ==6604==by 0xA04EBCD: g_free (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50100E: pci_ich9_uninit (ich.c:161)
> ==6604==by 0x5428AB: pci_qdev_unrealize (pci.c:1083)
> ==6604==by 0x4C5EE9: device_set_realized (qdev.c:988)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> ==6604==by 0x364083: handle_hmp_command (monitor.c:3119)
> ==6604==  Block was alloc'd at
> ==6604==at 0x4C2B975: calloc (vg_replace_malloc.c:711)
> ==6604==by 0xA04EB15: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.5000.3)
> ==6604==by 0x50094F: ahci_realize (ahci.c:1468)
> ==6604==by 0x501098: pci_ich9_ahci_realize (ich.c:115)
> ==6604==by 0x543E6D: pci_qdev_realize (pci.c:2002)
> ==6604==by 0x4C5E69: device_set_realized (qdev.c:914)
> ==6604==by 0x608DCD: property_set_bool (object.c:1886)
> ==6604==by 0x60CEBE: object_property_set_qobject (qom-qobject.c:27)
> ==6604==by 0x60AB6F: object_property_set_bool (object.c:1162)
> ==6604==by 0x4516F3: qdev_device_add (qdev-monitor.c:630)
> ==6604==by 0x451C82: qmp_device_add (qdev-monitor.c:807)
> ==6604==by 0x46B689: hmp_device_add (hmp.c:1925)
> 
> Reported-by: Thomas Huth 
> Message-Id: <59a56959-ca12-ea75-33fa-ff07eba1b...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  hw/acpi/piix4.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f276967365..d4df209a2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -385,7 +385,7 @@ static void piix4_device_plug_cb(HotplugHandler 
> *hotplug_dev,
>  dev, errp);
>  }
>  } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -if (!xen_enabled()) {
> +if (s->use_acpi_pci_hotplug) {
>  acpi_pcihp_device_plug_cb(hotplug_dev, >acpi_pci_hotplug, dev,
>errp);
>  }
s->use_acpi_pci_hotplug is a switch between legacy and current modes
of PCI hotplug and acpi_pcihp_device_plug_cb() should be able to handle
both.

Looking at history train-wreck started since
 (f0c9d64 pc: pcihp: avoid adding ACPI_PCIHP_PROP_BSEL twice)
when bsel became solely allocated by another component (acpi table builder)
with follow up fixups to make that split brain (hw(legacy|modern)/acpi(in 
seabios|in qemu))
combos work somehow.


I think Anthony's (CCed) patches is the right way to fix issues not only
for Xen but also for legacy SeaBIOS as well.

[PATCH for-2.10 v3 0/3] Fix hotplug of PCI passthrought dev
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg03216.html
Series still needs to 

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Igor Mammedov]

On Tue, 22 Aug 2017 15:50:14 +0200
Markus Armbruster <arm...@redhat.com> wrote:

> Igor Mammedov <imamm...@redhat.com> writes:
> 
> > On Mon,  7 Aug 2017 16:45:16 +0200
> > Markus Armbruster <arm...@redhat.com> wrote:
> >  
> >> Sizes and addresses should use QAPI type 'size' (uint64_t).
> >> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
> >> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
> >> 
> >> Change these PCDIMMDeviceInfo members to 'size'.
> >> 
> >> query-memory-devices now reports sizes and addresses above 2^63-1
> >> correctly instead of their (negative) two's complement.
> >> 
> >> HMP's "info memory-devices" already reported them correctly, because
> >> it printed the signed integers with PRIx64 and PRIu32.  
> > s/signed/unsigned/  
> 
> Before this patch: signed.  Afterwards: unsigned.  Would
> 
>HMP's "info memory-devices" already reported them correctly, because
>it printed the signed (before the patch) integers with PRIx64 and
>PRIu32.
> 
> be clearer?
yes, that's more clear

Thanks.

> 
> >> Signed-off-by: Markus Armbruster <arm...@redhat.com>  
> > Reviewed-by: Igor Mammedov <imamm...@redhat.com>  
> 
> Thanks!

Re: [Qemu-block] [Qemu-devel] [RFC PATCH 12/56] pc-dimm: Make size and address unsigned in QAPI/QMP

[Igor Mammedov]

On Mon,  7 Aug 2017 16:45:16 +0200
Markus Armbruster <arm...@redhat.com> wrote:

> Sizes and addresses should use QAPI type 'size' (uint64_t).
> PCDIMMDeviceInfo members @addr and @size are 'int' (int64_t).
> qmp_pc_dimm_device_list() implicitly converts from uint64_t.
> 
> Change these PCDIMMDeviceInfo members to 'size'.
> 
> query-memory-devices now reports sizes and addresses above 2^63-1
> correctly instead of their (negative) two's complement.
> 
> HMP's "info memory-devices" already reported them correctly, because
> it printed the signed integers with PRIx64 and PRIu32.
s/signed/unsigned/

 
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Igor Mammedov <imamm...@redhat.com>

> ---
>  qapi-schema.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 23eb60d..6aa6be9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -6057,8 +6057,8 @@
>  ##
>  { 'struct': 'PCDIMMDeviceInfo',
>'data': { '*id': 'str',
> -'addr': 'int',
> -'size': 'int',
> +'addr': 'size',
> +'size': 'size',
>  'slot': 'int',
>  'node': 'int',
>  'memdev': 'str',

[Qemu-block] [PATCH for-2.8] qtail: clean up direct access to tqe_prev field

[Igor Mammedov]

instead of accessing tqe_prev field dircetly outside
of queue.h use macros to check if element is in list
and make sure that afer element is removed from list
tqe_prev field could be used to do the same check.

Signed-off-by: Igor Mammedov <imamm...@redhat.com>
---
The patch is split from as cleanup is not urgent
  [PATCH 0/6] Fix migration issues with arbitrary cpu-hot(un)plug
and made on top of
  [PATCH v2 0/6] Fix migration issues with arbitrary cpu-hot(un)plug

posting it to list so that it won't be forgotten or lost
and affected people could review it at there leisure time.

---
 include/qemu/queue.h | 2 ++
 blockdev.c   | 2 +-
 exec.c   | 3 +--
 net/filter.c | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index c2b6c81..342073f 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -407,6 +407,7 @@ struct {
\
 else\
 (head)->tqh_last = (elm)->field.tqe_prev;   \
 *(elm)->field.tqe_prev = (elm)->field.tqe_next; \
+(elm)->field.tqe_prev = NULL;   \
 } while (/*CONSTCOND*/0)
 
 #define QTAILQ_FOREACH(var, head, field)\
@@ -430,6 +431,7 @@ struct {
\
 #define QTAILQ_EMPTY(head)   ((head)->tqh_first == NULL)
 #define QTAILQ_FIRST(head)   ((head)->tqh_first)
 #define QTAILQ_NEXT(elm, field)  ((elm)->field.tqe_next)
+#define QTAILQ_IN_USE(elm, field)((elm)->field.tqe_prev != NULL)
 
 #define QTAILQ_LAST(head, headname) \
 (*(((struct headname *)((head)->tqh_last))->tqh_last))
diff --git a/blockdev.c b/blockdev.c
index eafeba9..0b73158 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4031,7 +4031,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
 goto out;
 }
 
-if (!blk && !bs->monitor_list.tqe_prev) {
+if (!blk && !QTAILQ_IN_USE(bs, monitor_list)) {
 error_setg(errp, "Node %s is not owned by the monitor",
bs->node_name);
 goto out;
diff --git a/exec.c b/exec.c
index 50e3ee2..8e8416b 100644
--- a/exec.c
+++ b/exec.c
@@ -614,14 +614,13 @@ void cpu_exec_exit(CPUState *cpu)
 CPUClass *cc = CPU_GET_CLASS(cpu);
 
 cpu_list_lock();
-if (cpu->node.tqe_prev == NULL) {
+if (!QTAILQ_IN_USE(cpu, node)) {
 /* there is nothing to undo since cpu_exec_init() hasn't been called */
 cpu_list_unlock();
 return;
 }
 
 QTAILQ_REMOVE(, cpu, node);
-cpu->node.tqe_prev = NULL;
 cpu->cpu_index = UNASSIGNED_CPU_INDEX;
 cpu_list_unlock();
 
diff --git a/net/filter.c b/net/filter.c
index 888fe6d..1dfd2ca 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -239,7 +239,7 @@ static void netfilter_finalize(Object *obj)
 }
 
 if (nf->netdev && !QTAILQ_EMPTY(>netdev->filters) &&
-nf->next.tqe_prev) {
+QTAILQ_IN_USE(nf, next)) {
 QTAILQ_REMOVE(>netdev->filters, nf, next);
 }
 g_free(nf->netdev_id);
-- 
2.7.4

Re: [Qemu-block] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT

[Igor Mammedov]

On Thu, 7 Jan 2016 12:56:58 +0200
"Michael S. Tsirkin" <m...@redhat.com> wrote:

> On Thu, Jan 07, 2016 at 12:56:09PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Jan 06, 2016 at 03:04:40PM +0100, Igor Mammedov wrote:  
> > > On Wed, 30 Dec 2015 23:11:50 +0300
> > > Roman Kagan <rka...@virtuozzo.com> wrote:
> > >   
> > > > Windows on UEFI systems is only capable of detecting the presence and
> > > > the type of floppy drives via corresponding ACPI objects.
> > > > 
> > > > Those objects are added in patch 5; the preceding ones pave the way to
> > > > it, by making the necessary data public and by moving the whole
> > > > floppy drive controller description into runtime-generated SSDT.
> > > > 
> > > > Note that the series conflicts with Igor's patchset for dynamic DSDT, in
> > > > particular, with "[PATCH v2 27/51] pc: acpi: move FDC0 device from DSDT
> > > > to SSDT"; I haven't managed to avoid that while trying to meet
> > > > maintainer's comments.  
> > > 
> > > Tested with XPsp3 WS2008R2 WS2012R2, no regressions so far it boots fine 
> > > and can read floppy.
> > > 
> > > So for whole series:
> > > Reviewed-by: Igor Mammedov <imamm...@redhat.com>
> > >   
> > 
> > Igor, could you pls rebase this on top of your patches?
> > I've merged them in my tree.  
> 
> Pls remember to keep the author information intact though.
Sure,
I'll usually do it or when in doubt I just ask original
authors before changing it.

> 
> > > > Roman Kagan (6):
> > > >   i386/pc: expose identifying the floppy controller
> > > >   i386/acpi: make floppy controller object dynamic
> > > >   tests/acpi: update test data
> > > >   expose floppy drive geometry and CMOS type
> > > >   i386: populate floppy drive information in SSDT
> > > >   tests/acpi: update test data
> > > > 
> > > > Signed-off-by: Roman Kagan <rka...@virtuozzo.com>
> > > > Cc: "Michael S. Tsirkin" <m...@redhat.com>
> > > > Cc: Eduardo Habkost <ehabk...@redhat.com>
> > > > Cc: Igor Mammedov <imamm...@redhat.com>
> > > > Cc: John Snow <js...@redhat.com>
> > > > Cc: Kevin Wolf <kw...@redhat.com>
> > > > Cc: Paolo Bonzini <pbonz...@redhat.com>
> > > > Cc: Richard Henderson <r...@twiddle.net>
> > > > Cc: qemu-block@nongnu.org
> > > > Cc: qemu-sta...@nongnu.org
> > > > ---
> > > > changes since v4:
> > > >  - re-split out code changes from test data updates
> > > > 
> > > > changes since v3:
> > > >  - make FDC object fully dynamic in a separate patch
> > > >  - split out support patches
> > > >  - include test data updates with the respective patches to maintain
> > > >bisectability
> > > > 
> > > > changes since v2:
> > > >  - explicit endianness for buffer data
> > > >  - reorder code to reduce conflicts with dynamic DSDT patchset
> > > >  - update test data
> > > > 
> > > >  hw/block/fdc.c  |  11 +
> > > >  hw/i386/acpi-build.c|  92 
> > > > 
> > > >  hw/i386/acpi-dsdt-isa.dsl   |  18 ---
> > > >  hw/i386/acpi-dsdt.dsl   |   1 -
> > > >  hw/i386/pc.c|  46 ++
> > > >  hw/i386/q35-acpi-dsdt.dsl   |   7 +--
> > > >  include/hw/block/fdc.h  |   2 +
> > > >  include/hw/i386/pc.h|   3 ++
> > > >  tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes
> > > >  tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2635 bytes
> > > >  tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4494 bytes
> > > >  tests/acpi-test-data/q35/DSDT   | Bin 7666 -> 7578 bytes
> > > >  12 files changed, 137 insertions(+), 43 deletions(-)
> > > >   

Re: [Qemu-block] [PATCH v5 0/6] i386: expose floppy-related objects in SSDT

[Igor Mammedov]

On Wed, 30 Dec 2015 23:11:50 +0300
Roman Kagan <rka...@virtuozzo.com> wrote:

> Windows on UEFI systems is only capable of detecting the presence and
> the type of floppy drives via corresponding ACPI objects.
> 
> Those objects are added in patch 5; the preceding ones pave the way to
> it, by making the necessary data public and by moving the whole
> floppy drive controller description into runtime-generated SSDT.
> 
> Note that the series conflicts with Igor's patchset for dynamic DSDT, in
> particular, with "[PATCH v2 27/51] pc: acpi: move FDC0 device from DSDT
> to SSDT"; I haven't managed to avoid that while trying to meet
> maintainer's comments.

Tested with XPsp3 WS2008R2 WS2012R2, no regressions so far it boots fine and 
can read floppy.

So for whole series:
Reviewed-by: Igor Mammedov <imamm...@redhat.com>


> Roman Kagan (6):
>   i386/pc: expose identifying the floppy controller
>   i386/acpi: make floppy controller object dynamic
>   tests/acpi: update test data
>   expose floppy drive geometry and CMOS type
>   i386: populate floppy drive information in SSDT
>   tests/acpi: update test data
> 
> Signed-off-by: Roman Kagan <rka...@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Cc: Eduardo Habkost <ehabk...@redhat.com>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: John Snow <js...@redhat.com>
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Richard Henderson <r...@twiddle.net>
> Cc: qemu-block@nongnu.org
> Cc: qemu-sta...@nongnu.org
> ---
> changes since v4:
>  - re-split out code changes from test data updates
> 
> changes since v3:
>  - make FDC object fully dynamic in a separate patch
>  - split out support patches
>  - include test data updates with the respective patches to maintain
>bisectability
> 
> changes since v2:
>  - explicit endianness for buffer data
>  - reorder code to reduce conflicts with dynamic DSDT patchset
>  - update test data
> 
>  hw/block/fdc.c  |  11 +
>  hw/i386/acpi-build.c|  92 
> 
>  hw/i386/acpi-dsdt-isa.dsl   |  18 ---
>  hw/i386/acpi-dsdt.dsl   |   1 -
>  hw/i386/pc.c|  46 ++
>  hw/i386/q35-acpi-dsdt.dsl   |   7 +--
>  include/hw/block/fdc.h  |   2 +
>  include/hw/i386/pc.h|   3 ++
>  tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes
>  tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2635 bytes
>  tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4494 bytes
>  tests/acpi-test-data/q35/DSDT   | Bin 7666 -> 7578 bytes
>  12 files changed, 137 insertions(+), 43 deletions(-)
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v4 0/4] i386: expose floppy-related objects in SSDT

[Igor Mammedov]

On Tue, 29 Dec 2015 19:17:46 +0300
Roman Kagan <rka...@virtuozzo.com> wrote:

> On Tue, Dec 29, 2015 at 03:09:36PM +0100, Igor Mammedov wrote:
> > On Fri, 25 Dec 2015 18:04:08 +0300
> > Roman Kagan <rka...@virtuozzo.com> wrote:
> > 
> > > Windows on UEFI systems is only capable of detecting the presence and
> > > the type of floppy drives via corresponding ACPI objects.
> > > 
> > > Those objects are added in the last patch of the series; the three
> > > preceding ones pave the way to it, by making the necessary data
> > > public and by moving the whole floppy drive controller description into
> > > runtime-generated SSDT.
> > > 
> > > Note that the series conflicts with Igor's patchset for dynamic DSDT, in
> > > particular, with "[PATCH 50/74] pc: acpi: move FDC0 device from DSDT
> > > to SSDT"; I haven't managed to avoid that while trying to meet
> > > maintainer's comments.
> > To remove conflicts and to make it more suitable for stable, I'd drop
> >  "2/4 i386/acpi: make floppy controller object dynamic"
> 
> Erm...  I thought I did what Michael requested:
> 
> On Thu, Dec 24, 2015 at 08:17:45AM +0200, Michael S. Tsirkin wrote:
> > Or rather, start series with a patch making FDC conditional,
> 
> So what do I need to do to get this thing merged?
> 
> 
> > and split test blob out of
> >  "4/4 i386: populate floppy drive information in SSDT"
> > into a separate patch so one can see effects of applying 4/4
> > and then update blobs if resulting ASL diff is as expected.
> 
> This will break bisectability, won't it?
not really, it's only a warning when AML differs from the expected one.

> 
> Roman.
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v4 0/4] i386: expose floppy-related objects in SSDT

[Igor Mammedov]

On Fri, 25 Dec 2015 18:04:08 +0300
Roman Kagan <rka...@virtuozzo.com> wrote:

> Windows on UEFI systems is only capable of detecting the presence and
> the type of floppy drives via corresponding ACPI objects.
> 
> Those objects are added in the last patch of the series; the three
> preceding ones pave the way to it, by making the necessary data
> public and by moving the whole floppy drive controller description into
> runtime-generated SSDT.
> 
> Note that the series conflicts with Igor's patchset for dynamic DSDT, in
> particular, with "[PATCH 50/74] pc: acpi: move FDC0 device from DSDT
> to SSDT"; I haven't managed to avoid that while trying to meet
> maintainer's comments.
To remove conflicts and to make it more suitable for stable, I'd drop
 "2/4 i386/acpi: make floppy controller object dynamic"
and split test blob out of
 "4/4 i386: populate floppy drive information in SSDT"
into a separate patch so one can see effects of applying 4/4
and then update blobs if resulting ASL diff is as expected.


> 
> Roman Kagan (4):
>   i386/pc: expose identifying the floppy controller
>   i386/acpi: make floppy controller object dynamic
>   expose floppy drive geometry and CMOS type
>   i386: populate floppy drive information in SSDT
> 
> Signed-off-by: Roman Kagan <rka...@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <m...@redhat.com>
> Cc: Eduardo Habkost <ehabk...@redhat.com>
> Cc: Igor Mammedov <imamm...@redhat.com>
> Cc: John Snow <js...@redhat.com>
> Cc: Kevin Wolf <kw...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: Richard Henderson <r...@twiddle.net>
> Cc: qemu-block@nongnu.org
> Cc: qemu-sta...@nongnu.org
> ---
> changes since v3:
>  - make FDC object fully dynamic in a separate patch
>  - split out support patches
>  - include test data updates with the respective patches to maintain
>bisectability
> 
> changes since v2:
>  - explicit endianness for buffer data
>  - reorder code to reduce conflicts with dynamic DSDT patchset
>  - update test data
> 
> 
>  hw/block/fdc.c  |  11 +
>  hw/i386/acpi-build.c|  92 
> 
>  hw/i386/acpi-dsdt-isa.dsl   |  18 ---
>  hw/i386/acpi-dsdt.dsl   |   1 -
>  hw/i386/pc.c|  46 ++
>  hw/i386/q35-acpi-dsdt.dsl   |   7 +--
>  include/hw/block/fdc.h  |   2 +
>  include/hw/i386/pc.h|   3 ++
>  tests/acpi-test-data/pc/DSDT| Bin 3028 -> 2946 bytes
>  tests/acpi-test-data/pc/SSDT| Bin 2486 -> 2635 bytes
>  tests/acpi-test-data/pc/SSDT.bridge | Bin 4345 -> 4494 bytes
>  tests/acpi-test-data/q35/DSDT   | Bin 7666 -> 7578 bytes
>  12 files changed, 137 insertions(+), 43 deletions(-)
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Igor Mammedov]

On Mon, 2 Nov 2015 17:43:16 +
Stefan Hajnoczi  wrote:

> On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote:
> > Use address_space_read to make sure we handle the case of an indirect
> > descriptor crossing DIMM boundary correctly.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> > ---
> > 
> > Warning: compile-tested only.
> 
> Test (with your follow-up patch squashed in) with 6 4KB seqeuential read
> processes on running successfully.
> 
> I didn't test the DIMM boundary case.  Igor: what is the easiest way to
> reproduce the DIMM boundary crossing?

I've tested it, here is QEMU CLI:
qemu-system-x86_64 -enable-kvm -enable-kvm  -m 128M,slots=250,maxmem=32G  
-drive if=none,id=hd,file=rhel72,cache=none,aio=native,format=raw -device 
virtio-blk,drive=hd,scsi=off,config-wce=off,x-data-plane=on `for i in $(seq 0 
15); do echo -n "-object memory-backend-ram,id=m$i,size=10M -device 
pc-dimm,id=dimm$i,memdev=m$i "; done`

boot and then do:
dd if=/dev/vda of=/dev/null bs=16M

without fix it hangs either at boot time or when doing 'dd'

Re: [Qemu-block] [PATCH 2/2] dataplane: support non-contigious s/g

[Igor Mammedov]

On Wed, 28 Oct 2015 17:48:04 +0200
"Michael S. Tsirkin" <m...@redhat.com> wrote:

> bring_map currently fails if one of the entries it's mapping is
> contigious in GPA but not HVA address space.  Introduce a mapped_len
> parameter so it can handle this, returning the actual mapped length.
> 
> This will still fail if there's no space left in the sg, but luckily
> max queue size in use is currently 256, while max sg size is 1024, so
> we should be OK even is all entries happen to cross a single DIMM
> boundary.
> 
> Won't work well with very small DIMM sizes, unfortunately:
> e.g. this will fail with 4K DIMMs where a single
> request might span a large number of DIMMs.
> 
> Let's hope these are uncommon - at least we are not breaking things.
> 
> Reported-by: Stefan Hajnoczi <stefa...@redhat.com>
> Reported-by: Igor Mammedov <imamm...@redhat.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
with later posted fixup
Tested-by: Igor Mammedov <imamm...@redhat.com>

> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 68
> ++--- 1 file changed, 46
> insertions(+), 22 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 0b92fcf..9ae9424 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -25,15 +25,30 @@
>  
>  /* vring_map can be coupled with vring_unmap or (if you still have
> the
>   * value returned in *mr) memory_region_unref.
> + * Returns NULL on failure.
> + * Callers that can handle a partial mapping must supply mapped_len
> pointer to
> + * get the actual length mapped.
> + * Passing mapped_len == NULL requires either a full mapping or a
> failure. */
> -static void *vring_map(MemoryRegion **mr, hwaddr phys, hwaddr len,
> +static void *vring_map(MemoryRegion **mr, hwaddr phys,
> +   hwaddr len, hwaddr *mapped_len,
> bool is_write)
>  {
>  MemoryRegionSection section =
> memory_region_find(get_system_memory(), phys, len);
> +uint64_t size;
>  
> -if (!section.mr || int128_get64(section.size) < len) {
> +if (!section.mr) {
>  goto out;
>  }
> +
> +size = int128_get64(section.size);
> +assert(size);
> +
> +/* Passing mapped_len == NULL requires either a full mapping or
> a failure. */
> +if (!mapped_len && size < len) {
> +goto out;
> +}
> +
>  if (is_write && section.readonly) {
>  goto out;
>  }
> @@ -46,6 +61,10 @@ static void *vring_map(MemoryRegion **mr, hwaddr
> phys, hwaddr len, goto out;
>  }
>  
> +if (mapped_len) {
> +*mapped_len = MIN(size, len);
> +}
> +
>  *mr = section.mr;
>  return memory_region_get_ram_ptr(section.mr) +
> section.offset_within_region; 
> @@ -78,7 +97,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev,
> int n) addr = virtio_queue_get_desc_addr(vdev, n);
>  size = virtio_queue_get_desc_size(vdev, n);
>  /* Map the descriptor area as read only */
> -ptr = vring_map(>mr_desc, addr, size, false);
> +ptr = vring_map(>mr_desc, addr, size, NULL, false);
>  if (!ptr) {
>  error_report("Failed to map 0x%" HWADDR_PRIx " byte for
> vring desc " "at 0x%" HWADDR_PRIx,
> @@ -92,7 +111,7 @@ bool vring_setup(Vring *vring, VirtIODevice *vdev,
> int n) /* Add the size of the used_event_idx */
>  size += sizeof(uint16_t);
>  /* Map the driver area as read only */
> -ptr = vring_map(>mr_avail, addr, size, false);
> +ptr = vring_map(>mr_avail, addr, size, NULL, false);
>  if (!ptr) {
>  error_report("Failed to map 0x%" HWADDR_PRIx " byte for
> vring avail " "at 0x%" HWADDR_PRIx,
> @@ -106,7 +125,7 @@ bool vring_setup(Vring *vring, VirtIODevice
> *vdev, int n) /* Add the size of the avail_event_idx */
>  size += sizeof(uint16_t);
>  /* Map the device area as read-write */
> -ptr = vring_map(>mr_used, addr, size, true);
> +ptr = vring_map(>mr_used, addr, size, NULL, true);
>  if (!ptr) {
>  error_report("Failed to map 0x%" HWADDR_PRIx " byte for
> vring used " "at 0x%" HWADDR_PRIx,
> @@ -206,6 +225,7 @@ static int get_desc(Vring *vring,
> VirtQueueElement *elem, struct iovec *iov;
>  hwaddr *addr;
>  MemoryRegion *mr;
> +hwaddr len;
>  
>  if (desc->flags & VRING_DESC_F_WRITE) {
>  num = >in_num;
> @@ -224,26 +244,30 @@ static int get_desc(Vring *vring,
> VirtQueueElement *

Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] dataplane: simplify indirect descriptor read

[Igor Mammedov]

On Wed, 28 Oct 2015 17:48:02 +0200
"Michael S. Tsirkin" <m...@redhat.com> wrote:

> Use address_space_read to make sure we handle the case of an indirect
> descriptor crossing DIMM boundary correctly.
> 
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
Tested-by: Igor Mammedov <imamm...@redhat.com>

> ---
> 
> Warning: compile-tested only.
> 
>  hw/virtio/dataplane/vring.c | 28 ++--
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 68f1994..0b92fcf 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice
> *vdev, host->next = virtio_lduw_p(vdev, >next);
>  }
>  
> +static bool read_vring_desc(VirtIODevice *vdev,
> +hwaddr guest,
> +struct vring_desc *host)
> +{
> +if (address_space_read(_space_memory, guest,
> MEMTXATTRS_UNSPECIFIED,
> +   (uint8_t *)host, sizeof *host)) {
> +return false;
> +}
> +host->addr = virtio_tswap64(vdev, host->addr);
> +host->len = virtio_tswap32(vdev, host->len);
> +host->flags = virtio_tswap16(vdev, host->flags);
> +host->next = virtio_tswap16(vdev, host->next);
> +return true;
> +}
> +
>  /* This is stolen from linux/drivers/vhost/vhost.c. */
>  static int get_indirect(VirtIODevice *vdev, Vring *vring,
>  VirtQueueElement *elem, struct vring_desc
> *indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice
> *vdev, Vring *vring, }
>  
>  do {
> -struct vring_desc *desc_ptr;
> -MemoryRegion *mr;
> -
>  /* Translate indirect descriptor */
> -desc_ptr = vring_map(,
> - indirect->addr + found * sizeof(desc),
> - sizeof(desc), false);
> -if (!desc_ptr) {
> -error_report("Failed to map indirect descriptor "
> +if (!read_vring_desc(vdev, indirect->addr + found *
> sizeof(desc),
> + )) {
> +error_report("Failed to read indirect descriptor "
>   "addr %#" PRIx64 " len %zu",
>   (uint64_t)indirect->addr + found *
> sizeof(desc), sizeof(desc));
>  vring->broken = true;
>  return -EFAULT;
>  }
> -copy_in_vring_desc(vdev, desc_ptr, );
> -memory_region_unref(mr);
>  
>  /* Ensure descriptor has been loaded before accessing fields
> */ barrier(); /* read_barrier_depends(); */