[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH RFC 1/3] x86/pvclock: add setter for pvclock_pvti_cpu0_va

On 12/28/2015 11:45 PM, Andy Lutomirski wrote:
> On Mon, Dec 28, 2015 at 1:52 PM, Joao Martins <joao.m.martins@xxxxxxxxxx> 
> wrote:
>> Right now there is only a pvclock_pvti_cpu0_va() which is defined on
>> kvmclock since:
>> commit dac16fba6fc5
>> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap")
>> The only user of this interface so far is kvm. This commit adds a setter
>> function for the pvti page and moves pvclock_pvti_cpu0_va to pvclock, which
>> is a more generic place to have it; and would allow other PV clocksources
>> to use it, such as Xen.
>> +
>> +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti)
>> +{
>> +       pvti_cpu0_va = pvti;
>> +}
> IMO this either wants to be __init or wants a
> WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)).  The latter hasn't landed in
> -tip yet, but I think it'll land next week unless the merge window
> opens early.
OK, I will add those two once it lands in -tip.

I had a silly mistake in this patch as I bindly ommited the parameter name to
keep checkpatch happy, but didn't compile check when built without PARAVIRT.
Apologies for that and will fix that also on the next version.

> It may pay to actually separate out the kvm-clock clocksource and
> rename it rather than partially duplicating it, assuming the result
> wouldn't be messy.
Not sure if I follow but I moved out pvclock_pvti_cpu0_va from kvm-clock or do
you mean to separate out kvm-clock in it's enterity, or something else within
kvm-clock is that is common to both (such as kvm_setup_vsyscall_timeinfo) ?

> Can you CC me on the rest of the series for new versions?
Sure! Thanks for the prompt reply.

> BTW, since this seems to require hypervisor changes to be useful, it
> might make sense to rethink the interface a bit.  Are you actually
> planning to support per-cpu pvti for this in any useful way?  If not,
> I think that this would work a whole lot better and be considerably
> less code if you had a single global pvti that lived in
> hypervisor-allocated memory instead of an array that lives in guest
> memory.  I'd be happy to discuss next week in more detail (currently
> on vacation).
Initially I had this series using per-cpu pvti's based on Linux 4.4 but since
that was removed in favor of vdso using solely cpu0 pvti, then I ended up just
registering the cpu 0 page. I don't intend to add per-cpu pvti's since it would
only be used for this case: (unless the reviewers think it should be done)
meaning I would register pvti's for the other CPUs without having them used.
Having a global pvti as you suggest it would get a lot simpler for the guest,
but I guess this would only work assuming PVCLOCK_TSC_STABLE_BIT is there?
Looking forward to discuss it next week.


> --Andy

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.