Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices

[Tony Krowiak]

On 9/20/19 10:24 AM, Tony Krowiak wrote:
> On 9/19/19 6:34 AM, Halil Pasic wrote:
> > On Fri, 13 Sep 2019 17:26:52 -0400
> > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> >
> > > +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev 
> > > *matrix_mdev)
> > > +{
> > > +ÂÂÂ unsigned long apid, apqi;
> > > +ÂÂÂ unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(unsigned long);
> > > +
> > > +ÂÂÂ memset(matrix_mdev->crycb.apm, 0, masksz);
> > > +ÂÂÂ memset(matrix_mdev->crycb.apm, 0, masksz);
> > > +ÂÂÂ memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
> > > +
> > > +ÂÂÂ for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.apm_max + 1) {
> > > +ÂÂÂÂÂÂÂ for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->matrix.aqm_max + 1) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_bit_inv(apid,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->crycb.apm);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ set_bit_inv(apqi,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ matrix_mdev->crycb.aqm);
> > > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > > +ÂÂÂÂÂÂÂ }
> > > +ÂÂÂ }
> > > +}
> >
> > Even with the discussed typo fixed (zero crycb.aqm) this procedure does
> > not make sense to me. :(
> >
> > If in doubt please consider the following example:
> > matrix_mdev->matrix.apm and matrix_mdev->matrix.aqm have both just bits
> > 0 and 1 set (i.e. first byte 0xC0 the rest of the bytes 0x0). Queues
> > bound to the vfio_ap driver (0,0), (0,1), (1,0); not bound to vfio_ap is
> > however (1,1). If I read this correctly this filtering logic would grant
> > access to (1,1) which seems to contradict with the stated intention.
>
> Yep, I see your point. I'll have to rework this code.

As I see it, we have two choices here:

1. Do not set bit 1 in the APM of the guest's CRYCB because queue
   01.0001 is not bound to the vfio_ap device driver. This would
   preclude guest access to any domain in adapter 1 - i.e., the
   guest would have access to queues 00.0000 and 00.0001.

2. Do not set bit 1 in the AQM of the guest's CRYCB because queue
   01.0001 is not bound to the vfio_ap device driver. This would
   preclude guest access to domain 1 in both adapters - i.e., the
   guest would have access to queues 00.0000 and 01.0000.

There are ramifications for either choice. For example, if only one
adapter is assigned to the mdev, then option 1 will result in the
guest not having access to any AP queues. Likewise, the guest will
not get access to any AP queues if only one domain is assigned to
the mdev. Neither choice is optimal, but option 1 seems to make sense
because it somewhat models the behavior of the host system. For example,
only AP adapters can be configured online/offline and in order to
add/remove domains, an adapter must first be configured offline.

> > Regards,
> > Halil