| Message ID | 20211207205743.150299-13-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: enable zPCI for interpretive execution | expand |
On Tue, 2021-12-07 at 15:57 -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. Remove the unused data pointer > from clp_list_pci and __clp_add and instead optionally pass a pointer to > a model-dependent-data field. Additionally, allow for clp_list_pci calls > that don't specify a callback - in this case, just do the first pass of > list pci and exit. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > arch/s390/include/asm/pci.h | 6 ++++++ > arch/s390/include/asm/pci_clp.h | 2 +- > arch/s390/pci/pci.c | 19 +++++++++++++++++++ > arch/s390/pci/pci_clp.c | 16 ++++++++++------ > 4 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h > index 00a2c24d6d2b..86f43644756d 100644 > --- a/arch/s390/include/asm/pci.h > +++ b/arch/s390/include/asm/pci.h > @@ -219,12 +219,18 @@ int zpci_unregister_ioat(struct zpci_dev *, u8); > void zpci_remove_reserved_devices(void); > void zpci_update_fh(struct zpci_dev *zdev, u32 fh); > ---8<--- > -static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, > - void (*cb)(struct clp_fh_list_entry *, void *)) > +int clp_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, > + void (*cb)(struct clp_fh_list_entry *)) > { > u64 resume_token = 0; > int nentries, i, rc; > @@ -368,8 +368,12 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, > rc = clp_list_pci_req(rrb, &resume_token, &nentries); > if (rc) > return rc; > + if (mdd) > + *mdd = rrb->response.mdd; > + if (!cb) > + return 0; I think it would be slightly cleaner to instead de-static clp_list_pci_req() and call that directly. Just because that makes the clp_list_pci() still list all PCI functions and allows us to get rid of the data parameter completely. Also, I've been thinking about moving clp_scan_devices(), clp_get_state(), and clp_refresh_fh() out of pci_clp.c because they are higher level. I think that would nicely fit your zpci_get_mdd() in pci.c with or without the above suggestion. Then we could do the removal of the unused data parameter in that series as a cleanup. What do you think? > for (i = 0; i < nentries; i++) > - cb(&rrb->response.fh_list[i], data); > + cb(&rrb->response.fh_list[i]); > } while (resume_token); > > return rc; > @@ -398,7 +402,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid, > return -ENODEV; > } > > -static void __clp_add(struct clp_fh_list_entry *entry, void *data) > +static void __clp_add(struct clp_fh_list_entry *entry) > { > struct zpci_dev *zdev; >
Am 07.12.21 um 21:57 schrieb Matthew Rosato: > 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. Remove the unused data pointer > from clp_list_pci and __clp_add and instead optionally pass a pointer to > a model-dependent-data field. Additionally, allow for clp_list_pci calls > that don't specify a callback - in this case, just do the first pass of > list pci and exit. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > arch/s390/include/asm/pci.h | 6 ++++++ > arch/s390/include/asm/pci_clp.h | 2 +- > arch/s390/pci/pci.c | 19 +++++++++++++++++++ > arch/s390/pci/pci_clp.c | 16 ++++++++++------ > 4 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h > index 00a2c24d6d2b..86f43644756d 100644 > --- a/arch/s390/include/asm/pci.h > +++ b/arch/s390/include/asm/pci.h > @@ -219,12 +219,18 @@ int zpci_unregister_ioat(struct zpci_dev *, u8); > void zpci_remove_reserved_devices(void); > void zpci_update_fh(struct zpci_dev *zdev, u32 fh); > > +int zpci_get_mdd(u32 *mdd); > + > /* CLP */ > +void *clp_alloc_block(gfp_t gfp_mask); > +void clp_free_block(void *ptr); > int clp_setup_writeback_mio(void); > int clp_scan_pci_devices(void); > int clp_query_pci_fn(struct zpci_dev *zdev); > 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_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, > + void (*cb)(struct clp_fh_list_entry *)); > int clp_get_state(u32 fid, enum zpci_state *state); > int clp_refresh_fh(u32 fid, u32 *fh); > > 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.c b/arch/s390/pci/pci.c > index af1c0ae017b1..175854c861cd 100644 > --- a/arch/s390/pci/pci.c > +++ b/arch/s390/pci/pci.c > @@ -531,6 +531,25 @@ void zpci_update_fh(struct zpci_dev *zdev, u32 fh) > zpci_do_update_iomap_fh(zdev, fh); > } > > +int zpci_get_mdd(u32 *mdd) > +{ > + struct clp_req_rsp_list_pci *rrb; > + int rc; > + > + if (!mdd) > + return -EINVAL; > + > + rrb = clp_alloc_block(GFP_KERNEL); > + if (!rrb) > + return -ENOMEM; > + > + rc = clp_list_pci(rrb, mdd, NULL); > + > + clp_free_block(rrb); > + return rc; > +} > +EXPORT_SYMBOL_GPL(zpci_get_mdd); Maybe move this into pci_clp.c to avoid the export of clp_alloc_block and void clp_free_block? Niklas? In any case the code looks correct from a HW perspective. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > + > static struct resource *__alloc_res(struct zpci_dev *zdev, unsigned long start, > unsigned long size, unsigned long flags) > { > diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c > index bc7446566cbc..e18a548ac22d 100644 > --- a/arch/s390/pci/pci_clp.c > +++ b/arch/s390/pci/pci_clp.c > @@ -84,12 +84,12 @@ static __always_inline int clp_req(void *data, unsigned int lps) > return cc; > } > > -static void *clp_alloc_block(gfp_t gfp_mask) > +void *clp_alloc_block(gfp_t gfp_mask) > { > return (void *) __get_free_pages(gfp_mask, get_order(CLP_BLK_SIZE)); > } > > -static void clp_free_block(void *ptr) > +void clp_free_block(void *ptr) > { > free_pages((unsigned long) ptr, get_order(CLP_BLK_SIZE)); > } > @@ -358,8 +358,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb, > return rc; > } > > -static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, > - void (*cb)(struct clp_fh_list_entry *, void *)) > +int clp_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, > + void (*cb)(struct clp_fh_list_entry *)) > { > u64 resume_token = 0; > int nentries, i, rc; > @@ -368,8 +368,12 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, > rc = clp_list_pci_req(rrb, &resume_token, &nentries); > if (rc) > return rc; > + if (mdd) > + *mdd = rrb->response.mdd; > + if (!cb) > + return 0; > for (i = 0; i < nentries; i++) > - cb(&rrb->response.fh_list[i], data); > + cb(&rrb->response.fh_list[i]); > } while (resume_token); > > return rc; > @@ -398,7 +402,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid, > return -ENODEV; > } > > -static void __clp_add(struct clp_fh_list_entry *entry, void *data) > +static void __clp_add(struct clp_fh_list_entry *entry) > { > struct zpci_dev *zdev; > >
On 12/8/21 7:21 AM, Niklas Schnelle wrote: > On Tue, 2021-12-07 at 15:57 -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. Remove the unused data pointer >> from clp_list_pci and __clp_add and instead optionally pass a pointer to >> a model-dependent-data field. Additionally, allow for clp_list_pci calls >> that don't specify a callback - in this case, just do the first pass of >> list pci and exit. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> arch/s390/include/asm/pci.h | 6 ++++++ >> arch/s390/include/asm/pci_clp.h | 2 +- >> arch/s390/pci/pci.c | 19 +++++++++++++++++++ >> arch/s390/pci/pci_clp.c | 16 ++++++++++------ >> 4 files changed, 36 insertions(+), 7 deletions(-) >> >> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h >> index 00a2c24d6d2b..86f43644756d 100644 >> --- a/arch/s390/include/asm/pci.h >> +++ b/arch/s390/include/asm/pci.h >> @@ -219,12 +219,18 @@ int zpci_unregister_ioat(struct zpci_dev *, u8); >> void zpci_remove_reserved_devices(void); >> void zpci_update_fh(struct zpci_dev *zdev, u32 fh); >> > ---8<--- >> -static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, >> - void (*cb)(struct clp_fh_list_entry *, void *)) >> +int clp_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, >> + void (*cb)(struct clp_fh_list_entry *)) >> { >> u64 resume_token = 0; >> int nentries, i, rc; >> @@ -368,8 +368,12 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, >> rc = clp_list_pci_req(rrb, &resume_token, &nentries); >> if (rc) >> return rc; >> + if (mdd) >> + *mdd = rrb->response.mdd; >> + if (!cb) >> + return 0; > > I think it would be slightly cleaner to instead de-static > clp_list_pci_req() and call that directly. Just because that makes the > clp_list_pci() still list all PCI functions and allows us to get rid of > the data parameter completely. > Oops, I must have missed this comment before. Sure, makes sense. > Also, I've been thinking about moving clp_scan_devices(), > clp_get_state(), and clp_refresh_fh() out of pci_clp.c because they are > higher level. I think that would nicely fit your zpci_get_mdd() in > pci.c with or without the above suggestion. Then we could do the > removal of the unused data parameter in that series as a cleanup. What > do you think? Sure, that would be fine. So then this patch instead just exports alloc/free/clp_list_pci_req and the new zpci_get_mdd calls clp_list_pci_req directly. I'll drop the changes to clp_list_pci() and __clp_add (and re-word the commit message) > >> for (i = 0; i < nentries; i++) >> - cb(&rrb->response.fh_list[i], data); >> + cb(&rrb->response.fh_list[i]); >> } while (resume_token); >> >> return rc; >> @@ -398,7 +402,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid, >> return -ENODEV; >> } >> >> -static void __clp_add(struct clp_fh_list_entry *entry, void *data) >> +static void __clp_add(struct clp_fh_list_entry *entry) >> { >> struct zpci_dev *zdev; >> >
On Thu, 2021-12-09 at 16:47 +0100, Christian Borntraeger wrote: > Am 07.12.21 um 21:57 schrieb Matthew Rosato: > > 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. Remove the unused data pointer > > from clp_list_pci and __clp_add and instead optionally pass a pointer to > > a model-dependent-data field. Additionally, allow for clp_list_pci calls > > that don't specify a callback - in this case, just do the first pass of > > list pci and exit. > > > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > > --- > > arch/s390/include/asm/pci.h | 6 ++++++ > > arch/s390/include/asm/pci_clp.h | 2 +- > > arch/s390/pci/pci.c | 19 +++++++++++++++++++ > > arch/s390/pci/pci_clp.c | 16 ++++++++++------ > > 4 files changed, 36 insertions(+), 7 deletions(-) > > ---8<--- > > > > +int zpci_get_mdd(u32 *mdd) > > +{ > > + struct clp_req_rsp_list_pci *rrb; > > + int rc; > > + > > + if (!mdd) > > + return -EINVAL; > > + > > + rrb = clp_alloc_block(GFP_KERNEL); > > + if (!rrb) > > + return -ENOMEM; > > + > > + rc = clp_list_pci(rrb, mdd, NULL); > > + > > + clp_free_block(rrb); > > + return rc; > > +} > > +EXPORT_SYMBOL_GPL(zpci_get_mdd); > > Maybe move this into pci_clp.c to avoid the export of clp_alloc_block and void clp_free_block? > Niklas? That was actually my idea. I'm thinking of moving clp_get_state(), clp_scan_pci_devices(), ans clp_refresh_fh() to pci.c too because I feel these deal with higher level concerns than the rest of pci_clp.c. I have no strong opinion though and might be thinking ahead to much here. With the change discussed in the other mail of not modifying clp_list_pci() maybe it would be better to keep it here and thus this patch more focused and minimal and then possibly move it with the other similar functions.
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 00a2c24d6d2b..86f43644756d 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -219,12 +219,18 @@ int zpci_unregister_ioat(struct zpci_dev *, u8); void zpci_remove_reserved_devices(void); void zpci_update_fh(struct zpci_dev *zdev, u32 fh); +int zpci_get_mdd(u32 *mdd); + /* CLP */ +void *clp_alloc_block(gfp_t gfp_mask); +void clp_free_block(void *ptr); int clp_setup_writeback_mio(void); int clp_scan_pci_devices(void); int clp_query_pci_fn(struct zpci_dev *zdev); 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_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, + void (*cb)(struct clp_fh_list_entry *)); int clp_get_state(u32 fid, enum zpci_state *state); int clp_refresh_fh(u32 fid, u32 *fh); 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.c b/arch/s390/pci/pci.c index af1c0ae017b1..175854c861cd 100644 --- a/arch/s390/pci/pci.c +++ b/arch/s390/pci/pci.c @@ -531,6 +531,25 @@ void zpci_update_fh(struct zpci_dev *zdev, u32 fh) zpci_do_update_iomap_fh(zdev, fh); } +int zpci_get_mdd(u32 *mdd) +{ + struct clp_req_rsp_list_pci *rrb; + int rc; + + if (!mdd) + return -EINVAL; + + rrb = clp_alloc_block(GFP_KERNEL); + if (!rrb) + return -ENOMEM; + + rc = clp_list_pci(rrb, mdd, NULL); + + clp_free_block(rrb); + return rc; +} +EXPORT_SYMBOL_GPL(zpci_get_mdd); + static struct resource *__alloc_res(struct zpci_dev *zdev, unsigned long start, unsigned long size, unsigned long flags) { diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c index bc7446566cbc..e18a548ac22d 100644 --- a/arch/s390/pci/pci_clp.c +++ b/arch/s390/pci/pci_clp.c @@ -84,12 +84,12 @@ static __always_inline int clp_req(void *data, unsigned int lps) return cc; } -static void *clp_alloc_block(gfp_t gfp_mask) +void *clp_alloc_block(gfp_t gfp_mask) { return (void *) __get_free_pages(gfp_mask, get_order(CLP_BLK_SIZE)); } -static void clp_free_block(void *ptr) +void clp_free_block(void *ptr) { free_pages((unsigned long) ptr, get_order(CLP_BLK_SIZE)); } @@ -358,8 +358,8 @@ static int clp_list_pci_req(struct clp_req_rsp_list_pci *rrb, return rc; } -static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, - void (*cb)(struct clp_fh_list_entry *, void *)) +int clp_list_pci(struct clp_req_rsp_list_pci *rrb, u32 *mdd, + void (*cb)(struct clp_fh_list_entry *)) { u64 resume_token = 0; int nentries, i, rc; @@ -368,8 +368,12 @@ static int clp_list_pci(struct clp_req_rsp_list_pci *rrb, void *data, rc = clp_list_pci_req(rrb, &resume_token, &nentries); if (rc) return rc; + if (mdd) + *mdd = rrb->response.mdd; + if (!cb) + return 0; for (i = 0; i < nentries; i++) - cb(&rrb->response.fh_list[i], data); + cb(&rrb->response.fh_list[i]); } while (resume_token); return rc; @@ -398,7 +402,7 @@ static int clp_find_pci(struct clp_req_rsp_list_pci *rrb, u32 fid, return -ENODEV; } -static void __clp_add(struct clp_fh_list_entry *entry, void *data) +static void __clp_add(struct clp_fh_list_entry *entry) { struct zpci_dev *zdev;
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. Remove the unused data pointer from clp_list_pci and __clp_add and instead optionally pass a pointer to a model-dependent-data field. Additionally, allow for clp_list_pci calls that don't specify a callback - in this case, just do the first pass of list pci and exit. Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> --- arch/s390/include/asm/pci.h | 6 ++++++ arch/s390/include/asm/pci_clp.h | 2 +- arch/s390/pci/pci.c | 19 +++++++++++++++++++ arch/s390/pci/pci_clp.c | 16 ++++++++++------ 4 files changed, 36 insertions(+), 7 deletions(-)