> -----Original Message-----
> From: Xen-devel [mailto:[email protected]] On Behalf Of
> Jan Beulich
> Sent: 13 March 2019 13:15
> To: Paul Durrant <[email protected]>
> Cc: Stefano Stabellini <[email protected]>; Wei Liu
> <[email protected]>; Konrad Rzeszutek Wilk
> <[email protected]>; George Dunlap <[email protected]>; Andrew
> Cooper
> <[email protected]>; Ian Jackson <[email protected]>; Tim
> (Xen.org) <[email protected]>; Julien
> Grall <[email protected]>; xen-devel <[email protected]>;
> Roger Pau Monne
> <[email protected]>
> Subject: Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of
> synthetic interrupt MSRs
>
> >>> On 11.03.19 at 14:41, <[email protected]> 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
> [email protected]
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
