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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan

> On 21 Dec 2021, at 7:41 am, Jan Beulich <jbeulich@xxxxxxxx> 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.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”:

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
>