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

[Oleksandr <olekstysh@gmail.com>]

On 25.09.20 10:03, Jan Beulich wrote:

Hi Jan.

> On 24.09.2020 18:45, Oleksandr wrote:
>> On 24.09.20 14:16, Jan Beulich wrote:
>>
>> Hi Jan
>>
>>> On 22.09.2020 21:32, Oleksandr wrote:
>>>> On 16.09.20 11:50, Jan Beulich wrote:
>>>>> On 10.09.2020 22:22, Oleksandr Tyshchenko wrote:
>>>>>> --- 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?
>>> In _this_ series I'd strongly recommend you keep things working as
>>> they are. If independently you think you've found a reason to
>>> re-order certain operations, then feel free to send a patch with
>>> suitable justification.
>> Of course, I will try to retain current behavior.
>>
>>
>>>>> 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.
>>> Well, you need to be concerned whose qemu_mapcache_invalidate flag
>>> you set.
>> May I ask, in what cases the *curr_d* is the right domain?
> When a domain does a decrease-reservation on itself. I thought
> that's obvious. But perhaps your question was rather meant a to
> whether a->domain ever is _not_ the right one?
No, my question was about *curr_d*. I saw your answer
 > I'm also unconvinced curr_d is the right domain in all cases here;
and just wanted to clarify these cases. Sorry if I was unclear.


>
>> We need to make sure that domain is using IOREQ server(s) at least.
>> Hopefully, we have a helper for this
>> which is hvm_domain_has_ioreq_server(). Please clarify, anything else I
>> should taking care of?
> Nothing I can recall / think of right now, except that the change
> may want to come under a different title and with a different
> description.As indicated, I don't think this is correct for PVH
> Dom0 issuing the request against a HVM DomU, and addressing this
> will likely want this moved out of hvm_memory_op() anyway. Of
> course an option is to split this into two patches - the proposed
> bug fix (perhaps wanting backporting) and then the moving of the
> field out of arch.hvm. If you feel uneasy about the bug fix part,
> let me know and I (or maybe Roger) will see to put together a
> patch.

Thank you for the clarification.

Yes, it would be really nice if you (or maybe Roger) could create a 
patch for the bug fix part.

-- 
Regards,

Oleksandr Tyshchenko