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 v17 0/5] FPGA Image Load (previously Security Manager)
To:     "Wu, Hao" <hao.wu@intel.com>, "Xu, Yilun" <yilun.xu@intel.com>
CC:     Tom Rix <trix@redhat.com>, "mdf@kernel.org" <mdf@kernel.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lgoncalv@redhat.com" <lgoncalv@redhat.com>,
        "Gerlach, Matthew" <matthew.gerlach@intel.com>
References: <20211013010617.GE95330@yilunxu-OptiPlex-7050>
 <58036b2d-ca8f-2deb-f1b4-0301d633714b@intel.com>
 <20211014014947.GF95330@yilunxu-OptiPlex-7050>
 <7d1971d0-b50b-077f-2a82-83d822cd2ad7@intel.com>
 <20211015025140.GH95330@yilunxu-OptiPlex-7050>
 <2b26bea5-60d3-6763-00e8-9a94fa0bf45b@intel.com>
 <20211018081356.GB40070@yilunxu-OptiPlex-7050>
 <301850cf-9f34-530b-bd9c-fbe9bf9feee5@intel.com>
 <20211019025356.GC40070@yilunxu-OptiPlex-7050>
 <85d56e4b-5bed-693d-4f76-027173a6e7a0@intel.com>
 <20211020011608.GA145760@yilunxu-OptiPlex-7050>
 <38cfb530-8826-7890-da59-c1dd33b9cc7d@intel.com>
 <DM6PR11MB38198F9B969569FDDD71A1CC85849@DM6PR11MB3819.namprd11.prod.outlook.com>
 <03ff4983-d8a9-6ad7-a655-a8dcde3da360@intel.com>
 <DM6PR11MB38192DC4A39D654F88322D0C85859@DM6PR11MB3819.namprd11.prod.outlook.com>
From:   Russ Weight <russell.h.weight@intel.com>
Message-ID: <e5fa668d-d32c-2285-1145-b1e9128508ec@intel.com>
Date:   Wed, 27 Oct 2021 08:11:33 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.13.0
In-Reply-To: <DM6PR11MB38192DC4A39D654F88322D0C85859@DM6PR11MB3819.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WW9XZW5zcHpBOVlyNDJIQUt5NzNzSzdyc3I1Ukp3VzJzL1JIMWhqanREeUh0?=
 =?utf-8?B?SU00NkZqREhNL2YwSHZyR0NmRDR3b2ZUM3NnU0kxNVpMeWRBMFk3UmxadGhm?=
 =?utf-8?B?OGlyVFR5Y1VTRWt1VU9EK3FZT0VTeXRHbFlEVkFyd2xpRnhyOTJETWhocEhm?=
 =?utf-8?B?KytzOEZiNEdoSVAyVVczbUxOK1FUWjl2dEFmR2QyV1B5OGhGSFVEOUN1SHpI?=
 =?utf-8?B?TllGSnN6cHE3TkkyY2pFZ0R1d0UwSmh2Y1o3NlgxVm5hbzFRUHVYWCtzMGxj?=
 =?utf-8?B?WkhsTitobHdJTXhpL0RFM1ZZQTE3UG8rLzF5OFdPRXc5T1ZyM2dENnpCazlW?=
 =?utf-8?B?azFrVlhMTUNVQ2NuOWZqaXRrV3kwWHRhZ0dCMFV6eEhxT2RiR3dnUjhQZUFi?=
 =?utf-8?B?akExcDB2bnJZUzErM0NEeDRyL09rb28xU1pJU0JiMFUrem15L254YUt6UWFi?=
 =?utf-8?B?V0lkKytrN29yMmd2VXY4WlZOaVhlazJQeFpsbUxkOWJjRVYyNXc0OHRVckd6?=
 =?utf-8?B?dm5LSGgrSkV6cFRrTVJqSFkvcnprMkVxRXNSelF6WUU2NUdkNGZDSUdueTNw?=
 =?utf-8?B?MWhHU040azA3ZSsxeE8zdnNOZDZYcG1JZlZoZzlyQnV0a29nWGo1WTR5QnFh?=
 =?utf-8?B?aFlHODdBK1JqMlBmbjBlOWpad0EyZzlwMzVzYWtsMHJEcmhFQkZWdHBVUlpv?=
 =?utf-8?B?OWxJSENRazA0RDlQZWpTQmxQU2hjOHFRWEZPeUJydlpSN3VzZzBDMFNQTGx4?=
 =?utf-8?B?ZDRTSTNKSnVOc2VYSXRrL1R0dXBuQlR0ZE42TVE4M1F4eDFtOXRPbUM4QXRv?=
 =?utf-8?B?V3VKNWMrcTFlYzRmalNqUzdnQ1NRK0xjM1pJU3pTSXFsc0xWSDB3Mm9RNGhh?=
 =?utf-8?B?b0lWZG1OZWFDR2ZLcXNKRUt5aGhSa1R2bDJUOGYvWjZ2YzNuYWM0dG1jb1BK?=
 =?utf-8?B?Z0ZJaWYzZzBmNll2YXNJcVNhOCtEajdFQVlqOElQT1ZGcXRkZGJFaUhXdkVz?=
 =?utf-8?B?bnBYUEJNQjZRcHlvVFBpWlhlM0Y0cXFLV1IrWFIrS3A5a1BBU25BVkE5SXVx?=
 =?utf-8?B?ejd4YXZiTUNzczRYRXQyNkF6VkVySmJHT0h4ZkJDOThjb0c3dmdVbkpxMDJq?=
 =?utf-8?B?TEpaS2hzT1V4NjdXeGlxSjFVNnN0S3NJYzhwd1lVNDVnSm1Kb0xBOG9waEUw?=
 =?utf-8?B?UFMxSnNXaVF0TU9kYjVSKytjVmVaZktMd01NK0o5MS95TWQ4cmQrcFg4RkYv?=
 =?utf-8?B?T0tBWmdkRzg2SUlZWlJiWXNDOTloL0syV24xWE1YL1RJUHIveityNDdMMnhE?=
 =?utf-8?B?Nm5aN2ZjSjB6bEY4NDR4c1I1TC9Hd3JZdzZNeDF4NjltbDliNlBLdXppQVdp?=
 =?utf-8?B?N3ZVTWtTOEFxb0ZIVk5LQTBWeFB1T3I4SlgrMkFmMzYvSG10OGJ4ME5UMnVM?=
 =?utf-8?B?K0VrMjBqbnVZL2Z5eXcwbk4rNi9aZld1aG16aVhMbHpySkRNR0JGRmtCQlVX?=
 =?utf-8?B?SEMyK1ludm9XbFpDa1VzOFFsUDF6aURTNlRGcDV4S2Q2UFhJYUQ2STZmcGhL?=
 =?utf-8?B?S2FkNTFRc09qKzR5U2ZNRzdHdXIvQ2JvZ0VlRGtWVkIzVm83WGZCV01aQ3BK?=
 =?utf-8?B?bUtva0QxUEFpOXJCUG9FMytPUENYNW4wc0N2SVZjbEdMU0NIT2UvYlB5V0gw?=
 =?utf-8?B?UnBDSHczUkczSTZpMVNkdWdJYWFMU2xUSHlDM0ZxdE9yWGN2dHJGVVpldGNM?=
 =?utf-8?B?VkpFOFBVcEQzQjJqV0p4em8rVjlVdmNxR0ROazJ1bFFiOWNhdUUyR3NzVVBB?=
 =?utf-8?B?cW1WL3duYVlwYzZpend2aEZmNTFlLzNHaVAzSWlHNGtFUFZzZlFZU3plZUNs?=
 =?utf-8?B?NTNWS2ZleGNjQTM3UXVBOGhBL2RQaHhFMXBVN09wZ3UxQ3JrT1ZJdXVEZEpo?=
 =?utf-8?B?azRrWFBsQ2VpcmU2ZHl2UGxFZGphb1RYWUdzUk1PZHVXa3I0c1pFRjgzaWVH?=
 =?utf-8?B?dHZkZUFUaWsrMm9sTzZad2pYV08xWDg2R2F4ZzhPN0lKU2YyUFJrWVlzZ0ZC?=
 =?utf-8?B?emxDdUdST1ZjamxCTkplUUl2OXVkOHd1Z0dqZU9JTmdnRjBTQkpHYzdkTVc0?=
 =?utf-8?B?UExuY2M5ZndFaXFYdlpWUUlHSzZHbDI0VnJTMkMreHZ3dFZvaXNQOHNmMWoz?=
 =?utf-8?B?VlJhTHVpa2Yzb251OW9qVmlId0djUnNISGxSTHJWUXBrNzV3QjBsc21aUC9Y?=
 =?utf-8?B?NFhJeXJ2WS9JbDREcUltc1pieDBRPT0=?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b883af3-e451-4dde-3b57-08d9995c1851
X-MS-Exchange-CrossTenant-AuthSource: SA2PR11MB5163.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2021 15:11:47.4798
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yNER8sj9YzhsZQvYDYTq7svFj6BohkZab1VI0auWWJIddJbd/FSGaIZzQ938pTo/uHXyb7OxqXxMv84altswCc4KxyHgWFsxiOAl/11Yv2g=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB3501
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 10/26/21 8:29 PM, Wu, Hao wrote:
>>>>> The API should not only define what it won't do, but also define what
>>>>> it will do. But the "image load" just specifies the top half of the
>>>>> process. So I don't think this API would be accepted.
>>>> So what is the path forward. It seems like you are saying
>>>> that the self-describing files do not fit in the fpga-mgr.
>>>> Can we reconsider the FPGA Image Load Framework, which does
>>>> not make any assumptions about the contents of the image
>>>> files?
>>> Why we need such "generic data transfer" interface in FPGA
>>> framework?
>> Are you referring to the use of self-describing files?
>> or the generic nature of this class driver?
> Yes, why this is under FPGA framework? Per your description that
> it can be used to transfer any data, e.g. BMC images, some device
> specific data (self-describing image?). Let's take this as example,
> if FPGA device is replaced with ASIC on N3000, do you still want
> to use FPGA image load framework to transfer your device specific
> data, e.g. BMC images? I really hope that FPGA framework code only
> focus on common usage of FPGA. 
>
>>> we need to handle the common need for FPGA
>>> devices only, not all devices, like programming FPGA images.
>>> So far we even don't know, what's the hardware response on
>>> these self-describing files, how we define it as a common need
>>> interface in the framework?
>> The class driver does not _need_ to reside in the FPGA
>> framework. I sent an inquiry to the maintainer of the
>> Firmware update subsystem (and cc'd the kernel mailing list)
>> and received no responses. I placed it under the FPGA
>> framework only because the first user of the class driver
>> is an FPGA driver.
> You must have enough justifications why this needs to be included
> for everybody not for our own case.

How do we justify it when there are currently no other known
users? I can go ahead and work up some patches for the firmware
subsystem, if we can resolve the other concerns below.

>>> If you just want to reuse the
>>> fpga-mgr/framework code for your own purpose, Yes, it seems
>>> saving some code for you, but finally it loses flexibility, as it's
>>> not possible to extend common framework for your own
>>> purpose in the future.
>> If I understand correctly, you are saying that it doesn't
>> fit well in the FPGA manager, because not all file types
>> fit the definition of a firmware update? And future file
>> types may not fit in fpga-mgr context?
> Let's split the use cases, I think the use case that update a persistent
> storage for FPGA image, and later use hardware logic (FPGA loader)
> to load it into FPGA. This sounds like a common usage for FPGA
> devices, so I think this is why Yilun propose to have this part to be
> covered by fpga-mgr. But for other cases in your description, e.g.
> BMC images, device specific data, self-describing image and etc,
> they are out of scope of FPGA.

Self-describing files are not something new to us; _ALL_ of the image
files that we send to our FPGA cards, including the N3000 FPGA and BMC
images, root-entry hashes, key cancellations, etc. are self-describing
files. They always have been.


>
> Actually I don't fully understand why we need to introduce the
> "self-describing image" as a common data transfer interface, if
> I remember correctly, for N3000, different sub drivers will own
> different hardware sub function blocks, why expose such a new
> shared communication channel?

There is no change here. The N3000 files are self describing. The
secure-update sub-driver of the MAX10 BMC invokes the class driver,
funnels image data to the BMC, performs handshakes with the BMC,
and ultimately returns status through the class driver. All images
that are sent to the FPGA card follow this same path - and it works
fine.

To try to split out the purposes of each self-describing file to
use different kernel APIs means interfacing multiple class drivers
to the same MAX10 sub-driver. I think it also means replicating
code.

- Russ
> If "self-describing image" is a
> request to one of the sub function block, why not just expose
> new interface in such hardware block per modularization? I
> have some concern that this new requirement may break
> current driver architecture for N3000.
>
> Hao
>
>> - Russ
>>> Thanks
>>> Hao