Re: [PATCH 3/8] SFI: core support

From: Andi Kleen
Date: Tue Jun 23 2009 - 08:33:07 EST


Len Brown <lenb@xxxxxxxxxx> writes:

> +static ulong __init sfi_early_find_syst(void)
> +{
> + unsigned long i;
> + char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
> +
> + for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END; i += 16, pchar += 16) {
> + if (!strncmp(SFI_SIG_SYST, pchar, SFI_SIGNATURE_SIZE))
> + return SFI_SYST_SEARCH_BEGIN + i;


Such additional memory scans are always a bit risky, e.g. if there's
stray hardware there. Has it been verified that existing kernels
already scan this area?



> + mmapt = NULL;
> + for (i = 0; i < tbl_cnt; i++) {
> + if (!strncmp(SFI_SIG_MMAP, (char *)(u32)*pentry, 4)) {
> + mmapt = (struct sfi_table_simple *)(u32)*pentry;
> + break;
> + }
> + pentry++;
> + }
> + if (!mmapt)
> + return -1;

printk here?

> +
> + /* refer copy_e820_memory() */
> + num = SFI_GET_ENTRY_NUM(mmapt, sfi_mem_entry);
> + mentry = (struct sfi_mem_entry *)mmapt->pentry;
> + for (i = 0; i < num; i++) {
> + start = mentry->phy_start;
> + size = mentry->pages << PAGE_SHIFT;
> + end = start + size;
> +
> + if (start > end)
> + return -1;
> +
> + pr_debug(PREFIX "start = 0x%08x end = 0x%08x type = %d\n",
> + (u32)start, (u32)end, mentry->type);
> +
> + /* translate SFI mmap type to E820 map type */
> + switch (mentry->type) {
> + case EFI_CONVENTIONAL_MEMORY:
> + type = E820_RAM;
> + break;
> + case EFI_MEMORY_MAPPED_IO:
> + case EFI_UNUSABLE_MEMORY:
> + case EFI_RUNTIME_SERVICES_DATA:
> + mentry++;


Surely UNUSTABLE and RUNTIME_SERVICES should be added somewhere to the resources,
otherwise MMIO allocation might put something there.

> + }
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_X86_LOCAL_APIC
> +void __init mp_sfi_register_lapic_address(u64 address)
> +{
> + mp_lapic_addr = (unsigned long) address;
> +
> + set_fixmap_nocache(FIX_APIC_BASE, mp_lapic_addr);
> +
> + if (boot_cpu_physical_apicid == -1U)
> + boot_cpu_physical_apicid = read_apic_id();
> +
> + pr_info(PREFIX "Boot CPU = %d\n", boot_cpu_physical_apicid);

That's the same what ACPI does, isn't it? Some code sharing
would be good.

> +static int __init sfi_parse_cpus(struct sfi_table_header *table)
> +{
> + struct sfi_table_simple *sb;
> + struct sfi_cpu_table_entry *pentry;
> + int i;
> + int cpu_num;
> +
> + BUG_ON(!table);

Another useless BUG_ON. Some more below too.

> + sb = (struct sfi_table_simple *)table;
> +
> + cpu_num = SFI_GET_ENTRY_NUM(sb, sfi_cpu_table_entry);
> + pentry = (struct sfi_cpu_table_entry *) sb->pentry;
> +
> + for (i = 0; i < cpu_num; i++) {
> + mp_sfi_register_lapic(pentry->apicid);
> + pentry++;
> + }

Array overflow checking?


> + mp_ioapics[idx].apicver = 0;
> +#endif
> +
> + pr_info(PREFIX "IOAPIC[%d]: apic_id %d, version %d, address 0x%x\n",
> + idx, mp_ioapics[idx].apicid,
> + mp_ioapics[idx].apicver, (u32)mp_ioapics[idx].apicaddr);
> + /*
> + * Build basic GSI lookup table to facilitate gsi->io_apic lookups
> + * and to prevent reprogramming of IOAPIC pins (PCI GSIs).
> + */
> + mp_ioapic_routing[idx].apic_id = mp_ioapics[idx].apicid;
> + mp_ioapic_routing[idx].gsi_base = gsi_base;
> + mp_ioapic_routing[idx].gsi_end = gsi_base +
> + io_apic_get_redir_entries(idx);
> + gsi_base = mp_ioapic_routing[idx].gsi_end + 1;
> + pr_info(PREFIX "IOAPIC[%d]: apic_id %d, version %d, address 0x%x, "
> + "GSI %d-%d\n", idx, mp_ioapics[idx].apicid,
> + mp_ioapics[idx].apicver, (u32)mp_ioapics[idx].apicaddr,
> + mp_ioapic_routing[idx].gsi_base,
> + mp_ioapic_routing[idx].gsi_end);

That printk should be dependent on the runtime apic verbosity level?

> +
> +/*
> + * flag for whether using ioremap() to map the sfi tables, if yes
> + * each table only need be mapped once, otherwise each arch's
> + * early_ioremap and early_iounmap should be used each time a
> + * table is visited
> + */
> +static u32 sfi_tbl_permanant_mapped;

permanant is a typo?

> +
> +static void __iomem *sfi_map_memory(u32 phys, u32 size)
> +{
> + if (!phys || !size)
> + return NULL;
> +
> + if (sfi_tbl_permanant_mapped)



> + return ioremap((unsigned long)phys, size);
> + else
> + return arch_early_ioremap((unsigned long)phys, size);
> +}

imho it would be cleaner if the callers just called these functions
directly. Are the !phys !size checks really needed?


> +
> +void sfi_tb_install_table(u64 addr, u32 flags)
> +{
> + struct sfi_table_header *table;
> + u32 length;
> +
> + /* only map table header before knowing actual length */
> + table = sfi_map_memory(addr, sizeof(struct sfi_table_header));
> + if (!table)
> + return;
> +
> + length = table->length;
> + sfi_unmap_memory(table, sizeof(struct sfi_table_header));
> +
> + table = sfi_map_memory(addr, length);
> + if (!table)
> + return;


Since the mappings are always 4K you would only need to remap
if the size is > PAGE_SIZE

> +
> + if (sfi_tb_verify_checksum(table, length))
> + goto unmap_and_exit;
> +
> + /* Initialize sfi_tblist entry */
> + sfi_tblist.tables[sfi_tblist.count].flags = flags;
> + sfi_tblist.tables[sfi_tblist.count].address = addr;
> + sfi_tblist.tables[sfi_tblist.count].pointer = NULL;
> + memcpy(&sfi_tblist.tables[sfi_tblist.count].header,
> + table, sizeof(struct sfi_table_header));

To be honest I'm not sure why this list exists at all.
Is it that difficult to just rewalk the firmware supplied
table as needed?


-andi
--
ak@xxxxxxxxxxxxxxx -- Speaking for myself only.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/