Re: [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common

[Oleksandr <olekstysh@gmail.com>]

On 16.09.20 11:50, Jan Beulich wrote:

Hi Jan, Roger

> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1490,6 +1490,12 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
>>       /* Ensure the hypercall trap instruction is re-executed. */
>>       if ( current->hcall_preempted )
>>           regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
>> +
>> +#ifdef CONFIG_IOREQ_SERVER
>> +    if ( unlikely(current->domain->qemu_mapcache_invalidate) &&
>> +         test_and_clear_bool(current->domain->qemu_mapcache_invalidate) )
>> +        send_invalidate_req();
>> +#endif
>>   }
> There's a lot of uses of "current" here now, and these don't look to
> exactly be cheap on Arm either (they aren't on x86), so I wonder
> whether this is the point where at least "current" wants latching
> into a local variable here.

Sounds reasonable, will use local variable


>
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -18,8 +18,10 @@
>>    *
>>    * Copyright (c) 2017 Citrix Systems Ltd.
>>    */
>> +
>>   #include <xen/lib.h>
>>   #include <xen/hypercall.h>
>> +#include <xen/ioreq.h>
>>   #include <xen/nospec.h>
> While I don't care much about the presence of absence of the blank
> line between head comment and #include-s, I don't see why you add
> one here.

Accidentally), will remove.


>
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -1651,6 +1651,11 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>           break;
>>       }
>>   
>> +#ifdef CONFIG_IOREQ_SERVER
>> +    if ( op == XENMEM_decrease_reservation )
>> +        curr_d->qemu_mapcache_invalidate = true;
>> +#endif
> I don't see why you put this right into decrease_reservation(). This
> isn't just to avoid the extra conditional, but first and foremost to
> avoid bypassing the earlier return from the function (in the case of
> preemption). In the context of this I wonder whether the ordering of
> operations in hvm_hypercall() is actually correct.
Good point, indeed we may return earlier in case of preemption, I missed 
that.
Will move it to decrease_reservation(). But, we may return even earlier 
in case of error...
Now I am wondering should we move it to the very beginning of command 
processing or not?
AFAIU before this patch qemu_mapcache_invalidate was always set in 
hvm_memory_op() if XENMEM_decrease_reservation came
despite of possible errors in the command processing.


> I'm also unconvinced curr_d is the right domain in all cases here;
> while this may be a pre-existing issue in principle, I'm afraid it
> gets more pronounced by the logic getting moved to common code.

Sorry I didn't get your concern here.


> Roger - thoughts either way with, in particular, PVH Dom0 in mind?

>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -97,6 +97,8 @@ static inline bool hvm_ioreq_needs_completion(const ioreq_t *ioreq)
>>              (ioreq->type != IOREQ_TYPE_PIO || ioreq->dir != IOREQ_WRITE);
>>   }
>>   
>> +void send_invalidate_req(void);
> Perhaps rename to ioreq_send_invalidate(), ioreq_send_invalidate_req(),
> or send_invalidate_ioreq() at this occasion?

I would prefer function with ioreq_ prefix.


>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -512,6 +512,8 @@ struct domain
>>       /* Argo interdomain communication support */
>>       struct argo_domain *argo;
>>   #endif
>> +
>> +    bool_t qemu_mapcache_invalidate;
> "bool" please.

ok

-- 
Regards,

Oleksandr Tyshchenko