Re: [PATCH 07/17] hvmloader: add basic Q35 support

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 13, 2026 at 04:35:02PM +0000, Thierry Escande wrote:
> The current hvmloader implementation assumes a fixed PCI-to-ISA bridge
> at 00:01.0 (PIIX3). This patch introduces support for the ICH9 LPC
> bridge used in Q35 machine types, which resides at 00:1f.0.
> 
> It also initializes PIRQA...{PIRQD, PIRQH} routing accordingly to the
> emulated south bridge (either located on PCI_ISA_DEVFN or
> PCI_ICH9_LPC_DEVFN).
> 
> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  tools/firmware/hvmloader/config.h   |  1 +
>  tools/firmware/hvmloader/pci.c      | 34 ++++++++++++++++++++++++-----
>  tools/firmware/hvmloader/pci_regs.h |  1 +
>  3 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h
> index c159db30ee..baaed91c7f 100644
> --- a/tools/firmware/hvmloader/config.h
> +++ b/tools/firmware/hvmloader/config.h
> @@ -54,6 +54,7 @@ extern uint32_t *cpu_to_apicid;
>  
>  #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
>  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected */
> +#define PCI_ICH9_LPC_DEVFN  0xf8    /* dev 31, fn 0 */

We should introduce a macro to generate BDFs more easily in the
hvmloader context.  Having them as plain numbers is not very friendly.

>  
>  #define ACPI_TIS_HDR_ADDRESS 0xFED40F00UL
>  
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index a76d051bdf..91c7fd2171 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -84,6 +84,10 @@ static int find_next_rmrr(uint32_t base)
>      return next_rmrr;
>  }
>  
> +#define SCI_EN_IOPORT  (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + 0x30)
> +#define GBL_SMI_EN     (1 << 0)
> +#define APMC_EN        (1 << 5)
> +
>  static void class_specific_pci_device_setup(uint16_t vendor_id,
>                                              uint16_t device_id,
>                                              uint16_t class,
> @@ -140,6 +144,17 @@ static void class_specific_pci_device_setup(uint16_t vendor_id,
>              pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
>          }
>          break;
> +    case PCI_CLASS_BRIDGE_ISA:
> +        /* LPC bridge */
> +        if ( vendor_id == 0x8086 && device_id == 0x2918 )

If you don't add parentheses around the equal comparisons please also
removing the existing ones when moving the code.  Also device ID would
batter be a constant with a proper name.

> +        {
> +            pci_writeb(devfn, 0x3c, 0x09); /* Hardcoded IRQ9 */
> +            pci_writeb(devfn, 0x3d, 0x01);

0x3c and 0x3d are PCI_INTERRUPT_LINE and PCI_INTERRUPT_PIN
respectively, let's please try to use the named constants when
available (or when they can be introduced).

> +            pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
> +            pci_writeb(devfn, 0x44, 0x80); /* enable PM io space */
> +            outl(SCI_EN_IOPORT, inl(SCI_EN_IOPORT) | GBL_SMI_EN | APMC_EN);

Most of the above looks like black magic.  It's not like the context
of this function is great, most of the existing stuff is poorly
documented.  Can we get a bit more comments about what's this supposed
to do, and maybe a reference to the Intel specification that lists
where those PCI config space registers are coming from?

> +        }
> +        break;
>      }
>  }
>  
> @@ -152,6 +167,7 @@ void pci_setup(void)
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
>      uint8_t pci_devfn_decode_type[256] = {};
> +    int is_running_on_q35 = (machine_type == MACHINE_TYPE_Q35);

Maybe you can avoid that and instead simply store the BDF of the ISA
device?

unsigned int isa_bdf = (machine_type == MACHINE_TYPE_Q35) ? PCI_ICH9_LPC_DEVFN
                                                          : PCI_ISA_DEVFN;

Or if you really need it to be a boolean, let's make it:

bool is_q35 = ...

It makes no sense for it to be an int, and the naming could be shorter
IMO.

Thanks, Roger.