2022-01-14 23:06:07

by Matthew Rosato

[permalink] [raw]
Subject: [PATCH v2 08/30] s390/pci: stash associated GISA designation

For passthrough devices, we will need to know the GISA designation of the
guest if interpretation facilities are to be used. Setup to stash this in
the zdev and set a default of 0 (no GISA designation) for now; a subsequent
patch will set a valid GISA designation for passthrough devices.
Also, extend mpcific routines to specify this stashed designation as part
of the mpcific command.

Reviewed-by: Niklas Schnelle <[email protected]>
Reviewed-by: Christian Borntraeger <[email protected]>
Reviewed-by: Eric Farman <[email protected]>
Reviewed-by: Pierre Morel <[email protected]>
Signed-off-by: Matthew Rosato <[email protected]>
---
arch/s390/include/asm/pci.h | 1 +
arch/s390/include/asm/pci_clp.h | 3 ++-
arch/s390/pci/pci.c | 6 ++++++
arch/s390/pci/pci_clp.c | 1 +
arch/s390/pci/pci_irq.c | 5 +++++
5 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 90824be5ce9a..2474b8d30f2a 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -123,6 +123,7 @@ struct zpci_dev {
enum zpci_state state;
u32 fid; /* function ID, used by sclp */
u32 fh; /* function handle, used by insn's */
+ u32 gd; /* GISA designation for passthrough */
u16 vfn; /* virtual function number */
u16 pchid; /* physical channel ID */
u8 pfgid; /* function group ID */
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 1f4b666e85ee..3af8d196da74 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -173,7 +173,8 @@ struct clp_req_set_pci {
u16 reserved2;
u8 oc; /* operation controls */
u8 ndas; /* number of dma spaces */
- u64 reserved3;
+ u32 reserved3;
+ u32 gd; /* GISA designation */
} __packed;

/* Set PCI function response */
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index 792f8e0f2178..0c9879dae752 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -119,6 +119,7 @@ int zpci_register_ioat(struct zpci_dev *zdev, u8 dmaas,
fib.pba = base;
fib.pal = limit;
fib.iota = iota | ZPCI_IOTA_RTTO_FLAG;
+ fib.gd = zdev->gd;
cc = zpci_mod_fc(req, &fib, &status);
if (cc)
zpci_dbg(3, "reg ioat fid:%x, cc:%d, status:%d\n", zdev->fid, cc, status);
@@ -132,6 +133,8 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
struct zpci_fib fib = {0};
u8 cc, status;

+ fib.gd = zdev->gd;
+
cc = zpci_mod_fc(req, &fib, &status);
if (cc)
zpci_dbg(3, "unreg ioat fid:%x, cc:%d, status:%d\n", zdev->fid, cc, status);
@@ -159,6 +162,7 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
atomic64_set(&zdev->unmapped_pages, 0);

fib.fmb_addr = virt_to_phys(zdev->fmb);
+ fib.gd = zdev->gd;
cc = zpci_mod_fc(req, &fib, &status);
if (cc) {
kmem_cache_free(zdev_fmb_cache, zdev->fmb);
@@ -177,6 +181,8 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
if (!zdev->fmb)
return -EINVAL;

+ fib.gd = zdev->gd;
+
/* Function measurement is disabled if fmb address is zero */
cc = zpci_mod_fc(req, &fib, &status);
if (cc == 3) /* Function already gone. */
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index be077b39da33..e9ed0e4a5cf0 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -240,6 +240,7 @@ static int clp_set_pci_fn(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as, u8 comma
rrb->request.fh = zdev->fh;
rrb->request.oc = command;
rrb->request.ndas = nr_dma_as;
+ rrb->request.gd = zdev->gd;

rc = clp_req(rrb, CLP_LPS_PCI);
if (rrb->response.hdr.rsp == CLP_RC_SETPCIFN_BUSY) {
diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
index 2f675355fd0c..17e5adfe1273 100644
--- a/arch/s390/pci/pci_irq.c
+++ b/arch/s390/pci/pci_irq.c
@@ -43,6 +43,7 @@ static int zpci_set_airq(struct zpci_dev *zdev)
fib.fmt0.aibvo = 0; /* each zdev has its own interrupt vector */
fib.fmt0.aisb = virt_to_phys(zpci_sbv->vector) + (zdev->aisb / 64) * 8;
fib.fmt0.aisbo = zdev->aisb & 63;
+ fib.gd = zdev->gd;

return zpci_mod_fc(req, &fib, &status) ? -EIO : 0;
}
@@ -54,6 +55,8 @@ static int zpci_clear_airq(struct zpci_dev *zdev)
struct zpci_fib fib = {0};
u8 cc, status;

+ fib.gd = zdev->gd;
+
cc = zpci_mod_fc(req, &fib, &status);
if (cc == 3 || (cc == 1 && status == 24))
/* Function already gone or IRQs already deregistered. */
@@ -72,6 +75,7 @@ static int zpci_set_directed_irq(struct zpci_dev *zdev)
fib.fmt = 1;
fib.fmt1.noi = zdev->msi_nr_irqs;
fib.fmt1.dibvo = zdev->msi_first_bit;
+ fib.gd = zdev->gd;

return zpci_mod_fc(req, &fib, &status) ? -EIO : 0;
}
@@ -84,6 +88,7 @@ static int zpci_clear_directed_irq(struct zpci_dev *zdev)
u8 cc, status;

fib.fmt = 1;
+ fib.gd = zdev->gd;
cc = zpci_mod_fc(req, &fib, &status);
if (cc == 3 || (cc == 1 && status == 24))
/* Function already gone or IRQs already deregistered. */
--
2.27.0


2022-01-24 19:22:24

by Pierre Morel

[permalink] [raw]
Subject: Re: [PATCH v2 08/30] s390/pci: stash associated GISA designation



On 1/14/22 21:31, Matthew Rosato wrote:
> For passthrough devices, we will need to know the GISA designation of the
> guest if interpretation facilities are to be used. Setup to stash this in
> the zdev and set a default of 0 (no GISA designation) for now; a subsequent
> patch will set a valid GISA designation for passthrough devices.
> Also, extend mpcific routines to specify this stashed designation as part
> of the mpcific command.
>
> Reviewed-by: Niklas Schnelle <[email protected]>
> Reviewed-by: Christian Borntraeger <[email protected]>
> Reviewed-by: Eric Farman <[email protected]>
> Reviewed-by: Pierre Morel <[email protected]>
> Signed-off-by: Matthew Rosato <[email protected]>
> ---
> arch/s390/include/asm/pci.h | 1 +
> arch/s390/include/asm/pci_clp.h | 3 ++-
> arch/s390/pci/pci.c | 6 ++++++
> arch/s390/pci/pci_clp.c | 1 +
> arch/s390/pci/pci_irq.c | 5 +++++
> 5 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 90824be5ce9a..2474b8d30f2a 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -123,6 +123,7 @@ struct zpci_dev {
> enum zpci_state state;
> u32 fid; /* function ID, used by sclp */
> u32 fh; /* function handle, used by insn's */
> + u32 gd; /* GISA designation for passthrough */

I already gave my R-B, and do not want to remove it, but wouldn't it be
possible to use more explicit names like gisa_designation instead of
just gd.
It would not change anything to the functionality but would facilitate
the maintenance?

> u16 vfn; /* virtual function number */
> u16 pchid; /* physical channel ID */
> u8 pfgid; /* function group ID */
> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> index 1f4b666e85ee..3af8d196da74 100644
> --- a/arch/s390/include/asm/pci_clp.h
> +++ b/arch/s390/include/asm/pci_clp.h
> @@ -173,7 +173,8 @@ struct clp_req_set_pci {
> u16 reserved2;
> u8 oc; /* operation controls */
> u8 ndas; /* number of dma spaces */
> - u64 reserved3;
> + u32 reserved3;
> + u32 gd; /* GISA designation */

here too.


> } __packed;
>
> /* Set PCI function response */
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index 792f8e0f2178..0c9879dae752 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -119,6 +119,7 @@ int zpci_register_ioat(struct zpci_dev *zdev, u8 dmaas,
> fib.pba = base;
> fib.pal = limit;
> fib.iota = iota | ZPCI_IOTA_RTTO_FLAG;
> + fib.gd = zdev->gd;
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc)
> zpci_dbg(3, "reg ioat fid:%x, cc:%d, status:%d\n", zdev->fid, cc, status);
> @@ -132,6 +133,8 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
> struct zpci_fib fib = {0};
> u8 cc, status;
>
> + fib.gd = zdev->gd;
> +
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc)
> zpci_dbg(3, "unreg ioat fid:%x, cc:%d, status:%d\n", zdev->fid, cc, status);
> @@ -159,6 +162,7 @@ int zpci_fmb_enable_device(struct zpci_dev *zdev)
> atomic64_set(&zdev->unmapped_pages, 0);
>
> fib.fmb_addr = virt_to_phys(zdev->fmb);
> + fib.gd = zdev->gd;
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc) {
> kmem_cache_free(zdev_fmb_cache, zdev->fmb);
> @@ -177,6 +181,8 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
> if (!zdev->fmb)
> return -EINVAL;
>
> + fib.gd = zdev->gd;
> +
> /* Function measurement is disabled if fmb address is zero */
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc == 3) /* Function already gone. */
> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> index be077b39da33..e9ed0e4a5cf0 100644
> --- a/arch/s390/pci/pci_clp.c
> +++ b/arch/s390/pci/pci_clp.c
> @@ -240,6 +240,7 @@ static int clp_set_pci_fn(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as, u8 comma
> rrb->request.fh = zdev->fh;
> rrb->request.oc = command;
> rrb->request.ndas = nr_dma_as;
> + rrb->request.gd = zdev->gd;
>
> rc = clp_req(rrb, CLP_LPS_PCI);
> if (rrb->response.hdr.rsp == CLP_RC_SETPCIFN_BUSY) {
> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 2f675355fd0c..17e5adfe1273 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -43,6 +43,7 @@ static int zpci_set_airq(struct zpci_dev *zdev)
> fib.fmt0.aibvo = 0; /* each zdev has its own interrupt vector */
> fib.fmt0.aisb = virt_to_phys(zpci_sbv->vector) + (zdev->aisb / 64) * 8;
> fib.fmt0.aisbo = zdev->aisb & 63;
> + fib.gd = zdev->gd;
>
> return zpci_mod_fc(req, &fib, &status) ? -EIO : 0;
> }
> @@ -54,6 +55,8 @@ static int zpci_clear_airq(struct zpci_dev *zdev)
> struct zpci_fib fib = {0};
> u8 cc, status;
>
> + fib.gd = zdev->gd;
> +
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc == 3 || (cc == 1 && status == 24))
> /* Function already gone or IRQs already deregistered. */
> @@ -72,6 +75,7 @@ static int zpci_set_directed_irq(struct zpci_dev *zdev)
> fib.fmt = 1;
> fib.fmt1.noi = zdev->msi_nr_irqs;
> fib.fmt1.dibvo = zdev->msi_first_bit;
> + fib.gd = zdev->gd;
>
> return zpci_mod_fc(req, &fib, &status) ? -EIO : 0;
> }
> @@ -84,6 +88,7 @@ static int zpci_clear_directed_irq(struct zpci_dev *zdev)
> u8 cc, status;
>
> fib.fmt = 1;
> + fib.gd = zdev->gd;
> cc = zpci_mod_fc(req, &fib, &status);
> if (cc == 3 || (cc == 1 && status == 24))
> /* Function already gone or IRQs already deregistered. */
>

--
Pierre Morel
IBM Lab Boeblingen

2022-01-24 19:28:24

by Matthew Rosato

[permalink] [raw]
Subject: Re: [PATCH v2 08/30] s390/pci: stash associated GISA designation

On 1/24/22 9:08 AM, Pierre Morel wrote:
>
>
> On 1/14/22 21:31, Matthew Rosato wrote:
>> For passthrough devices, we will need to know the GISA designation of the
>> guest if interpretation facilities are to be used.  Setup to stash
>> this in
>> the zdev and set a default of 0 (no GISA designation) for now; a
>> subsequent
>> patch will set a valid GISA designation for passthrough devices.
>> Also, extend mpcific routines to specify this stashed designation as part
>> of the mpcific command.
>>
>> Reviewed-by: Niklas Schnelle <[email protected]>
>> Reviewed-by: Christian Borntraeger <[email protected]>
>> Reviewed-by: Eric Farman <[email protected]>
>> Reviewed-by: Pierre Morel <[email protected]>
>> Signed-off-by: Matthew Rosato <[email protected]>
>> ---
>>   arch/s390/include/asm/pci.h     | 1 +
>>   arch/s390/include/asm/pci_clp.h | 3 ++-
>>   arch/s390/pci/pci.c             | 6 ++++++
>>   arch/s390/pci/pci_clp.c         | 1 +
>>   arch/s390/pci/pci_irq.c         | 5 +++++
>>   5 files changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 90824be5ce9a..2474b8d30f2a 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -123,6 +123,7 @@ struct zpci_dev {
>>       enum zpci_state state;
>>       u32        fid;        /* function ID, used by sclp */
>>       u32        fh;        /* function handle, used by insn's */
>> +    u32        gd;        /* GISA designation for passthrough */
>
> I already gave my R-B, and do not want to remove it, but wouldn't it be
> possible to use more explicit names like gisa_designation instead of
> just gd.
> It would not change anything to the functionality but would facilitate
> the maintenance?
>

Honestly, I don't have a strong opinion on this one -- AFAICT struct
zpci_dev has a fair mix of short names (fh) and explicit names
(max_bus_speed).

It does require changes to this patch and various subsequent patches --
The changes are, as you say, not functional, so I think it's not a big deal?

I do think 'gisa_designation' is too verbose though -- How about just
'gisa', this is the same name used in the structure where we get this
value from (gisa in struct sie_page2)

As long as nobody objects I will s/gd/gisa/ here and in struct
clp_req_set_pci, retaining review tags.