Re: [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}

On Sun, Aug 29, 2021 at 10:11:46PM -0700, Andi Kleen wrote:

On 8/29/2021 3:26 PM, Michael S. Tsirkin wrote:

quoted

On Sun, Aug 29, 2021 at 09:17:53AM -0700, Andi Kleen wrote:

quoted

Also I changing this single call really that bad? It's not that we changing
anything drastic here, just give the low level subsystem a better hint about
the intention. If you don't like the function name, could make it an
argument instead?

My point however is that the API should say that the
driver has been audited,

We have that status in the struct device. If you want to tie the ioremap to
that we could define a ioremap_device() with a device argument and decide
based on that.

But it's not the device that is audited. And it's not the device
that might be secure or insecure. It's the driver.

Or we can add _audited to the name. ioremap_shared_audited?

But it's not the mapping that has to be done in handled special way.
It's any data we get from device, not all of it coming from IO, e.g.
there's DMA and interrupts that all have to be validated.
Wouldn't you say that what is really wanted is just not running
unaudited drivers in the first place?

quoted

not that the mapping has been
done in some special way. For example the mapping can be
in some kind of wrapper, not directly in the driver.
However you want the driver validated, not the wrapper.

Here's an idea:

I don't think magic differences of API behavior based on some define are a
good idea.  That's easy to miss.

Well ... my point is that actually there is no difference in API
behaviour. the map is the same map, exactly same data goes to device. If
anything any non-shared map is special in that encrypted data goes to
device.

That's a "COME FROM" in API design.

Also it wouldn't handle the case that a driver has both private and shared
ioremaps, e.g. for BIOS structures.

Hmm. Interesting.  It's bios maps that are unusual and need to be private though ...

And we've been avoiding that drivers can self declare auditing, we've been
trying to have a separate centralized list so that it's easier to enforce
and avoids any cut'n'paste mistakes.

-Andi

Now I'm confused. What is proposed here seems to be basically that,
drivers need to declare auditing by replacing ioremap with
ioremap_shared.

-- 
MST