Re: [RFC PATCH 06/12] hvmloader: add basic Q35 support

[Alexey G <x1917x@gmail.com>]

On Mon, 19 Mar 2018 15:30:14 +0000
Roger Pau Monné <roger.pau@citrix.com> wrote:

>On Tue, Mar 13, 2018 at 04:33:51AM +1000, Alexey Gerasimenko wrote:
>> This patch does following:
>> 
>> 1. Move PCI-device specific initialization out of pci_setup function
>> to the newly created class_specific_pci_device_setup function to
>> simplify code.
>> 
>> 2. PCI-device specific initialization extended with LPC controller
>> initialization
>> 
>> 3. Initialize 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>
>> ---
>>  tools/firmware/hvmloader/config.h |   1 +
>>  tools/firmware/hvmloader/pci.c    | 162
>> ++++++++++++++++++++++++-------------- 2 files changed, 104
>> insertions(+), 59 deletions(-)
>> 
>> diff --git a/tools/firmware/hvmloader/config.h
>> b/tools/firmware/hvmloader/config.h index 6e00413f2e..6fde6b7b60
>> 100644 --- a/tools/firmware/hvmloader/config.h
>> +++ b/tools/firmware/hvmloader/config.h
>> @@ -52,6 +52,7 @@ extern uint8_t ioapic_version;
>>  
>>  #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 */
>>  
>>  /* MMIO hole: Hardcoded defaults, which can be dynamically
>> expanded. */ #define PCI_MEM_END         0xfc000000
>> diff --git a/tools/firmware/hvmloader/pci.c
>> b/tools/firmware/hvmloader/pci.c index 0b708bf578..033bd20992 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -35,6 +35,7 @@ unsigned long pci_mem_end = PCI_MEM_END;
>>  uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>>  
>>  enum virtual_vga virtual_vga = VGA_none;
>> +uint32_t vga_devfn = 256;  
>
>uint8_t should be enough to store a devfn. Also this should be static
>maybe?

Yep, forgot 'static'. Changing uint32_t to uint8_t here will require
to change the
'    if ( vga_devfn != 256 )' condition as well -- it's a bit out of the
patch scope, probably a separate tiny patch would be better.

>>  unsigned long igd_opregion_pgbase = 0;
>>  
>>  /* Check if the specified range conflicts with any reserved device
>> memory. */ @@ -76,14 +77,93 @@ 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)  
>
>Alignment.

Will correct.

