| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d1a1b65e-75b4-2e04-2484-cd0a305cef7e@redhat.com>
Date: Fri, 25 Oct 2019 09:54:20 +0800
From: Jason Wang <jasowang@...hat.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: kvm@...r.kernel.org, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
intel-gfx@...ts.freedesktop.org,
intel-gvt-dev@...ts.freedesktop.org, kwankhede@...dia.com,
mst@...hat.com, tiwei.bie@...el.com,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
cohuck@...hat.com, maxime.coquelin@...hat.com,
cunming.liang@...el.com, zhihong.wang@...el.com,
rob.miller@...adcom.com, xiao.w.wang@...el.com,
haotian.wang@...ive.com, zhenyuw@...ux.intel.com,
zhi.a.wang@...el.com, jani.nikula@...ux.intel.com,
joonas.lahtinen@...ux.intel.com, rodrigo.vivi@...el.com,
airlied@...ux.ie, daniel@...ll.ch, farman@...ux.ibm.com,
pasic@...ux.ibm.com, sebott@...ux.ibm.com, oberpar@...ux.ibm.com,
heiko.carstens@...ibm.com, gor@...ux.ibm.com,
borntraeger@...ibm.com, akrowiak@...ux.ibm.com,
freude@...ux.ibm.com, lingshan.zhu@...el.com, idos@...lanox.com,
eperezma@...hat.com, lulu@...hat.com, parav@...lanox.com,
christophe.de.dinechin@...il.com, kevin.tian@...el.com,
stefanha@...hat.com
Subject: Re: [PATCH V5 4/6] mdev: introduce virtio device and its device ops
On 2019/10/25 上午4:44, Alex Williamson wrote:
> On Thu, 24 Oct 2019 11:51:35 +0800
> Jason Wang<jasowang@...hat.com> wrote:
>
>> On 2019/10/24 上午5:57, Alex Williamson wrote:
>>> On Wed, 23 Oct 2019 21:07:50 +0800
>>> Jason Wang<jasowang@...hat.com> wrote:
>>>
>>>> This patch implements basic support for mdev driver that supports
>>>> virtio transport for kernel virtio driver.
>>>>
>>>> Signed-off-by: Jason Wang<jasowang@...hat.com>
>>>> ---
>>>> drivers/vfio/mdev/mdev_core.c | 20 ++++
>>>> drivers/vfio/mdev/mdev_private.h | 2 +
>>>> include/linux/mdev.h | 6 ++
>>>> include/linux/virtio_mdev_ops.h | 159 +++++++++++++++++++++++++++++++
>>>> 4 files changed, 187 insertions(+)
>>>> create mode 100644 include/linux/virtio_mdev_ops.h
>>>>
>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>>> index 555bd61d8c38..9b00c3513120 100644
>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>> @@ -76,6 +76,26 @@ const struct vfio_mdev_device_ops *mdev_get_vfio_ops(struct mdev_device *mdev)
>>>> }
>>>> EXPORT_SYMBOL(mdev_get_vfio_ops);
>>>>
>>>> +/* Specify the virtio device ops for the mdev device, this
>>>> + * must be called during create() callback for virtio mdev device.
>>>> + */
>>>> +void mdev_set_virtio_ops(struct mdev_device *mdev,
>>>> + const struct virtio_mdev_device_ops *virtio_ops)
>>>> +{
>>>> + mdev_set_class(mdev, MDEV_CLASS_ID_VIRTIO);
>>>> + mdev->virtio_ops = virtio_ops;
>>>> +}
>>>> +EXPORT_SYMBOL(mdev_set_virtio_ops);
>>>> +
>>>> +/* Get the virtio device ops for the mdev device. */
>>>> +const struct virtio_mdev_device_ops *
>>>> +mdev_get_virtio_ops(struct mdev_device *mdev)
>>>> +{
>>>> + WARN_ON(mdev->class_id != MDEV_CLASS_ID_VIRTIO);
>>>> + return mdev->virtio_ops;
>>>> +}
>>>> +EXPORT_SYMBOL(mdev_get_virtio_ops);
>>>> +
>>>> struct device *mdev_dev(struct mdev_device *mdev)
>>>> {
>>>> return &mdev->dev;
>>>> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
>>>> index 0770410ded2a..7b47890c34e7 100644
>>>> --- a/drivers/vfio/mdev/mdev_private.h
>>>> +++ b/drivers/vfio/mdev/mdev_private.h
>>>> @@ -11,6 +11,7 @@
>>>> #define MDEV_PRIVATE_H
>>>>
>>>> #include <linux/vfio_mdev_ops.h>
>>>> +#include <linux/virtio_mdev_ops.h>
>>>>
>>>> int mdev_bus_register(void);
>>>> void mdev_bus_unregister(void);
>>>> @@ -38,6 +39,7 @@ struct mdev_device {
>>>> u16 class_id;
>>>> union {
>>>> const struct vfio_mdev_device_ops *vfio_ops;
>>>> + const struct virtio_mdev_device_ops *virtio_ops;
>>>> };
>>>> };
>>>>
>>>> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
>>>> index 4625f1a11014..9b69b0bbebfd 100644
>>>> --- a/include/linux/mdev.h
>>>> +++ b/include/linux/mdev.h
>>>> @@ -17,6 +17,7 @@
>>>>
>>>> struct mdev_device;
>>>> struct vfio_mdev_device_ops;
>>>> +struct virtio_mdev_device_ops;
>>>>
>>>> /*
>>>> * Called by the parent device driver to set the device which represents
>>>> @@ -112,6 +113,10 @@ void mdev_set_class(struct mdev_device *mdev, u16 id);
>>>> void mdev_set_vfio_ops(struct mdev_device *mdev,
>>>> const struct vfio_mdev_device_ops *vfio_ops);
>>>> const struct vfio_mdev_device_ops *mdev_get_vfio_ops(struct mdev_device *mdev);
>>>> +void mdev_set_virtio_ops(struct mdev_device *mdev,
>>>> + const struct virtio_mdev_device_ops *virtio_ops);
>>>> +const struct virtio_mdev_device_ops *
>>>> +mdev_get_virtio_ops(struct mdev_device *mdev);
>>>>
>>>> extern struct bus_type mdev_bus_type;
>>>>
>>>> @@ -127,6 +132,7 @@ struct mdev_device *mdev_from_dev(struct device *dev);
>>>>
>>>> enum {
>>>> MDEV_CLASS_ID_VFIO = 1,
>>>> + MDEV_CLASS_ID_VIRTIO = 2,
>>>> /* New entries must be added here */
>>>> };
>>>>
>>>> diff --git a/include/linux/virtio_mdev_ops.h b/include/linux/virtio_mdev_ops.h
>>>> new file mode 100644
>>>> index 000000000000..d417b41f2845
>>>> --- /dev/null
>>>> +++ b/include/linux/virtio_mdev_ops.h
>>>> @@ -0,0 +1,159 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> +/*
>>>> + * Virtio mediated device driver
>>>> + *
>>>> + * Copyright 2019, Red Hat Corp.
>>>> + * Author: Jason Wang<jasowang@...hat.com>
>>>> + */
>>>> +#ifndef _LINUX_VIRTIO_MDEV_H
>>>> +#define _LINUX_VIRTIO_MDEV_H
>>>> +
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/mdev.h>
>>>> +#include <uapi/linux/vhost.h>
>>>> +
>>>> +#define VIRTIO_MDEV_DEVICE_API_STRING "virtio-mdev"
>>>> +#define VIRTIO_MDEV_F_VERSION_1 0x1
>>>> +
>>>> +struct virtio_mdev_callback {
>>>> + irqreturn_t (*callback)(void *data);
>>>> + void *private;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct vfio_mdev_device_ops - Structure to be registered for each
>>>> + * mdev device to register the device for virtio/vhost drivers.
>>>> + *
>>>> + * The device ops that is supported by VIRTIO_MDEV_F_VERSION_1, the
>>>> + * callbacks are mandatory unless explicity mentioned.
>>> If the version of the callbacks is returned by a callback within the
>>> structure defined by the version... isn't that a bit circular? This
>>> seems redundant to me versus the class id. The fact that the parent
>>> driver defines the device as MDEV_CLASS_ID_VIRTIO should tell us this
>>> already. If it was incremented, we'd need an MDEV_CLASS_ID_VIRTIOv2,
>>> which the virtio-mdev bus driver could add to its id table and handle
>>> differently.
>> My understanding is versions are only allowed to increase monotonically,
>> this seems less flexible than features. E.g we have features A, B, C,
>> mdev device can choose to support only a subset. E.g when mdev device
>> can support dirty page logging, it can add a new feature bit for driver
>> to know that it support new device ops. MDEV_CLASS_ID_VIRTIOv2 may only
>> be useful when we will invent a complete new API.
> But this interface rather conflates features and versions by returning
> a version as a feature. If we simply want to say that there are no
> additional features, then get_mdev_features() should return an empty
> set. If dirty page logging is a feature, then I'd expect a bit in the
> get_mdev_features() return value to identify that feature.
>
> However, I've been under the impression (perhaps wrongly) that the
> class-id has a 1:1 correlation to the device-ops exposed to the bus
> driver, so if dirty page logging requires extra callbacks, that would
> imply a new device-ops, which requires a new class-id. In that case
> virtio-mdev would claim both class-ids and would need some way to
> differentiate them. But I also see that such a solution can become
> unmanageable as the set of class-ids would need to encompass every
> combination of features.
>
> So I think what's suggested by this is that the initial struct
> virtio_mdev_device_ops is a base set of callbacks which would be
> extended via features?
Yes.
> But then why does get_generation() not make use
> of this? And if we can define get_generation() as optional and simply
> test if it's implemented, then it seems we don't need any feature bits
> to extend the structure (unless we're looking at binary compatibility).
I think the mapping between ops and features is N:1. We start with a
base feature like VIRTIO_MDEV_VERSION_1, this allows us to do simply
checking during probe without the need to checking the existence of each
op. Testing the existence of a specific op should work but less
convenient when a feature requires several new ops. For the case of
get_generation(), it works because the bus driver have workaround when
parent doesn't provide that. This may not work for a real feature.
> So maybe get_mdev_features() is meant to expose underlying features
> unrelated to the callbacks? Which is not in line with the description?
At least not for current virtio-mdev/vhost-mdev. Virtio specific
features should be done via get_features().
> Hopefully you can see my confusion in what we're trying to do here.
I think I get you and hope my answer make sense. Suggestions are welcomed.
Thanks
> Thanks,
>
> Alex
>
Powered by blists - more mailing lists