Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <71eb83a1-131d-f667-b1ef-ae214c724ba4@linux.ibm.com>
Date:   Thu, 27 Jan 2022 17:29:44 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [RFC PATCH v1 06/10] KVM: s390: Add vm IOCTL for key checked
 guest absolute memory access
Content-Language: en-US
To:     Thomas Huth <thuth@redhat.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        kvm@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220118095210.1651483-1-scgl@linux.ibm.com>
 <20220118095210.1651483-7-scgl@linux.ibm.com>
 <069c72b6-457f-65c7-652e-e6eca7235fca@redhat.com>
 <8647fcaf-6d8a-4678-0695-4b1cc797b3b1@linux.ibm.com>
 <3035e023-d71a-407b-2ba6-45ad0ae85a9e@redhat.com>
From:   Janis Schoetterl-Glausch <scgl@linux.ibm.com>
In-Reply-To: <3035e023-d71a-407b-2ba6-45ad0ae85a9e@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 1/25/22 13:00, Thomas Huth wrote:
> On 20/01/2022 13.23, Janis Schoetterl-Glausch wrote:
>> On 1/20/22 11:38, Thomas Huth wrote:
>>> On 18/01/2022 10.52, Janis Schoetterl-Glausch wrote:
>>>> Channel I/O honors storage keys and is performed on absolute memory.
>>>> For I/O emulation user space therefore needs to be able to do key
>>>> checked accesses.
>>>> The vm IOCTL supports read/write accesses, as well as checking
>>>> if an access would succeed.
>>> ...
>>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>>> index e3f450b2f346..dd04170287fd 100644
>>>> --- a/include/uapi/linux/kvm.h
>>>> +++ b/include/uapi/linux/kvm.h
>>>> @@ -572,6 +572,8 @@ struct kvm_s390_mem_op {
>>>>    #define KVM_S390_MEMOP_LOGICAL_WRITE    1
>>>>    #define KVM_S390_MEMOP_SIDA_READ    2
>>>>    #define KVM_S390_MEMOP_SIDA_WRITE    3
>>>> +#define KVM_S390_MEMOP_ABSOLUTE_READ    4
>>>> +#define KVM_S390_MEMOP_ABSOLUTE_WRITE    5
>>>
>>> Not quite sure about this - maybe it is, but at least I'd like to see this discussed: Do we really want to re-use the same ioctl layout for both, the VM and the VCPU file handles? Where the userspace developer has to know that the *_ABSOLUTE_* ops only work with VM handles, and the others only work with the VCPU handles? A CPU can also address absolute memory, so why not adding the *_ABSOLUTE_* ops there, too? And if we'd do that, wouldn't it be sufficient to have the VCPU ioctls only - or do you want to call these ioctls from spots in QEMU where you do not have a VCPU handle available? (I/O instructions are triggered from a CPU, so I'd assume that you should have a VCPU handle around?)
>>
>> There are some differences between the vm and the vcpu memops.
>> No storage or fetch protection overrides apply to IO/vm memops, after all there is no control register to enable them.
>> Additionally, quiescing is not required for IO, tho in practice we use the same code path for the vcpu and the vm here.
>> Allowing absolute accesses with a vcpu is doable, but I'm not sure what the use case for it would be, I'm not aware of
>> a precedence in the architecture. Of course the vcpu memop already supports logical=real accesses.
> 
> Ok. Maybe it then would be better to call new ioctl and the new op defines differently, to avoid confusion? E.g. call it "vmmemop" and use:
> 
> #define KVM_S390_VMMEMOP_ABSOLUTE_READ    1
> #define KVM_S390_VMMEMOP_ABSOLUTE_WRITE   2
> 
> ?
> 
>  Thomas
> 

Thanks for the suggestion, I had to think about it for a while :). Here are my thoughts:
The ioctl type (vm/vcpu) and the operations cannot be completely orthogonal (vm + logical cannot work),
but with regards to the absolute operations they could be. We don't have a use case for that
right now and the semantics are a bit unclear, so I think we should choose a design now that
leaves us space for future extension. If we need to, we can add a NON_QUIESCING flag backwards compatibly
(tho it seems a rather unlikely requirement to me), that would behave the same for vm/vcpu memops.
We could also have a NO_PROT_OVERRIDE flag, which the vm memop would ignore.
Whether override is possible is dependent on the vcpu state, so user space leaves the exact behavior to KVM anyway.
If you wanted to enforce that protection override occurs, you would have to adjust
the vcpu state and therefore there should be no confusion about whether to use a vcpu or vm ioctl.

So I'm inclined to have one ioctl code and keep the operations as they are.
I moved the key to the union. One question that remains is whether to enforce that reserved bytes must be 0.
In general I think that it is a good idea, since it leaves a bigger design space for future extensions.
However the vcpu memop has not done that. I think it should be enforced for new functionality (operations, flags),
any objections?

I'll try to be thorough in documenting the currently supported behavior.