Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1632082 > unrolled thread

Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers

Started byBorislav Petkov <bp@suse.de>
First post2017-04-27 14:50 +0200
Last post2017-04-27 17:10 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}()  helpers Borislav Petkov <bp@suse.de> - 2017-04-27 14:50 +0200
    Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}()  helpers Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-04-27 15:20 +0200
      Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}()  helpers Borislav Petkov <bp@suse.de> - 2017-04-27 17:10 +0200

#1632082 — Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers

FromBorislav Petkov <bp@suse.de>
Date2017-04-27 14:50 +0200
SubjectRe: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers
Message-ID<tAR1U-F6-13@gated-at.bofh.it>
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
        leaq    16(%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]
        call    memcmp  #

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)
--

[toc] | [next] | [standalone]

#1632092

FromAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Date2017-04-27 15:20 +0200
Message-ID<tARuV-16R-11@gated-at.bofh.it>
In reply to#1632082
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
>         leaq    16(%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]
>         call    memcmp  #
> 
> 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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1632160

FromBorislav Petkov <bp@suse.de>
Date2017-04-27 17:10 +0200
Message-ID<tATdn-2kj-9@gated-at.bofh.it>
In reply to#1632092
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)
--

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web