[re-adding the list]
On 25.08.21 12:20, Bram Hooimeijer wrote:
>> On 25.08.21 11:38, Bram Hooimeijer wrote:
>>>> -----Original Message-----
>>>> From: Jan Kiszka
>>>> Sent: dinsdag 24 augustus 2021 23:13
>>>
>>>> On 02.02.21 17:44, Bram Hooimeijer wrote:
>>>>> The procedures for shrinking and extending the cat_mask of the rool
>>>>> cell affect other, non-root, cells as well, if these cell use the root
>>>>> COS.
>>>>> That is, when cells are configured without cache regions. The code
>>>>> is updated to reflect these changes not only in the root-cell, but
>>>>> in the struct corresponding to these non-root cells as well.
>>>>>
>>>>> Fixes: 3f04eb1753bb ("x86: Introduce Cache Allocation Technology
>>>>> support for Intel CPUs")
>>>>>
>>>>> Signed-off-by: Bram Hooimeijer
>>>>> <bram.hooimei...@prodrive-technologies.com>
>>>>> ---
>>>>> hypervisor/arch/x86/cat.c | 31 +++++++++++++++++++++++++++----
>>>>> 1 file changed, 27 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hypervisor/arch/x86/cat.c b/hypervisor/arch/x86/cat.c
>>>>> index f6719b1e..42fd83d9 100644
>>>>> --- a/hypervisor/arch/x86/cat.c
>>>>> +++ b/hypervisor/arch/x86/cat.c
>>>>> @@ -60,6 +60,13 @@ retry:
>>>>> return cos;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * Merge available bits in the CBM back to root by modifying the
>>>>> +cat_mask of
>>>>> + * the root.
>>>>> + *
>>>>> + * It is the callers responsibility to call
>>>>> +cat_update_cell(&root_cell), and
>>>>> + * to modify the cat_mask of the non-root cells sharing the root's COS.
>>>>> + */
>>>>> static bool merge_freed_mask_to_root(void) {
>>>>> bool updated = false;
>>>>> @@ -86,6 +93,7 @@ static bool shrink_root_cell_mask(u64 cell_mask) {
>>>>> unsigned int lo_mask_start, lo_mask_len;
>>>>> u64 lo_mask;
>>>>> + struct cell *cell;
>>>>>
>>>>> if ((root_cell.arch.cat_mask & ~cell_mask) == 0) {
>>>>> /*
>>>>> @@ -125,8 +133,17 @@ static bool shrink_root_cell_mask(u64
>> cell_mask)
>>>>> }
>>>>> }
>>>>>
>>>>> - printk("CAT: Shrunk root cell bitmask to %08llx\n",
>>>>> - root_cell.arch.cat_mask);
>>>>> + /* Cells using the root COS are also affected by shrinking. */
>>>>> + printk("CAT: Set COS %d bitmask to %08llx for root cell",
>>>>> + root_cell.arch.cos, root_cell.arch.cat_mask);
>>>>> + for_each_non_root_cell(cell)
>>>>> + if (cell->arch.cos == root_cell.arch.cos) {
>>>>> + cell->arch.cat_mask = root_cell.arch.cat_mask;
>>>>> + printk(", %s", cell->config->name);
>>>>> + }
>>>>> + printk("\n");
>>>>> + /* However, updating the bitmask once suffices. This can be done
>>>>> + * during code execution, no suspense required. (SDM
>>>>> + 17.19.6.3) */
>>>>> cat_update_cell(&root_cell);
>>>>>
>>>>> /* Drop this mask from the freed mask in case it was queued
>>>>> there. */ @@ -201,8 +218,14 @@ static void cat_cell_exit(struct cell
>> *cell)
>>>>> freed_mask |= cell->arch.cat_mask & orig_root_mask;
>>>>>
>>>>> if (merge_freed_mask_to_root()) {
>>>>> - printk("CAT: Extended root cell bitmask to %08llx\n",
>>>>> - root_cell.arch.cat_mask);
>>>>> + printk("CAT: Extended COS %d bitmask to %08llx for root
>>>>> cell",
>>>>> + root_cell.arch.cos, root_cell.arch.cat_mask);
>>>>> + for_each_non_root_cell(oth_cell)
>>>>> + if (oth_cell->arch.cos == root_cell.arch.cos) {
>>>>> + oth_cell->arch.cat_mask =
>>>>> root_cell.arch.cat_mask;
>>>>> + printk(", %s", cell->config->name);
>>>>> + }
>>>>> + printk("\n");
>>>>> cat_update_cell(&root_cell);
>>>>> }
>>>>> }
>>>>>
>>>>
>>>> Valid point that arch.cat_mask for the sharing cell gets out of sync.
>>>> But what is the practical impact? We don't run cat_update_cell() for
>>>> sharing cells, and cat_cell_exit() does nothing in that case. This is
>>>> first of all to understand the impact of the issue.
>>>
>>> Fair point. I am not 100% into the details anymore, but I guess you
>>> are right that this does not have a practical impact. Of course, it
>>> can get a practical impact in the future if someone decides to use the
>>> mask for something, so I thought it would be good to fix it regardless.
>>>
>>>>
>>>> If there is impact, I'm considering to use (also) a mask pointer so
>>>> that there is no need to walk all cells on root cell updates.
>>>
>>> Just curious to get a better understanding of jailhouse: would these
>>> walks lead to a performance hit? The other cells would not be
>>> preempted, right? So it is just a list linear in the number of cells?
>>
>> Nope, my concern is just code size growth.
>
> I see, will take that into account in the future.
>
>>
>> But while we may save a walk for updating the masks via indirection, we
>> conceptually still need walks for sharing cells to call cat_update_cell on
>> them.
>> And that is probably the real issue with sharing as it implies stopping that
>> cell
>> which is surely undesired.
>
> I agree that this is undesired. It goes against both Jailhouse's philosophy
> as well
> as the design intent for CAT, which is socket-shared indeed.
>
>>
>> We could only support "silent" sharing, i.e. between cells that have cores on
>> the same socket or otherwise withing the same CAT scope so that the
>> update done for the root cell CPUs automatically updates masks for the
>> other non-root CPUs as well (because the MSRs are shared).
>
> This would indeed be the logical, and expected functioning. I see that this
> would
> require Jailhouse to become socket-aware, which might make matters more
> complex.
Well, the refactoring I'm doing have to goal to at least statically (at
configuration time) model CAT topologies. Those follow the last-level
cache topology, e.g. 2-cores share a CAT domain on a quad-core Apollo
Lake SoC. Or each socket has its own domain as it has its L3 cache on
Xeon processors. That's why I'm adding a cache ID field so that CPUs can
refer to the same cache instance.
>
>> It's a complex beast, this CAT, despite its minimal interface.
>
> Let me look through your other mails and see if I can think of some ways to
> make it simpler.
>
And I'm trying to bring my refactoring in a sharable form. They seem to
work on L2, wasn't able to retest on L3 setups again, though.
Jan
--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to jailhouse-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/b33ef7aa-0210-1aab-c1f8-5194d28936f9%40siemens.com.
