| Message ID | 154014429795.2869.7870954544925695802.sendpatchset@octo (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [PATCH/RFC] iommu/ipmmu-vmsa: R-Car M3-N/V3H/E3 AVB whitelist prototype | expand |
Hi Magnus, Thanks for your patch! On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote: > From: Magnus Damm <damm@opensource.se> > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3. > > Not for upstream merge. > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > Applies on top of renesas-devel-20181019-v4.19-rc8 > > drivers/iommu/ipmmu-vmsa.c | 4 ++++ > 1 file changed, 4 insertions(+) > > --- 0001/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2018-10-22 02:46:30.139880557 +0900 > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st > > static bool ipmmu_slave_whitelist(struct device *dev) > { > + /* R-Car M3-N/V3H/E3 Ethernet-AVB */ > + if (!strcmp(dev_name(dev), "e6800000.ethernet")) > + return true; I'm afraid the whitelisting doesn't work that way: with the above check, it will be enabled on all R-Car Gen3 SoCs. > + > /* By default, do not allow use of IPMMU */ > return false; > } Gr{oetje,eeting}s, Geert
Hi Magnus-san, Geert-san, > From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM > > Hi Magnus, > > Thanks for your patch! > > On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote: > > From: Magnus Damm <damm@opensource.se> > > > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3. > > > > Not for upstream merge. > > > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > > --- > > > > Applies on top of renesas-devel-20181019-v4.19-rc8 > > > > drivers/iommu/ipmmu-vmsa.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > --- 0001/drivers/iommu/ipmmu-vmsa.c > > +++ work/drivers/iommu/ipmmu-vmsa.c 2018-10-22 02:46:30.139880557 +0900 > > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st > > > > static bool ipmmu_slave_whitelist(struct device *dev) > > { > > + /* R-Car M3-N/V3H/E3 Ethernet-AVB */ > > + if (!strcmp(dev_name(dev), "e6800000.ethernet")) > > + return true; > > I'm afraid the whitelisting doesn't work that way: with the above check, it will > be enabled on all R-Car Gen3 SoCs. I agree with Geert-san. So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following? I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*. --- diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index b98a031..7a528b8 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) } static const struct soc_device_attribute soc_rcar_gen3[] = { - { .soc_id = "r8a7795", }, - { .soc_id = "r8a7796", }, + { .soc_id = "r8a7795", .revision = "ES3.*" }, { .soc_id = "r8a77965", }, { .soc_id = "r8a77970", }, + { .soc_id = "r8a77990", }, { .soc_id = "r8a77995", }, { /* sentinel */ } }; Best regards, Yoshihiro Shimoda > > + > > /* By default, do not allow use of IPMMU */ > > return false; > > } > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Shimoda-san, On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM > > On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote: > > > From: Magnus Damm <damm@opensource.se> > > > > > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3. > > > > > > Not for upstream merge. > > > > > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > > > --- > > > > > > Applies on top of renesas-devel-20181019-v4.19-rc8 > > > > > > drivers/iommu/ipmmu-vmsa.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > --- 0001/drivers/iommu/ipmmu-vmsa.c > > > +++ work/drivers/iommu/ipmmu-vmsa.c 2018-10-22 02:46:30.139880557 +0900 > > > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st > > > > > > static bool ipmmu_slave_whitelist(struct device *dev) > > > { > > > + /* R-Car M3-N/V3H/E3 Ethernet-AVB */ > > > + if (!strcmp(dev_name(dev), "e6800000.ethernet")) > > > + return true; > > > > I'm afraid the whitelisting doesn't work that way: with the above check, it will > > be enabled on all R-Car Gen3 SoCs. > > I agree with Geert-san. > So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following? > I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*. Thanks for the information! > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) > } > > static const struct soc_device_attribute soc_rcar_gen3[] = { > - { .soc_id = "r8a7795", }, > - { .soc_id = "r8a7796", }, > + { .soc_id = "r8a7795", .revision = "ES3.*" }, > { .soc_id = "r8a77965", }, > { .soc_id = "r8a77970", }, > + { .soc_id = "r8a77990", }, > { .soc_id = "r8a77995", }, > { /* sentinel */ } > }; Given the above, I think the time is ripe to convert this from a whitelist to a blacklist? Gr{oetje,eeting}s, Geert
Hi Geert-san, Thank you for the comment! > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM > > Hi Shimoda-san, > > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > --- a/drivers/iommu/ipmmu-vmsa.c > > +++ b/drivers/iommu/ipmmu-vmsa.c > > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) > > } > > > > static const struct soc_device_attribute soc_rcar_gen3[] = { > > - { .soc_id = "r8a7795", }, > > - { .soc_id = "r8a7796", }, > > + { .soc_id = "r8a7795", .revision = "ES3.*" }, > > { .soc_id = "r8a77965", }, > > { .soc_id = "r8a77970", }, > > + { .soc_id = "r8a77990", }, > > { .soc_id = "r8a77995", }, > > { /* sentinel */ } > > }; > > Given the above, I think the time is ripe to convert this from a whitelist to a > blacklist? About the SoCs, I think so. (I updated example patch below and it seems better :) ) However, I would like to keep ipmmu_slave_whitelist to avoid any trouble for now... --- diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index b98a031..ab24128 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -757,12 +757,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) return false; } -static const struct soc_device_attribute soc_rcar_gen3[] = { - { .soc_id = "r8a7795", }, - { .soc_id = "r8a7796", }, - { .soc_id = "r8a77965", }, - { .soc_id = "r8a77970", }, - { .soc_id = "r8a77995", }, +static const struct soc_device_attribute soc_rcar_gen3_blacklist[] = { + { .soc_id = "r8a7795", .revision = "ES1.*" }, + { .soc_id = "r8a7795", .revision = "ES2.*" }, + { .soc_id = "r8a7796", .revision = "ES1.*" }, { /* sentinel */ } }; @@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev, struct of_phandle_args *spec) { /* For R-Car Gen3 use a white list to opt-in slave devices */ - if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev)) + if (!soc_device_match(soc_rcar_gen3_blacklist) && + !ipmmu_slave_whitelist(dev)) return -ENODEV; iommu_fwspec_add_ids(dev, spec->args, 1); --- Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Mon, Nov 12, 2018 at 09:18:41AM +0100, Geert Uytterhoeven wrote: > Hi Shimoda-san, > > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Geert Uytterhoeven, Sent: Sunday, October 28, 2018 10:33 PM > > > On Sun, Oct 21, 2018 at 7:56 PM Magnus Damm <magnus.damm@gmail.com> wrote: > > > > From: Magnus Damm <damm@opensource.se> > > > > > > > > For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3. > > > > > > > > Not for upstream merge. > > > > > > > > Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> > > > > --- > > > > > > > > Applies on top of renesas-devel-20181019-v4.19-rc8 > > > > > > > > drivers/iommu/ipmmu-vmsa.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > --- 0001/drivers/iommu/ipmmu-vmsa.c > > > > +++ work/drivers/iommu/ipmmu-vmsa.c 2018-10-22 02:46:30.139880557 +0900 > > > > @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st > > > > > > > > static bool ipmmu_slave_whitelist(struct device *dev) > > > > { > > > > + /* R-Car M3-N/V3H/E3 Ethernet-AVB */ > > > > + if (!strcmp(dev_name(dev), "e6800000.ethernet")) > > > > + return true; > > > > > > I'm afraid the whitelisting doesn't work that way: with the above check, it will > > > be enabled on all R-Car Gen3 SoCs. > > > > I agree with Geert-san. > > So, how about adding .revision into the soc_rcar_gen3 like a whitelist of SoCs first as following? > > I believe almost all R-Car Gen3 SoCs can use IPMMU safety, except H3 ES2.0 or older and M3-W ES1.*. > > Thanks for the information! > > > --- a/drivers/iommu/ipmmu-vmsa.c > > +++ b/drivers/iommu/ipmmu-vmsa.c > > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) > > } > > > > static const struct soc_device_attribute soc_rcar_gen3[] = { > > - { .soc_id = "r8a7795", }, > > - { .soc_id = "r8a7796", }, > > + { .soc_id = "r8a7795", .revision = "ES3.*" }, > > { .soc_id = "r8a77965", }, > > { .soc_id = "r8a77970", }, > > + { .soc_id = "r8a77990", }, > > { .soc_id = "r8a77995", }, > > { /* sentinel */ } > > }; > > Given the above, I think the time is ripe to convert this from a whitelist to a > blacklist? My understanding is that the motivation for the whitelist was to allow us to control enabling this device on a per-SOC basis in a very deliberate maner to avoid enabling non-working hardware/firmware/driver combinations. In moving to a whitelist I believe we would be saying that the risk of such non-working combinations has subsided and can satisfactorily be managed by a whitelist. Are we comfortable in saying that?
Hi Shimoda-san, On Mon, Nov 12, 2018 at 9:53 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM > > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda > > <yoshihiro.shimoda.uh@renesas.com> wrote: > <snip> > > > --- a/drivers/iommu/ipmmu-vmsa.c > > > +++ b/drivers/iommu/ipmmu-vmsa.c > > > @@ -758,10 +758,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) > > > } > > > > > > static const struct soc_device_attribute soc_rcar_gen3[] = { > > > - { .soc_id = "r8a7795", }, > > > - { .soc_id = "r8a7796", }, > > > + { .soc_id = "r8a7795", .revision = "ES3.*" }, > > > { .soc_id = "r8a77965", }, > > > { .soc_id = "r8a77970", }, > > > + { .soc_id = "r8a77990", }, > > > { .soc_id = "r8a77995", }, > > > { /* sentinel */ } > > > }; > > > > Given the above, I think the time is ripe to convert this from a whitelist to a > > blacklist? > > About the SoCs, I think so. (I updated example patch below and it seems better :) ) > However, I would like to keep ipmmu_slave_whitelist to avoid any trouble for now... OK, IC. > --- a/drivers/iommu/ipmmu-vmsa.c > +++ b/drivers/iommu/ipmmu-vmsa.c > @@ -757,12 +757,10 @@ static bool ipmmu_slave_whitelist(struct device *dev) > return false; > } > > -static const struct soc_device_attribute soc_rcar_gen3[] = { > - { .soc_id = "r8a7795", }, > - { .soc_id = "r8a7796", }, > - { .soc_id = "r8a77965", }, > - { .soc_id = "r8a77970", }, > - { .soc_id = "r8a77995", }, > +static const struct soc_device_attribute soc_rcar_gen3_blacklist[] = { > + { .soc_id = "r8a7795", .revision = "ES1.*" }, > + { .soc_id = "r8a7795", .revision = "ES2.*" }, I think you can combine both lines using "ES[12].*", too. But it may be considered less readable, and not greppable for e.g. "ES1". > + { .soc_id = "r8a7796", .revision = "ES1.*" }, > { /* sentinel */ } > }; > > @@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev, > struct of_phandle_args *spec) > { > /* For R-Car Gen3 use a white list to opt-in slave devices */ > - if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev)) > + if (!soc_device_match(soc_rcar_gen3_blacklist) && > + !ipmmu_slave_whitelist(dev)) > return -ENODEV; Ah, this has the side effect of applying ipmmu_slave_whitelist() on R-Car Gen2, too, which is probably not what we want? Gr{oetje,eeting}s, Geert
Hi Geert-san, > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 6:08 PM > > Hi Shimoda-san, > > On Mon, Nov 12, 2018 at 9:53 AM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > From: Geert Uytterhoeven, Sent: Monday, November 12, 2018 5:19 PM > > > On Mon, Nov 12, 2018 at 8:24 AM Yoshihiro Shimoda > > > <yoshihiro.shimoda.uh@renesas.com> wrote: <snip> > > @@ -770,7 +768,8 @@ static int ipmmu_of_xlate(struct device *dev, > > struct of_phandle_args *spec) > > { > > /* For R-Car Gen3 use a white list to opt-in slave devices */ > > - if (soc_device_match(soc_rcar_gen3) && !ipmmu_slave_whitelist(dev)) > > + if (!soc_device_match(soc_rcar_gen3_blacklist) && > > + !ipmmu_slave_whitelist(dev)) > > return -ENODEV; > > Ah, this has the side effect of applying ipmmu_slave_whitelist() on R-Car > Gen2, too, which is probably not what we want? Oops! You're correct. The current code behavior is: - Gen2 doesn't use any whitelist. - Gen3 uses the whitelist. If we apply this my patch above: - Gen2 and Gen3 use the whitelist anyway. This is not our expected, so I think we keep to use the current style. # Note that my first suggestion [1] also broke the code because # r8a7795 ES2.0 or older and r8a7796 don't use the whitelist... # # [1] https://www.spinics.net/lists/linux-renesas-soc/msg35048.html Best regards, Yoshihiro Shimoda > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
--- 0001/drivers/iommu/ipmmu-vmsa.c +++ work/drivers/iommu/ipmmu-vmsa.c 2018-10-22 02:46:30.139880557 +0900 @@ -756,6 +756,10 @@ static int ipmmu_init_platform_device(st static bool ipmmu_slave_whitelist(struct device *dev) { + /* R-Car M3-N/V3H/E3 Ethernet-AVB */ + if (!strcmp(dev_name(dev), "e6800000.ethernet")) + return true; + /* By default, do not allow use of IPMMU */ return false; }
From: Magnus Damm <damm@opensource.se> For testing purpose enable IPMMU for Ethernet-AVB on R-Car M3-N/V3H/E3. Not for upstream merge. Not-Yet-Signed-off-by: Magnus Damm <damm@opensource.se> --- Applies on top of renesas-devel-20181019-v4.19-rc8 drivers/iommu/ipmmu-vmsa.c | 4 ++++ 1 file changed, 4 insertions(+)