>>> On 12.03.18 at 19:33, <x19...@gmail.com> 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