Re: [PATCH v3 14/29] target/i386: Prefer fast cpu_env() over slower CPU QOM cast macro

[Igor Mammedov]

On Mon, 29 Jan 2024 17:44:56 +0100
Philippe Mathieu-Daudé  wrote:

> Mechanical patch produced running the command documented
> in scripts/coccinelle/cpu_env.cocci_template header.


commenting here since, I'm not expert on coccinelle scripts.

On negative side we are permanently loosing type checking in this area.
Is it worth it, what gains do we get with this series?

Side note,
QOM cast expenses you are replacing could be negated by disabling
CONFIG_QOM_CAST_DEBUG without killing type check code when it's enabled.
That way you will speed up not only cpuenv access but also all other casts
across the board.

> Signed-off-by: Philippe Mathieu-Daudé 
> ---
...
>  static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 &= ~HF2_NMI_MASK;

> +cpu_env(cpu)->hflags2 &= ~HF2_NMI_MASK;

this style of de-referencing return value of macro/function
was discouraged in past and preferred way was 'Foo f = CAST(me); f->some_access

(it's just imprint speaking, I don't recall where it comes from)

>  uint32_t gi = (uint32_t) rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi &= ~VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> @@ -207,10 +203,7 @@ static inline void vmx_clear_nmi_blocking(CPUState *cpu)
>  
>  static inline void vmx_set_nmi_blocking(CPUState *cpu)
>  {
> -X86CPU *x86_cpu = X86_CPU(cpu);
> -CPUX86State *env = _cpu->env;
> -
> -env->hflags2 |= HF2_NMI_MASK;
> +cpu_env(cpu)->hflags2 |= HF2_NMI_MASK;
>  uint32_t gi = (uint32_t)rvmcs(cpu->accel->fd, 
> VMCS_GUEST_INTERRUPTIBILITY);
>  gi |= VMCS_INTERRUPTIBILITY_NMI_BLOCKING;
>  wvmcs(cpu->accel->fd, VMCS_GUEST_INTERRUPTIBILITY, gi);
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index 7362daa45a..5239cd40fa 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -155,8 +155,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>  
>  void fw_cfg_build_feature_control(MachineState *ms, FWCfgState *fw_cfg)
>  {
> -X86CPU *cpu = X86_CPU(ms->possible_cpus->cpus[0].cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(ms->possible_cpus->cpus[0].cpu);
>  uint32_t unused, ebx, ecx, edx;
>  uint64_t feature_control_bits = 0;
>  uint64_t *val;
> diff --git a/hw/i386/vmmouse.c b/hw/i386/vmmouse.c
> index a8d014d09a..f292a14a15 100644
> --- a/hw/i386/vmmouse.c
> +++ b/hw/i386/vmmouse.c
> @@ -74,8 +74,7 @@ struct VMMouseState {
>  
>  static void vmmouse_get_data(uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  data[0] = env->regs[R_EAX]; data[1] = env->regs[R_EBX];
>  data[2] = env->regs[R_ECX]; data[3] = env->regs[R_EDX];
> @@ -84,8 +83,7 @@ static void vmmouse_get_data(uint32_t *data)
>  
>  static void vmmouse_set_data(const uint32_t *data)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  env->regs[R_EAX] = data[0]; env->regs[R_EBX] = data[1];
>  env->regs[R_ECX] = data[2]; env->regs[R_EDX] = data[3];
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index f42621e674..61e5060117 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -487,8 +487,7 @@ static void regs_to_cpu(vmware_regs_t *vmport_regs, 
> ioreq_t *req)
>  
>  static void regs_from_cpu(vmware_regs_t *vmport_regs)
>  {
> -X86CPU *cpu = X86_CPU(current_cpu);
> -CPUX86State *env = >env;
> +CPUX86State *env = cpu_env(current_cpu);
>  
>  vmport_regs->ebx = env->regs[R_EBX];
>  vmport_regs->ecx = env->regs[R_ECX];
> diff --git a/target/i386/arch_dump.c b/target/i386/arch_dump.c
> index c290910a04..8939ff9fa9 100644
> --- a/target/i386/arch_dump.c
> +++ b/target/i386/arch_dump.c
> @@ -203,7 +203,6 @@ int x86_cpu_write_elf64_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>   int cpuid, DumpState *s)
>  {
> -X86CPU *cpu = X86_CPU(cs);
>  x86_elf_prstatus prstatus;
>  Elf32_Nhdr *note;
>  char *buf;
> @@ -211,7 +210,7 @@ int x86_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>  const char *name = "CORE";
>  int ret;
>  
> -x86_fill_elf_prstatus(, >env, cpuid);
> +x86_fill_elf_prstatus(, cpu_env(cs), cpuid);
>  descsz = sizeof(x86_elf_prstatus);
>  note_size = ELF_NOTE_SIZE(sizeof(Elf32_Nhdr), name_size, descsz);
>  note = g_malloc0(note_size);
> @@ -381,17 +380,13 @@ static inline int 
> cpu_write_qemu_note(WriteCoreDumpFunction f,
>  int x86_cpu_write_elf64_qemunote(WriteCoreDumpFunction f, CPUState *cs,
>   DumpState *s)
>  {
> -X86CPU *cpu = X86_CPU(cs);
> -
> -return cpu_write_qemu_note(f, >env, s, 1);
> +

Re: [PATCH v2 6/6] xen_arm: Add virtual PCIe host bridge support

[Igor Mammedov]

On Tue, 21 Nov 2023 22:10:28 +
Volodymyr Babchuk  wrote:

> From: Oleksandr Tyshchenko 
> 
> The bridge is needed for virtio-pci support, as QEMU can emulate the
> whole bridge with any virtio-pci devices connected to it.
> 
> This patch provides a flexible way to configure PCIe brige resources
> with xenstore. We made this for several reasons:
> 
> - We don't want to clash with vPCI devices, so we need information
>   from Xen toolstack on which PCI bus to use.
> - The guest memory layout that describes these resources is not stable
>   and may vary between guests, so we cannot rely on static resources
>   to be always the same for both ends.
> - Also the device-models which run in different domains and serve
>   virtio-pci devices for the same guest should use different host
>   bridge resources for Xen to distinguish. The rule for the guest
>   device-tree generation is one PCI host bridge per backend domain.
> 
> Signed-off-by: Oleksandr Tyshchenko 
> Signed-off-by: Volodymyr Babchuk 
> 
> ---
> 
> Changes from v1:
> 
>  - Renamed virtio_pci_host to pcie_host entries in XenStore, because
>  there is nothing specific to virtio-pci: any PCI device can be
>  emulated via this newly created bridge.
> ---
>  hw/arm/xen_arm.c| 186 
>  hw/xen/xen-hvm-common.c |   9 +-
>  include/hw/xen/xen_native.h |   8 +-
>  3 files changed, 200 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
> index b9c3ae14b6..d506d55d0f 100644
> --- a/hw/arm/xen_arm.c
> +++ b/hw/arm/xen_arm.c
> @@ -22,6 +22,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  #include "qemu/error-report.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/visitor.h"
> @@ -34,6 +35,9 @@
>  #include "hw/xen/xen-hvm-common.h"
>  #include "sysemu/tpm.h"
>  #include "hw/xen/arch_hvm.h"
> +#include "exec/address-spaces.h"
> +#include "hw/pci-host/gpex.h"
> +#include "hw/virtio/virtio-pci.h"
>  
>  #define TYPE_XEN_ARM  MACHINE_TYPE_NAME("xenpvh")
>  OBJECT_DECLARE_SIMPLE_TYPE(XenArmState, XEN_ARM)
> @@ -58,6 +62,11 @@ struct XenArmState {
>  struct {
>  uint64_t tpm_base_addr;
>  } cfg;
> +
> +MemMapEntry pcie_mmio;
> +MemMapEntry pcie_ecam;
> +MemMapEntry pcie_mmio_high;
> +int pcie_irq;
>  };
>  
>  static MemoryRegion ram_lo, ram_hi;
> @@ -73,6 +82,7 @@ static MemoryRegion ram_lo, ram_hi;
>  #define NR_VIRTIO_MMIO_DEVICES   \
> (GUEST_VIRTIO_MMIO_SPI_LAST - GUEST_VIRTIO_MMIO_SPI_FIRST)
>  
> +/* TODO It should be xendevicemodel_set_pci_intx_level() for PCI interrupts. 
> */
>  static void xen_set_irq(void *opaque, int irq, int level)
>  {
>  if (xendevicemodel_set_irq_level(xen_dmod, xen_domid, irq, level)) {
> @@ -129,6 +139,176 @@ static void xen_init_ram(MachineState *machine)
>  }
>  }
>  
> +static void xen_create_pcie(XenArmState *xam)
> +{
> +MemoryRegion *mmio_alias, *mmio_alias_high, *mmio_reg;
> +MemoryRegion *ecam_alias, *ecam_reg;
> +DeviceState *dev;
> +int i;
> +
> +dev = qdev_new(TYPE_GPEX_HOST);
> +sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
> +
> +/* Map ECAM space */
> +ecam_alias = g_new0(MemoryRegion, 1);
> +ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> +memory_region_init_alias(ecam_alias, OBJECT(dev), "pcie-ecam",
> + ecam_reg, 0, xam->pcie_ecam.size);
> +memory_region_add_subregion(get_system_memory(), xam->pcie_ecam.base,
> +ecam_alias);
> +
> +/* Map the MMIO space */
> +mmio_alias = g_new0(MemoryRegion, 1);
> +mmio_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
> +memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio",
> + mmio_reg,
> + xam->pcie_mmio.base,
> + xam->pcie_mmio.size);
> +memory_region_add_subregion(get_system_memory(), xam->pcie_mmio.base,
> +mmio_alias);
> +
> +/* Map the MMIO_HIGH space */
> +mmio_alias_high = g_new0(MemoryRegion, 1);
> +memory_region_init_alias(mmio_alias_high, OBJECT(dev), "pcie-mmio-high",
> + mmio_reg,
> + xam->pcie_mmio_high.base,
> + xam->pcie_mmio_high.size);
> +memory_region_add_subregion(get_system_memory(), 
> xam->pcie_mmio_high.base,
> +mmio_alias_high);
> +
> +/* Legacy PCI interrupts (#INTA - #INTD) */
> +for (i = 0; i < GPEX_NUM_IRQS; i++) {
> +qemu_irq irq = qemu_allocate_irq(xen_set_irq, NULL,
> + xam->pcie_irq + i);
> +
> +sysbus_connect_irq(SYS_BUS_DEVICE(dev), i, irq);
> +gpex_set_irq_num(GPEX_HOST(dev), i, xam->pcie_irq + i);
> +}
> +

> +DPRINTF("Created PCIe host bridge\n");
replace DPRINTFs with trace_foo(), see 

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 v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Tue, 17 Jan 2023 09:50:23 -0500
Chuck Zmudzinski  wrote:

> On 1/17/2023 5:35 AM, Igor Mammedov wrote:
> > On Mon, 16 Jan 2023 13:00:53 -0500
> > Chuck Zmudzinski  wrote:
> >  
> > > On 1/16/23 10:33, Igor Mammedov wrote:  
> > > > On Fri, 13 Jan 2023 16:31:26 -0500
> > > > Chuck Zmudzinski  wrote:
> > > > 
> > > >> On 1/13/23 4:33 AM, Igor Mammedov wrote:
> > > >> > On Thu, 12 Jan 2023 23:14:26 -0500
> > > >> > Chuck Zmudzinski  wrote:
> > > >> >   
> > > >> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:  
> > > >> >> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote: 
> > > >> >> >
> > > >> >> >> I think the change Michael suggests is very minimalistic: Move 
> > > >> >> >> the if
> > > >> >> >> condition around xen_igd_reserve_slot() into the function itself 
> > > >> >> >> and
> > > >> >> >> always call it there unconditionally -- basically turning three 
> > > >> >> >> lines
> > > >> >> >> into one. Since xen_igd_reserve_slot() seems very problem 
> > > >> >> >> specific,
> > > >> >> >> Michael further suggests to rename it to something more general. 
> > > >> >> >> All
> > > >> >> >> in all no big changes required.
> > > >> >> > 
> > > >> >> > yes, exactly.
> > > >> >> > 
> > > >> >> 
> > > >> >> OK, got it. I can do that along with the other suggestions.  
> > > >> > 
> > > >> > have you considered instead of reservation, putting a slot check in 
> > > >> > device model
> > > >> > and if it's intel igd being passed through, fail at realize time  if 
> > > >> > it can't take
> > > >> > required slot (with a error directing user to fix command line)? 
> > > >> >  
> > > >> 
> > > >> Yes, but the core pci code currently already fails at realize time
> > > >> with a useful error message if the user tries to use slot 2 for the
> > > >> igd, because of the xen platform device which has slot 2. The user
> > > >> can fix this without patching qemu, but having the user fix it on
> > > >> the command line is not the best way to solve the problem, primarily
> > > >> because the user would need to hotplug the xen platform device via a
> > > >> command line option instead of having the xen platform device added by
> > > >> pc_xen_hvm_init functions almost immediately after creating the pci
> > > >> bus, and that delay in adding the xen platform device degrades
> > > >> startup performance of the guest.
> > > >> 
> > > >> > That could be less complicated than dealing with slot reservations 
> > > >> > at the cost of
> > > >> > being less convenient.  
> > > >> 
> > > >> And also a cost of reduced startup performance
> > > > 
> > > > Could you clarify how it affects performance (and how much).
> > > > (as I see, setup done at board_init time is roughly the same
> > > > as with '-device foo' CLI options, modulo time needed to parse
> > > > options which should be negligible. and both ways are done before
> > > > guest runs)
> > > 
> > > I preface my answer by saying there is a v9, but you don't
> > > need to look at that. I will answer all your questions here.
> > > 
> > > I am going by what I observe on the main HDMI display with the
> > > different approaches. With the approach of not patching Qemu
> > > to fix this, which requires adding the Xen platform device a
> > > little later, the length of time it takes to fully load the
> > > guest is increased. I also noticed with Linux guests that use
> > > the grub bootoader, the grub vga driver cannot display the
> > > grub boot menu at the native resolution of the display, which
> > > in the tested case is 1920x1080, when the Xen platform device
> > > is added via a command line option instead of by the
> > > pc_xen_hvm_init_pci fucntion in pc_piix.c, but with this patch
> > > to Qemu, the grub menu is displayed at the full, 1920x1080
> > > native resolution of the display. Once the guest fully loads,
> > > there is no noticeable difference in performance. It is mainly
> > > a degradation in startup performance, not performance once
> > > the guest OS is fully loaded.  
> > above hints on presence of bug[s] in igd-passthru implementation,
> > and this patch effectively hides problem instead of trying to figure
> > out what's wrong and fixing it.
> >  
> 
> I agree that the with the current Qemu there is a bug in the igd-passthru
> implementation. My proposed patch fixes it. So why not support the
> proposed patch with a Reviewed-by tag?
I see the patch as workaround (it might be a reasonable one) and
it's upto xen maintainers to give Reviewed-by/accept it.

Though I'd add to commit message something about performance issues
you are experiencing when trying to passthrough IGD manually
as one of justifications for workaround (it might push Xen folks
to investigate the issue and it won't be lost/forgotten on mail-list).

Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Tue, 17 Jan 2023 09:43:52 -0500
Chuck Zmudzinski  wrote:

> On 1/17/2023 5:35 AM, Igor Mammedov wrote:
> > On Mon, 16 Jan 2023 13:00:53 -0500
> > Chuck Zmudzinski  wrote:
> >  
> > > On 1/16/23 10:33, Igor Mammedov wrote:  
> > > > On Fri, 13 Jan 2023 16:31:26 -0500
> > > > Chuck Zmudzinski  wrote:
> > > > 
> > > >> On 1/13/23 4:33 AM, Igor Mammedov wrote:
> > > >> > On Thu, 12 Jan 2023 23:14:26 -0500
> > > >> > Chuck Zmudzinski  wrote:
> > > >> >   
> > > >> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:  
> > > >> >> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote: 
> > > >> >> >
> > > >> >> >> I think the change Michael suggests is very minimalistic: Move 
> > > >> >> >> the if
> > > >> >> >> condition around xen_igd_reserve_slot() into the function itself 
> > > >> >> >> and
> > > >> >> >> always call it there unconditionally -- basically turning three 
> > > >> >> >> lines
> > > >> >> >> into one. Since xen_igd_reserve_slot() seems very problem 
> > > >> >> >> specific,
> > > >> >> >> Michael further suggests to rename it to something more general. 
> > > >> >> >> All
> > > >> >> >> in all no big changes required.
> > > >> >> > 
> > > >> >> > yes, exactly.
> > > >> >> > 
> > > >> >> 
> > > >> >> OK, got it. I can do that along with the other suggestions.  
> > > >> > 
> > > >> > have you considered instead of reservation, putting a slot check in 
> > > >> > device model
> > > >> > and if it's intel igd being passed through, fail at realize time  if 
> > > >> > it can't take
> > > >> > required slot (with a error directing user to fix command line)? 
> > > >> >  
> > > >> 
> > > >> Yes, but the core pci code currently already fails at realize time
> > > >> with a useful error message if the user tries to use slot 2 for the
> > > >> igd, because of the xen platform device which has slot 2. The user
> > > >> can fix this without patching qemu, but having the user fix it on
> > > >> the command line is not the best way to solve the problem, primarily
> > > >> because the user would need to hotplug the xen platform device via a
> > > >> command line option instead of having the xen platform device added by
> > > >> pc_xen_hvm_init functions almost immediately after creating the pci
> > > >> bus, and that delay in adding the xen platform device degrades
> > > >> startup performance of the guest.
> > > >> 
> > > >> > That could be less complicated than dealing with slot reservations 
> > > >> > at the cost of
> > > >> > being less convenient.  
> > > >> 
> > > >> And also a cost of reduced startup performance
> > > > 
> > > > Could you clarify how it affects performance (and how much).
> > > > (as I see, setup done at board_init time is roughly the same
> > > > as with '-device foo' CLI options, modulo time needed to parse
> > > > options which should be negligible. and both ways are done before
> > > > guest runs)
> > > 
> > > I preface my answer by saying there is a v9, but you don't
> > > need to look at that. I will answer all your questions here.
> > > 
> > > I am going by what I observe on the main HDMI display with the
> > > different approaches. With the approach of not patching Qemu
> > > to fix this, which requires adding the Xen platform device a
> > > little later, the length of time it takes to fully load the
> > > guest is increased. I also noticed with Linux guests that use
> > > the grub bootoader, the grub vga driver cannot display the
> > > grub boot menu at the native resolution of the display, which
> > > in the tested case is 1920x1080, when the Xen platform device
> > > is added via a command line option instead of by the
> > > pc_xen_hvm_init_pci fucntion in pc_piix.c, but with this patch
> > > to Qemu, the grub menu is displayed at the full, 1920x1080
> > > native resolution of the display. Once the guest fully loads,
> > > there is no noticeable difference in performance. It is mainly
> > > a degradation in startup performance, not performance once
> > > the guest OS is fully loaded.  
> > above hints on presence of bug[s] in igd-passthru implementation,
> > and this patch effectively hides problem instead of trying to figure
> > out what's wrong and fixing it.
> >  
> 
> Why did you delete the rest of my answers to your other observations and
> not respond to them?

they are preserved on mail-list in you previous email.
It's usually fine to trim non relevant parts and keep only part/context
that is being replied to.

I didn't want to argue point that it's usually job of management
arrange correct IGD passthrough for QEMU like Alex pointed out.
Hence those part got trimmed.


> 

Re: [PATCH v2] x86/hotplug: Do not put offline vCPUs in mwait idle state

[Igor Mammedov]

On Fri, 20 Jan 2023 05:55:11 -0800
"Srivatsa S. Bhat"  wrote:

> Hi Igor and Thomas,
> 
> Thank you for your review!
> 
> On 1/19/23 1:12 PM, Thomas Gleixner wrote:
> > On Mon, Jan 16 2023 at 15:55, Igor Mammedov wrote:  
> >> "Srivatsa S. Bhat"  wrote:  
> >>> Fix this by preventing the use of mwait idle state in the vCPU offline
> >>> play_dead() path for any hypervisor, even if mwait support is
> >>> available.  
> >>
> >> if mwait is enabled, it's very likely guest to have cpuidle
> >> enabled and using the same mwait as well. So exiting early from
> >>  mwait_play_dead(), might just punt workflow down:
> >>   native_play_dead()
> >> ...
> >> mwait_play_dead();
> >> if (cpuidle_play_dead())   <- possible mwait here  
> >> 
> >> hlt_play_dead(); 
> >>
> >> and it will end up in mwait again and only if that fails
> >> it will go HLT route and maybe transition to VMM.  
> > 
> > Good point.
> >   
> >> Instead of workaround on guest side,
> >> shouldn't hypervisor force VMEXIT on being uplugged vCPU when it's
> >> actually hot-unplugging vCPU? (ex: QEMU kicks vCPU out from guest
> >> context when it is removing vCPU, among other things)  
> > 
> > For a pure guest side CPU unplug operation:
> > 
> > guest$ echo 0 >/sys/devices/system/cpu/cpu$N/online
> > 
> > the hypervisor is not involved at all. The vCPU is not removed in that
> > case.
> >   
> 
> Agreed, and this is indeed the scenario I was targeting with this patch,
> as opposed to vCPU removal from the host side. I'll add this clarification
> to the commit message.

commit message explicitly said:
"which prevents the hypervisor from running other vCPUs or workloads on the
corresponding pCPU."

and that implies unplug on hypervisor side as well.
Why? That's because when hypervisor exposes mwait to guest, it has to 
reserve/pin
a pCPU for each of present vCPUs. And you can safely run other VMs/workloads
on that pCPU only after it's not possible for it to be reused by VM where
it was used originally.

Now consider following worst (and most likely) case without unplug
on hypervisor side:

 1. vm1mwait: pin pCPU2 to vCPU2
 2. vm1mwait: guest$ echo 0 >/sys/devices/system/cpu/cpu2/online
-> HLT -> VMEXIT
 --
 3. vm2mwait: pin pCPU2 to vCPUx and start VM
 4. vm2mwait: guest OS onlines Vcpu and starts using it incl.
   going into idle=>mwait state
 --
 5. vm1mwait: it still thinks that vCPU is present it can rightfully do:
   guest$ echo 1 >/sys/devices/system/cpu/cpu2/online
 --  
 6.1 best case vm1mwait online fails after timeout
 6.2 worse case: vm2mwait does VMEXIT on vCPUx around time-frame when
 vm1mwait onlines vCPU2, the online may succeed and then vm2mwait's
 vCPUx will be stuck (possibly indefinitely) until for some reason
 VMEXIT happens on vm1mwait's vCPU2 _and_ host decides to schedule
 vCPUx on pCPU2 which would make vm1mwait stuck on vCPU2.
So either way it's expected behavior.

And if there is no intention to unplug vCPU on hypervisor side,
then VMEXIT on play_dead is not really necessary (mwait is better
then HLT), since hypervisor can't safely reuse pCPU elsewhere and
VCPU goes into deep sleep within guest context.

PS:
The only case where making HLT/VMEXIT on play_dead might work out,
would be if new workload weren't pinned to the same pCPU nor
used mwait (i.e. host can migrate it elsewhere and schedule
vCPU2 back on pCPU2).


> > So to ensure that this ends up in HLT something like the below is
> > required.
> > 
> > Note, the removal of the comment after mwait_play_dead() is intentional
> > because the comment is completely bogus. Not having MWAIT is not a
> > failure. But that wants to be a seperate patch.
> >   
> 
> Sounds good, will do and post a new version.
> 
> Thank you!
> 
> Regards,
> Srivatsa
> VMware Photon OS
> 
> 
> > Thanks,
> > 
> > tglx
> > ---
> > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > index 55cad72715d9..3f1f20f71ec5 100644
> > --- a/arch/x86/kernel/smpboot.c
> > +++ b/arch/x86/kernel/smpboot.c
> > @@ -1833,7 +1833,10 @@ void native_play_dead(void)
> > play_dead_common();
> > tboot_shutdown(TB_SHUTDOWN_WFS);
> >  
> > -   mwait_play_dead();  /* Only returns on failure */
> > +   if (this_cpu_has(X86_FEATURE_HYPERVISOR))
> > +   hlt_play_dead();
> > +
> > +   mwait_play_dead();
> > if (cpuidle_play_dead())
> > hlt_play_dead();
> >  }
> > 
> > 
> >   
> >   
> 

Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Mon, 16 Jan 2023 13:00:53 -0500
Chuck Zmudzinski  wrote:

> On 1/16/23 10:33, Igor Mammedov wrote:
> > On Fri, 13 Jan 2023 16:31:26 -0500
> > Chuck Zmudzinski  wrote:
> >   
> >> On 1/13/23 4:33 AM, Igor Mammedov wrote:  
> >> > On Thu, 12 Jan 2023 23:14:26 -0500
> >> > Chuck Zmudzinski  wrote:
> >> > 
> >> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:
> >> >> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote: 
> >> >> >  
> >> >> >> I think the change Michael suggests is very minimalistic: Move the if
> >> >> >> condition around xen_igd_reserve_slot() into the function itself and
> >> >> >> always call it there unconditionally -- basically turning three lines
> >> >> >> into one. Since xen_igd_reserve_slot() seems very problem specific,
> >> >> >> Michael further suggests to rename it to something more general. All
> >> >> >> in all no big changes required.  
> >> >> > 
> >> >> > yes, exactly.
> >> >> >   
> >> >> 
> >> >> OK, got it. I can do that along with the other suggestions.
> >> > 
> >> > have you considered instead of reservation, putting a slot check in 
> >> > device model
> >> > and if it's intel igd being passed through, fail at realize time  if it 
> >> > can't take
> >> > required slot (with a error directing user to fix command line)?
> >> 
> >> Yes, but the core pci code currently already fails at realize time
> >> with a useful error message if the user tries to use slot 2 for the
> >> igd, because of the xen platform device which has slot 2. The user
> >> can fix this without patching qemu, but having the user fix it on
> >> the command line is not the best way to solve the problem, primarily
> >> because the user would need to hotplug the xen platform device via a
> >> command line option instead of having the xen platform device added by
> >> pc_xen_hvm_init functions almost immediately after creating the pci
> >> bus, and that delay in adding the xen platform device degrades
> >> startup performance of the guest.
> >>   
> >> > That could be less complicated than dealing with slot reservations at 
> >> > the cost of
> >> > being less convenient.
> >> 
> >> And also a cost of reduced startup performance  
> > 
> > Could you clarify how it affects performance (and how much).
> > (as I see, setup done at board_init time is roughly the same
> > as with '-device foo' CLI options, modulo time needed to parse
> > options which should be negligible. and both ways are done before
> > guest runs)  
> 
> I preface my answer by saying there is a v9, but you don't
> need to look at that. I will answer all your questions here.
> 
> I am going by what I observe on the main HDMI display with the
> different approaches. With the approach of not patching Qemu
> to fix this, which requires adding the Xen platform device a
> little later, the length of time it takes to fully load the
> guest is increased. I also noticed with Linux guests that use
> the grub bootoader, the grub vga driver cannot display the
> grub boot menu at the native resolution of the display, which
> in the tested case is 1920x1080, when the Xen platform device
> is added via a command line option instead of by the
> pc_xen_hvm_init_pci fucntion in pc_piix.c, but with this patch
> to Qemu, the grub menu is displayed at the full, 1920x1080
> native resolution of the display. Once the guest fully loads,
> there is no noticeable difference in performance. It is mainly
> a degradation in startup performance, not performance once
> the guest OS is fully loaded.

Looking at igd-assign.txt, it recommends to add IGD using '-device' CLI
option, and actually drop at least graphics defaults explicitly.
So it is expected to work fine even when IGD is constructed with
'-device'.

Could you provide full CLI current xen starts QEMU with and then
a CLI you used (with explicit -device for IGD) that leads
to reduced performance?

CCing vfio folks who might have an idea what could be wrong based
on vfio experience.

Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Mon, 16 Jan 2023 13:00:53 -0500
Chuck Zmudzinski  wrote:

> On 1/16/23 10:33, Igor Mammedov wrote:
> > On Fri, 13 Jan 2023 16:31:26 -0500
> > Chuck Zmudzinski  wrote:
> >   
> >> On 1/13/23 4:33 AM, Igor Mammedov wrote:  
> >> > On Thu, 12 Jan 2023 23:14:26 -0500
> >> > Chuck Zmudzinski  wrote:
> >> > 
> >> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:
> >> >> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote: 
> >> >> >  
> >> >> >> I think the change Michael suggests is very minimalistic: Move the if
> >> >> >> condition around xen_igd_reserve_slot() into the function itself and
> >> >> >> always call it there unconditionally -- basically turning three lines
> >> >> >> into one. Since xen_igd_reserve_slot() seems very problem specific,
> >> >> >> Michael further suggests to rename it to something more general. All
> >> >> >> in all no big changes required.  
> >> >> > 
> >> >> > yes, exactly.
> >> >> >   
> >> >> 
> >> >> OK, got it. I can do that along with the other suggestions.
> >> > 
> >> > have you considered instead of reservation, putting a slot check in 
> >> > device model
> >> > and if it's intel igd being passed through, fail at realize time  if it 
> >> > can't take
> >> > required slot (with a error directing user to fix command line)?
> >> 
> >> Yes, but the core pci code currently already fails at realize time
> >> with a useful error message if the user tries to use slot 2 for the
> >> igd, because of the xen platform device which has slot 2. The user
> >> can fix this without patching qemu, but having the user fix it on
> >> the command line is not the best way to solve the problem, primarily
> >> because the user would need to hotplug the xen platform device via a
> >> command line option instead of having the xen platform device added by
> >> pc_xen_hvm_init functions almost immediately after creating the pci
> >> bus, and that delay in adding the xen platform device degrades
> >> startup performance of the guest.
> >>   
> >> > That could be less complicated than dealing with slot reservations at 
> >> > the cost of
> >> > being less convenient.
> >> 
> >> And also a cost of reduced startup performance  
> > 
> > Could you clarify how it affects performance (and how much).
> > (as I see, setup done at board_init time is roughly the same
> > as with '-device foo' CLI options, modulo time needed to parse
> > options which should be negligible. and both ways are done before
> > guest runs)  
> 
> I preface my answer by saying there is a v9, but you don't
> need to look at that. I will answer all your questions here.
> 
> I am going by what I observe on the main HDMI display with the
> different approaches. With the approach of not patching Qemu
> to fix this, which requires adding the Xen platform device a
> little later, the length of time it takes to fully load the
> guest is increased. I also noticed with Linux guests that use
> the grub bootoader, the grub vga driver cannot display the
> grub boot menu at the native resolution of the display, which
> in the tested case is 1920x1080, when the Xen platform device
> is added via a command line option instead of by the
> pc_xen_hvm_init_pci fucntion in pc_piix.c, but with this patch
> to Qemu, the grub menu is displayed at the full, 1920x1080
> native resolution of the display. Once the guest fully loads,
> there is no noticeable difference in performance. It is mainly
> a degradation in startup performance, not performance once
> the guest OS is fully loaded.
above hints on presence of bug[s] in igd-passthru implementation,
and this patch effectively hides problem instead of trying to figure
out what's wrong and fixing it.

Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Fri, 13 Jan 2023 16:31:26 -0500
Chuck Zmudzinski  wrote:

> On 1/13/23 4:33 AM, Igor Mammedov wrote:
> > On Thu, 12 Jan 2023 23:14:26 -0500
> > Chuck Zmudzinski  wrote:
> >   
> >> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:  
> >> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote:
> >> >> I think the change Michael suggests is very minimalistic: Move the if
> >> >> condition around xen_igd_reserve_slot() into the function itself and
> >> >> always call it there unconditionally -- basically turning three lines
> >> >> into one. Since xen_igd_reserve_slot() seems very problem specific,
> >> >> Michael further suggests to rename it to something more general. All
> >> >> in all no big changes required.
> >> > 
> >> > yes, exactly.
> >> > 
> >> 
> >> OK, got it. I can do that along with the other suggestions.  
> > 
> > have you considered instead of reservation, putting a slot check in device 
> > model
> > and if it's intel igd being passed through, fail at realize time  if it 
> > can't take
> > required slot (with a error directing user to fix command line)?  
> 
> Yes, but the core pci code currently already fails at realize time
> with a useful error message if the user tries to use slot 2 for the
> igd, because of the xen platform device which has slot 2. The user
> can fix this without patching qemu, but having the user fix it on
> the command line is not the best way to solve the problem, primarily
> because the user would need to hotplug the xen platform device via a
> command line option instead of having the xen platform device added by
> pc_xen_hvm_init functions almost immediately after creating the pci
> bus, and that delay in adding the xen platform device degrades
> startup performance of the guest.
> 
> > That could be less complicated than dealing with slot reservations at the 
> > cost of
> > being less convenient.  
> 
> And also a cost of reduced startup performance

Could you clarify how it affects performance (and how much).
(as I see, setup done at board_init time is roughly the same
as with '-device foo' CLI options, modulo time needed to parse
options which should be negligible. and both ways are done before
guest runs)

> However, the performance hit can be prevented by assigning slot
> 3 instead of slot 2 for the xen platform device if igd passthrough
> is configured on the command line instead of doing slot reservation,
> but there would still be less convenience and, for libxl users, an
> inability to easily configure the command line so that the igd can
> still have slot 2 without a hacky and error-prone patch to libxl to
> deal with this problem.
libvirt manages to get it right on management side without quirks on
QEMU side.

> I did post a patch on xen-devel to fix this using libxl, but so far
> it has not yet been reviewed and I mentioned in that patch that the
> approach of patching qemu so qemu reserves slot 2 for the igd is less
> prone to coding errors and is easier to maintain than the patch that
> would be required to implement the fix in libxl.

the patch is not trivial, and adds maintenance on QEMU.
Though I don't object to it as long as it's constrained to xen only
code and doesn't spill into generic PCI.
All I wanted is just point out there are other approach to problem
(i.e. do force user to user to provide correct configuration instead
of adding quirks whenever it's possible).

Re: [PATCH v2] x86/hotplug: Do not put offline vCPUs in mwait idle state

[Igor Mammedov]

On Sun, 15 Jan 2023 22:01:34 -0800
"Srivatsa S. Bhat"  wrote:

> From: "Srivatsa S. Bhat (VMware)" 
> 
> Under hypervisors that support mwait passthrough, a vCPU in mwait
> CPU-idle state remains in guest context (instead of yielding to the
> hypervisor via VMEXIT), which helps speed up wakeups from idle.
> 
> However, this runs into problems with CPU hotplug, because the Linux
> CPU offline path prefers to put the vCPU-to-be-offlined in mwait
> state, whenever mwait is available. As a result, since a vCPU in mwait
> remains in guest context and does not yield to the hypervisor, an
> offline vCPU *appears* to be 100% busy as viewed from the host, which
> prevents the hypervisor from running other vCPUs or workloads on the
> corresponding pCPU. [ Note that such a vCPU is not actually busy
> spinning though; it remains in mwait idle state in the guest ].
>
> Fix this by preventing the use of mwait idle state in the vCPU offline
> play_dead() path for any hypervisor, even if mwait support is
> available.

if mwait is enabled, it's very likely guest to have cpuidle
enabled and using the same mwait as well. So exiting early from
 mwait_play_dead(), might just punt workflow down:
  native_play_dead()
...
mwait_play_dead();
if (cpuidle_play_dead())   <- possible mwait here   
   
hlt_play_dead(); 

and it will end up in mwait again and only if that fails
it will go HLT route and maybe transition to VMM.

Instead of workaround on guest side,
shouldn't hypervisor force VMEXIT on being uplugged vCPU when it's
actually hot-unplugging vCPU? (ex: QEMU kicks vCPU out from guest
context when it is removing vCPU, among other things)

> Suggested-by: Peter Zijlstra (Intel) 
> Signed-off-by: Srivatsa S. Bhat (VMware) 
> Cc: Thomas Gleixner 
> Cc: Peter Zijlstra 
> Cc: Ingo Molnar 
> Cc: Borislav Petkov 
> Cc: Dave Hansen 
> Cc: "H. Peter Anvin" 
> Cc: "Rafael J. Wysocki" 
> Cc: "Paul E. McKenney" 
> Cc: Wyes Karny 
> Cc: Lewis Caroll 
> Cc: Tom Lendacky 
> Cc: Alexey Makhalov 
> Cc: Juergen Gross 
> Cc: x...@kernel.org
> Cc: VMware PV-Drivers Reviewers 
> Cc: virtualizat...@lists.linux-foundation.org
> Cc: k...@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org
> ---
> 
> v1: 
> https://lore.kernel.org/lkml/165843627080.142207.12667479241667142176.st...@csail.mit.edu/
> 
>  arch/x86/kernel/smpboot.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 55cad72715d9..125a5d4bfded 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1763,6 +1763,15 @@ static inline void mwait_play_dead(void)
>   return;
>   if (!this_cpu_has(X86_FEATURE_CLFLUSH))
>   return;
> +
> + /*
> +  * Do not use mwait in CPU offline play_dead if running under
> +  * any hypervisor, to make sure that the offline vCPU actually
> +  * yields to the hypervisor (which may not happen otherwise if
> +  * the hypervisor supports mwait passthrough).
> +  */
> + if (this_cpu_has(X86_FEATURE_HYPERVISOR))
> + return;
>   if (__this_cpu_read(cpu_info.cpuid_level) < CPUID_MWAIT_LEAF)
>   return;
>  

Re: [PATCH v8] xen/pt: reserve PCI slot 2 for Intel igd-passthru

[Igor Mammedov]

On Thu, 12 Jan 2023 23:14:26 -0500
Chuck Zmudzinski  wrote:

> On 1/12/23 6:03 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 12, 2023 at 10:55:25PM +, Bernhard Beschow wrote:  
> >> I think the change Michael suggests is very minimalistic: Move the if
> >> condition around xen_igd_reserve_slot() into the function itself and
> >> always call it there unconditionally -- basically turning three lines
> >> into one. Since xen_igd_reserve_slot() seems very problem specific,
> >> Michael further suggests to rename it to something more general. All
> >> in all no big changes required.  
> > 
> > yes, exactly.
> >   
> 
> OK, got it. I can do that along with the other suggestions.

have you considered instead of reservation, putting a slot check in device model
and if it's intel igd being passed through, fail at realize time  if it can't 
take
required slot (with a error directing user to fix command line)?
That could be less complicated than dealing with slot reservations at the cost 
of
being less convenient.

> 
> Thanks.
> 

Re: Question to mem-path support at QEMU for Xen

[Igor Mammedov]

On Thu, 28 Jul 2022 15:17:49 +0800
Huang Rui  wrote:

> Hi Igor,
> 
> Appreciate you for the reply!
> 
> On Wed, Jul 27, 2022 at 04:19:30PM +0800, Igor Mammedov wrote:
> > On Tue, 26 Jul 2022 15:27:07 +0800
> > Huang Rui  wrote:
> >   
> > > Hi Anthony and other Qemu/Xen guys,
> > > 
> > > We are trying to enable venus on Xen virtualization platform. And we would
> > > like to use the backend memory with memory-backend-memfd,id=mem1,size=4G
> > > options on QEMU, however, the QEMU will tell us the "-mem-path" is not
> > > supported with Xen. I verified the same function on KVM.
> > > 
> > > qemu-system-i386: -mem-path not supported with Xen
> > > 
> > > So may I know whether Xen has any limitation that support
> > > memory-backend-memfd in QEMU or just implementation is not done yet?  
> > 
> > Currently Xen doesn't use guest RAM allocation the way the rest of
> > accelerators do. (it has hacks in memory_region/ramblock API,
> > that divert RAM allocation calls to Xen specific API)  
> 
> I am new for Xen and QEMU, we are working on GPU. We would like to have a
> piece of backend memroy like video memory for VirtIO GPU to support guest
> VM Mesa Vulkan (Venus). Do you mean we can the memory_region/ramblock APIs
> to work around it?
> 
> > 
> > The sane way would extend Xen to accept RAM regions (whatever they are
> > ram or fd based) QEMU allocates instead of going its own way. This way
> > it could reuse all memory backends that QEMU provides for the rest of
> > the non-Xen world. (not to mention that we could drop non trivial
> > Xen hacks so that guest RAM handling would be consistent with other
> > accelerators)
> >   
> 
> May I know what do you mean by "going its own way"? This sounds good, could
> you please elaborate on how can we implement this? We would like to give a
> try to address the problem on Xen. Would you mind to point somewhere that I
> can learn and understand the RAM region. Very happy to see your
> suggestions!

see for example see ram_block_add(), if Xen could be persuaded to use memory
allocated by '!xen_enabled()' branch then it's likely file base backends
would also become usable.

Whether it is possible for Xen or not I don't know,
I guess CCed Xen folks will suggest something useful.
 
> Thanks & Best Regards,
> Ray
> 

Re: Question to mem-path support at QEMU for Xen

[Igor Mammedov]

On Tue, 26 Jul 2022 15:27:07 +0800
Huang Rui  wrote:

> Hi Anthony and other Qemu/Xen guys,
> 
> We are trying to enable venus on Xen virtualization platform. And we would
> like to use the backend memory with memory-backend-memfd,id=mem1,size=4G
> options on QEMU, however, the QEMU will tell us the "-mem-path" is not
> supported with Xen. I verified the same function on KVM.
> 
> qemu-system-i386: -mem-path not supported with Xen
> 
> So may I know whether Xen has any limitation that support
> memory-backend-memfd in QEMU or just implementation is not done yet?

Currently Xen doesn't use guest RAM allocation the way the rest of
accelerators do. (it has hacks in memory_region/ramblock API,
that divert RAM allocation calls to Xen specific API)

The sane way would extend Xen to accept RAM regions (whatever they are
ram or fd based) QEMU allocates instead of going its own way. This way
it could reuse all memory backends that QEMU provides for the rest of
the non-Xen world. (not to mention that we could drop non trivial
Xen hacks so that guest RAM handling would be consistent with other
accelerators)

> Looking forward to your reply!
> 
> Thanks a lot,
> Ray
> 

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 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-devel@lists.xenproject.org
> Cc: qemu-bl...@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-devel@lists.xenproject.org
> Cc: qemu-bl...@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-bl...@nongnu.org
> Cc: xen-devel@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: [Xen-devel] [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(-)
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v5 07/19] hw: apply accel compat properties without touching globals

[Igor Mammedov]

On Wed, 12 Dec 2018 16:00:06 +0400
Marc-André Lureau  wrote:

> Hi
> On Mon, Dec 10, 2018 at 8:55 PM Igor Mammedov  wrote:
> >
> > On Mon, 10 Dec 2018 17:45:22 +0100
> > Igor Mammedov  wrote:
> >  
> > > On Tue,  4 Dec 2018 18:20:11 +0400
> > > Marc-André Lureau  wrote:
> > >  
> > > > Instead of registering compat properties as globals, let's keep them
> > > > in their own array, to avoid mixing with user globals.
> > > >
> > > > Introduce object_apply_global_props() function, to apply compatibility
> > > > properties from a GPtrArray.
> > > >
> > > > Signed-off-by: Marc-André Lureau 
> > > > ---
> > > >  include/hw/qdev-core.h | 10 ++
> > > >  include/qom/object.h   |  3 +++
> > > >  include/sysemu/accel.h |  4 +---
> > > >  accel/accel.c  | 12 
> > > >  hw/core/qdev.c |  9 +
> > > >  hw/xen/xen-common.c|  9 ++---
> > > >  qom/object.c   | 25 +
> > > >  vl.c   |  1 -
> > > >  8 files changed, 54 insertions(+), 19 deletions(-)
> > > >  
> > > [...]  
> > > > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> > > > index 6ec14c73ca..4532aa8632 100644
> > > > --- a/hw/xen/xen-common.c
> > > > +++ b/hw/xen/xen-common.c
> > > > @@ -174,18 +174,21 @@ static GlobalProperty xen_compat_props[] = {
> > > >  .driver = "migration",
> > > >  .property = "send-section-footer",
> > > >  .value = "off",
> > > > -},
> > > > -{ /* end of list */ },
> > > > +}
> > > >  };
> > > >
> > > >  static void xen_accel_class_init(ObjectClass *oc, void *data)
> > > >  {
> > > >  AccelClass *ac = ACCEL_CLASS(oc);
> > > > +
> > > >  ac->name = "Xen";
> > > >  ac->init_machine = xen_init;
> > > >  ac->setup_post = xen_setup_post;
> > > >  ac->allowed = _allowed;
> > > > -ac->global_props = xen_compat_props;
> > > > +ac->compat_props = g_ptr_array_new();  
> > > where is matching free for that?  
> > can we at least annotate it somehow so that valgrind won't complain about 
> > this leak?  
> 
> If you check my commits on qemu, you should see that I do care (too
> much?) about leaks :)
> 
> In this case though, I don't see valgrind or asan complaining, I guess
> it's still a reachable reference.
> Do you think a FIXME comment would be helpful?
I've looked at other cases were we leak, and well we leak a lot so it's
probably futile exercise. But the comment won't hurt and will work as remainder.


> (/me wish we had a proper object system, GObject, but that ship as
> long sailed..)
> 
> >  
> > >  
> > > > +
> > > > +compat_props_add(ac->compat_props,
> > > > + xen_compat_props, G_N_ELEMENTS(xen_compat_props));
> > > >  }
> > > >
> > > >  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> > > > diff --git a/qom/object.c b/qom/object.c
> > > > index 17921c0a71..dbdab0aead 100644
> > > > --- a/qom/object.c
> > > > +++ b/qom/object.c
> > > > @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object 
> > > > *obj, TypeImpl *ti)
> > > >  }
> > > >  }
> > > >
> > > > +void object_apply_global_props(Object *obj, const GPtrArray *props, 
> > > > Error **errp)
> > > > +{
> > > > +Error *err = NULL;
> > > > +int i;
> > > > +
> > > > +if (!props) {
> > > > +return;
> > > > +}
> > > > +
> > > > +for (i = 0; i < props->len; i++) {
> > > > +GlobalProperty *p = g_ptr_array_index(props, i);
> > > > +
> > > > +if (object_dynamic_cast(obj, p->driver) == NULL) {
> > > > +continue;
> > > > +}
> > > > +p->used = true;
> > > > +object_property_parse(obj, p->value, p->property, );
> > > > +if (err != NULL) {
> > > > +error_prepend(, "can't apply global %s.%s=%s: ",
> > > > +  p->driver, p->property, p->value);
> > > > +error_propagate(errp, err);
> > > > +}
> > > > +}
> > > > +}
> > > > +
> > > >  static void object_initialize_with_type(void *data, size_t size, 
> > > > TypeImpl *type)
> > > >  {
> > > >  Object *obj = data;
> > > > diff --git a/vl.c b/vl.c
> > > > index a5ae5f23d2..88ba658572 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -2968,7 +2968,6 @@ static void user_register_global_props(void)
> > > >   */
> > > >  static void register_global_properties(MachineState *ms)
> > > >  {
> > > > -accel_register_compat_props(ms->accelerator);
> > > >  machine_register_compat_props(ms);
> > > >  user_register_global_props();
> > > >  }  
> > >
> > >  
> >
> >  
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v5 07/19] hw: apply accel compat properties without touching globals

[Igor Mammedov]

On Tue,  4 Dec 2018 18:20:11 +0400
Marc-André Lureau  wrote:

> Instead of registering compat properties as globals, let's keep them
> in their own array, to avoid mixing with user globals.
> 
> Introduce object_apply_global_props() function, to apply compatibility
> properties from a GPtrArray.
> 
> Signed-off-by: Marc-André Lureau 
other than static leak looks fine, considering that we already
leak compat_props, it doesn't really matter, so:

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/qdev-core.h | 10 ++
>  include/qom/object.h   |  3 +++
>  include/sysemu/accel.h |  4 +---
>  accel/accel.c  | 12 
>  hw/core/qdev.c |  9 +
>  hw/xen/xen-common.c|  9 ++---
>  qom/object.c   | 25 +
>  vl.c   |  1 -
>  8 files changed, 54 insertions(+), 19 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566..aeaa6dbbb8 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -267,6 +267,16 @@ typedef struct GlobalProperty {
>  Error **errp;
>  } GlobalProperty;
>  
> +static inline void
> +compat_props_add(GPtrArray *arr,
> + GlobalProperty props[], size_t nelem)
> +{
> +int i;
> +for (i = 0; i < nelem; i++) {
> +g_ptr_array_add(arr, (void *)[i]);
> +}
> +}
> +
>  /*** Board API.  This should go away once we have a machine config file.  
> ***/
>  
>  DeviceState *qdev_create(BusState *bus, const char *name);
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 0139838b69..5183c587f3 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -676,6 +676,9 @@ Object *object_new_with_propv(const char *typename,
>Error **errp,
>va_list vargs);
>  
> +void object_apply_global_props(Object *obj, const GPtrArray *props,
> +   Error **errp);
> +
>  /**
>   * object_set_props:
>   * @obj: the object instance to set properties on
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 637358f430..f331d128e9 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -49,7 +49,7 @@ typedef struct AccelClass {
>   * global properties may be overridden by machine-type
>   * compat_props or user-provided global properties.
>   */
> -GlobalProperty *global_props;
> +GPtrArray *compat_props;
>  } AccelClass;
>  
>  #define TYPE_ACCEL "accel"
> @@ -67,8 +67,6 @@ typedef struct AccelClass {
>  extern unsigned long tcg_tb_size;
>  
>  void configure_accelerator(MachineState *ms);
> -/* Register accelerator specific global properties */
> -void accel_register_compat_props(AccelState *accel);
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
>  
> diff --git a/accel/accel.c b/accel/accel.c
> index 3da26eb90f..6db5d8f4df 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -119,18 +119,6 @@ void configure_accelerator(MachineState *ms)
>  }
>  }
>  
> -void accel_register_compat_props(AccelState *accel)
> -{
> -AccelClass *class = ACCEL_GET_CLASS(accel);
> -GlobalProperty *prop = class->global_props;
> -
> -for (; prop && prop->driver; prop++) {
> -/* Any compat_props must never cause error */
> -prop->errp = _abort;
> -qdev_prop_register_global(prop);
> -}
> -}
> -
>  void accel_setup_post(MachineState *ms)
>  {
>  AccelState *accel = ms->accelerator;
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 6b3cc55b27..53b507164f 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -972,6 +972,15 @@ static void device_initfn(Object *obj)
>  
>  static void device_post_init(Object *obj)
>  {
> +if (object_dynamic_cast(qdev_get_machine(), TYPE_MACHINE)) {
> +MachineState *m = MACHINE(qdev_get_machine());
> +AccelClass *ac = ACCEL_GET_CLASS(m->accelerator);
> +
> +if (ac->compat_props) {
> +object_apply_global_props(obj, ac->compat_props, _abort);
> +}
> +}
> +
>  qdev_prop_set_globals(DEVICE(obj));
>  }
>  
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 6ec14c73ca..4532aa8632 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -174,18 +174,21 @@ static GlobalProperty xen_compat_props[] = {
>  .driver = "migration",
>  .property = "send-section-footer",
>  .value = "off",
> -},
> -{ /* 

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v5 07/19] hw: apply accel compat properties without touching globals

[Igor Mammedov]

On Mon, 10 Dec 2018 17:45:22 +0100
Igor Mammedov  wrote:

> On Tue,  4 Dec 2018 18:20:11 +0400
> Marc-André Lureau  wrote:
> 
> > Instead of registering compat properties as globals, let's keep them
> > in their own array, to avoid mixing with user globals.
> > 
> > Introduce object_apply_global_props() function, to apply compatibility
> > properties from a GPtrArray.
> > 
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  include/hw/qdev-core.h | 10 ++
> >  include/qom/object.h   |  3 +++
> >  include/sysemu/accel.h |  4 +---
> >  accel/accel.c  | 12 
> >  hw/core/qdev.c |  9 +
> >  hw/xen/xen-common.c|  9 ++---
> >  qom/object.c   | 25 +
> >  vl.c   |  1 -
> >  8 files changed, 54 insertions(+), 19 deletions(-)
> > 
> [...]
> > diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> > index 6ec14c73ca..4532aa8632 100644
> > --- a/hw/xen/xen-common.c
> > +++ b/hw/xen/xen-common.c
> > @@ -174,18 +174,21 @@ static GlobalProperty xen_compat_props[] = {
> >  .driver = "migration",
> >  .property = "send-section-footer",
> >  .value = "off",
> > -},
> > -{ /* end of list */ },
> > +}
> >  };
> >  
> >  static void xen_accel_class_init(ObjectClass *oc, void *data)
> >  {
> >  AccelClass *ac = ACCEL_CLASS(oc);
> > +
> >  ac->name = "Xen";
> >  ac->init_machine = xen_init;
> >  ac->setup_post = xen_setup_post;
> >  ac->allowed = _allowed;
> > -ac->global_props = xen_compat_props;
> > +ac->compat_props = g_ptr_array_new();
> where is matching free for that?
can we at least annotate it somehow so that valgrind won't complain about this 
leak?

> 
> > +
> > +compat_props_add(ac->compat_props,
> > + xen_compat_props, G_N_ELEMENTS(xen_compat_props));
> >  }
> >  
> >  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> > diff --git a/qom/object.c b/qom/object.c
> > index 17921c0a71..dbdab0aead 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object *obj, 
> > TypeImpl *ti)
> >  }
> >  }
> >  
> > +void object_apply_global_props(Object *obj, const GPtrArray *props, Error 
> > **errp)
> > +{
> > +Error *err = NULL;
> > +int i;
> > +
> > +if (!props) {
> > +return;
> > +}
> > +
> > +for (i = 0; i < props->len; i++) {
> > +GlobalProperty *p = g_ptr_array_index(props, i);
> > +
> > +if (object_dynamic_cast(obj, p->driver) == NULL) {
> > +continue;
> > +}
> > +p->used = true;
> > +object_property_parse(obj, p->value, p->property, );
> > +if (err != NULL) {
> > +error_prepend(, "can't apply global %s.%s=%s: ",
> > +  p->driver, p->property, p->value);
> > +error_propagate(errp, err);
> > +}
> > +}
> > +}
> > +
> >  static void object_initialize_with_type(void *data, size_t size, TypeImpl 
> > *type)
> >  {
> >  Object *obj = data;
> > diff --git a/vl.c b/vl.c
> > index a5ae5f23d2..88ba658572 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2968,7 +2968,6 @@ static void user_register_global_props(void)
> >   */
> >  static void register_global_properties(MachineState *ms)
> >  {
> > -accel_register_compat_props(ms->accelerator);
> >  machine_register_compat_props(ms);
> >  user_register_global_props();
> >  }
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-3.2 v5 07/19] hw: apply accel compat properties without touching globals

[Igor Mammedov]

On Tue,  4 Dec 2018 18:20:11 +0400
Marc-André Lureau  wrote:

> Instead of registering compat properties as globals, let's keep them
> in their own array, to avoid mixing with user globals.
> 
> Introduce object_apply_global_props() function, to apply compatibility
> properties from a GPtrArray.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/qdev-core.h | 10 ++
>  include/qom/object.h   |  3 +++
>  include/sysemu/accel.h |  4 +---
>  accel/accel.c  | 12 
>  hw/core/qdev.c |  9 +
>  hw/xen/xen-common.c|  9 ++---
>  qom/object.c   | 25 +
>  vl.c   |  1 -
>  8 files changed, 54 insertions(+), 19 deletions(-)
> 
[...]
> diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
> index 6ec14c73ca..4532aa8632 100644
> --- a/hw/xen/xen-common.c
> +++ b/hw/xen/xen-common.c
> @@ -174,18 +174,21 @@ static GlobalProperty xen_compat_props[] = {
>  .driver = "migration",
>  .property = "send-section-footer",
>  .value = "off",
> -},
> -{ /* end of list */ },
> +}
>  };
>  
>  static void xen_accel_class_init(ObjectClass *oc, void *data)
>  {
>  AccelClass *ac = ACCEL_CLASS(oc);
> +
>  ac->name = "Xen";
>  ac->init_machine = xen_init;
>  ac->setup_post = xen_setup_post;
>  ac->allowed = _allowed;
> -ac->global_props = xen_compat_props;
> +ac->compat_props = g_ptr_array_new();
where is matching free for that?

> +
> +compat_props_add(ac->compat_props,
> + xen_compat_props, G_N_ELEMENTS(xen_compat_props));
>  }
>  
>  #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen")
> diff --git a/qom/object.c b/qom/object.c
> index 17921c0a71..dbdab0aead 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object *obj, 
> TypeImpl *ti)
>  }
>  }
>  
> +void object_apply_global_props(Object *obj, const GPtrArray *props, Error 
> **errp)
> +{
> +Error *err = NULL;
> +int i;
> +
> +if (!props) {
> +return;
> +}
> +
> +for (i = 0; i < props->len; i++) {
> +GlobalProperty *p = g_ptr_array_index(props, i);
> +
> +if (object_dynamic_cast(obj, p->driver) == NULL) {
> +continue;
> +}
> +p->used = true;
> +object_property_parse(obj, p->value, p->property, );
> +if (err != NULL) {
> +error_prepend(, "can't apply global %s.%s=%s: ",
> +  p->driver, p->property, p->value);
> +error_propagate(errp, err);
> +}
> +}
> +}
> +
>  static void object_initialize_with_type(void *data, size_t size, TypeImpl 
> *type)
>  {
>  Object *obj = data;
> diff --git a/vl.c b/vl.c
> index a5ae5f23d2..88ba658572 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2968,7 +2968,6 @@ static void user_register_global_props(void)
>   */
>  static void register_global_properties(MachineState *ms)
>  {
> -accel_register_compat_props(ms->accelerator);
>  machine_register_compat_props(ms);
>  user_register_global_props();
>  }


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-3.2 v4 15/28] hw: apply accel compat properties without touching globals

[Igor Mammedov]

On Tue, 27 Nov 2018 13:27:48 +0400
Marc-André Lureau  wrote:

> Introduce object_apply_global_props() function, to apply compatibility
> properties from a GPtrArray.
> 
> For accel compatibility properties, apply them during
> device_post_init(), after accel_register_compat_props() has set them.
> 
> To populate the compatibility properties, introduce SET_COMPAT(), a
> more generic version of SET_MACHINE_COMPAT() that can set compat
> properties on other objects than Machine, and using GPtrArray.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/qdev-core.h | 13 +
>  include/qom/object.h   |  3 +++
>  include/sysemu/accel.h |  4 +---
>  accel/accel.c  | 12 
>  hw/core/qdev.c | 11 +++
>  hw/xen/xen-common.c| 38 +++---
>  qom/object.c   | 25 +
>  vl.c   |  2 +-
>  8 files changed, 73 insertions(+), 35 deletions(-)
> 
[...]

> diff --git a/vl.c b/vl.c
> index fa25d1ae2d..c06e94271c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2963,7 +2963,7 @@ static void user_register_global_props(void)
>   */
>  static void register_global_properties(MachineState *ms)
>  {
> -accel_register_compat_props(ms->accelerator);
> +
> accel_register_compat_props(ACCEL_GET_CLASS(ms->accelerator)->compat_props);
   ^^
Acceptable way to do above is:
 Foo *foo =CAST(something)
 FUNC(foo->member)
if I recall correctly

>  machine_register_compat_props(ms);
>  user_register_global_props();
>  }


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 10/14] qdev-props: call object_apply_global_props()

[Igor Mammedov]

On Tue, 27 Nov 2018 00:02:35 +0400
Marc-André Lureau  wrote:

> Hi
> On Mon, Nov 26, 2018 at 5:27 PM Igor Mammedov  wrote:
> >
> > On Wed,  7 Nov 2018 16:36:48 +0400
> > Marc-André Lureau  wrote:
> >  
> > > It's now possible to use the common function.
> > >
> > > Teach object_apply_global_props() to warn if Error argument is NULL.
> > >
> > > Signed-off-by: Marc-André Lureau 
> > > ---
> > >  hw/core/qdev-properties.c | 24 ++--
> > >  qom/object.c  |  6 +-
> > >  2 files changed, 7 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> > > index 8728cbab9f..239535a4cb 100644
> > > --- a/hw/core/qdev-properties.c
> > > +++ b/hw/core/qdev-properties.c
> > > @@ -1223,28 +1223,8 @@ int qdev_prop_check_globals(void)
> > >
> > >  void qdev_prop_set_globals(DeviceState *dev)
> > >  {
> > > -int i;
> > > -
> > > -for (i = 0; i < global_props()->len; i++) {
> > > -GlobalProperty *prop;
> > > -Error *err = NULL;
> > > -
> > > -prop = g_array_index(global_props(), GlobalProperty *, i);
> > > -if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> > > -continue;
> > > -}
> > > -prop->used = true;
> > > -object_property_parse(OBJECT(dev), prop->value, prop->property, 
> > > );
> > > -if (err != NULL) {
> > > -error_prepend(, "can't apply global %s.%s=%s: ",
> > > -  prop->driver, prop->property, prop->value);
> > > -if (!dev->hotplugged) {
> > > -error_propagate(_fatal, err);
> > > -} else {
> > > -warn_report_err(err);
> > > -}
> > > -}
> > > -}
> > > +object_apply_global_props(OBJECT(dev), global_props(),
> > > +  dev->hotplugged ? NULL : _fatal);  
> > arguably, it's up to caller to decide it warn or not.
> > I'd leave it warning code out of object_apply_global_props() and let caller 
> > do the job  
> 
> The problem is that there may be multiple errors, and the remaining
> globals should be applied.
>
> I'll add a comment.
ok

 
> > >  }
> > >
> > >  /* --- 64bit unsigned int 'size' type --- */
> > > diff --git a/qom/object.c b/qom/object.c
> > > index 9acdf9e16d..b1a7f70550 100644
> > > --- a/qom/object.c
> > > +++ b/qom/object.c
> > > @@ -392,7 +392,11 @@ void object_apply_global_props(Object *obj, GArray 
> > > *props, Error **errp)
> > >  if (err != NULL) {
> > >  error_prepend(, "can't apply global %s.%s=%s: ",
> > >p->driver, p->property, p->value);
> > > -error_propagate(errp, err);
> > > +if (errp) {
> > > +error_propagate(errp, err);
> > > +} else {
> > > +warn_report_err(err);
> > > +}
> > >  }
> > >  }
> > >  }  
> >
> >  
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 20/24] hw: acpi: Define ACPI tables builder interface

[Igor Mammedov]

On Thu, 22 Nov 2018 00:57:21 +0100
Samuel Ortiz  wrote:

> On Fri, Nov 16, 2018 at 05:02:26PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:43 +0100
> > Samuel Ortiz  wrote:
> >   
> > > In order to decouple ACPI APIs from specific machine types, we are
> > > creating an ACPI builder interface that each ACPI platform can choose to
> > > implement.
> > > This way, a new machine type can re-use the high level ACPI APIs and
> > > define some custom table build methods, without having to duplicate most
> > > of the existing implementation only to add small variations to it.  
> > I'm not sure about motivation behind so high APIs,
> > what obvious here is an extra level of indirection for not clear gain.
> > 
> > Yep using table callbacks, one can attempt to generalize
> > acpi_setup() and help boards to decide which tables do not build
> > (MCFG comes to the mind). But I'm not convinced that acpi_setup()
> > could be cleanly generalized as a whole (probably some parts but
> > not everything)  
> It's more about generalizing acpi_build(), and then having one
> acpi_setup for non hardware-reduced ACPI and a acpi_reduced_setup() for
> hardware-reduced.
> 
> Right now there's no generalization at all but with this patch we could
> already use the same acpi_reduced_setup() implementation for both arm
> and i386/virt.
> 
> > so it's minor benefit for extra headache of
> > figuring out what callback will be actually called when reading code.  
> This is the same complexity that already exists for essentially all
> current interfaces.
in case of callback vs plain function call, I'd choose the later
if it does the job and resort to the former when I have to.
 
> > However if board needs a slightly different table, it will have to
> > duplicate an exiting one and then modify to suit its needs.
> > 
> > to me it pretty much looks the same as calling build_foo()
> > we use now but with an extra indirection level and then
> > duplicating the later for usage in another board in slightly
> > different manner.  
> I believe what you're trying to say is that this abstraction may be
> useful but you're arguing the granularity is not properly defined? Am I
> getting this right?
yep, something along those lines. So far it's not useful much if at all.
So I'll not introduce it for now and try to get by with plain functions
calls. Later we might add fine-grained callbacks on case by case basis
(like 'adevc->madt_cpu') where it's possible to generalize.
 
> Cheers,
> Samuel.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 12/24] hw: acpi: Export the MCFG getter

[Igor Mammedov]

On Thu, 22 Nov 2018 00:21:06 +0100
Samuel Ortiz  wrote:

> Hi Igor,
> 
> On Thu, Nov 15, 2018 at 01:36:58PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:35 +0100
> > Samuel Ortiz  wrote:
> >   
> > > From: Yang Zhong 
> > > 
> > > The ACPI MCFG getter is not x86 specific and could be called from
> > > anywhere within generic ACPI API, so let's export it.  
> > So far it's x86 or more exactly q35 specific thing,  
> It's property based, and it's using a generic PCIE property afaict.
> So it's up to each machine type to define those properties.
> I'm curious here: What's the idiomatic way to define a machine
> setting/attribute/property, let each instance define it or not, and
> make it available at run time?
> Would you be getting the PCI host pointer from the ACPI build state and
> getting that information back from there?

Cleaner way would be make arm/virt board set PCIE_HOST_MCFG_BASE/
PCIE_HOST_MCFG_SIZE properties and then use common build_mcfg()(in aml-build.c).
Something like this:
  acpi_setup_reduced()
 AcpiMcfgInfo mcfg_info = {
   .base = object_property_get_uint(pcie, PCIE_HOST_MCFG_BASE, NULL),
   .size = object_property_get_uint(pcie, PCIE_HOST_MCFG_SIZE, NULL)
 };
 acpi_build() {
 build_mcfg("MCFG", );
 }
  }
and for legacy q35
  acpi_build() {
 if (pcie) {
AcpiMcfgInfo mcfg_info = {
  .base = object_property_get_uint(pcie, PCIE_HOST_MCFG_BASE, NULL),
  .size = object_property_get_uint(pcie, PCIE_HOST_MCFG_SIZE, NULL)
};
if (mcfg_info.base != PCIE_BASE_ADDR_UNMAPPED)
build_mcfg("MCFG", );
else
/* move comment here why we are doing it */
build_mcfg("QEMU", );
 }
  }

The thing I don't like about acpi_get_mcfg() is that it
does lookup acpi_get_i386_pci_host() each time it's called
and judges if it's PCI-E host by presence of properties.

I'd rather be explicit where PCI host be fetched once somewhere
in acpi_setup() or possibly passed down from the board as an argument
and board telling to i386/acpi_setup() if it's PCI or PCI-E host
so we don't have to guess it in acpi code.


> Cheers,
> Samuel.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 15/24] hw: i386: Export the i386 ACPI SRAT build method

[Igor Mammedov]

On Thu, 22 Nov 2018 00:27:33 +0100
Samuel Ortiz  wrote:

> On Thu, Nov 15, 2018 at 02:28:54PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:38 +0100
> > Samuel Ortiz  wrote:
> >   
> > > This is the standard way of building SRAT on x86 platfoms. But future
> > > machine types could decide to define their own custom SRAT build method
> > > through the ACPI builder methods.
> > > Moreover, we will also need to reach build_srat() from outside of
> > > acpi-build in order to use it as the ACPI builder SRAT build method.  
> > SRAT is usually highly machine specific (memory holes, layout, guest OS
> > specific quirks) so it's hard to generalize it.  
> Hence the need for an SRAT builder interface.
so far builder interface (trying to generalize acpi_build()) looks
not necessary.
I'd suggest to drop and call build_start() directly.

> > I'd  drop SRAT related patches from this series and introduce
> > i386/virt specific SRAT when you post patches for it.  
> virt uses the existing i386 build_srat() routine, there's nothing
> special about it. So this would be purely duplicated code.
Looking at build_srat(), it has a bunch of code to handle legacy
PC layout. You probably don't need any of it for new is86/virt
machine and can make simpler version of it.

In addition (probably repeating question I've asked elsewhere),
Do you have to use split initial memory model for new machine?
Is it possible to use only pc-dimms both for initial and hotplugged memory
at some address (4Gb?) without cutting out PCI hole or any toher holes in RAM 
layout?

> Cheers,
> Samuel.
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 00/14] Generalize machine compatibility properties

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:38 +0400
Marc-André Lureau  wrote:

> Hi,
> 
> During "[PATCH v2 05/10] qom/globals: generalize
> object_property_set_globals()" review, Eduardo suggested to rework the
> GlobalProperty handling, so that -global is limited to QDev only and
> we avoid mixing the machine compats and the user-provided -global
> properties (instead of generalizing -global to various object kinds,
> like I proposed in v2).
> 
> "qdev: do not mix compat props with global props" patch decouples a
> bit user-provided -global from machine compat properties. This allows
> to get rid of "user_provided" and "errp" fields in following patches.
> 
> Instead of explcitely calling object_apply_global_props() in the
> various object post_init, I opted for creating a new TYPE_COMPAT_PROPS
> interface. The interface approach gives a lot more flexibility on
> which objects can have compat props. This requires some interface
> improvments in "qom: teach interfaces to implement post-init".
> 
> A new compat property "x-use-canonical-path-for-ramblock-id" is added
> to hostmem for legacy canonical path names, set to true for -file and
> -memfd with qemu < 3.2.
> 
> (this series was initially titled "[PATCH v2 00/10] hostmem: use
> object "id" for memory region name with >= 3.1", but its focus is more
> in refactoring the global and compatilibity properties handling now)
That probably all feedback I'm able to give on this round of review,
so I'll wait till it will addressed.

> 
> v3:
> - GlobalProperties improvements/cleanups
> - drop generalizing the -global idea
> - "replace" the set_globals flag with a TYPE_COMPAT_PROPS interface
> - update hw/i386 machine version to 3.2
> - add "qom: make interface types abstract" interface cleanup
> 
> v2:
> - replace "qom/user-creatable: add a few helper macros" patch for a
>   more optimized "qom: make user_creatable_complete() specific to
>   UserCreatable"
> - rename register_global_list() to register_global_properties()
> - call object_property_set_globals() after post-init
> - add and use a ObjectClass.set_globals flag, instead of dynamically
>   check object class in object_property_set_globals()
> - use object "id" in >= 3.1 instead of canonical path, add compat
>   property "x-use-canonical-path-for-ramblock-id" in base hostmem
>   class.
> 
> Marc-André Lureau (14):
>   tests: qdev_prop_check_globals() doesn't return "all_used"
>   qom: make interface types abstract
>   qom: make user_creatable_complete() specific to UserCreatable
>   accel: register global_props like machine globals
>   qdev: move qdev_prop_register_global_list() to tests
>   qdev: do not mix compat props with global props
>   qdev: all globals are now user-provided
>   qdev-props: convert global_props to GArray
>   qdev-props: remove errp from GlobalProperty
>   qdev-props: call object_apply_global_props()
>   qom: teach interfaces to implement post-init
>   machine: add compat-props interface
>   hw/i386: add pc-i440fx-3.2 & pc-q35-3.2
>   hostmem: use object id for memory region name with >= 3.1
> 
>  include/hw/acpi/acpi_dev_interface.h |  6 +--
>  include/hw/arm/linux-boot-if.h   |  5 +-
>  include/hw/boards.h  |  3 +-
>  include/hw/compat.h  | 11 
>  include/hw/fw-path-provider.h|  4 +-
>  include/hw/hotplug.h |  6 +--
>  include/hw/i386/pc.h |  3 ++
>  include/hw/intc/intc.h   |  4 +-
>  include/hw/ipmi/ipmi.h   |  4 +-
>  include/hw/isa/isa.h |  4 --
>  include/hw/mem/memory-device.h   |  4 +-
>  include/hw/nmi.h |  4 +-
>  include/hw/qdev-core.h   |  9 
>  include/hw/qdev-properties.h | 30 ---
>  include/hw/stream.h  |  4 +-
>  include/hw/timer/m48t59.h|  4 +-
>  include/qom/object.h |  2 +
>  include/qom/object_interfaces.h  | 10 ++--
>  include/sysemu/accel.h   |  4 +-
>  include/sysemu/hostmem.h |  3 +-
>  include/sysemu/tpm.h |  4 +-
>  target/arm/idau.h|  4 +-
>  accel/accel.c|  7 +--
>  backends/hostmem-file.c  |  8 +--
>  backends/hostmem-memfd.c |  2 +-
>  backends/hostmem-ram.c   |  9 ++--
>  backends/hostmem.c   | 31 +++
>  hw/core/compat-props.c   | 43 +++
>  hw/core/machine.c| 18 ---
>  hw/core/qdev-properties.c| 73 ++---
>  hw/core/qdev.c   |  4 ++
>  hw/i386/pc_piix.c| 21 ++--
>  hw/i386/pc_q35.c | 19 ++-
>  hw/misc/ivshmem.c|  2 +-
>  hw/virtio/virtio-rng.c   |  2 +-
>  hw/xen/xen-common.c  |  9 +++-
>  qom/cpu.c|  1 -
>  qom/object.c | 49 +++--
>  

Re: [Xen-devel] [PATCH for-3.2 v3 10/14] qdev-props: call object_apply_global_props()

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:48 +0400
Marc-André Lureau  wrote:

> It's now possible to use the common function.
> 
> Teach object_apply_global_props() to warn if Error argument is NULL.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/core/qdev-properties.c | 24 ++--
>  qom/object.c  |  6 +-
>  2 files changed, 7 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 8728cbab9f..239535a4cb 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1223,28 +1223,8 @@ int qdev_prop_check_globals(void)
>  
>  void qdev_prop_set_globals(DeviceState *dev)
>  {
> -int i;
> -
> -for (i = 0; i < global_props()->len; i++) {
> -GlobalProperty *prop;
> -Error *err = NULL;
> -
> -prop = g_array_index(global_props(), GlobalProperty *, i);
> -if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
> -continue;
> -}
> -prop->used = true;
> -object_property_parse(OBJECT(dev), prop->value, prop->property, 
> );
> -if (err != NULL) {
> -error_prepend(, "can't apply global %s.%s=%s: ",
> -  prop->driver, prop->property, prop->value);
> -if (!dev->hotplugged) {
> -error_propagate(_fatal, err);
> -} else {
> -warn_report_err(err);
> -}
> -}
> -}
> +object_apply_global_props(OBJECT(dev), global_props(),
> +  dev->hotplugged ? NULL : _fatal);
arguably, it's up to caller to decide it warn or not.
I'd leave it warning code out of object_apply_global_props() and let caller do 
the job

>  }
>  
>  /* --- 64bit unsigned int 'size' type --- */
> diff --git a/qom/object.c b/qom/object.c
> index 9acdf9e16d..b1a7f70550 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -392,7 +392,11 @@ void object_apply_global_props(Object *obj, GArray 
> *props, Error **errp)
>  if (err != NULL) {
>  error_prepend(, "can't apply global %s.%s=%s: ",
>p->driver, p->property, p->value);
> -error_propagate(errp, err);
> +if (errp) {
> +error_propagate(errp, err);
> +} else {
> +warn_report_err(err);
> +}
>  }
>  }
>  }


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-3.2 v3 08/14] qdev-props: convert global_props to GArray

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:46 +0400
Marc-André Lureau  wrote:

> A step towards being able to call object_apply_global_props().
it also makes code more uniform as we don't have to deal with type
inform of GList.

maybe move it at the beginning of series and include accel part as well?

otherwise looks good



> Signed-off-by: Marc-André Lureau 
> ---
>  hw/core/qdev-properties.c | 29 -
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 43c30a57f4..353e67c05a 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1173,22 +1173,32 @@ void qdev_prop_set_ptr(DeviceState *dev, const char 
> *name, void *value)
>  *ptr = value;
>  }
>  
> -static GList *global_props;
> +static GArray *global_props(void)
> +{
> +static GArray *gp;
> +
> +if (!gp) {
> +gp = g_array_new(false, false, sizeof(GlobalProperty *));
> +}
> +
> +return gp;
> +}
>  
>  void qdev_prop_register_global(GlobalProperty *prop)
>  {
> -global_props = g_list_append(global_props, prop);
> +g_array_append_val(global_props(), prop);
>  }
>  
>  int qdev_prop_check_globals(void)
>  {
> -GList *l;
> -int ret = 0;
> +int i, ret = 0;
>  
> -for (l = global_props; l; l = l->next) {
> -GlobalProperty *prop = l->data;
> +for (i = 0; i < global_props()->len; i++) {
> +GlobalProperty *prop;
>  ObjectClass *oc;
>  DeviceClass *dc;
> +
> +prop = g_array_index(global_props(), GlobalProperty *, i);
>  if (prop->used) {
>  continue;
>  }
> @@ -1213,12 +1223,13 @@ int qdev_prop_check_globals(void)
>  
>  void qdev_prop_set_globals(DeviceState *dev)
>  {
> -GList *l;
> +int i;
>  
> -for (l = global_props; l; l = l->next) {
> -GlobalProperty *prop = l->data;
> +for (i = 0; i < global_props()->len; i++) {
> +GlobalProperty *prop;
>  Error *err = NULL;
>  
> +prop = g_array_index(global_props(), GlobalProperty *, i);
>  if (object_dynamic_cast(OBJECT(dev), prop->driver) == NULL) {
>  continue;
>  }


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-3.2 v3 07/14] qdev: all globals are now user-provided

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:45 +0400
Marc-André Lureau  wrote:

> Considering that CPU features are provided via command line, the
I can guess what it is about once I recall how -cpu foo,+-feat works,
but without that knowledge I don't get meaning behind the sentence.
Could you rephrase it?

> global_props are now all user-provided globals. No need to track this
> anymore for qdev_prop_check_globals().
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/qdev-core.h |  3 --
>  hw/core/qdev-properties.c  |  4 ---
>  tests/test-qdev-global-props.c | 57 --
>  vl.c   |  1 -
>  4 files changed, 6 insertions(+), 59 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index a24d0dd566..baaf097212 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -249,8 +249,6 @@ struct PropertyInfo {
>  
>  /**
>   * GlobalProperty:
> - * @user_provided: Set to true if property comes from user-provided config
> - * (command-line or config file).
>   * @used: Set to true if property was used when initializing a device.
>   * @errp: Error destination, used like first argument of error_setg()
>   *in case property setting fails later. If @errp is NULL, we
> @@ -262,7 +260,6 @@ typedef struct GlobalProperty {
>  const char *driver;
>  const char *property;
>  const char *value;
> -bool user_provided;
>  bool used;
>  Error **errp;
>  } GlobalProperty;
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index bd84c4ea4c..43c30a57f4 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1192,9 +1192,6 @@ int qdev_prop_check_globals(void)
>  if (prop->used) {
>  continue;
>  }
> -if (!prop->user_provided) {
> -continue;
> -}
>  oc = object_class_by_name(prop->driver);
>  oc = object_class_dynamic_cast(oc, TYPE_DEVICE);
>  if (!oc) {
> @@ -1233,7 +1230,6 @@ void qdev_prop_set_globals(DeviceState *dev)
>  if (!dev->hotplugged && prop->errp) {
>  error_propagate(prop->errp, err);
>  } else {
> -assert(prop->user_provided);
>  warn_report_err(err);
>  }
>  }
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index 3a8d3170a0..f49a1b70b5 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -215,12 +215,12 @@ static void test_dynamic_globalprop_subprocess(void)
>  {
>  MyType *mt;
>  static GlobalProperty props[] = {
> -{ TYPE_DYNAMIC_PROPS, "prop1", "101", true },
> -{ TYPE_DYNAMIC_PROPS, "prop2", "102", true },
> -{ TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", true },
> -{ TYPE_UNUSED_HOTPLUG, "prop4", "104", true },
> -{ TYPE_UNUSED_NOHOTPLUG, "prop5", "105", true },
> -{ TYPE_NONDEVICE, "prop6", "106", true },
> +{ TYPE_DYNAMIC_PROPS, "prop1", "101", },
> +{ TYPE_DYNAMIC_PROPS, "prop2", "102", },
> +{ TYPE_DYNAMIC_PROPS"-bad", "prop3", "103", },
> +{ TYPE_UNUSED_HOTPLUG, "prop4", "104", },
> +{ TYPE_UNUSED_NOHOTPLUG, "prop5", "105", },
> +{ TYPE_NONDEVICE, "prop6", "106", },
>  {}
>  };
>  int global_error;
> @@ -255,46 +255,6 @@ static void test_dynamic_globalprop(void)
>  g_test_trap_assert_stdout("");
>  }
>  
> -/* Test setting of dynamic properties using user_provided=false properties */
> -static void test_dynamic_globalprop_nouser_subprocess(void)
> -{
> -MyType *mt;
> -static GlobalProperty props[] = {
> -{ TYPE_DYNAMIC_PROPS, "prop1", "101" },
> -{ TYPE_DYNAMIC_PROPS, "prop2", "102" },
> -{ TYPE_DYNAMIC_PROPS"-bad", "prop3", "103" },
> -{ TYPE_UNUSED_HOTPLUG, "prop4", "104" },
> -{ TYPE_UNUSED_NOHOTPLUG, "prop5", "105" },
> -{ TYPE_NONDEVICE, "prop6", "106" },
> -{}
> -};
> -int global_error;
> -
> -register_global_properties(props);
> -
> -mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
> -qdev_init_nofail(DEVICE(mt));
> -
> -g_assert_cmpuint(mt->prop1, ==, 101);
> -g_assert_cmpuint(mt->prop2, ==, 102);
> -global_error = qdev_prop_check_globals();
> -g_assert_cmpuint(global_error, ==, 0);
> -g_assert(props[0].used);
> -g_assert(props[1].used);
> -g_assert(!props[2].used);
> -g_assert(!props[3].used);
> -g_assert(!props[4].used);
> -g_assert(!props[5].used);
> -}
> -
> -static void test_dynamic_globalprop_nouser(void)
> -{
> -
> g_test_trap_subprocess("/qdev/properties/dynamic/global/nouser/subprocess", 
> 0, 0);
> -g_test_trap_assert_passed();
> -g_test_trap_assert_stderr("");
> -g_test_trap_assert_stdout("");
> -}
> -
>  /* Test if global props affecting subclasses are applied in the right order 
> */
>  static void 

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 02/14] qom: make interface types abstract

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:40 +0400
Marc-André Lureau  wrote:

> Interfaces don't have instance, let's make the interface type really
> abstract to avoid confusion.
> 
> Signed-off-by: Marc-André Lureau 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/acpi/acpi_dev_interface.h | 6 +-
>  include/hw/arm/linux-boot-if.h   | 5 +
>  include/hw/fw-path-provider.h| 4 +---
>  include/hw/hotplug.h | 6 +-
>  include/hw/intc/intc.h   | 4 +---
>  include/hw/ipmi/ipmi.h   | 4 +---
>  include/hw/isa/isa.h | 4 
>  include/hw/mem/memory-device.h   | 4 +---
>  include/hw/nmi.h | 4 +---
>  include/hw/stream.h  | 4 +---
>  include/hw/timer/m48t59.h| 4 +---
>  include/qom/object_interfaces.h  | 6 +-
>  include/sysemu/tpm.h | 4 +---
>  target/arm/idau.h| 4 +---
>  tests/check-qom-interface.c  | 4 +---
>  15 files changed, 14 insertions(+), 53 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi_dev_interface.h 
> b/include/hw/acpi/acpi_dev_interface.h
> index dabf4c4fc9..43ff119179 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -25,11 +25,7 @@ typedef enum {
>   INTERFACE_CHECK(AcpiDeviceIf, (obj), \
>   TYPE_ACPI_DEVICE_IF)
>  
> -
> -typedef struct AcpiDeviceIf {
> -/*  */
> -Object Parent;
> -} AcpiDeviceIf;
> +typedef struct AcpiDeviceIf AcpiDeviceIf;
>  
>  void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
>  
> diff --git a/include/hw/arm/linux-boot-if.h b/include/hw/arm/linux-boot-if.h
> index aba4479a14..7bbdfd1cc6 100644
> --- a/include/hw/arm/linux-boot-if.h
> +++ b/include/hw/arm/linux-boot-if.h
> @@ -16,10 +16,7 @@
>  #define ARM_LINUX_BOOT_IF(obj) \
>  INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
>  
> -typedef struct ARMLinuxBootIf {
> -/*< private >*/
> -Object parent_obj;
> -} ARMLinuxBootIf;
> +typedef struct ARMLinuxBootIf ARMLinuxBootIf;
>  
>  typedef struct ARMLinuxBootIfClass {
>  /*< private >*/
> diff --git a/include/hw/fw-path-provider.h b/include/hw/fw-path-provider.h
> index 050cb05d92..5df893a3d8 100644
> --- a/include/hw/fw-path-provider.h
> +++ b/include/hw/fw-path-provider.h
> @@ -30,9 +30,7 @@
>  #define FW_PATH_PROVIDER(obj) \
>   INTERFACE_CHECK(FWPathProvider, (obj), TYPE_FW_PATH_PROVIDER)
>  
> -typedef struct FWPathProvider {
> -Object parent_obj;
> -} FWPathProvider;
> +typedef struct FWPathProvider FWPathProvider;
>  
>  typedef struct FWPathProviderClass {
>  InterfaceClass parent_class;
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index 1a0516a479..6321e292fd 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -23,11 +23,7 @@
>  #define HOTPLUG_HANDLER(obj) \
>   INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
>  
> -
> -typedef struct HotplugHandler {
> -/*  */
> -Object Parent;
> -} HotplugHandler;
> +typedef struct HotplugHandler HotplugHandler;
>  
>  /**
>   * hotplug_fn:
> diff --git a/include/hw/intc/intc.h b/include/hw/intc/intc.h
> index 27d9828943..fb3e8e621f 100644
> --- a/include/hw/intc/intc.h
> +++ b/include/hw/intc/intc.h
> @@ -15,9 +15,7 @@
>  INTERFACE_CHECK(InterruptStatsProvider, (obj), \
>  TYPE_INTERRUPT_STATS_PROVIDER)
>  
> -typedef struct InterruptStatsProvider {
> -Object parent;
> -} InterruptStatsProvider;
> +typedef struct InterruptStatsProvider InterruptStatsProvider;
>  
>  typedef struct InterruptStatsProviderClass {
>  InterfaceClass parent;
> diff --git a/include/hw/ipmi/ipmi.h b/include/hw/ipmi/ipmi.h
> index 0affe5a4d8..99661d2bf0 100644
> --- a/include/hw/ipmi/ipmi.h
> +++ b/include/hw/ipmi/ipmi.h
> @@ -114,9 +114,7 @@ uint32_t ipmi_next_uuid(void);
>  #define IPMI_INTERFACE_GET_CLASS(class) \
>   OBJECT_GET_CLASS(IPMIInterfaceClass, (class), TYPE_IPMI_INTERFACE)
>  
> -typedef struct IPMIInterface {
> -Object parent;
> -} IPMIInterface;
> +typedef struct IPMIInterface IPMIInterface;
>  
>  typedef struct IPMIInterfaceClass {
>  InterfaceClass parent;
> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
> index b9dbab24b4..e62ac91c19 100644
> --- a/include/hw/isa/isa.h
> +++ b/include/hw/isa/isa.h
> @@ -43,10 +43,6 @@ static inline uint16_t applesmc_port(void)
>  #define ISADMA(obj) \
>  INTERFACE_CHECK(IsaDma, (obj), TYPE_ISADMA)
>  
> -struct IsaDma {
> -Object parent;
> -};
> -
>  typedef 

Re: [Xen-devel] [PATCH for-3.2 v3 06/14] qdev: do not mix compat props with global props

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:44 +0400
Marc-André Lureau  wrote:

> Machine & Accel props are not provided by user. Let's not mix them
> with the global properties.
> 
> Call a new helper function object_apply_global_props() during
> device_post_init().
> 
> Add a stub for current_machine, so qemu-user and tests can find a
> fallback symbol when linking with QDev.
> 
> The following patches is going to reuse object_apply_global_props()
> for qdev globals.
There are several things ongoing here,
 1. switching from GlobalProperty to GArray for accel
   maybe generalize and reuse SET_MACHINE_COMPAT() there?
SET_MACHINE_COMPAT() -> SET_COMPAT(GArray*, COMPAT)

 2. decoupling compat vs globals
 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/boards.h|  1 -
>  include/qom/object.h   |  2 ++
>  include/sysemu/accel.h |  4 +---
>  accel/accel.c  | 12 
>  hw/core/machine.c  | 18 --
>  hw/core/qdev.c |  8 
>  hw/xen/xen-common.c|  9 -
>  qom/object.c   | 25 +
>  stubs/machine.c|  4  
>  tests/test-qdev-global-props.c |  1 -
>  vl.c   |  2 --
>  stubs/Makefile.objs|  1 +
>  12 files changed, 49 insertions(+), 38 deletions(-)
>  create mode 100644 stubs/machine.c
> 
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index f82f28468b..c02190fc52 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -69,7 +69,6 @@ int machine_kvm_shadow_mem(MachineState *machine);
>  int machine_phandle_start(MachineState *machine);
>  bool machine_dump_guest_core(MachineState *machine);
>  bool machine_mem_merge(MachineState *machine);
> -void machine_register_compat_props(MachineState *machine);
>  HotpluggableCPUList *machine_query_hotpluggable_cpus(MachineState *machine);
>  void machine_set_cpu_numa_node(MachineState *machine,
> const CpuInstanceProperties *props,
> diff --git a/include/qom/object.h b/include/qom/object.h
> index f0b0bf39cc..e58eeb280f 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -679,6 +679,8 @@ Object *object_new_with_propv(const char *typename,
>Error **errp,
>va_list vargs);
>  
> +void object_apply_global_props(Object *obj, GArray *props, Error **errp);
> +
>  /**
>   * object_set_props:
>   * @obj: the object instance to set properties on
> diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
> index 637358f430..f4f71134b5 100644
> --- a/include/sysemu/accel.h
> +++ b/include/sysemu/accel.h
> @@ -49,7 +49,7 @@ typedef struct AccelClass {
>   * global properties may be overridden by machine-type
>   * compat_props or user-provided global properties.
>   */
> -GlobalProperty *global_props;
> +GArray *compat_props;
>  } AccelClass;
>  
>  #define TYPE_ACCEL "accel"
> @@ -67,8 +67,6 @@ typedef struct AccelClass {
>  extern unsigned long tcg_tb_size;
>  
>  void configure_accelerator(MachineState *ms);
> -/* Register accelerator specific global properties */
> -void accel_register_compat_props(AccelState *accel);
>  /* Called just before os_setup_post (ie just before drop OS privs) */
>  void accel_setup_post(MachineState *ms);
>  
> diff --git a/accel/accel.c b/accel/accel.c
> index 3da26eb90f..6db5d8f4df 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -119,18 +119,6 @@ void configure_accelerator(MachineState *ms)
>  }
>  }
>  
> -void accel_register_compat_props(AccelState *accel)
> -{
> -AccelClass *class = ACCEL_GET_CLASS(accel);
> -GlobalProperty *prop = class->global_props;
> -
> -for (; prop && prop->driver; prop++) {
> -/* Any compat_props must never cause error */
> -prop->errp = _abort;
> -qdev_prop_register_global(prop);
> -}
> -}
> -
>  void accel_setup_post(MachineState *ms)
>  {
>  AccelState *accel = ms->accelerator;
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index da50ad6de7..d45945 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -844,24 +844,6 @@ static void machine_class_finalize(ObjectClass *klass, 
> void *data)
>  g_free(mc->name);
>  }
>  
> -void machine_register_compat_props(MachineState *machine)
> -{
> -MachineClass *mc = MACHINE_GET_CLASS(machine);
> -int i;
> -GlobalProperty *p;
> -
> -if (!mc->compat_props) {
> -return;
> -}
> -
> -for (i = 0; i < mc->compat_props->len; i++) {
> -p = g_array_index(mc->compat_props, GlobalProperty *, i);
> -/* Machine compat_props must never cause errors: */
> -p->errp = _abort;
> -qdev_prop_register_global(p);
> -}
> -}
> -
>  static const TypeInfo machine_info = {
>  .name = TYPE_MACHINE,
>  .parent = TYPE_OBJECT,
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 

Re: [Xen-devel] [Qemu-devel] [PATCH v5 11/24] hw: acpi: Export and generalize the PCI host AML API

[Igor Mammedov]

On Thu, 22 Nov 2018 00:12:17 +0100
Samuel Ortiz  wrote:

> Hi Igor,
> 
> On Wed, Nov 14, 2018 at 11:55:37AM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:34 +0100
> > Samuel Ortiz  wrote:
> >   
> > > From: Yang Zhong 
> > > 
> > > The AML build routines for the PCI host bridge and the corresponding
> > > DSDT addition are neither x86 nor PC machine type specific.
> > > We can move them to the architecture agnostic hw/acpi folder, and by
> > > carrying all the needed information through a new AcpiPciBus structure,
> > > we can make them PC machine type independent.  
> > 
> > I'm don't know anything about PCI, but functional changes doesn't look
> > correct to me.
> >
> > See more detailed comments below.
> > 
> > Marcel,
> > could you take a look on this patch (in particular main csr changes), pls?
> >   
> > > 
> > > Signed-off-by: Yang Zhong 
> > > Signed-off-by: Rob Bradford 
> > > Signed-off-by: Samuel Ortiz 
> > > ---
> > >  include/hw/acpi/aml-build.h |   8 ++
> > >  hw/acpi/aml-build.c | 157 
> > >  hw/i386/acpi-build.c| 115 ++
> > >  3 files changed, 173 insertions(+), 107 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index fde2785b9a..1861e37ebf 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -229,6 +229,12 @@ typedef struct AcpiMcfgInfo {
> > >  uint32_t mcfg_size;
> > >  } AcpiMcfgInfo;
> > >  
> > > +typedef struct AcpiPciBus {
> > > +PCIBus *pci_bus;
> > > +Range *pci_hole;
> > > +Range *pci_hole64;
> > > +} AcpiPciBus;  
> > Again, this and all below is not aml-build material.
> > Consider adding/using pci specific acpi file for it.
> > 
> > Also even though pci AML in arm/virt is to a large degree a subset
> > of x86 target and it would be much better to unify ARM part with x86,
> > it probably will be to big/complex of a change if we take on it in
> > one go.
> > 
> > So not to derail you from the goal too much, we probably should
> > generalize this a little bit less, limiting refactoring to x86
> > target for now.  
> So keeping it under i386 means it won't be accessible through hw/acpi/,
> which means we won't be able to have a generic hw/acpi/reduced.c
> implementation. From our perspective, this is the problem with keeping
> things under i386 because we're not sure yet how much generic it is: It
> still won't be shareable for a generic hardware-reduced ACPI
> implementation which means we'll have to temporarily have yet another
> hardware-reduced ACPI implementation under hw/i386 this time.
> I guess this is what Michael meant by keeping some parts of the code
> duplicated for now.
> 
> I feel it'd be easier to move those APIs under a shareable location, to
> make it easier for ARM to consume it even if it's not entirely generic yet.
> But you guys are the maintainers and if you think we should restric the
> generalization to x86 only for now, we can go for it.
If code is generic (you can reuse it with arm/virt in the same series)
then place it in hw/acpi otherwise if it's semi-generic put to hw/i386.
If it would be a separate file it would be easier to move it to generic
place when we are able to resuse it with arm/virt.

 
> Cheers,
> Samuel.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 10/24] hw: acpi: Export the PCI host and holes getters

[Igor Mammedov]

On Wed, 21 Nov 2018 16:43:12 +0100
Samuel Ortiz  wrote:

> On Tue, Nov 13, 2018 at 04:59:18PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:33 +0100
> > Samuel Ortiz  wrote:
> >   
> > > This is going to be needed by the hardware reduced implementation, so
> > > let's export it.
> > > Once the ACPI builder methods and getters will be implemented, the
> > > acpi_get_pci_host() implementation will become hardware agnostic.
> > > 
> > > Signed-off-by: Samuel Ortiz 
> > > ---
> > >  include/hw/acpi/aml-build.h |  2 ++
> > >  hw/acpi/aml-build.c | 43 +
> > >  hw/i386/acpi-build.c| 47 ++---
> > >  3 files changed, 47 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index c27c0935ae..fde2785b9a 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -400,6 +400,8 @@ build_header(BIOSLinker *linker, GArray *table_data,
> > >   const char *oem_id, const char *oem_table_id);
> > >  void *acpi_data_push(GArray *table_data, unsigned size);
> > >  unsigned acpi_data_len(GArray *table);
> > > +Object *acpi_get_pci_host(void);
> > > +void acpi_get_pci_holes(Range *hole, Range *hole64);
> > >  /* Align AML blob size to a multiple of 'align' */
> > >  void acpi_align_size(GArray *blob, unsigned align);
> > >  void acpi_add_table(GArray *table_offsets, GArray *table_data);
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index 2b9a636e75..b8e32f15f7 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1601,6 +1601,49 @@ void acpi_build_tables_cleanup(AcpiBuildTables 
> > > *tables, bool mfre)
> > >  g_array_free(tables->vmgenid, mfre);
> > >  }  
> >   
> > > +/*
> > > + * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> > > + */
> > > +Object *acpi_get_pci_host(void)
> > > +{
> > > +PCIHostState *host;
> > > +
> > > +host = OBJECT_CHECK(PCIHostState,
> > > +object_resolve_path("/machine/i440fx", NULL),
> > > +TYPE_PCI_HOST_BRIDGE);
> > > +if (!host) {
> > > +host = OBJECT_CHECK(PCIHostState,
> > > +object_resolve_path("/machine/q35", NULL),
> > > +TYPE_PCI_HOST_BRIDGE);
> > > +}
> > > +
> > > +return OBJECT(host);
> > > +}  
> > in general aml-build.c is a place to put ACPI spec primitives,
> > so I'd suggest to move it somewhere else.
> > 
> > Considering it's x86 code (so far), maybe move it to something like
> > hw/i386/acpi-pci.c
> > 
> > Also it might be good to get rid of acpi_get_pci_host() and pass
> > a pointer to pci_host as acpi_setup() an argument, since it's static
> > for life of boar we can keep it in AcpiBuildState, and reuse for
> > mfg/pci_hole/pci bus accesses.  
> That's what I'm trying to do with patches #23 and 24, but through the
> ACPI configuration structure. I could try using the build state instead,
> as it's platform agnostic as well.
May be it will work.
Note:
try not to pass whole build_state to concrete tables builders and use
arguments/dedicated structures to pass data needed for that concrete
table.

> 
> Cheers,
> Samuel.
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP

[Igor Mammedov]

On Wed, 21 Nov 2018 15:42:11 +0100
Samuel Ortiz  wrote:

> Hi Igor,
> 
> On Thu, Nov 08, 2018 at 03:16:23PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:28 +0100
> > Samuel Ortiz  wrote:
> >   
> > > XSDT is the 64-bit version of the legacy ACPI RSDT (Root System
> > > Description Table). RSDT only allow for 32-bit addressses and have thus
> > > been deprecated. Since ACPI version 2.0, RSDPs should point at XSDTs and
> > > no longer RSDTs, although RSDTs are still supported for backward
> > > compatibility.
> > > 
> > > Since version 2.0, RSDPs should add an extended checksum, a complete table
> > > length and a version field to the table.  
> > 
> > This patch re-implements what arm/virt board already does
> > and fixes checksum bug in the later and at the same time
> > without a user (within the patch).
> > 
> > I'd suggest redo it a way similar to FADT refactoring
> >   patch 1: fix checksum bug in virt/arm
> >   patch 2: update reference tables in test
> >   patch 3: introduce AcpiRsdpData similar to commit 937d1b587
> >  (both arm and x86) wich stores all data in hos byte order
> >   patch 4: convert arm's impl. to build_append_int_noprefix() API (commit 
> > 5d7a334f7)
> >
> >... move out to aml-build.c
> >   patch 5: reuse generalized arm's build_rsdp() for x86, dropping x86 
> > specific one
> >   amending it to generate rev1 variant defined by revision in 
> > AcpiRsdpData
> >   (commit dd1b2037a)  
> I agree patches #1, #2 and #5 make sense. 3 and 4 as well, but here
> you're asking about something that's out of scope of the current serie.
/me guilty of that, but I have excuses for doing so:
  * that's the only way to get rid of legacy approach given limited resources.
So task goes to whomever touches old code. /others and me included/
I'd be glad if someone would volunteer and do clean ups but in absence
of such, the victim is interested party.
  * contributor to ACPI part learns how to use preferred approach,
makes code more robust and clear as it's not possible to make
endianness mistakes, very simple to review and notice mistakes
as end result practically matches row by row a table described in spec.
  * there could be exceptions, like acpi/nvdimm.c also contributed by Intel
whole file written in legacy style (it probably started before I started
enforcing conversions, anyways it's late now and should be done as whole),
or odd fixes to existing tables, or too complex case.
(depending on case I might still ask for conversion)

My ranting aside, conversions I've asked for here are trivial and for
everyone's benefit /QEMU gets more maintainable code, users less bugs,
contributor knows requirements hence his patches go through less iterations,
hopefully if contributor stays around and does contributions/reviews to
acpi code, QEMU could get another co-maintainer for acpi part/

> I'll move those patches from this serie and build a 6 patches new serie
> as suggested.

Thanks!

> 
> Cheers,
> Samuel.
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 01/24] hw: i386: Decouple the ACPI build from the PC machine type

[Igor Mammedov]

On Wed, 21 Nov 2018 15:42:37 +0100
Samuel Ortiz  wrote:

> Hi Igor,
> 
> On Fri, Nov 09, 2018 at 03:23:16PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:24 +0100
> > Samuel Ortiz  wrote:
> >   
> > > ACPI tables are platform and machine type and even architecture
> > > agnostic, and as such we want to provide an internal ACPI API that
> > > only depends on platform agnostic information.
> > > 
> > > For the x86 architecture, in order to build ACPI tables independently
> > > from the PC or Q35 machine types, we are moving a few MachineState
> > > structure fields into a machine type agnostic structure called
> > > AcpiConfiguration. The structure fields we move are:  
> > 
> > It's not obvious why new structure is needed, especially at
> > the beginning of series. We probably should place this patch
> > much later in the series (if we need it at all) and try
> > generalize a much as possible without using it.  
> Patches order set aside, this new structure is needed to make the
> existing API not completely bound to the pc machine type anymore and
> "Decouple the ACPI build from the PC machine type".
> 
> It was either creating a structure to build ACPI tables in a machine
> type independent fashion, or pass custom structures (or potentially long
> list of arguments) to the existing APIs. See below.
> 
> 
> > And try to come up with an API that doesn't need centralized collection
> > of data somehow related to ACPI (most of the fields here are not generic
> > and applicable to a specific board/target).
> > 
> > For generic API, I'd prefer a separate building blocks
> > like build_fadt()/... that take as an input only parameters
> > necessary to compose a table/aml part with occasional board
> > interface hooks instead of all encompassing AcpiConfiguration
> > and board specific 'acpi_build' that would use them when/if needed.  
> Let's take build_madt as an example. With my patch we define:
> 
> void build_madt(GArray *table_data, BIOSLinker *linker,
> MachineState *ms, AcpiConfiguration *conf);
> 
> And you're suggesting we'd define:
> 
> void build_madt(GArray *table_data, BIOSLinker *linker,
> MachineState *ms, HotplugHandler *acpi_dev,
> bool apic_xrupt_override);
> 
> instead. Is that correct?
in general, yes
and let acpi_build() to fish out that info from board somehow.

In case of build_madt(), I doubt we can generalize it across targets.
What we can do with it though, is to generalize entries that are
placed into it. i.e. create helpers to create them instead of
open-codding them like now, something like:

void build_append_madt_apic(*table, uid, apic_id, flags);
void build_append_madt_x2apic(*table, uid, apic_id, flags);
void build_append_madt_ioapic(*table, io_apic_id, io_apic_addr, interrupt);
...
converting to build_append_int_noprefix() API ioapic entry example:

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index af8e023..5911b94 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -242,16 +242,6 @@ struct AcpiMadtProcessorApic {
 } QEMU_PACKED;
 typedef struct AcpiMadtProcessorApic AcpiMadtProcessorApic;
 
-struct AcpiMadtIoApic {
-ACPI_SUB_HEADER_DEF
-uint8_t  io_apic_id; /* I/O APIC ID */
-uint8_t  reserved;   /* Reserved - must be zero */
-uint32_t address;/* APIC physical address */
-uint32_t interrupt;  /* Global system interrupt where INTI
- * lines start */
-} QEMU_PACKED;
-typedef struct AcpiMadtIoApic AcpiMadtIoApic;
-
 struct AcpiMadtIntsrcovr {
 ACPI_SUB_HEADER_DEF
 uint8_t  bus;
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 6c36903..b28c2ce 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -409,6 +409,9 @@ build_append_gas_from_struct(GArray *table, const struct 
AcpiGenericAddress *s)
  s->access_width, s->address);
 }
 
+void build_append_madt_ioapic(GArray *tbl, uint8_t id, uint32_t addr,
+  uint32_t intr);
+
 void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
uint64_t len, int node, MemoryAffinityFlags flags);
 
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 1e43cd7..f0445df 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1655,6 +1655,23 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, 
uint64_t base,
 }
 
 /*
+ * ACPI 1.0b: 5.2.8.2 IO APIC
+   <--- earliest spec where structure was introduced, that's the way
+we avoid writing extra docs for things that are described in spec --

Re: [Xen-devel] [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition

[Igor Mammedov]

On Wed, 21 Nov 2018 15:38:16 +0100
Samuel Ortiz  wrote:

> Igor,
> 
> On Wed, Nov 21, 2018 at 03:15:26PM +0100, Igor Mammedov wrote:
> > On Wed, 21 Nov 2018 07:35:47 -0500
> > "Michael S. Tsirkin"  wrote:
> >   
> > > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 16 Nov 2018 17:37:54 +0100
> > > > Paolo Bonzini  wrote:
> > > > 
> > > > > On 16/11/18 17:29, Igor Mammedov wrote:
> > > > > > General suggestions for this series:
> > > > > >   1. Preferably don't do multiple changes within a patch
> > > > > >  neither post huge patches (unless it's pure code movement).
> > > > > >  (it's easy to squash patches later it necessary)
> > > > > >   2. Start small, pick a table generalize it and send as
> > > > > >  one small patchset. Tables are often independent
> > > > > >  and it's much easier on both author/reviewer to agree upon
> > > > > >  changes and rewrite it if necessary.  
> > > > > 
> > > > > How would that be done?  This series is on the bigger side, agreed, 
> > > > > but
> > > > > most of it is really just code movement.  It's a starting point, 
> > > > > having
> > > > > a generic ACPI library is way beyond what this is trying to do.
> > > > I've tried to give suggestions how to restructure series
> > > > on per patch basis. In my opinion it quite possible to split
> > > > series in several smaller ones and it should really help with
> > > > making series cleaner and easier/faster to review/amend/merge
> > > > vs what we have in v5.
> > > > (it's more frustrating to rework large series vs smaller one)
> > > > 
> > > > If something isn't clear, it's easy to reach out to me here
> > > > or directly (email/irc/github) for clarification/feed back.
> > > 
> > > I assume the #1 goal is to add reduced HW support.  So another
> > > option to speed up merging is to just go ahead and duplicate a
> > > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other
> > > file.
> > > This way it might be easier to see what's common code and what isn't.
> > > And I think offline Igor said he might prefer that way. Right Igor?  
> > You mean probably 'x86 reduced hw' support. That's was what I've
> > already suggested for PCI AML code during patch review. Just don't
> > call it generic when it's not and place code in hw/i386/ directory beside
> > acpi-build.c. It might apply to some other tables (i.e. complex cases).
> > 
> > On per patch review I gave suggestions how to amend series to make
> > it acceptable without doing complex refactoring and pointed out
> > places we probably shouldn't refactor now and just duplicate as
> > it's too complex or not clear how to generalize it yet.
> > 
> > Problem with duplication is that a random contributor is not
> > around to clean code up after a feature is merged and we end up
> > with a bunch of messy code.
> > 
> > A word to the contributors,
> > Don't do refactoring in silence, keep discussing approaches here,
> > suggest alternatives. That way it's easier to reach a compromise
> > and merge it with less iterations. And if you do split it in smaller
> > parts, the process should go even faster.
> > 
> > I'll sent a small RSDP refactoring series for reference.  
> I was already working on the RSDP changes. Let me know if I should drop
> that work too.
Go ahead,
you can reuse RSDP fixes I've just posted (you are CCed)
if you haven't written them yet on your own.

 
> Cheers,
> Samuel.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition

[Igor Mammedov]

On Wed, 21 Nov 2018 07:35:47 -0500
"Michael S. Tsirkin"  wrote:

> On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote:
> > On Fri, 16 Nov 2018 17:37:54 +0100
> > Paolo Bonzini  wrote:
> >   
> > > On 16/11/18 17:29, Igor Mammedov wrote:  
> > > > General suggestions for this series:
> > > >   1. Preferably don't do multiple changes within a patch
> > > >  neither post huge patches (unless it's pure code movement).
> > > >  (it's easy to squash patches later it necessary)
> > > >   2. Start small, pick a table generalize it and send as
> > > >  one small patchset. Tables are often independent
> > > >  and it's much easier on both author/reviewer to agree upon
> > > >  changes and rewrite it if necessary.
> > > 
> > > How would that be done?  This series is on the bigger side, agreed, but
> > > most of it is really just code movement.  It's a starting point, having
> > > a generic ACPI library is way beyond what this is trying to do.  
> > I've tried to give suggestions how to restructure series
> > on per patch basis. In my opinion it quite possible to split
> > series in several smaller ones and it should really help with
> > making series cleaner and easier/faster to review/amend/merge
> > vs what we have in v5.
> > (it's more frustrating to rework large series vs smaller one)
> > 
> > If something isn't clear, it's easy to reach out to me here
> > or directly (email/irc/github) for clarification/feed back.  
> 
> I assume the #1 goal is to add reduced HW support.  So another
> option to speed up merging is to just go ahead and duplicate a
> bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other
> file.
> This way it might be easier to see what's common code and what isn't.
> And I think offline Igor said he might prefer that way. Right Igor?
You mean probably 'x86 reduced hw' support. That's was what I've
already suggested for PCI AML code during patch review. Just don't
call it generic when it's not and place code in hw/i386/ directory beside
acpi-build.c. It might apply to some other tables (i.e. complex cases).

On per patch review I gave suggestions how to amend series to make
it acceptable without doing complex refactoring and pointed out
places we probably shouldn't refactor now and just duplicate as
it's too complex or not clear how to generalize it yet.

Problem with duplication is that a random contributor is not
around to clean code up after a feature is merged and we end up
with a bunch of messy code.

A word to the contributors,
Don't do refactoring in silence, keep discussing approaches here,
suggest alternatives. That way it's easier to reach a compromise
and merge it with less iterations. And if you do split it in smaller
parts, the process should go even faster.

I'll sent a small RSDP refactoring series for reference.

> > > Paolo
> > >   
> > > >   3. when you think about refactoring acpi into a generic API
> > > >  think about it as routines that go into a separate library
> > > >  (pure acpi spec code) and qemu/acpi glue routines and
> > > >   divide them correspondingly.
> > >   


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 02/14] qom: make interface types abstract

[Igor Mammedov]

On Tue, 20 Nov 2018 19:54:23 +0100
Laszlo Ersek  wrote:

> On 11/20/18 17:33, Igor Mammedov wrote:
> > On Wed,  7 Nov 2018 16:36:40 +0400
> > Marc-André Lureau  wrote:
> >   
> >> Interfaces don't have instance, let's make the interface type really
> >> abstract to avoid confusion.
> >>
> >> Signed-off-by: Marc-André Lureau 
> >> ---
> >>  include/hw/acpi/acpi_dev_interface.h | 6 +-
> >>  include/hw/arm/linux-boot-if.h   | 5 +
> >>  include/hw/fw-path-provider.h| 4 +---
> >>  include/hw/hotplug.h | 6 +-
> >>  include/hw/intc/intc.h   | 4 +---
> >>  include/hw/ipmi/ipmi.h   | 4 +---
> >>  include/hw/isa/isa.h | 4 
> >>  include/hw/mem/memory-device.h   | 4 +---
> >>  include/hw/nmi.h | 4 +---
> >>  include/hw/stream.h  | 4 +---
> >>  include/hw/timer/m48t59.h| 4 +---
> >>  include/qom/object_interfaces.h  | 6 +-
> >>  include/sysemu/tpm.h | 4 +---
> >>  target/arm/idau.h| 4 +---
> >>  tests/check-qom-interface.c  | 4 +---
> >>  15 files changed, 14 insertions(+), 53 deletions(-)
> >>
> >> diff --git a/include/hw/acpi/acpi_dev_interface.h 
> >> b/include/hw/acpi/acpi_dev_interface.h
> >> index dabf4c4fc9..43ff119179 100644
> >> --- a/include/hw/acpi/acpi_dev_interface.h
> >> +++ b/include/hw/acpi/acpi_dev_interface.h
> >> @@ -25,11 +25,7 @@ typedef enum {
> >>   INTERFACE_CHECK(AcpiDeviceIf, (obj), \
> >>   TYPE_ACPI_DEVICE_IF)
> >>  
> >> -
> >> -typedef struct AcpiDeviceIf {
> >> -/*  */
> >> -Object Parent;
> >> -} AcpiDeviceIf;
> >> +typedef struct AcpiDeviceIf AcpiDeviceIf;
> >>  
> >>  void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
> >>  
> >> diff --git a/include/hw/arm/linux-boot-if.h 
> >> b/include/hw/arm/linux-boot-if.h
> >> index aba4479a14..7bbdfd1cc6 100644
> >> --- a/include/hw/arm/linux-boot-if.h
> >> +++ b/include/hw/arm/linux-boot-if.h
> >> @@ -16,10 +16,7 @@
> >>  #define ARM_LINUX_BOOT_IF(obj) \
> >>  INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
> >>  
> >> -typedef struct ARMLinuxBootIf {
> >> -/*< private >*/
> >> -Object parent_obj;
> >> -} ARMLinuxBootIf;
> >> +typedef struct ARMLinuxBootIf ARMLinuxBootIf;  
> > I like how it makes interface truly opaque and removes the need for
> > structure declaration but:
> > 
> >  1: I'm not sure if it's acceptable thing to do from language point of view 
> >  
> 
> Yeah, it's fine. If you have just
> 
> struct ARMLinuxBootIf;
> 
> (and, optionally, a typedef to it,) then this type is called an
> "incomplete type" (for translation units that don't see the actual type
> definition). You can't apply the "sizeof" operator to it, you can't put
> it in other structs and arrays etc. I'm too lazy to look up the exact
> details in the C standard now. :) But, importantly,
> "pointer-to-ARMLinuxBootIf" is a complete type, and you can do all the
> usual things with that. (Define variables of that pointer type, embed
> them in other structures, use it as an array element type, pass them to
> functions, and so on.)

Thanks Laszlo, that's the answer I was looking for.

> Thanks
> Laszlo
> 
> >  2: For a reader not aware of a trick, it's sort of confusing to have 
> > forward declaration but without structure itself. So if #1 is acceptable we 
> > probably should document interface trick in object.h
> > 
> > [...]
> >   
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 05/14] qdev: move qdev_prop_register_global_list() to tests

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:43 +0400
Marc-André Lureau  wrote:

> The function is only used by a test, move it there.
> 
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Eduardo Habkost 

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/qdev-properties.h   |  1 -
>  hw/core/qdev-properties.c  |  9 -
>  tests/test-qdev-global-props.c | 18 ++
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index a95f4a73eb..3ab9cd2eb6 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -249,7 +249,6 @@ void qdev_prop_set_enum(DeviceState *dev, const char 
> *name, int value);
>  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>  
>  void qdev_prop_register_global(GlobalProperty *prop);
> -void qdev_prop_register_global_list(GlobalProperty *props);
>  int qdev_prop_check_globals(void);
>  void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ab61d502fd..bd84c4ea4c 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1180,15 +1180,6 @@ void qdev_prop_register_global(GlobalProperty *prop)
>  global_props = g_list_append(global_props, prop);
>  }
>  
> -void qdev_prop_register_global_list(GlobalProperty *props)
> -{
> -int i;
> -
> -for (i = 0; props[i].driver != NULL; i++) {
> -qdev_prop_register_global(props+i);
> -}
> -}
> -
>  int qdev_prop_check_globals(void)
>  {
>  GList *l;
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index ccdf6c57c1..b1eb505442 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -89,6 +89,16 @@ static void test_static_prop(void)
>  g_test_trap_assert_stdout("");
>  }
>  
> +static void register_global_properties(GlobalProperty *props)
> +{
> +int i;
> +
> +for (i = 0; props[i].driver != NULL; i++) {
> +qdev_prop_register_global(props + i);
> +}
> +}
> +
> +
>  /* Test setting of static property using global properties */
>  static void test_static_globalprop_subprocess(void)
>  {
> @@ -98,7 +108,7 @@ static void test_static_globalprop_subprocess(void)
>  {}
>  };
>  
> -qdev_prop_register_global_list(props);
> +register_global_properties(props);
>  
>  mt = STATIC_TYPE(object_new(TYPE_STATIC_PROPS));
>  qdev_init_nofail(DEVICE(mt));
> @@ -216,7 +226,7 @@ static void test_dynamic_globalprop_subprocess(void)
>  };
>  int global_error;
>  
> -qdev_prop_register_global_list(props);
> +register_global_properties(props);
>  
>  mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
>  qdev_init_nofail(DEVICE(mt));
> @@ -261,7 +271,7 @@ static void 
> test_dynamic_globalprop_nouser_subprocess(void)
>  };
>  int global_error;
>  
> -qdev_prop_register_global_list(props);
> +register_global_properties(props);
>  
>  mt = DYNAMIC_TYPE(object_new(TYPE_DYNAMIC_PROPS));
>  qdev_init_nofail(DEVICE(mt));
> @@ -299,7 +309,7 @@ static void test_subclass_global_props(void)
>  {}
>  };
>  
> -qdev_prop_register_global_list(props);
> +register_global_properties(props);
>  
>  mt = STATIC_TYPE(object_new(TYPE_SUBCLASS));
>  qdev_init_nofail(DEVICE(mt));


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-3.2 v3 03/14] qom: make user_creatable_complete() specific to UserCreatable

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:41 +0400
Marc-André Lureau  wrote:

> Instead of accepting any Object*, change user_creatable_complete() to
> require a UserCreatable*. Modify the callers to pass the appropriate
> argument, removing redundant dynamic cast checks in object creation.

Looks like it doesn't apply to current HEAD anymore
 
> 
> Signed-off-by: Marc-André Lureau 
> Reviewed-by: Igor Mammedov 
> ---
>  include/qom/object_interfaces.h |  4 ++--
>  hw/misc/ivshmem.c   |  2 +-
>  hw/virtio/virtio-rng.c  |  2 +-
>  qom/object.c| 12 
>  qom/object_interfaces.c | 14 +++---
>  5 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 652a16d2ba..682ba1d9b0 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -51,14 +51,14 @@ typedef struct UserCreatableClass {
>  
>  /**
>   * user_creatable_complete:
> - * @obj: the object whose complete() method is called if defined
> + * @uc: the user-creatable object whose complete() method is called if 
> defined
>   * @errp: if an error occurs, a pointer to an area to store the error
>   *
>   * Wrapper to call complete() method if one of types it's inherited
>   * from implements USER_CREATABLE interface, otherwise the call does
>   * nothing.
>   */
> -void user_creatable_complete(Object *obj, Error **errp);
> +void user_creatable_complete(UserCreatable *uc, Error **errp);
>  
>  /**
>   * user_creatable_can_be_deleted:
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index f88910e55c..478f41044c 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1279,7 +1279,7 @@ static void desugar_shm(IVShmemState *s)
>  object_property_set_bool(obj, true, "share", _abort);
>  object_property_add_child(OBJECT(s), "internal-shm-backend", obj,
>_abort);
> -user_creatable_complete(obj, _abort);
> +user_creatable_complete(USER_CREATABLE(obj), _abort);
>  s->hostmem = MEMORY_BACKEND(obj);
>  }
>  
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 855f1b41d1..30493a2586 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -191,7 +191,7 @@ static void virtio_rng_device_realize(DeviceState *dev, 
> Error **errp)
>  if (vrng->conf.rng == NULL) {
>  vrng->conf.default_backend = RNG_RANDOM(object_new(TYPE_RNG_RANDOM));
>  
> -user_creatable_complete(OBJECT(vrng->conf.default_backend),
> +user_creatable_complete(USER_CREATABLE(vrng->conf.default_backend),
>  _err);
>  if (local_err) {
>  error_propagate(errp, local_err);
> diff --git a/qom/object.c b/qom/object.c
> index 547dcf97c3..eb770dbf7f 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -417,6 +417,7 @@ void object_initialize_childv(Object *parentobj, const 
> char *propname,
>  {
>  Error *local_err = NULL;
>  Object *obj;
> +UserCreatable *uc;
>  
>  object_initialize(childobj, size, type);
>  obj = OBJECT(childobj);
> @@ -431,8 +432,9 @@ void object_initialize_childv(Object *parentobj, const 
> char *propname,
>  goto out;
>  }
>  
> -if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> -user_creatable_complete(obj, _err);
> +uc = (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> +if (uc) {
> +user_creatable_complete(uc, _err);
>  if (local_err) {
>  object_unparent(obj);
>  goto out;
> @@ -590,6 +592,7 @@ Object *object_new_with_propv(const char *typename,
>  Object *obj;
>  ObjectClass *klass;
>  Error *local_err = NULL;
> +UserCreatable *uc;
>  
>  klass = object_class_by_name(typename);
>  if (!klass) {
> @@ -612,8 +615,9 @@ Object *object_new_with_propv(const char *typename,
>  goto error;
>  }
>  
> -if (object_dynamic_cast(obj, TYPE_USER_CREATABLE)) {
> -user_creatable_complete(obj, _err);
> +uc = (UserCreatable *)object_dynamic_cast(obj, TYPE_USER_CREATABLE);
> +if (uc) {
> +user_creatable_complete(uc, _err);
>  if (local_err) {
>  object_unparent(obj);
>  goto error;
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 97b79b48bb..db85d1eb75 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -8,18 +8,10 @@
>  #include "qapi/opts-visitor.h"
>  #include "qemu/config-file.h"
>  
> -void user_cr

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 02/14] qom: make interface types abstract

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:40 +0400
Marc-André Lureau  wrote:

> Interfaces don't have instance, let's make the interface type really
> abstract to avoid confusion.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  include/hw/acpi/acpi_dev_interface.h | 6 +-
>  include/hw/arm/linux-boot-if.h   | 5 +
>  include/hw/fw-path-provider.h| 4 +---
>  include/hw/hotplug.h | 6 +-
>  include/hw/intc/intc.h   | 4 +---
>  include/hw/ipmi/ipmi.h   | 4 +---
>  include/hw/isa/isa.h | 4 
>  include/hw/mem/memory-device.h   | 4 +---
>  include/hw/nmi.h | 4 +---
>  include/hw/stream.h  | 4 +---
>  include/hw/timer/m48t59.h| 4 +---
>  include/qom/object_interfaces.h  | 6 +-
>  include/sysemu/tpm.h | 4 +---
>  target/arm/idau.h| 4 +---
>  tests/check-qom-interface.c  | 4 +---
>  15 files changed, 14 insertions(+), 53 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi_dev_interface.h 
> b/include/hw/acpi/acpi_dev_interface.h
> index dabf4c4fc9..43ff119179 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -25,11 +25,7 @@ typedef enum {
>   INTERFACE_CHECK(AcpiDeviceIf, (obj), \
>   TYPE_ACPI_DEVICE_IF)
>  
> -
> -typedef struct AcpiDeviceIf {
> -/*  */
> -Object Parent;
> -} AcpiDeviceIf;
> +typedef struct AcpiDeviceIf AcpiDeviceIf;
>  
>  void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
>  
> diff --git a/include/hw/arm/linux-boot-if.h b/include/hw/arm/linux-boot-if.h
> index aba4479a14..7bbdfd1cc6 100644
> --- a/include/hw/arm/linux-boot-if.h
> +++ b/include/hw/arm/linux-boot-if.h
> @@ -16,10 +16,7 @@
>  #define ARM_LINUX_BOOT_IF(obj) \
>  INTERFACE_CHECK(ARMLinuxBootIf, (obj), TYPE_ARM_LINUX_BOOT_IF)
>  
> -typedef struct ARMLinuxBootIf {
> -/*< private >*/
> -Object parent_obj;
> -} ARMLinuxBootIf;
> +typedef struct ARMLinuxBootIf ARMLinuxBootIf;
I like how it makes interface truly opaque and removes the need for
structure declaration but:

 1: I'm not sure if it's acceptable thing to do from language point of view

 2: For a reader not aware of a trick, it's sort of confusing to have forward 
declaration but without structure itself. So if #1 is acceptable we probably 
should document interface trick in object.h

[...]

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v3 01/14] tests: qdev_prop_check_globals() doesn't return "all_used"

[Igor Mammedov]

On Wed,  7 Nov 2018 16:36:39 +0400
Marc-André Lureau  wrote:

> Instead, it returns 1 if an error was detected, which is the case for:
> 
> /qdev/properties/dynamic/global/subprocess:
> warning: global dynamic-prop-type-bad.prop3 has invalid class name
> warning: global nohotplug-type.prop5=105 not used
> warning: global nondevice-type.prop6 has invalid class name
> 
> Clarify the function return value.
> 
> Signed-off-by: Marc-André Lureau 

Reviewed-by: Igor Mammedov 

> ---
>  tests/test-qdev-global-props.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/test-qdev-global-props.c b/tests/test-qdev-global-props.c
> index d81b0862d5..ccdf6c57c1 100644
> --- a/tests/test-qdev-global-props.c
> +++ b/tests/test-qdev-global-props.c
> @@ -214,7 +214,7 @@ static void test_dynamic_globalprop_subprocess(void)
>  { TYPE_NONDEVICE, "prop6", "106", true },
>  {}
>  };
> -int all_used;
> +int global_error;
>  
>  qdev_prop_register_global_list(props);
>  
> @@ -223,8 +223,8 @@ static void test_dynamic_globalprop_subprocess(void)
>  
>  g_assert_cmpuint(mt->prop1, ==, 101);
>  g_assert_cmpuint(mt->prop2, ==, 102);
> -all_used = qdev_prop_check_globals();
> -g_assert_cmpuint(all_used, ==, 1);
> +global_error = qdev_prop_check_globals();
> +g_assert_cmpuint(global_error, ==, 1);
>  g_assert(props[0].used);
>  g_assert(props[1].used);
>  g_assert(!props[2].used);
> @@ -259,7 +259,7 @@ static void 
> test_dynamic_globalprop_nouser_subprocess(void)
>  { TYPE_NONDEVICE, "prop6", "106" },
>  {}
>  };
> -int all_used;
> +int global_error;
>  
>  qdev_prop_register_global_list(props);
>  
> @@ -268,8 +268,8 @@ static void 
> test_dynamic_globalprop_nouser_subprocess(void)
>  
>  g_assert_cmpuint(mt->prop1, ==, 101);
>  g_assert_cmpuint(mt->prop2, ==, 102);
> -all_used = qdev_prop_check_globals();
> -g_assert_cmpuint(all_used, ==, 0);
> +global_error = qdev_prop_check_globals();
> +g_assert_cmpuint(global_error, ==, 0);
>  g_assert(props[0].used);
>  g_assert(props[1].used);
>  g_assert(!props[2].used);


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition

[Igor Mammedov]

On Mon, 19 Nov 2018 18:14:26 +0100
Paolo Bonzini  wrote:

> On 19/11/18 16:31, Igor Mammedov wrote:
> > I've tried to give suggestions how to restructure series
> > on per patch basis. In my opinion it quite possible to split
> > series in several smaller ones and it should really help with
> > making series cleaner and easier/faster to review/amend/merge
> > vs what we have in v5.  
> 
> This is true, on the other hand the series makes sense together and,
> even if the patches are more or less independent, they also all follow
> the same "plan".  For reviewing v6, are you aware of Patchew's series
> diff functionality?  It can tell you which patches had comments in v5,
> reorder patches if applicable, and display deleted and new patches at
> the right point in the series.
Thanks, I'll give it a try.

Suggestion to split series mostly comes from contributor's point of view,
it much easier to amend small series than a larger one.


> v4->v5 is a bit messed up because Samuel probably added a diff order
> setup
> (https://patchew.org/QEMU/20181101102303.16439-1-sa...@linux.intel.com/diff/20181105014047.26447-1-sa...@linux.intel.com/)
> but it's very useful in general.
> Paolo


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 19/24] hw: acpi: Retrieve the PCI bus from AcpiPciHpState

[Igor Mammedov]

On Mon, 19 Nov 2018 18:02:53 +
"Boeuf, Sebastien"  wrote:

> On Mon, 2018-11-19 at 16:37 +0100, Igor Mammedov wrote:
> > On Fri, 16 Nov 2018 19:42:08 +
> > "Boeuf, Sebastien"  wrote:
> >   
> > > 
> > > Hi Igor,
> > > 
> > > On Fri, 2018-11-16 at 10:39 +0100, Igor Mammedov wrote:  
> > > > 
> > > > On Mon,  5 Nov 2018 02:40:42 +0100
> > > > Samuel Ortiz  wrote:
> > > >     
> > > > > 
> > > > > 
> > > > > From: Sebastien Boeuf 
> > > > > 
> > > > > Instead of using the machine type specific method find_i440fx()
> > > > > to
> > > > > retrieve the PCI bus, this commit aims to rely on the fact that
> > > > > the
> > > > > PCI bus is known by the structure AcpiPciHpState.
> > > > > 
> > > > > When the structure is initialized through acpi_pcihp_init()
> > > > > call,
> > > > > it saves the PCI bus, which means there is no need to invoke a
> > > > > special function later on.
> > > > > 
> > > > > Based on the fact that find_i440fx() was only used there, this
> > > > > patch also removes the function find_i440fx() itself from the
> > > > > entire codebase.
> > > > > 
> > > > > Reviewed-by: Philippe Mathieu-Daudé 
> > > > > Tested-by: Philippe Mathieu-Daudé 
> > > > > Signed-off-by: Sebastien Boeuf 
> > > > > Signed-off-by: Jing Liu     
> > > > Thanks for cleaning it up
> > > > 
> > > > minor nit:
> > > > Taking in account that you're removing '/* TODO: Q35 support */'
> > > > comment along with find_i440fx(), it might be worth to mention
> > > > in this commit message. Something along lines that ACPI PCIHP
> > > > exist to support guests without SHPC support on PCI
> > > > based PC machine. Considering that Q35 provides native
> > > > PCI-E hotplug, there is no need to add ACPI hotplug there.    
> > > Oh yes sure we can update the commit message :). But just wanted to
> > > mention that 'pc' machine type uses ACPI PCIHP and does support
> > > SHPC, so it's not mutually exclusive.  
> > it supports both but is it relevant to this patch?
> > 
> > Point was that one shouldn't remove something silently without
> > any justification/explanation. So that readers that come later
> > wouldn't wonder about the reasons why the code was removed.
> >   
> 
> I understand the point but I think the comment was wrong in the first
> place since q35 never tried to support ACPI PCIHP, as they support PCIe
> native hotplug as you mentioned.
ok.

when you have something ready, feel free to ping me
(I don't mind to review on github if that helps to speed up process)

> 
> >    
> > >   
> > > > 
> > > > 
> > > > with commit message fixed
> > > > 
> > > > Reviewed-by: Igor Mammedov 
> > > >     
> > > > > 
> > > > > 
> > > > > ---
> > > > >  include/hw/i386/pc.h  |  1 -
> > > > >  hw/acpi/pcihp.c   | 10 --
> > > > >  hw/pci-host/piix.c|  8 
> > > > >  stubs/pci-host-piix.c |  6 --
> > > > >  stubs/Makefile.objs   |  1 -
> > > > >  5 files changed, 4 insertions(+), 22 deletions(-)
> > > > >  delete mode 100644 stubs/pci-host-piix.c
> > > > > 
> > > > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > > > index 44cb6bf3f3..8e5f1464eb 100644
> > > > > --- a/include/hw/i386/pc.h
> > > > > +++ b/include/hw/i386/pc.h
> > > > > @@ -255,7 +255,6 @@ PCIBus *i440fx_init(const char *host_type,
> > > > > const char *pci_type,
> > > > >  MemoryRegion *pci_memory,
> > > > >  MemoryRegion *ram_memory);
> > > > >  
> > > > > -PCIBus *find_i440fx(void);
> > > > >  /* piix4.c */
> > > > >  extern PCIDevice *piix4_dev;
> > > > >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index 80d42e12ff..254b2e50ab 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -93,10 +93,9 @@

Re: [Xen-devel] [PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP

[Igor Mammedov]

On Mon, 19 Nov 2018 13:27:14 -0500
"Michael S. Tsirkin"  wrote:

> On Thu, Nov 08, 2018 at 03:16:23PM +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:28 +0100
> > Samuel Ortiz  wrote:
> >   
> > > XSDT is the 64-bit version of the legacy ACPI RSDT (Root System
> > > Description Table). RSDT only allow for 32-bit addressses and have thus
> > > been deprecated. Since ACPI version 2.0, RSDPs should point at XSDTs and
> > > no longer RSDTs, although RSDTs are still supported for backward
> > > compatibility.
> > > 
> > > Since version 2.0, RSDPs should add an extended checksum, a complete table
> > > length and a version field to the table.  
> > 
> > This patch re-implements what arm/virt board already does
> > and fixes checksum bug in the later and at the same time
> > without a user (within the patch).
> > 
> > I'd suggest redo it a way similar to FADT refactoring
> >   patch 1: fix checksum bug in virt/arm
> >   patch 2: update reference tables in test
> >   patch 3: introduce AcpiRsdpData similar to commit 937d1b587
> >  (both arm and x86) wich stores all data in hos byte order
> >   patch 4: convert arm's impl. to build_append_int_noprefix() API (commit 
> > 5d7a334f7)
> >... move out to aml-build.c
> >   patch 5: reuse generalized arm's build_rsdp() for x86, dropping x86 
> > specific one
> >   amending it to generate rev1 variant defined by revision in 
> > AcpiRsdpData
> >   (commit dd1b2037a)
> > 
> >   'make check V=1' shouldn't observe any ACPI tables changes after patch 2. 
> >  
> 
> And your next suggestion is to add patch 6.  I guess it's doable but
> this will make a single patch a 6 patch series. At this rate this series
> will be at 200 patches easily.
> 
> Automated checks are cool but hey it's easy to see what changed in a
> disassembled table, and we do not update them blindly. So just note in
> the comment that there's a table change for ARM and expected files need
> to be updated and we should be fine IMHO.
Point was to move patches that change tables content first,
where we would pay extra attentions to changes in tables and
then refactoring which shouldn't cause any changes will be mostly
automatic  (at least at that point we won't have to worry about 
tables correctness)


> > > Signed-off-by: Samuel Ortiz 
> > > ---
> > >  include/hw/acpi/aml-build.h |  3 +++
> > >  hw/acpi/aml-build.c | 37 +
> > >  2 files changed, 40 insertions(+)
> > > 
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index c9bcb32d81..3580d0ce90 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -393,6 +393,9 @@ void
> > >  build_rsdp(GArray *table_data,
> > > BIOSLinker *linker, unsigned rsdt_tbl_offset);
> > >  void
> > > +build_rsdp_xsdt(GArray *table_data,
> > > +BIOSLinker *linker, unsigned xsdt_tbl_offset);
> > > +void
> > >  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
> > > const char *oem_id, const char *oem_table_id);
> > >  void
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index 51b608432f..a030d40674 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1651,6 +1651,43 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, 
> > > GArray *table_offsets,
> > >   (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, 
> > > oem_table_id);
> > >  }
> > >  
> > > +/* RSDP pointing at an XSDT */
> > > +void
> > > +build_rsdp_xsdt(GArray *rsdp_table,
> > > +BIOSLinker *linker, unsigned xsdt_tbl_offset)
> > > +{
> > > +AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> > > +unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> > > +unsigned xsdt_pa_offset =
> > > +(char *)>xsdt_physical_address - rsdp_table->data;
> > > +unsigned xsdt_offset =
> > > +(char *)>length - rsdp_table->data;  
> 
> There's a cleaner way to get at the offsets than pointer math:
> 1. save rsdp_table length before you push
> 2. add offset_of for fields
> 
> If switching to build_append_int_noprefix then it's even
> easier - just save length before you append the int
> you intend to patch.
> 
> 
> &

Re: [Xen-devel] [PATCH v5 19/24] hw: acpi: Retrieve the PCI bus from AcpiPciHpState

[Igor Mammedov]

On Fri, 16 Nov 2018 19:42:08 +
"Boeuf, Sebastien"  wrote:

> Hi Igor,
> 
> On Fri, 2018-11-16 at 10:39 +0100, Igor Mammedov wrote:
> > On Mon,  5 Nov 2018 02:40:42 +0100
> > Samuel Ortiz  wrote:
> >   
> > > 
> > > From: Sebastien Boeuf 
> > > 
> > > Instead of using the machine type specific method find_i440fx() to
> > > retrieve the PCI bus, this commit aims to rely on the fact that the
> > > PCI bus is known by the structure AcpiPciHpState.
> > > 
> > > When the structure is initialized through acpi_pcihp_init() call,
> > > it saves the PCI bus, which means there is no need to invoke a
> > > special function later on.
> > > 
> > > Based on the fact that find_i440fx() was only used there, this
> > > patch also removes the function find_i440fx() itself from the
> > > entire codebase.
> > > 
> > > Reviewed-by: Philippe Mathieu-Daudé 
> > > Tested-by: Philippe Mathieu-Daudé 
> > > Signed-off-by: Sebastien Boeuf 
> > > Signed-off-by: Jing Liu   
> > Thanks for cleaning it up
> > 
> > minor nit:
> > Taking in account that you're removing '/* TODO: Q35 support */'
> > comment along with find_i440fx(), it might be worth to mention
> > in this commit message. Something along lines that ACPI PCIHP
> > exist to support guests without SHPC support on PCI
> > based PC machine. Considering that Q35 provides native
> > PCI-E hotplug, there is no need to add ACPI hotplug there.  
> 
> Oh yes sure we can update the commit message :). But just wanted to
> mention that 'pc' machine type uses ACPI PCIHP and does support
> SHPC, so it's not mutually exclusive.
it supports both but is it relevant to this patch?

Point was that one shouldn't remove something silently without
any justification/explanation. So that readers that come later
wouldn't wonder about the reasons why the code was removed.
 
> > 
> > with commit message fixed
> > 
> > Reviewed-by: Igor Mammedov 
> >   
> > > 
> > > ---
> > >  include/hw/i386/pc.h  |  1 -
> > >  hw/acpi/pcihp.c   | 10 --
> > >  hw/pci-host/piix.c|  8 
> > >  stubs/pci-host-piix.c |  6 --
> > >  stubs/Makefile.objs   |  1 -
> > >  5 files changed, 4 insertions(+), 22 deletions(-)
> > >  delete mode 100644 stubs/pci-host-piix.c
> > > 
> > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > index 44cb6bf3f3..8e5f1464eb 100644
> > > --- a/include/hw/i386/pc.h
> > > +++ b/include/hw/i386/pc.h
> > > @@ -255,7 +255,6 @@ PCIBus *i440fx_init(const char *host_type,
> > > const char *pci_type,
> > >  MemoryRegion *pci_memory,
> > >  MemoryRegion *ram_memory);
> > >  
> > > -PCIBus *find_i440fx(void);
> > >  /* piix4.c */
> > >  extern PCIDevice *piix4_dev;
> > >  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 80d42e12ff..254b2e50ab 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -93,10 +93,9 @@ static void *acpi_set_bsel(PCIBus *bus, void
> > > *opaque)
> > >  return bsel_alloc;
> > >  }
> > >  
> > > -static void acpi_set_pci_info(void)
> > > +static void acpi_set_pci_info(AcpiPciHpState *s)
> > >  {
> > >  static bool bsel_is_set;
> > > -PCIBus *bus;
> > >  unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
> > >  
> > >  if (bsel_is_set) {
> > > @@ -104,10 +103,9 @@ static void acpi_set_pci_info(void)
> > >  }
> > >  bsel_is_set = true;
> > >  
> > > -bus = find_i440fx(); /* TODO: Q35 support */
> > > -if (bus) {
> > > +if (s->root) {
> > >  /* Scan all PCI buses. Set property to enable acpi based
> > > hotplug. */
> > > -pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL,
> > > _alloc);
> > > +pci_for_each_bus_depth_first(s->root, acpi_set_bsel, NULL,
> > > _alloc);
> > >  }
> > >  }
> > >  
> > > @@ -213,7 +211,7 @@ static void acpi_pcihp_update(AcpiPciHpState
> > > *s)
> > >  
> > >  void acpi_pcihp_reset(AcpiPciHpState *s)
> > >  {
> > > -acpi_set_pci_info();
> > > +acpi_set_pci_info(s);
> > >  acpi_pcihp_update(s);
> > >  }
> > >  
> &

