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] [day] [month] [year] [list]

Message-ID: <d30ab1d0-e09d-41ba-b98d-9b7d039f6d80@intel.com>
Date: Wed, 16 Apr 2025 10:37:15 -0700
From: Dave Hansen <dave.hansen@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>, 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/25 10:22, Chang S. Bae wrote:
> 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.

Can it? There are only two ways out of the loop: the explicit break and
the implicit break from the 'timeout' check. Both happen at the "end" of
the loop, unless the timeout< check failed up front:

	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++){
		msleep(1);
		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
		if (status & MASK_MBOX_STATUS_READY)
			break;
	}

> 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?

It's simpler to read the code the way you have it. You don't have to
consider if 'status' could be uninitialized. It makes the "timeout loop"
*COMPLETELY* independent from the rest of the function. It could be:

foo()
{
	spin_until_ready();

	status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);

	...
}

for example.

It's less likely to break, too. Let's say someone added a new break:

	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++){
+		if (some_other_condition());
+			break;
		msleep(1);
		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
		if (status & MASK_MBOX_STATUS_READY)
			break;
	}

you'd have a bug on your hands.

I'm fine with the way you have it. I probably would have written it that
way too.