Re: [RFC v1 32/38] target/arm: cpu: do not initialize TCG PMU for KVM

Re: [RFC v1 32/38] target/arm: cpu: do not initialize TCG PMU for KVM

[Philippe Mathieu-Daudé]

On 2/21/21 10:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> KVM uses its own PMU initialization.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a8321fecf8..d334987cad 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1648,6 +1648,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          unset_feature(env, ARM_FEATURE_PMU);
>      }
>      if (arm_feature(env, ARM_FEATURE_PMU)) {
> +#ifdef CONFIG_TCG

Shouldn't this be #if !defined(CONFIG_KVM) ?

>          pmu_init(cpu);
>  
>          if (!kvm_enabled()) {

And remove this ^

> @@ -1659,6 +1660,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
>                  cpu);
>  #endif
> +#endif /* CONFIG_TCG */
>      } else {
>          cpu->isar.id_aa64dfr0 =
>              FIELD_DP64(cpu->isar.id_aa64dfr0, ID_AA64DFR0, PMUVER, 0);
> 

Re: [RFC v1 32/38] target/arm: cpu: do not initialize TCG PMU for KVM

[Claudio Fontana]

On 2/21/21 10:53 AM, Philippe Mathieu-Daudé wrote:
> On 2/21/21 10:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> KVM uses its own PMU initialization.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/cpu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index a8321fecf8..d334987cad 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1648,6 +1648,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          unset_feature(env, ARM_FEATURE_PMU);
>>      }
>>      if (arm_feature(env, ARM_FEATURE_PMU)) {
>> +#ifdef CONFIG_TCG
> 
> Shouldn't this be #if !defined(CONFIG_KVM) ?

In this series pmu is built only for tcg in tcg/, and assumes the current pmu software implementation is TCG-only.

Should PMU instead be a separate feature, that is pulled in by TCG but not KVM?

Do we for example expect other hypervisors (hvf...) to use this PMU?


> 
>>          pmu_init(cpu);
>>  
>>          if (!kvm_enabled()) {
> 
> And remove this ^

Rather, maybe the check should be if (tcg_enabled()).

I think the code quoted (also in master) handles the --enable-tcg --enable-kvm build.

If you have both tcg and kvm "available" (currently controlled by what code is built, but in the future probably it will be modules),

it makes sese to only initialize the software PMU if kvm is not enabled (or, expressed better, if tcg is enabled).



> 
>> @@ -1659,6 +1660,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb,
>>                  cpu);
>>  #endif
>> +#endif /* CONFIG_TCG */
>>      } else {
>>          cpu->isar.id_aa64dfr0 =
>>              FIELD_DP64(cpu->isar.id_aa64dfr0, ID_AA64DFR0, PMUVER, 0);
>>
> 

Re: [RFC v1 32/38] target/arm: cpu: do not initialize TCG PMU for KVM

[Richard Henderson]

On 2/21/21 1:53 AM, Philippe Mathieu-Daudé wrote:
> On 2/21/21 10:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> KVM uses its own PMU initialization.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/cpu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index a8321fecf8..d334987cad 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1648,6 +1648,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          unset_feature(env, ARM_FEATURE_PMU);
>>      }
>>      if (arm_feature(env, ARM_FEATURE_PMU)) {
>> +#ifdef CONFIG_TCG
> 
> Shouldn't this be #if !defined(CONFIG_KVM) ?

No, because that would break the normal build which enables both tcg and kvm.

But I think there shouldn't be an ifdef at all, just a stub.


r~

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Philippe Mathieu-Daudé]

On 2/21/21 10:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> KVM has its own cpu->kvm_vtime.
> 
> Adjust cpu vmstate by putting unused fields instead of the
> VMSTATE_TIMER_PTR when TCG is not available.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu.c     | 4 +++-
>  target/arm/machine.c | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 1d81a1e7ac..b929109054 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>          }
>      }
>  
> +#ifdef CONFIG_TCG
>      {
>          uint64_t scale;
>  
> @@ -1347,7 +1348,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);

What about Xen?

>      }
> -#endif
> +#endif /* CONFIG_TCG */
> +#endif /* !CONFIG_USER_ONLY */
>  
>      cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 666ef329ef..13d7c6d930 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
> +#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 */
>          {
>              .name = "power_state",
>              .version_id = 0,
> 

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/21/21 10:55 AM, Philippe Mathieu-Daudé wrote:
> On 2/21/21 10:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> KVM has its own cpu->kvm_vtime.
>>
>> Adjust cpu vmstate by putting unused fields instead of the
>> VMSTATE_TIMER_PTR when TCG is not available.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/cpu.c     | 4 +++-
>>  target/arm/machine.c | 5 +++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 1d81a1e7ac..b929109054 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> +#ifdef CONFIG_TCG
>>      {
>>          uint64_t scale;
>>  
>> @@ -1347,7 +1348,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);
> 
> What about Xen?

Good question, what about it..

Ccing also Olaf.


> 
>>      }
>> -#endif
>> +#endif /* CONFIG_TCG */
>> +#endif /* !CONFIG_USER_ONLY */
>>  
>>      cpu_exec_realizefn(cs, &local_err);
>>      if (local_err != NULL) {
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 666ef329ef..13d7c6d930 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>> +#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 */
>>          {
>>              .name = "power_state",
>>              .version_id = 0,
>>
> 

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/21/21 2:59 PM, Claudio Fontana wrote:
> On 2/21/21 10:55 AM, Philippe Mathieu-Daudé wrote:
>> On 2/21/21 10:24 AM, Claudio Fontana wrote:
>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>
>>> KVM has its own cpu->kvm_vtime.
>>>
>>> Adjust cpu vmstate by putting unused fields instead of the
>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>  target/arm/cpu.c     | 4 +++-
>>>  target/arm/machine.c | 5 +++++
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 1d81a1e7ac..b929109054 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>          }
>>>      }
>>>  
>>> +#ifdef CONFIG_TCG
>>>      {
>>>          uint64_t scale;
>>>  
>>> @@ -1347,7 +1348,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);
>>
>> What about Xen?
> 
> Good question, what about it..
> 
> Ccing also Olaf.

This being arm, maybe Alex could shed some light? I think you are active on Xen on ARM right?

Thanks,

Ciao

