| Message ID | 1242757912-6041-2-git-send-email-weidong.han@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Weidong Han <weidong.han@intel.com> wrote: > Interrupt remapping table entry is 128bits. Currently, it only sets low > 64bits of irte in modify_irte and free_irte. This ignores high 64bits > setting of irte, that means source-id setting will be ignored. This patch > sets the whole 128bits of irte when modify/free it. Following source-id > checking patch depends on this. > > Signed-off-by: Weidong Han <weidong.han@intel.com> > --- > drivers/pci/intr_remapping.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c > index f5e0ea7..946e170 100644 > --- a/drivers/pci/intr_remapping.c > +++ b/drivers/pci/intr_remapping.c > @@ -309,7 +309,8 @@ int modify_irte(int irq, struct irte *irte_modified) > index = irq_iommu->irte_index + irq_iommu->sub_handle; > irte = &iommu->ir_table->base[index]; > > - set_64bit((unsigned long *)irte, irte_modified->low); > + set_64bit((unsigned long *)&irte->low, irte_modified->low); > + set_64bit((unsigned long *)&irte->high, irte_modified->high); > > __iommu_flush_cache(iommu, irte, sizeof(*irte)); > > rc = qi_flush_iec(iommu, index, 0); > @@ -386,8 +387,11 @@ int free_irte(int irq) > irte = &iommu->ir_table->base[index]; > > if (!irq_iommu->sub_handle) { > - for (i = 0; i < (1 << irq_iommu->irte_mask); i++) > - set_64bit((unsigned long *)(irte + i), 0); > + for (i = 0; i < (1 << irq_iommu->irte_mask); i++) { > + set_64bit((unsigned long *)&irte->low, 0); > + set_64bit((unsigned long *)&irte->high, 0); > + irte++; > + } The loop is a bit unclean. It has a side-effect on 'irte' - and other patterns in the driver usually treat 'irte' as a generally available variable. So the above code, while correct, opens up the possibility of later code added to this function relying on 'irte', thinking that it's set to "&iommu->ir_table->base[index]", and then breaking because 'irte' has been iterated to the end of it in certain circumstances. It's better to factor out the whole loop into a helper function, which does something like: int flush_entries(struct irq_2_iommu *irq_iommu) { struct irte *start, *entry, *end; struct intel_iommu *iommu; int index; if (irq_iommu->sub_handle) return 0; iommu = irq_iommu->iommu; index = irq_iommu->irte_index + irq_iommu->sub_handle; start = iommu->ir_table->base + index; end = start + (1 << irq_iommu->irte_mask); for (entry = start; entry < end; entry++) { set_64bit((unsigned long *)&entry->low, 0); set_64bit((unsigned long *)&entry->high, 0); } return qi_flush_iec(iommu, index, irq_iommu->irte_mask); } Note how clearer this is - the new method has one purpose and 'entry' is a clear iterator. ( And note how much clearer the flow of 'rc' has become as well as a side-effect: it is clear now that it's set to 0 when irq_iommu->sub_handle is still present. ) Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ingo Molnar wrote: > * Weidong Han <weidong.han@intel.com> wrote: > >> Interrupt remapping table entry is 128bits. Currently, it only sets >> low 64bits of irte in modify_irte and free_irte. This ignores high >> 64bits setting of irte, that means source-id setting will be >> ignored. This patch sets the whole 128bits of irte when modify/free >> it. Following source-id checking patch depends on this. >> >> Signed-off-by: Weidong Han <weidong.han@intel.com> >> --- >> drivers/pci/intr_remapping.c | 10 +++++++--- >> 1 files changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/intr_remapping.c >> b/drivers/pci/intr_remapping.c index f5e0ea7..946e170 100644 --- >> a/drivers/pci/intr_remapping.c +++ b/drivers/pci/intr_remapping.c >> @@ -309,7 +309,8 @@ int modify_irte(int irq, struct irte >> *irte_modified) index = irq_iommu->irte_index + >> irq_iommu->sub_handle; irte = &iommu->ir_table->base[index]; >> >> - set_64bit((unsigned long *)irte, irte_modified->low); >> + set_64bit((unsigned long *)&irte->low, irte_modified->low); >> + set_64bit((unsigned long *)&irte->high, irte_modified->high); >> >> __iommu_flush_cache(iommu, irte, sizeof(*irte)); >> >> rc = qi_flush_iec(iommu, index, 0); >> @@ -386,8 +387,11 @@ int free_irte(int irq) >> irte = &iommu->ir_table->base[index]; >> >> if (!irq_iommu->sub_handle) { >> - for (i = 0; i < (1 << irq_iommu->irte_mask); i++) >> - set_64bit((unsigned long *)(irte + i), 0); >> + for (i = 0; i < (1 << irq_iommu->irte_mask); i++) { >> + set_64bit((unsigned long *)&irte->low, 0); >> + set_64bit((unsigned long *)&irte->high, 0); >> + irte++; >> + } > > The loop is a bit unclean. It has a side-effect on 'irte' - and > other patterns in the driver usually treat 'irte' as a generally > available variable. > > So the above code, while correct, opens up the possibility of later > code added to this function relying on 'irte', thinking that it's > set to "&iommu->ir_table->base[index]", and then breaking because > 'irte' has been iterated to the end of it in certain circumstances. > > It's better to factor out the whole loop into a helper function, > which does something like: > > int flush_entries(struct irq_2_iommu *irq_iommu) > { > struct irte *start, *entry, *end; > struct intel_iommu *iommu; > int index; > > if (irq_iommu->sub_handle) > return 0; > > iommu = irq_iommu->iommu; > index = irq_iommu->irte_index + irq_iommu->sub_handle; > > start = iommu->ir_table->base + index; > end = start + (1 << irq_iommu->irte_mask); > > for (entry = start; entry < end; entry++) { > set_64bit((unsigned long *)&entry->low, 0); > set_64bit((unsigned long *)&entry->high, 0); > } > > return qi_flush_iec(iommu, index, irq_iommu->irte_mask); > } > > Note how clearer this is - the new method has one purpose and > 'entry' is a clear iterator. > > ( And note how much clearer the flow of 'rc' has become as well as a > side-effect: it is clear now that it's set to 0 when > irq_iommu->sub_handle is still present. ) > > Thanks, > > Ingo Yes, more clearer. will update it in next version. Thanks. Regards, Weidong-- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/intr_remapping.c b/drivers/pci/intr_remapping.c index f5e0ea7..946e170 100644 --- a/drivers/pci/intr_remapping.c +++ b/drivers/pci/intr_remapping.c @@ -309,7 +309,8 @@ int modify_irte(int irq, struct irte *irte_modified) index = irq_iommu->irte_index + irq_iommu->sub_handle; irte = &iommu->ir_table->base[index]; - set_64bit((unsigned long *)irte, irte_modified->low); + set_64bit((unsigned long *)&irte->low, irte_modified->low); + set_64bit((unsigned long *)&irte->high, irte_modified->high); __iommu_flush_cache(iommu, irte, sizeof(*irte)); rc = qi_flush_iec(iommu, index, 0); @@ -386,8 +387,11 @@ int free_irte(int irq) irte = &iommu->ir_table->base[index]; if (!irq_iommu->sub_handle) { - for (i = 0; i < (1 << irq_iommu->irte_mask); i++) - set_64bit((unsigned long *)(irte + i), 0); + for (i = 0; i < (1 << irq_iommu->irte_mask); i++) { + set_64bit((unsigned long *)&irte->low, 0); + set_64bit((unsigned long *)&irte->high, 0); + irte++; + } rc = qi_flush_iec(iommu, index, irq_iommu->irte_mask); }
Interrupt remapping table entry is 128bits. Currently, it only sets low 64bits of irte in modify_irte and free_irte. This ignores high 64bits setting of irte, that means source-id setting will be ignored. This patch sets the whole 128bits of irte when modify/free it. Following source-id checking patch depends on this. Signed-off-by: Weidong Han <weidong.han@intel.com> --- drivers/pci/intr_remapping.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)