Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted

2019-06-03 Thread Michael S. Tsirkin
On Mon, Jun 03, 2019 at 10:13:59PM -0300, Thiago Jung Bauermann wrote:
> 
> 
> Michael S. Tsirkin  writes:
> 
> > On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote:
> >> I rephrased it in terms of address translation. What do you think of
> >> this version? The flag name is slightly different too:
> >>
> >>
> >> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
> >> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
> >> with the exception that address translation is guaranteed to be
> >> unnecessary when accessing memory addresses supplied to the device
> >> by the driver. Which is to say, the device will always use physical
> >> addresses matching addresses used by the driver (typically meaning
> >> physical addresses used by the CPU) and not translated further. This
> >> flag should be set by the guest if offered, but to allow for
> >> backward-compatibility device implementations allow for it to be
> >> left unset by the guest. It is an error to set both this flag and
> >> VIRTIO_F_ACCESS_PLATFORM.
> >
> >
> > OK so VIRTIO_F_ACCESS_PLATFORM is designed to allow unpriveledged
> > drivers. This is why devices fail when it's not negotiated.
> 
> Just to clarify, what do you mean by unprivileged drivers? Is it drivers
> implemented in guest userspace such as with VFIO? Or unprivileged in
> some other sense such as needing to use bounce buffers for some reason?

I had drivers in guest userspace in mind.

> > This confuses me.
> > If driver is unpriveledged then what happens with this flag?
> > It can supply any address it wants. Will that corrupt kernel
> > memory?
> 
> Not needing address translation doesn't necessarily mean that there's no
> IOMMU. On powerpc we don't use VIRTIO_F_ACCESS_PLATFORM but there's
> always an IOMMU present. And we also support VFIO drivers. The VFIO API
> for pseries (sPAPR section in Documentation/vfio.txt) has extra ioctls
> to program the IOMMU.
> 
> For our use case, we don't need address translation because we set up an
> identity mapping in the IOMMU so that the device can use guest physical
> addresses.

And can it access any guest physical address?

> If the guest kernel is concerned that an unprivileged driver could
> jeopardize its integrity it should not negotiate this feature flag.

Unfortunately flag negotiation is done through config space
and so can be overwritten by the driver.

> Perhaps there should be a note about this in the flag definition? This
> concern is platform-dependant though. I don't believe it's an issue in
> pseries.

Again ACCESS_PLATFORM has a pretty open definition. It does actually
say it's all up to the platform.

Specifically how will VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION be
implemented portably? virtio has no portable way to know
whether DMA API bypasses translation.



> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted

2019-06-03 Thread Thiago Jung Bauermann



Michael S. Tsirkin  writes:

> On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote:
>> I rephrased it in terms of address translation. What do you think of
>> this version? The flag name is slightly different too:
>>
>>
>> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
>> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
>> with the exception that address translation is guaranteed to be
>> unnecessary when accessing memory addresses supplied to the device
>> by the driver. Which is to say, the device will always use physical
>> addresses matching addresses used by the driver (typically meaning
>> physical addresses used by the CPU) and not translated further. This
>> flag should be set by the guest if offered, but to allow for
>> backward-compatibility device implementations allow for it to be
>> left unset by the guest. It is an error to set both this flag and
>> VIRTIO_F_ACCESS_PLATFORM.
>
>
> OK so VIRTIO_F_ACCESS_PLATFORM is designed to allow unpriveledged
> drivers. This is why devices fail when it's not negotiated.

Just to clarify, what do you mean by unprivileged drivers? Is it drivers
implemented in guest userspace such as with VFIO? Or unprivileged in
some other sense such as needing to use bounce buffers for some reason?

> This confuses me.
> If driver is unpriveledged then what happens with this flag?
> It can supply any address it wants. Will that corrupt kernel
> memory?

Not needing address translation doesn't necessarily mean that there's no
IOMMU. On powerpc we don't use VIRTIO_F_ACCESS_PLATFORM but there's
always an IOMMU present. And we also support VFIO drivers. The VFIO API
for pseries (sPAPR section in Documentation/vfio.txt) has extra ioctls
to program the IOMMU.

For our use case, we don't need address translation because we set up an
identity mapping in the IOMMU so that the device can use guest physical
addresses.

If the guest kernel is concerned that an unprivileged driver could
jeopardize its integrity it should not negotiate this feature flag.
Perhaps there should be a note about this in the flag definition? This
concern is platform-dependant though. I don't believe it's an issue in
pseries.

--
Thiago Jung Bauermann
IBM Linux Technology Center

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 6/8] virtio/s390: add indirection to indicators access

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 17:55:54 +0200
Cornelia Huck  wrote:

> On Wed, 29 May 2019 14:26:55 +0200
> Michael Mueller  wrote:
> 
> > From: Halil Pasic 
> > 
> > This will come in handy soon when we pull out the indicators from
> > virtio_ccw_device to a memory area that is shared with the hypervisor
> > (in particular for protected virtualization guests).
> > 
> > Signed-off-by: Halil Pasic 
> > Reviewed-by: Pierre Morel 
> > Signed-off-by: Michael Mueller 
> > ---
> >  drivers/s390/virtio/virtio_ccw.c | 40 
> > +---
> >  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> Reviewed-by: Cornelia Huck 
> 

Thanks!

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 8/8] virtio/s390: make airq summary indicators DMA

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 18:03:37 +0200
Cornelia Huck  wrote:

> On Wed, 29 May 2019 14:26:57 +0200
> Michael Mueller  wrote:
> 
> > From: Halil Pasic 
> > 
> > Hypervisor needs to interact with the summary indicators, so these
> > need to be DMA memory as well (at least for protected virtualization
> > guests).
> > 
> > Signed-off-by: Halil Pasic 
> > Signed-off-by: Michael Mueller 
> > ---
> >  drivers/s390/virtio/virtio_ccw.c | 26 +++---
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> (...)
> 
> > @@ -1501,6 +1508,11 @@ static int __init virtio_ccw_init(void)
> >  {
> > /* parse no_auto string before we do anything further */
> > no_auto_parse();
> > +
> > +   summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS);
> > +   if (!summary_indicators)
> > +   return -ENOMEM;
> > +
> > return ccw_driver_register(_ccw_driver);
> 
> Don't you need to free summary_indicators again if registering the
> driver fails?
> 

We do! BTW as of today I'm back and I intend to handle things
regularly form now on ;).

Regards,
Halil

> >  }
> >  device_initcall(virtio_ccw_init);
> 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH 22/22] docs: fix broken documentation links

2019-06-03 Thread Mark Brown
On Wed, May 29, 2019 at 08:23:53PM -0300, Mauro Carvalho Chehab wrote:
> Mostly due to x86 and acpi conversion, several documentation
> links are still pointing to the old file. Fix them.

Acked-by: Mark Brown 


signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v3 7/8] virtio/s390: use DMA memory for ccw I/O and classic notifiers

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:56 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> Before virtio-ccw could get away with not using DMA API for the pieces of
> memory it does ccw I/O with. With protected virtualization this has to
> change, since the hypervisor needs to read and sometimes also write these
> pieces of memory.
> 
> The hypervisor is supposed to poke the classic notifiers, if these are
> used, out of band with regards to ccw I/O. So these need to be allocated
> as DMA memory (which is shared memory for protected virtualization
> guests).
> 
> Let us factor out everything from struct virtio_ccw_device that needs to
> be DMA memory in a satellite that is allocated as such.
> 
> Note: The control blocks of I/O instructions do not need to be shared.
> These are marshalled by the ultravisor.
> 
> Signed-off-by: Halil Pasic 
> Reviewed-by: Pierre Morel 
> Signed-off-by: Michael Mueller 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 177 
> +--
>  1 file changed, 96 insertions(+), 81 deletions(-)
> 

(...)

> @@ -176,6 +180,22 @@ static struct virtio_ccw_device *to_vc_device(struct 
> virtio_device *vdev)
>   return container_of(vdev, struct virtio_ccw_device, vdev);
>  }
>  
> +static inline void *__vc_dma_alloc(struct virtio_device *vdev, size_t size)
> +{
> + return ccw_device_dma_zalloc(to_vc_device(vdev)->cdev, size);
> +}
> +
> +static inline void __vc_dma_free(struct virtio_device *vdev, size_t size,
> +  void *cpu_addr)
> +{
> + return ccw_device_dma_free(to_vc_device(vdev)->cdev, cpu_addr, size);
> +}
> +
> +#define vc_dma_alloc_struct(vdev, ptr) \
> + ({ptr = __vc_dma_alloc(vdev, sizeof(*(ptr))); })
> +#define vc_dma_free_struct(vdev, ptr) \
> + __vc_dma_free(vdev, sizeof(*(ptr)), (ptr))
> +

I *still* don't like these #defines (and the __vc_dma_* functions), as I
already commented last time. I think they make it harder to follow the
code.

>  static void drop_airq_indicator(struct virtqueue *vq, struct airq_info *info)
>  {
>   unsigned long i, flags;
> @@ -336,8 +356,7 @@ static void virtio_ccw_drop_indicator(struct 
> virtio_ccw_device *vcdev,
>   struct airq_info *airq_info = vcdev->airq_info;
>  
>   if (vcdev->is_thinint) {
> - thinint_area = kzalloc(sizeof(*thinint_area),
> -GFP_DMA | GFP_KERNEL);
> + vc_dma_alloc_struct(>vdev, thinint_area);

Last time I wrote:

"Any reason why this takes a detour via the virtio device? The ccw
 device is already referenced in vcdev, isn't it?

thinint_area = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*thinint_area));

 looks much more obvious to me."

It still seems more obvious to me.

>   if (!thinint_area)
>   return;
>   thinint_area->summary_indicator =
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 8/8] virtio/s390: make airq summary indicators DMA

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:57 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> Hypervisor needs to interact with the summary indicators, so these
> need to be DMA memory as well (at least for protected virtualization
> guests).
> 
> Signed-off-by: Halil Pasic 
> Signed-off-by: Michael Mueller 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 26 +++---
>  1 file changed, 19 insertions(+), 7 deletions(-)

(...)

> @@ -1501,6 +1508,11 @@ static int __init virtio_ccw_init(void)
>  {
>   /* parse no_auto string before we do anything further */
>   no_auto_parse();
> +
> + summary_indicators = cio_dma_zalloc(MAX_AIRQ_AREAS);
> + if (!summary_indicators)
> + return -ENOMEM;
> +
>   return ccw_driver_register(_ccw_driver);

Don't you need to free summary_indicators again if registering the
driver fails?

>  }
>  device_initcall(virtio_ccw_init);

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 6/8] virtio/s390: add indirection to indicators access

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:55 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> This will come in handy soon when we pull out the indicators from
> virtio_ccw_device to a memory area that is shared with the hypervisor
> (in particular for protected virtualization guests).
> 
> Signed-off-by: Halil Pasic 
> Reviewed-by: Pierre Morel 
> Signed-off-by: Michael Mueller 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 40 
> +---
>  1 file changed, 25 insertions(+), 15 deletions(-)

Reviewed-by: Cornelia Huck 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 5/8] virtio/s390: use cacheline aligned airq bit vectors

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:54 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> The flag AIRQ_IV_CACHELINE was recently added to airq_iv_create(). Let
> us use it! We actually wanted the vector to span a cacheline all along.
> 
> Signed-off-by: Halil Pasic 
> Signed-off-by: Michael Mueller 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 4/8] s390/airq: use DMA memory for adapter interrupts

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:53 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> Protected virtualization guests have to use shared pages for airq
> notifier bit vectors, because hypervisor needs to write these bits.
> 
> Let us make sure we allocate DMA memory for the notifier bit vectors by
> replacing the kmem_cache with a dma_cache and kalloc() with
> cio_dma_zalloc().
> 
> Signed-off-by: Halil Pasic 
> Reviewed-by: Sebastian Ott 
> Signed-off-by: Michael Mueller 
> ---
>  arch/s390/include/asm/airq.h |  2 ++
>  drivers/s390/cio/airq.c  | 32 
>  drivers/s390/cio/cio.h   |  2 ++
>  drivers/s390/cio/css.c   |  1 +
>  4 files changed, 25 insertions(+), 12 deletions(-)

Apologies if that already has been answered (and I missed it in my mail
pile...), but two things had come to my mind previously:

- CHSC... does anything need to be done there? Last time I asked:
  "Anyway, css_bus_init() uses some chscs
   early (before cio_dma_pool_init), so we could not use the pools
   there, even if we wanted to. Do chsc commands either work, or else
   fail benignly on a protected virt guest?"
- PCI indicators... does this interact with any dma configuration on
  the pci device? (I know pci is not supported yet, and I don't really
  expect any problems.)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[PULL] vhost: cleanups and fixes

2019-06-03 Thread Michael S. Tsirkin
The following changes since commit cd6c84d8f0cdc911df435bb075ba22ce3c605b07:

  Linux 5.2-rc2 (2019-05-26 16:49:19 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus

for you to fetch changes up to c1ea02f15ab5efb3e93fc3144d895410bf79fcf2:

  vhost: scsi: add weight support (2019-05-27 11:08:23 -0400)


virtio: fixes

several fixes, some of them for CVEs.

Signed-off-by: Michael S. Tsirkin 


Fabrizio Castro (1):
  virtio: Fix indentation of VIRTIO_MMIO

Igor Stoppa (1):
  virtio: add unlikely() to WARN_ON_ONCE()

Jason Wang (4):
  vhost: introduce vhost_exceeds_weight()
  vhost_net: fix possible infinite loop
  vhost: vsock: add weight support
  vhost: scsi: add weight support

 drivers/vhost/net.c | 41 ++---
 drivers/vhost/scsi.c| 21 ++---
 drivers/vhost/vhost.c   | 20 +++-
 drivers/vhost/vhost.h   |  5 -
 drivers/vhost/vsock.c   | 28 +---
 drivers/virtio/Kconfig  |  8 
 tools/virtio/linux/kernel.h |  2 +-
 7 files changed, 77 insertions(+), 48 deletions(-)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 16:04:28 +0200
Halil Pasic  wrote:

> On Mon, 3 Jun 2019 14:09:02 +0200
> Michael Mueller  wrote:
> 
> > >> @@ -1059,16 +1168,19 @@ static int __init css_bus_init(void)
> > >>  if (ret)
> > >>  goto out_unregister;
> > >>  ret = register_pm_notifier(_power_notifier);
> > >> -if (ret) {
> > >> -unregister_reboot_notifier(_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!
> 
> I suppose we could also move cio_dma_pool_init() right before the
> register_reboot_notifier() call and goto out_unregister on error.
> 

Forget it, then we have to rollback the pool creation if the register
stuff fails... Sorry for the noise.

Regards,
Halil

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 14:09:02 +0200
Michael Mueller  wrote:

> >> @@ -1059,16 +1168,19 @@ static int __init css_bus_init(void)
> >>if (ret)
> >>goto out_unregister;
> >>ret = register_pm_notifier(_power_notifier);
> >> -  if (ret) {
> >> -  unregister_reboot_notifier(_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!

I suppose we could also move cio_dma_pool_init() right before the
register_reboot_notifier() call and goto out_unregister on error.

Regards,
Halil

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support

2019-06-03 Thread Michael Mueller




On 03.06.19 15:42, Cornelia Huck wrote:

On Mon, 3 Jun 2019 14:45:03 +0200
Michael Mueller  wrote:


On 03.06.19 14:06, Cornelia Huck wrote:

On Wed, 29 May 2019 14:26:52 +0200
Michael Mueller  wrote:



@@ -1593,20 +1625,31 @@ struct ccw_device * __init 
ccw_device_create_console(struct ccw_driver *drv)
return ERR_CAST(sch);
   
   	io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA);

-   if (!io_priv) {
-   put_device(>dev);
-   return ERR_PTR(-ENOMEM);
-   }
+   if (!io_priv)
+   goto err_priv;
+   io_priv->dma_area = dma_alloc_coherent(>dev,
+   sizeof(*io_priv->dma_area),
+   _priv->dma_area_dma, GFP_KERNEL);
+   if (!io_priv->dma_area)
+   goto err_dma_area;
set_io_private(sch, io_priv);
cdev = io_subchannel_create_ccwdev(sch);
if (IS_ERR(cdev)) {
put_device(>dev);
+   dma_free_coherent(>dev, sizeof(*io_priv->dma_area),
+ io_priv->dma_area, io_priv->dma_area_dma);
kfree(io_priv);



Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any
code would make use of it, but it's probably better to clean out
references to freed objects.


Added behind kfree(). I hope nobody asks for a separate patch. ;)


I would probably have added it just before the kfree, but I'm not
asking for a separate patch ;)


I moved it.







   

return cdev;
}
cdev->drv = drv;
ccw_device_set_int_class(cdev);
return cdev;
+
+err_dma_area:
+   kfree(io_priv);
+err_priv:
+   put_device(>dev);
+   return ERR_PTR(-ENOMEM);
   }
   
   void __init ccw_device_destroy_console(struct ccw_device *cdev)




___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Michael Mueller




On 03.06.19 15:34, Cornelia Huck wrote:

On Mon, 3 Jun 2019 14:57:30 +0200
Halil Pasic  wrote:


On Mon, 3 Jun 2019 14:09:02 +0200
Michael Mueller  wrote:


@@ -224,6 +226,8 @@ struct subchannel *css_alloc_subchannel(struct 
subchannel_id schid,
INIT_WORK(>todo_work, css_sch_todo);
sch->dev.release = _subchannel_release;
device_initialize(>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...
   


How about?

/*
  * The physical addresses of some the dma structures that
  * can belong  to a subchannel need to fit 31 bit width (examples ccw,).
  */


"e.g. ccw"?




 

+   sch->dev.coherent_dma_mask = DMA_BIT_MASK(31);
+   sch->dev.dma_mask = >dev.coherent_dma_mask;
return sch;
   
   err:

@@ -899,6 +903,8 @@ static int __init setup_css(int nr)
dev_set_name(>device, "css%x", nr);
css->device.groups = cssdev_attr_groups;
css->device.release = channel_subsystem_release;


...and 64 bit here.


and here.


/*
  * We currently allocate notifier bits with this (using css->device
  * as the device argument with the DMA API), and are fine with 64 bit
  * addresses.
  */


Thanks, that makes things hopefully clearer if we look at it some time
in the future ;)



Applied both with with requested change.

Michael

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support

2019-06-03 Thread Cornelia Huck
On Mon, 3 Jun 2019 14:45:03 +0200
Michael Mueller  wrote:

> On 03.06.19 14:06, Cornelia Huck wrote:
> > On Wed, 29 May 2019 14:26:52 +0200
> > Michael Mueller  wrote:

> >> @@ -1593,20 +1625,31 @@ struct ccw_device * __init 
> >> ccw_device_create_console(struct ccw_driver *drv)
> >>return ERR_CAST(sch);
> >>   
> >>io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA);
> >> -  if (!io_priv) {
> >> -  put_device(>dev);
> >> -  return ERR_PTR(-ENOMEM);
> >> -  }
> >> +  if (!io_priv)
> >> +  goto err_priv;
> >> +  io_priv->dma_area = dma_alloc_coherent(>dev,
> >> +  sizeof(*io_priv->dma_area),
> >> +  _priv->dma_area_dma, GFP_KERNEL);
> >> +  if (!io_priv->dma_area)
> >> +  goto err_dma_area;
> >>set_io_private(sch, io_priv);
> >>cdev = io_subchannel_create_ccwdev(sch);
> >>if (IS_ERR(cdev)) {
> >>put_device(>dev);
> >> +  dma_free_coherent(>dev, sizeof(*io_priv->dma_area),
> >> +io_priv->dma_area, io_priv->dma_area_dma);
> >>kfree(io_priv);  
> > 
> > 
> > Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any
> > code would make use of it, but it's probably better to clean out
> > references to freed objects.  
> 
> Added behind kfree(). I hope nobody asks for a separate patch. ;)

I would probably have added it just before the kfree, but I'm not
asking for a separate patch ;)

> 
> > 
> >   
> >>return cdev;
> >>}
> >>cdev->drv = drv;
> >>ccw_device_set_int_class(cdev);
> >>return cdev;
> >> +
> >> +err_dma_area:
> >> +  kfree(io_priv);
> >> +err_priv:
> >> +  put_device(>dev);
> >> +  return ERR_PTR(-ENOMEM);
> >>   }
> >>   
> >>   void __init ccw_device_destroy_console(struct ccw_device *cdev)  
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Cornelia Huck
On Mon, 3 Jun 2019 14:47:06 +0200
Halil Pasic  wrote:

> On Mon, 3 Jun 2019 13:37:45 +0200
> Cornelia Huck  wrote:
> 
> > On Wed, 29 May 2019 14:26:51 +0200
> > Michael Mueller  wrote:

> > > 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...   
> 
> Can you explain to me what is ugly about it so I can avoid similar
> mistakes in the future?
> 
> >I guess the alternative was
> > include hell?
> >   
> 
> What do you mean by include hell?
> 
> I decided to use a forward declaration because the guys that include
> "cio.h" are not expected to require the interfaces defined in
> linux/genalloc.h. My motivation to do it like this was the principle of
> encapsulation.

My personal rule-of-thumb is to include the header if it is
straightforward enough (e.g. if adding a basic header is enough). If
you need to include a header together with all of its friends and
family, a forward declaration is probably nicer. And of course,
sometimes it is simply needed.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Cornelia Huck
On Mon, 3 Jun 2019 14:57:30 +0200
Halil Pasic  wrote:

