Re: [PATCH] x86/HVM: refine when to send mapcache invalidation request to qemu

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2020 16:55, Hongyan Xia wrote:
> On Thu, 2020-12-10 at 14:37 +0100, Jan Beulich wrote:
>> On 10.12.2020 14:09, Hongyan Xia wrote:
>>> On Mon, 2020-09-28 at 12:44 +0200, Jan Beulich wrote:
>>>> Plus finally there's no point sending the request for the local
>>>> domain
>>>> when the domain acted upon is a different one. If anything that
>>>> domain's
>>>> qemu's mapcache may need invalidating, but it's unclear how
>>>> useful
>>>> this
>>>> would be: That remote domain may not execute hypercalls at all,
>>>> and
>>>> hence may never make it to the point where the request actually
>>>> gets
>>>> issued. I guess the assumption is that such manipulation is not
>>>> supposed
>>>> to happen anymore once the guest has been started?
>>>
>>> I may still want to set the invalidation signal to true even if the
>>> domain acted on is not the local domain. I know the remote domain
>>> may
>>> never reach the point to issue the invalidate, but it sounds to me
>>> that
>>> the problem is not whether we should set the signal but whether we
>>> can
>>> change where the signal is checked to make sure the point of issue
>>> can
>>> be reliably triggered, and the latter can be done in a future
>>> patch.
>>
>> One of Paul's replies was quite helpful here: The main thing to
> 
> Hmm, I seem to not be able to see the whole thread...

This may have been on the thread which had prompted the creation
of this patch.

>> worry about is for the vCPU to not continue running before the
>> invalidation request was signaled (or else, aiui, qemu may serve
>> a subsequent emulation request by the guest incorrectly, because
>> of using the stale mapping). Hence I believe for a non-paused
>> guest remote operations simply cannot be allowed when the may
>> lead to the need for invalidation. Therefore yes, if we assume
>> the guest is paused in such cases, we could drop the "is current"
>> check, but we'd then still need to arrange for actual signaling
>> before the guest gets to run again. I wonder whether
>> handle_hvm_io_completion() (or its caller, hvm_do_resume(),
>> right after that other call) wouldn't be a good place to do so.
> 
> Actually, the existing code must assume that when QEMU is up, the only
> one that manipulates the p2m is the guest itself like you said.

Not sure what "that" you mean existing code must assume, and why
you would outright exclude external p2m manipulation when the
guest is up. At the very least in a hypothetical memory-hot-
unplug scenario manipulation would still necessarily occur from
outside the guest (yet without real need for pausing it). And
obviously mem-sharing and mem-paging are also doing external
manipulations, albeit maybe they pause the guest for every
change.

> If the
> caller is XENMEM_decrease_reservation, the code does not even check
> which p2m this is for and unconditionally sets the QEMU invalidate flag
> for the current domain.

This observation is part of what had prompted the change here.

> Athough this assumption may simply be wrong
> now, so I agree care should be taken for remote p2m ops (I may need to
> read the code more to know how this should be done).

I believe the assumption stems from the time where the controlling
domain would necessarily be PV, and hence a decrease-reservation
request by a HVM domain could only have been for itself.

Jan