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

[Jordan Niethe <jniethe5@gmail.com>]

On 15/8/23 8:45 pm, Michael Ellerman wrote:
> "Nicholas Piggin" <npiggin@gmail.com> writes:
>> 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.
>>
>>> 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. Don't we risk having ambigious names by discarding the high bits?
> Not sure that would be a bug per se, but it could be confusing.

In this context is would always be constrained be the number of LPID 
bits so wouldn't be ambiguous, but I'm going to change the format.

> 
> cheers
>