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

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, October 24, 2016 5:54 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx
> Subject: RE: [PATCH v5 5/7] VT-d: No need to set irq affinity for posted 
> format
> IRTE
> 
> >>> On 24.10.16 at 10:57, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Monday, October 24, 2016 3:28 PM
> >> To: Wu, Feng <feng.wu@xxxxxxxxx>
> >> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> >> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> >> devel@xxxxxxxxxxxxx
> >> Subject: RE: [PATCH v5 5/7] VT-d: No need to set irq affinity for posted
> > format
> >> IRTE
> >>
> >> >>> On 17.10.16 at 09:02, <feng.wu@xxxxxxxxx> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> >> Sent: Wednesday, October 12, 2016 9:56 PM
> >> >> >>> On 11.10.16 at 02:57, <feng.wu@xxxxxxxxx> wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/intremap.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> >> >> > @@ -547,6 +547,49 @@ static int remap_entry_to_msi_msg(
> >> >> >      return 0;
> >> >> >  }
> >> >> >
> >> >> > +static bool_t pi_can_suppress_irte_update(struct iremap_entry *new,
> >> >>
> >> >> bool (and true/false respectively) please.
> >> >>
> >> >> And then the function name suggests that no modification gets done
> >> >> here (and hence the first parameter could be const too), yet the
> >> >> implementation does otherwise (and I don't understand why).
> >> >
> >> > The idea here is that if the old IRTE is in posted format and fields like
> >> > 'fpd', 'sid', 'sq', or 'svt' is going to be changed , we need to use 
> >> > these
> >> > new values for the new_ire, while we still need to use the old values
> >> > of other fields in IRTE, so this function returns the new irte in its 
> >> > first
> >> >  parameter it we cannot suppress the update. I try to do it in this
> >> > function.
> >>
> >> I don't understand: The caller fully constructs the new entry. Why
> >> would you want to do further modifications here? I continue to
> >> think that this function should solely check whether the changes
> >> between old and new entry are such that the actual update (and
> >> hence the flush) can be bypassed.
> >>
> >> >> > +    const struct iremap_entry *old)
> >> >> > +{
> >> >> > +    bool_t ret = 1;
> >> >> > +    u16 fpd, sid, sq, svt;
> >> >> > +
> >> >> > +    if ( !old->remap.p || !old->remap.im )
> >> >> > +        return 0;
> >> >> > +
> >> >> > +    fpd = new->post.fpd;
> >> >> > +    sid = new->post.sid;
> >> >> > +    sq = new->post.sq;
> >> >> > +    svt = new->post.svt;
> >> >> > +
> >> >> > +    *new = *old;
> >> >> > +
> >> >> > +    if ( fpd != old->post.fpd )
> >> >> > +    {
> >> >> > +        new->post.fpd = fpd;
> >> >> > +        ret = 0;
> >> >> > +    }
> >> >> > +
> >> >> > +    if ( sid != old->post.sid )
> >> >> > +    {
> >> >> > +        new->post.sid = sid;
> >> >> > +        ret = 0;
> >> >> > +    }
> >> >> > +
> >> >> > +    if ( sq != old->post.sq )
> >> >> > +    {
> >> >> > +        new->post.sq = sq;
> >> >> > +        ret = 0;
> >> >> > +    }
> >> >> > +
> >> >> > +    if ( svt != old->post.svt )
> >> >> > +    {
> >> >> > +        new->post.svt = svt;
> >> >> > +        ret = 0;
> >> >> > +    }
> >> >>
> >> >> What's the selection of the fields based on? Namely, what about
> >> >> vector, pda_l, and pda_h?
> >> >
> >> > These filed are the common field between posted format and remapped
> >> format.
> >> > 'vector' field has different meaning in the two formant, pda_l and pda_h 
> >> > is
> >> only
> >> > for posted format. As mentioned above, the purpose of this function is to
> find
> >> > whether use need to update this common field in posted format, if it is, 
> >> > we
> >> need
> >> > to use them and reuse the old value of other fields (pda_l, pda_h, 
> >> > vector,
> etc.).
> >> > since we need to suppress affinity related update for posted format.
> >>
> >> If that was the case, then the first thing you'd need to check would
> >> be whether the format actually changes. If it doesn't, all fields need
> >> to be compared, while if it does change, the write (and flush) clearly
> >> can't be suppressed.
> >>
> >
> > Let me elaborate a bit more on the function to make things clear:
> > 1. If the old IRTE is present, or it is in remapped mode, we cannot suppress
> > the update, such as we may create a new IRTE and put it in remapped mode,
> > or update the remapped mode to posted mode.
> > 2. If the condition in step 1 is false, that means the old IRTE is present 
> > and
> > in posted mode, so we need to suppress the affinity related updates,
> 
> But only if the new entry is in posted mode too - see my earlier reply.
> 
> > and
> > only update the fields:  'fpd', 'sid', 'sq', or 'svt'. (Here maybe we need 
> > to check
> > whether if the new IRTE is in posted mode, if it is we need to update all
> > the field, but currently if we update posted mode -> posted mode, we don't
> > go to this function, it is done in pi_update_irte(),
> 
> Which looks like a code flow problem anyway - there shouldn't be
> direct calls from vendor independent code to vendor dependent
> functions. And then I can't see how the call to pi_update_irte()
> prevents execution flow reaching msi_msg_to_remap_entry(); at
> best the function would just re-write the same entry unchanged.
> 
> In any event - msi_msg_to_remap_entry() should be correct for
> all current and future callers, and hence I continue to think you
> want to adjust the code as suggested.
> 
> > so maybe we can add a WARN_ON() for that case?)
> 
> We need to be very careful with such WARN_ON()s - they must
> not be guest triggerable (I think this one wouldn't be) and they
> should not be raised more than once until a "good" update
> happened again (to avoid spamming the log).

So based on your comments about, I summarize the handling flow:
1. The same as above
2. If the condition in step 1 is false, that means the old IRTE is present and
in posted mode. If the new IRTE is in posted mode, we just update it, but
if it is in remapped mode, we need to suppress the affinity related updates,
and only update the fields:  'fpd', 'sid', 'sq', and 'svt'.

Does this looks okay to you?

Thanks,
Feng

> 
> Jan

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