[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Mon, 19 Mar 2018 17:49:09 +0000 Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote: >On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote: >> This patch extends hvmloader_acpi_build_tables() with code which >> detects if MMCONFIG is available -- i.e. initialized and enabled >> (+we're running on Q35), obtains its base address and size and asks >> libacpi to build MCFG table for it via setting the flag >> ACPI_HAS_MCFG in a manner similar to other optional ACPI tables >> building. >> >> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx> >> --- >> tools/firmware/hvmloader/util.c | 70 >> +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 70 >> insertions(+) >> >> diff --git a/tools/firmware/hvmloader/util.c >> b/tools/firmware/hvmloader/util.c index d8db9e3c8e..c6fc81d52a 100644 >> --- a/tools/firmware/hvmloader/util.c >> +++ b/tools/firmware/hvmloader/util.c >> @@ -782,6 +782,69 @@ int get_pc_machine_type(void) >> return machine_type; >> } >> >> +#define PCIEXBAR_ADDR_MASK_64MB (~((1ULL << 26) - 1)) >> +#define PCIEXBAR_ADDR_MASK_128MB (~((1ULL << 27) - 1)) >> +#define PCIEXBAR_ADDR_MASK_256MB (~((1ULL << 28) - 1)) >> +#define PCIEXBAR_LENGTH_BITS(reg) (((reg) >> 1) & 3) >> +#define PCIEXBAREN 1 > >PCIEXBAR_ENABLE maybe? PCIEXBAREN is just an official name of this bit from the Intel datasheet. :) OK, will rename it to PCIEXBAR_ENABLE. >> + >> +static uint64_t mmconfig_get_base(void) >> +{ >> + uint64_t base; >> + uint32_t reg = pci_readl(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR); >> + >> + base = reg | (uint64_t) pci_readl(PCI_MCH_DEVFN, >> PCI_MCH_PCIEXBAR+4) << 32; > >Please add parentheses in the above expression. Agree, parentheses will make the op priority clearer. >> + >> + switch (PCIEXBAR_LENGTH_BITS(reg)) >> + { >> + case 0: >> + base &= PCIEXBAR_ADDR_MASK_256MB; >> + break; >> + case 1: >> + base &= PCIEXBAR_ADDR_MASK_128MB; >> + break; >> + case 2: >> + base &= PCIEXBAR_ADDR_MASK_64MB; >> + break; > >Missing newlines, plus this looks like it wants to use the defines >introduced in patch 7 (PCIEXBAR_{64,128,256}_BUSES). Also any reason >this patch and patch 7 cannot be put sequentially? I think all these #defines should find a way to pci_regs.h, it seems like an appropriate place for them. Regarding the order of hvmloader patches -- will verify this for the next version. >They are very related, and in fact I'm not sure why we need to write >this info to the device in patch 7 and then fetch it from the device >here. Isn't there an easier way to pass this information? At the end >this is all in hvmloader. Well, the hvmloader_acpi_build_tables() function mostly does device probing (using I/O instruction) and xenstore reads to collect system information in order to discover which ACPI_HAS_* flags it should pass to acpi_build_tables(), but using global variables to pass this kind of information for MMCONFIG will be OK too I think. >> + case 3: > >default: There is '& 3' for the switch argument, but ok I guess, it's clearer with 'default'. >> + BUG(); /* a reserved value encountered */ >> + } >> + >> + return base; >> +} >> + >> +static uint32_t mmconfig_get_size(void) > >unsigned int or size_t? Using types which are common to the existing code. size_t have almost zero use in hvmloader. unsigned int instead of uint32_t... well, the uint32_t still used more often as a type name anyway, but I have no objections to either choice. >> +{ >> + uint32_t reg = pci_readl(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR); >> + >> + switch (PCIEXBAR_LENGTH_BITS(reg)) >> + { >> + case 0: return MB(256); >> + case 1: return MB(128); >> + case 2: return MB(64); >> + case 3: >> + BUG(); /* a reserved value encountered */ > >Same comments as above about the labels and the case 3 label. >> + } >> + >> + return 0; >> +} >> + >> +static uint32_t mmconfig_is_enabled(void) >> +{ >> + return pci_readl(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR) & PCIEXBAREN; >> +} >> + >> +static int is_mmconfig_used(void) > >bool OK >> +{ >> + if (get_pc_machine_type() == MACHINE_TYPE_Q35) >> + { >> + if (mmconfig_is_enabled() && mmconfig_get_base()) > >Coding style. > >Also you can join the conditions: > >if ( get_pc_machine_type() == MACHINE_TYPE_Q35 && >mmconfig_is_enabled() && > mmconfig_get_base() ) > return true; > >Looking at this, is it actually a valid state to have >mmconfig_is_enabled() == true and mmconfig_get_base() == 0? Yes, in theory we can have either PCIEXBAREN=0 and a valid PCIEXBAR base, or vice versa. Of course normally we should not encounter a situation where base=0 and PCIEXBAREN=1, just covering here possible cases which the register format allows. Regarding check merging -- ok, sure. Short-circuit evaluation should guaranty that these registers are not touched on a different machine. >> + return 1; >> + } >> + >> + return 0; >> +} >> + >> static void validate_hvm_info(struct hvm_info_table *t) >> { >> uint8_t *ptr = (uint8_t *)t; >> @@ -993,6 +1056,13 @@ void hvmloader_acpi_build_tables(struct >> acpi_config *config, config->pci_hi_len = pci_hi_mem_end - >> pci_hi_mem_start; } >> >> + if ( is_mmconfig_used() ) >> + { >> + config->table_flags |= ACPI_HAS_MCFG; >> + config->mmconfig_addr = mmconfig_get_base(); >> + config->mmconfig_len = mmconfig_get_size(); >> + } >> + _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |