[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] xen/vpci: msix: move x86 specific code to x86 file



Hi Jan,

On 16/12/2021 13:37, Jan Beulich wrote:
On 16.12.2021 12:01, Roger Pau Monné wrote:
On Thu, Dec 16, 2021 at 10:18:32AM +0000, Rahul Singh wrote:
Hi Roger,

Thanks for reviewing the code.

On 14 Dec 2021, at 12:37 pm, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

On Tue, Dec 14, 2021 at 10:45:17AM +0000, Rahul Singh wrote:
+              unsigned long *data)
{
-    const struct domain *d = v->domain;
-    struct vpci_msix *msix = msix_find(d, addr);
     const struct vpci_msix_entry *entry;
     unsigned int offset;

     *data = ~0ul;

     if ( !msix )
-        return X86EMUL_RETRY;
+        return VPCI_EMUL_RETRY;

     if ( !access_allowed(msix->pdev, addr, len) )
-        return X86EMUL_OKAY;
+        return VPCI_EMUL_OKAY;

     if ( VMSIX_ADDR_IN_RANGE(addr, msix->pdev->vpci, VPCI_MSIX_PBA) )
     {
@@ -210,11 +194,11 @@ static int msix_read(struct vcpu *v, unsigned long addr, 
unsigned int len,
         switch ( len )
         {
         case 4:
-            *data = readl(addr);
+            *data = vpci_arch_readl(addr);

Why do you need a vpci wrapper around the read/write handlers? AFAICT
arm64 also has {read,write}{l,q}. And you likely want to protect the
64bit read with CONFIG_64BIT if this code is to be made available to
arm32.

I need the wrapper because {read,write}{l,q} function argument is different for 
ARM and x86.
ARM {read,wrie}(l,q}  function argument is pointer to the address whereas X86  
{read,wrie}(l,q}
function argument is address itself.

Oh, that's a shame. I don't think there's a need to tag those helpers
with the vpci_ prefix though. Could we maybe introduce
bus_{read,write}{b,w,l,q} helpers that take the same parameters on all
arches?

It would be even better to fix the current ones so they take the same
parameters on x86 and Arm, but that would mean changing all the call
places in one of the arches.

Yet still: +1 for removing the extra level of indirection. Imo these
trivial helpers should never have diverged between arches; I have
always been under the impression that on Linux they can be used by
arch-independent code (or else drivers would be quite hard to write).

So technically both helpers are able to cope with pointer. The x86 one is also allowing to pass an address.

From a brief look at the x86, it looks like most of the users are using a pointer. However, the vPCI msix code is one example where addresses are passed.

AFAICT, the read*/write* helpers on Linux only works with pointers. So I think the actions should be:
   1) Modify the vPCI MSIx code to use pointer
2) Modify the x86 read*/write* helpers to forbid any access other than pointer.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.