Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Thu, Mar 14, 2019 at 08:11:17AM +0100, Cédric Le Goater wrote:
> On 3/14/19 3:32 AM, David Gibson wrote:
> > On Wed, Mar 13, 2019 at 10:40:19AM +0100, Cédric Le Goater wrote:
> >> On 2/26/19 6:24 AM, Paul Mackerras wrote:
> >>> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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_SHIFT3
> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
> +
> +/* 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
> >> of the KVM device. But it is not a well chosen name for the group
> >> KVM_DEV_XIVE_GRP_EQ_CONFIG.
> >>
> >> I should be using "eq identifier" and "eq values" or "eq state".
> >
> > Yeah, that seems clearer.
> >
> >>> 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 size
> >> of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.
> >> I thought that one extra u64 was not enough room for future.
> >>
> >>>
> >>> [snip]
> >>>
> +page = 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 = 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 <= 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
> >> change the helper xive_native_validate_queue_size() to return -EINVAL
> >> for other page sizes.
> >
> > Ok.
> >
> >> Do you think it would be complex to support XIVE EQs using a page larger
> >> than the default one on the guest ?
> >
> > 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.
>
> 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.
--
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
signature.asc
Description: PGP signature
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On 3/14/19 3:32 AM, David Gibson wrote:
> On Wed, Mar 13, 2019 at 10:40:19AM +0100, Cédric Le Goater wrote:
>> On 2/26/19 6:24 AM, Paul Mackerras wrote:
>>> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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_SHIFT0
+#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
+#define KVM_XIVE_EQ_SERVER_SHIFT 3
+#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
+
+/* 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
>> of the KVM device. But it is not a well chosen name for the group
>> KVM_DEV_XIVE_GRP_EQ_CONFIG.
>>
>> I should be using "eq identifier" and "eq values" or "eq state".
>
> Yeah, that seems clearer.
>
>>> 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 size
>> of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.
>> I thought that one extra u64 was not enough room for future.
>>
>>>
>>> [snip]
>>>
+ page = 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 = 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 <= 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
>> change the helper xive_native_validate_queue_size() to return -EINVAL
>> for other page sizes.
>
> Ok.
>
>> Do you think it would be complex to support XIVE EQs using a page larger
>> than the default one on the guest ?
>
> 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.
The queue is only *one* page.
C.
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Wed, Mar 13, 2019 at 09:46:08AM +0100, Cédric Le Goater wrote: > On 3/13/19 5:03 AM, David Gibson wrote: > > On Tue, Mar 12, 2019 at 06:00:38PM +0100, Cédric Le Goater wrote: > >> On 2/25/19 3:39 AM, David Gibson wrote: > >>> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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. > >> > >> I think we need to add some kind of flags to differentiate the hcall > >> H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does > >> not need OPAL support call and this could help in the code > >> transition. > > > > Hrm. What's the actual difference in the semantics between the two > > cases. > > None. > > But we don't need to set the EQ state in the case of the HCALL and it's > (very) practical to run guests with XIVE enabled without the OPAL support. > The latter is the main reason clearly. > > Thinking of it, I could test the EQ toggle bit and index passed to KVM > and skip the OPAL call which restores the EQ state if they are zero. > This is because I know that the OPAL call configuring the EQ resets them. > > That will do. No need for a flag. That's a much better idea. -- 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 signature.asc Description: PGP signature
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Wed, Mar 13, 2019 at 10:40:19AM +0100, Cédric Le Goater wrote:
> On 2/26/19 6:24 AM, Paul Mackerras wrote:
> > On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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_SHIFT0
> >> +#define KVM_XIVE_EQ_PRIORITY_MASK 0x7
> >> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
> >> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
> >> +
> >> +/* 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
> of the KVM device. But it is not a well chosen name for the group
> KVM_DEV_XIVE_GRP_EQ_CONFIG.
>
> I should be using "eq identifier" and "eq values" or "eq state".
Yeah, that seems clearer.
> > 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 size
> of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.
> I thought that one extra u64 was not enough room for future.
>
> >
> > [snip]
> >
> >> + page = 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 = 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 <= 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
> change the helper xive_native_validate_queue_size() to return -EINVAL
> for other page sizes.
Ok.
> Do you think it would be complex to support XIVE EQs using a page larger
> than the default one on the guest ?
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.
--
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
signature.asc
Description: PGP signature
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On 2/26/19 6:24 AM, Paul Mackerras wrote:
> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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_SHIFT3
>> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
>> +
>> +/* 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
of the KVM device. But it is not a well chosen name for the group
KVM_DEV_XIVE_GRP_EQ_CONFIG.
I should be using "eq identifier" and "eq values" or "eq state".
> 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 size
of the XIVE END (the XIVE EQ descriptor in HW) which size is 32 bytes.
I thought that one extra u64 was not enough room for future.
>
> [snip]
>
>> +page = 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 = 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 <= 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
change the helper xive_native_validate_queue_size() to return -EINVAL
for other page sizes.
Do you think it would be complex to support XIVE EQs using a page larger
than the default one on the guest ?
Thanks,
C.
>
> Paul.
>
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On 3/13/19 5:03 AM, David Gibson wrote: > On Tue, Mar 12, 2019 at 06:00:38PM +0100, Cédric Le Goater wrote: >> On 2/25/19 3:39 AM, David Gibson wrote: >>> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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. >> >> I think we need to add some kind of flags to differentiate the hcall >> H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does >> not need OPAL support call and this could help in the code >> transition. > > Hrm. What's the actual difference in the semantics between the two > cases. None. But we don't need to set the EQ state in the case of the HCALL and it's (very) practical to run guests with XIVE enabled without the OPAL support. The latter is the main reason clearly. Thinking of it, I could test the EQ toggle bit and index passed to KVM and skip the OPAL call which restores the EQ state if they are zero. This is because I know that the OPAL call configuring the EQ resets them. That will do. No need for a flag. > The guest shouldn't have awareness of whether or not OPAL is involved. yes. Thanks, C.
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Tue, Mar 12, 2019 at 06:00:38PM +0100, Cédric Le Goater wrote: > On 2/25/19 3:39 AM, David Gibson wrote: > > On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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. > > I think we need to add some kind of flags to differentiate the hcall > H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does > not need OPAL support call and this could help in the code > transition. Hrm. What's the actual difference in the semantics between the two cases. The guest shouldn't have awareness of whether or not OPAL is involved. -- 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 signature.asc Description: PGP signature
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On 2/25/19 3:39 AM, David Gibson wrote:
> On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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.
I think we need to add some kind of flags to differentiate the hcall
H_INT_SET_QUEUE_CONFIG from the restore of the EQ. The hcall does
not need OPAL support call and this could help in the code transition.
But without OPAL support, we won't have migration. Would that be of
any use ?
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> arch/powerpc/include/asm/xive.h| 2 +
>> arch/powerpc/include/uapi/asm/kvm.h| 21 +++
>> arch/powerpc/kvm/book3s_xive.h | 2 +
>> arch/powerpc/kvm/book3s_xive.c | 15 +-
>> arch/powerpc/kvm/book3s_xive_native.c | 207 +
>> Documentation/virtual/kvm/devices/xive.txt | 29 +++
>> 6 files changed, 270 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/xive.h
>> b/arch/powerpc/include/asm/xive.h
>> index b579a943407b..46891f321606 100644
>> --- a/arch/powerpc/include/asm/xive.h
>> +++ b/arch/powerpc/include/asm/xive.h
>> @@ -73,6 +73,8 @@ struct xive_q {
>> u32 esc_irq;
>> atomic_tcount;
>> atomic_tpending_count;
>> +u64 guest_qpage;
>> +u32 guest_qsize;
>> };
>>
>> /* Global enable flags for the XIVE support */
>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
>> b/arch/powerpc/include/uapi/asm/kvm.h
>> index 91899c7f9abd..177e43f3edaf 100644
>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>> @@ -679,6 +679,7 @@ struct kvm_ppc_cpu_char {
>> #define KVM_DEV_XIVE_GRP_CTRL 1
>> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source
>> attributes */
>> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source
>> attributes */
>> +#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit eq attributes */
>>
>> /* Layout of 64-bit XIVE source attribute values */
>> #define KVM_XIVE_LEVEL_SENSITIVE(1ULL << 0)
>> @@ -694,4 +695,24 @@ struct kvm_ppc_cpu_char {
>> #define KVM_XIVE_SOURCE_EISN_SHIFT 33
>> #define KVM_XIVE_SOURCE_EISN_MASK 0xfffeULL
>>
>> +/* 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_SHIFT3
>> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
>> +
>> +/* 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];
>> +};
>> +
>> +#define KVM_XIVE_EQ_FLAG_ENABLED0x0001
>> +#define KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY 0x0002
>> +#define KVM_XIVE_EQ_FLAG_ESCALATE 0x0004
>> +
>> #endif /* __LINUX_KVM_POWERPC_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
>> index ab3ac152980d..6660d138c6b7 100644
>> --- a/arch/powerpc/kvm/book3s_xive.h
>> +++ b/arch/powerpc/kvm/book3s_xive.h
>> @@ -267,6 +267,8 @@ struct kvmppc_xive_src_block
>> *kvmppc_xive_create_src_block(
>> struct kvmppc_xive *xive, int irq);
>> void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
>> int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
>> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>> + bool single_escalation);
>>
>> #endif /* CONFIG_KVM_XICS */
>> #endif /* _KVM_PPC_BOOK3S_XICS_H */
>> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
>> index 086da91d7c6e..7431e31bc541 100644
>> --- a/arch/powerpc/kvm/book3s_xive.c
>> +++ b/arch/powerpc/kvm/book3s_xive.c
>> @@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
>> return IRQ_HANDLED;
>> }
>>
>> -static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
>> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
>> + bool single_escalation)
>> {
>>
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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_MASK0x7
> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
> +
> +/* 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 = 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 = 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 <= PAGE_SHIFT.
What happens if kvm_eq.qsize > PAGE_SHIFT?
Paul.
Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
On Fri, Feb 22, 2019 at 12:28:30PM +0100, Cédric 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.
>
> Signed-off-by: Cédric Le Goater
> ---
> arch/powerpc/include/asm/xive.h| 2 +
> arch/powerpc/include/uapi/asm/kvm.h| 21 +++
> arch/powerpc/kvm/book3s_xive.h | 2 +
> arch/powerpc/kvm/book3s_xive.c | 15 +-
> arch/powerpc/kvm/book3s_xive_native.c | 207 +
> Documentation/virtual/kvm/devices/xive.txt | 29 +++
> 6 files changed, 270 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
> index b579a943407b..46891f321606 100644
> --- a/arch/powerpc/include/asm/xive.h
> +++ b/arch/powerpc/include/asm/xive.h
> @@ -73,6 +73,8 @@ struct xive_q {
> u32 esc_irq;
> atomic_tcount;
> atomic_tpending_count;
> + u64 guest_qpage;
> + u32 guest_qsize;
> };
>
> /* Global enable flags for the XIVE support */
> diff --git a/arch/powerpc/include/uapi/asm/kvm.h
> b/arch/powerpc/include/uapi/asm/kvm.h
> index 91899c7f9abd..177e43f3edaf 100644
> --- a/arch/powerpc/include/uapi/asm/kvm.h
> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> @@ -679,6 +679,7 @@ struct kvm_ppc_cpu_char {
> #define KVM_DEV_XIVE_GRP_CTRL1
> #define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source
> attributes */
> #define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source
> attributes */
> +#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit eq attributes */
>
> /* Layout of 64-bit XIVE source attribute values */
> #define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
> @@ -694,4 +695,24 @@ struct kvm_ppc_cpu_char {
> #define KVM_XIVE_SOURCE_EISN_SHIFT 33
> #define KVM_XIVE_SOURCE_EISN_MASK0xfffeULL
>
> +/* Layout of 64-bit eq attribute */
> +#define KVM_XIVE_EQ_PRIORITY_SHIFT 0
> +#define KVM_XIVE_EQ_PRIORITY_MASK0x7
> +#define KVM_XIVE_EQ_SERVER_SHIFT 3
> +#define KVM_XIVE_EQ_SERVER_MASK 0xfff8ULL
> +
> +/* 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];
> +};
> +
> +#define KVM_XIVE_EQ_FLAG_ENABLED 0x0001
> +#define KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY 0x0002
> +#define KVM_XIVE_EQ_FLAG_ESCALATE0x0004
> +
> #endif /* __LINUX_KVM_POWERPC_H */
> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> index ab3ac152980d..6660d138c6b7 100644
> --- a/arch/powerpc/kvm/book3s_xive.h
> +++ b/arch/powerpc/kvm/book3s_xive.h
> @@ -267,6 +267,8 @@ struct kvmppc_xive_src_block
> *kvmppc_xive_create_src_block(
> struct kvmppc_xive *xive, int irq);
> void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
> int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> + bool single_escalation);
>
> #endif /* CONFIG_KVM_XICS */
> #endif /* _KVM_PPC_BOOK3S_XICS_H */
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 086da91d7c6e..7431e31bc541 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
> +int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
> + bool single_escalation)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> struct xive_q *q = >queues[prio];
> @@ -185,7 +186,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu,
> u8 prio)
> return -EIO;
> }
>
> - if (xc->xive->single_escalation)
> + if (single_escalation)
> name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
>vcpu->kvm->arch.lpid,
[PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
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.
Signed-off-by: Cédric Le Goater
---
arch/powerpc/include/asm/xive.h| 2 +
arch/powerpc/include/uapi/asm/kvm.h| 21 +++
arch/powerpc/kvm/book3s_xive.h | 2 +
arch/powerpc/kvm/book3s_xive.c | 15 +-
arch/powerpc/kvm/book3s_xive_native.c | 207 +
Documentation/virtual/kvm/devices/xive.txt | 29 +++
6 files changed, 270 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/xive.h b/arch/powerpc/include/asm/xive.h
index b579a943407b..46891f321606 100644
--- a/arch/powerpc/include/asm/xive.h
+++ b/arch/powerpc/include/asm/xive.h
@@ -73,6 +73,8 @@ struct xive_q {
u32 esc_irq;
atomic_tcount;
atomic_tpending_count;
+ u64 guest_qpage;
+ u32 guest_qsize;
};
/* Global enable flags for the XIVE support */
diff --git a/arch/powerpc/include/uapi/asm/kvm.h
b/arch/powerpc/include/uapi/asm/kvm.h
index 91899c7f9abd..177e43f3edaf 100644
--- a/arch/powerpc/include/uapi/asm/kvm.h
+++ b/arch/powerpc/include/uapi/asm/kvm.h
@@ -679,6 +679,7 @@ struct kvm_ppc_cpu_char {
#define KVM_DEV_XIVE_GRP_CTRL 1
#define KVM_DEV_XIVE_GRP_SOURCE2 /* 64-bit source
attributes */
#define KVM_DEV_XIVE_GRP_SOURCE_CONFIG 3 /* 64-bit source attributes */
+#define KVM_DEV_XIVE_GRP_EQ_CONFIG 4 /* 64-bit eq attributes */
/* Layout of 64-bit XIVE source attribute values */
#define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
@@ -694,4 +695,24 @@ struct kvm_ppc_cpu_char {
#define KVM_XIVE_SOURCE_EISN_SHIFT 33
#define KVM_XIVE_SOURCE_EISN_MASK 0xfffeULL
+/* 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_MASK0xfff8ULL
+
+/* 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];
+};
+
+#define KVM_XIVE_EQ_FLAG_ENABLED 0x0001
+#define KVM_XIVE_EQ_FLAG_ALWAYS_NOTIFY 0x0002
+#define KVM_XIVE_EQ_FLAG_ESCALATE 0x0004
+
#endif /* __LINUX_KVM_POWERPC_H */
diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
index ab3ac152980d..6660d138c6b7 100644
--- a/arch/powerpc/kvm/book3s_xive.h
+++ b/arch/powerpc/kvm/book3s_xive.h
@@ -267,6 +267,8 @@ struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
struct kvmppc_xive *xive, int irq);
void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb);
int kvmppc_xive_select_target(struct kvm *kvm, u32 *server, u8 prio);
+int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
+ bool single_escalation);
#endif /* CONFIG_KVM_XICS */
#endif /* _KVM_PPC_BOOK3S_XICS_H */
diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
index 086da91d7c6e..7431e31bc541 100644
--- a/arch/powerpc/kvm/book3s_xive.c
+++ b/arch/powerpc/kvm/book3s_xive.c
@@ -166,7 +166,8 @@ static irqreturn_t xive_esc_irq(int irq, void *data)
return IRQ_HANDLED;
}
-static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio)
+int kvmppc_xive_attach_escalation(struct kvm_vcpu *vcpu, u8 prio,
+ bool single_escalation)
{
struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
struct xive_q *q = >queues[prio];
@@ -185,7 +186,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8
prio)
return -EIO;
}
- if (xc->xive->single_escalation)
+ if (single_escalation)
name = kasprintf(GFP_KERNEL, "kvm-%d-%d",
vcpu->kvm->arch.lpid, xc->server_num);
else
@@ -217,7 +218,7 @@ static int xive_attach_escalation(struct kvm_vcpu *vcpu, u8
prio)
* interrupt, thus leaving it effectively masked after
* it fires once.
*/
- if
