From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1DC04C433F5 for ; Mon, 23 May 2022 17:19:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6gFt0fW8pHs0GOHJKqJXmPZto6y3JjMdwKA1mDaIL44=; b=3I/MlVYAtfmbp3 0sjLF2jYwJ4tD8zcxqVCpVmGyaJhvKz91rELdQFoXsNma8Q0qsRheLpue4x5E9WRbAInRS289O65u 8PvtPtzYq6UU4b94C6rULW2faAho6RQMucYmZtKf3K1oVvRDfrp85j/GWOV2OtGnbv0mlFIqAWXtB z3dMotdkbnu+PbH3MfyZV97xoBbmOEALKb2VD3ykBw3ejJTLEAH+vzQVjDhFBbRFdgSkpjYLTaQ9r ohc4We8i9IinzBlpNXQ45C2Gj2djgp2GwMu5ERTrmeRxhAAQrVSvaSIpwTxeFX88eVqOrVqIFQTlZ 11Or9qF1tAgp4vQf2NvQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ntBhC-005LdC-5x; Mon, 23 May 2022 17:18:18 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ntBh8-005LcA-RT for linux-arm-kernel@lists.infradead.org; Mon, 23 May 2022 17:18:16 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1A5031595; Mon, 23 May 2022 10:18:11 -0700 (PDT) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D7EC53F73D; Mon, 23 May 2022 10:18:09 -0700 (PDT) Date: Mon, 23 May 2022 18:18:07 +0100 From: Andre Przywara To: Alexandru Elisei Cc: will@kernel.org, julien.thierry.kdev@gmail.com, maz@kernel.org, suzuki.poulose@arm.com, julien@xen.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, james.morse@arm.com Subject: Re: [PATCH v2 kvmtool 02/12] builtin-run: Always use RAM size in bytes Message-ID: <20220523181807.07336f62@donnerap.cambridge.arm.com> In-Reply-To: <20220516155526.181694-3-alexandru.elisei@arm.com> References: <20220516155526.181694-1-alexandru.elisei@arm.com> <20220516155526.181694-3-alexandru.elisei@arm.com> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220523_101815_032306_28D12AFB X-CRM114-Status: GOOD ( 27.30 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 16 May 2022 16:55:16 +0100 Alexandru Elisei wrote: > The user can specify the virtual machine memory size in MB, which is saved > in cfg->ram_size. kvmtool validates it against the host memory size, > converted from bytes to MB. ram_size is then converted to bytes, and this > is how it is used throughout the rest of kvmtool. > > To avoid any confusion about the unit of measurement, especially once the > user is allowed to specify the unit of measurement, always use ram_size in > bytes. > > Signed-off-by: Alexandru Elisei > --- > builtin-run.c | 19 ++++++++++--------- > include/kvm/kvm-config.h | 7 ++++--- > include/kvm/kvm.h | 2 +- > 3 files changed, 15 insertions(+), 13 deletions(-) > > diff --git a/builtin-run.c b/builtin-run.c > index 0126c9fbcba6..236c59d2f86f 100644 > --- a/builtin-run.c > +++ b/builtin-run.c > @@ -36,6 +36,7 @@ > > #include > #include > +#include > > #include > #include > @@ -264,7 +265,7 @@ static u64 host_ram_size(void) > return 0; > } > > - return (nr_pages * page_size) >> MB_SHIFT; > + return nr_pages * page_size; So while looking for potential overflows on 32-bit systems, I realised that this was already broken: On my Calxeda Highbank (arm32) with 8GB of memory, this returns a negative number, because of the signed longs. Can you cast nr_pages to u64 while at it, to fix that? I don't think we need a separate patch for that ... > } > > /* > @@ -278,11 +279,11 @@ static u64 get_ram_size(int nr_cpus) > u64 available; > u64 ram_size; > > - ram_size = 64 * (nr_cpus + 3); > + ram_size = SZ_64M * (nr_cpus + 3); This overflows on 32-bit system starting with 29 vCPUs. If you use "3ULL", this problem goes away. Cheers, Andre > > available = host_ram_size() * RAM_SIZE_RATIO; > if (!available) > - available = MIN_RAM_SIZE_MB; > + available = MIN_RAM_SIZE; > > if (ram_size > available) > ram_size = available; > @@ -595,13 +596,13 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) > > if (!kvm->cfg.ram_size) > kvm->cfg.ram_size = get_ram_size(kvm->cfg.nrcpus); > + else > + kvm->cfg.ram_size <<= MB_SHIFT; > > if (kvm->cfg.ram_size > host_ram_size()) > pr_warning("Guest memory size %lluMB exceeds host physical RAM size %lluMB", > - (unsigned long long)kvm->cfg.ram_size, > - (unsigned long long)host_ram_size()); > - > - kvm->cfg.ram_size <<= MB_SHIFT; > + (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT, > + (unsigned long long)host_ram_size() >> MB_SHIFT); > > if (!kvm->cfg.dev) > kvm->cfg.dev = DEFAULT_KVM_DEV; > @@ -676,12 +677,12 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) > if (kvm->cfg.kernel_filename) { > printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME, > kvm->cfg.kernel_filename, > - (unsigned long long)kvm->cfg.ram_size / 1024 / 1024, > + (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT, > kvm->cfg.nrcpus, kvm->cfg.guest_name); > } else if (kvm->cfg.firmware_filename) { > printf(" # %s run --firmware %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME, > kvm->cfg.firmware_filename, > - (unsigned long long)kvm->cfg.ram_size / 1024 / 1024, > + (unsigned long long)kvm->cfg.ram_size >> MB_SHIFT, > kvm->cfg.nrcpus, kvm->cfg.guest_name); > } > > diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h > index 6a5720c4c7d4..31bc89520d52 100644 > --- a/include/kvm/kvm-config.h > +++ b/include/kvm/kvm-config.h > @@ -5,6 +5,8 @@ > #include "kvm/vfio.h" > #include "kvm/kvm-config-arch.h" > > +#include > + > #define DEFAULT_KVM_DEV "/dev/kvm" > #define DEFAULT_CONSOLE "serial" > #define DEFAULT_NETWORK "user" > @@ -15,14 +17,13 @@ > #define DEFAULT_SCRIPT "none" > #define DEFAULT_SANDBOX_FILENAME "guest/sandbox.sh" > > -#define MIN_RAM_SIZE_MB (64ULL) > -#define MIN_RAM_SIZE_BYTE (MIN_RAM_SIZE_MB << MB_SHIFT) > +#define MIN_RAM_SIZE SZ_64M > > struct kvm_config { > struct kvm_config_arch arch; > struct disk_image_params disk_image[MAX_DISK_IMAGES]; > struct vfio_device_params *vfio_devices; > - u64 ram_size; > + u64 ram_size; /* Guest memory size, in bytes */ > u8 num_net_devices; > u8 num_vfio_devices; > u64 vsock_cid; > diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h > index ad732e56f5ed..7b14b33b50ca 100644 > --- a/include/kvm/kvm.h > +++ b/include/kvm/kvm.h > @@ -87,7 +87,7 @@ struct kvm { > struct kvm_cpu **cpus; > > u32 mem_slots; /* for KVM_SET_USER_MEMORY_REGION */ > - u64 ram_size; > + u64 ram_size; /* Guest memory size, in bytes */ > void *ram_start; > u64 ram_pagesize; > struct mutex mem_banks_lock; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel