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

["Jorge Ramirez-Ortiz, Foundries" <jorge@foundries.io>]

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@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

> 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? 

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?


> 
> And the same style should work for SPL and also for U-Boot proper.

sure.

thanks for the quick response.

> 
> Thanks,
> Michal