Re: [Xen-devel] [PATCH] x86/mmcfg: add "force" option for MCFG

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2019 21:25, Igor Druzhinin wrote:
> On 29/08/2019 09:00, Roger Pau Monné wrote:
>>>
>>> I think we need to have this option to at least have a way to workaround
>>> problem (1). Problem (2) could be solved in Dom0 kernel by invoking
>>> xen_mcfg_late() earlier but before the first PCI bus enumertaion which
>>> currently happens somwhere during ACPI scan I think.
>>>
>>> The question is what the defult value for this option should be?
>>
>> Have you tested this against a variety of hardware?
> Not yet, I presume it's possible in theory that there is such a box in
> the wild that will misbehave if we attempt to access MCFG earlier it'd
> be discovered through ACPI. Other than that I don't see a reason why it
> would cause any problem. But you're right - we need to run some tests
> with this option set to true on our fleet before deciding.
> 
>> Have you found any box that has a wrong MMCFG area in the MCFG static
>> table? (ie: one that doesn't match the motherboard resource
>> reservation in the dynamic ACPI tables?)
> 
> I think it's possible that size of the table reported from ACPI is
> smaller which would be a problem if we access out-of-bounds preliminary
> - hence the ability to fall back. But I'm not aware of any hardware like
> that.

The reason why Linux had put these two checks (E820 and ACPI) in place
(and we cloned them) was that in the old days (at least until around
the first x86-64 systems appearing) there were many such systems (iirc
more than there were well behaved ones). The bad situation with BIOSes
was also why the various chipset probing methods had been introduced.

Anyway - the main risk with using MCFG without knowing the range is
suitably reserved is that other things may live in that range (e.g.
BARs may have been allocated there). Therefore I don't think we can
reasonably default this option to "true", irrespective whether
perhaps _all_ systems in your testing fleet are well behaved in this
regard.

>>> --- a/xen/arch/x86/e820.c
>>> +++ b/xen/arch/x86/e820.c
>>> @@ -37,6 +37,26 @@ struct e820map e820;
>>>  struct e820map __initdata e820_raw;
>>>  
>>>  /*
>>> + * This function checks if any part of the range <start,end> is mapped
>>> + * with type.
>>> + */
>>> +int __init e820_any_mapped(u64 start, u64 end, unsigned type)
>>
>> Please use uint64_t and unsigned int. Also it looks like this
>> function wants to return a bool instead of an int.
>>
> 
> The problem here is that I want this function be similar to the one
> below (e820_all_mapped) which is copied from Linux as well as the rest
> of the file. At the same time I don't want to introduce code churn
> fixing unrelated style issues. Perhaps it's better to keep style of this
> function as is? Or do you think it's still better to unify the style
> across both of them (perhaps in a separate commit)?

Since we're trying to gradually switch to uint<N>_t, I think new code
should by default use the intended types. Exceptions would be imports
of entire files from Linux. If you followed up your change with one
converting the other function(s) to the intended types as well, that'd
be much appreciated, but is imo not a requirement.

Jan

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