On 06.02.20 10:41, Cornelia Huck wrote:
> On Mon, 3 Feb 2020 08:19:54 -0500
> Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
>
>> From: Janosch Frank <frankja@xxxxxxxxxxxxx>
>>
>> Let's have some debug traces which stay around for longer than the
>> guest.
>>
>> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
>> ---
>> arch/s390/kvm/kvm-s390.c | 10 +++++++++-
>> arch/s390/kvm/kvm-s390.h | 9 +++++++++
>> arch/s390/kvm/pv.c | 21 +++++++++++++++++++--
>> 3 files changed, 37 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index 2beb93f0572f..d4dc156e2c3e 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -221,6 +221,7 @@ static struct kvm_s390_vm_cpu_subfunc kvm_s390_available_subfunc;
>> static struct gmap_notifier gmap_notifier;
>> static struct gmap_notifier vsie_gmap_notifier;
>> debug_info_t *kvm_s390_dbf;
>> +debug_info_t *kvm_s390_dbf_uv;
>>
>> /* Section: not file related */
>> int kvm_arch_hardware_enable(void)
>> @@ -462,7 +463,13 @@ int kvm_arch_init(void *opaque)
>> if (!kvm_s390_dbf)
>> return -ENOMEM;
>>
>> - if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view))
>> + kvm_s390_dbf_uv = debug_register("kvm-uv", 32, 1, 7 * sizeof(long));
>> + if (!kvm_s390_dbf_uv)
>> + return -ENOMEM;
>
> Doesn't that leak kvm_s390_dbf?
Yes, it does. I can simply goto out. The kvm_arch_exit will release all non-zero
dbfs.
>
>> +
>> +
>
> One blank line should be enough.
ack.
>
>> + if (debug_register_view(kvm_s390_dbf, &debug_sprintf_view) ||
>> + debug_register_view(kvm_s390_dbf_uv, &debug_sprintf_view))
>> goto out;
>>
>> kvm_s390_cpu_feat_init();
>> @@ -489,6 +496,7 @@ void kvm_arch_exit(void)
>> {
>> kvm_s390_gib_destroy();
>> debug_unregister(kvm_s390_dbf);
>> + debug_unregister(kvm_s390_dbf_uv);
>> }
>>
>> /* Section: device related */
>
> (...)
>
>> @@ -252,7 +269,7 @@ int kvm_s390_pv_unpack(struct kvm *kvm, unsigned long addr, unsigned long size,
>> addr += PAGE_SIZE;
>> tw[1] += PAGE_SIZE;
>> }
>> - VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: finished rc %x", rc);
>> + VM_EVENT(kvm, 3, "PROTVIRT VM UNPACK: finished with rc %x", rc);
>
> Can you merge this into the patch that introduces this log entry?
ack.
>
> Also, do you want to add logging into the new dbf here as well?
ack
>
>> return rc;
>> }
>>
>
> You often seem to log in pairs (into the per-vm dbf and into the new uv
> dbf). Would it make sense to introduce a new helper for that, or is
> that overkill?
I guess the logging will see several changes over time anyway. Let us start
with an initial variant.
