[RFC v2 00/17] AMD: Add Secure AVIC Guest Support

[RFC v2 00/17] AMD: Add Secure AVIC Guest Support

[Neeraj Upadhyay]

Introduction
------------

Secure AVIC is a new hardware feature in the AMD64 architecture to
allow SEV-SNP guests to prevent hypervisor from generating unexpected
interrupts to a vCPU or otherwise violate architectural assumptions
around APIC behavior.

One of the significant differences from AVIC or emulated x2APIC is that
Secure AVIC uses a guest-owned and managed APIC backing page. It also
introduces additional fields in both the VMCB and the Secure AVIC backing
page to aid the guest in limiting which interrupt vectors can be injected
into the guest.

Guest APIC Backing Page
-----------------------
Each vCPU has a guest-allocated APIC backing page of size 4K, which
maintains APIC state for that vCPU. The x2APIC MSRs are mapped at
their corresposing x2APIC MMIO offset within the guest APIC backing
page. All x2APIC accesses by guest or Secure AVIC hardware operate
on this backing page. The backing page should be pinned and NPT entry
for it should be always mapped while the corresponding vCPU is running.


MSR Accesses
------------
Secure AVIC only supports x2APIC MSR accesses. xAPIC MMIO offset based
accesses are not supported.

Some of the MSR accesses such as ICR writes (with shorthand equal to
self), SELF_IPI, EOI, TPR writes are accelerated by Secure AVIC
hardware. Other MSR accesses generate a #VC exception. The #VC
exception handler reads/writes to the guest APIC backing page.
As guest APIC backing page is accessible to the guest, the Secure
AVIC driver code optimizes APIC register access by directly
reading/writing to the guest APIC backing page (instead of taking
the #VC exception route).

In addition to the architected MSRs, following new fields are added to
the guest APIC backing page which can be modified directly by the
guest:

a. ALLOWED_IRR

ALLOWED_IRR vector indicates the interrupt vectors which the guest
allows the hypervisor to send. The combination of host-controlled
REQUESTED_IRR vectors (part of VMCB) and ALLOWED_IRR is used by
hardware to update the IRR vectors of the Guest APIC backing page.

#Offset        #bits        Description
204h           31:0         Guest allowed vectors 0-31
214h           31:0         Guest allowed vectors 32-63
...
274h           31:0         Guest allowed vectors 224-255

ALLOWED_IRR is meant to be used specifically for vectors that the
hypervisor is allowed to inject, such as device interrupts.  Interrupt
vectors used exclusively by the guest itself (like IPI vectors) should
not be allowed to be injected into the guest for security reasons.

b. NMI Request
 
#Offset        #bits        Description
278h           0            Set by Guest to request Virtual NMI


LAPIC Timer Support
-------------------
LAPIC timer is emulated by hypervisor. So, APIC_LVTT, APIC_TMICT and
APIC_TDCR, APIC_TMCCT APIC registers are not read/written to the guest
APIC backing page and are communicated to the hypervisor using SVM_EXIT_MSR
VMGEXIT. 

IPI Support
-----------
Only SELF_IPI is accelerated by Secure AVIC hardware. Other IPIs require
writing (from the Secure AVIC driver) to the IRR vector of the target CPU
backing page and then issuing VMGEXIT for the hypervisor to notify the
target vCPU.

KEXEC Support
-------------
Secure AVIC enabled guest can kexec to another kernel which has Secure
AVIC enabled, as the Hypervisor has Secure AVIC feature bit set in the
sev_status.

Driver Implementation Open Points
---------------------------------

The Secure AVIC driver only supports physical destination mode. If
logical destination mode need to be supported, then a separate x2apic
driver would be required for supporting logical destination mode.

Setting of ALLOWED_IRR vectors is done from vector.c for IOAPIC and MSI
interrupts. ALLOWED_IRR vector is not cleared when an interrupt vector
migrates to different CPU. Using a cleaner approach to manage and
configure allowed vectors needs more work.


Testing
-------

This series is based on top of commit 0f966b199269 "Merge branch into tip/master:
'x86/platform'" of tip/tip master branch.

Host Secure AVIC support patch series is at [1].

Qemu support patch is at [2].

QEMU commandline for testing Secure AVIC enabled guest:

qemu-system-x86_64 <...> -object sev-snp-guest,id=sev0,policy=0xb0000,cbitpos=51,reduced-phys-bits=1,allowed-sev-features=true,secure-avic=true

Following tests are done:

1) Boot to Prompt using initramfs and ubuntu fs.
2) Verified timer and IPI as part of the guest bootup.
3) Verified long run SCF TORTURE IPI test.

[1] https://github.com/AMDESE/linux-kvm/tree/savic-host-latest
[2] https://github.com/AMDESE/qemu/tree/secure-avic

Change since v1

  - Added Kexec support.
  - Instead of doing a 2M aligned allocation for backing pages,
    allocate individual PAGE_SIZE pages for vCPUs.
  - Instead of reading Extended Topology Enumeration CPUID, APIC_ID
    value is read from Hv and updated in APIC backing page. Hv returned
    ID is checked for any duplicates.
  - Propagate all LVT* register reads and writes to Hv.
  - Check that Secure AVIC control MSR is not intercepted by Hv.
  - Fix EOI handling for level-triggered interrupts.
  - Misc cleanups and commit log updates.

Kishon Vijay Abraham I (5):
  x86/apic: Support LAPIC timer for Secure AVIC
  x86/sev: Initialize VGIF for secondary VCPUs for Secure AVIC
  x86/apic: Add support to send NMI IPI for Secure AVIC
  x86/sev: Enable NMI support for Secure AVIC
  x86/sev: Indicate SEV-SNP guest supports Secure AVIC

Neeraj Upadhyay (12):
  x86/apic: Add new driver for Secure AVIC
  x86/apic: Initialize Secure AVIC APIC backing page
  x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver
  x86/apic: Initialize APIC ID for Secure AVIC
  x86/apic: Add update_vector callback for Secure AVIC
  x86/apic: Add support to send IPI for Secure AVIC
  x86/apic: Allow NMI to be injected from hypervisor for Secure AVIC
  x86/apic: Read and write LVT* APIC registers from HV for SAVIC guests
  x86/apic: Handle EOI writes for SAVIC guests
  x86/apic: Add kexec support for Secure AVIC
  x86/apic: Enable Secure AVIC in Control MSR
  x86/sev: Prevent SECURE_AVIC_CONTROL MSR interception for Secure AVIC
    guests

 arch/x86/Kconfig                    |  14 +
 arch/x86/boot/compressed/sev.c      |   4 +-
 arch/x86/coco/core.c                |   3 +
 arch/x86/coco/sev/core.c            | 145 +++++++-
 arch/x86/include/asm/apic.h         |   4 +
 arch/x86/include/asm/apicdef.h      |   2 +
 arch/x86/include/asm/msr-index.h    |   9 +-
 arch/x86/include/asm/sev.h          |  10 +
 arch/x86/include/uapi/asm/svm.h     |   3 +
 arch/x86/kernel/apic/Makefile       |   1 +
 arch/x86/kernel/apic/apic.c         |   9 +
 arch/x86/kernel/apic/vector.c       |   8 +
 arch/x86/kernel/apic/x2apic_savic.c | 530 ++++++++++++++++++++++++++++
 include/linux/cc_platform.h         |   8 +
 14 files changed, 743 insertions(+), 7 deletions(-)
 create mode 100644 arch/x86/kernel/apic/x2apic_savic.c


base-commit: 0f966b1992694763de4dae6bdf817c5c1c6fc66d
-- 
2.34.1

[RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

The Secure AVIC feature provides SEV-SNP guests hardware acceleration
for performance sensitive APIC accesses while securely managing the
guest-owned APIC state through the use of a private APIC backing page.
This helps prevent hypervisor from generating unexpected interrupts for
a vCPU or otherwise violate architectural assumptions around APIC
behavior.

Add a new x2APIC driver that will serve as the base of the Secure AVIC
support. It is initially the same as the x2APIC phys driver, but will be
modified as features of Secure AVIC are implemented.

If the hypervisor sets the Secure AVIC bit in SEV_STATUS and the bit is
not set in SNP_FEATURES_PRESENT, maintain the current behavior to
enforce the guest termination.

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 v1:

 - Updated the commit log to highlight the behavior for the case when
   guest SNP_FEATURES_PRESENT does not have SECURE AVIC set and
   Hv has set the bit in SEV_STATUS.

 - Select AMD_SECURE_AVIC config if AMD_MEM_ENCRYPT config is enabled.

 - Updated the config AMD_SECURE_AVIC description to highlight
   functional dependency on x2apic enablement.

 arch/x86/Kconfig                    |  14 ++++
 arch/x86/boot/compressed/sev.c      |   1 +
 arch/x86/coco/core.c                |   3 +
 arch/x86/include/asm/msr-index.h    |   4 +-
 arch/x86/kernel/apic/Makefile       |   1 +
 arch/x86/kernel/apic/x2apic_savic.c | 112 ++++++++++++++++++++++++++++
 include/linux/cc_platform.h         |   8 ++
 7 files changed, 142 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/kernel/apic/x2apic_savic.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 42c8a69bfb49..7776645e71d1 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -474,6 +474,19 @@ config X86_X2APIC
 
 	  If you don't know what to do here, say N.
 
+config AMD_SECURE_AVIC
+	bool "AMD Secure AVIC"
+	depends on X86_X2APIC
+	help
+	  This enables AMD Secure AVIC support on guests that have this feature.
+
+	  AMD Secure AVIC provides hardware acceleration for performance sensitive
+	  APIC accesses and support for managing guest owned APIC state for SEV-SNP
+	  guests. Secure AVIC does not support xapic mode. It has functional
+	  dependency on x2apic being enabled in the guest.
+
+	  If you don't know what to do here, say N.
+
 config X86_POSTED_MSI
 	bool "Enable MSI and MSI-x delivery by posted interrupts"
 	depends on X86_64 && IRQ_REMAP
@@ -1557,6 +1570,7 @@ config AMD_MEM_ENCRYPT
 	select X86_MEM_ENCRYPT
 	select UNACCEPTED_MEMORY
 	select CRYPTO_LIB_AESGCM
+	select AMD_SECURE_AVIC
 	help
 	  Say yes to enable support for the encryption of system memory.
 	  This requires an AMD processor that supports Secure Memory
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index bb55934c1cee..798fdd3dbd1e 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -394,6 +394,7 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
 				 MSR_AMD64_SNP_VMSA_REG_PROT |		\
 				 MSR_AMD64_SNP_RESERVED_BIT13 |		\
 				 MSR_AMD64_SNP_RESERVED_BIT15 |		\
+				 MSR_AMD64_SNP_SECURE_AVIC |		\
 				 MSR_AMD64_SNP_RESERVED_MASK)
 
 /*
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 9a0ddda3aa69..3d7bf37e2155 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -102,6 +102,9 @@ static bool noinstr amd_cc_platform_has(enum cc_attr attr)
 	case CC_ATTR_HOST_SEV_SNP:
 		return cc_flags.host_sev_snp;
 
+	case CC_ATTR_SNP_SECURE_AVIC:
+		return sev_status & MSR_AMD64_SNP_SECURE_AVIC;
+
 	default:
 		return false;
 	}
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 72765b2fe0d8..a42d88e9def8 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -683,7 +683,9 @@
 #define MSR_AMD64_SNP_VMSA_REG_PROT	BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
 #define MSR_AMD64_SNP_SMT_PROT_BIT	17
 #define MSR_AMD64_SNP_SMT_PROT		BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
-#define MSR_AMD64_SNP_RESV_BIT		18
+#define MSR_AMD64_SNP_SECURE_AVIC_BIT	18
+#define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
+#define MSR_AMD64_SNP_RESV_BIT		19
 #define MSR_AMD64_SNP_RESERVED_MASK	GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
 #define MSR_AMD64_RMP_BASE		0xc0010132
 #define MSR_AMD64_RMP_END		0xc0010133
diff --git a/arch/x86/kernel/apic/Makefile b/arch/x86/kernel/apic/Makefile
index 3bf0487cf3b7..12153993c12b 100644
--- a/arch/x86/kernel/apic/Makefile
+++ b/arch/x86/kernel/apic/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_X86_64),y)
 # APIC probe will depend on the listing order here
 obj-$(CONFIG_X86_NUMACHIP)	+= apic_numachip.o
 obj-$(CONFIG_X86_UV)		+= x2apic_uv_x.o
+obj-$(CONFIG_AMD_SECURE_AVIC)	+= x2apic_savic.o
 obj-$(CONFIG_X86_X2APIC)	+= x2apic_phys.o
 obj-$(CONFIG_X86_X2APIC)	+= x2apic_cluster.o
 obj-y				+= apic_flat_64.o
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
new file mode 100644
index 000000000000..c3a4d387c63f
--- /dev/null
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Secure AVIC Support (SEV-SNP Guests)
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *
+ * Author: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
+ */
+
+#include <linux/cpumask.h>
+#include <linux/cc_platform.h>
+
+#include <asm/apic.h>
+#include <asm/sev.h>
+
+#include "local.h"
+
+static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
+}
+
+static void x2apic_savic_send_IPI(int cpu, int vector)
+{
+	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
+
+	/* x2apic MSRs are special and need a special fence: */
+	weak_wrmsr_fence();
+	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
+}
+
+static void
+__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
+{
+	unsigned long query_cpu;
+	unsigned long this_cpu;
+	unsigned long flags;
+
+	/* x2apic MSRs are special and need a special fence: */
+	weak_wrmsr_fence();
+
+	local_irq_save(flags);
+
+	this_cpu = smp_processor_id();
+	for_each_cpu(query_cpu, mask) {
+		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
+			continue;
+		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
+				       vector, APIC_DEST_PHYSICAL);
+	}
+	local_irq_restore(flags);
+}
+
+static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
+{
+	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
+}
+
+static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
+{
+	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
+}
+
+static int x2apic_savic_probe(void)
+{
+	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
+		return 0;
+
+	if (!x2apic_mode) {
+		pr_err("Secure AVIC enabled in non x2APIC mode\n");
+		snp_abort();
+	}
+
+	pr_info("Secure AVIC Enabled\n");
+
+	return 1;
+}
+
+static struct apic apic_x2apic_savic __ro_after_init = {
+
+	.name				= "secure avic x2apic",
+	.probe				= x2apic_savic_probe,
+	.acpi_madt_oem_check		= x2apic_savic_acpi_madt_oem_check,
+
+	.dest_mode_logical		= false,
+
+	.disable_esr			= 0,
+
+	.cpu_present_to_apicid		= default_cpu_present_to_apicid,
+
+	.max_apic_id			= UINT_MAX,
+	.x2apic_set_max_apicid		= true,
+	.get_apic_id			= x2apic_get_apic_id,
+
+	.calc_dest_apicid		= apic_default_calc_apicid,
+
+	.send_IPI			= x2apic_savic_send_IPI,
+	.send_IPI_mask			= x2apic_savic_send_IPI_mask,
+	.send_IPI_mask_allbutself	= x2apic_savic_send_IPI_mask_allbutself,
+	.send_IPI_allbutself		= x2apic_send_IPI_allbutself,
+	.send_IPI_all			= x2apic_send_IPI_all,
+	.send_IPI_self			= x2apic_send_IPI_self,
+	.nmi_to_offline_cpu		= true,
+
+	.read				= native_apic_msr_read,
+	.write				= native_apic_msr_write,
+	.eoi				= native_apic_msr_eoi,
+	.icr_read			= native_x2apic_icr_read,
+	.icr_write			= native_x2apic_icr_write,
+};
+
+apic_driver(apic_x2apic_savic);
diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
index 0bf7d33a1048..7fcec025c5e0 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -96,6 +96,14 @@ enum cc_attr {
 	 * enabled to run SEV-SNP guests.
 	 */
 	CC_ATTR_HOST_SEV_SNP,
+
+	/**
+	 * @CC_ATTR_SNP_SECURE_AVIC: Secure AVIC mode is active.
+	 *
+	 * The host kernel is running with the necessary features enabled
+	 * to run SEV-SNP guests with full Secure AVIC capabilities.
+	 */
+	CC_ATTR_SNP_SECURE_AVIC,
 };
 
 #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
-- 
2.34.1

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Wed, Feb 26, 2025 at 02:35:09PM +0530, Neeraj Upadhyay wrote:
> +config AMD_SECURE_AVIC
> +	bool "AMD Secure AVIC"
> +	depends on X86_X2APIC
> +	help
> +	  This enables AMD Secure AVIC support on guests that have this feature.

"Enable this to get ..."

> +	  AMD Secure AVIC provides hardware acceleration for performance sensitive
> +	  APIC accesses and support for managing guest owned APIC state for SEV-SNP
> +	  guests. Secure AVIC does not support xapic mode. It has functional
> +	  dependency on x2apic being enabled in the guest.
> +
> +	  If you don't know what to do here, say N.
> +
>  config X86_POSTED_MSI
>  	bool "Enable MSI and MSI-x delivery by posted interrupts"
>  	depends on X86_64 && IRQ_REMAP
> @@ -1557,6 +1570,7 @@ config AMD_MEM_ENCRYPT
>  	select X86_MEM_ENCRYPT
>  	select UNACCEPTED_MEMORY
>  	select CRYPTO_LIB_AESGCM
> +	select AMD_SECURE_AVIC

AMD_MEM_ENCRYPT doesn't absolutely need AMD_SECURE_AVIC so this can go.

>  	help
>  	  Say yes to enable support for the encryption of system memory.
>  	  This requires an AMD processor that supports Secure Memory

...

> +static void x2apic_savic_send_IPI(int cpu, int vector)
> +{
> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
> +
> +	/* x2apic MSRs are special and need a special fence: */
> +	weak_wrmsr_fence();
> +	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
> +}
> +
> +static void

Unnecessary line break.

> +__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
> +{
> +	unsigned long query_cpu;
> +	unsigned long this_cpu;
> +	unsigned long flags;
> +
> +	/* x2apic MSRs are special and need a special fence: */
> +	weak_wrmsr_fence();
> +
> +	local_irq_save(flags);
> +
> +	this_cpu = smp_processor_id();
> +	for_each_cpu(query_cpu, mask) {
> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
> +			continue;
> +		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
> +				       vector, APIC_DEST_PHYSICAL);
> +	}
> +	local_irq_restore(flags);
> +}
> +
> +static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
> +{
> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
> +}
> +
> +static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
> +{
> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
> +}
> +
> +static int x2apic_savic_probe(void)
> +{
> +	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
> +		return 0;
> +
> +	if (!x2apic_mode) {
> +		pr_err("Secure AVIC enabled in non x2APIC mode\n");
> +		snp_abort();
> +	}
> +
> +	pr_info("Secure AVIC Enabled\n");

That's not necessary.

Actually, you could figure out why that

	pr_info("Switched APIC routing to: %s\n", driver->name);

doesn't come out in current kernels anymore:

$ dmesg | grep -i "switched apic"
$

and fix that as a separate patch.

Looks like it broke in 6.10 or so:

$ grep -E "Switched APIC" *
04-rc7+:Switched APIC routing to physical flat.
05-rc1+:Switched APIC routing to physical flat.
05-rc2+:Switched APIC routing to physical flat.
05-rc3+:Switched APIC routing to physical flat.
05-rc4+:APIC: Switched APIC routing to: physical flat
05-rc6+:Switched APIC routing to physical flat.
06-rc4+:APIC: Switched APIC routing to: physical flat
06-rc6+:APIC: Switched APIC routing to: physical flat
07-0+:APIC: Switched APIC routing to: physical flat
07-rc1+:APIC: Switched APIC routing to: physical flat
07-rc7+:APIC: Switched APIC routing to: physical flat
08-rc1+:APIC: Switched APIC routing to: physical flat
08-rc3+:APIC: Switched APIC routing to: physical flat
08-rc6+:APIC: Switched APIC routing to: physical flat
08-rc7+:APIC: Switched APIC routing to: physical flat
09-rc7+:APIC: Switched APIC routing to: physical flat
10-rc1+:APIC: Switched APIC routing to: physical flat
10-rc6+:APIC: Switched APIC routing to: physical flat
<--- EOF

Thx.


-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 3/20/2025 9:21 PM, Borislav Petkov wrote:
> On Wed, Feb 26, 2025 at 02:35:09PM +0530, Neeraj Upadhyay wrote:
>> +config AMD_SECURE_AVIC
>> +	bool "AMD Secure AVIC"
>> +	depends on X86_X2APIC
>> +	help
>> +	  This enables AMD Secure AVIC support on guests that have this feature.
> 
> "Enable this to get ..."
> 

Will update.

>> +	  AMD Secure AVIC provides hardware acceleration for performance sensitive
>> +	  APIC accesses and support for managing guest owned APIC state for SEV-SNP
>> +	  guests. Secure AVIC does not support xapic mode. It has functional
>> +	  dependency on x2apic being enabled in the guest.
>> +
>> +	  If you don't know what to do here, say N.
>> +
>>  config X86_POSTED_MSI
>>  	bool "Enable MSI and MSI-x delivery by posted interrupts"
>>  	depends on X86_64 && IRQ_REMAP
>> @@ -1557,6 +1570,7 @@ config AMD_MEM_ENCRYPT
>>  	select X86_MEM_ENCRYPT
>>  	select UNACCEPTED_MEMORY
>>  	select CRYPTO_LIB_AESGCM
>> +	select AMD_SECURE_AVIC
> 
> AMD_MEM_ENCRYPT doesn't absolutely need AMD_SECURE_AVIC so this can go.
> 

The intent here is to prevent a configuration where CONFIG_AMD_SECURE_AVIC
is disabled in build  and sev_status (features enabled in hypervisor) says Secure
AVIC is enabled. In this configuration, while SNP_FEATURES_PRESENT says
Secure AVIC feature is present in guest and snp_get_unsupported_features()
would not flag mismatched features between host and guest, guest would boot
without Secure AVIC apic driver being selected. Do you think we should
handle this case differently and not force select AMD_SECURE_AVIC config
when AMD_MEM_ENCRYPT config is enabled?

#define SNP_FEATURES_PRESENT    (MSR_AMD64_SNP_DEBUG_SWAP |     \
                                 MSR_AMD64_SNP_SECURE_TSC |     \
                                 MSR_AMD64_SNP_SECURE_AVIC)

u64 snp_get_unsupported_features(u64 status)
{
        if (!(status & MSR_AMD64_SEV_SNP_ENABLED))
                return 0;

        return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
}



>>  	help
>>  	  Say yes to enable support for the encryption of system memory.
>>  	  This requires an AMD processor that supports Secure Memory
> 
> ...
> 
>> +static void x2apic_savic_send_IPI(int cpu, int vector)
>> +{
>> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
>> +}
>> +
>> +static void
> 
> Unnecessary line break.
> 

Will update.

>> +__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
>> +{
>> +	unsigned long query_cpu;
>> +	unsigned long this_cpu;
>> +	unsigned long flags;
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +
>> +	local_irq_save(flags);
>> +
>> +	this_cpu = smp_processor_id();
>> +	for_each_cpu(query_cpu, mask) {
>> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
>> +			continue;
>> +		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
>> +				       vector, APIC_DEST_PHYSICAL);
>> +	}
>> +	local_irq_restore(flags);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>> +}
>> +
>> +static int x2apic_savic_probe(void)
>> +{
>> +	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
>> +		return 0;
>> +
>> +	if (!x2apic_mode) {
>> +		pr_err("Secure AVIC enabled in non x2APIC mode\n");
>> +		snp_abort();
>> +	}
>> +
>> +	pr_info("Secure AVIC Enabled\n");
> 
> That's not necessary.
> 

Will update.

> Actually, you could figure out why that
> 
> 	pr_info("Switched APIC routing to: %s\n", driver->name);
> 
> doesn't come out in current kernels anymore:
> 

Interesting. I see it working on 6.14-rc7 and master branch.

dmesg | grep -i "switched apic"
[    1.044435] APIC: Switched APIC routing to: physical x2apic


- Neeraj

> $ dmesg | grep -i "switched apic"
> $
> 
> and fix that as a separate patch.
> 
> Looks like it broke in 6.10 or so:
> 
> $ grep -E "Switched APIC" *
> 04-rc7+:Switched APIC routing to physical flat.
> 05-rc1+:Switched APIC routing to physical flat.
> 05-rc2+:Switched APIC routing to physical flat.
> 05-rc3+:Switched APIC routing to physical flat.
> 05-rc4+:APIC: Switched APIC routing to: physical flat
> 05-rc6+:Switched APIC routing to physical flat.
> 06-rc4+:APIC: Switched APIC routing to: physical flat
> 06-rc6+:APIC: Switched APIC routing to: physical flat
> 07-0+:APIC: Switched APIC routing to: physical flat
> 07-rc1+:APIC: Switched APIC routing to: physical flat
> 07-rc7+:APIC: Switched APIC routing to: physical flat
> 08-rc1+:APIC: Switched APIC routing to: physical flat
> 08-rc3+:APIC: Switched APIC routing to: physical flat
> 08-rc6+:APIC: Switched APIC routing to: physical flat
> 08-rc7+:APIC: Switched APIC routing to: physical flat
> 09-rc7+:APIC: Switched APIC routing to: physical flat
> 10-rc1+:APIC: Switched APIC routing to: physical flat
> 10-rc6+:APIC: Switched APIC routing to: physical flat
> <--- EOF
> 
> Thx.
> 
> 

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Fri, Mar 21, 2025 at 09:14:15AM +0530, Neeraj Upadhyay wrote:
> Do you think we should handle this case differently and not force select
> AMD_SECURE_AVIC config when AMD_MEM_ENCRYPT config is enabled?

Yeah, you'd have to do some simple CONFIG_AMD_SECURE_AVIC ifdeffery and add
(or not) the flag to SNP_FEATURES_PRESENT, depending...

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 7:25 PM, Borislav Petkov wrote:
> On Fri, Mar 21, 2025 at 09:14:15AM +0530, Neeraj Upadhyay wrote:
>> Do you think we should handle this case differently and not force select
>> AMD_SECURE_AVIC config when AMD_MEM_ENCRYPT config is enabled?
> 
> Yeah, you'd have to do some simple CONFIG_AMD_SECURE_AVIC ifdeffery and add
> (or not) the flag to SNP_FEATURES_PRESENT, depending...
> 

Ok, something like below?


+#ifdef CONFIG_AMD_SECURE_AVIC
+#define SNP_SECURE_AVIC    MSR_AMD64_SNP_SECURE_AVIC
+#else
+#define SNP_SECURE_AVIC    0
+#endif
+

 #define SNP_FEATURES_PRESENT   (MSR_AMD64_SNP_DEBUG_SWAP |     \
                                 MSR_AMD64_SNP_SECURE_TSC |     \
-                                MSR_AMD64_SNP_SECURE_AVIC)
+                                SNP_SECURE_AVIC)


- Neeraj

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Fri, Mar 21, 2025 at 09:39:22PM +0530, Neeraj Upadhyay wrote:
> Ok, something like below?

Or something like that:

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 76b16a2b03ee..1a5fa10ee4b9 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -476,7 +476,7 @@ config X86_X2APIC
 
 config AMD_SECURE_AVIC
 	bool "AMD Secure AVIC"
-	depends on X86_X2APIC
+	depends on AMD_MEM_ENCRYPT && X86_X2APIC
 	help
 	  This enables AMD Secure AVIC support on guests that have this feature.
 
@@ -1517,7 +1517,6 @@ config AMD_MEM_ENCRYPT
 	select X86_MEM_ENCRYPT
 	select UNACCEPTED_MEMORY
 	select CRYPTO_LIB_AESGCM
-	select AMD_SECURE_AVIC
 	help
 	  Say yes to enable support for the encryption of system memory.
 	  This requires an AMD processor that supports Secure Memory
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index edc31615cb67..ecf86b8a6601 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -685,8 +685,14 @@
 #define MSR_AMD64_SNP_VMSA_REG_PROT	BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
 #define MSR_AMD64_SNP_SMT_PROT_BIT	17
 #define MSR_AMD64_SNP_SMT_PROT		BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
+
 #define MSR_AMD64_SNP_SECURE_AVIC_BIT	18
-#define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
+#ifdef CONFIG_AMD_SECURE_AVIC
+#define MSR_AMD64_SNP_SECURE_AVIC	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
+#else
+#define MSR_AMD64_SNP_SECURE_AVIC	0
+#endif
+
 #define MSR_AMD64_SNP_RESV_BIT		19
 #define MSR_AMD64_SNP_RESERVED_MASK	GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
 #define MSR_AMD64_RMP_BASE		0xc0010132

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 10:41 PM, Borislav Petkov wrote:
> On Fri, Mar 21, 2025 at 09:39:22PM +0530, Neeraj Upadhyay wrote:
>> Ok, something like below?
> 
> Or something like that:
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 76b16a2b03ee..1a5fa10ee4b9 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -476,7 +476,7 @@ config X86_X2APIC
>  
>  config AMD_SECURE_AVIC
>  	bool "AMD Secure AVIC"
> -	depends on X86_X2APIC
> +	depends on AMD_MEM_ENCRYPT && X86_X2APIC
>  	help
>  	  This enables AMD Secure AVIC support on guests that have this feature.
>  
> @@ -1517,7 +1517,6 @@ config AMD_MEM_ENCRYPT
>  	select X86_MEM_ENCRYPT
>  	select UNACCEPTED_MEMORY
>  	select CRYPTO_LIB_AESGCM
> -	select AMD_SECURE_AVIC
>  	help
>  	  Say yes to enable support for the encryption of system memory.
>  	  This requires an AMD processor that supports Secure Memory
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index edc31615cb67..ecf86b8a6601 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -685,8 +685,14 @@
>  #define MSR_AMD64_SNP_VMSA_REG_PROT	BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
>  #define MSR_AMD64_SNP_SMT_PROT_BIT	17
>  #define MSR_AMD64_SNP_SMT_PROT		BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
> +
>  #define MSR_AMD64_SNP_SECURE_AVIC_BIT	18
> -#define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
> +#ifdef CONFIG_AMD_SECURE_AVIC
> +#define MSR_AMD64_SNP_SECURE_AVIC	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
> +#else
> +#define MSR_AMD64_SNP_SECURE_AVIC	0
> +#endif
> +

I missed this part. I think this does not work because if CONFIG_AMD_SECURE_AVIC
is not enabled, MSR_AMD64_SNP_SECURE_AVIC bit becomes 0 in both SNP_FEATURES_IMPL_REQ
and SNP_FEATURES_PRESENT.

So, snp_get_unsupported_features() won't report SECURE_AVIC feature as not being
enabled in guest launched with SECURE_AVIC vmsa feature enabled. Thoughts?

u64 snp_get_unsupported_features(u64 status)
{
        if (!(status & MSR_AMD64_SEV_SNP_ENABLED))
                return 0;

        return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
}


- Neeraj

>  #define MSR_AMD64_SNP_RESV_BIT		19
>  #define MSR_AMD64_SNP_RESERVED_MASK	GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
>  #define MSR_AMD64_RMP_BASE		0xc0010132
> 

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Tue, Apr 01, 2025 at 10:42:17AM +0530, Neeraj Upadhyay wrote:
> > diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> > index edc31615cb67..ecf86b8a6601 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -685,8 +685,14 @@
> >  #define MSR_AMD64_SNP_VMSA_REG_PROT	BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
> >  #define MSR_AMD64_SNP_SMT_PROT_BIT	17
> >  #define MSR_AMD64_SNP_SMT_PROT		BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
> > +
> >  #define MSR_AMD64_SNP_SECURE_AVIC_BIT	18
> > -#define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
> > +#ifdef CONFIG_AMD_SECURE_AVIC
> > +#define MSR_AMD64_SNP_SECURE_AVIC	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
> > +#else
> > +#define MSR_AMD64_SNP_SECURE_AVIC	0
> > +#endif
> > +
> 
> I missed this part. I think this does not work because if CONFIG_AMD_SECURE_AVIC
> is not enabled, MSR_AMD64_SNP_SECURE_AVIC bit becomes 0 in both SNP_FEATURES_IMPL_REQ
> and SNP_FEATURES_PRESENT.
> 
> So, snp_get_unsupported_features() won't report SECURE_AVIC feature as not being
> enabled in guest launched with SECURE_AVIC vmsa feature enabled. Thoughts?

Your formulations are killing me :-P

... won't report.. as not being enabled ... with feature enabled.

Double negation with a positive at the end.

So this translates to

"will report as enabled when enabled"

which doesn't make too much sense.

*IF* you have CONFIG_AMD_SECURE_AVIC disabled, then you don't have SAVIC
support and then SAVIC VMSA feature bit better be 0.

Or what do you mean?

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 4/2/2025 3:17 PM, Borislav Petkov wrote:
> On Tue, Apr 01, 2025 at 10:42:17AM +0530, Neeraj Upadhyay wrote:
>>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>>> index edc31615cb67..ecf86b8a6601 100644
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -685,8 +685,14 @@
>>>  #define MSR_AMD64_SNP_VMSA_REG_PROT	BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
>>>  #define MSR_AMD64_SNP_SMT_PROT_BIT	17
>>>  #define MSR_AMD64_SNP_SMT_PROT		BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
>>> +
>>>  #define MSR_AMD64_SNP_SECURE_AVIC_BIT	18
>>> -#define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
>>> +#ifdef CONFIG_AMD_SECURE_AVIC
>>> +#define MSR_AMD64_SNP_SECURE_AVIC	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
>>> +#else
>>> +#define MSR_AMD64_SNP_SECURE_AVIC	0
>>> +#endif
>>> +
>>
>> I missed this part. I think this does not work because if CONFIG_AMD_SECURE_AVIC
>> is not enabled, MSR_AMD64_SNP_SECURE_AVIC bit becomes 0 in both SNP_FEATURES_IMPL_REQ
>> and SNP_FEATURES_PRESENT.
>>
>> So, snp_get_unsupported_features() won't report SECURE_AVIC feature as not being
>> enabled in guest launched with SECURE_AVIC vmsa feature enabled. Thoughts?
> 
> Your formulations are killing me :-P
> 
> ... won't report.. as not being enabled ... with feature enabled.
> 
> Double negation with a positive at the end.
> 
> So this translates to
> 
> "will report as enabled when enabled"
> 
> which doesn't make too much sense.
> 
> *IF* you have CONFIG_AMD_SECURE_AVIC disabled, then you don't have SAVIC
> support and then SAVIC VMSA feature bit better be 0.
> 
> Or what do you mean?
> 

My bad. Let me try again.

In previous sentence

"SECURE_AVIC feature as not being enabled" - SAVIC not enabled inside guest.
"SECURE_AVIC vmsa feature enabled"  - SAVIC enabled/active in hypervisor for that guest.

This is basically continuation of our previous discussion here [1]

- "sev_status" reports the SEV features enabled/active in Hypervisor for a guest.

- If guest is launched (qemu/VMM launch) with SAVIC VMSA feature enabled, hypervisor
  uses SAVIC interrupt injection flow for that guest.

  SAVIC VMSA feature is reported in "sev_status" and tells guest that SAVIC 
  functionality is active (in hypervisor) for that guest.

- snp_get_unsupported_features() looks like below.
  It checks that, for the feature bits which are part of SNP_FEATURES_IMPL_REQ,
  if they are enabled in hypervisor (and so reported in sev_status),
  guest need to implement/enable those features. SAVIC also falls in that category
  of SNP features.

  So, if CONFIG_AMD_SECURE_AVIC is disabled, guest would run with SAVIC feature
  disabled in guest. This would cause undefined behavior for that guest if SAVIC
  feature is active for that guest in hypervisor.

 
  u64 snp_get_unsupported_features(u64 status)  << status = sev_status
  {
        if (!(status & MSR_AMD64_SEV_SNP_ENABLED))
                return 0;

        return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
  }

  /*
   * SNP_FEATURES_IMPL_REQ is the mask of SNP features that will need
   * guest side implementation for proper functioning of the guest. If any
   * of these features are enabled in the hypervisor but are lacking guest
   * side implementation, the behavior of the guest will be undefined. The
   * guest could fail in non-obvious way making it difficult to debug.
   */
  #define SNP_FEATURES_IMPL_REQ ...



 

[1] https://lore.kernel.org/lkml/20241009110224.GGZwZiwD27ZvME841d@fat_crate.local/#t


- Neeraj

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Wed, Apr 02, 2025 at 04:04:34PM +0530, Neeraj Upadhyay wrote:
> - snp_get_unsupported_features() looks like below.
>   It checks that, for the feature bits which are part of SNP_FEATURES_IMPL_REQ,
>   if they are enabled in hypervisor (and so reported in sev_status),
>   guest need to implement/enable those features. SAVIC also falls in that category
>   of SNP features.
> 
>   So, if CONFIG_AMD_SECURE_AVIC is disabled, guest would run with SAVIC feature
>   disabled in guest. This would cause undefined behavior for that guest if SAVIC
>   feature is active for that guest in hypervisor.

Ok, so SNP_FEATURES_IMPL_REQ will contain the SAVIC bit (unconditionally,
without the ifdeffery) and SNP_FEATURES_PRESENT will contain that thing you
had suggested with the ifdeffery to denote whether SAVIC support has been
enabled at build time:

https://lore.kernel.org/r/e0362a96-4b3a-44b1-8d54-806a6b045799@amd.com

or not.

Right?

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 4/7/2025 6:47 PM, Borislav Petkov wrote:
> On Wed, Apr 02, 2025 at 04:04:34PM +0530, Neeraj Upadhyay wrote:
>> - snp_get_unsupported_features() looks like below.
>>   It checks that, for the feature bits which are part of SNP_FEATURES_IMPL_REQ,
>>   if they are enabled in hypervisor (and so reported in sev_status),
>>   guest need to implement/enable those features. SAVIC also falls in that category
>>   of SNP features.
>>
>>   So, if CONFIG_AMD_SECURE_AVIC is disabled, guest would run with SAVIC feature
>>   disabled in guest. This would cause undefined behavior for that guest if SAVIC
>>   feature is active for that guest in hypervisor.
> 
> Ok, so SNP_FEATURES_IMPL_REQ will contain the SAVIC bit (unconditionally,
> without the ifdeffery) and SNP_FEATURES_PRESENT will contain that thing you
> had suggested with the ifdeffery to denote whether SAVIC support has been
> enabled at build time:
> 
> https://lore.kernel.org/r/e0362a96-4b3a-44b1-8d54-806a6b045799@amd.com
> 
> or not.
> 
> Right?
> 

Yes, that is the intent here.


- Neeraj

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Thomas Gleixner]

