search

acidanthera / bugtracker

Acidanthera Bugtracker
385 stars 44 forks source link

SerialInit does not work for all serial ports #1954

Closed joevt closed 2 years ago

joevt commented 2 years ago

The main issue SerialInit relies on BaseSerialPortLib16550 which relies on PCDs. It is not clear from the documentation how it can be used with arbitrary serial ports. For example, I have a PCIe serial port card which contains two serial ports. Can OpenCore output log messages to my serial ports or do I need to change some build settings? Maybe it can work since my serial ports are based on 16x50 which BaseSerialPortLib16550 should be able to work with, but what about other serial port options? Ideally, this feature should work without having to rebuild OpenCore.

Possible solution Maybe OpenCore should have options to select a device that has SerialIo protocol (using a device path in case there are many serial ports)? The serial port functions in OpenCore can be wrapped to support either the SerialIo Protocol or the SerialPortLib. Does the SerialPortLib work beyond ExitBootServices? It could be possible - all you need to use the 16x50 serial port is the in and out x86 instructions to access static I/O addresses. If SerialPortLib and SerialIo protocol have different usage or behaviour after ExitBootServices then that should be taken into account.

Using PCIe serial port for serial kprintf I have a kernel patch to use a PCIe serial port for serial kprintf in macOS. Click the spoiler to see:

PCIe serial patch for xnu kernel ``` Arch Any Base Comment serial output to PCIe card (change 3f8-3ff to 2008-200f which is the range of BAR0 of our PCIe serial card) Count 0 Enabled Find Zrr4Aw== Identifier kernel Limit 0 Mask ///4/w== MaxKernel MinKernel Replace ZroIIA== ReplaceMask ///4/w== Skip 0 ```

For this to work, I need to enable I/O addresses for the PCIe device. This can be done in the UEFI Shell: mm 180000004 1 -w 1 -pcie (my PCIe serial port is at 18:0:0) but a better way is to load a UEFI driver. SerialDxe.efi is one possibility, but that uses SerialPortLib so it takes more work to build. PciSioSerialDxe.efi is easy to build and seems to work directly with 16x50 based serial ports, at least for enabling I/O addresses but I haven't used it to transmit text over serial port while still in UEFI. Is there a way to redirect console input and output in the UEFI Shell? I understand there are some nvram variables for this but can they work when the serial driver loading is deferred? I think it would be useful for OpenShell to have a command to redirect console input/output and for OpenCore to be able to do the same.

For serial kprintf, you need some boot-args: debug=0x108 -v serial=3 msgbuf=1048576 serialbaud=115200 I'm not sure what msgbuf does or what the default value is. It doesn't appear to be a boot-arg in the latest xnu. It is mentioned in the OpenCore documentation.

It may be that when macOS attaches its Apple16X50PCI0 driver, it will disable kprintf. kprintf will start working again if I launch Serial.app to open the serial port. To stop Apple16X50PCI0 from attaching, I tried a DeviceProperties patch to alter the class-code:

DeviceProperties -> Add ``` PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0) class-code /////w== disabled reserved for serial kprintf ```
DeviceProperties -> Delete ``` PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0) class-code ```

The PCIe device has two functions (function 0 and function 2), one for each serial port. I only want to disable matching for the first serial port (that I've reserved for serial kprintf) at function 0 as indicated by the second 0x0 in the last Pci node of the device path. However, the patch gets applied to both functions. Similarly, if I change that 0x0 to 0x2 like this: PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x2) then the patch is not applied to either function. The OpenCore documentation regarding DeviceProperties doesn't seem to have an explanation for this (but it does say devices that don't exist in ACPI may have issues). It seems to me that OpenCore should have its own method of applying DeviceProperties, perhaps using Lilu, maybe by patching IOService->registerService so that the properties are applied before driver matching occurs. Currently, the resulting properties in the I/O Registry can only be Data. OpenCore's device properties patch could allow arbitrary types such as Number, String, Dict, Array.

Instead of using DeviceProperties to stop drivers from attaching to the serial port that is reserved for serial kprintf, I used a codeless kext injected by OpenCore to attach to the reserved serial port. I used IOPropertyMatch to match only function 0 (which has IOChildIndex 1). With this method, function 0 is no longer usable in macOS (it's only used for serial kprintf) and I can still use the other serial port (function 2).

PCIeSerialDisable.kext/Contents/Info.plist ``` CFBundleDevelopmentRegion English CFBundleIdentifier com.joevt.driver.PCIeSerialDisable CFBundleInfoDictionaryVersion 6.0 CFBundleName PCIeSerialDisable CFBundlePackageType KEXT CFBundleSignature ???? CFBundleVersion 1.0.1 IOKitPersonalities Disable a specific PCIe serial port which is used for serial kprintf (IOChildIndex 1 is function 0 of a PCIe card that has two functions - one per PCIe serial port) CFBundleIdentifier com.apple.kpi.iokit IOClass IOService IOProbeScore 30000 IOProviderClass IOPCIDevice IOPCIMatch 0x9100125b IOPropertyMatch IOChildIndex 1 IOName PCIeSerialDisable OSBundleRequired Safe Boot ```
Kernel -> Add ``` Arch Any BundlePath PCIeSerialDisable.kext Comment PCIeSerialDisable Enabled ExecutablePath MaxKernel MinKernel PlistPath Contents/Info.plist ```

I haven't tried this from Thunderbolt. It should work if the I/O addresses never change. This requires that the Thunderbolt devices exist in UEFI before macOS boots and that you don't do any hot plugging of Thunderbolt devices connected to the same port or bus that the PCIe card is connected to.

joevt commented 2 years ago

I can get serial output from EFI to my PCIe serial port by modifying console input and output device paths (I don't think console output is the same as the output that SerialInit in OpenCore gives).

First, I load the serial driver using Driver####. Loading the driver this way loads it before the Startup Manager (when you would hold the option key to view boot options from the Apple Boot Picker).

source ~/Downloads/gfxutil.sh
setbootvar Driver0086 1 "PciSioSerialDxe.efi" /Volumes/EFI*/*bay2/../drivers/PciSioSerialDxe.efi
setdriverorder Driver0086 Driver0080

(Driver0080 is where I have the apfs driver loading from)

Then you get additional options that can be used as console input and output device paths. The list of options are stored in ConInDev, ConOutDev, and ErrOutDev:

nvramp $efiguid:ConInDev | {echo "#ConIn all" ; gfxutil ; echo} | perl -pe 's|,/|\n|g'
nvramp $efiguid:ConOutDev | {echo "#ConOut all" ; gfxutil ; echo} | perl -pe 's|,/|\n|g'
nvramp $efiguid:ErrOutDev | {echo "#ErrOut all" ; gfxutil ; echo} | perl -pe 's|,/|\n|g'

The list for each is like this (one set for each serial port):

PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenPcAnsi()
PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100()
PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100Plus()
PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenUtf8()

For ConOut and ErrOut there's also the display device path included in each list:

PciRoot(0x0)/Pci(0x5,0x0)/Pci(0x0,0x0)

You can find the ioreg path that the EFI path is pointing to like this:

gfxutil -t | grep "PciRoot(0x0)/Pci(0x5,0x0)/Pci(0x0,0x0)"

The result indicates my graphics card:

11:00.0 10de:1180 /PCI0@0/NRP5@5/GFX0@0 = PciRoot(0x0)/Pci(0x5,0x0)/Pci(0x0,0x0)

So I changed ConIn, ConOut, and ErrOut to my serial port using VT100 terminal emulation since that's one of the options available in Serial.app which is what I'm using to view the output:

setnvramhex $efiguid:ErrOut $(gfxutil "PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100()")
setnvramhex $efiguid:ConOut $(gfxutil "PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100()")
setnvramhex $efiguid:ConIn $(gfxutil "PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100()")

With these settings, I can boot into RefindPlus, go into the EFI Shell, and see output go to my serial port. Since it's using VT100 terminal emulation, I also get the 8 colors and two intensity levels for the text just like what you see in the EFI Shell. I don't know if there's a mode that does the text colors and utf8?

But for some reason, my ConOut ended up pointing to two devices:

PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)/Uart(115200,8,N,1)/VenVt100()
PciRoot(0x0)/Pci(0x5,0x0)/Pci(0x0,0x0)

so the output is being sent to the screen and to the serial port. The problem is that the screen is stuck at 24 rows which causes the cursor to jump to the 24th row in the serial terminal (such as after an ls command in the EFI Shell).

With OpenCore, I get some serial output until OpenCore outputs "Install Console control" then I don't see serial output until the kernel's serial kprintf begins. Maybe there are some config.plist options I can tweak here to affect the console in OpenCore?

If I try to enter OpenShell.efi from OpenCore with these serial port settings, then I get a halt error.

joevt commented 2 years ago

A couple things I noticed:

joevt commented 2 years ago

By changing TextRenderer from BuiltinGraphics to SystemGeneric, I can get serial console output from OpenCore to continue after Install Console control. All other issues remain.

PMheart commented 2 years ago

If we need to update the PCD values, it can be done as shown in https://github.com/acidanthera/OpenCorePkg/commit/011ae925ef7b789529cba60575696119dbd24631. Does this help?

joevt commented 2 years ago

It appears PcdSerialPciDeviceInfo is a kind of PCI device path that can point to an arbitrary serial port PCIe device, so maybe it's sufficient for the SerialInit issue.

Shouldn't the PCD values be set before calling SerialPortInitialize? Or is it ok to change the values afterward?

But what exactly is a PCD value? Where are PCD values stored? Do they work on EFI 1.1 Macs like my MacPro3,1? If it requires recompiling, then it's not useful. Is there a way to load PCD values from nvram or config? Can this loading of PCD values be performed by OpenCore or a driver that loads before OpenCore?

PMheart commented 2 years ago

Yes, we can just read the values from config, which can be done within OcMainLib.

PMheart commented 2 years ago

UPDATE: I created a sample of PCD value overriding at https://github.com/acidanthera/OpenCorePkg/commit/36b58293cb7802b44741ee3079499018ccc80a71. (where PcdSerialPciDeviceInfo is the new PCD value to be set; for now I put a random one like 12 00 00 FF)

If this will be what we expect, then we can start working on this. Or did I miss anything?

joevt commented 2 years ago

Don't DevicePath and PcdSerialPciDeviceInfo mean the same thing? Certainly DevicePath is easier to read and understand.

I think the equivelant PcdSerialPciDeviceInfo for PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0) is 09 00 00 00 01 00 00 00 FF but I need to double check the code.

Are the nested array and dict necessary? It could be simplified:

        <key>Serial</key>
        <dict>
            <key>DevicePath</key>
            <string>PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)</string>
            <key>Init</key>
            <false/>
            <key>PcdSerialPciDeviceInfo</key>
            <data>CQAAAAEAAAD/</data>
        </dict>

Is Init a replacement for SerialInit?

PMheart commented 2 years ago

Don't DevicePath and PcdSerialPciDeviceInfo mean the same thing? Certainly DevicePath is easier to read and understand.

I am not sure to be honest. I was just trying to implement what you proposed. Actually, isn't it always 4-byte long as per https://github.com/acidanthera/audk/blob/bb1bba3d776733c41dbfa2d1dc0fe234819a79f2/MdeModulePkg/MdeModulePkg.dec#L1386?

Are the nested array and dict necessary? It could be simplified:

@vit9696 introduced this idea at first. I do not know whether we should add multiple devices support (i.e. via array).

Is Init a replacement for SerialInit?

I don’t know either. In my opinion, better to rename it to Enable?

joevt commented 2 years ago

I am not sure to be honest. I was just trying to implement what you proposed.

Does the stuff to be implemented require the DevicePath? Only if you were going to do something about the device properties issue (which I don't require since I'm using the kext method to disable the used serial port).

Actually, isn't it always 4-byte long as per https://github.com/acidanthera/audk/blob/bb1bba3d776733c41dbfa2d1dc0fe234819a79f2/MdeModulePkg/MdeModulePkg.dec#L1386?

I think the Pcd's with type VOID* are pointers to a variable length array of bytes. In the case of PcdSerialPciDeviceInfo, each item in the array should be 4 bytes, so I should use these bytes in my case (ignoring power management): 09 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 FF

But it seems to me that since Pcd's are not named with an ASCII string (are they? I don't know enough about Pcd's) then OpenCore will only be able to change Pcd's that it knows about (it cannot change a Pcd given its name as an ASCII string unless it knows the name). In that case, you might as well create a structure for each Pcd that makes it easiest to read. In the case of PcdSerialPciDeviceInfo, a format like this might be best: Pci(0x9,0x0,0x0)/Pci(0x0,0x0,0x0)/Pci(0x1,0x0,0x0)/Pci(0x0,0x0,0x0) where the third value in each node is optional and 16 bits (stored as little-endian I guess).

@vit9696 introduced this idea at first. I do not know whether we should add multiple devices support (i.e. via array).

Do you mean multiple Serial devices? Then it would be more like this:

        <key>Serial</key>
        <array>
            <dict>
                <key>DevicePath</key>
                <string>PciRoot(0x0)/Pci(0x9,0x0)/Pci(0x0,0x0)/Pci(0x1,0x0)/Pci(0x0,0x0)</string>
                <key>Init</key>
                <false/>
                <key>PcdSerialPciDeviceInfo</key>
                <data>EgAA/w==</data>
            </dict>
        </array>

I don’t know either. In my opinion, better to rename it to Enable?

I think so, If SerialInit does more than just Initialize something; such as changing debugging behaviour.

vit9696 commented 2 years ago

Fixed PCD variables are just global variables inside OpenCore, which we can update as long as we know how. When the complete list of the variables we need to change is figured out, they should be put to config.plist and made configurable. Full list of candidates is present in BaseSerialPortLib16550.inf and looks as follows:

gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterAccessWidth
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseMmio
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHardwareFlowControl
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialDetectCable
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialBaudRate
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialLineControl
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialPciDeviceInfo
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize
gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterStride

To be able to write to the variables of type data (e.g. PciDeviceInfo) we will need to set it to an array of 0xFF of at least 9 bytes, so that 2 structures of PCI_UART_DEVICE_INFO (root bus and device) and a terminating 0xFF can be put there, but allow up to 5 structures (5*4+1) seems logical to me. Since the value is not a device path, using string representation like Pci(0x9,0x0,0x0) will require a lot of parsing and checks that it contains the right data of expected size, thus I request to opt with a mere byte array.

I do not believe we need to support multiple device paths at once, and BaseSerialPortLib16550 does not support that either. Yet I do believe that being able to set serial to just the PCI cards is inadequate, since most serial outputs are not PCI-based, and thus we will need to provide configuration to at least:

@joevt, please add what you think is also necessary. Given the amount of entries to become configurable, Serial dictionary looks essential, and yes, SerialInit needs to be moved there under the name Init.

PMheart commented 2 years ago

Thanks @vit9696!

Shall we create a new structure for config.plist? Afterwards, I can work on that.

vit9696 commented 2 years ago

I think so.

joevt commented 2 years ago

but allow up to 5 structures (5*4+1) seems logical to me.

That should be enough for my PCIe Serial port path. Maybe double that for safety (41 bytes) in case someone wants to try something weird like serial port from Thunderbolt. Or make it arbitrary length and allocate memory from the pool? It's a singleton so it doesn't need to be cleaned up afterward to avoid memory leaks assuming OpenCore is opened multiple times during the same boot.

Since the value is not a device path, using string representation like Pci(0x9,0x0,0x0) will require a lot of parsing and checks that it contains the right data of expected size, thus I request to opt with a mere byte array.

That should be fine. It's easy enough to construct by hand using sed, xxd, and base64 if necessary.

I do not believe we need to support multiple device paths at once, and BaseSerialPortLib16550 does not support that either. Yet I do believe that being able to set serial to just the PCI cards is inadequate, since most serial outputs are not PCI-based, and thus we will need to provide configuration to at least:

  • BaudRate
  • PciDeviceInfo
  • UseMmio
  • RegisterAccessWidth
  • RegisterBase
  • RegisterStride

@joevt, please add what you think is also necessary. Given the amount of entries to become configurable, Serial dictionary looks essential, and yes, SerialInit needs to be moved there under the name Init.

To be safe, better just add all of the PcdSerial* pcd's that you listed. Adding 12 will take similar effort to adding 6 I believe. You didn't list PcdSerialUseHalfHandshake but I don't think that's used by BaseSerialPortLib16550.inf

vit9696 commented 2 years ago

That should be enough for my PCIe Serial port path. Maybe double that for safety (41 bytes) in case someone wants to try something weird like serial port from Thunderbolt. Or make it arbitrary length and allocate memory from the pool? It's a singleton so it doesn't need to be cleaned up afterward to avoid memory leaks assuming OpenCore is opened multiple times during the same boot.

You cannot allocate from pool there, as you need to update a global array of fixed size.

To be safe, better just add all of the PcdSerial* pcd's that you listed. Adding 12 will take similar effort to adding 6 I believe.

Fair.

You didn't list PcdSerialUseHalfHandshake but I don't think that's used by BaseSerialPortLib16550.inf

Yes, it is not used, so I do not see a reason to add it.

PMheart commented 2 years ago

One question - shall we always override all 12 keys? I do not see any good way not to override if we will.

As for config structure, I would agree with a new field at Misc. i.e. Misc -> Serial -> pcd keys to be overridden like on https://github.com/acidanthera/bugtracker/issues/1954#issuecomment-1082807637.

vit9696 commented 2 years ago

I think it is ok to fill all the fields with known values via .plist.

PMheart commented 2 years ago

https://github.com/acidanthera/OpenCorePkg/commit/c3b41c98d65926eff783b58a8d4c50912136b528 If we are ok with such hierarchy, then we can start working on this code-wise. I expect all these keys to be hardcoded into OcConfigurationLib under Serial entry. Then the default value from https://github.com/acidanthera/audk/blob/872f953262d68a11da7bc2fb3ded16df234b8700/MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf will be set if missing.

vit9696 commented 2 years ago

serial output to PCIe card (change 3f8-3ff to 2008-200f which is the range of BAR0 of our PCIe serial card

I think we should introduce a kernel quirk to perform such a patch, based on Misc → Serial data.

PMheart commented 2 years ago

@joevt Would you please explain how this is calculated based on Serial data?

EDIT - I created the quirk hierarchy at https://github.com/acidanthera/OpenCorePkg/pull/331. You can work on the PatchCustomPCISerialDevice () method in Library/OcAppleKernelLib/CommonPatches.c.

joevt commented 2 years ago

I've added my notes for the suggested quirk in https://github.com/acidanthera/OpenCorePkg/pull/331