[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 <yamahata at 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