[dpdk-dev] [ [PATCH v2] 05/13] virtio: change io_base datatype from uint32_t to uint64_type

Santosh Shukla sshukla at mvista.com
Wed Dec 16 15:39:40 CET 2015


On Wed, Dec 16, 2015 at 7:53 PM, Yuanhan Liu
<yuanhan.liu at 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 at 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 at 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


More information about the dev mailing list