Re: [PATCH] spapr: nested: Add support for DPDES SPR in GSB for TCG L0

[Amit Machhiwal]

Hi Harsh,

Thanks for looking into this patch. My comments are below:

On 2024/10/18 02:13 PM, Harsh Prateek Bora wrote:
> Hi Amit,
> 
> On 10/18/24 10:47, Harsh Prateek Bora wrote:
> > Hi Amit,
> > 
> > On 10/17/24 16:30, Amit Machhiwal wrote:
> > > The DPDES support for doorbell emulation and handling for KVM on PAPR
> > > guests was added in Linux via [1]. Subsequently, a new GSB element for
> > > DPDES was added in Linux; the same has been missing in QEMU L0. Add
> > 
> > s/QEMU L0/ TCG L0 implementation?
> > 
> > > support for DPDES register's APIv2 GSB element and required handling in
> > > `spapr_nested.c`.
> > > 
> > > Currently, booting a KVM guest inside a QEMU TCG guest fails with the
> > > below crash. The crash is encountered when GUEST_RUN_VCPU hcall made
> > > into QEMU TCG L0 fails because H_INVALID_ELEMENT_VALUE is returned as
> > > the mapping of the element ID corresponding to DPDES (unknown to QEMU
> > > TCG L0) in GSR (Guest State Request) of TCG guest's KVM to the GSB
> > > (Guest State Buffer) elements of QEMU TCG L0 fails.
> > 
> > GSB full form would be more appropriate along with first mention of GSB
> > in above text.
> > 
> > With that:
> > Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> > 
> > > 
> 
> <snip>
> 
> > > diff --git a/hw/ppc/spapr_nested.c b/hw/ppc/spapr_nested.c
> > > index c02785756c1e..b696ad537a77 100644
> > > --- a/hw/ppc/spapr_nested.c
> > > +++ b/hw/ppc/spapr_nested.c
> > > @@ -194,6 +194,7 @@ static void nested_save_state(struct
> > > nested_ppc_state *save, PowerPCCPU *cpu)
> > >           save->fscr = env->spr[SPR_FSCR];
> > >           save->pspb = env->spr[SPR_PSPB];
> > >           save->ctrl = env->spr[SPR_CTRL];
> > > +        save->dpdes = env->spr[SPR_DPDES];
> > >           save->vrsave = env->spr[SPR_VRSAVE];
> > >           save->dar = env->spr[SPR_DAR];
> > >           save->dsisr = env->spr[SPR_DSISR];
> > > @@ -293,6 +294,7 @@ static void nested_load_state(PowerPCCPU *cpu,
> > > struct nested_ppc_state *load)
> > >           env->spr[SPR_FSCR] = load->fscr;
> > >           env->spr[SPR_PSPB] = load->pspb;
> > >           env->spr[SPR_CTRL] = load->ctrl;
> > > +        env->spr[SPR_DPDES] = load->dpdes;
> > >           env->spr[SPR_VRSAVE] = load->vrsave;
> > >           env->spr[SPR_DAR] = load->dar;
> > >           env->spr[SPR_DSISR] = load->dsisr;
> 
> Looks like we overlooked above inits already present in existing code.
> I just checked above inits in nested_{save,load}_state are already
> present as common code between apiv1 & apiv2. So, we only need the inits
> below for initializing the lookup table.

The common code already has that handled, indeed. I'll make this change and the
ones you posted in the other response and send out a V2. Thanks for pointing
this out.

Thanks,
Amit

> 
> regards,
> Harsh
> > > @@ -982,6 +984,7 @@ struct guest_state_element_type
> > > guest_state_element_types[] = {
> > >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_FSCR,  fscr),
> > >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_PSPB,   pspb),
> > >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_CTRL,  ctrl),
> > > +    GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_DPDES, dpdes),
> > >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_VRSAVE, vrsave),
> > >       GUEST_STATE_ELEMENT_ENV_DW(GSB_VCPU_SPR_DAR,   dar),
> > >       GUEST_STATE_ELEMENT_ENV_W(GSB_VCPU_SPR_DSISR,  dsisr),
> > > diff --git a/include/hw/ppc/spapr_nested.h
> > > b/include/hw/ppc/spapr_nested.h
> > > index 93ef14adcc5e..3b5cd993c256 100644
> > > --- a/include/hw/ppc/spapr_nested.h
> > > +++ b/include/hw/ppc/spapr_nested.h
> > > @@ -99,7 +99,8 @@
> > >   #define GSB_VCPU_SPR_HASHKEYR   0x1050
> > >   #define GSB_VCPU_SPR_HASHPKEYR  0x1051
> > >   #define GSB_VCPU_SPR_CTRL       0x1052
> > > -                    /* RESERVED 0x1053 - 0x1FFF */
> > > +#define GSB_VCPU_SPR_DPDES      0x1053
> > > +                    /* RESERVED 0x1054 - 0x1FFF */
> > >   #define GSB_VCPU_SPR_CR         0x2000
> > >   #define GSB_VCPU_SPR_PIDR       0x2001
> > >   #define GSB_VCPU_SPR_DSISR      0x2002
> > > 
> > > base-commit: aa54f5be44be786636a5d51cc1612ad208a24849
> >