[SeaBIOS] Re: [PATCH v4] Add basic MXM 3.0 interrupt support

[Kevin O'Connor]

On Sun, Apr 14, 2024 at 10:30:55AM +0000, Riku Viitanen via SeaBIOS wrote:
> VGAROMs on MXM graphics cards need certain int15h functions present
> to function properly.
>
> On HP EliteBook 8560w with coreboot and Quadro 2000M, this warning
> is displayed for 30 seconds, making every boot extremely slow:
> ERROR: Valid MXM Structure not found
> POST halted for 30 seconds, P-state limited to P10...
>
> This patch implements the minimum functionality to get rid of it: the
> functions 0 and 1, version 3.0 of the specification [1]. Older version
> 2.1 is not implemented.
>
> Functions are enabled only if romfile "mxm-30-sis" exists. These functions
> are specific to the MXM revision, but not the board or GPU. Some boards
> might require more of the optional functions to be implemented, however.
>
> Function 0 returns information about supported functions and revision.
>
> Function 1 returns a pointer to a MXM configuration structure in low
>
> memory. This is copied from romfile "mxm-30-sis". It can be extracted
> from vendor BIOS, by using this same interrupt. I wrote a tool [2] to do
> that from Linux.
>
> TEST: HP EliteBook 8560w boots without error and delay, graphics work.
>
> [1]: MXM Graphics Module Software Specification Version 3.0, Revision 1.1
> [2]: https://codeberg.org/Riku_V/mxmdump/

Thanks.  In general it looks good to me.  I have a few minor comments.
> Signed-off-by: Riku Viitanen [e-mail]
>
> src/vgahooks.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/src/vgahooks.c b/src/vgahooks.c
> index 1f149532..6adf5b9b 100644
> --- a/src/vgahooks.c
> +++ b/src/vgahooks.c
> @@ -11,13 +11,16 @@
>  #include "hw/pcidevice.h" // pci_find_device
>  #include "hw/pci_ids.h" // PCI_VENDOR_ID_VIA
>  #include "hw/pci_regs.h" // PCI_VENDOR_ID
> +#include "malloc.h" // malloc_low
>  #include "output.h" // dprintf
> +#include "romfile.h" // struct romfile_s, romfile_find
>  #include "string.h" // strcmp
>  #include "util.h" // handle_155f, handle_157f
>
> #define VH_VIA 1
>  #define VH_INTEL 2
>  #define VH_SMI 3
> +#define VH_MXM30 4
>
> int VGAHookHandlerType VARFSEG;
>
> @@ -296,6 +299,77 @@ winent_mb6047_setup(struct pci_device *pci)
>      SmiBootDisplay = 0x02;
>  }
>
> +/****************************************************************
> MXM 3.0 hooks
> ****************************************************************/
>
> +void *MXM30SIS VARFSEG;
>
> +/* Function 0: Return Specification Support Level */
> +static void
> +mxm30_F00(struct bregs *regs)
> +{
> regs->ax = 0x005f; /* Success */
> regs->bl = 0x30; /* MXM version 3.0 */
> regs->cx = (1<<0) | (1<<1); /* Supported functions */
> set_success(regs);
>
> +}
>
> +/* Function 1: Return Pointer to MXM System Information Structure */
> +static void
> +mxm30_F01(struct bregs *regs)
> +{
> switch (regs->cx) {
> case 0x0030:
>    regs->ax = 0x005f; /* Success */
>    regs->es = (u32)GET_GLOBAL(MXM30SIS)/16;
>    regs->di = (u32)GET_GLOBAL(MXM30SIS)%16;

This should use SEG_LOW and LOWFLAT2LOW(MXM30SIS).
>    set_success(regs);
>    break;
> default:
>    handle_155fXX(regs);
>    break;
> }
>
> +}
>
> +static void
> +mxm30_155f80(struct bregs *regs)
> +{
> switch (regs->bx) {
> case 0xff00: mxm30_F00(regs); break;
> case 0xff01: mxm30_F01(regs); break;
> default:     handle_155fXX(regs); break;
> }
>
> +}
>
> +static void
> +mxm30_155f(struct bregs *regs)
> +{
> switch (regs->al) {
> case 0x80: mxm30_155f80(regs); break;
> default:   handle_155fXX(regs); break;
> }
>
> +}
>
> +static void
> +mxm30_setup(struct romfile_s *sis_file)
> +{
> /* Load MXM System Information Structure into low memory */
> MXM30SIS = malloc_low(sis_file->size);

This code does not check if sis_file is NULL, which could lead to
memory corruption.

Probably best to use romfile_loadfile(), check for non-null, and then
malloc_low(), manual memcpy(), and free() of the temporary space.  As
it's preferable to avoid calls to free() on malloc_low() allocations -
as that can lead to memory fragmentation of the scarce "low" region.
> if (!MXM30SIS) {
>    warn_noalloc();
>    return;
> }
> int ret = sis_file->copy(sis_file, MXM30SIS, sis_file->size);
> if (ret < 0) {
>    free(MXM30SIS);
>    MXM30SIS = NULL;
>    return;
> }
> VGAHookHandlerType = VH_MXM30;
>
> +}
>
> /****************************************************************
> Entry and setup
>
> ****************************************************************/
> @@ -313,6 +387,7 @@ handle_155f(struct bregs *regs)
>      switch (htype) {
>      case VH_VIA:   via_155f(regs); break;
>      case VH_INTEL: intel_155f(regs); break;
> case VH_MXM30: mxm30_155f(regs); break;
> default:       handle_155fXX(regs); break;
> }
>
> }
> @@ -340,6 +415,8 @@ vgahook_setup(struct pci_device *pci)
>      if (!CONFIG_VGAHOOKS)
>          return;
> struct romfile_s *mxm30sis = romfile_find("mxm-30-sis");
> if (strcmp(CBvendor, "KONTRON") == 0 && strcmp(CBpart, "986LCD-M") == 0)
>        kontron_setup(pci);
>    else if (strcmp(CBvendor, "GETAC") == 0 && strcmp(CBpart, "P470") == 0)
>
> @@ -352,4 +429,6 @@ vgahook_setup(struct pci_device *pci)
>          via_setup(pci);
>      else if (pci->vendor == PCI_VENDOR_ID_INTEL)
>          intel_setup(pci);
> else if (mxm30sis != NULL)
>    mxm30_setup(mxm30sis);

I think it would be preferable if mxm_setup() was passed the pci
device for uniformity, and have mxm_setup() call romfile_xxx() itself.

-Kevin
> }
>
> 2.44.0
>
> SeaBIOS mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]