[RFC PATCH 2/2] fpga: xilinx: allow loading authenticated images (DDR)

[Michal Simek]

On 10/5/21 4:03 PM, Jorge Ramirez-Ortiz, Foundries wrote:
> On 05/10/21, Michal Simek wrote:
>>
>>
>> On 10/5/21 1:13 PM, Jorge Ramirez-Ortiz wrote:
>>> Add new compatible string u-boot,zynqmp-fpga-ddrauth to handle this
>>> use case.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge at foundries.io>
>>> ---
>>>  drivers/fpga/xilinx.c   | 29 +++++++++++++++++++++++++----
>>>  drivers/fpga/zynqmppl.c |  4 ++--
>>>  include/xilinx.h        |  2 +-
>>>  3 files changed, 28 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c
>>> index cbebefb55f..c8035105e2 100644
>>> --- a/drivers/fpga/xilinx.c
>>> +++ b/drivers/fpga/xilinx.c
>>> @@ -135,23 +135,44 @@ int fpga_loadbitstream(int devnum, char *fpgadata, size_t size,
>>>  	dataptr += 4;
>>>  	printf("  bytes in bitstream = %d\n", swapsize);
>>>  
>>> -	return fpga_load(devnum, dataptr, swapsize, bstype);
>>> +	return fpga_load(devnum, dataptr, swapsize, bstype, NULL);
>>>  }
>>>  
>>>  int xilinx_load(xilinx_desc *desc, const void *buf, size_t bsize,
>>> -		bitstream_type bstype)
>>> +		bitstream_type bstype, const char *compatible)
>>>  {
>>> +	struct fpga_secure_info info = { 0 };
>>> +
>>>  	if (!xilinx_validate (desc, (char *)__FUNCTION__)) {
>>>  		printf ("%s: Invalid device descriptor\n", __FUNCTION__);
>>>  		return FPGA_FAIL;
>>>  	}
>>>  
>>> -	if (!desc->operations || !desc->operations->load) {
>>> +	if (!desc->operations) {
>>>  		printf("%s: Missing load operation\n", __func__);
>>>  		return FPGA_FAIL;
>>>  	}
>>>  
>>> -	return desc->operations->load(desc, buf, bsize, bstype);
>>> +	if (!compatible || !strcmp(compatible, "u-boot,fpga-legacy")) {
>>> +		if (!desc->operations->load) {
>>> +			printf("%s: Missing load operation\n", __func__);
>>> +			return FPGA_FAIL;
>>> +		}
>>> +		return desc->operations->load(desc, buf, bsize, bstype);
>>> +	}
>>> +
>>> +	if (!strcmp(compatible, "u-boot,zynqmp-fpga-ddrauth")) {
>>> +		if (!desc->operations->loads) {
>>> +			printf("%s: Missing load operation\n", __func__);
>>> +			return FPGA_FAIL;
>>> +		}
>>> +		/* DDR authentication */
>>> +		info.authflag = 1;
>>> +		return desc->operations->loads(desc, buf, bsize, &info);
>>> +	}
>>
>> I am not sure about this solution. First of all it forces SPL to have
>> additional code which just extending size. It means there must be a way
>> to enable/disable it.
> 
> that can be contained of course...so not really an issue (ie compile
> out either fpga_load or fpga_loads depending on needs).
> 
>>
>> The next thing is that you have zynqmp string in generic xilinx file
>> which doesn't look right. It would be better to deal with image types
>> directly in driver which is capable to handle it.
> 
> sure that is easy..but the idea is the same

We just need to find out how this should be properly propagated and stored.

> 
>> It means record fit image compatible string in the driver with
>> hooks/flags and determined what should be called.
> 
> what about the compatible names? will you define those? or should I do
> it? 

Just define it and let's see if this fits.

> 
> For our use case, we need DDR authentication so that must be
> defined. I can add others and someone else can add the support needed?
> 

Sure. None is asking you to support everything. Just add support for
regular bitstreams and your case. And adding support for something else
will be just +1 if interface is right.

Thanks,
Michal