On 30/07/2018 11:49, Cornelia Huck wrote:
On Fri, 27 Jul 2018 16:17:47 +0200
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
On 07/26/2018 09:54 PM, Christian Borntraeger wrote:
@@ -65,6 +66,21 @@ static int vfio_ap_matrix_dev_create(void)
{
int ret;
+ mutex_init(&matrix_dev.lock);
+ INIT_LIST_HEAD(&matrix_dev.mdev_list);
+
+ /* Test if PQAP(QCI) instruction is available */
+ if (test_facility(12)) {
+ ret = ap_qci(&matrix_dev.info);
+ if (ret && (ret != -EOPNOTSUPP)) {
After Connie's curiosity was piqued I gave this another look. If
I read the ap_qci() documentation and code correctly, it can return
either 0 or -EOPNOTSUPP, but nothing else. So basically this
is a dead branch.
Can it return -EOPNOTSUPP if facility 12 is present?
I do not think it is reasonable to continue if
we stated that facility 12 is present but the ap_qci function failed.
So I propose that we return an error and break the insmod.
Regards,
Pierre
Either removing the && (ret != -EOPNOTSUPP) and adding some fancy
error reporting, or removing the check alltoghether would make
more sense than what we have right now. Should we do something?
Does not having PQAP(QCI) available have any drawbacks, function-wise?
If yes, we should probably log something; if not, I don't think it needs
logging.
But don't misunderstand me, what we have right now is safe.
Yes, with the current code. We might also want to moan on any return
value that is not either 0 or -EOPNOTSUPP so that we notice early if
ap_qci() handling changed.
+ vfio_ap_mdev_unregister();
+
+ return ret;
+ }
+ }
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
