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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

Thanks, Roger.