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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2013-08-09:
>>>> On 09.08.13 at 10:49, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
>> From: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>> 
>> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel