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

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

Jan