[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: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Mon, 14 Mar 2022 09:58:29 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=6S3Ekb9PqgBkjODl0iEcXIH39Gu1GjWx+vf/VFmcyFA=; b=GxSg+rdCC+SlpX+gp76hussg6aynHwXbh37YJOA+Wpro4XqCfUvR19MOp6Up1MsINrK7msm0uRU3Pkpo0vTxi5zTDR/mwNNU2lg23cWRpZEjY2MDhBxfSa1GDFWnnM8RXxs7DtE0ExPzkh8zVN/cbDGz3dh4tBlwMKTfWJMAfF/u+ThGG0dQZOpls/0dCi3DGVAoGa9YktQRwnv3hFvWdCrA7TFNDJrKoKg3ZJ6pwCAqAPQQ13H2FD2X2e/HH6ZhvQlyHmiSx5lYPkPRGFQygrgU1evVhH9WWX5AI5WCHCEwbZNKxbaEnoo88WFCH2mvy++jg67Lke1fYf5wyq8BjA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NBlLRDiESJxKY9aaDkzSqojQ/K9eD4OYx4lgvJch4AosN9zPfmPc+bacT1B7f0tdXWSJwtU9qD4Mqitu2nErY9Deqba1DY8pTPX0Y3aKTVN6pyH4GuSy53PbUe5nP2mwufr6EqYHb5zCXVAy2+rSJeCMRK0urCcaXF+fuNlVL4ygZUl4MMiAHEUtfjhmPUiznO0Zib5ljemKuNlOJglLv6Y3qpx4Bcjselo5FkUsse0TmTuoEGNykfrYjdmyYVIMIwcVsvbTOPaSiwbzHdJojO3dHUc7pXUzHIzNZasui130jgEGvzdwvuCgSLb8+BS5K24bNK8tMGjqYTsWt+6cVw==
  • Authentication-results: esa6.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 14 Mar 2022 08:58:42 +0000
  • Ironport-data: A9a23:diPaSK6Q8z2SdqeeUDVYRQxRtDvHchMFZxGqfqrLsTDasY5as4F+v jMbCGuPO/iLYWL1e9l0Po3j8U8CsJCAmtBrSQJvqyBhHi5G8cbLO4+Ufxz6V8+wwmwvb67FA +E2MISowBUcFyeEzvuVGuG96yE6j8lkf5KkYAL+EnkZqTRMFWFw0XqPp8Zj2tQy2YPgX1vU0 T/Pi5a31GGNimYc3l08s8pvmDs31BglkGpF1rCWTakjUG72zxH5PrpGTU2CByKQrr1vNvy7X 47+IISRpQs1yfuP5uSNyd4XemVSKlLb0JPnZnB+A8BOiTAazsA+PzpS2FPxpi67hh3Q9+2dx umhurSVGToJN7+UpN8cWjsCSjw5JbFoxLLudC3XXcy7lyUqclPpyvRqSko3IZcZ6qB8BmQmG f4wcW5XKErZ3qTvnez9GrIEascLdaEHOKsFvX5t13fBBOsOSpHfWaTao9Rf2V/cg+gQQ66BN pdENFKDajzgUjlwO01LD6gBp+COqiDATGMb93uK8P9fD2/7k1UqjemF3MDuUsOObdVYmACfv G2u13T0BFQWOcKSzRKB82mwnanfkCXjQoUQGbaksPlwjzW7xGYeFRkXXluTuuSihwi1XNc3F qAP0nNw9+5orhXtF4SjGU3jyJKZgvICc/8ALs0z+lmx8avdwVzAGy8cUhB9ZfVz4afaWgcW/ lOOmtroAxlmv7uUVW+R+9+okN+iBcQGBTRcPHFZFGPp9/Gm+dhu1UyXEr6PBYbv1rXI9SfML ydmRcTUr5EaloY12qqy5jgraBr898GSHmbZCug6N19JDz+Vhqb4PeRECnCBtJ6sybp1qXHb4 BA5dzC2trxmMH10vHXlrB8xNL+o/e2ZFzbXnERiGZIsnxz0pSL8JNEPumkhex4xWirhRdMOS BWP0T69GbcJZCf6BUOJS9/Z5zsWIVjISo2+C6G8gitmaZltbg6XlByClmbLt10BZHMEyPllU b/CKJ7EJS9DVcxPkWrnL89AgORD7n1vmgvuqWXTkk3PPUy2PyXOF9/o8TKmM4gE0U9ziF6Mo ogFaJfSlUk3vS+XSnC/zLP/5GsidBATLZv3t9ZWZqiEJA9nE3smEPjf3fUqfIkNokifvr6gE q2VMqOA9GfCuA==
  • Ironport-hdrordr: A9a23:1E+o4qGCP0gV2QShpLqFCpHXdLJyesId70hD6qkvc3Nom52j+/ xGws536faVslcssHFJo6HmBEClewKnyXcT2/htAV7CZnichILMFu9fBOTZsl/d8kHFh4tgPO JbAtRD4b7LfClHZKTBkXCF+r8bqbHtmsDY5ts2jU0dNT2CA5sQkTuRYTzrdHGeKjM2YabQQ/ Gnl7V6TnebCDwqR/X+IkNAc/nIptXNmp6jSRkaByQ/4A3LqT+z8rb1HzWRwx9bClp0sP0f2F mAtza8yrSosvm9xBOZ/2jP765OkN+k7tdYHsSDhuUcNz2poAe1Y4ZKXaGEoVkO0aqSwWdvtO OJjwYrPsx15X+UVmapoSH10w2l6zoq42+K8y7uvVLT5ejCAB4qActIgoxUNjHD7VA7gd162K VXm0qEqpt+F3r77WvAzumNcysvulu/oHIkn+JWpWdYS5EiZLhYqpFa1F9JEa0HADnx5OkcYa VT5fnnlbdrmG6hHjDkVjEF+q3uYp1zJGbKfqE6gL3a79AM90oJjXfxx6Qk7wM9HdwGOtx5Dt //Q9dVfYF1P78rhJ1GdZU8qOuMexrwqEH3QSuvyWqOLtBzB5uKke+y3IkI
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Mon, Mar 14, 2022 at 09:25:07AM +0100, Jan Beulich wrote:
> 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?

Indeed, s/now/not/ is what I 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?

I considered the switch to use APIC_TIMER_MODE_ONESHOT one comment,
while the switch to set lvtt_value only once another one.

I'm fine if you want to leave the layout as-is while using
APIC_TIMER_MODE_ONESHOT.

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.



 


Rackspace

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