Re: [Qemu-devel] [QEMU-PPC] [PATCH V5 2/7] target/ppc/spapr_caps: Add macro to generate spapr_caps migration vmstate

[Alexey Kardashevskiy]

On 29/01/18 10:43, Suraj Jitindar Singh wrote:
> On Fri, 2018-01-19 at 16:18 +1100, Alexey Kardashevskiy wrote:
>> On 19/01/18 16:00, Suraj Jitindar Singh wrote:
>>> The vmstate description and the contained needed function for
>>> migration
>>> of spapr_caps is the same for each cap, with the name of the cap
>>> substituted. As such introduce a macro to allow for easier
>>> generation of
>>> these.
>>>
>>> Convert the three existing spapr_caps (htm, vsx, and dfp) to use
>>> this
>>> macro.
>>>
>>> Signed-off-by: Suraj Jitindar Singh <address@hidden>
>>>
>>> ---
>>>
>>> V5:
>>>  - Patch added to series
>>> ---
>>>  hw/ppc/spapr_caps.c | 78 +++++++++++++++++----------------------
>>> --------------
>>>  1 file changed, 24 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
>>> index d5c9ce774a..5d52969bd5 100644
>>> --- a/hw/ppc/spapr_caps.c
>>> +++ b/hw/ppc/spapr_caps.c
>>> @@ -228,62 +228,32 @@ int
>>> spapr_caps_post_migration(sPAPRMachineState *spapr)
>>>      return ok ? 0 : -EINVAL;
>>>  }
>>>  
>>> -static bool spapr_cap_htm_needed(void *opaque)
>>> -{
>>> -    sPAPRMachineState *spapr = opaque;
>>> -
>>> -    return spapr->cmd_line_caps[SPAPR_CAP_HTM] &&
>>> -           (spapr->eff.caps[SPAPR_CAP_HTM] != spapr-
>>>> def.caps[SPAPR_CAP_HTM]);
>>> -}
>>> -
>>> -const VMStateDescription vmstate_spapr_cap_htm = {
>>> -    .name = "spapr/cap/htm",
>>> -    .version_id = 1,
>>> -    .minimum_version_id = 1,
>>> -    .needed = spapr_cap_htm_needed,
>>> -    .fields = (VMStateField[]) {
>>> -        VMSTATE_UINT8(mig.caps[SPAPR_CAP_HTM], sPAPRMachineState),
>>> -        VMSTATE_END_OF_LIST()
>>> -    },
>>> -};
>>> -
>>> -static bool spapr_cap_vsx_needed(void *opaque)
>>> -{
>>> -    sPAPRMachineState *spapr = opaque;
>>> -
>>> -    return spapr->cmd_line_caps[SPAPR_CAP_VSX] &&
>>> -           (spapr->eff.caps[SPAPR_CAP_VSX] != spapr-
>>>> def.caps[SPAPR_CAP_VSX]);
>>> +/* Used to generate the migration field and needed function for a
>>> spapr cap */
>>> +#define SPAPR_CAP_MIG_STATE(cap, ccap)                  \
>>> +static bool spapr_cap_##cap##_needed(void *opaque)      \
>>> +{                                                       \
>>> +    sPAPRMachineState *spapr = opaque;                  \
>>> +                                                        \
>>> +    return spapr->cmd_line_caps[SPAPR_CAP_##ccap] &&    \
>>> +           (spapr->eff.caps[SPAPR_CAP_##ccap] !=        \
>>> +            spapr->def.caps[SPAPR_CAP_##ccap]);         \
>>> +}                                                       \
>>> +                                                        \
>>> +const VMStateDescription vmstate_spapr_cap_##cap = {    \
>>> +    .name = "spapr/cap/" #cap,                          \
>>> +    .version_id = 1,                                    \
>>> +    .minimum_version_id = 1,                            \
>>> +    .needed = spapr_cap_##cap##_needed,                 \
>>> +    .fields = (VMStateField[]) {                        \
>>> +        VMSTATE_UINT8(mig.caps[SPAPR_CAP_##ccap],       \
>>> +                      sPAPRMachineState),               \
>>> +        VMSTATE_END_OF_LIST()                           \
>>> +    },                                                  \
>>>  }
>>>  
>>> -const VMStateDescription vmstate_spapr_cap_vsx = {
>>> -    .name = "spapr/cap/vsx",
>>> -    .version_id = 1,
>>> -    .minimum_version_id = 1,
>>> -    .needed = spapr_cap_vsx_needed,
>>> -    .fields = (VMStateField[]) {
>>> -        VMSTATE_UINT8(mig.caps[SPAPR_CAP_VSX], sPAPRMachineState),
>>> -        VMSTATE_END_OF_LIST()
>>> -    },
>>> -};
>>> -
>>> -static bool spapr_cap_dfp_needed(void *opaque)
>>> -{
>>> -    sPAPRMachineState *spapr = opaque;
>>> -
>>> -    return spapr->cmd_line_caps[SPAPR_CAP_DFP] &&
>>> -           (spapr->eff.caps[SPAPR_CAP_DFP] != spapr-
>>>> def.caps[SPAPR_CAP_DFP]);
>>> -}
>>> -
>>> -const VMStateDescription vmstate_spapr_cap_dfp = {
>>> -    .name = "spapr/cap/dfp",
>>> -    .version_id = 1,
>>> -    .minimum_version_id = 1,
>>> -    .needed = spapr_cap_dfp_needed,
>>> -    .fields = (VMStateField[]) {
>>> -        VMSTATE_UINT8(mig.caps[SPAPR_CAP_DFP], sPAPRMachineState),
>>> -        VMSTATE_END_OF_LIST()
>>> -    },
>>> -};
>>> +SPAPR_CAP_MIG_STATE(htm, HTM);
>>> +SPAPR_CAP_MIG_STATE(vsx, VSX);
>>> +SPAPR_CAP_MIG_STATE(dfp, DFP);
>>
>>
>> A nit: I really dislike this kind of parameters which you cannot
>> really
>> cscope/ctags for because they are joined to some other things.
>>
>> "no matches found for cscope query g HTM of HTM" says vim's cscope
>> when I
>> search for "HTM".
> 
> Would something like:
> SPAPR_CAP_MIG_STATE(htm, SPAPR_CAP_HTM);
> SPAPR_CAP_MIG_STATE(htm, SPAPR_CAP_VSX);
> SPAPR_CAP_MIG_STATE(htm, SPAPR_CAP_DFP);
> 
> be better?

It would indeed.

> I could send a follow up

Too late, David already merged :( And this seems to bother only me anyway.


> 
>>
>>
>>>  
>>>  void spapr_caps_reset(sPAPRMachineState *spapr)
>>>  {
>>>
>>
>>


-- 
Alexey