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 7:31 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 13:10, <feng.wu@xxxxxxxxx> wrote:
> 
> >
> >> -----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?
> 
> First of all I think you mean "if it is in posted mode". But then yes,

I mean "if it is in remapped mode", here _it_ refers to the old IRTE.
we only need to suppress the affinity related field when we update
a remapped IRTE to posted IRTE. If the old IRTE is in posted mode,
we can just update the new posted mode IRTE.

> of course only fields that are relevant in the respective format
> need updating. Yet once again - a fresh IRTE gets prepared anyway,
> to it's really just a matter of which fields the checking function should
> compare in both modes (of course provided the mode itself doesn't
> change). And then - also as said before - I don't think the list you
> gave is exhaustive.

I really don't get the point why you think the list is not enough. Could
you please explain more, thanks a lot!

Thanks,
Feng

> 
> Jan

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