Re: [Xen-devel] [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition

[Igor Mammedov]

On Fri, 16 Nov 2018 17:37:54 +0100
Paolo Bonzini  wrote:

> On 16/11/18 17:29, Igor Mammedov wrote:
> > General suggestions for this series:
> >   1. Preferably don't do multiple changes within a patch
> >  neither post huge patches (unless it's pure code movement).
> >  (it's easy to squash patches later it necessary)
> >   2. Start small, pick a table generalize it and send as
> >  one small patchset. Tables are often independent
> >  and it's much easier on both author/reviewer to agree upon
> >  changes and rewrite it if necessary.  
> 
> How would that be done?  This series is on the bigger side, agreed, but
> most of it is really just code movement.  It's a starting point, having
> a generic ACPI library is way beyond what this is trying to do.
I've tried to give suggestions how to restructure series
on per patch basis. In my opinion it quite possible to split
series in several smaller ones and it should really help with
making series cleaner and easier/faster to review/amend/merge
vs what we have in v5.
(it's more frustrating to rework large series vs smaller one)

If something isn't clear, it's easy to reach out to me here
or directly (email/irc/github) for clarification/feed back.

> 
> Paolo
> 
> >   3. when you think about refactoring acpi into a generic API
> >  think about it as routines that go into a separate library
> >  (pure acpi spec code) and qemu/acpi glue routines and
> >   divide them correspondingly.  
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:23 +0100
Samuel Ortiz  wrote:

> This patch set provides an ACPI code reorganization in preparation for
> adding a shared hardware-reduced ACPI API to QEMU.
> 
> The changes are coming from the NEMU [1] project where we're defining
> a new x86 machine type: i386/virt. This is an EFI only, ACPI
> hardware-reduced platform that is built on top of a generic
> hardware-reduced ACPI API [2]. This API was initially based off the
> generic parts of the arm/virt-acpi-build.c implementation, and the goal
> is for both i386/virt and arm/virt to duplicate as little code as
> possible by using this new, shared API.
> 
> As a preliminary for adding this hardware-reduced ACPI API to QEMU, we did
> some ACPI code reorganization with the following goals:
> 
> * Share as much as possible of the current ACPI build APIs between
>   legacy and hardware-reduced ACPI.
> * Share the ACPI build code across machine types and architectures and
>   remove the typical PC machine type dependency.
> 
> The patches are also available in their own git branch [3].
I think, I'm done with reviewing this patchset, to sum up
thanks for trying generalize acpi parts. It is implemented not
exactly generic way and patches aren't split perfectly but
we can work on it.

General suggestions for this series:
  1. Preferably don't do multiple changes within a patch
 neither post huge patches (unless it's pure code movement).
 (it's easy to squash patches later it necessary)
  2. Start small, pick a table generalize it and send as
 one small patchset. Tables are often independent
 and it's much easier on both author/reviewer to agree upon
 changes and rewrite it if necessary.
  3. when you think about refactoring acpi into a generic API
 think about it as routines that go into a separate library
 (pure acpi spec code) and qemu/acpi glue routines and
  divide them correspondingly.

> [1] https://github.com/intel/nemu
> [2] https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c
> [3] https://github.com/intel/nemu/tree/topic/upstream/acpi
> 
> v1 -> v2:
>* Drop the hardware-reduced implementation for now. Our next patch
>* set
>  will add hardware-reduced and convert arm/virt to it.
>* Implement the ACPI build methods as a QOM Interface Class and
>* convert
>  the PC machine type to it.
>* acpi_conf_pc_init() uses a PCMachineState pointer and not a
>  MachineState one as its argument.
> 
> v2 -> v3:
>* Cc all relevant maintainers, no functional changes.
> 
> v3 -> v4:
>* Renamed all AcpiConfiguration pointers from conf to acpi_conf.
>* Removed the ACPI_BUILD_ALIGN_SIZE export.
>* Temporarily updated the arm virt build_rsdp() prototype for
>  bisectability purposes.
>* Removed unneeded pci headers from acpi-build.c.
>* Refactor the acpi PCI host getter so that it truly is architecture
>  agnostic, by carrying the PCI host pointer through the
>  AcpiConfiguration structure.
>* Splitted the PCI host AML builder API export patch from the PCI
>  host and holes getter one.
>* Reduced the build_srat() export scope to hw/i386 instead of the
>  broader hw/acpi. SRAT builders are truly architecture specific
>  and can hardly be generalized.
>* Completed the ACPI builder documentation.
> 
> v4 -> v5:
>* Reorganize the ACPI RSDP export and XSDT implementation into 3
>  patches.
>* Fix the hw/i386/acpi header inclusions.
> 
> Samuel Ortiz (16):
>   hw: i386: Decouple the ACPI build from the PC machine type
>   hw: acpi: Export ACPI build alignment API
>   hw: acpi: The RSDP build API can return void
>   hw: acpi: Export the RSDP build API
>   hw: acpi: Implement XSDT support for RSDP
>   hw: acpi: Factorize the RSDP build API implementation
>   hw: i386: Move PCI host definitions to pci_host.h
>   hw: acpi: Export the PCI host and holes getters
>   hw: acpi: Do not create hotplug method when handler is not defined
>   hw: i386: Make the hotpluggable memory size property more generic
>   hw: i386: Export the i386 ACPI SRAT build method
>   hw: i386: Export the MADT build method
>   hw: acpi: Define ACPI tables builder interface
>   hw: i386: Implement the ACPI builder interface for PC
>   hw: pci-host: piix: Return PCI host pointer instead of PCI bus
>   hw: i386: Set ACPI configuration PCI host pointer
> 
> Sebastien Boeuf (2):
>   hw: acpi: Export the PCI hotplug API
>   hw: acpi: Retrieve the PCI bus from AcpiPciHpState
> 
> Yang Zhong (6):
>   hw: acpi: Generalize AML build routines
>   hw: acpi: Factorize _OSC AML across architectures
>   hw: acpi: Export and generalize the PCI host AML API
>   hw: acpi: Export the MCFG getter
>   hw: acpi: Fix memory hotplug AML generation error
>   hw: i386: Refactor PCI host getter
> 
>  hw/i386/acpi-build.h   |9 +-
>  include/hw/acpi/acpi-defs.h|   14 +
>  include/hw/acpi/acpi.h |   44 ++
>  include/hw/acpi/aml-build.h|   47 

Re: [Xen-devel] [Qemu-devel] [PATCH v5 20/24] hw: acpi: Define ACPI tables builder interface

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:43 +0100
Samuel Ortiz  wrote:

> In order to decouple ACPI APIs from specific machine types, we are
> creating an ACPI builder interface that each ACPI platform can choose to
> implement.
> This way, a new machine type can re-use the high level ACPI APIs and
> define some custom table build methods, without having to duplicate most
> of the existing implementation only to add small variations to it.
I'm not sure about motivation behind so high APIs,
what obvious here is an extra level of indirection for not clear gain.

Yep using table callbacks, one can attempt to generalize
acpi_setup() and help boards to decide which tables do not build
(MCFG comes to the mind). But I'm not convinced that acpi_setup()
could be cleanly generalized as a whole (probably some parts but
not everything) so it's minor benefit for extra headache of
figuring out what callback will be actually called when reading code.

However if board needs a slightly different table, it will have to
duplicate an exiting one and then modify to suit its needs.

to me it pretty much looks the same as calling build_foo()
we use now but with an extra indirection level and then
duplicating the later for usage in another board in slightly
different manner.

I agree with Paolo's suggestion to use interfaces for generalization,
however I'd suggest a fine grained approach for providing board/target
specific items/actions for generic tables.

For example take a look at AcpiDeviceIfClass interface that is
implemented by GPE devices and its madt_cpu() method. That should
simplify generalizing cpu hotplug for arm/virt and also help
to generalize build_madt(). It's not cleanest impl. by far but
headed in the right generic direction. I have it on my TODO list to
do list to generalize acpi parts of build_fadt()/cpu hotplug
some day as part of the project that adds cpu hotplug to arm/virt board.

GPE/GED device might be not ideal place to implement that interface
(worked for pc/q35) and may be we should move it to machine level
as board has access to much more data for building ACPI tables.
For i386/virt, I'd extend/modify AcpiDeviceIfClass when/where it's
necessary instead of adding high level table hooks.
That way generic build_foo() API's would share much more code.

> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/acpi/builder.h | 100 ++
>  hw/acpi/builder.c |  97 
>  hw/acpi/Makefile.objs |   1 +
>  3 files changed, 198 insertions(+)
>  create mode 100644 include/hw/acpi/builder.h
>  create mode 100644 hw/acpi/builder.c
> 
> diff --git a/include/hw/acpi/builder.h b/include/hw/acpi/builder.h
> new file mode 100644
> index 00..a63b88ffe9
> --- /dev/null
> +++ b/include/hw/acpi/builder.h
> @@ -0,0 +1,100 @@
> +/*
> + *
> + * Copyright (c) 2018 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see .
> + */
> +
> +#ifndef ACPI_BUILDER_H
> +#define ACPI_BUILDER_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "qom/object.h"
> +
> +#define TYPE_ACPI_BUILDER "acpi-builder"
> +
> +#define ACPI_BUILDER_METHODS(klass) \
> + OBJECT_CLASS_CHECK(AcpiBuilderMethods, (klass), TYPE_ACPI_BUILDER)
> +#define ACPI_BUILDER_GET_METHODS(obj) \
> + OBJECT_GET_CLASS(AcpiBuilderMethods, (obj), TYPE_ACPI_BUILDER)
> +#define ACPI_BUILDER(obj)   \
> + INTERFACE_CHECK(AcpiBuilder, (obj), TYPE_ACPI_BUILDER)
> +
> +typedef struct AcpiConfiguration AcpiConfiguration;
> +typedef struct AcpiBuildState AcpiBuildState;
> +typedef struct AcpiMcfgInfo AcpiMcfgInfo;
> +
> +typedef struct AcpiBuilder {
> +/*  */
> +Object Parent;
> +} AcpiBuilder;
> +
> +/**
> + * AcpiBuildMethods:
> + *
> + * Interface to be implemented by a machine type that needs to provide
> + * custom ACPI tables build method.
> + *
> + * @parent: Opaque parent interface.
> + * @rsdp: ACPI RSDP (Root System Description Pointer) table build callback.
> + * @madt: ACPI MADT (Multiple APIC Description Table) table build callback.
> + * @mcfg: ACPI MCFG table build callback.
> + * @srat: ACPI SRAT (System/Static Resource Affinity Table)
> + *table build callback.
> + * @slit: ACPI SLIT (System Locality System Information Table)
> + *

Re: [Xen-devel] [PATCH v5 22/24] hw: pci-host: piix: Return PCI host pointer instead of PCI bus

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:45 +0100
Samuel Ortiz  wrote:

All remaining patches a bit out of proper order,
they should be around patch 12/24 where you started to touch MCFG code.

> For building the MCFG table, we need to track a given machine
> type PCI host pointer, and we can't get it from the bus pointer alone.
> As piix returns a PCI bus pointer, we simply modify its builder to
> return a PCI host pointer instead.
PC machine doesn't build MCFG so we don't really need it to provide
this pointer, having this patch doesn't hurt but I'm not sure we
need it.

CCing ARM folks since we are talking about generalizing MCFG table generation.

we have following invariants wrt using MCFG:

   pc [pci_host != NULL] -> bail out early + do not build MCFG

   pc [pci_host == NULL] -> would explode if not only for [has_acpi_build = 
false] guard
should be: do not even call acpi_get_mcfg().

   q35 [pci_host == NULL] -> not valid combo and must assert

   q35 [pci_host != NULL && mcfg_base != PCIE_BASE_ADDR_UNMAPPED]
generate MCFG using mcfg_base/size

   q35 [pci_host != NULL && mcfg_base == PCIE_BASE_ADDR_UNMAPPED]
generate place-holder 'QEMU' table for legacy machine versions without
resizable MemoryRegion support.
Mapped/not mapped could be dynamic accross reboots, so we need
access to PCIE(pci_host) to fetch current values.

   arm/virt gpex [memmap[ecam_id].base/size] do build MCFG
hacked up variant that doesn't use pci_host mcfg_base/size fields
not sure if it's possible to disable ecam as on q35 (does it need any 
fixing?)
we should fix arm/virt to use pci-host mcfg_base/size so we
could reuse properties PCIE_HOST_MCFG_BASE/PCIE_HOST_MCFG_SIZE
on ARM and generic build_mcfg()

So we have quite a mess here, the current acpi_get_mcfg() does 2 things
  1. indirectly checks that pci_host is PCI-E (presence of PCIE_HOST_MCFG_BASE 
property)
  2. fetches mcfg_base/size if it's PCI-E host

and i386/build_mcfg() is called only when #1 is true

As far as see we use pci_host only to fetch mcfg_base/size and not for anything
else.
Maybe as refactoring plan we should"
 * pass to acpi_setup(PCIHostState* pcie_host) as an argument pcie host pointer,
   which for PC will be NULL and for the rest set it to q35/gxpe/... PCI-E host.
 * call build_mcfg() if pcie_host != NULL
   (no more indirect guessing using PCIE_HOST_MCFG_BASE property presence)
 * move MCFG/QEMU table signature decision out of build_mcfg() and pass
   it as argument 'build_mcfg(...,char *mcfg_signature)'. It moves out masking
   table quirk into caller, where q35 can decide to change signature
   if ECAM is not mapped. The rest (arm|i386/virt) will always pass 'MCFG'.
   Or even better if ecam is mapped, create MCFG (with masking trick if q35
   machine is old and do not support resizable MemoryRegions).

> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/i386/pc.h | 21 +++--
>  hw/i386/pc_piix.c| 18 +++---
>  hw/pci-host/piix.c   | 24 
>  3 files changed, 34 insertions(+), 29 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 8e5f1464eb..b6b79e146d 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -244,16 +244,17 @@ typedef struct PCII440FXState PCII440FXState;
>   */
>  #define RCR_IOPORT 0xcf9
>  
> -PCIBus *i440fx_init(const char *host_type, const char *pci_type,
> -PCII440FXState **pi440fx_state, int *piix_devfn,
> -ISABus **isa_bus, qemu_irq *pic,
> -MemoryRegion *address_space_mem,
> -MemoryRegion *address_space_io,
> -ram_addr_t ram_size,
> -ram_addr_t below_4g_mem_size,
> -ram_addr_t above_4g_mem_size,
> -MemoryRegion *pci_memory,
> -MemoryRegion *ram_memory);
> +struct PCIHostState *i440fx_init(const char *host_type, const char *pci_type,
> + PCII440FXState **pi440fx_state,
> + int *piix_devfn,
> + ISABus **isa_bus, qemu_irq *pic,
> + MemoryRegion *address_space_mem,
> + MemoryRegion *address_space_io,
> + ram_addr_t ram_size,
> + ram_addr_t below_4g_mem_size,
> + ram_addr_t above_4g_mem_size,
> + MemoryRegion *pci_memory,
> + MemoryRegion *ram_memory);
>  
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 0620d10715..f5b139a3eb 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -32,6 +32,7 @@
>  #include "hw/display/ramfb.h"
>  #include "hw/smbios/smbios.h"
>  #include 

Re: [Xen-devel] [PATCH v5 19/24] hw: acpi: Retrieve the PCI bus from AcpiPciHpState

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:42 +0100
Samuel Ortiz  wrote:

> From: Sebastien Boeuf 
> 
> Instead of using the machine type specific method find_i440fx() to
> retrieve the PCI bus, this commit aims to rely on the fact that the
> PCI bus is known by the structure AcpiPciHpState.
> 
> When the structure is initialized through acpi_pcihp_init() call,
> it saves the PCI bus, which means there is no need to invoke a
> special function later on.
> 
> Based on the fact that find_i440fx() was only used there, this
> patch also removes the function find_i440fx() itself from the
> entire codebase.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Sebastien Boeuf 
> Signed-off-by: Jing Liu 
Thanks for cleaning it up

minor nit:
Taking in account that you're removing '/* TODO: Q35 support */'
comment along with find_i440fx(), it might be worth to mention
in this commit message. Something along lines that ACPI PCIHP
exist to support guests without SHPC support on PCI
based PC machine. Considering that Q35 provides native
PCI-E hotplug, there is no need to add ACPI hotplug there.


with commit message fixed

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/i386/pc.h  |  1 -
>  hw/acpi/pcihp.c   | 10 --
>  hw/pci-host/piix.c|  8 
>  stubs/pci-host-piix.c |  6 --
>  stubs/Makefile.objs   |  1 -
>  5 files changed, 4 insertions(+), 22 deletions(-)
>  delete mode 100644 stubs/pci-host-piix.c
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 44cb6bf3f3..8e5f1464eb 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -255,7 +255,6 @@ PCIBus *i440fx_init(const char *host_type, const char 
> *pci_type,
>  MemoryRegion *pci_memory,
>  MemoryRegion *ram_memory);
>  
> -PCIBus *find_i440fx(void);
>  /* piix4.c */
>  extern PCIDevice *piix4_dev;
>  int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn);
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 80d42e12ff..254b2e50ab 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -93,10 +93,9 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>  return bsel_alloc;
>  }
>  
> -static void acpi_set_pci_info(void)
> +static void acpi_set_pci_info(AcpiPciHpState *s)
>  {
>  static bool bsel_is_set;
> -PCIBus *bus;
>  unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>  
>  if (bsel_is_set) {
> @@ -104,10 +103,9 @@ static void acpi_set_pci_info(void)
>  }
>  bsel_is_set = true;
>  
> -bus = find_i440fx(); /* TODO: Q35 support */
> -if (bus) {
> +if (s->root) {
>  /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> -pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, _alloc);
> +pci_for_each_bus_depth_first(s->root, acpi_set_bsel, NULL, 
> _alloc);
>  }
>  }
>  
> @@ -213,7 +211,7 @@ static void acpi_pcihp_update(AcpiPciHpState *s)
>  
>  void acpi_pcihp_reset(AcpiPciHpState *s)
>  {
> -acpi_set_pci_info();
> +acpi_set_pci_info(s);
>  acpi_pcihp_update(s);
>  }
>  
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 47293a3915..658460264b 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -445,14 +445,6 @@ PCIBus *i440fx_init(const char *host_type, const char 
> *pci_type,
>  return b;
>  }
>  
> -PCIBus *find_i440fx(void)
> -{
> -PCIHostState *s = OBJECT_CHECK(PCIHostState,
> -   object_resolve_path("/machine/i440fx", 
> NULL),
> -   TYPE_PCI_HOST_BRIDGE);
> -return s ? s->bus : NULL;
> -}
> -
>  /* PIIX3 PCI to ISA bridge */
>  static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>  {
> diff --git a/stubs/pci-host-piix.c b/stubs/pci-host-piix.c
> deleted file mode 100644
> index 6ed81b1f21..00
> --- a/stubs/pci-host-piix.c
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -#include "qemu/osdep.h"
> -#include "hw/i386/pc.h"
> -PCIBus *find_i440fx(void)
> -{
> -return NULL;
> -}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 5dd0aeeec6..725f78bedc 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -41,6 +41,5 @@ stub-obj-y += pc_madt_cpu_entry.o
>  stub-obj-y += vmgenid.o
>  stub-obj-y += xen-common.o
>  stub-obj-y += xen-hvm.o
> -stub-obj-y += pci-host-piix.o
>  stub-obj-y += ram-block.o
>  stub-obj-y += ramfb.o


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 18/24] hw: i386: Export the MADT build method

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:41 +0100
Samuel Ortiz  wrote:

> It is going to be used by the PC machine type as the MADT table builder
> method and thus needs to be exported outside of acpi-build.c
> 
> Also, now that the generic build_madt() API is exported, we have to
> rename the ARM static one in order to avoid build time conflicts.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/i386/acpi.h   | 28 
>  hw/arm/virt-acpi-build.c |  4 ++--
>  hw/i386/acpi-build.c |  4 ++--
>  3 files changed, 32 insertions(+), 4 deletions(-)
>  create mode 100644 include/hw/i386/acpi.h
> 
> diff --git a/include/hw/i386/acpi.h b/include/hw/i386/acpi.h
> new file mode 100644
> index 00..b7a887111d
> --- /dev/null
> +++ b/include/hw/i386/acpi.h
[...]

> +/* ACPI MADT (Multiple APIC Description Table) build method */
> +void build_madt(GArray *table_data, BIOSLinker *linker,
> +MachineState *ms, AcpiConfiguration *conf);
> +
> +#endif
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b5e165543a..b0354c5f03 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -564,7 +564,7 @@ build_gtdt(GArray *table_data, BIOSLinker *linker, 
> VirtMachineState *vms)
>  
>  /* MADT */
>  static void
> -build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +virt_build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState 
> *vms)
>  {
You are moving build_madt() into x86 specific header i386/acpi.h
so question is why do you touch ARM variant at all?

[...]

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 15/24] hw: i386: Export the i386 ACPI SRAT build method

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:38 +0100
Samuel Ortiz  wrote:

> This is the standard way of building SRAT on x86 platfoms. But future
> machine types could decide to define their own custom SRAT build method
> through the ACPI builder methods.
> Moreover, we will also need to reach build_srat() from outside of
> acpi-build in order to use it as the ACPI builder SRAT build method.
SRAT is usually highly machine specific (memory holes, layout, guest OS
specific quirks) so it's hard to generalize it.

I'd  drop SRAT related patches from this series and introduce
i386/virt specific SRAT when you post patches for it.

What we could generalize here is building blocks used to
create entries into acpi/aml-build.c
   build_srat_memory -> build_srat_memory_entry()
   build_apic_entry()
   build_x2apic_entry()
and please switch these blocks to build_append_int_noprefix() API
before moving them to acpi/aml-build.c

> Signed-off-by: Samuel Ortiz 
> ---
>  hw/i386/acpi-build.h | 5 +
>  hw/i386/acpi-build.c | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 065a1d8250..d73c41fe8f 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -4,6 +4,11 @@
>  
>  #include "hw/acpi/acpi.h"
>  
> +/* ACPI SRAT (Static Resource Affinity Table) build method for x86 */
> +void
> +build_srat(GArray *table_data, BIOSLinker *linker,
> +   MachineState *machine, AcpiConfiguration *acpi_conf);
> +
>  void acpi_setup(MachineState *machine, AcpiConfiguration *acpi_conf);
>  
>  #endif
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1ef1a38441..673c5dfafc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1615,7 +1615,7 @@ build_tpm2(GArray *table_data, BIOSLinker *linker, 
> GArray *tcpalog)
>  #define HOLE_640K_START  (640 * KiB)
>  #define HOLE_640K_END   (1 * MiB)
>  
> -static void
> +void
>  build_srat(GArray *table_data, BIOSLinker *linker,
> MachineState *machine, AcpiConfiguration *acpi_conf)
>  {


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 14/24] hw: i386: Make the hotpluggable memory size property more generic

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:37 +0100
Samuel Ortiz  wrote:

> This property is currently defined under i386/pc while it only describes
> a region size that's eventually fetched from the AML ACPI code.
> 
> We can make it more generic and shareable across machine types by moving
> it to memory-device.h instead.
> 
> Signed-off-by: Samuel Ortiz 
not sure it belong to this series, but regardless where it end-ups

Reviewed-by: Igor Mammedov 

> ---
>  include/hw/i386/pc.h   | 1 -
>  include/hw/mem/memory-device.h | 2 ++
>  hw/i386/acpi-build.c   | 2 +-
>  hw/i386/pc.c   | 3 ++-
>  4 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index bbbdb33ea3..44cb6bf3f3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -62,7 +62,6 @@ struct PCMachineState {
>  };
>  
>  #define PC_MACHINE_ACPI_DEVICE_PROP "acpi-device"
> -#define PC_MACHINE_DEVMEM_REGION_SIZE "device-memory-region-size"
>  #define PC_MACHINE_MAX_RAM_BELOW_4G "max-ram-below-4g"
>  #define PC_MACHINE_VMPORT   "vmport"
>  #define PC_MACHINE_SMM  "smm"
> diff --git a/include/hw/mem/memory-device.h b/include/hw/mem/memory-device.h
> index e904e194d5..d9a4fc7c3e 100644
> --- a/include/hw/mem/memory-device.h
> +++ b/include/hw/mem/memory-device.h
> @@ -97,6 +97,8 @@ typedef struct MemoryDeviceClass {
>   MemoryDeviceInfo *info);
>  } MemoryDeviceClass;
>  
> +#define MEMORY_DEVICE_REGION_SIZE "memory-device-region-size"
> +
>  MemoryDeviceInfoList *qmp_memory_device_list(void);
>  uint64_t get_plugged_memory_size(void);
>  void memory_device_pre_plug(MemoryDeviceState *md, MachineState *ms,
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d8bba16776..1ef1a38441 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1628,7 +1628,7 @@ build_srat(GArray *table_data, BIOSLinker *linker,
>  MachineClass *mc = MACHINE_GET_CLASS(machine);
>  const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
>  ram_addr_t hotplugabble_address_space_size =
> -object_property_get_int(OBJECT(machine), 
> PC_MACHINE_DEVMEM_REGION_SIZE,
> +object_property_get_int(OBJECT(machine), MEMORY_DEVICE_REGION_SIZE,
>  NULL);
>  
>  srat_start = table_data->len;
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 090f969933..c9ffc8cff6 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -67,6 +67,7 @@
>  #include "hw/boards.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/mem/memory-device.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-visit-common.h"
>  #include "qapi/visitor.h"
> @@ -2443,7 +2444,7 @@ static void pc_machine_class_init(ObjectClass *oc, void 
> *data)
>  nc->nmi_monitor_handler = x86_nmi;
>  mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE;
>  
> -object_class_property_add(oc, PC_MACHINE_DEVMEM_REGION_SIZE, "int",
> +object_class_property_add(oc, MEMORY_DEVICE_REGION_SIZE, "int",
>  pc_machine_get_device_memory_region_size, NULL,
>  NULL, NULL, _abort);
>  


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 12/24] hw: acpi: Export the MCFG getter

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:35 +0100
Samuel Ortiz  wrote:

> From: Yang Zhong 
> 
> The ACPI MCFG getter is not x86 specific and could be called from
> anywhere within generic ACPI API, so let's export it.
So far it's x86 or more exactly q35 specific thing,
for example it won't work with arm/virt without refactoring
the later.

We probably over-engineered it and could get by without
properties here at all, but that's not related to this series.

So for now I'd suggest to move it to x86/acpi-pci.c for now
and we can generalize the way we get address/size later,
but if you feel like replacing all this property complications
and making it simpler go ahead.


 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Yang Zhong 
> ---
>  include/hw/acpi/aml-build.h |  1 +
>  hw/acpi/aml-build.c | 24 
>  hw/i386/acpi-build.c| 22 --
>  3 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1861e37ebf..64ea371656 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -408,6 +408,7 @@ void *acpi_data_push(GArray *table_data, unsigned size);
>  unsigned acpi_data_len(GArray *table);
>  Object *acpi_get_pci_host(void);
>  void acpi_get_pci_holes(Range *hole, Range *hole64);
> +bool acpi_get_mcfg(AcpiMcfgInfo *mcfg);
>  /* Align AML blob size to a multiple of 'align' */
>  void acpi_align_size(GArray *blob, unsigned align);
>  void acpi_add_table(GArray *table_offsets, GArray *table_data);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 869ed70db3..2c5446ab23 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -32,6 +32,8 @@
>  #include "hw/i386/pc.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "qom/qom-qobject.h"
> +#include "qapi/qmp/qnum.h"
>  
>  #define PCI_HOST_BRIDGE_CONFIG_ADDR0xcf8
>  #define PCI_HOST_BRIDGE_IO_0_MIN_ADDR  0x
> @@ -1657,6 +1659,28 @@ void acpi_get_pci_holes(Range *hole, Range *hole64)
> NULL));
>  }
>  
> +bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> +{
> +Object *pci_host;
> +QObject *o;
> +
> +pci_host = acpi_get_pci_host();

it would be better to get bus from struct AcpiPciBus
instead of doing lookup again. (that's a separate patch from moving function 
around)

> +g_assert(pci_host);
> +
> +o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
> +if (!o) {
> +return false;
> +}
> +mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
> +qobject_unref(o);
> +
> +o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
> +assert(o);
> +mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o));
> +qobject_unref(o);
> +return true;
> +}
> +
>  static void crs_range_insert(GPtrArray *ranges, uint64_t base, uint64_t 
> limit)
>  {
>  CrsRangeEntry *entry;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 14e2624d14..d8bba16776 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1856,28 +1856,6 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>  }
>  
> -static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
> -{
> -Object *pci_host;
> -QObject *o;
> -
> -pci_host = acpi_get_pci_host();
> -g_assert(pci_host);
> -
> -o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
> -if (!o) {
> -return false;
> -}
> -mcfg->mcfg_base = qnum_get_uint(qobject_to(QNum, o));
> -qobject_unref(o);
> -
> -o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_SIZE, NULL);
> -assert(o);
> -mcfg->mcfg_size = qnum_get_uint(qobject_to(QNum, o));
> -qobject_unref(o);
> -return true;
> -}
> -
>  static
>  void acpi_build(AcpiBuildTables *tables,
>  MachineState *machine, AcpiConfiguration *acpi_conf)


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 11/24] hw: acpi: Export and generalize the PCI host AML API

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:34 +0100
Samuel Ortiz  wrote:

> From: Yang Zhong 
> 
> The AML build routines for the PCI host bridge and the corresponding
> DSDT addition are neither x86 nor PC machine type specific.
> We can move them to the architecture agnostic hw/acpi folder, and by
> carrying all the needed information through a new AcpiPciBus structure,
> we can make them PC machine type independent.

I'm don't know anything about PCI, but functional changes doesn't look
correct to me. See more detailed comments below.

Marcel,
could you take a look on this patch (in particular main csr changes), pls?

> 
> Signed-off-by: Yang Zhong 
> Signed-off-by: Rob Bradford 
> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/acpi/aml-build.h |   8 ++
>  hw/acpi/aml-build.c | 157 
>  hw/i386/acpi-build.c| 115 ++
>  3 files changed, 173 insertions(+), 107 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index fde2785b9a..1861e37ebf 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -229,6 +229,12 @@ typedef struct AcpiMcfgInfo {
>  uint32_t mcfg_size;
>  } AcpiMcfgInfo;
>  
> +typedef struct AcpiPciBus {
> +PCIBus *pci_bus;
> +Range *pci_hole;
> +Range *pci_hole64;
> +} AcpiPciBus;
Again, this and all below is not aml-build material.
Consider adding/using pci specific acpi file for it.

Also even though pci AML in arm/virt is to a large degree a subset
of x86 target and it would be much better to unify ARM part with x86,
it probably will be to big/complex of a change if we take on it in
one go.

So not to derail you from the goal too much, we probably should
generalize this a little bit less, limiting refactoring to x86
target for now.

For example, move generic x86 pci parts to hw/i386/acpi-pci.[hc],
and structure it so that building blocks in acpi-pci.c could be
reused for x86 reduced profile later.
Once it's been done, it might be easier and less complex to
unify a bit more generic code in i386/acpi-pci.c with corresponding
ARM code.

Patch is too big and should be split into smaller logical chunks
and you should separate code movement vs functional changes you're
a making here.

Once you split patch properly, it should be easier to assess
changes.

>  typedef struct CrsRangeEntry {
>  uint64_t base;
>  uint64_t limit;
> @@ -411,6 +417,8 @@ Aml *build_osc_method(uint32_t value);
>  void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info);
>  Aml *build_gsi_link_dev(const char *name, uint8_t uid, uint8_t gsi);
>  Aml *build_prt(bool is_pci0_prt);
> +void acpi_dsdt_add_pci_bus(Aml *dsdt, AcpiPciBus *pci_host);
> +Aml *build_pci_host_bridge(Aml *table, AcpiPciBus *pci_host);
>  void crs_range_set_init(CrsRangeSet *range_set);
>  Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set);
>  void crs_replace_with_free_ranges(GPtrArray *ranges,
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b8e32f15f7..869ed70db3 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -29,6 +29,19 @@
>  #include "hw/pci/pci_bus.h"
>  #include "qemu/range.h"
>  #include "hw/pci/pci_bridge.h"
> +#include "hw/i386/pc.h"
> +#include "sysemu/tpm.h"
> +#include "hw/acpi/tpm.h"
> +
> +#define PCI_HOST_BRIDGE_CONFIG_ADDR0xcf8
> +#define PCI_HOST_BRIDGE_IO_0_MIN_ADDR  0x
> +#define PCI_HOST_BRIDGE_IO_0_MAX_ADDR  0x0cf7
> +#define PCI_HOST_BRIDGE_IO_1_MIN_ADDR  0x0d00
> +#define PCI_HOST_BRIDGE_IO_1_MAX_ADDR  0x
> +#define PCI_VGA_MEM_BASE_ADDR  0x000a
> +#define PCI_VGA_MEM_MAX_ADDR   0x000b
> +#define IO_0_LEN   0xcf8
> +#define VGA_MEM_LEN0x2
>  
>  static GArray *build_alloc_array(void)
>  {
> @@ -2142,6 +2155,150 @@ Aml *build_prt(bool is_pci0_prt)
>  return method;
>  }
>  
> +Aml *build_pci_host_bridge(Aml *table, AcpiPciBus *pci_host)
name doesn't reflect exactly what function does,
it builds device descriptions for expander buses (including their csr)
and then it builds csr for for main pci host but not pci device description.

I'd suggest to split out expander buses part into separate function
that returns an expander bus device description, updates crs_range_set
and let the caller to enumerate buses and add descriptions to dsdt.

Then after it we could do a generic csr generation function for the main pci 
host
if it's possible at all (main pci host csr seems heavily board depended)

Instead of taking table and adding stuff directly in to it
it should be cleaner to take as argument empty csr (crs = 
aml_resource_template();)
add stuff to it and let the caller to add/extend csr as/where necessary.

> +{
> +CrsRangeEntry *entry;
> +Aml *scope, *dev, *crs;
> +CrsRangeSet crs_range_set;
> +Range *pci_hole = NULL;
> +Range *pci_hole64 = NULL;
> +PCIBus *bus = NULL;
> +int 

Re: [Xen-devel] [Qemu-devel] [PATCH v5 10/24] hw: acpi: Export the PCI host and holes getters

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:33 +0100
Samuel Ortiz  wrote:

> This is going to be needed by the hardware reduced implementation, so
> let's export it.
> Once the ACPI builder methods and getters will be implemented, the
> acpi_get_pci_host() implementation will become hardware agnostic.
> 
> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/acpi/aml-build.h |  2 ++
>  hw/acpi/aml-build.c | 43 +
>  hw/i386/acpi-build.c| 47 ++---
>  3 files changed, 47 insertions(+), 45 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index c27c0935ae..fde2785b9a 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -400,6 +400,8 @@ build_header(BIOSLinker *linker, GArray *table_data,
>   const char *oem_id, const char *oem_table_id);
>  void *acpi_data_push(GArray *table_data, unsigned size);
>  unsigned acpi_data_len(GArray *table);
> +Object *acpi_get_pci_host(void);
> +void acpi_get_pci_holes(Range *hole, Range *hole64);
>  /* Align AML blob size to a multiple of 'align' */
>  void acpi_align_size(GArray *blob, unsigned align);
>  void acpi_add_table(GArray *table_offsets, GArray *table_data);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 2b9a636e75..b8e32f15f7 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1601,6 +1601,49 @@ void acpi_build_tables_cleanup(AcpiBuildTables 
> *tables, bool mfre)
>  g_array_free(tables->vmgenid, mfre);
>  }

> +/*
> + * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> + */
> +Object *acpi_get_pci_host(void)
> +{
> +PCIHostState *host;
> +
> +host = OBJECT_CHECK(PCIHostState,
> +object_resolve_path("/machine/i440fx", NULL),
> +TYPE_PCI_HOST_BRIDGE);
> +if (!host) {
> +host = OBJECT_CHECK(PCIHostState,
> +object_resolve_path("/machine/q35", NULL),
> +TYPE_PCI_HOST_BRIDGE);
> +}
> +
> +return OBJECT(host);
> +}
in general aml-build.c is a place to put ACPI spec primitives,
so I'd suggest to move it somewhere else.

Considering it's x86 code (so far), maybe move it to something like
hw/i386/acpi-pci.c

Also it might be good to get rid of acpi_get_pci_host() and pass
a pointer to pci_host as acpi_setup() an argument, since it's static
for life of boar we can keep it in AcpiBuildState, and reuse for
mfg/pci_hole/pci bus accesses.
That way we can simplify code a bit and avoid lookup cost of
object_resolve_path() that's called several times unnecessarily.

> +void acpi_get_pci_holes(Range *hole, Range *hole64)
> +{
> +Object *pci_host;
> +
> +pci_host = acpi_get_pci_host();
> +g_assert(pci_host);
> +
> +range_set_bounds1(hole,
> +  object_property_get_uint(pci_host,
> +   PCI_HOST_PROP_PCI_HOLE_START,
> +   NULL),
> +  object_property_get_uint(pci_host,
> +   PCI_HOST_PROP_PCI_HOLE_END,
> +   NULL));
> +range_set_bounds1(hole64,
> +  object_property_get_uint(pci_host,
> +   
> PCI_HOST_PROP_PCI_HOLE64_START,
> +   NULL),
> +  object_property_get_uint(pci_host,
> +   PCI_HOST_PROP_PCI_HOLE64_END,
> +   NULL));
> +}
> +
>  static void crs_range_insert(GPtrArray *ranges, uint64_t base, uint64_t 
> limit)
>  {
>  CrsRangeEntry *entry;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index bd147a6bd2..a5f5f83500 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -232,49 +232,6 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>  info->applesmc_io_base = applesmc_port();
>  }
>  
> -/*
> - * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
> - * On i386 arch we only have two pci hosts, so we can look only for them.
> - */
> -static Object *acpi_get_i386_pci_host(void)
> -{
> -PCIHostState *host;
> -
> -host = OBJECT_CHECK(PCIHostState,
> -object_resolve_path("/machine/i440fx", NULL),
> -TYPE_PCI_HOST_BRIDGE);
> -if (!host) {
> -host = OBJECT_CHECK(PCIHostState,
> -object_resolve_path("/machine/q35", NULL),
> -TYPE_PCI_HOST_BRIDGE);
> -}
> -
> -return OBJECT(host);
> -}
> -
> -static void acpi_get_pci_holes(Range *hole, Range *hole64)
> -{
> -Object *pci_host;
> -
> -pci_host = acpi_get_i386_pci_host();
> -g_assert(pci_host);
> -
> -range_set_bounds1(hole,
> -  

Re: [Xen-devel] [PATCH v5 09/24] hw: i386: Move PCI host definitions to pci_host.h

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:32 +0100
Samuel Ortiz  wrote:

> The PCI hole properties are not pc or i386 specific. They belong to the
> PCI host header instead.
> 
> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/i386/pc.h  | 5 -
>  include/hw/pci/pci_host.h | 6 ++
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index fed136fcdd..bbbdb33ea3 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -182,11 +182,6 @@ void pc_acpi_init(const char *default_dsdt);
>  
>  void pc_guest_info_init(PCMachineState *pcms);
>  
> -#define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
> -#define PCI_HOST_PROP_PCI_HOLE_END "pci-hole-end"
> -#define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
> -#define PCI_HOST_PROP_PCI_HOLE64_END   "pci-hole64-end"
> -#define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
>  #define PCI_HOST_BELOW_4G_MEM_SIZE "below-4g-mem-size"
>  #define PCI_HOST_ABOVE_4G_MEM_SIZE "above-4g-mem-size"
>  
> diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> index ba31595fc7..e343f4d9ca 100644
> --- a/include/hw/pci/pci_host.h
> +++ b/include/hw/pci/pci_host.h
> @@ -38,6 +38,12 @@
>  #define PCI_HOST_BRIDGE_GET_CLASS(obj) \
>   OBJECT_GET_CLASS(PCIHostBridgeClass, (obj), TYPE_PCI_HOST_BRIDGE)
>  
> +#define PCI_HOST_PROP_PCI_HOLE_START   "pci-hole-start"
> +#define PCI_HOST_PROP_PCI_HOLE_END "pci-hole-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_START "pci-hole64-start"
> +#define PCI_HOST_PROP_PCI_HOLE64_END   "pci-hole64-end"
> +#define PCI_HOST_PROP_PCI_HOLE64_SIZE  "pci-hole64-size"
these are pc/q53 machine specific properties and do not belong here


> +
>  struct PCIHostState {
>  SysBusDevice busdev;
>  


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 02/24] hw: acpi: Export ACPI build alignment API

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:25 +0100
Samuel Ortiz  wrote:

> This is going to be needed by the Hardware-reduced ACPI routines.
> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Samuel Ortiz 
the patch is probably misplaced withing series,
if there is an external user within this series then this patch should
be squashed there, otherwise it doesn't belong to this series.

> ---
>  include/hw/acpi/aml-build.h | 2 ++
>  hw/acpi/aml-build.c | 8 
>  hw/i386/acpi-build.c| 8 
>  3 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 6c36903c0a..73fc6659f2 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -384,6 +384,8 @@ build_header(BIOSLinker *linker, GArray *table_data,
>   const char *oem_id, const char *oem_table_id);
>  void *acpi_data_push(GArray *table_data, unsigned size);
>  unsigned acpi_data_len(GArray *table);
> +/* Align AML blob size to a multiple of 'align' */
> +void acpi_align_size(GArray *blob, unsigned align);
>  void acpi_add_table(GArray *table_offsets, GArray *table_data);
>  void acpi_build_tables_init(AcpiBuildTables *tables);
>  void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 1e43cd736d..51b608432f 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1565,6 +1565,14 @@ unsigned acpi_data_len(GArray *table)
>  return table->len;
>  }
>  
> +void acpi_align_size(GArray *blob, unsigned align)
> +{
> +/* Align size to multiple of given size. This reduces the chance
> + * we need to change size in the future (breaking cross version 
> migration).
> + */
> +g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
> +}
> +
>  void acpi_add_table(GArray *table_offsets, GArray *table_data)
>  {
>  uint32_t offset = table_data->len;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index d0362e1382..81d98fa34f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -282,14 +282,6 @@ static void acpi_get_pci_holes(Range *hole, Range 
> *hole64)
> NULL));
>  }
>  
> -static void acpi_align_size(GArray *blob, unsigned align)
> -{
> -/* Align size to multiple of given size. This reduces the chance
> - * we need to change size in the future (breaking cross version 
> migration).
> - */
> -g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
> -}
> -
>  /* FACS */
>  static void
>  build_facs(GArray *table_data, BIOSLinker *linker)


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 01/24] hw: i386: Decouple the ACPI build from the PC machine type

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:24 +0100
Samuel Ortiz  wrote:

> ACPI tables are platform and machine type and even architecture
> agnostic, and as such we want to provide an internal ACPI API that
> only depends on platform agnostic information.
> 
> For the x86 architecture, in order to build ACPI tables independently
> from the PC or Q35 machine types, we are moving a few MachineState
> structure fields into a machine type agnostic structure called
> AcpiConfiguration. The structure fields we move are:

It's not obvious why new structure is needed, especially at
the beginning of series. We probably should place this patch
much later in the series (if we need it at all) and try
generalize a much as possible without using it.

And try to come up with an API that doesn't need centralized collection
of data somehow related to ACPI (most of the fields here are not generic
and applicable to a specific board/target).

For generic API, I'd prefer a separate building blocks
like build_fadt()/... that take as an input only parameters
necessary to compose a table/aml part with occasional board
interface hooks instead of all encompassing AcpiConfiguration
and board specific 'acpi_build' that would use them when/if needed.

We probably should split series into several smaller
(if possible independent) ones, so people won't be scared of
its sheer size and run away from reviewing it.
That way it would be easier to review, amend certain parts and merge.

acpi_setup() & co probably should be the last things that's are
generalized as they are called by concrete boards and might collect
board specific data and apply compat workarounds for building ACPI tables
(assuming that we won't push non generic data into generic API).

See more comments below

>HotplugHandler *acpi_dev
>AcpiNVDIMMState acpi_nvdimm_state;
>FWCfgState *fw_cfg
>ram_addr_t below_4g_mem_size, above_4g_mem_size
>bool apic_xrupt_override
>unsigned apic_id_limit
>uint64_t numa_nodes
>uint64_t numa_mem
> 
> Signed-off-by: Samuel Ortiz 
> ---
>  hw/i386/acpi-build.h |   4 +-
>  include/hw/acpi/acpi.h   |  44 ++
>  include/hw/i386/pc.h |  19 ++---
>  hw/acpi/cpu_hotplug.c|   9 +-
>  hw/arm/virt-acpi-build.c |  10 ---
>  hw/i386/acpi-build.c | 136 ++
>  hw/i386/pc.c | 176 ---
>  hw/i386/pc_piix.c|  21 ++---
>  hw/i386/pc_q35.c |  21 ++---
>  hw/i386/xen/xen-hvm.c|  19 +++--
>  10 files changed, 257 insertions(+), 202 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 007332e51c..065a1d8250 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -2,6 +2,8 @@
>  #ifndef HW_I386_ACPI_BUILD_H
>  #define HW_I386_ACPI_BUILD_H
>  
> -void acpi_setup(void);
> +#include "hw/acpi/acpi.h"
> +
> +void acpi_setup(MachineState *machine, AcpiConfiguration *acpi_conf);
>  
>  #endif
> diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> index c20ace0d0b..254c8d0cfc 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -24,6 +24,8 @@
>  #include "exec/memory.h"
>  #include "hw/irq.h"
>  #include "hw/acpi/acpi_dev_interface.h"
> +#include "hw/hotplug.h"
> +#include "hw/mem/nvdimm.h"
>  
>  /*
>   * current device naming scheme supports up to 256 memory devices
> @@ -186,6 +188,48 @@ extern int acpi_enabled;
>  extern char unsigned *acpi_tables;
>  extern size_t acpi_tables_len;
>  
> +typedef
> +struct AcpiBuildState {
> +/* Copy of table in RAM (for patching). */
> +MemoryRegion *table_mr;
> +/* Is table patched? */
> +bool patched;
> +void *rsdp;
> +MemoryRegion *rsdp_mr;
> +MemoryRegion *linker_mr;
> +} AcpiBuildState;
> +
> +typedef
> +struct AcpiConfiguration {
We used to have a similar intermediate structure PcGuestInfo,
but got rid of it in the end. Even with other questions aside
I'm not quite convinced that it's good idea to reintroduce similar
one again.


> +/* Machine class ACPI settings */
> +int legacy_acpi_table_size;
> +bool rsdp_in_ram;
> +unsigned acpi_data_size;
 (*) well, all 2 are the legacy stuff, I'd rather not to push it into
generic API and keep it in the caller as board specific/machine
version code.

> +
> +/* Machine state ACPI settings */
> +HotplugHandler *acpi_dev;
> +AcpiNVDIMMState acpi_nvdimm_state;
> +
> +/*
> + * The fields below are machine settings that
> + * are not ACPI specific. However they are needed
> + * for building ACPI tables and as such should be
> + * carried through the ACPI configuration structure.
> + */
if they are not ACPI specific, then it shouldn't be in acpi
configuration. Some of the fields are compat hacks, which doesn't
belong to generic API so I'd leave them in board specific code
and some are target specific which also doesn't belong in generic
place.

> +bool legacy_cpu_hotplug;
> +bool linuxboot_dma_enabled;
> 

Re: [Xen-devel] [PATCH v5 07/24] hw: acpi: Generalize AML build routines

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:30 +0100
Samuel Ortiz  wrote:

> From: Yang Zhong 
> 
> Most of the AML build routines under acpi-build are not even
> architecture specific. They can be moved to the more generic hw/acpi
> folder where they could be shared across machine types and
> architectures.

I'd prefer if won't pull into aml-build PCI specific headers,
Suggest to create hw/acpi/pci.c and move generic PCI related
code there, with corresponding header the would export API
(preferably without PCI dependencies in it)


Also patch is too big and does too much at a time.
Here I'd suggest to split it in smaller parts to make it more digestible

1. split it in 3 parts
* MCFG
* CRS
* PTR
2. mcfg between x86 and ARM look pretty much the same with ARM
   open codding bus number calculation and missing migration hack
   * a patch to make bus number calculation in ARM the same as x86
   * a patch to bring migration hack (dummy MCFG table in case it's disabled)
 it's questionable if we actually need it in generic,
 we most likely need it for legacy machines that predate
 resizable MemeoryRegion, but we probably don't need it for
 later machines as problem doesn't exists there.
 So it might be better to push hack out from generic code
 to a legacy caller and keep generic MCFG clean.
 (this patch might be better at the beginning of the series as
  it might affect acpi test results, and might need an update to reference 
tables
  I don't really sure)
   * at this point arm and x86 impl. would be the same so
 a patch to move mcfg build routine to a generic place and replace
 x86/arm with a single impl.
   * a patch to convert mcfg build routine to build_append_int_noprefix() API
 and drop AcpiTableMcfg structure
 
 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Yang Zhong 
> ---
>  include/hw/acpi/aml-build.h |  25 ++
>  hw/acpi/aml-build.c | 498 ++
>  hw/arm/virt-acpi-build.c|   4 +-
>  hw/i386/acpi-build.c| 518 +---
>  4 files changed, 528 insertions(+), 517 deletions(-)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index a2ef8b6f31..4f678c45a5 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -3,6 +3,7 @@
>  
>  #include "hw/acpi/acpi-defs.h"
>  #include "hw/acpi/bios-linker-loader.h"
> +#include "hw/pci/pcie_host.h"
>  
>  /* Reserve RAM space for tables: add another order of magnitude. */
>  #define ACPI_BUILD_TABLE_MAX_SIZE 0x20
> @@ -223,6 +224,21 @@ struct AcpiBuildTables {
>  BIOSLinker *linker;
>  } AcpiBuildTables;
>  
> +typedef struct AcpiMcfgInfo {
> +uint64_t mcfg_base;
> +uint32_t mcfg_size;
> +} AcpiMcfgInfo;
> +
> +typedef struct CrsRangeEntry {
> +uint64_t base;
> +uint64_t limit;
> +} CrsRangeEntry;
> +
> +typedef struct CrsRangeSet {
> +GPtrArray *io_ranges;
> +GPtrArray *mem_ranges;
> +GPtrArray *mem_64bit_ranges;
> +} CrsRangeSet;
>  /**
I'd prefer not to put these into aml-build.h, it's supposed to host ACPI spec 
primitives mostly
so I'd suggest to move these to acpi-defs.h or PCI specific acpi header

>   * init_aml_allocator:
>   *
> @@ -389,6 +405,15 @@ void acpi_align_size(GArray *blob, unsigned align);
>  void acpi_add_table(GArray *table_offsets, GArray *table_data);
>  void acpi_build_tables_init(AcpiBuildTables *tables);
>  void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
> +Aml *build_osc_method(void);
> +void build_mcfg(GArray *table_data, BIOSLinker *linker, AcpiMcfgInfo *info);
> +Aml *build_gsi_link_dev(const char *name, uint8_t uid, uint8_t gsi);
> +Aml *build_prt(bool is_pci0_prt);
> +void crs_range_set_init(CrsRangeSet *range_set);
> +Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set);
> +void crs_replace_with_free_ranges(GPtrArray *ranges,
> +  uint64_t start, uint64_t end);
> +void crs_range_set_free(CrsRangeSet *range_set);
>  void
>  build_rsdp_rsdt(GArray *table_data,
>  BIOSLinker *linker, unsigned rsdt_tbl_offset);
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 8c2388274c..d3242c6b31 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -25,6 +25,10 @@
>  #include "qemu/bswap.h"
>  #include "qemu/bitops.h"
>  #include "sysemu/numa.h"
> +#include "hw/pci/pci.h"
> +#include "hw/pci/pci_bus.h"
> +#include "qemu/range.h"
> +#include "hw/pci/pci_bridge.h"
>  
>  static GArray *build_alloc_array(void)
>  {
> @@ -1597,6 +1601,500 @@ void acpi_build_tables_cleanup(AcpiBuildTables 
> *tables, bool mfre)
>  g_array_free(tables->vmgenid, mfre);
>  }
>  
> +static void crs_range_insert(GPtrArray *ranges, uint64_t base, uint64_t 
> limit)
> +{
> +CrsRangeEntry *entry;
> +
> +entry = g_malloc(sizeof(*entry));
> +entry->base = base;
> +entry->limit = limit;
> +
> +   

Re: [Xen-devel] [Qemu-devel] [PATCH v5 13/24] hw: acpi: Do not create hotplug method when handler is not defined

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:36 +0100
Samuel Ortiz  wrote:

> CPU and memory ACPI hotplug are not necessarily handled through SCI
> events. For example, with Hardware-reduced ACPI, the GED device will
> manage ACPI hotplug entirely.
> As a consequence, we make the CPU and memory specific events AML
> generation optional. The code will only be added when the method name is
> not NULL.
patch doesn't belong to this series, it should be go along with
GED device patch.

Suggest to drop it for now.

> 
> Reviewed-by: Philippe Mathieu-Daudé 
> Tested-by: Philippe Mathieu-Daudé 
> Signed-off-by: Samuel Ortiz 
> ---
>  hw/acpi/cpu.c|  8 +---
>  hw/acpi/memory_hotplug.c | 11 +++
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index f10b190019..cd41377b5a 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -569,9 +569,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, 
> CPUHotplugFeatures opts,
>  aml_append(sb_scope, cpus_dev);
>  aml_append(table, sb_scope);
>  
> -method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> -aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
> -aml_append(table, method);
> +if (event_handler_method) {
> +method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> +aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
> +aml_append(table, method);
> +}
>  
>  g_free(cphp_res_path);
>  }
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 8c7c1013f3..db2c4df961 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -715,10 +715,13 @@ void build_memory_hotplug_aml(Aml *table, uint32_t 
> nr_mem,
>  }
>  aml_append(table, dev_container);
>  
> -method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> -aml_append(method,
> -aml_call0(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD));
> -aml_append(table, method);
> +if (event_handler_method) {
> +method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> +aml_append(method,
> +   aml_call0(MEMORY_DEVICES_CONTAINER "."
> + MEMORY_SLOT_SCAN_METHOD));
> +aml_append(table, method);
> +}
>  
>  g_free(mhp_res_path);
>  }


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP

[Igor Mammedov]

On Thu, 8 Nov 2018 15:16:23 +0100
Igor Mammedov  wrote:

[...]
>   patch 4: convert arm's impl. to build_append_int_noprefix() API (commit 
> 5d7a334f7)

>... move out to aml-build.c
my mistake, generally when we move something out,
we should do it in separate path preferably without any changes to the moved 
code
so it would be easier to review. So it should be a separate patch.

[...]


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v5 03/24] hw: acpi: The RSDP build API can return void

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:26 +0100
Samuel Ortiz  wrote:

> For both x86 and ARM architectures, the internal RSDP build API can
> return void as the current return value is unused.
> 
> Signed-off-by: Samuel Ortiz 
Reviewed-by: Igor Mammedov 

> ---
>  hw/arm/virt-acpi-build.c | 4 +---
>  hw/i386/acpi-build.c | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f28a2faa53..fc59cce769 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -367,7 +367,7 @@ static void acpi_dsdt_add_power_button(Aml *scope)
>  }
>  
>  /* RSDP */
> -static GArray *
> +static void
>  build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>  {
>  AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> @@ -392,8 +392,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned xsdt_tbl_offset)
>  bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>  (char *)rsdp - rsdp_table->data, sizeof *rsdp,
>  (char *)>checksum - rsdp_table->data);
> -
> -return rsdp_table;
>  }
>  
>  static void
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 81d98fa34f..74419d0663 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2513,7 +2513,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>   "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
>  }
>  
> -static GArray *
> +static void
>  build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
>  {
>  AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> @@ -2535,8 +2535,6 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, 
> unsigned rsdt_tbl_offset)
>  bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>  (char *)rsdp - rsdp_table->data, sizeof *rsdp,
>  (char *)>checksum - rsdp_table->data);
> -
> -return rsdp_table;
>  }
>  
>  static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 16/24] hw: acpi: Fix memory hotplug AML generation error

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:39 +0100
Samuel Ortiz  wrote:

> From: Yang Zhong 
> 
> When using the generated memory hotplug AML, the iasl
> compiler would give the following error:
> 
> dsdt.dsl 266: Return (MOST (_UID, Arg0, Arg1, Arg2))
> Error 6080 - Called method returns no value ^
> 
> Signed-off-by: Yang Zhong 
Reviewed-by: Igor Mammedov 

I suggest to put this patch at the beginning of the series
before reference tables in test are updated.

> ---
>  hw/acpi/memory_hotplug.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index db2c4df961..893fc2bd27 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -686,15 +686,15 @@ void build_memory_hotplug_aml(Aml *table, uint32_t 
> nr_mem,
>  
>  method = aml_method("_OST", 3, AML_NOTSERIALIZED);
>  s = MEMORY_SLOT_OST_METHOD;
> -aml_append(method, aml_return(aml_call4(
> -s, aml_name("_UID"), aml_arg(0), aml_arg(1), aml_arg(2)
> -)));
> +aml_append(method,
> +   aml_call4(s, aml_name("_UID"), aml_arg(0),
> + aml_arg(1), aml_arg(2)));
>  aml_append(dev, method);
>  
>  method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
>  s = MEMORY_SLOT_EJECT_METHOD;
> -aml_append(method, aml_return(aml_call2(
> -   s, aml_name("_UID"), aml_arg(0;
> +aml_append(method,
> +   aml_call2(s, aml_name("_UID"), aml_arg(0)));
>  aml_append(dev, method);
>  
>  aml_append(dev_container, dev);


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 05/24] hw: acpi: Implement XSDT support for RSDP

[Igor Mammedov]

On Mon,  5 Nov 2018 02:40:28 +0100
Samuel Ortiz  wrote:

> XSDT is the 64-bit version of the legacy ACPI RSDT (Root System
> Description Table). RSDT only allow for 32-bit addressses and have thus
> been deprecated. Since ACPI version 2.0, RSDPs should point at XSDTs and
> no longer RSDTs, although RSDTs are still supported for backward
> compatibility.
> 
> Since version 2.0, RSDPs should add an extended checksum, a complete table
> length and a version field to the table.

This patch re-implements what arm/virt board already does
and fixes checksum bug in the later and at the same time
without a user (within the patch).

I'd suggest redo it a way similar to FADT refactoring
  patch 1: fix checksum bug in virt/arm
  patch 2: update reference tables in test
  patch 3: introduce AcpiRsdpData similar to commit 937d1b587
 (both arm and x86) wich stores all data in hos byte order
  patch 4: convert arm's impl. to build_append_int_noprefix() API (commit 
5d7a334f7)
   ... move out to aml-build.c
  patch 5: reuse generalized arm's build_rsdp() for x86, dropping x86 specific 
one
  amending it to generate rev1 variant defined by revision in AcpiRsdpData
  (commit dd1b2037a)

  'make check V=1' shouldn't observe any ACPI tables changes after patch 2.

> Signed-off-by: Samuel Ortiz 
> ---
>  include/hw/acpi/aml-build.h |  3 +++
>  hw/acpi/aml-build.c | 37 +
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index c9bcb32d81..3580d0ce90 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -393,6 +393,9 @@ void
>  build_rsdp(GArray *table_data,
> BIOSLinker *linker, unsigned rsdt_tbl_offset);
>  void
> +build_rsdp_xsdt(GArray *table_data,
> +BIOSLinker *linker, unsigned xsdt_tbl_offset);
> +void
>  build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
> const char *oem_id, const char *oem_table_id);
>  void
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 51b608432f..a030d40674 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1651,6 +1651,43 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, 
> GArray *table_offsets,
>   (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, oem_table_id);
>  }
>  
> +/* RSDP pointing at an XSDT */
> +void
> +build_rsdp_xsdt(GArray *rsdp_table,
> +BIOSLinker *linker, unsigned xsdt_tbl_offset)
> +{
> +AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> +unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
> +unsigned xsdt_pa_offset =
> +(char *)>xsdt_physical_address - rsdp_table->data;
> +unsigned xsdt_offset =
> +(char *)>length - rsdp_table->data;
> +
> +bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
> + true /* fseg memory */);
> +
> +memcpy(>signature, "RSD PTR ", 8);
> +memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
> +rsdp->length = cpu_to_le32(sizeof(*rsdp));
> +/* version 2, we will use the XSDT pointer */
> +rsdp->revision = 0x02;
> +
> +/* Address to be filled by Guest linker */
> +bios_linker_loader_add_pointer(linker,
> +ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
> +ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
> +
> +/* Legacy checksum to be filled by Guest linker */
> +bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +(char *)rsdp - rsdp_table->data, xsdt_offset,
> +(char *)>checksum - rsdp_table->data);
> +
> +/* Extended checksum to be filled by Guest linker */
> +bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +(char *)rsdp - rsdp_table->data, sizeof *rsdp,
> +(char *)>extended_checksum - rsdp_table->data);
> +}
> +
>  void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
> uint64_t len, int node, MemoryAffinityFlags flags)
>  {


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH 08/30] hw/i386: use the BYTE-based definitions

[Igor Mammedov]

On Mon, 5 Mar 2018 13:29:15 +0100
Igor Mammedov <imamm...@redhat.com> wrote:

> On Thu, 15 Feb 2018 01:28:38 -0300
> Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> 
> > It ease code review, unit is explicit.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> > ---  
> [...]
> 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index deb440f286..9ccc6192b5 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2320,8 +2320,8 @@ build_tpm2(GArray *table_data, BIOSLinker *linker, 
> > GArray *tcpalog)
> >   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, 
> > NULL);
> >  }
> >  
> > -#define HOLE_640K_START  (640 * 1024)
> > -#define HOLE_640K_END   (1024 * 1024)
> > +#define HOLE_640K_START  (640 * K_BYTE)
> > +#define HOLE_640K_END   (1024 * K_BYTE)  
> nit:
> could be 1 * M_BYTE
> 
> [...]
> 
> Reviewed-by: Igor Mammedov <imamm...@redhat.com>
taking it back

While building it, I get:

In file included from qemu/hw/acpi/ich9.c:38:0:
qemu/include/hw/i386/ich9.h:25:28: error: ‘K_BYTE’ undeclared here (not in a 
function)
 #define ICH9_CC_SIZE (16 * K_BYTE) /* Chipset configuration registers */
^
qemu/include/hw/i386/ich9.h:56:25: note: in expansion of macro ‘ICH9_CC_SIZE’
 uint8_t chip_config[ICH9_CC_SIZE];
 ^
make: *** [hw/acpi/ich9.o] Error 1

 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/30] hw/i386: use the BYTE-based definitions

[Igor Mammedov]

On Thu, 15 Feb 2018 01:28:38 -0300
Philippe Mathieu-Daudé <f4...@amsat.org> wrote:

> It ease code review, unit is explicit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> ---
[...]

> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index deb440f286..9ccc6192b5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2320,8 +2320,8 @@ build_tpm2(GArray *table_data, BIOSLinker *linker, 
> GArray *tcpalog)
>   (void *)tpm2_ptr, "TPM2", sizeof(*tpm2_ptr), 4, NULL, NULL);
>  }
>  
> -#define HOLE_640K_START  (640 * 1024)
> -#define HOLE_640K_END   (1024 * 1024)
> +#define HOLE_640K_START  (640 * K_BYTE)
> +#define HOLE_640K_END   (1024 * K_BYTE)
nit:
could be 1 * M_BYTE

[...]

Reviewed-by: Igor Mammedov <imamm...@redhat.com>

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel