Re: [PATCH RESEND 10/15] ppc: spapr: Initialize the GSB Elements lookup table.

[Harsh Prateek Bora]

On 9/7/23 08:31, Nicholas Piggin wrote:
> Might be good to add a common nested: prefix to all patches actually.
Noted.

> On Wed Sep 6, 2023 at 2:33 PM AEST, Harsh Prateek Bora wrote:
> > This is a first step towards enabling support for nested PAPR hcalls for
> > providing the get/set of various Guest State Buffer (GSB) elements via
> > h_guest_[g|s]et_state hcalls. This enables for identifying correct
> > callbacks for get/set for each of the elements supported via
> > h_guest_[g|s]et_state hcalls, support for which is added in next patch.
>
> Changelog could use work.
Sure, will update.

> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > ---
> >   hw/ppc/spapr_hcall.c          |   1 +
> >   hw/ppc/spapr_nested.c         | 487 ++++++++++++++++++++++++++++++++++
> >   include/hw/ppc/ppc.h          |   2 +
> >   include/hw/ppc/spapr_nested.h | 102 +++++++
> >   4 files changed, 592 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 9b1f225d4a..ca609cb5a4 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1580,6 +1580,7 @@ static void hypercall_register_types(void)
> >       spapr_register_hypercall(KVMPPC_H_UPDATE_DT, h_update_dt);
> >   
> >       spapr_register_nested();
> > +    init_nested();
>
> This is for hcall registration, not general subsystem init I think.
> Arguably not sure if it matters, it just looks odd for everything
> else to be an hcall except this. I would just add a new init
> function.

I have introduced a new spapr_nested_init routine in spapr_nested.c 
which shall be called from spapr_instance_init. I think we can move GSB 
init there.

> And actually now I look closer at this, I would not do your papr
> hcall init in the cap apply function, if it is possible to do
> inside spapr_register_nested(), then that function could look at
> which caps are enabled and register the appropriate hcalls. Then
> no change to move this into cap code.

IIRC, I had initially tried that during early development but faced 
runtime issues with spapr init at this stage, which is needed to 
identify nested.api. However, keeping cap specific registration in cap
apply function made more sense to me. Further optimizations can be taken 
up later though.

> >   }
> >   
> >   type_init(hypercall_register_types)
> > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > index e7956685af..6fbb1bcb02 100644
> > --- a/hw/ppc/spapr_nested.c
> > +++ b/hw/ppc/spapr_nested.c
>
> [snip]
>
> My eyes are going square, I'll review this later.

Sure.

> > diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> > index e095c002dc..d7acc28d17 100644
> > --- a/include/hw/ppc/ppc.h
> > +++ b/include/hw/ppc/ppc.h
> > @@ -33,6 +33,8 @@ struct ppc_tb_t {
> >       QEMUTimer *decr_timer;
> >       /* Hypervisor decrementer management */
> >       uint64_t hdecr_next;    /* Tick for next hdecr interrupt  */
> > +    /* TB that HDEC should fire and return ctrl back to the Host partition */
> > +    uint64_t hdecr_expiry_tb;
>
> Why is this here?

Since there is an existing hypervisor decrementer related variable, it 
appeared appropriate to me to keep it there. Will move it inside
SpaprMachineStateNestedGuestVcpu if that sounds better.

> >       QEMUTimer *hdecr_timer;
> >       int64_t purr_offset;
> >       void *opaque;
> > diff --git a/include/hw/ppc/spapr_nested.h b/include/hw/ppc/spapr_nested.h
> > index 2e8c6ba1ca..3c0d6a486e 100644
> > --- a/include/hw/ppc/spapr_nested.h
> > +++ b/include/hw/ppc/spapr_nested.h
>
> [snip]
>
> >   
> > +struct guest_state_element_type {
> > +    uint16_t id;
> > +    int size;
> > +#define GUEST_STATE_ELEMENT_TYPE_FLAG_GUEST_WIDE 0x1
> > +#define GUEST_STATE_ELEMENT_TYPE_FLAG_READ_ONLY  0x2
> > +   uint16_t flags;
> > +    void *(*location)(SpaprMachineStateNestedGuest *, target_ulong);
> > +    size_t offset;
> > +    void (*copy)(void *, void *, bool);
> > +    uint64_t mask;
> > +};
>
> I have to wonder whether this is the best way to go. Having
> these indicrect function calls and array of "ops" like this
> might be limiting the compiler. I wonder if it should just
> be done in a switch table, which is how most interpreters
> I've seen (which admittedly is not many) seem to do it.
Hmm, this was chosen after evaluating other approaches as it appeared
better. I think we can move forward with the existing approach and any
further optimizations can be taken up as a follow-up patch.

regards,
Harsh

> Thanks,
> Nick