On 21/10/2020 16:39, Andrew Cooper wrote:
>>> @@ -4051,27 +4057,28 @@ long do_mmu_update(
>>> break;
>>> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>>> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>>> - if ( !rc && pt_owner->arch.pv.xpti )
>>> + /* Paging structure maybe changed. Flush linear
>>> range. */
>>> + if ( !rc )
>>> {
>>> - bool local_in_use = false;
>>> + bool local_in_use = mfn_eq(
>>> + pagetable_get_mfn(curr->arch.guest_table),
>>> mfn);
>>>
>>> - if (
>>> mfn_eq(pagetable_get_mfn(curr->arch.guest_table),
>>> - mfn) )
>>> - {
>>> - local_in_use = true;
>>> - get_cpu_info()->root_pgt_changed = true;
>>> - }
>>> + flush_flags_all |= FLUSH_TLB;
>>> +
>>> + if ( local_in_use )
>>> + flush_flags_local |= FLUSH_TLB |
>>> FLUSH_ROOT_PGTBL;
>> Aiui here (and in the code consuming the flags) you build upon
>> flush_flags_local, when not zero, always being a superset of
>> flush_flags_all. I think this is a trap to fall into when later
>> wanting to change this code, but as per below this won't hold
>> anymore anyway, I think. Hence here I think you want to set
>> FLUSH_TLB unconditionally, and above for L3 and L2 you want to
>> set it in both variables. Or, if I'm wrong below, a comment to
>> that effect may help people avoid falling into this trap.
>>
>> An alternative would be to have
>>
>> flush_flags_local |= (flush_flags_all & FLUSH_TLB);
>>
>> before doing the actual flush.
Also, what I forgot to say in the previous reply, this is still buggy if
hypothetically speaking FLUSH_CACHE had managed to be accumulated in
flush_flags_all.
You cannot have general accumulation logic, a special case for local,
and any catch-all logic like that, because the only correct way to do
the catch-all logic will clobber the special case for local.
~Andrew
