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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel