[PATCH 07/16] KVM: arm64: timers: Allow physical offset without CNTPOFF_EL2

[Colton Lewis]

Marc Zyngier <maz at kernel.org> writes:

> +/* If _pred is true, set bit in _set, otherwise set it in _clr */
> +#define assign_clear_set_bit(_pred, _bit, _clr, _set)			\
> +	do {								\
> +		if (_pred)						\
> +			(_set) |= (_bit);				\
> +		else							\
> +			(_clr) |= (_bit);				\
> +	} while (0)
> +

I don't think the do-while wrapper is necessary. Is there any reason
besides style guide conformance?

> +	/*
> +	 * We have two possibility to deal with a physical offset:
> +	 *
> +	 * - Either we have CNTPOFF (yay!) or the offset is 0:
> +	 *   we let the guest freely access the HW
> +	 *
> +	 * - or neither of these condition apply:
> +	 *   we trap accesses to the HW, but still use it
> +	 *   after correcting the physical offset
> +	 */
> +	if (!has_cntpoff() && timer_get_offset(map->direct_ptimer))
> +		tpt = tpc = true;

If there are only two possibilites, then two different booleans makes
things more complicated than it has to be.

> +	assign_clear_set_bit(tpt, CNTHCTL_EL1PCEN << 10, set, clr);
> +	assign_clear_set_bit(tpc, CNTHCTL_EL1PCTEN << 10, set, clr);

Might be good to name the 10 something like VHE_SHIFT so people know why
it is applied.

> +
> +
> +	timer_set_traps(vcpu, &map);
>   }

>   bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu)
> @@ -1293,27 +1363,12 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>   }

>   /*
> - * On VHE system, we only need to configure the EL2 timer trap register  
> once,
> - * not for every world switch.
> - * The host kernel runs at EL2 with HCR_EL2.TGE == 1,
> - * and this makes those bits have no effect for the host kernel  
> execution.
> + * If we have CNTPOFF, permanently set ECV to enable it.
>    */
>   void kvm_timer_init_vhe(void)
>   {
> -	/* When HCR_EL2.E2H ==1, EL1PCEN and EL1PCTEN are shifted by 10 */
> -	u32 cnthctl_shift = 10;
> -	u64 val;
> -
> -	/*
> -	 * VHE systems allow the guest direct access to the EL1 physical
> -	 * timer/counter.
> -	 */
> -	val = read_sysreg(cnthctl_el2);
> -	val |= (CNTHCTL_EL1PCEN << cnthctl_shift);
> -	val |= (CNTHCTL_EL1PCTEN << cnthctl_shift);
>   	if (cpus_have_final_cap(ARM64_HAS_ECV_CNTPOFF))
> -		val |= CNTHCTL_ECV;
> -	write_sysreg(val, cnthctl_el2);
> +		sysreg_clear_set(cntkctl_el1, 0, CNTHCTL_ECV);
>   }

What is the reason for moving these register writes from initialization
to vcpu load time? This contradicts the comment that says this is only
needed once and not at every world switch. Seems like doing more work
for no reason.