| Message ID | 20240909100806.47280-2-frediano.ziglio@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Reuse PRI_gfn macro instead of manual specify the format | expand |
On 09.09.2024 12:08, Frediano Ziglio wrote: > Macros are defined to avoid type mismatch in format strings > but also to unify format amongst code. I'm certainly fine with this part. > In the meantime expands to 9 hexadecimal digits. What makes 9 special? What will the extra padding zeroes buy us? > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > --- > xen/common/grant_table.c | 6 +++--- > xen/include/xen/mm-frame.h | 4 ++-- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index ab36f45ded..775cd7e065 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) > if ( rc ) > { > gprintk(XENLOG_ERR, > - "Could not remove status frame %u (GFN %#lx) from P2M\n", > + "Could not remove status frame %u (GFN %"PRI_gfn") from P2M\n", The lost # means the number won't identify itself as hex anymore. Rather than ... > @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain *d) > if ( nr_active <= WARN_GRANT_MAX ) > printk(XENLOG_G_DEBUG "d%d has active grant %x (" > #ifndef NDEBUG > - "GFN %lx, " > + "GFN %"PRI_gfn", " > #endif > "MFN: %#"PRI_mfn")\n", (note this for below) > --- a/xen/include/xen/mm-frame.h > +++ b/xen/include/xen/mm-frame.h > @@ -5,7 +5,7 @@ > #include <xen/typesafe.h> > > TYPE_SAFE(unsigned long, mfn); > -#define PRI_mfn "05lx" > +#define PRI_mfn "09lx" > #define INVALID_MFN_RAW (~0UL) > #define INVALID_MFN _mfn(INVALID_MFN_RAW) > /* > @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y) > } > > TYPE_SAFE(unsigned long, gfn); > -#define PRI_gfn "05lx" > +#define PRI_gfn "09lx" ... moving to 09 (twice) here, how about we move to #? Requiring, of course, to drop already-questionable hashes like the one pointed out in the middle. Jan
On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote: > On 09.09.2024 12:08, Frediano Ziglio wrote: > > Macros are defined to avoid type mismatch in format strings > > but also to unify format amongst code. > > I'm certainly fine with this part. > > > In the meantime expands to 9 hexadecimal digits. > > What makes 9 special? What will the extra padding zeroes buy us? > > I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4, although possibly you would prefer 13 == (64 - 12) / 4. > > Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> > > --- > > xen/common/grant_table.c | 6 +++--- > > xen/include/xen/mm-frame.h | 4 ++-- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > > index ab36f45ded..775cd7e065 100644 > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, > struct grant_table *gt) > > if ( rc ) > > { > > gprintk(XENLOG_ERR, > > - "Could not remove status frame %u (GFN %#lx) > from P2M\n", > > + "Could not remove status frame %u (GFN > %"PRI_gfn") from P2M\n", > > The lost # means the number won't identify itself as hex anymore. Rather > than ... > > > @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain > *d) > > if ( nr_active <= WARN_GRANT_MAX ) > > printk(XENLOG_G_DEBUG "d%d has active grant %x (" > > #ifndef NDEBUG > > - "GFN %lx, " > > + "GFN %"PRI_gfn", " > > #endif > > "MFN: %#"PRI_mfn")\n", > > (note this for below) > > > --- a/xen/include/xen/mm-frame.h > > +++ b/xen/include/xen/mm-frame.h > > @@ -5,7 +5,7 @@ > > #include <xen/typesafe.h> > > > > TYPE_SAFE(unsigned long, mfn); > > -#define PRI_mfn "05lx" > > +#define PRI_mfn "09lx" > > #define INVALID_MFN_RAW (~0UL) > > #define INVALID_MFN _mfn(INVALID_MFN_RAW) > > /* > > @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y) > > } > > > > TYPE_SAFE(unsigned long, gfn); > > -#define PRI_gfn "05lx" > > +#define PRI_gfn "09lx" > > ... moving to 09 (twice) here, how about we move to #? Requiring, of > course, > to drop already-questionable hashes like the one pointed out in the middle. > > I suppose you are suggesting getting rid of the padding entirely and move to prefix, like "%#lx". > Jan > I can do it Frediano
On 09.09.2024 14:53, Frediano Ziglio wrote: > On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote: >> On 09.09.2024 12:08, Frediano Ziglio wrote: >>> Macros are defined to avoid type mismatch in format strings >>> but also to unify format amongst code. >> >> I'm certainly fine with this part. >> >>> In the meantime expands to 9 hexadecimal digits. >> >> What makes 9 special? What will the extra padding zeroes buy us? >> >> > I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4, > although possibly you would prefer 13 == (64 - 12) / 4. 64 is too much for x86; it would want to be 52 there. The way it is right now this is (imo deliberately) not arch-specific, though. >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, >> struct grant_table *gt) >>> if ( rc ) >>> { >>> gprintk(XENLOG_ERR, >>> - "Could not remove status frame %u (GFN %#lx) >> from P2M\n", >>> + "Could not remove status frame %u (GFN >> %"PRI_gfn") from P2M\n", >> >> The lost # means the number won't identify itself as hex anymore. Rather >> than ... >> >>> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain >> *d) >>> if ( nr_active <= WARN_GRANT_MAX ) >>> printk(XENLOG_G_DEBUG "d%d has active grant %x (" >>> #ifndef NDEBUG >>> - "GFN %lx, " >>> + "GFN %"PRI_gfn", " >>> #endif >>> "MFN: %#"PRI_mfn")\n", >> >> (note this for below) >> >>> --- a/xen/include/xen/mm-frame.h >>> +++ b/xen/include/xen/mm-frame.h >>> @@ -5,7 +5,7 @@ >>> #include <xen/typesafe.h> >>> >>> TYPE_SAFE(unsigned long, mfn); >>> -#define PRI_mfn "05lx" >>> +#define PRI_mfn "09lx" >>> #define INVALID_MFN_RAW (~0UL) >>> #define INVALID_MFN _mfn(INVALID_MFN_RAW) >>> /* >>> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y) >>> } >>> >>> TYPE_SAFE(unsigned long, gfn); >>> -#define PRI_gfn "05lx" >>> +#define PRI_gfn "09lx" >> >> ... moving to 09 (twice) here, how about we move to #? Requiring, of >> course, >> to drop already-questionable hashes like the one pointed out in the middle. >> > I suppose you are suggesting getting rid of the padding entirely and move > to prefix, like "%#lx". Yes, i.e. #define PRI_mfn "#lx" Then again I don't really know why "05lx" was chosen originally. Jan
On Mon, Sep 9, 2024 at 1:58 PM Jan Beulich <jbeulich@suse.com> wrote: > On 09.09.2024 14:53, Frediano Ziglio wrote: > > On Mon, Sep 9, 2024 at 11:45 AM Jan Beulich <jbeulich@suse.com> wrote: > >> On 09.09.2024 12:08, Frediano Ziglio wrote: > >>> Macros are defined to avoid type mismatch in format strings > >>> but also to unify format amongst code. > >> > >> I'm certainly fine with this part. > >> > >>> In the meantime expands to 9 hexadecimal digits. > >> > >> What makes 9 special? What will the extra padding zeroes buy us? > >> > >> > > I think either we want kind of fixed size or dynamic. 9 == (48 - 12) / 4, > > although possibly you would prefer 13 == (64 - 12) / 4. > > 64 is too much for x86; it would want to be 52 there. The way it is right > now this is (imo deliberately) not arch-specific, though. > > Yes, but still given the canonic form of x64 you would need to use 13 digits to have all the same size. > >>> --- a/xen/common/grant_table.c > >>> +++ b/xen/common/grant_table.c > >>> @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, > >> struct grant_table *gt) > >>> if ( rc ) > >>> { > >>> gprintk(XENLOG_ERR, > >>> - "Could not remove status frame %u (GFN %#lx) > >> from P2M\n", > >>> + "Could not remove status frame %u (GFN > >> %"PRI_gfn") from P2M\n", > >> > >> The lost # means the number won't identify itself as hex anymore. Rather > >> than ... > >> > >>> @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain > >> *d) > >>> if ( nr_active <= WARN_GRANT_MAX ) > >>> printk(XENLOG_G_DEBUG "d%d has active grant %x (" > >>> #ifndef NDEBUG > >>> - "GFN %lx, " > >>> + "GFN %"PRI_gfn", " > >>> #endif > >>> "MFN: %#"PRI_mfn")\n", > >> > >> (note this for below) > >> > >>> --- a/xen/include/xen/mm-frame.h > >>> +++ b/xen/include/xen/mm-frame.h > >>> @@ -5,7 +5,7 @@ > >>> #include <xen/typesafe.h> > >>> > >>> TYPE_SAFE(unsigned long, mfn); > >>> -#define PRI_mfn "05lx" > >>> +#define PRI_mfn "09lx" > >>> #define INVALID_MFN_RAW (~0UL) > >>> #define INVALID_MFN _mfn(INVALID_MFN_RAW) > >>> /* > >>> @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y) > >>> } > >>> > >>> TYPE_SAFE(unsigned long, gfn); > >>> -#define PRI_gfn "05lx" > >>> +#define PRI_gfn "09lx" > >> > >> ... moving to 09 (twice) here, how about we move to #? Requiring, of > >> course, > >> to drop already-questionable hashes like the one pointed out in the > middle. > >> > > I suppose you are suggesting getting rid of the padding entirely and move > > to prefix, like "%#lx". > > Yes, i.e. > > #define PRI_mfn "#lx" > > Surely more portable amongst different platforms. > Then again I don't really know why "05lx" was chosen originally. > > I assume x86 without PAE, 32 bit, so 5 == (32 - 12) / 4. > Jan > Sent updated one Frediano
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index ab36f45ded..775cd7e065 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -1848,7 +1848,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) if ( rc ) { gprintk(XENLOG_ERR, - "Could not remove status frame %u (GFN %#lx) from P2M\n", + "Could not remove status frame %u (GFN %"PRI_gfn") from P2M\n", i, gfn_x(gfn)); domain_crash(d); return rc; @@ -1863,7 +1863,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt) if ( paging_mode_translate(d) ) { gprintk(XENLOG_ERR, - "Wrong page state %#lx of status frame %u (GFN %#lx)\n", + "Wrong page state %#lx of status frame %u (GFN %"PRI_gfn")\n", pg->count_info, i, gfn_x(gfn)); domain_crash(d); } @@ -3981,7 +3981,7 @@ void grant_table_warn_active_grants(struct domain *d) if ( nr_active <= WARN_GRANT_MAX ) printk(XENLOG_G_DEBUG "d%d has active grant %x (" #ifndef NDEBUG - "GFN %lx, " + "GFN %"PRI_gfn", " #endif "MFN: %#"PRI_mfn")\n", d->domain_id, ref, diff --git a/xen/include/xen/mm-frame.h b/xen/include/xen/mm-frame.h index d973aec901..6f9fcc2a6a 100644 --- a/xen/include/xen/mm-frame.h +++ b/xen/include/xen/mm-frame.h @@ -5,7 +5,7 @@ #include <xen/typesafe.h> TYPE_SAFE(unsigned long, mfn); -#define PRI_mfn "05lx" +#define PRI_mfn "09lx" #define INVALID_MFN_RAW (~0UL) #define INVALID_MFN _mfn(INVALID_MFN_RAW) /* @@ -41,7 +41,7 @@ static inline bool mfn_eq(mfn_t x, mfn_t y) } TYPE_SAFE(unsigned long, gfn); -#define PRI_gfn "05lx" +#define PRI_gfn "09lx" #define INVALID_GFN_RAW (~0UL) #define INVALID_GFN _gfn(INVALID_GFN_RAW) /*
Macros are defined to avoid type mismatch in format strings but also to unify format amongst code. In the meantime expands to 9 hexadecimal digits. Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> --- xen/common/grant_table.c | 6 +++--- xen/include/xen/mm-frame.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)