Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On 12/18/2019 01:19 PM, Michael S. Tsirkin wrote:
On Wed, Dec 18, 2019 at 11:18:45AM +0800, Wei Wang wrote:
On 12/18/2019 03:06 AM, Daniel Verkamp wrote:
Ensure that elements of the array that correspond to unavailable
features are set to NULL; previously, they would be left uninitialized.
Since the corresponding names array elements were explicitly set to
NULL, the uninitialized callback pointers would not actually be
dereferenced; however, the uninitialized callbacks elements would still
be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
vectors required.
With your 2nd patch:
if (names[i] && callbacks[i])
++nvectors;
It seems this patch isn't necessary as names[i] is already NULL, isn't it?
Best,
Wei
Right but passing uninitialized values isn't nice even if
the function called happens to ignore them.
Hmm.. then please make one more change:
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
-callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
}
And I think all the commit description isn't accurate then, it seems to
be a coding style improvement,
instead of affecting "calculate the number of MSI-X vectors required".
Best,
Wei
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] drivers/vhost : Removes unnecessary 'else' in vhost_copy_from_user
On 2019/12/13 上午5:15, Leonardo Bras wrote:
There is no need for this else statement, given that if block will return.
This change is not supposed to change the output binary.
It reduces identation level on most lines in this function, and also
fixes an split string on vq_err().
Signed-off-by: Leonardo Bras
---
drivers/vhost/vhost.c | 50 +--
1 file changed, 24 insertions(+), 26 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index f44340b41494..b23d1b74c32f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -824,34 +824,32 @@ static int vhost_copy_from_user(struct vhost_virtqueue
*vq, void *to,
if (!vq->iotlb)
return __copy_from_user(to, from, size);
- else {
- /* This function should be called after iotlb
-* prefetch, which means we're sure that vq
-* could be access through iotlb. So -EAGAIN should
-* not happen in this case.
-*/
- void __user *uaddr = vhost_vq_meta_fetch(vq,
-(u64)(uintptr_t)from, size,
-VHOST_ADDR_DESC);
- struct iov_iter f;
- if (uaddr)
- return __copy_from_user(to, uaddr, size);
+ /* This function should be called after iotlb
+* prefetch, which means we're sure that vq
+* could be access through iotlb. So -EAGAIN should
+* not happen in this case.
+*/
+ void __user *uaddr = vhost_vq_meta_fetch(vq,
+(u64)(uintptr_t)from, size,
+VHOST_ADDR_DESC);
+ struct iov_iter f;
I think this will lead at least warnings from compiler.
Generally, I would not bother to make changes like this especially
consider it will bring troubles when trying to backporting fixes to
downstream in the future.
There're some more interesting things: e.g current metadata IOTLB
performance is bad for dynamic mapping since it will be reset each time
a new updating is coming.
We can optimize this by only reset the metadata IOTLB when the updating
is for metdata.
Want to try this?
Thanks
- ret = translate_desc(vq, (u64)(uintptr_t)from, size, vq->iotlb_iov,
-ARRAY_SIZE(vq->iotlb_iov),
-VHOST_ACCESS_RO);
- if (ret < 0) {
- vq_err(vq, "IOTLB translation failure: uaddr "
- "%p size 0x%llx\n", from,
- (unsigned long long) size);
- goto out;
- }
- iov_iter_init(, READ, vq->iotlb_iov, ret, size);
- ret = copy_from_iter(to, size, );
- if (ret == size)
- ret = 0;
- }
+ if (uaddr)
+ return __copy_from_user(to, uaddr, size);
+
+ ret = translate_desc(vq, (u64)(uintptr_t)from, size, vq->iotlb_iov,
+ARRAY_SIZE(vq->iotlb_iov),
+VHOST_ACCESS_RO);
+ if (ret < 0) {
+ vq_err(vq, "IOTLB translation failure: uaddr %p size 0x%llx\n",
+ from, (unsigned long long)size);
+ goto out;
+ }
+ iov_iter_init(, READ, vq->iotlb_iov, ret, size);
+ ret = copy_from_iter(to, size, );
+ if (ret == size)
+ ret = 0;
out:
return ret;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-pci: check name when counting MSI-X vectors
On Tue, Dec 17, 2019 at 11:06:10AM -0800, Daniel Verkamp wrote:
> VQs without a name specified are not valid; they are skipped in the
> later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
> loop above that counts the required number of vectors previously still
> counted any queue with a non-NULL callback as needing a vector.
>
> Add a check to the per_vq_vectors loop so that vectors with no name are
> not counted to make the two loops consistent. This prevents
> over-counting unnecessary vectors (e.g. for features which were not
> negotiated with the device).
>
> Signed-off-by: Daniel Verkamp
OK so I will add
Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
here too?
> ---
> drivers/virtio/virtio_pci_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index f2862f66c2ac..222d630c41fc 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
> unsigned nvqs,
> /* Best option: one for change interrupt, one per vq. */
> nvectors = 1;
> for (i = 0; i < nvqs; ++i)
> - if (callbacks[i])
> + if (names[i] && callbacks[i])
> ++nvectors;
> } else {
> /* Second best: one for change, shared for all vqs. */
> --
> 2.24.1.735.g03f4e72817-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On Wed, Dec 18, 2019 at 11:18:45AM +0800, Wei Wang wrote: > On 12/18/2019 03:06 AM, Daniel Verkamp wrote: > > Ensure that elements of the array that correspond to unavailable > > features are set to NULL; previously, they would be left uninitialized. > > > > Since the corresponding names array elements were explicitly set to > > NULL, the uninitialized callback pointers would not actually be > > dereferenced; however, the uninitialized callbacks elements would still > > be read in vp_find_vqs_msix() and used to calculate the number of MSI-X > > vectors required. > > With your 2nd patch: > if (names[i] && callbacks[i]) > ++nvectors; > > It seems this patch isn't necessary as names[i] is already NULL, isn't it? > > Best, > Wei Right but passing uninitialized values isn't nice even if the function called happens to ignore them. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On 12/18/2019 03:06 AM, Daniel Verkamp wrote: Ensure that elements of the array that correspond to unavailable features are set to NULL; previously, they would be left uninitialized. Since the corresponding names array elements were explicitly set to NULL, the uninitialized callback pointers would not actually be dereferenced; however, the uninitialized callbacks elements would still be read in vp_find_vqs_msix() and used to calculate the number of MSI-X vectors required. With your 2nd patch: if (names[i] && callbacks[i]) ++nvectors; It seems this patch isn't necessary as names[i] is already NULL, isn't it? Best, Wei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On Tue, Dec 17, 2019 at 12:05 PM Michael S. Tsirkin wrote:
>
> On Tue, Dec 17, 2019 at 11:06:09AM -0800, Daniel Verkamp wrote:
> > Ensure that elements of the array that correspond to unavailable
> > features are set to NULL; previously, they would be left uninitialized.
> >
> > Since the corresponding names array elements were explicitly set to
> > NULL, the uninitialized callback pointers would not actually be
> > dereferenced; however, the uninitialized callbacks elements would still
> > be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
> > vectors required.
> >
> > Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> > Signed-off-by: Daniel Verkamp
>
> Actually, we already have the issue with the stats VQ, no?
>
> So I think this one is more appropriate:
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon
> driver (V4)")
I think things were OK in 9564e138b1f6 because nvqs was calculated
based on the available features, so the later elements of the array
would not have been inspected by find_vqs. 86a559787e6f introduced
the uninitialized array elements as well as the removal of dynamic
nvqs based on features.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-balloon: initialize all vq callbacks
On Tue, Dec 17, 2019 at 11:06:09AM -0800, Daniel Verkamp wrote:
> Ensure that elements of the array that correspond to unavailable
> features are set to NULL; previously, they would be left uninitialized.
>
> Since the corresponding names array elements were explicitly set to
> NULL, the uninitialized callback pointers would not actually be
> dereferenced; however, the uninitialized callbacks elements would still
> be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
> vectors required.
>
> Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Signed-off-by: Daniel Verkamp
Actually, we already have the issue with the stats VQ, no?
So I think this one is more appropriate:
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon
driver (V4)")
> ---
> drivers/virtio/virtio_balloon.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 93f995f6cf36..8e400ece9273 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb)
> names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> + callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
> names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> + callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> --
> 2.24.1.735.g03f4e72817-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-pci: check name when counting MSI-X vectors
On Tue, Dec 17, 2019 at 11:06:10AM -0800, Daniel Verkamp wrote:
> VQs without a name specified are not valid; they are skipped in the
> later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
> loop above that counts the required number of vectors previously still
> counted any queue with a non-NULL callback as needing a vector.
>
> Add a check to the per_vq_vectors loop so that vectors with no name are
> not counted to make the two loops consistent. This prevents
> over-counting unnecessary vectors (e.g. for features which were not
> negotiated with the device).
>
> Signed-off-by: Daniel Verkamp
And I'll add:
Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon
driver (V4)")
here too.
> ---
> drivers/virtio/virtio_pci_common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index f2862f66c2ac..222d630c41fc 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
> unsigned nvqs,
> /* Best option: one for change interrupt, one per vq. */
> nvectors = 1;
> for (i = 0; i < nvqs; ++i)
> - if (callbacks[i])
> + if (names[i] && callbacks[i])
> ++nvectors;
> } else {
> /* Second best: one for change, shared for all vqs. */
> --
> 2.24.1.735.g03f4e72817-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/2] virtio-balloon: initialize all vq callbacks
Ensure that elements of the array that correspond to unavailable
features are set to NULL; previously, they would be left uninitialized.
Since the corresponding names array elements were explicitly set to
NULL, the uninitialized callback pointers would not actually be
dereferenced; however, the uninitialized callbacks elements would still
be read in vp_find_vqs_msix() and used to calculate the number of MSI-X
vectors required.
Fixes: 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
Signed-off-by: Daniel Verkamp
---
drivers/virtio/virtio_balloon.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 93f995f6cf36..8e400ece9273 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -475,7 +475,9 @@ static int init_vqs(struct virtio_balloon *vb)
names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
+ callbacks[VIRTIO_BALLOON_VQ_STATS] = NULL;
names[VIRTIO_BALLOON_VQ_STATS] = NULL;
+ callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
--
2.24.1.735.g03f4e72817-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/2] virtio-pci: check name when counting MSI-X vectors
VQs without a name specified are not valid; they are skipped in the
later loop that assigns MSI-X vectors to queues, but the per_vq_vectors
loop above that counts the required number of vectors previously still
counted any queue with a non-NULL callback as needing a vector.
Add a check to the per_vq_vectors loop so that vectors with no name are
not counted to make the two loops consistent. This prevents
over-counting unnecessary vectors (e.g. for features which were not
negotiated with the device).
Signed-off-by: Daniel Verkamp
---
drivers/virtio/virtio_pci_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index f2862f66c2ac..222d630c41fc 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -294,7 +294,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned nvqs,
/* Best option: one for change interrupt, one per vq. */
nvectors = 1;
for (i = 0; i < nvqs; ++i)
- if (callbacks[i])
+ if (names[i] && callbacks[i])
++nvectors;
} else {
/* Second best: one for change, shared for all vqs. */
--
2.24.1.735.g03f4e72817-goog
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: request nvqs based on features
On Tue, Dec 17, 2019 at 1:11 AM Cornelia Huck wrote:
>
> On Mon, 16 Dec 2019 15:14:29 -0800
> Daniel Verkamp wrote:
>
> > After 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"),
> > the virtio-balloon device unconditionally specifies 4 virtqueues as the
> > argument to find_vqs(), which means that 5 MSI-X vectors are required in
> > order to assign one vector per VQ plus one for configuration changes.
> >
> > However, in cases where the virtio device only provides exactly as many
> > MSI-X vectors as required for the features it implements (e.g. 3 for the
> > basic configuration of inflate + deflate + config), this results in the
> > selection of the fallback configuration where one interrupt vector is
> > used for all VQs instead of having one VQ per vector.
> >
> > Restore the logic that chose nvqs conditionally based on enabled
> > features, which was removed as part of the aforementioned commit.
> > This is slightly more complex than just incrementing a counter of the
> > number of VQs, since the queue for a given feature is assigned a fixed
> > index.
>
> As Wei already said, this should not be necessary, but see below.
>
> >
> > Signed-off-by: Daniel Verkamp
> > ---
> > drivers/virtio/virtio_balloon.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c
> > b/drivers/virtio/virtio_balloon.c
> > index 93f995f6cf36..67c6318d77c7 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -465,6 +465,7 @@ static int init_vqs(struct virtio_balloon *vb)
> > vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> > const char *names[VIRTIO_BALLOON_VQ_MAX];
> > int err;
> > + unsigned nvqs;
> >
> > /*
> >* Inflateq and deflateq are used unconditionally. The names[]
> > @@ -475,20 +476,24 @@ static int init_vqs(struct virtio_balloon *vb)
> > names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> > callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> > names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> > + nvqs = VIRTIO_BALLOON_VQ_DEFLATE + 1;
> > +
> > names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> > names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>
> Note that we set names[q] to NULL, but not callbacks[q].
Indeed, good catch - this looks like the root cause of the problem,
since callbacks for unavailable features are left uninitialized.
> >
> > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> > names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> > callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> > + nvqs = VIRTIO_BALLOON_VQ_STATS + 1;
> > }
> >
> > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> > callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> > + nvqs = VIRTIO_BALLOON_VQ_FREE_PAGE + 1;
> > }
> >
> > - err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
> > + err = vb->vdev->config->find_vqs(vb->vdev, nvqs,
> >vqs, callbacks, names, NULL, NULL);
>
> This will end up in vp_find_vqs_msix() eventually, which will try to
> determine the number of needed vectors based upon whether callbacks[q]
> is !NULL. That's probably the reason you end up trying to use more
> vectors than needed. (Further down in that function, the code will skip
> any queue with names[q] == NULL, but that's too late for determining
> the number of vectors.)
>
> So I think that either (a) virtio-pci should avoid trying to allocate a
> vector for queues with names[q] == NULL, or (b) virtio-balloon should
> clean out callbacks[q] for unused queues as well. Maybe both?
I've tested a patch for (b), which resolves the problem I was seeing
as well; this looks like a clear undefined behavior fix, so I'll send
at least that one.
I will also put together a patch for (a), although now that I
understand the find_vqs logic better, I think this mostly shouldn't be
necessary assuming virtio drivers correctly initialize the parameters
they pass to it.
Thanks,
-- Daniel
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: request nvqs based on features
On Tue, Dec 17, 2019 at 10:11:08AM +0100, Cornelia Huck wrote:
> On Mon, 16 Dec 2019 15:14:29 -0800
> Daniel Verkamp wrote:
>
> > After 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"),
> > the virtio-balloon device unconditionally specifies 4 virtqueues as the
> > argument to find_vqs(), which means that 5 MSI-X vectors are required in
> > order to assign one vector per VQ plus one for configuration changes.
> >
> > However, in cases where the virtio device only provides exactly as many
> > MSI-X vectors as required for the features it implements (e.g. 3 for the
> > basic configuration of inflate + deflate + config), this results in the
> > selection of the fallback configuration where one interrupt vector is
> > used for all VQs instead of having one VQ per vector.
> >
> > Restore the logic that chose nvqs conditionally based on enabled
> > features, which was removed as part of the aforementioned commit.
> > This is slightly more complex than just incrementing a counter of the
> > number of VQs, since the queue for a given feature is assigned a fixed
> > index.
>
> As Wei already said, this should not be necessary, but see below.
>
> >
> > Signed-off-by: Daniel Verkamp
> > ---
> > drivers/virtio/virtio_balloon.c | 7 ++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c
> > b/drivers/virtio/virtio_balloon.c
> > index 93f995f6cf36..67c6318d77c7 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -465,6 +465,7 @@ static int init_vqs(struct virtio_balloon *vb)
> > vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> > const char *names[VIRTIO_BALLOON_VQ_MAX];
> > int err;
> > + unsigned nvqs;
> >
> > /*
> > * Inflateq and deflateq are used unconditionally. The names[]
> > @@ -475,20 +476,24 @@ static int init_vqs(struct virtio_balloon *vb)
> > names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> > callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> > names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> > + nvqs = VIRTIO_BALLOON_VQ_DEFLATE + 1;
> > +
> > names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> > names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
>
> Note that we set names[q] to NULL, but not callbacks[q].
>
> >
> > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> > names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> > callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> > + nvqs = VIRTIO_BALLOON_VQ_STATS + 1;
> > }
> >
> > if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> > names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> > callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> > + nvqs = VIRTIO_BALLOON_VQ_FREE_PAGE + 1;
> > }
> >
> > - err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
> > + err = vb->vdev->config->find_vqs(vb->vdev, nvqs,
> > vqs, callbacks, names, NULL, NULL);
>
> This will end up in vp_find_vqs_msix() eventually, which will try to
> determine the number of needed vectors based upon whether callbacks[q]
> is !NULL. That's probably the reason you end up trying to use more
> vectors than needed. (Further down in that function, the code will skip
> any queue with names[q] == NULL, but that's too late for determining
> the number of vectors.)
> So I think that either (a) virtio-pci should avoid trying to allocate a
> vector for queues with names[q] == NULL, or (b) virtio-balloon should
> clean out callbacks[q] for unused queues as well. Maybe both?
>
> > if (err)
> > return err;
I'm inclined to either do a or both.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/5] KVM: arm64: Document PV-lock interface
From: Zengruan Ye Introduce a paravirtualization interface for KVM/arm64 to obtain the vcpu is currently running or not. A hypercall interface is provided for the guest to interrogate the hypervisor's support for this interface and the location of the shared memory structures. Signed-off-by: Zengruan Ye --- Documentation/virt/kvm/arm/pvlock.rst | 31 +++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/virt/kvm/arm/pvlock.rst diff --git a/Documentation/virt/kvm/arm/pvlock.rst b/Documentation/virt/kvm/arm/pvlock.rst new file mode 100644 index ..eec0c36edf17 --- /dev/null +++ b/Documentation/virt/kvm/arm/pvlock.rst @@ -0,0 +1,31 @@ +.. SPDX-License-Identifier: GPL-2.0 + +Paravirtualized lock support for arm64 +== + +KVM/arm64 provids some hypervisor service calls to support a paravirtualized +guest obtaining the vcpu is currently running or not. + +Two new SMCCC compatible hypercalls are defined: + +* PV_LOCK_FEATURES: 0xC540 +* PV_LOCK_PREEMPTED: 0xC541 + +The existence of the PV_LOCK hypercall should be probed using the SMCCC 1.1 +ARCH_FEATURES mechanism before calling it. + +PV_LOCK_FEATURES += == +Function ID: (uint32)0xC540 +PV_call_id: (uint32)The function to query for support. +Return value: (int64) NOT_SUPPORTED (-1) or SUCCESS (0) if the relevant + PV-lock feature is supported by the hypervisor. += == + +PV_LOCK_PREEMPTED += == +Function ID: (uint32)0xC541 +Return value: (int64) NOT_SUPPORTED (-1) or SUCCESS (0) if the IPA of + this vcpu's pv data structure is configured by + the hypervisor. += == -- 2.19.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 5/5] KVM: arm64: Support the vcpu preemption check
From: Zengruan Ye
Support the vcpu_is_preempted() functionality under KVM/arm64. This will
enhance lock performance on overcommitted hosts (more runnable vcpus
than physical cpus in the system) as doing busy waits for preempted
vcpus will hurt system performance far worse than early yielding.
unix benchmark result:
host: kernel 5.5.0-rc1, HiSilicon Kunpeng920, 8 cpus
guest: kernel 5.5.0-rc1, 16 vcpus
test-case|after-patch| before-patch
+---+--
Dhrystone 2 using register variables | 334600751.0 lps | 335319028.3 lps
Double-Precision Whetstone | 32856.1 MWIPS | 32849.6 MWIPS
Execl Throughput | 3662.1 lps | 2718.0 lps
File Copy 1024 bufsize 2000 maxblocks |432906.4 KBps |158011.8 KBps
File Copy 256 bufsize 500 maxblocks|116023.0 KBps | 37664.0 KBps
File Copy 4096 bufsize 8000 maxblocks | 1432769.8 KBps |441108.8 KBps
Pipe Throughput| 6405029.6 lps | 6021457.6 lps
Pipe-based Context Switching |185872.7 lps |184255.3 lps
Process Creation | 4025.7 lps | 3706.6 lps
Shell Scripts (1 concurrent) | 6745.6 lpm | 6436.1 lpm
Shell Scripts (8 concurrent) | 998.7 lpm | 931.1 lpm
System Call Overhead | 3913363.1 lps | 3883287.8 lps
+---+--
System Benchmarks Index Score | 1835.1 | 1327.6
Signed-off-by: Zengruan Ye
---
arch/arm64/include/asm/paravirt.h | 3 +
arch/arm64/kernel/paravirt.c | 91 +++
arch/arm64/kernel/setup.c | 2 +
include/linux/cpuhotplug.h| 1 +
4 files changed, 97 insertions(+)
diff --git a/arch/arm64/include/asm/paravirt.h
b/arch/arm64/include/asm/paravirt.h
index 7b1c81b544bb..a2cd0183bbef 100644
--- a/arch/arm64/include/asm/paravirt.h
+++ b/arch/arm64/include/asm/paravirt.h
@@ -29,6 +29,8 @@ static inline u64 paravirt_steal_clock(int cpu)
int __init pv_time_init(void);
+int __init kvm_guest_init(void);
+
__visible bool __native_vcpu_is_preempted(int cpu);
static inline bool pv_vcpu_is_preempted(int cpu)
@@ -39,6 +41,7 @@ static inline bool pv_vcpu_is_preempted(int cpu)
#else
#define pv_time_init() do {} while (0)
+#define kvm_guest_init() do {} while (0)
#endif // CONFIG_PARAVIRT
diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
index d8f1ba8c22ce..a86dead40473 100644
--- a/arch/arm64/kernel/paravirt.c
+++ b/arch/arm64/kernel/paravirt.c
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
struct static_key paravirt_steal_enabled;
struct static_key paravirt_steal_rq_enabled;
@@ -158,3 +159,93 @@ int __init pv_time_init(void)
return 0;
}
+
+DEFINE_PER_CPU(struct pvlock_vcpu_state, pvlock_vcpu_region) __aligned(64);
+EXPORT_PER_CPU_SYMBOL(pvlock_vcpu_region);
+
+static int pvlock_vcpu_state_dying_cpu(unsigned int cpu)
+{
+ struct pvlock_vcpu_state *reg;
+
+ reg = this_cpu_ptr(_vcpu_region);
+ if (!reg)
+ return -EFAULT;
+
+ memset(reg, 0, sizeof(*reg));
+
+ return 0;
+}
+
+static int init_pvlock_vcpu_state(unsigned int cpu)
+{
+ struct pvlock_vcpu_state *reg;
+ struct arm_smccc_res res;
+
+ reg = this_cpu_ptr(_vcpu_region);
+ if (!reg)
+ return -EFAULT;
+
+ /* Pass the memory address to host via hypercall */
+ arm_smccc_1_1_invoke(ARM_SMCCC_HV_PV_LOCK_PREEMPTED,
+virt_to_phys(reg), );
+
+ return 0;
+}
+
+static bool kvm_vcpu_is_preempted(int cpu)
+{
+ struct pvlock_vcpu_state *reg = _cpu(pvlock_vcpu_region, cpu);
+
+ if (reg)
+ return !!(reg->preempted & 1);
+
+ return false;
+}
+
+static int kvm_arm_init_pvlock(void)
+{
+ int ret;
+
+ ret = cpuhp_setup_state(CPUHP_AP_ARM_KVM_PVLOCK_STARTING,
+ "hypervisor/arm/pvlock:starting",
+ init_pvlock_vcpu_state,
+ pvlock_vcpu_state_dying_cpu);
+ if (ret < 0)
+ return ret;
+
+ pv_ops.lock.vcpu_is_preempted = kvm_vcpu_is_preempted;
+
+ pr_info("using PV-lock preempted\n");
+
+ return 0;
+}
+
+static bool has_kvm_pvlock(void)
+{
+ struct arm_smccc_res res;
+
+ /* To detect the presence of PV lock support we require SMCCC 1.1+ */
+ if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
+ return false;
+
+ arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
+ARM_SMCCC_HV_PV_LOCK_FEATURES, );
+
+ if (res.a0 != SMCCC_RET_SUCCESS)
+ return false;
+
+ return true;
+}
+
+int __init kvm_guest_init(void)
+{
+
[PATCH 2/5] KVM: arm64: Implement PV_LOCK_FEATURES call
From: Zengruan Ye
This provides a mechanism for querying which paravirtualized lock
features are available in this hypervisor.
Also add the header file which defines the ABI for the paravirtualized
lock features we're about to add.
Signed-off-by: Zengruan Ye
---
arch/arm64/include/asm/pvlock-abi.h | 16
include/linux/arm-smccc.h | 13 +
virt/kvm/arm/hypercalls.c | 3 +++
3 files changed, 32 insertions(+)
create mode 100644 arch/arm64/include/asm/pvlock-abi.h
diff --git a/arch/arm64/include/asm/pvlock-abi.h
b/arch/arm64/include/asm/pvlock-abi.h
new file mode 100644
index ..06e0c3d7710a
--- /dev/null
+++ b/arch/arm64/include/asm/pvlock-abi.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright(c) 2019 Huawei Technologies Co., Ltd
+ * Author: Zengruan Ye
+ */
+
+#ifndef __ASM_PVLOCK_ABI_H
+#define __ASM_PVLOCK_ABI_H
+
+struct pvlock_vcpu_state {
+ __le64 preempted;
+ /* Structure must be 64 byte aligned, pad to that size */
+ u8 padding[56];
+} __packed;
+
+#endif
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 59494df0f55b..59e65a951959 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -377,5 +377,18 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned
long a1,
ARM_SMCCC_OWNER_STANDARD_HYP,\
0x21)
+/* Paravirtualised lock calls */
+#define ARM_SMCCC_HV_PV_LOCK_FEATURES \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_64,\
+ ARM_SMCCC_OWNER_STANDARD_HYP,\
+ 0x40)
+
+#define ARM_SMCCC_HV_PV_LOCK_PREEMPTED \
+ ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_SMC_64,\
+ ARM_SMCCC_OWNER_STANDARD_HYP,\
+ 0x41)
+
#endif /*__ASSEMBLY__*/
#endif /*__LINUX_ARM_SMCCC_H*/
diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
index 550dfa3e53cd..ff13871fd85a 100644
--- a/virt/kvm/arm/hypercalls.c
+++ b/virt/kvm/arm/hypercalls.c
@@ -52,6 +52,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
case ARM_SMCCC_HV_PV_TIME_FEATURES:
val = SMCCC_RET_SUCCESS;
break;
+ case ARM_SMCCC_HV_PV_LOCK_FEATURES:
+ val = SMCCC_RET_SUCCESS;
+ break;
}
break;
case ARM_SMCCC_HV_PV_TIME_FEATURES:
--
2.19.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 0/5] KVM: arm64: vcpu preempted check support
From: Zengruan Ye This patch set aims to support the vcpu_is_preempted() functionality under KVM/arm64, which allowing the guest to obtain the vcpu is currently running or not. This will enhance lock performance on overcommitted hosts (more runnable vcpus than physical cpus in the system) as doing busy waits for preempted vcpus will hurt system performance far worse than early yielding. We have observed some performace improvements in uninx benchmark tests. unix benchmark result: host: kernel 5.5.0-rc1, HiSilicon Kunpeng920, 8 cpus guest: kernel 5.5.0-rc1, 16 vcpus test-case|after-patch| before-patch +---+-- Dhrystone 2 using register variables | 334600751.0 lps | 335319028.3 lps Double-Precision Whetstone | 32856.1 MWIPS | 32849.6 MWIPS Execl Throughput | 3662.1 lps |2718.0 lps File Copy 1024 bufsize 2000 maxblocks |432906.4 KBps | 158011.8 KBps File Copy 256 bufsize 500 maxblocks|116023.0 KBps | 37664.0 KBps File Copy 4096 bufsize 8000 maxblocks | 1432769.8 KBps | 441108.8 KBps Pipe Throughput| 6405029.6 lps | 6021457.6 lps Pipe-based Context Switching |185872.7 lps | 184255.3 lps Process Creation | 4025.7 lps |3706.6 lps Shell Scripts (1 concurrent) | 6745.6 lpm |6436.1 lpm Shell Scripts (8 concurrent) | 998.7 lpm | 931.1 lpm System Call Overhead | 3913363.1 lps | 3883287.8 lps +---+-- System Benchmarks Index Score | 1835.1 |1327.6 Zengruan Ye (5): KVM: arm64: Document PV-lock interface KVM: arm64: Implement PV_LOCK_FEATURES call KVM: arm64: Support pvlock preempted via shared structure KVM: arm64: Add interface to support vcpu preempted check KVM: arm64: Support the vcpu preemption check Documentation/virt/kvm/arm/pvlock.rst | 31 + arch/arm/include/asm/kvm_host.h| 13 arch/arm64/include/asm/kvm_host.h | 17 + arch/arm64/include/asm/paravirt.h | 15 arch/arm64/include/asm/pvlock-abi.h| 16 + arch/arm64/include/asm/spinlock.h | 7 ++ arch/arm64/kernel/Makefile | 2 +- arch/arm64/kernel/paravirt-spinlocks.c | 13 arch/arm64/kernel/paravirt.c | 95 +- arch/arm64/kernel/setup.c | 2 + arch/arm64/kvm/Makefile| 1 + include/linux/arm-smccc.h | 13 include/linux/cpuhotplug.h | 1 + virt/kvm/arm/arm.c | 8 +++ virt/kvm/arm/hypercalls.c | 7 ++ virt/kvm/arm/pvlock.c | 21 ++ 16 files changed, 260 insertions(+), 2 deletions(-) create mode 100644 Documentation/virt/kvm/arm/pvlock.rst create mode 100644 arch/arm64/include/asm/pvlock-abi.h create mode 100644 arch/arm64/kernel/paravirt-spinlocks.c create mode 100644 virt/kvm/arm/pvlock.c -- 2.19.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 3/5] KVM: arm64: Support pvlock preempted via shared structure
From: Zengruan Ye
Implement the service call for configuring a shared structure between a
vcpu and the hypervisor in which the hypervisor can tell the vcpu is
running or not.
The preempted field is zero if 1) some old KVM deos not support this filed.
2) the vcpu is not preempted. Other values means the vcpu has been preempted.
Signed-off-by: Zengruan Ye
---
arch/arm/include/asm/kvm_host.h | 13 +
arch/arm64/include/asm/kvm_host.h | 17 +
arch/arm64/kvm/Makefile | 1 +
virt/kvm/arm/arm.c| 8
virt/kvm/arm/hypercalls.c | 4
virt/kvm/arm/pvlock.c | 21 +
6 files changed, 64 insertions(+)
create mode 100644 virt/kvm/arm/pvlock.c
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 556cd818eccf..098375f1c89e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -356,6 +356,19 @@ static inline bool kvm_arm_is_pvtime_enabled(struct
kvm_vcpu_arch *vcpu_arch)
return false;
}
+static inline void kvm_arm_pvlock_preempted_init(struct kvm_vcpu_arch
*vcpu_arch)
+{
+}
+
+static inline bool kvm_arm_is_pvlock_preempted_ready(struct kvm_vcpu_arch
*vcpu_arch)
+{
+ return false;
+}
+
+static inline void kvm_update_pvlock_preempted(struct kvm_vcpu *vcpu, u64
preempted)
+{
+}
+
void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index c61260cf63c5..d9b2a21a87ac 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -354,6 +354,11 @@ struct kvm_vcpu_arch {
u64 last_steal;
gpa_t base;
} steal;
+
+ /* Guest PV lock state */
+ struct {
+ gpa_t base;
+ } pv;
};
/* Pointer to the vcpu's SVE FFR for sve_{save,load}_state() */
@@ -515,6 +520,18 @@ static inline bool kvm_arm_is_pvtime_enabled(struct
kvm_vcpu_arch *vcpu_arch)
return (vcpu_arch->steal.base != GPA_INVALID);
}
+static inline void kvm_arm_pvlock_preempted_init(struct kvm_vcpu_arch
*vcpu_arch)
+{
+ vcpu_arch->pv.base = GPA_INVALID;
+}
+
+static inline bool kvm_arm_is_pvlock_preempted_ready(struct kvm_vcpu_arch
*vcpu_arch)
+{
+ return (vcpu_arch->pv.base != GPA_INVALID);
+}
+
+void kvm_update_pvlock_preempted(struct kvm_vcpu *vcpu, u64 preempted);
+
void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 5ffbdc39e780..e4591f56d5f1 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -15,6 +15,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arm.o
$(KVM)/arm/mmu.o $(KVM)/arm/mmio.
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) += $(KVM)/arm/pvlock.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/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 12e0280291ce..c562f62fdd45 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -383,6 +383,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
kvm_arm_pvtime_vcpu_init(>arch);
+ kvm_arm_pvlock_preempted_init(>arch);
+
return kvm_vgic_vcpu_init(vcpu);
}
@@ -421,6 +423,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
vcpu_set_wfx_traps(vcpu);
vcpu_ptrauth_setup_lazy(vcpu);
+
+ if (kvm_arm_is_pvlock_preempted_ready(>arch))
+ kvm_update_pvlock_preempted(vcpu, 0);
}
void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -434,6 +439,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
vcpu->cpu = -1;
kvm_arm_set_running_vcpu(NULL);
+
+ if (kvm_arm_is_pvlock_preempted_ready(>arch))
+ kvm_update_pvlock_preempted(vcpu, 1);
}
static void vcpu_power_off(struct kvm_vcpu *vcpu)
diff --git a/virt/kvm/arm/hypercalls.c b/virt/kvm/arm/hypercalls.c
index ff13871fd85a..5964982ccd05 100644
--- a/virt/kvm/arm/hypercalls.c
+++ b/virt/kvm/arm/hypercalls.c
@@ -65,6 +65,10 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
if (gpa != GPA_INVALID)
val = gpa;
break;
+ case ARM_SMCCC_HV_PV_LOCK_PREEMPTED:
+ vcpu->arch.pv.base = smccc_get_arg1(vcpu);
+ val = SMCCC_RET_SUCCESS;
+ break;
default:
return kvm_psci_call(vcpu);
}
diff --git a/virt/kvm/arm/pvlock.c b/virt/kvm/arm/pvlock.c
new file mode 100644
index ..c3464958b0f5
--- /dev/null
+++ b/virt/kvm/arm/pvlock.c
@@ -0,0 +1,21 @@
+/*
[PATCH 4/5] KVM: arm64: Add interface to support vcpu preempted check
From: Zengruan Ye
This is to fix some lock holder preemption issues. Some other locks
implementation do a spin loop before acquiring the lock itself.
Currently kernel has an interface of bool vcpu_is_preempted(int cpu). It
takes the cpu as parameter and return true if the cpu is preempted.
Then kernel can break the spin loops upon the retval of vcpu_is_preempted.
As kernel has used this interface, So lets support it.
Signed-off-by: Zengruan Ye
---
arch/arm64/include/asm/paravirt.h | 12
arch/arm64/include/asm/spinlock.h | 7 +++
arch/arm64/kernel/Makefile | 2 +-
arch/arm64/kernel/paravirt-spinlocks.c | 13 +
arch/arm64/kernel/paravirt.c | 4 +++-
5 files changed, 36 insertions(+), 2 deletions(-)
create mode 100644 arch/arm64/kernel/paravirt-spinlocks.c
diff --git a/arch/arm64/include/asm/paravirt.h
b/arch/arm64/include/asm/paravirt.h
index cf3a0fd7c1a7..7b1c81b544bb 100644
--- a/arch/arm64/include/asm/paravirt.h
+++ b/arch/arm64/include/asm/paravirt.h
@@ -11,8 +11,13 @@ struct pv_time_ops {
unsigned long long (*steal_clock)(int cpu);
};
+struct pv_lock_ops {
+ bool (*vcpu_is_preempted)(int cpu);
+};
+
struct paravirt_patch_template {
struct pv_time_ops time;
+ struct pv_lock_ops lock;
};
extern struct paravirt_patch_template pv_ops;
@@ -24,6 +29,13 @@ static inline u64 paravirt_steal_clock(int cpu)
int __init pv_time_init(void);
+__visible bool __native_vcpu_is_preempted(int cpu);
+
+static inline bool pv_vcpu_is_preempted(int cpu)
+{
+ return pv_ops.lock.vcpu_is_preempted(cpu);
+}
+
#else
#define pv_time_init() do {} while (0)
diff --git a/arch/arm64/include/asm/spinlock.h
b/arch/arm64/include/asm/spinlock.h
index b093b287babf..45ff1b2949a6 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -7,8 +7,15 @@
#include
#include
+#include
/* See include/linux/spinlock.h */
#define smp_mb__after_spinlock() smp_mb()
+#define vcpu_is_preempted vcpu_is_preempted
+static inline bool vcpu_is_preempted(long cpu)
+{
+ return pv_vcpu_is_preempted(cpu);
+}
+
#endif /* __ASM_SPINLOCK_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index fc6488660f64..b23cdae433a4 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -50,7 +50,7 @@ obj-$(CONFIG_ARMV8_DEPRECATED)+=
armv8_deprecated.o
obj-$(CONFIG_ACPI) += acpi.o
obj-$(CONFIG_ACPI_NUMA)+= acpi_numa.o
obj-$(CONFIG_ARM64_ACPI_PARKING_PROTOCOL) += acpi_parking_protocol.o
-obj-$(CONFIG_PARAVIRT) += paravirt.o
+obj-$(CONFIG_PARAVIRT) += paravirt.o paravirt-spinlocks.o
obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o
obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o
\
diff --git a/arch/arm64/kernel/paravirt-spinlocks.c
b/arch/arm64/kernel/paravirt-spinlocks.c
new file mode 100644
index ..718aa773d45c
--- /dev/null
+++ b/arch/arm64/kernel/paravirt-spinlocks.c
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright(c) 2019 Huawei Technologies Co., Ltd
+ * Author: Zengruan Ye
+ */
+
+#include
+#include
+
+__visible bool __native_vcpu_is_preempted(int cpu)
+{
+ return false;
+}
diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
index 1ef702b0be2d..d8f1ba8c22ce 100644
--- a/arch/arm64/kernel/paravirt.c
+++ b/arch/arm64/kernel/paravirt.c
@@ -26,7 +26,9 @@
struct static_key paravirt_steal_enabled;
struct static_key paravirt_steal_rq_enabled;
-struct paravirt_patch_template pv_ops;
+struct paravirt_patch_template pv_ops = {
+ .lock.vcpu_is_preempted = __native_vcpu_is_preempted,
+};
EXPORT_SYMBOL_GPL(pv_ops);
struct pv_time_stolen_time_region {
--
2.19.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/5] iommu: arm: Use iommu_put_resv_regions_simple()
On Mon, Dec 09, 2019 at 03:50:04PM +0100, Thierry Reding wrote: > From: Thierry Reding > > Use the new standard function instead of open-coding it. > > Cc: Will Deacon > Cc: Robin Murphy > Signed-off-by: Thierry Reding > --- > drivers/iommu/arm-smmu-v3.c | 11 +-- > drivers/iommu/arm-smmu.c| 11 +-- > 2 files changed, 2 insertions(+), 20 deletions(-) Acked-by: Will Deacon Will ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 0/5] iommu: Implement iommu_put_resv_regions_simple()
Hi Thierry On Mon, Dec 09, 2019 at 03:50:02PM +0100, Thierry Reding wrote: > From: Thierry Reding > > Most IOMMU drivers only need to free the memory allocated for each > reserved region. Instead of open-coding the loop to do this in each > driver, extract the code into a common function that can be used by > all these drivers. > > Changes in v2: > - change subject prefix to "iommu: virtio: " for virtio-iommu.c driver > > Thierry > > Thierry Reding (5): > iommu: Implement iommu_put_resv_regions_simple() > iommu: arm: Use iommu_put_resv_regions_simple() > iommu: amd: Use iommu_put_resv_regions_simple() > iommu: intel: Use iommu_put_resv_regions_simple() > iommu: virtio: Use iommu_put_resv_regions_simple() Thanks, that is a nice consolidation. Just a minor nit, can you please rename iommu_put_resv_regions_simple to generic_iommu_put_resv_regsions(). That matches the naming in other places where we have done similar things. Thanks, Joerg ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: request nvqs based on features
On Mon, 16 Dec 2019 15:14:29 -0800
Daniel Verkamp wrote:
> After 86a559787e6f ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT"),
> the virtio-balloon device unconditionally specifies 4 virtqueues as the
> argument to find_vqs(), which means that 5 MSI-X vectors are required in
> order to assign one vector per VQ plus one for configuration changes.
>
> However, in cases where the virtio device only provides exactly as many
> MSI-X vectors as required for the features it implements (e.g. 3 for the
> basic configuration of inflate + deflate + config), this results in the
> selection of the fallback configuration where one interrupt vector is
> used for all VQs instead of having one VQ per vector.
>
> Restore the logic that chose nvqs conditionally based on enabled
> features, which was removed as part of the aforementioned commit.
> This is slightly more complex than just incrementing a counter of the
> number of VQs, since the queue for a given feature is assigned a fixed
> index.
As Wei already said, this should not be necessary, but see below.
>
> Signed-off-by: Daniel Verkamp
> ---
> drivers/virtio/virtio_balloon.c | 7 ++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 93f995f6cf36..67c6318d77c7 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -465,6 +465,7 @@ static int init_vqs(struct virtio_balloon *vb)
> vq_callback_t *callbacks[VIRTIO_BALLOON_VQ_MAX];
> const char *names[VIRTIO_BALLOON_VQ_MAX];
> int err;
> + unsigned nvqs;
>
> /*
>* Inflateq and deflateq are used unconditionally. The names[]
> @@ -475,20 +476,24 @@ static int init_vqs(struct virtio_balloon *vb)
> names[VIRTIO_BALLOON_VQ_INFLATE] = "inflate";
> callbacks[VIRTIO_BALLOON_VQ_DEFLATE] = balloon_ack;
> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
> + nvqs = VIRTIO_BALLOON_VQ_DEFLATE + 1;
> +
> names[VIRTIO_BALLOON_VQ_STATS] = NULL;
> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
Note that we set names[q] to NULL, but not callbacks[q].
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> names[VIRTIO_BALLOON_VQ_STATS] = "stats";
> callbacks[VIRTIO_BALLOON_VQ_STATS] = stats_request;
> + nvqs = VIRTIO_BALLOON_VQ_STATS + 1;
> }
>
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = "free_page_vq";
> callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
> + nvqs = VIRTIO_BALLOON_VQ_FREE_PAGE + 1;
> }
>
> - err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs,
>vqs, callbacks, names, NULL, NULL);
This will end up in vp_find_vqs_msix() eventually, which will try to
determine the number of needed vectors based upon whether callbacks[q]
is !NULL. That's probably the reason you end up trying to use more
vectors than needed. (Further down in that function, the code will skip
any queue with names[q] == NULL, but that's too late for determining
the number of vectors.)
So I think that either (a) virtio-pci should avoid trying to allocate a
vector for queues with names[q] == NULL, or (b) virtio-balloon should
clean out callbacks[q] for unused queues as well. Maybe both?
> if (err)
> return err;
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
