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

[Andreas Färber]

Am 08.06.2013 16:11, schrieb 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.

Then that notation is misleading: [author: ...] usually indicates that
author applied the noted changes to the patch, and just like tags - if
at all - this should get recorded in chronological order, i.e.

S-o-b David ...
[aik: fixed mistype ... spotted by Mike ...]
S-o-b you ...

making clearer who signed off which version.

> What is the general rule who needs
> to s-o-b?

For a formal description see Linux' SubmittingPatches docs.

In practice, whenever you git-am or git-cherry-pick a patch it should
have at least one Signed-off-by from the person you got it from.
Whenever you submit a patch it should carry your Sob as last one,
thereby recording the sequence of through whose hands a patch went.

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

Dunno about whether that is somewhere in HACKING or CODING_STYLE, and I
don't really mind either way.

But let me clarify that I wasn't talking about space-alignment, I was
talking about duplicating the type as you can see for pending_priority
and mfrr field above being on their own line each.

Andreas

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