linux-kernel - Re: [PATCH 1/5] PCI: add resizeable BAR infrastructure v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Mar 2017 14:20:24 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Christian König <deathsimple@...afone.de>
Cc:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        dri-devel@...ts.freedesktop.org,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        amd-gfx@...ts.freedesktop.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/5] PCI: add resizeable BAR infrastructure v2

On Mon, Mar 6, 2017 at 1:40 PM, Christian König <deathsimple@...afone.de> wrote:
> From: Christian König <christian.koenig@....com>
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf.
>
> v2: provide read helper as well

Commit message left away the explanation at which point this API might
be useful and how it fits in managed resources model?

>  /**
> + * pci_rbar_get_sizes - get possible sizes for BAR

Why not simple pci_rbar_get_possible_sizes() ?

> +u32 pci_rbar_get_sizes(struct pci_dev *pdev, int bar)
> +{
> +       int pos, nbars;
> +       u32 ctrl, cap;
> +       int i;
> +
> +       pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +       if (!pos)

> +               return 0x0;

return 0;

> +
> +       pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +       nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +

> +       for (i = 0; i < nbars; ++i, pos += 8) {

8 is defined somewhere in the spec? (Yes, I understand that is just 64
bits shift)

> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> +               return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
> +                       PCI_REBAR_CTRL_SIZES_SHIFT;
> +       }
> +

> +       return 0x0;

return 0;

> +/**
> + * pci_rbar_get_size - get the current size of a BAR

pci_rbar_get_current_size() ?

> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to

> + * @size: new size as defined in the spec.

 * @size: bitmasked value of new size (bit 0=1MB, ..., bit 19=512G)

?

It will briefly get a clue without reading either spec or long description.

> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */

> +bool pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)

I would return int and error code. It would be better in the future
and seems in alignment with above.

> +{
> +       int pos, nbars;
> +       u32 ctrl;
> +       int i;

All ints are unsigned?

-- 
With Best Regards,
Andy Shevchenko