Re: [PATCH v3 2/8] s390/cio: introduce DMA pools to cio

Michael Mueller
2019-06-03T12:09:13Z

On 03.06.19 13:37, Cornelia Huck wrote:
> On Wed, 29 May 2019 14:26:51 +0200
> Michael Mueller <mimu@linux.ibm.com> wrote:
>
>> From: Halil Pasic <pasic@linux.ibm.com>
>>
>> To support protected virtualization cio will need to make sure the
>> memory used for communication with the hypervisor is DMA memory.
>>
>> Let us introduce one global pool for cio.
>>
>> Our DMA pools are implemented as a gen_pool backed with DMA pages. The
>> idea is to avoid each allocation effectively wasting a page, as we
>> typically allocate much less than PAGE_SIZE.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> Reviewed-by: Sebastian Ott <sebott@linux.ibm.com>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>> arch/s390/Kconfig | 1 +
>> arch/s390/include/asm/cio.h | 11 ++++
>> drivers/s390/cio/css.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 128 insertions(+), 4 deletions(-)
>
> (...)
>
>> diff --git a/arch/s390/include/asm/cio.h b/arch/s390/include/asm/cio.h
>> index 1727180e8ca1..43c007d2775a 100644
>> --- a/arch/s390/include/asm/cio.h
>> +++ b/arch/s390/include/asm/cio.h
>> @@ -328,6 +328,17 @@ static inline u8 pathmask_to_pos(u8 mask)
>> void channel_subsystem_reinit(void);
>> extern void css_schedule_reprobe(void);
>>
>> +extern void *cio_dma_zalloc(size_t size);
>> +extern void cio_dma_free(void *cpu_addr, size_t size);
>> +extern struct device *cio_get_dma_css_dev(void);
>> +
>> +struct gen_pool;
>
> That forward declaration is a bit ugly... I guess the alternative was
> include hell?

That's an easy one.

#include <linux/bitops.h>
+#include <linux/genalloc.h>
#include <asm/types.h>

>
>> +void *cio_gp_dma_zalloc(struct gen_pool *gp_dma, struct device *dma_dev,
>> + size_t size);
>> +void cio_gp_dma_free(struct gen_pool *gp_dma, void *cpu_addr, size_t size);
>> +void cio_gp_dma_destroy(struct gen_pool *gp_dma, struct device *dma_dev);
>> +struct gen_pool *cio_gp_dma_create(struct device *dma_dev, int nr_pages);
>> +
>> /* Function from drivers/s390/cio/chsc.c */
>> int chsc_sstpc(void *page, unsigned int op, u16 ctrl, u64 *clock_delta);
>> int chsc_sstpi(void *page, void *result, size_t size);
>> diff --git a/drivers/s390/cio/css.c b/drivers/s390/cio/css.c
>> index aea502922646..b97618497848 100644
>> --- a/drivers/s390/cio/css.c
>> +++ b/drivers/s390/cio/css.c
>> @@ -20,6 +20,8 @@
>> #include <linux/reboot.h>
>> #include <linux/suspend.h>
>> #include <linux/proc_fs.h>
>> +#include <linux/genalloc.h>
>> +#include <linux/dma-mapping.h>
>> #include <asm/isc.h>
>> #include <asm/crw.h>
>>
>> @@ -224,6 +226,8 @@ struct subchannel *css_alloc_subchannel(struct subchannel_id schid,
>> INIT_WORK(&sch->todo_work, css_sch_todo);
>> sch->dev.release = &css_subchannel_release;
>> device_initialize(&sch->dev);
>
> It might be helpful to add a comment why you use 31 bit here...

@Halil, please let me know what comment you prefere here...

>
>> + sch->dev.coherent_dma_mask = DMA_BIT_MASK(31);
>> + sch->dev.dma_mask = &sch->dev.coherent_dma_mask;
>> return sch;
>>
>> err:
>> @@ -899,6 +903,8 @@ static int __init setup_css(int nr)
>> dev_set_name(&css->device, "css%x", nr);
>> css->device.groups = cssdev_attr_groups;
>> css->device.release = channel_subsystem_release;
>
> ...and 64 bit here.

and here.

>
>> + css->device.coherent_dma_mask = DMA_BIT_MASK(64);
>> + css->device.dma_mask = &css->device.coherent_dma_mask;
>>
>> mutex_init(&css->mutex);
>> css->cssid = chsc_get_cssid(nr);
>
> (...)
>
>> @@ -1059,16 +1168,19 @@ static int __init css_bus_init(void)
>> if (ret)
>> goto out_unregister;
>> ret = register_pm_notifier(&css_power_notifier);
>> - if (ret) {
>> - unregister_reboot_notifier(&css_reboot_notifier);
>> - goto out_unregister;
>> - }
>> + if (ret)
>> + goto out_unregister_rn;
>> + ret = cio_dma_pool_init();
>> + if (ret)
>> + goto out_unregister_rn;
>
> Don't you also need to unregister the pm notifier on failure here?

Mmh, that was the original intention. Thanks!

>
> Other than that, I noticed only cosmetic issues; seems reasonable to me.
>
>> css_init_done = 1;
>>
>> /* Enable default isc for I/O subchannels. */
>> isc_register(IO_SCH_ISC);
>>
>> return 0;
>> +out_unregister_rn:
>> + unregister_reboot_notifier(&css_reboot_notifier);
>> out_unregister:
>> while (i-- > 0) {
>> struct channel_subsystem *css = channel_subsystems[i];
>

Thanks,
Michael