Re: [Qemu-devel] [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes

[Jan Beulich]

>>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote:
>On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:

I'm sorry for getting back to this only now.

>> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>> >>  
>> >>      pirq = entry->pirq;
>> >>  
>> >> +    if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> >> +        (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> > 
>> > I admit I am having difficulties understanding the full purpose of these
>> > checks. Please add a comment on them.
>> 
>> The comment would (pointlessly imo) re-state what the code already
>> says:
>> 
>> > I guess the intention is only to make changes using the latest values,
>> > the ones in entry->latch, when the right conditions are met, otherwise
>> > keep using the old values. Is that right?
>> > 
>> > In that case, don't we want to use the latest values on MASKBIT ->
>> > !MASKBIT transitions? In general when unmasking?
>> 
>> This is what we want. And with that, the questions you ask further
>> down should be answered too: The function gets invoked with the
>> pre-change mask flag state in ->latch[], and updates the values
>> used for actually setting up when that one has the entry masked
>> (or mask-all is set). The actual new value gets written to ->latch[]
>> after the call.
>
>I think this logic is counter-intuitive and prone to confuse the reader.
>This change doesn't make sense on its own: when one will read
>xen_pt_msix_update_one, won't be able to understand the function without
>checking the call sites.
>
>Could we turn it around to be more obvious?  Here check if
>!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only
>call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions?
>
>Or something like that?

That would maybe be doable with just pci_msix_write() as a caller,
but not with the one in xen_pt_msix_update() (where we have no
transition of the mask bit from set to clear).

Jan