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

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

[Roger Pau Monne]

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;
+    }
 
     switch ( idx )
     {
     case HV_X64_MSR_EOI:
+        if ( v != current )
+        {
+            ASSERT_UNREACHABLE();
+            return X86EMUL_EXCEPTION;
+        }
         vlapic_EOI_set(vcpu_vlapic(v));
         break;
 
-- 
2.30.1

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

[Paul Durrant]

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>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
>   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;
> +    }
>   
>       switch ( idx )
>       {
>       case HV_X64_MSR_EOI:
> +        if ( v != current )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return X86EMUL_EXCEPTION;
> +        }
>           vlapic_EOI_set(vcpu_vlapic(v));
>           break;
>   
> 

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

[Jan Beulich]

On 01.04.2021 12:50, Paul Durrant 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@suse.com>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Paul Durrant <paul@xen.org>

Andrew,

can you please clarify whether your concern was addressed and this can
go in as-is, or (if not) reply to what Roger and I have said in response?

Thanks, Jan

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

[Jan Beulich]

On 15.04.2021 11:57, Jan Beulich wrote:
> On 01.04.2021 12:50, Paul Durrant 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@suse.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Paul Durrant <paul@xen.org>
> 
> Andrew,
> 
> can you please clarify whether your concern was addressed and this can
> go in as-is, or (if not) reply to what Roger and I have said in response?

This continues to be pending. I guess I'll consider ongoing silence as an
indication of your concerns having been addressed, committing the patch
perhaps early next week.

Jan

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

[Jan Beulich]

On 01.04.2021 12: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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- 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;
> +    }
>  
>      switch ( idx )
>      {
>      case HV_X64_MSR_EOI:
> +        if ( v != current )
> +        {
> +            ASSERT_UNREACHABLE();
> +            return X86EMUL_EXCEPTION;
> +        }
>          vlapic_EOI_set(vcpu_vlapic(v));

I suppose this function then also wants to lose its parameter. But
I'll reply to patch 1 of the other series again as well.

Jan

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

[Andrew Cooper]

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

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

[Jan Beulich]

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@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.

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

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

[Roger Pau Monné]

On Thu, Apr 01, 2021 at 01:44:59PM +0100, 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@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.

I'm confused, if it's reachable from control domain context then it
shouldn't be an assert?

> 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.

Looking at hvm_msr_write_intercept I see that indeed introspection can
monitor an MSR and defer a write, but AFAICT the postponed write will
be performed in guest vcpu context as part of
hvm_vm_event_do_resume?

Also AFAICT there's no way for a control domain to write to this MSR
ATM, as the allowed list in hvm_load_cpu_msrs doesn't contain
HV_X64_MSR_EOI.

Thanks, Roger.