[PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Thierry Escande]

With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
buffers), the driver mpt3sas fails to load because it cannot allocate
its DMA pool for an allocation size of ~2,3 MBytes. This is because the
alignement check added by 9f40ec84a797 fails and
xen_swiotlb_alloc_coherent() ends up calling
xen_create_contiguous_region() with a size order of 10 which is too high
for the current max value.

This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
such allocations.

Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
---
 arch/x86/xen/mmu_pv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 55a4996d0c04..7f110740e1a2 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
 }
 
 /* Protected by xen_reservation_lock. */
-#define MAX_CONTIG_ORDER 9 /* 2MB */
+#define MAX_CONTIG_ORDER 10 /* 4MB */
 static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
 
 #define VOID_PTE (mfn_pte(0, __pgprot(0)))
-- 
2.45.2

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Jan Beulich]

On 04.12.2024 18:14, Thierry Escande wrote:
> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
> buffers), the driver mpt3sas fails to load because it cannot allocate
> its DMA pool for an allocation size of ~2,3 MBytes. This is because the
> alignement check added by 9f40ec84a797 fails and
> xen_swiotlb_alloc_coherent() ends up calling
> xen_create_contiguous_region() with a size order of 10 which is too high
> for the current max value.
> 
> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
> such allocations.
> 
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  arch/x86/xen/mmu_pv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 55a4996d0c04..7f110740e1a2 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>  }
>  
>  /* Protected by xen_reservation_lock. */
> -#define MAX_CONTIG_ORDER 9 /* 2MB */
> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>  static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];

While lacking respective commentary, bumping this value imo also needs to
take into account Xen itself, at least commit-message-wise. The bumping is
fine for Dom0 in any event. It is also fine for DomU-s with the defaults
built into the hypervisor (orders 12 and 10 respectively for x86 and Arm),
yet especially for Arm (and in the future PPC and RISC-V) any further
bumping would be less straightforward.

However - does the driver really need this big a contiguous chunk? It
would seem far more desirable to me to break that up some, if possible.

Jan

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Thierry Escande]

Hi Jan,

On 09/12/2024 11:04, Jan Beulich wrote:
> On 04.12.2024 18:14, Thierry Escande wrote:
>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>> buffers), the driver mpt3sas fails to load because it cannot allocate
>> its DMA pool for an allocation size of ~2,3 MBytes. This is because the
>> alignement check added by 9f40ec84a797 fails and
>> xen_swiotlb_alloc_coherent() ends up calling
>> xen_create_contiguous_region() with a size order of 10 which is too high
>> for the current max value.
>>
>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>> such allocations.
>>
>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>> ---
>>  arch/x86/xen/mmu_pv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>> index 55a4996d0c04..7f110740e1a2 100644
>> --- a/arch/x86/xen/mmu_pv.c
>> +++ b/arch/x86/xen/mmu_pv.c
>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>  }
>>  
>>  /* Protected by xen_reservation_lock. */
>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>  static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
> 
> While lacking respective commentary, bumping this value imo also needs to
> take into account Xen itself, at least commit-message-wise. The bumping is
> fine for Dom0 in any event. It is also fine for DomU-s with the defaults
> built into the hypervisor (orders 12 and 10 respectively for x86 and Arm),
> yet especially for Arm (and in the future PPC and RISC-V) any further
> bumping would be less straightforward.

Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
problem for drivers trying to allocate more than that from a domU ?

> However - does the driver really need this big a contiguous chunk? It
> would seem far more desirable to me to break that up some, if possible.

Since this works on bare metal I'm afraid the driver maintainer (mpt
fusion driver) will just tell me to fix Xen.

Regards,
Thierry

> 
> Jan

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Thierry Escande]

Hi Jan,

On 09/12/2024 11:04, Jan Beulich wrote:
> On 04.12.2024 18:14, Thierry Escande wrote:
>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>> buffers), the driver mpt3sas fails to load because it cannot allocate
>> its DMA pool for an allocation size of ~2,3 MBytes. This is because the
>> alignement check added by 9f40ec84a797 fails and
>> xen_swiotlb_alloc_coherent() ends up calling
>> xen_create_contiguous_region() with a size order of 10 which is too high
>> for the current max value.
>>
>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>> such allocations.
>>
>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>> ---
>>  arch/x86/xen/mmu_pv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>> index 55a4996d0c04..7f110740e1a2 100644
>> --- a/arch/x86/xen/mmu_pv.c
>> +++ b/arch/x86/xen/mmu_pv.c
>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>  }
>>  
>>  /* Protected by xen_reservation_lock. */
>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>  static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
> 
> While lacking respective commentary, bumping this value imo also needs to
> take into account Xen itself, at least commit-message-wise. The bumping is
> fine for Dom0 in any event. It is also fine for DomU-s with the defaults
> built into the hypervisor (orders 12 and 10 respectively for x86 and Arm),
> yet especially for Arm (and in the future PPC and RISC-V) any further
> bumping would be less straightforward.

Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
problem for drivers trying to allocate more than that from a domU ?

