On Fri, Oct 19, 2012 at 04:41:01PM -0400, Jason Baron wrote:
> From: Isaku Yamahata <yamahata(a)valinux.co.jp>
>
> add q35 initialization functions.
>
Thanks. See comments below.
[...]
> +/* PCI_VENDOR_ID_INTEL && DEVICE_ID_INTEL_Q35_MCH */
> +void mch_mcfg_find(struct pci_device *dev, void *arg)
> +{
> + struct acpi_mcfg *mcfg = arg;
> +
> + mcfg->nr = 1;
> +}
> +
> +/* PCI_VENDOR_ID_INTEL && DEVICE_ID_INTEL_Q35_MCH */
> +void mch_mcfg_init(struct pci_device *dev, void *arg)
> +{
> + u16 bdf = dev->bdf;
> +
> + u64 addr = Q35_HOST_BRIDGE_PCIEXBAR_ADDR | Q35_HOST_BRIDGE_PCIEXBAREN;
> + u32 upper = addr >> 32;
> + u32 lower = addr & 0xffffffff;
> +
> + /* at first disable the region. and then update/enable it. */
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, 0);
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR + 4, upper);
> + pci_config_writel(bdf, Q35_HOST_BRIDGE_PCIEXBAR, lower);
> +
> + struct acpi_mcfg *mcfg = arg;
> + struct acpi_mcfg_allocation *alloc = &mcfg->mcfg->allocation[0];
> + alloc->address = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> + alloc->pci_segment = Q35_HOST_PCIE_PCI_SEGMENT;
> + alloc->start_bus_number = Q35_HOST_PCIE_START_BUS_NUMBER;
> + alloc->end_bus_number = Q35_HOST_PCIE_END_BUS_NUMBER;
> +
> + mcfg->e820->start = Q35_HOST_BRIDGE_PCIEXBAR_ADDR;
> + mcfg->e820->size = Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> + mcfg->e820->type = E820_RESERVED;
> +}
> +
> static const struct pci_device_id mcfg_find_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mcfg_find),
> PCI_DEVICE_END,
> };
>
> static const struct pci_device_id mcfg_init_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mcfg_init),
> PCI_DEVICE_END,
> };
As in my previous mail, I think mch_mcfg_init (or similar) should do
all the work necessary to build the final acpi mcfg table and not
spread the logic through build_madt/mch_mcfg_find.
[...]
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -12,6 +12,7 @@
> #include "ioport.h" // PORT_ATA1_CMD_BASE
> #include "config.h" // CONFIG_*
> #include "xen.h" // usingXen
> +#include "dev-q35.h" // usingXen
Stale comment.
[...]
> +void mch_mem_addr_init(struct pci_device *dev, void *arg)
> +{
> + u64 *start = (u64 *)arg;
> + /* mmconfig space */
> + *start = Q35_HOST_BRIDGE_PCIEXBAR_ADDR +
> + Q35_HOST_BRIDGE_PCIEXBAR_SIZE;
> + mtrr_base = *start;
> +}
> +
> +static const struct pci_device_id pci_mem_addr_tbl[] = {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_Q35_MCH,
> + mch_mem_addr_init),
> + PCI_DEVICE_END,
> +};
> +
> static void pci_bios_map_devices(struct pci_bus *busses)
> {
> pcimem_start = RamSize;
>
> + /* let's add in mmconfig space on q35 */
> + pci_find_init_device(pci_mem_addr_tbl, &pcimem_start);
I find this code confusing. Passing a pointer to the global
pcimem_start just obfuscates the assignment, and the setting of
pcimem_start/mtrr_base in pci_bios_map_devices is counter intuitive.
I suggest moving this code up into pci_setup() and performing the
global variable assignments directly.
[...]
> --- a/src/shadow.c
> +++ b/src/shadow.c
> @@ -11,6 +11,7 @@
> #include "pci_ids.h" // PCI_VENDOR_ID_INTEL
> #include "pci_regs.h" // PCI_VENDOR_ID
> #include "xen.h" // usingXen
> +#include "dev-q35.h" // PCI_VENDOR_ID_INTEL
Stale comment.
-Kevin