lkml.org 
[lkml]   [2019]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC 03/15] KVM: Add build-time error check on kvm_run size
    On Mon, Dec 02, 2019 at 03:53:15PM -0500, Peter Xu wrote:
    > On Mon, Dec 02, 2019 at 11:30:27AM -0800, Sean Christopherson wrote:
    > > On Fri, Nov 29, 2019 at 04:34:53PM -0500, Peter Xu wrote:
    > > > It's already going to reach 2400 Bytes (which is over half of page
    > > > size on 4K page archs), so maybe it's good to have this build-time
    > > > check in case it overflows when adding new fields.
    > >
    > > Please explain why exceeding PAGE_SIZE is a bad thing. I realize it's
    > > almost absurdly obvious when looking at the code, but a) the patch itself
    > > does not provide that context and b) the changelog should hold up on its
    > > own,
    >
    > Right, I'll enhance the commit message.
    >
    > > e.g. in a mostly hypothetical case where the allocation of vcpu->run
    > > were changed to something else.
    >
    > And that's why I added BUILD_BUG_ON right beneath that allocation. :)

    My point is that if the allocation were changed to no longer be a
    straightforward alloc_page() then someone reading the combined code would
    have no idea why the BUILD_BUG_ON() exists. It's a bit ridiculous for
    this case because the specific constraints of vcpu->run make it highly
    unlikely to use anything else, but that's beside the point.

    > It's just a helper for developers when adding new kvm_run fields, not
    > a risk for anyone who wants to start allocating more pages for it.

    But by adding a BUILD_BUG_ON without explaining *why*, you're placing an
    extra burden on someone that wants to increase the size of kvm->run, e.g.
    it's not at all obvious from the changelog whether this patch is adding
    the BUILD_BUG_ON purely because the code allocates memory for vcpu->run
    via alloc_page(), or if there is some fundamental aspect of vcpu->run that
    requires it to never span multiple pages.

    \
     
     \ /
      Last update: 2019-12-02 23:20    [W:3.424 / U:0.368 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site