Re: [Xen-devel] [PATCH 3/8] x86/SVM: Add AVIC vmexit handlers

["Natarajan, Janakarajan" <jnataraj@xxxxxxx>]

On 4/13/2018 12:48 PM, Andrew Cooper wrote:
> On 04/04/18 00:01, Janakarajan Natarajan wrote:
> > From: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> >
> > AVIC introduces two new #vmexit handlers:
> >
> > VMEXIT_INCOMP_IPI:
> > This occurs when an IPI could not be delivered to all targeted guest
> > virtual processors because at least one guest virtual processor
> > was not allocated to a physical core at the time. In this case,
> > Xen would try to emulate IPI.
> >
> > VMEXIT_DO_NOACCEL:
> > This occurs when a guest access to an APIC register that cannot be
> > accelerated by AVIC. In this case, Xen tries to emulate register accesses.
> >
> > This fault is also generated if an EOI is attempted when the highest priority
> > in-service interrupt is set for level-triggered mode.
> >
> > This patch also declare vlapic_read_aligned() and vlapic_reg_write()
> > as non-static to expose them to be used by AVIC.
> >
> > Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> > Signed-off-by: Janakarajan Natarajan <Janakarajan.Natarajan@xxxxxxx>
> > ---
> >   xen/arch/x86/hvm/svm/avic.c        | 296 +++++++++++++++++++++++++++++++++++++
> >   xen/arch/x86/hvm/svm/svm.c         |   8 +
> >   xen/arch/x86/hvm/vlapic.c          |   4 +-
> >   xen/include/asm-x86/hvm/svm/avic.h |   3 +
> >   xen/include/asm-x86/hvm/svm/vmcb.h |   6 +
> >   xen/include/asm-x86/hvm/vlapic.h   |   4 +
> >   6 files changed, 319 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/svm/avic.c b/xen/arch/x86/hvm/svm/avic.c
> > index 8108698911..e112469774 100644
> > --- a/xen/arch/x86/hvm/svm/avic.c
> > +++ b/xen/arch/x86/hvm/svm/avic.c
> > @@ -19,6 +19,7 @@
> >   #include <xen/sched.h>
> >   #include <xen/stdbool.h>
> >   #include <asm/acpi.h>
> > +#include <asm/apic.h>
> >   #include <asm/apicdef.h>
> >   #include <asm/atomic.h>
> >   #include <asm/event.h>
> > @@ -37,6 +38,8 @@
> >   
> >   #define AVIC_VAPIC_BAR_MASK     (((1ULL << 40) - 1) << PAGE_SHIFT)
> >   
> > +#define AVIC_UNACCEL_ACCESS_OFFSET_MASK    0xFF0
> > +
> >   /*
> >    * Note:
> >    * Currently, svm-avic mode is not supported with nested virtualization.
> > @@ -101,6 +104,8 @@ int svm_avic_dom_init(struct domain *d)
> >       d->arch.hvm_domain.svm.avic_physical_id_table_pg = pg;
> >       d->arch.hvm_domain.svm.avic_physical_id_table = 
> > __map_domain_page_global(pg);
> >   
> > +    spin_lock_init(&d->arch.hvm_domain.svm.avic_dfr_mode_lock);
> > +
> >       return ret;
> >    err_out:
> >       svm_avic_dom_destroy(d);
> > @@ -181,6 +186,297 @@ int svm_avic_init_vmcb(struct vcpu *v)
> >   }
> >   
> >   /*
> > + * Note:
> > + * This function handles the AVIC_INCOMP_IPI #vmexit when AVIC is enabled.
> > + * The hardware generates this fault when an IPI could not be delivered
> > + * to all targeted guest virtual processors because at least one guest
> > + * virtual processor was not allocated to a physical core at the time.
> > + */
> > +void svm_avic_vmexit_do_incomp_ipi(struct cpu_user_regs *regs)
> > +{
> > +    struct vcpu *curr = current;
> > +    struct domain *currd = curr->domain;
> > +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
> > +    u32 icrh = vmcb->exitinfo1 >> 32;
> > +    u32 icrl = vmcb->exitinfo1;
> > +    u32 id = vmcb->exitinfo2 >> 32;
> > +    u32 index = vmcb->exitinfo2 && 0xFF;
> > +
> > +    switch ( id )
> > +    {
> > +    case AVIC_INCMP_IPI_ERR_INVALID_INT_TYPE:
> > +        /*
> > +         * AVIC hardware handles the delivery of
> > +         * IPIs when the specified Message Type is Fixed
> > +         * (also known as fixed delivery mode) and
> > +         * the Trigger Mode is edge-triggered. The hardware
> > +         * also supports self and broadcast delivery modes
> > +         * specified via the Destination Shorthand(DSH)
> > +         * field of the ICRL. Logical and physical APIC ID
> > +         * formats are supported. All other IPI types cause
> > +         * a #VMEXIT, which needs to emulated.
> > +         */
> > +        vlapic_reg_write(curr, APIC_ICR2, icrh);
> > +        vlapic_reg_write(curr, APIC_ICR, icrl);
> > +        break;
> > +
> > +    case AVIC_INCMP_IPI_ERR_TARGET_NOT_RUN:
> > +    {
> > +        /*
> > +         * At this point, we expect that the AVIC HW has already
> > +         * set the appropriate IRR bits on the valid target
> > +         * vcpus. So, we just need to kick the appropriate vcpu.
> > +         */
> > +        struct vcpu *v;
> > +        uint32_t dest = GET_xAPIC_DEST_FIELD(icrh);
> > +        uint32_t short_hand = icrl & APIC_SHORT_MASK;
> > +        bool dest_mode = !!(icrl & APIC_DEST_MASK);
> No need for !!.  It is the explicit behaviour of the bool type.
>
> > +
> > +        for_each_vcpu ( currd,  v )
> > +        {
> > +            if ( v != curr &&
> > +                 vlapic_match_dest(vcpu_vlapic(v), vcpu_vlapic(curr),
> > +                                   short_hand, dest, dest_mode) )
> > +            {
> > +                vcpu_kick(v);
> > +                break;
> > +            }
> > +        }
> > +        break;
> > +    }
> > +
> > +    case AVIC_INCMP_IPI_ERR_INV_TARGET:
> > +        gprintk(XENLOG_ERR,
> > +                "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",
> > +                __func__, icrh, icrl, index);
> > +        domain_crash(currd);
> > +        break;
> > +
> > +    case AVIC_INCMP_IPI_ERR_INV_BK_PAGE:
> > +        gprintk(XENLOG_ERR,
> > +                "SVM: %s: Invalid bk page (icr=%#08x:%08x, idx=%u)\n",
> > +                __func__, icrh, icrl, index);
> > +        domain_crash(currd);
> > +        break;
> > +
> > +    default:
> > +        gprintk(XENLOG_ERR, "SVM: %s: Unknown IPI interception (%#x)\n",
> > +                __func__, id);
> > +        domain_crash(currd);
> > +    }
> > +}
> > +
> > +static struct avic_logical_id_entry *
> > +avic_get_logical_id_entry(struct svm_domain *d, u32 ldr, bool flat)
> > +{
> > +    unsigned int index;
> > +    unsigned int dest_id = GET_xAPIC_LOGICAL_ID(ldr);
> > +
> > +    if ( !dest_id )
> > +        return NULL;
> > +
> > +    if ( flat )
> > +    {
> > +        index = ffs(dest_id) - 1;
> > +        if ( index > 7 )
> > +            return NULL;
> > +    }
> > +    else
> > +    {
> > +        unsigned int cluster = (dest_id & 0xf0) >> 4;
> > +        int apic = ffs(dest_id & 0x0f) - 1;
> > +
> > +        if ( (apic < 0) || (apic > 7) || (cluster >= 0xf) )
> > +            return NULL;
> > +        index = (cluster << 2) + apic;
> > +    }
> > +
> > +    ASSERT(index <= 255);
> > +
> > +    return &d->avic_logical_id_table[index];
> > +}
> > +
> > +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
> > +{
> > +    struct avic_logical_id_entry *entry, new_entry;
> > +    u32 dfr = vlapic_read_aligned(vcpu_vlapic(v), APIC_DFR);
> > +
> > +    entry = avic_get_logical_id_entry(&v->domain->arch.hvm_domain.svm,
> > +                                      ldr, (dfr == APIC_DFR_FLAT));
> > +    if (!entry)
> 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();
> These barriers don't do what you want.  The pattern you are looking for
> would require an smp_mb() between setting valid and writing things
> back.  However, that is overkill - all that matters is that the compiler
> doesn't generate multiple partial updates.
>
> new_entry.raw = ACCESS_ONCE(entry->raw);
>
> new_entry.guest_phy_apic_id = g_phy_id;
> new_entry.valid = valid;
>
> ACCESS_ONCE(entry->raw) = new_entry.raw;
Since it was decided to not use

union ... {
        uint64_t raw;
        struct avic_logical_table_entry {
        ....
        ....
        };
};

would smp_mb() make sense here?

Thanks,
Janak

> > +
> > +    return 0;
> > +}
> > +
> > +static int avic_handle_ldr_update(struct vcpu *v)
> > +{
> > +    int ret = 0;
> > +    u32 ldr = vlapic_read_aligned(vcpu_vlapic(v), APIC_LDR);
> > +    u32 apic_id = vlapic_read_aligned(vcpu_vlapic(v), APIC_ID);
> > +
> > +    if ( !ldr )
> > +        return -EINVAL;
> > +
> > +    ret = avic_ldr_write(v, GET_xAPIC_ID(apic_id), ldr, true);
> > +    if ( ret && v->arch.hvm_svm.avic_last_ldr )
> > +    {
> > +        /*
> > +         * Note:
> > +         * In case of failure to update LDR register,
> > +         * we set the guest physical APIC ID to 0,
> > +         * and set the entry logical APID ID entry
> > +         * to invalid (false).
> > +         */
> > +        avic_ldr_write(v, 0, v->arch.hvm_svm.avic_last_ldr, false);
> > +        v->arch.hvm_svm.avic_last_ldr = 0;
> > +    }
> > +    else
> > +    {
> > +        /*
> > +         * Note:
> > +         * This saves the last valid LDR so that we
> > +         * know which entry in the local APIC ID
> > +         * to clean up when the LDR is updated.
> > +         */
> > +        v->arch.hvm_svm.avic_last_ldr = ldr;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static int avic_handle_dfr_update(struct vcpu *v)
> > +{
> > +    u32 mod;
> > +    struct svm_domain *d = &v->domain->arch.hvm_domain.svm;
> > +    u32 dfr = vlapic_read_aligned(vcpu_vlapic(v), APIC_DFR);
> > +
> > +    mod = (dfr >> 28) & 0xFu;
> > +
> > +    spin_lock(&d->avic_dfr_mode_lock);
> > +    if ( d->avic_dfr_mode != mod )
> > +    {
> > +        /*
> > +         * We assume that all local APICs are using the same type.
> > +         * If DFR mode changes, we need to flush the domain AVIC logical
> > +         * APIC id table.
> > +         */
> > +        clear_domain_page(_mfn(page_to_mfn(d->avic_logical_id_table_pg)));
> > +        d->avic_dfr_mode = mod;
> > +    }
> > +    spin_unlock(&d->avic_dfr_mode_lock);
> > +
> > +    if ( v->arch.hvm_svm.avic_last_ldr )
> > +        avic_handle_ldr_update(v);
> > +
> > +    return 0;
> > +}
> > +
> > +static int avic_unaccel_trap_write(struct vcpu *v)
> > +{
> > +    struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> > +    u32 offset = vmcb->exitinfo1 & AVIC_UNACCEL_ACCESS_OFFSET_MASK;
> > +    u32 reg = vlapic_read_aligned(vcpu_vlapic(v), offset);
> > +
> > +    switch ( offset )
> > +    {
> > +    case APIC_ID:
> > +        /*
> > +         * Currently, we do not support APIC_ID update while
> > +         * the vcpus are running, which might require updating
> > +         * AVIC max APIC ID in all VMCBs. This would require
> > +         * synchronize update on all running VCPUs.
> > +         */
> > +        return X86EMUL_UNHANDLEABLE;
> Newline please.
>
> > +    case APIC_LDR:
> > +        if ( avic_handle_ldr_update(v) )
> > +            return X86EMUL_UNHANDLEABLE;
> > +        break;
> > +
> > +    case APIC_DFR:
> > +        if ( avic_handle_dfr_update(v) )
> > +            return X86EMUL_UNHANDLEABLE;
> > +        break;
> > +
> > +    default:
> > +        break;
> > +    }
> > +
> > +    vlapic_reg_write(v, offset, reg);
> > +
> > +    return X86EMUL_OKAY;
> > +}
> > +
> > +static inline bool avic_is_trap(u32 offset)
> > +{
> > +    u32 pos = offset >> 4;
> > +    static const unsigned long avic_trap[] =
> > +        {
> > +#define REG(x) (1UL << (APIC_ ## x >> 4))
> > +            REG(ID)   | REG(EOI)     | REG(RRR)   | REG(LDR)  |
> > +            REG(DFR)  | REG(SPIV)    | REG(ESR)   | REG(ICR)  |
> > +            REG(LVTT) | REG(LVTTHMR) | REG(LVTPC) | REG(LVT0) |
> > +            REG(LVT1) | REG(LVTERR)  | REG(TMICT) | REG(TDCR)
> > +#undef REG
> > +        };
> I know I'm the author of the piece of code you've copied here, but I am
> in the process of trying to fix its style.
>
> static const unsigned long avic_trap[] = {
> #define REG(x) (1UL << (APIC_ ## x >> 4))
>      REG(ID)   | REG(EOI)     | REG(RRR)   | REG(LDR)  |
>      ....
> #undef REG
> };
>
> > +
> > +    if ( !test_bit(pos, avic_trap) )
> > +        return false;
> > +    return true;
> return pos < (sizeof(avic_trap) * 8) && test_bit(pos, avic_trap);
>
> You need to avoid reading beyond the end of avic_trap[].
>
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel