Re: [PATCH v13 03/15] iommu/arm-smmu-v3: Maintain a SID->device structure
Hi Eric, On 2021/2/2 1:19, Auger Eric wrote: > Hi Keqian, > > On 2/1/21 1:26 PM, Keqian Zhu wrote: >> Hi Eric, >> >> On 2020/11/18 19:21, Eric Auger wrote: >>> From: Jean-Philippe Brucker >>> >>> When handling faults from the event or PRI queue, we need to find the >>> struct device associated to a SID. Add a rb_tree to keep track of SIDs. >>> >>> Signed-off-by: Jean-Philippe Brucker >> [...] >> >>> } >>> >>> +static int arm_smmu_insert_master(struct arm_smmu_device *smmu, >>> + struct arm_smmu_master *master) [...] >>> kfree(master); >> >> Thanks, >> Keqian >> > Thank you for the review. Jean will address this issues in his own > series and on my end I will rebase on this latter. > > Best Regards > > Eric > Yeah, and hope this series can be accepted earlier ;-) Thanks, Keqian ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v13 03/15] iommu/arm-smmu-v3: Maintain a SID->device structure
Hi Jean, On 2021/2/1 23:15, Jean-Philippe Brucker wrote: > On Mon, Feb 01, 2021 at 08:26:41PM +0800, Keqian Zhu wrote: >>> +static int arm_smmu_insert_master(struct arm_smmu_device *smmu, >>> + struct arm_smmu_master *master) >>> +{ >>> + int i; >>> + int ret = 0; >>> + struct arm_smmu_stream *new_stream, *cur_stream; >>> + struct rb_node **new_node, *parent_node = NULL; >>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); >>> + >>> + master->streams = kcalloc(fwspec->num_ids, >>> + sizeof(struct arm_smmu_stream), GFP_KERNEL); >>> + if (!master->streams) >>> + return -ENOMEM; >>> + master->num_streams = fwspec->num_ids; >> This is not roll-backed when fail. > > No need, the caller frees master OK. > >>> + >>> + mutex_lock(>streams_mutex); >>> + for (i = 0; i < fwspec->num_ids && !ret; i++) { >> Check ret at here, makes it hard to decide the start index of rollback. >> >> If we fail at here, then start index is (i-2). >> If we fail in the loop, then start index is (i-1). >> > [...] >>> + if (ret) { >>> + for (; i > 0; i--) >> should be (i >= 0)? >> And the start index seems not correct. > > Indeed, this whole bit is wrong. I'll fix it while resending the IOPF > series. > > Thanks, > Jean OK, I am glad it helps. Thanks, Keqian ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v13 03/15] iommu/arm-smmu-v3: Maintain a SID->device structure
Hi Keqian, On 2/1/21 1:26 PM, Keqian Zhu wrote: > Hi Eric, > > On 2020/11/18 19:21, Eric Auger wrote: >> From: Jean-Philippe Brucker >> >> When handling faults from the event or PRI queue, we need to find the >> struct device associated to a SID. Add a rb_tree to keep track of SIDs. >> >> Signed-off-by: Jean-Philippe Brucker > [...] > >> } >> >> +static int arm_smmu_insert_master(struct arm_smmu_device *smmu, >> + struct arm_smmu_master *master) >> +{ >> +int i; >> +int ret = 0; >> +struct arm_smmu_stream *new_stream, *cur_stream; >> +struct rb_node **new_node, *parent_node = NULL; >> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); >> + >> +master->streams = kcalloc(fwspec->num_ids, >> + sizeof(struct arm_smmu_stream), GFP_KERNEL); >> +if (!master->streams) >> +return -ENOMEM; >> +master->num_streams = fwspec->num_ids; > This is not roll-backed when fail. > >> + >> +mutex_lock(>streams_mutex); >> +for (i = 0; i < fwspec->num_ids && !ret; i++) { > Check ret at here, makes it hard to decide the start index of rollback. > > If we fail at here, then start index is (i-2). > If we fail in the loop, then start index is (i-1). > >> +u32 sid = fwspec->ids[i]; >> + >> +new_stream = >streams[i]; >> +new_stream->id = sid; >> +new_stream->master = master; >> + >> +/* >> + * Check the SIDs are in range of the SMMU and our stream table >> + */ >> +if (!arm_smmu_sid_in_range(smmu, sid)) { >> +ret = -ERANGE; >> +break; >> +} >> + >> +/* Ensure l2 strtab is initialised */ >> +if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) { >> +ret = arm_smmu_init_l2_strtab(smmu, sid); >> +if (ret) >> +break; >> +} >> + >> +/* Insert into SID tree */ >> +new_node = &(smmu->streams.rb_node); >> +while (*new_node) { >> +cur_stream = rb_entry(*new_node, struct arm_smmu_stream, >> + node); >> +parent_node = *new_node; >> +if (cur_stream->id > new_stream->id) { >> +new_node = &((*new_node)->rb_left); >> +} else if (cur_stream->id < new_stream->id) { >> +new_node = &((*new_node)->rb_right); >> +} else { >> +dev_warn(master->dev, >> + "stream %u already in tree\n", >> + cur_stream->id); >> +ret = -EINVAL; >> +break; >> +} >> +} >> + >> +if (!ret) { >> +rb_link_node(_stream->node, parent_node, new_node); >> +rb_insert_color(_stream->node, >streams); >> +} >> +} >> + >> +if (ret) { >> +for (; i > 0; i--) > should be (i >= 0)? > And the start index seems not correct. > >> +rb_erase(>streams[i].node, >streams); >> +kfree(master->streams); >> +} >> +mutex_unlock(>streams_mutex); >> + >> +return ret; >> +} >> + >> +static void arm_smmu_remove_master(struct arm_smmu_master *master) >> +{ >> +int i; >> +struct arm_smmu_device *smmu = master->smmu; >> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); >> + >> +if (!smmu || !master->streams) >> +return; >> + >> +mutex_lock(>streams_mutex); >> +for (i = 0; i < fwspec->num_ids; i++) >> +rb_erase(>streams[i].node, >streams); >> +mutex_unlock(>streams_mutex); >> + >> +kfree(master->streams); >> +} >> + >> static struct iommu_ops arm_smmu_ops; >> >> static struct iommu_device *arm_smmu_probe_device(struct device *dev) >> { >> -int i, ret; >> +int ret; >> struct arm_smmu_device *smmu; >> struct arm_smmu_master *master; >> struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); >> @@ -2331,27 +2447,12 @@ static struct iommu_device >> *arm_smmu_probe_device(struct device *dev) >> >> master->dev = dev; >> master->smmu = smmu; >> -master->sids = fwspec->ids; >> -master->num_sids = fwspec->num_ids; >> INIT_LIST_HEAD(>bonds); >> dev_iommu_priv_set(dev, master); >> >> -/* Check the SIDs are in range of the SMMU and our stream table */ >> -for (i = 0; i < master->num_sids; i++) { >> -u32 sid = master->sids[i]; >> - >> -if (!arm_smmu_sid_in_range(smmu, sid)) { >> -ret = -ERANGE; >> -goto err_free_master; >> -} >> - >> -/* Ensure l2 strtab is initialised */ >> -
Re: [PATCH v13 03/15] iommu/arm-smmu-v3: Maintain a SID->device structure
On Mon, Feb 01, 2021 at 08:26:41PM +0800, Keqian Zhu wrote: > > +static int arm_smmu_insert_master(struct arm_smmu_device *smmu, > > + struct arm_smmu_master *master) > > +{ > > + int i; > > + int ret = 0; > > + struct arm_smmu_stream *new_stream, *cur_stream; > > + struct rb_node **new_node, *parent_node = NULL; > > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); > > + > > + master->streams = kcalloc(fwspec->num_ids, > > + sizeof(struct arm_smmu_stream), GFP_KERNEL); > > + if (!master->streams) > > + return -ENOMEM; > > + master->num_streams = fwspec->num_ids; > This is not roll-backed when fail. No need, the caller frees master > > + > > + mutex_lock(>streams_mutex); > > + for (i = 0; i < fwspec->num_ids && !ret; i++) { > Check ret at here, makes it hard to decide the start index of rollback. > > If we fail at here, then start index is (i-2). > If we fail in the loop, then start index is (i-1). > [...] > > + if (ret) { > > + for (; i > 0; i--) > should be (i >= 0)? > And the start index seems not correct. Indeed, this whole bit is wrong. I'll fix it while resending the IOPF series. Thanks, Jean ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v13 03/15] iommu/arm-smmu-v3: Maintain a SID->device structure
Hi Eric, On 2020/11/18 19:21, Eric Auger wrote: > From: Jean-Philippe Brucker > > When handling faults from the event or PRI queue, we need to find the > struct device associated to a SID. Add a rb_tree to keep track of SIDs. > > Signed-off-by: Jean-Philippe Brucker [...] > } > > +static int arm_smmu_insert_master(struct arm_smmu_device *smmu, > + struct arm_smmu_master *master) > +{ > + int i; > + int ret = 0; > + struct arm_smmu_stream *new_stream, *cur_stream; > + struct rb_node **new_node, *parent_node = NULL; > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); > + > + master->streams = kcalloc(fwspec->num_ids, > + sizeof(struct arm_smmu_stream), GFP_KERNEL); > + if (!master->streams) > + return -ENOMEM; > + master->num_streams = fwspec->num_ids; This is not roll-backed when fail. > + > + mutex_lock(>streams_mutex); > + for (i = 0; i < fwspec->num_ids && !ret; i++) { Check ret at here, makes it hard to decide the start index of rollback. If we fail at here, then start index is (i-2). If we fail in the loop, then start index is (i-1). > + u32 sid = fwspec->ids[i]; > + > + new_stream = >streams[i]; > + new_stream->id = sid; > + new_stream->master = master; > + > + /* > + * Check the SIDs are in range of the SMMU and our stream table > + */ > + if (!arm_smmu_sid_in_range(smmu, sid)) { > + ret = -ERANGE; > + break; > + } > + > + /* Ensure l2 strtab is initialised */ > + if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) { > + ret = arm_smmu_init_l2_strtab(smmu, sid); > + if (ret) > + break; > + } > + > + /* Insert into SID tree */ > + new_node = &(smmu->streams.rb_node); > + while (*new_node) { > + cur_stream = rb_entry(*new_node, struct arm_smmu_stream, > + node); > + parent_node = *new_node; > + if (cur_stream->id > new_stream->id) { > + new_node = &((*new_node)->rb_left); > + } else if (cur_stream->id < new_stream->id) { > + new_node = &((*new_node)->rb_right); > + } else { > + dev_warn(master->dev, > + "stream %u already in tree\n", > + cur_stream->id); > + ret = -EINVAL; > + break; > + } > + } > + > + if (!ret) { > + rb_link_node(_stream->node, parent_node, new_node); > + rb_insert_color(_stream->node, >streams); > + } > + } > + > + if (ret) { > + for (; i > 0; i--) should be (i >= 0)? And the start index seems not correct. > + rb_erase(>streams[i].node, >streams); > + kfree(master->streams); > + } > + mutex_unlock(>streams_mutex); > + > + return ret; > +} > + > +static void arm_smmu_remove_master(struct arm_smmu_master *master) > +{ > + int i; > + struct arm_smmu_device *smmu = master->smmu; > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(master->dev); > + > + if (!smmu || !master->streams) > + return; > + > + mutex_lock(>streams_mutex); > + for (i = 0; i < fwspec->num_ids; i++) > + rb_erase(>streams[i].node, >streams); > + mutex_unlock(>streams_mutex); > + > + kfree(master->streams); > +} > + > static struct iommu_ops arm_smmu_ops; > > static struct iommu_device *arm_smmu_probe_device(struct device *dev) > { > - int i, ret; > + int ret; > struct arm_smmu_device *smmu; > struct arm_smmu_master *master; > struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > @@ -2331,27 +2447,12 @@ static struct iommu_device > *arm_smmu_probe_device(struct device *dev) > > master->dev = dev; > master->smmu = smmu; > - master->sids = fwspec->ids; > - master->num_sids = fwspec->num_ids; > INIT_LIST_HEAD(>bonds); > dev_iommu_priv_set(dev, master); > > - /* Check the SIDs are in range of the SMMU and our stream table */ > - for (i = 0; i < master->num_sids; i++) { > - u32 sid = master->sids[i]; > - > - if (!arm_smmu_sid_in_range(smmu, sid)) { > - ret = -ERANGE; > - goto err_free_master; > - } > - > - /* Ensure l2 strtab is initialised */ > - if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) { > - ret = arm_smmu_init_l2_strtab(smmu,