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

Re: [PATCH v3 09/11] x86/vpt: switch interrupt injection model


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Wed, 14 Apr 2021 15:37:36 +0200
  • 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-SenderADCheck; bh=Soq1EBdFHw/yXJPdksV/Nn1vxVrkLWEhIiQuluttx6w=; b=V0kCcxJYdI8Oq5t3jNEDWyfRqK4nMz4SXQXLoxPuhIte0Tb1mGQvWjOCnFk88KpwTH7PuQ0Iz02wrCL+0FNL3q1OXEAA5uBPTpmA9KDyn1VfBmkcPVqpTjLmfsVpnjgAKcyCPeOwgmtw+jF0rCTYo/T/esqHwDTmoycJrCvKOT8FNo7c+ccxF4aKp2gfl2BfzfRRPc43zcjHDGPl3I5ZLiosolE1j16cJs2Or+hBgnmM1seN91+vekZH9/ZE0n8emA5ZW7xEJcLvi+9fXHAZfpH4OjHgnxRv9f0hz1YVZigL52yE/AgMtzTUOdB5j35QQkwVIqh/S/U0vHjjVsQLag==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dAmog8hdcOd9J5KLxGnhInijQpGp/4AyzRWDcOoXoWSrrPEDKgi0+D34/P9p063N4kJp91GzpbmoOLaopJpOCBEAIv8UQeCmfKhIHj6JC75cI2f9m9h5k6hkL4x7KB10o07ndwB3vasoZXDNf2pMgFcDYFr/rbxd5wwgHb+tx7X+GH/45aTKcbtEsxriU36/xHE8BhAOgQCY81MWS3qPj/mnlytr1l+apDqE7iToR+gAaffxlmYqH5cjg67RQnYP8XZfgV+TBxpO4U+bGtMcdNxWvmMyRQGzKmcS1ITpbdF4/JZ5dZKRX/0BlM7nSuLXYvFC0lbbRasZV0r13E4o4g==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 14 Apr 2021 13:37:48 +0000
  • Ironport-hdrordr: A9a23:Z8Xh+qn3Vi3398RNs050EgtJFj/pDfNRjmdD5ilNYBxZY6Wkvu iUtrAyyQL0hDENWHsphNCHP+26TWnB8INuiLNhX4uKdDLN/E+lNptr44en+T3vHCXi6vVQvJ 0QEJRWIObbSWJ3hcOS2mmFOv4rytWf7eSMjeDR039iQWhRGtBdxi1SYzzra3Febg9AGJY/Cd 647s1IuzKvdR0sH7SGL1MCWPXOoMCOqYL+bXc9dl4awSSHkD/A0s+GLzG2xREbOgk/posK0W +AqADh47XmjvfT8G6j60by9JJQod371594KffksKMoAxHNrirtW4h7Qb2Fu1kO0aKSwXInis PFrRtlH+kb0QKsQkiPrRHg2xbt3V8VghePpjH48B6TxfDRfz40B9FMgohUaHLimjUdlepxzb 5R2Cahv4dXZCmwwRjV3cTCVB1hiyOP0AIfuNMTlHBWXM8/b7JcvOUkjTloOaoABy7z5cQbFv BvBqjnlY1rWG6dBkqp2FVH8ZiNRXI1JxGcXwwrhaWuuQR+rTRc4WNd3tAVmncb7pI6TPB/it jsA+BNrvVjX8UWZaVyCKMqWs2sEFHARhrKLSa7PUnnPLtvAQOOl7fHpJEOoM26cp0By5U/3L 7bVklDiGI0c0XyTeWTwZxw9AzXSmnVZ0Wp9uhuo7xC/pHsTrviNiOODHo0ldG7nvkZCsrHH9 G+JYxRGP2mCWf1A45G00nfVvBpWD0jefxQnux+d0OFo8rNJIGvnPfcauzvKL3kFithVXj4Bn cFQTjvNMRN5k2mQRbD8V7sckKoXna60YN7EaDc8eRW4pMKLJdwvg8cjkn85szjE0wajoUGOG 9FZJ/3mKKyome7uUzS6X9yBxZbBkFJpLHpU3ZAox4WI1r5GIxz4+m3SCR35j+qNxV/R8TZHE p0vFJs45+6KJSW2GQlENKoMmWTinMJv3KUR5IAmqmOjP2VPa8QP9IDYuhcBA/LHxt6lUJBs2 FYcjIJQUfZC3fzk6m/lYcVA+vebtF4hw+uLadv2CninHTZgftqamoQXjaoX8LSvB0nQCBMgE Ztt4UFhqCbpDqpIWwjoegxPVFWcl6LCLZeAAntXvQPppnbPCVLCUaDn3izlgw6cGuCzTRguk XRaQmvPcztLnUYkHZCyaru+E5zbQymDjBNQ0E/l5Z8G2TAsmt0ysmRaMOIojSsQ2pH/8VYCh b5W38pBj5WrurHiCK9kCqeFHkg25UlNvHcCrNmaL3IxnaxMuSz5NQ7Nu4R85B/ON/0tOgXFe qZZg+ONTv9T/gkwgqPux8eSWdJgWhhlfPjwxv+6mekmHY5HPrJOVxjLotrb+20/izhR/yS1o 9+gs9wteysMn/pYtrDza3MdTZMJlfSpmGxJttY5qx8rOY3tLFpGYPcXiaN3HZb3A8mJMOxjV gAWs1Akfv8E54qe9ZXdzNS/1IvmtjKJEw3shbuCut7eV02lXfUM96A/rKgk8tvPmSR4A/rfV WP+SxU+PnIGzGO0rMXEKo8K2VbYkpU0gUuwMqSM4nLTAm6feBK+1S3dmKneLhGUa6fBPEeqA 1579zgpZ7eSwPonATL+T11LaJF/zz5HYe8AAeQFfVJ9NL/M1KWmaeu6NOyijCySTbTUTVvua RVMUgLKsJEgX0+iYdy1C64QKn+uFgknFtT+isPrC+m5qG2pGPAWVhbOgjYiIhMVTZdMnKUnd 3ImNLoo0jV8XxAw93fD09ecdFFBsgIQoX2JyloL9IMvLTAxdtnvg1TJBE0D2A9jzjh3+Rpmb ehsc+iL9HfNQ==
  • Ironport-sdr: LerZZRyA23BiGxvBbmBlVPKkrljyB3yn3Z6NX0VMz2MTwSktBH+uyds8DjUDOWHtg5P90Fiu0Y GXGbDIrktv5nEVYOPhjiNvwzoSLfonJtm50Jf/GxSl8fbaLCgFJqskcIlwetIThTXRAKMej5bc HgEffIbKQYraz/7KBG7xqHTpQOjQiA9Dp3OPRyA7Zg80JuRkur9V8wH6HjU00fY8AHPzUlbwxh oMCvr8wqbVPjJvib/Zb/dCd6niZsv1vxwN8iGU7lRq8jlkP7cV79rc1WqKR3GzlKbUuvepr63C Uu0=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Wed, Apr 14, 2021 at 12:28:43PM +0200, Jan Beulich wrote:
