|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 06/12] hvmloader: add basic Q35 support
On Mon, 19 Mar 2018 15:30:14 +0000
Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |