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

[Alexey Kardashevskiy]

On 06/08/2013 08:20 PM, Andreas Färber wrote:
> 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?


He did not change this patch, he found a mistype in our local source tree
which I decided to merge with this patch. I did not want him not to be
mentioned at all so I added this line. What is the general rule who needs
to s-o-b?



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


Is it mandatory? I personally do not see _any_ benefit in aligning struct
members with spaces.


-- 
Alexey