RE: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
> -Original Message-
> From: Marcelo Tosatti [mailto:mtosa...@redhat.com]
> Sent: Tuesday, February 24, 2015 6:22 AM
> To: Wu, Feng
> Cc: t...@linutronix.de; mi...@redhat.com; h...@zytor.com; x...@kernel.org;
> g...@kernel.org; pbonz...@redhat.com; dw...@infradead.org;
> j...@8bytes.org; alex.william...@redhat.com; jiang@linux.intel.com;
> eric.au...@linaro.org; linux-ker...@vger.kernel.org;
> io...@lists.linux-foundation.org; kvm@vger.kernel.org
> Subject: Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU
> is preempted
>
> On Fri, Dec 12, 2014 at 11:14:57PM +0800, Feng Wu wrote:
> > This patch updates the Posted-Interrupts Descriptor when vCPU
> > is preempted.
> >
> > sched out:
> > - Set 'SN' to suppress furture non-urgent interrupts posted for
> > the vCPU.
>
> What wakes the vcpu in the case of a non-urgent interrupt, then?
Here we set 'SN' when vCPU's state is transmitted from running to
runnable (waiting in the runqueue), after the vCPU is chosen to run
again, the 'SN' will be clear. So no need to wakeup it explicitly.
>
> I wonder how is software suppose to configure the urgent/non-urgent
> flag. Can you give examples of (hypothetical) urgent and non-urgent
> interrupts.
Well, urgent and non-urgent flag is supported in hardware, I think the
original purpose of urgent interrupts is for real time usage. Then, when
such urgent interrupts happen, we can change the behavior of the
scheduler and make the related vCPU run immediately. However, from
software's point of view, we didn't find a clear picture about which
interrupts should be urgent and how to configure it, so we don't support
this currently.
>
> > sched in:
> > - Clear 'SN'
> > - Change NDST if vCPU is scheduled to a different CPU
> > - Set 'NV' to POSTED_INTR_VECTOR
>
> What about:
>
> POSTED_INTR_VECTOR interrupt handler:
> - Wakeup vcpu.
- If the vCPU is still running (not preempted), we don't need
to wakeup it.
- In POSTED_INTR_VECTOR interrupt handler, it is a little hard
to get vCPU related information, even if we get, it is not accurate
and may harm the performance. (need search)
> - Set 'SN' to suppress future interrupts.
We only need to set 'SN' when the vCPU is waiting on the runqueue,
So seems set 'SN' in this handler is not a good idea.
>
> HLT emulation entry:
> - Clear 'SN' to receive VT-d interrupt notification.
>
> > Signed-off-by: Feng Wu
> > ---
> > arch/x86/kvm/vmx.c | 44
>
> > 1 file changed, 44 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index ee3b735..bf2e6cd 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -1916,10 +1916,54 @@ static void vmx_vcpu_load(struct kvm_vcpu
> *vcpu, int cpu)
> > vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */
> > vmx->loaded_vmcs->cpu = cpu;
> > }
> > +
> > + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > + struct pi_desc old, new;
> > + unsigned int dest;
> > +
> > + memset(&old, 0, sizeof(old));
> > + memset(&new, 0, sizeof(new));
> > +
> > + do {
> > + old.control = new.control = pi_desc->control;
> > + if (vcpu->cpu != cpu) {
> > + dest = cpu_physical_id(cpu);
> > +
> > + if (x2apic_enabled())
> > + new.ndst = dest;
> > + else
> > + new.ndst = (dest << 8) & 0xFF00;
> > + }
> > +
> > + pi_clear_sn(&new);
> > +
> > + /* set 'NV' to 'notification vector' */
> > + new.nv = POSTED_INTR_VECTOR;
> > + } while (cmpxchg(&pi_desc->control, old.control,
> > + new.control) != old.control);
> > + }
> > }
> >
> > static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
> > {
> > + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > + struct pi_desc old, new;
> > +
> > + memset(&old, 0, sizeof(old));
> > + memset(&new, 0, sizeof(new));
> > +
> > + /* Set SN when the vCPU i
Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
On Fri, Dec 12, 2014 at 11:14:57PM +0800, Feng Wu wrote:
> This patch updates the Posted-Interrupts Descriptor when vCPU
> is preempted.
>
> sched out:
> - Set 'SN' to suppress furture non-urgent interrupts posted for
> the vCPU.
What wakes the vcpu in the case of a non-urgent interrupt, then?
I wonder how is software suppose to configure the urgent/non-urgent
flag. Can you give examples of (hypothetical) urgent and non-urgent
interrupts.
> sched in:
> - Clear 'SN'
> - Change NDST if vCPU is scheduled to a different CPU
> - Set 'NV' to POSTED_INTR_VECTOR
What about:
POSTED_INTR_VECTOR interrupt handler:
- Wakeup vcpu.
- Set 'SN' to suppress future interrupts.
HLT emulation entry:
- Clear 'SN' to receive VT-d interrupt notification.
> Signed-off-by: Feng Wu
> ---
> arch/x86/kvm/vmx.c | 44
> 1 file changed, 44 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ee3b735..bf2e6cd 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1916,10 +1916,54 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int
> cpu)
> vmcs_writel(HOST_IA32_SYSENTER_ESP, sysenter_esp); /* 22.2.3 */
> vmx->loaded_vmcs->cpu = cpu;
> }
> +
> + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> + struct pi_desc old, new;
> + unsigned int dest;
> +
> + memset(&old, 0, sizeof(old));
> + memset(&new, 0, sizeof(new));
> +
> + do {
> + old.control = new.control = pi_desc->control;
> + if (vcpu->cpu != cpu) {
> + dest = cpu_physical_id(cpu);
> +
> + if (x2apic_enabled())
> + new.ndst = dest;
> + else
> + new.ndst = (dest << 8) & 0xFF00;
> + }
> +
> + pi_clear_sn(&new);
> +
> + /* set 'NV' to 'notification vector' */
> + new.nv = POSTED_INTR_VECTOR;
> + } while (cmpxchg(&pi_desc->control, old.control,
> + new.control) != old.control);
> + }
> }
>
> static void vmx_vcpu_put(struct kvm_vcpu *vcpu)
> {
> + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> + struct pi_desc old, new;
> +
> + memset(&old, 0, sizeof(old));
> + memset(&new, 0, sizeof(new));
> +
> + /* Set SN when the vCPU is preempted */
> + if (vcpu->preempted) {
> + do {
> + old.control = new.control = pi_desc->control;
> + pi_set_sn(&new);
> + } while (cmpxchg(&pi_desc->control, old.control,
> + new.control) != old.control);
> + }
> + }
> +
> __vmx_load_host_state(to_vmx(vcpu));
> if (!vmm_exclusive) {
> __loaded_vmcs_clear(to_vmx(vcpu)->loaded_vmcs);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
> -Original Message-
> From: linux-kernel-ow...@vger.kernel.org
> [mailto:linux-kernel-ow...@vger.kernel.org] On Behalf Of Paolo Bonzini
> Sent: Thursday, December 18, 2014 4:32 PM
> To: linux-ker...@vger.kernel.org
> Cc: io...@lists.linux-foundation.org; kvm@vger.kernel.org;
> linux-ker...@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU
> is preempted
>
>
>
> On 18/12/2014 04:15, Wu, Feng wrote:
> > Thanks for your comments, Paolo!
> >
> > If we use u64 new_control, we cannot use new.sn any more.
> > Maybe we can change the struct pi_desc {} like this:
> >
> > typedef struct pid_control{
> > u64 on : 1,
> > sn : 1,
> > rsvd_1 : 13,
> > ndm : 1,
> > nv : 8,
> > rsvd_2 : 8,
> > ndst: 32;
> > }pid_control_t;
> >
> > struct pi_desc {
> > u32 pir[8]; /* Posted interrupt requested */
> > pid_control_t control;
>
> Probably something like this to keep the union:
>
> typedef union pid_control {
> u64 full;
> struct {
> u64 on : 1,
> ...
> } fields;
> };
>
> > u32 rsvd[6];
> > } __aligned(64);
> >
> >
> > Then we can define pid_control_t new_control, old_control. And use
> new_control.sn = 0.
> >
> > What is your opinon?
>
> Sure. Alternatively, keep using struct pi_desc new; just
> do not zero it, nor access any field outide the control word.
>
> Paolo
Yes, this is also a good idea. Thanks!
Thanks,
Feng
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
On 18/12/2014 04:15, Wu, Feng wrote:
> Thanks for your comments, Paolo!
>
> If we use u64 new_control, we cannot use new.sn any more.
> Maybe we can change the struct pi_desc {} like this:
>
> typedef struct pid_control{
> u64 on : 1,
> sn : 1,
> rsvd_1 : 13,
> ndm : 1,
> nv : 8,
> rsvd_2 : 8,
> ndst: 32;
> }pid_control_t;
>
> struct pi_desc {
> u32 pir[8]; /* Posted interrupt requested */
> pid_control_t control;
Probably something like this to keep the union:
typedef union pid_control {
u64 full;
struct {
u64 on : 1,
...
} fields;
};
> u32 rsvd[6];
> } __aligned(64);
>
>
> Then we can define pid_control_t new_control, old_control. And use
> new_control.sn = 0.
>
> What is your opinon?
Sure. Alternatively, keep using struct pi_desc new; just
do not zero it, nor access any field outide the control word.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
> -Original Message-
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Thursday, December 18, 2014 1:11 AM
> To: Wu, Feng; Thomas Gleixner; Ingo Molnar; H. Peter Anvin; x...@kernel.org;
> Gleb Natapov; Paolo Bonzini; dw...@infradead.org; j...@8bytes.org; Alex
> Williamson; jiang.liu-VuQAYsv1563Yd54FQh9/c...@public.gmane.org
> Cc: io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; KVM list;
> Eric Auger
> Subject: Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU
> is preempted
>
>
>
> On 12/12/2014 16:14, Feng Wu wrote:
> > + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > + struct pi_desc old, new;
> > + unsigned int dest;
> > +
> > + memset(&old, 0, sizeof(old));
> > + memset(&new, 0, sizeof(new));
>
> This is quite expensive. Just use an u64 for old_control and
> new_control, instead of a full struct.
>
> >
> > + pi_clear_sn(&new);
>
> This can be simply new.sn = 0. It does not need atomic operations.
Thanks for your comments, Paolo!
If we use u64 new_control, we cannot use new.sn any more.
Maybe we can change the struct pi_desc {} like this:
typedef struct pid_control{
u64 on : 1,
sn : 1,
rsvd_1 : 13,
ndm : 1,
nv : 8,
rsvd_2 : 8,
ndst: 32;
}pid_control_t;
struct pi_desc {
u32 pir[8]; /* Posted interrupt requested */
pid_control_t control;
u32 rsvd[6];
} __aligned(64);
Then we can define pid_control_t new_control, old_control. And use
new_control.sn = 0.
What is your opinon?
Thanks,
Feng
>
> Same in patch 24 (if needed at all there---see the reply there).
>
> >
> > + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> > + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > + struct pi_desc old, new;
> > +
> > + memset(&old, 0, sizeof(old));
> > + memset(&new, 0, sizeof(new));
> > +
>
> Here you do not need old/new at all because...
>
> > + if (vcpu->preempted) {
> > + do {
> > + old.control = new.control = pi_desc->control;
> > + pi_set_sn(&new);
> > + } while (cmpxchg(&pi_desc->control, old.control,
> > + new.control) != old.control);
>
> this can do pi_set_sn directly on pi_desc, without the cmpxchg.
>
> Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v3 23/26] KVM: Update Posted-Interrupts Descriptor when vCPU is preempted
On 12/12/2014 16:14, Feng Wu wrote:
> + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> + struct pi_desc old, new;
> + unsigned int dest;
> +
> + memset(&old, 0, sizeof(old));
> + memset(&new, 0, sizeof(new));
This is quite expensive. Just use an u64 for old_control and
new_control, instead of a full struct.
>
> + pi_clear_sn(&new);
This can be simply new.sn = 0. It does not need atomic operations.
Same in patch 24 (if needed at all there---see the reply there).
>
> + if (irq_remapping_cap(IRQ_POSTING_CAP)) {
> + struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> + struct pi_desc old, new;
> +
> + memset(&old, 0, sizeof(old));
> + memset(&new, 0, sizeof(new));
> +
Here you do not need old/new at all because...
> + if (vcpu->preempted) {
> + do {
> + old.control = new.control = pi_desc->control;
> + pi_set_sn(&new);
> + } while (cmpxchg(&pi_desc->control, old.control,
> + new.control) != old.control);
this can do pi_set_sn directly on pi_desc, without the cmpxchg.
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
