On Thu Feb 27, 2025 at 7:29 AM GMT, Jan Beulich wrote: > On 26.02.2025 18:33, Roger Pau Monné wrote: > > On Wed, Feb 26, 2025 at 02:11:23PM +0100, Jan Beulich wrote: > >> On 18.02.2025 15:22, Alejandro Vallejo wrote: > >>> Today, Xen hardcodes apic_id = vcpu_id * 2, but this is unwise and > >>> interferes with providing accurate topology information to the guest. > >>> > >>> Introduce a new x2apic_id field into hvm_hw_lapic. This is immutable > >>> state from the guest's point of view, but it will allow the toolstack to > >>> eventually configure the value, and for the value to move on migrate. > >>> > >>> For backwards compatibility, the patch rebuilds the old-style APIC IDs > >>> from migration streams lacking them when they aren't present. > >> > >> Nit: "when they aren't present" looks to duplicate "lacking them"? > >> > >>> Signed-off-by: Alejandro Vallejo <alejandro.vall...@cloud.com> > >>> --- > >>> I've split this one from the rest of the topology series as it's > >>> independent > >>> and entangled with another patch from Andrew. > >> > >> Albeit I think meanwhile we've settled that the entangling isn't quite as > >> problematic. > >> > >>> @@ -1621,6 +1624,14 @@ static int cf_check lapic_load_hidden(struct > >>> domain *d, hvm_domain_context_t *h) > >>> return -EINVAL; > >>> } > >>> > >>> + /* > >>> + * Xen 4.20 and earlier had no x2APIC ID in the migration stream and > >>> + * hard-coded "vcpu_id * 2". Default back to this if we have a > >>> + * zero-extended record. > >>> + */ > >>> + if ( h->size <= offsetof(struct hvm_hw_lapic, x2apic_id) ) > >>> + s->hw.x2apic_id = v->vcpu_id * 2; > >> > >> While we better wouldn't get to see such input, it is in principle possible > >> to have an input stream with, say, half the field. Imo the condition ought > >> to be such that we'd make the adjustment when less than the full field is > >> available. > > > > I would add an additional check to ensure _rsvd0 remains 0, to avoid > > further additions from attempting to reuse that padding space. > > > > if ( s->hw._rsvd0 ) > > return -EINVAL; > > I agree we want such a check; I actually should have pointed that out, too. > I don't, however, see why the field couldn't be re-used going forward (under > the right conditions, of course).
It could be reused indeed, but at the point of making use of it we'd remove the check. Cheers, Alejandro
