Re: [RFC 04/14] x86/apic: Initialize APIC backing page for Secure AVIC

[Borislav Petkov]

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 = &regs };
> +	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