From: Paul Durrant <Paul.Durrant@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>,
"Keir (Xen.org)" <keir@xen.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io()
Date: Wed, 17 Jun 2015 13:54:58 +0000 [thread overview]
Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD0259531E4@AMSPEX01CL01.citrite.net> (raw)
In-Reply-To: <558192CF02000078000862CD@mail.emea.novell.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 17 June 2015 14:31
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io()
>
> >>> On 11.06.15 at 17:42, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -51,41 +51,23 @@ static void hvmtrace_io_assist(int is_mmio, ioreq_t
> *p)
> > }
> >
> > static int hvmemul_do_io(
> > - int is_mmio, paddr_t addr, unsigned long *reps, int size,
> > - paddr_t ram_gpa, int dir, int df, void *p_data)
> > + int is_mmio, paddr_t addr, unsigned long *reps, unsigned int size,
> > + uint8_t dir, bool_t df, bool_t data_is_addr, uint64_t data)
>
> is_mmio surely can become bool_t too?
>
Yes, it can.
> > {
> > struct vcpu *curr = current;
> > - struct hvm_vcpu_io *vio;
> > + struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
> > ioreq_t p = {
> > .type = is_mmio ? IOREQ_TYPE_COPY : IOREQ_TYPE_PIO,
> > .addr = addr,
> > .size = size,
> > .dir = dir,
> > .df = df,
> > - .data = ram_gpa,
> > - .data_is_ptr = (p_data == NULL),
> > + .data = data,
> > + .data_is_ptr = data_is_addr, /* ioreq_t field name is misleading */
> > };
> > - unsigned long ram_gfn = paddr_to_pfn(ram_gpa);
> > - p2m_type_t p2mt;
> > - struct page_info *ram_page;
> > + void *p_data = (void *)data;
>
> While presumably compiling fine, casting uint64_t to a pointer looks
> bogus without an intermediate cast to (unsigned) long or (u)intptr_t.
>
Ok. I may just change the arg to the function to a untptr_t then.
> > @@ -190,7 +141,12 @@ static int hvmemul_do_io(
> > p.count = *reps;
> >
> > if ( dir == IOREQ_WRITE )
> > + {
> > + if ( !data_is_addr )
> > + memcpy(&p.data, p_data, size);
> > +
> > hvmtrace_io_assist(is_mmio, &p);
> > + }
>
> Why would you need to do this _only_ here (i.e. either not at all,
> or not just for tracing purposes)? Or is this just to _restore_ the
> original value for tracing (in which case the question would be
> whether it indeed can get modified)?
Not sure I follow. If hvmemul_do_io is called with !data_is_addr then the data arg will be a pointer to a buffer, rather than a gpa. So, the gpa that was first copied into the ioreq (i.e. p) needs to be overwritten with the actual data if it's a write.
>
> > -int hvmemul_do_pio(
> > - unsigned long port, unsigned long *reps, int size,
> > - paddr_t ram_gpa, int dir, int df, void *p_data)
> > +int hvmemul_do_io_buffer(
>
> static?
Ok.
>
> > + bool_t is_mmio, paddr_t addr, unsigned long *reps, unsigned int size,
> > + uint8_t dir, bool_t df, uint8_t *buffer)
> > {
> > - return hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data);
> > + int rc;
> > +
> > + BUG_ON(buffer == NULL);
> > +
> > + rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 0,
> > + (uint64_t)buffer);
>
> Same remark regarding a cast between uint64_t and a pointer.
>
Ok.
> > +static int hvmemul_acquire_page(unsigned long gmfn, struct page_info
> **page)
> > +{
> > + struct vcpu *v = current;
> > + struct domain *d = v->domain;
>
> curr and currd please (albeit I don't see the need for the former as
> a local variable).
>
Ok.
> > +static void hvmemul_release_page(struct page_info *page)
>
> inline?
>
Probably, but I was hoping the compiler would make that decision.
> > +int hvmemul_do_io_addr(
>
> static?
>
Ok.
> > + bool_t is_mmio, paddr_t addr, unsigned long *reps,
> > + unsigned int size, uint8_t dir, bool_t df, paddr_t ram_gpa)
> > +{
> > + struct page_info *ram_page;
> > + int rc;
> > +
> > + rc = hvmemul_acquire_page(paddr_to_pfn(ram_gpa), &ram_page);
> > + if ( rc != X86EMUL_OKAY )
> > + return rc;
> > +
> > + rc = hvmemul_do_io(is_mmio, addr, reps, size, dir, df, 1,
> > + (uint64_t)ram_gpa);
>
> Pointless cast.
>
Yes, I guess.
> > +/*
> > + * Perform I/O between <port> and <buffer>. <dir> indicates the
> > + * direction: IOREQ_READ means a read from <port> to <buffer> and
> > + * IOREQ_WRITE means a write from <buffer> to <port>. Each access has
> > + * width <size> and up to *<reps> accesses will be performed. If
> > + * X86EMUL_OKAY is returned then <reps> will be updated with the
> number
> > + * of accesses actually performed.
> > + *
> > + * NOTE: If *<reps> is greater than 1, each access will use the
> > + * <buffer> pointer; there is no implicit interation over a
> > + * block of memory starting at <buffer>.
> > + */
> > +int hvmemul_do_pio_buffer(uint16_t port,
> > + unsigned long *reps,
>
> Considering the comment - can't you instead drop the reps parameter
> here then?
>
No. A rep stos does multiple port writes from the same buffer pointer.
> > @@ -1142,7 +1236,8 @@ static int hvmemul_read_io(
> > {
> > unsigned long reps = 1;
> > *val = 0;
> > - return hvmemul_do_pio(port, &reps, bytes, 0, IOREQ_READ, 0, val);
> > + return hvmemul_do_pio_buffer(port, &reps, bytes, IOREQ_READ,
> > + (uint8_t *)val);
> > }
> >
> > static int hvmemul_write_io(
> > @@ -1152,7 +1247,8 @@ static int hvmemul_write_io(
> > struct x86_emulate_ctxt *ctxt)
> > {
> > unsigned long reps = 1;
> > - return hvmemul_do_pio(port, &reps, bytes, 0, IOREQ_WRITE, 0, &val);
> > + return hvmemul_do_pio_buffer(port, &reps, bytes, IOREQ_WRITE,
> > + (uint8_t *)&val);
>
> To avoid such bogus casts, please have hvmemul_do_pio_buffer()
> take void * instead.
>
Ok.
Paul
> Jan
next prev parent reply other threads:[~2015-06-17 13:55 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-11 15:42 [PATCH v2 00/17] x86/hvm: I/O emulation cleanup and fix Paul Durrant
2015-06-11 15:42 ` [PATCH v2 01/17] x86/hvm: simplify hvmemul_do_io() Paul Durrant
2015-06-17 13:31 ` Jan Beulich
2015-06-17 13:54 ` Paul Durrant [this message]
2015-06-17 14:47 ` Jan Beulich
2015-06-17 14:55 ` Paul Durrant
2015-06-17 14:59 ` Jan Beulich
2015-06-11 15:42 ` [PATCH v2 02/17] x86/hvm: re-name struct hvm_mmio_handler to hvm_mmio_ops Paul Durrant
2015-06-17 12:43 ` Jan Beulich
2015-06-17 12:45 ` Paul Durrant
2015-06-11 15:42 ` [PATCH v2 03/17] x86/hvm: unify internal portio and mmio intercepts Paul Durrant
2015-06-17 14:22 ` Jan Beulich
2015-06-17 14:40 ` Paul Durrant
2015-06-17 14:55 ` Jan Beulich
2015-06-11 15:42 ` [PATCH v2 04/17] x86/hvm: unify dpci portio intercept with standard portio intercept Paul Durrant
2015-06-17 14:36 ` Jan Beulich
2015-06-17 14:46 ` Paul Durrant
2015-06-17 14:58 ` Jan Beulich
2015-06-17 15:17 ` Paul Durrant
2015-06-11 15:42 ` [PATCH v2 05/17] x86/hvm: unify stdvga mmio intercept with standard mmio intercept Paul Durrant
2015-06-17 15:50 ` Jan Beulich
2015-06-17 16:30 ` Paul Durrant
2015-06-18 6:23 ` Jan Beulich
2015-06-11 15:42 ` [PATCH v2 06/17] x86/hvm: revert 82ed8716b "fix direct PCI port I/O emulation retry Paul Durrant
2015-06-17 10:54 ` Paul Durrant
2015-06-11 15:42 ` [PATCH v2 07/17] x86/hvm: only call hvm_io_assist() from hvm_wait_for_io() Paul Durrant
2015-06-11 15:42 ` [PATCH v2 08/17] x86/hvm: split I/O completion handling from state model Paul Durrant
2015-06-11 15:42 ` [PATCH v2 09/17] x86/hvm: remove hvm_io_pending() check in hvmemul_do_io() Paul Durrant
2015-06-11 15:42 ` [PATCH v2 10/17] x86/hvm: remove HVMIO_dispatched I/O state Paul Durrant
2015-06-11 15:42 ` [PATCH v2 11/17] x86/hvm: remove hvm_io_state enumeration Paul Durrant
2015-06-11 15:42 ` [PATCH v2 12/17] x86/hvm: use ioreq_t to track in-flight state Paul Durrant
2015-06-11 15:42 ` [PATCH v2 13/17] x86/hvm: only acquire RAM pages for emulation when we need to Paul Durrant
2015-06-11 15:42 ` [PATCH v2 14/17] x86/hvm: remove extraneous parameter from hvmtrace_io_assist() Paul Durrant
2015-06-11 15:42 ` [PATCH v2 15/17] x86/hvm: make sure translated MMIO reads or writes fall within a page Paul Durrant
2015-06-11 15:43 ` [PATCH v2 16/17] x86/hvm: remove multiple open coded 'chunking' loops Paul Durrant
2015-06-11 15:43 ` [PATCH v2 17/17] x86/hvm: track large memory mapped accesses by buffer offset Paul Durrant
2015-06-12 10:44 ` [PATCH v2 00/17] x86/hvm: I/O emulation cleanup and fix Fabio Fantoni
2015-06-12 11:45 ` Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9AAE0902D5BC7E449B7C8E4E778ABCD0259531E4@AMSPEX01CL01.citrite.net \
--to=paul.durrant@citrix.com \
--cc=Andrew.Cooper3@citrix.com \
--cc=JBeulich@suse.com \
--cc=keir@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.