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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2021 14:44, Andrew Cooper wrote:
> 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@xxxxxxxx>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>>  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.

But if the original ASSERT() was correct, how can the ASSERT_UNREACHABLE()
above need dropping? Are you perhaps only talking about the other one?

Jan