On 09.10.2024 18:44, Alejandro Vallejo wrote: > On Wed Oct 9, 2024 at 2:28 PM BST, Jan Beulich wrote: >> On 01.10.2024 14:38, Alejandro Vallejo wrote: >>> If toolstack were to upload LAPIC contexts as part of domain creation it >> >> If it were to - yes. But it doesn't, an peeking ahead in the series I also >> couldn't spot this changing. Hence I don#t think I see why this change >> would be needed, and why ... > > Patch 10 does. It's the means by which (in a rather roundabout way) > toolstack overrides vlapic->hw.x2apic_id.
Oh, indeed - I managed to not spot this. I think you want to either re-word the description here to make clear there's actually a plan to do what is being said as purely hypothetical, or simply fold the patches. >>> would encounter a problem were the architectural state does not reflect >>> the APIC ID in the hidden state. This patch ensures updates to the >>> hidden state trigger an update in the architectural registers so the >>> APIC ID in both is consistent. >>> >>> Signed-off-by: Alejandro Vallejo <[email protected]> >>> --- >>> xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> >>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c >>> index 02570f9dd63a..a8183c3023da 100644 >>> --- a/xen/arch/x86/hvm/vlapic.c >>> +++ b/xen/arch/x86/hvm/vlapic.c >>> @@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain >>> *d, hvm_domain_context_t *h) >>> >>> s->loaded.hw = 1; >>> if ( s->loaded.regs ) >>> + { >>> + /* >>> + * We already processed architectural regs in lapic_load_regs(), so >>> + * this must be a migration. Fix up inconsistencies from any older >>> Xen. >>> + */ >>> lapic_load_fixup(s); >>> + } >>> + else >>> + { >>> + /* >>> + * We haven't seen architectural regs so this could be a migration >>> or a >>> + * plain domain create. In the domain create case it's fine to >>> modify >>> + * the architectural state to align it to the APIC ID that was just >>> + * uploaded and in the migrate case it doesn't matter because the >>> + * architectural state will be replaced by the LAPIC_REGS ctx >>> later on. >>> + */ >> >> ... a comment would need to mention a case that never really happens, thus >> only risking to cause confusion. > > I assume the "never really happens" is about the same as the previous > paragraph? If so, the same answer applies. Yes. > About the lack of ordering in the migrate stream the code already makes no > assumptions as to which HVM context blob might appear first in the vLAPIC > area. > > I'm not sure why, but I assumed it may be different on older Xen. I agree with being flexible here; I'm not aware of anything in the migration spec (and certainly not in the unwritten v1 one) mandating particular ordering. Jan
