xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
@ 2021-04-01 10:22 Roger Pau Monne
  2021-04-01 10:50 ` Paul Durrant
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Roger Pau Monne @ 2021-04-01 10:22 UTC (permalink / raw)
  To: xen-devel
  Cc: Roger Pau Monne, Paul Durrant, Wei Liu, Jan Beulich, Andrew Cooper

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



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 10:22 [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context Roger Pau Monne
@ 2021-04-01 10:50 ` Paul Durrant
  2021-04-15  9:57   ` Jan Beulich
  2021-04-01 10:55 ` Jan Beulich
  2021-04-01 12:44 ` Andrew Cooper
  2 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2021-04-01 10:50 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Wei Liu, Jan Beulich, 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>

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 10:22 [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context Roger Pau Monne
  2021-04-01 10:50 ` Paul Durrant
@ 2021-04-01 10:55 ` Jan Beulich
  2021-04-01 12:44 ` Andrew Cooper
  2 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2021-04-01 10:55 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Paul Durrant, Wei Liu, Andrew Cooper, xen-devel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 10:22 [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context Roger Pau Monne
  2021-04-01 10:50 ` Paul Durrant
  2021-04-01 10:55 ` Jan Beulich
@ 2021-04-01 12:44 ` Andrew Cooper
  2021-04-01 13:38   ` Jan Beulich
  2021-04-01 14:41   ` Roger Pau Monné
  2 siblings, 2 replies; 8+ messages in thread
From: Andrew Cooper @ 2021-04-01 12:44 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Paul Durrant, Wei Liu, Jan Beulich

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 12:44 ` Andrew Cooper
@ 2021-04-01 13:38   ` Jan Beulich
  2021-04-01 14:41   ` Roger Pau Monné
  1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2021-04-01 13:38 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Paul Durrant, Wei Liu, xen-devel, Roger Pau Monne

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 12:44 ` Andrew Cooper
  2021-04-01 13:38   ` Jan Beulich
@ 2021-04-01 14:41   ` Roger Pau Monné
  1 sibling, 0 replies; 8+ messages in thread
From: Roger Pau Monné @ 2021-04-01 14:41 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Paul Durrant, Wei Liu, Jan Beulich

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-01 10:50 ` Paul Durrant
@ 2021-04-15  9:57   ` Jan Beulich
  2022-01-05  7:46     ` Jan Beulich
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2021-04-15  9:57 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, xen-devel, Roger Pau Monne, paul

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
  2021-04-15  9:57   ` Jan Beulich
@ 2022-01-05  7:46     ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2022-01-05  7:46 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, xen-devel, Roger Pau Monne, paul

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-01-05  7:46 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-01 10:22 [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context Roger Pau Monne
2021-04-01 10:50 ` Paul Durrant
2021-04-15  9:57   ` Jan Beulich
2022-01-05  7:46     ` Jan Beulich
2021-04-01 10:55 ` Jan Beulich
2021-04-01 12:44 ` Andrew Cooper
2021-04-01 13:38   ` Jan Beulich
2021-04-01 14:41   ` Roger Pau Monné

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).