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

["André Przywara" <andre.przywara@arm.com>]

On 15/04/2020 16:43, Ard Biesheuvel wrote:

Hi Ard,

> On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>
>> Hi,
>>
>> I've tested this patch by running badblocks and fio on a flash device inside a
>> guest, everything worked as expected.
>>
>> I've also looked at the flowcharts for device operation from Intel Application
>> Note 646, pages 12-21, and they seem implemented correctly.
>>
>> A few minor issues below.
>>
>> On 2/21/20 4:55 PM, Andre Przywara wrote:
>>> From: Raphael Gault <raphael.gault@arm.com>
>>>
>>> The EDK II UEFI firmware implementation requires some storage for the EFI
>>> variables, which is typically some flash storage.
>>> Since this is already supported on the EDK II side, we add a CFI flash
>>> emulation to kvmtool.
>>> This is backed by a file, specified via the --flash or -F command line
>>> option. Any flash writes done by the guest will immediately be reflected
>>> into this file (kvmtool mmap's the file).
>>> The flash will be limited to the nearest power-of-2 size, so only the
>>> first 2 MB of a 3 MB file will be used.
>>>
>>> This implements a CFI flash using the "Intel/Sharp extended command
>>> set", as specified in:
>>> - JEDEC JESD68.01
>>> - JEDEC JEP137B
>>> - Intel Application Note 646
>>> Some gaps in those specs have been filled by looking at real devices and
>>> other implementations (QEMU, Linux kernel driver).
>>>
>>> At the moment this relies on DT to advertise the base address of the
>>> flash memory (mapped into the MMIO address space) and is only enabled
>>> for ARM/ARM64. The emulation itself is architecture agnostic, though.
>>>
>>> This is one missing piece toward a working UEFI boot with kvmtool on
>>> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>>
> 
> I have given this a spin with UEFI built for kvmtool, and it appears
> to be working correctly. However, I noticed that it is intolerably
> slow, which seems to be caused by the fact that both array mode and
> command mode (or whatever it is called in the CFI spec) are fully
> emulated, whereas in the QEMU implementation (for instance), the
> region is actually exposed to the guest using a read-only KVM memslot
> in array mode, and so the read accesses are made natively.

Yes, I have been thinking about this, but didn't implement it this way
for the following reasons:
1) The use case here is pure UEFI firmware load, which should not be too
much affected by performance. At least this what I was thinking so far.
Your "intolerably slow" make me wonder if this assumption is not true.
Can you elaborate on that? Do you have any numbers? Is that due to the
trapping or something else?
2) As you mentioned, we need to switch between trapping and mapping,
upon the guest switching between array mode and command mode. So that
just adds complexity. Given 1) it didn't seem worth to do.

> It is also causing problems in the UEFI implementation, as we can no
> longer use unaligned accesses to read from the region, which is
> something the code currently relies on (and which works fine on actual
> hardware as long as you use normal non-cacheable mappings)

So this is something I was discussing with Sami about already:
It seems to me that a parallel memory mapped flash chip is actually a
device, just with the twist of behaving with normal (ROM) memory
semantics under certain circumstances. So for write accesses and read
access in any of the query modes we would definitely need to use device
memory and MMIO accessors, otherwise the compiler could mess up the
carefully crafted access semantics. So does EDK-2 use separate mappings
for both cases? Does it make sure to not access the array when being in
command mode? I couldn't find any trace of two mappings in the Linux
driver, IIRC.

> Are there any plans to implement this as well? I am aware that this is
> a big ask, but for the general utility of this feature, I think it is
> rather important.

I wasn't aware that this has a significant impact, so avoided the added
complexity. I doesn't sound like a big deal, though, so I might have a
look at it.

Cheers,
Andre

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