On Tue, 2007-11-27 at 02:02 -0800, Andrew Morton wrote:
> > +
> > +static pgd_t save_pgd __initdata;
> > +static unsigned long efi_flags __initdata;
> > +/* efi_lock protects efi physical mode call */
> > +static __initdata DEFINE_SPINLOCK(efi_lock);
>
> It's peculiar to have a spinlock in __initdata. Often there just isn't any
> code path by which multiple threads/CPUs can access the same data that
> early in boot.
Yes. This spinlock is used only before efi_enter_virtual_mode, which is
far before smp_init, so this spinlock is unnecessary, I will remove it.
> > +void __init efi_call_phys_prelog(void) __acquires(efi_lock)
> > +{
> > + unsigned long vaddress;
> > +
> > + /*
> > + * Lock sequence is different from normal case because
> > + * efi_flags is global
> > + */
> > + spin_lock(&efi_lock);
> > + local_irq_save(efi_flags);
>
> I think we discussed this before, but I forget the result. It really
> should be described better in the comments here, because this code leaps out
> and shouts "wrong".
>
> a) Why not use spin_lock_irqsave()?
>
> b) If this is an open-coded spin_lock_irqsave() then it gets the two
> operations in the wrong order and is hence deadlockable.
>
> c) it isn't obvious to the reader that this locking is even needed in
> initial bootup.
>
> Now I _think_ all these issuses were addressed in discussion. But unless
> the code comment knocks them all on the head (it doesn't) then it will all
> come up again.
Because the efi_lock will removed, so this will be no longer a problem.
> > + early_runtime_code_mapping_set_exec(1);
> > + vaddress = (unsigned long)__va(0x0UL);
> > + pgd_val(save_pgd) = pgd_val(*pgd_offset_k(0x0UL));
> > + set_pgd(pgd_offset_k(0x0UL), *pgd_offset_k(vaddress));
> > + global_flush_tlb();
> > +}
> > +
> > +void __init efi_call_phys_epilog(void) __releases(efi_lock)
> > +{
> > + /*
> > + * After the lock is released, the original page table is restored.
> > + */
> > + set_pgd(pgd_offset_k(0x0UL), save_pgd);
> > + early_runtime_code_mapping_set_exec(0);
> > + global_flush_tlb();
> > + local_irq_restore(efi_flags);
> > + spin_unlock(&efi_lock);
> > +}
> > +
> >
> > ...
> >
> > +void __init runtime_code_page_mkexec(void)
> > +{
> > + efi_memory_desc_t *md;
>
> I thought we were going to use `struct efi_memory_desc'?
There is even no struct efi_memory_desc definition in
include/linux/efi.h. I can fix all such coding style problem across all
platforms if desired in another patchset.
> > +#include <asm/setup.h>
> > +#include <asm/efi.h>
> > +#include <asm/time.h>
> > +
> > +#define EFI_DEBUG 0
>
> I suspect you really want to turn on debug mode during initial public
> testing. Verify that it generates sufficient information for you to be
> able to fix problems if/when people report them.
OK, I will do it.
> > +void __init efi_init(void)
> > +{
> > + efi_config_table_t *config_tables;
> > + efi_runtime_services_t *runtime;
> > + efi_char16_t *c16;
> > + char vendor[100] = "unknown";
> > + int i = 0;
> > + void *tmp;
> > +
> > + memset(&efi, 0, sizeof(efi));
> > + memset(&efi_phys, 0, sizeof(efi_phys));
>
> These were already zeroed by the compiler (I have a feeling I said that a
> couple of months back)
I will fix it.
> > +#ifdef CONFIG_X86_32
>
> Strictly this isn't needed until [patch 4/4] but that's a very minor point.
>
> > + efi_phys.systab = (efi_system_table_t *)boot_params.efi_info.efi_systab;
> > + memmap.phys_map = (void *)boot_params.efi_info.efi_memmap;
> > +#else
> > + efi_phys.systab = (efi_system_table_t *)
> > + (boot_params.efi_info.efi_systab |
> > + ((__u64)boot_params.efi_info.efi_systab_hi<<32));
> > + memmap.phys_map = (void *)
> > + (boot_params.efi_info.efi_memmap |
> > + ((__u64)boot_params.efi_info.efi_memmap_hi<<32));
> > +#endif
> > + memmap.nr_map = boot_params.efi_info.efi_memmap_size /
> > + boot_params.efi_info.efi_memdesc_size;
> > + memmap.desc_version = boot_params.efi_info.efi_memdesc_version;
> > + memmap.desc_size = boot_params.efi_info.efi_memdesc_size;
> > +
> > + efi.systab = efi_early_ioremap((unsigned long)efi_phys.systab,
> > + sizeof(efi_system_table_t));
> > + if (efi.systab == NULL)
> > + printk(KERN_ERR "Woah! Couldn't map the EFI systema table.\n");
>
> s/systema/system/.
>
> I'd be inclined to s/Woah! //, too. Sorry, I'm boring.
I will fix it.
> > + memcpy(&efi_systab, efi.systab, sizeof(efi_system_table_t));
> > + efi_early_iounmap(efi.systab, sizeof(efi_system_table_t));
> > + efi.systab = &efi_systab;
> > +
> > + /*
> > + * Verify the EFI Table
> > + */
> > + if (efi.systab->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> > + printk(KERN_ERR "Woah! EFI system table "
> > + "signature incorrect\n");
> > + if ((efi.systab->hdr.revision >> 16) == 0)
> > + printk(KERN_ERR "Warning: EFI system table version "
> > + "%d.%02d, expected 1.00 or greater\n",
> > + efi.systab->hdr.revision >> 16,
> > + efi.systab->hdr.revision & 0xffff);
> > +
> > + /*
> > + * Show what we know for posterity
> > + */
> > + c16 = tmp = efi_early_ioremap(efi.systab->fw_vendor, 2);
> > + if (c16) {
> > + for (i = 0; i < sizeof(vendor) && *c16; ++i)
> > + vendor[i] = *c16++;
> > + vendor[i] = '\0';
> > + } else
> > + printk(KERN_ERR "Could not map the firmware vendor!\n");
>
> That would be a very confusing error message to any poor soul who received
> it. Please consider prefixing all such things with (say) "efi: ".
I will do it.
> > +/*
> > + * This function will switch the EFI runtime services to virtual mode.
> > + * Essentially, look through the EFI memmap and map every region that
> > + * has the runtime attribute bit set in its memory descriptor and update
> > + * that memory descriptor with the virtual address obtained from ioremap().
> > + * This enables the runtime services to be called without having to
> > + * thunk back into physical mode for every invocation.
> > + */
> > +void __init efi_enter_virtual_mode(void)
> > +{
> > + efi_memory_desc_t *md;
> > + efi_status_t status;
> > + unsigned long end;
> > + void *p;
> > +
> > + efi.systab = NULL;
> > + for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> > + md = p;
> > + if (!(md->attribute & EFI_MEMORY_RUNTIME))
> > + continue;
> > + if ((md->attribute & EFI_MEMORY_WB) &&
> > + (((md->phys_addr + (md->num_pages<<EFI_PAGE_SHIFT)) >>
> > + PAGE_SHIFT) < end_pfn_map))
> > + md->virt_addr = (unsigned long)__va(md->phys_addr);
> > + else
> > + md->virt_addr = (unsigned long)
> > + efi_ioremap(md->phys_addr,
> > + md->num_pages << EFI_PAGE_SHIFT);
> > + if (!md->virt_addr)
> > + printk(KERN_ERR "ioremap of 0x%llX failed!\n",
> > + (unsigned long long)md->phys_addr);
> > + end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
> > + if ((md->phys_addr <= (unsigned long)efi_phys.systab) &&
> > + ((unsigned long)efi_phys.systab < end))
> > + efi.systab = (efi_system_table_t *)(unsigned long)
> > + (md->virt_addr - md->phys_addr +
> > + (unsigned long)efi_phys.systab);
> > + }
> > +
> > + BUG_ON(!efi.systab);
> > +
> > + status = phys_efi_set_virtual_address_map(
> > + memmap.desc_size * memmap.nr_map,
> > + memmap.desc_size,
> > + memmap.desc_version,
> > + memmap.phys_map);
> > +
> > + if (status != EFI_SUCCESS) {
> > + printk(KERN_ALERT "You are screwed! "
>
> This came over when you copied the original file. This patchset would be a
> decent opportunity to de-stupid these messages. Frankly.
I will do it. And I will recheck all messages.
Best Regards,
Huang Ying
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
