Re: [Qemu-ppc] [PATCH 19/25] spapr: add hcalls support for the XIVE interrupt mode

[Cédric Le Goater]

[ ... ] 

>>>> +static bool priority_is_valid(uint32_t priority)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(reserved_priorities) / 2; i++) {
>>>> +        uint32_t base  = reserved_priorities[2 * i];
>>>> +        uint32_t count = reserved_priorities[2 * i + 1];
>>>> +
>>>> +        if (priority >= base && priority < base + count) {
>>>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: priority %d is 
>>>> reserved\n",
>>>> +                          __func__, priority);
>>>> +            return false;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return true;
>>>> +}
>>>
>>> This seems like overkill.  Aren't there only 0..7 levels supported in
>>> hardware, in which case a one byte bitmap will suffice to store the
>>> reserved levels.
>>
>> I was trying the use the same array that will be exposed in the device
>> tree in the "ibm,plat-res-int-priorities" property, defined as 
>> follow in PAPR:
>>
>>      property name that designates to the client program that the
>>      platform has reserved one or more interrupt priorities for its
>>      own use.
>>      
>>      prop-encoded-value: one or more (interrupt priority, range)
>>      pairs, where interrupt priority is a single cell hexidec- imal
>>      number between 0x00 and 0xFF, and range is an integer encoded as
>>      with encode-int that represents the number of contiguous
>>      interrupt priorities that have been reserved by the platform for
>>      its internal use.
>>
>>
>> But I agree, it's a bit overkill to check for 0..7 levels ...
> 
> Ok, I do see the point here.  Hmm.. not sure where best to go with
> this.  One source of data is always good, and this is probably less
> complex than deriving the DT list from a bitmap.
> 
> On the other hand I am wary these days of over-generalizing, since it
> can lead to nightmares for migration consistency.

I should be able to simplify by using one array of valid priorities
for the guest, and from that build the array of reserved priorities 
for the platform. 
 
[ ... ]

>>>> +    mmio_base = (uint64_t)xive->esb_base + (1ull << xive->esb_shift) * 
>>>> lisn;
>>>
>>> Hrm.. why was xive->esb_base not already a u64?
>>
>> its an 'hwaddr'. Yes I can remove it.
> 
> Right.  mmio_base should be a hwaddr too, so you shouldn't need a cast.

yes.

[ ... ]

>>> What does the availability of SRC_TRIGGER (and INT_ESB) depend on? 
>>
>> The CPU revision. But we won't introduce XIVE exploitation mode on 
>> anything else than DD2.0 which has full XIVE support. Even STORE_EOI 
>> that we should be adding.
> 
> Hrm.  Host CPU?  That's a problem - if guest visible properties like
> this vary with the host CPU, migration breaks.
> 
>>
>>> If it varies with host capabilities, that's going to be real pain for
>>> migration.
>>
>> Yes. I am not aware of any future extension but I agree this is
>> something we need to keep an eye on.
> 
> I'm not talking about future extension, I'm meaning right now.

I think Ben has answered these questions.

[ ... ] 

>> On that topic, could we include the PPC_BIT* macros somewhere under ppc ? 
> 
> Uh, sure, why not. target/ppc/cpu.h seems the logical place.

I will send a preliminary patch for 2.12 then.


Thanks,


C.