From: Kishon Vijay Abraham I <kvijayab@amd.com>
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 malicious 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.
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>
---
arch/x86/Kconfig | 12 +++
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, 140 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/kernel/apic/x2apic_savic.c
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 007bab9f2a0e..b05b4e9d2e49 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -469,6 +469,18 @@ 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 && AMD_MEM_ENCRYPT
+ 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.
+
+ 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
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index cd44e120fe53..ec038be0a048 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_ENABLED | \
MSR_AMD64_SNP_RESERVED_MASK)
/*
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 0f81f70aca82..4c3bc031e9a9 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -100,6 +100,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_ENABLED;
+
default:
return false;
}
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 82c6a4d350e0..d0583619c978 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -658,7 +658,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_ENABLED 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_VIRT_SPEC_CTRL 0xc001011f
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..97dac09a7f42
--- /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: Kishon Vijay Abraham I <kvijayab@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 caa4b4430634..801208678450 100644
--- a/include/linux/cc_platform.h
+++ b/include/linux/cc_platform.h
@@ -88,6 +88,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.1Hi Neeraj, On 9/13/2024 4:36 AM, Neeraj Upadhyay wrote: > From: Kishon Vijay Abraham I <kvijayab@amd.com> > > 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 malicious 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. > > 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> > --- > arch/x86/Kconfig | 12 +++ > 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, 140 insertions(+), 1 deletion(-) > create mode 100644 arch/x86/kernel/apic/x2apic_savic.c > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 007bab9f2a0e..b05b4e9d2e49 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -469,6 +469,18 @@ 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 && AMD_MEM_ENCRYPT If we remove the dependency on X2APIC, there are only 3 X2APIC functions which you call from this driver. Can we just expose them in the header, and then simply remove the dependency on X2APIC? Thanks Melody
On 11/19/2024 3:15 AM, Melody (Huibo) Wang wrote: > Hi Neeraj, > > On 9/13/2024 4:36 AM, Neeraj Upadhyay wrote: >> From: Kishon Vijay Abraham I <kvijayab@amd.com> >> >> 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 malicious 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. >> >> 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> >> --- >> arch/x86/Kconfig | 12 +++ >> 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, 140 insertions(+), 1 deletion(-) >> create mode 100644 arch/x86/kernel/apic/x2apic_savic.c >> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index 007bab9f2a0e..b05b4e9d2e49 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -469,6 +469,18 @@ 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 && AMD_MEM_ENCRYPT > > If we remove the dependency on X2APIC, there are only 3 X2APIC functions which you call from this driver. Can we just expose them in the header, and then simply remove the dependency on X2APIC? > APIC common code (arch/x86/kernel/apic/apic.c) and other parts of the x86 code use X86_X2APIC config to enable x2apic related initialization and functionality. So, dependency on X2APIC need to be there. - Neeraj > Thanks > Melody >
On Thu, Nov 21, 2024 at 10:35:35AM +0530, Neeraj Upadhyay wrote:
> APIC common code (arch/x86/kernel/apic/apic.c) and other parts of the
> x86 code use X86_X2APIC config to enable x2apic related initialization
> and functionality. So, dependency on X2APIC need to be there.
Have you actually tried to remove the dependency and see how it looks?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/21/2024 11:11 AM, Borislav Petkov wrote:
> On Thu, Nov 21, 2024 at 10:35:35AM +0530, Neeraj Upadhyay wrote:
>> APIC common code (arch/x86/kernel/apic/apic.c) and other parts of the
>> x86 code use X86_X2APIC config to enable x2apic related initialization
>> and functionality. So, dependency on X2APIC need to be there.
>
> Have you actually tried to remove the dependency and see how it looks?
>
No, I didn't try it previously, as based on checking the code below
is what I understand how the code is layered:
- Common x2APIC code in arch/x86/... initializes the x2APIC
architecture sequence and other parts of common x2apic initialization:
* Disable and enable x2apic (...kernel/apic/apic.c).
* max_apicid setting in (...kernel/apic/init.c)
* acpi_parse_x2apic() registration of APIC ID in early
topo maps (kernel/acpi/boot.c)
* Enable x2apic in startup code (...kernel/head_64.S).
- Each x2apic driver in arch/x86/kernel/apic provides callbacks for implementation
specific (x2apic_uv_x.c, apic_numachip.c) or a particular mode
specific (x2apic_phys.c, x2apic_cluster.c) functionality.
As SAVIC's guest APIC register accesses match x2avic (which uses x2APIC MSR
interface in guest), the x2apic common flow need to be executed in the
guest.
- Neeraj
> Thx.
>
On Thu, Nov 21, 2024 at 01:33:29PM +0530, Neeraj Upadhyay wrote:
> As SAVIC's guest APIC register accesses match x2avic (which uses x2APIC MSR
> interface in guest), the x2apic common flow need to be executed in the
> guest.
How much of that "common flow" is actually needed by SAVIC?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/21/2024 4:23 PM, Borislav Petkov wrote:
> On Thu, Nov 21, 2024 at 01:33:29PM +0530, Neeraj Upadhyay wrote:
>> As SAVIC's guest APIC register accesses match x2avic (which uses x2APIC MSR
>> interface in guest), the x2apic common flow need to be executed in the
>> guest.
>
> How much of that "common flow" is actually needed by SAVIC?
>
I see most of that flow required. By removing dependency on CONFIG_X86_X2APIC
and enabling SAVIC, I see below boot issues:
- Crash in register_lapic_address() in below path:
register_lapic_address+0x82/0xe0
early_acpi_boot_init+0xc7/0x160
setup_arch+0x9b2/0xec0
The issue happens as register_lapic_address() tries to setup APIC MMIO,
which applies to XAPIC and not to X2APIC. As SAVIC only supports X2APIC
msr interface, APIC MMIO setup fails.
void __init register_lapic_address(unsigned long address)
{
/* This should only happen once */
WARN_ON_ONCE(mp_lapic_addr);
mp_lapic_addr = address;
if (!x2apic_mode)
apic_set_fixmap(true);
}
- x2apic_enable() (which enables X2APIC in APIC base reg) not being called causes
read_msr_from_hv() to return below error:
Secure AVIC msr (0x803) read returned error (4)
KVM: unknown exit reason 24
- x2apic_set_max_apicid() not being called causes below BUG_ON to happen:
kernel BUG at arch/x86/kernel/apic/io_apic.c:2292!
void __init setup_IO_APIC(void)
{
...
for_each_ioapic(ioapic)
BUG_ON(mp_irqdomain_create(ioapic));
...
}
- Neeraj
On Mon, Nov 25, 2024 at 12:51:36PM +0530, Neeraj Upadhyay wrote:
> I see most of that flow required. By removing dependency on CONFIG_X86_X2APIC
> and enabling SAVIC, I see below boot issues:
Ok, then please extend the Kconfig help text so that it explicitly calls out
the fact that the CONFIG_X86_X2APIC dependency is not only build but
functional one too and that SAVIC relies on X2APIC machinery being present in
the guest.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 11/25/2024 3:38 PM, Borislav Petkov wrote: > On Mon, Nov 25, 2024 at 12:51:36PM +0530, Neeraj Upadhyay wrote: >> I see most of that flow required. By removing dependency on CONFIG_X86_X2APIC >> and enabling SAVIC, I see below boot issues: > > Ok, then please extend the Kconfig help text so that it explicitly calls out > the fact that the CONFIG_X86_X2APIC dependency is not only build but > functional one too and that SAVIC relies on X2APIC machinery being present in > the guest. > Ok, I will update. - Neeraj > Thx. >
On Fri, Sep 13, 2024 at 05:06:52PM +0530, Neeraj Upadhyay wrote:
> diff --git a/include/linux/cc_platform.h b/include/linux/cc_platform.h
> index caa4b4430634..801208678450 100644
> --- a/include/linux/cc_platform.h
> +++ b/include/linux/cc_platform.h
> @@ -88,6 +88,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,
I don't think CC attributes is the right way to track this kind of
features. My understanding of cc_platform interface is that it has to be
used to advertise some kind of property of the platform that generic code
and be interested in, not a specific implementation.
For the same reason, I think CC_ATTR_GUEST/HOST_SEV_SNP is also a bad use
of the interface.
Borislav, I know we had different view on this. What is your criteria on
what should and shouldn't be a CC attribute? I don't think we want a
parallel X86_FEATURE_*.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Wed, Oct 09, 2024 at 01:10:58PM +0300, Kirill A. Shutemov wrote:
> I don't think CC attributes is the right way to track this kind of
> features. My understanding of cc_platform interface is that it has to be
> used to advertise some kind of property of the platform that generic code
> and be interested in, not a specific implementation.
Yes.
>
> For the same reason, I think CC_ATTR_GUEST/HOST_SEV_SNP is also a bad use
> of the interface.
>
> Borislav, I know we had different view on this. What is your criteria on
> what should and shouldn't be a CC attribute? I don't think we want a
> parallel X86_FEATURE_*.
Yes, we don't.
Do you have a better idea which is cleaner than what we do now?
Yes yes, cc_platform reports aspects of the coco platform to generic code but
nothing stops the x86 code from calling those interfaces too, for simplicity
reasons.
Because the coco platform being a SNP guest or having an SAVIC are also two
aspects of that same confidential computing platform. So we might as well use
it this way too.
I'd say.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 09, 2024 at 12:42:34PM +0200, Borislav Petkov wrote: > Do you have a better idea which is cleaner than what we do now? I would rather convert these three attributes to synthetic X86_FEATUREs next to X86_FEATURE_TDX_GUEST. I suggested it once. > Yes yes, cc_platform reports aspects of the coco platform to generic code but > nothing stops the x86 code from calling those interfaces too, for simplicity > reasons. I don't see why it is any simpler than having a synthetic X86_FEATURE. -- Kiryl Shutsemau / Kirill A. Shutemov
On Wed, Oct 09, 2024 at 02:03:48PM +0300, Kirill A. Shutemov wrote:
> I would rather convert these three attributes to synthetic X86_FEATUREs
> next to X86_FEATURE_TDX_GUEST. I suggested it once.
And back then I answered that splitting the coco checks between a X86_FEATURE
and a cc_platform ones is confusing. Which ones do I use, X86_FEATURE or
cc_platform?
Oh, for SNP or TDX I use cpu_feature_enabled() but in generic code I use
cc_platform.
Sounds confusing to me.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 09, 2024 at 01:22:16PM +0200, Borislav Petkov wrote: > On Wed, Oct 09, 2024 at 02:03:48PM +0300, Kirill A. Shutemov wrote: > > I would rather convert these three attributes to synthetic X86_FEATUREs > > next to X86_FEATURE_TDX_GUEST. I suggested it once. > > And back then I answered that splitting the coco checks between a X86_FEATURE > and a cc_platform ones is confusing. Which ones do I use, X86_FEATURE or > cc_platform? > > Oh, for SNP or TDX I use cpu_feature_enabled() but in generic code I use > cc_platform. > > Sounds confusing to me. If you use SNP or TDX check in generic code something is wrong. Abstraction is broken somewhere. Generic code doesn't need to know concrete implementation. -- Kiryl Shutsemau / Kirill A. Shutemov
On Wed, Oct 09, 2024 at 03:12:41PM +0300, Kirill A. Shutemov wrote:
> If you use SNP or TDX check in generic code something is wrong. Abstraction
> is broken somewhere. Generic code doesn't need to know concrete
> implementation.
That's perhaps because you're thinking that the *actual* coco implementation type
should be hidden away from generic code. But SNP and TDX are pretty different
so we might as well ask for them by their name.
But I can see why you'd think there might be some abstraction violation there.
My goal here - even though there might be some bad taste of abstraction
violation here - is simplicity. As expressed a bunch of times already, having
cc_platform *and* X86_FEATURE* things used in relation to coco code can be
confusing. So I'd prefer to avoid that confusion.
Nothing says anywhere that arch code cannot use cc_platform interfaces.
Absolutely nothing. So for the sake of KISS I'm going in that direction.
If it turns out later that this was a bad idea and we need to change it, we
can always can. As we do for other interfaces in the kernel.
If you're still not convinced, I already asked you:
"Do you have a better idea which is cleaner than what we do now?"
Your turn.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 09, 2024 at 03:53:35PM +0200, Borislav Petkov wrote: > On Wed, Oct 09, 2024 at 03:12:41PM +0300, Kirill A. Shutemov wrote: > > If you use SNP or TDX check in generic code something is wrong. Abstraction > > is broken somewhere. Generic code doesn't need to know concrete > > implementation. > > That's perhaps because you're thinking that the *actual* coco implementation type > should be hidden away from generic code. But SNP and TDX are pretty different > so we might as well ask for them by their name. > > But I can see why you'd think there might be some abstraction violation there. > > My goal here - even though there might be some bad taste of abstraction > violation here - is simplicity. As expressed a bunch of times already, having > cc_platform *and* X86_FEATURE* things used in relation to coco code can be > confusing. So I'd prefer to avoid that confusion. > > Nothing says anywhere that arch code cannot use cc_platform interfaces. > Absolutely nothing. So for the sake of KISS I'm going in that direction. > > If it turns out later that this was a bad idea and we need to change it, we > can always can. As we do for other interfaces in the kernel. > > If you're still not convinced, I already asked you: > > "Do you have a better idea which is cleaner than what we do now?" > > Your turn. Okay, I've got your point. It is not what I would do, but I don't have sufficient argument to change what is already there. -- Kiryl Shutsemau / Kirill A. Shutemov
On Fri, Sep 13, 2024 at 05:06:52PM +0530, Neeraj Upadhyay wrote:
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index cd44e120fe53..ec038be0a048 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_ENABLED | \
> MSR_AMD64_SNP_RESERVED_MASK)
>
> /*
Shouldn't this hunk be in the last patch of the series, after all the sAVIC
enablement has happened?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/9/2024 12:45 AM, Borislav Petkov wrote: > On Fri, Sep 13, 2024 at 05:06:52PM +0530, Neeraj Upadhyay wrote: >> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c >> index cd44e120fe53..ec038be0a048 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_ENABLED | \ >> MSR_AMD64_SNP_RESERVED_MASK) >> >> /* > > Shouldn't this hunk be in the last patch of the series, after all the sAVIC > enablement has happened? > As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features()) by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest support is indicated by guest. - Neeraj
On Wed, Oct 09, 2024 at 07:26:55AM +0530, Neeraj Upadhyay wrote:
> As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features())
> by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit
> value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos
> was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest
> support is indicated by guest.
So what's the point of adding it to SNP_FEATURES_IMPL_REQ here? What does that
do at all in this patch alone? Why is this change needed in here?
IOW, why don't you do all the feature bit handling in the last patch, where it
all belongs logically?
In the last patch you can start *testing* for
MSR_AMD64_SNP_SECURE_AVIC_ENABLED *and* enforce it with SNP_FEATURES_PRESENT.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/9/2024 10:53 AM, Borislav Petkov wrote: > On Wed, Oct 09, 2024 at 07:26:55AM +0530, Neeraj Upadhyay wrote: >> As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features()) >> by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit >> value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos >> was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest >> support is indicated by guest. > > So what's the point of adding it to SNP_FEATURES_IMPL_REQ here? What does that > do at all in this patch alone? Why is this change needed in here? > Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would terminate in snp_check_features(). The reason for this is, SNP_FEATURES_IMPL_REQ had the Secure AVIC bit set before this patch, as that bit was part of MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, 18). #define SNP_FEATURES_IMPL_REQ (MSR_AMD64_SNP_VTOM | \ ... MSR_AMD64_SNP_RESERVED_MASK) Adding MSR_AMD64_SNP_SECURE_AVIC_BIT (bit 18) to SNP_FEATURES_IMPL_REQ in this patch keeps that behavior intact as now with this change MSR_AMD64_SNP_RESERVED_MASK becomes GENMASK_ULL(63, 19). > IOW, why don't you do all the feature bit handling in the last patch, where it > all belongs logically? > If we do that, then hypervisor could have enabled Secure AVIC support and the guest code at this patch won't catch the missing guest-support early and it can result in some unknown failures at later point during guest boot. - Neeraj > In the last patch you can start *testing* for > MSR_AMD64_SNP_SECURE_AVIC_ENABLED *and* enforce it with SNP_FEATURES_PRESENT. >
On 10/9/24 01:01, Neeraj Upadhyay wrote: > > > On 10/9/2024 10:53 AM, Borislav Petkov wrote: >> On Wed, Oct 09, 2024 at 07:26:55AM +0530, Neeraj Upadhyay wrote: >>> As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features()) >>> by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit >>> value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos >>> was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest >>> support is indicated by guest. >> >> So what's the point of adding it to SNP_FEATURES_IMPL_REQ here? What does that >> do at all in this patch alone? Why is this change needed in here? >> > > Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would > terminate in snp_check_features(). The reason for this is, SNP_FEATURES_IMPL_REQ had the Secure > AVIC bit set before this patch, as that bit was part of MSR_AMD64_SNP_RESERVED_MASK > GENMASK_ULL(63, 18). > > #define SNP_FEATURES_IMPL_REQ (MSR_AMD64_SNP_VTOM | \ > ... > MSR_AMD64_SNP_RESERVED_MASK) > > > > Adding MSR_AMD64_SNP_SECURE_AVIC_BIT (bit 18) to SNP_FEATURES_IMPL_REQ in this patch > keeps that behavior intact as now with this change MSR_AMD64_SNP_RESERVED_MASK becomes > GENMASK_ULL(63, 19). > > >> IOW, why don't you do all the feature bit handling in the last patch, where it >> all belongs logically? >> > > If we do that, then hypervisor could have enabled Secure AVIC support and the guest > code at this patch won't catch the missing guest-support early and it can result in some > unknown failures at later point during guest boot. Won't the SNP_RESERVED_MASK catch it? You are just renaming the bit position value, right? It was a 1 before and is still a 1. So the guest will terminate if the hypervisor sets the Secure AVIC bit both before and after this patch, right? Thanks, Tom > > > - Neeraj > >> In the last patch you can start *testing* for >> MSR_AMD64_SNP_SECURE_AVIC_ENABLED *and* enforce it with SNP_FEATURES_PRESENT. >>
On 10/9/2024 6:45 PM, Tom Lendacky wrote: > On 10/9/24 01:01, Neeraj Upadhyay wrote: >> >> >> On 10/9/2024 10:53 AM, Borislav Petkov wrote: >>> On Wed, Oct 09, 2024 at 07:26:55AM +0530, Neeraj Upadhyay wrote: >>>> As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features()) >>>> by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit >>>> value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos >>>> was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest >>>> support is indicated by guest. >>> >>> So what's the point of adding it to SNP_FEATURES_IMPL_REQ here? What does that >>> do at all in this patch alone? Why is this change needed in here? >>> >> >> Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would >> terminate in snp_check_features(). The reason for this is, SNP_FEATURES_IMPL_REQ had the Secure >> AVIC bit set before this patch, as that bit was part of MSR_AMD64_SNP_RESERVED_MASK >> GENMASK_ULL(63, 18). >> >> #define SNP_FEATURES_IMPL_REQ (MSR_AMD64_SNP_VTOM | \ >> ... >> MSR_AMD64_SNP_RESERVED_MASK) >> >> >> >> Adding MSR_AMD64_SNP_SECURE_AVIC_BIT (bit 18) to SNP_FEATURES_IMPL_REQ in this patch >> keeps that behavior intact as now with this change MSR_AMD64_SNP_RESERVED_MASK becomes >> GENMASK_ULL(63, 19). >> >> >>> IOW, why don't you do all the feature bit handling in the last patch, where it >>> all belongs logically? >>> >> >> If we do that, then hypervisor could have enabled Secure AVIC support and the guest >> code at this patch won't catch the missing guest-support early and it can result in some >> unknown failures at later point during guest boot. > > Won't the SNP_RESERVED_MASK catch it? You are just renaming the bit > position value, right? It was a 1 before and is still a 1. So the guest > will terminate if the hypervisor sets the Secure AVIC bit both before > and after this patch, right? > Yes that is right. SNP_RESERVED_MASK catches it before this patch. My reply to Boris above was for the case if we move setting of MSR_AMD64_SNP_SECURE_AVIC_ENABLED in SNP_FEATURES_IMPL_REQ from this patch to patch 14. - Neeraj > Thanks, > Tom > >> >> >> - Neeraj >> >>> In the last patch you can start *testing* for >>> MSR_AMD64_SNP_SECURE_AVIC_ENABLED *and* enforce it with SNP_FEATURES_PRESENT. >>>
On Wed, Oct 09, 2024 at 11:31:07AM +0530, Neeraj Upadhyay wrote:
> Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would
> terminate in snp_check_features().
We want the guest to terminate at this patch too.
The only case where the guest should not terminate is when the *full* sAVIC
support is in. I.e., at patch 14.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Wed, Oct 09, 2024 at 12:38:21PM +0200, Borislav Petkov wrote:
> On Wed, Oct 09, 2024 at 11:31:07AM +0530, Neeraj Upadhyay wrote:
> > Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would
> > terminate in snp_check_features().
>
> We want the guest to terminate at this patch too.
>
> The only case where the guest should not terminate is when the *full* sAVIC
> support is in. I.e., at patch 14.
I went and did a small C program doing all that - I see what you mean now
- you want to *enforce* the guest termination when SAVIC bit is not in
SNP_FEATURES_PRESENT.
Basically what I want you do to.
Please call that out in the commit message as it is important.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/9/2024 4:32 PM, Borislav Petkov wrote: > On Wed, Oct 09, 2024 at 12:38:21PM +0200, Borislav Petkov wrote: >> On Wed, Oct 09, 2024 at 11:31:07AM +0530, Neeraj Upadhyay wrote: >>> Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would >>> terminate in snp_check_features(). >> >> We want the guest to terminate at this patch too. >> >> The only case where the guest should not terminate is when the *full* sAVIC >> support is in. I.e., at patch 14. > > I went and did a small C program doing all that - I see what you mean now > - you want to *enforce* the guest termination when SAVIC bit is not in > SNP_FEATURES_PRESENT. > Yes. > Basically what I want you do to. > Cool! > Please call that out in the commit message as it is important. > Sure, will update in next version. Thanks! - Neeraj > Thx. >
On 10/9/2024 4:08 PM, Borislav Petkov wrote: > On Wed, Oct 09, 2024 at 11:31:07AM +0530, Neeraj Upadhyay wrote: >> Before this patch, if hypervisor enables Secure AVIC (reported in sev_status), guest would >> terminate in snp_check_features(). > > We want the guest to terminate at this patch too. > If I understand it correctly, you are fine with adding MSR_AMD64_SNP_SECURE_AVIC_ENABLED to SNP_FEATURES_IMPL_REQ in this patch. > The only case where the guest should not terminate is when the *full* sAVIC > support is in. I.e., at patch 14. > Got it. This version of the patch series is following that. - Neeraj
© 2016 - 2024 Red Hat, Inc.