Re: [PATCH 2/5] KVM: Add irq ack notifier list

["Yang, Sheng" <sheng.yang@intel.com>]

On Tuesday 29 July 2008 17:34:47 Amit Shah wrote:
> * On Tuesday 29 Jul 2008 12:44:17 Yang, Sheng wrote:
> > On Tuesday 29 July 2008 00:26:25 Ben-Ami Yassour wrote:
> > > FROM: Avi Kivity <avi@qumranet.com>
> > >
> > > This can be used by kvm subsystems that are interested in when
> > > interrupts
> > > are acked, for example time drift compenstation.
> > >
> > > [Ben: add notification call to the pic and ioapic]
> > >
> > > Signed-off-by: Avi Kivity <avi@qumranet.com>
> > > Signed-off-by: Ben-Ami Yassour <benami@il.ibm.com>
> > > ---
> > >  arch/x86/kvm/i8259.c       |    1 +
> > >  arch/x86/kvm/irq.c         |   22 ++++++++++++++++++++++
> > >  arch/x86/kvm/irq.h         |    5 +++++
> > >  include/asm-x86/kvm_host.h |    7 +++++++
> > >  virt/kvm/ioapic.c          |    2 ++
> > >  5 files changed, 37 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> > > index 55e179a..d2a61bf 100644
> > > --- a/arch/x86/kvm/i8259.c
> > > +++ b/arch/x86/kvm/i8259.c
> > > @@ -186,6 +186,7 @@ int kvm_pic_read_irq(struct kvm_pic *s)
> > >  		irq = 7;
> > >  		intno = s->pics[0].irq_base + irq;
> > >  	}
> > > +	kvm_notify_acked_irq(s->irq_request_opaque, irq);
> >
> > It's not what I mean, sorry to not tell it clearly... Now it got
> > confusing semantic.
> >
> > irq_request_opaque has nothing to do with acked_irq. What I mean
> > is
>
> The change here uses the irq_request_opaque field which actually is
> the kvm struct, pointed out by you, thanks for that.

Yeah, I know that... I just meant the meaning of words has no 
relevant. :)
>
> > rename irq_request_opaque to struct* kvm in struct kvm_pic, and
> > modify all irq_request() calling(three of them in all) with (void
> > *)kvm.
>
> 'opaque' fields can be later made to point to other structures
> without changing the structure itself. This is an advantage. Will
> the kvm_pic struct be needed to change in the future? Very
> unlikely. So we can rename it to struct kvm *, however, that gives
> us no real benefit as against opaque (just readability).

Yes, the readability...

I think people would be very curious about why 

void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi)

got a irq_request_opaque as a parameter. It's more like a hack, which 
is not my meaning...

Anyway, it's trivial one and just a coding style. :)

--
regards
Yang, Sheng