> 
> 
>>
>>>      }
>>> -#endif
>>> +#endif /* CONFIG_TCG */
>>> +#endif /* !CONFIG_USER_ONLY */
>>>  
>>>      cpu_exec_realizefn(cs, &local_err);
>>>      if (local_err != NULL) {
>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>> index 666ef329ef..13d7c6d930 100644
>>> --- a/target/arm/machine.c
>>> +++ b/target/arm/machine.c
>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>> +#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 */
>>>          {
>>>              .name = "power_state",
>>>              .version_id = 0,
>>>
>>
> 

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Am Sun, 21 Feb 2021 14:59:38 +0100
schrieb Claudio Fontana <cfontana@suse.de>:

> On 2/21/21 10:55 AM, Philippe Mathieu-Daudé wrote:
> > On 2/21/21 10:24 AM, Claudio Fontana wrote:  

> >> @@ -1347,7 +1348,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);  
> > What about Xen?  
> Good question, what about it..
> Ccing also Olaf.


Yes, what about Xen?

Does Xen on ARM use these code paths? I do not have the answer for this question.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/22/21 4:34 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> KVM has its own cpu->kvm_vtime.
>>
>> Adjust cpu vmstate by putting unused fields instead of the
>> VMSTATE_TIMER_PTR when TCG is not available.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/cpu.c     | 4 +++-
>>  target/arm/machine.c | 5 +++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>> index 1d81a1e7ac..b929109054 100644
>> --- a/target/arm/cpu.c
>> +++ b/target/arm/cpu.c
>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>          }
>>      }
>>  
>> +#ifdef CONFIG_TCG
>>      {
>>          uint64_t scale;
>>  
>> @@ -1347,7 +1348,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 */
>> +#endif /* !CONFIG_USER_ONLY */
>>  
>>      cpu_exec_realizefn(cs, &local_err);
>>      if (local_err != NULL) {
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 666ef329ef..13d7c6d930 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>> +#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 */
> 
> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
> just expose expired time but QEMUTimer has more in it than that. Paolo


I am not sure I follow can you state more precisely where the issue could be?

it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
it ends up in VMSTATE_POINTER where a single pointer is assigned;

so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
need to ensure that an unused number is there to assign, migrating from old to new version?


> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
> be better to have a VMSTATE_UNUSED_TIMER?
> 
> I don't think there is an impact for Xen because I'm fairly certain
> migration isn't a thing we do - but I'll double check.
> 

Thanks Alex, that would be helpful,
if Xen uses gt_timer in any way I would not want to unwillingly break it.

Thanks,

Claudio

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 2/22/21 4:34 PM, Alex Bennée wrote:
>> 
>> Claudio Fontana <cfontana@suse.de> writes:
>> 
>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>
>>> KVM has its own cpu->kvm_vtime.
>>>
>>> Adjust cpu vmstate by putting unused fields instead of the
>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>  target/arm/cpu.c     | 4 +++-
>>>  target/arm/machine.c | 5 +++++
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index 1d81a1e7ac..b929109054 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>          }
>>>      }
>>>  
>>> +#ifdef CONFIG_TCG
>>>      {
>>>          uint64_t scale;
>>>  
>>> @@ -1347,7 +1348,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 */
>>> +#endif /* !CONFIG_USER_ONLY */
>>>  
>>>      cpu_exec_realizefn(cs, &local_err);
>>>      if (local_err != NULL) {
>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>> index 666ef329ef..13d7c6d930 100644
>>> --- a/target/arm/machine.c
>>> +++ b/target/arm/machine.c
>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>> +#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 */
>> 
>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>> just expose expired time but QEMUTimer has more in it than that. Paolo
>
>
> I am not sure I follow can you state more precisely where the issue could be?
>
> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
> it ends up in VMSTATE_POINTER where a single pointer is assigned;

Does it? I thought it ended up with the .expire_time (int64_t) which
will be bigger than sizeof(QemuTimer *) on a 32 bit system.

>
> so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
> need to ensure that an unused number is there to assign, migrating from old to new version?
>
>
>> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
>> be better to have a VMSTATE_UNUSED_TIMER?
>> 
>> I don't think there is an impact for Xen because I'm fairly certain
>> migration isn't a thing we do - but I'll double check.
>> 
>
> Thanks Alex, that would be helpful,
> if Xen uses gt_timer in any way I would not want to unwillingly break
> it.

Not for ARM no, currently there is no ARM specific machine emulated by
QEMU for Xen. All ARM guests are PV guests.

>
> Thanks,
>
> Claudio


-- 
Alex Bennée

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/23/21 12:01 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 2/22/21 4:34 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>>
>>>> KVM has its own cpu->kvm_vtime.
>>>>
>>>> Adjust cpu vmstate by putting unused fields instead of the
>>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>>>  target/arm/cpu.c     | 4 +++-
>>>>  target/arm/machine.c | 5 +++++
>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>> index 1d81a1e7ac..b929109054 100644
>>>> --- a/target/arm/cpu.c
>>>> +++ b/target/arm/cpu.c
>>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>          }
>>>>      }
>>>>  
>>>> +#ifdef CONFIG_TCG
>>>>      {
>>>>          uint64_t scale;
>>>>  
>>>> @@ -1347,7 +1348,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 */
>>>> +#endif /* !CONFIG_USER_ONLY */
>>>>  
>>>>      cpu_exec_realizefn(cs, &local_err);
>>>>      if (local_err != NULL) {
>>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>>> index 666ef329ef..13d7c6d930 100644
>>>> --- a/target/arm/machine.c
>>>> +++ b/target/arm/machine.c
>>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>>> +#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 */
>>>
>>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>>> just expose expired time but QEMUTimer has more in it than that. Paolo
>>
>>
>> I am not sure I follow can you state more precisely where the issue could be?
>>
>> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
>> it ends up in VMSTATE_POINTER where a single pointer is assigned;
> 
> Does it? I thought it ended up with the .expire_time (int64_t) which
> will be bigger than sizeof(QemuTimer *) on a 32 bit system.

Ok I understand what you mean. Lets see:

Looking at vmstate.h,

#define VMSTATE_TIMER_PTR(_f, _s)                                         \
    VMSTATE_TIMER_PTR_V(_f, _s, 0)

#define VMSTATE_TIMER_PTR_V(_f, _s, _v)                                   \
    VMSTATE_POINTER(_f, _s, _v, vmstate_info_timer, QEMUTimer *)

#define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
    .name       = (stringify(_field)),                               \
    .version_id = (_version),                                        \
    .info       = &(_info),                                          \
    .size       = sizeof(_type),                                     \
    .flags      = VMS_SINGLE|VMS_POINTER,                            \
    .offset     = vmstate_offset_value(_state, _field, _type),       \
}

so here we get the vmstate field definition.

.size is fine, as it is sizeof(QEMUTimer *).

.info, is &vmstate_info_timer, migration/savevm.c:

const VMStateInfo vmstate_info_timer = {
    .name = "timer",
    .get  = get_timer,
    .put  = put_timer,
};

void timer_put(QEMUFile *f, QEMUTimer *ts)
{
    uint64_t expire_time;

    expire_time = timer_expire_time_ns(ts);
    qemu_put_be64(f, expire_time);
}

void timer_get(QEMUFile *f, QEMUTimer *ts)
{
    uint64_t expire_time;

    expire_time = qemu_get_be64(f);
    if (expire_time != -1) {
        timer_mod_ns(ts, expire_time);
    } else {
        timer_del(ts);
    }
}

---

And the migration code does: (migration/vmstate.c):

int vmstate_save_state_v() {
  ...
  ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
  ...
}

which puts a BE64 in the QEMUFile *f (see timer_put above).

The load code in the same file does:

int vmstate_load_state() {
  ...
  ret = field->info->get(f, curr_elem, size, field);
  ...
}

which reads a BE64 from the QEMUFile *f (see timer_get above).

Would be "fine" from the field sizes perspective (the .size of the field type, and the value of the BE64),

but it's the calculations done in timer_get and timer_put which are scary, as they dereference the timer pointer.


Should we actually have a check for null pointer in vmstate.c?

We _do_ have one in vmstate_save_state_v and vmstate_load_state, but it is actually active only for VMS_ARRAY_OF_POINTER.
Why? Why not also do the same (write the null pointer and not following it) for normal VMS_POINTER ?

int vmstate_save_state_v() {
 ...
                if (!curr_elem && size) {
                    /* if null pointer write placeholder and do not follow */
                    assert(field->flags & VMS_ARRAY_OF_POINTER);
                    ret = vmstate_info_nullptr.put(f, curr_elem, size, NULL,
                                                   NULL);

 ...


int vmstate_load_state() {

...
                if (!curr_elem && size) {
                    /* if null pointer check placeholder and do not follow */
                    assert(field->flags & VMS_ARRAY_OF_POINTER);
                    ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
...

}


This is worthwhile investigating further, any other ideas?

Thanks,

Claudio




















> 
>>
>> so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
>> need to ensure that an unused number is there to assign, migrating from old to new version?
>>
>>
>>> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
>>> be better to have a VMSTATE_UNUSED_TIMER?
>>>
>>> I don't think there is an impact for Xen because I'm fairly certain
>>> migration isn't a thing we do - but I'll double check.
>>>
>>
>> Thanks Alex, that would be helpful,
>> if Xen uses gt_timer in any way I would not want to unwillingly break
>> it.
> 
> Not for ARM no, currently there is no ARM specific machine emulated by
> QEMU for Xen. All ARM guests are PV guests.
> 
>>
>> Thanks,
>>
>> Claudio
> 
> 

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Claudio Fontana]

On 2/23/21 12:36 PM, Claudio Fontana wrote:
> On 2/23/21 12:01 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> On 2/22/21 4:34 PM, Alex Bennée wrote:
>>>>
>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>
>>>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>>>
>>>>> KVM has its own cpu->kvm_vtime.
>>>>>
>>>>> Adjust cpu vmstate by putting unused fields instead of the
>>>>> VMSTATE_TIMER_PTR when TCG is not available.
>>>>>
>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>> ---
>>>>>  target/arm/cpu.c     | 4 +++-
>>>>>  target/arm/machine.c | 5 +++++
>>>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>>>> index 1d81a1e7ac..b929109054 100644
>>>>> --- a/target/arm/cpu.c
>>>>> +++ b/target/arm/cpu.c
>>>>> @@ -1322,6 +1322,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>>>>>          }
>>>>>      }
>>>>>  
>>>>> +#ifdef CONFIG_TCG
>>>>>      {
>>>>>          uint64_t scale;
>>>>>  
>>>>> @@ -1347,7 +1348,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 */
>>>>> +#endif /* !CONFIG_USER_ONLY */
>>>>>  
>>>>>      cpu_exec_realizefn(cs, &local_err);
>>>>>      if (local_err != NULL) {
>>>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>>>> index 666ef329ef..13d7c6d930 100644
>>>>> --- a/target/arm/machine.c
>>>>> +++ b/target/arm/machine.c
>>>>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>>>>          VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>>>>          VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>>>>          VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>>>>> +#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 */
>>>>
>>>> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
>>>> just expose expired time but QEMUTimer has more in it than that. Paolo
>>>
>>>
>>> I am not sure I follow can you state more precisely where the issue could be?
>>>
>>> it's not a VMSTATE_TIMER, it's a VMSTATE_TIMER_PTR,
>>> it ends up in VMSTATE_POINTER where a single pointer is assigned;
>>
>> Does it? I thought it ended up with the .expire_time (int64_t) which
>> will be bigger than sizeof(QemuTimer *) on a 32 bit system.
> 
> Ok I understand what you mean. Lets see:
> 
> Looking at vmstate.h,
> 
> #define VMSTATE_TIMER_PTR(_f, _s)                                         \
>     VMSTATE_TIMER_PTR_V(_f, _s, 0)
> 
> #define VMSTATE_TIMER_PTR_V(_f, _s, _v)                                   \
>     VMSTATE_POINTER(_f, _s, _v, vmstate_info_timer, QEMUTimer *)
> 
> #define VMSTATE_POINTER(_field, _state, _version, _info, _type) {    \
>     .name       = (stringify(_field)),                               \
>     .version_id = (_version),                                        \
>     .info       = &(_info),                                          \
>     .size       = sizeof(_type),                                     \
>     .flags      = VMS_SINGLE|VMS_POINTER,                            \
>     .offset     = vmstate_offset_value(_state, _field, _type),       \
> }
> 
> so here we get the vmstate field definition.
> 
> .size is fine, as it is sizeof(QEMUTimer *).
> 
> .info, is &vmstate_info_timer, migration/savevm.c:
> 
> const VMStateInfo vmstate_info_timer = {
>     .name = "timer",
>     .get  = get_timer,
>     .put  = put_timer,
> };
> 
> void timer_put(QEMUFile *f, QEMUTimer *ts)
> {
>     uint64_t expire_time;
> 
>     expire_time = timer_expire_time_ns(ts);
>     qemu_put_be64(f, expire_time);
> }
> 
> void timer_get(QEMUFile *f, QEMUTimer *ts)
> {
>     uint64_t expire_time;
> 
>     expire_time = qemu_get_be64(f);
>     if (expire_time != -1) {
>         timer_mod_ns(ts, expire_time);
>     } else {
>         timer_del(ts);
>     }
> }
> 
> ---
> 
> And the migration code does: (migration/vmstate.c):
> 
> int vmstate_save_state_v() {
>   ...
>   ret = field->info->put(f, curr_elem, size, field, vmdesc_loop);
>   ...
> }
> 
> which puts a BE64 in the QEMUFile *f (see timer_put above).
> 
> The load code in the same file does:
> 
> int vmstate_load_state() {
>   ...
>   ret = field->info->get(f, curr_elem, size, field);
>   ...
> }
> 
> which reads a BE64 from the QEMUFile *f (see timer_get above).
> 
> Would be "fine" from the field sizes perspective (the .size of the field type, and the value of the BE64),
> 
> but it's the calculations done in timer_get and timer_put which are scary, as they dereference the timer pointer.
> 
> 
> Should we actually have a check for null pointer in vmstate.c?
> 
> We _do_ have one in vmstate_save_state_v and vmstate_load_state, but it is actually active only for VMS_ARRAY_OF_POINTER.
> Why? Why not also do the same (write the null pointer and not following it) for normal VMS_POINTER ?
> 
> int vmstate_save_state_v() {
>  ...
>                 if (!curr_elem && size) {
>                     /* if null pointer write placeholder and do not follow */
>                     assert(field->flags & VMS_ARRAY_OF_POINTER);
>                     ret = vmstate_info_nullptr.put(f, curr_elem, size, NULL,
>                                                    NULL);
> 
>  ...
> 
> 
> int vmstate_load_state() {
> 
> ...
>                 if (!curr_elem && size) {
>                     /* if null pointer check placeholder and do not follow */
>                     assert(field->flags & VMS_ARRAY_OF_POINTER);
>                     ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
> ...
> 
> }
> 
> 
> This is worthwhile investigating further, any other ideas?
> 
> Thanks,
> 
> Claudio
> 
> 

Btw here it would be good to be able to rely on the existing tests,
do we have full coverage of these compatibility situations?

According to make check it's all a-ok, but... is the testing coverage insufficient
for these VMSTATE compatibility issues?

Ciao,

Claudio


>>
>>>
>>> so if we don't use gt_timer at all (as is the case with !CONFIG_TCG), we just
>>> need to ensure that an unused number is there to assign, migrating from old to new version?
>>>
>>>
>>>> suggested a straight VMSTATE_UNUSED(8) on IRC but I wonder if it would
>>>> be better to have a VMSTATE_UNUSED_TIMER?
>>>>
>>>> I don't think there is an impact for Xen because I'm fairly certain
>>>> migration isn't a thing we do - but I'll double check.
>>>>
>>>
>>> Thanks Alex, that would be helpful,
>>> if Xen uses gt_timer in any way I would not want to unwillingly break
>>> it.
>>
>> Not for ARM no, currently there is no ARM specific machine emulated by
>> QEMU for Xen. All ARM guests are PV guests.
>>
>>>
>>> Thanks,
>>>
>>> Claudio
>>
>>
> 

Re: [RFC v1 34/38] target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG

[Paolo Bonzini]

On 23/02/21 12:01, Alex Bennée wrote:
>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>> index 666ef329ef..13d7c6d930 100644
>> --- a/target/arm/machine.c
>> +++ b/target/arm/machine.c
>> @@ -822,8 +822,13 @@ const VMStateDescription vmstate_arm_cpu = {
>>           VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
>>           VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>>           VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>> +#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 */
> I'm not sure this is correct - VMSTATE_TIMER_PTR chases the links to
> just expose expired time but QEMUTimer has more in it than that. Paolo

If the timer is never set, it is completely free to create it with 
timer_new.  So it seems simpler to do nothing here.

The observation about the null pointer makes sense, but I think it would 
break existing migration streams.  Also we would like to convert all 
QEMUTimer* to embedded QEMUTimers, so my advice is to not bother adding 
it, instead of figuring out how to solve those problems.

Paolo

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
>   target/arm: move psci.c into tcg/softmmu/

Terminology: the opposite of user-only is not "softmmu" but "system".

One glorious day in the far future user-only will, as an option, use softmmu.
It will fix all sorts of problems with alignment faults and host/guest virtual
address space mismatch.


r~

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/22/21 6:35 AM, Richard Henderson wrote:
> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>>   target/arm: move psci.c into tcg/softmmu/
> 
> Terminology: the opposite of user-only is not "softmmu" but "system".
> 
> One glorious day in the far future user-only will, as an option, use softmmu.
> It will fix all sorts of problems with alignment faults and host/guest virtual
> address space mismatch.
> 
> 
> r~
> 

Hi Richard,

first thanks for all the useful pointers in the series.

Regarding terminology, I think the mismatch is throughout the code right?

So many of the existing "softmmu" files and directories should actually be called "system",
or "sysemu" to match include/sysemu right?

To me these distinctions are a bit unclear.
Maybe it would be good to have clear documentation about this in devel/ to use as a reference and end-goal,

and then we could do a pass of the whole code to fix the discrepancies in the use of the terms?

Thanks,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 2/22/21 9:43 AM, Claudio Fontana wrote:
> On 2/22/21 6:35 AM, Richard Henderson wrote:
>> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>>>   target/arm: move psci.c into tcg/softmmu/
>>
>> Terminology: the opposite of user-only is not "softmmu" but "system".
>>
>> One glorious day in the far future user-only will, as an option, use softmmu.
>> It will fix all sorts of problems with alignment faults and host/guest virtual
>> address space mismatch.
>>
>>
>> r~
>>
> 
> Hi Richard,
> 
> first thanks for all the useful pointers in the series.
> 
> Regarding terminology, I think the mismatch is throughout the code right?
> 
> So many of the existing "softmmu" files and directories should actually be called "system",
> or "sysemu" to match include/sysemu right?

My understanding is there are currently 2 types of emulation:

- user mode (emulation of the kernel syscall API)

  -> Linux, BSD
  process is in unprivileged memory space, no access to I/O

- system mode (full machine emulated)

  - MMU implementation in software: softmmu

In that separation semihosting is borderline.

Also FYI I have a wip branch where I am prototyping a "supervisor
mode" emulation to run Cisco IOS images. These images run in
supervised mode with access to I/O space, but not access to protected
kernel space (where the firmware resides). Unlikely to hit upstream,
but fun experiment ;)

> To me these distinctions are a bit unclear.
> Maybe it would be good to have clear documentation about this in devel/ to use as a reference and end-goal,
> 
> and then we could do a pass of the whole code to fix the discrepancies in the use of the terms?
> 
> Thanks,
> 
> Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Richard Henderson]

On 2/22/21 12:43 AM, Claudio Fontana wrote:
> Regarding terminology, I think the mismatch is throughout the code right?

Yes, e.g. the top-level softmmu/.


> So many of the existing "softmmu" files and directories should actually be
> called "system", or "sysemu" to match include/sysemu right?
Yes, please.  Let's not add new mistakes.

> Maybe it would be good to have clear documentation about this in devel/ to
> use as a reference and end-goal, and then we could do a pass of the whole
> code to fix the discrepancies in the use of the terms?
I suppose.  Where do you suggest adding those couple of sentences?


r~

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 2/22/21 12:43 AM, Claudio Fontana wrote:
>> Regarding terminology, I think the mismatch is throughout the code right?
>
> Yes, e.g. the top-level softmmu/.
>
>
>> So many of the existing "softmmu" files and directories should actually be
>> called "system", or "sysemu" to match include/sysemu right?
> Yes, please.  Let's not add new mistakes.
>
>> Maybe it would be good to have clear documentation about this in devel/ to
>> use as a reference and end-goal, and then we could do a pass of the whole
>> code to fix the discrepancies in the use of the terms?
> I suppose.  Where do you suggest adding those couple of sentences?

I guess if we are formalising the build structure either in
docs/devel/build-system.rst or maybe CODING_STYLE.rst (which really
should be folded into the rest of the documents).

>
>
> r~


-- 
Alex Bennée

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/22/21 5:08 PM, Richard Henderson wrote:
> On 2/22/21 12:43 AM, Claudio Fontana wrote:
>> Regarding terminology, I think the mismatch is throughout the code right?
> 
> Yes, e.g. the top-level softmmu/.
> 
> 
>> So many of the existing "softmmu" files and directories should actually be
>> called "system", or "sysemu" to match include/sysemu right?
> Yes, please.  Let's not add new mistakes.
> 
>> Maybe it would be good to have clear documentation about this in devel/ to
>> use as a reference and end-goal, and then we could do a pass of the whole
>> code to fix the discrepancies in the use of the terms?
> I suppose.  Where do you suggest adding those couple of sentences?
> 
> 
> r~
> 

I'd say docs/devel/code-config-macros, or something like that.

I think for a new developer coming into the codebase, the big hurdle is trying to figure out why there are so many CONFIG_,

in which files and directories they can and should be used,

which effects they have and the most dangerous pitfalls (f.e. header files and mixed common/specific modules), ...

Ciao,

Claudio

Re: [RFC v1 20/38] target/arm: move arm_hcr_el2_eff to common_cpu

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> extract it from the tcg helpers, as functionality is needed
> for KVM too.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu-common.c | 68 +++++++++++++++++++++++++++++++++++++++++
>  target/arm/tcg/helper.c | 68 -----------------------------------------
>  2 files changed, 68 insertions(+), 68 deletions(-)

Hmm, not really.  The hypervisor (the linux kernel for kvm) uses EL2 itself,
and only exposes EL1 and EL0 to the virtualized guest.  Due to how
virtualization works on ARM, the same will be true for all hypervisors.

Thus, ARM_FEATURE_EL2 will always be unset, and arm_is_el2_enabled will always
be false.

I think you want a stub for !tcg like

/* EL2 is used by the hypervisor and never enabled for the guest. */
uint64_t arm_hcr_el2_eff(CPUARMState *env) { return 0; }


r~

Re: [RFC v1 23/38] target/arm: move arm_mmu_idx_el to common-cpu

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu-common.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  target/arm/tcg/helper.c | 51 -----------------------------------------
>  2 files changed, 51 insertions(+), 51 deletions(-)

How does this escape from tcg code?  I think this is indicative of something
else escaping first.


r~

Re: [RFC v1 24/38] target/arm: move aa64_va_parameter_tbi,tbid,tcma and arm_rebuild_hflags

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> they are needed for KVM too, move them out of TCG helpers.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/internals.h      |  37 +++++
>  target/arm/tcg/helper-tcg.h |  32 -----
>  target/arm/cpu-common.c     | 252 ++++++++++++++++++++++++++++++++++
>  target/arm/tcg/helper.c     | 264 +-----------------------------------
>  4 files changed, 293 insertions(+), 292 deletions(-)
> 
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 6589b63ebc..9eb5d7fd79 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1196,6 +1196,43 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>      return ptr;
>  }
>  
> +/*
> + * Convert a possible stage1+2 MMU index into the appropriate
> + * stage 1 MMU index
> + */
> +static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
> +{
> +    switch (mmu_idx) {
> +    case ARMMMUIdx_SE10_0:
> +        return ARMMMUIdx_Stage1_SE0;
> +    case ARMMMUIdx_SE10_1:
> +        return ARMMMUIdx_Stage1_SE1;
> +    case ARMMMUIdx_SE10_1_PAN:
> +        return ARMMMUIdx_Stage1_SE1_PAN;
> +    case ARMMMUIdx_E10_0:
> +        return ARMMMUIdx_Stage1_E0;
> +    case ARMMMUIdx_E10_1:
> +        return ARMMMUIdx_Stage1_E1;
> +    case ARMMMUIdx_E10_1_PAN:
> +        return ARMMMUIdx_Stage1_E1_PAN;
> +    default:
> +        return mmu_idx;
> +    }
> +}
> +
> +int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx);
> +int aa64_va_parameter_tbid(uint64_t tcr, ARMMMUIdx mmu_idx);

I can see these being needed for get-phys-addr -- and that probably answers my
arm_mmu_idx_el question earlier too.


> +uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
> +                            ARMMMUIdx mmu_idx);
> +uint32_t rebuild_hflags_a32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
> +uint32_t rebuild_hflags_m32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);

However these really really shouldn't be used for !tcg.  I would even wrap
CPUARMState::hflags in #ifdef CONFIG_TCG to enforce that.

I think maybe the best option is

    if (tcg_enabled()) {
        rebuild_hflags();
    }

so that we don't spend the time on the rebuild for a regular build that has
both tcg and kvm enabled, and the symbol gets
compiled out when tcg is disabled.


r~

Re: [RFC v1 24/38] target/arm: move aa64_va_parameter_tbi,tbid,tcma and arm_rebuild_hflags

[Claudio Fontana]

On 2/22/21 7:02 AM, Richard Henderson wrote:
> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> they are needed for KVM too, move them out of TCG helpers.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/internals.h      |  37 +++++
>>  target/arm/tcg/helper-tcg.h |  32 -----
>>  target/arm/cpu-common.c     | 252 ++++++++++++++++++++++++++++++++++
>>  target/arm/tcg/helper.c     | 264 +-----------------------------------
>>  4 files changed, 293 insertions(+), 292 deletions(-)
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 6589b63ebc..9eb5d7fd79 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1196,6 +1196,43 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>      return ptr;
>>  }
>>  
>> +/*
>> + * Convert a possible stage1+2 MMU index into the appropriate
>> + * stage 1 MMU index
>> + */
>> +static inline ARMMMUIdx stage_1_mmu_idx(ARMMMUIdx mmu_idx)
>> +{
>> +    switch (mmu_idx) {
>> +    case ARMMMUIdx_SE10_0:
>> +        return ARMMMUIdx_Stage1_SE0;
>> +    case ARMMMUIdx_SE10_1:
>> +        return ARMMMUIdx_Stage1_SE1;
>> +    case ARMMMUIdx_SE10_1_PAN:
>> +        return ARMMMUIdx_Stage1_SE1_PAN;
>> +    case ARMMMUIdx_E10_0:
>> +        return ARMMMUIdx_Stage1_E0;
>> +    case ARMMMUIdx_E10_1:
>> +        return ARMMMUIdx_Stage1_E1;
>> +    case ARMMMUIdx_E10_1_PAN:
>> +        return ARMMMUIdx_Stage1_E1_PAN;
>> +    default:
>> +        return mmu_idx;
>> +    }
>> +}
>> +
>> +int aa64_va_parameter_tbi(uint64_t tcr, ARMMMUIdx mmu_idx);
>> +int aa64_va_parameter_tbid(uint64_t tcr, ARMMMUIdx mmu_idx);
> 
> I can see these being needed for get-phys-addr -- and that probably answers my
> arm_mmu_idx_el question earlier too.
> 
> 
>> +uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
>> +                            ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_a32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
>> +uint32_t rebuild_hflags_m32(CPUARMState *env, int fp_el, ARMMMUIdx mmu_idx);
> 
> However these really really shouldn't be used for !tcg.  I would even wrap
> CPUARMState::hflags in #ifdef CONFIG_TCG to enforce that.
> 
> I think maybe the best option is
> 
>     if (tcg_enabled()) {
>         rebuild_hflags();
>     }
> 
> so that we don't spend the time on the rebuild for a regular build that has
> both tcg and kvm enabled, and the symbol gets
> compiled out when tcg is disabled.

is the code elimination for "if (0)" a guarantee, ie, we won't encounter compiler or compiler-options differences,
for the compilers we support?

This is a doubt that is bothering me since some time, since I remember I encountered problems like this before,
with my compiler behaving differently than Paolo's in particular.

Is there some way to force the compilers to not even look at what is in the if (0) block?
That should work also with --enable-debug?

This way we could avoid a lot of boilerplate/stubs...

Ciao,

Claudio


> 
> 
> r~
> 

Re: [RFC v1 24/38] target/arm: move aa64_va_parameter_tbi,tbid,tcma and arm_rebuild_hflags

[Richard Henderson]

On 2/23/21 2:07 AM, Claudio Fontana wrote:
> is the code elimination for "if (0)" a guarantee, ie, we won't encounter
> compiler or compiler-options differences, for the compilers we support?
Yes, it's a guarantee.

> Is there some way to force the compilers to not even look at what is in the
> if (0) block?

No, it must be syntactically correct, and it must not reference variables that
have not been declared.  But that's a feature -- making sure that nothing has
syntax errors in is a major improvement over ifdefs.

> That should work also with --enable-debug?

Yes.

> This way we could avoid a lot of boilerplate/stubs...

Eh, maybe, maybe not.  Stubs may still be required, depending on how complex
the condition is -- more than if (0).


r~

Re: [RFC v1 25/38] target/arm: move fp_exception_el outside of tcg helpers

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> it is needed also for KVM.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu-softmmu.c        | 95 +++++++++++++++++++++++++++++++++
>  target/arm/cpu-user.c           |  9 ++++
>  target/arm/tcg/softmmu/helper.c | 92 -------------------------------
>  target/arm/tcg/user/helper.c    |  8 ---
>  4 files changed, 104 insertions(+), 100 deletions(-)

This is related to rebuild_hflags, I assume.
Fix that and this isn't needed.


r~

Re: [RFC v1 26/38] target/arm: move sve_exception_el to cpu

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> it is needed for KVM too, move away from tcg helpers.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/cpu-softmmu.c        | 60 +++++++++++++++++++++++++++++++++
>  target/arm/cpu-user.c           |  5 +++
>  target/arm/tcg/softmmu/helper.c | 59 --------------------------------
>  target/arm/tcg/user/helper.c    |  5 ---
>  4 files changed, 65 insertions(+), 64 deletions(-)

This is related to rebuild_hflags, I assume.
Fix that and this isn't needed.


r~

Re: [RFC v1 27/38] target/arm: move sve_zcr_len_for_el to common_cpu

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> it is needed 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(-)

This is related to rebuild_hflags, I assume.
Fix that and this isn't needed.


r~

Re: [RFC v1 27/38] target/arm: move sve_zcr_len_for_el to common_cpu

[Claudio Fontana]

On 2/22/21 7:06 AM, Richard Henderson wrote:
> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> it is needed 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(-)
> 
> This is related to rebuild_hflags, I assume.
> Fix that and this isn't needed.
> 
> 
> r~
> 

This stuff comes in also from the dump state code, 

if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
    int j, zcr_len = sve_zcr_len_for_el(env, el);
...
}

will take another look..

Re: [RFC v1 27/38] target/arm: move sve_zcr_len_for_el to common_cpu

[Claudio Fontana]

On 2/25/21 6:28 PM, Claudio Fontana wrote:
> On 2/22/21 7:06 AM, Richard Henderson wrote:
>> On 2/21/21 1:24 AM, Claudio Fontana wrote:
>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>
>>> it is needed 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(-)
>>
>> This is related to rebuild_hflags, I assume.
>> Fix that and this isn't needed.
>>
>>
>> r~
>>
> 
> This stuff comes in also from the dump state code, 
> 
> if (cpu_isar_feature(aa64_sve, cpu) && sve_exception_el(env, el) == 0) {
>     int j, zcr_len = sve_zcr_len_for_el(env, el);
> ...
> }
> 
> will take another look..
> 

that was in cpu.c.

sve_zcr_len_for_el is also called in arch_dump.c in aarch64_write_el64_sve, via sve_current_vq().

Wonder if a stub is needed, or we need the whole implementation..

thanks,

CLaudio

Re: [RFC v1 27/38] target/arm: move sve_zcr_len_for_el to common_cpu

[Richard Henderson]

On 2/25/21 12:13 PM, Claudio Fontana wrote:
> sve_zcr_len_for_el is also called in arch_dump.c in aarch64_write_el64_sve, via sve_current_vq().
> 
> Wonder if a stub is needed, or we need the whole implementation..

For sve_zcr_len_for_el, I'd use the whole thing.  The dump seems to be callable
for any cpu state.


r~

Re: [RFC v1 28/38] target/arm: make arm_pmu_timer_cb TCG-only, starting tcg-stub

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> @@ -634,10 +635,11 @@ static int cpu_pre_save(void *opaque)
>  {
>      ARMCPU *cpu = opaque;
>  
> -    if (!kvm_enabled()) {
> +#ifdef CONFIG_TCG
> +    if (tcg_enabled()) {
>          pmu_op_start(&cpu->env);
>      }
> -
> +#endif /* CONFIG_TCG */

Why the ifdef?  Seems like a lack of stub.
I agree that !kvm rather than tcg is a bug.


r~

Re: [RFC v1 01/38] target/arm: move translate modules to tcg/

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/{ => tcg}/translate-a64.h      |  0
>  target/arm/{ => tcg}/translate.h          |  0
>  target/arm/{ => tcg}/a32-uncond.decode    |  0
>  target/arm/{ => tcg}/a32.decode           |  0
>  target/arm/{ => tcg}/m-nocp.decode        |  0
>  target/arm/{ => tcg}/neon-dp.decode       |  0
>  target/arm/{ => tcg}/neon-ls.decode       |  0
>  target/arm/{ => tcg}/neon-shared.decode   |  0
>  target/arm/{ => tcg}/sve.decode           |  0
>  target/arm/{ => tcg}/t16.decode           |  0
>  target/arm/{ => tcg}/t32.decode           |  0
>  target/arm/{ => tcg}/vfp-uncond.decode    |  0
>  target/arm/{ => tcg}/vfp.decode           |  0
>  target/arm/{ => tcg}/translate-a64.c      |  0
>  target/arm/{ => tcg}/translate-sve.c      |  0
>  target/arm/{ => tcg}/translate.c          |  0
>  target/arm/{ => tcg}/translate-neon.c.inc |  0
>  target/arm/{ => tcg}/translate-vfp.c.inc  |  0
>  target/arm/meson.build                    | 20 ++-----------------
>  target/arm/tcg/meson.build                | 24 +++++++++++++++++++++++
>  20 files changed, 26 insertions(+), 18 deletions(-)
>  rename target/arm/{ => tcg}/translate-a64.h (100%)
>  rename target/arm/{ => tcg}/translate.h (100%)
>  rename target/arm/{ => tcg}/a32-uncond.decode (100%)
>  rename target/arm/{ => tcg}/a32.decode (100%)
>  rename target/arm/{ => tcg}/m-nocp.decode (100%)
>  rename target/arm/{ => tcg}/neon-dp.decode (100%)
>  rename target/arm/{ => tcg}/neon-ls.decode (100%)
>  rename target/arm/{ => tcg}/neon-shared.decode (100%)
>  rename target/arm/{ => tcg}/sve.decode (100%)
>  rename target/arm/{ => tcg}/t16.decode (100%)
>  rename target/arm/{ => tcg}/t32.decode (100%)
>  rename target/arm/{ => tcg}/vfp-uncond.decode (100%)
>  rename target/arm/{ => tcg}/vfp.decode (100%)
>  rename target/arm/{ => tcg}/translate-a64.c (100%)
>  rename target/arm/{ => tcg}/translate-sve.c (100%)
>  rename target/arm/{ => tcg}/translate.c (100%)
>  rename target/arm/{ => tcg}/translate-neon.c.inc (100%)
>  rename target/arm/{ => tcg}/translate-vfp.c.inc (100%)
>  create mode 100644 target/arm/tcg/meson.build

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

r~

Re: [RFC v1 03/38] arm: tcg: only build under CONFIG_TCG

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/tcg/meson.build | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

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

r~

Re: [RFC v1 05/38] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Richard Henderson]

On 2/21/21 1:24 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 */

I presume this function gets moved later?  What does this aid in the interim?


r~

Re: [RFC v1 05/38] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Claudio Fontana]

On 2/22/21 7:21 AM, Richard Henderson wrote:
> On 2/21/21 1:24 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 */
> 
> I presume this function gets moved later?  What does this aid in the interim?
> 
> 
> r~
> 

Hi Richard,

actually this is a fix for an error I introduced when adding TCGOps:

arm_cpu_exec_interrupt should be wrapped in the ifdef, as it uses tcg_ops, which is TCG-only.
Maybe I should extract this and make it a standalone fix.

Currently, there is no real issue because the non-TCG build is not working for ARM anyway,
and that's why the issue went undetected.

Thanks!

Claudio

Re: [RFC v1 05/38] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Richard Henderson]

On 2/22/21 12:31 AM, Claudio Fontana wrote:
> actually this is a fix for an error I introduced when adding TCGOps:
> 
> arm_cpu_exec_interrupt should be wrapped in the ifdef, as it uses tcg_ops, which is TCG-only.
> Maybe I should extract this and make it a standalone fix.
> 
> Currently, there is no real issue because the non-TCG build is not working for ARM anyway,
> and that's why the issue went undetected.

If it doesn't cause a current problem, then let's not bother with a standalone
patch.  But I wouldn't bother with the ifdef either, but instead move the
function to an appropriate file.


r~

Re: [RFC v1 05/38] target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG

[Claudio Fontana]

On 2/22/21 4:54 PM, Richard Henderson wrote:
> On 2/22/21 12:31 AM, Claudio Fontana wrote:
>> actually this is a fix for an error I introduced when adding TCGOps:
>>
>> arm_cpu_exec_interrupt should be wrapped in the ifdef, as it uses tcg_ops, which is TCG-only.
>> Maybe I should extract this and make it a standalone fix.
>>
>> Currently, there is no real issue because the non-TCG build is not working for ARM anyway,
>> and that's why the issue went undetected.
> 
> If it doesn't cause a current problem, then let's not bother with a standalone
> patch.  But I wouldn't bother with the ifdef either, but instead move the
> function to an appropriate file.
> 
> 
> r~
> 

Yes, agree.

Ciao,

Claudio

Re: [RFC v1 08/38] target/arm/tcg: split softmmu parts of v8_cp_reginfo and el2_cp_reginfo

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
> 
> since ats_write64 is only used there, make it softmmu-only.
> 
> gt_cntvoff_write, gt_hyp_*_write and gt_hyp_timer_reset are only used
> in el2_cp_reginfo, so they can be softmmu-only too.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/arm/tcg/helper-tcg.h     |  16 ++--
>  target/arm/tcg/helper.c         | 129 +++-------------------------
>  target/arm/tcg/softmmu/helper.c | 145 +++++++++++++++++++++++++++++---
>  3 files changed, 149 insertions(+), 141 deletions(-)

Hum.  As long as we're moving code around, let's put all the cp_regs stuff in
its own file, rather than creating another massive file with random stuff in it.


r~

Re: [RFC v1 02/38] target/arm: move helpers to tcg/

[Richard Henderson]

On 2/21/21 1:24 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  meson.build                          |  1 +
>  target/arm/tcg/trace.h               |  1 +
>  target/arm/{ => tcg}/crypto_helper.c |  0
>  target/arm/{ => tcg}/debug_helper.c  |  0
>  target/arm/{ => tcg}/helper-a64.c    |  0
>  target/arm/{ => tcg}/helper.c        |  0
>  target/arm/{ => tcg}/iwmmxt_helper.c |  0
>  target/arm/{ => tcg}/m_helper.c      |  0
>  target/arm/{ => tcg}/mte_helper.c    |  0
>  target/arm/{ => tcg}/neon_helper.c   |  0
>  target/arm/{ => tcg}/op_helper.c     |  0
>  target/arm/{ => tcg}/pauth_helper.c  |  0
>  target/arm/{ => tcg}/sve_helper.c    |  0
>  target/arm/{ => tcg}/tlb_helper.c    |  0
>  target/arm/{ => tcg}/vec_helper.c    |  0
>  target/arm/{ => tcg}/vfp_helper.c    |  0
>  target/arm/meson.build               | 14 --------------
>  target/arm/tcg/meson.build           | 14 ++++++++++++++
>  target/arm/tcg/trace-events          | 10 ++++++++++
>  target/arm/trace-events              |  9 ---------
>  20 files changed, 26 insertions(+), 23 deletions(-)
>  create mode 100644 target/arm/tcg/trace.h
>  rename target/arm/{ => tcg}/crypto_helper.c (100%)
>  rename target/arm/{ => tcg}/debug_helper.c (100%)
>  rename target/arm/{ => tcg}/helper-a64.c (100%)
>  rename target/arm/{ => tcg}/helper.c (100%)
>  rename target/arm/{ => tcg}/iwmmxt_helper.c (100%)
>  rename target/arm/{ => tcg}/m_helper.c (100%)
>  rename target/arm/{ => tcg}/mte_helper.c (100%)
>  rename target/arm/{ => tcg}/neon_helper.c (100%)
>  rename target/arm/{ => tcg}/op_helper.c (100%)
>  rename target/arm/{ => tcg}/pauth_helper.c (100%)
>  rename target/arm/{ => tcg}/sve_helper.c (100%)
>  rename target/arm/{ => tcg}/tlb_helper.c (100%)
>  rename target/arm/{ => tcg}/vec_helper.c (100%)
>  rename target/arm/{ => tcg}/vfp_helper.c (100%)
>  create mode 100644 target/arm/tcg/trace-events

Can vec_internal.h be moved here too, rather than later?

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

r~

Re: [RFC v1 02/38] target/arm: move helpers to tcg/

[Philippe Mathieu-Daudé]

On 2/21/21 10:24 AM, Claudio Fontana wrote:
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  meson.build                          |  1 +
>  target/arm/tcg/trace.h               |  1 +
>  target/arm/{ => tcg}/crypto_helper.c |  0
>  target/arm/{ => tcg}/debug_helper.c  |  0
>  target/arm/{ => tcg}/helper-a64.c    |  0
>  target/arm/{ => tcg}/helper.c        |  0
>  target/arm/{ => tcg}/iwmmxt_helper.c |  0
>  target/arm/{ => tcg}/m_helper.c      |  0
>  target/arm/{ => tcg}/mte_helper.c    |  0
>  target/arm/{ => tcg}/neon_helper.c   |  0
>  target/arm/{ => tcg}/op_helper.c     |  0
>  target/arm/{ => tcg}/pauth_helper.c  |  0
>  target/arm/{ => tcg}/sve_helper.c    |  0
>  target/arm/{ => tcg}/tlb_helper.c    |  0
>  target/arm/{ => tcg}/vec_helper.c    |  0
>  target/arm/{ => tcg}/vfp_helper.c    |  0
>  target/arm/meson.build               | 14 --------------
>  target/arm/tcg/meson.build           | 14 ++++++++++++++
>  target/arm/tcg/trace-events          | 10 ++++++++++
>  target/arm/trace-events              |  9 ---------
>  20 files changed, 26 insertions(+), 23 deletions(-)
>  create mode 100644 target/arm/tcg/trace.h
>  rename target/arm/{ => tcg}/crypto_helper.c (100%)
>  rename target/arm/{ => tcg}/debug_helper.c (100%)
>  rename target/arm/{ => tcg}/helper-a64.c (100%)
>  rename target/arm/{ => tcg}/helper.c (100%)
>  rename target/arm/{ => tcg}/iwmmxt_helper.c (100%)
>  rename target/arm/{ => tcg}/m_helper.c (100%)
>  rename target/arm/{ => tcg}/mte_helper.c (100%)
>  rename target/arm/{ => tcg}/neon_helper.c (100%)
>  rename target/arm/{ => tcg}/op_helper.c (100%)
>  rename target/arm/{ => tcg}/pauth_helper.c (100%)
>  rename target/arm/{ => tcg}/sve_helper.c (100%)
>  rename target/arm/{ => tcg}/tlb_helper.c (100%)
>  rename target/arm/{ => tcg}/vec_helper.c (100%)
>  rename target/arm/{ => tcg}/vfp_helper.c (100%)
>  create mode 100644 target/arm/tcg/trace-events
> 
> diff --git a/meson.build b/meson.build
> index 5be4e5f38c..86b07f1a1a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1828,6 +1828,7 @@ if have_system or have_user
>      'accel/tcg',
>      'hw/core',
>      'target/arm',
> +    'target/arm/tcg',
>      'target/hppa',
>      'target/i386',
>      'target/i386/kvm',

Why add unconditionally and not such:

if config_all.has_key('CONFIG_TCG')
  trace_events_subdirs += ['target/arm/tcg']
endif
if config_all.has_key('CONFIG_KVM')
  trace_events_subdirs += ['target/i386/kvm']
endif

?

Re: [RFC v1 04/38] target/arm: move psci.c into tcg/softmmu/

[Claudio Fontana]

On 2/22/21 6:22 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/internals.h              | 23 ++++++++++-------------
>>  target/arm/tcg/helper.c             |  2 ++
>>  target/arm/{ => tcg/softmmu}/psci.c |  0
>>  target/arm/tcg/user/psci.c          | 26 ++++++++++++++++++++++++++
>>  target/arm/meson.build              |  1 -
>>  target/arm/tcg/meson.build          |  3 +++
>>  target/arm/tcg/softmmu/meson.build  |  4 ++++
>>  target/arm/tcg/user/meson.build     |  4 ++++
>>  8 files changed, 49 insertions(+), 14 deletions(-)
>>  rename target/arm/{ => tcg/softmmu}/psci.c (100%)
>>  create mode 100644 target/arm/tcg/user/psci.c
>>  create mode 100644 target/arm/tcg/softmmu/meson.build
>>  create mode 100644 target/arm/tcg/user/meson.build
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 05cebc8597..6384461177 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -292,21 +292,18 @@ 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'm not sure I'm a fan of pushing these #ifdef tweaks down into the main
> code when the compiler is good an eliding them away. I guess we need
> this because the helper.o wants to be a shared object between both user
> and softmmu/sysemu mode?


Hi Alex,

yes, but will try to clean this up, I agree that this is too much preprocessor stuff.

Ciao,

CLaudio

> 
>> -/* Return true if the r0/x0 value indicates that this SMC/HVC is a PSCI call. */
>> +#ifdef CONFIG_TCG
>> +/*
>> + * Return true only for softmmu, if the r0/x0 value indicates that this
>> + * SMC/HVC is a PSCI call.
>> + */
>>  bool arm_is_psci_call(ARMCPU *cpu, int excp_type);
>> -/* Actually handle a PSCI call */
>> +
>> +#ifndef CONFIG_USER_ONLY
>>  void arm_handle_psci_call(ARMCPU *cpu);
>> -#endif
>> +#endif /* !CONFIG_USER_ONLY */
>> +
>> +#endif /* CONFIG_TCG */
>>  
>>  /**
>>   * arm_clear_exclusive: clear the exclusive monitor
>> diff --git a/target/arm/tcg/helper.c b/target/arm/tcg/helper.c
>> index 0e1a3b9421..beb8a5deed 100644
>> --- a/target/arm/tcg/helper.c
>> +++ b/target/arm/tcg/helper.c
>> @@ -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 */
>>  
>>      /*
>>       * Semihosting semantics depend on the register width of the code
>> diff --git a/target/arm/psci.c b/target/arm/tcg/softmmu/psci.c
>> similarity index 100%
>> rename from target/arm/psci.c
>> rename to target/arm/tcg/softmmu/psci.c
>> diff --git a/target/arm/tcg/user/psci.c b/target/arm/tcg/user/psci.c
>> new file mode 100644
>> index 0000000000..f3e293c516
>> --- /dev/null
>> +++ b/target/arm/tcg/user/psci.c
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (C) 2014 - Linaro
>> + * Author: Rob Herring <rob.herring@linaro.org>
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published by
>> + *  the Free Software Foundation; either version 2 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "cpu.h"
>> +#include "internals.h"
>> +
>> +bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
>> +{
>> +    return false;
>> +}
>> diff --git a/target/arm/meson.build b/target/arm/meson.build
>> index 0172937b40..3d23a6c687 100644
>> --- a/target/arm/meson.build
>> +++ b/target/arm/meson.build
>> @@ -19,7 +19,6 @@ arm_softmmu_ss.add(files(
>>    'arm-powerctl.c',
>>    'machine.c',
>>    'monitor.c',
>> -  'psci.c',
>>  ))
>>  arm_user_ss = ss.source_set()
>>  
>> diff --git a/target/arm/tcg/meson.build b/target/arm/tcg/meson.build
>> index 3b4146d079..4d9ed4b9cf 100644
>> --- a/target/arm/tcg/meson.build
>> +++ b/target/arm/tcg/meson.build
>> @@ -36,3 +36,6 @@ arm_ss.add(when: ['TARGET_AARCH64','CONFIG_TCG'], if_true: files(
>>    'pauth_helper.c',
>>    'sve_helper.c',
>>  ))
>> +
>> +subdir('user')
>> +subdir('softmmu')
>> diff --git a/target/arm/tcg/softmmu/meson.build b/target/arm/tcg/softmmu/meson.build
>> new file mode 100644
>> index 0000000000..f136c8bb8b
>> --- /dev/null
>> +++ b/target/arm/tcg/softmmu/meson.build
>> @@ -0,0 +1,4 @@
>> +
>> +arm_softmmu_ss.add(when: ['CONFIG_TCG','CONFIG_SOFTMMU'], if_true: files(
>> +  'psci.c',
>> +))
>> diff --git a/target/arm/tcg/user/meson.build b/target/arm/tcg/user/meson.build
>> new file mode 100644
>> index 0000000000..f18d08c52c
>> --- /dev/null
>> +++ b/target/arm/tcg/user/meson.build
>> @@ -0,0 +1,4 @@
>> +
>> +arm_user_ss.add(when: ['CONFIG_TCG','CONFIG_USER_ONLY'], if_true: files(
>> +  'psci.c',
>> +))
> 
> 

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Claudio Fontana]

On 2/22/21 6:29 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  target/arm/internals.h   |   9 ++-
>>  target/arm/cpu-softmmu.c | 134 +++++++++++++++++++++++++++++++++++++++
>>  target/arm/cpu.c         |  95 ---------------------------
>>  target/arm/meson.build   |   1 +
>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>  create mode 100644 target/arm/cpu-softmmu.c
>>
>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>> index 6384461177..6589b63ebc 100644
>> --- a/target/arm/internals.h
>> +++ b/target/arm/internals.h
>> @@ -1196,4 +1196,11 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>      return ptr;
>>  }
>>  
>> -#endif
>> +#ifndef CONFIG_USER_ONLY
>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>> +#endif /* !CONFIG_USER_ONLY */
>> +
>> +#endif /* TARGET_ARM_INTERNALS_H */
>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>> new file mode 100644
>> index 0000000000..263d1fc588
>> --- /dev/null
>> +++ b/target/arm/cpu-softmmu.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * QEMU ARM CPU
> 
> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
> could expand the header comment.
> 
>   QEMU ARM CPU - Helpers for system emulation and KVM only
> 
> <snip>
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Should I rename all *softmmu in the series to "sysemu"?

I wonder if we should take the issue of sysemu/system/softmmu topic into a separate series.
Currently basically starting from the build system already, "softmmu", sysemu and system are treated as a single thing, and the convention from build system and directories seems to be "softmmu",
while from the header files we get "sysemu/".

I agree that this is not a sufficient model to account for the new feature that Richard wants to develop,
I just suggest we could also consider tackling this separately, with a pass through the whole code, gathering more input in the context of a dedicated series.

What do you think? Also Paolo, any comments, since softmmu is all over meson?

Ciao,

Claudio

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Philippe Mathieu-Daudé]

