Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On 07/08/2019 14:39, Steven Price wrote:
> On 03/08/2019 18:34, Marc Zyngier wrote:
>> On Sat, 3 Aug 2019 13:51:13 +0100
>> Marc Zyngier wrote:
>>
>> [forgot that one]
>>
>>> On Fri, 2 Aug 2019 15:50:14 +0100
>>> Steven Price wrote:
>>
>> [...]
>>
+static int __init kvm_pvtime_init(void)
+{
+ kvm_register_device_ops(_ops, KVM_DEV_TYPE_ARM_PV_TIME);
+
+ return 0;
+}
+
+late_initcall(kvm_pvtime_init);
>>
>> Why is it an initcall? So far, the only initcall we've used is the one
>> that initializes KVM itself. Can't we just the device_ops just like we
>> do for the vgic?
>
> So would you prefer a direct call from init_subsystems() in
> virt/kvm/arm/arm.c?
Yes. Consistency is important.
> The benefit of initcall is just that it keeps the code self-contained.
> In init_subsystems() I'd either need a #ifdef CONFIG_ARM64 or a dummy
> function for arm.
Having a dummy function for 32bit ARM is fine. Most of the code we add
to the 32bit port is made of empty stubs anyway.
Thanks,
M.
--
Jazz is not dead, it just smells funny...
Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On 03/08/2019 18:34, Marc Zyngier wrote:
> On Sat, 3 Aug 2019 13:51:13 +0100
> Marc Zyngier wrote:
>
> [forgot that one]
>
>> On Fri, 2 Aug 2019 15:50:14 +0100
>> Steven Price wrote:
>
> [...]
>
>>> +static int __init kvm_pvtime_init(void)
>>> +{
>>> + kvm_register_device_ops(_ops, KVM_DEV_TYPE_ARM_PV_TIME);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +late_initcall(kvm_pvtime_init);
>
> Why is it an initcall? So far, the only initcall we've used is the one
> that initializes KVM itself. Can't we just the device_ops just like we
> do for the vgic?
So would you prefer a direct call from init_subsystems() in
virt/kvm/arm/arm.c?
The benefit of initcall is just that it keeps the code self-contained.
In init_subsystems() I'd either need a #ifdef CONFIG_ARM64 or a dummy
function for arm.
Steve
Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On 05/08/2019 17:10, Steven Price wrote:
> On 03/08/2019 13:51, Marc Zyngier wrote:
>> On Fri, 2 Aug 2019 15:50:14 +0100
>> Steven Price wrote:
>>
>>> Allow user space to inform the KVM host where in the physical memory
>>> map the paravirtualized time structures should be located.
>>>
>>> A device is created which provides the base address of an array of
>>> Stolen Time (ST) structures, one for each VCPU. There must be (64 *
>>> total number of VCPUs) bytes of memory available at this location.
>>>
>>> The address is given in terms of the physical address visible to
>>> the guest and must be 64 byte aligned. The memory should be marked as
>>> reserved to the guest to stop it allocating it for other purposes.
>>
>> Why? You seem to be allocating the memory from the kernel, so as far as
>> the guest is concerned, this isn't generally usable memory.
>
> I obviously didn't word it very well - that's what I meant. The "memory"
> that represents the stolen time structure shouldn't be shown to the
> guest as normal memory, but "reserved" for the purpose of stolen time.
>
> To be honest it looks like I forgot to rewrite this commit message -
> which 64 byte alignment is all that the guest can rely on (because each
> vCPU has it's own structure), the actual array of structures needs to be
> page aligned to ensure we can safely map it into the guest.
>
>>>
>>> Signed-off-by: Steven Price
>>> ---
>>> arch/arm64/include/asm/kvm_mmu.h | 2 +
>>> arch/arm64/include/uapi/asm/kvm.h | 6 +
>>> arch/arm64/kvm/Makefile | 1 +
>>> include/uapi/linux/kvm.h | 2 +
>>> virt/kvm/arm/mmu.c| 44 +++
>>> virt/kvm/arm/pvtime.c | 190 ++
>>> 6 files changed, 245 insertions(+)
>>> create mode 100644 virt/kvm/arm/pvtime.c
[...]
>>> +static int kvm_arm_pvtime_set_attr(struct kvm_device *dev,
>>> + struct kvm_device_attr *attr)
>>> +{
>>> + struct kvm_arch_pvtime *pvtime = >kvm->arch.pvtime;
>>> + u64 __user *user = (u64 __user *)attr->addr;
>>> + u64 paddr;
>>> + int ret;
>>> +
>>> + switch (attr->group) {
>>> + case KVM_DEV_ARM_PV_TIME_PADDR:
>>> + if (get_user(paddr, user))
>>> + return -EFAULT;
>>> + if (paddr & 63)
>>> + return -EINVAL;
>>
>> You should check whether the device fits into the IPA space for this
>> guest, and whether it overlaps with anything else.
>
> pvtime_map_pages() should fail in the case of overlap. That seems
> sufficient to me - do you think we need something stronger?
Definitely. stage2_set_pte() won't fail for a non-IO overlapping
mapping, and will just treat it as guest memory. If this overlaps with a
memslot, we'll never be able to fault that page in, ending up with
interesting memory corruption... :-/
That's one of the reasons why I think option (2) in your earlier email
is an interesting one, as it sidesteps a whole lot of ugly and hard to
test corner cases.
Thanks,
M.
--
Jazz is not dead, it just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On 03/08/2019 13:51, Marc Zyngier wrote:
> On Fri, 2 Aug 2019 15:50:14 +0100
> Steven Price wrote:
>
>> Allow user space to inform the KVM host where in the physical memory
>> map the paravirtualized time structures should be located.
>>
>> A device is created which provides the base address of an array of
>> Stolen Time (ST) structures, one for each VCPU. There must be (64 *
>> total number of VCPUs) bytes of memory available at this location.
>>
>> The address is given in terms of the physical address visible to
>> the guest and must be 64 byte aligned. The memory should be marked as
>> reserved to the guest to stop it allocating it for other purposes.
>
> Why? You seem to be allocating the memory from the kernel, so as far as
> the guest is concerned, this isn't generally usable memory.
I obviously didn't word it very well - that's what I meant. The "memory"
that represents the stolen time structure shouldn't be shown to the
guest as normal memory, but "reserved" for the purpose of stolen time.
To be honest it looks like I forgot to rewrite this commit message -
which 64 byte alignment is all that the guest can rely on (because each
vCPU has it's own structure), the actual array of structures needs to be
page aligned to ensure we can safely map it into the guest.
>>
>> Signed-off-by: Steven Price
>> ---
>> arch/arm64/include/asm/kvm_mmu.h | 2 +
>> arch/arm64/include/uapi/asm/kvm.h | 6 +
>> arch/arm64/kvm/Makefile | 1 +
>> include/uapi/linux/kvm.h | 2 +
>> virt/kvm/arm/mmu.c| 44 +++
>> virt/kvm/arm/pvtime.c | 190 ++
>> 6 files changed, 245 insertions(+)
>> create mode 100644 virt/kvm/arm/pvtime.c
>>
>> diff --git a/arch/arm64/include/asm/kvm_mmu.h
>> b/arch/arm64/include/asm/kvm_mmu.h
>> index befe37d4bc0e..88c8a4b2836f 100644
>> --- a/arch/arm64/include/asm/kvm_mmu.h
>> +++ b/arch/arm64/include/asm/kvm_mmu.h
>> @@ -157,6 +157,8 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm);
>> void kvm_free_stage2_pgd(struct kvm *kvm);
>> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>>phys_addr_t pa, unsigned long size, bool writable);
>> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> + phys_addr_t pa, unsigned long size, bool writable);
>>
>> int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>>
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index 9a507716ae2f..95516a4198ea 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -367,6 +367,12 @@ struct kvm_vcpu_events {
>> #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS
>> #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED
>>
>> +/* Device Control API: PV_TIME */
>> +#define KVM_DEV_ARM_PV_TIME_PADDR 0
>> +#define KVM_DEV_ARM_PV_TIME_ST 0
>> +#define KVM_DEV_ARM_PV_TIME_STATE_SIZE 1
>> +#define KVM_DEV_ARM_PV_TIME_STATE 2
>> +
>> #endif
>>
>> #endif /* __ARM_KVM_H__ */
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index 73dce4d47d47..5ffbdc39e780 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -14,6 +14,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o
>> $(KVM)/coalesced_mmio.o $(KVM)/e
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o
>> $(KVM)/arm/mmio.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hypercalls.o
>> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/pvtime.o
>>
>> kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
>> kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index a7c19540ce21..04bffafa0708 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1222,6 +1222,8 @@ enum kvm_device_type {
>> #define KVM_DEV_TYPE_ARM_VGIC_ITS KVM_DEV_TYPE_ARM_VGIC_ITS
>> KVM_DEV_TYPE_XIVE,
>> #define KVM_DEV_TYPE_XIVE KVM_DEV_TYPE_XIVE
>> +KVM_DEV_TYPE_ARM_PV_TIME,
>> +#define KVM_DEV_TYPE_ARM_PV_TIMEKVM_DEV_TYPE_ARM_PV_TIME
>> KVM_DEV_TYPE_MAX,
>> };
>>
>> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
>> index 38b4c910b6c3..be28a4aee451 100644
>> --- a/virt/kvm/arm/mmu.c
>> +++ b/virt/kvm/arm/mmu.c
>> @@ -1368,6 +1368,50 @@ int kvm_phys_addr_ioremap(struct kvm *kvm,
>> phys_addr_t guest_ipa,
>> return ret;
>> }
>>
>> +/**
>> + * kvm_phys_addr_memremap - map a memory range to guest IPA
>> + *
>> + * @kvm:The KVM pointer
>> + * @guest_ipa: The IPA at which to insert the mapping
>> + * @pa: The physical address of the memory
>> + * @size: The size of the mapping
>> + */
>> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> + phys_addr_t pa,
Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On Sat, 3 Aug 2019 13:51:13 +0100
Marc Zyngier wrote:
[forgot that one]
> On Fri, 2 Aug 2019 15:50:14 +0100
> Steven Price wrote:
[...]
> > +static int __init kvm_pvtime_init(void)
> > +{
> > + kvm_register_device_ops(_ops, KVM_DEV_TYPE_ARM_PV_TIME);
> > +
> > + return 0;
> > +}
> > +
> > +late_initcall(kvm_pvtime_init);
Why is it an initcall? So far, the only initcall we've used is the one
that initializes KVM itself. Can't we just the device_ops just like we
do for the vgic?
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH 6/9] KVM: arm64: Provide a PV_TIME device to user space
On Fri, 2 Aug 2019 15:50:14 +0100
Steven Price wrote:
> Allow user space to inform the KVM host where in the physical memory
> map the paravirtualized time structures should be located.
>
> A device is created which provides the base address of an array of
> Stolen Time (ST) structures, one for each VCPU. There must be (64 *
> total number of VCPUs) bytes of memory available at this location.
>
> The address is given in terms of the physical address visible to
> the guest and must be 64 byte aligned. The memory should be marked as
> reserved to the guest to stop it allocating it for other purposes.
Why? You seem to be allocating the memory from the kernel, so as far as
the guest is concerned, this isn't generally usable memory.
>
> Signed-off-by: Steven Price
> ---
> arch/arm64/include/asm/kvm_mmu.h | 2 +
> arch/arm64/include/uapi/asm/kvm.h | 6 +
> arch/arm64/kvm/Makefile | 1 +
> include/uapi/linux/kvm.h | 2 +
> virt/kvm/arm/mmu.c| 44 +++
> virt/kvm/arm/pvtime.c | 190 ++
> 6 files changed, 245 insertions(+)
> create mode 100644 virt/kvm/arm/pvtime.c
>
> diff --git a/arch/arm64/include/asm/kvm_mmu.h
> b/arch/arm64/include/asm/kvm_mmu.h
> index befe37d4bc0e..88c8a4b2836f 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -157,6 +157,8 @@ int kvm_alloc_stage2_pgd(struct kvm *kvm);
> void kvm_free_stage2_pgd(struct kvm *kvm);
> int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> phys_addr_t pa, unsigned long size, bool writable);
> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
> + phys_addr_t pa, unsigned long size, bool writable);
>
> int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run);
>
> diff --git a/arch/arm64/include/uapi/asm/kvm.h
> b/arch/arm64/include/uapi/asm/kvm.h
> index 9a507716ae2f..95516a4198ea 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -367,6 +367,12 @@ struct kvm_vcpu_events {
> #define KVM_PSCI_RET_INVAL PSCI_RET_INVALID_PARAMS
> #define KVM_PSCI_RET_DENIED PSCI_RET_DENIED
>
> +/* Device Control API: PV_TIME */
> +#define KVM_DEV_ARM_PV_TIME_PADDR0
> +#define KVM_DEV_ARM_PV_TIME_ST 0
> +#define KVM_DEV_ARM_PV_TIME_STATE_SIZE 1
> +#define KVM_DEV_ARM_PV_TIME_STATE2
> +
> #endif
>
> #endif /* __ARM_KVM_H__ */
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 73dce4d47d47..5ffbdc39e780 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -14,6 +14,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o
> $(KVM)/coalesced_mmio.o $(KVM)/e
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o $(KVM)/arm/mmu.o
> $(KVM)/arm/mmio.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/psci.o $(KVM)/arm/perf.o
> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hypercalls.o
> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/pvtime.o
>
> kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o va_layout.o
> kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index a7c19540ce21..04bffafa0708 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1222,6 +1222,8 @@ enum kvm_device_type {
> #define KVM_DEV_TYPE_ARM_VGIC_ITSKVM_DEV_TYPE_ARM_VGIC_ITS
> KVM_DEV_TYPE_XIVE,
> #define KVM_DEV_TYPE_XIVEKVM_DEV_TYPE_XIVE
> + KVM_DEV_TYPE_ARM_PV_TIME,
> +#define KVM_DEV_TYPE_ARM_PV_TIME KVM_DEV_TYPE_ARM_PV_TIME
> KVM_DEV_TYPE_MAX,
> };
>
> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> index 38b4c910b6c3..be28a4aee451 100644
> --- a/virt/kvm/arm/mmu.c
> +++ b/virt/kvm/arm/mmu.c
> @@ -1368,6 +1368,50 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t
> guest_ipa,
> return ret;
> }
>
> +/**
> + * kvm_phys_addr_memremap - map a memory range to guest IPA
> + *
> + * @kvm: The KVM pointer
> + * @guest_ipa: The IPA at which to insert the mapping
> + * @pa: The physical address of the memory
> + * @size:The size of the mapping
> + */
> +int kvm_phys_addr_memremap(struct kvm *kvm, phys_addr_t guest_ipa,
> + phys_addr_t pa, unsigned long size, bool writable)
> +{
> + phys_addr_t addr, end;
> + int ret = 0;
> + unsigned long pfn;
> + struct kvm_mmu_memory_cache cache = { 0, };
> +
> + end = (guest_ipa + size + PAGE_SIZE - 1) & PAGE_MASK;
> + pfn = __phys_to_pfn(pa);
> +
> + for (addr = guest_ipa; addr < end; addr += PAGE_SIZE) {
> + pte_t pte = pfn_pte(pfn, PAGE_S2);
> +
> + if (writable)
> + pte = kvm_s2pte_mkwrite(pte);
> +
> + ret = mmu_topup_memory_cache(,
> + kvm_mmu_cache_min_pages(kvm),
> +
