Re: [PATCH v2 7/8] xen/x86: Derive topologically correct x2APIC IDs from the policy

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 24, 2024 at 06:03:22PM +0100, Alejandro Vallejo wrote:
> On 24/05/2024 09:39, Roger Pau Monné wrote:
> > On Wed, May 08, 2024 at 01:39:26PM +0100, Alejandro Vallejo wrote:
> > 
> > Also you could initialize x2apic_id at definition:
> > 
> > const struct test *t = &tests[j];
> > struct cpu_policy policy = { .x86_vendor = vendors[i] };
> > int rc = x86_topo_from_parts(&policy, t->threads_per_core, 
> > t->cores_per_pkg);
> > uint32_t x2apic_id = x86_x2apic_id_from_vcpu_id(&policy, t->vcpu_id);
> 
> Seeing this snippet I just realized there's a bug. The second loop
> should use j rather than i. Ugh.

Well, you shadow the outer variable with the inner one, which makes it
still fine.  Yet I don't like that shadowing much.  I was going to
comment, but for the requested change you need to not shadow the outer
loop variable (in the example chunk I've used 'j' to signal the outer
loop index).

> >> +}
> >> +
> >>  uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t 
> >> id)
> >>  {
> >> +    uint32_t shift = 0, x2apic_id = 0;
> >> +
> >> +    /* In the absence of topology leaves, fallback to traditional mapping 
> >> */
> >> +    if ( !p->topo.subleaf[0].type )
> >> +        return id * 2;
> >> +
> >>      /*
> >> -     * TODO: Derive x2APIC ID from the topology information inside `p`
> >> -     *       rather than from vCPU ID. This bodge is a temporary measure
> >> -     *       until all infra is in place to retrieve or derive the initial
> >> -     *       x2APIC ID from migrated domains.
> > 
> > I'm a bit confused with this, the policy is domain wide, so we will
> > always need to pass the vCPU ID into x86_x2apic_id_from_vcpu_id()?
> > IOW: the x2APIC ID will always be derived from the vCPU ID.
> > 
> > Thanks, Roger.
> 
> The x2APIC ID is derived (after the series) from the vCPU ID _and_ the
> topology information. The vCPU alone will work out in all cases because
> it'll be cached in the vlapic hvm structure.
> 
> I guess the comment could be rewritten as "... rather than from the vCPU
> ID alone..."

Yup, that's clearer :).

Thanks, Roger.