[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 6/9] viridian: add implementation of synthetic interrupt MSRs



>>> On 31.01.19 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -105,6 +132,73 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>              viridian_map_guest_page(d, &v->arch.hvm.viridian->vp_assist);
>          break;
>  
> +    case HV_X64_MSR_SCONTROL:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        v->arch.hvm.viridian->scontrol = val;
> +        break;
> +
> +    case HV_X64_MSR_SVERSION:
> +        return X86EMUL_EXCEPTION;
> +
> +    case HV_X64_MSR_SIEFP:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        v->arch.hvm.viridian->siefp = val;
> +        break;
> +
> +    case HV_X64_MSR_SIMP:
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        viridian_unmap_guest_page(&v->arch.hvm.viridian->simp);
> +        v->arch.hvm.viridian->simp.msr.raw = val;
> +        viridian_dump_guest_page(v, "SIMP", &v->arch.hvm.viridian->simp);
> +        if ( v->arch.hvm.viridian->simp.msr.fields.enabled )
> +            viridian_map_guest_page(d, &v->arch.hvm.viridian->simp);
> +        break;
> +
> +    case HV_X64_MSR_EOM:
> +    {
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        v->arch.hvm.viridian->msg_pending = 0;
> +        break;
> +    }
> +    case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:

Stray braces for the previous case, and a missing blank line between
both.

> +    {
> +        unsigned int sintx = idx - HV_X64_MSR_SINT0;
> +        uint8_t vector = v->arch.hvm.viridian->sint[sintx].fields.vector;
> +
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        /*
> +         * Invalidate any previous mapping by setting an out-of-range
> +         * index.
> +         */
> +        v->arch.hvm.viridian->vector_to_sintx[vector] =
> +            ARRAY_SIZE(v->arch.hvm.viridian->sint);
> +
> +        v->arch.hvm.viridian->sint[sintx].raw = val;
> +
> +        /* Vectors must be in the range 16-255 inclusive */
> +        vector = v->arch.hvm.viridian->sint[sintx].fields.vector;
> +        if ( vector < 16 )
> +            return X86EMUL_EXCEPTION;

The Viridian spec may surely specify architecturally inconsistent
behavior, but I'd like to double check that raising an exception
after having updated some state already is really intended here.

> +        printk(XENLOG_G_INFO "%pv: VIRIDIAN SINT%u: vector: %x\n", v, sintx,
> +               vector);
> +        v->arch.hvm.viridian->vector_to_sintx[vector] = sintx;
> +
> +        if ( v->arch.hvm.viridian->sint[sintx].fields.polling )
> +            clear_bit(sintx, &v->arch.hvm.viridian->msg_pending);
> +
> +        break;
> +    }
>      default:

Missing blank line above here again (and one more in the rdmsr code
below).

> @@ -116,6 +210,8 @@ int viridian_synic_wrmsr(struct vcpu *v, uint32_t idx, 
> uint64_t val)
>  
>  int viridian_synic_rdmsr(const struct vcpu *v, uint32_t idx, uint64_t *val)
>  {
> +    struct domain *d = v->domain;

const?

> +void viridian_synic_poll_messages(struct vcpu *v)

const ?

> +bool viridian_synic_is_auto_eoi_sint(struct vcpu *v, uint8_t vector)

const

> +{
> +    int sintx = v->arch.hvm.viridian->vector_to_sintx[vector];

I realize the array read from has uint8_t elements, but can this and ...

> +    if ( sintx >= ARRAY_SIZE(v->arch.hvm.viridian->sint) )
> +        return false;
> +
> +    return v->arch.hvm.viridian->sint[sintx].fields.auto_eoi;
> +}
> +
> +void viridian_synic_ack_sint(struct vcpu *v, uint8_t vector)
> +{
> +    int sintx = v->arch.hvm.viridian->vector_to_sintx[vector];

... this please still be unsigned int?

Also for both functions I wonder whether their first parameters
wouldn't better be struct viridian_vcpu *. This would certainly help
readability here.

> +    if ( sintx < ARRAY_SIZE(v->arch.hvm.viridian->sint) )
> +        clear_bit(sintx, &v->arch.hvm.viridian->msg_pending);

You also may want to use array_index_nospec() here and
array_access_nospec() above, despite there not being very big a
range to run past array bounds .

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -461,11 +461,15 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>  
>  void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>  {
> +    struct vcpu *v = vlapic_vcpu(vlapic);
>      struct domain *d = vlapic_domain(vlapic);
>  
>      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(d, vector);
>  
> +    if ( has_viridian_synic(v->domain) )

Please use d here. And could this be "else if()"?

> @@ -1301,6 +1305,13 @@ int vlapic_has_pending_irq(struct vcpu *v)
>      if ( !vlapic_enabled(vlapic) )
>          return -1;
>  
> +    /*
> +     * Poll the viridian message queues before checking the IRR since
> +     * a sythetic interrupt may be asserted during the poll.
> +     */
> +    if ( has_viridian_synic(v->domain) )
> +        viridian_synic_poll_messages(v);
> +
>      irr = vlapic_find_highest_irr(vlapic);
>      if ( irr == -1 )
>          return -1;

While architecturally IRR can indeed become set at any time, is it
acceptable to all of our other code for two successive invocations
to the function to potentially produce different results? I'm in
particular worried about {svm,vmx}_intr_assist(), and in their
context I also wonder whether you don't need a new
hvm_intsrc_* - it looks as if you use enough of the LAPIC to get
away without, but I'm not entirely certain.

> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -26,10 +26,30 @@ struct viridian_page
>      void *ptr;
>  };
>  
> +union viridian_sint_msr
> +{
> +    uint64_t raw;
> +    struct
> +    {
> +        uint64_t vector:8;
> +        uint64_t reserved_preserved1:8;
> +        uint64_t mask:1;
> +        uint64_t auto_eoi:1;
> +        uint64_t polling:1;
> +        uint64_t reserved_preserved2:45;
> +    } fields;

This being an internal header, does the inner struct really need
a field name?

Jan


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.