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;
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;
Regards, Roger.
