From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id CABA0941B08 for ; Wed, 18 Oct 2023 15:13:36 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=XyUPw+SXsFWGfH/cW6PaKbe3BJU6ApZ9Pkj92bLg1iM=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1697642015; v=1; b=shCrkdC+AUwcz88bQa3XREKSlNgbmLL4DpAuC50s4dGgs8w5EfRUUYvjjMlIMxUztv9ArpTK quxL3SDYna0FVIzEzXurWxfQqCRdUgXWW4JsBrGFzhMilBVrLACozljTnFGX0ELw99m9ZK6TiBY tc5JRrkkGT1ctIWU7Mp41ebY= X-Received: by 127.0.0.2 with SMTP id QCjPYY7687511x3fWwzPRVi7; Wed, 18 Oct 2023 08:13:35 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.283708.1697642014816608988 for ; Wed, 18 Oct 2023 08:13:35 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-561-QLBIIgm4P4e9q_EIk6Y7Eg-1; Wed, 18 Oct 2023 11:13:32 -0400 X-MC-Unique: QLBIIgm4P4e9q_EIk6Y7Eg-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E80141C11712; Wed, 18 Oct 2023 15:13:31 +0000 (UTC) X-Received: from [10.39.192.202] (unknown [10.39.192.202]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 85D55492BFA; Wed, 18 Oct 2023 15:13:30 +0000 (UTC) Message-ID: <662ef213-a39d-4212-3e35-a788cf1cacb5@redhat.com> Date: Wed, 18 Oct 2023 17:13:29 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen To: Gerd Hoffmann Cc: devel@edk2.groups.io, Jordan Justen , Ard Biesheuvel , Oliver Steffen , Jiewen Yao References: <20231018103328.91093-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 19GTsDQ7HpGhJ4cEOPnqpBnxx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=shCrkdC+; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 . (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] -=-=-=-=-=-=-=-=-=-=-=-