| Message ID | 20240913113705.419146-5-Neeraj.Upadhyay@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMD: Add Secure AVIC Guest Support | expand |
On Fri, Sep 13, 2024 at 05:06:55PM +0530, Neeraj Upadhyay wrote: > From: Kishon Vijay Abraham I <kvijayab@amd.com> > > Secure AVIC lets guest manage the APIC backing page (unlike emulated > x2APIC or x2AVIC where the hypervisor manages the APIC backing page). > > However the introduced Secure AVIC Linux design still maintains the > APIC backing page in the hypervisor to shadow the APIC backing page > maintained by guest (It should be noted only subset of the registers > are shadowed for specific usecases and registers like APIC_IRR, > APIC_ISR are not shadowed). > > Add sev_ghcb_msr_read() to invoke "SVM_EXIT_MSR" VMGEXIT to read > MSRs from hypervisor. Initialize the Secure AVIC's APIC backing > page by copying the initial state of shadow APIC backing page in > the hypervisor to the guest APIC backing page. Specifically copy > APIC_LVR, APIC_LDR, and APIC_LVT MSRs from the shadow APIC backing > page. You don't have to explain what the patch does - rather, why the patch exists in the first place and perhaps mention some non-obvious stuff why the code does what it does. Check your whole set pls. > 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/coco/sev/core.c | 41 ++++++++++++++++----- > arch/x86/include/asm/sev.h | 2 ++ > arch/x86/kernel/apic/x2apic_savic.c | 55 +++++++++++++++++++++++++++++ > 3 files changed, 90 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 93470538af5e..0e140f92cfef 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -1331,18 +1331,15 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd) > return 0; > } > > -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) Yeah, this one was bugging me already during Nikunj's set so I cleaned it up a bit differently: https://git.kernel.org/tip/8bca85cc1eb72e21a3544ab32e546a819d8674ca > +enum es_result sev_ghcb_msr_read(u64 msr, u64 *value) Why is this a separate function if it is called only once from x2avic_savic.c? I think you should merge it with read_msr_from_hv(), rename latter to x2avic_read_msr_from_hv() and leave it here in sev/core.c. > +{ > + struct pt_regs regs = { .cx = msr }; > + struct es_em_ctxt ctxt = { .regs = ®s }; > + 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) > + *value = regs.ax | regs.dx << 32; > + > + __sev_put_ghcb(&state); > + local_irq_restore(flags); > + > + return ret; > +} ... > diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c > index 6a471bbc3dba..99151be4e173 100644 > --- a/arch/x86/kernel/apic/x2apic_savic.c > +++ b/arch/x86/kernel/apic/x2apic_savic.c > @@ -11,6 +11,7 @@ > #include <linux/cc_platform.h> > #include <linux/percpu-defs.h> > #include <linux/align.h> > +#include <linux/sizes.h> > > #include <asm/apic.h> > #include <asm/sev.h> > @@ -20,6 +21,19 @@ > static DEFINE_PER_CPU(void *, apic_backing_page); > static DEFINE_PER_CPU(bool, savic_setup_done); > > +enum lapic_lvt_entry { What's that enum for? Oh, you want to use it below but you don't. Why? > + LVT_TIMER, > + LVT_THERMAL_MONITOR, > + LVT_PERFORMANCE_COUNTER, > + LVT_LINT0, > + LVT_LINT1, > + LVT_ERROR, > + > + APIC_MAX_NR_LVT_ENTRIES, > +}; > + > +#define APIC_LVTx(x) (APIC_LVTT + 0x10 * (x)) > + > 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); > @@ -35,6 +49,22 @@ static inline void set_reg(char *page, int reg_off, u32 val) > WRITE_ONCE(*((u32 *)(page + reg_off)), val); > } > > +static u32 read_msr_from_hv(u32 reg) A MSR's contents is u64. Make this function generic enough and have the callsite select only the lower dword. > +{ > + u64 data, msr; > + int ret; > + > + msr = APIC_BASE_MSR + (reg >> 4); > + ret = sev_ghcb_msr_read(msr, &data); > + if (ret != ES_OK) { > + pr_err("Secure AVIC msr (%#llx) read returned error (%d)\n", msr, ret); Prepend "0x" to the format specifier. > + /* MSR read failures are treated as fatal errors */ > + snp_abort(); > + } > + > + return lower_32_bits(data); > +}
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 93470538af5e..0e140f92cfef 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -1331,18 +1331,15 @@ int __init sev_es_efi_map_ghcbs(pgd_t *pgd) return 0; } -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; + u64 exit_info_1 = write ? 1 : 0; enum es_result ret; - u64 exit_info_1; - - /* Is it a WRMSR? */ - exit_info_1 = (ctxt->insn.opcode.bytes[1] == 0x30) ? 1 : 0; if (regs->cx == MSR_SVSM_CAA) { /* Writes to the SVSM CAA msr are ignored */ - if (exit_info_1) + if (write) return ES_OK; regs->ax = lower_32_bits(this_cpu_read(svsm_caa_pa)); @@ -1352,14 +1349,14 @@ static enum es_result vc_handle_msr(struct ghcb *ghcb, struct es_em_ctxt *ctxt) } ghcb_set_rcx(ghcb, regs->cx); - if (exit_info_1) { + if (write) { ghcb_set_rax(ghcb, regs->ax); ghcb_set_rdx(ghcb, regs->dx); } ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_MSR, exit_info_1, 0); - if ((ret == ES_OK) && (!exit_info_1)) { + if (ret == ES_OK && !write) { regs->ax = ghcb->save.rax; regs->dx = ghcb->save.rdx; } @@ -1367,6 +1364,34 @@ 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); +} + +enum es_result sev_ghcb_msr_read(u64 msr, u64 *value) +{ + struct pt_regs regs = { .cx = msr }; + struct es_em_ctxt ctxt = { .regs = ®s }; + 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) + *value = regs.ax | regs.dx << 32; + + __sev_put_ghcb(&state); + local_irq_restore(flags); + + return ret; +} + enum es_result sev_notify_savic_gpa(u64 gpa) { struct ghcb_state state; diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index e84fc7fcc32a..5e6385bfb85a 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -400,6 +400,7 @@ u64 sev_get_status(void); void sev_show_status(void); void snp_update_svsm_ca(void); enum es_result sev_notify_savic_gpa(u64 gpa); +enum es_result sev_ghcb_msr_read(u64 msr, u64 *value); #else /* !CONFIG_AMD_MEM_ENCRYPT */ @@ -437,6 +438,7 @@ static inline u64 sev_get_status(void) { return 0; } static inline void sev_show_status(void) { } static inline void snp_update_svsm_ca(void) { } static inline enum es_result sev_notify_savic_gpa(u64 gpa) { return ES_UNSUPPORTED; } +static inline enum es_result sev_ghcb_msr_read(u64 msr, u64 *value) { return ES_UNSUPPORTED; } #endif /* CONFIG_AMD_MEM_ENCRYPT */ diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c index 6a471bbc3dba..99151be4e173 100644 --- a/arch/x86/kernel/apic/x2apic_savic.c +++ b/arch/x86/kernel/apic/x2apic_savic.c @@ -11,6 +11,7 @@ #include <linux/cc_platform.h> #include <linux/percpu-defs.h> #include <linux/align.h> +#include <linux/sizes.h> #include <asm/apic.h> #include <asm/sev.h> @@ -20,6 +21,19 @@ static DEFINE_PER_CPU(void *, apic_backing_page); static DEFINE_PER_CPU(bool, savic_setup_done); +enum lapic_lvt_entry { + LVT_TIMER, + LVT_THERMAL_MONITOR, + LVT_PERFORMANCE_COUNTER, + LVT_LINT0, + LVT_LINT1, + LVT_ERROR, + + APIC_MAX_NR_LVT_ENTRIES, +}; + +#define APIC_LVTx(x) (APIC_LVTT + 0x10 * (x)) + 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); @@ -35,6 +49,22 @@ static inline void set_reg(char *page, int reg_off, u32 val) WRITE_ONCE(*((u32 *)(page + reg_off)), val); } +static u32 read_msr_from_hv(u32 reg) +{ + u64 data, msr; + int ret; + + msr = APIC_BASE_MSR + (reg >> 4); + ret = sev_ghcb_msr_read(msr, &data); + if (ret != ES_OK) { + pr_err("Secure AVIC msr (%#llx) read returned error (%d)\n", msr, ret); + /* MSR read failures are treated as fatal errors */ + snp_abort(); + } + + return lower_32_bits(data); +} + #define SAVIC_ALLOWED_IRR_OFFSET 0x204 static u32 x2apic_savic_read(u32 reg) @@ -168,6 +198,30 @@ 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) +{ + u32 val; + int i; + + val = read_msr_from_hv(APIC_LVR); + set_reg(backing_page, APIC_LVR, val); + + /* + * Hypervisor is used for all timer related functions, + * so don't copy those values. + */ + for (i = LVT_THERMAL_MONITOR; i < APIC_MAX_NR_LVT_ENTRIES; i++) { + val = read_msr_from_hv(APIC_LVTx(i)); + set_reg(backing_page, APIC_LVTx(i), val); + } + + val = read_msr_from_hv(APIC_LVT0); + set_reg(backing_page, APIC_LVT0, val); + + val = read_msr_from_hv(APIC_LDR); + set_reg(backing_page, APIC_LDR, val); +} + static void x2apic_savic_setup(void) { void *backing_page; @@ -178,6 +232,7 @@ static void x2apic_savic_setup(void) return; backing_page = this_cpu_read(apic_backing_page); + init_backing_page(backing_page); gpa = __pa(backing_page); ret = sev_notify_savic_gpa(gpa); if (ret != ES_OK)