RE: [PATCH v6 5/8] acpi: Align the size to 128k

2020-05-08 Thread miaoyubo



> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, May 4, 2020 10:03 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> ler...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org;
> berra...@redhat.com; Xiexiangyou 
> Subject: Re: [PATCH v6 5/8] acpi: Align the size to 128k
> 
> On Wed, Apr 08, 2020 at 08:58:13PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > If table size is changed between virt_acpi_build and
> > virt_acpi_build_update, the table size would not be updated to UEFI,
> > therefore, just align the size to 128kb, which is enough and same with
> > x86. It would warn if 64k is not enough and the align size should be
> > updated.
> >
> > Signed-off-by: miaoyubo 
> 
> does this affect migration in any way?
> 

No, it would not affect migration.
I migrated one vm between two qemus(one with tables aligned to 128k and one not)
and the vm could be migrated. 

> > ---
> >  hw/arm/virt-acpi-build.c | 25 +
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 7bcd04dfb7..89bb768b0c 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -54,6 +54,8 @@
> >  #include "hw/pci/pci_bridge.h"
> >  #define ARM_SPI_BASE 32
> >
> > +#define ACPI_BUILD_TABLE_SIZE 0x2
> > +
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)  {
> >  uint16_t i;
> > @@ -883,6 +885,15 @@ struct AcpiBuildState {
> >  bool patched;
> >  } AcpiBuildState;
> >
> > +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).
> > + */
> > 2.19.1
> >

Regards,
Miao



RE: [PATCH v6 4/8] acpi: Refactor the source of host bridge and build tables for pxb

2020-05-08 Thread miaoyubo
Thanks so much for such a careful review!!

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, May 4, 2020 10:01 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> ler...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org;
> berra...@redhat.com; Xiexiangyou 
> Subject: Re: [PATCH v6 4/8] acpi: Refactor the source of host bridge and build
> tables for pxb
> 
> On Wed, Apr 08, 2020 at 08:58:12PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > The resources of pxbs and obtained by crs_build and the resources used
> > by pxbs would be moved form the resources defined for host-bridge.
> >
> > The resources for pxb are composed of the bar space of the
> > pci-bridge/pcie-root-port behined it and the config space of devices
> > behind it.
> >
> > Signed-off-by: miaoyubo 
> 
> A bunch of spelling/syntax mistakes in the log, pls spellcheck.
> 
> Pls use the format
>  Yubo Miao 
> 
> 
> 

Thanks for pointing out the mistakes!

> > ---
> >  hw/arm/virt-acpi-build.c | 131
> > +--
> >  1 file changed, 111 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > e8ba09855c..7bcd04dfb7 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,9 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> > +#include "hw/pci/pci_bridge.h"
> >  #define ARM_SPI_BASE 32
> >
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus) @@ -266,19
> > +269,81 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, Aml *scope)  }
> >
> >  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry
> *memmap,
> > -  uint32_t irq, bool use_highmem, bool 
> > highmem_ecam)
> > +  uint32_t irq, bool use_highmem, bool 
> > highmem_ecam,
> > +  VirtMachineState *vms)
> >  {
> >  int ecam_id = VIRT_ECAM_ID(highmem_ecam);
> > -Aml *method, *crs;
> > +int i;
> > +Aml *method, *crs, *dev;
> >  hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base;
> >  hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size;
> >  hwaddr base_pio = memmap[VIRT_PCIE_PIO].base;
> >  hwaddr size_pio = memmap[VIRT_PCIE_PIO].size;
> >  hwaddr base_ecam = memmap[ecam_id].base;
> >  hwaddr size_ecam = memmap[ecam_id].size;
> > +CrsRangeEntry *entry;
> > +CrsRangeSet crs_range_set;
> > +
> > +crs_range_set_init(_range_set);
> >  int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> > +PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > +   object_resolve_path_type("",
> > +   "pcie-host-bridge", NULL),
> > +   TYPE_PCI_HOST_BRIDGE);
> 
> Not TYPE_PCIE_HOST_BRIDGE? And what if it's ambiguous?
> 
> 

Yes, TYPE_PCI_HOST_BRIDGE is ambiguous, I would fix it.  

> > +
> > +PCIBus *bus = s->bus;
> > +/* start to construct the tables for pxb*/
> 
> 
> coding style violation. weird that ehckpatch didn't notice it.
> 

Thanks for such a careful review!

> > +if (bus) {
> > +QLIST_FOREACH(bus, >child, sibling) {
> > +uint8_t bus_num = pci_bus_num(bus);
> > +uint8_t numa_node = pci_bus_numa_node(bus);
> > +
> > +if (!pci_bus_is_root(bus)) {
> > +continue;
> > +}
> > +/*
> > + * Coded up the MIN of the busNr defined for pxb-pcie,
> > + * the MIN - 1 would be the MAX bus number for the main
> > + * host bridge.
> 
> 
> Couldn't figure out this comment. Pls rephrase in some way so it's
> understandable.
> 

How about 
/* 0 - (nr_pcie_buses - 1) is the bus range for the main
  * host-bridge and it equals the MIN of the
  * busNr defined for pxb-pcie*/

> > + */
> > +if (bus_num < nr_pcie_buses) {
> > +nr_pcie_buses = bus_num;
> > +}
> > +
> > +}
> > +
> > +acpi_dsdt_add_pci_route_table(dev, scope, irq);
> > +
> > +/*
> > + * Resources deined for PXBs are composed by the folling parts:
> > +

RE: [PATCH v6 2/8] fw_cfg: Write the extra roots into the fw_cfg

2020-05-08 Thread miaoyubo
> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, May 4, 2020 10:03 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> ler...@redhat.com; imamm...@redhat.com; qemu-devel@nongnu.org;
> berra...@redhat.com; Xiexiangyou 
> Subject: Re: [PATCH v6 2/8] fw_cfg: Write the extra roots into the fw_cfg
> 
> On Wed, Apr 08, 2020 at 08:58:10PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Write the extra roots into the fw_cfg therefore the uefi could get the
> > extra roots. Only if the uefi know there are extra roots, the config
> > space of devices behind the root could be obtained.
> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/arm/virt.c | 23 +++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index
> > 7dc96abf72..0fdfe4129c 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -77,6 +77,7 @@
> >  #include "hw/acpi/generic_event_device.h"
> >  #include "hw/virtio/virtio-iommu.h"
> >  #include "hw/char/pl011.h"
> > +#include "hw/pci/pci_bus.h"
> >
> >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >  static void virt_##major##_##minor##_class_init(ObjectClass *oc,
> > \ @@ -1435,6 +1436,12 @@ void virt_machine_done(Notifier *notifier,
> void *data)
> >  ARMCPU *cpu = ARM_CPU(first_cpu);
> >  struct arm_boot_info *info = >bootinfo;
> >  AddressSpace *as = arm_boot_address_space(cpu, info);
> > +PCIHostState *s = OBJECT_CHECK(PCIHostState,
> > +   object_resolve_path_type("",
> > +   "pcie-host-bridge", NULL),
> > +   TYPE_PCI_HOST_BRIDGE);
> > +
> > +PCIBus *bus = s->bus;
> >
> >  /*
> >   * If the user provided a dtb, we assume the dynamic sysbus nodes
> 
> 
> Seems duplicated all over the place. Add an API for that?
>

Thanks for your reply. I will add the API in patch v7.
 
> > @@ -1453,6 +1460,22 @@ void virt_machine_done(Notifier *notifier, void
> *data)
> >  exit(1);
> >  }
> >
> > +if (bus) {
> > +int extra_hosts = 0;
> > +QLIST_FOREACH(bus, >child, sibling) {
> > +/* look for expander root buses */
> > +if (pci_bus_is_root(bus)) {
> > +extra_hosts++;
> > +}
> > +}
> > +if (extra_hosts && vms->fw_cfg) {
> > +uint64_t *val = g_malloc(sizeof(*val));
> > +*val = cpu_to_le64(extra_hosts);
> > +fw_cfg_add_file(vms->fw_cfg,
> > +   "etc/extra-pci-roots", val, sizeof(*val));
> > +}
> > +}
> > +
> >  virt_acpi_setup(vms);
> >  virt_build_smbios(vms);
> 
> 
> Duplicated from pc. Pls refactor.
> 

