| Message ID | 1476147473-30970-6-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.10.16 at 02:57, <feng.wu@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). > + 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? Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, October 12, 2016 9:56 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: Re: [PATCH v5 5/7] VT-d: No need to set irq affinity for posted format > IRTE > > >>> On 11.10.16 at 02:57, <feng.wu@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. > > > + 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. Thanks, Feng > > Jan
>>> On 17.10.16 at 09:02, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Wednesday, October 12, 2016 9:56 PM >> >>> On 11.10.16 at 02:57, <feng.wu@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. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 24, 2016 3:28 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@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.wu@intel.com> wrote: > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Wednesday, October 12, 2016 9:56 PM > >> >>> On 11.10.16 at 02:57, <feng.wu@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, 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(), so maybe we can add a WARN_ON() for that case?) Does the above sound clear to you, if it doesn't, don't hesitate to point it out, thanks a lot! Thanks, Feng > Jan
>>> On 24.10.16 at 10:57, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, October 24, 2016 3:28 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@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.wu@intel.com> wrote: >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: Wednesday, October 12, 2016 9:56 PM >> >> >>> On 11.10.16 at 02:57, <feng.wu@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). Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 24, 2016 5:54 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@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.wu@intel.com> wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Monday, October 24, 2016 3:28 PM > >> To: Wu, Feng <feng.wu@intel.com> > >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > >> devel@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.wu@intel.com> wrote: > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: Wednesday, October 12, 2016 9:56 PM > >> >> >>> On 11.10.16 at 02:57, <feng.wu@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? Thanks, Feng > > Jan
>>> On 24.10.16 at 12:18, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, October 24, 2016 5:54 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@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.wu@intel.com> wrote: >> >> > >> >> -----Original Message----- >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: Monday, October 24, 2016 3:28 PM >> >> To: Wu, Feng <feng.wu@intel.com> >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> >> devel@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.wu@intel.com> wrote: >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM >> >> >> >>> On 11.10.16 at 02:57, <feng.wu@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. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 24, 2016 6:57 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@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.wu@intel.com> wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Monday, October 24, 2016 5:54 PM > >> To: Wu, Feng <feng.wu@intel.com> > >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > >> devel@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.wu@intel.com> wrote: > >> > >> > > >> >> -----Original Message----- > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: Monday, October 24, 2016 3:28 PM > >> >> To: Wu, Feng <feng.wu@intel.com> > >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > >> >> devel@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.wu@intel.com> wrote: > >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM > >> >> >> >>> On 11.10.16 at 02:57, <feng.wu@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? > Jan
>>> On 24.10.16 at 13:10, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Monday, October 24, 2016 6:57 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@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.wu@intel.com> wrote: >> >> > >> >> -----Original Message----- >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> Sent: Monday, October 24, 2016 5:54 PM >> >> To: Wu, Feng <feng.wu@intel.com> >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> >> devel@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.wu@intel.com> wrote: >> >> >> >> > >> >> >> -----Original Message----- >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> >> Sent: Monday, October 24, 2016 3:28 PM >> >> >> To: Wu, Feng <feng.wu@intel.com> >> >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> >> >> devel@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.wu@intel.com> wrote: >> >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] >> >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM >> >> >> >> >>> On 11.10.16 at 02:57, <feng.wu@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
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Monday, October 24, 2016 7:31 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@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.wu@intel.com> wrote: > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Monday, October 24, 2016 6:57 PM > >> To: Wu, Feng <feng.wu@intel.com> > >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > >> devel@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.wu@intel.com> wrote: > >> > >> > > >> >> -----Original Message----- > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> Sent: Monday, October 24, 2016 5:54 PM > >> >> To: Wu, Feng <feng.wu@intel.com> > >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > >> >> devel@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.wu@intel.com> wrote: > >> >> > >> >> > > >> >> >> -----Original Message----- > >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> >> Sent: Monday, October 24, 2016 3:28 PM > >> >> >> To: Wu, Feng <feng.wu@intel.com> > >> >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > >> >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; > xen- > >> >> >> devel@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.wu@intel.com> wrote: > >> >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM > >> >> >> >> >>> On 11.10.16 at 02:57, <feng.wu@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
> From: Wu, Feng > Sent: Tuesday, October 25, 2016 9:05 AM > > > > -----Original Message----- > > From: Jan Beulich [mailto:JBeulich@suse.com] > > Sent: Monday, October 24, 2016 7:31 PM > > To: Wu, Feng <feng.wu@intel.com> > > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > > devel@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.wu@intel.com> wrote: > > > > > > > >> -----Original Message----- > > >> From: Jan Beulich [mailto:JBeulich@suse.com] > > >> Sent: Monday, October 24, 2016 6:57 PM > > >> To: Wu, Feng <feng.wu@intel.com> > > >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > > >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > > >> devel@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.wu@intel.com> wrote: > > >> > > >> > > > >> >> -----Original Message----- > > >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > > >> >> Sent: Monday, October 24, 2016 5:54 PM > > >> >> To: Wu, Feng <feng.wu@intel.com> > > >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > > >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > > >> >> devel@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.wu@intel.com> wrote: > > >> >> > > >> >> > > > >> >> >> -----Original Message----- > > >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > > >> >> >> Sent: Monday, October 24, 2016 3:28 PM > > >> >> >> To: Wu, Feng <feng.wu@intel.com> > > >> >> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > > >> >> >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; > > xen- > > >> >> >> devel@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.wu@intel.com> wrote: > > >> >> >> >> From: Jan Beulich [mailto:JBeulich@suse.com] > > >> >> >> >> Sent: Wednesday, October 12, 2016 9:56 PM > > >> >> >> >> >>> On 11.10.16 at 02:57, <feng.wu@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! > Not sure whether I understand Jan's point correctly. But after going through your original patch, I felt it's hard to understand the current flow - you always prepare new_irte in remappable format regardless of actual format of old irte, and then relying on this new function to adjust and check whether actual updates are required for posted mode. It's not very readable w/o checking VT-d spec to know which fields are shared between two formats. Would it be clearer if you can prepare new_irte explicitly for different modes in msi_msg_to_remap_entry, and then use this new check-only function to judge any optimization that you intend for posted case? Thanks Kevin
>>> On 25.10.16 at 03:04, <feng.wu@intel.com> wrote: > I really don't get the point why you think the list is not enough. Could > you please explain more, thanks a lot! I have to admit that I don't really know what else to say. Isn't it quite obvious that for you to suppress the actual update, _all_ meaningful (in the given format) fields have to match? Unless you indeed compare all of them, you make assumptions about your callers only covering a subset of all possible inputs. And then I have to also say that for this simple an operation (comparing two IRTEs) the discussion has already gone on way too long. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, October 25, 2016 4:09 PM > To: Wu, Feng <feng.wu@intel.com> > Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; > george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- > devel@lists.xen.org > Subject: RE: [PATCH v5 5/7] VT-d: No need to set irq affinity for posted format > IRTE > > >>> On 25.10.16 at 03:04, <feng.wu@intel.com> wrote: > > I really don't get the point why you think the list is not enough. Could > > you please explain more, thanks a lot! > > I have to admit that I don't really know what else to say. Isn't it > quite obvious that for you to suppress the actual update, _all_ > meaningful (in the given format) fields have to match? Unless > you indeed compare all of them, you make assumptions about > your callers only covering a subset of all possible inputs. Well, the problem is when the current IRTE is in posted mode, and we use this function to update the IRTE to remapped mode, we cannot compare all the fields, since they have different format, and that is why I compare the common field ( other fields are either posted specific or remapped specific), it doesn't make sense to compare them between the two format. Basically, we can divide the format into some category: - Common field - posted specific (PI related) or remapped specific (affinity related field) The purpose here is to suppress the affinity related field, so when updating posted IRTE to remapped IRTE and common field changes, we need update them with the new value while suppressing the affinity related field. Thanks, Feng > > And then I have to also say that for this simple an operation > (comparing two IRTEs) the discussion has already gone on way > too long. > > Jan
>>> On 26.10.16 at 11:12, <feng.wu@intel.com> wrote: > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Tuesday, October 25, 2016 4:09 PM >> To: Wu, Feng <feng.wu@intel.com> >> Cc: andrew.cooper3@citrix.com; dario.faggioli@citrix.com; >> george.dunlap@eu.citrix.com; Tian, Kevin <kevin.tian@intel.com>; xen- >> devel@lists.xen.org >> Subject: RE: [PATCH v5 5/7] VT-d: No need to set irq affinity for posted > format >> IRTE >> >> >>> On 25.10.16 at 03:04, <feng.wu@intel.com> wrote: >> > I really don't get the point why you think the list is not enough. Could >> > you please explain more, thanks a lot! >> >> I have to admit that I don't really know what else to say. Isn't it >> quite obvious that for you to suppress the actual update, _all_ >> meaningful (in the given format) fields have to match? Unless >> you indeed compare all of them, you make assumptions about >> your callers only covering a subset of all possible inputs. > > Well, the problem is when the current IRTE is in posted mode, and we use > this function to update the IRTE to remapped mode, we cannot compare > all the fields, since they have different format, and that is why I compare > the common field ( other fields are either posted specific or remapped specific), > it doesn't make sense to compare them between the two format. Of course, and I've repeatedly said that the first check of course needs to be whether the two format bits are different - if they are, no further checking is needed, and the update must not be bypassed. > Basically, we can divide the format into some category: > - Common field > - posted specific (PI related) or remapped specific (affinity related field) That's fine of course, but any such checks would go after the format bit comparison anyway. Jan
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c index bfd468b..8dd0373 100644 --- 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, + 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; + } + + return ret; +} + static int msi_msg_to_remap_entry( struct iommu *iommu, struct pci_dev *pdev, struct msi_desc *msi_desc, struct msi_msg *msg) @@ -637,9 +680,12 @@ static int msi_msg_to_remap_entry( remap_rte->address_hi = 0; remap_rte->data = index - i; - memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry)); - iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry)); - iommu_flush_iec_index(iommu, 0, index); + if ( !pi_can_suppress_irte_update(&new_ire, iremap_entry) ) + { + memcpy(iremap_entry, &new_ire, sizeof(struct iremap_entry)); + iommu_flush_cache_entry(iremap_entry, sizeof(struct iremap_entry)); + iommu_flush_iec_index(iommu, 0, index); + } unmap_vtd_domain_page(iremap_entries); spin_unlock_irqrestore(&ir_ctrl->iremap_lock, flags);
We don't set the affinity for posted format IRTE, since the destination of these interrupts is vCPU and the vCPU affinity is set during vCPU scheduling. Signed-off-by: Feng Wu <feng.wu@intel.com> --- v5: - Only suppress affinity related IRTE updates for PI xen/drivers/passthrough/vtd/intremap.c | 52 ++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-)