| Message ID | 20240416212216.9605-1-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | None | expand |
On Tue, Apr 16, 2024 at 02:22:16PM -0700, Tony Luck wrote: > New CPU #defines encode vendor and family as well as model. > > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > drivers/hwmon/peci/cputemp.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/peci/cputemp.c b/drivers/hwmon/peci/cputemp.c > index a812c15948d9..28cec5c318d4 100644 > --- a/drivers/hwmon/peci/cputemp.c > +++ b/drivers/hwmon/peci/cputemp.c > @@ -11,6 +11,7 @@ > #include <linux/peci-cpu.h> > #include <linux/units.h> > > +#include <asm/cpu_device_id.h> > #include "common.h" > > #define CORE_NUMS_MAX 64 > @@ -361,9 +362,9 @@ static int init_core_mask(struct peci_cputemp *priv) > > /* Get the RESOLVED_CORES register value */ > switch (peci_dev->info.model) { > - case INTEL_FAM6_ICELAKE_X: > - case INTEL_FAM6_ICELAKE_D: > - case INTEL_FAM6_SAPPHIRERAPIDS_X: > + case VFM_MODEL(INTEL_ICELAKE_X): > + case VFM_MODEL(INTEL_ICELAKE_D): > + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X): $ git describe v6.9-rc4-31-g96fca68c4fbf $ git grep VFM_MODEL $ So I guess this depends on some other patch ? That might be worth mentioning, especially since $ git describe next-20240416 $ git grep VFM_MODEL $ doesn't find it either. On top of that, it looks like #include <asm/cpu_device_id.h> introduces a dependency on X86 which is not currently the case. CONFIG_PECI is typically used on BMCs. Many of those do not use Intel CPUs. It does not seem appropriate to make support for PECI based hardware monitoring dependent on it running on an Intel CPU. Thanks, Guenter
Guenter, Thanks for taking a look at this patch. > > + case VFM_MODEL(INTEL_ICELAKE_X): > > + case VFM_MODEL(INTEL_ICELAKE_D): > > + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X): > > $ git describe > v6.9-rc4-31-g96fca68c4fbf > $ git grep VFM_MODEL > $ > > So I guess this depends on some other patch ? > That might be worth mentioning, especially since > > $ git describe > next-20240416 > $ git grep VFM_MODEL > $ > > doesn't find it either. Yes. This depends on parts 1,2,3 of this series. I should have added that to the "---" meta comment part of these patches that I'm spraying out to maintainers of random subsystems that use INTEL_FAM6 defines. > On top of that, it looks like > > #include <asm/cpu_device_id.h> > > introduces a dependency on X86 which is not currently the case. > CONFIG_PECI is typically used on BMCs. Many of those do not use > Intel CPUs. It does not seem appropriate to make support for PECI > based hardware monitoring dependent on it running on an Intel CPU. It seems that some use (or need to know about) Icelake and Sapphire Rapids. How does this code get the value "peci_dev->info.model" used in this switch? Given that it is doing some magic just for some recent Intel CPUs, it seems plausible that when non-family 6 CPUs appear in the market, it might have to grab both the family and the model to reliably determine what to do? Simple options to avoid including <asm/cpu_device_id.h> would be to either: 1) provide a copy of the VFM_MODEL macro here. or 2) Keep using the old INTEL_FAM6_* #define names, but define those for the three CPU models peci needs locally instead of getting them from <asm/intel-family.h>. It looks like there is a somewhat convoluted path to include that. I see in <linux/peci.cpu.h> #include "../../arch/x86/include/asm/intel-family.h" Or maybe some better option? -Tony
On Tue, Apr 16, 2024 at 11:05:15PM +0000, Luck, Tony wrote: > Guenter, > > Thanks for taking a look at this patch. > > > > + case VFM_MODEL(INTEL_ICELAKE_X): > > > + case VFM_MODEL(INTEL_ICELAKE_D): > > > + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X): > > > > $ git describe > > v6.9-rc4-31-g96fca68c4fbf > > $ git grep VFM_MODEL > > $ > > > > So I guess this depends on some other patch ? > > That might be worth mentioning, especially since > > > > $ git describe > > next-20240416 > > $ git grep VFM_MODEL > > $ > > > > doesn't find it either. > > > Yes. This depends on parts 1,2,3 of this series. I should have added > that to the "---" meta comment part of these patches that I'm spraying > out to maintainers of random subsystems that use INTEL_FAM6 defines. > > > On top of that, it looks like > > > > #include <asm/cpu_device_id.h> > > > > introduces a dependency on X86 which is not currently the case. > > CONFIG_PECI is typically used on BMCs. Many of those do not use > > Intel CPUs. It does not seem appropriate to make support for PECI > > based hardware monitoring dependent on it running on an Intel CPU. > > It seems that some use (or need to know about) Icelake and Sapphire Rapids. > > How does this code get the value "peci_dev->info.model" used in this switch? > My understanding is that the PECI protocol provides that information. > Given that it is doing some magic just for some recent Intel CPUs, it seems > plausible that when non-family 6 CPUs appear in the market, it might have > to grab both the family and the model to reliably determine what to do? > > Simple options to avoid including <asm/cpu_device_id.h> would be > to either: > > 1) provide a copy of the VFM_MODEL macro here. > > or > > 2) Keep using the old INTEL_FAM6_* #define names, but define those for > the three CPU models peci needs locally instead of getting them from > <asm/intel-family.h>. It looks like there is a somewhat convoluted path to > include that. I see in <linux/peci.cpu.h> > > #include "../../arch/x86/include/asm/intel-family.h" > > > Or maybe some better option? > If the CPU defines and the new macro are to be kept in architecture code, maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h. That would not be worse than today's include of intel-family.h. Guenter
> If the CPU defines and the new macro are to be kept in architecture code, > maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h. > That would not be worse than today's include of intel-family.h. Guenter, Looks like I did that to resolve one of the other peci problems. Because I already have: #include "../../arch/x86/include/asm/cpu_device_id.h" #include "../../arch/x86/include/asm/intel-family.h" in <linux/peci_cpu.h> Simply deleting the include from cputemp.c builds OK in the context of all the other changes in my patch series. -Tony
On Tue, 2024-04-16 at 23:57 +0000, Luck, Tony wrote: > > If the CPU defines and the new macro are to be kept in architecture code, > > maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h. > > That would not be worse than today's include of intel-family.h. > > Guenter, > > Looks like I did that to resolve one of the other peci problems. Because I > already have: > > #include "../../arch/x86/include/asm/cpu_device_id.h" > #include "../../arch/x86/include/asm/intel-family.h" > > in <linux/peci_cpu.h> > > Simply deleting the include from cputemp.c builds OK in the > context of all the other changes in my patch series. Hi Tony, It won't build on non-x86, as cpu_device_id.h includes <asm/intel-family.h>. I think the simplest way to solve the issue is to provide a copy of VFM_* macros and X86_VENDOR_INTEL in include/linux/peci-cpu.h. -Iwona > > -Tony
On Thu, Apr 18, 2024 at 01:32:15PM +0000, Winiarska, Iwona wrote: > On Tue, 2024-04-16 at 23:57 +0000, Luck, Tony wrote: > > > If the CPU defines and the new macro are to be kept in architecture code, > > > maybe include arch/x86/include/asm/cpu_device_id.h from linux/peci.cpu.h. > > > That would not be worse than today's include of intel-family.h. > > > > Guenter, > > > > Looks like I did that to resolve one of the other peci problems. Because I > > already have: > > > > #include "../../arch/x86/include/asm/cpu_device_id.h" > > #include "../../arch/x86/include/asm/intel-family.h" > > > > in <linux/peci_cpu.h> > > > > Simply deleting the include from cputemp.c builds OK in the > > context of all the other changes in my patch series. > > Hi Tony, > > It won't build on non-x86, as cpu_device_id.h includes <asm/intel-family.h>. > I think the simplest way to solve the issue is to provide a copy of VFM_* macros > and X86_VENDOR_INTEL in include/linux/peci-cpu.h. > I think the proper fix would really be to move the include files to a generic directory, such as include/linux/x86/ or include/linux/cpu/x86/. After all, they _are_ now needed in non-Intel code. Guenter
On Thu, 2024-04-18 at 06:52 -0700, Guenter Roeck wrote: > On Thu, Apr 18, 2024 at 01:32:15PM +0000, Winiarska, Iwona wrote: > > On Tue, 2024-04-16 at 23:57 +0000, Luck, Tony wrote: > > > > If the CPU defines and the new macro are to be kept in architecture > > > > code, > > > > maybe include arch/x86/include/asm/cpu_device_id.h from > > > > linux/peci.cpu.h. > > > > That would not be worse than today's include of intel-family.h. > > > > > > Guenter, > > > > > > Looks like I did that to resolve one of the other peci problems. Because I > > > already have: > > > > > > #include "../../arch/x86/include/asm/cpu_device_id.h" > > > #include "../../arch/x86/include/asm/intel-family.h" > > > > > > in <linux/peci_cpu.h> > > > > > > Simply deleting the include from cputemp.c builds OK in the > > > context of all the other changes in my patch series. > > > > Hi Tony, > > > > It won't build on non-x86, as cpu_device_id.h includes <asm/intel-family.h>. > > I think the simplest way to solve the issue is to provide a copy of VFM_* > > macros > > and X86_VENDOR_INTEL in include/linux/peci-cpu.h. > > > > I think the proper fix would really be to move the include files to a > generic directory, such as include/linux/x86/ or include/linux/cpu/x86/. > After all, they _are_ now needed in non-Intel code. Yeah, that was the initial proposal: https://lore.kernel.org/lkml/20210803113134.2262882-2-iwona.winiarska@intel.com/ Unfortunately, it ended up being simplified to just include arch/x86 directly. -Iwona > > Guenter
> > > > > If the CPU defines and the new macro are to be kept in architecture > > > > > code, > > > > > maybe include arch/x86/include/asm/cpu_device_id.h from > > > > > linux/peci.cpu.h. > > > > > That would not be worse than today's include of intel-family.h. > > > > > > > > Guenter, > > > > > > > > Looks like I did that to resolve one of the other peci problems. Because I > > > > already have: > > > > > > > > #include "../../arch/x86/include/asm/cpu_device_id.h" > > > > #include "../../arch/x86/include/asm/intel-family.h" > > > > > > > > in <linux/peci_cpu.h> > > > > > > > > Simply deleting the include from cputemp.c builds OK in the > > > > context of all the other changes in my patch series. > > > > > > Hi Tony, > > > > > > It won't build on non-x86, as cpu_device_id.h includes <asm/intel-family.h>. > > > I think the simplest way to solve the issue is to provide a copy of VFM_* > > > macros > > > and X86_VENDOR_INTEL in include/linux/peci-cpu.h. > > > > > > > I think the proper fix would really be to move the include files to a > > generic directory, such as include/linux/x86/ or include/linux/cpu/x86/. > > After all, they _are_ now needed in non-Intel code. > > Yeah, that was the initial proposal: > https://lore.kernel.org/lkml/20210803113134.2262882-2-iwona.winiarska@intel.com/ > > Unfortunately, it ended up being simplified to just include arch/x86 directly. Reading through that other thread (Iwona: thanks for the link) it seems that moving the x86 include files out of arch/x86/include/asm has been soundly rejected. I'm going to take Iwona's advice above and copy the VFM_* macros. -Tony
>> Unfortunately, it ended up being simplified to just include arch/x86 directly. > > Reading through that other thread (Iwona: thanks for the link) it seems that moving > the x86 include files out of arch/x86/include/asm has been soundly rejected. > > I'm going to take Iwona's advice above and copy the VFM_* macros. Iwona, I just pushed what might become v4 of this series to: git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git new_families_v4 If you have a moment, could you please check whether the peci bits build on a non-x86 system. Thanks -Tony
On Mon, 2024-04-22 at 22:19 +0000, Luck, Tony wrote: > > > Unfortunately, it ended up being simplified to just include arch/x86 > > > directly. > > > > Reading through that other thread (Iwona: thanks for the link) it seems that > > moving > > the x86 include files out of arch/x86/include/asm has been soundly rejected. > > > > I'm going to take Iwona's advice above and copy the VFM_* macros. > > Iwona, > > I just pushed what might become v4 of this series to: > > git://git.kernel.org/pub/scm/linux/kernel/git/aegl/linux.git > new_families_v4 > > If you have a moment, could you please check whether the peci bits build on > a non-x86 system. It still doesn't compile because X86_VENDOR_INTEL is missing in include/linux/peci-cpu.h: We need something like: diff --git a/include/linux/peci-cpu.h b/include/linux/peci-cpu.h index 218fc9372..38cb61203 100644 --- a/include/linux/peci-cpu.h +++ b/include/linux/peci-cpu.h @@ -6,6 +6,9 @@ #include <linux/types.h> +/* Copied from x86 <asm/processor.h> */ +#define X86_VENDOR_INTEL 0 + /* Copied from x86 <asm/cpu_device_id.h> */ #define VFM_MODEL_BIT 0 #define VFM_FAMILY_BIT 8 Thanks -Iwona > > Thanks > > -Tony
>> If you have a moment, could you please check whether the peci bits build on >> a non-x86 system. > > It still doesn't compile because X86_VENDOR_INTEL is missing in > include/linux/peci-cpu.h: Iwona: Thanks for testing. > We need something like: > +/* Copied from x86 <asm/processor.h> */ > +#define X86_VENDOR_INTEL 0 > + Agreed. I've added that. -Tony
diff --git a/drivers/hwmon/peci/cputemp.c b/drivers/hwmon/peci/cputemp.c index a812c15948d9..28cec5c318d4 100644 --- a/drivers/hwmon/peci/cputemp.c +++ b/drivers/hwmon/peci/cputemp.c @@ -11,6 +11,7 @@ #include <linux/peci-cpu.h> #include <linux/units.h> +#include <asm/cpu_device_id.h> #include "common.h" #define CORE_NUMS_MAX 64 @@ -361,9 +362,9 @@ static int init_core_mask(struct peci_cputemp *priv) /* Get the RESOLVED_CORES register value */ switch (peci_dev->info.model) { - case INTEL_FAM6_ICELAKE_X: - case INTEL_FAM6_ICELAKE_D: - case INTEL_FAM6_SAPPHIRERAPIDS_X: + case VFM_MODEL(INTEL_ICELAKE_X): + case VFM_MODEL(INTEL_ICELAKE_D): + case VFM_MODEL(INTEL_SAPPHIRERAPIDS_X): ret = peci_ep_pci_local_read(peci_dev, 0, reg->bus, reg->dev, reg->func, reg->offset + 4, &data); if (ret)
New CPU #defines encode vendor and family as well as model. Signed-off-by: Tony Luck <tony.luck@intel.com> --- drivers/hwmon/peci/cputemp.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)