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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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)?

Jan



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