On Thu, Sep 06, 2018 at 06:00:29AM +0000, Hu, Robert wrote: > > > -----Original Message----- > > From: Eric Blake [mailto:ebl...@redhat.com] > > Sent: Thursday, September 6, 2018 1:41 > > To: Eduardo Habkost <ehabk...@redhat.com> > > Cc: Robert Hoo <robert...@linux.intel.com>; Paolo Bonzini > > <pbonz...@redhat.com>; Hu, Robert <robert...@intel.com>; r...@twiddle.net; > > thomas.lenda...@amd.com; qemu-devel@nongnu.org; Liu, Jingqi > > <jingqi....@intel.com>; Markus Armbruster <arm...@redhat.com>; Jiri > > Denemark <jdene...@redhat.com> > > Subject: Re: [PATCH v3 3/3] Change other funcitons referring to > > feature_word_info[] > > > > On 09/05/2018 11:44 AM, Eduardo Habkost wrote: > > > On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote: > > >> On 09/05/2018 09:10 AM, Eduardo Habkost wrote: > > >>> Question to the QAPI schema maintainers below: > > >>> > > >> > > >>>> ## > > >>>> -{ 'struct': 'X86CPUFeatureWordInfo', > > >>>> - 'data': { 'cpuid-input-eax': 'int', > > >>>> - '*cpuid-input-ecx': 'int', > > >>>> - 'cpuid-register': 'X86CPURegister32', > > >>>> +{ 'struct': 'X86CPUIDFeatureWordInfo', > > >>>> + 'data': { 'input-eax': 'int', > > >>>> + '*input-ecx': 'int', > > >>>> + 'register': 'X86CPURegister32', > > >>>> 'features': 'int' } } > > >> > > >> You are renaming the struct (which is not visible over the wire), as > > >> well as renaming members within the struct (which is visible, if this > > >> type appears on the wire). > > >> > > >> However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits > > >> either before or after this patch (well, first you have to build with > > >> my currently-pending patch as part of Markus' pull request for this > > >> trick to work, https://lists.gnu.org/archive/html/qemu-devel/2018- > > 08/msg05968.html). > > >> Or, even without my patch, grepping for 'input-eax' has no hits as a > > >> member name in any struct. Which means that there are no QMP > > >> commands currently exposing this struct over the wire. > > > > > > There is one: qom-get. > > > > Oh, right, the nasty exception that is not visible to introspection :( > > > > >>> AFAICS, this will break the existing libvirt code: it will make > > >>> qemuMonitorJSONParseCPUx86Features() error out because it won't find > > >>> the "input-eax" field on all X86CPUFeatureWordInfo elements. > > >> > > >> No, this change can't break libvirt, since I just proved there is no > > >> QMP command that libvirt could be using that either supplies an input > > >> member or expects an output member named "input-eax" in the first place. > > > > > > I'm sure it will break libvirt. libvirt uses "qom-get path=<cpu-path> > > > property=feature-words" to get X86FeatureWordInfo data, and it expects > > > the returned data to have a "input-eax" field. > > > > Yeah, since this type is used in the qom-get backdoor that evades > > introspection, > > it's even more important that you follow the backwards-compatibility rules > > and > > not rename or delete any previously-mandatory members, since libvirt CAN'T > > introspect if such changes have happened. > > > [Robert Hoo] > Oh, this is more complicated than my thought. > So, Eduardo, how about leave the QAPI expansion for MSR based features to > other > professional people?
I think it's now clear that we can't add MSR features to "feature-words" in a compatible way, so any mechanism to introspect MSR features will need a separate property or QMP command. I also think "feature-words" needs to be replaced with something better. Probably libvirt should be able to grab all information it needs by looking at the boolean QOM properties instead of the low-level info at "feature-words". This means I agree to leave this functionality out of the MSR feature patch series for now. -- Eduardo