> However - does the driver really need this big a contiguous chunk? It
> would seem far more desirable to me to break that up some, if possible.

Since this works on bare metal I'm afraid the driver maintainer (mpt
fusion driver) will just tell me to fix Xen.

Regards,
Thierry

> 
> Jan

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Jan Beulich]

On 11.12.2024 19:20, Thierry Escande wrote:
> Hi Jan,
> 
> On 09/12/2024 11:04, Jan Beulich wrote:
>> On 04.12.2024 18:14, Thierry Escande wrote:
>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because the
>>> alignement check added by 9f40ec84a797 fails and
>>> xen_swiotlb_alloc_coherent() ends up calling
>>> xen_create_contiguous_region() with a size order of 10 which is too high
>>> for the current max value.
>>>
>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>> such allocations.
>>>
>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>> ---
>>>  arch/x86/xen/mmu_pv.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>> index 55a4996d0c04..7f110740e1a2 100644
>>> --- a/arch/x86/xen/mmu_pv.c
>>> +++ b/arch/x86/xen/mmu_pv.c
>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>  }
>>>  
>>>  /* Protected by xen_reservation_lock. */
>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>  static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>
>> While lacking respective commentary, bumping this value imo also needs to
>> take into account Xen itself, at least commit-message-wise. The bumping is
>> fine for Dom0 in any event. It is also fine for DomU-s with the defaults
>> built into the hypervisor (orders 12 and 10 respectively for x86 and Arm),
>> yet especially for Arm (and in the future PPC and RISC-V) any further
>> bumping would be less straightforward.
> 
> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
> problem for drivers trying to allocate more than that from a domU ?

A driver assumes a (physical) device to be in the DomU, at which point it
is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).

>> However - does the driver really need this big a contiguous chunk? It
>> would seem far more desirable to me to break that up some, if possible.
> 
> Since this works on bare metal I'm afraid the driver maintainer (mpt
> fusion driver) will just tell me to fix Xen.

Well. The bigger such allocations, the larger the risk that on systems
that have been up for a while such allocations can't be fulfilled anymore
even in the bare metal case.

Jan

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2957 bytes --]

On 12.12.24 11:22, Jan Beulich wrote:
> On 11.12.2024 19:20, Thierry Escande wrote:
>> Hi Jan,
>>
>> On 09/12/2024 11:04, Jan Beulich wrote:
>>> On 04.12.2024 18:14, Thierry Escande wrote:
>>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because the
>>>> alignement check added by 9f40ec84a797 fails and
>>>> xen_swiotlb_alloc_coherent() ends up calling
>>>> xen_create_contiguous_region() with a size order of 10 which is too high
>>>> for the current max value.
>>>>
>>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>>> such allocations.
>>>>
>>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>>> ---
>>>>   arch/x86/xen/mmu_pv.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>> index 55a4996d0c04..7f110740e1a2 100644
>>>> --- a/arch/x86/xen/mmu_pv.c
>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>>   }
>>>>   
>>>>   /* Protected by xen_reservation_lock. */
>>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>>   static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>>
>>> While lacking respective commentary, bumping this value imo also needs to
>>> take into account Xen itself, at least commit-message-wise. The bumping is
>>> fine for Dom0 in any event. It is also fine for DomU-s with the defaults
>>> built into the hypervisor (orders 12 and 10 respectively for x86 and Arm),
>>> yet especially for Arm (and in the future PPC and RISC-V) any further
>>> bumping would be less straightforward.
>>
>> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
>> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
>> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
>> problem for drivers trying to allocate more than that from a domU ?
> 
> A driver assumes a (physical) device to be in the DomU, at which point it
> is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).
> 
>>> However - does the driver really need this big a contiguous chunk? It
>>> would seem far more desirable to me to break that up some, if possible.
>>
>> Since this works on bare metal I'm afraid the driver maintainer (mpt
>> fusion driver) will just tell me to fix Xen.
> 
> Well. The bigger such allocations, the larger the risk that on systems
> that have been up for a while such allocations can't be fulfilled anymore
> even in the bare metal case.

Yes. I don't think we should just work around this issue without having
even tried to get the driver fixed. In case they refuse to change it, we
can still increase MAX_CONTIG_ORDER.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Thierry Escande]

On 12/12/2024 12:09, Juergen Gross wrote:
> On 12.12.24 11:22, Jan Beulich wrote:
>> On 11.12.2024 19:20, Thierry Escande wrote:
>>> Hi Jan,
>>>
>>> On 09/12/2024 11:04, Jan Beulich wrote:
>>>> On 04.12.2024 18:14, Thierry Escande wrote:
>>>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because
>>>>> the
>>>>> alignement check added by 9f40ec84a797 fails and
>>>>> xen_swiotlb_alloc_coherent() ends up calling
>>>>> xen_create_contiguous_region() with a size order of 10 which is too
>>>>> high
>>>>> for the current max value.
>>>>>
>>>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>>>> such allocations.
>>>>>
>>>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>>>> ---
>>>>>   arch/x86/xen/mmu_pv.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>>> index 55a4996d0c04..7f110740e1a2 100644
>>>>> --- a/arch/x86/xen/mmu_pv.c
>>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>>>   }
>>>>>     /* Protected by xen_reservation_lock. */
>>>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>>>   static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>>>
>>>> While lacking respective commentary, bumping this value imo also
>>>> needs to
>>>> take into account Xen itself, at least commit-message-wise. The
>>>> bumping is
>>>> fine for Dom0 in any event. It is also fine for DomU-s with the
>>>> defaults
>>>> built into the hypervisor (orders 12 and 10 respectively for x86 and
>>>> Arm),
>>>> yet especially for Arm (and in the future PPC and RISC-V) any further
>>>> bumping would be less straightforward.
>>>
>>> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
>>> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
>>> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
>>> problem for drivers trying to allocate more than that from a domU ?
>>
>> A driver assumes a (physical) device to be in the DomU, at which point it
>> is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).
>>
>>>> However - does the driver really need this big a contiguous chunk? It
>>>> would seem far more desirable to me to break that up some, if possible.
>>>
>>> Since this works on bare metal I'm afraid the driver maintainer (mpt
>>> fusion driver) will just tell me to fix Xen.
>>
>> Well. The bigger such allocations, the larger the risk that on systems
>> that have been up for a while such allocations can't be fulfilled anymore
>> even in the bare metal case.
>
> Yes. I don't think we should just work around this issue without having
> even tried to get the driver fixed. In case they refuse to change it, we
> can still increase MAX_CONTIG_ORDER.

Thanks for the feedback. I'll try to have a look at the driver if I have
time to do so.

Regards,
Thierry

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 3645 bytes --]

On 18.12.24 12:11, Thierry Escande wrote:
> 
> 
> On 12/12/2024 12:09, Juergen Gross wrote:
>> On 12.12.24 11:22, Jan Beulich wrote:
>>> On 11.12.2024 19:20, Thierry Escande wrote:
>>>> Hi Jan,
>>>>
>>>> On 09/12/2024 11:04, Jan Beulich wrote:
>>>>> On 04.12.2024 18:14, Thierry Escande wrote:
>>>>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>>>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>>>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because
>>>>>> the
>>>>>> alignement check added by 9f40ec84a797 fails and
>>>>>> xen_swiotlb_alloc_coherent() ends up calling
>>>>>> xen_create_contiguous_region() with a size order of 10 which is too
>>>>>> high
>>>>>> for the current max value.
>>>>>>
>>>>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>>>>> such allocations.
>>>>>>
>>>>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>>>>> ---
>>>>>>    arch/x86/xen/mmu_pv.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>>>> index 55a4996d0c04..7f110740e1a2 100644
>>>>>> --- a/arch/x86/xen/mmu_pv.c
>>>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>>>>    }
>>>>>>      /* Protected by xen_reservation_lock. */
>>>>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>>>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>>>>    static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>>>>
>>>>> While lacking respective commentary, bumping this value imo also
>>>>> needs to
>>>>> take into account Xen itself, at least commit-message-wise. The
>>>>> bumping is
>>>>> fine for Dom0 in any event. It is also fine for DomU-s with the
>>>>> defaults
>>>>> built into the hypervisor (orders 12 and 10 respectively for x86 and
>>>>> Arm),
>>>>> yet especially for Arm (and in the future PPC and RISC-V) any further
>>>>> bumping would be less straightforward.
>>>>
>>>> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
>>>> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
>>>> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
>>>> problem for drivers trying to allocate more than that from a domU ?
>>>
>>> A driver assumes a (physical) device to be in the DomU, at which point it
>>> is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).
>>>
>>>>> However - does the driver really need this big a contiguous chunk? It
>>>>> would seem far more desirable to me to break that up some, if possible.
>>>>
>>>> Since this works on bare metal I'm afraid the driver maintainer (mpt
>>>> fusion driver) will just tell me to fix Xen.
>>>
>>> Well. The bigger such allocations, the larger the risk that on systems
>>> that have been up for a while such allocations can't be fulfilled anymore
>>> even in the bare metal case.
>>
>> Yes. I don't think we should just work around this issue without having
>> even tried to get the driver fixed. In case they refuse to change it, we
>> can still increase MAX_CONTIG_ORDER.
> 
> Thanks for the feedback. I'll try to have a look at the driver if I have
> time to do so.

