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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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,
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.

Jan

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