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
