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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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;
        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