On 23.11.2023 18:30, Alejandro Vallejo wrote:
> @@ -1498,27 +1511,36 @@ static int cf_check lapic_save_regs(struct vcpu *v,
> hvm_domain_context_t *h)
> */
> static void lapic_load_fixup(struct vlapic *vlapic)
> {
> - uint32_t id = vlapic->loaded.id;
> + uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id);
>
> - if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 )
> - {
> + /* Skip fixups on xAPIC mode, or if the x2APIC LDR is already correct */
> + if ( !vlapic_x2apic_mode(vlapic) ||
> + (vlapic->loaded.ldr == good_ldr) )
> + return;
> +
> + if ( vlapic->loaded.ldr == 1 )
> + /*
> + * Xen <= 4.4 may have a bug by which all the APICs configured in
> + * x2APIC mode got LDR = 1. We can't leave it as-is because it
> + * assigned the same LDR to every CPU. We'll fix fix the bug now
> + * and assign an LDR value consistent with the APIC ID.
> + */
Just one comment on top of Andrew's: Is the double "fix" really intended
here? (I could see it might be, but then "fix the bug fix" would read
slightly more smoothly to me as a non-native speaker.)
Another aspect here is what exactly the comment states (and does not
state). Original comments made explicit that LDR == 1 contradicts ID == 0.
In the new comment you only emphasize that all CPUs cannot have that same
LDR. But the value of 1 being bogus in the first place doesn't become clear
anymore.
Jan
