Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-06 Thread Marcel Apfelbaum 

On 04/08/2017 23:47, Alexander Bezzubikov wrote:

2017-08-04 23:28 GMT+03:00 Laszlo Ersek :

On 08/04/17 20:59, Alexander Bezzubikov wrote:

2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov :

2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum :

On 31/07/2017 22:01, Alexander Bezzubikov wrote:


2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :


On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:


2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :


On 31/07/2017 17:00, Michael S. Tsirkin wrote:



On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:



On PCI init PCI bridge devices may need some
extra info about bus number to reserve, IO, memory and
prefetchable memory limits. QEMU can provide this
with special vendor-specific PCI capability.

This capability is intended to be used only
for Red Hat PCI bridges, i.e. QEMU cooperation.

Signed-off-by: Aleksandr Bezzubikov 
---
src/fw/dev-pci.h | 62

1 file changed, 62 insertions(+)
create mode 100644 src/fw/dev-pci.h

diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
new file mode 100644
index 000..fbd49ed
--- /dev/null
+++ b/src/fw/dev-pci.h
@@ -0,0 +1,62 @@
+#ifndef _PCI_CAP_H
+#define _PCI_CAP_H
+
+#include "types.h"
+
+/*
+
+QEMU-specific vendor(Red Hat)-specific capability.
+It's intended to provide some hints for firmware to init PCI
devices.
+
+Its is shown below:
+
+Header:
+
+u8 id;   Standard PCI Capability Header field
+u8 next; Standard PCI Capability Header field
+u8 len;  Standard PCI Capability Header field
+u8 type; Red Hat vendor-specific capability type:
+   now only REDHAT_QEMU_CAP 1 exists
+Data:
+
+u16 non_prefetchable_16; non-prefetchable memory limit
+
+u8 bus_res;  minimum bus number to reserve;
+ this is necessary for PCI Express Root Ports
+ to support PCIE-to-PCI bridge hotplug
+
+u8 io_8; IO limit in case of 8-bit limit value
+u32 io_32;   IO limit in case of 16-bit limit value
+ io_8 and io_16 are mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+
+u32 prefetchable_32; non-prefetchable memory limit
+ in case of 32-bit limit value
+u64 prefetchable_64; non-prefetchable memory limit
+ in case of 64-bit limit value
+ prefetachable_32 and prefetchable_64
are
+ mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+If any field in Data section is 0,
+it means that such kind of reservation
+is not needed.



I really don't like this 'mutually exclusive' fields approach because
IMHO it increases confusion level when undertanding this capability
structure.
But - if we came to consensus on that, then IO fields should be used
in the same way,
because as I understand, this 'mutual exclusivity' serves to distinguish
cases
when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
registers.
And this is how both IO and PREFETCHABLE works, isn't it?



I would just use simeple 64 bit registers. PCI spec has an ugly format
with fields spread all over the place but that is because of
compatibility concerns. It makes not sense to spend cycles just
to be similarly messy.



Then I suggest to use exactly one field of a maximum possible size
for each reserving object, and get rid of mutually exclusive fields.
Then it can be something like that (order and names can be changed):
u8 bus;
u16 non_pref;
u32 io;
u64 pref;



I think Michael suggested:

 u64 bus_res;
 u64 mem_res;
 u64 io_res;
 u64 mem_pref_res;

OR:
 u32 bus_res;
 u32 mem_res;
 u32 io_res;
 u64 mem_pref_res;


We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
requests.


Let's dwell on the second option (with -1 as 'ignore' sign), if no new
objections.



BTW, talking about limit values provided in the capability - do we
want to completely override
existing PCI resources allocation mechanism being used in SeaBIOS, I
mean, to assign capability
values hardly, not taking into consideration any existing checks,
or somehow make this process soft (not an obvious way, can lead to
another big discussion)?

In other words, how do we plan to use IO/MEM/PREF limits provided in
this capability
in application to the PCIE Root Port, what result is this supposed to achieve?


I think Gerd spoke about this earlier: when determining a given kind of
aperture for a given bridge, pick the maximum of:
- the actual cumulative need of the devices behind the bridge, and
- the hint for the given kind of aperture.

So basically, do the same thing as before, but if the hint is larger,
grow the aperture to that.


Looks like current SeaBIOS resource allocation

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-04 Thread Alexander Bezzubikov 
2017-08-04 23:28 GMT+03:00 Laszlo Ersek :
> On 08/04/17 20:59, Alexander Bezzubikov wrote:
>> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov :
>>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum :
 On 31/07/2017 22:01, Alexander Bezzubikov wrote:
>
> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :
>>
>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
>>>
>>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :

 On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>
>
> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>>
>>
>> On PCI init PCI bridge devices may need some
>> extra info about bus number to reserve, IO, memory and
>> prefetchable memory limits. QEMU can provide this
>> with special vendor-specific PCI capability.
>>
>> This capability is intended to be used only
>> for Red Hat PCI bridges, i.e. QEMU cooperation.
>>
>> Signed-off-by: Aleksandr Bezzubikov 
>> ---
>>src/fw/dev-pci.h | 62
>> 
>>1 file changed, 62 insertions(+)
>>create mode 100644 src/fw/dev-pci.h
>>
>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
>> new file mode 100644
>> index 000..fbd49ed
>> --- /dev/null
>> +++ b/src/fw/dev-pci.h
>> @@ -0,0 +1,62 @@
>> +#ifndef _PCI_CAP_H
>> +#define _PCI_CAP_H
>> +
>> +#include "types.h"
>> +
>> +/*
>> +
>> +QEMU-specific vendor(Red Hat)-specific capability.
>> +It's intended to provide some hints for firmware to init PCI
>> devices.
>> +
>> +Its is shown below:
>> +
>> +Header:
>> +
>> +u8 id;   Standard PCI Capability Header field
>> +u8 next; Standard PCI Capability Header field
>> +u8 len;  Standard PCI Capability Header field
>> +u8 type; Red Hat vendor-specific capability type:
>> +   now only REDHAT_QEMU_CAP 1 exists
>> +Data:
>> +
>> +u16 non_prefetchable_16; non-prefetchable memory limit
>> +
>> +u8 bus_res;  minimum bus number to reserve;
>> + this is necessary for PCI Express Root Ports
>> + to support PCIE-to-PCI bridge hotplug
>> +
>> +u8 io_8; IO limit in case of 8-bit limit value
>> +u32 io_32;   IO limit in case of 16-bit limit value
>> + io_8 and io_16 are mutually exclusive, in other words,
>> + they can't be non-zero simultaneously
>> +
>> +u32 prefetchable_32; non-prefetchable memory limit
>> + in case of 32-bit limit value
>> +u64 prefetchable_64; non-prefetchable memory limit
>> + in case of 64-bit limit value
>> + prefetachable_32 and prefetchable_64
>> are
>> + mutually exclusive, in other words,
>> + they can't be non-zero simultaneously
>> +If any field in Data section is 0,
>> +it means that such kind of reservation
>> +is not needed.
>>>
>>>
>>> I really don't like this 'mutually exclusive' fields approach because
>>> IMHO it increases confusion level when undertanding this capability
>>> structure.
>>> But - if we came to consensus on that, then IO fields should be used
>>> in the same way,
>>> because as I understand, this 'mutual exclusivity' serves to distinguish
>>> cases
>>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
>>> registers.
>>> And this is how both IO and PREFETCHABLE works, isn't it?
>>
>>
>> I would just use simeple 64 bit registers. PCI spec has an ugly format
>> with fields spread all over the place but that is because of
>> compatibility concerns. It makes not sense to spend cycles just
>> to be similarly messy.
>
>
> Then I suggest to use exactly one field of a maximum possible size
> for each reserving object, and get rid of mutually exclusive fields.
> Then it can be something like that (order and names can be changed):
> u8 bus;
> u16 non_pref;
> u32 io;
> u64 pref;
>

 I think Michael suggested:

 u64 bus_res;
 u64 mem_res;
 u64 io_res;
 u64 mem_pref_res;

 OR:
 u32 bus_res;
 u32 mem_res;
 u32 io_res;
 u64 mem_pref_res;


 We can use 0XFFF..F as "not-set" value

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-04 Thread Laszlo Ersek 
On 08/04/17 20:59, Alexander Bezzubikov wrote:
> 2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov :
>> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum :
>>> On 31/07/2017 22:01, Alexander Bezzubikov wrote:

 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :
>
> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
>>
>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
>>>
>>> On 31/07/2017 17:00, Michael S. Tsirkin wrote:


 On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>
>
> On PCI init PCI bridge devices may need some
> extra info about bus number to reserve, IO, memory and
> prefetchable memory limits. QEMU can provide this
> with special vendor-specific PCI capability.
>
> This capability is intended to be used only
> for Red Hat PCI bridges, i.e. QEMU cooperation.
>
> Signed-off-by: Aleksandr Bezzubikov 
> ---
>src/fw/dev-pci.h | 62
> 
>1 file changed, 62 insertions(+)
>create mode 100644 src/fw/dev-pci.h
>
> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
> new file mode 100644
> index 000..fbd49ed
> --- /dev/null
> +++ b/src/fw/dev-pci.h
> @@ -0,0 +1,62 @@
> +#ifndef _PCI_CAP_H
> +#define _PCI_CAP_H
> +
> +#include "types.h"
> +
> +/*
> +
> +QEMU-specific vendor(Red Hat)-specific capability.
> +It's intended to provide some hints for firmware to init PCI
> devices.
> +
> +Its is shown below:
> +
> +Header:
> +
> +u8 id;   Standard PCI Capability Header field
> +u8 next; Standard PCI Capability Header field
> +u8 len;  Standard PCI Capability Header field
> +u8 type; Red Hat vendor-specific capability type:
> +   now only REDHAT_QEMU_CAP 1 exists
> +Data:
> +
> +u16 non_prefetchable_16; non-prefetchable memory limit
> +
> +u8 bus_res;  minimum bus number to reserve;
> + this is necessary for PCI Express Root Ports
> + to support PCIE-to-PCI bridge hotplug
> +
> +u8 io_8; IO limit in case of 8-bit limit value
> +u32 io_32;   IO limit in case of 16-bit limit value
> + io_8 and io_16 are mutually exclusive, in other words,
> + they can't be non-zero simultaneously
> +
> +u32 prefetchable_32; non-prefetchable memory limit
> + in case of 32-bit limit value
> +u64 prefetchable_64; non-prefetchable memory limit
> + in case of 64-bit limit value
> + prefetachable_32 and prefetchable_64
> are
> + mutually exclusive, in other words,
> + they can't be non-zero simultaneously
> +If any field in Data section is 0,
> +it means that such kind of reservation
> +is not needed.
>>
>>
>> I really don't like this 'mutually exclusive' fields approach because
>> IMHO it increases confusion level when undertanding this capability
>> structure.
>> But - if we came to consensus on that, then IO fields should be used
>> in the same way,
>> because as I understand, this 'mutual exclusivity' serves to distinguish
>> cases
>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
>> registers.
>> And this is how both IO and PREFETCHABLE works, isn't it?
>
>
> I would just use simeple 64 bit registers. PCI spec has an ugly format
> with fields spread all over the place but that is because of
> compatibility concerns. It makes not sense to spend cycles just
> to be similarly messy.


 Then I suggest to use exactly one field of a maximum possible size
 for each reserving object, and get rid of mutually exclusive fields.
 Then it can be something like that (order and names can be changed):
 u8 bus;
 u16 non_pref;
 u32 io;
 u64 pref;

>>>
>>> I think Michael suggested:
>>>
>>> u64 bus_res;
>>> u64 mem_res;
>>> u64 io_res;
>>> u64 mem_pref_res;
>>>
>>> OR:
>>> u32 bus_res;
>>> u32 mem_res;
>>> u32 io_res;
>>> u64 mem_pref_res;
>>>
>>>
>>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
>>> requests.
>>
>> Let's dwell on the second option (with -1 as 'ignore' sign), if no new
>> objections.
>>
> 
> BTW, talking about limit values provided in the

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-04 Thread Alexander Bezzubikov 
2017-08-01 20:28 GMT+03:00 Alexander Bezzubikov :
> 2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum :
>> On 31/07/2017 22:01, Alexander Bezzubikov wrote:
>>>
>>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :

 On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
>
> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
>>
>> On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>>>
>>>
>>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:


 On PCI init PCI bridge devices may need some
 extra info about bus number to reserve, IO, memory and
 prefetchable memory limits. QEMU can provide this
 with special vendor-specific PCI capability.

 This capability is intended to be used only
 for Red Hat PCI bridges, i.e. QEMU cooperation.

 Signed-off-by: Aleksandr Bezzubikov 
 ---
src/fw/dev-pci.h | 62
 
1 file changed, 62 insertions(+)
create mode 100644 src/fw/dev-pci.h

 diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
 new file mode 100644
 index 000..fbd49ed
 --- /dev/null
 +++ b/src/fw/dev-pci.h
 @@ -0,0 +1,62 @@
 +#ifndef _PCI_CAP_H
 +#define _PCI_CAP_H
 +
 +#include "types.h"
 +
 +/*
 +
 +QEMU-specific vendor(Red Hat)-specific capability.
 +It's intended to provide some hints for firmware to init PCI
 devices.
 +
 +Its is shown below:
 +
 +Header:
 +
 +u8 id;   Standard PCI Capability Header field
 +u8 next; Standard PCI Capability Header field
 +u8 len;  Standard PCI Capability Header field
 +u8 type; Red Hat vendor-specific capability type:
 +   now only REDHAT_QEMU_CAP 1 exists
 +Data:
 +
 +u16 non_prefetchable_16; non-prefetchable memory limit
 +
 +u8 bus_res;  minimum bus number to reserve;
 + this is necessary for PCI Express Root Ports
 + to support PCIE-to-PCI bridge hotplug
 +
 +u8 io_8; IO limit in case of 8-bit limit value
 +u32 io_32;   IO limit in case of 16-bit limit value
 + io_8 and io_16 are mutually exclusive, in other words,
 + they can't be non-zero simultaneously
 +
 +u32 prefetchable_32; non-prefetchable memory limit
 + in case of 32-bit limit value
 +u64 prefetchable_64; non-prefetchable memory limit
 + in case of 64-bit limit value
 + prefetachable_32 and prefetchable_64
 are
 + mutually exclusive, in other words,
 + they can't be non-zero simultaneously
 +If any field in Data section is 0,
 +it means that such kind of reservation
 +is not needed.
>
>
> I really don't like this 'mutually exclusive' fields approach because
> IMHO it increases confusion level when undertanding this capability
> structure.
> But - if we came to consensus on that, then IO fields should be used
> in the same way,
> because as I understand, this 'mutual exclusivity' serves to distinguish
> cases
> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
> registers.
> And this is how both IO and PREFETCHABLE works, isn't it?


 I would just use simeple 64 bit registers. PCI spec has an ugly format
 with fields spread all over the place but that is because of
 compatibility concerns. It makes not sense to spend cycles just
 to be similarly messy.
>>>
>>>
>>> Then I suggest to use exactly one field of a maximum possible size
>>> for each reserving object, and get rid of mutually exclusive fields.
>>> Then it can be something like that (order and names can be changed):
>>> u8 bus;
>>> u16 non_pref;
>>> u32 io;
>>> u64 pref;
>>>
>>
>> I think Michael suggested:
>>
>> u64 bus_res;
>> u64 mem_res;
>> u64 io_res;
>> u64 mem_pref_res;
>>
>> OR:
>> u32 bus_res;
>> u32 mem_res;
>> u32 io_res;
>> u64 mem_pref_res;
>>
>>
>> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
>> requests.
>
> Let's dwell on the second option (with -1 as 'ignore' sign), if no new
> objections.
>

BTW, talking about limit values provided in the capability - do we
want to completely override
existing PCI resources allocation mechanism being used in SeaBIOS, I
mean, to assign capability
values hardly, not taking into

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-01 Thread Alexander Bezzubikov 
2017-08-01 16:38 GMT+03:00 Marcel Apfelbaum :
> On 31/07/2017 22:01, Alexander Bezzubikov wrote:
>>
>> 2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :
>>>
>>> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:

 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
>
> On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>>
>>
>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>>>
>>>
>>> On PCI init PCI bridge devices may need some
>>> extra info about bus number to reserve, IO, memory and
>>> prefetchable memory limits. QEMU can provide this
>>> with special vendor-specific PCI capability.
>>>
>>> This capability is intended to be used only
>>> for Red Hat PCI bridges, i.e. QEMU cooperation.
>>>
>>> Signed-off-by: Aleksandr Bezzubikov 
>>> ---
>>>src/fw/dev-pci.h | 62
>>> 
>>>1 file changed, 62 insertions(+)
>>>create mode 100644 src/fw/dev-pci.h
>>>
>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
>>> new file mode 100644
>>> index 000..fbd49ed
>>> --- /dev/null
>>> +++ b/src/fw/dev-pci.h
>>> @@ -0,0 +1,62 @@
>>> +#ifndef _PCI_CAP_H
>>> +#define _PCI_CAP_H
>>> +
>>> +#include "types.h"
>>> +
>>> +/*
>>> +
>>> +QEMU-specific vendor(Red Hat)-specific capability.
>>> +It's intended to provide some hints for firmware to init PCI
>>> devices.
>>> +
>>> +Its is shown below:
>>> +
>>> +Header:
>>> +
>>> +u8 id;   Standard PCI Capability Header field
>>> +u8 next; Standard PCI Capability Header field
>>> +u8 len;  Standard PCI Capability Header field
>>> +u8 type; Red Hat vendor-specific capability type:
>>> +   now only REDHAT_QEMU_CAP 1 exists
>>> +Data:
>>> +
>>> +u16 non_prefetchable_16; non-prefetchable memory limit
>>> +
>>> +u8 bus_res;  minimum bus number to reserve;
>>> + this is necessary for PCI Express Root Ports
>>> + to support PCIE-to-PCI bridge hotplug
>>> +
>>> +u8 io_8; IO limit in case of 8-bit limit value
>>> +u32 io_32;   IO limit in case of 16-bit limit value
>>> + io_8 and io_16 are mutually exclusive, in other words,
>>> + they can't be non-zero simultaneously
>>> +
>>> +u32 prefetchable_32; non-prefetchable memory limit
>>> + in case of 32-bit limit value
>>> +u64 prefetchable_64; non-prefetchable memory limit
>>> + in case of 64-bit limit value
>>> + prefetachable_32 and prefetchable_64
>>> are
>>> + mutually exclusive, in other words,
>>> + they can't be non-zero simultaneously
>>> +If any field in Data section is 0,
>>> +it means that such kind of reservation
>>> +is not needed.


 I really don't like this 'mutually exclusive' fields approach because
 IMHO it increases confusion level when undertanding this capability
 structure.
 But - if we came to consensus on that, then IO fields should be used
 in the same way,
 because as I understand, this 'mutual exclusivity' serves to distinguish
 cases
 when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
 registers.
 And this is how both IO and PREFETCHABLE works, isn't it?
>>>
>>>
>>> I would just use simeple 64 bit registers. PCI spec has an ugly format
>>> with fields spread all over the place but that is because of
>>> compatibility concerns. It makes not sense to spend cycles just
>>> to be similarly messy.
>>
>>
>> Then I suggest to use exactly one field of a maximum possible size
>> for each reserving object, and get rid of mutually exclusive fields.
>> Then it can be something like that (order and names can be changed):
>> u8 bus;
>> u16 non_pref;
>> u32 io;
>> u64 pref;
>>
>
> I think Michael suggested:
>
> u64 bus_res;
> u64 mem_res;
> u64 io_res;
> u64 mem_pref_res;
>
> OR:
> u32 bus_res;
> u32 mem_res;
> u32 io_res;
> u64 mem_pref_res;
>
>
> We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
> requests.

Let's dwell on the second option (with -1 as 'ignore' sign), if no new
objections.

>
> Thanks,
> Marcel
>
>
>>
>>>
>>>
>>
>>
>
> Hi Michael,
>
>> We also want a way to say "no hint for this type".
>>
>> One way to achive this would be to have instead multiple
>> vendor specific capabilities, one for each of
>> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
>> absence of capability would mean "no info, up to firmware".
>>
>
> First version of

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-08-01 Thread Marcel Apfelbaum 

On 31/07/2017 22:01, Alexander Bezzubikov wrote:

2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :

On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:

2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :

On 31/07/2017 17:00, Michael S. Tsirkin wrote:


On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:


On PCI init PCI bridge devices may need some
extra info about bus number to reserve, IO, memory and
prefetchable memory limits. QEMU can provide this
with special vendor-specific PCI capability.

This capability is intended to be used only
for Red Hat PCI bridges, i.e. QEMU cooperation.

Signed-off-by: Aleksandr Bezzubikov 
---
   src/fw/dev-pci.h | 62

   1 file changed, 62 insertions(+)
   create mode 100644 src/fw/dev-pci.h

diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
new file mode 100644
index 000..fbd49ed
--- /dev/null
+++ b/src/fw/dev-pci.h
@@ -0,0 +1,62 @@
+#ifndef _PCI_CAP_H
+#define _PCI_CAP_H
+
+#include "types.h"
+
+/*
+
+QEMU-specific vendor(Red Hat)-specific capability.
+It's intended to provide some hints for firmware to init PCI devices.
+
+Its is shown below:
+
+Header:
+
+u8 id;   Standard PCI Capability Header field
+u8 next; Standard PCI Capability Header field
+u8 len;  Standard PCI Capability Header field
+u8 type; Red Hat vendor-specific capability type:
+   now only REDHAT_QEMU_CAP 1 exists
+Data:
+
+u16 non_prefetchable_16; non-prefetchable memory limit
+
+u8 bus_res;  minimum bus number to reserve;
+ this is necessary for PCI Express Root Ports
+ to support PCIE-to-PCI bridge hotplug
+
+u8 io_8; IO limit in case of 8-bit limit value
+u32 io_32;   IO limit in case of 16-bit limit value
+ io_8 and io_16 are mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+
+u32 prefetchable_32; non-prefetchable memory limit
+ in case of 32-bit limit value
+u64 prefetchable_64; non-prefetchable memory limit
+ in case of 64-bit limit value
+ prefetachable_32 and prefetchable_64 are
+ mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+If any field in Data section is 0,
+it means that such kind of reservation
+is not needed.


I really don't like this 'mutually exclusive' fields approach because
IMHO it increases confusion level when undertanding this capability structure.
But - if we came to consensus on that, then IO fields should be used
in the same way,
because as I understand, this 'mutual exclusivity' serves to distinguish cases
when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
registers.
And this is how both IO and PREFETCHABLE works, isn't it?


I would just use simeple 64 bit registers. PCI spec has an ugly format
with fields spread all over the place but that is because of
compatibility concerns. It makes not sense to spend cycles just
to be similarly messy.


Then I suggest to use exactly one field of a maximum possible size
for each reserving object, and get rid of mutually exclusive fields.
Then it can be something like that (order and names can be changed):
u8 bus;
u16 non_pref;
u32 io;
u64 pref;



I think Michael suggested:

u64 bus_res;
u64 mem_res;
u64 io_res;
u64 mem_pref_res;

OR:
u32 bus_res;
u32 mem_res;
u32 io_res;
u64 mem_pref_res;


We can use 0XFFF..F as "not-set" value "merging" Gerd's and Michael's
requests.

Thanks,
Marcel











Hi Michael,


We also want a way to say "no hint for this type".

One way to achive this would be to have instead multiple
vendor specific capabilities, one for each of
bus#/io/mem/prefetch. 0 would mean do not reserve anything,
absence of capability would mean "no info, up to firmware".



First version of the series was implemented exactly like you propose,
however Gerd preferred only one capability with multiple fields.

I personally like the simplicity of vendor cap per io/mem/bus,
even if it is on the expense of the limited PCI Config space.



Personally I agree with Marcel since all this fields express
reservations of some objects.


We need a consensus here :)



Absolutely :)


Thanks,
Marcel





+
+*/
+
+/* Offset of vendor-specific capability type field */
+#define PCI_CAP_VNDR_SPEC_TYPE  3



This is a QEMU specific thing. Please name it as such.


+
+/* List of valid Red Hat vendor-specific capability types */
+#define REDHAT_CAP_TYPE_QEMU1
+
+
+/* Offsets of QEMU capability fields */
+#define QEMU_PCI_CAP_NON_PREF   4
+#define QEMU_PCI_CAP_BUS_RES6
+#define QEMU_PCI_CAP_IO_8   7
+#define QEMU_PCI_CAP_IO_32  8
+#define QEMU_PCI_CAP_PREF_3212
+#define QEMU_PCI_CAP_PREF_6416
+#define QEMU_PCI_CAP_SIZE   24
+

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Alexander Bezzubikov 
2017-07-31 21:57 GMT+03:00 Michael S. Tsirkin :
> On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
>> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
>> > On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>> >>
>> >> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>> >>>
>> >>> On PCI init PCI bridge devices may need some
>> >>> extra info about bus number to reserve, IO, memory and
>> >>> prefetchable memory limits. QEMU can provide this
>> >>> with special vendor-specific PCI capability.
>> >>>
>> >>> This capability is intended to be used only
>> >>> for Red Hat PCI bridges, i.e. QEMU cooperation.
>> >>>
>> >>> Signed-off-by: Aleksandr Bezzubikov 
>> >>> ---
>> >>>   src/fw/dev-pci.h | 62
>> >>> 
>> >>>   1 file changed, 62 insertions(+)
>> >>>   create mode 100644 src/fw/dev-pci.h
>> >>>
>> >>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
>> >>> new file mode 100644
>> >>> index 000..fbd49ed
>> >>> --- /dev/null
>> >>> +++ b/src/fw/dev-pci.h
>> >>> @@ -0,0 +1,62 @@
>> >>> +#ifndef _PCI_CAP_H
>> >>> +#define _PCI_CAP_H
>> >>> +
>> >>> +#include "types.h"
>> >>> +
>> >>> +/*
>> >>> +
>> >>> +QEMU-specific vendor(Red Hat)-specific capability.
>> >>> +It's intended to provide some hints for firmware to init PCI devices.
>> >>> +
>> >>> +Its is shown below:
>> >>> +
>> >>> +Header:
>> >>> +
>> >>> +u8 id;   Standard PCI Capability Header field
>> >>> +u8 next; Standard PCI Capability Header field
>> >>> +u8 len;  Standard PCI Capability Header field
>> >>> +u8 type; Red Hat vendor-specific capability type:
>> >>> +   now only REDHAT_QEMU_CAP 1 exists
>> >>> +Data:
>> >>> +
>> >>> +u16 non_prefetchable_16; non-prefetchable memory limit
>> >>> +
>> >>> +u8 bus_res;  minimum bus number to reserve;
>> >>> + this is necessary for PCI Express Root Ports
>> >>> + to support PCIE-to-PCI bridge hotplug
>> >>> +
>> >>> +u8 io_8; IO limit in case of 8-bit limit value
>> >>> +u32 io_32;   IO limit in case of 16-bit limit value
>> >>> + io_8 and io_16 are mutually exclusive, in other words,
>> >>> + they can't be non-zero simultaneously
>> >>> +
>> >>> +u32 prefetchable_32; non-prefetchable memory limit
>> >>> + in case of 32-bit limit value
>> >>> +u64 prefetchable_64; non-prefetchable memory limit
>> >>> + in case of 64-bit limit value
>> >>> + prefetachable_32 and prefetchable_64 are
>> >>> + mutually exclusive, in other words,
>> >>> + they can't be non-zero simultaneously
>> >>> +If any field in Data section is 0,
>> >>> +it means that such kind of reservation
>> >>> +is not needed.
>>
>> I really don't like this 'mutually exclusive' fields approach because
>> IMHO it increases confusion level when undertanding this capability 
>> structure.
>> But - if we came to consensus on that, then IO fields should be used
>> in the same way,
>> because as I understand, this 'mutual exclusivity' serves to distinguish 
>> cases
>> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
>> registers.
>> And this is how both IO and PREFETCHABLE works, isn't it?
>
> I would just use simeple 64 bit registers. PCI spec has an ugly format
> with fields spread all over the place but that is because of
> compatibility concerns. It makes not sense to spend cycles just
> to be similarly messy.

Then I suggest to use exactly one field of a maximum possible size
for each reserving object, and get rid of mutually exclusive fields.
Then it can be something like that (order and names can be changed):
u8 bus;
u16 non_pref;
u32 io;
u64 pref;


>
>
>> >>
>> >>
>> >
>> > Hi Michael,
>> >
>> >> We also want a way to say "no hint for this type".
>> >>
>> >> One way to achive this would be to have instead multiple
>> >> vendor specific capabilities, one for each of
>> >> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
>> >> absence of capability would mean "no info, up to firmware".
>> >>
>> >
>> > First version of the series was implemented exactly like you propose,
>> > however Gerd preferred only one capability with multiple fields.
>> >
>> > I personally like the simplicity of vendor cap per io/mem/bus,
>> > even if it is on the expense of the limited PCI Config space.
>> >
>>
>> Personally I agree with Marcel since all this fields express
>> reservations of some objects.
>>
>> > We need a consensus here :)
>> >
>>
>> Absolutely :)
>>
>> > Thanks,
>> > Marcel
>> >
>> >
>> >>
>> >>> +
>> >>> +*/
>> >>> +
>> >>> +/* Offset of vendor-specific capability type field */
>> >>> +#define PCI_CAP_VNDR_SPEC_TYPE  3
>> >>
>> >>
>> >> This is a QEMU specific thing. Please name it as such.
>> >>
>> >>> +
>> >>> +/* List of valid Red Hat

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Michael S. Tsirkin 
On Mon, Jul 31, 2017 at 09:54:55PM +0300, Alexander Bezzubikov wrote:
> 2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
> > On 31/07/2017 17:00, Michael S. Tsirkin wrote:
> >>
> >> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
> >>>
> >>> On PCI init PCI bridge devices may need some
> >>> extra info about bus number to reserve, IO, memory and
> >>> prefetchable memory limits. QEMU can provide this
> >>> with special vendor-specific PCI capability.
> >>>
> >>> This capability is intended to be used only
> >>> for Red Hat PCI bridges, i.e. QEMU cooperation.
> >>>
> >>> Signed-off-by: Aleksandr Bezzubikov 
> >>> ---
> >>>   src/fw/dev-pci.h | 62
> >>> 
> >>>   1 file changed, 62 insertions(+)
> >>>   create mode 100644 src/fw/dev-pci.h
> >>>
> >>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
> >>> new file mode 100644
> >>> index 000..fbd49ed
> >>> --- /dev/null
> >>> +++ b/src/fw/dev-pci.h
> >>> @@ -0,0 +1,62 @@
> >>> +#ifndef _PCI_CAP_H
> >>> +#define _PCI_CAP_H
> >>> +
> >>> +#include "types.h"
> >>> +
> >>> +/*
> >>> +
> >>> +QEMU-specific vendor(Red Hat)-specific capability.
> >>> +It's intended to provide some hints for firmware to init PCI devices.
> >>> +
> >>> +Its is shown below:
> >>> +
> >>> +Header:
> >>> +
> >>> +u8 id;   Standard PCI Capability Header field
> >>> +u8 next; Standard PCI Capability Header field
> >>> +u8 len;  Standard PCI Capability Header field
> >>> +u8 type; Red Hat vendor-specific capability type:
> >>> +   now only REDHAT_QEMU_CAP 1 exists
> >>> +Data:
> >>> +
> >>> +u16 non_prefetchable_16; non-prefetchable memory limit
> >>> +
> >>> +u8 bus_res;  minimum bus number to reserve;
> >>> + this is necessary for PCI Express Root Ports
> >>> + to support PCIE-to-PCI bridge hotplug
> >>> +
> >>> +u8 io_8; IO limit in case of 8-bit limit value
> >>> +u32 io_32;   IO limit in case of 16-bit limit value
> >>> + io_8 and io_16 are mutually exclusive, in other words,
> >>> + they can't be non-zero simultaneously
> >>> +
> >>> +u32 prefetchable_32; non-prefetchable memory limit
> >>> + in case of 32-bit limit value
> >>> +u64 prefetchable_64; non-prefetchable memory limit
> >>> + in case of 64-bit limit value
> >>> + prefetachable_32 and prefetchable_64 are
> >>> + mutually exclusive, in other words,
> >>> + they can't be non-zero simultaneously
> >>> +If any field in Data section is 0,
> >>> +it means that such kind of reservation
> >>> +is not needed.
> 
> I really don't like this 'mutually exclusive' fields approach because
> IMHO it increases confusion level when undertanding this capability structure.
> But - if we came to consensus on that, then IO fields should be used
> in the same way,
> because as I understand, this 'mutual exclusivity' serves to distinguish cases
> when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
> registers.
> And this is how both IO and PREFETCHABLE works, isn't it?

I would just use simeple 64 bit registers. PCI spec has an ugly format
with fields spread all over the place but that is because of
compatibility concerns. It makes not sense to spend cycles just
to be similarly messy.


> >>
> >>
> >
> > Hi Michael,
> >
> >> We also want a way to say "no hint for this type".
> >>
> >> One way to achive this would be to have instead multiple
> >> vendor specific capabilities, one for each of
> >> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
> >> absence of capability would mean "no info, up to firmware".
> >>
> >
> > First version of the series was implemented exactly like you propose,
> > however Gerd preferred only one capability with multiple fields.
> >
> > I personally like the simplicity of vendor cap per io/mem/bus,
> > even if it is on the expense of the limited PCI Config space.
> >
> 
> Personally I agree with Marcel since all this fields express
> reservations of some objects.
> 
> > We need a consensus here :)
> >
> 
> Absolutely :)
> 
> > Thanks,
> > Marcel
> >
> >
> >>
> >>> +
> >>> +*/
> >>> +
> >>> +/* Offset of vendor-specific capability type field */
> >>> +#define PCI_CAP_VNDR_SPEC_TYPE  3
> >>
> >>
> >> This is a QEMU specific thing. Please name it as such.
> >>
> >>> +
> >>> +/* List of valid Red Hat vendor-specific capability types */
> >>> +#define REDHAT_CAP_TYPE_QEMU1
> >>> +
> >>> +
> >>> +/* Offsets of QEMU capability fields */
> >>> +#define QEMU_PCI_CAP_NON_PREF   4
> >>> +#define QEMU_PCI_CAP_BUS_RES6
> >>> +#define QEMU_PCI_CAP_IO_8   7
> >>> +#define QEMU_PCI_CAP_IO_32  8
> >>> +#define QEMU_PCI_CAP_PREF_3212
> >>> +#define QEMU_PCI_CAP_PREF_6416
> >>> +#define QEMU_PCI_CAP_SIZE   24
> >>> +
> >>> +#endif

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Alexander Bezzubikov 
2017-07-31 17:09 GMT+03:00 Marcel Apfelbaum :
> On 31/07/2017 17:00, Michael S. Tsirkin wrote:
>>
>> On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
>>>
>>> On PCI init PCI bridge devices may need some
>>> extra info about bus number to reserve, IO, memory and
>>> prefetchable memory limits. QEMU can provide this
>>> with special vendor-specific PCI capability.
>>>
>>> This capability is intended to be used only
>>> for Red Hat PCI bridges, i.e. QEMU cooperation.
>>>
>>> Signed-off-by: Aleksandr Bezzubikov 
>>> ---
>>>   src/fw/dev-pci.h | 62
>>> 
>>>   1 file changed, 62 insertions(+)
>>>   create mode 100644 src/fw/dev-pci.h
>>>
>>> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
>>> new file mode 100644
>>> index 000..fbd49ed
>>> --- /dev/null
>>> +++ b/src/fw/dev-pci.h
>>> @@ -0,0 +1,62 @@
>>> +#ifndef _PCI_CAP_H
>>> +#define _PCI_CAP_H
>>> +
>>> +#include "types.h"
>>> +
>>> +/*
>>> +
>>> +QEMU-specific vendor(Red Hat)-specific capability.
>>> +It's intended to provide some hints for firmware to init PCI devices.
>>> +
>>> +Its is shown below:
>>> +
>>> +Header:
>>> +
>>> +u8 id;   Standard PCI Capability Header field
>>> +u8 next; Standard PCI Capability Header field
>>> +u8 len;  Standard PCI Capability Header field
>>> +u8 type; Red Hat vendor-specific capability type:
>>> +   now only REDHAT_QEMU_CAP 1 exists
>>> +Data:
>>> +
>>> +u16 non_prefetchable_16; non-prefetchable memory limit
>>> +
>>> +u8 bus_res;  minimum bus number to reserve;
>>> + this is necessary for PCI Express Root Ports
>>> + to support PCIE-to-PCI bridge hotplug
>>> +
>>> +u8 io_8; IO limit in case of 8-bit limit value
>>> +u32 io_32;   IO limit in case of 16-bit limit value
>>> + io_8 and io_16 are mutually exclusive, in other words,
>>> + they can't be non-zero simultaneously
>>> +
>>> +u32 prefetchable_32; non-prefetchable memory limit
>>> + in case of 32-bit limit value
>>> +u64 prefetchable_64; non-prefetchable memory limit
>>> + in case of 64-bit limit value
>>> + prefetachable_32 and prefetchable_64 are
>>> + mutually exclusive, in other words,
>>> + they can't be non-zero simultaneously
>>> +If any field in Data section is 0,
>>> +it means that such kind of reservation
>>> +is not needed.

I really don't like this 'mutually exclusive' fields approach because
IMHO it increases confusion level when undertanding this capability structure.
But - if we came to consensus on that, then IO fields should be used
in the same way,
because as I understand, this 'mutual exclusivity' serves to distinguish cases
when we employ only *_LIMIT register and both *_LIMIT an UPPER_*_LIMIT
registers.
And this is how both IO and PREFETCHABLE works, isn't it?

>>
>>
>
> Hi Michael,
>
>> We also want a way to say "no hint for this type".
>>
>> One way to achive this would be to have instead multiple
>> vendor specific capabilities, one for each of
>> bus#/io/mem/prefetch. 0 would mean do not reserve anything,
>> absence of capability would mean "no info, up to firmware".
>>
>
> First version of the series was implemented exactly like you propose,
> however Gerd preferred only one capability with multiple fields.
>
> I personally like the simplicity of vendor cap per io/mem/bus,
> even if it is on the expense of the limited PCI Config space.
>

Personally I agree with Marcel since all this fields express
reservations of some objects.

> We need a consensus here :)
>

Absolutely :)

> Thanks,
> Marcel
>
>
>>
>>> +
>>> +*/
>>> +
>>> +/* Offset of vendor-specific capability type field */
>>> +#define PCI_CAP_VNDR_SPEC_TYPE  3
>>
>>
>> This is a QEMU specific thing. Please name it as such.
>>
>>> +
>>> +/* List of valid Red Hat vendor-specific capability types */
>>> +#define REDHAT_CAP_TYPE_QEMU1
>>> +
>>> +
>>> +/* Offsets of QEMU capability fields */
>>> +#define QEMU_PCI_CAP_NON_PREF   4
>>> +#define QEMU_PCI_CAP_BUS_RES6
>>> +#define QEMU_PCI_CAP_IO_8   7
>>> +#define QEMU_PCI_CAP_IO_32  8
>>> +#define QEMU_PCI_CAP_PREF_3212
>>> +#define QEMU_PCI_CAP_PREF_6416
>>> +#define QEMU_PCI_CAP_SIZE   24
>>> +
>>> +#endif /* _PCI_CAP_H */
>>> --
>>> 2.7.4
>
>



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Marcel Apfelbaum 

On 31/07/2017 17:00, Michael S. Tsirkin wrote:

On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:

On PCI init PCI bridge devices may need some
extra info about bus number to reserve, IO, memory and
prefetchable memory limits. QEMU can provide this
with special vendor-specific PCI capability.

This capability is intended to be used only
for Red Hat PCI bridges, i.e. QEMU cooperation.

Signed-off-by: Aleksandr Bezzubikov 
---
  src/fw/dev-pci.h | 62 
  1 file changed, 62 insertions(+)
  create mode 100644 src/fw/dev-pci.h

diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
new file mode 100644
index 000..fbd49ed
--- /dev/null
+++ b/src/fw/dev-pci.h
@@ -0,0 +1,62 @@
+#ifndef _PCI_CAP_H
+#define _PCI_CAP_H
+
+#include "types.h"
+
+/*
+
+QEMU-specific vendor(Red Hat)-specific capability.
+It's intended to provide some hints for firmware to init PCI devices.
+
+Its is shown below:
+
+Header:
+
+u8 id;   Standard PCI Capability Header field
+u8 next; Standard PCI Capability Header field
+u8 len;  Standard PCI Capability Header field
+u8 type; Red Hat vendor-specific capability type:
+   now only REDHAT_QEMU_CAP 1 exists
+Data:
+
+u16 non_prefetchable_16; non-prefetchable memory limit
+
+u8 bus_res;  minimum bus number to reserve;
+ this is necessary for PCI Express Root Ports
+ to support PCIE-to-PCI bridge hotplug
+
+u8 io_8; IO limit in case of 8-bit limit value
+u32 io_32;   IO limit in case of 16-bit limit value
+ io_8 and io_16 are mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+
+u32 prefetchable_32; non-prefetchable memory limit
+ in case of 32-bit limit value
+u64 prefetchable_64; non-prefetchable memory limit
+ in case of 64-bit limit value
+ prefetachable_32 and prefetchable_64 are
+ mutually exclusive, in other words,
+ they can't be non-zero simultaneously
+If any field in Data section is 0,
+it means that such kind of reservation
+is not needed.




Hi Michael,


We also want a way to say "no hint for this type".

One way to achive this would be to have instead multiple
vendor specific capabilities, one for each of
bus#/io/mem/prefetch. 0 would mean do not reserve anything,
absence of capability would mean "no info, up to firmware".



First version of the series was implemented exactly like you propose,
however Gerd preferred only one capability with multiple fields.

I personally like the simplicity of vendor cap per io/mem/bus,
even if it is on the expense of the limited PCI Config space.

We need a consensus here :)

Thanks,
Marcel




+
+*/
+
+/* Offset of vendor-specific capability type field */
+#define PCI_CAP_VNDR_SPEC_TYPE  3


