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

From: "Jan Beulich" <JBeulich@suse.com>
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>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Tim Deegan <tim@xen.org>,
	george.dunlap@citrix.com, Julien Grall <julien.grall@arm.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Ian Jackson <Ian.Jackson@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v5 09/11] viridian: add implementation of synthetic interrupt MSRs
Date: Thu, 14 Mar 2019 01:47:57 -0600	[thread overview]
Message-ID: <5C8A072D020000780021E636@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <88ff04a52bdb47bcb1687782933a330e@AMSPEX02CL02.citrite.net>

>>> On 13.03.19 at 17:13, <Paul.Durrant@citrix.com> wrote:
>>  -----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
>> 
> 
> 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.

> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel