Re: [PATCH v8 8/8] hw/riscv/virt: Add IOPMP support

[Ethan Chen]

On Mon, Aug 12, 2024 at 10:48:40AM +1000, Alistair Francis wrote:
> [EXTERNAL MAIL]
> 
> On Fri, Aug 9, 2024 at 8:14 PM Ethan Chen <ethan84@andestech.com> wrote:
> >
> > On Thu, Aug 08, 2024 at 02:01:13PM +1000, Alistair Francis wrote:
> > >
> > > On Mon, Jul 15, 2024 at 8:15 PM Ethan Chen via <qemu-devel@nongnu.org> 
> > > wrote:
> > > >
> > > > - Add 'iopmp=on' option to enable IOPMP. It adds an iopmp device virt 
> > > > machine
> > > >   to protect all regions of system memory, and configures RRID of CPU.
> > > >
> > > > Signed-off-by: Ethan Chen <ethan84@andestech.com>
> > > > ---
> > > >  docs/system/riscv/virt.rst |  5 +++
> > > >  hw/riscv/Kconfig           |  1 +
> > > >  hw/riscv/virt.c            | 63 ++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/riscv/virt.h    |  5 ++-
> > > >  4 files changed, 73 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/docs/system/riscv/virt.rst b/docs/system/riscv/virt.rst
> > > > index 9a06f95a34..9fd006ccc2 100644
> > > > --- a/docs/system/riscv/virt.rst
> > > > +++ b/docs/system/riscv/virt.rst
> > > > @@ -116,6 +116,11 @@ The following machine-specific options are 
> > > > supported:
> > > >    having AIA IMSIC (i.e. "aia=aplic-imsic" selected). When not 
> > > > specified,
> > > >    the default number of per-HART VS-level AIA IMSIC pages is 0.
> > > >
> > > > +- iopmp=[on|off]
> > > > +
> > > > +  When this option is "on", an IOPMP device is added to machine. IOPMP 
> > > > checks
> > > > +  memory transcations in system memory. This option is assumed to be 
> > > > "off".
> > >
> > > We probably should have a a little more here. You don't even mention
> > > that this is the rapid-k model.
> >
> > I'll provide more details.
> >
> > >
> > > It might be worth adding a `model` field, to make it easier to add
> > > other models in the future. Thoughts?
> > >
> >
> > I think the IOPMP model should be a device property and not
> > configured here.
> 
> It should be a device property, but then how does a user configure
> that? I guess users can globally set device props, but it's a bit
> clunky
>

Because IOPMP has a lot props, I think it is better to configure them 
through global device props instead of machine option.

Thanks,
Ethan Chen