Re: [PATCH v2 05/15] s390: vfio-ap: base implementation of VFIO AP device driver

[Cornelia Huck]

On Wed, 28 Feb 2018 11:43:37 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxxxxxxx> wrote:

> On 02/28/2018 10:33 AM, Pierre Morel wrote:
> > On 27/02/2018 15:28, Tony Krowiak wrote:  

(...)

> >> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> >> index cbe1d97..9f23caf 100644
> >> --- a/arch/s390/Kconfig
> >> +++ b/arch/s390/Kconfig
> >> @@ -771,6 +771,14 @@ 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 m

Any reason you default to m instead of n here?

> >> +    prompt "Support for virtual Adjunct Processor device interface"  
> >
> > The VFIO AP devices are not virtual.
> > What about
> >     "VFIO support for AP devices"  
> Sounds good.

+1

> >  
> >> +    depends on ZCRYPT && VFIO_MDEV_DEVICE
> >> +    help
> >> +        driver grants access to Adjunct Processor (AP) devices

s/driver/This driver/

> >> +        via the VFIO mediated device interface.

You also might want to add

"To compile this driver as a module, choose M here: the
module will be called..."

> >> +
> >>   endmenu

It's a tad confusing to find this in the I/O submenu, but I don't
really have a better idea.

> >>
> >>   menu "Dump support"
> >> diff --git a/arch/s390/configs/default_defconfig 
> >> b/arch/s390/configs/default_defconfig
> >> index 5af8458..40fa3f6 100644
> >> --- a/arch/s390/configs/default_defconfig
> >> +++ b/arch/s390/configs/default_defconfig  
> >
> > Not sure that this file belongs to this patch  
> Neither am I, but at the time I inserted this here - well before August 
> of last year - I was using vfio-ccw as a model.
> If someone can verify this does not belong here, I'd be more than happy 
> to remove it.

I don't see any entry for VFIO_CCW in there?

> >
> >  
> >> @@ -719,3 +719,6 @@ CONFIG_APPLDATA_BASE=y
> >>   CONFIG_KVM=m
> >>   CONFIG_KVM_S390_UCONTROL=y
> >>   CONFIG_VHOST_NET=m
> >> +VFIO_MDEV=m
> >> +VFIO_MDEV_DEVICE=m
> >> +CONFIG_VFIO_AP=m  
> >
> > What is your goal when modifying this three files?
> > Could you add a comment in the commit message?  
> As stated above, this was originally based on the vfio-ccw model and has 
> been in the
> patch series since its inception. I'd be happy to remove it if it is not 
> necessary.

I'd vote for removing it.

(...)

> >> +static int vfio_ap_matrix_dev_create(void)
> >> +{
> >> +    int ret;
> >> +
> >> +    vfio_ap_root_device = root_device_register(VFIO_AP_ROOT_NAME);
> >> +
> >> +    ret = PTR_ERR_OR_ZERO(vfio_ap_root_device);  
> >
> > IS_ERR() is enough, root_device_register() never return NULL.  
> I searched the kernel code to look at other places the 
> root_device_register()
> function is called to see how the return value is handled. I've seen all 
> of the
> following used:
> if (IS_ERR())
>      ret = PTR_ERR()
> PTR_ERR()
> PTR_ERR_OR_ZERO()
> 
> I'm not sure why this is a concern, but I'll use the first option above
> since PTR_ERR_OR_ZERO() also embeds the first option.

PTR_ERR_OR_ZERO() seems like the best choice for the way the return
code is processed here. (It's just unfortunate that its name conjures
up connotations of NULL-pointer handling.)

> >> +    if (ret)
> >> +        goto done;