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

[Alexey G <x1917x@gmail.com>]

On Tue, 29 May 2018 08:46:13 -0600
"Jan Beulich" <JBeulich@suse.com> wrote:

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

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