Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
On Thu, 29 Nov 2018, Vitaly Kuznetsov wrote: > Nadav Amit writes: > > >> On Nov 28, 2018, at 5:07 AM, Thomas Gleixner wrote: > >> > >> On Wed, 28 Nov 2018, Vitaly Kuznetsov wrote: > >> > >>> Nadav Amit writes: > >>> > On a different note: how come all of the hyper-v structs are not marked > with the “packed" attribute? > >>> > >>> "packed" should not be needed with proper padding; I vaguely remember > >>> someone (from x86@?) arguing _against_ "packed". > >> > >> Packed needs to be used, when describing fixed format data structures in > >> hardware or other ABIs, so the compiler cannot put alignment holes into > >> them. > >> > >> Using packed for generic data structures might result in suboptimal layouts > >> and prevents layout randomization. > > > > Right, I forgot about the structs randomization. So at least for it, the > > attribute should be needed. > > > > Not sure when randomization.s used but Hyper-V drivers will of course be > utterly broken with it. > > > To prevent conflicts, I think that this series should also add the > > attribute in a first patch, which would be tagged for stable. > > As the patchset doesn't add new definitions and as Paolo already queued > it I'd go with a follow-up patch adding "packed" to all hyperv-tlfs.h > structures. The question is how to avoid conflicts when Linus will be > merging this. We can do: > - Topic branch in kvm > - Send the patch to x86, make topic branch and reabse kvm > - Send the patch to kvm > - ... ? > > Paolo/Thomas, what would be your preference? As Paolo already has it, just route it through his tree please. Thanks, tglx
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
Nadav Amit writes: >> On Nov 28, 2018, at 5:07 AM, Thomas Gleixner wrote: >> >> On Wed, 28 Nov 2018, Vitaly Kuznetsov wrote: >> >>> Nadav Amit writes: >>> On a different note: how come all of the hyper-v structs are not marked with the “packed" attribute? >>> >>> "packed" should not be needed with proper padding; I vaguely remember >>> someone (from x86@?) arguing _against_ "packed". >> >> Packed needs to be used, when describing fixed format data structures in >> hardware or other ABIs, so the compiler cannot put alignment holes into >> them. >> >> Using packed for generic data structures might result in suboptimal layouts >> and prevents layout randomization. > > Right, I forgot about the structs randomization. So at least for it, the > attribute should be needed. > Not sure when randomization.s used but Hyper-V drivers will of course be utterly broken with it. > To prevent conflicts, I think that this series should also add the > attribute in a first patch, which would be tagged for stable. As the patchset doesn't add new definitions and as Paolo already queued it I'd go with a follow-up patch adding "packed" to all hyperv-tlfs.h structures. The question is how to avoid conflicts when Linus will be merging this. We can do: - Topic branch in kvm - Send the patch to x86, make topic branch and reabse kvm - Send the patch to kvm - ... ? Paolo/Thomas, what would be your preference? -- Vitaly
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
On Wed, Nov 28, 2018 at 02:07:42PM +0100, Thomas Gleixner wrote: > On Wed, 28 Nov 2018, Vitaly Kuznetsov wrote: > > > Nadav Amit writes: > > > > > > > > On a different note: how come all of the hyper-v structs are not marked > > > with the “packed" attribute? > > > > "packed" should not be needed with proper padding; I vaguely remember > > someone (from x86@?) arguing _against_ "packed". > > Packed needs to be used, when describing fixed format data structures in > hardware or other ABIs, so the compiler cannot put alignment holes into > them. > > Using packed for generic data structures might result in suboptimal layouts > and prevents layout randomization. Sorry for my illiteracy, I didn't watch this field closely so I had to google about layout randomization. Is my understanding correct that one can't rely any more on the compiler to naturally align the struct members with minimal padding? My life will never be the same... Roman.
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
> On Nov 28, 2018, at 5:07 AM, Thomas Gleixner wrote: > > On Wed, 28 Nov 2018, Vitaly Kuznetsov wrote: > >> Nadav Amit writes: >> >>> On a different note: how come all of the hyper-v structs are not marked >>> with the “packed" attribute? >> >> "packed" should not be needed with proper padding; I vaguely remember >> someone (from x86@?) arguing _against_ "packed". > > Packed needs to be used, when describing fixed format data structures in > hardware or other ABIs, so the compiler cannot put alignment holes into > them. > > Using packed for generic data structures might result in suboptimal layouts > and prevents layout randomization. Right, I forgot about the structs randomization. So at least for it, the attribute should be needed. To prevent conflicts, I think that this series should also add the attribute in a first patch, which would be tagged for stable.
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
On Wed, 28 Nov 2018, Vitaly Kuznetsov wrote: > Nadav Amit writes: > > > > > On a different note: how come all of the hyper-v structs are not marked > > with the “packed" attribute? > > "packed" should not be needed with proper padding; I vaguely remember > someone (from x86@?) arguing _against_ "packed". Packed needs to be used, when describing fixed format data structures in hardware or other ABIs, so the compiler cannot put alignment holes into them. Using packed for generic data structures might result in suboptimal layouts and prevents layout randomization. Thanks, tglx
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
Nadav Amit writes: > > On a different note: how come all of the hyper-v structs are not marked > with the “packed" attribute? "packed" should not be needed with proper padding; I vaguely remember someone (from x86@?) arguing _against_ "packed". -- Vitaly
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
On 27/11/18 19:48, Roman Kagan wrote: > On Tue, Nov 27, 2018 at 02:10:49PM +0100, Vitaly Kuznetsov wrote: >> Roman Kagan writes: >>> On Mon, Nov 26, 2018 at 04:47:29PM +0100, Vitaly Kuznetsov wrote: >>> 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? > > Strictly speaking bitwise ops are portable while bitfields are not, but > I guess this is not much of an issue with gcc which is dependable to > produce the right thing. > > I came to dislike the bitfields for the false feeling of atomicity of > assignment while most of the time they are read-modify-write operations. > > And no, I don't feel strong about it, so if nobody backs me on this I > give up :) I agree, but I am deferring to the Hyper-V maintainers. KVM is mostly reading these structs. Paolo
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
> On Nov 27, 2018, at 10:48 AM, Roman Kagan wrote: > > On Tue, Nov 27, 2018 at 02:10:49PM +0100, Vitaly Kuznetsov wrote: >> Roman Kagan writes: >>> On Mon, Nov 26, 2018 at 04:47:29PM +0100, Vitaly Kuznetsov wrote: >>> 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? > > Strictly speaking bitwise ops are portable while bitfields are not, but > I guess this is not much of an issue with gcc which is dependable to > produce the right thing. > > I came to dislike the bitfields for the false feeling of atomicity of > assignment while most of the time they are read-modify-write operations. > > And no, I don't feel strong about it, so if nobody backs me on this I > give up :) Last time I tried to push a change from bitmasks to bitfields I failed: https://lkml.org/lkml/2014/9/16/245 On a different note: how come all of the hyper-v structs are not marked with the “packed" attribute?
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
On Tue, Nov 27, 2018 at 02:10:49PM +0100, Vitaly Kuznetsov wrote: > Roman Kagan writes: > > On Mon, Nov 26, 2018 at 04:47:29PM +0100, Vitaly Kuznetsov wrote: > > 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? Strictly speaking bitwise ops are portable while bitfields are not, but I guess this is not much of an issue with gcc which is dependable to produce the right thing. I came to dislike the bitfields for the false feeling of atomicity of assignment while most of the time they are read-modify-write operations. And no, I don't feel strong about it, so if nobody backs me on this I give up :) Roman.
RE: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
Out of pure curiosity I decided to check what 'gcc -O3' produces when we
use bitfields and masks. As of 'gcc version 8.2.1 20181105 (Red Hat 8.2.1-5)
(GCC)'
1) bitfields:
struct abc {
int enabled:1;
int _pad:7;
int vec:8;
};
int is_good(struct abc *s) {
if (s->enabled)
return s->vec;
else
return 0;
}
results in
is_good:
xorl%eax, %eax
testb $1, (%rdi)
je .L1
movsbl 1(%rdi), %eax
.L1:
ret
2) masks
#include
#define S_ENABLED 1
#define S_VEC_MASK 0xff00
#define S_VEC_SHIFT 8
int is_good(uint16_t *s) {
if (*s & S_ENABLED)
return (*s & S_VEC_MASK) >> S_VEC_SHIFT;
else
return 0;
}
results in
is_good:
movzwl (%rdi), %edx
movzbl %dh, %eax
andl$1, %edx
movl$0, %edx
cmove %edx, %eax
ret
so bitfields version looks somewhat more efficient. I'm not sure if my
example is too synthetic though.
--
Vitaly
RE: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
From: Vitaly Kuznetsov 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
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
Roman Kagan 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
>> ---
>> 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
Re: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
[ 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
> ---
> 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).
Roman.
RE: [PATCH v2 1/4] x86/hyper-v: move synic/stimer control structures definitions to hyperv-tlfs.h
From: Vitaly Kuznetsov Sent: Monday, November 26, 2018 7:47 AM > > We implement Hyper-V SynIC and synthetic timers in KVM too so there's some > room for code sharing. > > Signed-off-by: Vitaly Kuznetsov > --- > 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(-) > Reviewed-by: Michael Kelley
