On Tue, 2017-03-28 at 16:26 +1100, Paul Mackerras wrote:
>
> > --- a/arch/powerpc/include/asm/kvm_book3s_asm.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s_asm.h
> > @@ -111,6 +111,8 @@ struct kvmppc_host_state {
> > struct kvm_vcpu *kvm_vcpu;
> > struct kvmppc_vcore *kvm_vcore;
> > void __iomem *xics_phys;
> > + void __iomem *xive_tm_area_phys;
> > + void __iomem *xive_tm_area_virt;
>
> Does this cause the paca to become a cacheline larger? (Not that
> there is much alternative to having these fields.)
It does, though as you said, there's little I can do here.
.../...
> >
> > +/* QW0 and QW1 of a context */
> > +union xive_qw01 {
> > + struct {
> > + u8 nsr;
> > + u8 cppr;
> > + u8 ipb;
> > + u8 lsmfb;
> > + u8 ack;
> > + u8 inc;
> > + u8 age;
> > + u8 pipr;
> > + };
> > + __be64 qw;
> > +};
>
> This is slightly confusing because a "QW" (quadword) would normally
> be 128 bits, but this union is 64 bits.
It's me being wrong. It's not QW0 and QW1, it's word 0 and 1 of the QW.
Word 2 is used for setting up the CAM and Word 3 is unused. I'll fixup
the naming.
> >
> > +extern int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32
> > server,
> > + u32 priority);
> > +extern int kvmppc_xive_get_xive(struct kvm *kvm, u32 irq, u32
> > *server,
> > + u32 *priority);
>
> Might be worth a comment here to explain that the first xive is
> eXternal Interrupt Virtualization Engine and the second xive is
> eXternal Interrupt Vector Entry.
Haha, indeed ;-) I'll add something.
> >
> > +static inline void kvmppc_set_xive_tm_area_phys(int cpu, unsigned
> > long addr)
> > +{}
>
> Shouldn't this be kvmppc_set_xive_tm_area to match the other
> definition?
Yup. Bit-rot from earlier versions of the patch that only had "phys"
(real mode only).
> > --- a/arch/powerpc/include/asm/xive.h
> > +++ b/arch/powerpc/include/asm/xive.h
> > @@ -55,7 +55,8 @@ struct xive_q {
> > #define XIVE_ESB_SET_PQ_01 0xd00
> > #define XIVE_ESB_SET_PQ_10 0xe00
> > #define XIVE_ESB_SET_PQ_11 0xf00
> > -#define XIVE_ESB_MASK XIVE_ESB_SET_PQ_01
> > +#define XIVE_ESB_SOFT_MASK XIVE_ESB_SET_PQ_10
> > +#define XIVE_ESB_HARD_MASK XIVE_ESB_SET_PQ_01
>
> What's the difference between a "soft" mask and a "hard" mask?
I'll document, though I may not use the "aliases" anymore if it's
just confusing. (Basically soft mask will remember in Q if
something happens while masked, hard mask will not).
> >
> > - kvmppc_xics_set_mapped(kvm, guest_gsi, desc-
> > >irq_data.hwirq);
> > + if (xive_enabled())
> > + rc = kvmppc_xive_set_mapped(kvm, guest_gsi, desc);
> > + else
> > + kvmppc_xics_set_mapped(kvm, guest_gsi, desc-
> > >irq_data.hwirq);
> > + printk("set mapped for IRQ %d -> %d returned %d\n",
> > + host_irq, guest_gsi, rc);
>
> This seems like a debugging thing that should be removed or turned
> into a DBG().
Yup, forgot about it.
@@ -398,6 +422,9 @@ static long kvmppc_read_one_intr(bool *again)
> > u8 host_ipi;
> > int64_t rc;
> >
> > + if (xive_enabled())
> > + return 1;
>
> Why not do this in kvmppc_read_intr() rather than here?
Dunno, probably missed that loop. I'll change it
> > paca */
> > +#ifdef CONFIG_KVM_XICS
> > + /* We are exiting, pull the VP from the XIVE */
> > + lwz r0, VCPU_XIVE_PUSHED(r9)
> > + cmpwi cr0, r0, 0
> > + beq 1f
> > + li r7, TM_SPC_PULL_OS_CTX
> > + li r6, TM_QW1_OS
> > + mfmsr r0
> > + andi. r0, r0, MSR_IR /* in real
> > mode? */
> > + beq 2f
> > + ld r10, HSTATE_XIVE_TM_AREA_VIRT(r13)
> > + cmpldi cr0, r10, 0
> > + beq 1f
> > + lwzx r11, r7, r10
> > + eieio
> > + ldx r11, r6, r10
>
> I assume you meant to do these two loads into the same target
> register, but I don't know why, so a comment would be useful.
Right. We don't care about the result of the first one. It's
the special side-effect load to perform the pull. It doesn't
return useful info (the spec isn't clear there, so I should
document it). Once we have pulled, the TM OS area is frozen
so I can do a 64-bit load to get W0 and W1 & back them up.
> > + b 3f
> > +2: ld r10, HSTATE_XIVE_TM_AREA_PHYS(r13)
> > + cmpldi cr0, r10, 0
> > + beq 1f
> > + lwzcix r11, r7, r10
> > + eieio
> > + ldcix r11, r6, r10
> > +3: std r11, VCPU_XIVE_SAVED_STATE(r9)
> > + /* Fixup some of the state for the next load */
> > + li r10, 0
> > + li r0, 0xff
> > + stw r10, VCPU_XIVE_PUSHED(r9)
> > + stb r10, (VCPU_XIVE_SAVED_STATE+3)(r9)
> > + stb r0, (VCPU_XIVE_SAVED_STATE+4)(r9)
> > +1:
> > +#endif /* CONFIG_KVM_XICS */
> > /* Save more register state */
> > mfdar r6
> > mfdsisr r7
> > @@ -2035,7 +2086,7 @@ hcall_real_table:
> > .long DOTSYM(kvmppc_rm_h_eoi) - hcall_real_table
> > .long DOTSYM(kvmppc_rm_h_cppr) - hcall_real_table
> > .long DOTSYM(kvmppc_rm_h_ipi) - hcall_real_table
> > - .long 0 /* 0x70 - H_IPOLL */
> > + .long DOTSYM(kvmppc_rm_h_ipoll) - hcall_real_table
> > .long DOTSYM(kvmppc_rm_h_xirr) - hcall_real_table
> > #else
> > .long 0 /* 0x64 - H_EOI */
> > @@ -2205,7 +2256,11 @@ hcall_real_table:
> > .long 0 /* 0x2f0 */
> > .long 0 /* 0x2f4 */
> > .long 0 /* 0x2f8 */
> > - .long 0 /* 0x2fc */
> > +#ifdef CONFIG_KVM_XICS
> > + .long DOTSYM(kvmppc_rm_h_xirr_x) - hcall_real_table
> > +#else
> > + .long 0 /* 0x2fc - H_XIRR_X*/
> > +#endif
> > .long DOTSYM(kvmppc_h_random) - hcall_real_table
> > .globl hcall_real_table_end
> > hcall_real_table_end:
> > @@ -2980,6 +3035,7 @@ kvmppc_fix_pmao:
> > isync
> > blr
> >
> > +
>
> Gratuitous extra blank line.
Isn't it pretty ? :-)
> >
> > + /* Allocate the queue and retrieve infos on current node
> > for now */
> > + qpage = (__be32 *)__get_free_pages(GFP_KERNEL, xive-
> > >q_alloc_order);
>
> Possibly q_page_order would be a better name than q_alloc_order.
Maybe ... I'll add a comment too.
The rest of your comments don't need a reply :)
I'll respin.
Cheers,
Ben.
