Re: [PATCH v4 1/4] ACPICA: actbl2.h: Add bit constants of the Flags field in the IOVT IOMMU structure

[Bibo Mao <maobibo@loongson.cn>]

On 2026/11/7 下午8:25, Huacai Chen wrote:
> On Sat, Nov 7, 2026 at 9:43 AM Bibo Mao <maobibo@loongson.cn> wrote:
>>
>>
>>
>> On 2026/11/6 下午10:58, Huacai Chen wrote:
>>> On Thu, Nov 5, 2026 at 7:22 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>
>>>>
>>>>
>>>> On 2026/11/5 下午6:31, Huacai Chen wrote:
>>>>> On Thu, Nov 5, 2026 at 5:49 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2026/11/5 下午5:31, Huacai Chen wrote:
>>>>>>> On Thu, Nov 5, 2026 at 5:14 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2026/10/23 下午3:59, Huacai Chen wrote:
>>>>>>>>> Bibo,
>>>>>>>>>
>>>>>>>>> Now I have applied this series but with some modifications, please
>>>>>>>>> double check and test the code.
>>>>>>>>>
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/chenhuacai/linux-loongson.git/log/?h=loongarch-next
>>>>>>>> In my v4 submit, it is:
>>>>>>>> +               case ACPI_IOVT_DEVICE_ENTRY_START:
>>>>>>>> +                       is_start = true;
>>>>>>>> +                       start_devid = ep->device_id;
>>>>>>>> +                       break;
>>>>>>>> however in rc1, it is modified as:
>>>>>>>>                      case ACPI_IOVT_DEVICE_ENTRY_START:
>>>>>>>>                              is_start = true;
>>>>>>>>                              entry->start_devid = ep->device_id;
>>>>>>>>                              break;
>>>>>>>> Here entry is not allocated and NULL pointer, it is allocated in
>>>>>>>> ACPI_IOVT_DEVICE_ENTRY_END switch case.
>>>>>>>>
>>>>>>>> I really do not know what is aid to modify it and let others to test.
>>>>>>> Sorry, it's my fault.
>>>>>>>
>>>>>>> In theory I should point out the modification (eliminating the local
>>>>>>> variable start_devid) and refresh a new version. But we have no time
>>>>>>> to do this because when the first patch is merged into the ACPI repo
>>>>>>> it is near the merge window.
>>>>>> Maybe our work style is different, I do not care much for the coding style.
>>>>>>>
>>>>>>> If you agree, I will submit such modifications to fix it.
>>>>>> fix is welcome.
>>>>>>>
>>>>>>> diff --git a/drivers/acpi/loongarch/iovt.c b/drivers/acpi/loongarch/iovt.c
>>>>>>> index 87cee4f0fc80..4cdac28a1fd0 100644
>>>>>>> --- a/drivers/acpi/loongarch/iovt.c
>>>>>>> +++ b/drivers/acpi/loongarch/iovt.c
>>>>>>> @@ -115,6 +115,10 @@ static int __init iovt_add_iommu(struct
>>>>>>> acpi_iovt_iommu *iommu)
>>>>>>>             for (i = 0; i < iommu->device_entry_num; i++) {
>>>>>>>                     switch (ep->type) {
>>>>>>>                     case ACPI_IOVT_DEVICE_ENTRY_START:
>>>>>>> +                       entry = kzalloc_obj(struct iovt_device_entry,
>>>>>>> GFP_ATOMIC);
>>>>>> There will be memory leak problem if there are START/START/END in the table.
>>>>> That means a broken table? I think we don't need to consider broken
>>>>> tables, because there may be kinds of broken pattern rather than just
>>>>> a START/START/END.
>>>> yes, START/END/START, START/START/END, START/SINGLE/END. what is the
>>>> relationship between "don't need to consider" and kinds of broken pattern?
>>>>
>>>> what is the policy from your side with such BIOS table? let memory leak
>>>> and continue?
>>> The driver can only work with a good table or no table, but not with a
>>> broken table.
>> With a broken table, software can discard the whole table, or discard
>> the single table entry.
> Discard a single entry is dangerous because the pairing may be wrong,
> so if we should handle a broken table, we should discard the whole
> table.
> 
> Discarding the whole table need a rework, the is_start variable is not
> enough. You can consider the below solution:
> 
> diff --git a/drivers/acpi/loongarch/iovt.c b/drivers/acpi/loongarch/iovt.c
> index 87cee4f0fc80..8381de278e3a 100644
> --- a/drivers/acpi/loongarch/iovt.c
> +++ b/drivers/acpi/loongarch/iovt.c
> @@ -73,10 +73,44 @@ static int __init iovt_get_pci_iommu_fwnode(struct
> iovt_fwnode *np, u16 segment,
>          return 0;
>   }
> 
> +static int __init iovt_validate_entries(struct acpi_iovt_iommu *iommu)
> +{
> +       int i, done = 1;
> +       struct acpi_iovt_device_entry *ep;
> +
> +       ep = ACPI_ADD_PTR(struct acpi_iovt_device_entry, iommu,
> iommu->device_entry_offset);
> +       for (i = 0; i < iommu->device_entry_num; i++) {
> +               switch (ep->type) {
> +               case ACPI_IOVT_DEVICE_ENTRY_START:
> +                       if (!done)
> +                               return -EINVAL;
> +
> +                       done = 0;
> +                       break;
> +               case ACPI_IOVT_DEVICE_ENTRY_END:
> +                       if (done)
> +                               return -EINVAL;
> +
> +                       done = 1;
> +                       break;
> +               case ACPI_IOVT_DEVICE_ENTRY_SINGLE:
> +                       if (!done)
> +                               return -EINVAL;
> +
> +                       done = 1;
> +                       break;
> +               default:
one small bit about default case, which is reserved for future use. I 
prefer to skip this type rather than treat it as error,such as:
  +               case ACPI_IOVT_DEVICE_ENTRY_SINGLE:
  +               default:
  +                       if (!done)
  +                               return -EINVAL;
  +
  +                       done = 1;
  +                       break;

Because if iovt table spec is updated and new type is added, the old 
kernel will skip the new type and continue to run.

Regards
Bibo Mao
> +                       return -EINVAL;
> +               }
> +               ep = ACPI_ADD_PTR(struct acpi_iovt_device_entry, ep,
> ep->length);
> +       }
> +
> +       return done ? 0 : -EINVAL;
> +}
> +
>   static int __init iovt_add_iommu(struct acpi_iovt_iommu *iommu)
>   {
>          int i, ret;
> -       bool is_start = false;
>          struct iovt_fwnode *np;
>          struct iovt_device_entry *entry;
>          struct acpi_iovt_device_entry *ep;
> @@ -110,23 +144,22 @@ static int __init iovt_add_iommu(struct
> acpi_iovt_iommu *iommu)
>          if (np->flag & ACPI_IOVT_IOMMU_MAGAGE_BY_SEGMENT)
>                  goto skip;
> 
> +       if (iovt_validate_entries(iommu) < 0)
> +               return -EINVAL;
> +
>          INIT_LIST_HEAD(&np->ep_list);
>          ep = ACPI_ADD_PTR(struct acpi_iovt_device_entry, iommu,
> iommu->device_entry_offset);
>          for (i = 0; i < iommu->device_entry_num; i++) {
>                  switch (ep->type) {
>                  case ACPI_IOVT_DEVICE_ENTRY_START:
> -                       is_start = true;
> -                       entry->start_devid = ep->device_id;
> -                       break;
> -               case ACPI_IOVT_DEVICE_ENTRY_END:
> -                       if (!is_start)
> -                               break;
> -
>                          entry = kzalloc_obj(struct iovt_device_entry,
> GFP_ATOMIC);
>                          if (!entry)
>                                  return -ENOMEM;
> 
> -                       is_start = false;
> +                       entry->start_devid = ep->device_id;
> +                       break;
> +               case ACPI_IOVT_DEVICE_ENTRY_END:
> +                       /* START & END are paired */
>                          entry->end_devid = ep->device_id;
>                          list_add_tail(&entry->list, &np->ep_list);
>                          break;
> @@ -135,13 +168,12 @@ static int __init iovt_add_iommu(struct
> acpi_iovt_iommu *iommu)
>                          if (!entry)
>                                  return -ENOMEM;
> 
> -                       is_start = false;
>                          entry->start_devid = ep->device_id;
>                          entry->end_devid = ep->device_id;
>                          list_add_tail(&entry->list, &np->ep_list);
>                          break;
>                  default:
> -                       break;
> +                       return -EINVAL;
>                  }
>                  ep = ACPI_ADD_PTR(struct acpi_iovt_device_entry, ep,
> ep->length);
>          }
> 
>>>
>>> Take START1/START2/END as an example of broken table: in your original
>>> code, the END entry will be pairing with START2. But for a broken
>>> table who can ensure they are the correct pair? If the real correct
>>> pair is START1 and END, then (entry->start_devid, entry->end_devid)
>> there is only one policy, software does not know START1/END is right or
>> START2/END is right.
>>> will be a wrong device range. Such mistakes are much worse than a
>>> memery leak.
>>>
>>> If you want to say "I don't care about correct pairing, memory leak is
>>> bad in any case.", OK, we can do it like this:
>>>
>>> diff --git a/drivers/acpi/loongarch/iovt.c b/drivers/acpi/loongarch/iovt.c
>>> index 87cee4f0fc80..ebd11e22c9be 100644
>>> --- a/drivers/acpi/loongarch/iovt.c
>>> +++ b/drivers/acpi/loongarch/iovt.c
>>> @@ -115,6 +115,13 @@ static int __init iovt_add_iommu(struct
>>> acpi_iovt_iommu *iommu)
>>>           for (i = 0; i < iommu->device_entry_num; i++) {
>>>                   switch (ep->type) {
>>>                   case ACPI_IOVT_DEVICE_ENTRY_START:
>>> +                       if (is_start)
>>> +                               break;
>> there is still memory leak with START/SINGLE/END pairs, is_start set
>> with false with ACPI_IOVT_DEVICE_ENTRY_SINGLE case.
>>
>> Regards
>> Bibo Mao
>>> +
>>> +                       entry = kzalloc_obj(struct iovt_device_entry,
>>> GFP_ATOMIC);
>>> +                       if (!entry)
>>> +                               return -ENOMEM;
>>> +
>>>                           is_start = true;
>>>                           entry->start_devid = ep->device_id;
>>>                           break;
>>> @@ -122,10 +129,6 @@ static int __init iovt_add_iommu(struct
>>> acpi_iovt_iommu *iommu)
>>>                           if (!is_start)
>>>                                   break;
>>>
>>> -                       entry = kzalloc_obj(struct iovt_device_entry,
>>> GFP_ATOMIC);
>>> -                       if (!entry)
>>> -                               return -ENOMEM;
>>> -
>>>                           is_start = false;
>>>                           entry->end_devid = ep->device_id;
>>>                           list_add_tail(&entry->list, &np->ep_list);
>>>
>>> Huacai
>>>
>>>>
>>>>>
>>>>> Huacai
>>>>>
>>>>>>
>>>>>> Regards
>>>>>> Bibo Mao
>>>>>>> +                       if (!entry)
>>>>>>> +                               return -ENOMEM;
>>>>>>> +
>>>>>>>                             is_start = true;
>>>>>>>                             entry->start_devid = ep->device_id;
>>>>>>>                             break;
>>>>>>> @@ -122,10 +126,6 @@ static int __init iovt_add_iommu(struct
>>>>>>> acpi_iovt_iommu *iommu)
>>>>>>>                             if (!is_start)
>>>>>>>                                     break;
>>>>>>>
>>>>>>> -                       entry = kzalloc_obj(struct iovt_device_entry,
>>>>>>> GFP_ATOMIC);
>>>>>>> -                       if (!entry)
>>>>>>> -                               return -ENOMEM;
>>>>>>> -
>>>>>>>                             is_start = false;
>>>>>>>                             entry->end_devid = ep->device_id;
>>>>>>>                             list_add_tail(&entry->list, &np->ep_list);
>>>>>>>
>>>>>>> Huacai
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Huacai
>>>>>>>>>
>>>>>>>>> On Mon, Oct 12, 2026 at 3:16 PM Bibo Mao <maobibo@loongson.cn> wrote:
>>>>>>>>>>
>>>>>>>>>> This change is already merged in acpica tree.
>>>>>>>>>>
>>>>>>>>>> slightly ping.
>>>>>>>>>>
>>>>>>>>>> On Tue, Sep 22, 2026 at 08:10:55PM +0800, Bibo Mao wrote:
>>>>>>>>>>       > According to the IOVT specification, define macros for each bit of the
>>>>>>>>>>       > Flags field in the IOVT IOMMU structure.
>>>>>>>>>>       >
>>>>>>>>>>       > Link: https://github.com/open-acpica/acpica/commit/352f750e4cc0
>>>>>>>>>>       > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>>>>>>>>>>       > ---
>>>>>>>>>>       >  include/acpi/actbl2.h | 7 +++++++
>>>>>>>>>>       >  1 file changed, 7 insertions(+)
>>>>>>>>>>       >
>>>>>>>>>>       > diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
>>>>>>>>>>       > index baef525367b5..76e4a5bc79b4 100644
>>>>>>>>>>       > --- a/include/acpi/actbl2.h
>>>>>>>>>>       > +++ b/include/acpi/actbl2.h
>>>>>>>>>>       > @@ -919,6 +919,13 @@ struct acpi_iovt_iommu {
>>>>>>>>>>       >       u32 device_entry_offset;
>>>>>>>>>>       >  };
>>>>>>>>>>       >
>>>>>>>>>>       > +/* Masks for Flags field above */
>>>>>>>>>>       > +#define ACPI_IOVT_IOMMU_PCI_DEVICE           (1<<0)
>>>>>>>>>>       > +#define ACPI_IOVT_IOMMU_PXM_VALID            (1<<1)
>>>>>>>>>>       > +#define ACPI_IOVT_IOMMU_MAGAGE_BY_SEGMENT    (1<<2)
>>>>>>>>>>       > +#define ACPI_IOVT_IOMMU_HW_CAP_SUPPORT               (1<<3)
>>>>>>>>>>       > +#define ACPI_IOVT_IOMMU_MSI_INT_BYPASS               (1<<4)
>>>>>>>>>>       > +
>>>>>>>>>>       >  struct acpi_iovt_device_entry {
>>>>>>>>>>       >       u8 type;
>>>>>>>>>>       >       u8 length;
>>>>>>>>>>       > --
>>>>>>>>>>       > 2.39.3
>>>>>>>>>>       >
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>