On 12/05/2020 16:44, Alexandru Elisei wrote:
Hi,
> On 5/12/20 3:17 PM, André Przywara wrote:
>> On 06/05/2020 14:51, Alexandru Elisei wrote:
>>
>> Hi,
>>
>>> On 4/6/20 3:06 PM, André Przywara wrote:
>>>> On 26/03/2020 15:24, Alexandru Elisei wrote:
>>>>
>>>>>
>>>>> union pci_config_address {
>>>>> struct {
>>>>> @@ -58,6 +91,8 @@ union pci_config_address {
>>>>> };
>>>>> u32 w;
>>>>> };
>>>>> +#endif
>>>>> +#define PCI_DEV_CFG_MASK (PCI_DEV_CFG_SIZE - 1)
>>>>>
>>>>> struct msix_table {
>>>>> struct msi_msg msg;
>>>>> @@ -100,6 +135,33 @@ struct pci_cap_hdr {
>>>>> u8 next;
>>>>> };
>>>>>
>>>>> +struct pcie_cap {
>>>> I guess this is meant to map to the PCI Express Capability Structure as
>>>> described in the PCIe spec?
>>>> We would need to add the "packed" attribute then. But actually I am not
>>>> a fan of using C language constructs to model specified register
>>>> arrangements, the kernel tries to avoid this as well.
>>> I'm not sure what you are suggesting. Should we rewrite the entire PCI emulation
>>> code in kvmtool then?
>> At least not add more of that problematic code, especially if we don't
>
> I don't see why how the code is problematic. Did I miss it in a previous comment?
I was referring to that point that modelling hardware defined registers
using a C struct is somewhat dodgy. As the C standard says, in section
6.7.2.1, end of paragraph 15:
"There may be unnamed padding within a structure object, but not at its
beginning."
Yes, GCC and other implementations offer "packed" to somewhat overcome
this, but this is a compiler extension and has other issues, like if you
have non-aligned members and take pointers to it.
I think our case here is more sane, since we always seem to use it on
normal memory (do we?), and are not mapping it to some actual device memory.
So I leave this up to you, but I am still opposed to the idea of adding
code that is not used.
Cheers,
Andre.
>> need it. Maybe there is a better solution for the operations we will
>> need (byte array?), that's hard to say without seeing the code.
>>
>>>> Actually, looking closer: why do we need this in the first place? I
>>>> removed this and struct pm_cap, and it still compiles.
>>>> So can we lose those two structures at all? And move the discussion and
>>>> implementation (for VirtIO 1.0?) to a later series?
>>> I've answered both points in v2 of the series [1].
>>>
>>> [1] https://www.spinics.net/lists/kvm/msg209601.html:
>> From there:
>>>> But more importantly: Do we actually need those definitions? We
>>>> don't seem to use them, do we?
>>>> And the u8 __pad[PCI_DEV_CFG_SIZE] below should provide the extended
>>>> storage space a guest would expect?
>>> Yes, we don't use them for the reasons I explained in the commit
>>> message. I would rather keep them, because they are required by the
>>> PCIE spec.
>> I don't get the point of adding code / data structures that we don't
>> need, especially if it has issues. I understand it's mandatory as per
>> the spec, but just adding a struct here doesn't fix this or makes this
>> better.
>
> Sure, I can remove the unused structs, especially if they have issues. But I don't
> see what issues they have, would you mind expanding on that?
The best code is the one not written. ;-)
Cheers,
Andre
