Am 20.12.23 um 11:39 schrieb David Hildenbrand:
On 20.12.23 08:34, Christian Borntraeger wrote:Right now it is possible to see gmap->private being zero in kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the fact that we add gmap->private == kvm after creation: static int acquire_gmap_shadow(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) { [...] gmap = gmap_shadow(vcpu->arch.gmap, asce, edat); if (IS_ERR(gmap)) return PTR_ERR(gmap); gmap->private = vcpu->kvm; Instead of tracking kvm in the shadow gmap, simply use the parent one. Cc: David Hildenbrand <david@xxxxxxxxxx> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx> --- arch/s390/kvm/vsie.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c index 8207a892bbe2..6b06d8ec41b5 100644 --- a/arch/s390/kvm/vsie.c +++ b/arch/s390/kvm/vsie.c @@ -579,14 +579,17 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start, unsigned long end) { - struct kvm *kvm = gmap->private; struct vsie_page *cur; unsigned long prefix; struct page *page; + struct kvm *kvm; int i; if (!gmap_is_shadow(gmap)) return; + + kvm = gmap->parent->private; + /* * Only new shadow blocks are added to the list during runtime, * therefore we can safely reference them all the time. @@ -1220,7 +1223,6 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu, gmap = gmap_shadow(vcpu->arch.gmap, asce, edat); if (IS_ERR(gmap)) return PTR_ERR(gmap); - gmap->private = vcpu->kvm; vcpu->kvm->stat.gmap_shadow_create++; WRITE_ONCE(vsie_page->gmap, gmap); return 0;Why not let gmap_shadow handle it? Simply clone the parent private field. diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 6f96b5a71c63..e083fade7a5d 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -1691,6 +1691,7 @@ struct gmap *gmap_shadow(struct gmap *parent, unsigned long asce, return ERR_PTR(-ENOMEM); new->mm = parent->mm; new->parent = gmap_get(parent); + new->private = patent->private; new->orig_asce = asce; new->edat_level = edat_level; new->initialized = false; Or am I missing something?
That would work as well. I discussed several alternatives with Janosch. The only thing that bothers me is that the owner should define private. So an alternative would be to have a parameter for gmap_shadow. On the other hand I like the simplicity of this patch. (we need to get rid of the 2nd assignment in acquire_gmap_shadow to make it complete. So I can spin a v2 with this variant if Janosch is ok with it as well.
