2022-06-16 23:57:19

by Nicolin Chen

[permalink] [raw]
Subject: [RFT][PATCH v1 1/6] vfio/ap: Pass in physical address of ind to ap_aqic()

The ap_aqic() is called by vfio_ap_irq_enable() where it passes in a
virt value that's casted from a physical address "h_nib". Inside the
ap_aqic(), it does virt_to_phys() again.

Since ap_aqic() needs a physical address, let's just pass in a pa of
ind directly. So change the "ind" to "pa_ind".

Signed-off-by: Nicolin Chen <[email protected]>
---
arch/s390/include/asm/ap.h | 6 +++---
drivers/s390/crypto/ap_queue.c | 2 +-
drivers/s390/crypto/vfio_ap_ops.c | 7 ++++---
3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
index b515cfa62bd9..f508f5025e38 100644
--- a/arch/s390/include/asm/ap.h
+++ b/arch/s390/include/asm/ap.h
@@ -227,13 +227,13 @@ struct ap_qirq_ctrl {
* ap_aqic(): Control interruption for a specific AP.
* @qid: The AP queue number
* @qirqctrl: struct ap_qirq_ctrl (64 bit value)
- * @ind: The notification indicator byte
+ * @pa_ind: Physical address of the notification indicator byte
*
* Returns AP queue status.
*/
static inline struct ap_queue_status ap_aqic(ap_qid_t qid,
struct ap_qirq_ctrl qirqctrl,
- void *ind)
+ phys_addr_t pa_ind)
{
unsigned long reg0 = qid | (3UL << 24); /* fc 3UL is AQIC */
union {
@@ -241,7 +241,7 @@ static inline struct ap_queue_status ap_aqic(ap_qid_t qid,
struct ap_qirq_ctrl qirqctrl;
struct ap_queue_status status;
} reg1;
- unsigned long reg2 = virt_to_phys(ind);
+ unsigned long reg2 = pa_ind;

reg1.qirqctrl = qirqctrl;

diff --git a/drivers/s390/crypto/ap_queue.c b/drivers/s390/crypto/ap_queue.c
index c48b0db824e3..a32457b4cbb8 100644
--- a/drivers/s390/crypto/ap_queue.c
+++ b/drivers/s390/crypto/ap_queue.c
@@ -34,7 +34,7 @@ static int ap_queue_enable_irq(struct ap_queue *aq, void *ind)

qirqctrl.ir = 1;
qirqctrl.isc = AP_ISC;
- status = ap_aqic(aq->qid, qirqctrl, ind);
+ status = ap_aqic(aq->qid, qirqctrl, virt_to_phys(ind));
switch (status.response_code) {
case AP_RESPONSE_NORMAL:
case AP_RESPONSE_OTHERWISE_CHANGED:
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a7d2a95796d3..bb869b28cebd 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -154,7 +154,7 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
int retries = 5;

do {
- status = ap_aqic(q->apqn, aqic_gisa, NULL);
+ status = ap_aqic(q->apqn, aqic_gisa, 0);
switch (status.response_code) {
case AP_RESPONSE_OTHERWISE_CHANGED:
case AP_RESPONSE_NORMAL:
@@ -245,7 +245,8 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
struct kvm_s390_gisa *gisa;
int nisc;
struct kvm *kvm;
- unsigned long h_nib, g_pfn, h_pfn;
+ unsigned long g_pfn, h_pfn;
+ phys_addr_t h_nib;
int ret;

/* Verify that the notification indicator byte address is valid */
@@ -290,7 +291,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
aqic_gisa.ir = 1;
aqic_gisa.gisa = (uint64_t)gisa >> 4;

- status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
+ status = ap_aqic(q->apqn, aqic_gisa, h_nib);
switch (status.response_code) {
case AP_RESPONSE_NORMAL:
/* See if we did clear older IRQ configuration */
--
2.17.1


2022-06-20 10:19:39

by Harald Freudenberger

[permalink] [raw]
Subject: Re: [RFT][PATCH v1 1/6] vfio/ap: Pass in physical address of ind to ap_aqic()

On 2022-06-17 01:52, Nicolin Chen wrote:
> The ap_aqic() is called by vfio_ap_irq_enable() where it passes in a
> virt value that's casted from a physical address "h_nib". Inside the
> ap_aqic(), it does virt_to_phys() again.
>
> Since ap_aqic() needs a physical address, let's just pass in a pa of
> ind directly. So change the "ind" to "pa_ind".
>
> Signed-off-by: Nicolin Chen <[email protected]>
> ---
> arch/s390/include/asm/ap.h | 6 +++---
> drivers/s390/crypto/ap_queue.c | 2 +-
> drivers/s390/crypto/vfio_ap_ops.c | 7 ++++---
> 3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/s390/include/asm/ap.h b/arch/s390/include/asm/ap.h
> index b515cfa62bd9..f508f5025e38 100644
> --- a/arch/s390/include/asm/ap.h
> +++ b/arch/s390/include/asm/ap.h
> @@ -227,13 +227,13 @@ struct ap_qirq_ctrl {
> * ap_aqic(): Control interruption for a specific AP.
> * @qid: The AP queue number
> * @qirqctrl: struct ap_qirq_ctrl (64 bit value)
> - * @ind: The notification indicator byte
> + * @pa_ind: Physical address of the notification indicator byte
> *
> * Returns AP queue status.
> */
> static inline struct ap_queue_status ap_aqic(ap_qid_t qid,
> struct ap_qirq_ctrl qirqctrl,
> - void *ind)
> + phys_addr_t pa_ind)
> {
> unsigned long reg0 = qid | (3UL << 24); /* fc 3UL is AQIC */
> union {
> @@ -241,7 +241,7 @@ static inline struct ap_queue_status
> ap_aqic(ap_qid_t qid,
> struct ap_qirq_ctrl qirqctrl;
> struct ap_queue_status status;
> } reg1;
> - unsigned long reg2 = virt_to_phys(ind);
> + unsigned long reg2 = pa_ind;
>
> reg1.qirqctrl = qirqctrl;
>
> diff --git a/drivers/s390/crypto/ap_queue.c
> b/drivers/s390/crypto/ap_queue.c
> index c48b0db824e3..a32457b4cbb8 100644
> --- a/drivers/s390/crypto/ap_queue.c
> +++ b/drivers/s390/crypto/ap_queue.c
> @@ -34,7 +34,7 @@ static int ap_queue_enable_irq(struct ap_queue *aq,
> void *ind)
>
> qirqctrl.ir = 1;
> qirqctrl.isc = AP_ISC;
> - status = ap_aqic(aq->qid, qirqctrl, ind);
> + status = ap_aqic(aq->qid, qirqctrl, virt_to_phys(ind));
> switch (status.response_code) {
> case AP_RESPONSE_NORMAL:
> case AP_RESPONSE_OTHERWISE_CHANGED:
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c
> b/drivers/s390/crypto/vfio_ap_ops.c
> index a7d2a95796d3..bb869b28cebd 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -154,7 +154,7 @@ static struct ap_queue_status
> vfio_ap_irq_disable(struct vfio_ap_queue *q)
> int retries = 5;
>
> do {
> - status = ap_aqic(q->apqn, aqic_gisa, NULL);
> + status = ap_aqic(q->apqn, aqic_gisa, 0);
> switch (status.response_code) {
> case AP_RESPONSE_OTHERWISE_CHANGED:
> case AP_RESPONSE_NORMAL:
> @@ -245,7 +245,8 @@ static struct ap_queue_status
> vfio_ap_irq_enable(struct vfio_ap_queue *q,
> struct kvm_s390_gisa *gisa;
> int nisc;
> struct kvm *kvm;
> - unsigned long h_nib, g_pfn, h_pfn;
> + unsigned long g_pfn, h_pfn;
> + phys_addr_t h_nib;
> int ret;
>
> /* Verify that the notification indicator byte address is valid */
> @@ -290,7 +291,7 @@ static struct ap_queue_status
> vfio_ap_irq_enable(struct vfio_ap_queue *q,
> aqic_gisa.ir = 1;
> aqic_gisa.gisa = (uint64_t)gisa >> 4;
>
> - status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
> + status = ap_aqic(q->apqn, aqic_gisa, h_nib);
> switch (status.response_code) {
> case AP_RESPONSE_NORMAL:
> /* See if we did clear older IRQ configuration */
Add my Reviewed-By: Harald Freudenberger <[email protected]>

2022-06-21 21:28:37

by Nicolin Chen

[permalink] [raw]
Subject: Re: [RFT][PATCH v1 1/6] vfio/ap: Pass in physical address of ind to ap_aqic()

On Mon, Jun 20, 2022 at 12:00:53PM +0200, Harald Freudenberger wrote:
> External email: Use caution opening links or attachments
>
>
> On 2022-06-17 01:52, Nicolin Chen wrote:
> > The ap_aqic() is called by vfio_ap_irq_enable() where it passes in a
> > virt value that's casted from a physical address "h_nib". Inside the
> > ap_aqic(), it does virt_to_phys() again.
> >
> > Since ap_aqic() needs a physical address, let's just pass in a pa of
> > ind directly. So change the "ind" to "pa_ind".
> >
> > Signed-off-by: Nicolin Chen <[email protected]>

> Add my Reviewed-By: Harald Freudenberger <[email protected]>

Will do. Thanks!