linux-kernel - Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7

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: <5af77de6-3a18-a3b9-b492-c280ac4310a1@linux.intel.com>
Date:   Mon, 8 Jul 2019 15:07:15 +0800
From:   Jing Liu <jing2.liu@...ux.intel.com>
To:     Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Subject: Re: [PATCH 2/5] KVM: cpuid: extract do_cpuid_7_mask and support
 multiple subleafs

Hi Paolo,

Thank you for refining the cpuid codes especially for case 7! It looks
much clear now!

On 7/4/2019 10:07 PM, Paolo Bonzini wrote:
> CPUID function 7 has multiple subleafs.  Instead of having nested
> switch statements, move the logic to filter supported features to
> a separate function, and call it for each subleaf.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
> 	Here you would have something like entry->eax = min(entry->eax, 1)
> 	when adding subleaf 1.
> 
>   arch/x86/kvm/cpuid.c | 128 +++++++++++++++++++++++++++----------------
>   1 file changed, 81 insertions(+), 47 deletions(-)
> 
[...]
> +
> +	switch (index) {
> +	case 0:
> +		entry->eax = 0;

Here, mark: when adding subleaf 1, change to
entry->eax = min(entry->eax, 1).

> +		entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
> +		cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
> +		/* TSC_ADJUST is emulated */
> +		entry->ebx |= F(TSC_ADJUST);
> +
> +		entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> +		f_la57 = entry->ecx & F(LA57);
> +		cpuid_mask(&entry->ecx, CPUID_7_ECX);
> +		/* Set LA57 based on hardware capability. */
> +		entry->ecx |= f_la57;
> +		entry->ecx |= f_umip;
> +		/* PKU is not yet implemented for shadow paging. */
> +		if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
> +			entry->ecx &= ~F(PKU);
> +
> +		entry->edx &= kvm_cpuid_7_0_edx_x86_features;
> +		cpuid_mask(&entry->edx, CPUID_7_EDX);
> +		/*
> +		 * We emulate ARCH_CAPABILITIES in software even
> +		 * if the host doesn't support it.
> +		 */
> +		entry->edx |= F(ARCH_CAPABILITIES);
> +		break;
And when adding subleaf 1, plan to add codes,

case 1:
	entry->eax |= kvm_cpuid_7_1_eax_x86_features;
	entry->ebx = entry->ecx = entry->edx =0;
	break;

What do you think?

> +	default:
> +		WARN_ON_ONCE(1);
> +		entry->eax = 0;
> +		entry->ebx = 0;
> +		entry->ecx = 0;
> +		entry->edx = 0;
> +		break;
> +	}
> +}
> +
>   static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>   				  int *nent, int maxnent)
[...]
> +	/* function 7 has additional index. */
>   	case 7: {
> +		int i;
> +
>   		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> -		entry->eax = 0;
> -		/* Mask ebx against host capability word 9 */
> -		entry->ebx &= kvm_cpuid_7_0_ebx_x86_features;
> -		cpuid_mask(&entry->ebx, CPUID_7_0_EBX);
> -		// TSC_ADJUST is emulated
> -		entry->ebx |= F(TSC_ADJUST);
> -		entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> -		f_la57 = entry->ecx & F(LA57);
> -		cpuid_mask(&entry->ecx, CPUID_7_ECX);
> -		/* Set LA57 based on hardware capability. */
> -		entry->ecx |= f_la57;
> -		entry->ecx |= f_umip;
> -		/* PKU is not yet implemented for shadow paging. */
> -		if (!tdp_enabled || !boot_cpu_has(X86_FEATURE_OSPKE))
> -			entry->ecx &= ~F(PKU);
> -		entry->edx &= kvm_cpuid_7_0_edx_x86_features;
> -		cpuid_mask(&entry->edx, CPUID_7_EDX);
> -		/*
> -		 * We emulate ARCH_CAPABILITIES in software even
> -		 * if the host doesn't support it.
> -		 */
> -		entry->edx |= F(ARCH_CAPABILITIES);
> +		for (i = 0; ; ) {
> +			do_cpuid_7_mask(&entry[i], i);
> +			if (i == entry->eax)
> +				break;
> +			if (*nent >= maxnent)
> +				goto out;
> +
> +			++i;
> +			do_cpuid_1_ent(&entry[i], function, i);
> +			entry[i].flags |=
> +			       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +			++*nent;
> +		}

The new logic is great and adding subleaf support would be much easier!

>   		break;
>   	}
>   	case 9:
> 

Thanks,
Jing