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

[Jan Beulich]

>>> On 03.11.16 at 08:46,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Monday, October 31, 2016 10:57 PM
>> >>> On 28.10.16 at 04:37,  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.

You continue to look at this from what I think is the wrong angle:
msi_msg_to_remap_entry() should not make assumptions or
special case either of the formats. It should construct the new
entry (whichever format that's supposed to be) and compare
all relevant (for the given format) fields with those of the old
entry. If they all match, skip the 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 );

Btw, stray blanks here (in case I didn't point this out before).

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

It's the assumption expressed by the second sentence of your reply
which is the problem: You're making an assumption here which you
shouldn't make - I don't see the caller guaranteeing that assumption,
and even if this one caller did, a function like the one here should be
generic enough so that future callers won't need to worry about
preconditions.

Jan


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

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

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, October 31, 2016 10:57 PM
> To: Wu, Feng 
> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com;
> george.dun...@eu.citrix.com; Tian, Kevin ; xen-
> de...@lists.xen.org
> 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,  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 28.10.16 at 04:37,  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@lists.xen.org
https://lists.xen.org/xen-devel