RE: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h

[Michael Kelley]

From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> Tuesday, November 27, 2018 5:11 AM

> > I personally tend to prefer masks over bitfields, so I'd rather do the
> > consolidation in the opposite direction: use the definitions in
> > hyperv-tlfs.h and replace those unions/bitfields elsewhere.  (I vaguely
> > remember posting such a patchset a couple of years ago but I lacked the
> > motivation to complete it).
> 
> Are there any known advantages of using masks over bitfields or the
> resulting binary code is the same? Assuming there are no I'd personally
> prefer bitfields - not sure why but to me e.g.
>  (stimer->config.enabled && !stimer->config.direct_mode)
>  looks nicer than
>  (stimer->config & HV_STIMER_ENABLED && !(stimer->config & HV_STIMER_DIRECT))
> 
> + there's no need to do shifts, e.g.
> 
>  vector = stimer->config.apic_vector
> 
>  looks cleaner that
> 
>  vector = (stimer->config & HV_STIMER_APICVECTOR_MASK) >>
> HV_STIMER_APICVECTOR_SHIFT
> 
> ... and we already use a few in hyperv-tlfs.h. I'm, however, flexible :-)
> 
> K. Y., Michael, Haiyang, Stephen - would you prefer to get rid of all
> bitfields from hyperv-tlfs.h? If so I can work on a patchset. As to this
> series, my understanding is that Paolo already queued it so in any case
> this is going to be a separate effort (unless there are strong
> objections of course).
> 

I prefer to keep the bit fields.  I concur think it makes the code more
readable.   Even if there is a modest performance benefit, at least
within a Linux guest most of the manipulation of the fields is during
initialization when performance doesn't matter.  But I can't speak to
KVM's implementation of the hypervisor side.

Michael