Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 14 Apr 2022 17:34:45 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Sean Christopherson <seanjc@google.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Ben Gardon <bgardon@google.com>,
        David Matlack <dmatlack@google.com>,
        Andrew Jones <drjones@redhat.com>
Subject: Re: [PATCH] kvm: selftests: Fix cut-off of addr_gva2gpa lookup
Message-ID: <YliTdb1LjfJoIcFc@xz-m1.local>
References: <20220414010703.72683-1-peterx@redhat.com>
 <Ylgn/Jw+FMIFqqc0@google.com>
 <bf15209d-2c50-9957-af24-c4f428f213b1@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <bf15209d-2c50-9957-af24-c4f428f213b1@redhat.com>
Precedence: bulk

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