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

[Stefano Stabellini]

On Fri, 5 Jun 2015, Jan Beulich wrote:
> The remaining log message in pci_msix_write() is wrong, as there guest
> behavior may only appear to be wrong: For one, the old logic didn't
> take the mask-all bit into account. And then this shouldn't depend on
> host device state (i.e. the host may have masked the entry without the
> guest having done so). Plus these writes shouldn't be dropped even when
> an entry gets unmasked. Instead, if they can't be made take effect
> right away, they should take effect on the next unmasking or enabling
> operation - the specification explicitly describes such caching
> behavior.
> 
> Signed-off-by: Jan Beulich <address@hidden>
> 
> @@ -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.

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? Here it looks like we
are using the latest values when maskall is set or
PCI_MSIX_ENTRY_CTRL_MASKBIT is set, that is the opposite of what we
want.


> +        entry->addr = entry->latch(LOWER_ADDR) |
> +                      ((uint64_t)entry->latch(UPPER_ADDR) << 32);
> +        entry->data = entry->latch(DATA);
> +    }
> +
>      rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
>                          entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
>      if (rc) {
> @@ -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?


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


>      }
>  
>      set_entry_value(entry, offset, val);
> -
> -    if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
> -        if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> -            xen_pt_msix_update_one(s, entry_nr);
> -        }
> -    }
>  }
>  
>  static uint64_t pci_msix_read(void *opaque, hwaddr addr,