KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Christian Borntraeger <borntraeger@de.ibm.com>
To: David Hildenbrand <david@redhat.com>
Cc: Ulrich.Weigand@de.ibm.com, cohuck@redhat.com,
	frankja@linux.ibm.com, frankja@linux.vnet.ibm.com,
	gor@linux.ibm.com, imbrenda@linux.ibm.com, kvm@vger.kernel.org,
	linux-s390@vger.kernel.org, mimu@linux.ibm.com, thuth@redhat.com
Subject: Re: [PATCH v2.1] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling
Date: Tue, 18 Feb 2020 22:18:48 +0100
Message-ID: <f6bc712e-44b0-233e-886f-c72485ea13c3@de.ibm.com> (raw)
In-Reply-To: <42deaa19-d2ca-f1cc-3e83-af0d5d77347f@redhat.com>



On 18.02.20 10:12, David Hildenbrand wrote:
> On 18.02.20 09:39, Christian Borntraeger wrote:
>> From: Janosch Frank <frankja@linux.ibm.com>
>>
>> This contains 3 main changes:
>> 1. changes in SIE control block handling for secure guests
>> 2. helper functions for create/destroy/unpack secure guests
>> 3. KVM_S390_PV_COMMAND ioctl to allow userspace dealing with secure
>> machines
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> [borntraeger@de.ibm.com: patch merging, splitting, fixing]
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> 2->2.1  - combine CREATE/DESTROY CPU/VM into ENABLE DISABLE
>> 	- rework locking and check locks with lockdep
>> 	- I still have the PV_COMMAND_CPU in here for later use in
>> 	  the SET_IPL_PSW ioctl. If wanted I can move
> 
> I'd prefer to move, and eventually just turn this into a clean, separate
> ioctl without subcommands (e.g., if we'll only need a single subcommand
> in the near future). And it makes this patch a alittle easier to review
> ... :)
> 

Maybe the MP_STATE solution will work out. Then we can get rid of this.
will look into that when dealing with the other patch.

> [...]

> 
> Once we lock the VCPU, it cannot be running, right?

Right, while holding the vcpu->mutex, KVM_RUN is blocked. 

> 
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		mutex_lock(&vcpu->mutex);
>> +		r = kvm_s390_pv_destroy_cpu(vcpu, rc, rrc);
>> +		mutex_unlock(&vcpu->mutex);
>> +		if (r)
>> +			break;
>> +	}
> 
> Can this actually ever fail? If so, you would leave half-initialized
> state around. Warn and continue?
> 
> Especially, kvm_arch_vcpu_destroy() ignores any error from
> kvm_s390_pv_destroy_cpu() as well ...
> 
> IMHO, we should make kvm_s390_switch_from_pv() and
> kvm_s390_pv_destroy_cpu() never fail.

will answer this part tomorrow.

> 
>> +	return r;
>> +}
>> +
>> +static int kvm_s390_switch_to_pv(struct kvm *kvm, u16 *rc, u16 *rrc)
>> +{
>> +	int i, r = 0;
>> +	u16 dummy;
>> +
>> +	struct kvm_vcpu *vcpu;
>> +
>> +	kvm_for_each_vcpu(i, vcpu, kvm) {
>> +		mutex_lock(&vcpu->mutex);
>> +		r = kvm_s390_pv_create_cpu(vcpu, rc, rrc);
>> +		mutex_unlock(&vcpu->mutex);
>> +		if (r)
>> +			break;
>> +	}
>> +	if (r)
>> +		kvm_s390_switch_from_pv(kvm,&dummy, &dummy);
>> +	return r;
>> +}
>> +
>> +static int kvm_s390_handle_pv(struct kvm *kvm, struct kvm_pv_cmd *cmd)
>> +{
>> +	int r = 0;
>> +	u16 dummy;
>> +	void __user *argp = (void __user *)cmd->data;
>> +
>> +	switch (cmd->cmd) {
>> +	case KVM_PV_ENABLE: {
>> +		r = -EINVAL;
>> +		if (kvm_s390_pv_is_protected(kvm))
>> +			break;
> 
> Why not factor out this check, it's common for all sucommands.

Unfortunately it is not common. Sometimes it has an "!" sometimes not.

> 
>> +
>> +		r = kvm_s390_pv_alloc_vm(kvm);
>> +		if (r)
>> +			break;
>> +
>> +		kvm_s390_vcpu_block_all(kvm);
> 
> As kvm_s390_vcpu_block_all() does not support nesting, this will not
> work as expected - sca_switch_to_extended() already blocks. Are the
> vcpu->locks not enough?

Right, we would need to move the sca_switch out of this lock. On the other
hand the vcpu->locks should be sufficent for the host integrity (and also
for the secure guest integrity))
So I will just remove the block_all here and below.
> 
> [...]
> 
>> @@ -2558,10 +2735,16 @@ static void kvm_free_vcpus(struct kvm *kvm)
>>  
>>  void kvm_arch_destroy_vm(struct kvm *kvm)
>>  {
>> +	u16 rc, rrc;
>>  	kvm_free_vcpus(kvm);
>>  	sca_dispose(kvm);
>> -	debug_unregister(kvm->arch.dbf);
>>  	kvm_s390_gisa_destroy(kvm);
>> +	/* do not use the lock checking variant at tear-down */
>> +	if (kvm_s390_pv_handle(kvm)) {
> 
> kvm_s390_pv_is_protected ? I dislike using kvm_s390_pv_handle() when
> we're not interested in the handle.

Then I would need to take the kvm->mutex here. This is why I added
the comment. We are already at the last steps of killing the kvm
and the lock is not held. So the lockdep check would complain.
On the other hand grabbing kvm->lock is pointless as the kvm file
descriptor is already gone so the ioctls that we would protect against
cannot happen.


What about Improving the comment:

        /*
         * We are already at the end of life and kvm->lock is not taken.
         * This is ok as the file descriptor is closed by now and nobody
         * can mess with the pv state. To avoid lockdep_assert_held from
         * complaining we do not use kvm_s390_pv_is_protected.
         */

On the other hand This looks a bit too verbose.




[...]

>> +static inline u64 kvm_s390_pv_handle(struct kvm *kvm)
>> +{
>> +	return kvm->arch.pv.handle;
>> +}
> 
> Can we rename this to
> 
> kvm_s390_pv_get_handle()

ack.
> 
>> +
>> +static inline u64 kvm_s390_pv_handle_cpu(struct kvm_vcpu *vcpu)
>> +{
>> +	return vcpu->arch.pv.handle;
>> +}
> 
> Can we rename this to kvm_s390_pv_cpu_get_handle() ? (so it doesn't look
> like the function will handle something)

ack. 

[...]
>> +int kvm_s390_pv_alloc_vm(struct kvm *kvm)
>> +{
>> +	unsigned long base = uv_info.guest_base_stor_len;
>> +	unsigned long virt = uv_info.guest_virt_var_stor_len;
>> +	unsigned long npages = 0, vlen = 0;
>> +	struct kvm_memory_slot *memslot;
>> +
>> +	kvm->arch.pv.stor_var = NULL;
>> +	kvm->arch.pv.stor_base = __get_free_pages(GFP_KERNEL, get_order(base));
>> +	if (!kvm->arch.pv.stor_base)
>> +		return -ENOMEM;
>> +
>> +	/*
>> +	 * Calculate current guest storage for allocation of the
>> +	 * variable storage, which is based on the length in MB.
>> +	 *
>> +	 * Slots are sorted by GFN
>> +	 */
>> +	mutex_lock(&kvm->slots_lock);
>> +	memslot = kvm_memslots(kvm)->memslots;
>> +	npages = memslot->base_gfn + memslot->npages;
>> +	mutex_unlock(&kvm->slots_lock);
> 
> Are you blocking the addition of new memslots somehow?
> 

>> +int kvm_s390_pv_create_vm(struct kvm *kvm, u16 *rc, u16 *rrc)
>> +{
>> +	u16 drc, drrc;
>> +	int cc;
>> +
>> +	struct uv_cb_cgc uvcb = {
>> +		.header.cmd = UVC_CMD_CREATE_SEC_CONF,
>> +		.header.len = sizeof(uvcb)
>> +	};
>> +
>> +	if (kvm_s390_pv_handle(kvm))
> 
> Why is that necessary? We should only be called in PV mode.

It is not because the caller already checks. It was necessary in a previous version I guess.

[...]
>> +int kvm_s390_pv_set_sec_parms(struct kvm *kvm, void *hdr, u64 length, u16 *rc,
>> +			      u16 *rrc)
>> +{
>> +	struct uv_cb_ssc uvcb = {
>> +		.header.cmd = UVC_CMD_SET_SEC_CONF_PARAMS,
>> +		.header.len = sizeof(uvcb),
>> +		.sec_header_origin = (u64)hdr,
>> +		.sec_header_len = length,
>> +		.guest_handle = kvm_s390_pv_handle(kvm),
>> +	};
>> +	int cc;
>> +
>> +	if (!kvm_s390_pv_handle(kvm))
> 
> Why is that necessary? We should only be called in PV mode.

ack


[...]

>> +struct kvm_s390_pv_sec_parm {
>> +	__u64	origin;
>> +	__u64	length;
> 
> tabs vs. spaces. (I'd use a single space like in kvm_s390_pv_unp below)

ack.
[...]


  reply index

Thread overview: 132+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 22:26 [PATCH v2 00/42] KVM: s390: Add support for protected VMs Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 01/42] mm:gup/writeback: add callbacks for inaccessible pages Christian Borntraeger
2020-02-17  9:14   ` David Hildenbrand
2020-02-17 11:10     ` Christian Borntraeger
2020-02-18  8:27       ` David Hildenbrand
2020-02-18 15:46         ` Sean Christopherson
2020-02-18 16:02           ` Will Deacon
2020-02-18 16:15             ` Christian Borntraeger
2020-02-18 21:35               ` Sean Christopherson
2020-02-19  8:31                 ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 02/42] KVM: s390/interrupt: do not pin adapter interrupt pages Christian Borntraeger
2020-02-17  9:43   ` David Hildenbrand
2020-02-20 12:18     ` David Hildenbrand
2020-02-20 13:31     ` Christian Borntraeger
2020-02-20 13:34       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 03/42] s390/protvirt: introduce host side setup Christian Borntraeger
2020-02-17  9:53   ` David Hildenbrand
2020-02-17 11:11     ` Christian Borntraeger
2020-02-17 11:13       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 04/42] s390/protvirt: add ultravisor initialization Christian Borntraeger
2020-02-17  9:57   ` David Hildenbrand
2020-02-17 11:13     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 05/42] s390/mm: provide memory management functions for protected KVM guests Christian Borntraeger
2020-02-17 10:21   ` David Hildenbrand
2020-02-17 11:28     ` Christian Borntraeger
2020-02-17 12:07       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 06/42] s390/mm: add (non)secure page access exceptions handlers Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 07/42] KVM: s390: protvirt: Add UV debug trace Christian Borntraeger
2020-02-17 10:41   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 08/42] KVM: s390: add new variants of UV CALL Christian Borntraeger
2020-02-17 10:42   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 09/42] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling Christian Borntraeger
2020-02-17 10:56   ` David Hildenbrand
2020-02-17 12:04     ` Christian Borntraeger
2020-02-17 12:09       ` David Hildenbrand
2020-02-17 14:53         ` [PATCH 0/2] example changes Christian Borntraeger
2020-02-17 14:53           ` [PATCH 1/2] lock changes Christian Borntraeger
2020-02-17 14:53           ` [PATCH 2/2] merge vm/cpu create Christian Borntraeger
2020-02-17 15:00             ` Janosch Frank
2020-02-17 15:02               ` Christian Borntraeger
2020-02-19 11:02               ` Christian Borntraeger
2020-02-17 19:18           ` [PATCH 0/2] example changes David Hildenbrand
2020-02-18  8:09     ` [PATCH v2 09/42] KVM: s390: protvirt: Add initial vm and cpu lifecycle handling Christian Borntraeger
2020-02-18  8:39   ` [PATCH v2.1] " Christian Borntraeger
2020-02-18  9:12     ` David Hildenbrand
2020-02-18 21:18       ` Christian Borntraeger [this message]
2020-02-19  8:32         ` David Hildenbrand
2020-02-19 11:01       ` Christian Borntraeger
2020-02-18  9:56     ` David Hildenbrand
2020-02-18 20:26       ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 10/42] KVM: s390: protvirt: Add KVM api documentation Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 11/42] KVM: s390: protvirt: Secure memory is not mergeable Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 12/42] KVM: s390/mm: Make pages accessible before destroying the guest Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 13/42] KVM: s390: protvirt: Handle SE notification interceptions Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 14/42] KVM: s390: protvirt: Instruction emulation Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 15/42] KVM: s390: protvirt: Add interruption injection controls Christian Borntraeger
2020-02-17 10:59   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 16/42] KVM: s390: protvirt: Implement interruption injection Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 17/42] KVM: s390: protvirt: Add SCLP interrupt handling Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 18/42] KVM: s390: protvirt: Handle spec exception loops Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 19/42] KVM: s390: protvirt: Add new gprs location handling Christian Borntraeger
2020-02-17 11:01   ` David Hildenbrand
2020-02-17 11:33     ` Christian Borntraeger
2020-02-17 14:37     ` Janosch Frank
2020-02-14 22:26 ` [PATCH v2 20/42] KVM: S390: protvirt: Introduce instruction data area bounce buffer Christian Borntraeger
2020-02-17 11:08   ` David Hildenbrand
2020-02-17 14:47     ` Janosch Frank
2020-02-17 15:00       ` Christian Borntraeger
2020-02-17 15:38         ` Janosch Frank
2020-02-17 16:58           ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 21/42] KVM: s390: protvirt: handle secure guest prefix pages Christian Borntraeger
2020-02-17 11:11   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 22/42] KVM: s390/mm: handle guest unpin events Christian Borntraeger
2020-02-17 14:23   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 23/42] KVM: s390: protvirt: Write sthyi data to instruction data area Christian Borntraeger
2020-02-17 14:24   ` David Hildenbrand
2020-02-17 18:40     ` Christian Borntraeger
2020-02-17 19:16       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 24/42] KVM: s390: protvirt: STSI handling Christian Borntraeger
2020-02-18  8:35   ` David Hildenbrand
2020-02-18  8:44     ` Christian Borntraeger
2020-02-18  9:08       ` David Hildenbrand
2020-02-18  9:11         ` Christian Borntraeger
2020-02-18  9:13           ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 25/42] KVM: s390: protvirt: disallow one_reg Christian Borntraeger
2020-02-18  8:40   ` David Hildenbrand
2020-02-18  8:57     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 26/42] KVM: s390: protvirt: Do only reset registers that are accessible Christian Borntraeger
2020-02-18  8:42   ` David Hildenbrand
2020-02-18  9:20     ` Christian Borntraeger
2020-02-18  9:28       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 27/42] KVM: s390: protvirt: Only sync fmt4 registers Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 28/42] KVM: s390: protvirt: Add program exception injection Christian Borntraeger
2020-02-18  9:33   ` David Hildenbrand
2020-02-18  9:37     ` Christian Borntraeger
2020-02-18  9:39       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 29/42] KVM: s390: protvirt: Add diag 308 subcode 8 - 10 handling Christian Borntraeger
2020-02-18  9:38   ` David Hildenbrand
2020-02-19 12:45     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 30/42] KVM: s390: protvirt: UV calls in support of diag308 0, 1 Christian Borntraeger
2020-02-18  9:44   ` David Hildenbrand
2020-02-19 11:53     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 31/42] KVM: s390: protvirt: Report CPU state to Ultravisor Christian Borntraeger
2020-02-18  9:48   ` David Hildenbrand
2020-02-19 19:36     ` Christian Borntraeger
2020-02-19 19:46       ` Christian Borntraeger
2020-02-20 10:52         ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 32/42] KVM: s390: protvirt: Support cmd 5 operation state Christian Borntraeger
2020-02-18  9:50   ` David Hildenbrand
2020-02-19 11:06     ` Christian Borntraeger
2020-02-19 11:08       ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 33/42] KVM: s390: protvirt: Mask PSW interrupt bits for interception 104 and 112 Christian Borntraeger
2020-02-18  9:53   ` David Hildenbrand
2020-02-18 10:02     ` David Hildenbrand
2020-02-18 10:05     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 34/42] KVM: s390: protvirt: do not inject interrupts after start Christian Borntraeger
2020-02-18  9:53   ` David Hildenbrand
2020-02-18 10:02     ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 35/42] KVM: s390: protvirt: Add UV cpu reset calls Christian Borntraeger
2020-02-18  9:54   ` David Hildenbrand
2020-02-14 22:26 ` [PATCH v2 36/42] DOCUMENTATION: Protected virtual machine introduction and IPL Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 37/42] s390/uv: Fix handling of length extensions (already in s390 tree) Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 38/42] s390: protvirt: Add sysfs firmware interface for Ultravisor information Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 39/42] example for future extension: mm:gup/writeback: add callbacks for inaccessible pages: error cases Christian Borntraeger
2020-02-18 16:25   ` Will Deacon
2020-02-18 16:30     ` Christian Borntraeger
2020-02-18 16:33       ` Will Deacon
2020-02-14 22:26 ` [PATCH v2 40/42] example for future extension: mm:gup/writeback: add callbacks for inaccessible pages: source indication Christian Borntraeger
2020-02-17 14:15   ` Ulrich Weigand
2020-02-17 14:38     ` Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 41/42] potential fixup for "s390/mm: provide memory management functions for protected KVM guests" Christian Borntraeger
2020-02-14 22:26 ` [PATCH v2 42/42] KVM: s390: rstify new ioctls in api.rst Christian Borntraeger

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=f6bc712e-44b0-233e-886f-c72485ea13c3@de.ibm.com \
    --to=borntraeger@de.ibm.com \
    --cc=Ulrich.Weigand@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=david@redhat.com \
    --cc=frankja@linux.ibm.com \
    --cc=frankja@linux.vnet.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=imbrenda@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mimu@linux.ibm.com \
    --cc=thuth@redhat.com \
    /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

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git