[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes |
Date: |
Tue, 16 Jun 2015 15:48:16 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 15:35, <address@hidden> 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.
- [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones, Jan Beulich, 2015/06/05
- [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data, Jan Beulich, 2015/06/05