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: [PATCHv1] Add Intel Stratix10 service layer driver
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     arnd@arndb.de, linux-kernel@vger.kernel.org, atull@kernel.org
References: <1516898344-20020-1-git-send-email-richard.gong@linux.intel.com>
 <20180125165352.GA21640@kroah.com>
From:   Richard Gong <richard.gong@linux.intel.com>
Message-ID: <7e01745c-24aa-62f7-0be9-f74f19d6d880@linux.intel.com>
Date:   Mon, 29 Jan 2018 20:08:11 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <20180125165352.GA21640@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Greg,

Many thanks for your reviews.


On 01/25/2018 10:53 AM, Greg KH wrote:
> On Thu, Jan 25, 2018 at 10:39:03AM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
>> processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
>> which does the FPGA configuration, QSPI, Crypto and warm reset.
>>
>> When the FPGA is configured from HPS, there needs to be a way for HPS to
>> notify SDM the location and size of the configuration data. Then SDM will
>> get the configuration data from that location and perform the FPGA configuration.
>>
>> To meet the whole system security needs and support virtual machine
>> requesting communication with SDM, only the secure world of software (EL3,
>> Exception Level 3) can interface with SDM. All software entities running
>> on other exception levels must channel through the EL3 software whenever it
>> needs service from SDM.
>>
>> Intel Stratix10 service layer driver is added to provide the service for
>> FPGA configuration. Running at privileged exception level (EL1, Exception
>> Level 1), Intel Stratix10 service layer driver interfaces with the service
>> provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
>> call (SMC) to communicate with secure monitor software at secure monitor
>> exception level (EL3).
>>
>> Later the Intel Stratix10 service layer driver will be extended to provide
>> services for QSPI, Crypto and warm reset.
>>
>> Richard Gong (1):
>>    driver: misc: add Intel Stratix10 service layer driver
>>
>>   drivers/misc/Kconfig                       |   3 +-
>>   drivers/misc/Makefile                      |   3 +-
>>   drivers/misc/intel-service/Kconfig         |   9 +
>>   drivers/misc/intel-service/Makefile        |   2 +
>>   drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
>>   include/linux/intel-service-client.h       | 227 ++++++++++
>>   include/linux/intel-service.h              | 122 +++++
>>   include/linux/intel-smc.h                  | 246 ++++++++++
> Simple questions first:
>   - why do you have 3 different .h files for a single .c file?
This is because service layer driver interface with both the service 
provider and secure monitor SW.
intel-service-client.h is created to define interface between service 
providers (FPGA manager is one of them) and service layer. Alan Tull's 
FPGA manager .c file includes this header file
intel-smc.h defines the secure monitor call (SMC) message protocols used 
for service layer driver in normal world (EL1) to communicate with 
secure monitor SW in secure monitor exception level 3 (EL3). Also this 
header file is shared with firmware since both (FW, service layer) 
utilizes the same SMC message protocol.
intel-sevice.h is created to define service layer's own data structures 
(service controller, channel for communicating with service provider, 
shared memory region, private data etc)

>   - why do you have any public .h files for a single .c file?
intel-service-client.h is public .h and should be at include/linux/
intel-service.h and intel-smc.h are private .h files, should be in 
driver/misc/ (assume I move .c file from driver/misc/intel-service/ to 
driver/misc/)
>   - use the correct SPDX markers for your file licenses, Intel legal
>     knows all about this, please follow their rules.
I will follow those rules.
>   - why is this in a subdirectory for a single .c file?
Currently service layer is implemented to support FPGA configuration 
only. We have the new requirements and need to extend service layer to 
support additional features such as QSPI, Crypto and warm reset. It is 
expected that a few new files will be added later.
For now I can move the single .c file from driver/misc/intel-service/ to 
driver/misc/.

Regards,
Richard
> thanks,
>
> greg k-h