[PATCH v2 0/4] drm: Provide a simple encoder
Many DRM drivers implement an encoder with an empty implementation. This
patchset adds drm_simple_encoder_init() and drm_simple_encoder_create(),
which can be used by drivers instead. Except for the destroy callback, the
simple encoder's implementation is empty.
The patchset also converts 4 encoder instances to use the simple-encoder
helpers. But there are at least 11 other drivers which can use the helper
and I think I did not examine all drivers yet.
The patchset was smoke-tested on mgag200 by running the fbdev console
and Gnome on X11.
v2:
* move simple encoder to KMS helpers (Daniel)
* remove name argument; simplifies implementation (Gerd)
* don't allocate with devm_ interfaces; unsafe with DRM (Noralf)
Thomas Zimmermann (4):
drm/simple-kms: Add drm_simple_encoder_{init,create}()
drm/ast: Use simple encoder
drm/mgag200: Use simple encoder
drm/qxl: Use simple encoder
drivers/gpu/drm/ast/ast_drv.h | 6 +-
drivers/gpu/drm/ast/ast_mode.c | 25 +++-
drivers/gpu/drm/drm_simple_kms_helper.c | 83 -
drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
drivers/gpu/drm/mgag200/mgag200_mode.c | 61 +-
drivers/gpu/drm/qxl/qxl_display.c | 18 +-
include/drm/drm_simple_kms_helper.h | 7 +++
7 files changed, 102 insertions(+), 105 deletions(-)
--
2.25.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/4] drm/ast: Use simple encoder
The ast driver uses an empty implementation for its encoder. Replace
the code with the generic simple encoder.
v2:
* rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_drv.h | 6 +-
drivers/gpu/drm/ast/ast_mode.c | 25 -
2 files changed, 9 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index f5d8780776ae..656d591b154b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {
unsigned int next_index;
} cursor;
+ struct drm_encoder encoder;
struct drm_plane primary_plane;
struct drm_plane cursor_plane;
@@ -238,13 +239,8 @@ struct ast_crtc {
u8 offset_x, offset_y;
};
-struct ast_encoder {
- struct drm_encoder base;
-};
-
#define to_ast_crtc(x) container_of(x, struct ast_crtc, base)
#define to_ast_connector(x) container_of(x, struct ast_connector, base)
-#define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
struct ast_vbios_stdtable {
u8 misc;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 562ea6d9df13..7a9f20a2fd30 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -40,6 +40,7 @@
#include
#include
#include
+#include
#include "ast_drv.h"
#include "ast_tables.h"
@@ -968,28 +969,18 @@ static int ast_crtc_init(struct drm_device *dev)
* Encoder
*/
-static void ast_encoder_destroy(struct drm_encoder *encoder)
-{
- drm_encoder_cleanup(encoder);
- kfree(encoder);
-}
-
-static const struct drm_encoder_funcs ast_enc_funcs = {
- .destroy = ast_encoder_destroy,
-};
-
static int ast_encoder_init(struct drm_device *dev)
{
- struct ast_encoder *ast_encoder;
+ struct ast_private *ast = dev->dev_private;
+ struct drm_encoder *encoder = &ast->encoder;
+ int ret;
- ast_encoder = kzalloc(sizeof(struct ast_encoder), GFP_KERNEL);
- if (!ast_encoder)
- return -ENOMEM;
+ ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ if (ret)
+ return ret;
- drm_encoder_init(dev, &ast_encoder->base, &ast_enc_funcs,
-DRM_MODE_ENCODER_DAC, NULL);
+ encoder->possible_crtcs = 1;
- ast_encoder->base.possible_crtcs = 1;
return 0;
}
--
2.25.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 4/4] drm/qxl: Use simple encoder
The qxl driver uses an empty implementation for its encoder. Replace
the code with the generic simple encoder.
v2:
* rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/qxl/qxl_display.c | 18 +++---
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c
b/drivers/gpu/drm/qxl/qxl_display.c
index ab4f8dd00400..9c0e1add59fb 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -31,6 +31,7 @@
#include
#include
#include
+#include
#include "qxl_drv.h"
#include "qxl_object.h"
@@ -1007,9 +1008,6 @@ static struct drm_encoder *qxl_best_encoder(struct
drm_connector *connector)
return &qxl_output->enc;
}
-static const struct drm_encoder_helper_funcs qxl_enc_helper_funcs = {
-};
-
static const struct drm_connector_helper_funcs qxl_connector_helper_funcs = {
.get_modes = qxl_conn_get_modes,
.mode_valid = qxl_conn_mode_valid,
@@ -1059,15 +1057,6 @@ static const struct drm_connector_funcs
qxl_connector_funcs = {
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
-static void qxl_enc_destroy(struct drm_encoder *encoder)
-{
- drm_encoder_cleanup(encoder);
-}
-
-static const struct drm_encoder_funcs qxl_enc_funcs = {
- .destroy = qxl_enc_destroy,
-};
-
static int qxl_mode_create_hotplug_mode_update_property(struct qxl_device
*qdev)
{
if (qdev->hotplug_mode_update_property)
@@ -1098,15 +1087,14 @@ static int qdev_output_init(struct drm_device *dev, int
num_output)
drm_connector_init(dev, &qxl_output->base,
&qxl_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL);
- drm_encoder_init(dev, &qxl_output->enc, &qxl_enc_funcs,
-DRM_MODE_ENCODER_VIRTUAL, NULL);
+ drm_simple_encoder_init(dev, &qxl_output->enc,
+ DRM_MODE_ENCODER_VIRTUAL);
/* we get HPD via client monitors config */
connector->polled = DRM_CONNECTOR_POLL_HPD;
encoder->possible_crtcs = 1 << num_output;
drm_connector_attach_encoder(&qxl_output->base,
&qxl_output->enc);
- drm_encoder_helper_add(encoder, &qxl_enc_helper_funcs);
drm_connector_helper_add(connector, &qxl_connector_helper_funcs);
drm_object_attach_property(&connector->base,
--
2.25.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/4] drm/simple-kms: Add drm_simple_encoder_{init, create}()
This patch makes the internal encoder implementation of the simple
KMS helpers available to drivers.
These simple-encoder helpers initialize an encoder with an empty
implementation. This covers the requirements of most of the existing
DRM drivers. A call to drm_simple_encoder_create() allocates and
initializes an encoder instance, a call to drm_simple_encoder_init()
initializes a pre-allocated instance.
v2:
* move simple encoder to KMS helpers
* remove name argument; simplifies implementation
* don't allocate with devm_ interfaces; unsafe with DRM
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_simple_kms_helper.c | 83 -
include/drm/drm_simple_kms_helper.h | 7 +++
2 files changed, 87 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 15fb516ae2d8..745c2f34c42b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -26,12 +26,90 @@
* entity. Some flexibility for code reuse is provided through a separately
* allocated &drm_connector object and supporting optional &drm_bridge
* encoder drivers.
+ *
+ * Many drivers use an encoder with an empty implementation. Such encoders
+ * fulfill the minimum requirements of the display pipeline, but don't add
+ * additional functionality. The simple-encoder functions
+ * drm_simple_encoder_init() and drm_simple_encoder_create() provide an
+ * appropriate implementation.
*/
-static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
.destroy = drm_encoder_cleanup,
};
+/**
+ * drm_simple_encoder_init - Initialize a preallocated encoder
+ * @dev: drm device
+ * @funcs: callbacks for this encoder
+ * @encoder_type: user visible type of the encoder
+ *
+ * Initialises a preallocated encoder that has no further functionality. The
+ * encoder will be released automatically. Settings for possible CRTC and
+ * clones are left to their initial values. The encoder will be cleaned up
+ * automatically as part of the mode-setting cleanup.
+ *
+ * Also see drm_simple_encoder_create().
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_simple_encoder_init(struct drm_device *dev,
+ struct drm_encoder *encoder,
+ int encoder_type)
+{
+ return drm_encoder_init(dev, encoder,
+ &drm_simple_encoder_funcs_cleanup,
+ encoder_type, NULL);
+}
+EXPORT_SYMBOL(drm_simple_encoder_init);
+
+static void drm_encoder_destroy(struct drm_encoder *encoder)
+{
+ drm_encoder_cleanup(encoder);
+ kfree(encoder);
+}
+
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_destroy = {
+ .destroy = drm_encoder_destroy,
+};
+
+/**
+ * drm_simple_encoder_create - Allocate and initialize an encoder
+ * @dev: drm device
+ * @encoder_type: user visible type of the encoder
+ *
+ * Allocates and initialises an encoder that has no further functionality. The
+ * encoder will be destroyed automatically as part of the mode-setting cleanup.
+ *
+ * See drm_simple_encoder_init() for more information.
+ *
+ * Returns:
+ * The encoder on success, a pointer-encoder error code on failure.
+ */
+struct drm_encoder *drm_simple_encoder_create(struct drm_device *dev,
+ int encoder_type)
+{
+ struct drm_encoder *encoder;
+ int ret;
+
+ encoder = kzalloc(sizeof(*encoder), GFP_KERNEL);
+ if (!encoder)
+ return ERR_PTR(-ENOMEM);
+ ret = drm_encoder_init(dev, encoder,
+ &drm_simple_encoder_funcs_destroy,
+ encoder_type, NULL);
+ if (ret)
+ goto err_kfree;
+
+ return encoder;
+
+err_kfree:
+ kfree(encoder);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(drm_simple_encoder_create);
+
static enum drm_mode_status
drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
const struct drm_display_mode *mode)
@@ -288,8 +366,7 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
return ret;
encoder->possible_crtcs = drm_crtc_mask(crtc);
- ret = drm_encoder_init(dev, encoder, &drm_simple_kms_encoder_funcs,
- DRM_MODE_ENCODER_NONE, NULL);
+ ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_NONE);
if (ret || !connector)
return ret;
diff --git a/include/drm/drm_simple_kms_helper.h
b/include/drm/drm_simple_kms_helper.h
index e253ba7bea9d..54d5066d90c7 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -181,4 +181,11 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
const uint64_t *format_modifier
[PATCH v2 3/4] drm/mgag200: Use simple encoder
The mgag200 driver uses an empty implementation for its encoder. Replace
the code with the generic simple encoder.
v2:
* rebase onto new simple-encoder interface
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/mgag200/mgag200_drv.h | 7 ---
drivers/gpu/drm/mgag200/mgag200_mode.c | 61 ++
2 files changed, 3 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index aa32aad222c2..9bb9e8e14539 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -95,7 +95,6 @@
#define MATROX_DPMS_CLEARED (-1)
#define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
-#define to_mga_encoder(x) container_of(x, struct mga_encoder, base)
#define to_mga_connector(x) container_of(x, struct mga_connector, base)
struct mga_crtc {
@@ -110,12 +109,6 @@ struct mga_mode_info {
struct mga_crtc *crtc;
};
-struct mga_encoder {
- struct drm_encoder base;
- int last_dpms;
-};
-
-
struct mga_i2c_chan {
struct i2c_adapter adapter;
struct drm_device *dev;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 62a8e9ccb16d..957ea1057b6c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include "mgag200_drv.h"
@@ -1449,72 +1450,16 @@ static void mga_crtc_init(struct mga_device *mdev)
drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs);
}
-/*
- * The encoder comes after the CRTC in the output pipeline, but before
- * the connector. It's responsible for ensuring that the digital
- * stream is appropriately converted into the output format. Setup is
- * very simple in this case - all we have to do is inform qemu of the
- * colour depth in order to ensure that it displays appropriately
- */
-
-/*
- * These functions are analagous to those in the CRTC code, but are intended
- * to handle any encoder-specific limitations
- */
-static void mga_encoder_mode_set(struct drm_encoder *encoder,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
-{
-
-}
-
-static void mga_encoder_dpms(struct drm_encoder *encoder, int state)
-{
- return;
-}
-
-static void mga_encoder_prepare(struct drm_encoder *encoder)
-{
-}
-
-static void mga_encoder_commit(struct drm_encoder *encoder)
-{
-}
-
-static void mga_encoder_destroy(struct drm_encoder *encoder)
-{
- struct mga_encoder *mga_encoder = to_mga_encoder(encoder);
- drm_encoder_cleanup(encoder);
- kfree(mga_encoder);
-}
-
-static const struct drm_encoder_helper_funcs mga_encoder_helper_funcs = {
- .dpms = mga_encoder_dpms,
- .mode_set = mga_encoder_mode_set,
- .prepare = mga_encoder_prepare,
- .commit = mga_encoder_commit,
-};
-
-static const struct drm_encoder_funcs mga_encoder_encoder_funcs = {
- .destroy = mga_encoder_destroy,
-};
-
static struct drm_encoder *mga_encoder_init(struct drm_device *dev)
{
struct drm_encoder *encoder;
- struct mga_encoder *mga_encoder;
- mga_encoder = kzalloc(sizeof(struct mga_encoder), GFP_KERNEL);
- if (!mga_encoder)
+ encoder = drm_simple_encoder_create(dev, DRM_MODE_ENCODER_DAC);
+ if (IS_ERR(encoder))
return NULL;
- encoder = &mga_encoder->base;
encoder->possible_crtcs = 0x1;
- drm_encoder_init(dev, encoder, &mga_encoder_encoder_funcs,
-DRM_MODE_ENCODER_DAC, NULL);
- drm_encoder_helper_add(encoder, &mga_encoder_helper_funcs);
-
return encoder;
}
--
2.25.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-blk: fix hw_queue stopped on arbitrary error
On Tue, 18 Feb 2020 10:21:18 +0800 Ming Lei wrote: > On Thu, Feb 13, 2020 at 8:38 PM Halil Pasic wrote: > > > > Since nobody else is going to restart our hw_queue for us, the > > blk_mq_start_stopped_hw_queues() is in virtblk_done() is not sufficient > > necessarily sufficient to ensure that the queue will get started again. > > In case of global resource outage (-ENOMEM because mapping failure, > > because of swiotlb full) our virtqueue may be empty and we can get > > stuck with a stopped hw_queue. > > > > Let us not stop the queue on arbitrary errors, but only on -EONSPC which > > indicates a full virtqueue, where the hw_queue is guaranteed to get > > started by virtblk_done() before when it makes sense to carry on > > submitting requests. Let us also remove a stale comment. > > The generic solution may be to stop queue only when there is any > in-flight request > not completed. > I think this is a pretty close to that. The queue is stopped only on ENOSPC, which means virtqueue is full. > Checking -ENOMEM may not be enough, given -EIO can be returned from > virtqueue_add() > too in case of dma map failure. I'm not checking on -ENOMEM. So the queue would not be stopped on EIO. Maybe I'm misunderstanding something In any case, please have another look at the diff, and if your concerns persist please help me understand. Thanks for having a look! Regards, Halil > > Thanks, ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: introduce vDPA based backend
On Fri, Jan 31, 2020 at 11:36:51AM +0800, Tiwei Bie wrote:
> +static int vhost_vdpa_alloc_minor(struct vhost_vdpa *v)
> +{
> + return idr_alloc(&vhost_vdpa.idr, v, 0, MINORMASK + 1,
> + GFP_KERNEL);
> +}
Please don't use idr in new code, use xarray directly
> +static int vhost_vdpa_probe(struct device *dev)
> +{
> + struct vdpa_device *vdpa = dev_to_vdpa(dev);
> + const struct vdpa_config_ops *ops = vdpa->config;
> + struct vhost_vdpa *v;
> + struct device *d;
> + int minor, nvqs;
> + int r;
> +
> + /* Currently, we only accept the network devices. */
> + if (ops->get_device_id(vdpa) != VIRTIO_ID_NET) {
> + r = -ENOTSUPP;
> + goto err;
> + }
> +
> + v = kzalloc(sizeof(*v), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> + if (!v) {
> + r = -ENOMEM;
> + goto err;
> + }
> +
> + nvqs = VHOST_VDPA_VQ_MAX;
> +
> + v->vqs = kmalloc_array(nvqs, sizeof(struct vhost_virtqueue),
> +GFP_KERNEL);
> + if (!v->vqs) {
> + r = -ENOMEM;
> + goto err_alloc_vqs;
> + }
> +
> + mutex_init(&v->mutex);
> + atomic_set(&v->opened, 0);
> +
> + v->vdpa = vdpa;
> + v->nvqs = nvqs;
> + v->virtio_id = ops->get_device_id(vdpa);
> +
> + mutex_lock(&vhost_vdpa.mutex);
> +
> + minor = vhost_vdpa_alloc_minor(v);
> + if (minor < 0) {
> + r = minor;
> + goto err_alloc_minor;
> + }
> +
> + d = device_create(vhost_vdpa.class, NULL,
> + MKDEV(MAJOR(vhost_vdpa.devt), minor),
> + v, "%d", vdpa->index);
> + if (IS_ERR(d)) {
> + r = PTR_ERR(d);
> + goto err_device_create;
> + }
> +
I can't understand what this messing around with major/minor numbers
does. Without allocating a cdev via cdev_add/etc there is only a
single char dev in existence here. This and the stuff in
vhost_vdpa_open() looks non-functional.
> +static void vhost_vdpa_remove(struct device *dev)
> +{
> + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> + struct vhost_vdpa *v = dev_get_drvdata(dev);
> + int opened;
> +
> + add_wait_queue(&vhost_vdpa.release_q, &wait);
> +
> + do {
> + opened = atomic_cmpxchg(&v->opened, 0, 1);
> + if (!opened)
> + break;
> + wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
> + } while (1);
> +
> + remove_wait_queue(&vhost_vdpa.release_q, &wait);
*barf* use the normal refcount pattern please
read side:
refcount_inc_not_zero(uses)
//stuff
if (refcount_dec_and_test(uses))
complete(completer)
destroy side:
if (refcount_dec_and_test(uses))
complete(completer)
wait_for_completion(completer)
// refcount now permanently == 0
Use a completion in driver code
> + mutex_lock(&vhost_vdpa.mutex);
> + device_destroy(vhost_vdpa.class,
> +MKDEV(MAJOR(vhost_vdpa.devt), v->minor));
> + vhost_vdpa_free_minor(v->minor);
> + mutex_unlock(&vhost_vdpa.mutex);
> + kfree(v->vqs);
> + kfree(v);
This use after-fress vs vhost_vdpa_open prior to it setting the open
bit. Maybe use xarray, rcu and kfree_rcu ..
> +static int __init vhost_vdpa_init(void)
> +{
> + int r;
> +
> + idr_init(&vhost_vdpa.idr);
> + mutex_init(&vhost_vdpa.mutex);
> + init_waitqueue_head(&vhost_vdpa.release_q);
> +
> + /* /dev/vhost-vdpa/$vdpa_device_index */
> + vhost_vdpa.class = class_create(THIS_MODULE, "vhost-vdpa");
> + if (IS_ERR(vhost_vdpa.class)) {
> + r = PTR_ERR(vhost_vdpa.class);
> + goto err_class;
> + }
> +
> + vhost_vdpa.class->devnode = vhost_vdpa_devnode;
> +
> + r = alloc_chrdev_region(&vhost_vdpa.devt, 0, MINORMASK + 1,
> + "vhost-vdpa");
> + if (r)
> + goto err_alloc_chrdev;
> +
> + cdev_init(&vhost_vdpa.cdev, &vhost_vdpa_fops);
> + r = cdev_add(&vhost_vdpa.cdev, vhost_vdpa.devt, MINORMASK + 1);
> + if (r)
> + goto err_cdev_add;
It is very strange, is the intention to create a single global char
dev?
If so, why is there this:
+static int vhost_vdpa_open(struct inode *inode, struct file *filep)
+{
+ struct vhost_vdpa *v;
+ struct vhost_dev *dev;
+ struct vhost_virtqueue **vqs;
+ int nvqs, i, r, opened;
+
+ v = vhost_vdpa_get_from_minor(iminor(inode));
?
If the idea is to create a per-vdpa char dev then this stuff belongs
in vhost_vdpa_probe(), the cdev should be part of the vhost_vdpa, and
the above should be container_of not an idr lookup.
Jason
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 3/5] vDPA: introduce vDPA bus
On Mon, Feb 17, 2020 at 02:08:03PM +0800, Jason Wang wrote: > I thought you were copied in the patch [1], maybe we can move vhost related > discussion there to avoid confusion. > > [1] https://lwn.net/Articles/811210/ Wow, that is .. confusing. So this is supposed to duplicate the uAPI of vhost-user? But it is open coded and duplicated because .. vdpa? > So it's cheaper and simpler to introduce a new bus instead of refactoring a > well known bus and API where brunches of drivers and devices had been > implemented for years. If you reason for this approach is to ease the implementation then you should talk about it in the cover letters/etc Maybe it is reasonable to do this because the rework is too great, I don't know, but to me this whole thing looks rather messy. Remember this stuff is all uAPI as it shows up in sysfs, so you can easilly get stuck with it forever. Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] x86/ioperm: add new paravirt function update_io_bitmap
Commit 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control
ioperm() as well") reworked the iopl syscall to use I/O bitmaps.
Unfortunately this broke Xen PV domains using that syscall as there
is currently no I/O bitmap support in PV domains.
Add I/O bitmap support via a new paravirt function update_io_bitmap
which Xen PV domains can use to update their I/O bitmaps via a
hypercall.
Fixes: 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control ioperm() as
well")
Reported-by: Jan Beulich
Cc: # 5.5
Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
Tested-by: Jan Beulich
---
arch/x86/include/asm/io_bitmap.h | 9 -
arch/x86/include/asm/paravirt.h | 7 +++
arch/x86/include/asm/paravirt_types.h | 4
arch/x86/kernel/paravirt.c| 5 +
arch/x86/kernel/process.c | 2 +-
arch/x86/xen/enlighten_pv.c | 25 +
6 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/io_bitmap.h b/arch/x86/include/asm/io_bitmap.h
index 02c6ef8f7667..07344d82e88e 100644
--- a/arch/x86/include/asm/io_bitmap.h
+++ b/arch/x86/include/asm/io_bitmap.h
@@ -19,7 +19,14 @@ struct task_struct;
void io_bitmap_share(struct task_struct *tsk);
void io_bitmap_exit(void);
-void tss_update_io_bitmap(void);
+void native_tss_update_io_bitmap(void);
+
+#ifdef CONFIG_PARAVIRT_XXL
+#include
+#else
+#define tss_update_io_bitmap native_tss_update_io_bitmap
+#endif
+
#else
static inline void io_bitmap_share(struct task_struct *tsk) { }
static inline void io_bitmap_exit(void) { }
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 86e7317eb31f..694d8daf4983 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -295,6 +295,13 @@ static inline void write_idt_entry(gate_desc *dt, int
entry, const gate_desc *g)
PVOP_VCALL3(cpu.write_idt_entry, dt, entry, g);
}
+#ifdef CONFIG_X86_IOPL_IOPERM
+static inline void tss_update_io_bitmap(void)
+{
+ PVOP_VCALL0(cpu.update_io_bitmap);
+}
+#endif
+
static inline void paravirt_activate_mm(struct mm_struct *prev,
struct mm_struct *next)
{
diff --git a/arch/x86/include/asm/paravirt_types.h
b/arch/x86/include/asm/paravirt_types.h
index 84812964d3dd..732f62e04ddb 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -140,6 +140,10 @@ struct pv_cpu_ops {
void (*load_sp0)(unsigned long sp0);
+#ifdef CONFIG_X86_IOPL_IOPERM
+ void (*update_io_bitmap)(void);
+#endif
+
void (*wbinvd)(void);
/* cpuid emulation, mostly so that caps bits can be disabled */
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index 789f5e4f89de..c131ba4e70ef 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -30,6 +30,7 @@
#include
#include
#include
+#include
/*
* nop stub, which must not clobber anything *including the stack* to
@@ -341,6 +342,10 @@ struct paravirt_patch_template pv_ops = {
.cpu.iret = native_iret,
.cpu.swapgs = native_swapgs,
+#ifdef CONFIG_X86_IOPL_IOPERM
+ .cpu.update_io_bitmap = native_tss_update_io_bitmap,
+#endif
+
.cpu.start_context_switch = paravirt_nop,
.cpu.end_context_switch = paravirt_nop,
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 839b5244e3b7..3053c85e0e42 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -374,7 +374,7 @@ static void tss_copy_io_bitmap(struct tss_struct *tss,
struct io_bitmap *iobm)
/**
* tss_update_io_bitmap - Update I/O bitmap before exiting to usermode
*/
-void tss_update_io_bitmap(void)
+void native_tss_update_io_bitmap(void)
{
struct tss_struct *tss = this_cpu_ptr(&cpu_tss_rw);
struct thread_struct *t = ¤t->thread;
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 1f756e8b..feaf2e68ee5c 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -72,6 +72,9 @@
#include
#include
#include
+#ifdef CONFIG_X86_IOPL_IOPERM
+#include
+#endif
#ifdef CONFIG_ACPI
#include
@@ -837,6 +840,25 @@ static void xen_load_sp0(unsigned long sp0)
this_cpu_write(cpu_tss_rw.x86_tss.sp0, sp0);
}
+#ifdef CONFIG_X86_IOPL_IOPERM
+static void xen_update_io_bitmap(void)
+{
+ struct physdev_set_iobitmap iobitmap;
+ struct tss_struct *tss = this_cpu_ptr(&cpu_tss_rw);
+
+ native_tss_update_io_bitmap();
+
+ iobitmap.bitmap = (uint8_t *)(&tss->x86_tss) +
+ tss->x86_tss.io_bitmap_base;
+ if (tss->x86_tss.io_bitmap_base == IO_BITMAP_OFFSET_INVALID)
+ iobitmap.nr_ports = 0;
+ else
+ iobitmap.nr_ports = IO_BITMAP_BITS;
+
+ HYPERVISOR_physdev_op(PHYSDEVOP_set_iobitmap, &iobitmap);
+}
+#endif
+
static void xen_io_delay(
Re: [PATCH] x86/ioperm: add new paravirt function update_io_bitmap
Juergen Gross writes:
> Commit 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control
> ioperm() as well") reworked the iopl syscall to use I/O bitmaps.
>
> Unfortunately this broke Xen PV domains using that syscall as there
> is currently no I/O bitmap support in PV domains.
>
> Add I/O bitmap support via a new paravirt function update_io_bitmap
> which Xen PV domains can use to update their I/O bitmaps via a
> hypercall.
>
> Fixes: 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control ioperm() as
> well")
> Reported-by: Jan Beulich
> Cc: # 5.5
> Signed-off-by: Juergen Gross
> Reviewed-by: Jan Beulich
> Tested-by: Jan Beulich
Duh, sorry about that and thanks for fixing it.
BTW, why isn't stuff like this not catched during next or at least
before the final release? Is nothing running CI on upstream with all
that XEN muck active?
Thanks,
tglx
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-blk: fix hw_queue stopped on arbitrary error
On Tue, Feb 18, 2020 at 8:35 PM Halil Pasic wrote: > > On Tue, 18 Feb 2020 10:21:18 +0800 > Ming Lei wrote: > > > On Thu, Feb 13, 2020 at 8:38 PM Halil Pasic wrote: > > > > > > Since nobody else is going to restart our hw_queue for us, the > > > blk_mq_start_stopped_hw_queues() is in virtblk_done() is not sufficient > > > necessarily sufficient to ensure that the queue will get started again. > > > In case of global resource outage (-ENOMEM because mapping failure, > > > because of swiotlb full) our virtqueue may be empty and we can get > > > stuck with a stopped hw_queue. > > > > > > Let us not stop the queue on arbitrary errors, but only on -EONSPC which > > > indicates a full virtqueue, where the hw_queue is guaranteed to get > > > started by virtblk_done() before when it makes sense to carry on > > > submitting requests. Let us also remove a stale comment. > > > > The generic solution may be to stop queue only when there is any > > in-flight request > > not completed. > > > > I think this is a pretty close to that. The queue is stopped only on > ENOSPC, which means virtqueue is full. > > > Checking -ENOMEM may not be enough, given -EIO can be returned from > > virtqueue_add() > > too in case of dma map failure. > > I'm not checking on -ENOMEM. So the queue would not be stopped on EIO. > Maybe I'm misunderstanding something In any case, please have another > look at the diff, and if your concerns persist please help me understand. Looks I misread the patch, and this patch is fine: Reviewed-by: Ming Lei Thanks, Ming Lei ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: introduce vDPA based backend
On Tue, Feb 18, 2020 at 09:53:59AM -0400, Jason Gunthorpe wrote:
> On Fri, Jan 31, 2020 at 11:36:51AM +0800, Tiwei Bie wrote:
>
> > +static int vhost_vdpa_alloc_minor(struct vhost_vdpa *v)
> > +{
> > + return idr_alloc(&vhost_vdpa.idr, v, 0, MINORMASK + 1,
> > +GFP_KERNEL);
> > +}
>
> Please don't use idr in new code, use xarray directly
>
> > +static int vhost_vdpa_probe(struct device *dev)
> > +{
> > + struct vdpa_device *vdpa = dev_to_vdpa(dev);
> > + const struct vdpa_config_ops *ops = vdpa->config;
> > + struct vhost_vdpa *v;
> > + struct device *d;
> > + int minor, nvqs;
> > + int r;
> > +
> > + /* Currently, we only accept the network devices. */
> > + if (ops->get_device_id(vdpa) != VIRTIO_ID_NET) {
> > + r = -ENOTSUPP;
> > + goto err;
> > + }
> > +
> > + v = kzalloc(sizeof(*v), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> > + if (!v) {
> > + r = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + nvqs = VHOST_VDPA_VQ_MAX;
> > +
> > + v->vqs = kmalloc_array(nvqs, sizeof(struct vhost_virtqueue),
> > + GFP_KERNEL);
> > + if (!v->vqs) {
> > + r = -ENOMEM;
> > + goto err_alloc_vqs;
> > + }
> > +
> > + mutex_init(&v->mutex);
> > + atomic_set(&v->opened, 0);
> > +
> > + v->vdpa = vdpa;
> > + v->nvqs = nvqs;
> > + v->virtio_id = ops->get_device_id(vdpa);
> > +
> > + mutex_lock(&vhost_vdpa.mutex);
> > +
> > + minor = vhost_vdpa_alloc_minor(v);
> > + if (minor < 0) {
> > + r = minor;
> > + goto err_alloc_minor;
> > + }
> > +
> > + d = device_create(vhost_vdpa.class, NULL,
> > + MKDEV(MAJOR(vhost_vdpa.devt), minor),
> > + v, "%d", vdpa->index);
> > + if (IS_ERR(d)) {
> > + r = PTR_ERR(d);
> > + goto err_device_create;
> > + }
> > +
>
> I can't understand what this messing around with major/minor numbers
> does. Without allocating a cdev via cdev_add/etc there is only a
> single char dev in existence here. This and the stuff in
> vhost_vdpa_open() looks non-functional.
I followed the code in VFIO. Please see more details below.
>
> > +static void vhost_vdpa_remove(struct device *dev)
> > +{
> > + DEFINE_WAIT_FUNC(wait, woken_wake_function);
> > + struct vhost_vdpa *v = dev_get_drvdata(dev);
> > + int opened;
> > +
> > + add_wait_queue(&vhost_vdpa.release_q, &wait);
> > +
> > + do {
> > + opened = atomic_cmpxchg(&v->opened, 0, 1);
> > + if (!opened)
> > + break;
> > + wait_woken(&wait, TASK_UNINTERRUPTIBLE, HZ * 10);
> > + } while (1);
> > +
> > + remove_wait_queue(&vhost_vdpa.release_q, &wait);
>
> *barf* use the normal refcount pattern please
>
> read side:
>
> refcount_inc_not_zero(uses)
> //stuff
> if (refcount_dec_and_test(uses))
> complete(completer)
>
> destroy side:
> if (refcount_dec_and_test(uses))
> complete(completer)
> wait_for_completion(completer)
> // refcount now permanently == 0
>
> Use a completion in driver code
>
> > + mutex_lock(&vhost_vdpa.mutex);
> > + device_destroy(vhost_vdpa.class,
> > + MKDEV(MAJOR(vhost_vdpa.devt), v->minor));
> > + vhost_vdpa_free_minor(v->minor);
> > + mutex_unlock(&vhost_vdpa.mutex);
> > + kfree(v->vqs);
> > + kfree(v);
>
> This use after-fress vs vhost_vdpa_open prior to it setting the open
> bit. Maybe use xarray, rcu and kfree_rcu ..
>
> > +static int __init vhost_vdpa_init(void)
> > +{
> > + int r;
> > +
> > + idr_init(&vhost_vdpa.idr);
> > + mutex_init(&vhost_vdpa.mutex);
> > + init_waitqueue_head(&vhost_vdpa.release_q);
> > +
> > + /* /dev/vhost-vdpa/$vdpa_device_index */
> > + vhost_vdpa.class = class_create(THIS_MODULE, "vhost-vdpa");
> > + if (IS_ERR(vhost_vdpa.class)) {
> > + r = PTR_ERR(vhost_vdpa.class);
> > + goto err_class;
> > + }
> > +
> > + vhost_vdpa.class->devnode = vhost_vdpa_devnode;
> > +
> > + r = alloc_chrdev_region(&vhost_vdpa.devt, 0, MINORMASK + 1,
> > + "vhost-vdpa");
> > + if (r)
> > + goto err_alloc_chrdev;
> > +
> > + cdev_init(&vhost_vdpa.cdev, &vhost_vdpa_fops);
> > + r = cdev_add(&vhost_vdpa.cdev, vhost_vdpa.devt, MINORMASK + 1);
> > + if (r)
> > + goto err_cdev_add;
>
> It is very strange, is the intention to create a single global char
> dev?
No. It's to create a per-vdpa char dev named
vhost-vdpa/$vdpa_device_index in dev.
I followed the code in VFIO which creates char dev
vfio/$GROUP dynamically, e.g.:
https://github.com/torvalds/linux/blob/b1da3acc781c/drivers/vfio/vfio.c#L2164-L2180
https://github.com/torvalds/linux/blob/b1da3acc781c/drivers/vfio/vfio.c#L373-L387
https://github.com/torvalds/linux/blob/b1da3acc781c/drivers/vfio/vfio.c#L1553
Is it something unwanted?
Thanks for the review.
Regards,
Tiwei
>
> If so, why is there this:
>
> +static int vhost_vdpa_open(struct i
Re: [PATCH V2 3/5] vDPA: introduce vDPA bus
On Tue, Feb 18, 2020 at 01:56:12PM +, Jason Gunthorpe wrote: > On Mon, Feb 17, 2020 at 02:08:03PM +0800, Jason Wang wrote: > > > I thought you were copied in the patch [1], maybe we can move vhost related > > discussion there to avoid confusion. > > > > [1] https://lwn.net/Articles/811210/ > > Wow, that is .. confusing. > > So this is supposed to duplicate the uAPI of vhost-user? But it is > open coded and duplicated because .. vdpa? Do you mean the vhost-user in DPDK? There is no vhost-user in Linux kernel. Thanks, Tiwei > > > So it's cheaper and simpler to introduce a new bus instead of refactoring a > > well known bus and API where brunches of drivers and devices had been > > implemented for years. > > If you reason for this approach is to ease the implementation then you > should talk about it in the cover letters/etc > > Maybe it is reasonable to do this because the rework is too great, I > don't know, but to me this whole thing looks rather messy. > > Remember this stuff is all uAPI as it shows up in sysfs, so you can > easilly get stuck with it forever. > > Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] x86/ioperm: add new paravirt function update_io_bitmap
On 18.02.20 22:03, Thomas Gleixner wrote:
Juergen Gross writes:
Commit 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control
ioperm() as well") reworked the iopl syscall to use I/O bitmaps.
Unfortunately this broke Xen PV domains using that syscall as there
is currently no I/O bitmap support in PV domains.
Add I/O bitmap support via a new paravirt function update_io_bitmap
which Xen PV domains can use to update their I/O bitmaps via a
hypercall.
Fixes: 111e7b15cf10f6 ("x86/ioperm: Extend IOPL config to control ioperm() as
well")
Reported-by: Jan Beulich
Cc: # 5.5
Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
Tested-by: Jan Beulich
Duh, sorry about that and thanks for fixing it.
BTW, why isn't stuff like this not catched during next or at least
before the final release? Is nothing running CI on upstream with all
that XEN muck active?
This problem showed up by not being able to start the X server (probably
not the freshest one) in dom0 on a moderate aged AMD system.
Our CI tests tend do be more text console based for dom0.
Juergen
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V2 3/5] vDPA: introduce vDPA bus
On 2020/2/18 下午9:56, Jason Gunthorpe wrote: On Mon, Feb 17, 2020 at 02:08:03PM +0800, Jason Wang wrote: I thought you were copied in the patch [1], maybe we can move vhost related discussion there to avoid confusion. [1] https://lwn.net/Articles/811210/ Wow, that is .. confusing. So this is supposed to duplicate the uAPI of vhost-user? It tries to reuse the uAPI of vhost with some extension. But it is open coded and duplicated because .. vdpa? I'm not sure I get here, vhost module is reused for vhost-vdpa and all current vhost device (e.g net) uses their own char device. So it's cheaper and simpler to introduce a new bus instead of refactoring a well known bus and API where brunches of drivers and devices had been implemented for years. If you reason for this approach is to ease the implementation then you should talk about it in the cover letters/etc I will add more rationale in both cover letter and this patch. Thanks Maybe it is reasonable to do this because the rework is too great, I don't know, but to me this whole thing looks rather messy. Remember this stuff is all uAPI as it shows up in sysfs, so you can easilly get stuck with it forever. Jason ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