On Thu, Mar 20 2025 at 16:51, Borislav Petkov wrote:
> On Wed, Feb 26, 2025 at 02:35:09PM +0530, Neeraj Upadhyay wrote:
>> +static int x2apic_savic_probe(void)
>> +{
>> +	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
>> +		return 0;
>> +
>> +	if (!x2apic_mode) {
>> +		pr_err("Secure AVIC enabled in non x2APIC mode\n");
>> +		snp_abort();
>> +	}
>> +
>> +	pr_info("Secure AVIC Enabled\n");
>
> That's not necessary.
>
> Actually, you could figure out why that
>
> 	pr_info("Switched APIC routing to: %s\n", driver->name);
>
> doesn't come out in current kernels anymore:
>
> $ dmesg | grep -i "switched apic"
> $
>
> and fix that as a separate patch.
>
> Looks like it broke in 6.10 or so:
>
> $ grep -E "Switched APIC" *
> 10-rc1+:APIC: Switched APIC routing to: physical flat
> 10-rc6+:APIC: Switched APIC routing to: physical flat

It's very simple. Before that the default driver was logical flat.

838ba7733e4e ("x86/apic: Remove logical destination mode for 64-bit")

Removed logical destination mode and defaulted to physical flat.

So if you box does not switch to something else it keeps the default and
does not print. See the first condition in apic_install_driver().

But that SNP thing will switch and print....

Thanks,

        tglx

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Borislav Petkov]

On Fri, Mar 21, 2025 at 01:44:26PM +0100, Thomas Gleixner wrote:
> So if you box does not switch to something else it keeps the default and
> does not print. See the first condition in apic_install_driver().

Ofc.

> But that SNP thing will switch and print....

Can we pretty-please make that an unconditional pr_info_once() so that I know
what it is?

Even you and I have wondered in the past while debugging something, what APIC
driver the thing selects...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> +static void x2apic_savic_send_IPI(int cpu, int vector)
> +{
> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
> +
> +	/* x2apic MSRs are special and need a special fence: */
> +	weak_wrmsr_fence();
> +	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
> +}
> +
> +static void
> +__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
> +{
> +	unsigned long query_cpu;
> +	unsigned long this_cpu;
> +	unsigned long flags;
> +
> +	/* x2apic MSRs are special and need a special fence: */
> +	weak_wrmsr_fence();
> +
> +	local_irq_save(flags);
> +
> +	this_cpu = smp_processor_id();
> +	for_each_cpu(query_cpu, mask) {
> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
> +			continue;
> +		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
> +				       vector, APIC_DEST_PHYSICAL);
> +	}
> +	local_irq_restore(flags);
> +}
> +
> +static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
> +{
> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
> +}
> +
> +static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
> +{
> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
> +}

The above are identical copies (aside of the names) of the functions in
x2apic_phys.c.

Why can't this simply share the code ?

Thanks,

        tglx

Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 6:23 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> +static void x2apic_savic_send_IPI(int cpu, int vector)
>> +{
>> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
>> +}
>> +
>> +static void
>> +__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
>> +{
>> +	unsigned long query_cpu;
>> +	unsigned long this_cpu;
>> +	unsigned long flags;
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +
>> +	local_irq_save(flags);
>> +
>> +	this_cpu = smp_processor_id();
>> +	for_each_cpu(query_cpu, mask) {
>> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
>> +			continue;
>> +		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
>> +				       vector, APIC_DEST_PHYSICAL);
>> +	}
>> +	local_irq_restore(flags);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>> +}
> 
> The above are identical copies (aside of the names) of the functions in
> x2apic_phys.c.
> 
> Why can't this simply share the code ?
> 

In patch "06/17 x86/apic: Add support to send IPI for Secure AVIC",
__send_IPI_mask() becomes different. It is modified to call functions which do
Secure AVIC specific handling - weak_wrmsr_fence() is not required and instead
of wrmsr, APIC_IRR is updated in the APIC backing page memory. 


- Neeraj

> Thanks,
> 
>         tglx

[RFC v2 02/17] x86/apic: Initialize Secure AVIC APIC backing page

[Neeraj Upadhyay]

With Secure AVIC, the APIC backing page is owned and managed by guest.
Allocate and initialize APIC backing page for all guest CPUs.

The NPT entry for the vCPU's APIC backing page must always be present
when the vCPU is running in order for Secure AVIC to function. A
VMEXIT_BUSY is returned on VMRUN and the vCPU cannot be resumed if
the NPT entry for the APIC backing page is not present. Notify GPA of
the vCPU's APIC backing page to the hypervisor by using the
SVM_VMGEXIT_SECURE_AVIC GHCB protocol event. Before executing VMRUN,
the hypervisor makes use of this information to make sure the APIC backing
page is mapped in NPT.

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 v1:

 - Updated commit log.
 - Allocate APIC backing page for each CPU as a separate PAGE_SIZE
   allocation with GFP_KERNEL flag.
 - Update the GPA registeration API as per the latest GHCB spec updates
   for Secure AVIC GHCB protocol event (yet to be published).
   Corresponding KVM support is here:
   https://github.com/AMDESE/linux-kvm/commit/5fbf231861207edf73bb31742f75e22cae18607b
 - Remove savic_setup_done variable.
 - Removed initialization of LVT* regs in backing page from Hv values.
   These regs will reads/writes will be propagated to Hv in subsequent
   patches.
 - Move savic_ghcb_msr_read() definition to a later patch where it will
   be first used.

 arch/x86/coco/sev/core.c            | 32 +++++++++++++++++++++++++++
 arch/x86/include/asm/apic.h         |  1 +
 arch/x86/include/asm/sev.h          |  3 +++
 arch/x86/include/uapi/asm/svm.h     |  3 +++
 arch/x86/kernel/apic/apic.c         |  2 ++
 arch/x86/kernel/apic/x2apic_savic.c | 34 +++++++++++++++++++++++++++++
 6 files changed, 75 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 82492efc5d94..300bc8f6eb6f 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1504,6 +1504,38 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 	return ret;
 }
 
+/*
+ * Register GPA of the Secure AVIC backing page.
+ *
+ * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
+ *           doing the call.
+ * @gpa    : GPA of the Secure AVIC backing page.
+ */
+enum es_result savic_register_gpa(u64 apic_id, u64 gpa)
+{
+	struct ghcb_state state;
+	struct es_em_ctxt ctxt;
+	unsigned long flags;
+	struct ghcb *ghcb;
+	int ret = 0;
+
+	local_irq_save(flags);
+
+	ghcb = __sev_get_ghcb(&state);
+
+	vc_ghcb_invalidate(ghcb);
+
+	ghcb_set_rax(ghcb, apic_id);
+	ghcb_set_rbx(ghcb, gpa);
+	ret = sev_es_ghcb_hv_call(ghcb, &ctxt, SVM_VMGEXIT_SECURE_AVIC,
+			SVM_VMGEXIT_SECURE_AVIC_REGISTER_GPA, 0);
+
+	__sev_put_ghcb(&state);
+
+	local_irq_restore(flags);
+	return ret;
+}
+
 static void snp_register_per_cpu_ghcb(void)
 {
 	struct sev_es_runtime_data *data;
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index f21ff1932699..3f70aa2f3aba 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -305,6 +305,7 @@ struct apic {
 
 	/* Probe, setup and smpboot functions */
 	int	(*probe)(void);
+	void	(*setup)(void);
 	int	(*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
 
 	void	(*init_apic_ldr)(void);
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 1581246491b5..626588386cf2 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -483,6 +483,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 apic_id, u64 gpa);
 
 #else	/* !CONFIG_AMD_MEM_ENCRYPT */
 
@@ -526,6 +527,8 @@ static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_
 					 struct snp_guest_request_ioctl *rio) { return -ENODEV; }
 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 apic_id,
+						u64 gpa) { return ES_UNSUPPORTED; }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
diff --git a/arch/x86/include/uapi/asm/svm.h b/arch/x86/include/uapi/asm/svm.h
index 1814b413fd57..0bb70c5988bb 100644
--- a/arch/x86/include/uapi/asm/svm.h
+++ b/arch/x86/include/uapi/asm/svm.h
@@ -116,6 +116,9 @@
 #define SVM_VMGEXIT_AP_CREATE			1
 #define SVM_VMGEXIT_AP_DESTROY			2
 #define SVM_VMGEXIT_SNP_RUN_VMPL		0x80000018
+#define SVM_VMGEXIT_SECURE_AVIC			0x8000001a
+#define SVM_VMGEXIT_SECURE_AVIC_REGISTER_GPA	0
+#define SVM_VMGEXIT_SECURE_AVIC_UNREGISTER_GPA	1
 #define SVM_VMGEXIT_HV_FEATURES			0x8000fffd
 #define SVM_VMGEXIT_TERM_REQUEST		0x8000fffe
 #define SVM_VMGEXIT_TERM_REASON(reason_set, reason_code)	\
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index e893dc6f11c1..1c0b5f14435e 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1504,6 +1504,8 @@ static void setup_local_APIC(void)
 		return;
 	}
 
+	if (apic->setup)
+		apic->setup();
 	/*
 	 * If this comes from kexec/kcrash the APIC might be enabled in
 	 * SPIV. Soft disable it before doing further initialization.
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index c3a4d387c63f..c444161d81b3 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -9,12 +9,15 @@
 
 #include <linux/cpumask.h>
 #include <linux/cc_platform.h>
+#include <linux/percpu-defs.h>
 
 #include <asm/apic.h>
 #include <asm/sev.h>
 
 #include "local.h"
 
+static DEFINE_PER_CPU(void *, apic_backing_page);
+
 static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
@@ -61,6 +64,36 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
 	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
 }
 
+static void x2apic_savic_setup(void)
+{
+	void *backing_page;
+	enum es_result ret;
+	unsigned long gpa;
+
+	if (this_cpu_read(apic_backing_page))
+		return;
+
+	backing_page = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!backing_page)
+		snp_abort();
+	this_cpu_write(apic_backing_page, backing_page);
+	gpa = __pa(backing_page);
+
+	/*
+	 * The NPT entry for the vCPU's APIC backing page must always be
+	 * present when the vCPU is running in order for Secure AVIC to
+	 * function. A VMEXIT_BUSY is returned on VMRUN and the vCPU cannot
+	 * be resumed if the NPT entry for the APIC backing page is not
+	 * present. Notify GPA of the vCPU's APIC backing page to the
+	 * hypervisor by calling savic_register_gpa(). Before executing
+	 * VMRUN, the hypervisor makes use of this information to make sure
+	 * the APIC backing page is mapped in NPT.
+	 */
+	ret = savic_register_gpa(-1ULL, gpa);
+	if (ret != ES_OK)
+		snp_abort();
+}
+
 static int x2apic_savic_probe(void)
 {
 	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
@@ -81,6 +114,7 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 	.name				= "secure avic x2apic",
 	.probe				= x2apic_savic_probe,
 	.acpi_madt_oem_check		= x2apic_savic_acpi_madt_oem_check,
+	.setup				= x2apic_savic_setup,
 
 	.dest_mode_logical		= false,
 
-- 
2.34.1

Re: [RFC v2 02/17] x86/apic: Initialize Secure AVIC APIC backing page

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> @@ -1504,6 +1504,8 @@ static void setup_local_APIC(void)
>  		return;
>  	}
>  
> +	if (apic->setup)
> +		apic->setup();

That's broken for AP bringup. This is invoked from ap_starting()
_before_ anything of the CPU is populated. You _CANNOT_ 

> +static void x2apic_savic_setup(void)
> +{
> +	void *backing_page;
> +	enum es_result ret;
> +	unsigned long gpa;
> +
> +	if (this_cpu_read(apic_backing_page))
> +		return;
> +
> +	backing_page = kzalloc(PAGE_SIZE, GFP_KERNEL);

allocate memory at that point. This was clearly never tested with any
debugging enabled. And no GFP_ATOMIC is not the right thing either.

This allocation has to happen on the control CPU before the AP is kicked
into life.

But the right thing to do is:

struct apic_page __percpu *backing_page __ro_after_init;

and do once on the boot CPU:

    backing_page = alloc_percpu(struct apic_page);

I talk more about that struct apic_page in the context of a subsequent
patch.

Thanks,

        tglx

Re: [RFC v2 02/17] x86/apic: Initialize Secure AVIC APIC backing page

[Neeraj Upadhyay]

On 3/21/2025 6:38 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> @@ -1504,6 +1504,8 @@ static void setup_local_APIC(void)
>>  		return;
>>  	}
>>  
>> +	if (apic->setup)
>> +		apic->setup();
> 
> That's broken for AP bringup. This is invoked from ap_starting()
> _before_ anything of the CPU is populated. You _CANNOT_ 
> 
>> +static void x2apic_savic_setup(void)
>> +{
>> +	void *backing_page;
>> +	enum es_result ret;
>> +	unsigned long gpa;
>> +
>> +	if (this_cpu_read(apic_backing_page))
>> +		return;
>> +
>> +	backing_page = kzalloc(PAGE_SIZE, GFP_KERNEL);
> 
> allocate memory at that point. This was clearly never tested with any
> debugging enabled. And no GFP_ATOMIC is not the right thing either.
> 
> This allocation has to happen on the control CPU before the AP is kicked
> into life.
> 

I see. I missed this. I now see warnings with CONFIG_DEBUG_ATOMIC_SLEEP
enabled.

> But the right thing to do is:
> 
> struct apic_page __percpu *backing_page __ro_after_init;
> 
> and do once on the boot CPU:
> 
>     backing_page = alloc_percpu(struct apic_page);
> 

Ok, got it. Thanks for providing the correct way to do it!


- Neeraj

> I talk more about that struct apic_page in the context of a subsequent
> patch.
> 
> Thanks,
> 
>         tglx

Re: [RFC v2 02/17] x86/apic: Initialize Secure AVIC APIC backing page

[Francesco Lavra]

On 2025-02-26 at 9:05, Neeraj Upadhyay wrote:
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 82492efc5d94..300bc8f6eb6f 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1504,6 +1504,38 @@ static enum es_result vc_handle_msr(struct
> ghcb *ghcb, struct es_em_ctxt *ctxt)
>  	return ret;
>  }
>  
> +/*
> + * Register GPA of the Secure AVIC backing page.
> + *
> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
> + *           doing the call.
> + * @gpa    : GPA of the Secure AVIC backing page.
> + */
> +enum es_result savic_register_gpa(u64 apic_id, u64 gpa)
> +{
> +	struct ghcb_state state;
> +	struct es_em_ctxt ctxt;
> +	unsigned long flags;
> +	struct ghcb *ghcb;
> +	int ret = 0;

This should be an enum es_result, and there is no need to zero-
initialize it.
The same applies to savic_unregister_gpa() in patch 14.

Re: [RFC v2 02/17] x86/apic: Initialize Secure AVIC APIC backing page

[Neeraj Upadhyay]

On 3/21/2025 10:02 PM, Francesco Lavra wrote:
> On 2025-02-26 at 9:05, Neeraj Upadhyay wrote:
>> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
>> index 82492efc5d94..300bc8f6eb6f 100644
>> --- a/arch/x86/coco/sev/core.c
>> +++ b/arch/x86/coco/sev/core.c
>> @@ -1504,6 +1504,38 @@ static enum es_result vc_handle_msr(struct
>> ghcb *ghcb, struct es_em_ctxt *ctxt)
>>  	return ret;
>>  }
>>  
>> +/*
>> + * Register GPA of the Secure AVIC backing page.
>> + *
>> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
>> + *           doing the call.
>> + * @gpa    : GPA of the Secure AVIC backing page.
>> + */
>> +enum es_result savic_register_gpa(u64 apic_id, u64 gpa)
>> +{
>> +	struct ghcb_state state;
>> +	struct es_em_ctxt ctxt;
>> +	unsigned long flags;
>> +	struct ghcb *ghcb;
>> +	int ret = 0;
> 
> This should be an enum es_result, and there is no need to zero-
> initialize it.
> The same applies to savic_unregister_gpa() in patch 14.

Ah, yes! Thanks for pointing it out. Will fix.


- Neeraj

[RFC v2 03/17] x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver

[Neeraj Upadhyay]

The x2APIC registers are mapped at an offset within the guest APIC
backing page which is same as their x2APIC MMIO offset. Secure AVIC
adds new registers such as ALLOWED_IRRs (which are at 4-byte offset
within the IRR register offset range) and NMI_REQ to the APIC register
space.

Add read() and write() APIC callback functions to read and write x2APIC
registers directly from the guest APIC backing page.

When Secure AVIC is enabled, rdmsr/wrmsr of APIC registers result in
VC exception (for non-accelerated register accesses). The #VC
exception handler can read/write the x2APIC register in the guest
APIC backing page. Since doing this would increase the latency of
accessing x2APIC registers, instead of doing rdmsr/wrmsr based
accesses and handling apic register reads/writes in VC
VMEXIT_AVIC_NOACCEL error condition, the read() and write()
callbacks of Secure AVIC driver directly read/write APIC register
from/to the guest APIC backing page.

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 v1:

 - APIC_ID reg write is not allowed.
 - Put information about not using #VC exception path for register
   reads/writes as comments.
 - So not read backing page if WARN_ONCE is triggered for misaligned
   reads.
 - Cleanups.

 arch/x86/include/asm/apicdef.h      |   2 +
 arch/x86/kernel/apic/x2apic_savic.c | 120 +++++++++++++++++++++++++++-
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
index 094106b6a538..be39a543fbe5 100644
--- a/arch/x86/include/asm/apicdef.h
+++ b/arch/x86/include/asm/apicdef.h
@@ -135,6 +135,8 @@
 #define		APIC_TDR_DIV_128	0xA
 #define	APIC_EFEAT	0x400
 #define	APIC_ECTRL	0x410
+#define APIC_SEOI	0x420
+#define APIC_IER	0x480
 #define APIC_EILVTn(n)	(0x500 + 0x10 * n)
 #define		APIC_EILVT_NR_AMD_K8	1	/* # of extended interrupts */
 #define		APIC_EILVT_NR_AMD_10H	4
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index c444161d81b3..ba904f241d34 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -10,6 +10,7 @@
 #include <linux/cpumask.h>
 #include <linux/cc_platform.h>
 #include <linux/percpu-defs.h>
+#include <linux/align.h>
 
 #include <asm/apic.h>
 #include <asm/sev.h>
@@ -23,6 +24,121 @@ static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
 }
 
+static inline u32 get_reg(char *page, int reg)
+{
+	return READ_ONCE(*((u32 *)(page + reg)));
+}
+
+static inline void set_reg(char *page, int reg, u32 val)
+{
+	WRITE_ONCE(*((u32 *)(page + reg)), val);
+}
+
+#define SAVIC_ALLOWED_IRR_OFFSET	0x204
+
+static u32 x2apic_savic_read(u32 reg)
+{
+	void *backing_page = this_cpu_read(apic_backing_page);
+
+	/*
+	 * When Secure AVIC is enabled, rdmsr/wrmsr of APIC registers result in
+	 * #VC exception (for non-accelerated register accesses). The #VC
+	 * exception handler can read/write the x2APIC register in the guest
+	 * APIC backing page. Since doing this would increase the latency of
+	 * accessing x2APIC registers, instead of doing rdmsr/wrmsr based
+	 * accesses and handling apic register reads/writes in
+	 * #VC VMEXIT_AVIC_NOACCEL error condition, the read() and write()
+	 * callbacks of Secure AVIC driver directly read/write APIC register
+	 * from/to the guest APIC backing page.
+	 */
+	switch (reg) {
+	case APIC_LVTT:
+	case APIC_TMICT:
+	case APIC_TMCCT:
+	case APIC_TDCR:
+	case APIC_ID:
+	case APIC_LVR:
+	case APIC_TASKPRI:
+	case APIC_ARBPRI:
+	case APIC_PROCPRI:
+	case APIC_LDR:
+	case APIC_SPIV:
+	case APIC_ESR:
+	case APIC_ICR:
+	case APIC_LVTTHMR:
+	case APIC_LVTPC:
+	case APIC_LVT0:
+	case APIC_LVT1:
+	case APIC_LVTERR:
+	case APIC_EFEAT:
+	case APIC_ECTRL:
+	case APIC_SEOI:
+	case APIC_IER:
+	case APIC_EILVTn(0) ... APIC_EILVTn(3):
+		return get_reg(backing_page, reg);
+	case APIC_ISR ... APIC_ISR + 0x70:
+	case APIC_TMR ... APIC_TMR + 0x70:
+		if (WARN_ONCE(!IS_ALIGNED(reg, 16),
+			      "Reg offset 0x%x not aligned at 16 bytes", reg))
+			return 0;
+		return get_reg(backing_page, reg);
+	/* IRR and ALLOWED_IRR offset range */
+	case APIC_IRR ... APIC_IRR + 0x74:
+		/*
+		 * Either aligned at 16 bytes for valid IRR reg offset or a
+		 * valid Secure AVIC ALLOWED_IRR offset.
+		 */
+		if (WARN_ONCE(!(IS_ALIGNED(reg, 16) ||
+				IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)),
+			      "Misaligned IRR/ALLOWED_IRR reg offset 0x%x", reg))
+			return 0;
+		return get_reg(backing_page, reg);
+	default:
+		pr_err("Permission denied: read of Secure AVIC reg offset 0x%x\n", reg);
+		return 0;
+	}
+}
+
+#define SAVIC_NMI_REQ_OFFSET		0x278
+
+static void x2apic_savic_write(u32 reg, u32 data)
+{
+	void *backing_page = this_cpu_read(apic_backing_page);
+
+	switch (reg) {
+	case APIC_LVTT:
+	case APIC_LVT0:
+	case APIC_LVT1:
+	case APIC_TMICT:
+	case APIC_TDCR:
+	case APIC_SELF_IPI:
+	case APIC_TASKPRI:
+	case APIC_EOI:
+	case APIC_SPIV:
+	case SAVIC_NMI_REQ_OFFSET:
+	case APIC_ESR:
+	case APIC_ICR:
+	case APIC_LVTTHMR:
+	case APIC_LVTPC:
+	case APIC_LVTERR:
+	case APIC_ECTRL:
+	case APIC_SEOI:
+	case APIC_IER:
+	case APIC_EILVTn(0) ... APIC_EILVTn(3):
+		set_reg(backing_page, reg, data);
+		break;
+	/* ALLOWED_IRR offsets are writable */
+	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
+		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
+			set_reg(backing_page, reg, data);
+			break;
+		}
+		fallthrough;
+	default:
+		pr_err("Permission denied: write to Secure AVIC reg offset 0x%x\n", reg);
+	}
+}
+
 static void x2apic_savic_send_IPI(int cpu, int vector)
 {
 	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
@@ -136,8 +252,8 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 	.send_IPI_self			= x2apic_send_IPI_self,
 	.nmi_to_offline_cpu		= true,
 
-	.read				= native_apic_msr_read,
-	.write				= native_apic_msr_write,
+	.read				= x2apic_savic_read,
+	.write				= x2apic_savic_write,
 	.eoi				= native_apic_msr_eoi,
 	.icr_read			= native_x2apic_icr_read,
 	.icr_write			= native_x2apic_icr_write,
-- 
2.34.1

Re: [RFC v2 03/17] x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:

> +static inline u32 get_reg(char *page, int reg)
> +{
> +	return READ_ONCE(*((u32 *)(page + reg)));

This type casting is disgusting. First you cast the void pointer of the
per CPU backing page implicitely into a signed character pointer and
then cast that to a u32 pointer. Seriously?

struct apic_page {
	union {
		u32	regs[NR_APIC_REGS];
                u8	bytes[PAGE_SIZE];
	};
};                

and the per CPU allocation of apic_page makes this:

static __always_inline u32 get_reg(unsigned int offset)
{
        return READ_ONCE(this_cpu_ptr(apic_page)->regs[offset >> 2]));
}

which avoids the whole pointer indirection of your backing page construct.

Thanks,

        tglx

Re: [RFC v2 03/17] x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver

[Neeraj Upadhyay]

On 3/21/2025 7:08 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> 
>> +static inline u32 get_reg(char *page, int reg)
>> +{
>> +	return READ_ONCE(*((u32 *)(page + reg)));
> 
> This type casting is disgusting. First you cast the void pointer of the
> per CPU backing page implicitely into a signed character pointer and
> then cast that to a u32 pointer. Seriously?
> 
> struct apic_page {
> 	union {
> 		u32	regs[NR_APIC_REGS];
>                 u8	bytes[PAGE_SIZE];
> 	};
> };                
> 
> and the per CPU allocation of apic_page makes this:
> 
> static __always_inline u32 get_reg(unsigned int offset)
> {
>         return READ_ONCE(this_cpu_ptr(apic_page)->regs[offset >> 2]));
> }
> 
> which avoids the whole pointer indirection of your backing page construct.
> 

Thanks for suggesting this!

- Neeraj

> Thanks,
> 
>         tglx

[RFC v2 04/17] x86/apic: Initialize APIC ID for Secure AVIC

[Neeraj Upadhyay]

Initialize the APIC ID in the Secure AVIC APIC backing page with
the APIC_ID msr value read from Hypervisor. Maintain a hashmap to
check and report same APIC_ID value returned by Hypervisor for two
different vCPUs.

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:

 - Do not read APIC_ID from CPUID. Read APIC_ID from Hv and check for
   duplicates.
 - Add a more user-friendly log message on detecting duplicate APIC
   IDs.

 arch/x86/kernel/apic/x2apic_savic.c | 59 +++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index ba904f241d34..505ef2d29311 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -11,6 +11,8 @@
 #include <linux/cc_platform.h>
 #include <linux/percpu-defs.h>
 #include <linux/align.h>
+#include <linux/sizes.h>
+#include <linux/llist.h>
 
 #include <asm/apic.h>
 #include <asm/sev.h>
@@ -19,6 +21,16 @@
 
 static DEFINE_PER_CPU(void *, apic_backing_page);
 
+struct apic_id_node {
+	 struct llist_node node;
+	 u32 apic_id;
+	 int cpu;
+};
+
+static DEFINE_PER_CPU(struct apic_id_node, apic_id_node);
+
+static struct llist_head *apic_id_map;
+
 static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
@@ -180,6 +192,44 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
 	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
 }
 
+static void init_backing_page(void *backing_page)
+{
+	struct apic_id_node *next_node, *this_cpu_node;
+	unsigned int apic_map_slot;
+	u32 apic_id;
+	int cpu;
+
+	/*
+	 * Before Secure AVIC is enabled, APIC msr reads are
+	 * intercepted. APIC_ID msr read returns the value
+	 * from hv.
+	 */
+	apic_id = native_apic_msr_read(APIC_ID);
+	set_reg(backing_page, APIC_ID, apic_id);
+
+	if (!apic_id_map)
+		return;
+
+	cpu = smp_processor_id();
+	this_cpu_node = &per_cpu(apic_id_node, cpu);
+	this_cpu_node->apic_id = apic_id;
+	this_cpu_node->cpu = cpu;
+	/*
+	 * In common case, apic_ids for CPUs are sequentially numbered.
+	 * So, each CPU should hash to a different slot in the apic id
+	 * map.
+	 */
+	apic_map_slot = apic_id % nr_cpu_ids;
+	llist_add(&this_cpu_node->node, &apic_id_map[apic_map_slot]);
+	/* Each CPU checks only its next nodes for duplicates. */
+	llist_for_each_entry(next_node, this_cpu_node->node.next, node) {
+		if (WARN_ONCE(next_node->apic_id == apic_id,
+			      "Duplicate APIC %u for cpu %d and cpu %d. IPI handling will suffer!",
+			      apic_id, cpu, next_node->cpu))
+			break;
+	}
+}
+
 static void x2apic_savic_setup(void)
 {
 	void *backing_page;
@@ -193,6 +243,7 @@ static void x2apic_savic_setup(void)
 	if (!backing_page)
 		snp_abort();
 	this_cpu_write(apic_backing_page, backing_page);
+	init_backing_page(backing_page);
 	gpa = __pa(backing_page);
 
 	/*
@@ -212,6 +263,8 @@ static void x2apic_savic_setup(void)
 
 static int x2apic_savic_probe(void)
 {
+	int i;
+
 	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
 		return 0;
 
@@ -220,6 +273,12 @@ static int x2apic_savic_probe(void)
 		snp_abort();
 	}
 
+	apic_id_map = kvmalloc(nr_cpu_ids * sizeof(*apic_id_map), GFP_KERNEL);
+
+	if (apic_id_map)
+		for (i = 0; i < nr_cpu_ids; i++)
+			init_llist_head(&apic_id_map[i]);
+
 	pr_info("Secure AVIC Enabled\n");
 
 	return 1;
-- 
2.34.1

Re: [RFC v2 04/17] x86/apic: Initialize APIC ID for Secure AVIC

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> Initialize the APIC ID in the Secure AVIC APIC backing page with
> the APIC_ID msr value read from Hypervisor. Maintain a hashmap to
> check and report same APIC_ID value returned by Hypervisor for two
> different vCPUs.

What for?
 
> +struct apic_id_node {
> +	 struct llist_node node;
> +	 u32 apic_id;
> +	 int cpu;
> +};

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

and please read the rest of the document too.

> +static void init_backing_page(void *backing_page)
> +{
> +	struct apic_id_node *next_node, *this_cpu_node;
> +	unsigned int apic_map_slot;
> +	u32 apic_id;
> +	int cpu;
> +
> +	/*
> +	 * Before Secure AVIC is enabled, APIC msr reads are
> +	 * intercepted. APIC_ID msr read returns the value
> +	 * from hv.

Can you please write things out? i.e. s/hv/hypervisor/ This is not twatter.

> +	 */
> +	apic_id = native_apic_msr_read(APIC_ID);
> +	set_reg(backing_page, APIC_ID, apic_id);
> +
> +	if (!apic_id_map)
> +		return;
> +
> +	cpu = smp_processor_id();
> +	this_cpu_node = &per_cpu(apic_id_node, cpu);
> +	this_cpu_node->apic_id = apic_id;
> +	this_cpu_node->cpu = cpu;
> +	/*
> +	 * In common case, apic_ids for CPUs are sequentially numbered.
> +	 * So, each CPU should hash to a different slot in the apic id
> +	 * map.
> +	 */
> +	apic_map_slot = apic_id % nr_cpu_ids;
> +	llist_add(&this_cpu_node->node, &apic_id_map[apic_map_slot]);

Why does this need to be a llist? What's wrong about a trivial hlist?

> +	/* Each CPU checks only its next nodes for duplicates. */
> +	llist_for_each_entry(next_node, this_cpu_node->node.next, node) {
> +		if (WARN_ONCE(next_node->apic_id == apic_id,
> +			      "Duplicate APIC %u for cpu %d and cpu %d. IPI handling will suffer!",
> +			      apic_id, cpu, next_node->cpu))
> +			break;
> +	}

This does not make any sense at all. The warning is completely useless
because two milliseconds later the topology evaluation code will yell
about mismatch of APIC IDs and catch the duplicate.

So what is this overengineered thing buying you? Just more
incomprehensible security voodoo for no value.

Thanks,

        tglx

Re: [RFC v2 04/17] x86/apic: Initialize APIC ID for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 7:22 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> Initialize the APIC ID in the Secure AVIC APIC backing page with
>> the APIC_ID msr value read from Hypervisor. Maintain a hashmap to
>> check and report same APIC_ID value returned by Hypervisor for two
>> different vCPUs.
> 
> What for?
> 

Guest CPU reads the APIC ID from Hypervisor's vCPU (emulated) APIC state
and updates it in the APIC backing page which is used by Secure AVIC
hardware. As Hypervisor in untrusted, guest does a check for duplicate
APIC IDs. Guest and hypervisor's APIC ID for a vCPU need to match for
IPI flow to work.

IPI flow:

1. Guest source CPU updates the IRR bit in the destination vCPU's APIC backing page.
2. Guest source vCPU takes an exit to hypervisor (icr write msr vmexit). The
   destination APIC ID in ICR data contains guest APIC ID for the destination vCPU.
3. Hypervisor uses the apic id in the icr data provided by guest, to either kick
   the corresponding destination vCPU (if not running) or write to AVIC doorbell
   to notify the running destination vCPU about the new interrupt.


 
>> +struct apic_id_node {
>> +	 struct llist_node node;
>> +	 u32 apic_id;
>> +	 int cpu;
>> +};
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
> 
> and please read the rest of the document too.
> 

Ok.

>> +static void init_backing_page(void *backing_page)
>> +{
>> +	struct apic_id_node *next_node, *this_cpu_node;
>> +	unsigned int apic_map_slot;
>> +	u32 apic_id;
>> +	int cpu;
>> +
>> +	/*
>> +	 * Before Secure AVIC is enabled, APIC msr reads are
>> +	 * intercepted. APIC_ID msr read returns the value
>> +	 * from hv.
> 
> Can you please write things out? i.e. s/hv/hypervisor/ This is not twatter.
> 

Ok.

>> +	 */
>> +	apic_id = native_apic_msr_read(APIC_ID);
>> +	set_reg(backing_page, APIC_ID, apic_id);
>> +
>> +	if (!apic_id_map)
>> +		return;
>> +
>> +	cpu = smp_processor_id();
>> +	this_cpu_node = &per_cpu(apic_id_node, cpu);
>> +	this_cpu_node->apic_id = apic_id;
>> +	this_cpu_node->cpu = cpu;
>> +	/*
>> +	 * In common case, apic_ids for CPUs are sequentially numbered.
>> +	 * So, each CPU should hash to a different slot in the apic id
>> +	 * map.
>> +	 */
>> +	apic_map_slot = apic_id % nr_cpu_ids;
>> +	llist_add(&this_cpu_node->node, &apic_id_map[apic_map_slot]);
> 
> Why does this need to be a llist? What's wrong about a trivial hlist?
> 

I was not sure if the setup() can run in parallel on 2 CPUs. So,
used an atomic list.

>> +	/* Each CPU checks only its next nodes for duplicates. */
>> +	llist_for_each_entry(next_node, this_cpu_node->node.next, node) {
>> +		if (WARN_ONCE(next_node->apic_id == apic_id,
>> +			      "Duplicate APIC %u for cpu %d and cpu %d. IPI handling will suffer!",
>> +			      apic_id, cpu, next_node->cpu))
>> +			break;
>> +	}
> 
> This does not make any sense at all. The warning is completely useless
> because two milliseconds later the topology evaluation code will yell
> about mismatch of APIC IDs and catch the duplicate.
> 
> So what is this overengineered thing buying you? Just more
> incomprehensible security voodoo for no value.
> 

I see. I was not aware of the mismatch check in topology code. I will
remove this code.


- Neeraj

> Thanks,
> 
>         tglx

[RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay]

Add update_vector callback to set/clear ALLOWED_IRR field in
the APIC backing page. The ALLOWED_IRR field indicates the
interrupt vectors which the guest allows the hypervisor to
send (typically for emulated devices). Interrupt vectors used
exclusively by the guest itself (like IPI vectors) should not
be allowed to be injected into the guest for security reasons.
The update_vector callback is invoked from APIC vector domain
whenever a vector is allocated, freed or moved.

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 v1:

 - No change.

 arch/x86/include/asm/apic.h         |  2 ++
 arch/x86/kernel/apic/vector.c       |  8 ++++++++
 arch/x86/kernel/apic/x2apic_savic.c | 21 +++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3f70aa2f3aba..7970ead55f39 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -318,6 +318,8 @@ struct apic {
 	/* wakeup secondary CPU using 64-bit wakeup point */
 	int	(*wakeup_secondary_cpu_64)(u32 apicid, unsigned long start_eip);
 
+	void	(*update_vector)(unsigned int cpu, unsigned int vector, bool set);
+
 	char	*name;
 };
 
diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 72fa4bb78f0a..e0c9505e05f8 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -174,6 +174,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
 		apicd->prev_cpu = apicd->cpu;
 		WARN_ON_ONCE(apicd->cpu == newcpu);
 	} else {
+		if (apic->update_vector)
+			apic->update_vector(apicd->cpu, apicd->vector, false);
 		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
 				managed);
 	}
@@ -183,6 +185,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
 	apicd->cpu = newcpu;
 	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
 	per_cpu(vector_irq, newcpu)[newvec] = desc;
+	if (apic->update_vector)
+		apic->update_vector(apicd->cpu, apicd->vector, true);
 }
 
 static void vector_assign_managed_shutdown(struct irq_data *irqd)
@@ -528,11 +532,15 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
 	if (irqd_is_activated(irqd)) {
 		trace_vector_setup(virq, true, 0);
 		apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
+		if (apic->update_vector)
+			apic->update_vector(apicd->cpu, apicd->vector, true);
 	} else {
 		/* Release the vector */
 		apicd->can_reserve = true;
 		irqd_set_can_reserve(irqd);
 		clear_irq_vector(irqd);
+		if (apic->update_vector)
+			apic->update_vector(apicd->cpu, apicd->vector, false);
 		realloc = true;
 	}
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 505ef2d29311..d912c53dec7a 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -19,6 +19,9 @@
 
 #include "local.h"
 
+#define VEC_POS(v)	((v) & (32 - 1))
+#define REG_POS(v)	(((v) >> 5) << 4)
+
 static DEFINE_PER_CPU(void *, apic_backing_page);
 
 struct apic_id_node {
@@ -192,6 +195,22 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
 	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
 }
 
+static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
+{
+	void *backing_page;
+	unsigned long *reg;
+	int reg_off;
+
+	backing_page = per_cpu(apic_backing_page, cpu);
+	reg_off = SAVIC_ALLOWED_IRR_OFFSET + REG_POS(vector);
+	reg = (unsigned long *)((char *)backing_page + reg_off);
+
+	if (set)
+		test_and_set_bit(VEC_POS(vector), reg);
+	else
+		test_and_clear_bit(VEC_POS(vector), reg);
+}
+
 static void init_backing_page(void *backing_page)
 {
 	struct apic_id_node *next_node, *this_cpu_node;
@@ -316,6 +335,8 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 	.eoi				= native_apic_msr_eoi,
 	.icr_read			= native_x2apic_icr_read,
 	.icr_write			= native_x2apic_icr_write,
+
+	.update_vector			= x2apic_savic_update_vector,
 };
 
 apic_driver(apic_x2apic_savic);
-- 
2.34.1

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> Add update_vector callback to set/clear ALLOWED_IRR field in
> the APIC backing page. The ALLOWED_IRR field indicates the
> interrupt vectors which the guest allows the hypervisor to
> send (typically for emulated devices). Interrupt vectors used
> exclusively by the guest itself (like IPI vectors) should not
> be allowed to be injected into the guest for security reasons.
> The update_vector callback is invoked from APIC vector domain
> whenever a vector is allocated, freed or moved.

Your changelog tells a lot about the WHAT. Please read and follow the
documentation, which describes how a change log should be structured.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 72fa4bb78f0a..e0c9505e05f8 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -174,6 +174,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>  		apicd->prev_cpu = apicd->cpu;
>  		WARN_ON_ONCE(apicd->cpu == newcpu);
>  	} else {
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>  		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
>  				managed);
>  	}
> @@ -183,6 +185,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>  	apicd->cpu = newcpu;
>  	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
>  	per_cpu(vector_irq, newcpu)[newvec] = desc;
> +	if (apic->update_vector)
> +		apic->update_vector(apicd->cpu, apicd->vector, true);

A trivial

static inline void apic_update_vector(....)
{
        if (apic->update_vector)
           ....
}

would be too easy to read and add not enough line count, right?

>  static void vector_assign_managed_shutdown(struct irq_data *irqd)
> @@ -528,11 +532,15 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
>  	if (irqd_is_activated(irqd)) {
>  		trace_vector_setup(virq, true, 0);
>  		apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, true);
>  	} else {
>  		/* Release the vector */
>  		apicd->can_reserve = true;
>  		irqd_set_can_reserve(irqd);
>  		clear_irq_vector(irqd);
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>  		realloc = true;

This is as incomplete as it gets. None of the other code paths which
invoke clear_irq_vector() nor those which invoke free_moved_vector() are
mopping up the leftovers in the backing page.

And no, you don't sprinkle this nonsense all over the call sites. There
is only a very limited number of functions which are involed in setting
up and tearing down a vector. Doing this at the call sites is a
guarantee for missing out as you demonstrated.

> +#define VEC_POS(v)	((v) & (32 - 1))
> +#define REG_POS(v)	(((v) >> 5) << 4)

This is unreadable, undocumented and incomprehensible garbage.

>  static DEFINE_PER_CPU(void *, apic_backing_page);
>  
>  struct apic_id_node {
> @@ -192,6 +195,22 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
>  	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>  }
>  
> +static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
> +{
> +	void *backing_page;
> +	unsigned long *reg;
> +	int reg_off;
> +
> +	backing_page = per_cpu(apic_backing_page, cpu);
> +	reg_off = SAVIC_ALLOWED_IRR_OFFSET + REG_POS(vector);
> +	reg = (unsigned long *)((char *)backing_page + reg_off);
> +
> +	if (set)
> +		test_and_set_bit(VEC_POS(vector), reg);
> +	else
> +		test_and_clear_bit(VEC_POS(vector), reg);
> +}

What's the test_and_ for if you ignore the return value anyway?

Als I have no idea what SAVIC_ALLOWED_IRR_OFFSET means. Whether it's
something from the datashit or a made up thing does not matter. It's
patently non-informative.

Again:

struct apic_page {
	union {
		u32	regs[NR_APIC_REGS];
		u8	bytes[PAGE_SIZE];
	};
};                

       struct apic_page *ap = this_cpu_ptr(apic_page);
       unsigned long *sirr;

       /*
        * apic_page.regs[SAVIC_ALLOWED_IRR_OFFSET...] is an array of
        * consecutive 32-bit registers, which represents a vector bitmap.
        */
        sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR_OFFSET];
        if (set)
        	set_bit(sirr, vector);
        else
        	clear_bit(sirr, vector);

See how code suddenly becomes self explaining, obvious and
comprehensible?

Thanks,

        tglx

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 7:57 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> Add update_vector callback to set/clear ALLOWED_IRR field in
>> the APIC backing page. The ALLOWED_IRR field indicates the
>> interrupt vectors which the guest allows the hypervisor to
>> send (typically for emulated devices). Interrupt vectors used
>> exclusively by the guest itself (like IPI vectors) should not
>> be allowed to be injected into the guest for security reasons.
>> The update_vector callback is invoked from APIC vector domain
>> whenever a vector is allocated, freed or moved.
> 
> Your changelog tells a lot about the WHAT. Please read and follow the
> documentation, which describes how a change log should be structured.
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> 

Ok

>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index 72fa4bb78f0a..e0c9505e05f8 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -174,6 +174,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>>  		apicd->prev_cpu = apicd->cpu;
>>  		WARN_ON_ONCE(apicd->cpu == newcpu);
>>  	} else {
>> +		if (apic->update_vector)
>> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>>  		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
>>  				managed);
>>  	}
>> @@ -183,6 +185,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>>  	apicd->cpu = newcpu;
>>  	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
>>  	per_cpu(vector_irq, newcpu)[newvec] = desc;
>> +	if (apic->update_vector)
>> +		apic->update_vector(apicd->cpu, apicd->vector, true);
> 
> A trivial
> 
> static inline void apic_update_vector(....)
> {
>         if (apic->update_vector)
>            ....
> }
> 
> would be too easy to read and add not enough line count, right?
> 

Yes.

>>  static void vector_assign_managed_shutdown(struct irq_data *irqd)
>> @@ -528,11 +532,15 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
>>  	if (irqd_is_activated(irqd)) {
>>  		trace_vector_setup(virq, true, 0);
>>  		apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
>> +		if (apic->update_vector)
>> +			apic->update_vector(apicd->cpu, apicd->vector, true);
>>  	} else {
>>  		/* Release the vector */
>>  		apicd->can_reserve = true;
>>  		irqd_set_can_reserve(irqd);
>>  		clear_irq_vector(irqd);
>> +		if (apic->update_vector)
>> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>>  		realloc = true;
> 
> This is as incomplete as it gets. None of the other code paths which
> invoke clear_irq_vector() nor those which invoke free_moved_vector() are
> mopping up the leftovers in the backing page.
> 
> And no, you don't sprinkle this nonsense all over the call sites. There
> is only a very limited number of functions which are involed in setting
> up and tearing down a vector. Doing this at the call sites is a
> guarantee for missing out as you demonstrated.
> 

This is the part where I was looking for guidance. As ALLOWED_IRR (which
tells if Hypervisor is allowed to inject a vector to guest vCPU) is per
CPU, intent was to call it at places where vector's CPU affinity changes.
I surely have missed cleaning up ALLOWED_IRR on previously affined CPU.
I will follow your suggestion to do it during setup/teardown of vector (need
to figure out those functions) and configure it for all CPUs in those
functions.

>> +#define VEC_POS(v)	((v) & (32 - 1))
>> +#define REG_POS(v)	(((v) >> 5) << 4)
> 
> This is unreadable, undocumented and incomprehensible garbage.
> 

I will update it.

>>  static DEFINE_PER_CPU(void *, apic_backing_page);
>>  
>>  struct apic_id_node {
>> @@ -192,6 +195,22 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
>>  	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>>  }
>>  
>> +static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
>> +{
>> +	void *backing_page;
>> +	unsigned long *reg;
>> +	int reg_off;
>> +
>> +	backing_page = per_cpu(apic_backing_page, cpu);
>> +	reg_off = SAVIC_ALLOWED_IRR_OFFSET + REG_POS(vector);
>> +	reg = (unsigned long *)((char *)backing_page + reg_off);
>> +
>> +	if (set)
>> +		test_and_set_bit(VEC_POS(vector), reg);
>> +	else
>> +		test_and_clear_bit(VEC_POS(vector), reg);
>> +}
> 
> What's the test_and_ for if you ignore the return value anyway?
>

To not set it again if it already set. I will switch to set_bit/clear_bit()
as test_and_ is not necessary.

 
> Als I have no idea what SAVIC_ALLOWED_IRR_OFFSET means. Whether it's
> something from the datashit or a made up thing does not matter. It's
> patently non-informative.
> 

Ok, I had tried to give some details in the cover letter. These APIC
regs are at offset APIC_IRR(n) + 4 and are used by guest to configure the
interrupt vectors which can be injected by Hypervisor to Guest.


> Again:
> 
> struct apic_page {
> 	union {
> 		u32	regs[NR_APIC_REGS];
> 		u8	bytes[PAGE_SIZE];
> 	};
> };                
> 
>        struct apic_page *ap = this_cpu_ptr(apic_page);
>        unsigned long *sirr;
> 
>        /*
>         * apic_page.regs[SAVIC_ALLOWED_IRR_OFFSET...] is an array of
>         * consecutive 32-bit registers, which represents a vector bitmap.
>         */
>         sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR_OFFSET];
>         if (set)
>         	set_bit(sirr, vector);
>         else
>         	clear_bit(sirr, vector);
> 
> See how code suddenly becomes self explaining, obvious and
> comprehensible?
> 

Yes, thank you!

- Neeraj

> Thanks,
> 
>         tglx

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay]

Hi Thomas,

On 3/21/2025 9:05 PM, Neeraj Upadhyay wrote:
...

> 
>>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>>> index 72fa4bb78f0a..e0c9505e05f8 100644
>>> --- a/arch/x86/kernel/apic/vector.c
>>> +++ b/arch/x86/kernel/apic/vector.c
>>> @@ -174,6 +174,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>>>  		apicd->prev_cpu = apicd->cpu;
>>>  		WARN_ON_ONCE(apicd->cpu == newcpu);
>>>  	} else {
>>> +		if (apic->update_vector)
>>> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>>>  		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
>>>  				managed);
>>>  	}
>>> @@ -183,6 +185,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>>>  	apicd->cpu = newcpu;
>>>  	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
>>>  	per_cpu(vector_irq, newcpu)[newvec] = desc;
>>> +	if (apic->update_vector)
>>> +		apic->update_vector(apicd->cpu, apicd->vector, true);
>>
>> A trivial
>>
>> static inline void apic_update_vector(....)
>> {
>>         if (apic->update_vector)
>>            ....
>> }
>>
>> would be too easy to read and add not enough line count, right?
>>
> 
> Yes.
>

As apic_update_vector() is already defined, I used apic_drv_update_vector().
 
>>>  static void vector_assign_managed_shutdown(struct irq_data *irqd)
>>> @@ -528,11 +532,15 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
>>>  	if (irqd_is_activated(irqd)) {
>>>  		trace_vector_setup(virq, true, 0);
>>>  		apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
>>> +		if (apic->update_vector)
>>> +			apic->update_vector(apicd->cpu, apicd->vector, true);
>>>  	} else {
>>>  		/* Release the vector */
>>>  		apicd->can_reserve = true;
>>>  		irqd_set_can_reserve(irqd);
>>>  		clear_irq_vector(irqd);
>>> +		if (apic->update_vector)
>>> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>>>  		realloc = true;
>>
>> This is as incomplete as it gets. None of the other code paths which
>> invoke clear_irq_vector() nor those which invoke free_moved_vector() are
>> mopping up the leftovers in the backing page.
>>
>> And no, you don't sprinkle this nonsense all over the call sites. There
>> is only a very limited number of functions which are involed in setting
>> up and tearing down a vector. Doing this at the call sites is a
>> guarantee for missing out as you demonstrated.
>>
> 
> This is the part where I was looking for guidance. As ALLOWED_IRR (which
> tells if Hypervisor is allowed to inject a vector to guest vCPU) is per
> CPU, intent was to call it at places where vector's CPU affinity changes.
> I surely have missed cleaning up ALLOWED_IRR on previously affined CPU.
> I will follow your suggestion to do it during setup/teardown of vector (need
> to figure out those functions) and configure it for all CPUs in those
> functions.
> 

I updated it like below. Can you share your opinion on this, if this
looks fine or I got it wrong?

diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
index 736f62812f5c..fef6faffeed1 100644
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -139,6 +139,46 @@ static void apic_update_irq_cfg(struct irq_data *irqd, unsigned int vector,
                            apicd->hw_irq_cfg.dest_apicid);
 }

+static inline void apic_drv_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;
+
+       vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
+
+       if (vector < 0)
+               return vector;
+
+       apic_drv_update_vector(*cpu, vector, true);
+
+       return vector;
+}
+
+static int irq_alloc_managed_vector(unsigned int *cpu)
+{
+       int vector;
+
+       vector = irq_matrix_alloc_managed(vector_matrix, vector_searchmask, cpu);
+
+       if (vector < 0)
+               return vector;
+
+       apic_drv_update_vector(*cpu, vector, true);
+
+       return vector;
+}
+
+static void irq_free_vector(unsigned int cpu, unsigned int vector, bool managed)
+{
+       apic_drv_update_vector(cpu, vector, false);
+       irq_matrix_free(vector_matrix, cpu, vector, managed);
+}
+
 static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
                               unsigned int newcpu)
 {
@@ -174,8 +214,7 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
                apicd->prev_cpu = apicd->cpu;
                WARN_ON_ONCE(apicd->cpu == newcpu);
        } else {
-               irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
-                               managed);
+               irq_free_vector(apicd->cpu, apicd->vector, managed);
        }

 setnew:
@@ -256,7 +295,7 @@ assign_vector_locked(struct irq_data *irqd, const struct cpumask *dest)
        if (apicd->move_in_progress || !hlist_unhashed(&apicd->clist))
                return -EBUSY;

-       vector = irq_matrix_alloc(vector_matrix, dest, resvd, &cpu);
+       vector = irq_alloc_vector(dest, resvd, &cpu);
        trace_vector_alloc(irqd->irq, vector, resvd, vector);
        if (vector < 0)
                return vector;
@@ -332,8 +371,7 @@ assign_managed_vector(struct irq_data *irqd, const struct cpumask *dest)
        /* set_affinity might call here for nothing */
        if (apicd->vector && cpumask_test_cpu(apicd->cpu, vector_searchmask))
                return 0;
-       vector = irq_matrix_alloc_managed(vector_matrix, vector_searchmask,
-                                         &cpu);
+       vector = irq_alloc_managed_vector(&cpu);
        trace_vector_alloc_managed(irqd->irq, vector, vector);
        if (vector < 0)
                return vector;
@@ -357,7 +395,7 @@ static void clear_irq_vector(struct irq_data *irqd)
                           apicd->prev_cpu);

        per_cpu(vector_irq, apicd->cpu)[vector] = VECTOR_SHUTDOWN;
-       irq_matrix_free(vector_matrix, apicd->cpu, vector, managed);
+       irq_free_vector(apicd->cpu, vector, managed);
        apicd->vector = 0;

        /* Clean up move in progress */
@@ -366,7 +404,7 @@ static void clear_irq_vector(struct irq_data *irqd)
                return;

        per_cpu(vector_irq, apicd->prev_cpu)[vector] = VECTOR_SHUTDOWN;
-       irq_matrix_free(vector_matrix, apicd->prev_cpu, vector, managed);
+       irq_free_vector(apicd->prev_cpu, vector, managed);
        apicd->prev_vector = 0;
        apicd->move_in_progress = 0;
        hlist_del_init(&apicd->clist);
@@ -528,6 +566,7 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
        if (irqd_is_activated(irqd)) {
                trace_vector_setup(virq, true, 0);
                apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
+               apic_drv_update_vector(apicd->cpu, apicd->vector, true);
        } else {
                /* Release the vector */
                apicd->can_reserve = true;
@@ -949,7 +988,7 @@ static void free_moved_vector(struct apic_chip_data *apicd)
         *    affinity mask comes online.
         */
        trace_vector_free_moved(apicd->irq, cpu, vector, managed);
-       irq_matrix_free(vector_matrix, cpu, vector, managed);
+       irq_free_vector(cpu, vector, managed);
        per_cpu(vector_irq, cpu)[vector] = VECTOR_UNUSED;
        hlist_del_init(&apicd->clist);
        apicd->prev_vector = 0;


...

>  
>> Als I have no idea what SAVIC_ALLOWED_IRR_OFFSET means. Whether it's
>> something from the datashit or a made up thing does not matter. It's
>> patently non-informative.
>>
> 
> Ok, I had tried to give some details in the cover letter. These APIC
> regs are at offset APIC_IRR(n) + 4 and are used by guest to configure the
> interrupt vectors which can be injected by Hypervisor to Guest.
> 
> 
>> Again:
>>
>> struct apic_page {
>> 	union {
>> 		u32	regs[NR_APIC_REGS];
>> 		u8	bytes[PAGE_SIZE];
>> 	};
>> };                
>>
>>        struct apic_page *ap = this_cpu_ptr(apic_page);
>>        unsigned long *sirr;
>>
>>        /*
>>         * apic_page.regs[SAVIC_ALLOWED_IRR_OFFSET...] is an array of
>>         * consecutive 32-bit registers, which represents a vector bitmap.
>>         */
>>         sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR_OFFSET];
>>         if (set)
>>         	set_bit(sirr, vector);
>>         else
>>         	clear_bit(sirr, vector);
>>
>> See how code suddenly becomes self explaining, obvious and
>> comprehensible?
>>
> 
> Yes, thank you!
> 

After checking more on this, set_bit(vector, ) cannot be used directly  here, as
32-bit registers are not consecutive. Each register is aligned at 16 byte
boundary. So, I changed it to below:

--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -19,6 +19,26 @@

 /* APIC_EILVTn(3) is the last defined APIC register. */
 #define NR_APIC_REGS   (APIC_EILVTn(4) >> 2)
+/*
+ * APIC registers such as APIC_IRR, APIC_ISR, ... are mapped as
+ * 32-bit registers and are aligned at 16-byte boundary. For
+ * example, APIC_IRR registers mapping looks like below:
+ *
+ * #Offset    #bits         Description
+ *  0x200      31:0         vectors 0-31
+ *  0x210      31:0         vectors 32-63
+ *  ...
+ *  0x270      31:0         vectors 224-255
+ *
+ * VEC_BIT_POS gives the bit position of a vector in the APIC
+ * reg containing its state.
+ */
+#define VEC_BIT_POS(v) ((v) & (32 - 1))
+/*
+ * VEC_REG_OFF gives the relative (from the start offset of that APIC
+ * register) offset of the APIC register containing state for a vector.
+ */
+#define VEC_REG_OFF(v) (((v) >> 5) << 4)

 struct apic_page {
        union {
@@ -185,6 +205,35 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
        __send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
 }

+static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
+{
+       struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
+       unsigned long *sirr;
+       int vec_bit;
+       int reg_off;
+
+       /*
+        * ALLOWED_IRR registers are mapped in the apic_page at below byte
+        * offsets. Each register is a 32-bit register aligned at 16-byte
+        * boundary.
+        *
+        * #Offset                    #bits     Description
+        * SAVIC_ALLOWED_IRR_OFFSET   31:0      Guest allowed vectors 0-31
+        * "" + 0x10                  31:0      Guest allowed vectors 32-63
+        * ...
+        * "" + 0x70                  31:0      Guest allowed vectors 224-255
+        *
+        */
+       reg_off = SAVIC_ALLOWED_IRR_OFFSET + VEC_REG_OFF(vector);
+       sirr = (unsigned long *) &ap->regs[reg_off >> 2];
+       vec_bit = VEC_BIT_POS(vector);
+
+       if (set)
+               set_bit(vec_bit, sirr);
+       else
+               clear_bit(vec_bit, sirr);
+}
+
 static void init_backing_page(void *backing_page)
 {
        u32 apic_id;
@@ -272,6 +321,8 @@ static struct apic apic_x2apic_savic __ro_after_init = {
        .eoi                            = native_apic_msr_eoi,
        .icr_read                       = native_x2apic_icr_read,
        .icr_write                      = native_x2apic_icr_write,
+
+       .update_vector                  = x2apic_savic_update_vector,
 };


- Neeraj

> - Neeraj
> 
>> Thanks,
>>
>>         tglx
> 

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Thomas Gleixner]

On Tue, Mar 25 2025 at 17:40, Neeraj Upadhyay wrote:
> On 3/21/2025 9:05 PM, Neeraj Upadhyay wrote:
> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 736f62812f5c..fef6faffeed1 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -139,6 +139,46 @@ static void apic_update_irq_cfg(struct irq_data *irqd, unsigned int vector,
>                             apicd->hw_irq_cfg.dest_apicid);
>  }
>
> +static inline void apic_drv_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;
> +
> +       vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
> +
> +       if (vector < 0)
> +               return vector;
> +
> +       apic_drv_update_vector(*cpu, vector, true);
> +
> +       return vector;
> +}

static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu)
{
	int vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);

	if (vector > 0)
		apic_drv_update_vector(*cpu, vector, true);
        return vector;
}

Perhaps?

> After checking more on this, set_bit(vector, ) cannot be used directly  here, as
> 32-bit registers are not consecutive. Each register is aligned at 16 byte
> boundary.

Fair enough.

> So, I changed it to below:
>
> --- a/arch/x86/kernel/apic/x2apic_savic.c
> +++ b/arch/x86/kernel/apic/x2apic_savic.c
> @@ -19,6 +19,26 @@
>
>  /* APIC_EILVTn(3) is the last defined APIC register. */
>  #define NR_APIC_REGS   (APIC_EILVTn(4) >> 2)
> +/*
> + * APIC registers such as APIC_IRR, APIC_ISR, ... are mapped as
> + * 32-bit registers and are aligned at 16-byte boundary. For
> + * example, APIC_IRR registers mapping looks like below:
> + *
> + * #Offset    #bits         Description
> + *  0x200      31:0         vectors 0-31
> + *  0x210      31:0         vectors 32-63
> + *  ...
> + *  0x270      31:0         vectors 224-255
> + *
> + * VEC_BIT_POS gives the bit position of a vector in the APIC
> + * reg containing its state.
> + */
> +#define VEC_BIT_POS(v) ((v) & (32 - 1))
> +/*
> + * VEC_REG_OFF gives the relative (from the start offset of that APIC
> + * register) offset of the APIC register containing state for a vector.
> + */
> +#define VEC_REG_OFF(v) (((v) >> 5) << 4)
>
>  struct apic_page {
>         union {
> @@ -185,6 +205,35 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
>         __send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>  }
>
> +static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
> +{
> +       struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
> +       unsigned long *sirr;
> +       int vec_bit;
> +       int reg_off;
> +
> +       /*
> +        * ALLOWED_IRR registers are mapped in the apic_page at below byte
> +        * offsets. Each register is a 32-bit register aligned at 16-byte
> +        * boundary.
> +        *
> +        * #Offset                    #bits     Description
> +        * SAVIC_ALLOWED_IRR_OFFSET   31:0      Guest allowed vectors 0-31
> +        * "" + 0x10                  31:0      Guest allowed vectors 32-63
> +        * ...
> +        * "" + 0x70                  31:0      Guest allowed vectors 224-255
> +        *
> +        */
> +       reg_off = SAVIC_ALLOWED_IRR_OFFSET + VEC_REG_OFF(vector);
> +       sirr = (unsigned long *) &ap->regs[reg_off >> 2];
> +       vec_bit = VEC_BIT_POS(vector);
> +
> +       if (set)
> +               set_bit(vec_bit, sirr);
> +       else
> +               clear_bit(vec_bit, sirr);
> +}

If you need 20 lines of horrific comments to explain incomprehensible
macros and code, then something is fundamentally wrong. Then you want to
sit back and think about whether this can't be expressed in simple and
obvious ways. Let's look at the math.

The relevant registers are starting at regs[SAVIC_ALLOWED_IRR]. Due to
the 16-byte alignment the vector number obviously cannot be used for
linear bitmap addressing.

But the resulting bit number can be trivially calculated with:

   bit = vector + 32 * (vector / 32);

which can be converted to:

   bit = vector + (vector & ~0x1f);

That conversion should be done by any reasonable compiler.

Ergo the whole thing can be condensed to:

static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
{
	struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
	unsigned long *sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR];

        /*
         * The registers are 32-bit wide and 16-byte aligned.
         * Compensate for the resulting bit number spacing.
         */
        unsigned int bit = vector + 32 * (vector / 32);

	if (set)
		set_bit(vec_bit, sirr);
	else
		clear_bit(vec_bit, sirr);
}

Two comment lines plus one line of trivial math makes this
comprehensible and obvious. No?

If you need that adjustment for other places as well, then you can
provide a trivial and documented inline function for it.

Thanks,

        tglx

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay]

On 3/27/2025 3:57 PM, Thomas Gleixner wrote:
> On Tue, Mar 25 2025 at 17:40, Neeraj Upadhyay wrote:
>> On 3/21/2025 9:05 PM, Neeraj Upadhyay wrote:
>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index 736f62812f5c..fef6faffeed1 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -139,6 +139,46 @@ static void apic_update_irq_cfg(struct irq_data *irqd, unsigned int vector,
>>                             apicd->hw_irq_cfg.dest_apicid);
>>  }
>>
>> +static inline void apic_drv_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;
>> +
>> +       vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
>> +
>> +       if (vector < 0)
>> +               return vector;
>> +
>> +       apic_drv_update_vector(*cpu, vector, true);
>> +
>> +       return vector;
>> +}
> 
> static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu)
> {
> 	int vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
> 
> 	if (vector > 0)
> 		apic_drv_update_vector(*cpu, vector, true);
>         return vector;
> }
> 
> Perhaps?
> 

Sounds good.

>> After checking more on this, set_bit(vector, ) cannot be used directly  here, as
>> 32-bit registers are not consecutive. Each register is aligned at 16 byte
>> boundary.
> 
> Fair enough.
> 
>> So, I changed it to below:
>>
>> --- a/arch/x86/kernel/apic/x2apic_savic.c
>> +++ b/arch/x86/kernel/apic/x2apic_savic.c
>> @@ -19,6 +19,26 @@
>>
>>  /* APIC_EILVTn(3) is the last defined APIC register. */
>>  #define NR_APIC_REGS   (APIC_EILVTn(4) >> 2)
>> +/*
>> + * APIC registers such as APIC_IRR, APIC_ISR, ... are mapped as
>> + * 32-bit registers and are aligned at 16-byte boundary. For
>> + * example, APIC_IRR registers mapping looks like below:
>> + *
>> + * #Offset    #bits         Description
>> + *  0x200      31:0         vectors 0-31
>> + *  0x210      31:0         vectors 32-63
>> + *  ...
>> + *  0x270      31:0         vectors 224-255
>> + *
>> + * VEC_BIT_POS gives the bit position of a vector in the APIC
>> + * reg containing its state.
>> + */
>> +#define VEC_BIT_POS(v) ((v) & (32 - 1))
>> +/*
>> + * VEC_REG_OFF gives the relative (from the start offset of that APIC
>> + * register) offset of the APIC register containing state for a vector.
>> + */
>> +#define VEC_REG_OFF(v) (((v) >> 5) << 4)
>>
>>  struct apic_page {
>>         union {
>> @@ -185,6 +205,35 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
>>         __send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>>  }
>>
>> +static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
>> +{
>> +       struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
>> +       unsigned long *sirr;
>> +       int vec_bit;
>> +       int reg_off;
>> +
>> +       /*
>> +        * ALLOWED_IRR registers are mapped in the apic_page at below byte
>> +        * offsets. Each register is a 32-bit register aligned at 16-byte
>> +        * boundary.
>> +        *
>> +        * #Offset                    #bits     Description
>> +        * SAVIC_ALLOWED_IRR_OFFSET   31:0      Guest allowed vectors 0-31
>> +        * "" + 0x10                  31:0      Guest allowed vectors 32-63
>> +        * ...
>> +        * "" + 0x70                  31:0      Guest allowed vectors 224-255
>> +        *
>> +        */
>> +       reg_off = SAVIC_ALLOWED_IRR_OFFSET + VEC_REG_OFF(vector);
>> +       sirr = (unsigned long *) &ap->regs[reg_off >> 2];
>> +       vec_bit = VEC_BIT_POS(vector);
>> +
>> +       if (set)
>> +               set_bit(vec_bit, sirr);
>> +       else
>> +               clear_bit(vec_bit, sirr);
>> +}
> 
> If you need 20 lines of horrific comments to explain incomprehensible
> macros and code, then something is fundamentally wrong. Then you want to
> sit back and think about whether this can't be expressed in simple and
> obvious ways. Let's look at the math.
> 

Hmm, indeed. I will keep this in mind, thanks!

> The relevant registers are starting at regs[SAVIC_ALLOWED_IRR]. Due to
> the 16-byte alignment the vector number obviously cannot be used for
> linear bitmap addressing.
> 
> But the resulting bit number can be trivially calculated with:
> 
>    bit = vector + 32 * (vector / 32);
> 

Somehow, this math is not working for me. I will think more on how this
works. From what I understand, bit number is:

bit = vector % 32 +  (vector / 32) * 16 * 8

So, for example, vector number 32, bit number need to be 128.
With you formula, it comes as 64.




- Neeraj

> which can be converted to:
> 
>    bit = vector + (vector & ~0x1f);
> 
> That conversion should be done by any reasonable compiler.
> 
> Ergo the whole thing can be condensed to:
> 
> static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
> {
> 	struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
> 	unsigned long *sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR];
> 
>         /*
>          * The registers are 32-bit wide and 16-byte aligned.
>          * Compensate for the resulting bit number spacing.
>          */
>         unsigned int bit = vector + 32 * (vector / 32);
> 
> 	if (set)
> 		set_bit(vec_bit, sirr);
> 	else
> 		clear_bit(vec_bit, sirr);
> }
> 
> Two comment lines plus one line of trivial math makes this
> comprehensible and obvious. No?
> 
> If you need that adjustment for other places as well, then you can
> provide a trivial and documented inline function for it.
> 
> Thanks,
> 
>         tglx

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Thomas Gleixner]

On Thu, Mar 27 2025 at 16:47, Neeraj Upadhyay wrote:
> On 3/27/2025 3:57 PM, Thomas Gleixner wrote:
>> The relevant registers are starting at regs[SAVIC_ALLOWED_IRR]. Due to
>> the 16-byte alignment the vector number obviously cannot be used for
>> linear bitmap addressing.
>> 
>> But the resulting bit number can be trivially calculated with:
>> 
>>    bit = vector + 32 * (vector / 32);
>> 
>
> Somehow, this math is not working for me. I will think more on how this
> works. From what I understand, bit number is:
>
> bit = vector % 32 +  (vector / 32) * 16 * 8
>
> So, for example, vector number 32, bit number need to be 128.
> With you formula, it comes as 64.

Duh. I did the math for 8 byte alignment. But for 16 byte it's obviously
exactly the same formula just with a different multiplicator:

	bit = vector +  96 * (vector / 32);

No?

Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay]

On 3/27/2025 5:48 PM, Thomas Gleixner wrote:
> On Thu, Mar 27 2025 at 16:47, Neeraj Upadhyay wrote:
>> On 3/27/2025 3:57 PM, Thomas Gleixner wrote:
>>> The relevant registers are starting at regs[SAVIC_ALLOWED_IRR]. Due to
>>> the 16-byte alignment the vector number obviously cannot be used for
>>> linear bitmap addressing.
>>>
>>> But the resulting bit number can be trivially calculated with:
>>>
>>>    bit = vector + 32 * (vector / 32);
>>>
>>
>> Somehow, this math is not working for me. I will think more on how this
>> works. From what I understand, bit number is:
>>
>> bit = vector % 32 +  (vector / 32) * 16 * 8
>>
>> So, for example, vector number 32, bit number need to be 128.
>> With you formula, it comes as 64.
> 
> Duh. I did the math for 8 byte alignment. But for 16 byte it's obviously
> exactly the same formula just with a different multiplicator:
> 
> 	bit = vector +  96 * (vector / 32);
> 
> No?
> 

Ah ok. Got it. Makes sense. 96 * (vector / 32) counts all unused space.


- Neeraj

[RFC v2 06/17] x86/apic: Add support to send IPI for Secure AVIC

[Neeraj Upadhyay]

With Secure AVIC only Self-IPI is accelerated. To handle all the
other IPIs, add new callbacks for sending IPI, which write to the
IRR of the target guest vCPU's APIC backing page and then issue
GHCB protocol MSR write event for the hypervisor to notify the
target vCPU.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Co-developed-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - Remove write_msr_to_hv() and define savic_ghcb_msr_write() in
   sev/core.c.

 arch/x86/coco/sev/core.c            |  40 +++++++-
 arch/x86/include/asm/sev.h          |   2 +
 arch/x86/kernel/apic/x2apic_savic.c | 138 +++++++++++++++++++++++++---
 3 files changed, 162 insertions(+), 18 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 300bc8f6eb6f..4291cdeb5895 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1466,14 +1466,10 @@ static enum es_result __vc_handle_secure_tsc_msrs(struct pt_regs *regs, bool wri
 	return ES_OK;
 }
 
-static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
+static enum es_result __vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt, bool write)
 {
 	struct pt_regs *regs = ctxt->regs;
 	enum es_result ret;
-	bool write;
-
-	/* Is it a WRMSR? */
-	write = ctxt->insn.opcode.bytes[1] == 0x30;
 
 	switch (regs->cx) {
 	case MSR_SVSM_CAA:
@@ -1504,6 +1500,40 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt)
 	return ret;
 }
 
+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);
+}
+
+void savic_ghcb_msr_write(u32 reg, u64 value)
+{
+	u64 msr = APIC_BASE_MSR + (reg >> 4);
+	struct pt_regs regs = {
+		.cx = msr,
+		.ax = lower_32_bits(value),
+		.dx = upper_32_bits(value)
+	};
+	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, true);
+	if (ret != ES_OK) {
+		pr_err("Secure AVIC msr (0x%llx) write returned error (%d)\n", msr, ret);
+		/* MSR writes should never fail. Any failure is fatal error for SNP guest */
+		snp_abort();
+	}
+
+	__sev_put_ghcb(&state);
+	local_irq_restore(flags);
+}
+
 /*
  * Register GPA of the Secure AVIC backing page.
  *
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 626588386cf2..1beeb0daf9e6 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 apic_id, u64 gpa);
+void savic_ghcb_msr_write(u32 reg, u64 value);
 
 #else	/* !CONFIG_AMD_MEM_ENCRYPT */
 
@@ -529,6 +530,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 apic_id,
 						u64 gpa) { return ES_UNSUPPORTED; }
+static void savic_ghcb_msr_write(u32 reg, u64 value) { }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index d912c53dec7a..7e3843154997 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -119,6 +119,7 @@ static u32 x2apic_savic_read(u32 reg)
 static void x2apic_savic_write(u32 reg, u32 data)
 {
 	void *backing_page = this_cpu_read(apic_backing_page);
+	unsigned int cfg;
 
 	switch (reg) {
 	case APIC_LVTT:
@@ -126,7 +127,6 @@ static void x2apic_savic_write(u32 reg, u32 data)
 	case APIC_LVT1:
 	case APIC_TMICT:
 	case APIC_TDCR:
-	case APIC_SELF_IPI:
 	case APIC_TASKPRI:
 	case APIC_EOI:
 	case APIC_SPIV:
@@ -142,6 +142,11 @@ static void x2apic_savic_write(u32 reg, u32 data)
 	case APIC_EILVTn(0) ... APIC_EILVTn(3):
 		set_reg(backing_page, reg, data);
 		break;
+	/* Self IPIs are accelerated by hardware, use wrmsr */
+	case APIC_SELF_IPI:
+		cfg = __prepare_ICR(APIC_DEST_SELF, data, 0);
+		native_x2apic_icr_write(cfg, 0);
+		break;
 	/* ALLOWED_IRR offsets are writable */
 	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
 		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
@@ -154,13 +159,100 @@ static void x2apic_savic_write(u32 reg, u32 data)
 	}
 }
 
+static void send_ipi(int cpu, int vector)
+{
+	void *backing_page;
+	int reg_off;
+
+	backing_page = per_cpu(apic_backing_page, cpu);
+	reg_off = APIC_IRR + REG_POS(vector);
+	/*
+	 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
+	 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
+	 */
+	test_and_set_bit(VEC_POS(vector), (unsigned long *)((char *)backing_page + reg_off));
+}
+
+static void send_ipi_dest(u64 icr_data)
+{
+	int vector, cpu;
+
+	vector = icr_data & APIC_VECTOR_MASK;
+	cpu = icr_data >> 32;
+
+	send_ipi(cpu, vector);
+}
+
+static void send_ipi_target(u64 icr_data)
+{
+	if (icr_data & APIC_DEST_LOGICAL) {
+		pr_err("IPI target should be of PHYSICAL type\n");
+		return;
+	}
+
+	send_ipi_dest(icr_data);
+}
+
+static void send_ipi_allbut(u64 icr_data)
+{
+	const struct cpumask *self_cpu_mask = get_cpu_mask(smp_processor_id());
+	unsigned long flags;
+	int vector, cpu;
+
+	vector = icr_data & APIC_VECTOR_MASK;
+	local_irq_save(flags);
+	for_each_cpu_andnot(cpu, cpu_present_mask, self_cpu_mask)
+		send_ipi(cpu, vector);
+	savic_ghcb_msr_write(APIC_ICR, icr_data);
+	local_irq_restore(flags);
+}
+
+static void send_ipi_allinc(u64 icr_data)
+{
+	int vector;
+
+	send_ipi_allbut(icr_data);
+	vector = icr_data & APIC_VECTOR_MASK;
+	native_x2apic_icr_write(APIC_DEST_SELF | vector, 0);
+}
+
+static void x2apic_savic_icr_write(u32 icr_low, u32 icr_high)
+{
+	int dsh, vector;
+	u64 icr_data;
+
+	icr_data = ((u64)icr_high) << 32 | icr_low;
+	dsh = icr_low & APIC_DEST_ALLBUT;
+
+	switch (dsh) {
+	case APIC_DEST_SELF:
+		vector = icr_data & APIC_VECTOR_MASK;
+		x2apic_savic_write(APIC_SELF_IPI, vector);
+		break;
+	case APIC_DEST_ALLINC:
+		send_ipi_allinc(icr_data);
+		break;
+	case APIC_DEST_ALLBUT:
+		send_ipi_allbut(icr_data);
+		break;
+	default:
+		send_ipi_target(icr_data);
+		savic_ghcb_msr_write(APIC_ICR, icr_data);
+	}
+}
+
+static void __send_IPI_dest(unsigned int apicid, int vector, unsigned int dest)
+{
+	unsigned int cfg = __prepare_ICR(0, vector, dest);
+
+	x2apic_savic_icr_write(cfg, apicid);
+}
+
 static void x2apic_savic_send_IPI(int cpu, int vector)
 {
 	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
 
-	/* x2apic MSRs are special and need a special fence: */
-	weak_wrmsr_fence();
-	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
+	__send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
 }
 
 static void
@@ -170,18 +262,16 @@ __send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
 	unsigned long this_cpu;
 	unsigned long flags;
 
-	/* x2apic MSRs are special and need a special fence: */
-	weak_wrmsr_fence();
-
 	local_irq_save(flags);
 
 	this_cpu = smp_processor_id();
 	for_each_cpu(query_cpu, mask) {
 		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
 			continue;
-		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
-				       vector, APIC_DEST_PHYSICAL);
+		__send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu), vector,
+				      APIC_DEST_PHYSICAL);
 	}
+
 	local_irq_restore(flags);
 }
 
@@ -195,6 +285,28 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
 	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
 }
 
+static void __send_IPI_shorthand(int vector, u32 which)
+{
+	unsigned int cfg = __prepare_ICR(which, vector, 0);
+
+	x2apic_savic_icr_write(cfg, 0);
+}
+
+static void x2apic_savic_send_IPI_allbutself(int vector)
+{
+	__send_IPI_shorthand(vector, APIC_DEST_ALLBUT);
+}
+
+static void x2apic_savic_send_IPI_all(int vector)
+{
+	__send_IPI_shorthand(vector, APIC_DEST_ALLINC);
+}
+
+static void x2apic_savic_send_IPI_self(int vector)
+{
+	__send_IPI_shorthand(vector, APIC_DEST_SELF);
+}
+
 static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
 {
 	void *backing_page;
@@ -325,16 +437,16 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 	.send_IPI			= x2apic_savic_send_IPI,
 	.send_IPI_mask			= x2apic_savic_send_IPI_mask,
 	.send_IPI_mask_allbutself	= x2apic_savic_send_IPI_mask_allbutself,
-	.send_IPI_allbutself		= x2apic_send_IPI_allbutself,
-	.send_IPI_all			= x2apic_send_IPI_all,
-	.send_IPI_self			= x2apic_send_IPI_self,
+	.send_IPI_allbutself		= x2apic_savic_send_IPI_allbutself,
+	.send_IPI_all			= x2apic_savic_send_IPI_all,
+	.send_IPI_self			= x2apic_savic_send_IPI_self,
 	.nmi_to_offline_cpu		= true,
 
 	.read				= x2apic_savic_read,
 	.write				= x2apic_savic_write,
 	.eoi				= native_apic_msr_eoi,
 	.icr_read			= native_x2apic_icr_read,
-	.icr_write			= native_x2apic_icr_write,
+	.icr_write			= x2apic_savic_icr_write,
 
 	.update_vector			= x2apic_savic_update_vector,
 };
-- 
2.34.1

Re: [RFC v2 06/17] x86/apic: Add support to send IPI for Secure AVIC

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> +	/* Self IPIs are accelerated by hardware, use wrmsr */
> +	case APIC_SELF_IPI:
> +		cfg = __prepare_ICR(APIC_DEST_SELF, data, 0);
> +		native_x2apic_icr_write(cfg, 0);
> +		break;

Please move this into a proper inline helper with a understandable
comment and do not hide it in the maze of this write() wrapper.

>  	/* ALLOWED_IRR offsets are writable */
>  	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
>  		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
> @@ -154,13 +159,100 @@ static void x2apic_savic_write(u32 reg, u32 data)
>  	}
>  }
>  
> +static void send_ipi(int cpu, int vector)

Both are unsigned

> +{
> +	void *backing_page;
> +	int reg_off;
> +
> +	backing_page = per_cpu(apic_backing_page, cpu);
> +	reg_off = APIC_IRR + REG_POS(vector);
> +	/*
> +	 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
> +	 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
> +	 */
> +	test_and_set_bit(VEC_POS(vector), (unsigned long *)((char *)backing_page + reg_off));

See previous mail.

> +}
> +
> +static void send_ipi_dest(u64 icr_data)
> +{
> +	int vector, cpu;
> +
> +	vector = icr_data & APIC_VECTOR_MASK;
> +	cpu = icr_data >> 32;

Yes, converting from u64 to int is the proper conversion (NOT)

> +
> +	send_ipi(cpu, vector);
> +}
> +
> +static void send_ipi_target(u64 icr_data)
> +{
> +	if (icr_data & APIC_DEST_LOGICAL) {
> +		pr_err("IPI target should be of PHYSICAL type\n");
> +		return;
> +	}
> +
> +	send_ipi_dest(icr_data);
> +}
> +
> +static void send_ipi_allbut(u64 icr_data)
> +{
> +	const struct cpumask *self_cpu_mask = get_cpu_mask(smp_processor_id());
> +	unsigned long flags;
> +	int vector, cpu;
> +
> +	vector = icr_data & APIC_VECTOR_MASK;
> +	local_irq_save(flags);
> +	for_each_cpu_andnot(cpu, cpu_present_mask, self_cpu_mask)
> +		send_ipi(cpu, vector);
> +	savic_ghcb_msr_write(APIC_ICR, icr_data);
> +	local_irq_restore(flags);
> +}
> +
> +static void send_ipi_allinc(u64 icr_data)
> +{
> +	int vector;
> +
> +	send_ipi_allbut(icr_data);
> +	vector = icr_data & APIC_VECTOR_MASK;
> +	native_x2apic_icr_write(APIC_DEST_SELF | vector, 0);
> +}
> +
> +static void x2apic_savic_icr_write(u32 icr_low, u32 icr_high)
> +{
> +	int dsh, vector;
> +	u64 icr_data;
> +
> +	icr_data = ((u64)icr_high) << 32 | icr_low;
> +	dsh = icr_low & APIC_DEST_ALLBUT;
> +
> +	switch (dsh) {
> +	case APIC_DEST_SELF:
> +		vector = icr_data & APIC_VECTOR_MASK;

So you construct icr_data first and then extract the vector from it,
which is encoded in the low bits of icr_low.

> +		x2apic_savic_write(APIC_SELF_IPI, vector);
> +		break;
> +	case APIC_DEST_ALLINC:
> +		send_ipi_allinc(icr_data);

And you do the same nonsense in all other functions. Oh well...

Thanks,

        tglx

Re: [RFC v2 06/17] x86/apic: Add support to send IPI for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 8:36 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> +	/* Self IPIs are accelerated by hardware, use wrmsr */
>> +	case APIC_SELF_IPI:
>> +		cfg = __prepare_ICR(APIC_DEST_SELF, data, 0);
>> +		native_x2apic_icr_write(cfg, 0);
>> +		break;
> 
> Please move this into a proper inline helper with a understandable
> comment and do not hide it in the maze of this write() wrapper.
> 

Ok.

>>  	/* ALLOWED_IRR offsets are writable */
>>  	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
>>  		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
>> @@ -154,13 +159,100 @@ static void x2apic_savic_write(u32 reg, u32 data)
>>  	}
>>  }
>>  
>> +static void send_ipi(int cpu, int vector)
> 
> Both are unsigned
> 

Will update this.

>> +{
>> +	void *backing_page;
>> +	int reg_off;
>> +
>> +	backing_page = per_cpu(apic_backing_page, cpu);
>> +	reg_off = APIC_IRR + REG_POS(vector);
>> +	/*
>> +	 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
>> +	 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
>> +	 */
>> +	test_and_set_bit(VEC_POS(vector), (unsigned long *)((char *)backing_page + reg_off));
> 
> See previous mail.
> 
>> +}
>> +
>> +static void send_ipi_dest(u64 icr_data)
>> +{
>> +	int vector, cpu;
>> +
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	cpu = icr_data >> 32;
> 
> Yes, converting from u64 to int is the proper conversion (NOT)
> 

Do I need to use unsigned int? I will update this.

>> +
>> +	send_ipi(cpu, vector);
>> +}
>> +
>> +static void send_ipi_target(u64 icr_data)
>> +{
>> +	if (icr_data & APIC_DEST_LOGICAL) {
>> +		pr_err("IPI target should be of PHYSICAL type\n");
>> +		return;
>> +	}
>> +
>> +	send_ipi_dest(icr_data);
>> +}
>> +
>> +static void send_ipi_allbut(u64 icr_data)
>> +{
>> +	const struct cpumask *self_cpu_mask = get_cpu_mask(smp_processor_id());
>> +	unsigned long flags;
>> +	int vector, cpu;
>> +
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	local_irq_save(flags);
>> +	for_each_cpu_andnot(cpu, cpu_present_mask, self_cpu_mask)
>> +		send_ipi(cpu, vector);
>> +	savic_ghcb_msr_write(APIC_ICR, icr_data);
>> +	local_irq_restore(flags);
>> +}
>> +
>> +static void send_ipi_allinc(u64 icr_data)
>> +{
>> +	int vector;
>> +
>> +	send_ipi_allbut(icr_data);
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	native_x2apic_icr_write(APIC_DEST_SELF | vector, 0);
>> +}
>> +
>> +static void x2apic_savic_icr_write(u32 icr_low, u32 icr_high)
>> +{
>> +	int dsh, vector;
>> +	u64 icr_data;
>> +
>> +	icr_data = ((u64)icr_high) << 32 | icr_low;
>> +	dsh = icr_low & APIC_DEST_ALLBUT;
>> +
>> +	switch (dsh) {
>> +	case APIC_DEST_SELF:
>> +		vector = icr_data & APIC_VECTOR_MASK;
> 
> So you construct icr_data first and then extract the vector from it,
> which is encoded in the low bits of icr_low.
> 
>> +		x2apic_savic_write(APIC_SELF_IPI, vector);
>> +		break;
>> +	case APIC_DEST_ALLINC:
>> +		send_ipi_allinc(icr_data);
> 
> And you do the same nonsense in all other functions. Oh well...
> 

I will clean this up in the next version.

- Neeraj

> Thanks,
> 
>         tglx

[RFC v2 07/17] x86/apic: Support LAPIC timer for Secure AVIC

[Neeraj Upadhyay]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Secure AVIC requires LAPIC timer to be emulated by 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, configure APIC_ALLOWED_IRR for the hypervisor to inject
timer interrupt using LOCAL_TIMER_VECTOR.

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

 - Move savic_ghcb_msr_read() definition here.
 - Call update_vector() callback only when it is initialized.

 arch/x86/coco/sev/core.c            | 27 +++++++++++++++++++++++++++
 arch/x86/include/asm/sev.h          |  2 ++
 arch/x86/kernel/apic/apic.c         |  4 ++++
 arch/x86/kernel/apic/x2apic_savic.c |  7 +++++--
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 4291cdeb5895..e4c20023e554 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1505,6 +1505,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/sev.h b/arch/x86/include/asm/sev.h
index 1beeb0daf9e6..043fe8115ec7 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 apic_id, 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 apic_id,
 						u64 gpa) { return ES_UNSUPPORTED; }
+static inline u64 savic_ghcb_msr_read(u32 reg) { return 0; }
 static void savic_ghcb_msr_write(u32 reg, u64 value) { }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 1c0b5f14435e..23a566a82084 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -591,6 +591,10 @@ static void setup_APIC_timer(void)
 						0xF, ~0UL);
 	} else
 		clockevents_register_device(levt);
+
+	if (apic->update_vector)
+		apic->update_vector(smp_processor_id(), LOCAL_TIMER_VECTOR,
+				    true);
 }
 
 /*
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 7e3843154997..af46e1b57017 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -71,6 +71,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:
@@ -123,10 +124,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

[RFC v2 08/17] x86/sev: Initialize VGIF for secondary VCPUs for Secure AVIC

[Neeraj Upadhyay]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Secure AVIC requires VGIF to be configured in VMSA. Configure
for secondary vCPUs (the configuration for boot CPU is done in
hypervisor).

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <neeraj.upadhyay@amd.com>
---
Changes since v1:
 - No change

 arch/x86/coco/sev/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index e4c20023e554..8a4ad392d188 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1272,6 +1272,9 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
 	vmsa->x87_ftw		= AP_INIT_X87_FTW_DEFAULT;
 	vmsa->x87_fcw		= AP_INIT_X87_FCW_DEFAULT;
 
+	if (cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
+		vmsa->vintr_ctrl	|= V_GIF_MASK;
+
 	/* SVME must be set. */
 	vmsa->efer		= EFER_SVME;
 
-- 
2.34.1

[RFC v2 09/17] x86/apic: Add support to send NMI IPI for Secure AVIC

[Neeraj Upadhyay]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Secure AVIC has introduced a new field in the APIC backing page
"NmiReq" that has to be set by the guest to request a NMI IPI.

Add support to set NmiReq appropriately to send NMI IPI.

This also requires Virtual NMI feature to be enabled in VINTRL_CTRL
field in the VMSA. However this would be added by a later commit
after adding support for injecting NMI from the hypervisor.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - Do not set APIC_IRR for NMI IPI.

 arch/x86/kernel/apic/x2apic_savic.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index af46e1b57017..0067fc5c4ef3 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -162,28 +162,34 @@ static void x2apic_savic_write(u32 reg, u32 data)
 	}
 }
 
-static void send_ipi(int cpu, int vector)
+static void send_ipi(int cpu, int vector, bool nmi)
 {
 	void *backing_page;
 	int reg_off;
 
 	backing_page = per_cpu(apic_backing_page, cpu);
 	reg_off = APIC_IRR + REG_POS(vector);
-	/*
-	 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
-	 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
-	 */
-	test_and_set_bit(VEC_POS(vector), (unsigned long *)((char *)backing_page + reg_off));
+	if (!nmi)
+		/*
+		 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
+		 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
+		 * */
+		test_and_set_bit(VEC_POS(vector),
+				 (unsigned long *)((char *)backing_page + reg_off));
+	else
+		set_reg(backing_page, SAVIC_NMI_REQ_OFFSET, nmi);
 }
 
 static void send_ipi_dest(u64 icr_data)
 {
 	int vector, cpu;
+	bool nmi;
 
 	vector = icr_data & APIC_VECTOR_MASK;
 	cpu = icr_data >> 32;
+	nmi = ((icr_data & APIC_DM_FIXED_MASK) == APIC_DM_NMI);
 
-	send_ipi(cpu, vector);
+	send_ipi(cpu, vector, nmi);
 }
 
 static void send_ipi_target(u64 icr_data)
@@ -201,11 +207,13 @@ static void send_ipi_allbut(u64 icr_data)
 	const struct cpumask *self_cpu_mask = get_cpu_mask(smp_processor_id());
 	unsigned long flags;
 	int vector, cpu;
+	bool nmi;
 
 	vector = icr_data & APIC_VECTOR_MASK;
+	nmi = ((icr_data & APIC_DM_FIXED_MASK) == APIC_DM_NMI);
 	local_irq_save(flags);
 	for_each_cpu_andnot(cpu, cpu_present_mask, self_cpu_mask)
-		send_ipi(cpu, vector);
+		send_ipi(cpu, vector, nmi);
 	savic_ghcb_msr_write(APIC_ICR, icr_data);
 	local_irq_restore(flags);
 }
-- 
2.34.1

[RFC v2 10/17] x86/apic: Allow NMI to be injected from hypervisor for Secure AVIC

[Neeraj Upadhyay]

Secure AVIC requires "AllowedNmi" bit in the Secure AVIC Control MSR
to be set for NMI to be injected from hypervisor. Set "AllowedNmi"
bit in Secure AVIC Control MSR to allow NMI interrupts to be injected
from hypervisor.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - No change

 arch/x86/include/asm/msr-index.h    | 5 +++++
 arch/x86/kernel/apic/x2apic_savic.c | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index a42d88e9def8..a2dabde0d50c 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -687,6 +687,11 @@
 #define MSR_AMD64_SNP_SECURE_AVIC 	BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
 #define MSR_AMD64_SNP_RESV_BIT		19
 #define MSR_AMD64_SNP_RESERVED_MASK	GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
+#define MSR_AMD64_SECURE_AVIC_CONTROL	0xc0010138
+#define MSR_AMD64_SECURE_AVIC_EN_BIT	0
+#define MSR_AMD64_SECURE_AVIC_EN	BIT_ULL(MSR_AMD64_SECURE_AVIC_EN_BIT)
+#define MSR_AMD64_SECURE_AVIC_ALLOWEDNMI_BIT 1
+#define MSR_AMD64_SECURE_AVIC_ALLOWEDNMI BIT_ULL(MSR_AMD64_SECURE_AVIC_ALLOWEDNMI_BIT)
 #define MSR_AMD64_RMP_BASE		0xc0010132
 #define MSR_AMD64_RMP_END		0xc0010133
 #define MSR_AMD64_RMP_CFG		0xc0010136
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 0067fc5c4ef3..113d1b07a9e6 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -34,6 +34,11 @@ static DEFINE_PER_CPU(struct apic_id_node, apic_id_node);
 
 static struct llist_head *apic_id_map;
 
+static inline void savic_wr_control_msr(u64 val)
+{
+	native_wrmsr(MSR_AMD64_SECURE_AVIC_CONTROL, lower_32_bits(val), upper_32_bits(val));
+}
+
 static int x2apic_savic_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
 {
 	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_SECURE_AVIC);
@@ -401,6 +406,7 @@ static void x2apic_savic_setup(void)
 	ret = savic_register_gpa(-1ULL, gpa);
 	if (ret != ES_OK)
 		snp_abort();
+	savic_wr_control_msr(gpa | MSR_AMD64_SECURE_AVIC_ALLOWEDNMI);
 }
 
 static int x2apic_savic_probe(void)
-- 
2.34.1

[RFC v2 11/17] x86/sev: Enable NMI support for Secure AVIC

[Neeraj Upadhyay]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Now that support to send NMI IPI and support to inject NMI from
hypervisor has been added, set V_NMI_ENABLE in VINTR_CTRL field of
VMSA to enable NMI.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - No change

 arch/x86/coco/sev/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 8a4ad392d188..248ffd593bc3 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1273,7 +1273,7 @@ static int wakeup_cpu_via_vmgexit(u32 apic_id, unsigned long start_ip)
 	vmsa->x87_fcw		= AP_INIT_X87_FCW_DEFAULT;
 
 	if (cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
-		vmsa->vintr_ctrl	|= V_GIF_MASK;
+		vmsa->vintr_ctrl	|= (V_GIF_MASK | V_NMI_ENABLE_MASK);
 
 	/* SVME must be set. */
 	vmsa->efer		= EFER_SVME;
-- 
2.34.1

[RFC v2 12/17] x86/apic: Read and write LVT* APIC registers from HV for SAVIC guests

[Neeraj Upadhyay]

Hypervisor need information about the current state of LVT registers
for device emulation and NMI. So, forward reads and write of these
registers to the Hypervisor for Secure AVIC guests.

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - New change.

 arch/x86/kernel/apic/x2apic_savic.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 113d1b07a9e6..f6c72518f6ac 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -76,6 +76,11 @@ static u32 x2apic_savic_read(u32 reg)
 	case APIC_TMICT:
 	case APIC_TMCCT:
 	case APIC_TDCR:
+	case APIC_LVTTHMR:
+	case APIC_LVTPC:
+	case APIC_LVT0:
+	case APIC_LVT1:
+	case APIC_LVTERR:
 		return savic_ghcb_msr_read(reg);
 	case APIC_ID:
 	case APIC_LVR:
@@ -86,11 +91,6 @@ static u32 x2apic_savic_read(u32 reg)
 	case APIC_SPIV:
 	case APIC_ESR:
 	case APIC_ICR:
-	case APIC_LVTTHMR:
-	case APIC_LVTPC:
-	case APIC_LVT0:
-	case APIC_LVT1:
-	case APIC_LVTERR:
 	case APIC_EFEAT:
 	case APIC_ECTRL:
 	case APIC_SEOI:
@@ -131,19 +131,19 @@ static void x2apic_savic_write(u32 reg, u32 data)
 	case APIC_LVTT:
 	case APIC_TMICT:
 	case APIC_TDCR:
-		savic_ghcb_msr_write(reg, data);
-		break;
 	case APIC_LVT0:
 	case APIC_LVT1:
+	case APIC_LVTTHMR:
+	case APIC_LVTPC:
+	case APIC_LVTERR:
+		savic_ghcb_msr_write(reg, data);
+		break;
 	case APIC_TASKPRI:
 	case APIC_EOI:
 	case APIC_SPIV:
 	case SAVIC_NMI_REQ_OFFSET:
 	case APIC_ESR:
 	case APIC_ICR:
-	case APIC_LVTTHMR:
-	case APIC_LVTPC:
-	case APIC_LVTERR:
 	case APIC_ECTRL:
 	case APIC_SEOI:
 	case APIC_IER:
-- 
2.34.1

[RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Neeraj Upadhyay]

Secure AVIC accelerates EOI msr writes for edge-triggered interrupts.
For level-triggered interrupts, EOI msr writes trigger #VC exception
with SVM_EXIT_AVIC_UNACCELERATED_ACCESS error code. The #VC handler
would need to trigger a GHCB protocol MSR write event to the Hypervisor.
As #VC handling adds extra overhead, directly do a GHCB protocol based
EOI write from apic->eoi() callback for level-triggered interrupts.
Use wrmsr for edge-triggered interrupts, so that hardware re-evaluates
any pending interrupt which can be delivered to guest vCPU. For level-
triggered interrupts, re-evaluation happens on return from VMGEXIT.

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - New change.

 arch/x86/kernel/apic/x2apic_savic.c | 53 ++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index f6c72518f6ac..1d6f30866b5b 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -432,6 +432,57 @@ static int x2apic_savic_probe(void)
 	return 1;
 }
 
+static int find_highest_isr(void *backing_page)
+{
+	int vec_per_reg = 32;
+	int max_vec = 256;
+	u32 reg;
+	int vec;
+
+	for (vec = max_vec - 32; vec >= 0; vec -= vec_per_reg) {
+		reg = get_reg(backing_page, APIC_ISR + REG_POS(vec));
+		if (reg)
+			return __fls(reg) + vec;
+	}
+
+	return -1;
+}
+
+static void x2apic_savic_eoi(void)
+{
+	void *backing_page;
+	int reg_off;
+	int vec_pos;
+	u32 tmr;
+	int vec;
+
+	backing_page = this_cpu_read(apic_backing_page);
+
+	vec = find_highest_isr(backing_page);
+	if (WARN_ONCE(vec == -1, "EOI write without any active interrupt in APIC_ISR"))
+		return;
+
+	reg_off = REG_POS(vec);
+	vec_pos = VEC_POS(vec);
+	tmr = get_reg(backing_page, APIC_TMR + reg_off);
+	if (tmr & BIT(vec_pos)) {
+		clear_bit(vec_pos, backing_page + APIC_ISR + reg_off);
+		/*
+		 * Propagate the EOI write to hv for level-triggered interrupts.
+		 * Return to guest from GHCB protocol event takes care of
+		 * re-evaluating interrupt state.
+		 */
+		savic_ghcb_msr_write(APIC_EOI, 0);
+	} else {
+		/*
+		 * Hardware clears APIC_ISR and re-evaluates the interrupt state
+		 * to determine if there is any pending interrupt which can be
+		 * delivered to CPU.
+		 */
+		native_apic_msr_eoi();
+	}
+}
+
 static struct apic apic_x2apic_savic __ro_after_init = {
 
 	.name				= "secure avic x2apic",
@@ -461,7 +512,7 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 
 	.read				= x2apic_savic_read,
 	.write				= x2apic_savic_write,
-	.eoi				= native_apic_msr_eoi,
+	.eoi				= x2apic_savic_eoi,
 	.icr_read			= native_x2apic_icr_read,
 	.icr_write			= x2apic_savic_icr_write,
 
-- 
2.34.1

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> +static int find_highest_isr(void *backing_page)
> +{
> +	int vec_per_reg = 32;
> +	int max_vec = 256;
> +	u32 reg;
> +	int vec;
> +
> +	for (vec = max_vec - 32; vec >= 0; vec -= vec_per_reg) {
> +		reg = get_reg(backing_page, APIC_ISR + REG_POS(vec));
> +		if (reg)
> +			return __fls(reg) + vec;
> +	}
> +
> +	return -1;

Congrats. You managed to re-implement find_last_bit() in the most
incomprehesible way.

Thanks,

        tglx

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Sean Christopherson]

On Fri, Mar 21, 2025, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> > +static int find_highest_isr(void *backing_page)
> > +{
> > +	int vec_per_reg = 32;
> > +	int max_vec = 256;
> > +	u32 reg;
> > +	int vec;
> > +
> > +	for (vec = max_vec - 32; vec >= 0; vec -= vec_per_reg) {
> > +		reg = get_reg(backing_page, APIC_ISR + REG_POS(vec));
> > +		if (reg)
> > +			return __fls(reg) + vec;
> > +	}
> > +
> > +	return -1;
> 
> Congrats. You managed to re-implement find_last_bit() in the most
> incomprehesible way.

Heh, having burned myself quite badly by trying to use find_last_bit() to get
pending/in-service IRQs in KVM code...

Using find_last_bit() doesn't work because the ISR chunks aren't contiguous,
they're 4-byte registers at 16-byte strides.

That said, copy+pasting the aforementioned KVM code is absurd.  Please extract
KVM's find_highest_vector() to common code, along with any other APIC utilities
in KVM that would be useful.  I haven't looked at this series, but I suspect
there's a _lot_ of code in KVM's local APIC emulation that can be shared.

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Thomas Gleixner]

On Fri, Mar 21 2025 at 10:11, Sean Christopherson wrote:
> On Fri, Mar 21, 2025, Thomas Gleixner wrote:
>> 
>> Congrats. You managed to re-implement find_last_bit() in the most
>> incomprehesible way.
>
> Heh, having burned myself quite badly by trying to use find_last_bit() to get
> pending/in-service IRQs in KVM code...
>
> Using find_last_bit() doesn't work because the ISR chunks aren't contiguous,
> they're 4-byte registers at 16-byte strides.

Which is obvious to solve with trivial integer math:

      bit = vector + 32 * (vector / 32);

ergo

     vector = bit - 16 * (bit / 32);

No?

> That said, copy+pasting the aforementioned KVM code is absurd.  Please extract
> KVM's find_highest_vector() to common code.

No. Reimplement this with find_last_bit() and the trivial adjustment
above and remove find_highest_vector() as there is no point to
proliferate a bad reimplementation of find_last_bit().

Thanks,

        tglx

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Thomas Gleixner]

On Thu, Mar 27 2025 at 11:48, Thomas Gleixner wrote:

> On Fri, Mar 21 2025 at 10:11, Sean Christopherson wrote:
>> On Fri, Mar 21, 2025, Thomas Gleixner wrote:
>>> 
>>> Congrats. You managed to re-implement find_last_bit() in the most
>>> incomprehesible way.
>>
>> Heh, having burned myself quite badly by trying to use find_last_bit() to get
>> pending/in-service IRQs in KVM code...
>>
>> Using find_last_bit() doesn't work because the ISR chunks aren't contiguous,
>> they're 4-byte registers at 16-byte strides.
>
> Which is obvious to solve with trivial integer math:
>
>       bit = vector + 32 * (vector / 32);
>
> ergo
>
>      vector = bit - 16 * (bit / 32);
>
> No?

Actually no. As this is for 8 byte alignment. For 16 byte it's 

	bit = vector + 96 * (vector / 32);
ergo
        vector = bit - 24 * (bit / 32);

Which is still just shifts and add/sub.

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Sean Christopherson]

On Thu, Mar 27, 2025, Thomas Gleixner wrote:
> On Thu, Mar 27 2025 at 11:48, Thomas Gleixner wrote:
> 
> > On Fri, Mar 21 2025 at 10:11, Sean Christopherson wrote:
> >> On Fri, Mar 21, 2025, Thomas Gleixner wrote:
> >>> 
> >>> Congrats. You managed to re-implement find_last_bit() in the most
> >>> incomprehesible way.
> >>
> >> Heh, having burned myself quite badly by trying to use find_last_bit() to get
> >> pending/in-service IRQs in KVM code...
> >>
> >> Using find_last_bit() doesn't work because the ISR chunks aren't contiguous,
> >> they're 4-byte registers at 16-byte strides.
> >
> > Which is obvious to solve with trivial integer math:
> >
> >       bit = vector + 32 * (vector / 32);
> >
> > ergo
> >
> >      vector = bit - 16 * (bit / 32);
> >
> > No?
> 
> Actually no. As this is for 8 byte alignment. For 16 byte it's 
> 
> 	bit = vector + 96 * (vector / 32);
> ergo
>         vector = bit - 24 * (bit / 32);
> 
> Which is still just shifts and add/sub.

IIUC, the suggestion is to use find_last_bit() to walk the entire 128-byte range
covered by ISR registers, under the assumption that the holes are guaranteed to
be zero.  I suppose that works for Secure AVIC, but I don't want to do that for
KVM since KVM can't guarantee the holes are zero (userspace can stuff APIC state).

Re: [RFC v2 13/17] x86/apic: Handle EOI writes for SAVIC guests

[Thomas Gleixner]

On Thu, Mar 27 2025 at 07:19, Sean Christopherson wrote:
> On Thu, Mar 27, 2025, Thomas Gleixner wrote:
>> Actually no. As this is for 8 byte alignment. For 16 byte it's 
>> 
>> 	bit = vector + 96 * (vector / 32);
>> ergo
>>         vector = bit - 24 * (bit / 32);
>> 
>> Which is still just shifts and add/sub.
>
> IIUC, the suggestion is to use find_last_bit() to walk the entire 128-byte range
> covered by ISR registers, under the assumption that the holes are guaranteed to
> be zero.  I suppose that works for Secure AVIC, but I don't want to do that for
> KVM since KVM can't guarantee the holes are zero (userspace can stuff APIC state).

Fair enough. So yes, then making the current KVM function generic is the
right thing to do.

Thanks,

        tglx

[RFC v2 14/17] x86/apic: Add kexec support for Secure AVIC

[Neeraj Upadhyay]

Add a ->teardown callback to disable Secure AVIC before
rebooting into the new kernel. This ensures that the new
kernel does not access the old APIC backing page which was
allocated by the previous kernel. Such accesses can happen
if there are any APIC accesses done during guest boot before
Secure AVIC driver probe is done by the new kernel (as
Secure AVIC remains enabled in control msr).

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - New change.

 arch/x86/coco/sev/core.c            | 34 +++++++++++++++++++++++++++++
 arch/x86/include/asm/apic.h         |  1 +
 arch/x86/include/asm/sev.h          |  3 +++
 arch/x86/kernel/apic/apic.c         |  3 +++
 arch/x86/kernel/apic/x2apic_savic.c |  8 +++++++
 5 files changed, 49 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 248ffd593bc3..e48834d29518 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1596,6 +1596,40 @@ enum es_result savic_register_gpa(u64 apic_id, u64 gpa)
 	return ret;
 }
 
+/*
+ * Unregister GPA of the Secure AVIC backing page.
+ *
+ * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
+ *           doing the call.
+ *
+ * On success, returns previously registered GPA of the Secure AVIC
+ * backing page in gpa arg.
+ */
+enum es_result savic_unregister_gpa(u64 apic_id, u64 *gpa)
+{
+	struct ghcb_state state;
+	struct es_em_ctxt ctxt;
+	unsigned long flags;
+	struct ghcb *ghcb;
+	int ret = 0;
+
+	local_irq_save(flags);
+
+	ghcb = __sev_get_ghcb(&state);
+
+	vc_ghcb_invalidate(ghcb);
+
+	ghcb_set_rax(ghcb, apic_id);
+	ret = sev_es_ghcb_hv_call(ghcb, &ctxt, SVM_VMGEXIT_SECURE_AVIC,
+			SVM_VMGEXIT_SECURE_AVIC_UNREGISTER_GPA, 0);
+	if (gpa && ret == ES_OK)
+		*gpa = ghcb->save.rbx;
+	__sev_put_ghcb(&state);
+
+	local_irq_restore(flags);
+	return ret;
+}
+
 static void snp_register_per_cpu_ghcb(void)
 {
 	struct sev_es_runtime_data *data;
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 7970ead55f39..3f129c66529e 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -306,6 +306,7 @@ struct apic {
 	/* Probe, setup and smpboot functions */
 	int	(*probe)(void);
 	void	(*setup)(void);
+	void	(*teardown)(void);
 	int	(*acpi_madt_oem_check)(char *oem_id, char *oem_table_id);
 
 	void	(*init_apic_ldr)(void);
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 043fe8115ec7..e2b1d96b54e6 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 apic_id, u64 gpa);
+enum es_result savic_unregister_gpa(u64 apic_id, u64 *gpa);
 u64 savic_ghcb_msr_read(u32 reg);
 void savic_ghcb_msr_write(u32 reg, u64 value);
 
@@ -531,6 +532,8 @@ 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 apic_id,
 						u64 gpa) { return ES_UNSUPPORTED; }
+static inline enum es_result savic_unregister_gpa(u64 apic_id,
+						  u64 *gpa) { return ES_UNSUPPORTED; }
 static inline u64 savic_ghcb_msr_read(u32 reg) { return 0; }
 static void savic_ghcb_msr_write(u32 reg, u64 value) { }
 
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 23a566a82084..feb2671d1e46 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1171,6 +1171,9 @@ void disable_local_APIC(void)
 	if (!apic_accessible())
 		return;
 
+	if (apic->teardown)
+		apic->teardown();
+
 	apic_soft_disable();
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 1d6f30866b5b..6290b9b1144e 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -377,6 +377,13 @@ static void init_backing_page(void *backing_page)
 	}
 }
 
+static void x2apic_savic_teardown(void)
+{
+	/* Disable Secure AVIC */
+	native_wrmsr(MSR_AMD64_SECURE_AVIC_CONTROL, 0, 0);
+	savic_unregister_gpa(-1ULL, NULL);
+}
+
 static void x2apic_savic_setup(void)
 {
 	void *backing_page;
@@ -489,6 +496,7 @@ static struct apic apic_x2apic_savic __ro_after_init = {
 	.probe				= x2apic_savic_probe,
 	.acpi_madt_oem_check		= x2apic_savic_acpi_madt_oem_check,
 	.setup				= x2apic_savic_setup,
+	.teardown			= x2apic_savic_teardown,
 
 	.dest_mode_logical		= false,
 
-- 
2.34.1

Re: [RFC v2 14/17] x86/apic: Add kexec support for Secure AVIC

[Thomas Gleixner]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>  
> +/*
> + * Unregister GPA of the Secure AVIC backing page.
> + *
> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU

Yes, -1ULL is really a sensible value - NOT. Ever thought about
signed/unsigned?

> + *           doing the call.

How would this function ever make sense to be invoked from a remote CPU?

> + * On success, returns previously registered GPA of the Secure AVIC
> + * backing page in gpa arg.

Please use proper kernel-doc formatting and not some made up thing which
looks like it.

Thanks,

        tglx

Re: [RFC v2 14/17] x86/apic: Add kexec support for Secure AVIC

[Neeraj Upadhyay]

On 3/21/2025 9:18 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>>  
>> +/*
>> + * Unregister GPA of the Secure AVIC backing page.
>> + *
>> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
> 
> Yes, -1ULL is really a sensible value - NOT. Ever thought about
> signed/unsigned?
> 

In table "Table 7: List of Supported Non-Automatic Events" of GHCB spec [1],
0xffff_ffff_ffff_ffff is used for Secure AVIC GHCB event

"RAX will have the APIC ID of the target vCPU or 0xffff_ffff_ffff_ffff
 for the vCPU doing the call"

I am using -1ULL for that here.



[1] https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf


>> + *           doing the call.
> 
> How would this function ever make sense to be invoked from a remote CPU?
> 

I will update the interface in next version. Remote CPU interface is not used.

>> + * On success, returns previously registered GPA of the Secure AVIC
>> + * backing page in gpa arg.
> 
> Please use proper kernel-doc formatting and not some made up thing which
> looks like it.
> 

Ok. I will update this.


- Neeraj

> Thanks,
> 
>         tglx

Re: [RFC v2 14/17] x86/apic: Add kexec support for Secure AVIC

[Thomas Gleixner]

On Tue, Apr 01 2025 at 16:05, Neeraj Upadhyay wrote:

> On 3/21/2025 9:18 PM, Thomas Gleixner wrote:
>> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>>>  
>>> +/*
>>> + * Unregister GPA of the Secure AVIC backing page.
>>> + *
>>> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
>> 
>> Yes, -1ULL is really a sensible value - NOT. Ever thought about
>> signed/unsigned?
> 
>
> In table "Table 7: List of Supported Non-Automatic Events" of GHCB spec [1],
> 0xffff_ffff_ffff_ffff is used for Secure AVIC GHCB event
>
> "RAX will have the APIC ID of the target vCPU or 0xffff_ffff_ffff_ffff
>  for the vCPU doing the call"
>
> I am using -1ULL for that here.

Which is a horrible construct, while  ~0ULL is not.

Re: [RFC v2 14/17] x86/apic: Add kexec support for Secure AVIC

[Neeraj Upadhyay]

On 4/2/2025 12:01 AM, Thomas Gleixner wrote:
> On Tue, Apr 01 2025 at 16:05, Neeraj Upadhyay wrote:
> 
>> On 3/21/2025 9:18 PM, Thomas Gleixner wrote:
>>> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>>>>  
>>>> +/*
>>>> + * Unregister GPA of the Secure AVIC backing page.
>>>> + *
>>>> + * @apic_id: APIC ID of the vCPU. Use -1ULL for the current vCPU
>>>
>>> Yes, -1ULL is really a sensible value - NOT. Ever thought about
>>> signed/unsigned?
>>
>>
>> In table "Table 7: List of Supported Non-Automatic Events" of GHCB spec [1],
>> 0xffff_ffff_ffff_ffff is used for Secure AVIC GHCB event
>>
>> "RAX will have the APIC ID of the target vCPU or 0xffff_ffff_ffff_ffff
>>  for the vCPU doing the call"
>>
>> I am using -1ULL for that here.
> 
> Which is a horrible construct, while  ~0ULL is not.

Got it. Will replace with ~0ULL.


- Neeraj

[RFC v2 15/17] x86/apic: Enable Secure AVIC in Control MSR

[Neeraj Upadhyay]

With all the pieces in place now, enable Secure AVIC in Secure
AVIC Control MSR. Any access to x2APIC MSRs are emulated by
hypervisor before Secure AVIC is enabled in the Control MSR.
Post Secure AVIC enablement, all x2APIC MSR accesses (whether
accelerated by AVIC hardware or trapped as #VC exception) operate
on guest APIC backing page.

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - No change.

 arch/x86/kernel/apic/x2apic_savic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
index 6290b9b1144e..2f3482fdc117 100644
--- a/arch/x86/kernel/apic/x2apic_savic.c
+++ b/arch/x86/kernel/apic/x2apic_savic.c
@@ -413,7 +413,7 @@ static void x2apic_savic_setup(void)
 	ret = savic_register_gpa(-1ULL, gpa);
 	if (ret != ES_OK)
 		snp_abort();
-	savic_wr_control_msr(gpa | MSR_AMD64_SECURE_AVIC_ALLOWEDNMI);
+	savic_wr_control_msr(gpa | MSR_AMD64_SECURE_AVIC_EN | MSR_AMD64_SECURE_AVIC_ALLOWEDNMI);
 }
 
 static int x2apic_savic_probe(void)
-- 
2.34.1

[RFC v2 16/17] x86/sev: Prevent SECURE_AVIC_CONTROL MSR interception for Secure AVIC guests

[Neeraj Upadhyay]

The SECURE_AVIC_CONTROL MSR (0xc0010138) holds the GPA of the guest
APIC  backing page and bitfields to enable Secure AVIC and NMI.
This MSR is  populated by the guest and the hypervisor should not
intercept it. A #VC exception will be generated otherwise. If
this should occur and Secure AVIC is enabled, terminate guest
execution.

Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - New change.

 arch/x86/coco/sev/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index e48834d29518..0372779dae70 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -1483,6 +1483,15 @@ static enum es_result __vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt
 			return __vc_handle_secure_tsc_msrs(regs, write);
 		else
 			break;
+	case MSR_AMD64_SECURE_AVIC_CONTROL:
+		/*
+		 * AMD64_SECURE_AVIC_CONTROL should not be intercepted when
+		 * Secure AVIC is enabled. Terminate the Secure AVIC guest
+		 * if the interception is enabled.
+		 */
+		if (cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
+			return ES_VMM_ERROR;
+		fallthrough;
 	default:
 		break;
 	}
-- 
2.34.1

[RFC v2 17/17] x86/sev: Indicate SEV-SNP guest supports Secure AVIC

[Neeraj Upadhyay]

From: Kishon Vijay Abraham I <kvijayab@amd.com>

Now that Secure AVIC support is added in the guest, indicate SEV-SNP
guest supports Secure AVIC.

Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com>
Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>
---
Changes since v1:
 - No change.

 arch/x86/boot/compressed/sev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 798fdd3dbd1e..385063ceb89c 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -403,7 +403,8 @@ void do_boot_stage2_vc(struct pt_regs *regs, unsigned long exit_code)
  * guest kernel, a corresponding bit should be added to the mask.
  */
 #define SNP_FEATURES_PRESENT	(MSR_AMD64_SNP_DEBUG_SWAP |	\
-				 MSR_AMD64_SNP_SECURE_TSC)
+				 MSR_AMD64_SNP_SECURE_TSC |	\
+				 MSR_AMD64_SNP_SECURE_AVIC)
 
 u64 snp_get_unsupported_features(u64 status)
 {
-- 
2.34.1