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;
Subject: Re: [PATCH] firmware: arm_scmi: Pass shmem address to SMCCC call
To:     Sudeep Holla <sudeep.holla@arm.com>
Cc:     Daniele Alessandrelli <daniele.alessandrelli@linux.intel.com>,
        linux-arm-kernel@lists.infradead.org,
        Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
        Peng Fan <peng.fan@nxp.com>,
        "Paul J. Murphy" <paul.j.murphy@linux.intel.com>,
        "Paul J. Murphy" <paul.j.murphy@intel.com>,
        linux-kernel@vger.kernel.org
References: <20200715165518.57558-1-daniele.alessandrelli@linux.intel.com>
 <5f74221b-aec7-7715-19d1-5cbb406f1bdc@gmail.com>
 <20200717094555.GA24501@bogus>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <dce7c86e-1b3f-f9f9-d872-de84a352346d@gmail.com>
Date:   Fri, 17 Jul 2020 11:01:09 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Firefox/68.0 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200717094555.GA24501@bogus>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 7/17/2020 2:45 AM, Sudeep Holla wrote:
> On Wed, Jul 15, 2020 at 03:43:24PM -0700, Florian Fainelli wrote:
>>
>>
>> On 7/15/2020 9:55 AM, Daniele Alessandrelli wrote:
>>> From: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>
>>> Currently, when SMC/HVC is used as transport, the base address of the
>>> shared memory used for communication is not passed to the SMCCC call.
>>> This means that such an address must be hard-coded into the bootloader.
>>>
>>> In order to increase flexibility and allow the memory layout to be
>>> changed without modifying the bootloader, this patch adds the shared
>>> memory base address to the a1 argument of the SMCCC call.
>>>
>>> On the Secure Monitor side, the service call implementation can
>>> therefore read the a1 argument in order to know the location of the
>>> shared memory to use. This change is backward compatible to existing
>>> service call implementations as long as they don't check for a1 to be
>>> zero.
>>
>> resource_size_t being defined after phys_addr_t, its size is different
>> between 32-bit, 32-bit with PAE and 64-bit so it would probably make
>> more sense to define an physical address alignment, or maybe an address
>> that is in multiple of 4KBytes so you can address up to 36-bits of
>> physical address even on a 32-bit only system?
>>
> 
> Good point, I had forgotten about LPAE. Thanks for pointing it out.
> 
>> What discovery mechanism does the OS have that the specified address
>> within the SMCCC call has been accepted by the firmware given the return
>> value of that SMCCC call does not appear to be used or checked? Do we
>> just expect a timeout initializing the SCMI subsystem?
>>
> 
> Agreed, we need to add the check for proper return value then and
> definitely document it very clearly as we are trying to standardise
> a call to vendor SiP FID space of SMCCC.
> 
>> Given that the kernel must somehow reserve this memory as a shared
>> memory area for obvious reasons, and the trusted firmware must also
>> ensure it treats this memory region with specific permissions in its
>> translation regime, does it really make sense to give that much flexibility?
>>
> 
> I expect so and this comes as shmem property from DT already. We are
> just passing the value obtained from there as is. This is just to help
> TFA or the firmware to identify the specific channel/shmem as SMC/HVC
> otherwise has no way to do so.

OK, that is fair enough.

> 
>> If your boot loader has FDT patching capability, maybe it can also do a
>> SMC call to provide the address to your trusted firmware, prior to
>> loading the Linux kernel, and then they both agree, prior to boot about
>> the shared memory address?
>>
> 
> Yes, but we definitely can't rely on such mechanism in the kernel. It is
> more a platform choice as they run different bootloaders.
> 

That argument can be be used the other way too if this is a platform
choice, the platform boot loader can ensure that both ends of the SMC
agree on the shared memory region. I do see an advantage to the approach
being suggested here that the shared memory does not necessarily need to
be mapped by the TF prior to Linux booting, but it can be deferred until
when Linux makes the first SMC call but that may require more complexity
on the TF side to issue an appropriate MMU update, so maybe from a
security perspective this is more dangerous..

Alright, I am convinced now this is useful :)
-- 
Florian