linux-kernel - Re: [PATCH] zynq-fpga: Only route PR via PCAP when required

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Oct 2018 12:04:45 +0100
From:   Moritz Fischer <mdf@...nel.org>
To:     Mike Looijmans <mike.looijmans@...ic.nl>
Cc:     Moritz Fischer <mdf@...nel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "atull@...nel.org" <atull@...nel.org>
Subject: Re: [PATCH] zynq-fpga: Only route PR via PCAP when required

Hi Mike,

On Tue, Oct 23, 2018 at 10:53:50AM +0000, Mike Looijmans wrote:
> On 23-10-18 11:01, Moritz Fischer wrote:
> > Hi Mike,
> > 
> > seems like a good usecase (though uncommon), question below
> 
> Usecases for ICAP:
> - It's considerably faster than PCAP
> - Self-repairing logic (e.g. single-event upsets)
> - Being programmed from a remote FPGA
> - Programming through another bus (e.g. PCIe)

Yeah, I wasn't saying it's a bad usecase, just not super common :)

> 
> 
> > 
> > On Tue, Oct 23, 2018 at 08:31:19AM +0200, Mike Looijmans wrote:
> >> The Xilinx Zynq FPGA driver takes ownership of the PR interface, making
> >> it impossible to use the ICAP interface for partial reconfiguration.
> >>
> >> This patch changes the driver to only activate PR over PCAP while the
> >> device is actively being accessed by the driver for programming.
> >>
> >> This allows both PCAP and ICAP interfaces to be used for PR.
> >>
> >> Signed-off-by: Mike Looijmans <mike.looijmans@...ic.nl>
> >> ---
> >>   drivers/fpga/zynq-fpga.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> >> index 3110e00..f6c205a 100644
> >> --- a/drivers/fpga/zynq-fpga.c
> >> +++ b/drivers/fpga/zynq-fpga.c
> >> @@ -497,6 +497,10 @@ static int zynq_fpga_ops_write_complete(struct fpga_manager *mgr,
> >>   	int err;
> >>   	u32 intr_status;
> >>   
> >> +	/* Release 'PR' control back to the ICAP */
> >> +	zynq_fpga_write(priv, CTRL_OFFSET,
> >> +		zynq_fpga_read(priv, CTRL_OFFSET) & ~CTRL_PCAP_PR_MASK);
> >> +
> > 
> > Shouldn't that be after the below stanza that enables the clock?
> 
> I'm actually not sure, and I did not encounter any problems while testing 
> this, but it's easier to just move it than to find out, so I'll go for "yes, 
> let's enable the clock first".
> I'll await a bit more feedback and post a v2 for that.

Ok, I had suspected you tested it and probably didn't run into issues,
but just wanted to make sure we think about it. If you don't mind, swap
it around as I suggested just to be safe.

With that change feel free to add my Reviewed-by: Moritz Fischer
<mdf@...nel.org> in your v2.

Thanks for the patch,

Moritz