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

[Huacai Chen]

On Sat, Nov 7, 2026 at 9:43 AM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
>
>
>
> On 2026/11/6 下午10:58, Huacai Chen wrote:
> > On Thu, Nov 5, 2026 at 7:22 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 2026/11/5 下午6:31, Huacai Chen wrote:
> >>> On Thu, Nov 5, 2026 at 5:49 PM Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2026/11/5 下午5:31, Huacai Chen wrote:
> >>>>> On Thu, Nov 5, 2026 at 5:14 PM Bibo Mao <maobibo@xxxxxxxxxxx> 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:
+                       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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>>>>>>      > ---
> >>>>>>>>      >  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
> >>>>>>>>      >
> >>>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> >>
>