Re: [ros-svn] [hbirr] 12690: - Implemented KeAcquireInterruptSpinLock and KeReleaseInterruptSpinLock.

[Alex Ionescu]

[e-mail] wrote:
> Implemented KeAcquireInterruptSpinLock and KeReleaseInterruptSpinLock.
> Implemented the interrupt handling for smp machines.
>
> Updated files:
> trunk/reactos/include/ddk/kefuncs.h
> trunk/reactos/include/ntos/zwtypes.h
> trunk/reactos/ntoskrnl/io/irq.c
> trunk/reactos/ntoskrnl/ke/i386/irq.c
> trunk/reactos/ntoskrnl/ke/spinlock.c
>
> Ros-svn mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-svn

Hi,

Thanks for adding this feature to the kernel, I only have one problem 
with it... Would it be possible to modify the code to use the documented 
KINTERRUPT structure? This would (already does) wreck havoc on the new 
header system, which uses documented Windows structures (except in 
extreme cases where our code is very old, and those structures were 
unavailable, ie the Ob Manager structures... although enventually that 
code will have to be updated).

The correct KINTERRUPT structure is:

typedef struct _KINTERRUPT {
    CSHORT              Type;
    CSHORT              Size;
    LIST_ENTRY          InterruptListEntry;
    PKSERVICE_ROUTINE   ServiceRoutine;
    PVOID               ServiceContext;
    KSPIN_LOCK          SpinLock;
    ULONG               TickCount;
    PKSPIN_LOCK         ActualLock;
    PVOID               DispatchAddress;
    ULONG               Vector;
    KIRQL               Irql;
    KIRQL               SynchronizeIrql;
    BOOLEAN             FloatingSave;
    BOOLEAN             Connected;
    CHAR                Number;
    UCHAR               ShareVector;
    KINTERRUPT_MODE     Mode;
    ULONG               ServiceCount;
    ULONG               DispatchCount;
    ULONG               DispatchCode[106];
} *KINTERRUPT*, *PKINTERRUPT;

vs ours:

typedef struct _KINTERRUPT
{
   ULONG Vector;
   KAFFINITY ProcessorEnableMask;
   KSPIN_LOCK SpinLock;
   PKSPIN_LOCK ActualLock;
   BOOLEAN Shareable;
   BOOLEAN FloatingSave;
   CHAR ProcessorNumber;
   PKSERVICE_ROUTINE ServiceRoutine;
   PVOID ServiceContext;
   LIST_ENTRY Entry;
   KIRQL Irql;
   KIRQL SynchLevel;
   KINTERRUPT_MODE InterruptMode;
} KINTERRUPT;

Type/Size are Windows Ob Manager headers for each Object...if you look 
in KOBJECTS you'll find the right value...but we don't check for this 
yet, so it's not important yet. I don't know what "Shareable" is 
however. But we only need to rename/reposition some of the items. 
ProcessorEnableMask also seems to be missing in the real defintion.

If you'd be kind enough to do these changes, I would really appreciate 
it. If you have any comments, let me know.

Best regards, and happy new year,
Alex Ionescu

[Alex Ionescu]

> The correct KINTERRUPT structure is:
> typedef struct _KINTERRUPT {
> ...
>    UCHAR               ShareVector;
> ...
> } *KINTERRUPT*, *PKINTERRUPT;

That should be BOOLEAN.
> I don't know what "Shareable" is however.

It's ShareVector, of course. I don't know why I missed it.
> But we only need to rename/reposition some of the items. 
> ProcessorEnableMask also seems to be missing in the real defintion.

And it never existed, especially after your correct changes.
> If you'd be kind enough to do these changes, I would really appreciate 
> it. If you have any comments, let me know.

I have done the changes locally in my tree and it works well, but since 
you worked with the code I will be waiting for your input.
> Best regards, and happy new year,
> Alex Ionescu

Bis.

[Hartmut Birr]

Alex Ionescu schrieb:
> Hi,
>
> Thanks for adding this feature to the kernel, I only have one problem 
> with it... Would it be possible to modify the code to use the 
> documented KINTERRUPT structure? This would (already does) wreck havoc 
> on the new header system, which uses documented Windows structures 
> (except in extreme cases where our code is very old, and those 
> structures were unavailable, ie the Ob Manager structures... although 
> enventually that code will have to be updated).

I see no reason to make all internal structures from ntoskrnl compatible 
with the M$ one. I'm not sure if the KINTERRUPT structure is public 
documented. The end of the structure looks like a little bit if it was 
extracted from code or from a symbol file. For smp machines we need an 
array of KINTERRUPT structures. IMOH it should be an header with some 
informations (count, the nice ProcessorEnableMask, ..) and an array of 
the real interrupt structures. But in this case the structures for the 
Io- and Ke-functions are different.

- Hartmut

[Alex Ionescu]

Hartmut Birr wrote:
> I see no reason to make all internal structures from ntoskrnl 
> compatible with the M$ one.

Me neither, unless they are publicly documented.
> I'm not sure if the KINTERRUPT structure is public documented.

It's widely available, so people are bound to use it.
> The end of the structure looks like a little bit if it was extracted 
> from code or from a symbol file.

I don't really understand what's wrong with the end. I looked at the 
structure from 1) our w32api 2) ntifs.h 3) ms symbols.
> For smp machines we need an array of KINTERRUPT structures. IMOH it 
> should be an header with some informations (count, the nice 
> ProcessorEnableMask, ..) and an array of the real interrupt structures.

I think that's how IoConnectInterrupt works.
> But in this case the structures for the Io- and Ke-functions are 
> different.

Yes, a meta-function for IoConnectInterrupt storing the data you said. 
That one is not publically documented however, so ours can be anything 
you like. In my local test change, I stored the array and 
ProcessorEnableMask.

I'm only asking to move two or three items up or down, and rename two of 
them. The code changes took me less then 5 minutes. I stronlgy believe 
that we must duplicate public structures.
> Hartmut

Best regards,
Alex Ionescu