Code review comment for ~rafaeldtinoco/ubuntu/+source/qemu:lp1805256-bionic-refix

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

> @rafaeldtinoco: Testing all looks good on arm64/armhf. Can you change the name of the patch in debian/changelog to match the new name of the patch? It is missing the -arm-only suffix. With that, I'm +1. But, if you haven't already, I'd recommend testing w/ unifdef just to prove that the !aarch64 code is exactly what it was before.

I'll ask to @paelzer for a full set of regression tests from the package in the PPA so that will cover what you asked.

> btw, though I understand the reasoning, I'm personally not a fan of the #ifdef approach as that may cause maintenance headaches in upcoming years. If the submitter of bug 1885419 were to be kind enough to verify a non-ifdef'd version in his environment, would that give you enough confidence to ship it? Or is the concern about not-yet-uncovered regressions?

Based on the history: just seen in aarch64, difficult to reproduce (same cacheline, multiple cpus, aarch64), difficult to fix without performance impact (thus the long study from Paolo about using the minimum required mem ordering instructions), and my previous mistake in the SRU attempt.. and I do agree that #ifdef approach isn't great... Christian and I thought it was the safest AND something that would definitely raise the chances for the SRU to be accepted.

> Thanks for continuing to stick with this nasty issue for so long Rafael!

My pleasure! It became a question of honor #). Cheers o/

« Back to merge proposal