[PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()

Joao Martins posted 10 patches 3 years, 7 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
[PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()
Posted by Joao Martins 3 years, 7 months ago
This in preparation to allow pc_pci_hole64_start() to be called early
in pc_memory_init(), handle CXL memory region end when its underlying
memory region isn't yet initialized.

Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/i386/pc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8655cc3b8894..d6dff71012ab 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -857,6 +857,19 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
                 start = fw->mr.addr + memory_region_size(&fw->mr);
             }
         }
+    } else {
+        hwaddr cxl_size = MiB;
+
+        start = pc_get_cxl_range_start(pcms);
+        if (pcms->cxl_devices_state.fixed_windows) {
+            GList *it;
+
+            start = ROUND_UP(start + cxl_size, 256 * MiB);
+            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
+                CXLFixedWindow *fw = it->data;
+                start += fw->size;
+            }
+        }
     }
 
     return start;
-- 
2.17.2
Re: [PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()
Posted by Igor Mammedov 3 years, 7 months ago
On Fri,  1 Jul 2022 17:10:11 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> This in preparation to allow pc_pci_hole64_start() to be called early
> in pc_memory_init(), handle CXL memory region end when its underlying
> memory region isn't yet initialized.
> 
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/i386/pc.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 8655cc3b8894..d6dff71012ab 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -857,6 +857,19 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>                  start = fw->mr.addr + memory_region_size(&fw->mr);
>              }
>          }
> +    } else {


> +        hwaddr cxl_size = MiB;
> +
> +        start = pc_get_cxl_range_start(pcms);
> +        if (pcms->cxl_devices_state.fixed_windows) {
> +            GList *it;
> +
> +            start = ROUND_UP(start + cxl_size, 256 * MiB);
> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
> +                CXLFixedWindow *fw = it->data;
> +                start += fw->size;
> +            }
> +        }

/me wondering if this can replace block above that supposedly does
the same only using initialized cxl memory regions?

>      }
>  
>      return start;
Re: [PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()
Posted by Joao Martins 3 years, 7 months ago

On 7/7/22 14:05, Igor Mammedov wrote:
> On Fri,  1 Jul 2022 17:10:11 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> This in preparation to allow pc_pci_hole64_start() to be called early
>> in pc_memory_init(), handle CXL memory region end when its underlying
>> memory region isn't yet initialized.
>>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  hw/i386/pc.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 8655cc3b8894..d6dff71012ab 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -857,6 +857,19 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>>                  start = fw->mr.addr + memory_region_size(&fw->mr);
>>              }
>>          }
>> +    } else {
> 
> 
>> +        hwaddr cxl_size = MiB;
>> +
>> +        start = pc_get_cxl_range_start(pcms);
>> +        if (pcms->cxl_devices_state.fixed_windows) {
>> +            GList *it;
>> +
>> +            start = ROUND_UP(start + cxl_size, 256 * MiB);
>> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
>> +                CXLFixedWindow *fw = it->data;
>> +                start += fw->size;
>> +            }
>> +        }
> 
> /me wondering if this can replace block above that supposedly does
> the same only using initialized cxl memory regions?
> 

I was thinking about the same thing as of writing.

If the calculation returns the same values might as well just replace it
as opposed to branching out similar logic.

I can do that in v7.
Re: [PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()
Posted by Igor Mammedov 3 years, 7 months ago
On Thu, 7 Jul 2022 16:21:07 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> On 7/7/22 14:05, Igor Mammedov wrote:
> > On Fri,  1 Jul 2022 17:10:11 +0100
> > Joao Martins <joao.m.martins@oracle.com> wrote:
> >   
> >> This in preparation to allow pc_pci_hole64_start() to be called early
> >> in pc_memory_init(), handle CXL memory region end when its underlying
> >> memory region isn't yet initialized.
> >>
> >> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >> ---
> >>  hw/i386/pc.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 8655cc3b8894..d6dff71012ab 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -857,6 +857,19 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
> >>                  start = fw->mr.addr + memory_region_size(&fw->mr);
> >>              }
> >>          }
> >> +    } else {  
> > 
> >   
> >> +        hwaddr cxl_size = MiB;
> >> +
> >> +        start = pc_get_cxl_range_start(pcms);
> >> +        if (pcms->cxl_devices_state.fixed_windows) {
> >> +            GList *it;
> >> +
> >> +            start = ROUND_UP(start + cxl_size, 256 * MiB);
> >> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
> >> +                CXLFixedWindow *fw = it->data;
> >> +                start += fw->size;
> >> +            }
> >> +        }  
> > 
> > /me wondering if this can replace block above that supposedly does
> > the same only using initialized cxl memory regions?
> >   
> 
> I was thinking about the same thing as of writing.
> 
> If the calculation returns the same values might as well just replace it
> as opposed to branching out similar logic.

Let's drop not needed code, so reader won't have to wonder why
the same thing is done in 2 different ways.

> 
> I can do that in v7.
>
Re: [PATCH v6 07/10] i386/pc: handle unitialized mr in pc_get_cxl_range_end()
Posted by Joao Martins 3 years, 7 months ago
On 7/11/22 13:58, Igor Mammedov wrote:
> On Thu, 7 Jul 2022 16:21:07 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> On 7/7/22 14:05, Igor Mammedov wrote:
>>> On Fri,  1 Jul 2022 17:10:11 +0100
>>> Joao Martins <joao.m.martins@oracle.com> wrote:
>>>   
>>>> This in preparation to allow pc_pci_hole64_start() to be called early
>>>> in pc_memory_init(), handle CXL memory region end when its underlying
>>>> memory region isn't yet initialized.
>>>>
>>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>  hw/i386/pc.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 8655cc3b8894..d6dff71012ab 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -857,6 +857,19 @@ static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>>>>                  start = fw->mr.addr + memory_region_size(&fw->mr);
>>>>              }
>>>>          }
>>>> +    } else {  
>>>
>>>   
>>>> +        hwaddr cxl_size = MiB;
>>>> +
>>>> +        start = pc_get_cxl_range_start(pcms);
>>>> +        if (pcms->cxl_devices_state.fixed_windows) {
>>>> +            GList *it;
>>>> +
>>>> +            start = ROUND_UP(start + cxl_size, 256 * MiB);
>>>> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
>>>> +                CXLFixedWindow *fw = it->data;
>>>> +                start += fw->size;
>>>> +            }
>>>> +        }  
>>>
>>> /me wondering if this can replace block above that supposedly does
>>> the same only using initialized cxl memory regions?
>>>   
>>
>> I was thinking about the same thing as of writing.
>>
>> If the calculation returns the same values might as well just replace it
>> as opposed to branching out similar logic.
> 
> Let's drop not needed code, so reader won't have to wonder why
> the same thing is done in 2 different ways.
> 
/me nods.

I've removed the old code in this patch and replace with the latter block for v7.