This is a QEMU specific thing. Please name it as such.


+
+/* List of valid Red Hat vendor-specific capability types */
+#define REDHAT_CAP_TYPE_QEMU1
+
+
+/* Offsets of QEMU capability fields */
+#define QEMU_PCI_CAP_NON_PREF   4
+#define QEMU_PCI_CAP_BUS_RES6
+#define QEMU_PCI_CAP_IO_8   7
+#define QEMU_PCI_CAP_IO_32  8
+#define QEMU_PCI_CAP_PREF_3212
+#define QEMU_PCI_CAP_PREF_6416
+#define QEMU_PCI_CAP_SIZE   24
+
+#endif /* _PCI_CAP_H */
--
2.7.4

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Michael S. Tsirkin 
On Sat, Jul 29, 2017 at 02:34:31AM +0300, Aleksandr Bezzubikov wrote:
> On PCI init PCI bridge devices may need some
> extra info about bus number to reserve, IO, memory and
> prefetchable memory limits. QEMU can provide this
> with special vendor-specific PCI capability.
> 
> This capability is intended to be used only
> for Red Hat PCI bridges, i.e. QEMU cooperation.
> 
> Signed-off-by: Aleksandr Bezzubikov 
> ---
>  src/fw/dev-pci.h | 62 
> 
>  1 file changed, 62 insertions(+)
>  create mode 100644 src/fw/dev-pci.h
> 
> diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
> new file mode 100644
> index 000..fbd49ed
> --- /dev/null
> +++ b/src/fw/dev-pci.h
> @@ -0,0 +1,62 @@
> +#ifndef _PCI_CAP_H
> +#define _PCI_CAP_H
> +
> +#include "types.h"
> +
> +/*
> +
> +QEMU-specific vendor(Red Hat)-specific capability.
> +It's intended to provide some hints for firmware to init PCI devices.
> +
> +Its is shown below:
> +
> +Header:
> +
> +u8 id;   Standard PCI Capability Header field
> +u8 next; Standard PCI Capability Header field
> +u8 len;  Standard PCI Capability Header field
> +u8 type; Red Hat vendor-specific capability type:
> +   now only REDHAT_QEMU_CAP 1 exists
> +Data:
> +
> +u16 non_prefetchable_16; non-prefetchable memory limit
> +
> +u8 bus_res;  minimum bus number to reserve;
> + this is necessary for PCI Express Root Ports
> + to support PCIE-to-PCI bridge hotplug
> +
> +u8 io_8; IO limit in case of 8-bit limit value
> +u32 io_32;   IO limit in case of 16-bit limit value
> + io_8 and io_16 are mutually exclusive, in other words,
> + they can't be non-zero simultaneously
> +
> +u32 prefetchable_32; non-prefetchable memory limit
> + in case of 32-bit limit value
> +u64 prefetchable_64; non-prefetchable memory limit
> + in case of 64-bit limit value
> + prefetachable_32 and prefetchable_64 are
> + mutually exclusive, in other words,
> + they can't be non-zero simultaneously
> +If any field in Data section is 0,
> +it means that such kind of reservation
> +is not needed.

We also want a way to say "no hint for this type".

One way to achive this would be to have instead multiple
vendor specific capabilities, one for each of
bus#/io/mem/prefetch. 0 would mean do not reserve anything,
absence of capability would mean "no info, up to firmware".


> +
> +*/
> +
> +/* Offset of vendor-specific capability type field */
> +#define PCI_CAP_VNDR_SPEC_TYPE  3

This is a QEMU specific thing. Please name it as such.

> +
> +/* List of valid Red Hat vendor-specific capability types */
> +#define REDHAT_CAP_TYPE_QEMU1
> +
> +
> +/* Offsets of QEMU capability fields */
> +#define QEMU_PCI_CAP_NON_PREF   4
> +#define QEMU_PCI_CAP_BUS_RES6
> +#define QEMU_PCI_CAP_IO_8   7
> +#define QEMU_PCI_CAP_IO_32  8
> +#define QEMU_PCI_CAP_PREF_3212
> +#define QEMU_PCI_CAP_PREF_6416
> +#define QEMU_PCI_CAP_SIZE   24
> +
> +#endif /* _PCI_CAP_H */
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v3 2/3] pci: add QEMU-specific PCI capability structure

2017-07-31 Thread Marcel Apfelbaum 

On 29/07/2017 2:34, Aleksandr Bezzubikov wrote:

On PCI init PCI bridge devices may need some
extra info about bus number to reserve, IO, memory and
prefetchable memory limits. QEMU can provide this
with special vendor-specific PCI capability.

This capability is intended to be used only
for Red Hat PCI bridges, i.e. QEMU cooperation.

Signed-off-by: Aleksandr Bezzubikov 
---
  src/fw/dev-pci.h | 62 
  1 file changed, 62 insertions(+)
  create mode 100644 src/fw/dev-pci.h

