Re: [XEN PATCH 2/9] x86: don't access x86_cpu_to_apicid[] directly, use cpu_physical_id(cpu)

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

On 7.02.2024 17:28, Jan Beulich wrote:

> On 14.11.2023 18:49, Krystian Hebel wrote:
> > --- a/xen/arch/x86/apic.c
> > +++ b/xen/arch/x86/apic.c
> > @@ -950,7 +950,7 @@ __next:
> >       */
> >      if (boot_cpu_physical_apicid == -1U)
> >          boot_cpu_physical_apicid = get_apic_id();
> > -    x86_cpu_to_apicid[0] = get_apic_id();
> > +    cpu_physical_id(0) = get_apic_id();
> >       
> While personally I don't mind as much, I expect Andrew would not like
> this: Something that looks like a function call on the lhs is against
> what normal language structure would be.
>     
    This made me cringe as well, but I've seen something like this used
    in
    other places (per_cpu() mostly) so I thought it was OK. I can change
    it.

> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1615,7 +1615,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              break;
> >  
> >          cpu_id.phys_id =
> > -            (uint64_t)x86_cpu_to_apicid[v->vcpu_id] |
> > +            (uint64_t)cpu_physical_id(v->vcpu_id) |
> >              ((uint64_t)acpi_get_processor_id(v->vcpu_id) << 32);
> >       
> While the cast on the 2nd line is necessary, the one on the 2st isn't
> and would be nice to be dropped while touching the line anyway.
>     
    Ack
> > --- a/xen/arch/x86/numa.c
> > +++ b/xen/arch/x86/numa.c
> > @@ -70,7 +70,7 @@ void __init init_cpu_to_node(void)
> >  
> >      for ( i = 0; i < nr_cpu_ids; i++ )
> >      {
> > -        u32 apicid = x86_cpu_to_apicid[i];
> > +        u32 apicid = cpu_physical_id(i);
> >          if ( apicid == BAD_APICID )
> >              continue;
> >          node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] : NUMA_NO_NODE;
> >       
> We're in the process of phasing out u32 and friends, favoring uint32_t.
> Would be nice if in code being touched anyway (i.e. not just here) the
> conversion would be done right away. Then again fixed-width types are
> preferably avoided where not really needed (see ./CODING_STYLE), so
> quite likely it actually wants to be unsigned int here.
>
> Furthermore our style demands that declaration(s) and statement(s) are
> separated by a blank line. Inserting the missing one in cases like the
> one here would be very desirable as well.
>     
    Good to know, thanks.

> Jan
>     
    Best regards,
-- 
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com