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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.12.2021 15:32, Julien Grall wrote:
> 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:
>>>>> 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.

Okay, first of all I need to clean up some confusion cause by Rahul
saying "pointer to the address": That's where my "extra level of
indirection" came from. I would really wish one wouldn't need to go
to the code and verify such basic statements. There's no "pointer
to the address" here. The question is whether the argument has to
be a pointer (Arm) or is convertable to a pointer (x86). Therefore
...

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

... I'd suggest to go with 1), to avoid impacting other x86 code.
Longer term I wouldn't mind switching to 2) (unless vPCI really is
the only place using non-pointer arguments, in which case doing
the 2nd step right away [but still in a separate patch] would seem
quite reasonable).

Jan