cherry-picking something to -stable which might require other changes

[Michael Tokarev]

Hi!

I've an interesting situation here which I'd love to discuss.
Cc'ing authors of commits in question, but this is actually a much more
general topic than this very specific issue, - so also adding some more
addresses to Cc.

I tried to cherry-pick a trivial commit from master to stable-7.2, this one:

commit c255946e3df4d9660e4f468a456633c24393d468
Author: Thomas Huth <thuth@redhat.com>
Date:   Fri Jul 21 11:47:19 2023 +0200

    hw/char/riscv_htif: Fix printing of console characters on big endian hosts

--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -232,7 +232,8 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t 
val_written)
             s->tohost = 0; /* clear to indicate we read */
             return;
         } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
-            qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
+            uint8_t ch = (uint8_t)payload;
+            qemu_chr_fe_write(&s->chr, &ch, 1);
             resp = 0x100 | (uint8_t)payload;
         } else {
             qemu_log("HTIF device %d: unknown command\n", device);

(it's a whole commit).
The change is small, obvious and well-understood, but the patch does not
apply to 7.2.  For it to apply, either hand-editing the patch is necessary,
or other 2 changes are needed, which are (showing just the relevant parts
from much larger commits):

commit 753ae97abc7459e69d48712355118fb54268f8cb
Author: Bin Meng <bmeng@tinylab.org>
Date:   Thu Dec 29 17:18:17 2022 +0800

    hw/char: riscv_htif: Avoid using magic numbers

--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c

+#define HTIF_DEV_CONSOLE        1

             htifstate->env->mtohost = 0; /* clear to indicate we read */
             return;
-        } else if (cmd == 0x1) {
+        } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
             qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
             resp = 0x100 | (uint8_t)payload;
         } else {


and:

commit dadee9e3ce6ee6aad36fe3027eaa0f947358f812
Author: Bin Meng <bmeng@tinylab.org>
Date:   Thu Dec 29 17:18:20 2022 +0800

    hw/char: riscv_htif: Use conventional 's' for HTIFState

--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c

             return;
         } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
-            qemu_chr_fe_write(&htifstate->chr, (uint8_t *)&payload, 1);
+            qemu_chr_fe_write(&s->chr, (uint8_t *)&payload, 1);
             resp = 0x100 | (uint8_t)payload;
         } else {


Both are actually no-ops, - first one defines a bunch of constants, replaces
"magic values" with these constants, and adds comments; second renames 
variables.
Neither of them has any impact on the resulting code, there's no actual code
changes, just the renames and comments.  But after these 2 patches, the patch
in question applies cleanly, and it is much more likely that subsequent patches
in the same file will apply cleanly too.

In this very specific case, I tend to pick the other two patches too, - esp.
having in mind 7.2 is to be maintained for quite a while (if not only for
debian), - so long-term it should be easier.  But at the same time it's tempting
to just back-port the tiny change in question to the older release.

It's the same doubt as I had with reentrancy fixes which landed in 8.1.  When
backporting other changes (in this case ide/ahci fixes), I either had to fix
context, or include the reentrancy fixes before applying that ide/ahci fix.
This one was nice, because I was not sure if I want to include reentrancy fixes
since the change is quite large, but the ability to apply other patches
unedited was really appealing.

Another example is https://gitlab.com/qemu-project/qemu/-/issues/1808 - the
fix needs translator_io_start() which is this:

commit dfd1b81274140c5f511d549f7b3ec7675a6597f4
Author: Richard Henderson <richard.henderson@linaro.org>
Date:   Mon May 22 23:08:01 2023 -0700

    accel/tcg: Introduce translator_io_start

but this commit is definitely problematic. The problem is that it isn't only
introduces this function, but at the same time it changes a lot of code to
use it, and when trying to apply it to older release, many places conflicts.
If it were just translator_io_start introduction as the subject says, with
conversion to this new function follows, things would be much better. But
the way how it is, I either have to introduce this routine in a stable-7.2-
specific patch (taken as a part of this dfd1b81274140 change), or replace
translator_io_start() usage in subsequent changes like the fix for #1808 ,
neither of which are good.

Quite similar situation was with markings of coroutine_fn etc, - it would
be nice if, in case when something will be used in many places, the definition
would come in a separate patch, with usage/conversion coming in the next patch.


It's all examples of various interesting things I'm seeing, - there are more.
Sure it all is a case-by-case basis.

At any rate, I'd love to have some comments about the situation with this 
trivial
console printing fix (personally I tend to include 2 previous "no-op" changes 
even
if both are somewhat large, instead of back-porting just the fix itself), and 
about
general "other" patch back-porting like this.

BTW, dadee9e3 "hw/char: riscv_htif: Use conventional 's' for HTIFState" has an
issue (which were there before but it hasn't been fixed):

-#define TOHOST_OFFSET1 (htifstate->tohost_offset)
-#define TOHOST_OFFSET2 (htifstate->tohost_offset + 4)
+#define TOHOST_OFFSET1      (s->tohost_offset)
+#define TOHOST_OFFSET2      (s->tohost_offset + 4)

 /* CPU wants to read an HTIF register */
 static uint64_t htif_mm_read(void *opaque, hwaddr addr, unsigned size)
 {
-    HTIFState *htifstate = opaque;
+    HTIFState *s = opaque;
     if (addr == TOHOST_OFFSET1) {
-        return htifstate->env->mtohost & 0xFFFFFFFF;
+        return s->env->mtohost & 0xFFFFFFFF;
     } else if (addr == TOHOST_OFFSET2) {
-        return (htifstate->env->mtohost >> 32) & 0xFFFFFFFF;
+        return (s->env->mtohost >> 32) & 0xFFFFFFFF;

these FROMHOST/TOHOST #defines should take an argument (s in this case).
But this is a different issue.

Thanks!

/mjt