Re: [PATCH V3] pci: quirk: Apply APM ACS quirk to XGene devices

[Alex Williamson]

On Mon, 31 Jul 2017 22:35:41 -0700
Feng Kan <fkan@xxxxxxx> wrote:

> On Mon, Jul 31, 2017 at 2:55 PM, Alex Williamson
> <alex.williamson@xxxxxxxxxx> wrote:
> > On Mon, 31 Jul 2017 10:56:53 -0700
> > Feng Kan <fkan@xxxxxxx> wrote:
> >  
> >> On Fri, Jul 28, 2017 at 4:00 PM, Alex Williamson
> >> <alex.williamson@xxxxxxxxxx> wrote:  
> >> > On Fri, 28 Jul 2017 11:50:43 -0700
> >> > Feng Kan <fkan@xxxxxxx> wrote:
> >> >  
> >> >> The APM X-Gene PCIe root port does not support ACS at this point.
> >> >> However, the hw provides isolation and source validation through
> >> >> the SMMU. The stream ID generated by the PCIe ports contain both
> >> >> the BDF as well as the port ID in its 3 most significant bits.
> >> >> Turn on ACS but disable all the peer to peer features.
> >> >>
> >> >> Signed-off-by: Feng Kan <fkan@xxxxxxx>
> >> >> ---
> >> >>       V3 Change: Add comment regarding unique port id in stream ID
> >> >>       V2 Change: Move XGene ACS quirk to unique XGene function
> >> >>
> >> >>  drivers/pci/quirks.c | 15 +++++++++++++++
> >> >>  1 file changed, 15 insertions(+)
> >> >>
> >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> >> index 085fb78..0f8f1cd 100644
> >> >> --- a/drivers/pci/quirks.c
> >> >> +++ b/drivers/pci/quirks.c
> >> >> @@ -4120,6 +4120,19 @@ static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags)
> >> >>       return acs_flags ? 0 : 1;
> >> >>  }
> >> >>
> >> >> +static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags)
> >> >> +{
> >> >> +     /*
> >> >> +      * XGene root matching this quirk do not allow peer-to-peer
> >> >> +      * transactions with others, allowing masking out these bits as if they
> >> >> +      * were unimplemented in the ACS capability.
> >> >> +      */
> >> >> +     acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR |
> >> >> +                    PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT);
> >> >> +
> >> >> +     return acs_flags ? 0 : 1;
> >> >> +}
> >> >> +
> >> >>  /*
> >> >>   * Many Intel PCH root ports do provide ACS-like features to disable peer
> >> >>   * transactions and validate bus numbers in requests, but do not provide an
> >> >> @@ -4368,6 +4381,8 @@ static int pci_quirk_mf_endpoint_acs(struct pci_dev *dev, u16 acs_flags)
> >> >>       { 0x10df, 0x720, pci_quirk_mf_endpoint_acs }, /* Emulex Skyhawk-R */
> >> >>       /* Cavium ThunderX */
> >> >>       { PCI_VENDOR_ID_CAVIUM, PCI_ANY_ID, pci_quirk_cavium_acs },
> >> >> +     /* APM XGene */
> >> >> +     { PCI_VENDOR_ID_AMCC, 0xE004, pci_quirk_xgene_acs },
> >> >>       { 0 }
> >> >>  };
> >> >>  
> >> >
> >> > Hi Feng,
> >> >
> >> > Sorry, I have one more question as I happened to spend some time
> >> > looking at PCI_ACS_DT this week.  DT specifies that peer-to-peer should
> >> > occur normally between egress ports for transactions which are
> >> > pre-translated by an ATS unit on the endpoint.  Therefore if a root
> >> > port doesn't allow peer-to-peer, it seems like it should not claim to
> >> > support PCI_ACS_DT.  I know your quirk is just a copy of the Cavium
> >> > one, but we should also go back and verify this question with them, or
> >> > perhaps I'm misinterpreting this capability.  AIUI this is also a
> >> > performance capability, not an isolation capability, so it shouldn't
> >> > affect the ability to consider a device isolated, it only gets
> >> > confusing if we expect a performance benefit from this but don't
> >> > actually see one.  Does your root port have this ability to
> >> > selectively allow peer-to-peer of pre-translated transactions?  Thanks,  
> >> We do not support peer to peer between root ports (each root port is a
> >> root complex in itself).
> >> Therefore, this bit set/unset has no effect.
> >>
> >> As one of our guys pointed out in PCIe 3.1a, it may help address your
> >> concern above.
> >>
> >> """
> >> Root Port indication of ACS Direct Translated P2P support does not imply any
> >> particular level of peer-to-peer support by the Root Complex, or that
> >> peer-to-peer traffic is supported at all.
> >> """  
> >
> > That's interesting, but is that referring to the ACS capability or the
> > control?  
> 
> 
> I could imagine how advertising the capability would not
> > imply any particular level of p2p, but enabling the control (which is
> > what we're claiming via this quirk) the spec states:
> >
> >   When ACS Direct Translated P2P is enabled in a Downstream Port,
> >   peer-to-peer Memory Requests whose Address Type (AT) field indicates
> >   a Translated address must be routed normally (âdirectlyâ) to the peer
> >   Egress Port, regardless of ACS P2P Request Redirect and ACS P2P
> >   Egress Control settings. All other peer-to-peer Requests must still
> >   be subject to ACS P2P Request Redirect and ACS P2P Egress Control
> >   settings.
> >
> > Note the *must* phrasing.  Furthermore, 7.16.3:
> >
> >   ACS Direct Translated P2P Enable (T) â When Set, overrides the ACS
> >   P2P Request Redirect and ACS P2P Egress Control mechanisms with
> >   peer-to-peer Memory Requests whose Address Translation (AT) field
> >   indicates a Translated address (see Section 6.12.3).
> >
> >   This bit is ignored if ACS Translation Blocking Enable (B) is 1b.
> >
> >   Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> >   Direct Translated P2P functionality is not implemented.
> >
> > Note the 2nd sentence, DT is ignored if TB is 1, which you're also
> > claiming is enabled with the mask in the patch above.  So the
> > functionality is not implemented in hardware, exposing the control as
> > enabled seems to violate a must condition in the spec, and all is for
> > naught because the bit combination causes it to be ignored anyway.
> >
> > That leads to the question of why the patch above advertises
> > PCI_ACS_TB, where the same section as above describes:
> >
> >   ACS Translation Blocking Enable (B) â When Set, the component blocks
> >   all Upstream Memory Requests whose Address Translation (AT) field is
> >   not set to the default value.
> >
> >   Default value of this bit is 0b. Must be hardwired to 0b if the ACS
> >   Translation Blocking functionality is not implemented.
> >
> > Does the root port block transactions with the AT field set to a
> > non-default value?  
> We do not support ATS.

But if transactions with the AT field set to non-default are not
blocked, I would discourage advertising TB.
 
>  It seems that this would effectively nullify ATS on
> > a downstream endpoint.  Perhaps the endpoint would not enable ATS if
> > PCI_ACS_TB is enabled on a parent downstream port.
> >
> > All of this seems to stem from Cavium incorrectly stealing the ACS bit
> > mask from the multifunction quirk.  TB is included in that mask because
> > the spec indicates multifunction devices _must_not_ implement that
> > capability.  Same for SV and UF, note the comment in
> > pci_quirk_mf_endpoint_acs().  We include those in order to clear flags
> > that are either not relevant to those endpoints or we're claiming
> > are covered by lack or p2p between functions.  It's all a simplication
> > for the caller.  It's possible that the multifunction quirk exposing DT
> > is incorrect here in general.  Note that in the call path for these
> > quirks we're not testing what the device is only capable of, we're
> > testing what the device has effectively enabled, ie.
> > pci_dev_specific_acs_enabled().  Thanks,  
> I don't think I understood all your points, let me summarize and
> please correct me
> if I am wrong. I should not set DT since it overrides RR and EC. Also,
> TB should not
> be set since if so it would negate DT. So by setting those bits in the
> mask I am
> implying incorrect configuration?

Yes, your root port does not seem to have the behavior to justify
claiming either TB or DT and there's a conflict between the two where
TB overrides DT anyway.
 
> I believe the set of flags in the mask based on what you have
> indicated above should be:
> u16 flags = (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_SV);
> 
> If you are okay with the setting here, I will submit a new patch.

Yes, given what you've described as no peer-to-peer support and some
degree of source validation via the port ID component of the stream ID,
I think these are the appropriate set of ACS equivalent enables to
claim.  And it's also a sufficient set to support isolation for device
assignment.  Thanks,

Alex