* [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up
@ 2015-06-05 11:55 Jan Beulich
2015-06-05 11:59 ` Jan Beulich
` (5 more replies)
0 siblings, 6 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 11:55 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
1: MSI-X: latch MSI-X table writes
2: MSI-X: drive maskall and enable bits through hypercalls
3: MSI-X: really enforce alignment
4: pass-through: correctly deal with RW1C bits
5: pass-through: log errno values rather than function return ones
6: pass-through: constify some static data
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 11:59 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 4780 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>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -175,9 +175,8 @@ typedef struct XenPTMSIXEntry {
int pirq;
uint64_t addr;
uint32_t data;
- uint32_t vector_ctrl;
+ uint32_t latch[4];
bool updated; /* indicate whether MSI ADDR or DATA is updated */
- bool warned; /* avoid issuing (bogus) warning more than once */
} XenPTMSIXEntry;
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -25,6 +25,7 @@
#define XEN_PT_GFLAGSSHIFT_DELIV_MODE 12
#define XEN_PT_GFLAGSSHIFT_TRG_MODE 15
+#define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
/*
* Helpers
@@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
+ (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ entry->addr = entry->latch(LOWER_ADDR) |
+ ((uint64_t)entry->latch(UPPER_ADDR) << 32);
+ entry->data = entry->latch(DATA);
+ }
+
rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
if (rc) {
@@ -396,35 +404,15 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- return e->addr & UINT32_MAX;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- return e->addr >> 32;
- case PCI_MSIX_ENTRY_DATA:
- return e->data;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- return e->vector_ctrl;
- default:
- return 0;
- }
+ return !(offset % sizeof(*e->latch))
+ ? e->latch[offset / sizeof(*e->latch)] : 0;
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- e->addr = (e->addr & ((uint64_t)UINT32_MAX << 32)) | val;
- break;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- e->addr = (uint64_t)val << 32 | (e->addr & UINT32_MAX);
- break;
- case PCI_MSIX_ENTRY_DATA:
- e->data = val;
- break;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- e->vector_ctrl = val;
- break;
+ if (!(offset % sizeof(*e->latch)))
+ {
+ e->latch[offset / sizeof(*e->latch)] = val;
}
}
@@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
offset = addr % PCI_MSIX_ENTRY_SIZE;
if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
- const volatile uint32_t *vec_ctrl;
-
if (get_entry_value(entry, offset) == val
&& entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
return;
}
+ entry->updated = true;
+ } else if (msix->enabled && entry->updated &&
+ !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ const volatile uint32_t *vec_ctrl;
+
/*
* If Xen intercepts the mask bit access, entry->vec_ctrl may not be
* up-to-date. Read from hardware directly.
*/
vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL;
+ set_entry_value(entry, offset, *vec_ctrl);
- if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- if (!entry->warned) {
- entry->warned = true;
- XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
- " already enabled.\n", entry_nr);
- }
- return;
- }
-
- entry->updated = true;
+ xen_pt_msix_update_one(s, entry_nr);
}
set_entry_value(entry, offset, val);
-
- if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
- if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- xen_pt_msix_update_one(s, entry_nr);
- }
- }
}
static uint64_t pci_msix_read(void *opaque, hwaddr addr,
[-- Attachment #2: qemu-MSI-X-latch-writes.patch --]
[-- Type: text/plain, Size: 4815 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>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -175,9 +175,8 @@ typedef struct XenPTMSIXEntry {
int pirq;
uint64_t addr;
uint32_t data;
- uint32_t vector_ctrl;
+ uint32_t latch[4];
bool updated; /* indicate whether MSI ADDR or DATA is updated */
- bool warned; /* avoid issuing (bogus) warning more than once */
} XenPTMSIXEntry;
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -25,6 +25,7 @@
#define XEN_PT_GFLAGSSHIFT_DELIV_MODE 12
#define XEN_PT_GFLAGSSHIFT_TRG_MODE 15
+#define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
/*
* Helpers
@@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
+ (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ entry->addr = entry->latch(LOWER_ADDR) |
+ ((uint64_t)entry->latch(UPPER_ADDR) << 32);
+ entry->data = entry->latch(DATA);
+ }
+
rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
if (rc) {
@@ -396,35 +404,15 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- return e->addr & UINT32_MAX;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- return e->addr >> 32;
- case PCI_MSIX_ENTRY_DATA:
- return e->data;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- return e->vector_ctrl;
- default:
- return 0;
- }
+ return !(offset % sizeof(*e->latch))
+ ? e->latch[offset / sizeof(*e->latch)] : 0;
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- e->addr = (e->addr & ((uint64_t)UINT32_MAX << 32)) | val;
- break;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- e->addr = (uint64_t)val << 32 | (e->addr & UINT32_MAX);
- break;
- case PCI_MSIX_ENTRY_DATA:
- e->data = val;
- break;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- e->vector_ctrl = val;
- break;
+ if (!(offset % sizeof(*e->latch)))
+ {
+ e->latch[offset / sizeof(*e->latch)] = val;
}
}
@@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
offset = addr % PCI_MSIX_ENTRY_SIZE;
if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
- const volatile uint32_t *vec_ctrl;
-
if (get_entry_value(entry, offset) == val
&& entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
return;
}
+ entry->updated = true;
+ } else if (msix->enabled && entry->updated &&
+ !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ const volatile uint32_t *vec_ctrl;
+
/*
* If Xen intercepts the mask bit access, entry->vec_ctrl may not be
* up-to-date. Read from hardware directly.
*/
vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL;
+ set_entry_value(entry, offset, *vec_ctrl);
- if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- if (!entry->warned) {
- entry->warned = true;
- XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
- " already enabled.\n", entry_nr);
- }
- return;
- }
-
- entry->updated = true;
+ xen_pt_msix_update_one(s, entry_nr);
}
set_entry_value(entry, offset, val);
-
- if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
- if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- xen_pt_msix_update_one(s, entry_nr);
- }
- }
}
static uint64_t pci_msix_read(void *opaque, hwaddr addr,
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
@ 2015-06-05 11:59 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 11:59 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 4780 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>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -175,9 +175,8 @@ typedef struct XenPTMSIXEntry {
int pirq;
uint64_t addr;
uint32_t data;
- uint32_t vector_ctrl;
+ uint32_t latch[4];
bool updated; /* indicate whether MSI ADDR or DATA is updated */
- bool warned; /* avoid issuing (bogus) warning more than once */
} XenPTMSIXEntry;
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -25,6 +25,7 @@
#define XEN_PT_GFLAGSSHIFT_DELIV_MODE 12
#define XEN_PT_GFLAGSSHIFT_TRG_MODE 15
+#define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
/*
* Helpers
@@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
+ (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ entry->addr = entry->latch(LOWER_ADDR) |
+ ((uint64_t)entry->latch(UPPER_ADDR) << 32);
+ entry->data = entry->latch(DATA);
+ }
+
rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
if (rc) {
@@ -396,35 +404,15 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- return e->addr & UINT32_MAX;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- return e->addr >> 32;
- case PCI_MSIX_ENTRY_DATA:
- return e->data;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- return e->vector_ctrl;
- default:
- return 0;
- }
+ return !(offset % sizeof(*e->latch))
+ ? e->latch[offset / sizeof(*e->latch)] : 0;
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- e->addr = (e->addr & ((uint64_t)UINT32_MAX << 32)) | val;
- break;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- e->addr = (uint64_t)val << 32 | (e->addr & UINT32_MAX);
- break;
- case PCI_MSIX_ENTRY_DATA:
- e->data = val;
- break;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- e->vector_ctrl = val;
- break;
+ if (!(offset % sizeof(*e->latch)))
+ {
+ e->latch[offset / sizeof(*e->latch)] = val;
}
}
@@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
offset = addr % PCI_MSIX_ENTRY_SIZE;
if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
- const volatile uint32_t *vec_ctrl;
-
if (get_entry_value(entry, offset) == val
&& entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
return;
}
+ entry->updated = true;
+ } else if (msix->enabled && entry->updated &&
+ !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ const volatile uint32_t *vec_ctrl;
+
/*
* If Xen intercepts the mask bit access, entry->vec_ctrl may not be
* up-to-date. Read from hardware directly.
*/
vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL;
+ set_entry_value(entry, offset, *vec_ctrl);
- if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- if (!entry->warned) {
- entry->warned = true;
- XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
- " already enabled.\n", entry_nr);
- }
- return;
- }
-
- entry->updated = true;
+ xen_pt_msix_update_one(s, entry_nr);
}
set_entry_value(entry, offset, val);
-
- if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
- if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- xen_pt_msix_update_one(s, entry_nr);
- }
- }
}
static uint64_t pci_msix_read(void *opaque, hwaddr addr,
[-- Attachment #2: qemu-MSI-X-latch-writes.patch --]
[-- Type: text/plain, Size: 4815 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>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -175,9 +175,8 @@ typedef struct XenPTMSIXEntry {
int pirq;
uint64_t addr;
uint32_t data;
- uint32_t vector_ctrl;
+ uint32_t latch[4];
bool updated; /* indicate whether MSI ADDR or DATA is updated */
- bool warned; /* avoid issuing (bogus) warning more than once */
} XenPTMSIXEntry;
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -25,6 +25,7 @@
#define XEN_PT_GFLAGSSHIFT_DELIV_MODE 12
#define XEN_PT_GFLAGSSHIFT_TRG_MODE 15
+#define latch(fld) latch[PCI_MSIX_ENTRY_##fld / sizeof(uint32_t)]
/*
* Helpers
@@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
pirq = entry->pirq;
+ if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
+ (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ entry->addr = entry->latch(LOWER_ADDR) |
+ ((uint64_t)entry->latch(UPPER_ADDR) << 32);
+ entry->data = entry->latch(DATA);
+ }
+
rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
if (rc) {
@@ -396,35 +404,15 @@ int xen_pt_msix_update_remap(XenPCIPasst
static uint32_t get_entry_value(XenPTMSIXEntry *e, int offset)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- return e->addr & UINT32_MAX;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- return e->addr >> 32;
- case PCI_MSIX_ENTRY_DATA:
- return e->data;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- return e->vector_ctrl;
- default:
- return 0;
- }
+ return !(offset % sizeof(*e->latch))
+ ? e->latch[offset / sizeof(*e->latch)] : 0;
}
static void set_entry_value(XenPTMSIXEntry *e, int offset, uint32_t val)
{
- switch (offset) {
- case PCI_MSIX_ENTRY_LOWER_ADDR:
- e->addr = (e->addr & ((uint64_t)UINT32_MAX << 32)) | val;
- break;
- case PCI_MSIX_ENTRY_UPPER_ADDR:
- e->addr = (uint64_t)val << 32 | (e->addr & UINT32_MAX);
- break;
- case PCI_MSIX_ENTRY_DATA:
- e->data = val;
- break;
- case PCI_MSIX_ENTRY_VECTOR_CTRL:
- e->vector_ctrl = val;
- break;
+ if (!(offset % sizeof(*e->latch)))
+ {
+ e->latch[offset / sizeof(*e->latch)] = val;
}
}
@@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
offset = addr % PCI_MSIX_ENTRY_SIZE;
if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
- const volatile uint32_t *vec_ctrl;
-
if (get_entry_value(entry, offset) == val
&& entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
return;
}
+ entry->updated = true;
+ } else if (msix->enabled && entry->updated &&
+ !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
+ const volatile uint32_t *vec_ctrl;
+
/*
* If Xen intercepts the mask bit access, entry->vec_ctrl may not be
* up-to-date. Read from hardware directly.
*/
vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL;
+ set_entry_value(entry, offset, *vec_ctrl);
- if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- if (!entry->warned) {
- entry->warned = true;
- XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
- " already enabled.\n", entry_nr);
- }
- return;
- }
-
- entry->updated = true;
+ xen_pt_msix_update_one(s, entry_nr);
}
set_entry_value(entry, offset, val);
-
- if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
- if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
- xen_pt_msix_update_one(s, entry_nr);
- }
- }
}
static uint64_t pci_msix_read(void *opaque, hwaddr addr,
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 12:01 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:01 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 5899 bytes --]
Particularly the maskall bit has to be under exclusive hypervisor
control (and since they live in the same config space field, the
enable bit has to follow suit). Use the replacement hypercall
interfaces.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
bool enabled;
+ bool maskall;
int total_entries;
int bar_index;
uint64_t table_base;
@@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
void xen_pt_msix_delete(XenPCIPassthroughState *s);
int xen_pt_msix_update(XenPCIPassthroughState *s);
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
+void xen_pt_msix_enable(XenPCIPassthroughState *s);
void xen_pt_msix_disable(XenPCIPassthroughState *s);
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
{
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
uint16_t dev_value, uint16_t valid_mask)
{
XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
+ uint16_t writable_mask, value;
uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
int debug_msix_enabled_old;
/* modify emulate register */
writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ cfg_entry->data = value;
/* create value for writing to I/O device register */
*val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ debug_msix_enabled_old = s->msix->enabled;
+
/* update MSI-X */
- if ((*val & PCI_MSIX_FLAGS_ENABLE)
- && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
+ if ((value & PCI_MSIX_FLAGS_ENABLE)
+ && !(value & PCI_MSIX_FLAGS_MASKALL)) {
+ if (!s->msix->enabled) {
+ if (!s->msix->maskall) {
+ xen_pt_msix_maskall(s, true);
+ }
+ xen_pt_msix_enable(s);
+ }
xen_pt_msix_update(s);
- } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
- xen_pt_msix_disable(s);
+ s->msix->enabled = true;
+ s->msix->maskall = false;
+ xen_pt_msix_maskall(s, false);
+ } else if (s->msix->enabled) {
+ if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
+ xen_pt_msix_disable(s);
+ s->msix->enabled = false;
+ } else if (!s->msix->maskall) {
+ s->msix->maskall = true;
+ xen_pt_msix_maskall(s, true);
+ }
}
- debug_msix_enabled_old = s->msix->enabled;
- s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
if (s->msix->enabled != debug_msix_enabled_old) {
XEN_PT_LOG(&s->dev, "%s MSI-X\n",
s->msix->enabled ? "enable" : "disable");
}
+ xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
+
+ if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
+ } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
+ s->msix->maskall &&
+ !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
+ }
+
return 0;
}
@@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
.offset = PCI_MSI_FLAGS,
.size = 2,
.init_val = 0x0000,
- .res_mask = 0x3800,
- .ro_mask = 0x07FF,
- .emu_mask = 0x0000,
+ /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
+ * because even in permissive mode there must not be any write back
+ * to this register.
+ */
+ .ro_mask = 0x3FFF,
+ .emu_mask = 0xC000,
.init = xen_pt_msixctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msixctrl_reg_write,
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
return -1;
}
- return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
- enabled);
+ return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ enabled);
}
static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
@@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
return 0;
}
+void xen_pt_msix_enable(XenPCIPassthroughState *s)
+{
+ msix_set_enable(s, true);
+}
+
void xen_pt_msix_disable(XenPCIPassthroughState *s)
{
int i = 0;
@@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
}
}
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
+{
+ return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ mask);
+}
+
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
{
XenPTMSIXEntry *entry;
[-- Attachment #2: qemu-MSI-X-enable-maskall.patch --]
[-- Type: text/plain, Size: 5958 bytes --]
xen/MSI-X: drive maskall and enable bits through hypercalls
Particularly the maskall bit has to be under exclusive hypervisor
control (and since they live in the same config space field, the
enable bit has to follow suit). Use the replacement hypercall
interfaces.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
bool enabled;
+ bool maskall;
int total_entries;
int bar_index;
uint64_t table_base;
@@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
void xen_pt_msix_delete(XenPCIPassthroughState *s);
int xen_pt_msix_update(XenPCIPassthroughState *s);
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
+void xen_pt_msix_enable(XenPCIPassthroughState *s);
void xen_pt_msix_disable(XenPCIPassthroughState *s);
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
{
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
uint16_t dev_value, uint16_t valid_mask)
{
XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
+ uint16_t writable_mask, value;
uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
int debug_msix_enabled_old;
/* modify emulate register */
writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ cfg_entry->data = value;
/* create value for writing to I/O device register */
*val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ debug_msix_enabled_old = s->msix->enabled;
+
/* update MSI-X */
- if ((*val & PCI_MSIX_FLAGS_ENABLE)
- && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
+ if ((value & PCI_MSIX_FLAGS_ENABLE)
+ && !(value & PCI_MSIX_FLAGS_MASKALL)) {
+ if (!s->msix->enabled) {
+ if (!s->msix->maskall) {
+ xen_pt_msix_maskall(s, true);
+ }
+ xen_pt_msix_enable(s);
+ }
xen_pt_msix_update(s);
- } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
- xen_pt_msix_disable(s);
+ s->msix->enabled = true;
+ s->msix->maskall = false;
+ xen_pt_msix_maskall(s, false);
+ } else if (s->msix->enabled) {
+ if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
+ xen_pt_msix_disable(s);
+ s->msix->enabled = false;
+ } else if (!s->msix->maskall) {
+ s->msix->maskall = true;
+ xen_pt_msix_maskall(s, true);
+ }
}
- debug_msix_enabled_old = s->msix->enabled;
- s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
if (s->msix->enabled != debug_msix_enabled_old) {
XEN_PT_LOG(&s->dev, "%s MSI-X\n",
s->msix->enabled ? "enable" : "disable");
}
+ xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
+
+ if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
+ } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
+ s->msix->maskall &&
+ !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
+ }
+
return 0;
}
@@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
.offset = PCI_MSI_FLAGS,
.size = 2,
.init_val = 0x0000,
- .res_mask = 0x3800,
- .ro_mask = 0x07FF,
- .emu_mask = 0x0000,
+ /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
+ * because even in permissive mode there must not be any write back
+ * to this register.
+ */
+ .ro_mask = 0x3FFF,
+ .emu_mask = 0xC000,
.init = xen_pt_msixctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msixctrl_reg_write,
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
return -1;
}
- return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
- enabled);
+ return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ enabled);
}
static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
@@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
return 0;
}
+void xen_pt_msix_enable(XenPCIPassthroughState *s)
+{
+ msix_set_enable(s, true);
+}
+
void xen_pt_msix_disable(XenPCIPassthroughState *s)
{
int i = 0;
@@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
}
}
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
+{
+ return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ mask);
+}
+
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
{
XenPTMSIXEntry *entry;
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
@ 2015-06-05 12:01 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:01 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 5899 bytes --]
Particularly the maskall bit has to be under exclusive hypervisor
control (and since they live in the same config space field, the
enable bit has to follow suit). Use the replacement hypercall
interfaces.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
bool enabled;
+ bool maskall;
int total_entries;
int bar_index;
uint64_t table_base;
@@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
void xen_pt_msix_delete(XenPCIPassthroughState *s);
int xen_pt_msix_update(XenPCIPassthroughState *s);
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
+void xen_pt_msix_enable(XenPCIPassthroughState *s);
void xen_pt_msix_disable(XenPCIPassthroughState *s);
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
{
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
uint16_t dev_value, uint16_t valid_mask)
{
XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
+ uint16_t writable_mask, value;
uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
int debug_msix_enabled_old;
/* modify emulate register */
writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ cfg_entry->data = value;
/* create value for writing to I/O device register */
*val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ debug_msix_enabled_old = s->msix->enabled;
+
/* update MSI-X */
- if ((*val & PCI_MSIX_FLAGS_ENABLE)
- && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
+ if ((value & PCI_MSIX_FLAGS_ENABLE)
+ && !(value & PCI_MSIX_FLAGS_MASKALL)) {
+ if (!s->msix->enabled) {
+ if (!s->msix->maskall) {
+ xen_pt_msix_maskall(s, true);
+ }
+ xen_pt_msix_enable(s);
+ }
xen_pt_msix_update(s);
- } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
- xen_pt_msix_disable(s);
+ s->msix->enabled = true;
+ s->msix->maskall = false;
+ xen_pt_msix_maskall(s, false);
+ } else if (s->msix->enabled) {
+ if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
+ xen_pt_msix_disable(s);
+ s->msix->enabled = false;
+ } else if (!s->msix->maskall) {
+ s->msix->maskall = true;
+ xen_pt_msix_maskall(s, true);
+ }
}
- debug_msix_enabled_old = s->msix->enabled;
- s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
if (s->msix->enabled != debug_msix_enabled_old) {
XEN_PT_LOG(&s->dev, "%s MSI-X\n",
s->msix->enabled ? "enable" : "disable");
}
+ xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
+
+ if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
+ } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
+ s->msix->maskall &&
+ !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
+ }
+
return 0;
}
@@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
.offset = PCI_MSI_FLAGS,
.size = 2,
.init_val = 0x0000,
- .res_mask = 0x3800,
- .ro_mask = 0x07FF,
- .emu_mask = 0x0000,
+ /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
+ * because even in permissive mode there must not be any write back
+ * to this register.
+ */
+ .ro_mask = 0x3FFF,
+ .emu_mask = 0xC000,
.init = xen_pt_msixctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msixctrl_reg_write,
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
return -1;
}
- return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
- enabled);
+ return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ enabled);
}
static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
@@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
return 0;
}
+void xen_pt_msix_enable(XenPCIPassthroughState *s)
+{
+ msix_set_enable(s, true);
+}
+
void xen_pt_msix_disable(XenPCIPassthroughState *s)
{
int i = 0;
@@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
}
}
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
+{
+ return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ mask);
+}
+
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
{
XenPTMSIXEntry *entry;
[-- Attachment #2: qemu-MSI-X-enable-maskall.patch --]
[-- Type: text/plain, Size: 5958 bytes --]
xen/MSI-X: drive maskall and enable bits through hypercalls
Particularly the maskall bit has to be under exclusive hypervisor
control (and since they live in the same config space field, the
enable bit has to follow suit). Use the replacement hypercall
interfaces.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
typedef struct XenPTMSIX {
uint32_t ctrl_offset;
bool enabled;
+ bool maskall;
int total_entries;
int bar_index;
uint64_t table_base;
@@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
void xen_pt_msix_delete(XenPCIPassthroughState *s);
int xen_pt_msix_update(XenPCIPassthroughState *s);
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
+void xen_pt_msix_enable(XenPCIPassthroughState *s);
void xen_pt_msix_disable(XenPCIPassthroughState *s);
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
{
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
uint16_t dev_value, uint16_t valid_mask)
{
XenPTRegInfo *reg = cfg_entry->reg;
- uint16_t writable_mask = 0;
+ uint16_t writable_mask, value;
uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
int debug_msix_enabled_old;
/* modify emulate register */
writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+ cfg_entry->data = value;
/* create value for writing to I/O device register */
*val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
+ debug_msix_enabled_old = s->msix->enabled;
+
/* update MSI-X */
- if ((*val & PCI_MSIX_FLAGS_ENABLE)
- && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
+ if ((value & PCI_MSIX_FLAGS_ENABLE)
+ && !(value & PCI_MSIX_FLAGS_MASKALL)) {
+ if (!s->msix->enabled) {
+ if (!s->msix->maskall) {
+ xen_pt_msix_maskall(s, true);
+ }
+ xen_pt_msix_enable(s);
+ }
xen_pt_msix_update(s);
- } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
- xen_pt_msix_disable(s);
+ s->msix->enabled = true;
+ s->msix->maskall = false;
+ xen_pt_msix_maskall(s, false);
+ } else if (s->msix->enabled) {
+ if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
+ xen_pt_msix_disable(s);
+ s->msix->enabled = false;
+ } else if (!s->msix->maskall) {
+ s->msix->maskall = true;
+ xen_pt_msix_maskall(s, true);
+ }
}
- debug_msix_enabled_old = s->msix->enabled;
- s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
if (s->msix->enabled != debug_msix_enabled_old) {
XEN_PT_LOG(&s->dev, "%s MSI-X\n",
s->msix->enabled ? "enable" : "disable");
}
+ xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
+
+ if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
+ } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
+ s->msix->maskall &&
+ !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
+ XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
+ }
+
return 0;
}
@@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
.offset = PCI_MSI_FLAGS,
.size = 2,
.init_val = 0x0000,
- .res_mask = 0x3800,
- .ro_mask = 0x07FF,
- .emu_mask = 0x0000,
+ /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
+ * because even in permissive mode there must not be any write back
+ * to this register.
+ */
+ .ro_mask = 0x3FFF,
+ .emu_mask = 0xC000,
.init = xen_pt_msixctrl_reg_init,
.u.w.read = xen_pt_word_reg_read,
.u.w.write = xen_pt_msixctrl_reg_write,
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
return -1;
}
- return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
- enabled);
+ return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ enabled);
}
static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
@@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
return 0;
}
+void xen_pt_msix_enable(XenPCIPassthroughState *s)
+{
+ msix_set_enable(s, true);
+}
+
void xen_pt_msix_disable(XenPCIPassthroughState *s)
{
int i = 0;
@@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
}
}
+int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
+{
+ return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
+ s->real_device.bus,
+ PCI_DEVFN(s->real_device.dev,
+ s->real_device.func),
+ mask);
+}
+
int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
{
XenPTMSIXEntry *entry;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 12:02 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:02 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1821 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>
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -421,16 +421,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,
@@ -496,6 +494,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,
@@ -504,7 +508,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: 1854 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>
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -421,16 +421,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,
@@ -496,6 +494,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,
@@ -504,7 +508,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] 41+ messages in thread
* [PATCH 3/6] xen/MSI-X: really enforce alignment
@ 2015-06-05 12:02 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:02 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1821 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>
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -421,16 +421,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,
@@ -496,6 +494,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,
@@ -504,7 +508,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: 1854 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>
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -421,16 +421,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,
@@ -496,6 +494,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,
@@ -504,7 +508,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 #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 12:03 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:03 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 4544 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/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -611,6 +614,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,
@@ -910,6 +914,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,
@@ -930,6 +935,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,
@@ -966,26 +972,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);
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
@@ -1016,11 +1002,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: 4591 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/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -611,6 +614,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,
@@ -910,6 +914,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,
@@ -930,6 +935,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,
@@ -966,26 +972,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);
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
@@ -1016,11 +1002,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] 41+ messages in thread
* [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
@ 2015-06-05 12:03 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:03 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 4544 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/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -611,6 +614,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,
@@ -910,6 +914,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,
@@ -930,6 +935,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,
@@ -966,26 +972,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);
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
@@ -1016,11 +1002,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: 4591 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/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
}
@@ -611,6 +614,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,
@@ -910,6 +914,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,
@@ -930,6 +935,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,
@@ -966,26 +972,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);
-
- /* modify emulate register */
- writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
- cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
@@ -1016,11 +1002,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 #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 12:04 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:04 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 6871 bytes --]
Functions setting errno commonly return just -1, which is of no
particular use in the log file.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.c
+++ b/qemu/upstream/hw/xen/xen_pt.c
@@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
guest_port, machine_port, size,
op);
if (rc) {
- XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
} else {
pcibus_t guest_addr = sec->offset_within_address_space;
@@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
XEN_PFN(size + XC_PAGE_SIZE - 1),
op);
if (rc) {
- XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
}
}
@@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
if (rc < 0) {
- XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
- machine_irq, pirq, rc);
+ XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+ machine_irq, pirq, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
- XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
- e_intx, rc);
+ XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+ e_intx, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
- " (rc: %d)\n", machine_irq, rc);
+ " (err: %d)\n", machine_irq, errno);
}
}
s->machine_irq = 0;
@@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
0 /* isa_irq */);
if (rc < 0) {
XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
- " (machine irq: %i, rc: %d)"
+ " (machine irq: %i, err: %d)"
" But bravely continuing on..\n",
- 'a' + intx, machine_irq, rc);
+ 'a' + intx, machine_irq, errno);
}
}
@@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
if (rc < 0) {
- XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
+ XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
" But bravely continuing on..\n",
- machine_irq, rc);
+ machine_irq, errno);
}
}
}
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
msix_entry, table_base);
if (rc) {
XEN_PT_ERR(&s->dev,
- "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
- is_msix ? "-X" : "", rc, gvec, msix_entry);
+ "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
+ is_msix ? "-X" : "", errno, gvec, msix_entry);
return rc;
}
}
@@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
pirq, gflags, table_addr);
if (rc) {
- XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
- is_msix ? "-X" : "", rc);
+ XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
+ is_msix ? "-X" : "", errno);
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
- is_msix ? "-X" : "", *old_pirq);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
+ is_msix ? "-X" : "", *old_pirq, errno);
}
*old_pirq = XEN_PT_UNASSIGNED_PIRQ;
}
@@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
is_msix ? "-X" : "", pirq, gvec);
rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
if (rc) {
- XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
- is_msix ? "-X" : "", pirq, gvec);
+ XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+ is_msix ? "-X" : "", errno, pirq, gvec);
return rc;
}
}
@@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
if (rc) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
- is_msix ? "-X" : "", pirq, rc);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
+ is_msix ? "-X" : "", pirq, errno);
return rc;
}
@@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
if (ret) {
- XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
- entry->pirq);
+ XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
+ entry->pirq, errno);
}
entry->updated = true;
}
[-- Attachment #2: qemu-pt-log-errno.patch --]
[-- Type: text/plain, Size: 6938 bytes --]
xen/pass-through: log errno values rather than function return ones
Functions setting errno commonly return just -1, which is of no
particular use in the log file.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.c
+++ b/qemu/upstream/hw/xen/xen_pt.c
@@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
guest_port, machine_port, size,
op);
if (rc) {
- XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
} else {
pcibus_t guest_addr = sec->offset_within_address_space;
@@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
XEN_PFN(size + XC_PAGE_SIZE - 1),
op);
if (rc) {
- XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
}
}
@@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
if (rc < 0) {
- XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
- machine_irq, pirq, rc);
+ XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+ machine_irq, pirq, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
- XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
- e_intx, rc);
+ XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+ e_intx, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
- " (rc: %d)\n", machine_irq, rc);
+ " (err: %d)\n", machine_irq, errno);
}
}
s->machine_irq = 0;
@@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
0 /* isa_irq */);
if (rc < 0) {
XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
- " (machine irq: %i, rc: %d)"
+ " (machine irq: %i, err: %d)"
" But bravely continuing on..\n",
- 'a' + intx, machine_irq, rc);
+ 'a' + intx, machine_irq, errno);
}
}
@@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
if (rc < 0) {
- XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
+ XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
" But bravely continuing on..\n",
- machine_irq, rc);
+ machine_irq, errno);
}
}
}
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
msix_entry, table_base);
if (rc) {
XEN_PT_ERR(&s->dev,
- "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
- is_msix ? "-X" : "", rc, gvec, msix_entry);
+ "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
+ is_msix ? "-X" : "", errno, gvec, msix_entry);
return rc;
}
}
@@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
pirq, gflags, table_addr);
if (rc) {
- XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
- is_msix ? "-X" : "", rc);
+ XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
+ is_msix ? "-X" : "", errno);
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
- is_msix ? "-X" : "", *old_pirq);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
+ is_msix ? "-X" : "", *old_pirq, errno);
}
*old_pirq = XEN_PT_UNASSIGNED_PIRQ;
}
@@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
is_msix ? "-X" : "", pirq, gvec);
rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
if (rc) {
- XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
- is_msix ? "-X" : "", pirq, gvec);
+ XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+ is_msix ? "-X" : "", errno, pirq, gvec);
return rc;
}
}
@@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
if (rc) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
- is_msix ? "-X" : "", pirq, rc);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
+ is_msix ? "-X" : "", pirq, errno);
return rc;
}
@@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
if (ret) {
- XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
- entry->pirq);
+ XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
+ entry->pirq, errno);
}
entry->updated = true;
}
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 5/6] xen/pass-through: log errno values rather than function return ones
@ 2015-06-05 12:04 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:04 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 6871 bytes --]
Functions setting errno commonly return just -1, which is of no
particular use in the log file.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.c
+++ b/qemu/upstream/hw/xen/xen_pt.c
@@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
guest_port, machine_port, size,
op);
if (rc) {
- XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
} else {
pcibus_t guest_addr = sec->offset_within_address_space;
@@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
XEN_PFN(size + XC_PAGE_SIZE - 1),
op);
if (rc) {
- XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
}
}
@@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
if (rc < 0) {
- XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
- machine_irq, pirq, rc);
+ XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+ machine_irq, pirq, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
- XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
- e_intx, rc);
+ XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+ e_intx, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
- " (rc: %d)\n", machine_irq, rc);
+ " (err: %d)\n", machine_irq, errno);
}
}
s->machine_irq = 0;
@@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
0 /* isa_irq */);
if (rc < 0) {
XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
- " (machine irq: %i, rc: %d)"
+ " (machine irq: %i, err: %d)"
" But bravely continuing on..\n",
- 'a' + intx, machine_irq, rc);
+ 'a' + intx, machine_irq, errno);
}
}
@@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
if (rc < 0) {
- XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
+ XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
" But bravely continuing on..\n",
- machine_irq, rc);
+ machine_irq, errno);
}
}
}
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
msix_entry, table_base);
if (rc) {
XEN_PT_ERR(&s->dev,
- "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
- is_msix ? "-X" : "", rc, gvec, msix_entry);
+ "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
+ is_msix ? "-X" : "", errno, gvec, msix_entry);
return rc;
}
}
@@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
pirq, gflags, table_addr);
if (rc) {
- XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
- is_msix ? "-X" : "", rc);
+ XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
+ is_msix ? "-X" : "", errno);
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
- is_msix ? "-X" : "", *old_pirq);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
+ is_msix ? "-X" : "", *old_pirq, errno);
}
*old_pirq = XEN_PT_UNASSIGNED_PIRQ;
}
@@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
is_msix ? "-X" : "", pirq, gvec);
rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
if (rc) {
- XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
- is_msix ? "-X" : "", pirq, gvec);
+ XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+ is_msix ? "-X" : "", errno, pirq, gvec);
return rc;
}
}
@@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
if (rc) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
- is_msix ? "-X" : "", pirq, rc);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
+ is_msix ? "-X" : "", pirq, errno);
return rc;
}
@@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
if (ret) {
- XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
- entry->pirq);
+ XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
+ entry->pirq, errno);
}
entry->updated = true;
}
[-- Attachment #2: qemu-pt-log-errno.patch --]
[-- Type: text/plain, Size: 6938 bytes --]
xen/pass-through: log errno values rather than function return ones
Functions setting errno commonly return just -1, which is of no
particular use in the log file.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.c
+++ b/qemu/upstream/hw/xen/xen_pt.c
@@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
guest_port, machine_port, size,
op);
if (rc) {
- XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
} else {
pcibus_t guest_addr = sec->offset_within_address_space;
@@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
XEN_PFN(size + XC_PAGE_SIZE - 1),
op);
if (rc) {
- XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
- adding ? "create new" : "remove old", rc);
+ XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
+ adding ? "create new" : "remove old", errno);
}
}
}
@@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
if (rc < 0) {
- XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
- machine_irq, pirq, rc);
+ XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
+ machine_irq, pirq, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
PCI_SLOT(d->devfn),
e_intx);
if (rc < 0) {
- XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
- e_intx, rc);
+ XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
+ e_intx, errno);
/* Disable PCI intx assertion (turn on bit10 of devctl) */
cmd |= PCI_COMMAND_INTX_DISABLE;
@@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
- " (rc: %d)\n", machine_irq, rc);
+ " (err: %d)\n", machine_irq, errno);
}
}
s->machine_irq = 0;
@@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
0 /* isa_irq */);
if (rc < 0) {
XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
- " (machine irq: %i, rc: %d)"
+ " (machine irq: %i, err: %d)"
" But bravely continuing on..\n",
- 'a' + intx, machine_irq, rc);
+ 'a' + intx, machine_irq, errno);
}
}
@@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
if (rc < 0) {
- XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
+ XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
" But bravely continuing on..\n",
- machine_irq, rc);
+ machine_irq, errno);
}
}
}
--- a/qemu/upstream/hw/xen/xen_pt_msi.c
+++ b/qemu/upstream/hw/xen/xen_pt_msi.c
@@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
msix_entry, table_base);
if (rc) {
XEN_PT_ERR(&s->dev,
- "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
- is_msix ? "-X" : "", rc, gvec, msix_entry);
+ "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
+ is_msix ? "-X" : "", errno, gvec, msix_entry);
return rc;
}
}
@@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
pirq, gflags, table_addr);
if (rc) {
- XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
- is_msix ? "-X" : "", rc);
+ XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
+ is_msix ? "-X" : "", errno);
if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
- is_msix ? "-X" : "", *old_pirq);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
+ is_msix ? "-X" : "", *old_pirq, errno);
}
*old_pirq = XEN_PT_UNASSIGNED_PIRQ;
}
@@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
is_msix ? "-X" : "", pirq, gvec);
rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
if (rc) {
- XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
- is_msix ? "-X" : "", pirq, gvec);
+ XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
+ is_msix ? "-X" : "", errno, pirq, gvec);
return rc;
}
}
@@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
if (rc) {
- XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
- is_msix ? "-X" : "", pirq, rc);
+ XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
+ is_msix ? "-X" : "", pirq, errno);
return rc;
}
@@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
if (ret) {
- XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
- entry->pirq);
+ XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
+ entry->pirq, errno);
}
entry->updated = true;
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
@ 2015-06-05 12:04 ` Jan Beulich
2015-06-05 12:01 ` Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:04 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]
This is done indirectly by adjusting two typedefs and helps emphasizing
that the respective tables aren't supposed to be modified at runtime
(as they may be shared between devices).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
/* Helper */
#define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
-typedef struct XenPTRegInfo XenPTRegInfo;
+typedef const struct XenPTRegInfo XenPTRegInfo;
typedef struct XenPTReg XenPTReg;
typedef struct XenPCIPassthroughState XenPCIPassthroughState;
@@ -135,11 +135,11 @@ struct XenPTReg {
uint32_t data; /* emulated value */
};
-typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
+typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
/* emul reg group size initialize method */
typedef int (*xen_pt_reg_size_init_fn)
- (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
+ (XenPCIPassthroughState *, XenPTRegGroupInfo *,
uint32_t base_offset, uint8_t *size);
/* emulated register group information */
@@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
/* emul register group management table */
typedef struct XenPTRegGroup {
QLIST_ENTRY(XenPTRegGroup) entries;
- const XenPTRegGroupInfo *reg_grp;
+ XenPTRegGroupInfo *reg_grp;
uint32_t base_offset;
uint8_t size;
QLIST_HEAD(, XenPTReg) reg_tbl_list;
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
}
static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
- const XenPTRegInfo *reg,
- uint32_t valid_mask)
+ XenPTRegInfo *reg, uint32_t valid_mask)
{
uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
[-- Attachment #2: qemu-pt-const.patch --]
[-- Type: text/plain, Size: 2025 bytes --]
xen/pass-through: constify some static data
This is done indirectly by adjusting two typedefs and helps emphasizing
that the respective tables aren't supposed to be modified at runtime
(as they may be shared between devices).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
/* Helper */
#define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
-typedef struct XenPTRegInfo XenPTRegInfo;
+typedef const struct XenPTRegInfo XenPTRegInfo;
typedef struct XenPTReg XenPTReg;
typedef struct XenPCIPassthroughState XenPCIPassthroughState;
@@ -135,11 +135,11 @@ struct XenPTReg {
uint32_t data; /* emulated value */
};
-typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
+typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
/* emul reg group size initialize method */
typedef int (*xen_pt_reg_size_init_fn)
- (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
+ (XenPCIPassthroughState *, XenPTRegGroupInfo *,
uint32_t base_offset, uint8_t *size);
/* emulated register group information */
@@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
/* emul register group management table */
typedef struct XenPTRegGroup {
QLIST_ENTRY(XenPTRegGroup) entries;
- const XenPTRegGroupInfo *reg_grp;
+ XenPTRegGroupInfo *reg_grp;
uint32_t base_offset;
uint8_t size;
QLIST_HEAD(, XenPTReg) reg_tbl_list;
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
}
static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
- const XenPTRegInfo *reg,
- uint32_t valid_mask)
+ XenPTRegInfo *reg, uint32_t valid_mask)
{
uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 6/6] xen/pass-through: constify some static data
@ 2015-06-05 12:04 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-05 12:04 UTC (permalink / raw)
To: qemu-devel; +Cc: xen-devel, Stefano Stabellini
[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]
This is done indirectly by adjusting two typedefs and helps emphasizing
that the respective tables aren't supposed to be modified at runtime
(as they may be shared between devices).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
/* Helper */
#define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
-typedef struct XenPTRegInfo XenPTRegInfo;
+typedef const struct XenPTRegInfo XenPTRegInfo;
typedef struct XenPTReg XenPTReg;
typedef struct XenPCIPassthroughState XenPCIPassthroughState;
@@ -135,11 +135,11 @@ struct XenPTReg {
uint32_t data; /* emulated value */
};
-typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
+typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
/* emul reg group size initialize method */
typedef int (*xen_pt_reg_size_init_fn)
- (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
+ (XenPCIPassthroughState *, XenPTRegGroupInfo *,
uint32_t base_offset, uint8_t *size);
/* emulated register group information */
@@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
/* emul register group management table */
typedef struct XenPTRegGroup {
QLIST_ENTRY(XenPTRegGroup) entries;
- const XenPTRegGroupInfo *reg_grp;
+ XenPTRegGroupInfo *reg_grp;
uint32_t base_offset;
uint8_t size;
QLIST_HEAD(, XenPTReg) reg_tbl_list;
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
}
static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
- const XenPTRegInfo *reg,
- uint32_t valid_mask)
+ XenPTRegInfo *reg, uint32_t valid_mask)
{
uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
[-- Attachment #2: qemu-pt-const.patch --]
[-- Type: text/plain, Size: 2025 bytes --]
xen/pass-through: constify some static data
This is done indirectly by adjusting two typedefs and helps emphasizing
that the respective tables aren't supposed to be modified at runtime
(as they may be shared between devices).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/qemu/upstream/hw/xen/xen_pt.h
+++ b/qemu/upstream/hw/xen/xen_pt.h
@@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
/* Helper */
#define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
-typedef struct XenPTRegInfo XenPTRegInfo;
+typedef const struct XenPTRegInfo XenPTRegInfo;
typedef struct XenPTReg XenPTReg;
typedef struct XenPCIPassthroughState XenPCIPassthroughState;
@@ -135,11 +135,11 @@ struct XenPTReg {
uint32_t data; /* emulated value */
};
-typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
+typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
/* emul reg group size initialize method */
typedef int (*xen_pt_reg_size_init_fn)
- (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
+ (XenPCIPassthroughState *, XenPTRegGroupInfo *,
uint32_t base_offset, uint8_t *size);
/* emulated register group information */
@@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
/* emul register group management table */
typedef struct XenPTRegGroup {
QLIST_ENTRY(XenPTRegGroup) entries;
- const XenPTRegGroupInfo *reg_grp;
+ XenPTRegGroupInfo *reg_grp;
uint32_t base_offset;
uint8_t size;
QLIST_HEAD(, XenPTReg) reg_tbl_list;
--- a/qemu/upstream/hw/xen/xen_pt_config_init.c
+++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
@@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
}
static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
- const XenPTRegInfo *reg,
- uint32_t valid_mask)
+ XenPTRegInfo *reg, uint32_t valid_mask)
{
uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-05 11:59 ` Jan Beulich
(?)
(?)
@ 2015-06-16 13:35 ` Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:04 ` Jan Beulich
-1 siblings, 2 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 13:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> The remaining log message in pci_msix_write() is wrong, as there guest
> behavior may only appear to be wrong: For one, the old logic didn't
> take the mask-all bit into account. And then this shouldn't depend on
> host device state (i.e. the host may have masked the entry without the
> guest having done so). Plus these writes shouldn't be dropped even when
> an entry gets unmasked. Instead, if they can't be made take effect
> right away, they should take effect on the next unmasking or enabling
> operation - the specification explicitly describes such caching
> behavior.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>
> pirq = entry->pirq;
>
> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
I admit I am having difficulties understanding the full purpose of these
checks. Please add a comment on them.
I guess the intention is only to make changes using the latest values,
the ones in entry->latch, when the right conditions are met, otherwise
keep using the old values. Is that right?
In that case, don't we want to use the latest values on MASKBIT ->
!MASKBIT transitions? In general when unmasking? Here it looks like we
are using the latest values when maskall is set or
PCI_MSIX_ENTRY_CTRL_MASKBIT is set, that is the opposite of what we
want.
> + entry->addr = entry->latch(LOWER_ADDR) |
> + ((uint64_t)entry->latch(UPPER_ADDR) << 32);
> + entry->data = entry->latch(DATA);
> + }
> +
> rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
> entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
> if (rc) {
> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
> offset = addr % PCI_MSIX_ENTRY_SIZE;
>
> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - const volatile uint32_t *vec_ctrl;
> -
> if (get_entry_value(entry, offset) == val
> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
> return;
> }
>
> + entry->updated = true;
> + } else if (msix->enabled && entry->updated &&
> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> + const volatile uint32_t *vec_ctrl;
> +
> /*
> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
> * up-to-date. Read from hardware directly.
> */
> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
> + PCI_MSIX_ENTRY_VECTOR_CTRL;
> + set_entry_value(entry, offset, *vec_ctrl);
Why are you calling set_entry_value with the hardware vec_ctrl value? It
doesn't look correct to me. In any case, if you wanted to do it,
shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
whole *vec_ctrl?
> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - if (!entry->warned) {
> - entry->warned = true;
> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
> - " already enabled.\n", entry_nr);
> - }
> - return;
> - }
> -
> - entry->updated = true;
> + xen_pt_msix_update_one(s, entry_nr);
Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
MASKBIT -> !MASKBIT transition?
> }
>
> set_entry_value(entry, offset, val);
> -
> - if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - xen_pt_msix_update_one(s, entry_nr);
> - }
> - }
> }
>
> static uint64_t pci_msix_read(void *opaque, hwaddr addr,
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-05 11:59 ` Jan Beulich
(?)
@ 2015-06-16 13:35 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 13:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> The remaining log message in pci_msix_write() is wrong, as there guest
> behavior may only appear to be wrong: For one, the old logic didn't
> take the mask-all bit into account. And then this shouldn't depend on
> host device state (i.e. the host may have masked the entry without the
> guest having done so). Plus these writes shouldn't be dropped even when
> an entry gets unmasked. Instead, if they can't be made take effect
> right away, they should take effect on the next unmasking or enabling
> operation - the specification explicitly describes such caching
> behavior.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>
> pirq = entry->pirq;
>
> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
I admit I am having difficulties understanding the full purpose of these
checks. Please add a comment on them.
I guess the intention is only to make changes using the latest values,
the ones in entry->latch, when the right conditions are met, otherwise
keep using the old values. Is that right?
In that case, don't we want to use the latest values on MASKBIT ->
!MASKBIT transitions? In general when unmasking? Here it looks like we
are using the latest values when maskall is set or
PCI_MSIX_ENTRY_CTRL_MASKBIT is set, that is the opposite of what we
want.
> + entry->addr = entry->latch(LOWER_ADDR) |
> + ((uint64_t)entry->latch(UPPER_ADDR) << 32);
> + entry->data = entry->latch(DATA);
> + }
> +
> rc = msi_msix_setup(s, entry->addr, entry->data, &pirq, true, entry_nr,
> entry->pirq == XEN_PT_UNASSIGNED_PIRQ);
> if (rc) {
> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
> offset = addr % PCI_MSIX_ENTRY_SIZE;
>
> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - const volatile uint32_t *vec_ctrl;
> -
> if (get_entry_value(entry, offset) == val
> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
> return;
> }
>
> + entry->updated = true;
> + } else if (msix->enabled && entry->updated &&
> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> + const volatile uint32_t *vec_ctrl;
> +
> /*
> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
> * up-to-date. Read from hardware directly.
> */
> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
> + PCI_MSIX_ENTRY_VECTOR_CTRL;
> + set_entry_value(entry, offset, *vec_ctrl);
Why are you calling set_entry_value with the hardware vec_ctrl value? It
doesn't look correct to me. In any case, if you wanted to do it,
shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
whole *vec_ctrl?
> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - if (!entry->warned) {
> - entry->warned = true;
> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
> - " already enabled.\n", entry_nr);
> - }
> - return;
> - }
> -
> - entry->updated = true;
> + xen_pt_msix_update_one(s, entry_nr);
Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
MASKBIT -> !MASKBIT transition?
> }
>
> set_entry_value(entry, offset, val);
> -
> - if (offset == PCI_MSIX_ENTRY_VECTOR_CTRL) {
> - if (msix->enabled && !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> - xen_pt_msix_update_one(s, entry_nr);
> - }
> - }
> }
>
> static uint64_t pci_msix_read(void *opaque, hwaddr addr,
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-05 12:01 ` Jan Beulich
(?)
(?)
@ 2015-06-16 14:03 ` Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:19 ` [Qemu-devel] " Jan Beulich
-1 siblings, 2 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> Particularly the maskall bit has to be under exclusive hypervisor
> control (and since they live in the same config space field, the
> enable bit has to follow suit). Use the replacement hypercall
> interfaces.
>From this commit message, I expect a patch that simply substitutes
maskall and enable writings with hypercalls and nothing else.
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
> typedef struct XenPTMSIX {
> uint32_t ctrl_offset;
> bool enabled;
> + bool maskall;
> int total_entries;
> int bar_index;
> uint64_t table_base;
> @@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
> void xen_pt_msix_delete(XenPCIPassthroughState *s);
> int xen_pt_msix_update(XenPCIPassthroughState *s);
> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
> +void xen_pt_msix_enable(XenPCIPassthroughState *s);
> void xen_pt_msix_disable(XenPCIPassthroughState *s);
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
>
> static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> {
> --- a/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
> uint16_t dev_value, uint16_t valid_mask)
> {
> XenPTRegInfo *reg = cfg_entry->reg;
> - uint16_t writable_mask = 0;
> + uint16_t writable_mask, value;
> uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> int debug_msix_enabled_old;
>
> /* modify emulate register */
> writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> + value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> + cfg_entry->data = value;
>
> /* create value for writing to I/O device register */
> *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
>
> + debug_msix_enabled_old = s->msix->enabled;
> +
> /* update MSI-X */
> - if ((*val & PCI_MSIX_FLAGS_ENABLE)
Please explain in the commit message the reason of the *val/value
change.
> - && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
> + if ((value & PCI_MSIX_FLAGS_ENABLE)
> + && !(value & PCI_MSIX_FLAGS_MASKALL)) {
> + if (!s->msix->enabled) {
> + if (!s->msix->maskall) {
> + xen_pt_msix_maskall(s, true);
> + }
> + xen_pt_msix_enable(s);
> + }
> xen_pt_msix_update(s);
> - } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
> - xen_pt_msix_disable(s);
> + s->msix->enabled = true;
> + s->msix->maskall = false;
> + xen_pt_msix_maskall(s, false);
Please explain in the commit message the reason behind the
maskall->enable->update->un_maskall logic, that wasn't present before.
> + } else if (s->msix->enabled) {
> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> + xen_pt_msix_disable(s);
> + s->msix->enabled = false;
> + } else if (!s->msix->maskall) {
Why are you changing the state of s->msix->maskall here?
This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
maskall, right?
> + s->msix->maskall = true;
> + xen_pt_msix_maskall(s, true);
> + }
> }
>
> - debug_msix_enabled_old = s->msix->enabled;
> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
> if (s->msix->enabled != debug_msix_enabled_old) {
> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
> s->msix->enabled ? "enable" : "disable");
> }
>
> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
I have to say that I don't like the asymmetry between reading and
writing PCI config registers. If writes go via hypercalls, reads should
go via hypercalls too.
> + if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
> + XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
> + } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
> + s->msix->maskall &&
> + !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
> + XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
> + }
> +
> return 0;
> }
>
> @@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
> .offset = PCI_MSI_FLAGS,
> .size = 2,
> .init_val = 0x0000,
> - .res_mask = 0x3800,
> - .ro_mask = 0x07FF,
> - .emu_mask = 0x0000,
> + /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
> + * because even in permissive mode there must not be any write back
> + * to this register.
> + */
> + .ro_mask = 0x3FFF,
> + .emu_mask = 0xC000,
> .init = xen_pt_msixctrl_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> .u.w.write = xen_pt_msixctrl_reg_write,
> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
> return -1;
> }
>
> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> - enabled);
Would it make sense to remove msi_msix_enable completely to avoid any
further mistakes?
> + return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
> + s->real_device.bus,
> + PCI_DEVFN(s->real_device.dev,
> + s->real_device.func),
> + enabled);
> }
>
> static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
> @@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
> return 0;
> }
>
> +void xen_pt_msix_enable(XenPCIPassthroughState *s)
> +{
> + msix_set_enable(s, true);
> +}
> +
> void xen_pt_msix_disable(XenPCIPassthroughState *s)
> {
> int i = 0;
> @@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
> }
> }
>
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
> +{
> + return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
> + s->real_device.bus,
> + PCI_DEVFN(s->real_device.dev,
> + s->real_device.func),
> + mask);
> +}
> +
> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
> {
> XenPTMSIXEntry *entry;
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-05 12:01 ` Jan Beulich
(?)
@ 2015-06-16 14:03 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> Particularly the maskall bit has to be under exclusive hypervisor
> control (and since they live in the same config space field, the
> enable bit has to follow suit). Use the replacement hypercall
> interfaces.
>From this commit message, I expect a patch that simply substitutes
maskall and enable writings with hypercalls and nothing else.
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -181,6 +181,7 @@ typedef struct XenPTMSIXEntry {
> typedef struct XenPTMSIX {
> uint32_t ctrl_offset;
> bool enabled;
> + bool maskall;
> int total_entries;
> int bar_index;
> uint64_t table_base;
> @@ -293,7 +294,9 @@ int xen_pt_msix_init(XenPCIPassthroughSt
> void xen_pt_msix_delete(XenPCIPassthroughState *s);
> int xen_pt_msix_update(XenPCIPassthroughState *s);
> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index);
> +void xen_pt_msix_enable(XenPCIPassthroughState *s);
> void xen_pt_msix_disable(XenPCIPassthroughState *s);
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask);
>
> static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> {
> --- a/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -1436,32 +1436,58 @@ static int xen_pt_msixctrl_reg_write(Xen
> uint16_t dev_value, uint16_t valid_mask)
> {
> XenPTRegInfo *reg = cfg_entry->reg;
> - uint16_t writable_mask = 0;
> + uint16_t writable_mask, value;
> uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> int debug_msix_enabled_old;
>
> /* modify emulate register */
> writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> + value = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> + cfg_entry->data = value;
>
> /* create value for writing to I/O device register */
> *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
>
> + debug_msix_enabled_old = s->msix->enabled;
> +
> /* update MSI-X */
> - if ((*val & PCI_MSIX_FLAGS_ENABLE)
Please explain in the commit message the reason of the *val/value
change.
> - && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
> + if ((value & PCI_MSIX_FLAGS_ENABLE)
> + && !(value & PCI_MSIX_FLAGS_MASKALL)) {
> + if (!s->msix->enabled) {
> + if (!s->msix->maskall) {
> + xen_pt_msix_maskall(s, true);
> + }
> + xen_pt_msix_enable(s);
> + }
> xen_pt_msix_update(s);
> - } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
> - xen_pt_msix_disable(s);
> + s->msix->enabled = true;
> + s->msix->maskall = false;
> + xen_pt_msix_maskall(s, false);
Please explain in the commit message the reason behind the
maskall->enable->update->un_maskall logic, that wasn't present before.
> + } else if (s->msix->enabled) {
> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> + xen_pt_msix_disable(s);
> + s->msix->enabled = false;
> + } else if (!s->msix->maskall) {
Why are you changing the state of s->msix->maskall here?
This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
maskall, right?
> + s->msix->maskall = true;
> + xen_pt_msix_maskall(s, true);
> + }
> }
>
> - debug_msix_enabled_old = s->msix->enabled;
> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
> if (s->msix->enabled != debug_msix_enabled_old) {
> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
> s->msix->enabled ? "enable" : "disable");
> }
>
> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
I have to say that I don't like the asymmetry between reading and
writing PCI config registers. If writes go via hypercalls, reads should
go via hypercalls too.
> + if (s->msix->enabled && !(dev_value & PCI_MSIX_FLAGS_ENABLE)) {
> + XEN_PT_ERR(&s->dev, "MSI-X unexpectedly disabled\n");
> + } else if ((dev_value & PCI_MSIX_FLAGS_ENABLE) &&
> + s->msix->maskall &&
> + !(dev_value & PCI_MSIX_FLAGS_MASKALL)) {
> + XEN_PT_ERR(&s->dev, "MSI-X unexpectedly unmasked\n");
> + }
> +
> return 0;
> }
>
> @@ -1483,9 +1509,12 @@ static XenPTRegInfo xen_pt_emu_reg_msix[
> .offset = PCI_MSI_FLAGS,
> .size = 2,
> .init_val = 0x0000,
> - .res_mask = 0x3800,
> - .ro_mask = 0x07FF,
> - .emu_mask = 0x0000,
> + /* This must not be split into res_mask (0x3800) and ro_mask (0x07FF)
> + * because even in permissive mode there must not be any write back
> + * to this register.
> + */
> + .ro_mask = 0x3FFF,
> + .emu_mask = 0xC000,
> .init = xen_pt_msixctrl_reg_init,
> .u.w.read = xen_pt_word_reg_read,
> .u.w.write = xen_pt_msixctrl_reg_write,
> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
> return -1;
> }
>
> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> - enabled);
Would it make sense to remove msi_msix_enable completely to avoid any
further mistakes?
> + return xc_physdev_msix_enable(xen_xc, s->real_device.domain,
> + s->real_device.bus,
> + PCI_DEVFN(s->real_device.dev,
> + s->real_device.func),
> + enabled);
> }
>
> static int xen_pt_msix_update_one(XenPCIPassthroughState *s, int entry_nr)
> @@ -361,6 +364,11 @@ int xen_pt_msix_update(XenPCIPassthrough
> return 0;
> }
>
> +void xen_pt_msix_enable(XenPCIPassthroughState *s)
> +{
> + msix_set_enable(s, true);
> +}
> +
> void xen_pt_msix_disable(XenPCIPassthroughState *s)
> {
> int i = 0;
> @@ -378,6 +386,15 @@ void xen_pt_msix_disable(XenPCIPassthrou
> }
> }
>
> +int xen_pt_msix_maskall(XenPCIPassthroughState *s, bool mask)
> +{
> + return xc_physdev_msix_mask_all(xen_xc, s->real_device.domain,
> + s->real_device.bus,
> + PCI_DEVFN(s->real_device.dev,
> + s->real_device.func),
> + mask);
> +}
> +
> int xen_pt_msix_update_remap(XenPCIPassthroughState *s, int bar_index)
> {
> XenPTMSIXEntry *entry;
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-16 13:35 ` [Qemu-devel] " Stefano Stabellini
@ 2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:48 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
1 sibling, 2 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:04 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 15:35, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>>
>> pirq = entry->pirq;
>>
>> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>
> I admit I am having difficulties understanding the full purpose of these
> checks. Please add a comment on them.
The comment would (pointlessly imo) re-state what the code already
says:
> I guess the intention is only to make changes using the latest values,
> the ones in entry->latch, when the right conditions are met, otherwise
> keep using the old values. Is that right?
>
> In that case, don't we want to use the latest values on MASKBIT ->
> !MASKBIT transitions? In general when unmasking?
This is what we want. And with that, the questions you ask further
down should be answered too: The function gets invoked with the
pre-change mask flag state in ->latch[], and updates the values
used for actually setting up when that one has the entry masked
(or mask-all is set). The actual new value gets written to ->latch[]
after the call.
>> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
>> offset = addr % PCI_MSIX_ENTRY_SIZE;
>>
>> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>> - const volatile uint32_t *vec_ctrl;
>> -
>> if (get_entry_value(entry, offset) == val
>> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>> return;
>> }
>>
>> + entry->updated = true;
>> + } else if (msix->enabled && entry->updated &&
>> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> + const volatile uint32_t *vec_ctrl;
>> +
>> /*
>> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
>> * up-to-date. Read from hardware directly.
>> */
>> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
>> + PCI_MSIX_ENTRY_VECTOR_CTRL;
>> + set_entry_value(entry, offset, *vec_ctrl);
>
> Why are you calling set_entry_value with the hardware vec_ctrl value? It
> doesn't look correct to me. In any case, if you wanted to do it,
> shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> whole *vec_ctrl?
The comment above the code explains it: What we have stored locally
may not reflect reality, as we may not have seen all writes (and this
indeed isn't just a "may"). And if out cached value isn't valid anymore,
why would we not want to update all of it, rather than just the mask
bit?
>> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> - if (!entry->warned) {
>> - entry->warned = true;
>> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
>> - " already enabled.\n", entry_nr);
>> - }
>> - return;
>> - }
>> -
>> - entry->updated = true;
>> + xen_pt_msix_update_one(s, entry_nr);
>
> Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> MASKBIT -> !MASKBIT transition?
The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check in the if() surrounding this call and the
(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check inside the function guarantee just that (i.e. the function
invocation is benign in the other case, as entry->addr/entry->data
would remain unchanged).
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-16 13:35 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
@ 2015-06-16 14:04 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:04 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 15:35, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
>>
>> pirq = entry->pirq;
>>
>> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
>> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>
> I admit I am having difficulties understanding the full purpose of these
> checks. Please add a comment on them.
The comment would (pointlessly imo) re-state what the code already
says:
> I guess the intention is only to make changes using the latest values,
> the ones in entry->latch, when the right conditions are met, otherwise
> keep using the old values. Is that right?
>
> In that case, don't we want to use the latest values on MASKBIT ->
> !MASKBIT transitions? In general when unmasking?
This is what we want. And with that, the questions you ask further
down should be answered too: The function gets invoked with the
pre-change mask flag state in ->latch[], and updates the values
used for actually setting up when that one has the entry masked
(or mask-all is set). The actual new value gets written to ->latch[]
after the call.
>> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
>> offset = addr % PCI_MSIX_ENTRY_SIZE;
>>
>> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>> - const volatile uint32_t *vec_ctrl;
>> -
>> if (get_entry_value(entry, offset) == val
>> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>> return;
>> }
>>
>> + entry->updated = true;
>> + } else if (msix->enabled && entry->updated &&
>> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> + const volatile uint32_t *vec_ctrl;
>> +
>> /*
>> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
>> * up-to-date. Read from hardware directly.
>> */
>> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
>> + PCI_MSIX_ENTRY_VECTOR_CTRL;
>> + set_entry_value(entry, offset, *vec_ctrl);
>
> Why are you calling set_entry_value with the hardware vec_ctrl value? It
> doesn't look correct to me. In any case, if you wanted to do it,
> shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> whole *vec_ctrl?
The comment above the code explains it: What we have stored locally
may not reflect reality, as we may not have seen all writes (and this
indeed isn't just a "may"). And if out cached value isn't valid anymore,
why would we not want to update all of it, rather than just the mask
bit?
>> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
>> - if (!entry->warned) {
>> - entry->warned = true;
>> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
>> - " already enabled.\n", entry_nr);
>> - }
>> - return;
>> - }
>> -
>> - entry->updated = true;
>> + xen_pt_msix_update_one(s, entry_nr);
>
> Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> MASKBIT -> !MASKBIT transition?
The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check in the if() surrounding this call and the
(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
check inside the function guarantee just that (i.e. the function
invocation is benign in the other case, as entry->addr/entry->data
would remain unchanged).
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment
2015-06-05 12:02 ` Jan Beulich
(?)
(?)
@ 2015-06-16 14:08 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> 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/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -421,16 +421,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,
> @@ -496,6 +494,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,
> @@ -504,7 +508,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] 41+ messages in thread
* Re: [PATCH 3/6] xen/MSI-X: really enforce alignment
2015-06-05 12:02 ` Jan Beulich
(?)
@ 2015-06-16 14:08 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> 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/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -421,16 +421,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,
> @@ -496,6 +494,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,
> @@ -504,7 +508,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] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
2015-06-05 12:03 ` Jan Beulich
(?)
(?)
@ 2015-06-16 14:19 ` Stefano Stabellini
2015-06-16 14:38 ` Jan Beulich
2015-06-16 14:38 ` Jan Beulich
-1 siblings, 2 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 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>
>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -611,6 +614,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,
> @@ -910,6 +914,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,
> @@ -930,6 +935,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,
> @@ -966,26 +972,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);
> -
> - /* modify emulate register */
> - writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
> @@ -1016,11 +1002,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,
I can see that the code change doesn't cause a change in behaviour for
PCI_PM_CTRL, but it does for PCI_STATUS, PCI_EXP_DEVSTA and
PCI_EXP_LNKSTA. Please explain why in the commit message.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
2015-06-05 12:03 ` Jan Beulich
(?)
@ 2015-06-16 14:19 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:19 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 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>
>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -105,6 +105,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/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -176,7 +176,8 @@ static int xen_pt_byte_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -193,7 +194,8 @@ static int xen_pt_word_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -210,7 +212,8 @@ static int xen_pt_long_reg_write(XenPCIP
> cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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;
> }
> @@ -611,6 +614,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,
> @@ -910,6 +914,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,
> @@ -930,6 +935,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,
> @@ -966,26 +972,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);
> -
> - /* modify emulate register */
> - writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> - cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->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 */
> @@ -1016,11 +1002,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,
I can see that the code change doesn't cause a change in behaviour for
PCI_PM_CTRL, but it does for PCI_STATUS, PCI_EXP_DEVSTA and
PCI_EXP_LNKSTA. Please explain why in the commit message.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-16 14:03 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
@ 2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 14:56 ` Stefano Stabellini
1 sibling, 2 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:19 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> + } else if (s->msix->enabled) {
>> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
>> + xen_pt_msix_disable(s);
>> + s->msix->enabled = false;
>> + } else if (!s->msix->maskall) {
>
> Why are you changing the state of s->msix->maskall here?
> This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> maskall, right?
We're at an else if inside an else if here. The only case left
after the if() still seen above is that value has
PCI_MSIX_FLAGS_MASKALL set.
>> + s->msix->maskall = true;
>> + xen_pt_msix_maskall(s, true);
>> + }
>> }
>>
>> - debug_msix_enabled_old = s->msix->enabled;
>> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
>> if (s->msix->enabled != debug_msix_enabled_old) {
>> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
>> s->msix->enabled ? "enable" : "disable");
>> }
>>
>> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
>
> I have to say that I don't like the asymmetry between reading and
> writing PCI config registers. If writes go via hypercalls, reads should
> go via hypercalls too.
We're not doing any cfg register write via hypercalls (not here,
and not elsewhere). What is being replaced by the patch are
write to two bits which happen to live in PCI config space. Plus,
reading directly, and doing writes via hypercall only when really
needed would still be the right thing from a performance pov.
>> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
>> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
>> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>> return -1;
>> }
>>
>> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
>> - enabled);
>
> Would it make sense to remove msi_msix_enable completely to avoid any
> further mistakes?
Perhaps, yes. I think I actually had suggested so quite a while back.
But I don't see myself wasting much more time on this, ehm, code.
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-16 14:03 ` [Qemu-devel] " Stefano Stabellini
@ 2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:19 ` [Qemu-devel] " Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:19 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> + } else if (s->msix->enabled) {
>> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
>> + xen_pt_msix_disable(s);
>> + s->msix->enabled = false;
>> + } else if (!s->msix->maskall) {
>
> Why are you changing the state of s->msix->maskall here?
> This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> maskall, right?
We're at an else if inside an else if here. The only case left
after the if() still seen above is that value has
PCI_MSIX_FLAGS_MASKALL set.
>> + s->msix->maskall = true;
>> + xen_pt_msix_maskall(s, true);
>> + }
>> }
>>
>> - debug_msix_enabled_old = s->msix->enabled;
>> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
>> if (s->msix->enabled != debug_msix_enabled_old) {
>> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
>> s->msix->enabled ? "enable" : "disable");
>> }
>>
>> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
>
> I have to say that I don't like the asymmetry between reading and
> writing PCI config registers. If writes go via hypercalls, reads should
> go via hypercalls too.
We're not doing any cfg register write via hypercalls (not here,
and not elsewhere). What is being replaced by the patch are
write to two bits which happen to live in PCI config space. Plus,
reading directly, and doing writes via hypercall only when really
needed would still be the right thing from a performance pov.
>> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
>> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
>> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>> return -1;
>> }
>>
>> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
>> - enabled);
>
> Would it make sense to remove msi_msix_enable completely to avoid any
> further mistakes?
Perhaps, yes. I think I actually had suggested so quite a while back.
But I don't see myself wasting much more time on this, ehm, code.
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones
2015-06-05 12:04 ` Jan Beulich
(?)
(?)
@ 2015-06-16 14:23 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> Functions setting errno commonly return just -1, which is of no
> particular use in the log file.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> --- a/qemu/upstream/hw/xen/xen_pt.c
> +++ b/qemu/upstream/hw/xen/xen_pt.c
> @@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
> guest_port, machine_port, size,
> op);
> if (rc) {
> - XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
> - adding ? "create new" : "remove old", rc);
> + XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
> + adding ? "create new" : "remove old", errno);
> }
> } else {
> pcibus_t guest_addr = sec->offset_within_address_space;
> @@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
> XEN_PFN(size + XC_PAGE_SIZE - 1),
> op);
> if (rc) {
> - XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
> - adding ? "create new" : "remove old", rc);
> + XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
> + adding ? "create new" : "remove old", errno);
> }
> }
> }
> @@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
> rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
>
> if (rc < 0) {
> - XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
> - machine_irq, pirq, rc);
> + XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
> + machine_irq, pirq, errno);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
> PCI_SLOT(d->devfn),
> e_intx);
> if (rc < 0) {
> - XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
> - e_intx, rc);
> + XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
> + e_intx, errno);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
> if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
> if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
> XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
> - " (rc: %d)\n", machine_irq, rc);
> + " (err: %d)\n", machine_irq, errno);
> }
> }
> s->machine_irq = 0;
> @@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
> 0 /* isa_irq */);
> if (rc < 0) {
> XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
> - " (machine irq: %i, rc: %d)"
> + " (machine irq: %i, err: %d)"
> " But bravely continuing on..\n",
> - 'a' + intx, machine_irq, rc);
> + 'a' + intx, machine_irq, errno);
> }
> }
>
> @@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
> rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
>
> if (rc < 0) {
> - XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
> + XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
> " But bravely continuing on..\n",
> - machine_irq, rc);
> + machine_irq, errno);
> }
> }
> }
> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
> msix_entry, table_base);
> if (rc) {
> XEN_PT_ERR(&s->dev,
> - "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
> - is_msix ? "-X" : "", rc, gvec, msix_entry);
> + "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
> + is_msix ? "-X" : "", errno, gvec, msix_entry);
> return rc;
> }
> }
> @@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
> pirq, gflags, table_addr);
>
> if (rc) {
> - XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
> - is_msix ? "-X" : "", rc);
> + XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
> + is_msix ? "-X" : "", errno);
>
> if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
> - XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
> - is_msix ? "-X" : "", *old_pirq);
> + XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
> + is_msix ? "-X" : "", *old_pirq, errno);
> }
> *old_pirq = XEN_PT_UNASSIGNED_PIRQ;
> }
> @@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
> is_msix ? "-X" : "", pirq, gvec);
> rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> if (rc) {
> - XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
> - is_msix ? "-X" : "", pirq, gvec);
> + XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> + is_msix ? "-X" : "", errno, pirq, gvec);
> return rc;
> }
> }
> @@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
> XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
> rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
> if (rc) {
> - XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
> - is_msix ? "-X" : "", pirq, rc);
> + XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
> + is_msix ? "-X" : "", pirq, errno);
> return rc;
> }
>
> @@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
> ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
> PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
> if (ret) {
> - XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
> - entry->pirq);
> + XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
> + entry->pirq, errno);
> }
> entry->updated = true;
> }
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 5/6] xen/pass-through: log errno values rather than function return ones
2015-06-05 12:04 ` Jan Beulich
(?)
@ 2015-06-16 14:23 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> Functions setting errno commonly return just -1, which is of no
> particular use in the log file.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> --- a/qemu/upstream/hw/xen/xen_pt.c
> +++ b/qemu/upstream/hw/xen/xen_pt.c
> @@ -609,8 +609,8 @@ static void xen_pt_region_update(XenPCIP
> guest_port, machine_port, size,
> op);
> if (rc) {
> - XEN_PT_ERR(d, "%s ioport mapping failed! (rc: %i)\n",
> - adding ? "create new" : "remove old", rc);
> + XEN_PT_ERR(d, "%s ioport mapping failed! (err: %i)\n",
> + adding ? "create new" : "remove old", errno);
> }
> } else {
> pcibus_t guest_addr = sec->offset_within_address_space;
> @@ -623,8 +623,8 @@ static void xen_pt_region_update(XenPCIP
> XEN_PFN(size + XC_PAGE_SIZE - 1),
> op);
> if (rc) {
> - XEN_PT_ERR(d, "%s mem mapping failed! (rc: %i)\n",
> - adding ? "create new" : "remove old", rc);
> + XEN_PT_ERR(d, "%s mem mapping failed! (err: %i)\n",
> + adding ? "create new" : "remove old", errno);
> }
> }
> }
> @@ -738,8 +738,8 @@ static int xen_pt_initfn(PCIDevice *d)
> rc = xc_physdev_map_pirq(xen_xc, xen_domid, machine_irq, &pirq);
>
> if (rc < 0) {
> - XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (rc: %d)\n",
> - machine_irq, pirq, rc);
> + XEN_PT_ERR(d, "Mapping machine irq %u to pirq %i failed, (err: %d)\n",
> + machine_irq, pirq, errno);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -760,8 +760,8 @@ static int xen_pt_initfn(PCIDevice *d)
> PCI_SLOT(d->devfn),
> e_intx);
> if (rc < 0) {
> - XEN_PT_ERR(d, "Binding of interrupt %i failed! (rc: %d)\n",
> - e_intx, rc);
> + XEN_PT_ERR(d, "Binding of interrupt %i failed! (err: %d)\n",
> + e_intx, errno);
>
> /* Disable PCI intx assertion (turn on bit10 of devctl) */
> cmd |= PCI_COMMAND_INTX_DISABLE;
> @@ -770,7 +770,7 @@ static int xen_pt_initfn(PCIDevice *d)
> if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
> if (xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq)) {
> XEN_PT_ERR(d, "Unmapping of machine interrupt %i failed!"
> - " (rc: %d)\n", machine_irq, rc);
> + " (err: %d)\n", machine_irq, errno);
> }
> }
> s->machine_irq = 0;
> @@ -808,9 +808,9 @@ static void xen_pt_unregister_device(PCI
> 0 /* isa_irq */);
> if (rc < 0) {
> XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
> - " (machine irq: %i, rc: %d)"
> + " (machine irq: %i, err: %d)"
> " But bravely continuing on..\n",
> - 'a' + intx, machine_irq, rc);
> + 'a' + intx, machine_irq, errno);
> }
> }
>
> @@ -828,9 +828,9 @@ static void xen_pt_unregister_device(PCI
> rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
>
> if (rc < 0) {
> - XEN_PT_ERR(d, "unmapping of interrupt %i failed. (rc: %d)"
> + XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
> " But bravely continuing on..\n",
> - machine_irq, rc);
> + machine_irq, errno);
> }
> }
> }
> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> @@ -133,8 +133,8 @@ static int msi_msix_setup(XenPCIPassthro
> msix_entry, table_base);
> if (rc) {
> XEN_PT_ERR(&s->dev,
> - "Mapping of MSI%s (rc: %i, vec: %#x, entry %#x)\n",
> - is_msix ? "-X" : "", rc, gvec, msix_entry);
> + "Mapping of MSI%s (err: %i, vec: %#x, entry %#x)\n",
> + is_msix ? "-X" : "", errno, gvec, msix_entry);
> return rc;
> }
> }
> @@ -167,12 +167,12 @@ static int msi_msix_update(XenPCIPassthr
> pirq, gflags, table_addr);
>
> if (rc) {
> - XEN_PT_ERR(d, "Updating of MSI%s failed. (rc: %d)\n",
> - is_msix ? "-X" : "", rc);
> + XEN_PT_ERR(d, "Updating of MSI%s failed. (err: %d)\n",
> + is_msix ? "-X" : "", errno);
>
> if (xc_physdev_unmap_pirq(xen_xc, xen_domid, *old_pirq)) {
> - XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed.\n",
> - is_msix ? "-X" : "", *old_pirq);
> + XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %d)\n",
> + is_msix ? "-X" : "", *old_pirq, errno);
> }
> *old_pirq = XEN_PT_UNASSIGNED_PIRQ;
> }
> @@ -200,8 +200,8 @@ static int msi_msix_disable(XenPCIPassth
> is_msix ? "-X" : "", pirq, gvec);
> rc = xc_domain_unbind_msi_irq(xen_xc, xen_domid, gvec, pirq, gflags);
> if (rc) {
> - XEN_PT_ERR(d, "Unbinding of MSI%s failed. (pirq: %d, gvec: %#x)\n",
> - is_msix ? "-X" : "", pirq, gvec);
> + XEN_PT_ERR(d, "Unbinding of MSI%s failed. (err: %d, pirq: %d, gvec: %#x)\n",
> + is_msix ? "-X" : "", errno, pirq, gvec);
> return rc;
> }
> }
> @@ -209,8 +209,8 @@ static int msi_msix_disable(XenPCIPassth
> XEN_PT_LOG(d, "Unmap MSI%s pirq %d\n", is_msix ? "-X" : "", pirq);
> rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, pirq);
> if (rc) {
> - XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (rc: %i)\n",
> - is_msix ? "-X" : "", pirq, rc);
> + XEN_PT_ERR(d, "Unmapping of MSI%s pirq %d failed. (err: %i)\n",
> + is_msix ? "-X" : "", pirq, errno);
> return rc;
> }
>
> @@ -410,8 +410,8 @@ int xen_pt_msix_update_remap(XenPCIPasst
> ret = xc_domain_unbind_pt_irq(xen_xc, xen_domid, entry->pirq,
> PT_IRQ_TYPE_MSI, 0, 0, 0, 0);
> if (ret) {
> - XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed\n",
> - entry->pirq);
> + XEN_PT_ERR(&s->dev, "unbind MSI-X entry %d failed (err: %d)\n",
> + entry->pirq, errno);
> }
> entry->updated = true;
> }
>
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data
2015-06-05 12:04 ` Jan Beulich
(?)
(?)
@ 2015-06-16 14:27 ` Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:41 ` Jan Beulich
-1 siblings, 2 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> This is done indirectly by adjusting two typedefs and helps emphasizing
> that the respective tables aren't supposed to be modified at runtime
> (as they may be shared between devices).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
> /* Helper */
> #define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
>
> -typedef struct XenPTRegInfo XenPTRegInfo;
> +typedef const struct XenPTRegInfo XenPTRegInfo;
> typedef struct XenPTReg XenPTReg;
>
> typedef struct XenPCIPassthroughState XenPCIPassthroughState;
> @@ -135,11 +135,11 @@ struct XenPTReg {
> uint32_t data; /* emulated value */
> };
>
> -typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
> +typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
>
> /* emul reg group size initialize method */
> typedef int (*xen_pt_reg_size_init_fn)
> - (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
> + (XenPCIPassthroughState *, XenPTRegGroupInfo *,
> uint32_t base_offset, uint8_t *size);
>
> /* emulated register group information */
> @@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
> /* emul register group management table */
> typedef struct XenPTRegGroup {
> QLIST_ENTRY(XenPTRegGroup) entries;
> - const XenPTRegGroupInfo *reg_grp;
> + XenPTRegGroupInfo *reg_grp;
> uint32_t base_offset;
> uint8_t size;
> QLIST_HEAD(, XenPTReg) reg_tbl_list;
> --- a/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
> }
>
> static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
> - const XenPTRegInfo *reg,
> - uint32_t valid_mask)
> + XenPTRegInfo *reg, uint32_t valid_mask)
> {
> uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 6/6] xen/pass-through: constify some static data
2015-06-05 12:04 ` Jan Beulich
(?)
@ 2015-06-16 14:27 ` Stefano Stabellini
-1 siblings, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Fri, 5 Jun 2015, Jan Beulich wrote:
> This is done indirectly by adjusting two typedefs and helps emphasizing
> that the respective tables aren't supposed to be modified at runtime
> (as they may be shared between devices).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> --- a/qemu/upstream/hw/xen/xen_pt.h
> +++ b/qemu/upstream/hw/xen/xen_pt.h
> @@ -31,7 +31,7 @@ void xen_pt_log(const PCIDevice *d, cons
> /* Helper */
> #define XEN_PFN(x) ((x) >> XC_PAGE_SHIFT)
>
> -typedef struct XenPTRegInfo XenPTRegInfo;
> +typedef const struct XenPTRegInfo XenPTRegInfo;
> typedef struct XenPTReg XenPTReg;
>
> typedef struct XenPCIPassthroughState XenPCIPassthroughState;
> @@ -135,11 +135,11 @@ struct XenPTReg {
> uint32_t data; /* emulated value */
> };
>
> -typedef struct XenPTRegGroupInfo XenPTRegGroupInfo;
> +typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
>
> /* emul reg group size initialize method */
> typedef int (*xen_pt_reg_size_init_fn)
> - (XenPCIPassthroughState *, const XenPTRegGroupInfo *,
> + (XenPCIPassthroughState *, XenPTRegGroupInfo *,
> uint32_t base_offset, uint8_t *size);
>
> /* emulated register group information */
> @@ -154,7 +154,7 @@ struct XenPTRegGroupInfo {
> /* emul register group management table */
> typedef struct XenPTRegGroup {
> QLIST_ENTRY(XenPTRegGroup) entries;
> - const XenPTRegGroupInfo *reg_grp;
> + XenPTRegGroupInfo *reg_grp;
> uint32_t base_offset;
> uint8_t size;
> QLIST_HEAD(, XenPTReg) reg_tbl_list;
> --- a/qemu/upstream/hw/xen/xen_pt_config_init.c
> +++ b/qemu/upstream/hw/xen/xen_pt_config_init.c
> @@ -96,8 +96,7 @@ XenPTReg *xen_pt_find_reg(XenPTRegGroup
> }
>
> static uint32_t get_throughable_mask(const XenPCIPassthroughState *s,
> - const XenPTRegInfo *reg,
> - uint32_t valid_mask)
> + XenPTRegInfo *reg, uint32_t valid_mask)
> {
> uint32_t throughable_mask = ~(reg->emu_mask | reg->ro_mask);
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
2015-06-16 14:19 ` [Qemu-devel] " Stefano Stabellini
@ 2015-06-16 14:38 ` Jan Beulich
2015-06-16 14:38 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:38 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:19, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -1016,11 +1002,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,
>
> I can see that the code change doesn't cause a change in behaviour for
> PCI_PM_CTRL, but it does for PCI_STATUS, PCI_EXP_DEVSTA and
> PCI_EXP_LNKSTA. Please explain why in the commit message.
I'm not sure what you're after in a patch titled "correctly deal with
RW1C bits".
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits
2015-06-16 14:19 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:38 ` Jan Beulich
@ 2015-06-16 14:38 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:38 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:19, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> @@ -1016,11 +1002,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,
>
> I can see that the code change doesn't cause a change in behaviour for
> PCI_PM_CTRL, but it does for PCI_STATUS, PCI_EXP_DEVSTA and
> PCI_EXP_LNKSTA. Please explain why in the commit message.
I'm not sure what you're after in a patch titled "correctly deal with
RW1C bits".
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data
2015-06-16 14:27 ` [Qemu-devel] " Stefano Stabellini
@ 2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:43 ` Stefano Stabellini
2015-06-16 14:43 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
1 sibling, 2 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:27, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> This is done indirectly by adjusting two typedefs and helps emphasizing
>> that the respective tables aren't supposed to be modified at runtime
>> (as they may be shared between devices).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Perhaps patch 5 and 6 could go in right away, without waiting
for the issues on the other ones addressed? After all, it may
well be that patch 1 will be dropped following the discussion on
the hypervisor side patch series...
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 6/6] xen/pass-through: constify some static data
2015-06-16 14:27 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
@ 2015-06-16 14:41 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 14:41 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:27, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 5 Jun 2015, Jan Beulich wrote:
>> This is done indirectly by adjusting two typedefs and helps emphasizing
>> that the respective tables aren't supposed to be modified at runtime
>> (as they may be shared between devices).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Perhaps patch 5 and 6 could go in right away, without waiting
for the issues on the other ones addressed? After all, it may
well be that patch 1 will be dropped following the discussion on
the hypervisor side patch series...
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data
2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:43 ` Stefano Stabellini
@ 2015-06-16 14:43 ` Stefano Stabellini
1 sibling, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:27, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> This is done indirectly by adjusting two typedefs and helps emphasizing
> >> that the respective tables aren't supposed to be modified at runtime
> >> (as they may be shared between devices).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Perhaps patch 5 and 6 could go in right away, without waiting
> for the issues on the other ones addressed? After all, it may
> well be that patch 1 will be dropped following the discussion on
> the hypervisor side patch series...
Yes, I'll queue them up.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 6/6] xen/pass-through: constify some static data
2015-06-16 14:41 ` Jan Beulich
@ 2015-06-16 14:43 ` Stefano Stabellini
2015-06-16 14:43 ` [Qemu-devel] " Stefano Stabellini
1 sibling, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:27, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> This is done indirectly by adjusting two typedefs and helps emphasizing
> >> that the respective tables aren't supposed to be modified at runtime
> >> (as they may be shared between devices).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Perhaps patch 5 and 6 could go in right away, without waiting
> for the issues on the other ones addressed? After all, it may
> well be that patch 1 will be dropped following the discussion on
> the hypervisor side patch series...
Yes, I'll queue them up.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:48 ` Stefano Stabellini
@ 2015-06-16 14:48 ` Stefano Stabellini
1 sibling, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 15:35, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
> >>
> >> pirq = entry->pirq;
> >>
> >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
> >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >
> > I admit I am having difficulties understanding the full purpose of these
> > checks. Please add a comment on them.
>
> The comment would (pointlessly imo) re-state what the code already
> says:
>
> > I guess the intention is only to make changes using the latest values,
> > the ones in entry->latch, when the right conditions are met, otherwise
> > keep using the old values. Is that right?
> >
> > In that case, don't we want to use the latest values on MASKBIT ->
> > !MASKBIT transitions? In general when unmasking?
>
> This is what we want. And with that, the questions you ask further
> down should be answered too: The function gets invoked with the
> pre-change mask flag state in ->latch[], and updates the values
> used for actually setting up when that one has the entry masked
> (or mask-all is set). The actual new value gets written to ->latch[]
> after the call.
I think this logic is counter-intuitive and prone to confuse the reader.
This change doesn't make sense on its own: when one will read
xen_pt_msix_update_one, won't be able to understand the function without
checking the call sites.
Could we turn it around to be more obvious? Here check if
!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only
call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions?
Or something like that?
> >> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
> >> offset = addr % PCI_MSIX_ENTRY_SIZE;
> >>
> >> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
> >> - const volatile uint32_t *vec_ctrl;
> >> -
> >> if (get_entry_value(entry, offset) == val
> >> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
> >> return;
> >> }
> >>
> >> + entry->updated = true;
> >> + } else if (msix->enabled && entry->updated &&
> >> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >> + const volatile uint32_t *vec_ctrl;
> >> +
> >> /*
> >> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
> >> * up-to-date. Read from hardware directly.
> >> */
> >> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
> >> + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >> + set_entry_value(entry, offset, *vec_ctrl);
> >
> > Why are you calling set_entry_value with the hardware vec_ctrl value? It
> > doesn't look correct to me. In any case, if you wanted to do it,
> > shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> > whole *vec_ctrl?
>
> The comment above the code explains it: What we have stored locally
> may not reflect reality, as we may not have seen all writes (and this
> indeed isn't just a "may"). And if out cached value isn't valid anymore,
> why would we not want to update all of it, rather than just the mask
> bit?
OK, however the previous code wasn't actually updating the entirety of
vector_ctrl. It was just using the updated value to check for
PCI_MSIX_ENTRY_CTRL_MASKBIT. This is something else. The new behavior
might be correct, but at least the commit message needs to explain it.
> >> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >> - if (!entry->warned) {
> >> - entry->warned = true;
> >> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
> >> - " already enabled.\n", entry_nr);
> >> - }
> >> - return;
> >> - }
> >> -
> >> - entry->updated = true;
> >> + xen_pt_msix_update_one(s, entry_nr);
> >
> > Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> > PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> > MASKBIT -> !MASKBIT transition?
>
> The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> check in the if() surrounding this call and the
> (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> check inside the function guarantee just that (i.e. the function
> invocation is benign in the other case, as entry->addr/entry->data
> would remain unchanged).
OK, maybe the code works as is, but it took me a long time to make sense
of it because it relies on the combinations of three checks in three
different places. I would prefer to change it into something more
obvious.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
2015-06-16 14:04 ` Jan Beulich
@ 2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:48 ` [Qemu-devel] " Stefano Stabellini
1 sibling, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 15:35, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> @@ -322,6 +323,13 @@ static int xen_pt_msix_update_one(XenPCI
> >>
> >> pirq = entry->pirq;
> >>
> >> + if (pirq == XEN_PT_UNASSIGNED_PIRQ || s->msix->maskall ||
> >> + (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >
> > I admit I am having difficulties understanding the full purpose of these
> > checks. Please add a comment on them.
>
> The comment would (pointlessly imo) re-state what the code already
> says:
>
> > I guess the intention is only to make changes using the latest values,
> > the ones in entry->latch, when the right conditions are met, otherwise
> > keep using the old values. Is that right?
> >
> > In that case, don't we want to use the latest values on MASKBIT ->
> > !MASKBIT transitions? In general when unmasking?
>
> This is what we want. And with that, the questions you ask further
> down should be answered too: The function gets invoked with the
> pre-change mask flag state in ->latch[], and updates the values
> used for actually setting up when that one has the entry masked
> (or mask-all is set). The actual new value gets written to ->latch[]
> after the call.
I think this logic is counter-intuitive and prone to confuse the reader.
This change doesn't make sense on its own: when one will read
xen_pt_msix_update_one, won't be able to understand the function without
checking the call sites.
Could we turn it around to be more obvious? Here check if
!(entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT) and below only
call xen_pt_msix_update_one on MASKBIT -> !MASKBIT transactions?
Or something like that?
> >> @@ -444,39 +432,28 @@ static void pci_msix_write(void *opaque,
> >> offset = addr % PCI_MSIX_ENTRY_SIZE;
> >>
> >> if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
> >> - const volatile uint32_t *vec_ctrl;
> >> -
> >> if (get_entry_value(entry, offset) == val
> >> && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
> >> return;
> >> }
> >>
> >> + entry->updated = true;
> >> + } else if (msix->enabled && entry->updated &&
> >> + !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >> + const volatile uint32_t *vec_ctrl;
> >> +
> >> /*
> >> * If Xen intercepts the mask bit access, entry->vec_ctrl may not be
> >> * up-to-date. Read from hardware directly.
> >> */
> >> vec_ctrl = s->msix->phys_iomem_base + entry_nr * PCI_MSIX_ENTRY_SIZE
> >> + PCI_MSIX_ENTRY_VECTOR_CTRL;
> >> + set_entry_value(entry, offset, *vec_ctrl);
> >
> > Why are you calling set_entry_value with the hardware vec_ctrl value? It
> > doesn't look correct to me. In any case, if you wanted to do it,
> > shouldn't you just set/unset PCI_MSIX_ENTRY_CTRL_MASKBIT instead of the
> > whole *vec_ctrl?
>
> The comment above the code explains it: What we have stored locally
> may not reflect reality, as we may not have seen all writes (and this
> indeed isn't just a "may"). And if out cached value isn't valid anymore,
> why would we not want to update all of it, rather than just the mask
> bit?
OK, however the previous code wasn't actually updating the entirety of
vector_ctrl. It was just using the updated value to check for
PCI_MSIX_ENTRY_CTRL_MASKBIT. This is something else. The new behavior
might be correct, but at least the commit message needs to explain it.
> >> - if (msix->enabled && !(*vec_ctrl & PCI_MSIX_ENTRY_CTRL_MASKBIT)) {
> >> - if (!entry->warned) {
> >> - entry->warned = true;
> >> - XEN_PT_ERR(&s->dev, "Can't update msix entry %d since MSI-X is"
> >> - " already enabled.\n", entry_nr);
> >> - }
> >> - return;
> >> - }
> >> -
> >> - entry->updated = true;
> >> + xen_pt_msix_update_one(s, entry_nr);
> >
> > Shouldn't we call xen_pt_msix_update_one only if (*vec_ctrl &
> > PCI_MSIX_ENTRY_CTRL_MASKBIT)? In other words, only when we see a
> > MASKBIT -> !MASKBIT transition?
>
> The combination of the !(val & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> check in the if() surrounding this call and the
> (entry->latch(VECTOR_CTRL) & PCI_MSIX_ENTRY_CTRL_MASKBIT)
> check inside the function guarantee just that (i.e. the function
> invocation is benign in the other case, as entry->addr/entry->data
> would remain unchanged).
OK, maybe the code works as is, but it took me a long time to make sense
of it because it relies on the combinations of three checks in three
different places. I would prefer to change it into something more
obvious.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-16 14:19 ` [Qemu-devel] " Jan Beulich
@ 2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 16:03 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
1 sibling, 1 reply; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> + } else if (s->msix->enabled) {
> >> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> >> + xen_pt_msix_disable(s);
> >> + s->msix->enabled = false;
> >> + } else if (!s->msix->maskall) {
> >
> > Why are you changing the state of s->msix->maskall here?
> > This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> > maskall, right?
>
> We're at an else if inside an else if here. The only case left
> after the if() still seen above is that value has
> PCI_MSIX_FLAGS_MASKALL set.
You are right.
Maybe just add
/* (value & PCI_MSIX_FLAGS_MASKALL) at this point */
> >> + s->msix->maskall = true;
> >> + xen_pt_msix_maskall(s, true);
> >> + }
> >> }
> >>
> >> - debug_msix_enabled_old = s->msix->enabled;
> >> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
> >> if (s->msix->enabled != debug_msix_enabled_old) {
> >> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
> >> s->msix->enabled ? "enable" : "disable");
> >> }
> >>
> >> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
> >
> > I have to say that I don't like the asymmetry between reading and
> > writing PCI config registers. If writes go via hypercalls, reads should
> > go via hypercalls too.
>
> We're not doing any cfg register write via hypercalls (not here,
> and not elsewhere). What is being replaced by the patch are
> write to two bits which happen to live in PCI config space. Plus,
> reading directly, and doing writes via hypercall only when really
> needed would still be the right thing from a performance pov.
OK. It would be nice to document somewhere that QEMU is not supposed to
touch those two bits directly, but I wouldn't know where. Maybe a new
doc under docs/misc?
> >> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> >> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> >> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
> >> return -1;
> >> }
> >>
> >> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> >> - enabled);
> >
> > Would it make sense to remove msi_msix_enable completely to avoid any
> > further mistakes?
>
> Perhaps, yes. I think I actually had suggested so quite a while back.
> But I don't see myself wasting much more time on this, ehm, code.
Isn't it just a matter of removing msi_msix_enable?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-16 14:19 ` [Qemu-devel] " Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
@ 2015-06-16 14:56 ` Stefano Stabellini
1 sibling, 0 replies; 41+ messages in thread
From: Stefano Stabellini @ 2015-06-16 14:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, qemu-devel, Stefano Stabellini
On Tue, 16 Jun 2015, Jan Beulich wrote:
> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 5 Jun 2015, Jan Beulich wrote:
> >> + } else if (s->msix->enabled) {
> >> + if (!(value & PCI_MSIX_FLAGS_ENABLE)) {
> >> + xen_pt_msix_disable(s);
> >> + s->msix->enabled = false;
> >> + } else if (!s->msix->maskall) {
> >
> > Why are you changing the state of s->msix->maskall here?
> > This is the value & PCI_MSIX_FLAGS_ENABLE case, nothing to do with
> > maskall, right?
>
> We're at an else if inside an else if here. The only case left
> after the if() still seen above is that value has
> PCI_MSIX_FLAGS_MASKALL set.
You are right.
Maybe just add
/* (value & PCI_MSIX_FLAGS_MASKALL) at this point */
> >> + s->msix->maskall = true;
> >> + xen_pt_msix_maskall(s, true);
> >> + }
> >> }
> >>
> >> - debug_msix_enabled_old = s->msix->enabled;
> >> - s->msix->enabled = !!(*val & PCI_MSIX_FLAGS_ENABLE);
> >> if (s->msix->enabled != debug_msix_enabled_old) {
> >> XEN_PT_LOG(&s->dev, "%s MSI-X\n",
> >> s->msix->enabled ? "enable" : "disable");
> >> }
> >>
> >> + xen_host_pci_get_word(&s->real_device, s->msix->ctrl_offset, &dev_value);
> >
> > I have to say that I don't like the asymmetry between reading and
> > writing PCI config registers. If writes go via hypercalls, reads should
> > go via hypercalls too.
>
> We're not doing any cfg register write via hypercalls (not here,
> and not elsewhere). What is being replaced by the patch are
> write to two bits which happen to live in PCI config space. Plus,
> reading directly, and doing writes via hypercall only when really
> needed would still be the right thing from a performance pov.
OK. It would be nice to document somewhere that QEMU is not supposed to
touch those two bits directly, but I wouldn't know where. Maybe a new
doc under docs/misc?
> >> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
> >> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
> >> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
> >> return -1;
> >> }
> >>
> >> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
> >> - enabled);
> >
> > Would it make sense to remove msi_msix_enable completely to avoid any
> > further mistakes?
>
> Perhaps, yes. I think I actually had suggested so quite a while back.
> But I don't see myself wasting much more time on this, ehm, code.
Isn't it just a matter of removing msi_msix_enable?
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
2015-06-16 14:56 ` Stefano Stabellini
@ 2015-06-16 16:03 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 16:03 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:56, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:
>> >> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
>> >> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
>> >> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>> >> return -1;
>> >> }
>> >>
>> >> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
>> >> - enabled);
>> >
>> > Would it make sense to remove msi_msix_enable completely to avoid any
>> > further mistakes?
>>
>> Perhaps, yes. I think I actually had suggested so quite a while back.
>> But I don't see myself wasting much more time on this, ehm, code.
>
> Isn't it just a matter of removing msi_msix_enable?
It has another caller xen_pt_msi_set_enable(). If we went down
the route of what this patch does, then MSI's enable bit should
ultimately also be driven through a hypercall, and that would then
be the point where the function would naturally disappear. But as
said, it looks like we're intending to go a different route anyway.
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls
@ 2015-06-16 16:03 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2015-06-16 16:03 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel, qemu-devel
>>> On 16.06.15 at 16:56, <stefano.stabellini@eu.citrix.com> wrote:
> On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 16:03, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:
>> >> --- a/qemu/upstream/hw/xen/xen_pt_msi.c
>> >> +++ b/qemu/upstream/hw/xen/xen_pt_msi.c
>> >> @@ -301,8 +301,11 @@ static int msix_set_enable(XenPCIPassthr
>> >> return -1;
>> >> }
>> >>
>> >> - return msi_msix_enable(s, s->msix->ctrl_offset, PCI_MSIX_FLAGS_ENABLE,
>> >> - enabled);
>> >
>> > Would it make sense to remove msi_msix_enable completely to avoid any
>> > further mistakes?
>>
>> Perhaps, yes. I think I actually had suggested so quite a while back.
>> But I don't see myself wasting much more time on this, ehm, code.
>
> Isn't it just a matter of removing msi_msix_enable?
It has another caller xen_pt_msi_set_enable(). If we went down
the route of what this patch does, then MSI's enable bit should
ultimately also be driven through a hypercall, and that would then
be the point where the function would naturally disappear. But as
said, it looks like we're intending to go a different route anyway.
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2015-06-16 16:03 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-05 11:55 [Qemu-devel] [PATCH 0/6] xen/pass-through: XSA-120, 128...131 follow-up Jan Beulich
2015-06-05 11:59 ` [Qemu-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes Jan Beulich
2015-06-05 11:59 ` Jan Beulich
2015-06-16 13:35 ` Stefano Stabellini
2015-06-16 13:35 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-16 14:48 ` Stefano Stabellini
2015-06-16 14:48 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:04 ` Jan Beulich
2015-06-05 12:01 ` [Qemu-devel] [PATCH 2/6] xen/MSI-X: drive maskall and enable bits through hypercalls Jan Beulich
2015-06-05 12:01 ` Jan Beulich
2015-06-16 14:03 ` Stefano Stabellini
2015-06-16 14:03 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:19 ` Jan Beulich
2015-06-16 14:19 ` [Qemu-devel] " Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-16 16:03 ` Jan Beulich
2015-06-16 16:03 ` Jan Beulich
2015-06-16 14:56 ` Stefano Stabellini
2015-06-05 12:02 ` [Qemu-devel] [PATCH 3/6] xen/MSI-X: really enforce alignment Jan Beulich
2015-06-05 12:02 ` Jan Beulich
2015-06-16 14:08 ` Stefano Stabellini
2015-06-16 14:08 ` [Qemu-devel] " Stefano Stabellini
2015-06-05 12:03 ` [Qemu-devel] [PATCH 4/6] xen/pass-through: correctly deal with RW1C bits Jan Beulich
2015-06-05 12:03 ` Jan Beulich
2015-06-16 14:19 ` Stefano Stabellini
2015-06-16 14:19 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:38 ` Jan Beulich
2015-06-16 14:38 ` Jan Beulich
2015-06-05 12:04 ` [Qemu-devel] [PATCH 5/6] xen/pass-through: log errno values rather than function return ones Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:23 ` Stefano Stabellini
2015-06-16 14:23 ` [Qemu-devel] " Stefano Stabellini
2015-06-05 12:04 ` [Qemu-devel] [PATCH 6/6] xen/pass-through: constify some static data Jan Beulich
2015-06-05 12:04 ` Jan Beulich
2015-06-16 14:27 ` Stefano Stabellini
2015-06-16 14:27 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
2015-06-16 14:43 ` Stefano Stabellini
2015-06-16 14:43 ` [Qemu-devel] " Stefano Stabellini
2015-06-16 14:41 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.