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: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 13 November 2018 11:15
> To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant
> <Paul.Durrant@xxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 1/3] x86/HVM: __hvm_copy() should not write to
> p2m_ioreq_server pages
> 
> >>> On 13.11.18 at 12:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 13/11/2018 10:53, Paul Durrant wrote:
> >>> -----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.
> >
> > Lovely :(
> 
> Wasn't the rationale back then that we'd add further p2m types if we
> needed new distinct behavior (and in particular accept flags other than
> the single form currently accepted)?
> 

Yes, the idea was to have page-to-type map so that we could have dedicate types 
for each ioreq server and behaviour.

  Paul

> Jan
> 


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