Re: [Intel-gfx] [PATCH v2 7/7] vfio: Remove vfio_free_device
On Wed, Nov 02 2022, Eric Farman wrote:
> With the "mess" sorted out, we should be able to inline the
> vfio_free_device call introduced by commit cb9ff3f3b84c
> ("vfio: Add helpers for unifying vfio_device life cycle")
> and remove them from driver release callbacks.
>
> Signed-off-by: Eric Farman
> Reviewed-by: Jason Gunthorpe
> Reviewed-by: Kevin Tian
> ---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 -
> drivers/s390/cio/vfio_ccw_ops.c | 2 --
> drivers/s390/crypto/vfio_ap_ops.c | 6 --
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 1 -
> drivers/vfio/pci/vfio_pci_core.c | 1 -
> drivers/vfio/platform/vfio_amba.c | 1 -
> drivers/vfio/platform/vfio_platform.c | 1 -
> drivers/vfio/vfio_main.c | 22 --
> include/linux/vfio.h | 1 -
> samples/vfio-mdev/mbochs.c| 1 -
> samples/vfio-mdev/mdpy.c | 1 -
> samples/vfio-mdev/mtty.c | 1 -
> 12 files changed, 4 insertions(+), 35 deletions(-)
Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v3 1/2] vfio: Replace the DMA unmapping notifier with a callback
On Tue, Jul 19 2022, Jason Gunthorpe wrote:
> On Thu, Jul 07, 2022 at 03:37:16PM -0600, Alex Williamson wrote:
>> On Mon, 4 Jul 2022 21:59:03 -0300
>> Jason Gunthorpe wrote:
>> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
>> > b/drivers/s390/cio/vfio_ccw_ops.c
>> > index b49e2e9db2dc6f..09e0ce7b72324c 100644
>> > --- a/drivers/s390/cio/vfio_ccw_ops.c
>> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
>> > @@ -44,31 +44,19 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private
>> > *private)
>> >return ret;
>> > }
>> >
>> > -static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
>> > -unsigned long action,
>> > -void *data)
>> > +static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64
>> > length)
>> > {
>> >struct vfio_ccw_private *private =
>> > - container_of(nb, struct vfio_ccw_private, nb);
>> > -
>> > - /*
>> > - * Vendor drivers MUST unpin pages in response to an
>> > - * invalidation.
>> > - */
>> > - if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) {
>> > - struct vfio_iommu_type1_dma_unmap *unmap = data;
>> > -
>> > - if (!cp_iova_pinned(>cp, unmap->iova))
>> > - return NOTIFY_OK;
>> > + container_of(vdev, struct vfio_ccw_private, vdev);
>> >
>> > - if (vfio_ccw_mdev_reset(private))
>> > - return NOTIFY_BAD;
>> > + /* Drivers MUST unpin pages in response to an invalidation. */
>> > + if (!cp_iova_pinned(>cp, iova))
>> > + return;
>> >
>> > - cp_free(>cp);
>> > - return NOTIFY_OK;
>> > - }
>> > + if (vfio_ccw_mdev_reset(private))
>> > + return;
>> >
>> > - return NOTIFY_DONE;
>> > + cp_free(>cp);
>> > }
>>
>>
>> The cp_free() call is gone here with [1], so I think this function now
>> just ends with:
>>
>> ...
>> vfio_ccw_mdev_reset(private);
>> }
>>
>> There are also minor contextual differences elsewhere from that series,
>> so a quick respin to record the changes on list would be appreciated.
>>
>> However the above kind of highlights that NOTIFY_BAD that silently gets
>> dropped here. I realize we weren't testing the return value of the
>> notifier call chain and really we didn't intend that notifiers could
>> return a failure here, but does this warrant some logging or suggest
>> future work to allow a device to go offline here? Thanks.
>
> It looks like no.
>
> If the FSM trapped in a bad state here, such as
> VFIO_CCW_STATE_NOT_OPER, then it means it should have already unpinned
> the pages and this is considered a success for this purpose
A rather pathological case would be a subchannel that cannot be
quiesced and does not end up being non-operational; in theory, the
hardware could still try to access the buffers we provided for I/O. I'd
say that is extremely unlikely, we might log it, but really cannot do
anything else.
>
> The return code here exists only to return to userspace so it can
> detect during a VFIO_DEVICE_RESET that the device has crashed
> irrecoverably.
Does it imply only that ("it's dead, Jim"), or can it also imply a
runaway device? Not that userspace can do much in any case.
>
> Thus just continuing to silently ignore it seems like the best thing.
>
> Jason
Re: [Intel-gfx] [PATCH v2 6/9] vfio/mdev: Add mdev available instance checking to the core
On Thu, Sep 09 2021, Jason Gunthorpe wrote: > Many of the mdev drivers use a simple counter for keeping track of the > available instances. Move this code to the core code and store the counter > in the mdev_type. Implement it using correct locking, fixing mdpy. > > Drivers provide a get_available() callback to set the number of available > instances for their mtypes which is fixed at registration time. The core > provides a standard sysfs attribute to return the available_instances. So, according to the documentation, available_instances is mandatory. This means that drivers either need to provide get_available or implement their own version of the attribute. I think we want to update vfio-mediated-device.rst as well? > > Signed-off-by: Jason Gunthorpe > --- > drivers/s390/cio/vfio_ccw_drv.c | 1 - > drivers/s390/cio/vfio_ccw_ops.c | 26 ++- > drivers/s390/cio/vfio_ccw_private.h | 2 -- > drivers/s390/crypto/vfio_ap_ops.c | 32 ++- > drivers/s390/crypto/vfio_ap_private.h | 2 -- > drivers/vfio/mdev/mdev_core.c | 11 +++- > drivers/vfio/mdev/mdev_private.h | 2 ++ > drivers/vfio/mdev/mdev_sysfs.c| 37 +++ > include/linux/mdev.h | 2 ++ > samples/vfio-mdev/mdpy.c | 22 +--- > 10 files changed, 73 insertions(+), 64 deletions(-) Otherwise, looks good.
Re: [Intel-gfx] [PATCH v2 4/9] vfio/ccw: Make the FSM complete and synchronize it to the mdev
On Thu, Sep 09 2021, Jason Gunthorpe wrote: > The subchannel should be left in a quiescent state unless the VFIO device > FD is opened. When the FD is opened bring the chanel to active and allow > the VFIO device to operate. When the device FD is closed then quiesce the > channel. > > To make this work the FSM needs to handle the transitions to/from open and > closed so everything is sequenced. Rename state NOT_OPER to BROKEN and use > it wheneven the driver has malfunctioned. STANDBY becomes CLOSED. The > normal case FSM looks like: > CLOSED -> IDLE -> PROCESS/PENDING* -> IDLE -> CLOSED > > With a possible branch off to BROKEN from any state. Once the device is in > BROKEN it cannot be recovered other than be reloading the driver. Hm, not sure whether it is a good idea to conflate "something went wrong" and "device is not operational". In the latter case, we will eventually get a removal of the css device when the common I/O layer has processed the channel report for the subchannel; while the former case could mean all kind of things, but the subchannel will likely stay around. I think NOT_OPER was always meant to be a transitional state. > > Delete the triply redundant calls to > vfio_ccw_sch_quiesce(). vfio_ccw_mdev_close_device() always leaves the > subchannel quiescent. vfio_ccw_mdev_remove() cannot return until > vfio_ccw_mdev_close_device() completes and vfio_ccw_sch_remove() cannot > return until vfio_ccw_mdev_remove() completes. Have the FSM code take care > of calling cp_free() when appropriate. I remember some serialization issues wrt cp_free() etc. coming up every now and than; that might need extra care (I'm taking a look.) > > Device reset becomes a CLOSE/OPEN sequence which now properly handles the > situation if the device becomes BROKEN. > > Machine shutdown via vfio_ccw_sch_shutdown() now simply tries to close and > leaves the device BROKEN (though arguably the bus should take care to > quiet down the subchannel HW during shutdown, not the drivers) The problem is that there is not really a uniform thing that can be done for shutdown; e.g. we can quiesce and then disable I/O and EADM subchannels, but CHSC subchannels cannot be quiesced.
Re: [Intel-gfx] [PATCH v2 2/9] vfio/ccw: Pass vfio_ccw_private not mdev_device to various functions
On Thu, Sep 09 2021, Jason Gunthorpe wrote: > mdev_device should only be used in functions assigned to ops callbacks, > interior functions should use the struct vfio_ccw_private instead of > repeatedly trying to get it from the mdev. > > Signed-off-by: Jason Gunthorpe > --- > drivers/s390/cio/vfio_ccw_ops.c | 37 + > 1 file changed, 15 insertions(+), 22 deletions(-) Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v2 0/9] Move vfio_ccw to the new mdev API
On Fri, Sep 17 2021, Jason Gunthorpe wrote:
> On Fri, Sep 17, 2021 at 01:59:16PM +0200, Cornelia Huck wrote:
>> >ret = cio_cancel_halt_clear(sch, );
>> > -
>> >if (ret == -EIO) {
>> >pr_err("vfio_ccw: could not quiesce subchannel
>> > 0.%x.%04x!\n",
>> > sch->schid.ssid, sch->schid.sch_no);
>> > - break;
>> > + return ret;
>>
>> Looking at this, I wonder why we had special-cased -EIO -- for -ENODEV
>> we should be done as well, as then the device is dead and we do not need
>> to disable it.
>
> cio_cancel_halt_clear() should probably succeed in that case.
It will actually give us -ENODEV, as the very first call in that
function will already fail.
>
>> > @@ -413,13 +403,28 @@ static void fsm_close(struct vfio_ccw_private
>> > *private,
>> >spin_unlock_irq(sch->lock);
>> >
>> >if (ret == -EBUSY)
>> > - wait_for_completion_timeout(, 3*HZ);
>> > + wait_for_completion_timeout(, 3 * HZ);
>> >
>> >private->completion = NULL;
>> >flush_workqueue(vfio_ccw_work_q);
>> >spin_lock_irq(sch->lock);
>> >ret = cio_disable_subchannel(sch);
>> >} while (ret == -EBUSY);
>> > + return ret;
>> > +}
>> > +
>> > +static void fsm_close(struct vfio_ccw_private *private,
>> > +enum vfio_ccw_event event)
>> > +{
>> > + struct subchannel *sch = private->sch;
>> > + int ret;
>> > +
>> > + spin_lock_irq(sch->lock);
>> > + if (!sch->schib.pmcw.ena)
>> > + goto err_unlock;
>> > + ret = cio_disable_subchannel(sch);
>>
>> cio_disable_subchannel() should be happy to disable an already disabled
>> subchannel, so I guess we can just walk through this and end up in
>> CLOSED state... unless entering with !ena actually indicates that we
>> messed up somewhere else in the state machine. I still need to find time
>> to read the patches.
>
> I don't know, I looked at that ena stuff for a bit and couldn't guess
> what it is trying to do.
It is one of the bits in the pmcw control block that can be modified; if
it is 1, the subchannel is enabled and can be used for I/O, if it is 0,
the subchannel is disabled and all instructions that initiate or stop
I/O will fail. Basically, you enable the subchannel if you actually want
to access the device associated with it. Online/offline for (normal
usage) ccw devices maps (among other things) to associated subchannel
enabled/disabled; for a subchannel that is supposed to be passed via
vfio-ccw, we want to have it enabled so that it is actually usable.
I think the ena checking had been inspired from what the ccw bus
does. We could probably just forge ahead in any case and the called
functions in the css bus would be able to handle it just fine, but I
have not double checked.
> Arguably the channel should not be ripped away from vfio while the FSM
> is in the open states, so I'm not sure what a lot of this is for.
We could have surprise removal (i.e. a subchannel in active use being
ripped out), as that's what happens on real hardware as well. E.g. doing
a device_del in QEMU.
Re: [Intel-gfx] [PATCH v2 0/9] Move vfio_ccw to the new mdev API
On Tue, Sep 14 2021, Jason Gunthorpe wrote:
> On Mon, Sep 13, 2021 at 04:31:54PM -0400, Eric Farman wrote:
>> > I rebased it and fixed it up here:
>> >
>> > https://github.com/jgunthorpe/linux/tree/vfio_ccw
>> >
>> > Can you try again?
>>
>> That does address the crash, but then why is it processing a BROKEN
>> event? Seems problematic.
>
> The stuff related to the NOT_OPER looked really wonky to me. I'm
> guessing this is the issue - not sure about the pmcw.ena either..
[I have still not been able to digest the whole series, sorry.]
>
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 5ea392959c0711..0d4d4f425befac 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -380,29 +380,19 @@ static void fsm_open(struct vfio_ccw_private *private,
> spin_unlock_irq(sch->lock);
> }
>
> -static void fsm_close(struct vfio_ccw_private *private,
> - enum vfio_ccw_event event)
> +static int flush_sch(struct vfio_ccw_private *private)
> {
> struct subchannel *sch = private->sch;
> DECLARE_COMPLETION_ONSTACK(completion);
> int iretry, ret = 0;
>
> - spin_lock_irq(sch->lock);
> - if (!sch->schib.pmcw.ena)
> - goto err_unlock;
> - ret = cio_disable_subchannel(sch);
> - if (ret != -EBUSY)
> - goto err_unlock;
> -
> iretry = 255;
> do {
> -
> ret = cio_cancel_halt_clear(sch, );
> -
> if (ret == -EIO) {
> pr_err("vfio_ccw: could not quiesce subchannel
> 0.%x.%04x!\n",
> sch->schid.ssid, sch->schid.sch_no);
> - break;
> + return ret;
Looking at this, I wonder why we had special-cased -EIO -- for -ENODEV
we should be done as well, as then the device is dead and we do not need
to disable it.
> }
>
> /*
> @@ -413,13 +403,28 @@ static void fsm_close(struct vfio_ccw_private *private,
> spin_unlock_irq(sch->lock);
>
> if (ret == -EBUSY)
> - wait_for_completion_timeout(, 3*HZ);
> + wait_for_completion_timeout(, 3 * HZ);
>
> private->completion = NULL;
> flush_workqueue(vfio_ccw_work_q);
> spin_lock_irq(sch->lock);
> ret = cio_disable_subchannel(sch);
> } while (ret == -EBUSY);
> + return ret;
> +}
> +
> +static void fsm_close(struct vfio_ccw_private *private,
> + enum vfio_ccw_event event)
> +{
> + struct subchannel *sch = private->sch;
> + int ret;
> +
> + spin_lock_irq(sch->lock);
> + if (!sch->schib.pmcw.ena)
> + goto err_unlock;
> + ret = cio_disable_subchannel(sch);
cio_disable_subchannel() should be happy to disable an already disabled
subchannel, so I guess we can just walk through this and end up in
CLOSED state... unless entering with !ena actually indicates that we
messed up somewhere else in the state machine. I still need to find time
to read the patches.
> + if (ret == -EBUSY)
> + ret = flush_sch(private);
> if (ret)
> goto err_unlock;
> private->state = VFIO_CCW_STATE_CLOSED;
Re: [Intel-gfx] [PATCH v2 1/9] vfio/ccw: Use functions for alloc/free of the vfio_ccw_private
On Fri, Sep 10 2021, Christoph Hellwig wrote:
> On Thu, Sep 09, 2021 at 04:38:41PM -0300, Jason Gunthorpe wrote:
>> +
>> +private = kzalloc(sizeof(*private), GFP_KERNEL | GFP_DMA);
>> +if (!private)
>> +return ERR_PTR(-ENOMEM);
>
> Nit: there is no need to add GFP_KERNEL when using GFP_DMA.
>
> Also a question to the s390 maintainers: why do we need 31-bit
> addressability for the main private data structure?
I don't think we need it anymore since c98e16b2fa12 ("s390/cio: Convert
ccw_io_region to pointer") and probably should just drop the GFP_DMA.
Re: [Intel-gfx] [PATCH v4 14/14] vfio: Remove struct vfio_device_ops open/release
On Thu, Aug 05 2021, Jason Gunthorpe wrote: > Nothing uses this anymore, delete it. > > Signed-off-by: Yishai Hadas > Reviewed-by: Christoph Hellwig > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/mdev/vfio_mdev.c | 22 -- > drivers/vfio/vfio.c | 14 +- > include/linux/mdev.h | 7 --- > include/linux/vfio.h | 4 > 4 files changed, 1 insertion(+), 46 deletions(-) Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v4 10/14] vfio/pci: Reorganize VFIO_DEVICE_PCI_HOT_RESET to use the device set
On Thu, Aug 05 2021, Jason Gunthorpe wrote: > Like vfio_pci_dev_set_try_reset() this code wants to reset all of the > devices in the "reset group" which is the same membership as the device > set. > > Instead of trying to reconstruct the device set from the PCI list go > directly from the device set's device list to execute the reset. > > The same basic structure as vfio_pci_dev_set_try_reset() is used. The > 'vfio_devices' struct is replaced with the device set linked list and we > simply sweep it multiple times under the lock. > > This eliminates a memory allocation and get/put traffic and another > improperly locked test of pci_dev_driver(). > > Reviewed-off-by: Christoph Hellwig > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/pci/vfio_pci.c | 213 +++- > 1 file changed, 89 insertions(+), 124 deletions(-) Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v4 09/14] vfio/pci: Change vfio_pci_try_bus_reset() to use the dev_set
On Thu, Aug 05 2021, Jason Gunthorpe wrote: > vfio_pci_try_bus_reset() is triggering a reset of the entire_dev set if > any device within it has accumulated a needs_reset. This reset can only be > done once all of the drivers operating the PCI devices to be reset are in > a known safe state. > > Make this clearer by directly operating on the dev_set instead of the > vfio_pci_device. Rename the function to vfio_pci_dev_set_try_reset(). > > Use the device list inside the dev_set to check that all drivers are in a > safe state instead of working backwards from the pci_device. > > The dev_set->lock directly prevents devices from joining/leaving the set, > or changing their state, which further implies the pci_device cannot > change drivers or that the vfio_device be freed, eliminating the need for > get/put's. > > If a pci_device to be reset is not in the dev_set then the reset cannot be > used as we can't know what the state of that driver is. Directly measure > this by checking that every pci_device is in the dev_set - which > effectively proves that VFIO drivers are attached to everything. > > Remove the odd interaction around vfio_pci_set_power_state() - have the > only caller avoid its redundant vfio_pci_set_power_state() instead of > avoiding it inside vfio_pci_dev_set_try_reset(). > > This restructuring corrects a call to pci_dev_driver() without holding the > device_lock() and removes a hard wiring to _pci_driver. > > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/pci/vfio_pci.c | 182 +--- > 1 file changed, 86 insertions(+), 96 deletions(-) Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v4 08/14] vfio/pci: Move to the device set infrastructure
On Thu, Aug 05 2021, Jason Gunthorpe wrote: > From: Yishai Hadas > > PCI wants to have the usual open/close_device() logic with the slight > twist that the open/close_device() must be done under a singelton lock > shared by all of the vfio_devices that are in the PCI "reset group". > > The reset group, and thus the device set, is determined by what devices > pci_reset_bus() touches, which is either the entire bus or only the slot. > > Rely on the core code to do everything reflck was doing and delete reflck > entirely. > > Signed-off-by: Yishai Hadas > Reviewed-by: Christoph Hellwig > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/pci/vfio_pci.c | 162 +++- > drivers/vfio/pci/vfio_pci_private.h | 7 -- > 2 files changed, 37 insertions(+), 132 deletions(-) Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v4 02/14] vfio/mbochs: Fix missing error unwind of mbochs_used_mbytes
On Thu, Aug 05 2021, Jason Gunthorpe wrote:
> Convert mbochs to use an atomic scheme for this like mtty was changed
> into. The atomic fixes various race conditions with probing. Add the
> missing error unwind. Also add the missing kfree of mdev_state->pages.
>
> Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()")
> Reported-by: Cornelia Huck
> Co-developed-by: Alex Williamson
> Reviewed-by: Christoph Hellwig
> Signed-off-by: Jason Gunthorpe
> ---
> samples/vfio-mdev/mbochs.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
Reviewed-by: Cornelia Huck
Re: [Intel-gfx] [PATCH v2 07/14] vfio/platform: Use open_device() instead of open coding a refcnt scheme
On Tue, Jul 20 2021, Jason Gunthorpe wrote: > Platform simply wants to run some code when the device is first > opened/last closed. Use the core framework and locking for this. Aside > from removing a bit of code this narrows the locking scope from a global > lock. > > Signed-off-by: Yishai Hadas > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/platform/vfio_platform_common.c | 79 --- > drivers/vfio/platform/vfio_platform_private.h | 1 - > 2 files changed, 32 insertions(+), 48 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 04/14] vfio: Provide better generic support for open/release vfio_device_ops
On Tue, Jul 20 2021, Jason Gunthorpe wrote: > Currently the driver ops have an open/release pair that is called once > each time a device FD is opened or closed. Add an additional set of > open/close_device() ops which are called when the device FD is opened for > the first time and closed for the last time. > > An analysis shows that all of the drivers require this semantic. Some are > open coding it as part of their reflck implementation, and some are just > buggy and miss it completely. > > To retain the current semantics PCI and FSL depend on, introduce the idea > of a "device set" which is a grouping of vfio_device's that share the same > lock around opening. > > The device set is established by providing a 'set_id' pointer. All > vfio_device's that provide the same pointer will be joined to the same > singleton memory and lock across the whole set. This effectively replaces > the oddly named reflck. > > After conversion the set_id will be sourced from: > - A struct device from a fsl_mc_device (fsl) > - A struct pci_slot (pci) > - A struct pci_bus (pci) > - The struct vfio_device (everything) > > The design ensures that the above pointers are live as long as the > vfio_device is registered, so they form reliable unique keys to group > vfio_devices into sets. > > This implementation uses xarray instead of searching through the driver > core structures, which simplifies the somewhat tricky locking in this > area. > > Following patches convert all the drivers. > > Signed-off-by: Yishai Hadas > Signed-off-by: Jason Gunthorpe > --- > drivers/vfio/mdev/vfio_mdev.c | 22 + > drivers/vfio/vfio.c | 146 +- > include/linux/mdev.h | 2 + > include/linux/vfio.h | 19 + > 4 files changed, 167 insertions(+), 22 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/14] vfio: Introduce a vfio_uninit_group_dev() API call
On Tue, Jul 20 2021, Jason Gunthorpe wrote: > From: Max Gurtovoy > > This pairs with vfio_init_group_dev() and allows undoing any state that is > stored in the vfio_device unrelated to registration. Add appropriately > placed calls to all the drivers. > > The following patch will use this to add pre-registration state for the > device set. > > Signed-off-by: Max Gurtovoy > Signed-off-by: Jason Gunthorpe > --- > Documentation/driver-api/vfio.rst| 4 ++- > drivers/vfio/fsl-mc/vfio_fsl_mc.c| 7 ++--- > drivers/vfio/mdev/vfio_mdev.c| 13 +++--- > drivers/vfio/pci/vfio_pci.c | 6 +++-- > drivers/vfio/platform/vfio_platform_common.c | 7 +++-- > drivers/vfio/vfio.c | 5 > include/linux/vfio.h | 1 + > samples/vfio-mdev/mbochs.c | 2 ++ > samples/vfio-mdev/mdpy.c | 25 ++ > samples/vfio-mdev/mtty.c | 27 > 10 files changed, 64 insertions(+), 33 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/14] vfio/mbochs: Fix missing error unwind in mbochs_probe()
On Tue, Jul 20 2021, Jason Gunthorpe wrote: > On Tue, Jul 20, 2021 at 04:01:27PM -0600, Alex Williamson wrote: >> Hmm, doesn't this suggest we need another atomic conversion? (untested) > > Sure why not, I can add this as another patch Yes, I think that should be another patch. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/14] vfio/mbochs: Fix missing error unwind in mbochs_probe()
On Tue, Jul 20 2021, Jason Gunthorpe wrote:
> Compared to mbochs_remove() two cases are missing from the
> vfio_register_group_dev() unwind. Add them in.
>
> Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()")
> Reported-by: Cornelia Huck
> Signed-off-by: Jason Gunthorpe
> ---
> samples/vfio-mdev/mbochs.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Cornelia Huck
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/13] vfio/gvt: Fix open/close when multiple device FDs are open
On Wed, Jul 14 2021, Jason Gunthorpe wrote: > The user can open multiple device FDs if it likes, however the open > function calls vfio_register_notifier() on device global state. Calling > vfio_register_notifier() twice will trigger a WARN_ON from > notifier_chain_register() and the first close will wrongly delete the > notifier and more. > > Since these really want the new open/close_device() semantics just change > the function over. > > Signed-off-by: Jason Gunthorpe > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/13] vfio/mbochs: Fix close when multiple device FDs are open
On Wed, Jul 14 2021, Jason Gunthorpe wrote: > mbochs_close() iterates over global device state and frees it. Currently > this is done every time a device FD is closed, but if multiple device FDs > are open this could corrupt other still active FDs. > > Change this to use close_device() so it only runs on the last close. > > Signed-off-by: Jason Gunthorpe > --- > samples/vfio-mdev/mbochs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/13] vfio/ap, ccw: Fix open/close when multiple device FDs are open
On Wed, Jul 14 2021, Jason Gunthorpe wrote: > The user can open multiple device FDs if it likes, however these open() > functions call vfio_register_notifier() on some device global > state. Calling vfio_register_notifier() twice in will trigger a WARN_ON > from notifier_chain_register() and the first close will wrongly delete the > notifier and more. > > Since these really want the new open/close_device() semantics just change > the functions over. > > Signed-off-by: Jason Gunthorpe > --- > drivers/s390/cio/vfio_ccw_ops.c | 8 > drivers/s390/crypto/vfio_ap_ops.c | 8 > 2 files changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/13] vfio/samples: Delete useless open/close
On Wed, Jul 14 2021, Jason Gunthorpe wrote: > The core code no longer requires these ops to be defined, so delete these > empty functions and leave the op as NULL. mtty's functions only log a > pointless message, delete that entirely. > > Signed-off-by: Yishai Hadas > Signed-off-by: Jason Gunthorpe > --- > samples/vfio-mdev/mbochs.c | 6 -- > samples/vfio-mdev/mdpy.c | 11 --- > samples/vfio-mdev/mtty.c | 13 - > 3 files changed, 30 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/13] vfio: Provide better generic support for open/release vfio_device_ops
On Wed, Jul 14 2021, Jason Gunthorpe wrote:
> Currently the driver ops have an open/release pair that is called once
> each time a device FD is opened or closed. Add an additional set of
> open/close_device() ops which are called when the device FD is opened for
> the first time and closed for the last time.
>
> An analysis shows that all of the drivers require this semantic. Some are
> open coding it as part of their reflck implementation, and some are just
> buggy and miss it completely.
>
> To retain the current semantics PCI and FSL depend on, introduce the idea
> of a "device set" which is a grouping of vfio_device's that share the same
> lock around opening.
>
> The device set is established by providing a 'set_id' pointer. All
> vfio_device's that provide the same pointer will be joined to the same
> singleton memory and lock across the whole set. This effectively replaces
> the oddly named reflck.
>
> After conversion the set_id will be sourced from:
> - A struct device from a fsl_mc_device (fsl)
> - A struct pci_slot (pci)
> - A struct pci_bus (pci)
> - The struct vfio_device (everything)
>
> The design ensures that the above pointers are live as long as the
> vfio_device is registered, so they form reliable unique keys to group
> vfio_devices into sets.
>
> This implementation uses xarray instead of searching through the driver
> core structures, which simplifies the somewhat tricky locking in this
> area.
>
> Following patches convert all the drivers.
>
> Signed-off-by: Yishai Hadas
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/vfio/mdev/vfio_mdev.c | 22 ++
> drivers/vfio/vfio.c | 144 --
> include/linux/mdev.h | 2 +
> include/linux/vfio.h | 19 +
> 4 files changed, 165 insertions(+), 22 deletions(-)
>
(...)
> @@ -760,6 +829,13 @@ int vfio_register_group_dev(struct vfio_device *device)
> struct iommu_group *iommu_group;
> struct vfio_group *group;
>
> + /*
> + * If the driver doesn't specify a set then the device is added to a
> + * signleton set just for itself.
s/signleton/singleton/
> + */
> + if (!device->dev_set)
> + vfio_assign_device_set(device, device);
> +
> iommu_group = iommu_group_get(device->dev);
> if (!iommu_group)
> return -EINVAL;
> @@ -1361,7 +1437,8 @@ static int vfio_group_get_device_fd(struct vfio_group
> *group, char *buf)
> {
> struct vfio_device *device;
> struct file *filep;
> - int ret;
> + int fdno;
> + int ret = 0;
>
> if (0 == atomic_read(>container_users) ||
> !group->container->iommu_driver || !vfio_group_viable(group))
> @@ -1375,38 +1452,38 @@ static int vfio_group_get_device_fd(struct vfio_group
> *group, char *buf)
> return PTR_ERR(device);
>
> if (!try_module_get(device->dev->driver->owner)) {
> - vfio_device_put(device);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto err_device_put;
> }
>
> - ret = device->ops->open(device);
> - if (ret) {
> - module_put(device->dev->driver->owner);
> - vfio_device_put(device);
> - return ret;
> + mutex_lock(>dev_set->lock);
> + device->open_count++;
> + if (device->open_count == 1 && device->ops->open_device) {
> + ret = device->ops->open_device(device);
> + if (ret)
> + goto err_undo_count;
Won't that fail for mdev devices, until the patches later in this series
have been applied? (i.e. bad for bisect)
> + }
> + mutex_unlock(>dev_set->lock);
> +
> + if (device->ops->open) {
> + ret = device->ops->open(device);
> + if (ret)
> + goto err_close_device;
> }
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] vfio: Introduce a vfio_uninit_group_dev() API call
On Mon, Jul 19 2021, Jason Gunthorpe wrote: > On Mon, Jul 19, 2021 at 02:11:38PM +0200, Cornelia Huck wrote: >> On Wed, Jul 14 2021, Jason Gunthorpe wrote: >> >> > From: Max Gurtovoy >> > >> > This pairs with vfio_init_group_dev() and allows undoing any state that is >> > stored in the vfio_device unrelated to registration. Add appropriately >> > placed calls to all the drivers. >> > >> > The following patch will use this to add pre-registration state for the >> > device set. >> > >> > Signed-off-by: Max Gurtovoy >> > Signed-off-by: Jason Gunthorpe >> > Documentation/driver-api/vfio.rst| 4 ++- >> > drivers/vfio/fsl-mc/vfio_fsl_mc.c| 6 +++-- >> > drivers/vfio/mdev/vfio_mdev.c| 13 +++--- >> > drivers/vfio/pci/vfio_pci.c | 6 +++-- >> > drivers/vfio/platform/vfio_platform_common.c | 7 +++-- >> > drivers/vfio/vfio.c | 5 >> > include/linux/vfio.h | 1 + >> > samples/vfio-mdev/mbochs.c | 2 ++ >> > samples/vfio-mdev/mdpy.c | 25 ++ >> > samples/vfio-mdev/mtty.c | 27 >> > 10 files changed, 64 insertions(+), 32 deletions(-) >> >> (...) >> >> > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c >> > index e81b875b4d87b4..cf264d0bf11053 100644 >> > +++ b/samples/vfio-mdev/mbochs.c >> > @@ -558,6 +558,7 @@ static int mbochs_probe(struct mdev_device *mdev) >> >return 0; >> > >> > err_mem: >> > + vfio_uninit_group_dev(_state->vdev); >> >kfree(mdev_state->vconfig); >> >kfree(mdev_state); >> >return ret; > > Doesn't this leak pages? Sigh. I think it also fails to decrease mbochs_used_mbytes; looks like there need to be two cleanup labels. > >> > @@ -571,6 +572,7 @@ static void mbochs_remove(struct mdev_device *mdev) >> >vfio_unregister_group_dev(_state->vdev); >> >kfree(mdev_state->pages); >> >kfree(mdev_state->vconfig); >> > + vfio_uninit_group_dev(_state->vdev); >> >> Does the location of the uninit vs the kfree matter? Even if not, it >> might be good to keep it consistent. > > It does not, but I will reorder it anyhow > > Jason ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] vfio: Introduce a vfio_uninit_group_dev() API call
On Wed, Jul 14 2021, Jason Gunthorpe wrote: > From: Max Gurtovoy > > This pairs with vfio_init_group_dev() and allows undoing any state that is > stored in the vfio_device unrelated to registration. Add appropriately > placed calls to all the drivers. > > The following patch will use this to add pre-registration state for the > device set. > > Signed-off-by: Max Gurtovoy > Signed-off-by: Jason Gunthorpe > --- > Documentation/driver-api/vfio.rst| 4 ++- > drivers/vfio/fsl-mc/vfio_fsl_mc.c| 6 +++-- > drivers/vfio/mdev/vfio_mdev.c| 13 +++--- > drivers/vfio/pci/vfio_pci.c | 6 +++-- > drivers/vfio/platform/vfio_platform_common.c | 7 +++-- > drivers/vfio/vfio.c | 5 > include/linux/vfio.h | 1 + > samples/vfio-mdev/mbochs.c | 2 ++ > samples/vfio-mdev/mdpy.c | 25 ++ > samples/vfio-mdev/mtty.c | 27 > 10 files changed, 64 insertions(+), 32 deletions(-) (...) > diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c > index e81b875b4d87b4..cf264d0bf11053 100644 > --- a/samples/vfio-mdev/mbochs.c > +++ b/samples/vfio-mdev/mbochs.c > @@ -558,6 +558,7 @@ static int mbochs_probe(struct mdev_device *mdev) > return 0; > > err_mem: > + vfio_uninit_group_dev(_state->vdev); > kfree(mdev_state->vconfig); > kfree(mdev_state); > return ret; > @@ -571,6 +572,7 @@ static void mbochs_remove(struct mdev_device *mdev) > vfio_unregister_group_dev(_state->vdev); > kfree(mdev_state->pages); > kfree(mdev_state->vconfig); > + vfio_uninit_group_dev(_state->vdev); Does the location of the uninit vs the kfree matter? Even if not, it might be good to keep it consistent. > kfree(mdev_state); > } > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/13] vfio/samples: Remove module get/put
On Wed, Jul 14 2021, Jason Gunthorpe wrote:
> The patch to move the get/put to core and the patch to convert the samples
> to use vfio_device crossed in a way that this was missed. When both
> patches are together the samples do not need their own get/put.
>
> Fixes: 437e41368c01 ("vfio/mdpy: Convert to use vfio_register_group_dev()")
> Fixes: 681c1615f891 ("vfio/mbochs: Convert to use vfio_register_group_dev()")
> Signed-off-by: Jason Gunthorpe
> ---
> samples/vfio-mdev/mbochs.c | 4
> samples/vfio-mdev/mdpy.c | 4
> 2 files changed, 8 deletions(-)
Reviewed-by: Cornelia Huck
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/10] vfio/mdev: Allow the mdev_parent_ops to specify the device driver to bind
On Tue, Jun 15 2021, Christoph Hellwig wrote: > From: Jason Gunthorpe > > This allows a mdev driver to opt out of using vfio_mdev.c, instead the > driver will provide a 'struct mdev_driver' and register directly with the > driver core. > > Much of mdev_parent_ops becomes unused in this mode: > - create()/remove() are done via the mdev_driver probe()/remove() > - mdev_attr_groups becomes mdev_driver driver.dev_groups > - Wrapper function callbacks are replaced with the same ones from > struct vfio_device_ops > > Signed-off-by: Jason Gunthorpe > Signed-off-by: Christoph Hellwig > --- > drivers/vfio/mdev/mdev_core.c | 30 ++ > drivers/vfio/mdev/mdev_driver.c | 10 ++ > include/linux/mdev.h| 2 ++ > 3 files changed, 34 insertions(+), 8 deletions(-) > Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/10] driver core: Don't return EPROBE_DEFER to userspace during sysfs bind
On Tue, Jun 15 2021, Christoph Hellwig wrote:
> EPROBE_DEFER is an internal kernel error code and it should not be leaked
> to userspace via the bind_store() sysfs. Userspace doesn't have this
> constant and cannot understand it.
>
> Further, it doesn't really make sense to have userspace trigger a deferred
> probe via bind_store(), which could eventually succeed, while
> simultaneously returning an error back.
>
> Resolve this by splitting driver_probe_device so that the version used
> by the sysfs binding that turns EPROBE_DEFER into -EAGAIN, while the one
> used for internally binding keeps the error code, and calls
> driver_deferred_probe_add where needed. This also allows to nicely split
> out the defer_all_probes / probe_count checks so that they actually allow
> for full device_{block,unblock}_probing protection while not bothering
> the sysfs bind case.
>
> Signed-off-by: Christoph Hellwig
> Reviewed-by: Greg Kroah-Hartman
> ---
> drivers/base/dd.c | 78 +--
> 1 file changed, 42 insertions(+), 36 deletions(-)
>
Reviewed-by: Cornelia Huck
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/10] driver core: Better distinguish probe errors in really_probe
On Tue, Jun 15 2021, Christoph Hellwig wrote:
> really_probe tries to special case errors from ->probe, but due to all
> other initialization added to the function over time now a lot of
> internal errors hit that code path as well. Untangle that by adding
> a new probe_err local variable and apply the special casing only to
> that.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/base/dd.c | 72 +++
> 1 file changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 7477d3322b3a..fd83817240e6 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -513,12 +513,44 @@ static ssize_t state_synced_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(state_synced);
>
> +
> +static int call_driver_probe(struct device *dev, struct device_driver *drv)
> +{
> + int ret = 0;
> +
> + if (dev->bus->probe)
> + ret = dev->bus->probe(dev);
> + else if (drv->probe)
> + ret = drv->probe(dev);
> +
> + switch (ret) {
> + case 0:
> + break;
> + case -EPROBE_DEFER:
> + /* Driver requested deferred probing */
> + dev_dbg(dev, "Driver %s requests probe deferral\n", drv->name);
> + break;
> + case -ENODEV:
> + case -ENXIO:
> + pr_debug("%s: probe of %s rejects match %d\n",
> + drv->name, dev_name(dev), ret);
> + break;
> + default:
> + /* driver matched but the probe failed */
> + pr_warn("%s: probe of %s failed with error %d\n",
> + drv->name, dev_name(dev), ret);
Convert these two pr_* to dev_* when touching the code anyway?
> + break;
> + }
> +
> + return ret;
> +}
(...)
Reviewed-by: Cornelia Huck
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/10] vfio/mdev: Allow the mdev_parent_ops to specify the device driver to bind
On Mon, Jun 14 2021, Christoph Hellwig wrote: > From: Jason Gunthorpe > > This allows a mdev driver to opt out of using vfio_mdev.c, instead the > driver will provide a 'struct mdev_driver' and register directly with the > driver core. > > Much of mdev_parent_ops becomes unused in this mode: > - create()/remove() are done via the mdev_driver probe()/remove() > - mdev_attr_groups becomes mdev_driver driver.dev_groups > - Wrapper function callbacks are replaced with the same ones from > struct vfio_device_ops > > Signed-off-by: Jason Gunthorpe > Signed-off-by: Christoph Hellwig > --- > drivers/vfio/mdev/mdev_core.c | 30 ++ > drivers/vfio/mdev/mdev_driver.c | 10 ++ > include/linux/mdev.h| 2 ++ > 3 files changed, 34 insertions(+), 8 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] vfio/mdev: Remove CONFIG_VFIO_MDEV_DEVICE
On Mon, Jun 14 2021, Christoph Hellwig wrote: > From: Jason Gunthorpe > > For some reason the vfio_mdev shim mdev_driver has its own module and > kconfig. As the next patch requires access to it from mdev.ko merge the > two modules together and remove VFIO_MDEV_DEVICE. > > A later patch deletes this driver entirely. > > Signed-off-by: Jason Gunthorpe > Signed-off-by: Christoph Hellwig > --- > Documentation/s390/vfio-ap.rst | 1 - > arch/s390/Kconfig| 2 +- > drivers/gpu/drm/i915/Kconfig | 2 +- > drivers/vfio/mdev/Kconfig| 7 --- > drivers/vfio/mdev/Makefile | 3 +-- > drivers/vfio/mdev/mdev_core.c| 16 ++-- > drivers/vfio/mdev/mdev_private.h | 2 ++ > drivers/vfio/mdev/vfio_mdev.c| 24 +--- > samples/Kconfig | 6 +++--- > 9 files changed, 23 insertions(+), 40 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/10] driver core: Export device_driver_attach()
On Mon, Jun 14 2021, Christoph Hellwig wrote: > From: Jason Gunthorpe > > This is intended as a replacement API for device_bind_driver(). It has at > least the following benefits: > > - Internal locking. Few of the users of device_bind_driver() follow the > locking rules > > - Calls device driver probe() internally. Notably this means that devm > support for probe works correctly as probe() error will call > devres_release_all() > > - struct device_driver -> dev_groups is supported > > - Simplified calling convention, no need to manually call probe(). > > The general usage is for situations that already know what driver to bind > and need to ensure the bind is synchronized with other logic. Call > device_driver_attach() after device_add(). > > If probe() returns a failure then this will be preserved up through to the > error return of device_driver_attach(). > > Signed-off-by: Jason Gunthorpe > Signed-off-by: Christoph Hellwig > --- > drivers/base/base.h| 1 - > drivers/base/dd.c | 3 +++ > include/linux/device.h | 2 ++ > 3 files changed, 5 insertions(+), 1 deletion(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/10] driver core: Flow the return code from ->probe() through to sysfs bind
On Mon, Jun 14 2021, Christoph Hellwig wrote: > Currently really_probe() returns 1 on success and 0 if the probe() call > fails. This return code arrangement is designed to be useful for > __device_attach_driver() which is walking the device list and trying every > driver. 0 means to keep trying. > > However, it is not useful for the other places that call through to > really_probe() that do actually want to see the probe() return code. > > For instance bind_store() would be better to return the actual error code > from the driver's probe method, not discarding it and returning -ENODEV. > > Reorganize things so that really_probe() returns the error code from > ->probe as a (inverted) positive number, and 0 for successful attach. > > With this, __device_attach_driver can ignore the (positive) probe errors, > return 1 to exit the loop for a successful binding and pass on the > other negative errors, while device_driver_attach simplify inverts the > positive errors and returns all errors to the sysfs code. > > Signed-off-by: Christoph Hellwig > --- > drivers/base/bus.c | 6 +- > drivers/base/dd.c | 29 ----- > 2 files changed, 21 insertions(+), 14 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/10] driver core: Pull required checks into driver_probe_device()
On Mon, Jun 14 2021, Christoph Hellwig wrote: > From: Jason Gunthorpe > > Checking if the dev is dead or if the dev is already bound is a required > precondition to invoking driver_probe_device(). All the call chains > leading here duplicate these checks. > > Add it directly to driver_probe_device() so the precondition is clear and > remove the checks from device_driver_attach() and > __driver_attach_async_helper(). > > The other call chain going through __device_attach_driver() does have > these same checks but they are inlined into logic higher up the call stack > and can't be removed. > > The sysfs uAPI call chain starting at bind_store() is a bit confused > because it reads dev->driver unlocked and returns -ENODEV if it is !NULL, > otherwise it reads it again under lock and returns 0 if it is !NULL. Fix > this to always return -EBUSY and always read dev->driver under its lock. > > Done in preparation for the next patches which will add additional > callers to driver_probe_device() and will need these checks as well. > > Signed-off-by: Jason Gunthorpe > [hch: drop the extra checks in device_driver_attach and bind_store] > Signed-off-by: Christoph Hellwig > --- > drivers/base/bus.c | 2 +- > drivers/base/dd.c | 32 ++-- > 2 files changed, 11 insertions(+), 23 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/10] vfio/mdev: Remove CONFIG_VFIO_MDEV_DEVICE
On Mon, Jun 07 2021, Jason Gunthorpe wrote: > For some reason the vfio_mdev shim mdev_driver has its own module and > kconfig. As the next patch requires access to it from mdev.ko merge the > two modules together and remove VFIO_MDEV_DEVICE. > > A later patch deletes this driver entirely. > > Signed-off-by: Jason Gunthorpe > --- > Documentation/s390/vfio-ap.rst | 1 - > arch/s390/Kconfig| 2 +- > drivers/gpu/drm/i915/Kconfig | 2 +- > drivers/vfio/mdev/Kconfig| 7 --- > drivers/vfio/mdev/Makefile | 3 +-- > drivers/vfio/mdev/mdev_core.c| 16 ++-- > drivers/vfio/mdev/mdev_private.h | 2 ++ > drivers/vfio/mdev/vfio_mdev.c| 24 +--- > samples/Kconfig | 6 +++--- > 9 files changed, 23 insertions(+), 40 deletions(-) I think you missed my earlier Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 01/13] vfio/mdev: Remove CONFIG_VFIO_MDEV_DEVICE
On Mon, 26 Apr 2021 17:00:03 -0300 Jason Gunthorpe wrote: > For some reason the vfio_mdev shim mdev_driver has its own module and > kconfig. As the next patch requires access to it from mdev.ko merge the > two modules together and remove VFIO_MDEV_DEVICE. > > A later patch deletes this driver entirely. > > Signed-off-by: Jason Gunthorpe > --- > Documentation/s390/vfio-ap.rst | 1 - > arch/s390/Kconfig| 2 +- > drivers/gpu/drm/i915/Kconfig | 2 +- > drivers/vfio/mdev/Kconfig| 7 --- > drivers/vfio/mdev/Makefile | 3 +-- > drivers/vfio/mdev/mdev_core.c| 16 ++-- > drivers/vfio/mdev/mdev_private.h | 2 ++ > drivers/vfio/mdev/vfio_mdev.c| 24 +--- > samples/Kconfig | 6 +++--- > 9 files changed, 23 insertions(+), 40 deletions(-) This also fixes the dependencies for vfio-ccw, which never depended on VFIO_MDEV_DEVICE directly... Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 18/18] vfio/mdev: Correct the function signatures for the mdev_type_attributes
On Tue, 23 Mar 2021 14:55:35 -0300 Jason Gunthorpe wrote: > The driver core standard is to pass in the properly typed object, the > properly typed attribute and the buffer data. It stems from the root > kobject method: > > ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,..) > > Each subclass of kobject should provide their own function with the same > signature but more specific types, eg struct device uses: > > ssize_t (*show)(struct device *dev, struct device_attribute *attr,..) > > In this case the existing signature is: > > ssize_t (*show)(struct kobject *kobj, struct device *dev,..) > > Where kobj is a 'struct mdev_type *' and dev is 'mdev_type->parent->dev'. > > Change the mdev_type related sysfs attribute functions to: > > ssize_t (*show)(struct mdev_type *mtype, struct mdev_type_attribute > *attr,..) > > In order to restore type safety and match the driver core standard > > There are no current users of 'attr', but if it is ever needed it would be > hard to add in retroactively, so do it now. > > Signed-off-by: Jason Gunthorpe > --- > drivers/gpu/drm/i915/gvt/gvt.c| 21 +++-- > drivers/s390/cio/vfio_ccw_ops.c | 15 +-- > drivers/s390/crypto/vfio_ap_ops.c | 12 +++- > drivers/vfio/mdev/mdev_core.c | 14 -- > drivers/vfio/mdev/mdev_sysfs.c| 11 ++- > include/linux/mdev.h | 11 +++ > samples/vfio-mdev/mbochs.c| 26 +++--- > samples/vfio-mdev/mdpy.c | 24 ++-- > samples/vfio-mdev/mtty.c | 18 +- > 9 files changed, 90 insertions(+), 62 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 17/18] vfio/mdev: Remove kobj from mdev_parent_ops->create()
On Tue, 23 Mar 2021 14:55:34 -0300 Jason Gunthorpe wrote: > The kobj here is a type-erased version of mdev_type, which is already > stored in the struct mdev_device being passed in. It was only ever used to > compute the type_group_id, which is now extracted directly from the mdev. > > Signed-off-by: Jason Gunthorpe > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +- > drivers/s390/cio/vfio_ccw_ops.c | 2 +- > drivers/s390/crypto/vfio_ap_ops.c | 2 +- > drivers/vfio/mdev/mdev_core.c | 2 +- > include/linux/mdev.h | 3 +-- > samples/vfio-mdev/mbochs.c| 2 +- > samples/vfio-mdev/mdpy.c | 2 +- > samples/vfio-mdev/mtty.c | 2 +- > 8 files changed, 8 insertions(+), 9 deletions(-) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V13 6/6] docs: sample driver to demonstrate how to implement virtio-mdev framework
On Mon, 18 Nov 2019 18:59:23 +0800
Jason Wang wrote:
[Note: I have not looked into the reworked architecture of this *at all*
so far; just something that I noted...]
> This sample driver creates mdev device that simulate virtio net device
> over virtio mdev transport. The device is implemented through vringh
> and workqueue. A device specific dma ops is to make sure HVA is used
> directly as the IOVA. This should be sufficient for kernel virtio
> driver to work.
>
> Only 'virtio' type is supported right now. I plan to add 'vhost' type
> on top which requires some virtual IOMMU implemented in this sample
> driver.
>
> Signed-off-by: Jason Wang
> ---
> MAINTAINERS| 1 +
> samples/Kconfig| 10 +
> samples/vfio-mdev/Makefile | 1 +
> samples/vfio-mdev/mvnet_loopback.c | 690 +
> 4 files changed, 702 insertions(+)
> create mode 100644 samples/vfio-mdev/mvnet_loopback.c
>
> +static struct mvnet_dev {
> + struct class*vd_class;
> + struct idr vd_idr;
> + struct device dev;
> +} mvnet_dev;
This structure embeds a struct device (a reference-counted structure),
yet it is a static variable. This is giving a bad example to potential
implementers; just allocate it dynamically.
> +static void mvnet_device_release(struct device *dev)
> +{
> + dev_dbg(dev, "mvnet: released\n");
And that also means you need a proper release function here, of
course.
> +}
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V11 3/6] mdev: introduce device specific ops
On Thu, 7 Nov 2019 23:11:06 +0800 Jason Wang wrote: > Currently, except for the create and remove, the rest of > mdev_parent_ops is designed for vfio-mdev driver only and may not help > for kernel mdev driver. With the help of class id, this patch > introduces device specific callbacks inside mdev_device > structure. This allows different set of callback to be used by > vfio-mdev and virtio-mdev. > > Reviewed-by: Parav Pandit > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 35 + > MAINTAINERS | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > drivers/vfio/mdev/mdev_core.c | 24 - > drivers/vfio/mdev/mdev_private.h | 5 ++ > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > include/linux/mdev.h | 43 --- > include/linux/mdev_vfio_ops.h | 52 +++ > samples/vfio-mdev/mbochs.c| 20 --- > samples/vfio-mdev/mdpy.c | 20 --- > samples/vfio-mdev/mtty.c | 18 --- > 13 files changed, 206 insertions(+), 99 deletions(-) > create mode 100644 include/linux/mdev_vfio_ops.h You dropped my R-b :(, here it is again: Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V11 6/6] docs: sample driver to demonstrate how to implement virtio-mdev framework
On Thu, 7 Nov 2019 23:11:09 +0800 Jason Wang wrote: > This sample driver creates mdev device that simulate virtio net device > over virtio mdev transport. The device is implemented through vringh > and workqueue. A device specific dma ops is to make sure HVA is used > directly as the IOVA. This should be sufficient for kernel virtio > driver to work. > > Only 'virtio' type is supported right now. I plan to add 'vhost' type > on top which requires some virtual IOMMU implemented in this sample > driver. > > Signed-off-by: Jason Wang > --- > MAINTAINERS| 1 + > samples/Kconfig| 10 + > samples/vfio-mdev/Makefile | 1 + > samples/vfio-mdev/mvnet_loopback.c | 687 + > 4 files changed, 699 insertions(+) > create mode 100644 samples/vfio-mdev/mvnet_loopback.c Acked-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V10 0/6] mdev based hardware virtio offloading support
On Wed, 6 Nov 2019 21:35:25 +0800 Jason Wang wrote: > Hi all: > > There are hardwares that can do virtio datapath offloading while > having its own control path. This path tries to implement a mdev based > unified API to support using kernel virtio driver to drive those > devices. This is done by introducing a new mdev transport for virtio > (virtio_mdev) and register itself as a new kind of mdev driver. Then > it provides a unified way for kernel virtio driver to talk with mdev > device implementation. > > Though the series only contains kernel driver support, the goal is to > make the transport generic enough to support userspace drivers. This > means vhost-mdev[1] could be built on top as well by resuing the > transport. > > A sample driver is also implemented which simulate a virito-net > loopback ethernet device on top of vringh + workqueue. This could be > used as a reference implementation for real hardware driver. > > Also a real IFC VF driver was also posted here[2] which is a good > reference for vendors who is interested in their own virtio datapath > offloading product. > > Consider mdev framework only support VFIO device and driver right now, > this series also extend it to support other types. This is done > through introducing class id to the device and pairing it with > id_talbe claimed by the driver. On top, this seris also decouple > device specific ops out of the common ones for implementing class > specific operations over mdev bus. > > Pktgen test was done with virito-net + mvnet loop back device. > > Please review. All looking good to me now. > > [1] https://lkml.org/lkml/2019/11/5/424 > [2] https://lkml.org/lkml/2019/11/5/227 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V9 5/6] virtio: introduce a mdev based transport
On Wed, 6 Nov 2019 15:05:47 +0800 Jason Wang wrote: > This patch introduces a new mdev transport for virtio. This is used to > use kernel virtio driver to drive the mediated device that is capable > of populating virtqueue directly. > > A new virtio-mdev driver will be registered to the mdev bus, when a > new virtio-mdev device is probed, it will register the device with > mdev based config ops. This means it is a software transport between > mdev driver and mdev device. The transport was implemented through > device specific ops which is a part of mdev_parent_ops now. > > Signed-off-by: Jason Wang > --- > drivers/virtio/Kconfig | 13 ++ > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_mdev.c | 406 +++ > 3 files changed, 420 insertions(+) > create mode 100644 drivers/virtio/virtio_mdev.c > > diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig > index 078615cf2afc..558ac607d107 100644 > --- a/drivers/virtio/Kconfig > +++ b/drivers/virtio/Kconfig > @@ -43,6 +43,19 @@ config VIRTIO_PCI_LEGACY > > If unsure, say Y. > > +config VIRTIO_MDEV > + tristate "MDEV driver for virtio devices" > + depends on VFIO_MDEV && VIRTIO > + default n > + help > + This driver provides support for virtio based paravirtual > + device driver over MDEV bus. This requires your environemnt > + has appropriate virtio mdev device implementation which may > + operate on the physical device that the datapath of virtio > + could be offloaded to hardware. That sentence is a bit confusing to me... what about "For this to be useful, you need an appropriate virtio mdev device implementation that operates on a physical device to allow the datapath of virtio to be offloaded to hardware." ? > + > + If unsure, say M Building this as a module should not hurt (but please add a trailing '.' here :) > + > config VIRTIO_PMEM > tristate "Support for virtio pmem driver" > depends on VIRTIO With the changes above, Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V9 4/6] mdev: introduce virtio device and its device ops
On Wed, 6 Nov 2019 15:05:46 +0800 Jason Wang wrote: > This patch implements basic support for mdev driver that supports > virtio transport for kernel virtio driver. > > Signed-off-by: Jason Wang > --- > MAINTAINERS | 1 + > drivers/vfio/mdev/mdev_core.c| 21 + > drivers/vfio/mdev/mdev_private.h | 2 + > include/linux/mdev.h | 6 ++ > include/linux/mdev_virtio_ops.h | 147 +++ > 5 files changed, 177 insertions(+) > create mode 100644 include/linux/mdev_virtio_ops.h Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V9 3/6] mdev: introduce device specific ops
On Wed, 6 Nov 2019 15:05:45 +0800 Jason Wang wrote: > Currently, except for the create and remove, the rest of > mdev_parent_ops is designed for vfio-mdev driver only and may not help > for kernel mdev driver. With the help of class id, this patch > introduces device specific callbacks inside mdev_device > structure. This allows different set of callback to be used by > vfio-mdev and virtio-mdev. > > Reviewed-by: Parav Pandit > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 35 + > MAINTAINERS | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > drivers/vfio/mdev/mdev_core.c | 24 - > drivers/vfio/mdev/mdev_private.h | 5 ++ > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > include/linux/mdev.h | 43 --- > include/linux/mdev_vfio_ops.h | 52 +++ > samples/vfio-mdev/mbochs.c| 20 --- > samples/vfio-mdev/mdpy.c | 20 --- > samples/vfio-mdev/mtty.c | 18 --- > 13 files changed, 206 insertions(+), 99 deletions(-) > create mode 100644 include/linux/mdev_vfio_ops.h Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 6/6] docs: sample driver to demonstrate how to implement virtio-mdev framework
On Tue, 5 Nov 2019 17:32:40 +0800 Jason Wang wrote: > This sample driver creates mdev device that simulate virtio net device > over virtio mdev transport. The device is implemented through vringh > and workqueue. A device specific dma ops is to make sure HVA is used > directly as the IOVA. This should be sufficient for kernel virtio > driver to work. > > Only 'virtio' type is supported right now. I plan to add 'vhost' type > on top which requires some virtual IOMMU implemented in this sample > driver. > > Signed-off-by: Jason Wang > --- > MAINTAINERS| 1 + > samples/Kconfig| 7 + > samples/vfio-mdev/Makefile | 1 + > samples/vfio-mdev/mvnet.c | 685 + > 4 files changed, 694 insertions(+) > create mode 100644 samples/vfio-mdev/mvnet.c Have not really reviewed this, but looks sane at a glance. Acked-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 5/6] virtio: introduce a mdev based transport
On Tue, 5 Nov 2019 17:32:39 +0800 Jason Wang wrote: > This patch introduces a new mdev transport for virtio. This is used to > use kernel virtio driver to drive the mediated device that is capable > of populating virtqueue directly. > > A new virtio-mdev driver will be registered to the mdev bus, when a > new virtio-mdev device is probed, it will register the device with > mdev based config ops. This means it is a software transport between > mdev driver and mdev device. The transport was implemented through > device specific ops which is a part of mdev_parent_ops now. > > Signed-off-by: Jason Wang > --- > drivers/virtio/Kconfig | 7 + > drivers/virtio/Makefile | 1 + > drivers/virtio/virtio_mdev.c | 407 +++ > 3 files changed, 415 insertions(+) > create mode 100644 drivers/virtio/virtio_mdev.c Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 3/6] mdev: introduce device specific ops
On Tue, 5 Nov 2019 10:44:18 -0700 Alex Williamson wrote: > On Tue, 5 Nov 2019 17:50:25 +0100 > Cornelia Huck wrote: > > > On Tue, 5 Nov 2019 17:32:37 +0800 > > Jason Wang wrote: > > > > > Currently, except for the create and remove, the rest of > > > mdev_parent_ops is designed for vfio-mdev driver only and may not help > > > for kernel mdev driver. With the help of class id, this patch > > > introduces device specific callbacks inside mdev_device > > > structure. This allows different set of callback to be used by > > > vfio-mdev and virtio-mdev. > > > > > > Reviewed-by: Parav Pandit > > > Signed-off-by: Jason Wang > > > --- > > > .../driver-api/vfio-mediated-device.rst | 35 + > > > MAINTAINERS | 1 + > > > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > > > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > > > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > > > drivers/vfio/mdev/mdev_core.c | 24 - > > > drivers/vfio/mdev/mdev_private.h | 5 ++ > > > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > > > include/linux/mdev.h | 43 --- > > > include/linux/mdev_vfio_ops.h | 52 +++ > > > samples/vfio-mdev/mbochs.c| 20 --- > > > samples/vfio-mdev/mdpy.c | 20 --- > > > samples/vfio-mdev/mtty.c | 18 --- > > > 13 files changed, 206 insertions(+), 99 deletions(-) > > > create mode 100644 include/linux/mdev_vfio_ops.h > > > > > > > (...) > > > > > @@ -172,10 +163,34 @@ that a driver should use to unregister itself with > > > the mdev core driver:: > > > > > > extern void mdev_unregister_device(struct device *dev); > > > > > > -It is also required to specify the class_id in create() callback > > > through:: > > > +As multiple types of mediated devices may be supported, class id needs > > > +to be specified in the create callback(). This could be done > > > > The brackets should probably go behind 'create'? > > > > > +explicitly for the device that does not use on mdev bus for its > > > > "for devices that do not use the mdev bus" ? > > > > But why wouldn't they? I feel like I've missed some discussion here :/ > > The device ops provide a route through mdev-core for known callbacks, > which is primarily useful when we have 1:N relation between mdev bus > driver and vendor drivers. The obvious example here is vfio-mdev, > where we have GVT-g, vfio-ap, vfio-ccw, NVIDIA GRID, and various sample > drivers all advertising vfio-mdev support via their class id. However, > if we have a tightly coupled vendor driver and mdev bus driver, as the > mlx5 support that Parav is developing, the claim is that they prefer > not to expose any device ops and intend to interact directly with the > mdev device. At least that's my understanding. Thanks, > > Alex Ah, ok. So maybe use the phrasing "devices that interact with the mdev device directly" vs "devices that use device-specific ops" instead? Not a strong critique, though. > > > > +operation through: > > > > > > int mdev_set_class(struct mdev_device *mdev, u16 id); > > > > > > +For the device that uses on the mdev bus for its operation, the > > > class > > > > "For devices that use the mdev bus..." > > > > But same comment as above. > > > > > +should provide helper function to set class id and device > > > specific +ops. E.g for vfio-mdev devices, the function to be > > > called is:: + > > > + int mdev_set_vfio_ops(struct mdev_device *mdev, > > > + const struct mdev_vfio_device_ops > > > *vfio_ops); + > > > +The class id (set by this function to MDEV_CLASS_ID_VFIO) is > > > used to +match a device with an mdev driver via its id table. The > > > device +specific callbacks (specified in *vfio_ops) are > > > obtainable via +mdev_get_vfio_ops() (for use by the mdev bus > > > driver). A vfio-mdev +device (class id MDEV_CLASS_ID_VFIO) uses > > > the following +device-specific ops: > > > + > > > +* open: open callback of vfio mediated device > > > +* close: close callback of vfio mediated device > > > +* ioctl: ioctl callback of vfio mediated device > > > +* read : read emulation callback > > > +* write: write emulation callback > > > +* mmap: mmap emulation callback > > > + > > > Mediated Device Management Interface Through sysfs > > > == > > > > Otherwise, looks good. > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 4/6] mdev: introduce virtio device and its device ops
On Tue, 5 Nov 2019 17:32:38 +0800 Jason Wang wrote: > This patch implements basic support for mdev driver that supports > virtio transport for kernel virtio driver. > > Signed-off-by: Jason Wang > --- > drivers/vfio/mdev/mdev_core.c| 21 + > drivers/vfio/mdev/mdev_private.h | 2 + > include/linux/mdev.h | 6 ++ > include/linux/mdev_virtio_ops.h | 149 +++ > 4 files changed, 178 insertions(+) > create mode 100644 include/linux/mdev_virtio_ops.h Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 3/6] mdev: introduce device specific ops
On Tue, 5 Nov 2019 17:32:37 +0800 Jason Wang wrote: > Currently, except for the create and remove, the rest of > mdev_parent_ops is designed for vfio-mdev driver only and may not help > for kernel mdev driver. With the help of class id, this patch > introduces device specific callbacks inside mdev_device > structure. This allows different set of callback to be used by > vfio-mdev and virtio-mdev. > > Reviewed-by: Parav Pandit > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 35 + > MAINTAINERS | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > drivers/vfio/mdev/mdev_core.c | 24 - > drivers/vfio/mdev/mdev_private.h | 5 ++ > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > include/linux/mdev.h | 43 --- > include/linux/mdev_vfio_ops.h | 52 +++ > samples/vfio-mdev/mbochs.c| 20 --- > samples/vfio-mdev/mdpy.c | 20 --- > samples/vfio-mdev/mtty.c | 18 --- > 13 files changed, 206 insertions(+), 99 deletions(-) > create mode 100644 include/linux/mdev_vfio_ops.h > (...) > @@ -172,10 +163,34 @@ that a driver should use to unregister itself with the > mdev core driver:: > > extern void mdev_unregister_device(struct device *dev); > > -It is also required to specify the class_id in create() callback through:: > +As multiple types of mediated devices may be supported, class id needs > +to be specified in the create callback(). This could be done The brackets should probably go behind 'create'? > +explicitly for the device that does not use on mdev bus for its "for devices that do not use the mdev bus" ? But why wouldn't they? I feel like I've missed some discussion here :/ > +operation through: > > int mdev_set_class(struct mdev_device *mdev, u16 id); > > +For the device that uses on the mdev bus for its operation, the class "For devices that use the mdev bus..." But same comment as above. > +should provide helper function to set class id and device specific > +ops. E.g for vfio-mdev devices, the function to be called is:: > + > + int mdev_set_vfio_ops(struct mdev_device *mdev, > + const struct mdev_vfio_device_ops *vfio_ops); > + > +The class id (set by this function to MDEV_CLASS_ID_VFIO) is used to > +match a device with an mdev driver via its id table. The device > +specific callbacks (specified in *vfio_ops) are obtainable via > +mdev_get_vfio_ops() (for use by the mdev bus driver). A vfio-mdev > +device (class id MDEV_CLASS_ID_VFIO) uses the following > +device-specific ops: > + > +* open: open callback of vfio mediated device > +* close: close callback of vfio mediated device > +* ioctl: ioctl callback of vfio mediated device > +* read : read emulation callback > +* write: write emulation callback > +* mmap: mmap emulation callback > + > Mediated Device Management Interface Through sysfs > == Otherwise, looks good. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 2/6] modpost: add support for mdev class id
On Tue, 5 Nov 2019 17:32:36 +0800 Jason Wang wrote: > Add support to parse mdev class id table. > > Reviewed-by: Parav Pandit > Signed-off-by: Jason Wang > --- > drivers/vfio/mdev/vfio_mdev.c | 2 ++ > scripts/mod/devicetable-offsets.c | 3 +++ > scripts/mod/file2alias.c | 11 +++ > 3 files changed, 16 insertions(+) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V8 1/6] mdev: class id support
On Tue, 5 Nov 2019 17:32:35 +0800 Jason Wang wrote: > Mdev bus only supports vfio driver right now, so it doesn't implement > match method. But in the future, we may add drivers other than vfio, > the first driver could be virtio-mdev. This means we need to add > device class id support in bus match method to pair the mdev device > and mdev driver correctly. > > So this patch adds id_table to mdev_driver and class_id for mdev > device with the match method for mdev bus. > > Reviewed-by: Parav Pandit > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 5 > drivers/gpu/drm/i915/gvt/kvmgt.c | 1 + > drivers/s390/cio/vfio_ccw_ops.c | 1 + > drivers/s390/crypto/vfio_ap_ops.c | 1 + > drivers/vfio/mdev/mdev_core.c | 17 + > drivers/vfio/mdev/mdev_driver.c | 25 +++ > drivers/vfio/mdev/mdev_private.h | 1 + > drivers/vfio/mdev/vfio_mdev.c | 6 + > include/linux/mdev.h | 8 ++ > include/linux/mod_devicetable.h | 8 ++ > samples/vfio-mdev/mbochs.c| 1 + > samples/vfio-mdev/mdpy.c | 1 + > samples/vfio-mdev/mtty.c | 1 + > 13 files changed, 76 insertions(+) Reviewed-by: Cornelia Huck ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 5/6] virtio: introduce a mdev based transport
On Mon, 21 Oct 2019 13:59:23 +0800
Jason Wang wrote:
> On 2019/10/18 下午10:20, Cornelia Huck wrote:
> > On Thu, 17 Oct 2019 18:48:35 +0800
> > Jason Wang wrote:
> >
> >> This patch introduces a new mdev transport for virtio. This is used to
> >> use kernel virtio driver to drive the mediated device that is capable
> >> of populating virtqueue directly.
> >>
> >> A new virtio-mdev driver will be registered to the mdev bus, when a
> >> new virtio-mdev device is probed, it will register the device with
> >> mdev based config ops. This means it is a software transport between
> >> mdev driver and mdev device. The transport was implemented through
> >> device specific ops which is a part of mdev_parent_ops now.
> >>
> >> Signed-off-by: Jason Wang
> >> ---
> >> drivers/virtio/Kconfig | 7 +
> >> drivers/virtio/Makefile | 1 +
> >> drivers/virtio/virtio_mdev.c | 409 +++
> >> 3 files changed, 417 insertions(+)
> > (...)
> >
> >> +static int virtio_mdev_probe(struct device *dev)
> >> +{
> >> + struct mdev_device *mdev = mdev_from_dev(dev);
> >> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> >> + struct virtio_mdev_device *vm_dev;
> >> + int rc;
> >> +
> >> + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL);
> >> + if (!vm_dev)
> >> + return -ENOMEM;
> >> +
> >> + vm_dev->vdev.dev.parent = dev;
> >> + vm_dev->vdev.dev.release = virtio_mdev_release_dev;
> >> + vm_dev->vdev.config = _mdev_config_ops;
> >> + vm_dev->mdev = mdev;
> >> + INIT_LIST_HEAD(_dev->virtqueues);
> >> + spin_lock_init(_dev->lock);
> >> +
> >> + vm_dev->version = ops->get_mdev_features(mdev);
> >> + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) {
> >> + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n");
> >> + return -ENXIO;
> >> + }
> > Hm, so how is that mdev features interface supposed to work? If
> > VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for
> > its presence, and not for identity.
>
>
> This should be used by driver to detect the which sets of functions and
> their semantics that could be provided by the device. E.g when driver
> support both version 2 and version 1 but device only support version 1,
> driver can switch to use version 1. Btw, Is there a easy way for to test
> its presence or do you mean doing sanity testing on existence of the
> mandatory ops that provided by the device?
What I meant was something like:
features = ops->get_mdev_features(mdev);
if (features & VIRTIO_MDEV_F_VERSION_1)
vm_dev->version = 1;
else
//moan about missing support for version 1
Can there be class id specific extra features, or is this only for core
features? If the latter, maybe also do something like
supported_features = ORED_LIST_OF_FEATURES;
if (features & ~supported_features)
//moan about extra feature bits
>
>
> >
> > What will happen if we come up with a version 2? If this is backwards
> > compatible, will both version 2 and version 1 be set?
>
>
> Yes, I think so, and version 2 should be considered as some extensions
> of version 1. If it's completely, it should use a new class id.
Ok, that makes sense.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 5/6] virtio: introduce a mdev based transport
On Thu, 17 Oct 2019 18:48:35 +0800
Jason Wang wrote:
> This patch introduces a new mdev transport for virtio. This is used to
> use kernel virtio driver to drive the mediated device that is capable
> of populating virtqueue directly.
>
> A new virtio-mdev driver will be registered to the mdev bus, when a
> new virtio-mdev device is probed, it will register the device with
> mdev based config ops. This means it is a software transport between
> mdev driver and mdev device. The transport was implemented through
> device specific ops which is a part of mdev_parent_ops now.
>
> Signed-off-by: Jason Wang
> ---
> drivers/virtio/Kconfig | 7 +
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio_mdev.c | 409 +++
> 3 files changed, 417 insertions(+)
(...)
> +static int virtio_mdev_probe(struct device *dev)
> +{
> + struct mdev_device *mdev = mdev_from_dev(dev);
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> + struct virtio_mdev_device *vm_dev;
> + int rc;
> +
> + vm_dev = devm_kzalloc(dev, sizeof(*vm_dev), GFP_KERNEL);
> + if (!vm_dev)
> + return -ENOMEM;
> +
> + vm_dev->vdev.dev.parent = dev;
> + vm_dev->vdev.dev.release = virtio_mdev_release_dev;
> + vm_dev->vdev.config = _mdev_config_ops;
> + vm_dev->mdev = mdev;
> + INIT_LIST_HEAD(_dev->virtqueues);
> + spin_lock_init(_dev->lock);
> +
> + vm_dev->version = ops->get_mdev_features(mdev);
> + if (vm_dev->version != VIRTIO_MDEV_F_VERSION_1) {
> + dev_err(dev, "VIRTIO_MDEV_F_VERSION_1 is mandatory\n");
> + return -ENXIO;
> + }
Hm, so how is that mdev features interface supposed to work? If
VIRTIO_MDEV_F_VERSION_1 is a bit, I would expect this code to test for
its presence, and not for identity.
What will happen if we come up with a version 2? If this is backwards
compatible, will both version 2 and version 1 be set?
> +
> + vm_dev->vdev.id.device = ops->get_device_id(mdev);
> + if (vm_dev->vdev.id.device == 0)
> + return -ENODEV;
> +
> + vm_dev->vdev.id.vendor = ops->get_vendor_id(mdev);
> + rc = register_virtio_device(_dev->vdev);
> + if (rc)
> + put_device(dev);
> + else
> + dev_set_drvdata(dev, vm_dev);
> +
> + return rc;
> +}
(...)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 4/6] mdev: introduce virtio device and its device ops
On Fri, 18 Oct 2019 18:55:02 +0800 Jason Wang wrote: > On 2019/10/18 下午5:46, Cornelia Huck wrote: > > On Thu, 17 Oct 2019 18:48:34 +0800 > > Jason Wang wrote: > >> + * @get_vendor_id:Get virtio vendor id > >> + *@mdev: mediated device > >> + *Returns u32: virtio vendor id > > How is the vendor id defined? As for normal virtio-pci devices? > > > The vendor that provides this device. So something like this > > I notice that MMIO also had this so it looks to me it's not pci specific. Ok. Would be good to specify this more explicitly. > > > > > >> + * @get_status: Get the device status > >> + *@mdev: mediated device > >> + *Returns u8: virtio device status > >> + * @set_status: Set the device status > >> + *@mdev: mediated device > >> + *@status: virtio device status > >> + * @get_config: Read from device specific configuration space > >> + *@mdev: mediated device > >> + *@offset: offset from the beginning of > >> + *configuration space > >> + *@buf: buffer used to read to > >> + *@len: the length to read from > >> + *configration space > >> + * @set_config: Write to device specific configuration space > >> + *@mdev: mediated device > >> + *@offset: offset from the beginning of > >> + *configuration space > >> + *@buf: buffer used to write from > >> + *@len: the length to write to > >> + *configration space > >> + * @get_mdev_features:Get the feature of virtio mdev device > >> + *@mdev: mediated device > >> + *Returns the mdev features (API) support > >> by > >> + *the device. > > What kind of 'features' are supposed to go in there? Are these bits, > > like you defined for VIRTIO_MDEV_F_VERSION_1 above? > > > It's the API or mdev features other than virtio features. It could be > used by driver to determine the capability of the mdev device. Besides > _F_VERSION_1, we may add dirty page tracking etc which means we need new > device ops. Ok, so that's supposed to be distinct bits that can be or'ed together? Makes sense, but probably needs some more documentation somewhere. > > > > > >> + * @get_generation: Get device generaton > >> + *@mdev: mediated device > >> + *Returns u32: device generation > > Is that callback mandatory? > > > I think so, it's hard to emulate that completely in virtio-mdev transport. IIRC, the generation stuff is not mandatory in the current version of virtio, as not all transports have that concept. Generally, are any of the callbacks optional, or are all of them mandatory? From what I understand, you plan to add new things that depend on features... would that mean non-mandatory callbacks? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 4/6] mdev: introduce virtio device and its device ops
On Thu, 17 Oct 2019 18:48:34 +0800
Jason Wang wrote:
> This patch implements basic support for mdev driver that supports
> virtio transport for kernel virtio driver.
>
> Signed-off-by: Jason Wang
> ---
> drivers/vfio/mdev/mdev_core.c | 12 +++
> include/linux/mdev.h | 4 +
> include/linux/virtio_mdev.h | 151 ++
> 3 files changed, 167 insertions(+)
> create mode 100644 include/linux/virtio_mdev.h
>
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index d0f3113c8071..5834f6b7c7a5 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -57,6 +57,18 @@ void mdev_set_vfio_ops(struct mdev_device *mdev,
> }
> EXPORT_SYMBOL(mdev_set_vfio_ops);
>
> +/* Specify the virtio device ops for the mdev device, this
> + * must be called during create() callback for virtio mdev device.
> + */
Change this as for the vfio comment (last patch?)
> +void mdev_set_virtio_ops(struct mdev_device *mdev,
> + const struct virtio_mdev_device_ops *virtio_ops)
> +{
> + BUG_ON(mdev->class_id);
s/BUG_ON/WARN_ON/
> + mdev->class_id = MDEV_CLASS_ID_VIRTIO;
> + mdev->device_ops = virtio_ops;
> +}
> +EXPORT_SYMBOL(mdev_set_virtio_ops);
> +
> const void *mdev_get_dev_ops(struct mdev_device *mdev)
> {
> return mdev->device_ops;
(...)
> diff --git a/include/linux/virtio_mdev.h b/include/linux/virtio_mdev.h
> new file mode 100644
> index ..b965b50f9b24
> --- /dev/null
> +++ b/include/linux/virtio_mdev.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Virtio mediated device driver
> + *
> + * Copyright 2019, Red Hat Corp.
> + * Author: Jason Wang
> + */
> +#ifndef _LINUX_VIRTIO_MDEV_H
> +#define _LINUX_VIRTIO_MDEV_H
> +
> +#include
> +#include
> +#include
> +
> +#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 to virtio-mdev module.
> + *
> + * @set_vq_address: Set the address of virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @desc_area: address of desc area
> + * @driver_area: address of driver area
> + * @device_area: address of device area
> + * Returns integer: success (0) or error (< 0)
Probably dumb question (have not read the next patches yet): Is this
agnostic regarding split or packed queues?
> + * @set_vq_num: Set the size of virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @num: the size of virtqueue
> + * @kick_vq: Kick the virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @set_vq_cb: Set the interrupt callback function for
> + * a virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @cb: virtio-mdev interrupt callback structure
> + * @set_vq_ready:Set ready status for a virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @ready: ready (true) not ready(false)
> + * @get_vq_ready:Get ready status for a virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * Returns boolean: ready (true) or not (false)
> + * @set_vq_state:Set the state for a virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * @state: virtqueue state (last_avail_idx)
> + * Returns integer: success (0) or error (< 0)
> + * @get_vq_state:Get the state for a virtqueue
> + * @mdev: mediated device
> + * @idx: virtqueue index
> + * Returns virtqueue state (last_avail_idx)
> + * @get_vq_align:Get the virtqueue align requirement
> + * for the device
> + * @mdev: mediated device
> + * Returns virtqueue algin requirement
> + * @get_features:Get virtio features supported by the device
> + * @mdev: mediated device
> + * Returns the virtio features support by the
Re: [Intel-gfx] [PATCH V4 3/6] mdev: introduce device specific ops
On Thu, 17 Oct 2019 11:53:10 -0600 Alex Williamson wrote: > On Thu, 17 Oct 2019 17:07:55 +0200 > Cornelia Huck wrote: > > > On Thu, 17 Oct 2019 18:48:33 +0800 > > Jason Wang wrote: > > > > > Currently, except for the create and remove, the rest of > > > mdev_parent_ops is designed for vfio-mdev driver only and may not help > > > for kernel mdev driver. With the help of class id, this patch > > > introduces device specific callbacks inside mdev_device > > > structure. This allows different set of callback to be used by > > > vfio-mdev and virtio-mdev. > > > > > > Signed-off-by: Jason Wang > > > --- > > > .../driver-api/vfio-mediated-device.rst | 25 + > > > MAINTAINERS | 1 + > > > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > > > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > > > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > > > drivers/vfio/mdev/mdev_core.c | 18 +-- > > > drivers/vfio/mdev/mdev_private.h | 1 + > > > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > > > include/linux/mdev.h | 45 > > > include/linux/vfio_mdev.h | 52 +++ > > > samples/vfio-mdev/mbochs.c| 20 --- > > > samples/vfio-mdev/mdpy.c | 20 --- > > > samples/vfio-mdev/mtty.c | 18 --- > > > 13 files changed, 184 insertions(+), 103 deletions(-) > > > create mode 100644 include/linux/vfio_mdev.h > > > > > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst > > > b/Documentation/driver-api/vfio-mediated-device.rst > > > index f9a78d75a67a..0cca84d19603 100644 > > > --- a/Documentation/driver-api/vfio-mediated-device.rst > > > +++ b/Documentation/driver-api/vfio-mediated-device.rst > > > @@ -152,11 +152,22 @@ callbacks per mdev parent device, per mdev type, or > > > any other categorization. > > > Vendor drivers are expected to be fully asynchronous in this respect or > > > provide their own internal resource protection.) > > > > > > -The callbacks in the mdev_parent_ops structure are as follows: > > > - > > > -* open: open callback of mediated device > > > -* close: close callback of mediated device > > > -* ioctl: ioctl callback of mediated device > > > +As multiple types of mediated devices may be supported, the device > > > +must set up the class id and the device specific callbacks in create() > > > > > > > s/in create()/in the create()/ > > > > > +callback. E.g for vfio-mdev device it needs to be done through: > > > > "Each class provides a helper function to do so; e.g. for vfio-mdev > > devices, the function to be called is:" > > > > ? > > > > > + > > > +int mdev_set_vfio_ops(struct mdev_device *mdev, > > > + const struct vfio_mdev_ops *vfio_ops); > > > + > > > +The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device > > > > "(set by this helper function to MDEV_CLASS_ID_VFIO)" ? > > > > > +with an mdev driver via its id table. The device specific callbacks > > > +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by > > > > "(specified in *vfio_ops by the caller)" ? > > > > > +the mdev bus driver). A vfio-mdev device (class id MDEV_CLASS_ID_VFIO) > > > +uses the following device-specific ops: > > > + > > > +* open: open callback of vfio mediated device > > > +* close: close callback of vfio mediated device > > > +* ioctl: ioctl callback of vfio mediated device > > > * read : read emulation callback > > > * write: write emulation callback > > > * mmap: mmap emulation callback > > > @@ -167,10 +178,6 @@ register itself with the mdev core driver:: > > > extern int mdev_register_device(struct device *dev, > > >const struct mdev_parent_ops *ops); > > > > > > -It is also required to specify the class_id in create() callback > > > through:: > > > - > > > - int mdev_set_class(struct mdev_device *mdev, u16 id); > > > - > > > > I'm wondering if this patch set should start out with introducing > > helper functions already (i.e. don't introduce mdev_set_class(), but > > start out with mdev_set_class_vfio() which will gain the *vfio_ops > > argument in this patch.) > > Yes, it would be cleaner, but is it really worth the churn? Correct me > if I'm wrong, but I think we get to the same point after this patch and > aside from the function name itself, the difference is really just that > the class_id is briefly exposed to the parent driver, right? Thanks, Yes, it does not really matter much. If there's no other reason to rework things, I'd just keep this as it is now. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4 3/6] mdev: introduce device specific ops
On Thu, 17 Oct 2019 18:48:33 +0800
Jason Wang wrote:
> Currently, except for the create and remove, the rest of
> mdev_parent_ops is designed for vfio-mdev driver only and may not help
> for kernel mdev driver. With the help of class id, this patch
> introduces device specific callbacks inside mdev_device
> structure. This allows different set of callback to be used by
> vfio-mdev and virtio-mdev.
>
> Signed-off-by: Jason Wang
> ---
> .../driver-api/vfio-mediated-device.rst | 25 +
> MAINTAINERS | 1 +
> drivers/gpu/drm/i915/gvt/kvmgt.c | 18 ---
> drivers/s390/cio/vfio_ccw_ops.c | 18 ---
> drivers/s390/crypto/vfio_ap_ops.c | 14 +++--
> drivers/vfio/mdev/mdev_core.c | 18 +--
> drivers/vfio/mdev/mdev_private.h | 1 +
> drivers/vfio/mdev/vfio_mdev.c | 37 ++---
> include/linux/mdev.h | 45
> include/linux/vfio_mdev.h | 52 +++
> samples/vfio-mdev/mbochs.c| 20 ---
> samples/vfio-mdev/mdpy.c | 20 ---
> samples/vfio-mdev/mtty.c | 18 ---
> 13 files changed, 184 insertions(+), 103 deletions(-)
> create mode 100644 include/linux/vfio_mdev.h
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index f9a78d75a67a..0cca84d19603 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -152,11 +152,22 @@ callbacks per mdev parent device, per mdev type, or any
> other categorization.
> Vendor drivers are expected to be fully asynchronous in this respect or
> provide their own internal resource protection.)
>
> -The callbacks in the mdev_parent_ops structure are as follows:
> -
> -* open: open callback of mediated device
> -* close: close callback of mediated device
> -* ioctl: ioctl callback of mediated device
> +As multiple types of mediated devices may be supported, the device
> +must set up the class id and the device specific callbacks in create()
s/in create()/in the create()/
> +callback. E.g for vfio-mdev device it needs to be done through:
"Each class provides a helper function to do so; e.g. for vfio-mdev
devices, the function to be called is:"
?
> +
> +int mdev_set_vfio_ops(struct mdev_device *mdev,
> + const struct vfio_mdev_ops *vfio_ops);
> +
> +The class id (set to MDEV_CLASS_ID_VFIO) is used to match a device
"(set by this helper function to MDEV_CLASS_ID_VFIO)" ?
> +with an mdev driver via its id table. The device specific callbacks
> +(specified in *ops) are obtainable via mdev_get_dev_ops() (for use by
"(specified in *vfio_ops by the caller)" ?
> +the mdev bus driver). A vfio-mdev device (class id MDEV_CLASS_ID_VFIO)
> +uses the following device-specific ops:
> +
> +* open: open callback of vfio mediated device
> +* close: close callback of vfio mediated device
> +* ioctl: ioctl callback of vfio mediated device
> * read : read emulation callback
> * write: write emulation callback
> * mmap: mmap emulation callback
> @@ -167,10 +178,6 @@ register itself with the mdev core driver::
> extern int mdev_register_device(struct device *dev,
>const struct mdev_parent_ops *ops);
>
> -It is also required to specify the class_id in create() callback through::
> -
> - int mdev_set_class(struct mdev_device *mdev, u16 id);
> -
I'm wondering if this patch set should start out with introducing
helper functions already (i.e. don't introduce mdev_set_class(), but
start out with mdev_set_class_vfio() which will gain the *vfio_ops
argument in this patch.)
> However, the mdev_parent_ops structure is not required in the function call
> that a driver should use to unregister itself with the mdev core driver::
>
(...)
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index 3a9c52d71b4e..d0f3113c8071 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -45,15 +45,23 @@ void mdev_set_drvdata(struct mdev_device *mdev, void
> *data)
> }
> EXPORT_SYMBOL(mdev_set_drvdata);
>
> -/* Specify the class for the mdev device, this must be called during
> - * create() callback.
> +/* Specify the VFIO device ops for the mdev device, this
> + * must be called during create() callback for VFIO mdev device.
> */
/*
* Specify the mdev device to be a VFIO mdev device, and set the
* VFIO devices ops for it. This must be called from the create()
* callback for VFIO mdev devices.
*/
?
> -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)
> {
> WARN_ON(mdev->class_id);
> -
Re: [Intel-gfx] [PATCH V3 4/7] mdev: introduce device specific ops
On Thu, 17 Oct 2019 16:30:43 +0800
Jason Wang wrote:
> On 2019/10/17 上午12:53, Alex Williamson wrote:
> >>> Yet another suggestion: have the class id derive from the function
> >>> you use to set up the ops.
> >>>
> >>> void mdev_set_vfio_ops(struct mdev_device *mdev, const struct
> >>> vfio_mdev_ops *vfio_ops) {
> >>> mdev->device_ops = vfio_ops;
> >>> mdev->class_id = MDEV_ID_VFIO;
> >>> }
> >>>
> >>> void mdev_set_virtio_ops(struct mdev_device *mdev, const struct
> >>> virtio_mdev_ops *virtio_ops) {
> >>> mdev->device_ops = virtio_ops;
> >>> mdev->class_id = MDEV_ID_VIRTIO;
> >>> }
> >>>
> >>> void mdev_set_vhost_ops(struct mdev_device *mdev, const struct
> >>> virtio_mdev_ops *virtio_ops) {
> >>> mdev->device_ops = virtio_ops;
> >>> mdev->class_id = MDEV_ID_VHOST;
> >>> }
> >>>
> >>> void mdev_set_vendor_ops(struct mdev_device *mdev) /* no ops */ {
> >>> mdev->class_id = MDEV_ID_VENDOR;
> >>> }
> > One further step towards making this hard to use incorrectly might be
> > to return an error if class_id is already set. Thanks,
> >
> > Alex
>
>
> I will add a BUG_ON() when class_id has already set.
Probably better a WARN_ON()?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 4/7] mdev: introduce device specific ops
On Wed, 16 Oct 2019 05:50:08 +
Parav Pandit wrote:
> Hi Alex,
>
> > -Original Message-
> > From: Alex Williamson
> > Sent: Tuesday, October 15, 2019 12:27 PM
> > To: Jason Wang
> > Cc: Cornelia Huck ; k...@vger.kernel.org; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org; dri-
> > de...@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-gvt-
> > d...@lists.freedesktop.org; kwankh...@nvidia.com; m...@redhat.com;
> > tiwei@intel.com; virtualizat...@lists.linux-foundation.org;
> > net...@vger.kernel.org; maxime.coque...@redhat.com;
> > cunming.li...@intel.com; zhihong.w...@intel.com;
> > rob.mil...@broadcom.com; xiao.w.w...@intel.com;
> > haotian.w...@sifive.com; zhen...@linux.intel.com; zhi.a.w...@intel.com;
> > jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com;
> > rodrigo.v...@intel.com; airl...@linux.ie; dan...@ffwll.ch;
> > far...@linux.ibm.com; pa...@linux.ibm.com; seb...@linux.ibm.com;
> > ober...@linux.ibm.com; heiko.carst...@de.ibm.com; g...@linux.ibm.com;
> > borntrae...@de.ibm.com; akrow...@linux.ibm.com; fre...@linux.ibm.com;
> > lingshan@intel.com; Ido Shamay ;
> > epere...@redhat.com; l...@redhat.com; Parav Pandit
> > ; christophe.de.dinec...@gmail.com;
> > kevin.t...@intel.com
> > Subject: Re: [PATCH V3 4/7] mdev: introduce device specific ops
> >
> > On Tue, 15 Oct 2019 20:17:01 +0800
> > Jason Wang wrote:
> >
> > > On 2019/10/15 下午6:41, Cornelia Huck wrote:
> > > > On Fri, 11 Oct 2019 16:15:54 +0800
> > > > Jason Wang wrote:
> > > >> @@ -167,9 +176,10 @@ register itself with the mdev core driver::
> > > >>extern int mdev_register_device(struct device *dev,
> > > >> const struct mdev_parent_ops
> > > >> *ops);
> > > >>
> > > >> -It is also required to specify the class_id through::
> > > >> +It is also required to specify the class_id and device specific ops
> > through::
> > > >>
> > > >> - extern int mdev_set_class(struct device *dev, u16 id);
> > > >> + extern int mdev_set_class(struct device *dev, u16 id,
> > > >> +const void *ops);
> > > > Apologies if that has already been discussed, but do we want a 1:1
> > > > relationship between id and ops, or can different devices with the
> > > > same id register different ops?
> > >
> > >
> > > I think we have a N:1 mapping between id and ops, e.g we want both
> > > virtio-mdev and vhost-mdev use a single set of device ops.
> >
> > The contents of the ops structure is essentially defined by the id, which is
> > why I was leaning towards them being defined together. They are effectively
> > interlocked, the id defines which mdev "endpoint"
> > driver is loaded and that driver requires mdev_get_dev_ops() to return the
> > structure required by the driver. I wish there was a way we could
> > incorporate type checking here. We toyed with the idea of having the class
> > in the same structure as the ops, but I think this approach was chosen for
> > simplicity. We could still do something like:
> >
> > int mdev_set_class_struct(struct device *dev, const struct mdev_class_struct
> > *class);
> >
> > struct mdev_class_struct {
> > u16 id;
> > union {
> > struct vfio_mdev_ops vfio_ops;
> > struct virtio_mdev_ops virtio_ops;
> > };
> > };
> >
> > Maybe even:
> >
> > struct vfio_mdev_ops *mdev_get_vfio_ops(struct mdev_device *mdev) {
> > BUG_ON(mdev->class.id != MDEV_ID_VFIO);
> > return >class.vfio_ops;
> > }
> >
> > The match callback would of course just use the mdev->class.id value.
> > Functionally equivalent, but maybe better type characteristics. Thanks,
> >
> > Alex
>
> We have 3 use cases of mdev.
> 1. current mdev binding to vfio_mdev
> 2. mdev binding to virtio
> 3. mdev binding to mlx5_core without dev_ops
>
> Also
> (a) a given parent may serve multiple types of classes in future.
> (b) number of classes may not likely explode, they will be handful of them.
> (vfio_mdev, virtio)
>
> So, instead of making copies of this dev_ops pointer in each mdev, it is
> better to keep const multiple ops in their parent device.
> Something like below,
>
> struct mdev_parent {
> [..]
> struct md
Re: [Intel-gfx] [PATCH V3 4/7] mdev: introduce device specific ops
On Fri, 11 Oct 2019 16:15:54 +0800 Jason Wang wrote: > Currently, except for the create and remove, the rest of > mdev_parent_ops is designed for vfio-mdev driver only and may not help > for kernel mdev driver. With the help of class id, this patch > introduces device specific callbacks inside mdev_device > structure. This allows different set of callback to be used by > vfio-mdev and virtio-mdev. > > Signed-off-by: Jason Wang > --- > .../driver-api/vfio-mediated-device.rst | 22 +--- > MAINTAINERS | 1 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 18 --- > drivers/s390/cio/vfio_ccw_ops.c | 18 --- > drivers/s390/crypto/vfio_ap_ops.c | 14 +++-- > drivers/vfio/mdev/mdev_core.c | 9 +++- > drivers/vfio/mdev/mdev_private.h | 1 + > drivers/vfio/mdev/vfio_mdev.c | 37 ++--- > include/linux/mdev.h | 42 +++ > include/linux/vfio_mdev.h | 52 +++ > samples/vfio-mdev/mbochs.c| 20 --- > samples/vfio-mdev/mdpy.c | 21 +--- > samples/vfio-mdev/mtty.c | 18 --- > 13 files changed, 177 insertions(+), 96 deletions(-) > create mode 100644 include/linux/vfio_mdev.h > > diff --git a/Documentation/driver-api/vfio-mediated-device.rst > b/Documentation/driver-api/vfio-mediated-device.rst > index 2035e48da7b2..553574ebba73 100644 > --- a/Documentation/driver-api/vfio-mediated-device.rst > +++ b/Documentation/driver-api/vfio-mediated-device.rst > @@ -152,11 +152,20 @@ callbacks per mdev parent device, per mdev type, or any > other categorization. > Vendor drivers are expected to be fully asynchronous in this respect or > provide their own internal resource protection.) > > -The callbacks in the mdev_parent_ops structure are as follows: > +In order to support multiple types of device/driver, device needs to > +provide both class_id and device_ops through: "As multiple types of mediated devices may be supported, the device needs to set up the class id and the device specific callbacks via:" ? > > -* open: open callback of mediated device > -* close: close callback of mediated device > -* ioctl: ioctl callback of mediated device > +void mdev_set_class(struct mdev_device *mdev, u16 id, const void *ops); > + > +The class_id is used to be paired with ids in id_table in mdev_driver > +structure for probing the correct driver. "The class id (specified in id) is used to match a device with an mdev driver via its id table." ? > The device_ops is device > +specific callbacks which can be get through mdev_get_dev_ops() > +function by mdev bus driver. "The device specific callbacks (specified in *ops) are obtainable via mdev_get_dev_ops() (for use by the mdev bus driver.)" ? > For vfio-mdev device, its device specific > +ops are as follows: "A vfio-mdev device (class id MDEV_ID_VFIO) uses the following device-specific ops:" ? > + > +* open: open callback of vfio mediated device > +* close: close callback of vfio mediated device > +* ioctl: ioctl callback of vfio mediated device > * read : read emulation callback > * write: write emulation callback > * mmap: mmap emulation callback > @@ -167,9 +176,10 @@ register itself with the mdev core driver:: > extern int mdev_register_device(struct device *dev, >const struct mdev_parent_ops *ops); > > -It is also required to specify the class_id through:: > +It is also required to specify the class_id and device specific ops through:: > > - extern int mdev_set_class(struct device *dev, u16 id); > + extern int mdev_set_class(struct device *dev, u16 id, > + const void *ops); Apologies if that has already been discussed, but do we want a 1:1 relationship between id and ops, or can different devices with the same id register different ops? If the former, would it make sense to first register the ops for an id (once), and then have the ->create callback only set the class id (with the core doing the lookup of the ops)? > > However, the mdev_parent_ops structure is not required in the function call > that a driver should use to unregister itself with the mdev core driver:: ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 2/7] mdev: bus uevent support
On Fri, 11 Oct 2019 16:15:52 +0800
Jason Wang wrote:
> This patch adds bus uevent support for mdev bus in order to allow
> cooperation with userspace.
>
> Signed-off-by: Jason Wang
> ---
> drivers/vfio/mdev/mdev_driver.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
> index b7c40ce86ee3..319d886ffaf7 100644
> --- a/drivers/vfio/mdev/mdev_driver.c
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -82,9 +82,17 @@ static int mdev_match(struct device *dev, struct
> device_driver *drv)
> return 0;
> }
>
> +static int mdev_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct mdev_device *mdev = to_mdev_device(dev);
> +
> + return add_uevent_var(env, "MODALIAS=mdev:c%02X", mdev->class_id);
> +}
> +
> struct bus_type mdev_bus_type = {
> .name = "mdev",
> .match = mdev_match,
> + .uevent = mdev_uevent,
> .probe = mdev_probe,
> .remove = mdev_remove,
> };
I'd merge that into the previous patch.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V3 1/7] mdev: class id support
On Fri, 11 Oct 2019 16:15:51 +0800
Jason Wang wrote:
> Mdev bus only supports vfio driver right now, so it doesn't implement
> match method. But in the future, we may add drivers other than vfio,
> the first driver could be virtio-mdev. This means we need to add
> device class id support in bus match method to pair the mdev device
> and mdev driver correctly.
>
> So this patch adds id_table to mdev_driver and class_id for mdev
> device with the match method for mdev bus.
>
> Signed-off-by: Jason Wang
> ---
> Documentation/driver-api/vfio-mediated-device.rst | 7 ++-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 1 +
> drivers/s390/cio/vfio_ccw_ops.c | 1 +
> drivers/s390/crypto/vfio_ap_ops.c | 1 +
> drivers/vfio/mdev/mdev_core.c | 11 +++
> drivers/vfio/mdev/mdev_driver.c | 14 ++
> drivers/vfio/mdev/mdev_private.h | 1 +
> drivers/vfio/mdev/vfio_mdev.c | 6 ++
> include/linux/mdev.h | 8
> include/linux/mod_devicetable.h | 8
> samples/vfio-mdev/mbochs.c| 1 +
> samples/vfio-mdev/mdpy.c | 1 +
> samples/vfio-mdev/mtty.c | 1 +
> 13 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> b/Documentation/driver-api/vfio-mediated-device.rst
> index 25eb7d5b834b..2035e48da7b2 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -102,12 +102,14 @@ structure to represent a mediated device's driver::
>* @probe: called when new device created
>* @remove: called when device removed
>* @driver: device driver structure
> + * @id_table: the ids serviced by this driver
>*/
> struct mdev_driver {
>const char *name;
>int (*probe) (struct device *dev);
>void (*remove) (struct device *dev);
>struct device_driverdriver;
> + const struct mdev_class_id *id_table;
> };
>
> A mediated bus driver for mdev should use this structure in the function
> calls
> @@ -165,12 +167,15 @@ register itself with the mdev core driver::
> extern int mdev_register_device(struct device *dev,
>const struct mdev_parent_ops *ops);
>
> +It is also required to specify the class_id through::
> +
> + extern int mdev_set_class(struct device *dev, u16 id);
Should the document state explicitly that this should be done in the
->create() callback? Also, I think that the class_id might be different
for different mdevs (even if the parent is the same) -- should that be
mentioned explicitly?
> +
> However, the mdev_parent_ops structure is not required in the function call
> that a driver should use to unregister itself with the mdev core driver::
>
> extern void mdev_unregister_device(struct device *dev);
>
> -
> Mediated Device Management Interface Through sysfs
> ==
>
(...)
Looks reasonable to me.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC PATCH 2/2] mdev: introduce device specific ops
On Thu, 12 Sep 2019 17:40:12 +0800
Jason Wang wrote:
> Currently, except for the crate and remove. The rest fields of
> mdev_parent_ops is just designed for vfio-mdev driver and may not help
> for kernel mdev driver. So follow the device id support by previous
> patch, this patch introduces device specific ops which points to
> device specific ops (e.g vfio ops). This allows the future drivers
> like virtio-mdev to implement its own device specific ops.
>
> Signed-off-by: Jason Wang
> ---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 14 +++---
> drivers/s390/cio/vfio_ccw_ops.c | 14 +++---
> drivers/s390/crypto/vfio_ap_ops.c | 10 +++--
> drivers/vfio/mdev/vfio_mdev.c | 30 +++--
> include/linux/mdev.h | 72 ++-
> samples/vfio-mdev/mbochs.c| 16 ---
> samples/vfio-mdev/mdpy.c | 16 ---
> samples/vfio-mdev/mtty.c | 14 +++---
> 8 files changed, 113 insertions(+), 73 deletions(-)
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index f85045392120..3b8a76bc69cf 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -27,27 +27,9 @@ int mdev_set_iommu_device(struct device *dev, struct
> device *iommu_device);
> struct device *mdev_get_iommu_device(struct device *dev);
>
> /**
> - * struct mdev_parent_ops - Structure to be registered for each parent
> device to
> - * register the device to mdev module.
> + * struct vfio_mdev_parent_ops - Structure to be registered for each
> + * parent device to register the device to vfio-mdev module.
> *
> - * @owner: The module owner.
> - * @dev_attr_groups: Attributes of the parent device.
> - * @mdev_attr_groups:Attributes of the mediated device.
> - * @supported_type_groups: Attributes to define supported types. It is
> mandatory
> - * to provide supported types.
> - * @create: Called to allocate basic resources in parent device's
> - * driver for a particular mediated device. It is
> - * mandatory to provide create ops.
> - * @kobj: kobject of type for which 'create' is called.
> - * @mdev: mdev_device structure on of mediated device
> - * that is being created
> - * Returns integer: success (0) or error (< 0)
> - * @remove: Called to free resources in parent device's driver for a
> - * a mediated device. It is mandatory to provide 'remove'
> - * ops.
> - * @mdev: mdev_device device structure which is being
> - * destroyed
> - * Returns integer: success (0) or error (< 0)
> * @open:Open mediated device.
> * @mdev: mediated device.
> * Returns integer: success (0) or error (< 0)
> @@ -72,6 +54,43 @@ struct device *mdev_get_iommu_device(struct device *dev);
> * @mmap:mmap callback
> * @mdev: mediated device structure
> * @vma: vma structure
> + */
> +struct vfio_mdev_parent_ops {
> + int (*open)(struct mdev_device *mdev);
> + void(*release)(struct mdev_device *mdev);
> + ssize_t (*read)(struct mdev_device *mdev, char __user *buf,
> + size_t count, loff_t *ppos);
> + ssize_t (*write)(struct mdev_device *mdev, const char __user *buf,
> + size_t count, loff_t *ppos);
> + long(*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> + unsigned long arg);
> + int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> +};
> +
> +/**
> + * struct mdev_parent_ops - Structure to be registered for each parent
> device to
> + * register the device to mdev module.
> + *
> + * @owner: The module owner.
> + * @dev_attr_groups: Attributes of the parent device.
> + * @mdev_attr_groups:Attributes of the mediated device.
> + * @supported_type_groups: Attributes to define supported types. It is
> mandatory
> + * to provide supported types.
> + * @create: Called to allocate basic resources in parent device's
> + * driver for a particular mediated device. It is
> + * mandatory to provide create ops.
> + * @kobj: kobject of type for which 'create' is called.
> + * @mdev: mdev_device structure on of mediated device
> + * that is being created
> + * Returns integer: success (0) or error (< 0)
> + * @remove: Called to free resources in parent device's driver for a
> + * a mediated device. It is mandatory to provide 'remove'
> + * ops.
> + * @mdev: mdev_device device structure which is being
> + * destroyed
> + * Returns integer: success (0)
Re: [Intel-gfx] [RFC PATCH 1/2] mdev: device id support
On Thu, 12 Sep 2019 17:40:11 +0800
Jason Wang wrote:
> Mdev bus only support vfio driver right now, so it doesn't implement
> match method. But in the future, we may add drivers other than vfio,
> one example is virtio-mdev[1] driver. This means we need to add device
> id support in bus match method to pair the mdev device and mdev driver
> correctly.
Sounds reasonable.
>
> So this patch add id_table to mdev_driver and id for mdev parent, and
> implement the match method for mdev bus.
>
> [1] https://lkml.org/lkml/2019/9/10/135
>
> Signed-off-by: Jason Wang
> ---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 2 +-
> drivers/s390/cio/vfio_ccw_ops.c | 2 +-
> drivers/s390/crypto/vfio_ap_ops.c | 3 ++-
> drivers/vfio/mdev/mdev_core.c | 14 --
> drivers/vfio/mdev/mdev_driver.c | 14 ++
> drivers/vfio/mdev/mdev_private.h | 1 +
> drivers/vfio/mdev/vfio_mdev.c | 6 ++
> include/linux/mdev.h | 6 +-
> include/linux/mod_devicetable.h | 6 ++
> samples/vfio-mdev/mbochs.c| 2 +-
> samples/vfio-mdev/mdpy.c | 2 +-
> samples/vfio-mdev/mtty.c | 2 +-
> 12 files changed, 51 insertions(+), 9 deletions(-)
(...)
The transformations of the vendor drivers and the new interface look
sane.
(...)
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 5714fd35a83c..f1fc143df042 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -821,4 +821,10 @@ struct wmi_device_id {
> const void *context;
> };
>
> +/* MDEV */
> +
Maybe add some kerneldoc and give vfio as an example of what we're
matching here?
> +struct mdev_device_id {
> + __u8 id;
I agree with the suggestion to rename this to 'class_id'.
> +};
> +
> #endif /* LINUX_MOD_DEVICETABLE_H */
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
