Hi Jonathan,
On 28/07/2023 15:15, Jonathan Cameron wrote:
On Fri, 28 Jul 2023 14:39:15 +0100 Jonathan Cameron via Linaro-open-discussions linaro-open-discussions@op-lists.linaro.org wrote:
Speaking of which... I got some unexpected write values in the bandwidth bitmap registers. I had a small bit map - 16 entries and got 0x1ffff written wd is coming from CPBM_WD which (unlike many register fields in the spec) is not offset by 1.
Bug looks to be an off by one.
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers...
static void mpam_reset_msc_bitmap(struct mpam_msc *msc, u16 reg, u16 wd) { u32 bm = ~0; int i;
lockdep_assert_held(&msc->part_sel_lock);
/* * Write all but the last full-32bit-word. * The number of portions is 1<->wd, start at 1 instead of 0. */ for (i = 1; i < wd / 32; i++, reg += sizeof(bm))
for (i = 1; i < (wd + 31) /32; i++, reg += sizeof(bm))
DIV_ROUND_UP()
as you want to round up rather than down here.
Rounding down was so this always stops early, but starting at 1 covers that ... which just makes this code more confusing.
Testing with a width of 48 we want this to run once and the below to pick up last 16 bits.
You're winning here - I'm still waiting for the FVP to boot, and it won't emulate CPOR bitmaps larger than 32...
__mpam_write_reg(msc, reg, bm);/* and the last partial 32bit word */ bm = GENMASK(wd % 32, 0);
wd % 32 - 1
I think.
Blerg. This needs to produce '31' when wd is a multiple of 32, so yes.
That gave me enough of a headache that this thing now looks like this: ----------%<---------- /* * Write ~0 to all but the last 32bit-word, which may * have fewer bits... */ num_words = DIV_ROUND_UP(wd, 32); for (i = 0; i < num_words - 1; i++, reg += sizeof(bm)) __mpam_write_reg(msc, reg, bm);
/* * ....and then the last (maybe) partial 32bit word. When wd is a * multiple of 32, msb should be 31 to write a full 32bit word. */ msb = (wd - 1) % 32; bm = GENMASK(msb , 0); if (bm) __mpam_write_reg(msc, reg, bm); ----------%<----------
As this is an interaction I've not explored, beware that resctrl only supports 32bit bitmaps. Arbitrary size bitmaps would mean invasive changes to resctrl, but bumping it to 64bits is probably easy enough to do.
Other than the above, mostly seems to be working fine beyond a bug in the upstream PPTT parsing code that doesn't affect what you have implemented so far.
There is an infinite loop / deadlock hiding somewhere though in one of the error paths though that I haven't chased down yet. Maybe I'll be lucky and you've fixed it already :)
Oooeer, that sounds new .
There were some false-positive lockdep warnings that I've fixed/replaced-with-comments.
Otherwise the response to the error interrupt isn't fantastic at the moment, resctrl mostly manages to tear itself down, but there is evidently a reference counting problem somewhere as a few of the files live on. I can't find a way of triggering that code on x86, so its not too surprising. I haven't tested that for a while...
I was being lazy on interrupts - will wire that up at some point.
If I get really bored I might even wire up the PPC channel interfaces but don't hold your breath on that one. By coincidence I wired up PCC in QEMU for something else last week so it wouldn't be too tricky - it's just not that high on my list unless you'll find it 'really useful'.
I've not touched that thing, I was waiting for someone to show up with a model to prove the spec had everything that was needed. Last I heard this was likely to get deprecated in favour of PCC type... 3? I'd hold off for now.
If you want to find more bugs, the monitor code and the SMMU stuff are two things I can't test!
I'm about to repost the x86 patches at the bottom of all this, is it useful to CC linuxarm@huawei.com for that? Currently Shameerali and Bobo get CC'd... I'm certain my list is not up-to-date for the folk that care about this.
Thanks!
James