Re: [PATCH] Feature: Expose x86 CPU FMS from cpuid module

[Herrenschmidt, Benjamin]

On Wed, 2023-04-12 at 09:40 +0000, Paterson, Harley via Grub-devel wrote:
> This patch exposes an x86 CPU's model, family, and stepping via flag in
> the `cpuid` command.
> 
> Invoking `cpuid -f` will set the Grub environment variables
> `CPUID_FAMILY`, `CPUID_MODEL`, and `CPUID_STEP` for consumption by Grub
> scripts.
> 
> Accessing the CPU FMS data requires a `cpuid(0x1)` call on both x86 and
> x64 systems. Calling `cpuid(0x1)` first requires checking `cpuid` can be
> called on the processor, and then checking the maximum parameter
> accepted by the CPU's `cpuid` implementation is greater than or equal to
> one.
> 
> The x86 implementation of this code already has these checks (and calls
> `cpuid(0x1)`) to discover the presence of long mode and PAE. These
> features are currently assumed to be present on x64.
> 
> This patch runs the tests on both x86 and x86_64.  This existing shortcut
> saves trivial time, but we would need the same code paths for
> the FMS anyway - better always run the PAE tests too,
> rather than duplicate the code minus these fast tests (bitshifts and
> xors).
> 
> The CPU FMS format is common across both AMD and Intel devices. The long
> mode flags bit is undefined in x64. The long-mode check is disabled
> on x64 and assumed true, consistent with existing behaviour. See:
> 
> * https://www.amd.com/system/files/TechDocs/25481.pdf
> * 
> https://www.scss.tcd.ie/~jones/CS4021/processor-identification-cpuid-instruction-note.pdf
> ---
>  grub-core/commands/i386/cpuid.c | 73 ++++++++++++++++++++++++++++-----
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/grub-core/commands/i386/cpuid.c b/grub-core/commands/i386/cpuid.c
> index 42b9841..90d857b 100644
> --- a/grub-core/commands/i386/cpuid.c
> +++ b/grub-core/commands/i386/cpuid.c
> @@ -35,13 +35,15 @@ static const struct grub_arg_option options[] =
>         no argument is specified.  */
>      {"long-mode", 'l', 0, N_("Check if CPU supports 64-bit (long) mode 
> (default)."), 0, 0},
>      {"pae", 'p', 0, N_("Check if CPU supports Physical Address Extension."), 
> 0, 0},
> +    {"fms", 'f', 0, "Set CPUID_FAMILY, CPUID_MODEL, and CPUID_STEP 
> variables.", 0, 0},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
>  enum
>    {
>      MODE_LM = 0,
> -    MODE_PAE = 1
> +    MODE_PAE = 1,
> +    MODE_FMS = 2
>    };
>  
>  enum
> @@ -52,8 +54,45 @@ enum
>    {
>      bit_LM = (1 << 29)
>    };
> +enum
> +  {
> +    /* 8 bit number (3 base 10 digits) plus terminator. */
> +    FMS_BUF_LEN = 4,
> +  };
> +
> +unsigned char grub_cpuid_has_longmode = 0;
> +unsigned char grub_cpuid_has_pae = 0;
> +int grub_cpuid_family = -1;
> +int grub_cpuid_model = -1;
> +int grub_cpuid_step = -1;
>  
> -unsigned char grub_cpuid_has_longmode = 0, grub_cpuid_has_pae = 0;
> +
> +static void
> +set_cpuid_env(void)
> +{
> +  char fms[FMS_BUF_LEN] = {0};
> +  if (grub_cpuid_family != -1)
> +  {
> +    grub_snprintf(fms, FMS_BUF_LEN - 1, "%d", grub_cpuid_family);
> +    grub_env_set("CPUID_FAMILY", fms);
> +  }

So this never made it upstream, thus it should probably be resent. That
said we found an issue with it... the above should FMS_BUF_LEN not
FMS_BUF_LEN - 1 (and the pre-initialization is unnecessary) as grub
implementation of snprintf will already seems to implement the C99
behaviour which guarantees NUL termination of the result.

The existing code will fail to properly handle 3-digit strings.

And same for subsequent invocations..

> +  else
> +    grub_env_set("CPUID_FAMILY", "unknown");
> +  if (grub_cpuid_model != -1)
> +  {
> +    grub_snprintf(fms, FMS_BUF_LEN - 1, "%d", grub_cpuid_model);
> +    grub_env_set("CPUID_MODEL", fms);
> +  }
> +  else
> +    grub_env_set("CPUID_MODEL", "unknown");
> +  if (grub_cpuid_step != -1)
> +  {
> +    grub_snprintf(fms, FMS_BUF_LEN - 1, "%d", grub_cpuid_step);
> +    grub_env_set("CPUID_STEP", fms);
> +  }
> +  else
> +    grub_env_set("CPUID_STEP", "unknown");
> +}
>  
>  static grub_err_t
>  grub_cmd_cpuid (grub_extcmd_context_t ctxt,
> @@ -61,7 +100,12 @@ grub_cmd_cpuid (grub_extcmd_context_t ctxt,
>                 char **args __attribute__ ((unused)))
>  {
>    int val = 0;
> -  if (ctxt->state[MODE_PAE].set)
> +  if (ctxt->state[MODE_FMS].set)
> +  {
> +    set_cpuid_env();
> +    return GRUB_ERR_NONE;
> +  }
> +  else if (ctxt->state[MODE_PAE].set)
>      val = grub_cpuid_has_pae;
>    else
>      val = grub_cpuid_has_longmode;
> @@ -75,20 +119,24 @@ static grub_extcmd_t cmd;
>  
>  GRUB_MOD_INIT(cpuid)
>  {
> -#ifdef __x86_64__
> -  /* grub-emu */
> -  grub_cpuid_has_longmode = 1;
> -  grub_cpuid_has_pae = 1;
> -#else
> -  unsigned int eax, ebx, ecx, edx;
>    unsigned int max_level;
>    unsigned int ext_level;
>  
>    /* See if we can use cpuid.  */
> +#ifdef __x86_64__
> +  unsigned long eax, ebx, ecx, edx;
> +  grub_cpuid_has_longmode = 1;
> +  asm volatile ("pushf; pushf; pop %0; mov %0,%1; xor %2,%0;"
> +               "push %0; popf; pushf; pop %0; popf"
> +               : "=&r" (eax), "=&r" (ebx)
> +               : "i" (0x00200000));
> +#else
> +  unsigned int eax, ebx, ecx, edx;
>    asm volatile ("pushfl; pushfl; popl %0; movl %0,%1; xorl %2,%0;"
>                 "pushl %0; popfl; pushfl; popl %0; popfl"
>                 : "=&r" (eax), "=&r" (ebx)
>                 : "i" (0x00200000));
> +#endif
>    if (((eax ^ ebx) & 0x00200000) == 0)
>      goto done;
>  
> @@ -103,8 +151,12 @@ GRUB_MOD_INIT(cpuid)
>      {
>        grub_cpuid (1, eax, ebx, ecx, edx);
>        grub_cpuid_has_pae = !!(edx & bit_PAE);
> +      grub_cpuid_step = eax & 0xf;
> +      grub_cpuid_model = ((eax >> 4) & 0xf) + (((eax >> 16) & 0xf) << 4);
> +      grub_cpuid_family = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
>      }
>  
> +#ifndef __x86_64__
>    grub_cpuid (0x80000000, eax, ebx, ecx, edx);
>    ext_level = eax;
>    if (ext_level < 0x80000000)
> @@ -112,9 +164,8 @@ GRUB_MOD_INIT(cpuid)
>  
>    grub_cpuid (0x80000001, eax, ebx, ecx, edx);
>    grub_cpuid_has_longmode = !!(edx & bit_LM);
> -done:
>  #endif
> -
> +done:
>    cmd = grub_register_extcmd ("cpuid", grub_cmd_cpuid, 0,
>                               "[-l]", N_("Check for CPU features."), options);
>  }