From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Subject: Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration Date: Fri, 15 Mar 2019 11:29:11 +1100 Message-ID: <20190315002911.GV8211@umbus.fritz.box> References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-7-clg@kaod.org> <20190226052429.GC28015@blackberry> <33aeb002-39b2-1015-9923-613781c08fd6@kaod.org> <20190314023210.GL8211@umbus.fritz.box> <5fd4dd85-5bf0-dd95-546b-ddc7a3efdb45@kaod.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IgDDV5QArAYIxtdK" Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org To: =?iso-8859-1?Q?C=E9dric?= Le Goater Return-path: Content-Disposition: inline In-Reply-To: <5fd4dd85-5bf0-dd95-546b-ddc7a3efdb45@kaod.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org --IgDDV5QArAYIxtdK Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 14, 2019 at 08:11:17AM +0100, C=E9dric Le Goater wrote: > On 3/14/19 3:32 AM, David Gibson wrote: > > On Wed, Mar 13, 2019 at 10:40:19AM +0100, C=E9dric Le Goater wrote: > >> On 2/26/19 6:24 AM, Paul Mackerras wrote: > >>> 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. > >>>> > >>>> 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. > >>>> > >>>> 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? > >> > >> The "attribute" qualifier comes from the {get,set,has}_addr methods=20 > >> of the KVM device. But it is not a well chosen name for the group=20 > >> KVM_DEV_XIVE_GRP_EQ_CONFIG. > >> > >> I should be using "eq identifier" and "eq values" or "eq state".=20 > >=20 > > Yeah, that seems clearer. > >=20 > >>> Also, the kvm_ppc_xive_eq is not 64 bits, so the comment above it is > >>> wrong. Maybe you meant "64-byte"? > >> > >> That was a bad copy paste. I have padded the structure to twice the si= ze > >> of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.= =20 > >> I thought that one extra u64 was not enough room for future. > >> > >>> > >>> [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? > >> > >> Ah yes. Theoretically, it should not happen because we only advertise > >> 64K in the DT for the moment. I should at least add a check. So I will= =20 > >> change the helper xive_native_validate_queue_size() to return -EINVAL > >> for other page sizes. > >=20 > > Ok. > >=20 > >> Do you think it would be complex to support XIVE EQs using a page larg= er=20 > >> than the default one on the guest ? > >=20 > > Hm. The queue has to be physically contiguous from the host point of > > view, in order for the XIVE hardware to write to it, doesn't it? If > > so then supporting queues bigger than the guest page size would be > > very difficult. >=20 > The queue is only *one* page. Right, but it's one *host* page, right, which is by nature host physically contiguous. If the guest page size is different a single guest page might not be host physically contiguous. --=20 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 --IgDDV5QArAYIxtdK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlyK8dYACgkQbDjKyiDZ s5LBnRAA4IjL4eX1FugzPg/4jaRuOogHkKlDg+xpItTkSqVcffv+j+hQIY9SSyjY ytKb8GIqJEp5+zYh3s3HSB1+dJtlEcugqnToGbNErKwUnWFhFuAbo224fGsh13ar 1aG4cBfJzb9SzODCO0uGJtUz4lFpjrEKIsSYRcBAEH3YeneSqX0hvDN0PnNptS3x 15nPvKtbiOrTN+DK1UiTVNU80VnvcfuBv1eLDDyDdnLMqkmQaf/7N0KSsnITx3pj 8Y6hJvmlAGbo6Y8NcQG6XjW10raTy68Hp8oaYvYlIvgVM8CYNLECRvxLuohuGd2q Q6y40m8iSNJvcCI8ILzLpmjfjL6nkcb55XgwH3tXASHuuc9wIRFvN5OK6IMwRxT/ 51SxrAd+haDjUIi5WzlvwFXIA/XormHywxti2CjiLfkHYVMVAQ1+Idd2tNWzP9eX OXyMDhiQENW7b0P3WS5TLnP5vCIfwbqqA8gPqtZbnMsnBHLjsAD21NEaRhR6pewc 0eTEEIOlGSZ0ed4wfjC3b0iZvLaymj5DDBMerzcrSND3Pt0GZvEeJLeczZSA0BKG V8NGpWxtHDQiiFMtms9iSkC69eVsuVAXv87czxaQzKV/H7NyPq/AEQ1qMw/TuATX 9Ocn7UnJSMt3tg389L36rZdeqnlIhFJX68lB3WageTf3pI9CWQs= =B6nD -----END PGP SIGNATURE----- --IgDDV5QArAYIxtdK-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson Date: Fri, 15 Mar 2019 00:29:11 +0000 Subject: Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration Message-Id: <20190315002911.GV8211@umbus.fritz.box> MIME-Version: 1 Content-Type: multipart/mixed; boundary="IgDDV5QArAYIxtdK" List-Id: References: <20190222112840.25000-1-clg@kaod.org> <20190222112840.25000-7-clg@kaod.org> <20190226052429.GC28015@blackberry> <33aeb002-39b2-1015-9923-613781c08fd6@kaod.org> <20190314023210.GL8211@umbus.fritz.box> <5fd4dd85-5bf0-dd95-546b-ddc7a3efdb45@kaod.org> In-Reply-To: <5fd4dd85-5bf0-dd95-546b-ddc7a3efdb45@kaod.org> To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org --IgDDV5QArAYIxtdK Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 14, 2019 at 08:11:17AM +0100, C=E9dric Le Goater wrote: > On 3/14/19 3:32 AM, David Gibson wrote: > > On Wed, Mar 13, 2019 at 10:40:19AM +0100, C=E9dric Le Goater wrote: > >> On 2/26/19 6:24 AM, Paul Mackerras wrote: > >>> 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. > >>>> > >>>> 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. > >>>> > >>>> 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? > >> > >> The "attribute" qualifier comes from the {get,set,has}_addr methods=20 > >> of the KVM device. But it is not a well chosen name for the group=20 > >> KVM_DEV_XIVE_GRP_EQ_CONFIG. > >> > >> I should be using "eq identifier" and "eq values" or "eq state".=20 > >=20 > > Yeah, that seems clearer. > >=20 > >>> Also, the kvm_ppc_xive_eq is not 64 bits, so the comment above it is > >>> wrong. Maybe you meant "64-byte"? > >> > >> That was a bad copy paste. I have padded the structure to twice the si= ze > >> of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.= =20 > >> I thought that one extra u64 was not enough room for future. > >> > >>> > >>> [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? > >> > >> Ah yes. Theoretically, it should not happen because we only advertise > >> 64K in the DT for the moment. I should at least add a check. So I will= =20 > >> change the helper xive_native_validate_queue_size() to return -EINVAL > >> for other page sizes. > >=20 > > Ok. > >=20 > >> Do you think it would be complex to support XIVE EQs using a page larg= er=20 > >> than the default one on the guest ? > >=20 > > Hm. The queue has to be physically contiguous from the host point of > > view, in order for the XIVE hardware to write to it, doesn't it? If > > so then supporting queues bigger than the guest page size would be > > very difficult. >=20 > The queue is only *one* page. Right, but it's one *host* page, right, which is by nature host physically contiguous. If the guest page size is different a single guest page might not be host physically contiguous. --=20 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 --IgDDV5QArAYIxtdK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlyK8dYACgkQbDjKyiDZ s5LBnRAA4IjL4eX1FugzPg/4jaRuOogHkKlDg+xpItTkSqVcffv+j+hQIY9SSyjY ytKb8GIqJEp5+zYh3s3HSB1+dJtlEcugqnToGbNErKwUnWFhFuAbo224fGsh13ar 1aG4cBfJzb9SzODCO0uGJtUz4lFpjrEKIsSYRcBAEH3YeneSqX0hvDN0PnNptS3x 15nPvKtbiOrTN+DK1UiTVNU80VnvcfuBv1eLDDyDdnLMqkmQaf/7N0KSsnITx3pj 8Y6hJvmlAGbo6Y8NcQG6XjW10raTy68Hp8oaYvYlIvgVM8CYNLECRvxLuohuGd2q Q6y40m8iSNJvcCI8ILzLpmjfjL6nkcb55XgwH3tXASHuuc9wIRFvN5OK6IMwRxT/ 51SxrAd+haDjUIi5WzlvwFXIA/XormHywxti2CjiLfkHYVMVAQ1+Idd2tNWzP9eX OXyMDhiQENW7b0P3WS5TLnP5vCIfwbqqA8gPqtZbnMsnBHLjsAD21NEaRhR6pewc 0eTEEIOlGSZ0ed4wfjC3b0iZvLaymj5DDBMerzcrSND3Pt0GZvEeJLeczZSA0BKG V8NGpWxtHDQiiFMtms9iSkC69eVsuVAXv87czxaQzKV/H7NyPq/AEQ1qMw/TuATX 9Ocn7UnJSMt3tg389L36rZdeqnlIhFJX68lB3WageTf3pI9CWQs= =B6nD -----END PGP SIGNATURE----- --IgDDV5QArAYIxtdK--