On 2/23/21 9:55 AM, Claudio Fontana wrote:
> On 2/22/21 6:29 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>  target/arm/internals.h   |   9 ++-
>>>  target/arm/cpu-softmmu.c | 134 +++++++++++++++++++++++++++++++++++++++
>>>  target/arm/cpu.c         |  95 ---------------------------
>>>  target/arm/meson.build   |   1 +
>>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>>  create mode 100644 target/arm/cpu-softmmu.c
>>>
>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>> index 6384461177..6589b63ebc 100644
>>> --- a/target/arm/internals.h
>>> +++ b/target/arm/internals.h
>>> @@ -1196,4 +1196,11 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>>      return ptr;
>>>  }
>>>  
>>> -#endif
>>> +#ifndef CONFIG_USER_ONLY
>>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>>> +#endif /* !CONFIG_USER_ONLY */
>>> +
>>> +#endif /* TARGET_ARM_INTERNALS_H */
>>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>>> new file mode 100644
>>> index 0000000000..263d1fc588
>>> --- /dev/null
>>> +++ b/target/arm/cpu-softmmu.c
>>> @@ -0,0 +1,134 @@
>>> +/*
>>> + * QEMU ARM CPU
>>
>> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
>> could expand the header comment.
>>
>>   QEMU ARM CPU - Helpers for system emulation and KVM only
>>
>> <snip>
>>
>> Otherwise:
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>
> 
> Should I rename all *softmmu in the series to "sysemu"?
> 
> I wonder if we should take the issue of sysemu/system/softmmu topic into a separate series.
> Currently basically starting from the build system already, "softmmu", sysemu and system are treated as a single thing, and the convention from build system and directories seems to be "softmmu",
> while from the header files we get "sysemu/".
> 
> I agree that this is not a sufficient model to account for the new feature that Richard wants to develop,
> I just suggest we could also consider tackling this separately, with a pass through the whole code, gathering more input in the context of a dedicated series.
> 
> What do you think?

This is a valid reasoning. However I have my doubts "doing
that later" will ever be done/finished (not related to you
Claudio in particular, but with dealing with all subsystems).

Personally I'd rather see this sorted out with the arm target
then once done propose it as an example to the other ones.
You already considered the most complex cases, x86 and arm :)

> Also Paolo, any comments, since softmmu is all over meson?
> 
> Ciao,
> 
> Claudio
> 

Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c

[Claudio Fontana]

On 2/23/21 10:16 AM, Philippe Mathieu-Daudé wrote:
> On 2/23/21 9:55 AM, Claudio Fontana wrote:
>> On 2/22/21 6:29 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>>
>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>> ---
>>>>  target/arm/internals.h   |   9 ++-
>>>>  target/arm/cpu-softmmu.c | 134 +++++++++++++++++++++++++++++++++++++++
>>>>  target/arm/cpu.c         |  95 ---------------------------
>>>>  target/arm/meson.build   |   1 +
>>>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>>>  create mode 100644 target/arm/cpu-softmmu.c
>>>>
>>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>>> index 6384461177..6589b63ebc 100644
>>>> --- a/target/arm/internals.h
>>>> +++ b/target/arm/internals.h
>>>> @@ -1196,4 +1196,11 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>>>      return ptr;
>>>>  }
>>>>  
>>>> -#endif
>>>> +#ifndef CONFIG_USER_ONLY
>>>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>>>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>>>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>>>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>>>> +#endif /* !CONFIG_USER_ONLY */
>>>> +
>>>> +#endif /* TARGET_ARM_INTERNALS_H */
>>>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>>>> new file mode 100644
>>>> index 0000000000..263d1fc588
>>>> --- /dev/null
>>>> +++ b/target/arm/cpu-softmmu.c
>>>> @@ -0,0 +1,134 @@
>>>> +/*
>>>> + * QEMU ARM CPU
>>>
>>> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
>>> could expand the header comment.
>>>
>>>   QEMU ARM CPU - Helpers for system emulation and KVM only
>>>
>>> <snip>
>>>
>>> Otherwise:
>>>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>
>> Should I rename all *softmmu in the series to "sysemu"?
>>
>> I wonder if we should take the issue of sysemu/system/softmmu topic into a separate series.
>> Currently basically starting from the build system already, "softmmu", sysemu and system are treated as a single thing, and the convention from build system and directories seems to be "softmmu",
>> while from the header files we get "sysemu/".
>>
>> I agree that this is not a sufficient model to account for the new feature that Richard wants to develop,
>> I just suggest we could also consider tackling this separately, with a pass through the whole code, gathering more input in the context of a dedicated series.
>>
>> What do you think?
> 
> This is a valid reasoning. However I have my doubts "doing
> that later" will ever be done/finished (not related to you
> Claudio in particular, but with dealing with all subsystems).
> 
> Personally I'd rather see this sorted out with the arm target
> then once done propose it as an example to the other ones.
> You already considered the most complex cases, x86 and arm :)


Ok, if there are no other comments I would go with "sysemu", just because "system" is a bit too much of a loaded word,
and we have the precedent of include/sysemu/ .


> 
>> Also Paolo, any comments, since softmmu is all over meson?
>>

And Peter, any comments, preference?

Ciao,

Claudio

softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Claudio Fontana]

On 2/23/21 10:35 AM, Claudio Fontana wrote:
> On 2/23/21 10:16 AM, Philippe Mathieu-Daudé wrote:
>> On 2/23/21 9:55 AM, Claudio Fontana wrote:
>>> On 2/22/21 6:29 PM, Alex Bennée wrote:
>>>>
>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>
>>>>> From: Claudio Fontana <cfontana@centriq4.arch.suse.de>
>>>>>
>>>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>>>> ---
>>>>>  target/arm/internals.h   |   9 ++-
>>>>>  target/arm/cpu-softmmu.c | 134 +++++++++++++++++++++++++++++++++++++++
>>>>>  target/arm/cpu.c         |  95 ---------------------------
>>>>>  target/arm/meson.build   |   1 +
>>>>>  4 files changed, 143 insertions(+), 96 deletions(-)
>>>>>  create mode 100644 target/arm/cpu-softmmu.c
>>>>>
>>>>> diff --git a/target/arm/internals.h b/target/arm/internals.h
>>>>> index 6384461177..6589b63ebc 100644
>>>>> --- a/target/arm/internals.h
>>>>> +++ b/target/arm/internals.h
>>>>> @@ -1196,4 +1196,11 @@ static inline uint64_t useronly_maybe_clean_ptr(uint32_t desc, uint64_t ptr)
>>>>>      return ptr;
>>>>>  }
>>>>>  
>>>>> -#endif
>>>>> +#ifndef CONFIG_USER_ONLY
>>>>> +void arm_cpu_set_irq(void *opaque, int irq, int level);
>>>>> +void arm_cpu_kvm_set_irq(void *opaque, int irq, int level);
>>>>> +bool arm_cpu_virtio_is_big_endian(CPUState *cs);
>>>>> +uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri);
>>>>> +#endif /* !CONFIG_USER_ONLY */
>>>>> +
>>>>> +#endif /* TARGET_ARM_INTERNALS_H */
>>>>> diff --git a/target/arm/cpu-softmmu.c b/target/arm/cpu-softmmu.c
>>>>> new file mode 100644
>>>>> index 0000000000..263d1fc588
>>>>> --- /dev/null
>>>>> +++ b/target/arm/cpu-softmmu.c
>>>>> @@ -0,0 +1,134 @@
>>>>> +/*
>>>>> + * QEMU ARM CPU
>>>>
>>>> I guess apropos the discussion earlier it's really cpu-sysemu.c and we
>>>> could expand the header comment.
>>>>
>>>>   QEMU ARM CPU - Helpers for system emulation and KVM only
>>>>
>>>> <snip>
>>>>
>>>> Otherwise:
>>>>
>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>
>>> Should I rename all *softmmu in the series to "sysemu"?
>>>
>>> I wonder if we should take the issue of sysemu/system/softmmu topic into a separate series.
>>> Currently basically starting from the build system already, "softmmu", sysemu and system are treated as a single thing, and the convention from build system and directories seems to be "softmmu",
>>> while from the header files we get "sysemu/".
>>>
>>> I agree that this is not a sufficient model to account for the new feature that Richard wants to develop,
>>> I just suggest we could also consider tackling this separately, with a pass through the whole code, gathering more input in the context of a dedicated series.
>>>
>>> What do you think?
>>
>> This is a valid reasoning. However I have my doubts "doing
>> that later" will ever be done/finished (not related to you
>> Claudio in particular, but with dealing with all subsystems).

I went through this again, and looked at the todo.

Already just at the meson level, in the previous i386 series, the changes to split real softmmu (CONFIG_SOFTMMU?) and sysemu would be required.

Otherwise I don't feel so comfortable adding a half-baked discrepancy from the current convention, which is consistent
(even if "wrong", or "whose model is insufficiently detailed" to be able to implement new features).

If we don't believe that the work of splitting semantically real softmmu and sysemu would ever be completed,
why start in the middle of this series? The risk would then result in a worse situation than the current one I think,
with an even less homogeneous code base.

Could this work not be done independently of this series (and I could rebase on that if necessary),
and pushed by who really understand the problem? Probably starting from meson, configure, ...
I could help reviewing there, but probably I am not the best person to push it.

Why are we not confident that this can be done?


>>
>> Personally I'd rather see this sorted out with the arm target
>> then once done propose it as an example to the other ones.
>> You already considered the most complex cases, x86 and arm :)


I mean, after we have split the code a bit more properly, renaming the additional mentions of "softmmu" to "sysemu" should not be hard.
Or, we could do the softmmu vs sysemu split first, and I rebase on top of that.


> 
> 
> Ok, if there are no other comments I would go with "sysemu", just because "system" is a bit too much of a loaded word,
> and we have the precedent of include/sysemu/ .


I am all for "just getting it done", but here the i386 and the arm series become a mixture of things that I am not comfortable with,
I'd prefer a dedicated series..


>>> Also Paolo, any comments, since softmmu is all over meson?
>>>
> 
> And Peter, any comments, preference?
> 


Maybe let us give some more time for more comments to flow in?


Thanks,


Claudio

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Richard Henderson]

On 2/23/21 10:18 AM, Claudio Fontana wrote:
> I am all for "just getting it done", but here the i386 and the arm series become a mixture of things that I am not comfortable with,
> I'd prefer a dedicated series..

You're thinking too deeply about the problem that I'm reporting to you about
this patch set.

I just want the file naming done correctly, while you're renaming.  That is
something you are actively changing in this patch set, so we should get it right.

You don't have to worry about ifdef CONFIG_SOFTMMU vs ifndef CONFIG_USER_ONLY
within the code itself.


r~

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Philippe Mathieu-Daudé]

On 2/23/21 7:51 PM, Richard Henderson wrote:
> On 2/23/21 10:18 AM, Claudio Fontana wrote:
>> I am all for "just getting it done", but here the i386 and the arm series become a mixture of things that I am not comfortable with,
>> I'd prefer a dedicated series..
> 
> You're thinking too deeply about the problem that I'm reporting to you about
> this patch set.
> 
> I just want the file naming done correctly, while you're renaming.  That is
> something you are actively changing in this patch set, so we should get it right.

So what is the new directory structure?

- user/
- sysemu/{tcg,kvm,}/

or

- tcg/a-user.c
- tcg/b-sysemu.c
- kvm/kvm.c

> You don't have to worry about ifdef CONFIG_SOFTMMU vs ifndef CONFIG_USER_ONLY
> within the code itself.

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Richard Henderson]

On 2/23/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> On 2/23/21 7:51 PM, Richard Henderson wrote:
>> I just want the file naming done correctly, while you're renaming.  That is
>> something you are actively changing in this patch set, so we should get it right.
> 
> So what is the new directory structure?
> 
> - user/
> - sysemu/{tcg,kvm,}/
> 
> or
> 
> - tcg/a-user.c
> - tcg/b-sysemu.c
> - kvm/kvm.c

Personally I think this second one makes more sense, focused primarily on the
accelerator and secondarily on the kind of emulation.


r~

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Paolo Bonzini]

On 24/02/21 01:06, Richard Henderson wrote:
> On 2/23/21 3:43 PM, Philippe Mathieu-Daudé wrote:
>> On 2/23/21 7:51 PM, Richard Henderson wrote:
>>> I just want the file naming done correctly, while you're renaming.  That is
>>> something you are actively changing in this patch set, so we should get it right.
>>
>> So what is the new directory structure?
>>
>> - user/
>> - sysemu/{tcg,kvm,}/
>>
>> or
>>
>> - tcg/a-user.c
>> - tcg/b-sysemu.c
>> - kvm/kvm.c
> 
> Personally I think this second one makes more sense, focused primarily on the
> accelerator and secondarily on the kind of emulation.

I agree.

I don't care _too much_ about sysemu vs. softmmu.  In any case if we 
want to go with sysemu it can be done in steps:

- easy: rename files and directories

- medium: rename sourcesets in meson.build

- harder (or just larger): rename CONFIG_SOFTMMU

Paolo

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Claudio Fontana]

On 2/24/21 10:20 AM, Paolo Bonzini wrote:
> On 24/02/21 01:06, Richard Henderson wrote:
>> On 2/23/21 3:43 PM, Philippe Mathieu-Daudé wrote:
>>> On 2/23/21 7:51 PM, Richard Henderson wrote:
>>>> I just want the file naming done correctly, while you're renaming.  That is
>>>> something you are actively changing in this patch set, so we should get it right.
>>>
>>> So what is the new directory structure?
>>>
>>> - user/
>>> - sysemu/{tcg,kvm,}/
>>>
>>> or
>>>
>>> - tcg/a-user.c
>>> - tcg/b-sysemu.c
>>> - kvm/kvm.c
>>
>> Personally I think this second one makes more sense, focused primarily on the
>> accelerator and secondarily on the kind of emulation.
> 
> I agree.

Agree here are well.

> 
> I don't care _too much_ about sysemu vs. softmmu.


I am neutral here as well.


>In any case if we 
> want to go with sysemu it can be done in steps:
> 
> - easy: rename files and directories

right. The ask in this series from Richard is to rename new files and directories from "softmmu" to "sysemu".

This gives us a resulting codebase where some directories are softmmu, some are sysemu, and the meson stuff is still softmmu.
To me this is a worse situation than where we started, but if I am the only one perceiving this I will just call the new directories and files "sysemu".


> 
> - medium: rename sourcesets in meson.build

right

> 
> - harder (or just larger): rename CONFIG_SOFTMMU

indeed. Currently (I think) CONFIG_SOFTMMU is the same as !CONFIG_USER_ONLY. But IIUC CONFIG_SOFTMMU would assume a new meaning after the change.

> 
> Paolo
> 

Re: softmmu vs sysemu [Was: Re: [RFC v1 06/38] target/arm: split off cpu-softmmu.c]

[Paolo Bonzini]

On 24/02/21 10:30, Claudio Fontana wrote:
>> - easy: rename files and directories
> right. The ask in this series from Richard is to rename new files and
> directories from "softmmu" to "sysemu".
> 
> This gives us a resulting codebase where some directories are
> softmmu, some are sysemu, and the meson stuff is still softmmu. To me
> this is a worse situation than where we started, but if I am the only
> one perceiving this I will just call the new directories and files
> "sysemu".

We already have include/sysemu.  Renaming softmmu/ to sysemu/ would be 
easy enough, maybe even one-line.

If you are going to post the i386 version with the files renamed in the 
same way, I think that one was pretty close to ready?

Paolo

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 2/22/21 8:00 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> Hi all,
>>
>> this is an experiment, a cleanup based on and requiring the series
>> "i386 cleanup PART 2":
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>
>> The goal is to split the code between TCG-only and non-TCG code,
>> fixing the KVM-only build (configure --disable-tcg),
>>
>> and laying the ground for further cleanups and the use of the
>> new accel objects in the hierarchy to specialize the cpu
>> according to the accelerator.
>>
>> This is known to be an early state, with probably a lot of work
>> still needed.
> 
> Well early work is looking pretty good:
> 
>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*

:~)

> 
> and I've tested the KVM side works well enough with a basic image.

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/23/21 10:18 AM, Philippe Mathieu-Daudé wrote:
> On 2/22/21 8:00 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> Hi all,
>>>
>>> this is an experiment, a cleanup based on and requiring the series
>>> "i386 cleanup PART 2":
>>>
>>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>>
>>> The goal is to split the code between TCG-only and non-TCG code,
>>> fixing the KVM-only build (configure --disable-tcg),
>>>
>>> and laying the ground for further cleanups and the use of the
>>> new accel objects in the hierarchy to specialize the cpu
>>> according to the accelerator.
>>>
>>> This is known to be an early state, with probably a lot of work
>>> still needed.
>>
>> Well early work is looking pretty good:
>>
>>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
> 
> :~)
> 
>>
>> and I've tested the KVM side works well enough with a basic image.
> 
> 

I am working on the next version, one thing I noticed among others as I get close to the v2,
is the fact that tests/ for arm require tcg in many cases.

So there is even more cleanup needed to discern which are actually tcg-only, and how to tweak the others into working also with only kvm available..

Ciao,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

> On 2/23/21 10:18 AM, Philippe Mathieu-Daudé wrote:
>> On 2/22/21 8:00 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> Hi all,
>>>>
>>>> this is an experiment, a cleanup based on and requiring the series
>>>> "i386 cleanup PART 2":
>>>>
>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>>>
>>>> The goal is to split the code between TCG-only and non-TCG code,
>>>> fixing the KVM-only build (configure --disable-tcg),
>>>>
>>>> and laying the ground for further cleanups and the use of the
>>>> new accel objects in the hierarchy to specialize the cpu
>>>> according to the accelerator.
>>>>
>>>> This is known to be an early state, with probably a lot of work
>>>> still needed.
>>>
>>> Well early work is looking pretty good:
>>>
>>>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>>>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>>>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>>>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
>> 
>> :~)
>> 
>>>
>>> and I've tested the KVM side works well enough with a basic image.
>> 
>> 
>
> I am working on the next version, one thing I noticed among others as I get close to the v2,
> is the fact that tests/ for arm require tcg in many cases.

I think in a lot of cases they are historical because developers
generally weren't running on native hardware. That said off the top of
my head:

  tests/tcg - linux-user, so implies TCG
  tests/tcg/system - use semihosting (at least for arm/aarch64) - which implies TCG
  tests/acceptance/[replay_kernel/reverse_debugging/tcg_plugins] - all need TCG features

I don't think there is any reason the others can't run with KVM - and
probably should on real hardware.

>
> So there is even more cleanup needed to discern which are actually tcg-only, and how to tweak the others into working also with only kvm available..
>
> Ciao,
>
> Claudio


-- 
Alex Bennée

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/1/21 5:23 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> On 2/23/21 10:18 AM, Philippe Mathieu-Daudé wrote:
>>> On 2/22/21 8:00 PM, Alex Bennée wrote:
>>>>
>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>
>>>>> Hi all,
>>>>>
>>>>> this is an experiment, a cleanup based on and requiring the series
>>>>> "i386 cleanup PART 2":
>>>>>
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>>>>
>>>>> The goal is to split the code between TCG-only and non-TCG code,
>>>>> fixing the KVM-only build (configure --disable-tcg),
>>>>>
>>>>> and laying the ground for further cleanups and the use of the
>>>>> new accel objects in the hierarchy to specialize the cpu
>>>>> according to the accelerator.
>>>>>
>>>>> This is known to be an early state, with probably a lot of work
>>>>> still needed.
>>>>
>>>> Well early work is looking pretty good:
>>>>
>>>>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>>>>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>>>>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>>>>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
>>>
>>> :~)
>>>
>>>>
>>>> and I've tested the KVM side works well enough with a basic image.
>>>
>>>
>>
>> I am working on the next version, one thing I noticed among others as I get close to the v2,
>> is the fact that tests/ for arm require tcg in many cases.
> 
> I think in a lot of cases they are historical because developers
> generally weren't running on native hardware. That said off the top of
> my head:
> 
>   tests/tcg - linux-user, so implies TCG
>   tests/tcg/system - use semihosting (at least for arm/aarch64) - which implies TCG
>   tests/acceptance/[replay_kernel/reverse_debugging/tcg_plugins] - all need TCG features
> 
> I don't think there is any reason the others can't run with KVM - and
> probably should on real hardware.


One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
and the thing tries to create a cortex-a15 cpu model for some unknown reason.

Digging and sweating..

Ciao,

Claudio

> 
>>
>> So there is even more cleanup needed to discern which are actually tcg-only, and how to tweak the others into working also with only kvm available..
>>
>> Ciao,
>>
>> Claudio
> 
> 

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Peter Maydell]

On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
> and the thing tries to create a cortex-a15 cpu model for some unknown reason.

That is expected. The default CPU type for 'virt' is cortex-a15; if you want
something else then you need to specify the -cpu option.

-- PMM

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/3/21 7:09 PM, Peter Maydell wrote:
> On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
> 
> That is expected. The default CPU type for 'virt' is cortex-a15; if you want
> something else then you need to specify the -cpu option.
> 
> -- PMM
> 

I see, I'll experiment a bit thanks.

I assume changing the default to "max" is out of the question,
and we should instead feed the -cpu option from the tests?

Ciao,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/3/21 7:17 PM, Claudio Fontana wrote:
> On 3/3/21 7:09 PM, Peter Maydell wrote:
>> On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
>>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
>>
>> That is expected. The default CPU type for 'virt' is cortex-a15; if you want
>> something else then you need to specify the -cpu option.
>>
>> -- PMM
>>
> 
> I see, I'll experiment a bit thanks.
> 
> I assume changing the default to "max" is out of the question,
> and we should instead feed the -cpu option from the tests?
> 

And since we are on topic, should the qemu-system-aarch64 still contain the cortex-a15 cpu model for some reason?

Thanks,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 3/3/21 7:20 PM, Claudio Fontana wrote:
> On 3/3/21 7:17 PM, Claudio Fontana wrote:
>> On 3/3/21 7:09 PM, Peter Maydell wrote:
>>> On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
>>>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>>>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
>>>
>>> That is expected. The default CPU type for 'virt' is cortex-a15; if you want
>>> something else then you need to specify the -cpu option.
>>>
>>> -- PMM
>>>
>>
>> I see, I'll experiment a bit thanks.
>>
>> I assume changing the default to "max" is out of the question,
>> and we should instead feed the -cpu option from the tests?
>>
> 
> And since we are on topic, should the qemu-system-aarch64 still contain the cortex-a15 cpu model for some reason?

If it is available, certainly. Else no :)

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Richard Henderson]

On 3/3/21 10:20 AM, Claudio Fontana wrote:
> On 3/3/21 7:17 PM, Claudio Fontana wrote:
>> On 3/3/21 7:09 PM, Peter Maydell wrote:
>>> On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
>>>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>>>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
>>>
>>> That is expected. The default CPU type for 'virt' is cortex-a15; if you want
>>> something else then you need to specify the -cpu option.
>>>
>>> -- PMM
>>>
>>
>> I see, I'll experiment a bit thanks.
>>
>> I assume changing the default to "max" is out of the question,
>> and we should instead feed the -cpu option from the tests?
>>
> 
> And since we are on topic, should the qemu-system-aarch64 still contain the cortex-a15 cpu model for some reason?

The goal is for qemu-system-arm and qemu-system-aarch64 to be as compatible as 
possible.  That's why the default is the same for both.


r~

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/3/21 7:39 PM, Richard Henderson wrote:
> On 3/3/21 10:20 AM, Claudio Fontana wrote:
>> On 3/3/21 7:17 PM, Claudio Fontana wrote:
>>> On 3/3/21 7:09 PM, Peter Maydell wrote:
>>>> On Wed, 3 Mar 2021 at 17:57, Claudio Fontana <cfontana@suse.de> wrote:
>>>>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>>>>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
>>>>
>>>> That is expected. The default CPU type for 'virt' is cortex-a15; if you want
>>>> something else then you need to specify the -cpu option.
>>>>
>>>> -- PMM
>>>>
>>>
>>> I see, I'll experiment a bit thanks.
>>>
>>> I assume changing the default to "max" is out of the question,
>>> and we should instead feed the -cpu option from the tests?
>>>
>>
>> And since we are on topic, should the qemu-system-aarch64 still contain the cortex-a15 cpu model for some reason?
> 
> The goal is for qemu-system-arm and qemu-system-aarch64 to be as compatible as 
> possible.  That's why the default is the same for both.
> 
> 
> r~
> 

Ah too bad, then I need to rework some code,
and we need to keep lots of stuff that otherwise could have gone away.

It is a bit weird that qemu-system-aarch64 runs with a cortex-a15 model tbh, as cortex-a15 is not capable of aarch64.

Thanks,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Richard Henderson]

On 3/3/21 10:45 AM, Claudio Fontana wrote:
> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15 model tbh, as cortex-a15 is not capable of aarch64.

No, but qemu-system-aarch64 is capable of 32-bit emulation (because most 64-bit 
cpus retain 32-bit mode).  It takes no extra effort to run cortex-a15 than it 
does a cortex-a57.

I have wondered if we should have just one qemu-system-arm that does it all and 
drop the separate qemu-system-aarch64 -- or vice versa.  But we've had the 
separation around so long I'm sure someone would be confused.


r~

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 3/3/21 7:54 PM, Richard Henderson wrote:
> On 3/3/21 10:45 AM, Claudio Fontana wrote:
>> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15
>> model tbh, as cortex-a15 is not capable of aarch64.
> 
> No, but qemu-system-aarch64 is capable of 32-bit emulation (because most
> 64-bit cpus retain 32-bit mode).  It takes no extra effort to run
> cortex-a15 than it does a cortex-a57.

IIRC qemu-system-arm starts the aa64 cores in 32-bit mode, while
qemu-system-aarch64 in 64-bit (this gave me trouble because the
kernels for the raspi 64-bit SoCs are in 32-bit mode -- because
the GPU starts them in this mode).

> I have wondered if we should have just one qemu-system-arm that does it
> all and drop the separate qemu-system-aarch64 -- or vice versa.  But
> we've had the separation around so long I'm sure someone would be confused.

That would be great cleanup IMHO.

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/4/21 5:39 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/21 7:54 PM, Richard Henderson wrote:
>> On 3/3/21 10:45 AM, Claudio Fontana wrote:
>>> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15
>>> model tbh, as cortex-a15 is not capable of aarch64.
>>
>> No, but qemu-system-aarch64 is capable of 32-bit emulation (because most
>> 64-bit cpus retain 32-bit mode).  It takes no extra effort to run
>> cortex-a15 than it does a cortex-a57.
> 
> IIRC qemu-system-arm starts the aa64 cores in 32-bit mode, while
> qemu-system-aarch64 in 64-bit (this gave me trouble because the
> kernels for the raspi 64-bit SoCs are in 32-bit mode -- because
> the GPU starts them in this mode).
> 
>> I have wondered if we should have just one qemu-system-arm that does it
>> all and drop the separate qemu-system-aarch64 -- or vice versa.  But
>> we've had the separation around so long I'm sure someone would be confused.
> 
> That would be great cleanup IMHO.
> 

Would we still be able to configure a lean AARCH64-only qemu that only contains the cpu models we want,
(via board configuration / KConfig?),

for example, a kvm-only build that only has a few 64-bit cpu models in it, plus max/host and removes all the rest?

Ciao,

CLaudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 3/4/21 6:19 PM, Claudio Fontana wrote:
> On 3/4/21 5:39 PM, Philippe Mathieu-Daudé wrote:
>> On 3/3/21 7:54 PM, Richard Henderson wrote:
>>> On 3/3/21 10:45 AM, Claudio Fontana wrote:
>>>> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15
>>>> model tbh, as cortex-a15 is not capable of aarch64.
>>>
>>> No, but qemu-system-aarch64 is capable of 32-bit emulation (because most
>>> 64-bit cpus retain 32-bit mode).  It takes no extra effort to run
>>> cortex-a15 than it does a cortex-a57.
>>
>> IIRC qemu-system-arm starts the aa64 cores in 32-bit mode, while
>> qemu-system-aarch64 in 64-bit (this gave me trouble because the
>> kernels for the raspi 64-bit SoCs are in 32-bit mode -- because
>> the GPU starts them in this mode).
>>
>>> I have wondered if we should have just one qemu-system-arm that does it
>>> all and drop the separate qemu-system-aarch64 -- or vice versa.  But
>>> we've had the separation around so long I'm sure someone would be confused.
>>
>> That would be great cleanup IMHO.
>>
> 
> Would we still be able to configure a lean AARCH64-only qemu that only contains the cpu models we want,
> (via board configuration / KConfig?),

We shouldn't even worry about this IMO. Because ...

> for example, a kvm-only build that only has a few 64-bit cpu models in it, plus max/host and removes all the rest?

... since host word-size becomes irrelevant, you only have to consider
the accelerator possibilities. Restricting to KVM directly select a
subset of CPUs usable.

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Peter Maydell]

On Thu, 4 Mar 2021 at 16:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 3/3/21 7:54 PM, Richard Henderson wrote:
> > On 3/3/21 10:45 AM, Claudio Fontana wrote:
> >> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15
> >> model tbh, as cortex-a15 is not capable of aarch64.
> >
> > No, but qemu-system-aarch64 is capable of 32-bit emulation (because most
> > 64-bit cpus retain 32-bit mode).  It takes no extra effort to run
> > cortex-a15 than it does a cortex-a57.
>
> IIRC qemu-system-arm starts the aa64 cores in 32-bit mode, while

...no, it simply doesn't have them at all; it only has
the 32-bit-only cores. We only support '64-bit core that
starts in 32-bit mode' for KVM; it would be nice to have the
TCG support for that but nobody's ever written it.

> qemu-system-aarch64 in 64-bit (this gave me trouble because the
> kernels for the raspi 64-bit SoCs are in 32-bit mode -- because
> the GPU starts them in this mode).
>
> > I have wondered if we should have just one qemu-system-arm that does it
> > all and drop the separate qemu-system-aarch64 -- or vice versa.  But
> > we've had the separation around so long I'm sure someone would be confused.
>
> That would be great cleanup IMHO.

I figure it's not really worth the bother until/unless we ever
some day get to the holy grail of "one binary that supports
multiple target architectures", at which point we could have
one qemu-system for everything...

-- PMM

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/4/21 8:24 PM, Peter Maydell wrote:
> On Thu, 4 Mar 2021 at 16:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 3/3/21 7:54 PM, Richard Henderson wrote:
>>> On 3/3/21 10:45 AM, Claudio Fontana wrote:
>>>> It is a bit weird that qemu-system-aarch64 runs with a cortex-a15
>>>> model tbh, as cortex-a15 is not capable of aarch64.
>>>
>>> No, but qemu-system-aarch64 is capable of 32-bit emulation (because most
>>> 64-bit cpus retain 32-bit mode).  It takes no extra effort to run
>>> cortex-a15 than it does a cortex-a57.
>>
>> IIRC qemu-system-arm starts the aa64 cores in 32-bit mode, while
> 
> ...no, it simply doesn't have them at all; it only has
> the 32-bit-only cores. We only support '64-bit core that
> starts in 32-bit mode' for KVM; it would be nice to have the
> TCG support for that but nobody's ever written it.
> 
>> qemu-system-aarch64 in 64-bit (this gave me trouble because the
>> kernels for the raspi 64-bit SoCs are in 32-bit mode -- because
>> the GPU starts them in this mode).
>>
>>> I have wondered if we should have just one qemu-system-arm that does it
>>> all and drop the separate qemu-system-aarch64 -- or vice versa.  But
>>> we've had the separation around so long I'm sure someone would be confused.
>>
>> That would be great cleanup IMHO.
> 
> I figure it's not really worth the bother until/unless we ever
> some day get to the holy grail of "one binary that supports
> multiple target architectures", at which point we could have
> one qemu-system for everything...
> 
> -- PMM
> 

"one small binary, with optionally pluggable modules for multiple target architectures and optional features" ?

Ciao,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Philippe Mathieu-Daudé]

On 3/3/21 6:57 PM, Claudio Fontana wrote:
> On 3/1/21 5:23 PM, Alex Bennée wrote:
>>
>> Claudio Fontana <cfontana@suse.de> writes:
>>
>>> On 2/23/21 10:18 AM, Philippe Mathieu-Daudé wrote:
>>>> On 2/22/21 8:00 PM, Alex Bennée wrote:
>>>>>
>>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> this is an experiment, a cleanup based on and requiring the series
>>>>>> "i386 cleanup PART 2":
>>>>>>
>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>>>>>
>>>>>> The goal is to split the code between TCG-only and non-TCG code,
>>>>>> fixing the KVM-only build (configure --disable-tcg),
>>>>>>
>>>>>> and laying the ground for further cleanups and the use of the
>>>>>> new accel objects in the hierarchy to specialize the cpu
>>>>>> according to the accelerator.
>>>>>>
>>>>>> This is known to be an early state, with probably a lot of work
>>>>>> still needed.
>>>>>
>>>>> Well early work is looking pretty good:
>>>>>
>>>>>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>>>>>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>>>>>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>>>>>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
>>>>
>>>> :~)
>>>>
>>>>>
>>>>> and I've tested the KVM side works well enough with a basic image.
>>>>
>>>>
>>>
>>> I am working on the next version, one thing I noticed among others as I get close to the v2,
>>> is the fact that tests/ for arm require tcg in many cases.
>>
>> I think in a lot of cases they are historical because developers
>> generally weren't running on native hardware. That said off the top of
>> my head:
>>
>>   tests/tcg - linux-user, so implies TCG
>>   tests/tcg/system - use semihosting (at least for arm/aarch64) - which implies TCG
>>   tests/acceptance/[replay_kernel/reverse_debugging/tcg_plugins] - all need TCG features
>>
>> I don't think there is any reason the others can't run with KVM - and
>> probably should on real hardware.
> 
> 
> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
> 
> Digging and sweating..

I already sent a series to fix that, feel free to reuse
some patches:
https://www.mail-archive.com/qemu-block@nongnu.org/msg80440.html

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 3/3/21 7:34 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/21 6:57 PM, Claudio Fontana wrote:
>> On 3/1/21 5:23 PM, Alex Bennée wrote:
>>>
>>> Claudio Fontana <cfontana@suse.de> writes:
>>>
>>>> On 2/23/21 10:18 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 2/22/21 8:00 PM, Alex Bennée wrote:
>>>>>>
>>>>>> Claudio Fontana <cfontana@suse.de> writes:
>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> this is an experiment, a cleanup based on and requiring the series
>>>>>>> "i386 cleanup PART 2":
>>>>>>>
>>>>>>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>>>>>>
>>>>>>> The goal is to split the code between TCG-only and non-TCG code,
>>>>>>> fixing the KVM-only build (configure --disable-tcg),
>>>>>>>
>>>>>>> and laying the ground for further cleanups and the use of the
>>>>>>> new accel objects in the hierarchy to specialize the cpu
>>>>>>> according to the accelerator.
>>>>>>>
>>>>>>> This is known to be an early state, with probably a lot of work
>>>>>>> still needed.
>>>>>>
>>>>>> Well early work is looking pretty good:
>>>>>>
>>>>>>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>>>>>>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>>>>>>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>>>>>>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
>>>>>
>>>>> :~)
>>>>>
>>>>>>
>>>>>> and I've tested the KVM side works well enough with a basic image.
>>>>>
>>>>>
>>>>
>>>> I am working on the next version, one thing I noticed among others as I get close to the v2,
>>>> is the fact that tests/ for arm require tcg in many cases.
>>>
>>> I think in a lot of cases they are historical because developers
>>> generally weren't running on native hardware. That said off the top of
>>> my head:
>>>
>>>   tests/tcg - linux-user, so implies TCG
>>>   tests/tcg/system - use semihosting (at least for arm/aarch64) - which implies TCG
>>>   tests/acceptance/[replay_kernel/reverse_debugging/tcg_plugins] - all need TCG features
>>>
>>> I don't think there is any reason the others can't run with KVM - and
>>> probably should on real hardware.
>>
>>
>> One thing I noticed is that tests try to run qemu-system-aarch64 with accel "qtest" and machine "virt",
>> and the thing tries to create a cortex-a15 cpu model for some unknown reason.
>>
>> Digging and sweating..
> 
> I already sent a series to fix that, feel free to reuse
> some patches:
> https://www.mail-archive.com/qemu-block@nongnu.org/msg80440.html
> 

Ah yes I took already something from here, will look some more thanks!

Ciao,

Claudio

Re: [RFC v1 00/38] arm cleanup experiment for kvm-only build

[Claudio Fontana]

On 2/22/21 8:00 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> Hi all,
>>
>> this is an experiment, a cleanup based on and requiring the series
>> "i386 cleanup PART 2":
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg05935.html
>>
>> The goal is to split the code between TCG-only and non-TCG code,
>> fixing the KVM-only build (configure --disable-tcg),
>>
>> and laying the ground for further cleanups and the use of the
>> new accel objects in the hierarchy to specialize the cpu
>> according to the accelerator.
>>
>> This is known to be an early state, with probably a lot of work
>> still needed.
> 
> Well early work is looking pretty good:
> 
>   18:59:22 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 107M Feb 22 18:08 qemu-system-aarch64*
>   18:59:29 [alex@idun:~/l/q/b/default] review/arm-cleanup-rfc1|… ± ls -lh ../disable.tcg/qemu-system-aarch64
>   -rwxr-xr-x 1 alex alex 76M Feb 22 17:47 ../disable.tcg/qemu-system-aarch64*
> 
> and I've tested the KVM side works well enough with a basic image.

Thanks for giving it a spin :-)

Now needs a cleanup pass for sure..

Ciao,

Claudio

> 
>>
>> I thought it could be useful to share early, especially in light
>> of the combination of this with Philippe's work on building
>> only the machines and devices compatible with KVM for arm.
>>
>> Comments welcome, thanks,
>>
>> Claudio
>>
>>
>> Claudio Fontana (38):
>>   target/arm: move translate modules to tcg/
>>   target/arm: move helpers to tcg/
>>   arm: tcg: only build under CONFIG_TCG
>>   target/arm: move psci.c into tcg/softmmu/
>>   target/arm: wrap arm_cpu_exec_interrupt in CONFIG_TCG
>>   target/arm: split off cpu-softmmu.c
>>   target/arm: split off softmmu/helper.c
>>   target/arm/tcg: split softmmu parts of v8_cp_reginfo and
>>     el2_cp_reginfo
>>   target/arm/tcg: split softmmu parts of vhe_cp_reginfo
>>   target/arm/tcg: move v8_user_idregs to user-only subdir
>>   target/arm/tcg: move id_v8_user_midr_cp_reginfo to user-only subdir
>>   target/arm/tcg: move mpidr_user_cp_reginfo to user-only subdir
>>   target/arm/tcg: split vapa_cp_reginfo softmmu part
>>   target/arm: move vec_internal.h to tcg/
>>   target/arm: move aarch64_sync_32_to_64 and vv to cpu code
>>   target/arm: move arm_sctlr away from tcg helpers
>>   target/arm: move switch_mode and cpsr_read/write to cpu
>>   target/arm: split vfp state setting from tcg helpers
>>   target/arm: move register read/write to common_cpu
>>   target/arm: move arm_hcr_el2_eff to common_cpu
>>   target/arm: move cp regs definition functions to common_cpu
>>   target/arm: move arm_cpu_list to common_cpu
>>   target/arm: move arm_mmu_idx_el to common-cpu
>>   target/arm: move aa64_va_parameter_tbi,tbid,tcma and
>>     arm_rebuild_hflags
>>   target/arm: move fp_exception_el outside of tcg helpers
>>   target/arm: move sve_exception_el to cpu
>>   target/arm: move sve_zcr_len_for_el to common_cpu
>>   target/arm: make arm_pmu_timer_cb TCG-only, starting tcg-stub
>>   target/arm/tcg: add write_v7m_exception to stubs
>>   target/arm: make hw_watchpoint and hw_breakpoint stuff tcg-only
>>   target/arm: move cp register write-ignore and read-as-zero to cpu
>>   target/arm: cpu: do not initialize TCG PMU for KVM
>>   target/arm: cpu: do not initialize TCG view of cpregs
>>   target/arm: cpu: only initialize TCG gt timers under CONFIG_TCG
>>   target/arm: get-phys-addr: new module
>>   target/arm: move qmp_query_cpu_definitions to common_cpu
>>   target/arm: move arm_mmu_idx from tcg to get-phys-addr.c
>>   target/arm: move arm_cpu_do_interrupt from tcg to common code
>>
>>  meson.build                               |    2 +
>>  target/arm/cpu.h                          |    5 +
>>  target/arm/internals.h                    |   84 +-
>>  target/arm/tcg/helper-tcg.h               |   50 +
>>  target/arm/tcg/softmmu/trace.h            |    1 +
>>  target/arm/tcg/trace.h                    |    1 +
>>  target/arm/{ => tcg}/translate-a64.h      |    0
>>  target/arm/{ => tcg}/translate.h          |    0
>>  target/arm/{ => tcg}/vec_internal.h       |    0
>>  target/arm/{ => tcg}/a32-uncond.decode    |    0
>>  target/arm/{ => tcg}/a32.decode           |    0
>>  target/arm/{ => tcg}/m-nocp.decode        |    0
>>  target/arm/{ => tcg}/neon-dp.decode       |    0
>>  target/arm/{ => tcg}/neon-ls.decode       |    0
>>  target/arm/{ => tcg}/neon-shared.decode   |    0
>>  target/arm/{ => tcg}/sve.decode           |    0
>>  target/arm/{ => tcg}/t16.decode           |    0
>>  target/arm/{ => tcg}/t32.decode           |    0
>>  target/arm/{ => tcg}/vfp-uncond.decode    |    0
>>  target/arm/{ => tcg}/vfp.decode           |    0
>>  target/arm/cpu-common.c                   | 1388 +++++
>>  target/arm/cpu-softmmu.c                  | 1228 ++++
>>  target/arm/cpu-user.c                     |   77 +
>>  target/arm/cpu-vfp.c                      |  232 +
>>  target/arm/cpu.c                          |  109 +-
>>  target/arm/get-phys-addr.c                | 2286 +++++++
>>  target/arm/machine.c                      |   23 +-
>>  target/arm/{ => tcg}/crypto_helper.c      |    0
>>  target/arm/{ => tcg}/debug_helper.c       |    0
>>  target/arm/{ => tcg}/helper-a64.c         |    0
>>  target/arm/{ => tcg}/helper.c             | 6613 +--------------------
>>  target/arm/{ => tcg}/iwmmxt_helper.c      |    0
>>  target/arm/{ => tcg}/m_helper.c           |    0
>>  target/arm/{ => tcg}/mte_helper.c         |    0
>>  target/arm/{ => tcg}/neon_helper.c        |    0
>>  target/arm/{ => tcg}/op_helper.c          |    0
>>  target/arm/{ => tcg}/pauth_helper.c       |    0
>>  target/arm/tcg/softmmu/helper.c           | 1784 ++++++
>>  target/arm/{ => tcg/softmmu}/psci.c       |    0
>>  target/arm/{ => tcg}/sve_helper.c         |    0
>>  target/arm/tcg/tcg-stub.c                 |   20 +
>>  target/arm/{ => tcg}/tlb_helper.c         |    0
>>  target/arm/{ => tcg}/translate-a64.c      |    0
>>  target/arm/{ => tcg}/translate-sve.c      |    0
>>  target/arm/{ => tcg}/translate.c          |    0
>>  target/arm/tcg/user/helper.c              |  129 +
>>  target/arm/tcg/user/psci.c                |   26 +
>>  target/arm/{ => tcg}/vec_helper.c         |    0
>>  target/arm/{ => tcg}/vfp_helper.c         |  210 +-
>>  target/arm/{ => tcg}/translate-neon.c.inc |    0
>>  target/arm/{ => tcg}/translate-vfp.c.inc  |    0
>>  target/arm/meson.build                    |   42 +-
>>  target/arm/tcg/meson.build                |   43 +
>>  target/arm/tcg/softmmu/meson.build        |    5 +
>>  target/arm/tcg/softmmu/trace-events       |   10 +
>>  target/arm/tcg/trace-events               |    2 +
>>  target/arm/tcg/user/meson.build           |    5 +
>>  target/arm/trace-events                   |    9 -
>>  58 files changed, 7427 insertions(+), 6957 deletions(-)
>>  create mode 100644 target/arm/tcg/helper-tcg.h
>>  create mode 100644 target/arm/tcg/softmmu/trace.h
>>  create mode 100644 target/arm/tcg/trace.h
>>  rename target/arm/{ => tcg}/translate-a64.h (100%)
>>  rename target/arm/{ => tcg}/translate.h (100%)
>>  rename target/arm/{ => tcg}/vec_internal.h (100%)
>>  rename target/arm/{ => tcg}/a32-uncond.decode (100%)
>>  rename target/arm/{ => tcg}/a32.decode (100%)
>>  rename target/arm/{ => tcg}/m-nocp.decode (100%)
>>  rename target/arm/{ => tcg}/neon-dp.decode (100%)
>>  rename target/arm/{ => tcg}/neon-ls.decode (100%)
>>  rename target/arm/{ => tcg}/neon-shared.decode (100%)
>>  rename target/arm/{ => tcg}/sve.decode (100%)
>>  rename target/arm/{ => tcg}/t16.decode (100%)
>>  rename target/arm/{ => tcg}/t32.decode (100%)
>>  rename target/arm/{ => tcg}/vfp-uncond.decode (100%)
>>  rename target/arm/{ => tcg}/vfp.decode (100%)
>>  create mode 100644 target/arm/cpu-common.c
>>  create mode 100644 target/arm/cpu-softmmu.c
>>  create mode 100644 target/arm/cpu-user.c
>>  create mode 100644 target/arm/cpu-vfp.c
>>  create mode 100644 target/arm/get-phys-addr.c
>>  rename target/arm/{ => tcg}/crypto_helper.c (100%)
>>  rename target/arm/{ => tcg}/debug_helper.c (100%)
>>  rename target/arm/{ => tcg}/helper-a64.c (100%)
>>  rename target/arm/{ => tcg}/helper.c (53%)
>>  rename target/arm/{ => tcg}/iwmmxt_helper.c (100%)
>>  rename target/arm/{ => tcg}/m_helper.c (100%)
>>  rename target/arm/{ => tcg}/mte_helper.c (100%)
>>  rename target/arm/{ => tcg}/neon_helper.c (100%)
>>  rename target/arm/{ => tcg}/op_helper.c (100%)
>>  rename target/arm/{ => tcg}/pauth_helper.c (100%)
>>  create mode 100644 target/arm/tcg/softmmu/helper.c
>>  rename target/arm/{ => tcg/softmmu}/psci.c (100%)
>>  rename target/arm/{ => tcg}/sve_helper.c (100%)
>>  create mode 100644 target/arm/tcg/tcg-stub.c
>>  rename target/arm/{ => tcg}/tlb_helper.c (100%)
>>  rename target/arm/{ => tcg}/translate-a64.c (100%)
>>  rename target/arm/{ => tcg}/translate-sve.c (100%)
>>  rename target/arm/{ => tcg}/translate.c (100%)
>>  create mode 100644 target/arm/tcg/user/helper.c
>>  create mode 100644 target/arm/tcg/user/psci.c
>>  rename target/arm/{ => tcg}/vec_helper.c (100%)
>>  rename target/arm/{ => tcg}/vfp_helper.c (84%)
>>  rename target/arm/{ => tcg}/translate-neon.c.inc (100%)
>>  rename target/arm/{ => tcg}/translate-vfp.c.inc (100%)
>>  create mode 100644 target/arm/tcg/meson.build
>>  create mode 100644 target/arm/tcg/softmmu/meson.build
>>  create mode 100644 target/arm/tcg/softmmu/trace-events
>>  create mode 100644 target/arm/tcg/trace-events
>>  create mode 100644 target/arm/tcg/user/meson.build
> 
>