Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 31 October 2018 09:42
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [Xen-devel] [PATCH 7/8] viridian: define type for the
> 'virtual VP assist page'
> 
> >>> On 31.10.18 at 10:27, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Roger Pau Monne
> >> Sent: 31 October 2018 08:54
> >>
> >> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
> >> > > From: Roger Pau Monne
> >> > > Sent: 30 October 2018 17:09
> >> > >
> >> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> >> > > > --- a/xen/arch/x86/hvm/viridian/synic.c
> >> > > > +++ b/xen/arch/x86/hvm/viridian/synic.c
> >> > > > @@ -13,6 +13,18 @@
> >> > > >  #include <asm/apic.h>
> >> > > >  #include <asm/hvm/support.h>
> >> > > >
> >> > > > +typedef struct _HV_VIRTUAL_APIC_ASSIST
> >> > > > +{
> >> > > > +    uint32_t no_eoi:1;
> >> > >
> >> > > Maybe bool:1 so you can use true/false?
> >> > >
> >> >
> >> > No, I'm very specifically using a 32-bit bitfield to match what the
> >> spec. says.
> >>
> >> Right, but no_eoi is a single flag on that bitfield, unless I'm
> >> missing something I think you could just use:
> >>
> >> typedef union _HV_VIRTUAL_APIC_ASSIST
> >> {
> >>     struct {
> >>         bool no_eoi:1;
> >>     } fields;
> >>     uint32_t raw;
> >> } HV_VIRTUAL_APIC_ASSIST;
> >>
> >> If you wish to access the raw value as a uint32_t while keeping access
> >> to individual flags easy. This union also has the advantage that
> >> adding new fields won't require you to adjust the size of the
> >> reserved_zero field.
> >>
> >
> > Agreed it's easier from a coding PoV, but I still prefer to stick with
> > bitfield definitions that span the full 32-bits to make it line up with
> the
> > spec. (currently section 10.3.5). If Microsoft had actually put a struct
> > definition there then I would use that, but as it is the layout
> illustration
> > is all there is and I want to match it as closely as I can.
> 
> I'm afraid I disagree with this view of yours: A field of the form
> "uint32_t x:1" does not reserve the following 31 bits. That's in
> part because types other than plain, signed, or unsigned int as
> well as bool aren't allowed by the base C standard anyway for
> bit fields; allowing them is a (quite common) compiler extension
> (and there are actually quirks when it comes to using types
> wider than int, but a bit count not specifying more bits than an
> int can hold). Just look at the resulting code of this example:
> 
> #include <stddef.h>
> #include <stdint.h>
> 
> struct s {
>       uint32_t x:1;

Yes, the offset of c is 1. But adding "uint32_t y:31;" here makes the offset 4. 
My definition was:

typedef struct _HV_VIRTUAL_APIC_ASSIST
{
    uint32_t no_eoi:1;
    uint32_t reserved_zero:31;
} HV_VIRTUAL_APIC_ASSIST;

... which I plan to stick with.

  Paul

>       char c;
> };
> 
> unsigned test(void) {
>       return offsetof(struct s, c);
> }
> 
> Jan
> 


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