On Thu, Jun 10, 2021 at 11:11:37AM +0200, Joerg Roedel wrote:

From: Joerg Roedel <redacted>

The #VC handler only cares about IRQs being disabled while the GHCB is
active, as it must not be interrupted by something which could cause
another #VC while it holds the GHCB (NMI is the exception for which the
backup GHCB is there).

Make sure nothing interrupts the code path while the GHCB is active by
disabling IRQs in sev_es_get_ghcb() and restoring the previous irq state
in sev_es_put_ghcb().

Why this unnecessarily complicated passing of flags back and forth?

Why not simply "sandwich" them:

	local_irq_save()
	sev_es_get_ghcb()

	...blablabla

	sev_es_put_ghcb()
	local_irq_restore();

in every call site?

What's the difference in passing *flags in and have the
get_ghcb/put_ghcb save/restore flags instead of the callers?

-static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
+static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state,
+						    unsigned long *flags)
 {
 	struct sev_es_runtime_data *data;
 	struct ghcb *ghcb;
 
+	/*
+	 * Nothing shall interrupt this code path while holding the per-cpu
+	 * GHCB. The backup GHCB is only for NMIs interrupting this path.

Hmm, so why aren't you accessing/setting data->ghcb_active and
data->backup_ghcb_active safely using cmpxchg() if this path can be
interrupted by an NMI?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization