[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241107152831.GZZyzcn2Tn2eIrMlzq@fat_crate.local>
Date: Thu, 7 Nov 2024 16:28:31 +0100
From: Borislav Petkov <bp@...en8.de>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.de, mingo@...hat.com,
dave.hansen@...ux.intel.com, Thomas.Lendacky@....com,
nikunj@....com, Santosh.Shukla@....com, Vasant.Hegde@....com,
Suravee.Suthikulpanit@....com, David.Kaplan@....com, x86@...nel.org,
hpa@...or.com, peterz@...radead.org, seanjc@...gle.com,
pbonzini@...hat.com, kvm@...r.kernel.org
Subject: Re: [RFC 04/14] x86/apic: Initialize APIC backing page for Secure
AVIC
On Fri, Sep 13, 2024 at 05:06:55PM +0530, Neeraj Upadhyay wrote:
> From: Kishon Vijay Abraham I <kvijayab@....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@....com>
> Co-developed-by: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
> Signed-off-by: Neeraj Upadhyay <Neeraj.Upadhyay@....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);
> +}
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists