Re: [PATCH v2 3/3] xen/vpci: msix: move read/write call to MSI-X PBA entry to arch file

[Jan Beulich <jbeulich@xxxxxxxx>]

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