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

Jan

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