diff mbox series

[v11,10/13] fpga: zynqmp: optimize zynqmppl_load() code

Message ID 20220705192320.415861-11-oleksandr.suvorov@foundries.io
State Superseded
Delegated to: Michal Simek
Headers show
Series fpga: zynqmp: Adding support of loading authenticated images | expand

Commit Message

Oleksandr Suvorov July 5, 2022, 7:23 p.m. UTC 
Optimize function code preparing to add secure bitstream types
support.

Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
Tested-by: Ricardo Salveti <ricardo@foundries.io>
Tested-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
---

(no changes since v1)

 drivers/fpga/zynqmppl.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

Michal Simek July 8, 2022, 12:43 p.m. UTC | #1 
On 7/5/22 21:23, Oleksandr Suvorov wrote:
> Optimize function code preparing to add secure bitstream types
> support.

Can you please extend this? I understand what you do below but better 
description will be good.

> 
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> Tested-by: Ricardo Salveti <ricardo@foundries.io>
> Tested-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/fpga/zynqmppl.c | 27 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> index 239c498f7b5..6959b8ae97e 100644
> --- a/drivers/fpga/zynqmppl.c
> +++ b/drivers/fpga/zynqmppl.c
> @@ -199,46 +199,45 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
>   	return 0;
>   }
>   
> -static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
> -		     bitstream_type bstype, int flags)
> +static int zynqmp_load(xilinx_desc *desc, const void *buf,
> +		       size_t bsize, bitstream_type bstype,
> +		       int flags)

This is unrelated to commit. This is purely coding style change.


>   {
>   	ALLOC_CACHE_ALIGN_BUFFER(u32, bsizeptr, 1);
>   	u32 swap = 0;
>   	ulong bin_buf;
>   	int ret;
>   	u32 buf_lo, buf_hi;
> +	u32 bsize_req = (u32)bsize;
>   	u32 ret_payload[PAYLOAD_ARG_CNT];
> -	bool xilfpga_old = false;
> +
> +	debug("%s called!\n", __func__);
>   
>   	if (zynqmp_firmware_version() <= PMUFW_V1_0) {
>   		puts("WARN: PMUFW v1.0 or less is detected\n");
>   		puts("WARN: Not all bitstream formats are supported\n");
>   		puts("WARN: Please upgrade PMUFW\n");
> -		xilfpga_old = true;
> -		if (zynqmp_validate_bitstream(desc, buf, bsize, bsize, &swap))
> +		if (zynqmp_validate_bitstream(desc, buf, bsize,
> +					      bsize, &swap))

This is also coding style change only.

>   			return FPGA_FAIL;
>   		bsizeptr = (u32 *)&bsize;
>   		flush_dcache_range((ulong)bsizeptr,
>   				   (ulong)bsizeptr + sizeof(size_t));
> +		bsize_req = (u32)(uintptr_t)bsizeptr;
>   		bstype |= BIT(ZYNQMP_FPGA_BIT_NS);
> +	} else {
> +		bstype = 0;
>   	}
>   
>   	bin_buf = zynqmp_align_dma_buffer((u32 *)buf, bsize, swap);
>   
> -	debug("%s called!\n", __func__);

nit: And this also has nothing to do with optimization. You just changed location.

>   	flush_dcache_range(bin_buf, bin_buf + bsize);
>   
>   	buf_lo = (u32)bin_buf;
>   	buf_hi = upper_32_bits(bin_buf);
>   
> -	if (xilfpga_old)
> -		ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
> -					buf_hi, (u32)(uintptr_t)bsizeptr,
> -					bstype, ret_payload);
> -	else
> -		ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
> -					buf_hi, (u32)bsize, 0, ret_payload);
> -
> +	ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo, buf_hi,
> +				bsize_req, bstype, ret_payload);
>   	if (ret)
>   		printf("PL FPGA LOAD failed with err: 0x%08x\n", ret);
>   

M
Oleksandr Suvorov July 8, 2022, 1:15 p.m. UTC | #2 
Hi Michal,

On Fri, Jul 8, 2022 at 3:43 PM Michal Simek <michal.simek@amd.com> wrote:
>
>
>
> On 7/5/22 21:23, Oleksandr Suvorov wrote:
> > Optimize function code preparing to add secure bitstream types
> > support.
>
> Can you please extend this? I understand what you do below but better
> description will be good.

Ok, if I'll realize how to do this :)

> >
> > Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
> > Tested-by: Ricardo Salveti <ricardo@foundries.io>
> > Tested-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
> > ---
> >
> > (no changes since v1)
> >
> >   drivers/fpga/zynqmppl.c | 27 +++++++++++++--------------
> >   1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
> > index 239c498f7b5..6959b8ae97e 100644
> > --- a/drivers/fpga/zynqmppl.c
> > +++ b/drivers/fpga/zynqmppl.c
> > @@ -199,46 +199,45 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
> >       return 0;
> >   }
> >
> > -static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
> > -                  bitstream_type bstype, int flags)
> > +static int zynqmp_load(xilinx_desc *desc, const void *buf,
> > +                    size_t bsize, bitstream_type bstype,
> > +                    int flags)
>
> This is unrelated to commit. This is purely coding style change.

Ok, I'll separate to another commit.

> >   {
> >       ALLOC_CACHE_ALIGN_BUFFER(u32, bsizeptr, 1);
> >       u32 swap = 0;
> >       ulong bin_buf;
> >       int ret;
> >       u32 buf_lo, buf_hi;
> > +     u32 bsize_req = (u32)bsize;
> >       u32 ret_payload[PAYLOAD_ARG_CNT];
> > -     bool xilfpga_old = false;
> > +
> > +     debug("%s called!\n", __func__);
> >
> >       if (zynqmp_firmware_version() <= PMUFW_V1_0) {
> >               puts("WARN: PMUFW v1.0 or less is detected\n");
> >               puts("WARN: Not all bitstream formats are supported\n");
> >               puts("WARN: Please upgrade PMUFW\n");
> > -             xilfpga_old = true;
> > -             if (zynqmp_validate_bitstream(desc, buf, bsize, bsize, &swap))
> > +             if (zynqmp_validate_bitstream(desc, buf, bsize,
> > +                                           bsize, &swap))
>
> This is also coding style change only.

Ok.

> >                       return FPGA_FAIL;
> >               bsizeptr = (u32 *)&bsize;
> >               flush_dcache_range((ulong)bsizeptr,
> >                                  (ulong)bsizeptr + sizeof(size_t));
> > +             bsize_req = (u32)(uintptr_t)bsizeptr;
> >               bstype |= BIT(ZYNQMP_FPGA_BIT_NS);
> > +     } else {
> > +             bstype = 0;
> >       }
> >
> >       bin_buf = zynqmp_align_dma_buffer((u32 *)buf, bsize, swap);
> >
> > -     debug("%s called!\n", __func__);
>
> nit: And this also has nothing to do with optimization. You just changed location.

Michal, what is this closer to? Refactor?
It's not only about changing location. Now there is only one call of
xilinx_pm_request().

> >       flush_dcache_range(bin_buf, bin_buf + bsize);
> >
> >       buf_lo = (u32)bin_buf;
> >       buf_hi = upper_32_bits(bin_buf);
> >
> > -     if (xilfpga_old)
> > -             ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
> > -                                     buf_hi, (u32)(uintptr_t)bsizeptr,
> > -                                     bstype, ret_payload);
> > -     else
> > -             ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
> > -                                     buf_hi, (u32)bsize, 0, ret_payload);
> > -
> > +     ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo, buf_hi,
> > +                             bsize_req, bstype, ret_payload);
> >       if (ret)
> >               printf("PL FPGA LOAD failed with err: 0x%08x\n", ret);
> >
>
> M
Michal Simek July 8, 2022, 1:35 p.m. UTC | #3 
On 7/8/22 15:15, Oleksandr Suvorov wrote:
> Hi Michal,
> 
> On Fri, Jul 8, 2022 at 3:43 PM Michal Simek <michal.simek@amd.com> wrote:
>>
>>
>>
>> On 7/5/22 21:23, Oleksandr Suvorov wrote:
>>> Optimize function code preparing to add secure bitstream types
>>> support.
>>
>> Can you please extend this? I understand what you do below but better
>> description will be good.
> 
> Ok, if I'll realize how to do this :)
> 
>>>
>>> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@foundries.io>
>>> Tested-by: Ricardo Salveti <ricardo@foundries.io>
>>> Tested-by: Adrian Fiergolski <adrian.fiergolski@fastree3d.com>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>    drivers/fpga/zynqmppl.c | 27 +++++++++++++--------------
>>>    1 file changed, 13 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
>>> index 239c498f7b5..6959b8ae97e 100644
>>> --- a/drivers/fpga/zynqmppl.c
>>> +++ b/drivers/fpga/zynqmppl.c
>>> @@ -199,46 +199,45 @@ static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
>>>        return 0;
>>>    }
>>>
>>> -static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
>>> -                  bitstream_type bstype, int flags)
>>> +static int zynqmp_load(xilinx_desc *desc, const void *buf,
>>> +                    size_t bsize, bitstream_type bstype,
>>> +                    int flags)
>>
>> This is unrelated to commit. This is purely coding style change.
> 
> Ok, I'll separate to another commit.
> 
>>>    {
>>>        ALLOC_CACHE_ALIGN_BUFFER(u32, bsizeptr, 1);
>>>        u32 swap = 0;
>>>        ulong bin_buf;
>>>        int ret;
>>>        u32 buf_lo, buf_hi;
>>> +     u32 bsize_req = (u32)bsize;
>>>        u32 ret_payload[PAYLOAD_ARG_CNT];
>>> -     bool xilfpga_old = false;
>>> +
>>> +     debug("%s called!\n", __func__);
>>>
>>>        if (zynqmp_firmware_version() <= PMUFW_V1_0) {
>>>                puts("WARN: PMUFW v1.0 or less is detected\n");
>>>                puts("WARN: Not all bitstream formats are supported\n");
>>>                puts("WARN: Please upgrade PMUFW\n");
>>> -             xilfpga_old = true;
>>> -             if (zynqmp_validate_bitstream(desc, buf, bsize, bsize, &swap))
>>> +             if (zynqmp_validate_bitstream(desc, buf, bsize,
>>> +                                           bsize, &swap))
>>
>> This is also coding style change only.
> 
> Ok.
> 
>>>                        return FPGA_FAIL;
>>>                bsizeptr = (u32 *)&bsize;
>>>                flush_dcache_range((ulong)bsizeptr,
>>>                                   (ulong)bsizeptr + sizeof(size_t));
>>> +             bsize_req = (u32)(uintptr_t)bsizeptr;
>>>                bstype |= BIT(ZYNQMP_FPGA_BIT_NS);
>>> +     } else {
>>> +             bstype = 0;
>>>        }
>>>
>>>        bin_buf = zynqmp_align_dma_buffer((u32 *)buf, bsize, swap);
>>>
>>> -     debug("%s called!\n", __func__);
>>
>> nit: And this also has nothing to do with optimization. You just changed location.
> 
> Michal, what is this closer to? Refactor?
> It's not only about changing location. Now there is only one call of
> xilinx_pm_request().

I know but it s not described in commit message. Diff was really just passing 
different argument. You just change the logic about them where you use clear 
bstype for secure bitstreams and align bsize_req

But you are not changing logic around bsizeptr where old xilfpga expects size to 
be a pointer not value.

Thanks,
Michal

commit 31bcb3444cbd5002ca9d8f6a3a2644092748cdba
Author:     Siva Durga Prasad Paladugu <siva.durga.paladugu@xilinx.com>
AuthorDate: Thu Mar 15 00:17:24 2018 +0530
Commit:     Michal Simek <michal.simek@amd.com>
CommitDate: Mon Apr 9 12:14:50 2018 +0200

     fpga: zynqmp: Fix the nonsecure bitstream loading issue

     Xilfpga library expects the size of bitstream in a pointer
     but currenly we are passing the size as a value. This patch
     fixes this issue.

     Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
     Signed-off-by: Nava kishore Manne <navam@xilinx.com>
     Signed-off-by: Michal Simek <michal.simek@xilinx.com>
diff mbox series

Patch

diff --git a/drivers/fpga/zynqmppl.c b/drivers/fpga/zynqmppl.c
index 239c498f7b5..6959b8ae97e 100644
--- a/drivers/fpga/zynqmppl.c
+++ b/drivers/fpga/zynqmppl.c
@@ -199,46 +199,45 @@  static int zynqmp_validate_bitstream(xilinx_desc *desc, const void *buf,
 	return 0;
 }
 
-static int zynqmp_load(xilinx_desc *desc, const void *buf, size_t bsize,
-		     bitstream_type bstype, int flags)
+static int zynqmp_load(xilinx_desc *desc, const void *buf,
+		       size_t bsize, bitstream_type bstype,
+		       int flags)
 {
 	ALLOC_CACHE_ALIGN_BUFFER(u32, bsizeptr, 1);
 	u32 swap = 0;
 	ulong bin_buf;
 	int ret;
 	u32 buf_lo, buf_hi;
+	u32 bsize_req = (u32)bsize;
 	u32 ret_payload[PAYLOAD_ARG_CNT];
-	bool xilfpga_old = false;
+
+	debug("%s called!\n", __func__);
 
 	if (zynqmp_firmware_version() <= PMUFW_V1_0) {
 		puts("WARN: PMUFW v1.0 or less is detected\n");
 		puts("WARN: Not all bitstream formats are supported\n");
 		puts("WARN: Please upgrade PMUFW\n");
-		xilfpga_old = true;
-		if (zynqmp_validate_bitstream(desc, buf, bsize, bsize, &swap))
+		if (zynqmp_validate_bitstream(desc, buf, bsize,
+					      bsize, &swap))
 			return FPGA_FAIL;
 		bsizeptr = (u32 *)&bsize;
 		flush_dcache_range((ulong)bsizeptr,
 				   (ulong)bsizeptr + sizeof(size_t));
+		bsize_req = (u32)(uintptr_t)bsizeptr;
 		bstype |= BIT(ZYNQMP_FPGA_BIT_NS);
+	} else {
+		bstype = 0;
 	}
 
 	bin_buf = zynqmp_align_dma_buffer((u32 *)buf, bsize, swap);
 
-	debug("%s called!\n", __func__);
 	flush_dcache_range(bin_buf, bin_buf + bsize);
 
 	buf_lo = (u32)bin_buf;
 	buf_hi = upper_32_bits(bin_buf);
 
-	if (xilfpga_old)
-		ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
-					buf_hi, (u32)(uintptr_t)bsizeptr,
-					bstype, ret_payload);
-	else
-		ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo,
-					buf_hi, (u32)bsize, 0, ret_payload);
-
+	ret = xilinx_pm_request(PM_FPGA_LOAD, buf_lo, buf_hi,
+				bsize_req, bstype, ret_payload);
 	if (ret)
 		printf("PL FPGA LOAD failed with err: 0x%08x\n", ret);