Date:   Fri, 1 Mar 2019 13:10:37 +0100
From:   Pierre Morel <pmorel@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ibm.com>,
        Tony Krowiak <akrowiak@...ux.ibm.com>,
        alex.williamson@...hat.com, cohuck@...hat.com,
        linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org,
        kvm@...r.kernel.org, frankja@...ux.ibm.com, david@...hat.com,
        schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
        freude@...ux.ibm.com, mimu@...ux.ibm.com
Subject: Re: [PATCH v4 1/7] s390: ap: kvm: add PQAP interception for AQIC

On 28/02/2019 17:51, Halil Pasic wrote:
> On Thu, 28 Feb 2019 15:12:16 +0100
> Pierre Morel <pmorel@...ux.ibm.com> wrote:
> 
>> On 28/02/2019 13:39, Halil Pasic wrote:
>>> On Thu, 28 Feb 2019 10:42:23 +0100
>>> Christian Borntraeger <borntraeger@...ibm.com> wrote:
> [..]
>>>> Correct?
>>>
>>> IMHO mostly.
>>>
>>> I also doing the facility checks in kvm is easier, and I think this is
>>> something we can change later if needed without any major trouble.
>>>
>>> There are a couple of things I would do differently than Pierre does:
>>> 1) Do the PGM_PRIVILEGED_OP before the fc == 3 check.
>>
>> Idea was not to modify existing behavior for fc != 3
>>
>> Also Christian already proposed to handle all FC codes. So in this idea,
>> this must be done as you say.
>>
>>>
>>> 2) Do the test_kvm_facility(vcpu->kvm, 65) check in the context of fc ==
>>> 3. I.e. decide if this hook is about pqap or just about pqap aqic and
>>> make the code convey that decision to its reader.
>>>
>>> 3) I would most probably test if the queue is available by looking at the
>>> masks in CRYCB here. If not AP_RESPONSE_Q_NOT_AVAIL is what we need.
>>
>> This I do not agree with, it is typically the responsibility of the part
>> in charge of the virtualization to do this, also the vfio_driver.
>>
> 
> See at 4) regarding the details. My guess is you disagree with checking
> CRYCB explicitly but don't digress with AP_RESPONSE_Q_NOT_AVAIL if APCB
> does not authorize the queue. Your idea was to infer APCB all zero from
> the fact that pqap_hook is NULL.
> 
> If my assumption is right, then yes we can have an implicit coarse check
> here and a fine grained check in the client code (vfio_ap).




> 
>>>
>>> 4) If we have APIE and queues authorized by the CRYCB (i.e. we have a
>>> vfio_ap module loaded an an mdev associated with the kvm) the callback
>>> not set (!(vcpu->kvm->arch.crypto.pqap_hook)) is a BUG!
>>
>> I do not agree with this either, the maintainers ;) will not allow this.
> 
> After an offline discussion we came to the conclusion that I did not
> understand your code.
> 
> Your train of thought was:
> 
> !(vcpu->kvm->arch.crypto.pqap_hook) _implies_ APCB all zero (i.e. the
> masks in the CRYCB
> 
> This is *why* you respond with AP_RESPONSE_Q_NOT_AVAIL.
> 
> However if that is the case I would like that spelled out in a code
> comment at least. Furthermore setting pqap_hook and APCB needs to happen
> in the right sequence. Means client code (vfio_ap) may only set APCB
> after the qpap_hook has been set. Currently we have a race there (as
> you first do  kvm_arch_crypto_set_masks and only then
> kvm->arch.crypto.pqap_hook. Furthermore I guess
> kvm->arch.crypto.pqap_hook needs to be set with the kvm lock held, which
> does not seem to be the case.


Yes, that is right.
This part (setting/resetting hook and CRYCB will be modified for the 
reason you mention and also to correctly handle the order of releasing 
KVM and VFIO, as you and Christian mentioned.




> 
>>
>>> In that case
>>> lying that the queue is not available does not seem right. BTW this
>>> is something Pierre changed since the last version quietly (I can't
>>> recall a mention in the change log or somebody asking for this). If
>>> we want to be very pedantic about this bug scenario our best bet is
>>> probably response code 6.
>>
>>
>> RC 06 means "Invalid address of AP-queue notification byte"
>>
>> So you must have think about another code or I do not understand at
>> all what you mean.
>>
> 
> I did not assume you decided to ignore the possibility of a programming
> error (which you at least technically did commit yourself) for what I
> described as a BUG.
> 
> My train of thought was, if we are very pedantic we can make things work
> with degraded functionality in that case. I.e. without AP interrupts.
> For that we need to tell the guest something like: yes your queue is
> fine and there and all that but AQCI setup interrupts did not work. And
> RC 06 is the only RC I see being suitable to convey that.
> 
> Detect and handle if the client code does not hold up their end of the
> bargain or just ignore the possibility is a design decision. But at least
> you should spell out your expectations against the client code.
> 
> Regards,
> Halil
> 

I prefer to comment the obligation for the vfio_driver to register the 
callback instead to add code complexity for which will eventually go 
deeper and deeper.


Thanks,
Pierre


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Powered by blists - more mailing lists