>>> 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, 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@lists.xen.org https://lists.xen.org/xen-devel
