* Re: [PATCH 5/5] ocxl: Remove some unused exported symbols
From: Greg Kurz @ 2019-03-14 6:50 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Frederic Barrat, linuxppc-dev
In-Reply-To: <1dd6d24f3ee5a2029ee6fbb2890356197e881101.camel@au1.ibm.com>
On Thu, 14 Mar 2019 13:23:21 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> On Wed, 2019-03-13 at 10:10 +0100, Greg Kurz wrote:
> > On Wed, 13 Mar 2019 15:07:01 +1100
> > "Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> >
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > >
> > > Remove some unused exported symbols.
> > >
> > > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > > ---
> > > drivers/misc/ocxl/config.c | 2 --
> > > drivers/misc/ocxl/ocxl_internal.h | 23 +++++++++++++++++++++++
> > > include/misc/ocxl.h | 23 -----------------------
> > > 3 files changed, 23 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/misc/ocxl/config.c
> > > b/drivers/misc/ocxl/config.c
> > > index 026ac2ac4f9c..c90c2e4875bf 100644
> > > --- a/drivers/misc/ocxl/config.c
> > > +++ b/drivers/misc/ocxl/config.c
> > > @@ -299,7 +299,6 @@ int ocxl_config_check_afu_index(struct pci_dev
> > > *dev,
> > > }
> > > return 1;
> > > }
> > > -EXPORT_SYMBOL_GPL(ocxl_config_check_afu_index);
> > >
> > > static int read_afu_name(struct pci_dev *dev, struct
> > > ocxl_fn_config *fn,
> > > struct ocxl_afu_config *afu)
> > > @@ -535,7 +534,6 @@ int ocxl_config_get_pasid_info(struct pci_dev
> > > *dev, int *count)
> > > {
> > > return pnv_ocxl_get_pasid_count(dev, count);
> > > }
> > > -EXPORT_SYMBOL_GPL(ocxl_config_get_pasid_info);
> > >
> > > void ocxl_config_set_afu_pasid(struct pci_dev *dev, int pos, int
> > > pasid_base,
> > > u32 pasid_count_log)
> > > diff --git a/drivers/misc/ocxl/ocxl_internal.h
> > > b/drivers/misc/ocxl/ocxl_internal.h
> > > index 321b29e77f45..06fd98c989c8 100644
> > > --- a/drivers/misc/ocxl/ocxl_internal.h
> > > +++ b/drivers/misc/ocxl/ocxl_internal.h
> > > @@ -107,6 +107,29 @@ void ocxl_pasid_afu_free(struct ocxl_fn *fn,
> > > u32 start, u32 size);
> > > int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> > > void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> > >
> > > +/*
> > > + * Get the max PASID value that can be used by the function
> > > + */
> > > +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> > > +
> > > +/*
> > > + * Check if an AFU index is valid for the given function.
> > > + *
> > > + * AFU indexes can be sparse, so a driver should check all indexes
> > > up
> > > + * to the maximum found in the function description
> > > + */
> > > +int ocxl_config_check_afu_index(struct pci_dev *dev,
> > > + struct ocxl_fn_config *fn, int
> > > afu_idx);
> > > +
> > > +/**
> >
> > Two *s ?
> >
>
> These are Sphinx formatted comments (similar, but not quite the same as
> Doxygen).
>
Oh... should the other comments be converted to this format for consistency ?
> > Also, this results in an ocxl_internal.h header file where only these
> > three functions are documented... which looks a bit weird IMHO. Since
> > these are ocxl internals, do we _really_ need to keep the comments ?
>
> I believe we should, it's a courtesy to the next person who has to work
> in the area.
>
> There are more documentation comments coming in further series.
>
Fair enough.
Reviewed-by: Greg Kurz <groug@kaod.org>
^ permalink raw reply
* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: Cédric Le Goater @ 2019-03-14 7:11 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, linuxppc-dev
In-Reply-To: <20190314023210.GL8211@umbus.fritz.box>
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_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
>> 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.
^ permalink raw reply
* Re: [PATCH v2 10/16] KVM: PPC: Book3S HV: XIVE: add get/set accessors for the VP XIVE state
From: Cédric Le Goater @ 2019-03-14 7:08 UTC (permalink / raw)
To: David Gibson; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <20190314030914.GN8211@umbus.fritz.box>
On 3/14/19 4:09 AM, David Gibson wrote:
> On Wed, Mar 13, 2019 at 02:19:13PM +0100, Cédric Le Goater wrote:
>> On 2/25/19 4:31 AM, David Gibson wrote:
>>> On Fri, Feb 22, 2019 at 12:28:34PM +0100, Cédric Le Goater wrote:
>>>> At a VCPU level, the state of the thread interrupt management
>>>> registers needs to be collected. These registers are cached under the
>>>> 'xive_saved_state.w01' field of the VCPU when the VPCU context is
>>>> pulled from the HW thread. An OPAL call retrieves the backup of the
>>>> IPB register in the underlying XIVE NVT structure and merges it in the
>>>> KVM state.
>>>>
>>>> The structures of the interface between QEMU and KVM provisions some
>>>> extra room (two u64) for further extensions if more state needs to be
>>>> transferred back to QEMU.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>> arch/powerpc/include/asm/kvm_ppc.h | 11 +++
>>>> arch/powerpc/include/uapi/asm/kvm.h | 2 +
>>>> arch/powerpc/kvm/book3s.c | 24 +++++++
>>>> arch/powerpc/kvm/book3s_xive_native.c | 82 ++++++++++++++++++++++
>>>> Documentation/virtual/kvm/devices/xive.txt | 19 +++++
>>>> 5 files changed, 138 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>>>> index 1e61877fe147..664c65051612 100644
>>>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>>>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>>>> @@ -272,6 +272,7 @@ union kvmppc_one_reg {
>>>> u64 addr;
>>>> u64 length;
>>>> } vpaval;
>>>> + u64 xive_timaval[4];
>>>
>>> This is doubling the size of the userspace visible one_reg union. Is
>>> that safe?
>>
>> 'safe' as in compatibility on an older KVM which would still use the old
>> kvmppc_one_reg definition ?
>
> I was more thinking of old qemu with a new kernel.
>
>> It should be fine as KVM_REG_PPC_VP_STATE would not be handled. Am I
>> wrong ?
>
> Looks like it should be ok, because we only partially copy the
> structure to/from userspace due to the one_reg_size() logic. If the
> whole union was always copied, it would be hilariously unsafe.
>
>>
>>>> };
>>>>
>>>> struct kvmppc_ops {
>>>> @@ -604,6 +605,10 @@ extern int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>>>> extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
>>>> extern void kvmppc_xive_native_init_module(void);
>>>> extern void kvmppc_xive_native_exit_module(void);
>>>> +extern int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
>>>> + union kvmppc_one_reg *val);
>>>> +extern int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
>>>> + union kvmppc_one_reg *val);
>>>>
>>>> #else
>>>> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
>>>> @@ -636,6 +641,12 @@ static inline int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>>>> static inline void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu) { }
>>>> static inline void kvmppc_xive_native_init_module(void) { }
>>>> static inline void kvmppc_xive_native_exit_module(void) { }
>>>> +static inline int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
>>>> + union kvmppc_one_reg *val)
>>>> +{ return 0; }
>>>> +static inline int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
>>>> + union kvmppc_one_reg *val)
>>>> +{ return -ENOENT; }
>>>>
>>>> #endif /* CONFIG_KVM_XIVE */
>>>>
>>>> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
>>>> index cd78ad1020fe..42d4ef93ec2d 100644
>>>> --- a/arch/powerpc/include/uapi/asm/kvm.h
>>>> +++ b/arch/powerpc/include/uapi/asm/kvm.h
>>>> @@ -480,6 +480,8 @@ struct kvm_ppc_cpu_char {
>>>> #define KVM_REG_PPC_ICP_PPRI_SHIFT 16 /* pending irq priority */
>>>> #define KVM_REG_PPC_ICP_PPRI_MASK 0xff
>>>>
>>>> +#define KVM_REG_PPC_VP_STATE (KVM_REG_PPC | KVM_REG_SIZE_U256 | 0x8d)
>>>> +
>>>> /* Device control API: PPC-specific devices */
>>>> #define KVM_DEV_MPIC_GRP_MISC 1
>>>> #define KVM_DEV_MPIC_BASE_ADDR 0 /* 64-bit */
>>>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>>>> index 96d43f091255..f85a9211f30c 100644
>>>> --- a/arch/powerpc/kvm/book3s.c
>>>> +++ b/arch/powerpc/kvm/book3s.c
>>>> @@ -641,6 +641,18 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
>>>> *val = get_reg_val(id, kvmppc_xics_get_icp(vcpu));
>>>> break;
>>>> #endif /* CONFIG_KVM_XICS */
>>>> +#ifdef CONFIG_KVM_XIVE
>>>> + case KVM_REG_PPC_VP_STATE:
>>>> + if (!vcpu->arch.xive_vcpu) {
>>>> + r = -ENXIO;
>>>> + break;
>>>> + }
>>>> + if (xive_enabled())
>>>> + r = kvmppc_xive_native_get_vp(vcpu, val);
>>>> + else
>>>> + r = -ENXIO;
>>>> + break;
>>>> +#endif /* CONFIG_KVM_XIVE */
>>>> case KVM_REG_PPC_FSCR:
>>>> *val = get_reg_val(id, vcpu->arch.fscr);
>>>> break;
>>>> @@ -714,6 +726,18 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
>>>> r = kvmppc_xics_set_icp(vcpu, set_reg_val(id, *val));
>>>> break;
>>>> #endif /* CONFIG_KVM_XICS */
>>>> +#ifdef CONFIG_KVM_XIVE
>>>> + case KVM_REG_PPC_VP_STATE:
>>>> + if (!vcpu->arch.xive_vcpu) {
>>>> + r = -ENXIO;
>>>> + break;
>>>> + }
>>>> + if (xive_enabled())
>>>> + r = kvmppc_xive_native_set_vp(vcpu, val);
>>>> + else
>>>> + r = -ENXIO;
>>>> + break;
>>>> +#endif /* CONFIG_KVM_XIVE */
>>>> case KVM_REG_PPC_FSCR:
>>>> vcpu->arch.fscr = set_reg_val(id, *val);
>>>> break;
>>>> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
>>>> index 3debc876d5a0..132bff52d70a 100644
>>>> --- a/arch/powerpc/kvm/book3s_xive_native.c
>>>> +++ b/arch/powerpc/kvm/book3s_xive_native.c
>>>> @@ -845,6 +845,88 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
>>>> return ret;
>>>> }
>>>>
>>>> +/*
>>>> + * Interrupt Pending Buffer (IPB) offset
>>>> + */
>>>> +#define TM_IPB_SHIFT 40
>>>> +#define TM_IPB_MASK (((u64) 0xFF) << TM_IPB_SHIFT)
>>>> +
>>>> +int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>>>> +{
>>>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>>>> + u64 opal_state;
>>>> + int rc;
>>>> +
>>>> + if (!kvmppc_xive_enabled(vcpu))
>>>> + return -EPERM;
>>>> +
>>>> + if (!xc)
>>>> + return -ENOENT;
>>>> +
>>>> + /* Thread context registers. We only care about IPB and CPPR */
>>>> + val->xive_timaval[0] = vcpu->arch.xive_saved_state.w01;
>>>> +
>>>> + /*
>>>> + * Return the OS CAM line to print out the VP identifier in
>>>> + * the QEMU monitor. This is not restored.
>>>> + */
>>>> + val->xive_timaval[1] = vcpu->arch.xive_cam_word;
>>>
>>> I'm pretty dubious about this mixing of vital state information with
>>> what's basically debug information.
>>
>> I think QEMU deserves to know about the OS CAM line value. I was even
>> thinking about adding the POOL CAM line value for future use (nested)
>>
>>> Doubly so since it requires changing the ABI to increase
>>> the one_reg union's size.
>>
>> OK. That's one argument.
>>
>>> Might be better to have this control only return the 0th and 2nd u64s
>>> from the TIMA, with the CAM debug information returned via some other
>>> mechanism.
>>
>> Like an extra reg : KVM_REG_PPC_VP_CAM ?
>
> That would be the obvious choice, yes.
OK. Let's keep that in mind but I think it is overkill. I would rather
have one reg per ring instead.
>>>> +
>>>> + /* Get the VP state from OPAL */
>>>> + rc = xive_native_get_vp_state(xc->vp_id, &opal_state);
>>>> + if (rc)
>>>> + return rc;
>>>> +
>>>> + /*
>>>> + * Capture the backup of IPB register in the NVT structure and
>>>> + * merge it in our KVM VP state.
>>>> + */
>>>> + val->xive_timaval[0] |= cpu_to_be64(opal_state & TM_IPB_MASK);
>>>> +
>>>> + pr_devel("%s NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x opal=%016llx\n",
>>>> + __func__,
>>>> + vcpu->arch.xive_saved_state.nsr,
>>>> + vcpu->arch.xive_saved_state.cppr,
>>>> + vcpu->arch.xive_saved_state.ipb,
>>>> + vcpu->arch.xive_saved_state.pipr,
>>>> + vcpu->arch.xive_saved_state.w01,
>>>> + (u32) vcpu->arch.xive_cam_word, opal_state);
>>>
>>> Hrm.. except you don't seem to be using the last half of the timaval
>>> field anyway.
>>
>> Yes. The two u64 are extras. We can do without.
>>
>> Would that be ok if I stored the w01 regs in the first u64, the CAM line(s)
>> in the second and remove the extra two u64 ?
>
> I'd still prefer them in separate regs. They kind of belong to
> different categories of information, and I can't think of any
> particular reason you'd have to update or fetch them as a unit.
Because they belong to the same thread interrupt context and the same
ring (OS) even if only the hypervisor can set the OS CAM line. The OS
can only set the CPPR. QEMU operates at the hypervisor level so it is
not violating any privilege level.
>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
>>>> +{
>>>> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>>>> + struct kvmppc_xive *xive = vcpu->kvm->arch.xive;
>>>> +
>>>> + pr_devel("%s w01=%016llx vp=%016llx\n", __func__,
>>>> + val->xive_timaval[0], val->xive_timaval[1]);
>>>> +
>>>> + if (!kvmppc_xive_enabled(vcpu))
>>>> + return -EPERM;
>>>> +
>>>> + if (!xc || !xive)
>>>> + return -ENOENT;
>>>> +
>>>> + /* We can't update the state of a "pushed" VCPU */
>>>> + if (WARN_ON(vcpu->arch.xive_pushed))
>>>
>>> What prevents userspace from tripping this WARN_ON()?
>>
>> if the vCPU is executing a vCPU ioctl, it means that it exited the guest
>> and that its interrupt context has been pulled out of XIVE.
>
> But couldn't one user thread call the vcpu ioctl() while another is
> inside the guest?
Not while setting the VP state. The guest is not resumed.
Thanks,
C.
>
>>>> + return -EIO;
>>>
>>> EBUSY might be more appropriate here.
>>
>> OK.
>>
>> Thanks,
>>
>> C.
>>
>>>
>>>> +
>>>> + /*
>>>> + * Restore the thread context registers. IPB and CPPR should
>>>> + * be the only ones that matter.
>>>> + */
>>>> + vcpu->arch.xive_saved_state.w01 = val->xive_timaval[0];
>>>> +
>>>> + /*
>>>> + * There is no need to restore the XIVE internal state (IPB
>>>> + * stored in the NVT) as the IPB register was merged in KVM VP
>>>> + * state when captured.
>>>> + */
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int xive_native_debug_show(struct seq_file *m, void *private)
>>>> {
>>>> struct kvmppc_xive *xive = m->private;
>>>> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
>>>> index a26be635cff9..1b8957c50c53 100644
>>>> --- a/Documentation/virtual/kvm/devices/xive.txt
>>>> +++ b/Documentation/virtual/kvm/devices/xive.txt
>>>> @@ -102,6 +102,25 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
>>>> -EINVAL: Not initialized source number, invalid priority or
>>>> invalid CPU number.
>>>>
>>>> +* VCPU state
>>>> +
>>>> + The XIVE IC maintains VP interrupt state in an internal structure
>>>> + called the NVT. When a VP is not dispatched on a HW processor
>>>> + thread, this structure can be updated by HW if the VP is the target
>>>> + of an event notification.
>>>> +
>>>> + It is important for migration to capture the cached IPB from the NVT
>>>> + as it synthesizes the priorities of the pending interrupts. We
>>>> + capture a bit more to report debug information.
>>>> +
>>>> + KVM_REG_PPC_VP_STATE (4 * 64bits)
>>>> + bits: | 63 .... 32 | 31 .... 0 |
>>>> + values: | TIMA word0 | TIMA word1 |
>>>> + bits: | 127 .......... 64 |
>>>> + values: | VP CAM Line |
>>>> + bits: | 255 .......... 128 |
>>>> + values: | unused |
>>>> +
>>>> * Migration:
>>>>
>>>> Saving the state of a VM using the XIVE native exploitation mode
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v6 4/4] hugetlb: allow to free gigantic pages regardless of the configuration
From: Aneesh Kumar K.V @ 2019-03-14 5:52 UTC (permalink / raw)
To: Alexandre Ghiti, Andrew Morton, Vlastimil Babka, Catalin Marinas,
Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens,
Yoshinori Sato, Rich Felker, David S . Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, x86, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Mike Kravetz, linux-arm-kernel,
linux-kernel, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-mm
Cc: Alexandre Ghiti
In-Reply-To: <20190307132015.26970-5-alex@ghiti.fr>
Alexandre Ghiti <alex@ghiti.fr> writes:
> On systems without CONTIG_ALLOC activated but that support gigantic pages,
> boottime reserved gigantic pages can not be freed at all. This patch
> simply enables the possibility to hand back those pages to memory
> allocator.
>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> Acked-by: David S. Miller <davem@davemloft.net> [sparc]
> ---
> arch/arm64/Kconfig | 2 +-
> arch/arm64/include/asm/hugetlb.h | 4 --
> arch/powerpc/include/asm/book3s/64/hugetlb.h | 7 ---
> arch/powerpc/platforms/Kconfig.cputype | 2 +-
> arch/s390/Kconfig | 2 +-
> arch/s390/include/asm/hugetlb.h | 3 --
> arch/sh/Kconfig | 2 +-
> arch/sparc/Kconfig | 2 +-
> arch/x86/Kconfig | 2 +-
> arch/x86/include/asm/hugetlb.h | 4 --
> include/linux/gfp.h | 2 +-
> mm/hugetlb.c | 57 ++++++++++++--------
> mm/page_alloc.c | 4 +-
> 13 files changed, 44 insertions(+), 49 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 091a513b93e9..af687eff884a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -18,7 +18,7 @@ config ARM64
> select ARCH_HAS_FAST_MULTIPLIER
> select ARCH_HAS_FORTIFY_SOURCE
> select ARCH_HAS_GCOV_PROFILE_ALL
> - select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
> + select ARCH_HAS_GIGANTIC_PAGE
> select ARCH_HAS_KCOV
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_PTE_SPECIAL
> diff --git a/arch/arm64/include/asm/hugetlb.h b/arch/arm64/include/asm/hugetlb.h
> index fb6609875455..59893e766824 100644
> --- a/arch/arm64/include/asm/hugetlb.h
> +++ b/arch/arm64/include/asm/hugetlb.h
> @@ -65,8 +65,4 @@ extern void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr,
>
> #include <asm-generic/hugetlb.h>
>
> -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> -static inline bool gigantic_page_supported(void) { return true; }
> -#endif
> -
> #endif /* __ASM_HUGETLB_H */
> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> index 5b0177733994..d04a0bcc2f1c 100644
> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> @@ -32,13 +32,6 @@ static inline int hstate_get_psize(struct hstate *hstate)
> }
> }
>
> -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> -static inline bool gigantic_page_supported(void)
> -{
> - return true;
> -}
> -#endif
> -
> /* hugepd entry valid bit */
> #define HUGEPD_VAL_BITS (0x8000000000000000UL)
>
As explained in https://patchwork.ozlabs.org/patch/1047003/
architectures like ppc64 have a hypervisor assisted mechanism to indicate
where to find gigantic huge pages(16G pages). At this point, we don't use this
reserved pages for anything other than hugetlb backing and hence there
is no runtime free of this pages needed ( Also we don't do
runtime allocation of them).
I guess you can still achieve what you want to do in this patch by
keeping gigantic_page_supported()?
NOTE: We should rename gigantic_page_supported to be more specific to
support for runtime_alloc/free of gigantic pages
-aneesh
^ permalink raw reply
* Re: [PATCH v2] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Christophe Leroy @ 2019-03-14 5:46 UTC (permalink / raw)
To: Alastair D'Silva, alastair
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Naveen N. Rao, linuxppc-dev,
Andrew Morton
In-Reply-To: <20190314023125.10076-1-alastair@au1.ibm.com>
Le 14/03/2019 à 03:31, Alastair D'Silva a écrit :
> From: Alastair D'Silva <alastair@d-silva.org>
>
> When building an LTO kernel, the existing code generates warnings:
> ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> ‘local_paca’ used for multiple global register variables
> register struct paca_struct *local_paca asm("r13");
> ^
> ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> ‘local_paca’
>
> This patch reworks local_paca into an inline getter & setter function,
> which addresses the warning.
>
> Changelog:
> V2
> - Address whitespace issues
> - keep new implementation close to where the old implementation was
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--------
> arch/powerpc/kernel/paca.c | 2 +-
> 2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e843bc5d1a0f..2fa0b43357c9 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -34,19 +34,38 @@
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
>
> -register struct paca_struct *local_paca asm("r13");
> -
> #if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
> -/*
> - * Add standard checks that preemption cannot occur when using get_paca():
> - * otherwise the paca_struct it points to may be the wrong one just after.
> - */
> -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> -#else
> -#define get_paca() local_paca
> #endif
>
> +static inline struct paca_struct *get_paca_no_preempt_check(void)
> +{
> + register struct paca_struct *paca asm("r13");
> +
> + return paca;
> +}
> +
> +static inline struct paca_struct *get_paca(void)
> +{
> +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> + /*
> + * Add standard checks that preemption cannot occur when using get_paca():
> + * otherwise the paca_struct it points to may be the wrong one just after.
> + */
> + debug_smp_processor_id();
> +#endif
> + return get_paca_no_preempt_check();
> +}
> +
> +#define local_paca get_paca_no_preempt_check()
> +
> +static inline void set_paca(struct paca_struct *new)
> +{
> + register struct paca_struct *paca asm("r13");
> +
> + paca = new;
> +}
> +
> #ifdef CONFIG_PPC_PSERIES
> #define get_lppaca() (get_paca()->lppaca_ptr)
> #endif
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 913bfca09c4f..ae5c243f9d5a 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
> void setup_paca(struct paca_struct *new_paca)
> {
> /* Setup r13 */
> - local_paca = new_paca;
> + set_paca(new_paca);
>
> #ifdef CONFIG_PPC_BOOK3E
> /* On Book3E, initialize the TLB miss exception frames */
>
^ permalink raw reply
* Re: [PATCH v6 3/4] mm: Simplify MEMORY_ISOLATION && COMPACTION || CMA into CONTIG_ALLOC
From: Aneesh Kumar K.V @ 2019-03-14 5:41 UTC (permalink / raw)
To: Alexandre Ghiti, Andrew Morton, Vlastimil Babka, Catalin Marinas,
Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Martin Schwidefsky, Heiko Carstens,
Yoshinori Sato, Rich Felker, David S . Miller, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H . Peter Anvin, x86, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Mike Kravetz, linux-arm-kernel,
linux-kernel, linuxppc-dev, linux-s390, linux-sh, sparclinux,
linux-mm
Cc: Alexandre Ghiti
In-Reply-To: <20190307132015.26970-4-alex@ghiti.fr>
Alexandre Ghiti <alex@ghiti.fr> writes:
> This condition allows to define alloc_contig_range, so simplify
> it into a more accurate naming.
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> arch/arm64/Kconfig | 2 +-
> arch/powerpc/platforms/Kconfig.cputype | 2 +-
> arch/s390/Kconfig | 2 +-
> arch/sh/Kconfig | 2 +-
> arch/sparc/Kconfig | 2 +-
> arch/x86/Kconfig | 2 +-
> arch/x86/mm/hugetlbpage.c | 2 +-
> include/linux/gfp.h | 2 +-
> mm/Kconfig | 3 +++
> mm/page_alloc.c | 3 +--
> 10 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a4168d366127..091a513b93e9 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -18,7 +18,7 @@ config ARM64
> select ARCH_HAS_FAST_MULTIPLIER
> select ARCH_HAS_FORTIFY_SOURCE
> select ARCH_HAS_GCOV_PROFILE_ALL
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
> select ARCH_HAS_KCOV
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_PTE_SPECIAL
> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
> index 8c7464c3f27f..f677c8974212 100644
> --- a/arch/powerpc/platforms/Kconfig.cputype
> +++ b/arch/powerpc/platforms/Kconfig.cputype
> @@ -319,7 +319,7 @@ config ARCH_ENABLE_SPLIT_PMD_PTLOCK
> config PPC_RADIX_MMU
> bool "Radix MMU Support"
> depends on PPC_BOOK3S_64
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
> default y
> help
> Enable support for the Power ISA 3.0 Radix style MMU. Currently this
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index ed554b09eb3f..1c57b83c76f5 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -69,7 +69,7 @@ config S390
> select ARCH_HAS_ELF_RANDOMIZE
> select ARCH_HAS_FORTIFY_SOURCE
> select ARCH_HAS_GCOV_PROFILE_ALL
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
> select ARCH_HAS_KCOV
> select ARCH_HAS_PTE_SPECIAL
> select ARCH_HAS_SET_MEMORY
> diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
> index 299a17bed67c..c7266302691c 100644
> --- a/arch/sh/Kconfig
> +++ b/arch/sh/Kconfig
> @@ -53,7 +53,7 @@ config SUPERH
> select HAVE_FUTEX_CMPXCHG if FUTEX
> select HAVE_NMI
> select NEED_SG_DMA_LENGTH
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
>
> help
> The SuperH is a RISC processor targeted for use in embedded systems
> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> index 0b7f0e0fefa5..ca33c80870e2 100644
> --- a/arch/sparc/Kconfig
> +++ b/arch/sparc/Kconfig
> @@ -90,7 +90,7 @@ config SPARC64
> select ARCH_CLOCKSOURCE_DATA
> select ARCH_HAS_PTE_SPECIAL
> select PCI_DOMAINS if PCI
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
>
> config ARCH_DEFCONFIG
> string
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 68261430fe6e..8ba90f3e0038 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -23,7 +23,7 @@ config X86_64
> def_bool y
> depends on 64BIT
> # Options that are inherently 64-bit kernel only:
> - select ARCH_HAS_GIGANTIC_PAGE if (MEMORY_ISOLATION && COMPACTION) || CMA
> + select ARCH_HAS_GIGANTIC_PAGE if CONTIG_ALLOC
> select ARCH_SUPPORTS_INT128
> select ARCH_USE_CMPXCHG_LOCKREF
> select HAVE_ARCH_SOFT_DIRTY
> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
> index 92e4c4b85bba..fab095362c50 100644
> --- a/arch/x86/mm/hugetlbpage.c
> +++ b/arch/x86/mm/hugetlbpage.c
> @@ -203,7 +203,7 @@ static __init int setup_hugepagesz(char *opt)
> }
> __setup("hugepagesz=", setup_hugepagesz);
>
> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> +#ifdef CONFIG_CONTIG_ALLOC
> static __init int gigantic_pages_init(void)
> {
> /* With compaction or CMA we can allocate gigantic pages at runtime */
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 5f5e25fd6149..1f1ad9aeebb9 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -585,7 +585,7 @@ static inline bool pm_suspended_storage(void)
> }
> #endif /* CONFIG_PM_SLEEP */
>
> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> +#ifdef CONFIG_CONTIG_ALLOC
> /* The below functions must be run on a range from a single zone. */
> extern int alloc_contig_range(unsigned long start, unsigned long end,
> unsigned migratetype, gfp_t gfp_mask);
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 25c71eb8a7db..137eadc18732 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -258,6 +258,9 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
> config ARCH_ENABLE_THP_MIGRATION
> bool
>
> +config CONTIG_ALLOC
> + def_bool (MEMORY_ISOLATION && COMPACTION) || CMA
> +
> config PHYS_ADDR_T_64BIT
> def_bool 64BIT
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 35fdde041f5c..ac9c45ffb344 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8024,8 +8024,7 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> return true;
> }
>
> -#if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> -
> +#ifdef CONFIG_CONTIG_ALLOC
> static unsigned long pfn_max_align_down(unsigned long pfn)
> {
> return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES,
> --
> 2.20.1
^ permalink raw reply
* Re: [PATCH 4/5] ocxl: Remove superfluous 'extern' from headers
From: Andrew Donnellan @ 2019-03-14 5:08 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-5-alastair@au1.ibm.com>
On 13/3/19 3:07 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The 'extern' keyword adds no value here.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
> drivers/misc/ocxl/ocxl_internal.h | 54 +++++++++++++++----------------
> include/misc/ocxl.h | 36 ++++++++++-----------
> 2 files changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index a32f2151029f..321b29e77f45 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -16,7 +16,6 @@
>
> extern struct pci_driver ocxl_pci_driver;
>
> -
> struct ocxl_fn {
> struct device dev;
> int bar_used[3];
> @@ -92,41 +91,40 @@ struct ocxl_process_element {
> __be32 software_state;
> };
>
> +struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> +void ocxl_afu_put(struct ocxl_afu *afu);
>
> -extern struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> -extern void ocxl_afu_put(struct ocxl_afu *afu);
> -
> -extern int ocxl_create_cdev(struct ocxl_afu *afu);
> -extern void ocxl_destroy_cdev(struct ocxl_afu *afu);
> -extern int ocxl_register_afu(struct ocxl_afu *afu);
> -extern void ocxl_unregister_afu(struct ocxl_afu *afu);
> +int ocxl_create_cdev(struct ocxl_afu *afu);
> +void ocxl_destroy_cdev(struct ocxl_afu *afu);
> +int ocxl_register_afu(struct ocxl_afu *afu);
> +void ocxl_unregister_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_file_init(void);
> -extern void ocxl_file_exit(void);
> +int ocxl_file_init(void);
> +void ocxl_file_exit(void);
>
> -extern int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> -extern int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
>
> -extern struct ocxl_context *ocxl_context_alloc(void);
> -extern int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> +struct ocxl_context *ocxl_context_alloc(void);
> +int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping);
> -extern int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> -extern int ocxl_context_mmap(struct ocxl_context *ctx,
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> +int ocxl_context_mmap(struct ocxl_context *ctx,
> struct vm_area_struct *vma);
> -extern int ocxl_context_detach(struct ocxl_context *ctx);
> -extern void ocxl_context_detach_all(struct ocxl_afu *afu);
> -extern void ocxl_context_free(struct ocxl_context *ctx);
> +int ocxl_context_detach(struct ocxl_context *ctx);
> +void ocxl_context_detach_all(struct ocxl_afu *afu);
> +void ocxl_context_free(struct ocxl_context *ctx);
>
> -extern int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> -extern void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
> +int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> +void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> -extern int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> -extern void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> -extern int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> +int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> +int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> +void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> +int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> int eventfd);
> -extern u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
> +u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
>
> #endif /* _OCXL_INTERNAL_H_ */
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 9ff6ddc28e22..4544573cc93c 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -53,7 +53,7 @@ struct ocxl_fn_config {
> * Read the configuration space of a function and fill in a
> * ocxl_fn_config structure with all the function details
> */
> -extern int ocxl_config_read_function(struct pci_dev *dev,
> +int ocxl_config_read_function(struct pci_dev *dev,
> struct ocxl_fn_config *fn);
>
> /*
> @@ -62,14 +62,14 @@ extern int ocxl_config_read_function(struct pci_dev *dev,
> * AFU indexes can be sparse, so a driver should check all indexes up
> * to the maximum found in the function description
> */
> -extern int ocxl_config_check_afu_index(struct pci_dev *dev,
> +int ocxl_config_check_afu_index(struct pci_dev *dev,
> struct ocxl_fn_config *fn, int afu_idx);
>
> /*
> * Read the configuration space of a function for the AFU specified by
> * the index 'afu_idx'. Fills in a ocxl_afu_config structure
> */
> -extern int ocxl_config_read_afu(struct pci_dev *dev,
> +int ocxl_config_read_afu(struct pci_dev *dev,
> struct ocxl_fn_config *fn,
> struct ocxl_afu_config *afu,
> u8 afu_idx);
> @@ -77,7 +77,7 @@ extern int ocxl_config_read_afu(struct pci_dev *dev,
> /*
> * Get the max PASID value that can be used by the function
> */
> -extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
>
> /*
> * Tell an AFU, by writing in the configuration space, the PASIDs that
> @@ -87,7 +87,7 @@ extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> * 'afu_control_offset' is the offset of the AFU control DVSEC which
> * can be found in the function configuration
> */
> -extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> +void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> int afu_control_offset,
> int pasid_base, u32 pasid_count_log);
>
> @@ -98,7 +98,7 @@ extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> * 'supported' is the total number of actags desired by all the AFUs
> * of the function.
> */
> -extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> +int ocxl_config_get_actag_info(struct pci_dev *dev,
> u16 *base, u16 *enabled, u16 *supported);
>
> /*
> @@ -108,7 +108,7 @@ extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> * 'func_offset' is the offset of the Function DVSEC that can found in
> * the function configuration
> */
> -extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> +void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> u32 actag_base, u32 actag_count);
>
> /*
> @@ -118,7 +118,7 @@ extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> +void ocxl_config_set_afu_actag(struct pci_dev *dev,
> int afu_control_offset,
> int actag_base, int actag_count);
>
> @@ -128,7 +128,7 @@ extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> +void ocxl_config_set_afu_state(struct pci_dev *dev,
> int afu_control_offset, int enable);
>
> /*
> @@ -139,7 +139,7 @@ extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> * between the host and device, and set the Transaction Layer on both
> * accordingly.
> */
> -extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> +int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
>
> /*
> * Request an AFU to terminate a PASID.
> @@ -152,7 +152,7 @@ extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> +int ocxl_config_terminate_pasid(struct pci_dev *dev,
> int afu_control_offset, int pasid);
>
> /*
> @@ -165,13 +165,13 @@ extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> * Returns a 'link handle' that should be used for further calls for
> * the link
> */
> -extern int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> +int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> void **link_handle);
>
> /*
> * Remove the association between the function and its link.
> */
> -extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> +void ocxl_link_release(struct pci_dev *dev, void *link_handle);
>
> /*
> * Add a Process Element to the Shared Process Area for a link.
> @@ -183,7 +183,7 @@ extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> * 'xsl_err_data' is an argument passed to the above callback, if
> * defined
> */
> -extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> +int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> u64 amr, struct mm_struct *mm,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
> @@ -195,12 +195,12 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> * pasid: the PASID for the AFU context
> * tid: the new thread id for the process element
> */
> -extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
>
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
> -extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> +int ocxl_link_remove_pe(void *link_handle, int pasid);
>
> /*
> * Allocate an AFU interrupt associated to the link.
> @@ -212,12 +212,12 @@ extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> * interrupt. It is an MMIO address which needs to be remapped (one
> * page).
> */
> -extern int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> +int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> u64 *obj_handle);
>
> /*
> * Free a previously allocated AFU interrupt
> */
> -extern void ocxl_link_free_irq(void *link_handle, int hw_irq);
> +void ocxl_link_free_irq(void *link_handle, int hw_irq);
>
> #endif /* _MISC_OCXL_H_ */
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH 3/5] ocxl: read_pasid never returns an error, so make it void
From: Andrew Donnellan @ 2019-03-14 4:59 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, Greg Kurz, linux-kernel,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-4-alastair@au1.ibm.com>
On 13/3/19 3:06 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> No need for a return value in read_pasid as it only returns 0.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
> drivers/misc/ocxl/config.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index 0ee7856b033d..026ac2ac4f9c 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -68,7 +68,7 @@ static int find_dvsec_afu_ctrl(struct pci_dev *dev, u8 afu_idx)
> return 0;
> }
>
> -static int read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
> +static void read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
> {
> u16 val;
> int pos;
> @@ -89,7 +89,6 @@ static int read_pasid(struct pci_dev *dev, struct ocxl_fn_config *fn)
> out:
> dev_dbg(&dev->dev, "PASID capability:\n");
> dev_dbg(&dev->dev, " Max PASID log = %d\n", fn->max_pasid_log);
> - return 0;
> }
>
> static int read_dvsec_tl(struct pci_dev *dev, struct ocxl_fn_config *fn)
> @@ -205,11 +204,7 @@ int ocxl_config_read_function(struct pci_dev *dev, struct ocxl_fn_config *fn)
> {
> int rc;
>
> - rc = read_pasid(dev, fn);
> - if (rc) {
> - dev_err(&dev->dev, "Invalid PASID configuration: %d\n", rc);
> - return -ENODEV;
> - }
> + read_pasid(dev, fn);
>
> rc = read_dvsec_tl(dev, fn);
> if (rc) {
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH 2/5] ocxl: Clean up printf formats
From: Andrew Donnellan @ 2019-03-14 4:58 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel,
Alastair D'Silva, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313040702.14276-3-alastair@au1.ibm.com>
On 13/3/19 3:06 pm, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Use %# instead of using a literal '0x'
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
Acked-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
> drivers/misc/ocxl/config.c | 6 +++---
> drivers/misc/ocxl/context.c | 2 +-
> drivers/misc/ocxl/trace.h | 10 +++++-----
> 3 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index 8f2c5d8bd2ee..0ee7856b033d 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -178,9 +178,9 @@ static int read_dvsec_vendor(struct pci_dev *dev)
> pci_read_config_dword(dev, pos + OCXL_DVSEC_VENDOR_DLX_VERS, &dlx);
>
> dev_dbg(&dev->dev, "Vendor specific DVSEC:\n");
> - dev_dbg(&dev->dev, " CFG version = 0x%x\n", cfg);
> - dev_dbg(&dev->dev, " TLX version = 0x%x\n", tlx);
> - dev_dbg(&dev->dev, " DLX version = 0x%x\n", dlx);
> + dev_dbg(&dev->dev, " CFG version = %#x\n", cfg);
> + dev_dbg(&dev->dev, " TLX version = %#x\n", tlx);
> + dev_dbg(&dev->dev, " DLX version = %#x\n", dlx);
> return 0;
> }
>
> diff --git a/drivers/misc/ocxl/context.c b/drivers/misc/ocxl/context.c
> index c10a940e3b38..3498a0199bde 100644
> --- a/drivers/misc/ocxl/context.c
> +++ b/drivers/misc/ocxl/context.c
> @@ -134,7 +134,7 @@ static vm_fault_t ocxl_mmap_fault(struct vm_fault *vmf)
> vm_fault_t ret;
>
> offset = vmf->pgoff << PAGE_SHIFT;
> - pr_debug("%s: pasid %d address 0x%lx offset 0x%llx\n", __func__,
> + pr_debug("%s: pasid %d address %#lx offset %#llx\n", __func__,
> ctx->pasid, vmf->address, offset);
>
> if (offset < ctx->afu->irq_base_offset)
> diff --git a/drivers/misc/ocxl/trace.h b/drivers/misc/ocxl/trace.h
> index bcb7ff330c1e..68bf2f173a1a 100644
> --- a/drivers/misc/ocxl/trace.h
> +++ b/drivers/misc/ocxl/trace.h
> @@ -28,7 +28,7 @@ DECLARE_EVENT_CLASS(ocxl_context,
> __entry->tidr = tidr;
> ),
>
> - TP_printk("linux pid=%d spa=0x%p pasid=0x%x pidr=0x%x tidr=0x%x",
> + TP_printk("linux pid=%d spa=%p pasid=%#x pidr=%#x tidr=%#x",
> __entry->pid,
> __entry->spa,
> __entry->pasid,
> @@ -61,7 +61,7 @@ TRACE_EVENT(ocxl_terminate_pasid,
> __entry->rc = rc;
> ),
>
> - TP_printk("pasid=0x%x rc=%d",
> + TP_printk("pasid=%#x rc=%d",
> __entry->pasid,
> __entry->rc
> )
> @@ -87,7 +87,7 @@ DECLARE_EVENT_CLASS(ocxl_fault_handler,
> __entry->tfc = tfc;
> ),
>
> - TP_printk("spa=%p pe=0x%llx dsisr=0x%llx dar=0x%llx tfc=0x%llx",
> + TP_printk("spa=%p pe=%#llx dsisr=%#llx dar=%#llx tfc=%#llx",
> __entry->spa,
> __entry->pe,
> __entry->dsisr,
> @@ -127,7 +127,7 @@ TRACE_EVENT(ocxl_afu_irq_alloc,
> __entry->irq_offset = irq_offset;
> ),
>
> - TP_printk("pasid=0x%x irq_id=%d virq=%u hw_irq=%d irq_offset=0x%llx",
> + TP_printk("pasid=%#x irq_id=%d virq=%u hw_irq=%d irq_offset=%#llx",
> __entry->pasid,
> __entry->irq_id,
> __entry->virq,
> @@ -150,7 +150,7 @@ TRACE_EVENT(ocxl_afu_irq_free,
> __entry->irq_id = irq_id;
> ),
>
> - TP_printk("pasid=0x%x irq_id=%d",
> + TP_printk("pasid=%#x irq_id=%d",
> __entry->pasid,
> __entry->irq_id
> )
>
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply
* [PATCH] powerpc/powernv: Squash sparse warnings in opal-call.c
From: Andrew Donnellan @ 2019-03-14 4:27 UTC (permalink / raw)
To: linuxppc-dev, npiggin
sparse complains a lot about opal-call.c:
arch/powerpc/platforms/powernv/opal-call.c:128:1: warning: symbol 'opal_invalid_call' was not declared. Should it be static?
arch/powerpc/platforms/powernv/opal-call.c:129:1: warning: symbol 'opal_console_write' was not declared. Should it be static?
arch/powerpc/platforms/powernv/opal-call.c:130:1: warning: symbol 'opal_console_read' was not declared. Should it be static?
Those symbols are forward declared in opal.h, but we can't include that
because the function signatures in opal.h are different. So instead, just
add an extra forward declaration to the OPAL_CALL macro to shut sparse up.
Signed-off-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
---
arch/powerpc/platforms/powernv/opal-call.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
index 578757d403ab..9d8e43b2ebfd 100644
--- a/arch/powerpc/platforms/powernv/opal-call.c
+++ b/arch/powerpc/platforms/powernv/opal-call.c
@@ -120,6 +120,8 @@ static int64_t opal_call(int64_t a0, int64_t a1, int64_t a2, int64_t a3,
#define OPAL_CALL(name, opcode) \
int64_t name(int64_t a0, int64_t a1, int64_t a2, int64_t a3, \
+ int64_t a4, int64_t a5, int64_t a6, int64_t a7); \
+int64_t name(int64_t a0, int64_t a1, int64_t a2, int64_t a3, \
int64_t a4, int64_t a5, int64_t a6, int64_t a7) \
{ \
return opal_call(a0, a1, a2, a3, a4, a5, a6, a7, opcode); \
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Dan Williams @ 2019-03-14 4:02 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Ross Zwisler, Andrew Morton, linuxppc-dev, Kirill A . Shutemov
In-Reply-To: <871s3aqfup.fsf@linux.ibm.com>
On Wed, Mar 13, 2019 at 8:45 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
[..]
> >> Now w.r.t to failures, can device-dax do an opportunistic huge page
> >> usage?
> >
> > device-dax explicitly disclaims the ability to do opportunistic mappings.
> >
> >> I haven't looked at the device-dax details fully yet. Do we make the
> >> assumption of the mapping page size as a format w.r.t device-dax? Is that
> >> derived from nd_pfn->align value?
> >
> > Correct.
> >
> >>
> >> Here is what I am working on:
> >> 1) If the platform doesn't support huge page and if the device superblock
> >> indicated that it was created with huge page support, we fail the device
> >> init.
> >
> > Ok.
> >
> >> 2) Now if we are creating a new namespace without huge page support in
> >> the platform, then we force the align details to PAGE_SIZE. In such a
> >> configuration when handling dax fault even with THP enabled during
> >> the build, we should not try to use hugepage. This I think we can
> >> achieve by using TRANSPARENT_HUGEPAEG_DAX_FLAG.
> >
> > How is this dynamic property communicated to the guest?
>
> via device tree on powerpc. We have a device tree node indicating
> supported page sizes.
Ah, ok, yeah let's plumb that straight to the device-dax driver and
leave out the interaction / interpretation of the thp-enabled flags.
>
> >
> >>
> >> Also even if the user decided to not use THP, by
> >> echo "never" > transparent_hugepage/enabled , we should continue to map
> >> dax fault using huge page on platforms that can support huge pages.
> >>
> >> This still doesn't cover the details of a device-dax created with
> >> PAGE_SIZE align later booted with a kernel that can do hugepage dax.How
> >> should we handle that? That makes me think, this should be a VMA flag
> >> which got derived from device config? May be use VM_HUGEPAGE to indicate
> >> if device should use a hugepage mapping or not?
> >
> > device-dax configured with PAGE_SIZE always gets PAGE_SIZE mappings.
>
> Now what will be page size used for mapping vmemmap?
That's up to the architecture's vmemmap_populate() implementation.
> Architectures
> possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
> device-dax with struct page in the device will have pfn reserve area aligned
> to PAGE_SIZE with the above example? We can't map that using
> PMD_SIZE page size?
IIUC, that's a different alignment. Currently that's handled by
padding the reservation area up to a section (128MB on x86) boundary,
but I'm working on patches to allow sub-section sized ranges to be
mapped.
Now, that said, I expect there may be bugs lurking in the
implementation if PAGE_SIZE changes from one boot to the next simply
because I've never tested that.
I think this also indicates that the section padding logic can't be
removed until all arch vmemmap_populate() implementations understand
the sub-section case.
^ permalink raw reply
* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: David Gibson @ 2019-03-14 3:29 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <badcd1dc-2eef-590e-b362-0d3dfdf52d34@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] mm/dax: Don't enable huge dax mapping by default
From: Aneesh Kumar K.V @ 2019-03-14 3:45 UTC (permalink / raw)
To: Dan Williams
Cc: Jan Kara, linux-nvdimm, Linux Kernel Mailing List, Linux MM,
Ross Zwisler, Andrew Morton, linuxppc-dev, Kirill A . Shutemov
In-Reply-To: <CAPcyv4ir4irASBQrZD_a6kMkEUt=XPUCuKajF75O7wDCgeG=7Q@mail.gmail.com>
Dan Williams <dan.j.williams@intel.com> writes:
> On Wed, Mar 6, 2019 at 1:18 AM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> Dan Williams <dan.j.williams@intel.com> writes:
>>
>> > On Thu, Feb 28, 2019 at 1:40 AM Oliver <oohall@gmail.com> wrote:
>> >>
>> >> On Thu, Feb 28, 2019 at 7:35 PM Aneesh Kumar K.V
>> >> <aneesh.kumar@linux.ibm.com> wrote:
>> >> >
>> >> > Add a flag to indicate the ability to do huge page dax mapping. On architecture
>> >> > like ppc64, the hypervisor can disable huge page support in the guest. In
>> >> > such a case, we should not enable huge page dax mapping. This patch adds
>> >> > a flag which the architecture code will update to indicate huge page
>> >> > dax mapping support.
>> >>
>> >> *groan*
>> >>
>> >> > Architectures mostly do transparent_hugepage_flag = 0; if they can't
>> >> > do hugepages. That also takes care of disabling dax hugepage mapping
>> >> > with this change.
>> >> >
>> >> > Without this patch we get the below error with kvm on ppc64.
>> >> >
>> >> > [ 118.849975] lpar: Failed hash pte insert with error -4
>> >> >
>> >> > NOTE: The patch also use
>> >> >
>> >> > echo never > /sys/kernel/mm/transparent_hugepage/enabled
>> >> > to disable dax huge page mapping.
>> >> >
>> >> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> >> > ---
>> >> > TODO:
>> >> > * Add Fixes: tag
>> >> >
>> >> > include/linux/huge_mm.h | 4 +++-
>> >> > mm/huge_memory.c | 4 ++++
>> >> > 2 files changed, 7 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> >> > index 381e872bfde0..01ad5258545e 100644
>> >> > --- a/include/linux/huge_mm.h
>> >> > +++ b/include/linux/huge_mm.h
>> >> > @@ -53,6 +53,7 @@ vm_fault_t vmf_insert_pfn_pud(struct vm_area_struct *vma, unsigned long addr,
>> >> > pud_t *pud, pfn_t pfn, bool write);
>> >> > enum transparent_hugepage_flag {
>> >> > TRANSPARENT_HUGEPAGE_FLAG,
>> >> > + TRANSPARENT_HUGEPAGE_DAX_FLAG,
>> >> > TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
>> >> > TRANSPARENT_HUGEPAGE_DEFRAG_DIRECT_FLAG,
>> >> > TRANSPARENT_HUGEPAGE_DEFRAG_KSWAPD_FLAG,
>> >> > @@ -111,7 +112,8 @@ static inline bool __transparent_hugepage_enabled(struct vm_area_struct *vma)
>> >> > if (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_FLAG))
>> >> > return true;
>> >> >
>> >> > - if (vma_is_dax(vma))
>> >> > + if (vma_is_dax(vma) &&
>> >> > + (transparent_hugepage_flags & (1 << TRANSPARENT_HUGEPAGE_DAX_FLAG)))
>> >> > return true;
>> >>
>> >> Forcing PTE sized faults should be fine for fsdax, but it'll break
>> >> devdax. The devdax driver requires the fault size be >= the namespace
>> >> alignment since devdax tries to guarantee hugepage mappings will be
>> >> used and PMD alignment is the default. We can probably have devdax
>> >> fall back to the largest size the hypervisor has made available, but
>> >> it does run contrary to the design. Ah well, I suppose it's better off
>> >> being degraded rather than unusable.
>> >
>> > Given this is an explicit setting I think device-dax should explicitly
>> > fail to enable in the presence of this flag to preserve the
>> > application visible behavior.
>> >
>> > I.e. if device-dax was enabled after this setting was made then I
>> > think future faults should fail as well.
>>
>> Not sure I understood that. Now we are disabling the ability to map
>> pages as huge pages. I am now considering that this should not be
>> user configurable. Ie, this is something that platform can use to avoid
>> dax forcing huge page mapping, but if the architecture can enable huge
>> dax mapping, we should always default to using that.
>
> No, that's an application visible behavior regression. The side effect
> of this setting is that all huge-page configured device-dax instances
> must be disabled.
So if the device was created with a nd_pfn->align value of PMD_SIZE, that is
an indication that we would map the pages in PMD_SIZE?
Ok with that understanding, If the align value is not a supported
mapping size, we fail initializing the device.
>
>> Now w.r.t to failures, can device-dax do an opportunistic huge page
>> usage?
>
> device-dax explicitly disclaims the ability to do opportunistic mappings.
>
>> I haven't looked at the device-dax details fully yet. Do we make the
>> assumption of the mapping page size as a format w.r.t device-dax? Is that
>> derived from nd_pfn->align value?
>
> Correct.
>
>>
>> Here is what I am working on:
>> 1) If the platform doesn't support huge page and if the device superblock
>> indicated that it was created with huge page support, we fail the device
>> init.
>
> Ok.
>
>> 2) Now if we are creating a new namespace without huge page support in
>> the platform, then we force the align details to PAGE_SIZE. In such a
>> configuration when handling dax fault even with THP enabled during
>> the build, we should not try to use hugepage. This I think we can
>> achieve by using TRANSPARENT_HUGEPAEG_DAX_FLAG.
>
> How is this dynamic property communicated to the guest?
via device tree on powerpc. We have a device tree node indicating
supported page sizes.
>
>>
>> Also even if the user decided to not use THP, by
>> echo "never" > transparent_hugepage/enabled , we should continue to map
>> dax fault using huge page on platforms that can support huge pages.
>>
>> This still doesn't cover the details of a device-dax created with
>> PAGE_SIZE align later booted with a kernel that can do hugepage dax.How
>> should we handle that? That makes me think, this should be a VMA flag
>> which got derived from device config? May be use VM_HUGEPAGE to indicate
>> if device should use a hugepage mapping or not?
>
> device-dax configured with PAGE_SIZE always gets PAGE_SIZE mappings.
Now what will be page size used for mapping vmemmap? Architectures
possibly will use PMD_SIZE mapping if supported for vmemmap. Now a
device-dax with struct page in the device will have pfn reserve area aligned
to PAGE_SIZE with the above example? We can't map that using
PMD_SIZE page size?
-aneesh
^ permalink raw reply
* Re: [PATCH v2 10/16] KVM: PPC: Book3S HV: XIVE: add get/set accessors for the VP XIVE state
From: David Gibson @ 2019-03-14 3:09 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <d33a2cb1-5370-7e72-07d9-ed0028c85b8c@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 11922 bytes --]
On Wed, Mar 13, 2019 at 02:19:13PM +0100, Cédric Le Goater wrote:
> On 2/25/19 4:31 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:34PM +0100, Cédric Le Goater wrote:
> >> At a VCPU level, the state of the thread interrupt management
> >> registers needs to be collected. These registers are cached under the
> >> 'xive_saved_state.w01' field of the VCPU when the VPCU context is
> >> pulled from the HW thread. An OPAL call retrieves the backup of the
> >> IPB register in the underlying XIVE NVT structure and merges it in the
> >> KVM state.
> >>
> >> The structures of the interface between QEMU and KVM provisions some
> >> extra room (two u64) for further extensions if more state needs to be
> >> transferred back to QEMU.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> arch/powerpc/include/asm/kvm_ppc.h | 11 +++
> >> arch/powerpc/include/uapi/asm/kvm.h | 2 +
> >> arch/powerpc/kvm/book3s.c | 24 +++++++
> >> arch/powerpc/kvm/book3s_xive_native.c | 82 ++++++++++++++++++++++
> >> Documentation/virtual/kvm/devices/xive.txt | 19 +++++
> >> 5 files changed, 138 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> >> index 1e61877fe147..664c65051612 100644
> >> --- a/arch/powerpc/include/asm/kvm_ppc.h
> >> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> >> @@ -272,6 +272,7 @@ union kvmppc_one_reg {
> >> u64 addr;
> >> u64 length;
> >> } vpaval;
> >> + u64 xive_timaval[4];
> >
> > This is doubling the size of the userspace visible one_reg union. Is
> > that safe?
>
> 'safe' as in compatibility on an older KVM which would still use the old
> kvmppc_one_reg definition ?
I was more thinking of old qemu with a new kernel.
> It should be fine as KVM_REG_PPC_VP_STATE would not be handled. Am I
> wrong ?
Looks like it should be ok, because we only partially copy the
structure to/from userspace due to the one_reg_size() logic. If the
whole union was always copied, it would be hilariously unsafe.
>
> >> };
> >>
> >> struct kvmppc_ops {
> >> @@ -604,6 +605,10 @@ extern int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> extern void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu);
> >> extern void kvmppc_xive_native_init_module(void);
> >> extern void kvmppc_xive_native_exit_module(void);
> >> +extern int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
> >> + union kvmppc_one_reg *val);
> >> +extern int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
> >> + union kvmppc_one_reg *val);
> >>
> >> #else
> >> static inline int kvmppc_xive_set_xive(struct kvm *kvm, u32 irq, u32 server,
> >> @@ -636,6 +641,12 @@ static inline int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> static inline void kvmppc_xive_native_cleanup_vcpu(struct kvm_vcpu *vcpu) { }
> >> static inline void kvmppc_xive_native_init_module(void) { }
> >> static inline void kvmppc_xive_native_exit_module(void) { }
> >> +static inline int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu,
> >> + union kvmppc_one_reg *val)
> >> +{ return 0; }
> >> +static inline int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu,
> >> + union kvmppc_one_reg *val)
> >> +{ return -ENOENT; }
> >>
> >> #endif /* CONFIG_KVM_XIVE */
> >>
> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >> index cd78ad1020fe..42d4ef93ec2d 100644
> >> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >> @@ -480,6 +480,8 @@ struct kvm_ppc_cpu_char {
> >> #define KVM_REG_PPC_ICP_PPRI_SHIFT 16 /* pending irq priority */
> >> #define KVM_REG_PPC_ICP_PPRI_MASK 0xff
> >>
> >> +#define KVM_REG_PPC_VP_STATE (KVM_REG_PPC | KVM_REG_SIZE_U256 | 0x8d)
> >> +
> >> /* Device control API: PPC-specific devices */
> >> #define KVM_DEV_MPIC_GRP_MISC 1
> >> #define KVM_DEV_MPIC_BASE_ADDR 0 /* 64-bit */
> >> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> >> index 96d43f091255..f85a9211f30c 100644
> >> --- a/arch/powerpc/kvm/book3s.c
> >> +++ b/arch/powerpc/kvm/book3s.c
> >> @@ -641,6 +641,18 @@ int kvmppc_get_one_reg(struct kvm_vcpu *vcpu, u64 id,
> >> *val = get_reg_val(id, kvmppc_xics_get_icp(vcpu));
> >> break;
> >> #endif /* CONFIG_KVM_XICS */
> >> +#ifdef CONFIG_KVM_XIVE
> >> + case KVM_REG_PPC_VP_STATE:
> >> + if (!vcpu->arch.xive_vcpu) {
> >> + r = -ENXIO;
> >> + break;
> >> + }
> >> + if (xive_enabled())
> >> + r = kvmppc_xive_native_get_vp(vcpu, val);
> >> + else
> >> + r = -ENXIO;
> >> + break;
> >> +#endif /* CONFIG_KVM_XIVE */
> >> case KVM_REG_PPC_FSCR:
> >> *val = get_reg_val(id, vcpu->arch.fscr);
> >> break;
> >> @@ -714,6 +726,18 @@ int kvmppc_set_one_reg(struct kvm_vcpu *vcpu, u64 id,
> >> r = kvmppc_xics_set_icp(vcpu, set_reg_val(id, *val));
> >> break;
> >> #endif /* CONFIG_KVM_XICS */
> >> +#ifdef CONFIG_KVM_XIVE
> >> + case KVM_REG_PPC_VP_STATE:
> >> + if (!vcpu->arch.xive_vcpu) {
> >> + r = -ENXIO;
> >> + break;
> >> + }
> >> + if (xive_enabled())
> >> + r = kvmppc_xive_native_set_vp(vcpu, val);
> >> + else
> >> + r = -ENXIO;
> >> + break;
> >> +#endif /* CONFIG_KVM_XIVE */
> >> case KVM_REG_PPC_FSCR:
> >> vcpu->arch.fscr = set_reg_val(id, *val);
> >> break;
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> index 3debc876d5a0..132bff52d70a 100644
> >> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -845,6 +845,88 @@ static int kvmppc_xive_native_create(struct kvm_device *dev, u32 type)
> >> return ret;
> >> }
> >>
> >> +/*
> >> + * Interrupt Pending Buffer (IPB) offset
> >> + */
> >> +#define TM_IPB_SHIFT 40
> >> +#define TM_IPB_MASK (((u64) 0xFF) << TM_IPB_SHIFT)
> >> +
> >> +int kvmppc_xive_native_get_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
> >> +{
> >> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> + u64 opal_state;
> >> + int rc;
> >> +
> >> + if (!kvmppc_xive_enabled(vcpu))
> >> + return -EPERM;
> >> +
> >> + if (!xc)
> >> + return -ENOENT;
> >> +
> >> + /* Thread context registers. We only care about IPB and CPPR */
> >> + val->xive_timaval[0] = vcpu->arch.xive_saved_state.w01;
> >> +
> >> + /*
> >> + * Return the OS CAM line to print out the VP identifier in
> >> + * the QEMU monitor. This is not restored.
> >> + */
> >> + val->xive_timaval[1] = vcpu->arch.xive_cam_word;
> >
> > I'm pretty dubious about this mixing of vital state information with
> > what's basically debug information.
>
> I think QEMU deserves to know about the OS CAM line value. I was even
> thinking about adding the POOL CAM line value for future use (nested)
>
> > Doubly so since it requires changing the ABI to increase
> > the one_reg union's size.
>
> OK. That's one argument.
>
> > Might be better to have this control only return the 0th and 2nd u64s
> > from the TIMA, with the CAM debug information returned via some other
> > mechanism.
>
> Like an extra reg : KVM_REG_PPC_VP_CAM ?
That would be the obvious choice, yes.
> >> +
> >> + /* Get the VP state from OPAL */
> >> + rc = xive_native_get_vp_state(xc->vp_id, &opal_state);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + /*
> >> + * Capture the backup of IPB register in the NVT structure and
> >> + * merge it in our KVM VP state.
> >> + */
> >> + val->xive_timaval[0] |= cpu_to_be64(opal_state & TM_IPB_MASK);
> >> +
> >> + pr_devel("%s NSR=%02x CPPR=%02x IBP=%02x PIPR=%02x w01=%016llx w2=%08x opal=%016llx\n",
> >> + __func__,
> >> + vcpu->arch.xive_saved_state.nsr,
> >> + vcpu->arch.xive_saved_state.cppr,
> >> + vcpu->arch.xive_saved_state.ipb,
> >> + vcpu->arch.xive_saved_state.pipr,
> >> + vcpu->arch.xive_saved_state.w01,
> >> + (u32) vcpu->arch.xive_cam_word, opal_state);
> >
> > Hrm.. except you don't seem to be using the last half of the timaval
> > field anyway.
>
> Yes. The two u64 are extras. We can do without.
>
> Would that be ok if I stored the w01 regs in the first u64, the CAM line(s)
> in the second and remove the extra two u64 ?
I'd still prefer them in separate regs. They kind of belong to
different categories of information, and I can't think of any
particular reason you'd have to update or fetch them as a unit.
>
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +int kvmppc_xive_native_set_vp(struct kvm_vcpu *vcpu, union kvmppc_one_reg *val)
> >> +{
> >> + struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> + struct kvmppc_xive *xive = vcpu->kvm->arch.xive;
> >> +
> >> + pr_devel("%s w01=%016llx vp=%016llx\n", __func__,
> >> + val->xive_timaval[0], val->xive_timaval[1]);
> >> +
> >> + if (!kvmppc_xive_enabled(vcpu))
> >> + return -EPERM;
> >> +
> >> + if (!xc || !xive)
> >> + return -ENOENT;
> >> +
> >> + /* We can't update the state of a "pushed" VCPU */
> >> + if (WARN_ON(vcpu->arch.xive_pushed))
> >
> > What prevents userspace from tripping this WARN_ON()?
>
> if the vCPU is executing a vCPU ioctl, it means that it exited the guest
> and that its interrupt context has been pulled out of XIVE.
But couldn't one user thread call the vcpu ioctl() while another is
inside the guest?
> >> + return -EIO;
> >
> > EBUSY might be more appropriate here.
>
> OK.
>
> Thanks,
>
> C.
>
> >
> >> +
> >> + /*
> >> + * Restore the thread context registers. IPB and CPPR should
> >> + * be the only ones that matter.
> >> + */
> >> + vcpu->arch.xive_saved_state.w01 = val->xive_timaval[0];
> >> +
> >> + /*
> >> + * There is no need to restore the XIVE internal state (IPB
> >> + * stored in the NVT) as the IPB register was merged in KVM VP
> >> + * state when captured.
> >> + */
> >> + return 0;
> >> +}
> >> +
> >> static int xive_native_debug_show(struct seq_file *m, void *private)
> >> {
> >> struct kvmppc_xive *xive = m->private;
> >> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
> >> index a26be635cff9..1b8957c50c53 100644
> >> --- a/Documentation/virtual/kvm/devices/xive.txt
> >> +++ b/Documentation/virtual/kvm/devices/xive.txt
> >> @@ -102,6 +102,25 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
> >> -EINVAL: Not initialized source number, invalid priority or
> >> invalid CPU number.
> >>
> >> +* VCPU state
> >> +
> >> + The XIVE IC maintains VP interrupt state in an internal structure
> >> + called the NVT. When a VP is not dispatched on a HW processor
> >> + thread, this structure can be updated by HW if the VP is the target
> >> + of an event notification.
> >> +
> >> + It is important for migration to capture the cached IPB from the NVT
> >> + as it synthesizes the priorities of the pending interrupts. We
> >> + capture a bit more to report debug information.
> >> +
> >> + KVM_REG_PPC_VP_STATE (4 * 64bits)
> >> + bits: | 63 .... 32 | 31 .... 0 |
> >> + values: | TIMA word0 | TIMA word1 |
> >> + bits: | 127 .......... 64 |
> >> + values: | VP CAM Line |
> >> + bits: | 255 .......... 128 |
> >> + values: | unused |
> >> +
> >> * Migration:
> >>
> >> Saving the state of a VM using the XIVE native exploitation mode
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Alastair D'Silva @ 2019-03-14 3:09 UTC (permalink / raw)
To: Daniel Axtens
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Naveen N. Rao, linuxppc-dev,
Andrew Morton
In-Reply-To: <87ef7atjnk.fsf@dja-thinkpad.axtens.net>
On Thu, 2019-03-14 at 10:54 +1100, Daniel Axtens wrote:
> "Alastair D'Silva" <alastair@au1.ibm.com> writes:
>
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > When building an LTO kernel, the existing code generates warnings:
> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> > ‘local_paca’ used for multiple global register variables
> > register struct paca_struct *local_paca asm("r13");
> > ^
> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> > ‘local_paca’
> >
> > This patch reworks local_paca into an inline getter & setter
> > function,
> > which addresses the warning.
> >
> > Generated ASM from this patch is broadly similar (addresses have
> > changed and the compiler uses different GPRs in some places).
>
> Ditto to Christophe's comment; I'd love to know how to build this so
> I
> can actually see the differences. Perhaps you could bundle up all the
> required changes and send it as a patch series with a cover letter
> explaining this?
The differences are visible in a normal build, but if you want to play
with LTO, see my comments to Christophe.
>
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
> > + return paca;
> > +}
>
> Isn't the convention to have the { on the same line as the function,
> or
> am I horrible mis-remembering things?
>
You are :)
> Should these functions be __always_inline?
>
Yes, they should, I'll add that to V3.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH v2 03/16] KVM: PPC: Book3S HV: XIVE: introduce a new capability KVM_CAP_PPC_IRQ_XIVE
From: David Gibson @ 2019-03-14 2:29 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, linuxppc-dev
In-Reply-To: <a736c1b9-6406-2afa-d126-d0073fcd096e@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 1264 bytes --]
On Wed, Mar 13, 2019 at 09:34:53AM +0100, Cédric Le Goater wrote:
> On 2/25/19 5:35 AM, Paul Mackerras wrote:
> > On Fri, Feb 22, 2019 at 12:28:27PM +0100, Cédric Le Goater wrote:
> >> The user interface exposes a new capability to let QEMU connect the
> >> vCPU to the XIVE KVM device if required. The capability is only
> >> advertised on a PowerNV Hypervisor as support for nested guests
> >> (pseries KVM Hypervisor) is not yet available.
> >
> > If a bisection happened to land on this commit, we would have KVM
> > saying it had the ability to support guests using XIVE natively, but
> > it wouldn't actually work since we don't have all the code that is in
> > the following patches.
>
> OK. I didn't think migration was a must-have for bisection. I will move
> the enablement at end.
Any temporary feature regression potentially breaks bisection, because
you don't know what we'll want to bisect for. Obviously we're never
going to get that perfectly right, but that doesn't mean we shouldn't
try when we do see the problem in advance.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 04/16] KVM: PPC: Book3S HV: XIVE: add a control to initialize a source
From: David Gibson @ 2019-03-14 2:15 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <6974050a-cc11-b69d-f35b-9a076549bdd0@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 12358 bytes --]
On Tue, Mar 12, 2019 at 04:19:35PM +0100, Cédric Le Goater wrote:
> On 2/25/19 3:10 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:28PM +0100, Cédric Le Goater wrote:
> >> The associated HW interrupt source is simply allocated at the OPAL/HW
> >> level and then MASKED. KVM only needs to know about its type: LSI or
> >> MSI.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> arch/powerpc/include/uapi/asm/kvm.h | 5 +
> >> arch/powerpc/kvm/book3s_xive.h | 10 ++
> >> arch/powerpc/kvm/book3s_xive.c | 8 +-
> >> arch/powerpc/kvm/book3s_xive_native.c | 114 +++++++++++++++++++++
> >> Documentation/virtual/kvm/devices/xive.txt | 15 +++
> >> 5 files changed, 148 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >> index b002c0c67787..a9ad99f2a11b 100644
> >> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >> @@ -677,5 +677,10 @@ struct kvm_ppc_cpu_char {
> >>
> >> /* POWER9 XIVE Native Interrupt Controller */
> >> #define KVM_DEV_XIVE_GRP_CTRL 1
> >> +#define KVM_DEV_XIVE_GRP_SOURCE 2 /* 64-bit source attributes */
> >> +
> >> +/* Layout of 64-bit XIVE source attribute values */
> >> +#define KVM_XIVE_LEVEL_SENSITIVE (1ULL << 0)
> >> +#define KVM_XIVE_LEVEL_ASSERTED (1ULL << 1)
> >>
> >> #endif /* __LINUX_KVM_POWERPC_H */
> >> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> >> index bcb1bbcf0359..f22f2d46d0f0 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.h
> >> +++ b/arch/powerpc/kvm/book3s_xive.h
> >> @@ -12,6 +12,13 @@
> >> #ifdef CONFIG_KVM_XICS
> >> #include "book3s_xics.h"
> >>
> >> +/*
> >> + * The XIVE IRQ number space is aligned with the XICS IRQ number
> >> + * space, CPU IPIs being allocated in the first 4K.
> >
> > We do align these in qemu, but I don't see that the kernel part
> > cares: as far as it's concerned only one of XICS or XIVE is active at
> > a time, and the irq numbers are chosen by userspace.
>
> There is some relation with userspace nevertheless. The KVM device does
> not remap the numbers to some other range today and the limits are fixed
> values. Checks are being done in the has_attr() and the set_attr().
Hrm. I still think the comment needs to describe what the constraint
is from the point of view of the kernel, which this isn't. Maybe, "we
allow irqs in the range X..Y (allowing userspace to do ...)"
>
> >> + */
> >> +#define KVMPPC_XIVE_FIRST_IRQ 0
> >> +#define KVMPPC_XIVE_NR_IRQS KVMPPC_XICS_NR_IRQS
> >> +
> >> /*
> >> * State for one guest irq source.
> >> *
> >> @@ -253,6 +260,9 @@ extern int (*__xive_vm_h_eoi)(struct kvm_vcpu *vcpu, unsigned long xirr);
> >> */
> >> void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu);
> >> int kvmppc_xive_debug_show_queues(struct seq_file *m, struct kvm_vcpu *vcpu);
> >> +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);
> >>
> >> #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 d1cc18a5b1c4..6f950ecb3592 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.c
> >> +++ b/arch/powerpc/kvm/book3s_xive.c
> >
> > I wonder if we should rename this book3s_xics_on_xive.c or something
> > at some point, I keep getting confused because I forget that this is
> > only dealing with host xive, not guest xive.
>
> I am fine with renaming. Any objections ? book3s_xics_p9.c ?
Hm, maybe?
> >> @@ -1485,8 +1485,8 @@ static int xive_get_source(struct kvmppc_xive *xive, long irq, u64 addr)
> >> return 0;
> >> }
> >>
> >> -static struct kvmppc_xive_src_block *xive_create_src_block(struct kvmppc_xive *xive,
> >> - int irq)
> >> +struct kvmppc_xive_src_block *kvmppc_xive_create_src_block(
> >> + struct kvmppc_xive *xive, int irq)
> >> {
> >> struct kvm *kvm = xive->kvm;
> >> struct kvmppc_xive_src_block *sb;
> >
> > It's odd that this function, now used from the xive-on-xive path as
> > well as the xics-on-xive path references KVMPPC_XICS_ICS_SHIFT a few
> > lines down from this change.
>
> Yes. This is because of the definition of the struct kvmppc_xive_src_block.
>
> We could introduce new defines for XIVE or a common set of defines for
> XICS and XIVE.
I think making common definitions would be best, if possible.
>
> >> @@ -1565,7 +1565,7 @@ static int xive_set_source(struct kvmppc_xive *xive, long irq, u64 addr)
> >> sb = kvmppc_xive_find_source(xive, irq, &idx);
> >> if (!sb) {
> >> pr_devel("No source, creating source block...\n");
> >> - sb = xive_create_src_block(xive, irq);
> >> + sb = kvmppc_xive_create_src_block(xive, irq);
> >> if (!sb) {
> >> pr_devel("Failed to create block...\n");
> >> return -ENOMEM;
> >> @@ -1789,7 +1789,7 @@ static void kvmppc_xive_cleanup_irq(u32 hw_num, struct xive_irq_data *xd)
> >> xive_cleanup_irq_data(xd);
> >> }
> >>
> >> -static void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb)
> >> +void kvmppc_xive_free_sources(struct kvmppc_xive_src_block *sb)
> >> {
> >> int i;
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> index 1f3da47a4a6a..a9b2d2d9af99 100644
> >> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -31,6 +31,29 @@
> >>
> >> #include "book3s_xive.h"
> >>
> >> +/*
> >> + * TODO: introduce a common template file with the XIVE native layer
> >> + * and the XICS-on-XIVE glue for the utility functions
> >> + */
> >> +#define __x_eoi_page(xd) ((void __iomem *)((xd)->eoi_mmio))
> >> +#define __x_trig_page(xd) ((void __iomem *)((xd)->trig_mmio))
> >> +#define __x_readq __raw_readq
> >> +#define __x_writeq __raw_writeq
> >> +
> >> +static u8 xive_vm_esb_load(struct xive_irq_data *xd, u32 offset)
> >> +{
> >> + u64 val;
> >> +
> >> + if (xd->flags & XIVE_IRQ_FLAG_SHIFT_BUG)
> >> + offset |= offset << 4;
> >> +
> >> + val = __x_readq(__x_eoi_page(xd) + offset);
> >> +#ifdef __LITTLE_ENDIAN__
> >> + val >>= 64-8;
> >> +#endif
> >> + return (u8)val;
> >> +}
> >> +
> >> static void kvmppc_xive_native_cleanup_queue(struct kvm_vcpu *vcpu, int prio)
> >> {
> >> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> @@ -153,12 +176,89 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
> >> return rc;
> >> }
> >>
> >> +static int kvmppc_xive_native_set_source(struct kvmppc_xive *xive, long irq,
> >> + u64 addr)
> >> +{
> >> + struct kvmppc_xive_src_block *sb;
> >> + struct kvmppc_xive_irq_state *state;
> >> + u64 __user *ubufp = (u64 __user *) addr;
> >> + u64 val;
> >> + u16 idx;
> >> +
> >> + pr_devel("%s irq=0x%lx\n", __func__, irq);
> >> +
> >> + if (irq < KVMPPC_XIVE_FIRST_IRQ || irq >= KVMPPC_XIVE_NR_IRQS)
> >> + return -E2BIG;
> >> +
> >> + sb = kvmppc_xive_find_source(xive, irq, &idx);
> >> + if (!sb) {
> >> + pr_debug("No source, creating source block...\n");
> >> + sb = kvmppc_xive_create_src_block(xive, irq);
> >> + if (!sb) {
> >> + pr_err("Failed to create block...\n");
> >> + return -ENOMEM;
> >> + }
> >> + }
> >> + state = &sb->irq_state[idx];
> >> +
> >> + if (get_user(val, ubufp)) {
> >> + pr_err("fault getting user info !\n");
> >> + return -EFAULT;
> >> + }
> >
> > You should validate the value loaded here to check it doesn't have any
> > bits set we don't know about.
>
> ok
>
> >
> >> +
> >> + /*
> >> + * If the source doesn't already have an IPI, allocate
> >> + * one and get the corresponding data
> >> + */
> >> + if (!state->ipi_number) {
> >> + state->ipi_number = xive_native_alloc_irq();
> >> + if (state->ipi_number == 0) {
> >> + pr_err("Failed to allocate IRQ !\n");
> >> + return -ENXIO;
> >> + }
> >> + xive_native_populate_irq_data(state->ipi_number,
> >> + &state->ipi_data);
> >> + pr_debug("%s allocated hw_irq=0x%x for irq=0x%lx\n", __func__,
> >> + state->ipi_number, irq);
> >> + }
> >> +
> >> + arch_spin_lock(&sb->lock);
> >
> > Why the direct call to arch_spin_lock() rather than just spin_lock()?
>
> Paul answered this question but may be I should make the effort to
> decouple both devices on this aspect.
>
> Thanks,
>
> C.
>
> >> +
> >> + /* Restore LSI state */
> >> + if (val & KVM_XIVE_LEVEL_SENSITIVE) {
> >> + state->lsi = true;
> >> + if (val & KVM_XIVE_LEVEL_ASSERTED)
> >> + state->asserted = true;
> >> + pr_devel(" LSI ! Asserted=%d\n", state->asserted);
> >> + }
> >> +
> >> + /* Mask IRQ to start with */
> >> + state->act_server = 0;
> >> + state->act_priority = MASKED;
> >> + xive_vm_esb_load(&state->ipi_data, XIVE_ESB_SET_PQ_01);
> >> + xive_native_configure_irq(state->ipi_number, 0, MASKED, 0);
> >> +
> >> + /* Increment the number of valid sources and mark this one valid */
> >> + if (!state->valid)
> >> + xive->src_count++;
> >> + state->valid = true;
> >> +
> >> + arch_spin_unlock(&sb->lock);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int kvmppc_xive_native_set_attr(struct kvm_device *dev,
> >> struct kvm_device_attr *attr)
> >> {
> >> + struct kvmppc_xive *xive = dev->private;
> >> +
> >> switch (attr->group) {
> >> case KVM_DEV_XIVE_GRP_CTRL:
> >> break;
> >> + case KVM_DEV_XIVE_GRP_SOURCE:
> >> + return kvmppc_xive_native_set_source(xive, attr->attr,
> >> + attr->addr);
> >> }
> >> return -ENXIO;
> >> }
> >> @@ -175,6 +275,11 @@ static int kvmppc_xive_native_has_attr(struct kvm_device *dev,
> >> switch (attr->group) {
> >> case KVM_DEV_XIVE_GRP_CTRL:
> >> break;
> >> + case KVM_DEV_XIVE_GRP_SOURCE:
> >> + if (attr->attr >= KVMPPC_XIVE_FIRST_IRQ &&
> >> + attr->attr < KVMPPC_XIVE_NR_IRQS)
> >> + return 0;
> >> + break;
> >> }
> >> return -ENXIO;
> >> }
> >> @@ -183,6 +288,7 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
> >> {
> >> struct kvmppc_xive *xive = dev->private;
> >> struct kvm *kvm = xive->kvm;
> >> + int i;
> >>
> >> debugfs_remove(xive->dentry);
> >>
> >> @@ -191,6 +297,14 @@ static void kvmppc_xive_native_free(struct kvm_device *dev)
> >> if (kvm)
> >> kvm->arch.xive = NULL;
> >>
> >> + /* Mask and free interrupts */
> >> + for (i = 0; i <= xive->max_sbid; i++) {
> >> + if (xive->src_blocks[i])
> >> + kvmppc_xive_free_sources(xive->src_blocks[i]);
> >> + kfree(xive->src_blocks[i]);
> >> + xive->src_blocks[i] = NULL;
> >> + }
> >> +
> >> if (xive->vp_base != XIVE_INVALID_VP)
> >> xive_native_free_vp_block(xive->vp_base);
> >>
> >> diff --git a/Documentation/virtual/kvm/devices/xive.txt b/Documentation/virtual/kvm/devices/xive.txt
> >> index fdbd2ff92a88..cd8bfc37b72e 100644
> >> --- a/Documentation/virtual/kvm/devices/xive.txt
> >> +++ b/Documentation/virtual/kvm/devices/xive.txt
> >> @@ -17,3 +17,18 @@ the legacy interrupt mode, referred as XICS (POWER7/8).
> >>
> >> 1. KVM_DEV_XIVE_GRP_CTRL
> >> Provides global controls on the device
> >> +
> >> + 2. KVM_DEV_XIVE_GRP_SOURCE (write only)
> >> + Initializes a new source in the XIVE device and mask it.
> >> + Attributes:
> >> + Interrupt source number (64-bit)
> >> + The kvm_device_attr.addr points to a __u64 value:
> >> + bits: | 63 .... 2 | 1 | 0
> >> + values: | unused | level | type
> >> + - type: 0:MSI 1:LSI
> >> + - level: assertion level in case of an LSI.
> >> + Errors:
> >> + -E2BIG: Interrupt source number is out of range
> >> + -ENOMEM: Could not create a new source block
> >> + -EFAULT: Invalid user pointer for attr->addr.
> >> + -ENXIO: Could not allocate underlying HW interrupt
> >
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 16/16] KVM: PPC: Book3S HV: XIVE: clear the vCPU interrupt presenters
From: David Gibson @ 2019-03-14 2:26 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <3604baf2-0a39-6990-968f-1222d32f9278@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]
On Wed, Mar 13, 2019 at 09:17:17AM +0100, Cédric Le Goater wrote:
> On 2/25/19 5:18 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:40PM +0100, Cédric Le Goater wrote:
> >> When the VM boots, the CAS negotiation process determines which
> >> interrupt mode to use and invokes a machine reset. At that time, the
> >> previous KVM interrupt device is 'destroyed' before the chosen one is
> >> created. Upon destruction, the vCPU interrupt presenters using the KVM
> >> device should be cleared first, the machine will reconnect them later
> >> to the new device after it is created.
> >>
> >> When using the KVM device, there is still a race window with the early
> >> checks in kvmppc_native_connect_vcpu(). Yet to be fixed.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> arch/powerpc/kvm/book3s_xics.c | 19 +++++++++++++
> >> arch/powerpc/kvm/book3s_xive.c | 39 +++++++++++++++++++++++++--
> >> arch/powerpc/kvm/book3s_xive_native.c | 16 +++++++++++
> >> 3 files changed, 72 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c
> >> index f27ee57ab46e..81cdabf4295f 100644
> >> --- a/arch/powerpc/kvm/book3s_xics.c
> >> +++ b/arch/powerpc/kvm/book3s_xics.c
> >> @@ -1342,6 +1342,25 @@ static void kvmppc_xics_free(struct kvm_device *dev)
> >> struct kvmppc_xics *xics = dev->private;
> >> int i;
> >> struct kvm *kvm = xics->kvm;
> >> + struct kvm_vcpu *vcpu;
> >> +
> >> + /*
> >> + * When destroying the VM, the vCPUs are destroyed first and
> >> + * the vCPU list should be empty. If this is not the case,
> >> + * then we are simply destroying the device and we should
> >> + * clean up the vCPU interrupt presenters first.
> >> + */
> >> + if (atomic_read(&kvm->online_vcpus) != 0) {
> >> + /*
> >> + * call kick_all_cpus_sync() to ensure that all CPUs
> >> + * have executed any pending interrupts
> >> + */
> >> + if (is_kvmppc_hv_enabled(kvm))
> >> + kick_all_cpus_sync();
> >> +
> >> + kvm_for_each_vcpu(i, vcpu, kvm)
> >> + kvmppc_xics_free_icp(vcpu);
> >> + }
> >>
> >> debugfs_remove(xics->dentry);
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> >> index 7a14512b8944..0a1c11d6881c 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.c
> >> +++ b/arch/powerpc/kvm/book3s_xive.c
> >> @@ -1105,11 +1105,19 @@ void kvmppc_xive_disable_vcpu_interrupts(struct kvm_vcpu *vcpu)
> >> void kvmppc_xive_cleanup_vcpu(struct kvm_vcpu *vcpu)
> >> {
> >> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
> >> - struct kvmppc_xive *xive = xc->xive;
> >> + struct kvmppc_xive *xive;
> >> int i;
> >>
> >> + if (!kvmppc_xics_enabled(vcpu))
> >
> > This should be kvmppc_xive_enabled(), no?
>
> This is the KVM XICS-on-XIVE device and its IRQ type is KVMPPC_IRQ_XICS.
> So this is correct :/
Ah, right, sorry.
> May be we should introduce a KVMPPC_IRQ_XICS_ON_XIVE macro to
> clarify.
Yeah, maybe.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 06/16] KVM: PPC: Book3S HV: XIVE: add controls for the EQ configuration
From: David Gibson @ 2019-03-14 2:32 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, linuxppc-dev
In-Reply-To: <33aeb002-39b2-1015-9923-613781c08fd6@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 3679 bytes --]
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_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
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/16] KVM: PPC: Book3S HV: XIVE: add a control to dirty the XIVE EQ pages
From: David Gibson @ 2019-03-14 2:33 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: kvm, kvm-ppc, Paul Mackerras, linuxppc-dev
In-Reply-To: <e952c257-dbc9-fd4c-fd10-20533c4b2c42@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]
On Wed, Mar 13, 2019 at 12:48:57PM +0100, Cédric Le Goater wrote:
> On 2/25/19 3:53 AM, David Gibson wrote:
> > On Fri, Feb 22, 2019 at 12:28:33PM +0100, Cédric Le Goater wrote:
> >> When migration of a VM is initiated, a first copy of the RAM is
> >> transferred to the destination before the VM is stopped, but there is
> >> no guarantee that the EQ pages in which the event notification are
> >> queued have not been modified.
> >>
> >> To make sure migration will capture a consistent memory state, the
> >> XIVE device should perform a XIVE quiesce sequence to stop the flow of
> >> event notifications and stabilize the EQs. This is the purpose of the
> >> KVM_DEV_XIVE_EQ_SYNC control which will also marks the EQ pages dirty
> >> to force their transfer.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >> arch/powerpc/include/uapi/asm/kvm.h | 1 +
> >> arch/powerpc/kvm/book3s_xive_native.c | 67 ++++++++++++++++++++++
> >> Documentation/virtual/kvm/devices/xive.txt | 29 ++++++++++
> >> 3 files changed, 97 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/uapi/asm/kvm.h b/arch/powerpc/include/uapi/asm/kvm.h
> >> index 289c504b7c1d..cd78ad1020fe 100644
> >> --- a/arch/powerpc/include/uapi/asm/kvm.h
> >> +++ b/arch/powerpc/include/uapi/asm/kvm.h
> >> @@ -678,6 +678,7 @@ struct kvm_ppc_cpu_char {
> >> /* POWER9 XIVE Native Interrupt Controller */
> >> #define KVM_DEV_XIVE_GRP_CTRL 1
> >> #define KVM_DEV_XIVE_RESET 1
> >> +#define KVM_DEV_XIVE_EQ_SYNC 2
> >> #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 */
> >> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> >> index dd2a9d411fe7..3debc876d5a0 100644
> >> --- a/arch/powerpc/kvm/book3s_xive_native.c
> >> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> >> @@ -640,6 +640,70 @@ static int kvmppc_xive_reset(struct kvmppc_xive *xive)
> >> return 0;
> >> }
> >>
> >> +static void kvmppc_xive_native_sync_sources(struct kvmppc_xive_src_block *sb)
> >> +{
> >> + int j;
> >> +
> >> + for (j = 0; j < KVMPPC_XICS_IRQ_PER_ICS; j++) {
> >> + struct kvmppc_xive_irq_state *state = &sb->irq_state[j];
> >> + struct xive_irq_data *xd;
> >> + u32 hw_num;
> >> +
> >> + if (!state->valid)
> >> + continue;
> >> + if (state->act_priority == MASKED)
> >
> > Is this correct? If you masked an irq, then immediately did a sync,
> > couldn't there still be some of the irqs in flight? I thought the
> > reason we needed a sync was that masking and other such operations
> > _didn't_ implicitly synchronize.
>
> The struct kvmppc_xive_irq_state reflects the state of the EAS
> configuration and not the state of the source. The source is masked
> setting the PQ bits to '-Q', which is what is being done before calling
> the KVM_DEV_XIVE_EQ_SYNC control.
>
> If a source EAS is configured, OPAL syncs the XIVE IC of the source and
> the XIVE IC of the previous target if any.
>
> So I think we are fine.
Ok.
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 4/4] hugetlb: allow to free gigantic pages regardless of the configuration
From: Michael Ellerman @ 2019-03-14 2:53 UTC (permalink / raw)
To: Alexandre Ghiti, Andrew Morton, Vlastimil Babka, Catalin Marinas,
Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
Martin Schwidefsky, Heiko Carstens, Yoshinori Sato, Rich Felker,
David S . Miller, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H . Peter Anvin, x86, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Mike Kravetz, linux-arm-kernel, linux-kernel,
linuxppc-dev, linux-s390, linux-sh, sparclinux, linux-mm,
Aneesh Kumar K.V
Cc: Alexandre Ghiti
In-Reply-To: <20190307132015.26970-5-alex@ghiti.fr>
[ Cc += Aneesh ]
Alexandre Ghiti <alex@ghiti.fr> writes:
> diff --git a/arch/powerpc/include/asm/book3s/64/hugetlb.h b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> index 5b0177733994..d04a0bcc2f1c 100644
> --- a/arch/powerpc/include/asm/book3s/64/hugetlb.h
> +++ b/arch/powerpc/include/asm/book3s/64/hugetlb.h
> @@ -32,13 +32,6 @@ static inline int hstate_get_psize(struct hstate *hstate)
> }
> }
>
> -#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
> -static inline bool gigantic_page_supported(void)
> -{
> - return true;
> -}
> -#endif
This is going to clash with:
https://patchwork.ozlabs.org/patch/1047003/
Which does:
@@ -35,6 +35,13 @@ static inline int hstate_get_psize(struct hstate *hstate)
#ifdef CONFIG_ARCH_HAS_GIGANTIC_PAGE
static inline bool gigantic_page_supported(void)
{
+ /*
+ * We used gigantic page reservation with hypervisor assist in some case.
+ * We cannot use runtime allocation of gigantic pages in those platforms
+ * This is hash translation mode LPARs.
+ */
+ if (firmware_has_feature(FW_FEATURE_LPAR) && !radix_enabled())
+ return false;
return true;
}
#endif
Not sure how to resolve it.
cheers
^ permalink raw reply
* [PATCH v2] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Alastair D'Silva @ 2019-03-14 2:31 UTC (permalink / raw)
To: alastair
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Naveen N. Rao, linuxppc-dev,
Andrew Morton
In-Reply-To: <20190313034208.13134-1-alastair@au1.ibm.com>
From: Alastair D'Silva <alastair@d-silva.org>
When building an LTO kernel, the existing code generates warnings:
./arch/powerpc/include/asm/paca.h:37:30: warning: register of
‘local_paca’ used for multiple global register variables
register struct paca_struct *local_paca asm("r13");
^
./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
‘local_paca’
This patch reworks local_paca into an inline getter & setter function,
which addresses the warning.
Changelog:
V2
- Address whitespace issues
- keep new implementation close to where the old implementation was
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
arch/powerpc/include/asm/paca.h | 37 +++++++++++++++++++++++++--------
arch/powerpc/kernel/paca.c | 2 +-
2 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index e843bc5d1a0f..2fa0b43357c9 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -34,19 +34,38 @@
#include <asm/cpuidle.h>
#include <asm/atomic.h>
-register struct paca_struct *local_paca asm("r13");
-
#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
-/*
- * Add standard checks that preemption cannot occur when using get_paca():
- * otherwise the paca_struct it points to may be the wrong one just after.
- */
-#define get_paca() ((void) debug_smp_processor_id(), local_paca)
-#else
-#define get_paca() local_paca
#endif
+static inline struct paca_struct *get_paca_no_preempt_check(void)
+{
+ register struct paca_struct *paca asm("r13");
+
+ return paca;
+}
+
+static inline struct paca_struct *get_paca(void)
+{
+#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
+ /*
+ * Add standard checks that preemption cannot occur when using get_paca():
+ * otherwise the paca_struct it points to may be the wrong one just after.
+ */
+ debug_smp_processor_id();
+#endif
+ return get_paca_no_preempt_check();
+}
+
+#define local_paca get_paca_no_preempt_check()
+
+static inline void set_paca(struct paca_struct *new)
+{
+ register struct paca_struct *paca asm("r13");
+
+ paca = new;
+}
+
#ifdef CONFIG_PPC_PSERIES
#define get_lppaca() (get_paca()->lppaca_ptr)
#endif
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 913bfca09c4f..ae5c243f9d5a 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct *new_paca, int cpu)
void setup_paca(struct paca_struct *new_paca)
{
/* Setup r13 */
- local_paca = new_paca;
+ set_paca(new_paca);
#ifdef CONFIG_PPC_BOOK3E
/* On Book3E, initialize the TLB miss exception frames */
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] powerpc: use $(origin ARCH) to select KBUILD_DEFCONFIG
From: Michael Ellerman @ 2019-03-14 2:24 UTC (permalink / raw)
To: Mathieu Malaterre; +Cc: Masahiro Yamada, linuxppc-dev, LKML, Paul Mackerras
In-Reply-To: <CA+7wUsxruu0GdyRGJ3vsjRvSBAptMn8_f5BmeMo=-E3zbhGGxQ@mail.gmail.com>
Mathieu Malaterre <malat@debian.org> writes:
> On Sat, Feb 16, 2019 at 3:26 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>>
>> On Sat, Feb 16, 2019 at 1:11 AM Mathieu Malaterre <malat@debian.org> wrote:
>> >
>> > On Fri, Feb 15, 2019 at 10:41 AM Masahiro Yamada
>> > <yamada.masahiro@socionext.com> wrote:
>> > >
>> > > I often test all Kconfig commands for all architectures. To ease my
>> > > workflow, I want 'make defconfig' at least working without any cross
>> > > compiler.
>> > >
>> > > Currently, arch/powerpc/Makefile checks CROSS_COMPILE to decide the
>> > > default defconfig source.
>> > >
>> > > If CROSS_COMPILE is unset, it is likely to be the native build, so
>> > > 'uname -m' is useful to choose the defconfig. If CROSS_COMPILE is set,
>> > > the user is cross-building (i.e. 'uname -m' is probably x86_64), so
>> > > it falls back to ppc64_defconfig. Yup, make sense.
>> > >
>> > > However, I want to run 'make ARCH=* defconfig' without setting
>> > > CROSS_COMPILE for each architecture.
>> > >
>> > > My suggestion is to check $(origin ARCH).
>> > >
>> > > When you cross-compile the kernel, you need to set ARCH from your
>> > > environment or from the command line.
>> > >
>> > > For the native build, you do not need to set ARCH. The default in
>> > > the top Makefile is used:
>> > >
>> > > ARCH ?= $(SUBARCH)
>> > >
>> > > Hence, $(origin ARCH) returns 'file'.
>> > >
>> > > Before this commit, 'make ARCH=powerpc defconfig' failed:
>> >
>> > In case you have not seen it, please check:
>> >
>> > http://patchwork.ozlabs.org/patch/1037835/
>>
>> I did not know that because I do not subscribe to ppc ML.
>>
>> Michael's patch looks good to me.
>
> OK
>
>>
>> If you mimic x86, the following will work:
>>
>
> Nice! Michael do you have a preference ?
Yeah I don't like playing games with ARCH. Doing so means auto builders
and other build scripts need to learn about the special rules for ARCH,
which is a pain.
So I'll merge my patch, which I think will also work for Masahiro's
case.
cheers
^ permalink raw reply
* Re: [PATCH 5/5] ocxl: Remove some unused exported symbols
From: Alastair D'Silva @ 2019-03-14 2:23 UTC (permalink / raw)
To: Greg Kurz
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
Frederic Barrat, linuxppc-dev
In-Reply-To: <20190313101045.6712f156@bahia.lan>
On Wed, 2019-03-13 at 10:10 +0100, Greg Kurz wrote:
> On Wed, 13 Mar 2019 15:07:01 +1100
> "Alastair D'Silva" <alastair@au1.ibm.com> wrote:
>
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > Remove some unused exported symbols.
> >
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> > ---
> > drivers/misc/ocxl/config.c | 2 --
> > drivers/misc/ocxl/ocxl_internal.h | 23 +++++++++++++++++++++++
> > include/misc/ocxl.h | 23 -----------------------
> > 3 files changed, 23 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/misc/ocxl/config.c
> > b/drivers/misc/ocxl/config.c
> > index 026ac2ac4f9c..c90c2e4875bf 100644
> > --- a/drivers/misc/ocxl/config.c
> > +++ b/drivers/misc/ocxl/config.c
> > @@ -299,7 +299,6 @@ int ocxl_config_check_afu_index(struct pci_dev
> > *dev,
> > }
> > return 1;
> > }
> > -EXPORT_SYMBOL_GPL(ocxl_config_check_afu_index);
> >
> > static int read_afu_name(struct pci_dev *dev, struct
> > ocxl_fn_config *fn,
> > struct ocxl_afu_config *afu)
> > @@ -535,7 +534,6 @@ int ocxl_config_get_pasid_info(struct pci_dev
> > *dev, int *count)
> > {
> > return pnv_ocxl_get_pasid_count(dev, count);
> > }
> > -EXPORT_SYMBOL_GPL(ocxl_config_get_pasid_info);
> >
> > void ocxl_config_set_afu_pasid(struct pci_dev *dev, int pos, int
> > pasid_base,
> > u32 pasid_count_log)
> > diff --git a/drivers/misc/ocxl/ocxl_internal.h
> > b/drivers/misc/ocxl/ocxl_internal.h
> > index 321b29e77f45..06fd98c989c8 100644
> > --- a/drivers/misc/ocxl/ocxl_internal.h
> > +++ b/drivers/misc/ocxl/ocxl_internal.h
> > @@ -107,6 +107,29 @@ void ocxl_pasid_afu_free(struct ocxl_fn *fn,
> > u32 start, u32 size);
> > int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> > void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> >
> > +/*
> > + * Get the max PASID value that can be used by the function
> > + */
> > +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> > +
> > +/*
> > + * Check if an AFU index is valid for the given function.
> > + *
> > + * AFU indexes can be sparse, so a driver should check all indexes
> > up
> > + * to the maximum found in the function description
> > + */
> > +int ocxl_config_check_afu_index(struct pci_dev *dev,
> > + struct ocxl_fn_config *fn, int
> > afu_idx);
> > +
> > +/**
>
> Two *s ?
>
These are Sphinx formatted comments (similar, but not quite the same as
Doxygen).
> Also, this results in an ocxl_internal.h header file where only these
> three functions are documented... which looks a bit weird IMHO. Since
> these are ocxl internals, do we _really_ need to keep the comments ?
I believe we should, it's a courtesy to the next person who has to work
in the area.
There are more documentation comments coming in further series.
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [PATCH 1/1] arch/powerpc: Rework local_paca to avoid LTO warnings
From: Alastair D'Silva @ 2019-03-14 1:39 UTC (permalink / raw)
To: Christophe Leroy
Cc: Michal Hocko, Mahesh Salgaonkar, linux-kernel, Nicholas Piggin,
Mike Rapoport, Paul Mackerras, Naveen N. Rao, linuxppc-dev,
Andrew Morton
In-Reply-To: <bd858b12-7fab-057d-0065-2ccb98cbf648@c-s.fr>
On Wed, 2019-03-13 at 10:06 +0100, Christophe Leroy wrote:
> Hello,
Thanks for reviewing :)
>
> Le 13/03/2019 à 04:42, Alastair D'Silva a écrit :
> > From: Alastair D'Silva <alastair@d-silva.org>
> >
> > When building an LTO kernel, the existing code generates warnings:
> > ./arch/powerpc/include/asm/paca.h:37:30: warning: register of
> > ‘local_paca’ used for multiple global register variables
> > register struct paca_struct *local_paca asm("r13");
> > ^
> > ./arch/powerpc/include/asm/paca.h:37:30: note: conflicts with
> > ‘local_paca’
>
> How do you build a LTO kernel ?
I'm using Andi Kleen's LTO tree:
https://github.com/andikleen/linux-misc/tree/lto-420-1
with a few other patches:
https://github.com/andikleen/linux-misc/pull/27
You'll need to add the following to your .config:
CONFIG_LTO_MENU=y
CONFIG_LTO=y
>
> > This patch reworks local_paca into an inline getter & setter
> > function,
> > which addresses the warning.
>
> This patch adds sparse warnings, see
> https://patchwork.ozlabs.org/patch/1055875/
These warnings are bogus, they replace warnings that flagged against
spinlock.h.
> > Generated ASM from this patch is broadly similar (addresses have
> > changed and the compiler uses different GPRs in some places).
>
> Your text might be confusion. When I read it the first time I
> thought
> you were saying that the compiler was now using another GPR than r13.
>
I'll see if I can improve it.
> > Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
>
> I guess the same has to be done for current, see
> arch/powerpc/include/asm/current.h :
>
> /*
> * We keep `current' in r2 for speed.
> */
> register struct task_struct *current asm ("r2");
Hmm, I didn't see problems on PPC64 as that already uses an inline
function. I'll address this in another patch for the PPC32 case.
> > ---
> > arch/powerpc/include/asm/paca.h | 44 +++++++++++++++++++++++-----
> > -----
> > arch/powerpc/kernel/paca.c | 2 +-
> > 2 files changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/paca.h
> > b/arch/powerpc/include/asm/paca.h
> > index e843bc5d1a0f..9c9e2dea0f9b 100644
> > --- a/arch/powerpc/include/asm/paca.h
> > +++ b/arch/powerpc/include/asm/paca.h
> > @@ -34,19 +34,6 @@
> > #include <asm/cpuidle.h>
> > #include <asm/atomic.h>
> >
> > -register struct paca_struct *local_paca asm("r13");
> > -
> > -#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > -extern unsigned int debug_smp_processor_id(void); /* from
> > linux/smp.h */
> > -/*
> > - * Add standard checks that preemption cannot occur when using
> > get_paca():
> > - * otherwise the paca_struct it points to may be the wrong one
> > just after.
> > - */
> > -#define get_paca() ((void) debug_smp_processor_id(), local_paca)
> > -#else
> > -#define get_paca() local_paca
> > -#endif
> > -
> > #ifdef CONFIG_PPC_PSERIES
> > #define get_lppaca() (get_paca()->lppaca_ptr)
> > #endif
> > @@ -266,6 +253,37 @@ struct paca_struct {
> > #endif
> > } ____cacheline_aligned;
> >
> > +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > +extern unsigned int debug_smp_processor_id(void); /* from
> > linux/smp.h */
> > +#endif
>
> Why moving this down, why not leaving at the same place as before ?
>
> If you really need to move it, you should remove the 'extern' at the
> same time to make checkpatch happy.
I moved it to keep it close to the usage of it.
I suppose the new implementation should be in the same place though.
> > +
> > +static inline struct paca_struct *get_paca_no_preempt_check(void)
> > +{
> > + register struct paca_struct *paca asm("r13");
>
> Should be a blank line there.
Whoops, I thought I ran checkpatch, but clearly, I forgot. I'll
resubmit.
> > + return paca;
> > +}
> > +
> > +static inline struct paca_struct *get_paca(void)
> > +{
> > +#if defined(CONFIG_DEBUG_PREEMPT) && defined(CONFIG_SMP)
> > + /*
> > + * Add standard checks that preemption cannot occur when using
> > get_paca():
> > + * otherwise the paca_struct it points to may be the wrong one
> > just after.
> > + */
> > + debug_smp_processor_id();
> > +#endif
> > + return get_paca_no_preempt_check();
> > +}
> > +
> > +#define local_paca get_paca_no_preempt_check()
> > +
> > +static inline void set_paca(struct paca_struct *new)
> > +{
> > + register struct paca_struct *paca asm("r13");
>
> Blank line should be added here.
>
> > + paca = new;
> > +}
> > +
> > +
> > extern void copy_mm_to_paca(struct mm_struct *mm);
> > extern struct paca_struct **paca_ptrs;
> > extern void initialise_paca(struct paca_struct *new_paca, int
> > cpu);
> > diff --git a/arch/powerpc/kernel/paca.c
> > b/arch/powerpc/kernel/paca.c
> > index 913bfca09c4f..ae5c243f9d5a 100644
> > --- a/arch/powerpc/kernel/paca.c
> > +++ b/arch/powerpc/kernel/paca.c
> > @@ -172,7 +172,7 @@ void __init initialise_paca(struct paca_struct
> > *new_paca, int cpu)
> > void setup_paca(struct paca_struct *new_paca)
> > {
> > /* Setup r13 */
> > - local_paca = new_paca;
> > + set_paca(new_paca);
> >
> > #ifdef CONFIG_PPC_BOOK3E
> > /* On Book3E, initialize the TLB miss exception frames */
> >
>
> Christophe
>
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox