Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/18/19 11:57 AM, Cornelia Huck wrote:
> On Mon, 18 Feb 2019 11:35:45 -0500
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > On 2/18/19 7:01 AM, Cornelia Huck wrote:
> > > On Fri, 15 Feb 2019 16:59:33 -0500
> > > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> > >    
> > > > On 2/15/19 4:11 AM, Cornelia Huck wrote:
> > > > > On Thu, 14 Feb 2019 13:30:59 -0500
> > > > > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> > > > >       
> > > > > > On 2/14/19 12:36 PM, Pierre Morel wrote:
> > > > > > > On 14/02/2019 17:57, Cornelia Huck wrote:
> > >    
> > > > > > > > (And reading further in the current code, it seems we clear that
> > > > > > > > structure _after_ the matrix device had been setup, so how can that
> > > > > > > > even work? Where am I confused?)
> > > > > > >
> > > > > > > On device_register there were no bus, so the core just do not look for a
> > > > > > > driver and this field was nor tested nor overwritten.
> > > > >
> > > > > Hm... so has the callback in driver_for_each_device() in
> > > > > vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> > > > > patch fixes more than just libudev issues...
> > > >
> > > > It is this patch that rendered the driver_for_each_device() in
> > > > vfio_ap_verify_queue_reserved() erroneous. That function gets called
> > > > every time an adapter or domain is assigned to the mdev. This patch
> > > > introduced the problem with driver_for_each_device().
> > >
> > > So, does this function need to be removed or called from another place,
> > > then? (It looks like it was dead code before.)
> >
> > I don't see why you think it's dead code:
> >
> > assign_adapter_store
> > ==> vfio_ap_mdev_verify_queues_reserved_for_apid
> >       ==> vfio_ap_verify_queue_reserved
> >           ==> driver_for_each_device
> >
> > The only way that the vfio_ap_verify_queue_reserved - the function that
> > calls driver_for_each_device - does not get called is if no bits have
> > yet been set in matrix_mdev->matrix.aqm.
>
> What I don't see is how this can be called if no device has been, in
> fact, bound to the driver in the driver core...

Let's start with the fact that one can create an mdev device regardless
of whether a queue has been bound to the vfio_ap driver. Once an mdev
device is created, one can start assigning adapters, domains and control
domains to it. Let's say the admin now attempts to assign an adapter, in
which case the assign_adapter_store() function is invoked. After
verifying that the APID passed in is a valid adapter number, the
vfio_ap_mdev_verify_queues_reserved_for_apid() function is called.
This function first checks if any domains have been assigned and if not,
calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function
that the driver_for_each_device() function is called. Since there are
no devices bound to the vfio_ap device driver, the callback passed in to
the driver_for_each_device() function will never get called, so the
vfio_ap_mdev_verify_queues_reserved_for_apid() function will return
-EADDRNOTAVAIL. A similar flow will occur if the first assignment is for
a domain. The bottom line is, the driver_for_each_device() function is
called every time an adapter or domain is assigned.

>