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. The interface is not over generic i.e it does not try to replace KVM_RUN by writing special register. > How is writing documentation not creating new interfaces, if the > documentation is different from what the interface is currently > understood to do? If this case you misunderstand what I am proposing. The interface sets/gets irq chip state and this is what it will continue to do. What needs to be documented is the format of mpic irqchip. Nobody expects it to be the same for all irq chips. > Note that Marcelo seems to view KVM_SET_IRQCHIP as effectively being > a device reset, which is rather different. > Marcelo views the interface exactly the same as I view it. It is used for initializing device state (reset is one of those time when it happens) and to transfer device state for migration purposes. > >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. > > So basically, you want me to keep this interface but share the ioctl > number with an older interface? :-P If that is what you want. Obviously you can drop things that makes proposed interface generic one. > > >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. > > Three *groups* of properties. One of the property groups is per > source, and we can have hundreds of sources. Another exposes the > register space, which is 64 KiB (admittedly it's somewhat sparse, > but there's more than 512 bytes of real data in there). I mean that you still access each property one by one but since each individual one is not bigger than 64bit you can put it inline and do not need pointers, or you can access groups of properties if each one fits into the buffer. > And we > don't necessarily want to set *everything*. What are those cases? You do need to on reset/migration. > > >> 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? > >> 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? > > It should probably be get_user() instead, which is pretty fast in > the absence of a fault. > > >> >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. > > 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? Why it goes via userspace? 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. > >You need to show us the benefits of the new interface > >vs existing one, not vice versa. > > Well, as I said to Marcello, the real reason why we probably need to > use the existing interface is irqfd. That doesn't make the device > control stuff go away. > > >> 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? We had that problem with MSRs on x86. We had to, eventually, add a flag that tells us the reason of MSR access. > > >> 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. > > 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. 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. > > >> Could an error return be used for cases where the IRQ was not > >> delivered, in the very unlikely event that we want to implement > >> something similar on MPIC? > >We can, but I do not think it will be good API. This condition is > >not an > >error. > > -EBUSY seems appropriate enough... It is. Other commands may have more elaborate return status. Generic interface should take this into account. > > >> Note again that MPIC's decision to use > >> or not use KVM_IRQ_LINE is only about what MPIC does; it is not > >> inherent in the device control API. > >That's the crux of the problem though. MPIC tries to be different just > >for the sake to be different. Why? The only explanation you provide is > >because current API is "silly", not that you cannot implement MPIC > >with > >it or it will be unnecessary slow, just "silly". > > 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, so IRQ injection may be not the main point of the new interface, but communication with a device created by the new interface is something that cannot be ignored at the interface design stage. > > >> >Other devices may get other commands that need > >> >response, so if we design generic interface we should take it into > >> >account. I think using KVM_SET_DEVICE_ATTR to inject interrupts > >is a > >> >misnomer, you do not set internal device attribute, you toggle > >> >external > >> >input. My be another ioctl KVM_SEND_DEVICE_COMMAND is needed. > >> > >> 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. > > >> >> >ARM vGIC code, that is ready to go upstream, uses old way > >too. So > >> >> >it will > >> >> >be 2 archs against one. > >> >> > >> >> I wasn't aware that that's how it worked. :-P > >> >> > >> >What worked? That vGIC uses existing interface or that non generic > >> >interface used by many arches wins generic one used by only one > >arch? > >> > >> The latter. Two wrongs don't make a right, and adding another > >> inextensible, device-specific API is not the answer to the existing > >> APIs being too inextensible and device/arch-specific. Some portion > >> will always need to be device-specific because we're controlling the > >> creation and of a specific device, but the glue does not need to be. > >> > >This is not "adding another inextensible, device-specific API" vs > >"adding > >cool generic extensible API" though. It is "using existing > >inextensible, > >device-specific API" vs "adding cool generic extensible API". > > The "existing inextensible device-specific API" doesn't have support > for this "specific device". Something new has to be added one way > or another. > And extending existing interface (which is already supports more then one irqchips BTW) wins. > >> >APIs are easy to add and impossible to remove. > >> > >> That's why I want to get it right this time. > >> > >And what if you'll fail? > > That's always a possibility of course. I don't think that's a good > reason to avoid trying to move in the right direction. It is not, but that is not the point I am trying to make :) > > >What if next architecture will bring new > >developer that will proclaim your new interface "silly" since it > >does not > >allow for device destruction and do not return file descriptor for > >newly > >created device that userspace can do select on to wait for a device's > >events or mmap memory for fast userspace/device communication? > > The device id that gets returned is arbitrary; you could turn it > into an fd later with no loss of compatibility. > > Device destruction would complicate things and I would not support > requiring all devices to allow it. If someone wanted to add it for > certain devices, at the interface level it would just be a new > ioctl. > And here is the point that I am trying to make. You propose how your interface can be extended and I agree, it can. But that other guy will see it in other light: Why should I extend interface that is broken instead of providing new, perfect one. -- Gleb. -- 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
