Re: [PATCH 1/7] VT-d: parse ACPI "SoC Integrated Address Translation Cache Reporting Structure"s

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 12, 2024 at 10:32:00AM +0100, Jan Beulich wrote:
> On 09.02.2024 10:00, Roger Pau Monné wrote:
> > On Thu, Feb 08, 2024 at 04:29:34PM +0100, Jan Beulich wrote:
> >> On 08.02.2024 10:17, Roger Pau Monné wrote:
> >>> On Mon, Feb 05, 2024 at 02:55:17PM +0100, Jan Beulich wrote:
> >>>> +        {
> >>>> +            dprintk(XENLOG_WARNING VTDPREFIX,
> >>>> +                    " Non-existent device (%pp) is reported in SATC 
> >>>> scope!\n",
> >>>> +                    &PCI_SBDF(satcu->segment, b, d, f));
> >>>> +            ignore = true;
> >>>
> >>> This is kind of reporting is incomplete: as soon as one device is
> >>> found the loop is exited and no further detection happens.  If we want
> >>> to print such information, we should do the full scan and avoid
> >>> exiting early when a populated device is detected.
> >>
> >> Not sure I follow, but first of all - these are dprintk()s only, so
> >> meant to only help in dev environments. Specifically ...
> >>
> >>>> +        }
> >>>> +        else
> >>>> +        {
> >>>> +            ignore = false;
> >>>> +            break;
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    if ( ignore )
> >>>> +    {
> >>>> +        dprintk(XENLOG_WARNING VTDPREFIX,
> >>>> +                " Ignore SATC for seg %04x as no device under its scope 
> >>>> is PCI discoverable!\n",
> >>
> >> ... this message is then issued only bogus entries were found. IOW
> >> when a real device was found, there's no real reason to report N
> >> other bogus ones, I think.
> > 
> > I guess it's a question of taste.  I do find it odd (asymmetric
> > maybe?) that we stop reporting non-existing devices once a valid
> > device is found.  Makes me wonder what's the point of reporting them
> > in the first place, if the list of non-existing devices is not
> > complete?
> 
> Since you look to not be taking this into account, let me re-emphasize
> that these are dprintk() only. In the event of an issue, seeing the
> log messages you at least get a hint of one device that poses a
> problem. That may or may not be enough of an indication for figuring
> what's wrong. Making the loop run for longer than necessary when
> especially in a release build there's not going to be any change (but
> the logic would become [slightly] more complex, as after setting
> "ignore" to true we'd need to avoid clearing it back to false) is just
> pointless imo. IOW I view this 1st message as merely a courtesy for
> the case where the 2nd one would end up also being logged.

I will not insist anymore.

> >>>> +    satcu = xzalloc(struct acpi_satc_unit);
> >>>> +    if ( !satcu )
> >>>> +        return -ENOMEM;
> >>>> +
> >>>> +    satcu->segment = satc->segment;
> >>>> +    satcu->atc_required = satc->flags & 1;
> >>>
> >>> I would add this as a define in actbl2.h:
> >>>
> >>> #define ACPI_DMAR_ATC_REQUIRED (1U << 0)
> >>>
> >>> Or some such (maybe just using plain 1 is also fine).
> >>
> >> I intended to do so, but strictly staying in line with what Linux has.
> >> To my surprise they use a literal number and have no #define. Hence I
> >> didn't add any either.
> > 
> > I would prefer the define unless you have strong objections, even if
> > that means diverging from Linux.
> 
> I could probably accept such a #define living in one of dmar.[ch]. I'd
> rather not see it go into actbl2.h.

Fine.  I think the current open coding of 1 in Linux is wrong.  Other
flag fields in DMAR structures have the related defines.

Thanks, Roger.