Re: [RFC v2 04/24] target/arm: move psci.c into tcg/sysemu/

Re: [RFC v2 04/24] target/arm: move psci.c into tcg/sysemu/

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 05cebc8597..e18d475572 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -292,21 +292,15 @@ vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len);
>   /* Callback function for when a watchpoint or breakpoint triggers. */
>   void arm_debug_excp_handler(CPUState *cs);
>   
> -#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_TCG)
> -static inline bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
> -{
> -    return false;
> -}
> -static inline void arm_handle_psci_call(ARMCPU *cpu)
> -{
> -    g_assert_not_reached();
> -}
> -#else

I don't see that removing this ifdef...

> @@ -10040,11 +10040,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
>                         env->exception.syndrome);
>       }
>   
> +#ifndef CONFIG_USER_ONLY
>       if (arm_is_psci_call(cpu, cs->exception_index)) {
>           arm_handle_psci_call(cpu);
>           qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
>           return;
>       }
> +#endif /* CONFIG_USER_ONLY */

... is an improvement on adding this one.  Just leave the static inline stub 
alone.  At some point you move the do_interrupt pieces, and the stubs can be 
removed completely, perhaps?


r~

Re: [RFC v2 07/24] target/arm: move physical address translation to new modules

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1023,9 +1023,6 @@ typedef struct ARMVAParameters {
>       bool using64k   : 1;
>   } ARMVAParameters;
>   
> -ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
> -                                   ARMMMUIdx mmu_idx, bool data);
> -

Moving aa64_va_parameters away from its struct return is not good.  Either move 
them both or not at all.

The collection of stuff you've split out into get-phys-addr.h is pretty random. 
  Let's just leave it all in internals.h for now.  We can make a more reasoned 
decision vs all of the vma and regime related functions.


r~

Re: [RFC v2 08/24] target/arm: split cpregs from tcg/helper.c

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> +uint64_t raw_read(CPUARMState *env, const ARMCPRegInfo *ri);
> +void raw_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +               uint64_t value);

These shouldn't go in cpu.h; the new cpregs.h seems reasonable.

Otherwise, LGTM.

r~

Re: [RFC v2 10/24] target/arm: only perform TCG cpu and machine inits if tcg enabled

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> @@ -1321,6 +1323,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           }
>       }
>   
> +#ifdef CONFIG_TCG
>       {
>           uint64_t scale;
>   
> @@ -1346,7 +1349,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>           cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
>                                                     arm_gt_hvtimer_cb, cpu);
>       }
> -#endif
> +#endif /* CONFIG_TCG */

You can use tcg_enabled here.

> -        }
>   
>   #ifndef CONFIG_USER_ONLY
> -        cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
> -                cpu);
> +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
> +                                          cpu);

Incorrect indentation change.
Otherwise, LGTM.


r~

Re: [RFC v2 11/24] target/arm: kvm: add stubs for some helpers

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> --- /dev/null
> +++ b/target/arm/kvm/helper-stubs.c
> @@ -0,0 +1,18 @@
> +#include "qemu/osdep.h"
> +#include "cpu.h"

Need license boilerplate.  Otherwise looks fine.


r~

Re: [RFC v2 12/24] target/arm: move cpsr_read, cpsr_write to cpu_common

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> we need as a result to move switch_mode too,
> so we put an implementation into cpu_user and cpu_sysemu.
> 
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/cpu.h        |   2 +
>   target/arm/cpu-common.c | 182 +++++++++++++++++++++++++++++++++
>   target/arm/cpu-sysemu.c |  29 ++++++
>   target/arm/cpu-user.c   |  24 +++++
>   target/arm/tcg/helper.c | 221 ----------------------------------------
>   target/arm/meson.build  |   3 +
>   6 files changed, 240 insertions(+), 221 deletions(-)
>   create mode 100644 target/arm/cpu-user.c

Some commentary about what cpu-common.c is meant to be would be helpful.  So 
far it looks like helper.c under a different name.

r~

Re: [RFC v2 13/24] target/arm: add temporary stub for arm_rebuild_hflags

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> +#ifdef CONFIG_TCG
>       /* CP15 update requires rebuilding hflags */
>       arm_rebuild_hflags(&target_cpu->env);
> +#endif /* CONFIG_TCG */
...
> +/* XXX this is used all over in hw/arm, needs Philippe's work to remove */
> +void arm_rebuild_hflags(CPUARMState *env)
> +{
> +    g_assert_not_reached();
> +}
> 

If you have the stub, you can use tcg_enabled(), not an ifdef.

r~

Re: [RFC v2 14/24] target/arm: split vfp state setting from tcg helpers

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> vfp_get_fpsr and vfp_set_fpsr are needed also for KVM.
> 
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/cpu-vfp.h          |  29 +++++
>   target/arm/cpu-vfp.c          |  92 +++++++++++++++
>   target/arm/kvm/helper-stubs.c |  10 ++
>   target/arm/tcg/cpu-vfp.c      | 146 +++++++++++++++++++++++
>   target/arm/tcg/vfp_helper.c   | 210 +---------------------------------
>   target/arm/meson.build        |   1 +
>   target/arm/tcg/meson.build    |   1 +
>   7 files changed, 281 insertions(+), 208 deletions(-)
>   create mode 100644 target/arm/cpu-vfp.h
>   create mode 100644 target/arm/cpu-vfp.c
>   create mode 100644 target/arm/tcg/cpu-vfp.c

kvm/helper-stubs.c should be for all !tcg.


r~

Re: [RFC v2 15/24] target/arm: move arm_mmu_idx* to get-phys-addr

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/get-phys-addr.c | 95 ++++++++++++++++++++++++++++++++++++++
>   target/arm/tcg/helper.c    | 95 --------------------------------------
>   2 files changed, 95 insertions(+), 95 deletions(-)

So... get-phys-addr.c is now gaining stuff not related to get_phys_addr? 
What's your end goal?

Is there a better a better name, like mmu_helper.c?  Should these in fact go 
into a second file, leaving get-phys-addr to itself?


r~

Re: [RFC v2 16/24] target/arm: move sve_zcr_len_for_el to common_cpu

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> it is required by arch-dump.c and cpu.c, so apparently
> we need this for KVM too?
> 
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/cpu-common.c | 33 +++++++++++++++++++++++++++++++++
>   target/arm/tcg/helper.c | 33 ---------------------------------
>   2 files changed, 33 insertions(+), 33 deletions(-)

Same about the choice of file as with cpsr in patch 12.  This is 64-bit 
specific at least.


r~

Re: [RFC v2 18/24] target/arm: move arm_cpu_list to common_cpu

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/cpu-common.c | 42 +++++++++++++++++++++++++++++++++++++++++
>   target/arm/tcg/helper.c | 41 ----------------------------------------
>   2 files changed, 42 insertions(+), 41 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [RFC v2 05/24] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> +#ifdef CONFIG_TCG
>   bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>   {
>       CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -607,6 +608,7 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>       cc->tcg_ops->do_interrupt(cs);
>       return true;
>   }
> +#endif /* CONFIG_TCG */

Drop this and just wait for the move to tcg/tcg-cpu.c.


r~

Re: [RFC v2 06/24] target/arm: split off cpu-sysemu.c

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/internals.h  |   8 ++-
>   target/arm/cpu-sysemu.c | 105 ++++++++++++++++++++++++++++++++++++++++
>   target/arm/cpu.c        |  83 -------------------------------
>   target/arm/meson.build  |   1 +
>   4 files changed, 113 insertions(+), 84 deletions(-)
>   create mode 100644 target/arm/cpu-sysemu.c

It'd be nice to rearrange this into tcg/ and kvm/.

I think we could do with some macros like

#ifndef CONFIG_KVM
#define KVM_ERROR  QEMU_ERROR("kvm is disabled")
#endif
#ifndef CONFIG_TCG
#define TCG_ERROR  QEMU_ERROR("tcg is disabled")
#endif

Not sure where to put these, but certainly not arm specific.

Then,

void arm_cpu_tcg_set_irq(void *opaque, int irq, int level)
     TCG_ERROR;
void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
     KVM_ERROR;

     if (kvm_enabled()) {
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
     } else if (tcg_enabled()) {
         qdev_init_gpio_in(DEVICE(cpu), arm_cpu_tcg_set_irq, 4);
     } else {
         g_assert_not_reached();
     }

So arm_cpu_kvm_set_irq can go in kvm/ and needs no ifdef.

I'll let folks interested in xen and hvf figure our what needs doing with the 
above.


r~

Re: [RFC v2 19/24] target/arm: move aarch64_sync_32_to_64 (and vv) to cpu code

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> and arm_phys_excp_target_el since it is tied up inside the
> same #ifdef block.
> 
> aarch64_sync_32_to_64 and aarch64_sync_64_to_32 are
> mixed in with the tcg helpers, but they shouldn't, as they
> are needed for kvm too, in the sysemu case.

Really?  Now that *is* surprising.  Again, please document.  Because I surely 
cannot believe this to be used by kvm.


r~

Re: [RFC v2 24/24] target/arm: wrap call to aarch64_sve_change_el in tcg_enabled()

[Claudio Fontana]

Probably I should describe here that after this patch,

configure --enable-tcg --disable-kvm

is now buildable.

Ciao,

Claudio

On 3/1/21 5:49 PM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu-sysemu.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu-sysemu.c b/target/arm/cpu-sysemu.c
> index 451ad4f8bd..034d073c38 100644
> --- a/target/arm/cpu-sysemu.c
> +++ b/target/arm/cpu-sysemu.c
> @@ -754,11 +754,13 @@ static void arm_cpu_do_interrupt_aarch64(CPUState *cs)
>      unsigned int cur_el = arm_current_el(env);
>      int rt;
>  
> -    /*
> -     * Note that new_el can never be 0.  If cur_el is 0, then
> -     * el0_a64 is is_a64(), else el0_a64 is ignored.
> -     */
> -    aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
> +    if (tcg_enabled()) {
> +        /*
> +         * Note that new_el can never be 0.  If cur_el is 0, then
> +         * el0_a64 is is_a64(), else el0_a64 is ignored.
> +         */
> +        aarch64_sve_change_el(env, cur_el, new_el, is_a64(env));
> +    }
>  
>      if (cur_el < new_el) {
>          /* Entry vector offset depends on whether the implemented EL
> 

Re: [RFC v2 10/24] target/arm: only perform TCG cpu and machine inits if tcg enabled

[Claudio Fontana]

On 3/2/21 4:36 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> @@ -1321,6 +1323,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           }
>>       }
>>   
>> +#ifdef CONFIG_TCG
>>       {
>>           uint64_t scale;
>>   
>> @@ -1346,7 +1349,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>           cpu->gt_timer[GTIMER_HYPVIRT] = timer_new(QEMU_CLOCK_VIRTUAL, scale,
>>                                                     arm_gt_hvtimer_cb, cpu);
>>       }
>> -#endif
>> +#endif /* CONFIG_TCG */
> 
> You can use tcg_enabled here.

It is not possible currently. I probably could write a comment about it.

It would break migration for the --enable-kvm --enable-tcg build, due to

+#ifdef CONFIG_TCG
         VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
         VMSTATE_TIMER_PTR(gt_timer[GTIMER_VIRT], ARMCPU),
+#else
+        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
+        VMSTATE_UNUSED(sizeof(QEMUTimer *)),
+#endif /* CONFIG_TCG */



> 
>> -        }
>>   
>>   #ifndef CONFIG_USER_ONLY
>> -        cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
>> -                cpu);
>> +            cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
>> +                                          cpu);
> 
> Incorrect indentation change.
> Otherwise, LGTM.
> 
> 
> r~
> 

you need more lines of context in this patch than what you are quoting here,
the indentation change is intended.

The whole code, including cpu->pmu_timer is now only executed under tcg_enabled().

Thanks,

CLaudio

Re: [RFC v2 05/24] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Claudio Fontana]

On 3/2/21 6:50 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> +#ifdef CONFIG_TCG
>>   bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>   {
>>       CPUClass *cc = CPU_GET_CLASS(cs);
>> @@ -607,6 +608,7 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>       cc->tcg_ops->do_interrupt(cs);
>>       return true;
>>   }
>> +#endif /* CONFIG_TCG */
> 
> Drop this and just wait for the move to tcg/tcg-cpu.c.
> 
> 
> r~
> 

Right, will do, thanks.

C

Re: [RFC v2 06/24] target/arm: split off cpu-sysemu.c

[Claudio Fontana]

On 3/2/21 7:17 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/internals.h  |   8 ++-
>>   target/arm/cpu-sysemu.c | 105 ++++++++++++++++++++++++++++++++++++++++
>>   target/arm/cpu.c        |  83 -------------------------------
>>   target/arm/meson.build  |   1 +
>>   4 files changed, 113 insertions(+), 84 deletions(-)
>>   create mode 100644 target/arm/cpu-sysemu.c
> 
> It'd be nice to rearrange this into tcg/ and kvm/.

Yes.

This would be the objective of the next series,

basically the objective of this series is to make the kvm-only build succeed after moving helpers to tcg/ and picking stuff we need from helper.c .

configure --enable-kvm --disable-tcg

The objective of the next one would be to properly place the code in kvm-only and tcg-only sections,
and then add the classes that specialize the cpu, just like for i386.

But I can continue this series as a whole if preferrable.

> 
> I think we could do with some macros like
> 
> #ifndef CONFIG_KVM
> #define KVM_ERROR  QEMU_ERROR("kvm is disabled")
> #endif
> #ifndef CONFIG_TCG
> #define TCG_ERROR  QEMU_ERROR("tcg is disabled")
> #endif
> 
> Not sure where to put these, but certainly not arm specific.
> 
> Then,
> 
> void arm_cpu_tcg_set_irq(void *opaque, int irq, int level)
>      TCG_ERROR;
> void arm_cpu_kvm_set_irq(void *opaque, int irq, int level)
>      KVM_ERROR;
> 
>      if (kvm_enabled()) {
>          qdev_init_gpio_in(DEVICE(cpu), arm_cpu_kvm_set_irq, 4);
>      } else if (tcg_enabled()) {
>          qdev_init_gpio_in(DEVICE(cpu), arm_cpu_tcg_set_irq, 4);
>      } else {
>          g_assert_not_reached();
>      }
> 
> So arm_cpu_kvm_set_irq can go in kvm/ and needs no ifdef.
> 
> I'll let folks interested in xen and hvf figure our what needs doing with the 
> above.
> 
> 
> r~
> 

Thanks,

Claudio

Re: [RFC v2 12/24] target/arm: move cpsr_read, cpsr_write to cpu_common

[Claudio Fontana]

On 3/2/21 5:02 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> we need as a result to move switch_mode too,
>> so we put an implementation into cpu_user and cpu_sysemu.
>>
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/cpu.h        |   2 +
>>   target/arm/cpu-common.c | 182 +++++++++++++++++++++++++++++++++
>>   target/arm/cpu-sysemu.c |  29 ++++++
>>   target/arm/cpu-user.c   |  24 +++++
>>   target/arm/tcg/helper.c | 221 ----------------------------------------
>>   target/arm/meson.build  |   3 +
>>   6 files changed, 240 insertions(+), 221 deletions(-)
>>   create mode 100644 target/arm/cpu-user.c
> 
> Some commentary about what cpu-common.c is meant to be would be helpful.  So 
> far it looks like helper.c under a different name.
> 
> r~
> 

In the intention,

cpu-common.c, cpu-sysemu.c, cpu-user.c

cpu-common.c: cpu class definitions and functions common between sysemu and user
cpu-sysemu.c: sysemu-only cpu class definitions and functions
cpu-user.c: user-only

I agree that things need improving to get there, 
will take a good look,

thanks,

C

Re: [RFC v2 14/24] target/arm: split vfp state setting from tcg helpers

[Claudio Fontana]

On 3/2/21 5:24 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> vfp_get_fpsr and vfp_set_fpsr are needed also for KVM.
>>
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/cpu-vfp.h          |  29 +++++
>>   target/arm/cpu-vfp.c          |  92 +++++++++++++++
>>   target/arm/kvm/helper-stubs.c |  10 ++
>>   target/arm/tcg/cpu-vfp.c      | 146 +++++++++++++++++++++++
>>   target/arm/tcg/vfp_helper.c   | 210 +---------------------------------
>>   target/arm/meson.build        |   1 +
>>   target/arm/tcg/meson.build    |   1 +
>>   7 files changed, 281 insertions(+), 208 deletions(-)
>>   create mode 100644 target/arm/cpu-vfp.h
>>   create mode 100644 target/arm/cpu-vfp.c
>>   create mode 100644 target/arm/tcg/cpu-vfp.c
> 
> kvm/helper-stubs.c should be for all !tcg.
> 
> 
> r~
> 

Can you explain more what you mean?

In this patch kvm/helper-stubs.c is updated to include two stubs:

uint32_t vfp_get_fpscr_from_host(CPUARMState *env)
{
    return 0;
}

void vfp_set_fpscr_to_host(CPUARMState *env, uint32_t val)
{
}

The corresponding tcg code instead is in tcg/arm/cpu-vtp.c.

For the non-stub code that comprises

vfp_get_fpscr
vfp_set_fpscr

the code is moved to target/arm/cpu-vfp.c,

to be shared by tcg and kvm.

Thanks,

C

Re: [RFC v2 04/24] target/arm: move psci.c into tcg/sysemu/

[Claudio Fontana]

On 3/1/21 7:16 PM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 05cebc8597..e18d475572 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -292,21 +292,15 @@ vaddr arm_adjust_watchpoint_address(CPUState *cs, vaddr addr, int len);
>>   /* Callback function for when a watchpoint or breakpoint triggers. */
>>   void arm_debug_excp_handler(CPUState *cs);
>>   
>> -#if defined(CONFIG_USER_ONLY) || !defined(CONFIG_TCG)
>> -static inline bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>> -{
>> -    return false;
>> -}
>> -static inline void arm_handle_psci_call(ARMCPU *cpu)
>> -{
>> -    g_assert_not_reached();
>> -}
>> -#else
> 
> I don't see that removing this ifdef...
> 
>> @@ -10040,11 +10040,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>                         env->exception.syndrome);
>>       }
>>   
>> +#ifndef CONFIG_USER_ONLY
>>       if (arm_is_psci_call(cpu, cs->exception_index)) {
>>           arm_handle_psci_call(cpu);
>>           qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
>>           return;
>>       }
>> +#endif /* CONFIG_USER_ONLY */
> 
> ... is an improvement on adding this one.  Just leave the static inline stub 
> alone.  At some point you move the do_interrupt pieces, and the stubs can be 
> removed completely, perhaps?
> 
> 
> r~
> 

Yes, agreed.

Thanks, C

Re: [RFC v2 07/24] target/arm: move physical address translation to new modules

[Claudio Fontana]

On 3/1/21 7:49 PM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1023,9 +1023,6 @@ typedef struct ARMVAParameters {
>>       bool using64k   : 1;
>>   } ARMVAParameters;
>>   
>> -ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va,
>> -                                   ARMMMUIdx mmu_idx, bool data);
>> -
> 
> Moving aa64_va_parameters away from its struct return is not good.  Either move 
> them both or not at all.

Good point, will take a look at both options.
> 
> The collection of stuff you've split out into get-phys-addr.h is pretty random. 
>   Let's just leave it all in internals.h for now.  We can make a more reasoned 
> decision vs all of the vma and regime related functions.
> 
> 
> r~
> 

The goal here was to make get_phys_addr accessible from KVM too.

get_phys_addr requires the va_parameters family of functions.

Lets see..

Thanks,

C

Re: [RFC v2 04/24] target/arm: move psci.c into tcg/sysemu/

[Peter Maydell]

On Mon, 1 Mar 2021 at 16:49, Claudio Fontana <cfontana@suse.de> wrote:
>
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---

I note that the not-yet-landed aarch64 support for the hvf accelerator
is hoping to share the tcg psci implementation...

-- PMM

Re: [RFC v2 19/24] target/arm: move aarch64_sync_32_to_64 (and vv) to cpu code

[Claudio Fontana]

On 3/2/21 7:23 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> and arm_phys_excp_target_el since it is tied up inside the
>> same #ifdef block.
>>
>> aarch64_sync_32_to_64 and aarch64_sync_64_to_32 are
>> mixed in with the tcg helpers, but they shouldn't, as they
>> are needed for kvm too, in the sysemu case.
> 
> Really?  Now that *is* surprising.  Again, please document.  Because I surely 
> cannot believe this to be used by kvm.
> 
> 
> r~
> 

Ok, will add comment here as well, they are needed by kvm64.c

There is lots that surprised me too on how kvm is bolted on top of tcg code currently;

partly related to this, if it can help understand this,

in kvm.c notice the kvm_arm_init_cpreg_list():

it basically undoes all TCG initializations in the cpreg_indexes, cpreg_values etc, and 
rebuilds the cpreg list based on the list read from the kernel.

Then, in kvm64.c:
kvm_arch_get_registers()
{
    ...
    if (!is_a64(env)) {
        aarch64_sync_64_to_32(env);
    }
    ...
    write_kvmstate_to_list(cpu);
    ...
    write_list_to_cpustate(cpu);
    ...
}

kvm_arch_put_registers()
{
    ...
    if (!is_a64(env)) {
        aarch64_sync_32_to_64(env);
    }
    ...
    write_cpustate_to_list(cpu, true);
    ...
    write_list_to_kvmstate(cpu, level)
    ...
}

Thanks,

Claudio

Re: [RFC v2 19/24] target/arm: move aarch64_sync_32_to_64 (and vv) to cpu code

[Peter Maydell]

On Tue, 2 Mar 2021 at 11:59, Claudio Fontana <cfontana@suse.de> wrote:
> Then, in kvm64.c:
> kvm_arch_get_registers()
> {
>     ...
>     if (!is_a64(env)) {
>         aarch64_sync_64_to_32(env);
>     }
>     ...
>     write_kvmstate_to_list(cpu);
>     ...
>     write_list_to_cpustate(cpu);
>     ...
> }

The way to think about this is that there are three places where
system register state can be stored:
 * in the kernel (assuming we're using KVM)
 * in the 'list', which is the cpreg_indexes[]/cpreg_values[] arrays
 * in fields in QEMU's CPUARMState structure

The "list" data structure is a transitional one only: we use it:
 (1) for migration: outgoing migration is of the cpreg_indexes/values
      arrays, and incoming migration goes into these arrays
 (2) as the intermediate point when moving state between the kernel
      and the CPUARMState structure fields: we have functions for
      going between KVM state and the lists, and for going between
      the lists and CPU state fields

Nothing else except the migration and the conversion functions should
need to know about or touch the 'list' representation. All QEMU code
which needs to examine or update guest CPU state will arrange that
the in-kernel state is synced into the CPU state struct fields (going via the
list in the process) and then update CPU state fields. Before the guest
is run again we sync in the opposite direction (again via the list structures).

thanks
-- PMM

Re: [RFC v2 15/24] target/arm: move arm_mmu_idx* to get-phys-addr

[Claudio Fontana]

On 3/2/21 5:35 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/get-phys-addr.c | 95 ++++++++++++++++++++++++++++++++++++++
>>   target/arm/tcg/helper.c    | 95 --------------------------------------
>>   2 files changed, 95 insertions(+), 95 deletions(-)
> 
> So... get-phys-addr.c is now gaining stuff not related to get_phys_addr? 
> What's your end goal?
> 
> Is there a better a better name, like mmu_helper.c?  Should these in fact go 
> into a second file, leaving get-phys-addr to itself?
> 
> 
> r~
> 

These are all good questions;

I would like to avoid the word "helper", as it triggers tcg/ HELPER() stuff for me,
but I am not excluding it if we don't have a better idea.

The alternative to put this stuff into their own file is open for me.

Thanks,

Claudio

Re: [RFC v2 21/24] target/arm: cpu64: wrap TCG-only parts of aarch64_cpu_dump_state

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> -    if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
> +    if (tcg_enabled() &&
> +        cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {

There's nothing tcg-only about this -- kvm supports sve.

r~

Re: [RFC v2 22/24] target/arm: cpu64: wrap arm cpregs with CONFIG_TCG

[Richard Henderson]

On 3/1/21 8:49 AM, Claudio Fontana wrote:
> these cpregs should be moved to tcg/ later on.
> 
> Signed-off-by: Claudio Fontana<cfontana@suse.de>
> ---
>   target/arm/cpu64.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

What is this in aid of?  It's certainly wrong.

r~

Re: [RFC v2 21/24] target/arm: cpu64: wrap TCG-only parts of aarch64_cpu_dump_state

[Claudio Fontana]

On 3/2/21 5:56 PM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> -    if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
>> +    if (tcg_enabled() &&
>> +        cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
> 
> There's nothing tcg-only about this -- kvm supports sve.
> 
> r~
> 

Indeed, on my box I am using probably just the cpu_isar_feature check fails.

This is basically an open question: do we need an sve_exception_el helper stub that always returns 0?

Thanks,

Claudio

Re: [RFC v2 22/24] target/arm: cpu64: wrap arm cpregs with CONFIG_TCG

[Claudio Fontana]

On 3/2/21 5:58 PM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> these cpregs should be moved to tcg/ later on.
>>
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/cpu64.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
> What is this in aid of?  It's certainly wrong.
> 
> r~
> 

Yes, I dropped it, thanks.

Claudio

Re: [RFC v2 04/24] target/arm: move psci.c into tcg/sysemu/

[Claudio Fontana]

On 3/2/21 12:54 PM, Peter Maydell wrote:
> On Mon, 1 Mar 2021 at 16:49, Claudio Fontana <cfontana@suse.de> wrote:
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
> 
> I note that the not-yet-landed aarch64 support for the hvf accelerator
> is hoping to share the tcg psci implementation...
> 
> -- PMM
> 

Hi Peter,

interesting that HVF does not provide a PSCI interface.

I can change this to keep psci in the main dir, and for now only include it for TCG.

Then when ARM HVF is ready the condition can be changed to include HVF too.

Thanks for the headsup

Ciao,

Claudio

Re: [RFC v2 21/24] target/arm: cpu64: wrap TCG-only parts of aarch64_cpu_dump_state

[Richard Henderson]

On 3/2/21 10:01 AM, Claudio Fontana wrote:
> On 3/2/21 5:56 PM, Richard Henderson wrote:
>> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>>> -    if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
>>> +    if (tcg_enabled() &&
>>> +        cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
>>
>> There's nothing tcg-only about this -- kvm supports sve.
>>
>> r~
>>
> 
> Indeed, on my box I am using probably just the cpu_isar_feature check fails.

Yes, sve-enabled cpus are quite rare so far.

> This is basically an open question: do we need an sve_exception_el helper stub that always returns 0?

Hmm.  I think not -- this is checking first that sve is present in the cpu, and 
second that sve is actually enabled at runtime.  There's nothing tcg-specific 
in either test.

I think we'd want to keep the sve_exception_el check as-is.  Just document the 
use path.


r~

Re: [RFC v2 16/24] target/arm: move sve_zcr_len_for_el to common_cpu

[Claudio Fontana]

On 3/2/21 5:41 AM, Richard Henderson wrote:
> On 3/1/21 8:49 AM, Claudio Fontana wrote:
>> it is required by arch-dump.c and cpu.c, so apparently
>> we need this for KVM too?
>>
>> Signed-off-by: Claudio Fontana<cfontana@suse.de>
>> ---
>>   target/arm/cpu-common.c | 33 +++++++++++++++++++++++++++++++++
>>   target/arm/tcg/helper.c | 33 ---------------------------------
>>   2 files changed, 33 insertions(+), 33 deletions(-)
> 
> Same about the choice of file as with cpsr in patch 12.  This is 64-bit 
> specific at least.
> 
> 
> r~
> 

Indeed, fixing this requires more shuffling though, because the existing code
does not protect TARGET_AARCH64-only registers and code for this.