Re: [Qemu-devel] [RFC][PATCH 23/45] qemu-kvm: Rework MSI-X mask notifier to generic MSI config notifiers

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

On 2011-10-17 14:39, Michael S. Tsirkin wrote:
> On Mon, Oct 17, 2011 at 01:45:04PM +0200, Jan Kiszka wrote:
>> On 2011-10-17 13:40, Michael S. Tsirkin wrote:
>>> On Mon, Oct 17, 2011 at 11:27:57AM +0200, Jan Kiszka wrote:
>>>> MSI config notifiers are supposed to be triggered on every relevant
>>>> configuration change of MSI vectors or if MSI is enabled/disabled.
>>>>
>>>> Two notifiers are established, one for vector changes and one for general
>>>> enabling. The former notifier additionally passes the currently active
>>>> MSI message.
>>>> This will allow to update potential in-kernel IRQ routes on
>>>> changes. The latter notifier is optional and will only be used by a
>>>> subset of clients.
>>>>
>>>> These notifiers are currently only available for MSI-X but will be
>>>> extended to legacy MSI as well.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> Passing message, always, does not seem to make sense: message is only
>>> valid if it is unmasked.
>>
>> If we go from unmasked to masked, the consumer could just ignore the
>> message.
> 
> Why don't we let the consumer get the message if it needs it?

Because most consumer will need and because I want to keep the API simple.

> 
>>> Further, IIRC the spec requires any changes to be done while
>>> message is masked. So mask notifier makes more sense to me:
>>> it does the same thing using one notifier that you do
>>> using two notifiers.
>>
>> That's in fact a possible optimization (only invoke the callback on mask
>> transitions).
> 
> Further, it is one that is already implemented.
> So I would prefer not to add work by removing it :)

Generalization to cover MSI requires some changes. Unneeded behavioral
changes back and forth should and will of course be avoided. I will
rework this.

> 
>> Not sure if that applies to MSI as well, probably not.
> 
> Probably not. However, if per vector masking is
> supported, and while vector is masked, the address/
> data values might not make any sense.
> 
> So I think even msi users needs to know about masked state.

Yes, and they get this information via the config notifier.

> 
>> To
>> have common types, I would prefer to stay with vector config notifiers
>> as name then.
>>
>> Jan
> 
> So we pass in nonsense values and ask all users to know about MSIX rules.
> Ugh.
> 
> I do realize msi might change the vector without masking.
> We can either artificially call mask before value change
> and unmask after, or use 3 notifiers: mask,unmask,config.
> Add a comment that config is invoked when configuration
> for an unmasked vector is changed, and that
> it can only happen for msi, not msix.

I see no need in complicating the API like this. MSI-X still needs the
config information on unmask, so let's just consistently pass it via the
unified config notifier instead of forcing the consumers to create yet
two more handlers. I really do not see the benefit for the consumer.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]