Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Tue, 29 May 2018 08:46:13 -0600 "Jan Beulich" wrote: On 12.03.18 at 19:33, wrote: >> --- 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)) > >I don't see the value of these constants, the more that they're generic >64/128/256 Mb masks rather than being PCIEXBAR specific. They also >have no business living in pci_regs.h imo, including any of ... > >> +#define PCIEXBAR_LENGTH_BITS(reg) (((reg) >> 1) & 3) >> +#define PCIEXBAREN 1 > >... these: Only generic fields should be described there. If you want to >collect Q35 definitions in a central place, add q35.h. But if you do, >please properly prefix all of them such that there won't be any risk >collisions with possible future additions. OK, sure. >> +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; >> + >> +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; >> +case 3: >> +BUG(); /* a reserved value encountered */ >> +} > >Instead of this switch, why can't you ... > >> +return base; > >return base & ~(mmconfig_get_size() - 1); > >here, eliminating (afaics) the need for the constants above? I remember some MMCONFIG implementations using base alignment smaller than a possible MMCONFIG size, the code style was probably influenced by that fact. But as we deal with Q35 only, the mmconfig_get_size() for the base address mask is absolutely valid (and shorter). In this case it will be nicer, agree. And we still have an assert for the unimplemented value (3) via the mmconfig_get_size() call to catch errors like an emulator returning 0xFF's on register reads. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
>>> On 12.03.18 at 19:33, wrote: > --- 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)) I don't see the value of these constants, the more that they're generic 64/128/256 Mb masks rather than being PCIEXBAR specific. They also have no business living in pci_regs.h imo, including any of ... > +#define PCIEXBAR_LENGTH_BITS(reg) (((reg) >> 1) & 3) > +#define PCIEXBAREN 1 ... these: Only generic fields should be described there. If you want to collect Q35 definitions in a central place, add q35.h. But if you do, please properly prefix all of them such that there won't be any risk collisions with possible future additions. > +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; > + > +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; > +case 3: > +BUG(); /* a reserved value encountered */ > +} Instead of this switch, why can't you ... > +return base; return base & ~(mmconfig_get_size() - 1); here, eliminating (afaics) the need for the constants above? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
>>> On 20.03.18 at 21:53,wrote: > On Tue, 20 Mar 2018 03:36:57 -0600 > "Jan Beulich" wrote: > On 19.03.18 at 22:20, wrote: >>> On Mon, 19 Mar 2018 17:49:09 + >>> Roger Pau Monné wrote: On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote: > +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. >> >>I don't think device specific defines belong into pci_regs.h. > > Will gather all these #defines and macros in the new pci_regs_q35.h > file. It should not harm to include it from pci_regs.h I think, in > order to include pci_regs.h only in *.c. Well, no - no unnecessary dependencies please. If only a single file needs these definitions, only that file should include the respective header (if one is warranted in the first place). Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Tue, 20 Mar 2018 03:36:57 -0600 "Jan Beulich"wrote: On 19.03.18 at 22:20, wrote: >> On Mon, 19 Mar 2018 17:49:09 + >> Roger Pau Monné wrote: >>>On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote: --- 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. > >I think using names from the datasheet (where they exist) is >preferable in cases like this one. Leaving it intact then. +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. > >I don't think device specific defines belong into pci_regs.h. Will gather all these #defines and macros in the new pci_regs_q35.h file. It should not harm to include it from pci_regs.h I think, in order to include pci_regs.h only in *.c. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
>>> On 19.03.18 at 22:20,wrote: > On Mon, 19 Mar 2018 17:49:09 + > Roger Pau Monné wrote: >>On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote: >>> --- 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. I think using names from the datasheet (where they exist) is preferable in cases like this one. >>> +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. I don't think device specific defines belong into pci_regs.h. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Tue, Mar 20, 2018 at 07:20:53AM +1000, Alexey G wrote: > On Mon, 19 Mar 2018 17:49:09 + > Roger Pau Monné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 > >> --- > >> 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. Oh, if that's the name on the spec then leave it as-is. It's always best to be able to search directly on the spec. > >> + > >> +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. Hm, pci_regs.h seems to contain the generic PCI registers. Those should maybe live in a q35.h header, since it's very device specific AFAICT. > 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. It was just a suggestion, it seems kind of cumbersome to write something to a register and then fetch it afterwards, when it's all done in the same binary. > >> +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. If it's available I would rather use it. > >> +{ > >> +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. But those registers are set by hvmloader, and I don't think hvmloader will ever set PCIEXBAREN == 1 and PCIEXBAR base == 0? > Regarding check merging
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Mon, 19 Mar 2018 17:49:09 + Roger Pau Monné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 >> --- >> 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; >> +
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
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> --- > 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? > + > +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. > + > +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? 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. > +case 3: default: > +BUG(); /* a reserved value encountered */ > +} > + > +return base; > +} > + > +static uint32_t mmconfig_get_size(void) unsigned int or size_t? > +{ > +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 > +{ > +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? > +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(); > +} > + > s = xenstore_read("platform/generation-id", "0:0"); > if ( s ) > { > -- > 2.11.0 > > > ___ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table
On Tue, 13 Mar 2018 04:33:56 +1000 Alexey Gerasimenkowrote: >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 >--- > tools/firmware/hvmloader/util.c | 70 > + 1 file changed, 70 > insertions(+) Looks like I missed the patch for reserving MMCONFIG area in E820 map, it is required for Linux guests (otherwise MMCONFIG info will be rejected by linux kernel). Windows guests allow to use MMCONFIG without a corresponding E820 entry. Following lines need to be added to /hvmloader/e820.c: +/* mark MMCONFIG area */ +if ( is_mmconfig_used() ) +{ +e820[nr].addr = mmconfig_get_base(); +e820[nr].size = mmconfig_get_size(); +e820[nr].type = E820_RESERVED; +nr++; +} The corresponding patch-file is attached, will include it in v2 patches. hvmloader-mark-MMCONFIG-in-E820-map.patch Description: Binary data ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel