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;
Message-ID: <1b184587-128d-e5cc-67e9-1d27feb87213@kernel.org>
Date:   Wed, 4 Jan 2023 08:43:51 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] x86: also disable FSRM if ERMS is disabled
Content-Language: en-US
To:     Daniel Verkamp <dverkamp@chromium.org>,
        Borislav Petkov <bp@alien8.de>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Tony Luck <tony.luck@intel.com>
References: <20220923005827.1533380-1-dverkamp@chromium.org>
 <Yy2U2BW6Tx0imGpK@zn.tnic>
 <CABVzXAk9AXj2Ns7YAh7cCA38t2sGxOEYLv-EfLCoFHr-SUQ2Mw@mail.gmail.com>
 <Yy3yJfz213Lqo4KC@zn.tnic>
 <CABVzXAkO4pU+gpUcWOEWDw+W4id=1WEOgeP5+3tBG_LR6=oa=g@mail.gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <CABVzXAkO4pU+gpUcWOEWDw+W4id=1WEOgeP5+3tBG_LR6=oa=g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 07. 10. 22, 20:08, Daniel Verkamp wrote:
> On Fri, Sep 23, 2022 at 10:51 AM Borislav Petkov <bp@alien8.de> wrote:
>>
>> On Fri, Sep 23, 2022 at 10:25:05AM -0700, Daniel Verkamp wrote:
>>> Yes, we hit this in crosvm when booting the guest kernel with either
>>> OVMF or u-boot on an Intel 12th Gen CPU. The guest kernel boots fine
>>> when loaded directly (using the crosvm kernel loader and not running
>>> any firmware setup in the guest), but it crashes when booting with
>>> firmware inside the first forward memmove() after alternatives are set
>>> up (which happens to be in printk). I haven't gotten to the bottom of
>>> why exactly using firmware is causing this to be set up in an
>>> inconsistent way, but this is a real-world situation, not just a
>>> hypothetical.
>>
>> Sounds like broken virt firmware or so. And if that is not an issue on
>> baremetal, then the virt stack should be fixed - not the kernel.
>>
>>> Now that I look at it with fresh eyes again, maybe we should instead
>>> directly patch the memmove FSRM alternative so that the flag-set
>>> version just does the same jmp as the ERMS one. I can prepare a patch
>>> for that instead of (or in addition to) this one if that sounds
>>> better.
>>
>> So, if the virt firmware deviates from how the real hardware behaves,
>> then the kernel needs no fixing.
>>
>> So you'd have to figure out why is the virt firmware causing this and
>> not baremetal.
>>
>> Then we can talk about fixes.
> 
> Hi Borislav,
> 
> We found that the IA32_MISC_ENABLE MSR setup was missing in the crosvm
> firmware boot path (but not when directly booting a kernel, which is
> why it did not get noticed for a while). Setting the fast string bit
> in the MSR avoids the issue.
> 
> However, I still think it would be appropriate to apply this patch or
> something like it, since there could be a CPU, microcode update, BIOS,
> etc. that clears this bit while still having the CPUID flags for FSRM
> and ERMS.

Let me resurrect this thread... Our customer has an AMD CPU which has 
indeed both capabilities under normal circumstances. But they have a 
cool UEFI BIOS too. They say:

"""
In AMD platform, while disalbe ERMS(Enhanced Rep MOVSB/STOSB) in UEFI 
(system setup -> processor -> Enhanced Rep MOVSB/STOSB), the OS can't 
boot normally.
"""

That is exactly the case here. So can we have the patch (the original 
one, the one below or a better one) to fix this?

> The Intel SDM says: "Software can disable fast-string
> operation by clearing the fast-string-enable bit (bit 0) of
> IA32_MISC_ENABLE MSR", so it's not an invalid configuration for this
> bit to be unset.
> 
> Additionally, something like this avoids the problem by making the
> FSRM case jump directly to the REP MOVSB rather than falling through
> to the ERMS jump in the next instruction, which seems like basically
> free insurance (but if the FSRM flag gets used somewhere else in the
> future, having it set consistently with ERMS is probably still a good
> idea, per the original patch):
> 
> diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S
> index 724bbf83eb5b..8ac557409c7d 100644
> --- a/arch/x86/lib/memmove_64.S
> +++ b/arch/x86/lib/memmove_64.S
> @@ -38,7 +38,7 @@ SYM_FUNC_START(__memmove)
> 
>           /* FSRM implies ERMS => no length checks, do the copy directly */
>   .Lmemmove_begin_forward:
> -        ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "", X86_FEATURE_FSRM
> +        ALTERNATIVE "cmp $0x20, %rdx; jb 1f", "jmp .Lmemmove_erms",
> X86_FEATURE_FSRM
>           ALTERNATIVE "", "jmp .Lmemmove_erms", X86_FEATURE_ERMS
> 
> And hey, this means one less instruction to execute in the FSRM path. :)

thanks,
-- 
js
suse labs