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

[Nikunj A Dadhania]

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.

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

Regards
Nikunj