Re: [PATCH v10 5/9] hw/fsi: Aspeed APB2OPB interface, Onchip perif bus

[Ninad Palsule]

Hello Cedric,


> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Nah. Joel you should re-review next respin :)
Removed Joel's review tag.
> > ---
> >   include/hw/misc/aspeed-apb2opb.h |  50 +++++
> >   hw/misc/aspeed-apb2opb.c         | 338 +++++++++++++++++++++++++++++++
>
> As said in the cover letter, I think now that hw/fsi is a better place
> for these files and should be compiled if CONSG_ASPEED_SOC. Sorry about
> that. Also,please use 'aspeed_' for the file names.
Transferred to the old location and rename with "aspeed_" prefix.
> > +
> > +#define TYPE_OP_BUS "opb"
> > +OBJECT_DECLARE_SIMPLE_TYPE(OPBus, OP_BUS)
> > +
> > +typedef struct OPBus {
> > +        /*< private >*/
>
> please remove the private and public comment.
Removed.
> > +        BusState bus;
> > +
> > +        /*< public >*/
> > +        MemoryRegion mr;
> > +        AddressSpace as;
>
> indent is wrong.
Fixed indent.
> > +static void fsi_aspeed_apb2opb_init(Object *o)
> > +{
> > +    AspeedAPB2OPBState *s = ASPEED_APB2OPB(o);
> > +    int i;
> > +
> > +    for (i = 0; i < ASPEED_FSI_NUM; i++) {
> > +        qbus_init(&s->opb[i], sizeof(s->opb[i]), TYPE_OP_BUS, 
> > DEVICE(s),
> > +                  NULL);
>
> See comment in fsi_opb_init()
Moved it to si_aspeed_apb2opb_realize(),
> > +    for (i = 0; i < ASPEED_FSI_NUM; i++) {
> > +        if (!qdev_realize(DEVICE(&s->fsi[i]), BUS(&s->opb[i]),
> > +                errp)) {
>
> this could be a single line.
Removed extra line.
> Please remove the comments below, I am not sure they are valid
> anymore.
Removed the comment.
> > +        /*
> > +         * Avoid endianness issues by mapping each slave's memory 
> > region
> > +         * directly. Manually bridging multiple address-spaces 
> > causes endian
> > +         * swapping headaches as memory_region_dispatch_read() and
> > +         * memory_region_dispatch_write() correct the endianness 
> > based on the
> > +         * target machine endianness and not relative to the device 
> > endianness
> > +         * on either side of the bridge.
> > +         */
> > +        /*
> > +         * XXX: This is a bit hairy and will need to be fixed when I 
> > sort out
> > +         * the bus/slave relationship and any changes to the CFAM 
> > modelling
> > +         * (multiple slaves, LBUS)
> > +         */
> > +        memory_region_add_subregion(&s->opb[i].mr, 0xa0000000,
> > +                &s->fsi[i].opb2fsi);
> > +    }
> > +}
> >
> > +
> > +static void fsi_opb_init(Object *o)
> > +{
> > +    OPBus *opb = OP_BUS(o);
> > +
> > +    memory_region_init_io(&opb->mr, OBJECT(opb), NULL, opb,
> > +                          TYPE_FSI_OPB, UINT32_MAX);
>
> This is better :
>
> memory_region_init(&opb->mr, o, TYPE_OP_BUS, UINT32_MAX);
Changed it as per your suggestion.
> > +    address_space_init(&opb->as, &opb->mr, TYPE_FSI_OPB);
>
> This routine is problematic. If you run 'make check', you should see
> test tests/qtest/device-introspect-test crash in weird way because of
> a memory corruption. I didn't dig into the details but I suppose this
> a use after free problem.
>
> To solve, we should move qbus_init() done in fsi_aspeed_apb2opb_init()
> under fsi_aspeed_apb2opb_realize(), or improve the model a litle more.
>
> It seems we are lacking the OPB/FSI bridge :
>
> typedef struct OPBFSIBridge {
>     DeviceState parent;
>
>     OPBus opb;
>     FSIMasterState fsi;
>     MemoryRegion mr;
>     AddressSpace as;
> } OPBFSIBridge;
>
> Something like that. It is difficult to understand the design from
> the OpenFSI specs. The OPB bus seems overkill. It you could clarify
> this aspect, it would be nice.

For now moved qbus_init() to fsi_aspeed_apb2opb_realize(). I will run 
make check.

Thanks for the review.

Regards,

Ninad