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

Re: [PATCH] Differentiate between Active processors and Maximum processors

To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
From: "Durrant, Paul" <xadimgnik@xxxxxxxxx>
Date: Sat, 26 Feb 2022 17:49:35 +0000
Delivery-date: Sat, 26 Feb 2022 17:49:44 +0000
List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>

On 25/02/2022 09:17, Owen Smith wrote:

Limit iteration to the Active processor count when attempting to access the
processor arrays, whilst allocating enough space to cover the Maximum number
of processors.
Fixes a 0x0A Bugcheck on server OSs which support CPU hotplug.

Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>


This seems like overkill.

In system.c the processor array already is sized to accommodate themaximum number of processors so no issue there. I think what we arelacking is a flag or state in the SYSTEM_PROCESSOR structure to saywhether that particular processor has actually been added i.e. we'veseen the KeProcessorAddCompleteNotify callback. Then loops such as thatin SystemCheckProcessors() can test to see if a CPU has been seen beforetrying to wait on its (potentially uninitialized) event... which isprobably at least one source of the BSOD.

In the evtchn/shared_info code, because you have changed'ProcessorCount' to 'MaximumProcessors' it is also hard to spot thefunctional changes from the cosmetic ones. I also think it would beneater to have SystemProcessorVcpuId() fail with a distinctive error ifthe processor is not online and tolerate that failure by continuing.


  Paul

---
  src/xen/system.c         | 36 +++++++++++++++++++++-------------
  src/xenbus/evtchn.c      | 42 ++++++++++++++++++++++++----------------
  src/xenbus/evtchn_fifo.c |  2 +-
  src/xenbus/shared_info.c | 26 +++++++++++++------------
  4 files changed, 63 insertions(+), 43 deletions(-)

diff --git a/src/xen/system.c b/src/xen/system.c
index f85efc4..073b9b9 100644
--- a/src/xen/system.c
+++ b/src/xen/system.c
@@ -69,7 +69,7 @@ typedef struct _SYSTEM_CONTEXT {
      LONG                References;
      PACPI_MADT          Madt;
      PSYSTEM_PROCESSOR   Processor;
-    ULONG               ProcessorCount;
+    ULONG               MaximumProcessors;
      PVOID               PowerStateHandle;
      PVOID               ProcessorChangeHandle;
      PHYSICAL_ADDRESS    MaximumPhysicalAddress;
@@ -637,10 +637,13 @@ SystemProcessorVcpuId(
  {
      PSYSTEM_CONTEXT     Context = &SystemContext;
      PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];
+    ULONG               ProcessorCount;
      NTSTATUS            status;

+ ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);

+
      status = STATUS_UNSUCCESSFUL;
-    if (Cpu >= Context->ProcessorCount)
+    if (Cpu >= ProcessorCount)
          goto fail1;

*vcpu_id = Processor->ProcessorID;

@@ -659,10 +662,13 @@ SystemProcessorVcpuInfo(
  {
      PSYSTEM_CONTEXT     Context = &SystemContext;
      PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];
+    ULONG               ProcessorCount;
      NTSTATUS            status;

+ ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);

+
      status = STATUS_UNSUCCESSFUL;
-    if (Cpu >= Context->ProcessorCount)
+    if (Cpu >= ProcessorCount)
          goto fail1;

status = STATUS_NOT_SUPPORTED;

@@ -699,7 +705,7 @@ SystemProcessorRegisterVcpuInfo(
          goto done;

status = STATUS_UNSUCCESSFUL;

-    if (Cpu >= Context->ProcessorCount)
+    if (Cpu >= Context->MaximumProcessors)
          goto fail1;

ASSERT(Mdl->MdlFlags & MDL_MAPPED_TO_SYSTEM_VA);

@@ -799,7 +805,7 @@ SystemProcessorDpc(
      UNREFERENCED_PARAMETER(Argument2);

Cpu = KeGetCurrentProcessorNumberEx(&ProcNumber);

-    ASSERT3U(Cpu, <, Context->ProcessorCount);
+    ASSERT3U(Cpu, <, Context->MaximumProcessors);

Processor = &Context->Processor[Cpu];

      Processor->Status = STATUS_UNSUCCESSFUL;
@@ -859,7 +865,7 @@ SystemProcessorChangeCallback(
      case KeProcessorAddCompleteNotify: {
          PSYSTEM_PROCESSOR   Processor;

- ASSERT3U(Cpu, <, Context->ProcessorCount);

+        ASSERT3U(Cpu, <, Context->MaximumProcessors);

Processor = &Context->Processor[Cpu];@@ -983,12 +989,14 @@ SystemDeregisterProcessorChangeCallback(

      )
  {
      PSYSTEM_CONTEXT Context = &SystemContext;
+    ULONG           ProcessorCount;
      ULONG           Cpu;

KeDeregisterProcessorChangeCallback(Context->ProcessorChangeHandle);

      Context->ProcessorChangeHandle = NULL;

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];

SystemProcessorDeregisterVcpuInfo(Cpu);

@@ -1208,10 +1216,12 @@ SystemCheckProcessors(
      )
  {
      PSYSTEM_CONTEXT Context = &SystemContext;
+    ULONG           ProcessorCount;
      ULONG           Cpu;
      NTSTATUS        status;

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++)

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++)
      {
          PSYSTEM_PROCESSOR   Processor = &Context->Processor[Cpu];

@@ -1251,8 +1261,8 @@ SystemInitialize(

      if (References != 1)
          goto fail1;

- Context->ProcessorCount = KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);

-    Context->Processor = __SystemAllocate(sizeof (SYSTEM_PROCESSOR) * 
Context->ProcessorCount);
+    Context->MaximumProcessors = 
KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    Context->Processor = __SystemAllocate(sizeof (SYSTEM_PROCESSOR) * 
Context->MaximumProcessors);

status = STATUS_NO_MEMORY;

      if (Context->Processor == NULL)
@@ -1266,7 +1276,7 @@ SystemInitialize(
      if (NT_SUCCESS(status))
          Context->RegisterVcpuInfo = (RegisterVcpuInfo != 0) ? TRUE : FALSE;
      else
-        Context->RegisterVcpuInfo = (Context->ProcessorCount > 
XEN_LEGACY_MAX_VCPUS) ?
+        Context->RegisterVcpuInfo = (Context->MaximumProcessors > 
XEN_LEGACY_MAX_VCPUS) ?
                                      TRUE : FALSE;

status = SystemGetStartOptions();

@@ -1342,7 +1352,7 @@ fail3:
  fail2:
      Error("fail2\n");

- Context->ProcessorCount = 0;

+    Context->MaximumProcessors = 0;

fail1:

      Error("fail1 (%08x)\n", status);
@@ -1449,7 +1459,7 @@ SystemTeardown(
      __SystemFree(Context->Processor);
      Context->Processor = NULL;

- Context->ProcessorCount = 0;

+    Context->MaximumProcessors = 0;
      Context->RegisterVcpuInfo = FALSE;

(VOID) InterlockedDecrement(&Context->References);

diff --git a/src/xenbus/evtchn.c b/src/xenbus/evtchn.c
index 8942cdf..ad36448 100644
--- a/src/xenbus/evtchn.c
+++ b/src/xenbus/evtchn.c
@@ -106,7 +106,7 @@ struct _XENBUS_EVTCHN_CONTEXT {
      LONG                            References;
      PXENBUS_INTERRUPT               Interrupt;
      PXENBUS_EVTCHN_PROCESSOR        Processor;
-    ULONG                           ProcessorCount;
+    ULONG                           MaximumProcessors;
      XENBUS_SUSPEND_INTERFACE        SuspendInterface;
      PXENBUS_SUSPEND_CALLBACK        SuspendCallbackEarly;
      PXENBUS_SUSPEND_CALLBACK        SuspendCallbackLate;
@@ -276,7 +276,7 @@ EvtchnOpenVirq(

Cpu = KeGetProcessorIndexFromNumber(&ProcNumber);- ASSERT3U(Cpu, <, Context->ProcessorCount);

+    ASSERT3U(Cpu, <, Context->MaximumProcessors);
      Processor = &Context->Processor[Cpu];

status = STATUS_NOT_SUPPORTED;

@@ -531,7 +531,7 @@ EvtchnPoll(
      BOOLEAN                     DoneSomething;
      PLIST_ENTRY                 ListEntry;

- ASSERT3U(Cpu, <, Context->ProcessorCount);

+    ASSERT3U(Cpu, <, Context->MaximumProcessors);
      Processor = &Context->Processor[Cpu];

(VOID) XENBUS_EVTCHN_ABI(Poll,

@@ -613,7 +613,7 @@ EvtchnFlush(
      PXENBUS_INTERRUPT           Interrupt;
      KIRQL                       Irql;

- ASSERT3U(Cpu, <, Context->ProcessorCount);

+    ASSERT3U(Cpu, <, Context->MaximumProcessors);
      Processor = &Context->Processor[Cpu];

Interrupt = (Processor->UpcallEnabled) ?

@@ -702,7 +702,7 @@ EvtchnTrigger(
      Cpu = Channel->Cpu;
      KeReleaseSpinLock(&Channel->Lock, Irql);

- ASSERT3U(Cpu, <, Context->ProcessorCount);

+    ASSERT3U(Cpu, <, Context->MaximumProcessors);
      Processor = &Context->Processor[Cpu];

Interrupt = (Processor->UpcallEnabled) ?

@@ -751,7 +751,7 @@ EvtchnBind(

Cpu = KeGetProcessorIndexFromNumber(&ProcNumber);- ASSERT3U(Cpu, <, Context->ProcessorCount);

+    ASSERT3U(Cpu, <, Context->MaximumProcessors);
      Processor = &Context->Processor[Cpu];

status = STATUS_NOT_SUPPORTED;

@@ -1269,6 +1269,7 @@ EvtchnInterruptEnable(
  {
      ULONG                       Cpu;
      ULONG                       Line;
+    ULONG                       ProcessorCount;
      KIRQL                       Irql;
      NTSTATUS                    status;

@@ -1280,7 +1281,8 @@ EvtchnInterruptEnable(

          goto line;
      }

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PXENBUS_EVTCHN_PROCESSOR    Processor;
          unsigned int                vcpu_id;
          UCHAR                       Vector;
@@ -1340,6 +1342,7 @@ EvtchnInterruptDisable(
      )
  {
      ULONG                       Cpu;
+    ULONG                       ProcessorCount;
      NTSTATUS                    status;

UNREFERENCED_PARAMETER(Context);

@@ -1349,7 +1352,8 @@ EvtchnInterruptDisable(
      status = HvmSetParam(HVM_PARAM_CALLBACK_IRQ, 0);
      ASSERT(NT_SUCCESS(status));

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PXENBUS_EVTCHN_PROCESSOR    Processor;
          unsigned int                vcpu_id;

@@ -1531,6 +1535,7 @@ EvtchnAcquire(

      PXENBUS_FDO             Fdo = Context->Fdo;
      KIRQL                   Irql;
      PROCESSOR_NUMBER        ProcNumber;
+    ULONG                   ProcessorCount;
      ULONG                   Cpu;
      NTSTATUS                status;

@@ -1584,14 +1589,15 @@ EvtchnAcquire(

      if (!NT_SUCCESS(status))
          goto fail7;

- Context->ProcessorCount = KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);

-    Context->Processor = __EvtchnAllocate(sizeof (XENBUS_EVTCHN_PROCESSOR) * 
Context->ProcessorCount);
+    Context->MaximumProcessors = 
KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    Context->Processor = __EvtchnAllocate(sizeof (XENBUS_EVTCHN_PROCESSOR) * 
Context->MaximumProcessors);

status = STATUS_NO_MEMORY;

      if (Context->Processor == NULL)
          goto fail8;

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PXENBUS_EVTCHN_PROCESSOR    Processor;

if (!EvtchnIsProcessorEnabled(Context, Cpu))

@@ -1643,7 +1649,7 @@ done:
  fail9:
      Error("fail9\n");

- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PXENBUS_EVTCHN_PROCESSOR Processor;

ASSERT(Context->Processor != NULL);

@@ -1661,14 +1667,14 @@ fail9:
          Processor->Context = NULL;
      }

- ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_EVTCHN_PROCESSOR) * Context->ProcessorCount));

+    ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_EVTCHN_PROCESSOR) * 
Context->MaximumProcessors));
      __EvtchnFree(Context->Processor);
      Context->Processor = NULL;

fail8:

      Error("fail8\n");

- Context->ProcessorCount = 0;

+    Context->MaximumProcessors = 0;
      EvtchnAbiRelease(Context);

fail7:

@@ -1728,6 +1734,7 @@ EvtchnRelease(
      PXENBUS_EVTCHN_CONTEXT  Context = Interface->Context;
      PXENBUS_FDO             Fdo = Context->Fdo;
      KIRQL                   Irql;
+    ULONG                   ProcessorCount;
      ULONG                   Cpu;

KeAcquireSpinLock(&Context->Lock, &Irql);

@@ -1739,7 +1746,8 @@ EvtchnRelease(

EvtchnInterruptDisable(Context);- for (Cpu = 0; Cpu < Context->ProcessorCount; Cpu++) {

+    ProcessorCount = KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Cpu = 0; Cpu < ProcessorCount; Cpu++) {
          PXENBUS_EVTCHN_PROCESSOR Processor;

ASSERT(Context->Processor != NULL);

@@ -1760,10 +1768,10 @@ EvtchnRelease(
          Processor->Context = NULL;
      }

- ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_EVTCHN_PROCESSOR) * Context->ProcessorCount));

+    ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_EVTCHN_PROCESSOR) * 
Context->MaximumProcessors));
      __EvtchnFree(Context->Processor);
      Context->Processor = NULL;
-    Context->ProcessorCount = 0;
+    Context->MaximumProcessors = 0;

FdoFreeInterrupt(Fdo, Context->Interrupt);

      Context->Interrupt = NULL;
diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
index 3b3f493..27cdace 100644
--- a/src/xenbus/evtchn_fifo.c
+++ b/src/xenbus/evtchn_fifo.c
@@ -499,7 +499,7 @@ EvtchnFifoAcquire(

Trace("====>\n");- ProcessorCount = KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);

+    ProcessorCount = (LONG)KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);

Index = 0;

      while (Index < ProcessorCount) {
diff --git a/src/xenbus/shared_info.c b/src/xenbus/shared_info.c
index 9f11979..2968925 100644
--- a/src/xenbus/shared_info.c
+++ b/src/xenbus/shared_info.c
@@ -55,7 +55,7 @@ struct _XENBUS_SHARED_INFO_CONTEXT {
      PHYSICAL_ADDRESS                Address;
      shared_info_t                   *Shared;
      PXENBUS_SHARED_INFO_PROCESSOR   Processor;
-    ULONG                           ProcessorCount;
+    ULONG                           MaximumProcessors;
      XENBUS_SUSPEND_INTERFACE        SuspendInterface;
      PXENBUS_SUSPEND_CALLBACK        SuspendCallbackEarly;
      XENBUS_DEBUG_INTERFACE          DebugInterface;
@@ -169,7 +169,7 @@ SharedInfoUpcallSupported(
      PXENBUS_SHARED_INFO_CONTEXT     Context = Interface->Context;
      PXENBUS_SHARED_INFO_PROCESSOR   Processor = &Context->Processor[Index];

- ASSERT3U(Index, <, Context->ProcessorCount);

+    ASSERT3U(Index, <, Context->MaximumProcessors);

return (Processor->Vcpu != NULL) ? TRUE : FALSE;

  }
@@ -185,7 +185,7 @@ SharedInfoUpcallPending(
      vcpu_info_t                     *Vcpu;
      UCHAR                           Pending;

- ASSERT3U(Index, <, Context->ProcessorCount);

+    ASSERT3U(Index, <, Context->MaximumProcessors);

if (Processor->Vcpu == NULL)

          return FALSE;
@@ -218,7 +218,7 @@ SharedInfoEvtchnPoll(

DoneSomething = FALSE;- ASSERT3U(Index, <, Context->ProcessorCount);

+    ASSERT3U(Index, <, Context->MaximumProcessors);

if (Processor->Vcpu == NULL)

          goto done;
@@ -583,6 +583,7 @@ SharedInfoAcquire(
      KIRQL                           Irql;
      shared_info_t                   *Shared;
      LONG                            Index;
+    LONG                            ProcessorCount;
      PXENBUS_SHARED_INFO_PROCESSOR   Processor;
      NTSTATUS                        status;

@@ -626,8 +627,8 @@ SharedInfoAcquire(

      if (!NT_SUCCESS(status))
          goto fail5;

- Context->ProcessorCount = KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);

-    Context->Processor = __SharedInfoAllocate(sizeof 
(XENBUS_SHARED_INFO_PROCESSOR) * Context->ProcessorCount);
+    Context->MaximumProcessors = 
KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    Context->Processor = __SharedInfoAllocate(sizeof 
(XENBUS_SHARED_INFO_PROCESSOR) * Context->MaximumProcessors);

status = STATUS_NO_MEMORY;

      if (Context->Processor == NULL)
@@ -635,7 +636,8 @@ SharedInfoAcquire(

Shared = Context->Shared;- for (Index = 0; Index < (LONG)Context->ProcessorCount; Index++) {

+    ProcessorCount = (LONG)KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
+    for (Index = 0; Index < ProcessorCount; Index++) {
          Processor = &Context->Processor[Index];

status = SystemProcessorVcpuId(Index, &Processor->vcpu_id);

@@ -676,14 +678,14 @@ fail7:
          Processor->vcpu_id = 0;
      }

- ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_SHARED_INFO_PROCESSOR) * Context->ProcessorCount));

+    ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_SHARED_INFO_PROCESSOR) 
* Context->MaximumProcessors));
      __SharedInfoFree(Context->Processor);
      Context->Processor = NULL;

fail6:

      Error("fail6\n");

- Context->ProcessorCount = 0;

+    Context->MaximumProcessors = 0;

XENBUS_DEBUG(Deregister,

                   &Context->DebugInterface,
@@ -745,7 +747,7 @@ SharedInfoRelease (

Trace("====>\n");- Index = (LONG)Context->ProcessorCount;

+    Index = (LONG)KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
      while (--Index >= 0) {
          Processor = &Context->Processor[Index];

@@ -754,10 +756,10 @@ SharedInfoRelease (

          Processor->vcpu_id = 0;
      }

- ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_SHARED_INFO_PROCESSOR) * Context->ProcessorCount));

+    ASSERT(IsZeroMemory(Context->Processor, sizeof (XENBUS_SHARED_INFO_PROCESSOR) 
* Context->MaximumProcessors));
      __SharedInfoFree(Context->Processor);
      Context->Processor = NULL;
-    Context->ProcessorCount = 0;
+    Context->MaximumProcessors = 0;

XENBUS_DEBUG(Deregister,

                   &Context->DebugInterface,

References:
- [PATCH] Differentiate between Active processors and Maximum processors
  - From: Owen Smith

Prev by Date: [PATCH] Differentiate between Active processors and Maximum processors
Next by Date: [PATCH] All items in SYSTEM_PROCESSOR array may not be initialized
Previous by thread: [PATCH] Differentiate between Active processors and Maximum processors
Next by thread: [PATCH] All items in SYSTEM_PROCESSOR array may not be initialized
Index(es):
- Date
- Thread

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.