Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded Xen-tools from 4.15 to 4.19.0 #4133

Closed
wants to merge 1 commit into from

Conversation

roja-zededa
Copy link
Contributor

@roja-zededa roja-zededa commented Aug 6, 2024

WARNING: STALE PR. Please move to #4186

Due to failing Request Code Owners Review / auto_request_review (pull_request_target) moved this PR to #4186

Porting Xen-tools from 4.15 to 4.19.0 (QEMU Version 8.0.4).

Changes

  • Able to add ninja through apk in Xen-tools/Docker and xen/Docker
  • Changed the seabios and Xen version to 1.16.3 and 4.19.0-rc4 respectively
  • QEMU cpu pinning implementation is available with different signature, so removed nikholay's patch from 15-qemu-Set-the-affinity-of-QEMU-threads-according-to-t.patch
  • Warning treated as error flag is available by default so removed 12-disable-Werror-to-build-under-gcc-11.2.patch
  • NetdevTapOptions doesn't have has_br member so changed 10-bridge-helper-support.patch
  • Bydefault vhost-vsock and vhost-scsi is not available added a new patch file to enable them: 17-enable-qemu-vhost-vsock.patch
  • 08-Revert__Revert__vfio_pci-quirks_c__Disable_stolen_memory_for_igd_VFIO__.patch looked super messy, so cleaned it.

Apart from the above major changes, the index of other existing patches might have been changed.

@roja-zededa roja-zededa marked this pull request as draft August 6, 2024 19:13
@OhmSpectator
Copy link
Member

make pkg/xen-tools

fails with

Error: error building "lfedge/eve-xen-tools:d221f4cc6f2188780696466c15e49f0b4d4e883d": error building for arch amd64: failed to solve: failed to compute cache key: failed to calculate checksum of ref 7o96c6fay2hhmqjhly179hk55::n43xas19zec79oz9huevgv2va: "/patches-4.19.0-rc4": not found

@roja-zededa
Copy link
Contributor Author

roja-zededa commented Aug 6, 2024

@OhmSpectator Please rename the patches folder from pkg/xen-tools/patches-4.19.0-rc4-v3 to patches-4.19.0-rc4 . It should work now, I just made changed in my master branch.

@roja-zededa roja-zededa force-pushed the master branch 2 times, most recently from 888892e to e1d536d Compare August 6, 2024 22:47
+
if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
- !vfio_is_vga(vdev) || nr != 4 ||
- &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering what was the original reason of this patch and if we still need it.... @rucoder , do you know if we still need to revert these changes from Xen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rene As of now, I don't see any issue with my updated revert vfio patch so keeping the updated one.

pkg/xen/Dockerfile Outdated Show resolved Hide resolved
pkg/xen/Dockerfile Outdated Show resolved Hide resolved
@rene
Copy link
Contributor

rene commented Aug 7, 2024

@roja-zededa , just a reminder for the final version, don't forget to let just one Signed-off-by line on each commit and add a proper title + commit message....

@roja-zededa
Copy link
Contributor Author

@rene @OhmSpectator @rucoder Done with the suggested changes, please let me know if I need to change anything else for the approval.

@eriknordmark
Copy link
Contributor

@roja-zededa there are some unit test failures (due to different syntax in the kvm file??), and yetus/blanks complaints in https://github.com/lf-edge/eve/pull/4133/checks

@rucoder
Copy link
Contributor

rucoder commented Aug 8, 2024

@roja-zededa please test it carefully. Especually PCI pass-through part.

@rucoder
Copy link
Contributor

rucoder commented Aug 8, 2024

@roja-zededa and I would wait for stable release and not merge RC4

@roja-zededa
Copy link
Contributor Author

@roja-zededa and I would wait for stable release and not merge RC4

The stable version is out, able to port changes, no issues so far.

@roja-zededa roja-zededa force-pushed the master branch 2 times, most recently from 01ada0d to 82f4966 Compare August 8, 2024 23:00
@roja-zededa
Copy link
Contributor Author

@roja-zededa there are some unit test failures (due to different syntax in the kvm file??), and yetus/blanks complaints in https://github.com/lf-edge/eve/pull/4133/checks

Resolved the yetus/blanks issue. The unit test failure with hypervisor is due to removal of [realtime] parameter, the kvm_test.go file tests need modifications (ie. remove [realtime]).

Still debugging the unit test failures caused due to pci.

@roja-zededa roja-zededa force-pushed the master branch 2 times, most recently from 31fdaea to c365539 Compare August 12, 2024 19:18
@roja-zededa roja-zededa changed the title Upgraded Xen-tools from 4.15 to 4.19.0-rc4 Upgraded Xen-tools from 4.15 to 4.19.0 Aug 12, 2024
@roja-zededa
Copy link
Contributor Author

@OhmSpectator @rene @rucoder Made the necessary changes, please let me know what else I need to do for this pull request to be merged

@christoph-zededa Removed the [realtime] parameter from kvm.go as the new Qemu doesn't support it, the unit tests need to be changed in the future for this PR to be passing all the hypervisor test cases. (As of now I put [realtime] back)

@rouming
Copy link
Contributor

rouming commented Aug 28, 2024

Unit tests are failing. The kvm config tests are hard to deal with, would be good have the reference configs in separate files rather than hundreds of lines of strings embedded in go.

I have asked @rouming about changing the hypervisor.go unit test cases, would be a good idea to have a separate unit test file so I agree.

I apologize I did not recognize the question intonation: so what is the question? Do you need to change tests if they are failing? Answer is: yes, please :)

Copy link
Contributor

@rouming rouming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests fix is required

@OhmSpectator
Copy link
Member

Yey, thanks for the fix in the test! Could you please polish the commit messages as well?

@OhmSpectator
Copy link
Member

@roja-zededa reopen the PR, please =)

@OhmSpectator
Copy link
Member

@roja-zededa, ah... I see... You create a PR from your local master. In this case, part of our GitHub actions may fail... Let's ignore them for now. But the next time, please, create a PR from a dedicated branch, not from your master.

# disable golang as it does not play well together with musl (stderr is defined as FILE* const and fails to compile)
RUN ./configure --prefix=/usr --disable-xen --disable-golang --disable-qemu-traditional --disable-docs --enable-9pfs \
--with-system-ovmf=/usr/lib/xen/boot/ovmf.bin --disable-stubdom
--with-system-ovmf=/usr/lib/xen/boot/ovmf.bin --disable-stubdom \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the extra spaces back, increases the readability. same for EXTRA_QEMUU_CONFIGURE_ARGS.

Able to add ninja through apk in xen-tools/Docker and xen/Docker (1)

Changed the seabios and Xen version to 1.16.3 and 4.19.0 respectively (2)

Added 12-remove-vanillaqemu4.19-cpupinning.patch which removes new qemu_thread_set_affinity implementation (QEMU 8.0.4), also retained
Nikolay CPU Pinning PatchNo.15 (3)

Warning treated as error flag is available by default so removed 12-disable-Werror-to-build-under-gcc-11.2.patch (4)

NetdevTapOptions doesn't have has_br member so changed 10-bridge-helper-support.patch (5)

Bydefault vhost-vsock and vhost-scsi is enabled so removing the corresponding enable flags from xen-tools/Dockerfile (6)

Removed 11-char-socket-revert.patch as it's unnecessary (7)

Removed [realtime] option from kvm.go and replaced it with [overcommit], hypervisor.go unit test need to be changed to reflect [overcommit] (8)

Revert__Revert__vfio_pci-quirks_c__Disable_stolen_memory_for_igd_VFIO__.patch looked super messy, so cleaned it (9)

Replaced [realtime] with [overcommit] in kvm_test.go for the unit test case to pass (10)

Signed-off-by: Roja Eswaran <[email protected]>
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.69%. Comparing base (695652b) to head (032e91b).
Report is 43 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4133   +/-   ##
=======================================
  Coverage   19.69%   19.69%           
=======================================
  Files           8        8           
  Lines        2600     2600           
=======================================
  Hits          512      512           
  Misses       1985     1985           
  Partials      103      103           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants