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
>>> <[email protected]>
>>> ---
>>> 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.
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.
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).
It's a complex beast, this CAT, despite its minimal interface.
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/jailhouse-dev/bea596bb-88de-ed10-9329-782fd41d81a1%40siemens.com.
