Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <030888c7-1e32-1663-589a-55e998963ab7@quicinc.com>
Date:   Tue, 11 Apr 2023 13:48:43 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v11 10/26] gunyah: vm_mgr: Introduce basic VM Manager
Content-Language: en-US
To:     Alex Elder <elder@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@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>,
        Arnd Bergmann <arnd@arndb.de>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Will Deacon <will@kernel.org>, Andy Gross <agross@kernel.org>,
        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>
References: <20230304010632.2127470-1-quic_eberman@quicinc.com>
 <20230304010632.2127470-11-quic_eberman@quicinc.com>
 <37bc641a-77b9-a740-a08e-8b86d0b50630@linaro.org>
From:   Elliot Berman <quic_eberman@quicinc.com>
In-Reply-To: <37bc641a-77b9-a740-a08e-8b86d0b50630@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 3/31/2023 7:25 AM, Alex Elder wrote:
> On 3/3/23 7:06 PM, Elliot Berman wrote:
>> @@ -129,6 +131,7 @@ struct gh_rm_connection {
>>    * @cache: cache for allocating Tx messages
>>    * @send_lock: synchronization to allow only one request to be sent 
>> at a time
>>    * @nh: notifier chain for clients interested in RM notification 
>> messages
>> + * @miscdev: /dev/gunyah
>>    */
>>   struct gh_rm {
>>       struct device *dev;
>> @@ -145,6 +148,8 @@ struct gh_rm {
>>       struct kmem_cache *cache;
>>       struct mutex send_lock;
>>       struct blocking_notifier_head nh;
>> +
>> +    struct miscdevice miscdev;
>>   };
>>   /**
>> @@ -593,6 +598,21 @@ void gh_rm_put(struct gh_rm *rm)
>>   }
>>   EXPORT_SYMBOL_GPL(gh_rm_put);
> 
> I feel like /dev/gunyah code would more appropriately be found
> in "vm_mgr.c".  All gh_dev_ioctl() does is call the function
> defined there, and it's therefore a VM-oriented rather than
> resource-oriented device.

I'd like to keep the gh_dev_ioctl where it is because it keeps the 
struct gh_rm explicitly private to rsc_mgr.c and thinking this helps 
keep the design cleaner long term by preventing new members from 
sneaking into struct gh_rm.

>> +
>> +static long gh_dev_ioctl_create_vm(struct gh_rm *rm, unsigned long arg)
>> +{
>> +    struct gh_vm *ghvm;
>> +    struct file *file;
>> +    int fd, err;
>> +
>> +    /* arg reserved for future use. */
> 
> Do you have a clear idea of how this might be used in the future?

Not yet. I have some vague ideas to use it as a enumeration of "special" 
VM types. We might have special number for VMs which use "protected VM 
firmware"  for the Android boot flow, another number for the "Trusted UI 
VM", another for "OEM VM", etc. Passing 0 would always be the 
unauthenticated VM which we are creating today.

We're considering bumping the info to a separate ioctl since additional 
info needs to be passed from userspace to configure the VM. Userspace 
would do GH_CREATE_VM(). Another ioctl like GH_VM_SET_PVMFW_ADDRESS() 
would imply that the VM uses the protected VM firmware for the Android 
boot flow. Another ioctl call would be used to imply the "Trusted UI 
VM". In any case, we're still in early design phase.

> 
> I was thinking you could silently ignore the argument value, but
> I suppose if it *does* get used in the future, you want the caller
> to know it's being ignored.  (Is that right?)
> 

That's right.

Thanks,
Elliot