Re: [PATCH 15/14] x86/gds: Lock GDS mitigation when keylocker feature is present

[Chang S. Bae]

On 4/19/2024 10:47 AM, Pawan Gupta wrote:
>   
>   	/*
> @@ -840,6 +843,11 @@ static void __init gds_select_mitigation(void)
>   		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
>   	}
>   
> +	/* Keylocker can only be enabled when GDS mitigation is locked */
> +	if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> +	    gds_mitigation == GDS_MITIGATION_FULL)
> +		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> +

I'm having trouble understanding this change:

gds_select_mitigation()
{
	...
	if (gds_mitigation == GDS_MITIGATION_FORCE)
		gds_mitigation = GDS_MITIGATION_FULL;

	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
	if (mcu_ctrl & GDS_MITG_LOCKED) {
		...
		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
	}

	if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
	    gds_mitigation == GDS_MITIGATION_FULL)
		gds_mitigation = GDS_MITIGATION_FULL_LOCKED;

As I understand it, gds_mitigation is set to GDS_MITIGATION_FULL only if 
the gds force option is enabled but IA32_MCU_OPT_CTRL[GDS_MITG_LOCK] is 
not set.

Then, if the CPU has Key Locker, this code sets gds_mitigation to 
GDS_MITIGATION_FULL_LOCKED, which seems contradictory. I'm not sure why 
this change is necessary.

I'm also not convinced that the Key Locker series needs to modify this 
function. The Key Locker setup code should simply check the current 
mitigation status and enable the feature only if proper mitigation is in 
place. Am I missing something here?

Thanks,
Chang