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

Harald Freudenberger
2022-06-20T10:01:11Z

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 <nicolinc@nvidia.com>
> ---
> 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 <freude@linux.ibm.com>