Re: [Qemu-ppc] [RFC 11/17] ppc: Add ppc_set_compat_all()

[Alexey Kardashevskiy]

On 09/11/16 14:52, David Gibson wrote:
> On Wed, Nov 09, 2016 at 12:27:47PM +1100, Alexey Kardashevskiy wrote:
>> On 08/11/16 16:18, David Gibson wrote:
>>> On Fri, Nov 04, 2016 at 03:01:40PM +1100, Alexey Kardashevskiy wrote:
>>>> On 30/10/16 22:12, David Gibson wrote:
>>>>> Once a compatiblity mode is negotiated with the guest,
>>>>> h_client_architecture_support() uses run_on_cpu() to update each CPU to
>>>>> the new mode.  We're going to want this logic somewhere else shortly,
>>>>> so make a helper function to do this global update.
>>>>>
>>>>> We put it in target-ppc/compat.c - it makes as much sense at the CPU level
>>>>> as it does at the machine level.  We also move the cpu_synchronize_state()
>>>>> into ppc_set_compat(), since it doesn't really make any sense to call that
>>>>> without synchronizing state.
>>>>>
>>>>> Signed-off-by: David Gibson <address@hidden>
>>>>> ---
>>>>>  hw/ppc/spapr_hcall.c | 31 +++++--------------------------
>>>>>  target-ppc/compat.c  | 36 ++++++++++++++++++++++++++++++++++++
>>>>>  target-ppc/cpu.h     |  3 +++
>>>>>  3 files changed, 44 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>>>> index 3bd6d06..4eaf9a6 100644
>>>>> --- a/hw/ppc/spapr_hcall.c
>>>>> +++ b/hw/ppc/spapr_hcall.c
>>>>> @@ -881,20 +881,6 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, 
>>>>> sPAPRMachineState *spapr,
>>>>>      return ret;
>>>>>  }
>>>>>  
>>>>> -typedef struct {
>>>>> -    uint32_t compat_pvr;
>>>>> -    Error *err;
>>>>> -} SetCompatState;
>>>>> -
>>>>> -static void do_set_compat(CPUState *cs, void *arg)
>>>>> -{
>>>>> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>> -    SetCompatState *s = arg;
>>>>> -
>>>>> -    cpu_synchronize_state(cs);
>>>>> -    ppc_set_compat(cpu, s->compat_pvr, &s->err);
>>>>> -}
>>>>> -
>>>>>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>>>>>                                                    sPAPRMachineState 
>>>>> *spapr,
>>>>>                                                    target_ulong opcode,
>>>>> @@ -902,7 +888,6 @@ static target_ulong 
>>>>> h_client_architecture_support(PowerPCCPU *cpu,
>>>>>  {
>>>>>      target_ulong list = ppc64_phys_to_real(args[0]);
>>>>>      target_ulong ov_table;
>>>>> -    CPUState *cs;
>>>>>      bool explicit_match = false; /* Matched the CPU's real PVR */
>>>>>      uint32_t max_compat = cpu->max_compat;
>>>>>      uint32_t best_compat = 0;
>>>>> @@ -949,18 +934,12 @@ static target_ulong 
>>>>> h_client_architecture_support(PowerPCCPU *cpu,
>>>>>  
>>>>>      /* Update CPUs */
>>>>>      if (cpu->compat_pvr != best_compat) {
>>>>> -        CPU_FOREACH(cs) {
>>>>> -            SetCompatState s = {
>>>>> -                .compat_pvr = best_compat,
>>>>> -                .err = NULL,
>>>>> -            };
>>>>> +        Error *local_err = NULL;
>>>>>  
>>>>> -            run_on_cpu(cs, do_set_compat, &s);
>>>>> -
>>>>> -            if (s.err) {
>>>>> -                error_report_err(s.err);
>>>>> -                return H_HARDWARE;
>>>>> -            }
>>>>> +        ppc_set_compat_all(best_compat, &local_err);
>>>>> +        if (local_err) {
>>>>> +            error_report_err(local_err);
>>>>> +            return H_HARDWARE;
>>>>>          }
>>>>>      }
>>>>>  
>>>>> diff --git a/target-ppc/compat.c b/target-ppc/compat.c
>>>>> index 1059555..0b12b58 100644
>>>>> --- a/target-ppc/compat.c
>>>>> +++ b/target-ppc/compat.c
>>>>> @@ -124,6 +124,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t 
>>>>> compat_pvr, Error **errp)
>>>>>          pcr = compat->pcr;
>>>>>      }
>>>>>  
>>>>> +    cpu_synchronize_state(CPU(cpu));
>>>>> +
>>>>>      cpu->compat_pvr = compat_pvr;
>>>>>      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>>>>>  
>>>>> @@ -136,6 +138,40 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t 
>>>>> compat_pvr, Error **errp)
>>>>>      }
>>>>>  }
>>>>>  
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>> +typedef struct {
>>>>> +    uint32_t compat_pvr;
>>>>> +    Error *err;
>>>>> +} SetCompatState;
>>>>> +
>>>>> +static void do_set_compat(CPUState *cs, void *arg)
>>>>> +{
>>>>> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>>> +    SetCompatState *s = arg;
>>>>> +
>>>>> +    ppc_set_compat(cpu, s->compat_pvr, &s->err);
>>>>> +}
>>>>> +
>>>>> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
>>>>> +{
>>>>> +    CPUState *cs;
>>>>> +
>>>>> +    CPU_FOREACH(cs) {
>>>>> +        SetCompatState s = {
>>>>> +            .compat_pvr = compat_pvr,
>>>>> +            .err = NULL,
>>>>> +        };
>>>>> +
>>>>> +        run_on_cpu(cs, do_set_compat, &s);
>>>>> +
>>>>> +        if (s.err) {
>>>>> +            error_propagate(errp, s.err);
>>>>> +            return;
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>>  int ppc_compat_max_threads(PowerPCCPU *cpu)
>>>>>  {
>>>>>      const CompatInfo *compat = compat_by_pvr(cpu->compat_pvr);
>>>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>>>> index 91e8be8..201a655 100644
>>>>> --- a/target-ppc/cpu.h
>>>>> +++ b/target-ppc/cpu.h
>>>>> @@ -1317,6 +1317,9 @@ static inline int cpu_mmu_index (CPUPPCState *env, 
>>>>> bool ifetch)
>>>>>  bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
>>>>>                        uint32_t min_compat_pvr, uint32_t max_compat_pvr);
>>>>>  void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
>>>>> +#if !defined(CONFIG_USER_ONLY)
>>>>> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
>>>>> +#endif
>>>>
>>>> I would put all ppc*compat*() under #if defined(CONFIG_USER_ONLY) &&
>>>> defined(TARGET_PPC64) (or even moved this to target-ppc/Makefile.objs).
>>>
>>> I was originally going to do that, but decided against it.
>>>
>>>> Otherwise, functions like ppc_check_compat() have #if
>>>> !defined(CONFIG_USER_ONLY) which suggests that the rest of
>>>> ppc_check_compat() can actually be executed in ppc64-linux-user (while it
>>>> cannot, can it?).
>>>
>>> It won't be, but there's no theoretical reason they couldn't be.  User
>>> mode, like spapr, doesn't execute hypervisor privilege code, and so
>>> the PCR isn't owned by the "guest" (if you can call the user mode
>>> executable that).  Which means it could make sense to set it from the
>>> outside, although that's not something we currently do.
>>
>> Compatibility modes are designed to disable sets of instructions to keep
>> working old userspace software which relies on some opcodes to be invalid.
>>
>> linux-user is TCG, right? The user can pick any CPU he likes if there is
>> need to run such an old software, why on earth would anyone bother with
>> this compat mode in linux-user?
> 
> True, I can't really see any reason to do that.
> 
> On the other hand, compat mode does at least make theoretical sense,
> whereas, for example, compat mode on powernv is fundamentally
> nonsense.  At this point I'm not terribly include to take away the
> (token) user-only support unless there's a compelling reason *not* to
> include it.

What would make a compelling reason? :)

This will make makefile simpler and will reduce number of #ifdef, and in
fact it is not supported now anyway, it has not even been tried.



-- 
Alexey

signature.asc
Description: OpenPGP digital signature