> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: Monday, October 24, 2016 7:31 PM > To: Wu, Feng <feng...@intel.com> > Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- > de...@lists.xen.org > 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...@intel.com> wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> Sent: Monday, October 24, 2016 6:57 PM > >> To: Wu, Feng <feng...@intel.com> > >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- > >> de...@lists.xen.org > >> 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...@intel.com> wrote: > >> > >> > > >> >> -----Original Message----- > >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> Sent: Monday, October 24, 2016 5:54 PM > >> >> To: Wu, Feng <feng...@intel.com> > >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; xen- > >> >> de...@lists.xen.org > >> >> 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...@intel.com> wrote: > >> >> > >> >> > > >> >> >> -----Original Message----- > >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> Sent: Monday, October 24, 2016 3:28 PM > >> >> >> To: Wu, Feng <feng...@intel.com> > >> >> >> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com; > >> >> >> george.dun...@eu.citrix.com; Tian, Kevin <kevin.t...@intel.com>; > xen- > >> >> >> de...@lists.xen.org > >> >> >> 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...@intel.com> wrote: > >> >> >> >> From: Jan Beulich [mailto:jbeul...@suse.com] > >> >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM > >> >> >> >> >>> On 11.10.16 at 02:57, <feng...@intel.com> 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@lists.xen.org https://lists.xen.org/xen-devel
