Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com;
Date:   Wed, 20 Oct 2021 22:45:29 -0500
From:   Michael Roth <michael.roth@amd.com>
To:     Mingwei Zhang <mizhang@google.com>
CC:     <linux-kselftest@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, <x86@kernel.org>,
        Nathan Tempelman <natet@google.com>,
        Marc Orr <marcorr@google.com>,
        Steve Rutherford <srutherford@google.com>,
        Sean Christopherson <seanjc@google.com>,
        "Brijesh Singh" <brijesh.singh@amd.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "Varad Gautam" <varad.gautam@suse.com>,
        Shuah Khan <shuah@kernel.org>,
        "Vitaly Kuznetsov" <vkuznets@redhat.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        "Ricardo Koller" <ricarkol@google.com>,
        Jim Mattson <jmattson@google.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Joerg Roedel <joro@8bytes.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H . Peter Anvin" <hpa@zytor.com>
Subject: Re: [RFC 01/16] KVM: selftests: move vm_phy_pages_alloc() earlier in
 file
Message-ID: <20211021034529.gwv3hz5xhomtvnu7@amd.com>
References: <20211005234459.430873-1-michael.roth@amd.com>
 <20211005234459.430873-2-michael.roth@amd.com>
 <CAL715WK2toExGW7GGWGQyzhqBijMEhQfhamyb9_eZkrU=+LKnQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAL715WK2toExGW7GGWGQyzhqBijMEhQfhamyb9_eZkrU=+LKnQ@mail.gmail.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Oct 2021 03:47:09.5774
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: a55ce423-2f24-4b33-fbff-08d9944575a7
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT063.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB3182
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Oct 18, 2021 at 08:00:00AM -0700, Mingwei Zhang wrote:
> On Tue, Oct 5, 2021 at 4:46 PM Michael Roth <michael.roth@amd.com> wrote:
> >
> > Subsequent patches will break some of this code out into file-local
> > helper functions, which will be used by functions like vm_vaddr_alloc(),
> > which currently are defined earlier in the file, so a forward
> > declaration would be needed.
> >
> > Instead, move it earlier in the file, just above vm_vaddr_alloc() and
> > and friends, which are the main users.
> >
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  tools/testing/selftests/kvm/lib/kvm_util.c | 146 ++++++++++-----------
> >  1 file changed, 73 insertions(+), 73 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index 10a8ed691c66..92f59adddebe 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -1145,6 +1145,79 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
> >         list_add(&vcpu->list, &vm->vcpus);
> >  }
> >
> > +/*
> > + * Physical Contiguous Page Allocator
> > + *
> > + * Input Args:
> > + *   vm - Virtual Machine
> > + *   num - number of pages
> > + *   paddr_min - Physical address minimum
> > + *   memslot - Memory region to allocate page from
> > + *
> > + * Output Args: None
> > + *
> > + * Return:
> > + *   Starting physical address
> > + *
> > + * Within the VM specified by vm, locates a range of available physical
> > + * pages at or above paddr_min. If found, the pages are marked as in use
> > + * and their base address is returned. A TEST_ASSERT failure occurs if
> > + * not enough pages are available at or above paddr_min.
> > + */
> > +vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > +                             vm_paddr_t paddr_min, uint32_t memslot)
> > +{
> > +       struct userspace_mem_region *region;
> > +       sparsebit_idx_t pg, base;
> > +
> > +       TEST_ASSERT(num > 0, "Must allocate at least one page");
> > +
> > +       TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
> > +               "not divisible by page size.\n"
> > +               "  paddr_min: 0x%lx page_size: 0x%x",
> > +               paddr_min, vm->page_size);
> > +
> > +       region = memslot2region(vm, memslot);
> > +       base = pg = paddr_min >> vm->page_shift;
> > +
> > +       do {
> > +               for (; pg < base + num; ++pg) {
> > +                       if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
> > +                               base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
> > +                               break;
> > +                       }
> > +               }
> > +       } while (pg && pg != base + num);
> > +
> > +       if (pg == 0) {
> > +               fprintf(stderr, "No guest physical page available, "
> > +                       "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
> > +                       paddr_min, vm->page_size, memslot);
> > +               fputs("---- vm dump ----\n", stderr);
> > +               vm_dump(stderr, vm, 2);
> > +               abort();
> > +       }
> > +
> > +       for (pg = base; pg < base + num; ++pg)
> > +               sparsebit_clear(region->unused_phy_pages, pg);
> > +
> > +       return base * vm->page_size;
> > +}
> > +
> > +vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
> > +                            uint32_t memslot)
> > +{
> > +       return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
> > +}
> > +
> > +/* Arbitrary minimum physical address used for virtual translation tables. */
> > +#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000
> > +
> > +vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm)
> > +{
> > +       return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
> > +}
> > +
> >  /*
> >   * VM Virtual Address Unused Gap
> >   *
> > @@ -2149,79 +2222,6 @@ const char *exit_reason_str(unsigned int exit_reason)
> >         return "Unknown";
> >  }
> >
> > -/*
> > - * Physical Contiguous Page Allocator
> > - *
> > - * Input Args:
> > - *   vm - Virtual Machine
> > - *   num - number of pages
> > - *   paddr_min - Physical address minimum
> > - *   memslot - Memory region to allocate page from
> > - *
> > - * Output Args: None
> > - *
> > - * Return:
> > - *   Starting physical address
> > - *
> > - * Within the VM specified by vm, locates a range of available physical
> > - * pages at or above paddr_min. If found, the pages are marked as in use
> > - * and their base address is returned. A TEST_ASSERT failure occurs if
> > - * not enough pages are available at or above paddr_min.
> > - */
> > -vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > -                             vm_paddr_t paddr_min, uint32_t memslot)
> > -{
> > -       struct userspace_mem_region *region;
> > -       sparsebit_idx_t pg, base;
> > -
> > -       TEST_ASSERT(num > 0, "Must allocate at least one page");
> > -
> > -       TEST_ASSERT((paddr_min % vm->page_size) == 0, "Min physical address "
> > -               "not divisible by page size.\n"
> > -               "  paddr_min: 0x%lx page_size: 0x%x",
> > -               paddr_min, vm->page_size);
> > -
> > -       region = memslot2region(vm, memslot);
> > -       base = pg = paddr_min >> vm->page_shift;
> > -
> > -       do {
> > -               for (; pg < base + num; ++pg) {
> > -                       if (!sparsebit_is_set(region->unused_phy_pages, pg)) {
> > -                               base = pg = sparsebit_next_set(region->unused_phy_pages, pg);
> > -                               break;
> > -                       }
> > -               }
> > -       } while (pg && pg != base + num);
> > -
> > -       if (pg == 0) {
> > -               fprintf(stderr, "No guest physical page available, "
> > -                       "paddr_min: 0x%lx page_size: 0x%x memslot: %u\n",
> > -                       paddr_min, vm->page_size, memslot);
> > -               fputs("---- vm dump ----\n", stderr);
> > -               vm_dump(stderr, vm, 2);
> > -               abort();
> > -       }
> > -
> > -       for (pg = base; pg < base + num; ++pg)
> > -               sparsebit_clear(region->unused_phy_pages, pg);
> > -
> > -       return base * vm->page_size;
> > -}
> > -
> > -vm_paddr_t vm_phy_page_alloc(struct kvm_vm *vm, vm_paddr_t paddr_min,
> > -                            uint32_t memslot)
> > -{
> > -       return vm_phy_pages_alloc(vm, 1, paddr_min, memslot);
> > -}
> > -
> > -/* Arbitrary minimum physical address used for virtual translation tables. */
> > -#define KVM_GUEST_PAGE_TABLE_MIN_PADDR 0x180000
> > -
> > -vm_paddr_t vm_alloc_page_table(struct kvm_vm *vm)
> > -{
> > -       return vm_phy_page_alloc(vm, KVM_GUEST_PAGE_TABLE_MIN_PADDR, 0);
> > -}
> > -
> >  /*
> >   * Address Guest Virtual to Host Virtual
> >   *
> > --
> > 2.25.1
> >
> 
> Why move the function implementation? Maybe just adding a declaration
> at the top of kvm_util.c should suffice.

At least from working on other projects I'd gotten the impression that
forward function declarations should be avoided if they can be solved by
moving the function above the caller. Certainly don't mind taking your
suggestion and dropping this patch if that's not the case here though.