Re: [Xen-devel] [PATCH 1/3] x86/HVM: __hvm_copy() should not write to p2m_ioreq_server pages

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 13 November 2018 10:47
> To: Jan Beulich <JBeulich@xxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> p2m_ioreq_server pages
> 
> On 13/11/18 10:13, Jan Beulich wrote:
> > Commit 3bdec530a5 ("x86/HVM: split page straddling emulated accesses in
> > more cases") introduced a hvm_copy_to_guest_linear() attempt before
> > falling back to hvmemul_linear_mmio_write(). This is wrong for the
> > p2m_ioreq_server special case. That change widened a pre-existing issue
> > though: Other writes to such pages also need to be failed (or forced
> > through emulation), in particular hypercall buffer writes.
> >
> > Reported-by: ??? <???@citrix.com>
> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3202,6 +3202,12 @@ static enum hvm_translation_result __hvm
> >          if ( res != HVMTRANS_okay )
> >              return res;
> >
> > +        if ( (flags & HVMCOPY_to_guest) && p2mt == p2m_ioreq_server )
> 
> While this does address the issue, I'm concerned about hardcoding the
> behaviour here.
> 
> p2m_ioreq_server doesn't mean "I want shadowing properties". It has an
> as-yet unspecified per-ioreq-client meaning.
> 
> We either want to rename p2m_ioreq_server to something which indicates
> its "allow-reads/emulate writes" behaviour, or design a way for the
> ioreq client to specify the behaviour it wants.
> 

The comment in the public header is:

/*                                                                           
 * XEN_DMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id>   
                                                                         
 *                                      to specific memory type <type>       
 *                                      for specific accesses <flags>        
 *                                                                           
 * For now, flags only accept the value of XEN_DMOP_IOREQ_MEM_ACCESS_WRITE,  
 * which means only write operations are to be forwarded to an ioreq server. 
 * Support for the emulation of read operations can be added when an ioreq   
 * server has such requirement in future.                                    
 */

...so the write-intercept-only behaviour is baked in. Whilst I agree it would 
be nice not to proliferate this, I don't think it needs addressing in the short 
term.

  Paul

> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel