| Message ID | 1488987232-12349-4-git-send-email-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.03.17 at 16:33, <yu.c.zhang@linux.intel.com> wrote: > @@ -197,6 +217,10 @@ static int hvmemul_do_io( > * - If the IOREQ_MEM_ACCESS_WRITE flag is not set, treat it > * like a normal PIO or MMIO that doesn't have an ioreq > * server (i.e., by ignoring it). > + * > + * - If the accesss is a read, this could be part of a > + * read-modify-write instruction, emulate the read so that we > + * have it. "it" being what here? Grammatically the insn, but we don't care about "having" the insn. > @@ -226,6 +250,17 @@ static int hvmemul_do_io( > } > > /* > + * This is part of a read-modify-write instruction. "is" or "may be"? Jan
On 3/10/2017 11:33 PM, Jan Beulich wrote: >>>> On 08.03.17 at 16:33, <yu.c.zhang@linux.intel.com> wrote: >> @@ -197,6 +217,10 @@ static int hvmemul_do_io( >> * - If the IOREQ_MEM_ACCESS_WRITE flag is not set, treat it >> * like a normal PIO or MMIO that doesn't have an ioreq >> * server (i.e., by ignoring it). >> + * >> + * - If the accesss is a read, this could be part of a >> + * read-modify-write instruction, emulate the read so that we >> + * have it. > "it" being what here? Grammatically the insn, but we don't care > about "having" the insn. Here "have it" means " to have the value on this address copied in". Sorry for the inaccurate comment. How about just "emulate the read first"? >> @@ -226,6 +250,17 @@ static int hvmemul_do_io( >> } >> >> /* >> + * This is part of a read-modify-write instruction. > "is" or "may be"? I believe should be "is". It's the only scenario I can imagine when an read operation(only when this operation is also a write one) traps. Otherwise there shall be no VM exit. Thanks Yu > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 11.03.17 at 09:42, <yu.c.zhang@linux.intel.com> wrote: > On 3/10/2017 11:33 PM, Jan Beulich wrote: >>>>> On 08.03.17 at 16:33, <yu.c.zhang@linux.intel.com> wrote: >>> @@ -197,6 +217,10 @@ static int hvmemul_do_io( >>> * - If the IOREQ_MEM_ACCESS_WRITE flag is not set, treat it >>> * like a normal PIO or MMIO that doesn't have an ioreq >>> * server (i.e., by ignoring it). >>> + * >>> + * - If the accesss is a read, this could be part of a >>> + * read-modify-write instruction, emulate the read so that we >>> + * have it. >> "it" being what here? Grammatically the insn, but we don't care >> about "having" the insn. > > Here "have it" means " to have the value on this address copied in". > Sorry for the inaccurate comment. How about just "emulate the read first"? Sounds okay. >>> @@ -226,6 +250,17 @@ static int hvmemul_do_io( >>> } >>> >>> /* >>> + * This is part of a read-modify-write instruction. >> "is" or "may be"? > > I believe should be "is". > It's the only scenario I can imagine when an read operation(only when > this operation is > also a write one) traps. Otherwise there shall be no VM exit. Even with a racing update to the type? Jan
On 3/13/2017 7:22 PM, Jan Beulich wrote: >>>> On 11.03.17 at 09:42, <yu.c.zhang@linux.intel.com> wrote: >> On 3/10/2017 11:33 PM, Jan Beulich wrote: >>>>>> On 08.03.17 at 16:33, <yu.c.zhang@linux.intel.com> wrote: >>>> @@ -197,6 +217,10 @@ static int hvmemul_do_io( >>>> * - If the IOREQ_MEM_ACCESS_WRITE flag is not set, treat it >>>> * like a normal PIO or MMIO that doesn't have an ioreq >>>> * server (i.e., by ignoring it). >>>> + * >>>> + * - If the accesss is a read, this could be part of a >>>> + * read-modify-write instruction, emulate the read so that we >>>> + * have it. >>> "it" being what here? Grammatically the insn, but we don't care >>> about "having" the insn. >> Here "have it" means " to have the value on this address copied in". >> Sorry for the inaccurate comment. How about just "emulate the read first"? > Sounds okay. > >>>> @@ -226,6 +250,17 @@ static int hvmemul_do_io( >>>> } >>>> >>>> /* >>>> + * This is part of a read-modify-write instruction. >>> "is" or "may be"? >> I believe should be "is". >> It's the only scenario I can imagine when an read operation(only when >> this operation is >> also a write one) traps. Otherwise there shall be no VM exit. > Even with a racing update to the type? Yes. With patch 1/5, there shall be no racing update to the p2m type during this process. Besides, I believe even without patch 1/5, and we allow the racing p2m change, it will only happen on a p2m_ioreq_server page changed to p2m_ram_rw(in such case it is shall only be a write or a read-modify-write op that cause such vm exit). The race will not happen for a p2m_ram_rw page changed to p2m_ioreq_server, because there shall be no vm exit at all. Thanks Yu > Jan > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index fb56f7b..9744dcb 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -94,6 +94,26 @@ static const struct hvm_io_handler null_handler = { .ops = &null_ops }; +static int ioreq_server_read(const struct hvm_io_handler *io_handler, + uint64_t addr, + uint32_t size, + uint64_t *data) +{ + if ( hvm_copy_from_guest_phys(data, addr, size) != HVMCOPY_okay ) + return X86EMUL_UNHANDLEABLE; + + return X86EMUL_OKAY; +} + +static const struct hvm_io_ops ioreq_server_ops = { + .read = ioreq_server_read, + .write = null_write +}; + +static const struct hvm_io_handler ioreq_server_handler = { + .ops = &ioreq_server_ops +}; + static int hvmemul_do_io( bool_t is_mmio, paddr_t addr, unsigned long *reps, unsigned int size, uint8_t dir, bool_t df, bool_t data_is_addr, uintptr_t data) @@ -197,6 +217,10 @@ static int hvmemul_do_io( * - If the IOREQ_MEM_ACCESS_WRITE flag is not set, treat it * like a normal PIO or MMIO that doesn't have an ioreq * server (i.e., by ignoring it). + * + * - If the accesss is a read, this could be part of a + * read-modify-write instruction, emulate the read so that we + * have it. */ struct hvm_ioreq_server *s = NULL; p2m_type_t p2mt = p2m_invalid; @@ -226,6 +250,17 @@ static int hvmemul_do_io( } /* + * This is part of a read-modify-write instruction. + * Emulate the read part so we have the value cached. + */ + if ( dir == IOREQ_READ ) + { + rc = hvm_process_io_intercept(&ioreq_server_handler, &p); + vio->io_req.state = STATE_IOREQ_NONE; + break; + } + + /* * If the IOREQ_MEM_ACCESS_WRITE flag is not set, * we should set s to NULL, and just ignore such * access.