from:"Andrew Cooks"

Re: [PATCH v4 00/16] PCI/iommu: Fix DMA alias problems

2014-05-29 Thread Andrew Cooks

On Thu, May 29, 2014 at 4:29 AM, Bjorn Helgaas  wrote:
> On Thu, May 22, 2014 at 05:07:23PM -0600, Alex Williamson wrote:
>> For testing, this version can be found in my git tree:
>>
>> git://github.com/awilliam/linux-vfio.git dma-alias-v4
>>
>> Please report any issues.
>>
>> v4:
>>  - Change dma_func_alias to dma_alias_devfn, holding a single
>>devfn to alias, thereby supporting aliases to the wrong slot.
>>The DMA alias iterator is easily changed, but IOMMU grouping
>>requires significant rework.  This is now done in IOMMU code
>>rather than PCI code.
>>
>>  - AMD-Vi - try to incorporate IVRS aliases dynamically into
>>PCI alias quirks to make sure that our grouping remains the
>>same.  Potentially this could end up reporting BIOS aliases
>>that we can add to our list of quirks.
>>
>> v3:
>>  - Found several instances where I had PCI_SLOT when I meant
>>PCI_FUNC.  Thanks to Andrew for spotting this.  This should
>>fix the problem he was having with Ricoh quirks.  We also
>>pruned down the func0 quirks to only those that we know are
>>needed.  We can always add them back later.
>>
>>  - Found a case in intel-iommu of using dev_is_pci() where I
>>really wanted !dev_is_pci().  Fixed.
>>
>> v2:
>>  - Several new Marvell controllers added to quirks.  There's been
>>a lot of success reported with this series in
>>https://bugzilla.kernel.org/show_bug.cgi?id=42679
>>
>>  - Add quirk for ASMedia and Tundra PCIe-to-PCI bridges that do
>>not expose a PCIe capability.  These have been shown to use
>>the standard PCIe-to-PCI bridge requester ID.
>>
>>  - Fix copy/paste duplicate Ricoh quirk ID
>>
>>  - Fixed AMD IOMMU for the "ghost" function case where the DMA
>>alias is for an absent device.  The iommu rlookup table and
>>data fields need to be initializes.
>>
>>  - Fixed Intel interrupt remapping, I wasn't passing the target
>>bus number, only the alias bus number.
>>
>> These patches are split across PCI and IOMMU, but I've front-loaded
>> all of the PCI infrastructure so that the first 7 patches can be
>> applied to PCI-core, the IOMMU maintainers can pickup their patches,
>> then we can finish with dead code removal.  Bjorn might also be
>> willing to carry the IOMMU changes if the maintainers want to ack
>> them.
>
> I put 1-7 on a pci/iommu branch for v3.16.  I'm happy to include the rest,
> too, given acks from Joerg and David.  Or if they prefer to take them all,
> which might be easier than coordinating two trees, especially since there's
> PCI stuff at the beginning and end, here's my ack for the PCI bits (patches
> 1-7 and 15-16):
>
> Acked-by: Bjorn Helgaas 
>
> If you want to send me updated changelogs for patches 5 & 6, I'll drop
> those in.
>
> Didn't you have more testing reports?  I see George's, but I thought there
> were some others, too.
>
Tested-by: Andrew Cooks 

I've reviewed parts of this patch set, over multiple iterations, if
that's worth anything.

>> Original description:
>>
>> This series attempts to fix a couple issues we've had outstanding in
>> the PCI/IOMMU code for a while.  The first issue is with devices that
>> use the wrong requester ID for DMA transactions.  We already have a
>> sort of half-baked attempt to fix this for several Ricoh devices, but
>> the fix only helps them be useful through IOMMU groups, not the
>> general DMA case.  There are also several Marvell devices which use
>> use a different wrong requester ID and don't even fit into the DMA
>> source idea.  This series creates a DMA alias iterator that will
>> step through each possible alias of a device, allowing IOMMUs to
>> insert mappings for both the device and its aliases.
>>
>> Hand-in-hand with this is our broken pci_find_upstream_pcie_bridge()
>> function, which is known to blowup when it finds itself suddenly at
>> a PCIe device without crossing a PCIe-to-PCI bridge (as identified by
>> the PCIe capability).  It also likes to make the invalid assumption
>> that a PCIe device never has its requester ID masked by any usptream
>> bus.  We can fix this using the above new DMA alias iterator, since
>> that's effectively what this function was meant to do.
>>
>> Finally, with all these helpers, it makes sense to consolidate code
>> for determining IOMMU groups.  The first step in finding the root
>> of a group is finding the final upstream DMA alias for

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

2014-05-15 Thread Andrew Cooks

Hi Alex

On Fri, May 16, 2014 at 1:43 AM, Alex Williamson
 wrote:
> On Thu, 2014-05-15 at 07:40 +0800, Andrew Cooks wrote:
>> Hi Alex
>>
>> On Fri, May 9, 2014 at 11:28 PM, Alex Williamson
>>  wrote:
>> > 
>> >
>> > Original description:
>> >
>> > This series attempts to fix a couple issues we've had outstanding in
>> > the PCI/IOMMU code for a while.  The first issue is with devices that
>> > use the wrong requester ID for DMA transactions.  We already have a
>> > sort of half-baked attempt to fix this for several Ricoh devices, but
>> > the fix only helps them be useful through IOMMU groups, not the
>> > general DMA case.  There are also several Marvell devices which use
>> > use a different wrong requester ID and don't even fit into the DMA
>> > source idea.  This series creates a DMA alias iterator that will
>> > step through each possible alias of a device, allowing IOMMUs to
>> > insert mappings for both the device and its aliases.
>> >
>> > Hand-in-hand with this is our broken pci_find_upstream_pcie_bridge()
>> > function, which is known to blowup when it finds itself suddenly at
>> > a PCIe device without crossing a PCIe-to-PCI bridge (as identified by
>> > the PCIe capability).  It also likes to make the invalid assumption
>> > that a PCIe device never has its requester ID masked by any usptream
>> > bus.  We can fix this using the above new DMA alias iterator, since
>> > that's effectively what this function was meant to do.
>> >
>>
>> There are two cases where the DMA requester id seems to use the wrong
>> slot (as opposed to function) in the patch I attached to
>> https://bugzilla.kernel.org/show_bug.cgi?id=42679
>> The original bug reports are listed in the patch.
>>
>> Unfortunately, I wasn't able to get test feedback on those two cases,
>> but I'm wondering...
>> Did I understand correctly that a slot alias is something that could be 
>> needed?
>> If so, is it a variation of the PCIe-to-PCI bridge case that's already
>> covered or will it require a different approach?
>
> Wow, I didn't think that kind of broken was possible.  Maybe instead of
> a bitmap of function aliases we could have a single devfn alias for a
> device.  That means we'd only be able to support a single alias for a
> device, but since I don't think we've seen devices that use more than a
> single alias, maybe that's ok.

The current patch creates a context entry for the reported device
(function 0), plus it's alias (function 1). Is there reason to always
add a context entry for the reported devfn and define 'alias' to mean
'one additional devfn'? That will work for all the Marvell cases.

In the testing I did, the Marvell controllers needed context entries
for both function 0 and 1. It would be nice if someone could confirm
or debunk this. I tested with a 88SE9172 with both ports of the
controller in use.

Thanks,

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

2014-05-14 Thread Andrew Cooks

Hi Alex

On Fri, May 9, 2014 at 11:28 PM, Alex Williamson
 wrote:
> 
>
> Original description:
>
> This series attempts to fix a couple issues we've had outstanding in
> the PCI/IOMMU code for a while.  The first issue is with devices that
> use the wrong requester ID for DMA transactions.  We already have a
> sort of half-baked attempt to fix this for several Ricoh devices, but
> the fix only helps them be useful through IOMMU groups, not the
> general DMA case.  There are also several Marvell devices which use
> use a different wrong requester ID and don't even fit into the DMA
> source idea.  This series creates a DMA alias iterator that will
> step through each possible alias of a device, allowing IOMMUs to
> insert mappings for both the device and its aliases.
>
> Hand-in-hand with this is our broken pci_find_upstream_pcie_bridge()
> function, which is known to blowup when it finds itself suddenly at
> a PCIe device without crossing a PCIe-to-PCI bridge (as identified by
> the PCIe capability).  It also likes to make the invalid assumption
> that a PCIe device never has its requester ID masked by any usptream
> bus.  We can fix this using the above new DMA alias iterator, since
> that's effectively what this function was meant to do.
>

There are two cases where the DMA requester id seems to use the wrong
slot (as opposed to function) in the patch I attached to
https://bugzilla.kernel.org/show_bug.cgi?id=42679
The original bug reports are listed in the patch.

Unfortunately, I wasn't able to get test feedback on those two cases,
but I'm wondering...
Did I understand correctly that a slot alias is something that could be needed?
If so, is it a variation of the PCIe-to-PCI bridge case that's already
covered or will it require a different approach?

Thanks,

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

2014-05-10 Thread Andrew Cooks

On Sat, May 10, 2014 at 8:15 PM, George Spelvin  wrote:
> Well, chalk up one test failure.
>
> I'm running a GA-X79-UP4 motherboard with VT-d enabled.
> It has 3x Marvell SATA controllers:
> 05:00.0 SATA controller: Marvell Technology Group Ltd. 88SE9172 SATA 6Gb/s 
> Controller (rev 11)
> 06:00.0 SATA controller: Marvell Technology Group Ltd. 88SE9172 SATA 6Gb/s 
> Controller (rev 11)
> 07:00.0 SATA controller: Marvell Technology Group Ltd. 88SE9172 SATA 6Gb/s 
> Controller (rev 11)
>
> With Andrew's patch from https://bugzilla.kernel.org/show_bug.cgi?id=42679#c22
> the Marvell SATA ports work.  With your dma-alias branch (rebased onto 
> 3.15-rc5),
> the ports don't work:
>
> -ata7.00: failed to IDENTIFY (INIT_DEV_PARAMS failed, err_mask=0x80)
> -dmar: DRHD: handling fault status reg 502
> -dmar: DMAR:[DMA Write] Request device [05:00.1] fault addr fffc
> -DMAR:[fault reason 02] Present bit in context entry is clear

When I checked, about 12 hours ago, the dma-alias branch on github
didn't have all the v2 changes, so I applied the v2 patch set on
3.15-rc5. You may want to try that as well.

I didn't notice any problems on the board I used: GA-X79-UD5, with the
same 88SE9172 controllers as above and with drives attached to each of
the internal SATA ports.

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 03/13] PCI: quirk dma_func_alias for Ricoh devices

2014-05-09 Thread Andrew Cooks

Hi Alex

On Sat, May 3, 2014 at 1:15 PM, Alex Williamson
 wrote:
> On Sat, 2014-05-03 at 10:29 +0800, Andrew Cooks wrote:
>> Hi Alex
>>
>> On Fri, May 2, 2014 at 12:27 AM, Alex Williamson
>>  wrote:
>> > The existing quirk for these devices doesn't really solve the problem,
>> > re-implement it using the DMA alias iterator.  We'll come back later
>> > and remove the existing quirk and dma_source interface.
>> >
>> > Signed-off-by: Alex Williamson 
>> > ---
>> >  drivers/pci/quirks.c |   16 
>> >  1 file changed, 16 insertions(+)
>> >
>> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> > index e729206..a458c6b 100644
>> > --- a/drivers/pci/quirks.c
>> > +++ b/drivers/pci/quirks.c
>> > @@ -,6 +,22 @@ int pci_dev_specific_reset(struct pci_dev *dev, int 
>> > probe)
>> > return -ENOTTY;
>> >  }
>> >
>> > +static void quirk_dma_func0_alias(struct pci_dev *dev)
>> > +{
>> > +   if (PCI_SLOT(dev->devfn) != 0)
>> > +   dev->dma_func_alias |= (1 << 0);
>> > +}
>> > +
>> > +/*
>> > + * https://bugzilla.redhat.com/show_bug.cgi?id=605888
>> > + *
>> > + * Some Ricoh devices use function 0 as the PCIe requester ID for DMA.
>> > + */
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe822, 
>> > quirk_dma_func0_alias);
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe230, 
>> > quirk_dma_func0_alias);
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe832, 
>> > quirk_dma_func0_alias);
>> > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_RICOH, 0xe832, 
>> > quirk_dma_func0_alias);
>>
>> 0xe832 is listed twice.
>
> oops, copy-paste error
>
>> Previously only 0xe832 needed the dma alias on my thinkpad T410, which
>> has all three devices.
>>
>> > +
>> >  static struct pci_dev *pci_func_0_dma_source(struct pci_dev *dev)
>> >  {
>> > if (!PCI_FUNC(dev->devfn))
>> >
>>
>> Unfortunately, this quirk doesn't work for me. I tried it without
>> modification, as well as with each alias individually. I get:
>>
>> Set context mapping for 0d:00.3
>> firewire_ohci :0d:00.3: added OHCI v1.10 device as card 0, 4 IR +
>> 4 IT contexts, quirks 0x11
>> dmar: DRHD: handling fault status reg 2
>> dmar: DMAR:[DMA Read] Request device [0d:00.0] fault addr f000
>> DMAR:[fault reason 02] Present bit in context entry is clear
>>
>> I think I need to see
>> Set context mapping for 0d:00.0
>> before
>> Set context mapping for 0d:00.3
>
> It would actually be the reverse, we always set the device, then the
> alias for the device.

Ok, excuse my ignorance, but is there a window where the driver can
start doing DMA, before the alias is registered?

>
>> in the log, but it's not there. I'd love to look into this and
>> understand it properly, but I don't have time for the next four weeks.
>>
>> The devices are attached as follows:
>> BDF, device ID
>> 0d:00.0, e822
>> 0d:00.1, e230
>> 0d:00.3, e832
>>
>> The kernel log is attached.
>
> Hmm, there are only a few reasons why you'd never see 0d:00.3 followed
> by 0d:00.0...
>
> 1) dma_func_alias bit 0 isn't getting set on 0d:00.3; we are building
> with CONFIG_PCI_QUIRKS=y, right?

Yes.

> 2) domain_context_mapping_one called from domain_context_mapping_cb
> returns !0; there's only one possible non-zero return for a non-vm,
> non-si domain

domain_context_mapping_one returns 0 - I've checked.

> 3) something is broken in the first loop of pci_for_each_dma_alias; I'm
> not seeing anything obvious
>
> Anyway, appreciate an debugging you're able to fit in, my only ricoh
> device has only function 0.  Thanks,
>

Please note that this device works with the patch I attached to
https://bugzilla.kernel.org/show_bug.cgi?id=42679 (despite its many
faults).
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Who understand the IOMMU code?

2014-01-30 Thread Andrew Cooks

On 31 January 2014 04:12, George Spelvin  wrote:
>
> I'm trying to add a quirk to the IOMMU code to handle certain buggy
> devices which use the wrong requester id (devfn) for PCIe requests.
> In my case, there are a number of Marvell SATA controllers, popular with
> motherboard manufacturers, which generate DMA from functions 0 and 1,
> even though they only support function 0.
>
> Andrew cooks wrote a working (but, to my eyes, ugly) patch; see
> https://bugzilla.kernel.org/show_bug.cgi?id=42679#c22
>
> I'm trying to improve it, but even limiting myself to intel-iommu.c,
> it's definitely hairy.

What's hairy? The patch, the problem, or the iommu code?

> Right now, I'm trying to figure out where to remove the additional
> mapping.

Which additional mapping? Are you implying that the patch adds redundant
context entries or are you talking about folding the
quirk_map_multi_requester_ids()
and quirk_map_requester_id()  functions into the main code path?

> Currently I'm looking at adding another clear_context_table() call
> before iommu_detach_dev() in domain_remove_one_dev_info(); my goal is
> to piggyback on iommu_detach_dev's flush calls.
>
> But I'm having trouble figuring out the whole device_domain_info list
> business.
>
> In particular, does the comparison of domain, bus, and
> devinfo ever produce a different result than just comparing
> info->dev to pdev?
>
> I have a flag in the pci_device warning about the quirk, but
> I'm having trouble finding the function that's the opposite of
> domain_context_mapping().
>
> Does anyone actually understand this stuff well enough to review my
> patches when I finish?  May I ask for a bit of guidance on the way?
> Right now I don't even understand the Intel iommu code, much less all
> ther others I should be patching for a full fix.

Let me see if I understand you correctly.
1) You don't like the simple patch that clearly shows the problem (and offers
a simple work-around), because it's too ugly.
2) You complain that "this stuff" (presumably the stuff you didn't
learn from the
ugly patch) is hard to understand.
3) You question whether anyone else understands "this stuff".
4) You want to know whether anyone is qualified to review your patch when
you 'finish it' ...
5) ... as long as someone just help you a little bit?

It that really what you meant?

> The same code points would also be a good place to add phantom functions
> support some day.

I don't think you understood the relevance of Alex's patch set for a
"PCIe requester ID interface", that I referred you to.

Firstly, finding the full set of requester ids that an iommu might see for every
pdev below it is a bigger problem than just quirky Marvell SATA controllers and
phantom functions (which may or may not actually be used by anyone). PCIe
bridges, even spec compliant ones, introduce similar issues.

Secondly, that patch tries to unify the quirky and non quirky cases under the
pcie_for_each_requester() function, which seems like the prefered approach.

Thirdly, that patch was written and reviewed by people who do actually
understand how "this stuff" works, so there is much to learn from it if you want
to write the solution that is ultimately accepted into mainline and convince
them that you can (and will) maintain it.

Here's the link to the patch posting and review again:
https://lists.linuxfoundation.org/pipermail/iommu/2013-July/006083.html

When you've read and understood that discussion, have another look at the
current domain_context_mapping() and try to see the resemblance between
the handling of bridging, dependant devices and quirky devices.

Ultimately, this isn't going anywhere until David Woodhouse, the Intel iommu
maintainer, gets involved. I have not yet found the magical incantation to
achieve that.

May the odds be ever in your favour!

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

2014-01-20 Thread Andrew Cooks

On Tue, Jan 21, 2014 at 10:00 AM, George Spelvin  wrote:
> New recipient: Alex Williamson, who added the pci_get_dma_source()
> quirk that resembles this one.
>
> Alex, Andrew is working on a patch to fix some more buggy
> PCIe devices like the Ricoh ones that you added a quirk for.
>
> His current patch can be found at
>
> https://bugzilla.kernel.org/show_bug.cgi?id=42679#c22
>
> Ir only works on Intel IOMMUs, and sort of duplicates the functionality
> of your quirk.
>
> Obviously, I hate the duplication and would like to fold them together,
> but there are a few fundamental differences.

Hang on! The existing quirk only worked for vfio groups last time I
checked and didn't actually solve the problem for using the device on
the host.

Alex had done a bunch of work to support requester id quirks (see
http://marc.info/?l=linux-pci&m=137357663626523&w=3), but that seems
blocked.

> Most notably, Andrew's patch enables the incorrect requester ID
> *in addition to* the correct one.  I'm not sure if this is necessary,
> but it seems like a nice defensive feature in case the vendor fixes
> the problem in a minor rev, and the ability is also needed by PCIe
> phantom function support,
>
> The other thing is that pci_get_dma_source returns a pci_dev, but
> Andrew's patch deals with devices which use requester IDs which
> don't correspond to existing PCI devices at all:
>
> - Marvell SATA controllers which exist only as devid 00.0, but generate
>   request devids of 00.1.  (The latter function might be PATA support
>   on chips which support that.)

No, I can prove that both .0 and .1 are used on the 9172 controller,
depending on which of the two SATA port(s) is(are) in use. If you have
the same hardware, I suggest you verify this.

> - Mellanox 26428 Infiniband controllers which also only provide one
>   function, but use a source devid of 00.6
> - An LSI MegaRAID SAS 2078 PCI-X controller that is device 0e.0, but
>   generates request devids of 08.0
> - An Adaptec RAID controller ie 0e.0 that makes requests as 01.0.
>
> My thought for implementing Andrew's version was to add an 8-bit devfn_quirk
> field to struct pci_dev that contains the delta to the devfn that will
> appear in the source requester ID.
>
> (By using a delta rather than the target value, the default value of 0
> means "no quirk".)
>
> This would be initialized at boot time using a standard PCI quirk,
> avoiding the need for linearly searching potentially long device lists
> every time an IOMMU mapping is set up.
>
> (Even if I only use a single-bit flag, that would avoid the list
> searching for all the non-buggy devices.)
>
>
> But I'm not quite sure what the locking rules are on the whole swap_pci_ref
> business.  Or if the Ricoh devices have to be handled more carefully
> because multiple functions need to arbitrate over the IOMMU tables
> for function 0.

The Ricoh firewire function simply uses the wrong requester ID. The
other functions work correctly. Remapping only the firewire specific
function of the multifunction device (as in my patch) fixes the
problem.

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

2014-01-20 Thread Andrew Cooks

On Tue, Jan 21, 2014 at 7:35 AM, George Spelvin  wrote:
> I'm looking over the bug reports, and I'm trying to understand why you
> need two mechanisms.  You have one table (pci_dev_dma_source_map)
> that maps one devfn to a secondary one, and a second table
> (pci_dev_dma_multi_source_map) that gives a 2-bit bitmap of function
> numbers.
I haven't found a simple way to combine the two maps that still makes
the two distinct problems apparent.
pci_dev_dma_multi_source_map deals with devices that use multiple
functions, but the correct device number.
pci_dev_dma_source_map deals with devices that use a single incorrect
devfn that's mangled in both device and function numbers.

> But as I understand it, all the Marvell devices listed there always have
> a devfn of 00.0, so you could just move them to the first table with a
> mapping to 00.1.
>
> If they also have device/function 00.1 using a requester ID of 00.0
> you could add a second entry.
>
> The only time you'd need a bitmap would be if the device ended up on
> function 2 or higher and still used functions 0 and 1 in request IDs.

Sure, that sounds reasonable and certainly needs to be considered, but
since there's no complete record of what devices get this wrong and
how they get this wrong, I wanted to
* show all requester ids are used for each device in a concise entry,
* build up as much of a record of this problem as possible (hence the
comments and bug references),
* make it easy to see the general problem and have this discussion,
* not make assumptions about which functions would be visible and
which requester ids are actually used (like a device showing up at
00.0 that uses 00.1 and 00.2 only).

Who knows what will show up when iommu is turned on by default again.

> Now, unfortunately this does not cover the phantom functions case, but
> that could be done with separate code.
>
>
> The other question is when to do the lookup.  I don't know how often
> IOMMU mapping is done, and if linear search through a list of PCI devices
> would be a performance problem.  It would be nicer to have a standard
> PCI fixup done once which sets a flag in the pci_dev to either note the
> actual secondary devfn, or set a flag to trigger the secondary lookup
> only on the devices that need it.

Yes, I'd also like to hear what the gatekeepers think about this.

Thanks,

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

2014-01-20 Thread Andrew Cooks

On Tue, Jan 21, 2014 at 3:01 AM, George Spelvin  wrote:
>>> I massaged it a bit to fit my personal idea of "cleaner".
>>> The biggest logic change is the elimination of the fn_mapped
>>> variable from quirk_map_multi_requester_ids.
>
>> Thanks, that was detritus left over from debugging.
>
> The code still does the exact same thing; I just derive the value from
> (fn_map & ((1<>> I also got rid of the "fn_map << fn" logic (which is never false).
>
>> What if function 0 is not present? There were cases where the Marvell
>> 9172 only used function 1 (when only one port is in use). I don't
>> think it's safe to assume that function 0 is present when you're
>> trying to compensate for broken devices.
>
> Er... can you explain?  I did not change what the code *does* in the
> slightest, only the was it figures out what to do.
>
> I took this it out because it does nothing; it can be statically proved
> that the expression is always non-zero.
>
> Because "fn_map" is a non-zero 8-bit value and "fn" is an integer in
> the range 0-7, the value "fn_map << fn" is a non-zero 15-bit value.
> 15 bits can't overflow (even on a PDP-11 with 16-bit ints), so this is
> a non-zero value, and the loop will never terminate because of it.
>
> It sure looked like a bug to me, but I couldn't figure out what it
> was supposed to do.  Can you enlighten me?  "fn_map >> fn" (shifting
> right rather than left) achieves early termination of the loop, but
> isn't essential.

OK, I understand the problem now. Early exit was the intended effect
and for some reason I thought the new version could exit too early,
but it seems fine.

>>> +   fn_map &= ~(1
>> Do you really find this "cleaner", as in more readable?
>
> I find it easier to think about fn_map as the bitmap of functions to
> be operated on.  Rather than have two conditions in the following loop.

I see that now.

> The confusing thing is "why are we skipping that function?"  (Because this
> is a quirk and the "normal case" has already been handled.)  It would be
> nicer if all the domain_context_mapping_one calls were grouped together.

Sure, but since this was the first non-trivial patch I submitted, I
tried to keep it non-intrusive and simple.

> I would like to see if there's a clean way to combine this with phantom
> function support (which I haven't find the code for yet).

I'm willing to believe that if phantom functions haven't shown up and
caused problems yet, it probably doesn't need fixing in a hurry.
SR-IOV seems like a better and more common way for devices to use
multiple lightweight functions.

>
>>> +   for (i = pci_dev_dma_source_map; i->devfn; i++) {
>
>> The bug that Martin Oehrling pointed out in the ticket is still there.
>
> Ah, thank you; I had missed that that.  Yes, it should be testing
> i->vendor.  You really get devices using the wrong slot as well as
> function?  Wow!
>
> Using multiple functions is at least anticipated in the PCI spec,
> even if they do it wrong.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

2014-01-20 Thread Andrew Cooks

On Mon, Jan 20, 2014 at 9:13 PM, George Spelvin  wrote:
> This is requried for me to get a Marvell 9172 SATA controller working
> with VT-d.
>
> Andrew Cooks wrote the original; see
> https://bugzilla.kernel.org/show_bug.cgi?id=42679#c22
>
> Without it, I see the same symptoms as in that bug report:
> the device is recognized, but probing the attached disk fails.
>
> I massaged it a bit to fit my personal idea of "cleaner".
> The biggest logic change is the elimination of the fn_mapped
> variable from quirk_map_multi_requester_ids.

Thanks, that was detritus left over from debugging.

> I also got rid of the "fn_map << fn" logic (which is never false).

What if function 0 is not present? There were cases where the Marvell
9172 only used function 1 (when only one port is in use). I don't
think it's safe to assume that function 0 is present when you're
trying to compensate for broken devices.

> I can vouch for quirk_map_multi_requester_ids working in my case;
> I can't speak for quirk_map_requester_id.

I've tested quirk_map_requester_id on a Thinkpad T410 with Ricoh IEEE
1394 Controller.

> Who do I nudge about actually getting some variant of this merged?

Thanks for your efforts to get this merged. If you look through the
list archives you'll see that I've posted previous iterations of this
before, without much enthusiasm.

>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 43b9bfea48..4695865051 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1677,6 +1677,111 @@ static int domain_context_mapping_one(struct 
> dmar_domain *domain, int segment,
> return 0;
>  }
>
> +static void iommu_detach_dev(struct intel_iommu *iommu, u8 bus, u8 devfn);
> +
> +static void quirk_unmap_multi_requesters(struct pci_dev *pdev, u8 fn_map)
> +{
> +   int fn;
> +   struct intel_iommu *iommu = device_to_iommu(pci_domain_nr(pdev->bus),
> +   pdev->bus->number, 
> pdev->devfn);
> +
> +   /* something must be seriously wrong if we can't lookup the iommu. */
> +   BUG_ON(!iommu);
> +
> +   fn_map &= ~(1<devfn));  /* Skip the normal case */

Do you really find this "cleaner", as in more readable?

> +
> +   for (fn = 0; fn_map >> fn; fn++) {
> +   if (fn_map & (1< +   iommu_detach_dev(iommu,
> +   pdev->bus->number,
> +   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn));
> +   dev_dbg(&pdev->dev,
> +   "requester id quirk; ghost func %d unmapped",
> +   fn);
> +   }
> +   }
> +}
> +
> +/* For quirky devices that use multiple requester ids. */
> +static int quirk_map_multi_requester_ids(struct dmar_domain *domain,
> +   struct pci_dev *pdev,
> +   int translation)
> +{
> +   int fn, err = 0;
> +   u8 fn_map = pci_multi_requesters(pdev);
> +
> +   /* this is the common, non-quirky case. */
> +   if (!fn_map)
> +   return 0;
> +
> +   fn_map &= ~(1<devfn));  /* Skip the normal case */
> +
> +   for (fn = 0; fn_map >> fn; fn++) {
> +   if (fn_map & (1< +   err = domain_context_mapping_one(domain,
> +   pci_domain_nr(pdev->bus),
> +   pdev->bus->number,
> +   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
> +   translation);
> +   if (err) {
> +   dev_err(&pdev->dev,
> +   "mapping ghost func %d failed", fn);
> +   quirk_unmap_multi_requesters(pdev,
> +   fn_map & ((1< +   return err;
> +   }
> +   dev_dbg(&pdev->dev,
> +   "requester id quirk; ghost func %d mapped", 
> fn);
> +   }
> +   }
> +   return 0;
> +}
> +
> +
> +static void quirk_unmap_requester_id(struct pci_dev *pdev)
> +{
> +   u8 devfn = pci_requester(pdev);
> +   struct intel_iommu *iommu = device_to_iommu(pci_domain_nr(pdev->bus),
> +   pdev->bus->number, 
> pdev->devfn);
> +
> +   /* something must be

Re: [RFC PATCH v2 1/2] pci: Create PCIe requester ID interface

2013-07-24 Thread Andrew Cooks

>>
>> What are we going to do if somebody hits this BUG_ON()?  If it's impossible
>> to hit, we should just remove it.  If it's possible to hit it in some weird
>> topology you didn't consider, we should see IOMMU faults for any requester
>> ID we neglected to map, and that fault would be a better debugging hint
>> than a BUG_ON() here.
>
> It's mostly for readability.  I've learned that we never what to look at
> dev->bus->self without first testing pci_is_root_bus(dev->bus), so if I
> was reading this code I'd stumble when I got here and spend too long
> looking around for the assumption that makes it not needed.  I suppose I
> could just make it a comment, but I thought why not make it official w/
> a BUG.
>
>> > +
>> > +   dev = dev->bus->self;
>> > +
>> > +   } while (dev != requester);
>> > +
>> > +   /*
>> > +* If we've made it here, @requester is a bridge upstream from
>> > +* @requestee.
>> > +*/
>> > +   if (pci_bridge_uses_endpoint_requester(requester))
>> > +   return pci_do_requester_callback(&requester, fn, data);
>> > +
>> > +   return fn(requester, PCI_BRIDGE_REQUESTER_ID(requester), data);
>> > +}
>> > +
>> > +/*
>> >   * find the upstream PCIe-to-PCI bridge of a PCI device
>> >   * if the device is PCIE, return NULL
>> >   * if the device isn't connected to a PCIe bridge (that is its parent is a
>> > diff --git a/include/linux/pci.h b/include/linux/pci.h
>> > index 3a24e4f..94e81d1 100644
>> > --- a/include/linux/pci.h
>> > +++ b/include/linux/pci.h
>> > @@ -1873,6 +1873,13 @@ static inline struct eeh_dev 
>> > *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>> >  }
>> >  #endif
>> >
>> > +struct pci_dev *pci_get_visible_pcie_requester(struct pci_dev *requestee,
>> > +  struct pci_dev *bridge,
>> > +  u16 *requester_id);
>>
>> The structure of this interface implies that there is only one visible
>> requester ID, but the whole point of this patch is that a transaction from
>> @requestee may appear with one of several requester IDs.  So which one will
>> this return?
>
> I thought the point of this patch was to have an integrated interface
> for finding the requester ID and doing something across all devices with
> that requester ID and thereby remove pci_find_upstream_pcie_bridge(),
> provide a way to quirk broken PCIe-to-PCI bridge and quirk dma_ops for
> devices that use the wrong requester ID.  In what case does a device
> appear to have multiple requester IDs?  We have cases where devices use
> the wrong requester ID, but AFAIK they only use a single wrong requester
> ID.  Thanks,
>

The cases I've found where multiple requester IDs were used are all related
to Marvell Sata controllers.

Here's a table of affected devices with links to the
bug reports. In each case both functions 0 and 1 are used.

static const struct pci_dev_dma_multi_source_map {
   u16 vendor;
   u16 device;
} pci_dev_dma_multi_source_map[] = {
/* Reported by Patrick Bregman
 * https://bugzilla.redhat.com/show_bug.cgi?id=863653 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9120},

   /* Reported by  Paweł Żak, Korneliusz Jarzębski, Daniel Mayer
* https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
* Justin Piszcz  https://lkml.org/lkml/2012/11/24/94 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9123},

   /* Reported by Robert Cicconetti
* https://bugzilla.kernel.org/show_bug.cgi?id=42679 and by
* Fernando https://bugzilla.redhat.com/show_bug.cgi?id=757166 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9128},

   /* Reported by Stijn Tintel
* https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9130},

   /* Reported by Gaudenz Steinlin
* https://lkml.org/lkml/2013/3/5/288 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9143},

   /* Reported by Andrew Cooks
* https://bugzilla.kernel.org/show_bug.cgi?id=42679 */
   { PCI_VENDOR_ID_MARVELL_EXT, 0x9172}
};


> Alex
>
>> > +int pcie_for_each_requester(struct pci_dev *requestee, struct pci_dev 
>> > *bridge,
>> > +   int (*fn)(struct pci_dev *, u16 id, void *),
>> > +   void *data);
>> > +
>> >  /**
>> >   * pci_find_upstream_pcie_bridge - find upstream PCIe-to-PCI bridge of a 
>> > device
>> >   * @pdev: the PCI device
>> >
>
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Ricoh DMAR bug returns? (WAS Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.)

2013-05-15 Thread Andrew Cooks

On Wed, May 15, 2013 at 12:40 PM, Pat Erley  wrote:
> On 04/05/2013 01:50 AM, Pat Erley wrote:
>>
>> On 04/05/2013 12:44 AM, Andrew Cooks wrote:
>>>
>>> On Tue, Apr 2, 2013 at 11:47 PM, Pat Erley  wrote:
>>>>
>>>> On 04/02/2013 10:50 AM, Andrew Cooks wrote:
>>>>>
>>>>>
>>>>> On 2 Apr 2013 15:37, "Pat Erley" >>>> <mailto:pat-l...@erley.org>> wrote:
>>>>>   >
>>>>>   > On 03/07/2013 09:35 PM, Andrew Cooks wrote:
>>>>>   >>
>>>>>   >> --- a/drivers/pci/quirks.c
>>>>>   >> +++ b/drivers/pci/quirks.c
>>>>>   >>
>>>>>   >> +/* Table of multiple (ghost) source functions. This is similar
>>>>> to the
>>>>>   >> + * translated sources above, but with the following differences:
>>>>>   >> + * 1. the device may use multiple functions as DMA sources,
>>>>>   >> + * 2. these functions cannot be assumed to be actual devices,
>>>>> they're simply
>>>>>   >> + * incorrect DMA tags.
>>>>>   >> + * 3. the specific ghost function for a request can not always be
>>>>> predicted.
>>>>>   >> + * For example, the actual device could be xx:yy.1 and it
>>>>> could use
>>>>>   >> + * both 0 and 1 for different requests, with no obvious way to
>>>>> tell
>>>>> when
>>>>>   >> + * DMA will be tagged as comming from xx.yy.0 and and when it
>>>>> will
>>>>> be tagged
>>>>>   >> + * as comming from xx.yy.1.
>>>>>   >> + * The bitmap contains all of the functions used in DMA tags,
>>>>> including the
>>>>>   >> + * actual device.
>>>>>   >> + * See https://bugzilla.redhat.com/show_bug.cgi?id=757166,
>>>>>   >> + * https://bugzilla.kernel.org/show_bug.cgi?id=42679
>>>>>   >> + * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1089768
>>>>>   >> + */
>>>>>   >> +static const struct pci_dev_dma_multi_func_sources {
>>>>>   >> +   u16 vendor;
>>>>>   >> +   u16 device;
>>>>>   >> +   u8 func_map;/* bit map. lsb is fn 0. */
>>>>>   >> +} pci_dev_dma_multi_func_sources[] = {
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9143, (1<<0)|(1<<1)},
>>>>>   >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
>>>>>   >> +   { 0 }
>>>>>   >> +};
>>>>>   >
>>>>>   >
>>>>>   > Adding another buggy device.  I have a Ricoh multifunction device:
>>>>>   >
>>>>>   > 17:00.0 SD Host controller: Ricoh Co Ltd MMC/SD Host Controller
>>>>> (rev
>>>>> 01)
>>>>>   > 17:00.3 FireWire (IEEE 1394): Ricoh Co Ltd R5C832 PCIe IEEE 1394
>>>>>   > Controller (rev 01)
>>>>>   >
>>>>>   > 17:00.0 0805: 1180:e822 (rev 01)
>>>>>   > 17:00.3 0c00: 1180:e832 (rev 01)
>>>>>   >
>>>>>
>>>>> The Ricoh device issue has been known for some time and a quirk has
>>>>> been
>>>>> available since commit 12ea6cad1c7d046 in June 2012.  It's slightly
>>>>> different than the problem this patch tries to work around [1].
>>>>
>>>>
>>>>
>>>> Hmm, I've had this problem with many recent (vanilla) kernels, up to and
>>>> including 3.9-rc5
>>>>
>>>>
>>>>>   > that adding entries for also fixed booting.  I don't have any SD
>>>>> cards or firewire devices handy to test that they work, but the system
>>>>> now boots, which was not the case without your patch and IOMMU/DMAR
>>>>> enabled.
>>>>>
>>>>> That is really strange. Could you tell u

Re: RFC: vfio / iommu driver for hardware with no iommu

2013-04-26 Thread Andrew Cooks

On Fri, Apr 26, 2013 at 6:23 AM, Don Dutile  wrote:
> On 04/24/2013 10:49 PM, Sethi Varun-B16395 wrote:
>>
>>
>>
>>> -Original Message-
>>> From: iommu-boun...@lists.linux-foundation.org [mailto:iommu-
>>> boun...@lists.linux-foundation.org] On Behalf Of Don Dutile
>>> Sent: Thursday, April 25, 2013 1:11 AM
>>> To: Alex Williamson
>>> Cc: Yoder Stuart-B08248; iommu@lists.linux-foundation.org
>>> Subject: Re: RFC: vfio / iommu driver for hardware with no iommu
>>>
>>> On 04/23/2013 03:47 PM, Alex Williamson wrote:

 On Tue, 2013-04-23 at 19:16 +, Yoder Stuart-B08248 wrote:
>
>
>> -Original Message-
>> From: Alex Williamson [mailto:alex.william...@redhat.com]
>> Sent: Tuesday, April 23, 2013 11:56 AM
>> To: Yoder Stuart-B08248
>> Cc: Joerg Roedel; iommu@lists.linux-foundation.org
>> Subject: Re: RFC: vfio / iommu driver for hardware with no iommu
>>
>> On Tue, 2013-04-23 at 16:13 +, Yoder Stuart-B08248 wrote:
>>>
>>> Joerg/Alex,
>>>
>>> We have embedded systems where we use QEMU/KVM and have the
>>> requirement to do device assignment, but have no iommu.  So we
>>> would like to get vfio-pci working on systems like this.
>>>
>>> We're aware of the obvious limitations-- no protection, DMA'able
>>> memory must be physically contiguous and will have no iova->phy
>>> translation.  But there are use cases where all OSes involved are
>>> trusted and customers can
>>> live with those limitations.   Virtualization is used
>>> here not to sandbox untrusted code, but to consolidate multiple
>>> OSes.
>>>
>>> We would like to get your feedback on the rough idea.  There are
>>> two parts-- iommu driver and vfio-pci.
>>>
>>> 1.  iommu driver
>>>
>>> First, we still need device groups created because vfio is based on
>>> that, so we envision a 'dummy' iommu driver that implements only
>>> the add/remove device ops.  Something like:
>>>
>>>   static struct iommu_ops fsl_none_ops = {
>>>   .add_device = fsl_none_add_device,
>>>   .remove_device  = fsl_none_remove_device,
>>>   };
>>>
>>>   int fsl_iommu_none_init()
>>>   {
>>>   int ret = 0;
>>>
>>>   ret = iommu_init_mempool();
>>>   if (ret)
>>>   return ret;
>>>
>>>   bus_set_iommu(&platform_bus_type,&fsl_none_ops);
>>>   bus_set_iommu(&pci_bus_type,&fsl_none_ops);
>>>
>>>   return ret;
>>>   }
>>>
>>> 2.  vfio-pci
>>>
>>> For vfio-pci, we would ideally like to keep user space mostly
>>> unchanged.  User space will have to follow the semantics of mapping
>>> only physically contiguous chunks...and iova will equal phys.
>>>
>>> So, we propose to implement a new vfio iommu type, called
>>> VFIO_TYPE_NONE_IOMMU.  This implements any needed vfio interfaces,
>>> but there are no calls to the iommu layer...e.g. map_dma() is a
>>> noop.
>>>
>>> Would like your feedback.
>>
>>
>> My first thought is that this really detracts from vfio and iommu
>> groups being a secure interface, so somehow this needs to be clearly
>> an insecure mode that requires an opt-in and maybe taints the
>> kernel.  Any notion of unprivileged use needs to be blocked and it
>> should test CAP_COMPROMISE_KERNEL (or whatever it's called now) at
>> critical access points.  We might even have interfaces exported that
>> would allow this to be an out-of-tree driver (worth a check).
>>
>> I would guess that you would probably want to do all the iommu group
>> setup from the vfio fake-iommu driver.  In other words, that driver
>> both creates the fake groups and provides the dummy iommu backend for
>>>
>>> vfio.
>>
>> That would be a nice way to compartmentalize this as a
>> vfio-noiommu-special.
>
>
> So you mean don't implement any of the iommu driver ops at all and
> keep everything in the vfio layer?
>
> Would you still have real iommu groups?...i.e.
> $ readlink /sys/bus/pci/devices/:06:0d.0/iommu_group
> ../../../../kernel/iommu_groups/26
>
> ...and that is created by vfio-noiommu-special?


 I'm suggesting (but haven't checked if it's possible), to implement
 the iommu driver ops as part of the vfio iommu backend driver.  The
 primary motivation for this would be to a) keep a fake iommu groups
 interface out of the iommu proper (possibly containing it in an
 external driver) and b) modularizing it so we don't have fake iommu
 groups being created by default.  It would have to populate the iommu
 groups sysfs interfaces to be compatible with vfio.

> Right now when the PCI and platform buses are probed, the iommu
> driver add-dev

Re: [PATCH 0/9] AMD IOMMU cleanups, fixes and IVRS bug workarounds

2013-04-13 Thread Andrew Cooks

On Fri, Apr 12, 2013 at 4:06 PM, Joerg Roedel  wrote:
> Hi Shuah,
>
> On Wed, Apr 10, 2013 at 10:06:02AM -0600, Shuah Khan wrote:
>> On Tue, Apr 9, 2013 at 2:12 PM, Joerg Roedel  wrote:
>> >  Documentation/kernel-parameters.txt |   14 
>> >  drivers/iommu/amd_iommu.c   |   79 +++---
>> >  drivers/iommu/amd_iommu_init.c  |  151 
>> > +++
>> >  drivers/iommu/amd_iommu_types.h |1 +
>> >  4 files changed, 182 insertions(+), 63 deletions(-)
>
>> Reviewed all the patches in this set. No longer have access to test machine. 
>> :(
>
> Oh, that's sad. You were the only one having a machine wich actually has
> unity-mapped ranges defined in the BIOS table. The code for those
> mappings was basically untested before you ran it on that machine.
>
What is the machine in question? Maybe someone else has access to one,
if it's not too exotic.

--
a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Ricoh DMAR bug returns? (WAS Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.)

2013-04-04 Thread Andrew Cooks

On Tue, Apr 2, 2013 at 11:47 PM, Pat Erley  wrote:
> On 04/02/2013 10:50 AM, Andrew Cooks wrote:
>>
>> On 2 Apr 2013 15:37, "Pat Erley" > <mailto:pat-l...@erley.org>> wrote:
>>  >
>>  > On 03/07/2013 09:35 PM, Andrew Cooks wrote:
>>  >>
>>  >> --- a/drivers/pci/quirks.c
>>  >> +++ b/drivers/pci/quirks.c
>>  >>
>>  >> +/* Table of multiple (ghost) source functions. This is similar to the
>>  >> + * translated sources above, but with the following differences:
>>  >> + * 1. the device may use multiple functions as DMA sources,
>>  >> + * 2. these functions cannot be assumed to be actual devices,
>> they're simply
>>  >> + * incorrect DMA tags.
>>  >> + * 3. the specific ghost function for a request can not always be
>> predicted.
>>  >> + * For example, the actual device could be xx:yy.1 and it could use
>>  >> + * both 0 and 1 for different requests, with no obvious way to tell
>> when
>>  >> + * DMA will be tagged as comming from xx.yy.0 and and when it will
>> be tagged
>>  >> + * as comming from xx.yy.1.
>>  >> + * The bitmap contains all of the functions used in DMA tags,
>> including the
>>  >> + * actual device.
>>  >> + * See https://bugzilla.redhat.com/show_bug.cgi?id=757166,
>>  >> + * https://bugzilla.kernel.org/show_bug.cgi?id=42679
>>  >> + * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1089768
>>  >> + */
>>  >> +static const struct pci_dev_dma_multi_func_sources {
>>  >> +   u16 vendor;
>>  >> +   u16 device;
>>  >> +   u8 func_map;/* bit map. lsb is fn 0. */
>>  >> +} pci_dev_dma_multi_func_sources[] = {
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9143, (1<<0)|(1<<1)},
>>  >> +   { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
>>  >> +   { 0 }
>>  >> +};
>>  >
>>  >
>>  > Adding another buggy device.  I have a Ricoh multifunction device:
>>  >
>>  > 17:00.0 SD Host controller: Ricoh Co Ltd MMC/SD Host Controller (rev
>> 01)
>>  > 17:00.3 FireWire (IEEE 1394): Ricoh Co Ltd R5C832 PCIe IEEE 1394
>>  > Controller (rev 01)
>>  >
>>  > 17:00.0 0805: 1180:e822 (rev 01)
>>  > 17:00.3 0c00: 1180:e832 (rev 01)
>>  >
>>
>> The Ricoh device issue has been known for some time and a quirk has been
>> available since commit 12ea6cad1c7d046 in June 2012.  It's slightly
>> different than the problem this patch tries to work around [1].
>
>
> Hmm, I've had this problem with many recent (vanilla) kernels, up to and
> including 3.9-rc5
>
>
>>  > that adding entries for also fixed booting.  I don't have any SD
>> cards or firewire devices handy to test that they work, but the system
>> now boots, which was not the case without your patch and IOMMU/DMAR
>> enabled.
>>
>> That is really strange. Could you tell us what kernel version you tested
>> and provide dmesg output?
>
>
> I'll capture a vanilla 3.8.5 boot without any patches and iommu=off, then
> try to find another machine to catch what I can of a netconsole boot with
> iommu=on.  What's the preferred way to send these?  pastebin links?
>
> I'd been running the 'dirty' fix that's in the redhat bugzilla entry.  I
> checked my .config and have CONFIG_PCI_QUIRKS=y, and verified my devices are
> in the quirks table for the pci_func_0_dma_source fixup.
>
Do you mean that even though your hardware is specifically listed in
the quirk table, the quirk simply hasn't worked for you? That would be
unfortunate, to say the least.

The fedora kernel included a separate patch for this issue until
recently (see https://bugzilla.redhat.com/show_bug.cgi?id=880051).  It
basically just disabled DMAR when the Ricoh device is present, the
same as the patch to the mailing list you mentioned.

Is the dirty fix you're referring to comment 7?
https://bugzilla.redhat.com/show_bug.cgi?id=605888#c7

>>
>> [1] In the Ricoh case, multiple functions are used for real devices and
>> the bug is that these devices all use function 0 during DMA. In this
>> particular case, I'd expect the FireWire device 17:00.3 to issue DMA
>> from the SD Host Controller address 17:00.0. The quirk is not too much
>> of a terrible hack - it's a fairly simple translation.
>>
>> In the Marvell case, the real device uses DMA source tags that don't
>> actually belong to any visible devices. The quirk to make this work is
>> more invasive, not nearly as elegant and has not attracted much
>> enthusiasm from subsystem maintainers, though I'm still hopeful that a
>> quirk will be merged in some form or another.
>>
>
> Thanks for explaining the difference!
>
> Pat
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

2013-04-02 Thread Andrew Cooks

On 2 Apr 2013 15:37, "Pat Erley"  wrote:
>
> On 03/07/2013 09:35 PM, Andrew Cooks wrote:
>>
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>>
>> +/* Table of multiple (ghost) source functions. This is similar to the
>> + * translated sources above, but with the following differences:
>> + * 1. the device may use multiple functions as DMA sources,
>> + * 2. these functions cannot be assumed to be actual devices, they're
simply
>> + * incorrect DMA tags.
>> + * 3. the specific ghost function for a request can not always be
predicted.
>> + * For example, the actual device could be xx:yy.1 and it could use
>> + * both 0 and 1 for different requests, with no obvious way to tell when
>> + * DMA will be tagged as comming from xx.yy.0 and and when it will be
tagged
>> + * as comming from xx.yy.1.
>> + * The bitmap contains all of the functions used in DMA tags, including
the
>> + * actual device.
>> + * See  https://bugzilla.redhat.com/show_bug.cgi?id=757166,
>> + * https://bugzilla.kernel.org/show_bug.cgi?id=42679
>> + * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1089768
>> + */
>> +static const struct pci_dev_dma_multi_func_sources {
>> +   u16 vendor;
>> +   u16 device;
>> +   u8 func_map;/* bit map. lsb is fn 0. */
>> +} pci_dev_dma_multi_func_sources[] = {
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9143, (1<<0)|(1<<1)},
>> +   { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
>> +   { 0 }
>> +};
>
>
> Adding another buggy device.  I have a Ricoh multifunction device:
>
> 17:00.0 SD Host controller: Ricoh Co Ltd MMC/SD Host Controller (rev 01)
> 17:00.3 FireWire (IEEE 1394): Ricoh Co Ltd R5C832 PCIe IEEE 1394
> Controller (rev 01)
>
> 17:00.0 0805: 1180:e822 (rev 01)
> 17:00.3 0c00: 1180:e832 (rev 01)
>

The Ricoh device issue has been known for some time and a quirk has been
available since commit 12ea6cad1c7d046 in June 2012.  It's slightly
different than the problem this patch tries to work around [1].

> that adding entries for also fixed booting.  I don't have any SD cards or
firewire devices handy to test that they work, but the system now boots,
which was not the case without your patch and IOMMU/DMAR enabled.

That is really strange. Could you tell us what kernel version you tested
and provide dmesg output?

>  Here's a previous patch used for similar hardware that may also be fixed
by this:
>
>
http://lists.fedoraproject.org/pipermail/scm-commits/2010-October/510785.html
>
> and another thread/bug report this may solve:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=605888

I believe this is referenced in drivers/pci/quirks.c for versions newer
than 3.5.

> Feel free to include me in any future iterations of this patch you'd like
tested.
>
> Tested-By: Pat Erley 
>

Thanks for testing!

[1] In the Ricoh case, multiple functions are used for real devices and the
bug is that these devices all use function 0 during DMA. In this particular
case, I'd expect the FireWire device 17:00.3 to issue DMA from the SD Host
Controller address 17:00.0. The quirk is not too much of a terrible hack -
it's a fairly simple translation.

In the Marvell case, the real device uses DMA source tags that don't
actually belong to any visible devices. The quirk to make this work is more
invasive, not nearly as elegant and has not attracted much enthusiasm from
subsystem maintainers, though I'm still hopeful that a quirk will be merged
in some form or another.

--

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

2013-03-08 Thread Andrew Cooks

On Fri, Mar 8, 2013 at 7:43 PM, Gaudenz Steinlin  wrote:
>
> Hi Andrew
>
> Andrew Cooks  writes:
>
>> This patch creates a quirk to allow the Intel IOMMU to be enabled for devices
>> that use incorrect tags during DMA. It is similar to the quirk for Ricoh
>> devices, but allows mapping multiple functions and mapping of 'ghost'
>> functions that do not correspond to real devices. Devices that need this
>> include a variety of Marvell 88SE91xx based SATA controllers. [1][2]
>
> I can confirm that this version of the patch also works for my mini-PCIe
> device (88NV9143). See the my mail about it for more information. I had
> to manually fix the patch because the patch utility did not understand
> it. There is a formatting error in the last hunk for quirks.c (missing
> space before context line) and the line count in the hunk header is
> wrong (66 lines changed should be 56 lines). I hope nothing was missing
> from the patch.
>
> Tested on 3.8.2.

Thanks for testing.

The formatting error is embarrassing. I was impatient and removed some
unused content from the patch, instead of cleaning the source. The
thing about posting to open lists with thousands or subscribers and
searchable archives is that it's impossible to hide incompetence.

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Marvell 88NV9143 in mini-PCIe not working with intel_iommu=on

2013-03-07 Thread Andrew Cooks

On Thu, Mar 7, 2013 at 5:25 PM, Gaudenz Steinlin  wrote:
>
> Hi Andrew
>
> Andrew Cooks  writes:
>
>> On Tue, Mar 5, 2013 at 11:03 PM, Gaudenz Steinlin  
>> wrote:
>>>
>>> [ Sending this to the MVUMI driver authors and the IOMMU list as I can't
>>> tell which part is at fault. ]
>>>
>>> [ ... ]
>>> [4.342079] dmar: DRHD: handling fault status reg 2
>>> [4.342132] dmar: DMAR:[DMA Read] Request device [02:00.0] fault addr 
>>> f000
>>> [4.342132] DMAR:[fault reason 02] Present bit in context entry is clear
>>> [ ... ]
>>> [   34.344078] mvumi :02:00.1: no handshake response at state 0x2.
>>> [   34.344115] mvumi :02:00.1: isr : global=0x0,status=0x0.
>>> [   34.344146] mvumi :02:00.1: handshake failed at state 0x2.
>>> [   34.344266] mvumi: probe of :02:00.1 failed with error -22
>>>
>>
>> Looks like another Marvell DMA source tag issue.
>
> You are probably right with this. See below.
>
>>
>>> And the full lspic output for this device:
>>>
>>> gaudenz@meteor:~$ sudo lspci -vv -nnq -s 02:
>>> 02:00.0 Mass storage controller [01ff]: Marvell Technology Group Ltd. 
>>> Device [1b4b:91f3]
>>> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
>>> ...
>>> Capabilities: [140 v1] Virtual Channel
>>> Caps:   LPEVC=0 RefClk=100ns PATEntryBits=1
>>> Arb:Fixed- WRR32- WRR64- WRR128-
>>> Ctrl:   ArbSelect=Fixed
>>> Status: InProgress-
>>> VC0:Caps:   PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
>>> Arb:Fixed- WRR32- WRR64- WRR128- TWRR128- 
>>> WRR256-
>>> Ctrl:   Enable+ ID=0 ArbSelect=Fixed TC/VC=01
>>> Status: NegoPending- InProgress-
>>>
>>> 02:00.1 Mass storage controller [0180]: Marvell Technology Group Ltd. 
>>> Device [1b4b:9143] (rev 10)
>>> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
>>> ...
>>> Kernel driver in use: mvumi
>>>
>>> 02:00.2 Non-Volatile memory controller [0108]: Marvell Technology Group 
>>> Ltd. Device [1b4b:91e3] (prog-if 01)
>>> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
>>>...
>>
>> In this case it seems like a multifunction device with 02:00.1 being
>> the only function that the mvumi driver cares about.  So my guess is
>> that 02:00.1 is issuing DMA with the incorrect tag of 02:00.0.
>>
>> Perhaps Alex Williamson can tell us about iommu device groups, whether
>> it would be possible to group the functions together automatically and
>> whether that would solve the problem. It should also be possible to
>> adapt the quirk patch I posted recently to handle this, but I'm still
>> waiting to hear if that patch has a future.
>
> I adapted your quirk patch to my device and it works.

Thanks for testing it!

>  As I'm very new to this I don't know if my modifications are right or if 
> there is a better
> way to do this.

Don't worry, I'm also pretty new here.

For this device, I think the quirk used for Ricoh devices that's
already in the mainline kernel would also work, because both function
0 and function 1 are known and the device only seems to use function
0. However, since it is another Marvell device, it may look out of
place in that table.

I'm hoping one of the veteran developers would give some guidance for
the best approach to enable more devices in future.

> Diff on top of the latest version of the quirk you
> posted to the iommu list:
>
I included this in v4 of the patch I posted, before I saw your message.

Thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

2013-03-07 Thread Andrew Cooks

This patch creates a quirk to allow the Intel IOMMU to be enabled for devices
that use incorrect tags during DMA. It is similar to the quirk for Ricoh
devices, but allows mapping multiple functions and mapping of 'ghost'
functions that do not correspond to real devices. Devices that need this
include a variety of Marvell 88SE91xx based SATA controllers. [1][2]

Changelog:
v4: Process feedback received from Alex Williamson.
 * don't assume function 0 is a real device.
 * exit early if no ghost functions are known, or all known functions have
   been mapped.
 * cleanup failure case so mapping succeeds or fails for all ghost functions
   per device.
 * improve comments.

v3:
 * Adopt David Woodhouse's terminology by referring to the quirky functions as
 'ghost' functions.
 * Unmap ghost functions when device is detached from IOMMU.
 * Stub function for when CONFIG_PCI_QUIRKS is not enabled.


 This patch was generated against 3.9-rc1, but will also apply to 3.7.10.

 Bug reports:
 1. https://bugzilla.redhat.com/show_bug.cgi?id=757166
 2. https://bugzilla.kernel.org/show_bug.cgi?id=42679

Signed-off-by: Andrew Cooks 
---
 drivers/iommu/intel-iommu.c |   69 +++
 drivers/pci/quirks.c|   67 +-
 include/linux/pci.h |5 +++
 include/linux/pci_ids.h |1 +
 4 files changed, 141 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0099667..f53f3e3 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1674,6 +1674,69 @@ static int domain_context_mapping_one(struct dmar_domain 
*domain, int segment,
return 0;
 }
 
+static void iommu_detach_dev(struct intel_iommu *iommu, u8 bus, u8 devfn);
+
+static void unmap_ghost_dma_fn(struct pci_dev *pdev, u8 fn_map)
+{
+   u8 fn;
+   struct intel_iommu *iommu;
+
+   iommu = device_to_iommu(pci_domain_nr(pdev->bus), pdev->bus->number,
+   pdev->devfn);
+
+   /* something must be seriously fubar if we can't lookup the iommu. */
+   BUG_ON(!iommu);
+
+   for (fn = 0; fn <= 7 && fn_map << fn; fn++) {
+   if (fn == PCI_FUNC(pdev->devfn))
+   continue;
+   if (fn_map & (1<bus->number,
+   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn));
+   dev_dbg(&pdev->dev, "quirk; ghost func %d unmapped",
+   fn);
+   }
+   }
+}
+
+/* For quirky devices like Marvell 88SE91xx chips that use ghost functions. */
+static int map_ghost_dma_fn(struct dmar_domain *domain,
+   struct pci_dev *pdev,
+   int translation)
+{
+   u8 fn, fn_map;
+   u8 fn_mapped = 0;
+   int err = 0;
+
+   fn_map = pci_get_dma_source_map(pdev);
+
+   /* this is the common, non-quirky case. */
+   if (!fn_map)
+   return 0;
+
+   for (fn = 0; fn <= 7 && fn_map << fn; fn++) {
+   if (fn == PCI_FUNC(pdev->devfn))
+   continue;
+   if (fn_map & (1<bus),
+   pdev->bus->number,
+   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
+   translation);
+   if (err) {
+   dev_err(&pdev->dev,
+   "mapping ghost func %d failed", fn);
+   unmap_ghost_dma_fn(pdev, fn_mapped);
+   return err;
+   }
+   dev_dbg(&pdev->dev, "quirk; ghost func %d mapped", fn);
+   fn_mapped |= (1<bus, info->devfn);
iommu_detach_dependent_devices(iommu, pdev);
+   unmap_ghost_dma_fn(pdev, pci_get_dma_source_map(pdev));
free_devinfo_mem(info);
 
spin_lock_irqsave(&device_domain_lock, flags);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0369fb6..cf00acb 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3249,6 +3249,10 @@ static struct pci_dev *pci_func_0_dma_source(struct 
pci_dev *dev)
return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
+/* Table of source functions for real devices. The DMA requests for the
+ * device are tagged with a different real function as source. This is
+ * relevant to multifunction devices.
+ */
 static const struct pci_dev_dma_source {
u16 vendor;
u16 device;
@@ -3275,7 +3279,8 @@ static const struct pci_dev_dma_source {
  * the device doing the DMA, but sometimes hardware is broken and will
  * tag the DMA as being sou

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

2013-03-05 Thread Andrew Cooks

On Wed, Mar 6, 2013 at 12:04 PM, Alex Williamson
 wrote:
> On Fri, 2013-03-01 at 16:26 +0800, Andrew Cooks wrote:
>
>> +
>> + for (fn = 1; fn < 8; fn++) {
>
> Wouldn't you want to do 0 to 7, then add:
>
> if (fn == PCI_FUNC(pdev->devfn))
> continue;
>
> You could also get more creative with the loop using shifts and exit
> when the remaining map is 0.

Thanks, I'll use a shift instead.

Up to now I've assumed that function 0 will always be the real device
and that function 1-7 may be ghost functions, but as we saw in the
case of the Marvell 88NV9143 in the Super Talent CoreStore MV 64GB
mini-PCIe SSD, that assumption is probably wrong.

To handle the case where the real device is function 1 and function 0
needs to be mapped as a ghost function, would it be acceptable to
iterate over 0 to 7 and let domain_context_mapping_one take care of
preventing duplicates, or should I duplicate the
device_to_context_entry and context_present function calls?

>
>> + if (fn_map & (1 << fn)) {
>> + err = domain_context_mapping_one(domain,
>> + pci_domain_nr(pdev->bus),
>> + pdev->bus->number,
>> + PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
>> + translation);
>> + if (err)
>> + return err;
>
> I'd be worried that there's missing cleanup here, what if you were
> mapping multiple ghost functions and the 2nd one failed, leaving one
> attached?

I don't understand the failure cases sufficiently, but I understand
that it's better to have all mappings succeed or fail together. Will
fix it.

>> +/* Table of multiple (ghost) source functions. This is similar to the
>> + * translated sources above, but with the following differences:
>> + * 1. the device may use multiple functions as DMA sources,
>> + * 2. these functions cannot be assumed to be actual devices,
>> + * 3. the specific ghost function for a request can not be exactly 
>> predicted.
>> + * The bitmap only contains the additional quirk functions.
>> + */
>> +static const struct pci_dev_dma_multi_func_sources {
>> + u16 vendor;
>> + u16 device;
>> + u8 func_map;/* bit map. lsb is fn 0. */
>> +} pci_dev_dma_multi_func_sources[] = {
>> + { PCI_VENDOR_ID_MARVELL_2, 0x9123, (1<<0)|(1<<1)},
>> + { PCI_VENDOR_ID_MARVELL_2, 0x9125, (1<<0)|(1<<1)},
>> + { PCI_VENDOR_ID_MARVELL_2, 0x9128, (1<<0)|(1<<1)},
>> + { PCI_VENDOR_ID_MARVELL_2, 0x9130, (1<<0)|(1<<1)},
>> + { PCI_VENDOR_ID_MARVELL_2, 0x9172, (1<<0)|(1<<1)},
>
> Links to bug reports in the comments might be useful for future
> workarounds.  I'm also not sure what you mean in the 3rd bullet, the
> non-ghost function of some of these is sometimes 0, sometimes 1?  And
> the ghost function is the other?

The ghost function is the one that doesn't correspond to an actual
device, but the actual device could be either 0 or 1 and it could use
both 0 and 1 for different requests, with no obvious way to tell when
it will use 0 and when it will use 1. I'll reword the bullet.

> Skipping fn 0 above, I assume all
> cases are fn 0 exists and fn 1 is the ghost function, right?  If so,
> then we probably only want bit 1 set.  I'm afraid to ask whether there
> are configurations of this vendor/device that have a fn 1.

See comment about Marvell 88NV9143 above.

Thanks for reviewing!

a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Marvell 88NV9143 in mini-PCIe not working with intel_iommu=on

2013-03-05 Thread Andrew Cooks

On Tue, Mar 5, 2013 at 11:03 PM, Gaudenz Steinlin  wrote:
>
> [ Sending this to the MVUMI driver authors and the IOMMU list as I can't
> tell which part is at fault. ]
>
> [ ... ]
> [4.342079] dmar: DRHD: handling fault status reg 2
> [4.342132] dmar: DMAR:[DMA Read] Request device [02:00.0] fault addr 
> f000
> [4.342132] DMAR:[fault reason 02] Present bit in context entry is clear
> [ ... ]
> [   34.344078] mvumi :02:00.1: no handshake response at state 0x2.
> [   34.344115] mvumi :02:00.1: isr : global=0x0,status=0x0.
> [   34.344146] mvumi :02:00.1: handshake failed at state 0x2.
> [   34.344266] mvumi: probe of :02:00.1 failed with error -22
>

Looks like another Marvell DMA source tag issue.

> And the full lspic output for this device:
>
> gaudenz@meteor:~$ sudo lspci -vv -nnq -s 02:
> 02:00.0 Mass storage controller [01ff]: Marvell Technology Group Ltd. Device 
> [1b4b:91f3]
> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
> ...
> Capabilities: [140 v1] Virtual Channel
> Caps:   LPEVC=0 RefClk=100ns PATEntryBits=1
> Arb:Fixed- WRR32- WRR64- WRR128-
> Ctrl:   ArbSelect=Fixed
> Status: InProgress-
> VC0:Caps:   PATOffset=00 MaxTimeSlots=1 RejSnoopTrans-
> Arb:Fixed- WRR32- WRR64- WRR128- TWRR128- WRR256-
> Ctrl:   Enable+ ID=0 ArbSelect=Fixed TC/VC=01
> Status: NegoPending- InProgress-
>
> 02:00.1 Mass storage controller [0180]: Marvell Technology Group Ltd. Device 
> [1b4b:9143] (rev 10)
> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
> ...
> Kernel driver in use: mvumi
>
> 02:00.2 Non-Volatile memory controller [0108]: Marvell Technology Group Ltd. 
> Device [1b4b:91e3] (prog-if 01)
> Subsystem: Marvell Technology Group Ltd. Device [1b4b:9143]
>...

In this case it seems like a multifunction device with 02:00.1 being
the only function that the mvumi driver cares about.  So my guess is
that 02:00.1 is issuing DMA with the incorrect tag of 02:00.0.

Perhaps Alex Williamson can tell us about iommu device groups, whether
it would be possible to group the functions together automatically and
whether that would solve the problem. It should also be possible to
adapt the quirk patch I posted recently to handle this, but I'm still
waiting to hear if that patch has a future.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

2013-03-03 Thread Andrew Cooks

On Sat, Mar 2, 2013 at 7:18 AM, Justin Piszcz  wrote:
>
> Against a clean 3.7.10 (from ftp.kernel.org)
>
> # patch -p1 <
> ../patch/RFC-Fix-Intel-IOMMU-support-for-Marvell-88SE91xx-SATA-controllers..
> patch
> patching file drivers/iommu/intel-iommu.c
> patching file drivers/pci/quirks.c
> Hunk #1 succeeded at 3230 (offset 3 lines).
> patching file include/linux/pci.h
> # pwd
> /usr/src/linux-3.7.10
>

I've downloaded and patched the 3.7.10 tarball and still get the same
output I got before; different output from yours. I'm not sure the
patch is complete or applying correctly, are you?
Could you please check whether the patch you're applying is the same
as the attached file?

> Full dmesg with the patch applied: (but with IOMMU off)
> http://home.comcast.net/~jpiszcz/20130301/dmesg-full.txt
>
> Full dmesg (as much as possible through netconsole with IOMMU on)
> http://home.comcast.net/~jpiszcz/20130301/dmesg-iommu-on.txt
>
> Let me know if anything else is needed, thanks.

I think some important error messages might have been lost here.

You captured a complete dmesg at
https://home.comcast.net/~jpiszcz/20121128/dmesg.txt
with iommu on. Are you still able to get the same with 3.7.10 if you
exclude this patch, or has something else changed?

a.

marvell_ghost_funcs.patch
Description: Binary data
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

2013-03-01 Thread Andrew Cooks

On Sat, Mar 2, 2013 at 1:51 AM, Justin Piszcz  wrote:
>
>
> On Fri, Mar 1, 2013 at 3:26 AM, Andrew Cooks  wrote:
>>
>> This is my third submitted patch to make Marvell 88SE91xx SATA controllers
>> work when IOMMU is enabled.[1][2]
>>
>
> Hi,
>
> Against 3.7.10:
>
> # patch -p1 <
> ../RFC-Fix-Intel-IOMMU-support-for-Marvell-88SE91xx-SATA-controllers..patch
> patching file drivers/iommu/intel-iommu.c
> patching file drivers/pci/quirks.c
> Hunk #1 succeeded at 3230 (offset 3 lines).
> patching file include/linux/pci.h
> #

Thanks for testing!

Patching 3.7.10 looks somewhat different here. I'm using the
linux-3.7.y branch from
git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
at commit 356d8c6fb2a7cf49e836742738a8b9a47e77cfea. The output I get
is:

$ patch -p1 < 
~/devel/lk_patches_dma_source_maps-20130301/marvell_ghost_funcs.patch
patching file drivers/iommu/intel-iommu.c
Hunk #1 succeeded at 1672 (offset -2 lines).
Hunk #2 succeeded at 1729 (offset -2 lines).
Hunk #3 succeeded at 3833 (offset -2 lines).
patching file drivers/pci/quirks.c
Hunk #1 succeeded at 3210 (offset -39 lines).
Hunk #2 succeeded at 3240 (offset -39 lines).
Hunk #3 succeeded at 3258 (offset -39 lines).
patching file include/linux/pci.h
Hunk #1 succeeded at 1546 (offset -32 lines).
Hunk #2 succeeded at 1555 (offset -32 lines).
patching file include/linux/pci_ids.h

>
> Recompile kernel, reboot..
>
> Shutdown host, re-attach to Marvell Controller w/IOMMU.
>
> The host still failed to boot, dmesg/panic here:
> http://home.comcast.net/~jpiszcz/20130301/boot_failure.JPG
>
> (The root disk is /dev/sdc)

Sorry it doesn't work for you, but I can't really tell what's failing
from the photo you posted. I'm running 3.7.10 with this patch right
now.

According to the dmesg output you posted at
https://home.comcast.net/~jpiszcz/20121128/dmesg.txt and the lspci
output in http://www.kernelhub.org/?p=2&msg=171877, you've run into
two separate DMAR issues:

Problem A - nvidia graphics:
[2.968248] dmar: DRHD: handling fault status reg 202
[2.968253] dmar: DMAR:[DMA Read] Request device [04:00.0] fault addr 0
[2.968253] DMAR:[fault reason 06] PTE Read access is not set

Problem B - Marvell 88SE9123:
[2.974534] ata9: SATA link down (SStatus 0 SControl 300)
[2.976297] ata7: SATA link down (SStatus 0 SControl 300)
[2.977939] ata13: SATA link down (SStatus 0 SControl 300)
[2.979689] ata14: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
[2.981419] dmar: DRHD: handling fault status reg 2
[2.981431] ata8: SATA link down (SStatus 0 SControl 300)
[2.981449] ata11: SATA link down (SStatus 0 SControl 300)
[2.988479] dmar: DMAR:[DMA Read] Request device [84:00.1] fault
addr fff0
[2.988479] DMAR:[fault reason 02] Present bit in context entry is clear

Can you capture a full dmesg with this patch applied?

--
a.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

2013-03-01 Thread Andrew Cooks

This is my third submitted patch to make Marvell 88SE91xx SATA controllers work 
when IOMMU is enabled.[1][2]

What's changed:
* Adopt David Woodhouse's terminology by referring to the quirky functions as 
'ghost' functions.
* Unmap ghost functions when device is detached from IOMMU.
* Stub function for when CONFIG_PCI_QUIRKS is not enabled.

The bad:
* Still no AMD support.
* The table of affected chip IDs is as complete as I can make it by googling 
for bug reports.

This patch was generated against commit 
b0af9cd9aab60ceb17d3ebabb9fdf4ff0a99cf50, but will also apply cleanly to 3.7.10.

Bug reports:
1. https://bugzilla.redhat.com/show_bug.cgi?id=757166
2. https://bugzilla.kernel.org/show_bug.cgi?id=42679

Signed-off-by: Andrew Cooks 
---
 drivers/iommu/intel-iommu.c |   50 +++
 drivers/pci/quirks.c|   47 +++-
 include/linux/pci.h |5 
 include/linux/pci_ids.h |1 +
 4 files changed, 102 insertions(+), 1 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0099667..13323f2 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1674,6 +1674,50 @@ static int domain_context_mapping_one(struct dmar_domain 
*domain, int segment,
return 0;
 }
 
+/* For quirky devices like Marvell 88SE91xx chips that use ghost functions. */
+static int map_ghost_dma_fn(struct dmar_domain *domain,
+   struct pci_dev *pdev,
+   int translation)
+{
+   u8 fn, fn_map;
+   int err = 0;
+
+   fn_map = pci_get_dma_source_map(pdev);
+
+   for (fn = 1; fn < 8; fn++) {
+   if (fn_map & (1 << fn)) {
+   err = domain_context_mapping_one(domain,
+   pci_domain_nr(pdev->bus),
+   pdev->bus->number,
+   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
+   translation);
+   if (err)
+   return err;
+   dev_dbg(&pdev->dev, "dma quirk; func %d mapped", fn);
+   }
+   }
+   return 0;
+}
+
+static void iommu_detach_dev(struct intel_iommu *iommu, u8 bus, u8 devfn);
+
+static void unmap_ghost_dma_fn(struct intel_iommu *iommu,
+   struct pci_dev *pdev)
+{
+   u8 fn, fn_map;
+
+   fn_map = pci_get_dma_source_map(pdev);
+
+   for (fn = 1; fn < 8; fn++) {
+   if (fn_map & (1 << fn)) {
+   iommu_detach_dev(iommu,
+   pdev->bus->number,
+   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn));
+   dev_dbg(&pdev->dev, "dma quirk; func %d unmapped", fn);
+   }
+   }
+}
+
 static int
 domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
int translation)
@@ -1687,6 +1731,11 @@ domain_context_mapping(struct dmar_domain *domain, 
struct pci_dev *pdev,
if (ret)
return ret;
 
+   /* quirk for undeclared/ghost pci functions */
+   ret = map_ghost_dma_fn(domain, pdev, translation);
+   if (ret)
+   return ret;
+
/* dependent device mapping */
tmp = pci_find_upstream_pcie_bridge(pdev);
if (!tmp)
@@ -3786,6 +3835,7 @@ static void domain_remove_one_dev_info(struct dmar_domain 
*domain,
iommu_disable_dev_iotlb(info);
iommu_detach_dev(iommu, info->bus, info->devfn);
iommu_detach_dependent_devices(iommu, pdev);
+   unmap_ghost_dma_fn(iommu, pdev);
free_devinfo_mem(info);
 
spin_lock_irqsave(&device_domain_lock, flags);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 0369fb6..d311100 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3249,6 +3249,10 @@ static struct pci_dev *pci_func_0_dma_source(struct 
pci_dev *dev)
return pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
 }
 
+/* Table of source functions for real devices. The DMA requests for the
+ * device are tagged with a different real function as source. This is
+ * relevant to multifunction devices.
+ */
 static const struct pci_dev_dma_source {
u16 vendor;
u16 device;
@@ -3275,7 +3279,8 @@ static const struct pci_dev_dma_source {
  * the device doing the DMA, but sometimes hardware is broken and will
  * tag the DMA as being sourced from a different device.  This function
  * allows that translation.  Note that the reference count of the
- * returned device is incremented on all paths.
+ * returned device is incremented on all paths. Translation is done when
+ * the de

Re: [RFC PATCH] Fix Intel IOMMU support for Marvell 88SE91xx SATA controllers.

2013-02-25 Thread Andrew Cooks

On Sat, Feb 23, 2013 at 3:29 AM, Stijn Tintel  wrote:
> On 19-12-12 11:58, Andrew Cooks wrote:
>> This is my second attempt to make Marvell 88SE91xx SATA controllers work 
>> when IOMMU is enabled.[1][2]
>> As suggested, it no longer tries to add support for phantom functions.
>>
>> What's missing:
>> * No AMD support. I need some help with this.
>> * Table of affected chip IDs is incomplete. I think 0x9123, 0x9125, 0x9128 
>> are also affected.
> This one is also affected: 08:00.0 0106: 1b4b:9130 (rev 11), this is in
> dmesg:
>
> [1.914086] dmar: DMAR:[DMA Read] Request device [08:00.1] fault addr
> fff0

Handling specific chip revisions is another issue I'm not sure how to
handle. We could use another bitmap field, but we need more vendor
cooperation to know exactly which chip model and revision combinations
are affected.

>>
>> Patch is against 3.7.1
>>
>> Review and feedback would be appreciated.
> I changed your patch to check for my chip ID, and it solves my problem:
> no more hang during boot, and the HDD connected to this controller is
> now detected with IOMMU enabled.
>
> Also see 1 minor comment inline.

Thanks for the feedback!  I'll include your PCI_VENDOR_ID change in
the next iteration of the patch.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH] Fix Intel IOMMU support for Marvell 88SE91xx SATA controllers.

2012-12-19 Thread Andrew Cooks

This is my second attempt to make Marvell 88SE91xx SATA controllers work when 
IOMMU is enabled.[1][2] 
As suggested, it no longer tries to add support for phantom functions.

What's missing:
* No AMD support. I need some help with this.
* Table of affected chip IDs is incomplete. I think 0x9123, 0x9125, 0x9128 are 
also affected.

Patch is against 3.7.1

Review and feedback would be appreciated.

1. https://bugzilla.redhat.com/show_bug.cgi?id=757166
2. https://bugzilla.kernel.org/show_bug.cgi?id=42679

Signed-off-by: Andrew Cooks 
---
 drivers/iommu/intel-iommu.c |   36 ++--
 drivers/pci/quirks.c|   34 ++
 include/linux/pci.h |1 +
 3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0badfa4..17e64c0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1672,6 +1674,31 @@ static int domain_context_mapping_one(struct dmar_domain 
*domain, int segment,
return 0;
 }
 
+/* For buggy devices like Marvell 88SE91xx chips that use unclaimed
+ * functions.
+ */
+static int map_quirky_dma_fn(struct dmar_domain *domain,
+   struct pci_dev *pdev,
+   int translation)
+{
+   u8 fn;
+   int err = 0;
+
+   for (fn = 1; fn < 8; fn++) {
+   if (pci_func_is_dma_source(pdev, fn)) {
+   err = domain_context_mapping_one(domain,
+   pci_domain_nr(pdev->bus),
+   pdev->bus->number,
+   PCI_DEVFN(PCI_SLOT(pdev->devfn), fn),
+   translation);
+   if (err)
+   return err;
+   dev_dbg(&pdev->dev, "func: %d mapped dma quirk", fn);
+   }
+   }
+   return 0;
+}
+
 static int
 domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
int translation)
@@ -1685,6 +1712,11 @@ domain_context_mapping(struct dmar_domain *domain, 
struct pci_dev *pdev,
if (ret)
return ret;
 
+   /* quirk for undeclared pci functions */
+   ret = map_quirky_dma_fn(domain, pdev, translation);
+   if (ret)
+   return ret;
+
/* dependent device mapping */
tmp = pci_find_upstream_pcie_bridge(pdev);
if (!tmp)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 7a451ff..8d02bac 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3227,6 +3227,40 @@ static const struct pci_dev_dma_source {
{ 0 }
 };
 
+static const struct pci_dev_dma_source_funcs {
+   u16 vendor;
+   u16 device;
+   u8 func_map;/* bit map. lsb is fn 0. */
+} pci_dev_dma_source_funcs[] = {
+{0x1b4b, 0x9172, (1<<0)|(1<<1)},
+{ 0 }, 
+};
+
+static u8 pci_get_dma_source_map(struct pci_dev *dev)
+{
+   const struct pci_dev_dma_source_funcs *i;
+
+   for (i = pci_dev_dma_source_funcs; i->func_map; i++) {
+   if ((i->vendor == dev->vendor ||
+i->vendor == (u16)PCI_ANY_ID) &&
+   (i->device == dev->device ||
+i->device == (u16)PCI_ANY_ID)) {
+   return i->func_map; 
+   }
+   }
+   return 0;
+}
+
+int pci_func_is_dma_source(struct pci_dev *dev, int fn)
+{
+   u8 fn_map = pci_get_dma_source_map(dev);
+
+   if (fn_map & (1 << fn))
+   return 1;
+
+   return 0;
+}
+
 /*
  * IOMMUs with isolation capabilities need to be programmed with the
  * correct source ID of a device.  In most cases, the source ID matches
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ee21795..8f0fa7f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1546,6 +1546,7 @@ enum pci_fixup_pass {
 #ifdef CONFIG_PCI_QUIRKS
 void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev);
 struct pci_dev *pci_get_dma_source(struct pci_dev *dev);
+int pci_func_is_dma_source(struct pci_dev *dev, int fn);
 int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
 #else
 static inline void pci_fixup_device(enum pci_fixup_pass pass,
-- 
1.7.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] create context mapping entries for devices that use phantom functions.

2012-12-03 Thread Andrew Cooks

Create context mapping for phantom functions.

Signed-off-by: Andrew Cooks 
---
 drivers/iommu/intel-iommu.c |   30 ++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 554e6ac..b3c9b55 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1673,6 +1673,32 @@ static int domain_context_mapping_one(struct dmar_domain 
*domain, int segment,
 }
 
 static int
+context_map_phantoms(struct dmar_domain *domain, struct pci_dev *pdev,
+   int translation)
+{
+   unsigned fn;
+   int ret;
+
+   if (!pdev->phantoms_enabled)
+   return 0;
+
+   pr_debug("context_map_phantoms()\n");
+
+   for (fn = 1 ; fn < 8 ; fn++) {
+   ret = domain_context_mapping_one(domain,
+   pci_domain_nr(pdev->bus),
+   pdev->bus->number,
+   fn,
+   translation);
+   if (ret) {
+   pr_info("function %d phantom mapping failed.\n", fn);
+   return ret;
+   }
+   }
+   return 0;
+}
+
+static int
 domain_context_mapping(struct dmar_domain *domain, struct pci_dev *pdev,
int translation)
 {
@@ -1685,6 +1711,10 @@ domain_context_mapping(struct dmar_domain *domain, 
struct pci_dev *pdev,
if (ret)
return ret;
 
+   ret = context_map_phantoms(domain, pdev, translation);
+   if (ret)
+   return ret;
+
/* dependent device mapping */
tmp = pci_find_upstream_pcie_bridge(pdev);
if (!tmp)
-- 
1.7.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

DMA Remapping and Marvell 88SE9172, 88SE9128 SATA controllers

2012-08-27 Thread Andrew Cooks

There's a problem relating to DMA Remapping which affects systems with
Marvell SATA controllers. It was first reported on 2012-01-28 on
bugzilla.kernel.org[1] and is best described by Don Dutile: "...the
lspci dump in the bugzilla report doesn't show a device w/BDF=0b:00.1;
so, if the SATA device (which is 0b:00.0) is spitting out 0b:00.1 as
the source of any of its DMA packets, the IOMMU will fault on it,
since 0b:00.1 didn't request DMA mappings (0b:00.0 did)."[2]

Is this kind of problem caused by a missing/incorrect entry in an ACPI
table? Is it feasible to introduce a fake device for the missing
function using a pci quirk?

Thanks,

ac.

1. https://bugzilla.kernel.org/show_bug.cgi?id=42679
2. https://lists.linux-foundation.org/pipermail/iommu/2012-January/003552.html
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 00/16] PCI/iommu: Fix DMA alias problems

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

Re: [PATCH v2 00/15] PCI/iommu: Fix DMA alias problems

Re: [PATCH 03/13] PCI: quirk dma_func_alias for Ricoh devices

Re: Who understand the IOMMU code?

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

Re: [PATCH] Quirk for buggy dma source tags with Intel IOMMU.

Re: [RFC PATCH v2 1/2] pci: Create PCIe requester ID interface

Re: Ricoh DMAR bug returns? (WAS Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.)

Re: RFC: vfio / iommu driver for hardware with no iommu

Re: [PATCH 0/9] AMD IOMMU cleanups, fixes and IVRS bug workarounds

Ricoh DMAR bug returns? (WAS Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.)

Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

Re: [PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

Re: Marvell 88NV9143 in mini-PCIe not working with intel_iommu=on

[PATCH v4] Quirk for buggy dma source tags with Intel IOMMU.

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

Re: Marvell 88NV9143 in mini-PCIe not working with intel_iommu=on

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

Re: [PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

[PATCH] Quirk to support Marvell 88SE91xx SATA controllers with Intel IOMMU.

Re: [RFC PATCH] Fix Intel IOMMU support for Marvell 88SE91xx SATA controllers.

[RFC PATCH] Fix Intel IOMMU support for Marvell 88SE91xx SATA controllers.

[PATCH 4/4] create context mapping entries for devices that use phantom functions.

DMA Remapping and Marvell 88SE9172, 88SE9128 SATA controllers

28 matches

Site Navigation

Mail list logo

Footer information