Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers
On Thu, Apr 27, 2017 at 04:09:56PM +0300, Andy Shevchenko wrote: > Lukas pointed to this: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725 Yap, the same thing. Thanks. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers
On Thu, 2017-04-27 at 14:46 +0200, Borislav Petkov wrote: > On Fri, Apr 21, 2017 at 11:22:31PM +0200, Rafael J. Wysocki wrote: > > > #ifdef CONFIG_ACPI_APEI_PCIEAER > > > - else if (!uuid_le_cmp(*(uuid_le *)gdata- > > > >section_type, > > > - CPER_SEC_PCIE)) { > > > + else if (!uuid_le_cmp_p(sec_type, CPER_SEC_PCIE)) > > > { > > > struct cper_sec_pcie *pcie_err; > > > pcie_err = (struct cper_sec_pcie > > > *)(gdata+1); > > > if (sev == GHES_SEV_RECOVERABLE && > > > > > > > But this one is for Boris. > > I don't see anything wrong with it upon a brief inspection. Lukas pointed to this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725 > > What could be improved here, though, is if the whole uuid_* types > handling be changed so that gcc doesn't generate yucky code. Because > here's what it does now, regardless of this patch: > > .file 16 "./include/linux/uuid.h" > .loc 16 63 0 > leaq16(%rsp), %rsi #, > movl$16, %edx #, > movq%r15, %rdi # gdata, > movb$84, 16(%rsp) #, MEM[(struct *)&u2] > movb$-23, 17(%rsp) #, MEM[(struct *)&u2 + 1B] > movb$-107, 18(%rsp) #, MEM[(struct *)&u2 + 2B] > movb$-39, 19(%rsp) #, MEM[(struct *)&u2 + 3B] > movb$-63, 20(%rsp) #, MEM[(struct *)&u2 + 4B] > movb$-69, 21(%rsp) #, MEM[(struct *)&u2 + 5B] > movb$15, 22(%rsp) #, MEM[(struct *)&u2 + 6B] > movb$67, 23(%rsp) #, MEM[(struct *)&u2 + 7B] > movb$-83, 24(%rsp) #, MEM[(struct *)&u2 + 8B] > movb$-111, 25(%rsp) #, MEM[(struct *)&u2 + 9B] > movb$-76, 26(%rsp) #, MEM[(struct *)&u2 + 10B] > movb$77, 27(%rsp) #, MEM[(struct *)&u2 + 11B] > movb$-53, 28(%rsp) #, MEM[(struct *)&u2 + 12B] > movb$60, 29(%rsp) #, MEM[(struct *)&u2 + 13B] > movb$111, 30(%rsp) #, MEM[(struct *)&u2 + 14B] > movb$53, 31(%rsp) #, MEM[(struct *)&u2 + 15B] > callmemcmp # > > So it is basically building that UUID byte by byte before calling > memcmp. > > And I'm wondering if those 16-byte arrays could be replaced with > > typedef struct { > u64 a, b; > } u128; > > from the crypto code. > > And whether the code generated by gcc would look much saner. Because > the > CPU can handle two qwords much better/faster than 16 u8s. > > Anyway, in case someone feels bored... > > -- > Regards/Gruss, > Boris. > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, > HRB 21284 (AG Nürnberg) -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers
On Fri, Apr 21, 2017 at 11:22:31PM +0200, Rafael J. Wysocki wrote: > > #ifdef CONFIG_ACPI_APEI_PCIEAER > > - else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, > > - CPER_SEC_PCIE)) { > > + else if (!uuid_le_cmp_p(sec_type, CPER_SEC_PCIE)) { > > struct cper_sec_pcie *pcie_err; > > pcie_err = (struct cper_sec_pcie *)(gdata+1); > > if (sev == GHES_SEV_RECOVERABLE && > > > > But this one is for Boris. I don't see anything wrong with it upon a brief inspection. What could be improved here, though, is if the whole uuid_* types handling be changed so that gcc doesn't generate yucky code. Because here's what it does now, regardless of this patch: .file 16 "./include/linux/uuid.h" .loc 16 63 0 leaq16(%rsp), %rsi #, movl$16, %edx #, movq%r15, %rdi # gdata, movb$84, 16(%rsp) #, MEM[(struct *)&u2] movb$-23, 17(%rsp) #, MEM[(struct *)&u2 + 1B] movb$-107, 18(%rsp) #, MEM[(struct *)&u2 + 2B] movb$-39, 19(%rsp) #, MEM[(struct *)&u2 + 3B] movb$-63, 20(%rsp) #, MEM[(struct *)&u2 + 4B] movb$-69, 21(%rsp) #, MEM[(struct *)&u2 + 5B] movb$15, 22(%rsp) #, MEM[(struct *)&u2 + 6B] movb$67, 23(%rsp) #, MEM[(struct *)&u2 + 7B] movb$-83, 24(%rsp) #, MEM[(struct *)&u2 + 8B] movb$-111, 25(%rsp) #, MEM[(struct *)&u2 + 9B] movb$-76, 26(%rsp) #, MEM[(struct *)&u2 + 10B] movb$77, 27(%rsp) #, MEM[(struct *)&u2 + 11B] movb$-53, 28(%rsp) #, MEM[(struct *)&u2 + 12B] movb$60, 29(%rsp) #, MEM[(struct *)&u2 + 13B] movb$111, 30(%rsp) #, MEM[(struct *)&u2 + 14B] movb$53, 31(%rsp) #, MEM[(struct *)&u2 + 15B] callmemcmp # So it is basically building that UUID byte by byte before calling memcmp. And I'm wondering if those 16-byte arrays could be replaced with typedef struct { u64 a, b; } u128; from the crypto code. And whether the code generated by gcc would look much saner. Because the CPU can handle two qwords much better/faster than 16 u8s. Anyway, in case someone feels bored... -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers
On Friday, April 21, 2017 05:46:45 PM Andy Shevchenko wrote: > Recently introduced helpers take pointers to uuid_{be|le} instead of > reference. > > Using them makes code less ugly. > > Cc: "Rafael J. Wysocki" > Signed-off-by: Andy Shevchenko > --- > drivers/acpi/acpi_extlog.c | 2 +- > drivers/acpi/apei/ghes.c | 8 > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c > index 502ea4dc2080..45e299aefda7 100644 > --- a/drivers/acpi/acpi_extlog.c > +++ b/drivers/acpi/acpi_extlog.c > @@ -169,7 +169,7 @@ static int extlog_print(struct notifier_block *nb, > unsigned long val, > if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT) > fru_text = gdata->fru_text; > sec_type = (uuid_le *)gdata->section_type; > - if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) { > + if (!uuid_le_cmp_p(sec_type, CPER_SEC_PLATFORM_MEM)) { > struct cper_sec_mem_err *mem = (void *)(gdata + 1); > if (gdata->error_data_length >= sizeof(*mem)) > trace_extlog_mem_event(mem, err_seq, fru_id, fru_text, ACK for this one. > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index d0855c09f32f..f2a7ee26d45f 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -431,12 +431,13 @@ static void ghes_do_proc(struct ghes *ghes, > { > int sev, sec_sev; > struct acpi_hest_generic_data *gdata; > + uuid_le *sec_type; > > sev = ghes_severity(estatus->error_severity); > apei_estatus_for_each_section(estatus, gdata) { > + sec_type = (uuid_le *)gdata->section_type; > sec_sev = ghes_severity(gdata->error_severity); > - if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, > - CPER_SEC_PLATFORM_MEM)) { > + if (!uuid_le_cmp_p(sec_type, CPER_SEC_PLATFORM_MEM)) { > struct cper_sec_mem_err *mem_err; > mem_err = (struct cper_sec_mem_err *)(gdata+1); > ghes_edac_report_mem_error(ghes, sev, mem_err); > @@ -445,8 +446,7 @@ static void ghes_do_proc(struct ghes *ghes, > ghes_handle_memory_failure(gdata, sev); > } > #ifdef CONFIG_ACPI_APEI_PCIEAER > - else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type, > - CPER_SEC_PCIE)) { > + else if (!uuid_le_cmp_p(sec_type, CPER_SEC_PCIE)) { > struct cper_sec_pcie *pcie_err; > pcie_err = (struct cper_sec_pcie *)(gdata+1); > if (sev == GHES_SEV_RECOVERABLE && > But this one is for Boris. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html