* [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic
2016-04-28 9:35 [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
@ 2016-04-28 9:49 ` Jan Beulich
2016-04-28 10:22 ` Andrew Cooper
2016-04-28 10:44 ` Paul Durrant
2016-04-28 9:49 ` [PATCH 2/3] x86/vMSI-X: also snoop qword writes Jan Beulich
` (2 subsequent siblings)
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 9:49 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]
msixtbl_range(), as any other MMIO ->check() handlers, may get called
with other than the base address of an access - avoid the snoop logic
considering those.
Also avoid considering vCPU-s not blocked in the hypervisor in
msixtbl_pt_register(), just to be on the safe side.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -349,7 +349,7 @@ static int msixtbl_range(struct vcpu *v,
{
const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
- if ( r->state != STATE_IOREQ_READY )
+ if ( r->state != STATE_IOREQ_READY || r->addr != addr )
return 0;
ASSERT(r->type == IOREQ_TYPE_COPY);
if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
@@ -457,7 +457,8 @@ out:
for_each_vcpu ( d, v )
{
- if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
+ if ( (v->pause_flags & VPF_blocked_in_xen) &&
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
(gtable + msi_desc->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE +
PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
[-- Attachment #2: x86-vMSI-X-first-unmask-checks.patch --]
[-- Type: text/plain, Size: 1267 bytes --]
x86/vMSI-X: add further checks to snoop logic
msixtbl_range(), as any other MMIO ->check() handlers, may get called
with other than the base address of an access - avoid the snoop logic
considering those.
Also avoid considering vCPU-s not blocked in the hypervisor in
msixtbl_pt_register(), just to be on the safe side.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -349,7 +349,7 @@ static int msixtbl_range(struct vcpu *v,
{
const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
- if ( r->state != STATE_IOREQ_READY )
+ if ( r->state != STATE_IOREQ_READY || r->addr != addr )
return 0;
ASSERT(r->type == IOREQ_TYPE_COPY);
if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
@@ -457,7 +457,8 @@ out:
for_each_vcpu ( d, v )
{
- if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
+ if ( (v->pause_flags & VPF_blocked_in_xen) &&
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
(gtable + msi_desc->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE +
PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
[-- 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] 17+ messages in thread* Re: [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic
2016-04-28 9:49 ` [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic Jan Beulich
@ 2016-04-28 10:22 ` Andrew Cooper
2016-04-28 10:44 ` Paul Durrant
1 sibling, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2016-04-28 10:22 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Paul Durrant
On 28/04/16 10:49, Jan Beulich wrote:
> msixtbl_range(), as any other MMIO ->check() handlers, may get called
> with other than the base address of an access - avoid the snoop logic
> considering those.
>
> Also avoid considering vCPU-s not blocked in the hypervisor in
> msixtbl_pt_register(), just to be on the safe side.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic
2016-04-28 9:49 ` [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic Jan Beulich
2016-04-28 10:22 ` Andrew Cooper
@ 2016-04-28 10:44 ` Paul Durrant
1 sibling, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2016-04-28 10:44 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 10:49
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic
>
> msixtbl_range(), as any other MMIO ->check() handlers, may get called
> with other than the base address of an access - avoid the snoop logic
> considering those.
>
> Also avoid considering vCPU-s not blocked in the hypervisor in
> msixtbl_pt_register(), just to be on the safe side.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -349,7 +349,7 @@ static int msixtbl_range(struct vcpu *v,
> {
> const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
>
> - if ( r->state != STATE_IOREQ_READY )
> + if ( r->state != STATE_IOREQ_READY || r->addr != addr )
> return 0;
> ASSERT(r->type == IOREQ_TYPE_COPY);
> if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
> @@ -457,7 +457,8 @@ out:
>
> for_each_vcpu ( d, v )
> {
> - if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
> + if ( (v->pause_flags & VPF_blocked_in_xen) &&
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
> (gtable + msi_desc->msi_attrib.entry_nr *
> PCI_MSIX_ENTRY_SIZE +
> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] x86/vMSI-X: also snoop qword writes
2016-04-28 9:35 [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
2016-04-28 9:49 ` [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic Jan Beulich
@ 2016-04-28 9:49 ` Jan Beulich
2016-04-28 10:27 ` Andrew Cooper
2016-04-28 10:52 ` Paul Durrant
2016-04-28 9:50 ` [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS Jan Beulich
2016-04-28 9:51 ` [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 9:49 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
[-- Attachment #1: Type: text/plain, Size: 1959 bytes --]
... the high half of which may be a write to the Vector Control field.
This gets things in sync again with msixtbl_write().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -336,6 +336,7 @@ out:
static int msixtbl_range(struct vcpu *v, unsigned long addr)
{
const struct msi_desc *desc;
+ const ioreq_t *r;
rcu_read_lock(&msixtbl_rcu_lock);
desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
@@ -344,17 +345,29 @@ static int msixtbl_range(struct vcpu *v,
if ( desc )
return 1;
- if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
- PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
+ r = &v->arch.hvm_vcpu.hvm_io.io_req;
+ if ( r->state != STATE_IOREQ_READY || r->addr != addr )
+ return 0;
+ ASSERT(r->type == IOREQ_TYPE_COPY);
+ if ( r->dir == IOREQ_WRITE )
{
- const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
+ if ( !r->data_is_ptr )
+ {
+ unsigned int size = r->size;
+ uint64_t data = r->data;
- if ( r->state != STATE_IOREQ_READY || r->addr != addr )
- return 0;
- ASSERT(r->type == IOREQ_TYPE_COPY);
- if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
- && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
- v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ if ( size == 8 )
+ {
+ BUILD_BUG_ON(!(PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET & 4));
+ data >>= 32;
+ addr += size = 4;
+ }
+ if ( size == 4 &&
+ ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
+ PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
+ !(data & PCI_MSIX_VECTOR_BITMASK) )
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ }
}
return 0;
[-- Attachment #2: x86-vMSI-X-first-unmask-qword.patch --]
[-- Type: text/plain, Size: 1992 bytes --]
x86/vMSI-X: also snoop qword writes
... the high half of which may be a write to the Vector Control field.
This gets things in sync again with msixtbl_write().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -336,6 +336,7 @@ out:
static int msixtbl_range(struct vcpu *v, unsigned long addr)
{
const struct msi_desc *desc;
+ const ioreq_t *r;
rcu_read_lock(&msixtbl_rcu_lock);
desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
@@ -344,17 +345,29 @@ static int msixtbl_range(struct vcpu *v,
if ( desc )
return 1;
- if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
- PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
+ r = &v->arch.hvm_vcpu.hvm_io.io_req;
+ if ( r->state != STATE_IOREQ_READY || r->addr != addr )
+ return 0;
+ ASSERT(r->type == IOREQ_TYPE_COPY);
+ if ( r->dir == IOREQ_WRITE )
{
- const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
+ if ( !r->data_is_ptr )
+ {
+ unsigned int size = r->size;
+ uint64_t data = r->data;
- if ( r->state != STATE_IOREQ_READY || r->addr != addr )
- return 0;
- ASSERT(r->type == IOREQ_TYPE_COPY);
- if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
- && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
- v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ if ( size == 8 )
+ {
+ BUILD_BUG_ON(!(PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET & 4));
+ data >>= 32;
+ addr += size = 4;
+ }
+ if ( size == 4 &&
+ ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
+ PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
+ !(data & PCI_MSIX_VECTOR_BITMASK) )
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ }
}
return 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] 17+ messages in thread* Re: [PATCH 2/3] x86/vMSI-X: also snoop qword writes
2016-04-28 9:49 ` [PATCH 2/3] x86/vMSI-X: also snoop qword writes Jan Beulich
@ 2016-04-28 10:27 ` Andrew Cooper
2016-04-28 10:52 ` Paul Durrant
1 sibling, 0 replies; 17+ messages in thread
From: Andrew Cooper @ 2016-04-28 10:27 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Paul Durrant
On 28/04/16 10:49, Jan Beulich wrote:
> ... the high half of which may be a write to the Vector Control field.
> This gets things in sync again with msixtbl_write().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/3] x86/vMSI-X: also snoop qword writes
2016-04-28 9:49 ` [PATCH 2/3] x86/vMSI-X: also snoop qword writes Jan Beulich
2016-04-28 10:27 ` Andrew Cooper
@ 2016-04-28 10:52 ` Paul Durrant
1 sibling, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2016-04-28 10:52 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 10:50
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 2/3] x86/vMSI-X: also snoop qword writes
>
> ... the high half of which may be a write to the Vector Control field.
> This gets things in sync again with msixtbl_write().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -336,6 +336,7 @@ out:
> static int msixtbl_range(struct vcpu *v, unsigned long addr)
> {
> const struct msi_desc *desc;
> + const ioreq_t *r;
>
> rcu_read_lock(&msixtbl_rcu_lock);
> desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
> @@ -344,17 +345,29 @@ static int msixtbl_range(struct vcpu *v,
> if ( desc )
> return 1;
>
> - if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
> + r = &v->arch.hvm_vcpu.hvm_io.io_req;
> + if ( r->state != STATE_IOREQ_READY || r->addr != addr )
> + return 0;
> + ASSERT(r->type == IOREQ_TYPE_COPY);
> + if ( r->dir == IOREQ_WRITE )
> {
> - const ioreq_t *r = &v->arch.hvm_vcpu.hvm_io.io_req;
> + if ( !r->data_is_ptr )
> + {
> + unsigned int size = r->size;
> + uint64_t data = r->data;
>
> - if ( r->state != STATE_IOREQ_READY || r->addr != addr )
> - return 0;
> - ASSERT(r->type == IOREQ_TYPE_COPY);
> - if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
> - && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
> - v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> + if ( size == 8 )
> + {
> + BUILD_BUG_ON(!(PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET & 4));
> + data >>= 32;
> + addr += size = 4;
> + }
> + if ( size == 4 &&
> + ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> + !(data & PCI_MSIX_VECTOR_BITMASK) )
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> + }
> }
>
> return 0;
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 9:35 [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
2016-04-28 9:49 ` [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic Jan Beulich
2016-04-28 9:49 ` [PATCH 2/3] x86/vMSI-X: also snoop qword writes Jan Beulich
@ 2016-04-28 9:50 ` Jan Beulich
2016-04-28 10:34 ` Andrew Cooper
2016-04-28 11:17 ` Paul Durrant
2016-04-28 9:51 ` [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
3 siblings, 2 replies; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 9:50 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant
[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]
... as at least certain versions of Windows use such to update the
MSI-X table. However, to not overly complicate the logic for now
- only EFLAGS.DF=0 is being handled,
- only updates not crossing MSI-X table entry boundaries are handled.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
ASSERT(r->type == IOREQ_TYPE_COPY);
if ( r->dir == IOREQ_WRITE )
{
+ unsigned int size = r->size;
+
if ( !r->data_is_ptr )
{
- unsigned int size = r->size;
uint64_t data = r->data;
if ( size == 8 )
@@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
!(data & PCI_MSIX_VECTOR_BITMASK) )
+ {
v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
+ }
+ }
+ else if ( (size == 4 || size == 8) && !r->df &&
+ r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
+ !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
+ {
+ BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
+ (PCI_MSIX_ENTRY_SIZE - 1));
+
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
+ addr + size * r->count - 4;
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
+ r->data + size * r->count - 4;
}
}
@@ -471,6 +487,7 @@ out:
for_each_vcpu ( d, v )
{
if ( (v->pause_flags & VPF_blocked_in_xen) &&
+ !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
(gtable + msi_desc->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE +
@@ -551,9 +568,29 @@ void msixtbl_pt_cleanup(struct domain *d
void msix_write_completion(struct vcpu *v)
{
unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
+ unsigned long snoop_addr = v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
+ if ( !ctrl_address && snoop_addr &&
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
+ {
+ const struct msi_desc *desc;
+ uint32_t data;
+
+ rcu_read_lock(&msixtbl_rcu_lock);
+ desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
+ snoop_addr);
+ rcu_read_unlock(&msixtbl_rcu_lock);
+
+ if ( desc &&
+ hvm_copy_from_guest_phys(&data,
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
+ sizeof(data)) == HVMCOPY_okay &&
+ !(data & PCI_MSIX_VECTOR_BITMASK) )
+ ctrl_address = snoop_addr;
+ }
+
if ( !ctrl_address )
return;
--- unstable.orig/xen/include/asm-x86/hvm/vcpu.h 2016-04-27 14:47:25.000000000 +0200
+++ unstable/xen/include/asm-x86/hvm/vcpu.h 2016-04-25 16:04:48.000000000 +0200
@@ -86,6 +86,7 @@ struct hvm_vcpu_io {
unsigned long msix_unmask_address;
unsigned long msix_snoop_address;
+ unsigned long msix_snoop_gpa;
const struct g2m_ioport *g2m_ioport;
};
[-- Attachment #2: x86-vMSI-X-first-unmask-rep.patch --]
[-- Type: text/plain, Size: 3572 bytes --]
x86/vMSI-X: also snoop REP MOVS
... as at least certain versions of Windows use such to update the
MSI-X table. However, to not overly complicate the logic for now
- only EFLAGS.DF=0 is being handled,
- only updates not crossing MSI-X table entry boundaries are handled.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
ASSERT(r->type == IOREQ_TYPE_COPY);
if ( r->dir == IOREQ_WRITE )
{
+ unsigned int size = r->size;
+
if ( !r->data_is_ptr )
{
- unsigned int size = r->size;
uint64_t data = r->data;
if ( size == 8 )
@@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
!(data & PCI_MSIX_VECTOR_BITMASK) )
+ {
v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
+ }
+ }
+ else if ( (size == 4 || size == 8) && !r->df &&
+ r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
+ !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
+ {
+ BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
+ (PCI_MSIX_ENTRY_SIZE - 1));
+
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
+ addr + size * r->count - 4;
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
+ r->data + size * r->count - 4;
}
}
@@ -471,6 +487,7 @@ out:
for_each_vcpu ( d, v )
{
if ( (v->pause_flags & VPF_blocked_in_xen) &&
+ !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
(gtable + msi_desc->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE +
@@ -551,9 +568,29 @@ void msixtbl_pt_cleanup(struct domain *d
void msix_write_completion(struct vcpu *v)
{
unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
+ unsigned long snoop_addr = v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
+ if ( !ctrl_address && snoop_addr &&
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
+ {
+ const struct msi_desc *desc;
+ uint32_t data;
+
+ rcu_read_lock(&msixtbl_rcu_lock);
+ desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
+ snoop_addr);
+ rcu_read_unlock(&msixtbl_rcu_lock);
+
+ if ( desc &&
+ hvm_copy_from_guest_phys(&data,
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
+ sizeof(data)) == HVMCOPY_okay &&
+ !(data & PCI_MSIX_VECTOR_BITMASK) )
+ ctrl_address = snoop_addr;
+ }
+
if ( !ctrl_address )
return;
--- unstable.orig/xen/include/asm-x86/hvm/vcpu.h 2016-04-27 14:47:25.000000000 +0200
+++ unstable/xen/include/asm-x86/hvm/vcpu.h 2016-04-25 16:04:48.000000000 +0200
@@ -86,6 +86,7 @@ struct hvm_vcpu_io {
unsigned long msix_unmask_address;
unsigned long msix_snoop_address;
+ unsigned long msix_snoop_gpa;
const struct g2m_ioport *g2m_ioport;
};
[-- 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] 17+ messages in thread* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 9:50 ` [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS Jan Beulich
@ 2016-04-28 10:34 ` Andrew Cooper
2016-04-28 10:44 ` Jan Beulich
2016-04-28 11:17 ` Paul Durrant
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Cooper @ 2016-04-28 10:34 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Paul Durrant
On 28/04/16 10:50, Jan Beulich wrote:
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
IMO, under those circumstances we should domain_crash() and be rather
loud on the console.
Perhaps in a debug build only, but this will be a very unpleasant issue
to debug for whomever finds an OS which falls into of those unhandled
situations.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 10:34 ` Andrew Cooper
@ 2016-04-28 10:44 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 10:44 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Paul Durrant
>>> On 28.04.16 at 12:34, <andrew.cooper3@citrix.com> wrote:
> On 28/04/16 10:50, Jan Beulich wrote:
>> ... as at least certain versions of Windows use such to update the
>> MSI-X table. However, to not overly complicate the logic for now
>> - only EFLAGS.DF=0 is being handled,
>> - only updates not crossing MSI-X table entry boundaries are handled.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> IMO, under those circumstances we should domain_crash() and be rather
> loud on the console.
>
> Perhaps in a debug build only, but this will be a very unpleasant issue
> to debug for whomever finds an OS which falls into of those unhandled
> situations.
I disagree: At the time we snoop accesses, we don't know whether
they're targeting any MSI-X table entry. And we shouldn't crash the
guest just because it accessed _something else_ in a way the snoop
logic doesn't support. If any unsupported access gets done by a
guest, all that'll happen is that MSI-X again doesn't work inside the
guest, i.e. the same situation as the previous and this patches are
trying to deal with. (And yes, the debugging wasn't really pleasant,
but that's more because the original authors didn't design the whole
thing properly, and any other solution I could think of would have
caused issues with the qemu/Xen interface logic.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 9:50 ` [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS Jan Beulich
2016-04-28 10:34 ` Andrew Cooper
@ 2016-04-28 11:17 ` Paul Durrant
2016-04-28 11:44 ` Jan Beulich
1 sibling, 1 reply; 17+ messages in thread
From: Paul Durrant @ 2016-04-28 11:17 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 10:50
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
>
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
> ASSERT(r->type == IOREQ_TYPE_COPY);
> if ( r->dir == IOREQ_WRITE )
> {
> + unsigned int size = r->size;
> +
> if ( !r->data_is_ptr )
> {
> - unsigned int size = r->size;
> uint64_t data = r->data;
>
> if ( size == 8 )
> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
> ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> !(data & PCI_MSIX_VECTOR_BITMASK) )
> + {
> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> + }
> + }
> + else if ( (size == 4 || size == 8) && !r->df &&
> + r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> + !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
That's quite an if statement. Any chance of making it more decipherable? I also think it's worth putting the restrictions you outline in the commit in a comment here so that it's clear that the code is not trying to handle all corner cases.
> + {
> + BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> + (PCI_MSIX_ENTRY_SIZE - 1));
> +
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> + addr + size * r->count - 4;
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> + r->data + size * r->count - 4;
Does there need to be any explicit type promotion here since r->data is uint64_t?
Paul
> }
> }
>
> @@ -471,6 +487,7 @@ out:
> for_each_vcpu ( d, v )
> {
> if ( (v->pause_flags & VPF_blocked_in_xen) &&
> + !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
> v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
> (gtable + msi_desc->msi_attrib.entry_nr *
> PCI_MSIX_ENTRY_SIZE +
> @@ -551,9 +568,29 @@ void msixtbl_pt_cleanup(struct domain *d
> void msix_write_completion(struct vcpu *v)
> {
> unsigned long ctrl_address = v-
> >arch.hvm_vcpu.hvm_io.msix_unmask_address;
> + unsigned long snoop_addr = v-
> >arch.hvm_vcpu.hvm_io.msix_snoop_address;
>
> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
>
> + if ( !ctrl_address && snoop_addr &&
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
> + {
> + const struct msi_desc *desc;
> + uint32_t data;
> +
> + rcu_read_lock(&msixtbl_rcu_lock);
> + desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
> + snoop_addr);
> + rcu_read_unlock(&msixtbl_rcu_lock);
> +
> + if ( desc &&
> + hvm_copy_from_guest_phys(&data,
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
> + sizeof(data)) == HVMCOPY_okay &&
> + !(data & PCI_MSIX_VECTOR_BITMASK) )
> + ctrl_address = snoop_addr;
> + }
> +
> if ( !ctrl_address )
> return;
>
> --- unstable.orig/xen/include/asm-x86/hvm/vcpu.h 2016-04-27
> 14:47:25.000000000 +0200
> +++ unstable/xen/include/asm-x86/hvm/vcpu.h 2016-04-25
> 16:04:48.000000000 +0200
> @@ -86,6 +86,7 @@ struct hvm_vcpu_io {
>
> unsigned long msix_unmask_address;
> unsigned long msix_snoop_address;
> + unsigned long msix_snoop_gpa;
>
> const struct g2m_ioport *g2m_ioport;
> };
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 11:17 ` Paul Durrant
@ 2016-04-28 11:44 ` Jan Beulich
2016-04-28 11:58 ` Paul Durrant
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 11:44 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 28.04.16 at 13:17, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 28 April 2016 10:50
>> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>> ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>> !(data & PCI_MSIX_VECTOR_BITMASK) )
>> + {
>> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
>> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
>> + }
>> + }
>> + else if ( (size == 4 || size == 8) && !r->df &&
>> + r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>> + !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
>
> That's quite an if statement. Any chance of making it more decipherable? I
I don't see how I could be doing this.
> also think it's worth putting the restrictions you outline in the commit in a
> comment here so that it's clear that the code is not trying to handle all
> corner cases.
Sure. Question is whether by mixing code and comments things
would get better readable (to at least somewhat address your
request above), or whether that instead would make things
worse. Thoughts?
>> + {
>> + BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
>> + (PCI_MSIX_ENTRY_SIZE - 1));
>> +
>> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
>> + addr + size * r->count - 4;
>> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
>> + r->data + size * r->count - 4;
>
> Does there need to be any explicit type promotion here since r->data is
> uint64_t?
No, because both size and r->count did already get bounds
checked to very narrow ranges.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 11:44 ` Jan Beulich
@ 2016-04-28 11:58 ` Paul Durrant
2016-04-28 12:30 ` Jan Beulich
0 siblings, 1 reply; 17+ messages in thread
From: Paul Durrant @ 2016-04-28 11:58 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 12:44
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel
> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
>
> >>> On 28.04.16 at 13:17, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 28 April 2016 10:50
> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
> >> ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> >> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> >> !(data & PCI_MSIX_VECTOR_BITMASK) )
> >> + {
> >> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> >> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> >> + }
> >> + }
> >> + else if ( (size == 4 || size == 8) && !r->df &&
> >> + r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> >> + !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
> >
> > That's quite an if statement. Any chance of making it more decipherable? I
>
> I don't see how I could be doing this.
>
> > also think it's worth putting the restrictions you outline in the commit in a
> > comment here so that it's clear that the code is not trying to handle all
> > corner cases.
>
> Sure. Question is whether by mixing code and comments things
> would get better readable (to at least somewhat address your
> request above), or whether that instead would make things
> worse. Thoughts?
I think mentioning why you're only tackling the !r->df case would be worth commenting on and if the && !r->df were on a separate line then the comment could go inline. Also, do you really need to check r->count (seems like a count of 0 should have been picked up before the code got here) and then TBH I'm not even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) is even checking so how about an illustratively named macro?
Paul
>
> >> + {
> >> + BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> >> + (PCI_MSIX_ENTRY_SIZE - 1));
> >> +
> >> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> >> + addr + size * r->count - 4;
> >> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> >> + r->data + size * r->count - 4;
> >
> > Does there need to be any explicit type promotion here since r->data is
> > uint64_t?
>
> No, because both size and r->count did already get bounds
> checked to very narrow ranges.
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 11:58 ` Paul Durrant
@ 2016-04-28 12:30 ` Jan Beulich
2016-04-28 12:44 ` Paul Durrant
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 12:30 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 28.04.16 at 13:58, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 28 April 2016 12:44
>> To: Paul Durrant
>> Cc: Andrew Cooper; xen-devel
>> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
>>
>> >>> On 28.04.16 at 13:17, <Paul.Durrant@citrix.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: 28 April 2016 10:50
>> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>> >> ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>> >> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>> >> !(data & PCI_MSIX_VECTOR_BITMASK) )
>> >> + {
>> >> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
>> >> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
>> >> + }
>> >> + }
>> >> + else if ( (size == 4 || size == 8) && !r->df &&
>> >> + r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>> >> + !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
>> >
>> > That's quite an if statement. Any chance of making it more decipherable? I
>>
>> I don't see how I could be doing this.
>>
>> > also think it's worth putting the restrictions you outline in the commit in
> a
>> > comment here so that it's clear that the code is not trying to handle all
>> > corner cases.
>>
>> Sure. Question is whether by mixing code and comments things
>> would get better readable (to at least somewhat address your
>> request above), or whether that instead would make things
>> worse. Thoughts?
>
> I think mentioning why you're only tackling the !r->df case would be worth
> commenting on and if the && !r->df were on a separate line then the comment could
> go inline.
That's what I did.
> Also, do you really need to check r->count (seems like a count of 0
> should have been picked up before the code got here)
I've tried to fine where r->count == 0 would be dealt with, but
could spot the location (other than relying on x86_emulate.c
never passing such down), so since I want to subtract 1 from it
(or really 4 from its product with "size") I wanted to be on the
safe side. If you prefer, I could replace this by a respective
ASSERT()...
> and then TBH I'm not
> even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) is even
> checking so how about an illustratively named macro?
How about this (without macro)?
else if ( (size == 4 || size == 8) &&
/* Only support forward REP MOVS for now. */
!r->df &&
/*
* Only fully support accesses to a single table entry for
* now (if multiple ones get written to in one go, only the
* final one gets dealt with).
*/
r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
!((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
{
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
2016-04-28 12:30 ` Jan Beulich
@ 2016-04-28 12:44 ` Paul Durrant
0 siblings, 0 replies; 17+ messages in thread
From: Paul Durrant @ 2016-04-28 12:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 28 April 2016 13:31
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel
> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
>
> >>> On 28.04.16 at 13:58, <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 28 April 2016 12:44
> >> To: Paul Durrant
> >> Cc: Andrew Cooper; xen-devel
> >> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> >>
> >> >>> On 28.04.16 at 13:17, <Paul.Durrant@citrix.com> wrote:
> >> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> >> Sent: 28 April 2016 10:50
> >> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
> >> >> ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> >> >> PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> >> >> !(data & PCI_MSIX_VECTOR_BITMASK) )
> >> >> + {
> >> >> v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> >> >> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> >> >> + }
> >> >> + }
> >> >> + else if ( (size == 4 || size == 8) && !r->df &&
> >> >> + r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> >> >> + !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
> >> >
> >> > That's quite an if statement. Any chance of making it more
> decipherable? I
> >>
> >> I don't see how I could be doing this.
> >>
> >> > also think it's worth putting the restrictions you outline in the commit in
> > a
> >> > comment here so that it's clear that the code is not trying to handle all
> >> > corner cases.
> >>
> >> Sure. Question is whether by mixing code and comments things
> >> would get better readable (to at least somewhat address your
> >> request above), or whether that instead would make things
> >> worse. Thoughts?
> >
> > I think mentioning why you're only tackling the !r->df case would be worth
> > commenting on and if the && !r->df were on a separate line then the
> comment could
> > go inline.
>
> That's what I did.
>
> > Also, do you really need to check r->count (seems like a count of 0
> > should have been picked up before the code got here)
>
> I've tried to fine where r->count == 0 would be dealt with, but
> could spot the location (other than relying on x86_emulate.c
> never passing such down), so since I want to subtract 1 from it
> (or really 4 from its product with "size") I wanted to be on the
> safe side. If you prefer, I could replace this by a respective
> ASSERT()...
>
> > and then TBH I'm not
> > even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1))
> is even
> > checking so how about an illustratively named macro?
>
> How about this (without macro)?
>
> else if ( (size == 4 || size == 8) &&
> /* Only support forward REP MOVS for now. */
> !r->df &&
> /*
> * Only fully support accesses to a single table entry for
> * now (if multiple ones get written to in one go, only the
> * final one gets dealt with).
> */
> r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
> {
>
That looks a lot better to me :-)
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] x86/vMSI-X: further write snoop improvements
2016-04-28 9:35 [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
` (2 preceding siblings ...)
2016-04-28 9:50 ` [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS Jan Beulich
@ 2016-04-28 9:51 ` Jan Beulich
2016-04-28 10:26 ` Wei Liu
3 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2016-04-28 9:51 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Paul Durrant, xen-devel
>>> On 28.04.16 at 11:35, <JBeulich@suse.com> wrote:
> 1: add further checks to snoop logic
> 2: also snoop qword writes
> 3: also snoop REP MOVS
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Wei,
I've only now realized that I should have Cc-ed you on this series (all
intended for 4.7).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/3] x86/vMSI-X: further write snoop improvements
2016-04-28 9:51 ` [PATCH 0/3] x86/vMSI-X: further write snoop improvements Jan Beulich
@ 2016-04-28 10:26 ` Wei Liu
0 siblings, 0 replies; 17+ messages in thread
From: Wei Liu @ 2016-04-28 10:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Paul Durrant, Wei Liu, xen-devel
On Thu, Apr 28, 2016 at 03:51:40AM -0600, Jan Beulich wrote:
> >>> On 28.04.16 at 11:35, <JBeulich@suse.com> wrote:
> > 1: add further checks to snoop logic
> > 2: also snoop qword writes
> > 3: also snoop REP MOVS
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Wei,
>
> I've only now realized that I should have Cc-ed you on this series (all
> intended for 4.7).
>
Subject to ack / review from Andrew / Paul:
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread