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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf Of Jan Beulich
> Sent: 13 March 2019 13:15
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim (Xen.org) <tim@xen.org>; Julien
> Grall <julien.grall@arm.com>; xen-devel <xen-devel@lists.xenproject.org>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of synthetic interrupt MSRs
> 
> >>> On 11.03.19 at 14:41, <paul.durrant@citrix.com> 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?), ...

I guess I'm not aware. Given that using sintx cannot lead to an out-of-bounds access, what is the risk?

> 
> > +    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.

Sorry, yes. I changed it and then apparently lost the change.

> 
> > @@ -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.

Oops.

> 
> > +        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).

The flag is there to stop viridian_synic_poll_messages() being called multiple times as you requested. I can move the flag into the viridian code but I'll have to add add extra call to unblock the poll in this case and in the ack function.

> 
> > --- 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.

Ok.

  Paul

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