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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Monday, October 31, 2016 10:57 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>
> Cc: andrew.cooper3@xxxxxxxxxx; dario.faggioli@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; Tian, Kevin <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxx
> Subject: Re: [PATCH v6 5/7] VT-d: No need to set irq affinity for posted 
> format
> IRTE
> 
> >>> On 28.10.16 at 04:37, <feng.wu@xxxxxxxxx> wrote:
> > v6:
> > - Make pi_can_suppress_irte_update() a check-only function
> > - Introduce another function pi_get_new_irte() to update the 'new_ire' if
> > needed
> 
> I don't see what you need this function for. My earlier comments
> were not meant to make you split the function, but to drop all this
> secondary modification (unless there's actually a reason for this,
> which so far I haven't seen any proof of). Apart from that there
> already is setup_posted_irte(), which seems to do most if not all
> of what you really need.
> 
> In any event, the sequence of operations should be
> 1) create full new entry
> 2) check whether update is needed (i.e. whether old and new entries
>    have meaningful differences)
> 3) do update
> 4) flush.

Your early comment suggest we only suppress the affinity related
field. So the modification here is to handle the following case:

When the IRTE is currently in posted mode and this function tries
to update it he remapped mode, then we need to suppress
the affinity related bit, however, if the common fields (fpd, sid
sq, svt) in the new remapped IRTE is different from the value
in the old posted IRTE, what do you suggest we should do?
Here I copy these fields to the new IRTE and update them
accordingly.

> 
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -547,6 +547,54 @@ static int remap_entry_to_msi_msg(
> >      return 0;
> >  }
> >
> > +static bool pi_can_suppress_irte_update(const struct iremap_entry *new,
> > +    const struct iremap_entry *old)
> 
> The two parameter declarations should be aligned with one another.
> 
> > +{
> > +    ASSERT( old && new );
> > +
> > +    if ( !old->remap.p || !old->remap.im || !new->remap.p || new->remap.im 
> > )
> > +        return false;
> 
> The asymmetry regarding the IM bit is confusing, and imo indicates
> a problem with the logic.

Would you please share what the problem is here? If we get here, that means
we are updating a posted IRTE to remapped one.

Thanks,
Feng

> 
> > +    /*
> > +     * We are updating posted IRTE to remapped one, check whether
> > +     * the common fields are going to be changed.
> > +     */
> > +    if ( ( new->remap.fpd != old->post.fpd ) ||
> > +         ( new->remap.sid != old->post.sid ) ||
> > +         ( new->remap.sq != old->post.sq ) ||
> > +         ( new->remap.svt != old->post.svt ) )
> 
> Stray blanks inside the inner parentheses.
> 
> Jan


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