| Message ID | 5571AB41020000780008153F@mail.emea.novell.com |
|---|---|
| State | New |
| Headers | show |
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 <jbeulich@suse.com> > > @@ -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,
>>> 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
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.
--- a/qemu/upstream/hw/xen/xen_pt.h +++ b/qemu/upstream/hw/xen/xen_pt.h @@ -175,9 +175,8 @@ typedef struct XenPTMSIXEntry { int pirq; uint64_t addr; uint32_t data; - uint32_t vector_ctrl; + uint32_t latch[4]; bool updated; /* indicate whether MSI ADDR or DATA is updated */ - bool warned; /* avoid issuing (bogus) warning more than once */ } XenPTMSIXEntry; typedef struct XenPTMSIX { uint32_t ctrl_offset; --- a/qemu/upstream/hw/xen/xen_pt_msi.c +++ b/qemu/upstream/hw/xen/xen_pt_msi.c @@ -25,6 +25,7 @@ #define XEN_PT_GFLAGSSHIFT_DELIV_MODE 12 #define XEN_PT_GFLAGSSHIFT_TRG_MODE 15 +#define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)] /* * Helpers @@ -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)) { + 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) { @@ -396,35 +404,15 @@ int xen_pt_msix_update_remap(XenPCIPasst static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset) { - switch (offset) { - case PCI_MSIX_ENTRY_LOWER_ADDR: - return e->addr & UINT32_MAX; - case PCI_MSIX_ENTRY_UPPER_ADDR: - return e->addr >> 32; - case PCI_MSIX_ENTRY_DATA: - return e->data; - case PCI_MSIX_ENTRY_VECTOR_CTRL: - return e->vector_ctrl; - default: - return 0; - } + return !(offset % sizeof(*e->latch)) + ? e->latch[offset / sizeof(*e->latch)] : 0; } static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val) { - switch (offset) { - case PCI_MSIX_ENTRY_LOWER_ADDR: - e->addr = (e->addr & ((uint64_t)UINT32_MAX << 32)) | val; - break; - case PCI_MSIX_ENTRY_UPPER_ADDR: - e->addr = (uint64_t)val << 32 | (e->addr & UINT32_MAX); - break; - case PCI_MSIX_ENTRY_DATA: - e->data = val; - break; - case PCI_MSIX_ENTRY_VECTOR_CTRL: - e->vector_ctrl = val; - break; + if (!(offset % sizeof(*e->latch))) + { + e->latch[offset / sizeof(*e->latch)] = val; } } @@ -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); - 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); } 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,