Date:   Fri, 17 Feb 2023 18:57:54 +0000
In-Reply-To: <Y+1bom6CMFeNGWmm@google.com> (message from Sean Christopherson
 on Wed, 15 Feb 2023 14:24:34 -0800)
Mime-Version: 1.0
Message-ID: <diqzy1owjf0t.fsf@ackerleytng-cloudtop.c.googlers.com>
Subject: Re: [RFC PATCH v3 08/31] KVM: selftests: Require GCC to realign
 stacks on function entry
From:   Ackerley Tng <ackerleytng@google.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     erdemaktas@google.com, mail@maciej.szmigiero.name,
        linux-kselftest@vger.kernel.org, pbonzini@redhat.com,
        isaku.yamahata@intel.com, sagis@google.com, afranji@google.com,
        runanwang@google.com, shuah@kernel.org, drjones@redhat.com,
        maz@kernel.org, bgardon@google.com, jmattson@google.com,
        dmatlack@google.com, peterx@redhat.com, oupton@google.com,
        ricarkol@google.com, yang.zhong@intel.com, wei.w.wang@intel.com,
        xiaoyao.li@intel.com, pgonda@google.com, marcorr@google.com,
        eesposit@redhat.com, borntraeger@de.ibm.com, eric.auger@redhat.com,
        wangyanan55@huawei.com, aaronlewis@google.com, vkuznets@redhat.com,
        pshier@google.com, axelrasmussen@google.com,
        zhenzhong.duan@intel.com, like.xu@linux.intel.com,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"; format=flowed; delsp=yes
Precedence: bulk


> > I figured it out!
> >
> > GCC assumes that the stack is 16-byte aligned **before** the call
> > instruction. Since call pushes rip to the stack, GCC will compile code
> > assuming that on entrance to the function, the stack is -8 from a
> > 16-byte aligned address.
> >
> > Since for TDs we do a ljmp to guest code, providing a function's
> > address, the stack was not modified by a call instruction pushing rip to
> > the stack, so the stack is 16-byte aligned when the guest code starts
> > running, instead of 16-byte aligned -8 that GCC expects.
> >
> > For VMs, we set rip to a function pointer, and the VM starts running
> > with a 16-byte algined stack too.
> >
> > To fix this, I propose that in vm_arch_vcpu_add(), we align the
> > allocated stack address and then subtract 8 from that:
> >
> > @@ -573,10 +573,13 @@ struct kvm_vcpu *vm_arch_vcpu_add(struct kvm_vm  
> *vm,
> > uint32_t vcpu_id,
> >         vcpu_init_cpuid(vcpu, kvm_get_supported_cpuid());
> >         vcpu_setup(vm, vcpu);
> >
> > +       stack_vaddr += (DEFAULT_STACK_PGS * getpagesize());
> > +       stack_vaddr = ALIGN_DOWN(stack_vaddr, 16) - 8;

> The ALIGN_DOWN should be unnecessary, we've got larger issues if  
> getpagesize() isn't
> 16-byte aligned and/or if __vm_vaddr_alloc() returns anything but a  
> page-aligned
> address.  Maybe add a TEST_ASSERT() sanity check that stack_vaddr is  
> page-aligned
> at this point?

> And in addition to the comment suggested by Maciej, can you also add a  
> comment
> explaining the -8 adjust?  Yeah, someone can go read the changelog, but I  
> think
> this is worth explicitly documenting in code.

> Lastly, can you post it as a standalone patch?

> Many thanks!

Thanks Maciej and Sean, I've made the changes you requested and posted
it as a standalone patch at
https://lore.kernel.org/lkml/32866e5d00174697730d6231d2fb81f6b8d98c8a.1676659352.git.ackerleytng@google.com/