linux-kernel - Re: [PATCH 0/2] x86/kvm: Force legacy PCI hole as WB under SNP/TDX

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: <218c4e93-4da6-4286-bb17-dd68c9d6db66@suse.com>
Date: Thu, 21 Aug 2025 08:02:42 +0200
From: Jürgen Groß <jgross@...e.com>
To: Binbin Wu <binbin.wu@...ux.intel.com>,
 Sean Christopherson <seanjc@...gle.com>,
 Vishal Annapurve <vannapurve@...gle.com>
Cc: Nikolay Borisov <nik.borisov@...e.com>, Jianxiong Gao <jxgao@...gle.com>,
 "Borislav Petkov (AMD)" <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>,
 Dionna Glaze <dionnaglaze@...gle.com>, "H. Peter Anvin" <hpa@...or.com>,
 "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
 pbonzini@...hat.com, Peter Gonda <pgonda@...gle.com>,
 Thomas Gleixner <tglx@...utronix.de>, Tom Lendacky
 <thomas.lendacky@....com>, Vitaly Kuznetsov <vkuznets@...hat.com>,
 x86@...nel.org, Rick Edgecombe <rick.p.edgecombe@...el.com>,
 jiewen.yao@...el.com
Subject: Re: [PATCH 0/2] x86/kvm: Force legacy PCI hole as WB under SNP/TDX

On 21.08.25 07:23, Binbin Wu wrote:
> 
> 
> On 8/21/2025 11:30 AM, Binbin Wu wrote:
>>
>>
>> On 8/21/2025 1:56 AM, Sean Christopherson wrote:
>>> On Wed, Aug 20, 2025, Binbin Wu wrote:
> [...]
>>>> Hi Sean,
>>>>
>>>> Since guest_force_mtrr_state() also supports to force MTRR variable ranges,
>>>> I am wondering if we could use guest_force_mtrr_state() to set the legacy PCI
>>>> hole range as UC?
>>>>
>>>> Is it less hacky?
>>> Oh!  That's a way better idea than my hack.  I missed that the kernel would 
>>> still
>>> consult MTRRs.
>>>
>>> Compile tested only, but something like this?
>>>
>>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>>> index 8ae750cde0c6..45c8871cdda1 100644
>>> --- a/arch/x86/kernel/kvm.c
>>> +++ b/arch/x86/kernel/kvm.c
>>> @@ -933,6 +933,13 @@ static void kvm_sev_hc_page_enc_status(unsigned long 
>>> pfn, int npages, bool enc)
>>>     static void __init kvm_init_platform(void)
>>>   {
>>> +       u64 tolud = e820__end_of_low_ram_pfn() << PAGE_SHIFT;
>>> +       struct mtrr_var_range pci_hole = {
>>> +               .base_lo = tolud | X86_MEMTYPE_UC,
>>> +               .mask_lo = (u32)(~(SZ_4G - tolud - 1)) | BIT(11),
>>> +               .mask_hi = (BIT_ULL(boot_cpu_data.x86_phys_bits) - 1) >> 32,
>>> +       };
>>> +
>>
>> This value of tolud  may not meet the range size and alignment requirement for
>> variable MTRR.
>>
>> Variable MTRR has requirement for range size and alignment:
>> For ranges greater than 4 KBytes, each range must be of length 2^n and its base
>> address must be aligned on a 2^n boundary, where n is a value equal to or
>> greater than 12. The base-address alignment value cannot be less than its length.
> 
> Wait, Linux kernel converts MTRR register values to MTRR state (base and size) and
> cache it for later lookups (refer to map_add_var()). I.e., in Linux kernel,
> only the cached state will be used.
> 
> These MTRR register values are never programmed when using
> guest_force_mtrr_state() , so even the values doesn't meet the
> requirement from hardware perspective, Linux kernel can still get the right base 
> and size.
> 
> No bothering to force the base and size alignment.
> But a comment would be helpful.
> Also, BIT(11) could be replaced by MTRR_PHYSMASK_V.
> 
> How about:
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 90097df4eafd..a9582ffc3088 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -934,9 +934,15 @@ static void kvm_sev_hc_page_enc_status(unsigned long pfn, 
> int npages, bool enc)
>   static void __init kvm_init_platform(void)
>   {
>          u64 tolud = e820__end_of_low_ram_pfn() << PAGE_SHIFT;

I'd prefer to avoid open coding PFN_PHYS() here.

> +       /*
> +        * The range's base address and size may not meet the alignment
> +        * requirement for variable MTRR. However, Linux guest never
> +        * programs MTRRs when forcing guest MTRR state, no bothering to
> +        * enforce the base and range size alignment.
> +        */

Maybe a related comment should be added to guest_force_mtrr_state() in
order to avoid breaking this workaround again.


Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)