| Message ID | b9a300edca034d44375d8b16d352110186657e75.1619524463.git.costin.lupu@cs.pub.ro (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix redefinition errors for toolstack libs | expand |
Hi Costin, On 27/04/2021 13:05, Costin Lupu wrote: > tools/libs/foreignmemory/private.h | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h > index 1ee3626dd2..f3c1ba2867 100644 > --- a/tools/libs/foreignmemory/private.h > +++ b/tools/libs/foreignmemory/private.h > @@ -10,11 +10,13 @@ > #include <xen/xen.h> > #include <xen/sys/privcmd.h> > > -#ifndef PAGE_SHIFT /* Mini-os, Yukk */ > +#ifndef PAGE_SHIFT > #define PAGE_SHIFT 12 > #endif > -#ifndef __MINIOS__ /* Yukk */ > +#ifndef PAGE_SIZE > #define PAGE_SIZE (1UL << PAGE_SHIFT) > +#endif > +#ifndef PAGE_MASK > #define PAGE_MASK (~(PAGE_SIZE-1)) > #endif Looking at the usage, I believe PAGE_* are referring to the page granularity of Xen rather than the page granularity of the control domain itself. So it would be incorrect to use the domain's page granularity here and would break dom0 using 64KB page granularity on Arm. Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are still referring to the control domain granularity, then we should use getpageshift() (Or equivalent) because the userspace shouldn't rely on a specific page granularity. Cheers,
Hi Julien, On 4/28/21 12:03 PM, Julien Grall wrote: > Hi Costin, > > On 27/04/2021 13:05, Costin Lupu wrote: >> tools/libs/foreignmemory/private.h | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libs/foreignmemory/private.h >> b/tools/libs/foreignmemory/private.h >> index 1ee3626dd2..f3c1ba2867 100644 >> --- a/tools/libs/foreignmemory/private.h >> +++ b/tools/libs/foreignmemory/private.h >> @@ -10,11 +10,13 @@ >> #include <xen/xen.h> >> #include <xen/sys/privcmd.h> >> -#ifndef PAGE_SHIFT /* Mini-os, Yukk */ >> +#ifndef PAGE_SHIFT >> #define PAGE_SHIFT 12 >> #endif >> -#ifndef __MINIOS__ /* Yukk */ >> +#ifndef PAGE_SIZE >> #define PAGE_SIZE (1UL << PAGE_SHIFT) >> +#endif >> +#ifndef PAGE_MASK >> #define PAGE_MASK (~(PAGE_SIZE-1)) >> #endif > > Looking at the usage, I believe PAGE_* are referring to the page > granularity of Xen rather than the page granularity of the control > domain itself. > > So it would be incorrect to use the domain's page granularity here and > would break dom0 using 64KB page granularity on Arm. > > Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are > still referring to the control domain granularity, then we should use > getpageshift() (Or equivalent) because the userspace shouldn't rely on a > specific page granularity. Yes, this makes sense. One thing I need to clarify: what does XC stand for? I thought for some time it stands for Xen Control. Thanks, Costin
On 28/04/2021 19:27, Costin Lupu wrote: > Hi Julien, Hi Costin, > On 4/28/21 12:03 PM, Julien Grall wrote: >> Hi Costin, >> >> On 27/04/2021 13:05, Costin Lupu wrote: >>> tools/libs/foreignmemory/private.h | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/libs/foreignmemory/private.h >>> b/tools/libs/foreignmemory/private.h >>> index 1ee3626dd2..f3c1ba2867 100644 >>> --- a/tools/libs/foreignmemory/private.h >>> +++ b/tools/libs/foreignmemory/private.h >>> @@ -10,11 +10,13 @@ >>> #include <xen/xen.h> >>> #include <xen/sys/privcmd.h> >>> -#ifndef PAGE_SHIFT /* Mini-os, Yukk */ >>> +#ifndef PAGE_SHIFT >>> #define PAGE_SHIFT 12 >>> #endif >>> -#ifndef __MINIOS__ /* Yukk */ >>> +#ifndef PAGE_SIZE >>> #define PAGE_SIZE (1UL << PAGE_SHIFT) >>> +#endif >>> +#ifndef PAGE_MASK >>> #define PAGE_MASK (~(PAGE_SIZE-1)) >>> #endif >> >> Looking at the usage, I believe PAGE_* are referring to the page >> granularity of Xen rather than the page granularity of the control >> domain itself. >> >> So it would be incorrect to use the domain's page granularity here and >> would break dom0 using 64KB page granularity on Arm. >> >> Instead, we should replace PAGE_* with XC_PAGE_*. If some of them are >> still referring to the control domain granularity, then we should use >> getpageshift() (Or equivalent) because the userspace shouldn't rely on a >> specific page granularity. > > Yes, this makes sense. One thing I need to clarify: what does XC stand > for? I thought for some time it stands for Xen Control. I think it is Xen Control, which is a bit confusing for that specific define. Cheers,
diff --git a/tools/libs/foreignmemory/private.h b/tools/libs/foreignmemory/private.h index 1ee3626dd2..f3c1ba2867 100644 --- a/tools/libs/foreignmemory/private.h +++ b/tools/libs/foreignmemory/private.h @@ -10,11 +10,13 @@ #include <xen/xen.h> #include <xen/sys/privcmd.h> -#ifndef PAGE_SHIFT /* Mini-os, Yukk */ +#ifndef PAGE_SHIFT #define PAGE_SHIFT 12 #endif -#ifndef __MINIOS__ /* Yukk */ +#ifndef PAGE_SIZE #define PAGE_SIZE (1UL << PAGE_SHIFT) +#endif +#ifndef PAGE_MASK #define PAGE_MASK (~(PAGE_SIZE-1)) #endif
If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h header) then gcc will trigger a redefinition error because of -Werror. This commit also protects PAGE_SHIFT and PAGE_MASK definitions for keeping consistency. Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro> --- tools/libs/foreignmemory/private.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)