* [PATCH 0/2] target/m68k: implement unaligned accesses for m68k CPUs
@ 2024-06-23 11:57 Mark Cave-Ayland
2024-06-23 11:57 ` [PATCH 1/2] target/m68k: implement do_unaligned_access callback " Mark Cave-Ayland
2024-06-23 11:57 ` [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines Mark Cave-Ayland
0 siblings, 2 replies; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-23 11:57 UTC (permalink / raw)
To: qemu-devel, laurent
This series implements unaligned accesses for early m68k CPUs that do not
support word/long accesses at byte granularity. Patch 1 implements the
do_unaligned_access function for m68k CPUs, whilst patch 2 is based upon a
prototype patch developed by Laurent as part of Gitlab Issue #2165.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
Mark Cave-Ayland (2):
target/m68k: implement do_unaligned_access callback for m68k CPUs
target/m68k: pass alignment into TCG memory load/store routines
target/m68k/cpu.c | 1 +
target/m68k/cpu.h | 4 ++++
target/m68k/op_helper.c | 11 +++++++++++
target/m68k/translate.c | 18 +++++++++++++++---
4 files changed, 31 insertions(+), 3 deletions(-)
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
2024-06-23 11:57 [PATCH 0/2] target/m68k: implement unaligned accesses for m68k CPUs Mark Cave-Ayland
@ 2024-06-23 11:57 ` Mark Cave-Ayland
2024-06-23 15:11 ` BALATON Zoltan
2024-06-23 19:47 ` Richard Henderson
2024-06-23 11:57 ` [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines Mark Cave-Ayland
1 sibling, 2 replies; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-23 11:57 UTC (permalink / raw)
To: qemu-devel, laurent
For m68k CPUs that do not support unaligned accesses, any such access should
cause the CPU to raise an Address Error exception.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
target/m68k/cpu.c | 1 +
target/m68k/cpu.h | 4 ++++
target/m68k/op_helper.c | 11 +++++++++++
3 files changed, 16 insertions(+)
diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index efd6bbded8..25e95f9f68 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -538,6 +538,7 @@ static const TCGCPUOps m68k_tcg_ops = {
.cpu_exec_interrupt = m68k_cpu_exec_interrupt,
.do_interrupt = m68k_cpu_do_interrupt,
.do_transaction_failed = m68k_cpu_transaction_failed,
+ .do_unaligned_access = m68k_cpu_do_unaligned_access,
#endif /* !CONFIG_USER_ONLY */
};
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index b5bbeedb7a..d4c9531b1c 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -590,6 +590,10 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
unsigned size, MMUAccessType access_type,
int mmu_idx, MemTxAttrs attrs,
MemTxResult response, uintptr_t retaddr);
+G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
+ MMUAccessType access_type,
+ int mmu_idx,
+ uintptr_t retaddr);
#endif
#include "exec/cpu-all.h"
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 15bad5dd46..417b691d8d 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -558,6 +558,17 @@ raise_exception_format2(CPUM68KState *env, int tt, int ilen, uintptr_t raddr)
cpu_loop_exit(cs);
}
+#if !defined(CONFIG_USER_ONLY)
+G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
+ MMUAccessType access_type,
+ int mmu_idx, uintptr_t retaddr)
+{
+ CPUM68KState *env = cpu_env(cs);
+
+ raise_exception(env, EXCP_ADDRESS);
+}
+#endif
+
void HELPER(divuw)(CPUM68KState *env, int destr, uint32_t den, int ilen)
{
uint32_t num = env->dregs[destr];
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
2024-06-23 11:57 [PATCH 0/2] target/m68k: implement unaligned accesses for m68k CPUs Mark Cave-Ayland
2024-06-23 11:57 ` [PATCH 1/2] target/m68k: implement do_unaligned_access callback " Mark Cave-Ayland
@ 2024-06-23 11:57 ` Mark Cave-Ayland
2024-06-23 15:23 ` BALATON Zoltan
1 sibling, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-23 11:57 UTC (permalink / raw)
To: qemu-devel, laurent
Now that do_unaligned_access has been implemented for 68k CPUs, pass the required
alignment into the TCG memory load/store routines. This allows the TCG memory core
to generate an Address Error exception for unaligned memory accesses if required.
Suggested-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
---
target/m68k/translate.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 445966fb6a..661a7b4def 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv addr,
int sign, int index)
{
TCGv tmp = tcg_temp_new_i32();
+ MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
switch (opsize) {
case OS_BYTE:
+ tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
+ break;
case OS_WORD:
case OS_LONG:
- tcg_gen_qemu_ld_tl(tmp, addr, index,
- opsize | (sign ? MO_SIGN : 0) | MO_TE);
+ if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
+ memop |= MO_ALIGN_2;
+ }
+ tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
break;
default:
g_assert_not_reached();
@@ -321,11 +326,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv addr,
static inline void gen_store(DisasContext *s, int opsize, TCGv addr, TCGv val,
int index)
{
+ MemOp memop = opsize | MO_TE;
+
switch (opsize) {
case OS_BYTE:
+ tcg_gen_qemu_st_tl(val, addr, index, memop);
+ break;
case OS_WORD:
case OS_LONG:
- tcg_gen_qemu_st_tl(val, addr, index, opsize | MO_TE);
+ if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
+ memop |= MO_ALIGN_2;
+ }
+ tcg_gen_qemu_st_tl(val, addr, index, memop);
break;
default:
g_assert_not_reached();
--
2.39.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
2024-06-23 11:57 ` [PATCH 1/2] target/m68k: implement do_unaligned_access callback " Mark Cave-Ayland
@ 2024-06-23 15:11 ` BALATON Zoltan
2024-06-24 5:41 ` Mark Cave-Ayland
2024-06-23 19:47 ` Richard Henderson
1 sibling, 1 reply; 11+ messages in thread
From: BALATON Zoltan @ 2024-06-23 15:11 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, laurent
On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
> For m68k CPUs that do not support unaligned accesses, any such access should
> cause the CPU to raise an Address Error exception.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> target/m68k/cpu.c | 1 +
> target/m68k/cpu.h | 4 ++++
> target/m68k/op_helper.c | 11 +++++++++++
> 3 files changed, 16 insertions(+)
>
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index efd6bbded8..25e95f9f68 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -538,6 +538,7 @@ static const TCGCPUOps m68k_tcg_ops = {
> .cpu_exec_interrupt = m68k_cpu_exec_interrupt,
> .do_interrupt = m68k_cpu_do_interrupt,
> .do_transaction_failed = m68k_cpu_transaction_failed,
> + .do_unaligned_access = m68k_cpu_do_unaligned_access,
> #endif /* !CONFIG_USER_ONLY */
Why is it sysemu only? Shouldn't user mode cpu only emulation do the same?
I also don't get how this is restricted to pre 68020 CPUs or account for
differences between data and inst fetch on 20+ but I may be missing
somerhing as I don't know this code or 68k behaviour well. So this is just
a question, I'm not saying it's wrong but I don't understand why it's
right.
Regards,
BALATON Zoltan
> };
>
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index b5bbeedb7a..d4c9531b1c 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -590,6 +590,10 @@ void m68k_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
> unsigned size, MMUAccessType access_type,
> int mmu_idx, MemTxAttrs attrs,
> MemTxResult response, uintptr_t retaddr);
> +G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> + MMUAccessType access_type,
> + int mmu_idx,
> + uintptr_t retaddr);
> #endif
>
> #include "exec/cpu-all.h"
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 15bad5dd46..417b691d8d 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -558,6 +558,17 @@ raise_exception_format2(CPUM68KState *env, int tt, int ilen, uintptr_t raddr)
> cpu_loop_exit(cs);
> }
>
> +#if !defined(CONFIG_USER_ONLY)
> +G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> + MMUAccessType access_type,
> + int mmu_idx, uintptr_t retaddr)
> +{
> + CPUM68KState *env = cpu_env(cs);
> +
> + raise_exception(env, EXCP_ADDRESS);
> +}
> +#endif
> +
> void HELPER(divuw)(CPUM68KState *env, int destr, uint32_t den, int ilen)
> {
> uint32_t num = env->dregs[destr];
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
2024-06-23 11:57 ` [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines Mark Cave-Ayland
@ 2024-06-23 15:23 ` BALATON Zoltan
2024-06-23 19:56 ` Richard Henderson
2024-06-24 5:44 ` Mark Cave-Ayland
0 siblings, 2 replies; 11+ messages in thread
From: BALATON Zoltan @ 2024-06-23 15:23 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, laurent
On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
> Now that do_unaligned_access has been implemented for 68k CPUs, pass the required
> alignment into the TCG memory load/store routines. This allows the TCG memory core
> to generate an Address Error exception for unaligned memory accesses if required.
>
> Suggested-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
> ---
> target/m68k/translate.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 445966fb6a..661a7b4def 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv addr,
> int sign, int index)
> {
> TCGv tmp = tcg_temp_new_i32();
> + MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
>
> switch (opsize) {
> case OS_BYTE:
> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
> + break;
> case OS_WORD:
> case OS_LONG:
> - tcg_gen_qemu_ld_tl(tmp, addr, index,
> - opsize | (sign ? MO_SIGN : 0) | MO_TE);
> + if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
> + memop |= MO_ALIGN_2;
> + }
> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
You could swap the order of these so byte comes last and fall through to
it from word/long to avoid duplicated line.
Maybe this answers my question about where it's restriced by CPU type. I
wonder if this check for M68K_FEATURE_UNALIGNED_DATA could be avoded here
and done by checking it in init and only set the unaligned method for CPUs
that need it to not add overhead for most CPUs that don't need it.
Regards,
BALATON Zoltan
> break;
> default:
> g_assert_not_reached();
> @@ -321,11 +326,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv addr,
> static inline void gen_store(DisasContext *s, int opsize, TCGv addr, TCGv val,
> int index)
> {
> + MemOp memop = opsize | MO_TE;
> +
> switch (opsize) {
> case OS_BYTE:
> + tcg_gen_qemu_st_tl(val, addr, index, memop);
> + break;
> case OS_WORD:
> case OS_LONG:
> - tcg_gen_qemu_st_tl(val, addr, index, opsize | MO_TE);
> + if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
> + memop |= MO_ALIGN_2;
> + }
> + tcg_gen_qemu_st_tl(val, addr, index, memop);
> break;
> default:
> g_assert_not_reached();
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
2024-06-23 11:57 ` [PATCH 1/2] target/m68k: implement do_unaligned_access callback " Mark Cave-Ayland
2024-06-23 15:11 ` BALATON Zoltan
@ 2024-06-23 19:47 ` Richard Henderson
2024-06-24 5:55 ` Mark Cave-Ayland
1 sibling, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2024-06-23 19:47 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel, laurent
On 6/23/24 04:57, Mark Cave-Ayland wrote:
> +G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
> + MMUAccessType access_type,
> + int mmu_idx, uintptr_t retaddr)
> +{
> + CPUM68KState *env = cpu_env(cs);
> +
> + raise_exception(env, EXCP_ADDRESS);
> +}
Surely the address gets stored in the exception frame somewhere?
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
2024-06-23 15:23 ` BALATON Zoltan
@ 2024-06-23 19:56 ` Richard Henderson
2024-06-24 5:44 ` Mark Cave-Ayland
1 sibling, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2024-06-23 19:56 UTC (permalink / raw)
To: BALATON Zoltan, Mark Cave-Ayland; +Cc: qemu-devel, laurent
On 6/23/24 08:23, BALATON Zoltan wrote:
> On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
>> Now that do_unaligned_access has been implemented for 68k CPUs, pass the required
>> alignment into the TCG memory load/store routines. This allows the TCG memory core
>> to generate an Address Error exception for unaligned memory accesses if required.
>>
>> Suggested-by: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
>> ---
>> target/m68k/translate.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index 445966fb6a..661a7b4def 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv addr,
>> int sign, int index)
>> {
>> TCGv tmp = tcg_temp_new_i32();
>> + MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
>>
>> switch (opsize) {
>> case OS_BYTE:
>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>> + break;
>> case OS_WORD:
>> case OS_LONG:
>> - tcg_gen_qemu_ld_tl(tmp, addr, index,
>> - opsize | (sign ? MO_SIGN : 0) | MO_TE);
>> + if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
>> + memop |= MO_ALIGN_2;
>> + }
>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>
> You could swap the order of these so byte comes last and fall through to it from word/long
> to avoid duplicated line.
>
> Maybe this answers my question about where it's restriced by CPU type. I wonder if this
> check for M68K_FEATURE_UNALIGNED_DATA could be avoded here and done by checking it in init
> and only set the unaligned method for CPUs that need it to not add overhead for most CPUs
> that don't need it.
No, there's no overhead in having the unaligned method present always.
But swapping the order of case labels, or sinking the tcg_gen_qemu_ld_tl call below the
switch, is a good idea.
r~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
2024-06-23 15:11 ` BALATON Zoltan
@ 2024-06-24 5:41 ` Mark Cave-Ayland
0 siblings, 0 replies; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-24 5:41 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, laurent
On 23/06/2024 16:11, BALATON Zoltan wrote:
> On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
>> For m68k CPUs that do not support unaligned accesses, any such access should
>> cause the CPU to raise an Address Error exception.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> target/m68k/cpu.c | 1 +
>> target/m68k/cpu.h | 4 ++++
>> target/m68k/op_helper.c | 11 +++++++++++
>> 3 files changed, 16 insertions(+)
>>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index efd6bbded8..25e95f9f68 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -538,6 +538,7 @@ static const TCGCPUOps m68k_tcg_ops = {
>> .cpu_exec_interrupt = m68k_cpu_exec_interrupt,
>> .do_interrupt = m68k_cpu_do_interrupt,
>> .do_transaction_failed = m68k_cpu_transaction_failed,
>> + .do_unaligned_access = m68k_cpu_do_unaligned_access,
>> #endif /* !CONFIG_USER_ONLY */
>
> Why is it sysemu only? Shouldn't user mode cpu only emulation do the same? I also
> don't get how this is restricted to pre 68020 CPUs or account for differences between
> data and inst fetch on 20+ but I may be missing somerhing as I don't know this code
> or 68k behaviour well. So this is just a question, I'm not saying it's wrong but I
> don't understand why it's right.
I'm not exactly sure, but I'm guessing that this is handled by the host user code
since all CPUs that implement do_unaligned_access do so in a block contained within
#ifndef CONFIG_USER_ONLY ... #endif.
ATB,
Mark.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
2024-06-23 15:23 ` BALATON Zoltan
2024-06-23 19:56 ` Richard Henderson
@ 2024-06-24 5:44 ` Mark Cave-Ayland
2024-06-24 9:29 ` BALATON Zoltan
1 sibling, 1 reply; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-24 5:44 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: qemu-devel, laurent
On 23/06/2024 16:23, BALATON Zoltan wrote:
> On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
>> Now that do_unaligned_access has been implemented for 68k CPUs, pass the required
>> alignment into the TCG memory load/store routines. This allows the TCG memory core
>> to generate an Address Error exception for unaligned memory accesses if required.
>>
>> Suggested-by: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
>> ---
>> target/m68k/translate.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index 445966fb6a..661a7b4def 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int opsize, TCGv
>> addr,
>> int sign, int index)
>> {
>> TCGv tmp = tcg_temp_new_i32();
>> + MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
>>
>> switch (opsize) {
>> case OS_BYTE:
>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>> + break;
>> case OS_WORD:
>> case OS_LONG:
>> - tcg_gen_qemu_ld_tl(tmp, addr, index,
>> - opsize | (sign ? MO_SIGN : 0) | MO_TE);
>> + if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
>> + memop |= MO_ALIGN_2;
>> + }
>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>
> You could swap the order of these so byte comes last and fall through to it from
> word/long to avoid duplicated line.
>
> Maybe this answers my question about where it's restriced by CPU type. I wonder if
> this check for M68K_FEATURE_UNALIGNED_DATA could be avoded here and done by checking
> it in init and only set the unaligned method for CPUs that need it to not add
> overhead for most CPUs that don't need it.
I don't think that it matters too much if the method isn't implemented as the logic
surrounding when to call do_unaligned_access is contained within the TCG core.
I'll have a go at updating the ordering and send a v2 if it looks good.
ATB,
Mark.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] target/m68k: implement do_unaligned_access callback for m68k CPUs
2024-06-23 19:47 ` Richard Henderson
@ 2024-06-24 5:55 ` Mark Cave-Ayland
0 siblings, 0 replies; 11+ messages in thread
From: Mark Cave-Ayland @ 2024-06-24 5:55 UTC (permalink / raw)
To: Richard Henderson, qemu-devel, laurent
On 23/06/2024 20:47, Richard Henderson wrote:
> On 6/23/24 04:57, Mark Cave-Ayland wrote:
>> +G_NORETURN void m68k_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
>> + MMUAccessType access_type,
>> + int mmu_idx, uintptr_t retaddr)
>> +{
>> + CPUM68KState *env = cpu_env(cs);
>> +
>> + raise_exception(env, EXCP_ADDRESS);
>> +}
>
> Surely the address gets stored in the exception frame somewhere?
>
> r~
Well. There is already some logic there for EXCP_ADDRESS in m68k_interrupt_all() so I
thought this would just work as-is, but upon closer reading of older CPU datasheets
it appears there are at least 2 different stack frame formats for CPUs before the 68040.
It looks like I'll have to do a bit more research before posting a v2.
ATB,
Mark.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines
2024-06-24 5:44 ` Mark Cave-Ayland
@ 2024-06-24 9:29 ` BALATON Zoltan
0 siblings, 0 replies; 11+ messages in thread
From: BALATON Zoltan @ 2024-06-24 9:29 UTC (permalink / raw)
To: Mark Cave-Ayland; +Cc: qemu-devel, laurent
[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]
On Mon, 24 Jun 2024, Mark Cave-Ayland wrote:
> On 23/06/2024 16:23, BALATON Zoltan wrote:
>> On Sun, 23 Jun 2024, Mark Cave-Ayland wrote:
>>> Now that do_unaligned_access has been implemented for 68k CPUs, pass the
>>> required
>>> alignment into the TCG memory load/store routines. This allows the TCG
>>> memory core
>>> to generate an Address Error exception for unaligned memory accesses if
>>> required.
>>>
>>> Suggested-by: Laurent Vivier <laurent@vivier.eu>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2165
>>> ---
>>> target/m68k/translate.c | 18 +++++++++++++++---
>>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index 445966fb6a..661a7b4def 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -303,13 +303,18 @@ static inline TCGv gen_load(DisasContext *s, int
>>> opsize, TCGv addr,
>>> int sign, int index)
>>> {
>>> TCGv tmp = tcg_temp_new_i32();
>>> + MemOp memop = opsize | (sign ? MO_SIGN : 0) | MO_TE;
>>>
>>> switch (opsize) {
>>> case OS_BYTE:
>>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>>> + break;
>>> case OS_WORD:
>>> case OS_LONG:
>>> - tcg_gen_qemu_ld_tl(tmp, addr, index,
>>> - opsize | (sign ? MO_SIGN : 0) | MO_TE);
>>> + if (!m68k_feature(s->env, M68K_FEATURE_UNALIGNED_DATA)) {
>>> + memop |= MO_ALIGN_2;
>>> + }
>>> + tcg_gen_qemu_ld_tl(tmp, addr, index, memop);
>>
>> You could swap the order of these so byte comes last and fall through to it
>> from word/long to avoid duplicated line.
>>
>> Maybe this answers my question about where it's restriced by CPU type. I
>> wonder if this check for M68K_FEATURE_UNALIGNED_DATA could be avoded here
>> and done by checking it in init and only set the unaligned method for CPUs
>> that need it to not add overhead for most CPUs that don't need it.
>
> I don't think that it matters too much if the method isn't implemented as the
> logic surrounding when to call do_unaligned_access is contained within the
> TCG core.
I've seen this after I've sent a patch for PPC where removing a
conditional from a helper often called for memory access showed it had an
effect on performance. So I thought adding a conditional here might cause
some slow down for CPUs that don't need this. But this is compile time so
maybe it's not that big problem as in a hepler. Yet only adding the
unaligned method for CPUs then always set MO_ALIGN here avoiding the if
only calls the method when needed if that works at all. I don't know if
that's worth it, you could test with some memory intensive benchmark such
as STREAM benchmark to check if this has any effect.
Regards,
BALATON Zotan
> I'll have a go at updating the ordering and send a v2 if it looks good.
>
>
> ATB,
>
> Mark.
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-06-24 9:30 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-23 11:57 [PATCH 0/2] target/m68k: implement unaligned accesses for m68k CPUs Mark Cave-Ayland
2024-06-23 11:57 ` [PATCH 1/2] target/m68k: implement do_unaligned_access callback " Mark Cave-Ayland
2024-06-23 15:11 ` BALATON Zoltan
2024-06-24 5:41 ` Mark Cave-Ayland
2024-06-23 19:47 ` Richard Henderson
2024-06-24 5:55 ` Mark Cave-Ayland
2024-06-23 11:57 ` [PATCH 2/2] target/m68k: pass alignment into TCG memory load/store routines Mark Cave-Ayland
2024-06-23 15:23 ` BALATON Zoltan
2024-06-23 19:56 ` Richard Henderson
2024-06-24 5:44 ` Mark Cave-Ayland
2024-06-24 9:29 ` BALATON Zoltan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).