Re: [Xen-devel] [RFC PATCH 7/9] x86/SVM: Add vcpu scheduling support for AVIC

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 07:52, <suravee.suthikulpanit@xxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/svm/avic.c
> +++ b/xen/arch/x86/hvm/svm/avic.c
> @@ -45,6 +45,83 @@ avic_get_phy_ait_entry(struct vcpu *v, int index)
>  }
>  
>  /***************************************************************
> + * AVIC VCPU SCHEDULING
> + */
> +static void avic_vcpu_load(struct vcpu *v)
> +{
> +    struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    int h_phy_apic_id;
> +    struct svm_avic_phy_ait_entry entry;
> +
> +    if ( !svm_avic || !s->avic_phy_id_cache )

Instead of adding checks of svm_avic inside the functions, please don't
install them in the first place when !svm_avic.

> +        return;
> +
> +    if ( test_bit(_VPF_blocked, &v->pause_flags) )
> +        return;
> +
> +    /* Note: APIC ID = 0xff is used for broadcast.
> +     *       APIC ID > 0xff is reserved.
> +     */
> +    h_phy_apic_id = cpu_data[v->processor].apicid;
> +    if ( h_phy_apic_id >= AVIC_PHY_APIC_ID_MAX )
> +        return;

Wouldn't such better be an ASSERT(), if at all? Without x2APIC
support I can't see how such a processor could be in use.

> +    entry = *(s->avic_phy_id_cache);
> +    smp_rmb();
> +    entry.host_phy_apic_id = h_phy_apic_id;
> +    entry.is_running = 1;
> +    *(s->avic_phy_id_cache) = entry;
> +    smp_wmb();
> +}
> +
> +static void avic_vcpu_put(struct vcpu *v)

May I recommend giving this and its counterpart a consistent pair of
names (put not really being the opposite of load)?

Jan


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