Re: [v3 02/15] KVM: PPC: Book3S HV: Cleanup updates for kvm vcpu MMCR

From: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: ego@linux.vnet.ibm.com, Michael Neuling <mikey@neuling.org>,
	maddy@linux.vnet.ibm.com, kvm@vger.kernel.org,
	svaidyan@in.ibm.com, kvm-ppc@vger.kernel.org, acme@kernel.org,
	jolsa@kernel.org, linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [v3 02/15] KVM: PPC: Book3S HV: Cleanup updates for kvm vcpu MMCR
Date: Wed, 22 Jul 2020 11:19:53 +0530	[thread overview]
Message-ID: <4E79629C-CB5B-450F-B00B-04267FAF1F13@linux.vnet.ibm.com> (raw)
In-Reply-To: <87y2ncqi5s.fsf@mpe.ellerman.id.au>

[-- Attachment #1: Type: text/plain, Size: 4301 bytes --]

> On 22-Jul-2020, at 10:07 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> 
> Athira Rajeev <atrajeev@linux.vnet.ibm.com <mailto:atrajeev@linux.vnet.ibm.com>> writes:
>>> On 21-Jul-2020, at 9:24 AM, Paul Mackerras <paulus@ozlabs.org> wrote:
>>> On Fri, Jul 17, 2020 at 10:38:14AM -0400, Athira Rajeev wrote:
>>>> Currently `kvm_vcpu_arch` stores all Monitor Mode Control registers
>>>> in a flat array in order: mmcr0, mmcr1, mmcra, mmcr2, mmcrs
>>>> Split this to give mmcra and mmcrs its own entries in vcpu and
>>>> use a flat array for mmcr0 to mmcr2. This patch implements this
>>>> cleanup to make code easier to read.
>>> 
>>> Changing the way KVM stores these values internally is fine, but
>>> changing the user ABI is not.  This part:
>>> 
>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>>>> index 264e266..e55d847 100644
>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>> @@ -510,8 +510,8 @@ struct kvm_ppc_cpu_char {
>>>> 
>>>> #define KVM_REG_PPC_MMCR0	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x10)
>>>> #define KVM_REG_PPC_MMCR1	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x11)
>>>> -#define KVM_REG_PPC_MMCRA	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x12)
>>>> -#define KVM_REG_PPC_MMCR2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x13)
>>>> +#define KVM_REG_PPC_MMCR2	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x12)
>>>> +#define KVM_REG_PPC_MMCRA	(KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x13)
>>> 
>>> means that existing userspace programs that used to work would now be
>>> broken.  That is not acceptable (breaking the user ABI is only ever
>>> acceptable with a very compelling reason).  So NAK to this part of the
>>> patch.
>> 
>> Hi Paul
>> 
>> Thanks for checking the patch. I understood your point on user ABI breakage that this particular change can cause.
>> I will retain original KVM_REG_PPC_MMCRA and KVM_REG_PPC_MMCR2 order in `kvm.h`
>> And with that, additionally I will need below change ( on top of current patch ) for my clean up updates for kvm cpu MMCR to work,
>> Because now mmcra and mmcrs will have its own entries in vcpu and is not part of the mmcr[] array
>> Please suggest if this looks good
> 
> I did the same patch I think in my testing branch, it's here:
> 
> https://github.com/linuxppc/linux/commit/daea78154eff1b7e2f36be05a8f95feb5a588912 <https://github.com/linuxppc/linux/commit/daea78154eff1b7e2f36be05a8f95feb5a588912>
> 
> 
> Can you please check that matches what you sent.

Hi Michael,

Yes, it matches. Thanks for making this change.

> 
> cheers
> 
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 3f90eee261fc..b10bb404f0d5 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1679,10 +1679,13 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>>        case KVM_REG_PPC_UAMOR:
>>                *val = get_reg_val(id, vcpu->arch.uamor);
>>                break;
>> -       case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR2:
>> +       case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR1:
>>                i = id - KVM_REG_PPC_MMCR0;
>>                *val = get_reg_val(id, vcpu->arch.mmcr[i]);
>>                break;
>> +       case KVM_REG_PPC_MMCR2:
>> +               *val = get_reg_val(id, vcpu->arch.mmcr[2]);
>> +               break;
>>        case KVM_REG_PPC_MMCRA:
>>                *val = get_reg_val(id, vcpu->arch.mmcra);
>>                break;
>> @@ -1906,10 +1909,13 @@ static int kvmppc_set_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
>>        case KVM_REG_PPC_UAMOR:
>>                vcpu->arch.uamor = set_reg_val(id, *val);
>>                break;
>> -       case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR2:
>> +       case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR1:
>>                i = id - KVM_REG_PPC_MMCR0;
>>                vcpu->arch.mmcr[i] = set_reg_val(id, *val);
>>                break;
>> +       case KVM_REG_PPC_MMCR2:
>> +               vcpu->arch.mmcr[2] = set_reg_val(id, *val);
>> +               break;
>>        case KVM_REG_PPC_MMCRA:
>>                vcpu->arch.mmcra = set_reg_val(id, *val);
>>                break;
>> —
>> 
>> 
>>> 
>>> Regards,
>>> Paul.

[-- Attachment #2: Type: text/html, Size: 14788 bytes --]