Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of synthetic interrupt MSRs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.03.19 at 14:41, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -28,6 +29,32 @@ typedef union _HV_VP_ASSIST_PAGE
>      uint8_t ReservedZBytePadding[PAGE_SIZE];
>  } HV_VP_ASSIST_PAGE;
>  
> +typedef enum HV_MESSAGE_TYPE {
> +    HvMessageTypeNone,
> +    HvMessageTimerExpired = 0x80000010,
> +} HV_MESSAGE_TYPE;
> +
> +typedef struct HV_MESSAGE_FLAGS {
> +    uint8_t MessagePending:1;
> +    uint8_t Reserved:7;
> +} HV_MESSAGE_FLAGS;
> +
> +typedef struct HV_MESSAGE_HEADER {
> +    HV_MESSAGE_TYPE MessageType;
> +    uint16_t Reserved1;
> +    HV_MESSAGE_FLAGS MessageFlags;
> +    uint8_t PayloadSize;
> +    uint64_t Reserved2;
> +} HV_MESSAGE_HEADER;
> +
> +#define HV_MESSAGE_SIZE 256
> +#define HV_MESSAGE_MAX_PAYLOAD_QWORD_COUNT 30

Is this defined this way, or (given ...

> +typedef struct HV_MESSAGE {
> +    HV_MESSAGE_HEADER Header;
> +    uint64_t Payload[HV_MESSAGE_MAX_PAYLOAD_QWORD_COUNT];
> +} HV_MESSAGE;

... this) isn't it rather

#define HV_MESSAGE_MAX_PAYLOAD_QWORD_COUNT \
    ((HV_MESSAGE_SIZE - sizeof(HV_MESSAGE_HEADER) / 8)

> +    case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
> +    {
> +        unsigned int sintx = array_index_nospec(idx - HV_X64_MSR_SINT0,
> +                                                ARRAY_SIZE(vv->sint));

While here I can see the usefulness of using the local variable (but
you're aware of the remaining issue with going this route?), ...

> +    case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
> +    {
> +        unsigned int sintx = array_index_nospec(idx - HV_X64_MSR_SINT0,
> +                                                ARRAY_SIZE(vv->sint));
> +
> +        if ( !(viridian_feature_mask(d) & HVMPV_synic) )
> +            return X86EMUL_EXCEPTION;
> +
> +        *val = vv->sint[sintx].raw;
> +        break;

... I think you would better use array_access_nospec() here, to
avoid that very risk.

> +bool viridian_synic_is_auto_eoi_sint(const struct vcpu *v,
> +                                     unsigned int vector)
> +{
> +    const struct viridian_vcpu *vv = v->arch.hvm.viridian;
> +    unsigned int idx = vv->vector_to_sintx[vector];
> +    unsigned int sintx = array_index_nospec(idx, ARRAY_SIZE(vv->sint));
> +
> +    if ( idx >= ARRAY_SIZE(vv->sint) )
> +        return false;
> +
> +    return vv->sint[sintx].auto_eoi;

Same here then.

> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -461,10 +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);

Please use v->domain here.

> +    /* All synic SINTx vectors are edge triggered */
> +
>      if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>          vioapic_update_EOI(d, vector);
> +    else if ( has_viridian_synic(v->domain) )

And please use d here.

> @@ -1301,13 +1306,23 @@ 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.

synthetic

> @@ -1328,9 +1343,13 @@ int vlapic_has_pending_irq(struct vcpu *v)
>           (irr & 0xf0) <= (isr & 0xf0) )
>      {
>          viridian_apic_assist_clear(v);
> -        return -1;
> +        irr = -1;
>      }
>  
> +out:
> +    if (irr == -1)

Style: label indentation and spaces inside the parentheses.

> +        vlapic->polled_synic = false;

I'm struggling to understand the purpose of this flag, and the
situation is no helped by viridian_synic_poll_messages() currently
doing nothing. It would be really nice if maintenance of a flag like
this - if needed in the first place - could be kept local to Viridian
code (but of course not at the expense of adding various new
hooks for that purpose).

> --- 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;
> +    };
> +};
> +
>  struct viridian_vcpu
>  {
>      struct viridian_page vp_assist;
>      bool apic_assist_pending;
> +    uint64_t scontrol;
> +    uint64_t siefp;
> +    struct viridian_page simp;
> +    union viridian_sint_msr sint[16];
> +    uint8_t vector_to_sintx[256];

I've been wondering about the wasted initial 16 bytes here, but looking
at the code trying to save that space would apparently complicate some
of the array accesses in undue fashion.

> +    unsigned long msg_pending;

Does this really need to be unsigned long? There are only 16 bits used
here, and bitops ought to work fine on an unsigned int. Once reduced,
the field could then fill the gap between apic_assist_pending and scontrol.

Jan


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