Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v10 11/26] s390: vfio-ap: implement mediated device open
 callback
To:     David Hildenbrand <david@redhat.com>,
        Tony Krowiak <akrowiak@linux.vnet.ibm.com>,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     freude@de.ibm.com, schwidefsky@de.ibm.com,
        heiko.carstens@de.ibm.com, borntraeger@de.ibm.com,
        cohuck@redhat.com, kwankhede@nvidia.com,
        bjsdjshi@linux.vnet.ibm.com, pbonzini@redhat.com,
        alex.williamson@redhat.com, pmorel@linux.vnet.ibm.com,
        alifm@linux.vnet.ibm.com, mjrosato@linux.vnet.ibm.com,
        jjherne@linux.vnet.ibm.com, thuth@redhat.com,
        pasic@linux.vnet.ibm.com, berrange@redhat.com,
        fiuczy@linux.vnet.ibm.com, buendgen@de.ibm.com,
        frankja@linux.ibm.com
References: <1536781396-13601-1-git-send-email-akrowiak@linux.vnet.ibm.com>
 <1536781396-13601-12-git-send-email-akrowiak@linux.vnet.ibm.com>
 <e1ff9918-a1ab-31b1-0b32-a3980770d2ac@linux.ibm.com>
 <09a6b9e5-e335-14cf-debd-de0f92dafd5e@redhat.com>
 <ebe81ebf-9b61-dbf6-743e-6145162cdcfb@linux.ibm.com>
 <69b5e3d3-5d44-37c0-ca10-720345852134@redhat.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
Date:   Mon, 24 Sep 2018 15:46:06 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.0
MIME-Version: 1.0
In-Reply-To: <69b5e3d3-5d44-37c0-ca10-720345852134@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Message-Id: <df269178-b486-b091-4da4-731fc3f77e85@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 09/24/2018 02:40 PM, David Hildenbrand wrote:
> On 24/09/2018 18:07, Tony Krowiak wrote:
>> On 09/24/2018 04:40 AM, David Hildenbrand wrote:
>>>
>>>>     /**
>>>> - * Verify that the AP instructions are available on the guest. This is
>>>> indicated
>>>> - * via the  KVM_S390_VM_CPU_FEAT_AP CPU model feature.
>>>> + * Verify that the AP instructions are being interpreted by firmware
>>>> for the
>>>> + * guest. This is indicated by the kvm->arch.crypto.apie flag.
>>>>      */
>>>>     static int kvm_ap_validate_crypto_setup(struct kvm *kvm)
>>>>     {
>>>> -	if (test_bit_inv(KVM_S390_VM_CPU_FEAT_AP, kvm->arch.cpu_feat))
>>>> +	if (kvm->arch.crypto.apie)
>>>>     		return 0;
>>>
>>> I wonder if this check makes sense, because apie can be toggled during
>>> runtime. I guess it would be sufficient to check if the ap control block
>>> is available and apie is supported by the HW.
>>
>> I am not clear about what you are getting at here, but I'll attempt
>> to respond. There is no need to check if the AP control block (CRYCB)
>> is available as the address is set in the CRYCBD three instructions
>> above, even if AP instructions are not available. Regarding whether apie
>> is supported by the hardware, the value of vcpu->kvm->arch.crypto.apie
>> can not be set unless it is supported by the HW. In the patch (24/26)
>> that provides the VM attributes to toggle this value, it can only be
>> turned on if the AP instructions are available. I might also note that
>> the kvm_ap_validate_crypto_setup() function is called whenever one of
>> the VM crypto attributes is changed, so it makes sense that decisions
>> made in this function are based on a change to a VM crypto attribute. In
>> my first pass at changing this function, I checked
>> ap_instructions_available() here, but after considering all of the
>> above, it made sense to me to check the apie flag.
>>
> 
> I prefer ap_instructions_available(). As I said, kvm->arch.crypto.apie
> is a moving target.

Looking at this again, I think I responded before my brain shifted from
digesting comments about patch 24/26 (enable/disable APIE) to the
context for your comment here; namely, the device open callback. My
comment above makes no sense in this context. From the perspective of
the vfio_ap device driver, there is one requirement that must be met in
order to provide pass-through functionality: The AP instructions must be
must be interpreted by the HW (i.e., ECA.28 == 1). Checking whether AP
instructions are available does not tell us whether they are being
interpreted by HW. Checking whether the AP control block (i.e., CRYCB)
is available, even when combined with the instruction availability
check, does not provide any more insight into the value of ECA.28
becuase the CRYCB will be provided if the MSAX3 facility is installed
(STFLE.76) for the guest regardless of whether AP instructions are 
available or not. There is no doubt that if the AP instructions are
not available, then the mdev open callback should fail, but it doesn't
tell the whole story.

I realize that our CPU model protects against configuring a vfio-ap
device for the guest if ap=off, but this function knows nothing about
userspace. I can make a similar argument that kvm->arch.crypto.apie
will be switched on only if ap=on but again, that is userspace
configuration.

Having said all of the above, maybe it doesn't really matter whether
AP instructions are being interpreted or not. If ECA.28 == 0, then
the AP masks may very well be ignored since all AP instructions will
be intercepted; so, maybe checking AP instruction availability is all
that is needed. I will verify this and if I'm correct, I'll make the
change you suggested.

>