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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 14 March 2019 07:48
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper 
> <Andrew.Cooper3@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Roger Pau 
> Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>;
> xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; Konrad Rzeszutek Wilk 
> <konrad.wilk@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>
> Subject: RE: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of 
> synthetic interrupt MSRs
> 
> >>> On 13.03.19 at 17:13, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----Original Message-----
> >> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf 
> >> Of
> > Jan Beulich
> >> Sent: 13 March 2019 13:15
> >> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> >> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu 
> >> <wei.liu2@xxxxxxxxxx>;
> > Konrad Rzeszutek Wilk
> >> <konrad.wilk@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Andrew
> > Cooper
> >> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim 
> >> (Xen.org)
> > <tim@xxxxxxx>; Julien
> >> Grall <julien.grall@xxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>; 
> >> Roger
> > Pau Monne
> >> <roger.pau@xxxxxxxxxx>
> >> Subject: Re: [Xen-devel] [PATCH v5 09/11] viridian: add implementation of
> > synthetic interrupt MSRs
> >>
> >> >>> 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
> >>
> >
> > Missed this one before...
> >
> >> 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)
> >>
> >
> > I need the definition for the array in the struct so that sizeof(HV_MESSAGE)
> > == HV_MESSAGE_SIZE (for which there is a BUILD_BUG_ON()) later.
> 
> I don't understand this part - I'm not asking to ditch the #define.
> As to the BUILD_BUG_ON() - I see now, but that's only in patch
> 10, and in a specific message handler. I think this would belong
> here, and in the main viridian.c file.

Ok, I can see that it is more logical to co-locate the BUILD_BUG_ON() with the 
definitions.

  Paul

> 
> > It's also written that way in the spec. so I'd rather leave it as-is.
> 
> Well, okay then - I was sort of expecting this to be spelled out
> there in such a way. That doesn't change my overall opinion,
> but I can see your point of wanting to match the spec, and you
> being the maintainer I have no basis to insist anyway.
> 
> Jan
> 


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