On 10/18/23 15:13, Gerd Hoffmann wrote:
> Hi,
>
>>> - if (!VirtioFsFile->IsDirectory) {
>>> + if (!VirtioFsFile->IsDirectory && FileName[0] != '\\') {
>
>> It's nice to see this topic pop up on edk2-devel; apparently you started
>> testing shim on top of virtio-fs. :)
>
> Indeed. For starters just passed the host /boot/efi into the guest to
> see how far it goes.
>
> Amazingly ranking virtiofs first in bootorder JustWorks[tm].
Well, there's no magic :)
On one side, in OVMF's QemuBootOrderLib, the TranslatePciOfwNodes()
function contains a "Generic OpenFirmware device path for PCI devices"
branch (a catch-all branch, if none of the more specific branches match).
On the other side, I had changed QEMU to produce a "bootorder" fw_cfg
entry for virtio-fs, in commit 6da32fe5efdd ("vhost-user-fs: add the
"bootindex" property", 2021-01-13) -- specifically for this purpose.
(The QEMU commit message shows an example OFW device path:
/pci@i0cf8/pci-bridge@1,6/pci1af4,105a@0/filesystem@0
And, if I remember correctly, the "pci1af4,105a" part was so ugly that I
decided not to add a specific branch for it in QemuBootOrderLib!)
>
> Next was shim trapping into this when trying to read BOOTX64.CSV (i.e.
> probably it actually was fallback.efi) and a boot loop.
>
>>> strictly since the beginning, and a few months ago I reported USWG
>>> Mantis
>>> ticket #2367 [1] too, for clearing up the related confusion in the UEFI
>>> spec.
>
> I have to admit I didn't check the what the spec says yet. Looking ...
>
> Hmm, EFI_FILE_PROTOCOL.Open() description says "This would typically be
> an open handle to a directory". I don't read that as "MUST be a
> directory".
>
> With an absolute path the only thing we need from the Handle passed in
> is the reference to the Filesystem root, the actual file/directory is
> not needed, so accepting anything and not only directories makes sense
> to me. And most existing implementations apparently agree, otherwise we
> would see much more fallout from this.
>
> Updating the spec to clearly document expected behavior in that corner
> case would be good.
The spec is inconsistent, in multiple ways.
* Open() is inconsistent with OpenEx(), as the latter does require
"This" to be a directory -- the Summary on OpenEx() says, "Opens a new
file relative to the source directory’s location."
* The "location of base file handle" is inconsistently defined
(ill-defined) by the spec, between the base file being a directory (-->
implying a "child" relationship), versus the base file being a regular
file (--> implying a "sibling" relationship at best). This definition is
more fundamental IMO than the pathname being absolute or relative.
>> (2) With this modification in place, shim is happy, but grub isn't. When
>> I realized that, I looked relatively deeply into making grub work on top
>> of virtio-fs as well -- and my findings were horrendous.
>>
>> I wrote up my findings in a private email to some colleagues; you were
>> among the recipients.
>
> Yes, I remember.
>
> Didn't try anything in grub yet. It boots to the grub shell prompt, but
> there is no kernel on the host ESP anyway. And no real grub.cfg either,
> only a stub pointing to the real grub.cfg on a different filesystem.
>
> But, yes, that is probably a dead end. IIRC grub completely ignores the
> efi filesystem drivers and uses its own exclusively (using its own
> interfaces, not registering them in efi).
That's how I remember it too; grub uses BlockIo / DiskIo protocols as
the basis for its own filesystem drivers, and does not consume EFI
SimpleFs at all.
>
> Trying to fix that opens the can of worms where you can have both grub
> and efi filesystem drivers being active and stomping on each others
> feet.
Right; in a nutshell, that's the core issue.
>
>> If you have a use case where you rely on shim but *not* on Grub
>> (UKIs?), then I'm OK relaxing the strictness of VirtioFsDxe.
>
> No concrete plans yet. There are ideas/plans to allow booting container
> images as VM. In that case the root fileystem would be virtiofs, and
> given that OVMF has a driver it IMHO makes sense to use that to boot the
> system.
I'd love that; it was one idea behind
<https://bugzilla.redhat.com/show_bug.cgi?id=1741615>. (Unfortunately
that BZ is private; I don't know why -- probably because it was cloned
from a kernel ticket.)
> And, yes, using UKIs for that makes sense, although the exact
> workflow is far from being clear.
>
> I think one possible option would be to simply have an /EFI subdirectory
> on the root filesystem, place the files you would have on the ESP there,
> so the rootfs becomes EFI-bootable. No need to have a boot filesystem
> or directory, given that EFI can read the complete filesystem anyway you
> can slurp the UKIs directly from /lib/modules/$kernel/vmlinuz-virt.efi.
>
>> In that case, I'd prefer upstreaming my above patch, from April,
>> rather than taking yours. What do you think about that?
>
> Fine with me.
Thanks, I'll submit the patch then.
>
> The UEFI spec is not clear and I don't feel like bikeshedding whenever
> shim behavior is a bug or not.
That's very generous of you (I'm not kidding -- I'm serious).
I am not that generous; I insist we call this -- for now! -- a shim and
UEFI spec bug. The reason is that, when I was writing
VirtioFsSimpleFileOpen(), I struggled for a *very* long time making
heads or tails of the spec. I decided, already back then, that the spec
was buggy, so I chose the safest (most restrictive) interpretation.
If the spec gets changed "my way" (i.e., restricting the behavior, which
officially makes shim's current behavior a bug), then my edk2 patch can
remain in place. If the spec gets clarified the opposite way, then edk2
my patch can (and should) be replaced by yours.
> Once the UEFI spec has been fixed the
> one way or other we can revisit this.
Yes.
>
>> Here's a further (independent) caveat: if you are using VirtioFsDxe with
>> the rust language virtiofsd, then you might experience hangs in
>> VirtioFsInit. For fixing that, you need the following *qemu* patch set:
>>
>> [PATCH v3 0/7] vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously
>> https://patchew.org/QEMU/20231002203221.17241-1-ler...@redhat.com/
>>
>> (This patch set has been on qemu-devel for nearly 2 months now, counting
>> from v1; I'm going to ping MST again. It's been ready for merging for
>> weeks now!)
>
> Good to know, I think I tapped into this when I tried to pass the
> complete host filesystem into the guest.
On a tangent: the somewhat funny thing is that, the way the Linux
virtiofs driver is implemented, an indefinite time delay is *guaranteed*
to exist between the virtio setup and the FUSE_INIT message, in
practice. And that masks the race. Details here:
http://mid.mail-archive.com/b24315a1-906d-f3af-02e5-e6788765639c@redhat.com
> IIRC mst tends to do large pulls in large intervals, I wouldn't worry
> too much yet. Pinging doesn't hurt though.
Yes, Michael is on it now.
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109762): https://edk2.groups.io/g/devel/message/109762
Mute This Topic: https://groups.io/mt/102036263/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
