On Tue, Nov 26, 2024 at 06:11:12PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Nov 26, 2024 at 11:00:09AM +0100, Niklas Cassel wrote:
> > On Tue, Nov 26, 2024 at 09:55:23AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Nov 25, 2024 at 02:17:04PM -0500, Frank Li wrote:
> > > > On Sun, Nov 24, 2024 at 01:26:45PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Sat, Nov 16, 2024 at 09:40:44AM -0500, Frank Li wrote:
> > > > > > Add three registers: doorbell_bar, doorbell_addr, and doorbell_data,
> > > > > > along with doorbell_done. Use pci_epf_alloc_doorbell() to allocate a
> > > > >
> > > > > I don't see 'doorbell_done' defined anywhere.
> > > > >
> > > > > > doorbell address space.
> > > > > >
> > > > > > Enable the Root Complex (RC) side driver to trigger pci-epc-test's doorbell
> > > > > > callback handler by writing doorbell_data to the mapped doorbell_bar's
> > > > > > address space.
> > > > > >
> > > > > > Set doorbell_done in the doorbell callback to indicate completion.
> > > > > >
> > > > >
> > > > > Same here.
> > > > >
> > > > > > To avoid broken compatibility, add new command COMMAND_ENABLE_DOORBELL
> > > > >
> > > > > 'avoid breaking compatibility between host and endpoint,...'
> > > > >
> > > > > > and COMMAND_DISABLE_DOORBELL. Host side need send COMMAND_ENABLE_DOORBELL
> > > > > > to map one bar's inbound address to MSI space. the command
> > > > > > COMMAND_DISABLE_DOORBELL to recovery original inbound address mapping.
> > > > > >
> > > > > > Host side new driver Host side old driver
> > > > > >
> > > > > > EP: new driver S F
> > > > > > EP: old driver F F
> > > > >
> > > > > So the last case of old EP and host drivers will fail?
> > > >
> > > > doorbell test will fail if old EP.
> > > >
> > >
> > > How come there would be doorbell test if it is an old host driver?
> >
> > I also don't understand this.
> >
> > The new commands: DOORBELL_ENABLE / DOORBELL_DISABLE
> > can only be sent if there is a new host driver.
> >
> > Sending DOORBELL_ENABLE / DOORBELL_DISABLE will obviously
> > return "Invalid command" if the EP driver is old.
> >
> > If EP driver is new, DOORBELL_ENABLE will only return success if the SoC
> > has support for GIC ITS and has configured DTS with msi-parent
> > (i.e. if the pci_epf_alloc_doorbell() call was successful).
> >
> >
> > >
> > > > >
> > > > > >
> > > > > > S: If EP side support MSI, 'pcitest -B' return success.
> > > > > > If EP side doesn't support MSI, the same to 'F'.
> > > > > >
> > > > > > F: 'pcitest -B' return failure, other case as usual.
> > > > > >
> > > > > > Tested-by: Niklas Cassel <cassel@xxxxxxxxxx>
> > > > > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > > > > ---
> > > > > > Change from v7 to v8
> > > > > > - rename to pci_epf_align_inbound_addr_lo_hi()
> > > > > >
> > > > > > Change from v6 to v7
> > > > > > - use help function pci_epf_align_addr_lo_hi()
> > > > > >
> > > > > > Change from v5 to v6
> > > > > > - rename doorbell_addr to doorbell_offset
> > > > > >
> > > > > > Chagne from v4 to v5
> > > > > > - Add doorbell free at unbind function.
> > > > > > - Move msi irq handler to here to more complex user case, such as differece
> > > > > > doorbell can use difference handler function.
> > > > > > - Add Niklas's code to handle fixed bar's case. If need add your signed-off
> > > > > > tag or co-developer tag, please let me know.
> > > > > >
> > > > > > change from v3 to v4
> > > > > > - remove revid requirement
> > > > > > - Add command COMMAND_ENABLE_DOORBELL and COMMAND_DISABLE_DOORBELL.
> > > > > > - call pci_epc_set_bar() to map inbound address to MSI space only at
> > > > > > COMMAND_ENABLE_DOORBELL.
> > > > > > ---
> > > > > > drivers/pci/endpoint/functions/pci-epf-test.c | 117 ++++++++++++++++++++++++++
> > > > > > 1 file changed, 117 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > index ef6677f34116e..410b2f4bb7ce7 100644
> > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > > > > > @@ -11,12 +11,14 @@
> > > > > > #include <linux/dmaengine.h>
> > > > > > #include <linux/io.h>
> > > > > > #include <linux/module.h>
> > > > > > +#include <linux/msi.h>
> > > > > > #include <linux/slab.h>
> > > > > > #include <linux/pci_ids.h>
> > > > > > #include <linux/random.h>
> > > > > >
> > > > > > #include <linux/pci-epc.h>
> > > > > > #include <linux/pci-epf.h>
> > > > > > +#include <linux/pci-ep-msi.h>
> > > > > > #include <linux/pci_regs.h>
> > > > > >
> > > > > > #define IRQ_TYPE_INTX 0
> > > > > > @@ -29,6 +31,8 @@
> > > > > > #define COMMAND_READ BIT(3)
> > > > > > #define COMMAND_WRITE BIT(4)
> > > > > > #define COMMAND_COPY BIT(5)
> > > > > > +#define COMMAND_ENABLE_DOORBELL BIT(6)
> > > > > > +#define COMMAND_DISABLE_DOORBELL BIT(7)
> > > > > >
> > > > > > #define STATUS_READ_SUCCESS BIT(0)
> > > > > > #define STATUS_READ_FAIL BIT(1)
> > > > > > @@ -39,6 +43,11 @@
> > > > > > #define STATUS_IRQ_RAISED BIT(6)
> > > > > > #define STATUS_SRC_ADDR_INVALID BIT(7)
> > > > > > #define STATUS_DST_ADDR_INVALID BIT(8)
> > > > > > +#define STATUS_DOORBELL_SUCCESS BIT(9)
> > > > > > +#define STATUS_DOORBELL_ENABLE_SUCCESS BIT(10)
> > > > > > +#define STATUS_DOORBELL_ENABLE_FAIL BIT(11)
> > > > > > +#define STATUS_DOORBELL_DISABLE_SUCCESS BIT(12)
> > > > > > +#define STATUS_DOORBELL_DISABLE_FAIL BIT(13)
> > > > > >
> > > > > > #define FLAG_USE_DMA BIT(0)
> > > > > >
> > > > > > @@ -74,6 +83,9 @@ struct pci_epf_test_reg {
> > > > > > u32 irq_type;
> > > > > > u32 irq_number;
> > > > > > u32 flags;
> > > > > > + u32 doorbell_bar;
> > > > > > + u32 doorbell_offset;
> > > > > > + u32 doorbell_data;
> > > > > > } __packed;
> > > > > >
> > > > > > static struct pci_epf_header test_header = {
> > > > > > @@ -642,6 +654,63 @@ static void pci_epf_test_raise_irq(struct pci_epf_test *epf_test,
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +static void pci_epf_enable_doorbell(struct pci_epf_test *epf_test, struct pci_epf_test_reg *reg)
> > > > > > +{
> > > > > > + enum pci_barno bar = reg->doorbell_bar;
> > > > > > + struct pci_epf *epf = epf_test->epf;
> > > > > > + struct pci_epc *epc = epf->epc;
> > > > > > + struct pci_epf_bar db_bar;
> > > > >
> > > > > db_bar = {};
> > > > >
> > > > > > + struct msi_msg *msg;
> > > > > > + size_t offset;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (bar < BAR_0 || bar == epf_test->test_reg_bar || !epf->db_msg) {
> > > > >
> > > > > What is the need of BAR check here and below? pci_epf_alloc_doorbell() should've
> > > > > allocated proper BAR already.
> > > >
> > > > Not check it at call pci_epf_alloc_doorbell() because it optional feature.
> > >
> > > What is 'optional feature' here? allocating doorbell?
> > >
> > > > It return failure when it actually use it.
> > > >
> > >
> > > So host can call pci_epf_enable_doorbell() without pci_epf_alloc_doorbell()?
> >
> > This patch calls pci_epf_alloc_doorbell() in pci_epf_test_bind(), so at
> > .bind() time.
> >
> > DOORBELL_ENABLE and DOORBELL_DISABLE are two new commands, so the host driver
> > could theoretically send these even if pci_epf_alloc_doorbell() failed.
> >
> >
> > pci_epf_test_cmd_handler() additions looks like this:
> >
> > + case COMMAND_ENABLE_DOORBELL:
> > + pci_epf_enable_doorbell(epf_test, reg);
> > + pci_epf_test_raise_irq(epf_test, reg);
> > + break;
> > + case COMMAND_DISABLE_DOORBELL:
> > + pci_epf_disable_doorbell(epf_test, reg);
> > + pci_epf_test_raise_irq(epf_test, reg);
> > + break;
> >
> > so they will call pci_epf_enable_doorbell()/pci_epf_disable_doorbell()
> > unconditionally, without any check to see if the doorbell was allocated.
> >
> > We could move the was doorbell allocated check (if (!epf->db_msg)) to
> > pci_epf_test_cmd_handler(), but that would make pci_epf_test_cmd_handler()
> > more messy, so personally I think it is fine to keep the doorbell allocated
> > check in pci_epf_enable_doorbell()/pci_epf_disable_doorbell().
> >
> >
> > I did earlier suggest to Frank to move the pci_epf_alloc_doorbell() call
> > to pci_epf_enable_doorbell():
> > https://lore.kernel.org/linux-pci/Zy02mPTvaPAFFxGi@ryzen/
> >
> > His reply is here::
> > https://lore.kernel.org/linux-pci/Zy1CxtKSgRuEPX5A@lizhi-Precision-Tower-5810/
> >
> > "it may be too frequent to allocate and free msi resources when call
> > pci_epf_enable_doorbell()/pci_epf_disable_doorbell()."
> >
> > I don't think that is a good argument, as presumably (in the normal case) an
> > EPF driver will enable doorbell in the beginning, and then keep it enabled.
> >
> > However, one point could be that pci-epf-test currently does all allocations
> > (the allocations for the backing memory) in .bind(), so in one way it makes
> > sense to also allocate the doorbell in .bind().
> >
> > To play devil's advocate, I guess you could argue that doorbell feature is
> > optional, while allocating backing memory for BARs is not, so it makes sense
> > that they are not allocated at the same time.
> >
>
> I like the idea of calling pci_epf_alloc_doorbell() in
> pci_epf_{enable/disable}_doorbell() APIs. And as you said, it doesn't make sense
> to call these APIs too frequently.
I not sure what's you means and direction for next version.
Ideally, host driver should use doorbell for every command because it will
avoid EP's side polling reg change.
It still is problem to waste a bar to do doorbell. Ideally, we can append
doorbell ITS register after test bar, which require map two segmemt memory
region to bar0.
This patch just go first step. If we can append to ITS to bar0 in future,
pci_epf_alloc_doorbell() will become more reasonable at bind() function.
Frank
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
