Re: [patch 09/41] x86/kvm: Avoid looking up PKRU in XSAVE buffer

[Borislav Petkov]

On Fri, Jun 11, 2021 at 06:15:32PM +0200, Thomas Gleixner wrote:
> -		if (src) {
> -			u32 size, offset, ecx, edx;
> -			cpuid_count(XSTATE_CPUID, xfeature_nr,
> -				    &size, &offset, &ecx, &edx);
> -			if (xfeature_nr == XFEATURE_PKRU)
> -				memcpy(dest + offset, &vcpu->arch.pkru,
> -				       sizeof(vcpu->arch.pkru));
> -			else
> -				memcpy(dest + offset, src, size);
> +		cpuid_count(XSTATE_CPUID, xfeature_nr,
> +			    &size, &offset, &ecx, &edx);
>  
> +		if (xfeature_nr == XFEATURE_PKRU) {
> +			memcpy(dest + offset, &vcpu->arch.pkru,
> +			       sizeof(vcpu->arch.pkru));
> +		} else {
> +			src = get_xsave_addr(xsave, xfeature_nr);
> +			if (src)
> +				memcpy(dest + offset, src, size);
>  		}
>  
>  		valid -= xfeature_mask;

How about pulling up that PKRU case even further (pasting the whole
changed loop instead of an unreadable diff) and keeping the CPUID access
and the xsave address handling separate?

        valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
        while (valid) {
                u32 size, offset, ecx, edx;
                u64 xfeature_mask = valid & -valid;
                int xfeature_nr = fls64(xfeature_mask) - 1;
                void *src;

                if (xfeature_nr == XFEATURE_PKRU) {
                        memcpy(dest + offset, &vcpu->arch.pkru, sizeof(vcpu->arch.pkru));
                        continue;
                }

                cpuid_count(XSTATE_CPUID, xfeature_nr, &size, &offset, &ecx, &edx);

                src = get_xsave_addr(xsave, xfeature_nr);
                if (src)
                        memcpy(dest + offset, src, size);

                valid -= xfeature_mask;
        }

Btw, I'm wondering if that CPUID read in a loop can be replaced with
adding accessors for xstate_{offsets,sizes,..} etc and providing them to
kvm...


> @@ -4632,18 +4633,20 @@ static void load_xsave(struct kvm_vcpu *
>  	 */
>  	valid = xstate_bv & ~XFEATURE_MASK_FPSSE;
>  	while (valid) {
> +		u32 size, offset, ecx, edx;
>  		u64 xfeature_mask = valid & -valid;
>  		int xfeature_nr = fls64(xfeature_mask) - 1;
> -		void *dest = get_xsave_addr(xsave, xfeature_nr);
>  
> -		if (dest) {
> -			u32 size, offset, ecx, edx;
> -			cpuid_count(XSTATE_CPUID, xfeature_nr,
> -				    &size, &offset, &ecx, &edx);
> -			if (xfeature_nr == XFEATURE_PKRU)
> -				memcpy(&vcpu->arch.pkru, src + offset,
> -				       sizeof(vcpu->arch.pkru));
> -			else
> +		cpuid_count(XSTATE_CPUID, xfeature_nr,
> +			    &size, &offset, &ecx, &edx);
> +
> +		if (xfeature_nr == XFEATURE_PKRU) {
> +			memcpy(&vcpu->arch.pkru, src + offset,
> +			       sizeof(vcpu->arch.pkru));
> +		} else {
> +			void *dest = get_xsave_addr(xsave, xfeature_nr);
> +


^ Superfluous newline.

> +			if (dest)
>  				memcpy(dest, src + offset, size);
>  		}
>  
> 

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg