Re: [PATCH 3/9] KVM: PPC: Book3S: Add kernel emulation for the XICS interrupt controller

[Scott Wood <scottwood@freescale.com>]

On 02/14/2013 06:01:08 PM, Paul Mackerras wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> This adds in-kernel emulation of the XICS (eXternal Interrupt
> Controller Specification) interrupt controller specified by PAPR, for
> both HV and PR KVM guests.
> 
> This adds a new KVM_CREATE_IRQCHIP_ARGS ioctl, which is like
> KVM_CREATE_IRQCHIP in that it indicates that the virtual machine
> should use in-kernel interrupt controller emulation, but also takes an
> argument struct that contains the type of interrupt controller
> architecture and an optional parameter.  Currently only one type value
> is defined, that which indicates the XICS architecture.

Would the device config API I posted a couple days ago work for you?

> The XICS emulation supports up to 1048560 interrupt sources.
> Interrupt source numbers below 16 are reserved; 0 is used to mean no
> interrupt and 2 is used for IPIs.  Internally these are represented in
> blocks of 1024, called ICS (interrupt controller source) entities, but
> that is not visible to userspace.
> 
> Two other new ioctls allow userspace to control the interrupt
> sources.  The KVM_IRQCHIP_SET_SOURCES ioctl sets the priority,
> destination cpu, level/edge sensitivity and pending state of a range
> of interrupt sources, creating them if they don't already exist.  The
> KVM_IRQCHIP_GET_SOURCES ioctl returns that information for a range of
> interrupt sources (they are required to already exist).

Why is it userspace's job to control these?  If you use KVM_IRQ_PENDING
for interrupt injection, what if there's a race with the user changing
other flags via MMIO?  Maybe this isn't an issue with XICS, but this is
being presented as a generic API.

> +4.80 KVM_CREATE_IRQCHIP_ARGS
> +
> +Capability: KVM_CAP_IRQCHIP_ARGS
> +Architectures: ppc

Why just ppc?

> +struct kvm_irq_sources {
> +	__u32 irq;
> +	__u32 nr_irqs;
> +	__u64 __user *irqbuf;
> +};

Please no pointers in UAPI -- this would require a compat wrapper with
32-bit user and 64-bit kernel.

> +/* irqbuf entries are laid out like this: */
> +#define KVM_IRQ_SERVER_SHIFT	0
> +#define KVM_IRQ_SERVER_MASK	0xffffffffULL
> +#define KVM_IRQ_PRIORITY_SHIFT	32
> +#define KVM_IRQ_PRIORITY_MASK	0xff
> +#define KVM_IRQ_LEVEL_SENSITIVE	(1ULL << 40)
> +#define KVM_IRQ_MASKED		(1ULL << 41)
> +#define KVM_IRQ_PENDING		(1ULL << 42)

What does "server" mean?  Do you mean "laid out like this for XICS"?
Let's please have a clean separation between what is generic and what is
implementation-specific.

-Scott