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!
/mjtWhen I backport patches into RHEL, the general process I follow is: 1. For context conflicts, just adjust the patch to resolve them. 2. For real dependencies, backport the dependencies, if possible. 3. If backporting the dependencies is not possible, think of a downstream-only solution. This should be rare. People make different backporting decisions (just like structuring patch series). It can be a matter of taste. Stefan
On Tue, Sep 12, 2023 at 10:00:46AM -0400, Stefan Hajnoczi wrote: > When I backport patches into RHEL, the general process I follow is: > 1. For context conflicts, just adjust the patch to resolve them. > 2. For real dependencies, backport the dependencies, if possible. > 3. If backporting the dependencies is not possible, think of a > downstream-only solution. This should be rare. > > People make different backporting decisions (just like structuring > patch series). It can be a matter of taste. I tend to try to cherry-pick the dependancies in case (1) too unless they are functionally invasive. Any time you manually adjust a patch, you increase the likelihood that later cherry picks will also require manual work. So I always favour a clean cherry-pick until the point the functional risk becomes unacceptable in the context of testing the change I'm pulling back. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
12.09.2023 18:23, Daniel P. Berrangé wrote:
..
> I tend to try to cherry-pick the dependancies in case (1) too
> unless they are functionally invasive. Any time you manually
> adjust a patch, you increase the likelihood that later cherry
> picks will also require manual work. So I always favour a clean
> cherry-pick until the point the functional risk becomes
> unacceptable in the context of testing the change I'm pulling
> back.
Yeah, that's exactly my thought: if something in the subsystem
has changed, esp. when the new thing is now widely used, it is
best to try to pick it up (unless it is a big change by itself
or is a part of big change).
I already mentioned a trivial fix c255946e3df4 in this thread,
which can be applied cleanly if two other no-change patches are
in, 753ae97abc7 and dadee9e3ce6. It is much more likely to hit
conflicts in this area in future updates if such updates will
happen if such renames like these two aren't picked up.
So, right in this same patch series, there's one more very similar
change:
commit 9ff31406312500053ecb5f92df01dd9ce52e635d
Author: Conor Dooley <conor.dooley@microchip.com>
Date: Thu Jul 27 15:24:17 2023 +0100
hw/riscv: virt: Fix riscv,pmu DT node path
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -719,7 +719,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
MachineState *ms = MACHINE(s);
RISCVCPU hart = s->soc[0].harts[0];
- pmu_name = g_strdup_printf("/soc/pmu");
+ pmu_name = g_strdup_printf("/pmu");
qemu_fdt_add_subnode(ms->fdt, pmu_name);
qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
But all the nearby lines are touched by previous patch:
commit 568e0614d0979e0431a8d9dc0503a63b8b0f2d81
Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Date: Tue Jan 24 18:22:33 2023 -0300
hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms'
...
Rename all 'mc' MachineState pointers to 'ms'. This is a very tedious
and mechanical patch that was produced by doing the following:
- find/replace all 'MachineState *mc' to 'MachineState *ms';
- find/replace all 'mc->fdt' to 'ms->fdt';
- find/replace all 'mc->smp.cpus' to 'ms->smp.cpus';
- replace any remaining occurrences of 'mc' that the compiler complained
about.
This patch by Daniel is a no-code-change, it really is just a rename of
variables. I can rename variable back from ms to mc in the fix patch,
or I can apply this rename first and apply the fix patch cleanly, and
all subsequent changes will have much more chance to apply cleanly too.
What a wonderful world.. ;)
Thankfully, such cases are rare. But we do have a few famous cases like this
still, some of which I also mentioned in the first message in this thread.
/mjt
On Tue, Sep 12, 2023 at 09:01:43PM +0300, Michael Tokarev wrote:
> 12.09.2023 18:23, Daniel P. Berrangé wrote:
> ..
> > I tend to try to cherry-pick the dependancies in case (1) too
> > unless they are functionally invasive. Any time you manually
> > adjust a patch, you increase the likelihood that later cherry
> > picks will also require manual work. So I always favour a clean
> > cherry-pick until the point the functional risk becomes
> > unacceptable in the context of testing the change I'm pulling
> > back.
>
> Yeah, that's exactly my thought: if something in the subsystem
> has changed, esp. when the new thing is now widely used, it is
> best to try to pick it up (unless it is a big change by itself
> or is a part of big change).
>
> I already mentioned a trivial fix c255946e3df4 in this thread,
> which can be applied cleanly if two other no-change patches are
> in, 753ae97abc7 and dadee9e3ce6. It is much more likely to hit
> conflicts in this area in future updates if such updates will
> happen if such renames like these two aren't picked up.
>
> So, right in this same patch series, there's one more very similar
> change:
>
> commit 9ff31406312500053ecb5f92df01dd9ce52e635d
> Author: Conor Dooley <conor.dooley@microchip.com>
> Date: Thu Jul 27 15:24:17 2023 +0100
>
> hw/riscv: virt: Fix riscv,pmu DT node path
>
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -719,7 +719,7 @@ static void create_fdt_pmu(RISCVVirtState *s)
> MachineState *ms = MACHINE(s);
> RISCVCPU hart = s->soc[0].harts[0];
>
> - pmu_name = g_strdup_printf("/soc/pmu");
> + pmu_name = g_strdup_printf("/pmu");
> qemu_fdt_add_subnode(ms->fdt, pmu_name);
> qemu_fdt_setprop_string(ms->fdt, pmu_name, "compatible", "riscv,pmu");
> riscv_pmu_generate_fdt_node(ms->fdt, hart.cfg.pmu_num, pmu_name);
>
> But all the nearby lines are touched by previous patch:
>
> commit 568e0614d0979e0431a8d9dc0503a63b8b0f2d81
> Author: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Date: Tue Jan 24 18:22:33 2023 -0300
>
> hw/riscv/virt.c: rename MachineState 'mc' pointers to 'ms'
> ...
> Rename all 'mc' MachineState pointers to 'ms'. This is a very tedious
> and mechanical patch that was produced by doing the following:
>
> - find/replace all 'MachineState *mc' to 'MachineState *ms';
> - find/replace all 'mc->fdt' to 'ms->fdt';
> - find/replace all 'mc->smp.cpus' to 'ms->smp.cpus';
> - replace any remaining occurrences of 'mc' that the compiler complained
> about.
>
> This patch by Daniel is a no-code-change, it really is just a rename of
> variables. I can rename variable back from ms to mc in the fix patch,
> or I can apply this rename first and apply the fix patch cleanly, and
> all subsequent changes will have much more chance to apply cleanly too.
>
> What a wonderful world.. ;)
>
> Thankfully, such cases are rare. But we do have a few famous cases like this
> still, some of which I also mentioned in the first message in this thread.
Also this is the key reason why many reviewers will complain if patches
are too large, or contain a mixture of functional and non-functional
changes, or do two jobs at once. Bigger commits with varying & unrelated
changes makes cherry picking much more painful
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Sep 12, 2023, 8:01 AM Stefan Hajnoczi <stefanha@gmail.com> wrote: > When I backport patches into RHEL, the general process I follow is: > 1. For context conflicts, just adjust the patch to resolve them. > 2. For real dependencies, backport the dependencies, if possible. > 3. If backporting the dependencies is not possible, think of a > downstream-only solution. This should be rare. > > People make different backporting decisions (just like structuring > patch series). It can be a matter of taste. > We've done almost exactly the same thing in FreeBSD for the past almost 30 years (with varying degrees of success and nuance, to be true, often limited by early tools). It's an excellent ideal to shoot for, and we've had troubles more often than not the further one gets aways from it. Warner Stefan > >
© 2016 - 2024 Red Hat, Inc.