| Message ID | 20220110143346.455901-5-danielhb413@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | user creatable pnv-phb4 devices | expand |
On 1/10/22 15:33, Daniel Henrique Barboza wrote: > This alias is a indirect way of setting stack->phb->index. Since we have > access to a valid stack->phb (for default_enabled() at least - next > patch will deal with it accordingly) we can directly set the phb 'index' > attribute. > > Let's also take the opportunity to explain why we're having to deal with > stack->phb attributes during pec_realize(). > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c > index 1f264d0a9c..417fac4cef 100644 > --- a/hw/pci-host/pnv_phb4_pec.c > +++ b/hw/pci-host/pnv_phb4_pec.c > @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) > int phb_id = pnv_phb4_pec_get_phb_id(pec, i); > > object_property_set_int(stk_obj, "stack-no", i, &error_abort); > - object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort); > object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort); > + > + /* > + * stack->phb->index is dependent on the position the > + * stack occupies in pec->stacks[]. We have this information > + * available here via the 'i' iterator so it's convenient to > + * do it now. > + */ > + object_property_set_int(OBJECT(&stack->phb), "index", phb_id, > + &error_abort); I don't like the fact that we are exposing ->phb under the PEC model. It looks like this is going to be a problem afterwards when defaults are disabled. We should move the setting of the PHB ID under pnv_pec_stk_realize() before the PHB is realized and compute the id with : int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no); Thanks, C. > + > if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) { > return; > } > @@ -534,7 +543,6 @@ static void pnv_pec_stk_instance_init(Object *obj) > PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj); > > object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4); > - object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index"); > } > > static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) >
On 1/10/22 12:49, Cédric Le Goater wrote: > On 1/10/22 15:33, Daniel Henrique Barboza wrote: >> This alias is a indirect way of setting stack->phb->index. Since we have >> access to a valid stack->phb (for default_enabled() at least - next >> patch will deal with it accordingly) we can directly set the phb 'index' >> attribute. >> >> Let's also take the opportunity to explain why we're having to deal with >> stack->phb attributes during pec_realize(). >> >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >> index 1f264d0a9c..417fac4cef 100644 >> --- a/hw/pci-host/pnv_phb4_pec.c >> +++ b/hw/pci-host/pnv_phb4_pec.c >> @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) >> int phb_id = pnv_phb4_pec_get_phb_id(pec, i); >> object_property_set_int(stk_obj, "stack-no", i, &error_abort); >> - object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort); >> object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort); >> + >> + /* >> + * stack->phb->index is dependent on the position the >> + * stack occupies in pec->stacks[]. We have this information >> + * available here via the 'i' iterator so it's convenient to >> + * do it now. >> + */ >> + object_property_set_int(OBJECT(&stack->phb), "index", phb_id, >> + &error_abort); > > I don't like the fact that we are exposing ->phb under the PEC model. > It looks like this is going to be a problem afterwards when defaults > are disabled. > > We should move the setting of the PHB ID under pnv_pec_stk_realize() > before the PHB is realized and compute the id with : > > int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no); Oh, if stack->stack_no is the stack index of the pec->stacks[] array then we should instead move all this stuff into phb4_realize(). Daniel > > Thanks, > > C. > >> + >> if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) { >> return; >> } >> @@ -534,7 +543,6 @@ static void pnv_pec_stk_instance_init(Object *obj) >> PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj); >> object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4); >> - object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index"); >> } >> static void pnv_pec_stk_realize(DeviceState *dev, Error **errp) >> >
On 1/10/22 17:27, Daniel Henrique Barboza wrote: > > > On 1/10/22 12:49, Cédric Le Goater wrote: >> On 1/10/22 15:33, Daniel Henrique Barboza wrote: >>> This alias is a indirect way of setting stack->phb->index. Since we have >>> access to a valid stack->phb (for default_enabled() at least - next >>> patch will deal with it accordingly) we can directly set the phb 'index' >>> attribute. >>> >>> Let's also take the opportunity to explain why we're having to deal with >>> stack->phb attributes during pec_realize(). >>> >>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >>> --- >>> hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++-- >>> 1 file changed, 10 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c >>> index 1f264d0a9c..417fac4cef 100644 >>> --- a/hw/pci-host/pnv_phb4_pec.c >>> +++ b/hw/pci-host/pnv_phb4_pec.c >>> @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) >>> int phb_id = pnv_phb4_pec_get_phb_id(pec, i); >>> object_property_set_int(stk_obj, "stack-no", i, &error_abort); >>> - object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort); >>> object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort); >>> + >>> + /* >>> + * stack->phb->index is dependent on the position the >>> + * stack occupies in pec->stacks[]. We have this information >>> + * available here via the 'i' iterator so it's convenient to >>> + * do it now. >>> + */ >>> + object_property_set_int(OBJECT(&stack->phb), "index", phb_id, >>> + &error_abort); >> >> I don't like the fact that we are exposing ->phb under the PEC model. >> It looks like this is going to be a problem afterwards when defaults >> are disabled. >> >> We should move the setting of the PHB ID under pnv_pec_stk_realize() >> before the PHB is realized and compute the id with : >> >> int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no); > > > Oh, if stack->stack_no is the stack index of the pec->stacks[] array then we should > instead move all this stuff into phb4_realize(). The properties are generally set by the parent object and for user created PHB4 devices, the user will set the PHB4 "index" property which we will use to do a lookup of the owning stack. It's similar to chip-id. Thanks, C.
diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c index 1f264d0a9c..417fac4cef 100644 --- a/hw/pci-host/pnv_phb4_pec.c +++ b/hw/pci-host/pnv_phb4_pec.c @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp) int phb_id = pnv_phb4_pec_get_phb_id(pec, i); object_property_set_int(stk_obj, "stack-no", i, &error_abort); - object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort); object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort); + + /* + * stack->phb->index is dependent on the position the + * stack occupies in pec->stacks[]. We have this information + * available here via the 'i' iterator so it's convenient to + * do it now. + */ + object_property_set_int(OBJECT(&stack->phb), "index", phb_id, + &error_abort); + if (!qdev_realize(DEVICE(stk_obj), NULL, errp)) { return; } @@ -534,7 +543,6 @@ static void pnv_pec_stk_instance_init(Object *obj) PnvPhb4PecStack *stack = PNV_PHB4_PEC_STACK(obj); object_initialize_child(obj, "phb", &stack->phb, TYPE_PNV_PHB4); - object_property_add_alias(obj, "phb-id", OBJECT(&stack->phb), "index"); } static void pnv_pec_stk_realize(DeviceState *dev, Error **errp)
This alias is a indirect way of setting stack->phb->index. Since we have access to a valid stack->phb (for default_enabled() at least - next patch will deal with it accordingly) we can directly set the phb 'index' attribute. Let's also take the opportunity to explain why we're having to deal with stack->phb attributes during pec_realize(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)