Re: [Xen-devel] [PATCH v3 3/5] viridian: define type for the 'virtual VP assist page'

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 07 November 2018 11:49
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v3 3/5] viridian: define type for the
> 'virtual VP assist page'
> 
> On Wed, Nov 07, 2018 at 10:52:21AM +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@xxxxxxxxxx>
> > Reviewed-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Just one style nit below...
> 
> > ---
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >
> > v3:
> >  - Move the typedef of HV_VP_ASSIST_PAGE into viridian.h so that it can
> >    be used in the declaration of struct viridian_vcpu
> > ---
> >  xen/arch/x86/hvm/viridian/synic.c  | 52 +++++++++++++++++++++++--------
> -------
> >  xen/include/asm-x86/hvm/viridian.h |  4 ++-
> >  2 files changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/viridian/synic.c
> b/xen/arch/x86/hvm/viridian/synic.c
> > index 366608208f..d8d6f6e1c9 100644
> > --- a/xen/arch/x86/hvm/viridian/synic.c
> > +++ b/xen/arch/x86/hvm/viridian/synic.c
> > @@ -16,6 +16,18 @@
> >
> >  #include "private.h"
> >
> > +typedef struct _HV_VIRTUAL_APIC_ASSIST
> > +{
> > +    uint32_t no_eoi:1;
> > +    uint32_t reserved_zero:31;
> > +} HV_VIRTUAL_APIC_ASSIST;
> > +
> > +union _HV_VP_ASSIST_PAGE
> > +{
> > +    HV_VIRTUAL_APIC_ASSIST ApicAssist;
> > +    uint8_t ReservedZBytePadding[PAGE_SIZE];
> > +};
> 
> One struct uses CamelCase for fields while the other just uses
> underscores. I think it would be better to use a single style.
> 

That is deliberate. There is no struct definition in the spec. only the name. 
Thus the field names are ones I have invented based on the text, rather than 
being 'official'. I reserve CamelCase for the 'official' names :-)

  Paul

> Thanks, Roger.

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