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 05/17] x86/hvm: unify stdvga mmio intercept with standard mmio intercept
Date: Wed, 17 Jun 2015 16:30:53 +0000 [thread overview]
Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD025953A73@AMSPEX01CL01.citrite.net> (raw)
In-Reply-To: <5581B38202000078000864B3@mail.emea.novell.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 17 June 2015 16:51
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH v2 05/17] x86/hvm: unify stdvga mmio intercept with
> standard mmio intercept
>
> >>> On 11.06.15 at 17:42, <paul.durrant@citrix.com> wrote:
> > It's clear from the following check in hvmemul_rep_movs:
> >
> > if ( sp2mt == p2m_mmio_direct || dp2mt == p2m_mmio_direct ||
> > (sp2mt == p2m_mmio_dm && dp2mt == p2m_mmio_dm) )
> > return X86EMUL_UNHANDLEABLE;
> >
> > that mmio <-> mmio copy is not handled. This means the code in the
> > stdvga mmio intercept that explicitly handles mmio <-> mmio copy when
> > hvm_copy_to/from_guest_phys() fails is never going to be executed.
>
> Looking at the code I agree with the analysis, but I doubt the code
> was put there originally just for fun, and I wonder whether some of
> the slowness we "gained" over the years in boot loader graphics
> performance isn't related to this code having got hooked off. And in
> the end we should get to the point where MMIO -> MMIO transfers
> get emulated efficiently anyway, so I wonder whether this patch
> moves us in the right direction.
>
I think removing the stdvga code is the right thing to so. If we want to handle MMIO <-> MMIO then this needs to be done in a generic fashon. Given that (upstream) QEMU translates the 'data_is_addr' addresses through its memory map as well as the actual ioreq addr values I believe it will actually do MMIO <-> MMIO moves as it stands, so it's probably only Xen's code that needs modifying and I think this can be done fairly easily after this series. I'll see if I can come up with something.
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -273,6 +273,15 @@ static int hvmemul_acquire_page(unsigned long
> gmfn, struct page_info **page)
> > return X86EMUL_RETRY;
> > }
> >
> > + /* This code should not be reached if the gmfn is not RAM */
> > + if ( p2m_is_mmio(p2mt) )
> > + {
> > + domain_crash(d);
>
> Log a message at least? Silent domain death is rather ugly to analyze.
You do get a file+line (since domain_crash is actually a macro with a printk in it too) but I can also log something.
>
> > --- a/xen/arch/x86/hvm/stdvga.c
> > +++ b/xen/arch/x86/hvm/stdvga.c
> > @@ -275,7 +275,8 @@ static uint8_t stdvga_mem_readb(uint64_t addr)
> > return ret;
> > }
> >
> > -static uint64_t stdvga_mem_read(uint64_t addr, uint64_t size)
> > +static int stdvga_mem_read(struct vcpu *v, unsigned long addr,
> > + unsigned long size, unsigned long *p_data)
> > {
> > uint64_t data = 0;
> >
> > @@ -313,7 +314,9 @@ static uint64_t stdvga_mem_read(uint64_t addr,
> uint64_t size)
> > break;
> > }
> >
> > - return data;
> > + *p_data = data;
>
> Even if sizeof(long) == sizeof(uint64_t), please try to use
> consistent types (i.e. the new function parameter should be of
> uint64_t * type).
>
Ok.
> > + if (!hvm_buffered_io_send(&p))
> > + return X86EMUL_UNHANDLEABLE;
>
> Coding style.
>
True.
> > +static int stdvga_mem_check(struct vcpu *v, unsigned long addr)
> > {
> > + struct hvm_hw_stdvga *s = &v->domain->arch.hvm_domain.stdvga;
> > + int rc;
> >
> > spin_lock(&s->lock);
> >
> > + rc = s->stdvga && s->cache &&
> > + (addr >= VGA_MEM_BASE) &&
> > + (addr < (VGA_MEM_BASE + VGA_MEM_SIZE));
>
> This (compared with the code being removed) makes clear that the
> check() mechanism lacks the size (it did so before, but with your
> generalization I think this needs to be corrected).
>
> > +const struct hvm_mmio_ops stdvga_mem_ops = {
>
> static
>
Ok.
Paul
> Jan
next prev parent reply other threads:[~2015-06-17 16:30 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
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 [this message]
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=9AAE0902D5BC7E449B7C8E4E778ABCD025953A73@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.