Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 16:03,  wrote:
> On 12/19/2017 09:40 AM, Jan Beulich wrote:
> On 19.12.17 at 15:25,  wrote:
>>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
> + memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
 Is it really reasonable to have a static upper bound here? As we
 know especially EFI systems can come with a pretty scattered
 (pseudo) E820 table. Even if (iirc) this has a static upper bound
 right now in the hypervisor too, it would be nice if the kernel
 didn't need further changes once the hypervisor is being made
 more flexible.
>>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>>> that we should query for the size first?
>> That would be better, I think.
> 
> 
> I think we will first need to fix xen_memory_setup() to do that too and
> that would be a separate patch.
> 
> I am also not clear how this will work on earlier version of the
> hypervisor that didn't support querying for size. From what I am seeing
> in 4.4 we will get -EFAULT if the buffer is NULL.

That's not nice, I agree, but can be dealt with.

> + /* Mark non-RAM regions as not available. */
> + for (; i < memmap.nr_entries; i++) {
> + entry = _e820_table->entries[i];
> +
> + if (entry->type == E820_TYPE_RAM)
> + continue;
 I can't seem to match up this with ...

> + if (entry->addr >= hostmem_resource->end)
> + break;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + goto out;
> +
> + res->name = "Host memory";
 ... this. Do you mean != instead (with the comment ahead of the
 loop also clarified, saying something like "host RAM regions which
 aren't RAM for us")? And perhaps better "Host RAM"?
>>> Right, this is not memory but rather something else (and so "!=" is
>>> correct). "Unavailable host RAM"?
>> If you like to be even more specific than what I had suggested -
>> sure.
> 
> But did you want to have some changes in the preceding comment? Not sure
> I read your comment correctly.

Well, "non-RAM" is ambiguous in this context, so yes, I'd prefer it
to be clarified. Whether you use what I've suggested or something
else I don't care much.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 16:03,  wrote:
> On 12/19/2017 09:40 AM, Jan Beulich wrote:
> On 19.12.17 at 15:25,  wrote:
>>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
> + memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
 Is it really reasonable to have a static upper bound here? As we
 know especially EFI systems can come with a pretty scattered
 (pseudo) E820 table. Even if (iirc) this has a static upper bound
 right now in the hypervisor too, it would be nice if the kernel
 didn't need further changes once the hypervisor is being made
 more flexible.
>>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>>> that we should query for the size first?
>> That would be better, I think.
> 
> 
> I think we will first need to fix xen_memory_setup() to do that too and
> that would be a separate patch.
> 
> I am also not clear how this will work on earlier version of the
> hypervisor that didn't support querying for size. From what I am seeing
> in 4.4 we will get -EFAULT if the buffer is NULL.

That's not nice, I agree, but can be dealt with.

> + /* Mark non-RAM regions as not available. */
> + for (; i < memmap.nr_entries; i++) {
> + entry = _e820_table->entries[i];
> +
> + if (entry->type == E820_TYPE_RAM)
> + continue;
 I can't seem to match up this with ...

> + if (entry->addr >= hostmem_resource->end)
> + break;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + goto out;
> +
> + res->name = "Host memory";
 ... this. Do you mean != instead (with the comment ahead of the
 loop also clarified, saying something like "host RAM regions which
 aren't RAM for us")? And perhaps better "Host RAM"?
>>> Right, this is not memory but rather something else (and so "!=" is
>>> correct). "Unavailable host RAM"?
>> If you like to be even more specific than what I had suggested -
>> sure.
> 
> But did you want to have some changes in the preceding comment? Not sure
> I read your comment correctly.

Well, "non-RAM" is ambiguous in this context, so yes, I'd prefer it
to be clarified. Whether you use what I've suggested or something
else I don't care much.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 12/19/2017 09:40 AM, Jan Beulich wrote:
 On 19.12.17 at 15:25,  wrote:
>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>> On 18.12.17 at 23:22,  wrote:
 +  if (!xen_e820_table)
 +  return;
>>> Not saying "out of memory" here is certainly fine, but shouldn't
>>> there nevertheless be a warning, as failure to go through the
>>> rest of the function will impact overall functionality?
>> Commit ebfdc40969f claims that these types of messages are unnecessary
>> because allocation failures are signalled by the memory subsystem.
> But the memory subsystem can't possibly provide an indication of
> what will not work because of the failed allocation.


There should be a stack dump which will make it clear which routine failed.


>
 +  memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>>> Is it really reasonable to have a static upper bound here? As we
>>> know especially EFI systems can come with a pretty scattered
>>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>>> right now in the hypervisor too, it would be nice if the kernel
>>> didn't need further changes once the hypervisor is being made
>>> more flexible.
>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>> that we should query for the size first?
> That would be better, I think.


I think we will first need to fix xen_memory_setup() to do that too and
that would be a separate patch.

I am also not clear how this will work on earlier version of the
hypervisor that didn't support querying for size. From what I am seeing
in 4.4 we will get -EFAULT if the buffer is NULL.


>
 +  /* Mark non-RAM regions as not available. */
 +  for (; i < memmap.nr_entries; i++) {
 +  entry = _e820_table->entries[i];
 +
 +  if (entry->type == E820_TYPE_RAM)
 +  continue;
>>> I can't seem to match up this with ...
>>>
 +  if (entry->addr >= hostmem_resource->end)
 +  break;
 +
 +  res = kzalloc(sizeof(*res), GFP_KERNEL);
 +  if (!res)
 +  goto out;
 +
 +  res->name = "Host memory";
>>> ... this. Do you mean != instead (with the comment ahead of the
>>> loop also clarified, saying something like "host RAM regions which
>>> aren't RAM for us")? And perhaps better "Host RAM"?
>> Right, this is not memory but rather something else (and so "!=" is
>> correct). "Unavailable host RAM"?
> If you like to be even more specific than what I had suggested -
> sure.

But did you want to have some changes in the preceding comment? Not sure
I read your comment correctly.

-boris

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 12/19/2017 09:40 AM, Jan Beulich wrote:
 On 19.12.17 at 15:25,  wrote:
>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>> On 18.12.17 at 23:22,  wrote:
 +  if (!xen_e820_table)
 +  return;
>>> Not saying "out of memory" here is certainly fine, but shouldn't
>>> there nevertheless be a warning, as failure to go through the
>>> rest of the function will impact overall functionality?
>> Commit ebfdc40969f claims that these types of messages are unnecessary
>> because allocation failures are signalled by the memory subsystem.
> But the memory subsystem can't possibly provide an indication of
> what will not work because of the failed allocation.


There should be a stack dump which will make it clear which routine failed.


>
 +  memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>>> Is it really reasonable to have a static upper bound here? As we
>>> know especially EFI systems can come with a pretty scattered
>>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>>> right now in the hypervisor too, it would be nice if the kernel
>>> didn't need further changes once the hypervisor is being made
>>> more flexible.
>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>> that we should query for the size first?
> That would be better, I think.


I think we will first need to fix xen_memory_setup() to do that too and
that would be a separate patch.

I am also not clear how this will work on earlier version of the
hypervisor that didn't support querying for size. From what I am seeing
in 4.4 we will get -EFAULT if the buffer is NULL.


>
 +  /* Mark non-RAM regions as not available. */
 +  for (; i < memmap.nr_entries; i++) {
 +  entry = _e820_table->entries[i];
 +
 +  if (entry->type == E820_TYPE_RAM)
 +  continue;
>>> I can't seem to match up this with ...
>>>
 +  if (entry->addr >= hostmem_resource->end)
 +  break;
 +
 +  res = kzalloc(sizeof(*res), GFP_KERNEL);
 +  if (!res)
 +  goto out;
 +
 +  res->name = "Host memory";
>>> ... this. Do you mean != instead (with the comment ahead of the
>>> loop also clarified, saying something like "host RAM regions which
>>> aren't RAM for us")? And perhaps better "Host RAM"?
>> Right, this is not memory but rather something else (and so "!=" is
>> correct). "Unavailable host RAM"?
> If you like to be even more specific than what I had suggested -
> sure.

But did you want to have some changes in the preceding comment? Not sure
I read your comment correctly.

-boris

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 15:25,  wrote:
> On 12/19/2017 03:23 AM, Jan Beulich wrote:
> On 18.12.17 at 23:22,  wrote:
>>> +   if (!xen_e820_table)
>>> +   return;
>> Not saying "out of memory" here is certainly fine, but shouldn't
>> there nevertheless be a warning, as failure to go through the
>> rest of the function will impact overall functionality?
> 
> Commit ebfdc40969f claims that these types of messages are unnecessary
> because allocation failures are signalled by the memory subsystem.

But the memory subsystem can't possibly provide an indication of
what will not work because of the failed allocation.

>>> +   memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>> Is it really reasonable to have a static upper bound here? As we
>> know especially EFI systems can come with a pretty scattered
>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>> right now in the hypervisor too, it would be nice if the kernel
>> didn't need further changes once the hypervisor is being made
>> more flexible.
> 
> This is how we obtain the map in xen_memory_setup(). Are you suggesting
> that we should query for the size first?

That would be better, I think.

>>> +   /* Mark non-RAM regions as not available. */
>>> +   for (; i < memmap.nr_entries; i++) {
>>> +   entry = _e820_table->entries[i];
>>> +
>>> +   if (entry->type == E820_TYPE_RAM)
>>> +   continue;
>> I can't seem to match up this with ...
>>
>>> +   if (entry->addr >= hostmem_resource->end)
>>> +   break;
>>> +
>>> +   res = kzalloc(sizeof(*res), GFP_KERNEL);
>>> +   if (!res)
>>> +   goto out;
>>> +
>>> +   res->name = "Host memory";
>> ... this. Do you mean != instead (with the comment ahead of the
>> loop also clarified, saying something like "host RAM regions which
>> aren't RAM for us")? And perhaps better "Host RAM"?
> 
> Right, this is not memory but rather something else (and so "!=" is
> correct). "Unavailable host RAM"?

If you like to be even more specific than what I had suggested -
sure.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 15:25,  wrote:
> On 12/19/2017 03:23 AM, Jan Beulich wrote:
> On 18.12.17 at 23:22,  wrote:
>>> +   if (!xen_e820_table)
>>> +   return;
>> Not saying "out of memory" here is certainly fine, but shouldn't
>> there nevertheless be a warning, as failure to go through the
>> rest of the function will impact overall functionality?
> 
> Commit ebfdc40969f claims that these types of messages are unnecessary
> because allocation failures are signalled by the memory subsystem.

But the memory subsystem can't possibly provide an indication of
what will not work because of the failed allocation.

>>> +   memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>> Is it really reasonable to have a static upper bound here? As we
>> know especially EFI systems can come with a pretty scattered
>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>> right now in the hypervisor too, it would be nice if the kernel
>> didn't need further changes once the hypervisor is being made
>> more flexible.
> 
> This is how we obtain the map in xen_memory_setup(). Are you suggesting
> that we should query for the size first?

That would be better, I think.

>>> +   /* Mark non-RAM regions as not available. */
>>> +   for (; i < memmap.nr_entries; i++) {
>>> +   entry = _e820_table->entries[i];
>>> +
>>> +   if (entry->type == E820_TYPE_RAM)
>>> +   continue;
>> I can't seem to match up this with ...
>>
>>> +   if (entry->addr >= hostmem_resource->end)
>>> +   break;
>>> +
>>> +   res = kzalloc(sizeof(*res), GFP_KERNEL);
>>> +   if (!res)
>>> +   goto out;
>>> +
>>> +   res->name = "Host memory";
>> ... this. Do you mean != instead (with the comment ahead of the
>> loop also clarified, saying something like "host RAM regions which
>> aren't RAM for us")? And perhaps better "Host RAM"?
> 
> Right, this is not memory but rather something else (and so "!=" is
> correct). "Unavailable host RAM"?

If you like to be even more specific than what I had suggested -
sure.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 12/19/2017 03:23 AM, Jan Beulich wrote:
 On 18.12.17 at 23:22,  wrote:

 +
 +  xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> Wouldn't kmalloc() suffice here?

Yes.

>
>> +if (!xen_e820_table)
>> +return;
> Not saying "out of memory" here is certainly fine, but shouldn't
> there nevertheless be a warning, as failure to go through the
> rest of the function will impact overall functionality?


Commit ebfdc40969f claims that these types of messages are unnecessary
because allocation failures are signalled by the memory subsystem.


>
>> +memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
> Is it really reasonable to have a static upper bound here? As we
> know especially EFI systems can come with a pretty scattered
> (pseudo) E820 table. Even if (iirc) this has a static upper bound
> right now in the hypervisor too, it would be nice if the kernel
> didn't need further changes once the hypervisor is being made
> more flexible.


This is how we obtain the map in xen_memory_setup(). Are you suggesting
that we should query for the size first?


>
>> +/* Mark non-RAM regions as not available. */
>> +for (; i < memmap.nr_entries; i++) {
>> +entry = _e820_table->entries[i];
>> +
>> +if (entry->type == E820_TYPE_RAM)
>> +continue;
> I can't seem to match up this with ...
>
>> +if (entry->addr >= hostmem_resource->end)
>> +break;
>> +
>> +res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +if (!res)
>> +goto out;
>> +
>> +res->name = "Host memory";
> ... this. Do you mean != instead (with the comment ahead of the
> loop also clarified, saying something like "host RAM regions which
> aren't RAM for us")? And perhaps better "Host RAM"?

Right, this is not memory but rather something else (and so "!=" is
correct). "Unavailable host RAM"?

>
>> +rc = insert_resource(hostmem_resource, res);
>> +if (rc) {
>> +pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",
> [%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
> (Same issues somewhere further down.)

This will not be built for non-PAE configurations because memory hotplug
requires PAE.

>
>> +__func__, res->start, res->end, rc);
>> +kfree(res);
>> +goto  out;
> Perhaps better not to bail out of the loop here (at least if rc is
> not -ENOMEM)?

We shouldn't get -ENOMEM here since resource insertion doesn't allocate
anything.

The reason I decided to bail here was because I thought that if we fail
once it means there is a bug somewhere (since we shouldn't really fail)
and so subsequent attempts to insert the range would fail as well.


-boris

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Boris Ostrovsky]

On 12/19/2017 03:23 AM, Jan Beulich wrote:
 On 18.12.17 at 23:22,  wrote:

 +
 +  xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);
> Wouldn't kmalloc() suffice here?

Yes.

>
>> +if (!xen_e820_table)
>> +return;
> Not saying "out of memory" here is certainly fine, but shouldn't
> there nevertheless be a warning, as failure to go through the
> rest of the function will impact overall functionality?


Commit ebfdc40969f claims that these types of messages are unnecessary
because allocation failures are signalled by the memory subsystem.


>
>> +memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
> Is it really reasonable to have a static upper bound here? As we
> know especially EFI systems can come with a pretty scattered
> (pseudo) E820 table. Even if (iirc) this has a static upper bound
> right now in the hypervisor too, it would be nice if the kernel
> didn't need further changes once the hypervisor is being made
> more flexible.


This is how we obtain the map in xen_memory_setup(). Are you suggesting
that we should query for the size first?


>
>> +/* Mark non-RAM regions as not available. */
>> +for (; i < memmap.nr_entries; i++) {
>> +entry = _e820_table->entries[i];
>> +
>> +if (entry->type == E820_TYPE_RAM)
>> +continue;
> I can't seem to match up this with ...
>
>> +if (entry->addr >= hostmem_resource->end)
>> +break;
>> +
>> +res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +if (!res)
>> +goto out;
>> +
>> +res->name = "Host memory";
> ... this. Do you mean != instead (with the comment ahead of the
> loop also clarified, saying something like "host RAM regions which
> aren't RAM for us")? And perhaps better "Host RAM"?

Right, this is not memory but rather something else (and so "!=" is
correct). "Unavailable host RAM"?

>
>> +rc = insert_resource(hostmem_resource, res);
>> +if (rc) {
>> +pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",
> [%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
> (Same issues somewhere further down.)

This will not be built for non-PAE configurations because memory hotplug
requires PAE.

>
>> +__func__, res->start, res->end, rc);
>> +kfree(res);
>> +goto  out;
> Perhaps better not to bail out of the loop here (at least if rc is
> not -ENOMEM)?

We shouldn't get -ENOMEM here since resource insertion doesn't allocate
anything.

The reason I decided to bail here was because I thought that if we fail
once it means there is a bug somewhere (since we shouldn't really fail)
and so subsequent attempts to insert the range would fail as well.


-boris

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Juergen Gross]

On 19/12/17 10:27, Jan Beulich wrote:
 On 19.12.17 at 10:21,  wrote:
>> On 19/12/17 09:23, Jan Beulich wrote:
>> On 18.12.17 at 23:22,  wrote:
 +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
 +{
 +  struct xen_memory_map memmap;
 +  int rc;
 +  unsigned int i, last_guest_ram;
 +  phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
>>>
>>> PFN_PHYS() as right now you still have an issue on 32-bit.
>>
>> Why? PFN_PHYS is defined as:
>>
>> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)
> 
> Well, that's why I suggested its use (just like you did in your
> own review). IOW - now I'm confused.

Sorry, just got your answer wrong.

As I had already found the same issue somehow I assumked this remark
would be referencing my review. Sorry for the noise.


Juergen

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Juergen Gross]

On 19/12/17 10:27, Jan Beulich wrote:
 On 19.12.17 at 10:21,  wrote:
>> On 19/12/17 09:23, Jan Beulich wrote:
>> On 18.12.17 at 23:22,  wrote:
 +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
 +{
 +  struct xen_memory_map memmap;
 +  int rc;
 +  unsigned int i, last_guest_ram;
 +  phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
>>>
>>> PFN_PHYS() as right now you still have an issue on 32-bit.
>>
>> Why? PFN_PHYS is defined as:
>>
>> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)
> 
> Well, that's why I suggested its use (just like you did in your
> own review). IOW - now I'm confused.

Sorry, just got your answer wrong.

As I had already found the same issue somehow I assumked this remark
would be referencing my review. Sorry for the noise.


Juergen

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 10:21,  wrote:
> On 19/12/17 09:23, Jan Beulich wrote:
> On 18.12.17 at 23:22,  wrote:
>>> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
>>> +{
>>> +   struct xen_memory_map memmap;
>>> +   int rc;
>>> +   unsigned int i, last_guest_ram;
>>> +   phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
>> 
>> PFN_PHYS() as right now you still have an issue on 32-bit.
> 
> Why? PFN_PHYS is defined as:
> 
> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)

Well, that's why I suggested its use (just like you did in your
own review). IOW - now I'm confused.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 19.12.17 at 10:21,  wrote:
> On 19/12/17 09:23, Jan Beulich wrote:
> On 18.12.17 at 23:22,  wrote:
>>> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
>>> +{
>>> +   struct xen_memory_map memmap;
>>> +   int rc;
>>> +   unsigned int i, last_guest_ram;
>>> +   phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
>> 
>> PFN_PHYS() as right now you still have an issue on 32-bit.
> 
> Why? PFN_PHYS is defined as:
> 
> #define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)

Well, that's why I suggested its use (just like you did in your
own review). IOW - now I'm confused.

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Juergen Gross]

On 19/12/17 09:23, Jan Beulich wrote:
 On 18.12.17 at 23:22,  wrote:
>> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
>> +{
>> +struct xen_memory_map memmap;
>> +int rc;
>> +unsigned int i, last_guest_ram;
>> +phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
> 
> PFN_PHYS() as right now you still have an issue on 32-bit.

Why? PFN_PHYS is defined as:

#define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)


Juergen

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Juergen Gross]

On 19/12/17 09:23, Jan Beulich wrote:
 On 18.12.17 at 23:22,  wrote:
>> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
>> +{
>> +struct xen_memory_map memmap;
>> +int rc;
>> +unsigned int i, last_guest_ram;
>> +phys_addr_t max_addr = max_pfn << PAGE_SHIFT;
> 
> PFN_PHYS() as right now you still have an issue on 32-bit.

Why? PFN_PHYS is defined as:

#define PFN_PHYS(x) ((phys_addr_t)(x) << PAGE_SHIFT)


Juergen

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 18.12.17 at 23:22,  wrote:
> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
> +{
> + struct xen_memory_map memmap;
> + int rc;
> + unsigned int i, last_guest_ram;
> + phys_addr_t max_addr = max_pfn << PAGE_SHIFT;

PFN_PHYS() as right now you still have an issue on 32-bit.

> + struct e820_table *xen_e820_table;
> + struct e820_entry *entry;

const?

> + struct resource *res;
> +
> + if (!xen_initial_domain())
> + return;
> +
> + xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);

Wouldn't kmalloc() suffice here?

> + if (!xen_e820_table)
> + return;

Not saying "out of memory" here is certainly fine, but shouldn't
there nevertheless be a warning, as failure to go through the
rest of the function will impact overall functionality?

> + memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);

Is it really reasonable to have a static upper bound here? As we
know especially EFI systems can come with a pretty scattered
(pseudo) E820 table. Even if (iirc) this has a static upper bound
right now in the hypervisor too, it would be nice if the kernel
didn't need further changes once the hypervisor is being made
more flexible.

> + /* Mark non-RAM regions as not available. */
> + for (; i < memmap.nr_entries; i++) {
> + entry = _e820_table->entries[i];
> +
> + if (entry->type == E820_TYPE_RAM)
> + continue;

I can't seem to match up this with ...

> + if (entry->addr >= hostmem_resource->end)
> + break;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + goto out;
> +
> + res->name = "Host memory";

... this. Do you mean != instead (with the comment ahead of the
loop also clarified, saying something like "host RAM regions which
aren't RAM for us")? And perhaps better "Host RAM"?

> + rc = insert_resource(hostmem_resource, res);
> + if (rc) {
> + pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",

[%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
(Same issues somewhere further down.)

> + __func__, res->start, res->end, rc);
> + kfree(res);
> + goto  out;

Perhaps better not to bail out of the loop here (at least if rc is
not -ENOMEM)?

Jan

Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host memory as UNUSABLE

[Jan Beulich]

>>> On 18.12.17 at 23:22,  wrote:
> +void __init arch_xen_balloon_init(struct resource *hostmem_resource)
> +{
> + struct xen_memory_map memmap;
> + int rc;
> + unsigned int i, last_guest_ram;
> + phys_addr_t max_addr = max_pfn << PAGE_SHIFT;

PFN_PHYS() as right now you still have an issue on 32-bit.

> + struct e820_table *xen_e820_table;
> + struct e820_entry *entry;

const?

> + struct resource *res;
> +
> + if (!xen_initial_domain())
> + return;
> +
> + xen_e820_table = kzalloc(sizeof(*xen_e820_table), GFP_KERNEL);

Wouldn't kmalloc() suffice here?

> + if (!xen_e820_table)
> + return;

Not saying "out of memory" here is certainly fine, but shouldn't
there nevertheless be a warning, as failure to go through the
rest of the function will impact overall functionality?

> + memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);

Is it really reasonable to have a static upper bound here? As we
know especially EFI systems can come with a pretty scattered
(pseudo) E820 table. Even if (iirc) this has a static upper bound
right now in the hypervisor too, it would be nice if the kernel
didn't need further changes once the hypervisor is being made
more flexible.

> + /* Mark non-RAM regions as not available. */
> + for (; i < memmap.nr_entries; i++) {
> + entry = _e820_table->entries[i];
> +
> + if (entry->type == E820_TYPE_RAM)
> + continue;

I can't seem to match up this with ...

> + if (entry->addr >= hostmem_resource->end)
> + break;
> +
> + res = kzalloc(sizeof(*res), GFP_KERNEL);
> + if (!res)
> + goto out;
> +
> + res->name = "Host memory";

... this. Do you mean != instead (with the comment ahead of the
loop also clarified, saying something like "host RAM regions which
aren't RAM for us")? And perhaps better "Host RAM"?

> + rc = insert_resource(hostmem_resource, res);
> + if (rc) {
> + pr_warn("%s: Can't insert [%llx - %llx] (%d)\n",

[%llx,%llx) ? Plus won't "ll" cause issues with 32-bit non-PAE builds?
(Same issues somewhere further down.)

> + __func__, res->start, res->end, rc);
> + kfree(res);
> + goto  out;

Perhaps better not to bail out of the loop here (at least if rc is
not -ENOMEM)?

Jan