On Mon, Aug 24, 2020 at 06:07:28PM +0200, Jan Beulich wrote: > On 24.08.2020 16:44, Roger Pau Monné wrote: > > On Mon, Aug 24, 2020 at 04:06:31PM +0200, Jan Beulich wrote: > >> On 12.08.2020 14:47, Roger Pau Monne wrote: > >>> Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI > >>> and instead use the newly introduced EOI callback mechanism in order > >>> to register a callback for MSI vectors injected from passed through > >>> devices. > >> > >> In patch 2 you merely invoke the callback when the EOI arrived, > >> but you don't clear the callback (unless I've missed something). > >> Here you register the callback once per triggering of the IRQ, > >> without clearing it from the callback itself either. > > > > It gets cleared on the next call to vlapic_set_irq_callback, or set to > > a new callback value if the passed callback is not NULL. > > > >> Why is it > >> correct / safe to keep the callback registered? > > > > The next vector injected is going to clear it, so should be safe, as > > no vector can be injected without calling vlapic_set_irq_callback. > > But what about duplicate EOIs, even if just by bug or erratum? > I notice, for example, that VMX'es VMEXIT handler directly > calls vlapic_handle_EOI().
Yes, but that's expected and required, because when using VMX LAPIC virtualization you get an explicit vmexit (EXIT_REASON_EOI_INDUCED) on EOI of requested vectors by using the EOI exit bitmap (vmx_update_eoi_exit_bitmap). > I'd find it more safe if an > unexpected EOI didn't get any callback invoked. OK, the callback can be certainly cleared in vlapic_handle_EOI. > > >> What about > >> conflicting callbacks for shared vectors? > > > > In this callback model for vlapic only the last injected vector > > callback would get executed. It's my understanding that lapic > > vectors cannot be safely shared unless there's a higher level > > interrupt controller (ie: an IO-APIC) that does the sharing. > > As said on a different, but pretty recent thread: I do think > sharing is possible if devices and drivers meet certain criteria > (in particular have a way to determine which of the devices > actually require service). It's just not something one would > normally do. But iirc such a model was used in good old DOS to > overcome the 15 IRQs limit (of which quite a few had fixed > purposes, so for add-in devices there were only very few left). So my callback model for the vIO-APIC/vPIC is different from the one used for the vlapic. In that case callers must use a helper to register/remove a callback for GSIs, and a single GSI can have multiple callbacks attached. Such interface works well with the vIO-APIC/vPIC because interrupts from devices are statically assigned a GSI, and you only need to register the callback when the device is instantiated. For vlapic OTOH this would be more complex, as a guest might decide to change MSI vectors constantly and thus require a non-trivial amount of registrations/removals of callbacks. I was assuming that any sane OS wouldn't share a lapic vector for multiple devices, and that hence we could get away without having to implement multiple per-vector callback support. Would you be fine with clearing the callback in vlapic_handle_EOI and then vlapic_set_irq_callback complaining if it finds there's a previous callback different than the one provided already set for the to be injected vector? Thanks, Roger.
