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

[Alexey Kardashevskiy]

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?



-- 
Alexey

signature.asc
Description: OpenPGP digital signature