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 6:57 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 12:18, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----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?
> 
> No. Just to repeat: "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." IOW
> the checking function should really just be checking things, and it
> should do so (correctly) for all possible inputs. Its return value
> ought to indicate whether the update can be suppressed.
> 

Okay, I can make a checking only function. But I would like to listen
to your advice about how to handle the case: " if it is in remapped
mode, we need to suppress the affinity related updates, and only
update the fields:  'fpd', 'sid', 'sq', and 'svt'". Is this okay?

> Jan

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