Re: [RFC PATCH 1/6] kvm: add device control API

[Scott Wood <scottwood@freescale.com>]

On 02/21/2013 05:03:32 PM, Marcelo Tosatti wrote:
> On Wed, Feb 20, 2013 at 07:28:52PM -0600, Scott Wood wrote:
> > On 02/20/2013 06:14:37 PM, Marcelo Tosatti wrote:
> > >On Wed, Feb 20, 2013 at 05:53:20PM -0600, Scott Wood wrote:
> > >> >It is then not necessary to set device attributes on a live
> > >guest and
> > >> >deal with the complications associated with that.
> > >>
> > >> Which complications?
> > >>
> > >> -Scott
> > >
> > >Semantics of individual attribute writes, for one.
> >
> > When the attribute is a device register, the hardware documentation
> > takes care of that.
> 
> You are not writing to the registers from the CPU point of view.

That's exactly how KVM_DEV_MPIC_GRP_REGISTER is defined and implemented  
on MPIC (with the exception of registers whose behavior changes based  
on which specific vcpu you use to access them).  If/when we have a need  
to set/get state in a different manner, that's a separate attribute  
group.

> > Otherwise, the semantics are documented in the
> > device-specific documentation -- which can include restricting when
> > the access is allowed.  Same as with any other interface
> > documentation.
> 
> Again, you are talking about the semantics of device access from the
> software operating on the machine view. We are discussing hypervisor
> userspace <-> hypervisor kernel interface.

And I was talking about the userspace-to-hypervisor kernel interface  
documentation.  It just happens that one specific MPIC device group  
("when the attribute is a device register") is defined with respect to  
what guest software would see if it did a similar access.

> In general you never have to set attributes on a device after it has
> been initialized, because there is state associated with that device
> that requires proper handling (example: if you modify a timer counter
> register of a timer device, any software timers used to emulate the
> timer counter must be cancelled).

Yes, it requires proper handling and the MMIO code does that.

If and when we add raw state accessors, it's totally reasonable for  
there to be command/attribute-specific documented restrictions on when  
the access can be done.

> Also, it is necessary to provide proper locking of device attribute
> write versus vcpu device access. So far we have been focusing on  
> having
> a lockless vcpu path.

How is device access related to vcpus?  Existing irqchip code is not  
lockless.

> So when device attributes can be modified has implications beyond what
> may seem visible at first.
> 
> Are this reasonable arguments?
> 
> Basically abstract 'device attributes' are too abstract.

It's up to the device-specific documentation to make them not abstract  
(I admit there are a few details missing in mpic.txt that I've pointed  
out in this thread -- it is RFC v1 after all).  This wouldn't be any  
different if we used separate ioctls for everything.  It's like saying  
abstract 'ioctl' is too abstract.

> However, your proposed interface deals with sucky capability,  
> versioning
> and namespace conflicts we have now. Note these items can probably be
> improved separately.

Any particular proposals?

> > I suppose mpic.txt could use an additional statement that
> > KVM_DEV_MPIC_GRP_REGISTER performs an access as if it were performed
> > by the guest.
> >
> > >Locking versus currently executing VCPUs, for another (see how
> > >KVM_SET_IRQ's RCU usage, for instance, that is something should be
> > >shared).
> >
> > If you mean kvm_set_irq() in irq_comm.c, that's only relevant when
> > you have a GSI routing table, which this patchset doesn't.
> >
> > Assuming we end up having a routing table to support irqfd, we still
> > can't share the code as is, since it's APIC-specific.
> 
> Suppose it is worthwhile to attempt to share code as much as possible.

Sure... my point is it isn't a case of "the common code is right over  
there, why aren't you using it?"  I'll try to share what I reasonably  
can, subject to my limited knowledge of how the APIC stuff works.  The  
irqfd code is substantial enough that refactoring for sharing should be  
worthwhile.  I'm not so sure about irq_comm.c.

-scott