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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> 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.

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?


> >> @@ -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?

OK, however the previous code wasn't actually updating the entirety of
vector_ctrl. It was just using the updated value to check for
PCI_MSIX_ENTRY_CTRL_MASKBIT.  This is something else.  The new behavior
might be correct, but at least the commit message needs to explain it.


> >> -        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).

OK, maybe the code works as is, but it took me a long time to make sense
of it because it relies on the combinations of three checks in three
different places. I would prefer to change it into something more
obvious.