Please add the port getting/setting code, and add the original patch, while @joevt takes time to explain where to insert it.

We are discussing a kernel patch that will make the kernel output to a specific PCIe serial port. Applying the patch will not have any affect unless boot-args is setup to make the kernel do serial port output (usually used by kprintf).

The patch is for PCI cards based on the 16x50 serial controller that have an I/O BAR base address register (such as the StarTech PEX2S953). For a built-in serial port, the I/O base is usually 0x3f8. The patch will change this to the I/O base of a specified PCIe card.

BaseSerialPortLib16550.c has code for getting the I/O address from the PCIe card that has path PcdSerialPciDeviceInfo.

Function GetSerialRegisterBase in BaseSerialPortLib16550.c does a small amount of validation and initialization to make serial output work. The for BusNumber loop goes through all the PCI bridges in the PcdSerialPciDeviceInfo path to make sure they have proper bus numbers, subordinate bus numbers, and I/O ranges. Each node in PcdSerialPciDeviceInfo specifies a device and function (starting from bus 0).

The last node in the PcdSerialPciDeviceInfo path is for the serial port PCIe device. Function GetSerialRegisterBase uses the first I/O BAR for the SerialRegisterBase.

Perhaps CustomPciSerialDevice should be a UEFI path that is separate from gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo ? It's not really necessary that they be the same serial port (for example, you can have OpenCore go to one port and the kernel to go to another port). This way, you can get the device handle using the UEFI path, and use the PCI device protocol from the handle to access the PCI card. You can assume that all the PCI bridges and the PCI device are properly enumerated and setup (all the ranges are valid). Maybe verify that the specified card has an appropriate class code.

The final thing that GetSerialRegisterBase does is enable the I/O of the PCI device and the parent PCI bridges. I don't think we need to worry about parent PCI bridges? Just enable the I/O for the PCI device. I don't think we need to worry about the D0 state (at least in my setup; maybe more modern setups need this).

My patch (using Kernel -> Patch) replaces 66baf803 with 66ba0820 with mask fffff8ff which in assembly is:
mov dx, 0x3f8 -> mov dx, 0x2008 where 0x2008 is the I/O base for my PCIe card (it changes depending on the slot and other PCIe cards). The mask means the patch changes all values in the range 0x3f8..0x3ff to 0x2008..0x200f. I would describe the patch as changing the "base address" rather than the "address range". "base" implies that there is a range.

Since we are making a quirk, you can add a bit more safety to the search and replace. I/O addresses are read and written to using the in and out instructions. Therefore maybe you should ensure that each match is followed by ec or ee within 20 or so bytes (because there may be some instructions between mov and in or out). Add a debug statement if you don't find the in/out instruction. Add a debug statement to count the number of replacements.

