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

[Alexey Kardashevskiy]

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.


> Similarly, virtio too has
> introduced 64-bit regs.
>
> Aren't we back to square one?

Nope, at the first place I argued that you do not need them at all, now it 
is all about the names :)


> 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.

Or pci_ci_{read,write}64, to reflect that there are wrappers on top of 
other ci_{read,write}XX.


-- 
Alexey