[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] spapr_pci: Move DMA w
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] spapr_pci: Move DMA window enablement to a helper |
Date: |
Tue, 15 Mar 2016 16:30:48 +1100 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> >>---
> >> 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);
> >> }
> >
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH qemu v13 13/16] spapr_iommu: Remove need_vfio flag from sPAPRTCETable, Alexey Kardashevskiy, 2016/03/01
[Qemu-devel] [PATCH qemu v13 08/16] memory: Add reporting of supported page sizes, Alexey Kardashevskiy, 2016/03/01