From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paul Mackerras <paulus@ozlabs.org>
Date: Tue, 26 Feb 2019 05:24:29 +0000
Subject: Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
Message-Id: <20190226052429.GC28015@blackberry>
List-Id: <kvm-ppc.vger.kernel.org>
References: <20190222112840.25000-1-clg@kaod.org>
 <20190222112840.25000-7-clg@kaod.org>
In-Reply-To: <20190222112840.25000-7-clg@kaod.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
To: =?iso-8859-1?Q?C=E9dric?= Le Goater <clg@kaod.org>
Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, David Gibson <david@gibson.dropbear.id.au>

On Fri, Feb 22, 2019 at 12:28:30PM +0100, C=E9dric Le Goater wrote:
> These controls will be used by the H_INT_SET_QUEUE_CONFIG and
> H_INT_GET_QUEUE_CONFIG hcalls from QEMU. They will also be used to
> restore the configuration of the XIVE EQs in the KVM device and to
> capture the internal runtime state of the EQs. Both 'get' and 'set'
> rely on an OPAL call to access from the XIVE interrupt controller the
> EQ toggle bit and EQ index which are updated by the HW when event
> notifications are enqueued in the EQ.
>=20
> The value of the guest physical address of the event queue is saved in
> the XIVE internal xive_q structure for later use. That is when
> migration needs to mark the EQ pages dirty to capture a consistent
> memory state of the VM.
>=20
> To be noted that H_INT_SET_QUEUE_CONFIG does not require the extra
> OPAL call setting the EQ toggle bit and EQ index to configure the EQ,
> but restoring the EQ state will.

[snip]

> +/* Layout of 64-bit eq attribute */
> +#define KVM_XIVE_EQ_PRIORITY_SHIFT	0
> +#define KVM_XIVE_EQ_PRIORITY_MASK	0x7
> +#define KVM_XIVE_EQ_SERVER_SHIFT	3
> +#define KVM_XIVE_EQ_SERVER_MASK		0xfffffff8ULL
> +
> +/* Layout of 64-bit eq attribute values */
> +struct kvm_ppc_xive_eq {
> +	__u32 flags;
> +	__u32 qsize;
> +	__u64 qpage;
> +	__u32 qtoggle;
> +	__u32 qindex;
> +	__u8  pad[40];
> +};

This is confusing.  What's the difference between an "eq attribute"
and an "eq attribute value"?  Is the first actually a queue index or
a queue identifier?

Also, the kvm_ppc_xive_eq is not 64 bits, so the comment above it is
wrong.  Maybe you meant "64-byte"?

[snip]

> +	page =3D gfn_to_page(kvm, gpa_to_gfn(kvm_eq.qpage));
> +	if (is_error_page(page)) {
> +		pr_warn("Couldn't get guest page for %llx!\n", kvm_eq.qpage);
> +		return -ENOMEM;
> +	}
> +	qaddr =3D page_to_virt(page) + (kvm_eq.qpage & ~PAGE_MASK);

Isn't this assuming that we can map the whole queue with a single
gfn_to_page?  That would only be true if kvm_eq.qsize <=3D PAGE_SHIFT.
What happens if kvm_eq.qsize > PAGE_SHIFT?

Paul.