| Message ID | 20220114203145.242984-7-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: enable zPCI for interpretive execution | expand |
On Fri, 14 Jan 2022 15:31:21 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > When doing device passthrough where interrupts are being forwarded > from host to guest, we wish to use a pinned section of guest memory > as the vector (the same memory used by the guest as the vector). maybe expand the description of the patch to explain what exactly is being done in this patch. Namely: you add a parameter to a function (and some logic in the function to use the new parameter), but the function is not being used yet. And pinning is also done somewhere else. maybe you can add something like This patch adds a new parameter for airq_iv_create to pass the existing vector pinned in guest memory and to use it when needed instead of allocating a new one. Apart from that, the patch looks good. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > arch/s390/include/asm/airq.h | 4 +++- > arch/s390/pci/pci_irq.c | 8 ++++---- > drivers/s390/cio/airq.c | 10 +++++++--- > drivers/s390/virtio/virtio_ccw.c | 2 +- > 4 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/arch/s390/include/asm/airq.h b/arch/s390/include/asm/airq.h > index 7918a7d09028..e82e5626e139 100644 > --- a/arch/s390/include/asm/airq.h > +++ b/arch/s390/include/asm/airq.h > @@ -47,8 +47,10 @@ struct airq_iv { > #define AIRQ_IV_PTR 4 /* Allocate the ptr array */ > #define AIRQ_IV_DATA 8 /* Allocate the data array */ > #define AIRQ_IV_CACHELINE 16 /* Cacheline alignment for the vector */ > +#define AIRQ_IV_GUESTVEC 32 /* Vector is a pinned guest page */ > > -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags); > +struct airq_iv *airq_iv_create(unsigned long bits, unsigned long > flags, > + unsigned long *vec); > void airq_iv_release(struct airq_iv *iv); > unsigned long airq_iv_alloc(struct airq_iv *iv, unsigned long num); > void airq_iv_free(struct airq_iv *iv, unsigned long bit, unsigned > long num); diff --git a/arch/s390/pci/pci_irq.c > b/arch/s390/pci/pci_irq.c index cc4c8d7c8f5c..0d0a02a9fbbf 100644 > --- a/arch/s390/pci/pci_irq.c > +++ b/arch/s390/pci/pci_irq.c > @@ -296,7 +296,7 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int > nvec, int type) zdev->aisb = bit; > > /* Create adapter interrupt vector */ > - zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | > AIRQ_IV_BITLOCK); > + zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | > AIRQ_IV_BITLOCK, NULL); if (!zdev->aibv) > return -ENOMEM; > > @@ -419,7 +419,7 @@ static int __init zpci_directed_irq_init(void) > union zpci_sic_iib iib = {{0}}; > unsigned int cpu; > > - zpci_sbv = airq_iv_create(num_possible_cpus(), 0); > + zpci_sbv = airq_iv_create(num_possible_cpus(), 0, NULL); > if (!zpci_sbv) > return -ENOMEM; > > @@ -441,7 +441,7 @@ static int __init zpci_directed_irq_init(void) > zpci_ibv[cpu] = airq_iv_create(cache_line_size() * > BITS_PER_BYTE, AIRQ_IV_DATA | > AIRQ_IV_CACHELINE | > - (!cpu ? AIRQ_IV_ALLOC > : 0)); > + (!cpu ? AIRQ_IV_ALLOC > : 0), NULL); if (!zpci_ibv[cpu]) > return -ENOMEM; > } > @@ -458,7 +458,7 @@ static int __init zpci_floating_irq_init(void) > if (!zpci_ibv) > return -ENOMEM; > > - zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC); > + zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC, > NULL); if (!zpci_sbv) > goto out_free; > > diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c > index 2f2226786319..375a58b1c838 100644 > --- a/drivers/s390/cio/airq.c > +++ b/drivers/s390/cio/airq.c > @@ -122,10 +122,12 @@ static inline unsigned long iv_size(unsigned > long bits) > * airq_iv_create - create an interrupt vector > * @bits: number of bits in the interrupt vector > * @flags: allocation flags > + * @vec: pointer to pinned guest memory if AIRQ_IV_GUESTVEC > * > * Returns a pointer to an interrupt vector structure > */ > -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long > flags) +struct airq_iv *airq_iv_create(unsigned long bits, unsigned > long flags, > + unsigned long *vec) > { > struct airq_iv *iv; > unsigned long size; > @@ -146,6 +148,8 @@ struct airq_iv *airq_iv_create(unsigned long > bits, unsigned long flags) &iv->vector_dma); > if (!iv->vector) > goto out_free; > + } else if (flags & AIRQ_IV_GUESTVEC) { > + iv->vector = vec; > } else { > iv->vector = cio_dma_zalloc(size); > if (!iv->vector) > @@ -185,7 +189,7 @@ struct airq_iv *airq_iv_create(unsigned long > bits, unsigned long flags) kfree(iv->avail); > if (iv->flags & AIRQ_IV_CACHELINE && iv->vector) > dma_pool_free(airq_iv_cache, iv->vector, > iv->vector_dma); > - else > + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) > cio_dma_free(iv->vector, size); > kfree(iv); > out: > @@ -204,7 +208,7 @@ void airq_iv_release(struct airq_iv *iv) > kfree(iv->bitlock); > if (iv->flags & AIRQ_IV_CACHELINE) > dma_pool_free(airq_iv_cache, iv->vector, > iv->vector_dma); > - else > + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) > cio_dma_free(iv->vector, iv_size(iv->bits)); > kfree(iv->avail); > kfree(iv); > diff --git a/drivers/s390/virtio/virtio_ccw.c > b/drivers/s390/virtio/virtio_ccw.c index 52c376d15978..410498d693f8 > 100644 --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -241,7 +241,7 @@ static struct airq_info *new_airq_info(int index) > return NULL; > rwlock_init(&info->lock); > info->aiv = airq_iv_create(VIRTIO_IV_BITS, AIRQ_IV_ALLOC | > AIRQ_IV_PTR > - | AIRQ_IV_CACHELINE); > + | AIRQ_IV_CACHELINE, NULL); > if (!info->aiv) { > kfree(info); > return NULL;
On 1/14/22 21:31, Matthew Rosato wrote: > When doing device passthrough where interrupts are being forwarded > from host to guest, we wish to use a pinned section of guest memory > as the vector (the same memory used by the guest as the vector). > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > --- > arch/s390/include/asm/airq.h | 4 +++- > arch/s390/pci/pci_irq.c | 8 ++++---- > drivers/s390/cio/airq.c | 10 +++++++--- > drivers/s390/virtio/virtio_ccw.c | 2 +- > 4 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/arch/s390/include/asm/airq.h b/arch/s390/include/asm/airq.h > index 7918a7d09028..e82e5626e139 100644 > --- a/arch/s390/include/asm/airq.h > +++ b/arch/s390/include/asm/airq.h > @@ -47,8 +47,10 @@ struct airq_iv { > #define AIRQ_IV_PTR 4 /* Allocate the ptr array */ > #define AIRQ_IV_DATA 8 /* Allocate the data array */ > #define AIRQ_IV_CACHELINE 16 /* Cacheline alignment for the vector */ > +#define AIRQ_IV_GUESTVEC 32 /* Vector is a pinned guest page */ > > -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags); > +struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags, > + unsigned long *vec); > void airq_iv_release(struct airq_iv *iv); > unsigned long airq_iv_alloc(struct airq_iv *iv, unsigned long num); > void airq_iv_free(struct airq_iv *iv, unsigned long bit, unsigned long num); > diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c > index cc4c8d7c8f5c..0d0a02a9fbbf 100644 > --- a/arch/s390/pci/pci_irq.c > +++ b/arch/s390/pci/pci_irq.c > @@ -296,7 +296,7 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type) > zdev->aisb = bit; > > /* Create adapter interrupt vector */ > - zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | AIRQ_IV_BITLOCK); > + zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | AIRQ_IV_BITLOCK, NULL); > if (!zdev->aibv) > return -ENOMEM; > > @@ -419,7 +419,7 @@ static int __init zpci_directed_irq_init(void) > union zpci_sic_iib iib = {{0}}; > unsigned int cpu; > > - zpci_sbv = airq_iv_create(num_possible_cpus(), 0); > + zpci_sbv = airq_iv_create(num_possible_cpus(), 0, NULL); > if (!zpci_sbv) > return -ENOMEM; > > @@ -441,7 +441,7 @@ static int __init zpci_directed_irq_init(void) > zpci_ibv[cpu] = airq_iv_create(cache_line_size() * BITS_PER_BYTE, > AIRQ_IV_DATA | > AIRQ_IV_CACHELINE | > - (!cpu ? AIRQ_IV_ALLOC : 0)); > + (!cpu ? AIRQ_IV_ALLOC : 0), NULL); > if (!zpci_ibv[cpu]) > return -ENOMEM; > } > @@ -458,7 +458,7 @@ static int __init zpci_floating_irq_init(void) > if (!zpci_ibv) > return -ENOMEM; > > - zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC); > + zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC, NULL); > if (!zpci_sbv) > goto out_free; > > diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c > index 2f2226786319..375a58b1c838 100644 > --- a/drivers/s390/cio/airq.c > +++ b/drivers/s390/cio/airq.c > @@ -122,10 +122,12 @@ static inline unsigned long iv_size(unsigned long bits) > * airq_iv_create - create an interrupt vector > * @bits: number of bits in the interrupt vector > * @flags: allocation flags > + * @vec: pointer to pinned guest memory if AIRQ_IV_GUESTVEC > * > * Returns a pointer to an interrupt vector structure > */ > -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) > +struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags, > + unsigned long *vec) > { > struct airq_iv *iv; > unsigned long size; > @@ -146,6 +148,8 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) > &iv->vector_dma); > if (!iv->vector) > goto out_free; > + } else if (flags & AIRQ_IV_GUESTVEC) { > + iv->vector = vec; > } else { > iv->vector = cio_dma_zalloc(size); > if (!iv->vector) > @@ -185,7 +189,7 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) > kfree(iv->avail); > if (iv->flags & AIRQ_IV_CACHELINE && iv->vector) > dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma); > - else > + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) > cio_dma_free(iv->vector, size); > kfree(iv); > out: > @@ -204,7 +208,7 @@ void airq_iv_release(struct airq_iv *iv) > kfree(iv->bitlock); > if (iv->flags & AIRQ_IV_CACHELINE) > dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma); > - else > + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) > cio_dma_free(iv->vector, iv_size(iv->bits)); > kfree(iv->avail); > kfree(iv); > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index 52c376d15978..410498d693f8 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -241,7 +241,7 @@ static struct airq_info *new_airq_info(int index) > return NULL; > rwlock_init(&info->lock); > info->aiv = airq_iv_create(VIRTIO_IV_BITS, AIRQ_IV_ALLOC | AIRQ_IV_PTR > - | AIRQ_IV_CACHELINE); > + | AIRQ_IV_CACHELINE, NULL); > if (!info->aiv) { > kfree(info); > return NULL; >
On 1/17/22 7:29 AM, Claudio Imbrenda wrote: > On Fri, 14 Jan 2022 15:31:21 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> When doing device passthrough where interrupts are being forwarded >> from host to guest, we wish to use a pinned section of guest memory >> as the vector (the same memory used by the guest as the vector). > > maybe expand the description of the patch to explain what exactly is > being done in this patch. Namely: you add a parameter to a function > (and some logic in the function to use the new parameter), but the > function is not being used yet. And pinning is also done somewhere else. > > maybe you can add something like > > This patch adds a new parameter for airq_iv_create to pass the > existing vector pinned in guest memory and to use it when > needed instead of allocating a new one. > > Apart from that, the patch looks good. > Thanks, will re-work to: When doing device passthrough where interrupts are being forwarded from host to guest, we wish to use a pinned section of guest memory as the vector (the same memory used by the guest as the vector). To accomplish this, add a new parameter for airq_iv_create which allows passing an existing vector to be used instead of allocating a new one. The caller is responsible for ensuring the vector is pinned in memory as well as for unpinning the memory when the vector is no longer needed. A subsequent patch will use this new parameter for zPCI interpretation.
diff --git a/arch/s390/include/asm/airq.h b/arch/s390/include/asm/airq.h index 7918a7d09028..e82e5626e139 100644 --- a/arch/s390/include/asm/airq.h +++ b/arch/s390/include/asm/airq.h @@ -47,8 +47,10 @@ struct airq_iv { #define AIRQ_IV_PTR 4 /* Allocate the ptr array */ #define AIRQ_IV_DATA 8 /* Allocate the data array */ #define AIRQ_IV_CACHELINE 16 /* Cacheline alignment for the vector */ +#define AIRQ_IV_GUESTVEC 32 /* Vector is a pinned guest page */ -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags); +struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags, + unsigned long *vec); void airq_iv_release(struct airq_iv *iv); unsigned long airq_iv_alloc(struct airq_iv *iv, unsigned long num); void airq_iv_free(struct airq_iv *iv, unsigned long bit, unsigned long num); diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c index cc4c8d7c8f5c..0d0a02a9fbbf 100644 --- a/arch/s390/pci/pci_irq.c +++ b/arch/s390/pci/pci_irq.c @@ -296,7 +296,7 @@ int arch_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type) zdev->aisb = bit; /* Create adapter interrupt vector */ - zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | AIRQ_IV_BITLOCK); + zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | AIRQ_IV_BITLOCK, NULL); if (!zdev->aibv) return -ENOMEM; @@ -419,7 +419,7 @@ static int __init zpci_directed_irq_init(void) union zpci_sic_iib iib = {{0}}; unsigned int cpu; - zpci_sbv = airq_iv_create(num_possible_cpus(), 0); + zpci_sbv = airq_iv_create(num_possible_cpus(), 0, NULL); if (!zpci_sbv) return -ENOMEM; @@ -441,7 +441,7 @@ static int __init zpci_directed_irq_init(void) zpci_ibv[cpu] = airq_iv_create(cache_line_size() * BITS_PER_BYTE, AIRQ_IV_DATA | AIRQ_IV_CACHELINE | - (!cpu ? AIRQ_IV_ALLOC : 0)); + (!cpu ? AIRQ_IV_ALLOC : 0), NULL); if (!zpci_ibv[cpu]) return -ENOMEM; } @@ -458,7 +458,7 @@ static int __init zpci_floating_irq_init(void) if (!zpci_ibv) return -ENOMEM; - zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC); + zpci_sbv = airq_iv_create(ZPCI_NR_DEVICES, AIRQ_IV_ALLOC, NULL); if (!zpci_sbv) goto out_free; diff --git a/drivers/s390/cio/airq.c b/drivers/s390/cio/airq.c index 2f2226786319..375a58b1c838 100644 --- a/drivers/s390/cio/airq.c +++ b/drivers/s390/cio/airq.c @@ -122,10 +122,12 @@ static inline unsigned long iv_size(unsigned long bits) * airq_iv_create - create an interrupt vector * @bits: number of bits in the interrupt vector * @flags: allocation flags + * @vec: pointer to pinned guest memory if AIRQ_IV_GUESTVEC * * Returns a pointer to an interrupt vector structure */ -struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) +struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags, + unsigned long *vec) { struct airq_iv *iv; unsigned long size; @@ -146,6 +148,8 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) &iv->vector_dma); if (!iv->vector) goto out_free; + } else if (flags & AIRQ_IV_GUESTVEC) { + iv->vector = vec; } else { iv->vector = cio_dma_zalloc(size); if (!iv->vector) @@ -185,7 +189,7 @@ struct airq_iv *airq_iv_create(unsigned long bits, unsigned long flags) kfree(iv->avail); if (iv->flags & AIRQ_IV_CACHELINE && iv->vector) dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma); - else + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) cio_dma_free(iv->vector, size); kfree(iv); out: @@ -204,7 +208,7 @@ void airq_iv_release(struct airq_iv *iv) kfree(iv->bitlock); if (iv->flags & AIRQ_IV_CACHELINE) dma_pool_free(airq_iv_cache, iv->vector, iv->vector_dma); - else + else if (!(iv->flags & AIRQ_IV_GUESTVEC)) cio_dma_free(iv->vector, iv_size(iv->bits)); kfree(iv->avail); kfree(iv); diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index 52c376d15978..410498d693f8 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c @@ -241,7 +241,7 @@ static struct airq_info *new_airq_info(int index) return NULL; rwlock_init(&info->lock); info->aiv = airq_iv_create(VIRTIO_IV_BITS, AIRQ_IV_ALLOC | AIRQ_IV_PTR - | AIRQ_IV_CACHELINE); + | AIRQ_IV_CACHELINE, NULL); if (!info->aiv) { kfree(info); return NULL;
When doing device passthrough where interrupts are being forwarded from host to guest, we wish to use a pinned section of guest memory as the vector (the same memory used by the guest as the vector). Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- arch/s390/include/asm/airq.h | 4 +++- arch/s390/pci/pci_irq.c | 8 ++++---- drivers/s390/cio/airq.c | 10 +++++++--- drivers/s390/virtio/virtio_ccw.c | 2 +- 4 files changed, 15 insertions(+), 9 deletions(-)