Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-172-g9a2dae1853-fm-20230213.001-g9a2dae18
Mime-Version: 1.0
Message-Id: <15aa881f-5cf1-446e-a71e-f62d24e87500@app.fastmail.com>
In-Reply-To: <a50fa44d-fbc3-9ce2-175b-85c8cd7a9f7f@linaro.org>
References: <20230214211229.3239350-1-quic_eberman@quicinc.com>
 <20230214212356.3313181-1-quic_eberman@quicinc.com>
 <dbcfa4e9-a1ad-0f24-77bf-05934ca26bb2@linaro.org>
 <05c4aab8-2d26-b944-adb6-624d67e4a11d@quicinc.com>
 <52d944b1-3ea6-26b7-766a-2fed05dccf3a@linaro.org>
 <c5ff1523-7a62-3d3f-6fa9-792ce4d222e8@quicinc.com>
 <a50fa44d-fbc3-9ce2-175b-85c8cd7a9f7f@linaro.org>
Date:   Fri, 24 Feb 2023 14:20:40 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Srinivas Kandagatla" <srinivas.kandagatla@linaro.org>,
        "Elliot Berman" <quic_eberman@quicinc.com>,
        "Alex Elder" <elder@linaro.org>,
        "Jonathan Corbet" <corbet@lwn.net>,
        "Prakruthi Deepak Heragu" <quic_pheragu@quicinc.com>
Cc:     "Murali Nalajala" <quic_mnalajal@quicinc.com>,
        "Trilok Soni" <quic_tsoni@quicinc.com>,
        "Srivatsa Vaddagiri" <quic_svaddagi@quicinc.com>,
        "Carl van Schaik" <quic_cvanscha@quicinc.com>,
        "Dmitry Baryshkov" <dmitry.baryshkov@linaro.org>,
        "Bjorn Andersson" <andersson@kernel.org>,
        "Konrad Dybcio" <konrad.dybcio@linaro.org>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        "Bagas Sanjaya" <bagasdotme@gmail.com>,
        "Catalin Marinas" <catalin.marinas@arm.com>,
        "Jassi Brar" <jassisinghbrar@gmail.com>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v10 10/26] gunyah: vm_mgr: Introduce basic VM Manager
Content-Type: text/plain
Precedence: bulk

On Fri, Feb 24, 2023, at 11:29, Srinivas Kandagatla wrote:
> On 23/02/2023 22:40, Elliot Berman wrote:

>>>> Does this means adding #define GH_VM_DEFAULT_ARG 0 ? I am not sure 
>>>> yet what arguments to add here.
>>>>
>>>> The ABI can add new "long" values to GH_CREATE_VM and that wouldn't 
>>>
>>> Sorry, that is exactly what we want to avoid, we can not change the 
>>> UAPI its going to break the userspace.
>>>
>>>> break compatibility with old kernels; old kernels reject it as -EINVAL.
>>>
>>> If you have userspace built with older kernel headers then that will 
>>> break. Am not sure about old-kernels.
>>>
>>> What exactly is the argument that you want to add to GH_CREATE_VM?
>>>
>>> If you want to keep GH_CREATE_VM with no arguments that is fine but 
>>> remove the conflicting comments in the code and document so that its 
>>> not misleading readers/reviewers that the UAPI is going to be modified 
>>> in near future.
>>>
>>>
>> 
>> The convention followed here comes from KVM_CREATE_VM. Is this ioctl 
>> considered bad example?
>> 
>
> It is recommended to only use _IO for commands without arguments, and 
> use pointers for passing data. Even though _IO can indicate either 
> commands with no argument or passing an integer value instead of a 
> pointer. Am really not sure how this works in compat case.
>
> Am sure there are tricks that can be done with just using _IO() macro 
> (ex vfio), but this does not mean that we should not use _IOW to be more 
> explicit on the type and size of argument that we are expecting.
>
> On the other hand If its really not possible to change this IOCTL to 
> _IOW and argument that you are referring would be with in integer range, 
> then what you have with _IO macro should work.

Passing an 'unsigned long' value instead of a pointer is fine for compat
mode, as a 32-bit compat_ulong_t always fits inside of the 64-bit
unsigned long. The downside is that portable code cannot have a
single ioctl handler function that takes both commands with pointers
and other commands with integer arguments, as some architectures
(i.e. s390, possibly arm64+morello in the future) need to mangle
pointer arguments using compat_ptr() but must not do that on integer
arguments.

     Arnd