[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 7/8] viridian: define type for the 'virtual VP assist page'
>>> On 31.10.18 at 11:01, <Paul.Durrant@xxxxxxxxxx> wrote: >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx] >> Sent: 31 October 2018 09:42 >> >> >>> 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. I'm not overly concerned which way the above is coded; replacing the first uint32_t by bool would yield exactly the same code afaict, yet make it (even more) clear that this is a boolean field. The approach using a union and a (non-bitfield) "raw" member (which we use in a number of other places) has its own benefits, but if there's no need to ever access these values at their full width, the sticking to the non-union form is certainly fine. And in the end you're the maintainer of this code anyway ... Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |