* [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:47 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter Alexandru Elisei
` (15 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara, Marc Zyngier,
Will Deacon
From: Suzuki K Poulose <suzuki.poulose@arm.com>
With 16K pagesize, the hugepage size is 32M. Align the guest
memory to the hugepagesize for 16K.
To query the host page size, we use sysconf(_SC_PAGESIZE) instead of
getpagesize, as suggested by man 2 getpagesize for portable applications.
Also use the sysconf function instead of getpagesize when setting
kvm->ram_pagesize.
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/kvm.c | 36 +++++++++++++++++++++++++++++-------
builtin-run.c | 4 ++--
util/util.c | 2 +-
3 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/arm/kvm.c b/arm/kvm.c
index 1f85fc60588f..1c5bdb8026bf 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -59,14 +59,33 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
{
+ unsigned long alignment;
+
/*
- * Allocate guest memory. We must align our buffer to 64K to
- * correlate with the maximum guest page size for virtio-mmio.
- * If using THP, then our minimal alignment becomes 2M.
- * 2M trumps 64K, so let's go with that.
+ * Allocate guest memory. If the user wants to use hugetlbfs, then the
+ * specified guest memory size must be a multiple of the host huge page
+ * size in order for the allocation to succeed. The mmap return adress
+ * is naturally aligned to the huge page size, so in this case we don't
+ * need to perform any alignment.
+ *
+ * Otherwise, we must align our buffer to 64K to correlate with the
+ * maximum guest page size for virtio-mmio. If using THP, then our
+ * minimal alignment becomes 2M with a 4K page size. With a 16K page
+ * size, the alignment becomes 32M. 32M and 2M trump 64K, so let's go
+ * with the largest alignment supported by the host.
*/
+ if (hugetlbfs_path) {
+ /* Don't do any alignment. */
+ alignment = 0;
+ } else {
+ if (sysconf(_SC_PAGESIZE) == SZ_16K)
+ alignment = SZ_32M;
+ else
+ alignment = SZ_2M;
+ }
+
kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
- kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
+ kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
kvm->arch.ram_alloc_size);
@@ -74,8 +93,11 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
die("Failed to map %lld bytes for guest memory (%d)",
kvm->arch.ram_alloc_size, errno);
- kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start,
- SZ_2M);
+ kvm->ram_start = kvm->arch.ram_alloc_start;
+ /* The result of aligning to 0 is 0. Let's avoid that. */
+ if (alignment)
+ kvm->ram_start = (void *)ALIGN((unsigned long)kvm->ram_start,
+ alignment);
madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
MADV_MERGEABLE);
diff --git a/builtin-run.c b/builtin-run.c
index f8dc6c7229b0..c867c8ba0892 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -127,8 +127,8 @@ void kvm_run_set_wrapper_sandbox(void)
"Run this script when booting into custom" \
" rootfs"), \
OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path", \
- "Hugetlbfs path"), \
- \
+ "Hugetlbfs path. Memory size must be a multiple"\
+ " of the huge page size"), \
OPT_GROUP("Kernel options:"), \
OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel", \
"Kernel to boot in virtual machine"), \
diff --git a/util/util.c b/util/util.c
index 1877105e3c08..217addd75e6f 100644
--- a/util/util.c
+++ b/util/util.c
@@ -127,7 +127,7 @@ void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 si
*/
return mmap_hugetlbfs(kvm, hugetlbfs_path, size);
else {
- kvm->ram_pagesize = getpagesize();
+ kvm->ram_pagesize = sysconf(_SC_PAGESIZE);
return mmap(NULL, size, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host
2019-09-23 13:35 ` [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host Alexandru Elisei
@ 2019-11-06 16:47 ` Andre Przywara
2019-11-06 17:29 ` Alexandru Elisei
0 siblings, 1 reply; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:47 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Julien Thierry, Suzuki Poulose, Julien Grall, Marc Zyngier,
Will Deacon
On Mon, 23 Sep 2019 14:35:07 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> With 16K pagesize, the hugepage size is 32M. Align the guest
> memory to the hugepagesize for 16K.
>
> To query the host page size, we use sysconf(_SC_PAGESIZE) instead of
> getpagesize, as suggested by man 2 getpagesize for portable applications.
> Also use the sysconf function instead of getpagesize when setting
> kvm->ram_pagesize.
>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/kvm.c | 36 +++++++++++++++++++++++++++++-------
> builtin-run.c | 4 ++--
> util/util.c | 2 +-
> 3 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 1f85fc60588f..1c5bdb8026bf 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -59,14 +59,33 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>
> void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> {
> + unsigned long alignment;
> +
> /*
> - * Allocate guest memory. We must align our buffer to 64K to
> - * correlate with the maximum guest page size for virtio-mmio.
> - * If using THP, then our minimal alignment becomes 2M.
> - * 2M trumps 64K, so let's go with that.
> + * Allocate guest memory. If the user wants to use hugetlbfs, then the
> + * specified guest memory size must be a multiple of the host huge page
> + * size in order for the allocation to succeed. The mmap return adress
> + * is naturally aligned to the huge page size, so in this case we don't
> + * need to perform any alignment.
> + *
> + * Otherwise, we must align our buffer to 64K to correlate with the
> + * maximum guest page size for virtio-mmio. If using THP, then our
> + * minimal alignment becomes 2M with a 4K page size. With a 16K page
> + * size, the alignment becomes 32M. 32M and 2M trump 64K, so let's go
> + * with the largest alignment supported by the host.
> */
> + if (hugetlbfs_path) {
> + /* Don't do any alignment. */
> + alignment = 0;
> + } else {
> + if (sysconf(_SC_PAGESIZE) == SZ_16K)
> + alignment = SZ_32M;
> + else
> + alignment = SZ_2M;
> + }
> +
> kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
> - kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
> + kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
So that means that on a 16K page size host we always allocate 32MB more memory than requested. In practise the pages before the new start should stay unpopulated, but I wonder if we should munmap that unused region before the new start?
Just thinking that people tend to use kvmtool because of its smaller memory footprint ...
Otherwise the code looks alright.
Cheers,
Andre.
> kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
> kvm->arch.ram_alloc_size);
>
> @@ -74,8 +93,11 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> die("Failed to map %lld bytes for guest memory (%d)",
> kvm->arch.ram_alloc_size, errno);
>
> - kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start,
> - SZ_2M);
> + kvm->ram_start = kvm->arch.ram_alloc_start;
> + /* The result of aligning to 0 is 0. Let's avoid that. */
> + if (alignment)
> + kvm->ram_start = (void *)ALIGN((unsigned long)kvm->ram_start,
> + alignment);
>
> madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
> MADV_MERGEABLE);
> diff --git a/builtin-run.c b/builtin-run.c
> index f8dc6c7229b0..c867c8ba0892 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -127,8 +127,8 @@ void kvm_run_set_wrapper_sandbox(void)
> "Run this script when booting into custom" \
> " rootfs"), \
> OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path", \
> - "Hugetlbfs path"), \
> - \
> + "Hugetlbfs path. Memory size must be a multiple"\
> + " of the huge page size"), \
> OPT_GROUP("Kernel options:"), \
> OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel", \
> "Kernel to boot in virtual machine"), \
> diff --git a/util/util.c b/util/util.c
> index 1877105e3c08..217addd75e6f 100644
> --- a/util/util.c
> +++ b/util/util.c
> @@ -127,7 +127,7 @@ void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 si
> */
> return mmap_hugetlbfs(kvm, hugetlbfs_path, size);
> else {
> - kvm->ram_pagesize = getpagesize();
> + kvm->ram_pagesize = sysconf(_SC_PAGESIZE);
> return mmap(NULL, size, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
> }
> }
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host
2019-11-06 16:47 ` Andre Przywara
@ 2019-11-06 17:29 ` Alexandru Elisei
0 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-11-06 17:29 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Julien Thierry, Suzuki Poulose, Julien Grall, Marc Zyngier,
Will Deacon
Hi,
On 11/6/19 4:47 PM, Andre Przywara wrote:
> On Mon, 23 Sep 2019 14:35:07 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> With 16K pagesize, the hugepage size is 32M. Align the guest
>> memory to the hugepagesize for 16K.
>>
>> To query the host page size, we use sysconf(_SC_PAGESIZE) instead of
>> getpagesize, as suggested by man 2 getpagesize for portable applications.
>> Also use the sysconf function instead of getpagesize when setting
>> kvm->ram_pagesize.
>>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Tested-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> arm/kvm.c | 36 +++++++++++++++++++++++++++++-------
>> builtin-run.c | 4 ++--
>> util/util.c | 2 +-
>> 3 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/arm/kvm.c b/arm/kvm.c
>> index 1f85fc60588f..1c5bdb8026bf 100644
>> --- a/arm/kvm.c
>> +++ b/arm/kvm.c
>> @@ -59,14 +59,33 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>>
>> void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> {
>> + unsigned long alignment;
>> +
>> /*
>> - * Allocate guest memory. We must align our buffer to 64K to
>> - * correlate with the maximum guest page size for virtio-mmio.
>> - * If using THP, then our minimal alignment becomes 2M.
>> - * 2M trumps 64K, so let's go with that.
>> + * Allocate guest memory. If the user wants to use hugetlbfs, then the
>> + * specified guest memory size must be a multiple of the host huge page
>> + * size in order for the allocation to succeed. The mmap return adress
>> + * is naturally aligned to the huge page size, so in this case we don't
>> + * need to perform any alignment.
>> + *
>> + * Otherwise, we must align our buffer to 64K to correlate with the
>> + * maximum guest page size for virtio-mmio. If using THP, then our
>> + * minimal alignment becomes 2M with a 4K page size. With a 16K page
>> + * size, the alignment becomes 32M. 32M and 2M trump 64K, so let's go
>> + * with the largest alignment supported by the host.
>> */
>> + if (hugetlbfs_path) {
>> + /* Don't do any alignment. */
>> + alignment = 0;
>> + } else {
>> + if (sysconf(_SC_PAGESIZE) == SZ_16K)
>> + alignment = SZ_32M;
>> + else
>> + alignment = SZ_2M;
>> + }
>> +
>> kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
>> - kvm->arch.ram_alloc_size = kvm->ram_size + SZ_2M;
>> + kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
> So that means that on a 16K page size host we always allocate 32MB more memory than requested. In practise the pages before the new start should stay unpopulated, but I wonder if we should munmap that unused region before the new start?
> Just thinking that people tend to use kvmtool because of its smaller memory footprint ...
I don't think it matters, kvmtool will not touch that region, so the process'
resident set size will stay the same.
Thanks,
Alex
> Otherwise the code looks alright.
>
> Cheers,
> Andre.
>
>> kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
>> kvm->arch.ram_alloc_size);
>>
>> @@ -74,8 +93,11 @@ void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> die("Failed to map %lld bytes for guest memory (%d)",
>> kvm->arch.ram_alloc_size, errno);
>>
>> - kvm->ram_start = (void *)ALIGN((unsigned long)kvm->arch.ram_alloc_start,
>> - SZ_2M);
>> + kvm->ram_start = kvm->arch.ram_alloc_start;
>> + /* The result of aligning to 0 is 0. Let's avoid that. */
>> + if (alignment)
>> + kvm->ram_start = (void *)ALIGN((unsigned long)kvm->ram_start,
>> + alignment);
>>
>> madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
>> MADV_MERGEABLE);
>> diff --git a/builtin-run.c b/builtin-run.c
>> index f8dc6c7229b0..c867c8ba0892 100644
>> --- a/builtin-run.c
>> +++ b/builtin-run.c
>> @@ -127,8 +127,8 @@ void kvm_run_set_wrapper_sandbox(void)
>> "Run this script when booting into custom" \
>> " rootfs"), \
>> OPT_STRING('\0', "hugetlbfs", &(cfg)->hugetlbfs_path, "path", \
>> - "Hugetlbfs path"), \
>> - \
>> + "Hugetlbfs path. Memory size must be a multiple"\
>> + " of the huge page size"), \
>> OPT_GROUP("Kernel options:"), \
>> OPT_STRING('k', "kernel", &(cfg)->kernel_filename, "kernel", \
>> "Kernel to boot in virtual machine"), \
>> diff --git a/util/util.c b/util/util.c
>> index 1877105e3c08..217addd75e6f 100644
>> --- a/util/util.c
>> +++ b/util/util.c
>> @@ -127,7 +127,7 @@ void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 si
>> */
>> return mmap_hugetlbfs(kvm, hugetlbfs_path, size);
>> else {
>> - kvm->ram_pagesize = getpagesize();
>> + kvm->ram_pagesize = sysconf(_SC_PAGESIZE);
>> return mmap(NULL, size, PROT_RW, MAP_ANON_NORESERVE, -1, 0);
>> }
>> }
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:47 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks Alexandru Elisei
` (14 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
From: Julien Grall <julien.grall@arm.com>
The structure KVM already contains a pointer to the configuration. Both
hugetlbfs_path and ram_size are part of the configuration, so is it not
necessary to path them again in parameter.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/kvm.c | 5 ++++-
include/kvm/kvm.h | 2 +-
kvm.c | 2 +-
mips/kvm.c | 5 ++++-
powerpc/kvm.c | 5 ++++-
x86/kvm.c | 5 ++++-
6 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/arm/kvm.c b/arm/kvm.c
index 1c5bdb8026bf..198cee5c0997 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
{
}
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
{
unsigned long alignment;
+ /* Convenience aliases */
+ u64 ram_size = kvm->cfg.ram_size;
+ const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
/*
* Allocate guest memory. If the user wants to use hugetlbfs, then the
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 7a738183d67a..635ce0f40b1e 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -140,7 +140,7 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
void kvm__remove_socket(const char *name);
void kvm__arch_set_cmdline(char *cmdline, bool video);
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
+void kvm__arch_init(struct kvm *kvm);
void kvm__arch_delete_ram(struct kvm *kvm);
int kvm__arch_setup_firmware(struct kvm *kvm);
int kvm__arch_free_firmware(struct kvm *kvm);
diff --git a/kvm.c b/kvm.c
index 57c4ff98ec4c..36b238791fc1 100644
--- a/kvm.c
+++ b/kvm.c
@@ -392,7 +392,7 @@ int kvm__init(struct kvm *kvm)
goto err_vm_fd;
}
- kvm__arch_init(kvm, kvm->cfg.hugetlbfs_path, kvm->cfg.ram_size);
+ kvm__arch_init(kvm);
INIT_LIST_HEAD(&kvm->mem_banks);
kvm__init_ram(kvm);
diff --git a/mips/kvm.c b/mips/kvm.c
index 211770da0d85..e2a0c63b14b8 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
}
/* Architecture-specific KVM init */
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
{
int ret;
+ /* Convenience aliases */
+ u64 ram_size = kvm->cfg.ram_size;
+ const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
kvm->ram_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path, ram_size);
kvm->ram_size = ram_size;
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 702d67dca614..034bc4608ad9 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -88,10 +88,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
}
/* Architecture-specific KVM init */
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
{
int cap_ppc_rma;
unsigned long hpt;
+ /* Convenience aliases */
+ u64 ram_size = kvm->cfg.ram_size;
+ const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
kvm->ram_size = ram_size;
diff --git a/x86/kvm.c b/x86/kvm.c
index 3e0f0b743f8c..5abb41e370bb 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -130,10 +130,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
}
/* Architecture-specific KVM init */
-void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
+void kvm__arch_init(struct kvm *kvm)
{
struct kvm_pit_config pit_config = { .flags = 0, };
int ret;
+ /* Convenience aliases */
+ u64 ram_size = kvm->cfg.ram_size;
+ const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
ret = ioctl(kvm->vm_fd, KVM_SET_TSS_ADDR, 0xfffbd000);
if (ret < 0)
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter
2019-09-23 13:35 ` [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter Alexandru Elisei
@ 2019-11-06 16:47 ` Andre Przywara
2019-11-07 10:03 ` Alexandru Elisei
0 siblings, 1 reply; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:47 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:08 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> From: Julien Grall <julien.grall@arm.com>
>
> The structure KVM already contains a pointer to the configuration. Both
> hugetlbfs_path and ram_size are part of the configuration, so is it not
> necessary to path them again in parameter.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/kvm.c | 5 ++++-
> include/kvm/kvm.h | 2 +-
> kvm.c | 2 +-
> mips/kvm.c | 5 ++++-
> powerpc/kvm.c | 5 ++++-
> x86/kvm.c | 5 ++++-
> 6 files changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 1c5bdb8026bf..198cee5c0997 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> {
> }
>
> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> +void kvm__arch_init(struct kvm *kvm)
> {
> unsigned long alignment;
> + /* Convenience aliases */
> + u64 ram_size = kvm->cfg.ram_size;
> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
Do we really need these aliases?
Definitely we should lose the comment ...
> /*
> * Allocate guest memory. If the user wants to use hugetlbfs, then the
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 7a738183d67a..635ce0f40b1e 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -140,7 +140,7 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
> void kvm__remove_socket(const char *name);
>
> void kvm__arch_set_cmdline(char *cmdline, bool video);
> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
> +void kvm__arch_init(struct kvm *kvm);
> void kvm__arch_delete_ram(struct kvm *kvm);
> int kvm__arch_setup_firmware(struct kvm *kvm);
> int kvm__arch_free_firmware(struct kvm *kvm);
> diff --git a/kvm.c b/kvm.c
> index 57c4ff98ec4c..36b238791fc1 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -392,7 +392,7 @@ int kvm__init(struct kvm *kvm)
> goto err_vm_fd;
> }
>
> - kvm__arch_init(kvm, kvm->cfg.hugetlbfs_path, kvm->cfg.ram_size);
> + kvm__arch_init(kvm);
>
> INIT_LIST_HEAD(&kvm->mem_banks);
> kvm__init_ram(kvm);
> diff --git a/mips/kvm.c b/mips/kvm.c
> index 211770da0d85..e2a0c63b14b8 100644
> --- a/mips/kvm.c
> +++ b/mips/kvm.c
> @@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> }
>
> /* Architecture-specific KVM init */
> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> +void kvm__arch_init(struct kvm *kvm)
> {
> int ret;
> + /* Convenience aliases */
> + u64 ram_size = kvm->cfg.ram_size;
> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
Same here, there is only one user of hugetlbfs_path.
> kvm->ram_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path, ram_size);
> kvm->ram_size = ram_size;
> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
> index 702d67dca614..034bc4608ad9 100644
> --- a/powerpc/kvm.c
> +++ b/powerpc/kvm.c
> @@ -88,10 +88,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> }
>
> /* Architecture-specific KVM init */
> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> +void kvm__arch_init(struct kvm *kvm)
> {
> int cap_ppc_rma;
> unsigned long hpt;
> + /* Convenience aliases */
Not needed and not even true for hugetlbfs_path.
> + u64 ram_size = kvm->cfg.ram_size;
This is somewhat pointless, the only user is right below the next line ...
> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
We need to keep that, though, as the code modifies it.
>
> kvm->ram_size = ram_size;
>
> diff --git a/x86/kvm.c b/x86/kvm.c
> index 3e0f0b743f8c..5abb41e370bb 100644
> --- a/x86/kvm.c
> +++ b/x86/kvm.c
> @@ -130,10 +130,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> }
>
> /* Architecture-specific KVM init */
> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
> +void kvm__arch_init(struct kvm *kvm)
> {
> struct kvm_pit_config pit_config = { .flags = 0, };
> int ret;
> + /* Convenience aliases */
I don't think the comment is needed.
Otherwise it's a nice cleanup.
Cheers,
Andre
> + u64 ram_size = kvm->cfg.ram_size;
> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
>
> ret = ioctl(kvm->vm_fd, KVM_SET_TSS_ADDR, 0xfffbd000);
> if (ret < 0)
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter
2019-11-06 16:47 ` Andre Przywara
@ 2019-11-07 10:03 ` Alexandru Elisei
0 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-11-07 10:03 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
Hi,
On 11/6/19 4:47 PM, Andre Przywara wrote:
> On Mon, 23 Sep 2019 14:35:08 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> From: Julien Grall <julien.grall@arm.com>
>>
>> The structure KVM already contains a pointer to the configuration. Both
>> hugetlbfs_path and ram_size are part of the configuration, so is it not
>> necessary to path them again in parameter.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>> arm/kvm.c | 5 ++++-
>> include/kvm/kvm.h | 2 +-
>> kvm.c | 2 +-
>> mips/kvm.c | 5 ++++-
>> powerpc/kvm.c | 5 ++++-
>> x86/kvm.c | 5 ++++-
>> 6 files changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/arm/kvm.c b/arm/kvm.c
>> index 1c5bdb8026bf..198cee5c0997 100644
>> --- a/arm/kvm.c
>> +++ b/arm/kvm.c
>> @@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> {
>> }
>>
>> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> +void kvm__arch_init(struct kvm *kvm)
>> {
>> unsigned long alignment;
>> + /* Convenience aliases */
>> + u64 ram_size = kvm->cfg.ram_size;
>> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
> Do we really need these aliases?
> Definitely we should lose the comment ...
I agree, they need to go.
Thanks,
Alex
>
>> /*
>> * Allocate guest memory. If the user wants to use hugetlbfs, then the
>> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
>> index 7a738183d67a..635ce0f40b1e 100644
>> --- a/include/kvm/kvm.h
>> +++ b/include/kvm/kvm.h
>> @@ -140,7 +140,7 @@ int kvm__enumerate_instances(int (*callback)(const char *name, int pid));
>> void kvm__remove_socket(const char *name);
>>
>> void kvm__arch_set_cmdline(char *cmdline, bool video);
>> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size);
>> +void kvm__arch_init(struct kvm *kvm);
>> void kvm__arch_delete_ram(struct kvm *kvm);
>> int kvm__arch_setup_firmware(struct kvm *kvm);
>> int kvm__arch_free_firmware(struct kvm *kvm);
>> diff --git a/kvm.c b/kvm.c
>> index 57c4ff98ec4c..36b238791fc1 100644
>> --- a/kvm.c
>> +++ b/kvm.c
>> @@ -392,7 +392,7 @@ int kvm__init(struct kvm *kvm)
>> goto err_vm_fd;
>> }
>>
>> - kvm__arch_init(kvm, kvm->cfg.hugetlbfs_path, kvm->cfg.ram_size);
>> + kvm__arch_init(kvm);
>>
>> INIT_LIST_HEAD(&kvm->mem_banks);
>> kvm__init_ram(kvm);
>> diff --git a/mips/kvm.c b/mips/kvm.c
>> index 211770da0d85..e2a0c63b14b8 100644
>> --- a/mips/kvm.c
>> +++ b/mips/kvm.c
>> @@ -57,9 +57,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> }
>>
>> /* Architecture-specific KVM init */
>> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> +void kvm__arch_init(struct kvm *kvm)
>> {
>> int ret;
>> + /* Convenience aliases */
>> + u64 ram_size = kvm->cfg.ram_size;
>> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
> Same here, there is only one user of hugetlbfs_path.
>
>> kvm->ram_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path, ram_size);
>> kvm->ram_size = ram_size;
>> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
>> index 702d67dca614..034bc4608ad9 100644
>> --- a/powerpc/kvm.c
>> +++ b/powerpc/kvm.c
>> @@ -88,10 +88,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> }
>>
>> /* Architecture-specific KVM init */
>> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> +void kvm__arch_init(struct kvm *kvm)
>> {
>> int cap_ppc_rma;
>> unsigned long hpt;
>> + /* Convenience aliases */
> Not needed and not even true for hugetlbfs_path.
>
>> + u64 ram_size = kvm->cfg.ram_size;
> This is somewhat pointless, the only user is right below the next line ...
>
>> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
> We need to keep that, though, as the code modifies it.
>
>>
>> kvm->ram_size = ram_size;
>>
>> diff --git a/x86/kvm.c b/x86/kvm.c
>> index 3e0f0b743f8c..5abb41e370bb 100644
>> --- a/x86/kvm.c
>> +++ b/x86/kvm.c
>> @@ -130,10 +130,13 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> }
>>
>> /* Architecture-specific KVM init */
>> -void kvm__arch_init(struct kvm *kvm, const char *hugetlbfs_path, u64 ram_size)
>> +void kvm__arch_init(struct kvm *kvm)
>> {
>> struct kvm_pit_config pit_config = { .flags = 0, };
>> int ret;
>> + /* Convenience aliases */
> I don't think the comment is needed.
>
> Otherwise it's a nice cleanup.
>
> Cheers,
> Andre
>
>> + u64 ram_size = kvm->cfg.ram_size;
>> + const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
>>
>> ret = ioctl(kvm->vm_fd, KVM_SET_TSS_ADDR, 0xfffbd000);
>> if (ret < 0)
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 01/16] arm: Allow use of hugepage with 16K pagesize host Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 02/16] kvm__arch_init: Don't pass hugetlbfs_path and ram_size in parameter Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara
2020-02-05 18:07 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration Alexandru Elisei
` (13 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
From: Julien Grall <julien.grall@arm.com>
At the moment, virtio scsi registers only one bank starting at 0. On some
architectures (like on x86, for example), this may not be true and the
guest may have multiple memory regions.
Register all the memory regions to vhost by browsing kvm->mem_banks. The
code is based on the virtio_net__vhost_init implementation.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
virtio/scsi.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/virtio/scsi.c b/virtio/scsi.c
index a72bb2a9a206..63fc4f4635a2 100644
--- a/virtio/scsi.c
+++ b/virtio/scsi.c
@@ -190,24 +190,29 @@ static struct virtio_ops scsi_dev_virtio_ops = {
static void virtio_scsi_vhost_init(struct kvm *kvm, struct scsi_dev *sdev)
{
+ struct kvm_mem_bank *bank;
struct vhost_memory *mem;
u64 features;
- int r;
+ int r, i;
sdev->vhost_fd = open("/dev/vhost-scsi", O_RDWR);
if (sdev->vhost_fd < 0)
die_perror("Failed openning vhost-scsi device");
- mem = calloc(1, sizeof(*mem) + sizeof(struct vhost_memory_region));
+ mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
if (mem == NULL)
die("Failed allocating memory for vhost memory map");
- mem->nregions = 1;
- mem->regions[0] = (struct vhost_memory_region) {
- .guest_phys_addr = 0,
- .memory_size = kvm->ram_size,
- .userspace_addr = (unsigned long)kvm->ram_start,
- };
+ i = 0;
+ list_for_each_entry(bank, &kvm->mem_banks, list) {
+ mem->regions[i] = (struct vhost_memory_region) {
+ .guest_phys_addr = bank->guest_phys_addr,
+ .memory_size = bank->size,
+ .userspace_addr = (unsigned long)bank->host_addr,
+ };
+ i++;
+ }
+ mem->nregions = i;
r = ioctl(sdev->vhost_fd, VHOST_SET_OWNER);
if (r != 0)
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks
2019-09-23 13:35 ` [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks Alexandru Elisei
@ 2019-11-06 16:48 ` Andre Przywara
2020-02-05 18:07 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:48 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:09 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> From: Julien Grall <julien.grall@arm.com>
>
> At the moment, virtio scsi registers only one bank starting at 0. On some
> architectures (like on x86, for example), this may not be true and the
> guest may have multiple memory regions.
>
> Register all the memory regions to vhost by browsing kvm->mem_banks. The
> code is based on the virtio_net__vhost_init implementation.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> virtio/scsi.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index a72bb2a9a206..63fc4f4635a2 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -190,24 +190,29 @@ static struct virtio_ops scsi_dev_virtio_ops = {
>
> static void virtio_scsi_vhost_init(struct kvm *kvm, struct scsi_dev *sdev)
> {
> + struct kvm_mem_bank *bank;
> struct vhost_memory *mem;
> u64 features;
> - int r;
> + int r, i;
>
> sdev->vhost_fd = open("/dev/vhost-scsi", O_RDWR);
> if (sdev->vhost_fd < 0)
> die_perror("Failed openning vhost-scsi device");
>
> - mem = calloc(1, sizeof(*mem) + sizeof(struct vhost_memory_region));
> + mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
> if (mem == NULL)
> die("Failed allocating memory for vhost memory map");
>
> - mem->nregions = 1;
> - mem->regions[0] = (struct vhost_memory_region) {
> - .guest_phys_addr = 0,
> - .memory_size = kvm->ram_size,
> - .userspace_addr = (unsigned long)kvm->ram_start,
> - };
> + i = 0;
> + list_for_each_entry(bank, &kvm->mem_banks, list) {
> + mem->regions[i] = (struct vhost_memory_region) {
> + .guest_phys_addr = bank->guest_phys_addr,
> + .memory_size = bank->size,
> + .userspace_addr = (unsigned long)bank->host_addr,
> + };
> + i++;
> + }
> + mem->nregions = i;
>
> r = ioctl(sdev->vhost_fd, VHOST_SET_OWNER);
> if (r != 0)
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks
2019-09-23 13:35 ` [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara
@ 2020-02-05 18:07 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-05 18:07 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, Alexandru Elisei wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> At the moment, virtio scsi registers only one bank starting at 0. On some
> architectures (like on x86, for example), this may not be true and the
> guest may have multiple memory regions.
>
> Register all the memory regions to vhost by browsing kvm->mem_banks. The
> code is based on the virtio_net__vhost_init implementation.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> virtio/scsi.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/virtio/scsi.c b/virtio/scsi.c
> index a72bb2a9a206..63fc4f4635a2 100644
> --- a/virtio/scsi.c
> +++ b/virtio/scsi.c
> @@ -190,24 +190,29 @@ static struct virtio_ops scsi_dev_virtio_ops = {
>
> static void virtio_scsi_vhost_init(struct kvm *kvm, struct scsi_dev *sdev)
> {
> + struct kvm_mem_bank *bank;
> struct vhost_memory *mem;
> u64 features;
> - int r;
> + int r, i;
>
> sdev->vhost_fd = open("/dev/vhost-scsi", O_RDWR);
> if (sdev->vhost_fd < 0)
> die_perror("Failed openning vhost-scsi device");
>
> - mem = calloc(1, sizeof(*mem) + sizeof(struct vhost_memory_region));
> + mem = calloc(1, sizeof(*mem) + kvm->mem_slots * sizeof(struct vhost_memory_region));
> if (mem == NULL)
> die("Failed allocating memory for vhost memory map");
>
> - mem->nregions = 1;
> - mem->regions[0] = (struct vhost_memory_region) {
> - .guest_phys_addr = 0,
> - .memory_size = kvm->ram_size,
> - .userspace_addr = (unsigned long)kvm->ram_start,
> - };
> + i = 0;
> + list_for_each_entry(bank, &kvm->mem_banks, list) {
> + mem->regions[i] = (struct vhost_memory_region) {
> + .guest_phys_addr = bank->guest_phys_addr,
> + .memory_size = bank->size,
> + .userspace_addr = (unsigned long)bank->host_addr,
> + };
> + i++;
> + }
> + mem->nregions = i;
>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (2 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 03/16] virtio/scsi: Allow the use of multiple banks Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara
2020-02-05 18:16 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 05/16] kvmtool: Use MB consistently Alexandru Elisei
` (12 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
kvmtool accepts generic and architecture specific parameters. When creating
a virtual machine, only the generic parameters are checked against sane
values. Add a function to sanitize the architecture specific configuration
options and call it before the initialization routines.
This patch was inspired by Julien Grall's patch.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/aarch64/include/kvm/kvm-arch.h | 2 +-
arm/include/arm-common/kvm-arch.h | 4 ++++
arm/kvm.c | 11 +++++++++--
builtin-run.c | 2 ++
mips/include/kvm/kvm-arch.h | 4 ++++
mips/kvm.c | 5 +++++
powerpc/include/kvm/kvm-arch.h | 4 ++++
powerpc/kvm.c | 5 +++++
x86/include/kvm/kvm-arch.h | 4 ++++
x86/kvm.c | 24 ++++++++++++------------
10 files changed, 50 insertions(+), 15 deletions(-)
diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 9de623ac6cb9..1b3d0a5fb1b4 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -5,7 +5,7 @@
0x8000 : \
0x80000)
-#define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \
+#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
ARM_LOMAP_MAX_MEMORY : \
ARM_HIMAP_MAX_MEMORY)
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index b9d486d5eac2..965978d7cfb5 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -74,4 +74,8 @@ struct kvm_arch {
u64 dtb_guest_start;
};
+struct kvm_config;
+
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
+
#endif /* ARM_COMMON__KVM_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 198cee5c0997..5decc138fd3e 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -57,11 +57,18 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
{
}
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
+{
+ if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
+ cfg->ram_size = ARM_MAX_MEMORY(cfg);
+ pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
+ }
+}
+
void kvm__arch_init(struct kvm *kvm)
{
unsigned long alignment;
/* Convenience aliases */
- u64 ram_size = kvm->cfg.ram_size;
const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
/*
@@ -87,7 +94,7 @@ void kvm__arch_init(struct kvm *kvm)
alignment = SZ_2M;
}
- kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
+ kvm->ram_size = kvm->cfg.ram_size;
kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
kvm->arch.ram_alloc_size);
diff --git a/builtin-run.c b/builtin-run.c
index c867c8ba0892..532c06f90ba0 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -642,6 +642,8 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
kvm->cfg.real_cmdline = real_cmdline;
+ kvm__arch_sanitize_cfg(&kvm->cfg);
+
if (kvm->cfg.kernel_filename) {
printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
kvm->cfg.kernel_filename,
diff --git a/mips/include/kvm/kvm-arch.h b/mips/include/kvm/kvm-arch.h
index fdc09d830263..f0bfff50c7c9 100644
--- a/mips/include/kvm/kvm-arch.h
+++ b/mips/include/kvm/kvm-arch.h
@@ -47,4 +47,8 @@ struct kvm_arch {
bool is64bit;
};
+struct kvm_config;
+
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
+
#endif /* KVM__KVM_ARCH_H */
diff --git a/mips/kvm.c b/mips/kvm.c
index e2a0c63b14b8..63d651f29f70 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -56,6 +56,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
}
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
+{
+ /* We don't have any arch specific configuration. */
+}
+
/* Architecture-specific KVM init */
void kvm__arch_init(struct kvm *kvm)
{
diff --git a/powerpc/include/kvm/kvm-arch.h b/powerpc/include/kvm/kvm-arch.h
index 8126b96cb66a..42ea7df1325f 100644
--- a/powerpc/include/kvm/kvm-arch.h
+++ b/powerpc/include/kvm/kvm-arch.h
@@ -64,4 +64,8 @@ struct kvm_arch {
struct spapr_phb *phb;
};
+struct kvm_config;
+
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
+
#endif /* KVM__KVM_ARCH_H */
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 034bc4608ad9..73965640cf82 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -87,6 +87,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
/* We don't need anything unusual in here. */
}
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
+{
+ /* We don't have any arch specific configuration. */
+}
+
/* Architecture-specific KVM init */
void kvm__arch_init(struct kvm *kvm)
{
diff --git a/x86/include/kvm/kvm-arch.h b/x86/include/kvm/kvm-arch.h
index bfdd3438a9de..2cc65f30fcd2 100644
--- a/x86/include/kvm/kvm-arch.h
+++ b/x86/include/kvm/kvm-arch.h
@@ -40,4 +40,8 @@ struct kvm_arch {
struct interrupt_table interrupt_table;
};
+struct kvm_config;
+
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
+
#endif /* KVM__KVM_ARCH_H */
diff --git a/x86/kvm.c b/x86/kvm.c
index 5abb41e370bb..df5d48106c80 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -129,6 +129,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
strcat(cmdline, " earlyprintk=serial i8042.noaux=1");
}
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
+{
+ /* vidmode should be either specified or set by default */
+ if (cfg->vnc || cfg->sdl || cfg->gtk) {
+ if (!cfg->arch.vidmode)
+ cfg->arch.vidmode = 0x312;
+ } else {
+ cfg->arch.vidmode = 0;
+ }
+}
+
/* Architecture-specific KVM init */
void kvm__arch_init(struct kvm *kvm)
{
@@ -239,7 +250,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
size_t cmdline_size;
ssize_t file_size;
void *p;
- u16 vidmode;
/*
* See Documentation/x86/boot.txt for details no bzImage on-disk and
@@ -282,23 +292,13 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
memcpy(p, kernel_cmdline, cmdline_size - 1);
}
- /* vidmode should be either specified or set by default */
- if (kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk) {
- if (!kvm->cfg.arch.vidmode)
- vidmode = 0x312;
- else
- vidmode = kvm->cfg.arch.vidmode;
- } else {
- vidmode = 0;
- }
-
kern_boot = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, 0x00);
kern_boot->hdr.cmd_line_ptr = BOOT_CMDLINE_OFFSET;
kern_boot->hdr.type_of_loader = 0xff;
kern_boot->hdr.heap_end_ptr = 0xfe00;
kern_boot->hdr.loadflags |= CAN_USE_HEAP;
- kern_boot->hdr.vid_mode = vidmode;
+ kern_boot->hdr.vid_mode = kvm->cfg.arch.vidmode;
/*
* Read initrd image into guest memory
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration
2019-09-23 13:35 ` [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration Alexandru Elisei
@ 2019-11-06 16:48 ` Andre Przywara
2019-11-07 10:05 ` Alexandru Elisei
2020-02-05 18:16 ` Suzuki Kuruppassery Poulose
1 sibling, 1 reply; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:48 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:10 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> kvmtool accepts generic and architecture specific parameters. When creating
> a virtual machine, only the generic parameters are checked against sane
> values. Add a function to sanitize the architecture specific configuration
> options and call it before the initialization routines.
>
> This patch was inspired by Julien Grall's patch.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
That's a bit confusing: If it is based on Julien's patch, you should keep him as the author, adding a short comment here about what *you* changed.
If it's not, you should not have a S-o-b: line from Julien.
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
The code looks good to me:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> arm/aarch64/include/kvm/kvm-arch.h | 2 +-
> arm/include/arm-common/kvm-arch.h | 4 ++++
> arm/kvm.c | 11 +++++++++--
> builtin-run.c | 2 ++
> mips/include/kvm/kvm-arch.h | 4 ++++
> mips/kvm.c | 5 +++++
> powerpc/include/kvm/kvm-arch.h | 4 ++++
> powerpc/kvm.c | 5 +++++
> x86/include/kvm/kvm-arch.h | 4 ++++
> x86/kvm.c | 24 ++++++++++++------------
> 10 files changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 9de623ac6cb9..1b3d0a5fb1b4 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -5,7 +5,7 @@
> 0x8000 : \
> 0x80000)
>
> -#define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \
> +#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
> ARM_LOMAP_MAX_MEMORY : \
> ARM_HIMAP_MAX_MEMORY)
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index b9d486d5eac2..965978d7cfb5 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -74,4 +74,8 @@ struct kvm_arch {
> u64 dtb_guest_start;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* ARM_COMMON__KVM_ARCH_H */
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 198cee5c0997..5decc138fd3e 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -57,11 +57,18 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> {
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> + cfg->ram_size = ARM_MAX_MEMORY(cfg);
> + pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> + }
> +}
> +
> void kvm__arch_init(struct kvm *kvm)
> {
> unsigned long alignment;
> /* Convenience aliases */
> - u64 ram_size = kvm->cfg.ram_size;
> const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
>
> /*
> @@ -87,7 +94,7 @@ void kvm__arch_init(struct kvm *kvm)
> alignment = SZ_2M;
> }
>
> - kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
> + kvm->ram_size = kvm->cfg.ram_size;
> kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
> kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
> kvm->arch.ram_alloc_size);
> diff --git a/builtin-run.c b/builtin-run.c
> index c867c8ba0892..532c06f90ba0 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -642,6 +642,8 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>
> kvm->cfg.real_cmdline = real_cmdline;
>
> + kvm__arch_sanitize_cfg(&kvm->cfg);
> +
> if (kvm->cfg.kernel_filename) {
> printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
> kvm->cfg.kernel_filename,
> diff --git a/mips/include/kvm/kvm-arch.h b/mips/include/kvm/kvm-arch.h
> index fdc09d830263..f0bfff50c7c9 100644
> --- a/mips/include/kvm/kvm-arch.h
> +++ b/mips/include/kvm/kvm-arch.h
> @@ -47,4 +47,8 @@ struct kvm_arch {
> bool is64bit;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/mips/kvm.c b/mips/kvm.c
> index e2a0c63b14b8..63d651f29f70 100644
> --- a/mips/kvm.c
> +++ b/mips/kvm.c
> @@ -56,6 +56,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* We don't have any arch specific configuration. */
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> diff --git a/powerpc/include/kvm/kvm-arch.h b/powerpc/include/kvm/kvm-arch.h
> index 8126b96cb66a..42ea7df1325f 100644
> --- a/powerpc/include/kvm/kvm-arch.h
> +++ b/powerpc/include/kvm/kvm-arch.h
> @@ -64,4 +64,8 @@ struct kvm_arch {
> struct spapr_phb *phb;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
> index 034bc4608ad9..73965640cf82 100644
> --- a/powerpc/kvm.c
> +++ b/powerpc/kvm.c
> @@ -87,6 +87,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> /* We don't need anything unusual in here. */
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* We don't have any arch specific configuration. */
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> diff --git a/x86/include/kvm/kvm-arch.h b/x86/include/kvm/kvm-arch.h
> index bfdd3438a9de..2cc65f30fcd2 100644
> --- a/x86/include/kvm/kvm-arch.h
> +++ b/x86/include/kvm/kvm-arch.h
> @@ -40,4 +40,8 @@ struct kvm_arch {
> struct interrupt_table interrupt_table;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
> +
> #endif /* KVM__KVM_ARCH_H */
> diff --git a/x86/kvm.c b/x86/kvm.c
> index 5abb41e370bb..df5d48106c80 100644
> --- a/x86/kvm.c
> +++ b/x86/kvm.c
> @@ -129,6 +129,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
> strcat(cmdline, " earlyprintk=serial i8042.noaux=1");
> }
>
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + /* vidmode should be either specified or set by default */
> + if (cfg->vnc || cfg->sdl || cfg->gtk) {
> + if (!cfg->arch.vidmode)
> + cfg->arch.vidmode = 0x312;
> + } else {
> + cfg->arch.vidmode = 0;
> + }
> +}
> +
> /* Architecture-specific KVM init */
> void kvm__arch_init(struct kvm *kvm)
> {
> @@ -239,7 +250,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
> size_t cmdline_size;
> ssize_t file_size;
> void *p;
> - u16 vidmode;
>
> /*
> * See Documentation/x86/boot.txt for details no bzImage on-disk and
> @@ -282,23 +292,13 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
> memcpy(p, kernel_cmdline, cmdline_size - 1);
> }
>
> - /* vidmode should be either specified or set by default */
> - if (kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk) {
> - if (!kvm->cfg.arch.vidmode)
> - vidmode = 0x312;
> - else
> - vidmode = kvm->cfg.arch.vidmode;
> - } else {
> - vidmode = 0;
> - }
> -
> kern_boot = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, 0x00);
>
> kern_boot->hdr.cmd_line_ptr = BOOT_CMDLINE_OFFSET;
> kern_boot->hdr.type_of_loader = 0xff;
> kern_boot->hdr.heap_end_ptr = 0xfe00;
> kern_boot->hdr.loadflags |= CAN_USE_HEAP;
> - kern_boot->hdr.vid_mode = vidmode;
> + kern_boot->hdr.vid_mode = kvm->cfg.arch.vidmode;
>
> /*
> * Read initrd image into guest memory
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration
2019-11-06 16:48 ` Andre Przywara
@ 2019-11-07 10:05 ` Alexandru Elisei
0 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-11-07 10:05 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
Hi,
On 11/6/19 4:48 PM, Andre Przywara wrote:
> On Mon, 23 Sep 2019 14:35:10 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> kvmtool accepts generic and architecture specific parameters. When creating
>> a virtual machine, only the generic parameters are checked against sane
>> values. Add a function to sanitize the architecture specific configuration
>> options and call it before the initialization routines.
>>
>> This patch was inspired by Julien Grall's patch.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> That's a bit confusing: If it is based on Julien's patch, you should keep him as the author, adding a short comment here about what *you* changed.
> If it's not, you should not have a S-o-b: line from Julien.
I wasn't really sure what to do in this case. I'll keep him at the author, thanks
for the suggestion.
Thanks,
Alex
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> The code looks good to me:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre.
>
>> ---
>> arm/aarch64/include/kvm/kvm-arch.h | 2 +-
>> arm/include/arm-common/kvm-arch.h | 4 ++++
>> arm/kvm.c | 11 +++++++++--
>> builtin-run.c | 2 ++
>> mips/include/kvm/kvm-arch.h | 4 ++++
>> mips/kvm.c | 5 +++++
>> powerpc/include/kvm/kvm-arch.h | 4 ++++
>> powerpc/kvm.c | 5 +++++
>> x86/include/kvm/kvm-arch.h | 4 ++++
>> x86/kvm.c | 24 ++++++++++++------------
>> 10 files changed, 50 insertions(+), 15 deletions(-)
>>
>> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
>> index 9de623ac6cb9..1b3d0a5fb1b4 100644
>> --- a/arm/aarch64/include/kvm/kvm-arch.h
>> +++ b/arm/aarch64/include/kvm/kvm-arch.h
>> @@ -5,7 +5,7 @@
>> 0x8000 : \
>> 0x80000)
>>
>> -#define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \
>> +#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
>> ARM_LOMAP_MAX_MEMORY : \
>> ARM_HIMAP_MAX_MEMORY)
>>
>> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
>> index b9d486d5eac2..965978d7cfb5 100644
>> --- a/arm/include/arm-common/kvm-arch.h
>> +++ b/arm/include/arm-common/kvm-arch.h
>> @@ -74,4 +74,8 @@ struct kvm_arch {
>> u64 dtb_guest_start;
>> };
>>
>> +struct kvm_config;
>> +
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
>> +
>> #endif /* ARM_COMMON__KVM_ARCH_H */
>> diff --git a/arm/kvm.c b/arm/kvm.c
>> index 198cee5c0997..5decc138fd3e 100644
>> --- a/arm/kvm.c
>> +++ b/arm/kvm.c
>> @@ -57,11 +57,18 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> {
>> }
>>
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
>> +{
>> + if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
>> + cfg->ram_size = ARM_MAX_MEMORY(cfg);
>> + pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
>> + }
>> +}
>> +
>> void kvm__arch_init(struct kvm *kvm)
>> {
>> unsigned long alignment;
>> /* Convenience aliases */
>> - u64 ram_size = kvm->cfg.ram_size;
>> const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
>>
>> /*
>> @@ -87,7 +94,7 @@ void kvm__arch_init(struct kvm *kvm)
>> alignment = SZ_2M;
>> }
>>
>> - kvm->ram_size = min(ram_size, (u64)ARM_MAX_MEMORY(kvm));
>> + kvm->ram_size = kvm->cfg.ram_size;
>> kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
>> kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
>> kvm->arch.ram_alloc_size);
>> diff --git a/builtin-run.c b/builtin-run.c
>> index c867c8ba0892..532c06f90ba0 100644
>> --- a/builtin-run.c
>> +++ b/builtin-run.c
>> @@ -642,6 +642,8 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
>>
>> kvm->cfg.real_cmdline = real_cmdline;
>>
>> + kvm__arch_sanitize_cfg(&kvm->cfg);
>> +
>> if (kvm->cfg.kernel_filename) {
>> printf(" # %s run -k %s -m %Lu -c %d --name %s\n", KVM_BINARY_NAME,
>> kvm->cfg.kernel_filename,
>> diff --git a/mips/include/kvm/kvm-arch.h b/mips/include/kvm/kvm-arch.h
>> index fdc09d830263..f0bfff50c7c9 100644
>> --- a/mips/include/kvm/kvm-arch.h
>> +++ b/mips/include/kvm/kvm-arch.h
>> @@ -47,4 +47,8 @@ struct kvm_arch {
>> bool is64bit;
>> };
>>
>> +struct kvm_config;
>> +
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
>> +
>> #endif /* KVM__KVM_ARCH_H */
>> diff --git a/mips/kvm.c b/mips/kvm.c
>> index e2a0c63b14b8..63d651f29f70 100644
>> --- a/mips/kvm.c
>> +++ b/mips/kvm.c
>> @@ -56,6 +56,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>>
>> }
>>
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
>> +{
>> + /* We don't have any arch specific configuration. */
>> +}
>> +
>> /* Architecture-specific KVM init */
>> void kvm__arch_init(struct kvm *kvm)
>> {
>> diff --git a/powerpc/include/kvm/kvm-arch.h b/powerpc/include/kvm/kvm-arch.h
>> index 8126b96cb66a..42ea7df1325f 100644
>> --- a/powerpc/include/kvm/kvm-arch.h
>> +++ b/powerpc/include/kvm/kvm-arch.h
>> @@ -64,4 +64,8 @@ struct kvm_arch {
>> struct spapr_phb *phb;
>> };
>>
>> +struct kvm_config;
>> +
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
>> +
>> #endif /* KVM__KVM_ARCH_H */
>> diff --git a/powerpc/kvm.c b/powerpc/kvm.c
>> index 034bc4608ad9..73965640cf82 100644
>> --- a/powerpc/kvm.c
>> +++ b/powerpc/kvm.c
>> @@ -87,6 +87,11 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> /* We don't need anything unusual in here. */
>> }
>>
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
>> +{
>> + /* We don't have any arch specific configuration. */
>> +}
>> +
>> /* Architecture-specific KVM init */
>> void kvm__arch_init(struct kvm *kvm)
>> {
>> diff --git a/x86/include/kvm/kvm-arch.h b/x86/include/kvm/kvm-arch.h
>> index bfdd3438a9de..2cc65f30fcd2 100644
>> --- a/x86/include/kvm/kvm-arch.h
>> +++ b/x86/include/kvm/kvm-arch.h
>> @@ -40,4 +40,8 @@ struct kvm_arch {
>> struct interrupt_table interrupt_table;
>> };
>>
>> +struct kvm_config;
>> +
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
>> +
>> #endif /* KVM__KVM_ARCH_H */
>> diff --git a/x86/kvm.c b/x86/kvm.c
>> index 5abb41e370bb..df5d48106c80 100644
>> --- a/x86/kvm.c
>> +++ b/x86/kvm.c
>> @@ -129,6 +129,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>> strcat(cmdline, " earlyprintk=serial i8042.noaux=1");
>> }
>>
>> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
>> +{
>> + /* vidmode should be either specified or set by default */
>> + if (cfg->vnc || cfg->sdl || cfg->gtk) {
>> + if (!cfg->arch.vidmode)
>> + cfg->arch.vidmode = 0x312;
>> + } else {
>> + cfg->arch.vidmode = 0;
>> + }
>> +}
>> +
>> /* Architecture-specific KVM init */
>> void kvm__arch_init(struct kvm *kvm)
>> {
>> @@ -239,7 +250,6 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
>> size_t cmdline_size;
>> ssize_t file_size;
>> void *p;
>> - u16 vidmode;
>>
>> /*
>> * See Documentation/x86/boot.txt for details no bzImage on-disk and
>> @@ -282,23 +292,13 @@ static bool load_bzimage(struct kvm *kvm, int fd_kernel, int fd_initrd,
>> memcpy(p, kernel_cmdline, cmdline_size - 1);
>> }
>>
>> - /* vidmode should be either specified or set by default */
>> - if (kvm->cfg.vnc || kvm->cfg.sdl || kvm->cfg.gtk) {
>> - if (!kvm->cfg.arch.vidmode)
>> - vidmode = 0x312;
>> - else
>> - vidmode = kvm->cfg.arch.vidmode;
>> - } else {
>> - vidmode = 0;
>> - }
>> -
>> kern_boot = guest_real_to_host(kvm, BOOT_LOADER_SELECTOR, 0x00);
>>
>> kern_boot->hdr.cmd_line_ptr = BOOT_CMDLINE_OFFSET;
>> kern_boot->hdr.type_of_loader = 0xff;
>> kern_boot->hdr.heap_end_ptr = 0xfe00;
>> kern_boot->hdr.loadflags |= CAN_USE_HEAP;
>> - kern_boot->hdr.vid_mode = vidmode;
>> + kern_boot->hdr.vid_mode = kvm->cfg.arch.vidmode;
>>
>> /*
>> * Read initrd image into guest memory
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration
2019-09-23 13:35 ` [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration Alexandru Elisei
2019-11-06 16:48 ` Andre Przywara
@ 2020-02-05 18:16 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-05 18:16 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, Alexandru Elisei wrote:
> kvmtool accepts generic and architecture specific parameters. When creating
> a virtual machine, only the generic parameters are checked against sane
> values. Add a function to sanitize the architecture specific configuration
> options and call it before the initialization routines.
>
> This patch was inspired by Julien Grall's patch.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/aarch64/include/kvm/kvm-arch.h | 2 +-
> arm/include/arm-common/kvm-arch.h | 4 ++++
> arm/kvm.c | 11 +++++++++--
> builtin-run.c | 2 ++
> mips/include/kvm/kvm-arch.h | 4 ++++
> mips/kvm.c | 5 +++++
> powerpc/include/kvm/kvm-arch.h | 4 ++++
> powerpc/kvm.c | 5 +++++
> x86/include/kvm/kvm-arch.h | 4 ++++
> x86/kvm.c | 24 ++++++++++++------------
> 10 files changed, 50 insertions(+), 15 deletions(-)
>
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 9de623ac6cb9..1b3d0a5fb1b4 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -5,7 +5,7 @@
> 0x8000 : \
> 0x80000)
>
> -#define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \
> +#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
> ARM_LOMAP_MAX_MEMORY : \
> ARM_HIMAP_MAX_MEMORY)
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index b9d486d5eac2..965978d7cfb5 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -74,4 +74,8 @@ struct kvm_arch {
> u64 dtb_guest_start;
> };
>
> +struct kvm_config;
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
minor nit: We could have passed "struct kvm", which could
1) avoid the hunk above for ARM_MAX_MEMORY()
2) make better use of the other info available in KVM for
anything that we could potentially do.
But hey, we don't change the kvmtool that much. So, feel free
to ignore this.
Either way:
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 05/16] kvmtool: Use MB consistently
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (3 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 04/16] kvmtool: Add helper to sanitize arch specific KVM configuration Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2020-02-05 18:17 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes Alexandru Elisei
` (11 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
The help text for the -m/--mem argument states that the guest memory size
is in MiB (mebibyte). We all know that MB (megabyte) is the same thing as
MiB, and indeed this is how MB is used throughout kvmtool.
So replace MiB with MB, so people don't get the wrong idea and start
believing that for kvmtool a MB is 10^6 bytes, because it isn't.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
Documentation/kvmtool.1 | 4 ++--
builtin-run.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/kvmtool.1 b/Documentation/kvmtool.1
index 2b8c274dc3ff..25d46f8f51f9 100644
--- a/Documentation/kvmtool.1
+++ b/Documentation/kvmtool.1
@@ -10,7 +10,7 @@ kvmtool is a userland tool for creating and controlling KVM guests.
.SH "KVMTOOL COMMANDS"
.sp
.PP
-.B run -k <kernel\-image> [\-c <cores>] [\-m <MiB>] [\-p <command line>]
+.B run -k <kernel\-image> [\-c <cores>] [\-m <MB>] [\-p <command line>]
.br
.B [\-i <initrd>] [\-d <image file>] [\-\-console serial|virtio|hv]
.br
@@ -30,7 +30,7 @@ The number of virtual CPUs to run.
.sp
.B \-m, \-\-mem <n>
.RS 4
-Virtual machine memory size in MiB.
+Virtual machine memory size in MB.
.RE
.sp
.B \-p, \-\-params <parameters>
diff --git a/builtin-run.c b/builtin-run.c
index 532c06f90ba0..cff44047bb1c 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -98,7 +98,7 @@ void kvm_run_set_wrapper_sandbox(void)
"A name for the guest"), \
OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
- " size in MiB."), \
+ " size in MB."), \
OPT_CALLBACK('\0', "shmem", NULL, \
"[pci:]<addr>:<size>[:handle=<handle>][:create]", \
"Share host shmem with guest via pci device", \
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 05/16] kvmtool: Use MB consistently
2019-09-23 13:35 ` [PATCH kvmtool 05/16] kvmtool: Use MB consistently Alexandru Elisei
@ 2019-11-06 16:49 ` Andre Przywara
2020-02-05 18:17 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:49 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:11 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> The help text for the -m/--mem argument states that the guest memory size
> is in MiB (mebibyte). We all know that MB (megabyte) is the same thing as
> MiB, and indeed this is how MB is used throughout kvmtool.
>
> So replace MiB with MB, so people don't get the wrong idea and start
> believing that for kvmtool a MB is 10^6 bytes, because it isn't.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> Documentation/kvmtool.1 | 4 ++--
> builtin-run.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/kvmtool.1 b/Documentation/kvmtool.1
> index 2b8c274dc3ff..25d46f8f51f9 100644
> --- a/Documentation/kvmtool.1
> +++ b/Documentation/kvmtool.1
> @@ -10,7 +10,7 @@ kvmtool is a userland tool for creating and controlling KVM guests.
> .SH "KVMTOOL COMMANDS"
> .sp
> .PP
> -.B run -k <kernel\-image> [\-c <cores>] [\-m <MiB>] [\-p <command line>]
> +.B run -k <kernel\-image> [\-c <cores>] [\-m <MB>] [\-p <command line>]
> .br
> .B [\-i <initrd>] [\-d <image file>] [\-\-console serial|virtio|hv]
> .br
> @@ -30,7 +30,7 @@ The number of virtual CPUs to run.
> .sp
> .B \-m, \-\-mem <n>
> .RS 4
> -Virtual machine memory size in MiB.
> +Virtual machine memory size in MB.
> .RE
> .sp
> .B \-p, \-\-params <parameters>
> diff --git a/builtin-run.c b/builtin-run.c
> index 532c06f90ba0..cff44047bb1c 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -98,7 +98,7 @@ void kvm_run_set_wrapper_sandbox(void)
> "A name for the guest"), \
> OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
> OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
> - " size in MiB."), \
> + " size in MB."), \
> OPT_CALLBACK('\0', "shmem", NULL, \
> "[pci:]<addr>:<size>[:handle=<handle>][:create]", \
> "Share host shmem with guest via pci device", \
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 05/16] kvmtool: Use MB consistently
2019-09-23 13:35 ` [PATCH kvmtool 05/16] kvmtool: Use MB consistently Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
@ 2020-02-05 18:17 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-05 18:17 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, Alexandru Elisei wrote:
> The help text for the -m/--mem argument states that the guest memory size
> is in MiB (mebibyte). We all know that MB (megabyte) is the same thing as
> MiB, and indeed this is how MB is used throughout kvmtool.
>
> So replace MiB with MB, so people don't get the wrong idea and start
> believing that for kvmtool a MB is 10^6 bytes, because it isn't.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (4 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 05/16] kvmtool: Use MB consistently Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2020-02-05 19:03 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE Alexandru Elisei
` (10 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
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 aftwerwards converted to bytes, and
this is how it is used throughout the rest of the program.
Let's avoid any confusion about the unit of measurement and always use
cfg->ram_size in bytes.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
builtin-run.c | 19 ++++++++++---------
include/kvm/kvm-config.h | 2 +-
include/kvm/kvm.h | 2 +-
3 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/builtin-run.c b/builtin-run.c
index cff44047bb1c..4e0c52b3e027 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -262,7 +262,7 @@ static u64 host_ram_size(void)
return 0;
}
- return (nr_pages * page_size) >> MB_SHIFT;
+ return nr_pages * page_size;
}
/*
@@ -276,11 +276,11 @@ static u64 get_ram_size(int nr_cpus)
u64 available;
u64 ram_size;
- ram_size = 64 * (nr_cpus + 3);
+ ram_size = (64 * (nr_cpus + 3)) << MB_SHIFT;
available = host_ram_size() * RAM_SIZE_RATIO;
if (!available)
- available = MIN_RAM_SIZE_MB;
+ available = MIN_RAM_SIZE_BYTE;
if (ram_size > available)
ram_size = available;
@@ -531,13 +531,14 @@ 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
+ /* The user specifies the memory in MB. */
+ 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;
@@ -647,12 +648,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 a052b0bc7582..e0c9ee14e103 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -22,7 +22,7 @@ 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 image_count;
u8 num_net_devices;
u8 num_vfio_devices;
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 635ce0f40b1e..a866d5a825c4 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -68,7 +68,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 list_head mem_banks;
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes
2019-09-23 13:35 ` [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes Alexandru Elisei
@ 2019-11-06 16:49 ` Andre Przywara
2019-11-07 10:08 ` Alexandru Elisei
2020-02-05 19:03 ` Suzuki Kuruppassery Poulose
1 sibling, 1 reply; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:49 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:12 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> 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 aftwerwards converted to bytes, and
> this is how it is used throughout the rest of the program.
>
> Let's avoid any confusion about the unit of measurement and always use
> cfg->ram_size in bytes.
... which also means you can get rid of MIN_RAM_SIZE_MB in include/kvm/kvm-config.h.
Otherwise:
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre
> ---
> builtin-run.c | 19 ++++++++++---------
> include/kvm/kvm-config.h | 2 +-
> include/kvm/kvm.h | 2 +-
> 3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/builtin-run.c b/builtin-run.c
> index cff44047bb1c..4e0c52b3e027 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -262,7 +262,7 @@ static u64 host_ram_size(void)
> return 0;
> }
>
> - return (nr_pages * page_size) >> MB_SHIFT;
> + return nr_pages * page_size;
> }
>
> /*
> @@ -276,11 +276,11 @@ static u64 get_ram_size(int nr_cpus)
> u64 available;
> u64 ram_size;
>
> - ram_size = 64 * (nr_cpus + 3);
> + ram_size = (64 * (nr_cpus + 3)) << MB_SHIFT;
>
> available = host_ram_size() * RAM_SIZE_RATIO;
> if (!available)
> - available = MIN_RAM_SIZE_MB;
> + available = MIN_RAM_SIZE_BYTE;
>
> if (ram_size > available)
> ram_size = available;
> @@ -531,13 +531,14 @@ 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
> + /* The user specifies the memory in MB. */
> + 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;
> @@ -647,12 +648,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 a052b0bc7582..e0c9ee14e103 100644
> --- a/include/kvm/kvm-config.h
> +++ b/include/kvm/kvm-config.h
> @@ -22,7 +22,7 @@ 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 image_count;
> u8 num_net_devices;
> u8 num_vfio_devices;
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 635ce0f40b1e..a866d5a825c4 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -68,7 +68,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 list_head mem_banks;
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes
2019-11-06 16:49 ` Andre Przywara
@ 2019-11-07 10:08 ` Alexandru Elisei
0 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-11-07 10:08 UTC (permalink / raw)
To: Andre Przywara
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
Hi,
On 11/6/19 4:49 PM, Andre Przywara wrote:
> On Mon, 23 Sep 2019 14:35:12 +0100
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi,
>
>> 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 aftwerwards converted to bytes, and
>> this is how it is used throughout the rest of the program.
>>
>> Let's avoid any confusion about the unit of measurement and always use
>> cfg->ram_size in bytes.
> ... which also means you can get rid of MIN_RAM_SIZE_MB in include/kvm/kvm-config.h.
I'll do that, thanks for spotting it.
Thanks,
Alex
>
> Otherwise:
>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
>> ---
>> builtin-run.c | 19 ++++++++++---------
>> include/kvm/kvm-config.h | 2 +-
>> include/kvm/kvm.h | 2 +-
>> 3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/builtin-run.c b/builtin-run.c
>> index cff44047bb1c..4e0c52b3e027 100644
>> --- a/builtin-run.c
>> +++ b/builtin-run.c
>> @@ -262,7 +262,7 @@ static u64 host_ram_size(void)
>> return 0;
>> }
>>
>> - return (nr_pages * page_size) >> MB_SHIFT;
>> + return nr_pages * page_size;
>> }
>>
>> /*
>> @@ -276,11 +276,11 @@ static u64 get_ram_size(int nr_cpus)
>> u64 available;
>> u64 ram_size;
>>
>> - ram_size = 64 * (nr_cpus + 3);
>> + ram_size = (64 * (nr_cpus + 3)) << MB_SHIFT;
>>
>> available = host_ram_size() * RAM_SIZE_RATIO;
>> if (!available)
>> - available = MIN_RAM_SIZE_MB;
>> + available = MIN_RAM_SIZE_BYTE;
>>
>> if (ram_size > available)
>> ram_size = available;
>> @@ -531,13 +531,14 @@ 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
>> + /* The user specifies the memory in MB. */
>> + 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;
>> @@ -647,12 +648,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 a052b0bc7582..e0c9ee14e103 100644
>> --- a/include/kvm/kvm-config.h
>> +++ b/include/kvm/kvm-config.h
>> @@ -22,7 +22,7 @@ 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 image_count;
>> u8 num_net_devices;
>> u8 num_vfio_devices;
>> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
>> index 635ce0f40b1e..a866d5a825c4 100644
>> --- a/include/kvm/kvm.h
>> +++ b/include/kvm/kvm.h
>> @@ -68,7 +68,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 list_head mem_banks;
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes
2019-09-23 13:35 ` [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
@ 2020-02-05 19:03 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-05 19:03 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, 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 aftwerwards converted to bytes, and
> this is how it is used throughout the rest of the program.
>
> Let's avoid any confusion about the unit of measurement and always use
> cfg->ram_size in bytes.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> builtin-run.c | 19 ++++++++++---------
> include/kvm/kvm-config.h | 2 +-
> include/kvm/kvm.h | 2 +-
> 3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/builtin-run.c b/builtin-run.c
> index cff44047bb1c..4e0c52b3e027 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -262,7 +262,7 @@ static u64 host_ram_size(void)
> return 0;
> }
>
> - return (nr_pages * page_size) >> MB_SHIFT;
> + return nr_pages * page_size;
> }
>
> /*
> @@ -276,11 +276,11 @@ static u64 get_ram_size(int nr_cpus)
> u64 available;
> u64 ram_size;
>
> - ram_size = 64 * (nr_cpus + 3);
> + ram_size = (64 * (nr_cpus + 3)) << MB_SHIFT;
>
> available = host_ram_size() * RAM_SIZE_RATIO;
The host_ram_size() gives you size in MB isn't it ? You need to
fix that to make sure we aren't comparing "ram_size" in bytes
with "available" in MB below. So the best option is to
talk bytes everywhere.
Otherwise looks fine.
Suzuki
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (5 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 06/16] builtin-run.c: Always use ram_size in bytes Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
2020-02-06 11:49 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram Alexandru Elisei
` (9 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
ARM_PCI_CFG_SIZE has the same value as PCI_CFG_SIZE. The pci driver uses
PCI_CFG_SIZE and arm uses ARM_PCI_CFG_SIZE when generating the pci DT node.
Having two defines with the same value is confusing, and can lead to bugs
if one define is changed and the other isn't. So replace all instances of
ARM_PCI_CFG_SIZE with PCI_CFG_SIZE.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/include/arm-common/kvm-arch.h | 7 ++++---
arm/pci.c | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index 965978d7cfb5..f8f6b8f98c96 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -5,6 +5,8 @@
#include <linux/const.h>
#include <linux/types.h>
+#include "kvm/pci.h"
+
#include "arm-common/gic.h"
#define ARM_IOPORT_AREA _AC(0x0000000000000000, UL)
@@ -23,13 +25,12 @@
#define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
#define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
-#define ARM_PCI_CFG_SIZE (1ULL << 24)
#define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
- (ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
+ (ARM_AXI_AREA + PCI_CFG_SIZE))
#define KVM_IOPORT_AREA ARM_IOPORT_AREA
#define KVM_PCI_CFG_AREA ARM_AXI_AREA
-#define KVM_PCI_MMIO_AREA (KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
+#define KVM_PCI_MMIO_AREA (KVM_PCI_CFG_AREA + PCI_CFG_SIZE)
#define KVM_VIRTIO_MMIO_AREA ARM_MMIO_AREA
#define KVM_IOEVENTFD_HAS_PIO 0
diff --git a/arm/pci.c b/arm/pci.c
index 557cfa98938d..1a2fc2688c9e 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -33,7 +33,7 @@ void pci__generate_fdt_nodes(void *fdt)
u32 bus_range[] = { cpu_to_fdt32(0), cpu_to_fdt32(1), };
/* Configuration Space */
u64 cfg_reg_prop[] = { cpu_to_fdt64(KVM_PCI_CFG_AREA),
- cpu_to_fdt64(ARM_PCI_CFG_SIZE), };
+ cpu_to_fdt64(PCI_CFG_SIZE), };
/* Describe the memory ranges */
struct of_pci_ranges_entry ranges[] = {
{
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE
2019-09-23 13:35 ` [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE Alexandru Elisei
@ 2019-11-06 16:49 ` Andre Przywara
2020-02-06 11:49 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-06 16:49 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:13 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> ARM_PCI_CFG_SIZE has the same value as PCI_CFG_SIZE. The pci driver uses
> PCI_CFG_SIZE and arm uses ARM_PCI_CFG_SIZE when generating the pci DT node.
> Having two defines with the same value is confusing, and can lead to bugs
> if one define is changed and the other isn't. So replace all instances of
> ARM_PCI_CFG_SIZE with PCI_CFG_SIZE.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> arm/include/arm-common/kvm-arch.h | 7 ++++---
> arm/pci.c | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index 965978d7cfb5..f8f6b8f98c96 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -5,6 +5,8 @@
> #include <linux/const.h>
> #include <linux/types.h>
>
> +#include "kvm/pci.h"
> +
> #include "arm-common/gic.h"
>
> #define ARM_IOPORT_AREA _AC(0x0000000000000000, UL)
> @@ -23,13 +25,12 @@
>
> #define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
> #define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
> -#define ARM_PCI_CFG_SIZE (1ULL << 24)
> #define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
> - (ARM_AXI_AREA + ARM_PCI_CFG_SIZE))
> + (ARM_AXI_AREA + PCI_CFG_SIZE))
>
> #define KVM_IOPORT_AREA ARM_IOPORT_AREA
> #define KVM_PCI_CFG_AREA ARM_AXI_AREA
> -#define KVM_PCI_MMIO_AREA (KVM_PCI_CFG_AREA + ARM_PCI_CFG_SIZE)
> +#define KVM_PCI_MMIO_AREA (KVM_PCI_CFG_AREA + PCI_CFG_SIZE)
> #define KVM_VIRTIO_MMIO_AREA ARM_MMIO_AREA
>
> #define KVM_IOEVENTFD_HAS_PIO 0
> diff --git a/arm/pci.c b/arm/pci.c
> index 557cfa98938d..1a2fc2688c9e 100644
> --- a/arm/pci.c
> +++ b/arm/pci.c
> @@ -33,7 +33,7 @@ void pci__generate_fdt_nodes(void *fdt)
> u32 bus_range[] = { cpu_to_fdt32(0), cpu_to_fdt32(1), };
> /* Configuration Space */
> u64 cfg_reg_prop[] = { cpu_to_fdt64(KVM_PCI_CFG_AREA),
> - cpu_to_fdt64(ARM_PCI_CFG_SIZE), };
> + cpu_to_fdt64(PCI_CFG_SIZE), };
> /* Describe the memory ranges */
> struct of_pci_ranges_entry ranges[] = {
> {
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE
2019-09-23 13:35 ` [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE Alexandru Elisei
2019-11-06 16:49 ` Andre Przywara
@ 2020-02-06 11:49 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-06 11:49 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, Alexandru Elisei wrote:
> ARM_PCI_CFG_SIZE has the same value as PCI_CFG_SIZE. The pci driver uses
> PCI_CFG_SIZE and arm uses ARM_PCI_CFG_SIZE when generating the pci DT node.
> Having two defines with the same value is confusing, and can lead to bugs
> if one define is changed and the other isn't. So replace all instances of
> ARM_PCI_CFG_SIZE with PCI_CFG_SIZE.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (6 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 07/16] arm: Remove redundant define ARM_PCI_CFG_SIZE Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-07 13:46 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address Alexandru Elisei
` (8 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
From: Julien Grall <julien.grall@arm.com>
RAM initialization is currently split between kvm__init_ram and
kvm__arch_init. Move all code related to RAM initialization to
kvm__init_ram.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/kvm.c | 75 +++++++++++++++++++++++++++++++--------------------------------
1 file changed, 37 insertions(+), 38 deletions(-)
diff --git a/arm/kvm.c b/arm/kvm.c
index 5decc138fd3e..3e49db7704aa 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -29,44 +29,6 @@ void kvm__init_ram(struct kvm *kvm)
int err;
u64 phys_start, phys_size;
void *host_mem;
-
- phys_start = ARM_MEMORY_AREA;
- phys_size = kvm->ram_size;
- host_mem = kvm->ram_start;
-
- err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
- if (err)
- die("Failed to register %lld bytes of memory at physical "
- "address 0x%llx [err %d]", phys_size, phys_start, err);
-
- kvm->arch.memory_guest_start = phys_start;
-}
-
-void kvm__arch_delete_ram(struct kvm *kvm)
-{
- munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
-}
-
-void kvm__arch_read_term(struct kvm *kvm)
-{
- serial8250__update_consoles(kvm);
- virtio_console__inject_interrupt(kvm);
-}
-
-void kvm__arch_set_cmdline(char *cmdline, bool video)
-{
-}
-
-void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
-{
- if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
- cfg->ram_size = ARM_MAX_MEMORY(cfg);
- pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
- }
-}
-
-void kvm__arch_init(struct kvm *kvm)
-{
unsigned long alignment;
/* Convenience aliases */
const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
@@ -115,6 +77,43 @@ void kvm__arch_init(struct kvm *kvm)
madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
MADV_HUGEPAGE);
+ phys_start = ARM_MEMORY_AREA;
+ phys_size = kvm->ram_size;
+ host_mem = kvm->ram_start;
+
+ err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
+ if (err)
+ die("Failed to register %lld bytes of memory at physical "
+ "address 0x%llx [err %d]", phys_size, phys_start, err);
+
+ kvm->arch.memory_guest_start = phys_start;
+}
+
+void kvm__arch_delete_ram(struct kvm *kvm)
+{
+ munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
+}
+
+void kvm__arch_read_term(struct kvm *kvm)
+{
+ serial8250__update_consoles(kvm);
+ virtio_console__inject_interrupt(kvm);
+}
+
+void kvm__arch_set_cmdline(char *cmdline, bool video)
+{
+}
+
+void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
+{
+ if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
+ cfg->ram_size = ARM_MAX_MEMORY(cfg);
+ pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
+ }
+}
+
+void kvm__arch_init(struct kvm *kvm)
+{
/* Create the virtual GIC. */
if (gic__create(kvm, kvm->cfg.arch.irqchip))
die("Failed to create virtual GIC");
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram
2019-09-23 13:35 ` [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram Alexandru Elisei
@ 2019-11-07 13:46 ` Andre Przywara
0 siblings, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-07 13:46 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:14 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> From: Julien Grall <julien.grall@arm.com>
>
> RAM initialization is currently split between kvm__init_ram and
> kvm__arch_init. Move all code related to RAM initialization to
> kvm__init_ram.
The diff is a bit confusing to read, but indeed this just moves the code from arch_init() to init_ram() (confirmed by moving the code and comparing).
One thing to note is that this changes the order of initialisation slightly: the GIC is now created before the RAM (since we call arch_init() before init_ram()).
Nevertheless:
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Cheers,
Andre.
> ---
> arm/kvm.c | 75 +++++++++++++++++++++++++++++++--------------------------------
> 1 file changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 5decc138fd3e..3e49db7704aa 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -29,44 +29,6 @@ void kvm__init_ram(struct kvm *kvm)
> int err;
> u64 phys_start, phys_size;
> void *host_mem;
> -
> - phys_start = ARM_MEMORY_AREA;
> - phys_size = kvm->ram_size;
> - host_mem = kvm->ram_start;
> -
> - err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
> - if (err)
> - die("Failed to register %lld bytes of memory at physical "
> - "address 0x%llx [err %d]", phys_size, phys_start, err);
> -
> - kvm->arch.memory_guest_start = phys_start;
> -}
> -
> -void kvm__arch_delete_ram(struct kvm *kvm)
> -{
> - munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
> -}
> -
> -void kvm__arch_read_term(struct kvm *kvm)
> -{
> - serial8250__update_consoles(kvm);
> - virtio_console__inject_interrupt(kvm);
> -}
> -
> -void kvm__arch_set_cmdline(char *cmdline, bool video)
> -{
> -}
> -
> -void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> -{
> - if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> - cfg->ram_size = ARM_MAX_MEMORY(cfg);
> - pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> - }
> -}
> -
> -void kvm__arch_init(struct kvm *kvm)
> -{
> unsigned long alignment;
> /* Convenience aliases */
> const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
> @@ -115,6 +77,43 @@ void kvm__arch_init(struct kvm *kvm)
> madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
> MADV_HUGEPAGE);
>
> + phys_start = ARM_MEMORY_AREA;
> + phys_size = kvm->ram_size;
> + host_mem = kvm->ram_start;
> +
> + err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
> + if (err)
> + die("Failed to register %lld bytes of memory at physical "
> + "address 0x%llx [err %d]", phys_size, phys_start, err);
> +
> + kvm->arch.memory_guest_start = phys_start;
> +}
> +
> +void kvm__arch_delete_ram(struct kvm *kvm)
> +{
> + munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
> +}
> +
> +void kvm__arch_read_term(struct kvm *kvm)
> +{
> + serial8250__update_consoles(kvm);
> + virtio_console__inject_interrupt(kvm);
> +}
> +
> +void kvm__arch_set_cmdline(char *cmdline, bool video)
> +{
> +}
> +
> +void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> +{
> + if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> + cfg->ram_size = ARM_MAX_MEMORY(cfg);
> + pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> + }
> +}
> +
> +void kvm__arch_init(struct kvm *kvm)
> +{
> /* Create the virtual GIC. */
> if (gic__create(kvm, kvm->cfg.arch.irqchip))
> die("Failed to create virtual GIC");
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (7 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 08/16] arm: Move anything related to RAM initialization in kvm__init_ram Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-07 13:54 ` Andre Przywara
2020-02-06 12:20 ` Suzuki Kuruppassery Poulose
2019-09-23 13:35 ` [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory Alexandru Elisei
` (7 subsequent siblings)
16 siblings, 2 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
At the moment, the user can specify the amount of RAM a virtual machine
has, which starts at the fixed address ARM_MEMORY_AREA. The memory below
this address is used by MMIO and PCI devices.
However, it might be interesting to specify a different starting address
for the guest RAM. With this patch, the user can specify the address and
the amount of RAM a virtual machine has from the command line by using the
syntax -m/--mem size[@addr]. The address is optional, and must be higher or
equal to ARM_MEMORY_AREA. If it's not specified, the old behavior is
preserved.
This option is guarded by the define ARCH_SUPPORT_CFG_RAM_BASE, and at
the moment only the arm architecture has support for it. If an
architecture doesn't implement this feature, then the old behavior is
preserved and specifying the RAM base address is considered an user
error.
This patch is based upon previous work by Julien Grall.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/aarch32/include/kvm/kvm-arch.h | 2 +-
arm/aarch64/include/kvm/kvm-arch.h | 6 ++---
arm/include/arm-common/kvm-arch.h | 6 +++--
arm/kvm.c | 17 ++++++++++----
builtin-run.c | 48 ++++++++++++++++++++++++++++++++++----
include/kvm/kvm-config.h | 3 +++
kvm.c | 6 +++++
7 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/arm/aarch32/include/kvm/kvm-arch.h b/arm/aarch32/include/kvm/kvm-arch.h
index cd31e72971bd..0aa5db40502d 100644
--- a/arm/aarch32/include/kvm/kvm-arch.h
+++ b/arm/aarch32/include/kvm/kvm-arch.h
@@ -3,7 +3,7 @@
#define ARM_KERN_OFFSET(...) 0x8000
-#define ARM_MAX_MEMORY(...) ARM_LOMAP_MAX_MEMORY
+#define ARM_MAX_ADDR(...) ARM_LOMAP_MAX_ADDR
#include "arm-common/kvm-arch.h"
diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 1b3d0a5fb1b4..0c58675654c5 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -5,9 +5,9 @@
0x8000 : \
0x80000)
-#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
- ARM_LOMAP_MAX_MEMORY : \
- ARM_HIMAP_MAX_MEMORY)
+#define ARM_MAX_ADDR(cfg) ((cfg)->arch.aarch32_guest ? \
+ ARM_LOMAP_MAX_ADDR : \
+ ARM_HIMAP_MAX_ADDR)
#include "arm-common/kvm-arch.h"
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index f8f6b8f98c96..ad1a0e6872dc 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -14,8 +14,8 @@
#define ARM_AXI_AREA _AC(0x0000000040000000, UL)
#define ARM_MEMORY_AREA _AC(0x0000000080000000, UL)
-#define ARM_LOMAP_MAX_MEMORY ((1ULL << 32) - ARM_MEMORY_AREA)
-#define ARM_HIMAP_MAX_MEMORY ((1ULL << 40) - ARM_MEMORY_AREA)
+#define ARM_LOMAP_MAX_ADDR (1ULL << 32)
+#define ARM_HIMAP_MAX_ADDR (1ULL << 40)
#define ARM_GIC_DIST_BASE (ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
#define ARM_GIC_CPUI_BASE (ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
@@ -35,6 +35,8 @@
#define KVM_IOEVENTFD_HAS_PIO 0
+#define ARCH_SUPPORT_CFG_RAM_BASE 1
+
/*
* On a GICv3 there must be one redistributor per vCPU.
* The value here is the size for one, we multiply this at runtime with
diff --git a/arm/kvm.c b/arm/kvm.c
index 3e49db7704aa..355c118b098a 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -77,7 +77,7 @@ void kvm__init_ram(struct kvm *kvm)
madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
MADV_HUGEPAGE);
- phys_start = ARM_MEMORY_AREA;
+ phys_start = kvm->cfg.ram_base;
phys_size = kvm->ram_size;
host_mem = kvm->ram_start;
@@ -106,8 +106,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
{
- if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
- cfg->ram_size = ARM_MAX_MEMORY(cfg);
+ if (cfg->ram_base == INVALID_MEM_ADDR)
+ cfg->ram_base = ARM_MEMORY_AREA;
+
+ if (cfg->ram_base < ARM_MEMORY_AREA ||
+ cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
+ cfg->ram_base = ARM_MEMORY_AREA;
+ pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
+ }
+
+ if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
+ cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
}
}
@@ -229,7 +238,7 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
/* For default firmware address, lets load it at the begining of RAM */
if (fw_addr == 0)
- fw_addr = ARM_MEMORY_AREA;
+ fw_addr = kvm->cfg.ram_base;
if (!validate_fw_addr(kvm, fw_addr))
die("Bad firmware destination: 0x%016llx", fw_addr);
diff --git a/builtin-run.c b/builtin-run.c
index 4e0c52b3e027..df255cc44078 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -87,6 +87,44 @@ void kvm_run_set_wrapper_sandbox(void)
kvm_run_wrapper = KVM_RUN_SANDBOX;
}
+static int mem_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ char *next;
+ u64 size, addr = INVALID_MEM_ADDR;
+
+ /* Parse memory size. */
+ size = strtoll(p, &next, 0);
+ if (next == p)
+ die("Invalid memory size");
+
+ /* The user specifies the memory in MB, we use bytes. */
+ size <<= MB_SHIFT;
+
+ if (*next == '\0')
+ goto out;
+ else if (*next == '@')
+ p = next + 1;
+ else
+ die("Unexpected character after memory size: %c", *next);
+
+ addr = strtoll(p, &next, 0);
+ if (next == p)
+ die("Invalid memory address");
+
+#ifndef ARCH_SUPPORT_CFG_RAM_BASE
+ if (addr != INVALID_MEM_ADDR)
+ die("Specifying the memory address not supported by the architecture");
+#endif
+
+out:
+ cfg->ram_base = addr;
+ cfg->ram_size = size;
+
+ return 0;
+}
+
#ifndef OPT_ARCH_RUN
#define OPT_ARCH_RUN(...)
#endif
@@ -97,8 +135,11 @@ void kvm_run_set_wrapper_sandbox(void)
OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name", \
"A name for the guest"), \
OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
- OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
- " size in MB."), \
+ OPT_CALLBACK('m', "mem", cfg, \
+ "size[@addr]", \
+ "Virtual machine memory size in MB," \
+ " optionally starting at <addr>.", \
+ mem_parser, NULL), \
OPT_CALLBACK('\0', "shmem", NULL, \
"[pci:]<addr>:<size>[:handle=<handle>][:create]", \
"Share host shmem with guest via pci device", \
@@ -531,9 +572,6 @@ 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
- /* The user specifies the memory in MB. */
- 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",
diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
index e0c9ee14e103..76edb54e8bae 100644
--- a/include/kvm/kvm-config.h
+++ b/include/kvm/kvm-config.h
@@ -18,10 +18,13 @@
#define MIN_RAM_SIZE_MB (64ULL)
#define MIN_RAM_SIZE_BYTE (MIN_RAM_SIZE_MB << MB_SHIFT)
+#define INVALID_MEM_ADDR (~0ULL)
+
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_base;
u64 ram_size; /* Guest memory size, in bytes */
u8 image_count;
u8 num_net_devices;
diff --git a/kvm.c b/kvm.c
index 36b238791fc1..55a7465960b0 100644
--- a/kvm.c
+++ b/kvm.c
@@ -160,6 +160,12 @@ struct kvm *kvm__new(void)
kvm->sys_fd = -1;
kvm->vm_fd = -1;
+ /*
+ * Make sure we don't mistake the initialization value 0 for ram_base
+ * with an user specifying address 0.
+ */
+ kvm->cfg.ram_base = INVALID_MEM_ADDR;
+
#ifdef KVM_BRLOCK_DEBUG
kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER;
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address
2019-09-23 13:35 ` [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address Alexandru Elisei
@ 2019-11-07 13:54 ` Andre Przywara
2020-02-06 12:20 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-07 13:54 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:15 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> At the moment, the user can specify the amount of RAM a virtual machine
> has, which starts at the fixed address ARM_MEMORY_AREA. The memory below
> this address is used by MMIO and PCI devices.
>
> However, it might be interesting to specify a different starting address
> for the guest RAM. With this patch, the user can specify the address and
> the amount of RAM a virtual machine has from the command line by using the
> syntax -m/--mem size[@addr]. The address is optional, and must be higher or
> equal to ARM_MEMORY_AREA. If it's not specified, the old behavior is
> preserved.
>
> This option is guarded by the define ARCH_SUPPORT_CFG_RAM_BASE, and at
> the moment only the arm architecture has support for it. If an
> architecture doesn't implement this feature, then the old behavior is
> preserved and specifying the RAM base address is considered an user
> error.
>
> This patch is based upon previous work by Julien Grall.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Same thing here with "S-o-b: Julien" vs. authorship.
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/aarch32/include/kvm/kvm-arch.h | 2 +-
> arm/aarch64/include/kvm/kvm-arch.h | 6 ++---
> arm/include/arm-common/kvm-arch.h | 6 +++--
> arm/kvm.c | 17 ++++++++++----
> builtin-run.c | 48 ++++++++++++++++++++++++++++++++++----
> include/kvm/kvm-config.h | 3 +++
> kvm.c | 6 +++++
> 7 files changed, 73 insertions(+), 15 deletions(-)
>
> diff --git a/arm/aarch32/include/kvm/kvm-arch.h b/arm/aarch32/include/kvm/kvm-arch.h
> index cd31e72971bd..0aa5db40502d 100644
> --- a/arm/aarch32/include/kvm/kvm-arch.h
> +++ b/arm/aarch32/include/kvm/kvm-arch.h
> @@ -3,7 +3,7 @@
>
> #define ARM_KERN_OFFSET(...) 0x8000
>
> -#define ARM_MAX_MEMORY(...) ARM_LOMAP_MAX_MEMORY
> +#define ARM_MAX_ADDR(...) ARM_LOMAP_MAX_ADDR
>
> #include "arm-common/kvm-arch.h"
>
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 1b3d0a5fb1b4..0c58675654c5 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -5,9 +5,9 @@
> 0x8000 : \
> 0x80000)
>
> -#define ARM_MAX_MEMORY(cfg) ((cfg)->arch.aarch32_guest ? \
> - ARM_LOMAP_MAX_MEMORY : \
> - ARM_HIMAP_MAX_MEMORY)
> +#define ARM_MAX_ADDR(cfg) ((cfg)->arch.aarch32_guest ? \
> + ARM_LOMAP_MAX_ADDR : \
> + ARM_HIMAP_MAX_ADDR)
>
> #include "arm-common/kvm-arch.h"
>
> diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
> index f8f6b8f98c96..ad1a0e6872dc 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -14,8 +14,8 @@
> #define ARM_AXI_AREA _AC(0x0000000040000000, UL)
> #define ARM_MEMORY_AREA _AC(0x0000000080000000, UL)
>
> -#define ARM_LOMAP_MAX_MEMORY ((1ULL << 32) - ARM_MEMORY_AREA)
> -#define ARM_HIMAP_MAX_MEMORY ((1ULL << 40) - ARM_MEMORY_AREA)
> +#define ARM_LOMAP_MAX_ADDR (1ULL << 32)
> +#define ARM_HIMAP_MAX_ADDR (1ULL << 40)
>
> #define ARM_GIC_DIST_BASE (ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
> #define ARM_GIC_CPUI_BASE (ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
> @@ -35,6 +35,8 @@
>
> #define KVM_IOEVENTFD_HAS_PIO 0
>
> +#define ARCH_SUPPORT_CFG_RAM_BASE 1
> +
> /*
> * On a GICv3 there must be one redistributor per vCPU.
> * The value here is the size for one, we multiply this at runtime with
> diff --git a/arm/kvm.c b/arm/kvm.c
> index 3e49db7704aa..355c118b098a 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -77,7 +77,7 @@ void kvm__init_ram(struct kvm *kvm)
> madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
> MADV_HUGEPAGE);
>
> - phys_start = ARM_MEMORY_AREA;
> + phys_start = kvm->cfg.ram_base;
> phys_size = kvm->ram_size;
> host_mem = kvm->ram_start;
>
> @@ -106,8 +106,17 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
>
> void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> {
> - if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> - cfg->ram_size = ARM_MAX_MEMORY(cfg);
> + if (cfg->ram_base == INVALID_MEM_ADDR)
> + cfg->ram_base = ARM_MEMORY_AREA;
> +
> + if (cfg->ram_base < ARM_MEMORY_AREA ||
> + cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
> + cfg->ram_base = ARM_MEMORY_AREA;
> + pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
> + }
> +
> + if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
> + cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
> pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> }
Should we check for alignment of the base address here as well? It definitely needs to be at least page aligned.
But even then: -m 512@0x87654000 doesn't really go anywhere, I needed 1MB alignment for Linux to boot.
But that's probably a Linux restriction. Maybe a warning if the lower 20 bits are not zero?
> }
> @@ -229,7 +238,7 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
>
> /* For default firmware address, lets load it at the begining of RAM */
> if (fw_addr == 0)
> - fw_addr = ARM_MEMORY_AREA;
> + fw_addr = kvm->cfg.ram_base;
>
> if (!validate_fw_addr(kvm, fw_addr))
> die("Bad firmware destination: 0x%016llx", fw_addr);
> diff --git a/builtin-run.c b/builtin-run.c
> index 4e0c52b3e027..df255cc44078 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -87,6 +87,44 @@ void kvm_run_set_wrapper_sandbox(void)
> kvm_run_wrapper = KVM_RUN_SANDBOX;
> }
>
> +static int mem_parser(const struct option *opt, const char *arg, int unset)
> +{
> + struct kvm_config *cfg = opt->value;
> + const char *p = arg;
> + char *next;
> + u64 size, addr = INVALID_MEM_ADDR;
> +
> + /* Parse memory size. */
> + size = strtoll(p, &next, 0);
> + if (next == p)
> + die("Invalid memory size");
> +
> + /* The user specifies the memory in MB, we use bytes. */
> + size <<= MB_SHIFT;
> +
> + if (*next == '\0')
> + goto out;
> + else if (*next == '@')
I think coding style dictates no "else" if the "if" statement always terminates (return or goto).
> + p = next + 1;
> + else
> + die("Unexpected character after memory size: %c", *next);
And you could move this up, so that you check for bail outs first:
if (*next == '\0')
goto out;
if (*next != '@')
die();
p = next + 1;
...
> +
> + addr = strtoll(p, &next, 0);
> + if (next == p)
> + die("Invalid memory address");
> +
> +#ifndef ARCH_SUPPORT_CFG_RAM_BASE
> + if (addr != INVALID_MEM_ADDR)
Should this #ifndef cover more of the parsing routine? It looks a bit weird to first do all the work and then throw it away.
And can we somehow avoid the #ifdef at all? Maybe replacing it with a proper "if" statement? To integrate into the conditions above:
if (!IS_ENABLED(ARCH_SUPPORT_CFG_RAM_BASE) || *next != '@')
die();
Cheers,
Andre.
> + die("Specifying the memory address not supported by the architecture");
> +#endif
> +
> +out:
> + cfg->ram_base = addr;
> + cfg->ram_size = size;
> +
> + return 0;
> +}
> +
> #ifndef OPT_ARCH_RUN
> #define OPT_ARCH_RUN(...)
> #endif
> @@ -97,8 +135,11 @@ void kvm_run_set_wrapper_sandbox(void)
> OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name", \
> "A name for the guest"), \
> OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
> - OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
> - " size in MB."), \
> + OPT_CALLBACK('m', "mem", cfg, \
> + "size[@addr]", \
> + "Virtual machine memory size in MB," \
> + " optionally starting at <addr>.", \
> + mem_parser, NULL), \
> OPT_CALLBACK('\0', "shmem", NULL, \
> "[pci:]<addr>:<size>[:handle=<handle>][:create]", \
> "Share host shmem with guest via pci device", \
> @@ -531,9 +572,6 @@ 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
> - /* The user specifies the memory in MB. */
> - 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",
> diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> index e0c9ee14e103..76edb54e8bae 100644
> --- a/include/kvm/kvm-config.h
> +++ b/include/kvm/kvm-config.h
> @@ -18,10 +18,13 @@
> #define MIN_RAM_SIZE_MB (64ULL)
> #define MIN_RAM_SIZE_BYTE (MIN_RAM_SIZE_MB << MB_SHIFT)
>
> +#define INVALID_MEM_ADDR (~0ULL)
> +
> 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_base;
> u64 ram_size; /* Guest memory size, in bytes */
> u8 image_count;
> u8 num_net_devices;
> diff --git a/kvm.c b/kvm.c
> index 36b238791fc1..55a7465960b0 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -160,6 +160,12 @@ struct kvm *kvm__new(void)
> kvm->sys_fd = -1;
> kvm->vm_fd = -1;
>
> + /*
> + * Make sure we don't mistake the initialization value 0 for ram_base
> + * with an user specifying address 0.
> + */
> + kvm->cfg.ram_base = INVALID_MEM_ADDR;
> +
> #ifdef KVM_BRLOCK_DEBUG
> kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER;
> #endif
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address
2019-09-23 13:35 ` [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address Alexandru Elisei
2019-11-07 13:54 ` Andre Przywara
@ 2020-02-06 12:20 ` Suzuki Kuruppassery Poulose
1 sibling, 0 replies; 40+ messages in thread
From: Suzuki Kuruppassery Poulose @ 2020-02-06 12:20 UTC (permalink / raw)
To: Alexandru Elisei, kvm, will, julien.thierry.kdev
Cc: maz, julien.grall, andre.przywara
On 23/09/2019 14:35, Alexandru Elisei wrote:
> At the moment, the user can specify the amount of RAM a virtual machine
> has, which starts at the fixed address ARM_MEMORY_AREA. The memory below
> this address is used by MMIO and PCI devices.
>
> However, it might be interesting to specify a different starting address
> for the guest RAM. With this patch, the user can specify the address and
> the amount of RAM a virtual machine has from the command line by using the
> syntax -m/--mem size[@addr]. The address is optional, and must be higher or
> equal to ARM_MEMORY_AREA. If it's not specified, the old behavior is
> preserved.
>
> This option is guarded by the define ARCH_SUPPORT_CFG_RAM_BASE, and at
> the moment only the arm architecture has support for it. If an
> architecture doesn't implement this feature, then the old behavior is
> preserved and specifying the RAM base address is considered an user
> error.
>
> This patch is based upon previous work by Julien Grall.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> arm/aarch32/include/kvm/kvm-arch.h | 2 +-
> arm/aarch64/include/kvm/kvm-arch.h | 6 ++---
> arm/include/arm-common/kvm-arch.h | 6 +++--
> arm/kvm.c | 17 ++++++++++----
> builtin-run.c | 48 ++++++++++++++++++++++++++++++++++----
> include/kvm/kvm-config.h | 3 +++
> kvm.c | 6 +++++
> 7 files changed, 73 insertions(+), 15 deletions(-)
>
> diff --git a/arm/aarch32/include/kvm/kvm-arch.h b/arm/aarch32/include/kvm/kvm-arch.h
> index cd31e72971bd..0aa5db40502d 100644
> --- a/arm/aarch32/include/kvm/kvm-arch.h
> +++ b/arm/aarch32/include/kvm/kvm-arch.h
> @@ -3,7 +3,7 @@
>
> #define ARM_KERN_OFFSET(...) 0x8000
>
> -#define ARM_MAX_MEMORY(...) ARM_LOMAP_MAX_MEMORY
> +#define ARM_MAX_ADDR(...) ARM_LOMAP_MAX_ADDR
>
> #include "arm-common/kvm-arch.h"
>
>
> void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
> {
> - if (cfg->ram_size > ARM_MAX_MEMORY(cfg)) {
> - cfg->ram_size = ARM_MAX_MEMORY(cfg);
> + if (cfg->ram_base == INVALID_MEM_ADDR)
> + cfg->ram_base = ARM_MEMORY_AREA;
> +
> + if (cfg->ram_base < ARM_MEMORY_AREA ||
> + cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
> + cfg->ram_base = ARM_MEMORY_AREA;
> + pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
> + }
> +
> + if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
> + cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
> pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
> }
> }
> @@ -229,7 +238,7 @@ bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename)
>
> /* For default firmware address, lets load it at the begining of RAM */
> if (fw_addr == 0)
> - fw_addr = ARM_MEMORY_AREA;
> + fw_addr = kvm->cfg.ram_base;
At this time, we have kvm->arch.memory_guest_start set. Even though they
both might be the same, I think kvm->arch.memory_guest_start is safer
here.
>
> if (!validate_fw_addr(kvm, fw_addr))
> die("Bad firmware destination: 0x%016llx", fw_addr);
> diff --git a/builtin-run.c b/builtin-run.c
> index 4e0c52b3e027..df255cc44078 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -87,6 +87,44 @@ void kvm_run_set_wrapper_sandbox(void)
> kvm_run_wrapper = KVM_RUN_SANDBOX;
> }
>
> +static int mem_parser(const struct option *opt, const char *arg, int unset)
> +{
> + struct kvm_config *cfg = opt->value;
> + const char *p = arg;
> + char *next;
> + u64 size, addr = INVALID_MEM_ADDR;
> +
> + /* Parse memory size. */
> + size = strtoll(p, &next, 0);
> + if (next == p)
> + die("Invalid memory size");
> +
> + /* The user specifies the memory in MB, we use bytes. */
> + size <<= MB_SHIFT;
> +
> + if (*next == '\0')
> + goto out;
> + else if (*next == '@')
> + p = next + 1;
> + else
> + die("Unexpected character after memory size: %c", *next);
> +
> + addr = strtoll(p, &next, 0);
> + if (next == p)
> + die("Invalid memory address");
Could we use "memory base address" to be explicit ?
> +
> +#ifndef ARCH_SUPPORT_CFG_RAM_BASE
> + if (addr != INVALID_MEM_ADDR)
> + die("Specifying the memory address not supported by the architecture");
Same here ^
> +#endif
> +
> +out:
> + cfg->ram_base = addr;
> + cfg->ram_size = size;
> +
> + return 0;
> +}
> +
> #ifndef OPT_ARCH_RUN
> #define OPT_ARCH_RUN(...)
> #endif
> @@ -97,8 +135,11 @@ void kvm_run_set_wrapper_sandbox(void)
> OPT_STRING('\0', "name", &(cfg)->guest_name, "guest name", \
> "A name for the guest"), \
> OPT_INTEGER('c', "cpus", &(cfg)->nrcpus, "Number of CPUs"), \
> - OPT_U64('m', "mem", &(cfg)->ram_size, "Virtual machine memory" \
> - " size in MB."), \
> + OPT_CALLBACK('m', "mem", cfg, \
> + "size[@addr]", \
> + "Virtual machine memory size in MB," \
> + " optionally starting at <addr>.", \
> + mem_parser, NULL), \
Given that we only support this option for archs who opt-in, could we
keep the "help" message consistent with what is built ?
#ifdef ARCH_SUPPORT_CFG_RAM_BASE
#define MEM_OPT_HELP_SHORT "size[@addr]"
#define MEM_OPT_HELP_DESC \
"Virtual machine memory size in MB, optionally start at <addr>"
#else
#define MEM_OPT_HELP_SHORT "size"
#define MEM_OPT_HELP_DESC "Virtual machine memory size in MB."
#endif
Or some other means, so that the help text is consistent with what
we build.
Rest looks fine to me.
Suzuki
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (8 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 09/16] arm: Allow the user to specify RAM base address Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-11-07 13:55 ` Andre Przywara
2019-09-23 13:35 ` [PATCH kvmtool 11/16] arm/pci: Remove unused ioports Alexandru Elisei
` (6 subsequent siblings)
16 siblings, 1 reply; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
From: Suzuki K Poulose <suzuki.poulose@arm.com>
Allow standard suffixes, K, M, G, T & P suffixes (lowercase and uppercase)
for sizes and addresses for memory bank parameters. By default, the size is
specified in MB.
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
builtin-run.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 49 insertions(+), 6 deletions(-)
diff --git a/builtin-run.c b/builtin-run.c
index df255cc44078..757ede4ac0d1 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -49,9 +49,11 @@
#include <ctype.h>
#include <stdio.h>
-#define MB_SHIFT (20)
#define KB_SHIFT (10)
+#define MB_SHIFT (20)
#define GB_SHIFT (30)
+#define TB_SHIFT (40)
+#define PB_SHIFT (50)
__thread struct kvm_cpu *current_kvm_cpu;
@@ -87,6 +89,48 @@ void kvm_run_set_wrapper_sandbox(void)
kvm_run_wrapper = KVM_RUN_SANDBOX;
}
+static int parse_unit(char **next)
+{
+ int shift = 0;
+
+ switch(**next) {
+ case 'K': case 'k': shift = KB_SHIFT; break;
+ case 'M': case 'm': shift = MB_SHIFT; break;
+ case 'G': case 'g': shift = GB_SHIFT; break;
+ case 'T': case 't': shift = TB_SHIFT; break;
+ case 'P': case 'p': shift = PB_SHIFT; break;
+ }
+
+ if (shift)
+ (*next)++;
+
+ return shift;
+}
+
+static u64 parse_size(char **next)
+{
+ int shift = parse_unit(next);
+
+ /* By default the size is in MB, if none is specified. */
+ if (!shift)
+ shift = 20;
+
+ while (**next != '\0' && **next != '@')
+ (*next)++;
+
+ return ((u64)1) << shift;
+}
+
+static u64 parse_addr(char **next)
+{
+ int shift = parse_unit(next);
+
+ while (**next != '\0')
+ (*next)++;
+
+ return ((u64)1) << shift;
+}
+
static int mem_parser(const struct option *opt, const char *arg, int unset)
{
struct kvm_config *cfg = opt->value;
@@ -99,15 +143,12 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
if (next == p)
die("Invalid memory size");
- /* The user specifies the memory in MB, we use bytes. */
- size <<= MB_SHIFT;
+ size *= parse_size(&next);
if (*next == '\0')
goto out;
- else if (*next == '@')
- p = next + 1;
else
- die("Unexpected character after memory size: %c", *next);
+ p = next + 1;
addr = strtoll(p, &next, 0);
if (next == p)
@@ -118,6 +159,8 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
die("Specifying the memory address not supported by the architecture");
#endif
+ addr *= parse_addr(&next);
+
out:
cfg->ram_base = addr;
cfg->ram_size = size;
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory
2019-09-23 13:35 ` [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory Alexandru Elisei
@ 2019-11-07 13:55 ` Andre Przywara
0 siblings, 0 replies; 40+ messages in thread
From: Andre Przywara @ 2019-11-07 13:55 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, Will Deacon, Julien Thierry, Marc Zyngier, Suzuki Poulose,
Julien Grall
On Mon, 23 Sep 2019 14:35:16 +0100
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
Hi,
> From: Suzuki K Poulose <suzuki.poulose@arm.com>
>
> Allow standard suffixes, K, M, G, T & P suffixes (lowercase and uppercase)
> for sizes and addresses for memory bank parameters. By default, the size is
> specified in MB.
>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
> builtin-run.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/builtin-run.c b/builtin-run.c
> index df255cc44078..757ede4ac0d1 100644
> --- a/builtin-run.c
> +++ b/builtin-run.c
> @@ -49,9 +49,11 @@
> #include <ctype.h>
> #include <stdio.h>
>
> -#define MB_SHIFT (20)
> #define KB_SHIFT (10)
> +#define MB_SHIFT (20)
> #define GB_SHIFT (30)
> +#define TB_SHIFT (40)
> +#define PB_SHIFT (50)
>
> __thread struct kvm_cpu *current_kvm_cpu;
>
> @@ -87,6 +89,48 @@ void kvm_run_set_wrapper_sandbox(void)
> kvm_run_wrapper = KVM_RUN_SANDBOX;
> }
>
> +static int parse_unit(char **next)
> +{
> + int shift = 0;
> +
> + switch(**next) {
> + case 'K': case 'k': shift = KB_SHIFT; break;
> + case 'M': case 'm': shift = MB_SHIFT; break;
> + case 'G': case 'g': shift = GB_SHIFT; break;
> + case 'T': case 't': shift = TB_SHIFT; break;
> + case 'P': case 'p': shift = PB_SHIFT; break;
> + }
> +
> + if (shift)
> + (*next)++;
> +
> + return shift;
> +}
> +
> +static u64 parse_size(char **next)
> +{
> + int shift = parse_unit(next);
> +
> + /* By default the size is in MB, if none is specified. */
> + if (!shift)
> + shift = 20;
> +
> + while (**next != '\0' && **next != '@')
> + (*next)++;
Mmh, doesn't that skip over invalid characters, which should be reported as an error? Like "12Three"? Why do we need to skip something here anyway?
> +
> + return ((u64)1) << shift;
Is there any reason we don't just return the shift value back? Seems more efficient to me.
> +}
> +
> +static u64 parse_addr(char **next)
> +{
> + int shift = parse_unit(next);
> +
> + while (**next != '\0')
> + (*next)++;
Same here, why do we skip characters? Especially since we check for \0 in the caller below.
Cheers,
Andre
> +
> + return ((u64)1) << shift;
> +}
> +
> static int mem_parser(const struct option *opt, const char *arg, int unset)
> {
> struct kvm_config *cfg = opt->value;
> @@ -99,15 +143,12 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
> if (next == p)
> die("Invalid memory size");
>
> - /* The user specifies the memory in MB, we use bytes. */
> - size <<= MB_SHIFT;
> + size *= parse_size(&next);
>
> if (*next == '\0')
> goto out;
> - else if (*next == '@')
> - p = next + 1;
> else
> - die("Unexpected character after memory size: %c", *next);
> + p = next + 1;
>
> addr = strtoll(p, &next, 0);
> if (next == p)
> @@ -118,6 +159,8 @@ static int mem_parser(const struct option *opt, const char *arg, int unset)
> die("Specifying the memory address not supported by the architecture");
> #endif
>
> + addr *= parse_addr(&next);
> +
> out:
> cfg->ram_base = addr;
> cfg->ram_size = size;
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH kvmtool 11/16] arm/pci: Remove unused ioports
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (9 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 10/16] kvmtool: Allow standard size specifiers for memory Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 12/16] Fold kvm__init_ram call in kvm__arch_init and rename it Alexandru Elisei
` (5 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
ARM does not use the PCI_CONFIG_DATA and PCI_CONFIG_ADDRESS I/O ports and
has no way of knowing about them, since they are not generated in the DTB
file. Do not register these I/O addresses for the ARM architecture.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
pci.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/pci.c b/pci.c
index 689869cb79a3..88ee78ad7d08 100644
--- a/pci.c
+++ b/pci.c
@@ -68,7 +68,9 @@ static void *pci_config_address_ptr(u16 port)
return base + offset;
}
-static bool pci_config_address_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool __used pci_config_address_out(struct ioport *ioport,
+ struct kvm_cpu *vcpu,
+ u16 port, void *data, int size)
{
void *p = pci_config_address_ptr(port);
@@ -77,7 +79,9 @@ static bool pci_config_address_out(struct ioport *ioport, struct kvm_cpu *vcpu,
return true;
}
-static bool pci_config_address_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool __used pci_config_address_in(struct ioport *ioport,
+ struct kvm_cpu *vcpu,
+ u16 port, void *data, int size)
{
void *p = pci_config_address_ptr(port);
@@ -86,7 +90,7 @@ static bool pci_config_address_in(struct ioport *ioport, struct kvm_cpu *vcpu, u
return true;
}
-static struct ioport_operations pci_config_address_ops = {
+static struct ioport_operations __used pci_config_address_ops = {
.io_in = pci_config_address_in,
.io_out = pci_config_address_out,
};
@@ -106,7 +110,9 @@ static bool pci_device_exists(u8 bus_number, u8 device_number, u8 function_numbe
return !IS_ERR_OR_NULL(device__find_dev(DEVICE_BUS_PCI, device_number));
}
-static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool __used pci_config_data_out(struct ioport *ioport,
+ struct kvm_cpu *vcpu,
+ u16 port, void *data, int size)
{
union pci_config_address pci_config_address;
@@ -122,7 +128,9 @@ static bool pci_config_data_out(struct ioport *ioport, struct kvm_cpu *vcpu, u16
return true;
}
-static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16 port, void *data, int size)
+static bool __used pci_config_data_in(struct ioport *ioport,
+ struct kvm_cpu *vcpu,
+ u16 port, void *data, int size)
{
union pci_config_address pci_config_address;
@@ -138,7 +146,7 @@ static bool pci_config_data_in(struct ioport *ioport, struct kvm_cpu *vcpu, u16
return true;
}
-static struct ioport_operations pci_config_data_ops = {
+static struct ioport_operations __used pci_config_data_ops = {
.io_in = pci_config_data_in,
.io_out = pci_config_data_out,
};
@@ -225,6 +233,13 @@ struct pci_device_header *pci__find_dev(u8 dev_num)
return hdr->data;
}
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+int pci__init(struct kvm *kvm)
+{
+ return kvm__register_mmio(kvm, KVM_PCI_CFG_AREA, PCI_CFG_SIZE, false,
+ pci_config_mmio_access, kvm);
+}
+#else
int pci__init(struct kvm *kvm)
{
int r;
@@ -250,6 +265,7 @@ err_unregister_data:
ioport__unregister(kvm, PCI_CONFIG_DATA);
return r;
}
+#endif
dev_base_init(pci__init);
int pci__exit(struct kvm *kvm)
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH kvmtool 12/16] Fold kvm__init_ram call in kvm__arch_init and rename it
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (10 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 11/16] arm/pci: Remove unused ioports Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 13/16] arm: Allow any base address for RAM Alexandru Elisei
` (4 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
From: Julien Grall <julien.grall@arm.com>
When we will add support for allocating the MMIO memory dynamically, we
will need to initialize the memory before the irqchip. Move the
kvm__init_ram call in kvm__arch_init so we can be flexible in the future
with the regards to when we call it.
kvm__init_ram isn't a globally visible function anymore, so rename it to
init_ram.
Note that it is necessary to move the call to kvm__arch_init after the
initialization of the list kvm->mem_banks because kvm__init_ram was
relying on it.
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/kvm.c | 4 +++-
include/kvm/kvm.h | 1 -
kvm.c | 4 +---
mips/kvm.c | 4 +++-
powerpc/kvm.c | 4 +++-
x86/kvm.c | 6 ++++--
6 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/arm/kvm.c b/arm/kvm.c
index 355c118b098a..138ef5763cc2 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -24,7 +24,7 @@ bool kvm__arch_cpu_supports_vm(void)
return true;
}
-void kvm__init_ram(struct kvm *kvm)
+static void init_ram(struct kvm *kvm)
{
int err;
u64 phys_start, phys_size;
@@ -126,6 +126,8 @@ void kvm__arch_init(struct kvm *kvm)
/* Create the virtual GIC. */
if (gic__create(kvm, kvm->cfg.arch.irqchip))
die("Failed to create virtual GIC");
+
+ init_ram(kvm);
}
#define FDT_ALIGN SZ_2M
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index a866d5a825c4..8787a92b4dbb 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -94,7 +94,6 @@ int kvm__init(struct kvm *kvm);
struct kvm *kvm__new(void);
int kvm__recommended_cpus(struct kvm *kvm);
int kvm__max_cpus(struct kvm *kvm);
-void kvm__init_ram(struct kvm *kvm);
int kvm__exit(struct kvm *kvm);
bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename);
bool kvm__load_kernel(struct kvm *kvm, const char *kernel_filename,
diff --git a/kvm.c b/kvm.c
index 55a7465960b0..4da413e0681d 100644
--- a/kvm.c
+++ b/kvm.c
@@ -398,10 +398,8 @@ int kvm__init(struct kvm *kvm)
goto err_vm_fd;
}
- kvm__arch_init(kvm);
-
INIT_LIST_HEAD(&kvm->mem_banks);
- kvm__init_ram(kvm);
+ kvm__arch_init(kvm);
if (!kvm->cfg.firmware_filename) {
if (!kvm__load_kernel(kvm, kvm->cfg.kernel_filename,
diff --git a/mips/kvm.c b/mips/kvm.c
index 63d651f29f70..54f1c134a9d7 100644
--- a/mips/kvm.c
+++ b/mips/kvm.c
@@ -17,7 +17,7 @@ void kvm__arch_read_term(struct kvm *kvm)
virtio_console__inject_interrupt(kvm);
}
-void kvm__init_ram(struct kvm *kvm)
+static void init_ram(struct kvm *kvm)
{
u64 phys_start, phys_size;
void *host_mem;
@@ -80,6 +80,8 @@ void kvm__arch_init(struct kvm *kvm)
ret = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
if (ret < 0)
die_perror("KVM_CREATE_IRQCHIP ioctl");
+
+ init_ram(kvm);
}
void kvm__irq_line(struct kvm *kvm, int irq, int level)
diff --git a/powerpc/kvm.c b/powerpc/kvm.c
index 73965640cf82..3a5e11eee806 100644
--- a/powerpc/kvm.c
+++ b/powerpc/kvm.c
@@ -60,7 +60,7 @@ bool kvm__arch_cpu_supports_vm(void)
return true;
}
-void kvm__init_ram(struct kvm *kvm)
+static void init_ram(struct kvm *kvm)
{
u64 phys_start, phys_size;
void *host_mem;
@@ -144,6 +144,8 @@ void kvm__arch_init(struct kvm *kvm)
SPAPR_PCI_MEM_WIN_SIZE,
SPAPR_PCI_IO_WIN_ADDR,
SPAPR_PCI_IO_WIN_SIZE);
+
+ init_ram(kvm);
}
void kvm__arch_delete_ram(struct kvm *kvm)
diff --git a/x86/kvm.c b/x86/kvm.c
index df5d48106c80..2627fcb959b5 100644
--- a/x86/kvm.c
+++ b/x86/kvm.c
@@ -86,7 +86,7 @@ bool kvm__arch_cpu_supports_vm(void)
* a gap between 0xe0000000 and 0x100000000 in the guest virtual mem space.
*/
-void kvm__init_ram(struct kvm *kvm)
+static void init_ram(struct kvm *kvm)
{
u64 phys_start, phys_size;
void *host_mem;
@@ -165,7 +165,7 @@ void kvm__arch_init(struct kvm *kvm)
kvm->ram_size = ram_size + KVM_32BIT_GAP_SIZE;
if (kvm->ram_start != MAP_FAILED)
/*
- * We mprotect the gap (see kvm__init_ram() for details) PROT_NONE so that
+ * We mprotect the gap (see init_ram() for details) PROT_NONE so that
* if we accidently write to it, we will know.
*/
mprotect(kvm->ram_start + KVM_32BIT_GAP_START, KVM_32BIT_GAP_SIZE, PROT_NONE);
@@ -178,6 +178,8 @@ void kvm__arch_init(struct kvm *kvm)
ret = ioctl(kvm->vm_fd, KVM_CREATE_IRQCHIP);
if (ret < 0)
die_perror("KVM_CREATE_IRQCHIP ioctl");
+
+ init_ram(kvm);
}
void kvm__arch_delete_ram(struct kvm *kvm)
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH kvmtool 13/16] arm: Allow any base address for RAM
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (11 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 12/16] Fold kvm__init_ram call in kvm__arch_init and rename it Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 14/16] arm: Move memory related code to memory.c Alexandru Elisei
` (3 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
The ARM memory layout is fixed: the guest RAM must start at an address
higher or equal to 2G, the ioport area is a between 0 and 64K, the MMIO
space lives above it and PCI is situated between 1G and 2G. However, when
it comes to real hardware, there is a wide range of different memory
layouts.
Let's be more flexible with the way we construct the memory layout for the
guest. Introduce a memory allocator which will be used for allocating
chunks from the guest address space for the different memory regions based
on their size and remaining memory.
Special care has been taken to keep the current behavior as much as
possible. The memory allocator allocates consecutive memory chunks where
possible, and if the guest RAM is situated above 2G, the current layout is
mostly preserved. The only difference is in the location of the GIC
components: instead of being at the top of the MMIO address space
(immediately below 1G), they are now at the bottom (directly above the
ioport space).
Because of the default sizes for the various MMIO regions, there are
limitations to what an user can specify. The most restrictive requirement
is that there needs to be enough space in the lower 4G memory region to
allocate one contiguous chunks of approximately 1G for MMIO and another one
for PCI MMIO.
We've also taken this opportunity to remove the unused include assert.h
from pci.c.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
Makefile | 2 +-
arm/allocator.c | 150 +++++++++++++++++++++++++++++++++++++
arm/gic.c | 30 +++++---
arm/include/arm-common/allocator.h | 25 +++++++
arm/include/arm-common/kvm-arch.h | 52 ++++++-------
arm/kvm.c | 94 ++++++++++++++++++++++-
pci.c | 8 +-
virtio/mmio.c | 7 ++
8 files changed, 324 insertions(+), 44 deletions(-)
create mode 100644 arm/allocator.c
create mode 100644 arm/include/arm-common/allocator.h
diff --git a/Makefile b/Makefile
index b76d844f2e01..0ae52c6ad88b 100644
--- a/Makefile
+++ b/Makefile
@@ -158,7 +158,7 @@ endif
# ARM
OBJS_ARM_COMMON := arm/fdt.o arm/gic.o arm/gicv2m.o arm/ioport.o \
arm/kvm.o arm/kvm-cpu.o arm/pci.o arm/timer.o \
- arm/pmu.o
+ arm/pmu.o arm/allocator.o
HDRS_ARM_COMMON := arm/include
ifeq ($(ARCH), arm)
DEFINES += -DCONFIG_ARM
diff --git a/arm/allocator.c b/arm/allocator.c
new file mode 100644
index 000000000000..301f4975c305
--- /dev/null
+++ b/arm/allocator.c
@@ -0,0 +1,150 @@
+#include "kvm/kvm.h"
+#include "kvm/virtio-mmio.h"
+
+#include "arm-common/allocator.h"
+
+#include <linux/list.h>
+#include <linux/sizes.h>
+
+#include <stdlib.h>
+#include <assert.h>
+
+struct mem_region {
+ u64 start;
+ u64 size;
+ struct list_head list;
+};
+
+struct allocator *allocator_create(u64 start, u64 size)
+{
+ struct allocator *allocator;
+ struct mem_region *chunk;
+
+ allocator = calloc(1, sizeof(*allocator));
+ if (!allocator)
+ die_perror("calloc");
+
+ chunk = calloc(1, sizeof(*chunk));
+ if (!chunk)
+ die_perror("calloc");
+
+ assert(size != 0);
+
+ allocator->start = start;
+ allocator->size = size;
+ INIT_LIST_HEAD(&allocator->freelist);
+
+ chunk->start = start;
+ chunk->size = size;
+ list_add(&chunk->list, &allocator->freelist);
+
+ return allocator;
+}
+
+int allocator_reserve(struct allocator *allocator,
+ u64 start, u64 size)
+{
+ struct mem_region *pos;
+ struct mem_region *chunk = NULL;
+ struct mem_region *new_chunk;
+ unsigned long limit;
+
+ if (start < allocator->start ||
+ start + size > allocator->start + allocator->size)
+ return 1;
+
+ limit = start + size;
+
+ list_for_each_entry(pos, &allocator->freelist, list)
+ if (start >= pos->start && limit <= pos->start + pos->size) {
+ chunk = pos;
+ break;
+ }
+
+ if (!chunk)
+ return 1;
+
+ if (start == chunk->start) {
+ chunk->size = chunk->start + chunk->size - limit;
+ chunk->start = limit;
+
+ if (chunk->size == 0) {
+ list_del(&chunk->list);
+ free(chunk);
+ }
+ } else {
+ new_chunk = calloc(1, sizeof(*chunk));
+ if (!new_chunk)
+ die_perror("calloc");
+ new_chunk->start = limit;
+ new_chunk->size = chunk->start + chunk->size - limit;
+ list_add(&new_chunk->list, &chunk->list);
+
+ chunk->size = start - chunk->start;
+ }
+
+ return 0;
+}
+
+u64 allocator_alloc_align(struct allocator *allocator,
+ u64 size, unsigned long alignment)
+{
+ struct mem_region *pos;
+ struct mem_region *chunk = NULL;
+ unsigned long start, limit;
+ unsigned long addr;
+
+ list_for_each_entry(pos, &allocator->freelist, list) {
+ start = ALIGN(pos->start, alignment);
+ limit = start + size;
+ if (limit <= pos->start + pos->size) {
+ chunk = pos;
+ break;
+ }
+ }
+
+ if (!chunk)
+ return ALLOC_INVALID_ADDR;
+
+ addr = start;
+
+ chunk->size = chunk->start + chunk->size - limit;
+ chunk->start = limit;
+
+ if (chunk->size == 0) {
+ list_del(&pos->list);
+ free(pos);
+ }
+
+ return addr;
+}
+
+u64 allocator_alloc(struct allocator *allocator, u64 size)
+{
+ return allocator_alloc_align(allocator, size, ALLOC_PAGE_SIZE);
+}
+
+void allocator_destroy(struct allocator *allocator)
+{
+ struct mem_region *pos, *n;
+
+ list_for_each_entry_safe(pos, n, &allocator->freelist, list) {
+ list_del(&pos->list);
+ free(pos);
+ }
+
+ free(allocator);
+}
+
+void allocator_print(struct allocator *allocator)
+{
+ struct mem_region *pos;
+ int i = 0;
+
+ fprintf(stderr, "Allocator [0x%llx@0x%llx]:\n",
+ allocator->size, allocator->start);
+ list_for_each_entry(pos, &allocator->freelist, list) {
+ fprintf(stderr, "%3d: 0x%8llx@0x%-8llx\n", i, pos->size, pos->start);
+ i++;
+ }
+}
diff --git a/arm/gic.c b/arm/gic.c
index 26be4b4c650b..ab3245e53cab 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -13,6 +13,8 @@
#define IRQCHIP_GIC 0
static int gic_fd = -1;
+static u64 gic_dist_base;
+static u64 gic_cpu_if_base;
static u64 gic_redists_base;
static u64 gic_redists_size;
static u64 gic_msi_base;
@@ -151,19 +153,17 @@ static int gic__create_msi_frame(struct kvm *kvm, enum irqchip_type type,
static int gic__create_device(struct kvm *kvm, enum irqchip_type type)
{
int err;
- u64 cpu_if_addr = ARM_GIC_CPUI_BASE;
- u64 dist_addr = ARM_GIC_DIST_BASE;
struct kvm_create_device gic_device = {
.flags = 0,
};
struct kvm_device_attr cpu_if_attr = {
.group = KVM_DEV_ARM_VGIC_GRP_ADDR,
.attr = KVM_VGIC_V2_ADDR_TYPE_CPU,
- .addr = (u64)(unsigned long)&cpu_if_addr,
+ .addr = (u64)(unsigned long)&gic_cpu_if_base,
};
struct kvm_device_attr dist_attr = {
.group = KVM_DEV_ARM_VGIC_GRP_ADDR,
- .addr = (u64)(unsigned long)&dist_addr,
+ .addr = (u64)(unsigned long)&gic_dist_base,
};
struct kvm_device_attr redist_attr = {
.group = KVM_DEV_ARM_VGIC_GRP_ADDR,
@@ -230,12 +230,12 @@ static int gic__create_irqchip(struct kvm *kvm)
[0] = {
.id = KVM_VGIC_V2_ADDR_TYPE_DIST |
(KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT),
- .addr = ARM_GIC_DIST_BASE,
+ .addr = gic_dist_base,
},
[1] = {
.id = KVM_VGIC_V2_ADDR_TYPE_CPU |
(KVM_ARM_DEVICE_VGIC_V2 << KVM_ARM_DEVICE_ID_SHIFT),
- .addr = ARM_GIC_CPUI_BASE,
+ .addr = gic_cpu_if_base,
}
};
@@ -256,6 +256,10 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
enum irqchip_type try;
int err;
+ gic_dist_base = kvm__arch_alloc_mmio_block(ARM_GIC_DIST_SIZE);
+ if (type == IRQCHIP_GICV2M || type == IRQCHIP_GICV2)
+ gic_cpu_if_base = kvm__arch_alloc_mmio_block(ARM_GIC_CPUI_SIZE);
+
switch (type) {
case IRQCHIP_AUTO:
for (try = IRQCHIP_GICV3_ITS; try >= IRQCHIP_GICV2; try--) {
@@ -270,18 +274,20 @@ int gic__create(struct kvm *kvm, enum irqchip_type type)
return 0;
case IRQCHIP_GICV2M:
gic_msi_size = KVM_VGIC_V2M_SIZE;
- gic_msi_base = ARM_GIC_CPUI_BASE - gic_msi_size;
+ gic_msi_base = kvm__arch_alloc_mmio_block(gic_msi_size);
break;
case IRQCHIP_GICV2:
break;
case IRQCHIP_GICV3_ITS:
- /* The 64K page with the doorbell is included. */
gic_msi_size = KVM_VGIC_V3_ITS_SIZE;
/* fall through */
case IRQCHIP_GICV3:
gic_redists_size = kvm->cfg.nrcpus * ARM_GIC_REDIST_SIZE;
- gic_redists_base = ARM_GIC_DIST_BASE - gic_redists_size;
- gic_msi_base = gic_redists_base - gic_msi_size;
+ gic_redists_base = kvm__arch_alloc_mmio_block(gic_redists_size);
+ if (gic_msi_size == 0)
+ gic_msi_base = gic_redists_base;
+ else
+ gic_msi_base = kvm__arch_alloc_mmio_block(gic_msi_size);
break;
default:
return -ENODEV;
@@ -349,7 +355,7 @@ void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type)
const char *compatible, *msi_compatible = NULL;
u64 msi_prop[2];
u64 reg_prop[] = {
- cpu_to_fdt64(ARM_GIC_DIST_BASE), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
+ cpu_to_fdt64(gic_dist_base), cpu_to_fdt64(ARM_GIC_DIST_SIZE),
0, 0, /* to be filled */
};
@@ -359,7 +365,7 @@ void gic__generate_fdt_nodes(void *fdt, enum irqchip_type type)
/* fall-through */
case IRQCHIP_GICV2:
compatible = "arm,cortex-a15-gic";
- reg_prop[2] = cpu_to_fdt64(ARM_GIC_CPUI_BASE);
+ reg_prop[2] = cpu_to_fdt64(gic_cpu_if_base);
reg_prop[3] = cpu_to_fdt64(ARM_GIC_CPUI_SIZE);
break;
case IRQCHIP_GICV3_ITS:
diff --git a/arm/include/arm-common/allocator.h b/arm/include/arm-common/allocator.h
new file mode 100644
index 000000000000..6b649905907a
--- /dev/null
+++ b/arm/include/arm-common/allocator.h
@@ -0,0 +1,25 @@
+#ifndef ARM_COMMON_ALLOCATOR_H
+#define ARM_COMMON_ALLOCATOR_H
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#define ALLOC_PAGE_SIZE SZ_64K
+#define ALLOC_INVALID_ADDR (~0ul)
+
+struct allocator {
+ struct list_head freelist;
+ u64 start;
+ u64 size;
+};
+
+struct allocator *allocator_create(u64 start, u64 size);
+void allocator_destroy(struct allocator *allocator);
+void allocator_print(struct allocator *allocator);
+int allocator_reserve(struct allocator *allocator, u64 start, u64 size);
+u64 allocator_alloc_align(struct allocator *allocator, u64 size,
+ unsigned long alignment);
+u64 allocator_alloc(struct allocator *allocator, u64 size);
+
+#endif
diff --git a/arm/include/arm-common/kvm-arch.h b/arm/include/arm-common/kvm-arch.h
index ad1a0e6872dc..a058c550b902 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -4,46 +4,43 @@
#include <stdbool.h>
#include <linux/const.h>
#include <linux/types.h>
+#include <linux/sizes.h>
#include "kvm/pci.h"
#include "arm-common/gic.h"
-#define ARM_IOPORT_AREA _AC(0x0000000000000000, UL)
-#define ARM_MMIO_AREA _AC(0x0000000000010000, UL)
-#define ARM_AXI_AREA _AC(0x0000000040000000, UL)
-#define ARM_MEMORY_AREA _AC(0x0000000080000000, UL)
+extern u64 ioport_addr;
+extern u64 mmio_addr;
+extern u64 pci_cfg_addr;
+extern u64 pci_mmio_addr;
-#define ARM_LOMAP_MAX_ADDR (1ULL << 32)
-#define ARM_HIMAP_MAX_ADDR (1ULL << 40)
+#define ARM_IOPORT_SIZE SZ_64K
+#define ARM_MMIO_SIZE (SZ_1G - ARM_IOPORT_SIZE)
+#define ARM_PCI_MMIO_SIZE (SZ_1G - PCI_CFG_SIZE)
-#define ARM_GIC_DIST_BASE (ARM_AXI_AREA - ARM_GIC_DIST_SIZE)
-#define ARM_GIC_CPUI_BASE (ARM_GIC_DIST_BASE - ARM_GIC_CPUI_SIZE)
-#define ARM_GIC_SIZE (ARM_GIC_DIST_SIZE + ARM_GIC_CPUI_SIZE)
-#define ARM_GIC_DIST_SIZE 0x10000
-#define ARM_GIC_CPUI_SIZE 0x20000
+#define ARM_GIC_DIST_SIZE SZ_64K
+#define ARM_GIC_CPUI_SIZE SZ_128K
+/*
+ * On a GICv3 there must be one redistributor per vCPU.
+ * The value here is the size for one, we multiply this at runtime with
+ * the number of requested vCPUs to get the actual size.
+ */
+#define ARM_GIC_REDIST_SIZE SZ_128K
-#define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
-#define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
-#define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
- (ARM_AXI_AREA + PCI_CFG_SIZE))
+#define KVM_IOPORT_AREA ioport_addr
+#define KVM_MMIO_AREA mmio_addr
+#define KVM_PCI_CFG_AREA pci_cfg_addr
+#define KVM_PCI_MMIO_AREA pci_mmio_addr
-#define KVM_IOPORT_AREA ARM_IOPORT_AREA
-#define KVM_PCI_CFG_AREA ARM_AXI_AREA
-#define KVM_PCI_MMIO_AREA (KVM_PCI_CFG_AREA + PCI_CFG_SIZE)
-#define KVM_VIRTIO_MMIO_AREA ARM_MMIO_AREA
+#define ARM_MEMORY_AREA SZ_2G
+#define ARM_LOMAP_MAX_ADDR (1ULL << 32)
+#define ARM_HIMAP_MAX_ADDR (1ULL << 40)
#define KVM_IOEVENTFD_HAS_PIO 0
#define ARCH_SUPPORT_CFG_RAM_BASE 1
-/*
- * On a GICv3 there must be one redistributor per vCPU.
- * The value here is the size for one, we multiply this at runtime with
- * the number of requested vCPUs to get the actual size.
- */
-#define ARM_GIC_REDIST_SIZE 0x20000
-
#define KVM_IRQ_OFFSET GIC_SPI_IRQ_BASE
#define KVM_VM_TYPE 0
@@ -81,4 +78,7 @@ struct kvm_config;
void kvm__arch_sanitize_cfg(struct kvm_config *cfg);
+u32 kvm__arch_alloc_mmio_block(u32 size);
+u32 kvm__arch_alloc_pci_mmio_block(u32 size);
+
#endif /* ARM_COMMON__KVM_ARCH_H */
diff --git a/arm/kvm.c b/arm/kvm.c
index 138ef5763cc2..a12a75d94cdd 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -4,13 +4,17 @@
#include "kvm/8250-serial.h"
#include "kvm/virtio-console.h"
#include "kvm/fdt.h"
+#include "kvm/pci.h"
#include "arm-common/gic.h"
+#include "arm-common/allocator.h"
#include <linux/kernel.h>
#include <linux/kvm.h>
#include <linux/sizes.h>
+#include <assert.h>
+
struct kvm_ext kvm_req_ext[] = {
{ DEFINE_KVM_EXT(KVM_CAP_IRQCHIP) },
{ DEFINE_KVM_EXT(KVM_CAP_ONE_REG) },
@@ -18,12 +22,57 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 },
};
+u64 ioport_addr = ALLOC_INVALID_ADDR;
+u64 mmio_addr = ALLOC_INVALID_ADDR;
+u64 pci_cfg_addr = ALLOC_INVALID_ADDR;
+u64 pci_mmio_addr = ALLOC_INVALID_ADDR;
+
+static struct allocator *lomap_allocator;
+static struct allocator *mmio_allocator;
+static struct allocator *pci_mmio_allocator;
+
bool kvm__arch_cpu_supports_vm(void)
{
/* The KVM capability check is enough. */
return true;
}
+static void init_mmio_mem(struct kvm *kvm)
+{
+ u64 reserve;
+ int ret;
+
+ /* Allocate MMIO memory below 4G so 32 bit arm can use it. */
+ lomap_allocator = allocator_create(0, ARM_LOMAP_MAX_ADDR);
+
+ if (kvm->arch.memory_guest_start + kvm->ram_size > ARM_LOMAP_MAX_ADDR)
+ reserve = ARM_LOMAP_MAX_ADDR - kvm->arch.memory_guest_start;
+ else
+ reserve = kvm->ram_size;
+ ret = allocator_reserve(lomap_allocator, kvm->arch.memory_guest_start,
+ reserve);
+ if (ret)
+ die("Could not reserve RAM address space");
+
+ ioport_addr = allocator_alloc(lomap_allocator, ARM_IOPORT_SIZE);
+ if (ioport_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_IOPORT_AREA");
+
+ mmio_addr = allocator_alloc(lomap_allocator, ARM_MMIO_SIZE);
+ if (mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_MMIO_AREA");
+ mmio_allocator = allocator_create(mmio_addr, ARM_MMIO_SIZE);
+
+ pci_cfg_addr = allocator_alloc(lomap_allocator, PCI_CFG_SIZE);
+ if (pci_cfg_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_PCI_CFG_AREA");
+
+ pci_mmio_addr = allocator_alloc(lomap_allocator, ARM_PCI_MMIO_SIZE);
+ if (pci_mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_PCI_MMIO_AREA");
+ pci_mmio_allocator = allocator_create(pci_mmio_addr, ARM_PCI_MMIO_SIZE);
+}
+
static void init_ram(struct kvm *kvm)
{
int err;
@@ -92,6 +141,9 @@ static void init_ram(struct kvm *kvm)
void kvm__arch_delete_ram(struct kvm *kvm)
{
munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
+ allocator_destroy(mmio_allocator);
+ allocator_destroy(pci_mmio_allocator);
+ allocator_destroy(lomap_allocator);
}
void kvm__arch_read_term(struct kvm *kvm)
@@ -109,8 +161,7 @@ void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
if (cfg->ram_base == INVALID_MEM_ADDR)
cfg->ram_base = ARM_MEMORY_AREA;
- if (cfg->ram_base < ARM_MEMORY_AREA ||
- cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
+ if (cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
cfg->ram_base = ARM_MEMORY_AREA;
pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
}
@@ -123,11 +174,12 @@ void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
void kvm__arch_init(struct kvm *kvm)
{
+ init_ram(kvm);
+ init_mmio_mem(kvm);
+
/* Create the virtual GIC. */
if (gic__create(kvm, kvm->cfg.arch.irqchip))
die("Failed to create virtual GIC");
-
- init_ram(kvm);
}
#define FDT_ALIGN SZ_2M
@@ -279,3 +331,37 @@ int kvm__arch_setup_firmware(struct kvm *kvm)
{
return 0;
}
+
+u32 kvm__arch_alloc_mmio_block(u32 size)
+{
+ unsigned long addr;
+
+ assert(mmio_allocator);
+
+ addr = allocator_alloc(mmio_allocator, size);
+ if (addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate a MMIO block");
+
+ /*
+ * The allocator memory lives entirely below 4G on both arm and arm64,
+ * so the cast is safe to do.
+ */
+ return (u32)addr;
+}
+
+u32 kvm__arch_alloc_pci_mmio_block(u32 size)
+{
+ unsigned long addr;
+
+ assert(pci_mmio_allocator);
+
+ addr = allocator_alloc_align(pci_mmio_allocator, size, size);
+ if (addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate a PCI MMIO block");
+
+ /*
+ * The allocator memory lives entirely below 4G on both arm and arm64,
+ * so the cast is safe to do.
+ */
+ return (u32)addr;
+}
diff --git a/pci.c b/pci.c
index 88ee78ad7d08..87c7079e0fe9 100644
--- a/pci.c
+++ b/pci.c
@@ -6,10 +6,15 @@
#include "kvm/kvm.h"
#include <linux/err.h>
-#include <assert.h>
static u32 pci_config_address_bits;
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+u32 pci_get_io_space_block(u32 size)
+{
+ return kvm__arch_alloc_pci_mmio_block(size);
+}
+#else
/* This is within our PCI gap - in an unused area.
* Note this is a PCI *bus address*, is used to assign BARs etc.!
* (That's why it can still 32bit even with 64bit guests-- 64bit
@@ -26,6 +31,7 @@ u32 pci_get_io_space_block(u32 size)
io_space_blocks = block + size;
return block;
}
+#endif
void *pci_find_cap(struct pci_device_header *hdr, u8 cap_type)
{
diff --git a/virtio/mmio.c b/virtio/mmio.c
index 03cecc366292..9599774b825b 100644
--- a/virtio/mmio.c
+++ b/virtio/mmio.c
@@ -11,6 +11,12 @@
#include <linux/virtio_mmio.h>
#include <string.h>
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+static u32 virtio_mmio_get_io_space_block(u32 size)
+{
+ return kvm__arch_alloc_mmio_block(size);
+}
+#else
static u32 virtio_mmio_io_space_blocks = KVM_VIRTIO_MMIO_AREA;
static u32 virtio_mmio_get_io_space_block(u32 size)
@@ -20,6 +26,7 @@ static u32 virtio_mmio_get_io_space_block(u32 size)
return block;
}
+#endif
static void virtio_mmio_ioevent_callback(struct kvm *kvm, void *param)
{
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH kvmtool 14/16] arm: Move memory related code to memory.c
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (12 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 13/16] arm: Allow any base address for RAM Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 15/16] kvmtool: Make the size@addr option parser globally visible Alexandru Elisei
` (2 subsequent siblings)
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
The amount of memory related code in kvm.c has grown to a respectable size,
and it will only grow larger with the introduction of user specified MMIO
memory regions. Let's keep things tidy and move the code to its own
separate file in memory.c.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
Makefile | 2 +-
arm/include/arm-common/memory.h | 10 +++
arm/kvm.c | 170 +------------------------------------
arm/memory.c | 182 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 196 insertions(+), 168 deletions(-)
create mode 100644 arm/include/arm-common/memory.h
create mode 100644 arm/memory.c
diff --git a/Makefile b/Makefile
index 0ae52c6ad88b..146c3e4ce0e9 100644
--- a/Makefile
+++ b/Makefile
@@ -158,7 +158,7 @@ endif
# ARM
OBJS_ARM_COMMON := arm/fdt.o arm/gic.o arm/gicv2m.o arm/ioport.o \
arm/kvm.o arm/kvm-cpu.o arm/pci.o arm/timer.o \
- arm/pmu.o arm/allocator.o
+ arm/pmu.o arm/allocator.o arm/memory.o
HDRS_ARM_COMMON := arm/include
ifeq ($(ARCH), arm)
DEFINES += -DCONFIG_ARM
diff --git a/arm/include/arm-common/memory.h b/arm/include/arm-common/memory.h
new file mode 100644
index 000000000000..a22e503f3349
--- /dev/null
+++ b/arm/include/arm-common/memory.h
@@ -0,0 +1,10 @@
+#ifndef ARM_COMMON_MEMORY_H
+#define ARM_COMMON_MEMORY_H
+
+#include "kvm/kvm.h"
+#include "kvm/kvm-config.h"
+
+void memory_init(struct kvm *kvm);
+void memory_sanitize_cfg(struct kvm_config *cfg);
+
+#endif
diff --git a/arm/kvm.c b/arm/kvm.c
index a12a75d94cdd..45aae76eddfb 100644
--- a/arm/kvm.c
+++ b/arm/kvm.c
@@ -7,7 +7,7 @@
#include "kvm/pci.h"
#include "arm-common/gic.h"
-#include "arm-common/allocator.h"
+#include "arm-common/memory.h"
#include <linux/kernel.h>
#include <linux/kvm.h>
@@ -22,130 +22,12 @@ struct kvm_ext kvm_req_ext[] = {
{ 0, 0 },
};
-u64 ioport_addr = ALLOC_INVALID_ADDR;
-u64 mmio_addr = ALLOC_INVALID_ADDR;
-u64 pci_cfg_addr = ALLOC_INVALID_ADDR;
-u64 pci_mmio_addr = ALLOC_INVALID_ADDR;
-
-static struct allocator *lomap_allocator;
-static struct allocator *mmio_allocator;
-static struct allocator *pci_mmio_allocator;
-
bool kvm__arch_cpu_supports_vm(void)
{
/* The KVM capability check is enough. */
return true;
}
-static void init_mmio_mem(struct kvm *kvm)
-{
- u64 reserve;
- int ret;
-
- /* Allocate MMIO memory below 4G so 32 bit arm can use it. */
- lomap_allocator = allocator_create(0, ARM_LOMAP_MAX_ADDR);
-
- if (kvm->arch.memory_guest_start + kvm->ram_size > ARM_LOMAP_MAX_ADDR)
- reserve = ARM_LOMAP_MAX_ADDR - kvm->arch.memory_guest_start;
- else
- reserve = kvm->ram_size;
- ret = allocator_reserve(lomap_allocator, kvm->arch.memory_guest_start,
- reserve);
- if (ret)
- die("Could not reserve RAM address space");
-
- ioport_addr = allocator_alloc(lomap_allocator, ARM_IOPORT_SIZE);
- if (ioport_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_IOPORT_AREA");
-
- mmio_addr = allocator_alloc(lomap_allocator, ARM_MMIO_SIZE);
- if (mmio_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_MMIO_AREA");
- mmio_allocator = allocator_create(mmio_addr, ARM_MMIO_SIZE);
-
- pci_cfg_addr = allocator_alloc(lomap_allocator, PCI_CFG_SIZE);
- if (pci_cfg_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_PCI_CFG_AREA");
-
- pci_mmio_addr = allocator_alloc(lomap_allocator, ARM_PCI_MMIO_SIZE);
- if (pci_mmio_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_PCI_MMIO_AREA");
- pci_mmio_allocator = allocator_create(pci_mmio_addr, ARM_PCI_MMIO_SIZE);
-}
-
-static void init_ram(struct kvm *kvm)
-{
- int err;
- u64 phys_start, phys_size;
- void *host_mem;
- unsigned long alignment;
- /* Convenience aliases */
- const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
-
- /*
- * Allocate guest memory. If the user wants to use hugetlbfs, then the
- * specified guest memory size must be a multiple of the host huge page
- * size in order for the allocation to succeed. The mmap return adress
- * is naturally aligned to the huge page size, so in this case we don't
- * need to perform any alignment.
- *
- * Otherwise, we must align our buffer to 64K to correlate with the
- * maximum guest page size for virtio-mmio. If using THP, then our
- * minimal alignment becomes 2M with a 4K page size. With a 16K page
- * size, the alignment becomes 32M. 32M and 2M trump 64K, so let's go
- * with the largest alignment supported by the host.
- */
- if (hugetlbfs_path) {
- /* Don't do any alignment. */
- alignment = 0;
- } else {
- if (sysconf(_SC_PAGESIZE) == SZ_16K)
- alignment = SZ_32M;
- else
- alignment = SZ_2M;
- }
-
- kvm->ram_size = kvm->cfg.ram_size;
- kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
- kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
- kvm->arch.ram_alloc_size);
-
- if (kvm->arch.ram_alloc_start == MAP_FAILED)
- die("Failed to map %lld bytes for guest memory (%d)",
- kvm->arch.ram_alloc_size, errno);
-
- kvm->ram_start = kvm->arch.ram_alloc_start;
- /* The result of aligning to 0 is 0. Let's avoid that. */
- if (alignment)
- kvm->ram_start = (void *)ALIGN((unsigned long)kvm->ram_start,
- alignment);
-
- madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
- MADV_MERGEABLE);
-
- madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
- MADV_HUGEPAGE);
-
- phys_start = kvm->cfg.ram_base;
- phys_size = kvm->ram_size;
- host_mem = kvm->ram_start;
-
- err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
- if (err)
- die("Failed to register %lld bytes of memory at physical "
- "address 0x%llx [err %d]", phys_size, phys_start, err);
-
- kvm->arch.memory_guest_start = phys_start;
-}
-
-void kvm__arch_delete_ram(struct kvm *kvm)
-{
- munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
- allocator_destroy(mmio_allocator);
- allocator_destroy(pci_mmio_allocator);
- allocator_destroy(lomap_allocator);
-}
-
void kvm__arch_read_term(struct kvm *kvm)
{
serial8250__update_consoles(kvm);
@@ -158,24 +40,12 @@ void kvm__arch_set_cmdline(char *cmdline, bool video)
void kvm__arch_sanitize_cfg(struct kvm_config *cfg)
{
- if (cfg->ram_base == INVALID_MEM_ADDR)
- cfg->ram_base = ARM_MEMORY_AREA;
-
- if (cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
- cfg->ram_base = ARM_MEMORY_AREA;
- pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
- }
-
- if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
- cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
- pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
- }
+ memory_sanitize_cfg(cfg);
}
void kvm__arch_init(struct kvm *kvm)
{
- init_ram(kvm);
- init_mmio_mem(kvm);
+ memory_init(kvm);
/* Create the virtual GIC. */
if (gic__create(kvm, kvm->cfg.arch.irqchip))
@@ -331,37 +201,3 @@ int kvm__arch_setup_firmware(struct kvm *kvm)
{
return 0;
}
-
-u32 kvm__arch_alloc_mmio_block(u32 size)
-{
- unsigned long addr;
-
- assert(mmio_allocator);
-
- addr = allocator_alloc(mmio_allocator, size);
- if (addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate a MMIO block");
-
- /*
- * The allocator memory lives entirely below 4G on both arm and arm64,
- * so the cast is safe to do.
- */
- return (u32)addr;
-}
-
-u32 kvm__arch_alloc_pci_mmio_block(u32 size)
-{
- unsigned long addr;
-
- assert(pci_mmio_allocator);
-
- addr = allocator_alloc_align(pci_mmio_allocator, size, size);
- if (addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate a PCI MMIO block");
-
- /*
- * The allocator memory lives entirely below 4G on both arm and arm64,
- * so the cast is safe to do.
- */
- return (u32)addr;
-}
diff --git a/arm/memory.c b/arm/memory.c
new file mode 100644
index 000000000000..39ed9e224c72
--- /dev/null
+++ b/arm/memory.c
@@ -0,0 +1,182 @@
+#include "kvm/kvm.h"
+
+#include "arm-common/allocator.h"
+#include "arm-common/memory.h"
+
+#include <linux/types.h>
+
+#include <assert.h>
+
+u64 ioport_addr = ALLOC_INVALID_ADDR;
+u64 mmio_addr = ALLOC_INVALID_ADDR;
+u64 pci_cfg_addr = ALLOC_INVALID_ADDR;
+u64 pci_mmio_addr = ALLOC_INVALID_ADDR;
+
+static struct allocator *lomap_allocator;
+static struct allocator *mmio_allocator;
+static struct allocator *pci_mmio_allocator;
+
+void memory_sanitize_cfg(struct kvm_config *cfg)
+{
+ if (cfg->ram_base == INVALID_MEM_ADDR)
+ cfg->ram_base = ARM_MEMORY_AREA;
+
+ if (cfg->ram_base >= ARM_MAX_ADDR(cfg)) {
+ cfg->ram_base = ARM_MEMORY_AREA;
+ pr_warning("Changing RAM base to 0x%llx", cfg->ram_base);
+ }
+
+ if (cfg->ram_base + cfg->ram_size > ARM_MAX_ADDR(cfg)) {
+ cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
+ pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
+ }
+}
+
+static void init_mmio(struct kvm *kvm)
+{
+ u64 reserve;
+ int ret;
+
+ /* Allocate MMIO memory below 4G so 32 bit arm can use it. */
+ lomap_allocator = allocator_create(0, ARM_LOMAP_MAX_ADDR);
+
+ if (kvm->arch.memory_guest_start + kvm->ram_size > ARM_LOMAP_MAX_ADDR)
+ reserve = ARM_LOMAP_MAX_ADDR - kvm->arch.memory_guest_start;
+ else
+ reserve = kvm->ram_size;
+ ret = allocator_reserve(lomap_allocator, kvm->arch.memory_guest_start,
+ reserve);
+ if (ret)
+ die("Could not reserve RAM address space");
+
+ ioport_addr = allocator_alloc(lomap_allocator, ARM_IOPORT_SIZE);
+ if (ioport_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_IOPORT_AREA");
+
+ mmio_addr = allocator_alloc(lomap_allocator, ARM_MMIO_SIZE);
+ if (mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_MMIO_AREA");
+ mmio_allocator = allocator_create(mmio_addr, ARM_MMIO_SIZE);
+
+ pci_cfg_addr = allocator_alloc(lomap_allocator, PCI_CFG_SIZE);
+ if (pci_cfg_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_PCI_CFG_AREA");
+
+ pci_mmio_addr = allocator_alloc(lomap_allocator, ARM_PCI_MMIO_SIZE);
+ if (pci_mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_PCI_MMIO_AREA");
+ pci_mmio_allocator = allocator_create(pci_mmio_addr, ARM_PCI_MMIO_SIZE);
+}
+
+static void init_ram(struct kvm *kvm)
+{
+ int err;
+ u64 phys_start, phys_size;
+ void *host_mem;
+ unsigned long alignment;
+ /* Convenience aliases */
+ const char *hugetlbfs_path = kvm->cfg.hugetlbfs_path;
+
+ /*
+ * Allocate guest memory. If the user wants to use hugetlbfs, then the
+ * specified guest memory size must be a multiple of the host huge page
+ * size in order for the allocation to succeed. The mmap return adress
+ * is naturally aligned to the huge page size, so in this case we don't
+ * need to perform any alignment.
+ *
+ * Otherwise, we must align our buffer to 64K to correlate with the
+ * maximum guest page size for virtio-mmio. If using THP, then our
+ * minimal alignment becomes 2M with a 4K page size. With a 16K page
+ * size, the alignment becomes 32M. 32M and 2M trump 64K, so let's go
+ * with the largest alignment supported by the host.
+ */
+ if (hugetlbfs_path) {
+ /* Don't do any alignment. */
+ alignment = 0;
+ } else {
+ if (sysconf(_SC_PAGESIZE) == SZ_16K)
+ alignment = SZ_32M;
+ else
+ alignment = SZ_2M;
+ }
+
+ kvm->ram_size = kvm->cfg.ram_size;
+ kvm->arch.ram_alloc_size = kvm->ram_size + alignment;
+ kvm->arch.ram_alloc_start = mmap_anon_or_hugetlbfs(kvm, hugetlbfs_path,
+ kvm->arch.ram_alloc_size);
+
+ if (kvm->arch.ram_alloc_start == MAP_FAILED)
+ die("Failed to map %lld bytes for guest memory (%d)",
+ kvm->arch.ram_alloc_size, errno);
+
+ kvm->ram_start = kvm->arch.ram_alloc_start;
+ /* The result of aligning to 0 is 0. Let's avoid that. */
+ if (alignment)
+ kvm->ram_start = (void *)ALIGN((unsigned long)kvm->ram_start,
+ alignment);
+
+ madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
+ MADV_MERGEABLE);
+
+ madvise(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size,
+ MADV_HUGEPAGE);
+
+ phys_start = kvm->cfg.ram_base;
+ phys_size = kvm->ram_size;
+ host_mem = kvm->ram_start;
+
+ err = kvm__register_ram(kvm, phys_start, phys_size, host_mem);
+ if (err)
+ die("Failed to register %lld bytes of memory at physical "
+ "address 0x%llx [err %d]", phys_size, phys_start, err);
+
+ kvm->arch.memory_guest_start = phys_start;
+}
+
+void memory_init(struct kvm *kvm)
+{
+ init_ram(kvm);
+ init_mmio(kvm);
+}
+
+void kvm__arch_delete_ram(struct kvm *kvm)
+{
+ munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
+ allocator_destroy(mmio_allocator);
+ allocator_destroy(pci_mmio_allocator);
+ allocator_destroy(lomap_allocator);
+}
+
+u32 kvm__arch_alloc_mmio_block(u32 size)
+{
+ unsigned long addr;
+
+ assert(mmio_allocator);
+
+ addr = allocator_alloc(mmio_allocator, size);
+ if (addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate a MMIO block");
+
+ /*
+ * The allocator memory lives entirely below 4G on both arm and arm64,
+ * so the cast is safe to do.
+ */
+ return (u32)addr;
+}
+
+u32 kvm__arch_alloc_pci_mmio_block(u32 size)
+{
+ unsigned long addr;
+
+ assert(pci_mmio_allocator);
+
+ addr = allocator_alloc_align(pci_mmio_allocator, size, size);
+ if (addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate a PCI MMIO block");
+
+ /*
+ * The allocator memory lives entirely below 4G on both arm and arm64,
+ * so the cast is safe to do.
+ */
+ return (u32)addr;
+}
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH kvmtool 15/16] kvmtool: Make the size@addr option parser globally visible
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (13 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 14/16] arm: Move memory related code to memory.c Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2019-09-23 13:35 ` [PATCH kvmtool 16/16] arm: Allow the user to define the MMIO regions Alexandru Elisei
2020-02-05 17:16 ` [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Will Deacon
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
The arm architecture is going to allow the user to specify the size and
address of various memory regions. Let's make the function to parse
size@addr options globally visible, because we will be using it.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
builtin-run.c | 29 ++++++++++++++++++-----------
include/kvm/kvm.h | 2 ++
2 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/builtin-run.c b/builtin-run.c
index 757ede4ac0d1..70ece3754644 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -131,37 +131,44 @@ static u64 parse_addr(char **next)
return ((u64)1) << shift;
}
-static int mem_parser(const struct option *opt, const char *arg, int unset)
+void kvm__parse_size_addr_option(const char *p, u64 *size, u64 *addr)
{
- struct kvm_config *cfg = opt->value;
- const char *p = arg;
char *next;
- u64 size, addr = INVALID_MEM_ADDR;
+
+ *addr = INVALID_MEM_ADDR;
/* Parse memory size. */
- size = strtoll(p, &next, 0);
+ *size = strtoll(p, &next, 0);
if (next == p)
die("Invalid memory size");
- size *= parse_size(&next);
+ *size *= parse_size(&next);
if (*next == '\0')
- goto out;
+ return;
else
p = next + 1;
- addr = strtoll(p, &next, 0);
+ *addr = strtoll(p, &next, 0);
if (next == p)
die("Invalid memory address");
+ *addr *= parse_addr(&next);
+}
+
+static int mem_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ u64 size, addr;
+
+ kvm__parse_size_addr_option(p, &size, &addr);
+
#ifndef ARCH_SUPPORT_CFG_RAM_BASE
if (addr != INVALID_MEM_ADDR)
die("Specifying the memory address not supported by the architecture");
#endif
- addr *= parse_addr(&next);
-
-out:
cfg->ram_base = addr;
cfg->ram_size = size;
diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 8787a92b4dbb..a93e30d1a320 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -192,4 +192,6 @@ static inline void kvm__set_thread_name(const char *name)
prctl(PR_SET_NAME, name);
}
+void kvm__parse_size_addr_option(const char *p, u64 *size, u64 *addr);
+
#endif /* KVM__KVM_H */
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH kvmtool 16/16] arm: Allow the user to define the MMIO regions
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (14 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 15/16] kvmtool: Make the size@addr option parser globally visible Alexandru Elisei
@ 2019-09-23 13:35 ` Alexandru Elisei
2020-02-05 17:16 ` [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Will Deacon
16 siblings, 0 replies; 40+ messages in thread
From: Alexandru Elisei @ 2019-09-23 13:35 UTC (permalink / raw)
To: kvm, will, julien.thierry.kdev
Cc: maz, suzuki.poulose, julien.grall, andre.przywara
Allow the user to specify the I/O port (--ioport), MMIO (--mmio) and PCI
MMIO (--pci-mmio) memory regions using the size@addr format.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
arm/include/arm-common/kvm-config-arch.h | 25 +++++
arm/include/arm-common/memory.h | 3 +
arm/memory.c | 168 ++++++++++++++++++++++++++++---
arm/pci.c | 5 +-
kvm.c | 5 +
5 files changed, 192 insertions(+), 14 deletions(-)
diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h
index 5734c46ab9e6..3d2be1ae4c60 100644
--- a/arm/include/arm-common/kvm-config-arch.h
+++ b/arm/include/arm-common/kvm-config-arch.h
@@ -6,6 +6,12 @@
struct kvm_config_arch {
const char *dump_dtb_filename;
unsigned int force_cntfrq;
+ u64 ioport_addr;
+ u64 ioport_size;
+ u64 mmio_addr;
+ u64 mmio_size;
+ u64 pci_mmio_addr;
+ u64 pci_mmio_size;
bool virtio_trans_pci;
bool aarch32_guest;
bool has_pmuv3;
@@ -15,10 +21,29 @@ struct kvm_config_arch {
};
int irqchip_parser(const struct option *opt, const char *arg, int unset);
+int ioport_parser(const struct option *opt, const char *arg, int unset);
+int mmio_parser(const struct option *opt, const char *arg, int unset);
+int pci_mmio_parser(const struct option *opt, const char *arg, int unset);
#define OPT_ARCH_RUN(pfx, cfg) \
pfx, \
ARM_OPT_ARCH_RUN(cfg) \
+ OPT_CALLBACK('\0', "ioport", (cfg), \
+ "size[@addr]", \
+ "Virtual machine I/O port region size, " \
+ "optionally starting at <addr> This is where " \
+ "the 8250 UART is emulated.", \
+ ioport_parser, NULL), \
+ OPT_CALLBACK('\0', "mmio", (cfg), \
+ "size[@addr]", \
+ "Virtual machine MMIO memory size, " \
+ " optionally starting at <addr>.", \
+ mmio_parser, NULL), \
+ OPT_CALLBACK('\0', "pci-mmio", (cfg), \
+ "size[@addr]", \
+ "Virtual machine PCI MMIO memory size, " \
+ " optionally starting at <addr>.", \
+ pci_mmio_parser, NULL), \
OPT_STRING('\0', "dump-dtb", &(cfg)->dump_dtb_filename, \
".dtb file", "Dump generated .dtb to specified file"), \
OPT_UINTEGER('\0', "override-bad-firmware-cntfrq", &(cfg)->force_cntfrq,\
diff --git a/arm/include/arm-common/memory.h b/arm/include/arm-common/memory.h
index a22e503f3349..074b2a5ff82c 100644
--- a/arm/include/arm-common/memory.h
+++ b/arm/include/arm-common/memory.h
@@ -7,4 +7,7 @@
void memory_init(struct kvm *kvm);
void memory_sanitize_cfg(struct kvm_config *cfg);
+u64 memory_get_ioport_size(void);
+u64 memory_get_pci_mmio_size(void);
+
#endif
diff --git a/arm/memory.c b/arm/memory.c
index 39ed9e224c72..818a9add80e3 100644
--- a/arm/memory.c
+++ b/arm/memory.c
@@ -12,12 +12,58 @@ u64 mmio_addr = ALLOC_INVALID_ADDR;
u64 pci_cfg_addr = ALLOC_INVALID_ADDR;
u64 pci_mmio_addr = ALLOC_INVALID_ADDR;
+static u64 ioport_size;
+
static struct allocator *lomap_allocator;
static struct allocator *mmio_allocator;
static struct allocator *pci_mmio_allocator;
+int ioport_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ u64 size, addr;
+
+ kvm__parse_size_addr_option(p, &size, &addr);
+
+ cfg->arch.ioport_addr = addr;
+ cfg->arch.ioport_size = size;
+
+ return 0;
+}
+
+int mmio_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ u64 size, addr;
+
+ kvm__parse_size_addr_option(p, &size, &addr);
+
+ cfg->arch.mmio_addr = addr;
+ cfg->arch.mmio_size = size;
+
+ return 0;
+}
+
+int pci_mmio_parser(const struct option *opt, const char *arg, int unset)
+{
+ struct kvm_config *cfg = opt->value;
+ const char *p = arg;
+ u64 size, addr;
+
+ kvm__parse_size_addr_option(p, &size, &addr);
+
+ cfg->arch.pci_mmio_addr = addr;
+ cfg->arch.pci_mmio_size = size;
+
+ return 0;
+}
+
void memory_sanitize_cfg(struct kvm_config *cfg)
{
+ struct kvm_config_arch *arch = &cfg->arch;
+
if (cfg->ram_base == INVALID_MEM_ADDR)
cfg->ram_base = ARM_MEMORY_AREA;
@@ -30,6 +76,102 @@ void memory_sanitize_cfg(struct kvm_config *cfg)
cfg->ram_size = ARM_MAX_ADDR(cfg) - cfg->ram_base;
pr_warning("Capping memory to %lluMB", cfg->ram_size >> 20);
}
+
+ if (!arch->ioport_size)
+ arch->ioport_size = ARM_IOPORT_SIZE;
+
+ if (arch->ioport_size < ALLOC_PAGE_SIZE) {
+ /*
+ * Allocate at least one page of maximum size for ioports. No
+ * need to check the sized for general MMIO and PCI MMIO, as
+ * allocations from those regions are larger than the page size
+ * and they will fail if the regions are too small.
+ */
+ arch->ioport_size = ALLOC_PAGE_SIZE;
+ pr_warning("Changing IOPORT memory size to 0x%x", ALLOC_PAGE_SIZE);
+ }
+
+ if (arch->ioport_size & (ALLOC_PAGE_SIZE - 1)) {
+ arch->ioport_size = ALIGN(arch->ioport_size, ALLOC_PAGE_SIZE);
+ pr_warning("Aligning IOPORT size to maximum page size (now is 0x%llx)",
+ arch->ioport_size);
+ }
+
+ if (!cfg->arch.mmio_size)
+ cfg->arch.mmio_size = ARM_MMIO_SIZE;
+
+ if (arch->mmio_size & (ALLOC_PAGE_SIZE - 1)) {
+ arch->mmio_size = ALIGN(arch->mmio_size, ALLOC_PAGE_SIZE);
+ pr_warning("Aligning MMIO size to maximum page size (now is 0x%llx)",
+ arch->mmio_size);
+ }
+
+ if (!cfg->arch.pci_mmio_size)
+ cfg->arch.pci_mmio_size = ARM_PCI_MMIO_SIZE;
+
+ if (arch->pci_mmio_size & (ALLOC_PAGE_SIZE - 1)) {
+ arch->pci_mmio_size = ALIGN(arch->pci_mmio_size, ALLOC_PAGE_SIZE);
+ pr_warning("Aligning PCI MMIO size to maximum page size (now is 0x%llx)",
+ arch->pci_mmio_size);
+ }
+}
+
+static void init_ioport_region(struct kvm *kvm)
+{
+ struct kvm_config_arch *arch = &kvm->cfg.arch;
+ int ret;
+
+ if (arch->ioport_addr == INVALID_MEM_ADDR) {
+ ioport_addr = allocator_alloc(lomap_allocator, arch->ioport_size);
+ if (ioport_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_IOPORT_AREA");
+ } else {
+ ret = allocator_reserve(lomap_allocator, arch->ioport_addr,
+ arch->ioport_size);
+ if (ret)
+ die("Not enough memory to reserve KVM_IOPORT_AREA");
+ ioport_addr = arch->ioport_addr;
+ }
+ ioport_size = arch->ioport_size;
+}
+
+static void init_mmio_region(struct kvm *kvm)
+{
+ struct kvm_config_arch *arch = &kvm->cfg.arch;
+ int ret;
+
+ if (arch->mmio_addr == INVALID_MEM_ADDR) {
+ mmio_addr = allocator_alloc(lomap_allocator, arch->mmio_size);
+ if (mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_MMIO_AREA");
+ } else {
+ ret = allocator_reserve(lomap_allocator, arch->mmio_addr,
+ arch->mmio_size);
+ if (ret)
+ die("Not enough memory to reserve KVM_MMIO_AREA");
+ mmio_addr = arch->mmio_addr;
+ }
+ mmio_allocator = allocator_create(mmio_addr, arch->mmio_size);
+}
+
+static void init_pci_mmio_region(struct kvm *kvm)
+{
+ struct kvm_config_arch *arch = &kvm->cfg.arch;
+ int ret;
+
+ if (arch->pci_mmio_addr == INVALID_MEM_ADDR) {
+ pci_mmio_addr = allocator_alloc(lomap_allocator, arch->pci_mmio_size);
+ if (pci_mmio_addr == ALLOC_INVALID_ADDR)
+ die("Not enough memory to allocate KVM_PCI_MMIO_AREA");
+ } else {
+ ret = allocator_reserve(lomap_allocator,
+ arch->pci_mmio_addr,
+ arch->pci_mmio_size);
+ if (ret)
+ die("Not enough memory to reserve KVM_PCI_MMIO_AREA");
+ pci_mmio_addr = arch->pci_mmio_addr;
+ }
+ pci_mmio_allocator = allocator_create(pci_mmio_addr, arch->pci_mmio_size);
}
static void init_mmio(struct kvm *kvm)
@@ -49,23 +191,15 @@ static void init_mmio(struct kvm *kvm)
if (ret)
die("Could not reserve RAM address space");
- ioport_addr = allocator_alloc(lomap_allocator, ARM_IOPORT_SIZE);
- if (ioport_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_IOPORT_AREA");
-
- mmio_addr = allocator_alloc(lomap_allocator, ARM_MMIO_SIZE);
- if (mmio_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_MMIO_AREA");
- mmio_allocator = allocator_create(mmio_addr, ARM_MMIO_SIZE);
+ init_ioport_region(kvm);
+ init_mmio_region(kvm);
+ /* The user cannot define the PCI CFG area. */
pci_cfg_addr = allocator_alloc(lomap_allocator, PCI_CFG_SIZE);
if (pci_cfg_addr == ALLOC_INVALID_ADDR)
die("Not enough memory to allocate KVM_PCI_CFG_AREA");
- pci_mmio_addr = allocator_alloc(lomap_allocator, ARM_PCI_MMIO_SIZE);
- if (pci_mmio_addr == ALLOC_INVALID_ADDR)
- die("Not enough memory to allocate KVM_PCI_MMIO_AREA");
- pci_mmio_allocator = allocator_create(pci_mmio_addr, ARM_PCI_MMIO_SIZE);
+ init_pci_mmio_region(kvm);
}
static void init_ram(struct kvm *kvm)
@@ -180,3 +314,13 @@ u32 kvm__arch_alloc_pci_mmio_block(u32 size)
*/
return (u32)addr;
}
+
+u64 memory_get_ioport_size(void)
+{
+ return ioport_size;
+}
+
+u64 memory_get_pci_mmio_size(void)
+{
+ return pci_mmio_allocator->size;
+}
diff --git a/arm/pci.c b/arm/pci.c
index 1a2fc2688c9e..01242017c143 100644
--- a/arm/pci.c
+++ b/arm/pci.c
@@ -6,6 +6,7 @@
#include "kvm/util.h"
#include "arm-common/pci.h"
+#include "arm-common/memory.h"
/*
* An entry in the interrupt-map table looks like:
@@ -43,7 +44,7 @@ void pci__generate_fdt_nodes(void *fdt)
.lo = 0,
},
.cpu_addr = cpu_to_fdt64(KVM_IOPORT_AREA),
- .length = cpu_to_fdt64(ARM_IOPORT_SIZE),
+ .length = cpu_to_fdt64(memory_get_ioport_size()),
},
{
.pci_addr = {
@@ -52,7 +53,7 @@ void pci__generate_fdt_nodes(void *fdt)
.lo = cpu_to_fdt32(KVM_PCI_MMIO_AREA),
},
.cpu_addr = cpu_to_fdt64(KVM_PCI_MMIO_AREA),
- .length = cpu_to_fdt64(ARM_PCI_MMIO_SIZE),
+ .length = cpu_to_fdt64(memory_get_pci_mmio_size()),
},
};
diff --git a/kvm.c b/kvm.c
index 4da413e0681d..b6b0abf7cd59 100644
--- a/kvm.c
+++ b/kvm.c
@@ -165,6 +165,11 @@ struct kvm *kvm__new(void)
* with an user specifying address 0.
*/
kvm->cfg.ram_base = INVALID_MEM_ADDR;
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
+ kvm->cfg.arch.ioport_addr = INVALID_MEM_ADDR;
+ kvm->cfg.arch.mmio_addr = INVALID_MEM_ADDR;
+ kvm->cfg.arch.pci_mmio_addr = INVALID_MEM_ADDR;
+#endif
#ifdef KVM_BRLOCK_DEBUG
kvm->brlock_sem = (pthread_rwlock_t) PTHREAD_RWLOCK_INITIALIZER;
--
2.7.4
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout
2019-09-23 13:35 [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Alexandru Elisei
` (15 preceding siblings ...)
2019-09-23 13:35 ` [PATCH kvmtool 16/16] arm: Allow the user to define the MMIO regions Alexandru Elisei
@ 2020-02-05 17:16 ` Will Deacon
2020-02-05 17:18 ` Alexandru Elisei
16 siblings, 1 reply; 40+ messages in thread
From: Will Deacon @ 2020-02-05 17:16 UTC (permalink / raw)
To: Alexandru Elisei
Cc: kvm, julien.thierry.kdev, maz, suzuki.poulose, julien.grall,
andre.przywara
On Mon, Sep 23, 2019 at 02:35:06PM +0100, Alexandru Elisei wrote:
> The guest memory layout created by kvmtool is fixed: regular MMIO is below
> 1G, PCI MMIO is below 2G, and the RAM always starts at the 2G mark. Real
> hardware can have a different memory layout, and being able to create a
> specific memory layout can be very useful for testing the guest kernel.
>
> This series allows the user the specify the memory layout for the
> virtual machine by expanding the -m/--mem option to take an <addr>
> parameter, and by adding architecture specific options to define the I/O
> ports, regular MMIO and PCI MMIO memory regions.
>
> The user defined memory regions are implemented in patch #16; I consider
> the patch to be an RFC because I'm not really sure that my approach is the
> correct one; for example, I decided to make the options arch dependent
> because that seemed like the path of least resistance, but they could have
> just as easily implemented as arch independent and each architecture
> advertised having support for them via a define (like with RAM base
> address).
Do you plan to repost this with Andre's comments addressed?
Will
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout
2020-02-05 17:16 ` [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout Will Deacon
@ 2020-02-05 17:18 ` Alexandru Elisei
2020-02-06 9:20 ` Marc Zyngier
0 siblings, 1 reply; 40+ messages in thread
From: Alexandru Elisei @ 2020-02-05 17:18 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, julien.thierry.kdev, maz, suzuki.poulose, julien.grall,
andre.przywara
Hi,
On 2/5/20 5:16 PM, Will Deacon wrote:
> On Mon, Sep 23, 2019 at 02:35:06PM +0100, Alexandru Elisei wrote:
>> The guest memory layout created by kvmtool is fixed: regular MMIO is below
>> 1G, PCI MMIO is below 2G, and the RAM always starts at the 2G mark. Real
>> hardware can have a different memory layout, and being able to create a
>> specific memory layout can be very useful for testing the guest kernel.
>>
>> This series allows the user the specify the memory layout for the
>> virtual machine by expanding the -m/--mem option to take an <addr>
>> parameter, and by adding architecture specific options to define the I/O
>> ports, regular MMIO and PCI MMIO memory regions.
>>
>> The user defined memory regions are implemented in patch #16; I consider
>> the patch to be an RFC because I'm not really sure that my approach is the
>> correct one; for example, I decided to make the options arch dependent
>> because that seemed like the path of least resistance, but they could have
>> just as easily implemented as arch independent and each architecture
>> advertised having support for them via a define (like with RAM base
>> address).
> Do you plan to repost this with Andre's comments addressed?
The series will conflict with my other series which add support for assignable
BARs and PCIE. I am definitely still interested in reposting this because I think
it's very useful, and I'll do it after the other patches get merged.
Thank you for taking a look!
Thanks,
Alex
>
> Will
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH kvmtool 00/16] arm: Allow the user to define the memory layout
2020-02-05 17:18 ` Alexandru Elisei
@ 2020-02-06 9:20 ` Marc Zyngier
0 siblings, 0 replies; 40+ messages in thread
From: Marc Zyngier @ 2020-02-06 9:20 UTC (permalink / raw)
To: Alexandru Elisei
Cc: Will Deacon, kvm, julien.thierry.kdev, suzuki.poulose,
julien.grall, andre.przywara
On 2020-02-05 17:18, Alexandru Elisei wrote:
> Hi,
>
> On 2/5/20 5:16 PM, Will Deacon wrote:
>> On Mon, Sep 23, 2019 at 02:35:06PM +0100, Alexandru Elisei wrote:
>>> The guest memory layout created by kvmtool is fixed: regular MMIO is
>>> below
>>> 1G, PCI MMIO is below 2G, and the RAM always starts at the 2G mark.
>>> Real
>>> hardware can have a different memory layout, and being able to create
>>> a
>>> specific memory layout can be very useful for testing the guest
>>> kernel.
>>>
>>> This series allows the user the specify the memory layout for the
>>> virtual machine by expanding the -m/--mem option to take an <addr>
>>> parameter, and by adding architecture specific options to define the
>>> I/O
>>> ports, regular MMIO and PCI MMIO memory regions.
>>>
>>> The user defined memory regions are implemented in patch #16; I
>>> consider
>>> the patch to be an RFC because I'm not really sure that my approach
>>> is the
>>> correct one; for example, I decided to make the options arch
>>> dependent
>>> because that seemed like the path of least resistance, but they could
>>> have
>>> just as easily implemented as arch independent and each architecture
>>> advertised having support for them via a define (like with RAM base
>>> address).
>> Do you plan to repost this with Andre's comments addressed?
>
> The series will conflict with my other series which add support for
> assignable
> BARs and PCIE. I am definitely still interested in reposting this
> because I think
> it's very useful, and I'll do it after the other patches get merged.
I'd be happy to review the rebased version. I definitely need it to
support some of my most funky HW...
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 40+ messages in thread