Re: [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
>>> Tue, 16 Jun 2015 15:48:16 +0100 Stefano Stabellini wrote:
>On Tue, 16 Jun 2015, Jan Beulich wrote:
>> >>> On 16.06.15 at 15:35, Stefano Stabellini wrote:
>> > On Fri, 5 Jun 2015, Jan Beulich wrote:
I'm sorry for getting back to this only now.
>> >> @@ -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?
That would maybe be doable with just pci_msix_write() as a caller,
but not with the one in xen_pt_msix_update() (where we have no
transition of the mask bit from set to clear).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
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 jbeul...@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,
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
On 16.06.15 at 15:35, stefano.stabell...@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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/6] xen/MSI-X: latch MSI-X table writes
On Tue, 16 Jun 2015, Jan Beulich wrote:
On 16.06.15 at 15:35, stefano.stabell...@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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH 1/6] 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 jbeul...@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,
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
