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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

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.

Together with Andrew's pointing out of the ballooning case as
another situation where machine state changes behind our backs
during insn emulation, I meanwhile think that we really need to
lock out machine state changes while insn emulation is in flight for
one of the guest's vCPU-s. As this may be rather bad for
performance, we'd need to strive to make the lock out window
as small as possible. Otoh my hope would be that (in the common
case) there aren't too frequent machine state changes in the first
place.

Jan



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