On Thu, Mar 7, 2024 at 8:31 PM Björn Töpel <bjorn@xxxxxxxxxx> wrote: > > Anup Patel <apatel@xxxxxxxxxxxxxxxx> writes: > > > On Wed, Mar 6, 2024 at 9:22 PM Björn Töpel <bjorn@xxxxxxxxxx> wrote: > >> > >> Anup Patel <apatel@xxxxxxxxxxxxxxxx> writes: > >> > >> > diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c > >> > new file mode 100644 > >> > index 000000000000..b2a25e011bb2 > >> > --- /dev/null > >> > +++ b/drivers/irqchip/irq-riscv-aplic-msi.c > >> > +static void aplic_msi_write_msg(struct irq_data *d, struct msi_msg *msg) > >> > +{ > >> > + unsigned int group_index, hart_index, guest_index, val; > >> > + struct aplic_priv *priv = irq_data_get_irq_chip_data(d); > >> > + struct aplic_msicfg *mc = &priv->msicfg; > >> > + phys_addr_t tppn, tbppn, msg_addr; > >> > + void __iomem *target; > >> > + > >> > + /* For zeroed MSI, simply write zero into the target register */ > >> > + if (!msg->address_hi && !msg->address_lo && !msg->data) { > >> > + target = priv->regs + APLIC_TARGET_BASE; > >> > + target += (d->hwirq - 1) * sizeof(u32); > >> > + writel(0, target); > >> > >> Is the fence needed here (writel_relaxed())... > > > > The pci_write_msg_msix() (called via pci_msi_domain_write_msg()) > > uses writel() hence taking inspiration from that we use writel() over here > > as well. > > > > If that's wrong then pci_write_msg_msix() must be fixed as well. > > Huh? The writel()s in pci_write_msg_msix() are because there's an > ordering constraint, and code would be broken w/o it. My question was > "what are the ordering constraints for this piece of code", because it > looks like this is a single I/O write without any ordering constraints. Whatever ordering constraints apply to pci_write_msg_msix() also apply to APLIC MSI-mode because both create the leaf-level IRQ domain for the client device driver (PCIe or Platform device) whose parent is IMSIC base domain. > > I'm not a fan of sprinkling fences around "to be safe", but I don't want > to delay the v16 because of it. It can be fixed later, if it's not > needed. I don't think there is a clear way of proving that using write_relaxed() in aplic_msi_write_msg() is safe considering there is a vast variety of platform drivers who would be clients of the APLIC MSI-mode domain. I agree that we should deal with this later. Regards, Anup