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

[Vitaly Kuznetsov]

Roman Kagan <rkagan@xxxxxxxxxxxxx> writes:

> [ Sorry for having missed v1 ]
>
> On Mon, Nov 26, 2018 at 04:47:29PM +0100, Vitaly Kuznetsov wrote:
>> We implement Hyper-V SynIC and synthetic timers in KVM too so there's some
>> room for code sharing.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/hyperv-tlfs.h | 69 ++++++++++++++++++++++++++++++
>>  drivers/hv/hv.c                    |  2 +-
>>  drivers/hv/hyperv_vmbus.h          | 68 -----------------------------
>>  3 files changed, 70 insertions(+), 69 deletions(-)
>> 
>> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
>> index 4139f7650fe5..b032c05cd3ee 100644
>> --- a/arch/x86/include/asm/hyperv-tlfs.h
>> +++ b/arch/x86/include/asm/hyperv-tlfs.h
>> @@ -731,6 +731,75 @@ struct hv_enlightened_vmcs {
>>  #define HV_STIMER_AUTOENABLE		(1ULL << 3)
>>  #define HV_STIMER_SINT(config)		(__u8)(((config) >> 16) & 0x0F)
>>  
>> +/* Define synthetic interrupt controller flag constants. */
>> +#define HV_EVENT_FLAGS_COUNT		(256 * 8)
>> +#define HV_EVENT_FLAGS_LONG_COUNT	(256 / sizeof(unsigned long))
>> +
>> +/*
>> + * Synthetic timer configuration.
>> + */
>> +union hv_stimer_config {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 enable:1;
>> +		u64 periodic:1;
>> +		u64 lazy:1;
>> +		u64 auto_enable:1;
>> +		u64 apic_vector:8;
>> +		u64 direct_mode:1;
>> +		u64 reserved_z0:3;
>> +		u64 sintx:4;
>> +		u64 reserved_z1:44;
>> +	};
>> +};
>> +
>> +
>> +/* Define the synthetic interrupt controller event flags format. */
>> +union hv_synic_event_flags {
>> +	unsigned long flags[HV_EVENT_FLAGS_LONG_COUNT];
>> +};
>> +
>> +/* Define SynIC control register. */
>> +union hv_synic_scontrol {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 enable:1;
>> +		u64 reserved:63;
>> +	};
>> +};
>> +
>> +/* Define synthetic interrupt source. */
>> +union hv_synic_sint {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 vector:8;
>> +		u64 reserved1:8;
>> +		u64 masked:1;
>> +		u64 auto_eoi:1;
>> +		u64 reserved2:46;
>> +	};
>> +};
>> +
>> +/* Define the format of the SIMP register */
>> +union hv_synic_simp {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 simp_enabled:1;
>> +		u64 preserved:11;
>> +		u64 base_simp_gpa:52;
>> +	};
>> +};
>> +
>> +/* Define the format of the SIEFP register */
>> +union hv_synic_siefp {
>> +	u64 as_uint64;
>> +	struct {
>> +		u64 siefp_enabled:1;
>> +		u64 preserved:11;
>> +		u64 base_siefp_gpa:52;
>> +	};
>> +};
>> +
>>  struct hv_vpset {
>>  	u64 format;
>>  	u64 valid_bank_mask;
>
> 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).

Thanks!

-- 
Vitaly