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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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