[ipxe-devel] [PATCH v2 2/3] [virtio] Renumber virtio_pci_region flags

[Marcel Apfelbaum]

On 05/02/2016 05:38 PM, Ladi Prosek wrote:
> On Mon, May 2, 2016 at 4:21 PM, Marcel Apfelbaum <marcel at redhat.com> wrote:
>> On 05/02/2016 04:11 PM, Ladi Prosek wrote:
>>>
>>> Some of the regions may end up being unmapped, either because
>>> they are optional or because the attempt to map them has failed.
>>> Region types starting at 0 didn't make it easy to test for this
>>> condition.
>>>
>>> This commit adds VIRTIO_PCI_REGION_NONE with the value of 0
>>> and bumps the valid region types up by 1.
>>>
>>> Signed-off-by: Ladi Prosek <lprosek at redhat.com>
>>> ---
>>>    src/include/ipxe/virtio-pci.h | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/include/ipxe/virtio-pci.h b/src/include/ipxe/virtio-pci.h
>>> index c7452c8..ec8efaa 100644
>>> --- a/src/include/ipxe/virtio-pci.h
>>> +++ b/src/include/ipxe/virtio-pci.h
>>> @@ -106,12 +106,14 @@ struct virtio_pci_region {
>>>
>>>    /* How to interpret the base field */
>>>    #define VIRTIO_PCI_REGION_TYPE_MASK  0x00000003
>>> +/* The region is unused */
>>> +#define VIRTIO_PCI_REGION_NONE       0x00000000
>>>    /* The base field is a memory address */
>>> -#define VIRTIO_PCI_REGION_MEMORY     0x00000000
>>> +#define VIRTIO_PCI_REGION_MEMORY     0x00000001
>>>    /* The base field is a port address */
>>> -#define VIRTIO_PCI_REGION_PORT       0x00000001
>>> +#define VIRTIO_PCI_REGION_PORT       0x00000002
>>>    /* The base field is an offset within the PCI bar */
>>> -#define VIRTIO_PCI_REGION_PCI_CONFIG 0x00000002
>>> +#define VIRTIO_PCI_REGION_PCI_CONFIG 0x00000003
>>
>>
>>
>> Is OK if you are not interested to use the flags as bits.
>> Otherwise you may want to give VIRTIO_PCI_REGION_PCI_CONFIG the value 4.
>
> Yes, I just wanted to reserve 2 bits out of the flags field for this.
> The flags are exclusive so no need to combine them and assign one bit
> to each.
>
>> And I also didn't see you used the new VIRTIO_PCI_REGION_NONE flag.
>> I suggest to wait with this patch until you will actually use it.
>
> Patch 3/3 has a dependency on the new numbers in
> virtnet_uses_region_type as vdev->device may not be mapped and flags
> be 0. The old numbers were chosen pretty unfortunately (my fault) so
> you couldn't tell an unused region and a memory region apart.
>

This I understood.

> You're right that VIRTIO_PCI_REGION_NONE is not used. I included it
> for completeness, to be explicit about what 0 means. I can remove it
> if you'd prefer that.

I wouldn't add it if is not used. People searching for it will get
angry/frustrated and git-blame will point to you ... :)

0 as a "disabled" value should be enough until you will actually
use it, then indeed is a good decision.

All of the above IMHO.

Thanks,
Marcel

>
> Thanks,
> Ladi
>
>>
>> Thanks,
>> Marcel
>>
>>>        unsigned flags;
>>>    };
>>>
>>>
>>