RE: Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 24 June 2020 16:32
> To: Paul Durrant <paul@xxxxxxx>
> Cc: Grzegorz Uriasz <gorbak25@xxxxxxxxx>; Wei Liu <wl@xxxxxxx>; 
> jakub@xxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; marmarek@xxxxxxxxxxxxxxxxxxxxxx; 
> j.nowak26@xxxxxxxxxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; contact@xxxxxxxxxxxx; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>
> Subject: Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR 
> width
> 
> On 22.06.2020 10:49, Jan Beulich wrote:
> > On 20.06.2020 00:00, Grzegorz Uriasz wrote:
> >> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct 
> >> acpi_table_header *table)
> >>     */
> >>    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 < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER)
> >> +        printk(KERN_WARNING PREFIX "PM-Timer is too short\n");
> >
> > Indentation is screwed up here.
> >
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void)
> >>  static s64 __init init_pmtimer(struct platform_timesource *pts)
> >>  {
> >>      u64 start;
> >> -    u32 count, target, mask = 0xffffff;
> >> +    u32 count, target, mask;
> >>
> >> -    if ( !pmtmr_ioport || !pmtmr_width )
> >> +    if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) )
> >>          return 0;
> >>
> >> -    if ( pmtmr_width == 32 )
> >> -    {
> >> -        pts->counter_bits = 32;
> >> -        mask = 0xffffffff;
> >> -    }
> >> +    pts->counter_bits = pmtmr_width;
> >> +    mask = (1ull << pmtmr_width) - 1;
> >
> > "mask" being of "u32" type, the use of 1ull is certainly a little odd
> > here, and one of the reasons why I think this extra "cleanup" would
> > better go in a separate patch.
> >
> > Seeing that besides cosmetic aspects the patch looks okay now, I'd be
> > willing to leave the bigger adjustment mostly as is, albeit I'd
> > prefer the 1ull to go away, by instead switching to e.g.
> >
> >     mask = 0xffffffff >> (32 - pmtmr_width);
> >
> > With the adjustments (which I'd be happy to make while committing,
> > provided you agree)
> > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > Also Cc-ing Paul for a possible release ack (considering this is a
> > backporting candidate).
> 
> Paul?
> 

Looks ok.

Release-acked-by: Paul Durrant <paul@xxxxxxx>

> Thanks, Jan