In Hopper.app, you can search for matches using this: 66BAF[8-9a-f]03(..)*e[ec] (where the * is a non-greedy capture (which one would usually write as *?)

Are there any kernels that don't use register dx for the port address, or register al for the value? The instructions are from /pexpert/i386/pe_serial.c. They use functions outb and inb. Those are defined in i386/pio.h as:
__asm__ volatile("inb %w1, %b0" : "=a" (datum) : "Nd" (port));
__asm__ volatile("outb %b0, %w1" : : "a" (datum), "Nd" (port));
I'm not sure what Nd means. I guess a is al since the only other value supported by in and out is ax which is a 16-bit value.

The reason for this patch:
pe_serial.c can do serial output to the following (ordered by preference):

1. PCH LPSS UART2: MMIO base: mmio_uart boot-arg, 0xFE036000, or 0xFE034000 (macOS 10.12.6)
2. Legacy: I/O base 0x3f8 (macOS 10.3.9)
3. PCIE_MMIO UART: MMIO base: pcie_mmio_uart boot-arg, or 0xFE410000 (macOS 10.14.1)

My serial PCIe card does not support the PCIE_MMIO UART method (and that method doesn't exist before macOS 10.14), and the legacy method doesn't have a boot-arg to change the I/O base. Therefore, this patch. This patch can't work if serial_init chooses option 1 (PCH) but you can get around that by having boot-arg mmio_uart=0.

So, what CustomPciSerialDevice will do is redirect kernel legacy serial I/O to a PCIe card. For this to work, you also need to make sure that the Apple16X50PCI0 driver does not attach to the serial port because it will stop the serial output. For this I use a code-less kext as described in acidanthera/bugtracker#1954

In my opinion, we can assume that the serial ports are the same for simplicity of the interface. For a quirk I am not sure that trying to match out/in will be easily doable with the compiler generating different code.

For MMIO I think we need to make our patch automatically replace the mmio address too.

All in all there should be decent docs in OC describing all this stuff.

Thanks @joevt and @vit9696.

With @vit9696's help, I implemented an initial draft of both the PMIO and MMIO patches.

As for the PMIO patch, I believe it is safe enough to match the instruction mov dx, 0x3fx (0x66, 0xBA, 0xF8, 0x03) by solving symbol _serial_init. This way the searching process is done only starting from this routine, and with Limit set to 1024 we won't search very farther from the beginning. So, I did not try to match in (0xEC) or out (0xEE) for both simplicity and performance.

For MMIO, there is one constant 0xFE0340xx within _serial_init as of 10.13. With the same matching method talked above, this is easily done as well.

Actually, how about debug kernels? To be honest I did not check carefully, but at a glance 0xFE0340xx did not seem to exist; or I think it is done otherwise (e.g. via mov dword ptrs)?

Under function PatchSetPciSerialDeviceRegisterBase, the mode of patching (i.e. MMIO vs PMIO) is determined based on the size of RegisterBase received from GetSerialRegisterBase function call. Since we override RegisterBase in Misc->Serial section, does the order matter?

In my opinion, we can assume that the serial ports are the same for simplicity of the interface.

Ok, so Serial -> Init is for OpenCore serial output.
CustomPciSerialDevice is for xnu serial output.
If Init or CustomPciSerialDevice are true, then call SerialPortInitialize.
If Init is not true, then don't do OpenCore serial output.
If CustomPciSerialDevice is not true, then don't patch the kernel.

For a quirk I am not sure that trying to match out/in will be easily doable with the compiler generating different code.

My comments about inb and outb was to suggest methods of ensuring that in and out are always present in the same form near the bytes to be patched. I will write a script to check all the macOS kernels to get statistics.

For MMIO I think we need to make our patch automatically replace the mmio address too.

Sounds like fun. macOS has boot-args for changing mmio address for the two MMIO options, but you think it would be better if the values came from the BAR registers of the device specified by PcdSerialPciDeviceInfo?

For MMIO, there is one constant 0xFE0340xx within _serial_init as of 10.13. With the same matching method talked above, this is easily done as well.

That's for the PCH LPSS UART2 option. It's not used if there's a UART at 0xFE0360xx so that should be the register to look for. It has existed in the xnu source code since at least macOS 10.12.6 (I did not check every version of 10.12). Is the PCH LPSS UART2 option usable by BaseSerialPortLib16550.c? Is the PCH LPSS UART2 a PCI device? It seems to have different registers than the PCIE_MMIO UART option - actually the registers are similar but the access width is 32 bits so maybe it can work.

For the PCIE_MMIO UART option, the address to modify is 0xFE4100xx.

As for the PMIO patch, I believe it is safe enough to match the instruction mov dx, 0x3fx (0x66, 0xBA, 0xF8, 0x03) by solving symbol _serial_init. This way the searching process is done only starting from this routine, and with Limit set to 1024 we won't search very farther from the beginning. So, I did not try to match in (0xEC) or out (0xEE) for both simplicity and performance.

But there are other places in the kernel that do serial port output (hibernate? stuff)

What does PMIO stand for?

Under function PatchSetPciSerialDeviceRegisterBase, the mode of patching (i.e. MMIO vs PMIO) is determined based on the size of RegisterBase received from GetSerialRegisterBase function call. Since we override RegisterBase in Misc->Serial section, does the order matter?

If you want to support both MMIO options (LPSS and PCIE), then you need to be able to differentiate between the two. Perform the LPSS patch if PcdSerialUseMmio is true and PcdSerialRegisterAccessWidth is 32 bit. Perform the PCIE_MMIO patch if PcdSerialUseMmio is true and PcdSerialRegisterAccessWidth is 8 bit.

For PMIO, check the PcdSerialUseMmio and PcdSerialRegisterAccessWidth. Don't perform the PMIO patch if PcdSerialUseMmio is false or PcdSerialRegisterAccessWidth is not 8 bit.

@joevt Why would we initialise serial when Init=false? This sounds borked. It’s not “OC serial output” (that should be determined by Debug->Target) but “init serial port”.

PMIO is port-mapped I/O.

@joevt Why would we initialise serial when Init=false? This sounds borked. It’s not “OC serial output” (that should be determined by Debug->Target) but “init serial port”.

PMIO is port-mapped I/O.

Well, this comment is correct. Serial → Init is literally "init serial port", it has nothing to do to the fact whether OC uses serial or not. Perhaps we should separate Enable and Init though, but in reality no PCI card would work without Init,

We need to at least call GetSerialRegisterBase to ensure that IO or MMIO is enabled for the PCI device and parent PCI bridges. SerialPortInitialize calls GetSerialRegisterBase.

If we're not doing serial output in OpenCore, then calling GetSerialRegisterBase should be sufficient, as xnu has its own code that performs initialization and set baud rate.

Should we override baud rate and clock since we are overriding base register?
pe_serial.c:
LEGACY_UART_CLOCK 1843200
DEFAULT_UART_BAUD_RATE 115200

If these patches are contributed, it is ok to me, but otherwise I would act based on the actual need to.

I temporarily removed the MMIO patch, and will go back to it later.

@joevt You are indeed correct that mov dx, 0x3fx not only exists within _serial_init. As a result, I implemented a quick draft which searches such sequence bytes thoroughly at 238628c. Could you please take a look/test?

UPDATE - I just created a test in out TextKextInject tool at d18c175, and looks like my patch worked correctly by patching both _serial_init and hibernation methods.

Regarding baud rate and clock in XNU - We can implement it. I would expect we update these values when performing Serial init. i.e.:

• Misc->Serial section updates the PCD values
• CustomPciSerialDevice kernel quirk patches these values in XNU, if applicable

@vit9696 @mhaeuser What are your thoughts?

I ran some statistics.

IFS=$'\n'
for themacos in $(
for thekernel in $((ls /Volumes/*/System/Library/Kernels/kernel; ls /Volumes/*/mach_kernel) 2> /dev/null); do
	thevolume=$(perl -pe 's|(/Volumes/[^/]*).*|\1|' <<< $thekernel)
	macosversion=$(/usr/libexec/PlistBuddy -c 'Print :ProductVersion' "$thevolume/System/Library/CoreServices/SystemVersion.plist")
	echo "$macosversion $thekernel"
done | sort -V
); do
	thekernel="${themacos#* }"
	perl -0777 -nE '
		$curpos=0;
		while ( /(?{$x=pos()})\x66\xba([\xf8-\xff])\x03/mg ) {
			$offset = ord($1) - 0xf8;
			if ( /(?{$y=pos()})([\xee\xec])/mg ) {
				$out = ord($1) == 0xee ? 1 : 0;
				print $out . " " . $offset . " " . ($y - $x - 4) . "\n";
			} else {
				print "x " . $offset . " x\n";
			}
			pos() = $x + 4;
		}
	' $thekernel > /tmp/matches.txt
	echo "$themacos"
	sort -V /tmp/matches.txt | uniq -c
	echo $(wc -l < /tmp/matches.txt)
done

It seems we need a different patch for Mac OS X versions 10.4 to 10.6. I'll have a look.
10.9.5 Mavericks has a suspicious gap of 46 bytes between a match and the in instruction. I'll double check the disassembly.
counts.txt

Thanks. I took a quick glance at 10.4.11 XNU as well, and indeed a different patch is needed.

For 10.9, we can just change the max search distance (currently 32) to a higher value. e.g. 64.

I don't think we can do a simple search/replace for 10.4.11. It has optimizations that make it difficult to find.
Look at this example:

003bd181 BBFB030000             mov        ebx, 0x3fb
003bd186 B880FFFFFF             mov        eax, 0xffffff80
003bd18b 89DA                   mov        edx, ebx
003bd18d EE                     out        dx, al
003bd18e B2F8                   mov        dl, 0xf8
003bd190 B85A000000             mov        eax, 0x5a
003bd195 EE                     out        dx, al
003bd196 EC                     in         al, dx

It moves 0x3fb into ebx which is used for the first out, but it replaces only the low byte with 0xf8 for the next out and in. I suppose you could just replace the entire function (but you have to replace all functions that use the same static globals).

This sounds complex. In my opinion, do not bother with 10.4 for now?

do not bother with 10.4 for now?

I'm ok with that.

The Mavericks 46 byte gap seems benign.

ffffff80006edb5b 66BAFD03               mov        dx, 0x3fd
ffffff80006edb5f 90                     nop

                                    loc_ffffff80006edb60:
ffffff80006edb60 C745FC00000000         mov        dword [rbp+var_4], 0x0       ; CODE XREF=_serial_putc+80
ffffff80006edb67 8B45FC                 mov        eax, dword [rbp+var_4]
ffffff80006edb6a 3D0F270000             cmp        eax, 0x270f
ffffff80006edb6f 7F1C                   jg         loc_ffffff80006edb8d

ffffff80006edb71 6666666666662E0F1F840000000000  nop        word [cs:rax+rax]

                                    loc_ffffff80006edb80:
ffffff80006edb80 FF45FC                 inc        dword [rbp+var_4]            ; CODE XREF=_serial_putc+75
ffffff80006edb83 8B45FC                 mov        eax, dword [rbp+var_4]
ffffff80006edb86 3D10270000             cmp        eax, 0x2710
ffffff80006edb8b 7CF3                   jl         loc_ffffff80006edb80

                                    loc_ffffff80006edb8d:
ffffff80006edb8d EC                     in         al, dx                       ; CODE XREF=_serial_putc+47

Attached is a list of file offsets for non-zero gaps.
counts_offsets_nonzerogaps.txt

OK, thanks. I just increased the maximum search size to 64. Looks fine now?

If so, let's go back to the MMIO patch.

Let me know what you would like me to check or test.

@dakanji This makes sense. Thanks. Renamed.

Looks good

@joevt @mhaeuser Do you have any other thoughts? If not, shall we merge?

I have only the following two suggestions:

1. Regarding this:

The patch changes the PMIO register base address from the default \texttt{0x3F8-0x3FF} assumed by XNU to the actucal one used by the PCI serial card, such that the real PMIO address of the serial device will also be recognised by XNU.

I think this needs to be reworded so that base is not implied to be a range. Rather, base points to the beginning of a range. Also, the patch also works for non PCI serial ports (these may appear as ACPI devices? I have to boot my hackintosh into macOS to be sure). Maybe something like this:

The patch changes the PMIO register base address that the XNU kernel uses for serial input and output from that of the default built-in COM1 serial port \texttt{0x3F8} to the base address stored in the first IO BAR of a specified PCI device or to a specific base address (for example, \texttt{0x2F8} for COM2).

2. Regarding this note:

\emph{Note 4}: Since this patch does not provide MMIO support, \texttt{UseMmio} under section \texttt{Misc->Serial->Custom} must be disabled.

I think the user should know that this patch is for PMIO but they can use MMIO for OpenCore serial output and that there are boot-args to enable MMIO for XNU (but the addresses are not calculated automatically - they need to enter the correct address in boot-args themselves). Maybe this:

\emph{Note 4}: This patch is for PMIO support and is therefore not applied if \texttt{UseMmio} under section \texttt{Misc->Serial->Custom} is false. In that case, there are boot-args used in XNU's pe_serial.c that allow MMIO support.

1. I am ok with the change.

2. I do not think we should mention anything at source code level; plus I prefer having this at UseMmio instead.

I do not think we should mention anything at source code level

Note that the docs already includes at least one source code level reference ... something about a Tianocore source file.
Seems better to provide a reference unless will be listing the args. On the other hand, what is to stop the source file from changing?

We can just mention the other boot-args and then the user can search for the source file themselves to learn how they work and how they differ from each other and from the COM1 PMIO option and what version of macOS they work with. MMIO support exists but is not part of this patch so we can be vague about it. MMIO is fully supported for OpenCore but not XNU except for the boot-args which are sufficient for full capability.

there are boot-args pcie_mmio_uart= and mmio_uart= that allow the kernel to use MMIO for serial port access (in certain versions of macOS).

Thanks. I updated the descriptions at Docs: Update quirk description.