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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 16.06.15 at 15:35, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -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.

>> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
>>      offset = addr % PCI_MSIX_ENTRY_SIZE;
>>  
>>      if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>> -        const volatile uint32_t *vec_ctrl;
>> -
>>          if (get_entry_value(entry, offset) == val
>>              && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>>              return;
>>          }
>>  
>> +        entry->updated = true;
>> +    } else if (msix->enabled && entry->updated &&
>> +               !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> +        const volatile uint32_t *vec_ctrl;
>> +
>>          /*
>>           * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
>>           * up-to-date. Read from hardware directly.
>>           */
>>          vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
>>              + PCI_MSIX_ENTRY_VECTOR_CTRL;
>> +        set_entry_value(entry, offset, *vec_ctrl);
> 
> Why are you calling set_entry_value with the hardware vec_ctrl value? It
> doesn't look correct to me.  In any case, if you wanted to do it,
> shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> whole *vec_ctrl?

The comment above the code explains it: What we have stored locally
may not reflect reality, as we may not have seen all writes (and this
indeed isn't just a "may"). And if out cached value isn't valid anymore,
why would we not want to update all of it, rather than just the mask
bit?

>> -        if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> -            if (!entry->warned) {
>> -                entry->warned = true;
>> -                XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
>> -                           " already enabled.\n", entry_nr);
>> -            }
>> -            return;
>> -        }
>> -
>> -        entry->updated = true;
>> +        xen_pt_msix_update_one(s, entry_nr);
> 
> Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> MASKBIT -> !MASKBIT transition?

The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check in the if() surrounding this call and the
(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check inside the function guarantee just that (i.e. the function
invocation is benign in the other case, as entry->addr/entry->data
would remain unchanged).

Jan