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
