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

Ingo Molnar
2009-07-18T14:07:27Z

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