{read,write}{l,q} function argument is different for ARM and x86.
ARM {read,wrie}(l,q} function argument is pointer whereas X86
{read,wrie}(l,q} function argument is address itself.
{read,write}{l,q} is only used in common file to access the MSI-X PBA
structure. To avoid impacting other x86 code and to make the code common
move the read/write call to MSI-X PBA to arch specific file.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes since v1:
- Added in this version
---
xen/arch/x86/hvm/vmsi.c | 47 +++++++++++++++++++++++++++++++++++++++++
xen/drivers/vpci/msix.c | 43 ++-----------------------------------
xen/include/xen/vpci.h | 6 ++++++
3 files changed, 55 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/hvm/vmsi.c b/xen/arch/x86/hvm/vmsi.c
index 761ce674d7..f124a1d07d 100644
--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -1033,4 +1033,51 @@ void vpci_msix_arch_register(struct vpci_msix *msix, struct domain *d)
list_add(&msix->next, &d->arch.hvm.msix_tables);
}
+
+bool vpci_msix_arch_pba_read(unsigned long addr, unsigned int len,
+ unsigned long *data)
+{
+ /*
+ * Access to PBA.
+ *
+ * TODO: note that this relies on having the PBA identity mapped to the
+ * guest address space. If this changes the address will need to be
+ * translated.
+ */
+ switch ( len )
+ {
+ case 4:
+ *data = readl(addr);
+ break;
+
+ case 8:
+ *data = readq(addr);
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ break;
+ }
+
+ return true;
+}
+
+void vpci_msix_arch_pba_write(unsigned long addr, unsigned int len,
+ unsigned long data)
+{
+ switch ( len )
+ {
+ case 4:
+ writel(data, addr);
+ break;
+
+ case 8:
+ writeq(data, addr);
+ break;
+
+ default:
+ ASSERT_UNREACHABLE();
+ break;
+ }
+}
#endif /* CONFIG_HAS_VPCI */
diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 5b315757ef..b6720f1a1a 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -170,31 +170,7 @@ bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
return true;
if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
- {
- /*
- * Access to PBA.
- *
- * TODO: note that this relies on having the PBA identity mapped to the
- * guest address space. If this changes the address will need to be
- * translated.
- */
- switch ( len )
- {
- case 4:
- *data = readl(addr);
- break;
-
- case 8:
- *data = readq(addr);
- break;
-
- default:
- ASSERT_UNREACHABLE();
- break;
- }
-
- return true;
- }
+ return vpci_msix_arch_pba_read(addr, len, data);
spin_lock(&msix->pdev->vpci->lock);
entry = get_entry(msix, addr);
@@ -247,22 +223,7 @@ bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
{
/* Ignore writes to PBA for DomUs, it's behavior is undefined. */
if ( is_hardware_domain(d) )
- {
- switch ( len )
- {
- case 4:
- writel(data, addr);
- break;
-
- case 8:
- writeq(data, addr);
- break;
-
- default:
- ASSERT_UNREACHABLE();
- break;
- }
- }
+ vpci_msix_arch_pba_write(addr, len, data);
return true;
}
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 1c36845abf..a61daf9d53 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -231,6 +231,12 @@ bool vpci_msix_write(struct vpci_msix *msix, unsigned long addr,
bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
unsigned int len, unsigned long *data);
+bool vpci_msix_arch_pba_read(unsigned long addr, unsigned int len,
+ unsigned long *data);
+
+void vpci_msix_arch_pba_write(unsigned long addr, unsigned int len,
+ unsigned long data);
+
#endif /* __XEN__ */
#else /* !CONFIG_HAS_VPCI */
--
2.25.1
On 15.02.2022 16:25, Rahul Singh wrote:
> {read,write}{l,q} function argument is different for ARM and x86.
> ARM {read,wrie}(l,q} function argument is pointer whereas X86
> {read,wrie}(l,q} function argument is address itself.
I'm afraid I don't follow: x86 has
#define readl(x) (*(volatile uint32_t *)(x))
#define readq(x) (*(volatile uint64_t *)(x))
That's no different from Arm64:
#define readl(c) ({ u32 __v = readl_relaxed(c); __iormb(); __v; })
#define readl_relaxed(c) ({ u32 __v = le32_to_cpu((__force __le32)__raw_readl(c)); __v; })
static inline u32 __raw_readl(const volatile void __iomem *addr)
The difference is whether the address is expressed as a pointer, or
_may_ also be expressed as unsigned long. IOW the x86 variant is
perfectly fine to be passed e.g. a void * (preferably qualified
appropriately). The conversion from unsigned long to a pointer type
is actually expressed ...
> @@ -170,31 +170,7 @@ bool vpci_msix_read(struct vpci_msix *msix, unsigned long addr,
> return true;
>
> if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
> - {
> - /*
> - * Access to PBA.
> - *
> - * TODO: note that this relies on having the PBA identity mapped to the
> - * guest address space. If this changes the address will need to be
> - * translated.
> - */
> - switch ( len )
> - {
> - case 4:
> - *data = readl(addr);
> - break;
> -
> - case 8:
> - *data = readq(addr);
> - break;
> -
> - default:
> - ASSERT_UNREACHABLE();
> - break;
> - }
... in the comment ahead of this switch() (and the assumption is likely
wrong for DomU).
But then, Roger: What "identity mapped" is meant here? Surely not GVA ->
GPA, but rather GPA -> HPA? The address here is a guest physical one,
but read{l,q}() act on (host) virtual addresses. This would have been
easier to notice as wrong if read{l,q}() weren't allowing unsigned long
arguments to be passed to them.
Jan
On Tue, Feb 15, 2022 at 03:25:18PM +0000, Rahul Singh wrote:
> {read,write}{l,q} function argument is different for ARM and x86.
> ARM {read,wrie}(l,q} function argument is pointer whereas X86
> {read,wrie}(l,q} function argument is address itself.
>
> {read,write}{l,q} is only used in common file to access the MSI-X PBA
> structure. To avoid impacting other x86 code and to make the code common
> move the read/write call to MSI-X PBA to arch specific file.
I think we agreed where going to unify {read,write}{l,q} so they could
be used in arch-agnostic code?
Thanks, Roger.
Hi Roger,
> On 25 Feb 2022, at 8:20 am, Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Tue, Feb 15, 2022 at 03:25:18PM +0000, Rahul Singh wrote:
>> {read,write}{l,q} function argument is different for ARM and x86.
>> ARM {read,wrie}(l,q} function argument is pointer whereas X86
>> {read,wrie}(l,q} function argument is address itself.
>>
>> {read,write}{l,q} is only used in common file to access the MSI-X PBA
>> structure. To avoid impacting other x86 code and to make the code common
>> move the read/write call to MSI-X PBA to arch specific file.
>
> I think we agreed where going to unify {read,write}{l,q} so they could
> be used in arch-agnostic code?
We agreed to modify the vPCI MSIx code to use a pointer, but that not
helped me to make code arch-agnostic. I decided to move the PBA handling code to an
arch-specific file to make the code usable.
Thanks for the series "vpci/msix: fix PBA acceses” series that will help to use the
code for ARM arch also without any modification to {read,write}{l,q} .
Regards,
Rahul
>
> Thanks, Roger.
>
© 2016 - 2024 Red Hat, Inc.