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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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