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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien

> On 17 Dec 2021, at 2:32 pm, Julien Grall <julien@xxxxxxx> 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é <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.

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