Re: [PATCH v3 4/6] KVM: PPC: Book3s HV: Hold LPIDs in an unsigned long

[Jordan Niethe <jniethe5@gmail.com>]

On 14/8/23 6:12 pm, Nicholas Piggin wrote:
> On Mon Aug 7, 2023 at 11:45 AM AEST, Jordan Niethe wrote:
>> The LPID register is 32 bits long. The host keeps the lpids for each
>> guest in an unsigned word struct kvm_arch. Currently, LPIDs are already
>> limited by mmu_lpid_bits and KVM_MAX_NESTED_GUESTS_SHIFT.
>>
>> The nestedv2 API returns a 64 bit "Guest ID" to be used be the L1 host
>> for each L2 guest. This value is used as an lpid, e.g. it is the
>> parameter used by H_RPT_INVALIDATE. To minimize needless special casing
>> it makes sense to keep this "Guest ID" in struct kvm_arch::lpid.
>>
>> This means that struct kvm_arch::lpid is too small so prepare for this
>> and make it an unsigned long. This is not a problem for the KVM-HV and
>> nestedv1 cases as their lpid values are already limited to valid ranges
>> so in those contexts the lpid can be used as an unsigned word safely as
>> needed.
>>
>> In the PAPR, the H_RPT_INVALIDATE pid/lpid parameter is already
>> specified as an unsigned long so change pseries_rpt_invalidate() to
>> match that.  Update the callers of pseries_rpt_invalidate() to also take
>> an unsigned long if they take an lpid value.
> 
> I don't suppose it would be worth having an lpid_t.

I actually introduced that when I was developing for the purpose of 
doing the conversion, but I felt like it was unnecessary in the end, it 
is just a wider integer and it is simpler to treat it that way imho.

> 
>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>> index 4adff4f1896d..229f0a1ffdd4 100644
>> --- a/arch/powerpc/kvm/book3s_xive.c
>> +++ b/arch/powerpc/kvm/book3s_xive.c
>> @@ -886,10 +886,10 @@ int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>>   
>>   	if (single_escalation)
>>   		name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
>> -				 vcpu->kvm->arch.lpid, xc->server_num);
>> +				 (unsigned int)vcpu->kvm->arch.lpid, xc->server_num);
>>   	else
>>   		name = kasprintf(GFP_KERNEL, "kvm-%d-%d-%d",
>> -				 vcpu->kvm->arch.lpid, xc->server_num, prio);
>> +				 (unsigned int)vcpu->kvm->arch.lpid, xc->server_num, prio);
>>   	if (!name) {
>>   		pr_err("Failed to allocate escalation irq name for queue %d of VCPU %d\n",
>>   		       prio, xc->server_num);
> 
> I would have thought you'd keep the type and change the format.

yeah, I will do that.

> 
> Otherwise seems okay too.

Thanks.

> 
> Thanks,
> Nick
>