[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 3/4] x86/APIC: skip unnecessary parts of __setup_APIC_LVTT()


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 14 Mar 2022 09:25:07 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=I8HsELeuSl5qSC96BUnbXHlryahWZFIYkzCkhT54+4s=; b=MXUnNlXBdgZzOmtOGKKKW/dH8toVPUJ2UQA6QmFlCqLcF4E5/s2wq7xHB1HcTNKsoIv8WjXDqw0PYF1ql1YZqEZcSelFQxHgdZXoehysxZKqYPH21UNzQKUEAR3f4BMkr6x4wgYxbQRa6p2uHIdIGusQcsdb7njqXww3n7x1s8skP03NvlYGyHwB2q3n1ZgtQRPQMyGG9XAE/fEnXu01crEFmpYDdJj9ZYm7bsyXQSZb9oGAZzR+fgoct1o+39H9l62tbbVmc12l54aXgqNn4B33VERcuYUOrMM5PJ5J8w7OnptqJwA7huvdhHabSSB3hQVjrTXI0H/sopbrXLnWFQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XUZP/Wgx/A+3QVNe6QQyhg18t2ej4QHLdKAJOHZUvsmBL9ge02/OT0ACC3f5mhkO09pHGRZmshRQn8SEza5p77F1oi+JwgAsh3HOAlR3R+6+ozc0Ze3Au2mNANmhD1VOFAh+/D/+J8VHZqAi0I0Qeqdw7OsoPpOAS9ymWPPm3/wkODxmIliV3dIByk4ZdjqUF3EzINAHWMV/PU58USuVfDNlKMOkALtfVq0yr7twN26EPEXeWHFNWawCcDmrTTrxUt/6pBdSfNi9Qc0L42jmxM4YphjrOeB8OClbgzDR9f/VpxgKHq3yCsHLRE8859OK/BeyuC4ImZGGmUBF5/WGEw==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 14 Mar 2022 08:25:19 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 11.03.2022 15:05, Roger Pau Monné wrote:
> On Mon, Feb 14, 2022 at 10:25:31AM +0100, Jan Beulich wrote:
>> In TDT mode there's no point writing TDCR or TMICT, while outside of
>> that mode there's no need for the MFENCE.
>>
>> No change intended to overall functioning.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I've got some comments below, now that the current proposal is bad,
> but I think we could simplify a bit more.

I'm struggling with the sentence; perhaps s/now/not/ was meant?

>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1059,24 +1059,25 @@ static void __setup_APIC_LVTT(unsigned i
>>  {
>>      unsigned int lvtt_value, tmp_value;
>>  
>> -    /* NB. Xen uses local APIC timer in one-shot mode. */
>> -    lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR;
>> -
>>      if ( tdt_enabled )
>>      {
>> -        lvtt_value &= (~APIC_TIMER_MODE_MASK);
>> -        lvtt_value |= APIC_TIMER_MODE_TSC_DEADLINE;
>> +        lvtt_value = APIC_TIMER_MODE_TSC_DEADLINE | LOCAL_TIMER_VECTOR;
>> +        apic_write(APIC_LVTT, lvtt_value);
>> +
>> +        /*
>> +         * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode,
>> +         * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized.
>> +         * According to Intel, MFENCE can do the serialization here.
>> +         */
>> +        asm volatile( "mfence" : : : "memory" );
>> +
>> +        return;
>>      }
>>  
>> +    /* NB. Xen uses local APIC timer in one-shot mode. */
>> +    lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR;
> 
> While here I wouldn't mind if you replaced the comment(s) here with
> APIC_TIMER_MODE_ONESHOT. I think that's clearer.
> 
> I wouldn't mind if you did something like:
> 
> unsigned int lvtt_value = (tdt_enabled ? APIC_TIMER_MODE_TSC_DEADLINE
>                                        : APIC_TIMER_MODE_ONESHOT) |
>                           LOCAL_TIMER_VECTOR;

I'm happy to switch to using APIC_TIMER_MODE_ONESHOT, but ...

> apic_write(APIC_LVTT, lvtt_value);
> 
> if ( tdt_enabled )
> {
>     MFENCE;
>     return;
> }

... I'd prefer to stick to just a single tdt_enabled conditional.
But then I'm also unclear about your use of "comment(s)" - what is
the (optional?) plural referring to?

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.