| Message ID | 1456224728-28163-2-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2016-02-23 at 18:52 +0800, Peter Xu wrote: > A new enum type is added to define ARM GIC types. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qapi-schema.json | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 8d04897..81654bd 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -4083,3 +4083,20 @@ > ## > { 'enum': 'ReplayMode', > 'data': [ 'none', 'record', 'play' ] } > + > +## > +# @GICType: > +# > +# An enumeration of GIC types > +# > +# @gicv2: GICv2 support without kernel irqchip > +# > +# @gicv3: GICv3 support without kernel irqchip > +# > +# @gicv2-kvm: GICv3 support with kernel irqchip > +# > +# @gicv3-kvm: GICv3 support with kernel irqchip > +# > +# Since: 2.6 > +## > +{ 'enum': 'GICType', 'data': [ 'gicv2', 'gicv3', 'gicv2-kvm', 'gicv3-kvm' ] } Wouldn't this conflate the use of accel= and kernel_irqchip= options? IIUC, depending on the hardware, you might find yourself in the following situation: accel=tcg,gic-version=3 unavailable accel=kvm,kernel_irqchip=off,gic-version=3 unavailable accel=kvm,gic-version=3 available so I'd expect the output to be something like [ "v2": { "tcg": true, "kvm-without-kernel-irqchip": true, "kvm": true }, "v3": { "tcg": false, "kvm-without-kernel-irqchip": false, "kvm": true } ] Since libvirt currently doesn't have support for the kernel_irqchip= option, it would only take the "tcg" and "kvm" values into account; on the other hand, if at some point libvirt will grow support for that option it would be able to retrieve all the required information. Cheers. -- Andrea Bolognani Software Engineer - Virtualization Team
On 02/23/2016 03:52 AM, Peter Xu wrote: > A new enum type is added to define ARM GIC types. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > qapi-schema.json | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 8d04897..81654bd 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -4083,3 +4083,20 @@ > ## > { 'enum': 'ReplayMode', > 'data': [ 'none', 'record', 'play' ] } > + > +## > +# @GICType: > +# > +# An enumeration of GIC types Worth spelling out the acronym? > +# > +# @gicv2: GICv2 support without kernel irqchip > +# > +# @gicv3: GICv3 support without kernel irqchip > +# > +# @gicv2-kvm: GICv3 support with kernel irqchip Doc typo, should be GICv2 > +# > +# @gicv3-kvm: GICv3 support with kernel irqchip > +# > +# Since: 2.6 > +## > +{ 'enum': 'GICType', 'data': [ 'gicv2', 'gicv3', 'gicv2-kvm', 'gicv3-kvm' ] } >
On Tue, Mar 01, 2016 at 03:20:59PM +0100, Andrea Bolognani wrote: > On Tue, 2016-02-23 at 18:52 +0800, Peter Xu wrote: > > +{ 'enum': 'GICType', 'data': [ 'gicv2', 'gicv3', 'gicv2-kvm', 'gicv3-kvm' ] } > > Wouldn't this conflate the use of accel= and kernel_irqchip= options? AFAIU, it's not a problem. Let me paste some lines from the original RFC thread which explains the definition of the entries: - gicv2: GIC version 2 without kernel IRQ chip - gicv2-kvm: GIC version 2 with kernel IRQ chip - gicv3: GIC version 3 without kernel IRQ chip (not supported) - gicv3-kvm: GIC version 3 with kernel IRQ chip (from https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg02882.html) So... what I understand is that, we are not talking about "accel=" at all. Instead, we are talking about "kernel_irqchip=" only. Or say, all these GIC version information we provide from QEMU does not tell whether KVM is supported or not (for "accel=", it is provided by another QMP message named "query-kvm"). We are only talking about which kind of GIC we support. In our case, for each version, it could be supported either in userspace, or in kernel. > > IIUC, depending on the hardware, you might find yourself in the > following situation: > > accel=tcg,gic-version=3 unavailable > accel=kvm,kernel_irqchip=off,gic-version=3 unavailable As explained above, IIUC, both of these two "unavailable" ones correspond to "gicv3" entry of the results. > accel=kvm,gic-version=3 available And this one corresponds to "gicv3-kvm" entry. > > so I'd expect the output to be something like > > [ "v2": { "tcg": true, > "kvm-without-kernel-irqchip": true, > "kvm": true }, > "v3": { "tcg": false, > "kvm-without-kernel-irqchip": false, > "kvm": true } ] Actually, this reminded me about the "kernel_irqchip=split" case. Do we need to consider that one? AFAIK, splitted irqchip is only used for x86 currently. Whether ARM will possibly support splitted kernel irqchip one day just like x86? If so, I would prefer to change the query result layout from array to dict, like: [ "v2": { "emulated": true, "split": false, "kernel": true }, "v3": { "emulated": false, "split": false, "kernel": true } ] Since the matrix is big enough (2x3) to consider drop the array format (I'd admit maybe dict is always the best one...). Peter > > Since libvirt currently doesn't have support for the > kernel_irqchip= option, it would only take the "tcg" and "kvm" > values into account; on the other hand, if at some point > libvirt will grow support for that option it would be able to > retrieve all the required information. > > Cheers. > > -- > Andrea Bolognani > Software Engineer - Virtualization Team
On Tue, Mar 01, 2016 at 09:46:07AM -0700, Eric Blake wrote: > On 02/23/2016 03:52 AM, Peter Xu wrote: > > A new enum type is added to define ARM GIC types. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > qapi-schema.json | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/qapi-schema.json b/qapi-schema.json > > index 8d04897..81654bd 100644 > > --- a/qapi-schema.json > > +++ b/qapi-schema.json > > @@ -4083,3 +4083,20 @@ > > ## > > { 'enum': 'ReplayMode', > > 'data': [ 'none', 'record', 'play' ] } > > + > > +## > > +# @GICType: > > +# > > +# An enumeration of GIC types > > Worth spelling out the acronym? Sure. I can do it in next spin. > > > +# > > +# @gicv2: GICv2 support without kernel irqchip > > +# > > +# @gicv3: GICv3 support without kernel irqchip > > +# > > +# @gicv2-kvm: GICv3 support with kernel irqchip > > Doc typo, should be GICv2 Will fix. If dict to be used finally, will rework as a whole. Thanks! Peter
On Wed, Mar 02, 2016 at 11:34:44AM +0800, Peter Xu wrote: > [ "v2": { "emulated": true, > "split": false, > "kernel": true }, > "v3": { "emulated": false, > "split": false, > "kernel": true } ] Or something like this: [{ "version": 2, "emulated": true, "split": false, "kernel": true }, { "version": 3, "emulated": false, "split": false, "kernel": true }] If temporarily not considering kernel_irqchip=split case: [{ "version": 2, "emulated": true, "kernel": true }, { "version": 3, "emulated": false, "kernel": true }] To use array rather than dict so that we do not need to change qapi schema again when GICv4 comes. Peter
Peter Xu <peterx@redhat.com> writes: > On Wed, Mar 02, 2016 at 11:34:44AM +0800, Peter Xu wrote: >> [ "v2": { "emulated": true, >> "split": false, >> "kernel": true }, >> "v3": { "emulated": false, >> "split": false, >> "kernel": true } ] > > Or something like this: > > [{ > "version": 2, > "emulated": true, > "split": false, > "kernel": true > }, > { > "version": 3, > "emulated": false, > "split": false, > "kernel": true > }] > > If temporarily not considering kernel_irqchip=split case: > > [{ > "version": 2, > "emulated": true, > "kernel": true > }, > { > "version": 3, > "emulated": false, > "kernel": true > }] > > To use array rather than dict so that we do not need to change qapi > schema again when GICv4 comes. Drive-by shooting without sufficient context: we may *want* to change the QAPI schema, because that makes the change introspectable with query-schema.
On Wed, Mar 02, 2016 at 10:47:39AM +0100, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > If temporarily not considering kernel_irqchip=split case: > > > > [{ > > "version": 2, > > "emulated": true, > > "kernel": true > > }, > > { > > "version": 3, > > "emulated": false, > > "kernel": true > > }] > > > > To use array rather than dict so that we do not need to change qapi > > schema again when GICv4 comes. > > Drive-by shooting without sufficient context: we may *want* to change > the QAPI schema, because that makes the change introspectable with > query-schema. Failed to catch the point. :( What's "query-schema"? Is that a QMP command? What I meant is that, we can define the following (for example): { 'struct': 'GICCapInfo', 'data': [ 'version': 'int', 'emulated': 'bool', 'kernel': 'bool'] } And: { 'command': 'query-gic-capability', 'returns': ['GICCapInfo'] } So we can keep this schema as it is when new versions arrive. We can just push another element in. Peter
Peter Xu <peterx@redhat.com> writes: > On Wed, Mar 02, 2016 at 10:47:39AM +0100, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> > If temporarily not considering kernel_irqchip=split case: >> > >> > [{ >> > "version": 2, >> > "emulated": true, >> > "kernel": true >> > }, >> > { >> > "version": 3, >> > "emulated": false, >> > "kernel": true >> > }] >> > >> > To use array rather than dict so that we do not need to change qapi >> > schema again when GICv4 comes. >> >> Drive-by shooting without sufficient context: we may *want* to change >> the QAPI schema, because that makes the change introspectable with >> query-schema. > > Failed to catch the point. :( > > What's "query-schema"? Is that a QMP command? Yes. More than you ever wanted to know: http://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf https://www.youtube.com/watch?v=IEa8Ao8_B9o&list=PLW3ep1uCIRfyLNSu708gWG7uvqlolk0ep&index=28 > What I meant is that, we can define the following (for example): > > { 'struct': 'GICCapInfo', > 'data': [ > 'version': 'int', > 'emulated': 'bool', > 'kernel': 'bool'] } > > And: > > { 'command': 'query-gic-capability', > 'returns': ['GICCapInfo'] } > > So we can keep this schema as it is when new versions arrive. We > can just push another element in. To answer questions of the sort "can this QEMU version do X?", it's often useful to tie X to a schema change that is visible in the result of query-schema. Often != always. I'd rather not mess with the schema in unnatural ways just to make something visible in query-schema. Note that "can this *board* do X" is a different question.
On Wed, Mar 02, 2016 at 02:59:57PM +0100, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > What's "query-schema"? Is that a QMP command? > > Yes. > > More than you ever wanted to know: > http://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf > https://www.youtube.com/watch?v=IEa8Ao8_B9o&list=PLW3ep1uCIRfyLNSu708gWG7uvqlolk0ep&index=28 Thanks for the pointers and cool slides! It's a good thing to pick up. :-) It'll be cool we treat data as codes, and codes as data. I see that qapi-introspect branch is not there now. Is it merged to some other branch already? When will it be there in QEMU master (still not in, right?)? Just curious about it. > > > What I meant is that, we can define the following (for example): > > > > { 'struct': 'GICCapInfo', > > 'data': [ > > 'version': 'int', > > 'emulated': 'bool', > > 'kernel': 'bool'] } > > > > And: > > > > { 'command': 'query-gic-capability', > > 'returns': ['GICCapInfo'] } > > > > So we can keep this schema as it is when new versions arrive. We > > can just push another element in. > > To answer questions of the sort "can this QEMU version do X?", it's > often useful to tie X to a schema change that is visible in the result > of query-schema. Now I can understand. For this case, I guess both ways work, right? Considering that if "query-schema" is still not there, I'd still prefer the "array" solution. At least, it can keep the schema several lines shorter (as you have mentioned already, it's *big* enough :). Also, even we would have "query-schema", I would still prefer not change schema unless necessary. What do you think? Thanks! Peter
Peter Xu <peterx@redhat.com> writes: > On Wed, Mar 02, 2016 at 02:59:57PM +0100, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> > What's "query-schema"? Is that a QMP command? >> >> Yes. >> >> More than you ever wanted to know: >> http://events.linuxfoundation.org/sites/events/files/slides/armbru-qemu-introspection.pdf >> https://www.youtube.com/watch?v=IEa8Ao8_B9o&list=PLW3ep1uCIRfyLNSu708gWG7uvqlolk0ep&index=28 > > Thanks for the pointers and cool slides! It's a good thing to pick > up. :-) > > It'll be cool we treat data as codes, and codes as data. > > I see that qapi-introspect branch is not there now. Is it merged to > some other branch already? When will it be there in QEMU master > (still not in, right?)? Just curious about it. Merged in commit 9e72681. Between the talk and the merge, query-schema got renamed to query-qmp-schema. Sometimes I relapse. Sorry for the confusion! >> > What I meant is that, we can define the following (for example): >> > >> > { 'struct': 'GICCapInfo', >> > 'data': [ >> > 'version': 'int', >> > 'emulated': 'bool', >> > 'kernel': 'bool'] } >> > >> > And: >> > >> > { 'command': 'query-gic-capability', >> > 'returns': ['GICCapInfo'] } >> > >> > So we can keep this schema as it is when new versions arrive. We >> > can just push another element in. >> >> To answer questions of the sort "can this QEMU version do X?", it's >> often useful to tie X to a schema change that is visible in the result >> of query-schema. > > Now I can understand. For this case, I guess both ways work, right? > Considering that if "query-schema" is still not there, I'd still > prefer the "array" solution. At least, it can keep the schema > several lines shorter (as you have mentioned already, it's *big* > enough :). Also, even we would have "query-schema", I would still > prefer not change schema unless necessary. What do you think? I can't say without understanding what the introspection question would be. That needs actual thought, which is in short supply, especially before breakfast ;)
On Thu, Mar 03, 2016 at 07:34:21AM +0100, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > I see that qapi-introspect branch is not there now. Is it merged to > > some other branch already? When will it be there in QEMU master > > (still not in, right?)? Just curious about it. > > Merged in commit 9e72681. > > Between the talk and the merge, query-schema got renamed to > query-qmp-schema. Sometimes I relapse. Sorry for the confusion! Got it! > > Now I can understand. For this case, I guess both ways work, right? > > Considering that if "query-schema" is still not there, I'd still > > prefer the "array" solution. At least, it can keep the schema > > several lines shorter (as you have mentioned already, it's *big* > > enough :). Also, even we would have "query-schema", I would still > > prefer not change schema unless necessary. What do you think? > > I can't say without understanding what the introspection question would > be. That needs actual thought, which is in short supply, especially > before breakfast ;) Yah, anyway, looking forward to further review comments! For now, maybe I can start to work on v2 if no big problem. If there is better way, v3 is ready to go. :) Peter
diff --git a/qapi-schema.json b/qapi-schema.json index 8d04897..81654bd 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -4083,3 +4083,20 @@ ## { 'enum': 'ReplayMode', 'data': [ 'none', 'record', 'play' ] } + +## +# @GICType: +# +# An enumeration of GIC types +# +# @gicv2: GICv2 support without kernel irqchip +# +# @gicv3: GICv3 support without kernel irqchip +# +# @gicv2-kvm: GICv3 support with kernel irqchip +# +# @gicv3-kvm: GICv3 support with kernel irqchip +# +# Since: 2.6 +## +{ 'enum': 'GICType', 'data': [ 'gicv2', 'gicv3', 'gicv2-kvm', 'gicv3-kvm' ] }
A new enum type is added to define ARM GIC types. Signed-off-by: Peter Xu <peterx@redhat.com> --- qapi-schema.json | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)