| Message ID | 6575cd1b9c82f5b28cd29d78308dc23821c9d246.1570628924.git.artem_mygaiev@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minor Coverity fixes | expand |
Hi Artem, On 09/10/2019 15:20, Artem Mygaiev wrote: > Also do not set mark device as SMMU protected when there are no more > SMMU resources left This is a sanity check on the content of the node, not a lack of resource in this case. TBH, the current placement is probably not correct. But I am not convinced the new placement is better. Xen 4.12 and earlier will ignore any failure and continue, so we cannot use this device at all. Indeed, Xen will configure the SMMU to deny any transaction. If you fail to initialize the device, then it will be in an unusable state. In this case, we don't want a domain (including Dom0) to use it at all. This could be achieved by calling dt_device_set_protected. Xen 4.13 will stop booting if any of the SMMU fails to configure (this include Master Device cannot be assigned). So there are no difference there. My preference is to cater for all Xen versions so we can consider the patch for a backport and potentially revert of the Xen 4.13 behavior (i.e. crashing when one IOMMU is not correctly setup). So we would need to move the call at the beginning of the function. > > Coverity-ID: 1381862 > Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> > --- > xen/drivers/passthrough/arm/smmu.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c > index f151b9f5b5..cf42335eed 100644 > --- a/xen/drivers/passthrough/arm/smmu.c > +++ b/xen/drivers/passthrough/arm/smmu.c > @@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > master->of_node = masterspec->np; > master->cfg.num_streamids = masterspec->args_count; > > - /* Xen: Let Xen know that the device is protected by an SMMU */ > - dt_device_set_protected(masterspec->np); > - > for (i = 0; i < master->cfg.num_streamids; ++i) { > u16 streamid = masterspec->args[i]; > > @@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu, > dev_err(dev, > "stream ID for master device %s greater than maximum allowed (%d)\n", > masterspec->np->name, smmu->num_mapping_groups); > + devm_free(master); > return -ERANGE; > } > master->cfg.streamids[i] = streamid; > } > + > + /* Xen: Let Xen know that the device is protected by an SMMU */ > + dt_device_set_protected(masterspec->np); This code is using Linux coding style, so it should be hard tab here. > + > return insert_smmu_master(smmu, master); > } > > Cheers,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index f151b9f5b5..cf42335eed 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device *smmu, master->of_node = masterspec->np; master->cfg.num_streamids = masterspec->args_count; - /* Xen: Let Xen know that the device is protected by an SMMU */ - dt_device_set_protected(masterspec->np); - for (i = 0; i < master->cfg.num_streamids; ++i) { u16 streamid = masterspec->args[i]; @@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu, dev_err(dev, "stream ID for master device %s greater than maximum allowed (%d)\n", masterspec->np->name, smmu->num_mapping_groups); + devm_free(master); return -ERANGE; } master->cfg.streamids[i] = streamid; } + + /* Xen: Let Xen know that the device is protected by an SMMU */ + dt_device_set_protected(masterspec->np); + return insert_smmu_master(smmu, master); }
Also do not set mark device as SMMU protected when there are no more SMMU resources left Coverity-ID: 1381862 Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com> --- xen/drivers/passthrough/arm/smmu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)