Re: [Xen-devel] [PATCH] x86/hvm: finish IOREQ correctly on completion path

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.03.19 at 12:09, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 11 March 2019 11:04
>> 
>> >>> On 11.03.19 at 11:30, <Paul.Durrant@xxxxxxxxxx> wrote:
>> >> From: Igor Druzhinin [mailto:igor.druzhinin@xxxxxxxxxx]
>> >> Sent: 08 March 2019 21:31
>> >>
>> >> --- a/xen/arch/x86/hvm/emulate.c
>> >> +++ b/xen/arch/x86/hvm/emulate.c
>> >> @@ -1080,7 +1080,15 @@ static int linear_read(unsigned long addr, 
>> >> unsigned int bytes, void *p_data,
>> >>                         uint32_t pfec, struct hvm_emulate_ctxt 
>> >> *hvmemul_ctxt)
>> >>  {
>> >>      pagefault_info_t pfinfo;
>> >> -    int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, 
>> >> &pfinfo);
>> >> +    const struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
>> >> +    int rc = HVMTRANS_bad_gfn_to_mfn;
>> >> +
>> >> +    /*
>> >> +     * If the memory access can be handled immediately - do it,
>> >> +     * otherwise re-enter ioreq completion path to properly consume it.
>> >> +     */
>> >> +    if ( !hvm_ioreq_needs_completion(&vio->io_req) )
>> >> +        rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, 
>> >> &pfinfo);
>> >
>> > I think this is the right thing to do
>> 
>> It's not, and that's why I had written that earlier explanation which
>> you then responded to saying that the issue needs to be dealt with
>> by enforcing a consistent view of MMIO (or not) during initial try and
>> replay. That's _not_ what the above does in the general case: It
>> simply forces _all_ accesses into the slow path, thus re-introducing
>> the problem of page straddling accesses not getting routed correctly.
> 
> Why? If there is no pending ioreq then why would the call to 
> hvm_copy_from_guest_linear() not happen? AFAICT vio->io_req will only be 
> updated when hvmemul_do_io() issues a new ioreq, so the test appears correct. 
> How is that _all_ access fail this test?

"All" was too heavy, as per this discussion:

>> Even worse, it forces _all_ memory accesses by the insn under
>> emulation into the MMIO path. While this would happen to be okay
>> for a PUSH from MMIO (because the read comes first, and hence the
>> write would no longer see a pending IOREQ), it's wrong for (among
>> many other cases) a POP to MMIO, as the read (from stack, i.e. RAM)
>> will be replayed first, while the IOREQ is still marked incomplete. I'd
>> expect this to trigger the very domain_crash() in hvmemul_do_io()
>> that was also triggering because of the P2M type change behind our
>> backs.

I should have said "all accesses preceding the one really accessing
MMIO". Using the provided example of POP, the linear_read() invocation
during replay (to read the stack) will find a pending IOREQ, and wrongly
go the MMIO path. This would, in this example, be correct only for
linear_write() to do. So the suggested change is correct only for any
insn accessing no more than one memory location (if there's no memory
access then of course we won't make it here in the first place).

Jan



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