Re: [Qemu-devel] [PATCH 08/10] ppc/xics: allow ICSState to have an offset 0

["Cédric Le Goater" <clg@kaod.org>]

On 1/3/19 5:33 AM, David Gibson wrote:
> On Wed, Jan 02, 2019 at 06:57:41AM +0100, Cédric Le Goater wrote:
>> commit 15ed653fa49a ("ppc/xics: An ICS with offset 0 is assumed to be
>> uninitialized") introduced an extra check on the ICS offset which is
>> not strictly necessary.
> 
> The commit message for that suggests it was added to make pnv easier.
> I take it you no longer need this for the current or expected pnv
> code?

This is really just a runtime check which makes sure that the 
ICSState offset is initialized by the FW before being used.

The problem is more global. It comes from the fact the ICSState 
offset in QEMU and the XICS offset in KVM is hardcoded to 0x1000. 

  Thinking aloud :

    May be we should default the offset to 0 and introduce a service 
    to set the value in QEMU and KVM.   

The 'dual' interrupt mode (using the qirq array at the machine level) 
needs the IRQ number space of the XIVE and XICS interrupt mode to
be in sync, that is to start a 0x0, even if the lower 4K are ignored
in XICS.   

So the 'dual' mode tunes the offset to 0 once the ICSState is created
and ignores the lower 4K when initializing the KVM XICS device. 

C.

>> Revert the change to be able to map the XICS IRQ number space on the
>> XIVE IRQ number space.
> 
> 
> 
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/xics.h | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>> index 7668c381a887..07508cbd217e 100644
>> --- a/include/hw/ppc/xics.h
>> +++ b/include/hw/ppc/xics.h
>> @@ -139,8 +139,7 @@ struct ICSState {
>>  
>>  static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
>>  {
>> -    return (ics->offset != 0) && (nr >= ics->offset)
>> -        && (nr < (ics->offset + ics->nr_irqs));
>> +    return (nr >= ics->offset) && (nr < (ics->offset + ics->nr_irqs));
>>  }
>>  
>>  struct ICSIRQState {
>