Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Am 16.06.2013 21:25, schrieb Andreas Färber: Am 12.06.2013 22:32, schrieb Scott Wood: Enables support for the in-kernel MPIC that thas been merged into the KVM next branch. This includes irqfd/KVM_IRQ_LINE support from Alex Graf (along with some other improvements). Note from Alex regarding kvm_irqchip_create(): On x86, one would call kvm_irqchip_create() to initialize an in-kernel interrupt controller. That function then goes ahead and initializes global capability variables as well as the default irq routing table. On ppc, we can't call kvm_irqchip_create() because we can have different types of interrupt controllers. So we want to do all the things that function would do for us in the in-kernel device init handler. Signed-off-by: Scott Wood scottw...@freescale.com [...] --- default-configs/ppc-softmmu.mak |1 + default-configs/ppc64-softmmu.mak |1 + This breaks KVM-enabled ppcemb-softmmu build with unresolved symbol kvm_openpic_connect_vcpu() in e500.o. Fix in my patch: http://patchwork.ozlabs.org/patch/251731/ This issue brought up an interesting question: Why didn't the buildbots report this build breakage on ppc-next branch? Both of us didn't see any buildbot emails for quite a while - was this disabled for some reason? I didn't find the answer in https://github.com/b1-systems/buildbot/blob/master/qemu-master.cfg but I noticed that it still has Stefan's rather than Michael's trivial tree URL. Looking at http://buildbot.b1-systems.de/qemu/one_line_per_build I see that most builders fail due to configure - which I guess points at lack of compatible libfdt or dtc submodule. Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
On 06/16/2013 02:25:04 PM, Andreas Färber wrote: Subject is misleading: it's intc/openpic_kvm, not kvm/openpic. Alex, please fix when squashing. I meant it as a general description of the functional area, not as a literal pathname. It looks like that format is more of a Linux kernel thing and not used by QEMU much (if at all) -- sorry about that. Should have been kvm: openpic: in-kernel mpic support. -Scott
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
On 06/16/2013 02:11:58 PM, Andreas Färber wrote: Am 15.06.2013 00:57, schrieb Scott Wood: ...which of those would make me think hmm, there's something in here that I need to read before submitting patches for in-kernel mpic? I'm not trying to be difficult -- I'm just trying to point out that there's room for improvement in how the QEMU community communicates to developers what is expected. Maybe an announcement list that just contains important updates and summaries? +1 - but surely not all changes would get communicated on such a list. Sure. It doesn't need to be perfect to be a big help. And making other people even more work goes both ways... Right. But if you're complaining about QOM and QOM realize, address your complaints to Anthony instead. :) We're a community, and sending patches in write-only mode conflicts with my understanding of being e500 co-maintainer. It wasn't write-only mode -- I've accepted and acted on plenty of other feedback in this and other patchsets (in fact, some of that feedback told me specifically to use things like qdev_init_nofail, which apparently is deprecated). And I'm not opposed to cleaning up/modernizing existing e500 code (or delegating it to a coworker, which includes Alex, who works for us part-time) if it's clear what is expected. I was just looking for help with a part of QEMU that I find pretty opaque, and was put off by the tone of the initial complaint. I have no personal advantage of this e500 KVM PIC, it just makes more work for me. So it's your job to keep the code from bitrotting, especially when not everyone can actually compile-test it. Anyway, I have just sent Alex a fixup patch to squash as code says more than a thousand words - now you have no more excuses for the future. :P Thanks. When would kvm_openpic_realize/unrealize/finalize get called? Today realize is called as part of your qdev_init_nofail() or via object_property_set_bool(). In the future it will be called last thing before the machine starts executing - therefore moving basic initializations into instance_init. Documented in include/hw/qdev-core.h. Note that we must create the kernel side of the device in kvm_openpic_init(), because we need to return failure if it's not supported, so that the platform can fall back onto creating a normal QEMU openpic instead. No, you don't have to and you shouldn't. SysBusDeviceClass::init is legacy cruft. As you can see from my patch, kvm_openpic_realize allows for even better error reporting. My concern isn't how good the error reporting is, but how early we detect the error. If qdev_init_nofail() goes away, then when will platform code have a chance to create a non-KVM openpic device instead? From devices must not create children during @realize it sounds like it might be too late at that point. Originally, I had the creation of the kernel MPIC object done by a factory function. If the kernel object was able to be created, then the function created a qdev object and passed the kernel ID as a property. Otherwise, it returned NULL so that platform code knew to create a normal openpic device (or error out if the user explicitly asked for an in-kernel pic). Alex asked me to change this to the qdev_init_nofail() approach. It sounds like we may need to change back. -Scott
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Am 15.06.2013 00:57, schrieb Scott Wood:
On 06/14/2013 09:59:18 AM, Andreas Färber wrote:
Am 13.06.2013 19:32, schrieb Scott Wood:
On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
Am 12.06.2013 22:32, schrieb Scott Wood:
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
SysBusDevice parent_obj; please!
http://wiki.qemu.org/QOMConventions
The word QOMConventions doesn't exist once in the QEMU source tree.
How is one supposed to know that this documentation exists? :-P
I do expect people to read at least the subject of patch series on
qemu-devel,
I have over 12000 currently unread e-mails from that list.
FWIW I have over 53000 unread.
They are not
separated into patch and other. Even among the patches, they get
posted at least twice due to the (unique to QEMU and KVM as far as I can
tell) practice of reposting everything before a pull request.
There's just no way I can keep up with all of this, *plus* all the
non-QEMU stuff I need to keep up with. Sorry. I generally rely on Alex
to guide me on things like qdev/QOM. Quite frankly, I didn't even
realize I was using QOM. I thought this was still qdev. I even create
it using qdev_create()...
short of individual review comments. CPU, PReP PCI,
Versatile PCI, ISA and more recently virtio series had been posted.
...which of those would make me think hmm, there's something in here
that I need to read before submitting patches for in-kernel mpic?
I'm not trying to be difficult -- I'm just trying to point out that
there's room for improvement in how the QEMU community communicates to
developers what is expected. Maybe an announcement list that just
contains important updates and summaries?
+1 - but surely not all changes would get communicated on such a list.
Also, even starting on
http://wiki.qemu.org/ I don't see any obvious path to get to
QOMConventions. It's not even linked to from the main QOM page.
I do understand the frustration of having to correct people on the same
things over and over -- I experience it myself in other contexts. But
there are more constructive ways to deal with it than exclamation points.
Plus, this is just copied from the non-KVM MPIC file, and I see many
other instances throughout the source tree.
Which exactly is the reason for my grief: Your ignorance is making other
people even more work, and at least Alex should've spotted it - I can't
review all patches just because they might or might not be touching on
QOM.
Just as we are supposed to not copy old Coding Style in new patches, we
should be applying new patterns and conventions in new patches, too.
I'm usually the first person to complain about bad copy and paste, but
this is a situation where the KVM version of openpic is supposed to
interface with the rest of the system in exactly the same way as the
regular openpic. It really does not make sense to write the glue code
from scratch. And it's not as if the rest of QEMU had just been fixed,
and this patch reintroduces the old stuff. I count 166 instances of
SysBusDevice busdev and only 9 instances of SysBusDevice
parent_obj. Perhaps these could all be fixed up in an automated way?
No, they can't, because FROM_SYSBUS() needs to be replaced with
per-device macros like I asked you to introduce for your new device. And
what code to put in which function is not automatic either.
And making other people even more work goes both ways...
Right. But if you're complaining about QOM and QOM realize, address your
complaints to Anthony instead. :)
We're a community, and sending patches in write-only mode conflicts with
my understanding of being e500 co-maintainer. I have no personal
advantage of this e500 KVM PIC, it just makes more work for me. So it's
your job to keep the code from bitrotting, especially when not everyone
can actually compile-test it.
Anyway, I have just sent Alex a fixup patch to squash as code says more
than a thousand words - now you have no more excuses for the future. :P
+static int kvm_openpic_init(SysBusDevice *dev)
Please make this instance_init + realize functions - dev should
rather
be reserved for DeviceState.
Could you elaborate? I'm really not familiar with this stuff, and have
not found much documentation. Again, this is patterned after the
existing
non-KVM openpic file.
static void kvm_openpic_init(Object *obj) should initialize simple
variables, MemoryRegions that don't depend on parameters and any QOM
properties.
static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
initialize the rest.
kvm_openpic_unrealize(Device *dev, Error **errp) and
kvm_openpic_finalize(Object *obj) would be their counterparts for
cleanup.
When would kvm_openpic_realize/unrealize/finalize get called?
Today realize is called as part of your qdev_init_nofail() or via
object_property_set_bool().
In the future it will be called last thing before the machine starts
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Subject is misleading: it's intc/openpic_kvm, not kvm/openpic. Alex, please fix when squashing. Am 12.06.2013 22:32, schrieb Scott Wood: Enables support for the in-kernel MPIC that thas been merged into the KVM next branch. This includes irqfd/KVM_IRQ_LINE support from Alex Graf (along with some other improvements). Note from Alex regarding kvm_irqchip_create(): On x86, one would call kvm_irqchip_create() to initialize an in-kernel interrupt controller. That function then goes ahead and initializes global capability variables as well as the default irq routing table. On ppc, we can't call kvm_irqchip_create() because we can have different types of interrupt controllers. So we want to do all the things that function would do for us in the in-kernel device init handler. Signed-off-by: Scott Wood scottw...@freescale.com --- v2: fix llx - PRI_x64, and remove some broken leftover code involving reg_base. --- default-configs/ppc-softmmu.mak |1 + default-configs/ppc64-softmmu.mak |1 + This breaks KVM-enabled ppcemb-softmmu build with unresolved symbol kvm_openpic_connect_vcpu() in e500.o. Fix in my patch: http://patchwork.ozlabs.org/patch/251731/ Because intc/openpic.c gets rebuilt for each of the three ppc*-softmmu, I added a patch to my qom-cpu-10 series to stop that for openpic. For openpic_kvm I believe that won't be possible due to sysemu/kvm.h's inline stubs. Andreas hw/intc/Makefile.objs |1 + hw/intc/openpic_kvm.c | 250 + hw/ppc/e500.c | 79 +++- include/hw/ppc/openpic.h |2 +- 6 files changed, 328 insertions(+), 6 deletions(-) create mode 100644 hw/intc/openpic_kvm.c -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Am 13.06.2013 19:32, schrieb Scott Wood:
On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
Am 12.06.2013 22:32, schrieb Scott Wood:
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
SysBusDevice parent_obj; please!
http://wiki.qemu.org/QOMConventions
The word QOMConventions doesn't exist once in the QEMU source tree.
How is one supposed to know that this documentation exists? :-P
I do expect people to read at least the subject of patch series on
qemu-devel, short of individual review comments. CPU, PReP PCI,
Versatile PCI, ISA and more recently virtio series had been posted.
Some such review comments have been collected into the above Wiki page
because this is a recurring topic unfortunately.
Plus, this is just copied from the non-KVM MPIC file, and I see many
other instances throughout the source tree.
Which exactly is the reason for my grief: Your ignorance is making other
people even more work, and at least Alex should've spotted it - I can't
review all patches just because they might or might not be touching on QOM.
Just as we are supposed to not copy old Coding Style in new patches, we
should be applying new patterns and conventions in new patches, too.
+static int kvm_openpic_init(SysBusDevice *dev)
Please make this instance_init + realize functions - dev should rather
be reserved for DeviceState.
Could you elaborate? I'm really not familiar with this stuff, and have
not found much documentation. Again, this is patterned after the existing
non-KVM openpic file.
static void kvm_openpic_init(Object *obj) should initialize simple
variables, MemoryRegions that don't depend on parameters and any QOM
properties.
static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
initialize the rest.
kvm_openpic_unrealize(Device *dev, Error **errp) and
kvm_openpic_finalize(Object *obj) would be their counterparts for cleanup.
+{
+KVMState *s = kvm_state;
+KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead for
new devices - has been a topic for several weeks and months now.
There's way too much traffic on the list for me to know about something
just because it's been a topic. Lately it's been too much for me to
even scan the subject lines looking for things that look important,
given that QEMU is only a fraction of what I spend my time on.
If you'd like to update hw/intc/openpic.c to comply with the style of
the day, then this could be updated to match...
Also note that this is hardly the first time this patch has been posted
(v1 was a few weeks ago, and there were RFC patches well before that).
The first version may have even preceded this topic. This seems a bit
late in the process for a bunch of style churn, when existing code
hasn't been updated.
I'm not talking about style churn, I'm talking about using the wrong
infrastructure and making it even more difficult to drop FROM_SYSBUS()
macro.
Again, whether or not some particular other file uses some style doesn't
mean that it's okay to apply that to new files. Instead of complaining
it would've been a task of five minutes to supply Alex with a fixup
patch to squash/follow-up or to post a v3. QOM realize is merged since
January 2013 and was presented by Anthony in January 2012.
+int kvm_openpic_model;
+struct kvm_create_device cd = {0};
+int ret, i;
+
+if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
+return -EINVAL;
+}
+
+switch (opp-model) {
+case OPENPIC_MODEL_FSL_MPIC_20:
+kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
+break;
+
+case OPENPIC_MODEL_FSL_MPIC_42:
+kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
+break;
+
+default:
+return -EINVAL;
+}
If there's only two supported enum-style options, why not make it two
devices with the value set as a class field?
I'm not 100% sure what you mean here, but it is not intended that there
will always be only two supported options. At the least we will
probably support v4.3 at some point.
Afterwards I saw that you copied the model property from the non-KVM.
What I was pointing about here is that unlike qdev QOM allows to have
arbitrary levels of inheritence, i.e. you can have a kvm-openpic base
class and specialized subclasses like mipc42-kvm-openpic rather than
kvm-openpic,model=42 given that the user can't specify arbitrary
integers - whether two or three doesn't really matter in that respect.
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
On 06/14/2013 09:59:18 AM, Andreas Färber wrote:
Am 13.06.2013 19:32, schrieb Scott Wood:
On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
Am 12.06.2013 22:32, schrieb Scott Wood:
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
SysBusDevice parent_obj; please!
http://wiki.qemu.org/QOMConventions
The word QOMConventions doesn't exist once in the QEMU source
tree.
How is one supposed to know that this documentation exists? :-P
I do expect people to read at least the subject of patch series on
qemu-devel,
I have over 12000 currently unread e-mails from that list. They are
not separated into patch and other. Even among the patches, they
get posted at least twice due to the (unique to QEMU and KVM as far as
I can tell) practice of reposting everything before a pull request.
There's just no way I can keep up with all of this, *plus* all the
non-QEMU stuff I need to keep up with. Sorry. I generally rely on
Alex to guide me on things like qdev/QOM. Quite frankly, I didn't even
realize I was using QOM. I thought this was still qdev. I even create
it using qdev_create()...
short of individual review comments. CPU, PReP PCI,
Versatile PCI, ISA and more recently virtio series had been posted.
...which of those would make me think hmm, there's something in here
that I need to read before submitting patches for in-kernel mpic?
I'm not trying to be difficult -- I'm just trying to point out that
there's room for improvement in how the QEMU community communicates to
developers what is expected. Maybe an announcement list that just
contains important updates and summaries? Also, even starting on
http://wiki.qemu.org/ I don't see any obvious path to get to
QOMConventions. It's not even linked to from the main QOM page.
I do understand the frustration of having to correct people on the same
things over and over -- I experience it myself in other contexts. But
there are more constructive ways to deal with it than exclamation
points.
Plus, this is just copied from the non-KVM MPIC file, and I see many
other instances throughout the source tree.
Which exactly is the reason for my grief: Your ignorance is making
other
people even more work, and at least Alex should've spotted it - I
can't
review all patches just because they might or might not be touching
on QOM.
Just as we are supposed to not copy old Coding Style in new patches,
we
should be applying new patterns and conventions in new patches, too.
I'm usually the first person to complain about bad copy and paste, but
this is a situation where the KVM version of openpic is supposed to
interface with the rest of the system in exactly the same way as the
regular openpic. It really does not make sense to write the glue code
from scratch. And it's not as if the rest of QEMU had just been fixed,
and this patch reintroduces the old stuff. I count 166 instances of
SysBusDevice busdev and only 9 instances of SysBusDevice
parent_obj. Perhaps these could all be fixed up in an automated way?
And making other people even more work goes both ways...
+static int kvm_openpic_init(SysBusDevice *dev)
Please make this instance_init + realize functions - dev should
rather
be reserved for DeviceState.
Could you elaborate? I'm really not familiar with this stuff, and
have
not found much documentation. Again, this is patterned after the
existing
non-KVM openpic file.
static void kvm_openpic_init(Object *obj) should initialize simple
variables, MemoryRegions that don't depend on parameters and any QOM
properties.
static void kvm_openpic_realize(DeviceState *dev, Error **errp) should
initialize the rest.
kvm_openpic_unrealize(Device *dev, Error **errp) and
kvm_openpic_finalize(Object *obj) would be their counterparts for
cleanup.
When would kvm_openpic_realize/unrealize/finalize get called?
Note that we must create the kernel side of the device in
kvm_openpic_init(), because we need to return failure if it's not
supported, so that the platform can fall back onto creating a normal
QEMU openpic instead.
Also note that an in-kernel MPIC cannot be destroyed, without
destroying the entire VM. So I'm not sure what unrealize/finalize
would do.
All of this is basically done the way Alex told/showed me to do it.
+{
+KVMState *s = kvm_state;
+KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
NACK, please introduce your own KVM_OPENPIC(obj) cast macro
instead for
new devices - has been a topic for several weeks and months now.
There's way too much traffic on the list for me to know about
something
just because it's been a topic. Lately it's been too much for me
to
even scan the subject lines looking for things that look important,
given that QEMU is only a fraction of what I spend my time on.
If you'd like to update hw/intc/openpic.c to comply with the style
of
the day, then this could be updated to
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Am 12.06.2013 22:32, schrieb Scott Wood:
Enables support for the in-kernel MPIC that thas been merged into the
KVM next branch. This includes irqfd/KVM_IRQ_LINE support from Alex
Graf (along with some other improvements).
Note from Alex regarding kvm_irqchip_create():
On x86, one would call kvm_irqchip_create() to initialize an
in-kernel interrupt controller. That function then goes ahead and
initializes global capability variables as well as the default irq
routing table.
On ppc, we can't call kvm_irqchip_create() because we can have
different types of interrupt controllers. So we want to do all the
things that function would do for us in the in-kernel device init
handler.
Signed-off-by: Scott Wood scottw...@freescale.com
---
v2: fix llx - PRI_x64, and remove some broken leftover code
involving reg_base.
---
default-configs/ppc-softmmu.mak |1 +
default-configs/ppc64-softmmu.mak |1 +
hw/intc/Makefile.objs |1 +
hw/intc/openpic_kvm.c | 250
+
hw/ppc/e500.c | 79 +++-
include/hw/ppc/openpic.h |2 +-
6 files changed, 328 insertions(+), 6 deletions(-)
create mode 100644 hw/intc/openpic_kvm.c
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index cc3587f..63255dc 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -43,5 +43,6 @@ CONFIG_XILINX=y
CONFIG_XILINX_ETHLITE=y
CONFIG_OPENPIC=y
CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
# For PReP
CONFIG_MC146818RTC=y
diff --git a/default-configs/ppc64-softmmu.mak
b/default-configs/ppc64-softmmu.mak
index 884ea8a..e3c0c68 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -44,6 +44,7 @@ CONFIG_XILINX_ETHLITE=y
CONFIG_OPENPIC=y
CONFIG_PSERIES=$(CONFIG_FDT)
CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
# For pSeries
CONFIG_PCI_HOTPLUG=y
# For PReP
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 718d97a..837ef19 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -20,4 +20,5 @@ obj-$(CONFIG_GRLIB) += grlib_irqmp.o
obj-$(CONFIG_IOAPIC) += ioapic.o
obj-$(CONFIG_OMAP) += omap_intc.o
obj-$(CONFIG_OPENPIC) += openpic.o
+obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
obj-$(CONFIG_SH4) += sh_intc.o
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
new file mode 100644
index 000..809b34b
--- /dev/null
+++ b/hw/intc/openpic_kvm.c
@@ -0,0 +1,250 @@
+/*
+ * KVM in-kernel OpenPIC
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
copy
+ * of this software and associated documentation files (the Software), to
deal
+ * in the Software without restriction, including without limitation the
rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include sys/ioctl.h
+#include exec/address-spaces.h
+#include hw/hw.h
+#include hw/ppc/openpic.h
+#include hw/pci/msi.h
+#include hw/sysbus.h
+#include sysemu/kvm.h
+#include qemu/log.h
+
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
SysBusDevice parent_obj; please!
http://wiki.qemu.org/QOMConventions
+MemoryRegion mem;
+MemoryListener mem_listener;
+uint32_t fd;
+uint32_t model;
+} KVMOpenPICState;
+
+static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level)
+{
+kvm_set_irq(kvm_state, n_IRQ, level);
+}
+
+static void kvm_openpic_reset(DeviceState *d)
+{
+qemu_log_mask(LOG_UNIMP, %s: unimplemented\n, __func__);
+}
+
+static void kvm_openpic_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{
+KVMOpenPICState *opp = opaque;
+struct kvm_device_attr attr;
+uint32_t val32 = val;
+int ret;
+
+attr.group = KVM_DEV_MPIC_GRP_REGISTER;
+attr.attr = addr;
+attr.addr = (uint64_t)(unsigned long)val32;
+
+ret = ioctl(opp-fd, KVM_SET_DEVICE_ATTR, attr);
+
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
On 06/13/2013 06:01:49 AM, Andreas Färber wrote:
Am 12.06.2013 22:32, schrieb Scott Wood:
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
SysBusDevice parent_obj; please!
http://wiki.qemu.org/QOMConventions
The word QOMConventions doesn't exist once in the QEMU source tree.
How is one supposed to know that this documentation exists? :-P
Plus, this is just copied from the non-KVM MPIC file, and I see many
other instances throughout the source tree.
+static int kvm_openpic_init(SysBusDevice *dev)
Please make this instance_init + realize functions - dev should
rather
be reserved for DeviceState.
Could you elaborate? I'm really not familiar with this stuff, and have
not found much documentation. Again, this is patterned after the
existing
non-KVM openpic file.
+{
+KVMState *s = kvm_state;
+KVMOpenPICState *opp = FROM_SYSBUS(typeof(*opp), dev);
NACK, please introduce your own KVM_OPENPIC(obj) cast macro instead
for
new devices - has been a topic for several weeks and months now.
There's way too much traffic on the list for me to know about something
just because it's been a topic. Lately it's been too much for me to
even scan the subject lines looking for things that look important,
given that QEMU is only a fraction of what I spend my time on.
If you'd like to update hw/intc/openpic.c to comply with the style of
the day, then this could be updated to match...
Also note that this is hardly the first time this patch has been posted
(v1 was a few weeks ago, and there were RFC patches well before that).
The first version may have even preceded this topic. This seems a
bit late in the process for a bunch of style churn, when existing code
hasn't been updated.
+int kvm_openpic_model;
+struct kvm_create_device cd = {0};
+int ret, i;
+
+if (!kvm_check_extension(s, KVM_CAP_DEVICE_CTRL)) {
+return -EINVAL;
+}
+
+switch (opp-model) {
+case OPENPIC_MODEL_FSL_MPIC_20:
+kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_20;
+break;
+
+case OPENPIC_MODEL_FSL_MPIC_42:
+kvm_openpic_model = KVM_DEV_TYPE_FSL_MPIC_42;
+break;
+
+default:
+return -EINVAL;
+}
If there's only two supported enum-style options, why not make it two
devices with the value set as a class field?
I'm not 100% sure what you mean here, but it is not intended that there
will always be only two supported options. At the least we will
probably support v4.3 at some point.
-Scott
[Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
Enables support for the in-kernel MPIC that thas been merged into the
KVM next branch. This includes irqfd/KVM_IRQ_LINE support from Alex
Graf (along with some other improvements).
Note from Alex regarding kvm_irqchip_create():
On x86, one would call kvm_irqchip_create() to initialize an
in-kernel interrupt controller. That function then goes ahead and
initializes global capability variables as well as the default irq
routing table.
On ppc, we can't call kvm_irqchip_create() because we can have
different types of interrupt controllers. So we want to do all the
things that function would do for us in the in-kernel device init
handler.
Signed-off-by: Scott Wood scottw...@freescale.com
---
v2: fix llx - PRI_x64, and remove some broken leftover code
involving reg_base.
---
default-configs/ppc-softmmu.mak |1 +
default-configs/ppc64-softmmu.mak |1 +
hw/intc/Makefile.objs |1 +
hw/intc/openpic_kvm.c | 250 +
hw/ppc/e500.c | 79 +++-
include/hw/ppc/openpic.h |2 +-
6 files changed, 328 insertions(+), 6 deletions(-)
create mode 100644 hw/intc/openpic_kvm.c
diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index cc3587f..63255dc 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -43,5 +43,6 @@ CONFIG_XILINX=y
CONFIG_XILINX_ETHLITE=y
CONFIG_OPENPIC=y
CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
# For PReP
CONFIG_MC146818RTC=y
diff --git a/default-configs/ppc64-softmmu.mak
b/default-configs/ppc64-softmmu.mak
index 884ea8a..e3c0c68 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -44,6 +44,7 @@ CONFIG_XILINX_ETHLITE=y
CONFIG_OPENPIC=y
CONFIG_PSERIES=$(CONFIG_FDT)
CONFIG_E500=$(CONFIG_FDT)
+CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
# For pSeries
CONFIG_PCI_HOTPLUG=y
# For PReP
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 718d97a..837ef19 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -20,4 +20,5 @@ obj-$(CONFIG_GRLIB) += grlib_irqmp.o
obj-$(CONFIG_IOAPIC) += ioapic.o
obj-$(CONFIG_OMAP) += omap_intc.o
obj-$(CONFIG_OPENPIC) += openpic.o
+obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
obj-$(CONFIG_SH4) += sh_intc.o
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
new file mode 100644
index 000..809b34b
--- /dev/null
+++ b/hw/intc/openpic_kvm.c
@@ -0,0 +1,250 @@
+/*
+ * KVM in-kernel OpenPIC
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the Software), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include sys/ioctl.h
+#include exec/address-spaces.h
+#include hw/hw.h
+#include hw/ppc/openpic.h
+#include hw/pci/msi.h
+#include hw/sysbus.h
+#include sysemu/kvm.h
+#include qemu/log.h
+
+typedef struct KVMOpenPICState {
+SysBusDevice busdev;
+MemoryRegion mem;
+MemoryListener mem_listener;
+uint32_t fd;
+uint32_t model;
+} KVMOpenPICState;
+
+static void kvm_openpic_set_irq(void *opaque, int n_IRQ, int level)
+{
+kvm_set_irq(kvm_state, n_IRQ, level);
+}
+
+static void kvm_openpic_reset(DeviceState *d)
+{
+qemu_log_mask(LOG_UNIMP, %s: unimplemented\n, __func__);
+}
+
+static void kvm_openpic_write(void *opaque, hwaddr addr, uint64_t val,
+ unsigned size)
+{
+KVMOpenPICState *opp = opaque;
+struct kvm_device_attr attr;
+uint32_t val32 = val;
+int ret;
+
+attr.group = KVM_DEV_MPIC_GRP_REGISTER;
+attr.attr = addr;
+attr.addr = (uint64_t)(unsigned long)val32;
+
+ret = ioctl(opp-fd, KVM_SET_DEVICE_ATTR, attr);
+if (ret 0) {
+qemu_log_mask(LOG_UNIMP, %s: %s % PRIx64 \n, __func__,
+ strerror(errno), attr.attr);
+}
+}
+
+static uint64_t kvm_openpic_read(void *opaque, hwaddr addr, unsigned size)
+{
+KVMOpenPICState *opp =
Re: [Qemu-devel] [PATCH v2] kvm/openpic: in-kernel mpic support
On 12.06.2013, at 22:32, Scott Wood wrote: Enables support for the in-kernel MPIC that thas been merged into the KVM next branch. This includes irqfd/KVM_IRQ_LINE support from Alex Graf (along with some other improvements). Note from Alex regarding kvm_irqchip_create(): On x86, one would call kvm_irqchip_create() to initialize an in-kernel interrupt controller. That function then goes ahead and initializes global capability variables as well as the default irq routing table. On ppc, we can't call kvm_irqchip_create() because we can have different types of interrupt controllers. So we want to do all the things that function would do for us in the in-kernel device init handler. Signed-off-by: Scott Wood scottw...@freescale.com Thanks, applied to ppc-next. Alex
