On Fri, Sep 13, 2024 at 05:06:55PM +0530, Neeraj Upadhyay wrote: > From: Kishon Vijay Abraham I <kvijayab@xxxxxxx> > > 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@xxxxxxx> > Co-developed-by: Neeraj Upadhyay <Neeraj.Upadhyay@xxxxxxx> > Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@xxxxxxx> > --- > 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); > +} -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
