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

[Pierre Morel <pmorel@xxxxxxxxxxxxx>]

On 1/26/22 11:13, Claudio Imbrenda wrote:
> On Tue, 18 Jan 2022 11:36:06 +0100
> Pierre Morel <pmorel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > >    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);
> > >    

--
Pierre Morel
IBM Lab Boeblingen