[PATCH v3 07/17] x86/apic: Support LAPIC timer for Secure AVIC

Neeraj Upadhyay posted 17 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v3 07/17] x86/apic: Support LAPIC timer for Secure AVIC
Posted by Neeraj Upadhyay 1 month, 1 week ago
Secure AVIC requires LAPIC timer to be emulated by the hypervisor.
KVM already supports emulating LAPIC timer using hrtimers. In order
to emulate LAPIC timer, APIC_LVTT, APIC_TMICT and APIC_TDCR register
values need to be propagated to the hypervisor for arming the timer.
APIC_TMCCT register value has to be read from the hypervisor, which
is required for calibrating the APIC timer. So, read/write all APIC
timer registers from/to the hypervisor.

In addition, add a static call for apic's update_vector() callback,
to configure ALLOWED_IRR for the hypervisor to inject timer interrupt
using LOCAL_TIMER_VECTOR.

Co-developed-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v2:

 - Add static call for apic_update_vector()

 arch/x86/coco/sev/core.c            | 27 +++++++++++++++++++++++++++
 arch/x86/include/asm/apic.h         |  8 ++++++++
 arch/x86/include/asm/sev.h          |  2 ++
 arch/x86/kernel/apic/apic.c         |  2 ++
 arch/x86/kernel/apic/init.c         |  3 +++
 arch/x86/kernel/apic/vector.c       |  6 ------
 arch/x86/kernel/apic/x2apic_savic.c |  7 +++++--
 7 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index e53147a630c3..1122cf93983d 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1502,6 +1502,33 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 	return __vc_handle_msr(ghcb, ctxt, ctxt->insn.opcode.bytes[1] == 0x30);
 }
 
+u64 savic_ghcb_msr_read(u32 reg)
+{
+	u64 msr = APIC_BASE_MSR + (reg >> 4);
+	struct pt_regs regs = { .cx = msr };
+	struct es_em_ctxt ctxt = { .regs = &regs };
+	struct ghcb_state state;
+	unsigned long flags;
+	enum es_result ret;
+	struct ghcb *ghcb;
+
+	local_irq_save(flags);
+	ghcb = __sev_get_ghcb(&state);
+	vc_ghcb_invalidate(ghcb);
+
+	ret = __vc_handle_msr(ghcb, &ctxt, false);
+	if (ret != ES_OK) {
+		pr_err("Secure AVIC msr (0x%llx) read returned error (%d)\n", msr, ret);
+		/* MSR read failures are treated as fatal errors */
+		snp_abort();
+	}
+
+	__sev_put_ghcb(&state);
+	local_irq_restore(flags);
+
+	return regs.ax | regs.dx << 32;
+}
+
 void savic_ghcb_msr_write(u32 reg, u64 value)
 {
 	u64 msr = APIC_BASE_MSR + (reg >> 4);
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index b510008c586f..7616a622248c 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -338,6 +338,7 @@ struct apic_override {
 	void	(*icr_write)(u32 low, u32 high);
 	int	(*wakeup_secondary_cpu)(u32 apicid, unsigned long start_eip);
 	int	(*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip);
+	void	(*update_vector)(unsigned int cpu, unsigned int vector, bool set);
 };
 
 /*
@@ -397,6 +398,7 @@ DECLARE_APIC_CALL(wait_icr_idle);
 DECLARE_APIC_CALL(wakeup_secondary_cpu);
 DECLARE_APIC_CALL(wakeup_secondary_cpu_64);
 DECLARE_APIC_CALL(write);
+DECLARE_APIC_CALL(update_vector);
 
 static __always_inline u32 apic_read(u32 reg)
 {
@@ -473,6 +475,11 @@ static __always_inline bool apic_id_valid(u32 apic_id)
 	return apic_id <= apic->max_apic_id;
 }
 
+static __always_inline void apic_update_vector(unsigned int cpu, unsigned int vector, bool set)
+{
+	static_call(apic_call_update_vector)(cpu, vector, set);
+}
+
 #else /* CONFIG_X86_LOCAL_APIC */
 
 static inline u32 apic_read(u32 reg) { return 0; }
@@ -484,6 +491,7 @@ static inline void apic_wait_icr_idle(void) { }
 static inline u32 safe_apic_wait_icr_idle(void) { return 0; }
 static inline void apic_native_eoi(void) { WARN_ON_ONCE(1); }
 static inline void apic_setup_apic_calls(void) { }
+static inline void apic_update_vector(unsigned int cpu, unsigned int vector, bool set) { }
 
 #define apic_update_callback(_callback, _fn) do { } while (0)
 
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 855c705ee074..7c942b9c593a 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -484,6 +484,7 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
 void __init snp_secure_tsc_prepare(void);
 void __init snp_secure_tsc_init(void);
 enum es_result savic_register_gpa(u64 gpa);
+u64 savic_ghcb_msr_read(u32 reg);
 void savic_ghcb_msr_write(u32 reg, u64 value);
 
 #else	/* !CONFIG_AMD_MEM_ENCRYPT */
@@ -530,6 +531,7 @@ static inline void __init snp_secure_tsc_prepare(void) { }
 static inline void __init snp_secure_tsc_init(void) { }
 static inline enum es_result savic_register_gpa(u64 gpa) { return ES_UNSUPPORTED; }
 static inline void savic_ghcb_msr_write(u32 reg, u64 value) { }
+static inline u64 savic_ghcb_msr_read(u32 reg) { return 0; }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index f59ed284ec5b..86f9c3c7df1c 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -591,6 +591,8 @@ static void setup_APIC_timer(void)
 						0xF, ~0UL);
 	} else
 		clockevents_register_device(levt);
+
+	apic_update_vector(smp_processor_id(), LOCAL_TIMER_VECTOR, true);
 }
 
 /*
diff --git a/arch/x86/kernel/apic/init.c b/arch/x86/kernel/apic/init.c
index 821e2e536f19..b420f9cd0ddb 100644
--- a/arch/x86/kernel/apic/init.c
+++ b/arch/x86/kernel/apic/init.c
@@ -29,6 +29,7 @@ DEFINE_APIC_CALL(wait_icr_idle);
 DEFINE_APIC_CALL(wakeup_secondary_cpu);
 DEFINE_APIC_CALL(wakeup_secondary_cpu_64);
 DEFINE_APIC_CALL(write);
+DEFINE_APIC_CALL(update_vector);
 
 EXPORT_STATIC_CALL_TRAMP_GPL(apic_call_send_IPI_mask);
 EXPORT_STATIC_CALL_TRAMP_GPL(apic_call_send_IPI_self);
@@ -56,6 +57,7 @@ static __init void restore_override_callbacks(void)
 	apply_override(icr_write);
 	apply_override(wakeup_secondary_cpu);
 	apply_override(wakeup_secondary_cpu_64);
+	apply_override(update_vector);
 }
 
 #define update_call(__cb)					\
@@ -78,6 +80,7 @@ static __init void update_static_calls(void)
 	update_call(wait_icr_idle);
 	update_call(wakeup_secondary_cpu);
 	update_call(wakeup_secondary_cpu_64);
+	update_call(update_vector);
 }
 
 void __init apic_setup_apic_calls(void)
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 897e85e58139..09eb553269b8 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -139,12 +139,6 @@ static void apic_update_irq_cfg(struct irq_data *irqd, unsigned int vector,
 			    apicd->hw_irq_cfg.dest_apicid);
 }
 
-static inline void apic_update_vector(unsigned int cpu, unsigned int vector, bool set)
-{
-	if (apic->update_vector)
-		apic->update_vector(cpu, vector, set);
-}
-
 static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu)
 {
 	int vector;
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 657e560978e7..1088d82e3adb 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -83,6 +83,7 @@ static u32 x2apic_savic_read(u32 reg)
 	case APIC_TMICT:
 	case APIC_TMCCT:
 	case APIC_TDCR:
+		return savic_ghcb_msr_read(reg);
 	case APIC_ID:
 	case APIC_LVR:
 	case APIC_TASKPRI:
@@ -143,10 +144,12 @@ static void x2apic_savic_write(u32 reg, u32 data)
 {
 	switch (reg) {
 	case APIC_LVTT:
-	case APIC_LVT0:
-	case APIC_LVT1:
 	case APIC_TMICT:
 	case APIC_TDCR:
+		savic_ghcb_msr_write(reg, data);
+		break;
+	case APIC_LVT0:
+	case APIC_LVT1:
 	case APIC_TASKPRI:
 	case APIC_EOI:
 	case APIC_SPIV:
-- 
2.34.1
Re: [PATCH v3 07/17] x86/apic: Support LAPIC timer for Secure AVIC
Posted by Thomas Gleixner 1 month, 1 week ago
On Tue, Apr 01 2025 at 17:06, Neeraj Upadhyay wrote:
> In addition, add a static call for apic's update_vector() callback,
> to configure ALLOWED_IRR for the hypervisor to inject timer interrupt
> using LOCAL_TIMER_VECTOR.

How is this static call related to the timer vector? It just works with
the conditional callback. apic_update_vector() is not used in a
hotpath.

Even if there is a valid reason for the static call, why is this not
part of the patch, which adds the update_vector() callback?

It's well documented that you should not do random unrelated things in
patches.

You really try hard to make review a pain.

Thanks,

        tglx
Re: [PATCH v3 07/17] x86/apic: Support LAPIC timer for Secure AVIC
Posted by Neeraj Upadhyay 1 month, 1 week ago
On 4/3/2025 5:43 PM, Thomas Gleixner wrote:
> On Tue, Apr 01 2025 at 17:06, Neeraj Upadhyay wrote:
>> In addition, add a static call for apic's update_vector() callback,
>> to configure ALLOWED_IRR for the hypervisor to inject timer interrupt
>> using LOCAL_TIMER_VECTOR.
> 
> How is this static call related to the timer vector? It just works with
> the conditional callback. apic_update_vector() is not used in a
> hotpath.
>

No it is not related. I missed the point that static call is only for
callbacks which are used in hotpaths. I thought that they are for
callbacks which are called from multiple call sites. So, when I added
second caller here (first was added in 05/17, in vector.c), I converted
local apic_update_vector() in vector.c to static call.

 
> Even if there is a valid reason for the static call, why is this not
> part of the patch, which adds the update_vector() callback?
> 

Again, this was based on incorrect understanding mentioned above.

> It's well documented that you should not do random unrelated things in
> patches.
> 
> You really try hard to make review a pain.
> 

Apologies for that.


- Neeraj

> Thanks,
> 
>         tglx
> 
>