1. Patchew
  2. Xen
  3. View series

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

Roger Pau Monne posted 1 patch 3 years ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210401102252.95196-1-roger.pau@citrix.com
xen/arch/x86/hvm/viridian/synic.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
[PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
Posted by Roger Pau Monne 3 years ago 
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
Posted by Jan Beulich 3 years ago 
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
Posted by Andrew Cooper 3 years ago 
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
Posted by Jan Beulich 3 years ago 
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
Posted by Roger Pau Monné 3 years ago 
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.
Re: [PATCH] x86/viridian: EOI MSR should always happen in affected vCPU context
Posted by Paul Durrant 3 years ago 
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
Posted by Jan Beulich 3 years ago 
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
Posted by Jan Beulich 2 years, 3 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.