gdbstub/next (MemTxAttrs and re-factoring)

[PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs

Posted by Alex Bennée 3 years, 4 months ago

As most HVF devices are done purely in software we need to make sure
we properly encode the source CPU in MemTxAttrs. This will allow the
device emulations to use those attributes rather than relying on
current_cpu (although current_cpu will still be correct in this case).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mads Ynddal <mads@ynddal.dk>
Cc: Alexander Graf <agraf@csgraf.de>
---
 target/arm/hvf/hvf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 060aa0ccf4..13b7971560 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1233,11 +1233,11 @@ int hvf_vcpu_exec(CPUState *cpu)
             val = hvf_get_reg(cpu, srt);
             address_space_write(&address_space_memory,
                                 hvf_exit->exception.physical_address,
-                                MEMTXATTRS_UNSPECIFIED, &val, len);
+                                MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
         } else {
             address_space_read(&address_space_memory,
                                hvf_exit->exception.physical_address,
-                               MEMTXATTRS_UNSPECIFIED, &val, len);
+                               MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
             hvf_set_reg(cpu, srt, val);
         }
 
-- 
2.34.1

Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs

Posted by Alexander Graf 3 years, 4 months ago

On 26.09.22 15:38, Alex Bennée wrote:
> As most HVF devices are done purely in software we need to make sure
> we properly encode the source CPU in MemTxAttrs. This will allow the
> device emulations to use those attributes rather than relying on
> current_cpu (although current_cpu will still be correct in this case).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> Cc: Alexander Graf <agraf@csgraf.de>
> ---
>   target/arm/hvf/hvf.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 060aa0ccf4..13b7971560 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1233,11 +1233,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>               val = hvf_get_reg(cpu, srt);
>               address_space_write(&address_space_memory,
>                                   hvf_exit->exception.physical_address,
> -                                MEMTXATTRS_UNSPECIFIED, &val, len);
> +                                MEMTXATTRS_CPU(cpu->cpu_index), &val, len);


I think it would make a safer API if MEMTXATTRS_CPU() would take 
CPUState * as argument so you can just pass in cpu here.

For the HVF part however,

Acked-by: Alexander Graf <agraf@csgraf.de>


Alex

Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs

Posted by Peter Maydell 3 years, 4 months ago

On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> As most HVF devices are done purely in software we need to make sure
> we properly encode the source CPU in MemTxAttrs. This will allow the
> device emulations to use those attributes rather than relying on
> current_cpu (although current_cpu will still be correct in this case).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mads Ynddal <mads@ynddal.dk>
> Cc: Alexander Graf <agraf@csgraf.de>
> ---
>  target/arm/hvf/hvf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 060aa0ccf4..13b7971560 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -1233,11 +1233,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>              val = hvf_get_reg(cpu, srt);
>              address_space_write(&address_space_memory,
>                                  hvf_exit->exception.physical_address,
> -                                MEMTXATTRS_UNSPECIFIED, &val, len);
> +                                MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
>          } else {
>              address_space_read(&address_space_memory,
>                                 hvf_exit->exception.physical_address,
> -                               MEMTXATTRS_UNSPECIFIED, &val, len);
> +                               MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
>              hvf_set_reg(cpu, srt, val);
>          }

Don't we need a similar thing for KVM ? (In that case it's in
the generic code in accel/kvm/kvm-all.c, for the KVM_EXIT_MMIO
handling.)

-- PMM

Re: [PATCH v2 03/11] target/arm: ensure HVF traps set appropriate MemTxAttrs

Posted by Alex Bennée 3 years, 4 months ago

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 26 Sept 2022 at 14:39, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> As most HVF devices are done purely in software we need to make sure
>> we properly encode the source CPU in MemTxAttrs. This will allow the
>> device emulations to use those attributes rather than relying on
>> current_cpu (although current_cpu will still be correct in this case).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Mads Ynddal <mads@ynddal.dk>
>> Cc: Alexander Graf <agraf@csgraf.de>
>> ---
>>  target/arm/hvf/hvf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>> index 060aa0ccf4..13b7971560 100644
>> --- a/target/arm/hvf/hvf.c
>> +++ b/target/arm/hvf/hvf.c
>> @@ -1233,11 +1233,11 @@ int hvf_vcpu_exec(CPUState *cpu)
>>              val = hvf_get_reg(cpu, srt);
>>              address_space_write(&address_space_memory,
>>                                  hvf_exit->exception.physical_address,
>> -                                MEMTXATTRS_UNSPECIFIED, &val, len);
>> +                                MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
>>          } else {
>>              address_space_read(&address_space_memory,
>>                                 hvf_exit->exception.physical_address,
>> -                               MEMTXATTRS_UNSPECIFIED, &val, len);
>> +                               MEMTXATTRS_CPU(cpu->cpu_index), &val, len);
>>              hvf_set_reg(cpu, srt, val);
>>          }
>
> Don't we need a similar thing for KVM ? (In that case it's in
> the generic code in accel/kvm/kvm-all.c, for the KVM_EXIT_MMIO
> handling.)

Added to MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
which at least anticipates an out-of-kernel GIC.


>
> -- PMM


-- 
Alex Bennée