linux-kernel - Re: [patch 1/4] x86: vSMP: Fix is_vsmp

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]

Date:	Fri, 21 Mar 2008 10:11:39 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Ravikiran G Thirumalai <kiran@...lex86.org>
Cc:	Yinghai Lu <yhlu.kernel@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org,
	Glauber de Oliveira Costa <gcosta@...hat.com>,
	Andi Kleen <ak@...e.de>, shai@...lex86.org
Subject: Re: [patch 1/4] x86: vSMP: Fix is_vsmp_box()


* Ravikiran G Thirumalai <kiran@...lex86.org> wrote:

> >>         /* Check if we are running on a ScaleMP vSMP box */
> >>  -       if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
> >>  -            (PCI_VENDOR_ID_SCALEMP || (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
> >>  +       if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
> >>  +            PCI_VENDOR_ID_SCALEMP) &&
> >>  +           (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
> >>  +           PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
> >>                 vsmp = 1;
> >>
> >>         return vsmp;
> >
> >why read two times
> >
> 
> Well, the pci cfg space read happens just _once_ during the boot, as 
> the result is cached in a static flag. The above code is better 
> readable. So readability is better than micro-optimization here.

i think the patch below results in even more readable and a bit smaller 
code - because it's such a simple check?

OTOH, you are right in general, for example in mmconf-fam10h_64.c's 
get_fam10h_pci_mmconf_base() function, we do this:

                id = read_pci_config(bus, slot, 0, PCI_VENDOR_ID);

                vendor = id & 0xffff;
                device = (id>>16) & 0xffff;
                if (pci_probes[i].vendor == vendor &&
                    pci_probes[i].device == device) {

here it would indeed be cleaner to simply do:

                vendor = read_pci_config_16(bus, slot, 0, PCI_VENDOR_ID);
                device = read_pci_config_16(bus, slot, 0, PCI_DEVICE_ID);

instead of open-coding the 16-bit splitting.

	Ingo

------------>
Subject: x86: vsmp fix x86 vsmp fix is vsmp box cleanup
From: Ingo Molnar <mingo@...e.hu>
Date: Fri Mar 21 09:55:06 CET 2008

code got a bit smaller:

arch/x86/kernel/vsmp_64.o:

   text	   data	    bss	    dec	    hex	filename
    205	      4	      0	    209	     d1	vsmp_64.o.before
    181	      4	      0	    185	     b9	vsmp_64.o.after

Signed-off-by: Ingo Molnar <mingo@...e.hu>
---
 arch/x86/kernel/vsmp_64.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: linux-x86.q/arch/x86/kernel/vsmp_64.c
===================================================================
--- linux-x86.q.orig/arch/x86/kernel/vsmp_64.c
+++ linux-x86.q/arch/x86/kernel/vsmp_64.c
@@ -120,10 +120,8 @@ int is_vsmp_box(void)
 		return vsmp;
 
 	/* Check if we are running on a ScaleMP vSMP box */
-	if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) ==
-	     PCI_VENDOR_ID_SCALEMP) &&
-	    (read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) ==
-	    PCI_DEVICE_ID_SCALEMP_VSMP_CTL))
+	if (read_pci_config(0, 0x1f, 0, PCI_VENDOR_ID) ==
+	     (PCI_VENDOR_ID_SCALEMP | (PCI_DEVICE_ID_SCALEMP_VSMP_CTL << 16)))
 		vsmp = 1;
 
 	return vsmp;
--
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/