| Message ID | 20210330145430.996981-1-maz@kernel.org |
|---|---|
| Headers | show |
| Series | KVM: arm64: Add host/guest KVM-PTP support | expand |
On 2021/3/30 22:54, Marc Zyngier wrote: > +int kvm_arch_ptp_init(void) > +{ > + int ret; > + > + ret = kvm_arm_hyp_service_available(ARM_SMCCC_KVM_FUNC_PTP); > + if (ret <= 0) kvm_arm_hyp_service_available() returns boolean. Maybe write as ? bool ret; ret = kvm_arm_hyp_service_available(); if (!ret) return -ENODEV; > + return -EOPNOTSUPP; > + > + return 0; > +}
On Sat, 17 Apr 2021 09:42:37 +0100, Zenghui Yu <yuzenghui@huawei.com> wrote: > > On 2021/3/30 22:54, Marc Zyngier wrote: > > +int kvm_arch_ptp_init(void) > > +{ > > + int ret; > > + > > + ret = kvm_arm_hyp_service_available(ARM_SMCCC_KVM_FUNC_PTP); > > + if (ret <= 0) > > kvm_arm_hyp_service_available() returns boolean. Maybe write as ? > > bool ret; > > ret = kvm_arm_hyp_service_available(); > if (!ret) > return -ENODEV; Fixed in 300bb1fe7671, as previously reported by Dan Carpenter in [1]. Thanks, M. https://lore.kernel.org/r/20210331043704.GG2065@kadam -- Without deviation from the norm, progress is not possible.
On 2021/3/30 22:54, Marc Zyngier wrote:
> #define SMCCC_ARCH_WORKAROUND_RET_UNAFFECTED 1
I think it'd be better to keep this definition together with other
wa Function IDs. It's only a cosmetic comment anyway.
Zenghui
Hi Marc, Jianyong, On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > From: Jianyong Wu <jianyong.wu@arm.com> > > Currently, there is no mechanism to keep time sync between guest and host > in arm/arm64 virtualization environment. Time in guest will drift compared > with host after boot up as they may both use third party time sources > to correct their time respectively. The time deviation will be in order > of milliseconds. But in some scenarios,like in cloud environment, we ask > for higher time precision. > > kvm ptp clock, which chooses the host clock source as a reference > clock to sync time between guest and host, has been adopted by x86 > which takes the time sync order from milliseconds to nanoseconds. > > This patch enables kvm ptp clock for arm/arm64 and improves clock sync precision > significantly. > --- a/drivers/ptp/Kconfig > +++ b/drivers/ptp/Kconfig > @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > config PTP_1588_CLOCK_KVM > tristate "KVM virtual PTP clock" > depends on PTP_1588_CLOCK > - depends on KVM_GUEST && X86 > + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER) Why does this not depend on KVM_GUEST on ARM? I.e. shouldn't the dependency be: KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) ? > default y > help > This driver adds support for using kvm infrastructure as a PTP Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 2021-05-11 10:07, Geert Uytterhoeven wrote: > Hi Marc, Jianyong, > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: >> From: Jianyong Wu <jianyong.wu@arm.com> >> >> Currently, there is no mechanism to keep time sync between guest and >> host >> in arm/arm64 virtualization environment. Time in guest will drift >> compared >> with host after boot up as they may both use third party time sources >> to correct their time respectively. The time deviation will be in >> order >> of milliseconds. But in some scenarios,like in cloud environment, we >> ask >> for higher time precision. >> >> kvm ptp clock, which chooses the host clock source as a reference >> clock to sync time between guest and host, has been adopted by x86 >> which takes the time sync order from milliseconds to nanoseconds. >> >> This patch enables kvm ptp clock for arm/arm64 and improves clock sync >> precision >> significantly. > >> --- a/drivers/ptp/Kconfig >> +++ b/drivers/ptp/Kconfig >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH >> config PTP_1588_CLOCK_KVM >> tristate "KVM virtual PTP clock" >> depends on PTP_1588_CLOCK >> - depends on KVM_GUEST && X86 >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && >> ARM_ARCH_TIMER) > > Why does this not depend on KVM_GUEST on ARM? > I.e. shouldn't the dependency be: > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > ? arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, and KVM/arm64 doesn't know about this configuration symbol. Thanks, M. -- Jazz is not dead. It just smells funny...
Hi Marc, On Tue, May 11, 2021 at 11:13 AM Marc Zyngier <maz@kernel.org> wrote: > On 2021-05-11 10:07, Geert Uytterhoeven wrote: > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > >> From: Jianyong Wu <jianyong.wu@arm.com> > >> > >> Currently, there is no mechanism to keep time sync between guest and > >> host > >> in arm/arm64 virtualization environment. Time in guest will drift > >> compared > >> with host after boot up as they may both use third party time sources > >> to correct their time respectively. The time deviation will be in > >> order > >> of milliseconds. But in some scenarios,like in cloud environment, we > >> ask > >> for higher time precision. > >> > >> kvm ptp clock, which chooses the host clock source as a reference > >> clock to sync time between guest and host, has been adopted by x86 > >> which takes the time sync order from milliseconds to nanoseconds. > >> > >> This patch enables kvm ptp clock for arm/arm64 and improves clock sync > >> precision > >> significantly. > > > >> --- a/drivers/ptp/Kconfig > >> +++ b/drivers/ptp/Kconfig > >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > >> config PTP_1588_CLOCK_KVM > >> tristate "KVM virtual PTP clock" > >> depends on PTP_1588_CLOCK > >> - depends on KVM_GUEST && X86 > >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && > >> ARM_ARCH_TIMER) > > > > Why does this not depend on KVM_GUEST on ARM? > > I.e. shouldn't the dependency be: > > > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > > > ? > > arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, > and KVM/arm64 doesn't know about this configuration symbol. OK. Does PTP_1588_CLOCK_KVM need to default to yes? Perhaps only on X86, to maintain the status quo? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Wed, 26 May 2021 08:52:42 +0100, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Tue, May 11, 2021 at 11:13 AM Marc Zyngier <maz@kernel.org> wrote: > > On 2021-05-11 10:07, Geert Uytterhoeven wrote: > > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > > >> From: Jianyong Wu <jianyong.wu@arm.com> > > > > > >> --- a/drivers/ptp/Kconfig > > >> +++ b/drivers/ptp/Kconfig > > >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > > >> config PTP_1588_CLOCK_KVM > > >> tristate "KVM virtual PTP clock" > > >> depends on PTP_1588_CLOCK > > >> - depends on KVM_GUEST && X86 > > >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && > > >> ARM_ARCH_TIMER) > > > > > > Why does this not depend on KVM_GUEST on ARM? > > > I.e. shouldn't the dependency be: > > > > > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > > > > > ? > > > > arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, > > and KVM/arm64 doesn't know about this configuration symbol. > > OK. > > Does PTP_1588_CLOCK_KVM need to default to yes? > Perhaps only on X86, to maintain the status quo? I think I don't really understand the problem you are trying to solve. Is it that 'make oldconfig' now asks you about this new driver? Why is that an issue? Thanks, M. -- Without deviation from the norm, progress is not possible.
Hi Marc, On Wed, May 26, 2021 at 10:01 AM Marc Zyngier <maz@kernel.org> wrote: > On Wed, 26 May 2021 08:52:42 +0100, > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Tue, May 11, 2021 at 11:13 AM Marc Zyngier <maz@kernel.org> wrote: > > > On 2021-05-11 10:07, Geert Uytterhoeven wrote: > > > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > > > >> From: Jianyong Wu <jianyong.wu@arm.com> > > > > > > > >> --- a/drivers/ptp/Kconfig > > > >> +++ b/drivers/ptp/Kconfig > > > >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > > > >> config PTP_1588_CLOCK_KVM > > > >> tristate "KVM virtual PTP clock" > > > >> depends on PTP_1588_CLOCK > > > >> - depends on KVM_GUEST && X86 > > > >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && > > > >> ARM_ARCH_TIMER) > > > > > > > > Why does this not depend on KVM_GUEST on ARM? > > > > I.e. shouldn't the dependency be: > > > > > > > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > > > > > > > ? > > > > > > arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, > > > and KVM/arm64 doesn't know about this configuration symbol. > > > > OK. > > > > Does PTP_1588_CLOCK_KVM need to default to yes? > > Perhaps only on X86, to maintain the status quo? > > I think I don't really understand the problem you are trying to > solve. Is it that 'make oldconfig' now asks you about this new driver? > Why is that an issue? My first "problem" was that it asked about this new driver on arm/arm64, while I assumed there were some missing dependencies (configuring a kernel should not ask useless questions). That turned out to be a wrong assumption, so there is no such problem here. The second problem is "default y": code that is not critical should not be enabled by default. Hence my last question. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, 26 May 2021 09:18:27 +0100, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Marc, > > On Wed, May 26, 2021 at 10:01 AM Marc Zyngier <maz@kernel.org> wrote: > > On Wed, 26 May 2021 08:52:42 +0100, > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Tue, May 11, 2021 at 11:13 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On 2021-05-11 10:07, Geert Uytterhoeven wrote: > > > > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > > > > >> From: Jianyong Wu <jianyong.wu@arm.com> > > > > > > > > > >> --- a/drivers/ptp/Kconfig > > > > >> +++ b/drivers/ptp/Kconfig > > > > >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > > > > >> config PTP_1588_CLOCK_KVM > > > > >> tristate "KVM virtual PTP clock" > > > > >> depends on PTP_1588_CLOCK > > > > >> - depends on KVM_GUEST && X86 > > > > >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && > > > > >> ARM_ARCH_TIMER) > > > > > > > > > > Why does this not depend on KVM_GUEST on ARM? > > > > > I.e. shouldn't the dependency be: > > > > > > > > > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > > > > > > > > > ? > > > > > > > > arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, > > > > and KVM/arm64 doesn't know about this configuration symbol. > > > > > > OK. > > > > > > Does PTP_1588_CLOCK_KVM need to default to yes? > > > Perhaps only on X86, to maintain the status quo? > > > > I think I don't really understand the problem you are trying to > > solve. Is it that 'make oldconfig' now asks you about this new driver? > > Why is that an issue? > > My first "problem" was that it asked about this new driver on > arm/arm64, while I assumed there were some missing dependencies > (configuring a kernel should not ask useless questions). That turned > out to be a wrong assumption, so there is no such problem here. > > The second problem is "default y": code that is not critical should > not be enabled by default. Hence my last question. I think consistency between architectures is important. Certainly, distributions depend on that, and we otherwise end-up with distro kernels missing functionalities. The notion of "critical" is also pretty relative. defconfig contains a gazillion of things that are not critical to most people, for example, and yet misses a bunch of things that are needed to boot on some of my systems. That's just to say that I find it difficult to make that choice from the PoV of a kernel hacker. I'm personally more inclined to leave things enabled and let people *disable* things if they want to reduce the footprint of their kernel. Thanks, M. -- Without deviation from the norm, progress is not possible.
Hi Marc, On Wed, May 26, 2021 at 10:32 AM Marc Zyngier <maz@kernel.org> wrote: > On Wed, 26 May 2021 09:18:27 +0100, > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, May 26, 2021 at 10:01 AM Marc Zyngier <maz@kernel.org> wrote: > > > On Wed, 26 May 2021 08:52:42 +0100, > > > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Tue, May 11, 2021 at 11:13 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > On 2021-05-11 10:07, Geert Uytterhoeven wrote: > > > > > > On Tue, Mar 30, 2021 at 4:56 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > >> From: Jianyong Wu <jianyong.wu@arm.com> > > > > > > > > > > > >> --- a/drivers/ptp/Kconfig > > > > > >> +++ b/drivers/ptp/Kconfig > > > > > >> @@ -108,7 +108,7 @@ config PTP_1588_CLOCK_PCH > > > > > >> config PTP_1588_CLOCK_KVM > > > > > >> tristate "KVM virtual PTP clock" > > > > > >> depends on PTP_1588_CLOCK > > > > > >> - depends on KVM_GUEST && X86 > > > > > >> + depends on (KVM_GUEST && X86) || (HAVE_ARM_SMCCC_DISCOVERY && > > > > > >> ARM_ARCH_TIMER) > > > > > > > > > > > > Why does this not depend on KVM_GUEST on ARM? > > > > > > I.e. shouldn't the dependency be: > > > > > > > > > > > > KVM_GUEST && (X86 || (HAVE_ARM_SMCCC_DISCOVERY && ARM_ARCH_TIMER)) > > > > > > > > > > > > ? > > > > > > > > > > arm/arm64 do not select KVM_GUEST. Any kernel can be used for a guest, > > > > > and KVM/arm64 doesn't know about this configuration symbol. > > > > > > > > OK. > > > > > > > > Does PTP_1588_CLOCK_KVM need to default to yes? > > > > Perhaps only on X86, to maintain the status quo? > > > > > > I think I don't really understand the problem you are trying to > > > solve. Is it that 'make oldconfig' now asks you about this new driver? > > > Why is that an issue? > > > > My first "problem" was that it asked about this new driver on > > arm/arm64, while I assumed there were some missing dependencies > > (configuring a kernel should not ask useless questions). That turned > > out to be a wrong assumption, so there is no such problem here. > > > > The second problem is "default y": code that is not critical should > > not be enabled by default. Hence my last question. > > I think consistency between architectures is important. Certainly, > distributions depend on that, and we otherwise end-up with distro > kernels missing functionalities. > > The notion of "critical" is also pretty relative. defconfig contains a I'm not talking about defconfig, but about "default y" in defconfig. > gazillion of things that are not critical to most people, for example, > and yet misses a bunch of things that are needed to boot on some of my > systems. Perhaps those should be added, so those systems can be tested using defconfig? At least for arm64, I think that's aligned with the arm64 defconfig policy. > That's just to say that I find it difficult to make that choice from > the PoV of a kernel hacker. I'm personally more inclined to leave > things enabled and let people *disable* things if they want to reduce > the footprint of their kernel. The standard question to respond to w.r.t. "default y" is: "Why is your feature so special that it needs to be enabled by default?", which implies "default y" is the exception, not the rule. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Given that this series[0] has languished in my Inbox for the best of the past two years, and in an effort to eventually get it merged, I've taken the liberty to pick it up and do the changes I wanted to see instead of waiting to go through yet another round. All the patches have a link to their original counterpart (though I have squashed a couple of them where it made sense). Tested both 64 and 32bit guests for a good measure. Of course, I claim full responsibility for any bug introduced here. Unless someone screams now, this is going in 5.13, because I'm frankly fed up with it! ;-) * From v18 [2] - Fix kvm_hypercall2() return type - Rebased on top of 5.12-rc3 - Added RBs * From v17 [1]: - Fixed compilation issue on 32bit systems not selecting CONFIG_HAVE_ARM_SMCCC_DISCOVERY - Fixed KVM service discovery not properly parsing the reply from the hypervisor * From v16 [0]: - Moved the KVM service discovery to its own file, plugged it into PSCI instead of the arch code, dropped the inlining, made use of asm/hypervisor.h. - Tidied-up the namespacing - Cleanup the hypercall handler - De-duplicate the guest code - Tidied-up arm64-specific documentation - Dropped the generic PTP documentation as it needs a new location, and some cleanup - Squashed hypercall documentation and capability into the main KVM patch - Rebased on top of 5.11-rc4 [0] https://lore.kernel.org/r/20201209060932.212364-1-jianyong.wu@arm.com [1] https://lore.kernel.org/r/20210202141204.3134855-1-maz@kernel.org [2] https://lore.kernel.org/r/20210208134029.3269384-1-maz@kernel.org Jianyong Wu (4): ptp: Reorganize ptp_kvm.c to make it arch-independent clocksource: Add clocksource id for arm arch counter KVM: arm64: Add support for the KVM PTP service ptp: arm/arm64: Enable ptp_kvm for arm/arm64 Thomas Gleixner (1): time: Add mechanism to recognize clocksource in time_get_snapshot Will Deacon (2): arm/arm64: Probe for the presence of KVM hypervisor KVM: arm64: Advertise KVM UID to guests via SMCCC Documentation/virt/kvm/api.rst | 10 +++ Documentation/virt/kvm/arm/index.rst | 1 + Documentation/virt/kvm/arm/ptp_kvm.rst | 25 ++++++ arch/arm/include/asm/hypervisor.h | 3 + arch/arm64/include/asm/hypervisor.h | 3 + arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/hypercalls.c | 80 +++++++++++++++-- drivers/clocksource/arm_arch_timer.c | 36 ++++++++ drivers/firmware/psci/psci.c | 2 + drivers/firmware/smccc/Makefile | 2 +- drivers/firmware/smccc/kvm_guest.c | 50 +++++++++++ drivers/firmware/smccc/smccc.c | 1 + drivers/ptp/Kconfig | 2 +- drivers/ptp/Makefile | 2 + drivers/ptp/ptp_kvm_arm.c | 28 ++++++ drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} | 84 +++++------------- drivers/ptp/ptp_kvm_x86.c | 97 +++++++++++++++++++++ include/linux/arm-smccc.h | 41 +++++++++ include/linux/clocksource.h | 6 ++ include/linux/clocksource_ids.h | 12 +++ include/linux/ptp_kvm.h | 19 ++++ include/linux/timekeeping.h | 12 +-- include/uapi/linux/kvm.h | 1 + kernel/time/clocksource.c | 2 + kernel/time/timekeeping.c | 1 + 25 files changed, 443 insertions(+), 78 deletions(-) create mode 100644 Documentation/virt/kvm/arm/ptp_kvm.rst create mode 100644 drivers/firmware/smccc/kvm_guest.c create mode 100644 drivers/ptp/ptp_kvm_arm.c rename drivers/ptp/{ptp_kvm.c => ptp_kvm_common.c} (60%) create mode 100644 drivers/ptp/ptp_kvm_x86.c create mode 100644 include/linux/clocksource_ids.h create mode 100644 include/linux/ptp_kvm.h