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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> +    /*
> +     * 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