Re: [PATCH] enable x2APIC without interrupt remapping under KVM

[Ingo Molnar <mingo@elte.hu>]

i've only got some small stylistic gripes:

> +void __init enable_IR_x2apic(void)
> +{
> +	unsigned long flags;
> +	struct IO_APIC_route_entry **ioapic_entries = NULL;
> +	int ret, x2apic_enabled = 0;
> +
>  	ioapic_entries = alloc_ioapic_entries();
>  	if (!ioapic_entries) {
> -		pr_info("Allocate ioapic_entries failed: %d\n", ret);
> -		goto end;
> +		pr_info("Allocate ioapic_entries failed\n");
> +		goto out;

That's probably a fatal condition so perhaps a WARN() or pr_err() 
would be more appropriate.

> +	ret = enable_IR();
> +	if (!ret) {
> +		/* IR is required if x2apic is enabled by BIOS even
> +		 * when running in kvm since this indicates present
> +		 * of APIC ID > 255 */

please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle.

> +		if (max_physical_apicid > 255 || !kvm_para_available())
> +			goto nox2apic;
> +		/* without IR all CPUs can be addressed by IOAPIC/MSI
> +		 * only in physical mode */

ditto.

> +		x2apic_phys = 1;
> +	}
>  
> -	pr_info("Enabled Interrupt-remapping\n");
> +	x2apic_enabled = 1;
>  
>  	if (x2apic_supported() && !x2apic_mode) {
>  		x2apic_mode = 1;
> @@ -1414,41 +1437,30 @@ void __init enable_IR_x2apic(void)
>  		pr_info("Enabled x2apic\n");
>  	}
>  
> -end_restore:
> -	if (ret)
> -		/*
> -		 * IR enabling failed
> -		 */
> +nox2apic:
> +	if (!ret) /* IR enabling failed */
>  		restore_IO_APIC_setup(ioapic_entries);
> -
>  	unmask_8259A();
>  	local_irq_restore(flags);
>  
> -end:
> +out:
>  	if (ioapic_entries)
>  		free_ioapic_entries(ioapic_entries);
>  
> -	if (!ret)
> +	if (x2apic_enabled)
>  		return;
>  
> -ir_failed:
> -	if (x2apic_preenabled)
> +	if (x2apic_preenabled) {
> +#ifdef CONFIG_INTR_REMAP
>  		panic("x2apic enabled by bios. But IR enabling failed");
> -	else if (cpu_has_x2apic)
> -		pr_info("Not enabling x2apic,Intr-remapping\n");
>  #else
> -	if (!cpu_has_x2apic)
> -		return;
> -
> -	if (x2apic_preenabled)
>  		panic("x2apic enabled prior OS handover,"
> -		      " enable CONFIG_X86_X2APIC, CONFIG_INTR_REMAP");
> +				" enable CONFIG_X86_X2APIC, CONFIG_INTR_REMAP");
>  #endif

can this #ifdef be avoided?

> -
> -	return;
> +	} else if (cpu_has_x2apic)
> +		pr_info("Not enabling x2apic,Intr-remapping\n");

please put a space after commas.

Thanks,

	Ingo