Re: [PATCH 4/8] i386: hvf: Implement CPU kick

[Paolo Bonzini <pbonzini@redhat.com>]

Ok, I'll review the patch to see what you've implemented.  Thanks!

Paolo

On 30/06/20 12:12, Roman Bolshakov wrote:
> On Mon, Jun 29, 2020 at 04:18:46PM +0200, Paolo Bonzini wrote:
>> On 29/06/20 16:04, Roman Bolshakov wrote:
>>> My approach is based
>>> hv_vcpu_run() and should hopefully work almost anywhere where
>>> Hypervisor.framework is available because Hypervisor framework exposes
>>> timer value
>>> (https://developer.apple.com/documentation/hypervisor/vmcs_guest_vmx_timer_value)
>>> since macOS 10.10.3+.
>>
>> There are a few other constants for which it would be unwise to write
>> from userspace, so that's not a big consolation. :)
>>
> 
> Hi Paolo,
> 
> So, I've tried Big Sur Beta and it has exactly the same performance
> issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
> worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.
> 
>>> I can also test how hv_vcpu_run_until() performs with HV_DEADLINE_FOREVER
>>> on the Beta. And if the performance issues with VMX-preemption timer and
>>> hv_vcpu_run_until() are fixed there.
>>
>> Thanks!  The main thing to test on Big Sur would be: 1) whether the
>> preemption timer bit in the pin controls "sticks" to 0 after setting it
> 
> It does not. If it's set, it stays there.
> 
>> 2) whether the bit reads back as zero after
>> hv_vcpu_run_until(HV_DEADLINE_FOREVER).
>>
> 
> Likewise, it's not cleared if set.
> 
> As far as I understand, hv_vcpu_run_until(HV_DEADLINE_FOREVER) works
> like hv_vcpu_run() without VMX-preemption timer. Otherwise
> hv_vcpu_run_until() implicitly sets VMX-preemption timer Pin-based
> control and sets the timer value.
> 
> Thanks,
> Roman
> 
> Here's the patch over v2 that adds support of hv_vcpu_run_until() on Big Sur:
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index 317304aa1d..ad202f7358 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -72,8 +72,12 @@
>  #include "sysemu/accel.h"
>  #include "target/i386/cpu.h"
>  
> +#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
> +#define HVF_MAX_DEADLINE HV_DEADLINE_FOREVER
> +#else
>  /* Maximum value of VMX-preemption timer */
>  #define HVF_MAX_DEADLINE UINT32_MAX
> +#endif
>  
>  HVFState *hvf_state;
>  
> @@ -693,6 +697,7 @@ int hvf_vcpu_exec(CPUState *cpu)
>      CPUX86State *env = &x86_cpu->env;
>      int ret = 0;
>      uint64_t rip = 0;
> +    hv_return_t r;
>  
>      if (hvf_process_events(cpu)) {
>          return EXCP_HLT;
> @@ -718,10 +723,22 @@ int hvf_vcpu_exec(CPUState *cpu)
>          /* Use VMX-preemption timer trick only if available */
>          if (rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS) &
>              VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER) {
> +#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
> +            r = hv_vcpu_run_until(cpu->hvf_fd,
> +                                  atomic_read(&env->hvf_deadline));
> +        } else {
> +            /*
> +             * Equivalent to behaviour of hv_vcpu_run() with VMX-preemption
> +             * timer disabled, prone to kick loss.
> +             */
> +            r = hv_vcpu_run_until(cpu->hvf_fd, HVF_MAX_DEADLINE);
> +        }
> +#else
>              wvmcs(cpu->hvf_fd, VMCS_PREEMPTION_TIMER_VALUE,
>                    atomic_read(&env->hvf_deadline));
>          }
> -        hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
> +        r = hv_vcpu_run(cpu->hvf_fd);
> +#endif
>          atomic_set(&env->hvf_deadline, HVF_MAX_DEADLINE);
>          assert_hvf_ok(r);
>  
>