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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 31 October 2018 08:54
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan Beulich
> <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> 'virtual VP assist page'
> 
> On Tue, Oct 30, 2018 at 05:11:30PM +0000, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Roger Pau Monne
> > > Sent: 30 October 2018 17:09
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> > > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Jan
> Beulich
> > > <jbeulich@suse.com>
> > > Subject: Re: [Xen-devel] [PATCH 7/8] viridian: define type for the
> > > 'virtual VP assist page'
> > >
> > > On Mon, Oct 29, 2018 at 06:02:10PM +0000, Paul Durrant wrote:
> > > > The specification [1] defines a type so we should use it, rather
> than
> > > just
> > > > OR-ing and AND-ing magic bits.
> > > >
> > > > No functional change.
> > > >
> > > > NOTE: The type defined in the specification does include an
> anonymous
> > > >       sub-struct in the page type but, as we currently use only the
> > > first
> > > >       element, the struct declaration has been omitted.
> > > >
> > > > [1] https://github.com/MicrosoftDocs/Virtualization-
> > >
> Documentation/raw/live/tlfs/Hypervisor%20Top%20Level%20Functional%20Specif
> > > ication%20v5.0C.pdf
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > ---
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Cc: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > >  xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++----
> ----
> > > -------
> > > >  xen/include/asm-x86/hvm/viridian.h |  2 +-
> > > >  2 files changed, 33 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> > > b/xen/arch/x86/hvm/viridian/synic.c
> > > > index 3f043f34ee..735c4c897c 100644
> > > > --- 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.

  Paul

> Thanks, Roger.

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