> On Mon, 3 Jun 2019 14:09:02 +0200
> Michael Mueller  wrote:
> 
> > >> @@ -224,6 +226,8 @@ struct subchannel *css_alloc_subchannel(struct 
> > >> subchannel_id schid,
> > >>  INIT_WORK(>todo_work, css_sch_todo);
> > >>  sch->dev.release = _subchannel_release;
> > >>  device_initialize(>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...
> >   
> 
> How about?
> 
> /*
>  * The physical addresses of some the dma structures that
>  * can belong  to a subchannel need to fit 31 bit width (examples ccw,).
>  */

"e.g. ccw"?

> 
> 
> > > 
> > >> +sch->dev.coherent_dma_mask = DMA_BIT_MASK(31);
> > >> +sch->dev.dma_mask = >dev.coherent_dma_mask;
> > >>  return sch;
> > >>   
> > >>   err:
> > >> @@ -899,6 +903,8 @@ static int __init setup_css(int nr)
> > >>  dev_set_name(>device, "css%x", nr);
> > >>  css->device.groups = cssdev_attr_groups;
> > >>  css->device.release = channel_subsystem_release;
> > > 
> > > ...and 64 bit here.
> > 
> > and here.  
> 
> /*
>  * We currently allocate notifier bits with this (using css->device
>  * as the device argument with the DMA API), and are fine with 64 bit
>  * addresses.
>  */

Thanks, that makes things hopefully clearer if we look at it some time
in the future ;)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 14:09:02 +0200
Michael Mueller  wrote:

> >> @@ -224,6 +226,8 @@ struct subchannel *css_alloc_subchannel(struct 
> >> subchannel_id schid,
> >>INIT_WORK(>todo_work, css_sch_todo);
> >>sch->dev.release = _subchannel_release;
> >>device_initialize(>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...
> 

How about?

/*
 * The physical addresses of some the dma structures that
 * can belong  to a subchannel need to fit 31 bit width (examples ccw,).
 */


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

/*
 * We currently allocate notifier bits with this (using css->device
 * as the device argument with the DMA API), and are fine with 64 bit
 * addresses.
 */

Regards,
Halil

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Halil Pasic
On Mon, 3 Jun 2019 13:37:45 +0200
Cornelia Huck  wrote:

> 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... 

Can you explain to me what is ugly about it so I can avoid similar
mistakes in the future?

>I guess the alternative was
> include hell?
> 

What do you mean by include hell?

I decided to use a forward declaration because the guys that include
"cio.h" are not expected to require the interfaces defined in
linux/genalloc.h. My motivation to do it like this was the principle of
encapsulation.

Regards,
Halil

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support

2019-06-03 Thread Michael Mueller




On 03.06.19 14:06, Cornelia Huck wrote:

On Wed, 29 May 2019 14:26:52 +0200
Michael Mueller  wrote:


From: Halil Pasic 

As virtio-ccw devices are channel devices, we need to use the dma area
for any communication with the hypervisor.


"we need to use the dma area within the common I/O layer for any
communication with the hypervisor. Note that we do not need to use that
area for control blocks directly referenced by instructions, e.g. the
orb."


using this now



...although I'm still not particularly confident about the actual
distinction here? I'm trusting you that you actually have tested it,
though :)



It handles neither QDIO in the common code, nor any device type specific
stuff (like channel programs constructed by the DASD driver).

An interesting side effect is that virtio structures are now going to
get allocated in 31 bit addressable storage.

Signed-off-by: Halil Pasic 
Reviewed-by: Sebastian Ott 
Signed-off-by: Michael Mueller 
---
  arch/s390/include/asm/ccwdev.h   |  4 +++
  drivers/s390/cio/ccwreq.c|  9 +++---
  drivers/s390/cio/device.c| 67 +---
  drivers/s390/cio/device_fsm.c| 49 +
  drivers/s390/cio/device_id.c | 20 ++--
  drivers/s390/cio/device_ops.c| 21 +++--
  drivers/s390/cio/device_pgid.c   | 22 +++--
  drivers/s390/cio/device_status.c | 24 +++---
  drivers/s390/cio/io_sch.h| 20 +---
  drivers/s390/virtio/virtio_ccw.c | 10 --
  10 files changed, 163 insertions(+), 83 deletions(-)


(...)


@@ -1593,20 +1625,31 @@ struct ccw_device * __init 
ccw_device_create_console(struct ccw_driver *drv)
return ERR_CAST(sch);
  
  	io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA);

-   if (!io_priv) {
-   put_device(>dev);
-   return ERR_PTR(-ENOMEM);
-   }
+   if (!io_priv)
+   goto err_priv;
+   io_priv->dma_area = dma_alloc_coherent(>dev,
+   sizeof(*io_priv->dma_area),
+   _priv->dma_area_dma, GFP_KERNEL);
+   if (!io_priv->dma_area)
+   goto err_dma_area;
set_io_private(sch, io_priv);
cdev = io_subchannel_create_ccwdev(sch);
if (IS_ERR(cdev)) {
put_device(>dev);
+   dma_free_coherent(>dev, sizeof(*io_priv->dma_area),
+ io_priv->dma_area, io_priv->dma_area_dma);
kfree(io_priv);



Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any
code would make use of it, but it's probably better to clean out
references to freed objects.


Added behind kfree(). I hope nobody asks for a separate patch. ;)





return cdev;
}
cdev->drv = drv;
ccw_device_set_int_class(cdev);
return cdev;
+
+err_dma_area:
+   kfree(io_priv);
+err_priv:
+   put_device(>dev);
+   return ERR_PTR(-ENOMEM);
  }
  
  void __init ccw_device_destroy_console(struct ccw_device *cdev)


With the reservations above,
Reviewed-by: Cornelia Huck 



Thanks,
Michael

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Michael Mueller




On 03.06.19 13:37, Cornelia Huck wrote:

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?


That's an easy one.

 #include 
+#include 
 #include 




+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(>todo_work, css_sch_todo);
sch->dev.release = _subchannel_release;
device_initialize(>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 = >dev.coherent_dma_mask;
return sch;
  
  err:

@@ -899,6 +903,8 @@ static int __init setup_css(int nr)
dev_set_name(>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 = >device.coherent_dma_mask;
  
  	mutex_init(>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(_power_notifier);
-   if (ret) {
-   unregister_reboot_notifier(_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(_reboot_notifier);
  out_unregister:
while (i-- > 0) {
struct channel_subsystem *css = channel_subsystems[i];




Thanks,
Michael

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 3/8] s390/cio: add basic protected virtualization support

2019-06-03 Thread Cornelia Huck
On Wed, 29 May 2019 14:26:52 +0200
Michael Mueller  wrote:

> From: Halil Pasic 
> 
> As virtio-ccw devices are channel devices, we need to use the dma area
> for any communication with the hypervisor.

"we need to use the dma area within the common I/O layer for any
communication with the hypervisor. Note that we do not need to use that
area for control blocks directly referenced by instructions, e.g. the
orb."

...although I'm still not particularly confident about the actual
distinction here? I'm trusting you that you actually have tested it,
though :)

> 
> It handles neither QDIO in the common code, nor any device type specific
> stuff (like channel programs constructed by the DASD driver).
> 
> An interesting side effect is that virtio structures are now going to
> get allocated in 31 bit addressable storage.
> 
> Signed-off-by: Halil Pasic 
> Reviewed-by: Sebastian Ott 
> Signed-off-by: Michael Mueller 
> ---
>  arch/s390/include/asm/ccwdev.h   |  4 +++
>  drivers/s390/cio/ccwreq.c|  9 +++---
>  drivers/s390/cio/device.c| 67 
> +---
>  drivers/s390/cio/device_fsm.c| 49 +
>  drivers/s390/cio/device_id.c | 20 ++--
>  drivers/s390/cio/device_ops.c| 21 +++--
>  drivers/s390/cio/device_pgid.c   | 22 +++--
>  drivers/s390/cio/device_status.c | 24 +++---
>  drivers/s390/cio/io_sch.h| 20 +---
>  drivers/s390/virtio/virtio_ccw.c | 10 --
>  10 files changed, 163 insertions(+), 83 deletions(-)

(...)

> @@ -1593,20 +1625,31 @@ struct ccw_device * __init 
> ccw_device_create_console(struct ccw_driver *drv)
>   return ERR_CAST(sch);
>  
>   io_priv = kzalloc(sizeof(*io_priv), GFP_KERNEL | GFP_DMA);
> - if (!io_priv) {
> - put_device(>dev);
> - return ERR_PTR(-ENOMEM);
> - }
> + if (!io_priv)
> + goto err_priv;
> + io_priv->dma_area = dma_alloc_coherent(>dev,
> + sizeof(*io_priv->dma_area),
> + _priv->dma_area_dma, GFP_KERNEL);
> + if (!io_priv->dma_area)
> + goto err_dma_area;
>   set_io_private(sch, io_priv);
>   cdev = io_subchannel_create_ccwdev(sch);
>   if (IS_ERR(cdev)) {
>   put_device(>dev);
> + dma_free_coherent(>dev, sizeof(*io_priv->dma_area),
> +   io_priv->dma_area, io_priv->dma_area_dma);
>   kfree(io_priv);


Shouldn't that branch do set_io_private(sch, NULL)? Not sure if any
code would make use of it, but it's probably better to clean out
references to freed objects.


>   return cdev;
>   }
>   cdev->drv = drv;
>   ccw_device_set_int_class(cdev);
>   return cdev;
> +
> +err_dma_area:
> + kfree(io_priv);
> +err_priv:
> + put_device(>dev);
> + return ERR_PTR(-ENOMEM);
>  }
>  
>  void __init ccw_device_destroy_console(struct ccw_device *cdev)

With the reservations above,
Reviewed-by: Cornelia Huck 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


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

2019-06-03 Thread Cornelia Huck
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(>todo_work, css_sch_todo);
>   sch->dev.release = _subchannel_release;
>   device_initialize(>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 = >dev.coherent_dma_mask;
>   return sch;
>  
>  err:
> @@ -899,6 +903,8 @@ static int __init setup_css(int nr)
>   dev_set_name(>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 = >device.coherent_dma_mask;
>  
>   mutex_init(>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(_power_notifier);
> - if (ret) {
> - unregister_reboot_notifier(_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(_reboot_notifier);
>  out_unregister:
>   while (i-- > 0) {
>   struct channel_subsystem *css = channel_subsystems[i];

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 2/5] vsock/virtio: fix locking for fwd_cnt and buf_alloc

2019-06-03 Thread Stefano Garzarella
On Sun, Jun 02, 2019 at 06:03:34PM -0700, David Miller wrote:
> From: Stefano Garzarella 
> Date: Fri, 31 May 2019 15:39:51 +0200
> 
> > @@ -434,7 +434,9 @@ void virtio_transport_set_buffer_size(struct vsock_sock 
> > *vsk, u64 val)
> > if (val > vvs->buf_size_max)
> > vvs->buf_size_max = val;
> > vvs->buf_size = val;
> > +   spin_lock_bh(>rx_lock);
> > vvs->buf_alloc = val;
> > +   spin_unlock_bh(>rx_lock);
> 
> This locking doesn't do anything other than to strongly order the
> buf_size store to occur before the buf_alloc one.

Sure, I'll remove the lock. I was confused because I moved its reading
under the rx_lock (together with other variables), but here I'm updating
only buf_alloc, so this lock is useless.

Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH v3 5/8] virtio/s390: use cacheline aligned airq bit vectors

2019-06-03 Thread Christian Borntraeger



On 29.05.19 14:26, Michael Mueller wrote:
> From: Halil Pasic 
> 
> The flag AIRQ_IV_CACHELINE was recently added to airq_iv_create(). Let
> us use it! We actually wanted the vector to span a cacheline all along.

Given that VIRTIO_IV_BITS is (L1_CACHE_BYTES * 8) this makes perfect sense.

Reviewed-by: Christian Borntraeger 

> 
> Signed-off-by: Halil Pasic 
> Signed-off-by: Michael Mueller 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c 
> b/drivers/s390/virtio/virtio_ccw.c
> index f995798bb025..1da7430f94c8 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -216,7 +216,8 @@ static struct airq_info *new_airq_info(void)
>   if (!info)
>   return NULL;
>   rwlock_init(>lock);
> - info->aiv = airq_iv_create(VIRTIO_IV_BITS, AIRQ_IV_ALLOC | AIRQ_IV_PTR);
> + info->aiv = airq_iv_create(VIRTIO_IV_BITS, AIRQ_IV_ALLOC | AIRQ_IV_PTR
> +| AIRQ_IV_CACHELINE);
>   if (!info->aiv) {
>   kfree(info);
>   return NULL;
> 

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization