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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Feb 21, 2013 at 08:00:25PM -0600, Scott Wood wrote:
> 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.

Can you describe usage of this register again?

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

VCPUS access in-kernel devices. Yes, it is lockless (see RCU usage in
virt/kvm/).

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

Perhaps a better way to put it is that its too permissive.

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

Namespace conflicts: Reserve ranges for each arch. 

The other two items, haven't though. I am not the one bothered :-) (yes, they
suck).

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

Note just pointing out drawbacks of device attributes (if something of
that sort is integrated, x86 should also use it).