Hi Jan
> On 21 Dec 2021, at 7:41 am, Jan Beulich <jbeul...@suse.com> wrote:
>
> 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....@citrix.com>
>>>>>> 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”:
Sorry for the confusion.
> 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).
I will choose option 1 as of now to avoid any x86 specific change to
{read,write}{b,w,l,q}.
Regards,
Rahul
> Jan
>