diff --git a/src/fw/dev-pci.h b/src/fw/dev-pci.h
new file mode 100644
index 000..fbd49ed
--- /dev/null
+++ b/src/fw/dev-pci.h
@@ -0,0 +1,62 @@
+#ifndef _PCI_CAP_H
+#define _PCI_CAP_H
+
+#include "types.h"
+
+/*
+


Hi Aleksander,


+QEMU-specific vendor(Red Hat)-specific capability.
+It's intended to provide some hints for firmware to init PCI devices.
+
+Its is shown below:
+
+Header:
+
+u8 id;   Standard PCI Capability Header field
+u8 next; Standard PCI Capability Header field
+u8 len;  Standard PCI Capability Header field
+u8 type; Red Hat vendor-specific capability type:
+   now only REDHAT_QEMU_CAP 1 exists
+Data:
+
+u16 non_prefetchable_16; non-prefetchable memory limit
+


Maybe we should name it "mem". And if I remember right Gerd
suggested keeping them all 32 bits:

u32 mem_res


+u8 bus_res;  minimum bus number to reserve;
+ this is necessary for PCI Express Root Ports
+ to support PCIE-to-PCI bridge hotplug
+
+u8 io_8; IO limit in case of 8-bit limit value


I must have missed it, but why do we need io_8 field?


+u32 io_32;   IO limit in case of 16-bit limit value
+ io_8 and io_16 are mutually exclusive, in other words,
+ they can't be non-zero simultaneously


I don't see any io_16 field.
Maybe only one field:
  u32 io_res


+
+u32 prefetchable_32; non-prefetchable memory limit
+ in case of 32-bit limit value


Name and comment mismatch


+u64 prefetchable_64; non-prefetchable memory limit
+ in case of 64-bit limit value
+ prefetachable_32 and prefetchable_64 are
+ mutually exclusive, in other words,
+ they can't be non-zero simultaneously



Name and comment mismatch

It should look like:
- u32 bus_res
- u32 io_res
- u32 mem_res,
- u32 mem_prefetchable_32,
- u64 mem_prefetchable_64, (mutually exclusive with the above)

Does it look right to all?


+If any field in Data section is 0,
+it means that such kind of reservation
+is not needed.
+
+*/
+
+/* Offset of vendor-specific capability type field */
+#define PCI_CAP_VNDR_SPEC_TYPE  3
+
+/* List of valid Red Hat vendor-specific capability types */
+#define REDHAT_CAP_TYPE_QEMU1


Maybe we should be more concrete:
  REDHAT_CAP_TYPE_RES_RESERVE


+
+
+/* Offsets of QEMU capability fields */
+#define QEMU_PCI_CAP_NON_PREF   4
+#define QEMU_PCI_CAP_BUS_RES6
+#define QEMU_PCI_CAP_IO_8   7
+#define QEMU_PCI_CAP_IO_32  8
+#define QEMU_PCI_CAP_PREF_3212
+#define QEMU_PCI_CAP_PREF_6416
+#define QEMU_PCI_CAP_SIZE   24
+
+#endif /* _PCI_CAP_H */



I know the exact layout is less important for your current
project, but is important to get it right the first time.

Thanks,
Marcel