linux-kernel - Re: [PATCH v3 5/6] x86/microcode/intel: Support mailbox transfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <166b29f2-a178-4d60-8f5d-218bfd7b51ac@intel.com>
Date: Wed, 16 Apr 2025 10:22:34 -0700
From: "Chang S. Bae" <chang.seok.bae@...el.com>
To: Chao Gao <chao.gao@...el.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<colinmitchell@...gle.com>
Subject: Re: [PATCH v3 5/6] x86/microcode/intel: Support mailbox transfer

On 4/16/2025 7:14 AM, Chao Gao wrote:
>> +/*
>> + * Wait for the hardware to complete a transaction.
>> + * Return true on success, false on failure.
>> + */
>> +static bool wait_for_transaction(struct staging_state *ss)
>> +{
>> +	u32 timeout, status;
>> +
>> +	/* Allow time for hardware to complete the operation: */
>> +	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++) {
>> +		msleep(1);
>> +
>> +		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
>> +		/* Break out early if the hardware is ready: */
>> +		if (status & MASK_MBOX_STATUS_READY)
>> +			break;
> 
> Shouldn't we exit the loop if the MASK_MBOX_STATUS_ERROR is set, or is the
> ERROR bit always set in conjunction with the READY bit?
> 
>> +	}
>> +
>> +	status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
> 
> I still think this read is not needed.

No, if it times out, it could exit without having read the status.

But this is a slow path — instead of trying to save a few cycles, I 
wanted to present the logic more clearly:

   * Give the hardware enough time, but not forever.
     * Oh, we don't need to wait anymore if it's done

   * After waiting, check the status and handle it properly.

Isn’t it clearer to read the status after the wait, right before the 
error handling?

Also, reading the status inside the loop is just a way to determine 
whether to break — not to interpret or act on it. That should come after 
the wait completes.