Re: [SeaBIOS] [PATCH v3 5/8] seabios: add q35 initialization functions.

[Kevin O'Connor]

On Fri, Oct 19, 2012 at 04:41:01PM -0400, Jason Baron wrote:
> From: Isaku Yamahata
>
> 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