Re: [PATCH kvmtool] Add emulation for CFI compatible flash memory

From: Andre Przywara <andre.przywara@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Raphael Gault <raphael.gault@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Sami Mujawar <sami.mujawar@arm.com>,
	Robin Murphy <Robin.Murphy@arm.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH kvmtool] Add emulation for CFI compatible flash memory
Date: Thu, 6 Feb 2020 17:13:05 +0000	[thread overview]
Message-ID: <20200206171305.4029832c@donnerap.cambridge.arm.com> (raw)
In-Reply-To: <20200206144211.34a27285@donnerap.cambridge.arm.com>

On Thu, 6 Feb 2020 14:42:11 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

Hi,

> On Wed, 5 Feb 2020 17:11:57 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> Hi Will,
> 
> many thanks for having a look!
> 
> > On Wed, Jan 08, 2020 at 06:32:12PM +0000, Andre Przywara wrote:  
> > > From: Raphael Gault <raphael.gault@arm.com>

[ ... ]

> > > +/* We only support synchronous page mode read accesses. */
> > > +static void read_flash(struct cfi_flash_device *sfdev,
> > > +		       u64 addr, u8 *buffer, int len)
> > > +{
> > > +	memcpy(buffer, sfdev->flash_memory + addr, len);
> > > +}    
> > 
> > Hmm, you open-code the memcpy when writing the flash so it's a bit weird  
> 
> Not sure what you refer to exactly?
> The only open-code access I see is in the MMIO handler when doing the CFI QRY *read*, which is a very special MMIO style read access. Every other write access (word_program(), buffer_confirm()) is already using memcpy.
> What am I missing here?

Robin pointed out that you probably mean that there is a wrapper around the memcpy on read_flash, but not on the write operations?
The reason for that is that read and write are two very different operations on any flash memory: the read side is following proper memory semantics: no side effects, could be cached, etc. I was briefly tempted to actually map it r/o into the guest, but the problem is that this semantics only holds when we are in read mode. In any other mode and whenever we write, the CFI flash is actually an MMIO mapped device, where each access triggers something and the access width matters. So that would require frequently changing the memslot, because we need to trap reads when not in read mode.

Anyway, adding a write wrapper doesn't make sense here, but I can of course easily replace the read_flash() call with the respective memcpy() line.

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel