| Message ID | 1525705912-12815-6-git-send-email-akrowiak@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/2018 05:11 PM, Tony Krowiak wrote: > Registers the matrix device created by the VFIO AP device > driver with the VFIO mediated device framework. > Registering the matrix device will create the sysfs > structures needed to create mediated matrix devices > each of which will be used to configure the AP matrix > for a guest and connect it to the VFIO AP device driver. > [..] > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > new file mode 100644 > index 0000000..d7d36fb > --- /dev/null > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -0,0 +1,106 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Adjunct processor matrix VFIO device driver callbacks. > + * > + * Copyright IBM Corp. 2017 > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + */ > +#include <linux/string.h> > +#include <linux/vfio.h> > +#include <linux/device.h> > +#include <linux/list.h> > +#include <linux/ctype.h> > + > +#include "vfio_ap_private.h" > + > +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" > +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" > + > +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > +{ > + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > + > + ap_matrix->available_instances--; > + > + return 0; > +} > + > +static int vfio_ap_mdev_remove(struct mdev_device *mdev) > +{ > + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > + > + ap_matrix->available_instances++; > + > + return 0; > +} > + The above functions seem to be called with the lock of this auto-generated mdev parent device held. That's why we don't have to care about synchronization ourselves, right? A small comment in the code could be helpful for mdev non-experts. Hell, I would even consider documenting it for all mdev -- took me some time to figure out. [..] > +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) > +{ > + int ret; > + > + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); > + if (ret) > + return ret; > + > + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; > + > + return 0; > +} > + > +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) > +{ > + ap_matrix->available_instances--; What is this for? I don't understand. Regards, Halil > + mdev_unregister_device(&ap_matrix->device); > +}
On 05/11/2018 01:18 PM, Halil Pasic wrote: > > > On 05/07/2018 05:11 PM, Tony Krowiak wrote: >> Registers the matrix device created by the VFIO AP device >> driver with the VFIO mediated device framework. >> Registering the matrix device will create the sysfs >> structures needed to create mediated matrix devices >> each of which will be used to configure the AP matrix >> for a guest and connect it to the VFIO AP device driver. >> > [..] >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >> b/drivers/s390/crypto/vfio_ap_ops.c >> new file mode 100644 >> index 0000000..d7d36fb >> --- /dev/null >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -0,0 +1,106 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Adjunct processor matrix VFIO device driver callbacks. >> + * >> + * Copyright IBM Corp. 2017 >> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> + * >> + */ >> +#include <linux/string.h> >> +#include <linux/vfio.h> >> +#include <linux/device.h> >> +#include <linux/list.h> >> +#include <linux/ctype.h> >> + >> +#include "vfio_ap_private.h" >> + >> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >> + >> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >> mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances--; >> + >> + return 0; >> +} >> + >> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances++; >> + >> + return 0; >> +} >> + > > The above functions seem to be called with the lock of this > auto-generated > mdev parent device held. That's why we don't have to care about > synchronization > ourselves, right? I would assume as much. The comments for the 'struct mdev_parent_ops' in include/linux/mdev.h do not mention anything about synchronization, nor did I see any locking or synchronization in the vfio_ccw implementation after which I modeled my code, so frankly it is something I did not consider. > > > A small comment in the code could be helpful for mdev non-experts. > Hell, I would > even consider documenting it for all mdev -- took me some time to > figure out. You may want to bring this up with the VFIO mdev maintainers, but I'd be happy to include a comment in the functions in question if you think it important. > > > [..] > > >> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >> +{ >> + int ret; >> + >> + ret = mdev_register_device(&ap_matrix->device, >> &vfio_ap_matrix_ops); >> + if (ret) >> + return ret; >> + >> + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; >> + >> + return 0; >> +} >> + >> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >> +{ >> + ap_matrix->available_instances--; > > What is this for? I don't understand. To control the number of mediated devices one can create for the matrix device. Once the max is reached, the mdev framework will not allow creation of another mediated device until one is removed. This counter keeps track of the number of instances that can be created. This is documented with the mediated framework. You may want to take a look at: Documentation/vfio-mediated-device.txt Documentation/vfio.txt Documentation/virtual/kvm/devices/vfio.txt > > > Regards, > Halil > >> + mdev_unregister_device(&ap_matrix->device); >> +}
On 14/05/2018 21:42, Tony Krowiak wrote: > On 05/11/2018 01:18 PM, Halil Pasic wrote: >> >> >> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>> Registers the matrix device created by the VFIO AP device >>> driver with the VFIO mediated device framework. >>> Registering the matrix device will create the sysfs >>> structures needed to create mediated matrix devices >>> each of which will be used to configure the AP matrix >>> for a guest and connect it to the VFIO AP device driver. >>> >> [..] >>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >>> b/drivers/s390/crypto/vfio_ap_ops.c >>> new file mode 100644 >>> index 0000000..d7d36fb >>> --- /dev/null >>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>> @@ -0,0 +1,106 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * Adjunct processor matrix VFIO device driver callbacks. >>> + * >>> + * Copyright IBM Corp. 2017 >>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>> + * >>> + */ >>> +#include <linux/string.h> >>> +#include <linux/vfio.h> >>> +#include <linux/device.h> >>> +#include <linux/list.h> >>> +#include <linux/ctype.h> >>> + >>> +#include "vfio_ap_private.h" >>> + >>> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >>> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >>> + >>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >>> mdev_device *mdev) >>> +{ >>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>> + >>> + ap_matrix->available_instances--; >>> + >>> + return 0; >>> +} >>> + >>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>> +{ >>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>> + >>> + ap_matrix->available_instances++; >>> + >>> + return 0; >>> +} >>> + >> >> The above functions seem to be called with the lock of this >> auto-generated >> mdev parent device held. That's why we don't have to care about >> synchronization >> ourselves, right? > > I would assume as much. The comments for the 'struct mdev_parent_ops' in > include/linux/mdev.h do not mention anything about synchronization, > nor did I > see any locking or synchronization in the vfio_ccw implementation > after which > I modeled my code, so frankly it is something I did not consider. > >> >> >> A small comment in the code could be helpful for mdev non-experts. >> Hell, I would >> even consider documenting it for all mdev -- took me some time to >> figure out. > > You may want to bring this up with the VFIO mdev maintainers, but I'd > be happy to > include a comment in the functions in question if you think it important. > >> >> >> [..] >> >> >>> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >>> +{ >>> + int ret; >>> + >>> + ret = mdev_register_device(&ap_matrix->device, >>> &vfio_ap_matrix_ops); >>> + if (ret) >>> + return ret; >>> + >>> + ap_matrix->available_instances = >>> AP_MATRIX_MAX_AVAILABLE_INSTANCES; >>> + >>> + return 0; >>> +} >>> + >>> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >>> +{ >>> + ap_matrix->available_instances--; >> >> What is this for? I don't understand. > > To control the number of mediated devices one can create for the > matrix device. > Once the max is reached, the mdev framework will not allow creation of > another > mediated device until one is removed. This counter keeps track of the > number > of instances that can be created. This is documented with the mediated > framework. You may want to take a look at: > > Documentation/vfio-mediated-device.txt > Documentation/vfio.txt > Documentation/virtual/kvm/devices/vfio.txt This is what you do in create/remove. But here in unregister I agree with Halil, it does not seem to be usefull. > >> >> >> Regards, >> Halil >> >>> + mdev_unregister_device(&ap_matrix->device); >>> +} > >
On 05/15/2018 10:17 AM, Pierre Morel wrote: > On 14/05/2018 21:42, Tony Krowiak wrote: >> On 05/11/2018 01:18 PM, Halil Pasic wrote: >>> >>> >>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>>> Registers the matrix device created by the VFIO AP device >>>> driver with the VFIO mediated device framework. >>>> Registering the matrix device will create the sysfs >>>> structures needed to create mediated matrix devices >>>> each of which will be used to configure the AP matrix >>>> for a guest and connect it to the VFIO AP device driver. >>>> >>> [..] >>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >>>> b/drivers/s390/crypto/vfio_ap_ops.c >>>> new file mode 100644 >>>> index 0000000..d7d36fb >>>> --- /dev/null >>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>>> @@ -0,0 +1,106 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +/* >>>> + * Adjunct processor matrix VFIO device driver callbacks. >>>> + * >>>> + * Copyright IBM Corp. 2017 >>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>> + * >>>> + */ >>>> +#include <linux/string.h> >>>> +#include <linux/vfio.h> >>>> +#include <linux/device.h> >>>> +#include <linux/list.h> >>>> +#include <linux/ctype.h> >>>> + >>>> +#include "vfio_ap_private.h" >>>> + >>>> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >>>> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >>>> + >>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >>>> mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix *ap_matrix = >>>> to_ap_matrix(mdev_parent_dev(mdev)); >>>> + >>>> + ap_matrix->available_instances--; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix *ap_matrix = >>>> to_ap_matrix(mdev_parent_dev(mdev)); >>>> + >>>> + ap_matrix->available_instances++; >>>> + >>>> + return 0; >>>> +} >>>> + >>> >>> The above functions seem to be called with the lock of this >>> auto-generated >>> mdev parent device held. That's why we don't have to care about >>> synchronization >>> ourselves, right? >> >> I would assume as much. The comments for the 'struct mdev_parent_ops' in >> include/linux/mdev.h do not mention anything about synchronization, >> nor did I >> see any locking or synchronization in the vfio_ccw implementation >> after which >> I modeled my code, so frankly it is something I did not consider. >> >>> >>> >>> A small comment in the code could be helpful for mdev non-experts. >>> Hell, I would >>> even consider documenting it for all mdev -- took me some time to >>> figure out. >> >> You may want to bring this up with the VFIO mdev maintainers, but I'd >> be happy to >> include a comment in the functions in question if you think it >> important. >> >>> >>> >>> [..] >>> >>> >>>> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = mdev_register_device(&ap_matrix->device, >>>> &vfio_ap_matrix_ops); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ap_matrix->available_instances = >>>> AP_MATRIX_MAX_AVAILABLE_INSTANCES; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >>>> +{ >>>> + ap_matrix->available_instances--; >>> >>> What is this for? I don't understand. >> >> To control the number of mediated devices one can create for the >> matrix device. >> Once the max is reached, the mdev framework will not allow creation >> of another >> mediated device until one is removed. This counter keeps track of the >> number >> of instances that can be created. This is documented with the mediated >> framework. You may want to take a look at: >> >> Documentation/vfio-mediated-device.txt >> Documentation/vfio.txt >> Documentation/virtual/kvm/devices/vfio.txt > > This is what you do in create/remove. > But here in unregister I agree with Halil, it does not seem to be usefull. If that is in fact what Halil was asking, then I misinterpreted his question; I thought he was asking what the available_instances was used for. You are correct, this does not belong here although it makes little difference given this is called only when the driver, which creates the matrix device, is unloaded. It is necessary in the register function to initialize its value, but I'll remove it from here. > > >> >>> >>> >>> Regards, >>> Halil >>> >>>> + mdev_unregister_device(&ap_matrix->device); >>>> +} >> >> >
On 05/15/2018 05:16 PM, Tony Krowiak wrote: > On 05/15/2018 10:17 AM, Pierre Morel wrote: >> On 14/05/2018 21:42, Tony Krowiak wrote: >>> On 05/11/2018 01:18 PM, Halil Pasic wrote: >>>> >>>> >>>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>>>> Registers the matrix device created by the VFIO AP device >>>>> driver with the VFIO mediated device framework. >>>>> Registering the matrix device will create the sysfs >>>>> structures needed to create mediated matrix devices >>>>> each of which will be used to configure the AP matrix >>>>> for a guest and connect it to the VFIO AP device driver. >>>>> >>>> [..] >>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >>>>> new file mode 100644 >>>>> index 0000000..d7d36fb >>>>> --- /dev/null >>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>>>> @@ -0,0 +1,106 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0+ >>>>> +/* >>>>> + * Adjunct processor matrix VFIO device driver callbacks. >>>>> + * >>>>> + * Copyright IBM Corp. 2017 >>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>>> + * >>>>> + */ >>>>> +#include <linux/string.h> >>>>> +#include <linux/vfio.h> >>>>> +#include <linux/device.h> >>>>> +#include <linux/list.h> >>>>> +#include <linux/ctype.h> >>>>> + >>>>> +#include "vfio_ap_private.h" >>>>> + >>>>> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >>>>> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >>>>> + >>>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) >>>>> +{ >>>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>>> + >>>>> + ap_matrix->available_instances--; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>>>> +{ >>>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>>> + >>>>> + ap_matrix->available_instances++; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>> >>>> The above functions seem to be called with the lock of this auto-generated >>>> mdev parent device held. That's why we don't have to care about synchronization >>>> ourselves, right? >>> >>> I would assume as much. The comments for the 'struct mdev_parent_ops' in >>> include/linux/mdev.h do not mention anything about synchronization, nor did I >>> see any locking or synchronization in the vfio_ccw implementation after which >>> I modeled my code, so frankly it is something I did not consider. >>> >>>> >>>> >>>> A small comment in the code could be helpful for mdev non-experts. Hell, I would >>>> even consider documenting it for all mdev -- took me some time to figure out. >>> >>> You may want to bring this up with the VFIO mdev maintainers, but I'd be happy to >>> include a comment in the functions in question if you think it important. >>> >>>> >>>> >>>> [..] >>>> >>>> >>>>> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >>>>> +{ >>>>> + ap_matrix->available_instances--; >>>> >>>> What is this for? I don't understand. >>> >>> To control the number of mediated devices one can create for the matrix device. >>> Once the max is reached, the mdev framework will not allow creation of another >>> mediated device until one is removed. This counter keeps track of the number >>> of instances that can be created. This is documented with the mediated >>> framework. You may want to take a look at: >>> >>> Documentation/vfio-mediated-device.txt >>> Documentation/vfio.txt >>> Documentation/virtual/kvm/devices/vfio.txt >> >> This is what you do in create/remove. >> But here in unregister I agree with Halil, it does not seem to be usefull. > > If that is in fact what Halil was asking, then I misinterpreted his question; I > thought he was asking what the available_instances was used for. You are > correct, this does not belong here although it makes little difference given > this is called only when the driver, which creates the matrix device, is unloaded. > It is necessary in the register function to initialize its value, but I'll > remove it from here. > I questioned the dubious usage of ap_matrix->available_instances rather than asking what is the variable for. If I've had this deemed damaging I would have asked if it's damaging in a way I think it is. For example take my comment on 'KVM: s390: interfaces to manage guest's AP matrix'. Regards, Halil >> >> >>> >>>> >>>> >>>> Regards, >>>> Halil >>>> >>>>> + mdev_unregister_device(&ap_matrix->device); >>>>> +} >>> >>> >> >
On 05/15/2018 11:48 AM, Halil Pasic wrote: > > > On 05/15/2018 05:16 PM, Tony Krowiak wrote: >> On 05/15/2018 10:17 AM, Pierre Morel wrote: >>> On 14/05/2018 21:42, Tony Krowiak wrote: >>>> On 05/11/2018 01:18 PM, Halil Pasic wrote: >>>>> >>>>> >>>>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>>>>> Registers the matrix device created by the VFIO AP device >>>>>> driver with the VFIO mediated device framework. >>>>>> Registering the matrix device will create the sysfs >>>>>> structures needed to create mediated matrix devices >>>>>> each of which will be used to configure the AP matrix >>>>>> for a guest and connect it to the VFIO AP device driver. >>>>>> >>>>> [..] >>>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >>>>>> b/drivers/s390/crypto/vfio_ap_ops.c >>>>>> new file mode 100644 >>>>>> index 0000000..d7d36fb >>>>>> --- /dev/null >>>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c >>>>>> @@ -0,0 +1,106 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0+ >>>>>> +/* >>>>>> + * Adjunct processor matrix VFIO device driver callbacks. >>>>>> + * >>>>>> + * Copyright IBM Corp. 2017 >>>>>> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >>>>>> + * >>>>>> + */ >>>>>> +#include <linux/string.h> >>>>>> +#include <linux/vfio.h> >>>>>> +#include <linux/device.h> >>>>>> +#include <linux/list.h> >>>>>> +#include <linux/ctype.h> >>>>>> + >>>>>> +#include "vfio_ap_private.h" >>>>>> + >>>>>> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >>>>>> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >>>>>> + >>>>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >>>>>> mdev_device *mdev) >>>>>> +{ >>>>>> + struct ap_matrix *ap_matrix = >>>>>> to_ap_matrix(mdev_parent_dev(mdev)); >>>>>> + >>>>>> + ap_matrix->available_instances--; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>>>>> +{ >>>>>> + struct ap_matrix *ap_matrix = >>>>>> to_ap_matrix(mdev_parent_dev(mdev)); >>>>>> + >>>>>> + ap_matrix->available_instances++; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>> >>>>> The above functions seem to be called with the lock of this >>>>> auto-generated >>>>> mdev parent device held. That's why we don't have to care about >>>>> synchronization >>>>> ourselves, right? >>>> >>>> I would assume as much. The comments for the 'struct >>>> mdev_parent_ops' in >>>> include/linux/mdev.h do not mention anything about synchronization, >>>> nor did I >>>> see any locking or synchronization in the vfio_ccw implementation >>>> after which >>>> I modeled my code, so frankly it is something I did not consider. >>>> >>>>> >>>>> >>>>> A small comment in the code could be helpful for mdev non-experts. >>>>> Hell, I would >>>>> even consider documenting it for all mdev -- took me some time to >>>>> figure out. >>>> >>>> You may want to bring this up with the VFIO mdev maintainers, but >>>> I'd be happy to >>>> include a comment in the functions in question if you think it >>>> important. >>>> >>>>> >>>>> >>>>> [..] >>>>> >>>>> >>>>>> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >>>>>> +{ >>>>>> + int ret; >>>>>> + >>>>>> + ret = mdev_register_device(&ap_matrix->device, >>>>>> &vfio_ap_matrix_ops); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + ap_matrix->available_instances = >>>>>> AP_MATRIX_MAX_AVAILABLE_INSTANCES; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >>>>>> +{ >>>>>> + ap_matrix->available_instances--; >>>>> >>>>> What is this for? I don't understand. >>>> >>>> To control the number of mediated devices one can create for the >>>> matrix device. >>>> Once the max is reached, the mdev framework will not allow creation >>>> of another >>>> mediated device until one is removed. This counter keeps track of >>>> the number >>>> of instances that can be created. This is documented with the mediated >>>> framework. You may want to take a look at: >>>> >>>> Documentation/vfio-mediated-device.txt >>>> Documentation/vfio.txt >>>> Documentation/virtual/kvm/devices/vfio.txt >>> >>> This is what you do in create/remove. >>> But here in unregister I agree with Halil, it does not seem to be >>> usefull. >> >> If that is in fact what Halil was asking, then I misinterpreted his >> question; I >> thought he was asking what the available_instances was used for. You are >> correct, this does not belong here although it makes little >> difference given >> this is called only when the driver, which creates the matrix device, >> is unloaded. >> It is necessary in the register function to initialize its value, but >> I'll >> remove it from here. >> > > I questioned the dubious usage of ap_matrix->available_instances > rather than > asking what is the variable for. I said I'd remove it. > > > If I've had this deemed damaging I would have asked if it's damaging > in a way > I think it is. For example take my comment on 'KVM: s390: interfaces > to manage > guest's AP matrix'. I apologize for not being able to read your mind. While this is not necessarily necessary, it is not damaging because this is called only when the driver is being unloaded. The point is moot, however, because I am removing it. > > > Regards, > Halil > >>> >>> >>>> >>>>> >>>>> >>>>> Regards, >>>>> Halil >>>>> >>>>>> + mdev_unregister_device(&ap_matrix->device); >>>>>> +} >>>> >>>> >>> >>
On Mon, 7 May 2018 11:11:44 -0400 Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > Registers the matrix device created by the VFIO AP device > driver with the VFIO mediated device framework. > Registering the matrix device will create the sysfs > structures needed to create mediated matrix devices > each of which will be used to configure the AP matrix > for a guest and connect it to the VFIO AP device driver. > > Registering the matrix device with the VFIO mediated device > framework will create the following sysfs structures: > > /sys/devices/vfio_ap > ... [matrix] > ...... [mdev_supported_types] > ......... [vfio_ap-passthrough] > ............ create > > To create a mediated device for the AP matrix device, write a UUID > to the create file: > > uuidgen > create > > A symbolic link to the mediated device's directory will be created in the > devices subdirectory named after the generated $uuid: > > /sys/devices/vfio_ap > ... [matrix] > ...... [mdev_supported_types] > ......... [vfio_ap-passthrough] > ............ [devices] > ............... [$uuid] > > Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> > --- > MAINTAINERS | 1 + > drivers/s390/crypto/Makefile | 2 +- > drivers/s390/crypto/vfio_ap_drv.c | 9 +++ > drivers/s390/crypto/vfio_ap_ops.c | 106 +++++++++++++++++++++++++++++++++ > drivers/s390/crypto/vfio_ap_private.h | 17 +++++ > 5 files changed, 134 insertions(+), 1 deletions(-) > create mode 100644 drivers/s390/crypto/vfio_ap_ops.c > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > new file mode 100644 > index 0000000..d7d36fb > --- /dev/null > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -0,0 +1,106 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Adjunct processor matrix VFIO device driver callbacks. > + * > + * Copyright IBM Corp. 2017 Should be '2018' (also in some other files in this series; please double check.) > + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> > + * > + */ > +#include <linux/string.h> > +#include <linux/vfio.h> > +#include <linux/device.h> > +#include <linux/list.h> > +#include <linux/ctype.h> > + > +#include "vfio_ap_private.h" > + > +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" > +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" > + > +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) > +{ > + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > + > + ap_matrix->available_instances--; Shouldn't the code check whether available_instances is actually > 0? > + > + return 0; > +} > + > +static int vfio_ap_mdev_remove(struct mdev_device *mdev) > +{ > + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > + > + ap_matrix->available_instances++; > + > + return 0; > +} > + > +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf) > +{ > + return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT); > +} > + > +MDEV_TYPE_ATTR_RO(name); > + > +static ssize_t available_instances_show(struct kobject *kobj, > + struct device *dev, char *buf) > +{ > + struct ap_matrix *ap_matrix; > + > + ap_matrix = to_ap_matrix(dev); Move this with the declaration? > + > + return sprintf(buf, "%d\n", ap_matrix->available_instances); > +} > + > +MDEV_TYPE_ATTR_RO(available_instances); > + > +static ssize_t device_api_show(struct kobject *kobj, struct device *dev, > + char *buf) > +{ > + return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING); > +} > + > +MDEV_TYPE_ATTR_RO(device_api); > + > +static struct attribute *vfio_ap_mdev_type_attrs[] = { > + &mdev_type_attr_name.attr, > + &mdev_type_attr_device_api.attr, > + &mdev_type_attr_available_instances.attr, > + NULL, > +}; > + > +static struct attribute_group vfio_ap_mdev_hwvirt_type_group = { > + .name = VFOP_AP_MDEV_TYPE_HWVIRT, > + .attrs = vfio_ap_mdev_type_attrs, > +}; > + > +static struct attribute_group *vfio_ap_mdev_type_groups[] = { > + &vfio_ap_mdev_hwvirt_type_group, > + NULL, > +}; > + > +static const struct mdev_parent_ops vfio_ap_matrix_ops = { > + .owner = THIS_MODULE, > + .supported_type_groups = vfio_ap_mdev_type_groups, > + .create = vfio_ap_mdev_create, > + .remove = vfio_ap_mdev_remove, > +}; > + > +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) > +{ > + int ret; > + > + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); > + if (ret) > + return ret; > + > + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; > + > + return 0; > +} > + > +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) > +{ > + ap_matrix->available_instances--; > + mdev_unregister_device(&ap_matrix->device); > +} > diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h > index cf23675..afd8dbc 100644 > --- a/drivers/s390/crypto/vfio_ap_private.h > +++ b/drivers/s390/crypto/vfio_ap_private.h > @@ -10,14 +10,31 @@ > #define _VFIO_AP_PRIVATE_H_ > > #include <linux/types.h> > +#include <linux/device.h> > +#include <linux/mdev.h> > > #include "ap_bus.h" > > #define VFIO_AP_MODULE_NAME "vfio_ap" > #define VFIO_AP_DRV_NAME "vfio_ap" > +/** > + * There must be one mediated matrix device per guest. If every APQN is assigned One, or at most one? Or one for every guest using ap devices? > + * to a guest, then the maximum number of guests with a unique APQN assigned > + * would be 255 adapters x 255 domains = 72351 guests. > + */ > +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES 72351 > > struct ap_matrix { > struct device device; > + int available_instances; > }; > > +static inline struct ap_matrix *to_ap_matrix(struct device *dev) > +{ > + return container_of(dev, struct ap_matrix, device); > +} > + > +extern int vfio_ap_mdev_register(struct ap_matrix *ap_matrix); > +extern void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix); > + > #endif /* _VFIO_AP_PRIVATE_H_ */
On 05/16/2018 06:42 AM, Cornelia Huck wrote: > On Mon, 7 May 2018 11:11:44 -0400 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > >> Registers the matrix device created by the VFIO AP device >> driver with the VFIO mediated device framework. >> Registering the matrix device will create the sysfs >> structures needed to create mediated matrix devices >> each of which will be used to configure the AP matrix >> for a guest and connect it to the VFIO AP device driver. >> >> Registering the matrix device with the VFIO mediated device >> framework will create the following sysfs structures: >> >> /sys/devices/vfio_ap >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [vfio_ap-passthrough] >> ............ create >> >> To create a mediated device for the AP matrix device, write a UUID >> to the create file: >> >> uuidgen > create >> >> A symbolic link to the mediated device's directory will be created in the >> devices subdirectory named after the generated $uuid: >> >> /sys/devices/vfio_ap >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [vfio_ap-passthrough] >> ............ [devices] >> ............... [$uuid] >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> MAINTAINERS | 1 + >> drivers/s390/crypto/Makefile | 2 +- >> drivers/s390/crypto/vfio_ap_drv.c | 9 +++ >> drivers/s390/crypto/vfio_ap_ops.c | 106 +++++++++++++++++++++++++++++++++ >> drivers/s390/crypto/vfio_ap_private.h | 17 +++++ >> 5 files changed, 134 insertions(+), 1 deletions(-) >> create mode 100644 drivers/s390/crypto/vfio_ap_ops.c > >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> new file mode 100644 >> index 0000000..d7d36fb >> --- /dev/null >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -0,0 +1,106 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Adjunct processor matrix VFIO device driver callbacks. >> + * >> + * Copyright IBM Corp. 2017 > Should be '2018' (also in some other files in this series; please > double check.) Gee, I thought I got all of these fixed. I must have a gremlin in my laptop. > >> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> + * >> + */ >> +#include <linux/string.h> >> +#include <linux/vfio.h> >> +#include <linux/device.h> >> +#include <linux/list.h> >> +#include <linux/ctype.h> >> + >> +#include "vfio_ap_private.h" >> + >> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >> + >> +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances--; > Shouldn't the code check whether available_instances is actually > 0? It is my understanding that once the available_instances hits zero, the mdev framework will not allow any more mediated devices to be created, so the value should always be greater than zero when this function is invoked. I did an experiment to verify my understanding. I initialized the available_instances to 1. I was able to create 2 mediated devices. It seems that the framework refuses to create a mediated device only after the available_instances sysfs attribute is a negative number, so I have two choices: Initialize available_instances to one less than desires; add the check you suggested. I think I'll go with the latter. > >> + >> + return 0; >> +} >> + >> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances++; >> + >> + return 0; >> +} >> + >> +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf) >> +{ >> + return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT); >> +} >> + >> +MDEV_TYPE_ATTR_RO(name); >> + >> +static ssize_t available_instances_show(struct kobject *kobj, >> + struct device *dev, char *buf) >> +{ >> + struct ap_matrix *ap_matrix; >> + >> + ap_matrix = to_ap_matrix(dev); > Move this with the declaration? > >> + >> + return sprintf(buf, "%d\n", ap_matrix->available_instances); >> +} >> + >> +MDEV_TYPE_ATTR_RO(available_instances); >> + >> +static ssize_t device_api_show(struct kobject *kobj, struct device *dev, >> + char *buf) >> +{ >> + return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING); >> +} >> + >> +MDEV_TYPE_ATTR_RO(device_api); >> + >> +static struct attribute *vfio_ap_mdev_type_attrs[] = { >> + &mdev_type_attr_name.attr, >> + &mdev_type_attr_device_api.attr, >> + &mdev_type_attr_available_instances.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group vfio_ap_mdev_hwvirt_type_group = { >> + .name = VFOP_AP_MDEV_TYPE_HWVIRT, >> + .attrs = vfio_ap_mdev_type_attrs, >> +}; >> + >> +static struct attribute_group *vfio_ap_mdev_type_groups[] = { >> + &vfio_ap_mdev_hwvirt_type_group, >> + NULL, >> +}; >> + >> +static const struct mdev_parent_ops vfio_ap_matrix_ops = { >> + .owner = THIS_MODULE, >> + .supported_type_groups = vfio_ap_mdev_type_groups, >> + .create = vfio_ap_mdev_create, >> + .remove = vfio_ap_mdev_remove, >> +}; >> + >> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >> +{ >> + int ret; >> + >> + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); >> + if (ret) >> + return ret; >> + >> + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; >> + >> + return 0; >> +} >> + >> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >> +{ >> + ap_matrix->available_instances--; >> + mdev_unregister_device(&ap_matrix->device); >> +} >> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h >> index cf23675..afd8dbc 100644 >> --- a/drivers/s390/crypto/vfio_ap_private.h >> +++ b/drivers/s390/crypto/vfio_ap_private.h >> @@ -10,14 +10,31 @@ >> #define _VFIO_AP_PRIVATE_H_ >> >> #include <linux/types.h> >> +#include <linux/device.h> >> +#include <linux/mdev.h> >> >> #include "ap_bus.h" >> >> #define VFIO_AP_MODULE_NAME "vfio_ap" >> #define VFIO_AP_DRV_NAME "vfio_ap" >> +/** >> + * There must be one mediated matrix device per guest. If every APQN is assigned > One, or at most one? Or one for every guest using ap devices? > >> + * to a guest, then the maximum number of guests with a unique APQN assigned >> + * would be 255 adapters x 255 domains = 72351 guests. >> + */ >> +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES 72351 >> >> struct ap_matrix { >> struct device device; >> + int available_instances; >> }; >> >> +static inline struct ap_matrix *to_ap_matrix(struct device *dev) >> +{ >> + return container_of(dev, struct ap_matrix, device); >> +} >> + >> +extern int vfio_ap_mdev_register(struct ap_matrix *ap_matrix); >> +extern void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix); >> + >> #endif /* _VFIO_AP_PRIVATE_H_ */
On 05/16/2018 06:42 AM, Cornelia Huck wrote: > On Mon, 7 May 2018 11:11:44 -0400 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > >> Registers the matrix device created by the VFIO AP device >> driver with the VFIO mediated device framework. >> Registering the matrix device will create the sysfs >> structures needed to create mediated matrix devices >> each of which will be used to configure the AP matrix >> for a guest and connect it to the VFIO AP device driver. >> >> Registering the matrix device with the VFIO mediated device >> framework will create the following sysfs structures: >> >> /sys/devices/vfio_ap >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [vfio_ap-passthrough] >> ............ create >> >> To create a mediated device for the AP matrix device, write a UUID >> to the create file: >> >> uuidgen > create >> >> A symbolic link to the mediated device's directory will be created in the >> devices subdirectory named after the generated $uuid: >> >> /sys/devices/vfio_ap >> ... [matrix] >> ...... [mdev_supported_types] >> ......... [vfio_ap-passthrough] >> ............ [devices] >> ............... [$uuid] >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> --- >> MAINTAINERS | 1 + >> drivers/s390/crypto/Makefile | 2 +- >> drivers/s390/crypto/vfio_ap_drv.c | 9 +++ >> drivers/s390/crypto/vfio_ap_ops.c | 106 +++++++++++++++++++++++++++++++++ >> drivers/s390/crypto/vfio_ap_private.h | 17 +++++ >> 5 files changed, 134 insertions(+), 1 deletions(-) >> create mode 100644 drivers/s390/crypto/vfio_ap_ops.c > >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c >> new file mode 100644 >> index 0000000..d7d36fb >> --- /dev/null >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -0,0 +1,106 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Adjunct processor matrix VFIO device driver callbacks. >> + * >> + * Copyright IBM Corp. 2017 > Should be '2018' (also in some other files in this series; please > double check.) > >> + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> >> + * >> + */ >> +#include <linux/string.h> >> +#include <linux/vfio.h> >> +#include <linux/device.h> >> +#include <linux/list.h> >> +#include <linux/ctype.h> >> + >> +#include "vfio_ap_private.h" >> + >> +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" >> +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" >> + >> +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances--; > Shouldn't the code check whether available_instances is actually > 0? Disregard my last response to this, I'm an idiot; it is the return of a negative number from this function that causes the create to fail. > >> + >> + return 0; >> +} >> + >> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >> +{ >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >> + >> + ap_matrix->available_instances++; >> + >> + return 0; >> +} >> + >> +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf) >> +{ >> + return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT); >> +} >> + >> +MDEV_TYPE_ATTR_RO(name); >> + >> +static ssize_t available_instances_show(struct kobject *kobj, >> + struct device *dev, char *buf) >> +{ >> + struct ap_matrix *ap_matrix; >> + >> + ap_matrix = to_ap_matrix(dev); > Move this with the declaration? Okay, will do. > >> + >> + return sprintf(buf, "%d\n", ap_matrix->available_instances); >> +} >> + >> +MDEV_TYPE_ATTR_RO(available_instances); >> + >> +static ssize_t device_api_show(struct kobject *kobj, struct device *dev, >> + char *buf) >> +{ >> + return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING); >> +} >> + >> +MDEV_TYPE_ATTR_RO(device_api); >> + >> +static struct attribute *vfio_ap_mdev_type_attrs[] = { >> + &mdev_type_attr_name.attr, >> + &mdev_type_attr_device_api.attr, >> + &mdev_type_attr_available_instances.attr, >> + NULL, >> +}; >> + >> +static struct attribute_group vfio_ap_mdev_hwvirt_type_group = { >> + .name = VFOP_AP_MDEV_TYPE_HWVIRT, >> + .attrs = vfio_ap_mdev_type_attrs, >> +}; >> + >> +static struct attribute_group *vfio_ap_mdev_type_groups[] = { >> + &vfio_ap_mdev_hwvirt_type_group, >> + NULL, >> +}; >> + >> +static const struct mdev_parent_ops vfio_ap_matrix_ops = { >> + .owner = THIS_MODULE, >> + .supported_type_groups = vfio_ap_mdev_type_groups, >> + .create = vfio_ap_mdev_create, >> + .remove = vfio_ap_mdev_remove, >> +}; >> + >> +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) >> +{ >> + int ret; >> + >> + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); >> + if (ret) >> + return ret; >> + >> + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; >> + >> + return 0; >> +} >> + >> +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) >> +{ >> + ap_matrix->available_instances--; >> + mdev_unregister_device(&ap_matrix->device); >> +} >> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h >> index cf23675..afd8dbc 100644 >> --- a/drivers/s390/crypto/vfio_ap_private.h >> +++ b/drivers/s390/crypto/vfio_ap_private.h >> @@ -10,14 +10,31 @@ >> #define _VFIO_AP_PRIVATE_H_ >> >> #include <linux/types.h> >> +#include <linux/device.h> >> +#include <linux/mdev.h> >> >> #include "ap_bus.h" >> >> #define VFIO_AP_MODULE_NAME "vfio_ap" >> #define VFIO_AP_DRV_NAME "vfio_ap" >> +/** >> + * There must be one mediated matrix device per guest. If every APQN is assigned > One, or at most one? Or one for every guest using ap devices? 'One mediated matrix device for every guest using AP devices' is most accurate. I'll make the change. > >> + * to a guest, then the maximum number of guests with a unique APQN assigned >> + * would be 255 adapters x 255 domains = 72351 guests. >> + */ >> +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES 72351 >> >> struct ap_matrix { >> struct device device; >> + int available_instances; >> }; >> >> +static inline struct ap_matrix *to_ap_matrix(struct device *dev) >> +{ >> + return container_of(dev, struct ap_matrix, device); >> +} >> + >> +extern int vfio_ap_mdev_register(struct ap_matrix *ap_matrix); >> +extern void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix); >> + >> #endif /* _VFIO_AP_PRIVATE_H_ */
On Mon, 14 May 2018 15:42:18 -0400 Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > On 05/11/2018 01:18 PM, Halil Pasic wrote: > > > > > > On 05/07/2018 05:11 PM, Tony Krowiak wrote: > >> Registers the matrix device created by the VFIO AP device > >> driver with the VFIO mediated device framework. > >> Registering the matrix device will create the sysfs > >> structures needed to create mediated matrix devices > >> each of which will be used to configure the AP matrix > >> for a guest and connect it to the VFIO AP device driver. > >> +static int vfio_ap_mdev_create(struct kobject *kobj, struct > >> mdev_device *mdev) > >> +{ > >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > >> + > >> + ap_matrix->available_instances--; > >> + > >> + return 0; > >> +} > >> + > >> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) > >> +{ > >> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > >> + > >> + ap_matrix->available_instances++; > >> + > >> + return 0; > >> +} > >> + > > > > The above functions seem to be called with the lock of this > > auto-generated > > mdev parent device held. That's why we don't have to care about > > synchronization > > ourselves, right? > > I would assume as much. The comments for the 'struct mdev_parent_ops' in > include/linux/mdev.h do not mention anything about synchronization, nor > did I > see any locking or synchronization in the vfio_ccw implementation after > which > I modeled my code, so frankly it is something I did not consider. > > > > > > > A small comment in the code could be helpful for mdev non-experts. > > Hell, I would > > even consider documenting it for all mdev -- took me some time to > > figure out. > > You may want to bring this up with the VFIO mdev maintainers, but I'd be > happy to > include a comment in the functions in question if you think it important. Important note: There's currently a patch on list that removes the mdev parent mutex, and it seems there was never intended to be any serialization in that place by the mdev core. (Look for "vfio/mdev: Check globally for duplicate devices".)
On 05/17/2018 03:44 AM, Cornelia Huck wrote: > On Mon, 14 May 2018 15:42:18 -0400 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > >> On 05/11/2018 01:18 PM, Halil Pasic wrote: >>> >>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>>> Registers the matrix device created by the VFIO AP device >>>> driver with the VFIO mediated device framework. >>>> Registering the matrix device will create the sysfs >>>> structures needed to create mediated matrix devices >>>> each of which will be used to configure the AP matrix >>>> for a guest and connect it to the VFIO AP device driver. >>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >>>> mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>> + >>>> + ap_matrix->available_instances--; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>>> +{ >>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>> + >>>> + ap_matrix->available_instances++; >>>> + >>>> + return 0; >>>> +} >>>> + >>> The above functions seem to be called with the lock of this >>> auto-generated >>> mdev parent device held. That's why we don't have to care about >>> synchronization >>> ourselves, right? >> I would assume as much. The comments for the 'struct mdev_parent_ops' in >> include/linux/mdev.h do not mention anything about synchronization, nor >> did I >> see any locking or synchronization in the vfio_ccw implementation after >> which >> I modeled my code, so frankly it is something I did not consider. >> >>> >>> A small comment in the code could be helpful for mdev non-experts. >>> Hell, I would >>> even consider documenting it for all mdev -- took me some time to >>> figure out. >> You may want to bring this up with the VFIO mdev maintainers, but I'd be >> happy to >> include a comment in the functions in question if you think it important. > Important note: There's currently a patch on list that removes the mdev > parent mutex, and it seems there was never intended to be any > serialization in that place by the mdev core. (Look for "vfio/mdev: > Check globally for duplicate devices".) The patch on the list holds the mdev_list_lock during create and remove of an mdev device, so it looks like no synchronization is necessary on the part of the vendor code in the create/remove callbacks; does that sound about right? >
On Mon, 21 May 2018 11:13:58 -0400 Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > On 05/17/2018 03:44 AM, Cornelia Huck wrote: > > On Mon, 14 May 2018 15:42:18 -0400 > > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > > > >> On 05/11/2018 01:18 PM, Halil Pasic wrote: > >>> > >>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: > >>>> Registers the matrix device created by the VFIO AP device > >>>> driver with the VFIO mediated device framework. > >>>> Registering the matrix device will create the sysfs > >>>> structures needed to create mediated matrix devices > >>>> each of which will be used to configure the AP matrix > >>>> for a guest and connect it to the VFIO AP device driver. > >>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct > >>>> mdev_device *mdev) > >>>> +{ > >>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > >>>> + > >>>> + ap_matrix->available_instances--; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) > >>>> +{ > >>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); > >>>> + > >>>> + ap_matrix->available_instances++; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>> The above functions seem to be called with the lock of this > >>> auto-generated > >>> mdev parent device held. That's why we don't have to care about > >>> synchronization > >>> ourselves, right? > >> I would assume as much. The comments for the 'struct mdev_parent_ops' in > >> include/linux/mdev.h do not mention anything about synchronization, nor > >> did I > >> see any locking or synchronization in the vfio_ccw implementation after > >> which > >> I modeled my code, so frankly it is something I did not consider. > >> > >>> > >>> A small comment in the code could be helpful for mdev non-experts. > >>> Hell, I would > >>> even consider documenting it for all mdev -- took me some time to > >>> figure out. > >> You may want to bring this up with the VFIO mdev maintainers, but I'd be > >> happy to > >> include a comment in the functions in question if you think it important. > > Important note: There's currently a patch on list that removes the mdev > > parent mutex, and it seems there was never intended to be any > > serialization in that place by the mdev core. (Look for "vfio/mdev: > > Check globally for duplicate devices".) > > The patch on the list holds the mdev_list_lock during create and remove > of an mdev device, so it looks like no synchronization is necessary on the > part of the vendor code in the create/remove callbacks; does that sound > about right? v1/v2 did that; v3/v4 hold the list lock only while the device is added to the mdev list. v4 also adds a note regarding locking to the documentation.
On 05/22/2018 04:19 AM, Cornelia Huck wrote: > On Mon, 21 May 2018 11:13:58 -0400 > Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: > >> On 05/17/2018 03:44 AM, Cornelia Huck wrote: >>> On Mon, 14 May 2018 15:42:18 -0400 >>> Tony Krowiak <akrowiak@linux.vnet.ibm.com> wrote: >>> >>>> On 05/11/2018 01:18 PM, Halil Pasic wrote: >>>>> On 05/07/2018 05:11 PM, Tony Krowiak wrote: >>>>>> Registers the matrix device created by the VFIO AP device >>>>>> driver with the VFIO mediated device framework. >>>>>> Registering the matrix device will create the sysfs >>>>>> structures needed to create mediated matrix devices >>>>>> each of which will be used to configure the AP matrix >>>>>> for a guest and connect it to the VFIO AP device driver. >>>>>> +static int vfio_ap_mdev_create(struct kobject *kobj, struct >>>>>> mdev_device *mdev) >>>>>> +{ >>>>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>>>> + >>>>>> + ap_matrix->available_instances--; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int vfio_ap_mdev_remove(struct mdev_device *mdev) >>>>>> +{ >>>>>> + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); >>>>>> + >>>>>> + ap_matrix->available_instances++; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>> The above functions seem to be called with the lock of this >>>>> auto-generated >>>>> mdev parent device held. That's why we don't have to care about >>>>> synchronization >>>>> ourselves, right? >>>> I would assume as much. The comments for the 'struct mdev_parent_ops' in >>>> include/linux/mdev.h do not mention anything about synchronization, nor >>>> did I >>>> see any locking or synchronization in the vfio_ccw implementation after >>>> which >>>> I modeled my code, so frankly it is something I did not consider. >>>> >>>>> A small comment in the code could be helpful for mdev non-experts. >>>>> Hell, I would >>>>> even consider documenting it for all mdev -- took me some time to >>>>> figure out. >>>> You may want to bring this up with the VFIO mdev maintainers, but I'd be >>>> happy to >>>> include a comment in the functions in question if you think it important. >>> Important note: There's currently a patch on list that removes the mdev >>> parent mutex, and it seems there was never intended to be any >>> serialization in that place by the mdev core. (Look for "vfio/mdev: >>> Check globally for duplicate devices".) >> The patch on the list holds the mdev_list_lock during create and remove >> of an mdev device, so it looks like no synchronization is necessary on the >> part of the vendor code in the create/remove callbacks; does that sound >> about right? > v1/v2 did that; v3/v4 hold the list lock only while the device is added > to the mdev list. v4 also adds a note regarding locking to the > documentation. I'll add some synchronization to the read/update of available instances. >
diff --git a/MAINTAINERS b/MAINTAINERS index 2792c81..cecadf2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12246,6 +12246,7 @@ 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 +F: drivers/s390/crypto/vfio_ap_ops.c S390 ZFCP DRIVER M: Steffen Maier <maier@linux.ibm.com> diff --git a/drivers/s390/crypto/Makefile b/drivers/s390/crypto/Makefile index 48e466e..8d36b05 100644 --- a/drivers/s390/crypto/Makefile +++ b/drivers/s390/crypto/Makefile @@ -17,5 +17,5 @@ pkey-objs := pkey_api.o obj-$(CONFIG_PKEY) += pkey.o # adjunct processor matrix -vfio_ap-objs := vfio_ap_drv.o +vfio_ap-objs := vfio_ap_drv.o vfio_ap_ops.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 index 014d70f..cc7fbd7 100644 --- a/drivers/s390/crypto/vfio_ap_drv.c +++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -121,11 +121,20 @@ int __init vfio_ap_init(void) return ret; } + ret = vfio_ap_mdev_register(ap_matrix); + if (ret) { + ap_driver_unregister(&vfio_ap_drv); + vfio_ap_matrix_dev_destroy(ap_matrix); + + return ret; + } + return 0; } void __exit vfio_ap_exit(void) { + vfio_ap_mdev_unregister(ap_matrix); ap_driver_unregister(&vfio_ap_drv); vfio_ap_matrix_dev_destroy(ap_matrix); } diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c new file mode 100644 index 0000000..d7d36fb --- /dev/null +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -0,0 +1,106 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Adjunct processor matrix VFIO device driver callbacks. + * + * Copyright IBM Corp. 2017 + * Author(s): Tony Krowiak <akrowiak@linux.vnet.ibm.com> + * + */ +#include <linux/string.h> +#include <linux/vfio.h> +#include <linux/device.h> +#include <linux/list.h> +#include <linux/ctype.h> + +#include "vfio_ap_private.h" + +#define VFOP_AP_MDEV_TYPE_HWVIRT "passthrough" +#define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device" + +static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev) +{ + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); + + ap_matrix->available_instances--; + + return 0; +} + +static int vfio_ap_mdev_remove(struct mdev_device *mdev) +{ + struct ap_matrix *ap_matrix = to_ap_matrix(mdev_parent_dev(mdev)); + + ap_matrix->available_instances++; + + return 0; +} + +static ssize_t name_show(struct kobject *kobj, struct device *dev, char *buf) +{ + return sprintf(buf, "%s\n", VFIO_AP_MDEV_NAME_HWVIRT); +} + +MDEV_TYPE_ATTR_RO(name); + +static ssize_t available_instances_show(struct kobject *kobj, + struct device *dev, char *buf) +{ + struct ap_matrix *ap_matrix; + + ap_matrix = to_ap_matrix(dev); + + return sprintf(buf, "%d\n", ap_matrix->available_instances); +} + +MDEV_TYPE_ATTR_RO(available_instances); + +static ssize_t device_api_show(struct kobject *kobj, struct device *dev, + char *buf) +{ + return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING); +} + +MDEV_TYPE_ATTR_RO(device_api); + +static struct attribute *vfio_ap_mdev_type_attrs[] = { + &mdev_type_attr_name.attr, + &mdev_type_attr_device_api.attr, + &mdev_type_attr_available_instances.attr, + NULL, +}; + +static struct attribute_group vfio_ap_mdev_hwvirt_type_group = { + .name = VFOP_AP_MDEV_TYPE_HWVIRT, + .attrs = vfio_ap_mdev_type_attrs, +}; + +static struct attribute_group *vfio_ap_mdev_type_groups[] = { + &vfio_ap_mdev_hwvirt_type_group, + NULL, +}; + +static const struct mdev_parent_ops vfio_ap_matrix_ops = { + .owner = THIS_MODULE, + .supported_type_groups = vfio_ap_mdev_type_groups, + .create = vfio_ap_mdev_create, + .remove = vfio_ap_mdev_remove, +}; + +int vfio_ap_mdev_register(struct ap_matrix *ap_matrix) +{ + int ret; + + ret = mdev_register_device(&ap_matrix->device, &vfio_ap_matrix_ops); + if (ret) + return ret; + + ap_matrix->available_instances = AP_MATRIX_MAX_AVAILABLE_INSTANCES; + + return 0; +} + +void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix) +{ + ap_matrix->available_instances--; + mdev_unregister_device(&ap_matrix->device); +} diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index cf23675..afd8dbc 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -10,14 +10,31 @@ #define _VFIO_AP_PRIVATE_H_ #include <linux/types.h> +#include <linux/device.h> +#include <linux/mdev.h> #include "ap_bus.h" #define VFIO_AP_MODULE_NAME "vfio_ap" #define VFIO_AP_DRV_NAME "vfio_ap" +/** + * There must be one mediated matrix device per guest. If every APQN is assigned + * to a guest, then the maximum number of guests with a unique APQN assigned + * would be 255 adapters x 255 domains = 72351 guests. + */ +#define AP_MATRIX_MAX_AVAILABLE_INSTANCES 72351 struct ap_matrix { struct device device; + int available_instances; }; +static inline struct ap_matrix *to_ap_matrix(struct device *dev) +{ + return container_of(dev, struct ap_matrix, device); +} + +extern int vfio_ap_mdev_register(struct ap_matrix *ap_matrix); +extern void vfio_ap_mdev_unregister(struct ap_matrix *ap_matrix); + #endif /* _VFIO_AP_PRIVATE_H_ */
Registers the matrix device created by the VFIO AP device driver with the VFIO mediated device framework. Registering the matrix device will create the sysfs structures needed to create mediated matrix devices each of which will be used to configure the AP matrix for a guest and connect it to the VFIO AP device driver. Registering the matrix device with the VFIO mediated device framework will create the following sysfs structures: /sys/devices/vfio_ap ... [matrix] ...... [mdev_supported_types] ......... [vfio_ap-passthrough] ............ create To create a mediated device for the AP matrix device, write a UUID to the create file: uuidgen > create A symbolic link to the mediated device's directory will be created in the devices subdirectory named after the generated $uuid: /sys/devices/vfio_ap ... [matrix] ...... [mdev_supported_types] ......... [vfio_ap-passthrough] ............ [devices] ............... [$uuid] Signed-off-by: Tony Krowiak <akrowiak@linux.vnet.ibm.com> --- MAINTAINERS | 1 + drivers/s390/crypto/Makefile | 2 +- drivers/s390/crypto/vfio_ap_drv.c | 9 +++ drivers/s390/crypto/vfio_ap_ops.c | 106 +++++++++++++++++++++++++++++++++ drivers/s390/crypto/vfio_ap_private.h | 17 +++++ 5 files changed, 134 insertions(+), 1 deletions(-) create mode 100644 drivers/s390/crypto/vfio_ap_ops.c