RE: [EXTERNAL] Re: [PATCH] target/arm/whpx: fix whpx-arm reset hang

> > +    { WHvArm64RegisterCntvCvalEl0, WHPX_SYSREG(14, 3, 3, 3, 2) },
> > +    { WHvArm64RegisterSpEl1, WHPX_SYSREG(4, 1, 3, 4, 0) },
> > };
> 
> Ow, carried this mess in formats from HVF, don’t think we wanna keep this
> really.
> 
> (switching to the ENCODE_AA64_CP_REG format across the board would be
> better)

I'm happy to do that.

> > +
> > +    if (level == WHPX_LEVEL_RESET_STATE) {
> > +        /*
> > +         * WHPX keeps the "vCPU is in a low-power wait" state (entered when
> the
> > +         * guest executes WFI/WFE) in WHvRegisterInternalActivityState, and
> that
> > +         * state is not affected by writing the architectural registers above.
> > +         * If the guest happened to be idle when the reset was requested, the
> > +         * vCPU would stay suspended forever.
> > +         *
> > +         * StartupSuspend is the bit WHP uses to hold a vCPU that has not been
> > +         * started yet, and it is how secondary vCPUs are parked until PSCI
> > +         * CPU_ON releases them.
> > +         */
> > +        WHV_REGISTER_NAME name = WHvRegisterInternalActivityState;
> > +        bool powered_off = arm_cpu->power_state == PSCI_OFF;
> > +        WHV_REGISTER_VALUE ia;
> > +        HRESULT hr;
> > +
> > +        clean_whv_register_value(&ia);
> > +        hr = whp_dispatch.WHvGetVirtualProcessorRegisters(
> > +            whpx_global.partition, cpu->cpu_index, &name, 1, &ia);
> > +        if (SUCCEEDED(hr) &&
> > +            (ia.InternalActivity.IdleSuspend ||
> > +             ia.InternalActivity.HaltSuspend ||
> > +             ia.InternalActivity.StartupSuspend != powered_off)) {
> > +            ia.InternalActivity.IdleSuspend = 0;
> > +            ia.InternalActivity.HaltSuspend = 0;
> > +            ia.InternalActivity.StartupSuspend = powered_off;
> > +            whpx_set_reg(cpu, WHvRegisterInternalActivityState, ia);
> > +        }
> > +    }
> > }
> 
> Looks good :) Although perhaps this is better as a separate commit from the
> change above.

Happy to do that too.

> But not this bit, which should be dropped.
> 
> The CPU model gets synced through this bit and registers that error out
> through ARM_CP_NO_RAW should be behind a #ifdef SYNC_NO_RAW_REGS
> block.
> 
> The former characteristic is what allows for example -cpu cortex-a72 to be
> used on WHPX instead of only the native machine, while masking off the
> newer feature from feature regs.
> 
> > +        if (ri && !(ri->type & (ARM_CP_NO_RAW | ARM_CP_CONST))) {
> >             whpx_sreg_match[i].cp_idx = sregs_cnt;
> >             arm_cpu->cpreg_indexes[sregs_cnt++] = cpreg_to_kvm_id(key);
> >         } else {

I'll need a bit of advice in this case. Without this change, we start getting
assertion failures after fixing ENCODE_AA64_CP_REG parameter ordering, due to
register read-back not matching after write. Do you have a recommendation on
how to approach this?