I'm wondering why support for 32 bit acpi pm timers was introduced to xen. Linux doesn't bother and just crops it to 24 bits: https://github.com/torvalds/linux/blob/a5dc8300df75e8b8384b4c82225f1e4a0b4d9b55/drivers/clocksource/acpi_pm.c#L37
Best regards,
Grzegorz Uriasz
On 16/06/2020 16:59, Roger Pau Monné wrote:
> On Tue, Jun 16, 2020 at 03:25:42PM +0200, Jan Beulich wrote:
>> On 16.06.2020 12:32, Roger Pau Monné wrote:
>>> On Tue, Jun 16, 2020 at 10:07:05AM +0200, Jan Beulich wrote:
>>>> On 14.06.2020 16:36, Grzegorz Uriasz wrote:
>>>>> --- a/xen/arch/x86/acpi/boot.c
>>>>> +++ b/xen/arch/x86/acpi/boot.c
>>>>> @@ -480,7 +480,10 @@ static int __init acpi_parse_fadt(struct
>>>>> acpi_table_header *table)
>>>>> if (fadt->xpm_timer_block.space_id ==
>>>>> ACPI_ADR_SPACE_SYSTEM_IO) {
>>>>> pmtmr_ioport = fadt->xpm_timer_block.address;
>>>>> - pmtmr_width = fadt->xpm_timer_block.bit_width;
>>>>> + if (fadt->flags & ACPI_FADT_32BIT_TIMER)
>>>>> + pmtmr_width = 32;
>>>>> + else
>>>>> + pmtmr_width = 24;
>>>> I think disagreement of the two wants logging, and you want to
>>>> default to using the smaller of the two (or even to ignoring the
>>>> timer altogether). Then there wants to be a way to override
>>>> (unless we already have one) our defaulting, in case it's wrong.
>>> TBH, I presume timer_block will always return 32bits, because that's
>>> the size of the register. Then the timer can implement less bits than
>>> the full size of the register, and that's what gets signaled using the
>>> ACPI flags. What we care about here is the number of bits used by the
>>> timer, not the size of the register.
>>>
>>> I think we should only ignore the timer if pm_timer_block.bit_width <
>>> pmtmr_width.
>>>
>>> Printing a (debug) message when those values disagree is fine, but I
>>> bet it's going to trigger always when the implemented timer is only
>>> using 24bits.
>> The 2nd system I tried on would trigger it, so maybe there's no point
>> in logging indeed. How about the below as a basis?
>>
>> Jan
>>
>> --- unstable.orig/xen/arch/x86/acpi/boot.c
>> +++ unstable/xen/arch/x86/acpi/boot.c
>> @@ -480,7 +480,9 @@ static int __init acpi_parse_fadt(struct
>> if (fadt->header.revision >= FADT2_REVISION_ID) {
>> /* FADT rev. 2 */
>> if (fadt->xpm_timer_block.space_id ==
>> - ACPI_ADR_SPACE_SYSTEM_IO) {
>> + ACPI_ADR_SPACE_SYSTEM_IO &&
>> + (fadt->xpm_timer_block.access_width == 0 ||
>> + fadt->xpm_timer_block.access_width == 3)) {
> We should really have defines for those values, or else they seem to
> imply actual access sizes. What about adding
> ACPI_ADDR_ACCESS_{LEGACY,BYTE,WORD,DWORD,QWORD}?
>
> Also the check for the access size seems kind of unrelated to the
> patch itself? (not that I'm opposed to it)
>
>> pmtmr_ioport = fadt->xpm_timer_block.address;
>> pmtmr_width = fadt->xpm_timer_block.bit_width;
>> }
>> @@ -492,8 +494,10 @@ static int __init acpi_parse_fadt(struct
>> */
>> if (!pmtmr_ioport) {
>> pmtmr_ioport = fadt->pm_timer_block;
>> - pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0;
>> + pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0;
>> }
>> + if (pmtmr_width > 24 && !(fadt->flags & ACPI_FADT_32BIT_TIMER))
>> + pmtmr_width = 24;
>> if (pmtmr_ioport)
>> printk(KERN_INFO PREFIX "PM-Timer IO Port: %#x (%u bits)\n",
>> pmtmr_ioport, pmtmr_width);
>> --- unstable.orig/xen/arch/x86/time.c
>> +++ unstable/xen/arch/x86/time.c
>> @@ -465,7 +465,7 @@ static s64 __init init_pmtimer(struct pl
>> u64 start;
>> u32 count, target, mask = 0xffffff;
>>
>> - if ( !pmtmr_ioport || !pmtmr_width )
>> + if ( !pmtmr_ioport )
>> return 0;
>>
>> if ( pmtmr_width == 32 )
>> @@ -473,6 +473,8 @@ static s64 __init init_pmtimer(struct pl
>> pts->counter_bits = 32;
>> mask = 0xffffffff;
>> }
>> + else if ( pmtmr_width != pts->counter_bits )
>> + return 0;
>>
>> count = inl(pmtmr_ioport) & mask;
>> start = rdtsc_ordered();
> The rest LGTM.
>
> Thanks, Roger.
signature.asc
Description: OpenPGP digital signature
