[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Julien Grall <julien@xxxxxxx>
  • From: Rahul Singh <Rahul.Singh@xxxxxxx>
  • Date: Fri, 17 Dec 2021 14:58:10 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=r9H+v4v13vyVx3PBnO9+FeLGyfSE/g37ryQFf6eq9rw=; b=Y8d87kG1n7L2vjEViINafvAf2iN56SEA1BGF2N8GoTmdJYDlrBatnGT/uoEPS2+K1yB4mlyDydPfUiBYvuY4hO0TE/kJg01whTRmyJ8PgVfG8/ymS/kuFnCnNejgApRFNzs3pw18IUcX7FpX9vgJCe1UwaAvDBqglfMrjhRRs9O1JsBN4gDLOU1QNdwxj/MKrq4Yv/0YVBrZwqvdijXhFZAikMvTqRXekxFpCWfjIavmxi1Yc/0ZHayhDDMUdTkByU8YT9aW9P6ENLc7PMdffqTvDzG2cRR0O/3DedW97W49Q0wVA7+zQdfhBZPLfPqD+T5pK83Dx9/ilZ/Y4pxlhw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=S+kYfuWtKHXt1yHSVE5VePQVR4o2XZn8NlZwvepYZeHq+kk6N/HgWdIrNcgVGsJq5KryAAMPQsKN7I6fXsQwsTvmedYf6uJVsapF5I11jIBQZjUWsoYGruPked5v5PUFFTa/A7qV/+sSbrGHIqtoSw7t8wmjrEUguWYYsB+MZuTSCX4Gd3Hs1gdtfNHB8ysogV0g1zX8S6HxK1oQBHysBqbnFvu2vNY3dCTkHCChWcD7Zm6mkPuF7HbgYwuahxv+T5mCM5Hkk8Bt11QEN5y1gIjXxZ1m1vzVfiHFMWj4jw1AlNH6xTw1f0Srfj48D+i3os2K/Ak9606N4+KE2PmdvA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Jan Beulich <jbeulich@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • Delivery-date: Fri, 17 Dec 2021 14:58:42 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHX8NfN625eD/1es0eaHkpOvTmQ3Kwx7OWAgAL9+QCAAAwcgIAAK1kAgAGhwICAAAc/gA==
  • Thread-topic: [PATCH] xen/vpci: msix: move x86 specific code to x86 file

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.