[ ... ]
>>>> +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.
