On 10/12/2018 10:56, Mark Rutland wrote:
> 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.
>
> [...]
Sure, I'll update to avoid referring to it as a page. Although note that
when mapping it into the guest we can obviously only map in page sized
granules, so in practise the LPT structure is contained within a entire
page given to the guest...
>> + 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;
> };
>
> [...]
>
>> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
>> index 8bf259dae9f6..fd3a2caabeb2 100644
>> --- a/include/linux/kvm_types.h
>> +++ b/include/linux/kvm_types.h
>> @@ -49,6 +49,8 @@ typedef unsigned long gva_t;
>> typedef u64 gpa_t;
>> typedef u64 gfn_t;
>>
>> +#define GPA_INVALID -1
>
> 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)
>
> [...]
I'll go with the latter as I think that's clearer.
>> +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.
Fair enough
> [...]
>
>> +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;
Yes, much clearer - no need for the comment :)
Thanks,
Steve
> Thanks,
> Mark.
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