>> +
>> +static void class_specific_pci_device_setup(uint16_t vendor_id,
>> +                                            uint16_t device_id,
>> +                                            uint8_t bus, uint8_t
>> devfn) +{
>> +    uint16_t class;
>> +
>> +    class = pci_readw(devfn, PCI_CLASS_DEVICE);
>> +
>> +    switch ( class )  
>
>switch ( pci_readw(devfn, PCI_CLASS_DEVICE) ) ?
>
>I don't see class being used elsewhere.

>Also why is vendor_id/device_id provided by the caller but not class?
>It seems kind of pointless.

'class' is not used by pci_setup(), thus moved to
class_specific_pci_device_setup().

pci_readw(devfn, PCI_CLASS_DEVICE) inside the switch condition to drop
the variable -- sure, agree.

Passing vendor_id/device_id pair via function args allows to avoid
reading the vendor_id/device_id from PCI conf twice -- a bit less
garbage in the polluted PCI setup debuglog. It's not a big problem
really, so this can be changed to passing only BDF to
class_specific_pci_device_setup().

>Why not fetch vendor/device from the function itself and move the
>(vendor_id == 0xffff) && (device_id == 0xffff) check inside the
>function?

Hmm, this is a part of the PCI bus enumeration, not PCI device setup.

>Also in this case I think it would be better to have a non-functional
>patch that introduces class_specific_pci_device_setup and a second
>patch that adds support for ICH9.
>
>Having code movement and new code in the same patch makes it harder to
>very what you are actually moving vs introducing.

Agree, will split this actions to separate patches for the next version.

>> +    {
>> +    case 0x0300:  
>
>All this values need to be defines documented somewhere.

Agree... although it was not me who introduced all these hardcoded PCI
class values. :) I'll change these numbers into newly added pci_regs.h
#defines in the non-functional patch.

>> +        /* If emulated VGA is found, preserve it as primary VGA. */
>> +        if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
>> +        {
>> +            vga_devfn = devfn;
>> +            virtual_vga = VGA_std;
>> +        }
>> +        else if ( (vendor_id == 0x1013) && (device_id == 0xb8) )
>> +        {
>> +            vga_devfn = devfn;
>> +            virtual_vga = VGA_cirrus;
>> +        }
>> +        else if ( virtual_vga == VGA_none )
>> +        {
>> +            vga_devfn = devfn;
>> +            virtual_vga = VGA_pt;
>> +            if ( vendor_id == 0x8086 )
>> +            {
>> +                igd_opregion_pgbase =
>> mem_hole_alloc(IGD_OPREGION_PAGES);
>> +                /*
>> +                 * Write the the OpRegion offset to give the
>> opregion
>> +                 * address to the device model. The device model
>> will trap
>> +                 * and map the OpRegion at the give address.
>> +                 */
>> +                pci_writel(vga_devfn, PCI_INTEL_OPREGION,
>> +                           igd_opregion_pgbase << PAGE_SHIFT);
>> +            }
>> +        }
>> +        break;
>> +
>> +    case 0x0680:
>> +        /* PIIX4 ACPI PM. Special device with special PCI config
>> space. */
>> +        ASSERT((vendor_id == 0x8086) && (device_id == 0x7113));
>> +        pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
>> +        pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
>> +        pci_writew(devfn, 0x22, 0x0000);
>> +        pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
>> +        pci_writew(devfn, 0x3d, 0x0001);
>> +        pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
>> +        pci_writeb(devfn, 0x80, 0x01); /* enable PM io space */
>> +        break;
>> +
>> +    case 0x0601:
>> +        /* LPC bridge */
>> +        if (vendor_id == 0x8086 && device_id == 0x2918)
>> +        {
>> +            pci_writeb(devfn, 0x3c, 0x09); /* Hardcoded IRQ9 */
>> +            pci_writeb(devfn, 0x3d, 0x01);
>> +            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);
>> +        }
>> +        break;
>> +
>> +    case 0x0101:
>> +        if ( vendor_id == 0x8086 )
>> +        {
>> +            /* Intel ICHs since PIIX3: enable IDE legacy mode. */
>> +            pci_writew(devfn, 0x40, 0x8000); /* enable IDE0 */
>> +            pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
>> +        }
>> +        break;
>> +    }
>> +}
>> +
>>  void pci_setup(void)
>>  {
>>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>>      uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
>> -    uint32_t vga_devfn = 256;
>> -    uint16_t class, vendor_id, device_id;
>> +    uint16_t vendor_id, device_id;
>>      unsigned int bar, pin, link, isa_irq;
>> +    int is_running_on_q35 = 0;  
>
>bool is_running_on_q35 = (get_pc_machine_type() == MACHINE_TYPE_Q35);

OK

>>  
>>      /* Resources assignable to PCI devices via BARs. */
>>      struct resource {
>> @@ -130,13 +210,28 @@ void pci_setup(void)
>>      if ( s )
>>          mmio_hole_size = strtoll(s, NULL, 0);
>>  
>> +    /* check if we are on Q35 and set the flag if it is the case */
>> +    is_running_on_q35 = get_pc_machine_type() == MACHINE_TYPE_Q35;
>> +
>>      /* Program PCI-ISA bridge with appropriate link routes. */
>>      isa_irq = 0;
>>      for ( link = 0; link < 4; link++ )
>>      {
>>          do { isa_irq = (isa_irq + 1) & 15;
>>          } while ( !(PCI_ISA_IRQ_MASK & (1U << isa_irq)) );
>> -        pci_writeb(PCI_ISA_DEVFN, 0x60 + link, isa_irq);
>> +
>> +        if (is_running_on_q35)  
>
>Coding style.

OK

>> +        {
>> +            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
>> +
>> +            /* PIRQE..PIRQH are unused */
>> +            pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link, 0x80);  
>
>According to the spec 0x80 is the default value for this registers, do
>you really need to write it?
>
>Is maybe QEMU not correctly setting the default value?

Won't agree here. We're initializing PIRQ[n] routing in this
fragment, it's better not to rely on any values but simply initialize
all PIRQ[n]_ROUT registers, this makes it explicit.

Even if it is unnecessary due to defaults it's more obvious to set
these registers to our own values than to force a reader to either look
up their emulation in QEMU code or read the ICH9 pdf to confirm
assumptions.

>> +        }
>> +        else
>> +        {
>> +            pci_writeb(PCI_ISA_DEVFN, 0x60 + link, isa_irq);  
>
>Is all this magic described somewhere that you can reference?

It's setting up PCI interrupt routing for PIC mode. All this 
PIRQ[n]_ROUT stuff basically needed for legacy compatibility only,
normally we deal with APIC mode (+MSIs).

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel