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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.05.18 at 23:07, <Janakarajan.Natarajan@xxxxxxx> wrote:
> @@ -180,6 +185,293 @@ 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.

Please utilize the permitted line length (also elsewhere).

> +         */
> +        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;
> +
> +        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;

Why do you break out of the loop here? With a shorthand more than
one vCPU might be the target.

> +            }
> +        }
> +        break;
> +    }
> +
> +    case AVIC_INCMP_IPI_ERR_INV_TARGET:
> +        gprintk(XENLOG_ERR,
> +                "SVM: %s: Invalid IPI target (icr=%#08x:%08x, idx=%u)\n",

%#08x produces something like 0x012345, rather than a full eight digits.
Preferably drop the #, or if you really think it's needed replace it be an
explicit 0x.

> +                __func__, icrh, icrl, index);

Please use __func__ only when a log message really can't be disambiguated
another way.

For both of these - same further down.

> +static avic_logical_id_entry_t *
> +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) )

I can't see a way for apic to be larger than 3 with the calculation above.

> +            return NULL;
> +        index = (cluster << 2) + apic;
> +    }
> +
> +    ASSERT(index <= 255);

Which of the many possible meanings of 255 is this?

> +static int avic_ldr_write(struct vcpu *v, u8 g_phy_id, u32 ldr, bool valid)
> +{
> +    avic_logical_id_entry_t *entry, new_entry;
> +    u32 dfr = vlapic_reg_read(vcpu_vlapic(v), APIC_DFR);

Just to give another example - looks like this too could be vlapic_get_reg().

> +static int avic_handle_ldr_update(struct vcpu *v)
> +{
> +    int ret = 0;

Pointless initializer.

> +    u32 ldr = vlapic_reg_read(vcpu_vlapic(v), APIC_LDR);
> +    u32 apic_id = vlapic_reg_read(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;

The comment says "last valid", but you may get here also when the
first avic_ldr_write() failed. I think you mean

    if ( !ret )
    ...
    else if ( v->arch.hvm_svm.avic_last_ldr )
    ...

> +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_reg_read(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;
> +
> +    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;

This default case is unnecessary.

> +void svm_avic_vmexit_do_noaccel(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *curr = current;
> +    struct vmcb_struct *vmcb = curr->arch.hvm_svm.vmcb;
> +    u32 offset = vmcb->exitinfo1 & 0xFF0;
> +    u32 rw = (vmcb->exitinfo1 >> 32) & 0x1;

bool?

> +    if ( avic_is_trap(offset) )
> +    {
> +        /* Handling AVIC Trap (intercept right after the access). */
> +        if ( !rw )
> +        {
> +            /*
> +             * If a read trap happens, the CPU microcode does not
> +             * implement the spec.
> +             */
> +            gprintk(XENLOG_ERR, "%s: Invalid #VMEXIT due to trap read 
> (%#x)\n",
> +                    __func__, offset);
> +            domain_crash(curr->domain);
> +        }
> +
> +        if ( avic_unaccel_trap_write(curr) != X86EMUL_OKAY )

ITYM "else if" here.

> +        {
> +            gprintk(XENLOG_ERR, "%s: Failed to handle trap write (%#x)\n",
> +                    __func__, offset);
> +            domain_crash(curr->domain);
> +        }
> +    }
> +    else
> +        /* Handling AVIC Fault (intercept before the access). */
> +        hvm_emulate_one_vm_event(EMUL_KIND_NORMAL, TRAP_invalid_op,
> +                                 X86_EVENT_NO_EC);

What's the rationale behind having chosen this function? I don't think it is
supposed to be called from outside the VM event code.

> +    return;
> +}

Please omit such redundant return statements.

Jan


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