Hi, On Wed Feb 26, 2025 at 5:33 PM GMT, 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;
That's already on lapic_check_hidden(), so it's guaranteed to be zero. Unless you mean something else? > > if ( s->hw._rsvd0 ) > return -EINVAL; > > In fact I would be tempted to overwrite the ID if the stream size > doesn't match the expected one, ie: > > if ( h->size < (offsetof(struct hvm_hw_lapic, _rsvd0) + > sizeof(s->hw._rsvd0)) ) > s->hw.x2apic_id = v->vcpu_id * 2; That looks better. I'll do that instead. > > Regards, Roger. Cheers, Alejandro
