Re: [Xen-devel] [v3 14/15] Update Posted-Interrupts Descriptor during vCPU scheduling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.15 at 07:18, <feng.wu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -6475,6 +6475,12 @@ enum hvm_intblk nhvm_interrupt_blocked(struct vcpu *v)
>      return hvm_funcs.nhvm_intr_blocked(v);
>  }
>  
> +void arch_pi_desc_update(struct vcpu *v, int old_state)
> +{
> +    if ( is_hvm_vcpu(v) && hvm_funcs.pi_desc_update )
> +        hvm_funcs.pi_desc_update(v, old_state);
> +}

Shouldn't this use has_hvm_container_vcpu()?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -168,6 +168,7 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>  
>      INIT_LIST_HEAD(&v->arch.hvm_vmx.pi_blocked_vcpu_list);
>  
> +    v->arch.hvm_vmx.pi_block_cpu = -1;

The field should be of unsigned type, at which point it might be better
(but isn't required) to store NR_CPUS here (and below), and check
against that value further down.

> @@ -1778,6 +1779,124 @@ static void vmx_handle_eoi(u8 vector)
>      __vmwrite(GUEST_INTR_STATUS, status);
>  }
>  
> +static void vmx_pi_desc_update(struct vcpu *v, int old_state)
> +{
> +    struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +    struct pi_desc old, new;
> +    unsigned long flags;
> +
> +    ASSERT(iommu_intpost);
> +
> +    switch ( v->runstate.state )
> +    {
> +    case RUNSTATE_runnable:
> +    case RUNSTATE_offline:
> +        /*
> +         * We don't need to send notification event to a non-running
> +         * vcpu, the interrupt information will be delivered to it before
> +         * VM-ENTRY when the vcpu is scheduled to run next time.
> +         */
> +        pi_set_sn(pi_desc);
> +
> +        /*
> +         * If the state is transferred from RUNSTATE_blocked,
> +         * we should set 'NV' feild back to posted_intr_vector,
> +         * so the Posted-Interrupts can be delivered to the vCPU
> +         * by VT-d HW after it is scheduled to run.
> +         */
> +        if ( old_state == RUNSTATE_blocked )
> +        {
> +            write_atomic((uint8_t*)&new.nv, posted_intr_vector);
> +
> +            /*
> +             * Delete the vCPU from the related block list
> +             * if we are resuming from blocked state
> +             */
> +            ASSERT(v->arch.hvm_vmx.pi_block_cpu != -1);
> +            spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock,
> +                              v->arch.hvm_vmx.pi_block_cpu), flags);
> +            list_del(&v->arch.hvm_vmx.pi_blocked_vcpu_list);
> +            spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_lock,
> +                                   v->arch.hvm_vmx.pi_block_cpu), flags);
> +            v->arch.hvm_vmx.pi_block_cpu = -1;
> +        }
> +        break;
> +
> +    case RUNSTATE_blocked:
> +        ASSERT(v->arch.hvm_vmx.pi_block_cpu == -1);
> +
> +        /*
> +         * The vCPU is blocked on the block list. Add the blocked
> +         * vCPU on the list of the v->arch.hvm_vmx.pi_block_cpu,
> +         * which is the destination of the wake-up notification event.
> +         */
> +        v->arch.hvm_vmx.pi_block_cpu = v->processor;
> +        spin_lock_irqsave(&per_cpu(pi_blocked_vcpu_lock,
> +                          v->arch.hvm_vmx.pi_block_cpu), flags);
> +        list_add_tail(&v->arch.hvm_vmx.pi_blocked_vcpu_list,
> +                      &per_cpu(pi_blocked_vcpu, 
> v->arch.hvm_vmx.pi_block_cpu));
> +        spin_unlock_irqrestore(&per_cpu(pi_blocked_vcpu_lock,
> +                               v->arch.hvm_vmx.pi_block_cpu), flags);
> +
> +        do {
> +            old.control = new.control = pi_desc->control;
> +
> +            /*
> +             * We should not block the vCPU if
> +             * an interrupt was posted for it.
> +             */
> +
> +            if ( old.on )
> +            {
> +                /*
> +                 * The vCPU will be removed from the block list
> +                 * during its state transferring from RUNSTATE_blocked
> +                 * to RUNSTATE_runnable after the following tasklet
> +                 * is executed.
> +                 */
> +                tasklet_schedule(&v->arch.hvm_vmx.pi_vcpu_wakeup_tasklet);
> +                return;

"break" here has the same effect and looks better overall (single
function return point).

> +            }
> +
> +            /*
> +             * Change the 'NDST' field to v->arch.hvm_vmx.pi_block_cpu,
> +             * so when external interrupts from assigned deivces happen,
> +             * wakeup notifiction event will go to
> +             * v->arch.hvm_vmx.pi_block_cpu, then in pi_wakeup_interrupt()
> +             * we can find the vCPU in the right list to wake up.
> +             */
> +            if ( x2apic_enabled )
> +                new.ndst = cpu_physical_id(v->arch.hvm_vmx.pi_block_cpu);
> +            else
> +                new.ndst = MASK_INSR(cpu_physical_id(
> +                                     v->arch.hvm_vmx.pi_block_cpu),
> +                                     PI_xAPIC_NDST_MASK);

This isn't the first time I see this - please add a helper inline function.

> +            new.sn = 0;
> +            new.nv = pi_wakeup_vector;
> +        } while ( cmpxchg(&pi_desc->control, old.control, new.control)
> +                  != old.control );
> +        break;
> +
> +    case RUNSTATE_running:
> +        ASSERT( pi_desc->sn == 1 );

Stray blanks.

> +        if ( x2apic_enabled )
> +            write_atomic(&new.ndst, cpu_physical_id(v->processor));
> +        else
> +            write_atomic(&new.ndst,
> +                         MASK_INSR(cpu_physical_id(v->processor),
> +                         PI_xAPIC_NDST_MASK));

Ah, another use case for that helper...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel