Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 01/04/2021 11:22, Roger Pau Monne wrote:
> The HV_X64_MSR_EOI wrmsr should always happen with the target vCPU
> as current, as there's no support for EOI'ing interrupts on a remote
> vCPU.
>
> While there also turn the unconditional assert at the top of the
> function into an error on non-debug builds.
>
> No functional change intended.
>
> Requested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/viridian/synic.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/viridian/synic.c b/xen/arch/x86/hvm/viridian/synic.c
> index 22e2df27e5d..e18538c60a6 100644
> --- a/xen/arch/x86/hvm/viridian/synic.c
> +++ b/xen/arch/x86/hvm/viridian/synic.c
> @@ -79,11 +79,20 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, uint64_t val)
>      struct viridian_vcpu *vv = v->arch.hvm.viridian;
>      struct domain *d = v->domain;
>  
> -    ASSERT(v == current || !v->is_running);
> +    if ( v != current && v->is_running )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return X86EMUL_EXCEPTION;
> +    }

The original ASSERT() was correct - both of these are easily reachable
in control domain context.

If you want EOI to not be used, you need to raise #GP from it, but that
in principle breaks introspection which really does write MSRs on the
guests behalf.

It's perhaps fine in principle to leave that problem to whomever first
wants to poke this MSR from introspection context, but the
ASSERT_UNREACHABLE()s need dropping whatever the introspection angle.

~Andrew