| Message ID | 1600529672-10243-1-git-send-email-mjrosato@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Retrieve zPCI hardware information from VFIO | expand |
On Sat, 19 Sep 2020 11:34:28 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > From: Pierre Morel <pmorel@linux.ibm.com> > > To have a clean separation between s390-pci-bus.h and s390-pci-inst.h > headers we export the PCI CLP instructions in a dedicated header. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.h | 1 + > hw/s390x/s390-pci-clp.h | 211 +++++++++++++++++++++++++++++++++++++++++++++++ > hw/s390x/s390-pci-inst.h | 196 ------------------------------------------- > 3 files changed, 212 insertions(+), 196 deletions(-) > create mode 100644 hw/s390x/s390-pci-clp.h Looks sane; but I wonder whether we should move the stuff under include/hw/s390x/.
On Sat, 19 Sep 2020 11:34:29 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > From: Pierre Morel <pmorel@linux.ibm.com> > > We use a S390PCIGroup structure to hold the information related to a > zPCI Function group. > > This allows us to be ready to support multiple groups and to retrieve > the group information from the host. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > --- > hw/s390x/s390-pci-bus.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > hw/s390x/s390-pci-bus.h | 10 ++++++++++ > hw/s390x/s390-pci-inst.c | 22 +++++++++++++--------- > 3 files changed, 65 insertions(+), 9 deletions(-) > > diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c > index 92146a2..3015d86 100644 > --- a/hw/s390x/s390-pci-bus.c > +++ b/hw/s390x/s390-pci-bus.c > @@ -737,6 +737,46 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn) > object_unref(OBJECT(iommu)); > } > > +static S390PCIGroup *s390_grp_create(int ug) I think you made the identifiers a bit too compact :) s390_group_create() is not that long, and I have no idea what the 'ug' (ugh :) parameter is supposed to mean. > +{ > + S390PCIGroup *grp; group? > + S390pciState *s = s390_get_phb(); > + > + grp = g_new0(S390PCIGroup, 1); > + grp->ug = ug; > + QTAILQ_INSERT_TAIL(&s->zpci_grps, grp, link); zpci_groups? I think you get the idea :) > + return grp; > +} (...) No objection to the patch in general.
On 9/25/20 5:17 AM, Cornelia Huck wrote: > On Sat, 19 Sep 2020 11:34:28 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> From: Pierre Morel <pmorel@linux.ibm.com> >> >> To have a clean separation between s390-pci-bus.h and s390-pci-inst.h >> headers we export the PCI CLP instructions in a dedicated header. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> hw/s390x/s390-pci-bus.h | 1 + >> hw/s390x/s390-pci-clp.h | 211 +++++++++++++++++++++++++++++++++++++++++++++++ >> hw/s390x/s390-pci-inst.h | 196 ------------------------------------------- >> 3 files changed, 212 insertions(+), 196 deletions(-) >> create mode 100644 hw/s390x/s390-pci-clp.h > > Looks sane; but I wonder whether we should move the stuff under > include/hw/s390x/. > Probably. I'd be fine with creating this file under include, but if we're going to do that we should plan to move the other s390-pci* ones too. For this patchset, I can change this patch to put the new header in include/hw/s390x, easy enough. I'll plan to do a separate cleanup patchset to move s390-pci-bus.h and s390-pci-inst.h. How would you like me to handle s390-pci-vfio.h (this is a new file added by both this patch set and 's390x/pci: Accomodate vfio DMA limiting') -- It seems likely that the latter patch set will merge first, so my thought would be to avoid a cleanup on this one and just re-send 's390x/pci: Accomodate vfio DMA limiting' once the kernel part hits mainline (it's currently in linux-next via Alex) with s390-pci-vfio.h also created in include/hw/s390x (and I guess the MAINTAINERS hit for it too). Sound OK?
On 9/25/20 5:41 AM, Cornelia Huck wrote: > On Sat, 19 Sep 2020 11:34:29 -0400 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> From: Pierre Morel <pmorel@linux.ibm.com> >> >> We use a S390PCIGroup structure to hold the information related to a >> zPCI Function group. >> >> This allows us to be ready to support multiple groups and to retrieve >> the group information from the host. >> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> hw/s390x/s390-pci-bus.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> hw/s390x/s390-pci-bus.h | 10 ++++++++++ >> hw/s390x/s390-pci-inst.c | 22 +++++++++++++--------- >> 3 files changed, 65 insertions(+), 9 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c >> index 92146a2..3015d86 100644 >> --- a/hw/s390x/s390-pci-bus.c >> +++ b/hw/s390x/s390-pci-bus.c >> @@ -737,6 +737,46 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn) >> object_unref(OBJECT(iommu)); >> } >> >> +static S390PCIGroup *s390_grp_create(int ug) > > I think you made the identifiers a bit too compact :) > s390_group_create() is not that long, and I have no idea what the 'ug' > (ugh :) parameter is supposed to mean. > Ha :) Message received, something like s390_group_create(int id) is probably more appropriate. >> +{ >> + S390PCIGroup *grp; > > group? > >> + S390pciState *s = s390_get_phb(); >> + >> + grp = g_new0(S390PCIGroup, 1); >> + grp->ug = ug; >> + QTAILQ_INSERT_TAIL(&s->zpci_grps, grp, link); > > zpci_groups? I think you get the idea :) > Yep, thanks! >> + return grp; >> +} > > (...) > > No objection to the patch in general. >
On Fri, 25 Sep 2020 10:10:12 -0400 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 9/25/20 5:17 AM, Cornelia Huck wrote: > > On Sat, 19 Sep 2020 11:34:28 -0400 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> From: Pierre Morel <pmorel@linux.ibm.com> > >> > >> To have a clean separation between s390-pci-bus.h and s390-pci-inst.h > >> headers we export the PCI CLP instructions in a dedicated header. > >> > >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >> --- > >> hw/s390x/s390-pci-bus.h | 1 + > >> hw/s390x/s390-pci-clp.h | 211 +++++++++++++++++++++++++++++++++++++++++++++++ > >> hw/s390x/s390-pci-inst.h | 196 ------------------------------------------- > >> 3 files changed, 212 insertions(+), 196 deletions(-) > >> create mode 100644 hw/s390x/s390-pci-clp.h > > > > Looks sane; but I wonder whether we should move the stuff under > > include/hw/s390x/. > > > > Probably. I'd be fine with creating this file under include, but if > we're going to do that we should plan to move the other s390-pci* ones > too. For this patchset, I can change this patch to put the new header > in include/hw/s390x, easy enough. > > I'll plan to do a separate cleanup patchset to move s390-pci-bus.h and > s390-pci-inst.h. > > How would you like me to handle s390-pci-vfio.h (this is a new file > added by both this patch set and 's390x/pci: Accomodate vfio DMA > limiting') -- It seems likely that the latter patch set will merge > first, so my thought would be to avoid a cleanup on this one and just > re-send 's390x/pci: Accomodate vfio DMA limiting' once the kernel part > hits mainline (it's currently in linux-next via Alex) with > s390-pci-vfio.h also created in include/hw/s390x (and I guess the > MAINTAINERS hit for it too). Sound OK? Yes, I guess that would be best.