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

[Nikunj A Dadhania]

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. Similarly, virtio too has
introduced 64-bit regs.

Aren't we back to square one?

Lets settle for virtio_pci_{read,write}64.

Regards,
Nikunj