> On 31.03.2021 12:33, Roger Pau Monne wrote:
> > ---
> >  xen/arch/x86/hvm/svm/intr.c   |   3 -
> >  xen/arch/x86/hvm/vmx/intr.c   |  59 ------
> >  xen/arch/x86/hvm/vpt.c        | 334 ++++++++++++++--------------------
> >  xen/include/asm-x86/hvm/vpt.h |   5 +-
> >  4 files changed, 143 insertions(+), 258 deletions(-)
> 
> Nice.
> 
> > @@ -285,189 +238,144 @@ static void pt_irq_fired(struct vcpu *v, struct 
> > periodic_time *pt)
> >              list_del(&pt->list);
> >          pt->on_list = false;
> >          pt->pending_intr_nr = 0;
> > +
> > +        return;
> >      }
> > -    else if ( mode_is(v->domain, one_missed_tick_pending) ||
> > -              mode_is(v->domain, no_missed_ticks_pending) )
> > +
> > +    if ( mode_is(v->domain, one_missed_tick_pending) ||
> > +         mode_is(v->domain, no_missed_ticks_pending) )
> >      {
> > -        pt->last_plt_gtime = hvm_get_guest_time(v);
> >          pt_process_missed_ticks(pt);
> >          pt->pending_intr_nr = 0; /* 'collapse' all missed ticks */
> > +    }
> > +    else if ( !pt->pending_intr_nr )
> > +        pt_process_missed_ticks(pt);
> 
> Did you lose a -- here? I.e. does the condition mean to match ...
> 
> > +    if ( !pt->pending_intr_nr )
> >          set_timer(&pt->timer, pt->scheduled);
> > +}
> > +
> > +static void pt_timer_fn(void *data)
> > +{
> > +    struct periodic_time *pt = data;
> > +    struct vcpu *v;
> > +    time_cb *cb = NULL;
> > +    void *cb_priv;
> > +    unsigned int irq;
> > +
> > +    pt_lock(pt);
> > +
> > +    v = pt->vcpu;
> > +    irq = pt->irq;
> > +
> > +    if ( inject_interrupt(pt) )
> > +    {
> > +        pt->scheduled += pt->period;
> > +        pt->do_not_freeze = 0;
> > +        cb = pt->cb;
> > +        cb_priv = pt->priv;
> >      }
> >      else
> >      {
> > -        pt->last_plt_gtime += pt->period;
> > -        if ( --pt->pending_intr_nr == 0 )
> 
> ... this original code? Otherwise I can't see why the condition
> guards a pt_process_missed_ticks() invocation.

I think the logic here changed enough to not match anymore. Certainly
pending_intr_nr shouldn't be decreased there, as pt_irq_fired is
invoked after an EOI in this patch, instead of being invoked when a
vpt related interrupt was injected. I think I should better rename
pt_irq_fired to pt_irq_eoi and that would make it clearer.

FWIW, decreasing pending_intr_nr should only be done after an
inject_interrupt call.

> > @@ -617,20 +556,29 @@ void pt_adjust_global_vcpu_target(struct vcpu *v)
> >      write_unlock(&pl_time->vhpet.lock);
> >  }
> >  
> > -
> >  static void pt_resume(struct periodic_time *pt)
> >  {
> > +    struct vcpu *v;
> > +    time_cb *cb = NULL;
> > +    void *cb_priv;
> > +
> >      if ( pt->vcpu == NULL )
> >          return;
> >  
> >      pt_lock(pt);
> > -    if ( pt->pending_intr_nr && !pt->on_list )
> > +    if ( pt->pending_intr_nr && !pt->on_list && inject_interrupt(pt) )
> >      {
> > +        pt->pending_intr_nr--;
> > +        cb = pt->cb;
> > +        cb_priv = pt->priv;
> > +        v = pt->vcpu;
> >          pt->on_list = 1;
> >          list_add(&pt->list, &pt->vcpu->arch.hvm.tm_list);
> > -        vcpu_kick(pt->vcpu);
> >      }
> >      pt_unlock(pt);
> > +
> > +    if ( cb )
> > +        cb(v, cb_priv);
> >  }
> 
> I'm afraid until we raise our supported gcc versions baseline, v and
> cb_priv will need an initializer at the top of the function just like
> cb.

Will add such initializations.

Thanks, Roger.



 


Rackspace

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