Dashboard

I think the variable name "EventData" here is misleading. I think there actually was a "EventData" in the disabled code, along with something that should be called "EventNotification" we use to reg...

I think the variable name "EventData" here is misleading. I think there actually was a "EventData" in the disabled code, along with something that should be called "EventNotification" we use to register the event notification.

  • More
  • CR-80
  • resumed reviewing
  • More
  • CR-117
  • finished reviewing
Double semicolon.

Double semicolon.

NTSTATUS foobar(void)

NTSTATUS foobar(void)

From looking at this code, the two types of firmware flags are also exclusive with regard to each other. I'm surprised the parameter checks don't cover this case (i.e. if ((WhichType & BL_MM_INCLUD...

From looking at this code, the two types of firmware flags are also exclusive with regard to each other. I'm surprised the parameter checks don't cover this case (i.e. if ((WhichType & BL_MM_INCLUDE_ONLY_FIRMWARE_MEMORY) == BL_MM_INCLUDE_ONLY_FIRMWARE_MEMORY) return;)

This is really just checking for any bits that don't have defines, i.e. are invalid flags.

This is really just checking for any bits that don't have defines, i.e. are invalid flags.

Just saw that these are combinations of flags, not single bits. This makes sense then, though you could still simplify it to the single & if you wanted.

Just saw that these are combinations of flags, not single bits. This makes sense then, though you could still simplify it to the single & if you wanted.

This is equivalent to WhichTypes & ~(INCLUDE_NO_FIRMWARE | INCLUDE_ONLY_FIRMWARE) meaning "if any flags other than these 2 are specified". If you really want "one of these two must be specified" yo...

This is equivalent to WhichTypes & ~(INCLUDE_NO_FIRMWARE | INCLUDE_ONLY_FIRMWARE) meaning "if any flags other than these 2 are specified".
If you really want "one of these two must be specified" you need !(WhichTypes & (INCLUDE_NO_FIRMWARE | INCLUDE_ONLY_FIRMWARE)) aka ~(WhichTypes & INCLUDE_NO_FIRMWARE) && !(WhichTypes & INCLUDE_ONLY_FIRMWARE)

QuadPart = (ULONG_PTR)VirtualAddress seems more logical

QuadPart = (ULONG_PTR)VirtualAddress seems more logical

Remove ULONG_PTR cast from MmPteBase and you get the << 2 for free Also, 12 is PAGE_SHIFT

Remove ULONG_PTR cast from MmPteBase and you get the << 2 for free
Also, 12 is PAGE_SHIFT

No reason to truncate this to LowPart... I think

No reason to truncate this to LowPart... I think

User mappings are automatically top-down? Seems odd

User mappings are automatically top-down? Seems odd

Use (PVOID)(ULONG_PTR)QuadPart for x64 compat?

Use (PVOID)(ULONG_PTR)QuadPart for x64 compat?

MAXULONG. Also, this maximum is presumably different for x64?

MAXULONG. Also, this maximum is presumably different for x64?

You don't need this again, just make the above say DoFirmware || DoFirmware2?

You don't need this again, just make the above say DoFirmware || DoFirmware2?

+= here would look more consistent

+= here would look more consistent

Needs a better name. Some variation of "firmware no-coalesce", presumably

Needs a better name. Some variation of "firmware no-coalesce", presumably

Add some defines?

Add some defines?

This is not a boolean

This is not a boolean

(1ULL << 52) - 1 would avoid some F-counting

(1ULL << 52) - 1 would avoid some F-counting

This is broken, and not only on 64 bit.

This is broken, and not only on 64 bit.

Physical address and PVOID don't go together

Physical address and PVOID don't go together

There's quite a lot of shared validation code between these three functions. Would a shared helper be feasible?

There's quite a lot of shared validation code between these three functions. Would a shared helper be feasible?

MAXULONG

MAXULONG

MAXULONG

MAXULONG

Align this with lines 478ff. No reason to have code that does the same look completely different

Align this with lines 478ff. No reason to have code that does the same look completely different

MAXULONG

MAXULONG

fewer https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/tongue.gif

fewer

You have a backup plan for "No RAM found"? That's great computing right there! https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/biggrin.gif

You have a backup plan for "No RAM found"? That's great computing right there!