| Message ID | 20171101140316.31333-1-julien.grall@linaro.org |
|---|---|
| Headers | show |
| Series | xen: Convert __page_to_mfn and _mfn_to_page to use typesafe MFN | expand |
>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: > Most of the users of page_to_mfn and mfn_to_page are either overriding > the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest > of the function use mfn_t. > > So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe > MFN. I have to admit that I still find the overall goal confusing: Afaict the double-underscore-prefixed versions exist only to allow easily overriding the non-prefixed ones. Hence the first and foremost goal ought to be to convert everyone to using the non-prefixed versions. Files wanting to avoid the typed forms could then continue to use / be switched to the prefixed ones. What you're doing here is producing a mess: The prefixed versions should never have been touched in the first place. And iirc this was discussed before, with the suggestion to use overrides (for the non-prefixed versions) to limit overall patch size. Jan
Hi Jan, On 06/11/17 11:37, Jan Beulich wrote: >>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: >> Most of the users of page_to_mfn and mfn_to_page are either overriding >> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest >> of the function use mfn_t. >> >> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe >> MFN. > > I have to admit that I still find the overall goal confusing: Afaict > the double-underscore-prefixed versions exist only to allow > easily overriding the non-prefixed ones. Hence the first and > foremost goal ought to be to convert everyone to using the > non-prefixed versions. Files wanting to avoid the typed forms > could then continue to use / be switched to the prefixed ones. > > What you're doing here is producing a mess: The prefixed > versions should never have been touched in the first place. > And iirc this was discussed before, with the suggestion to use > overrides (for the non-prefixed versions) to limit overall patch > size. At the end of the discussion in the previous version, you were happy with the modification done here (see [1]). Overall, I think this is an improvement compare to what we have today. Because we enforce the use of MFN typesafe by default. The developer would have to override the helpers if he wants to to use the non-typesafe version. With your suggestion here, you would just keep the override around even when they are not necessary. They will have to be dropped at some point, so why not now? Cheers, [1] https://lists.xenproject.org/archives/html/xen-devel/2017-10/msg00986.html > > Jan >
>>> On 06.11.17 at 12:47, <julien.grall@linaro.org> wrote: > Hi Jan, > > On 06/11/17 11:37, Jan Beulich wrote: >>>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: >>> Most of the users of page_to_mfn and mfn_to_page are either overriding >>> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest >>> of the function use mfn_t. >>> >>> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe >>> MFN. >> >> I have to admit that I still find the overall goal confusing: Afaict >> the double-underscore-prefixed versions exist only to allow >> easily overriding the non-prefixed ones. Hence the first and >> foremost goal ought to be to convert everyone to using the >> non-prefixed versions. Files wanting to avoid the typed forms >> could then continue to use / be switched to the prefixed ones. >> >> What you're doing here is producing a mess: The prefixed >> versions should never have been touched in the first place. >> And iirc this was discussed before, with the suggestion to use >> overrides (for the non-prefixed versions) to limit overall patch >> size. > > At the end of the discussion in the previous version, you were happy > with the modification done here (see [1]). From the angle looked at it back then I indeed was, but I'm looking at this from a slightly different angle now with the reply above. > Overall, I think this is an improvement compare to what we have today. > Because we enforce the use of MFN typesafe by default. The developer > would have to override the helpers if he wants to to use the > non-typesafe version. > > With your suggestion here, you would just keep the override around even > when they are not necessary. They will have to be dropped at some point, > so why not now? Why would we keep the overrides in place once no longer needed? All I'm asking for is that the double-underscore prefixed macros be left alone, and the non-prefixed versions be converted to be type-safe by default (with the option to override). That'll allow the prefixed variants to go way altogether once all code was switched to typesafe, no longer requiring any overrides. Jan
On 06/11/17 12:11, Jan Beulich wrote: >>>> On 06.11.17 at 12:47, <julien.grall@linaro.org> wrote: >> Hi Jan, >> >> On 06/11/17 11:37, Jan Beulich wrote: >>>>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: >>>> Most of the users of page_to_mfn and mfn_to_page are either overriding >>>> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest >>>> of the function use mfn_t. >>>> >>>> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe >>>> MFN. >>> >>> I have to admit that I still find the overall goal confusing: Afaict >>> the double-underscore-prefixed versions exist only to allow >>> easily overriding the non-prefixed ones. Hence the first and >>> foremost goal ought to be to convert everyone to using the >>> non-prefixed versions. Files wanting to avoid the typed forms >>> could then continue to use / be switched to the prefixed ones. >>> >>> What you're doing here is producing a mess: The prefixed >>> versions should never have been touched in the first place. >>> And iirc this was discussed before, with the suggestion to use >>> overrides (for the non-prefixed versions) to limit overall patch >>> size. >> >> At the end of the discussion in the previous version, you were happy >> with the modification done here (see [1]). > > From the angle looked at it back then I indeed was, but I'm looking > at this from a slightly different angle now with the reply above. > >> Overall, I think this is an improvement compare to what we have today. >> Because we enforce the use of MFN typesafe by default. The developer >> would have to override the helpers if he wants to to use the >> non-typesafe version. >> >> With your suggestion here, you would just keep the override around even >> when they are not necessary. They will have to be dropped at some point, >> so why not now? > > Why would we keep the overrides in place once no longer needed? > All I'm asking for is that the double-underscore prefixed macros be > left alone, and the non-prefixed versions be converted to be > type-safe by default (with the option to override). That'll allow the > prefixed variants to go way altogether once all code was switched > to typesafe, no longer requiring any overrides. If you left the double-underscore alone, then you can't convert headers using them to typesafe. This is because they can't use the non-prefixed version as they may be override. So what you are suggesting here is will avoid converting those headers until someone step up and finish to convert all the source to MFN typesafe. Personally, I find quite silly to have to delay that... Cheers, > > Jan >
>>> On 06.11.17 at 13:16, <julien.grall@linaro.org> wrote: > > On 06/11/17 12:11, Jan Beulich wrote: >>>>> On 06.11.17 at 12:47, <julien.grall@linaro.org> wrote: >>> Hi Jan, >>> >>> On 06/11/17 11:37, Jan Beulich wrote: >>>>>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: >>>>> Most of the users of page_to_mfn and mfn_to_page are either overriding >>>>> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest >>>>> of the function use mfn_t. >>>>> >>>>> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe >>>>> MFN. >>>> >>>> I have to admit that I still find the overall goal confusing: Afaict >>>> the double-underscore-prefixed versions exist only to allow >>>> easily overriding the non-prefixed ones. Hence the first and >>>> foremost goal ought to be to convert everyone to using the >>>> non-prefixed versions. Files wanting to avoid the typed forms >>>> could then continue to use / be switched to the prefixed ones. >>>> >>>> What you're doing here is producing a mess: The prefixed >>>> versions should never have been touched in the first place. >>>> And iirc this was discussed before, with the suggestion to use >>>> overrides (for the non-prefixed versions) to limit overall patch >>>> size. >>> >>> At the end of the discussion in the previous version, you were happy >>> with the modification done here (see [1]). >> >> From the angle looked at it back then I indeed was, but I'm looking >> at this from a slightly different angle now with the reply above. >> >>> Overall, I think this is an improvement compare to what we have today. >>> Because we enforce the use of MFN typesafe by default. The developer >>> would have to override the helpers if he wants to to use the >>> non-typesafe version. >>> >>> With your suggestion here, you would just keep the override around even >>> when they are not necessary. They will have to be dropped at some point, >>> so why not now? >> >> Why would we keep the overrides in place once no longer needed? >> All I'm asking for is that the double-underscore prefixed macros be >> left alone, and the non-prefixed versions be converted to be >> type-safe by default (with the option to override). That'll allow the >> prefixed variants to go way altogether once all code was switched >> to typesafe, no longer requiring any overrides. > > If you left the double-underscore alone, then you can't convert headers > using them to typesafe. This is because they can't use the non-prefixed > version as they may be override. > > So what you are suggesting here is will avoid converting those headers > until someone step up and finish to convert all the source to MFN > typesafe. Personally, I find quite silly to have to delay that... Hmm, I see your point, but if we went the route you suggest, what would be the steps to reach the ultimate result I've described (the prefixed variants gone)? I seems to me that this would require touching a lot of code a second time that is being touched now, or is going to be touched as further conversion happens. Otoh, considering all the acks you've got already, if I'm the only one thinking that switching the prefixed ones around is the wrong thing, I perhaps shouldn't stand in the way of this patch going in as is. Jan
Hi Jan, On 06/11/17 12:44, Jan Beulich wrote: >>>> On 06.11.17 at 13:16, <julien.grall@linaro.org> wrote: > >> >> On 06/11/17 12:11, Jan Beulich wrote: >>>>>> On 06.11.17 at 12:47, <julien.grall@linaro.org> wrote: >>>> Hi Jan, >>>> >>>> On 06/11/17 11:37, Jan Beulich wrote: >>>>>>>> On 01.11.17 at 15:03, <julien.grall@linaro.org> wrote: >>>>>> Most of the users of page_to_mfn and mfn_to_page are either overriding >>>>>> the macros to make them work with mfn_t or use mfn_x/_mfn becaue the rest >>>>>> of the function use mfn_t. >>>>>> >>>>>> So I think it is time to make __page_to_mfn and __mfn_to_page using typesafe >>>>>> MFN. >>>>> >>>>> I have to admit that I still find the overall goal confusing: Afaict >>>>> the double-underscore-prefixed versions exist only to allow >>>>> easily overriding the non-prefixed ones. Hence the first and >>>>> foremost goal ought to be to convert everyone to using the >>>>> non-prefixed versions. Files wanting to avoid the typed forms >>>>> could then continue to use / be switched to the prefixed ones. >>>>> >>>>> What you're doing here is producing a mess: The prefixed >>>>> versions should never have been touched in the first place. >>>>> And iirc this was discussed before, with the suggestion to use >>>>> overrides (for the non-prefixed versions) to limit overall patch >>>>> size. >>>> >>>> At the end of the discussion in the previous version, you were happy >>>> with the modification done here (see [1]). >>> >>> From the angle looked at it back then I indeed was, but I'm looking >>> at this from a slightly different angle now with the reply above. >>> >>>> Overall, I think this is an improvement compare to what we have today. >>>> Because we enforce the use of MFN typesafe by default. The developer >>>> would have to override the helpers if he wants to to use the >>>> non-typesafe version. >>>> >>>> With your suggestion here, you would just keep the override around even >>>> when they are not necessary. They will have to be dropped at some point, >>>> so why not now? >>> >>> Why would we keep the overrides in place once no longer needed? >>> All I'm asking for is that the double-underscore prefixed macros be >>> left alone, and the non-prefixed versions be converted to be >>> type-safe by default (with the option to override). That'll allow the >>> prefixed variants to go way altogether once all code was switched >>> to typesafe, no longer requiring any overrides. >> >> If you left the double-underscore alone, then you can't convert headers >> using them to typesafe. This is because they can't use the non-prefixed >> version as they may be override. >> >> So what you are suggesting here is will avoid converting those headers >> until someone step up and finish to convert all the source to MFN >> typesafe. Personally, I find quite silly to have to delay that... > > Hmm, I see your point, but if we went the route you suggest, > what would be the steps to reach the ultimate result I've > described (the prefixed variants gone)? I seems to me that > this would require touching a lot of code a second time that is > being touched now, or is going to be touched as further > conversion happens. We would indeed need to modify the headers to use page_to_mfn/mfn_to_page instead of __page_to_mfn/__mfn_to_page. Those headers are: - asm-arm/mm.h - xen/domain_page.h - asm-x86/mm.h - asm-x86/page.h - asm-x86/p2m.h I had a look at the files that needs to convert. It seems there are few files with page_to_mfn/mfn_to_page re-defined but no callers: - arch/x86/mm/hap/nested_hap.c - arch/x86/mm/p2m-pt.c - arch/x86/pv/traps.c - arch/x86/pv/mm.c - arch/x86/pv/iret.c Those can be fixed now. That leave the following files: - arch/x86/mm/p2m-ept.c In that file, the override prevents all the caller to use the construction mfn_to_page(_mfn(...)) as it mostly deals with hardware. - arch/x86/x86_64/mm.c This is could be done without too much trouble. - arch/x86/pv/dom0_build.c It would need a bit of rework to get the code MFN typesafe in a neat way. - drivers/passthrough/amd/iommu_map.c It would need a bit of rework to get the code MFN typesafe in a neat way. - common/trace.c I think it should be ok. Though I am bit surprised to see uint32_t been used for MFN... - common/memory.c It would need a bit of rework to get the code MFN typesafe in a neat way. - common/page_alloc.c This is could be done without too much trouble. - common/grant_table.c It would need a bit of rework to get the code MFN typesafe in a neat way. Cheers,
>>> On 09.11.17 at 16:20, <julien.grall@linaro.org> wrote: > I had a look at the files that needs to convert. It seems there are few > files with page_to_mfn/mfn_to_page re-defined but no callers: > - arch/x86/mm/hap/nested_hap.c > - arch/x86/mm/p2m-pt.c > - arch/x86/pv/traps.c > - arch/x86/pv/mm.c > - arch/x86/pv/iret.c > > Those can be fixed now. That leave the following files: > - arch/x86/mm/p2m-ept.c > In that file, the override prevents all the caller to use the > construction mfn_to_page(_mfn(...)) as it mostly deals with hardware. I'm not clear what you're trying to tell me here. The file has a total for four mfn_to_page() uses - if overrides don't help (and perhaps regardless of if they do), I think it wouldn't be very difficult to simply change the four places. And note that plain staging has no override there right now. Jan
Hi, On 09/11/17 15:36, Jan Beulich wrote: >>>> On 09.11.17 at 16:20, <julien.grall@linaro.org> wrote: >> I had a look at the files that needs to convert. It seems there are few >> files with page_to_mfn/mfn_to_page re-defined but no callers: >> - arch/x86/mm/hap/nested_hap.c >> - arch/x86/mm/p2m-pt.c >> - arch/x86/pv/traps.c >> - arch/x86/pv/mm.c >> - arch/x86/pv/iret.c >> >> Those can be fixed now. That leave the following files: >> - arch/x86/mm/p2m-ept.c >> In that file, the override prevents all the caller to use the >> construction mfn_to_page(_mfn(...)) as it mostly deals with hardware. > > I'm not clear what you're trying to tell me here. The file has a total > for four mfn_to_page() uses - if overrides don't help (and perhaps > regardless of if they do), I think it wouldn't be very difficult to simply > change the four places. And note that plain staging has no override > there right now. Because the plain staging still has page_to_mfn() not using typesafe... You would need to override it even I follow your suggestion. What I meant is you would replace the 4 occurrences by mfn_to_page(_mfn(...)). If you are happy with that, then fine. Cheers,
>>> On 09.11.17 at 16:39, <julien.grall@linaro.org> wrote: > On 09/11/17 15:36, Jan Beulich wrote: >>>>> On 09.11.17 at 16:20, <julien.grall@linaro.org> wrote: >>> I had a look at the files that needs to convert. It seems there are few >>> files with page_to_mfn/mfn_to_page re-defined but no callers: >>> - arch/x86/mm/hap/nested_hap.c >>> - arch/x86/mm/p2m-pt.c >>> - arch/x86/pv/traps.c >>> - arch/x86/pv/mm.c >>> - arch/x86/pv/iret.c >>> >>> Those can be fixed now. That leave the following files: >>> - arch/x86/mm/p2m-ept.c >>> In that file, the override prevents all the caller to use the >>> construction mfn_to_page(_mfn(...)) as it mostly deals with hardware. >> >> I'm not clear what you're trying to tell me here. The file has a total >> for four mfn_to_page() uses - if overrides don't help (and perhaps >> regardless of if they do), I think it wouldn't be very difficult to simply >> change the four places. And note that plain staging has no override >> there right now. > Because the plain staging still has page_to_mfn() not using typesafe... > You would need to override it even I follow your suggestion. > > What I meant is you would replace the 4 occurrences by > mfn_to_page(_mfn(...)). If you are happy with that, then fine. Oh, sure, that's a fine intermediate state, which we have all over the place right now. It's clear that it'll take a while to fully carry through typesafeness to everywhere. Jan
Hi Jan, On 09/11/17 15:47, Jan Beulich wrote: >>>> On 09.11.17 at 16:39, <julien.grall@linaro.org> wrote: >> On 09/11/17 15:36, Jan Beulich wrote: >>>>>> On 09.11.17 at 16:20, <julien.grall@linaro.org> wrote: >>>> I had a look at the files that needs to convert. It seems there are few >>>> files with page_to_mfn/mfn_to_page re-defined but no callers: >>>> - arch/x86/mm/hap/nested_hap.c >>>> - arch/x86/mm/p2m-pt.c >>>> - arch/x86/pv/traps.c >>>> - arch/x86/pv/mm.c >>>> - arch/x86/pv/iret.c >>>> >>>> Those can be fixed now. That leave the following files: >>>> - arch/x86/mm/p2m-ept.c >>>> In that file, the override prevents all the caller to use the >>>> construction mfn_to_page(_mfn(...)) as it mostly deals with hardware. >>> >>> I'm not clear what you're trying to tell me here. The file has a total >>> for four mfn_to_page() uses - if overrides don't help (and perhaps >>> regardless of if they do), I think it wouldn't be very difficult to simply >>> change the four places. And note that plain staging has no override >>> there right now. >> Because the plain staging still has page_to_mfn() not using typesafe... >> You would need to override it even I follow your suggestion. >> >> What I meant is you would replace the 4 occurrences by >> mfn_to_page(_mfn(...)). If you are happy with that, then fine. > > Oh, sure, that's a fine intermediate state, which we have all over > the place right now. It's clear that it'll take a while to fully carry > through typesafeness to everywhere. Would you be fine with other part of Xen too? If so, I can have a go at removing completely __page_to_mfn/__mfn_to_page. Cheers,
>>> On 09.11.17 at 16:48, <julien.grall@linaro.org> wrote: > On 09/11/17 15:47, Jan Beulich wrote: >>>>> On 09.11.17 at 16:39, <julien.grall@linaro.org> wrote: >>> What I meant is you would replace the 4 occurrences by >>> mfn_to_page(_mfn(...)). If you are happy with that, then fine. >> >> Oh, sure, that's a fine intermediate state, which we have all over >> the place right now. It's clear that it'll take a while to fully carry >> through typesafeness to everywhere. > > Would you be fine with other part of Xen too? If so, I can have a go at > removing completely __page_to_mfn/__mfn_to_page. Oh, if you want to go that extra mile, that's certainly fine with me. Jan
On Thu, 9 Nov 2017, Jan Beulich wrote: > >>> On 09.11.17 at 16:48, <julien.grall@linaro.org> wrote: > > On 09/11/17 15:47, Jan Beulich wrote: > >>>>> On 09.11.17 at 16:39, <julien.grall@linaro.org> wrote: > >>> What I meant is you would replace the 4 occurrences by > >>> mfn_to_page(_mfn(...)). If you are happy with that, then fine. > >> > >> Oh, sure, that's a fine intermediate state, which we have all over > >> the place right now. It's clear that it'll take a while to fully carry > >> through typesafeness to everywhere. > > > > Would you be fine with other part of Xen too? If so, I can have a go at > > removing completely __page_to_mfn/__mfn_to_page. > > Oh, if you want to go that extra mile, that's certainly fine with me. FYI I committed the first two patches (which are ARM only). I'll leave to other to commit the other two.
Hi Stefano, On 12/11/2017 11:37 PM, Stefano Stabellini wrote: > On Thu, 9 Nov 2017, Jan Beulich wrote: >>>>> On 09.11.17 at 16:48, <julien.grall@linaro.org> wrote: >>> On 09/11/17 15:47, Jan Beulich wrote: >>>>>>> On 09.11.17 at 16:39, <julien.grall@linaro.org> wrote: >>>>> What I meant is you would replace the 4 occurrences by >>>>> mfn_to_page(_mfn(...)). If you are happy with that, then fine. >>>> >>>> Oh, sure, that's a fine intermediate state, which we have all over >>>> the place right now. It's clear that it'll take a while to fully carry >>>> through typesafeness to everywhere. >>> >>> Would you be fine with other part of Xen too? If so, I can have a go at >>> removing completely __page_to_mfn/__mfn_to_page. >> >> Oh, if you want to go that extra mile, that's certainly fine with me. > > FYI I committed the first two patches (which are ARM only). I'll leave > to other to commit the other two. Thank you! The rest of the series is going to be reworked to fully drop __page_to_mfn/__mfn_to_page. Not sure I will resend it though. Cheers,