Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-14 Thread Tony Krowiak 

On 06/13/2018 08:16 AM, Pierre Morel wrote:

On 13/06/2018 14:12, Cornelia Huck wrote:

On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:


On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_matrix_dev_create(void)
+{
+int ret;
+
+vfio_ap_root_device = 
root_device_register(VFIO_AP_ROOT_NAME);

+
+if (IS_ERR(vfio_ap_root_device)) {
+ret = PTR_ERR(vfio_ap_root_device);
+goto done;
+}
+
+ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+if (!ap_matrix) {
+ret = -ENOMEM;
+goto matrix_alloc_err;
+}
+
+ap_matrix->device.type = _ap_dev_type;
+dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+ap_matrix->device.parent = vfio_ap_root_device;
+ap_matrix->device.release = vfio_ap_matrix_dev_release;
+ap_matrix->device.driver = _ap_drv.driver;
+
+ret = device_register(_matrix->device);
+if (ret)
+goto matrix_reg_err;
+
+goto done;
+
+matrix_reg_err:
+put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.
No, this must not be a kfree. Once you've tried to register 
something

embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for 
device_register().

IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is
embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.


yes right, thanks.


See the vfio_ap_matrix_dev_release() callback.







+
+matrix_alloc_err:
+root_device_unregister(vfio_ap_root_device);
+
+done:
+return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix 
*ap_matrix)

+{
+device_unregister(_matrix->device);
+root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-14 Thread Tony Krowiak 

On 06/13/2018 08:16 AM, Pierre Morel wrote:

On 13/06/2018 14:12, Cornelia Huck wrote:

On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:


On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_matrix_dev_create(void)
+{
+int ret;
+
+vfio_ap_root_device = 
root_device_register(VFIO_AP_ROOT_NAME);

+
+if (IS_ERR(vfio_ap_root_device)) {
+ret = PTR_ERR(vfio_ap_root_device);
+goto done;
+}
+
+ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+if (!ap_matrix) {
+ret = -ENOMEM;
+goto matrix_alloc_err;
+}
+
+ap_matrix->device.type = _ap_dev_type;
+dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+ap_matrix->device.parent = vfio_ap_root_device;
+ap_matrix->device.release = vfio_ap_matrix_dev_release;
+ap_matrix->device.driver = _ap_drv.driver;
+
+ret = device_register(_matrix->device);
+if (ret)
+goto matrix_reg_err;
+
+goto done;
+
+matrix_reg_err:
+put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.
No, this must not be a kfree. Once you've tried to register 
something

embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for 
device_register().

IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is
embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.


yes right, thanks.


See the vfio_ap_matrix_dev_release() callback.







+
+matrix_alloc_err:
+root_device_unregister(vfio_ap_root_device);
+
+done:
+return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix 
*ap_matrix)

+{
+device_unregister(_matrix->device);
+root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 14:12, Cornelia Huck wrote:

On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:


On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:
  

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:
 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...
 

+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is
embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.


yes right, thanks.



  
 
 

+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.
 




--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 14:12, Cornelia Huck wrote:

On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:


On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:
  

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:
 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...
 

+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is
embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.


yes right, thanks.



  
 
 

+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.
 




--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:

> On 13/06/2018 13:14, Cornelia Huck wrote:
> > On Wed, 13 Jun 2018 12:54:40 +0200
> > Pierre Morel  wrote:
> >  
> >> On 13/06/2018 09:48, Cornelia Huck wrote:  
> >>> On Wed, 13 Jun 2018 09:41:16 +0200
> >>> Pierre Morel  wrote:
> >>> 
>  On 07/05/2018 17:11, Tony Krowiak wrote:  
> > Introduces a new AP device driver. This device driver
> > is built on the VFIO mediated device framework. The framework
> > provides sysfs interfaces that facilitate passthrough
> > access by guests to devices installed on the linux host.  
>  ...snip...
>  
> > +static int vfio_ap_matrix_dev_create(void)
> > +{
> > +   int ret;
> > +
> > +   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> > +
> > +   if (IS_ERR(vfio_ap_root_device)) {
> > +   ret = PTR_ERR(vfio_ap_root_device);
> > +   goto done;
> > +   }
> > +
> > +   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> > +   if (!ap_matrix) {
> > +   ret = -ENOMEM;
> > +   goto matrix_alloc_err;
> > +   }
> > +
> > +   ap_matrix->device.type = _ap_dev_type;
> > +   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> > +   ap_matrix->device.parent = vfio_ap_root_device;
> > +   ap_matrix->device.release = vfio_ap_matrix_dev_release;
> > +   ap_matrix->device.driver = _ap_drv.driver;
> > +
> > +   ret = device_register(_matrix->device);
> > +   if (ret)
> > +   goto matrix_reg_err;
> > +
> > +   goto done;
> > +
> > +matrix_reg_err:
> > +   put_device(_matrix->device);  
>  Did not see this before but here you certainly want to
>  do a kfree and not a put_device.  
> >>> No, this must not be a kfree. Once you've tried to register something
> >>> embedding a struct device with the driver core, you need to use
> >>> put_device, as another path may have acquired a reference, even if
> >>> registering ultimately failed. See the comment for device_register().
> >>> IOW, the code is correct.  
> >> learned something again :) ,
> >> but still, a kfree is needed for the kzalloc.
> >> Does'nt it?  
> > No, the put callback for the embedding structure needs to take care of
> > freeing things. Otherwise it is buggy.  
> 
> Seems buggy to me.
> How does the put_device knows if it is embedded and then in what it is 
> embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.

> 
> >  
> >>> 
>  
> > +
> > +matrix_alloc_err:
> > +   root_device_unregister(vfio_ap_root_device);
> > +
> > +done:
> > +   return ret;
> > +}
> > +
> > +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> > +{
> > +   device_unregister(_matrix->device);
> > +   root_device_unregister(vfio_ap_root_device);  
>  Also here you need a kfree(ap_matrix) too .  
> >>> Same here.
> >>> 
> 
>

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 14:01:54 +0200
Pierre Morel  wrote:

> On 13/06/2018 13:14, Cornelia Huck wrote:
> > On Wed, 13 Jun 2018 12:54:40 +0200
> > Pierre Morel  wrote:
> >  
> >> On 13/06/2018 09:48, Cornelia Huck wrote:  
> >>> On Wed, 13 Jun 2018 09:41:16 +0200
> >>> Pierre Morel  wrote:
> >>> 
>  On 07/05/2018 17:11, Tony Krowiak wrote:  
> > Introduces a new AP device driver. This device driver
> > is built on the VFIO mediated device framework. The framework
> > provides sysfs interfaces that facilitate passthrough
> > access by guests to devices installed on the linux host.  
>  ...snip...
>  
> > +static int vfio_ap_matrix_dev_create(void)
> > +{
> > +   int ret;
> > +
> > +   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> > +
> > +   if (IS_ERR(vfio_ap_root_device)) {
> > +   ret = PTR_ERR(vfio_ap_root_device);
> > +   goto done;
> > +   }
> > +
> > +   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> > +   if (!ap_matrix) {
> > +   ret = -ENOMEM;
> > +   goto matrix_alloc_err;
> > +   }
> > +
> > +   ap_matrix->device.type = _ap_dev_type;
> > +   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> > +   ap_matrix->device.parent = vfio_ap_root_device;
> > +   ap_matrix->device.release = vfio_ap_matrix_dev_release;
> > +   ap_matrix->device.driver = _ap_drv.driver;
> > +
> > +   ret = device_register(_matrix->device);
> > +   if (ret)
> > +   goto matrix_reg_err;
> > +
> > +   goto done;
> > +
> > +matrix_reg_err:
> > +   put_device(_matrix->device);  
>  Did not see this before but here you certainly want to
>  do a kfree and not a put_device.  
> >>> No, this must not be a kfree. Once you've tried to register something
> >>> embedding a struct device with the driver core, you need to use
> >>> put_device, as another path may have acquired a reference, even if
> >>> registering ultimately failed. See the comment for device_register().
> >>> IOW, the code is correct.  
> >> learned something again :) ,
> >> but still, a kfree is needed for the kzalloc.
> >> Does'nt it?  
> > No, the put callback for the embedding structure needs to take care of
> > freeing things. Otherwise it is buggy.  
> 
> Seems buggy to me.
> How does the put_device knows if it is embedded and then in what it is 
> embedded ?

It does not need to know; the code registering the structure needs to
set up device->release correctly.

> 
> >  
> >>> 
>  
> > +
> > +matrix_alloc_err:
> > +   root_device_unregister(vfio_ap_root_device);
> > +
> > +done:
> > +   return ret;
> > +}
> > +
> > +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> > +{
> > +   device_unregister(_matrix->device);
> > +   root_device_unregister(vfio_ap_root_device);  
>  Also here you need a kfree(ap_matrix) too .  
> >>> Same here.
> >>> 
> 
>

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:


On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:
  

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...
  

+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.


Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is 
embedded ?




  
  

+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.
  



--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 13:14, Cornelia Huck wrote:

On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:


On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:
  

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...
  

+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.


Seems buggy to me.
How does the put_device knows if it is embedded and then in what it is 
embedded ?




  
  

+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.
  



--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:

> On 13/06/2018 09:48, Cornelia Huck wrote:
> > On Wed, 13 Jun 2018 09:41:16 +0200
> > Pierre Morel  wrote:
> >  
> >> On 07/05/2018 17:11, Tony Krowiak wrote:  
> >>> Introduces a new AP device driver. This device driver
> >>> is built on the VFIO mediated device framework. The framework
> >>> provides sysfs interfaces that facilitate passthrough
> >>> access by guests to devices installed on the linux host.  
> >> ...snip...
> >>  
> >>> +static int vfio_ap_matrix_dev_create(void)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> >>> +
> >>> + if (IS_ERR(vfio_ap_root_device)) {
> >>> + ret = PTR_ERR(vfio_ap_root_device);
> >>> + goto done;
> >>> + }
> >>> +
> >>> + ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> >>> + if (!ap_matrix) {
> >>> + ret = -ENOMEM;
> >>> + goto matrix_alloc_err;
> >>> + }
> >>> +
> >>> + ap_matrix->device.type = _ap_dev_type;
> >>> + dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> >>> + ap_matrix->device.parent = vfio_ap_root_device;
> >>> + ap_matrix->device.release = vfio_ap_matrix_dev_release;
> >>> + ap_matrix->device.driver = _ap_drv.driver;
> >>> +
> >>> + ret = device_register(_matrix->device);
> >>> + if (ret)
> >>> + goto matrix_reg_err;
> >>> +
> >>> + goto done;
> >>> +
> >>> +matrix_reg_err:
> >>> + put_device(_matrix->device);  
> >> Did not see this before but here you certainly want to
> >> do a kfree and not a put_device.  
> > No, this must not be a kfree. Once you've tried to register something
> > embedding a struct device with the driver core, you need to use
> > put_device, as another path may have acquired a reference, even if
> > registering ultimately failed. See the comment for device_register().
> > IOW, the code is correct.  
> 
> learned something again :) ,
> but still, a kfree is needed for the kzalloc.
> Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

> 
> >  
> >>
> >>  
> >>> +
> >>> +matrix_alloc_err:
> >>> + root_device_unregister(vfio_ap_root_device);
> >>> +
> >>> +done:
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> >>> +{
> >>> + device_unregister(_matrix->device);
> >>> + root_device_unregister(vfio_ap_root_device);  
> >> Also here you need a kfree(ap_matrix) too .  
> > Same here.
> >  
>

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 12:54:40 +0200
Pierre Morel  wrote:

> On 13/06/2018 09:48, Cornelia Huck wrote:
> > On Wed, 13 Jun 2018 09:41:16 +0200
> > Pierre Morel  wrote:
> >  
> >> On 07/05/2018 17:11, Tony Krowiak wrote:  
> >>> Introduces a new AP device driver. This device driver
> >>> is built on the VFIO mediated device framework. The framework
> >>> provides sysfs interfaces that facilitate passthrough
> >>> access by guests to devices installed on the linux host.  
> >> ...snip...
> >>  
> >>> +static int vfio_ap_matrix_dev_create(void)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> >>> +
> >>> + if (IS_ERR(vfio_ap_root_device)) {
> >>> + ret = PTR_ERR(vfio_ap_root_device);
> >>> + goto done;
> >>> + }
> >>> +
> >>> + ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> >>> + if (!ap_matrix) {
> >>> + ret = -ENOMEM;
> >>> + goto matrix_alloc_err;
> >>> + }
> >>> +
> >>> + ap_matrix->device.type = _ap_dev_type;
> >>> + dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> >>> + ap_matrix->device.parent = vfio_ap_root_device;
> >>> + ap_matrix->device.release = vfio_ap_matrix_dev_release;
> >>> + ap_matrix->device.driver = _ap_drv.driver;
> >>> +
> >>> + ret = device_register(_matrix->device);
> >>> + if (ret)
> >>> + goto matrix_reg_err;
> >>> +
> >>> + goto done;
> >>> +
> >>> +matrix_reg_err:
> >>> + put_device(_matrix->device);  
> >> Did not see this before but here you certainly want to
> >> do a kfree and not a put_device.  
> > No, this must not be a kfree. Once you've tried to register something
> > embedding a struct device with the driver core, you need to use
> > put_device, as another path may have acquired a reference, even if
> > registering ultimately failed. See the comment for device_register().
> > IOW, the code is correct.  
> 
> learned something again :) ,
> but still, a kfree is needed for the kzalloc.
> Does'nt it?

No, the put callback for the embedding structure needs to take care of
freeing things. Otherwise it is buggy.

> 
> >  
> >>
> >>  
> >>> +
> >>> +matrix_alloc_err:
> >>> + root_device_unregister(vfio_ap_root_device);
> >>> +
> >>> +done:
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> >>> +{
> >>> + device_unregister(_matrix->device);
> >>> + root_device_unregister(vfio_ap_root_device);  
> >> Also here you need a kfree(ap_matrix) too .  
> > Same here.
> >  
>

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:


On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...


+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.


learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?







+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.



--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 13/06/2018 09:48, Cornelia Huck wrote:

On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:


On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...


+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);

Did not see this before but here you certainly want to
do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.


learned something again :) ,
but still, a kfree is needed for the kzalloc.
Does'nt it?







+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);

Also here you need a kfree(ap_matrix) too .

Same here.



--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:

> On 07/05/2018 17:11, Tony Krowiak wrote:
> > Introduces a new AP device driver. This device driver
> > is built on the VFIO mediated device framework. The framework
> > provides sysfs interfaces that facilitate passthrough
> > access by guests to devices installed on the linux host.  
> ...snip...
> 
> > +static int vfio_ap_matrix_dev_create(void)
> > +{
> > +   int ret;
> > +
> > +   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> > +
> > +   if (IS_ERR(vfio_ap_root_device)) {
> > +   ret = PTR_ERR(vfio_ap_root_device);
> > +   goto done;
> > +   }
> > +
> > +   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> > +   if (!ap_matrix) {
> > +   ret = -ENOMEM;
> > +   goto matrix_alloc_err;
> > +   }
> > +
> > +   ap_matrix->device.type = _ap_dev_type;
> > +   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> > +   ap_matrix->device.parent = vfio_ap_root_device;
> > +   ap_matrix->device.release = vfio_ap_matrix_dev_release;
> > +   ap_matrix->device.driver = _ap_drv.driver;
> > +
> > +   ret = device_register(_matrix->device);
> > +   if (ret)
> > +   goto matrix_reg_err;
> > +
> > +   goto done;
> > +
> > +matrix_reg_err:
> > +   put_device(_matrix->device);  
> 
> Did not see this before but here you certainly want to
> do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

> 
> 
> 
> > +
> > +matrix_alloc_err:
> > +   root_device_unregister(vfio_ap_root_device);
> > +
> > +done:
> > +   return ret;
> > +}
> > +
> > +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> > +{
> > +   device_unregister(_matrix->device);
> > +   root_device_unregister(vfio_ap_root_device);  
> 
> Also here you need a kfree(ap_matrix) too .

Same here.

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Cornelia Huck 
On Wed, 13 Jun 2018 09:41:16 +0200
Pierre Morel  wrote:

> On 07/05/2018 17:11, Tony Krowiak wrote:
> > Introduces a new AP device driver. This device driver
> > is built on the VFIO mediated device framework. The framework
> > provides sysfs interfaces that facilitate passthrough
> > access by guests to devices installed on the linux host.  
> ...snip...
> 
> > +static int vfio_ap_matrix_dev_create(void)
> > +{
> > +   int ret;
> > +
> > +   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> > +
> > +   if (IS_ERR(vfio_ap_root_device)) {
> > +   ret = PTR_ERR(vfio_ap_root_device);
> > +   goto done;
> > +   }
> > +
> > +   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
> > +   if (!ap_matrix) {
> > +   ret = -ENOMEM;
> > +   goto matrix_alloc_err;
> > +   }
> > +
> > +   ap_matrix->device.type = _ap_dev_type;
> > +   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
> > +   ap_matrix->device.parent = vfio_ap_root_device;
> > +   ap_matrix->device.release = vfio_ap_matrix_dev_release;
> > +   ap_matrix->device.driver = _ap_drv.driver;
> > +
> > +   ret = device_register(_matrix->device);
> > +   if (ret)
> > +   goto matrix_reg_err;
> > +
> > +   goto done;
> > +
> > +matrix_reg_err:
> > +   put_device(_matrix->device);  
> 
> Did not see this before but here you certainly want to
> do a kfree and not a put_device.

No, this must not be a kfree. Once you've tried to register something
embedding a struct device with the driver core, you need to use
put_device, as another path may have acquired a reference, even if
registering ultimately failed. See the comment for device_register().
IOW, the code is correct.

> 
> 
> 
> > +
> > +matrix_alloc_err:
> > +   root_device_unregister(vfio_ap_root_device);
> > +
> > +done:
> > +   return ret;
> > +}
> > +
> > +static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
> > +{
> > +   device_unregister(_matrix->device);
> > +   root_device_unregister(vfio_ap_root_device);  
> 
> Also here you need a kfree(ap_matrix) too .

Same here.

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...


+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);


Did not see this before but here you certainly want to
do a kfree and not a put_device.




+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);


Also here you need a kfree(ap_matrix) too .

Pierre


--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-13 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...


+static int vfio_ap_matrix_dev_create(void)
+{
+   int ret;
+
+   vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
+
+   if (IS_ERR(vfio_ap_root_device)) {
+   ret = PTR_ERR(vfio_ap_root_device);
+   goto done;
+   }
+
+   ap_matrix = kzalloc(sizeof(*ap_matrix), GFP_KERNEL);
+   if (!ap_matrix) {
+   ret = -ENOMEM;
+   goto matrix_alloc_err;
+   }
+
+   ap_matrix->device.type = _ap_dev_type;
+   dev_set_name(_matrix->device, "%s", VFIO_AP_DEV_NAME);
+   ap_matrix->device.parent = vfio_ap_root_device;
+   ap_matrix->device.release = vfio_ap_matrix_dev_release;
+   ap_matrix->device.driver = _ap_drv.driver;
+
+   ret = device_register(_matrix->device);
+   if (ret)
+   goto matrix_reg_err;
+
+   goto done;
+
+matrix_reg_err:
+   put_device(_matrix->device);


Did not see this before but here you certainly want to
do a kfree and not a put_device.




+
+matrix_alloc_err:
+   root_device_unregister(vfio_ap_root_device);
+
+done:
+   return ret;
+}
+
+static void vfio_ap_matrix_dev_destroy(struct ap_matrix *ap_matrix)
+{
+   device_unregister(_matrix->device);
+   root_device_unregister(vfio_ap_root_device);


Also here you need a kfree(ap_matrix) too .

Pierre


--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-07 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

The VFIO AP device driver will serve two purposes:

1. Provide the interfaces to reserve AP devices for exclusive
use by KVM guests. This is accomplished by unbinding the
devices to be reserved for guest usage from the default AP
device driver and binding them to the VFIO AP device driver.

2. Implements the functions, callbacks and sysfs attribute
interfaces required to create one or more VFIO mediated
devices each of which will be used to configure the AP
matrix for a guest and serve as a file descriptor
for facilitating communication between QEMU and the
VFIO AP device driver.

When the VFIO AP device driver is initialized:

* It registers with the AP bus for control of type 10 (CEX4
   and newer) AP queue devices. This limitation was imposed
   due to:

   1. A lack of access to older systems needed to test the
  older AP device models;

   2. A desire to keep the code as simple as possible;

   3. Some older models are no longer supported by the kernel
  and others are getting close to end of service.

   The probe and remove callbacks will be provided to support
   the binding/unbinding of AP queue devices to/from the VFIO
   AP device driver.

* Creates a /sys/devices/vfio-ap/matrix device to hold
   the APQNs of the AP devices bound to the VFIO
   AP device driver and serves as the parent of the
   mediated devices created for each guest.

Signed-off-by: Tony Krowiak 
---
  MAINTAINERS   |   10 +++
  arch/s390/Kconfig |   11 +++
  drivers/s390/crypto/Makefile  |4 +
  drivers/s390/crypto/vfio_ap_drv.c |  134 +
  drivers/s390/crypto/vfio_ap_private.h |   23 ++
  include/uapi/linux/vfio.h |2 +
  6 files changed, 184 insertions(+), 0 deletions(-)
  create mode 100644 drivers/s390/crypto/vfio_ap_drv.c
  create mode 100644 drivers/s390/crypto/vfio_ap_private.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 224e97b..2792c81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12237,6 +12237,16 @@ W: 
http://www.ibm.com/developerworks/linux/linux390/
  S:Supported
  F:drivers/s390/crypto/

+S390 VFIO AP DRIVER
+M: Tony Krowiak 
+M: Christian Borntraeger 
+M: Martin Schwidefsky 
+L: linux-s...@vger.kernel.org
+W: http://www.ibm.com/developerworks/linux/linux390/
+S: Supported
+F: drivers/s390/crypto/vfio_ap_drv.c
+F: drivers/s390/crypto/vfio_ap_private.h
+
  S390 ZFCP DRIVER
  M:Steffen Maier 
  M:Benjamin Block 
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 199ac3e..8d833be 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -786,6 +786,17 @@ config VFIO_CCW
  To compile this driver as a module, choose M here: the
  module will be called vfio_ccw.

+config VFIO_AP
+   def_tristate n
+   prompt "VFIO support for AP devices"
+   depends on ZCRYPT && VFIO_MDEV_DEVICE && KVM
+   help
+   This driver grants access to Adjunct Processor (AP) devices
+   via the VFIO mediated device interface.
+
+   To compile this driver as a module, choose M here: the module
+   will be called vfio_ap.
+
  endmenu

  menu "Dump support"
diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile
index b59af54..48e466e 100644
--- a/drivers/s390/crypto/Makefile
+++ b/drivers/s390/crypto/Makefile
@@ -15,3 +15,7 @@ obj-$(CONFIG_ZCRYPT) += zcrypt_pcixcc.o zcrypt_cex2a.o 
zcrypt_cex4.o
  # pkey kernel module
  pkey-objs := pkey_api.o
  obj-$(CONFIG_PKEY) += pkey.o
+
+# adjunct processor matrix
+vfio_ap-objs := vfio_ap_drv.o
+obj-$(CONFIG_VFIO_AP) += vfio_ap.o
diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
b/drivers/s390/crypto/vfio_ap_drv.c
new file mode 100644
index 000..014d70f
--- /dev/null
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VFIO based AP device driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Tony Krowiak 
+ */
+
+#include 
+#include 
+#include 
+
+#include "vfio_ap_private.h"
+
+#define VFIO_AP_ROOT_NAME "vfio_ap"
+#define VFIO_AP_DEV_TYPE_NAME "ap_matrix"
+#define VFIO_AP_DEV_NAME "matrix"
+
+MODULE_AUTHOR("IBM Corporation");
+MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2017");
+MODULE_LICENSE("GPL v2");
+
+static struct device *vfio_ap_root_device;
+
+static struct ap_driver vfio_ap_drv;
+
+static struct ap_matrix *ap_matrix;
+
+static struct device_type vfio_ap_dev_type = {
+   .name = VFIO_AP_DEV_TYPE_NAME,
+};
+
+/* Only type 10 adapters (CEX4 and later) are supported
+ * by the AP matrix device driver
+ */
+static struct ap_device_id ap_queue_ids[] =

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-06-07 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

The VFIO AP device driver will serve two purposes:

1. Provide the interfaces to reserve AP devices for exclusive
use by KVM guests. This is accomplished by unbinding the
devices to be reserved for guest usage from the default AP
device driver and binding them to the VFIO AP device driver.

2. Implements the functions, callbacks and sysfs attribute
interfaces required to create one or more VFIO mediated
devices each of which will be used to configure the AP
matrix for a guest and serve as a file descriptor
for facilitating communication between QEMU and the
VFIO AP device driver.

When the VFIO AP device driver is initialized:

* It registers with the AP bus for control of type 10 (CEX4
   and newer) AP queue devices. This limitation was imposed
   due to:

   1. A lack of access to older systems needed to test the
  older AP device models;

   2. A desire to keep the code as simple as possible;

   3. Some older models are no longer supported by the kernel
  and others are getting close to end of service.

   The probe and remove callbacks will be provided to support
   the binding/unbinding of AP queue devices to/from the VFIO
   AP device driver.

* Creates a /sys/devices/vfio-ap/matrix device to hold
   the APQNs of the AP devices bound to the VFIO
   AP device driver and serves as the parent of the
   mediated devices created for each guest.

Signed-off-by: Tony Krowiak 
---
  MAINTAINERS   |   10 +++
  arch/s390/Kconfig |   11 +++
  drivers/s390/crypto/Makefile  |4 +
  drivers/s390/crypto/vfio_ap_drv.c |  134 +
  drivers/s390/crypto/vfio_ap_private.h |   23 ++
  include/uapi/linux/vfio.h |2 +
  6 files changed, 184 insertions(+), 0 deletions(-)
  create mode 100644 drivers/s390/crypto/vfio_ap_drv.c
  create mode 100644 drivers/s390/crypto/vfio_ap_private.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 224e97b..2792c81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12237,6 +12237,16 @@ W: 
http://www.ibm.com/developerworks/linux/linux390/
  S:Supported
  F:drivers/s390/crypto/

+S390 VFIO AP DRIVER
+M: Tony Krowiak 
+M: Christian Borntraeger 
+M: Martin Schwidefsky 
+L: linux-s...@vger.kernel.org
+W: http://www.ibm.com/developerworks/linux/linux390/
+S: Supported
+F: drivers/s390/crypto/vfio_ap_drv.c
+F: drivers/s390/crypto/vfio_ap_private.h
+
  S390 ZFCP DRIVER
  M:Steffen Maier 
  M:Benjamin Block 
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 199ac3e..8d833be 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -786,6 +786,17 @@ config VFIO_CCW
  To compile this driver as a module, choose M here: the
  module will be called vfio_ccw.

+config VFIO_AP
+   def_tristate n
+   prompt "VFIO support for AP devices"
+   depends on ZCRYPT && VFIO_MDEV_DEVICE && KVM
+   help
+   This driver grants access to Adjunct Processor (AP) devices
+   via the VFIO mediated device interface.
+
+   To compile this driver as a module, choose M here: the module
+   will be called vfio_ap.
+
  endmenu

  menu "Dump support"
diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile
index b59af54..48e466e 100644
--- a/drivers/s390/crypto/Makefile
+++ b/drivers/s390/crypto/Makefile
@@ -15,3 +15,7 @@ obj-$(CONFIG_ZCRYPT) += zcrypt_pcixcc.o zcrypt_cex2a.o 
zcrypt_cex4.o
  # pkey kernel module
  pkey-objs := pkey_api.o
  obj-$(CONFIG_PKEY) += pkey.o
+
+# adjunct processor matrix
+vfio_ap-objs := vfio_ap_drv.o
+obj-$(CONFIG_VFIO_AP) += vfio_ap.o
diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
b/drivers/s390/crypto/vfio_ap_drv.c
new file mode 100644
index 000..014d70f
--- /dev/null
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VFIO based AP device driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Tony Krowiak 
+ */
+
+#include 
+#include 
+#include 
+
+#include "vfio_ap_private.h"
+
+#define VFIO_AP_ROOT_NAME "vfio_ap"
+#define VFIO_AP_DEV_TYPE_NAME "ap_matrix"
+#define VFIO_AP_DEV_NAME "matrix"
+
+MODULE_AUTHOR("IBM Corporation");
+MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2017");
+MODULE_LICENSE("GPL v2");
+
+static struct device *vfio_ap_root_device;
+
+static struct ap_driver vfio_ap_drv;
+
+static struct ap_matrix *ap_matrix;
+
+static struct device_type vfio_ap_dev_type = {
+   .name = VFIO_AP_DEV_TYPE_NAME,
+};
+
+/* Only type 10 adapters (CEX4 and later) are supported
+ * by the AP matrix device driver
+ */
+static struct ap_device_id ap_queue_ids[] =

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Tony Krowiak 

On 05/16/2018 04:21 AM, Pierre Morel wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


This is not possible without a change to the AP bus. The remove callback
returns void, so there is no way to indicate to the AP bus not to remove
the queue device. I'll talk to Harald about this.






+return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+kfree(ap_matrix);
+}

...snip...

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Tony Krowiak 

On 05/16/2018 04:21 AM, Pierre Morel wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


This is not possible without a change to the AP bus. The remove callback
returns void, so there is no way to indicate to the AP bus not to remove
the queue device. I'll talk to Harald about this.






+return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+kfree(ap_matrix);
+}

...snip...

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Tony Krowiak 

On 05/16/2018 04:21 AM, Pierre Morel wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


That makes sense, will do.





+return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+kfree(ap_matrix);
+}

...snip...

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Tony Krowiak 

On 05/16/2018 04:21 AM, Pierre Morel wrote:

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


That makes sense, will do.





+return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+kfree(ap_matrix);
+}

...snip...

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


+   return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+   struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+   kfree(ap_matrix);
+}

...snip...

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-16 Thread Pierre Morel 

On 07/05/2018 17:11, Tony Krowiak wrote:

Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

...snip...

+static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
+{


You should take care of the ap devices when they are added or removed
from the matrix.
I suggest you add a remove callback to avoid unbinding a queue while
it is assigned to a guest.


+   return 0;
+}
+
+static void vfio_ap_matrix_dev_release(struct device *dev)
+{
+   struct ap_matrix *ap_matrix = dev_get_drvdata(dev);
+
+   kfree(ap_matrix);
+}

