From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cornelia Huck Subject: Re: [PATCH v3 2/8] s390/cio: introduce DMA pools to cio Date: Mon, 3 Jun 2019 13:37:45 +0200 Message-ID: <20190603133745.240c00a7.cohuck@redhat.com> References: <20190529122657.166148-1-mimu@linux.ibm.com> <20190529122657.166148-3-mimu@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20190529122657.166148-3-mimu@linux.ibm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: Michael Mueller Cc: Vasily Gorbik , Linux-S390 Mailing List , Thomas Huth , Claudio Imbrenda , KVM Mailing List , Sebastian Ott , "Michael S . Tsirkin" , Pierre Morel , Farhan Ali , Heiko Carstens , Eric Farman , virtualization@lists.linux-foundation.org, Halil Pasic , Christian Borntraeger , Christoph Hellwig , Viktor Mihajlovski , Janosch Frank List-Id: virtualization@lists.linuxfoundation.org On Wed, 29 May 2019 14:26:51 +0200 Michael Mueller wrote: > From: Halil Pasic > > 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 > Reviewed-by: Sebastian Ott > Signed-off-by: Michael Mueller > --- > 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? > +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 > #include > #include > +#include > +#include > #include > #include > > @@ -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... > + 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. > + 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? 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];