Re: [Xen-devel] [PATCH v7 4/6] 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: Tuesday, November 8, 2016 1:00 AM
> 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 v7 4/6] VT-d: No need to set irq affinity for posted 
> format
> IRTE
> 
> >>> On 07.11.16 at 09:10, <feng.wu@xxxxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -597,31 +597,34 @@ static int msi_msg_to_remap_entry(
> >
> >      memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry));
> >
> > -    /* Set interrupt remapping table entry */
> > -    new_ire.remap.fpd = 0;
> > -    new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
> & 0x1;
> > -    new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > -    new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) &
> 0x1;
> > -    /* Hardware require RH = 1 for LPR delivery mode */
> > -    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > -    new_ire.remap.avail = 0;
> > -    new_ire.remap.res_1 = 0;
> > -    new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > -                            MSI_DATA_VECTOR_MASK;
> > -    new_ire.remap.res_2 = 0;
> > -    if ( x2apic_enabled )
> > -        new_ire.remap.dst = msg->dest32;
> > -    else
> > -        new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
> > -                             & 0xff) << 8;
> > -
> >      if ( pdev )
> >          set_msi_source_id(pdev, &new_ire);
> >      else
> >          set_hpet_source_id(msi_desc->hpet_id, &new_ire);
> > -    new_ire.remap.res_3 = 0;
> > -    new_ire.remap.res_4 = 0;
> > -    new_ire.remap.p = 1;    /* finally, set present bit */
> > +
> > +    if ( !new_ire.remap.p || !new_ire.remap.im )
> > +    {
> > +        /* Set interrupt remapping table entry */
> > +        new_ire.remap.fpd = 0;
> > +        new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
> & 0x1;
> > +        new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > +        new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT)
> & 0x1;
> > +        /* Hardware require RH = 1 for LPR delivery mode */
> > +        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > +        new_ire.remap.avail = 0;
> > +        new_ire.remap.res_1 = 0;
> > +        new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > +                                MSI_DATA_VECTOR_MASK;
> > +        new_ire.remap.res_2 = 0;
> > +        if ( x2apic_enabled )
> > +            new_ire.remap.dst = msg->dest32;
> > +        else
> > +            new_ire.remap.dst = ((msg->address_lo >> 
> > MSI_ADDR_DEST_ID_SHIFT)
> > +                                 & 0xff) << 8;
> > +        new_ire.remap.res_3 = 0;
> > +        new_ire.remap.res_4 = 0;
> > +        new_ire.remap.p = 1;    /* finally, set present bit */
> > +    }
> 
> Why does setting the fields depend on the previous state of p and
> im? 

Okay, here are the questions. How do you think we should set the
'new_ire'? how to decide to set it to remapped mode or posted mode?

Here, I set 'new_ire' based on the following policy:
1. if previous p is 0, which means we initial a new IRTE, then we can
only set it to remapped mode. We never set a new IRTE to posted mode.
2. if previous p is 1 and it is in remapped mode, we can only set it to
remapped mode in _this_ function, setting it to posted mode is in
another function: pi_update_irte().
3. The common field are set for both format, which is covered by
set_msi_source_id()/set_hpet_source_id()

> And where's the else to deal with the posted format case? 

We don't need the else part, 'new_ire' is gotten from old 'iremap_entry',
and if 'iremap_entry' is in posted mode, we don't need to modify the
posted related bit in 'new_ire' and we only need to update the common
field which is done by set_msi_source_id()/set_hpet_source_id() above.

Thanks,
Feng


> I have
> to admit that I don't see how this fits the model previously outlined,
> so if you could please explain ...
> 
> Jan


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