Re: proposed cpuctl modification

[Masanobu SAITOH <msaitoh%execsw.org@localhost>]

Hello.

I'm sorry for the late of this reply. I was busy for a few weeks.

On 2023/03/03 6:59, Robert Elz wrote:
> This message is about a proposed userland modification, but it seems
> more kernelish to me, hence I am asking here on tech-kern, rather than
> on tech-userlevel
> 
> When my system boots (intel cpu) it runs the intel-microcode (from pkgsrc)
> microcode update.
> 
> Since it is an Intel cpu, that means running the ioctl to perform the
> microcode update on every core.   The cpu has hyperthreading, and while
> I sometimes am inclined to turn that off, I haven't so far.
> 
> Naturally, as they're really the same core, while the base core ucode
> update works fine, the hyperthread companion always fails, as there the
> microcode is already up to the expected version (surprise surprise, since
> we just did that).    I guess we could look and skip the update on
> the hyperthread companion cores (pseudo-cores) but that's not what I am
> proposing, partly because I'd have to work out how to do that, but also
> because that by itself would only solve half the problem.
> 
> My processor also has 8 cores, with no hyperthreading, where it looks as
> if internally, there's just 2 sets of ucode, one for the first 4, and one
> for the second 4.

Alder Lake-N? 4 E-cores share one microcode image. I have i7-12700 and it
has 4 E-cores. Those 4 cores share one microcode image.

>   Updates of the other 3 of each group find the ucode
> version already installed, and error out.
> 
> So, what I am proposing is to have cpuctl simply ignore EEXIST errors from
> the update the microcode" ioctl. unless the -v option (which already exists)
> is given.
> 
> Note that this isn't fixing any real problem - everything works as it is now,
> and everything (from what I can tell) gets updated correctly.   The issue is
> more cosmetic - the ucode update currently issues error messages for more than
> half of my (apparent) cores, which looks ugly during the boot process, and
> could lead to people wondering if there is a problem.

I think your idea is the best. Thank you for your commit.

Another solutions is that the kernel returns 0 instead of EEXIST if the
version number is the same as the running microcode's version. I think it's not
good because it hides the information that the running micocode is the latest.

> When I first installed the package from pgksrc to make this happen (a while
> ago now) I immediately disabled it (in rc.conf) so it ran no more, as the
> microcode version offered by the package was what my CPU came equipped with,
> and every core complained about the version not actually being updated.
> 
> pkgsrc was updated to a newer version, with newer microcode, I installed
> that, and enabled it again - and now I'm stuck with the ugly errors.
> 
> Perhaps someone has a better solution than this, perhaps checking which
> microcode version is installed on each core, and not attemmpting to install
> the update if it is the same version, but that's beyond my knowledge.
> It also seems likely to me that it is simpler to just install anyway, and
> ignore the "already there" error if it happens.   Probably.

I agree with you. Since we do not know what kind of sharing scheme future
CPUs will have, I think simple is the best.

 Thanks.


> My proposed patch is appended - it builds, installs, and seems to work
> just fine (not too much of a surprise, it is a trivial change).
> 
> Opinions?
> 
> kre
> 
> Index: cpuctl.8
> ===================================================================
> RCS file: /cvsroot/src/usr.sbin/cpuctl/cpuctl.8,v
> retrieving revision 1.20
> diff -u -r1.20 cpuctl.8
> --- cpuctl.8	17 May 2019 23:51:35 -0000	1.20
> +++ cpuctl.8	2 Mar 2023 21:36:06 -0000
> @@ -72,6 +72,10 @@
>  .Op Ar file
>  .Xc
>  This applies the microcode patch to CPUs.
> +Unless
> +.Fl v
> +was given, errors indicating that the microcode
> +already exists on the CPU in question are ignored.
>  If
>  .Ar cpu
>  is not specified or \-1, all CPUs are updated.
> Index: cpuctl.c
> ===================================================================
> RCS file: /cvsroot/src/usr.sbin/cpuctl/cpuctl.c,v
> retrieving revision 1.32
> diff -u -r1.32 cpuctl.c
> --- cpuctl.c	1 Feb 2022 10:45:02 -0000	1.32
> +++ cpuctl.c	2 Mar 2023 21:36:06 -0000
> @@ -247,7 +247,7 @@
>  		cpuset_destroy(cpuset);
>  	}
>  	error = ioctl(fd, IOC_CPU_UCODE_APPLY, &uc);
> -	if (error < 0) {
> +	if (error < 0 && (verbose || errno != EEXIST)) {
>  		if (uc.fwname[0])
>  			err(EXIT_FAILURE, "%s", uc.fwname);
>  		else
> 
> 

-- 
-----------------------------------------------
                SAITOH Masanobu (msaitoh%execsw.org@localhost
                                 msaitoh%netbsd.org@localhost)