| Message ID | 1456823441-46757-3-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Tue, Mar 01, 2016 at 08:10:27PM +1100, Alexey Kardashevskiy wrote: > We are going to have multiple DMA windows soon so let's start preparing. > > This adds a new helper to create a DMA window and makes use of it in > sPAPRPHBState::realize(). > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 13 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 3d1145e..248f20a 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > return buf; > } > > +static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, > + uint32_t liobn, uint32_t page_shift, > + uint64_t window_addr, > + uint64_t window_size) > +{ > + sPAPRTCETable *tcet; > + uint32_t nb_table = window_size >> page_shift; > + > + if (!nb_table) { > + return -1; > + } The caller shouldn't do this, so this probably makes more sense as an assert() than an error return. > + > + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, > + page_shift, nb_table, false); > + if (!tcet) { > + return -1; > + } Since you're adding error reporting, you might as well make it via the error API instead of a return code. That way if we wasnt to add more detailed error API reporting to spapr_tce_new_table() in future, there's less to change. > + > + memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > + spapr_tce_get_iommu(tcet)); > + return 0; > +} > + > /* Macros to operate with address in OF binding to PCI */ > #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) > #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ > @@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > int i; > PCIBus *bus; > uint64_t msi_window_size = 4096; > - sPAPRTCETable *tcet; > - uint32_t nb_table; > > if (sphb->index != (uint32_t)-1) { > hwaddr windows_base; > @@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > } > } > > - nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; > - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, > - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); > - if (!tcet) { > - error_setg(errp, "Unable to create TCE table for %s", > - sphb->dtbusname); > - return; > - } > - > /* Register default 32bit DMA window */ > - memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr, > - spapr_tce_get_iommu(tcet)); > + if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, > + sphb->dma_win_addr, sphb->dma_win_size)) { > + error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); > + } > > sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); > }
On 03/03/2016 12:40 PM, David Gibson wrote: > On Tue, Mar 01, 2016 at 08:10:27PM +1100, Alexey Kardashevskiy wrote: >> We are going to have multiple DMA windows soon so let's start preparing. >> >> This adds a new helper to create a DMA window and makes use of it in >> sPAPRPHBState::realize(). >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++------------- >> 1 file changed, 27 insertions(+), 13 deletions(-) >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index 3d1145e..248f20a 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) >> return buf; >> } >> >> +static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, >> + uint32_t liobn, uint32_t page_shift, >> + uint64_t window_addr, >> + uint64_t window_size) >> +{ >> + sPAPRTCETable *tcet; >> + uint32_t nb_table = window_size >> page_shift; >> + >> + if (!nb_table) { >> + return -1; >> + } > > The caller shouldn't do this, so this probably makes more sense as an > assert() than an error return. @dma_win_size is a PHB property so the cli can set it to zero - where is it supposed to fail? When DMA won't work? This will be not really obvious for the user. Check dma_win_size==0 where we check mem_win_addr/...? > >> + >> + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, >> + page_shift, nb_table, false); >> + if (!tcet) { >> + return -1; >> + } > > Since you're adding error reporting, you might as well make it via the > error API instead of a return code. That way if we wasnt to add more > detailed error API reporting to spapr_tce_new_table() in future, > there's less to change. Well, the table allocation is the only real thing which may fail there and spapr_phb_realize() does not pass Error to the callers so spapr_phb_dma_window_enable() would be the first one to propagate an error and it just seems a bit over engineered. Should I still do that? >> + >> + memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, >> + spapr_tce_get_iommu(tcet)); >> + return 0; >> +} >> + >> /* Macros to operate with address in OF binding to PCI */ >> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) >> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ >> @@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> int i; >> PCIBus *bus; >> uint64_t msi_window_size = 4096; >> - sPAPRTCETable *tcet; >> - uint32_t nb_table; >> >> if (sphb->index != (uint32_t)-1) { >> hwaddr windows_base; >> @@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> } >> >> - nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; >> - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, >> - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); >> - if (!tcet) { >> - error_setg(errp, "Unable to create TCE table for %s", >> - sphb->dtbusname); >> - return; >> - } >> - >> /* Register default 32bit DMA window */ >> - memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr, >> - spapr_tce_get_iommu(tcet)); >> + if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, >> + sphb->dma_win_addr, sphb->dma_win_size)) { >> + error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); >> + } >> >> sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> } >
On Thu, Mar 10, 2016 at 04:47:04PM +1100, Alexey Kardashevskiy wrote: > On 03/03/2016 12:40 PM, David Gibson wrote: > >On Tue, Mar 01, 2016 at 08:10:27PM +1100, Alexey Kardashevskiy wrote: > >>We are going to have multiple DMA windows soon so let's start preparing. > >> > >>This adds a new helper to create a DMA window and makes use of it in > >>sPAPRPHBState::realize(). > >> > >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >>--- > >> hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++------------- > >> 1 file changed, 27 insertions(+), 13 deletions(-) > >> > >>diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > >>index 3d1145e..248f20a 100644 > >>--- a/hw/ppc/spapr_pci.c > >>+++ b/hw/ppc/spapr_pci.c > >>@@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) > >> return buf; > >> } > >> > >>+static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, > >>+ uint32_t liobn, uint32_t page_shift, > >>+ uint64_t window_addr, > >>+ uint64_t window_size) > >>+{ > >>+ sPAPRTCETable *tcet; > >>+ uint32_t nb_table = window_size >> page_shift; > >>+ > >>+ if (!nb_table) { > >>+ return -1; > >>+ } > > > >The caller shouldn't do this, so this probably makes more sense as an > >assert() than an error return. > > > @dma_win_size is a PHB property so the cli can set it to zero - where is it > supposed to fail? When DMA won't work? This will be not really obvious for > the user. Check dma_win_size==0 where we check mem_win_addr/...? Ah.. good point. It could be checked in the caller, but it doesn't make a lot of sense to. > >>+ > >>+ tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, > >>+ page_shift, nb_table, false); > >>+ if (!tcet) { > >>+ return -1; > >>+ } > > > >Since you're adding error reporting, you might as well make it via the > >error API instead of a return code. That way if we wasnt to add more > >detailed error API reporting to spapr_tce_new_table() in future, > >there's less to change. > > Well, the table allocation is the only real thing which may fail there and > spapr_phb_realize() does not pass Error to the callers ?? I'm not sure what you mean by that. > so > spapr_phb_dma_window_enable() would be the first one to propagate an error > and it just seems a bit over engineered. Should I still do that? Yes. You can argue it's overengineered, but better we move towards overengineered in a consistent way, than continue to use a mishmash of error codes and the error api. > > > >>+ > >>+ memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > >>+ spapr_tce_get_iommu(tcet)); > >>+ return 0; > >>+} > >>+ > >> /* Macros to operate with address in OF binding to PCI */ > >> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) > >> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ > >>@@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > >> int i; > >> PCIBus *bus; > >> uint64_t msi_window_size = 4096; > >>- sPAPRTCETable *tcet; > >>- uint32_t nb_table; > >> > >> if (sphb->index != (uint32_t)-1) { > >> hwaddr windows_base; > >>@@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > >> } > >> } > >> > >>- nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; > >>- tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, > >>- 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); > >>- if (!tcet) { > >>- error_setg(errp, "Unable to create TCE table for %s", > >>- sphb->dtbusname); > >>- return; > >>- } > >>- > >> /* Register default 32bit DMA window */ > >>- memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr, > >>- spapr_tce_get_iommu(tcet)); > >>+ if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, > >>+ sphb->dma_win_addr, sphb->dma_win_size)) { > >>+ error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); > >>+ } > >> > >> sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); > >> } > > > >
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 3d1145e..248f20a 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev) return buf; } +static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, + uint32_t liobn, uint32_t page_shift, + uint64_t window_addr, + uint64_t window_size) +{ + sPAPRTCETable *tcet; + uint32_t nb_table = window_size >> page_shift; + + if (!nb_table) { + return -1; + } + + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, + page_shift, nb_table, false); + if (!tcet) { + return -1; + } + + memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, + spapr_tce_get_iommu(tcet)); + return 0; +} + /* Macros to operate with address in OF binding to PCI */ #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ @@ -1228,8 +1251,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) int i; PCIBus *bus; uint64_t msi_window_size = 4096; - sPAPRTCETable *tcet; - uint32_t nb_table; if (sphb->index != (uint32_t)-1) { hwaddr windows_base; @@ -1381,18 +1402,11 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) } } - nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); - if (!tcet) { - error_setg(errp, "Unable to create TCE table for %s", - sphb->dtbusname); - return; - } - /* Register default 32bit DMA window */ - memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr, - spapr_tce_get_iommu(tcet)); + if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, + sphb->dma_win_addr, sphb->dma_win_size)) { + error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); + } sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); }
We are going to have multiple DMA windows soon so let's start preparing. This adds a new helper to create a DMA window and makes use of it in sPAPRPHBState::realize(). Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)