>>> On Sun, Apr 8, 2007 at 5:58 AM, in message
<[EMAIL PROTECTED]>, Christoph Hellwig <[EMAIL PROTECTED]>
wrote:
>> diff -- git a/drivers/kvm/kvm_irqdevice.h b/drivers/kvm/kvm_irqdevice.h
>> new file mode 100644
>> index 0000000..9c91d15
>> --- /dev/null
>> +++ b/drivers/kvm/kvm_irqdevice.h
>> @@ - 0,0 +1,206 @@
>> +/*
>> + * kvm_irqdevice.h
>
> No need to mention the file name in the top of file comment. Quite
> contrary,
> this comment can easily get out of sync and thus is not encouraged.
Ack
>
>> + *
>> + * Defines an interface for an abstract interrupt controller. The model
>> + * consists of a unit with an arbitrary number of input lines (IRQ0- N), an
>> + * output line (INTR), and methods for completing an interrupt- acknowledge
>> + * cycle (INTA). A particular implementation of this model will define
>> + * various policies, such as irq- to- vector translation, INTA/auto- EOI
>> policy,
>> + * etc.
>> + *
>> + * In addition, the INTR callback mechanism allows the unit to be "wired"
> to
>> + * an interruptible source in a very flexible manner. For instance, an
>> + * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another
>> + * interrupt controller (ala cascaded i8259s)
>> + *
>> + * Copyright (C) 2007 Novell
>> + *
>> + * Authors:
>> + * Gregory Haskins <[EMAIL PROTECTED]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along
> with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59
> Temple
>> + * Place - Suite 330, Boston, MA 02111- 1307 USA.
>
> It would be nice if you could follow the top of file comment style used
> in the rest of the kvm code instead of using a very different one.
I actually copied this header from one of the KVM files, but I will take a
deeper look to see if I can figure out what you are talking about. Perhaps you
are referring to how I described the file, rather than the other details like
the GPL section, etc.
>
>> +/*
>> + * kvm_irq_init()
>> + *
>> + * Description:
>> + * Initialized the kvm_irqdevice for use. Should be called before
> calling
>> + * any derived implementation init functions
>> + *
>> + * Parameters:
>> + * struct kvm_irqdevice *dev: This device
>> + *
>> + * Returns: (void)
>> + */
>
> Please use kernel- doc style comments that can be used to auto- generate
> documentation.
I can do this if its the right thing to do...but isnt that for general kernel
API? This stuff is really just private to KVM. Please advise.
>
>> +static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
>> +{
>> + return dev- >pending(dev, flags);
>> +}
I respectfully disagree with these next set of objections related to the
wrappers. I will try to detail the reasons why
>
> This wrapper is not needed at all, just call the method directly.
I would agree with you if were were talking about disparate pointers: e.g.
kvm_arch_ops for function context and kvm_vcpu for data context. However, in
this case we have the same pointer for both (e.g. we are emulating the
"implicit this" from C++). The two *always* being the same is an invariant
that this wrapper helps to enforce. Lack of this wrapper is just asking for a
bug, IMHO.
>
>> +static inline int kvm_irq_read_vector(struct kvm_irqdevice *dev, int flags)
>> +{
>> + return dev- >read_vector(dev, flags);
>> +}
>
> ditto
>
>> +static inline int kvm_irq_set_pin(struct kvm_irqdevice *dev, int pin,
>> + int level, int flags)
>> +{
>> + return dev- >set_pin(dev, pin, level, flags);
>> +}
>
> ditto
>
>> +static inline int kvm_irq_summary(struct kvm_irqdevice *dev, void *data)
>> +{
>> + return dev- >summary(dev, data);
>> +}
>
> ditto.
>
>> +static inline void kvm_irq_register_sink(struct kvm_irqdevice *dev,
>> + const struct kvm_irqsink *sink)
>> +{
>> + dev- >sink = *sink;
>> +}
>
> Completely useless wrapper, just do the assignment in the caller.
This has to do with encapsulation. The fact that the data is stored in
dev->sink is an implementation detail that I prefer to see hidden by the
interface. For instance, what if someone changes their mind about the current
policy of a singleton callback (e.g. allow multiple callback registrations)?
Or what if someone wants to add error checking to make sure a callback is
registered only once?
> Also the convention of copying the operation vectors is rather unnatural
> for linux, just set the pointer (and make sure the operations regustired
> are file- scope not local- scope as in your current code)
Ack.
>
>> +static inline void kvm_irq_raise_intr(struct kvm_irqdevice *dev)
>> +{
>> + struct kvm_irqsink *sink = &dev- >sink;
>> + if (sink- >raise_intr)
>> + sink- >raise_intr(sink, dev);
>> +}
>
> Similarly this is rather pointless and could be opencoded in the only
> caller.
Keep in mind that this one patch from a series. There are other consumers
downstream in my queue that will use this as well. Writing this code to check
if there is a callback over and over is a pain and unnecessary. I am trying to
model a pub/sub interface where the publisher need not care about the presence
of subscribers. In addition, this touches upon my first objection where the
invariant enforcement of sink->raise_int(sink, ...) is desirable.
>
>> +/*----------------------------------------------------------------------
>> + * optimized bitarray object - works like bitarrays in bitops, but uses
>> + * a summary field to accelerate lookups. Assumes external locking
>> + *--------------------------------------------------------------------- */
>> +
>> +struct bitarray {
>> + unsigned long summary; /* 1 per word in pending */
>> + unsigned long pending[NR_IRQ_WORDS];
>> +};
>> +
>> +static inline int bitarray_pending(struct bitarray *this)
>> +{
>> + return this- >summary ? 1 : 0;
>> +}
>> +
>> +static inline int bitarray_findhighest(struct bitarray *this)
>> +{
>> + if (!this- >summary)
>> + return - 1;
>> + else {
>> + int word_index = __ffs(this- >summary);
>> + int bit_index = __ffs(this- >pending[word_index]);
>> +
>> + return word_index * BITS_PER_LONG + bit_index;
>> + }
>> +}
>> +
>> +static inline void bitarray_set(struct bitarray *this, int nr)
>> +{
>> + __set_bit(nr, &this- >pending);
>> + __set_bit(nr / BITS_PER_LONG, &this- >summary);
>> +}
>> +
>> +static inline void bitarray_clear(struct bitarray *this, int nr)
>> +{
>> + int word = nr / BITS_PER_LONG;
>> +
>> + __clear_bit(nr, &this- >pending);
>> + if (!this- >pending[word])
>> + __clear_bit(word, &this- >summary);
>> +}
>> +
>> +static inline int bitarray_test(struct bitarray *this, int nr)
>> +{
>> + return test_bit(nr, &this- >pending);
>> +}
>
> This should go into a separate header, probably even in include/linux/
Avi has already addressed this
>
>> +/*----------------------------------------------------------------------
>> + * userint interface - provides the actual kvm_irqdevice implementation
>> + *--------------------------------------------------------------------- */
>> +
>> +typedef struct {
>> + spinlock_t lock;
>> + struct bitarray irq_pending;
>> + int nmi_pending;
>> +}kvm_userint;
>
> Please just use a struct type instead of the typedef.
Ack
>
>> +int kvm_userint_init(struct kvm_irqdevice *dev)
>> +{
>> + kvm_userint *s;
>> +
>> + s = (kvm_userint *)kzalloc(sizeof(kvm_userint), GFP_KERNEL);
>
> No need to case the kzalloc return value. But you need to check
> the return value for NULL and handle the error.
Ack
Thanks for the review. Greatly appreciated.
Regards,
-Greg
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
