Re: [PATCH v5 18/31] KVM: s390: gmap: Move storage key and CMMA code to kvm/s390

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] CMMA and storage key function declarations were not fully extracted from generic gmap headers.
--

--- Patch [18]: [PATCH v5 18/31] KVM: s390: gmap: Move storage key and CMMA code to kvm/s390 ---
commit bde63a0d30be0c82cba34cb31a22562bc02b69e0
Author: Steffen Eiden <[email protected]>
Subject: KVM: s390: gmap: Move storage key and CMMA code to kvm/s390

This commit extracts storage key and CMMA functionality from kvm/gmap to
kvm/s390. This enables other KVM implementations to use gmap without
implementing s390-specific storage key or CMMA handling.

> diff --git a/arch/s390/kvm/gmap/dat.h b/arch/s390/kvm/gmap/dat.h
> --- a/arch/s390/kvm/gmap/dat.h
> +++ b/arch/s390/kvm/gmap/dat.h
> @@ -532,13 +532,6 @@ int dat_entry_walk(struct kvm_s390_mmu_cache *mc, gfn_t gfn, union asce asce, in
>  void dat_free_level(struct crst_table *table, bool owns_ptes);
>  struct crst_table *dat_alloc_crst_sleepable(unsigned long init);
>  int dat_set_asce_limit(struct kvm_s390_mmu_cache *mc, union asce *asce, int newtype);
> -int dat_get_storage_key(union asce asce, gfn_t gfn, union skey *skey);
> -int dat_set_storage_key(struct kvm_s390_mmu_cache *mc, union asce asce, gfn_t gfn,
> -			union skey skey, bool nq);
> -int dat_cond_set_storage_key(struct kvm_s390_mmu_cache *mmc, union asce asce, gfn_t gfn,
> -			     union skey skey, union skey *oldkey, bool nq, bool mr, bool mc);
> -int dat_reset_reference_bit(union asce asce, gfn_t gfn);
> -long dat_reset_skeys(union asce asce, gfn_t start);
>  
>  unsigned long dat_get_ptval(struct page_table *table, struct ptval_param param);
>  void dat_set_ptval(struct page_table *table, struct ptval_param param, unsigned long val);

[ ... ]

>  int dat_set_prefix_notif_bit(union asce asce, gfn_t gfn);
>  bool dat_test_age_gfn(union asce asce, gfn_t start, gfn_t end);
>  
>  int dat_perform_essa(union asce asce, gfn_t gfn, int orc, union essa_state *state, bool *dirty);

[Severity: Low]
Does this commit fully extract CMMA functionality from the generic headers?

It appears that while the implementations were moved to kvm/s390, several CMMA
function declarations like dat_perform_essa() and dat_reset_cmma() were left
behind in arch/s390/kvm/gmap/dat.h.

Since the stated goal is to decouple these subsystems so other KVM
implementations can use gmap without s390-specific handling, shouldn't these
prototypes also be moved out of the generic headers alongside their newly
relocated implementations?

> diff --git a/arch/s390/kvm/gmap/gmap.h b/arch/s390/kvm/gmap/gmap.h
> --- a/arch/s390/kvm/gmap/gmap.h
> +++ b/arch/s390/kvm/gmap/gmap.h
> @@ -100,10 +100,9 @@ int gmap_ucas_map(struct gmap *gmap, gfn_t p_gfn, gfn_t c_gfn, unsigned long cou
>  void gmap_ucas_unmap(struct gmap *gmap, gfn_t c_gfn, unsigned long count);
>  int gmap_enable_skeys(struct gmap *gmap);
>  int gmap_pv_destroy_range(struct gmap *gmap, gfn_t start, gfn_t end, bool interruptible);
>  int gmap_insert_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn,
>  		     gfn_t r_gfn, int level);
>  int gmap_protect_rmap(struct kvm_s390_mmu_cache *mc, struct gmap *sg, gfn_t p_gfn, gfn_t r_gfn,
>  		      kvm_pfn_t pfn, int level, bool wr);
> -void _gmap_set_cmma_all(struct gmap *gmap, bool dirty);
>  void _gmap_handle_vsie_unshadow_event(struct gmap *parent, gfn_t gfn);

[Severity: Low]
Are there duplicate or leftover declarations here?

While gmap_enable_skeys() was correctly duplicated into
arch/s390/kvm/s390/s390.h where it is now implemented, it seems the original
prototype was not removed from arch/s390/kvm/gmap/gmap.h.

Leaving it here continues to unnecessarily expose s390-specific storage key
operations through the generic gmap headers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18