Re: [PATCH v7 14/16] i386: Use CPUCacheInfo.share_level to encode CPUID[4]

[Zhao Liu]

Hi Xiaoyao,

On Mon, Jan 15, 2024 at 03:00:25PM +0800, Xiaoyao Li wrote:
> Date: Mon, 15 Jan 2024 15:00:25 +0800
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: Re: [PATCH v7 14/16] i386: Use CPUCacheInfo.share_level to encode
>  CPUID[4]
> 
> On 1/15/2024 2:25 PM, Zhao Liu wrote:
> > Hi Xiaoyao,
> > 
> > On Mon, Jan 15, 2024 at 12:25:19PM +0800, Xiaoyao Li wrote:
> > > Date: Mon, 15 Jan 2024 12:25:19 +0800
> > > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > > Subject: Re: [PATCH v7 14/16] i386: Use CPUCacheInfo.share_level to encode
> > >   CPUID[4]
> > > 
> > > On 1/15/2024 11:40 AM, Zhao Liu wrote:
> > > > > > +{
> > > > > > +    uint32_t num_ids = 0;
> > > > > > +
> > > > > > +    switch (share_level) {
> > > > > > +    case CPU_TOPO_LEVEL_CORE:
> > > > > > +        num_ids = 1 << apicid_core_offset(topo_info);
> > > > > > +        break;
> > > > > > +    case CPU_TOPO_LEVEL_DIE:
> > > > > > +        num_ids = 1 << apicid_die_offset(topo_info);
> > > > > > +        break;
> > > > > > +    case CPU_TOPO_LEVEL_PACKAGE:
> > > > > > +        num_ids = 1 << apicid_pkg_offset(topo_info);
> > > > > > +        break;
> > > > > > +    default:
> > > > > > +        /*
> > > > > > +         * Currently there is no use case for SMT and MODULE, so 
> > > > > > use
> > > > > > +         * assert directly to facilitate debugging.
> > > > > > +         */
> > > > > > +        g_assert_not_reached();
> > > > > > +    }
> > > > > > +
> > > > > > +    return num_ids - 1;
> > > > > suggest to just return num_ids, and let the caller to do the -1 work.
> > > > Emm, SDM calls the whole "num_ids - 1" (CPUID.0x4.EAX[bits 14-25]) as
> > > > "maximum number of addressable IDs for logical processors sharing this
> > > > cache"...
> > > > 
> > > > So if this helper just names "num_ids" as max_lp_ids_share_the_cache,
> > > > I'm not sure there would be ambiguity here?
> > > 
> > > I don't think it will.
> > > 
> > > if this function is going to used anywhere else, people will need to keep 
> > > in
> > > mind to do +1 stuff to get the actual number.
> > > 
> > > leaving the -1 trick to where CPUID value gets encoded. let's make this
> > > function generic.
> > 
> > This helper is the complete pattern to get addressable IDs, this is to
> > say, the "- 1" is also the part of this calculation.
> > 
> > Its own meaning is self-consistent and generic enough to meet the common
> > definitions of AMD and Intel.
> 
> OK. I stop bikeshedding on it.
>

Thanks for your review ;-).

Regards,
Zhao