Re: [Qemu-ppc] [Qemu-devel] [PATCH] pseries: Support for in-kernel XICS interrupt controller

[Andreas Färber]

Am 05.06.2013 09:39, schrieb Alexey Kardashevskiy:
> From: David Gibson <address@hidden>
> 
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM.  This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
> 
> This should give considerable performance improvements.  e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
> 
> [Mike Qiu <address@hidden>: fixed mistype which caused ics_set_kvm_state() to 
> fail]
> Signed-off-by: David Gibson <address@hidden>
> Signed-off-by: Alexey Kardashevskiy <address@hidden>

If a Mike Qiu changed this patch, don't we require his Signed-off-by?

CPUState usage looks fine, can't judge the kernel interface, two
nitpicks below.

[...]
> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> index 02e44a0..b83f19f 100644
> --- a/hw/ppc/xics.c
> +++ b/hw/ppc/xics.c
> @@ -29,12 +29,19 @@
>  #include "trace.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "sysemu/kvm.h"
> +#include "config.h"
> +#include "qemu/config-file.h"
> +
> +#include <sys/ioctl.h>
>  
>  /*
>   * ICP: Presentation layer
>   */
>  
>  struct icp_server_state {
> +    CPUState *cs;
>      uint32_t xirr;
>      uint8_t pending_priority;
>      uint8_t mfrr;
> @@ -53,6 +60,9 @@ struct icp_state {
>      uint32_t nr_servers;
>      struct icp_server_state *ss;
>      struct ics_state *ics;
> +    uint32_t set_xive_token, get_xive_token,
> +        int_off_token, int_on_token;

FWIW normally we place struct fields below each other...

> +    int kernel_xics_fd;
>  };
>  
>  static void ics_reject(struct ics_state *ics, int nr);
[...]
> @@ -535,11 +734,14 @@ static int ics_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +

Intentional white space change?

>  static const VMStateDescription vmstate_icp_server = {
>      .name = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
> +    .pre_save = icp_pre_save,
> +    .post_load = icp_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32(xirr, struct icp_server_state),
[snip]

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg