hw/i386/cxl: ensure maxram is greater than ram size for calculating cxl range

[PATCH] hw/i386/cxl: ensure maxram is greater than ram size for calculating cxl range

Posted by Ani Sinha 7 months ago

pc_get_device_memory_range() finds the device memory size by calculating the
difference between maxram and ram sizes. This calculation makes sense only when
maxram is greater than the ram size. Make sure we check for that before calling
pc_get_device_memory_range().

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/i386/pc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f72e2c3b35..948c58171c 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -820,10 +820,12 @@ static void pc_get_device_memory_range(PCMachineState *pcms,
 static uint64_t pc_get_cxl_range_start(PCMachineState *pcms)
 {
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    MachineState *ms = MACHINE(pcms);
     hwaddr cxl_base;
     ram_addr_t size;
 
-    if (pcmc->has_reserved_memory) {
+    if (pcmc->has_reserved_memory &&
+        (ms->ram_size < ms->maxram_size)) {
         pc_get_device_memory_range(pcms, &cxl_base, &size);
         cxl_base += size;
     } else {
-- 
2.42.0

Re: [PATCH] hw/i386/cxl: ensure maxram is greater than ram size for calculating cxl range

Posted by Jonathan Cameron via 7 months ago

On Wed, 11 Oct 2023 16:23:35 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> pc_get_device_memory_range() finds the device memory size by calculating the
> difference between maxram and ram sizes. This calculation makes sense only when
> maxram is greater than the ram size. Make sure we check for that before calling
> pc_get_device_memory_range().
> 
> Signed-off-by: Ani Sinha <anisinha@redhat.com>

Whilst this is similar to other cases, I can't remember or quickly work
out if the 'else' path here is appropriate.  Can we add something to the
patch description to talk about that?

For reference it's:

cxl_base = pc_above_4g_end(pcms);

> ---
>  hw/i386/pc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f72e2c3b35..948c58171c 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -820,10 +820,12 @@ static void pc_get_device_memory_range(PCMachineState *pcms,
>  static uint64_t pc_get_cxl_range_start(PCMachineState *pcms)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    MachineState *ms = MACHINE(pcms);
>      hwaddr cxl_base;
>      ram_addr_t size;
>  
> -    if (pcmc->has_reserved_memory) {
> +    if (pcmc->has_reserved_memory &&
> +        (ms->ram_size < ms->maxram_size)) {
>          pc_get_device_memory_range(pcms, &cxl_base, &size);
>          cxl_base += size;
>      } else {

Re: [PATCH] hw/i386/cxl: ensure maxram is greater than ram size for calculating cxl range

Posted by Ani Sinha 7 months ago


> On 11-Oct-2023, at 10:01 PM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> On Wed, 11 Oct 2023 16:23:35 +0530
> Ani Sinha <anisinha@redhat.com> wrote:
> 
>> pc_get_device_memory_range() finds the device memory size by calculating the
>> difference between maxram and ram sizes. This calculation makes sense only when
>> maxram is greater than the ram size. Make sure we check for that before calling
>> pc_get_device_memory_range().
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> 
> Whilst this is similar to other cases, I can't remember or quickly work
> out if the 'else' path here is appropriate.  Can we add something to the
> patch description to talk about that?
> 
> For reference it's:
> 
> cxl_base = pc_above_4g_end(pcms);

Leaving the alignment adjustments aside, the hotplugged memory devices are mapped starting from where the “above_4g” memory ends.
The cxl memory starts after the region reserved for hot plugged memory devices. If there is no hot plugged memory device region (maxmem == mem), the cxl memory can start right after where “above_4g” memory ends.
See also pc_pci_hole64_start() and pc_max_used_gpa().
I did not want to add any alignment adjustments because I was not sure if it would add regression and incompatibility with older machine types. 


> 
>> ---
>> hw/i386/pc.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index f72e2c3b35..948c58171c 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -820,10 +820,12 @@ static void pc_get_device_memory_range(PCMachineState *pcms,
>> static uint64_t pc_get_cxl_range_start(PCMachineState *pcms)
>> {
>>     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    MachineState *ms = MACHINE(pcms);
>>     hwaddr cxl_base;
>>     ram_addr_t size;
>> 
>> -    if (pcmc->has_reserved_memory) {
>> +    if (pcmc->has_reserved_memory &&
>> +        (ms->ram_size < ms->maxram_size)) {
>>         pc_get_device_memory_range(pcms, &cxl_base, &size);
>>         cxl_base += size;
>>     } else {
>

Re: [PATCH] hw/i386/cxl: ensure maxram is greater than ram size for calculating cxl range

Posted by Jonathan Cameron via 7 months ago

On Thu, 12 Oct 2023 10:10:36 +0530
Ani Sinha <anisinha@redhat.com> wrote:

> > On 11-Oct-2023, at 10:01 PM, Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > 
> > On Wed, 11 Oct 2023 16:23:35 +0530
> > Ani Sinha <anisinha@redhat.com> wrote:
> >   
> >> pc_get_device_memory_range() finds the device memory size by calculating the
> >> difference between maxram and ram sizes. This calculation makes sense only when
> >> maxram is greater than the ram size. Make sure we check for that before calling
> >> pc_get_device_memory_range().
> >> 
> >> Signed-off-by: Ani Sinha <anisinha@redhat.com>  
> > 
> > Whilst this is similar to other cases, I can't remember or quickly work
> > out if the 'else' path here is appropriate.  Can we add something to the
> > patch description to talk about that?
> > 
> > For reference it's:
> > 
> > cxl_base = pc_above_4g_end(pcms);  
> 
> Leaving the alignment adjustments aside, the hotplugged memory devices are mapped starting from where the “above_4g” memory ends.
> The cxl memory starts after the region reserved for hot plugged memory devices. If there is no hot plugged memory device region (maxmem == mem), the cxl memory can start right after where “above_4g” memory ends.
> See also pc_pci_hole64_start() and pc_max_used_gpa().
> I did not want to add any alignment adjustments because I was not sure if it would add regression and incompatibility with older machine types. 
> 

Thanks for explanation.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> 
> >   
> >> ---
> >> hw/i386/pc.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index f72e2c3b35..948c58171c 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -820,10 +820,12 @@ static void pc_get_device_memory_range(PCMachineState *pcms,
> >> static uint64_t pc_get_cxl_range_start(PCMachineState *pcms)
> >> {
> >>     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >> +    MachineState *ms = MACHINE(pcms);
> >>     hwaddr cxl_base;
> >>     ram_addr_t size;
> >> 
> >> -    if (pcmc->has_reserved_memory) {
> >> +    if (pcmc->has_reserved_memory &&
> >> +        (ms->ram_size < ms->maxram_size)) {
> >>         pc_get_device_memory_range(pcms, &cxl_base, &size);
> >>         cxl_base += size;
> >>     } else {  
> >   
>