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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.

> 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.

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).

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.

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.

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

> 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.