[RFC,00/12] s390: virtio: support protected virtualization
mbox series

Message ID 20190404231622.52531-1-pasic@linux.ibm.com
Headers show
Series
  • s390: virtio: support protected virtualization
Related show

Message

Halil Pasic April 4, 2019, 11:16 p.m. UTC
Enhanced virtualization protection technology may require the use of
bounce buffers for I/O. While support for this was built into the virtio
core,  virtio-ccw wasn't changed accordingly.

Thus what needs to be done to bring virtio-ccw up to speed with respect
to this is:
* use some 'new' common virtio stuff
* make sure that virtio-ccw specific stuff uses shared memory when
  talking to the hypervisor (except communication blocks like ORB, these
  are handled by the hypervisor)
* make sure the DMA API does what is necessary to talk through shared
  memory if we are a protected virtualization guest.
* make sure the common IO layer plays along as well (airqs, sense).

The series is structured in incremental fashion: some of the changes are
overridden by following patches. The main reason why is that this is how I
developed. But I think it ain't bad for the didactic and we are a bit more
flexible with regards to throwing out some of the stuff in the end.

Important notes:

* This is an early (WIP) RFC that does not add any function to the
  kernel at his stage, as the ultravisor interactions are left out.
  The purpose is getting some early feedback ASAP.

* In future these patches will depend on some code interacting with the
  ultravisor (WIP by Vasily and Janosch).

* The s390 names are by no means final, and are not properly explained. Should
  not hamper understanding too much. If it does please ask.

* The existing naming in the common infrastructure (kernel internal
  interfaces) is pretty much based on the AMD SEV terminology. Thus the
  names aren't always perfect. There might be merit to changing these
  names to more abstract ones. I did not put much thought into that at
  the current stage.


Testing:

Please use iommu_platform=on for any virtio devices you are going
to test this code with (so virtio actually uses the DMA API).

Looking forward to your review or any other type of input.

Halil Pasic (12):
  virtio/s390: use vring_create_virtqueue
  virtio/s390: DMA support for virtio-ccw
  s390/mm: force swiotlb for protected virtualization
  s390/cio: introduce cio DMA pool
  s390/cio: add protected virtualization support to cio
  s390/airq: use DMA memory for adapter interrupts
  virtio/s390: use DMA memory for ccw I/O
  virtio/s390: add indirection to indicators access
  virtio/s390: use DMA memory for notifiers
  virtio/s390: consolidate DMA allocations
  virtio/s390: use the cio DMA pool
  virtio/s390:  make airq summary indicators DMA

 arch/s390/Kconfig                   |   5 +
 arch/s390/include/asm/Kbuild        |   1 -
 arch/s390/include/asm/airq.h        |   2 +
 arch/s390/include/asm/cio.h         |   4 +
 arch/s390/include/asm/dma-mapping.h |  13 ++
 arch/s390/include/asm/mem_encrypt.h |  18 +++
 arch/s390/mm/init.c                 |  44 +++++
 drivers/s390/cio/airq.c             |  18 ++-
 drivers/s390/cio/ccwreq.c           |   8 +-
 drivers/s390/cio/css.c              |  63 ++++++++
 drivers/s390/cio/device.c           |  46 ++++--
 drivers/s390/cio/device_fsm.c       |  40 ++---
 drivers/s390/cio/device_id.c        |  18 +--
 drivers/s390/cio/device_ops.c       |   4 +-
 drivers/s390/cio/device_pgid.c      |  20 +--
 drivers/s390/cio/device_status.c    |  24 +--
 drivers/s390/cio/io_sch.h           |  19 ++-
 drivers/s390/virtio/virtio_ccw.c    | 310 ++++++++++++++++++++----------------
 18 files changed, 444 insertions(+), 213 deletions(-)
 create mode 100644 arch/s390/include/asm/dma-mapping.h
 create mode 100644 arch/s390/include/asm/mem_encrypt.h

Comments

Cornelia Huck April 10, 2019, 9:20 a.m. UTC | #1
On Fri,  5 Apr 2019 01:16:10 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> Enhanced virtualization protection technology may require the use of
> bounce buffers for I/O. While support for this was built into the virtio
> core,  virtio-ccw wasn't changed accordingly.
> 
> Thus what needs to be done to bring virtio-ccw up to speed with respect
> to this is:
> * use some 'new' common virtio stuff
> * make sure that virtio-ccw specific stuff uses shared memory when
>   talking to the hypervisor (except communication blocks like ORB, these
>   are handled by the hypervisor)
> * make sure the DMA API does what is necessary to talk through shared
>   memory if we are a protected virtualization guest.
> * make sure the common IO layer plays along as well (airqs, sense).

It would be good to have a summary somewhere in the code (or
Documentation/) as to what needs the dma treatment and what doesn't,
for later reference. We don't want people to accidentally break things
(especially if they cannot refer to architecture documentation - or
will at least some of that be published?)

> 
> The series is structured in incremental fashion: some of the changes are
> overridden by following patches. The main reason why is that this is how I
> developed. But I think it ain't bad for the didactic and we are a bit more
> flexible with regards to throwing out some of the stuff in the end.

FWIW, I think reshuffling the patches in the next iteration would ease
review.

> 
> Important notes:
> 
> * This is an early (WIP) RFC that does not add any function to the
>   kernel at his stage, as the ultravisor interactions are left out.
>   The purpose is getting some early feedback ASAP.

I would like some comments from people who have experience with the dma
api.

> 
> * In future these patches will depend on some code interacting with the
>   ultravisor (WIP by Vasily and Janosch).
> 
> * The s390 names are by no means final, and are not properly explained. Should
>   not hamper understanding too much. If it does please ask.
> 
> * The existing naming in the common infrastructure (kernel internal
>   interfaces) is pretty much based on the AMD SEV terminology. Thus the
>   names aren't always perfect. There might be merit to changing these
>   names to more abstract ones. I did not put much thought into that at
>   the current stage.

If we can find some generic names that work well for everyone,
converting seems like a good idea. But following SEV is not that bad,
either (you'll probably find more people who have heard about SEV than
folks familiar with s390 ;)

> 
> 
> Testing:
> 
> Please use iommu_platform=on for any virtio devices you are going
> to test this code with (so virtio actually uses the DMA API).
> 
> Looking forward to your review or any other type of input.

I have now read through the whole series and commented in some places.
But I'd really like to see comments from others as well.
Halil Pasic April 10, 2019, 3:57 p.m. UTC | #2
On Wed, 10 Apr 2019 11:20:48 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri,  5 Apr 2019 01:16:10 +0200
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > Enhanced virtualization protection technology may require the use of
> > bounce buffers for I/O. While support for this was built into the
> > virtio core,  virtio-ccw wasn't changed accordingly.
> > 
> > Thus what needs to be done to bring virtio-ccw up to speed with
> > respect to this is:
> > * use some 'new' common virtio stuff
> > * make sure that virtio-ccw specific stuff uses shared memory when
> >   talking to the hypervisor (except communication blocks like ORB,
> > these are handled by the hypervisor)
> > * make sure the DMA API does what is necessary to talk through shared
> >   memory if we are a protected virtualization guest.
> > * make sure the common IO layer plays along as well (airqs, sense).
> 
> It would be good to have a summary somewhere in the code (or
> Documentation/) as to what needs the dma treatment and what doesn't,
> for later reference. We don't want people to accidentally break things
> (especially if they cannot refer to architecture documentation - or
> will at least some of that be published?)
> 

I can put documentation on my TODO list. This cover letter was also
supposed to provide a bird's-eye view on what needs to be done.

> > 
> > The series is structured in incremental fashion: some of the changes
> > are overridden by following patches. The main reason why is that
> > this is how I developed. But I think it ain't bad for the didactic
> > and we are a bit more flexible with regards to throwing out some of
> > the stuff in the end.
> 
> FWIW, I think reshuffling the patches in the next iteration would ease
> review.
> 

Can you please tell me more about what is desired here? I mean, I made
some tentative proposals on squashing some patches together. I don't
remember any requests to reorder patches or split.

> > 
> > Important notes:
> > 
> > * This is an early (WIP) RFC that does not add any function to the
> >   kernel at his stage, as the ultravisor interactions are left out.
> >   The purpose is getting some early feedback ASAP.
> 
> I would like some comments from people who have experience with the dma
> api.
> 

I can only second that! 

From the Linux on Z folks Sebastian seems to be the most likely suspect.
I've briefly shown this series to him before posting here, and I asked
him to have another look today. Unfortunately he seems quite busy at the
moment, but he promised me that he will make some time for this
(soonish).

Getting some more feedback from Christoph Hellwig would be nice as well.

> > 
> > * In future these patches will depend on some code interacting with
> > the ultravisor (WIP by Vasily and Janosch).
> > 
> > * The s390 names are by no means final, and are not properly
> > explained. Should not hamper understanding too much. If it does
> > please ask.
> > 
> > * The existing naming in the common infrastructure (kernel internal
> >   interfaces) is pretty much based on the AMD SEV terminology. Thus
> > the names aren't always perfect. There might be merit to changing
> > these names to more abstract ones. I did not put much thought into
> > that at the current stage.
> 
> If we can find some generic names that work well for everyone,
> converting seems like a good idea. But following SEV is not that bad,
> either (you'll probably find more people who have heard about SEV than
> folks familiar with s390 ;)
> 

I agree. We should not swap out the names for worse ones for sure. :)

I think the names discussion can be postponed. I would like to get the
code right first.

> > 
> > 
> > Testing:
> > 
> > Please use iommu_platform=on for any virtio devices you are going
> > to test this code with (so virtio actually uses the DMA API).
> > 
> > Looking forward to your review or any other type of input.
> 
> I have now read through the whole series and commented in some places.
> But I'd really like to see comments from others as well.
>

Thank you so much for the quick review. Thanks to your quality review I will
have a bunch of things to change (for the better) for v1.

I intend to wait a bit for more feedback and also for the my dependencies
to get published. I hope to send out a functional v1 mid next week.

Thank you again!

Regards,
Halil
Cornelia Huck April 10, 2019, 4:24 p.m. UTC | #3
On Wed, 10 Apr 2019 17:57:50 +0200
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 10 Apr 2019 11:20:48 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri,  5 Apr 2019 01:16:10 +0200
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > Enhanced virtualization protection technology may require the use of
> > > bounce buffers for I/O. While support for this was built into the
> > > virtio core,  virtio-ccw wasn't changed accordingly.
> > > 
> > > Thus what needs to be done to bring virtio-ccw up to speed with
> > > respect to this is:
> > > * use some 'new' common virtio stuff
> > > * make sure that virtio-ccw specific stuff uses shared memory when
> > >   talking to the hypervisor (except communication blocks like ORB,
> > > these are handled by the hypervisor)
> > > * make sure the DMA API does what is necessary to talk through shared
> > >   memory if we are a protected virtualization guest.
> > > * make sure the common IO layer plays along as well (airqs, sense).  
> > 
> > It would be good to have a summary somewhere in the code (or
> > Documentation/) as to what needs the dma treatment and what doesn't,
> > for later reference. We don't want people to accidentally break things
> > (especially if they cannot refer to architecture documentation - or
> > will at least some of that be published?)
> >   
> 
> I can put documentation on my TODO list. This cover letter was also
> supposed to provide a bird's-eye view on what needs to be done.

Some comments in the code to prevent further headscratching are also a
good idea.

> 
> > > 
> > > The series is structured in incremental fashion: some of the changes
> > > are overridden by following patches. The main reason why is that
> > > this is how I developed. But I think it ain't bad for the didactic
> > > and we are a bit more flexible with regards to throwing out some of
> > > the stuff in the end.  
> > 
> > FWIW, I think reshuffling the patches in the next iteration would ease
> > review.
> >   
> 
> Can you please tell me more about what is desired here? I mean, I made
> some tentative proposals on squashing some patches together. I don't
> remember any requests to reorder patches or split.

Just try to avoid to rewrite things multiple times -- if we agree on
the end result, we should be able to go there directly :)
David Hildenbrand April 12, 2019, 1:47 p.m. UTC | #4
On 05.04.19 01:16, Halil Pasic wrote:
> Enhanced virtualization protection technology may require the use of
> bounce buffers for I/O. While support for this was built into the virtio
> core,  virtio-ccw wasn't changed accordingly.

Can you elaborate some more about the general approach (Enhanced
virtualization protection technology, ultravisor, concept, issues, how
to squeeze it into QEMU/KVM/kernel) etc =

For my taste, this cover letter misses some important context :)

> 
> Thus what needs to be done to bring virtio-ccw up to speed with respect
> to this is:
> * use some 'new' common virtio stuff
> * make sure that virtio-ccw specific stuff uses shared memory when
>   talking to the hypervisor (except communication blocks like ORB, these
>   are handled by the hypervisor)
> * make sure the DMA API does what is necessary to talk through shared
>   memory if we are a protected virtualization guest.
> * make sure the common IO layer plays along as well (airqs, sense).
> 
> The series is structured in incremental fashion: some of the changes are
> overridden by following patches. The main reason why is that this is how I
> developed. But I think it ain't bad for the didactic and we are a bit more
> flexible with regards to throwing out some of the stuff in the end.
> 
> Important notes:
> 
> * This is an early (WIP) RFC that does not add any function to the
>   kernel at his stage, as the ultravisor interactions are left out.
>   The purpose is getting some early feedback ASAP.
> 
> * In future these patches will depend on some code interacting with the
>   ultravisor (WIP by Vasily and Janosch).
> 
> * The s390 names are by no means final, and are not properly explained. Should
>   not hamper understanding too much. If it does please ask.
> 
> * The existing naming in the common infrastructure (kernel internal
>   interfaces) is pretty much based on the AMD SEV terminology. Thus the
>   names aren't always perfect. There might be merit to changing these
>   names to more abstract ones. I did not put much thought into that at
>   the current stage.
> 
> 
> Testing:
> 
> Please use iommu_platform=on for any virtio devices you are going
> to test this code with (so virtio actually uses the DMA API).
> 
> Looking forward to your review or any other type of input.
> 
> Halil Pasic (12):
>   virtio/s390: use vring_create_virtqueue
>   virtio/s390: DMA support for virtio-ccw
>   s390/mm: force swiotlb for protected virtualization
>   s390/cio: introduce cio DMA pool
>   s390/cio: add protected virtualization support to cio
>   s390/airq: use DMA memory for adapter interrupts
>   virtio/s390: use DMA memory for ccw I/O
>   virtio/s390: add indirection to indicators access
>   virtio/s390: use DMA memory for notifiers
>   virtio/s390: consolidate DMA allocations
>   virtio/s390: use the cio DMA pool
>   virtio/s390:  make airq summary indicators DMA
> 
>  arch/s390/Kconfig                   |   5 +
>  arch/s390/include/asm/Kbuild        |   1 -
>  arch/s390/include/asm/airq.h        |   2 +
>  arch/s390/include/asm/cio.h         |   4 +
>  arch/s390/include/asm/dma-mapping.h |  13 ++
>  arch/s390/include/asm/mem_encrypt.h |  18 +++
>  arch/s390/mm/init.c                 |  44 +++++
>  drivers/s390/cio/airq.c             |  18 ++-
>  drivers/s390/cio/ccwreq.c           |   8 +-
>  drivers/s390/cio/css.c              |  63 ++++++++
>  drivers/s390/cio/device.c           |  46 ++++--
>  drivers/s390/cio/device_fsm.c       |  40 ++---
>  drivers/s390/cio/device_id.c        |  18 +--
>  drivers/s390/cio/device_ops.c       |   4 +-
>  drivers/s390/cio/device_pgid.c      |  20 +--
>  drivers/s390/cio/device_status.c    |  24 +--
>  drivers/s390/cio/io_sch.h           |  19 ++-
>  drivers/s390/virtio/virtio_ccw.c    | 310 ++++++++++++++++++++----------------
>  18 files changed, 444 insertions(+), 213 deletions(-)
>  create mode 100644 arch/s390/include/asm/dma-mapping.h
>  create mode 100644 arch/s390/include/asm/mem_encrypt.h
>
Halil Pasic April 16, 2019, 11:10 a.m. UTC | #5
On Fri, 12 Apr 2019 15:47:50 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 05.04.19 01:16, Halil Pasic wrote:
> > Enhanced virtualization protection technology may require the use of
> > bounce buffers for I/O. While support for this was built into the virtio
> > core,  virtio-ccw wasn't changed accordingly.
> 
> Can you elaborate some more about the general approach (Enhanced
> virtualization protection technology, ultravisor, concept, issues, how
> to squeeze it into QEMU/KVM/kernel) etc =
> 
> For my taste, this cover letter misses some important context :)
> 

I'm aware. Unfortunately we don't have a decision yet about which parts
of the protected virtualization architecture are going to be PoP
material.

You can get some more context immediately by having a look at Martin's
features branch and looking at the s390/protvirt  and s390/uv patches.

I will try to provide more background information for v1. But having
a remotely complete and reliable documentation will take some time. What
I can offer at the moment is answers to specific questions.

Regards,
Halil
David Hildenbrand April 16, 2019, 11:50 a.m. UTC | #6
On 16.04.19 13:10, Halil Pasic wrote:
> On Fri, 12 Apr 2019 15:47:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 05.04.19 01:16, Halil Pasic wrote:
>>> Enhanced virtualization protection technology may require the use of
>>> bounce buffers for I/O. While support for this was built into the virtio
>>> core,  virtio-ccw wasn't changed accordingly.
>>
>> Can you elaborate some more about the general approach (Enhanced
>> virtualization protection technology, ultravisor, concept, issues, how
>> to squeeze it into QEMU/KVM/kernel) etc =
>>
>> For my taste, this cover letter misses some important context :)
>>
> 
> I'm aware. Unfortunately we don't have a decision yet about which parts
> of the protected virtualization architecture are going to be PoP
> material.

Oh, okay.

> 
> You can get some more context immediately by having a look at Martin's
> features branch and looking at the s390/protvirt  and s390/uv patches.

As I don't have time to dig through random branches to
discover/reverse-engineer the obvious, I won't be reviewing this patch
series. But as I am not an I/O expert, this might not be bad at all :)

Maybe other people can help.

> 
> I will try to provide more background information for v1. But having
> a remotely complete and reliable documentation will take some time. What
> I can offer at the moment is answers to specific questions.

Waiting for v1 then.

Cheers!