Re: [PATCH kvmtool v4 0/5] Add CFI flash emulation

From: "André Przywara" <andre.przywara@arm.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: kvm@vger.kernel.org, Raphael Gault <raphael.gault@arm.com>,
	Sami Mujawar <sami.mujawar@arm.com>,
	Will Deacon <will@kernel.org>,
	kvmarm <kvmarm@lists.cs.columbia.edu>
Subject: Re: [PATCH kvmtool v4 0/5] Add CFI flash emulation
Date: Thu, 23 Apr 2020 22:31:24 +0100	[thread overview]
Message-ID: <9e742184-86c1-a4be-c2cb-fe96979e0f1f@arm.com> (raw)
In-Reply-To: <CAMj1kXEjckV3HzcX_XXTSn-tDDQ5H8=LgteDcP5USThn=OgTQg@mail.gmail.com>

On 23/04/2020 21:43, Ard Biesheuvel wrote:

Hi Ard,

> On Thu, 23 Apr 2020 at 19:55, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Thu, 23 Apr 2020 at 19:39, Andre Przywara <andre.przywara@arm.com> wrote:
>>>
>>> Hi,
>>>
>>> an update for the CFI flash emulation, addressing Alex' comments and
>>> adding direct mapping support.
>>> The actual code changes to the flash emulation are minimal, mostly this
>>> is about renaming and cleanups.
>>> This versions now adds some patches. 1/5 is a required fix, the last
>>> three patches add mapping support as an extension. See below.
>>>
>>> In addition to a branch with this series[1], I also put a git branch with
>>> all the changes compared to v3[2] as separate patches on the server, please
>>> have a look if you want to verify against a previous review.
>>>
>>> ===============
>>> 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, and looks like a
>>> generic standard, this series adds a CFI flash emulation to kvmtool.
>>>
>>> Patch 2/5 is the actual emulation code, patch 1/5 is a bug-fix for
>>> registering MMIO devices, which is needed for this device.
>>> Patches 3-5 add support for mapping the flash memory into guest, should
>>> it be in read-array mode. For this to work, patch 3/5 is cherry-picked
>>> from Alex' PCIe reassignable BAR series, to support removing a memslot
>>> mapping. Patch 4/5 adds support for read-only mappings, while patch 5/5
>>> adds or removes the mapping based on the current state.
>>> I am happy to squash 5/5 into 2/5, if we agree that patch 3/5 should be
>>> merged either separately or the PCIe series is actually merged before
>>> this one.
>>>
>>> This is one missing piece towards a working UEFI boot with kvmtool on
>>> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>> This series alone already enables UEFI boot, but only with virtio-mmio.
>>>
>>
>> Excellent! Thanks for taking the time to implement the r/o memslot for
>> the flash, it really makes the UEFI firmware much more usable.
>>
>> I will test this as soon as I get a chance, probably tomorrow.
>>
> 
> I tested this on a SynQuacer box as a host, using EFI firmware [0]
> built from patches provided by Sami.
> 
> I booted the Debian buster installer, completed the installation, and
> could boot into the system. The only glitch was the fact that the
> reboot didn't work, but I suppose we are not preserving the memory the
> contains the firmware image, so there is nothing to reboot into.

It's even worth, kvmtool does actually not support reset at all:
https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/tree/kvm-cpu.c#n220

And yeah, the UEFI firmware is loaded at the beginning of RAM, so most
of it is long gone by then.
kvmtool could reload the image and reset the VCPUs, but every device
emulation would need to be reset, for which there is no code yet.

> But
> just restarting kvmtool with the flash image containing the EFI
> variables got me straight into GRUB and the installed OS.

So, yeah, this is the way to do it ;-)

> Tested-by: Ard Biesheuvel <ardb@kernel.org>

Many thanks for that!

> Thanks again for getting this sorted.

It was actually easier than I thought (see the last patch).

Just curious: the images Sami gave me this morning did not show any
issues anymore (no no-syndrome fault, no alignment issues), even without
the mapping [1]. And even though I saw the 800k read traps, I didn't
notice any real performance difference (on a Juno). The PXE timeout was
definitely much more noticeable.

So did you see any performance impact with this series?

> [0] https://people.linaro.org/~ard.biesheuvel/KVMTOOL_EFI.fd

Ah, nice, will this stay there for a while? I can't provide binaries,
but wanted others to be able to easily test this.

Cheers,
Andre.

[1]
http://www.linux-arm.org/git?p=kvmtool.git;a=commitdiff;h=2f2cf67f9514894d88e9ca799bb9dacd1f7557d4
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm