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

[Julien Grall <julien@xxxxxxx>]

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