Re: [xen-unstable test] 153602: regressions - FAIL

[Costin Lupu]

On 9/3/20 7:17 PM, Ian Jackson wrote:
> Jan Beulich writes ("Re: [xen-unstable test] 153602: regressions - FAIL"):
>> On 03.09.2020 12:24, osstest service owner wrote:
>>> flight 153602 xen-unstable real [real]
>>> http://logs.test-lab.xenproject.org/osstest/logs/153602/
>>>
>>> Regressions :-(
>>>
>>> Tests which did not succeed and are blocking,
>>> including tests which could not be run:
>>>  test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 10 debian-hvm-install 
>>> fail REGR. vs. 152877
>>>  test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsm 10 
>>> debian-hvm-install fail REGR. vs. 152877
>>
>> While at least the hypervisor logs don't provide clear indication
>> (and I don't know where else to look among the files osstest
>> provides) I can't help thinking that stubdom apparently
>> crashing is still fallout from the mini-os changes (and no-one
>> really looks to care). In particular I think that this
> 
> I haven't looked at this in detail but I notice that I am having
> build failures.
> 
> Prior to e013e8514389 "config: use mini-os master for unstable", the
> version of mini-os used for builds was controlled by xen.git's
> Config.mk.
> 
> Since then it has been mini-os master.  NB there is no push gate for
> mini-os.  IIRC we discussed this at the time and it was thought that
> breakage due to mini-os would be unlikely.
> 
> To unblock development in xen.git I suggest reverting the minios part
> of 165f3afbfc3d "Config.mk: Unnail versions (for unstable branch)",
> choosing some known-working version of minios to put in Config.mk.
> 
> Perhaps there should indeed be a minios pushgate.  Then osstest would
> use the tested version.

Sorry for breaking this, guys. I've just sent the fix for mini-os on the
mailing list.

Cheers,
Costin

[PATCH] stubdom/grub: Update init_netfront() call for mini-os

[Costin Lupu]

This patch updates the call of init_netfront() function according to its
recently updated declaration which can also include parameters for gateway
and netmask addresses. While we are here, the patch also removes passing
the ip parameter because (a) it is not used anywhere and (b) it wastes
memory since it would reference a dynamically allocated string.

Signed-off-by: Costin Lupu 
---
 stubdom/grub/mini-os.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/stubdom/grub/mini-os.c b/stubdom/grub/mini-os.c
index 4fc052a255..b33dbf02fb 100644
--- a/stubdom/grub/mini-os.c
+++ b/stubdom/grub/mini-os.c
@@ -291,8 +291,6 @@ struct netfront_dev *net_dev;
 int
 minios_probe (struct nic *nic)
 {
-char *ip;
-
 if (net_dev)
 return 1;
 
@@ -300,7 +298,7 @@ minios_probe (struct nic *nic)
 grub_memset ((char *) arptable, 0,
  MAX_ARP * sizeof (struct arptable_t));
 
-net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, &ip);
+net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, NULL, NULL, 
NULL);
 if (!net_dev)
 return 0;
 
-- 
2.20.1

Re: [PATCH] stubdom/grub: Update init_netfront() call for mini-os

[Costin Lupu]

Hi Samuel,

Sorry for spamming. I resent the patch because I wasn't subscribed to
xen-devel when I sent the first one and I know there might be some
issues with the patches arriving on the list for authors who aren't
subscribed.

Cheers,
Costin

On 8/27/20 10:16 PM, Samuel Thibault wrote:
> Costin Lupu, le jeu. 27 août 2020 22:12:57 +0300, a ecrit:
>> This patch updates the call of init_netfront() function according to its
>> recently updated declaration which can also include parameters for gateway
>> and netmask addresses. While we are here, the patch also removes passing
>> the ip parameter because (a) it is not used anywhere and (b) it wastes
>> memory since it would reference a dynamically allocated string.
>>
>> Signed-off-by: Costin Lupu 
> 
> Reviewed-by: Samuel Thibault 
> 
>> ---
>>  stubdom/grub/mini-os.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/stubdom/grub/mini-os.c b/stubdom/grub/mini-os.c
>> index 4fc052a255..b33dbf02fb 100644
>> --- a/stubdom/grub/mini-os.c
>> +++ b/stubdom/grub/mini-os.c
>> @@ -291,8 +291,6 @@ struct netfront_dev *net_dev;
>>  int
>>  minios_probe (struct nic *nic)
>>  {
>> -char *ip;
>> -
>>  if (net_dev)
>>  return 1;
>>  
>> @@ -300,7 +298,7 @@ minios_probe (struct nic *nic)
>>  grub_memset ((char *) arptable, 0,
>>   MAX_ARP * sizeof (struct arptable_t));
>>  
>> -net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, &ip);
>> +net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, NULL, NULL, 
>> NULL);
>>  if (!net_dev)
>>  return 0;
>>  
>> -- 
>> 2.20.1
>>

[PATCH] stubdom/grub: Update init_netfront() call for mini-os

[Costin Lupu]

This patch updates the call of init_netfront() function according to its
recently updated declaration which can also include parameters for gateway
and netmask addresses. While we are here, the patch also removes passing
the ip parameter because (a) it is not used anywhere and (b) it wastes
memory since it would reference a dynamically allocated string.

Signed-off-by: Costin Lupu 
---
 stubdom/grub/mini-os.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/stubdom/grub/mini-os.c b/stubdom/grub/mini-os.c
index 4fc052a255..b33dbf02fb 100644
--- a/stubdom/grub/mini-os.c
+++ b/stubdom/grub/mini-os.c
@@ -291,8 +291,6 @@ struct netfront_dev *net_dev;
 int
 minios_probe (struct nic *nic)
 {
-char *ip;
-
 if (net_dev)
 return 1;
 
@@ -300,7 +298,7 @@ minios_probe (struct nic *nic)
 grub_memset ((char *) arptable, 0,
  MAX_ARP * sizeof (struct arptable_t));
 
-net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, &ip);
+net_dev = init_netfront(NULL, (void*) -1, nic->node_addr, NULL, NULL, 
NULL);
 if (!net_dev)
 return 0;
 
-- 
2.20.1

Re: [xen-unstable-smoke test] 152898: regressions - FAIL

[Costin Lupu]

On 8/27/20 7:07 PM, Jan Beulich wrote:
> On 27.08.2020 17:49, osstest service owner wrote:
>> flight 152898 xen-unstable-smoke real [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/152898/
>>
>> Regressions :-(
>>
>> Tests which did not succeed and are blocking,
>> including tests which could not be run:
>>  build-amd64   6 xen-buildfail REGR. vs. 
>> 152892
> 
> This looks to be an issue in the mini-os tree, and I'm having
> trouble understanding how it can cause the main tree to first
> discover it. Is there no push gate for that tree?
> 
> In any event, commit 1b8ed31f4ce4 ("mini-os: netfront: Read netmask
> and gateway from Xenstore") looks to have missed callers to
> init_netfront(), both in the mini-os tree itself and in stubdom/grub/
> of the main tree.

Hi Jan,

Sorry for missing that. I've just sent a patch on the mini-os mailing
list. Please let me know if it needs anything else.

Cheers,
Costin

Re: [PATCH v4 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 7/8/21 8:33 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 08/06/2021 13:35, Costin Lupu wrote:
>> 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
>> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order
>> to avoid
>> confusion between control domain page granularity (PAGE_* definitions)
>> and
>> guest domain page granularity.
>>
>> The exception is in osdep_xenforeignmemory_map() where we need the
>> system page
> 
> Did you mean osdep_gnttab_grant_map?
> 

Argh, yes, sorry about that. Can we fix this on upstreaming or should I
send a new version?

>> size to check whether the PFN array should be allocated with mmap() or
>> with
>> dynamic allocation.
>>
>> Signed-off-by: Costin Lupu 
> 
> Other than the question above:
> 
> Reviewed-by: Julien Grall 
> 

Cheers,
Costin

Re: [PATCH v4 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Jan,

First of all sorry for breaking the stubdom build. Please see inline.

On 7/13/21 9:47 AM, Jan Beulich wrote:
> On 08.06.2021 14:35, Costin Lupu wrote:
>> --- a/tools/libs/foreignmemory/private.h
>> +++ b/tools/libs/foreignmemory/private.h
>> @@ -1,6 +1,7 @@
>>  #ifndef XENFOREIGNMEMORY_PRIVATE_H
>>  #define XENFOREIGNMEMORY_PRIVATE_H
>>  
>> +#include 
>>  #include 
>>  
>>  #include 
> 
> At the risk of repeating what may have been discussed on irc already yesterday
> (which I would not have seen), this is the cause for the present smoke test
> failure:
> 
> In file included from 
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:39,
>  from 
> /home/osstest/build.163627.build-amd64/xen/tools/include/xenctrl.h:36,
>  from private.h:4,
>  from minios.c:29:
> /home/osstest/build.163627.build-amd64/xen/xen/include/public/memory.h:407:5: 
> error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(const_uint8) buffer;
>  ^~~
> In file included from 
> /home/osstest/build.163627.build-amd64/xen/tools/include/xenctrl.h:36,
>  from private.h:4,
>  from minios.c:29:
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:101:34:
>  error: field 'arch' has incomplete type
>  struct xen_arch_domainconfig arch;
>   ^~~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:152:34:
>  error: field 'arch_config' has incomplete type
>  struct xen_arch_domainconfig arch_config;
>   ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:182:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(xen_pfn_t) array;
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:263:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(uint8) dirty_bitmap;
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:280:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(vcpu_guest_context_t) ctxt; /* IN/OUT */
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:301:26:
>  error: field 'nodemap' has incomplete type
>  struct xenctl_bitmap nodemap;/* IN */
>   ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:337:26:
>  error: field 'cpumap_hard' has incomplete type
>  struct xenctl_bitmap cpumap_hard;
>   ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:338:26:
>  error: field 'cpumap_soft' has incomplete type
>  struct xenctl_bitmap cpumap_soft;
>   ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:418:13:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(xen_domctl_schedparam_vcpu_t) vcpus;
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:473:5:
>  error: unknown type name 'int64_aligned_t'
>  int64_aligned_t time_offset_seconds; /* applied to domain wallclock time 
> */
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:480:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(uint8) buffer; /* IN/OUT: data, or call
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:533:13:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node 
> */
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:544:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(uint32)  sdev_array;   /* OUT */
>  ^~~
> /home/osstest/build.163627.build-amd64/xen/stubdom/include/xen/domctl.h:685:5:
>  error: expected specifier-qualifier-list before 'XEN_GUEST_HANDLE_64'
>  XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT */
>  ^~~
> /hom

Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5

[Costin Lupu]

Hi guys,

On 7/13/21 4:00 PM, Julien Grall wrote:
> 
> 
> On 13/07/2021 13:39, Andrew Cooper wrote:
>> On 13/07/2021 12:53, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> On 13/07/2021 12:23, Andrew Cooper wrote:
 On 13/07/2021 12:21, Julien Grall wrote:
> Hi Andrew,
>
> On 13/07/2021 10:35, Andrew Cooper wrote:
>> On 13/07/2021 10:27, Juergen Gross wrote:
>>> On 13.07.21 11:20, Julien Grall wrote:
 From: Julien Grall 

 Commit 0dbb4be739c5 add the inclusion of xenctrl.h from private.h
 and
 wreck the build in an interesting way:

 In file included from xen/stubdom/include/xen/domctl.h:39:0,
  from xen/tools/include/xenctrl.h:36,
  from private.h:4,
  from minios.c:29:
 xen/include/public/memory.h:407:5: error: expected
 specifier-qualifier-list before ‘XEN_GUEST_HANDLE_64’
  XEN_GUEST_HANDLE_64(const_uint8) buffer;
  ^~~

 This is happening because xenctrl.h defines __XEN_TOOLS__ and
 therefore
 the public headers will start to expose the non-stable ABI.
 However,
 xen.h has already been included by a mini-OS header before hand. So
 there is a mismatch in the way the headers are included.

 For now solve it in a very simple (and gross) way by including
 xenctrl.h before the mini-os headers.

 Fixes: 0dbb4be739c5 ("tools/libs/foreignmemory: Fix PAGE_SIZE
 redefinition error")
 Signed-off-by: Julien Grall 

 ---

 Cc: Andrew Cooper 

 I couldn't find a better way with would not result to revert the
 patch
 (and break build on some system) or involve a longer rework of the
 headers.
>>>
>>> Just adding a "#define __XEN_TOOLS__" before the #include statements
>>> doesn't work?
>>
>> Not really, no.
>>
>> libxenforeignmem has nothing at all to do with any Xen unstable
>> interfaces.  Including xenctrl.h in the first place was wrong,
>> because
>> it is an unstable library.  By extension, the use of XC_PAGE_SIZE is
>> also wrong.
>
> Well... Previously we were using PAGE_SIZE which is just plain wrong
> on Arm.
>
> At the moment, we don't have a way to query the page granularity of
> the hypervisor. But we know it can't change because of the way the
> current ABI was designed. Hence why using XC_PAGE_SIZE is the best of
> option we had until we go to ABIv2.

 Still doesn't mean that XC_PAGE_SIZE was ok to use.
>>>
>>> Note that I wrote "best of the option". The series has been sitting
>>> for ages with no-one answering... You could have provided your option
>>> back then if you thought it wasn't a good use...
>>
>> On a series I wasn't even CC'd on?
> 
> You had the link on IRC because we discussed it.
> 
>>
>> And noone had even bothered to compile test?
> 
> Well, that was a mistake. At the same time, if it compiled the "issue"
> you describe would have gone unnoticed. ;)
> 
>>>

 Sounds like the constant needs moving into the Xen public headers, and
 the inclusions of xenctrl.h into stable libraries needs reverting.
>>>
>>> This could work. Are you planning to work on it?
>>
>> No.  I don't have enough time to do my own work thanks to all the CI
>> breakage and regressions being committed.
>> This needs fixing, or the original series reverting for 4.16 because the
>> current form (with or without this emergency build fix) isn't acceptable
>> to release with.
> I disagree with this caracterization. Yes, this is including a
> non-stable header but it doesn't link with non-stable library.
> 
> In fact, reverting the series will bring back two issues:
>   1) Xen tools will not build on all the distros
>   2) Using PAGE_{SIZE, SHIFT} break arm tools because the userspace is
> not meant to rely on a given kernel page granularity.
> 
> So this doesn't look like a priority for 4.16. Although, it would be a
> nice clean-up to have so the libraries are more compliant.

First of all, sorry for breaking the build.

As Jan already suggested on a different thread, we can fix this by
isolating the XC_PAGE_* definitions of the toolstack in a header of
their own. I'm open to suggestions regarding the name of the header (my
suggestion would be xenctrl_page.h) and path (I guess it should be in
tools/include, right?). Also, should we change the names of the macros
from XC_PAGE_* to something else in order to reflect that they are
toolstack related instead of xenctrl specific?

@Andrew: Can you please tell me why XC_PAGE_SIZE wasn't ok to use? I'm
asking this in order to fully understand the issue.


Cheers,
Costin

Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5

[Costin Lupu]

On 7/13/21 6:20 PM, Juergen Gross wrote:
> On 13.07.21 17:15, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 13/07/2021 16:09, Juergen Gross wrote:
>>> On 13.07.21 16:38, Julien Grall wrote:
 Hi Juergen,

 On 13/07/2021 15:23, Juergen Gross wrote:
> On 13.07.21 16:19, Julien Grall wrote:
>> Hi Jan,
>>
>> On 13/07/2021 15:14, Jan Beulich wrote:
 And I don't think it should be named XC_PAGE_*, but rather
 XEN_PAGE_*.
>>>
>>> Even that doesn't seem right to me, at least in principle. There
>>> shouldn't
>>> be a build time setting when it may vary at runtime. IOW on Arm I
>>> think a
>>> runtime query to the hypervisor would be needed instead.
>>
>> Yes, we want to be able to use the same userspace/OS without
>> rebuilding to a specific hypervisor page size.
>
> This define is used for accessing data of other domains. See the
> define
> for XEN_PAGE_SIZE in xen/include/public/io/ring.h
>
> So it should be a constant (minimal) page size for all hypervisors and
> guests of an architecture.

 Do you mean the maximum rather than minimal? If you use the minimal
 (4KB), then you would not be able to map the page in the stage-2 if
 the hypervisor is using 64KB.
>>>
>>> But this would mean that the current solution to use XC_PAGE_SIZE is
>>> wrong, as this is 4k.
>>
>> The existing ABI is implicitely based on using the hypervisor page
>> granularity (currently 4KB).
>>
>> There is really no way we can support existing guest on 64KB
>> hypervisor. But if we were going to break them, then we should
>> consider to do one of the following option:
>>     1) use 64KB page granularity for ABI
>>     2) query the hypervisor page granularity at runtime
>>
>> The ideal is 2) because it is more scalable for the future. We also
>> need to consider to extend the PV protocol so the backend and frontend
>> can agree on the page size.
> 
> I absolutely agree, but my suggestion was to help finding a proper way
> to cleanup the current interface mess. And this should be done the way
> I suggested IMO.
> 
> A later interface extension for future guests can still be done on top
> of that.

Alright, let's have a little recap to see if I got it right and to agree
on the next steps. There are 2 proposed solutions, let's say a static
one and a dynamic one.

1) Static solution (proposed by Juergen)
- We define XEN_PAGE_* values in a xen/include/public/arch-*/*.h header.
- Q: Should we define a new header for that? page.h or page_size.h are
ok as new filenames?

Pros:
- We fix the interfaces mess and we can get rid of xenctrl lib
dependency for some of the libs that need only the XEN_PAGE_* definitions.
- It's faster to implement, with fewer changes.

Cons:
- Well, it's static, it doesn't allow the hypervisor to provide
different values for different guests.


2) Dynamic solution (proposed by Jan and Julien)
We get the value(s) by calling a hypcall, probably as a query related to
some guest domain.

Pros:
- It's dynamic and scalable. We would support different values for
different guests.

Cons:
- More difficult to implement. It changes the paradigm in the toolstack
libs, every occurrence of XC_PAGE_* would have to be amended. Moreover,
we might want to make the hypcall once and save the value for later
(probably several toolstack structures should be extended for that)


I searched for the occurrences of XC_PAGE_* in the toolstack libs and
it's a *lot* of them. IMHO I think we should pick the static solution
for now, considering that it would be faster to implement. Please let me
know if this is OK or not. Any comments are appreciated.

Cheers,
Costin

Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5

[Costin Lupu]

On 7/27/21 4:36 PM, Andrew Cooper wrote:
> On 16/07/2021 19:28, Costin Lupu wrote:
>> On 7/13/21 6:20 PM, Juergen Gross wrote:
>>> On 13.07.21 17:15, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 13/07/2021 16:09, Juergen Gross wrote:
>>>>> On 13.07.21 16:38, Julien Grall wrote:
>>>>>> Hi Juergen,
>>>>>>
>>>>>> On 13/07/2021 15:23, Juergen Gross wrote:
>>>>>>> On 13.07.21 16:19, Julien Grall wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> On 13/07/2021 15:14, Jan Beulich wrote:
>>>>>>>>>> And I don't think it should be named XC_PAGE_*, but rather
>>>>>>>>>> XEN_PAGE_*.
>>>>>>>>> Even that doesn't seem right to me, at least in principle. There
>>>>>>>>> shouldn't
>>>>>>>>> be a build time setting when it may vary at runtime. IOW on Arm I
>>>>>>>>> think a
>>>>>>>>> runtime query to the hypervisor would be needed instead.
>>>>>>>> Yes, we want to be able to use the same userspace/OS without
>>>>>>>> rebuilding to a specific hypervisor page size.
>>>>>>> This define is used for accessing data of other domains. See the
>>>>>>> define
>>>>>>> for XEN_PAGE_SIZE in xen/include/public/io/ring.h
>>>>>>>
>>>>>>> So it should be a constant (minimal) page size for all hypervisors and
>>>>>>> guests of an architecture.
>>>>>> Do you mean the maximum rather than minimal? If you use the minimal
>>>>>> (4KB), then you would not be able to map the page in the stage-2 if
>>>>>> the hypervisor is using 64KB.
>>>>> But this would mean that the current solution to use XC_PAGE_SIZE is
>>>>> wrong, as this is 4k.
>>>> The existing ABI is implicitely based on using the hypervisor page
>>>> granularity (currently 4KB).
>>>>
>>>> There is really no way we can support existing guest on 64KB
>>>> hypervisor. But if we were going to break them, then we should
>>>> consider to do one of the following option:
>>>>     1) use 64KB page granularity for ABI
>>>>     2) query the hypervisor page granularity at runtime
>>>>
>>>> The ideal is 2) because it is more scalable for the future. We also
>>>> need to consider to extend the PV protocol so the backend and frontend
>>>> can agree on the page size.
>>> I absolutely agree, but my suggestion was to help finding a proper way
>>> to cleanup the current interface mess. And this should be done the way
>>> I suggested IMO.
>>>
>>> A later interface extension for future guests can still be done on top
>>> of that.
>> Alright, let's have a little recap to see if I got it right and to agree
>> on the next steps. There are 2 proposed solutions, let's say a static
>> one and a dynamic one.
>>
>> 1) Static solution (proposed by Juergen)
>> - We define XEN_PAGE_* values in a xen/include/public/arch-*/*.h header.
>> - Q: Should we define a new header for that? page.h or page_size.h are
>> ok as new filenames?
>>
>> Pros:
>> - We fix the interfaces mess and we can get rid of xenctrl lib
>> dependency for some of the libs that need only the XEN_PAGE_* definitions.
>> - It's faster to implement, with fewer changes.
>>
>> Cons:
>> - Well, it's static, it doesn't allow the hypervisor to provide
>> different values for different guests.
>>
>>
>> 2) Dynamic solution (proposed by Jan and Julien)
>> We get the value(s) by calling a hypcall, probably as a query related to
>> some guest domain.
>>
>> Pros:
>> - It's dynamic and scalable. We would support different values for
>> different guests.
>>
>> Cons:
>> - More difficult to implement. It changes the paradigm in the toolstack
>> libs, every occurrence of XC_PAGE_* would have to be amended. Moreover,
>> we might want to make the hypcall once and save the value for later
>> (probably several toolstack structures should be extended for that)
>>
>>
>> I searched for the occurrences of XC_PAGE_* in the toolstack libs and
>> it's a *lot* of them. IMHO I think we should pick the static solution
>> for now, considering that it would be faster to impl

[PATCH] tools/libs/ctrl/xc_vm_event.c: Remove redundant newlines from error messages

[Costin Lupu]

Messages printed with PERROR() do not need explicit newlines (\n)
because they are added implicitly.

Signed-off-by: Costin Lupu 
---
 tools/libs/ctrl/xc_vm_event.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/libs/ctrl/xc_vm_event.c b/tools/libs/ctrl/xc_vm_event.c
