Message-ID: <20250102090734.GBZ3ZXVqpo0OgEwbrQ@fat_crate.local>
Date: Thu, 2 Jan 2025 10:07:34 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Nikunj A. Dadhania" <nikunj@....com>
Cc: thomas.lendacky@....com, linux-kernel@...r.kernel.org, x86@...nel.org,
	kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
	pbonzini@...hat.com
Subject: Re: [PATCH v15 06/13] x86/sev: Prevent GUEST_TSC_FREQ MSR
 interception for Secure TSC enabled guests

On Thu, Jan 02, 2025 at 10:33:26AM +0530, Nikunj A. Dadhania wrote:
> I think we are dragging this a little too far

What does that mean?

Is there some time limit I don't know about, on how long patches should be
reviewed on the mailing list?

> and the implementation[3] that I gave is good without any side effects.

Well, apparently not good enough - otherwise I won't say anything, would I?
And I wouldn't review your patches on my holiday, would I?

Geez.

Now lemme try again, this time in greater detail with the hope it is more
clear.

If you handle TSC MSRs and then you have a function __vc_handle_msr_tsc() then
*all* handling should happen there! There should not be if-conditionals in the
switch-case which makes following the code flow harder. That's why I'm asking
you to push the conditional inside. So that everything is concentrated
in a single function!

But there's this thing with handling TSC MSRs and non-STSC guests and that
needs special, later handling and decision.

So *that* needs to be made obvious.

As in: I will handle the TSC MSRs for STSC guests and the other flow for
non-STSC guests should remain. For now.

And make that goddamn explicit.

One possible way to do that is this:

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 6235286a0eda..61100532c259 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1439,7 +1439,7 @@ static enum es_result __vc_handle_msr_caa(struct pt_regs *regs, bool write)
  * Reads:  Reads of MSR_IA32_TSC should return the current TSC
  *         value, use the value returned by RDTSC.
  */
-static enum es_result __vc_handle_msr_tsc(struct pt_regs *regs, bool write)
+static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool write)
 {
 	u64 tsc;
 
@@ -1477,7 +1477,9 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 	case MSR_IA32_TSC:
 	case MSR_AMD64_GUEST_TSC_FREQ:
 		if (sev_status & MSR_AMD64_SNP_SECURE_TSC)
-			return __vc_handle_msr_tsc(regs, write);
+			return __vc_handle_secure_tsc_msrs(regs, write);
+		else
+			break;
 	default:
 		break;
 	}
---

You can still push the if-conditional inside the function but that'll need
more surgery as you need a different retval to tell vc_handle_msr() to
fall-through to the GHCB HV call instead of returning, yadda, yadda so this
version above is shorter.

And it can be revisited later, when we decide what we wanna do with TSC MSRs
on !STSC guests.

IOW, the code is still clear and there's enough breadcrumbs left to know what
needs to happen there in the future.

Versus: lemme drop my enablement patches and disappear and the maintainers can
mop up after me. Who cares! :-(

I hope this makes it a lot more clear now.

And again, if this takes too long for you, just lemme know: I have absolutely
no problem if someone else who's faster reviews your code - I have more than
enough TODOs on my plate so not dealing with this would be more than welcome
for me.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists