[SLOF] [PATCH v2 12/19] virtio: add 64-bit virtio helpers for 1.0

[Alexey Kardashevskiy]

On 01/27/2016 07:20 PM, Nikunj A Dadhania wrote:
> Thomas Huth <thuth at redhat.com> writes:
>
>> On 27.01.2016 04:12, Alexey Kardashevskiy wrote:
>>> On 01/26/2016 03:04 AM, Thomas Huth wrote:
>>>> On 25.01.2016 01:48, Alexey Kardashevskiy wrote:
>>>>> On 01/22/2016 07:09 PM, Nikunj A Dadhania wrote:
>>>>>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>>>>>
>>>>>>> On 01/22/2016 05:30 PM, Thomas Huth wrote:
>>>>>>>> On 22.01.2016 06:25, Alexey Kardashevskiy wrote:
>>>>>>>>> On 01/22/2016 04:18 PM, Nikunj A Dadhania wrote:
>>>>>>>>>> Alexey Kardashevskiy <aik at ozlabs.ru> writes:
>>>>>>>>>>
>>>>>>>>>>> On 01/21/2016 06:58 PM, Nikunj A Dadhania wrote:
>>>>>>>>>>>> Thomas Huth <thuth at redhat.com> writes:
>>>>>>>>>>>>
>>>>>>>>>>>>> On 20.01.2016 13:10, Nikunj A Dadhania wrote:
>>>>>>>>>>>>>>        } __attribute__ ((packed));
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> +/* virtio 1.0 Spec: 4.1.3 PCI Device Layout
>>>>>>>>>>>>>> + *
>>>>>>>>>>>>>> + * Fields of different sizes are present in the device
>>>>>>>>>>>>>> configuration regions.
>>>>>>>>>>>>>> + * All 64-bit, 32-bit and 16-bit fields are little-endian.
>>>>>>>>>>>>>> 64-bit
>>>>>>>>>>>>>> fields are to
>>>>>>>>>>>>>> + * be treated as two 32-bit fields, with low 32 bit part
>>>>>>>>>>>>>> followed
>>>>>>>>>>>>>> by the high 32
>>>>>>>>>>>>>> + * bit part.
>>>>>>>>>>>>>> + */
>>>>>>>>>>>>>> +static void virtio_write64(void *addr, uint64_t val)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +    uint32_t hi = (val >> 32) & 0xFFFFFFFF;
>>>>>>>>>>>>>> +    uint32_t lo = val & 0xFFFFFFFF;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    ci_write_32(addr, cpu_to_le32(lo));
>>>>>>>>>>>>>> +    ci_write_32(addr + 4, cpu_to_le32(hi));
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static uint64_t virtio_read64(void *addr)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> +    uint64_t hi, lo;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +    lo = le32_to_cpu(ci_read_32(addr));
>>>>>>>>>>>>>> +    hi = le32_to_cpu(ci_read_32(addr + 4));
>>>>>>>>>>>>>> +    return (hi << 32) | lo;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>
>>>>>>>>>>>>> I'd maybe name them "virtio_pci_write64" and
>>>>>>>>>>>>> "virtio_pci_read64" to
>>>>>>>>>>>>> make
>>>>>>>>>>>>> it clear that they are only to be used when reading and writing
>>>>>>>>>>>>> to the
>>>>>>>>>>>>> PCI space - otherwise somebody might use them for accessing the
>>>>>>>>>>>>> virtio
>>>>>>>>>>>>> rings one day, for example.
>>>>>>>>>>>>
>>>>>>>>>>>> Right, will rename.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I'd rename these to virtio_ci_write_64/virtio_ci_read_64 as
>>>>>>>>>>> they are
>>>>>>>>>>> wrappers on top of ci_read_xx/ci_write_xx. And frankly they can be
>>>>>>>>>>> used to
>>>>>>>>>>> access any guest physical memory and any MMIO.
>>>>>>>>>>
>>>>>>>>>> They can be used, but should not be, as the register in MMIO are
>>>>>>>>>> not
>>>>>>>>>> split into two registers (hi,low).This is  very specific to VIRTIO
>>>>>>>>>> PCI.
>>>>>>>>>> IMO, the current naming is correct.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This is why I kept "virtio_" prefix. PCI is redundant here - there
>>>>>>>>> is no
>>>>>>>>> other virtio, and "ci" is what it really does. Thomas?
>>>>>>>>
>>>>>>>> The split for 2x 32-bit only makes sense for the PCI access, so I'd
>>>>>>>> also
>>>>>>>> use "_pci_" here.
>>>>>>>
>>>>>>>
>>>>>>> Ok, PCI config space cannot be accessed by 8 bytes. Then remove
>>>>>>> "virtio_"
>>>>>>> from these helpers.
>>>>>>
>>>>>> PCI config space cannot be accessed by 8 bytes, but there are regs that
>>>>>> can be 64-bit: for example BAR registers programming for 64-bit
>>>>>> address.
>>>>>> Check assign-bar-value64 in pci-properties.fs.
>>>>>
>>>>> For these, you have SLOF_pci_config_writeXX API which does not use
>>>>> "virtio_" for these helpers.
>>>> ...
>>>>>> Lets settle for virtio_pci_{read,write}64.
>>>>>
>>>>>
>>>>> pci_{read,write}64 or explain how virtio is special, i.e. other devices
>>>>> PCI would do 64 access in a different (reverse?) order (which ones? I
>>>>> seriously doubt there ever were or will be any). What I see here is that
>>>>> these helpers do read/write little-endian 64bit values. Nothing special
>>>>> about virtio whatsoever.
>>>>
>>>> But the fact that you have to use 2 x 32 bit accesses instead of 1 x 64
>>>> bit is special to virtio, isn't it? Or is there the same limitation for
>>>> all other PCI cards, too?
>>>
>>>
>>> afaict doing 2x32bit accesses in this specific order is the only way to
>>> access 64bit registers via a PCI bus which is little-endian by design.
>>
>> Not sure, but isn't that a rule for config space only?
>
> Yes, its only for config space, for MMIO I am using ci_*_64() calls.

You are both right, sorry. I was confused by absence of ioread64/iowrite64 
in the kernel sources :)


>> ... the virtio
>> rings here are MMIO space instead ... I slightly remember having used
>> full 64-bit accesses to MMIO space in the past already when working with
>> other PCI cards... but my memory might be wrong here...
>
> You are right.




-- 
Alexey