...snip...

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

[PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-07 Thread Tony Krowiak 
Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

The VFIO AP device driver will serve two purposes:

1. Provide the interfaces to reserve AP devices for exclusive
   use by KVM guests. This is accomplished by unbinding the
   devices to be reserved for guest usage from the default AP
   device driver and binding them to the VFIO AP device driver.

2. Implements the functions, callbacks and sysfs attribute
   interfaces required to create one or more VFIO mediated
   devices each of which will be used to configure the AP
   matrix for a guest and serve as a file descriptor
   for facilitating communication between QEMU and the
   VFIO AP device driver.

When the VFIO AP device driver is initialized:

* It registers with the AP bus for control of type 10 (CEX4
  and newer) AP queue devices. This limitation was imposed
  due to:

  1. A lack of access to older systems needed to test the
 older AP device models;

  2. A desire to keep the code as simple as possible;

  3. Some older models are no longer supported by the kernel
 and others are getting close to end of service.

  The probe and remove callbacks will be provided to support
  the binding/unbinding of AP queue devices to/from the VFIO
  AP device driver.

* Creates a /sys/devices/vfio-ap/matrix device to hold
  the APQNs of the AP devices bound to the VFIO
  AP device driver and serves as the parent of the
  mediated devices created for each guest.

Signed-off-by: Tony Krowiak 
---
 MAINTAINERS   |   10 +++
 arch/s390/Kconfig |   11 +++
 drivers/s390/crypto/Makefile  |4 +
 drivers/s390/crypto/vfio_ap_drv.c |  134 +
 drivers/s390/crypto/vfio_ap_private.h |   23 ++
 include/uapi/linux/vfio.h |2 +
 6 files changed, 184 insertions(+), 0 deletions(-)
 create mode 100644 drivers/s390/crypto/vfio_ap_drv.c
 create mode 100644 drivers/s390/crypto/vfio_ap_private.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 224e97b..2792c81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12237,6 +12237,16 @@ W: 
http://www.ibm.com/developerworks/linux/linux390/
 S: Supported
 F: drivers/s390/crypto/
 
+S390 VFIO AP DRIVER
+M: Tony Krowiak 
+M: Christian Borntraeger 
+M: Martin Schwidefsky 
+L: linux-s...@vger.kernel.org
+W: http://www.ibm.com/developerworks/linux/linux390/
+S: Supported
+F: drivers/s390/crypto/vfio_ap_drv.c
+F: drivers/s390/crypto/vfio_ap_private.h
+
 S390 ZFCP DRIVER
 M: Steffen Maier 
 M: Benjamin Block 
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 199ac3e..8d833be 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -786,6 +786,17 @@ config VFIO_CCW
  To compile this driver as a module, choose M here: the
  module will be called vfio_ccw.
 
+config VFIO_AP
+   def_tristate n
+   prompt "VFIO support for AP devices"
+   depends on ZCRYPT && VFIO_MDEV_DEVICE && KVM
+   help
+   This driver grants access to Adjunct Processor (AP) devices
+   via the VFIO mediated device interface.
+
+   To compile this driver as a module, choose M here: the module
+   will be called vfio_ap.
+
 endmenu
 
 menu "Dump support"
diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile
index b59af54..48e466e 100644
--- a/drivers/s390/crypto/Makefile
+++ b/drivers/s390/crypto/Makefile
@@ -15,3 +15,7 @@ obj-$(CONFIG_ZCRYPT) += zcrypt_pcixcc.o zcrypt_cex2a.o 
zcrypt_cex4.o
 # pkey kernel module
 pkey-objs := pkey_api.o
 obj-$(CONFIG_PKEY) += pkey.o
+
+# adjunct processor matrix
+vfio_ap-objs := vfio_ap_drv.o
+obj-$(CONFIG_VFIO_AP) += vfio_ap.o
diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
b/drivers/s390/crypto/vfio_ap_drv.c
new file mode 100644
index 000..014d70f
--- /dev/null
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VFIO based AP device driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Tony Krowiak 
+ */
+
+#include 
+#include 
+#include 
+
+#include "vfio_ap_private.h"
+
+#define VFIO_AP_ROOT_NAME "vfio_ap"
+#define VFIO_AP_DEV_TYPE_NAME "ap_matrix"
+#define VFIO_AP_DEV_NAME "matrix"
+
+MODULE_AUTHOR("IBM Corporation");
+MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2017");
+MODULE_LICENSE("GPL v2");
+
+static struct device *vfio_ap_root_device;
+
+static struct ap_driver vfio_ap_drv;
+
+static struct ap_matrix *ap_matrix;
+
+static struct device_type vfio_ap_dev_type = {
+   .name = VFIO_AP_DEV_TYPE_NAME,
+};
+
+/* Only type 10 adapters (CEX4 and

[PATCH v5 04/13] s390: vfio-ap: base implementation of VFIO AP device driver

2018-05-07 Thread Tony Krowiak 
Introduces a new AP device driver. This device driver
is built on the VFIO mediated device framework. The framework
provides sysfs interfaces that facilitate passthrough
access by guests to devices installed on the linux host.

The VFIO AP device driver will serve two purposes:

1. Provide the interfaces to reserve AP devices for exclusive
   use by KVM guests. This is accomplished by unbinding the
   devices to be reserved for guest usage from the default AP
   device driver and binding them to the VFIO AP device driver.

2. Implements the functions, callbacks and sysfs attribute
   interfaces required to create one or more VFIO mediated
   devices each of which will be used to configure the AP
   matrix for a guest and serve as a file descriptor
   for facilitating communication between QEMU and the
   VFIO AP device driver.

When the VFIO AP device driver is initialized:

* It registers with the AP bus for control of type 10 (CEX4
  and newer) AP queue devices. This limitation was imposed
  due to:

  1. A lack of access to older systems needed to test the
 older AP device models;

  2. A desire to keep the code as simple as possible;

  3. Some older models are no longer supported by the kernel
 and others are getting close to end of service.

  The probe and remove callbacks will be provided to support
  the binding/unbinding of AP queue devices to/from the VFIO
  AP device driver.

* Creates a /sys/devices/vfio-ap/matrix device to hold
  the APQNs of the AP devices bound to the VFIO
  AP device driver and serves as the parent of the
  mediated devices created for each guest.

Signed-off-by: Tony Krowiak 
---
 MAINTAINERS   |   10 +++
 arch/s390/Kconfig |   11 +++
 drivers/s390/crypto/Makefile  |4 +
 drivers/s390/crypto/vfio_ap_drv.c |  134 +
 drivers/s390/crypto/vfio_ap_private.h |   23 ++
 include/uapi/linux/vfio.h |2 +
 6 files changed, 184 insertions(+), 0 deletions(-)
 create mode 100644 drivers/s390/crypto/vfio_ap_drv.c
 create mode 100644 drivers/s390/crypto/vfio_ap_private.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 224e97b..2792c81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12237,6 +12237,16 @@ W: 
http://www.ibm.com/developerworks/linux/linux390/
 S: Supported
 F: drivers/s390/crypto/
 
+S390 VFIO AP DRIVER
+M: Tony Krowiak 
+M: Christian Borntraeger 
+M: Martin Schwidefsky 
+L: linux-s...@vger.kernel.org
+W: http://www.ibm.com/developerworks/linux/linux390/
+S: Supported
+F: drivers/s390/crypto/vfio_ap_drv.c
+F: drivers/s390/crypto/vfio_ap_private.h
+
 S390 ZFCP DRIVER
 M: Steffen Maier 
 M: Benjamin Block 
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 199ac3e..8d833be 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -786,6 +786,17 @@ config VFIO_CCW
  To compile this driver as a module, choose M here: the
  module will be called vfio_ccw.
 
+config VFIO_AP
+   def_tristate n
+   prompt "VFIO support for AP devices"
+   depends on ZCRYPT && VFIO_MDEV_DEVICE && KVM
+   help
+   This driver grants access to Adjunct Processor (AP) devices
+   via the VFIO mediated device interface.
+
+   To compile this driver as a module, choose M here: the module
+   will be called vfio_ap.
+
 endmenu
 
 menu "Dump support"
diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile
index b59af54..48e466e 100644
--- a/drivers/s390/crypto/Makefile
+++ b/drivers/s390/crypto/Makefile
@@ -15,3 +15,7 @@ obj-$(CONFIG_ZCRYPT) += zcrypt_pcixcc.o zcrypt_cex2a.o 
zcrypt_cex4.o
 # pkey kernel module
 pkey-objs := pkey_api.o
 obj-$(CONFIG_PKEY) += pkey.o
+
+# adjunct processor matrix
+vfio_ap-objs := vfio_ap_drv.o
+obj-$(CONFIG_VFIO_AP) += vfio_ap.o
diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
b/drivers/s390/crypto/vfio_ap_drv.c
new file mode 100644
index 000..014d70f
--- /dev/null
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VFIO based AP device driver
+ *
+ * Copyright IBM Corp. 2018
+ *
+ * Author(s): Tony Krowiak 
+ */
+
+#include 
+#include 
+#include 
+
+#include "vfio_ap_private.h"
+
+#define VFIO_AP_ROOT_NAME "vfio_ap"
+#define VFIO_AP_DEV_TYPE_NAME "ap_matrix"
+#define VFIO_AP_DEV_NAME "matrix"
+
+MODULE_AUTHOR("IBM Corporation");
+MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2017");
+MODULE_LICENSE("GPL v2");
+
+static struct device *vfio_ap_root_device;
+
+static struct ap_driver vfio_ap_drv;
+
+static struct ap_matrix *ap_matrix;
+
+static struct device_type vfio_ap_dev_type = {
+   .name = VFIO_AP_DEV_TYPE_NAME,
+};
+
+/* Only type 10 adapters (CEX4 and later) are supported
+ * by the AP matrix device driver
+ */
+static struct ap_device_id ap_queue_ids[] = {
+   { .dev_type = AP_DEVICE_TYPE_CEX4,
+ .match_flags =