Sure. It would be done in patch v7

> >  }
> > --
> > 2.19.1
> >

Regards,
Miao



RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm

2020-02-26 Thread miaoyubo



> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Tuesday, February 25, 2020 9:15 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com; qemu-
> de...@nongnu.org; berra...@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 09:50:25AM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > In this patch,PXB-PCIE is supproted by arm and certain resource is
> > allocated for each pxb-pcie in acpi table.
> > The resource for the main host bridge is also reallocated.
> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/arm/virt-acpi-build.c | 115
> ---
> >  hw/arm/virt.c|   3 +
> >  include/hw/arm/virt.h|   7 +++
> >  3 files changed, 118 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 37c34748a6..be1986c60d 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >  #define ARM_SPI_BASE 32
> >
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus) @@ -266,19
> > +268,116 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, Aml *scope)  }
> >
> >  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry
> *memmap,
> > -  uint32_t irq, bool use_highmem, bool 
> > highmem_ecam)
> > +  uint32_t irq, bool use_highmem, bool 
> > highmem_ecam,
> > +  VirtMachineState *vms)
> >  {
> >  int ecam_id = VIRT_ECAM_ID(highmem_ecam);
> > -Aml *method, *crs;
> > +Aml *method, *crs, *dev;
> > +int count = 0;
> >  hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base;
> >  hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size;
> >  hwaddr base_pio = memmap[VIRT_PCIE_PIO].base;
> >  hwaddr size_pio = memmap[VIRT_PCIE_PIO].size;
> >  hwaddr base_ecam = memmap[ecam_id].base;
> >  hwaddr size_ecam = memmap[ecam_id].size;
> > +/*
> > + * 0x60 would be enough for pxb device
> 
> It's not clear where does pxb come in here.
> 

Thanks for replying, I would add comments when pxb comes.

> > + * if it is too small, there is no enough space
> > + * for a pcie device plugged in a pcie-root port
> > + */
> > +hwaddr size_addr = 0x60;
> > +hwaddr size_io = 0x4000;
> 
> Please explain how are memory and io partitioned.
> Looks like you are forcing a very specific layout here, but it's not 
> documented
> anywhere, which can cause issues like running out of memory where we
> previously were ok.
> 
> 

The resources allocated for pxb is  in the end of the resources used to
be allocated for pci host bridge. The memory length for each pxb-pcie is
0x60 and the io is 16k. The resources allocated for pxbs are quiet small,
Therefore, the resources for the main host bridge should be large enough.
 
For piix4, the memory resource length for each PXB is  0x20, but
In arm, 20 is not enough, if one nvme is plugged on pcie-root-port and plug
on the pxb, 60 is enough for the memory, therefore, the memory length are 
defined
as 0x60. 
 
Just using 0x60 and 0x4000 is not so decent, 
I will try to find if there's a better way to allocate the resources.

> >  int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> > +PCIBus *bus = VIRT_MACHINE(vms)->bus;
> > +
> > +if (bus) {
> > +QLIST_FOREACH(bus, >child, sibling) {
> > +uint8_t bus_num = pci_bus_num(bus);
> > +uint8_t numa_node = pci_bus_numa_node(bus);
> > +
> > +if (!pci_bus_is_root(bus)) {
> > +continue;
> > +}
> > +/*
> > + * Coded up the MIN of the busNr defined for pxb-pcie,
> > + * the MIN - 1 would be the MAX bus number for the main
> > + * host bridge.
> > + */
> > +if (bus_num < nr_pcie_buses) {
> > +nr_pcie_buses = bus_num;
> > +}
> > +count++;
> > +dev = aml_device("PC%.02X", bus_num);
> > + 

RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm

2020-02-26 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Tuesday, February 25, 2020 9:12 PM
> To: miaoyubo 
> Cc: Philippe Mathieu-Daudé ;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> berra...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> ; imamm...@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 12:44:15PM +, miaoyubo wrote:
> >
> >
> > > -Original Message-
> > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > Sent: Tuesday, February 25, 2020 8:27 PM
> > > To: miaoyubo 
> > > Cc: Philippe Mathieu-Daudé ;
> > > peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> > > berra...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > > ; imamm...@redhat.com
> > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > for arm
> > >
> > > On Tue, Feb 25, 2020 at 12:12:12PM +, miaoyubo wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
> > > > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > > > To: miaoyubo ; peter.mayd...@linaro.org;
> > > > > shannon.zha...@gmail.com
> > > > > Cc: berra...@redhat.com; m...@redhat.com; qemu-
> de...@nongnu.org;
> > > > > Xiexiangyou ; imamm...@redhat.com
> > > > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb
> > > > > support for arm
> > > > >
> > > > > On 2/25/20 2:50 AM, Yubo Miao wrote:
> > > > > > From: miaoyubo 
> > > > > >
> > > > > > Currently virt machine is not supported by pxb-pcie, and only
> > > > > > one main host bridge described in ACPI tables.
> > > > > > In this patch,PXB-PCIE is supproted by arm and certain
> > > > >
> > > > > Typos: "expander" in subject and "supported" here.
> > > > >
> > > >
> > > > Thanks for your reply and sorry for the mistakes.
> > > > I will check all the subjects and comments.
> > > >
> > > > > > resource is allocated for each pxb-pcie in acpi table.
> > > > > > The resource for the main host bridge is also reallocated.
> > > > > >
> > > > > > Signed-off-by: miaoyubo 
> > > > > > ---
> > > > > >   hw/arm/virt-acpi-build.c | 115
> > > > > ---
> > > > > >   hw/arm/virt.c|   3 +
> > > > > >   include/hw/arm/virt.h|   7 +++
> > > > > >   3 files changed, 118 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/arm/virt-acpi-build.c
> > > > > > b/hw/arm/virt-acpi-build.c index 37c34748a6..be1986c60d 100644
> > > > > > --- a/hw/arm/virt-acpi-build.c
> > > > > > +++ b/hw/arm/virt-acpi-build.c
> > > > > > @@ -49,6 +49,8 @@
> > > > > >   #include "kvm_arm.h"
> > > > > >   #include "migration/vmstate.h"
> > > > > >
> > > > > > +#include "hw/arm/virt.h"
> > > > > > +#include "hw/pci/pci_bus.h"
> > > > > >   #define ARM_SPI_BASE 32
> > > > > >
> > > > > >   if (use_highmem) {
> > > > > >   hwaddr base_mmio_high =
> > > > > > memmap[VIRT_HIGH_PCIE_MMIO].base;
> > > > > @@
> > > > > > -746,7 +847,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> > > > > > *linker,
> > > > > VirtMachineState *vms)
> > > > > >   acpi_dsdt_add_virtio(scope, [VIRT_MMIO],
> > > > > >   (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
> > > > > NUM_VIRTIO_TRANSPORTS);
> > > > > >   acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] +
> > > > > ARM_SPI_BASE),
> > > > > > -  vms->highmem, vms->highmem_ecam);
> > > > > > +  vms->highmem, vms->highmem_ecam, vms);
> > > > > >   if (vms->acpi_dev) {
> > > > > >   build_ged_aml(scope, "\\_SB."GED_DEVICE,
> > > > > > HOTPLUG_HANDL

RE: [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm

2020-02-25 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Tuesday, February 25, 2020 7:25 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com; qemu-
> de...@nongnu.org; berra...@redhat.com
> Subject: Re: [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie for
> arm
> 
> On Tue, Feb 25, 2020 at 09:50:26AM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently, pxb-pcie could be defined by the cmdline like
> > --device pxb-pcie,id=pci.9,bus_nr=128 However pxb-pcie is not
> > described in acpi tables for arm.
> >
> > The formal two patches support pxb-pcie for arm, escpcially the
> > specification for pxb-pcie in DSDT table.
> 
> 
> especially? Pls spell-check comments and commit log, it's not hard to do.
> 

Thanks for pointing out and sorry for the mistakes, 
I will check all the comments and commit log.

> > Add a testcase to make sure the ACPI table is correct for guest.
> >
> > The following table need to be added for this test:
> > tests/data/acpi/virt/DSDT.pxb
> > Since the ASL diff has 1000+ lines, it would be presented in commit
> > log with the simply diff. the diff are:
> > Device (PC80) is presented in DSDT.
> > Resources allocated for Device (PCI0) is changed.
> >
> >   * Disassembling to symbolic ASL+ operators
> >   *
> > - * Disassembly of /home/DSDT, Mon Feb 24 19:35:28 2020
> > + * Disassembly of /home/DSDT.pxb, Mon Feb 24 19:33:38 2020
> >   *
> >   * Original Table Header:
> >   * Signature"DSDT"
> > - * Length   0x14BB (5307)
> > + * Length   0x1F70 (8048)
> >   * Revision 0x02
> > - * Checksum 0xD1
> > + * Checksum 0xCF
> >   * OEM ID   "BOCHS "
> >   * OEM Table ID "BXPCDSDT"
> >   * OEM Revision 0x0001 (1)
> >  })
> >  }
> >
> >  {
> >  Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware
> ID
> >  WordBusNumber (ResourceProducer, MinFixed, MaxFixed,
> PosDecode,
> >  0x, // Granularity
> >  0x, // Range Minimum
> > -0x00FF, // Range Maximum
> > +0x007F, // Range Maximum
> >  0x, // Translation Offset
> > -0x0100, // Length
> > +0x0080, // Length
> >  ,, )
> >  DWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
> >  0x, // Granularity
> >  0x1000, // Range Minimum
> > -0x3EFE, // Range Maximum
> > +0x3E9E, // Range Maximum
> >  0x, // Translation Offset
> > -0x2EFF, // Length
> > +0x2E9F, // Length
> >  ,, , AddressRangeMemory, TypeStatic)
> >  DWordIO (ResourceProducer, MinFixed, MaxFixed, 
> > PosDecode,
> EntireRange,
> >  0x, // Granularity
> >  0x, // Range Minimum
> > -0x, // Range Maximum
> > +0xBFFF, // Range Maximum
> >  0x3EFF, // Translation Offset
> > -0x0001, // Length
> > +0xC000, // Length
> >  ,, , TypeStatic, DenseTranslation)
> >  QWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
> >  0x, // Granularity
> >
> > Signed-off-by: miaoyubo 
> 
> 
> Seems to fail in patchew.
> 

The failure is due to CONFIG_PXB is not configured.
Since it is not configured by default, I will add ifdef CONFIG_PXB
before the pxb unit test to solve this problem. 

> > ---
> >  tests/data/acpi/virt/DSDT.pxb   | Bin 0 -> 8048 bytes
> >  tests/qtest/bios-tables-test-allowed-diff.h |   1 +
> >  tests/qtest/

RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm

2020-02-25 Thread miaoyubo

> -Original Message-
> From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
> Sent: Tuesday, February 25, 2020 5:48 PM
> To: miaoyubo ; peter.mayd...@linaro.org;
> shannon.zha...@gmail.com
> Cc: berra...@redhat.com; m...@redhat.com; qemu-devel@nongnu.org;
> Xiexiangyou ; imamm...@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On 2/25/20 2:50 AM, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > In this patch,PXB-PCIE is supproted by arm and certain
> 
> Typos: "expander" in subject and "supported" here.
> 

Thanks for your reply and sorry for the mistakes.
I will check all the subjects and comments.

> > resource is allocated for each pxb-pcie in acpi table.
> > The resource for the main host bridge is also reallocated.
> >
> > Signed-off-by: miaoyubo 
> > ---
> >   hw/arm/virt-acpi-build.c | 115
> ---
> >   hw/arm/virt.c|   3 +
> >   include/hw/arm/virt.h|   7 +++
> >   3 files changed, 118 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 37c34748a6..be1986c60d 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >   #include "kvm_arm.h"
> >   #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >   #define ARM_SPI_BASE 32
> >
> >   if (use_highmem) {
> >   hwaddr base_mmio_high = memmap[VIRT_HIGH_PCIE_MMIO].base;
> @@
> > -746,7 +847,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >   acpi_dsdt_add_virtio(scope, [VIRT_MMIO],
> >   (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
> NUM_VIRTIO_TRANSPORTS);
> >   acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] +
> ARM_SPI_BASE),
> > -  vms->highmem, vms->highmem_ecam);
> > +  vms->highmem, vms->highmem_ecam, vms);
> >   if (vms->acpi_dev) {
> >   build_ged_aml(scope, "\\_SB."GED_DEVICE,
> > HOTPLUG_HANDLER(vms->acpi_dev), diff --git
> > a/hw/arm/virt.c b/hw/arm/virt.c index f788fe27d6..6314928671 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1246,6 +1246,9 @@ static void create_pcie(VirtMachineState *vms)
> >   }
> >
> >   pci = PCI_HOST_BRIDGE(dev);
> > +
> > +VIRT_MACHINE(qdev_get_machine())->bus = pci->bus;
> > +
> >   if (pci->bus) {
> >   for (i = 0; i < nb_nics; i++) {
> >   NICInfo *nd = _table[i]; diff --git
> > a/include/hw/arm/virt.h b/include/hw/arm/virt.h index
> > 71508bf40c..90f10a1e46 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -140,6 +140,13 @@ typedef struct {
> >   DeviceState *gic;
> >   DeviceState *acpi_dev;
> >   Notifier powerdown_notifier;
> > +/*
> > + * pointer to devices and objects
> > + * Via going through the bus, all
> > + * pci devices and related objectes
> 
> Typo "objects", but I don't understand the comment well.
> 

Sorry for any confusion caused ,I will rewrite the comment 
/* point to the root bus, which is pcie.0 */
Does this comment make sense?

> > + * could be gained.
> > + */
> > +PCIBus *bus;
> >   } VirtMachineState;
> >
> >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > VIRT_PCIE_ECAM)
> >

Regards,
Miao


RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm

2020-02-25 Thread miaoyubo



> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Tuesday, February 25, 2020 8:27 PM
> To: miaoyubo 
> Cc: Philippe Mathieu-Daudé ;
> peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> berra...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> ; imamm...@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 12:12:12PM +, miaoyubo wrote:
> >
> > > -Original Message-
> > > From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
> > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > To: miaoyubo ; peter.mayd...@linaro.org;
> > > shannon.zha...@gmail.com
> > > Cc: berra...@redhat.com; m...@redhat.com; qemu-devel@nongnu.org;
> > > Xiexiangyou ; imamm...@redhat.com
> > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > for arm
> > >
> > > On 2/25/20 2:50 AM, Yubo Miao wrote:
> > > > From: miaoyubo 
> > > >
> > > > Currently virt machine is not supported by pxb-pcie, and only one
> > > > main host bridge described in ACPI tables.
> > > > In this patch,PXB-PCIE is supproted by arm and certain
> > >
> > > Typos: "expander" in subject and "supported" here.
> > >
> >
> > Thanks for your reply and sorry for the mistakes.
> > I will check all the subjects and comments.
> >
> > > > resource is allocated for each pxb-pcie in acpi table.
> > > > The resource for the main host bridge is also reallocated.
> > > >
> > > > Signed-off-by: miaoyubo 
> > > > ---
> > > >   hw/arm/virt-acpi-build.c | 115
> > > ---
> > > >   hw/arm/virt.c|   3 +
> > > >   include/hw/arm/virt.h|   7 +++
> > > >   3 files changed, 118 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > > index 37c34748a6..be1986c60d 100644
> > > > --- a/hw/arm/virt-acpi-build.c
> > > > +++ b/hw/arm/virt-acpi-build.c
> > > > @@ -49,6 +49,8 @@
> > > >   #include "kvm_arm.h"
> > > >   #include "migration/vmstate.h"
> > > >
> > > > +#include "hw/arm/virt.h"
> > > > +#include "hw/pci/pci_bus.h"
> > > >   #define ARM_SPI_BASE 32
> > > >
> > > >   if (use_highmem) {
> > > >   hwaddr base_mmio_high =
> > > > memmap[VIRT_HIGH_PCIE_MMIO].base;
> > > @@
> > > > -746,7 +847,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> > > > *linker,
> > > VirtMachineState *vms)
> > > >   acpi_dsdt_add_virtio(scope, [VIRT_MMIO],
> > > >   (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
> > > NUM_VIRTIO_TRANSPORTS);
> > > >   acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] +
> > > ARM_SPI_BASE),
> > > > -  vms->highmem, vms->highmem_ecam);
> > > > +  vms->highmem, vms->highmem_ecam, vms);
> > > >   if (vms->acpi_dev) {
> > > >   build_ged_aml(scope, "\\_SB."GED_DEVICE,
> > > > HOTPLUG_HANDLER(vms->acpi_dev), diff --git
> > > > a/hw/arm/virt.c b/hw/arm/virt.c index f788fe27d6..6314928671
> > > > 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -1246,6 +1246,9 @@ static void create_pcie(VirtMachineState
> *vms)
> > > >   }
> > > >
> > > >   pci = PCI_HOST_BRIDGE(dev);
> > > > +
> > > > +VIRT_MACHINE(qdev_get_machine())->bus = pci->bus;
> > > > +
> > > >   if (pci->bus) {
> > > >   for (i = 0; i < nb_nics; i++) {
> > > >   NICInfo *nd = _table[i]; diff --git
> > > > a/include/hw/arm/virt.h b/include/hw/arm/virt.h index
> > > > 71508bf40c..90f10a1e46 100644
> > > > --- a/include/hw/arm/virt.h
> > > > +++ b/include/hw/arm/virt.h
> > > > @@ -140,6 +140,13 @@ typedef struct {
> > > >   DeviceState *gic;
> > > >   DeviceState *acpi_dev;
> > > >   Notifier powerdown_notifier;
> > > > +/*
> > > > + * pointer to devices and objects
> > > > + * Via going through the bus, all
> > > > + * pci devices and related objectes
> > >
> > > Typo "objects", but I don't understand the comment well.
> > >
> >
> > Sorry for any confusion caused ,I will rewrite the comment
> > /* point to the root bus, which is pcie.0 */ Does this comment make
> > sense?
> 
> Not really. E.g. it doesn't say what happens if there's more than one root.
> 

If there's more than one root, like pcie.0 and pcie.1, it still point to pcie.0.
In docs/pci_expander_bridge.txt, it points out pxb could be placed only
on bus 0 (pci.0). Therfore, the structure still could help us to find all pxb 
devices.
/* point to the bus 0, which is pcie.0
  * pxb devices could only be placed on bus 0.
  */
Is this ok?

> > > > + * could be gained.
> > > > + */
> > > > +PCIBus *bus;
> > > >   } VirtMachineState;
> > > >
> > > >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > > VIRT_PCIE_ECAM)
> > > >
> >
> > Regards,
> > Miao

Regards,
Miao



RE: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under arm.

2020-02-24 Thread miaoyubo

> -Original Message-
> From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> Sent: Monday, February 24, 2020 8:36 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; m...@redhat.com; qemu-devel@nongnu.org;
> Xiexiangyou ; shannon.zha...@gmail.com;
> imamm...@redhat.com
> Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie
> under arm.
> 
> On Sat, Feb 15, 2020 at 08:59:28AM +, miaoyubo wrote:
> >
> > > -Original Message-
> > > From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> > > Sent: Friday, February 14, 2020 6:25 PM
> > > To: miaoyubo 
> > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> > > imamm...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > > ; m...@redhat.com
> > > Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to
> > > pxb-pcie under arm.
> > >
> > > On Fri, Feb 14, 2020 at 07:25:43AM +, miaoyubo wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> > > > > Sent: Thursday, February 13, 2020 9:52 PM
> > > > > To: miaoyubo 
> > > > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> > > > > imamm...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > > > > ; m...@redhat.com
> > > > > Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to
> > > > > pxb-pcie under arm.
> > > > >
> > > > > On Thu, Feb 13, 2020 at 03:49:52PM +0800, Yubo Miao wrote:
> > > > > > From: miaoyubo 
> > > > > >
> > > > > > Since devices could not directly plugged into pxb-pcie, under
> > > > > > arm, one pcie-root port is plugged into pxb-pcie. Due to the
> > > > > > bus for each pxb-pcie is defined as 2 in acpi dsdt tables(one
> > > > > > for pxb-pcie, one for pcie-root-port), only one device could
> > > > > > be plugged into
> > > one pxb-pcie.
> > > > >
> > > > > What is the cause of this arm specific requirement for pxb-pcie
> > > > > and more importantly can be fix it so that we don't need this patch ?
> > > > > I think it is highly undesirable to have such a per-arch
> > > > > difference in configuration of the pxb-pcie device. It means any
> > > > > mgmt app which already supports pxb-pcie will be broken and need
> to special case arm.
> > > > >
> > > >
> > > > Thanks for your reply, Without this patch, the pxb-pcie is also
> > > > useable, however, one extra pcie-root-port or pci-bridge or
> > > > something else need to be defined by mgmt. app. This patch will could
> be abandoned.
> > >
> > > That's not really answering my question. IIUC, this pxb-pcie device
> > > works fine on x86_64, and I want to know why it doesn't work on arm ?
> > > Requiring different setups by the mgmt apps is not at all nice
> > > because it will inevitably lead to broken arm setups. x86_64 gets
> > > far more testing & usage, developers won't realize arm is different.
> > >
> > >
> >
> > Thanks for replying. Currently, on x86_64, pxb-pcie devices is
> > presented in acpi tables but on arm, It is not, only one main host
> > bridge is presented for arm in acpi dsdt tables. That's why pxb-pcie
> > works on
> > x86_64 but doesn't work on arm. The patch 1/2 do the work to present
> > and allocate resources for pxb-pcie in arm.
> 
> Yes, this first patch makes sense
> 

Thanks for the comments, the patch has been updated to v4, pls check it.

> > For x86_64, if one device is going to be plugged into pxb-pcie, one
> > extra pcie-root-port or pci-bridge have to be defined and plugged on
> > pxb-pcie, then the device is plugged on the pcie-root-port or pci-bridge.
> 
> > This patch 2/2 just auto defined one pcie-root-port for arm. If this
> > patch abandoned, the usage of pxb-pcie would be the same with x86_64,
> > therefore, to keep the same step for x86 and arm, this patch 2/2 could
> > be abandonded.
> 
> Yes, I think abandoning this patch 2 is best. Applications that know how to
> use pxb-pcie on x86_64, will already do the right thing on arm too, once your
> first patch is merged.
> 

This patch has been abandoned since v3.

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


Regards,
Miao


RE: [RFC v3 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm

2020-02-22 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, February 21, 2020 7:19 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com;
> qemu-devel@nongnu.org
> Subject: Re: [RFC v3 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm
> 
> On Fri, Feb 21, 2020 at 02:35:12PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently, pxb-pcie could be defined by the cmdline like
> > --device pxb-pcie,id=pci.9,bus_nr=128 However pxb-pcie is not
> > described in acpi tables for arm.
> >
> > The formal two patches support pxb-pcie for arm, escpcially the
> > specification for pxb-pcie in DSDT table.
> >
> > Add a testcase to make sure the ACPI table is correct for guest.
> >
> > Signed-off-by: miaoyubo 
> 
> 
> Please look at the top of tests/qtest/bios-tables-test.c for how to add or
> update tests.
> 

Thanks for replying, I didn't notice that, I would follow the steps to rebuild 
this patch.

> > ---
> >  tests/data/acpi/virt/DSDT.pxb  | Bin 0 -> 34209 bytes
> > tests/qtest/bios-tables-test.c |  54 +
> >  2 files changed, 48 insertions(+), 6 deletions(-)  create mode 100644
> > tests/data/acpi/virt/DSDT.pxb
> >
> > diff --git a/tests/data/acpi/virt/DSDT.pxb
> > b/tests/data/acpi/virt/DSDT.pxb new file mode 100644 index
> >
> ..4eea3192c75ff28f7054d626
> a936
> > 3ca025b6c0ad
> > GIT binary patch
> 
> I can't read this.
> 

I just have a question that is: 
I just rebuild this aml with tests/data/acpi/rebuild-expected-aml.sh
and git send it or send the aml with attachment?

> > literal 34209
> >
> zcmeI*cXU+szJ~D)1PGxe5PG+us9-{YGz}UAMT!L#ks?x*Dx!d5hoIP
> d
> >
> z?}}o>iWL;GW5HgrlKbvVM??^)~qbMIProvd|8p2_U*%qO!m?AgcPkRQ( r)WX
> >
> zR3DKyBsMVKK5tZUEMJ#Z3xXj0I{cizY-H-_vUpxu>HLbszg^Hd*
> >
> zYT59@{GfDxK}u{$QSzH5MFX?4va_qcnOYVriD$G-YqqdX5KgQUqzA#0T0ymH9a
> J-P
> > zt=#;Qdf_)p=V$jH6t9{xXmH68P3ev)8EFlwrs(=X$_(9dxJh>6UU8FZi5vcVla%Bp
> >
> zz50)g^-pXvw4i9XAYFAU@nN}Xb+t___n%uw)`
> 8L
> >
> z7F4goX88!*;pB+$X8_2BOj*;OO*!h6xx+mI)uU#l*o>||BPVi3ji?#5Y(|dH
> >
> z=oUF6C2B^h<}5a88su#W_0%%JtAk+ikeZ+X7unGJtJq-j+)WHX7uzKy&`9
> %
> 
> ...

