Marcel Apfelbaum <marcel.apfelb...@gmail.com> 于2018年8月25日周六 下午8:08写道：
>
> Hi Zihan,
>
> On 08/19/2018 04:51 AM, Zihan Yang wrote:
> > Hi Marcel,
> >
> > Marcel Apfelbaum <marcel.apfelb...@gmail.com> 于2018年8月18日周六 上午1:14写道：
> >> Hi Zihan,
> >>
> >> On 08/09/2018 09:33 AM, Zihan Yang wrote:
> >>> The inner host bridge created by pxb-pcie is TYPE_PXB_PCI_HOST by default,
> >>> change it to a new type TYPE_PXB_PCIE_HOST to better utilize ECAM of PCIe
> >>>
> >>> Signed-off-by: Zihan Yang <whois.zihan.y...@gmail.com>
> >>> ---
> >>> hw/pci-bridge/pci_expander_bridge.c | 127
> >>> ++++++++++++++++++++++++++++++++++--
> >>> 1 file changed, 122 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/hw/pci-bridge/pci_expander_bridge.c
> >>> b/hw/pci-bridge/pci_expander_bridge.c
> >>> index e62de42..6dd38de 100644
> >>> --- a/hw/pci-bridge/pci_expander_bridge.c
> >>> +++ b/hw/pci-bridge/pci_expander_bridge.c
> >>> @@ -15,10 +15,12 @@
> >>> #include "hw/pci/pci.h"
> >>> #include "hw/pci/pci_bus.h"
> >>> #include "hw/pci/pci_host.h"
> >>> +#include "hw/pci/pcie_host.h"
> >>> #include "hw/pci/pci_bridge.h"
> >>> #include "qemu/range.h"
> >>> #include "qemu/error-report.h"
> >>> #include "sysemu/numa.h"
> >>> +#include "qapi/visitor.h"
> >>>
> >>> #define TYPE_PXB_BUS "pxb-bus"
> >>> #define PXB_BUS(obj) OBJECT_CHECK(PXBBus, (obj), TYPE_PXB_BUS)
> >>> @@ -40,11 +42,20 @@ typedef struct PXBBus {
> >>> #define TYPE_PXB_PCIE_DEVICE "pxb-pcie"
> >>> #define PXB_PCIE_DEV(obj) OBJECT_CHECK(PXBDev, (obj),
> >>> TYPE_PXB_PCIE_DEVICE)
> >>>
> >>> +#define PROP_PXB_PCIE_DEV "pxbdev"
> >>> +
> >>> +#define PROP_PXB_PCIE_DOMAIN_NR "domain_nr"
> >>> +#define PROP_PXB_PCIE_MAX_BUS "max_bus"
> >>> +#define PROP_PXB_BUS_NR "bus_nr"
> >>> +#define PROP_PXB_NUMA_NODE "numa_node"
> >>> +
> >>> typedef struct PXBDev {
> >>> /*< private >*/
> >>> PCIDevice parent_obj;
> >>> /*< public >*/
> >>>
> >>> + uint32_t domain_nr; /* PCI domain number, non-zero means separate
> >>> domain */
> >> The commit message suggests this patch is only about
> >> re-factoring of the pxb host type, but you add here more fields.
> >> Please use two separate patches.
> >>
> >>> + uint8_t max_bus; /* max bus number to use(including this one) */
> >> That's a great idea! Limiting the max_bus will save us a lot
> >> of resource space, we will not need 256 buses on pxbs probably.
> >>
> >> My concern is what happens with the current mode.
> >> Currently bus_nr is used to divide PCI domain 0 buses between pxbs.
> >> So if you have a pxb with bus_nr 100, and another with bus_nr 200,
> >> we divide them like this:
> >> main host bridge 0...99
> >> pxb1 100 -199
> >> pxb2 200-255
> >>
> >> What will be the meaning of max_bus if we don't use the domain_nr
> >> parameter?
> >> Maybe it will mean that some bus numbers are not assigned at all, for
> >> example:
> >> pxb1: bus_nr 100, max_bus 150
> >> pxb2: bus_nr 200, max_bus 210
> >>
> >> It may work.
> > Yes, it should mean so. Actually max_bus does not have to be specified
> > if domain_nr
> > is not used, but if users decide to use domain_nr and want to save
> > space, max_bus
> > could be used.
> >
> >>> uint8_t bus_nr;
> >>> uint16_t numa_node;
> >>> } PXBDev;
> >>> @@ -58,6 +69,16 @@ static PXBDev *convert_to_pxb(PCIDevice *dev)
> >>> static GList *pxb_dev_list;
> >>>
> >>> #define TYPE_PXB_HOST "pxb-host"
> >>> +#define TYPE_PXB_PCIE_HOST "pxb-pcie-host"
> >>> +#define PXB_PCIE_HOST_DEVICE(obj) \
> >>> + OBJECT_CHECK(PXBPCIEHost, (obj), TYPE_PXB_PCIE_HOST)
> >>> +
> >>> +typedef struct PXBPCIEHost {
> >>> + PCIExpressHost parent_obj;
> >>> +
> >>> + /* pointers to PXBDev */
> >>> + PXBDev *pxbdev;
> >>> +} PXBPCIEHost;
> >>>
> >>> static int pxb_bus_num(PCIBus *bus)
> >>> {
> >>> @@ -111,6 +132,35 @@ static const char
> >>> *pxb_host_root_bus_path(PCIHostState *host_bridge,
> >>> return bus->bus_path;
> >>> }
> >>>
> >>> +/* Use a dedicated function for PCIe since pxb-host does
> >>> + * not have a domain_nr field */
> >>> +static const char *pxb_pcie_host_root_bus_path(PCIHostState *host_bridge,
> >>> + PCIBus *rootbus)
> >>> +{
> >>> + if (!pci_bus_is_express(rootbus)) {
> >>> + /* pxb-pcie-host cannot reside on a PCI bus */
> >>> + return NULL;
> >>> + }
> >>> + PXBBus *bus = PXB_PCIE_BUS(rootbus);
> >>> +
> >>> + /* get the pointer to PXBDev */
> >>> + Object *obj = object_property_get_link(OBJECT(host_bridge),
> >>> + PROP_PXB_PCIE_DEV, NULL);
> >> I don't think you need a link here.
> >> I think rootbus->parent_dev returns the pxb device.
> >> (See the implementation of pxb_bus_num() )
> > OK, I'll change it in next version.
> >
> >>> +
> >>> + snprintf(bus->bus_path, 8, "%04lx:%02x",
> >>> + object_property_get_uint(obj, PROP_PXB_PCIE_DOMAIN_NR,
> >>> NULL),
> >>> + pxb_bus_num(rootbus));
> >>> + return bus->bus_path;
> >>> +}
> >>> +
> >>> +static void pxb_pcie_host_get_mmcfg_size(Object *obj, Visitor *v, const
> >>> char *name,
> >>> + void *opaque, Error **errp)
> >>> +{
> >>> + PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
> >>> +
> >>> + visit_type_uint64(v, name, &e->size, errp);
> >>> +}
> >>> +
> >>> static char *pxb_host_ofw_unit_address(const SysBusDevice *dev)
> >>> {
> >>> const PCIHostState *pxb_host;
> >>> @@ -142,6 +192,31 @@ static char *pxb_host_ofw_unit_address(const
> >>> SysBusDevice *dev)
> >>> return NULL;
> >>> }
> >>>
> >>> +static void pxb_pcie_host_initfn(Object *obj)
> >>> +{
> >>> + PXBPCIEHost *s = PXB_PCIE_HOST_DEVICE(obj);
> >>> + PCIHostState *phb = PCI_HOST_BRIDGE(obj);
> >>> +
> >>> + memory_region_init_io(&phb->conf_mem, obj, &pci_host_conf_le_ops,
> >>> phb,
> >>> + "pci-conf-idx", 4);
> >>> + memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
> >>> phb,
> >>> + "pci-conf-data", 4);
> >>> +
> >> I don't understand the above. Do you want the pxb to respond to
> >> legacy PCI configuration cycles?
> >> I don't think it will be possible since it is accessible only through
> >> addresses
> >> 0xcf8 and 0xcfc which are already "taken" by the Q35 host brigde.
> >>
> >> More importantly, we don't need them, we want to configure
> >> the PXB using MCFG.
> > I see your comment on later patch, there are two reasons.
> >
> > The first is that seabios uses the port IO to read pci configuration space,
> > but 0xcf8 and 0xcfc is binded to pcie.0 bus as you have pointed out. I think
> > sticking with port io could minimize the modification of seabios, so I use
> > another port pair for pxb hosts. Are you suggesting we use mmio for pxb
> > devices specially?
> >
> > The second is that seabios has not loaded RSDP when doing pci_setup,
> > therefore we don't have the base address of each mcfg entry yet. So I still
> > use the legacy ioport method as a temporary workaround.
> >
>
> As I pointed out in the SeaBIOS series, this issue needs to be addressed
> before continuing.
>
> I repeat it here for QEMU developers, maybe somebody has an idea.
>
> If I understand correctly, the only way SeaBIOS lets us configure the
> devices
> is using the 0xcf8/0xcfc registers.
> Since we don't want at this point to support random IO ports for each PCI
> domain, maybe we can try a different angle:
>
> We don't have to configure the PCI devices residing in PCI domain > 0.
> The only drawback is we won't be able to boot from a PCI device
> belonging to such PCI domain, and maybe is OK.
>
> What we need from SeaBIOS is to 'assign' enough address space for each
> MMCFG and return their addresses to QEMU. Then QEMU can create the
> ACPI tables and let the guest OS configure the PCI devices.
>
> The problem remains the computation of the actual IO/MEM resources
> needed by these devices. (Not the MMCFG table).
> If SeaBIOS can't reach the PCI devices, it can't compute the needed
> resources, so QEMU can't divide the IO/MEM address space between
> the PCI domains.
>
> Any idea would be welcomed.
It is welcomed by me too :)
> Thanks,
> Marcel
>
>
>
>
> [...]
