On Fri, Dec 11, 2020 at 04:27:44PM +1100, Paul Mackerras wrote:

On Fri, Dec 11, 2020 at 12:16:39PM +1100, David Gibson wrote:

quoted

On Thu, Dec 10, 2020 at 09:54:18AM +0530, Bharata B Rao wrote:

quoted

On Wed, Dec 09, 2020 at 03:15:42PM +1100, Paul Mackerras wrote:

quoted

On Mon, Oct 19, 2020 at 04:56:41PM +0530, Bharata B Rao wrote:

quoted

Implements H_RPT_INVALIDATE hcall and supports only nested case
currently.

A KVM capability KVM_CAP_RPT_INVALIDATE is added to indicate the
support for this hcall.

I have a couple of questions about this patch:

1. Is this something that is useful today, or is it something that may
become useful in the future depending on future product plans? In
other words, what advantage is there to forcing L2 guests to use this
hcall instead of doing tlbie themselves?

H_RPT_INVALIDATE will replace the use of the existing H_TLB_INVALIDATE
for nested partition scoped invalidations. Implementations that want to
off-load invalidations to the host (when GTSE=0) would have to bother
about only one hcall (H_RPT_INVALIDATE)

quoted

2. Why does it need to be added to the default-enabled hcall list?

There is a concern that if this is enabled by default we could get the
situation where a guest using it gets migrated to a host that doesn't
support it, which would be bad.  That is the reason that all new
things like this are disabled by default and only enabled by userspace
(i.e. QEMU) in situations where we can enforce that it is available on
all hosts to which the VM might be migrated.

As you suggested privately, I am thinking of falling back to
H_TLB_INVALIDATE in case where this new hcall fails due to not being
present. That should address the migration case that you mention
above. With that and leaving the new hcall enabled by default
is good okay?

No.  Assuming that guests will have some fallback is not how the qemu
migration compatibility model works.  If we specify an old machine
type, we need to provide the same environment that the older host
would have.

I misunderstood what this patchset is about when I first looked at
it.  H_RPT_INVALIDATE has two separate functions; one is to do
process-scoped invalidations for a guest when LPCR[GTSE] = 0 (i.e.,
when the guest is not permitted to do tlbie itself), and the other is
to do partition-scoped invalidations that an L1 hypervisor needs to do
on behalf of an L2 guest.  The second function is a replacement and
standardization of the existing H_TLB_INVALIDATE which was introduced
with the nested virtualization code (using a hypercall number from the
platform-specific range).

This patchset only implements the second function, not the first.  The
first function remains unimplemented in KVM at present.

Given that QEMU will need changes for a guest to be able to exploit
H_RPT_INVALIDATE (at a minimum, adding a device tree property), it
doesn't seem onerous for QEMU to have to enable the hcall with
KVM_CAP_PPC_ENABLE_HCALL.  I think that the control on whether the
hcall is handled in KVM along with the control on nested hypervisor
function provides adequate control for QEMU without needing a writable
capability.  The read-only capability to say whether the hcall exists
does seem useful.

Given all that, I'm veering towards taking Bharata's patchset pretty
much as-is, minus the addition of H_RPT_INVALIDATE to the
default-enabled set.

Yes, that's fine.  I was only the suggestion that it be on the
default-enabled set I was objecting to.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson