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

[miaoyubo]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden]
> Sent: Tuesday, February 25, 2020 9:12 PM
> To: miaoyubo <address@hidden>
> Cc: Philippe Mathieu-Daudé <address@hidden>;
> address@hidden; address@hidden;
> address@hidden; address@hidden; Xiexiangyou
> <address@hidden>; address@hidden
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 12:44:15PM +0000, miaoyubo wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:address@hidden]
> > > Sent: Tuesday, February 25, 2020 8:27 PM
> > > To: miaoyubo <address@hidden>
> > > Cc: Philippe Mathieu-Daudé <address@hidden>;
> > > address@hidden; address@hidden;
> > > address@hidden; address@hidden; Xiexiangyou
> > > <address@hidden>; address@hidden
> > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > for arm
> > >
> > > On Tue, Feb 25, 2020 at 12:12:12PM +0000, miaoyubo wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Philippe Mathieu-Daudé [mailto:address@hidden]
> > > > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > > > To: miaoyubo <address@hidden>; address@hidden;
> > > > > address@hidden
> > > > > Cc: address@hidden; address@hidden; qemu-
> address@hidden;
> > > > > Xiexiangyou <address@hidden>; address@hidden
> > > > > 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 <address@hidden>
> > > > > >
> > > > > > 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 <address@hidden>
> > > > > > ---
> > > > > >   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, &memmap[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 = &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?
> 
> All this needs more comments in the code constructing the tables.
> 

Thanks for replying, I will add more comments in the table construction.

> Also, instead of trying to store bus and spreading logic around like this, how
> about just using object_resolve_path_type?
> 
> 

Thanks for the suggestion, using object_resolve_path_type  seems to be better. 

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

Regards,
Miao