qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [SeaBIOS PATCH 2/2] allow CPUs to have non-contiguous L


From: Gleb Natapov
Subject: Re: [Qemu-devel] [SeaBIOS PATCH 2/2] allow CPUs to have non-contiguous Local APIC IDs (v2)
Date: Thu, 26 Jul 2012 09:08:40 +0300

On Wed, Jul 25, 2012 at 03:42:21PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 23, 2012 at 03:20:14PM +0300, Gleb Natapov wrote:
> > On Fri, Jul 20, 2012 at 02:04:50PM -0300, Eduardo Habkost wrote:
> > > Extract Local APIC IDs directly from the CPUs, and instead of check for
> > > "i < CountCPUs", check if the APIC ID was present on boot, when building
> > > ACPI tables and the MP-Table.
> > > 
> > > This keeps ACPI Processor ID == APIC ID, but allows the
> > > hardware<->SeaBIOS interface be completely APIC-ID based and not depend
> > > on any other kind of "CPU identifier". This way, SeaBIOS may change the
> > > way ACPI Processor IDs are chosen in the future.
> > > 
> > > As currently SeaBIOS supports only xAPIC and not x2APIC, the list of
> > > present-on-boot APIC IDs is a 256-bit bitmap. If one day SeaBIOS starts
> > > to support x2APIC, the data structure used to enumerate the APIC IDs
> > > will have to be changed (but this is an internal implementation detail,
> > > not visible to the OS or on any hardware<=>SeaBIOS interface).
> > > 
> > > For current QEMU versions (that always make the APIC IDs contiguous),
> > > the OS-visible behavior and resulting ACPI tables should be exactly the
> > > same. This patch will simply allow QEMU to start setting non-contiguous
> > > APIC IDs (that is a requirement for some sockets/cores/threads topology
> > > settings).
> > > 
> > > Changes v1 -> v2:
> > >  - Use size suffixes on all asm instructions on smp.c
> > >  - New patch description
> > > 
> > > Signed-off-by: Eduardo Habkost <address@hidden>
> > > ---
> > >  src/acpi-dsdt.dsl |    4 +++-
> > >  src/acpi.c        |    9 +++++----
> > >  src/mptable.c     |    2 +-
> > >  src/smp.c         |   17 +++++++++++++++++
> > >  src/util.h        |    1 +
> > >  5 files changed, 27 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> > > index 2060686..72dc7d8 100644
> > > --- a/src/acpi-dsdt.dsl
> > > +++ b/src/acpi-dsdt.dsl
> > > @@ -676,6 +676,7 @@ DefinitionBlock (
> > >          /* Methods called by run-time generated SSDT Processor objects */
> > >          Method (CPMA, 1, NotSerialized) {
> > >              // _MAT method - create an madt apic buffer
> > > +            // Arg0 = Processor ID = Local APIC ID
> > >              // Local0 = CPON flag for this cpu
> > >              Store(DerefOf(Index(CPON, Arg0)), Local0)
> > >              // Local1 = Buffer (in madt apic form) to return
> > > @@ -688,6 +689,7 @@ DefinitionBlock (
> > >          }
> > >          Method (CPST, 1, NotSerialized) {
> > >              // _STA method - return ON status of cpu
> > > +            // Arg0 = Processor ID = Local APIC ID
> > >              // Local0 = CPON flag for this cpu
> > >              Store(DerefOf(Index(CPON, Arg0)), Local0)
> > >              If (Local0) { Return(0xF) } Else { Return(0x0) }
> > > @@ -708,7 +710,7 @@ DefinitionBlock (
> > >              Store (PRS, Local5)
> > >              // Local2 = last read byte from bitmap
> > >              Store (Zero, Local2)
> > > -            // Local0 = cpuid iterator
> > > +            // Local0 = Processor ID / APIC ID iterator
> > >              Store (Zero, Local0)
> > >              While (LLess(Local0, SizeOf(CPON))) {
> > >                  // Local1 = CPON flag for this cpu
> > > diff --git a/src/acpi.c b/src/acpi.c
> > > index da3bc57..39b7172 100644
> > > --- a/src/acpi.c
> > > +++ b/src/acpi.c
> > > @@ -327,7 +327,7 @@ build_madt(void)
> > >          apic->length = sizeof(*apic);
> > >          apic->processor_id = i;
> > >          apic->local_apic_id = i;
> > > -        if (i < CountCPUs)
> > > +        if (apic_id_is_present(apic->local_apic_id))
> > >              apic->flags = cpu_to_le32(1);
> > >          else
> > >              apic->flags = cpu_to_le32(0);
> > > @@ -445,6 +445,7 @@ build_ssdt(void)
> > >      }
> > >  
> > >      // build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, 
> > > Arg1)} ...}"
> > > +    // Arg0 = Processor ID = APIC ID
> > >      *(ssdt_ptr++) = 0x14; // MethodOp
> > >      ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*acpi_cpus), 2);
> > >      *(ssdt_ptr++) = 'N';
> > > @@ -477,7 +478,7 @@ build_ssdt(void)
> > >      ssdt_ptr = encodeLen(ssdt_ptr, 2+1+(1*acpi_cpus), 2);
> > >      *(ssdt_ptr++) = acpi_cpus;
> > >      for (i=0; i<acpi_cpus; i++)
> > > -        *(ssdt_ptr++) = (i < CountCPUs) ? 0x01 : 0x00;
> > > +        *(ssdt_ptr++) = (apic_id_is_present(i)) ? 0x01 : 0x00;
> > >  
> > >      // store pci io windows: start, end, length
> > >      // this way we don't have to do the math in the dsdt
> > > @@ -656,10 +657,10 @@ build_srat(void)
> > >          core->proximity_lo = curnode;
> > >          memset(core->proximity_hi, 0, 3);
> > >          core->local_sapic_eid = 0;
> > > -        if (i < CountCPUs)
> > > +        if (apic_id_is_present(i))
> > >              core->flags = cpu_to_le32(1);
> > >          else
> > > -            core->flags = 0;
> > > +            core->flags = cpu_to_le32(0);
> > >          core++;
> > >      }
> > >  
> > > diff --git a/src/mptable.c b/src/mptable.c
> > > index 103f462..9406f98 100644
> > > --- a/src/mptable.c
> > > +++ b/src/mptable.c
> > > @@ -59,7 +59,7 @@ mptable_init(void)
> > >          cpu->apicid = i;
> > >          cpu->apicver = apic_version;
> > >          /* cpu flags: enabled, bootstrap cpu */
> > > -        cpu->cpuflag = ((i<CountCPUs) ? 0x01 : 0x00) | ((i==0) ? 0x02 : 
> > > 0x00);
> > > +        cpu->cpuflag = (apic_id_is_present(i) ? 0x01 : 0x00) | ((i==0) ? 
> > > 0x02 : 0x00);
> > >          cpu->cpusignature = cpuid_signature;
> > >          cpu->featureflag = cpuid_features;
> > >          cpu++;
> > > diff --git a/src/smp.c b/src/smp.c
> > > index 8c077a1..3c36f8c 100644
> > > --- a/src/smp.c
> > > +++ b/src/smp.c
> > > @@ -36,6 +36,8 @@ wrmsr_smp(u32 index, u64 val)
> > >  
> > >  u32 CountCPUs VAR16VISIBLE;
> > >  u32 MaxCountCPUs VAR16VISIBLE;
> > > +// 256 bits for the found APIC IDs
> > > +u32 FoundAPICIDs[256/32] VAR16VISIBLE;
> > >  extern void smp_ap_boot_code(void);
> > >  ASM16(
> > >      "  .global smp_ap_boot_code\n"
> > > @@ -59,6 +61,12 @@ ASM16(
> > >      "  jmp 1b\n"
> > >      "2:\n"
> > >  
> > > +    // get apic ID on EBX, set bit on FoundAPICIDs
> > > +    "  movl $1, %eax\n"
> > > +    "  cpuid\n"
> > > +    "  shrl $24, %ebx\n"
> > > +    "  lock btsl %ebx, FoundAPICIDs\n"
> > > +
> > >      // Increment the cpu counter
> > >      "  lock incl CountCPUs\n"
> > You can get rid of CountCPUs by calculating FoundAPICIDs bitmap weight.
> 
> I was going to do that after you suggested, but then I saw this:
> 
>         while (cmos_smp_count + 1 != readl(&CountCPUs))
>             yield();
> 
> It's possible to replace the atomic read of CountCPUs with the bitmap weight
> calculation on the loop, but: is it really worth it?
> 
Why not? This eliminates one more global state.

--
                        Gleb.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]