|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
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@xxxxxxxx>
> ---
> 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,
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |