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

2020-07-22 Thread Madhavan Srinivasan 




On 7/22/20 10:24 AM, Paul Mackerras wrote:

On Wed, Jul 22, 2020 at 07:39:26AM +0530, Athira Rajeev wrote:



On 21-Jul-2020, at 9:24 AM, Paul Mackerras  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

Yes, that looks fine.

By the way, is the new MMCRS register here at all related to the MMCRS

Hi Paul,

We have only split the current array (mmcr[]) and separated it to mmcra 
and mmcrs.

Only new spr that is added is mmcr3 (for Power10).

Maddy


that there used to be on POWER8, but which wasn't present (as far as I
know) on POWER9?

Paul.

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

2020-07-22 Thread Athira Rajeev 


> On 22-Jul-2020, at 10:07 AM, Michael Ellerman  wrote:
> 
> Athira Rajeev  > writes:
>>> On 21-Jul-2020, at 9:24 AM, Paul Mackerras  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
>  
> 
> 
> 
> 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.

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

2020-07-21 Thread Paul Mackerras 
On Wed, Jul 22, 2020 at 07:39:26AM +0530, Athira Rajeev wrote:
> 
> 
> > On 21-Jul-2020, at 9:24 AM, Paul Mackerras  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

Yes, that looks fine.

By the way, is the new MMCRS register here at all related to the MMCRS
that there used to be on POWER8, but which wasn't present (as far as I
know) on POWER9?

Paul.

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

2020-07-21 Thread Michael Ellerman 
Paul Mackerras  writes:
> 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.

I assume we don't have a KVM unit test that would have caught this?

cheers

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

2020-07-21 Thread Michael Ellerman 
Athira Rajeev  writes:
>> On 21-Jul-2020, at 9:24 AM, Paul Mackerras  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


Can you please check that matches what you sent.

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.

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

2020-07-21 Thread Athira Rajeev 


> On 21-Jul-2020, at 9:24 AM, Paul Mackerras  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

Thanks
Athira 

—
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.

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

2020-07-20 Thread Paul Mackerras 
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.

Regards,
Paul.

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

2020-07-17 Thread Athira Rajeev 
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.

Signed-off-by: Athira Rajeev 
---
 arch/powerpc/include/asm/kvm_host.h   |  4 +++-
 arch/powerpc/include/uapi/asm/kvm.h   |  4 ++--
 arch/powerpc/kernel/asm-offsets.c |  2 ++
 arch/powerpc/kvm/book3s_hv.c  | 16 ++--
 arch/powerpc/kvm/book3s_hv_rmhandlers.S   | 12 ++--
 tools/arch/powerpc/include/uapi/asm/kvm.h |  4 ++--
 6 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h 
b/arch/powerpc/include/asm/kvm_host.h
index 7e2d061..fc115e2 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -637,7 +637,9 @@ struct kvm_vcpu_arch {
u32 ccr1;
u32 dbsr;
 
-   u64 mmcr[5];
+   u64 mmcr[3];
+   u64 mmcra;
+   u64 mmcrs;
u32 pmc[8];
u32 spmc[2];
u64 siar;
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)
 #define KVM_REG_PPC_MMCRS  (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x14)
 #define KVM_REG_PPC_SIAR   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x15)
 #define KVM_REG_PPC_SDAR   (KVM_REG_PPC | KVM_REG_SIZE_U64 | 0x16)
diff --git a/arch/powerpc/kernel/asm-offsets.c 
b/arch/powerpc/kernel/asm-offsets.c
index 6657dc6..6fa4853 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -559,6 +559,8 @@ int main(void)
OFFSET(VCPU_IRQ_PENDING, kvm_vcpu, arch.irq_pending);
OFFSET(VCPU_DBELL_REQ, kvm_vcpu, arch.doorbell_request);
OFFSET(VCPU_MMCR, kvm_vcpu, arch.mmcr);
+   OFFSET(VCPU_MMCRA, kvm_vcpu, arch.mmcra);
+   OFFSET(VCPU_MMCRS, kvm_vcpu, arch.mmcrs);
OFFSET(VCPU_PMC, kvm_vcpu, arch.pmc);
OFFSET(VCPU_SPMC, kvm_vcpu, arch.spmc);
OFFSET(VCPU_SIAR, kvm_vcpu, arch.siar);
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 6bf66649..3f90eee 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1679,10 +1679,16 @@ 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_MMCRS:
+   case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR2:
i = id - KVM_REG_PPC_MMCR0;
*val = get_reg_val(id, vcpu->arch.mmcr[i]);
break;
+   case KVM_REG_PPC_MMCRA:
+   *val = get_reg_val(id, vcpu->arch.mmcra);
+   break;
+   case KVM_REG_PPC_MMCRS:
+   *val = get_reg_val(id, vcpu->arch.mmcrs);
+   break;
case KVM_REG_PPC_PMC1 ... KVM_REG_PPC_PMC8:
i = id - KVM_REG_PPC_PMC1;
*val = get_reg_val(id, vcpu->arch.pmc[i]);
@@ -1900,10 +1906,16 @@ 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_MMCRS:
+   case KVM_REG_PPC_MMCR0 ... KVM_REG_PPC_MMCR2:
i = id - KVM_REG_PPC_MMCR0;
vcpu->arch.mmcr[i] = set_reg_val(id, *val);
break;
+   case KVM_REG_PPC_MMCRA:
+   vcpu->arch.mmcra = set_reg_val(id, *val);
+   break;
+   case KVM_REG_PPC_MMCRS:
+   vcpu->arch.mmcrs = set_reg_val(id, *val);
+   break;
case KVM_REG_PPC_PMC1 ... KVM_REG_PPC_PMC8:
i = id - KVM_REG_PPC_PMC1;
vcpu->arch.pmc[i] = set_reg_val(id, *val);
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S 
b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index 7194389..702eaa2 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -3428,7 +3428,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_PMAO_BUG)
mtspr   SPRN_PMC6, r9
ld  r3, VCPU_MMCR(r4)
ld  r5, VCPU_MMCR + 8(r4)
-   ld  r6, VCPU_MMCR + 16(r4)
+   ld  r6, VCPU_MMCRA(r4)
ld  r7, VCPU_SIAR(r4)
ld