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

[Thomas Huth]

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?

 Thomas