Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.03.18 at 19:33, <x1917x@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel