| Message ID | 1450098032-21198-6-git-send-email-sshukla@mvista.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 3A9AC8E94; Mon, 14 Dec 2015 14:00:59 +0100 (CET) Received: from mail-pf0-f176.google.com (mail-pf0-f176.google.com [209.85.192.176]) by dpdk.org (Postfix) with ESMTP id 622B38E93 for <dev@dpdk.org>; Mon, 14 Dec 2015 14:00:58 +0100 (CET) Received: by pff63 with SMTP id 63so11003646pff.2 for <dev@dpdk.org>; Mon, 14 Dec 2015 05:00:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mvista-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=SVbpPbnhKCBFJXd/qfh/8E8u1km7AaB8iM2WLwNBgZ8=; b=UJdUE41G5RxC1lLJRpq8HZLga0GmKxjwlfUA+QmkIXU+4sV7wpOz1rOv3LU7sQ2klt oJ4bru+gGWog7uJG0X5jczyFZB2zeAaKqJF751YArQxdxgMNYETh2cBpgi33YIEeiSNJ zT24bTYaxWNiS3sd3ozofb8oWxxDUhmdK3GOILDJo1xuiXushAtTFPCKwlht4ms0Z7Gm 4WqRnGDd0FejmL5ISW3g47ypQOkDgDtwXqk8O0rkGynq2u71P3TWUCU+vOhrnwi9Dm5U Iuv4DuHzqJez71yiTyyNij/KPuZg2d+iUPJ9R4ZJIf7Ku0UJj+QpnqH41BX4wq/tVXLZ HHxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=SVbpPbnhKCBFJXd/qfh/8E8u1km7AaB8iM2WLwNBgZ8=; b=b+QPeuikkoovPerSKAn3ZEWazJcg4NBrpg2WZs+8vHmp0SAU8PuG1v1XTg3cPeYW8o kf3J6ZKW09hQKs5bMfwLm3iIb9wV6B7wclXmJRCztVdaQzb3z8LjpPX5BDPRK2j+yCZC sviMCKhx7B4J3NgHg0HclCES3rg4C6NiU+iFUbRKtAr6KAAzS6myoaPDxdl0t+jCwDGR onN0Mw0wdOkJRxkTJBmQSmhNDLvkl5IO6ADAPQ/P5NSoeTfnUpCHleiJoNhMwVOdUMwD 0EN7rwTXHH34IE2G0ohvgwI3AWn5aMtglsvEcNFXvv1enQtNUQ+lckrd5v06OQog41Q7 O4Pg== X-Gm-Message-State: ALoCoQn/QUVdug6IAE5xH+kSIS3xnZpD0PyDO3+UEAG0UCPmJqiSoDgkS+WqNHevATqGAuO16ivkUq0fkWTMZE5Hc7UgWMjzvA== X-Received: by 10.98.10.195 with SMTP id 64mr1025809pfk.20.1450098057774; Mon, 14 Dec 2015 05:00:57 -0800 (PST) Received: from santosh-Latitude-E5530-non-vPro.mvista.com ([110.172.16.5]) by smtp.gmail.com with ESMTPSA id 9sm42506405pfn.51.2015.12.14.05.00.55 (version=TLS1_1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Mon, 14 Dec 2015 05:00:56 -0800 (PST) From: Santosh Shukla <sshukla@mvista.com> To: dev@dpdk.org Date: Mon, 14 Dec 2015 18:30:24 +0530 Message-Id: <1450098032-21198-6-git-send-email-sshukla@mvista.com> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1450098032-21198-1-git-send-email-sshukla@mvista.com> References: <1450098032-21198-1-git-send-email-sshukla@mvista.com> Subject: [dpdk-dev] [ [PATCH v2] 05/13] virtio: change io_base datatype from uint32_t to uint64_type X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Santosh Shukla
Dec. 14, 2015, 1 p.m. UTC
In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0
to ffff but in non-x86 case in particular arm64 it need to store more than 32
bit address so changing io_base datatype from 32 to 64.
Signed-off-by: Santosh Shukla <sshukla@mvista.com>
---
drivers/net/virtio/virtio_ethdev.c | 2 +-
drivers/net/virtio/virtio_pci.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
Comments
On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: > In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 > to ffff but in non-x86 case in particular arm64 it need to store more than 32 > bit address so changing io_base datatype from 32 to 64. > > Signed-off-by: Santosh Shukla <sshukla@mvista.com> > --- > drivers/net/virtio/virtio_ethdev.c | 2 +- > drivers/net/virtio/virtio_pci.h | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index d928339..620e0d4 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > return -1; > > hw->use_msix = virtio_has_msix(&pci_dev->addr); > - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; > + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() so that we could do the correct cast there, say cast it to uint32_t for X86, and uint64_t for others. --yliu > > /* Reset the device although not necessary at startup */ > vtpci_reset(hw); > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 3f4ff80..f3e4178 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -169,7 +169,7 @@ struct virtqueue; > > struct virtio_hw { > struct virtqueue *cvq; > - uint32_t io_base; > + uint64_t io_base; > uint32_t guest_features; > uint32_t max_tx_queues; > uint32_t max_rx_queues; > @@ -231,7 +231,7 @@ outl_p(unsigned int data, unsigned int port) > #endif > > #define VIRTIO_PCI_REG_ADDR(hw, reg) \ > - (unsigned short)((hw)->io_base + (reg)) > + (unsigned long)((hw)->io_base + (reg)) > > #define VIRTIO_READ_REG_1(hw, reg) \ > inb((VIRTIO_PCI_REG_ADDR((hw), (reg)))) > -- > 1.7.9.5
On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 >> bit address so changing io_base datatype from 32 to 64. >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> >> --- >> drivers/net/virtio/virtio_ethdev.c | 2 +- >> drivers/net/virtio/virtio_pci.h | 4 ++-- >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> index d928339..620e0d4 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) >> return -1; >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; > > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() > so that we could do the correct cast there, say cast it to uint32_t for > X86, and uint64_t for others. > Ok. This was deliberately done considering your 1.0 virtio spec patch do care for uint64_t types and in arm64 case, If I plan to use those future patches, IMO it make more sense to me keep it in uint64_t way; Also in x86 case max address could of type 0x1000-101f and so forth; changing data-type to uint64_t default wont effect such address, right? And hw->io_base by looking at virtio_pci.h function like inb/outb etc.. takes io_base address as unsigned long types which is arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower level rd/wr apis are taking care of data-types accordingly. > --yliu > >> >> /* Reset the device although not necessary at startup */ >> vtpci_reset(hw); >> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h >> index 3f4ff80..f3e4178 100644 >> --- a/drivers/net/virtio/virtio_pci.h >> +++ b/drivers/net/virtio/virtio_pci.h >> @@ -169,7 +169,7 @@ struct virtqueue; >> >> struct virtio_hw { >> struct virtqueue *cvq; >> - uint32_t io_base; >> + uint64_t io_base; >> uint32_t guest_features; >> uint32_t max_tx_queues; >> uint32_t max_rx_queues; >> @@ -231,7 +231,7 @@ outl_p(unsigned int data, unsigned int port) >> #endif >> >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ >> - (unsigned short)((hw)->io_base + (reg)) >> + (unsigned long)((hw)->io_base + (reg)) >> >> #define VIRTIO_READ_REG_1(hw, reg) \ >> inb((VIRTIO_PCI_REG_ADDR((hw), (reg)))) >> -- >> 1.7.9.5
On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: > On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: > >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 > >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 > >> bit address so changing io_base datatype from 32 to 64. > >> > >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> > >> --- > >> drivers/net/virtio/virtio_ethdev.c | 2 +- > >> drivers/net/virtio/virtio_pci.h | 4 ++-- > >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > >> index d928339..620e0d4 100644 > >> --- a/drivers/net/virtio/virtio_ethdev.c > >> +++ b/drivers/net/virtio/virtio_ethdev.c > >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > >> return -1; > >> > >> hw->use_msix = virtio_has_msix(&pci_dev->addr); > >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; > >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; > > > > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() > > so that we could do the correct cast there, say cast it to uint32_t for > > X86, and uint64_t for others. > > > > Ok. > > This was deliberately done considering your 1.0 virtio spec patch do > care for uint64_t types and in arm64 case, If I plan to use those > future patches, IMO it make more sense to me keep it in uint64_t way; I did different cast, 32 bit for legacy virtio pci device, and 64 bit for modern virtio pci device. > Also in x86 case max address could of type 0x1000-101f and so forth; > changing data-type to uint64_t default wont effect such address, > right? Right, but what's the harm of doing the right cast? :) > And hw->io_base by looking at virtio_pci.h function like > inb/outb etc.. takes io_base address as unsigned long types which is > arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower > level rd/wr apis are taking care of data-types accordingly. Didn't get it. inb/outb takes "unsigned short" arguments, but not "unsigned long". --yliu
On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: >> On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu >> <yuanhan.liu@linux.intel.com> wrote: >> > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: >> >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 >> >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 >> >> bit address so changing io_base datatype from 32 to 64. >> >> >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> >> >> --- >> >> drivers/net/virtio/virtio_ethdev.c | 2 +- >> >> drivers/net/virtio/virtio_pci.h | 4 ++-- >> >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> >> index d928339..620e0d4 100644 >> >> --- a/drivers/net/virtio/virtio_ethdev.c >> >> +++ b/drivers/net/virtio/virtio_ethdev.c >> >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) >> >> return -1; >> >> >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); >> >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> > >> > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() >> > so that we could do the correct cast there, say cast it to uint32_t for >> > X86, and uint64_t for others. >> > >> >> Ok. >> >> This was deliberately done considering your 1.0 virtio spec patch do >> care for uint64_t types and in arm64 case, If I plan to use those >> future patches, IMO it make more sense to me keep it in uint64_t way; > > I did different cast, 32 bit for legacy virtio pci device, and 64 bit > for modern virtio pci device. > >> Also in x86 case max address could of type 0x1000-101f and so forth; >> changing data-type to uint64_t default wont effect such address, >> right? > > Right, but what's the harm of doing the right cast? :) > Agree. >> And hw->io_base by looking at virtio_pci.h function like >> inb/outb etc.. takes io_base address as unsigned long types which is >> arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower >> level rd/wr apis are taking care of data-types accordingly. > > Didn't get it. inb/outb takes "unsigned short" arguments, but not > "unsigned long". > sys/io.h in x86 case using unsigned short int types.. include/asm-generic/io.h for arm64 using it unsigned long (from linux header files) In such case keeping #define VIRTIO_PCI_REG_ADDR(hw, reg) \ (unsigned short)((hw)->io_base + (reg)) would be x86 specific and what I thought and used in this patch is #define VIRTIO_PCI_REG_ADDR(hw, reg) \ (unsigned long)((hw)->io_base + (reg)) to avoid ifdef ARM or non-x86..clutter, I know data-type is not right fit for x86 sys/io.h but considering possible address inside hw->io_base, wont effect functionality and performance my any mean. That is why at virtio_ethdev_init() i choose to keep it in hw->io_base = (uint64_t) types. Otherwise I'll have to duplicate VIRTIO_PCI_REG_XXX definition for non-x86 case, Pl. suggest better alternative. Thanks > --yliu
On Wed, Dec 16, 2015 at 08:09:40PM +0530, Santosh Shukla wrote: > On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: > >> On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu > >> <yuanhan.liu@linux.intel.com> wrote: > >> > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: > >> >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 > >> >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 > >> >> bit address so changing io_base datatype from 32 to 64. > >> >> > >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> > >> >> --- > >> >> drivers/net/virtio/virtio_ethdev.c | 2 +- > >> >> drivers/net/virtio/virtio_pci.h | 4 ++-- > >> >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > >> >> index d928339..620e0d4 100644 > >> >> --- a/drivers/net/virtio/virtio_ethdev.c > >> >> +++ b/drivers/net/virtio/virtio_ethdev.c > >> >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > >> >> return -1; > >> >> > >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); > >> >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; > >> >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; > >> > > >> > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() > >> > so that we could do the correct cast there, say cast it to uint32_t for > >> > X86, and uint64_t for others. > >> > > >> > >> Ok. > >> > >> This was deliberately done considering your 1.0 virtio spec patch do > >> care for uint64_t types and in arm64 case, If I plan to use those > >> future patches, IMO it make more sense to me keep it in uint64_t way; > > > > I did different cast, 32 bit for legacy virtio pci device, and 64 bit > > for modern virtio pci device. > > > >> Also in x86 case max address could of type 0x1000-101f and so forth; > >> changing data-type to uint64_t default wont effect such address, > >> right? > > > > Right, but what's the harm of doing the right cast? :) > > > > Agree. > > >> And hw->io_base by looking at virtio_pci.h function like > >> inb/outb etc.. takes io_base address as unsigned long types which is > >> arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower > >> level rd/wr apis are taking care of data-types accordingly. > > > > Didn't get it. inb/outb takes "unsigned short" arguments, but not > > "unsigned long". > > > > sys/io.h in x86 case using unsigned short int types.. > > include/asm-generic/io.h for arm64 using it unsigned long (from linux > header files) > > In such case keeping > #define VIRTIO_PCI_REG_ADDR(hw, reg) \ > (unsigned short)((hw)->io_base + (reg)) > > would be x86 specific and what I thought and used in this patch is > > #define VIRTIO_PCI_REG_ADDR(hw, reg) \ > (unsigned long)((hw)->io_base + (reg)) > > to avoid ifdef ARM or non-x86..clutter, I know data-type is not right > fit for x86 sys/io.h but considering possible address inside > hw->io_base, wont effect functionality and performance my any mean. > That is why at virtio_ethdev_init() i choose to keep it in hw->io_base > = (uint64_t) types. > > Otherwise I'll have to duplicate VIRTIO_PCI_REG_XXX definition for > non-x86 case, Pl. suggest better alternative. Thanks My understanding is that if you have done the right cast in the first time (at the io_base assignment), casting from a short type to a longer type will not matter: the upper bits will be filled with zero. So, I guess we are fine here. I'm thinking that the extra cast in VIRTIO_PCI_REG_ADDR() is not necessary, as C will do the right cast for different inb(), say cast it to "unsigned short" for x86, and "unsigned long" for your arm implementation. The same to other io helpers. --yliu
On Wed, Dec 16, 2015 at 8:28 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > On Wed, Dec 16, 2015 at 08:09:40PM +0530, Santosh Shukla wrote: >> On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu >> <yuanhan.liu@linux.intel.com> wrote: >> > On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: >> >> On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu >> >> <yuanhan.liu@linux.intel.com> wrote: >> >> > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: >> >> >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 >> >> >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 >> >> >> bit address so changing io_base datatype from 32 to 64. >> >> >> >> >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> >> >> >> --- >> >> >> drivers/net/virtio/virtio_ethdev.c | 2 +- >> >> >> drivers/net/virtio/virtio_pci.h | 4 ++-- >> >> >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> >> >> >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> >> >> index d928339..620e0d4 100644 >> >> >> --- a/drivers/net/virtio/virtio_ethdev.c >> >> >> +++ b/drivers/net/virtio/virtio_ethdev.c >> >> >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) >> >> >> return -1; >> >> >> >> >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); >> >> >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> >> >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> >> > >> >> > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() >> >> > so that we could do the correct cast there, say cast it to uint32_t for >> >> > X86, and uint64_t for others. >> >> > >> >> >> >> Ok. >> >> >> >> This was deliberately done considering your 1.0 virtio spec patch do >> >> care for uint64_t types and in arm64 case, If I plan to use those >> >> future patches, IMO it make more sense to me keep it in uint64_t way; >> > >> > I did different cast, 32 bit for legacy virtio pci device, and 64 bit >> > for modern virtio pci device. >> > >> >> Also in x86 case max address could of type 0x1000-101f and so forth; >> >> changing data-type to uint64_t default wont effect such address, >> >> right? >> > >> > Right, but what's the harm of doing the right cast? :) >> > >> >> Agree. >> >> >> And hw->io_base by looking at virtio_pci.h function like >> >> inb/outb etc.. takes io_base address as unsigned long types which is >> >> arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower >> >> level rd/wr apis are taking care of data-types accordingly. >> > >> > Didn't get it. inb/outb takes "unsigned short" arguments, but not >> > "unsigned long". >> > >> >> sys/io.h in x86 case using unsigned short int types.. >> >> include/asm-generic/io.h for arm64 using it unsigned long (from linux >> header files) >> >> In such case keeping >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ >> (unsigned short)((hw)->io_base + (reg)) >> >> would be x86 specific and what I thought and used in this patch is >> >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ >> (unsigned long)((hw)->io_base + (reg)) >> >> to avoid ifdef ARM or non-x86..clutter, I know data-type is not right >> fit for x86 sys/io.h but considering possible address inside >> hw->io_base, wont effect functionality and performance my any mean. >> That is why at virtio_ethdev_init() i choose to keep it in hw->io_base >> = (uint64_t) types. >> >> Otherwise I'll have to duplicate VIRTIO_PCI_REG_XXX definition for >> non-x86 case, Pl. suggest better alternative. Thanks > > > My understanding is that if you have done the right cast in the first > time (at the io_base assignment), casting from a short type to a longer > type will not matter: the upper bits will be filled with zero. > > So, I guess we are fine here. I'm thinking that the extra cast in > VIRTIO_PCI_REG_ADDR() is not necessary, as C will do the right > cast for different inb(), say cast it to "unsigned short" for x86, > and "unsigned long" for your arm implementation. The same to > other io helpers. > so to summarize and correct me if i misunderstood, keep hw->io_base = (uint64_t) and remove extra cast {i.e.. (unsigned short) for x86 or (unsigned long) for non-x86/arm64 case} in VIRTIO_PCI_REG_ADDR(). did I got everything alright? > --yliu
On Wed, Dec 16, 2015 at 08:35:58PM +0530, Santosh Shukla wrote: > On Wed, Dec 16, 2015 at 8:28 PM, Yuanhan Liu > <yuanhan.liu@linux.intel.com> wrote: > > On Wed, Dec 16, 2015 at 08:09:40PM +0530, Santosh Shukla wrote: > >> On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu > >> <yuanhan.liu@linux.intel.com> wrote: > >> > On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: > >> >> On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu > >> >> <yuanhan.liu@linux.intel.com> wrote: > >> >> > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: > >> >> >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 > >> >> >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 > >> >> >> bit address so changing io_base datatype from 32 to 64. > >> >> >> > >> >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> > >> >> >> --- > >> >> >> drivers/net/virtio/virtio_ethdev.c | 2 +- > >> >> >> drivers/net/virtio/virtio_pci.h | 4 ++-- > >> >> >> 2 files changed, 3 insertions(+), 3 deletions(-) > >> >> >> > >> >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > >> >> >> index d928339..620e0d4 100644 > >> >> >> --- a/drivers/net/virtio/virtio_ethdev.c > >> >> >> +++ b/drivers/net/virtio/virtio_ethdev.c > >> >> >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) > >> >> >> return -1; > >> >> >> > >> >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); > >> >> >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; > >> >> >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; > >> >> > > >> >> > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() > >> >> > so that we could do the correct cast there, say cast it to uint32_t for > >> >> > X86, and uint64_t for others. > >> >> > > >> >> > >> >> Ok. > >> >> > >> >> This was deliberately done considering your 1.0 virtio spec patch do > >> >> care for uint64_t types and in arm64 case, If I plan to use those > >> >> future patches, IMO it make more sense to me keep it in uint64_t way; > >> > > >> > I did different cast, 32 bit for legacy virtio pci device, and 64 bit > >> > for modern virtio pci device. > >> > > >> >> Also in x86 case max address could of type 0x1000-101f and so forth; > >> >> changing data-type to uint64_t default wont effect such address, > >> >> right? > >> > > >> > Right, but what's the harm of doing the right cast? :) > >> > > >> > >> Agree. > >> > >> >> And hw->io_base by looking at virtio_pci.h function like > >> >> inb/outb etc.. takes io_base address as unsigned long types which is > >> >> arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower > >> >> level rd/wr apis are taking care of data-types accordingly. > >> > > >> > Didn't get it. inb/outb takes "unsigned short" arguments, but not > >> > "unsigned long". > >> > > >> > >> sys/io.h in x86 case using unsigned short int types.. > >> > >> include/asm-generic/io.h for arm64 using it unsigned long (from linux > >> header files) > >> > >> In such case keeping > >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ > >> (unsigned short)((hw)->io_base + (reg)) > >> > >> would be x86 specific and what I thought and used in this patch is > >> > >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ > >> (unsigned long)((hw)->io_base + (reg)) > >> > >> to avoid ifdef ARM or non-x86..clutter, I know data-type is not right > >> fit for x86 sys/io.h but considering possible address inside > >> hw->io_base, wont effect functionality and performance my any mean. > >> That is why at virtio_ethdev_init() i choose to keep it in hw->io_base > >> = (uint64_t) types. > >> > >> Otherwise I'll have to duplicate VIRTIO_PCI_REG_XXX definition for > >> non-x86 case, Pl. suggest better alternative. Thanks > > > > > > My understanding is that if you have done the right cast in the first > > time (at the io_base assignment), casting from a short type to a longer > > type will not matter: the upper bits will be filled with zero. > > > > So, I guess we are fine here. I'm thinking that the extra cast in > > VIRTIO_PCI_REG_ADDR() is not necessary, as C will do the right > > cast for different inb(), say cast it to "unsigned short" for x86, > > and "unsigned long" for your arm implementation. The same to > > other io helpers. > > > > so to summarize and correct me if i misunderstood, > keep hw->io_base = (uint64_t) I still want a different explicit cast for x86 and non-x86. And actually, we should cast it to (unsigned short) but not (uint32_t) for x86, don't we? On the other hand, we may cast it to uint64_t unconditionally, and then have an explicit sanity check for io_base for x86, say if ((unsigned short)hw->io_base != hw->io_base) { PMD_INIT_LOG(ERR, "invalid io port: %"PRIx64, ...); return -1; } It's better than the (unsigned short) cast, as the later simply hides issue when something went wrong, though it's not rare. What do you think of that? > and remove extra cast {i.e.. (unsigned short) for x86 or (unsigned > long) for non-x86/arm64 case} in VIRTIO_PCI_REG_ADDR(). Technically speaking, yes, we don't need this kind of cast. --yliu
On Thu, Dec 17, 2015 at 12:49 PM, Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote: > On Wed, Dec 16, 2015 at 08:35:58PM +0530, Santosh Shukla wrote: >> On Wed, Dec 16, 2015 at 8:28 PM, Yuanhan Liu >> <yuanhan.liu@linux.intel.com> wrote: >> > On Wed, Dec 16, 2015 at 08:09:40PM +0530, Santosh Shukla wrote: >> >> On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu >> >> <yuanhan.liu@linux.intel.com> wrote: >> >> > On Wed, Dec 16, 2015 at 07:31:57PM +0530, Santosh Shukla wrote: >> >> >> On Wed, Dec 16, 2015 at 7:18 PM, Yuanhan Liu >> >> >> <yuanhan.liu@linux.intel.com> wrote: >> >> >> > On Mon, Dec 14, 2015 at 06:30:24PM +0530, Santosh Shukla wrote: >> >> >> >> In x86 case io_base to store ioport address not more than 65535 ioports. i.e..0 >> >> >> >> to ffff but in non-x86 case in particular arm64 it need to store more than 32 >> >> >> >> bit address so changing io_base datatype from 32 to 64. >> >> >> >> >> >> >> >> Signed-off-by: Santosh Shukla <sshukla@mvista.com> >> >> >> >> --- >> >> >> >> drivers/net/virtio/virtio_ethdev.c | 2 +- >> >> >> >> drivers/net/virtio/virtio_pci.h | 4 ++-- >> >> >> >> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> >> >> >> >> >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> >> >> >> index d928339..620e0d4 100644 >> >> >> >> --- a/drivers/net/virtio/virtio_ethdev.c >> >> >> >> +++ b/drivers/net/virtio/virtio_ethdev.c >> >> >> >> @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) >> >> >> >> return -1; >> >> >> >> >> >> >> >> hw->use_msix = virtio_has_msix(&pci_dev->addr); >> >> >> >> - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> >> >> >> + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; >> >> >> > >> >> >> > I'd suggest to move the io_base assignment (and cast) into virtio_ioport_init() >> >> >> > so that we could do the correct cast there, say cast it to uint32_t for >> >> >> > X86, and uint64_t for others. >> >> >> > >> >> >> >> >> >> Ok. >> >> >> >> >> >> This was deliberately done considering your 1.0 virtio spec patch do >> >> >> care for uint64_t types and in arm64 case, If I plan to use those >> >> >> future patches, IMO it make more sense to me keep it in uint64_t way; >> >> > >> >> > I did different cast, 32 bit for legacy virtio pci device, and 64 bit >> >> > for modern virtio pci device. >> >> > >> >> >> Also in x86 case max address could of type 0x1000-101f and so forth; >> >> >> changing data-type to uint64_t default wont effect such address, >> >> >> right? >> >> > >> >> > Right, but what's the harm of doing the right cast? :) >> >> > >> >> >> >> Agree. >> >> >> >> >> And hw->io_base by looking at virtio_pci.h function like >> >> >> inb/outb etc.. takes io_base address as unsigned long types which is >> >> >> arch dependent; i.e.. 4 byte for 32 bit and 8 for 64 bit so the lower >> >> >> level rd/wr apis are taking care of data-types accordingly. >> >> > >> >> > Didn't get it. inb/outb takes "unsigned short" arguments, but not >> >> > "unsigned long". >> >> > >> >> >> >> sys/io.h in x86 case using unsigned short int types.. >> >> >> >> include/asm-generic/io.h for arm64 using it unsigned long (from linux >> >> header files) >> >> >> >> In such case keeping >> >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ >> >> (unsigned short)((hw)->io_base + (reg)) >> >> >> >> would be x86 specific and what I thought and used in this patch is >> >> >> >> #define VIRTIO_PCI_REG_ADDR(hw, reg) \ >> >> (unsigned long)((hw)->io_base + (reg)) >> >> >> >> to avoid ifdef ARM or non-x86..clutter, I know data-type is not right >> >> fit for x86 sys/io.h but considering possible address inside >> >> hw->io_base, wont effect functionality and performance my any mean. >> >> That is why at virtio_ethdev_init() i choose to keep it in hw->io_base >> >> = (uint64_t) types. >> >> >> >> Otherwise I'll have to duplicate VIRTIO_PCI_REG_XXX definition for >> >> non-x86 case, Pl. suggest better alternative. Thanks >> > >> > >> > My understanding is that if you have done the right cast in the first >> > time (at the io_base assignment), casting from a short type to a longer >> > type will not matter: the upper bits will be filled with zero. >> > >> > So, I guess we are fine here. I'm thinking that the extra cast in >> > VIRTIO_PCI_REG_ADDR() is not necessary, as C will do the right >> > cast for different inb(), say cast it to "unsigned short" for x86, >> > and "unsigned long" for your arm implementation. The same to >> > other io helpers. >> > >> >> so to summarize and correct me if i misunderstood, >> keep hw->io_base = (uint64_t) > > I still want a different explicit cast for x86 and non-x86. And > actually, we should cast it to (unsigned short) but not (uint32_t) > for x86, don't we? > > On the other hand, we may cast it to uint64_t unconditionally, > and then have an explicit sanity check for io_base for x86, say > > if ((unsigned short)hw->io_base != hw->io_base) { > PMD_INIT_LOG(ERR, "invalid io port: %"PRIx64, ...); > return -1; > } > > It's better than the (unsigned short) cast, as the later simply hides > issue when something went wrong, though it's not rare. > > What do you think of that? > Liked the later part which is casting uint64_t unconditionally and keeping a (unsigned short) check for x86 case. We'll do it in v3. >> and remove extra cast {i.e.. (unsigned short) for x86 or (unsigned >> long) for non-x86/arm64 case} in VIRTIO_PCI_REG_ADDR(). > > Technically speaking, yes, we don't need this kind of cast. > Ok. > --yliu
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index d928339..620e0d4 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -1291,7 +1291,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev) return -1; hw->use_msix = virtio_has_msix(&pci_dev->addr); - hw->io_base = (uint32_t)(uintptr_t)pci_dev->mem_resource[0].addr; + hw->io_base = (uint64_t)(uintptr_t)pci_dev->mem_resource[0].addr; /* Reset the device although not necessary at startup */ vtpci_reset(hw); diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index 3f4ff80..f3e4178 100644 --- a/drivers/net/virtio/virtio_pci.h +++ b/drivers/net/virtio/virtio_pci.h @@ -169,7 +169,7 @@ struct virtqueue; struct virtio_hw { struct virtqueue *cvq; - uint32_t io_base; + uint64_t io_base; uint32_t guest_features; uint32_t max_tx_queues; uint32_t max_rx_queues; @@ -231,7 +231,7 @@ outl_p(unsigned int data, unsigned int port) #endif #define VIRTIO_PCI_REG_ADDR(hw, reg) \ - (unsigned short)((hw)->io_base + (reg)) + (unsigned long)((hw)->io_base + (reg)) #define VIRTIO_READ_REG_1(hw, reg) \ inb((VIRTIO_PCI_REG_ADDR((hw), (reg))))