Re: [Xen-devel] [PATCH v2 06/10] x86/SVM: Add AVIC vmexit handlers

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

> +
> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
> +{
> +    struct avic_log_apic_id_ent *entry, new_entry;
> +    u32 *bp = avic_get_bk_page_entry(v, APIC_DFR);

dfr would be a better name (and you use it in avic_handle_dfr_update()).

Also, 'logical' instead of 'log' in avic_log_apic_id_ent would be far
less confusing imo.

> +
> +    if ( !bp )
> +        return -EINVAL;
> +
> +    entry = avic_get_logical_id_entry(v, ldr, (*bp == APIC_DFR_FLAT));
> +    if (!entry)
> +        return -EINVAL;
> +
> +    new_entry = *entry;
> +    smp_rmb();
> +    new_entry.guest_phy_apic_id = g_phy_id;
> +    new_entry.valid = valid;
> +    *entry = new_entry;
> +    smp_wmb();
> +
> +    return 0;
> +}



> +
> +static int avic_handle_apic_id_update(struct vcpu *v, bool init)
> +{
> +    struct avic_phy_apic_id_ent *old, *new;
> +    struct arch_svm_struct *s = &v->arch.hvm_svm;
> +    u32 *apic_id_reg = avic_get_bk_page_entry(v, APIC_ID);
> +
> +    if ( !apic_id_reg )
> +        return -EINVAL;
> +
> +    old = s->avic_last_phy_id;
> +    ASSERT(old);
> +
> +    new = avic_get_phy_apic_id_ent(v, GET_APIC_PHYSICAL_ID(*apic_id_reg));
> +    if ( !new )
> +        return 0;
> +
> +    /* We need to move physical_id_entry to new offset */
> +    *new = *old;
> +    *((u64 *)old) = 0ULL;

This is pretty ugly. Can you define an invalid entry and assign it here
instead?

> +    s->avic_last_phy_id = new;
> +
> +    /*
> +     * Update the guest physical APIC ID in the logical
> +     * APIC ID table entry if LDR is already setup.
> +     */
> +    if ( v->arch.hvm_svm.avic_last_ldr )
> +        avic_handle_ldr_update(v);
> +
> +    return 0;
> +}
> +
> +static int avic_handle_dfr_update(struct vcpu *v)
> +{
> +    u32 mod;
> +    struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> +    u32 *dfr = avic_get_bk_page_entry(v, APIC_DFR);
> +
> +    if ( !dfr )
> +        return -EINVAL;
> +
> +    mod = (*dfr >> 28) & 0xFu;
> +
> +    spin_lock(&d->avic_ldr_mode_lock);
> +    if ( d->avic_ldr_mode != mod )
> +    {
> +        /*
> +         * We assume that all local APICs are using the same type.
> +         * If LDR mode changes, we need to flush the domain AVIC logical
> +         * APIC id table.
> +         */
> +        clear_domain_page(_mfn(d->avic_log_apic_id_table_mfn));
> +        smp_wmb();

Is this needed? I think clear_page() guarantees visibility/ordering (we
have SFENCE in clear_page_sse2() for this reason).


-boris


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