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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.08.13 at 04:59, "Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx> wrote:
> 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. 

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel