Re: rutabaga 0.1.3
On Mon, Mar 04, 2024 at 04:23:20PM -0800, Gurchetan Singh wrote: > On Sat, Mar 2, 2024 at 6:38 AM Alyssa Ross wrote: > > > Hi Gurchetan, > > > > > >> > Would this be a suitable commit for the 0.1.3 release of rutabaga? > > > >> > > > > >> > https://chromium.googlesource.com/crosvm/crosvm/+/5dfd74a0680d317c6edf44138def886f47cb1c7c > > > >> > > > > >> > The gfxstream/AEMU commits would remain unchanged. > > > >> > > > >> That combination works for me. > > > > > > > > Just FYI, still working on it. Could take 1-2 more weeks. > > > > > > FYI: > > > > > > https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/tags/v0.1.2-gfxstream-release > > > > > > https://android.googlesource.com/platform/hardware/google/aemu/+/refs/tags/v0.1.2-aemu-release > > > > > > > > https://chromium.googlesource.com/crosvm/crosvm/+/refs/tags/v0.1.3-rutabaga-release > > > > Unlike the commit I tested for you, the commit that ended up being > > tagged as v0.1.3-rutabaga-release doesn't work for me: > > > > qemu: The errno is EBADF: Bad file number > > qemu: CHECK failed in rutabaga_cmd_resource_map_blob() > > ../hw/display/virtio-gpu-rutabaga.c:655 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 > > qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() > > ../hw/display/virtio-gpu-rutabaga.c:723 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 > > qemu: The errno is EBADF: Bad file number > > qemu: CHECK failed in rutabaga_cmd_resource_map_blob() > > ../hw/display/virtio-gpu-rutabaga.c:655 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 > > qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() > > ../hw/display/virtio-gpu-rutabaga.c:723 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 > > qemu: The errno is EBADF: Bad file number > > qemu: CHECK failed in rutabaga_cmd_resource_map_blob() > > ../hw/display/virtio-gpu-rutabaga.c:655 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 > > qemu: invalid resource id > > qemu: CHECK failed in rutabaga_cmd_submit_3d() > > ../hw/display/virtio-gpu-rutabaga.c:341 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x207, error 0x1200 > > qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() > > ../hw/display/virtio-gpu-rutabaga.c:723 > > qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 > > > > Thank you for the bug report .. does crrev.com/c/5342655 fix this for you? Hi Gurchetan, thanks for looking into it, and sorry for the late reply. Alas it doesn't seem to make a difference. (The commit message is also incorrect. AsFd is implemented for SafeDescriptor in rutabaga_gfx/src/rutabaga_os/sys/linux/descriptor.rs.) > I bisected it to: > > > > commit f3dbf20eedadb135e2fd813474fbb9731d465f3a > > Author: Andrew Walbran > > Date: Wed Nov 29 17:23:45 2023 + > > > > rutabaga_gfx: Uprev nix to 0.27.1 > > > > The new version of nix uses OwnedFd in various places, which > > allows us > > to have less unsafe code. > > > > TEST=CQ > > BUG=b:293289578 > > > > Change-Id: I61aa80c4105eaf1182c5c325109b5aba11cf60de > > Reviewed-on: > > https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5072293 > > Auto-Submit: Andrew Walbran > > Reviewed-by: Gurchetan Singh > > Reviewed-by: Frederick Mayle > > Commit-Queue: Frederick Mayle > > signature.asc Description: PGP signature
Re: rutabaga 0.1.3
Hi Gurchetan, > >> > Would this be a suitable commit for the 0.1.3 release of rutabaga? > >> > > >> > https://chromium.googlesource.com/crosvm/crosvm/+/5dfd74a0680d317c6edf44138def886f47cb1c7c > >> > > >> > The gfxstream/AEMU commits would remain unchanged. > >> > >> That combination works for me. > > > > Just FYI, still working on it. Could take 1-2 more weeks. > > FYI: > > https://android.googlesource.com/platform/hardware/google/gfxstream/+/refs/tags/v0.1.2-gfxstream-release > > https://android.googlesource.com/platform/hardware/google/aemu/+/refs/tags/v0.1.2-aemu-release > > https://chromium.googlesource.com/crosvm/crosvm/+/refs/tags/v0.1.3-rutabaga-release Unlike the commit I tested for you, the commit that ended up being tagged as v0.1.3-rutabaga-release doesn't work for me: qemu: The errno is EBADF: Bad file number qemu: CHECK failed in rutabaga_cmd_resource_map_blob() ../hw/display/virtio-gpu-rutabaga.c:655 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() ../hw/display/virtio-gpu-rutabaga.c:723 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 qemu: The errno is EBADF: Bad file number qemu: CHECK failed in rutabaga_cmd_resource_map_blob() ../hw/display/virtio-gpu-rutabaga.c:655 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() ../hw/display/virtio-gpu-rutabaga.c:723 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 qemu: The errno is EBADF: Bad file number qemu: CHECK failed in rutabaga_cmd_resource_map_blob() ../hw/display/virtio-gpu-rutabaga.c:655 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x208, error 0x1200 qemu: invalid resource id qemu: CHECK failed in rutabaga_cmd_submit_3d() ../hw/display/virtio-gpu-rutabaga.c:341 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x207, error 0x1200 qemu: CHECK failed in rutabaga_cmd_resource_unmap_blob() ../hw/display/virtio-gpu-rutabaga.c:723 qemu: virtio_gpu_rutabaga_process_cmd: ctrl 0x209, error 0x1200 I bisected it to: commit f3dbf20eedadb135e2fd813474fbb9731d465f3a Author: Andrew Walbran Date: Wed Nov 29 17:23:45 2023 + rutabaga_gfx: Uprev nix to 0.27.1 The new version of nix uses OwnedFd in various places, which allows us to have less unsafe code. TEST=CQ BUG=b:293289578 Change-Id: I61aa80c4105eaf1182c5c325109b5aba11cf60de Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5072293 Auto-Submit: Andrew Walbran Reviewed-by: Gurchetan Singh Reviewed-by: Frederick Mayle Commit-Queue: Frederick Mayle signature.asc Description: PGP signature
Re: [PATCH v15 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes:
> On Sat, Jan 20, 2024 at 4:19 AM Alyssa Ross wrote:
>
>> Gurchetan Singh writes:
>>
>> > On Fri, Jan 19, 2024 at 1:13 PM Alyssa Ross wrote:
>> >>
>> >> Hi Gurchetan,
>> >>
>> >> > Thanks for the reminder. I did make a request to create the release
>> >> > tags, but changes were requested by Fedora packaging effort:
>> >> >
>> >> > https://bugzilla.redhat.com/show_bug.cgi?id=2242058
>> >> > https://bugzilla.redhat.com/show_bug.cgi?id=2241701
>> >> >
>> >> > So the request was canceled, but never re-requested. I'll fire off
>> >> > another request, with:
>> >> >
>> >> > gfxstream: 23d05703b94035ac045df60823fb1fc4be0fdf1c ("gfxstream:
>> >> > manually add debug logic")
>> >> > AEMU: dd8b929c247ce9872c775e0e5ddc4300011d0e82 ("aemu: improve
>> licensing")
>> >> >
>> >> > as the commits. These match the Fedora requests, and the AEMU one has
>> >> > been merged into Fedora already it seems.
>> >>
>> >> These revisions have the problem I mentioned in my previous message:
>> >>
>> >> >> The gfxstream ref mentioned here isn't compatible with
>> >> >> v0.1.2-rutabaga-release, because it no longer provides
>> logging_base.pc,
>> >>
>> >> rutabaga was not fixed to use the new AEMU package names until after the
>> >> v0.1.2-rutabaga-release tag, in commit 5dfd74a06. So will there be a
>> >> new Rutabaga release that's compatible with these release versions of
>> >> gfxstream and AEMU?
>> >
>> > Good catch.
>> >
>> > One possible workaround is to build gfxstream as a shared library. I
>> > think that would avoid rutabaga looking for AEMU package config files.
>> >
>> > But if another rutabaga release is desired with support for a static
>> > library, then we can make that happen too.
>>
>> We're exclusively building gfxstream as a shared library.
>>
>> Looking at rutabaga's build.rs, it appears to me like pkg-config is
>> always used for gfxstream unless overridden by GFXSTREAM_PATH.
>>
>
> Hmm, it seems we should be checking pkg-config --static before looking for
> AEMU in build.rs -- oh well.
>
> Would this be a suitable commit for the 0.1.3 release of rutabaga?
>
> https://chromium.googlesource.com/crosvm/crosvm/+/5dfd74a0680d317c6edf44138def886f47cb1c7c
>
> The gfxstream/AEMU commits would remain unchanged.
That combination works for me.
signature.asc
Description: PGP signature
Re: [PATCH v15 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes:
> On Fri, Jan 19, 2024 at 1:13 PM Alyssa Ross wrote:
>>
>> Hi Gurchetan,
>>
>> > Thanks for the reminder. I did make a request to create the release
>> > tags, but changes were requested by Fedora packaging effort:
>> >
>> > https://bugzilla.redhat.com/show_bug.cgi?id=2242058
>> > https://bugzilla.redhat.com/show_bug.cgi?id=2241701
>> >
>> > So the request was canceled, but never re-requested. I'll fire off
>> > another request, with:
>> >
>> > gfxstream: 23d05703b94035ac045df60823fb1fc4be0fdf1c ("gfxstream:
>> > manually add debug logic")
>> > AEMU: dd8b929c247ce9872c775e0e5ddc4300011d0e82 ("aemu: improve licensing")
>> >
>> > as the commits. These match the Fedora requests, and the AEMU one has
>> > been merged into Fedora already it seems.
>>
>> These revisions have the problem I mentioned in my previous message:
>>
>> >> The gfxstream ref mentioned here isn't compatible with
>> >> v0.1.2-rutabaga-release, because it no longer provides logging_base.pc,
>>
>> rutabaga was not fixed to use the new AEMU package names until after the
>> v0.1.2-rutabaga-release tag, in commit 5dfd74a06. So will there be a
>> new Rutabaga release that's compatible with these release versions of
>> gfxstream and AEMU?
>
> Good catch.
>
> One possible workaround is to build gfxstream as a shared library. I
> think that would avoid rutabaga looking for AEMU package config files.
>
> But if another rutabaga release is desired with support for a static
> library, then we can make that happen too.
We're exclusively building gfxstream as a shared library.
Looking at rutabaga's build.rs, it appears to me like pkg-config is
always used for gfxstream unless overridden by GFXSTREAM_PATH.
signature.asc
Description: PGP signature
Re: [PATCH v15 0/9] rutabaga_gfx + gfxstream
Hi Gurchetan,
> Thanks for the reminder. I did make a request to create the release
> tags, but changes were requested by Fedora packaging effort:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=2242058
> https://bugzilla.redhat.com/show_bug.cgi?id=2241701
>
> So the request was canceled, but never re-requested. I'll fire off
> another request, with:
>
> gfxstream: 23d05703b94035ac045df60823fb1fc4be0fdf1c ("gfxstream:
> manually add debug logic")
> AEMU: dd8b929c247ce9872c775e0e5ddc4300011d0e82 ("aemu: improve licensing")
>
> as the commits. These match the Fedora requests, and the AEMU one has
> been merged into Fedora already it seems.
These revisions have the problem I mentioned in my previous message:
>> The gfxstream ref mentioned here isn't compatible with
>> v0.1.2-rutabaga-release, because it no longer provides logging_base.pc,
rutabaga was not fixed to use the new AEMU package names until after the
v0.1.2-rutabaga-release tag, in commit 5dfd74a06. So will there be a
new Rutabaga release that's compatible with these release versions of
gfxstream and AEMU?
signature.asc
Description: PGP signature
Re: [PATCH v15 0/9] rutabaga_gfx + gfxstream
Hi Gurchetan, Gurchetan Singh writes: > - As mentioned in v14: > * AEMU: d6e6b99 "Delete VpxFrameParser.cpp" > * gfxstream: 2131f78d Merge "gfxstream: add egl & gles deps.." > > are the proposed v.0.1.2 release points. If those commits are sufficient > for packaging AEMU + gfxstream, let me know and I'll have official release > tags made. If additional changes are required for packaging, let me know > as well. Were these releases ever made? The gfxstream ref mentioned here isn't compatible with v0.1.2-rutabaga-release, because it no longer provides logging_base.pc, and this email is the last mention I can find of these releases. In Nixpkgs, I've gone for packaging gfxstream and aemu with your initial proposed release points, which works fine, but it would be great to have this clearer upstream. signature.asc Description: PGP signature
Re: [PATCH v13 6/9] gfxstream + rutabaga: add initial support for gfxstream
Akihiko Odaki writes: > Practically there is very low chance to hit the bug. I think only > fuzzers and malicious actors will trigger it, and probably no one will > dare using virtio-gpu-rutabaga or virtio-gpu-gl in a security-sensitive > context. Well, this is exactly what Chrome OS does, albiet with crosvm rather than QEMU, right? signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes: > It's harder to get the attention of the Android build team than the Chrome > build team. Though, there are a few issues with AEMU/gfxstream packaging > we also need to figure out -- see "[PATCH v13 0/9] rutabaga_gfx + > gfxstream" for details -- interested in your opinion on the matter! None of the other points there are issues for me — in Nixpkgs, every package is installed to a unique prefix (different versions of the same package, or even just different build recipes for the same version, or different dependencies result in different prefixes), so library versioning and the /usr/include directories are not blockers. Static libraries are also fine for Nixpkgs — any change to a library, static or dynamic, causes all dependents to be rebuild against the new library, so the only real disadvantage to static libraries is the duplication on disk, which isn't a big deal. All that's to say, I'm ready to have rutabaga support, including gfxstream, in our QEMU package, as soon as a release of QEMU including it is made. Everything Marc-André has identified would still be nice to have fixed, but for us specifically, none of it is a blocker, even the tags I asked for. signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes: > On Fri, Aug 25, 2023 at 12:37 PM Alyssa Ross wrote: > >> Alyssa Ross writes: >> >> > Gurchetan Singh writes: >> > >> >> On Fri, Aug 25, 2023 at 12:11 AM Alyssa Ross wrote: >> >> >> >>> Gurchetan Singh writes: >> >>> >> >>> > On Wed, Aug 23, 2023 at 4:07 AM Alyssa Ross wrote: >> >>> > >> >>> >> Gurchetan Singh writes: >> >>> >> >> >>> >> > - Official "release commits" issued for rutabaga_gfx_ffi, >> >>> >> > gfxstream, aemu-base. For example, see crrev.com/c/4778941 >> >>> >> > >> >>> >> > - The release commits can make packaging easier, though once >> >>> >> > again all known users will likely just build from sources >> >>> >> > anyways >> >>> >> >> >>> >> It's a small thing, but could there be actual tags, rather than just >> >>> >> blessed commits? It'd just make them easier to find, and save a >> bit of >> >>> >> time in review for packages. >> >>> >> >> >>> > >> >>> > I added: >> >>> > >> >>> > >> >>> >> https://crosvm.dev/book/appendix/rutabaga_gfx.html#latest-releases-for-potential-packaging >> >>> > >> >>> > Tags are possible, but I want to clarify the use case before >> packaging. >> >>> > Where are you thinking of packaging it for (Debian??)? Are you mostly >> >>> > interested in Wayland passthrough (my guess) or gfxstream too? >> Depending >> >>> > your use case, we may be able to minimize the work involved. >> >>> >> >>> Packaging for Nixpkgs (where I already maintain what to my knowledge is >> >>> the only crosvm distro package). I'm personally mostly interested in >> >>> Wayland passthroug, but I wouldn't be surprised if others are >> interested >> >>> in gfxstream. The packaging work is already done, I've just been >> >>> holding off actually pushing the packages waiting for the stable >> >>> releases. >> >>> >> >>> The reason that tags would be useful is that it allows a reviewer of >> the >> >>> package to see at a glance that the package is built from a stable >> >>> release. If it's just built from a commit hash, they have to go and >> >>> verify that it's a stable release, which is mildly annoying and >> >>> unconventional. >> >>> >> >> >> >> Understood. Request to have gfxstream and AEMU v0.1.2 release tags >> made. >> >> >> >> For rutabaga_gfx_ffi, is the crates.io upload sufficient? >> >> >> >> https://crates.io/crates/rutabaga_gfx_ffi >> >> >> >> Debian, for example, treats crates.io as the source of truth and builds >> >> tooling around that. I wonder if Nixpkgs as similar tooling around >> >> crates.io. >> > >> > We do, and I'll use the crates.io release for the package — good >> > suggestion, but it's still useful to also have a tag in a git repo. It >> > makes it easier if I need to do a bisect, for example. As a distro >> > developer, I'm frequently jumping across codebases I am not very >> > familiar with to try to track down regressions, etc., and it's much >> > easier when I don't have to learn some special quirk of the package like >> > not having git tags. >> >> Aha, trying to switch my package over to it has revealed that there is >> actually a reason not to use the crates.io release. It doesn't include >> a Cargo.lock, which would mean we'd have to obtain one from elsewhere. >> Either from the crosvm git repo, at which point we might just get all >> the sources from there, or by vendoring a Cargo.lock into our own git >> tree for packages, which we try to avoid because when you have a lot of >> them, they become quite a large proportion of the overall size of the >> repo. >> > > Ack. Request to have a rutabaga release tag in crosvm also made, should be > complete in a few days. Thanks! I've found the rutabaga tag, but I still don't see any relevant tags for aemu or gfxstream. Any news there? signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Alyssa Ross writes: > Gurchetan Singh writes: > >> On Fri, Aug 25, 2023 at 12:11 AM Alyssa Ross wrote: >> >>> Gurchetan Singh writes: >>> >>> > On Wed, Aug 23, 2023 at 4:07 AM Alyssa Ross wrote: >>> > >>> >> Gurchetan Singh writes: >>> >> >>> >> > - Official "release commits" issued for rutabaga_gfx_ffi, >>> >> > gfxstream, aemu-base. For example, see crrev.com/c/4778941 >>> >> > >>> >> > - The release commits can make packaging easier, though once >>> >> > again all known users will likely just build from sources >>> >> > anyways >>> >> >>> >> It's a small thing, but could there be actual tags, rather than just >>> >> blessed commits? It'd just make them easier to find, and save a bit of >>> >> time in review for packages. >>> >> >>> > >>> > I added: >>> > >>> > >>> https://crosvm.dev/book/appendix/rutabaga_gfx.html#latest-releases-for-potential-packaging >>> > >>> > Tags are possible, but I want to clarify the use case before packaging. >>> > Where are you thinking of packaging it for (Debian??)? Are you mostly >>> > interested in Wayland passthrough (my guess) or gfxstream too? Depending >>> > your use case, we may be able to minimize the work involved. >>> >>> Packaging for Nixpkgs (where I already maintain what to my knowledge is >>> the only crosvm distro package). I'm personally mostly interested in >>> Wayland passthroug, but I wouldn't be surprised if others are interested >>> in gfxstream. The packaging work is already done, I've just been >>> holding off actually pushing the packages waiting for the stable >>> releases. >>> >>> The reason that tags would be useful is that it allows a reviewer of the >>> package to see at a glance that the package is built from a stable >>> release. If it's just built from a commit hash, they have to go and >>> verify that it's a stable release, which is mildly annoying and >>> unconventional. >>> >> >> Understood. Request to have gfxstream and AEMU v0.1.2 release tags made. >> >> For rutabaga_gfx_ffi, is the crates.io upload sufficient? >> >> https://crates.io/crates/rutabaga_gfx_ffi >> >> Debian, for example, treats crates.io as the source of truth and builds >> tooling around that. I wonder if Nixpkgs as similar tooling around >> crates.io. > > We do, and I'll use the crates.io release for the package — good > suggestion, but it's still useful to also have a tag in a git repo. It > makes it easier if I need to do a bisect, for example. As a distro > developer, I'm frequently jumping across codebases I am not very > familiar with to try to track down regressions, etc., and it's much > easier when I don't have to learn some special quirk of the package like > not having git tags. Aha, trying to switch my package over to it has revealed that there is actually a reason not to use the crates.io release. It doesn't include a Cargo.lock, which would mean we'd have to obtain one from elsewhere. Either from the crosvm git repo, at which point we might just get all the sources from there, or by vendoring a Cargo.lock into our own git tree for packages, which we try to avoid because when you have a lot of them, they become quite a large proportion of the overall size of the repo. (This probably differs from Debian, etc., because in Nixpkgs, we don't package each crate dependency separately. We only have packages for applications (or occasionally, C ABI libraries written in Rust), and each of those gets to bring in whatever crate dependencies it wants as part of its build. This means we use the upstream Cargo.lock, and accept that different Rust packages will use lots of different versions of dependencies, which I don't believe is the case with other distros that take a more purist approach to Rust packaging.) signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes: > On Fri, Aug 25, 2023 at 12:11 AM Alyssa Ross wrote: > >> Gurchetan Singh writes: >> >> > On Wed, Aug 23, 2023 at 4:07 AM Alyssa Ross wrote: >> > >> >> Gurchetan Singh writes: >> >> >> >> > - Official "release commits" issued for rutabaga_gfx_ffi, >> >> > gfxstream, aemu-base. For example, see crrev.com/c/4778941 >> >> > >> >> > - The release commits can make packaging easier, though once >> >> > again all known users will likely just build from sources >> >> > anyways >> >> >> >> It's a small thing, but could there be actual tags, rather than just >> >> blessed commits? It'd just make them easier to find, and save a bit of >> >> time in review for packages. >> >> >> > >> > I added: >> > >> > >> https://crosvm.dev/book/appendix/rutabaga_gfx.html#latest-releases-for-potential-packaging >> > >> > Tags are possible, but I want to clarify the use case before packaging. >> > Where are you thinking of packaging it for (Debian??)? Are you mostly >> > interested in Wayland passthrough (my guess) or gfxstream too? Depending >> > your use case, we may be able to minimize the work involved. >> >> Packaging for Nixpkgs (where I already maintain what to my knowledge is >> the only crosvm distro package). I'm personally mostly interested in >> Wayland passthroug, but I wouldn't be surprised if others are interested >> in gfxstream. The packaging work is already done, I've just been >> holding off actually pushing the packages waiting for the stable >> releases. >> >> The reason that tags would be useful is that it allows a reviewer of the >> package to see at a glance that the package is built from a stable >> release. If it's just built from a commit hash, they have to go and >> verify that it's a stable release, which is mildly annoying and >> unconventional. >> > > Understood. Request to have gfxstream and AEMU v0.1.2 release tags made. > > For rutabaga_gfx_ffi, is the crates.io upload sufficient? > > https://crates.io/crates/rutabaga_gfx_ffi > > Debian, for example, treats crates.io as the source of truth and builds > tooling around that. I wonder if Nixpkgs as similar tooling around > crates.io. We do, and I'll use the crates.io release for the package — good suggestion, but it's still useful to also have a tag in a git repo. It makes it easier if I need to do a bisect, for example. As a distro developer, I'm frequently jumping across codebases I am not very familiar with to try to track down regressions, etc., and it's much easier when I don't have to learn some special quirk of the package like not having git tags. signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes: > On Wed, Aug 23, 2023 at 4:07 AM Alyssa Ross wrote: > >> Gurchetan Singh writes: >> >> > - Official "release commits" issued for rutabaga_gfx_ffi, >> > gfxstream, aemu-base. For example, see crrev.com/c/4778941 >> > >> > - The release commits can make packaging easier, though once >> > again all known users will likely just build from sources >> > anyways >> >> It's a small thing, but could there be actual tags, rather than just >> blessed commits? It'd just make them easier to find, and save a bit of >> time in review for packages. >> > > I added: > > https://crosvm.dev/book/appendix/rutabaga_gfx.html#latest-releases-for-potential-packaging > > Tags are possible, but I want to clarify the use case before packaging. > Where are you thinking of packaging it for (Debian??)? Are you mostly > interested in Wayland passthrough (my guess) or gfxstream too? Depending > your use case, we may be able to minimize the work involved. Packaging for Nixpkgs (where I already maintain what to my knowledge is the only crosvm distro package). I'm personally mostly interested in Wayland passthroug, but I wouldn't be surprised if others are interested in gfxstream. The packaging work is already done, I've just been holding off actually pushing the packages waiting for the stable releases. The reason that tags would be useful is that it allows a reviewer of the package to see at a glance that the package is built from a stable release. If it's just built from a commit hash, they have to go and verify that it's a stable release, which is mildly annoying and unconventional. signature.asc Description: PGP signature
Re: [PATCH v11 0/9] rutabaga_gfx + gfxstream
Gurchetan Singh writes: > - Official "release commits" issued for rutabaga_gfx_ffi, > gfxstream, aemu-base. For example, see crrev.com/c/4778941 > > - The release commits can make packaging easier, though once > again all known users will likely just build from sources > anyways It's a small thing, but could there be actual tags, rather than just blessed commits? It'd just make them easier to find, and save a bit of time in review for packages. signature.asc Description: PGP signature
Re: Rutabaga backwards compatibility
Gurchetan Singh writes: > On Tue, Aug 1, 2023 at 8:18 AM Alyssa Ross wrote: > >> Gurchetan Singh writes: >> >> > On Mon, Jul 24, 2023 at 2:56 AM Alyssa Ross wrote: >> >> >> >> Gurchetan Singh writes: >> >> >> >> > In terms of API stability/versioning/packaging, once this series is >> >> > reviewed, the plan is to cut a "gfxstream upstream release branch". We >> >> > will have the same API guarantees as any other QEMU project then, i.e no >> >> > breaking API changes for 5 years. >> >> >> >> What about Rutabaga? >> > >> > Yes, rutabaga + gfxstream will both be versioned and maintain API >> > backwards compatibility in line with QEMU guidelines. >> >> In that case, I should draw your attention to >> <https://crrev.com/c/4584252>, which I've just realised while testing v2 >> of your series here breaks the build of the rutabaga ffi, and which will >> require the addition of a "prot" field to struct rutabaga_handle (a >> breaking change). I'll push a new version of that CL to fix rutabaga >> ffi in the next few days. > > Sorry, I didn't see this until now. At first glance, do we need to modify > the rutabaga_handle? Can't we do fcntl(.., GET_FL) to get the access flags > when needed? That was my original approach[1], but it was difficult to make work on Windows and not popular. [1]: https://crrev.com/c/4543310 signature.asc Description: PGP signature
Rutabaga backwards compatibility
Gurchetan Singh writes: > On Mon, Jul 24, 2023 at 2:56 AM Alyssa Ross wrote: >> >> Gurchetan Singh writes: >> >> > In terms of API stability/versioning/packaging, once this series is >> > reviewed, the plan is to cut a "gfxstream upstream release branch". We >> > will have the same API guarantees as any other QEMU project then, i.e no >> > breaking API changes for 5 years. >> >> What about Rutabaga? > > Yes, rutabaga + gfxstream will both be versioned and maintain API > backwards compatibility in line with QEMU guidelines. In that case, I should draw your attention to <https://crrev.com/c/4584252>, which I've just realised while testing v2 of your series here breaks the build of the rutabaga ffi, and which will require the addition of a "prot" field to struct rutabaga_handle (a breaking change). I'll push a new version of that CL to fix rutabaga ffi in the next few days. Since this is already coming up, before the release has even been made, is it worth exploring how to limit the rutabaga API to avoid more breaking changes after the release? Could there be more use of opaque structs, for example? (CCing the crosvm list) signature.asc Description: PGP signature
Re: [PATCH v2 6/9] gfxstream + rutabaga: add initial support for gfxstream
Gurchetan Singh writes:
> +static int virtio_gpu_rutabaga_init(VirtIOGPU *g, Error **errp)
> +{
> +int result;
> +uint64_t capset_mask;
> +struct rutabaga_channels channels = { 0 };
> +struct rutabaga_builder builder = { 0 };
> +
> +VirtIOGPURutabaga *vr = VIRTIO_GPU_RUTABAGA(g);
> +vr->rutabaga = NULL;
> +
> +if (!vr->capset_names) {
> +error_setg(errp, "a capset name from virtio-gpu spec");
> +return -EINVAL;
> +}
> +
> +builder.wsi = RUTABAGA_WSI_SURFACELESS;
> +/*
> + * Currently, if WSI is specified, the only valid strings are
> "surfaceless"
> + * or "headless". Surfaceless doesn't create a native window surface,
> but
> + * does copy from the render target to the Pixman buffer if a virtio-gpu
> + * 2D hypercall is issued. Surfacless is the default.
> + *
> + * Headless is like surfaceless, but doesn't copy to the Pixman buffer.
> The
> + * use case is automated testing environments where there is no need to
> view
> + * results.
> + *
> + * In the future, more performant virtio-gpu 2D UI integration may be
> added.
> + */
> +if (vr->wsi) {
> +if (g_str_equal(vr->wsi, "surfaceless")) {
> +vr->headless = false;
> +} else if (g_str_equal(vr->wsi, "headless")) {
> +vr->headless = true;
> +} else {
> +error_setg(errp, "invalid wsi option selected");
> +return -EINVAL;
> +}
> +}
> +
> +result = rutabaga_calculate_capset_mask(vr->capset_names, _mask);
> +if (result) {
> +error_setg(errp, "invalid capset names: %s", vr->capset_names);
> +return result;
> +}
> +
> +builder.fence_cb = virtio_gpu_rutabaga_fence_cb;
> +builder.debug_cb = virtio_gpu_rutabaga_debug_cb;
> +builder.capset_mask = capset_mask;
> +
> +/*
> + * Using GPOINTER_TO_UINT(g) below causes segfaults.
> + */
> +builder.user_data = (uint64_t)(uintptr_t *)(void *)g;
> +
> +if (vr->wayland_socket_path) {
> +if ((builder.capset_mask & (1 << RUTABAGA_CAPSET_CROSS_DOMAIN)) ==
> 0) {
> +error_setg(errp, "cross-domain required with wayland socket");
> +return -EINVAL;
> +}
> +
> +channels.channels = g_new0(struct rutabaga_channel, 1);
> +channels.num_channels = 1;
> +channels.channels[0].channel_name = vr->wayland_socket_path;
> +channels.channels[0].channel_type = RUTABAGA_CHANNEL_TYPE_WAYLAND;
> +builder.channels =
> +}
> +
Would it be feasible to identify whether Wayland should be used in some
other way, to avoid users having to manually specify the socket path and
allow the standard wl_display_connect() logic to be used?
> +result = rutabaga_init(, >rutabaga);
> +if (builder.capset_mask & (1 << RUTABAGA_CAPSET_CROSS_DOMAIN)) {
> +g_free(channels.channels);
> +}
> +
> +return result;
> +}
signature.asc
Description: PGP signature
Re: [PATCH v2 9/9] docs/system: add basic virtio-gpu documentation
Gurchetan Singh writes: > +virtio-gpu rutabaga > +--- > + > +virtio-gpu can also leverage `rutabaga_gfx`_ to provide `gfxstream`_ > rendering > +and `Wayland display passthrough`_. With the gfxstream rendering mode, GLES > +and Vulkan calls are forwarded directly to the host with minimal > modification. > + > +The crosvm book provides directions on how to build a `gfxstream-enabled > +rutabaga`_ and launch a `guest Wayland compositor`_. > + > +This device does require host blob support (``hostmem`` field below), but not > +all capsets (``capset_names`` below) have to enabled when starting the > device. A more thorough description of what hostmem does, and how to determine what value it should have, would be very welcome. signature.asc Description: PGP signature
Re: [PATCH v2 0/9] gfxstream + rutabaga_gfx
I was able to replicate my existing crosvm cross-domain testing setup using QEMU, and all worked. I didn't test any other capsets. Tested-by: Alyssa Ross signature.asc Description: PGP signature
Re: [PATCH v1 0/9] gfxstream + rutabaga_gfx
Gurchetan Singh writes: > In terms of API stability/versioning/packaging, once this series is > reviewed, the plan is to cut a "gfxstream upstream release branch". We > will have the same API guarantees as any other QEMU project then, i.e no > breaking API changes for 5 years. What about Rutabaga? signature.asc Description: PGP signature
Re: [PATCH] docs/devel: remove incorrect claim about git send-email
Linus Heckemann writes: > Alyssa Ross writes: > >> Alyssa Ross writes: >> >>> Linus Heckemann writes: >>> >>>> While it's unclear to me what git send-email actually does with the >>>> -v2 parameter (it is not documented, but also not rejected), it does >>>> not add a v2 tag to the email's subject, which is what led to the >>>> mishap in [1]. >>>> >>>> [1]: https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg00679.html >>> >>> It does for me! >>> >>> Tested with: >>> >>>git send-email -v2 --to h...@alyssa.is HEAD~ >>> >>> X-Mailer: git-send-email 2.37.1 >> >> I wouldn't be surprised if it only adds it when it's generating the >> patch though. Did you perhaps run git format-patch first to generate a >> patch file, and then use git send-email to send it? > > Yes! I didn't realise that git send-email can be used without the > intermediate format-patch step. I guess it's a git bug that git > send-email will silently ignore -v when used with a patch file. I'll > have a look at fixing that. Yeah, that sounds like the best way to go. I think it'll swallow /any/ format-patch options when used that way. Would be nice if it warned. signature.asc Description: PGP signature
Re: [PATCH] docs/devel: remove incorrect claim about git send-email
Alyssa Ross writes: > Linus Heckemann writes: > >> While it's unclear to me what git send-email actually does with the >> -v2 parameter (it is not documented, but also not rejected), it does >> not add a v2 tag to the email's subject, which is what led to the >> mishap in [1]. >> >> [1]: https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg00679.html > > It does for me! > > Tested with: > >git send-email -v2 --to h...@alyssa.is HEAD~ > > X-Mailer: git-send-email 2.37.1 I wouldn't be surprised if it only adds it when it's generating the patch though. Did you perhaps run git format-patch first to generate a patch file, and then use git send-email to send it? signature.asc Description: PGP signature
Re: [PATCH] docs/devel: remove incorrect claim about git send-email
Linus Heckemann writes: > While it's unclear to me what git send-email actually does with the > -v2 parameter (it is not documented, but also not rejected), it does > not add a v2 tag to the email's subject, which is what led to the > mishap in [1]. > > [1]: https://lists.nongnu.org/archive/html/qemu-devel/2022-09/msg00679.html It does for me! Tested with: git send-email -v2 --to h...@alyssa.is HEAD~ X-Mailer: git-send-email 2.37.1 signature.asc Description: PGP signature
[PATCH] docs: clarify absence of set_features in vhost-user
The previous wording was (at least to me) ambiguous about whether a backend should enable features immediately after they were set using VHOST_USER_SET_PROTOCOL_FEATURES, or wait for support for protocol features to be acknowledged if it hasn't been yet before enabling those features. This patch attempts to make it clearer that VHOST_USER_SET_PROTOCOL_FEATURES should immediately enable features, even if support for protocol features has not yet been acknowledged, while still also making clear that the frontend SHOULD acknowledge support for protocol features. Previous discussion begins here: <https://lore.kernel.org/qemu-devel/87sgd1ktx9@alyssa.is/> Cc: Michael S. Tsirkin Signed-off-by: Alyssa Ross --- docs/interop/vhost-user.rst | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 3f18ab424e..c8b9771a16 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -906,9 +906,9 @@ Front-end message types ``VHOST_USER_SET_FEATURES``. .. Note:: - Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must - support this message even before ``VHOST_USER_SET_FEATURES`` was - called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + back-end must allow ``VHOST_USER_GET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. ``VHOST_USER_SET_PROTOCOL_FEATURES`` :id: 16 @@ -923,8 +923,12 @@ Front-end message types ``VHOST_USER_SET_FEATURES``. .. Note:: - Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must support - this message even before ``VHOST_USER_SET_FEATURES`` was called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + back-end must allow ``VHOST_USER_SET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. + The back-end must not wait for ``VHOST_USER_SET_FEATURES`` before + enabling protocol features requested with + ``VHOST_USER_SET_PROTOCOL_FEATURES``. ``VHOST_USER_SET_OWNER`` :id: 3 base-commit: e93ded1bf6c94ab95015b33e188bc8b0b0c32670 -- 2.37.1
[PATCH] meson: fix logic for gnutls check
The logic before was
if not get_option('gnutls').auto() or have_system
Which is equivalent to
if get_option('gnutls').enabled() or get_option('gnutls').disabled() or
have_system
This means that the check for gnutls is performed even if gnutls is
disabled, which means that the build system will insist on having
libtasn1 if gnutls is found, even if gnutls support is disabled.
When gnutls is disabled, the check for gnutls shouldn't be performed,
to ensure that further build system logic (like the check for
libtasn1) doesn't make decisions based on the presence of gnutls,
rather than the gnutls option.
After making this change, I can successfully ./configure --disable-gnutls
on my system with gnutls installed, but not libtasn1.
Signed-off-by: Alyssa Ross
---
meson.build | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index af9bbb83db..b3e7ec0e92 100644
--- a/meson.build
+++ b/meson.build
@@ -824,7 +824,7 @@ endif
gnutls = not_found
gnutls_crypto = not_found
-if not get_option('gnutls').auto() or have_system
+if get_option('gnutls').enabled() or (get_option('gnutls').auto() and
have_system)
# For general TLS support our min gnutls matches
# that implied by our platform support matrix
#
--
2.32.0
[PATCH] vhost-user: add missing space in error message
This would previously give error messages like
> Received unexpected msg type.Expected 0 received 1
Signed-off-by: Alyssa Ross
---
hw/virtio/vhost-user.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 29ea2b4fce..ad9fc40271 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -429,7 +429,7 @@ static int process_message_reply(struct vhost_dev *dev,
}
if (msg_reply.hdr.request != msg->hdr.request) {
-error_report("Received unexpected msg type."
+error_report("Received unexpected msg type. "
"Expected %d received %d",
msg->hdr.request, msg_reply.hdr.request);
return -1;
base-commit: bccabb3a5d60182645c7749e89f21a9ff307a9eb
--
2.32.0
[PATCH RESEND] docs: clarify absence of set_features in vhost-user
The previous wording was (at least to me) ambiguous about whether a backend should enable features immediately after they were set using VHOST_USER_SET_PROTOCOL_FEATURES, or wait for support for protocol features to be acknowledged if it hasn't been yet before enabling those features. This patch attempts to make it clearer that VHOST_USER_SET_PROTOCOL_FEATURES should immediately enable features, even if support for protocol features has not yet been acknowledged, while still also making clear that the frontend SHOULD acknowledge support for protocol features. Previous discussion begins here: <https://lore.kernel.org/qemu-devel/87sgd1ktx9@alyssa.is/> Cc: Michael S. Tsirkin Signed-off-by: Alyssa Ross --- docs/interop/vhost-user.rst | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index d6085f7045..c42150331d 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -871,9 +871,9 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must - support this message even before ``VHOST_USER_SET_FEATURES`` was - called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + backend must allow ``VHOST_USER_GET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. ``VHOST_USER_SET_PROTOCOL_FEATURES`` :id: 16 @@ -886,8 +886,12 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support - this message even before ``VHOST_USER_SET_FEATURES`` was called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + backend must allow ``VHOST_USER_SET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. + The backend must not wait for ``VHOST_USER_SET_FEATURES`` before + enabling protocol features requested with + ``VHOST_USER_SET_PROTOCOL_FEATURES``. ``VHOST_USER_SET_OWNER`` :id: 3 -- 2.30.0
[PATCH] docs: clarify absence of set_features in vhost-user
The previous wording was (at least to me) ambiguous about whether a backend should enable features immediately after they were set using VHOST_USER_SET_PROTOCOL_FEATURES, or wait for support for protocol features to be acknowledged if it hasn't been yet before enabling those features. This patch attempts to make it clearer that VHOST_USER_SET_PROTOCOL_FEATURES should immediately enable features, even if support for protocol features has not yet been acknowledged, while still also making clear that the frontend SHOULD acknowledge support for protocol features. Previous discussion begins here: <https://lore.kernel.org/qemu-devel/87sgd1ktx9@alyssa.is/> Cc: Michael S. Tsirkin Signed-off-by: Alyssa Ross --- docs/interop/vhost-user.rst | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 10e3e3475e..bc78c9947f 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -854,9 +854,9 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must - support this message even before ``VHOST_USER_SET_FEATURES`` was - called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + backend must allow ``VHOST_USER_GET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. ``VHOST_USER_SET_PROTOCOL_FEATURES`` :id: 16 @@ -869,8 +869,12 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support - this message even before ``VHOST_USER_SET_FEATURES`` was called. + While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a + backend must allow ``VHOST_USER_SET_PROTOCOL_FEATURES`` even if + ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet. + The backend must not wait for ``VHOST_USER_SET_FEATURES`` before + enabling protocol features requested with + ``VHOST_USER_SET_PROTOCOL_FEATURES``. ``VHOST_USER_SET_OWNER`` :id: 3 -- 2.27.0
Re: vhost-user protocol feature negotiation
"Michael S. Tsirkin" writes: > On Thu, Aug 06, 2020 at 08:59:09AM +, Alyssa Ross wrote: >> "Michael S. Tsirkin" writes: >> >> > On Wed, Aug 05, 2020 at 03:13:06PM +, Alyssa Ross wrote: >> >> Quoting from the definition of VHOST_USER_SET_PROTOCOL_FEATURES in >> >> vhost-user.rst: >> >> >> >> > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is >> >> > present in >> >> > ``VHOST_USER_GET_FEATURES``. >> >> > >> >> > .. Note:: >> >> >Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support >> >> >this message even before ``VHOST_USER_SET_FEATURES`` was called. >> >> >> >> To me, this could mean either of two things: >> >> >> >> (1) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving >> >> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should enable the >> >> protocol features immediately. >> >> >> >> (2) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving >> >> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should store those >> >> feature bits, but not actually consider them to be enabled until >> >> after VHOST_USER_SET_FEATURES has been received (presumably >> >> containing VHOST_USER_F_PROTOCOL_FEATURES). >> >> >> >> The reason I bring this up is that QEMU appears to interpret it as (1), >> >> while the vhost-user-net backend in Intel's cloud-hypervisor[1] >> >> interprets it as (2). So I'm looking for a clarification. >> >> >> >> [1]: https://github.com/cloud-hypervisor/cloud-hypervisor >> >> >> >> Thanks in advance. >> > >> > >> > IMHO the intent was this: VHOST_USER_F_PROTOCOL_FEATURES bit in >> > VHOST_USER_GET_FEATURES means that qemu can send >> > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES. >> > >> > With most feature bits in VHOST_USER_GET_FEATURES, the >> > specific functionality needs to only be enabled after >> > VHOST_USER_SET_FEATURES. >> > >> > However, this is for functionality dealing with guest activity. >> > VHOST_USER_SET_PROTOCOL_FEATURES has nothing to do with guest directly, >> > it's about negotiation between qemu and backend: it is only in >> > VHOST_USER_GET_FEATURES for the reason that this is the only message >> > (very) old backends reported. Thus, the backend should not check >> > whether VHOST_USER_SET_FEATURES sets VHOST_USER_F_PROTOCOL_FEATURES, >> > instead it should simply always be ready to receive >> > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES. >> > >> > Backend that isn't always ready to handle >> > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES >> > should not set VHOST_USER_F_PROTOCOL_FEATURES in >> > VHOST_USER_GET_FEATURES. >> >> Thanks for the explanation. That matches what I had in mind with (1). >> >> > This appears to be closer to (1), but if qemu can't distinguish >> > then we don't care, right? For example, VHOST_USER_PROTOCOL_F_REPLY_ACK >> > enables acks on arbitrary messages. Does the backend in question >> > ignore the affected bit until SET_FEATURES? If yes won't this >> > make qemu hang? >> >> Yes. That was my motivation for asking what the correct behaviour was, >> so that I could fix the incorrect one. :) I suspect that up to this point, >> the cloud-hypervisor vhost-user-net backend has only been used with >> cloud-hypervisor, and so this incompatibilty with QEMU was not noticed. >> >> > How would you suggest clarifying the wording? >> >> Do you think this communicates everything required? >> >> --- >> diff --git i/docs/interop/vhost-user.rst w/docs/interop/vhost-user.rst >> index 10e3e3475e..72724d292a 100644 >> --- i/docs/interop/vhost-user.rst >> +++ w/docs/interop/vhost-user.rst >> @@ -854,9 +854,8 @@ Master message types >>``VHOST_USER_GET_FEATURES``. >> >> .. Note:: >> - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must >> - support this message even before ``VHOST_USER_SET_FEATURES`` was >> - called. >> + ``VHOST_USER_F_PROTOCOL_FEATURES`` does not need to be acknowledged >> + with ``VHOST_USER_SET_FEATURES``. >> >> ``VHOST_USER_SET_PROTOCOL_FEATURES`` >>:id: 16 > > Hmm
Re: vhost-user protocol feature negotiation
"Michael S. Tsirkin" writes: > On Wed, Aug 05, 2020 at 03:13:06PM +, Alyssa Ross wrote: >> Quoting from the definition of VHOST_USER_SET_PROTOCOL_FEATURES in >> vhost-user.rst: >> >> > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present >> > in >> > ``VHOST_USER_GET_FEATURES``. >> > >> > .. Note:: >> >Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support >> >this message even before ``VHOST_USER_SET_FEATURES`` was called. >> >> To me, this could mean either of two things: >> >> (1) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving >> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should enable the >> protocol features immediately. >> >> (2) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving >> VHOST_USER_SET_PROTOCOL_FEATURES, a backend should store those >> feature bits, but not actually consider them to be enabled until >> after VHOST_USER_SET_FEATURES has been received (presumably >> containing VHOST_USER_F_PROTOCOL_FEATURES). >> >> The reason I bring this up is that QEMU appears to interpret it as (1), >> while the vhost-user-net backend in Intel's cloud-hypervisor[1] >> interprets it as (2). So I'm looking for a clarification. >> >> [1]: https://github.com/cloud-hypervisor/cloud-hypervisor >> >> Thanks in advance. > > > IMHO the intent was this: VHOST_USER_F_PROTOCOL_FEATURES bit in > VHOST_USER_GET_FEATURES means that qemu can send > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES. > > With most feature bits in VHOST_USER_GET_FEATURES, the > specific functionality needs to only be enabled after > VHOST_USER_SET_FEATURES. > > However, this is for functionality dealing with guest activity. > VHOST_USER_SET_PROTOCOL_FEATURES has nothing to do with guest directly, > it's about negotiation between qemu and backend: it is only in > VHOST_USER_GET_FEATURES for the reason that this is the only message > (very) old backends reported. Thus, the backend should not check > whether VHOST_USER_SET_FEATURES sets VHOST_USER_F_PROTOCOL_FEATURES, > instead it should simply always be ready to receive > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES. > > Backend that isn't always ready to handle > VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES > should not set VHOST_USER_F_PROTOCOL_FEATURES in > VHOST_USER_GET_FEATURES. Thanks for the explanation. That matches what I had in mind with (1). > This appears to be closer to (1), but if qemu can't distinguish > then we don't care, right? For example, VHOST_USER_PROTOCOL_F_REPLY_ACK > enables acks on arbitrary messages. Does the backend in question > ignore the affected bit until SET_FEATURES? If yes won't this > make qemu hang? Yes. That was my motivation for asking what the correct behaviour was, so that I could fix the incorrect one. :) I suspect that up to this point, the cloud-hypervisor vhost-user-net backend has only been used with cloud-hypervisor, and so this incompatibilty with QEMU was not noticed. > How would you suggest clarifying the wording? Do you think this communicates everything required? --- diff --git i/docs/interop/vhost-user.rst w/docs/interop/vhost-user.rst index 10e3e3475e..72724d292a 100644 --- i/docs/interop/vhost-user.rst +++ w/docs/interop/vhost-user.rst @@ -854,9 +854,8 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must - support this message even before ``VHOST_USER_SET_FEATURES`` was - called. + ``VHOST_USER_F_PROTOCOL_FEATURES`` does not need to be acknowledged + with ``VHOST_USER_SET_FEATURES``. ``VHOST_USER_SET_PROTOCOL_FEATURES`` :id: 16 @@ -869,8 +868,8 @@ Master message types ``VHOST_USER_GET_FEATURES``. .. Note:: - Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support - this message even before ``VHOST_USER_SET_FEATURES`` was called. + ``VHOST_USER_F_PROTOCOL_FEATURES`` does not need to be acknowledged + with ``VHOST_USER_SET_FEATURES``. ``VHOST_USER_SET_OWNER`` :id: 3
vhost-user protocol feature negotiation
Quoting from the definition of VHOST_USER_SET_PROTOCOL_FEATURES in vhost-user.rst: > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > ``VHOST_USER_GET_FEATURES``. > > .. Note:: >Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support >this message even before ``VHOST_USER_SET_FEATURES`` was called. To me, this could mean either of two things: (1) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving VHOST_USER_SET_PROTOCOL_FEATURES, a backend should enable the protocol features immediately. (2) If VHOST_USER_F_PROTOCOL_FEATURES hasn't been set, upon receiving VHOST_USER_SET_PROTOCOL_FEATURES, a backend should store those feature bits, but not actually consider them to be enabled until after VHOST_USER_SET_FEATURES has been received (presumably containing VHOST_USER_F_PROTOCOL_FEATURES). The reason I bring this up is that QEMU appears to interpret it as (1), while the vhost-user-net backend in Intel's cloud-hypervisor[1] interprets it as (2). So I'm looking for a clarification. [1]: https://github.com/cloud-hypervisor/cloud-hypervisor Thanks in advance.
Re: Testing the virtio-vhost-user QEMU patch
Stefan Hajnoczi writes: > On Fri, Jul 24, 2020 at 10:58:45AM +0000, Alyssa Ross wrote: >> Alyssa Ross writes: >> >> > Stefan Hajnoczi writes: >> > >> >> On Tue, Jul 21, 2020 at 07:14:38AM +, Alyssa Ross wrote: >> >>> Hi -- I hope it's okay me reaching out like this. >> >>> >> >>> I've been trying to test out the virtio-vhost-user implementation that's >> >>> been posted to this list a couple of times, but have been unable to get >> >>> it to boot a kernel following the steps listed either on >> >>> <https://wiki.qemu.org/Features/VirtioVhostUser> or >> >>> <https://ndragazis.github.io/dpdk-vhost-vvu-demo.html>. >> >>> >> >>> Specifically, the kernel appears to be unable to write to the >> >>> virtio-vhost-user device's PCI registers. I've included the full panic >> >>> output from the kernel at the end of this message. The panic is >> >>> reproducible with two different kernels I tried (with different configs >> >>> and versions). I tried both versions of the virtio-vhost-user I was >> >>> able to find[1][2], and both exhibited the same behaviour. >> >>> >> >>> Is this a known issue? Am I doing something wrong? >> >> >> >> Hi, >> >> Unfortunately I'm not sure what the issue is. This is an early >> >> virtio-pci register access before a driver for any specific device type >> >> (net, blk, vhost-user, etc) comes into play. >> > >> > Small update here: I tried on another computer, and it worked. Made >> > sure that it was exactly the same QEMU binary, command line, and VM >> > disk/initrd/kernel, so I think I can fairly confidently say the panic >> > depends on what hardware QEMU is running on. I set -cpu value to the >> > same on both as well (SandyBridge). >> > >> > I also discovered that it works on my primary computer (the one it >> > panicked on before) with KVM disabled. >> > >> > Note that I've only got so far as finding that it boots on the other >> > machine -- I haven't verified yet that it actually works. >> > >> > Bad host CPU: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz >> > Good host CPU: AMD EPYC 7401P 24-Core Processor >> > >> > May I ask what host CPUs other people have tested this on? Having more >> > data would probably be useful. Could it be an AMD vs. Intel thing? >> >> I think I've figured it out! >> >> Sandy Bridge and Ivy Bridge hosts encounter this panic because the >> "additional resources" bar size is too big, at 1 << 36. If I change >> this to 1 << 35, no more kernel panic. >> >> Skylake and later are fine with 1 << 36. In between Ivy Bridge and >> Skylake were Haswell and Broadwell, but I couldn't find anybody who was >> able to help me test on either of those, so I don't know what they do. >> >> Perhaps related, the hosts that produce panics all seem to have a >> physical address size of 36 bits, while the hosts that work have larger >> physical address sizes, as reported by lscpu. > > I have run it successfully on Broadwell but never tried 64GB or larger > shared memory resources. To clarify, I haven't been using big shared memory resources either -- this has all been about getting the backend VM to start at all. The panic happens at boot, and the 1 << 36 BAR allocation comes from here, during realization: https://github.com/ndragazis/qemu/blob/f9ab08c0c8/hw/virtio/virtio-vhost-user-pci.c#L291
Re: Testing the virtio-vhost-user QEMU patch
Alyssa Ross writes: > Stefan Hajnoczi writes: > >> On Tue, Jul 21, 2020 at 07:14:38AM +0000, Alyssa Ross wrote: >>> Hi -- I hope it's okay me reaching out like this. >>> >>> I've been trying to test out the virtio-vhost-user implementation that's >>> been posted to this list a couple of times, but have been unable to get >>> it to boot a kernel following the steps listed either on >>> <https://wiki.qemu.org/Features/VirtioVhostUser> or >>> <https://ndragazis.github.io/dpdk-vhost-vvu-demo.html>. >>> >>> Specifically, the kernel appears to be unable to write to the >>> virtio-vhost-user device's PCI registers. I've included the full panic >>> output from the kernel at the end of this message. The panic is >>> reproducible with two different kernels I tried (with different configs >>> and versions). I tried both versions of the virtio-vhost-user I was >>> able to find[1][2], and both exhibited the same behaviour. >>> >>> Is this a known issue? Am I doing something wrong? >> >> Hi, >> Unfortunately I'm not sure what the issue is. This is an early >> virtio-pci register access before a driver for any specific device type >> (net, blk, vhost-user, etc) comes into play. > > Small update here: I tried on another computer, and it worked. Made > sure that it was exactly the same QEMU binary, command line, and VM > disk/initrd/kernel, so I think I can fairly confidently say the panic > depends on what hardware QEMU is running on. I set -cpu value to the > same on both as well (SandyBridge). > > I also discovered that it works on my primary computer (the one it > panicked on before) with KVM disabled. > > Note that I've only got so far as finding that it boots on the other > machine -- I haven't verified yet that it actually works. > > Bad host CPU: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz > Good host CPU: AMD EPYC 7401P 24-Core Processor > > May I ask what host CPUs other people have tested this on? Having more > data would probably be useful. Could it be an AMD vs. Intel thing? I think I've figured it out! Sandy Bridge and Ivy Bridge hosts encounter this panic because the "additional resources" bar size is too big, at 1 << 36. If I change this to 1 << 35, no more kernel panic. Skylake and later are fine with 1 << 36. In between Ivy Bridge and Skylake were Haswell and Broadwell, but I couldn't find anybody who was able to help me test on either of those, so I don't know what they do. Perhaps related, the hosts that produce panics all seem to have a physical address size of 36 bits, while the hosts that work have larger physical address sizes, as reported by lscpu.
Re: Testing the virtio-vhost-user QEMU patch
Stefan Hajnoczi writes: > On Tue, Jul 21, 2020 at 07:14:38AM +0000, Alyssa Ross wrote: >> Hi -- I hope it's okay me reaching out like this. >> >> I've been trying to test out the virtio-vhost-user implementation that's >> been posted to this list a couple of times, but have been unable to get >> it to boot a kernel following the steps listed either on >> <https://wiki.qemu.org/Features/VirtioVhostUser> or >> <https://ndragazis.github.io/dpdk-vhost-vvu-demo.html>. >> >> Specifically, the kernel appears to be unable to write to the >> virtio-vhost-user device's PCI registers. I've included the full panic >> output from the kernel at the end of this message. The panic is >> reproducible with two different kernels I tried (with different configs >> and versions). I tried both versions of the virtio-vhost-user I was >> able to find[1][2], and both exhibited the same behaviour. >> >> Is this a known issue? Am I doing something wrong? > > Hi, > Unfortunately I'm not sure what the issue is. This is an early > virtio-pci register access before a driver for any specific device type > (net, blk, vhost-user, etc) comes into play. Small update here: I tried on another computer, and it worked. Made sure that it was exactly the same QEMU binary, command line, and VM disk/initrd/kernel, so I think I can fairly confidently say the panic depends on what hardware QEMU is running on. I set -cpu value to the same on both as well (SandyBridge). I also discovered that it works on my primary computer (the one it panicked on before) with KVM disabled. Note that I've only got so far as finding that it boots on the other machine -- I haven't verified yet that it actually works. Bad host CPU: Intel(R) Core(TM) i5-2520M CPU @ 2.50GHz Good host CPU: AMD EPYC 7401P 24-Core Processor May I ask what host CPUs other people have tested this on? Having more data would probably be useful. Could it be an AMD vs. Intel thing?
Re: Testing the virtio-vhost-user QEMU patch
Stefan Hajnoczi writes: > On Tue, Jul 21, 2020 at 07:14:38AM +0000, Alyssa Ross wrote: >> Hi -- I hope it's okay me reaching out like this. >> >> I've been trying to test out the virtio-vhost-user implementation that's >> been posted to this list a couple of times, but have been unable to get >> it to boot a kernel following the steps listed either on >> <https://wiki.qemu.org/Features/VirtioVhostUser> or >> <https://ndragazis.github.io/dpdk-vhost-vvu-demo.html>. >> >> Specifically, the kernel appears to be unable to write to the >> virtio-vhost-user device's PCI registers. I've included the full panic >> output from the kernel at the end of this message. The panic is >> reproducible with two different kernels I tried (with different configs >> and versions). I tried both versions of the virtio-vhost-user I was >> able to find[1][2], and both exhibited the same behaviour. >> >> Is this a known issue? Am I doing something wrong? > > Hi, > Unfortunately I'm not sure what the issue is. This is an early > virtio-pci register access before a driver for any specific device type > (net, blk, vhost-user, etc) comes into play. > > Did you test the git trees linked below or did you rebase the commits > on top of your own QEMU tree? I tested the git trees. For your one I had to make a slight modification to delete the memfd syscall wrapper in util/memfd.c, since it conflicted with the one that is now provided by Glibc. Nikos's tree I used totally unmodified. > Is your guest kernel a stock kernel.org/distro kernel or has it been > modified (especially with security patches)? I tried a slightly modified Chromium OS kernel (5.4.23), and a stock Ubuntu 18.10 kernel (4.15.0). I think the most "normal" setup I tried was building QEMU on Fedora 32, and then attempting to boot a freshly installed Ubuntu Server 18.10 VM with -chardev socket,id=chardev0,path=vhost-user.sock,server,nowait \ -device virtio-vhost-user-pci,chardev=chardev0 (The crash was reproducible with the full QEMU command lines in the write-ups, but these seemed to be the load-bearing bits.) > If no one else knows what is wrong here then it will be necessary to > check the Intel manuals to figure out the exact meaning of > "error_code(0x000b) - reserved bit violation" and why Linux triggers it > with "PGD 3b128067 P4D 3b128067 PUD 3b129067 PMD 3b12a067 PTE > 80200073". Thanks for your insight. Now I at least have a place to start if nobody else knows what's up. :)
Testing the virtio-vhost-user QEMU patch
Hi -- I hope it's okay me reaching out like this. I've been trying to test out the virtio-vhost-user implementation that's been posted to this list a couple of times, but have been unable to get it to boot a kernel following the steps listed either on <https://wiki.qemu.org/Features/VirtioVhostUser> or <https://ndragazis.github.io/dpdk-vhost-vvu-demo.html>. Specifically, the kernel appears to be unable to write to the virtio-vhost-user device's PCI registers. I've included the full panic output from the kernel at the end of this message. The panic is reproducible with two different kernels I tried (with different configs and versions). I tried both versions of the virtio-vhost-user I was able to find[1][2], and both exhibited the same behaviour. Is this a known issue? Am I doing something wrong? Thanks in advance -- I'm excitedly following the progress of this feature. Alyssa Ross [1]: https://github.com/ndragazis/qemu/commits/virtio-vhost-user [2]: https://github.com/stefanha/qemu/commits/virtio-vhost-user [1.287979] BUG: unable to handle page fault for address: b8ca40025014 [1.288311] #PF: supervisor write access in kernel mode [1.288311] #PF: error_code(0x000b) - reserved bit violation [1.288311] PGD 3b128067 P4D 3b128067 PUD 3b129067 PMD 3b12a067 PTE 80200073 [1.288311] Oops: 000b [#1] SMP PTI [1.288311] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.4.28 #1-NixOS [1.288311] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014 [1.288311] RIP: 0010:iowrite8+0xe/0x30 [1.288311] Code: fe ff ff 48 c7 c0 ff ff ff ff c3 48 8b 3f 48 89 f8 c3 66 2e 0f 1f 84 00 00 00 00 00 89 f8 48 89 f7 48 81 fe ff ff 3 [1.288311] RSP: :b8ca40013cd8 EFLAGS: 00010292 [1.288311] RAX: RBX: b8ca40013d60 RCX: [1.288311] RDX: 002f RSI: b8ca40025014 RDI: b8ca40025014 [1.288311] RBP: 9c742ea20400 R08: 9c742f0a60af R09: [1.288311] R10: 0018 R11: 9c742f0a60af R12: [1.288311] R13: 9c742ea20410 R14: R15: [1.288311] FS: () GS:9c743b70() knlGS: [1.288311] CS: 0010 DS: ES: CR0: 80050033 [1.288311] CR2: b8ca40025014 CR3: 37a0a001 CR4: 00060ee0 [1.288311] Call Trace: [1.288311] vp_reset+0x1b/0x50 [1.288311] register_virtio_device+0x74/0xe0 [1.288311] virtio_pci_probe+0xaf/0x140 [1.288311] local_pci_probe+0x42/0x80 [1.288311] pci_device_probe+0x104/0x1b0 [1.288311] really_probe+0x147/0x3c0 [1.288311] driver_probe_device+0xb6/0x100 [1.288311] device_driver_attach+0x53/0x60 [1.288311] __driver_attach+0x8a/0x150 [1.288311] ? device_driver_attach+0x60/0x60 [1.288311] bus_for_each_dev+0x78/0xc0 [1.288311] bus_add_driver+0x14d/0x1f0 [1.288311] driver_register+0x6c/0xc0 [1.288311] ? dma_bus_init+0xbf/0xbf [1.288311] do_one_initcall+0x46/0x1f4 [1.288311] kernel_init_freeable+0x176/0x200 [1.288311] ? rest_init+0xab/0xab [1.288311] kernel_init+0xa/0x105 [1.288311] ret_from_fork+0x35/0x40 [1.288311] Modules linked in: [1.288311] CR2: b8ca40025014 [1.288311] ---[ end trace 5164b2fa531e028f ]--- [1.288311] RIP: 0010:iowrite8+0xe/0x30 [1.288311] Code: fe ff ff 48 c7 c0 ff ff ff ff c3 48 8b 3f 48 89 f8 c3 66 2e 0f 1f 84 00 00 00 00 00 89 f8 48 89 f7 48 81 fe ff ff 3 [1.288311] RSP: :b8ca40013cd8 EFLAGS: 00010292 [1.288311] RAX: RBX: b8ca40013d60 RCX: [1.288311] RDX: 002f RSI: b8ca40025014 RDI: b8ca40025014 [1.288311] RBP: 9c742ea20400 R08: 9c742f0a60af R09: [1.288311] R10: 0018 R11: 9c742f0a60af R12: [1.288311] R13: 9c742ea20410 R14: R15: [1.288311] FS: () GS:9c743b70() knlGS: [1.288311] CS: 0010 DS: ES: CR0: 80050033 [1.288311] CR2: b8ca40025014 CR3: 37a0a001 CR4: 00060ee0 [1.288311] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0009 [1.288311] Kernel Offset: 0x2120 from 0x8100 (relocation range: 0x8000-0xbfff) [1.288311] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0009 ]---
