Re: [SeaBIOS] [PATCH v2] add acpi pmtimer support

2012-09-05 Thread Don Slutz 

On 09/05/12 01:27, Gerd Hoffmann wrote:

On 09/02/12 22:42, Kevin O'Connor wrote:

On Tue, Aug 14, 2012 at 07:29:19AM +0200, Gerd Hoffmann wrote:

This patch makes seabios use the acpi pmtimer instead of tsc for
timekeeping.  The pmtimer has a fixed frequency and doesn't need
calibration, thus it doesn't suffer from calibration errors due to a
loaded host machine.

The patch looks okay to me, but is it still needed?  (I recall seeing
something on the kvm list about a bug fix to the main timer.)

It is still a good idea to make timing in a virtual machine more robust.


+u32 pmtimer = inl(ioport);
+return (u64)wraps  24 | pmtimer;

BTW, why is this  24, and if it should be that way, shouldn't the
pmtimer be inl(ioport)  0xff ?

The pmtimer is defined to be 24 bits wide, so the shift is correct.
This is not true in general.  It can be either 24 or 32 bits.  What it 
is depends on ACPI data (acpi_gbl_FADT-tmr_val_ext).  However it is 
valid to only used 24 bits.


*/
/*


But, yes, the ioport read should better be masked to be on the safe
side.  v3 will go out in a minute.

cheers,
   Gerd

___
SeaBIOS mailing list
seab...@seabios.org
http://www.seabios.org/mailman/listinfo/seabios

  -Don Slutz
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [SeaBIOS] [PATCH v2] add acpi pmtimer support

2012-09-05 Thread Gerd Hoffmann 
  Hi,

 +u32 pmtimer = inl(ioport);
 +return (u64)wraps  24 | pmtimer;
 BTW, why is this  24, and if it should be that way, shouldn't the
 pmtimer be inl(ioport)  0xff ?
 The pmtimer is defined to be 24 bits wide, so the shift is correct.
 This is not true in general.  It can be either 24 or 32 bits.  What it
 is depends on ACPI data (acpi_gbl_FADT-tmr_val_ext).

The piix4 emulated by qemu has 24 bits.

  However it is
 valid to only used 24 bits.

And we certainly want to mask the ioport read (as suggested by kevin and
done in v3 of the patch) so we only pick up the 24 bits we actually use.

thanks
  Gerd

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] add acpi pmtimer support

2012-09-04 Thread Avi Kivity 
On 09/02/2012 11:42 PM, Kevin O'Connor wrote:
 On Tue, Aug 14, 2012 at 07:29:19AM +0200, Gerd Hoffmann wrote:
 This patch makes seabios use the acpi pmtimer instead of tsc for
 timekeeping.  The pmtimer has a fixed frequency and doesn't need
 calibration, thus it doesn't suffer from calibration errors due to a
 loaded host machine.
 
 The patch looks okay to me, but is it still needed?  (I recall seeing
 something on the kvm list about a bug fix to the main timer.)

Timing will always be fragile in a vm, so I think this can make things
more robust.


-- 
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] add acpi pmtimer support

2012-09-04 Thread Gerd Hoffmann 
On 09/02/12 22:42, Kevin O'Connor wrote:
 On Tue, Aug 14, 2012 at 07:29:19AM +0200, Gerd Hoffmann wrote:
 This patch makes seabios use the acpi pmtimer instead of tsc for
 timekeeping.  The pmtimer has a fixed frequency and doesn't need
 calibration, thus it doesn't suffer from calibration errors due to a
 loaded host machine.
 
 The patch looks okay to me, but is it still needed?  (I recall seeing
 something on the kvm list about a bug fix to the main timer.)

It is still a good idea to make timing in a virtual machine more robust.

 +u32 pmtimer = inl(ioport);

 +return (u64)wraps  24 | pmtimer;
 
 BTW, why is this  24, and if it should be that way, shouldn't the
 pmtimer be inl(ioport)  0xff ?

The pmtimer is defined to be 24 bits wide, so the shift is correct.
But, yes, the ioport read should better be masked to be on the safe
side.  v3 will go out in a minute.

cheers,
  Gerd
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] add acpi pmtimer support

2012-09-02 Thread Kevin O'Connor 
On Tue, Aug 14, 2012 at 07:29:19AM +0200, Gerd Hoffmann wrote:
 This patch makes seabios use the acpi pmtimer instead of tsc for
 timekeeping.  The pmtimer has a fixed frequency and doesn't need
 calibration, thus it doesn't suffer from calibration errors due to a
 loaded host machine.

The patch looks okay to me, but is it still needed?  (I recall seeing
something on the kvm list about a bug fix to the main timer.)

[...]
 +static u64 pmtimer_get(void)
 +{
 +u16 ioport = GET_GLOBAL(pmtimer_ioport);
 +u32 wraps = GET_LOW(pmtimer_wraps);
 +u32 pmtimer = inl(ioport);
 +
 +if (pmtimer  GET_LOW(pmtimer_last)) {
 +wraps++;
 +SET_LOW(pmtimer_wraps, wraps);
 +}
 +SET_LOW(pmtimer_last, pmtimer);
 +
 +dprintf(9, pmtimer: %u:%u\n, wraps, pmtimer);
 +return (u64)wraps  24 | pmtimer;

BTW, why is this  24, and if it should be that way, shouldn't the
pmtimer be inl(ioport)  0xff ?

-Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] add acpi pmtimer support

2012-08-13 Thread Gerd Hoffmann 
This patch makes seabios use the acpi pmtimer instead of tsc for
timekeeping.  The pmtimer has a fixed frequency and doesn't need
calibration, thus it doesn't suffer from calibration errors due to a
loaded host machine.

[ v2: add CONFIG_PMTIMER ]

Signed-off-by: Gerd Hoffmann kra...@redhat.com
---
 src/Kconfig   |6 ++
 src/clock.c   |   31 +++
 src/pciinit.c |5 +
 src/util.h|1 +
 4 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/src/Kconfig b/src/Kconfig
index 6de3e71..b5dd63b 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -222,6 +222,12 @@ menu Hardware support
 default y
 help
 Initialize the Memory Type Range Registers (on emulators).
+config PMTIMER
+depends on !COREBOOT
+bool Use ACPI timer
+default y
+help
+Use the ACPI timer instead of the TSC for timekeeping (on qemu).
 endmenu
 
 menu BIOS interfaces
diff --git a/src/clock.c b/src/clock.c
index 69e9f17..b4abf37 100644
--- a/src/clock.c
+++ b/src/clock.c
@@ -129,11 +129,42 @@ emulate_tsc(void)
 return ret;
 }
 
+u16 pmtimer_ioport VAR16VISIBLE;
+u32 pmtimer_wraps VARLOW;
+u32 pmtimer_last VARLOW;
+
+void pmtimer_init(u16 ioport, u32 khz)
+{
+if (!CONFIG_PMTIMER)
+return;
+dprintf(1, Using pmtimer, ioport 0x%x, freq %d kHz\n, ioport, khz);
+SET_GLOBAL(pmtimer_ioport, ioport);
+SET_GLOBAL(cpu_khz, khz);
+}
+
+static u64 pmtimer_get(void)
+{
+u16 ioport = GET_GLOBAL(pmtimer_ioport);
+u32 wraps = GET_LOW(pmtimer_wraps);
+u32 pmtimer = inl(ioport);
+
+if (pmtimer  GET_LOW(pmtimer_last)) {
+wraps++;
+SET_LOW(pmtimer_wraps, wraps);
+}
+SET_LOW(pmtimer_last, pmtimer);
+
+dprintf(9, pmtimer: %u:%u\n, wraps, pmtimer);
+return (u64)wraps  24 | pmtimer;
+}
+
 static u64
 get_tsc(void)
 {
 if (unlikely(GET_GLOBAL(no_tsc)))
 return emulate_tsc();
+if (CONFIG_PMTIMER  GET_GLOBAL(pmtimer_ioport))
+return pmtimer_get();
 return rdtscll();
 }
 
diff --git a/src/pciinit.c b/src/pciinit.c
index 68f302a..31115ee 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -180,6 +180,9 @@ static const struct pci_device_id pci_class_tbl[] = {
 PCI_DEVICE_END,
 };
 
+/* PM Timer ticks per second (HZ) */
+#define PM_TIMER_FREQUENCY  3579545
+
 /* PIIX4 Power Management device (for ACPI) */
 static void piix4_pm_init(struct pci_device *pci, void *arg)
 {
@@ -191,6 +194,8 @@ static void piix4_pm_init(struct pci_device *pci, void *arg)
 pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */
 pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1);
 pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */
+
+pmtimer_init(PORT_ACPI_PM_BASE + 0x08, PM_TIMER_FREQUENCY / 1000);
 }
 
 static const struct pci_device_id pci_device_tbl[] = {
diff --git a/src/util.h b/src/util.h
index 89e928c..1603a57 100644
--- a/src/util.h
+++ b/src/util.h
@@ -312,6 +312,7 @@ void lpt_setup(void);
 // clock.c
 #define PIT_TICK_RATE 1193180   // Underlying HZ of PIT
 #define PIT_TICK_INTERVAL 65536 // Default interval for 18.2Hz timer
+void pmtimer_init(u16 ioport, u32 khz);
 int check_tsc(u64 end);
 void timer_setup(void);
 void ndelay(u32 count);
-- 
1.7.1

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html