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

[Alexey G]

On Tue, 29 May 2018 08:46:13 -0600
"Jan Beulich"  wrote:

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

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

[Jan Beulich]

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

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

[Jan Beulich]

>>> On 20.03.18 at 21:53,  wrote:
> On Tue, 20 Mar 2018 03:36:57 -0600
> "Jan Beulich"  wrote:
> On 19.03.18 at 22:20,  wrote:  
>>> On Mon, 19 Mar 2018 17:49:09 +
>>> Roger Pau Monné  wrote:  
On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote:  
> +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.  
>>
>>I don't think device specific defines belong into pci_regs.h.
> 
> Will gather all these #defines and macros in the new pci_regs_q35.h
> file. It should not harm to include it from pci_regs.h I think, in
> order to include pci_regs.h only in *.c.

Well, no - no unnecessary dependencies please. If only a single
file needs these definitions, only that file should include the
respective header (if one is warranted in the first place).

Jan

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

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

[Alexey G]

On Tue, 20 Mar 2018 03:36:57 -0600
"Jan Beulich"  wrote:

 On 19.03.18 at 22:20,  wrote:  
>> On Mon, 19 Mar 2018 17:49:09 +
>> Roger Pau Monné  wrote:  
>>>On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko 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))
 +#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.  
>
>I think using names from the datasheet (where they exist) is
>preferable in cases like this one.

Leaving it intact then.

 +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.  
>
>I don't think device specific defines belong into pci_regs.h.

Will gather all these #defines and macros in the new pci_regs_q35.h
file. It should not harm to include it from pci_regs.h I think, in
order to include pci_regs.h only in *.c.

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

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

[Jan Beulich]

>>> On 19.03.18 at 22:20,  wrote:
> On Mon, 19 Mar 2018 17:49:09 +
> Roger Pau Monné  wrote:
>>On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko 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))
>>> +#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.

I think using names from the datasheet (where they exist) is
preferable in cases like this one.

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

I don't think device specific defines belong into pci_regs.h.

Jan

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

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

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 07:20:53AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 17:49:09 +
> Roger Pau Monné  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 
> >> ---
> >>  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 

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

[Alexey G]

On Mon, 19 Mar 2018 17:49:09 +
Roger Pau Monné  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 
>> ---
>>  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;
>> +

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

[Roger Pau Monné]

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

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

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

[Alexey G]

On Tue, 13 Mar 2018 04:33:56 +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 
>---
> tools/firmware/hvmloader/util.c | 70
> + 1 file changed, 70
> insertions(+)

Looks like I missed the patch for reserving MMCONFIG area in E820 map,
it is required for Linux guests (otherwise MMCONFIG info will be
rejected by linux kernel). Windows guests allow to use MMCONFIG without
a corresponding E820 entry.

Following lines need to be added to /hvmloader/e820.c:

+/* mark MMCONFIG area */
+if ( is_mmconfig_used() )
+{
+e820[nr].addr = mmconfig_get_base();
+e820[nr].size = mmconfig_get_size();
+e820[nr].type = E820_RESERVED;
+nr++;
+}

The corresponding patch-file is attached, will include it in v2 patches.

hvmloader-mark-MMCONFIG-in-E820-map.patch
Description: Binary data
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel