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

[Matthew Rosato <mjrosato@xxxxxxxxxxxxx>]

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@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.

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?