On Wed, 29 Mar 2017 12:41:01 +1100 Alexey Kardashevskiy <[email protected]> wrote:
> On 29/03/17 04:48, Alex Williamson wrote: > > On Tue, 28 Mar 2017 20:05:28 +1100 > > Alexey Kardashevskiy <[email protected]> wrote: > > > >> Signed-off-by: Alexey Kardashevskiy <[email protected]> > >> --- > >> 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) > > > >
