| Message ID | 1552602649-14358-1-git-send-email-igor.druzhinin@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] x86/hvm: split all linear reads and writes at page boundary | expand |
> -----Original Message----- > From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com] > Sent: 14 March 2019 22:31 > To: xen-devel@lists.xenproject.org > Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; > Igor Druzhinin <igor.druzhinin@citrix.com> > Subject: [PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary > > Ruling out page straddling at linear level makes it easier to > distinguish chunks that require proper handling as MMIO access > and not complete them as page straddling memory transactions > prematurely. This doesn't change the general behavior. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> I think this makes things easier to reason about. Reviewed-by: Paul Durrant <paul.durrant@citrix.com> > --- > Changes in v3: > * new patch in v3 to address the concern of P2M type change along with > page straddling > --- > xen/arch/x86/hvm/emulate.c | 72 ++++++++++++++++++++++++---------------------- > 1 file changed, 38 insertions(+), 34 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 754baf6..4879ccb 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -1089,12 +1089,25 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data, > uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt) > { > pagefault_info_t pfinfo; > - int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo); > + unsigned int offset = addr & ~PAGE_MASK; > + int rc; > > - switch ( rc ) > + if ( offset + bytes > PAGE_SIZE ) > { > - unsigned int offset, part1; > + unsigned int part1 = PAGE_SIZE - offset; > + > + /* Split the access at the page boundary. */ > + rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt); > + if ( rc == X86EMUL_OKAY ) > + rc = linear_read(addr + part1, bytes - part1, p_data + part1, > + pfec, hvmemul_ctxt); > + return rc; > + } > + > + rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo); > > + switch ( rc ) > + { > case HVMTRANS_okay: > return X86EMUL_OKAY; > > @@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data, > if ( pfec & PFEC_insn_fetch ) > return X86EMUL_UNHANDLEABLE; > > - offset = addr & ~PAGE_MASK; > - if ( offset + bytes <= PAGE_SIZE ) > - return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, > - hvmemul_ctxt, > - known_gla(addr, bytes, pfec)); > - > - /* Split the access at the page boundary. */ > - part1 = PAGE_SIZE - offset; > - rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt); > - if ( rc == X86EMUL_OKAY ) > - rc = linear_read(addr + part1, bytes - part1, p_data + part1, > - pfec, hvmemul_ctxt); > - return rc; > - > + return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, > + hvmemul_ctxt, > + known_gla(addr, bytes, pfec)); > case HVMTRANS_gfn_paged_out: > case HVMTRANS_gfn_shared: > return X86EMUL_RETRY; > @@ -1132,12 +1134,25 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data, > uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt) > { > pagefault_info_t pfinfo; > - int rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo); > + unsigned int offset = addr & ~PAGE_MASK; > + int rc; > > - switch ( rc ) > + if ( offset + bytes > PAGE_SIZE ) > { > - unsigned int offset, part1; > + unsigned int part1 = PAGE_SIZE - offset; > > + /* Split the access at the page boundary. */ > + rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt); > + if ( rc == X86EMUL_OKAY ) > + rc = linear_write(addr + part1, bytes - part1, p_data + part1, > + pfec, hvmemul_ctxt); > + return rc; > + } > + > + rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo); > + > + switch ( rc ) > + { > case HVMTRANS_okay: > return X86EMUL_OKAY; > > @@ -1146,20 +1161,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data, > return X86EMUL_EXCEPTION; > > case HVMTRANS_bad_gfn_to_mfn: > - offset = addr & ~PAGE_MASK; > - if ( offset + bytes <= PAGE_SIZE ) > - return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec, > - hvmemul_ctxt, > - known_gla(addr, bytes, pfec)); > - > - /* Split the access at the page boundary. */ > - part1 = PAGE_SIZE - offset; > - rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt); > - if ( rc == X86EMUL_OKAY ) > - rc = linear_write(addr + part1, bytes - part1, p_data + part1, > - pfec, hvmemul_ctxt); > - return rc; > - > + return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec, > + hvmemul_ctxt, > + known_gla(addr, bytes, pfec)); > case HVMTRANS_gfn_paged_out: > case HVMTRANS_gfn_shared: > return X86EMUL_RETRY; > -- > 2.7.4
>>> On 14.03.19 at 23:30, <igor.druzhinin@citrix.com> wrote: > Ruling out page straddling at linear level makes it easier to > distinguish chunks that require proper handling as MMIO access > and not complete them as page straddling memory transactions > prematurely. This doesn't change the general behavior. > > Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one cosmetic aspect taken care of (can be done while committing): > @@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data, > if ( pfec & PFEC_insn_fetch ) > return X86EMUL_UNHANDLEABLE; > > - offset = addr & ~PAGE_MASK; > - if ( offset + bytes <= PAGE_SIZE ) > - return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, > - hvmemul_ctxt, > - known_gla(addr, bytes, pfec)); > - > - /* Split the access at the page boundary. */ > - part1 = PAGE_SIZE - offset; > - rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt); > - if ( rc == X86EMUL_OKAY ) > - rc = linear_read(addr + part1, bytes - part1, p_data + part1, > - pfec, hvmemul_ctxt); > - return rc; > - > + return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, > + hvmemul_ctxt, > + known_gla(addr, bytes, pfec)); > case HVMTRANS_gfn_paged_out: Please retain the blank line above here (and also in the write case). I notice that sadly the change doesn't allow removing the respective logic from hvmemul_linear_mmio_access() yet, due to its use by hvmemul_cmpxchg(). Jan
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 754baf6..4879ccb 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -1089,12 +1089,25 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data, uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt) { pagefault_info_t pfinfo; - int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo); + unsigned int offset = addr & ~PAGE_MASK; + int rc; - switch ( rc ) + if ( offset + bytes > PAGE_SIZE ) { - unsigned int offset, part1; + unsigned int part1 = PAGE_SIZE - offset; + + /* Split the access at the page boundary. */ + rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt); + if ( rc == X86EMUL_OKAY ) + rc = linear_read(addr + part1, bytes - part1, p_data + part1, + pfec, hvmemul_ctxt); + return rc; + } + + rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo); + switch ( rc ) + { case HVMTRANS_okay: return X86EMUL_OKAY; @@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data, if ( pfec & PFEC_insn_fetch ) return X86EMUL_UNHANDLEABLE; - offset = addr & ~PAGE_MASK; - if ( offset + bytes <= PAGE_SIZE ) - return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, - hvmemul_ctxt, - known_gla(addr, bytes, pfec)); - - /* Split the access at the page boundary. */ - part1 = PAGE_SIZE - offset; - rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt); - if ( rc == X86EMUL_OKAY ) - rc = linear_read(addr + part1, bytes - part1, p_data + part1, - pfec, hvmemul_ctxt); - return rc; - + return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec, + hvmemul_ctxt, + known_gla(addr, bytes, pfec)); case HVMTRANS_gfn_paged_out: case HVMTRANS_gfn_shared: return X86EMUL_RETRY; @@ -1132,12 +1134,25 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data, uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt) { pagefault_info_t pfinfo; - int rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo); + unsigned int offset = addr & ~PAGE_MASK; + int rc; - switch ( rc ) + if ( offset + bytes > PAGE_SIZE ) { - unsigned int offset, part1; + unsigned int part1 = PAGE_SIZE - offset; + /* Split the access at the page boundary. */ + rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt); + if ( rc == X86EMUL_OKAY ) + rc = linear_write(addr + part1, bytes - part1, p_data + part1, + pfec, hvmemul_ctxt); + return rc; + } + + rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo); + + switch ( rc ) + { case HVMTRANS_okay: return X86EMUL_OKAY; @@ -1146,20 +1161,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data, return X86EMUL_EXCEPTION; case HVMTRANS_bad_gfn_to_mfn: - offset = addr & ~PAGE_MASK; - if ( offset + bytes <= PAGE_SIZE ) - return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec, - hvmemul_ctxt, - known_gla(addr, bytes, pfec)); - - /* Split the access at the page boundary. */ - part1 = PAGE_SIZE - offset; - rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt); - if ( rc == X86EMUL_OKAY ) - rc = linear_write(addr + part1, bytes - part1, p_data + part1, - pfec, hvmemul_ctxt); - return rc; - + return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec, + hvmemul_ctxt, + known_gla(addr, bytes, pfec)); case HVMTRANS_gfn_paged_out: case HVMTRANS_gfn_shared: return X86EMUL_RETRY;
Ruling out page straddling at linear level makes it easier to distinguish chunks that require proper handling as MMIO access and not complete them as page straddling memory transactions prematurely. This doesn't change the general behavior. Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com> --- Changes in v3: * new patch in v3 to address the concern of P2M type change along with page straddling --- xen/arch/x86/hvm/emulate.c | 72 ++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 34 deletions(-)