index a97c615b18..2ac27c71b2 100644
--- a/tools/libs/ctrl/xc_vm_event.c
+++ b/tools/libs/ctrl/xc_vm_event.c
@@ -58,7 +58,7 @@ void *xc_vm_event_enable(xc_interface *xch, uint32_t 
domain_id, int param,
 rc1 = xc_domain_pause(xch, domain_id);
 if ( rc1 != 0 )
 {
-PERROR("Unable to pause domain\n");
+PERROR("Unable to pause domain");
 return NULL;
 }
 
@@ -66,7 +66,7 @@ void *xc_vm_event_enable(xc_interface *xch, uint32_t 
domain_id, int param,
 rc1 = xc_hvm_param_get(xch, domain_id, param, &pfn);
 if ( rc1 != 0 )
 {
-PERROR("Failed to get pfn of ring page\n");
+PERROR("Failed to get pfn of ring page");
 goto out;
 }
 
@@ -80,7 +80,7 @@ void *xc_vm_event_enable(xc_interface *xch, uint32_t 
domain_id, int param,
   &ring_pfn);
 if ( rc1 != 0 )
 {
-PERROR("Failed to populate ring pfn\n");
+PERROR("Failed to populate ring pfn");
 goto out;
 }
 }
@@ -90,7 +90,7 @@ void *xc_vm_event_enable(xc_interface *xch, uint32_t 
domain_id, int param,
  &mmap_pfn, 1);
 if ( !ring_page )
 {
-PERROR("Could not map the ring page\n");
+PERROR("Could not map the ring page");
 goto out;
 }
 
@@ -124,7 +124,7 @@ void *xc_vm_event_enable(xc_interface *xch, uint32_t 
domain_id, int param,
 rc1 = xc_vm_event_control(xch, domain_id, op, mode, port);
 if ( rc1 != 0 )
 {
-PERROR("Failed to enable vm_event\n");
+PERROR("Failed to enable vm_event");
 goto out;
 }
 
-- 
2.20.1

[PATCH 0/4] Introduce XEN_PAGE_* definitions for mapping guests memory

[Costin Lupu]

This series tries to fix a side-effect introduced by commits 0dbb4be7 and
d1b32abd which added a dependency to xenctrl for foreignmemory and gnntab
libraries library only because they needed to use the XC_PAGE_* values.

These changes introduce the XEN_PAGE_* definitions that will be used by any
toolstack component that doesn't need a dependency to xenctrl library.  

Costin Lupu (4):
  public: Add page related definitions for accessing guests memory
  libs/ctrl: Use Xen values for XC_PAGE_* definitions
  libs/foreignmemory: Use XEN_PAGE_* definitions
  libs/gnttab: Use XEN_PAGE_* definitions

 tools/include/xenctrl.h|  7 +++---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 
 tools/libs/foreignmemory/linux.c   | 18 +++---
 tools/libs/foreignmemory/minios.c  | 10 +---
 tools/libs/foreignmemory/netbsd.c  | 10 
 tools/libs/foreignmemory/private.h |  2 +-
 tools/libs/foreignmemory/solaris.c |  6 ++---
 tools/libs/gnttab/freebsd.c| 20 
 tools/libs/gnttab/linux.c  | 20 
 tools/libs/gnttab/netbsd.c | 20 
 xen/include/public/arch-arm/page.h | 34 ++
 xen/include/public/arch-x86/page.h | 34 ++
 xen/include/public/page.h  | 38 ++
 14 files changed, 165 insertions(+), 66 deletions(-)
 create mode 100644 xen/include/public/arch-arm/page.h
 create mode 100644 xen/include/public/arch-x86/page.h
 create mode 100644 xen/include/public/page.h

-- 
2.20.1

[PATCH 2/4] libs/ctrl: Use Xen values for XC_PAGE_* definitions

[Costin Lupu]

We use the values provided by the Xen public interface for defining the
XC_PAGE_* macros.

Signed-off-by: Costin Lupu 
---
 tools/include/xenctrl.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 14adaa0c10..90bb969fa0 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -54,10 +54,11 @@
 #include 
 #include 
 #endif
+#include 
 
-#define XC_PAGE_SHIFT   12
-#define XC_PAGE_SIZE(1UL << XC_PAGE_SHIFT)
-#define XC_PAGE_MASK(~(XC_PAGE_SIZE-1))
+#define XC_PAGE_SHIFT   XEN_PAGE_SHIFT
+#define XC_PAGE_SIZEXEN_PAGE_SIZE
+#define XC_PAGE_MASKXEN_PAGE_MASK
 
 #define INVALID_MFN  (~0UL)
 
-- 
2.20.1

[PATCH 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

These changes introduce the page related definitions needed for mapping and
accessing guests memory. These values are intended to be used by any toolstack
component that needs to map guests memory. Until now, the values were defined
by the xenctrl.h header, therefore whenever a component had to use them it also
had to add a dependency for the xenctrl library.

For this patch we set the same values for both x86 and ARM architectures.

Signed-off-by: Costin Lupu 
---
 xen/include/public/arch-arm/page.h | 34 ++
 xen/include/public/arch-x86/page.h | 34 ++
 xen/include/public/page.h  | 38 ++
 3 files changed, 106 insertions(+)
 create mode 100644 xen/include/public/arch-arm/page.h
 create mode 100644 xen/include/public/arch-x86/page.h
 create mode 100644 xen/include/public/page.h

diff --git a/xen/include/public/arch-arm/page.h 
b/xen/include/public/arch-arm/page.h
new file mode 100644
index 00..e970feb49c
--- /dev/null
+++ b/xen/include/public/arch-arm/page.h
@@ -0,0 +1,34 @@
+/**
+ * page.h
+ *
+ * Page definitions for accessing guests memory on ARM
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2021, Costin Lupu
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_ARM_PAGE_H__
+#define __XEN_PUBLIC_ARCH_ARM_PAGE_H__
+
+#define XEN_PAGE_SHIFT   12
+#define XEN_PAGE_SIZE(1UL << XEN_PAGE_SHIFT)
+#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
+
+#endif /* __XEN_PUBLIC_ARCH_ARM_PAGE_H__ */
diff --git a/xen/include/public/arch-x86/page.h 
b/xen/include/public/arch-x86/page.h
new file mode 100644
index 00..b1924ea3cb
--- /dev/null
+++ b/xen/include/public/arch-x86/page.h
@@ -0,0 +1,34 @@
+/**
+ * page.h
+ *
+ * Page definitions for accessing guests memory on x86
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2021, Costin Lupu
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_X86_PAGE_H__
+#define __XEN_PUBLIC_ARCH_X86_PAGE_H__
+
+#define XEN_PAGE_SHIFT   12
+#define XEN_PAGE_SIZE(1UL << XEN_PAGE_SHIFT)
+#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
+
+#endif /* __XEN_PUBLIC_ARCH_X86_PAGE_H__ */
diff --git a/xen/include/public/page.h b/xen/include/public/page.h
new file mode 100644
index 00..d3e95fdb4a
--- /dev/null
+++ b/xen/include/public/page.h
@@ -0,0 +1,38 @@
+/**
+ * page.h
+ *
+ * Page definitions for accessing guests memory
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the 

[PATCH 4/4] libs/gnttab: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in d1b32abd which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/freebsd.c | 20 ++--
 tools/libs/gnttab/linux.c   | 20 ++--
 tools/libs/gnttab/netbsd.c  | 20 ++--
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index e42ac3fbf3..7ecb0e3b38 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -28,9 +28,9 @@
 #include 
 #include 
 
+#include 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -74,7 +74,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- XC_PAGE_SHIFT);
+ XEN_PAGE_SHIFT);
 int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
@@ -105,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XEN_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -114,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -129,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * XC_PAGE_SIZE);
+munmap(addr, count * XEN_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -187,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XEN_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -254,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XEN_PAGE_SIZE, PROT_READ | PROT_WRITE, 
MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -266,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -281,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * XC_PAGE_SIZE);
+munmap(area, count * XEN_PAGE_SIZE);
 area = NULL;
 }
 
@@ -304,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * XC_PAGE_SIZE);
+return munmap(start_address, count * XEN_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 5628fd5719..11f1acb771 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -29,10 +29,10 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -101,7 +101,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 map = alloca(map_size);
 else
 {
-map_size = ROUNDUP(map_size, XC_PAGE_SHIFT);
+map_size = ROUNDUP(map_size, XEN_PAGE_SHIFT);
 map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
 if ( map == MAP_FAILED )
@@ -125,7 +125,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 }
 
  retry:
-addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XEN_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map->index);
 
 if (addr == MAP_FAILED && errno == EAGAIN)
@@ -150,7 +150,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 struct ioctl_gntdev_unmap_notify notify;
 notify.index = map->

[PATCH 3/4] libs/foreignmemory: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in 0dbb4be7 which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 18 +-
 tools/libs/foreignmemory/minios.c  | 10 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  2 +-
 tools/libs/foreignmemory/solaris.c |  6 +++---
 7 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 7edc6f0dbf..ad1ad9fc67 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << XC_PAGE_SHIFT;
+*size = fres.nr_frames << XEN_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index 2cf0fa1c38..9439c4ca6a 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XEN_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << XC_PAGE_SHIFT);
+(void)munmap(addr, num << XEN_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << XC_PAGE_SHIFT);
+return munmap(addr, num << XEN_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 9062117407..9dabf28cae 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i<addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(
@@ -313,7 +313,7 @@ int osdep_xenforeignmemory_map_resource(
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/minios.c 
b/tools/libs/foreignmemory/minios.c
index f2f4dfb2be..2454eb9af3 100644
--- a/tools/libs/foreignmemory/minios.c
+++ b/tools/libs/foreignmemory/minios.c
@@ -17,14 +17,6 @@
  * Copyright 2007-2008 Samuel Thibault .
  */
 
-/*
- * xenctrl.h currently defines __XEN_TOOLS__ which affects what is
- * exposed by Xen headers. As the define needs to be set consistently,
- * we want to include xenctrl.h before the mini-os headers (they include
- * public headers).
- */
-#include 
-
 #include 
 #include 
 #include 
@@ -63,7 +55,7 @@ void *osdep_xenforeignmemory_map(x

[PATCH v2 2/4] libs/ctrl: Use Xen values for XC_PAGE_* definitions

[Costin Lupu]

We use the values provided by the Xen public interface for defining the
XC_PAGE_* macros.

Signed-off-by: Costin Lupu 
---
 tools/include/xenctrl.h | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index b77726eab7..2031308458 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -54,10 +54,11 @@
 #include 
 #include 
 #endif
+#include 
 
-#define XC_PAGE_SHIFT   12
-#define XC_PAGE_SIZE(1UL << XC_PAGE_SHIFT)
-#define XC_PAGE_MASK(~(XC_PAGE_SIZE-1))
+#define XC_PAGE_SHIFT   XEN_PAGE_SHIFT
+#define XC_PAGE_SIZEXEN_PAGE_SIZE
+#define XC_PAGE_MASKXEN_PAGE_MASK
 
 #define INVALID_MFN  (~0UL)
 
-- 
2.20.1

[PATCH v2 0/4] Introduce XEN_PAGE_* definitions for mapping guests memory

[Costin Lupu]

This series tries to fix a side-effect introduced by commits 0dbb4be7 and
d1b32abd which added a dependency to xenctrl for foreignmemory and gnntab
libraries library only because they needed to use the XC_PAGE_* values.

These changes introduce the XEN_PAGE_* definitions that will be used by any
toolstack component that doesn't need a dependency to xenctrl library.

Changes since v1:
- Use same page definitions for both x86_64 and ARM (i.e. a single page.h file)
- Introduce xen_mk_long()

Costin Lupu (4):
  public: Add page related definitions for accessing guests memory
  libs/ctrl: Use Xen values for XC_PAGE_* definitions
  libs/foreignmemory: Use XEN_PAGE_* definitions
  libs/gnttab: Use XEN_PAGE_* definitions

 tools/include/xenctrl.h|  7 +++---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 -
 tools/libs/foreignmemory/linux.c   | 18 +++
 tools/libs/foreignmemory/minios.c  | 10 +
 tools/libs/foreignmemory/netbsd.c  | 10 -
 tools/libs/foreignmemory/private.h |  2 +-
 tools/libs/foreignmemory/solaris.c |  6 ++---
 tools/libs/gnttab/freebsd.c| 20 -
 tools/libs/gnttab/linux.c  | 20 -
 tools/libs/gnttab/netbsd.c | 20 -
 xen/include/public/page.h  | 36 ++
 xen/include/public/xen.h   |  3 +++
 13 files changed, 98 insertions(+), 66 deletions(-)
 create mode 100644 xen/include/public/page.h

-- 
2.20.1

[PATCH v2 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

These changes introduce the page related definitions needed for mapping and
accessing guests memory. These values are intended to be used by any toolstack
component that needs to map guests memory. Until now, the values were defined
by the xenctrl.h header, therefore whenever a component had to use them it also
had to add a dependency for the xenctrl library.

This patch also introduces xen_mk_long() macrodefinition for defining long
constants both for C and assembler code.

Signed-off-by: Costin Lupu 
---
 xen/include/public/page.h | 36 
 xen/include/public/xen.h  |  3 +++
 2 files changed, 39 insertions(+)
 create mode 100644 xen/include/public/page.h

diff --git a/xen/include/public/page.h b/xen/include/public/page.h
new file mode 100644
index 00..6b06259bad
--- /dev/null
+++ b/xen/include/public/page.h
@@ -0,0 +1,36 @@
+/**
+ * page.h
+ *
+ * Page definitions for accessing guests memory
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (c) 2021, Costin Lupu
+ */
+
+#ifndef __XEN_PUBLIC_PAGE_H__
+#define __XEN_PUBLIC_PAGE_H__
+
+#include "xen.h"
+
+#define XEN_PAGE_SHIFT   12
+#define XEN_PAGE_SIZE(xen_mk_long(1) << XEN_PAGE_SHIFT)
+#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
+
+#endif /* __XEN_PUBLIC_PAGE_H__ */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index e373592c33..12531c02b5 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -64,11 +64,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
 /* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U
+#define __xen_mk_long(x)  x ## L
 #define __xen_mk_ulong(x) x ## UL
 #ifndef __xen_mk_ullong
 # define __xen_mk_ullong(x) x ## ULL
 #endif
 #define xen_mk_uint(x)__xen_mk_uint(x)
+#define xen_mk_long(x)__xen_mk_long(x)
 #define xen_mk_ulong(x)   __xen_mk_ulong(x)
 #define xen_mk_ullong(x)  __xen_mk_ullong(x)
 
@@ -76,6 +78,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
 /* In assembly code we cannot use C numeric constant suffixes. */
 #define xen_mk_uint(x)   x
+#define xen_mk_long(x)   x
 #define xen_mk_ulong(x)  x
 #define xen_mk_ullong(x) x
 
-- 
2.20.1

[PATCH v2 4/4] libs/gnttab: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in d1b32abd which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/freebsd.c | 20 ++--
 tools/libs/gnttab/linux.c   | 20 ++--
 tools/libs/gnttab/netbsd.c  | 20 ++--
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index e42ac3fbf3..7ecb0e3b38 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -28,9 +28,9 @@
 #include 
 #include 
 
+#include 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -74,7 +74,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- XC_PAGE_SHIFT);
+ XEN_PAGE_SHIFT);
 int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
@@ -105,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XEN_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -114,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -129,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * XC_PAGE_SIZE);
+munmap(addr, count * XEN_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -187,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XEN_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -254,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XEN_PAGE_SIZE, PROT_READ | PROT_WRITE, 
MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -266,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -281,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * XC_PAGE_SIZE);
+munmap(area, count * XEN_PAGE_SIZE);
 area = NULL;
 }
 
@@ -304,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * XC_PAGE_SIZE);
+return munmap(start_address, count * XEN_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 5628fd5719..11f1acb771 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -29,10 +29,10 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -101,7 +101,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 map = alloca(map_size);
 else
 {
-map_size = ROUNDUP(map_size, XC_PAGE_SHIFT);
+map_size = ROUNDUP(map_size, XEN_PAGE_SHIFT);
 map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
 if ( map == MAP_FAILED )
@@ -125,7 +125,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 }
 
  retry:
-addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XEN_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map->index);
 
 if (addr == MAP_FAILED && errno == EAGAIN)
@@ -150,7 +150,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 struct ioctl_gntdev_unmap_notify notify;
 notify.index = map->

[PATCH v2 3/4] libs/foreignmemory: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in 0dbb4be7 which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 18 +-
 tools/libs/foreignmemory/minios.c  | 10 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  2 +-
 tools/libs/foreignmemory/solaris.c |  6 +++---
 7 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 7edc6f0dbf..ad1ad9fc67 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << XC_PAGE_SHIFT;
+*size = fres.nr_frames << XEN_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index 2cf0fa1c38..9439c4ca6a 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XEN_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << XC_PAGE_SHIFT);
+(void)munmap(addr, num << XEN_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << XC_PAGE_SHIFT);
+return munmap(addr, num << XEN_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 9062117407..9dabf28cae 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i<addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(
@@ -313,7 +313,7 @@ int osdep_xenforeignmemory_map_resource(
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/minios.c 
b/tools/libs/foreignmemory/minios.c
index f2f4dfb2be..2454eb9af3 100644
--- a/tools/libs/foreignmemory/minios.c
+++ b/tools/libs/foreignmemory/minios.c
@@ -17,14 +17,6 @@
  * Copyright 2007-2008 Samuel Thibault .
  */
 
-/*
- * xenctrl.h currently defines __XEN_TOOLS__ which affects what is
- * exposed by Xen headers. As the define needs to be set consistently,
- * we want to include xenctrl.h before the mini-os headers (they include
- * public headers).
- */
-#include 
-
 #include 
 #include 
 #include 
@@ -63,7 +55,7 @@ void *osdep_xenforeignmemory_map(x

Re: [PATCH 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

Hi Jan,

Thanks for your comments. I've just sent a v2.

Cheers,
Costin

On 8/10/21 9:41 AM, Jan Beulich wrote:
> On 09.08.2021 16:47, Costin Lupu wrote:
>> These changes introduce the page related definitions needed for mapping and
>> accessing guests memory. These values are intended to be used by any 
>> toolstack
>> component that needs to map guests memory. Until now, the values were defined
>> by the xenctrl.h header, therefore whenever a component had to use them it 
>> also
>> had to add a dependency for the xenctrl library.
>>
>> For this patch we set the same values for both x86 and ARM architectures.
>>
>> Signed-off-by: Costin Lupu 
>> ---
>>  xen/include/public/arch-arm/page.h | 34 ++
>>  xen/include/public/arch-x86/page.h | 34 ++
>>  xen/include/public/page.h  | 38 ++
>>  3 files changed, 106 insertions(+)
>>  create mode 100644 xen/include/public/arch-arm/page.h
>>  create mode 100644 xen/include/public/arch-x86/page.h
>>  create mode 100644 xen/include/public/page.h
> 
> I'm not convinced of these warranting introduction of new headers, but
> I'm also not meaning to say that I'm strictly opposed. I don't recall
> this aspect having had any consideration, yet.
> 
>> --- /dev/null
>> +++ b/xen/include/public/arch-arm/page.h
>> @@ -0,0 +1,34 @@
>> +/**
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory on ARM
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Copyright (c) 2021, Costin Lupu
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_ARCH_ARM_PAGE_H__
>> +#define __XEN_PUBLIC_ARCH_ARM_PAGE_H__
>> +
>> +#define XEN_PAGE_SHIFT   12
>> +#define XEN_PAGE_SIZE(1UL << XEN_PAGE_SHIFT)
>> +#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
> 
> The latter two, being identical ...
> 
>> --- /dev/null
>> +++ b/xen/include/public/arch-x86/page.h
>> @@ -0,0 +1,34 @@
>> +/**
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory on x86
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 

Re: [PATCH v2 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

Hi Jan,

Please see inline.

On 8/20/21 9:52 AM, Jan Beulich wrote:
> On 19.08.2021 19:50, Costin Lupu wrote:
>> These changes introduce the page related definitions needed for mapping and
>> accessing guests memory. These values are intended to be used by any 
>> toolstack
>> component that needs to map guests memory. Until now, the values were defined
>> by the xenctrl.h header, therefore whenever a component had to use them it 
>> also
>> had to add a dependency for the xenctrl library.
>>
>> This patch also introduces xen_mk_long() macrodefinition for defining long
>> constants both for C and assembler code.
> 
> I'm still missing justification for the addition of a new header, especially
> as I don't see that header to gain much more contents down the road.
> 

For the first version, since it didn't need to include other headers, I
thought it would make sense to isolate the definitions in their own
headers. Now maybe it makes more sense to put the definitions in
arch-x86/xen.h, arch-arm.h and xen.h (the latter two) respectively. What
do you think? I'm open to suggestions here.

>> --- /dev/null
>> +++ b/xen/include/public/page.h
>> @@ -0,0 +1,36 @@
>> +/**
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Copyright (c) 2021, Costin Lupu
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_PAGE_H__
>> +#define __XEN_PUBLIC_PAGE_H__
>> +
>> +#include "xen.h"
>> +
>> +#define XEN_PAGE_SHIFT   12
>> +#define XEN_PAGE_SIZE(xen_mk_long(1) << XEN_PAGE_SHIFT)
>> +#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
> 
> You went too far here, I'm afraid: In reply to v1 I did say "The latter
> two, being identical ..." - XEN_PAGE_SHIFT ought to continue to be a
> per-arch constant, even if right now it is the same for Arm and x86.
> 

Alright, now I got it.

> Thinking of which - with exposing this as a stable ABI (not just the
> abstraction, but the specific values and the fact that they're
> invariable become part of the stable ABI this way), what is the plan
> for supporting 64k(?) page size on Arm in the future? At that point
> you _cannot_ simply remove or replace the #define you add here. As
> the immediate need is by the tool stack, enclosing in 
> 
> #if defined(__XEN__) || defined(__XEN_TOOLS__)
> 
> might be an option, with the downside of having stable libraries
> (foreignmemory and gnttab) depend on an unstable hypervisor interface
> (again). I can't seem to be able to think of anything better ...

Here I can be only guessing because I don't know all the requirements
and I would need more input. One would have to add new values for ARM
that would be enabled by some flag and then both the toolstack and the
hypervisor would have to be rebuilt. With the current approach I don't
think there is room for anything else, but I'm curious to hear about
other ideas.

Cheers,
Costin

[PATCH v3 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

These changes introduce the page related definitions needed for mapping and
accessing guests memory. These values are intended to be used by any toolstack
component that needs to map guests memory. Until now, the values were defined
by the xenctrl.h header, therefore whenever a component had to use them it also
had to add a dependency for the xenctrl library.

This patch also introduces xen_mk_long() macrodefinition for defining long
constants both for C and assembler code.

Signed-off-by: Costin Lupu 
---
 xen/include/public/arch-arm.h | 8 
 xen/include/public/arch-x86/xen.h | 8 
 xen/include/public/xen.h  | 9 +
 3 files changed, 25 insertions(+)

diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 64a2ca30da..caf7825d95 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -471,6 +471,14 @@ typedef uint64_t xen_callback_t;
 typedef struct xen_pmu_arch { uint8_t dummy; } xen_pmu_arch_t;
 #endif
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+/*
+ *  Page definitions needed for accessing guests memory
+ */
+#define XEN_PAGE_SHIFT   12
+
+#endif/* __XEN__ || __XEN_TOOLS__ */
+
 #endif /*  __XEN_PUBLIC_ARCH_ARM_H__ */
 
 /*
diff --git a/xen/include/public/arch-x86/xen.h 
b/xen/include/public/arch-x86/xen.h
index 7acd94c8eb..f9939c742b 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -385,6 +385,14 @@ DEFINE_XEN_GUEST_HANDLE(xen_msr_entry_t);
  */
 #define XEN_HVM_DEBUGCONS_IOPORT 0xe9
 
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+/*
+ *  Page definitions needed for accessing guests memory
+ */
+#define XEN_PAGE_SHIFT   12
+
+#endif /* __XEN__ || __XEN_TOOLS__ */
+
 #endif /* __XEN_PUBLIC_ARCH_X86_XEN_H__ */
 
 /*
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index e373592c33..c6486040b9 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -64,11 +64,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
 /* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U
+#define __xen_mk_long(x)  x ## L
 #define __xen_mk_ulong(x) x ## UL
 #ifndef __xen_mk_ullong
 # define __xen_mk_ullong(x) x ## ULL
 #endif
 #define xen_mk_uint(x)__xen_mk_uint(x)
+#define xen_mk_long(x)__xen_mk_long(x)
 #define xen_mk_ulong(x)   __xen_mk_ulong(x)
 #define xen_mk_ullong(x)  __xen_mk_ullong(x)
 
@@ -76,6 +78,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 
 /* In assembly code we cannot use C numeric constant suffixes. */
 #define xen_mk_uint(x)   x
+#define xen_mk_long(x)   x
 #define xen_mk_ulong(x)  x
 #define xen_mk_ullong(x) x
 
@@ -1034,6 +1037,12 @@ struct xenctl_bitmap {
 typedef struct xenctl_bitmap xenctl_bitmap_t;
 #endif
 
+/*
+ *  Page definitions needed for accessing guests memory
+ */
+#define XEN_PAGE_SIZE(xen_mk_long(1) << XEN_PAGE_SHIFT)
+#define XEN_PAGE_MASK(~(XEN_PAGE_SIZE - 1))
+
 #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
 
 #endif /* __XEN_PUBLIC_XEN_H__ */
-- 
2.20.1

[PATCH v3 2/4] libs/ctrl: Use Xen values for XC_PAGE_* definitions

[Costin Lupu]

We use the values provided by the Xen public interface for defining the
XC_PAGE_* macros.

Signed-off-by: Costin Lupu 
---
 tools/include/xenctrl.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index b77726eab7..52a1768ee7 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -55,9 +55,9 @@
 #include 
 #endif
 
-#define XC_PAGE_SHIFT   12
-#define XC_PAGE_SIZE(1UL << XC_PAGE_SHIFT)
-#define XC_PAGE_MASK(~(XC_PAGE_SIZE-1))
+#define XC_PAGE_SHIFT   XEN_PAGE_SHIFT
+#define XC_PAGE_SIZEXEN_PAGE_SIZE
+#define XC_PAGE_MASKXEN_PAGE_MASK
 
 #define INVALID_MFN  (~0UL)
 
-- 
2.20.1

[PATCH v3 0/4] Introduce XEN_PAGE_* definitions for mapping guests memory

[Costin Lupu]

This series tries to fix a side-effect introduced by commits 0dbb4be7 and
d1b32abd which added a dependency to xenctrl for foreignmemory and gnntab
libraries library only because they needed to use the XC_PAGE_* values.

These changes introduce the XEN_PAGE_* definitions that will be used by any
toolstack component that doesn't need a dependency to xenctrl library.

Changes since v1:
- Use same page definitions for both x86_64 and ARM (i.e. a single page.h file)
- Introduce xen_mk_long()

Changes since v2:
- Get rid of new page.h header and use instead arch-x86/xen.h, arch-arm.h and
  xen.h headers

Costin Lupu (4):
  public: Add page related definitions for accessing guests memory
  libs/ctrl: Use Xen values for XC_PAGE_* definitions
  libs/foreignmemory: Use XEN_PAGE_* definitions
  libs/gnttab: Use XEN_PAGE_* definitions

 tools/include/xenctrl.h|  6 +++---
 tools/libs/foreignmemory/Makefile  |  2 ++
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 18 +-
 tools/libs/foreignmemory/minios.c  | 10 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  1 -
 tools/libs/foreignmemory/solaris.c |  6 +++---
 tools/libs/gnttab/Makefile |  2 ++
 tools/libs/gnttab/freebsd.c| 19 +--
 tools/libs/gnttab/linux.c  | 19 +--
 tools/libs/gnttab/netbsd.c | 19 +--
 xen/include/public/arch-arm.h  |  8 
 xen/include/public/arch-x86/xen.h  |  8 
 xen/include/public/xen.h   |  9 +
 16 files changed, 83 insertions(+), 66 deletions(-)

-- 
2.20.1

[PATCH v3 4/4] libs/gnttab: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in d1b32abd which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/Makefile  |  2 ++
 tools/libs/gnttab/freebsd.c | 19 +--
 tools/libs/gnttab/linux.c   | 19 +--
 tools/libs/gnttab/netbsd.c  | 19 +--
 4 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/tools/libs/gnttab/Makefile b/tools/libs/gnttab/Makefile
index ae390ce60f..a884860378 100644
--- a/tools/libs/gnttab/Makefile
+++ b/tools/libs/gnttab/Makefile
@@ -13,4 +13,6 @@ SRCS-$(CONFIG_FreeBSD) += $(SRCS-GNTTAB) $(SRCS-GNTSHR) 
freebsd.c
 SRCS-$(CONFIG_NetBSD)  += $(SRCS-GNTTAB) $(SRCS-GNTSHR) netbsd.c
 SRCS-$(CONFIG_SunOS)   += gnttab_unimp.c gntshr_unimp.c
 
+CFLAGS   += -D__XEN_TOOLS__
+
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index e42ac3fbf3..548e21361d 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -30,7 +30,6 @@
 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -74,7 +73,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- XC_PAGE_SHIFT);
+ XEN_PAGE_SHIFT);
 int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
@@ -105,7 +104,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XEN_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -114,7 +113,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -129,7 +128,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * XC_PAGE_SIZE);
+munmap(addr, count * XEN_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -187,7 +186,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XEN_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -254,7 +253,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XEN_PAGE_SIZE, PROT_READ | PROT_WRITE, 
MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -266,7 +265,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < XC_PAGE_SIZE * count )
+if ( notify_offset < XEN_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -281,7 +280,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * XC_PAGE_SIZE);
+munmap(area, count * XEN_PAGE_SIZE);
 area = NULL;
 }
 
@@ -304,7 +303,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * XC_PAGE_SIZE);
+return munmap(start_address, count * XEN_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 5628fd5719..064aa3097f 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -32,7 +32,6 @@
 #include 
 #include 
 
-#include 
 #include 
 
 #include "private.h"
@@ -101,7 +100,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 map = alloca(map_size);
 else
 {
-map_size = ROUNDUP(map_size, XC_PAGE_SHIFT);
+map_size = ROUNDUP(map_size, XEN_PAGE_SHIFT);
 map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
 if ( map == MAP_FAILED )
@@ -125,7 +124,7 @@ void *osdep_gnttab_grant_map(

[PATCH v3 3/4] libs/foreignmemory: Use XEN_PAGE_* definitions

[Costin Lupu]

These changes refine the changes in 0dbb4be7 which added a dependency to
xenctrl library. We use the XEN_PAGE_* definitions instead of the XC_PAGE_*
definitions and therefore we get rid of the unnecessary dependency.

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/Makefile  |  2 ++
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 18 +-
 tools/libs/foreignmemory/minios.c  | 10 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  1 -
 tools/libs/foreignmemory/solaris.c |  6 +++---
 8 files changed, 26 insertions(+), 33 deletions(-)

diff --git a/tools/libs/foreignmemory/Makefile 
b/tools/libs/foreignmemory/Makefile
index 0eb9a3a712..e3f417f5ca 100644
--- a/tools/libs/foreignmemory/Makefile
+++ b/tools/libs/foreignmemory/Makefile
@@ -11,4 +11,6 @@ SRCS-$(CONFIG_SunOS)   += compat.c solaris.c
 SRCS-$(CONFIG_NetBSD)  += netbsd.c
 SRCS-$(CONFIG_MiniOS)  += minios.c
 
+CFLAGS   += -D__XEN_TOOLS__
+
 include $(XEN_ROOT)/tools/libs/libs.mk
diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 7edc6f0dbf..ad1ad9fc67 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << XC_PAGE_SHIFT;
+*size = fres.nr_frames << XEN_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index 2cf0fa1c38..9439c4ca6a 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XEN_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << XC_PAGE_SHIFT);
+(void)munmap(addr, num << XEN_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << XC_PAGE_SHIFT);
+return munmap(addr, num << XEN_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 9062117407..9dabf28cae 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i<addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(
@@ -313,7 +313,7 @@ int osdep_xenforeignmemory_map_resource(
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XEN_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/minios.c 
b/tools/libs/foreignmemory/minios.c
index f2f4dfb2be..2454eb9af3 100644
--- a/tools/libs/foreignmemory/minios.c

Re: [PATCH v2 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu]

Hi guys,

On 8/23/21 8:16 PM, Julien Grall wrote:
> Hi Jan,
> 
> On 20/08/2021 10:26, Jan Beulich wrote:
>> On 20.08.2021 11:08, Julien Grall wrote:
>>> On 20/08/2021 08:44, Costin Lupu wrote:
>>>> On 8/20/21 9:52 AM, Jan Beulich wrote:
>>>>>> --- /dev/null
>>>>>> +++ b/xen/include/public/page.h
>>>>>> @@ -0,0 +1,36 @@
>>>>>> +/**
>>>>>>
>>>>>> + * page.h
>>>>>> + *
>>>>>> + * Page definitions for accessing guests memory
>>>>>> + *
>>>>>> + * Permission is hereby granted, free of charge, to any person
>>>>>> obtaining a copy
>>>>>> + * of this software and associated documentation files (the
>>>>>> "Software"), to
>>>>>> + * deal in the Software without restriction, including without
>>>>>> limitation the
>>>>>> + * rights to use, copy, modify, merge, publish, distribute,
>>>>>> sublicense, and/or
>>>>>> + * sell copies of the Software, and to permit persons to whom the
>>>>>> Software is
>>>>>> + * furnished to do so, subject to the following conditions:
>>>>>> + *
>>>>>> + * The above copyright notice and this permission notice shall be
>>>>>> included in
>>>>>> + * all copies or substantial portions of the Software.
>>>>>> + *
>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>>>>>> KIND, EXPRESS OR
>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>>>> MERCHANTABILITY,
>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>>>>>> EVENT SHALL THE
>>>>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>>>>> OR OTHER
>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>>>> OTHERWISE, ARISING
>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>>>>> OTHER
>>>>>> + * DEALINGS IN THE SOFTWARE.
>>>>>> + *
>>>>>> + * Copyright (c) 2021, Costin Lupu
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef __XEN_PUBLIC_PAGE_H__
>>>>>> +#define __XEN_PUBLIC_PAGE_H__
>>>>>> +
>>>>>> +#include "xen.h"
>>>>>> +
>>>>>> +#define XEN_PAGE_SHIFT   12
>>>>>> +#define XEN_PAGE_SIZE    (xen_mk_long(1) << XEN_PAGE_SHIFT)
>>>
>>> This will use UL whereas on Arm a page frame should always be 64-bit
>>> regardless the bitness. Shouldn't this be converted to use xen_ulong_t
>>> instead?
>>
>> As pointed out on v1, XEN_PAGE_SIZE would better not end up as a
>> value of signed type, for ...
> 
> Did you mean "not end up as a value of **unsigned** type"...
> 
>>
>>>>>> +#define XEN_PAGE_MASK    (~(XEN_PAGE_SIZE - 1))
>>
>> ... this to suitably sign-extend to wider types is necessary.
> 
> ... because, if I am not mistaken, the sign-extension wouldn't happen
> with unsigned type. But then on v1 you wrote:
> 
> "Imo the smallest type this should evaluate to is xen_ulong_t"
> 
> Which I interpreted as this value should be 64-bit on Arm32. If this not
> what you meant then I am lost.
> 
>>
>> Also unless you expect someone to use typeof(XEN_PAGE_SIZE) I'm
>> afraid I don't see where the constant being long vs xen_long_t
>> (if such existed) might matter.
>> Otoh perhaps xen_mk_ulong() would
>> better have produced a xen_ulong_t typed values in the first
>> place, but I'm afraid we can't alter the existing macro.
> 
> We can create a new one.
> 
>>> Our stable ABI has not been designed with multiple page granularity in
>>> mind. We could introduce a hypercall to query the page size used by the
>>> ABI. But then, I don't think we have the full picture of how this is
>>> going to pan out (I haven't try to use another page size on Xen yet).
>>>
>>> I think we have three choices here:
>>>     1) Stick with the existing definition in the tools
>>>     2) Move the definition in the public headers and only expose them to
>>> the tools.
>>>     3) Query the page size via a new hypervisor
>>>
>>> As I wrote above, 3) is going to take some time to get it right. So the
>>> question here is whether 2) is temporarily better than 1).
>>
>> Because I understand 3) is some way out, and because I think 2) is
>> better than 1), I wrote "might be an option" for what you call 2).
>> But I could see people (Andrew for example) to take a different
>> position and object to such a temporary measure.
> 
> I think we need to make a decision so Costin doesn't keep sending
> version on something that can't be merged. What does the others thinks?

>From what I understood, in his last reply to 'stubdom: foreignmemory:
Fix build after 0dbb4be739c5' thread, Andrew was OK with solution 2).

Cheers,
Costin

[PATCH] x86/ACPI: Fix build error when tboot is disabled

[Costin Lupu]

When tboot is disabled via menuconfig we get undefined reference error for
g_tboot_shared. This patch fixes that by disabling the causing source code
when tboot is disabled.

Signed-off-by: Costin Lupu 
---
 xen/arch/x86/acpi/power.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 91a8c4d0bd..29d9775aed 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -407,6 +407,7 @@ static int acpi_get_wake_status(void)
 return val;
 }
 
+#ifdef CONFIG_TBOOT
 static void tboot_sleep(u8 sleep_state)
 {
 uint32_t shutdown_type;
@@ -451,18 +452,21 @@ static void tboot_sleep(u8 sleep_state)
 
 tboot_shutdown(shutdown_type);
 }
+#endif
 
 /* System is really put into sleep state by this stub */
 acpi_status acpi_enter_sleep_state(u8 sleep_state)
 {
 acpi_status status;
 
+#ifdef CONFIG_TBOOT
 if ( tboot_in_measured_env() )
 {
 tboot_sleep(sleep_state);
 printk(XENLOG_ERR "TBOOT failed entering s3 state\n");
 return_ACPI_STATUS(AE_ERROR);
 }
+#endif
 
 ACPI_FLUSH_CPU_CACHE();
 
-- 
2.20.1

Re: [PATCH] x86/ACPI: Fix build error when tboot is disabled

[Costin Lupu]

On 4/29/21 3:40 PM, Jan Beulich wrote:
> On 29.04.2021 14:11, Costin Lupu wrote:
>> When tboot is disabled via menuconfig we get undefined reference error for
>> g_tboot_shared. This patch fixes that by disabling the causing source code
>> when tboot is disabled.
> 
> There must be more to this: Our shim config also builds with tboot
> disabled, without running into any build issue. Furthermore ...
> 
>> --- a/xen/arch/x86/acpi/power.c
>> +++ b/xen/arch/x86/acpi/power.c
>> @@ -407,6 +407,7 @@ static int acpi_get_wake_status(void)
>>  return val;
>>  }
>>  
>> +#ifdef CONFIG_TBOOT
>>  static void tboot_sleep(u8 sleep_state)
>>  {
>>  uint32_t shutdown_type;
>> @@ -451,18 +452,21 @@ static void tboot_sleep(u8 sleep_state)
>>  
>>  tboot_shutdown(shutdown_type);
>>  }
>> +#endif
>>  
>>  /* System is really put into sleep state by this stub */
>>  acpi_status acpi_enter_sleep_state(u8 sleep_state)
>>  {
>>  acpi_status status;
>>  
>> +#ifdef CONFIG_TBOOT
>>  if ( tboot_in_measured_env() )
>>  {
>>  tboot_sleep(sleep_state);
>>  printk(XENLOG_ERR "TBOOT failed entering s3 state\n");
>>  return_ACPI_STATUS(AE_ERROR);
>>  }
>> +#endif
> 
> ... tboot_in_measured_env() already has a stub returning 0 when
> !TBOOT (which is what I would have recommended instead of the
> #ifdef-ary).
> 
> If there is a specific case where the compiler fails to DCE the
> offending code, then you need to describe this in sufficient
> detail.

Yes, indeed. My bad, it is for a debug build with -O0, so without DCE.

Cheers,
Costin

[PATCH v2 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Changes since v1:
- Use XC_PAGE_* macros instead of PAGE_* macros

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c  | 15 --
 tools/debugger/kdd/kdd.c  | 21 +++---
 tools/libfsimage/ext2fs/fsys_ext2fs.c |  2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c |  2 ++
 tools/libs/foreignmemory/core.c   |  2 +-
 tools/libs/foreignmemory/freebsd.c| 10 +++
 tools/libs/foreignmemory/linux.c  | 23 +++
 tools/libs/foreignmemory/minios.c |  2 +-
 tools/libs/foreignmemory/netbsd.c | 10 +++
 tools/libs/foreignmemory/private.h|  9 +-
 tools/libs/gnttab/freebsd.c   | 28 +--
 tools/libs/gnttab/linux.c | 28 +--
 tools/libs/gnttab/netbsd.c| 23 +++
 tools/ocaml/libs/xc/xenctrl_stubs.c   | 10 +++
 .../ocaml/libs/xentoollog/xentoollog_stubs.c  |  4 +++
 tools/ocaml/libs/xl/xenlight_stubs.c  |  4 +++
 16 files changed, 93 insertions(+), 100 deletions(-)

-- 
2.20.1

[PATCH v2 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu 
Reviewed-by: Julien Grall 
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c 
b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC  0xEF53   /* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO  2   /* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX1024   /* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT 5   /* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c 
b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 017) == 004)
 #define S_ISLNK(mode) (((mode) & 017) == 012)
 
+#ifndef PATH_MAX
 #define PATH_MAX   1024/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT5/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

[PATCH v2 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu 
---
 tools/debugger/kdd/kdd-xen.c | 15 ++-
 tools/debugger/kdd/kdd.c | 21 ++---
 2 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..d6308929e8 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,9 +48,6 @@
 
 #define MAPSIZE 4093 /* Prime */
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (1U << PAGE_SHIFT)
-
 struct kdd_guest {
 struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
 xc_interface *xc_handle;
@@ -72,7 +69,7 @@ static void flush_maps(kdd_guest *g)
 int i;
 for (i = 0; i < MAPSIZE; i++) {
 if (g->maps[i] != NULL)
-munmap(g->maps[i], PAGE_SIZE);
+munmap(g->maps[i], XC_PAGE_SIZE);
 g->maps[i] = NULL;
 }
 }
@@ -490,13 +487,13 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 uint32_t map_pfn, map_offset;
 uint8_t *map;
 
-map_pfn = (addr >> PAGE_SHIFT);
-map_offset = addr & (PAGE_SIZE - 1);
+map_pfn = (addr >> XC_PAGE_SHIFT);
+map_offset = addr & (XC_PAGE_SIZE - 1);
 
 /* Evict any mapping of the wrong frame from our slot */ 
 if (g->pfns[map_pfn % MAPSIZE] != map_pfn
 && g->maps[map_pfn % MAPSIZE] != NULL) {
-munmap(g->maps[map_pfn % MAPSIZE], PAGE_SIZE);
+munmap(g->maps[map_pfn % MAPSIZE], XC_PAGE_SIZE);
 g->maps[map_pfn % MAPSIZE] = NULL;
 }
 g->pfns[map_pfn % MAPSIZE] = map_pfn;
@@ -507,7 +504,7 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 else {
 map = xc_map_foreign_range(g->xc_handle,
g->domid,
-   PAGE_SIZE,
+   XC_PAGE_SIZE,
PROT_READ|PROT_WRITE,
map_pfn);
 
@@ -533,7 +530,7 @@ uint32_t kdd_access_physical(kdd_guest *g, uint64_t addr,
 {
 uint32_t chunk, rv, done = 0;
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = XC_PAGE_SIZE - (addr & (XC_PAGE_SIZE - 1));
 if (chunk > len) 
 chunk = len;
 rv = kdd_access_physical_page(g, addr, chunk, buf, write);
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..fa23948242 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -50,6 +50,8 @@
 #include 
 #include 
 
+#include 
+
 #include "kdd.h"
 
 /*
@@ -288,9 +290,6 @@ static void kdd_log_pkt(kdd_state *s, const char *name, 
kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
-#define PAGE_SHIFT (12)
-#define PAGE_SIZE (1ULL << PAGE_SHIFT) 
-
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
   uint32_t len, void *buf)
 {
@@ -352,7 +351,7 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 
 /* Walk the appropriate number of levels */
 for (i = levels; i > 0; i--) {
-shift = PAGE_SHIFT + bits * (i-1);
+shift = XC_PAGE_SHIFT + bits * (i-1);
 mask = ((1ULL << bits) - 1) << shift;
 offset = ((va & mask) >> shift) * width;
 KDD_DEBUG(s, "level %i: mask 0x%16.16"PRIx64" pa 0x%16.16"PRIx64
@@ -364,12 +363,12 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 return -1ULL; // Not present
 pa = entry & 0x000ff000ULL;
 if (pse && (i == 2) && (entry & 0x80)) { // Superpage
-mask = ((1ULL << (PAGE_SHIFT + bits)) - 1);
+mask = ((1ULL << (XC_PAGE_SHIFT + bits)) - 1);
 return (pa & ~mask) + (va & mask);
 }
 }
 
-return pa + (va & (PAGE_SIZE - 1));
+return pa + (va & (XC_PAGE_SIZE - 1));
 }
 
 static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, uint64_t addr,
@@ -380,7 +379,7 @@ static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, 
uint64_t addr,
 
 /* Process one page at a time */
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = XC_PAGE_SIZE - (addr & (XC_PAGE_SIZE - 1));
 if (chunk > len) 
 chunk = len;
 pa = v2p(s, cpuid, addr);
@@ -591,7 +590,7 @@ static void get_os_info_64(kdd_state *s)
 uint64_t dbgkd_a

[PATCH v2 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/freebsd.c | 28 +---
 tools/libs/gnttab/linux.c   | 28 +---
 tools/libs/gnttab/netbsd.c  | 23 ++-
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 768af701c6..e42ac3fbf3 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -30,14 +30,11 @@
 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#define PAGE_SIZE(1UL << PAGE_SHIFT)
-#define PAGE_MASK(~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/gntdev"
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
@@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- PAGE_SHIFT);
+ XC_PAGE_SHIFT);
+int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
-if ( refs_size <= PAGE_SIZE )
+if ( refs_size <= os_page_size )
 map.refs = malloc(refs_size);
 else
 {
@@ -107,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -116,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -131,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * PAGE_SIZE);
+munmap(addr, count * XC_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -150,7 +148,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 }
 
  out:
-if ( refs_size > PAGE_SIZE )
+if ( refs_size > os_page_size )
 munmap(map.refs, refs_size);
 else
 free(map.refs);
@@ -189,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -256,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -268,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -283,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * PAGE_SIZE);
+munmap(area, count * XC_PAGE_SIZE);
 area = NULL;
 }
 
@@ -306,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * PAGE_SIZE);
+return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..9ce27bee6e 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -32,14 +32,11 @@
 #include 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#define PAGE_SIZE(1UL << PAGE_SHIFT)
-#define PAGE_MASK(~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/"
 

[PATCH v2 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already defined in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu 
---
 tools/ocaml/libs/xc/xenctrl_stubs.c| 10 --
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 
 tools/ocaml/libs/xl/xenlight_stubs.c   |  4 
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..f9e33e599a 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,12 @@
 
 #include "mmap_stubs.h"
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE   (1UL << PAGE_SHIFT)
-#define PAGE_MASK   (~(PAGE_SIZE-1))
-
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
((Field(array, index) == Val_none) ? NULL : 
String_val(Field(Field(array, index), 0)))
@@ -818,7 +816,7 @@ CAMLprim value 
stub_xc_domain_memory_increase_reservation(value xch,
CAMLparam3(xch, domid, mem_kb);
int retval;
 
-   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(PAGE_SHIFT - 10);
+   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(XC_PAGE_SHIFT - 10);
 
uint32_t c_domid = _D(domid);
caml_enter_blocking_section();
@@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
 {
CAMLparam1(pages);
 
-   CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
+   CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
 }
 
 
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

[PATCH v2 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 23 ---
 tools/libs/foreignmemory/minios.c  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  9 +
 6 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 28ec311af1..7edc6f0dbf 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << PAGE_SHIFT;
+*size = fres.nr_frames << XC_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index d94ea07862..2cf0fa1c38 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index c1f35e2db7..a5f6d62567 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i< PAGE_SIZE )
+if ( pfn_arr_size > os_page_size )
 munmap(pfn, pfn_arr_size);
 
 if ( rc == -ENOENT && i == num )
@@ -270,7 +271,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -281,7 +282,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -293,7 +294,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(
 xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+return fres ? 

Re: [PATCH v2 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

@Christian: This version is slightly changed, it uses XC_PAGE_* macros
instead of PAGE_* macros and that's why I didn't add your ack.

Cheers,
Costin

On 4/30/21 2:28 PM, Costin Lupu wrote:
> 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
> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
> confusion between control domain page granularity (PAGE_* definitions) and
> guest domain page granularity (which is what we are dealing with here).
> 
> Same issue applies for redefinitions of Val_none and Some_val macros which
> can be already defined in the OCaml system headers (e.g.
> /usr/lib/ocaml/caml/mlvalues.h).
> 
> Signed-off-by: Costin Lupu 
> ---
>  tools/ocaml/libs/xc/xenctrl_stubs.c| 10 --
>  tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 
>  tools/ocaml/libs/xl/xenlight_stubs.c   |  4 
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index d05d7bb30e..f9e33e599a 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -36,14 +36,12 @@
>  
>  #include "mmap_stubs.h"
>  
> -#define PAGE_SHIFT   12
> -#define PAGE_SIZE   (1UL << PAGE_SHIFT)
> -#define PAGE_MASK   (~(PAGE_SIZE-1))
> -
>  #define _H(__h) ((xc_interface *)(__h))
>  #define _D(__d) ((uint32_t)Int_val(__d))
>  
> +#ifndef Val_none
>  #define Val_none (Val_int(0))
> +#endif
>  
>  #define string_of_option_array(array, index) \
>   ((Field(array, index) == Val_none) ? NULL : 
> String_val(Field(Field(array, index), 0)))
> @@ -818,7 +816,7 @@ CAMLprim value 
> stub_xc_domain_memory_increase_reservation(value xch,
>   CAMLparam3(xch, domid, mem_kb);
>   int retval;
>  
> - unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
> (PAGE_SHIFT - 10);
> + unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
> (XC_PAGE_SHIFT - 10);
>  
>   uint32_t c_domid = _D(domid);
>   caml_enter_blocking_section();
> @@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
>  {
>   CAMLparam1(pages);
>  
> - CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
> + CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
>  }
>  
>  
> diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
> b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> index bf64b211c2..e4306a0c2f 100644
> --- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> +++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> @@ -53,8 +53,12 @@ static char * dup_String_val(value s)
>  #include "_xtl_levels.inc"
>  
>  /* Option type support as per 
> http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
> +#ifndef Val_none
>  #define Val_none Val_int(0)
> +#endif
> +#ifndef Some_val
>  #define Some_val(v) Field(v,0)
> +#endif
>  
>  static value Val_some(value v)
>  {
> diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
> b/tools/ocaml/libs/xl/xenlight_stubs.c
> index 352a00134d..45b8af61c7 100644
> --- a/tools/ocaml/libs/xl/xenlight_stubs.c
> +++ b/tools/ocaml/libs/xl/xenlight_stubs.c
> @@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
>  }
>  
>  /* Option type support as per 
> http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
> +#ifndef Val_none
>  #define Val_none Val_int(0)
> +#endif
> +#ifndef Some_val
>  #define Some_val(v) Field(v,0)
> +#endif
>  
>  static value Val_some(value v)
>  {
> 

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Tim,

On 4/29/21 10:58 PM, Tim Deegan wrote:
> Hi,
> 
> At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
>> 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 definitions for
>> keeping consistency.
> 
> Thanks for looking into this!  I think properly speaking we should fix
> this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
> those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
> kdd-xen.c.  If for some reason we ever ended up with a system-defined
> PAGE_SIZE that wasn't 4096u then we would not want to use it here
> because it would break our guest operations.

As discussed for another patch of the series, an agreed solution that
would apply for other libs as well would be to use XC_PAGE_* macros
instead of PAGE_* macros. I've just sent a v2 doing that. Please let me
know if you think it would be better to introduce the KDD_PAGE_*
definitions instead.

Cheers,
Costin

Re: [PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

On 4/30/21 9:45 PM, Tim Deegan wrote:
> At 14:36 +0300 on 30 Apr (1619793419), Costin Lupu wrote:
>> Hi Tim,
>>
>> On 4/29/21 10:58 PM, Tim Deegan wrote:
>>> Hi,
>>>
>>> At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote:
>>>> 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 definitions for
>>>> keeping consistency.
>>>
>>> Thanks for looking into this!  I think properly speaking we should fix
>>> this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using
>>> those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and
>>> kdd-xen.c.  If for some reason we ever ended up with a system-defined
>>> PAGE_SIZE that wasn't 4096u then we would not want to use it here
>>> because it would break our guest operations.
>>
>> As discussed for another patch of the series, an agreed solution that
>> would apply for other libs as well would be to use XC_PAGE_* macros
>> instead of PAGE_* macros. I've just sent a v2 doing that. Please let me
>> know if you think it would be better to introduce the KDD_PAGE_*
>> definitions instead.
> 
> Sorry to be annoying, but yes, please do introduce the KDD_ versions.
> All the xen-specific code in KDD lives in kdd-xen.c; kdd.c shouldn't
> include any xen headers.

No worries, will do. I imagined that might be the case for kdd.c, but I
wasn't sure.

Cheers,
Costin

[PATCH v3 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Changes since v1:
- Use XC_PAGE_* macros instead of PAGE_* macros

Changes since v2:
- Define KDD_PAGE_* macros for changes in debugger/kdd/

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c  | 15 --
 tools/debugger/kdd/kdd.c  | 19 ++---
 tools/debugger/kdd/kdd.h  |  7 +
 tools/libfsimage/ext2fs/fsys_ext2fs.c |  2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c |  2 ++
 tools/libs/foreignmemory/core.c   |  2 +-
 tools/libs/foreignmemory/freebsd.c| 10 +++
 tools/libs/foreignmemory/linux.c  | 23 +++
 tools/libs/foreignmemory/minios.c |  2 +-
 tools/libs/foreignmemory/netbsd.c | 10 +++
 tools/libs/foreignmemory/private.h|  9 +-
 tools/libs/gnttab/freebsd.c   | 28 +--
 tools/libs/gnttab/linux.c | 28 +--
 tools/libs/gnttab/netbsd.c| 23 +++
 tools/ocaml/libs/xc/xenctrl_stubs.c   | 10 +++
 .../ocaml/libs/xentoollog/xentoollog_stubs.c  |  4 +++
 tools/ocaml/libs/xl/xenlight_stubs.c  |  4 +++
 17 files changed, 98 insertions(+), 100 deletions(-)

-- 
2.20.1

[PATCH v3 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 23 ---
 tools/libs/foreignmemory/minios.c  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  9 +
 6 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 28ec311af1..7edc6f0dbf 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << PAGE_SHIFT;
+*size = fres.nr_frames << XC_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index d94ea07862..2cf0fa1c38 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index c1f35e2db7..a5f6d62567 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i< PAGE_SIZE )
+if ( pfn_arr_size > os_page_size )
 munmap(pfn, pfn_arr_size);
 
 if ( rc == -ENOENT && i == num )
@@ -270,7 +271,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -281,7 +282,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -293,7 +294,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(
 xenforeignmemory_handle *fmem, xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+return fres ? 

[PATCH v3 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with KDD_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

We chose to define the KDD_PAGE_* macros instead of using XC_PAGE_* macros
because (1) the code in kdd.c should not include any Xen headers and (2) to add
consistency for code in both kdd.c and kdd-xen.c.

Signed-off-by: Costin Lupu 
---
 tools/debugger/kdd/kdd-xen.c | 15 ++-
 tools/debugger/kdd/kdd.c | 19 ---
 tools/debugger/kdd/kdd.h |  7 +++
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..e78c9311c4 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,9 +48,6 @@
 
 #define MAPSIZE 4093 /* Prime */
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (1U << PAGE_SHIFT)
-
 struct kdd_guest {
 struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
 xc_interface *xc_handle;
@@ -72,7 +69,7 @@ static void flush_maps(kdd_guest *g)
 int i;
 for (i = 0; i < MAPSIZE; i++) {
 if (g->maps[i] != NULL)
-munmap(g->maps[i], PAGE_SIZE);
+munmap(g->maps[i], KDD_PAGE_SIZE);
 g->maps[i] = NULL;
 }
 }
@@ -490,13 +487,13 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 uint32_t map_pfn, map_offset;
 uint8_t *map;
 
-map_pfn = (addr >> PAGE_SHIFT);
-map_offset = addr & (PAGE_SIZE - 1);
+map_pfn = (addr >> KDD_PAGE_SHIFT);
+map_offset = addr & (KDD_PAGE_SIZE - 1);
 
 /* Evict any mapping of the wrong frame from our slot */ 
 if (g->pfns[map_pfn % MAPSIZE] != map_pfn
 && g->maps[map_pfn % MAPSIZE] != NULL) {
-munmap(g->maps[map_pfn % MAPSIZE], PAGE_SIZE);
+munmap(g->maps[map_pfn % MAPSIZE], KDD_PAGE_SIZE);
 g->maps[map_pfn % MAPSIZE] = NULL;
 }
 g->pfns[map_pfn % MAPSIZE] = map_pfn;
@@ -507,7 +504,7 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 else {
 map = xc_map_foreign_range(g->xc_handle,
g->domid,
-   PAGE_SIZE,
+   KDD_PAGE_SIZE,
PROT_READ|PROT_WRITE,
map_pfn);
 
@@ -533,7 +530,7 @@ uint32_t kdd_access_physical(kdd_guest *g, uint64_t addr,
 {
 uint32_t chunk, rv, done = 0;
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = KDD_PAGE_SIZE - (addr & (KDD_PAGE_SIZE - 1));
 if (chunk > len) 
 chunk = len;
 rv = kdd_access_physical_page(g, addr, chunk, buf, write);
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..320c623eda 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -288,9 +288,6 @@ static void kdd_log_pkt(kdd_state *s, const char *name, 
kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
-#define PAGE_SHIFT (12)
-#define PAGE_SIZE (1ULL << PAGE_SHIFT) 
-
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
   uint32_t len, void *buf)
 {
@@ -352,7 +349,7 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 
 /* Walk the appropriate number of levels */
 for (i = levels; i > 0; i--) {
-shift = PAGE_SHIFT + bits * (i-1);
+shift = KDD_PAGE_SHIFT + bits * (i-1);
 mask = ((1ULL << bits) - 1) << shift;
 offset = ((va & mask) >> shift) * width;
 KDD_DEBUG(s, "level %i: mask 0x%16.16"PRIx64" pa 0x%16.16"PRIx64
@@ -364,12 +361,12 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 return -1ULL; // Not present
 pa = entry & 0x000ff000ULL;
 if (pse && (i == 2) && (entry & 0x80)) { // Superpage
-mask = ((1ULL << (PAGE_SHIFT + bits)) - 1);
+mask = ((1ULL << (KDD_PAGE_SHIFT + bits)) - 1);
 return (pa & ~mask) + (va & mask);
 }
 }
 
-return pa + (va & (PAGE_SIZE - 1));
+return pa + (va & (KDD_PAGE_SIZE - 1));
 }
 
 static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, uint64_t addr,
@@ -380,7 +377,7 @@ static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, 
uint64_t addr,
 
 /* Process one page at a time */
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = KDD_PAGE_SIZE - (addr & (KDD_PAGE_SIZE - 1))

[PATCH v3 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu 
Reviewed-by: Julien Grall 
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c 
b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC  0xEF53   /* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO  2   /* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX1024   /* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT 5   /* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c 
b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 017) == 004)
 #define S_ISLNK(mode) (((mode) & 017) == 012)
 
+#ifndef PATH_MAX
 #define PATH_MAX   1024/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT5/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

[PATCH v3 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/freebsd.c | 28 +---
 tools/libs/gnttab/linux.c   | 28 +---
 tools/libs/gnttab/netbsd.c  | 23 ++-
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 768af701c6..e42ac3fbf3 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -30,14 +30,11 @@
 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#define PAGE_SIZE(1UL << PAGE_SHIFT)
-#define PAGE_MASK(~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/gntdev"
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
@@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- PAGE_SHIFT);
+ XC_PAGE_SHIFT);
+int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
-if ( refs_size <= PAGE_SIZE )
+if ( refs_size <= os_page_size )
 map.refs = malloc(refs_size);
 else
 {
@@ -107,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -116,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -131,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * PAGE_SIZE);
+munmap(addr, count * XC_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -150,7 +148,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 }
 
  out:
-if ( refs_size > PAGE_SIZE )
+if ( refs_size > os_page_size )
 munmap(map.refs, refs_size);
 else
 free(map.refs);
@@ -189,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -256,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -268,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -283,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * PAGE_SIZE);
+munmap(area, count * XC_PAGE_SIZE);
 area = NULL;
 }
 
@@ -306,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * PAGE_SIZE);
+return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..9ce27bee6e 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -32,14 +32,11 @@
 #include 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#define PAGE_SIZE(1UL << PAGE_SHIFT)
-#define PAGE_MASK(~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/"
 

[PATCH v3 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already define in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu 
---
 tools/ocaml/libs/xc/xenctrl_stubs.c| 10 --
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 
 tools/ocaml/libs/xl/xenlight_stubs.c   |  4 
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..f9e33e599a 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,12 @@
 
 #include "mmap_stubs.h"
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE   (1UL << PAGE_SHIFT)
-#define PAGE_MASK   (~(PAGE_SIZE-1))
-
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
((Field(array, index) == Val_none) ? NULL : 
String_val(Field(Field(array, index), 0)))
@@ -818,7 +816,7 @@ CAMLprim value 
stub_xc_domain_memory_increase_reservation(value xch,
CAMLparam3(xch, domid, mem_kb);
int retval;
 
-   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(PAGE_SHIFT - 10);
+   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(XC_PAGE_SHIFT - 10);
 
uint32_t c_domid = _D(domid);
caml_enter_blocking_section();
@@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
 {
CAMLparam1(pages);
 
-   CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
+   CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
 }
 
 
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

[PATCH v4 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Changes since v1:
- Use XC_PAGE_* macros instead of PAGE_* macros

Changes since v2:
- Define KDD_PAGE_* macros for changes in debugger/kdd/

Changes since v3:
- Use sysconf(_SC_PAGESIZE) instead of getpagesize()

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c  | 15 --
 tools/debugger/kdd/kdd.c  | 19 ++---
 tools/debugger/kdd/kdd.h  |  7 +
 tools/libfsimage/ext2fs/fsys_ext2fs.c |  2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c |  2 ++
 tools/libs/foreignmemory/core.c   |  2 +-
 tools/libs/foreignmemory/freebsd.c| 10 +++
 tools/libs/foreignmemory/linux.c  | 23 +++
 tools/libs/foreignmemory/minios.c |  2 +-
 tools/libs/foreignmemory/netbsd.c | 10 +++
 tools/libs/foreignmemory/private.h|  9 +-
 tools/libs/gnttab/freebsd.c   | 28 +--
 tools/libs/gnttab/linux.c | 28 +--
 tools/libs/gnttab/netbsd.c| 23 +++
 tools/ocaml/libs/xc/xenctrl_stubs.c   | 10 +++
 .../ocaml/libs/xentoollog/xentoollog_stubs.c  |  4 +++
 tools/ocaml/libs/xl/xenlight_stubs.c  |  4 +++
 17 files changed, 98 insertions(+), 100 deletions(-)

-- 
2.20.1

[PATCH v4 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with KDD_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

We chose to define the KDD_PAGE_* macros instead of using XC_PAGE_* macros
because (1) the code in kdd.c should not include any Xen headers and (2) to add
consistency for code in both kdd.c and kdd-xen.c.

Signed-off-by: Costin Lupu 
Reviewed-by: Tim Deegan 
---
 tools/debugger/kdd/kdd-xen.c | 15 ++-
 tools/debugger/kdd/kdd.c | 19 ---
 tools/debugger/kdd/kdd.h |  7 +++
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..e78c9311c4 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,9 +48,6 @@
 
 #define MAPSIZE 4093 /* Prime */
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (1U << PAGE_SHIFT)
-
 struct kdd_guest {
 struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
 xc_interface *xc_handle;
@@ -72,7 +69,7 @@ static void flush_maps(kdd_guest *g)
 int i;
 for (i = 0; i < MAPSIZE; i++) {
 if (g->maps[i] != NULL)
-munmap(g->maps[i], PAGE_SIZE);
+munmap(g->maps[i], KDD_PAGE_SIZE);
 g->maps[i] = NULL;
 }
 }
@@ -490,13 +487,13 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 uint32_t map_pfn, map_offset;
 uint8_t *map;
 
-map_pfn = (addr >> PAGE_SHIFT);
-map_offset = addr & (PAGE_SIZE - 1);
+map_pfn = (addr >> KDD_PAGE_SHIFT);
+map_offset = addr & (KDD_PAGE_SIZE - 1);
 
 /* Evict any mapping of the wrong frame from our slot */ 
 if (g->pfns[map_pfn % MAPSIZE] != map_pfn
 && g->maps[map_pfn % MAPSIZE] != NULL) {
-munmap(g->maps[map_pfn % MAPSIZE], PAGE_SIZE);
+munmap(g->maps[map_pfn % MAPSIZE], KDD_PAGE_SIZE);
 g->maps[map_pfn % MAPSIZE] = NULL;
 }
 g->pfns[map_pfn % MAPSIZE] = map_pfn;
@@ -507,7 +504,7 @@ static uint32_t kdd_access_physical_page(kdd_guest *g, 
uint64_t addr,
 else {
 map = xc_map_foreign_range(g->xc_handle,
g->domid,
-   PAGE_SIZE,
+   KDD_PAGE_SIZE,
PROT_READ|PROT_WRITE,
map_pfn);
 
@@ -533,7 +530,7 @@ uint32_t kdd_access_physical(kdd_guest *g, uint64_t addr,
 {
 uint32_t chunk, rv, done = 0;
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = KDD_PAGE_SIZE - (addr & (KDD_PAGE_SIZE - 1));
 if (chunk > len) 
 chunk = len;
 rv = kdd_access_physical_page(g, addr, chunk, buf, write);
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..320c623eda 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -288,9 +288,6 @@ static void kdd_log_pkt(kdd_state *s, const char *name, 
kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
-#define PAGE_SHIFT (12)
-#define PAGE_SIZE (1ULL << PAGE_SHIFT) 
-
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
   uint32_t len, void *buf)
 {
@@ -352,7 +349,7 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 
 /* Walk the appropriate number of levels */
 for (i = levels; i > 0; i--) {
-shift = PAGE_SHIFT + bits * (i-1);
+shift = KDD_PAGE_SHIFT + bits * (i-1);
 mask = ((1ULL << bits) - 1) << shift;
 offset = ((va & mask) >> shift) * width;
 KDD_DEBUG(s, "level %i: mask 0x%16.16"PRIx64" pa 0x%16.16"PRIx64
@@ -364,12 +361,12 @@ static uint64_t v2p(kdd_state *s, int cpuid, uint64_t va)
 return -1ULL; // Not present
 pa = entry & 0x000ff000ULL;
 if (pse && (i == 2) && (entry & 0x80)) { // Superpage
-mask = ((1ULL << (PAGE_SHIFT + bits)) - 1);
+mask = ((1ULL << (KDD_PAGE_SHIFT + bits)) - 1);
 return (pa & ~mask) + (va & mask);
 }
 }
 
-return pa + (va & (PAGE_SIZE - 1));
+return pa + (va & (KDD_PAGE_SIZE - 1));
 }
 
 static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, uint64_t addr,
@@ -380,7 +377,7 @@ static uint32_t kdd_access_virtual(kdd_state *s, int cpuid, 
uint64_t addr,
 
 /* Process one page at a time */
 while (len > 0) {
-chunk = PAGE_SIZE - (addr & (PAGE_SIZE - 1));
+chunk = KDD_PAGE_SIZE - (addr 

[PATCH v4 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu 
Reviewed-by: Julien Grall 
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c 
b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC  0xEF53   /* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO  2   /* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX1024   /* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT 5   /* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c 
b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 017) == 004)
 #define S_ISLNK(mode) (((mode) & 017) == 012)
 
+#ifndef PATH_MAX
 #define PATH_MAX   1024/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT5/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

[PATCH v4 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity.

The exception is in osdep_xenforeignmemory_map() where we need the system page
size to check whether the PFN array should be allocated with mmap() or with
dynamic allocation.

Signed-off-by: Costin Lupu 
---
 tools/libs/foreignmemory/core.c|  2 +-
 tools/libs/foreignmemory/freebsd.c | 10 +-
 tools/libs/foreignmemory/linux.c   | 23 ---
 tools/libs/foreignmemory/minios.c  |  2 +-
 tools/libs/foreignmemory/netbsd.c  | 10 +-
 tools/libs/foreignmemory/private.h |  9 +
 6 files changed, 25 insertions(+), 31 deletions(-)

diff --git a/tools/libs/foreignmemory/core.c b/tools/libs/foreignmemory/core.c
index 28ec311af1..7edc6f0dbf 100644
--- a/tools/libs/foreignmemory/core.c
+++ b/tools/libs/foreignmemory/core.c
@@ -202,7 +202,7 @@ int xenforeignmemory_resource_size(
 if ( rc )
 return rc;
 
-*size = fres.nr_frames << PAGE_SHIFT;
+*size = fres.nr_frames << XC_PAGE_SHIFT;
 return 0;
 }
 
diff --git a/tools/libs/foreignmemory/freebsd.c 
b/tools/libs/foreignmemory/freebsd.c
index d94ea07862..2cf0fa1c38 100644
--- a/tools/libs/foreignmemory/freebsd.c
+++ b/tools/libs/foreignmemory/freebsd.c
@@ -63,7 +63,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 privcmd_mmapbatch_t ioctlx;
 int rc;
 
-addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
+addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED, fd, 0);
 if ( addr == MAP_FAILED )
 return NULL;
 
@@ -78,7 +78,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -89,7 +89,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -101,7 +101,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(xenforeignmemory_handle *fmem,
 xenforeignmemory_resource_handle *fres)
 {
-return fres ? munmap(fres->addr, fres->nr_frames << PAGE_SHIFT) : 0;
+return fres ? munmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT) : 0;
 }
 
 int osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
@@ -120,7 +120,7 @@ int 
osdep_xenforeignmemory_map_resource(xenforeignmemory_handle *fmem,
 /* Request for resource size.  Skip mmap(). */
 goto skip_mmap;
 
-fres->addr = mmap(fres->addr, fres->nr_frames << PAGE_SHIFT,
+fres->addr = mmap(fres->addr, fres->nr_frames << XC_PAGE_SHIFT,
   fres->prot, fres->flags | MAP_SHARED, fmem->fd, 0);
 if ( fres->addr == MAP_FAILED )
 return -1;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index c1f35e2db7..9062117407 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -134,7 +134,7 @@ static int retry_paged(int fd, uint32_t dom, void *addr,
 /* At least one gfn is still in paging state */
 ioctlx.num = 1;
 ioctlx.dom = dom;
-ioctlx.addr = (unsigned long)addr + (i< PAGE_SIZE )
+if ( pfn_arr_size > os_page_size )
 munmap(pfn, pfn_arr_size);
 
 if ( rc == -ENOENT && i == num )
@@ -270,7 +271,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 {
 int saved_errno = errno;
 
-(void)munmap(addr, num << PAGE_SHIFT);
+(void)munmap(addr, num << XC_PAGE_SHIFT);
 errno = saved_errno;
 return NULL;
 }
@@ -281,7 +282,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap(xenforeignmemory_handle *fmem,
  void *addr, size_t num)
 {
-return munmap(addr, num << PAGE_SHIFT);
+return munmap(addr, num << XC_PAGE_SHIFT);
 }
 
 int osdep_xenforeignmemory_restrict(xenforeignmemory_handle *fmem,
@@ -293,7 +294,7 @@ int osdep_xenforeignmemory_restrict(xenforeignmemory_handle 
*fmem,
 int osdep_xenforeignmemory_unmap_resource(
 xenforeignmemory_handle *fmem, xenforeignmemory_r

[PATCH v4 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity.

The exception is in osdep_xenforeignmemory_map() where we need the system page
size to check whether the PFN array should be allocated with mmap() or with
dynamic allocation.

Signed-off-by: Costin Lupu 
---
 tools/libs/gnttab/freebsd.c | 28 +---
 tools/libs/gnttab/linux.c   | 28 +---
 tools/libs/gnttab/netbsd.c  | 23 ++-
 3 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index 768af701c6..e42ac3fbf3 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -30,14 +30,11 @@
 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#define PAGE_SIZE(1UL << PAGE_SHIFT)
-#define PAGE_MASK(~(PAGE_SIZE-1))
-
 #define DEVXEN "/dev/xen/gntdev"
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
@@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 int domids_stride;
 unsigned int refs_size = ROUNDUP(count *
  sizeof(struct ioctl_gntdev_grant_ref),
- PAGE_SHIFT);
+ XC_PAGE_SHIFT);
+int os_page_size = getpagesize();
 
 domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
-if ( refs_size <= PAGE_SIZE )
+if ( refs_size <= os_page_size )
 map.refs = malloc(refs_size);
 else
 {
@@ -107,7 +105,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 goto out;
 }
 
-addr = mmap(NULL, PAGE_SIZE * count, prot, MAP_SHARED, fd,
+addr = mmap(NULL, XC_PAGE_SIZE * count, prot, MAP_SHARED, fd,
 map.index);
 if ( addr != MAP_FAILED )
 {
@@ -116,7 +114,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 
 notify.index = map.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -131,7 +129,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 if ( rv )
 {
 GTERROR(xgt->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(addr, count * PAGE_SIZE);
+munmap(addr, count * XC_PAGE_SIZE);
 addr = MAP_FAILED;
 }
 }
@@ -150,7 +148,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
 }
 
  out:
-if ( refs_size > PAGE_SIZE )
+if ( refs_size > os_page_size )
 munmap(map.refs, refs_size);
 else
 free(map.refs);
@@ -189,7 +187,7 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 }
 
 /* Next, unmap the memory. */
-if ( (rc = munmap(start_address, count * PAGE_SIZE)) )
+if ( (rc = munmap(start_address, count * XC_PAGE_SIZE)) )
 return rc;
 
 /* Finally, unmap the driver slots used to store the grant information. */
@@ -256,7 +254,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 goto out;
 }
 
-area = mmap(NULL, count * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
+area = mmap(NULL, count * XC_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
 fd, gref_info.index);
 
 if ( area == MAP_FAILED )
@@ -268,7 +266,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 
 notify.index = gref_info.index;
 notify.action = 0;
-if ( notify_offset < PAGE_SIZE * count )
+if ( notify_offset < XC_PAGE_SIZE * count )
 {
 notify.index += notify_offset;
 notify.action |= UNMAP_NOTIFY_CLEAR_BYTE;
@@ -283,7 +281,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 if ( err )
 {
 GSERROR(xgs->logger, "ioctl SET_UNMAP_NOTIFY failed");
-munmap(area, count * PAGE_SIZE);
+munmap(area, count * XC_PAGE_SIZE);
 area = NULL;
 }
 
@@ -306,7 +304,7 @@ void *osdep_gntshr_share_pages(xengntshr_handle *xgs,
 int osdep_gntshr_unshare(xengntshr_handle *xgs,
  void *start_address, uint32_t count)
 {
-return munmap(start_address, count * PAGE_SIZE);
+return munmap(start_address, count * XC_PAGE_SIZE);
 }
 
 /*
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..5628fd5719 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -32,14 +32,11 @@
 #include 
 #include 
 
+#include 
 #include 
 
 #include "private.h"
 
-#define PAGE_SHIFT   12
-#de

[PATCH v4 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

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
patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order to avoid
confusion between control domain page granularity (PAGE_* definitions) and
guest domain page granularity (which is what we are dealing with here).

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already define in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu 
Reviewed-by: Julien Grall 
Tested-by: Dario Faggioli 
---
 tools/ocaml/libs/xc/xenctrl_stubs.c| 10 --
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c |  4 
 tools/ocaml/libs/xl/xenlight_stubs.c   |  4 
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..f9e33e599a 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,12 @@
 
 #include "mmap_stubs.h"
 
-#define PAGE_SHIFT 12
-#define PAGE_SIZE   (1UL << PAGE_SHIFT)
-#define PAGE_MASK   (~(PAGE_SIZE-1))
-
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
((Field(array, index) == Val_none) ? NULL : 
String_val(Field(Field(array, index), 0)))
@@ -818,7 +816,7 @@ CAMLprim value 
stub_xc_domain_memory_increase_reservation(value xch,
CAMLparam3(xch, domid, mem_kb);
int retval;
 
-   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(PAGE_SHIFT - 10);
+   unsigned long nr_extents = ((unsigned long)(Int64_val(mem_kb))) >> 
(XC_PAGE_SHIFT - 10);
 
uint32_t c_domid = _D(domid);
caml_enter_blocking_section();
@@ -924,7 +922,7 @@ CAMLprim value stub_pages_to_kib(value pages)
 {
CAMLparam1(pages);
 
-   CAMLreturn(caml_copy_int64(Int64_val(pages) << (PAGE_SHIFT - 10)));
+   CAMLreturn(caml_copy_int64(Int64_val(pages) << (XC_PAGE_SHIFT - 10)));
 }
 
 
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

Re: [PATCH v3 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 5/17/21 9:12 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 10/05/2021 09:35, Costin Lupu wrote:
>> @@ -168,7 +168,7 @@ void
>> *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>>   size_t i;
>>   int rc;
>>   -    addr = mmap(addr, num << PAGE_SHIFT, prot, flags | MAP_SHARED,
>> +    addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED,
>>   fd, 0);
>>   if ( addr == MAP_FAILED )
>>   return NULL;
>> @@ -198,9 +198,10 @@ void
>> *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>>    */
>>   privcmd_mmapbatch_t ioctlx;
>>   xen_pfn_t *pfn;
>> -    unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
>> PAGE_SHIFT);
>> +    unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)),
>> XC_PAGE_SHIFT);
>> +    int os_page_size = getpagesize();
> 
> Hmmm... Sorry I only realized now that the manpage suggests that
> getpagesize() is legacy:
> 
>    SVr4, 4.4BSD, SUSv2.  In SUSv2 the getpagesize() call is labeled
> LEGACY, and in POSIX.1-2001 it has been dropped; HP-UX does not have
> this call.
> 
> And then:
> 
>   Portable applications should employ sysconf(_SC_PAGESIZE) instead of
> getpagesize():
> 
>   #include 
>   long sz = sysconf(_SC_PAGESIZE);
> 
> As this is only used by Linux, it is not clear to me whether this is
> important. Ian, what do you think?
> 

I think it would be safer to follow the man page indication. I've just
sent a v4.

>>   -    if ( pfn_arr_size <= PAGE_SIZE )
>> +    if ( pfn_arr_size <= os_page_size )
> 
> Your commit message suggests we are only s/PAGE_SHIFT/XC_PAGE_SHIFT/ but
> this is using getpagesize() instead. I agree it should be using the OS
> size. However, this should be clarified in the commit message.
> 

Done.

> The rest of the patch looks fine to me .

Thanks,
Costin

Re: [PATCH v3 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

Hi Julien,

On 5/17/21 9:16 PM, Julien Grall wrote:
> Hi Costin,
> 
> On 10/05/2021 09:35, Costin Lupu wrote:
>> 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
>> patch replaces usage of PAGE_* macros with XC_PAGE_* macros in order
>> to avoid
>> confusion between control domain page granularity (PAGE_* definitions)
>> and
>> guest domain page granularity (which is what we are dealing with here).
>>
>> Signed-off-by: Costin Lupu 
>> ---
>>   tools/libs/gnttab/freebsd.c | 28 +---
>>   tools/libs/gnttab/linux.c   | 28 +---
>>   tools/libs/gnttab/netbsd.c  | 23 ++-
>>   3 files changed, 36 insertions(+), 43 deletions(-)
>>
>> diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
>> index 768af701c6..e42ac3fbf3 100644
>> --- a/tools/libs/gnttab/freebsd.c
>> +++ b/tools/libs/gnttab/freebsd.c
>> @@ -30,14 +30,11 @@
>>     #include 
>>   +#include 
>>   #include 
>>     #include "private.h"
>>   -#define PAGE_SHIFT   12
>> -#define PAGE_SIZE    (1UL << PAGE_SHIFT)
>> -#define PAGE_MASK    (~(PAGE_SIZE-1))
>> -
>>   #define DEVXEN "/dev/xen/gntdev"
>>     int osdep_gnttab_open(xengnttab_handle *xgt)
>> @@ -77,10 +74,11 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
>>   int domids_stride;
>>   unsigned int refs_size = ROUNDUP(count *
>>    sizeof(struct
>> ioctl_gntdev_grant_ref),
>> - PAGE_SHIFT);
>> + XC_PAGE_SHIFT);
>> +    int os_page_size = getpagesize();
> 
> Same remark as for patch #4. This at least want to be explained in the
> commit message.

Done.


Thanks,
Costin

[PATCH] tools/libs/light: Remove unnecessary libxl_list_vm() call

[Costin Lupu]

The removed lines were initially added by commit 314e64084d31, but the
subsequent code which was using the nb_vm variable was later removed by
commit 2ba368d13893, which makes these lines of code an overlooked
reminiscence. Moreover, the call becomes very expensive when there is a
considerable number of VMs (~1000 instances) running on the host.

Signed-off-by: Costin Lupu 
---
 tools/libs/light/libxl_create.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 0c64268f66..43e9ba9c63 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -578,7 +578,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
*d_config,
uint32_t *domid, bool soft_reset)
 {
 libxl_ctx *ctx = libxl__gc_owner(gc);
-int ret, rc, nb_vm;
+int ret, rc;
 const char *dom_type;
 char *uuid_string;
 char *dom_path, *vm_path, *libxl_path;
@@ -586,7 +586,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config 
*d_config,
 struct xs_permissions rwperm[1];
 struct xs_permissions noperm[1];
 xs_transaction_t t = 0;
-libxl_vminfo *vm_list;
 
 /* convenience aliases */
 libxl_domain_create_info *info = &d_config->c_info;
@@ -869,14 +868,6 @@ retry_transaction:
 ARRAY_SIZE(rwperm));
 }
 
-vm_list = libxl_list_vm(ctx, &nb_vm);
-if (!vm_list) {
-LOGD(ERROR, *domid, "cannot get number of running guests");
-rc = ERROR_FAIL;
-goto out;
-}
-libxl_vminfo_list_free(vm_list, nb_vm);
-
 xs_write(ctx->xsh, t, GCSPRINTF("%s/uuid", vm_path), uuid_string, 
strlen(uuid_string));
 xs_write(ctx->xsh, t, GCSPRINTF("%s/name", vm_path), info->name, 
strlen(info->name));
 
-- 
2.20.1

[PATCH 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

For replication I used gcc 10.3 on an Alpine system. In order to replicate the
redefinition error for PAGE_SIZE one should install the 'fortify-headers'
package which will change the chain of included headers by indirectly including
/usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.

Costin Lupu (5):
  tools/debugger: Fix PAGE_SIZE redefinition error
  tools/libfsimage: Fix PATH_MAX redefinition error
  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
  tools/ocaml: Fix redefinition errors

 tools/debugger/kdd/kdd-xen.c   | 4 
 tools/debugger/kdd/kdd.c   | 4 
 tools/libfsimage/ext2fs/fsys_ext2fs.c  | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c  | 2 ++
 tools/libs/foreignmemory/private.h | 6 --
 tools/libs/gnttab/linux.c  | 6 ++
 tools/ocaml/libs/xc/xenctrl_stubs.c| 8 
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 
 tools/ocaml/libs/xl/xenlight_stubs.c   | 4 
 9 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/5] tools/debugger: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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 definitions for
keeping consistency.

Signed-off-by: Costin Lupu 
---
 tools/debugger/kdd/kdd-xen.c | 4 
 tools/debugger/kdd/kdd.c | 4 
 2 files changed, 8 insertions(+)

diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c
index f3f9529f9f..04d2361ba7 100644
--- a/tools/debugger/kdd/kdd-xen.c
+++ b/tools/debugger/kdd/kdd-xen.c
@@ -48,8 +48,12 @@
 
 #define MAPSIZE 4093 /* Prime */
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT 12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE (1U << PAGE_SHIFT)
+#endif
 
 struct kdd_guest {
 struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */
diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c
index 17513c2650..acd5832714 100644
--- a/tools/debugger/kdd/kdd.c
+++ b/tools/debugger/kdd/kdd.c
@@ -288,8 +288,12 @@ static void kdd_log_pkt(kdd_state *s, const char *name, 
kdd_pkt *p)
  *  Memory access: virtual addresses and syntactic sugar.
  */
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT (12)
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE (1ULL << PAGE_SHIFT) 
+#endif
 
 static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, 
   uint32_t len, void *buf)
-- 
2.20.1

[PATCH 4/5] tools/libs/gnttab: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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 
---
 tools/libs/gnttab/linux.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 74331a4c7b..e12f2697a5 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -36,9 +36,15 @@
 
 #include "private.h"
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT   12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE(1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK(~(PAGE_SIZE-1))
+#endif
 
 #define DEVXEN "/dev/xen/"
 
-- 
2.20.1

[PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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 
---
 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 
 #include 
 
-#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
 
-- 
2.20.1

[PATCH 5/5] tools/ocaml: Fix redefinition errors

[Costin Lupu]

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.

Same issue applies for redefinitions of Val_none and Some_val macros which
can be already define in the OCaml system headers (e.g.
/usr/lib/ocaml/caml/mlvalues.h).

Signed-off-by: Costin Lupu 
---
 tools/ocaml/libs/xc/xenctrl_stubs.c| 8 
 tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 
 tools/ocaml/libs/xl/xenlight_stubs.c   | 4 
 3 files changed, 16 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
b/tools/ocaml/libs/xc/xenctrl_stubs.c
index d05d7bb30e..1c82e76b19 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -36,14 +36,22 @@
 
 #include "mmap_stubs.h"
 
+#ifndef PAGE_SHIFT
 #define PAGE_SHIFT 12
+#endif
+#ifndef PAGE_SIZE
 #define PAGE_SIZE   (1UL << PAGE_SHIFT)
+#endif
+#ifndef PAGE_MASK
 #define PAGE_MASK   (~(PAGE_SIZE-1))
+#endif
 
 #define _H(__h) ((xc_interface *)(__h))
 #define _D(__d) ((uint32_t)Int_val(__d))
 
+#ifndef Val_none
 #define Val_none (Val_int(0))
+#endif
 
 #define string_of_option_array(array, index) \
((Field(array, index) == Val_none) ? NULL : 
String_val(Field(Field(array, index), 0)))
diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c 
b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
index bf64b211c2..e4306a0c2f 100644
--- a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
+++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
@@ -53,8 +53,12 @@ static char * dup_String_val(value s)
 #include "_xtl_levels.inc"
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c 
b/tools/ocaml/libs/xl/xenlight_stubs.c
index 352a00134d..45b8af61c7 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -227,8 +227,12 @@ static value Val_string_list(libxl_string_list *c_val)
 }
 
 /* Option type support as per 
http://www.linux-nantes.org/~fmonnier/ocaml/ocaml-wrapping-c.php */
+#ifndef Val_none
 #define Val_none Val_int(0)
+#endif
+#ifndef Some_val
 #define Some_val(v) Field(v,0)
+#endif
 
 static value Val_some(value v)
 {
-- 
2.20.1

[PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

If PATH_MAX is already defined in the system (e.g. in /usr/include/limits.h
header) then gcc will trigger a redefinition error because of -Werror.

Signed-off-by: Costin Lupu 
---
 tools/libfsimage/ext2fs/fsys_ext2fs.c | 2 ++
 tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c 
b/tools/libfsimage/ext2fs/fsys_ext2fs.c
index a4ed10419c..5ed8fce90e 100644
--- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
+++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
@@ -278,7 +278,9 @@ struct ext4_extent_header {
 
 #define EXT2_SUPER_MAGIC  0xEF53   /* include/linux/ext2_fs.h */
 #define EXT2_ROOT_INO  2   /* include/linux/ext2_fs.h */
+#ifndef PATH_MAX
 #define PATH_MAX1024   /* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT 5   /* number of symbolic links to follow */
 
 /* made up, these are pointers into FSYS_BUF */
diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c 
b/tools/libfsimage/reiserfs/fsys_reiserfs.c
index 916eb15292..10ca657476 100644
--- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
+++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
@@ -284,7 +284,9 @@ struct reiserfs_de_head
 #define S_ISDIR(mode) (((mode) & 017) == 004)
 #define S_ISLNK(mode) (((mode) & 017) == 012)
 
+#ifndef PATH_MAX
 #define PATH_MAX   1024/* include/linux/limits.h */
+#endif
 #define MAX_LINK_COUNT5/* number of symbolic links to follow */
 
 /* The size of the node cache */
-- 
2.20.1

Re: [PATCH 3/5] tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error

[Costin Lupu]

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 
>>   #include 
>>   -#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

Re: [PATCH 2/5] tools/libfsimage: Fix PATH_MAX redefinition error

[Costin Lupu]

On 4/28/21 12:04 PM, Julien Grall wrote:
> 
> 
> On 27/04/2021 13:05, Costin Lupu wrote:
>> If PATH_MAX is already defined in the system (e.g. in
>> /usr/include/limits.h
>> header) then gcc will trigger a redefinition error because of -Werror.
>>
>> Signed-off-by: Costin Lupu 
>> ---
>>   tools/libfsimage/ext2fs/fsys_ext2fs.c | 2 ++
>>   tools/libfsimage/reiserfs/fsys_reiserfs.c | 2 ++
>>   2 files changed, 4 insertions(+)
>>
>> diff --git a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> index a4ed10419c..5ed8fce90e 100644
>> --- a/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> +++ b/tools/libfsimage/ext2fs/fsys_ext2fs.c
>> @@ -278,7 +278,9 @@ struct ext4_extent_header {
>>     #define EXT2_SUPER_MAGIC  0xEF53    /* include/linux/ext2_fs.h */
>>   #define EXT2_ROOT_INO  2    /* include/linux/ext2_fs.h */
>> +#ifndef PATH_MAX
>>   #define PATH_MAX    1024    /* include/linux/limits.h */
>> +#endif
> 
> Can we drop it completely and just rely on limits.h?
> 

One problem here is that the system limits.h header doesn't necessarily
include linux/limits.h, which would mean we would have to include
linux/limits.h. But this is problematic for other systems such as BSD.

I had a look on a FreeBSD source tree to see how this is done there. It
seems that there are lots of submodules, apps and libs that redefine
PATH_MAX in case it wasn't defined before so the changes introduced by
the current patch seem to be very popular. Another clean approach I saw
was for jemalloc [1] which includes unistd.h. They redefine PATH_MAX
only for MS C compiler, but AFAIK we don't need that.

So IMHO the current changes seem to be the most portable, but I'm open
to any suggestions.

[1]
https://github.com/jemalloc/jemalloc/blob/dev/include/jemalloc/internal/jemalloc_internal_decls.h#L76


>>   #define MAX_LINK_COUNT 5    /* number of symbolic links
>> to follow */
>>     /* made up, these are pointers into FSYS_BUF */
>> diff --git a/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> b/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> index 916eb15292..10ca657476 100644
>> --- a/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> +++ b/tools/libfsimage/reiserfs/fsys_reiserfs.c
>> @@ -284,7 +284,9 @@ struct reiserfs_de_head
>>   #define S_ISDIR(mode) (((mode) & 017) == 004)
>>   #define S_ISLNK(mode) (((mode) & 017) == 012)
>>   +#ifndef PATH_MAX
>>   #define PATH_MAX   1024    /* include/linux/limits.h */
>> +#endif
>>   #define MAX_LINK_COUNT    5    /* number of symbolic links to follow */
>>     /* The size of the node cache */
>>
> 

Re: [PATCH 0/5] Fix redefinition errors for toolstack libs

[Costin Lupu]

On 4/28/21 3:34 PM, Christian Lindig wrote:
> 
> 
>> On 27 Apr 2021, at 13:05, Costin Lupu > <mailto:costin.l...@cs.pub.ro>> wrote:
>>
>> For replication I used gcc 10.3 on an Alpine system. In order to
>> replicate the
>> redefinition error for PAGE_SIZE one should install the 'fortify-headers'
>> package which will change the chain of included headers by indirectly
>> including
>> /usr/include/limits.h where PAGE_SIZE and PATH_MAX are defined.
>>
>> Costin Lupu (5):
>>  tools/debugger: Fix PAGE_SIZE redefinition error
>>  tools/libfsimage: Fix PATH_MAX redefinition error
>>  tools/libs/foreignmemory: Fix PAGE_SIZE redefinition error
>>  tools/libs/gnttab: Fix PAGE_SIZE redefinition error
>>  tools/ocaml: Fix redefinition errors
>>
>> tools/debugger/kdd/kdd-xen.c   | 4 
>> tools/debugger/kdd/kdd.c   | 4 
>> tools/libfsimage/ext2fs/fsys_ext2fs.c  | 2 ++
>> tools/libfsimage/reiserfs/fsys_reiserfs.c  | 2 ++
>> tools/libs/foreignmemory/private.h | 6 --
>> tools/libs/gnttab/linux.c  | 6 ++
>> tools/ocaml/libs/xc/xenctrl_stubs.c    | 8 
>> tools/ocaml/libs/xentoollog/xentoollog_stubs.c | 4 
>> tools/ocaml/libs/xl/xenlight_stubs.c   | 4 
>> 9 files changed, 38 insertions(+), 2 deletions(-)
>>
>> —
>> 2.20.1
>>
> 
> For the OCaml bindings, this avoids redefinitions as you say. Looks good
> to me.
> 
> Acked-by: Christian Lindig  <mailto:christian.lin...@citrix.com>>
> 

Thanks, Christian, I'll add your ack on the Ocaml patch for the next
version of the series.

Cheers,
Costin