Code review comment for ~sergiodj/ubuntu/+source/qemu:tcg-crash-mantic

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Some trivial findings first:

- patches are in debian/patches/ubuntu/ but the CL says - d/p/lp... should be - d/p/u/lp...
- some places mention kernel "6.13" while it is "6.3"
- avoid some ambiguities by stating clearly that the kernel >=6.3 is in meant to be the one in the guest, otherwise some might think "so what, 6.3 isn't supported in jammy/mantic anyway" or similar

On the patches:
- lp-2051965-accel-tcg-Split-out-cpu_exec_longjmp_cleanup.patch
  That seems fine, it just moves identical code to a shared function
  The assert should not matter, and on the actual binary this most likely is inlined and thereby as before
- lp-2051965-accel-tcg-Always-lock-pages-before-translation.patch
  Wow, this is more busy.
  Conceptually it is ok - at high abstraction more locking is slower but safer.
  It didn't help that the diff was hard to read by so muhc movement
  But eventually the new code is easier to read with just one kind of locking instead of 4
  I spent some time in a rabbit hole reading this, but it is either ok or too complex for me
  to see the issue
- lp-2051965-accel-tcg-Clear-tcg_ctx-gen_tb-on-buffer-overflow.patch
  That one is easy again, clearing a condition introduced by the former
- I found no other fixups than the one you already spotted

I can see why you said Jammy backport might be more complex, there are probably plenty of changes to that code. My gut says that you'd be ok to backport quite some more changes (instead of only adapting those three patches) if you need it, but that is up to you as you see fit.

+1 on this one with the minor things fixed.

For the "where issues occur" in the SRU statement, the most important bit is that this seems purely in tcg code, so we can assume that HW-virt will not be affected ever.

review: Approve

« Back to merge proposal