On Thu, Aug 16, 2018 at 04:21:17PM +0800, Leizhen (ThunderTown) wrote: > On 2018/8/15 20:26, Robin Murphy wrote: > > On 15/08/18 11:23, Zhen Lei wrote: > >> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > >> index 1d64710..3f5c236 100644 > >> --- a/drivers/iommu/arm-smmu-v3.c > >> +++ b/drivers/iommu/arm-smmu-v3.c > >> @@ -566,7 +566,7 @@ struct arm_smmu_device { > >> > >> int gerr_irq; > >> int combined_irq; > >> - atomic_t sync_nr; > >> + u32 sync_nr; > >> > >> unsigned long ias; /* IPA */ > >> unsigned long oas; /* PA */ > >> @@ -775,6 +775,11 @@ static int queue_remove_raw(struct arm_smmu_queue *q, > >> u64 *ent) > >> return 0; > >> } > >> > >> +static inline void arm_smmu_cmdq_sync_set_msidata(u64 *cmd, u32 msidata) > > > > If we *are* going to go down this route then I think it would make sense > > to move the msiaddr and CMDQ_SYNC_0_CS_MSI logic here as well; i.e. > > arm_smmu_cmdq_build_cmd() always generates a "normal" SEV-based sync > > command, then calling this guy would convert it to an MSI-based one. > > As-is, having bits of mutually-dependent data handled across two > > separate places just seems too messy and error-prone. > > Yes, How about create a new function "arm_smmu_cmdq_build_sync_msi_cmd"? > > static inline > void arm_smmu_cmdq_build_sync_msi_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) > { > cmd[0] = FIELD_PREP(CMDQ_0_OP, ent->opcode); > cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_IRQ); > cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH); > cmd[0] |= FIELD_PREP(CMDQ_SYNC_0_MSIATTR, ARM_SMMU_MEMATTR_OIWB); > cmd[1] = ent->sync.msiaddr & CMDQ_SYNC_1_MSIADDR_MASK; > }
None of this seems justified given the numbers from John, so please just do the simple thing and build the command with the lock held. Will _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu