On Wed, Nov 28, 2018 at 02:45:21PM +0000, Steven Price wrote:

Provide a method for a guest to derive a paravirtualized counter/timer
which isn't dependent on the host's counter frequency. This allows a
guest to be migrated onto a new host which doesn't have the same
frequency without the virtual counter being disturbed.

I have a number of concerns about paravirtualizing the timer frequency,
but I'll bring that up in reply to the cover letter.

I have some orthogonal comments below.

The host provides a shared page which contains coefficients that can be
used to map the real counter from the host (the Arm "virtual counter")
to a paravirtualized view of time. On migration the new host updates the
coefficients to ensure that the guests view of time (after using the
coefficients) doesn't change and that the derived counter progresses at
the same real frequency.

Can we please avoid using the term 'page' here?

There is a data structure in shared memory, but it is not page-sized,
and referring to it as a page here and elsewhere is confusing. The spec
never uses the term 'page'

Could we please say something like:

  The host provides a datastrucutre in shared memory which ...

... to avoid the implication this is page sized/aligned etc.

[...]

+	struct kvm_arch_pvtime {
+		void *pv_page;
+
+		gpa_t lpt_page;
+		u32 lpt_fpv;
+	} pvtime;

To remove the page terminology, perhaps something like:

	struct kvm_arch_pvtime {
 		struct lpt	*lpt;
 		gpa_t		lpt_gpa;
 		u32		lpt_fpv;
 	};

[...]

To avoid any fun with signed/unsigned comparison, can we please make
this:

#define GPA_INVALID	((gpa_t)-1)

... or:

#define GPA_INVALID     (~(gpa_t)0)

[...]

+static void update_vtimer_cval(struct kvm *kvm, u32 previous_rate)
+{
+	u32 current_rate = arch_timer_get_rate();
+	u64 current_time = kvm_phys_timer_read();
+	int i;
+	struct kvm_vcpu *vcpu;
+	u64 rel_cval;
+
+	/* Early out if there's nothing to do */
+	if (likely(previous_rate == current_rate))
+		return;

Given this only happens on migration, I don't think we need to care
about likely/unlikely here, and can drop that from the condition.

[...]

+int kvm_arm_update_lpt_sequence(struct kvm *kvm)
+{
+	struct pvclock_vm_time_info *pvclock;
+	u64 lpt_ipa = kvm->arch.pvtime.lpt_page;
+	u64 native_freq, pv_freq, scale_mult, div_by_pv_freq_mult;
+	u64 shift = 0;
+	u64 sequence_number = 0;
+
+	if (lpt_ipa == GPA_INVALID)
+		return -EINVAL;
+
+	/* Page address must be 64 byte aligned */
+	if (lpt_ipa & 63)
+		return -EINVAL;

Please use IS_ALIGNED(), e.g.

	if (!IS_ALIGNED(lpt_ipa, 64))
		return -EINVAL;

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel