From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrioshnan Subject: Re: [PATCH V2] ns16550: Add support for UART present in Broadcom TruManage capable NetXtreme chips Date: Tue, 19 Nov 2013 10:40:21 -0600 Message-ID: <528B9475.9080809@amd.com> References: <1384450802-1870-1-git-send-email-Aravind.Gopalakrishnan@amd.com> <5285F7F40200007800103710@nat28.tlf.novell.com> <52865F3D.8000207@amd.com> <5289D8130200007800103E48@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3811764645832046031==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VioLp-0008Ut-39 for xen-devel@lists.xenproject.org; Tue, 19 Nov 2013 16:40:33 +0000 In-Reply-To: <5289D8130200007800103E48@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Thomas Lendacky , keir@xen.org, andrew.cooper3@citrix.com, SuraveeSuthikulpanit , sherry.hurwitz@amd.com, xen-devel , shurd@broadcom.com List-Id: xen-devel@lists.xenproject.org --===============3811764645832046031== Content-Type: multipart/alternative; boundary="------------040002070704050505090402" --------------040002070704050505090402 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 11/18/2013 2:04 AM, Jan Beulich wrote: >>>>>> On 14.11.13 at 18:40, Aravind Gopalakrishnan wrote: >>>> static struct ns16550 { >>>> - int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq; >>>> + int baud, clock_hz, data_bits, parity, stop_bits, irq; >>>> u64 io_base; /* I/O port or memory-mapped I/O address. */ >>>> + u32 fifo_size; >>> Is this in any way related to the main purpose of the patch here? >>> And if there is (i.e. in response to Andrew's comment on v1), then >>> I don't see why most/all of the other fields shouldn't follow suit. >> I don't mind changing them all in one patch.. But - >> >> Since it is not directly related to the main purpose of the patch, would >> it be okay >> if I did this in a follow up patch and go back to using 'int' for now? > "Go back" is probably the wrong term - in the new code you add > you should strive to use unsigned int where possible. And in a > second patch, converting the bogus uses of plain int to unsigned > would be desirable. Okay, Will do.. >>>> /* Not IO */ >>>> if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) ) >>>> - continue; >>>> + { >>>> + vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID); >>>> + device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID); >>>> + >>>> + /* Check for quirks in uart_config lookup table */ >>>> + for ( i = 0; i < ARRAY_SIZE(uart_config); i++) >>>> + { >>>> + if ( uart_config[i].vendor_id != vendor ) >>>> + continue; >>>> + >>>> + if ( uart_config[i].dev_id != device ) >>>> + continue; >>>> + >>>> + if ( bar_idx >= uart_config[i].max_bars ) >>>> + break; >>> Did you not mean "continue" here? >> No.. break is fine here since we only support bar0 for this specific >> device.. >> (By the time we reach this condition, we have already verified 'vendor' >> and 'device' from >> uart_config table anyway.. it is needless to iterate through other >> devices in the table) > Here you make assumptions on the single entry you know the list > is currently holding. But you should consider the general case, > and I don't see why there couldn't be two flavors of a device > having the same (vendor, device) pair but different max_bars. Okay, I'll make it 'continue' here too.. Although, to differentaite between two 'flavors' of device having same (vendor, device) id's, It might be worthwhile(in the future) to add a new field in 'uart_config' table that clearly identifies the flavors.. >>>> + /* Not 8 bytes */ >>>> + if ( (len & 0xffff) != 0xfff9 ) >>>> + continue; >>> I think this size checking actually should also be done in the MMIO >>> case. >> This check would not work for this device as the length of region is 64K. > I didn't mean you to make the exact same check; but I do expect > you to do some similar checking. > >> But if we really want to force a length check for MMIO cases as well, >> we can define another field in struct uart_config and verify if length >> matches.. >> Do let me know what you make of it.. > If you think an exact match check is necessary, then yes, such a > new field might be needed. But since I don't see Xen depending > on the exact size, but just on it being at least a certain size, doing > just that check would seem fine to me. Okay, Will fix this. > But then you'd need to carefully consider what the remainder of > the MMIO range is used for - if any of that could conflict with > what Xen needs to fully control the device, then you should also > hide any PAGE_SIZE chunks from all domains (including Dom0). > (Unfortunately we can't currently hide sub-page chunks in an > effective manner.) With respect to this, I am not seeing any untoward side effects to Xen from letting the code run as-is. I have tested the patch with the Broadcom 5725 UART chip and a IO based UART as well and both seem to function fine.. I did try using 'iomem_deny_access' to hide the MMIO range from dom0. But I don't see an effect. Not sure if I am missing something or is there a different way to hide PAGE_SIZE chunks? I will spin out a V3 now as you can verify the changes I make.. Thanks, -Aravind. --------------040002070704050505090402 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 11/18/2013 2:04 AM, Jan Beulich wrote:
On 14.11.13 at 18:40, Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com> wrote:

  static struct ns16550 {
-    int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
+    int baud, clock_hz, data_bits, parity, stop_bits, irq;
      u64 io_base;   /* I/O port or memory-mapped I/O address. */
+    u32 fifo_size;

Is this in any way related to the main purpose of the patch here?
And if there is (i.e. in response to Andrew's comment on v1), then
I don't see why most/all of the other fields shouldn't follow suit.

I don't mind changing them all in one patch.. But -

Since it is not directly related to the main purpose of the patch, would 
it be okay
if I did this in a follow up patch and go back to using 'int' for now?

"Go back" is probably the wrong term - in the new code you add
you should strive to use unsigned int where possible. And in a
second patch, converting the bogus uses of plain int to unsigned
would be desirable.

Okay, Will do..


      

        

          

            
                  /* Not IO */
                  if ( !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
-                    continue;
+                {
+                    vendor = pci_conf_read16(0, b, d, f, PCI_VENDOR_ID);
+                    device = pci_conf_read16(0, b, d, f, PCI_DEVICE_ID);
+
+                    /* Check for quirks in uart_config lookup table */
+                    for ( i = 0; i < ARRAY_SIZE(uart_config); i++)
+                    {
+                        if ( uart_config[i].vendor_id != vendor )
+                            continue;
+
+                        if ( uart_config[i].dev_id != device )
+                            continue;
+
+                        if ( bar_idx >= uart_config[i].max_bars )
+                            break;


          

          
Did you not mean "continue" here?


        

        
No.. break is fine here since we only support bar0 for this specific 
device..
(By the time we reach this condition, we have already verified 'vendor' 
and 'device' from
uart_config table anyway.. it is needless to iterate through other 
devices in the table)


      

      
Here you make assumptions on the single entry you know the list
is currently holding. But you should consider the general case,
and I don't see why there couldn't be two flavors of a device
having the same (vendor, device) pair but different max_bars.


    

    

    Okay, I'll make it 'continue' here too.. 

    

    Although, to differentaite between two 'flavors' of device having
    same (vendor, device) id's, 

    It might be worthwhile(in the future) to add a new field in
    'uart_config' table that clearly identifies the flavors..

    

    

      

      

        

          

            
+                    /* Not 8 bytes */
+                    if ( (len & 0xffff) != 0xfff9 )
+                        continue;


          

          
I think this size checking actually should also be done in the MMIO
case.


        

        
This check would not work for this device as the length of region is 64K.


      

      
I didn't mean you to make the exact same check; but I do expect
you to do some similar checking.



      

        
But if we really want to force a length check for MMIO cases as well,
we can define another field in struct uart_config and verify if length 
matches..
Do let me know what you make of it..


      

      
If you think an exact match check is necessary, then yes, such a
new field might be needed. But since I don't see Xen depending
on the exact size, but just on it being at least a certain size, doing
just that check would seem fine to me.


    

    

    Okay, Will fix this.

    

    

      
But then you'd need to carefully consider what the remainder of
the MMIO range is used for - if any of that could conflict with
what Xen needs to fully control the device, then you should also
hide any PAGE_SIZE chunks from all domains (including Dom0).
(Unfortunately we can't currently hide sub-page chunks in an
effective manner.)


    

    

    With respect to this,

    I am not seeing any untoward side effects to Xen from letting the
    code run as-is.

    I have tested the patch with the Broadcom 5725 UART chip and a IO
    based UART as well

    and both seem to function fine..

    

    I did try using 'iomem_deny_access' to hide the MMIO range from
    dom0. But I don't see an effect.

    Not sure if I am missing something or is there a different way to
    hide PAGE_SIZE chunks?

    

    I will spin out a V3 now as you can verify the changes I make..

    

    
    
    
    
    
    
    
    
    
    Thanks,

    -Aravind.

  


--------------040002070704050505090402--


--===============3811764645832046031==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--===============3811764645832046031==--