Re: [Xen-devel] [PATCH v4 1/2] iommu VT-d: separate rmrr addition function

[Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>]

On Tue, May 05, 2015 at 10:50:56AM +0100, Jan Beulich wrote:
> >>> On 28.04.15 at 01:50, <elena.ufimtseva@xxxxxxxxxx> wrote:
> > From: Elena Ufimtseva <elena.ufimtseva@xxxxxxxxxx>
> > 
> > In preparation for auxiliary RMRR data provided on Xen
> > command line, make RMRR adding a separate function.
> > No functional changes.
> > 
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > 
> > Acked-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> 
> Are these the ones from v1? Did the patch change so little since then
> that they're valid to be retained? And searching for "separate rmrr"
> in the archive I can't even spot a v3. Did you send that out under
> a different title?

Hi Jan

Thank you fore reviewing this.
I thought I did post v3, but as you said I cant find it either.
That means I did not actually send it, even though I had it prepared to
send.
> 
> > --- a/xen/drivers/passthrough/vtd/dmar.c
> > +++ b/xen/drivers/passthrough/vtd/dmar.c
> > @@ -567,6 +567,66 @@ out:
> >      return ret;
> >  }
> >  
> > +static int register_one_rmrr(struct acpi_rmrr_unit *rmrru)
> > +{
> > +    bool_t ignore = 0;
> > +    unsigned int i = 0;
> > +    int ret = 0;
> > +
> > +    /* Skip checking if segment is not accessible yet. */
> > +    if ( !pci_known_segment(rmrru->segment) )
> > +        i = UINT_MAX;
> > +
> > +    for ( ; i < rmrru->scope.devices_cnt; i++ )
> > +    {
> > +        u8 b = PCI_BUS(rmrru->scope.devices[i]);
> > +        u8 d = PCI_SLOT(rmrru->scope.devices[i]);
> > +        u8 f = PCI_FUNC(rmrru->scope.devices[i]);
> > +
> > +        if ( pci_device_detect(rmrru->segment, b, d, f) == 0 )
> > +        {
> > +            dprintk(XENLOG_WARNING VTDPREFIX,
> > +                    " Non-existent device (%04x:%02x:%02x.%u) is reported"
> > +                    " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> > +                    rmrru->segment, b, d, f,
> > +                    rmrru->base_address, rmrru->end_address);
> > +            ignore = 1;
> > +        }
> > +        else
> > +        {
> > +            ignore = 0;
> > +            break;
> > +        }
> > +    }
> > +
> > +    if ( ignore )
> > +    {
> > +        dprintk(XENLOG_WARNING VTDPREFIX,
> > +            "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> > +            "devices under its scope are not PCI discoverable!\n",
> > +            rmrru->base_address, rmrru->end_address);
> > +        ret = -EFAULT;
> 
> This wasn't there in the original code afaics, and adding it alters
> behavior (contrary to what the description claims).

Understood, will change patch description.
> 
> > +    }
> > +    else if ( rmrru->base_address > rmrru->end_address )
> > +    {
> > +        dprintk(XENLOG_WARNING VTDPREFIX,
> > +            "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> > +            rmrru->base_address, rmrru->end_address);
> > +        ret = -EFAULT;
> > +    }
> > +    else
> > +    {
> > +        if ( iommu_verbose )
> > +            dprintk(VTDPREFIX,
> > +                    "  RMRR region: base_addr %"PRIx64
> > +                    " end_address %"PRIx64"\n",
> > +                    rmrru->base_address, rmrru->end_address);
> > +        acpi_register_rmrr_unit(rmrru);
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> >  static int __init
> >  acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> >  {
> > @@ -616,67 +676,10 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> >      dev_scope_end   = ((void *)rmrr) + header->length;
> >      ret = acpi_parse_dev_scope(dev_scope_start, dev_scope_end,
> >                                 &rmrru->scope, RMRR_TYPE, rmrr->segment);
> > -
> > -    if ( ret || (rmrru->scope.devices_cnt == 0) )
> > +    if ( !ret && (rmrru->scope.devices_cnt != 0) )
> > +        ret = register_one_rmrr(rmrru);
> > +    if ( ret )
> >          xfree(rmrru);
> > -    else
> > -    {
> > -        u8 b, d, f;
> > -        bool_t ignore = 0;
> > -        unsigned int i = 0;
> > -
> > -        /* Skip checking if segment is not accessible yet. */
> > -        if ( !pci_known_segment(rmrr->segment) )
> > -            i = UINT_MAX;
> > -
> > -        for ( ; i < rmrru->scope.devices_cnt; i++ )
> > -        {
> > -            b = PCI_BUS(rmrru->scope.devices[i]);
> > -            d = PCI_SLOT(rmrru->scope.devices[i]);
> > -            f = PCI_FUNC(rmrru->scope.devices[i]);
> > -
> > -            if ( pci_device_detect(rmrr->segment, b, d, f) == 0 )
> > -            {
> > -                dprintk(XENLOG_WARNING VTDPREFIX,
> > -                        " Non-existent device (%04x:%02x:%02x.%u) is 
> > reported"
> > -                        " in RMRR (%"PRIx64", %"PRIx64")'s scope!\n",
> > -                        rmrr->segment, b, d, f,
> > -                        rmrru->base_address, rmrru->end_address);
> > -                ignore = 1;
> > -            }
> > -            else
> > -            {
> > -                ignore = 0;
> > -                break;
> > -            }
> > -        }
> > -
> > -        if ( ignore )
> > -        {
> > -            dprintk(XENLOG_WARNING VTDPREFIX,
> > -                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
> > -                "devices under its scope are not PCI discoverable!\n",
> > -                rmrru->base_address, rmrru->end_address);
> > -            xfree(rmrru);
> > -        }
> > -        else if ( base_addr > end_addr )
> > -        {
> > -            dprintk(XENLOG_WARNING VTDPREFIX,
> > -                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
> > -                rmrru->base_address, rmrru->end_address);
> > -            xfree(rmrru);
> > -            ret = -EFAULT;
> > -        }
> > -        else
> > -        {
> > -            if ( iommu_verbose )
> > -                dprintk(VTDPREFIX,
> > -                        "  RMRR region: base_addr %"PRIx64
> > -                        " end_address %"PRIx64"\n",
> > -                        rmrru->base_address, rmrru->end_address);
> > -            acpi_register_rmrr_unit(rmrru);
> > -        }
> > -    }
> >  
> >      return ret;
> >  }
> > -- 
> > 2.1.3
> 
> 

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