Re: [PATCHv1] Add Intel Stratix10 service layer driver

[Alan Tull]

On Tue, Jan 30, 2018 at 1:41 AM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Jan 29, 2018 at 08:08:11PM -0600, Richard Gong wrote:
>> 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@xxxxxxxxxxxxxxx wrote:
>> > > From: Richard Gong <richard.gong@xxxxxxxxx>
>> > >
>> > > 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)
>
> That's very complex for a single patch submission, don't you think?

Hi Greg,

I'm reviewing this patchset now.  v2 will simplify things down and add
better documentation.  Only 2 headers (one for the client API and one
to define the interface with the out of tree code this talks to).

>
> Please do not add new apis / interfaces for code that is not part of
> your patch series, otherwise we don't know what the future is going to
> hold :)

The v2 patch set will include my FPGA manager driver which is the
first client of this service.  My driver will use all the API
functions exported by this service driver.

>
> This feels like it should be a series of patches, to properly explain
> this and hook up all of the new interfaces you are adding, right?

There will be other clients, so there will be other patchsets on top
of this.  We'll be sure to include new API functions and the client
that needs them in the same patch set in the future.

Thanks for your feedback, helpful as always!

Alan

>
> thanks,
>
> greg k-h