Re: [PATCH v7 18/22] s390: vfio-ap: zeroize the AP queues.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 31 Jul 2018 16:18:03 +0200
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

> On 07/31/2018 03:40 PM, Cornelia Huck wrote:
> > On Tue, 31 Jul 2018 15:12:59 +0200
> > Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
> >   
> >> On 07/31/2018 02:24 PM, Cornelia Huck wrote:  
> >>>> +static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev, bool force)
> >>>> +{
> >>>> +	int ret;
> >>>> +	int rc = 0;
> >>>> +	unsigned long apid, apqi;
> >>>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >>>> +
> >>>> +	if (!matrix_mdev->activated) {
> >>>> +		pr_err("%s: reset failed for mdev %s, not activated",
> >>>> +		       VFIO_AP_MODULE_NAME, matrix_mdev->name);
> >>>> +
> >>>> +		return -EPERM;
> >>>> +	}  
> >>> Hm... if we stick with the activation approach, what happens if we:
> >>> - open the mdev
> >>> - activate the matrix
> >>> - deactivate the matrix  
> >>
> >> I guess this step will fail and the matrix remains activated.
> >>
> >> Have a look at vfio_ap_mdev_deactivate()  
> > 
> > Hm, I don't see it...
> >   
> static int vfio_ap_mdev_deactivate(struct ap_matrix_mdev *matrix_mdev)
> {
>          int ret = 0;
>                                                                                  
>          if (!matrix_mdev->activated)
>                  return 0;
>                                                                                  
>          if (matrix_mdev->kvm) {
>                  pr_warn("%s: %s: deactivate failed, mdev %s is in use by guest %s\n",
>                          VFIO_AP_MODULE_NAME, __func__, matrix_mdev->name,
>                          matrix_mdev->kvm->arch.dbf->name);
>                  return -EBUSY;       <===================================== Return -EBUSY and don't deactivate.

Yes, but what forces ->kvm to be !NULL? I did not see this in the code,
and I'm not sure what I'm missing. (If ->kvm is guaranteed to be !NULL,
why do we check for it during open?)

>                                                    
>          }
>                                                                                  
>          matrix_mdev->activated = false;
>                                                                                  
>          return ret;
> }
> 
> 
> >>  
> >>> - release the mdev, triggering this function
> >>>
> >>> It seems we won't call PAPQ(ZAPQ) in that case -- is that how it is
> >>> supposed to work?
> >>>      
> >>
> >> So basically the device remains active until the mdev release is
> >> called if I'm correct.  
> > 
> > So, why do we need the activate/deactivate approach, then? Why can't we
> > do the verification etc. during open?
> >   
> 
> We ca do the verification during open. If there was no activate before
> the open we actually try to do one.
> 
> With activate however the admin can claim the resources before the
> open (that is the guest start). We have a mechanism for both 'let's
> overcommit and fail when we hit the wall' and 'let's make sure everything
> is reserved before we give it to the guest'. The admin is free to
> decide on his policy.

OK, let's discuss that in the other thread, then. I am not sure that
departing from the general mdev model is worth it.

(I have not yet gotten to replying there.)

> 
> Please take note that activate also has an effect on the availability
> of certain operations (e.g. assign/unassign).
> 
> Regards,
> Halil
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux