lkml.org 
[lkml]   [2024]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v19 059/130] KVM: x86/tdp_mmu: Don't zap private pages for unsupported cases
    Date
    On Mon, 2024-02-26 at 00:26 -0800, isaku.yamahata@intel.com wrote:
    > +/* Used by mmu notifier via kvm_unmap_gfn_range() */
    >  bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range
    > *range,
    >                                  bool flush)
    >  {
    >         struct kvm_mmu_page *root;
    > +       bool zap_private = false;
    > +
    > +       if (kvm_gfn_shared_mask(kvm)) {
    > +               if (!range->only_private && !range->only_shared)
    > +                       /* attributes change */
    > +                       zap_private = !(range->arg.attributes &
    > +                                       KVM_MEMORY_ATTRIBUTE_PRIVATE);
    > +               else
    > +                       zap_private = range->only_private;
    > +       }
    >  
    >         __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id,
    > false)
    >                 flush = tdp_mmu_zap_leafs(kvm, root, range->start, range->end,
    > -                                         range->may_block, flush);
    > +                                         range->may_block, flush,
    > +                                         zap_private && is_private_sp(root));
    >  



    I'm trying to apply the feedback:
    - Drop MTRR support
    - Changing only_private/shared to exclude_private/shared
    ...and update the log accordingly. These changes are all intersect in this
    function and I'm having a hard time trying to justify the resulting logic.

    It seems the point of passing the the attributes is because:
    "However, looking at kvm_mem_attrs_changed() again, I think invoking
    kvm_unmap_gfn_range() from generic KVM code is a mistake and shortsighted.
    Zapping in response to *any* attribute change is very private/shared centric.
    E.g. if/when we extend attributes to provide per-page RWX protections, zapping
    existing SPTEs in response to granting *more* permissions may not be necessary
    or even desirable."
    https://lore.kernel.org/all/ZJX0hk+KpQP0KUyB@google.com/

    But I think shoving the logic for how to handle the attribute changes deep into
    the zapping code is the opposite extreme. It results in this confusing logic
    with the decision on what to zap is spread all around.

    Instead we should have kvm_arch_pre_set_memory_attributes() adjust the range so
    it can tell kvm_unmap_gfn_range() which ranges to zap (private/shared).

    So:
    kvm_vm_set_mem_attributes() - passes attributes
    kvm_arch_pre_set_memory_attributes() - chooses which private/shared alias to 
    zap based on attribute.
    kvm_unmap_gfn_range/kvm_tdp_mmu_unmap_gfn_range - zaps the private/shared alias
    tdp_mmu_zap_leafs() - doesn't care about the root type, just zaps leafs


    This zapping function can then just do the simple thing it's told to do. It ends
    up looking like:

    bool kvm_tdp_mmu_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range,
    bool flush)
    {
    struct kvm_mmu_page *root;
    bool exclude_private = false;
    bool exclude_shared = false;

    if (kvm_gfn_shared_mask(kvm)) {
    exclude_private = range->exclude_private;
    exclude_shared = range->exclude_shared;
    }

    __for_each_tdp_mmu_root_yield_safe(kvm, root, range->slot->as_id,
    false) {
    if (exclude_private && is_private_sp(root))
    continue;
    if (exclude_shared && !is_private_sp(root))
    continue;

    flush = tdp_mmu_zap_leafs(kvm, root, range->start, range->end,
    range->may_block, flush);
    }

    return flush;
    }

    The resulting logic should be the same. Separately, we might be able to simplify
    it further if we change the behavior a bit (lose the kvm_gfn_shared_mask() check
    or the exclude_shared member), but in the meantime this seems a lot easier to
    explain and review for what I think is equivalent behavior.
    \
     
     \ /
      Last update: 2024-05-27 17:55    [W:9.568 / U:0.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site