Re: [Xen-devel] [PATCH] AMD IOMMU: Introduce support for IVHD block type 11h

[Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>]

Hi Jan,

On 05/17/2016 09:25 AM, Jan Beulich wrote:
> > > > On 13.05.16 at 21:54, <suravee.suthikulpanit@xxxxxxx> wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> > [...]
> > @@ -901,7 +911,7 @@ static int __init parse_ivhd_block(const struct 
> > acpi_ivrs_hardware *ivhd_block)
> >                   ivhd_block->header.length, block_length, iommu);
> >               break;
> >           default:
> > -            AMD_IOMMU_DEBUG("IVHD Error: Invalid Device Type!\n");
> > +            AMD_IOMMU_DEBUG("IVHD Error: %s: Invalid Device Type!\n", 
> > __func__);
> Why?
There are some duplicated error message (in get_last_bdf_ivhd() and 
parse_ivhd_block(). So, I just want to differentiate them a bit. But 
this is not a big deal. I can just get rid of this change.
> > [...]
> > +        {
> > +            AMD_IOMMU_DEBUG("IVRS Block: Found type %#x flags %#x len %#x id 
> > %#x\n",
> > +                            ivrs_block->type, ivrs_block->flags,
> > +                            ivrs_block->length, ivrs_block->device_id);
> > +            if ( ivrs_block->type > IVHD_HIGHEST_SUPPORT_TYPE )
> > +                break;
> Is there a requirement for the table elements to appear in numerical
> order?
That is not in the spec. Although it seems to the convention.

> And anyway - this if() appears to be redundant with the
> enclosing one.
I am not sure what you mean by this comment. Could you please elaborate?

> > +int __init amd_iommu_get_supported_ivhd_type(void)
> > +{
> > +    if ( unlikely(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_MSI) )
> > +        return -EPERM;
> This check appears out of the blue, and isn't being mentioned in
> the description. Best would probably be to split it out, but at the
> very least it needs to be (briefly) explained.
This logic was actually duplicated from the 
amd_iommu_update_ivrs_mapping_acpi(). I believe this was added by the
    commit 992fdf6f46252a459c6b1b8d971b2c71f01460f8
        honor ACPI v4 FADT flags

It might make more sense to pull this out to just check once in the 
amd_iommu_init() along with adding some explanation.
> > @@ -1233,8 +1234,11 @@ int __init amd_iommu_init(void)
> >            amd_sp5100_erratum28() )
> >           goto error_out;
> >
> > -    ivrs_bdf_entries = amd_iommu_get_ivrs_dev_entries();
> > +    ivhd_type = amd_iommu_get_supported_ivhd_type();
> > +    if ( !ivhd_type )
> > +        goto error_out;
> Is the ! here meant to catch errors? The function returns -E...
> values to indicate errors ...
Sorry, my bad. I'll fix this.

Thanks,
Suravee

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel