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

[Feng Tang <feng.tang@intel.com>]

On Tue, 23 Jun 2009 15:56:43 +0800
Ingo Molnar <mingo@elte.hu> wrote:


> >
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > +static u64 sfi_lapic_addr __initdata = APIC_DEFAULT_PHYS_BASE;
> > +#endif
> 
> if SFI adds a 'depends on X86_LOCAL_APIC' the ugly #ifdef can be
> dropped.

When Len designed the SFI spec, he considered the possibility of being used by
multiple archs, so we chose not to add a x86 dependency, though adding these
#ifdef does make code ugly :P

> > +{
> > +     unsigned long i;
> 
> ... like here.
> 
> > +     char *pchar = (char *)SFI_SYST_SEARCH_BEGIN;
> > +
> > +     for (i = 0; SFI_SYST_SEARCH_BEGIN + i < SFI_SYST_SEARCH_END;
> > i += 16, pchar += 16) {
> 
> What does the magic constant '16' mean here?

My bad not puting clear comments here, the SFI spec defines SYST table starts
at a 16-byte boundary


> > +
> > +static int __init sfi_parse_apic(struct sfi_table_header *table)
> > +{
> > +     struct sfi_table_simple *sb;
> > +     struct sfi_apic_table_entry *pentry;
> > +     int i, num;
> > +
> > +     BUG_ON(!table);
> 
> Same as comment above - is this case anticipated? If yes, is a crash
> the best answer?

Yes, usually table won't be NULL

> > +
> > +#define SFI_ACPI_TABLE       1
> 
> In general, nice stuff - basically SFI is cleanly implemented ACPI
> tables without any of the run-code-in-acpi-tables complications,
> right?

Thanks for the comments, I really got inspired :). The expectation for SFI
is to be able to run cleanly with CONFIG_ACPI=n, and it works fine on some
intel platform.

Thanks,
Feng

> 
>         Ingo