| Message ID | 20230525232558.1758967-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_LSE2 | expand |
Hi, On 26/5/23 01:25, Richard Henderson wrote: > Document the meaning of exclusive_high in a big-endian context, > and why we can't change it now. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index d469a2637b..4e16eab82e 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -677,8 +677,15 @@ typedef struct CPUArchState { > uint64_t zcr_el[4]; /* ZCR_EL[1-3] */ > uint64_t smcr_el[4]; /* SMCR_EL[1-3] */ > } vfp; > + > uint64_t exclusive_addr; > uint64_t exclusive_val; > + /* > + * Contains the 'val' for the second 64-bit register of LDXP, which comes > + * from the higher address, not the high part of a complete 128-bit value. > + * This is perhaps confusingly named, but the name is now baked into the > + * migration format. > + */ > uint64_t exclusive_high; Can't we rename the field if we add the old name to check_fields_match() in scripts/vmstate-static-checker.py? Juan, could we store this renamed information directly in the code in VMState? Maybe adding some VMSTATE_KEY_ALIAS(_old_key, _new_key) macro and have the migration/ code magically deal with that :) I.e. here: VMSTATE_KEY_ALIAS("exclusive_val", high_addr),
Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > Hi, > > On 26/5/23 01:25, Richard Henderson wrote: >> Document the meaning of exclusive_high in a big-endian context, >> and why we can't change it now. >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/cpu.h | 7 +++++++ >> 1 file changed, 7 insertions(+) >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >> index d469a2637b..4e16eab82e 100644 >> --- a/target/arm/cpu.h >> +++ b/target/arm/cpu.h >> @@ -677,8 +677,15 @@ typedef struct CPUArchState { >> uint64_t zcr_el[4]; /* ZCR_EL[1-3] */ >> uint64_t smcr_el[4]; /* SMCR_EL[1-3] */ >> } vfp; >> + >> uint64_t exclusive_addr; >> uint64_t exclusive_val; >> + /* >> + * Contains the 'val' for the second 64-bit register of LDXP, which comes >> + * from the higher address, not the high part of a complete 128-bit value. >> + * This is perhaps confusingly named, but the name is now baked into the >> + * migration format. >> + */ >> uint64_t exclusive_high; > > Can't we rename the field if we add the old name to check_fields_match() > in scripts/vmstate-static-checker.py? > > Juan, could we store this renamed information directly in the code in > VMState? Maybe adding some VMSTATE_KEY_ALIAS(_old_key, _new_key) macro > and have the migration/ code magically deal with that :) > > I.e. here: > > VMSTATE_KEY_ALIAS("exclusive_val", high_addr), You are asking for magic? In VMState, that nobody understands. Sniff. I remembered that VMState only cares about values, not for field names. We can rename fields without any trouble.... Until we arrive to dump_vmstate_vmsf(). But I think we can have both things, the only thing that we really care about vmstate dump is to do comparisons. And for doing comparisons you should be using vmstate-static-checker.py That already have support for this. Look up: check_fields_match() and see how it renamed other fields. As you know better how to do this, can you play with the script and see if you can get what you want? If not, I can try to modify the script to get to what you need. Later, Juan.
On 5/26/23 02:49, Juan Quintela wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> Hi, >> >> On 26/5/23 01:25, Richard Henderson wrote: >>> Document the meaning of exclusive_high in a big-endian context, >>> and why we can't change it now. >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> target/arm/cpu.h | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >>> index d469a2637b..4e16eab82e 100644 >>> --- a/target/arm/cpu.h >>> +++ b/target/arm/cpu.h >>> @@ -677,8 +677,15 @@ typedef struct CPUArchState { >>> uint64_t zcr_el[4]; /* ZCR_EL[1-3] */ >>> uint64_t smcr_el[4]; /* SMCR_EL[1-3] */ >>> } vfp; >>> + >>> uint64_t exclusive_addr; >>> uint64_t exclusive_val; >>> + /* >>> + * Contains the 'val' for the second 64-bit register of LDXP, which comes >>> + * from the higher address, not the high part of a complete 128-bit value. >>> + * This is perhaps confusingly named, but the name is now baked into the >>> + * migration format. >>> + */ >>> uint64_t exclusive_high; >> >> Can't we rename the field if we add the old name to check_fields_match() >> in scripts/vmstate-static-checker.py? >> >> Juan, could we store this renamed information directly in the code in >> VMState? Maybe adding some VMSTATE_KEY_ALIAS(_old_key, _new_key) macro >> and have the migration/ code magically deal with that :) >> >> I.e. here: >> >> VMSTATE_KEY_ALIAS("exclusive_val", high_addr), > > You are asking for magic? > In VMState, that nobody understands. > > Sniff. > > I remembered that VMState only cares about values, not for field names. > We can rename fields without any trouble.... > > Until we arrive to dump_vmstate_vmsf(). > > But I think we can have both things, the only thing that we really care > about vmstate dump is to do comparisons. And for doing comparisons you > should be using vmstate-static-checker.py > > That already have support for this. Look up: > > check_fields_match() > > and see how it renamed other fields. > > As you know better how to do this, can you play with the script and see > if you can get what you want? > > If not, I can try to modify the script to get to what you need. It's not worth any effort to rename. Just needed documentation. r~
On Fri, 26 May 2023 at 15:44, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/26/23 02:49, Juan Quintela wrote: > > Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> Hi, > >> > >> On 26/5/23 01:25, Richard Henderson wrote: > >>> Document the meaning of exclusive_high in a big-endian context, > >>> and why we can't change it now. > >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >>> --- > >>> target/arm/cpu.h | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h > >>> index d469a2637b..4e16eab82e 100644 > >>> --- a/target/arm/cpu.h > >>> +++ b/target/arm/cpu.h > >>> @@ -677,8 +677,15 @@ typedef struct CPUArchState { > >>> uint64_t zcr_el[4]; /* ZCR_EL[1-3] */ > >>> uint64_t smcr_el[4]; /* SMCR_EL[1-3] */ > >>> } vfp; > >>> + > >>> uint64_t exclusive_addr; > >>> uint64_t exclusive_val; > >>> + /* > >>> + * Contains the 'val' for the second 64-bit register of LDXP, which comes > >>> + * from the higher address, not the high part of a complete 128-bit value. > >>> + * This is perhaps confusingly named, but the name is now baked into the > >>> + * migration format. > >>> + */ > >>> uint64_t exclusive_high; > >> > >> Can't we rename the field if we add the old name to check_fields_match() > >> in scripts/vmstate-static-checker.py? > It's not worth any effort to rename. Just needed documentation. Yeah; the important point here is "we can't trivially switch to recording the exclusive value as 'high:low' of a guest 128 bit value" -- it has to remain "value from low address, value from high address". Really what is baked into the migration format is that the semantics of the two fields are from-low-addr and from-high-addr. If you replace this: > >>> + * This is perhaps confusingly named, but the name is now baked into the > >>> + * migration format. with: * In some ways it might be more convenient to record the * exclusive value as the low and high halves of a 128 bit * data value, but the current semantics of these fields are * baked into the migration format. then: Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index d469a2637b..4e16eab82e 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -677,8 +677,15 @@ typedef struct CPUArchState { uint64_t zcr_el[4]; /* ZCR_EL[1-3] */ uint64_t smcr_el[4]; /* SMCR_EL[1-3] */ } vfp; + uint64_t exclusive_addr; uint64_t exclusive_val; + /* + * Contains the 'val' for the second 64-bit register of LDXP, which comes + * from the higher address, not the high part of a complete 128-bit value. + * This is perhaps confusingly named, but the name is now baked into the + * migration format. + */ uint64_t exclusive_high; /* iwMMXt coprocessor state. */
Document the meaning of exclusive_high in a big-endian context, and why we can't change it now. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu.h | 7 +++++++ 1 file changed, 7 insertions(+)