kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Steven Price <steven.price@arm.com>
To: Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu
Cc: "Mark Rutland" <mark.rutland@arm.com>,
	kvm@vger.kernel.org, "Radim Krčmář" <rkrcmar@redhat.com>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	"Suzuki K Pouloze" <suzuki.poulose@arm.com>,
	linux-doc@vger.kernel.org, "Russell King" <linux@armlinux.org.uk>,
	linux-kernel@vger.kernel.org, "James Morse" <james.morse@arm.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Julien Thierry" <julien.thierry.kdev@gmail.com>
Subject: Re: [PATCH v4 07/10] KVM: arm64: Provide VCPU attributes for stolen time
Date: Fri, 30 Aug 2019 16:04:52 +0100	[thread overview]
Message-ID: <31f2bb01-7376-ec4d-9885-e4ca7135eb4f@arm.com> (raw)
In-Reply-To: <36104ec0-2237-fb0e-376f-ab50c23c6101@kernel.org>

On 30/08/2019 11:02, Marc Zyngier wrote:
> On 30/08/2019 09:42, Steven Price wrote:
>> Allow user space to inform the KVM host where in the physical memory
>> map the paravirtualized time structures should be located.
>>
>> User space can set an attribute on the VCPU providing the IPA base
>> address of the stolen time structure for that VCPU. This must be
>> repeated for every VCPU in the VM.
>>
>> The address is given in terms of the physical address visible to
>> the guest and must be 64 byte aligned. The guest will discover the
>> address via a hypercall.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  7 +++++
>>  arch/arm64/include/uapi/asm/kvm.h |  2 ++
>>  arch/arm64/kvm/guest.c            |  9 ++++++
>>  include/uapi/linux/kvm.h          |  2 ++
>>  virt/kvm/arm/pvtime.c             | 47 +++++++++++++++++++++++++++++++
>>  5 files changed, 67 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 1697e63f6dd8..6af16b29a41f 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -489,6 +489,13 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu);
>>  long kvm_hypercall_stolen_time(struct kvm_vcpu *vcpu);
>>  int kvm_update_stolen_time(struct kvm_vcpu *vcpu, bool init);
>>  
>> +int kvm_arm_pvtime_set_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr);
>> +int kvm_arm_pvtime_get_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr);
>> +int kvm_arm_pvtime_has_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr);
>> +
>>  static inline void kvm_arm_pvtime_vcpu_init(struct kvm_vcpu_arch *vcpu_arch)
>>  {
>>  	vcpu_arch->steal.base = GPA_INVALID;
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
>> index 9a507716ae2f..bde9f165ad3a 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -323,6 +323,8 @@ struct kvm_vcpu_events {
>>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
>> +#define KVM_ARM_VCPU_PVTIME_CTRL	2
>> +#define   KVM_ARM_VCPU_PVTIME_SET_IPA	0
>>  
>>  /* KVM_IRQ_LINE irq field index values */
>>  #define KVM_ARM_IRQ_TYPE_SHIFT		24
>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
>> index dfd626447482..d3ac9d2fd405 100644
>> --- a/arch/arm64/kvm/guest.c
>> +++ b/arch/arm64/kvm/guest.c
>> @@ -858,6 +858,9 @@ int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu,
>>  	case KVM_ARM_VCPU_TIMER_CTRL:
>>  		ret = kvm_arm_timer_set_attr(vcpu, attr);
>>  		break;
>> +	case KVM_ARM_VCPU_PVTIME_CTRL:
>> +		ret = kvm_arm_pvtime_set_attr(vcpu, attr);
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> @@ -878,6 +881,9 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,
>>  	case KVM_ARM_VCPU_TIMER_CTRL:
>>  		ret = kvm_arm_timer_get_attr(vcpu, attr);
>>  		break;
>> +	case KVM_ARM_VCPU_PVTIME_CTRL:
>> +		ret = kvm_arm_pvtime_get_attr(vcpu, attr);
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> @@ -898,6 +904,9 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,
>>  	case KVM_ARM_VCPU_TIMER_CTRL:
>>  		ret = kvm_arm_timer_has_attr(vcpu, attr);
>>  		break;
>> +	case KVM_ARM_VCPU_PVTIME_CTRL:
>> +		ret = kvm_arm_pvtime_has_attr(vcpu, attr);
>> +		break;
>>  	default:
>>  		ret = -ENXIO;
>>  		break;
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 5e3f12d5359e..265156a984f2 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_TIME	KVM_DEV_TYPE_ARM_PV_TIME
>>  	KVM_DEV_TYPE_MAX,
>>  };
>>  
>> diff --git a/virt/kvm/arm/pvtime.c b/virt/kvm/arm/pvtime.c
>> index d9d0dbc6994b..7b1834b98a68 100644
>> --- a/virt/kvm/arm/pvtime.c
>> +++ b/virt/kvm/arm/pvtime.c
>> @@ -2,7 +2,9 @@
>>  // Copyright (C) 2019 Arm Ltd.
>>  
>>  #include <linux/arm-smccc.h>
>> +#include <linux/kvm_host.h>
>>  
>> +#include <asm/kvm_mmu.h>
>>  #include <asm/pvclock-abi.h>
>>  
>>  #include <kvm/arm_hypercalls.h>
>> @@ -75,3 +77,48 @@ long kvm_hypercall_stolen_time(struct kvm_vcpu *vcpu)
>>  
>>  	return vcpu->arch.steal.base;
>>  }
>> +
>> +int kvm_arm_pvtime_set_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr)
>> +{
>> +	u64 __user *user = (u64 __user *)attr->addr;
>> +	u64 ipa;
>> +
>> +	if (attr->attr != KVM_ARM_VCPU_PVTIME_SET_IPA)
>> +		return -ENXIO;
>> +
>> +	if (get_user(ipa, user))
>> +		return -EFAULT;
>> +	if (ipa & 63)
> 
> nit: Please express this as !IS_ALIGNED(ipa, 64) instead.

Sure

>> +		return -EINVAL;
>> +	if (vcpu->arch.steal.base != GPA_INVALID)
>> +		return -EEXIST;
>> +	vcpu->arch.steal.base = ipa;
> 
> I'm still worried that you end-up not knowing whether the IPA is valid
> or not at this stage, nor that we check about overlapping vcpus. How do
> we validate that?

Considering we really can't reasonably validate IPA overlapping with
guest memory (how is the host to know what is 'guest memory'), I'm not
convinced it's worth the code to detect overlapping vcpus. Nothing bad
will happen to the host in this case (the kvm_put_guest() calls will
just clobber each other).

In terms of checking the IPA is valid - again this is something that can
change in the lifetime of the VM, so the check at setup time isn't
particularly useful. Currently it's also possible to create the vcpus
(including setting up the stolen time) before the memory is assigned to
the guest as long as the vcpus are not started. This seems like a useful
level of flexibility.

> I also share Christoffer's concern that the memslot parsing may be
> expensive on a system with multiple memslots. But maybe that can be
> solved by adding some caching capabilities to your kvm_put_guest(),
> should this become a problem.

Yes it should be possible to add it - I'd like to wait to see whether
user actually want to use multiple memslots though - it's quite possible
to use a single memslot for both guest memory and stolen time structures.

>> +	return 0;
>> +}
>> +
>> +int kvm_arm_pvtime_get_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr)
>> +{
>> +	u64 __user *user = (u64 __user *)attr->addr;
>> +	u64 ipa;
>> +
>> +	if (attr->attr != KVM_ARM_VCPU_PVTIME_SET_IPA)
> 
> It is a bit odd that this is using "SET_IPA" as a way to GET it.

Yes that does look weird. I'll drop the "SET_" part from the symbol. I'm
not sure what I was thinking when I named that.

Thanks,

Steve

>> +		return -ENXIO;
>> +
>> +	ipa = vcpu->arch.steal.base;
>> +
>> +	if (put_user(ipa, user))
>> +		return -EFAULT;
>> +	return 0;
>> +}
>> +
>> +int kvm_arm_pvtime_has_attr(struct kvm_vcpu *vcpu,
>> +			    struct kvm_device_attr *attr)
>> +{
>> +	switch (attr->attr) {
>> +	case KVM_ARM_VCPU_PVTIME_SET_IPA:
>> +		return 0;
>> +	}
>> +	return -ENXIO;
>> +}
>>
> 
> Thanks,
> 
> 	M.
> 


  reply	other threads:[~2019-08-30 15:05 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-30  8:42 [PATCH v4 00/10] arm64: Stolen time support Steven Price
2019-08-30  8:42 ` [PATCH v4 01/10] KVM: arm64: Document PV-time interface Steven Price
2019-08-30 14:47   ` Andrew Jones
2019-08-30 15:25     ` Steven Price
2019-09-02 12:52       ` Andrew Jones
2019-09-04 13:55         ` Steven Price
2019-09-04 14:22           ` Andrew Jones
2019-09-04 15:07             ` Steven Price
2019-08-30  8:42 ` [PATCH v4 02/10] KVM: arm/arm64: Factor out hypercall handling from PSCI code Steven Price
2019-09-02  7:06   ` kbuild test robot
2019-09-04 15:05     ` Steven Price
2019-09-02 13:07   ` kbuild test robot
2019-08-30  8:42 ` [PATCH v4 03/10] KVM: arm64: Implement PV_FEATURES call Steven Price
2019-08-30  8:42 ` [PATCH v4 04/10] KVM: Implement kvm_put_guest() Steven Price
2019-08-30  8:42 ` [PATCH v4 05/10] KVM: arm64: Support stolen time reporting via shared structure Steven Price
2019-08-30  9:42   ` Christoffer Dall
2019-08-30  9:52     ` Steven Price
2019-09-03  9:14   ` Zenghui Yu
2019-09-04 15:53     ` Steven Price
2019-08-30  8:42 ` [PATCH v4 06/10] KVM: Allow kvm_device_ops to be const Steven Price
2019-08-30  8:42 ` [PATCH v4 07/10] KVM: arm64: Provide VCPU attributes for stolen time Steven Price
2019-08-30 10:02   ` Marc Zyngier
2019-08-30 15:04     ` Steven Price [this message]
2019-08-30  8:42 ` [PATCH v4 08/10] arm/arm64: Provide a wrapper for SMCCC 1.1 calls Steven Price
2019-08-30  8:42 ` [PATCH v4 09/10] arm/arm64: Make use of the SMCCC 1.1 wrapper Steven Price
2019-08-30  8:42 ` [PATCH v4 10/10] arm64: Retrieve stolen time as paravirtualized guest Steven Price
2019-09-03  8:47   ` Andrew Jones
2019-09-04 16:01     ` Steven Price
2019-09-03  8:03 ` [PATCH v4 00/10] arm64: Stolen time support Andrew Jones
2019-09-03  8:49   ` Andrew Jones
2019-09-04 16:02     ` Steven Price

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=31f2bb01-7376-ec4d-9885-e4ca7135eb4f@arm.com \
    --to=steven.price@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=james.morse@arm.com \
    --cc=julien.thierry.kdev@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rkrcmar@redhat.com \
    --cc=suzuki.poulose@arm.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v4 07/10] KVM: arm64: Provide VCPU attributes for stolen time' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).