Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 12 Aug 2023 00:25:00 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>, x86@kernel.org,
        Ashok Raj <ashok.raj@intel.com>,
        Arjan van de Ven <arjan@linux.intel.com>
Subject: Re: [patch 08/30] x86/microcode/intel: Rip out mixed stepping
 support for Intel CPUs
Message-ID: <20230811222500.GOZNa1PIUVJeBDiBGI@fat_crate.local>
References: <20230810153317.850017756@linutronix.de>
 <20230810160805.418991257@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230810160805.418991257@linutronix.de>
Precedence: bulk

On Thu, Aug 10, 2023 at 08:37:38PM +0200, Thomas Gleixner wrote:
> @@ -319,6 +264,7 @@ scan_microcode(void *data, size_t size,
>  {
>  	struct microcode_header_intel *mc_header;
>  	struct microcode_intel *patch = NULL;
> +	u32 cur_rev = uci->cpu_sig.rev;
>  	unsigned int mc_size;
>  
>  	while (size) {
> @@ -328,8 +274,7 @@ scan_microcode(void *data, size_t size,
>  		mc_header = (struct microcode_header_intel *)data;
>  
>  		mc_size = get_totalsize(mc_header);
> -		if (!mc_size ||
> -		    mc_size > size ||
> +		if (!mc_size || mc_size > size ||
>  		    intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
>  			break;
>  
> @@ -341,31 +286,16 @@ scan_microcode(void *data, size_t size,
>  			continue;
>  		}
>  
> -		if (save) {
> -			save_microcode_patch(uci, data, mc_size);
> +		/* BSP scan: Check whether there is newer microcode */
> +		if (save && cur_rev >= mc_header->rev)
>  			goto next;
> -		}
> -
>  
> -		if (!patch) {
> -			if (!has_newer_microcode(data,
> -						 uci->cpu_sig.sig,
> -						 uci->cpu_sig.pf,
> -						 uci->cpu_sig.rev))
> -				goto next;
> -
> -		} else {
> -			struct microcode_header_intel *phdr = &patch->hdr;
> -
> -			if (!has_newer_microcode(data,
> -						 phdr->sig,
> -						 phdr->pf,
> -						 phdr->rev))
> -				goto next;
> -		}
> +		/* Save scan: Check whether there is newer or matching microcode */
> +		if (save && cur_rev != mc_header->rev)
> +			goto next;

I'm confused: when you look at those statements when this patch is
applied, they look like this:

                /* BSP scan: Check whether there is newer microcode */
                if (save && cur_rev >= mc_header->rev)
                        goto next;

                /* Save scan: Check whether there is newer or matching microcode */
                if (save && cur_rev != mc_header->rev)
                        goto next;

You'd only hit the second one if

		cur_rev < mc_header->rev

but then that implies

		cur_rev != mc_header->rev

too. I *think* you wanna have the first test be only ">" as you're
looking for newer microcode.

Besides, __load_ucode_intel() is calling this function with safe ==
false so those statements would never check anything. I guess that's
still ok because the above intel_find_matching_signature() would match.

Hmmm?

Uff, this function is ugly and can be simplified. Perhaps that happens
later.


>  
> -		/* We have a newer patch, save it. */
>  		patch = data;
> +		cur_rev = mc_header->rev;
>  
>  next:
>  		data += mc_size;
> @@ -374,18 +304,22 @@ scan_microcode(void *data, size_t size,
>  	if (size)
>  		return NULL;
>  
> +	if (save && patch)
> +		save_microcode_patch(patch, mc_size);
> +
>  	return patch;
>  }
>  
>  static void show_saved_mc(void)
>  {
>  #ifdef DEBUG

Yeah, what Nikolay said - move the next one before this one and then the
show_saved_mc() hunks are gone.

> -	int i = 0, j;
>  	unsigned int sig, pf, rev, total_size, data_size, date;
> +	struct extended_sigtable *ext_header;
> +	struct extended_signature *ext_sig;
>  	struct ucode_cpu_info uci;
> -	struct ucode_patch *p;
> +	int j, ext_sigcount;
>  
> -	if (list_empty(&microcode_cache)) {
> +	if (!intel_ucode_patch) {
>  		pr_debug("no microcode data saved.\n");
>  		return;
>  	}

...

> @@ -451,7 +374,7 @@ static void save_mc_for_early(struct uco
>  
>  	mutex_lock(&x86_cpu_microcode_mutex);
>  
> -	save_microcode_patch(uci, mc, size);
> +	save_microcode_patch(mc, size);
>  	show_saved_mc();
>  
>  	mutex_unlock(&x86_cpu_microcode_mutex);
> @@ -675,26 +598,10 @@ void load_ucode_intel_ap(void)
>  	apply_microcode_early(&uci, true);
>  }
>  
> -static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
> +/* Accessor for microcode pointer */
> +static struct microcode_intel *ucode_get_patch(void)

static function - "get_patch" only is fine.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette