| Message ID | 20191219180205.25191-1-felipe@nutanix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve default object property_add uint helpers | expand |
On 20/12/2019 05:02, Felipe Franciosi wrote: > This improves the family of object_property_add_uintXX_ptr helpers by enabling > a default getter/setter only when desired. To prevent an API behavioural change > (from clients that already used these helpers and did not want a setter), we > add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 > enhances the API and modify current users. > > While modifying the clients of the API, a couple of improvement opportunities > were observed in ich9. These were added in separate patches (2 and 3). > > Patch 3 cleans up a lot of existing code by moving various objects to the > enhanced API. Previously, those objects had their own getters/setters that only > updated the values without further checks. Some of them actually lacked a check > for setting overflows, which could have resulted in undesired values being set. > The new default setters include a check for that, not updating the values in > case of errors (and propagating them). If they did not provide an error > pointer, then that behaviour was maintained. A weird thing happens - when I apply patches from my mailer (thunderbird -> open the source -> cut-n-paste to "git am") - they fail to apply. And the mails themselves look suspicious - too many "MS-Exchange" and "X-Proofpoint" :) A bundle from https://patchwork.ozlabs.org/project/qemu-devel/list/?series=149673 applies fine though. Anyway, this works on powerpc. Thanks, > > Felipe Franciosi (4): > qom/object: enable setter for uint types > ich9: fix getter type for sci_int property > ich9: Simplify ich9_lpc_initfn > qom/object: Use common get/set uint helpers > > hw/acpi/ich9.c | 99 ++------------------ > hw/acpi/pcihp.c | 7 +- > hw/acpi/piix4.c | 12 +-- > hw/isa/lpc_ich9.c | 27 ++---- > hw/misc/edu.c | 13 +-- > hw/pci-host/q35.c | 14 +-- > hw/ppc/spapr.c | 18 +--- > hw/ppc/spapr_drc.c | 3 +- > include/qom/object.h | 44 +++++++-- > memory.c | 15 +-- > qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- > target/arm/cpu.c | 22 +---- > target/i386/sev.c | 106 ++------------------- > ui/console.c | 4 +- > 14 files changed, 282 insertions(+), 318 deletions(-) >
On Thu, Dec 19, 2019 at 06:02:28PM +0000, Felipe Franciosi wrote: > This improves the family of object_property_add_uintXX_ptr helpers by enabling > a default getter/setter only when desired. To prevent an API behavioural change > (from clients that already used these helpers and did not want a setter), we > add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 > enhances the API and modify current users. > > While modifying the clients of the API, a couple of improvement opportunities > were observed in ich9. These were added in separate patches (2 and 3). > > Patch 3 cleans up a lot of existing code by moving various objects to the > enhanced API. Previously, those objects had their own getters/setters that only > updated the values without further checks. Some of them actually lacked a check > for setting overflows, which could have resulted in undesired values being set. > The new default setters include a check for that, not updating the values in > case of errors (and propagating them). If they did not provide an error > pointer, then that behaviour was maintained. > > Felipe Franciosi (4): > qom/object: enable setter for uint types > ich9: fix getter type for sci_int property > ich9: Simplify ich9_lpc_initfn > qom/object: Use common get/set uint helpers > > hw/acpi/ich9.c | 99 ++------------------ > hw/acpi/pcihp.c | 7 +- > hw/acpi/piix4.c | 12 +-- > hw/isa/lpc_ich9.c | 27 ++---- > hw/misc/edu.c | 13 +-- > hw/pci-host/q35.c | 14 +-- > hw/ppc/spapr.c | 18 +--- > hw/ppc/spapr_drc.c | 3 +- > include/qom/object.h | 44 +++++++-- > memory.c | 15 +-- > qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- > target/arm/cpu.c | 22 +---- > target/i386/sev.c | 106 ++------------------- > ui/console.c | 4 +- > 14 files changed, 282 insertions(+), 318 deletions(-) > > -- > 2.20.1 > > Changelog: > v1->v2: > - Update sci_int directly instead of using stack variable > - Defining an enhanced ObjectPropertyFlags instead of just 'readonly' > - Erroring out directly (instead of using gotos) on default setters > - Retaining lack of errp passing when it wasn't there > v2->v3: > - Rename flags _RD to _READ and _WR to _WRITE > - Add a convenience _READWRITE flag > - Drop the usage of UL in the bit flag definitions > v3->v4: > - Drop changes to hw/vfio/pci-quirks.c Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Hi, > On Dec 19, 2019, at 11:56 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > > > > On 20/12/2019 05:02, Felipe Franciosi wrote: >> This improves the family of object_property_add_uintXX_ptr helpers by enabling >> a default getter/setter only when desired. To prevent an API behavioural change >> (from clients that already used these helpers and did not want a setter), we >> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 >> enhances the API and modify current users. >> >> While modifying the clients of the API, a couple of improvement opportunities >> were observed in ich9. These were added in separate patches (2 and 3). >> >> Patch 3 cleans up a lot of existing code by moving various objects to the >> enhanced API. Previously, those objects had their own getters/setters that only >> updated the values without further checks. Some of them actually lacked a check >> for setting overflows, which could have resulted in undesired values being set. >> The new default setters include a check for that, not updating the values in >> case of errors (and propagating them). If they did not provide an error >> pointer, then that behaviour was maintained. > > A weird thing happens - when I apply patches from my mailer (thunderbird > -> open the source -> cut-n-paste to "git am") - they fail to apply. And > the mails themselves look suspicious - too many "MS-Exchange" and > "X-Proofpoint" :) I apologise for that... as you can see from below, our company's "anti-spam" / "anti-virus" mail servers tend to mangle incoming e-mails in ways that make it challenging to work with MLs: > > A bundle from > https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_project_qemu-2Ddevel_list_-3Fseries-3D149673&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=CCrJKVC5zGot8PrnI-iYe00MdX4pgdQfMRigp14Ptmk&m=TxZxCvPIyfiAMkXeqOwUO_oYhAWFHAG66jA3SeEldzU&s=vMLzvHMGCJ9VeW3wSKvJVPrthKmVoud-ACf5eh6w2Rg&e= > applies fine though. Now I know why both you and Marc-Andre had problems applying my patches: apparently our servers also mangle with outgoing e-mails too. I heard that pulling the patches from mbox work, but I'll make sure to post patches on Github in the future to make things easier for others. I've already asked them to look into this and whitelist e-mails on various criteria. Thanks for your help in making sure these patches work for you despite the extra hurdles! F. > > > Anyway, this works on powerpc. Thanks, > > > >> >> Felipe Franciosi (4): >> qom/object: enable setter for uint types >> ich9: fix getter type for sci_int property >> ich9: Simplify ich9_lpc_initfn >> qom/object: Use common get/set uint helpers >> >> hw/acpi/ich9.c | 99 ++------------------ >> hw/acpi/pcihp.c | 7 +- >> hw/acpi/piix4.c | 12 +-- >> hw/isa/lpc_ich9.c | 27 ++---- >> hw/misc/edu.c | 13 +-- >> hw/pci-host/q35.c | 14 +-- >> hw/ppc/spapr.c | 18 +--- >> hw/ppc/spapr_drc.c | 3 +- >> include/qom/object.h | 44 +++++++-- >> memory.c | 15 +-- >> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- >> target/arm/cpu.c | 22 +---- >> target/i386/sev.c | 106 ++------------------- >> ui/console.c | 4 +- >> 14 files changed, 282 insertions(+), 318 deletions(-) >> > > -- > Alexey
Hi On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi <felipe@nutanix.com> wrote: > > This improves the family of object_property_add_uintXX_ptr helpers by enabling > a default getter/setter only when desired. To prevent an API behavioural change > (from clients that already used these helpers and did not want a setter), we > add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 > enhances the API and modify current users. > > While modifying the clients of the API, a couple of improvement opportunities > were observed in ich9. These were added in separate patches (2 and 3). > > Patch 3 cleans up a lot of existing code by moving various objects to the > enhanced API. Previously, those objects had their own getters/setters that only > updated the values without further checks. Some of them actually lacked a check > for setting overflows, which could have resulted in undesired values being set. > The new default setters include a check for that, not updating the values in > case of errors (and propagating them). If they did not provide an error > pointer, then that behaviour was maintained. > > Felipe Franciosi (4): > qom/object: enable setter for uint types > ich9: fix getter type for sci_int property > ich9: Simplify ich9_lpc_initfn > qom/object: Use common get/set uint helpers > > hw/acpi/ich9.c | 99 ++------------------ > hw/acpi/pcihp.c | 7 +- > hw/acpi/piix4.c | 12 +-- > hw/isa/lpc_ich9.c | 27 ++---- > hw/misc/edu.c | 13 +-- > hw/pci-host/q35.c | 14 +-- > hw/ppc/spapr.c | 18 +--- > hw/ppc/spapr_drc.c | 3 +- > include/qom/object.h | 44 +++++++-- > memory.c | 15 +-- > qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- > target/arm/cpu.c | 22 +---- > target/i386/sev.c | 106 ++------------------- > ui/console.c | 4 +- > 14 files changed, 282 insertions(+), 318 deletions(-) It conflicts with some recent changes, so you'll need to send a new version, but that one looks good to me: Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Paolo, is it going through your queue? > > -- > 2.20.1 > > Changelog: > v1->v2: > - Update sci_int directly instead of using stack variable > - Defining an enhanced ObjectPropertyFlags instead of just 'readonly' > - Erroring out directly (instead of using gotos) on default setters > - Retaining lack of errp passing when it wasn't there > v2->v3: > - Rename flags _RD to _READ and _WR to _WRITE > - Add a convenience _READWRITE flag > - Drop the usage of UL in the bit flag definitions > v3->v4: > - Drop changes to hw/vfio/pci-quirks.c
> On Dec 20, 2019, at 3:15 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi <felipe@nutanix.com> wrote: >> >> This improves the family of object_property_add_uintXX_ptr helpers by enabling >> a default getter/setter only when desired. To prevent an API behavioural change >> (from clients that already used these helpers and did not want a setter), we >> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 >> enhances the API and modify current users. >> >> While modifying the clients of the API, a couple of improvement opportunities >> were observed in ich9. These were added in separate patches (2 and 3). >> >> Patch 3 cleans up a lot of existing code by moving various objects to the >> enhanced API. Previously, those objects had their own getters/setters that only >> updated the values without further checks. Some of them actually lacked a check >> for setting overflows, which could have resulted in undesired values being set. >> The new default setters include a check for that, not updating the values in >> case of errors (and propagating them). If they did not provide an error >> pointer, then that behaviour was maintained. >> >> Felipe Franciosi (4): >> qom/object: enable setter for uint types >> ich9: fix getter type for sci_int property >> ich9: Simplify ich9_lpc_initfn >> qom/object: Use common get/set uint helpers >> >> hw/acpi/ich9.c | 99 ++------------------ >> hw/acpi/pcihp.c | 7 +- >> hw/acpi/piix4.c | 12 +-- >> hw/isa/lpc_ich9.c | 27 ++---- >> hw/misc/edu.c | 13 +-- >> hw/pci-host/q35.c | 14 +-- >> hw/ppc/spapr.c | 18 +--- >> hw/ppc/spapr_drc.c | 3 +- >> include/qom/object.h | 44 +++++++-- >> memory.c | 15 +-- >> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- >> target/arm/cpu.c | 22 +---- >> target/i386/sev.c | 106 ++------------------- >> ui/console.c | 4 +- >> 14 files changed, 282 insertions(+), 318 deletions(-) > > It conflicts with some recent changes, so you'll need to send a new > version, but that one looks good to me: > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Ah, no worries. I did do a rebase on top of master before sending out v4, though. Which conflict did you see? I can rebase later or maybe whomever is pulling can fix the merge if it's straightforward? F. > > Paolo, is it going through your queue? > >> >> -- >> 2.20.1 >> >> Changelog: >> v1->v2: >> - Update sci_int directly instead of using stack variable >> - Defining an enhanced ObjectPropertyFlags instead of just 'readonly' >> - Erroring out directly (instead of using gotos) on default setters >> - Retaining lack of errp passing when it wasn't there >> v2->v3: >> - Rename flags _RD to _READ and _WR to _WRITE >> - Add a convenience _READWRITE flag >> - Drop the usage of UL in the bit flag definitions >> v3->v4: >> - Drop changes to hw/vfio/pci-quirks.c > > > > -- > Marc-André Lureau
Hi Marc-Andre and Paolo, > On Dec 20, 2019, at 3:15 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi > > On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi <felipe@nutanix.com> wrote: >> >> This improves the family of object_property_add_uintXX_ptr helpers by enabling >> a default getter/setter only when desired. To prevent an API behavioural change >> (from clients that already used these helpers and did not want a setter), we >> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 >> enhances the API and modify current users. >> >> While modifying the clients of the API, a couple of improvement opportunities >> were observed in ich9. These were added in separate patches (2 and 3). >> >> Patch 3 cleans up a lot of existing code by moving various objects to the >> enhanced API. Previously, those objects had their own getters/setters that only >> updated the values without further checks. Some of them actually lacked a check >> for setting overflows, which could have resulted in undesired values being set. >> The new default setters include a check for that, not updating the values in >> case of errors (and propagating them). If they did not provide an error >> pointer, then that behaviour was maintained. >> >> Felipe Franciosi (4): >> qom/object: enable setter for uint types >> ich9: fix getter type for sci_int property >> ich9: Simplify ich9_lpc_initfn >> qom/object: Use common get/set uint helpers >> >> hw/acpi/ich9.c | 99 ++------------------ >> hw/acpi/pcihp.c | 7 +- >> hw/acpi/piix4.c | 12 +-- >> hw/isa/lpc_ich9.c | 27 ++---- >> hw/misc/edu.c | 13 +-- >> hw/pci-host/q35.c | 14 +-- >> hw/ppc/spapr.c | 18 +--- >> hw/ppc/spapr_drc.c | 3 +- >> include/qom/object.h | 44 +++++++-- >> memory.c | 15 +-- >> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- >> target/arm/cpu.c | 22 +---- >> target/i386/sev.c | 106 ++------------------- >> ui/console.c | 4 +- >> 14 files changed, 282 insertions(+), 318 deletions(-) > > It conflicts with some recent changes, so you'll need to send a new > version, but that one looks good to me: > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Paolo, is it going through your queue? I didn't see any response after this. Did the series get lost? Cheers, Felipe > >> >> -- >> 2.20.1 >> >> Changelog: >> v1->v2: >> - Update sci_int directly instead of using stack variable >> - Defining an enhanced ObjectPropertyFlags instead of just 'readonly' >> - Erroring out directly (instead of using gotos) on default setters >> - Retaining lack of errp passing when it wasn't there >> v2->v3: >> - Rename flags _RD to _READ and _WR to _WRITE >> - Add a convenience _READWRITE flag >> - Drop the usage of UL in the bit flag definitions >> v3->v4: >> - Drop changes to hw/vfio/pci-quirks.c > > > > -- > Marc-André Lureau
Hi Felipe, On Fri, Jan 24, 2020 at 11:49 AM Felipe Franciosi <felipe@nutanix.com> wrote: > > Hi Marc-Andre and Paolo, > > > On Dec 20, 2019, at 3:15 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > > > Hi > > > > On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi <felipe@nutanix.com> wrote: > >> > >> This improves the family of object_property_add_uintXX_ptr helpers by enabling > >> a default getter/setter only when desired. To prevent an API behavioural change > >> (from clients that already used these helpers and did not want a setter), we > >> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 > >> enhances the API and modify current users. > >> > >> While modifying the clients of the API, a couple of improvement opportunities > >> were observed in ich9. These were added in separate patches (2 and 3). > >> > >> Patch 3 cleans up a lot of existing code by moving various objects to the > >> enhanced API. Previously, those objects had their own getters/setters that only > >> updated the values without further checks. Some of them actually lacked a check > >> for setting overflows, which could have resulted in undesired values being set. > >> The new default setters include a check for that, not updating the values in > >> case of errors (and propagating them). If they did not provide an error > >> pointer, then that behaviour was maintained. > >> > >> Felipe Franciosi (4): > >> qom/object: enable setter for uint types > >> ich9: fix getter type for sci_int property > >> ich9: Simplify ich9_lpc_initfn > >> qom/object: Use common get/set uint helpers > >> > >> hw/acpi/ich9.c | 99 ++------------------ > >> hw/acpi/pcihp.c | 7 +- > >> hw/acpi/piix4.c | 12 +-- > >> hw/isa/lpc_ich9.c | 27 ++---- > >> hw/misc/edu.c | 13 +-- > >> hw/pci-host/q35.c | 14 +-- > >> hw/ppc/spapr.c | 18 +--- > >> hw/ppc/spapr_drc.c | 3 +- > >> include/qom/object.h | 44 +++++++-- > >> memory.c | 15 +-- > >> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- > >> target/arm/cpu.c | 22 +---- > >> target/i386/sev.c | 106 ++------------------- > >> ui/console.c | 4 +- > >> 14 files changed, 282 insertions(+), 318 deletions(-) > > > > It conflicts with some recent changes, so you'll need to send a new > > version, but that one looks good to me: > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Paolo, is it going through your queue? > > I didn't see any response after this. Did the series get lost? Can you send a rebased version? thanks
Heya, > On Jan 28, 2020, at 3:54 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > > Hi Felipe, > > On Fri, Jan 24, 2020 at 11:49 AM Felipe Franciosi <felipe@nutanix.com> wrote: >> >> Hi Marc-Andre and Paolo, >> >>> On Dec 20, 2019, at 3:15 PM, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: >>> >>> Hi >>> >>> On Thu, Dec 19, 2019 at 10:02 PM Felipe Franciosi <felipe@nutanix.com> wrote: >>>> >>>> This improves the family of object_property_add_uintXX_ptr helpers by enabling >>>> a default getter/setter only when desired. To prevent an API behavioural change >>>> (from clients that already used these helpers and did not want a setter), we >>>> add a OBJ_PROP_FLAG_READ flag that allow clients to only have a getter. Patch 1 >>>> enhances the API and modify current users. >>>> >>>> While modifying the clients of the API, a couple of improvement opportunities >>>> were observed in ich9. These were added in separate patches (2 and 3). >>>> >>>> Patch 3 cleans up a lot of existing code by moving various objects to the >>>> enhanced API. Previously, those objects had their own getters/setters that only >>>> updated the values without further checks. Some of them actually lacked a check >>>> for setting overflows, which could have resulted in undesired values being set. >>>> The new default setters include a check for that, not updating the values in >>>> case of errors (and propagating them). If they did not provide an error >>>> pointer, then that behaviour was maintained. >>>> >>>> Felipe Franciosi (4): >>>> qom/object: enable setter for uint types >>>> ich9: fix getter type for sci_int property >>>> ich9: Simplify ich9_lpc_initfn >>>> qom/object: Use common get/set uint helpers >>>> >>>> hw/acpi/ich9.c | 99 ++------------------ >>>> hw/acpi/pcihp.c | 7 +- >>>> hw/acpi/piix4.c | 12 +-- >>>> hw/isa/lpc_ich9.c | 27 ++---- >>>> hw/misc/edu.c | 13 +-- >>>> hw/pci-host/q35.c | 14 +-- >>>> hw/ppc/spapr.c | 18 +--- >>>> hw/ppc/spapr_drc.c | 3 +- >>>> include/qom/object.h | 44 +++++++-- >>>> memory.c | 15 +-- >>>> qom/object.c | 216 ++++++++++++++++++++++++++++++++++++++----- >>>> target/arm/cpu.c | 22 +---- >>>> target/i386/sev.c | 106 ++------------------- >>>> ui/console.c | 4 +- >>>> 14 files changed, 282 insertions(+), 318 deletions(-) >>> >>> It conflicts with some recent changes, so you'll need to send a new >>> version, but that one looks good to me: >>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> >>> Paolo, is it going through your queue? >> >> I didn't see any response after this. Did the series get lost? > > Can you send a rebased version? Sorry for the delay as I was on travels. Done. F. > > thanks > > > > -- > Marc-André Lureau