Re: [Qemu-devel] [PATCH v2 51/54] qapi: make s390 commands depend on TARGET_S390X

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, 22 Aug 2017 10:41:34 -0400 (EDT)
> Marc-André Lureau <address@hidden> wrote:
> 
> > Hi
> > 
> > ----- Original Message -----
> > > On 22.08.2017 16:24, Cornelia Huck wrote:
> > > > On Tue, 22 Aug 2017 15:22:52 +0200
> > > > Marc-André Lureau <address@hidden> wrote:
> 
> > > >> @@ -4621,7 +4622,9 @@
> > > >>  ##
> > > >>  { 'command': 'query-cpu-model-comparison',
> > > >>    'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
> > > >> -  'returns': 'CpuModelCompareInfo' }
> > > >> +  'returns': 'CpuModelCompareInfo',
> > > >> +  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
> > > >> +
> > > >>  
> > > >>  ##
> > > >>  # @CpuModelBaselineInfo:
> > > >> @@ -4673,7 +4676,8 @@
> > > >>  { 'command': 'query-cpu-model-baseline',
> > > >>    'data': { 'modela': 'CpuModelInfo',
> > > >>              'modelb': 'CpuModelInfo' },
> > > >> -  'returns': 'CpuModelBaselineInfo' }
> > > >> +  'returns': 'CpuModelBaselineInfo',
> > > >> +  'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
> > > > 
> > > > ...but I'm not sure about the cpu model stuff. Wasn't the idea to move
> > > > to this model for all architectures later? (Given that we have stubs
> > > > for architectures not implementing this, instead of ifdeffing it in
> > > > monitor.c)
> > > >   
> > > 
> > > +1, not architecture specific (in contrast to skey), simply not
> > > supported _yet_ on other architectures.
> > 
> > We can add other archs once they implement it. See for example:
> > "qapi: make query-cpu-model-expansion depend on s390 or x86"
> 
> That seems a bit like whack-a-mole, though. Depending on something like
> "provides cpumodel feature xy" makes it clearer that this is supposed
> to be non-architecture-specific.

I would say the function name and documentation should say if it's architecture 
specific. The #ifdef in qapi generation or in qemu code base is just an 
implementation detail. I suggest to add FIXME in the schema so people looking 
at this realize it's not yet there (instead of error stubs in qemu code / 
runtime).
>