| Message ID | 1421418247-30068-11-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 27/01/15 16:30, Stefano Stabellini wrote: > On Fri, 16 Jan 2015, Julien Grall wrote: >> From: Andreas Herrmann <andreas.herrmann@calxeda.com> >> >> If DT information lists one stream ID twice for the master devices of >> an SMMU this can cause a multi match when stream ID matching is used. >> For stream ID indexing this might trigger an overwrite of an S2CR that >> is already in use. >> >> So better check for duplicates when DT information is parsed. >> >> Taken from the linux ML: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html >> >> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com> >> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Why didn't you just take a more recent version of the Linux smmu driver? The SMMU driver very is recent (see commit in the previous patch)... Just this patch has never reached upstream. Regards,
On 27/01/15 17:02, Stefano Stabellini wrote: > On Tue, 27 Jan 2015, Julien Grall wrote: >> On 27/01/15 16:30, Stefano Stabellini wrote: >>> On Fri, 16 Jan 2015, Julien Grall wrote: >>>> From: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>> >>>> If DT information lists one stream ID twice for the master devices of >>>> an SMMU this can cause a multi match when stream ID matching is used. >>>> For stream ID indexing this might trigger an overwrite of an S2CR that >>>> is already in use. >>>> >>>> So better check for duplicates when DT information is parsed. >>>> >>>> Taken from the linux ML: >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html >>>> >>>> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com> >>>> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> >>> Why didn't you just take a more recent version of the Linux smmu driver? >> >> The SMMU driver very is recent (see commit in the previous patch)... >> Just this patch has never reached upstream. > > That is not good. It might be worth to wait for it to go upstream. The patch was sent one year ago. Just before Calxeda was shutdown. This is a requirement for the following patch. Do you think the other patch should be upstream to Linux before? If so, Calxeda server won't be able to use properly SMMU. Even though the server will never be used, I do all my SMMU development on it. FWIW, we use already use the 2 patches in the current SMMU drivers. Regards,
On 27/01/15 17:44, Ian Campbell wrote: > On Tue, 2015-01-27 at 17:33 +0000, Julien Grall wrote: >> On 27/01/15 17:02, Stefano Stabellini wrote: >>> On Tue, 27 Jan 2015, Julien Grall wrote: >>>> On 27/01/15 16:30, Stefano Stabellini wrote: >>>>> On Fri, 16 Jan 2015, Julien Grall wrote: >>>>>> From: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>>>> >>>>>> If DT information lists one stream ID twice for the master devices of >>>>>> an SMMU this can cause a multi match when stream ID matching is used. >>>>>> For stream ID indexing this might trigger an overwrite of an S2CR that >>>>>> is already in use. >>>>>> >>>>>> So better check for duplicates when DT information is parsed. >>>>>> >>>>>> Taken from the linux ML: >>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html >>>>>> >>>>>> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com> >>>>>> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>> >>>>> Why didn't you just take a more recent version of the Linux smmu driver? >>>> >>>> The SMMU driver very is recent (see commit in the previous patch)... >>>> Just this patch has never reached upstream. >>> >>> That is not good. It might be worth to wait for it to go upstream. >> >> The patch was sent one year ago. Just before Calxeda was shutdown. >> >> This is a requirement for the following patch. Do you think the other >> patch should be upstream to Linux before? > > In general we should be reluctant to diverge over these things, since it > just ends up making things harder for us in the future (e.g. the next > resync or the one after). > > Is the issue being fixed here specific to Calxeda? e.g. is duplicate > stream ids a h/w bug which has yet to be observed elsewhere or is it a > valid configuration per the h/w specs which it just happens only Calxeda > implemented? It's for catching possible error in the device tree. If the ID is duplicated the multi match algo in the following patch may not work correctly. > > Unless it's a h/w bug I don't see why it couldn't go upstream first. > Even if it is it might still stand a chance. Because nobody take care to upstream it (see why below). >> If so, Calxeda server won't be able to use properly SMMU. >> >> Even though the server will never be used, I do all my SMMU development >> on it. > > The fact that Calxeda are no longer around doesn't in itself preclude > getting these patches upstreamed. After all, no one is deleting CX > support and some people do have them (not just us). It's hardly the most > obscure platform supported by Linux... By default Linux is by-passing the stream ID and the device is not using the SMMU. You will have to use either passthrough or manually said : "I want to enforce this device". And then you will see the error. So by default both patches are not necessary. On Xen, we can't by-pass the SMMU by default. At least it's not the way it should work. Regards,
On 28/01/15 10:38, Ian Campbell wrote: > On Tue, 2015-01-27 at 17:51 +0000, Julien Grall wrote: >> On 27/01/15 17:44, Ian Campbell wrote: >>> On Tue, 2015-01-27 at 17:33 +0000, Julien Grall wrote: >>>> On 27/01/15 17:02, Stefano Stabellini wrote: >>>>> On Tue, 27 Jan 2015, Julien Grall wrote: >>>>>> On 27/01/15 16:30, Stefano Stabellini wrote: >>>>>>> On Fri, 16 Jan 2015, Julien Grall wrote: >>>>>>>> From: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>>>>>> >>>>>>>> If DT information lists one stream ID twice for the master devices of >>>>>>>> an SMMU this can cause a multi match when stream ID matching is used. >>>>>>>> For stream ID indexing this might trigger an overwrite of an S2CR that >>>>>>>> is already in use. >>>>>>>> >>>>>>>> So better check for duplicates when DT information is parsed. >>>>>>>> >>>>>>>> Taken from the linux ML: >>>>>>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226099.html >>>>>>>> >>>>>>>> Cc: Andreas Herrmann <herrmann.der.user@googlemail.com> >>>>>>>> Signed-off-by: Andreas Herrmann <andreas.herrmann@calxeda.com> >>>>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>>>> >>>>>>> Why didn't you just take a more recent version of the Linux smmu driver? >>>>>> >>>>>> The SMMU driver very is recent (see commit in the previous patch)... >>>>>> Just this patch has never reached upstream. >>>>> >>>>> That is not good. It might be worth to wait for it to go upstream. >>>> >>>> The patch was sent one year ago. Just before Calxeda was shutdown. >>>> >>>> This is a requirement for the following patch. Do you think the other >>>> patch should be upstream to Linux before? >>> >>> In general we should be reluctant to diverge over these things, since it >>> just ends up making things harder for us in the future (e.g. the next >>> resync or the one after). >>> >>> Is the issue being fixed here specific to Calxeda? e.g. is duplicate >>> stream ids a h/w bug which has yet to be observed elsewhere or is it a >>> valid configuration per the h/w specs which it just happens only Calxeda >>> implemented? >> >> It's for catching possible error in the device tree. > > So Calxeda h/w was buggy and provided a DT with duplicate IDs? Or does > the hardware actually use duplicate IDs? No, it's a safe guard for the algorithm. > Can we fix this up in xen/arch/arm/platforms/midway.c somehow, perhaps > with a new quirk or hook (e.g. ->fixup_dtb) instead of patching the > imported code. Or by doing a post-hoc fixup after we init the smmu, i.e. > out of line from the imported code. > >> If the ID is >> duplicated the multi match algo in the following patch may not work >> correctly. > > Are either of these situations (duplicates in DT or in h/w) actually > improper according to the spec? If not then the multi match algo ought > to cope somehow, even if only by detecting and ignoring. It would not be possible in the h/w. Because one streamid is assigned to specific device. But, the developer made have written by mistake twice the streamid for the device. >>> Unless it's a h/w bug I don't see why it couldn't go upstream first. >>> Even if it is it might still stand a chance. >> >> Because nobody take care to upstream it (see why below). > > I'm afraid I'm none the wiser having read below... > >>>> If so, Calxeda server won't be able to use properly SMMU. >>>> >>>> Even though the server will never be used, I do all my SMMU development >>>> on it. >>> >>> The fact that Calxeda are no longer around doesn't in itself preclude >>> getting these patches upstreamed. After all, no one is deleting CX >>> support and some people do have them (not just us). It's hardly the most >>> obscure platform supported by Linux... >> >> By default Linux is by-passing the stream ID and the device is not using >> the SMMU. >> >> You will have to use either passthrough or manually said : "I want to >> enforce this device". And then you will see the error. So by default >> both patches are not necessary. > > Are you talking about under Xen or under Linux here? I was talking about Linux. > If Linux then the fact that you need to use passthrough or take other > manual action to expose the issue doesn't invalidate the possibility of > upstreaming the fix. This patch is not strictly necessary, I could drop if you don't want it. But the following patch is required to use the SATA with SMMU. It has more stream IDs than stream matching register. So Linux would have a problem when enforcing the device and/or use passthrough for SATA. Honestly, the patches has been for a long time on the mailing list and nobody really care about it for now. So I don't have any plan to spend time on upstreaming patch in Linux and it's not required for our use case. Regards,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 6cd47b7..bfc1069 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -51,6 +51,9 @@ /* Maximum number of stream IDs assigned to a single device */ #define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS +/* Maximum stream ID */ +#define ARM_SMMU_MAX_STREAMID (SZ_64K - 1) + /* Maximum number of context banks per SMMU */ #define ARM_SMMU_MAX_CBS 128 @@ -519,7 +522,8 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, static int register_smmu_master(struct arm_smmu_device *smmu, struct device *dev, - struct of_phandle_args *masterspec) + struct of_phandle_args *masterspec, + unsigned long *smmu_sids) { int i; struct arm_smmu_master *master; @@ -556,6 +560,12 @@ static int register_smmu_master(struct arm_smmu_device *smmu, masterspec->np->name, smmu->num_mapping_groups); return -ERANGE; } + + if (test_and_set_bit(streamid, smmu_sids)) { + dev_err(dev, "duplicate stream ID (%d)\n", streamid); + return -EEXIST; + } + master->cfg.streamids[i] = streamid; } return insert_smmu_master(smmu, master); @@ -1977,6 +1987,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct rb_node *node; struct of_phandle_args masterspec; + unsigned long *smmu_sids; int num_irqs, i, err; smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL); @@ -2035,20 +2046,30 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) if (err) return err; + smmu_sids = kzalloc(BITS_TO_LONGS(ARM_SMMU_MAX_STREAMID) * + sizeof(long), GFP_KERNEL); + if (!smmu_sids) { + dev_err(dev, + "failed to allocate bitmap for stream ID tracking\n"); + return -ENOMEM; + } + i = 0; smmu->masters = RB_ROOT; while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters", "#stream-id-cells", i, &masterspec)) { - err = register_smmu_master(smmu, dev, &masterspec); + err = register_smmu_master(smmu, dev, &masterspec, smmu_sids); if (err) { dev_err(dev, "failed to add master %s\n", masterspec.np->name); + kfree(smmu_sids); goto out_put_masters; } i++; } + kfree(smmu_sids); dev_notice(dev, "registered %d master devices\n", i); parse_driver_options(smmu);