| Message ID | 1478506242-14606-1-git-send-email-feng.wu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 07.11.16 at 09:10, <feng.wu@intel.com> 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? And where's the else to deal with the posted format case? I have to admit that I don't see how this fits the model previously outlined, so if you could please explain ... Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Tuesday, November 8, 2016 1:00 AM > 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 v7 4/6] VT-d: No need to set irq affinity for posted format > IRTE > > >>> On 07.11.16 at 09:10, <feng.wu@intel.com> 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
>>> On 08.11.16 at 07:28, <feng.wu@intel.com> wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Tuesday, November 8, 2016 1:00 AM >> >>> On 07.11.16 at 09:10, <feng.wu@intel.com> 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. That's because ...? (And the answer would really belong in a code comment, if that conditional is to stay.) > 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(). Which may be part of the problem: Why are there two functions? > 3. The common field are set for both format, which is covered by > set_msi_source_id()/set_hpet_source_id() Yes, and that's fine. >> 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. As said, updating the common fields is fine, but I did ask before why updating the posted mode specific fields is unnecessary here. Once again - the function ought to be doing what its name says, without making (undocumented / un-ASSERT()ed-for) assumptions on caller behavior. Jan
> > > 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(). > > Which may be part of the problem: Why are there two functions? > I think the reason is that pi_update_irte() was introduced when we first enabled VT-d PI, and this patch handles some cases which need to be done in msi_msg_to_remap_entry(). But I think your suggestion here is good, I am thinking of calling msi_msg_to_remap_entry() (Need to add new parameter to this function) in pi_update_irte(). Thanks, Feng
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c index bfd468b..3f8c109 100644 --- 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 */ + } /* now construct new MSI/MSI-X rte entry */ remap_rte = (struct msi_msg_remap_entry *)msg; @@ -637,9 +640,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 ( iremap_entry->val != new_ire.val ) + { + 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> --- v7: - Compare all the field in IRTE to justify whether we can suppress the update 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 v5: - Only suppress affinity related IRTE updates for PI v4: - Keep the construction of new_ire and only modify the hardware IRTE when it is not in posted mode. xen/drivers/passthrough/vtd/intremap.c | 54 +++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 24 deletions(-)