Re: [PATCH v1 2/3] KVM: s390: add stat counter for shadow gmap events

Nico Boehr
2023-05-09T14:53:27Z

Quoting Claudio Imbrenda (2023-05-09 13:43:51)
[...]
> > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > index 3eb85f254881..8348a0095f3a 100644
> > --- a/arch/s390/kvm/gaccess.c
> > +++ b/arch/s390/kvm/gaccess.c
> > @@ -1382,6 +1382,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> > unsigned long *pgt, int *dat_protection,
> > int *fake)
> > {
> > + struct kvm *kvm;
> > struct gmap *parent;
> > union asce asce;
> > union vaddress vaddr;
> > @@ -1390,6 +1391,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> >
> > *fake = 0;
> > *dat_protection = 0;
> > + kvm = sg->private;
> > parent = sg->parent;
> > vaddr.addr = saddr;
> > asce.val = sg->orig_asce;
> > @@ -1450,6 +1452,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> > rc = gmap_shadow_r2t(sg, saddr, rfte.val, *fake);
> > if (rc)
> > return rc;
> > + kvm->stat.gmap_shadow_r2++;
> > }
> > fallthrough;
> > case ASCE_TYPE_REGION2: {
> > @@ -1478,6 +1481,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> > rc = gmap_shadow_r3t(sg, saddr, rste.val, *fake);
> > if (rc)
> > return rc;
> > + kvm->stat.gmap_shadow_r3++;
> > }
> > fallthrough;
> > case ASCE_TYPE_REGION3: {
> > @@ -1515,6 +1519,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> > rc = gmap_shadow_sgt(sg, saddr, rtte.val, *fake);
> > if (rc)
> > return rc;
> > + kvm->stat.gmap_shadow_segment++;
> > }
> > fallthrough;
> > case ASCE_TYPE_SEGMENT: {
> > @@ -1548,6 +1553,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg, unsigned long saddr,
> > rc = gmap_shadow_pgt(sg, saddr, ste.val, *fake);
> > if (rc)
> > return rc;
> > + kvm->stat.gmap_shadow_page++;
>
> do I understand correctly that, if several levels need to be shadowed
> at the same time, you will increment every affected counter, and not
> just the highest or lowest level?
>
> if so, please add a brief explanation to the patch description

Yes, that seemed like the simplest thing to do.

Will add a explanation.

Or should I add a flag and only increment the top level?