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

[Scott Wood <scottwood@freescale.com>]

On 02/20/2013 06:01:00 PM, Marcelo Tosatti wrote:
> The main problem, to me, is that the interface allows flexibility but
> the details of using such flexibility are not worked out.
> 
> That is, lack of definitions such as when setting attributes is  
> allowed.

That is defined by the individual attributes.

There is one constraint that I did forget to indicate in mpic.txt --  
KVM_DEV_MPIC_GRP_REGISTER can only be used when the registers are  
mapped.

> With a big blog, that information is implicit: a SET operation is a  
> full
> device reset.

So, for example, how would I handle the guest changing the location of  
the MPIC registers dynamically (similar to changing a PCI BAR)?  QEMU  
doesn't implement this currently, but the hardware does, so the kernel  
interface shouldn't preclude it.

What if we later discover that we need some extra bit of state that  
wasn't included in the initial definition?  Especially since we don't  
do migration at all yet on ppc, so we can't even use what QEMU  
currently saves as a reference.

What if a particular array within the state blob needs to be scaled up  
because we now have more sources?

Will we need to version the blob?  It just gets to be a mess, like  
SREGS.

And then we need special code for packing/unpacking the blob, whereas  
with this patchset we reuse the same emulation code guest MMIO uses.

> With individual attributes:
> - Which attributes can be set individually?
> - Is there an order which must be respected?

If there are constraints, the device documentation should specify them.

> - Locking.

Same locking I need for MMIO and interrupt injection via other paths.

> - End result: more logic/code necessary.

That's a tradeoff for individual devices to make, in cases where it's  
actually true.

> > It's not about how often we do it, but how much state we have,
> > especially if we ever want to implement migration.
> 
> Migration reads/writes the device state once, which is supposedly much
> smaller than VM's RAM, so can't see the logic here.

struct kvm_irqchip can only hold 512 bytes.  We have more state than  
that.

-Scott