Mailing List Archive: Xen: Devel

[PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

Index | Next | Previous | View Threaded

yang.z.zhang at intel

Aug 9, 2013, 1:49 AM

Post #1 of 6 (14 views)
Permalink

[PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1

From: Yang Zhang <yang.z.zhang [at] Intel>

If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
But when L2 is running, external interrupt will casue L1 vmexit with
reason external interrupt. Then L1 will pick up the interrupt through
vmcs. So we need to update APIC-v related structure accordingly;

Signed-off-by: Yang Zhang <yang.z.zhang [at] Intel>
---
xen/arch/x86/hvm/vmx/vvmx.c | 37 +++++++++++++++++++++++++++++++++++++
1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5dfbc54..9ba169d 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1283,6 +1283,41 @@ static void sync_exception_state(struct vcpu *v)
}
}

+static void nvmx_update_apicv(struct vcpu *v)
+{
+ struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+ struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
+ unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON);
+ uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO);
+
+ if ( !cpu_has_vmx_virtual_intr_delivery )
+ return;
+
+ if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
+ nvmx->intr.source == hvm_intsrc_lapic &&
+ (intr_info & INTR_INFO_VALID_MASK) )
+ {
+ unsigned long status;
+ uint32_t rvi, ppr;
+ uint32_t vector = intr_info & 0xff;
+ struct vlapic *vlapic = vcpu_vlapic(v);
+
+ WARN_ON(vector <= 0);
+
+ vlapic_ack_pending_irq(v, vector, 1);
+
+ ppr = vlapic_set_ppr(vlapic);
+ BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
+
+ status = (vector << 8) & 0xff00;
+ rvi = vlapic_has_pending_irq(v);
+ if ( rvi != -1 )
+ status |= rvi & 0xff;
+
+ __vmwrite(GUEST_INTR_STATUS, status);
+ }
+}
+
static void virtual_vmexit(struct cpu_user_regs *regs)
{
struct vcpu *v = current;
@@ -1328,6 +1363,8 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
/* updating host cr0 to sync TS bit */
__vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);

+ nvmx_update_apicv(v);
+
vmreturn(regs, VMSUCCEED);
}

--
1.7.1

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

andrew.cooper3 at citrix

Aug 9, 2013, 3:49 AM

Post #2 of 6 (11 views)
Permalink

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1 [In reply to]

On 09/08/13 09:49, Yang Zhang wrote:
> From: Yang Zhang <yang.z.zhang [at] Intel>
>
> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
> But when L2 is running, external interrupt will casue L1 vmexit with
> reason external interrupt. Then L1 will pick up the interrupt through
> vmcs. So we need to update APIC-v related structure accordingly;
>
> Signed-off-by: Yang Zhang <yang.z.zhang [at] Intel>
> ---
> xen/arch/x86/hvm/vmx/vvmx.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 5dfbc54..9ba169d 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1283,6 +1283,41 @@ static void sync_exception_state(struct vcpu *v)
> }
> }
>
> +static void nvmx_update_apicv(struct vcpu *v)
> +{
> + struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> + struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> + unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON);
> + uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO);
> +
> + if ( !cpu_has_vmx_virtual_intr_delivery )
> + return;

Is it worth hoisting this check outside of the function, to avoid
needless __get_vvmcs() calls on some hardware?

> +
> + if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> + nvmx->intr.source == hvm_intsrc_lapic &&
> + (intr_info & INTR_INFO_VALID_MASK) )
> + {
> + unsigned long status;
> + uint32_t rvi, ppr;
> + uint32_t vector = intr_info & 0xff;
> + struct vlapic *vlapic = vcpu_vlapic(v);
> +
> + WARN_ON(vector <= 0);
> +
> + vlapic_ack_pending_irq(v, vector, 1);
> +
> + ppr = vlapic_set_ppr(vlapic);
> + BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
> +
> + status = (vector << 8) & 0xff00;

Given "vector = intr_info & 0xff;" above, this "& 0xff00" looks useless.

~Andrew

> + rvi = vlapic_has_pending_irq(v);
> + if ( rvi != -1 )
> + status |= rvi & 0xff;
> +
> + __vmwrite(GUEST_INTR_STATUS, status);
> + }
> +}
> +
> static void virtual_vmexit(struct cpu_user_regs *regs)
> {
> struct vcpu *v = current;
> @@ -1328,6 +1363,8 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
> /* updating host cr0 to sync TS bit */
> __vmwrite(HOST_CR0, v->arch.hvm_vmx.host_cr0);
>
> + nvmx_update_apicv(v);
> +
> vmreturn(regs, VMSUCCEED);
> }
>

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

JBeulich at suse

Aug 9, 2013, 5:31 AM

Post #3 of 6 (10 views)
Permalink

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1 [In reply to]

>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang [at] intel> wrote:
> From: Yang Zhang <yang.z.zhang [at] Intel>
>
> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
> But when L2 is running, external interrupt will casue L1 vmexit with
> reason external interrupt. Then L1 will pick up the interrupt through
> vmcs. So we need to update APIC-v related structure accordingly;

This doesn't sound logical to me: If L1 picks up the interrupt from
VMCS, how can the be a reason/explanation for the need to update
the APIC-v related data?

> +static void nvmx_update_apicv(struct vcpu *v)
> +{
> + struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> + struct nestedvcpu *nvcpu = &vcpu_nestedhvm(v);
> + unsigned long reason = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON);
> + uint32_t intr_info = __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_INTR_INFO);
> +
> + if ( !cpu_has_vmx_virtual_intr_delivery )
> + return;
> +
> + if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> + nvmx->intr.source == hvm_intsrc_lapic &&
> + (intr_info & INTR_INFO_VALID_MASK) )
> + {
> + unsigned long status;
> + uint32_t rvi, ppr;
> + uint32_t vector = intr_info & 0xff;
> + struct vlapic *vlapic = vcpu_vlapic(v);
> +
> + WARN_ON(vector <= 0);

For both this ...

> +
> + vlapic_ack_pending_irq(v, vector, 1);
> +
> + ppr = vlapic_set_ppr(vlapic);
> + BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );

... and this: Is it guaranteed that the guest have no influence on
the participating values? Because otherwise either or both are
security issues.

It also looks inconsistent to me that the former is a WARN_ON()
while the latter is a BUG_ON().

Besides that I agree with all of Andrew's comments.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

yang.z.zhang at intel

Aug 10, 2013, 7:59 PM

Post #4 of 6 (8 views)
Permalink

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1 [In reply to]

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang [at] intel> wrote:
>> From: Yang Zhang <yang.z.zhang [at] Intel>
>>
>> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
>> But when L2 is running, external interrupt will casue L1 vmexit with
>> reason external interrupt. Then L1 will pick up the interrupt
>> through vmcs. So we need to update APIC-v related structure
>> accordingly;
>
> This doesn't sound logical to me: If L1 picks up the interrupt from
> VMCS, how can the be a reason/explanation for the need to update the
> APIC-v related data?
If L1 has the APIC-v, the interrupt is injected and acked by hardware normally. In this case, L1 picks up the interrupt from VMCS, but it didn't update the APIC-v related filed. Then when L1 issue the EOI, since SVI is wrong, the ISR will never be cleared. Also, if the PPR and RVI are wrong, the next interrupt handle logic will totally wrong.

>
>> +static void nvmx_update_apicv(struct vcpu *v) { + struct nestedvmx
>> *nvmx = &vcpu_2_nvmx(v); + struct nestedvcpu *nvcpu =
>> &vcpu_nestedhvm(v); + unsigned long reason =
>> __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); + uint32_t intr_info =
>> __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + + if (
>> !cpu_has_vmx_virtual_intr_delivery ) + return; + + if (
>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && +
>> nvmx->intr.source == hvm_intsrc_lapic && + (intr_info &
>> INTR_INFO_VALID_MASK) ) + { + unsigned long status; +
>> uint32_t rvi, ppr; + uint32_t vector = intr_info & 0xff; +
>> struct vlapic *vlapic = vcpu_vlapic(v); + + WARN_ON(vector <=
>> 0);
>
> For both this ...
>
>> +
>> + vlapic_ack_pending_irq(v, vector, 1);
>> +
>> + ppr = vlapic_set_ppr(vlapic);
>> + BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>
> ... and this: Is it guaranteed that the guest have no influence on the
> participating values? Because otherwise either or both are security issues.
>
> It also looks inconsistent to me that the former is a WARN_ON() while
> the latter is a BUG_ON().
If the vector is wrong, it is handled in L1 not L0, so just using WARN_ON here and L1 will handle it correctly. If PPR is wrong, then there should be somewhere wrong in L0's interrupt handle logic, so using BUG_ON.

>
> Besides that I agree with all of Andrew's comments.
>
> Jan

Best regards,
Yang

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

JBeulich at suse

Aug 11, 2013, 11:53 PM

Post #5 of 6 (3 views)
Permalink

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1 [In reply to]

>>> On 11.08.13 at 04:59, "Zhang, Yang Z" <yang.z.zhang [at] intel> wrote:
> Jan Beulich wrote on 2013-08-09:
>>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang [at] intel> wrote:
>>> From: Yang Zhang <yang.z.zhang [at] Intel>
>>>
>>> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
>>> But when L2 is running, external interrupt will casue L1 vmexit with
>>> reason external interrupt. Then L1 will pick up the interrupt
>>> through vmcs. So we need to update APIC-v related structure
>>> accordingly;
>>
>> This doesn't sound logical to me: If L1 picks up the interrupt from
>> VMCS, how can the be a reason/explanation for the need to update the
>> APIC-v related data?
> If L1 has the APIC-v, the interrupt is injected and acked by hardware
> normally. In this case, L1 picks up the interrupt from VMCS, but it didn't
> update the APIC-v related filed. Then when L1 issue the EOI, since SVI is
> wrong, the ISR will never be cleared. Also, if the PPR and RVI are wrong, the
> next interrupt handle logic will totally wrong.

Just saying the same with different wording doesn't really help
me in this case...

>>> +static void nvmx_update_apicv(struct vcpu *v) { + struct nestedvmx
>>> *nvmx = &vcpu_2_nvmx(v); + struct nestedvcpu *nvcpu =
>>> &vcpu_nestedhvm(v); + unsigned long reason =
>>> __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); + uint32_t intr_info =
>>> __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + + if (
>>> !cpu_has_vmx_virtual_intr_delivery ) + return; + + if (
>>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && +
>>> nvmx->intr.source == hvm_intsrc_lapic && + (intr_info &
>>> INTR_INFO_VALID_MASK) ) + { + unsigned long status; +
>>> uint32_t rvi, ppr; + uint32_t vector = intr_info & 0xff; +
>>> struct vlapic *vlapic = vcpu_vlapic(v); + + WARN_ON(vector <=
>>> 0);
>>
>> For both this ...
>>
>>> +
>>> + vlapic_ack_pending_irq(v, vector, 1);
>>> +
>>> + ppr = vlapic_set_ppr(vlapic);
>>> + BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>>
>> ... and this: Is it guaranteed that the guest have no influence on the
>> participating values? Because otherwise either or both are security issues.

You didn't reply to this at all.

>> It also looks inconsistent to me that the former is a WARN_ON() while
>> the latter is a BUG_ON().
> If the vector is wrong, it is handled in L1 not L0, so just using WARN_ON
> here and L1 will handle it correctly. If PPR is wrong, then there should be
> somewhere wrong in L0's interrupt handle logic, so using BUG_ON.

But you realize that if the warning triggers, it's almost guaranteed
that the BUG_ON() would also trigger. So I continue to not be
convinced.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

yang.z.zhang at intel

Aug 12, 2013, 6:08 PM

Post #6 of 6 (1 views)
Permalink

Re: [PATCH 6/7] Nested VMX: Update APIC-v(RVI/SVI) when vmexit to L1 [In reply to]

Jan Beulich wrote on 2013-08-12:
>>>> On 11.08.13 at 04:59, "Zhang, Yang Z" <yang.z.zhang [at] intel> wrote:
>> Jan Beulich wrote on 2013-08-09:
>>>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang [at] intel> wrote:
>>>> From: Yang Zhang <yang.z.zhang [at] Intel>
>>>>
>>>> If enabling APIC-v, all interrupts to L1 is delivered through APIC-v.
>>>> But when L2 is running, external interrupt will casue L1 vmexit
>>>> with reason external interrupt. Then L1 will pick up the interrupt
>>>> through vmcs. So we need to update APIC-v related structure
>>>> accordingly;
>>>
>>> This doesn't sound logical to me: If L1 picks up the interrupt from
>>> VMCS, how can the be a reason/explanation for the need to update
>>> the APIC-v related data?
>> If L1 has the APIC-v, the interrupt is injected and acked by
>> hardware normally. In this case, L1 picks up the interrupt from
>> VMCS, but it didn't update the APIC-v related filed. Then when L1
>> issue the EOI, since SVI is wrong, the ISR will never be cleared.
>> Also, if the PPR and RVI are wrong, the next interrupt handle logic will totally wrong.
>
> Just saying the same with different wording doesn't really help me in
> this case...
Consider the following case:
virtual vmexit happen;
L1 running and the vmexit reason is external interrupt;
L1 read vector info from vmcs and go to the interrupt handler;
Interrupt handler issues EOI to ack this interrupt;
Then APIC-v hardware will trap the EOI and do vEOI update: set VISR[SVI]=0, perform PPR update and deliver the next pending interrupt.

The problem is that, SVI is not set by hardware because the interrupt isn't delivered through APIC-v hardware. When software to ack the interrupt, it will never clear the vISR successfully because the wrong SVI. So we need to update the SVI/RVI/PPR to the right value just like the interrupt is delivered through APIC-v hardware to make sure the following vEOI update and vPPR update correctly.

>
>>>> +static void nvmx_update_apicv(struct vcpu *v) { + struct
>>>> nestedvmx *nvmx = &vcpu_2_nvmx(v); + struct nestedvcpu *nvcpu =
>>>> &vcpu_nestedhvm(v); + unsigned long reason =
>>>> __get_vvmcs(nvcpu->nv_vvmcx, VM_EXIT_REASON); + uint32_t intr_info
>>>> = __get_vvmcs(nvcpu->nv_vvmcx, +VM_EXIT_INTR_INFO); + + if (
>>>> !cpu_has_vmx_virtual_intr_delivery ) + return; + + if (
>>>> reason == EXIT_REASON_EXTERNAL_INTERRUPT && + nvmx->intr.source ==
>>>> hvm_intsrc_lapic && + (intr_info & INTR_INFO_VALID_MASK) )
>>>> + { + unsigned long status; + uint32_t rvi, ppr; +
>>>> uint32_t vector = intr_info & 0xff; +
>>>> struct vlapic *vlapic = vcpu_vlapic(v); + + WARN_ON(vector <=
>>>> 0);
>>>
>>> For both this ...
>>>
>>>> +
>>>> + vlapic_ack_pending_irq(v, vector, 1);
>>>> +
>>>> + ppr = vlapic_set_ppr(vlapic);
>>>> + BUG_ON( (ppr & 0xf0) != (vector & 0xf0) );
>>>
>>> ... and this: Is it guaranteed that the guest have no influence on
>>> the participating values? Because otherwise either or both are
>>> security
> issues.
>
> You didn't reply to this at all.
The intr_info is set by hypervisor only. If guest is able to write it, then it should be a bug in current Xen and may cause the security issue.

>>> It also looks inconsistent to me that the former is a WARN_ON()
>>> while the latter is a BUG_ON().
>> If the vector is wrong, it is handled in L1 not L0, so just using
>> WARN_ON here and L1 will handle it correctly. If PPR is wrong, then
>> there should be somewhere wrong in L0's interrupt handle logic, so
>> using
> BUG_ON.
>
> But you realize that if the warning triggers, it's almost guaranteed
> that the
> BUG_ON() would also trigger. So I continue to not be convinced.
You are right. The two will be triggered at same time. Also, I realized that the vector will never less than zero since I use uint32. So I will remove the WARN_ON.
As you mentioned, to avoid the security issue, it's better to use the WARN_ON instead the BUG_ON.

Best regards,
Yang

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads