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

[Michael S. Tsirkin]

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; 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.

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


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