On Thu, Apr 14, 2022 at 04:14:22PM +0200, Paolo Bonzini wrote:
> On 4/14/22 15:56, Sean Christopherson wrote:
> > > - return (pte[index[0]].pfn * vm->page_size) + (gva & 0xfffu);
> > > + return ((vm_paddr_t)pte[index[0]].pfn * vm->page_size) + (gva & 0xfffu);
> > This is but one of many paths that can get burned by pfn being 40 bits. The
> > most backport friendly fix is probably to add a pfn=>gpa helper and use that to
> > place the myriad "pfn * vm->page_size" instances.
> >
> > For a true long term solution, my vote is to do away with the bit field struct
> > and use #define'd masks and whatnot.
>
> Yes, bitfields larger than 32 bits are a mess.
It's very interesting to know this..
I just tried out with <32 bits bitfield and indeed gcc will behave
differently, by having the calculation done with 32bit (eax) rather than
64bit (rax).
The question is for >=32 bits it needs an extra masking instruction, while
that does not exist for the <32bits bitfield.
---8<---
#include <stdio.h>
struct test1 {
unsigned long a:${X};
unsigned long b:10;
};
int main(void)
{
struct test1 val;
val.a = 0x1234;
printf("0x%lx\n", val.a * 16);
return 0;
}
---8<---
When X=20:
0000000000401126 <main>:
401126: 55 push %rbp
401127: 48 89 e5 mov %rsp,%rbp
40112a: 48 83 ec 10 sub $0x10,%rsp
40112e: 8b 45 f8 mov -0x8(%rbp),%eax
401131: 25 00 00 f0 ff and $0xfff00000,%eax
401136: 0d 34 12 00 00 or $0x1234,%eax
40113b: 89 45 f8 mov %eax,-0x8(%rbp)
40113e: 8b 45 f8 mov -0x8(%rbp),%eax
401141: 25 ff ff 0f 00 and $0xfffff,%eax
401146: c1 e0 04 shl $0x4,%eax <----------- calculation (no further masking)
401149: 89 c6 mov %eax,%esi
40114b: bf 10 20 40 00 mov $0x402010,%edi
401150: b8 00 00 00 00 mov $0x0,%eax
401155: e8 d6 fe ff ff callq 401030 <printf@plt>
When X=40:
0000000000401126 <main>:
401126: 55 push %rbp
401127: 48 89 e5 mov %rsp,%rbp
40112a: 48 83 ec 10 sub $0x10,%rsp
40112e: 48 8b 45 f8 mov -0x8(%rbp),%rax
401132: 48 ba 00 00 00 00 00 movabs $0xffffff0000000000,%rdx
401139: ff ff ff
40113c: 48 21 d0 and %rdx,%rax
40113f: 48 0d 34 12 00 00 or $0x1234,%rax
401145: 48 89 45 f8 mov %rax,-0x8(%rbp)
401149: 48 b8 ff ff ff ff ff movabs $0xffffffffff,%rax
401150: 00 00 00
401153: 48 23 45 f8 and -0x8(%rbp),%rax
401157: 48 c1 e0 04 shl $0x4,%rax <------------ calculation
40115b: 48 ba ff ff ff ff ff movabs $0xffffffffff,%rdx
401162: 00 00 00
401165: 48 21 d0 and %rdx,%rax <------------ masking (again)
401168: 48 89 c6 mov %rax,%rsi
40116b: bf 10 20 40 00 mov $0x402010,%edi
401170: b8 00 00 00 00 mov $0x0,%eax
401175: e8 b6 fe ff ff callq 401030 <printf@plt>
That feels a bit less consistent to me, comparing to clang where at least
the behavior keeps the same for whatever length of bits in the bitfields.
Thanks,
--
Peter Xu
