Re: [PATCH v5 08/21] s390/pci: stash associated GISA designation

[Christian Borntraeger <borntraeger@linux.ibm.com>]

Am 05.04.22 um 10:03 schrieb Niklas Schnelle:
> On Mon, 2022-04-04 at 13:43 -0400, 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.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

>> ---
>>   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         | 5 +++++
>>   arch/s390/pci/pci_irq.c         | 5 +++++
>>   5 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index fdb9745ee998..42a4a312a6dd 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		gisa;		/* 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..f3286bc5ba6e 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 gisa;			/* GISA designation */
>>   } __packed;
>>   
>>   /* Set PCI function response */
>> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
>> index e563cb65c0c4..a86cd1cbb80e 100644
>> --- a/arch/s390/pci/pci.c
>> +++ b/arch/s390/pci/pci.c
>> @@ -120,6 +120,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->gisa;
>>   	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);
>> @@ -133,6 +134,8 @@ int zpci_unregister_ioat(struct zpci_dev *zdev, u8 dmaas)
>>   	struct zpci_fib fib = {0};
>>   	u8 cc, status;
>>   
>> +	fib.gd = zdev->gisa;
> 
> This could go into the initializer which becomes:
> 
>     struct zpci_fib fib = {.gd = zdev->gisa};
> 
>> +
>>   	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);
>> @@ -160,6 +163,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->gisa;
>>   	cc = zpci_mod_fc(req, &fib, &status);
>>   	if (cc) {
>>   		kmem_cache_free(zdev_fmb_cache, zdev->fmb);
>> @@ -178,6 +182,8 @@ int zpci_fmb_disable_device(struct zpci_dev *zdev)
>>   	if (!zdev->fmb)
>>   		return -EINVAL;
>>   
>> +	fib.gd = zdev->gisa;
>> +
>>   	/* 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 1057d7af4a55..deed35edea14 100644
>> --- a/arch/s390/pci/pci_clp.c
>> +++ b/arch/s390/pci/pci_clp.c
>> @@ -229,12 +229,16 @@ static int clp_set_pci_fn(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as, u8 comma
>>   {
>>   	struct clp_req_rsp_set_pci *rrb;
>>   	int rc, retries = 100;
>> +	u32 gisa = 0;
>>   
>>   	*fh = 0;
>>   	rrb = clp_alloc_block(GFP_KERNEL);
>>   	if (!rrb)
>>   		return -ENOMEM;
>>   
>> +	if (command != CLP_SET_DISABLE_PCI_FN)
>> +		gisa = zdev->gisa;
>> +
>>   	do {
>>   		memset(rrb, 0, sizeof(*rrb));
>>   		rrb->request.hdr.len = sizeof(rrb->request);
>> @@ -243,6 +247,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.gisa = gisa;
>>   
>>   		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 f2b3145b6697..a2b42a63a53b 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->gisa;
>>   
>>   	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->gisa;
>> +
> 
> Same as in zpci_register_ioat()
> 
>>   	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->gisa;
>>   
>>   	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->gisa;
>>   	cc = zpci_mod_fc(req, &fib, &status);
>>   	if (cc == 3 || (cc == 1 && status == 24))
>>   		/* Function already gone or IRQs already deregistered. */
> 
> Looks good with or without using an initializer:
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
>