* [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
2015-11-24 15:09 [Qemu-devel] [PATCH v2 0/4] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-11-24 15:15 ` Jan Beulich
2015-12-07 12:41 ` Stefano Stabellini
2015-11-24 15:15 ` [Qemu-devel] [PATCH v2 2/4] xen/MSI-X: really enforce alignment Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-11-24 15:15 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 5926 bytes --]
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;
+ 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,
[-- Attachment #2: qemu-MSI-X-latch-writes.patch --]
[-- Type: text/plain, Size: 5961 bytes --]
xen/MSI-X: latch MSI-X table writes
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;
+ 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,
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
2015-11-24 15:15 ` [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes Jan Beulich
@ 2015-12-07 12:41 ` Stefano Stabellini
2015-12-07 15:57 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-07 12:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
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,
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-07 12:41 ` Stefano Stabellini
@ 2015-12-07 15:57 ` Jan Beulich
2015-12-07 16:46 ` Jan Beulich
2015-12-08 11:09 ` [Qemu-devel] [PATCH v3 " Jan Beulich
2 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-12-07 15:57 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-07 12:41 ` Stefano Stabellini
2015-12-07 15:57 ` Jan Beulich
@ 2015-12-07 16:46 ` Jan Beulich
2015-12-08 10:41 ` Stefano Stabellini
2015-12-08 11:09 ` [Qemu-devel] [PATCH v3 " Jan Beulich
2 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-07 16:46 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> 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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-07 16:46 ` Jan Beulich
@ 2015-12-08 10:41 ` Stefano Stabellini
0 siblings, 0 replies; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-08 10:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v3 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-07 12:41 ` Stefano Stabellini
2015-12-07 15:57 ` Jan Beulich
2015-12-07 16:46 ` Jan Beulich
@ 2015-12-08 11:09 ` Jan Beulich
2015-12-09 15:55 ` [Qemu-devel] [Xen-devel] " Stefano Stabellini
2 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-08 11:09 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 6273 bytes --]
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>
---
v3: Add comment to xen_pt_msix_update_one().
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,19 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ /*
+ * 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 get deferred
+ * until the next masked -> unmasked transition.
+ */
+ 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 +372,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 +421,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 +449,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,
[-- Attachment #2: qemu-MSI-X-latch-writes.patch --]
[-- Type: text/plain, Size: 6308 bytes --]
xen/MSI-X: latch MSI-X table writes
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>
---
v3: Add comment to xen_pt_msix_update_one().
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,19 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ /*
+ * 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 get deferred
+ * until the next masked -> unmasked transition.
+ */
+ 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 +372,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 +421,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 +449,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,
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [Xen-devel] [PATCH v3 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-08 11:09 ` [Qemu-devel] [PATCH v3 " Jan Beulich
@ 2015-12-09 15:55 ` Stefano Stabellini
2015-12-09 16:28 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-09 15:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 8 Dec 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>
I have applied this patch and the first 2 of the series to my "next"
branch. I have to think a bit more about the fourth.
> v3: Add comment to xen_pt_msix_update_one().
> 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,19 @@ static int xen_pt_msix_update_one(XenPCI
>
> pirq = entry->pirq;
>
> + /*
> + * 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 get deferred
> + * until the next masked -> unmasked transition.
> + */
> + 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 +372,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 +421,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 +449,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,
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [Xen-devel] [PATCH v3 1/4] xen/MSI-X: latch MSI-X table writes
2015-12-09 15:55 ` [Qemu-devel] [Xen-devel] " Stefano Stabellini
@ 2015-12-09 16:28 ` Jan Beulich
0 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-12-09 16:28 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 09.12.15 at 16:55, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 8 Dec 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>
>
> I have applied this patch and the first 2 of the series to my "next"
> branch. I have to think a bit more about the fourth.
Thanks (I guess you mean this one and the _next_ 2 of this series).
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH v2 2/4] xen/MSI-X: really enforce alignment
2015-11-24 15:09 [Qemu-devel] [PATCH v2 0/4] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
2015-11-24 15:15 ` [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes Jan Beulich
@ 2015-11-24 15:15 ` Jan Beulich
2015-11-24 15:16 ` [Qemu-devel] [PATCH v2 3/4] xen/pass-through: correctly deal with RW1C bits Jan Beulich
2015-11-24 15:17 ` [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability Jan Beulich
3 siblings, 0 replies; 27+ messages in thread
From: Jan Beulich @ 2015-11-24 15:15 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]
The way the generic infrastructure works the intention of not allowing
unaligned accesses can't be achieved by simply setting .unaligned to
false. The benefit is that we can now replace the conditionals in
{get,set}_entry_value() by assert()-s.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -415,16 +415,14 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- return !(offset % sizeof(*e->latch))
- ? e->latch[offset / sizeof(*e->latch)] : 0;
+ assert(!(offset % sizeof(*e->latch)));
+ return e->latch[offset / sizeof(*e->latch)];
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- if (!(offset % sizeof(*e->latch)))
- {
- e->latch[offset / sizeof(*e->latch)] = val;
- }
+ assert(!(offset % sizeof(*e->latch)));
+ e->latch[offset / sizeof(*e->latch)] = val;
}
static void pci_msix_write(void *opaque, hwaddr addr,
@@ -488,6 +486,12 @@ static uint64_t pci_msix_read(void *opaq
}
}
+static bool pci_msix_accepts(void *opaque, hwaddr addr,
+ unsigned size, bool is_write)
+{
+ return !(addr & (size - 1));
+}
+
static const MemoryRegionOps pci_msix_ops = {
.read = pci_msix_read,
.write = pci_msix_write,
@@ -496,7 +500,13 @@ static const MemoryRegionOps pci_msix_op
.min_access_size = 4,
.max_access_size = 4,
.unaligned = false,
+ .accepts = pci_msix_accepts
},
+ .impl = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false
+ }
};
int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
[-- Attachment #2: qemu-MSI-X-force-align.patch --]
[-- Type: text/plain, Size: 1894 bytes --]
xen/MSI-X: really enforce alignment
The way the generic infrastructure works the intention of not allowing
unaligned accesses can't be achieved by simply setting .unaligned to
false. The benefit is that we can now replace the conditionals in
{get,set}_entry_value() by assert()-s.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -415,16 +415,14 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- return !(offset % sizeof(*e->latch))
- ? e->latch[offset / sizeof(*e->latch)] : 0;
+ assert(!(offset % sizeof(*e->latch)));
+ return e->latch[offset / sizeof(*e->latch)];
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- if (!(offset % sizeof(*e->latch)))
- {
- e->latch[offset / sizeof(*e->latch)] = val;
- }
+ assert(!(offset % sizeof(*e->latch)));
+ e->latch[offset / sizeof(*e->latch)] = val;
}
static void pci_msix_write(void *opaque, hwaddr addr,
@@ -488,6 +486,12 @@ static uint64_t pci_msix_read(void *opaq
}
}
+static bool pci_msix_accepts(void *opaque, hwaddr addr,
+ unsigned size, bool is_write)
+{
+ return !(addr & (size - 1));
+}
+
static const MemoryRegionOps pci_msix_ops = {
.read = pci_msix_read,
.write = pci_msix_write,
@@ -496,7 +500,13 @@ static const MemoryRegionOps pci_msix_op
.min_access_size = 4,
.max_access_size = 4,
.unaligned = false,
+ .accepts = pci_msix_accepts
},
+ .impl = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ .unaligned = false
+ }
};
int xen_pt_msix_init(XenPCIPassthroughState *s, uint32_t base)
^ permalink raw reply [flat|nested] 27+ messages in thread* [Qemu-devel] [PATCH v2 3/4] xen/pass-through: correctly deal with RW1C bits
2015-11-24 15:09 [Qemu-devel] [PATCH v2 0/4] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
2015-11-24 15:15 ` [Qemu-devel] [PATCH v2 1/4] xen/MSI-X: latch MSI-X table writes Jan Beulich
2015-11-24 15:15 ` [Qemu-devel] [PATCH v2 2/4] xen/MSI-X: really enforce alignment Jan Beulich
@ 2015-11-24 15:16 ` Jan Beulich
2015-12-07 12:43 ` Stefano Stabellini
2015-11-24 15:17 ` [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability Jan Beulich
3 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-11-24 15:16 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 4458 bytes --]
Introduce yet another mask for them, so that the generic routine can
handle them, at once rendering xen_pt_pmcsr_reg_write() superfluous.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -113,6 +113,8 @@ struct XenPTRegInfo {
uint32_t res_mask;
/* reg read only field mask (ON:RO/ROS, OFF:other) */
uint32_t ro_mask;
+ /* reg read/write-1-clear field mask (ON:RW1C/RW1CS, OFF:other) */
+ uint32_t rw1c_mask;
/* reg emulate field mask (ON:emu, OFF:passthrough) */
uint32_t emu_mask;
xen_pt_conf_reg_init init;
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -179,7 +179,8 @@ static int xen_pt_byte_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -197,7 +198,8 @@ static int xen_pt_word_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -215,7 +217,8 @@ static int xen_pt_long_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -633,6 +636,7 @@ static XenPTRegInfo xen_pt_emu_reg_heade
.init_val = 0x0000,
.res_mask = 0x0007,
.ro_mask = 0x06F8,
+ .rw1c_mask = 0xF900,
.emu_mask = 0x0010,
.init = xen_pt_status_reg_init,
.u.w.read = xen_pt_word_reg_read,
@@ -944,6 +948,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
.size = 2,
.res_mask = 0xFFC0,
.ro_mask = 0x0030,
+ .rw1c_mask = 0x000F,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_word_reg_write,
@@ -964,6 +969,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
.offset = PCI_EXP_LNKSTA,
.size = 2,
.ro_mask = 0x3FFF,
+ .rw1c_mask = 0xC000,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_word_reg_write,
@@ -1000,27 +1006,6 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
* Power Management Capability
*/
-/* write Power Management Control/Status register */
-static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
- XenPTReg *cfg_entry, uint16_t *val,
- uint16_t dev_value, uint16_t valid_mask)
-{
- XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
- uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
- uint16_t *data = cfg_entry->ptr.half_word;
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
-
- /* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
- throughable_mask);
-
- return 0;
-}
-
/* Power Management Capability reg static information table */
static XenPTRegInfo xen_pt_emu_reg_pm[] = {
/* Next Pointer reg */
@@ -1051,11 +1036,12 @@ static XenPTRegInfo xen_pt_emu_reg_pm[]
.size = 2,
.init_val = 0x0008,
.res_mask = 0x00F0,
- .ro_mask = 0xE10C,
+ .ro_mask = 0x610C,
+ .rw1c_mask = 0x8000,
.emu_mask = 0x810B,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
- .u.w.write = xen_pt_pmcsr_reg_write,
+ .u.w.write = xen_pt_word_reg_write,
},
{
.size = 0,
[-- Attachment #2: qemu-pt-RW1C-bits.patch --]
[-- Type: text/plain, Size: 4505 bytes --]
xen/pass-through: correctly deal with RW1C bits
Introduce yet another mask for them, so that the generic routine can
handle them, at once rendering xen_pt_pmcsr_reg_write() superfluous.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -113,6 +113,8 @@ struct XenPTRegInfo {
uint32_t res_mask;
/* reg read only field mask (ON:RO/ROS, OFF:other) */
uint32_t ro_mask;
+ /* reg read/write-1-clear field mask (ON:RW1C/RW1CS, OFF:other) */
+ uint32_t rw1c_mask;
/* reg emulate field mask (ON:emu, OFF:passthrough) */
uint32_t emu_mask;
xen_pt_conf_reg_init init;
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -179,7 +179,8 @@ static int xen_pt_byte_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -197,7 +198,8 @@ static int xen_pt_word_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -215,7 +217,8 @@ static int xen_pt_long_reg_write(XenPCIP
*data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
/* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
+ throughable_mask);
return 0;
}
@@ -633,6 +636,7 @@ static XenPTRegInfo xen_pt_emu_reg_heade
.init_val = 0x0000,
.res_mask = 0x0007,
.ro_mask = 0x06F8,
+ .rw1c_mask = 0xF900,
.emu_mask = 0x0010,
.init = xen_pt_status_reg_init,
.u.w.read = xen_pt_word_reg_read,
@@ -944,6 +948,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
.size = 2,
.res_mask = 0xFFC0,
.ro_mask = 0x0030,
+ .rw1c_mask = 0x000F,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_word_reg_write,
@@ -964,6 +969,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
.offset = PCI_EXP_LNKSTA,
.size = 2,
.ro_mask = 0x3FFF,
+ .rw1c_mask = 0xC000,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_word_reg_write,
@@ -1000,27 +1006,6 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
* Power Management Capability
*/
-/* write Power Management Control/Status register */
-static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
- XenPTReg *cfg_entry, uint16_t *val,
- uint16_t dev_value, uint16_t valid_mask)
-{
- XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
- uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
- uint16_t *data = cfg_entry->ptr.half_word;
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
-
- /* create value for writing to I/O device register */
- *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
- throughable_mask);
-
- return 0;
-}
-
/* Power Management Capability reg static information table */
static XenPTRegInfo xen_pt_emu_reg_pm[] = {
/* Next Pointer reg */
@@ -1051,11 +1036,12 @@ static XenPTRegInfo xen_pt_emu_reg_pm[]
.size = 2,
.init_val = 0x0008,
.res_mask = 0x00F0,
- .ro_mask = 0xE10C,
+ .ro_mask = 0x610C,
+ .rw1c_mask = 0x8000,
.emu_mask = 0x810B,
.init = xen_pt_common_reg_init,
.u.w.read = xen_pt_word_reg_read,
- .u.w.write = xen_pt_pmcsr_reg_write,
+ .u.w.write = xen_pt_word_reg_write,
},
{
.size = 0,
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH v2 3/4] xen/pass-through: correctly deal with RW1C bits
2015-11-24 15:16 ` [Qemu-devel] [PATCH v2 3/4] xen/pass-through: correctly deal with RW1C bits Jan Beulich
@ 2015-12-07 12:43 ` Stefano Stabellini
0 siblings, 0 replies; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-07 12:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 24 Nov 2015, Jan Beulich wrote:
> Introduce yet another mask for them, so that the generic routine can
> handle them, at once rendering xen_pt_pmcsr_reg_write() superfluous.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -113,6 +113,8 @@ struct XenPTRegInfo {
> uint32_t res_mask;
> /* reg read only field mask (ON:RO/ROS, OFF:other) */
> uint32_t ro_mask;
> + /* reg read/write-1-clear field mask (ON:RW1C/RW1CS, OFF:other) */
> + uint32_t rw1c_mask;
> /* reg emulate field mask (ON:emu, OFF:passthrough) */
> uint32_t emu_mask;
> xen_pt_conf_reg_init init;
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -179,7 +179,8 @@ static int xen_pt_byte_reg_write(XenPCIP
> *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>
> /* create value for writing to I/O device register */
> - *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> + *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
> + throughable_mask);
>
> return 0;
> }
> @@ -197,7 +198,8 @@ static int xen_pt_word_reg_write(XenPCIP
> *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>
> /* create value for writing to I/O device register */
> - *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> + *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
> + throughable_mask);
>
> return 0;
> }
> @@ -215,7 +217,8 @@ static int xen_pt_long_reg_write(XenPCIP
> *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>
> /* create value for writing to I/O device register */
> - *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> + *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~reg->rw1c_mask,
> + throughable_mask);
>
> return 0;
> }
> @@ -633,6 +636,7 @@ static XenPTRegInfo xen_pt_emu_reg_heade
> .init_val = 0x0000,
> .res_mask = 0x0007,
> .ro_mask = 0x06F8,
> + .rw1c_mask = 0xF900,
> .emu_mask = 0x0010,
> .init = xen_pt_status_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> @@ -944,6 +948,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
> .size = 2,
> .res_mask = 0xFFC0,
> .ro_mask = 0x0030,
> + .rw1c_mask = 0x000F,
> .init = xen_pt_common_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> .u.w.write = xen_pt_word_reg_write,
> @@ -964,6 +969,7 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
> .offset = PCI_EXP_LNKSTA,
> .size = 2,
> .ro_mask = 0x3FFF,
> + .rw1c_mask = 0xC000,
> .init = xen_pt_common_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> .u.w.write = xen_pt_word_reg_write,
> @@ -1000,27 +1006,6 @@ static XenPTRegInfo xen_pt_emu_reg_pcie[
> * Power Management Capability
> */
>
> -/* write Power Management Control/Status register */
> -static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
> - XenPTReg *cfg_entry, uint16_t *val,
> - uint16_t dev_value, uint16_t valid_mask)
> -{
> - XenPTRegInfo *reg = cfg_entry->reg;
> - uint16_t writable_mask = 0;
> - uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> - uint16_t *data = cfg_entry->ptr.half_word;
> -
> - /* modify emulate register */
> - writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> - *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
> -
> - /* create value for writing to I/O device register */
> - *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
> - throughable_mask);
> -
> - return 0;
> -}
> -
> /* Power Management Capability reg static information table */
> static XenPTRegInfo xen_pt_emu_reg_pm[] = {
> /* Next Pointer reg */
> @@ -1051,11 +1036,12 @@ static XenPTRegInfo xen_pt_emu_reg_pm[]
> .size = 2,
> .init_val = 0x0008,
> .res_mask = 0x00F0,
> - .ro_mask = 0xE10C,
> + .ro_mask = 0x610C,
> + .rw1c_mask = 0x8000,
> .emu_mask = 0x810B,
> .init = xen_pt_common_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> - .u.w.write = xen_pt_pmcsr_reg_write,
> + .u.w.write = xen_pt_word_reg_write,
> },
> {
> .size = 0,
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-11-24 15:09 [Qemu-devel] [PATCH v2 0/4] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
` (2 preceding siblings ...)
2015-11-24 15:16 ` [Qemu-devel] [PATCH v2 3/4] xen/pass-through: correctly deal with RW1C bits Jan Beulich
@ 2015-11-24 15:17 ` Jan Beulich
2015-12-07 12:45 ` Stefano Stabellini
3 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-11-24 15:17 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]
Now that the hypervisor intercepts all config space writes and monitors
changes to the masking flags, this undoes the main effect of the
XSA-129 fix, exposing the masking capability again to guests.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: We probably need to deal with running on an older hypervisor. I
can't, however, immediately see a way for qemu to find out.
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1333,7 +1333,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.init_val = 0x0000,
.res_mask = 0xFE00,
.ro_mask = 0x018E,
- .emu_mask = 0x017E,
+ .emu_mask = 0x007E,
.init = xen_pt_msgctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msgctrl_reg_write,
@@ -1387,8 +1387,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.offset = PCI_MSI_MASK_32,
.size = 4,
.init_val = 0x00000000,
- .ro_mask = 0xFFFFFFFF,
- .emu_mask = 0xFFFFFFFF,
+ .ro_mask = 0x00000000,
+ .emu_mask = 0x00000000,
.init = xen_pt_mask_reg_init,
.u.dw.read = xen_pt_long_reg_read,
.u.dw.write = xen_pt_long_reg_write,
@@ -1398,8 +1398,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.offset = PCI_MSI_MASK_64,
.size = 4,
.init_val = 0x00000000,
- .ro_mask = 0xFFFFFFFF,
- .emu_mask = 0xFFFFFFFF,
+ .ro_mask = 0x00000000,
+ .emu_mask = 0x00000000,
.init = xen_pt_mask_reg_init,
.u.dw.read = xen_pt_long_reg_read,
.u.dw.write = xen_pt_long_reg_write,
[-- Attachment #2: qemu-maskable-MSI.patch --]
[-- Type: text/plain, Size: 1777 bytes --]
xen/MSI: re-expose masking capability
Now that the hypervisor intercepts all config space writes and monitors
changes to the masking flags, this undoes the main effect of the
XSA-129 fix, exposing the masking capability again to guests.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: We probably need to deal with running on an older hypervisor. I
can't, however, immediately see a way for qemu to find out.
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1333,7 +1333,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.init_val = 0x0000,
.res_mask = 0xFE00,
.ro_mask = 0x018E,
- .emu_mask = 0x017E,
+ .emu_mask = 0x007E,
.init = xen_pt_msgctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msgctrl_reg_write,
@@ -1387,8 +1387,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.offset = PCI_MSI_MASK_32,
.size = 4,
.init_val = 0x00000000,
- .ro_mask = 0xFFFFFFFF,
- .emu_mask = 0xFFFFFFFF,
+ .ro_mask = 0x00000000,
+ .emu_mask = 0x00000000,
.init = xen_pt_mask_reg_init,
.u.dw.read = xen_pt_long_reg_read,
.u.dw.write = xen_pt_long_reg_write,
@@ -1398,8 +1398,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
.offset = PCI_MSI_MASK_64,
.size = 4,
.init_val = 0x00000000,
- .ro_mask = 0xFFFFFFFF,
- .emu_mask = 0xFFFFFFFF,
+ .ro_mask = 0x00000000,
+ .emu_mask = 0x00000000,
.init = xen_pt_mask_reg_init,
.u.dw.read = xen_pt_long_reg_read,
.u.dw.write = xen_pt_long_reg_write,
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-11-24 15:17 ` [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability Jan Beulich
@ 2015-12-07 12:45 ` Stefano Stabellini
2015-12-07 13:05 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-07 12:45 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 24 Nov 2015, Jan Beulich wrote:
> Now that the hypervisor intercepts all config space writes and monitors
> changes to the masking flags, this undoes the main effect of the
> XSA-129 fix, exposing the masking capability again to guests.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: We probably need to deal with running on an older hypervisor. I
> can't, however, immediately see a way for qemu to find out.
Actually QEMU has already an infrastructure to detect the hypervisor
version at compile time, see include/hw/xen/xen_common.h. You could
#define the right emu_mask depending on the hypervisor.
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1333,7 +1333,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
> .init_val = 0x0000,
> .res_mask = 0xFE00,
> .ro_mask = 0x018E,
> - .emu_mask = 0x017E,
> + .emu_mask = 0x007E,
> .init = xen_pt_msgctrl_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> .u.w.write = xen_pt_msgctrl_reg_write,
> @@ -1387,8 +1387,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
> .offset = PCI_MSI_MASK_32,
> .size = 4,
> .init_val = 0x00000000,
> - .ro_mask = 0xFFFFFFFF,
> - .emu_mask = 0xFFFFFFFF,
> + .ro_mask = 0x00000000,
> + .emu_mask = 0x00000000,
> .init = xen_pt_mask_reg_init,
> .u.dw.read = xen_pt_long_reg_read,
> .u.dw.write = xen_pt_long_reg_write,
> @@ -1398,8 +1398,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[]
> .offset = PCI_MSI_MASK_64,
> .size = 4,
> .init_val = 0x00000000,
> - .ro_mask = 0xFFFFFFFF,
> - .emu_mask = 0xFFFFFFFF,
> + .ro_mask = 0x00000000,
> + .emu_mask = 0x00000000,
> .init = xen_pt_mask_reg_init,
> .u.dw.read = xen_pt_long_reg_read,
> .u.dw.write = xen_pt_long_reg_write,
>
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-07 12:45 ` Stefano Stabellini
@ 2015-12-07 13:05 ` Jan Beulich
2015-12-07 14:56 ` Stefano Stabellini
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-07 13:05 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 24 Nov 2015, Jan Beulich wrote:
>> Now that the hypervisor intercepts all config space writes and monitors
>> changes to the masking flags, this undoes the main effect of the
>> XSA-129 fix, exposing the masking capability again to guests.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: We probably need to deal with running on an older hypervisor. I
>> can't, however, immediately see a way for qemu to find out.
>
> Actually QEMU has already an infrastructure to detect the hypervisor
> version at compile time, see include/hw/xen/xen_common.h. You could
> #define the right emu_mask depending on the hypervisor.
We don't want compile time detection here, but runtime one.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-07 13:05 ` Jan Beulich
@ 2015-12-07 14:56 ` Stefano Stabellini
2015-12-07 15:35 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-07 14:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Mon, 7 Dec 2015, Jan Beulich wrote:
> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
> > On Tue, 24 Nov 2015, Jan Beulich wrote:
> >> Now that the hypervisor intercepts all config space writes and monitors
> >> changes to the masking flags, this undoes the main effect of the
> >> XSA-129 fix, exposing the masking capability again to guests.
Could you please mention the relevant commit ids in Xen?
What happens if QEMU, with this change, is running on top of an older
Xen that doesn't intercepts all config space writes?
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> TBD: We probably need to deal with running on an older hypervisor. I
> >> can't, however, immediately see a way for qemu to find out.
> >
> > Actually QEMU has already an infrastructure to detect the hypervisor
> > version at compile time, see include/hw/xen/xen_common.h. You could
> > #define the right emu_mask depending on the hypervisor.
>
> We don't want compile time detection here, but runtime one.
I guess the issue is that a fix was backported to Xen that changed its
behaviour in past releases, right?
Is there a way to detect the presence of this fix in Xen, by invoking an
hypercall and checking the returned values and error numbers?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-07 14:56 ` Stefano Stabellini
@ 2015-12-07 15:35 ` Jan Beulich
2015-12-11 14:33 ` Stefano Stabellini
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-07 15:35 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 7 Dec 2015, Jan Beulich wrote:
>> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Tue, 24 Nov 2015, Jan Beulich wrote:
>> >> Now that the hypervisor intercepts all config space writes and monitors
>> >> changes to the masking flags, this undoes the main effect of the
>> >> XSA-129 fix, exposing the masking capability again to guests.
>
> Could you please mention the relevant commit ids in Xen?
It's just one (which I've now added a reference to), unless you want
all the prereqs listed.
> What happens if QEMU, with this change, is running on top of an older
> Xen that doesn't intercepts all config space writes?
The security issue would resurface.
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> >> ---
>> >> TBD: We probably need to deal with running on an older hypervisor. I
>> >> can't, however, immediately see a way for qemu to find out.
>> >
>> > Actually QEMU has already an infrastructure to detect the hypervisor
>> > version at compile time, see include/hw/xen/xen_common.h. You could
>> > #define the right emu_mask depending on the hypervisor.
>>
>> We don't want compile time detection here, but runtime one.
>
> I guess the issue is that a fix was backported to Xen that changed its
> behaviour in past releases, right?
No, we shouldn't try to guess whether this is present in any pre-4.6
hypervisors; we should simply accept that maskable MSI is not
available for guests there, because ...
> Is there a way to detect the presence of this fix in Xen, by invoking an
> hypercall and checking the returned values and error numbers?
... there's nothing to (reliably) probe here. This really is just an
implementation detail of the hypervisor, and hence a version check
is all we have available.
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-07 15:35 ` Jan Beulich
@ 2015-12-11 14:33 ` Stefano Stabellini
2015-12-11 15:18 ` Jan Beulich
0 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-11 14:33 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Mon, 7 Dec 2015, Jan Beulich wrote:
> >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
> >> >> Now that the hypervisor intercepts all config space writes and monitors
> >> >> changes to the masking flags, this undoes the main effect of the
> >> >> XSA-129 fix, exposing the masking capability again to guests.
> >
> > Could you please mention the relevant commit ids in Xen?
>
> It's just one (which I've now added a reference to), unless you want
> all the prereqs listed.
One is probably OK.
> > What happens if QEMU, with this change, is running on top of an older
> > Xen that doesn't intercepts all config space writes?
>
> The security issue would resurface.
>
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> >> ---
> >> >> TBD: We probably need to deal with running on an older hypervisor. I
> >> >> can't, however, immediately see a way for qemu to find out.
> >> >
> >> > Actually QEMU has already an infrastructure to detect the hypervisor
> >> > version at compile time, see include/hw/xen/xen_common.h. You could
> >> > #define the right emu_mask depending on the hypervisor.
> >>
> >> We don't want compile time detection here, but runtime one.
> >
> > I guess the issue is that a fix was backported to Xen that changed its
> > behaviour in past releases, right?
>
> No, we shouldn't try to guess whether this is present in any pre-4.6
> hypervisors; we should simply accept that maskable MSI is not
> available for guests there, because ...
>
> > Is there a way to detect the presence of this fix in Xen, by invoking an
> > hypercall and checking the returned values and error numbers?
>
> ... there's nothing to (reliably) probe here. This really is just an
> implementation detail of the hypervisor, and hence a version check
> is all we have available.
In that case, I think we should stay on the safe side, and only expose
the masking capability (only take into effects the changes that this
patch makes) for Xen >= 4.7.
What do you think?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 14:33 ` Stefano Stabellini
@ 2015-12-11 15:18 ` Jan Beulich
2015-12-11 16:44 ` Stefano Stabellini
0 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-11 15:18 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 11.12.15 at 15:33, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 7 Dec 2015, Jan Beulich wrote:
>> >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Mon, 7 Dec 2015, Jan Beulich wrote:
>> >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
>> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
>> >> >> TBD: We probably need to deal with running on an older hypervisor. I
>> >> >> can't, however, immediately see a way for qemu to find out.
>> >> >
>> >> > Actually QEMU has already an infrastructure to detect the hypervisor
>> >> > version at compile time, see include/hw/xen/xen_common.h. You could
>> >> > #define the right emu_mask depending on the hypervisor.
>> >>
>> >> We don't want compile time detection here, but runtime one.
>> >
>> > I guess the issue is that a fix was backported to Xen that changed its
>> > behaviour in past releases, right?
>>
>> No, we shouldn't try to guess whether this is present in any pre-4.6
>> hypervisors; we should simply accept that maskable MSI is not
>> available for guests there, because ...
>>
>> > Is there a way to detect the presence of this fix in Xen, by invoking an
>> > hypercall and checking the returned values and error numbers?
>>
>> ... there's nothing to (reliably) probe here. This really is just an
>> implementation detail of the hypervisor, and hence a version check
>> is all we have available.
>
> In that case, I think we should stay on the safe side, and only expose
> the masking capability (only take into effects the changes that this
> patch makes) for Xen >= 4.7.
>
> What do you think?
That's what I suggested, with the hope of getting a hint where the
necessary infrastructure is (somehow I didn't find any run time
version dependent code to clone from).
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 15:18 ` Jan Beulich
@ 2015-12-11 16:44 ` Stefano Stabellini
2015-12-11 16:56 ` [Qemu-devel] [Xen-devel] " Ian Campbell
0 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-11 16:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 11 Dec 2015, Jan Beulich wrote:
> >>> On 11.12.15 at 15:33, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Mon, 7 Dec 2015, Jan Beulich wrote:
> >> >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote:
> >> >> > On Tue, 24 Nov 2015, Jan Beulich wrote:
> >> >> >> TBD: We probably need to deal with running on an older hypervisor. I
> >> >> >> can't, however, immediately see a way for qemu to find out.
> >> >> >
> >> >> > Actually QEMU has already an infrastructure to detect the hypervisor
> >> >> > version at compile time, see include/hw/xen/xen_common.h. You could
> >> >> > #define the right emu_mask depending on the hypervisor.
> >> >>
> >> >> We don't want compile time detection here, but runtime one.
> >> >
> >> > I guess the issue is that a fix was backported to Xen that changed its
> >> > behaviour in past releases, right?
> >>
> >> No, we shouldn't try to guess whether this is present in any pre-4.6
> >> hypervisors; we should simply accept that maskable MSI is not
> >> available for guests there, because ...
> >>
> >> > Is there a way to detect the presence of this fix in Xen, by invoking an
> >> > hypercall and checking the returned values and error numbers?
> >>
> >> ... there's nothing to (reliably) probe here. This really is just an
> >> implementation detail of the hypervisor, and hence a version check
> >> is all we have available.
> >
> > In that case, I think we should stay on the safe side, and only expose
> > the masking capability (only take into effects the changes that this
> > patch makes) for Xen >= 4.7.
> >
> > What do you think?
>
> That's what I suggested, with the hope of getting a hint where the
> necessary infrastructure is (somehow I didn't find any run time
> version dependent code to clone from).
It is not possible to do this at runtime. I think we should do this at
compile time because in any case it is not supported to run a QEMU built
for a given Xen version on a different Xen version.
The infrastructure to do this at compile time is in
./include/hw/xen/xen_common.h
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 16:44 ` Stefano Stabellini
@ 2015-12-11 16:56 ` Ian Campbell
2015-12-14 8:02 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Ian Campbell @ 2015-12-11 16:56 UTC (permalink / raw)
To: Stefano Stabellini, Jan Beulich; +Cc: xen-devel, qemu-devel
On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote:
>
> It is not possible to do this at runtime. I think we should do this at
> compile time because in any case it is not supported to run a QEMU built
> for a given Xen version on a different Xen version.
I am currently working pretty hard to make this possible in the future, it
would be a shame to add another reason it wasn't possible at this stage.
I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as the
various stable libraries extracted from libxenctrl we will probably also
want to have a libxendevicemodel.so at some point, to provide a stable way
to interface with all the stuff which being a DM involves.
Maybe that library could contain a way to get this information? (In which
case it could be hardcoded at compile time now and I'll see what I can do
when I get to producing the library).
For the original issue here, could the flag be exposed as a
XEN_SYSCTL_PHYSCAP_????
Ian.
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 16:56 ` [Qemu-devel] [Xen-devel] " Ian Campbell
@ 2015-12-14 8:02 ` Jan Beulich
2015-12-14 11:23 ` Stefano Stabellini
2015-12-14 11:19 ` Stefano Stabellini
2015-12-14 16:01 ` Jan Beulich
2 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-14 8:02 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote:
> On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote:
>>
>> It is not possible to do this at runtime. I think we should do this at
>> compile time because in any case it is not supported to run a QEMU built
>> for a given Xen version on a different Xen version.
>
> I am currently working pretty hard to make this possible in the future, it
> would be a shame to add another reason it wasn't possible at this stage.
And I don't think it's not possible - if anything, the infrastructure to
do so is just missing. I'm definitely not going to make this a build time
check, since I deem it very wrong namely when considering
--with-system-qemu (as in that case there shouldn't be any
dependency on the precise Xen tools versions in use - using plural
intentionally here to point out the possibility of multiple ones being
present).
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-14 8:02 ` Jan Beulich
@ 2015-12-14 11:23 ` Stefano Stabellini
0 siblings, 0 replies; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-14 11:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: qemu-devel, xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 14 Dec 2015, Jan Beulich wrote:
> >>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote:
> >>
> >> It is not possible to do this at runtime. I think we should do this at
> >> compile time because in any case it is not supported to run a QEMU built
> >> for a given Xen version on a different Xen version.
> >
> > I am currently working pretty hard to make this possible in the future, it
> > would be a shame to add another reason it wasn't possible at this stage.
>
> And I don't think it's not possible - if anything, the infrastructure to
> do so is just missing. I'm definitely not going to make this a build time
> check, since I deem it very wrong namely when considering
> --with-system-qemu (as in that case there shouldn't be any
> dependency on the precise Xen tools versions in use - using plural
> intentionally here to point out the possibility of multiple ones being
> present).
Compile time checks are indeed suboptimal, but so are runtime checks:
what if we backport the fix to more Xen releases? What if we revert the
fix on the Xen tree for any reason?
I think that a feature flag is the best course of action.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 16:56 ` [Qemu-devel] [Xen-devel] " Ian Campbell
2015-12-14 8:02 ` Jan Beulich
@ 2015-12-14 11:19 ` Stefano Stabellini
2015-12-14 11:41 ` Ian Campbell
2015-12-14 16:01 ` Jan Beulich
2 siblings, 1 reply; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-14 11:19 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel, qemu-devel, Jan Beulich, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1631 bytes --]
On Fri, 11 Dec 2015, Ian Campbell wrote:
> On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote:
> >
> > It is not possible to do this at runtime. I think we should do this at
> > compile time because in any case it is not supported to run a QEMU built
> > for a given Xen version on a different Xen version.
>
> I am currently working pretty hard to make this possible in the future, it
> would be a shame to add another reason it wasn't possible at this stage.
>
> I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as the
> various stable libraries extracted from libxenctrl we will probably also
> want to have a libxendevicemodel.so at some point, to provide a stable way
> to interface with all the stuff which being a DM involves.
I understand the direction we are heading toward, but unfortunately we
are still pretty far from it. I don't think we want to block this patch
until we have a stable libxendevicemodel ABI? Also this particular
change regards PCI passthrough, which is not convered by the proposed
ABI yet.
> Maybe that library could contain a way to get this information? (In which
> case it could be hardcoded at compile time now and I'll see what I can do
> when I get to producing the library).
Given the choice, I would rather have only compile time or only run time
Xen version checks in QEMU and not both to avoid complexity. Especially
as long as the underlying libraries don't make any stability guarantees.
> For the original issue here, could the flag be exposed as a
> XEN_SYSCTL_PHYSCAP_????
Feature flags are welcome and the best course of action in my opinion.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-14 11:19 ` Stefano Stabellini
@ 2015-12-14 11:41 ` Ian Campbell
0 siblings, 0 replies; 27+ messages in thread
From: Ian Campbell @ 2015-12-14 11:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel, Jan Beulich
On Mon, 2015-12-14 at 11:19 +0000, Stefano Stabellini wrote:
> On Fri, 11 Dec 2015, Ian Campbell wrote:
> > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote:
> > >
> > > It is not possible to do this at runtime. I think we should do this
> > > at
> > > compile time because in any case it is not supported to run a QEMU
> > > built
> > > for a given Xen version on a different Xen version.
> >
> > I am currently working pretty hard to make this possible in the future,
> > it
> > would be a shame to add another reason it wasn't possible at this
> > stage.
> >
> > I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as
> > the
> > various stable libraries extracted from libxenctrl we will probably
> > also
> > want to have a libxendevicemodel.so at some point, to provide a stable
> > way
> > to interface with all the stuff which being a DM involves.
>
> I understand the direction we are heading toward, but unfortunately we
> are still pretty far from it. I don't think we want to block this patch
> until we have a stable libxendevicemodel ABI?
No, but I would appreciate if such things were explicitly considered on a
case by case by case basis rather than just bundled under a generic "it's
not possible yet", since there may be cases where we want to hold off, or
more likely where doing something a particular way now will ease things for
the transition in the future.
> Also this particular
> change regards PCI passthrough, which is not convered by the proposed
> ABI yet.
>
>
> > Maybe that library could contain a way to get this information? (In
> > which
> > case it could be hardcoded at compile time now and I'll see what I can
> > do
> > when I get to producing the library).
>
> Given the choice, I would rather have only compile time or only run time
> Xen version checks in QEMU and not both to avoid complexity. Especially
> as long as the underlying libraries don't make any stability guarantees.
"that library" obviously will make such guarantees as a matter of design.
Ian.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-11 16:56 ` [Qemu-devel] [Xen-devel] " Ian Campbell
2015-12-14 8:02 ` Jan Beulich
2015-12-14 11:19 ` Stefano Stabellini
@ 2015-12-14 16:01 ` Jan Beulich
2015-12-14 16:27 ` Stefano Stabellini
2 siblings, 1 reply; 27+ messages in thread
From: Jan Beulich @ 2015-12-14 16:01 UTC (permalink / raw)
To: Ian Campbell, Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote:
> For the original issue here, could the flag be exposed as a
> XEN_SYSCTL_PHYSCAP_????
Yes, I think it could, albeit calling this a "capability" or "feature"
seems odd (since really the original behavior was bogus/buggy).
But - with sysctl not being a stable interface, is making qemu use
this actually a good idea? I.e. won't we paint ourselves into the
corner of needing to write compatibility wrappers in qemu
whenever XEN_SYSCTL_physinfo (or its libxc wrapper) changes?
Jan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [Xen-devel] [PATCH RFC v2 4/4] xen/MSI: re-expose masking capability
2015-12-14 16:01 ` Jan Beulich
@ 2015-12-14 16:27 ` Stefano Stabellini
0 siblings, 0 replies; 27+ messages in thread
From: Stefano Stabellini @ 2015-12-14 16:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: qemu-devel, xen-devel, Ian Campbell, Stefano Stabellini
On Mon, 14 Dec 2015, Jan Beulich wrote:
> >>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote:
> > For the original issue here, could the flag be exposed as a
> > XEN_SYSCTL_PHYSCAP_????
>
> Yes, I think it could, albeit calling this a "capability" or "feature"
> seems odd (since really the original behavior was bogus/buggy).
> But - with sysctl not being a stable interface, is making qemu use
> this actually a good idea? I.e. won't we paint ourselves into the
> corner of needing to write compatibility wrappers in qemu
> whenever XEN_SYSCTL_physinfo (or its libxc wrapper) changes?
Even though it might increase the number of compatibility wrappers that
we need today, I think that Ian will be able to eventually switch it
over to a stable interface?
^ permalink raw reply [flat|nested] 27+ messages in thread