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

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

On Tue, Mar 20, 2018 at 07:20:53AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 17:49:09 +0000
> Roger Pau Monné <roger.pau@xxxxxxxxxx> 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@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?
> 
> PCIEXBAREN is just an official name of this bit from the
> Intel datasheet. :) OK, will rename it to PCIEXBAR_ENABLE.

Oh, if that's the name on the spec then leave it as-is. It's always
best to be able to search directly on the spec.

> >> +
> >> +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.

Hm, pci_regs.h seems to contain the generic PCI registers. Those
should maybe live in a q35.h header, since it's very device specific
AFAICT.

> 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.

It was just a suggestion, it seems kind of cumbersome to write
something to a register and then fetch it afterwards, when it's all
done in the same binary.

> >> +    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.

If it's available I would rather use it.

> >> +{
> >> +    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.

But those registers are set by hvmloader, and I don't think hvmloader
will ever set PCIEXBAREN == 1 and PCIEXBAR base == 0?

> Regarding check merging -- ok, sure. Short-circuit evaluation should
> guaranty that these registers are not touched on a different
> machine.

Yes, if you first check for the chipset type.

Thanks, Roger.

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