| Message ID | 56548D1C02000078000B8834@prv-mh.provo.novell.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 24 Nov 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> > --- > v2: Pass original vec_ctrl to xen_pt_msix_update_one() instead of > (ab)using latch[]. > > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -1499,6 +1499,8 @@ static int xen_pt_msixctrl_reg_write(Xen > xen_pt_msix_disable(s); > } > > + s->msix->maskall = *val & PCI_MSIX_FLAGS_MASKALL; > + > debug_msix_enabled_old = s->msix->enabled; > s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE); > if (s->msix->enabled != debug_msix_enabled_old) { > --- a/hw/xen/xen_pt.h > +++ b/hw/xen/xen_pt.h > @@ -187,13 +187,13 @@ 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; > bool enabled; > + bool maskall; > int total_entries; > int bar_index; > uint64_t table_base; > --- a/hw/xen/xen_pt_msi.c > +++ b/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 > @@ -314,7 +315,8 @@ static int msix_set_enable(XenPCIPassthr > enabled); > } > > -static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr) > +static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr, > + uint32_t vec_ctrl) > { > XenPTMSIXEntry *entry = NULL; > int pirq; > @@ -332,6 +334,13 @@ static int xen_pt_msix_update_one(XenPCI > > pirq = entry->pirq; Hi Jan, I know that in your opinion is superfluous, nonetheless could you please add 2-3 lines of in-code comment right here, to explain what you are doing with the check? Something like: /* * Update the entry addr and data to the latest values only when the * entry is masked or they are all masked, as required by the spec. * Addr and data changes while the MSI-X entry is unmasked will be * delayed until the next masking->unmasking. */ > + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall || > + (vec_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) { > @@ -357,7 +366,7 @@ int xen_pt_msix_update(XenPCIPassthrough > int i; > > for (i = 0; i < msix->total_entries; i++) { > - xen_pt_msix_update_one(s, i); > + xen_pt_msix_update_one(s, i, msix->msix_entry[i].latch(VECTOR_CTRL)); > } > > return 0; > @@ -406,35 +415,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; > } > } > > @@ -454,39 +443,26 @@ 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; > - > - 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, *vec_ctrl); > } > > 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 07.12.15 at 13:41, <stefano.stabellini@eu.citrix.com> wrote: > On Tue, 24 Nov 2015, Jan Beulich wrote: >> @@ -332,6 +334,13 @@ static int xen_pt_msix_update_one(XenPCI >> >> pirq = entry->pirq; > > I know that in your opinion is superfluous, nonetheless could you please > add 2-3 lines of in-code comment right here, to explain what you are > doing with the check? Something like: > > /* > * Update the entry addr and data to the latest values only when the > * entry is masked or they are all masked, as required by the spec. > * Addr and data changes while the MSI-X entry is unmasked will be > * delayed until the next masking->unmasking. > */ > > >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall || >> + (vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) { >> + entry->addr = entry->latch(LOWER_ADDR) | >> + ((uint64_t)entry->latch(UPPER_ADDR) << 32); >> + entry->data = entry->latch(DATA); >> + } Adding a comment like this is fine of course, namely if it helps acceptance. Jan
>>> On 07.12.15 at 13:41, <stefano.stabellini@eu.citrix.com> wrote: > I know that in your opinion is superfluous, nonetheless could you please > add 2-3 lines of in-code comment right here, to explain what you are > doing with the check? Something like: > > /* > * Update the entry addr and data to the latest values only when the > * entry is masked or they are all masked, as required by the spec. > * Addr and data changes while the MSI-X entry is unmasked will be > * delayed until the next masking->unmasking. > */ Btw, will it be okay to just resend this one patch as v3, or do I need to resend the whole series (the rest of which didn't change)? Jan
On Mon, 7 Dec 2015, Jan Beulich wrote: > >>> On 07.12.15 at 13:41, <stefano.stabellini@eu.citrix.com> wrote: > > I know that in your opinion is superfluous, nonetheless could you please > > add 2-3 lines of in-code comment right here, to explain what you are > > doing with the check? Something like: > > > > /* > > * Update the entry addr and data to the latest values only when the > > * entry is masked or they are all masked, as required by the spec. > > * Addr and data changes while the MSI-X entry is unmasked will be > > * delayed until the next masking->unmasking. > > */ > > Btw, will it be okay to just resend this one patch as v3, or do I need > to resend the whole series (the rest of which didn't change)? Just this patch is fine.
--- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -1499,6 +1499,8 @@ static int xen_pt_msixctrl_reg_write(Xen xen_pt_msix_disable(s); } + s->msix->maskall = *val & PCI_MSIX_FLAGS_MASKALL; + debug_msix_enabled_old = s->msix->enabled; s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE); if (s->msix->enabled != debug_msix_enabled_old) { --- a/hw/xen/xen_pt.h +++ b/hw/xen/xen_pt.h @@ -187,13 +187,13 @@ 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; bool enabled; + bool maskall; int total_entries; int bar_index; uint64_t table_base; --- a/hw/xen/xen_pt_msi.c +++ b/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 @@ -314,7 +315,8 @@ static int msix_set_enable(XenPCIPassthr enabled); } -static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr) +static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr, + uint32_t vec_ctrl) { XenPTMSIXEntry *entry = NULL; int pirq; @@ -332,6 +334,13 @@ static int xen_pt_msix_update_one(XenPCI pirq = entry->pirq; + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall || + (vec_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) { @@ -357,7 +366,7 @@ int xen_pt_msix_update(XenPCIPassthrough int i; for (i = 0; i < msix->total_entries; i++) { - xen_pt_msix_update_one(s, i); + xen_pt_msix_update_one(s, i, msix->msix_entry[i].latch(VECTOR_CTRL)); } return 0; @@ -406,35 +415,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; } } @@ -454,39 +443,26 @@ 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; - - 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, *vec_ctrl); } 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,