Message ID | 20190726104519.23812-4-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ARM virt: ACPI memory hotplug support | expand |
On Fri, 26 Jul 2019 11:45:13 +0100 Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > From: Samuel Ortiz <sameo@linux.intel.com> > > The ACPI Generic Event Device (GED) is a hardware-reduced specific > device[ACPI v6.1 Section 5.6.9] that handles all platform events, > including the hotplug ones. This patch generates the AML code that > defines GEDs. > > Platforms need to specify their own GED Event bitmap to describe > what kind of events they want to support through GED. Also this > uses a a single interrupt for the GED device, relying on IO > memory region to communicate the type of device affected by the > interrupt. This way, we can support up to 32 events with a unique > interrupt. > > This supports only memory hotplug for now. > > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > new file mode 100644 > index 0000000000..7902e9d706 > --- /dev/null > +++ b/hw/acpi/generic_event_device.c [...] > +void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev, > + uint32_t ged_irq, AmlRegionSpace rs) > +{ [...] > + > + if (ged_events) { > + error_report("GED: Unsupported events specified"); > + exit(1); I'd use error_abort instead, since it's programing error, if you have to respin series. > + } > + } > + > + /* Append _EVT method */ > + aml_append(dev, evt); > + > + aml_append(table, dev); > +} > + [...] > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > +{ > + AcpiGedState *s = ACPI_GED(dev); > + > + assert(s->ged_base); > + acpi_ged_init(get_system_memory(), dev, &s->ged_state); calling get_system_memory() from device code used to be a reason for rejecting patch, I'm not sure what suggest though. Maybe Paolo could suggest something. > + > + if (s->memhp_state.is_enabled) { > + assert(s->memhp_base); > + acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev), ^^^^ ditto > + &s->memhp_state, > + s->memhp_base); > + } > +} [...] > + > +#endif
Hi Igor, > -----Original Message----- > From: Qemu-devel > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn > u.org] On Behalf Of Igor Mammedov > Sent: 30 July 2019 16:25 > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com; > ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com; > qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org; > Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com; > lersek@redhat.com > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic > Event Device Support > > On Fri, 26 Jul 2019 11:45:13 +0100 > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > > From: Samuel Ortiz <sameo@linux.intel.com> > > > > The ACPI Generic Event Device (GED) is a hardware-reduced specific > > device[ACPI v6.1 Section 5.6.9] that handles all platform events, > > including the hotplug ones. This patch generates the AML code that > > defines GEDs. > > > > Platforms need to specify their own GED Event bitmap to describe > > what kind of events they want to support through GED. Also this > > uses a a single interrupt for the GED device, relying on IO > > memory region to communicate the type of device affected by the > > interrupt. This way, we can support up to 32 events with a unique > > interrupt. > > > > This supports only memory hotplug for now. > > > > > diff --git a/hw/acpi/generic_event_device.c > b/hw/acpi/generic_event_device.c > > new file mode 100644 > > index 0000000000..7902e9d706 > > --- /dev/null > > +++ b/hw/acpi/generic_event_device.c > [...] > > +void build_ged_aml(Aml *table, const char *name, HotplugHandler > *hotplug_dev, > > + uint32_t ged_irq, AmlRegionSpace rs) > > +{ > [...] > > + > > + if (ged_events) { > > + error_report("GED: Unsupported events specified"); > > + exit(1); > I'd use error_abort instead, since it's programing error, if you have to respin > series. Ok. > > + } > > + } > > + > > + /* Append _EVT method */ > > + aml_append(dev, evt); > > + > > + aml_append(table, dev); > > +} > > + > [...] > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > > +{ > > + AcpiGedState *s = ACPI_GED(dev); > > + > > + assert(s->ged_base); > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state); > > calling get_system_memory() from device code used to be a reason for > rejecting patch, > I'm not sure what suggest though. > > Maybe Paolo could suggest something. How about using object_property_set_link()? Something like below. ------8----- diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c index f00b0ab14b..eb1ed38f4a 100644 --- a/hw/acpi/generic_event_device.c +++ b/hw/acpi/generic_event_device.c @@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp) AcpiGedState *s = ACPI_GED(dev); assert(s->ged_base); - acpi_ged_init(get_system_memory(), dev, &s->ged_state); + assert(s->sys_mem); + acpi_ged_init(s->sys_mem, dev, &s->ged_state); if (s->memhp_state.is_enabled) { assert(s->memhp_base); - acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev), + acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev), &s->memhp_state, s->memhp_base); } @@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = { * because GED handles memory hotplug event and acpi-mem-hotplug * memory region gets initialized when GED device is realized. */ + DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION, + MemoryRegion *), DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0), DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState, memhp_state.is_enabled, true), diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 73a758d9a9..0cbaf6c6e1 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic) DeviceState *dev; int irq = vms->irqmap[VIRT_ACPI_GED]; uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT; + MemoryRegion *sys_mem = get_system_memory(); dev = DEVICE(object_new(TYPE_ACPI_GED)); + + object_property_set_link(OBJECT(dev), OBJECT(sys_mem), + "memory", &error_abort); qdev_prop_set_uint64(dev, "memhp-base", vms->memmap[VIRT_PCDIMM_ACPI].base); qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base); diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h index 63104f1344..f1f2f337f7 100644 --- a/include/hw/acpi/generic_event_device.h +++ b/include/hw/acpi/generic_event_device.h @@ -89,6 +89,7 @@ typedef struct GEDState { typedef struct AcpiGedState { DeviceClass parent_obj; + MemoryRegion *sys_mem; MemHotplugState memhp_state; hwaddr memhp_base; hwaddr ged_base; ---8----
On Thu, 1 Aug 2019 08:36:33 +0000 Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote: > Hi Igor, > > > -----Original Message----- > > From: Qemu-devel > > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn > > u.org] On Behalf Of Igor Mammedov > > Sent: 30 July 2019 16:25 > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> > > Cc: peter.maydell@linaro.org; sameo@linux.intel.com; > > ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com; > > qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm > > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org; > > Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com; > > lersek@redhat.com > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic > > Event Device Support > > > > On Fri, 26 Jul 2019 11:45:13 +0100 > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote: > > > > > From: Samuel Ortiz <sameo@linux.intel.com> > > > > > > The ACPI Generic Event Device (GED) is a hardware-reduced specific > > > device[ACPI v6.1 Section 5.6.9] that handles all platform events, > > > including the hotplug ones. This patch generates the AML code that > > > defines GEDs. > > > > > > Platforms need to specify their own GED Event bitmap to describe > > > what kind of events they want to support through GED. Also this > > > uses a a single interrupt for the GED device, relying on IO > > > memory region to communicate the type of device affected by the > > > interrupt. This way, we can support up to 32 events with a unique > > > interrupt. > > > > > > This supports only memory hotplug for now. > > > > > > > > diff --git a/hw/acpi/generic_event_device.c > > b/hw/acpi/generic_event_device.c > > > new file mode 100644 > > > index 0000000000..7902e9d706 > > > --- /dev/null > > > +++ b/hw/acpi/generic_event_device.c > > [...] > > > +void build_ged_aml(Aml *table, const char *name, HotplugHandler > > *hotplug_dev, > > > + uint32_t ged_irq, AmlRegionSpace rs) > > > +{ > > [...] > > > + > > > + if (ged_events) { > > > + error_report("GED: Unsupported events specified"); > > > + exit(1); > > I'd use error_abort instead, since it's programing error, if you have to respin > > series. > > Ok. > > > > + } > > > + } > > > + > > > + /* Append _EVT method */ > > > + aml_append(dev, evt); > > > + > > > + aml_append(table, dev); > > > +} > > > + > > [...] > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > > > +{ > > > + AcpiGedState *s = ACPI_GED(dev); > > > + > > > + assert(s->ged_base); > > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state); > > > > calling get_system_memory() from device code used to be a reason for > > rejecting patch, > > I'm not sure what suggest though. > > > > Maybe Paolo could suggest something. > > How about using object_property_set_link()? Something like below. I'm afraid it doesn't help much. Issue here is that we are letting device to manage whole address space (which should be managed by machine) So I'd just keep get_system_memory() as is for now if there aren't any objections. > ------8----- > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c > index f00b0ab14b..eb1ed38f4a 100644 > --- a/hw/acpi/generic_event_device.c > +++ b/hw/acpi/generic_event_device.c > @@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > AcpiGedState *s = ACPI_GED(dev); > > assert(s->ged_base); > - acpi_ged_init(get_system_memory(), dev, &s->ged_state); > + assert(s->sys_mem); > + acpi_ged_init(s->sys_mem, dev, &s->ged_state); > > if (s->memhp_state.is_enabled) { > assert(s->memhp_base); > - acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev), > + acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev), > &s->memhp_state, > s->memhp_base); > } > @@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = { > * because GED handles memory hotplug event and acpi-mem-hotplug > * memory region gets initialized when GED device is realized. > */ > + DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION, > + MemoryRegion *), > DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0), > DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState, > memhp_state.is_enabled, true), > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 73a758d9a9..0cbaf6c6e1 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic) > DeviceState *dev; > int irq = vms->irqmap[VIRT_ACPI_GED]; > uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT; > + MemoryRegion *sys_mem = get_system_memory(); > > dev = DEVICE(object_new(TYPE_ACPI_GED)); > + > + object_property_set_link(OBJECT(dev), OBJECT(sys_mem), > + "memory", &error_abort); > qdev_prop_set_uint64(dev, "memhp-base", > vms->memmap[VIRT_PCDIMM_ACPI].base); > qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base); > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h > index 63104f1344..f1f2f337f7 100644 > --- a/include/hw/acpi/generic_event_device.h > +++ b/include/hw/acpi/generic_event_device.h > @@ -89,6 +89,7 @@ typedef struct GEDState { > > typedef struct AcpiGedState { > DeviceClass parent_obj; > + MemoryRegion *sys_mem; > MemHotplugState memhp_state; > hwaddr memhp_base; > hwaddr ged_base; > > ---8---- >
On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote: > > On Thu, 1 Aug 2019 08:36:33 +0000 > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote: > > > Hi Igor, > > > > > -----Original Message----- > > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > > > > +{ > > > > + AcpiGedState *s = ACPI_GED(dev); > > > > + > > > > + assert(s->ged_base); > > > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state); > > > > > > calling get_system_memory() from device code used to be a reason for > > > rejecting patch, > > > I'm not sure what suggest though. > > > > > > Maybe Paolo could suggest something. > > > > How about using object_property_set_link()? Something like below. > I'm afraid it doesn't help much. Issue here is that we are letting > device to manage whole address space (which should be managed by machine) > So I'd just keep get_system_memory() as is for now if there aren't any > objections. What are we trying to do with this device, and what does it need the system memory region for? In this case, we seem to do: +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st) +{ + AcpiGedState *s = ACPI_GED(dev); + + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st, + TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN); + memory_region_add_subregion(as, s->ged_base, &ged_st->io); + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1); +} This is definitely a bad idea -- devices should not add their own memory regions to the system memory MR. They should expose their MRs (by being a sysbus-device) and let the board code do the wiring up of the MRs into the right memory space at the right address. thanks -- PMM
On Mon, 5 Aug 2019 14:42:38 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote: > > > > On Thu, 1 Aug 2019 08:36:33 +0000 > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote: > > > > > Hi Igor, > > > > > > > -----Original Message----- > > > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) > > > > > +{ > > > > > + AcpiGedState *s = ACPI_GED(dev); > > > > > + > > > > > + assert(s->ged_base); > > > > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state); > > > > > > > > calling get_system_memory() from device code used to be a reason for > > > > rejecting patch, > > > > I'm not sure what suggest though. > > > > > > > > Maybe Paolo could suggest something. > > > > > > How about using object_property_set_link()? Something like below. > > I'm afraid it doesn't help much. Issue here is that we are letting > > device to manage whole address space (which should be managed by machine) > > So I'd just keep get_system_memory() as is for now if there aren't any > > objections. > > What are we trying to do with this device, and what does it need > the system memory region for? > > In this case, we seem to do: > > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st) > +{ > + AcpiGedState *s = ACPI_GED(dev); > + > + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st, > + TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN); > + memory_region_add_subregion(as, s->ged_base, &ged_st->io); > + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1); > +} > > > This is definitely a bad idea -- devices should not add their > own memory regions to the system memory MR. They should > expose their MRs (by being a sysbus-device) and let the board > code do the wiring up of the MRs into the right memory space > at the right address. it's not the only place in GED that is trying to add to system address space, optionally if called acpi_memory_hotplug_init() will do the same, then later we could add cpu hotplug memory region over there. Perhaps we could use bus-less device plug code path, in that case memory_region_init_io()/qdev_init_gpio_out_named() should be moved to ged_initfn() and mapping part into specialized helper (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb) callback during completing device realize stage, it would be something like: virt.c: virt_machine_device_plug_cb() if dev == GED_TYPE machine_ged_plug_helper(system_memory) generic_event_device.c: machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized connect_irq() memory_region_add_subregion(as, ged->ged_base, &ged->io) if ged->memory-hotplug-support memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io) in this case addresses could be normally hard-codded in board code if device is not optional (as in patch 6/9: create_acpi_ged() ) or potentially they could come from CLI as -device parameters (/me thinking about building blocks that allow to create machine from config) sysbus device might be fine as shortcut if we are thinking about only creating device during machine_init (although I have a reservations towards sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th region at some address)). > thanks > -- PMM
On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote: > On Mon, 5 Aug 2019 14:42:38 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > > This is definitely a bad idea -- devices should not add their > > own memory regions to the system memory MR. They should > > expose their MRs (by being a sysbus-device) and let the board > > code do the wiring up of the MRs into the right memory space > > at the right address. > > it's not the only place in GED that is trying to add to system address > space, optionally if called acpi_memory_hotplug_init() will do the same, > then later we could add cpu hotplug memory region over there. > > Perhaps we could use bus-less device plug code path, > in that case memory_region_init_io()/qdev_init_gpio_out_named() > should be moved to ged_initfn() and mapping part into specialized helper > (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb) > callback during completing device realize stage, it would be something like: > > virt.c: > virt_machine_device_plug_cb() > if dev == GED_TYPE > machine_ged_plug_helper(system_memory) > > generic_event_device.c: > machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized > connect_irq() > memory_region_add_subregion(as, ged->ged_base, &ged->io) > if ged->memory-hotplug-support > memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io) I don't really understand why we want to do this complicated thing, rather than just doing the normal thing for devices that live at particular addresses, ie make them sysbus devices and have the board map their memory regions in the right place. > in this case addresses could be normally hard-codded in board code if device is not optional > (as in patch 6/9: create_acpi_ged() ) > or potentially they could come from CLI as -device parameters > (/me thinking about building blocks that allow to create machine from config) I don't think we want to do this. The user should not have to know anything about addresses or have to specify them on the command line. (This is why you can't create sysbus devices with -device except for some odd special cases to do with passthrough of hardware.) > sysbus device might be fine as shortcut if we are thinking about > only creating device during machine_init (although I have a reservations towards > sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th > region at some address)). Not sure entirely what you have in mind here? (though yes, the sysbus device API has its awkward corners, some of which are just down to how old it is.) thanks -- PMM
On Mon, 5 Aug 2019 16:54:06 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote: > > On Mon, 5 Aug 2019 14:42:38 +0100 > > Peter Maydell <peter.maydell@linaro.org> wrote: > > > This is definitely a bad idea -- devices should not add their > > > own memory regions to the system memory MR. They should > > > expose their MRs (by being a sysbus-device) and let the board > > > code do the wiring up of the MRs into the right memory space > > > at the right address. > > > > it's not the only place in GED that is trying to add to system address > > space, optionally if called acpi_memory_hotplug_init() will do the same, > > then later we could add cpu hotplug memory region over there. > > > > Perhaps we could use bus-less device plug code path, > > in that case memory_region_init_io()/qdev_init_gpio_out_named() > > should be moved to ged_initfn() and mapping part into specialized helper > > (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb) > > callback during completing device realize stage, it would be something like: > > > > virt.c: > > virt_machine_device_plug_cb() > > if dev == GED_TYPE > > machine_ged_plug_helper(system_memory) > > > > generic_event_device.c: > > machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized > > connect_irq() > > memory_region_add_subregion(as, ged->ged_base, &ged->io) > > if ged->memory-hotplug-support > > memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io) > > I don't really understand why we want to do this complicated > thing, rather than just doing the normal thing for devices > that live at particular addresses, ie make them sysbus devices > and have the board map their memory regions in the right place. hotplug path is basically the same as sysbus the only difference is that it uses machine's (pre_)plug handler to wire up devices and more flexible than sysbus. > > in this case addresses could be normally hard-codded in board code if device is not optional > > (as in patch 6/9: create_acpi_ged() ) > > or potentially they could come from CLI as -device parameters > > (/me thinking about building blocks that allow to create machine from config) > > I don't think we want to do this. The user should not have to > know anything about addresses or have to specify them on the > command line. (This is why you can't create sysbus devices > with -device except for some odd special cases to do with passthrough > of hardware.) > > > sysbus device might be fine as shortcut if we are thinking about > > only creating device during machine_init (although I have a reservations towards > > sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th > > region at some address)). > > Not sure entirely what you have in mind here? (though yes, the > sysbus device API has its awkward corners, some of which are > just down to how old it is.) since it's a fixed device I don't mind using sysbus either, lets do it this way. > thanks > -- PMM
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig index 7c59cf900b..12e3f1e86e 100644 --- a/hw/acpi/Kconfig +++ b/hw/acpi/Kconfig @@ -31,3 +31,7 @@ config ACPI_VMGENID bool default y depends on PC + +config ACPI_HW_REDUCED + bool + depends on ACPI diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs index 9bb2101e3b..655a9c1973 100644 --- a/hw/acpi/Makefile.objs +++ b/hw/acpi/Makefile.objs @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o +common-obj-$(CONFIG_ACPI_HW_REDUCED) += generic_event_device.o common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o common-obj-y += acpi_interface.o diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c new file mode 100644 index 0000000000..7902e9d706 --- /dev/null +++ b/hw/acpi/generic_event_device.c @@ -0,0 +1,322 @@ +/* + * + * Copyright (c) 2018 Intel Corporation + * Copyright (c) 2019 Huawei Technologies R & D (UK) Ltd + * Written by Samuel Ortiz, Shameer Kolothum + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2 or later, as published by the Free Software Foundation. + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "exec/address-spaces.h" +#include "hw/acpi/acpi.h" +#include "hw/acpi/generic_event_device.h" +#include "hw/mem/pc-dimm.h" +#include "qemu/error-report.h" + +static const uint32_t ged_supported_events[] = { + ACPI_GED_MEM_HOTPLUG_EVT, +}; + +/* + * The ACPI Generic Event Device (GED) is a hardware-reduced specific + * device[ACPI v6.1 Section 5.6.9] that handles all platform events, + * including the hotplug ones. Platforms need to specify their own + * GED Event bitmap to describe what kind of events they want to support + * through GED. This routine uses a single interrupt for the GED device, + * relying on IO memory region to communicate the type of device + * affected by the interrupt. This way, we can support up to 32 events + * with a unique interrupt. + */ +void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev, + uint32_t ged_irq, AmlRegionSpace rs) +{ + AcpiGedState *s = ACPI_GED(hotplug_dev); + Aml *crs = aml_resource_template(); + Aml *evt, *field; + Aml *dev = aml_device("%s", name); + Aml *evt_sel = aml_local(0); + Aml *esel = aml_name(AML_GED_EVT_SEL); + + assert(s->ged_base); + + /* _CRS interrupt */ + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, + AML_EXCLUSIVE, &ged_irq, 1)); + + aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0013"))); + aml_append(dev, aml_name_decl("_UID", aml_string(GED_DEVICE))); + aml_append(dev, aml_name_decl("_CRS", crs)); + + /* Append IO region */ + aml_append(dev, aml_operation_region(AML_GED_EVT_REG, rs, + aml_int(s->ged_base + ACPI_GED_EVT_SEL_OFFSET), + ACPI_GED_EVT_SEL_LEN)); + field = aml_field(AML_GED_EVT_REG, AML_DWORD_ACC, AML_NOLOCK, + AML_WRITE_AS_ZEROS); + aml_append(field, aml_named_field(AML_GED_EVT_SEL, + ACPI_GED_EVT_SEL_LEN * BITS_PER_BYTE)); + aml_append(dev, field); + + /* + * For each GED event we: + * - Add a conditional block for each event, inside a loop. + * - Call a method for each supported GED event type. + * + * The resulting ASL code looks like: + * + * Local0 = ESEL + * If ((Local0 & One) == One) + * { + * MethodEvent0() + * } + * + * If ((Local0 & 0x2) == 0x2) + * { + * MethodEvent1() + * } + * ... + */ + evt = aml_method("_EVT", 1, AML_SERIALIZED); + { + Aml *if_ctx; + uint32_t i; + uint32_t ged_events = ctpop32(s->ged_event_bitmap); + + /* Local0 = ESEL */ + aml_append(evt, aml_store(esel, evt_sel)); + + for (i = 0; i < ARRAY_SIZE(ged_supported_events) && ged_events; i++) { + uint32_t event = s->ged_event_bitmap & ged_supported_events[i]; + + if (!event) { + continue; + } + + if_ctx = aml_if(aml_equal(aml_and(evt_sel, aml_int(event), NULL), + aml_int(event))); + switch (event) { + case ACPI_GED_MEM_HOTPLUG_EVT: + aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "." + MEMORY_SLOT_SCAN_METHOD)); + break; + default: + /* + * Please make sure all the events in ged_supported_events[] + * are handled above. + */ + g_assert_not_reached(); + } + + aml_append(evt, if_ctx); + ged_events--; + } + + if (ged_events) { + error_report("GED: Unsupported events specified"); + exit(1); + } + } + + /* Append _EVT method */ + aml_append(dev, evt); + + aml_append(table, dev); +} + +/* Memory read by the GED _EVT AML dynamic method */ +static uint64_t ged_read(void *opaque, hwaddr addr, unsigned size) +{ + uint64_t val = 0; + GEDState *ged_st = opaque; + + switch (addr) { + case ACPI_GED_EVT_SEL_OFFSET: + /* Read the selector value and reset it */ + val = ged_st->sel; + ged_st->sel = 0; + break; + default: + break; + } + + return val; +} + +/* Nothing is expected to be written to the GED memory region */ +static void ged_write(void *opaque, hwaddr addr, uint64_t data, + unsigned int size) +{ +} + +static const MemoryRegionOps ged_ops = { + .read = ged_read, + .write = ged_write, + .endianness = DEVICE_LITTLE_ENDIAN, + .valid = { + .min_access_size = 4, + .max_access_size = 4, + }, +}; + +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st) +{ + AcpiGedState *s = ACPI_GED(dev); + + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st, + TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN); + memory_region_add_subregion(as, s->ged_base, &ged_st->io); + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1); +} + +static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + AcpiGedState *s = ACPI_GED(hotplug_dev); + + if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { + if (s->memhp_state.is_enabled) { + acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp); + } else { + error_setg(errp, + "memory hotplug is not enabled: %s.memory-hotplug-support " + "is not set", object_get_typename(OBJECT(s))); + } + } else { + error_setg(errp, "virt: device plug request for unsupported device" + " type: %s", object_get_typename(OBJECT(dev))); + } +} + +static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev) +{ + AcpiGedState *s = ACPI_GED(adev); + GEDState *ged_st = &s->ged_state; + uint32_t sel; + + if (ev & ACPI_MEMORY_HOTPLUG_STATUS) { + sel = ACPI_GED_MEM_HOTPLUG_EVT; + } else { + /* Unknown event. Return without generating interrupt. */ + warn_report("GED: Unsupported event %d. No irq injected", ev); + return; + } + + /* + * Set the GED selector field to communicate the event type. + * This will be read by GED aml code to select the appropriate + * event method. + */ + ged_st->sel |= sel; + + /* Trigger the event by sending an interrupt to the guest. */ + qemu_irq_pulse(s->irq); +} + +static void acpi_ged_device_realize(DeviceState *dev, Error **errp) +{ + AcpiGedState *s = ACPI_GED(dev); + + assert(s->ged_base); + acpi_ged_init(get_system_memory(), dev, &s->ged_state); + + if (s->memhp_state.is_enabled) { + assert(s->memhp_base); + acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev), + &s->memhp_state, + s->memhp_base); + } +} + +static Property acpi_ged_properties[] = { + /* + * Memory hotplug base address is a property of GED here, + * because GED handles memory hotplug event and acpi-mem-hotplug + * memory region gets initialized when GED device is realized. + */ + DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0), + DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState, + memhp_state.is_enabled, true), + DEFINE_PROP_UINT64("ged-base", AcpiGedState, ged_base, 0), + DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +static bool vmstate_test_use_memhp(void *opaque) +{ + AcpiGedState *s = opaque; + return s->memhp_state.is_enabled; +} + +static const VMStateDescription vmstate_memhp_state = { + .name = "acpi-ged/memhp", + .version_id = 1, + .minimum_version_id = 1, + .needed = vmstate_test_use_memhp, + .fields = (VMStateField[]) { + VMSTATE_MEMORY_HOTPLUG(memhp_state, AcpiGedState), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_ged_state = { + .name = "acpi-ged-state", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32(sel, GEDState), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_acpi_ged = { + .name = "acpi-ged", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState), + VMSTATE_END_OF_LIST(), + }, + .subsections = (const VMStateDescription * []) { + &vmstate_memhp_state, + NULL + } +}; + +static void acpi_ged_class_init(ObjectClass *class, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(class); + HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(class); + AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class); + + dc->desc = "ACPI Generic Event Device"; + dc->props = acpi_ged_properties; + dc->realize = acpi_ged_device_realize; + dc->vmsd = &vmstate_acpi_ged; + + hc->plug = acpi_ged_device_plug_cb; + + adevc->send_event = acpi_ged_send_event; +} + +static const TypeInfo acpi_ged_info = { + .name = TYPE_ACPI_GED, + .parent = TYPE_DEVICE, + .instance_size = sizeof(AcpiGedState), + .class_init = acpi_ged_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_HANDLER }, + { TYPE_ACPI_DEVICE_IF }, + { } + } +}; + +static void acpi_ged_register_types(void) +{ + type_register_static(&acpi_ged_info); +} + +type_init(acpi_ged_register_types) diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h new file mode 100644 index 0000000000..f0152b0018 --- /dev/null +++ b/include/hw/acpi/generic_event_device.h @@ -0,0 +1,100 @@ +/* + * + * Copyright (c) 2018 Intel Corporation + * Copyright (c) 2019 Huawei Technologies R & D (UK) Ltd + * Written by Samuel Ortiz, Shameer Kolothum + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2 or later, as published by the Free Software Foundation. + * + * The ACPI Generic Event Device (GED) is a hardware-reduced specific + * device[ACPI v6.1 Section 5.6.9] that handles all platform events, + * including the hotplug ones. Generic Event Device allows platforms + * to handle interrupts in ACPI ASL statements. It follows a very + * similar approach like the _EVT method from GPIO events. All + * interrupts are listed in _CRS and the handler is written in _EVT + * method. Here, we use a single interrupt for the GED device, relying + * on IO memory region to communicate the type of device affected by + * the interrupt. This way, we can support up to 32 events with a + * unique interrupt. + * + * Here is an example. + * + * Device (\_SB.GED) + * { + * Name (_HID, "ACPI0013") + * Name (_UID, Zero) + * Name (_CRS, ResourceTemplate () + * { + * Interrupt (ResourceConsumer, Edge, ActiveHigh, Exclusive, ,, ) + * { + * 0x00000029, + * } + * }) + * OperationRegion (EREG, SystemMemory, 0x09080000, 0x04) + * Field (EREG, DWordAcc, NoLock, WriteAsZeros) + * { + * ESEL, 32 + * } + * + * Method (_EVT, 1, Serialized) // _EVT: Event + * { + * Local0 = ESEL // ESEL = IO memory region which specifies the + * // device type. + * If (((Local0 & One) == One)) + * { + * MethodEvent1() + * } + * If ((Local0 & 0x2) == 0x2) + * { + * MethodEvent2() + * } + * ... + * } + * } + * + */ + +#ifndef HW_ACPI_GED_H +#define HW_ACPI_GED_H + +#include "hw/acpi/memory_hotplug.h" + +#define TYPE_ACPI_GED "acpi-ged" +#define ACPI_GED(obj) \ + OBJECT_CHECK(AcpiGedState, (obj), TYPE_ACPI_GED) + +#define ACPI_GED_EVT_SEL_OFFSET 0x0 +#define ACPI_GED_EVT_SEL_LEN 0x4 + +#define GED_DEVICE "GED" +#define AML_GED_EVT_REG "EREG" +#define AML_GED_EVT_SEL "ESEL" + +/* + * Platforms need to specify the GED event bitmap + * to describe what kind of events they want to support + * through GED. + */ +#define ACPI_GED_MEM_HOTPLUG_EVT 0x1 + +typedef struct GEDState { + MemoryRegion io; + uint32_t sel; +} GEDState; + +typedef struct AcpiGedState { + DeviceClass parent_obj; + MemHotplugState memhp_state; + hwaddr memhp_base; + hwaddr ged_base; + GEDState ged_state; + uint32_t ged_event_bitmap; + qemu_irq irq; +} AcpiGedState; + +void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev, + uint32_t ged_irq, AmlRegionSpace rs); + +#endif