Another thought would be to change the generic DMA allocation to not require
alignment based on the rounded up size, but on the largest power-of-2 chunk
fitting into the requested size.

I don't see why a 2.3 MB memory allocation would need to be 4 MB aligned. It
should be perfectly fine to align it to 2 MB only.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Jan Beulich]

On 18.12.2024 12:24, Jürgen Groß wrote:
> On 18.12.24 12:11, Thierry Escande wrote:
>>
>>
>> On 12/12/2024 12:09, Juergen Gross wrote:
>>> On 12.12.24 11:22, Jan Beulich wrote:
>>>> On 11.12.2024 19:20, Thierry Escande wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 09/12/2024 11:04, Jan Beulich wrote:
>>>>>> On 04.12.2024 18:14, Thierry Escande wrote:
>>>>>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>>>>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>>>>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because
>>>>>>> the
>>>>>>> alignement check added by 9f40ec84a797 fails and
>>>>>>> xen_swiotlb_alloc_coherent() ends up calling
>>>>>>> xen_create_contiguous_region() with a size order of 10 which is too
>>>>>>> high
>>>>>>> for the current max value.
>>>>>>>
>>>>>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>>>>>> such allocations.
>>>>>>>
>>>>>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>>>>>> ---
>>>>>>>    arch/x86/xen/mmu_pv.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>>>>> index 55a4996d0c04..7f110740e1a2 100644
>>>>>>> --- a/arch/x86/xen/mmu_pv.c
>>>>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>>>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>>>>>    }
>>>>>>>      /* Protected by xen_reservation_lock. */
>>>>>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>>>>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>>>>>    static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>>>>>
>>>>>> While lacking respective commentary, bumping this value imo also
>>>>>> needs to
>>>>>> take into account Xen itself, at least commit-message-wise. The
>>>>>> bumping is
>>>>>> fine for Dom0 in any event. It is also fine for DomU-s with the
>>>>>> defaults
>>>>>> built into the hypervisor (orders 12 and 10 respectively for x86 and
>>>>>> Arm),
>>>>>> yet especially for Arm (and in the future PPC and RISC-V) any further
>>>>>> bumping would be less straightforward.
>>>>>
>>>>> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
>>>>> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
>>>>> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
>>>>> problem for drivers trying to allocate more than that from a domU ?
>>>>
>>>> A driver assumes a (physical) device to be in the DomU, at which point it
>>>> is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).
>>>>
>>>>>> However - does the driver really need this big a contiguous chunk? It
>>>>>> would seem far more desirable to me to break that up some, if possible.
>>>>>
>>>>> Since this works on bare metal I'm afraid the driver maintainer (mpt
>>>>> fusion driver) will just tell me to fix Xen.
>>>>
>>>> Well. The bigger such allocations, the larger the risk that on systems
>>>> that have been up for a while such allocations can't be fulfilled anymore
>>>> even in the bare metal case.
>>>
>>> Yes. I don't think we should just work around this issue without having
>>> even tried to get the driver fixed. In case they refuse to change it, we
>>> can still increase MAX_CONTIG_ORDER.
>>
>> Thanks for the feedback. I'll try to have a look at the driver if I have
>> time to do so.
> 
> Another thought would be to change the generic DMA allocation to not require
> alignment based on the rounded up size, but on the largest power-of-2 chunk
> fitting into the requested size.
> 
> I don't see why a 2.3 MB memory allocation would need to be 4 MB aligned. It
> should be perfectly fine to align it to 2 MB only.

Yet that wouldn't make a difference here, would it? We'd still need a 4M
chunk of contiguous space, just with less alignment.

Jan

Re: [PATCH] x86/xen/mmu: Increase MAX_CONTIG_ORDER

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4203 bytes --]

On 19.12.24 08:12, Jan Beulich wrote:
> On 18.12.2024 12:24, Jürgen Groß wrote:
>> On 18.12.24 12:11, Thierry Escande wrote:
>>>
>>>
>>> On 12/12/2024 12:09, Juergen Gross wrote:
>>>> On 12.12.24 11:22, Jan Beulich wrote:
>>>>> On 11.12.2024 19:20, Thierry Escande wrote:
>>>>>> Hi Jan,
>>>>>>
>>>>>> On 09/12/2024 11:04, Jan Beulich wrote:
>>>>>>> On 04.12.2024 18:14, Thierry Escande wrote:
>>>>>>>> With change 9f40ec84a797 (xen/swiotlb: add alignment check for dma
>>>>>>>> buffers), the driver mpt3sas fails to load because it cannot allocate
>>>>>>>> its DMA pool for an allocation size of ~2,3 MBytes. This is because
>>>>>>>> the
>>>>>>>> alignement check added by 9f40ec84a797 fails and
>>>>>>>> xen_swiotlb_alloc_coherent() ends up calling
>>>>>>>> xen_create_contiguous_region() with a size order of 10 which is too
>>>>>>>> high
>>>>>>>> for the current max value.
>>>>>>>>
>>>>>>>> This patch increases the MAX_CONTIG_ORDER from 9 to 10 (4MB) to allow
>>>>>>>> such allocations.
>>>>>>>>
>>>>>>>> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
>>>>>>>> ---
>>>>>>>>     arch/x86/xen/mmu_pv.c | 2 +-
>>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>>>>>> index 55a4996d0c04..7f110740e1a2 100644
>>>>>>>> --- a/arch/x86/xen/mmu_pv.c
>>>>>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>>>>>> @@ -2200,7 +2200,7 @@ void __init xen_init_mmu_ops(void)
>>>>>>>>     }
>>>>>>>>       /* Protected by xen_reservation_lock. */
>>>>>>>> -#define MAX_CONTIG_ORDER 9 /* 2MB */
>>>>>>>> +#define MAX_CONTIG_ORDER 10 /* 4MB */
>>>>>>>>     static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
>>>>>>>
>>>>>>> While lacking respective commentary, bumping this value imo also
>>>>>>> needs to
>>>>>>> take into account Xen itself, at least commit-message-wise. The
>>>>>>> bumping is
>>>>>>> fine for Dom0 in any event. It is also fine for DomU-s with the
>>>>>>> defaults
>>>>>>> built into the hypervisor (orders 12 and 10 respectively for x86 and
>>>>>>> Arm),
>>>>>>> yet especially for Arm (and in the future PPC and RISC-V) any further
>>>>>>> bumping would be less straightforward.
>>>>>>
>>>>>> Thanks for pointing this out. On the Xen side, CONFIG_CTLDOM_MAX_ORDER
>>>>>> and CONFIG_HWDOM_MAX_ORDER seem big enough on all architectures. But I
>>>>>> see CONFIG_DOMU_MAX_ORDER set to 9 (also all archs). Won't that be a
>>>>>> problem for drivers trying to allocate more than that from a domU ?
>>>>>
>>>>> A driver assumes a (physical) device to be in the DomU, at which point it
>>>>> is CONFIG_PTDOM_MAX_ORDER which applies (PT standing for pass-through).
>>>>>
>>>>>>> However - does the driver really need this big a contiguous chunk? It
>>>>>>> would seem far more desirable to me to break that up some, if possible.
>>>>>>
>>>>>> Since this works on bare metal I'm afraid the driver maintainer (mpt
>>>>>> fusion driver) will just tell me to fix Xen.
>>>>>
>>>>> Well. The bigger such allocations, the larger the risk that on systems
>>>>> that have been up for a while such allocations can't be fulfilled anymore
>>>>> even in the bare metal case.
>>>>
>>>> Yes. I don't think we should just work around this issue without having
>>>> even tried to get the driver fixed. In case they refuse to change it, we
>>>> can still increase MAX_CONTIG_ORDER.
>>>
>>> Thanks for the feedback. I'll try to have a look at the driver if I have
>>> time to do so.
>>
>> Another thought would be to change the generic DMA allocation to not require
>> alignment based on the rounded up size, but on the largest power-of-2 chunk
>> fitting into the requested size.
>>
>> I don't see why a 2.3 MB memory allocation would need to be 4 MB aligned. It
>> should be perfectly fine to align it to 2 MB only.
> 
> Yet that wouldn't make a difference here, would it? We'd still need a 4M
> chunk of contiguous space, just with less alignment.

Thierry stated that the driver failed to load due to the added alignment
check introduced with commit 9f40ec84a797. I was targeting this reasoning
with my remark.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]