[PATCH v4] Add basic MXM 3.0 interrupt support

[Riku Viitanen]

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/

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;
+        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);
+    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);
 }

[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]

[Kevin O'Connor]

On Sun, Apr 14, 2024 at 11:51:33AM -0400, Kevin O'Connor wrote:
> 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.

Ah, I missed that vgahook_setup() checks for NULL.  Out of curiosity,
is it possible to limit this support to a particular hardware vendor
instead of by existence of file?

-Kevin
> 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]

[Riku Viitanen]

> Ah, I missed that vgahook_setup() checks for NULL. Out of curiosity,
> is it possible to limit this support to a particular hardware vendor
> instead of by existence of file?

ATI, AMD, and Nvidia based MXM cards exist. I don't know of a way to
probe it to find out if it's an MXM format card. And even if there
was a way to tell with 100% reliability, the spec allows for the
System Information Structure to be provided by an EEPROM on the
mainboard, connected to the MXM GPU via I²C. In that case, we wouldn't
have to provide these interrupts.

Despite the declining popularity, new MXM cards are still released.
Maybe one day we'd have to add support for Intel Arc cards as well.

Or do you mean CBvendor/CBpart? I fear that list could grow quickly
and would just make it more annoying to port new boards. Coreboot
already has EliteBook 8770w and HP Compaq Elite 8300 USDT (that's a
desktop btw). And Dell Precision ports are in progress.

-- Riku

[Kevin O'Connor]

On Sun, Apr 14, 2024 at 05:39:21PM +0000, Riku Viitanen wrote:
> > Ah, I missed that vgahook_setup() checks for NULL. Out of curiosity,
> > is it possible to limit this support to a particular hardware vendor
> > instead of by existence of file?
>
> ATI, AMD, and Nvidia based MXM cards exist. I don't know of a way to
> probe it to find out if it's an MXM format card. And even if there
> was a way to tell with 100% reliability, the spec allows for the
> System Information Structure to be provided by an EEPROM on the
> mainboard, connected to the MXM GPU via I²C. In that case, we wouldn't
> have to provide these interrupts.
>
> Despite the declining popularity, new MXM cards are still released.
> Maybe one day we'd have to add support for Intel Arc cards as well.
>
> Or do you mean CBvendor/CBpart? I fear that list could grow quickly
> and would just make it more annoying to port new boards. Coreboot
> already has EliteBook 8770w and HP Compaq Elite 8300 USDT (that's a
> desktop btw). And Dell Precision ports are in progress.

Okay, thanks for the explanation.

Maybe unconditionally call mxm30_setup() and that function can exit
early if "mxm-30-sis" doesn't exist?

-Kevin

[Riku Viitanen]

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

Do you mean, run mxm30_setup unconditionally?
And it checks if the file exists:

else if (mxm30_setup(pci))
        /* nothing left to do here */

[Kevin O'Connor]

On Sun, Apr 14, 2024 at 05:56:31PM +0000, Riku Viitanen wrote:
> > I think it would be preferable if mxm_setup() was passed the pci
> > device for uniformity, and have mxm_setup() call romfile_xxx() itself.
>
> Do you mean, run mxm30_setup unconditionally?
> And it checks if the file exists:
> else if (mxm30_setup(pci))
>     /* nothing left to do here */

That's fine - or something like:

int ret = mxm30_setup(pci);
    if (!ret)
        return;

-Kevin