Re: [Xen-devel] [RFC v1 08/15] Update IRTE according to guest interrupt config changes

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Wu, Feng
> Sent: Thursday, April 02, 2015 4:03 PM
> 
> 
> 
> > -----Original Message-----
> > From: Tian, Kevin
> > Sent: Thursday, April 02, 2015 2:50 PM
> > To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> > Cc: JBeulich@xxxxxxxx; keir@xxxxxxx; Zhang, Yang Z
> > Subject: RE: [RFC v1 08/15] Update IRTE according to guest interrupt config
> > changes
> >
> > > From: Wu, Feng
> > > Sent: Thursday, April 02, 2015 2:21 PM
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Tian, Kevin
> > > > Sent: Thursday, April 02, 2015 1:52 PM
> > > > To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> > > > Cc: JBeulich@xxxxxxxx; keir@xxxxxxx; Zhang, Yang Z
> > > > Subject: RE: [RFC v1 08/15] Update IRTE according to guest interrupt
> config
> > > > changes
> > > >
> > > > > From: Wu, Feng
> > > > > Sent: Wednesday, March 25, 2015 8:32 PM
> > > > >
> > > > > When guest changes its interrupt configuration (such as, vector, etc.)
> > > > > for direct-assigned devices, we need to update the associated IRTE
> > > > > with the new guest vector, so external interrupts from the assigned
> > > > > devices can be injected to guests without VM-Exit.
> > > > >
> > > > > For lowest-priority interrupts, we use vector-hashing mechamisn to
> find
> > > > > the destination vCPU. This follows the hardware behavior, since
> modern
> > > > > Intel CPUs use vector hashing to handle the lowest-priority interrupt.
> > > > >
> > > > > For multicase/broadcast vCPU, we cannot handle it via interrupt
> posting,
> > > > > still use interrupt remapping.
> > > > >
> > > > > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > > > > ---
> > > > >  xen/drivers/passthrough/io.c | 77
> > > > > +++++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 76 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/xen/drivers/passthrough/io.c 
> > > > > b/xen/drivers/passthrough/io.c
> > > > > index ae050df..1d9a132 100644
> > > > > --- a/xen/drivers/passthrough/io.c
> > > > > +++ b/xen/drivers/passthrough/io.c
> > > > > @@ -26,6 +26,7 @@
> > > > >  #include <asm/hvm/iommu.h>
> > > > >  #include <asm/hvm/support.h>
> > > > >  #include <xen/hvm/irq.h>
> > > > > +#include <asm/io_apic.h>
> > > > >
> > > > >  static DEFINE_PER_CPU(struct list_head, dpci_list);
> > > > >
> > > > > @@ -199,6 +200,61 @@ void free_hvm_irq_dpci(struct hvm_irq_dpci
> > > *dpci)
> > > > >      xfree(dpci);
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * Here we handle the following cases:
> > > > > + * - For lowest-priority interrupts, we find the destination vCPU 
> > > > > from
> the
> > > > > + *   guest vector using vector-hashing mechamisn and return true.
> This
> > > > > follows
> > > > > + *   the hardware behavior, since modern Intel CPUs use vector
> > hashing
> > > to
> > > > > + *   handle the lowest-priority interrupt.
> > > > > + * - Otherwise, for single destination interrupt, it is 
> > > > > straightforward to
> > > > > + *   find the destination vCPU and return true.
> > > > > + * - For multicase/broadcast vCPU, we cannot handle it via interrupt
> > > posting,
> > > > > + *   so return false.
> > > > > + */
> > > > > +static bool_t pi_find_dest_vcpu(struct domain *d, uint8_t dest_id,
> > > > > +                                uint8_t dest_mode, uint8_t
> > > > > deliver_mode,
> > > > > +                                uint32_t gvec, struct vcpu
> > > > > **dest_vcpu)
> > > > > +{
> > > > > +    struct vcpu *v, **dest_vcpu_array;
> > > > > +    unsigned int dest_vcpu_num = 0;
> > > > > +    int ret;
> > > > > +
> > > > > +    if ( deliver_mode == dest_LowestPrio )
> > > > > +        dest_vcpu_array = xzalloc_array(struct vcpu *,
> d->max_vcpus);
> > > > > +
> > > > > +    for_each_vcpu ( d, v )
> > > > > +    {
> > > > > +        if ( !vlapic_match_dest(vcpu_vlapic(v), NULL, 0,
> > > > > +                                dest_id, dest_mode) )
> > > > > +            continue;
> > > > > +
> > > > > +        dest_vcpu_num++;
> > > > > +
> > > > > +        if ( deliver_mode == dest_LowestPrio )
> > > > > +            dest_vcpu_array[dest_vcpu_num] = v;
> > > > > +        else
> > > > > +            *dest_vcpu = v;
> > > > > +    }
> > > > > +
> > > > > +    if ( deliver_mode == dest_LowestPrio )
> > > > > +    {
> > > > > +        if (  dest_vcpu_num != 0 )
> > > > > +        {
> > > > > +            *dest_vcpu = dest_vcpu_array[gvec %
> dest_vcpu_num];
> > > > > +            ret = 1;
> > > > > +        }
> > > > > +        else
> > > > > +            ret = 0;
> > > > > +
> > > > > +        xfree(dest_vcpu_array);
> > > > > +        return ret;
> > > > > +    }
> > > > > +    else if (  dest_vcpu_num == 1 )
> > > > > +        return 1;
> > > > > +    else
> > > > > +        return 0;
> > > > > +}
> > > > > +
> > > > >  int pt_irq_create_bind(
> > > > >      struct domain *d, xen_domctl_bind_pt_irq_t *pt_irq_bind)
> > > > >  {
> > > > > @@ -257,7 +313,7 @@ int pt_irq_create_bind(
> > > > >      {
> > > > >      case PT_IRQ_TYPE_MSI:
> > > > >      {
> > > > > -        uint8_t dest, dest_mode;
> > > > > +        uint8_t dest, dest_mode, deliver_mode;
> > > > >          int dest_vcpu_id;
> > > > >
> > > > >          if ( !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) )
> > > > > @@ -330,11 +386,30 @@ int pt_irq_create_bind(
> > > > >          /* Calculate dest_vcpu_id for MSI-type pirq migration. */
> > > > >          dest = pirq_dpci->gmsi.gflags & VMSI_DEST_ID_MASK;
> > > > >          dest_mode = !!(pirq_dpci->gmsi.gflags &
> VMSI_DM_MASK);
> > > > > +        deliver_mode = (pirq_dpci->gmsi.gflags >>
> > > > > GFLAGS_SHIFT_DELIV_MODE) &
> > > > > +                        VMSI_DELIV_MASK;
> > > > >          dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest,
> > dest_mode);
> > > > >          pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
> > > > >          spin_unlock(&d->event_lock);
> > > > >          if ( dest_vcpu_id >= 0 )
> > > > >              hvm_migrate_pirqs(d->vcpu[dest_vcpu_id]);
> > > > > +
> > > > > +        /* Use interrupt posting if it is supported */
> > > > > +        if ( iommu_intpost )
> > > > > +        {
> > > > > +            struct vcpu *vcpu = NULL;
> > > > > +
> > > > > +            if ( !pi_find_dest_vcpu(d, dest, dest_mode,
> deliver_mode,
> > > > > +                                    pirq_dpci->gmsi.gvec,
> > > &vcpu) )
> > > > > +                break;
> > > > > +
> > > >
> > > > Is it possible this new pi_find_dest_vcpu will return a different target
> from
> > > > earlier hvm_girq_des_2_vcpu_id? if yes it will cause tricky issues since
> > > > earlier pirqs are migrated according to different policy. We need
> consolidate
> > > > vcpu selection policies together to keep consistency.
> > >
> > > In my understanding, what you described above is the software way to
> > deliver
> > > the interrupts to vCPU, when posted-interrupt is used, interrupts are
> > delivered
> > > by hardware according to the settings in IRTE, hence those software path
> will
> > > not get touched for these interrupts. So do we need to care about how
> > > software
> > > might migrate the interrupts here?
> >
> > just curious why we can't use one policy for vcpu selection. if multicast
> > handling is a difference, you may pass intpost as a parameter to use
> > same function.
> >
> 
> Digging into hvm_girq_dest_2_vcpu_id, I find that hvm_girq_dest_2_vcpu_id()
> is introduced by commit 023e3bc7, and it is just an optimization for 
> interrupts
> with single destination. For most case, the destination of a vCPU is 
> determined
> by vmsi_deliver().
> 

if can't combine at all, at least we can make the function name clearer to
better reflect distinct purpose. :-)

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel