On 24.02.2013, at 16:46, Gleb Natapov wrote: > On Thu, Feb 21, 2013 at 08:17:54PM -0600, Scott Wood wrote: >> On 02/21/2013 02:22:09 AM, Gleb Natapov wrote: >>> On Wed, Feb 20, 2013 at 08:05:12PM -0600, Scott Wood wrote: >>>> On 02/20/2013 07:09:49 AM, 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. >>>> >>>> You mean like what we did with SREGS, that got deprecated and >>>> replaced with ONE_REG? >>>> >>> SREGS is implemented by ppc. I see ONE_REG as addition to REGS >>> interface. You can access all register at once or you can access them >>> one by one. If there is a need we can add MULTIPLE_REGS that will get >>> list of requested REGS. >> >> http://www.spinics.net/lists/kvm-ppc/msg04876.html >> http://www.spinics.net/lists/kvm-ppc/msg05842.html >> > If Alex prefers ONE_REG interface this is his call :) There's a reason I prefer it :). We ran out of space in the SREGS call, we had a bit map with multiple chunks of data you could set in there and worst of all, when we ran out of space we didn't realize it until the code was already in Linus' tree! The normal "here's a chunk of data" ioctl style works great when you know all or most of the details of what you want to transfer. In the case of kvm, we need to expose an interface we a) Don't control itself. Hardware guys make hardware. We just follow. b) Is pretty complex. There can be a lot of state in a CPU / device. So because if that we needed something more flexible and dynamic. Something where we could add registers we forgot without jumping through lots of hoops. That's why I prefer ONE_REG for vcpus. Since I see the same danger on device emulation, I tend to prefer it here as well. > >>> The interface is not over generic i.e it does >>> not try to replace KVM_RUN by writing special register. >> >> Sigh. >> >>>> And we >>>> don't necessarily want to set *everything*. >>> What are those cases? You do need to on reset/migration. >> >> Why do we want to set all the registers on reset, rather than tell >> the in-kernel device to reset? The default state came from the >> kernel in the first place on irqchip creation... >> > I have nothing against telling in-kernel device to reset provided there > is a way to do so, which current interface lacks. Reset in userspase has > its advantage too: bugs are easier to fix, there may be different kind > of resets (hard/soft). The same argument holds for state access. If you suddenly realize "oh, I need to also set this one hidden state bit in my hardware" you only want to add that specific write on reset. If you suddenly find a race condition in migration that happens very rarely, you want to add the one register that you forgot to synchronize in addition in your new code. You don't want to change the whole ioctl passed struct. That's why we have the padding in our ioctl structs. And feature bitmaps. And once we exceed them, we're screwed. I want to make sure we don't hit that point. Actually, there's a really easy way to understand what this interface does. It basically implements a state get/set ioctl which allows for very fine grained control over your ioctl struct. It does that by only passing one register at a time. If we run into concurrency issues (multiple registers need to be set in a locked manner at the same time), we can always add a new ioctl that sets multiple attrs/regs at once. For CPUs, we haven't hit that case yet. > >>>>>> 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. >>>> >>>> It's either that, or have the data direction of the "chip" field in >>>> KVM_GET_IRQCHIP not be entirely in the "get" direction. >>>> >>> Still do not follow. Example? >> >> struct kvm_irqchip has "chip_id", "pad", and "chip". "pad" is >> insufficient to communicate attribute type plus a pointer. So if we >> want to provide a pointer for the kernel to write the attribute >> into, it has to read from memory that the ioctl definition suggests >> should only be written to. > Yes, but this is not different from the interface you propose. > >> >>>>> 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. >>>> >>>> What if I set the address at a time that isn't init/migration (the >>>> hardware allows moving it, like a PCI BAR)? Suddenly it becomes not >>>> an attribute due to how the caller uses it? >>>> >>> What's the interface for guest to move it? >> >> Some non-MPIC registers called CCSRBARH, CCSRBARL, and CCSRBAR. >> >>> Why it goes via userspace? >> >> Because the mechanism in question doesn't just move MPIC. It moves >> a big block of a bunch of different devices all at once. >> >>> You can move APIC base too, but this does not involve userspace. But >>> even if you do go via userspace, it is just a guest asking to >>> change device >>> configuration, so using SET_ATTR to set new configuration is fine. >>> >>>>>> What is the benefit of KVM_IRQ_LINE over what MPIC does? >>> What real >>>>>> (non-glue/wrapper) code can become common? >>>>>> >>>>> No new ioctl with exactly same result (well actually even >>> faster since >>>>> less copying is done). >>>> >>>> Which ioctl would go away? >>>> >>> Those that you propose in your new interface. >> >> No, they wouldn't. At most one MPIC attribute group would go away >> (though as I've noted it would still be useful to be able to "get" >> those attributes for debugging). >> >>>>>> And I really hope you don't want us to do MSIs the x86 way. >>>>>> >>>>> What is wrong with KVM_SIGNAL_MSI? Except that you'll need code to >>>>> glue it >>>>> to mpic. >>>> >>>> We'll have to write extra code for it compared to the current way >>>> where it works with *zero* code beyond what is wanted for other >>>> purposes such as debug and (eventually) migration. At least it's >>>> more direct than having to establish a GSI route... >>> If just writing a register cause MSI to be send how do you distinguish >>> between write that should send MSI and write that is done on migration >>> to transfer current value? >> >> It is a write-only command register. The registers that contain the >> state are elsewhere. >> > The register may be write-only from OS point of view, but its internal > state may still need to be transfered on migration. This brings us back > to the point that state and commands should have different accessors. Scott expressed this through his "groups". Hidden registers are a separate group, but still accessed using the same ideology of one register at a time. I think that makes sense. > >> Again, we do not currently support migration on MPIC. It is a very >> low priority for embedded. We do not wish to rule it out entirely, >> but it most likely would require adding more state accesors. > The interface suppose to be generic, we are not talking about MPIC > specifically here. Regarding migration "never say never" :) > >> >>> We had that problem with MSRs on x86. We had >>> to, eventually, add a flag that tells us the reason of MSR access. >> >> The equivalent to that flag would be using the right kind of >> accessor for what you want to do (simulated guest access versus >> backdoor state access). > Ugh. If we need to modify hidden state for migration, we can always create hidden registers in this interface. We won't know until we actually implement it. And then a new MPIC revision gets released and we need to do things all over again. The point of a generic get/set interface really is that we can change and extend the type of state we move between user and kernel space. > >> >>>>>> In the XICS thread, Paul brought up the possibliity of cascaded >>>>>> MPICs. It's not relevant to the systems we're trying to >>> model, but >>>>>> if one did want to use the in-kernel irqchip interface for >>> that, it >>>>>> would be really nice to be able to operate on a specific MPIC for >>>>>> injection rather than have to come up with some sort of global >>>>>> identifier (above and beyond the minor flattening we'd need >>> to do to >>>>>> represent a single MPIC's interrupts in a flat numberspace). >>>>>> >>>>> ARM encodes information in irq field of KVM_IRQ_LINE like that: >>>>> bits: | 31 ... 24 | 23 ... 16 | 15 ... 0 | >>>>> field: | irq_type | vcpu_index | irq_number | >>>>> Why will similar approach not work? >>>> >>>> Well, it conflicts with the GSI routing stuff, and I don't see an >>>> irq chip ID field... >>> It does :( Can't say I am happy about it, but I skipped the discussion >>> about the interface back then and it is too late to complain now. >>> Since, >>> as you notices, irqfd interfaces with irq routing I wonder what's ARM >>> plan about it. But if you choose to go ARM way the format is ARM >>> specific, >>> so you can use your own encoding and put irq chip information there. >> >> Well, we do want to support irqfd, so I don't think we'll be >> following ARM here. >> > I think they want too. May be they have a plan to enhance irqfd. S390 > people do something about it now. Haven't look at proposed patches yet. > >>>> But otherwise (or assuming you mean to use such an encoding when >>>> setting up a GSI route), I didn't say this part couldn't be made to >>>> work. It will require new kernel code for managing a GSI table in a >>>> non-APIC way, and a new callback into the device code, but as I've >>>> said elsewhere I think we need it for irqfd anyway. If I use >>>> KVM_IRQ_LINE for injecting interrupts, do you still object to the >>>> rest of it? >>> The rest of what, proposed interface? There are two separate >>> discussions >>> happening here interleaved. First is "do we need to introduce new >>> generic >>> interface for device creation when existing one, albeit not ideal, >>> can be >>> used" and I am OK with that as long as ARM moves to it for 3.10, >>> although >>> I would prefer to have some example of what this interface will be >>> used >>> for besides irq chips otherwise it will be just another way to create >>> irqchip. >> >> We need a new way to create irqchips anyway, even if it's just what >> the XICS patchset adds (KVM_CREATE_IRQCHIP_ARGS, which is similar to >> KVM_CREATE_DEVICE except it doesn't return an identifier for >> operating on a specific device). And of course we want to sort this >> out before either patchset gets merged, so we don't end up adding >> both methods. I suspect the XICS patchset flew under your radar >> because it has "PPC:" in front of it and the subject line doesn't >> mention the ioctl, but I'm not the only one that felt the need for a >> few new ioctls. >> > I noticed the thread (there was in-kernel irqchip there to compensate for > PPC), but haven't read it before sending the email otherwise I would > have added here that you guys need to agree on common interface. > > Now I looked closer into proposed interface. I am not sure why Paul > decided to implement KVM_CREATE_IRQCHIP_ARGS instead of using > KVM_CREATE_IRQCHIP. He supports only one type with his patches and I am > not sure if he is planning add something else. KVM_IRQCHIP_SET_SOURCES > looks like it tires to reimplement irq routing. Because the same host hardware can have either an in-kernel MPIC or in-kernel XICS, depending on the type of guest you want to run. > >> As for other types of devices, x86 has i8254.c emulated in-kernel -- >> I know that's not going to switch to the new interface, but it could >> have if it existed back then. > Since it is not going to switch it is not a good example. On x86 probably > interrupt remapping device will have to have some kernel component that > may take advantage of the new interface, but I haven't thought about > interrupt remapping implementation enough to be sure. > >> I can also see creating an in-kernel >> emulation device for doing MMIO filtering on some piece of embedded >> hardware that guests need to access with reasonable performance, but >> the hardware desginers screwed up the protection slightly (e.g. put >> other things in the same 4K page). We've done such filtering before >> in our standalone hypervisor; the question is whether it happens to >> anything with enough performance requirements to be done in the >> kernel. > I am not sure why special device is needed for such filtering. If MMIO > is not handled by the kernel it is forwarded to a userspace. Have you attended my talk at KVM Forum? :) Emulated device access (PIO / MMIO) in kvm is about 2-3x faster than going to QEMU on x86. Pure roundtrip time. And it gets worse with hypercalls. See slide 41: https://dl.dropbox.com/u/8976842/KVM%20Forum%202012/MMIO%20Tuning.pdf I haven't done these benchmarks on PPC yet, but I don't expect them to look vastly different. > >> >>> Second one is "how the interface should look like". And here I >>> think that strong distinction is needed between setting the attributes >>> and sending commands with side effects for reasons explained all over >>> this ml thread. >> >> OK, so let's just call them "commands". I like the split into >> "read" and "write" commands, especially when most of the commands >> naturally come in such pairs, but if you don't like that part it can >> be reduced to a single read/write command (and then we'd define >> separate set/get commands where appropriate). > I think read/write will be simpler. > >> >> Note that the XICS patchset also involves device commands. It does >> it by passing any unknown vm ioctl to the irqchip (XICS implements >> KVM_IRQCHIP_GET_SOURCES and KVM_IRQCHIP_SET_SOURCES in addition to >> KVM_IRQ_LINE). Obviously both ways can work; I've given my reasons >> elsewhere in the thread for preferring something that doesn't >> require a new ioctl for every device command. >> >>>> It's not about "silliness" as that this new thing I added for other >>>> reasons did the job just as well (again, except when it comes to >>>> irqfd), and avoided the need for a GSI table, etc. IRQ injection >>>> was not the main point of the new interface. >>> Having generic interface for device creation and then make some >>> devices >>> special by allowing them to be used with KVM_IRQ_LINE makes little >>> sense, >> >> Well, we'd want to document which devices tie into which generic >> interfaces, and which device is selected if multiple such devices >> are created (if that is allowed for a particular device class). >> >> For KVM_IRQ_LINE, we could perhaps use the device id as the irqchip >> id in kvm_irq_routing_irqchip (or, have an attribute/command that >> retrieves the id to be used there). Unfortunately there is no >> irqchip field in kvm_irq_routing_msi, though since it's basically a >> command to write to an arbitrary MMIO address, maybe it could just >> be implemented that way? >> > How MSI is delivered with MPIC? From device point of view sending an MSI > is doing write at address X of data Y. How PPC with MPIC translates this > into actual interrupt? The device DMAs into an MPIC register to trigger the MSI. The MPIC then enables the "level high" bit for that particular internal interrupt line. It also triggers an interrupt on a CPU - the same way it would with level or timer interrupts. Filtering and priority also work the same way. Once the guest ACKs that interrupt, the "level high" bit for that MSI line gets cleared. So essentially, the MPIC converts MSIs into level based interrupts. That's why pushing an MSI into the in-kernel MPIC by using the live migration interface works. All you need to do is live migrate the "MSI interrupt line is active" bit into the device state. > >>>>>> I see no need for a separate ioctl in terms of the underlying >>>>>> infrastructure for distinguishing "attribute" from "write-only >>>>>> command". I'm open to improvements on what the ioctl is called. >>>>>> It's basically like setting a register on a device, except I was >>>>>> concerned that if we actually called it a "register" that people >>>>>> would take it too literally and think it's only for the >>> architected >>>>>> register state of the emulated device. >>>>> I agree "attribute" is better name than "register", but injecting >>>>> interrupt is not setting an attribute. >>>> >>>> It's a dynamic attribute -- the state of the input line. Better >>>> names are welcome. I don't see this difference as enough to warrant >>>> separate ioctls. >>> As long as you use the same attribute for migration and interrupt >>> injection >>> purpose I do. If you use separate attributes for migration and >>> interrupt >>> injection then not having separate ioctl is just a hack. >> >> Why is it a hack? Is it also a hack to not use a separate ioctl to >> reset the device, to move its address map, etc? >> > It is a hack because purpose of the interface becomes unclear. If you > see it called in a code you have no idea what semantics is expected. > For instance getting/setting of a state should be done when vcpus are > not running, but commands may be sent while they are. This also > encourage bugs when incorrect attribute is used during migration or vice > versa. In short interface does not help you, it requires you to read > documentation of each device very carefully. Reset is one thing that > will be implemented as a command ioctl. Moving address map depends. If > an address is a part of device's sate and will be migrated as such then it > is device attribute, otherwise use command to move it. I agree here. I think it makes sense to have a separate interface to set interrupt states. This separate interface can be a separate group inside this interface too. But I agree that we shouldn't use the live migration interface to set the interrupt line of the MPIC. Alex -- 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
