| Message ID | 1404251378-5242-6-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote: > For e500 our approach to supporting dynamically spawned sysbus devices is to > create a simple bus from the guest's point of view within which we map those > devices dynamically. > > We allocate memory regions always within the "platform" hole in address > space and map IRQs to predetermined IRQ lines that are reserved for platform > device usage. > > This maps really nicely into device tree logic, so we can just tell the > guest about our virtual simple bus in device tree as well. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/ppc/e500.h | 1 + > hw/ppc/e500plat.c | 1 + > 3 files changed, 253 insertions(+) > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c > index bb2e75f..bf704b0 100644 > --- a/hw/ppc/e500.c > +++ b/hw/ppc/e500.c > @@ -36,6 +36,7 @@ > #include "exec/address-spaces.h" > #include "qemu/host-utils.h" > #include "hw/pci-host/ppce500.h" > +#include "qemu/error-report.h" > > #define EPAPR_MAGIC (0x45504150) > #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb" > @@ -47,6 +48,14 @@ > > #define RAM_SIZES_ALIGN (64UL << 20) > > +#define E500_PLATFORM_BASE 0xF0000000ULL Should the IRQ and address range be parameterized? Even if the platform bus is going to be restricted to only e500plat, it seems like it would be good to keep all assumptions that are e500plat-specific inside the e500plat file. Plus, let's please not hardcode any more addresses that are going to be a problem for giving guests a large amount of RAM (yes, CCSRBAR is also blocking that, but that has a TODO to parameterize). How about 0xf00000000ULL? Unless of course we're emulating an e500v1, which doesn't support 36-bit physical addressing. Parameterization would help there as well. > +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */ > +#define E500_PLATFORM_PAGE_SHIFT 12 > +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \ > + E500_PLATFORM_PAGE_SHIFT) > +#define E500_PLATFORM_FIRST_IRQ 5 > +#define E500_PLATFORM_NUM_IRQS 10 What is the "hole"? If that's meant to be the size to go along with E500_PLATFORM_BASE, that seems like odd terminology. > + > /* TODO: parameterize */ > #define MPC8544_CCSRBAR_BASE 0xE0000000ULL > #define MPC8544_CCSRBAR_SIZE 0x00100000ULL > @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, > } > } > > +typedef struct PlatformDevtreeData { > + void *fdt; > + const char *mpic; > + int irq_start; > + const char *node; > + int id; > +} PlatformDevtreeData; What is id? How does irq_start work? > +static int sysbus_device_create_devtree(Object *obj, void *opaque) > +{ > + PlatformDevtreeData *data = opaque; > + Object *dev; > + SysBusDevice *sbdev; > + bool matched = false; > + > + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); > + sbdev = (SysBusDevice *)dev; > + > + if (!sbdev) { > + /* Container, traverse it for children */ > + return object_child_foreach(obj, sysbus_device_create_devtree, data); > + } > + > + if (matched) { > + data->id++; > + } else { > + error_report("Device %s is not supported by this machine yet.", > + qdev_fw_name(DEVICE(dev))); > + exit(1); > + } > + > + return 0; > +} It's not clear to me how this function is creating a device tree node. > + > +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr, > + const char *mpic, int irq_start, > + int nr_irqs) > +{ > + const char platcomp[] = "qemu,platform\0simple-bus"; > + PlatformDevtreeData data; > + Object *container; > + > + /* Create a /platform node that we can put all devices into */ > + > + qemu_fdt_add_subnode(fdt, node); > + qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp)); > + qemu_fdt_setprop_string(fdt, node, "device_type", "platform"); Where did this device_type come from? device_type is deprecated and new uses should not be introduced. > diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h > index 08b25fa..3a588ed 100644 > --- a/hw/ppc/e500.h > +++ b/hw/ppc/e500.h > @@ -11,6 +11,7 @@ typedef struct PPCE500Params { > void (*fixup_devtree)(struct PPCE500Params *params, void *fdt); > > int mpic_version; > + bool has_platform; > } PPCE500Params; It would be clearer to refer to this as "platform bus", "platform devices", or similar rather than just "platform" -- the latter is confusing relative to existing use of the word "platform", such as e500plat.c. -Scott
On 02.07.14 00:50, Scott Wood wrote: > On Tue, 2014-07-01 at 23:49 +0200, Alexander Graf wrote: >> For e500 our approach to supporting dynamically spawned sysbus devices is to >> create a simple bus from the guest's point of view within which we map those >> devices dynamically. >> >> We allocate memory regions always within the "platform" hole in address >> space and map IRQs to predetermined IRQ lines that are reserved for platform >> device usage. >> >> This maps really nicely into device tree logic, so we can just tell the >> guest about our virtual simple bus in device tree as well. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/ppc/e500.h | 1 + >> hw/ppc/e500plat.c | 1 + >> 3 files changed, 253 insertions(+) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index bb2e75f..bf704b0 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -36,6 +36,7 @@ >> #include "exec/address-spaces.h" >> #include "qemu/host-utils.h" >> #include "hw/pci-host/ppce500.h" >> +#include "qemu/error-report.h" >> >> #define EPAPR_MAGIC (0x45504150) >> #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb" >> @@ -47,6 +48,14 @@ >> >> #define RAM_SIZES_ALIGN (64UL << 20) >> >> +#define E500_PLATFORM_BASE 0xF0000000ULL > Should the IRQ and address range be parameterized? Even if the platform > bus is going to be restricted to only e500plat, it seems like it would > be good to keep all assumptions that are e500plat-specific inside the > e500plat file. Good idea. The only thing I'll leave here is the page_shift. The fact that we only allocate in 4k chunks is inherently implementation specific. > Plus, let's please not hardcode any more addresses that are going to be > a problem for giving guests a large amount of RAM (yes, CCSRBAR is also > blocking that, but that has a TODO to parameterize). How about > 0xf00000000ULL? Unless of course we're emulating an e500v1, which > doesn't support 36-bit physical addressing. Parameterization would help > there as well. I don't think we have to worry about e500v1. I'll change it :). > >> +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */ >> +#define E500_PLATFORM_PAGE_SHIFT 12 >> +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \ >> + E500_PLATFORM_PAGE_SHIFT) >> +#define E500_PLATFORM_FIRST_IRQ 5 >> +#define E500_PLATFORM_NUM_IRQS 10 > What is the "hole"? If that's meant to be the size to go along with > E500_PLATFORM_BASE, that seems like odd terminology. True - renamed to "size". > >> + >> /* TODO: parameterize */ >> #define MPC8544_CCSRBAR_BASE 0xE0000000ULL >> #define MPC8544_CCSRBAR_SIZE 0x00100000ULL >> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, >> } >> } >> >> +typedef struct PlatformDevtreeData { >> + void *fdt; >> + const char *mpic; >> + int irq_start; >> + const char *node; >> + int id; >> +} PlatformDevtreeData; > What is id? How does irq_start work? "id" is just a linear counter over all devices in the platform bus so that if you need to have a unique identifier, you can have one. "irq_start" is the offset of the first mpic irq that's connected to the platform bus. > >> +static int sysbus_device_create_devtree(Object *obj, void *opaque) >> +{ >> + PlatformDevtreeData *data = opaque; >> + Object *dev; >> + SysBusDevice *sbdev; >> + bool matched = false; >> + >> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); >> + sbdev = (SysBusDevice *)dev; >> + >> + if (!sbdev) { >> + /* Container, traverse it for children */ >> + return object_child_foreach(obj, sysbus_device_create_devtree, data); >> + } >> + >> + if (matched) { >> + data->id++; >> + } else { >> + error_report("Device %s is not supported by this machine yet.", >> + qdev_fw_name(DEVICE(dev))); >> + exit(1); >> + } >> + >> + return 0; >> +} > It's not clear to me how this function is creating a device tree node. It's not yet - it's only the stub that allows to plug in specific device code that then generates device tree nodes :). > >> + >> +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr, >> + const char *mpic, int irq_start, >> + int nr_irqs) >> +{ >> + const char platcomp[] = "qemu,platform\0simple-bus"; >> + PlatformDevtreeData data; >> + Object *container; >> + >> + /* Create a /platform node that we can put all devices into */ >> + >> + qemu_fdt_add_subnode(fdt, node); >> + qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp)); >> + qemu_fdt_setprop_string(fdt, node, "device_type", "platform"); > Where did this device_type come from? > > device_type is deprecated and new uses should not be introduced. Fair enough, will remove it :) > >> diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h >> index 08b25fa..3a588ed 100644 >> --- a/hw/ppc/e500.h >> +++ b/hw/ppc/e500.h >> @@ -11,6 +11,7 @@ typedef struct PPCE500Params { >> void (*fixup_devtree)(struct PPCE500Params *params, void *fdt); >> >> int mpic_version; >> + bool has_platform; >> } PPCE500Params; > It would be clearer to refer to this as "platform bus", "platform > devices", or similar rather than just "platform" -- the latter is > confusing relative to existing use of the word "platform", such as > e500plat.c. Renamed to "platform_bus" :). In fact, I'll go through the file with a small sweeper and try to make sure we're reasonably consistent in our naming. Alex
On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: > On 02.07.14 00:50, Scott Wood wrote: > > Plus, let's please not hardcode any more addresses that are going to be > > a problem for giving guests a large amount of RAM (yes, CCSRBAR is also > > blocking that, but that has a TODO to parameterize). How about > > 0xf00000000ULL? Unless of course we're emulating an e500v1, which > > doesn't support 36-bit physical addressing. Parameterization would help > > there as well. > > I don't think we have to worry about e500v1. I'll change it :). We theoretically support it elsewhere... Once parameterized, it shouldn't be hard to base the address for this, CCSRBAR, and similar things on whether MAS7 is supported. > >> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, > >> } > >> } > >> > >> +typedef struct PlatformDevtreeData { > >> + void *fdt; > >> + const char *mpic; > >> + int irq_start; > >> + const char *node; > >> + int id; > >> +} PlatformDevtreeData; > > What is id? How does irq_start work? > > "id" is just a linear counter over all devices in the platform bus so > that if you need to have a unique identifier, you can have one. > > "irq_start" is the offset of the first mpic irq that's connected to the > platform bus. OK, but why is that here but no irq_end, and no address range? How do allocations from the irq range happen? > >> +static int sysbus_device_create_devtree(Object *obj, void *opaque) > >> +{ > >> + PlatformDevtreeData *data = opaque; > >> + Object *dev; > >> + SysBusDevice *sbdev; > >> + bool matched = false; > >> + > >> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); > >> + sbdev = (SysBusDevice *)dev; > >> + > >> + if (!sbdev) { > >> + /* Container, traverse it for children */ > >> + return object_child_foreach(obj, sysbus_device_create_devtree, data); > >> + } > >> + > >> + if (matched) { > >> + data->id++; > >> + } else { > >> + error_report("Device %s is not supported by this machine yet.", > >> + qdev_fw_name(DEVICE(dev))); > >> + exit(1); > >> + } > >> + > >> + return 0; > >> +} > > It's not clear to me how this function is creating a device tree node. > > It's not yet - it's only the stub that allows to plug in specific device > code that then generates device tree nodes :). How does the plugging in work? It looks like all this does is increment id. -Scott
On 02.07.14 19:26, Scott Wood wrote: > On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: >> On 02.07.14 00:50, Scott Wood wrote: >>> Plus, let's please not hardcode any more addresses that are going to be >>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also >>> blocking that, but that has a TODO to parameterize). How about >>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which >>> doesn't support 36-bit physical addressing. Parameterization would help >>> there as well. >> I don't think we have to worry about e500v1. I'll change it :). > We theoretically support it elsewhere... Once parameterized, it > shouldn't be hard to base the address for this, CCSRBAR, and similar > things on whether MAS7 is supported. It gets parametrized in the machine file, CPU selection comes after machine selection. So parameterizing it doesn't really solve it. However, again, I don't think we have to worry about it. > >>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, >>>> } >>>> } >>>> >>>> +typedef struct PlatformDevtreeData { >>>> + void *fdt; >>>> + const char *mpic; >>>> + int irq_start; >>>> + const char *node; >>>> + int id; >>>> +} PlatformDevtreeData; >>> What is id? How does irq_start work? >> "id" is just a linear counter over all devices in the platform bus so >> that if you need to have a unique identifier, you can have one. >> >> "irq_start" is the offset of the first mpic irq that's connected to the >> platform bus. > OK, but why is that here but no irq_end, and no address range? How do > allocations from the irq range happen? There are 2 phases: 1) Device association with the machine 2) Device tree generation The allocation of IRQ ranges happens during the association phase. That phase also updates all the hints in the devices to reflect their current IRQ (and MMIO) mappings. The device tree generation phase only needs to read those bits then - and add the IRQ offset to get from the "platform bus IRQ range" to "MPIC IRQ range". > >>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque) >>>> +{ >>>> + PlatformDevtreeData *data = opaque; >>>> + Object *dev; >>>> + SysBusDevice *sbdev; >>>> + bool matched = false; >>>> + >>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); >>>> + sbdev = (SysBusDevice *)dev; >>>> + >>>> + if (!sbdev) { >>>> + /* Container, traverse it for children */ >>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data); >>>> + } >>>> + >>>> + if (matched) { >>>> + data->id++; >>>> + } else { >>>> + error_report("Device %s is not supported by this machine yet.", >>>> + qdev_fw_name(DEVICE(dev))); >>>> + exit(1); >>>> + } >>>> + >>>> + return 0; >>>> +} >>> It's not clear to me how this function is creating a device tree node. >> It's not yet - it's only the stub that allows to plug in specific device >> code that then generates device tree nodes :). > How does the plugging in work? > > It looks like all this does is increment id. I'm not sure I understand. The plugging in is different code :). This really only does increment an id. Maybe I'll just remove it if it confuses you? Alex
On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote: > On 02.07.14 19:26, Scott Wood wrote: > > On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: > >> On 02.07.14 00:50, Scott Wood wrote: > >>> Plus, let's please not hardcode any more addresses that are going to be > >>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also > >>> blocking that, but that has a TODO to parameterize). How about > >>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which > >>> doesn't support 36-bit physical addressing. Parameterization would help > >>> there as well. > >> I don't think we have to worry about e500v1. I'll change it :). > > We theoretically support it elsewhere... Once parameterized, it > > shouldn't be hard to base the address for this, CCSRBAR, and similar > > things on whether MAS7 is supported. > > It gets parametrized in the machine file, CPU selection comes after > machine selection. So parameterizing it doesn't really solve it. Why can't e500plat_init() look at args->cpu_model? Or the parameterization could take two sets of addresses, one for a 32-bit layout and one for a 36-bit layout. It might make sense to make this a user-settable parameter; some OSes might not be able to handle a 36-bit layout (or might not be as efficient at handling it) even on e500v2. Many of the e500v2 boards can be built for either a 32 or 36 bit address layout in U-Boot. > However, again, I don't think we have to worry about it. It's not a huge worry, but it'd be nice to not break it gratuitously. If we do break it we should explicitly disallow e500v1 with e500plat. > >>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, > >>>> } > >>>> } > >>>> > >>>> +typedef struct PlatformDevtreeData { > >>>> + void *fdt; > >>>> + const char *mpic; > >>>> + int irq_start; > >>>> + const char *node; > >>>> + int id; > >>>> +} PlatformDevtreeData; > >>> What is id? How does irq_start work? > >> "id" is just a linear counter over all devices in the platform bus so > >> that if you need to have a unique identifier, you can have one. > >> > >> "irq_start" is the offset of the first mpic irq that's connected to the > >> platform bus. > > OK, but why is that here but no irq_end, and no address range? How do > > allocations from the irq range happen? > > There are 2 phases: > > 1) Device association with the machine > 2) Device tree generation > > The allocation of IRQ ranges happens during the association phase. That > phase also updates all the hints in the devices to reflect their current > IRQ (and MMIO) mappings. The device tree generation phase only needs to > read those bits then - and add the IRQ offset to get from the "platform > bus IRQ range" to "MPIC IRQ range". I think the answer to my original question is that irqs are allocated based on zero because they go in an array, while memory regions are allocated with their actual addresses because they don't. > >>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque) > >>>> +{ > >>>> + PlatformDevtreeData *data = opaque; > >>>> + Object *dev; > >>>> + SysBusDevice *sbdev; > >>>> + bool matched = false; > >>>> + > >>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); > >>>> + sbdev = (SysBusDevice *)dev; > >>>> + > >>>> + if (!sbdev) { > >>>> + /* Container, traverse it for children */ > >>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data); > >>>> + } > >>>> + > >>>> + if (matched) { > >>>> + data->id++; > >>>> + } else { > >>>> + error_report("Device %s is not supported by this machine yet.", > >>>> + qdev_fw_name(DEVICE(dev))); > >>>> + exit(1); > >>>> + } > >>>> + > >>>> + return 0; > >>>> +} > >>> It's not clear to me how this function is creating a device tree node. > >> It's not yet - it's only the stub that allows to plug in specific device > >> code that then generates device tree nodes :). > > How does the plugging in work? > > > > It looks like all this does is increment id. > > I'm not sure I understand. The plugging in is different code :). This > really only does increment an id. Maybe I'll just remove it if it > confuses you? My confusion is that it is called sysbus_device_create_devtree(), not sysbus_device_alloc_id(). Am I missing some sort of virtual function mechanism here that would allow this function to be replaced? /me looks at patch 6/6 again Oh, you just add to this function in future patches. I was expecting something fancier given the QOM stuff and my misunderstanding about what file patch 6/6 was touching. :-) -Scott
On 02.07.14 19:52, Scott Wood wrote: > On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote: >> On 02.07.14 19:26, Scott Wood wrote: >>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: >>>> On 02.07.14 00:50, Scott Wood wrote: >>>>> Plus, let's please not hardcode any more addresses that are going to be >>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also >>>>> blocking that, but that has a TODO to parameterize). How about >>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which >>>>> doesn't support 36-bit physical addressing. Parameterization would help >>>>> there as well. >>>> I don't think we have to worry about e500v1. I'll change it :). >>> We theoretically support it elsewhere... Once parameterized, it >>> shouldn't be hard to base the address for this, CCSRBAR, and similar >>> things on whether MAS7 is supported. >> It gets parametrized in the machine file, CPU selection comes after >> machine selection. So parameterizing it doesn't really solve it. > Why can't e500plat_init() look at args->cpu_model? Or the > parameterization could take two sets of addresses, one for a 32-bit > layout and one for a 36-bit layout. It might make sense to make this a > user-settable parameter; some OSes might not be able to handle a 36-bit > layout (or might not be as efficient at handling it) even on e500v2. > Many of the e500v2 boards can be built for either a 32 or 36 bit address > layout in U-Boot. > >> However, again, I don't think we have to worry about it. > It's not a huge worry, but it'd be nice to not break it gratuitously. > If we do break it we should explicitly disallow e500v1 with e500plat. I'd prefer if we don't overparameterize - it'll just become a headache further down. Today we don't explicitly disallow anything anywhere - you could theoretically stick a G3 into e500plat. I don't see why we should start with heavy sanity checks now :). Plus, the machine works just fine today if you don't pass in -device eTSEC. It's not like we're moving all devices to the new "platform bus". > >>>>>> @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, >>>>>> } >>>>>> } >>>>>> >>>>>> +typedef struct PlatformDevtreeData { >>>>>> + void *fdt; >>>>>> + const char *mpic; >>>>>> + int irq_start; >>>>>> + const char *node; >>>>>> + int id; >>>>>> +} PlatformDevtreeData; >>>>> What is id? How does irq_start work? >>>> "id" is just a linear counter over all devices in the platform bus so >>>> that if you need to have a unique identifier, you can have one. >>>> >>>> "irq_start" is the offset of the first mpic irq that's connected to the >>>> platform bus. >>> OK, but why is that here but no irq_end, and no address range? How do >>> allocations from the irq range happen? >> There are 2 phases: >> >> 1) Device association with the machine >> 2) Device tree generation >> >> The allocation of IRQ ranges happens during the association phase. That >> phase also updates all the hints in the devices to reflect their current >> IRQ (and MMIO) mappings. The device tree generation phase only needs to >> read those bits then - and add the IRQ offset to get from the "platform >> bus IRQ range" to "MPIC IRQ range". > I think the answer to my original question is that irqs are allocated > based on zero because they go in an array, while memory regions are > allocated with their actual addresses because they don't. Memory regions are allocated based on zero as well, they get mapped into a subregion. From a device's point of view, the regions for MMIO and IRQs that it sees all start at 0 relative to the platform bus. > >>>>>> +static int sysbus_device_create_devtree(Object *obj, void *opaque) >>>>>> +{ >>>>>> + PlatformDevtreeData *data = opaque; >>>>>> + Object *dev; >>>>>> + SysBusDevice *sbdev; >>>>>> + bool matched = false; >>>>>> + >>>>>> + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); >>>>>> + sbdev = (SysBusDevice *)dev; >>>>>> + >>>>>> + if (!sbdev) { >>>>>> + /* Container, traverse it for children */ >>>>>> + return object_child_foreach(obj, sysbus_device_create_devtree, data); >>>>>> + } >>>>>> + >>>>>> + if (matched) { >>>>>> + data->id++; >>>>>> + } else { >>>>>> + error_report("Device %s is not supported by this machine yet.", >>>>>> + qdev_fw_name(DEVICE(dev))); >>>>>> + exit(1); >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>> It's not clear to me how this function is creating a device tree node. >>>> It's not yet - it's only the stub that allows to plug in specific device >>>> code that then generates device tree nodes :). >>> How does the plugging in work? >>> >>> It looks like all this does is increment id. >> I'm not sure I understand. The plugging in is different code :). This >> really only does increment an id. Maybe I'll just remove it if it >> confuses you? > My confusion is that it is called sysbus_device_create_devtree(), not > sysbus_device_alloc_id(). Am I missing some sort of virtual function > mechanism here that would allow this function to be replaced? I've removed the id bit - hope that makes it more obvious :). > > /me looks at patch 6/6 again > > Oh, you just add to this function in future patches. I was expecting > something fancier given the QOM stuff and my misunderstanding about what > file patch 6/6 was touching. :-) Heh, yeah, nothing impressively fancy :). Alex
On Wed, 2014-07-02 at 19:59 +0200, Alexander Graf wrote: > On 02.07.14 19:52, Scott Wood wrote: > > On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote: > >> On 02.07.14 19:26, Scott Wood wrote: > >>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: > >>>> On 02.07.14 00:50, Scott Wood wrote: > >>>>> Plus, let's please not hardcode any more addresses that are going to be > >>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also > >>>>> blocking that, but that has a TODO to parameterize). How about > >>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which > >>>>> doesn't support 36-bit physical addressing. Parameterization would help > >>>>> there as well. > >>>> I don't think we have to worry about e500v1. I'll change it :). > >>> We theoretically support it elsewhere... Once parameterized, it > >>> shouldn't be hard to base the address for this, CCSRBAR, and similar > >>> things on whether MAS7 is supported. > >> It gets parametrized in the machine file, CPU selection comes after > >> machine selection. So parameterizing it doesn't really solve it. > > Why can't e500plat_init() look at args->cpu_model? Or the > > parameterization could take two sets of addresses, one for a 32-bit > > layout and one for a 36-bit layout. It might make sense to make this a > > user-settable parameter; some OSes might not be able to handle a 36-bit > > layout (or might not be as efficient at handling it) even on e500v2. > > Many of the e500v2 boards can be built for either a 32 or 36 bit address > > layout in U-Boot. > > > >> However, again, I don't think we have to worry about it. > > It's not a huge worry, but it'd be nice to not break it gratuitously. > > If we do break it we should explicitly disallow e500v1 with e500plat. > > I'd prefer if we don't overparameterize - it'll just become a headache > further down. "We shouldn't overparameterize" is a tautology. The question is what constitutes "over". I don't think this is excessive. Again, it's parameterization that U-Boot already does, even disregarding e500v1, and QEMU plays the role of U-Boot to a certain degree (even in the new mode of actually running U-Boot, the address map is fixed). Perhaps it could be simplified by just saying that, in 36-bit mode, all physical addresses other than RAM have 0xf prepended. This is similar to what U-Boot does. > Today we don't explicitly disallow anything anywhere - you > could theoretically stick a G3 into e500plat. I don't see why we should > start with heavy sanity checks now :). Ugh. It should at least be documented, since unlike a G3, e500v1 isn't an unrealistic expectation on a platform called e500plat. > Plus, the machine works just fine today if you don't pass in -device > eTSEC. It's not like we're moving all devices to the new "platform bus". We have a TODO to move CCSR as well. -Scott
On 02.07.14 21:34, Scott Wood wrote: > On Wed, 2014-07-02 at 19:59 +0200, Alexander Graf wrote: >> On 02.07.14 19:52, Scott Wood wrote: >>> On Wed, 2014-07-02 at 19:30 +0200, Alexander Graf wrote: >>>> On 02.07.14 19:26, Scott Wood wrote: >>>>> On Wed, 2014-07-02 at 19:12 +0200, Alexander Graf wrote: >>>>>> On 02.07.14 00:50, Scott Wood wrote: >>>>>>> Plus, let's please not hardcode any more addresses that are going to be >>>>>>> a problem for giving guests a large amount of RAM (yes, CCSRBAR is also >>>>>>> blocking that, but that has a TODO to parameterize). How about >>>>>>> 0xf00000000ULL? Unless of course we're emulating an e500v1, which >>>>>>> doesn't support 36-bit physical addressing. Parameterization would help >>>>>>> there as well. >>>>>> I don't think we have to worry about e500v1. I'll change it :). >>>>> We theoretically support it elsewhere... Once parameterized, it >>>>> shouldn't be hard to base the address for this, CCSRBAR, and similar >>>>> things on whether MAS7 is supported. >>>> It gets parametrized in the machine file, CPU selection comes after >>>> machine selection. So parameterizing it doesn't really solve it. >>> Why can't e500plat_init() look at args->cpu_model? Or the >>> parameterization could take two sets of addresses, one for a 32-bit >>> layout and one for a 36-bit layout. It might make sense to make this a >>> user-settable parameter; some OSes might not be able to handle a 36-bit >>> layout (or might not be as efficient at handling it) even on e500v2. >>> Many of the e500v2 boards can be built for either a 32 or 36 bit address >>> layout in U-Boot. >>> >>>> However, again, I don't think we have to worry about it. >>> It's not a huge worry, but it'd be nice to not break it gratuitously. >>> If we do break it we should explicitly disallow e500v1 with e500plat. >> I'd prefer if we don't overparameterize - it'll just become a headache >> further down. > "We shouldn't overparameterize" is a tautology. The question is what > constitutes "over". I don't think this is excessive. Again, it's > parameterization that U-Boot already does, even disregarding e500v1, and > QEMU plays the role of U-Boot to a certain degree (even in the new mode > of actually running U-Boot, the address map is fixed). > > Perhaps it could be simplified by just saying that, in 36-bit mode, all > physical addresses other than RAM have 0xf prepended. This is similar > to what U-Boot does. I think we should just have another machine type for that case - one that is 32bit and one that is 36bit compatible. > >> Today we don't explicitly disallow anything anywhere - you >> could theoretically stick a G3 into e500plat. I don't see why we should >> start with heavy sanity checks now :). > Ugh. > > It should at least be documented, since unlike a G3, e500v1 isn't an > unrealistic expectation on a platform called e500plat. > >> Plus, the machine works just fine today if you don't pass in -device >> eTSEC. It's not like we're moving all devices to the new "platform bus". > We have a TODO to move CCSR as well. Yes, that's certainly a good goal to have :). Alex
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index bb2e75f..bf704b0 100644 --- a/hw/ppc/e500.c +++ b/hw/ppc/e500.c @@ -36,6 +36,7 @@ #include "exec/address-spaces.h" #include "qemu/host-utils.h" #include "hw/pci-host/ppce500.h" +#include "qemu/error-report.h" #define EPAPR_MAGIC (0x45504150) #define BINARY_DEVICE_TREE_FILE "mpc8544ds.dtb" @@ -47,6 +48,14 @@ #define RAM_SIZES_ALIGN (64UL << 20) +#define E500_PLATFORM_BASE 0xF0000000ULL +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */ +#define E500_PLATFORM_PAGE_SHIFT 12 +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \ + E500_PLATFORM_PAGE_SHIFT) +#define E500_PLATFORM_FIRST_IRQ 5 +#define E500_PLATFORM_NUM_IRQS 10 + /* TODO: parameterize */ #define MPC8544_CCSRBAR_BASE 0xE0000000ULL #define MPC8544_CCSRBAR_SIZE 0x00100000ULL @@ -122,6 +131,77 @@ static void dt_serial_create(void *fdt, unsigned long long offset, } } +typedef struct PlatformDevtreeData { + void *fdt; + const char *mpic; + int irq_start; + const char *node; + int id; +} PlatformDevtreeData; + +static int sysbus_device_create_devtree(Object *obj, void *opaque) +{ + PlatformDevtreeData *data = opaque; + Object *dev; + SysBusDevice *sbdev; + bool matched = false; + + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); + sbdev = (SysBusDevice *)dev; + + if (!sbdev) { + /* Container, traverse it for children */ + return object_child_foreach(obj, sysbus_device_create_devtree, data); + } + + if (matched) { + data->id++; + } else { + error_report("Device %s is not supported by this machine yet.", + qdev_fw_name(DEVICE(dev))); + exit(1); + } + + return 0; +} + +static void platform_create_devtree(void *fdt, const char *node, uint64_t addr, + const char *mpic, int irq_start, + int nr_irqs) +{ + const char platcomp[] = "qemu,platform\0simple-bus"; + PlatformDevtreeData data; + Object *container; + + /* Create a /platform node that we can put all devices into */ + + qemu_fdt_add_subnode(fdt, node); + qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp)); + qemu_fdt_setprop_string(fdt, node, "device_type", "platform"); + + /* Our platform hole is less than 32bit big, so 1 cell is enough for address + and size */ + qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1); + qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1); + qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr, + E500_PLATFORM_HOLE); + + qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic); + + /* Loop through all devices and create nodes for known ones */ + + data.fdt = fdt; + data.mpic = mpic; + data.irq_start = irq_start; + data.node = node; + data.id = 0; + + container = container_get(qdev_get_machine(), "/peripheral"); + sysbus_device_create_devtree(container, &data); + container = container_get(qdev_get_machine(), "/peripheral-anon"); + sysbus_device_create_devtree(container, &data); +} + static int ppce500_load_device_tree(MachineState *machine, PPCE500Params *params, hwaddr addr, @@ -379,6 +459,14 @@ static int ppce500_load_device_tree(MachineState *machine, qemu_fdt_setprop_cell(fdt, pci, "#address-cells", 3); qemu_fdt_setprop_string(fdt, "/aliases", "pci0", pci); + if (params->has_platform) { + gchar *node = g_strdup_printf("/platform@%llx", E500_PLATFORM_BASE); + platform_create_devtree(fdt, node, E500_PLATFORM_BASE, mpic, + E500_PLATFORM_FIRST_IRQ, + E500_PLATFORM_NUM_IRQS); + g_free(node); + } + params->fixup_devtree(params, fdt); if (toplevel_compat) { @@ -618,6 +706,155 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params *params, MemoryRegion *ccsr, return mpic; } +typedef struct PlatformNotifier { + Notifier notifier; + MemoryRegion *address_space_mem; + qemu_irq *mpic; +} PlatformNotifier; + +typedef struct PlatformInitData { + unsigned long *used_irqs; + unsigned long *used_mem; + MemoryRegion *mem; + qemu_irq *irqs; + int device_count; +} PlatformInitData; + +static int platform_map_irq(SysBusDevice *sbdev, int n, unsigned long *used_irqs, + qemu_irq *platform_irqs) +{ + int max_irqs = E500_PLATFORM_NUM_IRQS; + int irqn = sbdev->user_irqs[n]; + + if (irqn == (uint32_t)SYSBUS_DYNAMIC) { + /* Find the first available IRQ */ + irqn = find_first_zero_bit(used_irqs, max_irqs); + } + + if ((irqn >= max_irqs) || test_and_set_bit(irqn, used_irqs)) { + hw_error("e500: IRQ %d is already allocated or no free IRQ left", irqn); + } + + sysbus_connect_irq(sbdev, n, platform_irqs[irqn]); + sbdev->user_irqs[n] = irqn; + + return 0; +} + +static int platform_map_mmio(SysBusDevice *sbdev, int n, unsigned long *used_mem, + MemoryRegion *pmem) +{ + MemoryRegion *device_mem = sbdev->mmio[n].memory; + uint64_t size = memory_region_size(device_mem); + uint64_t page_size = (1 << E500_PLATFORM_PAGE_SHIFT); + uint64_t page_mask = page_size - 1; + uint64_t size_pages = (size + page_mask) >> E500_PLATFORM_PAGE_SHIFT; + hwaddr addr = sbdev->user_mmios[n]; + int page; + int i; + + page = addr >> E500_PLATFORM_PAGE_SHIFT; + if (addr == (uint64_t)SYSBUS_DYNAMIC) { + uint64_t size_pages_align; + + /* Align the region to at least its own size granularity */ + if (is_power_of_2(size_pages)) { + size_pages_align = size_pages; + } else { + size_pages_align = pow2floor(size_pages) << 1; + } + + /* Find the first available region that fits */ + page = bitmap_find_next_zero_area(used_mem, E500_PLATFORM_HOLE_PAGES, 0, + size_pages, size_pages_align); + + addr = (uint64_t)page << E500_PLATFORM_PAGE_SHIFT; + } + + if (page >= E500_PLATFORM_HOLE_PAGES || test_bit(page, used_mem) || + (find_next_bit(used_mem, E500_PLATFORM_HOLE_PAGES, page) < size_pages)) { + hw_error("e500: Memory [%"PRIx64":%"PRIx64" is already allocated or " + "no slot left", addr, size); + } + + for (i = page; i < (page + size_pages); i++) { + set_bit(i, used_mem); + } + + memory_region_add_subregion(pmem, addr, device_mem); + sbdev->mmio[n].addr = addr; + sbdev->user_mmios[n] = addr; + + return 0; +} + +static int sysbus_device_check(Object *obj, void *opaque) +{ + PlatformInitData *init = opaque; + Object *dev; + SysBusDevice *sbdev; + int i; + + dev = object_dynamic_cast(obj, TYPE_SYS_BUS_DEVICE); + sbdev = (SysBusDevice *)dev; + + if (!sbdev) { + /* Container, traverse it for children */ + return object_child_foreach(obj, sysbus_device_check, opaque); + } + + /* Connect sysbus device to virtual platform bus */ + for (i = 0; i < sbdev->num_irq; i++) { + if (!sbdev->irqp[i]) { + /* This IRQ is an incoming IRQ, we can't wire those here */ + continue; + } + platform_map_irq(sbdev, i, init->used_irqs, init->irqs); + } + + for (i = 0; i < sbdev->num_mmio; i++) { + platform_map_mmio(sbdev, i, init->used_mem, init->mem); + } + + return 0; +} + +static void sysbus_devices_init(MemoryRegion *address_space_mem, + qemu_irq *mpic) +{ + DECLARE_BITMAP(used_irqs, E500_PLATFORM_NUM_IRQS); + DECLARE_BITMAP(used_mem, E500_PLATFORM_HOLE_PAGES); + MemoryRegion *platform_region = g_new(MemoryRegion, 1); + Object *container; + PlatformInitData init = { + .used_irqs = used_irqs, + .used_mem = used_mem, + .mem = platform_region, + .irqs = &mpic[E500_PLATFORM_FIRST_IRQ], + }; + + memory_region_init(platform_region, NULL, "platform devices", + E500_PLATFORM_HOLE); + + bitmap_clear(used_irqs, 0, E500_PLATFORM_NUM_IRQS); + bitmap_clear(used_mem, 0, E500_PLATFORM_HOLE_PAGES); + + /* Loop through all sysbus devices that were spawened outside the machine */ + container = container_get(qdev_get_machine(), "/peripheral"); + sysbus_device_check(container, &init); + container = container_get(qdev_get_machine(), "/peripheral-anon"); + sysbus_device_check(container, &init); + + memory_region_add_subregion(address_space_mem, E500_PLATFORM_BASE, + platform_region); +} + +static void sysbus_devices_init_notify(Notifier *notifier, void *data) +{ + PlatformNotifier *pn = (PlatformNotifier *)notifier; + sysbus_devices_init(pn->address_space_mem, pn->mpic); +} + void ppce500_init(MachineState *machine, PPCE500Params *params) { MemoryRegion *address_space_mem = get_system_memory(); @@ -770,6 +1007,20 @@ void ppce500_init(MachineState *machine, PPCE500Params *params) cur_base = (32 * 1024 * 1024); } + /* Platform Devices */ + if (params->has_platform) { + PlatformNotifier *notifier = g_new(PlatformNotifier, 1); + Object *obj = OBJECT(machine); + + notifier->notifier.notify = sysbus_devices_init_notify; + notifier->address_space_mem = address_space_mem; + notifier->mpic = mpic; + qemu_add_machine_init_done_notifier(¬ifier->notifier); + + /* Promote the machine as dynamic sysbus capable */ + object_property_add_bool(obj, "has-dynamic-sysbus", NULL, NULL, NULL); + } + /* Load kernel. */ if (machine->kernel_filename) { kernel_base = cur_base; diff --git a/hw/ppc/e500.h b/hw/ppc/e500.h index 08b25fa..3a588ed 100644 --- a/hw/ppc/e500.h +++ b/hw/ppc/e500.h @@ -11,6 +11,7 @@ typedef struct PPCE500Params { void (*fixup_devtree)(struct PPCE500Params *params, void *fdt); int mpic_version; + bool has_platform; } PPCE500Params; void ppce500_init(MachineState *machine, PPCE500Params *params); diff --git a/hw/ppc/e500plat.c b/hw/ppc/e500plat.c index 27df31d..bb84283 100644 --- a/hw/ppc/e500plat.c +++ b/hw/ppc/e500plat.c @@ -35,6 +35,7 @@ static void e500plat_init(MachineState *machine) .pci_nr_slots = PCI_SLOT_MAX - 1, .fixup_devtree = e500plat_fixup_devtree, .mpic_version = OPENPIC_MODEL_FSL_MPIC_42, + .has_platform = true, }; /* Older KVM versions don't support EPR which breaks guests when we announce
For e500 our approach to supporting dynamically spawned sysbus devices is to create a simple bus from the guest's point of view within which we map those devices dynamically. We allocate memory regions always within the "platform" hole in address space and map IRQs to predetermined IRQ lines that are reserved for platform device usage. This maps really nicely into device tree logic, so we can just tell the guest about our virtual simple bus in device tree as well. Signed-off-by: Alexander Graf <agraf@suse.de> --- hw/ppc/e500.c | 251 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ hw/ppc/e500.h | 1 + hw/ppc/e500plat.c | 1 + 3 files changed, 253 insertions(+)