From: Halil Pasic <pasic@linux.ibm.com>
To: Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
Janosch Frank <frankja@linux.ibm.com>,
David Hildenbrand <david@redhat.com>,
Cornelia Huck <cohuck@redhat.com>,
Heiko Carstens <hca@linux.ibm.com>,
Vasily Gorbik <gor@linux.ibm.com>,
kvm@vger.kernel.org, linux-s390@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Michael Mueller <mimu@linux.ibm.com>,
Halil Pasic <pasic@linux.ibm.com>
Subject: Re: [PATCH 1/1] KVM: s390: index kvm->arch.idle_mask by vcpu_idx
Date: Fri, 27 Aug 2021 23:19:21 +0200 [thread overview]
Message-ID: <20210827231921.267ad3df.pasic@linux.ibm.com> (raw)
In-Reply-To: <20210827160616.532d6699@p-imbrenda>
On Fri, 27 Aug 2021 16:06:16 +0200
Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> On Fri, 27 Aug 2021 14:54:29 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
>
> > While in practice vcpu->vcpu_idx == vcpu->vcp_id is often true,
s/vcp_id/vcpu_id/
> > it may not always be, and we must not rely on this.
>
> why?
>
> maybe add a simple explanation of why vcpu_idx and vcpu_id can be
> different, namely:
> KVM decides the vcpu_idx, userspace decides the vcpu_id, thus the two
> might not match
Not sure that is a good explanation. A quote from
Documentation/virt/kvm/api.rst:
"""
4.7 KVM_CREATE_VCPU
-------------------
:Capability: basic
:Architectures: all
:Type: vm ioctl
:Parameters: vcpu id (apic id on x86)
:Returns: vcpu fd on success, -1 on error
This API adds a vcpu to a virtual machine. No more than max_vcpus may be added.
The vcpu id is an integer in the range [0, max_vcpu_id).
The recommended max_vcpus value can be retrieved using the KVM_CAP_NR_VCPUS of
the KVM_CHECK_EXTENSION ioctl() at run-time.
The maximum possible value for max_vcpus can be retrieved using the
KVM_CAP_MAX_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time.
"""
Based on that and a quick look at the code, it looks to me like the
set of valid vcpu_id values are a subset of the range of vcpu_idx-es,
i.e. that kvm could decide to choose vcpu_id for the value of vcpu_idx.
I don't think it should, but it could. Were the set of valid vcpu_id
values not a subset of the set of supported vcpu_idx values, then one
could argue that this is why.
I didn't want to get into explaining the why, I just wanted to state the
fact.
>
> >
> > Currently kvm->arch.idle_mask is indexed by vcpu_id, which implies
> > that code like
> > for_each_set_bit(vcpu_id, kvm->arch.idle_mask, online_vcpus) {
> > vcpu = kvm_get_vcpu(kvm, vcpu_id);
>
> you can also add a sentence to clarify that kvm_get_vcpu expects an
> vcpu_idx, not an vcpu_id.
maybe ...
>
> > do_stuff(vcpu);
> > }
> > is not legit. The trouble is, we do actually use kvm->arch.idle_mask
... s/legit\./legit, because kvm_get_vcpu() expects a vcpu_idx and not a
vcpu_id.
But I agree kvm_get_vcpu(kvm, vcpu_id); does not scream BUG at me while
kvm_get_vcpu_by_idx(kvm, vcpu_id) would look much more suspicious.
[..]
>
> otherwise looks good to me.
>
> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Thanks for your reveiew!
Halil
[..]
next prev parent reply other threads:[~2021-08-27 21:19 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-27 12:54 [PATCH 1/1] KVM: s390: index kvm->arch.idle_mask by vcpu_idx Halil Pasic
2021-08-27 13:42 ` Christian Borntraeger
2021-08-27 14:06 ` Claudio Imbrenda
2021-08-27 16:36 ` Christian Borntraeger
2021-08-27 21:23 ` Halil Pasic
2021-08-29 6:25 ` Christian Borntraeger
2021-08-27 21:19 ` Halil Pasic [this message]
2021-08-28 11:04 ` Michael Mueller
2022-01-31 10:13 ` Petr Tesařík
2022-01-31 11:53 ` Halil Pasic
2022-01-31 12:09 ` Petr Tesařík
2022-01-31 12:24 ` Halil Pasic
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=20210827231921.267ad3df.pasic@linux.ibm.com \
--to=pasic@linux.ibm.com \
--cc=borntraeger@de.ibm.com \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=frankja@linux.ibm.com \
--cc=gor@linux.ibm.com \
--cc=hca@linux.ibm.com \
--cc=imbrenda@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-s390@vger.kernel.org \
--cc=mimu@linux.ibm.com \
--cc=stable@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).