| Message ID | 1526896771-9787-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | acpi: hpet: fix getting invalid vendor ID and clock period | expand |
On 21/05/18 10:59, Ivan Hu wrote: > Some platforms get the report again, > Invalid Vendor ID: ffff - this should be configured. > Invalid clock period 4294967295, must be non-zero and less than 10^8. > after the commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 applied. > > some platforms can be fixed by commit# > ee769ccf294cff2c63769482e84866a979d42f44, but still got fail on > some Intel platforms with latest Bios. > > The root cause is commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 > adds safe memory read checking, so do two memory reads immediately, HW might > not able to get ready for registers to be read and get the false values. > > Adding delay between two reads could solve this issue, and also change to 64 > bits safe memory read check. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/acpi/hpet/hpet.c | 4 +++- > src/lib/include/fwts_safe_mem.h | 1 + > src/lib/src/fwts_safe_mem.c | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/src/acpi/hpet/hpet.c b/src/acpi/hpet/hpet.c > index fca11e6..92a0c58 100644 > --- a/src/acpi/hpet/hpet.c > +++ b/src/acpi/hpet/hpet.c > @@ -428,12 +428,14 @@ static int hpet_check_test4(fwts_framework *fw) > return FWTS_ERROR; > } > > - if (fwts_safe_memread32(hpet_base_v, HPET_REG_SIZE / 4) != FWTS_OK) { > + if (fwts_safe_memread64(hpet_base_v, HPET_REG_SIZE / 8) != FWTS_OK) { > fwts_log_info(fw, "Test skipped because HPET region cannot be read."); > (void)fwts_munmap(hpet_base_v, HPET_REG_SIZE); > return FWTS_SKIP; > } > > + usleep(10); > + > hpet_id = *(uint64_t*) hpet_base_v; > vendor_id = (hpet_id & 0xffff0000) >> 16; > > diff --git a/src/lib/include/fwts_safe_mem.h b/src/lib/include/fwts_safe_mem.h > index 964cb8d..3ba9094 100644 > --- a/src/lib/include/fwts_safe_mem.h > +++ b/src/lib/include/fwts_safe_mem.h > @@ -23,5 +23,6 @@ > int fwts_safe_memcpy(void *dst, const void *src, const size_t n); > int fwts_safe_memread(const void *src, const size_t n); > int fwts_safe_memread32(const void *src, const size_t n); > +int fwts_safe_memread64(const void *src, const size_t n); > > #endif > diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c > index 199b27f..10d4f7c 100644 > --- a/src/lib/src/fwts_safe_mem.c > +++ b/src/lib/src/fwts_safe_mem.c > @@ -127,3 +127,38 @@ int OPTIMIZE0 fwts_safe_memread32(const void *src, const size_t n) > > return FWTS_OK; > } > + > +/* > + * fwts_safe_memread64() > + * check we can safely read a region of memory. This catches > + * SIGSEGV/SIGBUS errors and returns FWTS_ERROR if it is not > + * readable or FWTS_OK if it's OK. > + * > + * n = number of of 64 bit words to check > + */ > +int OPTIMIZE0 fwts_safe_memread64(const void *src, const size_t n) > +{ > + static uint64_t buffer[256]; > + const uint64_t *ptr, *end = (uint64_t *)src + n; > + uint64_t *bufptr; > + const uint64_t *bufend = buffer + (sizeof(buffer) / sizeof(*buffer)); > + > + if (sigsetjmp(jmpbuf, 1) != 0) > + return FWTS_ERROR; > + > + fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action); > + fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action); > + > + for (bufptr = buffer, ptr = src; ptr < end; ptr++) { > + /* Force data to be read */ > + __builtin_prefetch(ptr, 0, 3); > + *bufptr = *ptr; > + bufptr++; > + bufptr = (bufptr >= bufend) ? buffer : bufptr; > + } > + > + fwts_sig_handler_restore(SIGSEGV, &old_segv_action); > + fwts_sig_handler_restore(SIGBUS, &old_bus_action); > + > + return FWTS_OK; > +} > Acked-by: Colin Ian King <colin.king@canonical.com>
On 2018-05-21 02:59 AM, Ivan Hu wrote: > Some platforms get the report again, > Invalid Vendor ID: ffff - this should be configured. > Invalid clock period 4294967295, must be non-zero and less than 10^8. > after the commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 applied. > > some platforms can be fixed by commit# > ee769ccf294cff2c63769482e84866a979d42f44, but still got fail on > some Intel platforms with latest Bios. > > The root cause is commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 > adds safe memory read checking, so do two memory reads immediately, HW might > not able to get ready for registers to be read and get the false values. > > Adding delay between two reads could solve this issue, and also change to 64 > bits safe memory read check. > > Signed-off-by: Ivan Hu <ivan.hu@canonical.com> > --- > src/acpi/hpet/hpet.c | 4 +++- > src/lib/include/fwts_safe_mem.h | 1 + > src/lib/src/fwts_safe_mem.c | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/src/acpi/hpet/hpet.c b/src/acpi/hpet/hpet.c > index fca11e6..92a0c58 100644 > --- a/src/acpi/hpet/hpet.c > +++ b/src/acpi/hpet/hpet.c > @@ -428,12 +428,14 @@ static int hpet_check_test4(fwts_framework *fw) > return FWTS_ERROR; > } > > - if (fwts_safe_memread32(hpet_base_v, HPET_REG_SIZE / 4) != FWTS_OK) { > + if (fwts_safe_memread64(hpet_base_v, HPET_REG_SIZE / 8) != FWTS_OK) { > fwts_log_info(fw, "Test skipped because HPET region cannot be read."); > (void)fwts_munmap(hpet_base_v, HPET_REG_SIZE); > return FWTS_SKIP; > } > > + usleep(10); > + > hpet_id = *(uint64_t*) hpet_base_v; > vendor_id = (hpet_id & 0xffff0000) >> 16; > > diff --git a/src/lib/include/fwts_safe_mem.h b/src/lib/include/fwts_safe_mem.h > index 964cb8d..3ba9094 100644 > --- a/src/lib/include/fwts_safe_mem.h > +++ b/src/lib/include/fwts_safe_mem.h > @@ -23,5 +23,6 @@ > int fwts_safe_memcpy(void *dst, const void *src, const size_t n); > int fwts_safe_memread(const void *src, const size_t n); > int fwts_safe_memread32(const void *src, const size_t n); > +int fwts_safe_memread64(const void *src, const size_t n); > > #endif > diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c > index 199b27f..10d4f7c 100644 > --- a/src/lib/src/fwts_safe_mem.c > +++ b/src/lib/src/fwts_safe_mem.c > @@ -127,3 +127,38 @@ int OPTIMIZE0 fwts_safe_memread32(const void *src, const size_t n) > > return FWTS_OK; > } > + > +/* > + * fwts_safe_memread64() > + * check we can safely read a region of memory. This catches > + * SIGSEGV/SIGBUS errors and returns FWTS_ERROR if it is not > + * readable or FWTS_OK if it's OK. > + * > + * n = number of of 64 bit words to check > + */ > +int OPTIMIZE0 fwts_safe_memread64(const void *src, const size_t n) > +{ > + static uint64_t buffer[256]; > + const uint64_t *ptr, *end = (uint64_t *)src + n; > + uint64_t *bufptr; > + const uint64_t *bufend = buffer + (sizeof(buffer) / sizeof(*buffer)); > + > + if (sigsetjmp(jmpbuf, 1) != 0) > + return FWTS_ERROR; > + > + fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action); > + fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action); > + > + for (bufptr = buffer, ptr = src; ptr < end; ptr++) { > + /* Force data to be read */ > + __builtin_prefetch(ptr, 0, 3); > + *bufptr = *ptr; > + bufptr++; > + bufptr = (bufptr >= bufend) ? buffer : bufptr; > + } > + > + fwts_sig_handler_restore(SIGSEGV, &old_segv_action); > + fwts_sig_handler_restore(SIGBUS, &old_bus_action); > + > + return FWTS_OK; > +} > Acked-by: Alex Hung <alex.hung@canonical.com>
diff --git a/src/acpi/hpet/hpet.c b/src/acpi/hpet/hpet.c index fca11e6..92a0c58 100644 --- a/src/acpi/hpet/hpet.c +++ b/src/acpi/hpet/hpet.c @@ -428,12 +428,14 @@ static int hpet_check_test4(fwts_framework *fw) return FWTS_ERROR; } - if (fwts_safe_memread32(hpet_base_v, HPET_REG_SIZE / 4) != FWTS_OK) { + if (fwts_safe_memread64(hpet_base_v, HPET_REG_SIZE / 8) != FWTS_OK) { fwts_log_info(fw, "Test skipped because HPET region cannot be read."); (void)fwts_munmap(hpet_base_v, HPET_REG_SIZE); return FWTS_SKIP; } + usleep(10); + hpet_id = *(uint64_t*) hpet_base_v; vendor_id = (hpet_id & 0xffff0000) >> 16; diff --git a/src/lib/include/fwts_safe_mem.h b/src/lib/include/fwts_safe_mem.h index 964cb8d..3ba9094 100644 --- a/src/lib/include/fwts_safe_mem.h +++ b/src/lib/include/fwts_safe_mem.h @@ -23,5 +23,6 @@ int fwts_safe_memcpy(void *dst, const void *src, const size_t n); int fwts_safe_memread(const void *src, const size_t n); int fwts_safe_memread32(const void *src, const size_t n); +int fwts_safe_memread64(const void *src, const size_t n); #endif diff --git a/src/lib/src/fwts_safe_mem.c b/src/lib/src/fwts_safe_mem.c index 199b27f..10d4f7c 100644 --- a/src/lib/src/fwts_safe_mem.c +++ b/src/lib/src/fwts_safe_mem.c @@ -127,3 +127,38 @@ int OPTIMIZE0 fwts_safe_memread32(const void *src, const size_t n) return FWTS_OK; } + +/* + * fwts_safe_memread64() + * check we can safely read a region of memory. This catches + * SIGSEGV/SIGBUS errors and returns FWTS_ERROR if it is not + * readable or FWTS_OK if it's OK. + * + * n = number of of 64 bit words to check + */ +int OPTIMIZE0 fwts_safe_memread64(const void *src, const size_t n) +{ + static uint64_t buffer[256]; + const uint64_t *ptr, *end = (uint64_t *)src + n; + uint64_t *bufptr; + const uint64_t *bufend = buffer + (sizeof(buffer) / sizeof(*buffer)); + + if (sigsetjmp(jmpbuf, 1) != 0) + return FWTS_ERROR; + + fwts_sig_handler_set(SIGSEGV, sig_handler, &old_segv_action); + fwts_sig_handler_set(SIGBUS, sig_handler, &old_bus_action); + + for (bufptr = buffer, ptr = src; ptr < end; ptr++) { + /* Force data to be read */ + __builtin_prefetch(ptr, 0, 3); + *bufptr = *ptr; + bufptr++; + bufptr = (bufptr >= bufend) ? buffer : bufptr; + } + + fwts_sig_handler_restore(SIGSEGV, &old_segv_action); + fwts_sig_handler_restore(SIGBUS, &old_bus_action); + + return FWTS_OK; +}
Some platforms get the report again, Invalid Vendor ID: ffff - this should be configured. Invalid clock period 4294967295, must be non-zero and less than 10^8. after the commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 applied. some platforms can be fixed by commit# ee769ccf294cff2c63769482e84866a979d42f44, but still got fail on some Intel platforms with latest Bios. The root cause is commit#7cfe1a41956294bd8062b7680e1778ed9ee147b2 adds safe memory read checking, so do two memory reads immediately, HW might not able to get ready for registers to be read and get the false values. Adding delay between two reads could solve this issue, and also change to 64 bits safe memory read check. Signed-off-by: Ivan Hu <ivan.hu@canonical.com> --- src/acpi/hpet/hpet.c | 4 +++- src/lib/include/fwts_safe_mem.h | 1 + src/lib/src/fwts_safe_mem.c | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-)