| Message ID | CAEgOgz5CGekoYvEuTN1519tVxu1gPgGO11m8C25C3xK2hdBCfQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: > diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c > index 0c39cff..ae09170 100644 > --- a/hw/timer/xilinx_timer.c > +++ b/hw/timer/xilinx_timer.c > @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) > ptimer_set_freq(xt->ptimer, t->freq_hz); > } > > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, > R_MAX * 4 * num_timers(t)); > sysbus_init_mmio(dev, &t->mmio); > return 0; > @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass Isn't this a false positive? Paolo
Hi Paolo, On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: >> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c >> index 0c39cff..ae09170 100644 >> --- a/hw/timer/xilinx_timer.c >> +++ b/hw/timer/xilinx_timer.c >> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) >> ptimer_set_freq(xt->ptimer, t->freq_hz); >> } >> >> - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", >> + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, >> R_MAX * 4 * num_timers(t)); >> sysbus_init_mmio(dev, &t->mmio); >> return 0; >> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass > > Isn't this a false positive? > Not really, For consistency I just the TYPE_FOO for the memory region name, so I don't see why that shouldn't be macrofiied along with the rest. It is quite deliberately the same - please advise if this is wrong. Same for the VMSD name. Slightly off topic, should we perhaps use the object canonical patch for the device as the memory region name string?: - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", + memory_region_init_io(&t->mmio, &timer_ops, t, object_get_canonical_path(OBJECT(dev)), R_MAX * 4 * num_timers(t)); Its unambiguous and solves the problem where you have two-of-a-kind devices in the one system and you need to differentiate. Regards, Peter > Paolo >
On Fri, Jun 21, 2013 at 09:36:46AM +0200, Paolo Bonzini wrote: > Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: > > diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c > > index 0c39cff..ae09170 100644 > > --- a/hw/timer/xilinx_timer.c > > +++ b/hw/timer/xilinx_timer.c > > @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) > > ptimer_set_freq(xt->ptimer, t->freq_hz); > > } > > > > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > > + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, > > R_MAX * 4 * num_timers(t)); > > sysbus_init_mmio(dev, &t->mmio); > > return 0; > > @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass > > Isn't this a false positive? Yes. I see patches doing QOM'ify don't update things like this.
On Fri, Jun 21, 2013 at 02:21:02PM +1000, Peter Crosthwaite wrote: > Hi Andreas, Hu, > > I thought Id share with you a little script I made (not very polished) > that I used to help with some of my patches creating the QOM cast > macros (mainly the PCI ones). May be useful in speeding up the > QOMification effort. Andreas, im guessing you may have something > similar going if your able to comment? I know Hu mentioned he wanted > to work on QOMification of sysbus - which is a big job so stuff like > this may make life easier. > > example usage: > > $ source ./object_macro_maker hw/timer/xilinx_timer.c XILINX_TIMER > > 1st arg is target file, 2 arg is the name of the type, I.e. FOO in TYPE_FOO Very helpful. Thanks! > > It will automatically find replace usages of the string literal type > inplace and give you a fragment to copy-paste into the source defining > the type string and object cast macro. I extended the script a bit more to search for the struct and insert the fragment, as follows: #!/bin/bash sed -n '/^static const TypeInfo.*$/,/^};.*$/p' $1 | \ grep "\(\.instance_size\|\.name\)"\ > typeinfo.tmp cat typeinfo.tmp STRING=$(grep -o "\".*\"" typeinfo.tmp | sed 's/\"//g') echo echo "String is ${STRING}" echo sed "s/\"${STRING}\"/TYPE_${2}/g" -i ${1} git diff ${1} | cat STATE_STRUCT=$(grep -o "(.*)" typeinfo.tmp | sed "s/(//" | sed "s/)//") echo "State Struct is ${STATE_STRUCT}" echo "------------------ cut here ------------------------" echo "#define TYPE_${2} \"${STRING}\"" echo "" echo "#define ${2}(obj) \\" echo " OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})" found="" grep "^.*struct ${STATE_STRUCT} *{" ${1} if [ $? -eq 0 ]; then found=${1} else files=$(sed -n "s/^#include \"\(.*\)\"/\1/ p" ${1}) for f in $(echo $files); do f=$(echo "include/$f") grep "^.*struct ${STATE_STRUCT} *{" ${f} if [ $? -eq 0 ]; then found=${f} break fi done fi if [ "${found}" == "" ]; then echo "not found file." exit 1 fi # define TYPE_FOO sed -i "/^.*struct ${STATE_STRUCT} *{\$/i#define TYPE_${2} \"${STRING}\"" ${found} sed -i "/^.*struct ${STATE_STRUCT} *{\$/i\\\n" ${found} sed -i "/^.*struct ${STATE_STRUCT} *{\$/i#define ${2}(obj) \\\\" ${found} sed -i "/^.*struct ${STATE_STRUCT} *{\$/i\ \ \ \ OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})" ${found} sed -i "/^.*struct ${STATE_STRUCT} *{\$/i\\\n" ${found}
Hi, Am 21.06.2013 09:44, schrieb Peter Crosthwaite: > On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: >>> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c >>> index 0c39cff..ae09170 100644 >>> --- a/hw/timer/xilinx_timer.c >>> +++ b/hw/timer/xilinx_timer.c >>> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) >>> ptimer_set_freq(xt->ptimer, t->freq_hz); >>> } >>> >>> - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", >>> + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, >>> R_MAX * 4 * num_timers(t)); >>> sysbus_init_mmio(dev, &t->mmio); >>> return 0; >>> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass >> >> Isn't this a false positive? >> > > Not really, > > For consistency I just the TYPE_FOO for the memory region name, so I > don't see why that shouldn't be macrofiied along with the rest. It is > quite deliberately the same - please advise if this is wrong. Same for > the VMSD name. I concur with Paolo: The MemoryRegion name is free text that can be changed as desired and doesn't need to match the type name. The TYPE_* constant serves to keep consistency between usages of the string literal but does not prohibit changing its value. The VMStateDescription should thus not use the TYPE_* constant because it must not ever change for live migration. Renaming a type is a valid thing to do if there is no command line compatibility to keep (think board-level "xlnx.foo" -> "xlnx,foo"). > Slightly off topic, should we perhaps use the object canonical patch > for the device as the memory region name string?: > > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > + memory_region_init_io(&t->mmio, &timer_ops, t, > object_get_canonical_path(OBJECT(dev)), > R_MAX * 4 * num_timers(t)); > > Its unambiguous and solves the problem where you have two-of-a-kind > devices in the one system and you need to differentiate. MemoryRegion names can well be changed, but why hardcode a path there? We could just as well associate the MemoryRegion with the Object and let info mtree obtain the current path at runtime. The number of MMIO regions per device is not limited to 1, so the interesting information is what the region is for, not just whom it is from. If there's only one region and the object is associated with it, we could make a NULL argument default to the object's type or something. Anyway, since the path of most devices will look like /machine/unassigned/device[42] I don't see much added value in using it... Anthony once had patches to QOM'ify MemoryRegions, at which point they would probably be child<>s of the device; either Object::parent (controversial) or a backlink could be used to go from MemoryRegion to containing object then. Regards, Andreas
Hi Peter, Am 21.06.2013 06:21, schrieb Peter Crosthwaite: > I thought Id share with you a little script I made (not very polished) > that I used to help with some of my patches creating the QOM cast > macros (mainly the PCI ones). May be useful in speeding up the > QOMification effort. Andreas, im guessing you may have something > similar going if your able to comment? In fact my conversion patches both for QOM and for CPUState were all hand-tailored, checking with git-grep for missed occurrences. My focus was on the cast macros though, with TYPE_* being useful since reused inside the cast macro. Sometimes I use gEdit's Search-and-replace dialog or similar tools that let me visually verify before changing. Were your PCI conversions already scripted? In that case we should take a closer look for false positives. CC'ing mst. > I know Hu mentioned he wanted > to work on QOMification of sysbus - which is a big job so stuff like > this may make life easier. That's great to hear. I'm wondering how to best go about that - would there be interest in reviving my qom-next tree for staging? Problem I see is that these series are equally touching on maintained and not actively maintained devices and that on the one hand some maintainers are ignorant of these QOM concepts and can't really review (but should still help test for regressions) whereas on the other hand other contributed functional patches may conflict with the refactorings. Also some coordination to avoid duplicate conversions might make sense. > example usage: > > $ source ./object_macro_maker hw/timer/xilinx_timer.c XILINX_TIMER > > 1st arg is target file, 2 arg is the name of the type, I.e. FOO in TYPE_FOO > > It will automatically find replace usages of the string literal type > inplace and give you a fragment to copy-paste into the source defining > the type string and object cast macro. > > It has the limitation that it only works with files that define a > single QOM type. I didnt bother trying to generalise as such files are > the exception and not the rule. Depending on the specific case I believe it may make sense to extract devices from such a large file, especially now with the restructured hw/ directory. ADB is one such case where I started looking into it; other examples would be SBus, MusicPal and e500. > > Example output below: > > diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c > index 0c39cff..ae09170 100644 > --- a/hw/timer/xilinx_timer.c > +++ b/hw/timer/xilinx_timer.c > @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) > ptimer_set_freq(xt->ptimer, t->freq_hz); > } > > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, > R_MAX * 4 * num_timers(t)); > sysbus_init_mmio(dev, &t->mmio); > return 0; > @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass > *klass, void *data) > } > > static const TypeInfo xilinx_timer_info = { > - .name = "xlnx.xps-timer", > + .name = TYPE_XILINX_TIMER, > .parent = TYPE_SYS_BUS_DEVICE, > .instance_size = sizeof(struct timerblock), > .class_init = xilinx_timer_class_init, > State Struct is struct timerblock > ------------------ cut here ------------------------ > #define TYPE_XILINX_TIMER "xlnx.xps-timer" > > #define XILINX_TIMER(obj) \ > OBJECT_CHECK(struct timerblock, (obj), TYPE_XILINX_TIMER) > ----------------------------------------------------------------- That's functionally correct, but when occurrences were low I've tried to fix CamelCase and lack of typedef as well. Could be done in a separate patch, with a different script of course. Cheers, Andreas > > > And the script itself: > > #!/bin/bash > > sed -n '/^static const TypeInfo.*$/,/^};.*$/p' $1 | \ > grep "\(\.instance_size\|\.name\)"\ > > typeinfo.tmp > cat typeinfo.tmp > STRING=$(grep -o "\".*\"" typeinfo.tmp | sed 's/\"//g') > > echo "String is ${STRING}" > sed "s/\"${STRING}\"/TYPE_${2}/g" -i ${1} > git diff ${1} | cat > > STATE_STRUCT=$(grep -o "(.*)" typeinfo.tmp | sed "s/(//" | sed "s/)//") > echo "State Struct is ${STATE_STRUCT}" > echo "------------------ cut here ------------------------" > > echo "#define TYPE_${2} \"${STRING}\"" > echo "" > echo "#define ${2}(obj) \\" > echo " OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})" > > > Regards, > Peter >
Hi Andreas, On Fri, Jun 21, 2013 at 7:33 PM, Andreas Färber <afaerber@suse.de> wrote: > Hi Peter, > > Am 21.06.2013 06:21, schrieb Peter Crosthwaite: >> I thought Id share with you a little script I made (not very polished) >> that I used to help with some of my patches creating the QOM cast >> macros (mainly the PCI ones). May be useful in speeding up the >> QOMification effort. Andreas, im guessing you may have something >> similar going if your able to comment? > > In fact my conversion patches both for QOM and for CPUState were all > hand-tailored, checking with git-grep for missed occurrences. My focus > was on the cast macros though, with TYPE_* being useful since reused > inside the cast macro. Sometimes I use gEdit's Search-and-replace dialog > or similar tools that let me visually verify before changing. > This scripted flow probably can achieve a similar level of checking with git add -p, then the author can use git to opt-in per hunk and discard false positives while still keep reasonable automation. That or we make the script smarter - which is not that hard. > Were your PCI conversions already scripted? In that case we should take > a closer look for false positives. CC'ing mst. > I used this script but eyeballed the changes manually. I did however commit memory region name and VMSD changes in places as I did not think it an issue. Ill do a respin and revert appropriate hunks. >> I know Hu mentioned he wanted >> to work on QOMification of sysbus - which is a big job so stuff like >> this may make life easier. > > That's great to hear. I'm wondering how to best go about that - would > there be interest in reviving my qom-next tree for staging? > Yes please. It makes sense that there is a central queue if there are three of us sending these patches - also to catch conflicts between each others' series. > Problem I see is that these series are equally touching on maintained > and not actively maintained devices and that on the one hand some > maintainers are ignorant of these QOM concepts and can't really review > (but should still help test for regressions) whereas on the other hand > other contributed functional patches may conflict with the refactorings. > > Also some coordination to avoid duplicate conversions might make sense. > >> example usage: >> >> $ source ./object_macro_maker hw/timer/xilinx_timer.c XILINX_TIMER >> >> 1st arg is target file, 2 arg is the name of the type, I.e. FOO in TYPE_FOO >> >> It will automatically find replace usages of the string literal type >> inplace and give you a fragment to copy-paste into the source defining >> the type string and object cast macro. >> >> It has the limitation that it only works with files that define a >> single QOM type. I didnt bother trying to generalise as such files are >> the exception and not the rule. > > Depending on the specific case I believe it may make sense to extract > devices from such a large file, especially now with the restructured hw/ > directory. ADB is one such case where I started looking into it; other > examples would be SBus, MusicPal and e500. > >> >> Example output below: >> >> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c >> index 0c39cff..ae09170 100644 >> --- a/hw/timer/xilinx_timer.c >> +++ b/hw/timer/xilinx_timer.c >> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) >> ptimer_set_freq(xt->ptimer, t->freq_hz); >> } >> >> - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", >> + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, >> R_MAX * 4 * num_timers(t)); >> sysbus_init_mmio(dev, &t->mmio); >> return 0; >> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass >> *klass, void *data) >> } >> >> static const TypeInfo xilinx_timer_info = { >> - .name = "xlnx.xps-timer", >> + .name = TYPE_XILINX_TIMER, >> .parent = TYPE_SYS_BUS_DEVICE, >> .instance_size = sizeof(struct timerblock), >> .class_init = xilinx_timer_class_init, >> State Struct is struct timerblock >> ------------------ cut here ------------------------ >> #define TYPE_XILINX_TIMER "xlnx.xps-timer" >> >> #define XILINX_TIMER(obj) \ >> OBJECT_CHECK(struct timerblock, (obj), TYPE_XILINX_TIMER) >> ----------------------------------------------------------------- > > That's functionally correct, but when occurrences were low I've tried to > fix CamelCase and lack of typedef as well. Could be done in a separate > patch, with a different script of course. > Yes, and that one is usually just a simple find-replace. Regards, Peter > Cheers, > Andreas > >> >> >> And the script itself: >> >> #!/bin/bash >> >> sed -n '/^static const TypeInfo.*$/,/^};.*$/p' $1 | \ >> grep "\(\.instance_size\|\.name\)"\ >> > typeinfo.tmp >> cat typeinfo.tmp >> STRING=$(grep -o "\".*\"" typeinfo.tmp | sed 's/\"//g') >> >> echo "String is ${STRING}" >> sed "s/\"${STRING}\"/TYPE_${2}/g" -i ${1} >> git diff ${1} | cat >> >> STATE_STRUCT=$(grep -o "(.*)" typeinfo.tmp | sed "s/(//" | sed "s/)//") >> echo "State Struct is ${STATE_STRUCT}" >> echo "------------------ cut here ------------------------" >> >> echo "#define TYPE_${2} \"${STRING}\"" >> echo "" >> echo "#define ${2}(obj) \\" >> echo " OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})" >> >> >> Regards, >> Peter >> > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
Il 21/06/2013 09:44, Peter Crosthwaite ha scritto: > Hi Paolo, > > On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 21/06/2013 06:21, Peter Crosthwaite ha scritto: >>> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c >>> index 0c39cff..ae09170 100644 >>> --- a/hw/timer/xilinx_timer.c >>> +++ b/hw/timer/xilinx_timer.c >>> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) >>> ptimer_set_freq(xt->ptimer, t->freq_hz); >>> } >>> >>> - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", >>> + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, >>> R_MAX * 4 * num_timers(t)); >>> sysbus_init_mmio(dev, &t->mmio); >>> return 0; >>> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass >> >> Isn't this a false positive? >> > > Not really, > > For consistency I just the TYPE_FOO for the memory region name, so I > don't see why that shouldn't be macrofiied along with the rest. It is > quite deliberately the same - please advise if this is wrong. Same for > the VMSD name. > > Slightly off topic, should we perhaps use the object canonical patch > for the device as the memory region name string?: > > - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", > + memory_region_init_io(&t->mmio, &timer_ops, t, > object_get_canonical_path(OBJECT(dev)), > R_MAX * 4 * num_timers(t)); > > Its unambiguous and solves the problem where you have two-of-a-kind > devices in the one system and you need to differentiate. I think the memory region name should be just a descriptive text, for example "mmio". I'm going to add an owner Object to MemoryRegion, and the owner+name should be unique (except that some regions that are created by the board, typically RAM, and thus do not have an owner). (Which also means that the less you touch the memory_region_init* lines, the best. My patch has to touch all 800 of them!!!). Paolo
diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c index 0c39cff..ae09170 100644 --- a/hw/timer/xilinx_timer.c +++ b/hw/timer/xilinx_timer.c @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev) ptimer_set_freq(xt->ptimer, t->freq_hz); } - memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer", + memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER, R_MAX * 4 * num_timers(t)); sysbus_init_mmio(dev, &t->mmio); return 0; @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass *klass, void *data) } static const TypeInfo xilinx_timer_info = { - .name = "xlnx.xps-timer", + .name = TYPE_XILINX_TIMER, .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(struct timerblock),
Hi Andreas, Hu, I thought Id share with you a little script I made (not very polished) that I used to help with some of my patches creating the QOM cast macros (mainly the PCI ones). May be useful in speeding up the QOMification effort. Andreas, im guessing you may have something similar going if your able to comment? I know Hu mentioned he wanted to work on QOMification of sysbus - which is a big job so stuff like this may make life easier. example usage: $ source ./object_macro_maker hw/timer/xilinx_timer.c XILINX_TIMER 1st arg is target file, 2 arg is the name of the type, I.e. FOO in TYPE_FOO It will automatically find replace usages of the string literal type inplace and give you a fragment to copy-paste into the source defining the type string and object cast macro. It has the limitation that it only works with files that define a single QOM type. I didnt bother trying to generalise as such files are the exception and not the rule. Example output below: .class_init = xilinx_timer_class_init, State Struct is struct timerblock ------------------ cut here ------------------------ #define TYPE_XILINX_TIMER "xlnx.xps-timer" #define XILINX_TIMER(obj) \ OBJECT_CHECK(struct timerblock, (obj), TYPE_XILINX_TIMER) ----------------------------------------------------------------- And the script itself: #!/bin/bash sed -n '/^static const TypeInfo.*$/,/^};.*$/p' $1 | \ grep "\(\.instance_size\|\.name\)"\ > typeinfo.tmp cat typeinfo.tmp STRING=$(grep -o "\".*\"" typeinfo.tmp | sed 's/\"//g') echo "String is ${STRING}" sed "s/\"${STRING}\"/TYPE_${2}/g" -i ${1} git diff ${1} | cat STATE_STRUCT=$(grep -o "(.*)" typeinfo.tmp | sed "s/(//" | sed "s/)//") echo "State Struct is ${STATE_STRUCT}" echo "------------------ cut here ------------------------" echo "#define TYPE_${2} \"${STRING}\"" echo "" echo "#define ${2}(obj) \\" echo " OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})" Regards, Peter