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

[Alexey G <x1917x@gmail.com>]

On Mon, 19 Mar 2018 17:49:09 +0000
Roger Pau Monné <roger.pau@citrix.com> 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@gmail.com>
>> ---
>>  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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel