Re: [PATCH] x86/hvm: Add APIC IDs to the per-vLAPIC save area

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

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.vallejo@xxxxxxxxx>
> >>> ---
> >>> 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