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

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

On Mon, May 06, 2024 at 01:01:48PM +0200, Jan Beulich wrote:
> On 06.05.2024 12:29, Roger Pau Monné wrote:
> > On Thu, Feb 15, 2024 at 11:14:31AM +0100, Jan Beulich wrote:
> >> This is a prereq to us, in particular, respecting the "ATC required"
> >> flag.
> >>
> >> Note that ACPI_SATC_ATC_REQUIRED has its #define put in dmar.h, as we
> >> try to keep actbl*.h in sync what Linux (who in turn inherit from ACPI
> >> CA) has.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I think however it should be mentioned in the description that
introduced code attempts to stay in sync with the existing
register_one_satc and acpi_parse_one_*() functions.

> >> ---
> >> Lovely: On the SPR system with the SATC I tried passing "ats" (the
> >> "required" flag is clear there), just to then hit "IOMMU#4: QI dev wait
> >> descriptor taking too long" while setting up Dom0. The 2nd message there
> >> doesn't ever appear, so the request never completes. Not sure whether
> >> that's us doing something wrong or the hardware acting up. In the former
> >> case I'd generally expect an IOMMU fault to be raised, though. FTR same
> >> on 4.18 with just "VT-d: correct ATS checking for root complex
> >> integrated devices" backported there.
> > 
> > Great, so we likely have a bug in our ATS implementation?
> 
> Or there's a hardware / firmware issue. As said in the remark, while I'm
> not really sure which one it is, I'd kind of expect some form of error
> indication rather than just a hang if we did something wrong.
> 
> >> --- a/xen/drivers/passthrough/vtd/dmar.c
> >> +++ b/xen/drivers/passthrough/vtd/dmar.c
> >> @@ -47,6 +47,7 @@ LIST_HEAD_READ_MOSTLY(acpi_drhd_units);
> >>  LIST_HEAD_READ_MOSTLY(acpi_rmrr_units);
> >>  static LIST_HEAD_READ_MOSTLY(acpi_atsr_units);
> >>  static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
> >> +static LIST_HEAD_READ_MOSTLY(acpi_satc_units);
> > 
> > We could even make this one RO after init.
> 
> Maybe, after first introducing LIST_HEAD_RO_AFTER_INIT() and then
> perhaps switching the others up front. IOW I'd prefer to keep those
> consistent and then (if so desired) update them all in one go.
> 
> >> @@ -750,6 +751,93 @@ acpi_parse_one_rhsa(struct acpi_dmar_hea
> >>      return ret;
> >>  }
> >>  
> >> +static int __init register_one_satc(struct acpi_satc_unit *satcu)
> >> +{
> >> +    bool ignore = false;
> >> +    unsigned int i = 0;
> >> +    int ret = 0;
> >> +
> >> +    /* Skip checking if segment is not accessible yet. */
> >> +    if ( !pci_known_segment(satcu->segment) )
> >> +        i = UINT_MAX;
> >> +
> >> +    for ( ; i < satcu->scope.devices_cnt; i++ )
> >> +    {
> >> +        uint8_t b = PCI_BUS(satcu->scope.devices[i]);
> >> +        uint8_t d = PCI_SLOT(satcu->scope.devices[i]);
> >> +        uint8_t f = PCI_FUNC(satcu->scope.devices[i]);
> >> +
> >> +        if ( !pci_device_detect(satcu->segment, b, d, f) )
> >> +        {
> >> +            dprintk(XENLOG_WARNING VTDPREFIX,
> >> +                    " Non-existent device (%pp) is reported in SATC 
> >> scope!\n",
> >> +                    &PCI_SBDF(satcu->segment, b, d, f));
> >> +            ignore = true;
> >> +        }
> >> +        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",
> >> +                satcu->segment);
> > 
> > Re the error messages: won't it be better to print them using plain
> > printk and gate on iommu_verbose being enabled if anything?
> > 
> > It does seem a bit odd that such messages won't be printed when
> > iommu={debug,verbose} is enabled on the command line.
> 
> Well, perhaps yes. Yet I'm trying here to stay (largely) in sync with how
> in particular register_one_rmrr() behaves. Do you strictly think I should
> diverge here?
> 
> >> +static int __init
> >> +acpi_parse_one_satc(const struct acpi_dmar_header *header)
> >> +{
> >> +    const struct acpi_dmar_satc *satc =
> >> +        container_of(header, const struct acpi_dmar_satc, header);
> >> +    struct acpi_satc_unit *satcu;
> >> +    const void *dev_scope_start, *dev_scope_end;
> >> +    int ret = acpi_dmar_check_length(header, sizeof(*satc));
> >> +
> >> +    if ( ret )
> >> +        return ret;
> >> +
> >> +    satcu = xzalloc(struct acpi_satc_unit);
> >> +    if ( !satcu )
> >> +        return -ENOMEM;
> >> +
> >> +    satcu->segment = satc->segment;
> >> +    satcu->atc_required = satc->flags & ACPI_SATC_ATC_REQUIRED;
> >> +
> >> +    dev_scope_start = (const void *)(satc + 1);
> >> +    dev_scope_end   = (const void *)satc + header->length;
> > 
> > Isn't it enough to just cast to void * and inherit the const from the
> > left side variable declaration?
> 
> Misra won't like the (transient) removal of const, afaict. Personally I
> also consider it bad practice to omit such const.
> 
> > You could even initialize dev_scope_{start,end} at definition.
> 
> Right. This is again the way it is to be in sync with other
> acpi_parse_one_...() functions. It's always hard to judge where to diverge
> and where consistency is weighed higher. Whichever way you do it, you may
> get comment asking for the opposite ...

Oh, yes, IIRC you already mentioned this in v1, yet I've forgot when
reviewing this one.

Thanks, Roger.