Re: [PATCH v5 40/43] hw/loongarch: Add LoongArch ls7a acpi device support
Ignor,
Thanks for guidance, I reply inline.
在 2022/5/30 下午6:21, Igor Mammedov 写道:
On Fri, 27 May 2022 06:18:43 +0800
maobibo wrote:
On 5/26/22 16:42, Igor Mammedov wrote:
On Tue, 24 May 2022 16:18:01 +0800
Xiaojuan Yang wrote:
commit message needs pointers to specification,
+ in patch comments that point to specific chapters
within the spec for newly introduced registers
Igor,
Thanks for reviewing the patch and guidance, ls7A acpi registers has
minimium registers required by ACPI spec, including pm1a stat/en/cnt,
pm_tmr and GPE stat/enable registers, there is no smi mode in loongarch
those only required for legacy 'Fixed Hardware Programming Model'
which is historically used on x86.
For new platforms if you don't have hardware yet it's better to use
'Hardware-Reduced ACPI' approach and reuse code we already have for
aarch64.
The real loongarch hardware has hw acpi registers like x86,
'Hardware-Reduced ACPI' approach is a good choice for loongarch virt
platform and we will investigate this method. Previously we only
consider to emulate real hardware, anyway the general method is ok for
virt machine.
architecture. The LS7A acpi driver is copied from acpi core driver
since register layout of pm1a/pm_tmr/gpe is different from x86 acpi
registers.
sorry, I couldn't parse above sentence.
The real loongarch ACPI hw registers has PM1a_EVT_BLK/PM1a_CNT_BLK, only
that PM1_EVT_LEN is 8 bytes, and PM1a_STS/PM1a_EN register width is 4
bytes, which is different legacy acpi registers on x86, this causes
different acpi register size and offset. So we do not use original
driver hw/acpi/core.c, and partly copy and modify from it.
By the ACPI spec, there is no specific requirement for layout of ACPI
registers, later we will reuse acpi core driver if the acpi registers
layout can be set dynamically. And we will send the second patch with
detailed description with LS7A ACPI module.
regardless of a separate doc patch, this patch should have a minimal
documentation as it have been pointed earlier, otherwise reviewer or
someone who will later have to look on this code, will have no point
of reference and have no idea if this code is correct or not.
Well we will add document about acpi module.
Also introducing ACPI hardware without an ACPI tables to complement
it is rather pointless as OSPM won't be able to discover/use it.
It might be better to drop this patch until you have corresponding
ACPI tables to describe it.
Originally there is ACPI table and fw_cfg table, in order to speed up
merging progrss it is dropped. We add acpi driver since the testcases
need to shutdown machine. How about dropping the acpi patch and adding
simple virt pm device with shutdown machine function?
After this patch is merged, linux booting function will be added
incluing acpi driver/table and fw_cfg table also etc.
regards
bibo,mao
From: Song Gao
Signed-off-by: Xiaojuan Yang
Signed-off-by: Song Gao
---
MAINTAINERS| 2 +
hw/acpi/Kconfig| 4 +
hw/acpi/ls7a.c | 374 +
hw/acpi/meson.build| 1 +
hw/loongarch/Kconfig | 2 +
hw/loongarch/loongson3.c | 19 +-
include/hw/acpi/ls7a.h | 53 ++
include/hw/pci-host/ls7a.h | 6 +
8 files changed, 458 insertions(+), 3 deletions(-)
create mode 100644 hw/acpi/ls7a.c
create mode 100644 include/hw/acpi/ls7a.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e03a8bca8..6f861dec0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1138,6 +1138,8 @@ F: include/hw/intc/loongarch_*.h
F: hw/intc/loongarch_*.c
F: include/hw/pci-host/ls7a.h
F: hw/rtc/ls7a_rtc.c
+F: include/hw/acpi/ls7a.h
+F: hw/acpi/ls7a.c
M68K Machines
-
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 3703aca212..c65965c9b9 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -13,6 +13,10 @@ config ACPI_X86
select ACPI_PCIHP
select ACPI_ERST
+config ACPI_LOONGARCH
+bool
+select ACPI
+
config ACPI_X86_ICH
bool
select ACPI_X86
diff --git a/hw/acpi/ls7a.c b/hw/acpi/ls7a.c
new file mode 100644
index 00..cc658422dd
--- /dev/null
+++ b/hw/acpi/ls7a.c
@@ -0,0 +1,374 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * LoongArch ACPI implementation
+ *
+ * Copyright (C) 2021 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/sysemu.h"
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/ls7a.h"
+#include "hw/nvram/fw_cfg.h"
+#include "qemu/config-file.h"
+#include "qapi/opts-visitor.h"
+#include "qapi/qapi-events-run-state.h"
+#include "qapi/error.h"
+#include "hw/pci-host/ls7a.h"
+#include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
+#include "migration/vmstate.h"
+
+static void ls7a_pm_update_sci_fn(ACPIREGS *regs)
+{
+LS7APMState *pm =
Re: [PATCH v5 40/43] hw/loongarch: Add LoongArch ls7a acpi device support
On Fri, 27 May 2022 06:18:43 +0800
maobibo wrote:
> On 5/26/22 16:42, Igor Mammedov wrote:
> > On Tue, 24 May 2022 16:18:01 +0800
> > Xiaojuan Yang wrote:
> >
> > commit message needs pointers to specification,
> > + in patch comments that point to specific chapters
> > within the spec for newly introduced registers
> Igor,
>
> Thanks for reviewing the patch and guidance, ls7A acpi registers has
> minimium registers required by ACPI spec, including pm1a stat/en/cnt,
> pm_tmr and GPE stat/enable registers, there is no smi mode in loongarch
those only required for legacy 'Fixed Hardware Programming Model'
which is historically used on x86.
For new platforms if you don't have hardware yet it's better to use
'Hardware-Reduced ACPI' approach and reuse code we already have for
aarch64.
> architecture. The LS7A acpi driver is copied from acpi core driver
> since register layout of pm1a/pm_tmr/gpe is different from x86 acpi
> registers.
sorry, I couldn't parse above sentence.
> By the ACPI spec, there is no specific requirement for layout of ACPI
> registers, later we will reuse acpi core driver if the acpi registers
> layout can be set dynamically. And we will send the second patch with
> detailed description with LS7A ACPI module.
regardless of a separate doc patch, this patch should have a minimal
documentation as it have been pointed earlier, otherwise reviewer or
someone who will later have to look on this code, will have no point
of reference and have no idea if this code is correct or not.
Also introducing ACPI hardware without an ACPI tables to complement
it is rather pointless as OSPM won't be able to discover/use it.
It might be better to drop this patch until you have corresponding
ACPI tables to describe it.
> regards
> bibo, mao
> >
> >> From: Song Gao
> >>
> >> Signed-off-by: Xiaojuan Yang
> >> Signed-off-by: Song Gao
> >> ---
> >> MAINTAINERS| 2 +
> >> hw/acpi/Kconfig| 4 +
> >> hw/acpi/ls7a.c | 374 +
> >> hw/acpi/meson.build| 1 +
> >> hw/loongarch/Kconfig | 2 +
> >> hw/loongarch/loongson3.c | 19 +-
> >> include/hw/acpi/ls7a.h | 53 ++
> >> include/hw/pci-host/ls7a.h | 6 +
> >> 8 files changed, 458 insertions(+), 3 deletions(-)
> >> create mode 100644 hw/acpi/ls7a.c
> >> create mode 100644 include/hw/acpi/ls7a.h
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6e03a8bca8..6f861dec0a 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -1138,6 +1138,8 @@ F: include/hw/intc/loongarch_*.h
> >> F: hw/intc/loongarch_*.c
> >> F: include/hw/pci-host/ls7a.h
> >> F: hw/rtc/ls7a_rtc.c
> >> +F: include/hw/acpi/ls7a.h
> >> +F: hw/acpi/ls7a.c
> >>
> >> M68K Machines
> >> -
> >> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> >> index 3703aca212..c65965c9b9 100644
> >> --- a/hw/acpi/Kconfig
> >> +++ b/hw/acpi/Kconfig
> >> @@ -13,6 +13,10 @@ config ACPI_X86
> >> select ACPI_PCIHP
> >> select ACPI_ERST
> >>
> >> +config ACPI_LOONGARCH
> >> +bool
> >> +select ACPI
> >> +
> >> config ACPI_X86_ICH
> >> bool
> >> select ACPI_X86
> >> diff --git a/hw/acpi/ls7a.c b/hw/acpi/ls7a.c
> >> new file mode 100644
> >> index 00..cc658422dd
> >> --- /dev/null
> >> +++ b/hw/acpi/ls7a.c
> >> @@ -0,0 +1,374 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * LoongArch ACPI implementation
> >> + *
> >> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "hw/hw.h"
> >> +#include "hw/irq.h"
> >> +#include "sysemu/reset.h"
> >> +#include "sysemu/runstate.h"
> >> +#include "hw/acpi/acpi.h"
> >> +#include "hw/acpi/ls7a.h"
> >> +#include "hw/nvram/fw_cfg.h"
> >> +#include "qemu/config-file.h"
> >> +#include "qapi/opts-visitor.h"
> >> +#include "qapi/qapi-events-run-state.h"
> >> +#include "qapi/error.h"
> >> +#include "hw/pci-host/ls7a.h"
> >> +#include "hw/mem/pc-dimm.h"
> >> +#include "hw/mem/nvdimm.h"
> >> +#include "migration/vmstate.h"
> >> +
> >> +static void ls7a_pm_update_sci_fn(ACPIREGS *regs)
> >> +{
> >> +LS7APMState *pm = container_of(regs, LS7APMState, acpi_regs);
> >> +acpi_update_sci(>acpi_regs, pm->irq);
> >> +}
> >> +
> >> +static uint64_t ls7a_gpe_readb(void *opaque, hwaddr addr, unsigned width)
> >> +{
> >> +LS7APMState *pm = opaque;
> >> +return acpi_gpe_ioport_readb(>acpi_regs, addr);
> >> +}
> >> +
> >> +static void ls7a_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> >> +unsigned width)
> >> +{
> >> +LS7APMState *pm = opaque;
> >> +acpi_gpe_ioport_writeb(>acpi_regs, addr, val);
> >> +acpi_update_sci(>acpi_regs, pm->irq);
> >> +}
> >> +
> >> +static const MemoryRegionOps ls7a_gpe_ops = {
> >> +.read = ls7a_gpe_readb,
> >> +.write = ls7a_gpe_writeb,
> >> +
Re: [PATCH v5 40/43] hw/loongarch: Add LoongArch ls7a acpi device support
On 5/26/22 16:42, Igor Mammedov wrote:
On Tue, 24 May 2022 16:18:01 +0800
Xiaojuan Yang wrote:
commit message needs pointers to specification,
+ in patch comments that point to specific chapters
within the spec for newly introduced registers
Igor,
Thanks for reviewing the patch and guidance, ls7A acpi registers has
minimium registers required by ACPI spec, including pm1a stat/en/cnt,
pm_tmr and GPE stat/enable registers, there is no smi mode in loongarch
architecture. The LS7A acpi driver is copied from acpi core driver
since register layout of pm1a/pm_tmr/gpe is different from x86 acpi
registers.
By the ACPI spec, there is no specific requirement for layout of ACPI
registers, later we will reuse acpi core driver if the acpi registers
layout can be set dynamically. And we will send the second patch with
detailed description with LS7A ACPI module.
regards
bibo, mao
From: Song Gao
Signed-off-by: Xiaojuan Yang
Signed-off-by: Song Gao
---
MAINTAINERS| 2 +
hw/acpi/Kconfig| 4 +
hw/acpi/ls7a.c | 374 +
hw/acpi/meson.build| 1 +
hw/loongarch/Kconfig | 2 +
hw/loongarch/loongson3.c | 19 +-
include/hw/acpi/ls7a.h | 53 ++
include/hw/pci-host/ls7a.h | 6 +
8 files changed, 458 insertions(+), 3 deletions(-)
create mode 100644 hw/acpi/ls7a.c
create mode 100644 include/hw/acpi/ls7a.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 6e03a8bca8..6f861dec0a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1138,6 +1138,8 @@ F: include/hw/intc/loongarch_*.h
F: hw/intc/loongarch_*.c
F: include/hw/pci-host/ls7a.h
F: hw/rtc/ls7a_rtc.c
+F: include/hw/acpi/ls7a.h
+F: hw/acpi/ls7a.c
M68K Machines
-
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 3703aca212..c65965c9b9 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -13,6 +13,10 @@ config ACPI_X86
select ACPI_PCIHP
select ACPI_ERST
+config ACPI_LOONGARCH
+bool
+select ACPI
+
config ACPI_X86_ICH
bool
select ACPI_X86
diff --git a/hw/acpi/ls7a.c b/hw/acpi/ls7a.c
new file mode 100644
index 00..cc658422dd
--- /dev/null
+++ b/hw/acpi/ls7a.c
@@ -0,0 +1,374 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * LoongArch ACPI implementation
+ *
+ * Copyright (C) 2021 Loongson Technology Corporation Limited
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/sysemu.h"
+#include "hw/hw.h"
+#include "hw/irq.h"
+#include "sysemu/reset.h"
+#include "sysemu/runstate.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/ls7a.h"
+#include "hw/nvram/fw_cfg.h"
+#include "qemu/config-file.h"
+#include "qapi/opts-visitor.h"
+#include "qapi/qapi-events-run-state.h"
+#include "qapi/error.h"
+#include "hw/pci-host/ls7a.h"
+#include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
+#include "migration/vmstate.h"
+
+static void ls7a_pm_update_sci_fn(ACPIREGS *regs)
+{
+LS7APMState *pm = container_of(regs, LS7APMState, acpi_regs);
+acpi_update_sci(>acpi_regs, pm->irq);
+}
+
+static uint64_t ls7a_gpe_readb(void *opaque, hwaddr addr, unsigned width)
+{
+LS7APMState *pm = opaque;
+return acpi_gpe_ioport_readb(>acpi_regs, addr);
+}
+
+static void ls7a_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
+unsigned width)
+{
+LS7APMState *pm = opaque;
+acpi_gpe_ioport_writeb(>acpi_regs, addr, val);
+acpi_update_sci(>acpi_regs, pm->irq);
+}
+
+static const MemoryRegionOps ls7a_gpe_ops = {
+.read = ls7a_gpe_readb,
+.write = ls7a_gpe_writeb,
+.valid.min_access_size = 1,
+.valid.max_access_size = 8,
+.impl.min_access_size = 1,
+.impl.max_access_size = 1,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+#define VMSTATE_GPE_ARRAY(_field, _state)\
+ { \
+ .name = (stringify(_field)), \
+ .version_id = 0,\
+ .num= ACPI_GPE0_LEN,\
+ .info = _info_uint8, \
+ .size = sizeof(uint8_t), \
+ .flags = VMS_ARRAY | VMS_POINTER, \
+ .offset = vmstate_offset_pointer(_state, _field, uint8_t), \
+ }
+
+static uint64_t ls7a_reset_readw(void *opaque, hwaddr addr, unsigned width)
+{
+return 0;
+}
+
+static void ls7a_reset_writew(void *opaque, hwaddr addr, uint64_t val,
+ unsigned width)
+{
+if (val & 1) {
+qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+return;
+}
+}
+
+static const MemoryRegionOps ls7a_reset_ops = {
+.read = ls7a_reset_readw,
+.write = ls7a_reset_writew,
+.valid.min_access_size = 4,
+.valid.max_access_size = 4,
+.endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+const VMStateDescription
Re: [PATCH v5 40/43] hw/loongarch: Add LoongArch ls7a acpi device support
On Tue, 24 May 2022 16:18:01 +0800
Xiaojuan Yang wrote:
commit message needs pointers to specification,
+ in patch comments that point to specific chapters
within the spec for newly introduced registers
> From: Song Gao
>
> Signed-off-by: Xiaojuan Yang
> Signed-off-by: Song Gao
> ---
> MAINTAINERS| 2 +
> hw/acpi/Kconfig| 4 +
> hw/acpi/ls7a.c | 374 +
> hw/acpi/meson.build| 1 +
> hw/loongarch/Kconfig | 2 +
> hw/loongarch/loongson3.c | 19 +-
> include/hw/acpi/ls7a.h | 53 ++
> include/hw/pci-host/ls7a.h | 6 +
> 8 files changed, 458 insertions(+), 3 deletions(-)
> create mode 100644 hw/acpi/ls7a.c
> create mode 100644 include/hw/acpi/ls7a.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6e03a8bca8..6f861dec0a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1138,6 +1138,8 @@ F: include/hw/intc/loongarch_*.h
> F: hw/intc/loongarch_*.c
> F: include/hw/pci-host/ls7a.h
> F: hw/rtc/ls7a_rtc.c
> +F: include/hw/acpi/ls7a.h
> +F: hw/acpi/ls7a.c
>
> M68K Machines
> -
> diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
> index 3703aca212..c65965c9b9 100644
> --- a/hw/acpi/Kconfig
> +++ b/hw/acpi/Kconfig
> @@ -13,6 +13,10 @@ config ACPI_X86
> select ACPI_PCIHP
> select ACPI_ERST
>
> +config ACPI_LOONGARCH
> +bool
> +select ACPI
> +
> config ACPI_X86_ICH
> bool
> select ACPI_X86
> diff --git a/hw/acpi/ls7a.c b/hw/acpi/ls7a.c
> new file mode 100644
> index 00..cc658422dd
> --- /dev/null
> +++ b/hw/acpi/ls7a.c
> @@ -0,0 +1,374 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * LoongArch ACPI implementation
> + *
> + * Copyright (C) 2021 Loongson Technology Corporation Limited
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/hw.h"
> +#include "hw/irq.h"
> +#include "sysemu/reset.h"
> +#include "sysemu/runstate.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/ls7a.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "qemu/config-file.h"
> +#include "qapi/opts-visitor.h"
> +#include "qapi/qapi-events-run-state.h"
> +#include "qapi/error.h"
> +#include "hw/pci-host/ls7a.h"
> +#include "hw/mem/pc-dimm.h"
> +#include "hw/mem/nvdimm.h"
> +#include "migration/vmstate.h"
> +
> +static void ls7a_pm_update_sci_fn(ACPIREGS *regs)
> +{
> +LS7APMState *pm = container_of(regs, LS7APMState, acpi_regs);
> +acpi_update_sci(>acpi_regs, pm->irq);
> +}
> +
> +static uint64_t ls7a_gpe_readb(void *opaque, hwaddr addr, unsigned width)
> +{
> +LS7APMState *pm = opaque;
> +return acpi_gpe_ioport_readb(>acpi_regs, addr);
> +}
> +
> +static void ls7a_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
> +unsigned width)
> +{
> +LS7APMState *pm = opaque;
> +acpi_gpe_ioport_writeb(>acpi_regs, addr, val);
> +acpi_update_sci(>acpi_regs, pm->irq);
> +}
> +
> +static const MemoryRegionOps ls7a_gpe_ops = {
> +.read = ls7a_gpe_readb,
> +.write = ls7a_gpe_writeb,
> +.valid.min_access_size = 1,
> +.valid.max_access_size = 8,
> +.impl.min_access_size = 1,
> +.impl.max_access_size = 1,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +#define VMSTATE_GPE_ARRAY(_field, _state)\
> + { \
> + .name = (stringify(_field)), \
> + .version_id = 0,\
> + .num= ACPI_GPE0_LEN,\
> + .info = _info_uint8, \
> + .size = sizeof(uint8_t), \
> + .flags = VMS_ARRAY | VMS_POINTER, \
> + .offset = vmstate_offset_pointer(_state, _field, uint8_t), \
> + }
> +
> +static uint64_t ls7a_reset_readw(void *opaque, hwaddr addr, unsigned width)
> +{
> +return 0;
> +}
> +
> +static void ls7a_reset_writew(void *opaque, hwaddr addr, uint64_t val,
> + unsigned width)
> +{
> +if (val & 1) {
> +qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +return;
> +}
> +}
> +
> +static const MemoryRegionOps ls7a_reset_ops = {
> +.read = ls7a_reset_readw,
> +.write = ls7a_reset_writew,
> +.valid.min_access_size = 4,
> +.valid.max_access_size = 4,
> +.endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +const VMStateDescription vmstate_ls7a_pm = {
> +.name = "ls7a_pm",
> +.version_id = 1,
> +.minimum_version_id = 1,
> +.fields = (VMStateField[]) {
> +VMSTATE_UINT16(acpi_regs.pm1.evt.sts, LS7APMState),
> +VMSTATE_UINT16(acpi_regs.pm1.evt.en, LS7APMState),
> +VMSTATE_UINT16(acpi_regs.pm1.cnt.cnt, LS7APMState),
> +VMSTATE_TIMER_PTR(acpi_regs.tmr.timer, LS7APMState),
> +
