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

From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Julien Grall" <julien@xen.org>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <ian.jackson@eu.citrix.com>, "Wei Liu" <wl@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Julien Grall" <julien.grall@arm.com>
Subject: Re: [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common
Date: Fri, 2 Oct 2020 12:55:45 +0300	[thread overview]
Message-ID: <0aa9a225-1231-fa98-f2a1-caf898a3ed86@gmail.com> (raw)
In-Reply-To: <47ecdde7-6575-bee8-7981-7b1a31715a0b@gmail.com>

Hi Jan

On 25.09.20 16:05, Oleksandr wrote:
>
> 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.

Thank you for your patch [1].

If I got it correctly there won't be a suitable common place where to 
set qemu_mapcache_invalidate flag anymore
as XENMEM_decrease_reservation is not a single place we need to make a 
decision whether to set it
By principle of analogy, on Arm we probably want to do so in 
guest_physmap_remove_page (or maybe better in p2m_remove_mapping).
Julien, what do you think?

I will modify current patch to not alter the common code.

[1] https://patchwork.kernel.org/patch/11803383/

-- 

Regards,

Oleksandr Tyshchenko