Re: [Qemu-devel] [RFC v2 3/3] acpi-build: describe new pci domain in AML

[Zihan Yang <whois.zihan.yang@gmail.com>]

Marcel Apfelbaum <marcel.apfelbaum@gmail.com> 于2018年6月20日周三 下午3:46写道：
>
>
>
> On 06/12/2018 12:13 PM, Zihan Yang wrote:
> > Describe new pci segments of host bridges in AML. The host bridge list is
> > replaced by QTAILQ to let q35 host be processed first in every traverse
> >
> > Signed-off-by: Zihan Yang <whois.zihan.yang@gmail.com>
> > ---
> >   hw/i386/acpi-build.c      | 69 ++++++++++++++++++++++++++++++-----------------
> >   hw/pci/pci.c              |  9 ++++---
> >   include/hw/pci/pci_host.h |  2 +-
> >   3 files changed, 50 insertions(+), 30 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 104e52d..a9f1503 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2123,36 +2123,55 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >       sb_scope = aml_scope("\\_SB");
> >       {
> >           Object *pci_host;
> > +        QObject *o;
> >           PCIBus *bus = NULL;
> > +        uint32_t domain_nr;
> > +        bool q35host = true;
> >
> >           pci_host = acpi_get_i386_pci_host();
> > -        if (pci_host) {
> > +        while (pci_host) {
> > +            o = object_property_get_qobject(pci_host, "domain_nr", NULL);
> > +            assert(o);
> > +            domain_nr = qnum_get_uint(qobject_to(QNum, o));
> > +            qobject_unref(o);
> > +
> > +            /* skip expander bridges that still reside in domain 0 */
> > +            if (!q35host && domain_nr == 0) {
> > +                pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next));
>
> Why do you skip pci_hosts without domain? We still want to add them to
> the DSDT, right ?

I think I might misunderstand it. Currently QEMU seems to treat expander
host bridge as normal a PCIe device under main system bus. I thought we
want to preserve current behavior, so my purpose is to only add expander
bridges with a non-zero domain_nr into DSDT, and let the other bridges
be the PCIe devices of these bridges. Do you mean we should add every
host bridge no matter we want it to reside in a different domain or not?

> > +                continue;
> > +            }
> >               bus = PCI_HOST_BRIDGE(pci_host)->bus;
> > -        }
> >
> > -        if (bus) {
> > -            Aml *scope = aml_scope("PCI0");
> > -            /* Scan all PCI buses. Generate tables to support hotplug. */
> > -            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> > +            if (bus) {
> > +                Aml *scope = aml_scope("PCI0");
> > +                aml_append(scope, aml_name_decl("_SEG", aml_int(domain_nr)));
> > +                /* For simplicity, base bus number starts from 0 */
> > +                aml_append(scope, aml_name_decl("_BBN", aml_int(0)));
>
> Nice. _SEG and _BBN should be enough to let the guest know we have multiple
> PCI domains.
>
> > +                /* Scan all PCI buses. Generate tables to support hotplug. */
> > +                build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> >
> > -            if (TPM_IS_TIS(tpm_find())) {
> > -                dev = aml_device("ISA.TPM");
> > -                aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> > -                aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> > -                crs = aml_resource_template();
> > -                aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> > -                           TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> > -                /*
> > -                    FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
> > -                    Rewrite to take IRQ from TPM device model and
> > -                    fix default IRQ value there to use some unused IRQ
> > -                 */
> > -                /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> > -                aml_append(dev, aml_name_decl("_CRS", crs));
> > -                aml_append(scope, dev);
> > +                if (TPM_IS_TIS(tpm_find())) {
> > +                    dev = aml_device("ISA.TPM");
> > +                    aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
> > +                    aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> > +                    crs = aml_resource_template();
> > +                    aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
> > +                               TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
> > +                    /*
> > +                        FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
> > +                        Rewrite to take IRQ from TPM device model and
> > +                        fix default IRQ value there to use some unused IRQ
> > +                     */
> > +                    /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
> > +                    aml_append(dev, aml_name_decl("_CRS", crs));
> > +                    aml_append(scope, dev);
> > +                }
> > +
> > +                aml_append(sb_scope, scope);
> >               }
> > -
> > -            aml_append(sb_scope, scope);
> > +            /* q35 host is the first one in the tail queue */
> > +            q35host = false;
>
> I don't think you should use this hack. Add a domain_nr property to the
> q35 host
> and set it always to 0. Then you don't need special cases.

This looks the same as the problem above, if each host bridge will be added
into DSDT regardless of its domain_nr, then no special cases are needed.

> > +            pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next));
> >           }
> >       }
> >
> > @@ -2645,7 +2664,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void)
> >           qobject_unref(o);
> >           /* skip q35 host and bridges that reside in the same domain with it */
> >           if (domain_nr == 0) {
> > -            pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next));
> > +            pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next));
> >               continue;
> >           }
> >
> > @@ -2674,7 +2693,7 @@ static AcpiMcfgInfo *acpi_get_mcfg(void)
> >           assert(o);
> >           mcfg->domain_nr = qnum_get_uint(qobject_to(QNum, o));
> >
> > -        pci_host = OBJECT(QLIST_NEXT(PCI_HOST_BRIDGE(pci_host), next));
> > +        pci_host = OBJECT(QTAILQ_NEXT(PCI_HOST_BRIDGE(pci_host), next));
> >       }
> >
> >       return head;
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 80bc459..f63385f 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -196,7 +196,8 @@ static void pci_del_option_rom(PCIDevice *pdev);
> >   static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET;
> >   static uint16_t pci_default_sub_device_id = PCI_SUBDEVICE_ID_QEMU;
> >
> > -static QLIST_HEAD(, PCIHostState) pci_host_bridges;
> > +static QTAILQ_HEAD(, PCIHostState) pci_host_bridges =
> > +    QTAILQ_HEAD_INITIALIZER(pci_host_bridges);
> >
> >   int pci_bar(PCIDevice *d, int reg)
> >   {
> > @@ -330,7 +331,7 @@ static void pci_host_bus_register(DeviceState *host)
> >   {
> >       PCIHostState *host_bridge = PCI_HOST_BRIDGE(host);
> >
> > -    QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
> > +    QTAILQ_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
> >   }
> >
> >   PCIBus *pci_device_root_bus(const PCIDevice *d)
> > @@ -1798,7 +1799,7 @@ PciInfoList *qmp_query_pci(Error **errp)
> >       PciInfoList *info, *head = NULL, *cur_item = NULL;
> >       PCIHostState *host_bridge;
> >
> > -    QLIST_FOREACH(host_bridge, &pci_host_bridges, next) {
> > +    QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) {
> >           info = g_malloc0(sizeof(*info));
> >           info->value = qmp_query_pci_bus(host_bridge->bus,
> >                                           pci_bus_num(host_bridge->bus));
> > @@ -2493,7 +2494,7 @@ int pci_qdev_find_device(const char *id, PCIDevice **pdev)
> >       PCIHostState *host_bridge;
> >       int rc = -ENODEV;
> >
> > -    QLIST_FOREACH(host_bridge, &pci_host_bridges, next) {
> > +    QTAILQ_FOREACH(host_bridge, &pci_host_bridges, next) {
> >           int tmp = pci_qdev_find_recursive(host_bridge->bus, id, pdev);
> >           if (!tmp) {
> >               rc = 0;
> > diff --git a/include/hw/pci/pci_host.h b/include/hw/pci/pci_host.h
> > index ba31595..a5617cf 100644
> > --- a/include/hw/pci/pci_host.h
> > +++ b/include/hw/pci/pci_host.h
> > @@ -47,7 +47,7 @@ struct PCIHostState {
> >       uint32_t config_reg;
> >       PCIBus *bus;
> >
> > -    QLIST_ENTRY(PCIHostState) next;
> > +    QTAILQ_ENTRY(PCIHostState) next;
> >   };
> >
> >   typedef struct PCIHostBridgeClass {
>
> Looking good, do you have something working?

Not yet, but I will finish my last exam next week, then I will try to get
seabios part working as soon as possible. Sorry for the delay.

> Thanks,
> Marcel
>