Re: [PATCH v5 5/5] hw/isa/vt82c686: Implement software-based SMI triggering

[BALATON Zoltan]

On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> Am 28. Oktober 2023 13:03:41 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
> > On Sat, 28 Oct 2023, Bernhard Beschow wrote:
> > > If enabled, SMIs can be triggered via software by writing to an IO-mapped port.
> > > SMIs usually trigger execution of BIOS code. If appropriate values are written
> > > to the port, the BIOS transitions the system into or out of ACPI mode.
> > >
> > > Note that APMState implements Intel-specific behavior where there are two IO
> > > ports which are mapped at fixed addresses. In VIA, there is only one such port
> > > which is located inside a relocatable IO-mapped region. Hence, there is no point
> > > in reusing APMState.
> > >
> > > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > ---
> > > hw/isa/vt82c686.c | 95 +++++++++++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 87 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> > > index e8ec63dea9..361b3bed0a 100644
> > > --- a/hw/isa/vt82c686.c
> > > +++ b/hw/isa/vt82c686.c
> > > @@ -27,7 +27,6 @@
> > > #include "hw/timer/i8254.h"
> > > #include "hw/rtc/mc146818rtc.h"
> > > #include "migration/vmstate.h"
> > > -#include "hw/isa/apm.h"
> > > #include "hw/acpi/acpi.h"
> > > #include "hw/i2c/pm_smbus.h"
> > > #include "qapi/error.h"
> > > @@ -42,6 +41,16 @@
> > > #define TYPE_VIA_PM "via-pm"
> > > OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
> > >
> > > +#define VIA_PM_IO_GBLEN 0x2a
> > > +#define VIA_PM_IO_GBLEN_SW_SMI_EN (1 << 6)
> > > +
> > > +#define VIA_PM_IO_GBLCTL 0x2c
> > > +#define VIA_PM_IO_GBLCTL_SMI_EN 1
> > > +#define VIA_PM_IO_GBLCTL_SMIIG (1 << 4)
> > > +#define VIA_PM_IO_GBLCTL_INSMI (1 << 8)
> > > +
> > > +#define VIA_PM_IO_SMI_CMD 0x2f
> > > +
> > > #define VIA_PM_GPE_LEN 4
> > >
> > > #define VIA_PM_SCI_SELECT_OFS 0x42
> >
> > If we'll make a copy of the data sheet in form of #defines could these be in 
> > the header to less clutter the source?
>
> Last time I did that I was asked to move the defines back into the 
> source file. I can't find the link right now, otherwise I'd have placed 
> it here.

Yeah, the public header is probably not a good place for these either. 
Maybe add a local vt82c686_regs,h or similar private header in hw/isa next 
to the .c file for these? I'd just say we could use the constant values 
directly as reviewing them will need to look up the data sheet anyway but 
Philippe was in favour of adding defines for constants. This isn't a big 
deal though.

Regards,
BALATON Zoltan