Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Reply-To: jjherne@linux.ibm.com
Subject: Re: [PATCH v4 2/2] s390/vfio-ap: control access to PQAP(AQIC)
 interception handler
To:     Tony Krowiak <akrowiak@linux.ibm.com>, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     borntraeger@de.ibm.com, cohuck@redhat.com,
        pasic@linux.vnet.ibm.com, jgg@nvidia.com,
        alex.williamson@redhat.com, kwankhede@nvidia.com,
        frankja@linux.ibm.com, david@redhat.com, imbrenda@linux.ibm.com,
        hca@linux.ibm.com
References: <20210521193648.940864-1-akrowiak@linux.ibm.com>
 <20210521193648.940864-3-akrowiak@linux.ibm.com>
From:   "Jason J. Herne" <jjherne@linux.ibm.com>
Organization: IBM
Message-ID: <5d15fdf2-aee8-4e6c-c3e1-f07c76ce5974@linux.ibm.com>
Date:   Mon, 24 May 2021 10:37:29 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.0
MIME-Version: 1.0
In-Reply-To: <20210521193648.940864-3-akrowiak@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/21/21 3:36 PM, Tony Krowiak wrote:
> The function pointer to the handler that processes interception of the
> PQAP instruction is contained in the mdev. If the mdev is removed and
> its storage de-allocated during the processing of the PQAP instruction,
> the function pointer could get wiped out before the function is called
> because there is currently nothing that controls access to it.
> 
> This patch introduces two new functions:
> * The kvm_arch_crypto_register_hook() function registers a function pointer
>    for processing intercepted crypto instructions.
> * The kvm_arch_crypto_register_hook() function un-registers a function
>    pointer that was previously registered.

Typo: You meant kvm_arch_crypto_UNregister_hook() in the second bullet.


Just one overall observation on this one. The whole hook system seems kind of 
over-engineered if this is our only use for it. It looks like a kvm_s390_crypto_hook is 
meant to link a specific module with a function pointer. Do we really need this concept?

I think a simpler design could be to just place a mutex and a function pointer in the 
kvm_s390_crypto struct. Then you can grab the mutex in vfio_ap_ops.c when 
registering/unregistering. You would also grab the mutex in priv.c when calling the 
function pointer. What I am suggesting is essentially the exact same scheme you have 
implemented here, but simpler and with less infrastructure.

With that said, I'll point out that I am relative new to this code (and this patch series) 
so maybe I've missed something and the extra complexity is needed for some reason. But if 
it is not, I'm all in favor of keeping things simple.

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)