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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

On Thu, Jan 03, 2019 at 06:45:25PM +0100, Cédric Le Goater wrote:
> 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.

Hm, ok.

I've applied patches 5..8.

> 
> 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 {
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]