Message-ID: <cc6176cb-6792-ef11-f42a-d2d5b12d04b4@intel.com>
Date:   Mon, 27 Feb 2023 14:42:30 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.2
Subject: Re: [PATCH v1 0/6] PolarFire SoC Auto Update Support
Content-Language: en-US
To:     Conor Dooley <conor@kernel.org>
CC:     Conor Dooley <conor.dooley@microchip.com>,
        Xu Yilun <yilun.xu@intel.com>,
        Daire McNamara <daire.mcnamara@microchip.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
        Tom Rix <trix@redhat.com>, <linux-riscv@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-fpga@vger.kernel.org>
References: <20230217164023.14255-1-conor@kernel.org>
 <Y/ht1eHgtRrLxIhC@yilunxu-OptiPlex-7050> <Y/h1KFGSw13OabYw@wendy>
 <21bebf9d-eb27-5607-b0a9-274c46ef8aa3@intel.com> <Y/0r1aR6Wik2KYBd@spud>
From:   Russ Weight <russell.h.weight@intel.com>
In-Reply-To: <Y/0r1aR6Wik2KYBd@spud>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NVRtUC93aHhTZWlGY2RDeE9WejNxdlhQdUF4RUtzVCtDdWlBZXpxWVJ0MHNO?=
 =?utf-8?B?VUxWNTNTVWFHUnRtUDdFazRBa1l6U3pTdm4rR3UvUjVFSVVZdnpDcDM2VC9t?=
 =?utf-8?B?SzV2eWsvMTJEeStrZHpFUmI3VXVibGNwZ0JXRUNKYjhVQmhHeWxjQnhZQkQr?=
 =?utf-8?B?M3FWcDNBclN4S1lXbGhyUVpFUTNtNXVUeHVpVjVSa05WMUJ3QlNlSGF6dHNO?=
 =?utf-8?B?MlRRRSt0U0F1MEJ3K1NpNmhiUmF4YWQyejhsWHhKSkVLRm9kSVJaeXF3OVhJ?=
 =?utf-8?B?NmVOZTRiT05aK2ErVnhUZEQwc0FXbTQzV0hVclJNYlFkUlg4MjhKTzZnbkNK?=
 =?utf-8?B?ZVBrdDUyMkhscDZTenBZcmFpNkxrcjd4NnlaN3Z3YnM1VXh4UityWW5Wckg4?=
 =?utf-8?B?MHg1NlR0dHhFaHcrMFVXNUtJMjNkWHpqamtyRTY3RW1aT2lSZjl2d2h6dnlE?=
 =?utf-8?B?V1I1TnpnWE1McGJsV1hxK255N1B0aTUrVkdGLzFJcHBJejFWR2pBV1RIV0ZW?=
 =?utf-8?B?bzFDTHhxT011Z05jd04yZjE3cm0wc1V1ZEpZbGlUUUwrT0JYeU5HZWtxUGEw?=
 =?utf-8?B?YnkydGd3QVI5ZnNobWp4TW4zZitkT3RTRkQ0b2V3WDd5THpVWjlSNmRoZjJD?=
 =?utf-8?B?dDFPaVNpN21zOFhOVDVvdUVxcUl2SFB1MTJwVVBoWGJ2MURlTk1tbHRteFF0?=
 =?utf-8?B?c2RlU3R2endtNXhTb2FMT3d0RUp6UndLeWZvQ3dHVjRxRWs5UkluczZpY1d2?=
 =?utf-8?B?YU5OTUJuL0pGcmRiYU1zUVJjb2kyazN2OGFXUENEaDlDRWJzK3FCd1BXY3Fa?=
 =?utf-8?B?NzI5OWRQckFnTU9tUkZKa0kvRys1V0FHNXcwQ2hBLzJQbG1tWWtkeWdzb2xP?=
 =?utf-8?B?UjhCZGhUYWpscVpPcUcwR2dwYTdYMENrUjk5SlJpbTlzZWNUeWpxMzJPZUdK?=
 =?utf-8?B?aU85dkE2bVRqcjhyNjNaZDFVMUw4Nm5jRXdaZ3hFc2Q1TTQxSWptWnRPemdh?=
 =?utf-8?B?SUVDN1l5MVM1TkRsekpIWlpuV0RuMDJsRUl5TmljdHgwYU5uN290VkVZZVVB?=
 =?utf-8?B?cDVjUVVkcUpmWUtTTU5yTGxpeFdmQUJJM1A4NzdDb1hJRHhwekpialhYOUhP?=
 =?utf-8?B?Y1JsY091RW83b3o5VFFndVhhYXRSdTh0ZjRBdVhCclFlbDgvcllBcndjV3Zu?=
 =?utf-8?B?SzlpZDVENUR5Sm5ZL0xKaTIxMkhlYkd0SFdBN0F5VWxXdnRJSitzVDR3VWox?=
 =?utf-8?B?NmM3UWU4UTNVcW5PaVBTcjQ1VDEvbnJQYUcwdnV5N2duREIxUDZHS1hudk9T?=
 =?utf-8?B?MVQ0ajVhdHBJM21PUWhvc2tMSG1BeHRNY2xMdi8xTXcxdVc1b3pVUFVPY0Vi?=
 =?utf-8?B?dXJzbW9vM0QxcHkvTUt1RTJXV0Q0cGdFRnI4ekZTVm4vT3drQitoWVYwUkgw?=
 =?utf-8?B?V1pIMmF4N25PMlRxb1ZEYUQySnJKcnNwaWlDK2F3VEdLdDM1UFNnSDdGV24w?=
 =?utf-8?B?RVRSUU9IcURjYjhobUI2U2tacnVIblB1cER2ZTl6dXAydkVEQkV0djdBajZ1?=
 =?utf-8?B?RGE3Q21oT0toU1Z4VVZWckJXamNYcGdZTGRpUTZUZjdHT0VJRnZmTVh5MVFG?=
 =?utf-8?B?NXRkOS9zZGpGaE1JRS9HOEFDNTQ0cnIyNXkvN2ZNcWE2S0Y2a0hHR2JJVDNF?=
 =?utf-8?B?eXd4eUlYRHNUeHRCUjh2NS9jdFNFL1pQWmZ6QU15MEJpQ1ozVDBlRFp0YlRz?=
 =?utf-8?B?YkVTLzZ4RnBwd2J6SWtNaGRydEF4eDkrUkJGUWR0SEZHcG9VTjI0SU5Id1dq?=
 =?utf-8?B?akgxZzRiUVZDRjBzYy8rTEZiUDc3SjliVzFvOUU5OElKbGpEWkdxaGNacUky?=
 =?utf-8?B?Mnh4RTFvZUtVZVhoM2NwY0p6b1FQY0F6S2J0eWV1Q0FXekNmRTNTLzY4UCtU?=
 =?utf-8?B?dktXb2lvOUI3RHdDYmxLL28xQWM2WmVPQ0hkcGlvMkJRR3pkakRiaW40OTQ4?=
 =?utf-8?B?SnQwWWN6YmJGdnpOdTBsWXJmQzZvOU1ub2xCem92bTl2ajB3emx2VGI3VG9a?=
 =?utf-8?B?V2M2L0YzekQ0MTRaWngrRURYcjM2SGthc3Z3M0VUU2dXVGpCVGttdTljVlMv?=
 =?utf-8?B?YkFpMEIrQ0xuYzdDN3lCVG5lY1Yra3Z1NzQrWVYvcENmZ3REa01PcmV4ZURq?=
 =?utf-8?B?RXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6737323c-845f-413c-4887-08db1913ef8b
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1899.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Feb 2023 22:42:41.9512
 (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: 4ev7GBWBphzmO3SOUNdPh1EEcEVGEq95YUr/v0/6Grw+XYUwgRoHY4mRFwf6397MRWr3TxNkiWOZIc0/w5kP5Dyi7ePYFTTe7To2AUsYijE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: LV2PR11MB6069
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/27/23 14:16, Conor Dooley wrote:
> Hey Russ,
>
> On Mon, Feb 27, 2023 at 02:04:36PM -0800, Russ Weight wrote:
>> On 2/24/23 00:28, Conor Dooley wrote:
>>> On Fri, Feb 24, 2023 at 03:57:09PM +0800, Xu Yilun wrote:
>>>> On 2023-02-17 at 16:40:17 +0000, Conor Dooley wrote:
>>>>> This patchset adds support for the "Auto Update" feature on PolarFire
>>>>> SoC that allows for writing an FPGA bistream to the SPI flash connected
>>>>> to the system controller.
>>>> I haven't fully checked the patches yet, just some quick comments:
>>>>
>>>> Since this feature is just to R/W the flash, and would not affect the
>>>> runtime FPGA region, I don't think an FPGA manager is actually needed.
>>>> Why not just use the MTD uAPI? There is a set of exsiting MTD uAPI &
>>>> MTD tool if I remember correctly.
>>> A lack of interest in opening up the system controller to userspace!
>>> You're right in that the writing of the image can be done that way, and
>>> while I was testing I used the userspace bits of mtd along the way - but
>>> for validating that the image we are writing we rely on the system
>>> controller.
>>> I'm really not interested in exposing the system controller's
>>> functionality, especially the bitstream manipulation parts, to userspace
>>> due to the risk of input validation bugs, so at least that side of
>>> things should remain in the kernel.
>>> I suppose I could implement something custom in drivers/soc that does
>>> the validation only, and push the rest out to userspace. Just seemed
>>> fitting to do the whole lot in drivers/fpga.
>> In case you haven't already looked at the new firmware-upload
>> support in the firmware-loader, I'll give you some references
>> here to see if it fit you needs. This would only support the
>> write (not the read) but it would allow the controller to do
>> validation on the write.
>>
>> The is the cover letter which shows a usage example:
>> https://lore.kernel.org/lkml/20220421212204.36052-1-russell.h.weight@intel.com/
>>
>> And this is a pointer to the latest documentation for it:
>> https://docs.kernel.org/driver-api/firmware/fw_upload.html
>>
>> The only current user is: drivers/fpga/intel-m10-bmc-sec-update.c
> Sounds interesting, I shall go and take a look. Just quickly, when you
> say it wouldn't support the read, what exactly do you mean?
> The only read that I am really interested in doing is reading the first
> 1K of flash as I need to RMW it, but I don't think that that's what you
> mean.
> Do you mean that I would not be able to dump the firmware using your
> fw_upload interface? If so, that's an acceptable constraint IMO.

Yes - I mean that you couldn't dump the firmware image from userspace
using the fw_upload interface. The sysfs interface allows you to read
and write a temporary buffer, but once you "echo 0 > loading", there
is no sysfs interface associated with the firmware-loader that would
allow you to read the image from flash. Your controller actually does
the writes, so RMW in that context is fine.

- Russ
>
> Your interface also circumvents us (Microchip) defining yet another
> method for interacting with the FPGA, since from my nosing around,
> everyone seems to do something different.
>
> Cheers,
> Conor.
>