diff mbox series

[v2,12/30] s390/pci: get SHM information from list pci

Message ID 20220114203145.242984-13-mjrosato@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series KVM: s390: enable zPCI for interpretive execution | expand

Commit Message

Matthew Rosato Jan. 14, 2022, 8:31 p.m. UTC
KVM will need information on the special handle mask used to indicate
emulated devices.  In order to obtain this, a new type of list pci call
must be made to gather the information.  Extend clp_list_pci_req to
also fetch the model-dependent-data field that holds this mask.

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 arch/s390/include/asm/pci.h     |  1 +
 arch/s390/include/asm/pci_clp.h |  2 +-
 arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
 3 files changed, 27 insertions(+), 4 deletions(-)

Comments

Pierre Morel Jan. 18, 2022, 10:36 a.m. UTC | #1
On 1/14/22 21:31, Matthew Rosato wrote:
> KVM will need information on the special handle mask used to indicate
> emulated devices.  In order to obtain this, a new type of list pci call
> must be made to gather the information.  Extend clp_list_pci_req to
> also fetch the model-dependent-data field that holds this mask.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   arch/s390/include/asm/pci.h     |  1 +
>   arch/s390/include/asm/pci_clp.h |  2 +-
>   arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
>   3 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 00a2c24d6d2b..f3cd2da8128c 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
>   int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
>   int clp_get_state(u32 fid, enum zpci_state *state);
>   int clp_refresh_fh(u32 fid, u32 *fh);
> +int zpci_get_mdd(u32 *mdd);
>   
>   /* UID */
>   void update_uid_checking(bool new);
> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> index 124fadfb74b9..d6bc324763f3 100644
> --- a/arch/s390/include/asm/pci_clp.h
> +++ b/arch/s390/include/asm/pci_clp.h
> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
>   struct clp_rsp_list_pci {
>   	struct clp_rsp_hdr hdr;
>   	u64 resume_token;
> -	u32 reserved2;
> +	u32 mdd;
>   	u16 max_fn;
>   	u8			: 7;
>   	u8 uid_checking		: 1;
> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> index bc7446566cbc..308ffb93413f 100644
> --- a/arch/s390/pci/pci_clp.c
> +++ b/arch/s390/pci/pci_clp.c
> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
>   }
>   
>   static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> -			    u64 *resume_token, int *nentries)
> +			    u64 *resume_token, int *nentries, u32 *mdd)
>   {
>   	int rc;
>   
> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>   	*nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
>   		rrb->response.entry_size;
>   	*resume_token = rrb->response.resume_token;
> +	if (mdd)
> +		*mdd = rrb->response.mdd;
>   
>   	return rc;
>   }
> @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
>   	int nentries, i, rc;
>   
>   	do {
> -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>   		if (rc)
>   			return rc;
>   		for (i = 0; i < nentries; i++)
> @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
>   	int nentries, i, rc;
>   
>   	do {
> -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>   		if (rc)
>   			return rc;
>   		fh_list = rrb->response.fh_list;
> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
>   	return rc;
>   }
>   
> +int zpci_get_mdd(u32 *mdd)
> +{
> +	struct clp_req_rsp_list_pci *rrb;
> +	u64 resume_token = 0;
> +	int nentries, rc;
> +
> +	if (!mdd)
> +		return -EINVAL;

I think this tests is not useful.
The caller must take care not to call with a NULL pointer,
what the only caller today make sure.


> +
> +	rrb = clp_alloc_block(GFP_KERNEL);
> +	if (!rrb)
> +		return -ENOMEM;
> +
> +	rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> +
> +	clp_free_block(rrb);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> +
>   static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
>   {
>   	unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
>
Claudio Imbrenda Jan. 26, 2022, 10:13 a.m. UTC | #2
On Tue, 18 Jan 2022 11:36:06 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 1/14/22 21:31, Matthew Rosato wrote:
> > KVM will need information on the special handle mask used to indicate
> > emulated devices.  In order to obtain this, a new type of list pci call
> > must be made to gather the information.  Extend clp_list_pci_req to
> > also fetch the model-dependent-data field that holds this mask.
> > 
> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> > ---
> >   arch/s390/include/asm/pci.h     |  1 +
> >   arch/s390/include/asm/pci_clp.h |  2 +-
> >   arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
> >   3 files changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > index 00a2c24d6d2b..f3cd2da8128c 100644
> > --- a/arch/s390/include/asm/pci.h
> > +++ b/arch/s390/include/asm/pci.h
> > @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
> >   int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
> >   int clp_get_state(u32 fid, enum zpci_state *state);
> >   int clp_refresh_fh(u32 fid, u32 *fh);
> > +int zpci_get_mdd(u32 *mdd);
> >   
> >   /* UID */
> >   void update_uid_checking(bool new);
> > diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> > index 124fadfb74b9..d6bc324763f3 100644
> > --- a/arch/s390/include/asm/pci_clp.h
> > +++ b/arch/s390/include/asm/pci_clp.h
> > @@ -76,7 +76,7 @@ struct clp_req_list_pci {
> >   struct clp_rsp_list_pci {
> >   	struct clp_rsp_hdr hdr;
> >   	u64 resume_token;
> > -	u32 reserved2;
> > +	u32 mdd;
> >   	u16 max_fn;
> >   	u8			: 7;
> >   	u8 uid_checking		: 1;
> > diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> > index bc7446566cbc..308ffb93413f 100644
> > --- a/arch/s390/pci/pci_clp.c
> > +++ b/arch/s390/pci/pci_clp.c
> > @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
> >   }
> >   
> >   static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> > -			    u64 *resume_token, int *nentries)
> > +			    u64 *resume_token, int *nentries, u32 *mdd)
> >   {
> >   	int rc;
> >   
> > @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
> >   	*nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
> >   		rrb->response.entry_size;
> >   	*resume_token = rrb->response.resume_token;
> > +	if (mdd)
> > +		*mdd = rrb->response.mdd;
> >   
> >   	return rc;
> >   }
> > @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
> >   	int nentries, i, rc;
> >   
> >   	do {
> > -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> > +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> >   		if (rc)
> >   			return rc;
> >   		for (i = 0; i < nentries; i++)
> > @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
> >   	int nentries, i, rc;
> >   
> >   	do {
> > -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
> > +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
> >   		if (rc)
> >   			return rc;
> >   		fh_list = rrb->response.fh_list;
> > @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
> >   	return rc;
> >   }
> >   
> > +int zpci_get_mdd(u32 *mdd)
> > +{
> > +	struct clp_req_rsp_list_pci *rrb;
> > +	u64 resume_token = 0;
> > +	int nentries, rc;
> > +
> > +	if (!mdd)
> > +		return -EINVAL;  
> 
> I think this tests is not useful.
> The caller must take care not to call with a NULL pointer,
> what the only caller today make sure.

what if the caller does it anyway?

I think the test is useful. if passing NULL is a bug, then maybe
consider using BUG_ON, or WARN_ONCE

> 
> 
> > +
> > +	rrb = clp_alloc_block(GFP_KERNEL);
> > +	if (!rrb)
> > +		return -ENOMEM;
> > +
> > +	rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> > +
> > +	clp_free_block(rrb);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> > +
> >   static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
> >   {
> >   	unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
> >   
>
Niklas Schnelle Jan. 27, 2022, 10:29 a.m. UTC | #3
On Fri, 2022-01-14 at 15:31 -0500, Matthew Rosato wrote:
> KVM will need information on the special handle mask used to indicate
> emulated devices.  In order to obtain this, a new type of list pci call
> must be made to gather the information.  Extend clp_list_pci_req to
> also fetch the model-dependent-data field that holds this mask.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>  arch/s390/include/asm/pci.h     |  1 +
>  arch/s390/include/asm/pci_clp.h |  2 +-
>  arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
>  3 files changed, 27 insertions(+), 4 deletions(-)
> 
---8<---
>  
> +int zpci_get_mdd(u32 *mdd)
> +{
> +	struct clp_req_rsp_list_pci *rrb;
> +	u64 resume_token = 0;
> +	int nentries, rc;
> +
> +	if (!mdd)
> +		return -EINVAL;
> +
> +	rrb = clp_alloc_block(GFP_KERNEL);
> +	if (!rrb)
> +		return -ENOMEM;
> +
> +	rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
> +
> +	clp_free_block(rrb);
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
> +
>  static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
>  {
>  	unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);

Looks good.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Pierre Morel Jan. 27, 2022, 1:41 p.m. UTC | #4
On 1/26/22 11:13, Claudio Imbrenda wrote:
> On Tue, 18 Jan 2022 11:36:06 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 1/14/22 21:31, Matthew Rosato wrote:
>>> KVM will need information on the special handle mask used to indicate
>>> emulated devices.  In order to obtain this, a new type of list pci call
>>> must be made to gather the information.  Extend clp_list_pci_req to
>>> also fetch the model-dependent-data field that holds this mask.
>>>
>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>> ---
>>>    arch/s390/include/asm/pci.h     |  1 +
>>>    arch/s390/include/asm/pci_clp.h |  2 +-
>>>    arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
>>>    3 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>> index 00a2c24d6d2b..f3cd2da8128c 100644
>>> --- a/arch/s390/include/asm/pci.h
>>> +++ b/arch/s390/include/asm/pci.h
>>> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
>>>    int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
>>>    int clp_get_state(u32 fid, enum zpci_state *state);
>>>    int clp_refresh_fh(u32 fid, u32 *fh);
>>> +int zpci_get_mdd(u32 *mdd);
>>>    
>>>    /* UID */
>>>    void update_uid_checking(bool new);
>>> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
>>> index 124fadfb74b9..d6bc324763f3 100644
>>> --- a/arch/s390/include/asm/pci_clp.h
>>> +++ b/arch/s390/include/asm/pci_clp.h
>>> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
>>>    struct clp_rsp_list_pci {
>>>    	struct clp_rsp_hdr hdr;
>>>    	u64 resume_token;
>>> -	u32 reserved2;
>>> +	u32 mdd;
>>>    	u16 max_fn;
>>>    	u8			: 7;
>>>    	u8 uid_checking		: 1;
>>> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
>>> index bc7446566cbc..308ffb93413f 100644
>>> --- a/arch/s390/pci/pci_clp.c
>>> +++ b/arch/s390/pci/pci_clp.c
>>> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
>>>    }
>>>    
>>>    static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>> -			    u64 *resume_token, int *nentries)
>>> +			    u64 *resume_token, int *nentries, u32 *mdd)
>>>    {
>>>    	int rc;
>>>    
>>> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>>    	*nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
>>>    		rrb->response.entry_size;
>>>    	*resume_token = rrb->response.resume_token;
>>> +	if (mdd)
>>> +		*mdd = rrb->response.mdd;
>>>    
>>>    	return rc;
>>>    }
>>> @@ -365,7 +367,7 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
>>>    	int nentries, i, rc;
>>>    
>>>    	do {
>>> -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>> +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>    		if (rc)
>>>    			return rc;
>>>    		for (i = 0; i < nentries; i++)
>>> @@ -383,7 +385,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
>>>    	int nentries, i, rc;
>>>    
>>>    	do {
>>> -		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>> +		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>    		if (rc)
>>>    			return rc;
>>>    		fh_list = rrb->response.fh_list;
>>> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
>>>    	return rc;
>>>    }
>>>    
>>> +int zpci_get_mdd(u32 *mdd)
>>> +{
>>> +	struct clp_req_rsp_list_pci *rrb;
>>> +	u64 resume_token = 0;
>>> +	int nentries, rc;
>>> +
>>> +	if (!mdd)
>>> +		return -EINVAL;
>>
>> I think this tests is not useful.
>> The caller must take care not to call with a NULL pointer,
>> what the only caller today make sure.
> 
> what if the caller does it anyway?
> 
> I think the test is useful. if passing NULL is a bug, then maybe
> consider using BUG_ON, or WARN_ONCE

I think generally the caller is responsible for the test.
In our case for example the caller can use directly the address
of a u32 allocated on the stack or globaly and he knows if a test is 
useful or not.

Of course we can systematically check in every kernel function all 
pointer parameters against NULL.
But this is not userland, not even a inter-architecture core function 
and we can expect the kernel programmer to programm correctly.

For our special case zpci_get_mdd() nor clp_list_pci_req() do access 
*mdd if mdd is NULL so that giving a NULL mdd pointer will not trigger a 
fault.
Also, the function is named zpci_get_mdd(u32 *mdd) if the caller do not 
give a pointer to mdd which would be quite stupid to call a function 
zpci_get_mdd() in this circumstance he will just no get mdd, no side effect.
So the only purpose having this test here is to say the caller that he 
forgot to check his mdd allocation.
My opinion is that he should have check.

> 
>>
>>
>>> +
>>> +	rrb = clp_alloc_block(GFP_KERNEL);
>>> +	if (!rrb)
>>> +		return -ENOMEM;
>>> +
>>> +	rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
>>> +
>>> +	clp_free_block(rrb);
>>> +	return rc;
>>> +}
>>> +EXPORT_SYMBOL_GPL(zpci_get_mdd);
>>> +
>>>    static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
>>>    {
>>>    	unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);
>>>    
>>
>
Matthew Rosato Jan. 27, 2022, 3:14 p.m. UTC | #5
On 1/27/22 8:41 AM, Pierre Morel wrote:
> 
> 
> On 1/26/22 11:13, Claudio Imbrenda wrote:
>> On Tue, 18 Jan 2022 11:36:06 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 1/14/22 21:31, Matthew Rosato wrote:
>>>> KVM will need information on the special handle mask used to indicate
>>>> emulated devices.  In order to obtain this, a new type of list pci call
>>>> must be made to gather the information.  Extend clp_list_pci_req to
>>>> also fetch the model-dependent-data field that holds this mask.
>>>>
>>>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> ---
>>>>    arch/s390/include/asm/pci.h     |  1 +
>>>>    arch/s390/include/asm/pci_clp.h |  2 +-
>>>>    arch/s390/pci/pci_clp.c         | 28 +++++++++++++++++++++++++---
>>>>    3 files changed, 27 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>>> index 00a2c24d6d2b..f3cd2da8128c 100644
>>>> --- a/arch/s390/include/asm/pci.h
>>>> +++ b/arch/s390/include/asm/pci.h
>>>> @@ -227,6 +227,7 @@ int clp_enable_fh(struct zpci_dev *zdev, u32 
>>>> *fh, u8 nr_dma_as);
>>>>    int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
>>>>    int clp_get_state(u32 fid, enum zpci_state *state);
>>>>    int clp_refresh_fh(u32 fid, u32 *fh);
>>>> +int zpci_get_mdd(u32 *mdd);
>>>>    /* UID */
>>>>    void update_uid_checking(bool new);
>>>> diff --git a/arch/s390/include/asm/pci_clp.h 
>>>> b/arch/s390/include/asm/pci_clp.h
>>>> index 124fadfb74b9..d6bc324763f3 100644
>>>> --- a/arch/s390/include/asm/pci_clp.h
>>>> +++ b/arch/s390/include/asm/pci_clp.h
>>>> @@ -76,7 +76,7 @@ struct clp_req_list_pci {
>>>>    struct clp_rsp_list_pci {
>>>>        struct clp_rsp_hdr hdr;
>>>>        u64 resume_token;
>>>> -    u32 reserved2;
>>>> +    u32 mdd;
>>>>        u16 max_fn;
>>>>        u8            : 7;
>>>>        u8 uid_checking        : 1;
>>>> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
>>>> index bc7446566cbc..308ffb93413f 100644
>>>> --- a/arch/s390/pci/pci_clp.c
>>>> +++ b/arch/s390/pci/pci_clp.c
>>>> @@ -328,7 +328,7 @@ int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
>>>>    }
>>>>    static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
>>>> -                u64 *resume_token, int *nentries)
>>>> +                u64 *resume_token, int *nentries, u32 *mdd)
>>>>    {
>>>>        int rc;
>>>> @@ -354,6 +354,8 @@ static int clp_list_pci_req(struct 
>>>> clp_req_rsp_list_pci *rrb,
>>>>        *nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
>>>>            rrb->response.entry_size;
>>>>        *resume_token = rrb->response.resume_token;
>>>> +    if (mdd)
>>>> +        *mdd = rrb->response.mdd;
>>>>        return rc;
>>>>    }
>>>> @@ -365,7 +367,7 @@ static int clp_list_pci(struct 
>>>> clp_req_rsp_list_pci *rrb, void *data,
>>>>        int nentries, i, rc;
>>>>        do {
>>>> -        rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>>> +        rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>>            if (rc)
>>>>                return rc;
>>>>            for (i = 0; i < nentries; i++)
>>>> @@ -383,7 +385,7 @@ static int clp_find_pci(struct 
>>>> clp_req_rsp_list_pci *rrb, u32 fid,
>>>>        int nentries, i, rc;
>>>>        do {
>>>> -        rc = clp_list_pci_req(rrb, &resume_token, &nentries);
>>>> +        rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
>>>>            if (rc)
>>>>                return rc;
>>>>            fh_list = rrb->response.fh_list;
>>>> @@ -468,6 +470,26 @@ int clp_get_state(u32 fid, enum zpci_state *state)
>>>>        return rc;
>>>>    }
>>>> +int zpci_get_mdd(u32 *mdd)
>>>> +{
>>>> +    struct clp_req_rsp_list_pci *rrb;
>>>> +    u64 resume_token = 0;
>>>> +    int nentries, rc;
>>>> +
>>>> +    if (!mdd)
>>>> +        return -EINVAL;
>>>
>>> I think this tests is not useful.
>>> The caller must take care not to call with a NULL pointer,
>>> what the only caller today make sure.
>>
>> what if the caller does it anyway?
>>
>> I think the test is useful. if passing NULL is a bug, then maybe
>> consider using BUG_ON, or WARN_ONCE
> 
> I think generally the caller is responsible for the test.
> In our case for example the caller can use directly the address
> of a u32 allocated on the stack or globaly and he knows if a test is 
> useful or not.
> 
> Of course we can systematically check in every kernel function all 
> pointer parameters against NULL.
> But this is not userland, not even a inter-architecture core function 
> and we can expect the kernel programmer to programm correctly.

I appreciate your optimism :)

> 
> For our special case zpci_get_mdd() nor clp_list_pci_req() do access 
> *mdd if mdd is NULL so that giving a NULL mdd pointer will not trigger a 
> fault.
> Also, the function is named zpci_get_mdd(u32 *mdd) if the caller do not 
> give a pointer to mdd which would be quite stupid to call a function 
> zpci_get_mdd() in this circumstance he will just no get mdd, no side 
> effect.
> So the only purpose having this test here is to say the caller that he 
> forgot to check his mdd allocation.
> My opinion is that he should have check.

Based on the thread of conversation, I'm going to assume you meant 'My 
opinion that he should -not- have the check here'

So, I'm generally a fan of being a bit defensive in paths that are not 
performance-intensive and would therefore typically tend to agree with 
Claudio.  But in this particular case, I'm willing to just drop this 
check and move on, primarily because 1) we only have a single caller 
already checking this case and 2) I don't really anticipate any 
additional callers later.

@Niklas do you care / is it OK to keep your R-b if I remove this 
if(!mdd) check?
diff mbox series

Patch

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 00a2c24d6d2b..f3cd2da8128c 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -227,6 +227,7 @@  int clp_enable_fh(struct zpci_dev *zdev, u32 *fh, u8 nr_dma_as);
 int clp_disable_fh(struct zpci_dev *zdev, u32 *fh);
 int clp_get_state(u32 fid, enum zpci_state *state);
 int clp_refresh_fh(u32 fid, u32 *fh);
+int zpci_get_mdd(u32 *mdd);
 
 /* UID */
 void update_uid_checking(bool new);
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 124fadfb74b9..d6bc324763f3 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -76,7 +76,7 @@  struct clp_req_list_pci {
 struct clp_rsp_list_pci {
 	struct clp_rsp_hdr hdr;
 	u64 resume_token;
-	u32 reserved2;
+	u32 mdd;
 	u16 max_fn;
 	u8			: 7;
 	u8 uid_checking		: 1;
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index bc7446566cbc..308ffb93413f 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -328,7 +328,7 @@  int clp_disable_fh(struct zpci_dev *zdev, u32 *fh)
 }
 
 static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
-			    u64 *resume_token, int *nentries)
+			    u64 *resume_token, int *nentries, u32 *mdd)
 {
 	int rc;
 
@@ -354,6 +354,8 @@  static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb,
 	*nentries = (rrb->response.hdr.len - LIST_PCI_HDR_LEN) /
 		rrb->response.entry_size;
 	*resume_token = rrb->response.resume_token;
+	if (mdd)
+		*mdd = rrb->response.mdd;
 
 	return rc;
 }
@@ -365,7 +367,7 @@  static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data,
 	int nentries, i, rc;
 
 	do {
-		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
 		if (rc)
 			return rc;
 		for (i = 0; i < nentries; i++)
@@ -383,7 +385,7 @@  static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid,
 	int nentries, i, rc;
 
 	do {
-		rc = clp_list_pci_req(rrb, &resume_token, &nentries);
+		rc = clp_list_pci_req(rrb, &resume_token, &nentries, NULL);
 		if (rc)
 			return rc;
 		fh_list = rrb->response.fh_list;
@@ -468,6 +470,26 @@  int clp_get_state(u32 fid, enum zpci_state *state)
 	return rc;
 }
 
+int zpci_get_mdd(u32 *mdd)
+{
+	struct clp_req_rsp_list_pci *rrb;
+	u64 resume_token = 0;
+	int nentries, rc;
+
+	if (!mdd)
+		return -EINVAL;
+
+	rrb = clp_alloc_block(GFP_KERNEL);
+	if (!rrb)
+		return -ENOMEM;
+
+	rc = clp_list_pci_req(rrb, &resume_token, &nentries, mdd);
+
+	clp_free_block(rrb);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(zpci_get_mdd);
+
 static int clp_base_slpc(struct clp_req *req, struct clp_req_rsp_slpc *lpcb)
 {
 	unsigned long limit = PAGE_SIZE - sizeof(lpcb->request);