linux-kernel - RE: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out of staging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6E21E5352C11B742B20C142EB499E0481FC8D4@TK5EX14MBXC122.redmond.corp.microsoft.com>
Date:	Mon, 23 May 2011 18:50:42 +0000
From:	KY Srinivasan <kys@...rosoft.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	"gregkh@...e.de" <gregkh@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"johnstul@...ibm.com" <johnstul@...ibm.com>,
	"hch@...radead.org" <hch@...radead.org>,
	Hank Janssen <hjanssen@...rosoft.com>,
	Haiyang Zhang <haiyangz@...rosoft.com>
Subject: RE: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver
 out of staging



> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@...utronix.de]
> Sent: Monday, May 23, 2011 2:16 PM
> To: KY Srinivasan
> Cc: gregkh@...e.de; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; johnstul@...ibm.com; hch@...radead.org; Hank
> Janssen; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out
> of staging
> 
> On Mon, 23 May 2011, K. Y. Srinivasan wrote:
> > +
> > +#include <linux/version.h>
> > +#include <linux/clocksource.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/dmi.h>
> > +#include <asm/hyperv.h>
> > +#include <asm/mshyperv.h>
> > +#include <asm/hypervisor.h>
> > +
> > +#define HV_CLOCK_SHIFT	22
> 
> Please do not use hard coded conversion factors. See below

I will fix this.
> 
> > +static cycle_t read_hv_clock(struct clocksource *arg)
> > +{
> > +	cycle_t current_tick;
> > +	/*
> > +	 * Read the partition counter to get the current tick count. This count
> > +	 * is set to 0 when the partition is created and is incremented in
> > +	 * 100 nanosecond units.
> > +	 */
> > +	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> > +	return current_tick;
> > +}
> > +
> > +static struct clocksource hyperv_cs = {
> > +	.name           = "hyperv_clocksource",
> > +	.rating         = 400, /* use this when running on Hyperv*/
> > +	.read           = read_hv_clock,
> > +	.mask           = CLOCKSOURCE_MASK(64),
> > +	/*
> > +	 * The time ref counter in HyperV is in 100ns units.
> > +	 * The definition of mult is:
> > +	 * mult/2^shift = ns/cyc = 100
> > +	 * mult = (100 << shift)
> > +	 */
> > +	.mult           = (100 << HV_CLOCK_SHIFT),
> > +	.shift          = HV_CLOCK_SHIFT,
> > +};
> > +
> > +static const struct dmi_system_id __initconst
> > +hv_timesource_dmi_table[] __maybe_unused  = {
> > +	{
> > +		.ident = "Hyper-V",
> > +		.matches = {
> > +			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft
> Corporation"),
> > +			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
> > +			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
> > +		},
> > +	},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(dmi, hv_timesource_dmi_table);
> > +
> > +static const struct pci_device_id __initconst
> > +hv_timesource_pci_table[] __maybe_unused = {
> > +	{ PCI_DEVICE(0x1414, 0x5353) }, /* VGA compatible controller */
> > +	{ 0 }
> > +};
> > +MODULE_DEVICE_TABLE(pci, hv_timesource_pci_table);
> 
> This pci_id table is unused. What's the purpose ?

Both the PCI and DMI tables were defined to ensure autoloading
> 
> > +
> > +static int __init init_hv_clocksource(void)
> > +{
> > +	if ((x86_hyper != &x86_hyper_ms_hyperv) ||
> > +		!(ms_hyperv.features &
> HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
> > +		return -ENODEV;
> 
> Isn't this a sufficient indicator ?

This ensures we are running on Hyper-V and for that this check is sufficient.

> 
> > +	if (!dmi_check_system(hv_timesource_dmi_table))
> > +		return -ENODEV;
> 
> Do we really need this additional check? I'd say if x86_hyper ==
> x86_hyper_ms_hyperv then failing the DMI check would be more than
> silly.

Both the PCI and DMI tables were introduced by Greg to support autoloading.
I will see if I can clean this up.

> 
> > +	pr_info("Registering HyperV clock source\n");
> > +	return clocksource_register(&hyperv_cs);
> 
> Please use clocksource_register_hz/khz and get rid of the hard coded
> constants.

Will do.
> 
> > +}
> > +
> > +module_init(init_hv_clocksource);
> > +MODULE_DESCRIPTION("HyperV based clocksource");
> > +MODULE_AUTHOR("K. Y. Srinivasan <ksrinivasan@...ell.com>");
> > +MODULE_LICENSE("GPL");
> 
> Why do we need to build this as a module?

No particular reason. This is the way, I had it in the staging directory.
Do you want me to build this as part of the kernel? I would then not have
to worry about auto-loading issues.

Regards,

K. Y 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/