On 11/7/2024 8:58 PM, Borislav Petkov wrote: > 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. I will improve on this in the next version. >> -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 > Ok nice! I will rebase. >> +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? > As sev_ghcb_msr_read() work with any msr and is not limited to reading x2apic msrs, I created a global sev function for it. > 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. > Ok sure, I will leave generalizing this to future use cases (if/when they come up) and provide a secure avic specific function here (will do the same for sev_ghcb_msr_write(), which comes later in this series). "x2avic" terminology is not used in guest code. As this function only has secure avic user, does secure_avic_ghcb_msr_read() work? >> +enum lapic_lvt_entry { > > What's that enum for? It's used in init_backing_page() 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); } > > Oh, you want to use it below but you don't. Why? > As LVT_TIMER is unused, I will remove it: enum lapic_lvt_entry { LVT_THERMAL_MONITOR = 1, LVT_PERFORMANCE_COUNTER, LVT_LINT0, LVT_LINT1, LVT_ERROR, APIC_MAX_NR_LVT_ENTRIES, }; >> + 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. > Ok sure, will update. >> +{ >> + 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. > Using '#' prepends "0x". Am I missing something here? - Neeraj
