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

["Roger Pau Monné" <roger.pau@citrix.com>]

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:
> >> This is a prereq to us, in particular, respecting the "ATC required"
> >> flag.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Should we check scope entries for appropriate types? (If so, then also
> >> for e.g. ATSR.)
> > 
> > Hm, I guess we could do so in acpi_parse_dev_scope() since that
> > function already gets passed a 'type' argument.
> 
> Right, I transiently had it there, but then dropped it again for being
> inconsistent with what we have right now. I'll try to remember to add
> another patch.

No strict requirement - but since it's on the spec we might as well
try to honor it.

> >> +        {
> >> +            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?

> Plus, whatever we change here ought to also / first change in
> register_one_rmrr().

I could live with those looking differently, or register_one_rmrr()
can be adjusted later.  Existing examples shouldn't be an argument to
not make new additions better.

But that's only if you agree the suggested changes make the code
better, if you prefer the current implementation then there's no point
in arguing whether we should keep register_one_rmrr() and the newly
introduce function similar enough.

> >> +    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.

> >> +
> >> +    dev_scope_start = (const void *)(satc + 1);
> >> +    dev_scope_end   = (const void *)satc + header->length;
> >> +    ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> >> +                               &satcu->scope, SATC_TYPE, satc->segment);
> >> +
> >> +    if ( !ret && satcu->scope.devices_cnt )
> >> +    {
> >> +        ret = register_one_satc(satcu);
> >> +        /*
> >> +         * register_one_satc() returns greater than 0 when a specified
> >> +         * PCIe device cannot be detected. To prevent VT-d from being
> >> +         * disabled in such cases, reset the return value to 0 here.
> >> +         */
> >> +        if ( ret > 0 )
> >> +            ret = 0;
> >> +    }
> >> +    else
> >> +        xfree(satcu);
> > 
> > You can safely use scope_devices_free() even if acpi_parse_dev_scope()
> > failed, so you could unify the freeing here, instead of doing it in
> > register_one_satc() also.
> 
> Moving that out of acpi_parse_dev_scope() would feel wrong - if a
> function fails, it would better not leave cleanup to its caller(s).

Given that the caller here is the one that did the allocation my
preference would be to also do the cleanup there - register_one_satc()
has no need to know what needs freeing, and allows unifying the
cleanup in a single place.

> > Also why not make register_one_satc() return a boolean instead of 0/1?
> 
> To leave room to also return errors, like register_one_rmrr() does.
> 
> For both of these aspects you raise: I'd really like to avoid these
> sibling functions going too much out of sync.

I could live with those going out-of-sync (or adjusted later), but only
if you think the suggestions are an improvement.

> >> @@ -817,6 +907,11 @@ static int __init cf_check acpi_parse_dm
> >>                  printk(VTDPREFIX "found ACPI_DMAR_RHSA:\n");
> >>              ret = acpi_parse_one_rhsa(entry_header);
> >>              break;
> >> +        case ACPI_DMAR_TYPE_SATC:
> >> +            if ( iommu_verbose )
> >> +                printk(VTDPREFIX "found ACPI_DMAR_SATC:\n");
> >> +            ret = acpi_parse_one_satc(entry_header);
> >> +            break;
> > 
> > I know the surrounding code doesn't use it, but readability would
> > benefit from adding a blank line after the break statement.
> 
> Well, yes, done so. I'm not generally in favor of introducing such
> inconsistencies, but it looks tolerable here. (In cases like this
> I do - and did here - consider this as an option, but in most cases
> I end up valuing uniform look slightly higher.)

 Yeah, overall I think it's an improvement if we go switching those as
 we modify the code for other reasons.

 Thanks, Roger.