| Message ID | 20170328090530.20052-2-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Tue, 28 Mar 2017 20:05:28 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > include/exec/memory.h | 2 ++ > hw/ppc/spapr_iommu.c | 8 ++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e39256ad03..925c10b35b 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { > void (*notify_flag_changed)(MemoryRegion *iommu, > IOMMUNotifierFlag old_flags, > IOMMUNotifierFlag new_flags); > + /* Returns a kernel fd for IOMMU */ > + int (*get_fd)(MemoryRegion *iommu); What if we used this as a prototype: int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); And then we defined: typedef enum { SPAPR_IOMMU_TABLE_FD = 0, } IOMMUFdType; Such that you're actually asking the IOMMUOps for a specific type of FD and it either has it or not, so the caller doesn't need to assume what it is they get back. Furthermore, add: int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr) { assert(memory_region_is_iommu(mr)); if (mr->iommu_ops && mr->iommu_ops->get_fd) { return mr->iommu_ops->get_fd(type, mr); } return -1; } > }; > This should be two patches, patch 1 above, patch 2 below > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 9e30e148d6..b61c8f053e 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > } > } > > +static int spapr_tce_get_fd(MemoryRegion *iommu) > +{ > + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); > + > + return tcet->fd; This would then be: return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1; > +} > + > static int spapr_tce_table_post_load(void *opaque, int version_id) > { > sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); > @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { > .translate = spapr_tce_translate_iommu, > .get_min_page_size = spapr_tce_get_min_page_size, > .notify_flag_changed = spapr_tce_notify_flag_changed, > + .get_fd = spapr_tce_get_fd, > }; > > static int spapr_tce_table_realize(DeviceState *dev)
On 29/03/17 04:48, Alex Williamson wrote: > On Tue, 28 Mar 2017 20:05:28 +1100 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> include/exec/memory.h | 2 ++ >> hw/ppc/spapr_iommu.c | 8 ++++++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index e39256ad03..925c10b35b 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { >> void (*notify_flag_changed)(MemoryRegion *iommu, >> IOMMUNotifierFlag old_flags, >> IOMMUNotifierFlag new_flags); >> + /* Returns a kernel fd for IOMMU */ >> + int (*get_fd)(MemoryRegion *iommu); > > What if we used this as a prototype: > > int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); > > And then we defined: > > typedef enum { > SPAPR_IOMMU_TABLE_FD = 0, > } IOMMUFdType; Where do I put this enum definition? include/exec/memory.h? It does not have any mention of any platform yet... I could pass char* instead of IOMMUFdType (and pass there something like TYPE_SPAPR_TCE_TABLE), would it be any better? > > Such that you're actually asking the IOMMUOps for a specific type of FD > and it either has it or not, so the caller doesn't need to assume what > it is they get back. > > Furthermore, add: > > int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr) > { > assert(memory_region_is_iommu(mr)); > > if (mr->iommu_ops && mr->iommu_ops->get_fd) { > return mr->iommu_ops->get_fd(type, mr); > } > > return -1; > } > >> }; >> > > This should be two patches, patch 1 above, patch 2 below > >> typedef struct CoalescedMemoryRange CoalescedMemoryRange; >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >> index 9e30e148d6..b61c8f053e 100644 >> --- a/hw/ppc/spapr_iommu.c >> +++ b/hw/ppc/spapr_iommu.c >> @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, >> } >> } >> >> +static int spapr_tce_get_fd(MemoryRegion *iommu) >> +{ >> + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); >> + >> + return tcet->fd; > > > This would then be: > > return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1; > >> +} >> + >> static int spapr_tce_table_post_load(void *opaque, int version_id) >> { >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); >> @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { >> .translate = spapr_tce_translate_iommu, >> .get_min_page_size = spapr_tce_get_min_page_size, >> .notify_flag_changed = spapr_tce_notify_flag_changed, >> + .get_fd = spapr_tce_get_fd, >> }; >> >> static int spapr_tce_table_realize(DeviceState *dev) >
On Wed, 29 Mar 2017 12:41:01 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 29/03/17 04:48, Alex Williamson wrote: > > On Tue, 28 Mar 2017 20:05:28 +1100 > > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >> --- > >> include/exec/memory.h | 2 ++ > >> hw/ppc/spapr_iommu.c | 8 ++++++++ > >> 2 files changed, 10 insertions(+) > >> > >> diff --git a/include/exec/memory.h b/include/exec/memory.h > >> index e39256ad03..925c10b35b 100644 > >> --- a/include/exec/memory.h > >> +++ b/include/exec/memory.h > >> @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { > >> void (*notify_flag_changed)(MemoryRegion *iommu, > >> IOMMUNotifierFlag old_flags, > >> IOMMUNotifierFlag new_flags); > >> + /* Returns a kernel fd for IOMMU */ > >> + int (*get_fd)(MemoryRegion *iommu); > > > > What if we used this as a prototype: > > > > int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); > > > > And then we defined: > > > > typedef enum { > > SPAPR_IOMMU_TABLE_FD = 0, > > } IOMMUFdType; > > > Where do I put this enum definition? include/exec/memory.h? It does not > have any mention of any platform yet... I would assume memory.h, yes. It seems like the enum is just an abstraction, what does "get fd" mean generically to an IOMMU MemoryRegion? How can anyone else ever implement that callback if the initial user is assuming that the returned fd is a specific, yet unspecified type. If the API is "give me an fd for this type of thing" then the IOMMU driver can either provide it or indicate that type is not supported. There's really no platform knowledge at the memory API level, it's just a type of thing that means something to the driver providing the MemoryRegionIOMMUOps and the caller. > I could pass char* instead of IOMMUFdType (and pass there something like > TYPE_SPAPR_TCE_TABLE), would it be any better? Gack, an enum seems so much cleaner than requiring a strcmp. Thanks, Alex > > Such that you're actually asking the IOMMUOps for a specific type of FD > > and it either has it or not, so the caller doesn't need to assume what > > it is they get back. > > > > Furthermore, add: > > > > int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr) > > { > > assert(memory_region_is_iommu(mr)); > > > > if (mr->iommu_ops && mr->iommu_ops->get_fd) { > > return mr->iommu_ops->get_fd(type, mr); > > } > > > > return -1; > > } > > > >> }; > >> > > > > This should be two patches, patch 1 above, patch 2 below > > > >> typedef struct CoalescedMemoryRange CoalescedMemoryRange; > >> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > >> index 9e30e148d6..b61c8f053e 100644 > >> --- a/hw/ppc/spapr_iommu.c > >> +++ b/hw/ppc/spapr_iommu.c > >> @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > >> } > >> } > >> > >> +static int spapr_tce_get_fd(MemoryRegion *iommu) > >> +{ > >> + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); > >> + > >> + return tcet->fd; > > > > > > This would then be: > > > > return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1; > > > >> +} > >> + > >> static int spapr_tce_table_post_load(void *opaque, int version_id) > >> { > >> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); > >> @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { > >> .translate = spapr_tce_translate_iommu, > >> .get_min_page_size = spapr_tce_get_min_page_size, > >> .notify_flag_changed = spapr_tce_notify_flag_changed, > >> + .get_fd = spapr_tce_get_fd, > >> }; > >> > >> static int spapr_tce_table_realize(DeviceState *dev) > > > >
On Tue, Mar 28, 2017 at 11:48:29AM -0600, Alex Williamson wrote: > On Tue, 28 Mar 2017 20:05:28 +1100 > Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > --- > > include/exec/memory.h | 2 ++ > > hw/ppc/spapr_iommu.c | 8 ++++++++ > > 2 files changed, 10 insertions(+) > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index e39256ad03..925c10b35b 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { > > void (*notify_flag_changed)(MemoryRegion *iommu, > > IOMMUNotifierFlag old_flags, > > IOMMUNotifierFlag new_flags); > > + /* Returns a kernel fd for IOMMU */ > > + int (*get_fd)(MemoryRegion *iommu); > > What if we used this as a prototype: > > int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); > > And then we defined: > > typedef enum { > SPAPR_IOMMU_TABLE_FD = 0, > } IOMMUFdType; Are we expecting any new types of fd? Maybe it would be simpler just to name this spapr_tce_fd() or something more specific, and only generalize if we really need it for another fd type. > > Such that you're actually asking the IOMMUOps for a specific type of FD > and it either has it or not, so the caller doesn't need to assume what > it is they get back. > > Furthermore, add: > > int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr) > { > assert(memory_region_is_iommu(mr)); > > if (mr->iommu_ops && mr->iommu_ops->get_fd) { > return mr->iommu_ops->get_fd(type, mr); > } > > return -1; > } > > > }; > > > > This should be two patches, patch 1 above, patch 2 below > > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > > index 9e30e148d6..b61c8f053e 100644 > > --- a/hw/ppc/spapr_iommu.c > > +++ b/hw/ppc/spapr_iommu.c > > @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > } > > } > > > > +static int spapr_tce_get_fd(MemoryRegion *iommu) > > +{ > > + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); > > + > > + return tcet->fd; > > > This would then be: > > return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1; > > > +} > > + > > static int spapr_tce_table_post_load(void *opaque, int version_id) > > { > > sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); > > @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { > > .translate = spapr_tce_translate_iommu, > > .get_min_page_size = spapr_tce_get_min_page_size, > > .notify_flag_changed = spapr_tce_notify_flag_changed, > > + .get_fd = spapr_tce_get_fd, > > }; > > > > static int spapr_tce_table_realize(DeviceState *dev) >
On 29/03/17 14:35, David Gibson wrote: > On Tue, Mar 28, 2017 at 11:48:29AM -0600, Alex Williamson wrote: >> On Tue, 28 Mar 2017 20:05:28 +1100 >> Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> include/exec/memory.h | 2 ++ >>> hw/ppc/spapr_iommu.c | 8 ++++++++ >>> 2 files changed, 10 insertions(+) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index e39256ad03..925c10b35b 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { >>> void (*notify_flag_changed)(MemoryRegion *iommu, >>> IOMMUNotifierFlag old_flags, >>> IOMMUNotifierFlag new_flags); >>> + /* Returns a kernel fd for IOMMU */ >>> + int (*get_fd)(MemoryRegion *iommu); >> >> What if we used this as a prototype: >> >> int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); >> >> And then we defined: >> >> typedef enum { >> SPAPR_IOMMU_TABLE_FD = 0, >> } IOMMUFdType; > > Are we expecting any new types of fd? Maybe it would be simpler just > to name this spapr_tce_fd() or something more specific, and only > generalize if we really need it for another fd type. So far we managed to keep VFIO and sPAPR-IOMMU relatively separate - they do not include each others headers and interact via memory regions and kernel uapi interface. The only direct connection between VFIO and sPAPR at all is vfio_eeh_as_ok/vfio_eeh_as_op which is rather workaround. I like this separation tbh. > >> >> Such that you're actually asking the IOMMUOps for a specific type of FD >> and it either has it or not, so the caller doesn't need to assume what >> it is they get back. >> >> Furthermore, add: >> >> int memory_region_iommu_get_fd(IOMMUFdType type, MemoryRegion *mr) >> { >> assert(memory_region_is_iommu(mr)); >> >> if (mr->iommu_ops && mr->iommu_ops->get_fd) { >> return mr->iommu_ops->get_fd(type, mr); >> } >> >> return -1; >> } >> >>> }; >>> >> >> This should be two patches, patch 1 above, patch 2 below >> >>> typedef struct CoalescedMemoryRange CoalescedMemoryRange; >>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >>> index 9e30e148d6..b61c8f053e 100644 >>> --- a/hw/ppc/spapr_iommu.c >>> +++ b/hw/ppc/spapr_iommu.c >>> @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, >>> } >>> } >>> >>> +static int spapr_tce_get_fd(MemoryRegion *iommu) >>> +{ >>> + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); >>> + >>> + return tcet->fd; >> >> >> This would then be: >> >> return type == SPAPR_IOMMU_TABLE_FD ? tcet->fd : -1; >> >>> +} >>> + >>> static int spapr_tce_table_post_load(void *opaque, int version_id) >>> { >>> sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); >>> @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { >>> .translate = spapr_tce_translate_iommu, >>> .get_min_page_size = spapr_tce_get_min_page_size, >>> .notify_flag_changed = spapr_tce_notify_flag_changed, >>> + .get_fd = spapr_tce_get_fd, >>> }; >>> >>> static int spapr_tce_table_realize(DeviceState *dev) >> >
On 29/03/2017 05:04, Alex Williamson wrote: >>> What if we used this as a prototype: >>> >>> int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); >>> >>> And then we defined: >>> >>> typedef enum { >>> SPAPR_IOMMU_TABLE_FD = 0, >>> } IOMMUFdType; >> >> Where do I put this enum definition? include/exec/memory.h? It does not >> have any mention of any platform yet... > > I would assume memory.h, yes. It seems like the enum is just an > abstraction, what does "get fd" mean generically to an IOMMU > MemoryRegion? How can anyone else ever implement that callback if the > initial user is assuming that the returned fd is a specific, yet > unspecified type. If the API is "give me an fd for this type of thing" > then the IOMMU driver can either provide it or indicate that type is not > supported. There's really no platform knowledge at the memory API > level, it's just a type of thing that means something to the driver > providing the MemoryRegionIOMMUOps and the caller. I think we should move to a QOM hierarchy like AbstractMemoryRegion MemoryRegion << adds MemoryRegionOps IOMMUMemoryRegion << adds MemoryRegionIOMMUOps sPAPR_IOMMUMemoryRegion << adds get_fd but for now I'm okay with Alex's proposal. Paolo
On Wed, Mar 29, 2017 at 10:04:47AM +0200, Paolo Bonzini wrote: > > > On 29/03/2017 05:04, Alex Williamson wrote: > >>> What if we used this as a prototype: > >>> > >>> int (*get_fd)(IOMMUFdType type, MemoryRegion *iommu); > >>> > >>> And then we defined: > >>> > >>> typedef enum { > >>> SPAPR_IOMMU_TABLE_FD = 0, > >>> } IOMMUFdType; > >> > >> Where do I put this enum definition? include/exec/memory.h? It does not > >> have any mention of any platform yet... > > > > I would assume memory.h, yes. It seems like the enum is just an > > abstraction, what does "get fd" mean generically to an IOMMU > > MemoryRegion? How can anyone else ever implement that callback if the > > initial user is assuming that the returned fd is a specific, yet > > unspecified type. If the API is "give me an fd for this type of thing" > > then the IOMMU driver can either provide it or indicate that type is not > > supported. There's really no platform knowledge at the memory API > > level, it's just a type of thing that means something to the driver > > providing the MemoryRegionIOMMUOps and the caller. > > I think we should move to a QOM hierarchy like > > AbstractMemoryRegion > MemoryRegion << adds MemoryRegionOps > IOMMUMemoryRegion << adds MemoryRegionIOMMUOps > sPAPR_IOMMUMemoryRegion << adds get_fd Right, this makes more sense to me than the enum as well. > but for now I'm okay with Alex's proposal.
diff --git a/include/exec/memory.h b/include/exec/memory.h index e39256ad03..925c10b35b 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -174,6 +174,8 @@ struct MemoryRegionIOMMUOps { void (*notify_flag_changed)(MemoryRegion *iommu, IOMMUNotifierFlag old_flags, IOMMUNotifierFlag new_flags); + /* Returns a kernel fd for IOMMU */ + int (*get_fd)(MemoryRegion *iommu); }; typedef struct CoalescedMemoryRange CoalescedMemoryRange; diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 9e30e148d6..b61c8f053e 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -170,6 +170,13 @@ static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, } } +static int spapr_tce_get_fd(MemoryRegion *iommu) +{ + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); + + return tcet->fd; +} + static int spapr_tce_table_post_load(void *opaque, int version_id) { sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); @@ -251,6 +258,7 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = { .translate = spapr_tce_translate_iommu, .get_min_page_size = spapr_tce_get_min_page_size, .notify_flag_changed = spapr_tce_notify_flag_changed, + .get_fd = spapr_tce_get_fd, }; static int spapr_tce_table_realize(DeviceState *dev)
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- include/exec/memory.h | 2 ++ hw/ppc/spapr_iommu.c | 8 ++++++++ 2 files changed, 10 insertions(+)