Re: [Xen-devel] [PATCH v8 5/7] VT-d: No need to set irq affinity for posted format IRTE

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Tian, Kevin
> Sent: Friday, November 18, 2016 12:44 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: jbeulich@xxxxxxxx; andrew.cooper3@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; dario.faggioli@xxxxxxxxxx
> Subject: RE: [PATCH v8 5/7] VT-d: No need to set irq affinity for posted 
> format
> IRTE
> 
> > From: Wu, Feng
> > Sent: Friday, November 18, 2016 9:59 AM
> >
> > We don't set the affinity for posted format IRTE, since the
> > destination of these interrupts is vCPU and the vCPU affinity
> > is set during vCPU scheduling.
> >
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> > v8:
> > - Changes based on [6/7]
> 
> [5/7]?

Sorry, should be [4/7]

> 
> >
> > v7:
> > - Compare all the field in IRTE to justify whether we can suppress the 
> > update
> >
> > v6:
> > - Make pi_can_suppress_irte_update() a check-only function
> > - Introduce another function pi_get_new_irte() to update the 'new_ire' if
> needed
> >
> > v5:
> > - Only suppress affinity related IRTE updates for PI
> >
> > v4:
> > - Keep the construction of new_ire and only modify the hardware
> > IRTE when it is not in posted mode.
> >
> >  xen/drivers/passthrough/vtd/intremap.c | 87
> > ++++++++++++++++++++--------------
> >  1 file changed, 52 insertions(+), 35 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/intremap.c
> > b/xen/drivers/passthrough/vtd/intremap.c
> > index fd2a49a..0cb8c37 100644
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -600,27 +600,41 @@ static int msi_msg_to_remap_entry(
> >
> >      if ( !pi_desc )
> >      {
> > -        /* Set interrupt remapping table entry */
> > -        new_ire.remap.fpd = 0;
> > -        new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
> &
> > 0x1;
> > -        new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > -        new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT)
> &
> > 0x1;
> > -        /* Hardware require RH = 1 for LPR delivery mode */
> > -        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > -        new_ire.remap.avail = 0;
> > -        new_ire.remap.res_1 = 0;
> > -        new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > -                                MSI_DATA_VECTOR_MASK;
> > -        new_ire.remap.res_2 = 0;
> > -        if ( x2apic_enabled )
> > -            new_ire.remap.dst = msg->dest32;
> > -        else
> > -            new_ire.remap.dst = ((msg->address_lo >> 
> > MSI_ADDR_DEST_ID_SHIFT)
> > -                                 & 0xff) << 8;
> > +        /*
> > +         * We are here because we are trying to update the IRTE to remapped
> mode,
> > +         * we only need to update the remapped specific fields for the 
> > following
> > +         * two cases:
> 
> you said "we only need to update the remapped specific fields", while later...
> 
> > +         * 1. When we create a new IRTE. A new IRTE is created when we 
> > create
> a
> > +         *    new irq, so a new IRTE is always initialized with remapped 
> > format.
> > +         * 2. When the old IRTE is present and in remapped mode. Since if 
> > the old
> > +         *    IRTE is in posted mode, we cannot update it to remapped mode 
> > and
> > +         *    this is what we need to suppress. So we don't update the 
> > remapped
> > +         *    specific fields here, we only update the commom field.
> 
> you said "we don't update remapped specific fields"...

Sorry for the confusion. When I said "So we don't update the remapped specific
fields here, we only update the common field" above, I actually would like to
mean for the case "when we are updating remapped IRTE -> posted IRTE".
I will reword this to make it clear.

> 
> It's also unclear to me why we cannot change irte from posted mode back to
> remapped mode. Is it defined as a VT-d arch limitation? What about the other
> direction from remapped mode to posted mode?

It is not a VT-d arch limitation. We don't need that and currently don't have 
that
path to trigger the transition from posted IRTE to remapped one. And in fact 
that
is the main purpose of this patch to suppress affinity update to a posted mode
IRTE. And the other direction from remapped mode to posted mode is
straightforward, when IRTE is created with remapped mode, sometime later,
our code will update it to posted mode if the device is assigned to guest and
the guest updates the affinity.

> 
> > +         */
> > +        if ( !iremap_entry->remap.p || !iremap_entry->remap.im )
> > +        {
> > +            /* Set interrupt remapping table entry */
> > +            new_ire.remap.fpd = 0;
> > +            new_ire.remap.dm = (msg->address_lo >>
> MSI_ADDR_DESTMODE_SHIFT)
> > & 0x1;
> > +            new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > +            new_ire.remap.dlm = (msg->data >>
> MSI_DATA_DELIVERY_MODE_SHIFT)
> > & 0x1;
> > +            /* Hardware require RH = 1 for LPR delivery mode */
> > +            new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > +            new_ire.remap.avail = 0;
> > +            new_ire.remap.res_1 = 0;
> > +            new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > +                                    MSI_DATA_VECTOR_MASK;
> > +            new_ire.remap.res_2 = 0;
> > +            if ( x2apic_enabled )
> > +                new_ire.remap.dst = msg->dest32;
> > +            else
> > +                new_ire.remap.dst = ((msg->address_lo >>
> > MSI_ADDR_DEST_ID_SHIFT)
> > +                                     & 0xff) << 8;
> >
> > -        new_ire.remap.res_3 = 0;
> > -        new_ire.remap.res_4 = 0;
> > -        new_ire.remap.p = 1;    /* finally, set present bit */
> > +            new_ire.remap.res_3 = 0;
> > +            new_ire.remap.res_4 = 0;
> > +            new_ire.remap.p = 1;    /* finally, set present bit */
> > +        }
> 
> what about old_ire is in posted mode? If it cannot happen from posted
> to remap as you explained earlier, then should be an ASSERT here.
> Otherwise you just leave a condition unhandled...

If the old_ire is in posted mode, it is the case which we need to suppress
the affinity update. So here we don't update the 'new_ire' to remapped
mode and just leave it as the old new, and update the common field
in set_msi_source_id() if needed after the if-else part. And this is the
main purpose of this patch: suppress the affinity related update to
posted mode IRTE, but update the common fields if need. Back to
your comments in [4/7] about whether we need copy to old IRTE
value to 'new_ire', I think it is needed in this case.

Thanks,
Feng

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