Regards,
Miao



RE: [RFC v3 2/3] acpi:pci-expender-bus: Add pxb support for arm

2020-02-22 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, February 21, 2020 7:18 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com;
> qemu-devel@nongnu.org
> Subject: Re: [RFC v3 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Fri, Feb 21, 2020 at 02:35:11PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > In this patch,PXB-PCIE is supproted by arm and certain resource is
> > allocated for each pxb-pcie in acpi table.
> > The resource for the main host bridge is also reallocated.
> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/arm/virt-acpi-build.c | 125
> +++
> >  hw/pci-host/gpex.c   |   4 ++
> >  include/hw/arm/virt.h|   7 +++
> >  3 files changed, 125 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 0540234b8a..2c1e0d2aaa 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >  #define ARM_SPI_BASE 32
> >
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus) @@ -271,19
> > +273,117 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, Aml *scope)  }
> >
> >  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry
> *memmap,
> > -  uint32_t irq, bool use_highmem, bool
> highmem_ecam)
> > +  uint32_t irq, bool use_highmem, bool
> highmem_ecam,
> > +  VirtMachineState *vms)
> >  {
> >  int ecam_id = VIRT_ECAM_ID(highmem_ecam);
> > -Aml *method, *crs;
> > +Aml *method, *dev, *crs;
> > +int count = 0;
> >  hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base;
> >  hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size;
> >  hwaddr base_pio = memmap[VIRT_PCIE_PIO].base;
> >  hwaddr size_pio = memmap[VIRT_PCIE_PIO].size;
> >  hwaddr base_ecam = memmap[ecam_id].base;
> >  hwaddr size_ecam = memmap[ecam_id].size;
> > +/*
> > + * 0x60 would be enough for pxb device
> > + * if it is too small, there is no enough space
> > + * for a pcie device plugged in a pcie-root port
> > + */
> > +hwaddr size_addr = 0x60;
> > +hwaddr size_io = 0x4000;
> >  int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> > +int root_bus_limit = 0xFF;
> 
> what's this?
> 

Thanks for replying.
This is used to define the bus numbers for the main host bridge, 
If no pxb-pcie is defined, the bus number for the main host bridge
would range form 0 to 255.

> > +PCIBus *bus = NULL;
> > +bus = VIRT_MACHINE(vms)->bus;
> 
> So just move assignment as part of declaration.
> 

Thanks for the suggestion!

> > +
> > +if (bus) {
> > +QLIST_FOREACH(bus, >child, sibling) {
> > +uint8_t bus_num = pci_bus_num(bus);
> > +uint8_t numa_node = pci_bus_numa_node(bus);
> > +
> > +if (!pci_bus_is_root(bus)) {
> > +continue;
> > +}
> > +if (bus_num < root_bus_limit) {
> > +root_bus_limit = bus_num - 1;
> 
> what is this doing? manually coded up MIN?
> 

This coded up the MIN of busNr defined in pxb-pcie devices, 
and the Min busNr-1 would be the MAX bus Number for the main host bridge.
For example, if one pxb-pcie with busNr 128(which is 80) defined, 
The bus for the main host bridge would be 0-7F, and the bus for pxb-pcie
would be 80-81(just allocate two buses,keep the same with X86).
If pxb-pcie is not defined, the bus for main host bridge would be 0-FF.

> > +}
> > +count++;
> > +dev = aml_device("PC%.02X", bus_num);
> > +aml_append(dev, aml_name_decl("_HID",
> aml_string("PNP0A08")));
> > +aml_append(dev, aml_name_decl("_CID",
> aml_string("PNP0A03")));
> > +aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > +aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > +aml_append(dev, aml_name_dec

RE: [RFC v2 1/1] arm: acpi: pci-expender-bus: Make arm to support PXB-PCIE

2020-02-17 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, February 17, 2020 9:09 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com; qemu-
> de...@nongnu.org
> Subject: Re: [RFC v2 1/1] arm: acpi: pci-expender-bus: Make arm to support
> PXB-PCIE
> 
> On Mon, Feb 17, 2020 at 07:18:18PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > Under this circumstance, different io numas for differnt devices is
> > not possible, in order to present io numas to the guest, especially
> > for host pssthrough devices. PXB-PCIE is supproted by arm and certain
> > resource is allocated for each pxb-pcie in acpi table.
> >
> > Signed-off-by: miaoyubo 
> 
> A unit test would be nic.
> 

Thanks for replying, I will add the unit test in patch V3.

> > ---
> >  hw/arm/virt-acpi-build.c | 240 +---
> ---
> >  hw/pci-host/gpex.c   |   4 +
> >  include/hw/arm/virt.h|   1 +
> >  3 files changed, 184 insertions(+), 61 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > bd5f771e9b..fc11525042 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >  #define ARM_SPI_BASE 32
> >
> > +method = aml_method("_CRS", 0, AML_NOTSERIALIZED);
> > +Aml *rbuf = aml_resource_template();
> > +aml_append(rbuf,
> > +aml_word_bus_number(AML_MIN_FIXED, AML_MAX_FIXED,
> AML_POS_DECODE,
> > +0x, 0x, root_bus_limit, 0x,
> > +root_bus_limit + 1));
> > +aml_append(rbuf,
> > +aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> AML_MAX_FIXED,
> > + AML_NON_CACHEABLE, AML_READ_WRITE, 0x,
> base_mmio,
> > + base_mmio + size_mmio - 1 - size_addr * count,
> > + 0x, size_mmio - size_addr * count));
> > +aml_append(rbuf,
> > +aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
> AML_POS_DECODE,
> > + AML_ENTIRE_RANGE, 0x, 0x,
> > + size_pio / 2 - 1 - size_io * count, base_pio,
> > + size_pio / 2 - size_io * count));
> > +
> > +if (use_highmem) {
> > +hwaddr base_mmio_high = memmap[VIRT_HIGH_PCIE_MMIO].base;
> > +hwaddr size_mmio_high = memmap[VIRT_HIGH_PCIE_MMIO].size;
> > +
> > +aml_append(rbuf,
> > +aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> AML_MAX_FIXED,
> > + AML_NON_CACHEABLE, AML_READ_WRITE, 0x,
> > + base_mmio_high,
> > + base_mmio_high + size_mmio_high - 1 -
> > + size_addr * count,
> > + 0x, size_mmio_high - size_addr * count));
> > +}
> > +
> > +aml_append(method, aml_name_decl("RBUF", rbuf));
> > +aml_append(method, aml_return(rbuf));
> > +aml_append(dev, method);
> > +
> > +acpi_dsdt_add_pci_osc(dev, scope);
> >
> >  Aml *dev_rp0 = aml_device("%s", "RP0");
> >  aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> 
> 
> this will be easier to review if you first refactor existing code, then add 
> pxb
> support on top.
> 

Thanks for the suggestion, the next patch would separate this patch into two 
patches, 
one is to refactor existing code and another one add pxb support.

> > @@ -744,7 +862,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >  acpi_dsdt_add_virtio(scope, [VIRT_MMIO],
> >  (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
> NUM_VIRTIO_TRANSPORTS);
> >  acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] +
> ARM_SPI_BASE),
> > -  vms->highmem, vms->highmem_ecam);
> > +  vms->highmem, vms->highmem_ecam, vms);
> >  if (vms->acpi_dev) {
> >  build_ged_aml(scope, "\\_SB."GED_DEVICE,
> >HOTPLUG_HANDLER(vms->acpi_dev), diff --git

RE: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under arm.

2020-02-15 Thread miaoyubo

> -Original Message-
> From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> Sent: Friday, February 14, 2020 6:25 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> imamm...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> ; m...@redhat.com
> Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under
> arm.
> 
> On Fri, Feb 14, 2020 at 07:25:43AM +, miaoyubo wrote:
> >
> > > -Original Message-
> > > From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> > > Sent: Thursday, February 13, 2020 9:52 PM
> > > To: miaoyubo 
> > > Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> > > imamm...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > > ; m...@redhat.com
> > > Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to
> > > pxb-pcie under arm.
> > >
> > > On Thu, Feb 13, 2020 at 03:49:52PM +0800, Yubo Miao wrote:
> > > > From: miaoyubo 
> > > >
> > > > Since devices could not directly plugged into pxb-pcie, under arm,
> > > > one pcie-root port is plugged into pxb-pcie. Due to the bus for
> > > > each pxb-pcie is defined as 2 in acpi dsdt tables(one for
> > > > pxb-pcie, one for pcie-root-port), only one device could be plugged into
> one pxb-pcie.
> > >
> > > What is the cause of this arm specific requirement for pxb-pcie and
> > > more importantly can be fix it so that we don't need this patch ?
> > > I think it is highly undesirable to have such a per-arch difference
> > > in configuration of the pxb-pcie device. It means any mgmt app which
> > > already supports pxb-pcie will be broken and need to special case arm.
> > >
> >
> > Thanks for your reply, Without this patch, the pxb-pcie is also
> > useable, however, one extra pcie-root-port or pci-bridge or something
> > else need to be defined by mgmt. app. This patch will could be abandoned.
> 
> That's not really answering my question. IIUC, this pxb-pcie device works fine
> on x86_64, and I want to know why it doesn't work on arm ?
> Requiring different setups by the mgmt apps is not at all nice because it will
> inevitably lead to broken arm setups. x86_64 gets far more testing & usage,
> developers won't realize arm is different.
> 
>

Thanks for replying. Currently, on x86_64, pxb-pcie devices is presented in 
acpi tables but on arm, It is not, only one main host bridge is presented for 
arm in acpi dsdt tables. That's why pxb-pcie works on x86_64 but doesn't work 
on arm. The patch 1/2 do the work to present and allocate resources for 
pxb-pcie in arm.
For x86_64, if one device is going to be plugged into pxb-pcie, one extra 
pcie-root-port or pci-bridge have to be defined and plugged on pxb-pcie, then 
the device is plugged on the pcie-root-port or pci-bridge.
This patch 2/2 just auto defined one pcie-root-port for arm. If this patch 
abandoned, the usage of pxb-pcie would be the same with x86_64, therefore, to 
keep the same step for x86 and arm, this patch 2/2 could be abandonded.

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

Regards,
Miao


RE: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under arm.

2020-02-13 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Thursday, February 13, 2020 6:17 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com; qemu-
> de...@nongnu.org
> Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie
> under arm.
> 
> On Thu, Feb 13, 2020 at 03:49:52PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Since devices could not directly plugged into pxb-pcie,
> 
> Hmm is this different from the root port? intergrated devices do exist for
> that actually.
> 

Thanks for replying
The pxb-pcie is like a host bridge,
 you have to plug pcie-root-port or pci-bridge so devices could be plugged

> > under arm,
> 
> how is arm special?
> 

Cureently, if u define a pxb-pcie device, one pcie-root-port or pci-bridge or 
something 
else have to be defined also, The patch just auto generate pcie-root-port for 
arm to 
avoid affect other architecture

> > one
> > pcie-root port is plugged into pxb-pcie. Due to the bus for each
> > pxb-pcie is defined as 2 in acpi dsdt tables(one for pxb-pcie, one for
> > pcie-root-port), only one device could be plugged into one pxb-pcie.
> 
> So why can't we have users specify any number of root ports using -device?
> then make acpi tables match the # of ports created?
> 
> 

Users could specify multiply pxb-devices, it is supported.
But only one device could be plugged for each pxb-pcie,  it is the same with 
pxb-pci for piix.

> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/pci-bridge/pci_expander_bridge.c | 9 +
> >  include/hw/pci/pcie_port.h  | 1 +
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 47aaaf8fd1..3d896dd452 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -15,6 +15,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_bus.h"
> >  #include "hw/pci/pci_host.h"
> > +#include "hw/pci/pcie_port.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/pci/pci_bridge.h"
> >  #include "qemu/range.h"
> > @@ -233,7 +234,15 @@ static void pxb_dev_realize_common(PCIDevice
> > *dev, bool pcie, Error **errp)
> >
> >  ds = qdev_create(NULL, TYPE_PXB_HOST);
> >  if (pcie) {
> > +#ifdef __aarch64__
> > +bus = pci_root_bus_new(ds, "pxb-pcie-internal",
> > +   NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
> > +bds = qdev_create(BUS(bus), "pcie-root-port");
> > +bds->id = dev_name;
> > +qdev_prop_set_uint8(bds, PCIE_ROOT_PORT_PROP_CHASSIS,
> > +pxb->bus_nr); #else
> >  bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0,
> > TYPE_PXB_PCIE_BUS);
> > +#endif
> 
> What does all this have to do with building on aarch64?
> 

Based on the comments, this patch would be abandoned in patch V2, 
PXB-PCIE would also be useful but pcie-root-port or pci-bridge have to Be 
defined by user.

> >  } else {
> >  bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> TYPE_PXB_BUS);
> >  bds = qdev_create(BUS(bus), "pci-bridge"); diff --git
> > a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h index
> > 4b3d254b08..b41d473220 100644
> > --- a/include/hw/pci/pcie_port.h
> > +++ b/include/hw/pci/pcie_port.h
> > @@ -64,6 +64,7 @@ int pcie_chassis_add_slot(struct PCIESlot *slot);
> > void pcie_chassis_del_slot(PCIESlot *s);
> >
> >  #define TYPE_PCIE_ROOT_PORT "pcie-root-port-base"
> > +#define PCIE_ROOT_PORT_PROP_CHASSIS "chassis"
> 
> If you are going to do this, replace other instances of "chassis"
> with the macro.
> 

Thanks for your replay, this patch would be abandoned.

> >  #define PCIE_ROOT_PORT_CLASS(klass) \
> >   OBJECT_CLASS_CHECK(PCIERootPortClass, (klass),
> > TYPE_PCIE_ROOT_PORT)  #define PCIE_ROOT_PORT_GET_CLASS(obj) \
> > --
> > 2.19.1
> >

Regards,
Miao




RE: [RFC 1/2] arm: acpi: pci-expender-bus: Make arm to support PXB-PCIE

2020-02-13 Thread miaoyubo


> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Thursday, February 13, 2020 6:23 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Xiexiangyou
> ; imamm...@redhat.com; qemu-
> de...@nongnu.org
> Subject: Re: [RFC 1/2] arm: acpi: pci-expender-bus: Make arm to support
> PXB-PCIE
> 
> On Thu, Feb 13, 2020 at 03:49:51PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > Under this circumstance, different io numas for differnt devices is
> > not possible, in order to present io numas to the guest, especially
> > for host pssthrough devices. PXB-PCIE is supproted by arm and certain
> > resource is allocated for each pxb-pcie in acpi table.
> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/arm/virt-acpi-build.c | 234
> +--
> >  hw/pci-host/gpex.c   |   4 +
> >  include/hw/arm/virt.h|   1 +
> >  3 files changed, 228 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > bd5f771e9b..2e449d0098 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >  #define ARM_SPI_BASE 32
> >
> > +/*
> > + * PCI Firmware Specification 3.0
> > + * 4.6.1. _DSM for PCI Express Slot Information
> > + * The UUID in _DSM in this context is
> > + * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
> > + */
> > +UUID = aml_touuid("E5C937D0-3553-4D7A-9117-EA4D19C3434D");
> > +ifctx = aml_if(aml_equal(aml_arg(0), UUID));
> > +ifctx1 = aml_if(aml_equal(aml_arg(2), aml_int(0)));
> > +uint8_t byte_list[1] = {1};
> > +buf = aml_buffer(1, byte_list);
> > +aml_append(ifctx1, aml_return(buf));
> > +aml_append(ifctx, ifctx1);
> > +aml_append(method, ifctx);
> > +
> > +byte_list[0] = 0;
> > +buf = aml_buffer(1, byte_list);
> > +aml_append(method, aml_return(buf));
> > +aml_append(dev, method);
> > +
> > +Aml *dev_rp0 = aml_device("%s", "RP0");
> > +aml_append(dev_rp0, aml_name_decl("_ADR", aml_int(0)));
> > +aml_append(dev, dev_rp0);
> > +
> > +aml_append(scope, dev);
> > +
> > +}
> > +}
> 
> 
> There's a bunch of code duplicated here. Please refactor creating APIs for
> reused code.
> 

Thanks for your reply, this would be done by patch V2.

> >
> > -Aml *dev = aml_device("%s", "PCI0");
> > +dev = aml_device("%s", "PCI0");
> >  aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A08")));
> >  aml_append(dev, aml_name_decl("_CID", aml_string("PNP0A03")));
> >  aml_append(dev, aml_name_decl("_SEG", aml_int(0))); @@ -219,16
> > +428,18 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> >  Aml *rbuf = aml_resource_template();
> >  aml_append(rbuf,
> >  aml_word_bus_number(AML_MIN_FIXED, AML_MAX_FIXED,
> AML_POS_DECODE,
> > -0x, 0x, nr_pcie_buses - 1, 0x,
> > -nr_pcie_buses));
> > +0x, 0x, root_bus_limit, 0x,
> > +root_bus_limit + 1));
> >  aml_append(rbuf,
> >  aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> AML_MAX_FIXED,
> >   AML_NON_CACHEABLE, AML_READ_WRITE, 0x,
> base_mmio,
> > - base_mmio + size_mmio - 1, 0x, size_mmio));
> > + base_mmio + size_mmio - 1 - size_addr * count,
> > + 0x, size_mmio - size_addr * count));
> >  aml_append(rbuf,
> >  aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
> AML_POS_DECODE,
> > - AML_ENTIRE_RANGE, 0x, 0x, size_pio - 1, 
> > base_pio,
> > - size_pio));
> > + AML_ENTIRE_RANGE, 0x, 

RE: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under arm.

2020-02-13 Thread miaoyubo

> -Original Message-
> From: Daniel P. Berrangé [mailto:berra...@redhat.com]
> Sent: Thursday, February 13, 2020 9:52 PM
> To: miaoyubo 
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> imamm...@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> ; m...@redhat.com
> Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie
> under arm.
> 
> On Thu, Feb 13, 2020 at 03:49:52PM +0800, Yubo Miao wrote:
> > From: miaoyubo 
> >
> > Since devices could not directly plugged into pxb-pcie, under arm, one
> > pcie-root port is plugged into pxb-pcie. Due to the bus for each
> > pxb-pcie is defined as 2 in acpi dsdt tables(one for pxb-pcie, one for
> > pcie-root-port), only one device could be plugged into one pxb-pcie.
> 
> What is the cause of this arm specific requirement for pxb-pcie and more
> importantly can be fix it so that we don't need this patch ?
> I think it is highly undesirable to have such a per-arch difference in
> configuration of the pxb-pcie device. It means any mgmt app which already
> supports pxb-pcie will be broken and need to special case arm.
> 

Thanks for your reply, Without this patch, the pxb-pcie is also useable, 
however, one extra pcie-root-port or pci-bridge or something else need 
to be defined by mgmt. app. This patch will could be abandoned.

> >
> > Signed-off-by: miaoyubo 
> > ---
> >  hw/pci-bridge/pci_expander_bridge.c | 9 +
> >  include/hw/pci/pcie_port.h  | 1 +
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 47aaaf8fd1..3d896dd452 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -15,6 +15,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_bus.h"
> >  #include "hw/pci/pci_host.h"
> > +#include "hw/pci/pcie_port.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/pci/pci_bridge.h"
> >  #include "qemu/range.h"
> > @@ -233,7 +234,15 @@ static void pxb_dev_realize_common(PCIDevice
> > *dev, bool pcie, Error **errp)
> >
> >  ds = qdev_create(NULL, TYPE_PXB_HOST);
> >  if (pcie) {
> > +#ifdef __aarch64__
> > +bus = pci_root_bus_new(ds, "pxb-pcie-internal",
> > +   NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
> > +bds = qdev_create(BUS(bus), "pcie-root-port");
> > +bds->id = dev_name;
> > +qdev_prop_set_uint8(bds, PCIE_ROOT_PORT_PROP_CHASSIS,
> > +pxb->bus_nr); #else
> >  bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0,
> > TYPE_PXB_PCIE_BUS);
> > +#endif
> >  } else {
> >  bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> TYPE_PXB_BUS);
> >  bds = qdev_create(BUS(bus), "pci-bridge"); diff --git
> > a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h index
> > 4b3d254b08..b41d473220 100644
> > --- a/include/hw/pci/pcie_port.h
> > +++ b/include/hw/pci/pcie_port.h
> > @@ -64,6 +64,7 @@ int pcie_chassis_add_slot(struct PCIESlot *slot);
> > void pcie_chassis_del_slot(PCIESlot *s);
> >
> >  #define TYPE_PCIE_ROOT_PORT "pcie-root-port-base"
> > +#define PCIE_ROOT_PORT_PROP_CHASSIS "chassis"
> >  #define PCIE_ROOT_PORT_CLASS(klass) \
> >   OBJECT_CLASS_CHECK(PCIERootPortClass, (klass),
> > TYPE_PCIE_ROOT_PORT)  #define PCIE_ROOT_PORT_GET_CLASS(obj) \
> > --
> > 2.19.1
> >
> >
> >
> 
> Regards,
> Daniel
> --
> |: https://berrange.com  -o-
> https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-
> https://www.instagram.com/dberrange :|

Regards,
Miao