On 02/20/2013 03:28:24 PM, Marcelo Tosatti wrote:
On Wed, Feb 20, 2013 at 03:09:49PM +0200, Gleb Natapov wrote:
> On Tue, Feb 19, 2013 at 03:16:37PM -0600, Scott Wood wrote:
> > On 02/19/2013 06:24:18 AM, Gleb Natapov wrote:
> > >On Mon, Feb 18, 2013 at 05:01:40PM -0600, Scott Wood wrote:
> > >> The ability to set/get attributes is needed. Sorry, but "get
or set
> > >> one blob of data, up to 512 bytes, for the entire irqchip" is
just
> > >> not good enough -- assuming you don't want us to start sticking
> > >> pointers and commands in *that* data. :-)
> > >>
> > >Proposed interface sticks pointers into ioctl data, so why doing
> > >the same
> > >for KVM_SET_IRQCHIP/KVM_GET_IRQCHIP makes you smile.
> >
> > There's a difference between putting a pointer in an ioctl control
> > structure that is specifically documented as being that way (as in
> > ONE_REG), versus taking an ioctl that claims to be
setting/getting a
> > blob of state and embedding pointers in it. It would be like
> > sticking a pointer in the attribute payload of this API, which I
> > think is something to be discouraged.
> If documentation is what differentiate for you between silly and
smart
> then write documentation instead of new interfaces.
>
> KVM_SET_IRQCHIP/KVM_GET_IRQCHIP is defined to operate on blob of
data on
> x86, nothing prevent you from adding MPIC specifics to the
interface,
> Add mpic state into kvm_irqchip structure and if 512 bytes is not
enough
> for you to transfer the state put pointers there and _document_
them.
> But with 512 bytes you can transfer properties inline, so you
probably
> do not need pointer there anyway. I see you have three properties 2
of
> them 32bit and one 64bit.
>
> > It'd also be using
> > KVM_SET_IRQCHIP to read data, which is the sort of thing you
object
> > to later on regarding KVM_IRQ_LINE_STATUS.
> >
> Do not see why.
>
> > Then there's the silliness of transporting 512 bytes just to read
a
> > descriptor for transporting something else.
> >
> Yes, agree. But is this enough of a reason to introduce entirely new
> interface? Is it on performance critical path? Doubt it, unless you
> abuse the interface to send interrupts, but then isn't it silty to
> do copy_from_user() twice to inject an interrupt like proposed
interface
> does?
>
> > >For signaling irqs (I think this is what you mean by "commands")
> > >we have KVM_IRQ_LINE.
> >
> > It's one type of command. Another is setting the address.
Another
> > is writing to registers that have side effects (this is how MSI
> > injection is done on MPIC, just as in real hardware).
> >
> Setting the address is setting an attribute. Sending MSI is a
command.
> Things you set/get during init/migration are attributes. Things you
do
> to cause side-effects are commands.
Yes, it would be good to restrict what can be done via the interface
(to avoid abstracting away problems). At a first glance, i would say
it should allow for "initial configuration of device state", such as
registers etc.
Why exactly you need to set attributes Scott?
That's best answered by looking at patch 6/6 and discussing the actual
attributes that are defined so far.
The need to set the base address of the registers is straightforward.
When ARM added their special ioctl for this, we discussed it being
eventually replaced with a more general API (in fact, that was the
reason they put "ARM" in the name).
Access to device registers was originally intended for debugging, and
eventually migration. It turned out to also be very useful for
injecting MSIs -- nothing special needed to be done. It Just Works(tm)
the same way it does in hardware, with an MMIO write from a PCI device
to a specific MPIC register. Again, irqfd may complicate this, but
there's no need for QEMU-generated MSIs to have to take a circuitous
path.
Finally, there's the interrupt source attribute. Even if we get rid of
that, we'll need MPIC-specific documentation on how to flatten the IRQ
numberspace, and if we ever have a cascaded PIC situation things could
get ugly since there's no way to identify a specific IRQ controller in
KVM_IRQ_LINE.
Also related to this question is the point of avoiding the
implementation of devices to be spread across userspace and the kernel
(this is one point Avi brought up often). If the device emulation
is implemented entirely in the kernel, all is necessary are initial
values of device registers (and retrieval of those values later for
savevm/migration).
MPIC emulation is entirely in the kernel with this patchset -- though
the register that lets you reset cores will likely need to be kicked
back to QEMU.
It is then not necessary to set device attributes on a live guest and
deal with the complications associated with that.
Which complications?
-Scott
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
