Re: [PATCH v3 1/6] KVM: PPC: Use getters and setters for vcpu register state

[Jordan Niethe <jniethe5@gmail.com>]

On 14/8/23 6:08 pm, Nicholas Piggin wrote:
> On Mon Aug 7, 2023 at 11:45 AM AEST, Jordan Niethe wrote:
>> There are already some getter and setter functions used for accessing
>> vcpu register state, e.g. kvmppc_get_pc(). There are also more
>> complicated examples that are generated by macros like
>> kvmppc_get_sprg0() which are generated by the SHARED_SPRNG_WRAPPER()
>> macro.
>>
>> In the new PAPR "Nestedv2" API for nested guest partitions the L1 is
>> required to communicate with the L0 to modify and read nested guest
>> state.
>>
>> Prepare to support this by replacing direct accesses to vcpu register
>> state with wrapper functions. Follow the existing pattern of using
>> macros to generate individual wrappers. These wrappers will
>> be augmented for supporting Nestedv2 guests later.
>>
>> Signed-off-by: Gautam Menghani <gautam@linux.ibm.com>
>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> ---
>> v3:
>>    - Do not add a helper for pvr
>>    - Use an expression when declaring variable in case
>>    - Squash in all getters and setters
>>    - Guatam: Pass vector registers by reference
>> ---
>>   arch/powerpc/include/asm/kvm_book3s.h  | 123 +++++++++++++-
>>   arch/powerpc/include/asm/kvm_booke.h   |  10 ++
>>   arch/powerpc/kvm/book3s.c              |  38 ++---
>>   arch/powerpc/kvm/book3s_64_mmu_hv.c    |   4 +-
>>   arch/powerpc/kvm/book3s_64_mmu_radix.c |   9 +-
>>   arch/powerpc/kvm/book3s_64_vio.c       |   4 +-
>>   arch/powerpc/kvm/book3s_hv.c           | 220 +++++++++++++------------
>>   arch/powerpc/kvm/book3s_hv.h           |  58 +++++++
>>   arch/powerpc/kvm/book3s_hv_builtin.c   |  10 +-
>>   arch/powerpc/kvm/book3s_hv_p9_entry.c  |   4 +-
>>   arch/powerpc/kvm/book3s_hv_ras.c       |   5 +-
>>   arch/powerpc/kvm/book3s_hv_rm_mmu.c    |   8 +-
>>   arch/powerpc/kvm/book3s_hv_rm_xics.c   |   4 +-
>>   arch/powerpc/kvm/book3s_xive.c         |   9 +-
>>   arch/powerpc/kvm/emulate_loadstore.c   |   2 +-
>>   arch/powerpc/kvm/powerpc.c             |  76 ++++-----
>>   16 files changed, 395 insertions(+), 189 deletions(-)
>>
> 
> [snip]
> 
>> +
>>   /* Expiry time of vcpu DEC relative to host TB */
>>   static inline u64 kvmppc_dec_expires_host_tb(struct kvm_vcpu *vcpu)
>>   {
>> -	return vcpu->arch.dec_expires - vcpu->arch.vcore->tb_offset;
>> +	return kvmppc_get_dec_expires(vcpu) - kvmppc_get_tb_offset_hv(vcpu);
>>   }
> 
> I don't see kvmppc_get_tb_offset_hv in this patch.

It should be generated by:

KVMPPC_BOOK3S_VCORE_ACCESSOR(tb_offset, 64)

> 
>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> index 7f765d5ad436..738f2ecbe9b9 100644
>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> @@ -347,7 +347,7 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
>>   	unsigned long v, orig_v, gr;
>>   	__be64 *hptep;
>>   	long int index;
>> -	int virtmode = vcpu->arch.shregs.msr & (data ? MSR_DR : MSR_IR);
>> +	int virtmode = kvmppc_get_msr(vcpu) & (data ? MSR_DR : MSR_IR);
>>   
>>   	if (kvm_is_radix(vcpu->kvm))
>>   		return kvmppc_mmu_radix_xlate(vcpu, eaddr, gpte, data, iswrite);
> 
> So this isn't _only_ adding new accessors. This should be functionally a
> noop, but I think it introduces a branch if PR is defined.

That being checking kvmppc_shared_big_endian()?

> 
> Shared page is a slight annoyance for HV, I'd like to get rid of it...
> but that's another story. I think the pattern here would be to add a
> kvmppc_get_msr_hv() accessor.

Yes, that will work.

> 
> And as a nitpick, for anywhere employing existing access functions, gprs
> and such, could that be split into its own patch?

Sure will do. One other thing I could do is make the existing functions 
use the macros if they don't already. Do you think that is worth doing?

> 
> Looks pretty good aside from those little things.

Thanks.

> 
> Thanks,
> Nick
>