| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<OSZPR01MB87985DC21589F6ABA9C11BE68B122@OSZPR01MB8798.jpnprd01.prod.outlook.com>
Date: Wed, 8 Jan 2025 06:45:02 +0000
From: "Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>
To: 'Dave Martin' <Dave.Martin@....com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, Zeng Heng <zengheng4@...wei.com>,
James Morse <james.morse@....com>
Subject: RE: [RFC PATCH 4/6] arm_mpam: Introduce flexible CLOSID/RMID
translation
Hello Dave,
> Hi,
>
> On Fri, Dec 20, 2024 at 05:01:30AM +0000, Shaopeng Tan (Fujitsu) wrote:
> > Hello Dave,
>
> [...]
>
> > > diff --git a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > b/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > index 30f2caec11d7..0473286ec65a 100644
> > > --- a/drivers/platform/arm64/mpam/mpam_resctrl.c
> > > +++ b/drivers/platform/arm64/mpam/mpam_resctrl.c
>
> [...]
>
> > > @@ -163,35 +175,71 @@ static bool mpam_resctrl_hide_cdp(enum
> > > resctrl_res_level rid)
> > > */
> > > u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored) {
> > > - return mpam_partid_max + 1;
> > > + u32 res = (mpam_partid_max + 1) / partid_per_closid;
> >
> > If there is a remainder, are you going to ignore the remainder PARTIDs?
> > It might be better to notify the user with a warning message.
>
> You are right: the remainder is just thrown away, because the remaining
> PARTIDs would not be sufficient for a whole resctrl control group.
>
> It would be a good idea to print a warning somewhere though; I will try to add
> that.
>
> [...]
>
> > I think it is better to check whether PARTID narrowing feature is
> > supported or not before using this function.
> > If it is checked here, since this function is called multiple times,
> > the message should be printed only once by WARN_ON_ONCE().
> >
> > Also, if the partid_per_closid is greater than (mpam_partid_max + 1),
> > returning "0" will not work properly.
> > To switch to default behavior (PARTID narrowing disabled), should it
> > return mapm_patid_max+ 1)?
>
> The idea is that partid_per_closid is set by some configuration logic that is not
> implemented yet. This will be a bit more complicated than just exposing this
> variable directly as a kernel parameter.
>
> Currently I just implemented a basic range check to prevent the kernel
> parameter being set to stupid values, but I didn't try to make it completely
> robust.
>
>
> In any case, since partid_per_closid defaults to 1, the behavior of this function
> should just match the non-PARTID-Narrowing behaviour unless the user
> requested a different value.
>
> Bearing that in mind, I think that a "bad" value of partid_per_closid should not
> be seen here, when the rest of the implementation is complete.
>
> Does this sound sensible?
>
> It could make sense to have a WARN_ON_ONCE() here anyway, though, since
> this code is not on a fast path.
It sounds sensible.
> > > -u32 resctrl_arch_system_num_rmid_idx(void)
> > > +static void mpam_resctrl_partid_range(u32 closid, enum
> > > +resctrl_conf_type
> > > type,
> > > + const struct rdt_resource *r,
> > > + u16 *min_partid, u16 *max_partid)
> > > {
> > > - u8 closid_shift = fls(mpam_pmg_max);
> > > - u32 num_partid = resctrl_arch_get_num_closid(NULL);
> > > + u16 base_partid = closid;
> > > + u16 span = 1;
> > > +
> > > + if (cdp_enabled) {
> > > + base_partid *= 2;
> > > + if (mpam_resctrl_hide_cdp(r->rid) ||
> > > + type == CDP_NONE)
> > > + span *= 2;
> > > + }
> > >
> > > - return num_partid << closid_shift;
> > > + *min_partid = base_partid * partid_per_closid;
> > > + if (max_partid)
> > > + *max_partid = *min_partid + (span * partid_per_closid - 1);
> > > }
> >
> > I think the min_partid/max_partid is different depending on the type
> > (CDP_DATA or CDP_CODE).
>
> You're right! Looking at my fixup patches from the end of December, it looks
> like I already encountered this problem and wrote a workaround, but I didn't
> post it at the time.
>
> [...]
>
> > > @@ -1190,22 +1219,13 @@ int resctrl_arch_update_one(struct
> > > rdt_resource *r, struct rdt_ctrl_domain *d,
> > > return -EINVAL;
> > > }
> > >
> > > - /*
> > > - * When CDP is enabled, but the resource doesn't support it, we need
> > > to
> > > - * apply the same configuration to the other partid.
> > > - */
> > > - if (mpam_resctrl_hide_cdp(r->rid)) {
> > > - partid = resctrl_get_config_index(closid, CDP_CODE);
> > > + for (partid = min_partid; partid <= max_partid; partid++) {
> > > err = mpam_apply_config(dom->comp, partid, &cfg);
> > > if (err)
> > > - return err;
> > > -
> > > - partid = resctrl_get_config_index(closid, CDP_DATA);
> > > - return mpam_apply_config(dom->comp, partid, &cfg);
> > > -
> > > - } else {
> > > - return mpam_apply_config(dom->comp, partid, &cfg);
> > > + break;
> > > }
> > > +
> > > + return err;
> > > }
> >
> > For a mixture of MSCs system, MSCs that do not support PARTID
> > narrowing and support Maximum Partition feature may not work properly.
>
> This is true.
>
> For this series, we just trust that the user understands what they are doing, but
> the complete implementation will need additional checks.
>
> My plan was to do the appropriate checks when the driver starts up, to decide
> whether it is safe to set partid_per_closid to a value different from 1.
>
> Once we get to this code, the decision has already been taken and so the value
> of partid_per_closid (and the resulting mapping of resctrl groups onto PARTIDs)
> can be trusted as being appropriate for the hardware.
>
> Can you see any problems with this approach?
This approach sounds sensible.
>
> [...]
>
> > I know this is not a complete implementation.
> > Mapping relationship between intPARTIDs and reqPARTIDs need to be
> > written to register MPAMCFG_INTPARTID/MPAMCFG_PART_SEL.
> >
> > Best regards,
> > Shaopeng TAN
>
> Agreed. I have some prototype code for that, but I need to split up the patches
> and clean it up before posting it.
>
> Thanks for the review.
I will review the rest of the prototype code when it is posted.
Best regards,
Shaopeng TAN
Powered by blists - more mailing lists