Re: [PATCH 11/25] xen: Xen SMP guest support

[Jeremy Fitzhardinge <jeremy@goop.org>]

Andi Kleen wrote:
>   
>>  #ifdef CONFIG_X86_LOCAL_APIC
>>  static unsigned long xen_apic_read(unsigned long reg)
>>     
>
> Can't you just make them all NULL? Crashing here should be correct.
>   

No.  The APIC setup code does at least one unconditional read to get the
APIC version before deciding the APIC is disabled/not present.  In my
testing that's the only access to the APIC which happens under Xen, but
it wouldn't surprise me if there were other stray accesses with
different configurations (like the P4_THERMAL stuff).

>> +#ifdef CONFIG_SMP
>> +static const struct smp_ops xen_smp_ops __initdata = {
>>     
>
> Why __initdata? 
>   

It gets copied into the real smp_ops early on, so it isn't needed after
that.

>> +/* VCPUs are single-cored, and have no siblings */
>> +static void set_cpu_sibling_map(int cpu)
>>     
>
> Can you put this somewhere generic and use it everywhere? Don't want
> duplication of this code.
>   

OK.

> Strange check?
>   

Er, yes, that does look odd.

>> +
>> + * We need to reload %cr3 since the page tables may be going
>> + * away from under us..
>> + */
>> +static inline void leave_mm (unsigned long cpu)
>> +{
>> +#ifdef CONFIG_SMP
>> +	if (per_cpu(cpu_tlbstate, cpu).state == TLBSTATE_OK)
>> +		BUG();
>> +	cpu_clear(cpu, per_cpu(cpu_tlbstate, cpu).active_mm->cpu_vm_mask);
>> +#endif
>> +	load_cr3(swapper_pg_dir);
>>  }
>>     
>
> Should be out of line
>   

OK.

    J