Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen

["Laszlo Ersek" <lersek@redhat.com>]

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-lersek@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-