Hi Julien
> On 17 Dec 2021, at 2:32 pm, Julien Grall <[email protected]> wrote:
>
> 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é <[email protected]> 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.
Yes you are right.
>
> 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
I am also thinking to change the misx_read/write to use a pointer to address to
avoid change in {read,write}{b,w,l,q}
If everyone is ok I will send the next version to modify the same.
Regards,
Rahul
> 2) Modify the x86 read*/write* helpers to forbid any access other than
> pointer.
>
> Cheers,
>
> --
> Julien Grall
